diff --git a/src/agent_harness/__main__.py b/src/agent_harness/__main__.py index 1eba4d6..97b1b03 100644 --- a/src/agent_harness/__main__.py +++ b/src/agent_harness/__main__.py @@ -2247,8 +2247,27 @@ def routes_for(project_id: str) -> dict[str, Chain]: required_roles = {"reviewer"} if not direct_mode else {"planner", "implementer", "reviewer"} if direct_mode and not required_roles <= set(routes): missing = ", ".join(sorted(required_roles - set(routes))) - print(f"local fleet: no route for role(s): {missing}", file=sys.stderr) - raise SystemExit(2) + # Not fatal, for the same reason the reviewer case below is not, and + # this used to exit(2) three lines from that argument. + # + # `serve` is a supervised deployment. Exiting means the API and the + # GUI never come up, so nobody can read *why* — and a process manager + # restarts it into the same missing configuration for ever. Observed + # on the Node B deployment: a container restarting every 60 seconds, + # with the one sentence explaining it visible only to whoever thought + # to read container logs. + # + # Nothing unsafe is allowed by starting. Preflight already refuses to + # start a project whose roles are not routed, and a fleet with no + # routes claims nothing. Coming up means `/api/readiness` can say what + # is missing, which is the whole point of having it. + print( + f"warning: local fleet has no route for role(s): {missing}. " + "Monitoring and every read work; preflight will refuse to start a " + "project until they are routed — set them with --planner/--implementer/" + "--reviewer and --endpoint, or PUT /api/roles.", + file=sys.stderr, + ) if "reviewer" not in routes: # Not fatal, and not silent: preflight blocks the start with exactly # this reason, so the fleet may as well exist and say why now. diff --git a/src/agent_harness/adapters/docker.py b/src/agent_harness/adapters/docker.py index 950d36a..7de1c39 100644 --- a/src/agent_harness/adapters/docker.py +++ b/src/agent_harness/adapters/docker.py @@ -61,13 +61,18 @@ def _docker(self, *args: str, timeout: float | None = None) -> subprocess.Comple raise DockerEnvironmentError(f"docker command failed to start: {exc}") from exc def check(self) -> tuple[bool, str]: + """The same answer the factory gives, from the same helpers. + + This was a second, divergent copy of the daemon check, and it kept the + `--format` defect after the factory's copy was fixed: an unreachable + daemon reported a Go template error instead of naming the daemon. + """ if shutil.which("docker") is None: return False, "docker is not installed or is not on PATH" - result = self._docker("info", "--format", "{{.ServerVersion}}", timeout=5) + result = self._docker("info", timeout=10) if result.returncode != 0: - detail = result.stderr.strip().splitlines()[-1:] or ["Docker daemon is unavailable"] - return False, detail[0] - return True, f"Docker daemon {result.stdout.strip() or 'available'}" + return False, f"Docker daemon is unreachable: {_diagnosis(result.stderr)}" + return True, f"Docker daemon {_server_version(result.stdout) or 'available'}" def _ensure_started(self) -> None: if self.started: @@ -139,7 +144,16 @@ def run(self, command: str, *, cwd: Path, timeout: int) -> EnvironmentResult: target = "/workspace" if str(relative) == "." else f"/workspace/{relative}" # `timeout` is inside the container so a client-side timeout cannot # leave a test process running after docker exec has gone away. - wrapped = f"timeout --signal=TERM {timeout}s /bin/sh -lc {shlex.quote(command)}" + # + # `-s TERM` and bare seconds, not `--signal=TERM 30s`: the long option + # and the unit suffix are GNU coreutils, and an agent image is not + # required to ship those. Against BusyBox -- Alpine, which is the + # small acceptance image -- the GNU form made EVERY command fail with + # `timeout: unrecognized option: signal=TERM` and returncode 1, which + # reads as the agent's command failing rather than the harness's own + # wrapper being unportable. Found on the first live run against a real + # daemon. Both GNU and BusyBox accept this form. + wrapped = f"timeout -s TERM {timeout} /bin/sh -lc {shlex.quote(command)}" result = self._docker( "exec", "--user", diff --git a/tests/test_execution_environment.py b/tests/test_execution_environment.py index 528b5d0..9be30cf 100644 --- a/tests/test_execution_environment.py +++ b/tests/test_execution_environment.py @@ -133,11 +133,78 @@ def fake_run(argv: list[str], **kwargs: Any) -> subprocess.CompletedProcess[str] assert "SAFE=yes" in create assert "TOKEN" not in " ".join(create) exec_call = next(call for call in calls if call[1] == "exec") - assert "7s" in " ".join(exec_call) + # Portable `timeout`, not GNU's. An agent image is not required to ship + # coreutils, and asserting the GNU spelling is what let the unportable + # form reach a real daemon (see the BusyBox test below). + assert "timeout -s TERM 7 " in " ".join(exec_call) assert result.stdout == "ok\n" assert any(call[1:4] == ["rm", "--force", "--volumes"] for call in calls) +def test_the_command_timeout_works_on_busybox_not_only_gnu( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The wrapper must not assume GNU coreutils inside the agent image. + + The first live run against a real daemon used Alpine, whose BusyBox + `timeout` rejects `--signal=TERM` and a `30s` suffix. Every command in the + sandbox returned 1 with `timeout: unrecognized option: signal=TERM`, which + reads as the agent's command failing rather than the harness's wrapper + being unportable -- the exact misattribution this repository keeps paying + for. `-s TERM` with bare seconds is accepted by both implementations. + """ + calls: list[list[str]] = [] + + def fake_run(argv: list[str], **kwargs: Any) -> subprocess.CompletedProcess[str]: + calls.append(argv) + if argv[1] == "inspect": + return subprocess.CompletedProcess(argv, 0, "sha256:resolved\n", "") + return subprocess.CompletedProcess(argv, 0, "container-id\n", "") + + monkeypatch.setattr("agent_harness.adapters.docker.shutil.which", lambda _: "/usr/bin/docker") + monkeypatch.setattr("agent_harness.adapters.docker.subprocess.run", fake_run) + item = tmp_path / "item" + item.mkdir() + environment = DockerItemEnvironment( + EnvironmentSpec(image="alpine:3.21", worktree=item.resolve(), network="none") + ) + + environment.start() + environment.run("printf ok", cwd=item, timeout=30) + + wrapper = " ".join(next(call for call in calls if call[1] == "exec")) + assert "timeout -s TERM 30 " in wrapper + assert "--signal" not in wrapper, "GNU-only long option is back" + assert "30s" not in wrapper, "GNU-only unit suffix is back" + + +def test_the_item_environment_and_its_factory_report_the_daemon_the_same_way( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Two copies of one check drifted, and only one of them was fixed.""" + + def fake_run(argv: list[str], **kwargs: Any) -> subprocess.CompletedProcess[str]: + return subprocess.CompletedProcess( + argv, + 1, + "", + "Cannot connect to the Docker daemon at unix:///var/run/docker.sock.\n" + 'template: :1:2: executing "" at <.ServerVersion>: reflect: ' + "indirection through nil pointer to embedded struct field Info\n", + ) + + monkeypatch.setattr("agent_harness.adapters.docker.shutil.which", lambda _: "/usr/bin/docker") + monkeypatch.setattr("agent_harness.adapters.docker.subprocess.run", fake_run) + item = tmp_path / "item" + item.mkdir() + environment = DockerItemEnvironment(EnvironmentSpec(image="alpine:3.21", worktree=item)) + + for ok, detail in (environment.check(), DockerEnvironmentFactory().check()): + assert ok is False + assert "Cannot connect to the Docker daemon" in detail + assert "reflect:" not in detail and "template:" not in detail + + def test_docker_reaps_only_containers_for_the_requested_worktree( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/test_serve_fleet.py b/tests/test_serve_fleet.py index 758a5f6..a3f16da 100644 --- a/tests/test_serve_fleet.py +++ b/tests/test_serve_fleet.py @@ -616,3 +616,68 @@ def test_serve_event_sink_writes_live_telemetry_to_the_audit_store(tmp_path: Pat assert errors["total"] == 1 assert errors["by_role"][0]["key"] == "reviewer" assert errors["by_endpoint"][0]["key"] == "https://e" + + +def test_a_local_fleet_without_routes_serves_instead_of_crash_looping( + repo: Path, tmp_path: Path, capsys: Any, monkeypatch: Any +) -> None: + """Observed on the Node B deployment, and fixed here. + + A container configured for a local fleet with no routes exited 2, so a + process manager restarted it every 60 seconds and the one sentence + explaining why was visible only in container logs -- the API and GUI never + came up to say it. Nothing unsafe is permitted by starting: preflight + still refuses a project whose roles are not routed, and a fleet with no + routes claims nothing. The reviewer-only case three lines away already + took this decision; this makes the two agree. + """ + from agent_harness import execution_environments, role_runners + + class Backend: + name = "fixture-host" + api_version = 1 + version = "test" + + def check(self) -> tuple[bool, str]: + return True, "fixture environment available" + + class Runner: + name = "fixture-runner" + api_version = 1 + version = "test" + + monkeypatch.setattr(role_runners, "resolve", lambda _name: Runner()) + monkeypatch.setattr(role_runners, "describe", lambda _name: "fixture-runner test") + monkeypatch.setattr(execution_environments, "resolve", lambda _name: Backend()) + + queue = WorkQueue(str(tmp_path / "w.sqlite")) + queue.add_project(Project(project_id="p", name="P", work_dir=str(repo))) + args = argparse.Namespace( + session_host="", + role_runner="fixture-runner", + environment_backend="fixture-host", + environment_image="fixture-image", + environment_network="bridge", + environment_mount=[], + reviewer="", + planner="", + implementer="", + endpoint="", + preset="", + events=tmp_path / "events.jsonl", + db=str(tmp_path / "w.sqlite"), + agent="", + no_push=True, + poll=1.0, + runner_step_limit=10, + runner_command_timeout=30, + ) + + fleet, _client, _host, _roles = _fleet_for_serve(args, queue) + + assert fleet is not None, "serve refused to come up with no routes configured" + warning = capsys.readouterr().err + assert "no route for role(s)" in warning + assert "implementer" in warning and "planner" in warning + # The operator is told what to do about it, not merely what is wrong. + assert "PUT /api/roles" in warning