feat: a reviewer's surplus insight becomes the next item, not a refusal (#171)

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,

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}",

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

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