Review the change set since the stage began, not since HEAD by adamw · Pull Request #59 · VirtusLab/orca

adamw · 2026-08-02T15:23:22Z

The defect

ReviewFixLoop.sampleDiff() asked git.reviewDiff() — git diff HEAD plus
untracked file contents. Nothing in orca forbids a coding agent from committing
its own work, and agents do it. Once they have, that diff is empty:

reviewer selection ran on changedFiles = Nil, so the file-pattern
eligibility filter had nothing to match and the picker judged the task from
its title alone;
every reviewer's first prompt carried the literal
(no diff captured — review the working tree), and each one then rediscovered
the change set by hand with git log / git show — every round, in parallel.
A cost sink on top of a correctness problem.

The fix

The change set under review is everything the enclosing stage has produced,
committed or not.

A stage now records the commit HEAD pointed at when it began —
enterStage(name, baseCommit), read back as FlowControl.stageBaseCommit. It
has to be captured there: by the time anything asks, the body's agent may
already have moved HEAD. reviewAndFixLoop samples
git.reviewDiff(stageBaseCommit) — tracked changes since that commit plus
untracked file contents, .orca/ excluded — fresh each round, so a reviewer
joining a later round also sees the fixer's edits.

The stage, not the branch, bounds the change set. A branch-wide base
(git.defaultBase()) answers a different question and would pull every earlier
task of a multi-task plan into the review.

Unaffected by skip-branch mode and by detached HEAD, since the baseline is a
commit, not a ref.

Selection across rounds

Re-selection is deliberately not re-run per round: prepare's picker stays a
single LLM call per loop (the cost that design exists to bound), and per-round
narrowing keeps working off history — which reviewers reported last round,
a better signal than which files the fixer happened to touch. What changes is
that the once-computed changedFiles is now the task's real file set instead of
empty.

API

GitTool.headCommit(): Option[String] — HEAD's hash, best-effort.
GitTool.reviewDiff(since: Option[String] = None) — None still means HEAD.
FlowControl.enterStage(name, baseCommit) and FlowControl.stageBaseCommit.
reviewAndFixLoop's initialDiff survives as the "pin the diff" override.
Its second documented rationale — "when the change set is already committed
and git.reviewDiff() would be empty" — is what this removes the need for,
so it is now documented by its real remaining use: tests skipping the git
call.

Tests

Every behavioural edge was mutation-checked: mutation applied, exactly the
intended test failed, reverted.

Test	Mutation that breaks it
`reviewDiff since a base commit reports work committed after it`	`reviewDiff` ignores `since`
`headCommit is empty in a repository with no commits`	drop the exit-code check
`a stage's base commit is the one it started from, not a live HEAD`	`enterStage(name, None)`
`a reviewer sees work the coding agent committed inside the stage`	`sampleDiff` back to `reviewDiff()`
`a reviewer joining a later round sees an edit the fixer committed`	sample the diff once per loop instead of per round
`reviewer selection sees the files of work the agent committed`	`changedFiles = Nil` (the buggy state itself)

That last one came out of review: changedFiles = Nil originally passed all
349 flow tests, so the defect's other half — the empty list the selector saw
— had no regression guard at all.

sbt scalafmtCheckAll clean, sbt clean compile test green across all modules,
zero warnings.

Review notes

Three reviewers (correctness, simplicity, test minimality) ran over the first
commit; the second commit is their surviving findings. Two things I checked and
declined:

stageBaseCommit doesn't call assertOwnerThread. StageFrames'
convention is that mutators assert (enterStage, exitStage,
nextSessionOccurrence); inStage is the existing precedent for a pure
reader that doesn't. stageBaseCommit follows it.
A no-base fallback test. The branch is unreachable: reviewAndFixLoop
needs InStage + WorkspaceWrite, obtainable only inside a stage, and in
the one state that does produce no baseline (a repo with no commits)
git diff HEAD exits 128 and throws rather than falling back. The test only
reached it by forging tokens, and it was the one test that still passed with
the feature removed. Dropped, along with the doc claims asserting a fallback
that doesn't exist.

Found in review, deliberately not fixed here

Pre-existing on master (from #46), verified but out of scope — each worth its
own change:

An untracked symlink to a directory aborts the review loop.
git status -uall lists it, then git diff --no-index exits 1 with stderr,
so untrackedFileDiff's differs predicate is false and it throws. Any repo
with a non-gitignored docs/latest -> docs/v2 fails the stage.
When workDir is a repo subdirectory, reviewDiff mixes path relativity:
the tracked half is root-relative (:(top)), the untracked half
workDir-relative, and both reach the selector as one list.
pendingChanges' scaladoc says "sampled in ONE pass"; it's four-plus git
invocations. The claim it actually needs — that the untracked list and
renderings come from one sample — is true.

A coding agent is free to commit its own work, and does. Once it has, `git diff HEAD` is empty, so reviewer selection ran on an empty changed-file list and every reviewer was prompted with "(no diff captured — review the working tree)", then rediscovered the change set by hand with git log/show, every round. A stage now records the commit HEAD pointed at when it began (`enterStage(name, baseCommit)`, read back as `FlowControl.stageBaseCommit`), and `reviewAndFixLoop` samples `git.reviewDiff(stageBaseCommit)` — tracked changes since that commit plus untracked file contents, `.orca/` excluded. The stage, not the branch, bounds the change set, so a multi-task plan's earlier tasks stay out of it. With no baseline the loop falls back to the previous working-tree-only view.

Review follow-ups: - Nothing pinned the defect's other half. Setting `changedFiles = Nil` — the buggy state itself — passed all 349 flow tests. A recording selector now asserts the loop derives the changed-file list from the stage-based diff. - The documented "falls back to the working-tree view" when no base was recorded is false: with no commits `git diff HEAD` exits 128 and throws, and the other route needs forged capability tokens. Dropped the claim from four doc sites and the test that pinned it; ADR 0018 records the inherited limitation instead. - `stageBaseCommit` narrowed to `private[orca]`; `rev-parse --verify` shared between `headCommit` and `refExists`; `initialDiff` documented by its real use; rationale cut to one home per fact. - `captureReviewer` folded onto `FakeAgent.seenPrompts`, which supersedes it.

# Conflicts: # README.md # adr/0011-reviewer-roster.md

An untracked symlink pointing at a directory aborted the whole review loop. `git status --porcelain -uall` reports such a symlink as a single `?? linkdir` entry — git does not recurse into it. `git diff --no-index -- /dev/null linkdir` then follows the link, finds a directory on one side and a file on the other, and tries to pair `/dev/null` with `linkdir/null`: ``` error: Could not access 'linkdir/null' ``` exit 1, message on stderr. `OsGitTool.untrackedFileDiff` classifies that correctly — its `exit 1 && empty stderr` predicate means "the sides differ", and this is exit 1 *with* stderr — so it calls `fail`, which throws `OrcaFlowException`. One such symlink anywhere in the working tree took down `reviewDiff()` and with it the review loop. ## The fix The predicate is untouched. It was tightened deliberately in #46, and it is correct: before that, exit 1 was unconditionally read as "the sides differ", which swallowed a real failure — from a subdirectory, `git diff --no-index -- /dev/null sub/newfile.txt` also exits 1 with an access error, and every new file's contents were vanishing while `reviewDiff()` reported success. Loosening it would restore that bug. The symlink case is the same signal from a different cause, so the fix is to stop asking git to render the path at all. `untrackedFileDiff` now short-circuits a symlink that resolves to a directory and emits a line naming it instead. It is the single shared path — both `reviewDiff()` and `pendingChanges()` reach it through `withNewFileContents` — so both are fixed once. ## What was measured, not assumed Against git 2.53.0, for each kind of untracked symlink: | symlink to | exit | stderr | stdout | |---|---|---|---| | a directory | 1 | `error: Could not access 'linkdir/null'` | empty | | a file | 1 | empty | `new file mode 120000`, link target as content | | nothing (dangling) | 1 | empty | `new file mode 120000`, link target as content | Only the directory case is broken. git never follows a symlink to a file — it renders the link itself as a mode-120000 blob holding the target path. **Only directory symlinks are held back**, not all symlinks: a symlink to a file or to nothing already renders correctly and is a real change the reviewer should see, so dropping those would trade one silent omission for another. **Skipped symlinks are announced**, not silently omitted, as a `# skipped <path>: symlink to a directory` line. A reviewer who believes it saw everything is worse than one told something was skipped — the same reason `CommitDiff` marks a truncated diff. It also keeps the reviewer able to flag a newly created symlink at all, which matters given this repo already treats symlinks in the working tree as a surface worth refusing (`OrcaDir`, `ProgressScan`). `untrackedPaths()` is deliberately left alone, so `pendingChanges().newFiles` still lists the symlink — verified that `git add -A` does commit it, as a mode-120000 blob, which is exactly what that list promises. ## Tests Two in `OsGitToolTest`, each mutation-checked to fail alone. **The abort.** A repo with an untracked symlink-to-directory alongside an ordinary new file. Replacing the guard condition with `false` fails exactly this test, with `orca.OrcaFlowException: git diff --no-index -- /dev/null linkdir failed (exit 1): error: Could not access 'linkdir/null'` — the reported error verbatim. **How narrow the filter is** — deliberately not a test of the fix, and not redundant with the one above. It pins that a symlink to a *file* still renders as its own mode-120000 diff. Widening the guard to `os.isLink(path)` alone fails exactly this test and nothing else; without it, that widening would silently swallow every file-symlink's diff — the same "reviewer told nothing was there" failure this PR exists to prevent. Please don't delete it as a duplicate of the directory test; it guards the decision, not the bug. ## Sequencing vs #59 Branched off `master`, not off #59. #59 (`review-change-set-since-stage-base`) adds a `since` parameter and touches `reviewDiff`, `withNewFileContents` and the trait scaladoc in the same file, but does not touch `untrackedFileDiff` or `untrackedPaths` — the two changes are independent. Whichever merges second picks up a small conflict in the `reviewDiff` scaladoc block and in `withNewFileContents`'s signature; both resolve by keeping both edits. No ordering is required either way. ## Known adjacent defect, not fixed here The identical abort fires for an untracked **nested git repository**: `git status -uall` does not recurse into one either, reporting `?? nested/` (with a trailing slash), and `git diff --no-index -- /dev/null nested/` fails the same way with `error: Could not access 'nested/null'`. Verified on git 2.53.0. It is a different cause with a different fix and a different message, so it is left for its own change rather than widened into this one.

# Conflicts: # tools/src/main/scala/orca/tools/GitTool.scala

T2.5: should the re-review prompt carry more than the eight lines it sends today — the fixer's `FixOutcome`, a diff, or something else? Findings only — one new file under `docs/research/run-cost/`, no code change. Source read at `59597ca5`. ## The answer - **Do not send `fixed` titles — as the default, pending the `+fixed` experiment arm.** It answers the question the round exists to ask, it endangers the confidence contract (`initial-review.md:17-22`, `ReviewIssue.scala:23-32`) and the "reviewer re-checks its own fix" property, and it saves nothing: the reviewer must open the code either way. The rubber-stamping risk is argued, not measured; §8 specifies the experiment that would measure it. - **Do send `ignored` titles with reasons.** ~50–150 tokens, and the one thing in the loop not recoverable from the tree at any price. Today a declined finding is re-reported and re-declined every round. - **Prioritise a diff over both**: the change since the reviewer's previous round. It is evidence rather than a claim, so it leaves the confidence contract alone, and it closes a live correctness hole. #72 ships the coarser form of this (the whole change set, classified against what each reviewer was last sent); the per-reviewer increment remains open. ## Evidence **A resumed reviewer receives no diff at all, including after #59.** `runReviewersAndLint` samples the diff only when some active reviewer has no session yet (`ReviewLoop.scala:477-478`), and under every shipped selector the active set at round N is a subset of round 1's — `agentDriven` returns a constant arrow, `narrowingAcrossRounds` only filters it, and its floor falls back to that same pick. So `needsDiff` is false from round 2 onward and #59's re-sampled diff never reaches anybody. The premise T2.5 was written on — reviewers rediscovering the change set by hand every round — is still exactly true from round 2 on. That is also why the correctness hole above is live: a resumed reviewer falls back to its own `git diff HEAD`, which is empty the moment the fixer commits. **The 9.3-vs-4.2 tool-call evidence reproduces and survives, but was mis-grouped.** Measured over the baseline run's ten reviewer transcripts (40 rounds). Tool calls are unique `tool_use` **block** ids, `StructuredOutput` included; the token columns are deduplicated by assistant **message** id. The two axes cannot share a key — message-id dedup on tool calls yields 52 rather than 303, because streaming partials repeat a message id with different content. | | tool calls (mean/median) | cache write | cache read | est. $/reviewer | |---|---|---|---|---| | round 1 | 11.3 / 9.5 | 37.3k | 399k | 0.75 | | round 2 | 10.3 / 10.5 | 63.2k | 576k | 1.14 | | rounds >=3 | 4.35 / 4.0 | 9.1k | 397k | 0.36 | Round 2 is the loop's most expensive round, which "round 1 vs follow-up" hides. Rounds >=3 spend 68% of their shell calls — 38 of that round's 56 `Bash` calls — on `git status`/`git diff` reconstructing the change set. The figures survive #59 because #59 cannot reach a resumed reviewer, and survive #48 per reviewer, since narrowing changes how many reviewers run, not what each does. **This table does not contradict #73's.** #73 reports the same three rounds as 10.3 / 9.3 / 3.35 — exactly 1.00 lower in each, because it excludes the mandatory `StructuredOutput` call that this table includes. 303 blocks here against 263 there, one `StructuredOutput` per round. The document records the reconciliation. ## Caveats a reader should carry - **No measurement of the current code exists.** Every manifest on disk is `manifestVersion: 2`, so #61's schema v3 has not been exercised by a run. The document says so rather than estimating. - **Open PR #72 fixes the no-diff defect.** The document carries staleness markers that are correct whichever of the two lands first. - `initial-review.md:7` and `select-reviewers.md:8` still describe the `git diff HEAD` sampling that #59 replaced. Both are corrected by #72.

## The defect `runReviewersAndLint` sampled the review diff only when some active reviewer lacked a session: ```scala val needsDiff = active.exists(e => storedFor(e).isEmpty) val currentDiff = if needsDiff then sampleDiff() else "" ``` Under every shipped selector the round-N active set is a subset of round 1's: `agentDriven` returns a constant pick, `narrowingAcrossRounds` only filters that pick and falls back to it, `allEveryRound` returns all. So from round 2 onward every active reviewer already held a session, `needsDiff` was false, and #59's re-sampled diff reached nobody. A resumed reviewer got no change set and fell back to running its own `git diff HEAD`, which is empty as soon as the fixer commits. An empty diff reads as "nothing changed", so the reviewer could report clean without ever seeing the fix. #59 removed this from round 1; rounds 2+ still had it. ## The fix Sample the stage diff whenever any reviewer runs, and pass it to `resumeReview` as well as `firstReview`. `re-review.md` had no diff placeholder at all, so it gains one. Sampling now runs once per round. `reviewDiff` is `untrackedPaths()` plus a tracked `git diff` plus one `git diff --no-index` per untracked file — a handful of git calls, not one. It runs on the collecting thread before the thunks are built, so it stays outside the capture-checked fan-out; `Configured` resolution at loop entry is untouched. ## Bounding what a resume re-sends Two problems the plain fix would have introduced: - A pinned `initialDiff` returns the same constant every round, so a resumed reviewer would get a byte-identical pre-fix diff framed as freshly re-sampled, and would re-report the finding just fixed, round after round to the cap. Pinning is documented public API, so this is a live path. - A resume replays the transcript, so re-sending the diff every round gives one reviewer round-count copies of it — worst on the largest changes. `lint` already caps its payload; the reviewer diff did not. Both are handled by recording, per reviewer, the change set it was last **sent** (`SessionEntry.lastDiff`) and classifying each round against it (`ReReviewChanges.of`): unchanged sends nothing, a change past the inline threshold sends the changed paths for the reviewer to open, otherwise the diff. The session fold now replaces a reviewer's entry instead of prepending. The initial diff stays uncapped, as on master — a general diff-size policy is separate work. This bounds only the per-round repetition. ## Why the full stage diff, not a per-reviewer delta The alternative — the change since that reviewer's own previous round — is smaller, and #65's research names it as the larger cost lever. Deliberately not in this PR: - It needs a per-round tree snapshot (a new `GitTool` primitive writing objects under `WorkspaceWrite`) threaded into the round loop, which is the capability `ReviewFixLoop` is structured to keep out of that region, plus a new field on `ReviewLoopState`. #65 sequences that work after a diff-size policy exists, and notes an uncapped per-round delta can cost more than the two tool calls it removes. - #65 §7 names attention narrowing as the delta's risk: a reviewer handed only what changed may stop checking whether its earlier finding is still open outside the delta. The full change set doesn't have that problem, which is what this correctness fix needs. The delta stays layerable later — it changes what `sampleDiff` returns per reviewer, not the plumbing added here. ## Depends on #75, which must land first The over-threshold path names the changed files instead of sending hunks, and takes those names from `GitTool.changedFiles` (added by #75), not from `extractChangedFiles`. The diff-text parser can't see binary changes or 100%-similarity renames and mis-splits paths containing spaces, so it would hand a reviewer a silently incomplete list on exactly the large change sets the threshold exists for — worse than a truncated diff, because nothing signals the omission. #75 keeps that parser for the pinned-`initialDiff` case, where a git query would describe the current tree rather than the pinned change set. That case cannot reach this branch: `ReReviewChanges.of` tests equality before size, and a pinned sample is byte-identical every round, so a resume always classifies `AlreadySeen`. The pinned test uses an over-threshold diff, and the path list never appears. #75 is merged into this branch so it is verifiable standalone. ## Stale lines corrected - `initial-review.md` described the sampling #59 replaced. - `select-reviewers.md` told the picker to prefer `git diff HEAD`. - The `initialDiff` scaladoc and the README paragraph framed the override and the re-sampling as initial-prompt-only. - The loop scaladoc's "a reviewer joining the active set on iteration N sees the earlier fixes too" described a path only a custom selector could reach. It is now true for every reviewer, so the sentence is made true rather than hedged. ## Tests Three in `ReviewChangeSetTest`, one scenario each, mutation-checked against 369 flow tests: - a two-round loop whose fixer commits, asserting the round-2 resumed reviewer's prompt carries the change set — restoring the `needsDiff` gate fails it; - a pinned `initialDiff` is not re-sent to the resumed reviewer — forcing `AlreadySeen` off fails exactly it; - a change set past the inline threshold arrives as paths, not hunks — forcing `TooLarge` off fails exactly it. The first cannot pass for the wrong reason: a sampled-but-empty diff renders the no-sample fallback, and the re-review template contains no filename. That round 2 takes the resume path is already pinned by `ReviewAndFixTest`'s "reviewer is called with the same session id on every iteration". ## What to check `ReviewLoop.scala` changes in five separate places: the sampling in `runReviewersAndLint`, the session fold beside it, `SessionEntry`, the `resumeReview`/`reviewWithSession` pair, and the scaladoc. `ReviewLoopState` itself is unchanged — only `SessionEntry` gains a field. No change to the selector, `Lint.scala`, `CheckedPar`, or the capture-checking structure; `CcNegativeCompileTest` still rejects an `InStage` capture. Also touched: `ReviewLoopPrompts.scala`, the three prompt resources, one README paragraph and one README bullet. `sbt scalafmtCheckAll` clean; `sbt clean compile test` green with zero warnings.

adamw added 2 commits August 2, 2026 15:07

adamw mentioned this pull request Aug 2, 2026

Skip untracked symlinks-to-directories in the review diff #63

Merged

adamw added 2 commits August 2, 2026 18:42

Merge remote-tracking branch 'origin/master' into HEAD

c05dcbd

Merge remote-tracking branch 'origin/master' into HEAD

19bba13

# Conflicts: # README.md # adr/0011-reviewer-roster.md

Merge remote-tracking branch 'origin/master' into HEAD

0d131b6

# Conflicts: # tools/src/main/scala/orca/tools/GitTool.scala

adamw merged commit 08d83bb into master Aug 2, 2026
6 checks passed

adamw deleted the review-change-set-since-stage-base branch August 2, 2026 19:23

This was referenced Aug 2, 2026

Research: should the re-review prompt carry more payload? (T2.5) #65

Merged

Send the re-sampled change set to resumed reviewers too #72

Merged

Navigation Menu

Uh oh!

Review the change set since the stage began, not since HEAD - #59

Review the change set since the stage began, not since HEAD#59
adamw merged 5 commits into
masterfrom
review-change-set-since-stage-base

adamw commented Aug 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

adamw commented Aug 2, 2026

The defect

The fix

Selection across rounds

API

Tests

Review notes

Found in review, deliberately not fixed here

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant