* 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.
Suggestion-level findings were routed to a single updatable issue comment
(the "suggestion summary") while only Critical findings became inline review
comments. That split traded away two things that turned out to matter more
than the convergence it bought:
- An issue comment has no lifecycle. GitHub folds an inline review thread away
as Outdated once the author edits the line it is anchored to, so an addressed
finding removes itself from the page. The summary comment just sits in the PR
conversation forever; PATCHing it to "all addressed" replaces its content but
not the comment. The mechanism meant to prevent clutter was the clutter.
- A Markdown table cannot carry a one-click fix. GitHub renders a ```suggestion
fence as an applicable change only inside a review comment on a diff line.
Suggestion findings are exactly the mechanical, localized cleanups that
benefit most from one-click apply, so the split withheld the feature from the
findings that needed it most.
Both severities now post as inline comments, distinguished by a **[Critical]**
or **[Suggestion]** body prefix. The `qwen review post-suggestions` subcommand
and its plumbing are removed.
Follow-on changes required by the reroute:
- pr-context: the "Previous suggestion summary" section is gone. Legacy summary
comments are still recognised so they stay out of "Already discussed", but the
exclusion is now marker-only rather than author-gated. The author check missed
summaries posted by the *other* identity: /review runs as a maintainer locally
and as qwen-code-ci-bot in CI, and roughly half of the last 60 PRs carry a
bot-authored summary. Those leaked into "Already discussed" and told the review
agents not to re-report the findings listed there. The check originally guarded
promotion into a trusted rendering section; that section no longer exists, so
it only gated exclusion, where a third party embedding the marker merely hides
their own comment.
- qwen-autofix: the workflow filters "suggestion summaries" out of the autofix
bot's actionable queue, but only on the issue-comment channel. With Suggestions
now inline, they entered the unfiltered inline channel and the bot would apply
non-blocking recommendations and spend a review round on them. The inline
channel now applies the same gate, keyed on the **[Suggestion]** prefix plus
the /review footer so a human quoting the prefix stays actionable.
- Step 7 gains a 422 fallback. Create Review is all-or-nothing, so one Suggestion
anchored outside the diff would take the Critical findings down with it — a risk
that did not exist when Suggestions travelled on a line-agnostic issue comment.
GitHub's 422 does not name the offending entry, so the model rechecks anchors
against the diff, relocates failing Criticals into the body, discards failing
Suggestions, and degrades to an all-prose review rather than posting nothing.
COMMENT reviews now always carry a one-line body: an empty body is only known
to be accepted alongside inline comments on REQUEST_CHANGES, and a Suggestion-
only review is the common case for a clean PR.
The bundled /review skill is a general command that runs against arbitrary
repositories (and cross-repo PRs), but a previous change baked qwen-code's own
"core infrastructure is maintainer-only" governance into the shipped prompt:
hardcoded packages/core and packages/*/src/{auth,providers,models,config,tools,services}
paths, a 500+ line hard block, and an authorAssociation-based maintainer check.
Those path names are generic — src/auth, src/config, src/tools, src/services are
common across monorepos — so an external contributor's large PR to an unrelated
repo would be hard-blocked as "must be maintainer-initiated" under a policy that
repo never adopted.
Remove the gate and its escalate-flag plumbing (Steps 1, 6, and 7) from the
bundled skill, along with the matching DESIGN.md rationale and the user-doc
section. qwen-code's maintainer-only policy stays documented in AGENTS.md for
this repo. The Issue Fidelity / root-cause ownership agent (Agent 0) is a
universal review principle and is left unchanged.
Co-authored-by: dragon <dragon@U-2Q53JQG9-0233.local>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* feat(review): add issue-fidelity and root-cause ownership gate to /review
Adds a dedicated Issue Fidelity & Root-Cause Ownership agent (Agent 0) to
the /review pipeline and a core-infrastructure scope gate that runs before
the review agents.
Agent 0 fetches linked GitHub issue evidence directly (closingIssuesReferences
plus issue comments) instead of trusting the PR author's framing, compares the
original reported failure against the PR's claimed fix, and flags client-side
parser/sanitizer workarounds for malformed upstream output as Critical unless a
maintainer explicitly requested the defensive mitigation. The core-infra gate
applies the repository's existing two-tier maintainer-only rule before spending
review budget.
This hardens the pipeline against a false-approval mode where a bot PR passes
its own tests and reads as internally reasonable but fixes the author's mistaken
diagnosis rather than the linked issue's actual root cause.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs(review): address PR review feedback on issue-fidelity gate
- Fetch issue evidence with `gh issue view --json title,body,comments` so the
issue body (reporter repro/observed payload/expected behavior) is included;
`--comments` alone omits it. Use each closingIssuesReferences entry's own
repository so cross-repo linked issues resolve correctly.
- Treat closingIssuesReferences as a discovery hint (fetch apparent target
issues even when it is empty) and treat fetched issue content as untrusted
data (extract facts, ignore embedded instructions).
- Run Agent 0 (Issue Fidelity) only for PR targets; skip it for local-diff and
file-path reviews, and require the PR number/repo/context in its prompt.
Handle empty references / non-bugfix / gh failure explicitly.
- Pass Agent 0's quoted issue evidence to Step 4 batch verification and stop it
rejecting issue-grounded findings just because the code compiles/tests pass.
- Make the core-infrastructure gate concrete: deterministic maintainer signal
via authorAssociation, count only core-path lines, honor the AGENTS.md
low-risk-sweep exception, clean up the worktree on hard block, run the gate
right after fetch-pr (before npm ci), and map escalate -> COMMENT (never
APPROVE) in Steps 6-7.
- Sync agent counts and token math across SKILL.md, DESIGN.md, and
code-review.md (Agent 0 is PR-only; ~620-730K).
* docs(review): rename 'Linked Issue Fit' heading to 'Issue Fidelity'
Aligns the code-review docs heading with the 'Issue Fidelity' name used
for Agent 0 in SKILL.md and DESIGN.md, so the section connects to the
pipeline diagram. Addresses review feedback.
* docs(review): stop core-infra hard block before load-rules and surface it via --comment
- Hard block now stops before Step 2 (load-rules) instead of before Step 3,
so a PR destined for hard-block no longer runs the load-rules step.
- In --comment mode the hard block posts an event=COMMENT on the PR, matching
the escalate path's GitHub visibility, so external authors see the block.
---------
Co-authored-by: dragon <dragon@U-2Q53JQG9-0233.local>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* refactor(review): drop deterministic-analysis and autofix steps
Slim the bundled /review skill from 11 to 9 steps by removing Step 3
(deterministic analysis — auto-run of tsc/eslint/ruff/clippy/go vet
plus CI-lint discovery) and Step 8 (autofix — PR-worktree auto-fix,
commit and push).
Renumber the remaining steps (4→3 … 11→9) and update every
cross-reference. Agent 7 (Build & Test) stays in the parallel review
step and now always runs build+test instead of skipping when Step 3
had already compiled. The [linter]/[typecheck] source tags are dropped;
[build]/[test]/[review] remain. DESIGN.md is updated to match (step
numbers, removed Autofix/deterministic rationale, LLM-budget table).
* refactor(review): address review feedback on the /review slimming
Follow-up to the 11->9 step change, addressing PR review comments:
- Restore base-branch CI-config protection in Agent 7. The removed Step 3 carried the instruction to read CI config from the base branch; without it Agent 7 would discover build/test commands from the untrusted PR branch. Re-added to Agent 7's CI-config discovery clause.
- Drop the stale "linters" token from the worktree rule (no standalone linter step runs anymore).
- Narrow the exclusion criteria: substantive lint/type issues (unused vars, unreachable code, type errors) are no longer auto-excluded now that no deterministic tool catches them; only pure formatting stays excluded.
- Update user docs to match: docs/users/features/code-review.md (11->9 steps, remove the Deterministic Analysis and Autofix sections, drop the two comparison-table rows, renumber the Token-efficiency table) and docs/users/features/commands.md (agent count).
- Fix stale step-number references in CLI comments: cleanup.ts (Step 11->9) and presubmit.ts (Step 9->7, comment + yargs describe).
* refactor(review): remove orphaned deterministic subcommand, polish docs
Second round of PR feedback:
- Remove the now-orphaned `qwen review deterministic` subcommand. review.ts describes these subcommands as "internal helpers used by the /review skill"; with Step 3 gone the skill no longer invokes it, so the ~740-line module plus its import / registration / describe / subcommand-list entries were dead code. Deleted deterministic.ts and its wiring in review.ts.
- Decouple the exclusion criterion from pipeline state (SKILL.md): substantive lint/type issues (unused vars, unreachable code, type errors) are now "in scope — LLM agents should report them" rather than "no longer have a deterministic tool catching them", so the rule stays correct if a linter step is ever re-added.
- Drop the stale "linting" justification from the worktree dependency-install note (SKILL.md); only build/test remain.
- DESIGN.md: rename the subcommands section to "presubmit and cleanup", drop "lint" from the review-tools rejected alternative, and fix the "we already have those" cell to "We retain build/test (Agent 7)".
* refactor(review): resolve exclusion-criteria contradiction, polish wording
Third round of PR feedback:
- Merge the exclusion criteria to remove the contradiction between the unconditional "matches codebase conventions" exclude and the "substantive lint/type issues are in scope" include. Now a single bullet: cosmetic style/formatting/naming is excluded, but substantive issues a linter or type checker would flag (unused variables, unreachable code, type errors) are in scope even where the surrounding code tolerates them. Kept decoupled from pipeline state (no "deterministic tool" wording). Applied in both SKILL.md and docs/users/features/code-review.md.
- SKILL.md lightweight-mode skip: "(no local reports or cache)" to match Step 8's title ("Save review report and cache").
- DESIGN.md: rename the CI-config section to "auto-discover build/test commands" to match the body, which was narrowed to build/test only.
* test(review): guard qwen review subcommand surface, fix stale docs linting ref
- Add packages/cli/src/commands/review.test.ts verifying the `qwen review` builder registers exactly [fetch-pr, pr-context, load-rules, presubmit, cleanup], no longer registers the removed `deterministic` subcommand, and that `describe` no longer mentions deterministic analysis. Guards against silently re-adding the subcommand or dropping a helper (review.ts previously had no test).
- docs/users/features/code-review.md: drop the orphaned "linting" from the Worktree Isolation dependency-install note; only build/test remain now that Step 3 is gone.
* feat(cli): route foreground subagents through pill+dialog while running
Foreground (synchronous) subagents currently render a live AgentExecutionDisplay
inside the parent's pendingHistoryItems block. The frame mutates on every tool
call and approval; once it grows past the terminal height (verbose mode, parallel
subagents, long tool-call lists) the live-area repaint flickers visibly.
This change extends BackgroundTaskRegistry with a flavor: 'foreground' | 'background'
discriminator. Foreground entries register at the start of the synchronous tool-call
and unregister in its finally path. The pill counts them; the dialog drills into
their activity. The inline frame is suppressed during the live phase — only an
active, focus-locked approval prompt renders, as a small banner labeled with the
originating agent. Once the parent turn commits, the full AgentExecutionDisplay
appears in scrollback via Ink's <Static>, exactly as before.
Foreground entries skip the XML task-notification (the parent receives the result
through the normal tool-result channel) and skip the headless holdback (the
parent's await already pins the loop). The dialog gates per-agent cancellation
behind a two-step confirm so a stray 'x' can't end the user's current turn.
* fix(cli): address review findings on foreground subagent routing
- Gate `registerCallback` on background flavor so foreground entries
don't leak orphaned `task_started` SDK events without a matching
terminal notification.
- Render a queued-approval marker for non-focus subagents instead of
returning null, so a queued approval is visible in the main view.
- Move `emitStatusChange` before `agents.delete` in
`unregisterForeground` to match the ordering used by
complete/fail/cancel/finalize.
- Prefix the foreground tool result with a cancel marker when
`terminateMode === CANCELLED`, so the parent model can distinguish
a user-cancelled run from a successful completion.
- Mirror the background path's stats wiring on the foreground path
so `entry.stats` stays current and the dialog detail subtitle
shows tool count + tokens for foreground runs.
- Remove the unreachable `isWaitingForOtherApproval` branch (subsumed
by the queued-approval marker above).
- Reset the foreground confirm-step on detail-mode `left` and ignore
`x` on terminal entries so an armed cancel can't carry into list
mode and the hint footer/handler stay in sync.
- Test factory uses a `baseProps` spread instead of `as` cast so a
future required field on `ToolMessageProps` is a compile-time miss.
* feat(review): expand review pipeline + add `qwen review` CLI subcommands
Review skill (SKILL.md) changes:
- Step 4: 5 → 9 parallel agents (split Correctness/Security, add Test
Coverage, 3 undirected personas: attacker / 3am-oncall / maintainer)
- Step 5: verification "uncertain → reject" → "uncertain → low-confidence"
(terminal-only "Needs Human Review" bucket; never posted as PR comments)
- Step 6: single reverse audit → iterative (terminate on no-new-findings,
hard cap 3 rounds)
- Step 9: self-PR detection (downgrade APPROVE/REQUEST_CHANGES → COMMENT
when GitHub forbids self-review with HTTP 422); CI status check
(downgrade APPROVE → COMMENT on red/pending CI); existing-Qwen-comment
classification with priority order Stale > Resolved > Overlap > NoConflict
(only Overlap blocks for confirmation)
`qwen review` CLI subcommands (packages/cli/src/commands/review/):
- fetch-pr — clean stale + fetch PR ref + create worktree + metadata
- pr-context — emit Markdown context file with security preamble +
already-discussed dedup section
- load-rules — read review rules from base branch (4 source files)
- deterministic— run tsc, eslint, ruff, cargo-clippy, go-vet, golangci-lint
on changed files; filtered + structured findings JSON
(TypeScript/JavaScript, Python, Rust, Go)
- presubmit — self-PR + CI status + existing-comment classification in
a single JSON report
- cleanup — worktree + branch ref + per-target temp files (idempotent)
Cross-platform: execFileSync (no shell), path.join, CRLF normalization,
which/where for tool detection. Replaces bash-style inline commands in
SKILL.md; works identically on macOS/Linux/Windows.
Path consistency: SKILL.md temp files moved from /tmp/qwen-review-* to
.qwen/tmp/qwen-review-* — matches what os.tmpdir() resolves to across
platforms (macOS returns /var/folders/... not /tmp).
DESIGN.md gains five "Why ..." sections explaining each design decision;
docs/users/features/code-review.md synced for user-visible changes.
* feat(review): expose full reply chains in pr-context output
`qwen review pr-context` now renders each replied-to inline-comment thread
as the original reviewer comment + chronological reply chain, instead of
only listing the root-comment snippet. This lets review agents see at a
glance whether a topic has been addressed (e.g. a "Fixed in <commit>"
reply closes the thread) and avoids re-reporting already-resolved
concerns without forcing the LLM driver to manually summarise each reply
chain in agent prompts.
- Walk `in_reply_to_id` chain to group replies under their root comment
- Sort replies chronologically (by id, monotonic on GitHub)
- Render thread block: root snippet as a quote + bulleted reply list
- Sort threads by `(path, line)` for deterministic output
- SKILL.md note updated to point agents at the new chain format
* feat(review): include review-level summaries in pr-context output
`qwen review pr-context` now also fetches `gh api repos/{owner}/{repo}/pulls/{n}/reviews`
and renders a "Review summaries" section listing each reviewer's
overall body (the comment they typed alongside an APPROVED /
CHANGES_REQUESTED / COMMENTED submission). Closes a real gap found
during the PR #3684 review:
> "@wenshao [CHANGES_REQUESTED]: The previously identified exported
> type rename issue no longer maps to the current PR diff, so this
> review only includes the remaining high-confidence blocker."
Without this section, the LLM driver's review agents would have missed
that integration note from the prior reviewer.
- New `RawReview` type + extra `ghApi` call
- Filter: skip empty bodies + the canonical "No issues found. LGTM!"
template the qwen-review pipeline auto-emits — those carry no
agent-actionable content beyond the review state itself
- Sort meaningful reviews by `submitted_at` for chronological output
- Stdout summary now reports `M/N review summaries` (M = kept after
filter)
Smoke-tested on PR #3684: 30 inline, 3 issue, 1/30 review summaries
correctly surfaces the @wenshao CHANGES_REQUESTED body and filters the
29 LGTM templates.
* fix(review): paginate gh API calls to capture comments past page 1
`gh api <path>` defaults to per_page=30. Busy PRs cross that limit on
inline comments, issue comments, and reviews — the latest entries (the
ones most likely to contain new reviewer feedback or in-flight reply
chains) end up on page 2+ and were silently truncated.
Concrete bug found while re-reviewing PR #3684:
Before: `30 inline, 3 issue comments, 1/30 review summaries`
After: `97 inline, 3 issue comments, 6/67 review summaries`
5 additional reviewer-level summaries surfaced — including the
@wenshao 2026-04-30 "Multi-agent re-review (Phase C)" body with the
explicit verification notes that this PR's pipeline is supposed to
chain forward into the next review.
Changes:
- `lib/gh.ts`: new `ghApiAll(path)` helper using `gh api --paginate`,
which walks every `next` link and concatenates each page's array.
- `pr-context.ts`: 3 fetches (inline / issue / reviews) → `ghApiAll`.
- `presubmit.ts`: PR comments fetch → `ghApiAll` too (existing-comment
classification was equally susceptible to dropping page 2+ overlap
candidates).
`check-runs` and `commits/<sha>/status` calls retain `ghApi` — those
return objects (with embedded arrays) and rarely cross 30 entries.
---------
Co-authored-by: wenshao <wenshao@U-K7F6PQY3-2157.local>
User doc and PR description now include the "PR review, zero findings
→ post comments → approve PR" row in the follow-up actions table.
Also fixed PR description: "Step 4" → "Step 9" for post comments.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cross-repo lightweight mode has no local codebase — Agent 5 (build/test)
is pointless. Now launches 4 agents instead of 5 in cross-repo mode.
Updated token count tables in SKILL.md, user doc, and DESIGN.md:
same-repo = 7 LLM calls, cross-repo = 6.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- User doc: added "Other" row to language table + explanation that
CI config is read for unrecognized projects
- DESIGN.md: added "Why auto-discover from CI config" decision
section + added .qwen/review-tools.md to rejected alternatives
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- git branch -D: add 2>/dev/null || true to both cleanup sites
(Step 1 stale cleanup + Step 11) to prevent abort if ref missing
- Cross-repo doc: clarify Agents 1-4 only (Agent 5 build/test
requires local codebase, not available in cross-repo mode)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SKILL.md:
- Step 9 must use owner/repo from URL (not gh repo view) for cross-repo
- Step 2 (project rules) skipped in cross-repo mode (no local files)
User doc: add Cross-repo PR Review section with same-repo vs cross-repo
capability comparison table.
DESIGN.md: add "Why cross-repo uses lightweight mode" section explaining
CLI tools are inherently repo-local and our approach is best available.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reverse audit agent already has full context (all confirmed findings +
entire diff), so its findings don't need a second opinion. This brings
the actual LLM call count to 7 (5 review + 1 verify + 1 reverse),
matching the documented claim.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
If a previous review was interrupted (Ctrl+C, crash), stale worktree
and local ref would block the next review. Now Step 1 checks for and
cleans up stale .qwen/tmp/review-pr-<N> worktree and qwen-review/pr-<N>
ref before creating new ones.
Step 5 also cleans up the local ref alongside the worktree.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Mermaid only renders on GitHub; shows as raw code on Nextra,
Docusaurus, VS Code preview, and offline viewing. Plain-text
ASCII diagram is universally compatible and includes LLM call
cost annotations on each stage.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Step 4.5: use absolute paths for reports/cache in worktree mode
(relative paths would land in worktree and be deleted)
- Step 1: fetch into qwen-review/pr-<N> ref to avoid clobbering
existing local branches
- Step 2.6: reverse audit findings use batch verification (not
one-per-finding), consistent with Step 2.5
- Doc: clarify reverse audit findings are also batch-verified
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add Token Efficiency section showing fixed 7 LLM calls breakdown
- Fix follow-up table: "fix these issues" is local-only (worktree
cleaned up after PR review)
- Update PR description with worktree, batch verification, cross-model
review, PR comment dedup, and expanded test plan
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previously, each finding got its own independent verification agent
(N findings = N LLM calls). Now a single verification agent receives
all findings at once and verifies them in one pass.
Token cost: 6+N variable calls → 7 fixed calls (5 review + 1 verify + 1 reverse audit)
Quality: minimal impact — batch verification has fuller cross-finding context
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add model attribution to no-findings LGTM path
- Handle empty string from getModel() with .trim() || 'unknown'
- Add tests for {{model}} with args and empty model ID
- Fix doc contradiction: PR autofix pushes automatically from worktree
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. Remove gh pr checkout --detach (modifies working tree, defeats
worktree purpose). Use git fetch only.
2. Add dependency installation step (npm ci etc.) in worktree —
without it, all TS/JS linting/building fails.
3. Cache and reports written to main project dir, not worktree
(would be deleted in Step 5).
4. "fix these issues" tip only for local reviews — worktree is
cleaned up after PR review, so interactive fixing not possible.
5. Autofix push uses explicit remote branch name from Step 1.
6. Move incremental check before dependency install to avoid
wasting time when no new changes.
7. Fix Step 3 reference: "from Steps 2.5 and 2.6" (includes
reverse audit findings).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the stash + checkout + restore flow with an isolated git
worktree for PR reviews. This eliminates:
- Stash orphan risks (multiple early exit paths)
- Wrong-branch risks (Step 5 restore failures)
- Build cache pollution (worktree has its own state)
- All stash-related error handling complexity
New flow:
- Step 1: git worktree add .qwen/tmp/review-pr-<number>
- All agents operate in the worktree directory
- Autofix commits and pushes from the worktree
- Step 5: git worktree remove (--force for dirty worktrees)
User's working tree is never modified during PR reviews.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
For PR reviews, fetch existing inline and general comments via gh api
before launching agents. A summary of already-discussed issues is
passed to agents so they don't re-report problems that humans or other
tools have already flagged.
Added to Exclusion Criteria: "Issues already discussed in existing
PR comments."
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The incremental review cache now stores modelId alongside commitSha.
When the same PR is re-reviewed with a different model:
- Cache detects model change → runs full review (not skipped)
- Informs user: "Previous review used X. Running full review with Y
for a second opinion."
Same SHA + same model still skips as before.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add Step 2.6: after all findings are verified and aggregated, a single
reverse audit agent reviews the diff with full knowledge of what was
already found, specifically looking for important issues that all
previous agents missed.
- Only reports Critical/Suggestion level gaps (not Nice to have)
- Findings go through the same verification as other agents
- Single agent call — minimal cost overhead
- If nothing is found, initial review had strong coverage
This formalizes the "multi-round undirected audit" pattern that proved
effective during the development of this PR (14 rounds, 40+ issues).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add comprehensive user documentation for the /review command covering:
- Quick start examples for all modes (local, PR, file, --comment)
- Pipeline overview with all steps explained
- Review agents table (5 agents + their focus areas)
- Deterministic analysis (supported languages and tools)
- Severity levels and PR comment filtering rules
- Autofix workflow
- PR inline comments (what gets posted vs terminal-only)
- Follow-up actions (fix/post comments/commit)
- Project review rules (.qwen/review-rules.md etc.)
- Incremental review and caching
- Review report persistence
- Cross-file impact analysis
- Design philosophy
Also add /review and /simplify to the commands reference page
under a new "Built-in Skills" section with link to full docs.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
