Compare commits

...

2 Commits

Author SHA1 Message Date
didericis-claude 18e610c7a8 fix: resolve pylint/pyright issues in runner, sidecar, and test_runner
lint / lint (push) Successful in 2m1s
test / unit (pull_request) Successful in 50s
test / integration (pull_request) Successful in 17s
test / coverage (pull_request) Successful in 1m3s
runner.py: use 'from bot_bottle import api' (satisfies R0402) with
type: ignore and pylint disable for the cross-branch dependency on
bot_bottle.api (added in PR #318, which merges before this one).
sidecar.py: add pylint disable for intentional broad-exception-caught.
test_runner.py: annotate _make_api_stub(**overrides: object) -> Any and
type stub variable as Any to allow attribute assignment without
type: ignore per-line.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-07-01 20:27:57 +00:00
didericis-claude d5fb159857 refactor(orchestrator): swap SubprocessBottleRunner → ProgrammaticBottleRunner
lint / lint (push) Failing after 2m15s
test / unit (pull_request) Successful in 51s
test / integration (pull_request) Successful in 21s
test / coverage (pull_request) Successful in 1m7s
BottleRunner Protocol tightened: start() → str, freeze/resume/destroy → None.
RunResult removed. lifecycle.py unpacks the slug directly. FakeRunner and
test_runner updated to match. Config.bot_bottle_cli dropped (nothing uses it).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-07-01 19:48:06 +00:00
8 changed files with 107 additions and 134 deletions
+2 -2
View File
@@ -22,7 +22,7 @@ from ..contrib.gitea.forge_state import ForgeState, SqliteForgeStateStore
from .config import Config from .config import Config
from .lifecycle import Orchestrator from .lifecycle import Orchestrator
from .model import RunRecord from .model import RunRecord
from .runner import SubprocessBottleRunner from .runner import ProgrammaticBottleRunner
from .sidecar import ForgeSidecar, OpLog, drain_done_events from .sidecar import ForgeSidecar, OpLog, drain_done_events
from .watchdog import Watchdog from .watchdog import Watchdog
from .webhook import WebhookServer from .webhook import WebhookServer
@@ -104,7 +104,7 @@ def make_sidecar(
def build(config: Config) -> tuple[WebhookServer, Watchdog, Orchestrator]: def build(config: Config) -> tuple[WebhookServer, Watchdog, Orchestrator]:
store = BotBottleStateStore(config.db_path) store = BotBottleStateStore(config.db_path)
runner = SubprocessBottleRunner(cli=config.bot_bottle_cli, base_env=dict(os.environ)) runner = ProgrammaticBottleRunner()
membership_forge = make_forge(config, "_", "_") membership_forge = make_forge(config, "_", "_")
orchestrator = Orchestrator( orchestrator = Orchestrator(
forge=membership_forge, forge=membership_forge,
-2
View File
@@ -26,7 +26,6 @@ class Config:
watchdog_timeout_secs: int watchdog_timeout_secs: int
webhook_host: str webhook_host: str
webhook_port: int webhook_port: int
bot_bottle_cli: str
queue_dir: Path queue_dir: Path
sidecar_socket: Path sidecar_socket: Path
db_path: Path | None db_path: Path | None
@@ -43,7 +42,6 @@ class Config:
watchdog_timeout_secs=int(e.get("FORGE_WATCHDOG_TIMEOUT", "1800")), watchdog_timeout_secs=int(e.get("FORGE_WATCHDOG_TIMEOUT", "1800")),
webhook_host=e.get("FORGE_WEBHOOK_HOST", "127.0.0.1"), webhook_host=e.get("FORGE_WEBHOOK_HOST", "127.0.0.1"),
webhook_port=int(e.get("FORGE_WEBHOOK_PORT", "8477")), 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"))), queue_dir=Path(e.get("FORGE_QUEUE_DIR", str(default_root / "forge-queue"))),
sidecar_socket=Path( sidecar_socket=Path(
e.get("FORGE_SIDECAR_SOCKET", str(default_root / "forge-sidecar.sock")) e.get("FORGE_SIDECAR_SOCKET", str(default_root / "forge-sidecar.sock"))
+2 -2
View File
@@ -110,7 +110,7 @@ class Orchestrator:
def _launch(self, event: IssueAssigned, target: Target) -> None: def _launch(self, event: IssueAssigned, target: Target) -> None:
label = self._label_for(target.agent_name, event) label = self._label_for(target.agent_name, event)
bottles = [target.bottle_override] if target.bottle_override else [] bottles = [target.bottle_override] if target.bottle_override else []
result = self._runner.start( slug = self._runner.start(
agent=target.agent_name, agent=target.agent_name,
bottles=bottles, bottles=bottles,
label=label, label=label,
@@ -122,7 +122,7 @@ class Orchestrator:
owner=event.owner, owner=event.owner,
repo=event.repo, repo=event.repo,
issue_number=event.issue_number, issue_number=event.issue_number,
slug=result.slug, slug=slug,
agent_name=target.agent_name, agent_name=target.agent_name,
bottle_names=bottles, bottle_names=bottles,
status=STATUS_RUNNING, status=STATUS_RUNNING,
+35 -70
View File
@@ -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; `BottleRunner` is the interface the lifecycle depends on;
`SubprocessBottleRunner` shells out to the bot-bottle `cli.py` `ProgrammaticBottleRunner` calls into the bot_bottle Python API directly
(`start --headless`, `commit`, `resume --headless`). The subprocess (no subprocess). The slug returned by `start` is the actual slug minted
callable is injectable so tests never spawn a process. 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 `slugify` is retained for `FakeRunner` (tests) and for the label scheme
container-slug rule; the orchestrator picks labels that embed the issue the orchestrator uses to predict collision-free slugs.
identity so slugs are unique and collisions never rename them.
""" """
from __future__ import annotations from __future__ import annotations
import re import re
import subprocess from collections.abc import Sequence
import sys
from collections.abc import Callable, Sequence
from dataclasses import dataclass
from typing import Protocol from typing import Protocol
@dataclass(frozen=True)
class RunResult:
slug: str
exit_code: int
class BottleRunner(Protocol): class BottleRunner(Protocol):
def start( def start(
self, self,
@@ -35,13 +26,13 @@ class BottleRunner(Protocol):
label: str, label: str,
prompt: str, prompt: str,
forge_env: dict[str, 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]+") _SLUG_RE = re.compile(r"[^a-z0-9]+")
@@ -53,34 +44,13 @@ def slugify(label: str) -> str:
return _SLUG_RE.sub("-", label.lower()).strip("-") return _SLUG_RE.sub("-", label.lower()).strip("-")
# A subprocess.run-shaped callable, injectable for tests. class ProgrammaticBottleRunner:
RunFn = Callable[[Sequence[str], dict[str, str]], int] """Calls into the bot_bottle Python API directly — no subprocess.
Imports are deferred to call time so tests can inject a mock into
def _default_run(argv: Sequence[str], env: dict[str, str]) -> int: sys.modules['bot_bottle.api'] before calling runner methods.
return subprocess.run(list(argv), env=env, check=False).returncode bot_bottle.api is added in the forge-native-integration PR (#318),
which merges before this one."""
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]
def start( def start(
self, self,
@@ -90,29 +60,24 @@ class SubprocessBottleRunner:
label: str, label: str,
prompt: str, prompt: str,
forge_env: dict[str, str], forge_env: dict[str, str],
) -> RunResult: ) -> str:
argv = self._argv( from bot_bottle import api # type: ignore[import-not-found] # pylint: disable=import-error,no-name-in-module
"start", agent, "--headless", "--label", label, "--prompt", prompt 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: def freeze(self, slug: str) -> None:
# bot-bottle's `commit` snapshots a running bottle's state. from bot_bottle import api # type: ignore[import-not-found] # pylint: disable=import-error,no-name-in-module
return self._run(self._argv("commit", slug), self._base_env) api.freeze(slug)
def resume(self, slug: str, prompt: str) -> RunResult: def resume(self, slug: str, prompt: str) -> None:
code = self._run( from bot_bottle import api # type: ignore[import-not-found] # pylint: disable=import-error,no-name-in-module
self._argv("resume", slug, "--headless", "--prompt", prompt), api.resume_headless(slug, prompt=prompt)
self._base_env,
)
return RunResult(slug=slug, exit_code=code)
def destroy(self, slug: str) -> int: def destroy(self, slug: str) -> None:
# NOTE: bot-bottle `cleanup` currently targets all bottles; a from bot_bottle import api # type: ignore[import-not-found] # pylint: disable=import-error,no-name-in-module
# per-slug teardown command is a known integration follow-up api.destroy(slug)
# (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)
+1 -1
View File
@@ -106,7 +106,7 @@ class ForgeSidecar:
def dispatch(self, method: str, params: dict[str, Any]) -> dict[str, Any]: def dispatch(self, method: str, params: dict[str, Any]) -> dict[str, Any]:
try: try:
result = self._invoke(method, params) result = self._invoke(method, params)
except Exception as exc: # noqa: BLE001 — surface as JSON-RPC error except Exception as exc: # noqa: BLE001 # pylint: disable=broad-exception-caught
self._log.record(method, params.get("number"), f"error: {exc}") self._log.record(method, params.get("number"), f"error: {exc}")
return {"ok": False, "error": str(exc)} return {"ok": False, "error": str(exc)}
return {"ok": True, "result": result} return {"ok": True, "result": result}
+6 -9
View File
@@ -7,7 +7,7 @@ from __future__ import annotations
from collections.abc import Sequence from collections.abc import Sequence
from bot_bottle.orchestrator.runner import RunResult, slugify from bot_bottle.orchestrator.runner import slugify
class FakeForge: class FakeForge:
@@ -52,18 +52,15 @@ class FakeRunner:
label: str, label: str,
prompt: str, prompt: str,
forge_env: dict[str, str], forge_env: dict[str, str],
) -> RunResult: ) -> str:
self.calls.append(("start", agent, tuple(bottles), label, prompt, dict(forge_env))) 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)) 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)) 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)) self.calls.append(("destroy", slug))
return 0
@@ -28,7 +28,6 @@ def _config(tmp: str) -> Config:
watchdog_timeout_secs=1800, watchdog_timeout_secs=1800,
webhook_host="127.0.0.1", webhook_host="127.0.0.1",
webhook_port=0, webhook_port=0,
bot_bottle_cli="cli.py",
queue_dir=Path(tmp) / "q", queue_dir=Path(tmp) / "q",
sidecar_socket=Path(tmp) / "s.sock", sidecar_socket=Path(tmp) / "s.sock",
db_path=None, db_path=None,
+61 -47
View File
@@ -1,11 +1,14 @@
"""Unit: SubprocessBottleRunner + slugify (injected run fn).""" """Unit: ProgrammaticBottleRunner + slugify."""
from __future__ import annotations from __future__ import annotations
import sys
import types
import unittest import unittest
from collections.abc import Sequence from typing import Any
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): class SlugifyTest(unittest.TestCase):
@@ -17,64 +20,75 @@ class SlugifyTest(unittest.TestCase):
self.assertEqual("a-b-c", slugify(" A_B/C!! ")) self.assertEqual("a-b-c", slugify(" A_B/C!! "))
class SubprocessRunnerTest(unittest.TestCase): def _make_api_stub(**overrides: object) -> Any:
def setUp(self): """Return a mock bot_bottle.api module with sensible defaults."""
self.argvs: list[list[str]] = [] stub: Any = types.ModuleType("bot_bottle.api")
self.envs: list[dict[str, str]] = [] 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
def fake_run(argv: Sequence[str], env: dict[str, str]) -> int:
self.argvs.append(list(argv))
self.envs.append(dict(env))
return 0
self.runner = SubprocessBottleRunner( class ProgrammaticRunnerTest(unittest.TestCase):
cli="/x/cli.py", base_env={"PATH": "/bin"}, python="/py", run=fake_run def setUp(self) -> None:
) self._api: Any = _make_api_stub()
sys.modules["bot_bottle.api"] = self._api
self.runner = ProgrammaticBottleRunner()
def test_start_argv_and_env(self): def tearDown(self) -> None:
result = self.runner.start( sys.modules.pop("bot_bottle.api", None)
def test_start_returns_slug_from_api(self) -> None:
slug = self.runner.start(
agent="impl", bottles=["claude", "dev"], label="impl-r-17", agent="impl", bottles=["claude", "dev"], label="impl-r-17",
prompt="do it", forge_env={"FORGE_OWNER": "didericis"}, prompt="do it", forge_env={"FORGE_OWNER": "didericis"},
) )
self.assertEqual("impl-r-17", result.slug) self.assertEqual("impl-r-17", 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"])
def test_start_no_bottles_omits_flag(self): def test_start_forwards_all_args(self) -> None:
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) -> None:
self.runner.start(agent="impl", bottles=[], label="l", prompt="p", forge_env={}) 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) -> None:
self.runner.freeze("slug-1") 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): def test_freeze_returns_none(self) -> None:
r = self.runner.resume("slug-1", "address review") result = self.runner.freeze("slug-1")
self.assertEqual("slug-1", r.slug) self.assertIsNone(result)
self.assertEqual(
["/py", "/x/cli.py", "resume", "slug-1", "--headless", "--prompt",
"address review"], self.argvs[0])
def test_destroy_calls_cleanup(self): def test_resume_delegates_to_api(self) -> None:
code = self.runner.destroy("slug-7") self.runner.resume("slug-1", "address review")
self.assertEqual(0, code) self._api.resume_headless.assert_called_once_with("slug-1", prompt="address review")
self.assertEqual(["/py", "/x/cli.py", "cleanup", "slug-7"], self.argvs[0])
def test_resume_returns_none(self) -> None:
result = self.runner.resume("slug-1", "p")
self.assertIsNone(result)
class DefaultRunTest(unittest.TestCase): def test_destroy_delegates_to_api(self) -> None:
def test_calls_subprocess_and_returns_code(self): self.runner.destroy("slug-7")
from unittest.mock import MagicMock, patch self._api.destroy.assert_called_once_with("slug-7")
from bot_bottle.orchestrator.runner import _default_run
with patch("subprocess.run") as mock_run: def test_destroy_returns_none(self) -> None:
mock_run.return_value = MagicMock(returncode=42) result = self.runner.destroy("slug-7")
code = _default_run(["echo", "hi"], {"PATH": "/bin"}) self.assertIsNone(result)
self.assertEqual(42, code)
mock_run.assert_called_once_with(["echo", "hi"], env={"PATH": "/bin"}, check=False)
if __name__ == "__main__": if __name__ == "__main__":