mirror of https://github.com/QwenLM/qwen-code.git synced 2026-07-17 04:59:06 +00:00

feat(review): give every line of a large diff an accountable reviewer (#6612 )

* feat(review): give every line of a large diff an accountable reviewer

Review agents were handed the diff *command* and left to run it themselves.
Shell tool output is capped at 30 000 characters and split head-1/5 / tail-4/5,
so on a large changeset every agent received a few hundred lines off the top of
the first file, the tail of the last file, and a truncation marker in place of
everything between. Measured on a 211 000-character diff: 14.4% of the
changeset, the same 14.4% for all ten agents. Nineteen of the twenty defects
maintainers eventually confirmed on that PR lay in the hidden 85.6%. The
ten-way dimension fan-out multiplied redundant reads of the visible sliver
rather than adding coverage, and each review round sampled a different subset
of the bugs depending on which files an agent happened to open on its own.

The diff is now captured to a file and partitioned. `read_file` still caps a
single read at ~25 000 characters, so writing the diff out is necessary but not
sufficient — a whole-file read of that diff returns its first 611 lines. Chunks
are therefore bounded by both a line budget (attention) and a character budget
(what one un-truncated read returns), split on hunk boundaries, and never
through the middle of a function. They tile the diff exactly, which is what
makes the new coverage receipts checkable: past 500 diff lines each chunk gets
one agent that owns it and must account for it, and a chunk with no receipt is
re-reviewed before the run proceeds. "No blockers" can no longer be reported
over code nobody read.

Coverage alone did not close the gap. Chunk agents held every state-machine
defect in that PR inside their assigned territory and reported none of them:
the bugs were not inside any hunk but between new lines sitting two thousand
lines apart, and what the agents lacked was not the lines but the question. A
heavily rewritten file now also gets three whole-file agents that walk a fixed
invariant checklist — mutable fields cleared on every exit path, timers
cancelled on every close without discarding captured data, map inserts matched
by deletes, retry counters incremented at every entry, status returns actually
checked, error codes classified permanent versus transient, config honoured on
every path, early returns that skip a required side effect. The checklist is
split three ways deliberately: one agent asked to run all eight checks over a
2 400-line file runs one of them properly.

Verification is sharded at eight findings per agent, because one verifier
re-reading code for sixty findings degrades on the tail of its list. A verifier
may now downgrade a Critical but never delete one — a rejected Critical is
invisible to every later stage, a downgraded one still reaches a human. The
reverse audit fans out per chunk instead of asking a single context-starved
agent to re-read the whole diff, no longer skips verification, and stops after
two consecutive dry rounds rather than one: on the PR that motivated this, the
review reported "no blockers" twice and the next round surfaced five Criticals,
three of them in code present since the first commit.

* fix(review): keep small-diff reads inside the read_file cap

Step 3A told every agent to read the whole diff in one call. `read_file`
truncates a single call at ~25 000 characters, so a 500-line diff of long lines
would come back short — the same blind spot the chunk plan removes, reintroduced
at a smaller scale. Across the last 39 merged PRs that take the Step 3A path the
largest diff is 23 570 characters, so this never fired in practice, but the
margin is six percent. Step 3A now walks the chunk ranges, which are sized to
fit one un-truncated read: one or two calls at this size.

Derive a file's pre-change line count from the diff instead of measuring it with
a second `git show` per file. `git show <base>:<newpath>` returns nothing for a
renamed file, reporting zero pre-change lines and classifying a wholesale
rewrite as light. The identity holds exactly for creations, deletions, renames
and ordinary edits, and halves the process spawns.

* fix(review): choose the topology from source lines, not diff lines

Diff size is a bad proxy for review risk because test code dominates it. Across
this repo's last 40 merged PRs the median diff is 41% test code and 14 of the 40
are more than half tests; PR #6457, which motivated the territory fan-out, is
itself 58% tests. Gating on raw diff lines therefore carved small production
changes into territories: a change of 173 source lines shipping 489 lines of new
tests went to the chunked topology, where its production code ended up owned by
a single agent, when the dimension fan-out would have read it through eight
lenses. Territory fan-out is worth it when there is a lot of risky code to
divide, not a lot of lines.

The gate is now `srcDiffLines > 500`, with `diffLines > 2400` as a second clause
— a delivery bound rather than a risk one, since past that point chunking uses
fewer agents than the ten-lens topology anyway and reading a diff that large
dilutes all ten. On the 40-PR sample six PRs move back to the dimension fan-out,
for about 5% more agents in total across the sample.

Paths are classified as source, test, or generated, and the per-kind line counts
ship in the fetch report. Chunking is unchanged: the plan still tiles every
line, tests and generated files included. What the gate decides is how many
reviewers there are and what each is asked to do. Heaviness is likewise
restricted to source files — the invariant checklist asks about fields, timers,
collections, and error taxonomies, and a rewritten test file has none of those.

* fix(review): decode C-quoted diff paths as bytes

`git diff` C-quotes any path with a control character or a non-ASCII byte, so a
file named `sub/中文文件.ts` arrives as `"b/sub/\344\270\255..."`. The chunk
planner stripped the backslashes, turning it into `sub/344270255...ts` — a name
that exists nowhere. Every downstream use of the path then failed silently: the
line count came back zero, the file could never be classified as heavy, and the
chunk agent was told it was reviewing a file that does not exist. Reuse core's
`unquoteCStylePath`, which reassembles the octal escapes as UTF-8 bytes, rather
than keeping a second, wrong decoder here.

Coverage was never affected — line ranges stayed correct — but this repo has
non-ASCII paths, so the mislabelling was reachable.

Also correct two places that claimed hunks are never split. They are: a hunk
larger than the chunk target is split at a top-level declaration, because a
brand-new file arrives as one enormous hunk and treating it as atomic would hand
a single agent a 50 000-character territory.

* fix(review): make diff capture and header parsing robust to git config

Four defects, all found in review of this branch.

Diff capture obeyed whatever the user's git config said. With `color.diff=always`
every `diff --git` line arrives wrapped in ANSI escapes, the parser recognises
none of them, and the plan comes back with zero files and zero chunks — the
coverage guarantee silently evaluates to nothing. `diff.mnemonicPrefix` renames
the `a/`/`b/` prefixes to `i/`/`w/` and every path is then wrong; `diff.external`
and textconv filters emit output that is not a unified diff at all. Capture now
pins `--no-ext-diff --no-textconv --no-color --unified=3` and the two prefixes.

The `diff --git` header was split with a greedy regex. Git separates the two
paths with a space and does not quote a path merely for containing one, so
`a/img with space.png b/img with space.png` split into `space.png`. Usually the
`---`/`+++` headers disambiguate, but a binary or mode-only section has neither.
For a non-rename both paths are the same string, so the split point is
arithmetic; a rename states its new path outright in `rename to`.

A chunk boundary could land on a `-` line. Those exist only on the old side, so
the "starts at a top-level declaration" guarantee did not hold for the
post-change file an invariant agent later reads. Split points are now restricted
to lines present on the new side.

An `oversized` chunk — one hunk with no safe interior boundary — can exceed what
a single `read_file` returns. Chunks now carry their character count, and a
chunk agent is told to page when a read reports truncation. A `Covered:` receipt
for a range the agent only half read is worse than no receipt at all.

* fix(review): split past a distant boundary, and stop probing GitHub for anchors

Both defects surfaced running the new review against PR #6591.

A 1431-line React component was emitted as a single 45 675-character chunk —
nearly twice what one `read_file` returns — because the splitter looked for a
safe boundary only inside the 400-line budget window, found none, and gave up on
the entire remainder. Twenty-seven boundaries existed further along; the first
sat 460 lines in. It now reaches past the window for the next one, so a single
distant boundary can no longer collapse a whole file into one chunk. That PR
goes from 15 chunks with one over the read cap to 18 with none.

Step 7 validated comment anchors by trial. GitHub rejects an entire review with
a 422 if any comment's line falls outside every hunk of its file, and the skill
offered no cheap way to check, so a run against a real PR submitted five
throwaway reviews carrying the bodies `Test`, `Test`, `t`, `t`, `t` to discover
which anchors would stick. Those are permanent, public reviews on someone else's
pull request. The fetch report now carries each file's hunks as new-side line
ranges, which turns the check into a lookup, and the skill states plainly that
a review is never submitted to test an anchor.

* fix(review): stop reading hunk payload as metadata, and harden the plan

Eleven defects from review of this branch. The worst two were silent.

A unified diff emits a removed line whose content starts with `-- ` as
`--- ...`, and an added line whose content starts with `++ ` as `+++ ...`. SQL,
Lua and Haskell comments start with `-- `. The parser read those payload lines
as file headers: the path was overwritten by the line's text, and the line
vanished from the add/remove counts. A two-file diff — one SQL file losing a
comment, one text file gaining a `++ ` line — came back with the second file
named `plus line`. Metadata is now only recognised before a file's first hunk.

The tiling invariant — every diff line belongs to exactly one chunk, which is
what makes a missing coverage receipt mean something — was asserted only in
tests. `buildDiffPlan` now checks it and refuses to return a plan with a hole.

The rest: a split point could take a *deleted* blank line as evidence of the
blank line before a declaration, though that blank exists only in the old file;
whole-file invariant agents were pointed at `chunks[].files[]`, which merges
hunks at lines 10 and 900 into one `10-902` span and would have had them report
pre-existing defects as new; pure-deletion hunks were exported as the inclusive
range `[N, N]`, so a right-side comment could be anchored where GitHub has no
line and the 422 would sink the whole review; a deleted file could be marked
heavy and send three agents to read a post-image that does not exist; a chunk
holding a single line longer than one `read_file` can never be fully read by
paging, and must now report itself uncoverable rather than receipt a lie;
capture did not pin rename detection or `--no-relative`; `gitRaw` had no
timeout, so a credential prompt on headless CI would hang forever; a failed
base fetch was swallowed, leaving a stale merge-base and a structurally
complete report describing the wrong diff; and local reviews still captured
with a bare `git diff`, which `color.diff=always` alone renders unparseable.

Adds an integration test that drives the real capture against a real repository
under hostile git config, covering the paths synthetic fixtures cannot: renames
and binaries and mode-only changes with spaces in their names, C-quoted
non-ASCII names, and payload lines that impersonate headers.

* fix(review): pin submodule output, and separate written lines from hunk spans

Four defects from review of this branch.

Diff capture left submodules to user config. `diff.ignoreSubmodules=all` hides a
changed gitlink completely — a silent coverage hole in the file that is now the
review's source of truth — and `diff.submodule=log` replaces the whole
`diff --git` section with prose no parser can read. Both are pinned now, and the
integration test asserts a bumped gitlink survives them.

Whole-file invariant agents were handed `files[].hunks[]` as "the changed
lines". A hunk spans the three context lines git prints either side of every
change: on PR #6457's `QQChannel.ts` those spans cover 1 962 new-side lines of
which only 1 403 were written. The agent would have reported defects in 559
lines that predate the PR. The report now also carries `addedRanges[]` — the
exact lines the change wrote — and the skill gates invariant agents on those,
keeping `hunks[]` for the one thing it is right for, GitHub anchor validation.

`Uncoverable:` was introduced as a chunk agent's answer for a chunk holding a
line longer than one read, but the receipt accounting still demanded a
`Covered:` line from every chunk and relaunched any chunk lacking one — so an
uncoverable chunk would have been retried forever. It is now a first-class
terminal status: accepted by the accounting, carried into Step 6 under "Not
reviewed", and it blocks an Approve verdict. Step 3A, which also walks the
chunk plan, is covered by the same rule.

The integration test built its fixture repository inside the developer's git
environment, so a global `core.hooksPath` or `commit.gpgsign` ran during the
test and `~/.gitconfig` decided what the "clean" baseline was. It now disables
system and global config, hooks and signing, and sets the executable bit through
the index rather than shelling out to `chmod`, which does nothing on Windows.

* feat(review): plan any captured diff, and stop the report outgrowing one read

Seven items from review of this branch. None blocking; two of them were the
skill promising a topology it could not deliver.

Step 3B's chunk agents are "one per entry in `chunks[]`", and only `fetch-pr`
produced a chunk plan. A local-diff review, and a cross-repo review in
lightweight mode, therefore routed into the territory fan-out with no chunk
list, no receipts and no tiling guarantee. `qwen review plan-diff <diff-file>`
now emits the same plan from any captured diff; redirecting `git diff` or
`gh pr diff` to a file already sidesteps the shell's character cap, so all four
review paths share one mechanism. A bare diff has no tree to read a post-image
from, so it gets chunk agents but no invariant agents, and says so by omission.

The fetch report is read with the same `read_file` that truncates at 25 000
characters — and for a seven-file PR it was already 28 056. The tail of
`chunks[]` was being silently lost: the coverage hole this design closes,
reappearing one level up. `addedRanges[]` now ships only on `heavy` files, its
only consumer, which brings that report to 24 992; the skill says to page the
read; and the command prints a note when the report exceeds one read. It stays
pretty-printed on purpose — a compact one-line JSON cannot be paged by line.

The tiling assertion threw inside `fetch-pr` after the worktree existed and
before any report was written, so an unforeseen diff shape killed the review
outright. It now degrades to the documented diff-less report with a loud
warning, keeping both the loudness and the review.

`gitOpt` and `git` had no timeout, and `resolveMergeBase` uses `gitOpt` for a
network fetch — the exact path whose credential prompt the `gitRaw` timeout was
added to survive. All three wrappers now share a deadline and
`GIT_TERMINAL_PROMPT=0`.

Markdown under `docs/` or at the repository root classifies as `docs` and stays
out of `srcDiffLines`, so a translation PR does not trip the territory gate.
Markdown inside a source tree stays `source` — the bundled skill prompts are
behaviour, not prose.

Also: the user docs stated the gate without its `diffLines > 2400` clause, and
`READ_FILE_CHAR_CAP` was exported but never used. It now backs the report-size
warning.

* test(review): unit-test the merge-base and plan-report seams

The last open review thread asked for `resolveMergeBase`, `fileMetrics` and
`gitRaw` to be testable with git mocked out. Three of the four functions it
named have since moved: `classifyHeavy` is a pure function with unit tests,
`fileMetrics` became `buildPlanReport`, which already takes an injected
post-image resolver, and `gitRaw`'s output path is exercised by the real-git
integration test. `resolveMergeBase` was still private and untested.

It now lives behind a three-method `GitProbe` — fetch, refExists, mergeBase —
that `fetch-pr` fills from the real wrappers. Seven tests cover the branches
that matter and that no end-to-end run reaches: the tracking ref preferred over
the local branch, the fall-through when the tracking ref shares no history, and
above all the dangerous one — a failed fetch that still resolves a merge-base
from a stale local ref, which produces a structurally complete report describing
a diff nobody wrote.

`buildPlanReport` gains seven of its own: the injected resolver is asked once
per file and never for a binary, a null resolver means "no tree, decide nothing"
rather than a guess, `addedRanges` ship only where an invariant agent will read
them, and a pure-deletion hunk never reaches the anchorable ranges.

* fix(review): see deletions, survive suppressBlankEmpty, and stop approving unread code

Seven findings from review of the merged head. Three of them were the design
contradicting itself.

`diff.suppressBlankEmpty` prints a blank context line as a physically empty
record rather than a lone space, and there is no command-line flag to override
it — only `-c`. The parser advanced its new-side cursor for space-prefixed
context alone, so every `addedRanges` entry after the first blank line shifted
up by one, and the split-point heuristic stopped recognising blank lines. The
capture now pins the config, and the parser treats an empty hunk-body record as
context regardless, because a diff from `gh pr diff` or a hand-captured file
never passes through that pin.

A whole-file invariant agent was given the post-change file and the ranges the
PR wrote. A deletion appears in neither. Removing a `clearTimeout()`, a
`Map.delete()`, or a retry-counter increment is exactly what the checklist
hunts, and the text it was handed cannot show a line that is no longer there —
telling it to "cite the surrounding hunk" pointed at data it never received.
Heavy files now carry a `diffRange` into the report, and the agent reads its own
slice of the diff, where the `-` lines are.

The receipt accounting demanded exactly one per chunk and said it applied to
Step 3A, where nine dimension agents each walk every chunk: literal execution
yields nine receipts or none. Territory ownership is a Step 3B idea. What both
paths share is the uncoverable rule, and that needs no agent — a chunk is
uncoverable iff its `maxLineChars` exceeds the read cap, which the orchestrator
reads out of the plan before launching anything.

That rule was also never threaded into Step 7, so a green PR with an unread
chunk could receive a public LGTM. Any uncoverable chunk now downgrades APPROVE
to COMMENT and must be named in the body.

Also: the capture recipes redirected into `.qwen/tmp` before anything created
it; a file-path review of an unchanged file produced an empty plan that no agent
could read, and the skill now branches to a full-file read instead; and the docs
classifier called `website/src/App.tsx` prose while calling
`packages/cua-driver/docs/*.md` source — it now matches prose extensions under a
documentation directory at any depth.

* fix(review): tell agents what a severity means before asking for one

The severity definitions lived once, in Step 6 — after every severity had
already been assigned. Step 3's finding format asked each agent for
`Severity: Critical | Suggestion | Nice to have` and never said what the words
meant. The agents that fill that field are separate subagents with separate
priors and no shared definition between them, so each fell back on its own, and
the priors disagree.

Observed on a live review of PR #6635 — a run of the skill as it stands on main,
whose Step 3 and Step 6 text this branch inherits unchanged. One review,
CHANGES_REQUESTED, ten inline comments. Six were Critical, and four of those six
were coverage gaps: "zero test coverage", "no references to `workers`", "no test
exercises this". Two Suggestions in the same review were the identical class.
The verdict is computed from Criticals alone, so that PR was blocked partly on
the strength of findings its own reviewer had, elsewhere, called suggestions.
The two genuine Criticals — a fail-fast that no longer fires before the daemon
reports healthy, and a startup failure path that never closes the HTTP server —
would have blocked it on their own.

The definitions now sit in the finding format that every agent is handed, they
are listed among the things every agent prompt must carry, and Step 6 points
back at them rather than restating them. A missing test is a Suggestion: "this
file has zero references to X" is a coverage statistic, not a defect. Two shapes
stay Critical because something is genuinely wrong — a test asserting the
opposite of the intended behaviour, and a test weakened or deleted in the diff
so new behaviour passes. If a missing test would let a specific incorrect
behaviour ship, report that behaviour and cite the gap as evidence.

* fix(review): walk cross-file edges in both directions

Cross-file impact analysis only ever asked "will the existing callers break?"
Every bullet was about signature compatibility, and the budget rule told agents
in so many words to "skip unchanged-signature modifications". A field added to
an interface changes no signature and breaks no caller, so the analysis was
blind to it by construction.

The failure that exposed this, on PR #6621: the diff added `deviceFlowRegistry?`
to WorkspaceRuntime and passed it into the dispatcher for every secondary ACP
mount, and nothing anywhere assigned it. The reviewing agent saw the
declaration, found no writer, wrote "intentionally deferred to a later
milestone", and filed a Suggestion to fix the JSDoc. The reader was AcpDispatcher
— a file the diff never touched — where `if (!this.deviceFlowRegistry)` turned
`auth/device_flow/start` into an INTERNAL_ERROR and `auth/status` into an empty
list on every non-primary workspace. Workspace-qualified ACP shipped its
authentication dead, and the review called it a documentation nit. A second
reviewer filed the same observation as Critical; the author fixed it with code
and dropped the field.

Reading cannot find this. The declaration, the pass-through, and the read sit in
three different places, and the read is outside the diff, so no agent reaches it
by paging through hunks. Only a grep for the read sites does.

So: for every field, option, or optional parameter the diff adds, grep its read
sites, including outside the diff, and ask what happens when it arrives
undefined. Severity is decided at the read site, not the declaration. And an
agent must not explain an unpopulated field with author intent it cannot
observe — "reserved for future use" is a claim about a person, not about code,
and reaching for one means filling a hole in your own field of view.

* fix(review): pin the diff base, and make the review body checkable

Three defects, all found by reading what live reviews actually posted.

The diff base. Agents were handed a diff command and left to choose a base.
`main..HEAD` and `main...HEAD` differ by one character and by the entire meaning
of the review: a two-dot diff against a main that has moved shows main's later
commits reversed, so main's fixes read as the branch's regressions. A review of
PR #6626 approved the four files the PR actually changed, then warned the author
publicly that their branch carried "typo regressions" in a file the PR never
touched and should be rebased. main had corrected `compatability` to
`compatibility` after the fork point. The branch had done nothing. Capture now
resolves the base once and hands agents a file; they never see a ref name, and a
finding in a file outside the report's `files[]` is not a finding about this PR.

The review body. "A Suggestion never goes in body" is stated twice and was
violated anyway, because a model holding a finding it cannot anchor would rather
say it somewhere than drop it. On PR #6631 an unanchorable Suggestion about
`session.ts:2048` — a line in no hunk — became a second paragraph of the public
review body. So the rule stops being prose: for COMMENT the body is exactly one
of three sentences plus the footer and nothing else, and you read what you are
about to send and confirm it. A Suggestion that will not anchor is deleted; it is
already in the terminal output and the Step 8 report.

The downgrade sentence. On PR #6489 a review with three Suggestions and no
Critical announced it had been "downgraded from Approve" — telling the author the
PR would otherwise have been approved, which was false: a Suggestion-only review
is COMMENT on its own. Decide the event from the findings first, apply the
downgrade flag second, and write the sentence only if it changed the answer.

* fix(review): decide the event by counting, not by weighing

A review of PR #6584 filed three inline Suggestions and submitted APPROVE with
an empty body. GitHub recorded it as an approval. The rule it broke has been in
Step 7 all along --- APPROVE means no Critical *and* no Suggestion --- and so has
the one about the body, which is empty only for REQUEST_CHANGES. Both were
stated twice. Both were ignored.

They are ignored because at submit time the model is reasoning about what it
wants to say, and "these are only suggestions, the PR is fine" is a sentence it
can talk itself into. Nothing in that sentence is a count.

So the event and the body become arithmetic. Count the Criticals, count the
Suggestions, read the row off a three-row table, and only then apply the
downgrade flags --- which can turn APPROVE or REQUEST_CHANGES into COMMENT and
nothing else. Then read back what you are about to send and confirm it matches
the row. A body holding text the table does not authorise is a finding that
failed to anchor; if it is a Suggestion, it gets deleted, not relocated into
public prose that no line of code answers to.

This subsumes the body-only invariant added in the previous commit, which the
same submit-time reasoning had already defeated once, on PR #6631.

* fix(review): stop the plan report outgrowing the read it must fit in

The report tells an agent how to page everything else, so it has to be readable
in one `read_file` — about 25 000 characters. Running the real `fetch-pr`
against PR #6457 produced 25 070.

Two constraints pull against each other. Compact JSON is a single enormous line,
and `read_file` pages at line boundaries, so a report too big for one call could
never be read at all. Indented JSON pages fine but spends four lines on
`{ "start": 812, "end": 815 }`, and a heavily rewritten file contributes hundreds
of them: `QQChannel.ts` alone carries 140 added ranges and 49 hunks.

So indent the structure and inline the leaves. Same JSON, same keys, one range
per line, still pageable — and 28% smaller. The #6457 report goes from 25 070
bytes to 18 042, and the "page it" warning that used to fire on a seven-file PR
now stays quiet.

The earlier attempt at this trimmed `addedRanges` to heavy files only and landed
at 24 992 bytes on the same PR. Eight bytes of headroom was not a fix.

Tests pin the three properties that matter: the collapsed text parses back to an
identical object, no range spans two lines, and a path that literally spells a
range is not mistaken for one — JSON escapes the quotes inside a string value,
and the collapse patterns require unescaped ones.

* fix(review): prune the worktree registration a deleted directory leaves behind

`cleanStale` and `cleanup` both guarded `git worktree remove` behind
`existsSync(path)`, and neither ever pruned. Delete the directory by hand — which
is exactly what reclaiming disk with `rm -rf .qwen/tmp` does — and git keeps the
worktree registered but missing. From then on `/review` on that PR cannot run:

$ git worktree add .qwen/tmp/review-pr-6457 qwen-review/pr-6457
fatal: '...' is a missing but already registered worktree;
use 'add -f' to override, or 'prune' or 'remove' to clear

and the branch delete that `cleanStale` does next fails too, because the phantom
worktree still has that branch checked out. Nothing in the review command surface
ran `git worktree prune`, so nothing ever cleared it.

This surfaced running the real skill: the orchestrator's first `fetch-pr` failed,
it fell back to `qwen review cleanup`, and retried. The leak is not rare — three
abandoned worktrees from May and June were still registered in this checkout,
one per review that died before Step 9.

`releaseWorktree` now does both halves in the order they depend on: remove the
directory if it is there, prune the registration unconditionally (a no-op when
nothing is stale), and only then let the caller delete the branch. Both callers
share it.

The tests drive real git. Deleting a worktree directory by hand and re-adding it
throws "missing but already registered" without the prune, and `branch -D` throws
"used by worktree" — both assertions fail if the prune is removed, which is the
point of writing them.

* fix(review): put the open comments where a truncated read will find them

`read_file` returns the first `truncateToolOutputThreshold` characters — 25 000
by default — sets `isTruncated`, and pages by line. `pr-context` wrote
"## Open inline comments (no replies yet — may still need attention)" last, so
on a PR with a long history it was the first thing lost, and nothing read the
flag that said so.

On PR #5738 that section began at character 27 125 of a 31 220-character file.
The review submitted "Reviewed — no blockers." Five Critical threads were
unresolved; four had in fact been addressed, but the fifth — `clearCiEnv()`
clearing only `CI*` while `writeTerminalTitle` branches on `TMUX`/`STY`/
`ZELLIJ`/`DVTM` — was live, in the diff, and never seen.

Regenerating the context for ten PRs: four lost part or all of the section, and
all four were the PRs with the most review rounds. Small PRs never trip it.

- Emit the open threads before the already-discussed ones. The findings a round
must answer outrank the ones already settled.
- `pr-context` warns when the file exceeds the threshold, naming any headings
past the cut, and says so plainly when the loss is inside the last section's
body instead.
- Step 2 of SKILL.md now tells the agent to read `isTruncated` and page the
remainder before Step 3.

Reordering buys headroom; it does not create it. A 40 000-character context still
loses its tail, which is what the warning is for.

* fix(review): load this repo's review rules, and re-check open Criticals before approving

Two gaps the dogfood on live PRs surfaced, both invisible from reading the skill.

`load-rules` looks for a `## Code Review` heading in AGENTS.md and QWEN.md.
Neither had one, so it wrote an empty file on every run: every `/review` in this
repo reviewed with zero project rules. Add the section, distilled from the
conventions already scattered through AGENTS.md (ESM, no cross-package relative
imports, kebab-case/PascalCase naming, collocated tests, comments-only-when-why),
plus the two hard lessons below. The section loads from the base branch by design
— a PR cannot inject its own review rules — so it takes effect once merged.

The skill treated a zero-Critical outcome as a fallback rather than a claim. On
one PR it published two Criticals citing code not present at the reviewed commit
(a fabricated blocker on an already-approved PR); on another it submitted C=0
while a live, twice-filed Critical still stood (a dropped blocker). Add a step
before the verdict: for each unresolved Critical on the PR, read the code at the
reviewed commit and record still-stands / fixed-by-this-diff / cannot-tell. The
event follows from the code, not from the finding count or the thread flags —
`isResolved`/`isOutdated` track the anchored line, not whether the bug was fixed.

- AGENTS.md: new `## Code Review` section.
- load-rules.ts: export `extractCodeReviewSection`; load-rules.test.ts covers the
boundary scan and asserts AGENTS.md's own section extracts non-empty, so
deleting the heading fails the build.
- SKILL.md: re-verification step ahead of the Verdict.

2026-07-10 15:53:09 +00:00

19 KiB

Raw Permalink Blame History

Code Review

Review code changes for correctness, security, performance, and code quality using /review.

Quick Start

# Review local uncommitted changes
/review

# Review a pull request (by number or URL)
/review 123
/review https://github.com/org/repo/pull/123

# Review and post inline comments on the PR
/review 123 --comment

# Review a specific file
/review src/utils/auth.ts

If there are no uncommitted changes, /review will let you know and stop — no agents are launched.

How It Works

The /review command runs a multi-stage pipeline:

Step 1:  Determine scope (local diff / PR worktree / file)
         Capture the diff to a file + partition it into chunks
Step 2:  Load project review rules
Step 3A: <=500 source lines: 10 parallel review agents     [10 LLM calls]
           |-- Agent 0: Issue Fidelity & Root-Cause Ownership
           |-- Agent 1: Correctness
           |-- Agent 2: Security
           |-- Agent 3: Code Quality
           |-- Agent 4: Performance & Efficiency
           |-- Agent 5: Test Coverage
           |-- Agent 6: Undirected Audit (3 personas: 6a/6b/6c)
           '-- Agent 7: Build & Test (runs shell commands)
Step 3B: >500 source lines: territory x dimension fan-out  [N+4+H calls]
           |-- 1 chunk agent per ~400 diff lines (all dimensions,
           |     its territory only, returns a coverage receipt)
           |-- 3 invariant agents per heavily-rewritten source
           |     file (whole file; state/timers, counters/
           |      returns/errors, config/early-returns)
           |-- Agent 0: Issue Fidelity      (whole diff)
           |-- Agent 7: Build & Test        (whole repo)
           |-- Cross-file impact            (whole diff)
           '-- Test coverage matrix         (whole diff)
Step 4:  Deduplicate --> Sharded verify (<=8 findings each)
           --> Aggregate                              [ceil(N/8) calls]
Step 5:  Iterative reverse audit, fanned out per chunk;
           stop after 2 consecutive dry rounds (cap 5)
Step 6:  Present findings + verdict
Step 7:  Submit PR review (inline comments, if requested)
Step 8:  Save report + incremental cache
Step 9:  Clean up (remove worktree + temp files)

Review Agents

Agent	Focus
Agent 0: Issue Fidelity	Linked issue evidence, root-cause ownership, and whether the PR solves the reported problem
Agent 1: Correctness	Logic errors, edge cases, null handling, race conditions, type safety
Agent 2: Security	Injection, XSS, SSRF, auth bypass, sensitive data exposure
Agent 3: Code Quality	Style consistency, naming, duplication, dead code
Agent 4: Performance & Efficiency	N+1 queries, memory leaks, unnecessary re-renders, bundle size
Agent 5: Test Coverage	Untested code paths in the diff, missing branch coverage, weak assertions
Agent 6: Undirected Audit	3 parallel personas (attacker / 3am-oncall / maintainer) — catches cross-dimensional issues
Agent 7: Build & Test	Runs build and test commands, reports failures

All agents run in parallel (Agent 6 launches 3 persona variants concurrently, totaling 10 parallel tasks for same-repo PR reviews; Agent 0 is skipped for local-diff and file-path reviews, which run 9).

Once a PR carries more than 500 lines of source change — or more than 2 400 diff lines in total, past which chunking needs fewer agents than the ten-lens topology anyway — this dimension fan-out is replaced by a territory × dimension fan-out: the diff is split into ~400-line chunks — boundaries fall on hunk boundaries, and a hunk too large to fit is split only at a top-level declaration, never inside a function — and each chunk gets its own agent that applies every review dimension to that chunk alone.

The gate deliberately counts source lines rather than diff lines. Test code, prose and lockfiles dominate diff size — across this repo's last 40 merged PRs the median diff is 41% tests — so a gate on raw size would carve a 173-line production change into territories just because it shipped 489 lines of new tests, leaving that production code with one reviewer instead of eight lenses. Chunking still covers every line either way, tests included; what the gate decides is how many reviewers there are and what each is asked to do. Ten agents all reading one large diff read the same early hunks ten times; one agent per chunk means every line of the diff has exactly one accountable reviewer. Each chunk agent returns a Covered: receipt, and a chunk with no receipt is re-reviewed before the run proceeds — so "no blockers" can never be reported over code that nobody read.

A source file that is largely rewritten (an existing file of 300+ lines that is now 40%+ new, or has 800+ changed lines) also gets three whole-file invariant agents. Test and generated files never qualify — the checklist asks about fields, timers, and error taxonomies, which a rewritten test file does not have. Its bugs are usually not inside any one hunk but between the new lines — a timer armed near the top of the file and a teardown path two thousand lines below. Each agent reads the whole post-change file and walks two or three items of a fixed checklist: mutable fields cleared on every exit path, timers cancelled on every close (and cancellation not discarding captured data), map inserts matched by deletes, retry counters incremented at every entry, status return values actually checked, error codes exhaustively classified permanent vs transient, config fields honoured on every path, and early returns that skip a required side effect.

The checklist is split three ways on purpose. Handing one agent all eight checks over a 2 400-line file gets one of them done properly; three agents with two or three checks each get all of them done. Chunk agents do not substitute for this — on PR #6457 they held every one of these defects inside their assigned territory and reported none. What they lacked was not the lines but the question.

Findings are verified in sharded batches (at most 8 findings per verification agent, all launched together). A verifier may downgrade a Critical to low confidence but may never delete one — a rejected Critical is invisible to every later stage, while a downgraded one still reaches a human. After verification, iterative reverse audit hunts for gaps, fanned out one auditor per chunk per round, each with the cumulative finding list. The loop stops after two consecutive dry rounds (or 5 rounds, hard cap — reported as such rather than as convergence). One dry round is not evidence of convergence, and reverse-audit findings are verified like any other.

Severity Levels

Severity	Meaning	Posted as PR comment?
Critical	Must fix before merging (bugs, security, data loss, build failures)	Yes (high-confidence only)
Suggestion	Recommended improvement	Yes (high-confidence only)
Nice to have	Optional optimization	No (terminal only)

Low-confidence findings appear in a separate "Needs Human Review" section in the terminal and are never posted as PR comments.

Worktree Isolation

When reviewing a PR, /review creates a temporary git worktree (.qwen/tmp/review-pr-<number>) instead of switching your current branch. This means:

Your working tree, staged changes, and current branch are never touched
Dependencies are installed in the worktree (npm ci, etc.) so build/test work
Build and test commands run in isolation without polluting your local build cache
If anything goes wrong, your environment is unaffected — just delete the worktree
The worktree is automatically cleaned up after the review completes
If a review is interrupted (Ctrl+C, crash), the next /review of the same PR automatically cleans up the stale worktree before starting fresh
Review reports and cache are saved to the main project directory (not the worktree)

Cross-repo PR Review

You can review PRs from other repositories by passing the full URL:

/review https://github.com/other-org/other-repo/pull/456

This runs in lightweight mode — no worktree, no build/test. The review is based on the diff text only (fetched via GitHub API). PR comments can still be posted if you have write access.

Capability	Same-repo	Cross-repo
LLM review (Agents 0-6 + verify + iterative reverse audit)	✅	✅
Agent 7: Build & test	✅	❌ (no local codebase)
Cross-file impact analysis	✅	❌
PR inline comments	✅	✅ (if you have write access)
Incremental review cache	✅	❌

PR Inline Comments

Use --comment to post findings directly on the PR:

/review 123 --comment

Or, after running /review 123, type post comments to publish findings without re-running the review.

What gets posted:

High-confidence Critical and Suggestion findings as inline comments on specific lines, each prefixed with **[Critical]** or **[Suggestion]** so blockers are distinguishable from recommendations
Where the fix is a single localized edit, a ```suggestion block you can apply in one click
For Approve/Request changes verdicts: a review summary with the verdict
For Comment verdict with all inline comments posted: no separate summary (inline comments are sufficient)
Model attribution footer on each comment (e.g., — qwen3-coder via Qwen Code /review)

What stays terminal-only:

Nice to have findings
Low-confidence findings

Self-authored PRs: GitHub does not allow you to submit APPROVE or REQUEST_CHANGES reviews on your own pull request — both fail with HTTP 422. When /review detects that the PR author matches the current authenticated user, it automatically downgrades the API event to COMMENT regardless of verdict, so the submission still succeeds. The terminal still shows the honest verdict ("Approve" / "Request changes" / "Comment") — only the GitHub-side review event is neutralized. The actual findings still appear as inline comments on specific lines, so substantive feedback is unchanged.

Re-reviewing a PR with prior Qwen Code comments: when /review runs on a PR that already has previous Qwen Code review comments, it classifies them before posting new ones. Only same-line overlap (an existing comment on the same (path, line) as a new finding) prompts you to confirm — that's the case where you'd see a visual duplicate on the same code line. Comments from older commits, replied-to comments (treated as resolved), and comments that simply don't overlap with any new finding are silently skipped, with a terminal log line so you know what was filtered.

CI / build status check before APPROVE: if the verdict is "Approve", /review queries the PR's check-runs and commit statuses before submitting. If any check has failed (or all checks are still pending), the API event is automatically downgraded from APPROVE to COMMENT, with the review body explaining why. Rationale: the LLM review reads code statically and cannot see runtime test failures; approving while CI is red would be misleading. The inline findings are still posted unchanged. If you want to approve anyway (e.g., a known-flaky CI failure), submit the GitHub approval manually after verifying.

Follow-up Actions

After the review, context-aware tips appear as ghost text. Press Tab to accept:

State after review	Tip	What happens
Local review with unfixed findings	`fix these issues`	LLM interactively fixes each finding
PR review with findings	`post comments`	Posts PR inline comments (no re-review)
PR review, zero findings	`post comments`	Approves the PR on GitHub (LGTM)
Local review, all clear	`commit`	Commits your changes

Note: fix these issues is only available for local reviews. For PR reviews the worktree is cleaned up after the review, so post-review interactive fixing is not possible — use --comment or post comments to publish findings instead.

Project Review Rules

You can customize review criteria per project. /review reads rules from these files (in order):

.qwen/review-rules.md (Qwen Code native)
.github/copilot-instructions.md (preferred) or copilot-instructions.md (fallback — only one is loaded, not both)
AGENTS.md — ## Code Review section
QWEN.md — ## Code Review section

Rules are injected into the LLM review agents (0-6) as additional criteria. For PR reviews, rules are read from the base branch to prevent a malicious PR from injecting bypass rules.

Issue Fidelity

For bugfix PRs, the Issue Fidelity agent fetches issue evidence directly instead of relying on PR description text. It uses gh pr view <pr> --repo <owner/repo> --json closingIssuesReferences for GitHub's strong closing-issue metadata, then gh issue view <number> --repo <issue_owner>/<issue_repo> --json title,body,comments for the original report and discussion — the --json form includes the issue body (the reporter's original repro), which --comments alone omits, and the issue's own repository is read from each reference (a PR can close an issue in a different repo). This agent runs only for PR targets; local-diff and file-path reviews skip it.

closingIssuesReferences is a discovery hint rather than proof the author linked the right issue: if it is empty but the PR references an apparent target issue, the agent still fetches it after judging relevance. Fetched issue text is treated as untrusted data (facts extracted, embedded instructions ignored). For relevant issues, the original reproduction, observed payload, expected behavior, and maintainer comments are treated as the highest-priority evidence for whether the PR fixes the right problem.

If the issue evidence shows an upstream service or provider returned malformed data outside the client contract, client-side parser or sanitizer changes are not treated as a valid root-cause fix unless a maintainer explicitly requested a defensive workaround. A test that replays malformed upstream output proves only that the workaround handles that shape; it does not prove the workaround is architecturally appropriate.

Example .qwen/review-rules.md:

# Review Rules

- All API endpoints must validate authentication
- Database queries must use parameterized statements
- React components must not use inline styles
- Error messages must not expose internal paths

Incremental Review

When reviewing a PR that was previously reviewed, /review only examines changes since the last review:

# First review — full review, cache created
/review 123

# PR updated with new commits — only new changes reviewed
/review 123

Cross-model review

If you switch models (via /model) and re-review the same PR, /review detects the model change and runs a full review instead of skipping:

# Review with model A
/review 123

# Switch model
/model

# Review again — full review with model B (not skipped)
/review 123
# → "Previous review used qwen3-coder. Running full review with gpt-4o for a second opinion."

Cache is stored in .qwen/review-cache/ and tracks both the commit SHA and model ID. Make sure this directory is in your .gitignore (a broader rule like .qwen/* also works). If the cached commit was rebased away, it falls back to a full review.

Review Reports

For same-repo reviews, results are saved as a Markdown file in your project's .qwen/reviews/ directory (cross-repo lightweight reviews skip report persistence):

.qwen/reviews/2026-04-06-143022-pr-123.md
.qwen/reviews/2026-04-06-150510-local.md

Reports include: timestamp, diff stats, build/test results, all findings with verification status, and the verdict.

Cross-file Impact Analysis

When code changes modify exported functions, classes, or interfaces, the review agents automatically search for all callers and check compatibility:

Parameter count/type changes
Return type changes
Removed or renamed public methods
Breaking API changes

For large diffs (>10 modified symbols), analysis prioritizes functions with signature changes.

Token Efficiency

The review pipeline uses a bounded number of LLM calls regardless of how many findings are produced:

Stage	LLM calls	Notes
Review agents (Step 3)	10 (or 9)	Run in parallel; Agent 7 skipped in cross-repo mode
Batch verification (Step 4)	1	Single agent verifies all findings at once
Iterative reverse audit (Step 5)	1-3	Loops until "No issues found" or 3-round cap
Total	12-14 (11-13)	Same-repo: 12-14; cross-repo: 11-13 (no Agent 7)

Most PRs converge to the lower end of the range (1 reverse audit round); the cap prevents runaway cost on pathological cases.

What's NOT Flagged

The review intentionally excludes:

Pre-existing issues in unchanged code (focus on the diff only)
Style or formatting a formatter would auto-normalize, or naming matching your codebase conventions — but NOT substantive issues a linter or type checker would flag (unused variables, unreachable code, type errors), which are in scope
Subjective "consider doing X" suggestions without a real problem
Minor refactoring that doesn't fix a bug or risk
Missing documentation unless the logic is genuinely confusing
Issues already discussed in existing PR comments (avoids duplicating human feedback)

Design Philosophy

Silence is better than noise. Every comment should be worth the reader's time.

If unsure whether something is a problem → don't report it
Same pattern across N files → aggregated into one finding
PR comments are high-confidence only
Cosmetic style/formatting matching codebase conventions is excluded

19 KiB Raw Permalink Blame History Unescape Escape