From f4973a3d97e48b1ae36c5b334356cc75581d14f5 Mon Sep 17 00:00:00 2001 From: sprooty Date: Wed, 5 Aug 2026 02:33:11 +0000 Subject: [PATCH] feat: a reviewer's surplus insight becomes the next item, not a refusal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two items were rejected having met every criterion their briefs stated, both because the reviewer would additionally have done something else: "the other two bundle entry points have no explanation at all" "the code does not demonstrate any explicit guard against future accidental serialization of Snippet::text" Both are good observations. Neither was in the item. Rejecting on them discarded the work *and* the observation: the item goes back to be rewritten identically, and nothing records what was noticed. So the rubric now asks for them. When every stated criterion is met the reviewer approves and lists what else it would have done under Follow-ups, and those are kept as plan-shaped markdown for a person to accept or discard — plus an event each, because the stream is the source of truth and a coordinator should not need to know where a file is. **Only on approval, and that is the whole safety property.** A rejection has already said what is wrong and gets no second channel; allowing follow-ups there is how "approve and defer" grows into a way to wave a failed criterion through. The prompt says a criterion the task states and the change does not meet is still a rejection and no follow-up substitutes for one, and a test asserts a rejection produces nothing. Two smaller boundaries, both tested. "- none" is an answer, not an item: a backlog filled with the absence of findings is worse than an empty one. And prose after the list ends the list — attributing a proposal the reviewer did not make is worse than missing one, because a person triages it, finds nothing behind it, and trusts the next one less. A plan document rather than queue rows, for the reason `inception` produces a PLAN.md: writing straight to the queue forks the pipeline into a generated path and a hand-written one that diverge forever. Both executors share the parser and the recorder, so this cannot drift the way the review rubric did (#167). Closes #171. Co-Authored-By: Claude Opus 5 (1M context) --- src/agent_harness/__main__.py | 1 + src/agent_harness/executor.py | 111 ++++++++++++++++++++++++++ src/agent_harness/session_executor.py | 15 +++- tests/test_reviewer.py | 85 ++++++++++++++++++++ 4 files changed, 211 insertions(+), 1 deletion(-) diff --git a/src/agent_harness/__main__.py b/src/agent_harness/__main__.py index 089eead..d42b414 100644 --- a/src/agent_harness/__main__.py +++ b/src/agent_harness/__main__.py @@ -725,6 +725,7 @@ def live_routes() -> dict[str, Chain]: base_branch=args.base, ui_base_url=args.session_host, context_budget=args.context_budget, + follow_ups=artifacts, on_event=emit, push=not args.no_push, project_id=args.project, diff --git a/src/agent_harness/executor.py b/src/agent_harness/executor.py index 14c33f4..2176fbd 100644 --- a/src/agent_harness/executor.py +++ b/src/agent_harness/executor.py @@ -1293,6 +1293,27 @@ def is_disk_exhaustion(detail: str) -> bool: as not included is genuinely unavailable to you, and "the diff does not show it" is not a reason when the file does. 3. **Why** — one paragraph. +4. **Follow-ups** — only when you APPROVED. One per line, each starting with + `- `. Anything you noticed that this task did not ask for and that you + believe is worth doing: a gap the change leaves, a hazard it does not + introduce but does not close, work its existence implies. Write `- none` + if there is nothing. + + These become **proposed work items for a person to accept or discard**. + They are not conditions on this change and nothing waits for them. + +## A follow-up is not a rejection + +If the change meets every criterion the task states, approve it — and put +what else you would have done under Follow-ups. + +"It should also have done X" is a proposal. Refusing work that did what it was +asked, because you would additionally have done something else, discards the +work *and* the observation: the item goes back to be rewritten identically, +and nothing records what you noticed. + +This does not soften anything below. A criterion the task states and the +change does not meet is a rejection, and no follow-up substitutes for one. ## Reject if @@ -1378,6 +1399,89 @@ def review_reason(verdict_text: str, limit: int = 1200) -> str: return "…" + tail.strip() +def parse_follow_ups(verdict_text: str) -> tuple[str, ...]: + """What the reviewer would also have done, from its own answer. + + Only the bullets under the Follow-ups heading, and only ones with content: + a reviewer that writes "- none" has answered the question, and turning that + into an item would fill a backlog with the absence of findings. + + Deliberately forgiving about the heading — models vary on `## Follow-ups`, + `4. **Follow-ups**` and `**Follow-ups**` — and deliberately strict about + what counts as an entry, because the cost of a false one is a person + triaging noise. + """ + lowered = verdict_text.lower() + marker = lowered.rfind("follow-up") + if marker == -1: + return () + found: list[str] = [] + for raw in verdict_text[marker:].splitlines()[1:]: + line = raw.strip() + if not line: + continue + if not line.startswith(("- ", "* ")): + # The section has ended. A reviewer that carries on in prose has + # stopped listing, and swallowing that as an item would attribute + # a proposal it did not make. + break + entry = line[2:].strip().strip("*_` ") + if not entry or entry.lower() in {"none", "n/a", "nothing", "none."}: + continue + found.append(entry) + return tuple(found) + + +def record_follow_ups( + directory: Any, + project_id: str, + item_id: str, + verdict: str, + verdict_text: str, + now: float, +) -> tuple[str, ...]: + """Keep what the reviewer would also have done, when it approved. + + **Only on approval, and that is the whole safety property.** A rejection + has already said what is wrong and needs no second channel; allowing + follow-ups there would let "approve and defer" grow into a way to wave a + failed criterion through, which is the gate-weakening this exists to avoid. + """ + if verdict != APPROVED or directory is None: + return () + found = parse_follow_ups(verdict_text) + if not found: + return () + with contextlib.suppress(Exception): + path = Path(directory) + path.mkdir(parents=True, exist_ok=True) + target = path / "FOLLOW-UPS.md" + with target.open("a", encoding="utf-8") as handle: + handle.write(render_follow_ups(project_id, item_id, found, now)) + return found + + +def render_follow_ups(project_id: str, item_id: str, follow_ups: Sequence[str], now: float) -> str: + """The proposals as plan-shaped markdown, for a person to accept. + + A plan document, not queue rows. Writing straight to the queue would fork + the pipeline into a generated path and a hand-written one that diverge + forever — the same reasoning that keeps `inception` producing a `PLAN.md`. + Nothing here is admitted to anything until a human moves it into a plan. + """ + lines = [ + f"### Follow-ups from reviewing {item_id} ({project_id})", + "", + "Proposed by the reviewer while approving that item. **Nothing is", + "queued and nothing waits on these** — move one into a plan to make it", + "work, or delete it.", + "", + ] + lines.extend(f"- {entry}" for entry in follow_ups) + lines.append("") + return "\n".join(lines) + + def review_context(repo: Path, diff: str, budget: int) -> str: """The touched files as they now stand, for the reviewer. @@ -2427,6 +2531,13 @@ def _review_stage( mode=mode, ) outcome.verdict = verdict + for proposal in record_follow_ups( + self.artifacts, self.project_id, record.item_id, verdict, verdict_text, self.now() + ): + # An event as well as a file: the stream is the source of truth, + # and a coordinator can read this without knowing where the file + # is. + self._emit(record, "follow_up_proposed", detail=proposal) self._emit( record, f"review_{verdict}", diff --git a/src/agent_harness/session_executor.py b/src/agent_harness/session_executor.py index c897868..745007b 100644 --- a/src/agent_harness/session_executor.py +++ b/src/agent_harness/session_executor.py @@ -139,7 +139,7 @@ #: items were rejected for exactly the artefacts those fixes had already #: retired — in the other file (#167). from .executor import REVIEW_PROMPT as REVIEW_PROMPT # noqa: E402 -from .executor import review_reason # noqa: E402 +from .executor import record_follow_ups, review_reason # noqa: E402 @dataclass @@ -175,6 +175,7 @@ def __init__( branch_prefix: str = "harness/", worktrees: Path | None = None, context_budget: int = DEFAULT_CONTEXT_BUDGET, + follow_ups: Path | None = None, ui_base_url: str = "", session_max_age: float = DEFAULT_MAX_AGE_SECONDS, on_event: Callable[[dict[str, Any]], None] | None = None, @@ -207,6 +208,9 @@ def __init__( #: The headless executor has taken this from configuration since #150. #: Session mode reviewing large files needs the same lever. self.context_budget = context_budget + #: Where a reviewer's "it should also have done X" is kept, when it + #: approved. None keeps nothing and changes nothing. + self.follow_ups = follow_ups self.ui_base_url = ui_base_url self.session_max_age = session_max_age self.on_event = on_event @@ -648,6 +652,15 @@ def _execute(self, record: WorkRecord) -> Outcome: verdict_text = self._review(record, tree, True, "", base=base) outcome.stages.append("review") verdict = APPROVED if verdict_text.strip().upper().startswith("APPROVED") else REJECTED + for proposal in record_follow_ups( + self.follow_ups, + self.project_id, + record.item_id, + verdict, + verdict_text, + time.time(), + ): + self._emit(record, "follow_up_proposed", detail=proposal, session_id=session.id) outcome.verdict = verdict self._emit( record, f"review_{verdict}", detail=verdict_text[:2000], session_id=session.id diff --git a/tests/test_reviewer.py b/tests/test_reviewer.py index 418c0ea..7294a11 100644 --- a/tests/test_reviewer.py +++ b/tests/test_reviewer.py @@ -299,3 +299,88 @@ def test_the_session_reviewer_budget_is_configurable(tmp_path: Any) -> None: assert default.context_budget == DEFAULT_CONTEXT_BUDGET, "unchanged by default" assert raised.context_budget == 700_000, "and a deployment can raise it" + + +# ------------------------------- a follow-up is not a refusal (#171) + + +def test_surplus_insight_is_kept_when_the_reviewer_approves(tmp_path: Any) -> None: + """A reviewer's "it should also have done X" is a proposal, not a + condition. Refusing work that did what it was asked discards the work + *and* the observation — the item goes back to be rewritten identically and + nothing records what was noticed. + + Both of the observations that motivated this were real: that two other + Support-bundle controls explained nothing, and that a doc comment is all + that stops `Snippet::text` reaching the bundle. Each cost an approval and + was recorded nowhere. + """ + from agent_harness.executor import APPROVED, record_follow_ups + + verdict = ( + "APPROVED\n\n" + "3. **Why** — it does what was asked.\n\n" + "4. **Follow-ups**\n" + "- the other two Support bundle controls explain nothing\n" + "- nothing structurally stops Snippet::text reaching the bundle\n" + ) + + found = record_follow_ups(tmp_path, "rdpapp", "R4", APPROVED, verdict, 1000.0) + + assert len(found) == 2 + kept = (tmp_path / "FOLLOW-UPS.md").read_text() + assert "the other two Support bundle controls" in kept + assert "R4" in kept and "rdpapp" in kept + assert "Nothing is" in kept and "queued" in kept, "and it must say nothing waits on them" + + +def test_a_rejection_never_produces_follow_ups(tmp_path: Any) -> None: + """The safety property, and the reason this cannot become a way to wave + work through. A rejection has already said what is wrong; a second channel + there would let "approve and defer" grow over a failed criterion.""" + from agent_harness.executor import REJECTED, record_follow_ups + + verdict = ( + "REJECTED\n\n3. **Why** — it fails criterion 2.\n\n" + "4. **Follow-ups**\n- something else entirely\n" + ) + + found = record_follow_ups(tmp_path, "rdpapp", "R4", REJECTED, verdict, 1000.0) + + assert found == () + assert not (tmp_path / "FOLLOW-UPS.md").exists() + + +def test_no_follow_ups_writes_nothing(tmp_path: Any) -> None: + """A reviewer answering "none" has answered the question. Turning that + into a file would fill a backlog with the absence of findings.""" + from agent_harness.executor import APPROVED, record_follow_ups + + assert record_follow_ups(tmp_path, "p", "T1", APPROVED, "APPROVED\n\n- none", 1.0) == () + assert not (tmp_path / "FOLLOW-UPS.md").exists() + + +def test_prose_after_the_list_is_not_swallowed_as_an_item() -> None: + """Attributing a proposal the reviewer did not make is worse than missing + one: a person triages it, finds nothing behind it, and trusts the next + one less.""" + from agent_harness.executor import parse_follow_ups + + verdict = ( + "APPROVED\n\n4. **Follow-ups**\n" + "- a real one\n\n" + "I should add that the overall design seems sound.\n" + ) + + assert parse_follow_ups(verdict) == ("a real one",) + + +def test_the_rubric_tells_the_reviewer_a_follow_up_is_not_a_rejection() -> None: + """The behaviour lives in the prompt; without this the parser has nothing + to parse.""" + from agent_harness.executor import REVIEW_PROMPT + + assert "A follow-up is not a rejection" in REVIEW_PROMPT + assert "proposed work items for a person to accept or discard" in REVIEW_PROMPT + # And the boundary must survive alongside it. + assert "no follow-up substitutes for one" in REVIEW_PROMPT