Skip to content

fix: review the change that was made, not the one that was proposed - #134

Merged
thedancingdeveloper merged 1 commit into
mainfrom
fix/review-what-was-applied
Aug 3, 2026
Merged

fix: review the change that was made, not the one that was proposed#134
thedancingdeveloper merged 1 commit into
mainfrom
fix/review-what-was-applied

Conversation

@thedancingdeveloper

Copy link
Copy Markdown
Contributor

Closes #132.

Executor passed the model's own diff text to the reviewer. SessionExecutor
already passed git diff HEAD. The plain one was the wrong way round.

This matters because the tolerance ladder's whole purpose is to rescue patches
whose text is malformed — so the text and the resulting change are routinely
different.

Evidence, from a live run

Model's text (what the reviewer used to see):

@@ -0,0 +1,4 @@
+def multiply(a, b):
+    return a * b

What actually landed:

@@ -1,3 +1,7 @@
+def multiply(a, b):
+    return a * b
+
+
 """A deliberately tiny module, so an agent's diff is easy to judge."""

The reviewer rejected the item because it "could not verify that existing
functions and tests were kept untouched, because both files are shown as being
created from empty". Correct about the text; false about the change. The
checks had already passed against the applied tree, including the pre-existing
tests — the harness had proof the file was intact and showed the reviewer
something else.

Live effect

Same three-item backlog, before and after:

Rejecting good work was the visible half. The invisible half is worse: a gate
told to catch a change that "claims more than it did" could not, because it
was reading the claim.

Tests

Two, both failing without the change: the reviewer's prompt must contain the
surviving context and must not contain the proposed @@ -0,0 header; and the
prompt must show where a rescued hunk landed relative to existing content.

548+ tests, ruff, mypy . green.

`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.
@thedancingdeveloper
thedancingdeveloper merged commit b1bb7f4 into main Aug 3, 2026
2 checks passed
@thedancingdeveloper
thedancingdeveloper deleted the fix/review-what-was-applied branch August 3, 2026 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: the reviewer is shown the model's proposed diff, not the one that was applied

1 participant