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."