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