From c0ed86661e1c6cc0c477ea17528e56633fcd6ffa Mon Sep 17 00:00:00 2001 From: sprooty Date: Wed, 5 Aug 2026 01:40:08 +0000 Subject: [PATCH] fix: keep the reviewer's objection, not its praise MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `last_error` is what a retry is told and what an operator reads. It kept the first 500 characters of the verdict — and the rubric asks for "what I verified" first and "why" last, so those 500 characters were the list of things the reviewer was *happy* with. The objection, the only part anyone can act on, was cut off. Measured. Two items were rejected twice for the same fault, and both retries had been told, faithfully, only the preamble praising the parts that were fine: R3: 517 chars, ending "...er still gets the original API shape.\n- The support bundle p" R4: 517 chars, ending "...visual pattern.\n\n2. **What I could not verify** — The diff d" Neither reached section 3. This is why #166 looked ineffective: the feedback channel worked and carried the wrong half. So the tail wins when something has to go, cut at a section boundary so a retry is not handed half a sentence about something it cannot see the start of. The head is a summary of a diff the reader already has; the tail is the decision. Both executors truncated identically and both now share one helper, so this cannot drift the way the review rubric did (#167). Co-Authored-By: Claude Opus 5 (1M context) --- src/agent_harness/executor.py | 30 ++++++++++++++++++++++- src/agent_harness/session_executor.py | 3 ++- tests/test_reviewer.py | 34 +++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 2 deletions(-) diff --git a/src/agent_harness/executor.py b/src/agent_harness/executor.py index c9f503a..14c33f4 100644 --- a/src/agent_harness/executor.py +++ b/src/agent_harness/executor.py @@ -1350,6 +1350,34 @@ def is_disk_exhaustion(detail: str) -> bool: """ +def review_reason(verdict_text: str, limit: int = 1200) -> str: + """The reviewer's verdict, kept from the end rather than the beginning. + + `last_error` is what a retry is told and what an operator reads, and it + used to be the first 500 characters of the verdict. The rubric asks for + "what I verified" first and "why" last, so those 500 characters were the + list of things the reviewer was *happy* with, and the objection — the only + part anyone can act on — was cut off. + + Measured: two items were rejected twice for the same fault after being + told, both times, only the preamble praising the parts that were fine. + + So the tail wins when something has to go. The head is a summary of the + diff, which the reader already has; the tail is the decision. + """ + text = verdict_text.strip() + if len(text) <= limit: + return text + # Prefer starting at a section boundary, so a retry is not handed half a + # sentence about something it cannot see the start of. + tail = text[-limit:] + for marker in ("\n3. ", "\n2. ", "\n\n"): + cut = tail.find(marker) + if 0 <= cut < limit // 2: + return "…" + tail[cut:].strip() + return "…" + tail.strip() + + def review_context(repo: Path, diff: str, budget: int) -> str: """The touched files as they now stand, for the reviewer. @@ -2413,7 +2441,7 @@ def _review_stage( if outcome.pr_url and self.github is not None: self._record_verdict(record, outcome.pr_url, verdict, verdict_text) if verdict != APPROVED: - outcome.reason = f"review rejected: {verdict_text.strip()[:500]}" + outcome.reason = f"review rejected: {review_reason(verdict_text)}" outcome.stop = Stop(REFUSED, REVIEW_REJECTED, detail=outcome.reason) return outcome diff --git a/src/agent_harness/session_executor.py b/src/agent_harness/session_executor.py index f43f85e..01ce3d7 100644 --- a/src/agent_harness/session_executor.py +++ b/src/agent_harness/session_executor.py @@ -139,6 +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 @dataclass @@ -647,7 +648,7 @@ def _execute(self, record: WorkRecord) -> Outcome: self._record_verdict(record, outcome.pr_url, verdict, verdict_text) if verdict != APPROVED: - outcome.reason = f"review rejected: {verdict_text.strip()[:500]}" + outcome.reason = f"review rejected: {review_reason(verdict_text)}" outcome.stop = Stop(REFUSED, REVIEW_REJECTED, detail=outcome.reason) return outcome diff --git a/tests/test_reviewer.py b/tests/test_reviewer.py index 8e044b3..e63c4c8 100644 --- a/tests/test_reviewer.py +++ b/tests/test_reviewer.py @@ -240,3 +240,37 @@ def call(self, _role: str, messages: Any, **_: Any) -> Any: assert "hello harness" in seen["prompt"], "the reviewer was not shown the committed work" assert "-hello world" in seen["prompt"], "nor what it replaced" + + +def test_the_kept_reason_is_the_objection_not_the_praise() -> None: + """`last_error` is what a retry is told, and it kept the wrong end. + + The rubric asks for "what I verified" first and "why" last. Keeping the + first 500 characters therefore kept the list of things the reviewer was + happy with and cut off the objection — the only part anyone can act on. + + Measured: two items were rejected twice for the same fault, each retry + having been told only the preamble praising the parts that were fine. + """ + from agent_harness.executor import review_reason + + verdict = ( + "REJECTED\n\n" + "1. **What I verified** — " + "the call site is right, " * 60 + "\n\n" + "2. **What I could not verify** — " + "nothing much, " * 20 + "\n\n" + "3. **Why**\nIt widens the repository trait's return type." + ) + + kept = review_reason(verdict) + + assert "widens the repository trait" in kept, "the objection must survive" + assert len(kept) <= 1300 + # And the old behaviour must not come back: the head is a summary of a + # diff the reader already has. + assert not kept.startswith("REJECTED\n\n1. **What I verified**") + + +def test_a_short_verdict_is_kept_whole() -> None: + from agent_harness.executor import review_reason + + assert review_reason("REJECTED\nToo narrow.") == "REJECTED\nToo narrow."