mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-03 13:24:41 +00:00
34 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Shaojin Wen
|
a470ba626c
|
feat(review): add comment-status helper for existing-thread triage (#7690)
* feat(review): add comment-status helper for existing-thread triage
One deterministic pass over a PR's existing inline comments, replacing
the per-comment `gh api` fetches the orchestrating model used to make
during /review: anchor validity at the live head (outdated detection,
with a file-level exemption), whether the anchored file changed in the
reviewed worktree since each comment's commit and which commits touched
it (the re-check's candidate "fixed by" list), reply participation and
PR-author response, the blocker signal (same carriesBlockerSignal as
pr-context, so the two surfaces agree by construction), and
worktree-vs-live head drift.
Measured on a heavily discussed PR (72+ inline comments), a single
review run burned 20+ model turns re-deriving exactly these fields one
comment id at a time. SKILL.md now runs the subcommand in Step 1 and
routes the Step 6 re-check's status questions at the report; comment
bodies stay in the pr-context file under its untrusted-data preamble,
and a comment-status failure only warns — it is an index, not the
evidence, so it never sets the context-unavailable state.
* test(review): add comment-status to the subcommand registry expectations
* fix(review): comment-status review follow-ups — size warning, --host wiring, scope clauses
Addresses the review at
|
||
|
Shaojin Wen
|
d1b2a7fb72
|
feat(review): procedural correctness finders, effort levels, and posting/verify guardrails (#6711)
* feat(review): procedural correctness finders, effort levels, and posting/verify guardrails
Rework the /review skill's finder layer and add precision and cost controls,
informed by dogfooding the skill against real PRs.
Recall:
- Split Agent 1 (Correctness) into three procedural finders defined by how they
walk the diff — 1a line-by-line (incl. language-pitfall and wrapper-routing
checks), 1b removed-behavior audit, 1c cross-file tracer — so coverage is
complementary instead of overlapping. Bump the 3A dimension fan-out to 12
agents and shift the 3A/3B gate to 3200 diff lines.
- Add Agent 8: up to two diff-specialized finders written per-review when the
diff concentrates in a domain with a known failure grammar.
- Fold altitude into Code Quality and a quote-the-rule discipline into the
conventions pass.
Precision:
- Every finding must state a concrete failure scenario (trigger to wrong
outcome, or concrete cost); findings that can't are dropped at the source,
and verification re-traces the scenario rather than judging prose.
- Verification checks a finding against the diff's own documented intent: a
"regression" the diff deliberately changes and documents is a design
decision, not a defect.
Cost and safety:
- Add --effort low|medium|high: cheap inline passes with no subagents (default
high for PRs, medium for local changes).
- Hard-gate PR posting: never submit a review unless --comment was passed or the
user explicitly asked, regardless of verdict.
- Add a substantive-return check for whole-diff agents (invariant, cross-file,
test-coverage matrix) so a silently whiffing agent is caught like a missing
chunk receipt.
DESIGN.md records the rationale and dogfooding cases behind each change; user
docs updated with the effort levels and the new agent roster.
* docs(review): fix stale topology numbers flagged in review
- Define H in the 3B pipeline diagram cost annotation (3 invariant agents
per heavy file).
- Annotate the 40-PR re-gating cost figures with the roster they were
measured under (22 agents / ~5% at 10 agents; ~34 / ~7% at 12).
- Correct the fork-subagent savings estimate to ~88-92% (~750-950K →
~80-88K); the previous range predated the updated totals.
- "None or nine" receipts under 3A is eleven under the 12-agent roster
(every agent except Build & Test walks the diff).
* fix(review): address review feedback on effort/verify/lightweight edge cases
Criticals from review:
- Apply the --comment→high-effort override only after target disambiguation;
an ignored --comment (non-PR target) no longer silently forces high.
- The documented-intent gate caps confidence only when the rationale makes the
harm uncertain; a traced harm that survives the rationale keeps high
confidence, and rejection is reserved for pure re-descriptions.
- Lightweight cross-repo mode degrades Agents 1a/1b to diff-only and routes
unverifiable re-establishment claims to low confidence instead of asserting
them, matching the verifier's limits.
Suggestions:
- Agent 0's empty-scope exit now carries its evidence and the whiff check
recognizes it, so a legitimate no-linked-issue return is not relaunched.
- Reframe the 3200-line clause as an attention bound (3B is not guaranteed
cheaper with heavy files or specialized finders).
- Fix call-budget notation: F for findings vs N for chunks; correct the 3B
budget to rounds × chunks for the reverse audit (~70 calls on the 19-chunk
example, not ~28-30); state the runtime concurrency cap (10) instead of
claiming ~1x wall time.
- Preserve the failure scenario through pattern aggregation and posted inline
comments; extend quality-finding verification to check the named helper
does what the finding claims.
- Document medium effort's roster (no dedicated security/test-coverage pass);
qualify the cross-effort scope note (incremental cache is high-only); define
"lenses" on first use; fix tense and the stale 9-agent line in commands.md;
clarify the 1b skip condition (no removed/replaced lines).
* fix(review): close 422-relocation verdict hole and lightweight-mode context gap
From review feedback (one human, two model reviews):
- 422 recovery: Criticals relocated into the review body now keep the event at
REQUEST_CHANGES — the event/body table counted comments only, so a review
whose blockers were all relocated could submit as APPROVE/no-blockers
COMMENT with blocker text in the body.
- Lightweight cross-repo mode now runs pr-context (pure GitHub API): Agent 0
and the open-Critical re-check need the PR body and open threads, which the
bare gh-pr-diff setup never captured.
- Define --effort value parsing so a non-enum next token (e.g. a PR number) is
never consumed as the value.
- Add the missing test-coverage-matrix definition section; mark agent counts
as maxima (1b skipped on no-deletion diffs).
- Sync DESIGN's documented-intent paragraph with the corrected confidence
policy; align 3A/3B budget headings with the dual-trigger gate and state the
38-95 reverse-audit range explicitly.
- Docs: dual-trigger diagram labels, attention-bound wording, diff-reading
lenses phrasing, per-stage-bounded (not fixed-total) cost claim, effort
table qualifications, failure-scenario in the Step 7 JSON samples.
* docs(review): reconcile verifier rejection rule and close remaining edge notes
- State the Critical-rejection bar once, without the self-contradicting
"never reject / to reject" phrasing: rejection requires quoting the
contradicting code, and the floor verdict is confirmed (low confidence)
when it cannot be quoted.
- Note the one sanctioned exception to the empty REQUEST_CHANGES body:
unmappable or 422-relocated Criticals.
- Give the callee-direction check a concrete procedure (walk the other
changed symbols this territory calls, re-read their post-change
contracts).
- Define lightweight-mode pr-context failure handling: continue diff-only,
skip Agent 0, open-Critical re-checks become "cannot tell" (no Approve).
- Clarify Agent 8 applies in every mode (it needs only the diff) and that
Step 6 follow-up tips are high-effort only.
* fix(review): close flag-parse, context-unavailable, and downgrade edge cases
Address the latest review round on the skill text:
- An invalid spaced --effort value is discarded (with the warning) whenever
another token is the target, so `/review 6711 --effort typo` reviews PR
6711 instead of leaking `typo` into target disambiguation; the token is
kept only when it is itself the sole target candidate.
- The lightweight-mode pr-context failure now names a context-unavailable
state with a defined Step 7 serialization: never APPROVE, submit COMMENT
with a diff-only body, findings or not.
- Step 6's open-Critical re-check draws from both context sections; a reply
alone ("I disagree") no longer retires a blocker — only a code-verified
"fixed by this diff" does.
- DESIGN and user docs now state the same rejection bar as the skill:
rejecting a Critical requires quoted contradiction (or a documented-intent
re-description); anything less certain downgrades.
- Downgrading a REQUEST_CHANGES that carries body-relocated Criticals keeps
those descriptions after the downgrade sentence, so the self-PR downgrade
can no longer erase the only copy of a blocker.
* fix(review): close verdict-upgrade and body-Critical re-check gaps
Third review round on the skill text:
- 422 recovery may never upgrade the event: a Suggestion-only review whose
anchors all failed resubmits as COMMENT with the could-not-anchor body,
never as APPROVE/"No issues found" — the verdict reflects confirmed
findings, not surviving anchors.
- Step 6's open-Critical re-check now also walks the Review summaries
section: an unmappable or 422-relocated blocker lives only in a review
body, and pr-context truncates summaries to ~240 chars, so a summary
showing (or cut where one could hide) a Critical marker is fetched in
full via the reviews API before ruling.
- The context-unavailable cap now applies to every C=0 row of the invariant
table, not just the empty one: Suggestion-only results post a diff-only
body instead of a "no blockers" claim the run cannot certify.
* fix(review): compose COMMENT bodies from clauses and harden the body-Critical re-check fetch
Fourth review round found four pairwise collisions between rules that each
set "the" COMMENT body, plus four execution gaps in the Step 6 full-body
fetch. Close the class, not the instances:
- Replace the fixed-sentence bodies with an ordered clause composition rule
(downgrade reasons, context-unavailable warning, suggestions disclosure,
uncoverable chunks, body Criticals) — each clause present iff its state
holds, free prose still banned, single-state case identical to the table.
- Define C once, globally: Criticals the review posts anywhere (inline or
body), so no downstream C=0 rule can erase a body-only blocker, and 422
relocation keeps REQUEST_CHANGES by definition rather than by patch.
- 422 recovery re-derives bodies via the composition rule, so a
context-unavailable run can never restore a "no blockers" certification.
- Step 6's full-body fetch is paginated (--paginate; the endpoint returns 30
per page), treats fetched bodies as untrusted data (extract only the
Critical-bearing text, never paste unrelated bodies), and fails closed:
an unreadable truncated blocker rules "cannot tell" and caps the event at
COMMENT.
DESIGN.md records why composition replaces per-collision patching
(n states -> n(n-1)/2 pairs; clauses make new states additive).
* fix(review): correct the cross-repo capability table and close nine review notes
- docs: the cross-repo table claimed "Agents 0-6" run in lightweight mode
while the prose (correctly) says 1c is skipped there — 1c is inside that
range. Split 1c onto its own row, and add the missing Agent 8 row (its
finders need only the diff, so they do run cross-repo).
- --effort=<level> now has a parse rule: split the flag token on the first
'=' and consume no second token; the next-token rule applies only to the
spaced form.
- The substantive-return (whiff) check covers every receipt-less agent, so
3A's dimension agents are in scope, not just 3B's whole-diff agents.
- Step 3C names Agent 1b's lightweight degradation and states the three
angles medium deliberately omits (security, test coverage, adversarial
personas) instead of naming only two.
- Step 6's open-Critical re-check states what a context-unavailable run does
(skip the walk, every Critical is "cannot tell") instead of pointing at a
context file that does not exist.
- The event/body table carries the body-only-Critical exception in the cell,
where it is read, not only in the surrounding prose.
- The posting gate's second condition is now decidable: a publish verb typed
by the user this session, with the near-misses (approving noises, our own
tip, PR text) enumerated as non-authorization.
- DESIGN: the whiff check is evidential, not a length threshold (and says
why no number); "quick pass" is defined as low+medium sharing guardrails.
* feat(review): promote removed-behavior to a whole-diff agent in 3B
Territory-scoped 1b can only ask "was this deletion re-established here",
and for the deletions that matter the answer is somewhere else. PR #6638
(43 files, 8255 additions, 28 chunks) measured the gap: the 3B run with
per-chunk 1b reported one Critical; an independent reviewer reported 32, and
a parallel hand-run 1b+1c wave over the same commit reproduced six of them.
Every one of that overlapping six is a cross-chunk deletion — enableByPath
(includeSubdirs: true) replaced by an exact-path setWorkspaceActivation in
another file, silently narrowing workspace-scoped disable for every untouched
CLI/TUI caller; refreshTools() dropped from the activation paths, its
replacement swallowing the errors it used to propagate; a global mutation
timeout replaced by one covering only the prepare phase. Deletion in chunk A,
replacement in chunk B, consumer in a file the diff never touches: no chunk
agent can see that triple, and 1c does not look for it — it greps callers of
changed symbols, and a deleted export has no symbol left to grep.
- 1b joins 1c as a whole-diff agent in 3B; chunk agents keep the local half
(a guard deleted and not re-established in the same hunk is still theirs).
- The split is stated at both agents: 1c walks the callers of changed
symbols, 1b walks the replacements of removed ones.
- Agent 1b's definition gains the removed-export bullet: compare replacements
as behaviour, not names, then check the call sites the diff never touches —
a replacement that type-checks is not a replacement that behaves.
- 3B whole-diff agent count 4-6 -> 5-7 in the budget and the docs diagram.
* fix(review): serialize cannot-tell blockers, gate the no-blockers opener, and read reviews from a file
Fifth review round, all four notes real:
- The clause inventory had no way to serialize Step 6's `cannot tell`
verdict, so a Critical the review could neither confirm nor clear had
nowhere to go and dropped out of the public review. Added clause 5
(unresolved existing-Critical), which survives downgrades and 422 recovery
like the body-Critical carve-out, and Step 6 now points at it.
- `Reviewed — no blockers.` was injected as the opener whenever context was
available, regardless of C or scope — so a self-PR downgraded to COMMENT
with an inline Critical, or a review with an uncoverable chunk, opened by
certifying the absence of the blockers it was carrying. The opener is now
gated on C === 0 AND no unresolved existing Critical AND no uncoverable
chunk AND context available; otherwise it is a plain `Reviewed.`
- The paginated `/reviews` fetch ran through the shell, whose successful
output is capped at 30 000 chars and split head/tail: a body-only blocker
in the elided middle passes with exit 0 and the fail-closed branch never
fires. It is now redirected to a file and paged with read_file, and a body
read only in part is `cannot tell`, not "no Critical in it" — the same
lesson as "the diff is a file, not a command".
- The substantive-return gate rejected a bare "No issues found" while the
agent contract demanded exactly that string. The contract now asks for
`No issues found — <one line naming what you examined>`, and the relaunch
is capped at one attempt per agent, with the dimension reported under "Not
reviewed" if the second return is still bare.
* fix(review): wire whole-diff 1b into the gates, cap the event on unread scope, page reviews as NDJSON
Sixth round. Four Criticals all trace to the two previous commits:
- Whole-diff Agent 1b was declared but never wired in: it was missing from
the receipt-less roster (so a whiffing 1b passed undetected) and the launch
contract handed every 3B agent "its own chunk range", which is exactly what
a cross-chunk pairing agent cannot work from. The payload contract now
splits by role — chunk agents get one range, every whole-diff agent gets
the entire chunks[] plan.
- The whiff check ended at "note it as Not reviewed", which left an
unreviewed Security or removed-behavior lens able to ship an LGTM. It now
carries an unreviewedDimensions state that forbids Approve, caps the event
at COMMENT, and is serialized in the body next to uncoverable chunks.
- Clause 5 put an undecidable existing Critical in the body while event
selection still chose APPROVE from the C/S table — a review approving the
very blocker it asks the author to confirm. The table now has explicit
overrides: cannot-tell existing Critical, uncoverable chunk, and unreviewed
dimension each cap the event at COMMENT (a confirmed Critical still earns
REQUEST_CHANGES).
- Redirecting `gh api --paginate` to a file does not make it pageable: it
emits compact JSON, so the file is one 150 KB+ line that read_file
truncates and offset skips past to EOF. The fetch now filters with --jq to
marker-bearing bodies and emits line-delimited records that page normally.
Also: the 1b/1c split is by task, not by symbol (1c greps the removed
export's old name and owns caller compatibility; 1b owns the pairing and the
semantic comparison) — the earlier "no symbol left to grep" claim understated
1c and risked dropping its removed-symbol pass. Plus stale arithmetic from the
larger roster (+4 -> +5, worked example 26-28 -> 27-29), the receipt count's
missing Agent 8, "0 LLM calls" -> "0 subagent calls", the fixed "12 parallel
tasks" -> its real range, and 1c's callee procedure no longer speaking of a
"territory" it does not have.
* fix(review): select body Criticals offline, propagate unreviewed dimensions, close the no-findings bypass
Seventh (final self-review) round. The three Criticals all attack the newest
machinery:
- The NDJSON fetch filtered on a literal [Critical] marker, but a body-only
blocker is not guaranteed to carry it (a real emitted review on this repo
does not) — the filter discarded exactly what the re-check exists to
recover. The --jq now keeps every nonempty body and selection happens
offline after reading records whole; clauses 5 and 7 additionally mandate
the marker on everything we serialize, so our own output stays
self-identifying.
- unreviewedDimensions stopped at the event cap: Step 6's Not-reviewed
section only listed uncoverable chunks (a non-posting run hid the missing
lens entirely), and the body invariant made the required disclosure
illegal on a REQUEST_CHANGES. The section now lists both, and the
not-reviewed clause is the second sanctioned REQUEST_CHANGES body
exception — a confirmed Critical must not squeeze out the disclosure of
what was never read.
- The no-confirmed-findings branch still said "APPROVE by default",
special-casing only presubmit and context-unavailable — bypassing the
cannot-tell/uncoverable/unreviewed caps added one commit earlier. The
branch now runs the same machinery as every submission: table with
overrides, then downgrades, then composition; the hard-coded LGTM example
applies only with no cap state present.
Plus the round's consistency notes: cross-file trace marked same-repo-only
in the docs' medium row; +5 -> +4 in the crossover arithmetic (Build & Test
reads no diff) so "crosses twelve about there" is true at 3200; DESIGN's
whole-diff enumeration gains 1b; budget total widened to the honest 15-21
row-sum; fork-subagent math redone at 52K/agent; the payload paragraph
names the invariant agents' third payload class; consumer-direction grep
patterns get Python/Go forms; 3C medium states 1a's lightweight degradation
and scopes the grep permission; the aggregated-format shorthand carries
Failure scenario and Severity; the exactly-one-sentence rule forward-
references the composition rule; and the Step 7 comment template embeds the
failure-scenario shape it was already demanding in prose.
* feat(review): sink argument parsing into a tested parse-args subcommand
The --comment/--effort grammar and target disambiguation were ~400 words of
prose in SKILL.md that the model re-simulated on every run; three separate
parsing bugs shipped that way (the spaced form consuming a flag as its
value, the --effort=<level> form left undefined, and an invalid value token
surviving into target disambiguation). Each is now a table-driven test case.
qwen review parse-args '<raw args>' emits a JSON verdict: classified target
(pr-number / pr-url with owner+repo+number extracted / file / local),
resolved effort with its source (explicit / default / forced-by-comment),
comment.requested vs comment.effective, verbatim warnings, and leftover
tokens the parser refuses to guess about. The skill's Step 1 shrinks to
"run the parser, use the verdict verbatim", and the target branches key off
target.type instead of hand-classifying tokens.
* feat(review): sink event selection and body composition into compose-review
The Step 7 machine — the C/S table, three event-capping overrides, the
seven-clause body composition, and the presubmit downgrade carve-outs — was
restated across four places in SKILL.md, and keeping the restatements in
sync by hand produced five shipped bugs (four Critical), all one shape: a
downstream branch not updated when an upstream rule gained a new state.
qwen review compose-review reads a state JSON (inline/body Critical and
Suggestion counts, discarded anchors, cannot-tell existing Criticals,
uncoverable chunks, unreviewed dimensions, context-unavailable, presubmit
flags, model id) and returns {event, body, baseEvent, cappedBy, downgraded}
for verbatim submission. The truth-table tests pin every previously shipped
bug as a named case: caps forbid APPROVE but never soften a REQUEST_CHANGES;
discarded Suggestions still count toward S so a 422 resubmit can never
upgrade to LGTM; a self-PR downgrade keeps body Criticals after the
downgrade sentence; the no-blockers opener appears only when certifiable;
every disclosure survives every stacking. Writing the tests immediately
caught one more instance of the class (all-discarded -> S=0 -> APPROVE).
SKILL.md's Step 7 shrinks to gathering the state and using the output
verbatim; the 422 recovery becomes "re-run compose-review with updated
counts"; the no-findings branch is the same call with zero counts; the
posting gate (judgment, not bookkeeping) stays prose.
* feat(review): render review bodies in full, quarantine replied Criticals, raise the gh buffer
The Step 6 body-fetch instruction was rewritten five times in four review
rounds (missing pagination -> shell truncation -> unpageable single-line
JSON -> a marker filter that discarded markerless blockers -> offline
selection) — the signature of a download program written in English. This
ends the chain at its root, in pr-context itself:
- Review bodies render in full under "Review summaries" instead of
240-char snippets: an unmappable or 422-relocated blocker lives only
there, and a snippet once hid one from the re-check. A body past the 8000
cap ends by naming its review id, so the tail stays fetchable as a single
object; a body read in part is `cannot tell`, not "no Critical in it".
- Replied Critical threads are quarantined into their own "Replied
Criticals" section, rendered before the settled threads, instead of
sinking into "Already discussed" — a reply alone ("I disagree") never
retires a blocker, and marker-matching in this direction is fail-safe: a
forged marker can only add a thread to the re-check list, never hide one.
- The gh wrapper's maxBuffer rises from Node's 1 MiB default to 64 MiB,
closing the ENOBUFS that killed pr-context and presubmit mid-review on a
comment-heavy 43-file PR.
SKILL.md's NDJSON fetch block is deleted: the re-check reads the context
file's three finding-bearing sections under its untrusted-data preamble,
with one residual single-object fetch for capped bodies. Verified against
this PR's own 100+-comment history: the markerless body-Critical review
that motivated the last rewrite now renders whole, and the fetch survives
without ENOBUFS.
DESIGN.md records the sinking rationale for all three subcommand changes;
the user docs note that parsing and the event/body decision are now pinned
by unit tests rather than prompt text.
* test(review): register parse-args and compose-review in the exact-list assertion
The parent-command test pins the exact subcommand roster; the two new
subcommands landed without updating it, which is precisely the drift the
assertion exists to catch — it caught it in CI, one directory above where
the new tests were run locally.
* fix(review): carry every disclosure on REQUEST_CHANGES and select blockers semantically
Review round on the new subcommands, plus the prompt notes it surfaced:
- compose-review's REQUEST_CHANGES branch dropped the context-unavailable
clause entirely and gated the not-reviewed disclosure on other parts being
present — an RC with only an uncoverable chunk disclosed nothing. Every
clause whose state holds now appears on every event (a confirmed blocker
must not squeeze out the trust warning or the unread-scope disclosure);
four new tests pin it.
- Step 6 selects blockers semantically, not by the literal [Critical]
marker: legacy body-only blockers were emitted markerless, and a marker
filter once discarded exactly such a review.
- The same-repo pr-context failure now sets context-unavailable like the
lightweight path (the guard's "lightweight" narrowing is removed) — a
same-repo run that lost the context file must not behave as if it had
read it.
- Step 5's dry-round return aligns with the agent contract (receipt-bearing
"No issues found — <what it re-examined>"), ending the contradiction where
a compliant reverse auditor would be flagged as whiffing.
- Consumer-direction grep forms for Python/Go are call sites now, with the
declaration forms explicitly labeled as callee lookup.
- The 15-19 totals left downstream (docs table, DESIGN heading and cost
row) move to the honest 15-21 / 13-20.
* fix(review): stdin transport for parse-args, validated compose input, full-body re-check context
Round 9 of review-the-review on this PR: 19 unique findings across three
reviews, each verified against source before fixing.
parse-args:
- The documented positional invocation broke on any flag-first raw string
(`qwen review parse-args '--effort low'` -> "Unknown argument") and the
`--` form silently returned a wrong local/default verdict. The raw
string now travels on stdin (`--stdin`; SKILL.md pipes a quoted
heredoc, immune to leading dashes, quotes, and $(...)); positional +
--stdin and post-`--` smuggling are refused loudly. Wiring-level tests
drive the real yargs command, pinning the strict-mode rejection that
pure-function tests could not see.
- PR URL identity hardened: the number must end its path segment
(/pull/42oops is refused, never PR 42), owner/repo restricted to
GitHub's name charset (keeps shell metacharacters out of derived
values), scheme matched case-insensitively, url canonicalized
(lowercase scheme/host, query/fragment dropped) with a new host field;
near-miss URLs are warned about and reported in extraTokens, never
guessed into a file path or PR number. Step 1 remote matching now
requires host AND owner/repo.
- Repeated --effort warnings state what is actually in effect (last valid
occurrence / --comment forcing / the default), composed after
resolution; previously a later typo claimed the default while an
earlier valid effort stayed active.
compose-review:
- Input validated at the boundary: absent counts default to 0; malformed
values throw typed errors naming the field. Previously
{bodyCriticals:["x"], modelId} made undefined+1=NaN, failed both event
comparisons, and returned APPROVE over the only blocker.
- "Suggestions are inline." keys off suggestionsInline, not s: an
all-discarded 422 recovery no longer claims inline suggestions while
the discarded sentence says the opposite (s still decides the event).
- canCertify requires !downgraded: a downgraded Approve opens with the
neutral "Reviewed." instead of certifying "no blockers" two clauses
after naming failing CI.
- unreviewedDimensions entries may carry their own reason after an
em-dash and render verbatim (used by Agent 0's fetch failure below).
pr-context:
- Replied-Critical root bodies render in full (shared capBody; a cut
names the comment id and the exact fetch); reply snippets name their
comment id when cut. The Step 6 re-check no longer rules on
silently-truncated claims, and the fail-closed "read in part = cannot
tell" rule can actually fire for this section.
- The LGTM filter matches the exact canonical template, anchored to the
whole body: a legacy body opening with the LGTM line but carrying a
relocated blocker below it is shown instead of dropped.
- classifyInlineThreads() extracted: buildMarkdown and the stdout count
use the same walk, so the count cannot diverge from the file.
SKILL.md:
- Step 6 re-check scope: every comment-bearing section, including
"Already discussed" (inline threads and issue-level comments) — the
quarantine keys on the literal marker, a floor not a ceiling, so
unmarked blockers settle there; the false "holds only non-Critical
threads" parenthetical is gone. The residual long-body fetch redirects
to a file (shell output truncates at 30k) and is read paged.
- Step 5 reverse audit: dry = zero new findings WITH the evidence-bearing
receipt; the substantive-return check runs after every round (one
relaunch); a twice-whiffed agent's round is never dry.
- Step 3: Agent 7 added to both whiff-check rosters (evidence = commands
run + outcomes; build-and-test recorded in unreviewedDimensions on the
second whiff). Agent 0's linked-issue fetch failure is fail-closed
after one retry via a self-explained unreviewedDimensions entry.
- Step 8: a fail-closed run (unreviewed dimensions, uncoverable chunks,
context-unavailable) must not advance the incremental cache — caching
it would exempt the disclosed-unreviewed scope from every future run.
- Counting truthfulness: "Twelve agents all reading the same diff" is
eleven (every 3A agent except Build & Test walks the chunk plan); fixed
in the 3B rationale, the diff-capture section, and the user docs.
review.ts: demandCommand message names plan-diff, with a test that the
message stays in sync with the registered roster.
* fix(review): nested-safe stdin guard, validated presubmit, refetchable snippets everywhere
Round 10: 12 findings, all verified before fixing. The headline is
self-inflicted: the round-9 post-`--` guard read argv._ as
['parse-args', ...extras], but the real CLI nests the command, so argv._
is ['review', 'parse-args'] and the guard rejected every real
invocation — while the wiring tests, which register the command
top-level, stayed green. Reproduced against the built CLI before
fixing.
parse-args:
- The smuggle guard skips the command-path prefix in argv._; new wiring
tests go through the real parent `review` command (nested stdin
invocation + nested post-`--` refusal).
- --effort values match case-insensitively (`--effort High` is not a
file target named High); the verdict keeps the lowercase form.
- Single-dash tokens are unknown flags, never target candidates
(`/review -c 6711` reviewed a nonexistent file `-c` and demoted the
PR number to extraTokens).
compose-review:
- presubmit and contextUnavailable get the same boundary validation as
the counts: boolean flags reject stringified "false" (truthy — it
flipped an inline-Critical RC to COMMENT and published the diff-only
warning on runs that fetched context fine), downgradeReasons rejects
scalars with the field name (was a raw .join TypeError), presubmit
rejects non-objects.
- Certification is gated on what presubmit PERMITS, not on whether it
changed the event: a Suggestion-only review is already COMMENT, so
failing CI flipped nothing and the body still certified "no
blockers". Either downgrade flag now suppresses the certifying
opener.
pr-context:
- Every truncating render carries an exact refetch ref: open-root
snippets, settled replied threads (roots and replies), and
issue-level comments (their own issues/comments endpoint). The
Step 6 semantic re-check reads these sections, and a markerless
blocker past the 240-char cut was invisible with no way back.
- Refs are copy-runnable: buildMarkdown threads owner/repo and PR
number into every ref, so emitted commands carry real values.
`gh api` substitutes only {owner}/{repo} — from the CURRENT repo,
wrong in cross-repo mode — and passes {n} through literally.
SKILL.md:
- Step 1: the raw argument string travels via write_file to
.qwen/tmp/qwen-review-args-input.txt and stdin redirection. A quoted
heredoc disables expansion but not delimiter recognition, so a raw
string containing the delimiter line would end the heredoc early and
execute the rest as shell. Step 9 removes the file.
- Step 1: remote matching is structural segment equality (host AND
owner/repo, .git stripped, case-insensitive) — substring "contains"
let shao/qwen-code match a wenshao/qwen-code remote. Non-github.com
hosts must carry GH_HOST on every gh call for the PR.
- Step 5: a twice-whiffed reverse-audit scope is tracked, cleared only
by a later substantive audit, and fed into unreviewedDimensions as a
self-explained entry when the loop ends — terminal prose alone let a
capped run approve with an audit that never ran.
- Step 6: snippet cuts carry their own filled-in fetch note; ruling on
a cut prefix is the fail-closed violation.
- Step 7: the stale hand-derivation bullets (event table, empty-RC-body
rule, one-line COMMENT inventory) are replaced with descriptions of
what compose-review guarantees; the sanity check is byte equality
with the subcommand's output; the last-resort 422 branch re-runs
compose-review instead of hand-building "the one-line body".
- Step 8: the fail-closed cache rule includes cannotTellCriticals — a
cached SHA plus the same-SHA shortcut would skip the very re-check
that must re-rule on an undecided blocker.
MSG2
git log --oneline -1; git push origin feat/review-procedural-finders-effort 2>&1 | tail -2
* feat(review): deterministic overlap disposal, --host routing, machine-readable completion line
Three changes measured out of the first six-PR dogfood batch, not
predicted from review comments.
Overlap disposal (SKILL.md Step 7): presubmit's overlap report used to
end in "list the overlaps to the user, ask whether to proceed" — 2 of 6
batch runs stalled on an improvised interactive question (fatal for a
headless run) while the other 4 proceeded. An overlap is a duplicate by
the Exclusion Criteria; the rule is now drop the overlapping finding,
adjust the counts handed to compose-review (a dropped finding never
flips the verdict), note "already reported at <path>:<line>" in the
terminal, and continue without asking. Zero findings left after
dropping is still not a question — compose-review handles the shape.
--host routing (lib/gh.ts + fetch-pr/pr-context/presubmit): the
round-10 GH_HOST-by-prose rule required the model to remember a prefix
on every call; a forgotten one silently reads from and posts to
github.com's same-named owner/repo. The three gh-calling subcommands
now accept --host and thread it through setGhHost()/ghEnv(), so every
wrapped gh call carries GH_HOST in code; hostname input is
charset-validated. SKILL.md keeps the prose prefix only for the gh
commands the orchestrating model runs directly (Agent 0's fetches,
Step 6's residual body fetch, Step 7's submission).
Completion line (SKILL.md Step 9): three different ad-hoc completion
phrasings across one batch each needed their own driver regex. Every
run now ends with exactly one line, `Review complete: <target> —
<disposition>`, with a closed disposition grammar covering posted
events, unposted verdicts, and quick passes — detectable with a single
^Review complete: match.
Tests: gh host-state unit tests (inherit-by-default, GH_HOST extension,
host:port, charset rejection), presubmit handler --host threading (set
and reset), builder registration checks for fetch-pr and pr-context.
|
||
|
Shaojin Wen
|
51888210aa
|
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. |
||
|
Shaojin Wen
|
41c405b3bf
|
feat(review): post Suggestion findings as inline comments (#6593)
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. |
||
Dragon
|
80340fb73f
|
fix(review): remove qwen-code-specific core-infra gate from bundled /review (#6412)
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>
|
||
|
Dragon
|
57326e55be
|
feat(review): add issue-fidelity and root-cause ownership gate to /review (#6395)
* 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> |
||
|
Shaojin Wen
|
17fbaa25cd
|
refactor(review): drop deterministic-analysis and autofix steps (#6092)
* 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.
|
||
tanzhenxin
|
808d0978eb
|
feat(cli): route foreground subagents through pill+dialog while running (#3768)
* 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. |
||
|
Shaojin Wen
|
35fe97e0f6
|
feat(review): expand review pipeline + qwen review CLI subcommands (#3754)
Some checks are pending
Qwen Code CI / Lint (push) Waiting to run
Qwen Code CI / Test (push) Blocked by required conditions
Qwen Code CI / Test-1 (push) Blocked by required conditions
Qwen Code CI / Test-2 (push) Blocked by required conditions
Qwen Code CI / Test-3 (push) Blocked by required conditions
Qwen Code CI / Test-4 (push) Blocked by required conditions
Qwen Code CI / Test-5 (push) Blocked by required conditions
Qwen Code CI / Test-6 (push) Blocked by required conditions
Qwen Code CI / Test-7 (push) Blocked by required conditions
Qwen Code CI / Test-8 (push) Blocked by required conditions
Qwen Code CI / Post Coverage Comment (push) Blocked by required conditions
Qwen Code CI / CodeQL (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:docker (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:none (push) Waiting to run
E2E Tests / E2E Test - macOS (push) Waiting to run
* 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>
|
||
|
wenshao
|
820191d99d |
docs: add zero-findings PR tip to follow-up table
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> |
||
|
wenshao
|
a9038a1769 |
fix(review): 5 issues — CI security, incremental+comment, doc accuracy
1. CI config auto-discovery: read from base branch for PR reviews (PR branch is untrusted, malicious PR could inject commands) 2. Incremental early-exit: don't block --comment on unchanged PR — allow posting comments from previous review findings 3. Doc: review summary not always posted (Comment verdict skips it) 4. Doc: cross-repo reviews skip report persistence 5. Doc: clarify "Agents 1-4 findings verified" (not all — reverse audit findings skip verification) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
wenshao
|
c826c24f6e |
fix(review): skip Agent 5 in cross-repo mode, update token counts
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> |
||
|
wenshao
|
8f723575bd |
docs: add CI config auto-discovery to user doc and design doc
- 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> |
||
|
wenshao
|
cbf2aa06ac |
feat(review): add Java and C/C++ support for deterministic analysis
Step 3 now supports: - Java: mvn compile, checkstyle, spotbugs, pmd (Maven); gradle compileJava, checkstyleMain (Gradle) - C/C++: clang-tidy (when compile_commands.json available) Agent 5 build/test precedence now includes Maven and Gradle before Makefile to avoid duplicate builds. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
wenshao
|
720796e699 |
fix(review): safe branch cleanup + cross-repo agent count
- 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> |
||
|
wenshao
|
559f2efd42 |
fix(review): fix cross-repo mode and add documentation
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> |
||
|
wenshao
|
01c105b5e7 |
refactor(review): renumber steps from sub-steps to sequential 1-11
Replace confusing sub-step numbering (1, 1.1, 1.5, 2, 2.5, 2.6, 3, 3.5, 4, 4.5, 5) with clean sequential numbering (1-11). Mapping: 1→1, 1.1→2, 1.5→3, 2→4, 2.5→5, 2.6→6, 3→7, 3.5→8, 4→9, 4.5→10, 5→11 Updated all cross-references in SKILL.md, user docs, and PR description. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
wenshao
|
09c95c44c8 |
fix(review): address 5 Copilot comments
- Add error handling for git fetch and gh pr view failures in Step 1 - Skip worktree cleanup on autofix commit/push failure (preserve uncommitted fixes for manual recovery) - Fix Agent 5 counting: it's 1 of the 5 LLM agents (not a separate zero-cost stage). Remove misleading "zero LLM" annotation and duplicate row from token efficiency table. - Reverse audit skip-verification already implemented (comment #53 was stale) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
wenshao
|
6d7afafe3c |
perf(review): skip verification for reverse audit findings
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> |
||
|
wenshao
|
d6b9b35350 |
fix(review): handle interrupted review cleanup
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> |
||
|
wenshao
|
6b28920a07 |
docs: fix autofix section redundancy and add pre-fix verdict note
- Remove duplicate worktree commit+push bullet (lines 107 vs 109) - Add note that PR submission uses pre-fix verdict since remote isn't updated until autofix push completes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
wenshao
|
ce47f64ae6 |
docs: replace Mermaid with plain-text pipeline diagram
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> |
||
|
wenshao
|
1db1dec517 |
docs: replace step list with Mermaid flowchart in How It Works
Visual pipeline diagram showing: - Sequential flow from scope detection to cleanup - 5 parallel agents subgraph - Decision branches for autofix and PR comments - Zero-LLM-cost stages marked - GitHub renders Mermaid natively Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
wenshao
|
295e907d25 |
fix(review): address 4 Copilot comments on worktree and verification
- 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> |
||
|
wenshao
|
9b9bccd27d |
docs: update user doc with token efficiency, fix follow-up table
- 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> |
||
|
wenshao
|
e65e5bd353 |
perf(review): replace N verification agents with single batch verification
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> |
||
|
wenshao
|
08a797cf76 |
fix(review): address 4 Copilot comments
- 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>
|
||
|
wenshao
|
a5cc2c38cb |
fix(review): fix 5 worktree issues found in audit
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> |
||
|
wenshao
|
dd2de17de5 |
feat(review): use ephemeral worktree for PR reviews
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> |
||
|
wenshao
|
5effbb696f |
feat(review): read existing PR comments to avoid duplicate feedback
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> |
||
|
wenshao
|
757bd9865a |
feat(review): model-aware incremental cache for cross-model review
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> |
||
|
wenshao
|
50d25733d7 |
feat(review): add reverse audit step to find coverage gaps
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> |
||
|
wenshao
|
95a62da039 |
docs: fix review doc accuracy and remove non-existent /simplify
code-review.md: - Add PR URL support to Quick Start - Add "no changes" behavior note - Fix copilot-instructions.md precedence (prefer .github/, not both) - Fix "automatically gitignored" → user must ensure .gitignore coverage - Clarify reports directory is project-relative - Add "What's NOT Flagged" section (exclusion criteria) commands.md: - Replace non-existent /simplify with actual bundled skills (/loop, /qc-helper) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
wenshao
|
ac179b0e02 |
docs: add /review user documentation
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> |