From 285eb00655d5498b1e18da52983b8381119ba728 Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 25 Jun 2026 06:55:06 +0000 Subject: [PATCH] feat(#269): separate agent and bottle selection at launch time - `bottle:` in agent frontmatter is now optional; agents without it are portable and require bottles to be selected at launch. - Adds `filter_multiselect` to `tui.py`: multi-select picker with ordered selection list, Space/Enter to toggle, Ctrl-D to confirm. - `ManifestIndex` gains `all_bottle_names` and `load_for_agent` accepts `bottle_names: tuple[str, ...]` to merge bottles in order at runtime. - `merge_bottles_runtime` in `manifest_extends.py` applies the same field-merge rules as `extends:` to pre-resolved bottle objects. - `BottleSpec` gains `bottle_names`; `_validate` and `write_launch_metadata` thread it through so `resume` replays the same bottle configuration. - `cmd_start` shows the bottle multiselect after agent selection, pre-populated from the agent's `bottle:` field when present. - Existing agents with `bottle:` declared continue to work unchanged. --- bot_bottle/backend/__init__.py | 16 +- bot_bottle/backend/resolve_common.py | 1 + bot_bottle/bottle_state.py | 9 + bot_bottle/cli/resume.py | 1 + bot_bottle/cli/start.py | 47 +++++ bot_bottle/cli/tui.py | 209 +++++++++++++++++++++++ bot_bottle/manifest.py | 115 +++++++++++-- bot_bottle/manifest_agent.py | 29 ++-- bot_bottle/manifest_extends.py | 52 ++++++ bot_bottle/manifest_loader.py | 19 +++ bot_bottle/manifest_schema.py | 4 +- tests/unit/test_cli_start_selector.py | 154 ++++++++++++----- tests/unit/test_cli_tui.py | 15 +- tests/unit/test_manifest_bottle_merge.py | 200 ++++++++++++++++++++++ 14 files changed, 791 insertions(+), 80 deletions(-) create mode 100644 tests/unit/test_manifest_bottle_merge.py diff --git a/bot_bottle/backend/__init__.py b/bot_bottle/backend/__init__.py index 06b279e..0454346 100644 --- a/bot_bottle/backend/__init__.py +++ b/bot_bottle/backend/__init__.py @@ -72,6 +72,9 @@ class BottleSpec: identity: str = "" label: str = "" color: str = "" + # Ordered bottle names selected at launch (issue #269). When non-empty + # they are merged in order and replace the agent's `bottle:` field. + bottle_names: tuple[str, ...] = () @dataclass(frozen=True) @@ -130,7 +133,11 @@ class BottlePlan(ABC): info(f"provider : {self.agent_provision.template}") print_multi("env ", env_names) print_multi("skills ", list(agent.skills)) - info(f"bottle : {agent.bottle}") + effective_bottles = ( + list(spec.bottle_names) if spec.bottle_names + else ([agent.bottle] if agent.bottle else []) + ) + print_multi("bottle ", effective_bottles) identity = manifest.git_identity_summary() if identity: @@ -364,7 +371,7 @@ class BottleBackend(ABC, Generic[PlanT, CleanupT]): Returns the loaded Manifest for the selected agent. Subclasses with additional preconditions should override and call `super()._validate(spec)` first.""" - manifest = spec.manifest.load_for_agent(spec.agent_name) + manifest = spec.manifest.load_for_agent(spec.agent_name, spec.bottle_names) self._validate_skills(manifest.agent.skills) self._validate_agent_provider_dockerfile(spec, manifest) return manifest @@ -390,9 +397,12 @@ class BottleBackend(ABC, Generic[PlanT, CleanupT]): if not path.is_absolute(): path = Path(spec.user_cwd) / path if not path.is_file(): + effective = ( + ", ".join(spec.bottle_names) if spec.bottle_names else manifest.agent.bottle + ) die( f"agent_provider.dockerfile for bottle " - f"'{manifest.agent.bottle}' not found: {path}" + f"'{effective}' not found: {path}" ) @abstractmethod diff --git a/bot_bottle/backend/resolve_common.py b/bot_bottle/backend/resolve_common.py index cb9322f..d1ad0dc 100644 --- a/bot_bottle/backend/resolve_common.py +++ b/bot_bottle/backend/resolve_common.py @@ -63,6 +63,7 @@ def write_launch_metadata( backend=backend, label=spec.label, color=spec.color, + bottle_names=spec.bottle_names, )) diff --git a/bot_bottle/bottle_state.py b/bot_bottle/bottle_state.py index 93b671a..63fe2ef 100644 --- a/bot_bottle/bottle_state.py +++ b/bot_bottle/bottle_state.py @@ -112,6 +112,10 @@ class BottleMetadata: backend: str = "" label: str = "" color: str = "" + # Ordered bottle names selected at launch (issue #269). Empty tuple + # for state dirs written before this change; resume falls back to + # the agent's `bottle:` field in that case. + bottle_names: tuple[str, ...] = () def metadata_path(identity: str) -> Path: @@ -139,6 +143,10 @@ def read_metadata(identity: str) -> BottleMetadata | None: if not isinstance(raw, dict): return None raw_typed = cast(dict[str, object], raw) + raw_bottle_names = raw_typed.get("bottle_names", []) + bottle_names: tuple[str, ...] = () + if isinstance(raw_bottle_names, list): + bottle_names = tuple(str(n) for n in raw_bottle_names if isinstance(n, str)) return BottleMetadata( identity=str(raw_typed.get("identity", identity)), agent_name=str(raw_typed.get("agent_name", "")), @@ -149,6 +157,7 @@ def read_metadata(identity: str) -> BottleMetadata | None: backend=str(raw_typed.get("backend", "")), label=str(raw_typed.get("label", "")), color=str(raw_typed.get("color", "")), + bottle_names=bottle_names, ) diff --git a/bot_bottle/cli/resume.py b/bot_bottle/cli/resume.py index 557c293..490f000 100644 --- a/bot_bottle/cli/resume.py +++ b/bot_bottle/cli/resume.py @@ -51,6 +51,7 @@ def cmd_resume(argv: list[str]) -> int: copy_cwd=metadata.copy_cwd, user_cwd=metadata.cwd or USER_CWD, identity=metadata.identity, + bottle_names=tuple(metadata.bottle_names), ) backend_name = metadata.backend or None return _launch_bottle( diff --git a/bot_bottle/cli/start.py b/bot_bottle/cli/start.py index c752cfc..6781aba 100644 --- a/bot_bottle/cli/start.py +++ b/bot_bottle/cli/start.py @@ -75,6 +75,20 @@ def cmd_start(argv: list[str]) -> int: backend_name: str | None = args.backend + # Bottle multiselect: always show after agent selection so operators + # can compose bottles at launch time without editing agent manifests. + available_bottles = manifest.all_bottle_names + initial_bottle = _peek_agent_bottle(manifest, agent_name) + initial_bottles = [initial_bottle] if initial_bottle else [] + selected_bottles = tui.filter_multiselect( + available_bottles, + title="Select bottles", + initial=initial_bottles, + ) + if selected_bottles is None: + return 0 + bottle_names = tuple(selected_bottles) + label, color = tui.name_color_modal(default_label=agent_name) label, color = _resolve_unique_label(label, color) @@ -85,6 +99,7 @@ def cmd_start(argv: list[str]) -> int: user_cwd=USER_CWD, label=label, color=color, + bottle_names=bottle_names, ) return _launch_bottle( spec, @@ -194,6 +209,38 @@ def _identity_from_plan(plan: object) -> str: return getattr(plan, "slug", "") +def _peek_agent_bottle(manifest: ManifestIndex, agent_name: str) -> str: + """Return the `bottle:` value from the named agent's frontmatter without + fully parsing the agent file, or "" when absent or unreadable. + + Used to pre-populate the bottle multiselect with the agent's default + bottle so operators who haven't removed `bottle:` from their manifests + don't need to re-select it every time.""" + if manifest.home_md is None: + # Eager mode (from_json_obj): agent is pre-parsed. + if agent_name in manifest.agents: + return manifest.agents[agent_name].bottle + return "" + + from ..manifest_loader import scan_agent_names + from ..yaml_subset import YamlSubsetError, parse_frontmatter + + home_agents = scan_agent_names(manifest.home_md / "agents") + cwd_agents: dict[str, Path] = {} + if manifest.cwd_md is not None: + cwd_agents = scan_agent_names(manifest.cwd_md / "agents") + merged = {**home_agents, **cwd_agents} + path = merged.get(agent_name) + if path is None: + return "" + try: + fm, _ = parse_frontmatter(path.read_text()) + bottle = fm.get("bottle", "") + return str(bottle) if isinstance(bottle, str) else "" + except (OSError, YamlSubsetError): + return "" + + def _resolve_unique_label(label: str, color: str) -> tuple[str, str]: """Re-prompt with a disclaimer until the label's slug is not already in use among running bottles. Passes through unchanged when no diff --git a/bot_bottle/cli/tui.py b/bot_bottle/cli/tui.py index 57cced3..08f14b2 100644 --- a/bot_bottle/cli/tui.py +++ b/bot_bottle/cli/tui.py @@ -17,6 +17,42 @@ import sys from typing import Any, Optional +def filter_multiselect( + items: list[str], + *, + title: str = "", + initial: Optional[list[str]] = None, + tty_path: str = "/dev/tty", +) -> Optional[list[str]]: + """Render a multi-select picker over *items*. + + Returns the ordered list of selected items, or ``None`` if the user + cancelled (Esc / ``q`` / Ctrl-C / Ctrl-D with no items). + + Press Space or Enter to toggle the item under the cursor. + Press Ctrl-D to confirm the current selection (returns even if empty). + Press Esc/q to cancel (returns None). + + *initial* pre-populates the selection in insertion order. Items + added are appended; removed items leave the remaining order unchanged. + """ + if not items: + return [] + + try: + tty_fd = open(tty_path, "r+b", buffering=0) + except OSError: + return None + + try: + fd_dup = os.dup(tty_fd.fileno()) + return _run_multiselect( + items, title=title, initial=list(initial or []), tty_fd=fd_dup + ) + finally: + tty_fd.close() + + def filter_select( items: list[str], *, @@ -221,6 +257,179 @@ def _addstr_safe(screen: Any, row: int, col: int, text: str, attr: int = curses. pass +# --------------------------------------------------------------------------- +# filter_multiselect internals +# --------------------------------------------------------------------------- + +_KEY_SPACE = 32 + + +def _run_multiselect( + items: list[str], *, title: str, initial: list[str], tty_fd: int +) -> Optional[list[str]]: + """Drive a curses multi-select session on *tty_fd*.""" + os.environ.setdefault("TERM", "xterm-256color") + + orig_stdin = sys.__stdin__ + orig_stdout = sys.__stdout__ + + try: + import io + tty_text = io.TextIOWrapper(io.FileIO(tty_fd, mode='r+'), write_through=True) + sys.__stdin__ = tty_text # type: ignore[assignment] + sys.__stdout__ = tty_text # type: ignore[assignment] + + screen = curses.initscr() + curses.noecho() + curses.cbreak() + screen.keypad(True) + + try: + result = _multiselect_loop(screen, items, title=title, initial=initial) + finally: + screen.keypad(False) + curses.nocbreak() + curses.echo() + curses.endwin() + except Exception: # noqa: W0718 + return None + finally: + sys.__stdin__ = orig_stdin # type: ignore[assignment] + sys.__stdout__ = orig_stdout # type: ignore[assignment] + + return result + + +def _multiselect_loop( + screen: Any, items: list[str], *, title: str, initial: list[str] +) -> Optional[list[str]]: + query = "" + cursor = 0 + selected: list[str] = [s for s in initial if s in items] + + while True: + filtered = _filter_items(items, query) + + if not filtered: + cursor = 0 + elif cursor >= len(filtered): + cursor = len(filtered) - 1 + + try: + _render_multiselect(screen, filtered, cursor, query=query, title=title, selected=selected) + except curses.error: + return None + + try: + key = screen.getch() + except KeyboardInterrupt: + return None + + if key in (_KEY_ESC, _KEY_CTRL_C, ord("q")): + return None + + if key == _KEY_CTRL_D: + return list(selected) + + if key in (curses.KEY_ENTER, _KEY_ENTER_ALT, ord("\r"), _KEY_SPACE): + if filtered: + item = filtered[cursor] + if item in selected: + selected.remove(item) + else: + selected.append(item) + + elif key in (curses.KEY_UP, ord("k")): + if cursor > 0: + cursor -= 1 + + elif key in (curses.KEY_DOWN, ord("j")): + if cursor < len(filtered) - 1: + cursor += 1 + + elif key in (curses.KEY_BACKSPACE, _KEY_BACKSPACE_WIN, 127): + query = query[:-1] + new_filtered = _filter_items(items, query) + if cursor >= len(new_filtered): + cursor = max(0, len(new_filtered) - 1) + + elif 32 <= key <= 126 and key != _KEY_SPACE: + query += chr(key) + cursor = 0 + + +def _render_multiselect( + screen: Any, + filtered: list[str], + cursor: int, + *, + query: str, + title: str, + selected: list[str], +) -> None: + screen.erase() + rows, cols = screen.getmaxyx() + min_rows = 7 + + if rows < min_rows: + raise curses.error("terminal too small") + + row = 0 + + if title and row < rows - 1: + _addstr_safe(screen, row, 0, title[:cols - 1], curses.A_BOLD) + row += 1 + + filter_label = f"Filter: {query}" + if row < rows - 1: + _addstr_safe(screen, row, 0, filter_label[:cols - 1]) + row += 1 + + sep = "─" * min(cols - 1, 40) + if row < rows - 1: + _addstr_safe(screen, row, 0, sep) + row += 1 + + # Reserve rows for: sep + selected-line + sep + help-line = 4 + list_start = row + list_rows = rows - list_start - 4 + if list_rows < 1: + return + + selected_set = set(selected) + scroll = max(0, cursor - list_rows + 1) + visible = filtered[scroll: scroll + list_rows] + + for idx, item in enumerate(visible): + abs_idx = scroll + idx + mark = "[*]" if item in selected_set else "[ ]" + prefix = "> " if abs_idx == cursor else " " + line = (prefix + mark + " " + item)[:cols - 1] + attr = curses.A_REVERSE if abs_idx == cursor else curses.A_NORMAL + if row < rows - 4: + _addstr_safe(screen, row, 0, line, attr) + row += 1 + + if row < rows - 3: + _addstr_safe(screen, row, 0, sep) + row += 1 + + selected_summary = "Selected (in order): " + (", ".join(selected) if selected else "(none)") + if row < rows - 2: + _addstr_safe(screen, row, 0, selected_summary[:cols - 1]) + row += 1 + + if row < rows - 1: + _addstr_safe(screen, row, 0, sep) + row += 1 + + help_line = "[↑↓/jk] move [Space/Enter] toggle [Ctrl-D] done [Esc/q] cancel" + if row < rows: + _addstr_safe(screen, min(rows - 1, row), 0, help_line[:cols - 1]) + + screen.refresh() + + # --------------------------------------------------------------------------- # name_color_modal — two-step label + color picker # --------------------------------------------------------------------------- diff --git a/bot_bottle/manifest.py b/bot_bottle/manifest.py index 72192e2..f053b47 100644 --- a/bot_bottle/manifest.py +++ b/bot_bottle/manifest.py @@ -215,6 +215,65 @@ def _merge_git_user( ) +def _resolve_effective_bottle_eager( + agent_name: str, + agent: "ManifestAgent", + bottle_names: "tuple[str, ...]", + bottles: "Mapping[str, ManifestBottle]", +) -> "ManifestBottle": + """Return the effective ManifestBottle for the eager (from_json_obj) path. + + When bottle_names is non-empty they are merged in order. When empty, falls + back to agent.bottle. Raises ManifestError when neither is set.""" + from .manifest_extends import merge_bottles_runtime + + if bottle_names: + resolved: list[ManifestBottle] = [] + for bn in bottle_names: + if bn not in bottles: + available = ", ".join(sorted(bottles.keys())) or "(none)" + raise ManifestError( + f"bottle '{bn}' not defined. Available: {available}" + ) + resolved.append(bottles[bn]) + return merge_bottles_runtime(resolved) + + if not agent.bottle: + raise ManifestError( + f"agent '{agent_name}' has no 'bottle' field and no bottles were " + f"selected at launch. Select at least one bottle or add " + f"'bottle: ' to the agent manifest." + ) + return bottles[agent.bottle] + + +def _resolve_effective_bottle_lazy( + agent_name: str, + agent_bottle: str, + bottle_names: "tuple[str, ...]", + bottles_dir: "Path", +) -> "ManifestBottle": + """Return the effective ManifestBottle for the lazy (from_md_dirs) path. + + When bottle_names is non-empty they are resolved from disk and merged in + order. When empty, falls back to agent_bottle. Raises ManifestError when + neither is set.""" + from .manifest_extends import merge_bottles_runtime + from .manifest_loader import load_bottle_chain_from_dir + + if bottle_names: + resolved = [load_bottle_chain_from_dir(bn, bottles_dir) for bn in bottle_names] + return merge_bottles_runtime(resolved) + + if not agent_bottle: + raise ManifestError( + f"agent '{agent_name}' has no 'bottle' field and no bottles were " + f"selected at launch. Select at least one bottle or add " + f"'bottle: ' to the agent manifest." + ) + return load_bottle_chain_from_dir(agent_bottle, bottles_dir) + + @dataclass(frozen=True) class Manifest: """Single-agent/bottle value type. Returned by ManifestIndex.load_for_agent(). @@ -360,6 +419,18 @@ class ManifestIndex: } return cls(bottles=bottles, agents=agents) + @property + def all_bottle_names(self) -> list[str]: + """Sorted list of all discoverable bottle names. + + In names-only mode (from resolve/from_md_dirs) this scans bottle + filenames without reading their content. In eager mode (from + from_json_obj) it returns the pre-parsed bottles' names.""" + if self.home_md is not None: + from .manifest_loader import scan_bottle_names + return scan_bottle_names(self.home_md / "bottles") + return sorted(self.bottles.keys()) + @property def all_agent_names(self) -> list[str]: """Sorted list of all discoverable agent names. @@ -376,9 +447,18 @@ class ManifestIndex: return sorted(home_names | cwd_names) return sorted(self.agents.keys()) - def load_for_agent(self, agent_name: str) -> "Manifest": + def load_for_agent( + self, + agent_name: str, + bottle_names: "tuple[str, ...] | None" = None, + ) -> "Manifest": """Parse the named agent and its bottle; return a single-value Manifest. + `bottle_names` is an ordered list of bottles selected at launch time. + When non-empty they are resolved and merged in order (index 0 = base; + later entries override). When empty or None, falls back to the agent's + own `bottle:` field. Raises ManifestError when neither is set. + In lazy mode (from resolve/from_md_dirs) the agent file and its bottle chain are read from disk for the first time here. In eager mode (from_json_obj) the data is already parsed; this just filters @@ -389,6 +469,8 @@ class ManifestIndex: Always raises ManifestError if the agent is unknown or invalid. Backends call this at preflight inside _validate.""" + effective_bottle_names: tuple[str, ...] = bottle_names or () + if self.home_md is None: # Eager manifest (from_json_obj): data already parsed; filter to # the one requested agent and its bottle so the returned Manifest @@ -399,7 +481,9 @@ class ManifestIndex: f"agent '{agent_name}' not defined. Available: {available}" ) agent = self.agents[agent_name] - raw_bottle = self.bottles[agent.bottle] + raw_bottle = _resolve_effective_bottle_eager( + agent_name, agent, effective_bottle_names, self.bottles + ) merged = _merge_git_user(agent.git_user, raw_bottle.git_user) bottle = raw_bottle if merged == raw_bottle.git_user else replace(raw_bottle, git_user=merged) return Manifest(agent=agent, bottle=bottle) @@ -431,26 +515,31 @@ class ManifestIndex: validate_agent_frontmatter_keys(agent_path, fm.keys()) - bottle_name = fm.get("bottle") - if not isinstance(bottle_name, str) or not bottle_name: - raise ManifestError( - f"agent '{agent_name}' must declare a 'bottle' field " - f"naming a defined bottle" - ) - - # Load the bottle chain (may raise ManifestError). + # Determine the effective bottle name(s). + agent_bottle = fm.get("bottle") or "" bottles_dir = self.home_md / "bottles" - raw_bottle = load_bottle_chain_from_dir(bottle_name, bottles_dir) + raw_bottle = _resolve_effective_bottle_lazy( + agent_name, str(agent_bottle), effective_bottle_names, bottles_dir + ) + effective_bottle_name = ( + effective_bottle_names[-1] if effective_bottle_names + else str(agent_bottle) + ) # Build and validate the full ManifestAgent. agent_dict: dict[str, object] = { - "bottle": bottle_name, "skills": fm.get("skills", []), "prompt": body.strip(), } + if agent_bottle: + agent_dict["bottle"] = agent_bottle if "git-gate" in fm: agent_dict["git-gate"] = fm["git-gate"] - agent = ManifestAgent.from_dict(agent_name, agent_dict, {bottle_name}) + # Pass the effective bottle name as the known-bottles set so agents + # that have bottle: set are validated; agents without bottle: pass {} + # since bottle_names were already resolved above. + known = {effective_bottle_name} if effective_bottle_name else set() + agent = ManifestAgent.from_dict(agent_name, agent_dict, known) merged_user = _merge_git_user(agent.git_user, raw_bottle.git_user) bottle = raw_bottle if merged_user == raw_bottle.git_user else replace(raw_bottle, git_user=merged_user) diff --git a/bot_bottle/manifest_agent.py b/bot_bottle/manifest_agent.py index 0ebfd42..0c7f9b2 100644 --- a/bot_bottle/manifest_agent.py +++ b/bot_bottle/manifest_agent.py @@ -109,7 +109,8 @@ class ManifestAgentProvider: @dataclass(frozen=True) class ManifestAgent: - bottle: str + # Optional: when empty the operator selects bottles at launch time. + bottle: str = "" skills: tuple[str, ...] = () prompt: str = "" # Per-agent git identity (issue #94). Overlays the referenced @@ -129,18 +130,20 @@ class ManifestAgent: f"allowed keys are {allowed}." ) - bottle = d.get("bottle") - if not isinstance(bottle, str) or not bottle: - raise ManifestError( - f"agent '{name}' must declare a 'bottle' field naming a " - f"defined bottle" - ) - if bottle not in bottle_names: - available = ", ".join(sorted(bottle_names)) or "(none defined)" - raise ManifestError( - f"agent '{name}' references bottle '{bottle}', which is not defined. " - f"Available: {available}" - ) + bottle_raw = d.get("bottle") + bottle = "" + if bottle_raw is not None: + if not isinstance(bottle_raw, str) or not bottle_raw: + raise ManifestError( + f"agent '{name}' bottle must be a non-empty string when declared" + ) + if bottle_raw not in bottle_names: + available = ", ".join(sorted(bottle_names)) or "(none defined)" + raise ManifestError( + f"agent '{name}' references bottle '{bottle_raw}', which is not defined. " + f"Available: {available}" + ) + bottle = bottle_raw skills: tuple[str, ...] = () skills_raw = d.get("skills") diff --git a/bot_bottle/manifest_extends.py b/bot_bottle/manifest_extends.py index 3432f64..68c2827 100644 --- a/bot_bottle/manifest_extends.py +++ b/bot_bottle/manifest_extends.py @@ -9,6 +9,58 @@ if TYPE_CHECKING: from .manifest_egress import ManifestEgressConfig +def merge_bottles_runtime(bottles: "list[ManifestBottle]") -> "ManifestBottle": + """Merge an ordered list of pre-resolved ManifestBottle objects. + + Index 0 is the base; each subsequent entry is applied on top using + the same field-merge rules as the file-based extends machinery: + env: dict merge, later wins; git_user: per-field overlay, later + wins on non-empty; git (repos): union by name, later wins; egress + routes: concatenate; agent_provider, supervise: later replaces. + """ + if not bottles: + raise ValueError("merge_bottles_runtime requires at least one bottle") + result = bottles[0] + for override in bottles[1:]: + result = _merge_two_bottles_runtime(result, override) + return result + + +def _merge_two_bottles_runtime(base: "ManifestBottle", override: "ManifestBottle") -> "ManifestBottle": + from .manifest import ManifestBottle, ManifestGitUser + from .manifest_egress import ManifestEgressConfig + + merged_env = {**base.env, **override.env} + + merged_git_user = ManifestGitUser( + name=override.git_user.name or base.git_user.name, + email=override.git_user.email or base.git_user.email, + ) + + # git repos: union keyed by Name, override wins per-name. + base_repos_by_name = {entry.Name: entry for entry in base.git} + override_repos_by_name = {entry.Name: entry for entry in override.git} + merged_repos_names = list(base_repos_by_name) + [ + n for n in override_repos_by_name if n not in base_repos_by_name + ] + merged_git = tuple( + override_repos_by_name.get(n, base_repos_by_name[n]) + for n in merged_repos_names + ) + + merged_routes: tuple[ManifestEgressRoute, ...] = base.egress.routes + override.egress.routes + merged_egress = ManifestEgressConfig(routes=merged_routes, Log=override.egress.Log) + + return ManifestBottle( + env=merged_env, + agent_provider=override.agent_provider, + git=merged_git, + git_user=merged_git_user, + egress=merged_egress, + supervise=override.supervise, + ) + + def resolve_bottles(raws: dict[str, dict[str, object]]) -> dict[str, ManifestBottle]: """Apply `extends:` chains and return resolved ManifestBottle objects.""" cache: dict[str, ManifestBottle] = {} diff --git a/bot_bottle/manifest_loader.py b/bot_bottle/manifest_loader.py index 6ce632a..1c2ae71 100644 --- a/bot_bottle/manifest_loader.py +++ b/bot_bottle/manifest_loader.py @@ -32,6 +32,25 @@ def check_stale_json(dir_path: Path, md_dir: Path, label: str) -> None: ) +def scan_bottle_names(bottles_dir: Path) -> list[str]: + """Scan `/*.md` for valid filenames and return sorted bottle names. + + No file content is read. Invalid filenames are skipped with a warning.""" + result: list[str] = [] + if not bottles_dir.is_dir(): + return result + for path in sorted(bottles_dir.glob("*.md")): + name = entity_name_from_path(path) + if name is None: + warn( + f"skipping {path}: filename must match " + f"[a-z][a-z0-9-]*.md (got {path.name!r})" + ) + continue + result.append(name) + return result + + def scan_agent_names(agents_dir: Path) -> dict[str, Path]: """Scan `/*.md` for valid filenames and return `{name: path}`. diff --git a/bot_bottle/manifest_schema.py b/bot_bottle/manifest_schema.py index 1925cf8..13c86bf 100644 --- a/bot_bottle/manifest_schema.py +++ b/bot_bottle/manifest_schema.py @@ -18,8 +18,8 @@ _FILENAME_RX = re.compile(r"^[a-z][a-z0-9-]*$") BOTTLE_KEYS = frozenset( {"env", "extends", "agent_provider", "git-gate", "egress", "supervise"} ) -AGENT_KEYS_REQUIRED = frozenset({"bottle"}) -AGENT_KEYS_OPTIONAL = frozenset({"skills", "git-gate"}) +AGENT_KEYS_REQUIRED: frozenset[str] = frozenset() +AGENT_KEYS_OPTIONAL = frozenset({"bottle", "skills", "git-gate"}) # Claude Code subagent fields bot-bottle ignores at launch but does # not reject. This lets the same file double as diff --git a/tests/unit/test_cli_start_selector.py b/tests/unit/test_cli_start_selector.py index 928652f..57b9700 100644 --- a/tests/unit/test_cli_start_selector.py +++ b/tests/unit/test_cli_start_selector.py @@ -1,7 +1,8 @@ -"""Unit: cmd_start selector dispatch (PRD 0051). +"""Unit: cmd_start selector dispatch (PRD 0051, issue #269). Tests that cmd_start calls filter_select only when the agent name is -absent, skips it when the agent is explicit, and returns 0 on cancel. +absent, shows the bottle multiselect after agent selection, and skips +pickers when both are explicitly set. All actual launch work is stubbed so no container is created. """ @@ -10,17 +11,23 @@ from __future__ import annotations import os import unittest -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, call, patch import bot_bottle.cli.start as start_mod import bot_bottle.cli.tui as tui_mod from bot_bottle.backend import ActiveAgent -def _make_manifest(agent_names: list[str]): +def _make_manifest( + agent_names: list[str], + bottle_names: list[str] | None = None, + agent_bottle: str = "", +): manifest = MagicMock() - manifest.agents = {name: MagicMock() for name in agent_names} + manifest.agents = {name: MagicMock(bottle=agent_bottle) for name in agent_names} manifest.all_agent_names = sorted(agent_names) + manifest.all_bottle_names = sorted(bottle_names or []) + manifest.home_md = None # eager mode so _peek_agent_bottle uses agents dict return manifest @@ -28,27 +35,27 @@ class TestCmdStartSelector(unittest.TestCase): """Drive cmd_start with a minimal set of stubs.""" def setUp(self): - # Stub Manifest.resolve so no on-disk manifest is needed. - self._manifest = _make_manifest(["researcher", "implementer"]) + self._manifest = _make_manifest(["researcher", "implementer"], ["claude", "dev"]) self._resolve_patch = patch( "bot_bottle.cli.start.ManifestIndex.resolve", return_value=self._manifest, ) self._resolve_patch.start() - # Stub _launch_bottle so no real container work happens. self._launch_patch = patch( "bot_bottle.cli.start._launch_bottle", return_value=0, ) self._launch_mock = self._launch_patch.start() - # Stub filter_select to avoid opening /dev/tty. - self._tui_patch = patch.object(tui_mod, "filter_select") - self._tui_mock = self._tui_patch.start() + # Stub filter_select (agent picker) and filter_multiselect (bottle picker). + self._agent_picker_patch = patch.object(tui_mod, "filter_select") + self._agent_picker_mock = self._agent_picker_patch.start() + + self._bottle_picker_patch = patch.object(tui_mod, "filter_multiselect") + self._bottle_picker_mock = self._bottle_picker_patch.start() + self._bottle_picker_mock.return_value = ["claude"] # default: one bottle selected - # Ensure BOT_BOTTLE_BACKEND is absent so omitted --backend - # flows through to the resolver default. self._env_patch = patch.dict(os.environ, {}, clear=False) self._env_patch.start() os.environ.pop("BOT_BOTTLE_BACKEND", None) @@ -56,50 +63,108 @@ class TestCmdStartSelector(unittest.TestCase): def tearDown(self): self._resolve_patch.stop() self._launch_patch.stop() - self._tui_patch.stop() + self._agent_picker_patch.stop() + self._bottle_picker_patch.stop() self._env_patch.stop() # ------------------------------------------------------------------ - # Both explicit — no picker shown + # Agent explicit — agent picker skipped; bottle picker always shown # ------------------------------------------------------------------ - def test_both_explicit_skips_picker(self): - self._tui_mock.return_value = "researcher" + def test_explicit_agent_skips_agent_picker(self): rc = start_mod.cmd_start(["--backend=docker", "researcher"]) self.assertEqual(0, rc) - self._tui_mock.assert_not_called() + self._agent_picker_mock.assert_not_called() + self._bottle_picker_mock.assert_called_once() self._launch_mock.assert_called_once() - _, kwargs = self._launch_mock.call_args - self.assertEqual("docker", kwargs["backend_name"]) + + def test_explicit_agent_bottle_picker_shows_available_bottles(self): + start_mod.cmd_start(["researcher"]) + call_kwargs = self._bottle_picker_mock.call_args + self.assertEqual(["claude", "dev"], call_kwargs[0][0]) + self.assertIn("bottle", call_kwargs[1]["title"].lower()) # ------------------------------------------------------------------ - # Agent absent → agent picker fires; backend explicit + # Agent absent → agent picker fires; bottle picker always follows # ------------------------------------------------------------------ def test_agent_absent_shows_agent_picker(self): - self._tui_mock.return_value = "researcher" + self._agent_picker_mock.return_value = "researcher" rc = start_mod.cmd_start(["--backend=docker"]) self.assertEqual(0, rc) - self._tui_mock.assert_called_once() - call_kwargs = self._tui_mock.call_args + self._agent_picker_mock.assert_called_once() + call_kwargs = self._agent_picker_mock.call_args self.assertEqual(["implementer", "researcher"], call_kwargs[0][0]) self.assertIn("agent", call_kwargs[1]["title"].lower()) + # Bottle picker must also fire after agent selection. + self._bottle_picker_mock.assert_called_once() - def test_agent_picker_cancel_returns_0(self): - self._tui_mock.return_value = None + def test_agent_picker_cancel_skips_bottle_picker(self): + self._agent_picker_mock.return_value = None rc = start_mod.cmd_start(["--backend=docker"]) self.assertEqual(0, rc) + self._bottle_picker_mock.assert_not_called() + self._launch_mock.assert_not_called() + + def test_bottle_picker_cancel_returns_0(self): + self._bottle_picker_mock.return_value = None + rc = start_mod.cmd_start(["researcher"]) + self.assertEqual(0, rc) self._launch_mock.assert_not_called() # ------------------------------------------------------------------ - # Agent explicit, backend absent → no picker + # Bottle selection is forwarded to BottleSpec # ------------------------------------------------------------------ - def test_backend_absent_uses_default_without_picker(self): - rc = start_mod.cmd_start(["researcher"]) - self.assertEqual(0, rc) - self._tui_mock.assert_not_called() + def test_selected_bottles_forwarded_to_spec(self): + self._bottle_picker_mock.return_value = ["claude", "dev"] + start_mod.cmd_start(["researcher"]) self._launch_mock.assert_called_once() + spec = self._launch_mock.call_args[0][0] + self.assertEqual(("claude", "dev"), spec.bottle_names) + + def test_empty_bottle_selection_forwarded(self): + self._bottle_picker_mock.return_value = [] + start_mod.cmd_start(["researcher"]) + self._launch_mock.assert_called_once() + spec = self._launch_mock.call_args[0][0] + self.assertEqual((), spec.bottle_names) + + # ------------------------------------------------------------------ + # Agent default bottle pre-populates the picker + # ------------------------------------------------------------------ + + def test_agent_bottle_prepopulates_bottle_picker(self): + manifest = _make_manifest( + ["implementer"], ["claude", "dev"], agent_bottle="claude" + ) + with patch( + "bot_bottle.cli.start.ManifestIndex.resolve", return_value=manifest + ): + start_mod.cmd_start(["implementer"]) + call_kwargs = self._bottle_picker_mock.call_args + self.assertEqual(["claude"], call_kwargs[1]["initial"]) + + def test_no_agent_bottle_empty_initial(self): + manifest = _make_manifest(["researcher"], ["claude", "dev"], agent_bottle="") + with patch( + "bot_bottle.cli.start.ManifestIndex.resolve", return_value=manifest + ): + start_mod.cmd_start(["researcher"]) + call_kwargs = self._bottle_picker_mock.call_args + self.assertEqual([], call_kwargs[1]["initial"]) + + # ------------------------------------------------------------------ + # Backend wiring + # ------------------------------------------------------------------ + + def test_explicit_backend_forwarded(self): + start_mod.cmd_start(["--backend=docker", "researcher"]) + _, kwargs = self._launch_mock.call_args + self.assertEqual("docker", kwargs["backend_name"]) + + def test_absent_backend_uses_default(self): + start_mod.cmd_start(["researcher"]) _, kwargs = self._launch_mock.call_args self.assertIsNone(kwargs["backend_name"]) @@ -110,28 +175,21 @@ class TestCmdStartSelector(unittest.TestCase): finally: os.environ.pop("BOT_BOTTLE_BACKEND", None) self.assertEqual(0, rc) - self._tui_mock.assert_not_called() - # ------------------------------------------------------------------ - # Both absent → only agent picker - # ------------------------------------------------------------------ - - def test_both_absent_shows_only_agent_picker(self): - self._tui_mock.return_value = "researcher" + def test_both_absent_shows_agent_picker_then_bottle_picker(self): + self._agent_picker_mock.return_value = "researcher" rc = start_mod.cmd_start([]) self.assertEqual(0, rc) - self._tui_mock.assert_called_once() - title = self._tui_mock.call_args[1]["title"].lower() - self.assertIn("agent", title) + self._agent_picker_mock.assert_called_once() + self._bottle_picker_mock.assert_called_once() self._launch_mock.assert_called_once() - _, kwargs = self._launch_mock.call_args - self.assertIsNone(kwargs["backend_name"]) - def test_both_absent_agent_cancel_skips_backend_picker(self): - self._tui_mock.side_effect = [None] + def test_both_absent_agent_cancel_skips_bottle_and_launch(self): + self._agent_picker_mock.return_value = None rc = start_mod.cmd_start([]) self.assertEqual(0, rc) - self.assertEqual(1, self._tui_mock.call_count) + self._agent_picker_mock.assert_called_once() + self._bottle_picker_mock.assert_not_called() self._launch_mock.assert_not_called() @@ -149,11 +207,13 @@ class TestCmdStartLabelCollision(unittest.TestCase): """cmd_start re-prompts when the label's slug is already running.""" def setUp(self): - self._manifest = _make_manifest(["researcher"]) + self._manifest = _make_manifest(["researcher"], ["claude"]) patch("bot_bottle.cli.start.ManifestIndex.resolve", return_value=self._manifest).start() self._launch_mock = patch( "bot_bottle.cli.start._launch_bottle", return_value=0, ).start() + # Stub the bottle picker to always return a selection. + patch.object(tui_mod, "filter_multiselect", return_value=["claude"]).start() self.addCleanup(patch.stopall) def test_no_collision_proceeds_without_reprompt(self): diff --git a/tests/unit/test_cli_tui.py b/tests/unit/test_cli_tui.py index 1f2ca5b..f27fb89 100644 --- a/tests/unit/test_cli_tui.py +++ b/tests/unit/test_cli_tui.py @@ -1,4 +1,4 @@ -"""Unit tests for bot_bottle.cli.tui — filter_select internals. +"""Unit tests for bot_bottle.cli.tui — filter_select and filter_multiselect. We test the pure-Python logic (_filter_items, cursor movement, confirm, cancel) by exercising the internal helpers directly, without spinning up @@ -9,7 +9,7 @@ from __future__ import annotations import unittest -from bot_bottle.cli.tui import _filter_items, filter_select +from bot_bottle.cli.tui import _filter_items, filter_multiselect, filter_select class TestFilterItems(unittest.TestCase): @@ -46,5 +46,16 @@ class TestFilterSelectEmptyItems(unittest.TestCase): self.assertIsNone(result) +class TestFilterMultiselectEmptyItems(unittest.TestCase): + def test_returns_empty_list_for_empty_items(self): + # No TTY needed — short-circuits before opening tty. + result = filter_multiselect([], title="Select", tty_path="/dev/null") + self.assertEqual([], result) + + def test_returns_none_when_tty_unavailable(self): + result = filter_multiselect(["a", "b"], tty_path="/nonexistent/tty") + self.assertIsNone(result) + + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/test_manifest_bottle_merge.py b/tests/unit/test_manifest_bottle_merge.py new file mode 100644 index 0000000..f32d59b --- /dev/null +++ b/tests/unit/test_manifest_bottle_merge.py @@ -0,0 +1,200 @@ +"""Unit: runtime bottle composition (issue #269). + +Tests for merge_bottles_runtime and ManifestIndex.load_for_agent with +the new bottle_names parameter. +""" + +from __future__ import annotations + +import os +import shutil +import tempfile +import textwrap +import unittest +from pathlib import Path + +from bot_bottle.manifest import ManifestBottle, ManifestError, ManifestIndex +from bot_bottle.manifest_extends import merge_bottles_runtime + + +def _index(bottles: dict, agents: dict) -> ManifestIndex: + return ManifestIndex.from_json_obj({"bottles": bottles, "agents": agents}) + + +def _bottle(**kwargs) -> ManifestBottle: + return ManifestBottle.from_dict("test", kwargs) + + +class TestMergeBottlesRuntime(unittest.TestCase): + def test_single_bottle_returns_as_is(self): + b = _bottle(env={"FOO": "1"}) + result = merge_bottles_runtime([b]) + self.assertEqual({"FOO": "1"}, dict(result.env)) + + def test_env_later_wins(self): + base = _bottle(env={"FOO": "base", "ONLY_BASE": "x"}) + override = _bottle(env={"FOO": "override", "ONLY_OVERRIDE": "y"}) + result = merge_bottles_runtime([base, override]) + self.assertEqual("override", result.env["FOO"]) + self.assertEqual("x", result.env["ONLY_BASE"]) + self.assertEqual("y", result.env["ONLY_OVERRIDE"]) + + def test_egress_routes_concatenated(self): + from bot_bottle.manifest_egress import ManifestEgressConfig, ManifestEgressRoute + r1 = ManifestEgressRoute(Host="api.a.com") + r2 = ManifestEgressRoute(Host="api.b.com") + base = ManifestBottle(egress=ManifestEgressConfig(routes=(r1,))) + override = ManifestBottle(egress=ManifestEgressConfig(routes=(r2,))) + result = merge_bottles_runtime([base, override]) + hosts = [r.Host for r in result.egress.routes] + self.assertIn("api.a.com", hosts) + self.assertIn("api.b.com", hosts) + + def test_supervise_later_wins(self): + base = _bottle(supervise=True) + override = _bottle(supervise=False) + result = merge_bottles_runtime([base, override]) + self.assertFalse(result.supervise) + + def test_three_bottles_merged_left_to_right(self): + b1 = _bottle(env={"A": "1", "B": "1", "C": "1"}) + b2 = _bottle(env={"B": "2", "C": "2"}) + b3 = _bottle(env={"C": "3"}) + result = merge_bottles_runtime([b1, b2, b3]) + self.assertEqual("1", result.env["A"]) + self.assertEqual("2", result.env["B"]) + self.assertEqual("3", result.env["C"]) + + def test_empty_list_raises(self): + with self.assertRaises(ValueError): + merge_bottles_runtime([]) + + +class TestLoadForAgentWithBottleNames(unittest.TestCase): + def test_bottle_names_override_agent_bottle(self): + idx = _index( + bottles={ + "base": {"env": {"X": "base"}}, + "override": {"env": {"X": "override"}}, + }, + agents={"impl": {"bottle": "base", "skills": [], "prompt": ""}}, + ) + m = idx.load_for_agent("impl", ("override",)) + self.assertEqual("override", m.bottle.env["X"]) + + def test_bottle_names_merged_in_order(self): + idx = _index( + bottles={ + "a": {"env": {"X": "a", "A": "only-a"}}, + "b": {"env": {"X": "b", "B": "only-b"}}, + }, + agents={"impl": {"bottle": "a", "skills": [], "prompt": ""}}, + ) + m = idx.load_for_agent("impl", ("a", "b")) + self.assertEqual("b", m.bottle.env["X"]) + self.assertEqual("only-a", m.bottle.env["A"]) + self.assertEqual("only-b", m.bottle.env["B"]) + + def test_empty_bottle_names_uses_agent_bottle(self): + idx = _index( + bottles={"base": {"env": {"X": "base"}}}, + agents={"impl": {"bottle": "base", "skills": [], "prompt": ""}}, + ) + m = idx.load_for_agent("impl", ()) + self.assertEqual("base", m.bottle.env["X"]) + + def test_no_bottle_and_no_bottle_names_raises(self): + idx = _index( + bottles={"base": {}}, + agents={"impl": {"skills": [], "prompt": ""}}, + ) + with self.assertRaises(ManifestError) as ctx: + idx.load_for_agent("impl", ()) + self.assertIn("no 'bottle' field", str(ctx.exception)) + + def test_unknown_bottle_name_raises(self): + idx = _index( + bottles={"base": {}}, + agents={"impl": {"bottle": "base", "skills": [], "prompt": ""}}, + ) + with self.assertRaises(ManifestError) as ctx: + idx.load_for_agent("impl", ("nonexistent",)) + self.assertIn("nonexistent", str(ctx.exception)) + + def test_agent_without_bottle_works_with_bottle_names(self): + idx = _index( + bottles={"base": {"env": {"X": "base"}}}, + agents={"impl": {"skills": [], "prompt": ""}}, + ) + m = idx.load_for_agent("impl", ("base",)) + self.assertEqual("base", m.bottle.env["X"]) + + +class TestAllBottleNames(unittest.TestCase): + def test_eager_mode_returns_bottle_names(self): + idx = _index( + bottles={"alpha": {}, "beta": {}, "gamma": {}}, + agents={"impl": {"bottle": "alpha", "skills": [], "prompt": ""}}, + ) + self.assertEqual(["alpha", "beta", "gamma"], idx.all_bottle_names) + + def test_lazy_mode_scans_files(self): + home = Path(tempfile.mkdtemp(prefix="cb-home-")) + orig_home = os.environ.get("HOME") + os.environ["HOME"] = str(home) + try: + bottles_dir = home / ".bot-bottle" / "bottles" + agents_dir = home / ".bot-bottle" / "agents" + bottles_dir.mkdir(parents=True) + agents_dir.mkdir(parents=True) + (bottles_dir / "claude.md").write_text("---\n---\n") + (bottles_dir / "dev.md").write_text("---\n---\n") + (agents_dir / "impl.md").write_text("---\nbottle: claude\n---\n") + idx = ManifestIndex.resolve(str(home)) + self.assertEqual(["claude", "dev"], idx.all_bottle_names) + finally: + if orig_home is None: + os.environ.pop("HOME", None) + else: + os.environ["HOME"] = orig_home + shutil.rmtree(home, ignore_errors=True) + + +class TestAgentOptionalBottleMd(unittest.TestCase): + """Agent file without bottle: works when bottle_names are provided at launch.""" + + def setUp(self) -> None: + self.home = Path(tempfile.mkdtemp(prefix="cb-home-")) + self._orig_home = os.environ.get("HOME") + os.environ["HOME"] = str(self.home) + + def tearDown(self) -> None: + if self._orig_home is None: + os.environ.pop("HOME", None) + else: + os.environ["HOME"] = self._orig_home + shutil.rmtree(self.home, ignore_errors=True) + + def _write(self, rel: str, text: str) -> None: + p = self.home / ".bot-bottle" / rel + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(textwrap.dedent(text).lstrip("\n")) + + def test_agent_without_bottle_resolves_with_bottle_names(self): + self._write("bottles/dev.md", "---\nenv:\n X: dev\n---\n") + self._write("agents/impl.md", "---\n---\nimpl agent.\n") + idx = ManifestIndex.resolve(str(self.home)) + m = idx.load_for_agent("impl", ("dev",)) + self.assertEqual("dev", m.bottle.env["X"]) + + def test_agent_without_bottle_fails_without_bottle_names(self): + self._write("bottles/dev.md", "---\n---\n") + self._write("agents/impl.md", "---\n---\nimpl agent.\n") + idx = ManifestIndex.resolve(str(self.home)) + with self.assertRaises(ManifestError) as ctx: + idx.load_for_agent("impl", ()) + self.assertIn("no 'bottle' field", str(ctx.exception)) + + +if __name__ == "__main__": + unittest.main()