Skip to content

fix: the session reviewer was shown an empty diff, always - #162

Closed
thedancingdeveloper wants to merge 1 commit into
mainfrom
fix/session-review-sees-an-empty-diff
Closed

fix: the session reviewer was shown an empty diff, always#162
thedancingdeveloper wants to merge 1 commit into
mainfrom
fix/session-review-sees-an-empty-diff

Conversation

@thedancingdeveloper

Copy link
Copy Markdown
Contributor

Session mode has never approved anything, and could not have.

Found by running session mode end to end against a real repository (#56) — the
first time it has been exercised at all.

The bug

The checkpoint before the expensive gate commits the agent's work. That is
deliberate and load-bearing: a worker killed during review must not lose what
already passed the cheap gates.

_review then computed the diff as git diff HEAD — "what is uncommitted?".
By the time a reviewer is called, the answer is always "nothing".

So every session-mode reviewer received an empty diff, and said the only
correct thing there is to say about one:

The supplied diff is empty, so it demonstrates none of that and cannot be
judged as satisfying the request.

Measured on a real 48-line change to a 634 KB file that had already passed its
checks. I ran its four tests by hand afterwards — all passing. The work was
right; the reviewer was shown nothing.

The fix

Diff against the item's base, which is what the branch actually changed.
git diff HEAD is kept for the no-base case rather than assumed away.

Why it survived

Every unit test passed with the bug present. Nothing asserted what the
reviewer is shown after the checkpoint has run, so the suite was green while
the mode USAGE.md calls "the mode worth using" could not complete a single
item. The regression test here fails without the fix — verified by reverting
it, not by assuming.

Related findings from the same run, not fixed here

Each is real, each cost an attempt, and none is documented:

  • A worktree sits one level deeper than the repository, so a relative path
    dependency (../apps/FluentGUI) stops resolving. The worktree location is
    hardcoded in core, which sits badly against AGENTS.md's first rule about
    not baking in directory conventions — it wants a --worktrees flag.
  • A fresh worktree has none of the gitignored build inputs. node_modules
    is not in git, so a tsc check could not start and the item failed for a
    reason unrelated to its work.
  • The agent has no write permission unless the deployment grants it. One
    attempt produced no changes because Edit was refused — and the agent
    correctly declined to shell around it. That reads exactly like a model that
    decided to do nothing.

The finding that motivated all of this

Session mode edited a 634 KB file correctly on its first attempt, where
ten headless attempts had failed. #158 is a limitation of the unified-diff
protocol, not of the harness.

All four gates green.

🤖 Generated with Claude Code

Session mode has never approved anything, and could not have.

The checkpoint before the expensive gate commits the agent's work — that is
deliberate and load-bearing, so a worker killed during review does not lose
what already passed the cheap gates. `_review` then computed the diff as
`git diff HEAD`, which asks "what is uncommitted?". By the time a reviewer is
called the answer is always "nothing".

So every session-mode reviewer received an empty diff and said the only
correct thing there is to say about one:

  "The supplied diff is empty, so it demonstrates none of that and cannot be
   judged as satisfying the request."

Measured on a real 48-line change to a 634 KB file that had already passed its
checks, whose four tests I ran by hand afterwards — all passing. The work was
right; the reviewer was shown nothing.

The diff is now taken against the item's base, which is what the branch
actually changed. `git diff HEAD` is kept for the no-base case rather than
assumed away.

Two things worth saying about how this survived until now.

**Every unit test passed with the bug present.** Nothing asserted what the
reviewer is shown *after* the checkpoint has run, so the whole suite was green
while the mode the documentation calls "the mode worth using" could not
complete a single item. The regression test added here fails without the fix —
verified by reverting it, not by assuming.

**It took running the thing end to end against a real repository to find.**
The same exercise found the other session-mode gaps: a worktree sits one level
deeper than the repository so relative path dependencies stop resolving, a
fresh worktree has none of the gitignored build inputs a check may need, and
the agent has no write permission unless the deployment grants it — which
looks exactly like a model that decided to do nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thedancingdeveloper pushed a commit that referenced this pull request Aug 5, 2026
I branched this from main, which does not carry #162, and re-applied the
empty-diff fix here while rewriting _review. That left the same change in two
open pull requests — my error in branching, not a second bug.

Folding them: this branch already had the fix and was missing the test that
proves it stays fixed, which is the half worth keeping. #162 closes as
superseded rather than both being merged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@thedancingdeveloper

Copy link
Copy Markdown
Contributor Author

Folded into #168.

I branched fix/one-review-rubric from main, which does not carry this fix, and re-applied it there while rewriting _review for #167. That left the same change in two open pull requests — my error in branching, not a second bug.

#168 now carries both the fix and this PR's regression test (test_the_reviewer_sees_the_work_after_the_checkpoint_committed_it), which was the half it was missing. Nothing is lost by closing this; merging both would have applied the same change twice.

The finding itself stands and is unchanged: the session reviewer was shown an empty diff on every item, so no session-mode item could ever have been approved.

thedancingdeveloper pushed a commit that referenced this pull request Aug 5, 2026
I branched this from main, which does not carry #162, and re-applied the
empty-diff fix here while rewriting _review. That left the same change in two
open pull requests — my error in branching, not a second bug.

Folding them: this branch already had the fix and was missing the test that
proves it stays fixed, which is the half worth keeping. #162 closes as
superseded rather than both being merged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

1 participant