Skip to content

Keep file-gated reviewers on an empty diff; narrow the reviewer set across rounds - #48

Merged
adamw merged 5 commits into
masterfrom
review-selection-narrowing
Aug 2, 2026
Merged

Keep file-gated reviewers on an empty diff; narrow the reviewer set across rounds#48
adamw merged 5 commits into
masterfrom
review-selection-narrowing

Conversation

@adamw

@adamw adamw commented Aug 1, 2026

Copy link
Copy Markdown
Member

Two independent defects in reviewer selection, in the same two files.

1. An empty diff silently dropped file-gated reviewers

reviewAndFixLoop derives changedFiles from the review diff, which is Nil
whenever that diff is empty. ReviewerSelector.agentDriven then filtered
candidates by their files: pattern, so scala-fp (today the only reviewer
declaring one) was removed before the picker ever saw it: on a Scala change with
an empty diff, the Scala reviewer simply did not run. The safety floor could not
restore it, because it falls back to eligible, not all.

An empty change set now means "which files changed is unknown", not "no files
changed": the pre-filter is skipped and an OrcaEvent.Step names the reviewers
it kept eligible.

2. The reviewer set never narrowed across rounds

agentDriven.prepare computed its pick once and returned _ => active,
discarding the review history it was handed — so every picked reviewer ran in
every round of the fix loop, whether or not it had anything left to say. A
measured run spent 11 rounds this way.

New combinator ReviewerSelector.narrowingAcrossRounds(base): round one runs
base's pick; every later round keeps only those of base's reviewers that
reported an issue in the previous round. It only ever filters base's result,
so a reviewer the picker excluded is never resurrected. A reviewer that reported
is the one whose finding the fixer just acted on, so it must re-check its own
verdict; everything else would re-read an unchanged verdict at a full turn's
cost per round.

Narrowing never empties a non-empty set. Reviewer silence alone ends the loop,
but a lint gate keeps it iterating through silence while the fixer edits;
without a floor the narrowed set is absorbing and later rounds run zero
reviewers. When narrowing would leave none, base's pick runs again and a
Step says so.

A files: pattern gates eligibility only

Per ADR 0011 the files: frontmatter exists to keep a language-specific
reviewer off changes it cannot speak to — it is a relevance filter, not a
priority signal. So it grants no exemption from narrowing: a quiet scala-fp is
dropped like any other quiet reviewer. Since narrowing only ever filters the
picker's own result, such an exemption could in any case only retain an
already-picked reviewer; it never re-admitted one the picker had excluded.

Dropping it removes narrowingAcrossRounds's filePatterns parameter and the
four-case FileClaim enum, which existed only to answer the eligibility and
narrowing questions differently. One question is left, so eligibility is one
predicate.

It also leaves changedFiles with exactly one consumer —
ReviewerSelector.prepare, which runs once, at loop start (ADR 0011: the
per-round arrow is pure and cannot re-sample the diff). The loop-start diff
sample can therefore no longer go stale against a later round's edits.

Default, not opt-in

ReviewerSelector.default (= narrowingAcrossRounds(agentDriven)) is now
reviewAndFixLoop's shipped default, replacing bare agentDriven.

The evidence for not shipping this as opt-in is in this repo:
ReviewerSelector.onlyPreviouslyReporting has implemented narrowing for a while
and no shipped flow uses itimplement.sc, implement-enhanced.sc,
issue-pr.sc and issue-pr-bugfix.sc all take the default. Another opt-in
would change nothing for anyone. The cost this removes is the dominant term in a
review stage (reviewers × rounds), and the narrowing rule is conservative on the
axis that matters: the reviewer that raised an issue always re-checks the fix.

What is given up is stated plainly in the README and the scaladoc: a reviewer
that goes quiet won't see fixes made after it stopped running.
ReviewerSelector.allEveryRound remains the one-word opt-out for flows that
want regression coverage over tokens, and agentDriven(model) still means "pick
once, replay every round" for anyone who wants exactly today's behaviour with a
chosen picker.

Expected effect on review rounds

Round one is unchanged (picker output, now including file-gated reviewers that
an empty diff used to hide). From round two on, the active set is the previous
round's reporters — in a converging loop that shrinks toward the one or two
reviewers still finding things, instead of staying at the full picked set for
every round. Rounds themselves are not capped by this change.

Regression guarded

A reviewer dropped in round two must still contribute its retained
gateRejects to the final IgnoredIssues — that is exactly why the loop keeps
each agent's last-seen rejects. The test pinning it now drives the shipped
default (the bespoke first-round-only selector it used is gone), so the
guarantee is asserted against the behaviour users actually get.

Known limitation, not addressed here

The diff both the selection and the reviewer prompts are built from is
git.reviewDiff()git diff HEAD plus untracked file contents. It is
therefore empty whenever the implementing agent has committed its own work, and
reviewer selection then runs on an empty change set while every first-time
reviewer is prompted with "(no diff captured — review the working tree)". The
mitigation above (an empty change set no longer drops file-gated reviewers)
limits the damage but does not fix the cause.

Fixing it properly needs a base commit to diff against, and the natural one —
HEAD as the enclosing stage began — is recorded nowhere: neither FlowContext
nor ProgressHeader nor StageFrames carries it, and startingBranch is not a
substitute (in skip-branch mode it is the current branch). That is
cross-cutting plumbing rather than a change to these two files, so it is left
out of this PR. initialDiff = Some(...) remains the per-call escape hatch.

Tests

  • ReviewerSelectorTest: an empty diff keeps file-pattern reviewers eligible
    and announces the skipped filter; narrowing re-runs only the previous round's
    reporters, never empties the active set, and never resurrects a reviewer the
    base excluded.
  • ReviewAndFixTest: in a three-round loop a reviewer that reports nothing in
    round one opens exactly one session; a gate reject survives its reviewer being
    narrowed out; a lint-only round still leaves the next round with reviewers.

sbt scalafmtCheckAll and sbt clean compile test are green with zero
warnings; every behavioural change above is mutation-checked.

adamw added 4 commits August 2, 2026 13:55
Two selection fixes in `ReviewerSelector`:

- An empty diff no longer drops file-pattern reviewers. `changedFiles` is
  Nil whenever the diff is empty (e.g. the work is already committed), and
  the pre-filter read that as "no files changed", silently removing every
  reviewer with a `files:` pattern before the picker saw it — the safety
  floor could not restore them, since it falls back to `eligible`, not
  `all`. An empty change set is now treated as unknown: the pre-filter is
  skipped and a Step names the reviewers it kept.

- The picked set now narrows as the loop iterates. `agentDriven` computed
  one pick and replayed it every round, so every picked reviewer ran in
  every round. The new `narrowingAcrossRounds` combinator wraps a selector
  so later rounds keep only the reviewers that reported last round plus
  those whose file pattern matches the change set;
  `ReviewerSelector.default` composes it over `agentDriven` and is the
  loop's new default.

Gate rejects still survive a reviewer being narrowed out — the loop's
last-seen-per-agent set is unchanged, and the test that pins it now runs
through the shipped default rather than a bespoke selector.
…ewers

Review feedback on the narrowing default:

- Narrowing had no lower bound. Reviewer silence alone ends the loop, but a
  lint gate keeps it iterating through silence, and the narrowed set was
  absorbing: once a round produced no gate-passing reviewer issue, later
  rounds ran zero reviewers while the fixer kept editing. The set now never
  narrows to nothing — the selected set runs again and a Step says so.

- On an empty diff every file-gated reviewer was permanently exempt from
  narrowing, because one helper answered both "may this reviewer be dropped
  before the picker sees it?" and "does it claim the files under review?".
  An empty change set means unknown, which is a reason to keep a reviewer
  eligible but not a standing claim to re-run it forever. The two questions
  now go through a named `FileClaim` (Ungated/Matches/NoMatch/Unknown), so
  each call site states which cases it acts on.

Also: delete `onlyPreviouslyReporting`, which had no call sites and narrowed
without either floor; name the way back to the previous default (parameterless
`agentDriven`) in the loop's own param doc and the README; drop a rationale
comment that described a mechanism the code can't have; and pin the two fixes
with tests that fail without them, including the first test to combine a lint
gate with the default selection.
Review round two, docs plus test coverage:

- The README described the floor as firing when nobody reported, but it
  fires when narrowing would leave NO reviewer — a file-claiming reviewer
  surviving a silent round means no fallback and no Step. Reworded, along
  with the "stops costing a turn per round" claim, which doesn't hold in the
  lint-driven rounds where the floor fires.

- ADR 0011 and the `ReviewerSelector` trait doc said the per-round arrow is
  "checked to capture nothing" and told implementers to hoist every
  per-round effect into `prepare`. What `->` enforces is that no CAPABILITY
  is captured — `InStage` above all; untracked values such as `FlowContext`
  are outside the check, and the shipped default captures one to announce
  its decision. Both now state the enforced contract, and the ADR gains an
  amendment covering the default's composition and its two bounds.

Tests: the "never resurrects" case only excluded a reviewer on the ground
that didn't matter, so a union implementation passed it; it now excludes on
both grounds. The `active.isEmpty` arm of the floor guard, the file-claim
exemption at loop level (every other default-selector test pins an empty
diff, where no pattern can match), and the absence of a phantom "re-running
all 0 reviewer(s)" announcement are now pinned too — each verified by a
mutant that the suite previously let through.

Eligibility routes through an exhaustive match, so a `FileClaim` case added
later can't silently land in the ineligible bucket.
A reviewer's `files:` frontmatter gates whether it is offered to the picker
at all — per ADR 0011 it keeps a language-specific reviewer off changes it
cannot speak to. It is not a priority signal, so it no longer also exempts a
reviewer from per-round narrowing: since narrowing only ever filters the
picker's own result, that exemption could only retain an already-picked
reviewer, and it retained a quiet `scala-fp` on grounds that apply to no
other quiet reviewer.

Removing it drops `narrowingAcrossRounds`'s `filePatterns` parameter and
the four-case `FileClaim` enum, which existed only to answer the eligibility
and narrowing questions differently. One question is left, so eligibility is
one predicate. It also leaves `changedFiles` with a single consumer —
`ReviewerSelector.prepare`, which runs once, at loop start — so the
loop-start diff sample can no longer go stale against a later round.

Docs: the `->` scaladoc and the ADR amendment state the rule an implementer
follows rather than explaining capture checking, and the README's
reviewer-selection section is a paragraph plus a selector table instead of
the same trade-off restated three times.
@adamw
adamw force-pushed the review-selection-narrowing branch from 473df95 to b2fc65d Compare August 2, 2026 14:08
@adamw
adamw merged commit dc8a79e into master Aug 2, 2026
6 checks passed
@adamw
adamw deleted the review-selection-narrowing branch August 2, 2026 18:56
adamw added a commit that referenced this pull request Aug 4, 2026
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.
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