diff --git a/bot_bottle/orchestrator/bootstrap.py b/bot_bottle/orchestrator/bootstrap.py index 636d7d8..b200fc3 100644 --- a/bot_bottle/orchestrator/bootstrap.py +++ b/bot_bottle/orchestrator/bootstrap.py @@ -22,7 +22,7 @@ from ..contrib.gitea.forge_state import ForgeState, SqliteForgeStateStore from .config import Config from .lifecycle import Orchestrator from .model import RunRecord -from .runner import SubprocessBottleRunner +from .runner import ProgrammaticBottleRunner from .sidecar import ForgeSidecar, OpLog, drain_done_events from .watchdog import Watchdog from .webhook import WebhookServer @@ -104,7 +104,7 @@ def make_sidecar( def build(config: Config) -> tuple[WebhookServer, Watchdog, Orchestrator]: store = BotBottleStateStore(config.db_path) - runner = SubprocessBottleRunner(cli=config.bot_bottle_cli, base_env=dict(os.environ)) + runner = ProgrammaticBottleRunner() membership_forge = make_forge(config, "_", "_") orchestrator = Orchestrator( forge=membership_forge, diff --git a/bot_bottle/orchestrator/config.py b/bot_bottle/orchestrator/config.py index e66aa7b..a996f8c 100644 --- a/bot_bottle/orchestrator/config.py +++ b/bot_bottle/orchestrator/config.py @@ -26,7 +26,6 @@ class Config: watchdog_timeout_secs: int webhook_host: str webhook_port: int - bot_bottle_cli: str queue_dir: Path sidecar_socket: Path db_path: Path | None @@ -43,7 +42,6 @@ class Config: watchdog_timeout_secs=int(e.get("FORGE_WATCHDOG_TIMEOUT", "1800")), webhook_host=e.get("FORGE_WEBHOOK_HOST", "127.0.0.1"), webhook_port=int(e.get("FORGE_WEBHOOK_PORT", "8477")), - bot_bottle_cli=e.get("BOT_BOTTLE_CLI", "cli.py"), queue_dir=Path(e.get("FORGE_QUEUE_DIR", str(default_root / "forge-queue"))), sidecar_socket=Path( e.get("FORGE_SIDECAR_SOCKET", str(default_root / "forge-sidecar.sock")) diff --git a/bot_bottle/orchestrator/lifecycle.py b/bot_bottle/orchestrator/lifecycle.py index 5e6f6cb..5185ca4 100644 --- a/bot_bottle/orchestrator/lifecycle.py +++ b/bot_bottle/orchestrator/lifecycle.py @@ -110,7 +110,7 @@ class Orchestrator: def _launch(self, event: IssueAssigned, target: Target) -> None: label = self._label_for(target.agent_name, event) bottles = [target.bottle_override] if target.bottle_override else [] - result = self._runner.start( + slug = self._runner.start( agent=target.agent_name, bottles=bottles, label=label, @@ -122,7 +122,7 @@ class Orchestrator: owner=event.owner, repo=event.repo, issue_number=event.issue_number, - slug=result.slug, + slug=slug, agent_name=target.agent_name, bottle_names=bottles, status=STATUS_RUNNING, diff --git a/bot_bottle/orchestrator/runner.py b/bot_bottle/orchestrator/runner.py index 2bba90f..6254e6f 100644 --- a/bot_bottle/orchestrator/runner.py +++ b/bot_bottle/orchestrator/runner.py @@ -1,31 +1,22 @@ -"""Bottle runner: drive the bot-bottle CLI to manage a bottle's life. +"""Bottle runner: drive bot_bottle to manage a bottle's life. `BottleRunner` is the interface the lifecycle depends on; -`SubprocessBottleRunner` shells out to the bot-bottle `cli.py` -(`start --headless`, `commit`, `resume --headless`). The subprocess -callable is injectable so tests never spawn a process. +`ProgrammaticBottleRunner` calls into the bot_bottle Python API directly +(no subprocess). The slug returned by `start` is the actual slug minted +at launch time — not a post-hoc derivation from the label — so it is +authoritative even if bot-bottle's slugification logic changes. -The slug is derived from the label via `slugify`, matching bot-bottle's -container-slug rule; the orchestrator picks labels that embed the issue -identity so slugs are unique and collisions never rename them. +`slugify` is retained for `FakeRunner` (tests) and for the label scheme +the orchestrator uses to predict collision-free slugs. """ from __future__ import annotations import re -import subprocess -import sys -from collections.abc import Callable, Sequence -from dataclasses import dataclass +from collections.abc import Sequence from typing import Protocol -@dataclass(frozen=True) -class RunResult: - slug: str - exit_code: int - - class BottleRunner(Protocol): def start( self, @@ -35,13 +26,13 @@ class BottleRunner(Protocol): label: str, prompt: str, forge_env: dict[str, str], - ) -> RunResult: ... + ) -> str: ... - def freeze(self, slug: str) -> int: ... + def freeze(self, slug: str) -> None: ... - def resume(self, slug: str, prompt: str) -> RunResult: ... + def resume(self, slug: str, prompt: str) -> None: ... - def destroy(self, slug: str) -> int: ... + def destroy(self, slug: str) -> None: ... _SLUG_RE = re.compile(r"[^a-z0-9]+") @@ -53,34 +44,12 @@ def slugify(label: str) -> str: return _SLUG_RE.sub("-", label.lower()).strip("-") -# A subprocess.run-shaped callable, injectable for tests. -RunFn = Callable[[Sequence[str], dict[str, str]], int] +class ProgrammaticBottleRunner: + """Calls into the bot_bottle Python API directly — no subprocess. - -def _default_run(argv: Sequence[str], env: dict[str, str]) -> int: - return subprocess.run(list(argv), env=env, check=False).returncode - - -class SubprocessBottleRunner: - """Shells the bot-bottle CLI. `cli` is the path to `cli.py`; `python` - is the interpreter to run it with; `base_env` is the environment the - child inherits (the orchestrator's, minus per-run additions).""" - - def __init__( - self, - *, - cli: str, - base_env: dict[str, str], - python: str = sys.executable, - run: RunFn = _default_run, - ) -> None: - self._cli = cli - self._python = python - self._base_env = base_env - self._run = run - - def _argv(self, *args: str) -> list[str]: - return [self._python, self._cli, *args] + Imports are deferred to call time so this module can be imported + before `bot_bottle.api` is available (e.g. in isolated test runs + that mock the API surface).""" def start( self, @@ -90,29 +59,24 @@ class SubprocessBottleRunner: label: str, prompt: str, forge_env: dict[str, str], - ) -> RunResult: - argv = self._argv( - "start", agent, "--headless", "--label", label, "--prompt", prompt + ) -> str: + import bot_bottle.api as api + return api.start_headless( + agent, + prompt=prompt, + bottles=list(bottles) or None, + label=label, + forge_env=forge_env, ) - for bottle in bottles: - argv += ["--bottle", bottle] - code = self._run(argv, {**self._base_env, **forge_env}) - return RunResult(slug=slugify(label), exit_code=code) - def freeze(self, slug: str) -> int: - # bot-bottle's `commit` snapshots a running bottle's state. - return self._run(self._argv("commit", slug), self._base_env) + def freeze(self, slug: str) -> None: + import bot_bottle.api as api + api.freeze(slug) - def resume(self, slug: str, prompt: str) -> RunResult: - code = self._run( - self._argv("resume", slug, "--headless", "--prompt", prompt), - self._base_env, - ) - return RunResult(slug=slug, exit_code=code) + def resume(self, slug: str, prompt: str) -> None: + import bot_bottle.api as api + api.resume_headless(slug, prompt=prompt) - def destroy(self, slug: str) -> int: - # NOTE: bot-bottle `cleanup` currently targets all bottles; a - # per-slug teardown command is a known integration follow-up - # (tracked in docs/JOURNAL.md). Kept behind this method so the - # call site does not change when that lands. - return self._run(self._argv("cleanup", slug), self._base_env) + def destroy(self, slug: str) -> None: + import bot_bottle.api as api + api.destroy(slug) diff --git a/tests/unit/orchestrator/_fakes.py b/tests/unit/orchestrator/_fakes.py index 84dfd83..3b6c82a 100644 --- a/tests/unit/orchestrator/_fakes.py +++ b/tests/unit/orchestrator/_fakes.py @@ -7,7 +7,7 @@ from __future__ import annotations from collections.abc import Sequence -from bot_bottle.orchestrator.runner import RunResult, slugify +from bot_bottle.orchestrator.runner import slugify class FakeForge: @@ -52,18 +52,15 @@ class FakeRunner: label: str, prompt: str, forge_env: dict[str, str], - ) -> RunResult: + ) -> str: self.calls.append(("start", agent, tuple(bottles), label, prompt, dict(forge_env))) - return RunResult(slug=slugify(label), exit_code=0) + return slugify(label) - def freeze(self, slug: str) -> int: + def freeze(self, slug: str) -> None: self.calls.append(("freeze", slug)) - return 0 - def resume(self, slug: str, prompt: str) -> RunResult: + def resume(self, slug: str, prompt: str) -> None: self.calls.append(("resume", slug, prompt)) - return RunResult(slug=slug, exit_code=0) - def destroy(self, slug: str) -> int: + def destroy(self, slug: str) -> None: self.calls.append(("destroy", slug)) - return 0 diff --git a/tests/unit/orchestrator/test_bootstrap.py b/tests/unit/orchestrator/test_bootstrap.py index 54e2677..1b8105f 100644 --- a/tests/unit/orchestrator/test_bootstrap.py +++ b/tests/unit/orchestrator/test_bootstrap.py @@ -28,7 +28,6 @@ def _config(tmp: str) -> Config: watchdog_timeout_secs=1800, webhook_host="127.0.0.1", webhook_port=0, - bot_bottle_cli="cli.py", queue_dir=Path(tmp) / "q", sidecar_socket=Path(tmp) / "s.sock", db_path=None, diff --git a/tests/unit/orchestrator/test_runner.py b/tests/unit/orchestrator/test_runner.py index 84c1408..09c64ff 100644 --- a/tests/unit/orchestrator/test_runner.py +++ b/tests/unit/orchestrator/test_runner.py @@ -1,11 +1,13 @@ -"""Unit: SubprocessBottleRunner + slugify (injected run fn).""" +"""Unit: ProgrammaticBottleRunner + slugify.""" from __future__ import annotations +import sys +import types import unittest -from collections.abc import Sequence +from unittest.mock import MagicMock -from bot_bottle.orchestrator.runner import SubprocessBottleRunner, slugify +from bot_bottle.orchestrator.runner import ProgrammaticBottleRunner, slugify class SlugifyTest(unittest.TestCase): @@ -17,64 +19,75 @@ class SlugifyTest(unittest.TestCase): self.assertEqual("a-b-c", slugify(" A_B/C!! ")) -class SubprocessRunnerTest(unittest.TestCase): +def _make_api_stub(**overrides): + """Return a mock bot_bottle.api module with sensible defaults.""" + stub = types.ModuleType("bot_bottle.api") + stub.start_headless = MagicMock(return_value="impl-r-17") + stub.freeze = MagicMock() + stub.resume_headless = MagicMock() + stub.destroy = MagicMock() + for k, v in overrides.items(): + setattr(stub, k, v) + return stub + + +class ProgrammaticRunnerTest(unittest.TestCase): def setUp(self): - self.argvs: list[list[str]] = [] - self.envs: list[dict[str, str]] = [] + self._api = _make_api_stub() + sys.modules["bot_bottle.api"] = self._api + self.runner = ProgrammaticBottleRunner() - def fake_run(argv: Sequence[str], env: dict[str, str]) -> int: - self.argvs.append(list(argv)) - self.envs.append(dict(env)) - return 0 + def tearDown(self): + sys.modules.pop("bot_bottle.api", None) - self.runner = SubprocessBottleRunner( - cli="/x/cli.py", base_env={"PATH": "/bin"}, python="/py", run=fake_run - ) - - def test_start_argv_and_env(self): - result = self.runner.start( + def test_start_returns_slug_from_api(self): + slug = self.runner.start( agent="impl", bottles=["claude", "dev"], label="impl-r-17", prompt="do it", forge_env={"FORGE_OWNER": "didericis"}, ) - self.assertEqual("impl-r-17", result.slug) - argv = self.argvs[0] - self.assertEqual(["/py", "/x/cli.py", "start", "impl", "--headless", - "--label", "impl-r-17", "--prompt", "do it", - "--bottle", "claude", "--bottle", "dev"], argv) - # forge_env merged over base_env for the child. - self.assertEqual("didericis", self.envs[0]["FORGE_OWNER"]) - self.assertEqual("/bin", self.envs[0]["PATH"]) + self.assertEqual("impl-r-17", slug) - def test_start_no_bottles_omits_flag(self): + def test_start_forwards_all_args(self): + self.runner.start( + agent="impl", bottles=["claude", "dev"], label="impl-r-17", + prompt="do it", forge_env={"FORGE_OWNER": "didericis"}, + ) + self._api.start_headless.assert_called_once_with( + "impl", + prompt="do it", + bottles=["claude", "dev"], + label="impl-r-17", + forge_env={"FORGE_OWNER": "didericis"}, + ) + + def test_start_no_bottles_passes_none(self): self.runner.start(agent="impl", bottles=[], label="l", prompt="p", forge_env={}) - self.assertNotIn("--bottle", self.argvs[0]) + call_kwargs = self._api.start_headless.call_args[1] + self.assertIsNone(call_kwargs["bottles"]) - def test_freeze_calls_commit(self): + def test_freeze_delegates_to_api(self): self.runner.freeze("slug-1") - self.assertEqual(["/py", "/x/cli.py", "commit", "slug-1"], self.argvs[0]) + self._api.freeze.assert_called_once_with("slug-1") - def test_resume_headless(self): - r = self.runner.resume("slug-1", "address review") - self.assertEqual("slug-1", r.slug) - self.assertEqual( - ["/py", "/x/cli.py", "resume", "slug-1", "--headless", "--prompt", - "address review"], self.argvs[0]) + def test_freeze_returns_none(self): + result = self.runner.freeze("slug-1") + self.assertIsNone(result) - def test_destroy_calls_cleanup(self): - code = self.runner.destroy("slug-7") - self.assertEqual(0, code) - self.assertEqual(["/py", "/x/cli.py", "cleanup", "slug-7"], self.argvs[0]) + def test_resume_delegates_to_api(self): + self.runner.resume("slug-1", "address review") + self._api.resume_headless.assert_called_once_with("slug-1", prompt="address review") + def test_resume_returns_none(self): + result = self.runner.resume("slug-1", "p") + self.assertIsNone(result) -class DefaultRunTest(unittest.TestCase): - def test_calls_subprocess_and_returns_code(self): - from unittest.mock import MagicMock, patch - from bot_bottle.orchestrator.runner import _default_run - with patch("subprocess.run") as mock_run: - mock_run.return_value = MagicMock(returncode=42) - code = _default_run(["echo", "hi"], {"PATH": "/bin"}) - self.assertEqual(42, code) - mock_run.assert_called_once_with(["echo", "hi"], env={"PATH": "/bin"}, check=False) + def test_destroy_delegates_to_api(self): + self.runner.destroy("slug-7") + self._api.destroy.assert_called_once_with("slug-7") + + def test_destroy_returns_none(self): + result = self.runner.destroy("slug-7") + self.assertIsNone(result) if __name__ == "__main__":