From acad846c4de9ff58f6162ead5f467d10055805ae Mon Sep 17 00:00:00 2001 From: sprooty Date: Mon, 3 Aug 2026 22:22:07 +0000 Subject: [PATCH] fix: review the change that was made, not the one that was proposed `Executor` showed the reviewer the diff text the model produced. The tolerance ladder exists to rescue malformed patches, so that text and the change it produces are routinely different: a `@@ -0,0` hunk git apply would refuse becomes, under --unidiff-zero, a real insertion with real context. Reviewing the text had both failure directions. Good work was rejected for an artefact of the plumbing -- the reviewer said, correctly about what it was shown and falsely about what happened, that it "could not verify that existing functions were kept untouched, because both files are shown as being created from empty". And a diff that claimed more than it did would have been reviewed as truth, which is the one thing the review prompt exists to catch. `SessionExecutor` already reviewed `git diff HEAD`. This makes the two agree. Live effect on the same three-item backlog: an item completed end to end for the first time, and the two that still fail now fail on their merits -- the reviewer can see that a rescued hunk landed above the file's existing imports, which is a real defect (#133) that the proposed diff could not express. --- src/agent_harness/executor.py | 10 +++- tests/test_executor.py | 86 +++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 1 deletion(-) diff --git a/src/agent_harness/executor.py b/src/agent_harness/executor.py index c766bcc..4a9c661 100644 --- a/src/agent_harness/executor.py +++ b/src/agent_harness/executor.py @@ -664,6 +664,14 @@ def _execute(self, record: WorkRecord) -> Outcome: self._abandon_branch(branch) return outcome self._emit(record, "applied", detail=how) + # What landed, not what was proposed. The tolerance ladder exists to + # rescue malformed patches, so the text a model produced and the change + # it produced are routinely different -- a `@@ -0,0` hunk that git + # apply would refuse becomes a real insertion with real context. The + # reviewer has to see the second one: reviewing the first rejects good + # work for an artefact of the plumbing, and, worse, makes the gate + # structurally unable to catch a diff that claims more than it did. + applied_diff = run_git(self.repo, "diff", "HEAD") or diff self._keepalive(record) # 5. Cheap checks BEFORE the expensive reviewer call. Paying a model @@ -692,7 +700,7 @@ def _execute(self, record: WorkRecord) -> Outcome: REVIEWER, REVIEW_PROMPT.format( brief=record.brief, - diff=diff[:20000], + diff=applied_diff[:20000], checks="passed" if passed else failure, ), ) diff --git a/tests/test_executor.py b/tests/test_executor.py index 6ae77b3..e348a45 100644 --- a/tests/test_executor.py +++ b/tests/test_executor.py @@ -862,3 +862,89 @@ def test_with_nowhere_to_keep_it_the_item_still_fails_cleanly( assert outcome is not None and outcome.state == FAILED assert "patch kept at" not in outcome.reason assert any(e["outcome"] == "patch_malformed" for e in events) + + +# ------------------------------------------------- what the reviewer sees + + +#: A hunk header `git apply` refuses against an existing file, which the +#: tolerance ladder rescues by inserting at line 1. The text says "this file +#: was empty"; the result is an insertion above the existing content. +ZERO_CONTEXT_DIFF = """\ +diff --git a/hello.txt b/hello.txt +--- a/hello.txt ++++ b/hello.txt +@@ -0,0 +1,1 @@ ++a new first line +""" + + +class PromptCapturingModel(ScriptedModel): + """Keeps the prompt each role was given, so a test can assert on it.""" + + def __init__(self, replies: Mapping[str, str]) -> None: + super().__init__(replies) + self.prompts: dict[str, str] = {} + + def __call__( + self, route: Route, messages: Sequence[Mapping[str, Any]], options: Mapping[str, Any] + ) -> Response: + role = str(route.options.get("role", route.model)) + self.prompts[role] = str(messages[-1].get("content", "")) + return super().__call__(route, messages, options) + + +def test_the_reviewer_sees_the_applied_diff_not_the_proposed_one( + repo: Path, tmp_path: Path +) -> None: + """The regression. + + The tolerance ladder exists to rescue malformed patches, so a model's diff + text and the change it produces are routinely different. Reviewing the + text rejected good work for an artefact of the plumbing — and made the + gate unable to catch a diff that claims more than it did, which is the one + thing the review prompt says it is for. + """ + executor, queue, transport = build( + repo, + tmp_path, + {"planner": "plan", "implementer": ZERO_CONTEXT_DIFF, "reviewer": "APPROVED\nfine"}, + ) + capturing = PromptCapturingModel( + {"planner": "plan", "implementer": ZERO_CONTEXT_DIFF, "reviewer": "APPROVED\nfine"} + ) + executor.client.transport = capturing + add_item(queue) + + outcome = executor.run_once() + + assert outcome is not None + assert outcome.state == DONE, outcome.reason + shown = capturing.prompts["reviewer"] + # The real change: an insertion, with the existing line as context. + assert "a new first line" in shown + assert "hello world" in shown, "the reviewer must see the surrounding context that survived" + # Not the model's claim that the file was created from empty. + assert "@@ -0,0" not in shown, "the reviewer must not be shown the proposed hunk header" + + +def test_a_reviewer_can_tell_where_a_rescued_hunk_landed(repo: Path, tmp_path: Path) -> None: + """Placement is the thing the model's text cannot carry (#133). The + applied diff shows it, which is what makes rejecting misplacement possible + at all.""" + executor, queue, _ = build( + repo, + tmp_path, + {"planner": "plan", "implementer": ZERO_CONTEXT_DIFF, "reviewer": "APPROVED\nfine"}, + ) + capturing = PromptCapturingModel( + {"planner": "plan", "implementer": ZERO_CONTEXT_DIFF, "reviewer": "APPROVED\nfine"} + ) + executor.client.transport = capturing + add_item(queue) + + executor.run_once() + + shown = capturing.prompts["reviewer"] + # The new line appears BEFORE the pre-existing one, and the diff says so. + assert shown.index("+a new first line") < shown.index("hello world")