From cf373e8248c7d5523c2f83b470c8171c93bc8012 Mon Sep 17 00:00:00 2001 From: sprooty Date: Wed, 5 Aug 2026 03:18:23 +0000 Subject: [PATCH] fix: an escalation lands in blocked, and does not read as a failed run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both found by running rdpapp R7 through the path added in #175. The agent refused an impossible item correctly, with citations, and the harness then mishandled the answer twice. **The state never reached the queue.** `Stop(state=BLOCKED)` was honoured only on the checks path, which copied `stop.state` across by hand; every other path left the item in `failed`. So R7 was recorded `escalated / item_impossible` with `attempts=0` — all correct — and sat in `failed`, where nothing looking for work that needs a person would ever find it. The state is now applied centrally, at the release call, beside `consumes_attempt` which was already read there. An empty `state` still means the caller chose, which is every path older than this taxonomy. **The run announced it as `FAIL R7`.** `ok` and `FAIL` were the whole vocabulary, so an outcome where nothing went wrong and no attempt was spent was reported as a failure — and set exit 1, which makes a queue of well-formed questions read to CI as a broken run. There is now a `YOU` marker, a closing `waiting on you, not on a retry` line, and `_is_failure` excludes `NEEDS_A_PERSON` from the exit status. The summary is extracted as `run_summary()` returning lines rather than printing them, because a formatting decision that only exists inside a print loop cannot be tested, and this one is a decision. One older test asserted `FAILED` for a clean tree while its own docstring said "that is a real answer, not a failure to paper over". It asserts `BLOCKED` now. Co-Authored-By: Claude Opus 5 (1M context) --- events.jsonl | 5 +++ src/agent_harness/__main__.py | 48 ++++++++++++++++++++++++--- src/agent_harness/session_executor.py | 10 ++++++ tests/test_cli_roles.py | 46 +++++++++++++++++++++++++ tests/test_session_executor.py | 29 ++++++++++++++-- 5 files changed, 131 insertions(+), 7 deletions(-) create mode 100644 events.jsonl diff --git a/events.jsonl b/events.jsonl new file mode 100644 index 0000000..50de244 --- /dev/null +++ b/events.jsonl @@ -0,0 +1,5 @@ +{"ts": 1785899243.5450313, "kind": "work", "worker": "mydevenv2-dev:3107904", "item_id": "R7", "issue": null, "outcome": "started", "error_class": null, "detail": null, "session_id": null, "session_url": null, "project_id": "rdpapp"} +{"ts": 1785899243.6182284, "kind": "work", "worker": "mydevenv2-dev:3107904", "item_id": "R7", "issue": null, "outcome": "agent_started", "error_class": null, "detail": "affa8f68-0507-446c-9281-34280bd77af3", "session_id": "affa8f68-0507-446c-9281-34280bd77af3", "session_url": "http://localhost:8910/t/affa8f68-0507-446c-9281-34280bd77af3", "project_id": "rdpapp"} +{"ts": 1785899353.6382027, "kind": "work", "worker": "mydevenv2-dev:3107904", "item_id": "R7", "issue": null, "outcome": "agent_finished", "error_class": null, "detail": null, "session_id": "affa8f68-0507-446c-9281-34280bd77af3", "session_url": null, "project_id": "rdpapp"} +{"ts": 1785899353.6437557, "kind": "work", "worker": "mydevenv2-dev:3107904", "item_id": "R7", "issue": null, "outcome": "refused_as_impossible", "error_class": null, "detail": "# Refusal: the support-bundle tests cannot build a bundle without a live Postgres\n\nThe item states its own stopping condition:\n\n> If the existing tests cannot build a bundle without a live Postgres, say so\n> and stop rather than introducing a mock \u2014 that is a correct outcome and a\n> different item.\n\nThat condition holds. Nothing else in the item is ambiguous; this one fact\nrules out delivering a test that \"passes now\" under the stated check.\n\n## What I found\n\n**The only support-bundle assertions live inside an ignored Postgres test.**\nEvery existing reference to the bundle in `crates/rdpapp-gateway/src/main.rs`\nis inside `postgres_auth_csrf_roles_and_connection_persistence`\n(`crates/rdpapp-gateway/src/main.rs:15195`), which is declared:\n\n```rust\n#[tokio::test]\n#[ignore = \"requires RDPAPP_TEST_DATABASE_URL\"]\nasync fn postgres_auth_csrf_roles_and_connection_persistence() {\n let database_url = env::var(\"RDPAPP_TEST_DATABASE_URL\")\n .expect(\"set RDPAPP_TEST_DATABASE_URL to a disposable Postgres database\");\n```\n\nIts own doc comment says the ignore is deliberate: \"Keeping this ignored in the\nordinary unit suite makes `cargo test` independent of a developer's local\nDocker setup.\" The bundle assertions run at\n`crates/rdpapp-gateway/src/main.rs:15841`-`15876`, including the existing\n`assert!(!support_bundle_text.contains(\"rotated-secret\"))` \u2014 the closest\nneighbour to what this item asks for.\n\n**The fixture is a real database, not an in-memory one.** That test builds\n`Repository::new(pool, test_vault())` from a `PgPoolOptions::connect(...)`,\nmigrates, `TRUNCATE`s, and calls `app_with_repository(...)`\n(`crates/rdpapp-gateway/src/main.rs:11996`\u2026 declared at\n`crates/rdpapp-gateway/src/main.rs:10996`). `just test-integration` (justfile\nline 25) exists precisely to bring up `docker compose ... postgres` for it.\n\n**There is no non-Postgres route to a serialised bundle.**\n`download_support_bundle` (`crates/rdpapp-gateway/src/main.rs:6915`) obtains its\ndata through `repository(&state)?`\n(`crates/rdpapp-gateway/src/main.rs:6252`), which returns the concrete\n`Repository` struct (`crates/rdpapp-gateway/src/main.rs:95`) or a\n`503 repository_unavailable` when `state.repository` is `None`. `Repository` is\na struct over a `PgPool`, not a trait, so it cannot be substituted. The snippet\nrows the bundle maps from come from\n`Repository::snippets` (`crates/rdpapp-gateway/src/main.rs:2807`), a direct\n`sqlx::query(\"SELECT id,name,text,revision FROM snippets ...\")` against\n`self.pool`. Creating a workspace snippet with a sentinel `text` and then\nserialising a bundle for that workspace \u2014 exactly what the item asks \u2014 therefore\nrequires a live Postgres.\n\n## Why this collides with the item as written\n\n- Scope point 2 requires the test to **pass now**. The judging check is\n `cargo test -p rdpapp-models -p rdpapp-sessions -p rdpapp-gateway`, which does\n not run `#[ignore]`d tests and does not set `RDPAPP_TEST_DATABASE_URL`. A test\n written against the neighbouring fixture would be skipped, not passed.\n- Scope point 4 requires reusing the neighbours' fixture rather than\n introducing a new one, and point 3 forbids any production change. So the two\n routes that would make the test run under the stated command are both closed:\n a fake/in-memory repository is the mock the item rules out, and making\n `Repository` substitutable is a production change.\n\n## What the different item would be\n\nAdding an `#[ignore = \"requires RDPAPP_TEST_DATABASE_URL\"]` sentinel test beside\n`postgres_auth_csrf_roles_and_connection_persistence`, guarding the bundle under\n`just test-integration` rather than under `cargo test`. That is a real guard and\nit is not a mock, but it does not satisfy \"passes now\" against the check this\nitem is judged by, so I have left it undone rather than quietly redefining the\nscope. Deciding whether the integration suite is the right home for this guard\nis the call the follow-up item should make.\n\nNo other change is left in the working tree.", "session_id": "affa8f68-0507-446c-9281-34280bd77af3", "session_url": null, "project_id": "rdpapp"} +{"ts": 1785899353.654138, "kind": "work", "worker": "mydevenv2-dev:3107904", "item_id": "R7", "issue": null, "outcome": "worktree_removed", "error_class": null, "detail": "reclaimed 13094912 bytes", "session_id": null, "session_url": null, "project_id": "rdpapp", "worktree_bytes": 13094912} diff --git a/src/agent_harness/__main__.py b/src/agent_harness/__main__.py index d42b414..d94c539 100644 --- a/src/agent_harness/__main__.py +++ b/src/agent_harness/__main__.py @@ -25,6 +25,7 @@ import shutil import sys import time +from collections.abc import Sequence from pathlib import Path from typing import Any @@ -777,13 +778,50 @@ def live_routes() -> dict[str, Chain]: if not outcomes: print("nothing to do") return 0 + for line in run_summary(outcomes): + print(line) + return 1 if any(_is_failure(o) for o in outcomes) else 0 + + +def _is_failure(outcome: Any) -> bool: + """Did this go wrong, as opposed to needing a person? + + An escalation is not a failure. Nothing malfunctioned, no attempt was + wasted, and the item is waiting on a decision only a human can make — so + it must not colour the exit status, or a queue full of well-formed + questions reads to CI as a broken run. + """ + from .outcomes import NEEDS_A_PERSON + + if outcome.stop is not None and outcome.stop.disposition in NEEDS_A_PERSON: + return False + return not outcome.ok + + +def run_summary(outcomes: Sequence[Any]) -> list[str]: + """The lines a finished run prints, as data so they can be tested. + + `FAIL` and `ok` were the whole vocabulary, so an item that escalated — + nothing went wrong, a person is needed — was announced as a failure. + Measured on rdpapp R7: an agent correctly refused an impossible item, with + citations and no attempt spent, and the run's last word on it was `FAIL R7`. + """ + from .outcomes import NEEDS_A_PERSON + + lines = [] + waiting = [o for o in outcomes if o.stop and o.stop.disposition in NEEDS_A_PERSON] for outcome in outcomes: - mark = "ok " if outcome.ok else "FAIL" + mark = "YOU" if outcome in waiting else ("ok " if outcome.ok else "FAIL") detail = outcome.pr_url or outcome.reason[:100] - print(f" {mark} {outcome.item_id}: {' -> '.join(outcome.stages)} {detail}") - failed = [o for o in outcomes if not o.ok] - print(f"{len(outcomes) - len(failed)}/{len(outcomes)} items completed") - return 1 if failed else 0 + lines.append(f" {mark} {outcome.item_id}: {' -> '.join(outcome.stages)} {detail}") + done = [o for o in outcomes if o.ok] + lines.append(f"{len(done)}/{len(outcomes)} items completed") + if waiting: + lines.append( + f"{len(waiting)} waiting on you, not on a retry: " + + ", ".join(o.item_id for o in waiting) + ) + return lines def _assessor(args: argparse.Namespace) -> Any: diff --git a/src/agent_harness/session_executor.py b/src/agent_harness/session_executor.py index b9aa090..285c8ae 100644 --- a/src/agent_harness/session_executor.py +++ b/src/agent_harness/session_executor.py @@ -373,6 +373,16 @@ def run_once(self) -> Outcome | None: partial.stop = stop return partial return Outcome(record.item_id, FAILED, reason=str(exc), stop=stop) + # A `Stop` that names a state means it, and it is the last word: the + # checks path used to copy `stop.state` across by hand and every other + # path forgot to, so an item that escalated with `state=BLOCKED` was + # still released as `failed`. Measured on rdpapp R7 — correctly refused, + # correctly `escalated / item_impossible` with no attempt spent, and + # sitting in `failed` where nothing looking for work that needs a + # person would ever find it. An empty `state` still means the caller + # already chose, which is every path older than this taxonomy. + if outcome.stop is not None and outcome.stop.state: + outcome.state = outcome.stop.state self.queue.release( record.item_id, outcome.state, diff --git a/tests/test_cli_roles.py b/tests/test_cli_roles.py index e7c987c..2c1ab57 100644 --- a/tests/test_cli_roles.py +++ b/tests/test_cli_roles.py @@ -317,3 +317,49 @@ def test_the_cli_agent_default_is_the_executor_s() -> None: __import__("agent_harness.session_executor", fromlist=["x"]) ), "and that command must still grant edit permission" assert "--permission-mode" in " ".join(DEFAULT_AGENT_COMMAND) + + +# ------------------------------------- an escalation is not a failed run + + +def _outcome(item_id: str, *, ok: bool, disposition: str = "", reason: str = "") -> Any: + from agent_harness.executor import Outcome + from agent_harness.outcomes import Stop + + out = Outcome(item_id, "done" if ok else "blocked", reason=reason) + if disposition: + out.stop = Stop(disposition, detail=reason) + return out + + +def test_an_escalated_item_is_marked_for_a_person_not_as_a_failure() -> None: + """Measured on rdpapp R7: a correct refusal was announced as `FAIL R7`.""" + from agent_harness.__main__ import run_summary + from agent_harness.outcomes import ESCALATED + + lines = run_summary( + [_outcome("R7", ok=False, disposition=ESCALATED, reason="needs a live Postgres")] + ) + + assert lines[0].startswith(" YOU R7:") + assert "FAIL" not in "\n".join(lines) + assert "waiting on you, not on a retry: R7" in lines[-1] + + +def test_a_real_failure_still_reads_as_one() -> None: + from agent_harness.__main__ import run_summary + from agent_harness.outcomes import REFUSED + + lines = run_summary([_outcome("R2", ok=False, disposition=REFUSED, reason="review rejected")]) + + assert lines[0].startswith(" FAIL R2:") + assert "waiting on you" not in "\n".join(lines) + + +def test_an_escalation_does_not_colour_the_exit_status() -> None: + """A queue of well-formed questions is not a broken run.""" + from agent_harness.__main__ import _is_failure + from agent_harness.outcomes import ESCALATED, REFUSED + + assert _is_failure(_outcome("R7", ok=False, disposition=ESCALATED)) is False + assert _is_failure(_outcome("R2", ok=False, disposition=REFUSED)) is True diff --git a/tests/test_session_executor.py b/tests/test_session_executor.py index 4414c42..1680ea3 100644 --- a/tests/test_session_executor.py +++ b/tests/test_session_executor.py @@ -330,13 +330,18 @@ def test_a_nonzero_exit_fails_the_item_without_reviewing(repo: Path, tmp_path: P def test_an_agent_that_changed_nothing_is_reported_as_such(repo: Path, tmp_path: Path) -> None: """A CLI agent that decided the task was impossible leaves a clean tree. - That is a real answer, not a failure to paper over.""" + That is a real answer, not a failure to paper over. + + This test used to assert `FAILED`, which is what the docstring above spent + two lines saying it was not. It is `BLOCKED` now: a real answer nobody can + act on without a person is what `blocked` is for. + """ devenv = FakeDevEnv(agent=None) executor, queue = build(repo, tmp_path, devenv) add_item(queue) outcome = executor.run_once() assert outcome is not None - assert outcome.state == FAILED + assert outcome.state == BLOCKED assert "no changes" in outcome.reason @@ -844,3 +849,23 @@ def read_prompt(tree: Path) -> None: executor.run_once() assert REFUSAL_FILE in seen[0] + + +def test_a_refusal_lands_in_blocked_not_failed(repo: Path, tmp_path: Path) -> None: + """Measured on rdpapp R7, and the reason the Stop carries a state at all. + + Disposition, reason kind and attempt count were all correct and the item + still sat in `failed`, because only the checks path copied `stop.state` + across. An escalation filed under `failed` is invisible to exactly the + person it was raised for. + """ + executor, queue = build(repo, tmp_path, FakeDevEnv(agent=refuse("needs a live database"))) + add_item(queue) + + outcome = executor.run_once() + + assert outcome is not None + assert outcome.state == BLOCKED + item = queue.get("W1") + assert item is not None + assert item.state == BLOCKED