From 991cf075620c0777e7d19029d11098ad9ea44d48 Mon Sep 17 00:00:00 2001 From: sprooty Date: Wed, 5 Aug 2026 03:37:45 +0000 Subject: [PATCH] fix: a rewritten brief returns a stalled item to the queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The other half of the loop #174 opened. An agent can now say "this item cannot be done as written" and have that recorded as work needing a person — and until this, the person acting on it changed nothing: R7 -> pending [already failed in the queue] applied refresh queue row item R7 in project rdpapp: update title, brief and dependencies; state stays failed The heading is the proposal and the line below it is what happened. The brief updated, the item stayed unclaimable, and the next run said `nothing to do`. Not resetting state on a refresh is the right instinct — `adopt` must not silently un-fail work, and an item that failed its checks five times should not become pending because someone fixed a typo in its title. But a changed **brief** is a different item in the only sense that matters: the attempt that failed was made against wording that no longer exists, so keeping the verdict records a decision about a question nobody is asking. So `revives()` is deliberately narrow: - only `failed` and `blocked`. `done` is not stalled, and editing the description of finished work does not un-finish it; - only the brief. A changed title, label or dependency moves nothing; - compared stripped, so re-parsing a plan cannot revive an item because a newline moved. `last_error` is cleared with the state. The next attempt is told what the previous one was refused for, and that refusal is about wording it can no longer reproduce. The attempt *count* is untouched: what changed is the question, not the fact that it was once attempted. The report line no longer contradicts itself two lines apart — it says either "state returns to pending from failed" or "state stays failed", whichever is about to be true. One existing test seeded an item with no brief at all and then adopted a plan that supplied one, which is a rewrite under this rule. Reseeded with the plan's brief so it tests the unchanged re-sync its name claims, with the revive case pinned separately rather than the original assertion loosened. Closes #178. Co-Authored-By: Claude Opus 5 (1M context) --- src/agent_harness/adoption.py | 32 +++++++++++-- src/agent_harness/work.py | 42 +++++++++++++++++- tests/test_adoption.py | 54 +++++++++++++++++++++- tests/test_work.py | 84 +++++++++++++++++++++++++++++++++++ 4 files changed, 206 insertions(+), 6 deletions(-) diff --git a/src/agent_harness/adoption.py b/src/agent_harness/adoption.py index 48fdc76..b94d028 100644 --- a/src/agent_harness/adoption.py +++ b/src/agent_harness/adoption.py @@ -25,8 +25,17 @@ without citations, a verification that failed — biases towards `not_started`. **Prior harness attempts are evidence, not authority.** An item the queue has -already failed keeps its attempts, its error and its event history. Adoption -reports the prior failure so a human can decide; it never rewrites it. +already failed keeps its attempts and its event history. Adoption reports the +prior failure so a human can decide; it never overrules it. + +**Except when the question changed.** A refresh that **rewrites an item's +brief** returns it to `pending` from `failed` or `blocked`, and clears the +error with it. The attempt that failed was made against wording that no longer +exists, so keeping the verdict records a decision about a question nobody is +asking. This is the second half of the loop that lets an agent refuse an +impossible item: without it, a human who rewrites the item in response is told +"nothing to do" (#178). Nothing else moves state — a changed title, label or +dependency leaves it exactly where it was. """ from __future__ import annotations @@ -44,7 +53,7 @@ from .github import MARKER, GitHub from .outcomes import ESCALATE, RETRY from .plan import ParsedPlan, WorkItem -from .work import CLAIMED, DONE, PENDING, Project, WorkQueue, WorkRecord +from .work import CLAIMED, DONE, PENDING, Project, WorkQueue, WorkRecord, revives #: The lifecycle of one adoption (proposal §5.1). `rejected` and `revise` are #: the two ways out of `proposed` that do not mutate anything. @@ -183,6 +192,9 @@ class AdoptionItem: #: What the queue already says about this item, if anything. Adoption #: reports it rather than overwriting it. queue_state: str | None = None + #: The brief the queue is holding, so the report can tell a re-sync that + #: changes nothing from one that rewrites the item. + queue_brief: str | None = None proposed_state: str = PENDING evidence: list[Evidence] = field(default_factory=list) candidates: list[ExternalCandidate] = field(default_factory=list) @@ -582,6 +594,7 @@ def _inspect_item( brief=item.brief(), depends_on=list(item.depends_on), queue_state=existing.state if existing else None, + queue_brief=existing.brief if existing else None, candidates=list(candidates), ) @@ -700,8 +713,19 @@ def _add_mutations(self, project_id: str, result: AdoptionItem) -> None: ProposedMutation( kind="refresh queue row", target=f"item {result.item_id} in project {project_id}", + # The report used to say `R7 -> pending` in its heading and + # `state stays failed` in this line, which is the report + # contradicting itself two lines apart. Say what will + # happen: a rewritten brief revives a stalled item, and + # nothing else moves it. detail=( - f"update title, brief and dependencies; state stays {result.queue_state}" + "update title, brief and dependencies; " + + ( + f"the brief changed, so state returns to pending " + f"from {result.queue_state}" + if revives(result.queue_state or "", result.queue_brief, result.brief) + else f"state stays {result.queue_state}" + ) ), ) ) diff --git a/src/agent_harness/work.py b/src/agent_harness/work.py index aae03c3..5a0b28a 100644 --- a/src/agent_harness/work.py +++ b/src/agent_harness/work.py @@ -318,6 +318,28 @@ def _run(self) -> None: return +#: States a rewritten brief rescues an item from. `done` is absent on purpose — +#: editing the description of finished work does not un-finish it — and so are +#: `claimed` and `pending`, which are not stuck. +STALLED = frozenset({FAILED, BLOCKED}) + + +def revives(state: str, was: str | None, now: str | None) -> bool: + """Does this refresh un-stick a stalled item? + + Only a changed **brief** does. Re-syncing an unchanged plan must leave + every state exactly where it was, and fixing a typo in a title or adding a + label is not an answer to whatever stopped the item. + + A changed brief is different in the one way that matters: the attempt that + failed was made against wording that no longer exists. Leaving it `failed` + records a verdict on a question nobody is asking any more — and it is + silent, so a person who rewrites an item in response to an agent refusing + it as impossible (#174) watches the next run say "nothing to do" (#178). + """ + return state in STALLED and (was or "").strip() != (now or "").strip() + + def _process_alive(pid: int) -> bool: """Whether a pid on this host is still running. @@ -782,7 +804,7 @@ def add( ) for record in records: existing = conn.execute( - "SELECT state FROM work WHERE project_id = ? AND item_id = ?", + "SELECT state, brief FROM work WHERE project_id = ? AND item_id = ?", (project_id, record.item_id), ).fetchone() if existing is None: @@ -815,6 +837,24 @@ def add( record.item_id, ), ) + if revives(existing["state"], existing["brief"], record.brief): + # The verdict that stopped this item was reached against + # wording that no longer exists, so it is no longer a + # verdict on anything. Clearing `last_error` with it: the + # next attempt is told what the previous one was refused + # for, and that refusal is about a question nobody is + # asking any more. + conn.execute( + "UPDATE work SET state = ?, last_error = NULL " + "WHERE project_id = ? AND item_id = ?", + (PENDING, project_id, record.item_id), + ) + log.info( + "%s/%s: brief rewritten, returning it to pending from %s", + project_id, + record.item_id, + existing["state"], + ) self.graph.set_edges( project_id, record.item_id, diff --git a/tests/test_adoption.py b/tests/test_adoption.py index 1d5f6a1..4e03b2d 100644 --- a/tests/test_adoption.py +++ b/tests/test_adoption.py @@ -589,7 +589,19 @@ def test_prior_harness_attempts_are_evidence_and_history_is_retained( ) -> None: audit = AuditStore(tmp_path / "audit.sqlite") sink = event_sink(audit, source="stage-c") - queue.add([WorkRecord(item_id="T5", title="Never started")], project_id="existing") + # Seeded with the brief the plan carries, so this is a re-sync that changes + # nothing. A refresh that *rewrites* the brief deliberately does move the + # state, which the test below this one covers. + queue.add( + [ + WorkRecord( + item_id="T5", + title="Never started", + brief="Never started\n\nNothing anywhere refers to this.", + ) + ], + project_id="existing", + ) queue.set_control("running", project_id="existing") claimed = queue.claim("worker-1", project_id="existing") assert claimed is not None and claimed.item_id == "T5" @@ -892,3 +904,43 @@ def test_adopt_cli_dry_run_leaves_no_trace(tmp_path: Path, repo: Path, capsys: A stored = WorkQueue(str(db)) assert stored.items() == [] assert stored.get_setting("adoption:existing") is None + + +def test_a_rewritten_brief_revives_a_failed_item_and_the_report_says_so( + tmp_path: Path, queue: WorkQueue, repo: Path +) -> None: + """#178: the half of the refusal loop that acts on the human's answer. + + The report used to say `T5 -> pending` in its heading and `state stays + failed` in the mutation line two lines below, and the second one was true. + A human who rewrote an item in response to an agent refusing it as + impossible was then told "nothing to do". + """ + queue.add( + [WorkRecord(item_id="T5", title="Never started", brief="the original wording")], + project_id="existing", + ) + queue.set_control("running", project_id="existing") + queue.claim("worker-1", project_id="existing") + queue.release( + "T5", FAILED, error="cannot be done as specified", owner="worker-1", project_id="existing" + ) + + adopter = adoption(queue, repo) + report = adopter.inspect("existing", parse_plan(PLAN)) + + t5 = by_id(report)["T5"] + refresh = next(m for m in t5.mutations if m.kind == "refresh queue row") + assert "state returns to pending" in refresh.detail + assert "state stays" not in refresh.detail + + adopter.approve("existing", approved_drops=[]) + adopter.reconcile("existing") + + record = queue.get("T5", project_id="existing") + assert record is not None + assert record.state == PENDING + assert not record.last_error + # The attempt itself is history and stays history. What changed is the + # question, not the fact that it was once attempted. + assert record.attempts == 1 diff --git a/tests/test_work.py b/tests/test_work.py index 457d621..bd5feb3 100644 --- a/tests/test_work.py +++ b/tests/test_work.py @@ -8,9 +8,11 @@ import pytest from agent_harness.graph import ResolverOutcome +from agent_harness.outcomes import BLOCKED from agent_harness.work import ( CLAIMED, DONE, + FAILED, PENDING, WorkQueue, WorkRecord, @@ -329,3 +331,85 @@ def test_settings_are_shared_across_processes(tmp_path: Path) -> None: def test_an_unset_setting_is_none_not_an_error(queue: WorkQueue) -> None: assert queue.get_setting("nope") is None + + +# ------------------------------- a rewritten brief revives a stalled item + + +def _stalled(tmp_path: Path, state: str, *, brief: str = "the brief") -> WorkQueue: + """An item the queue has stopped, ready for a plan re-sync to land on.""" + queue = WorkQueue(str(tmp_path / "w.sqlite")) + queue.add([WorkRecord(item_id="R7", title="t", brief=brief)]) + queue.set_control("running") + queue.claim(owner="w") + queue.release("R7", state, owner="w", error="cannot be done as specified") + return queue + + +def test_a_rewritten_brief_returns_a_blocked_item_to_pending(tmp_path: Path) -> None: + """The second half of the refusal loop (#178). + + An agent refuses an item as impossible, a human rewrites it in response, + and until this the rewrite was inert: the brief updated, the item stayed + put, and the next run said "nothing to do". + """ + queue = _stalled(tmp_path, BLOCKED) + + queue.add([WorkRecord(item_id="R7", title="t", brief="an achievable thing instead")]) + + item = queue.get("R7") + assert item is not None + assert item.state == PENDING + assert item.brief == "an achievable thing instead" + # Feeding the old refusal to the next attempt would be telling it not to + # repeat a fault it can no longer commit. + assert not item.last_error + + +def test_a_rewritten_brief_also_revives_a_failed_item(tmp_path: Path) -> None: + queue = _stalled(tmp_path, FAILED) + + queue.add([WorkRecord(item_id="R7", title="t", brief="something else entirely")]) + + item = queue.get("R7") + assert item is not None + assert item.state == PENDING + + +def test_an_unchanged_brief_leaves_a_failed_item_where_it_was(tmp_path: Path) -> None: + """A routine re-sync must not un-fail work, and a better title is not an answer.""" + queue = _stalled(tmp_path, FAILED) + + queue.add([WorkRecord(item_id="R7", title="a better title", brief="the brief")]) + + item = queue.get("R7") + assert item is not None + assert item.state == FAILED + assert item.title == "a better title" + assert item.last_error == "cannot be done as specified" + + +def test_whitespace_alone_is_not_a_rewrite(tmp_path: Path) -> None: + """Re-parsing a plan must not revive an item because a newline moved.""" + queue = _stalled(tmp_path, FAILED) + + queue.add([WorkRecord(item_id="R7", title="t", brief=" the brief\n")]) + + item = queue.get("R7") + assert item is not None + assert item.state == FAILED + + +def test_rewriting_the_brief_of_finished_work_does_not_unfinish_it(tmp_path: Path) -> None: + """`done` is not stalled, and editing a description is not a reason to redo it.""" + queue = WorkQueue(str(tmp_path / "w.sqlite")) + queue.add([WorkRecord(item_id="R1", title="t", brief="the brief")]) + queue.set_control("running") + queue.claim(owner="w") + queue.release("R1", DONE, owner="w") + + queue.add([WorkRecord(item_id="R1", title="t", brief="a completely different brief")]) + + item = queue.get("R1") + assert item is not None + assert item.state == DONE