fix(review): prove the diff was read, build every agent's prompt, and compute the verdict - #6892
Merged
Merged
Conversation
…survived the trip Three defects, all measured against the harness's own transcripts of real /review runs against QwenLM/qwen-code PRs #6766 (Step 3B) and #6579 (Step 3A). 1. Step 3A reviews were told nobody had read them. Coverage was attributed by one question: did an agent whose launch prompt says `chunk N of M` make a successful tool call? No Step 3A prompt says that — there every dimension agent walks the whole diff — so no chunk was ever attributed to anyone. Run against a real 3A review whose twelve agents each opened the diff, walked both chunks and filed findings, check-coverage returned Coverage: 0/2 chunk(s) reviewed. 16 agent(s) ran; 16 did work ERROR: 2 chunk(s) were not reviewed — 1, 2. Nobody read those lines. in one breath. compose-review runs the same computation on the way to the verdict, so a flawless small-PR review was capped away from Approve and the body it would have POSTED to the pull request said nobody had read it. Step 3A is the topology most pull requests get. The only reason it never blew up is that check-coverage lived inside Step 3B and was never reached from 3A — two bugs cancelling. Coverage is now the intersection of two things the harness wrote down: the lines each agent was pointed at (its launch prompt) and the fact that it opened the diff (a successful tool call naming the diff file). Topology-blind. It also no longer credits a chunk to an agent on the strength of any successful call — a glob for test files was enough. 2. The whole-diff agents were still launched blind. agent-prompt built the territory agents' prompts and left the other half of the fan-out to prose. All three whole-diff agents of the 3B run — cross-file tracer, test-coverage matrix, build & test — got a prompt naming no diff file at all. The test-coverage matrix was told to "Read the diff chunks" and given no path to read them from; it read the post-change source instead, which on a deletion shows it nothing. These agents own the classes a chunk agent is structurally blind to, and the gate could not see it: it only asked that question of agents whose prompt said `chunk N of M`. `agent-prompt --whole-diff` now builds their diff-reading block too. 3. The prompt the CLI built was rewritten on the way to the agent. The 3B run invoked agent-prompt correctly for all five chunks and then paraphrased what it printed: the delivered prompt dropped the rule against reciting a stock sentence, dropped the half-read warning, replaced the project's review rules with a three-sentence summary of its own, and invented an instruction that was never in the original. Nothing could see it, because a paraphrase keeps the diff path. So agent-prompt records what it emitted, at a path derived from the plan that the caller is never given; check-coverage reads it back against the launch prompt the harness recorded. Replayed against that run's real transcripts, all five chunk agents are now named. Verified end to end: a fresh /review of #6829 (3A) calls `agent-prompt --whole-diff`, passes it verbatim, and Step 3D reports 2/2 chunks reviewed, 12 agents, 12 did work — a gate that path could not reach before, and could not have passed if it had.
Two more failures, both measured against the harness's own transcripts of real /review runs. 1. Agent 0 was never launched, and nothing could tell. The skill says issue fidelity runs on every PR review. Dogfooded on #6766, it did not run — and every check passed, because every check asks a question of an agent that RAN. An agent that does not run leaves no transcript to ask. An omission is invisible precisely because it is an omission. So `check-coverage` now derives a roster from the plan — which the caller does not write — and names every required agent that never ran, with the exact `agent-prompt` call that builds it. The plan already knew everything the roster turns on: the topology, whether the diff deletes anything, which files were rewritten heavily enough to need invariant agents, whether there is a worktree to build in and a pull request to check an issue against. `agent-prompt --role <role>` builds all of them: 0, 1a, 1b, 1c, 2, 3, 4, 5, 6a/6b/6c, 7, the test matrix, and the three invariant agents per heavy file. The briefs move out of SKILL.md and into code, because a brief the orchestrator retypes is a brief that drifts. 2. A 4 652-character prompt is not a thing an orchestrator will paste twelve times. With the briefs welded into the launch prompt, the first dogfood delivered **2 893** characters of one: it kept the head, added a preamble of its own, and cut nineteen hundred characters out of the middle. The delivery check caught it — and the run then read the check's exit-3, concluded "the agents clearly did their job", skipped `compose-review`, and filed an **Approve it had written itself**. A gate that always fails is a gate that gets talked around. So the brief goes where the diff already goes: on disk, read by the agent that needs it. The launch prompt drops to ~500-800 characters — it names the role, points at the brief file, and lists the diff reads — and whether the agent actually read its brief stops being a hope and becomes a tool call the harness wrote down (`unreadBriefs`). Verified end to end. A fresh /review of #6847 built all twelve role prompts, and against the harness's transcripts: 12 of 12 delivered **byte-for-byte verbatim**, 12 of 12 **opened their brief**, 3-19 successful tool calls each — Agent 0 among them. Step 3D: `1/1 chunks reviewed, 12/12 agents did work`, no errors.
`compose-review` has computed the event and the body since the C/S table stopped
being prose. The skill then told the orchestrator to "copy event/body verbatim
into the review JSON" — a transcription, into a document the model writes, of a
decision the CLI had already made. That is the exact anti-pattern `submit`'s own
header repudiates, and it left two ways for a run to author its own verdict:
- **The terminal.** Step 6's verdict was composed by the model, from prose
rules. Dogfooded, a run read the coverage check's refusal, concluded that
"the agents clearly did their job", never called `compose-review` at all, and
printed `Review complete — Approve` on a review whose gate had just refused.
- **The wire.** `submit` took `{event, body}` as fields. Nothing stopped a run
that had skipped the computation from posting the conclusion it preferred.
So `submit` composes. It takes the findings — the inline comments and the states
Step 6 established — and derives everything that follows, including how many
blockers there are: `criticalsInline` and `suggestionsInline` are counted off the
`**[Critical]**` / `**[Suggestion]**` prefixes of the comments actually attached,
not accepted as numbers beside them. (A number beside a list is a number that can
disagree with the list, and one did: the breaching run posted a body reading
"Suggestions are inline" next to an empty `comments` array and a summary claiming
`0 Suggestion inline`.) A payload carrying `event`/`body` is refused rather than
silently overruled — the caller was trying to author a verdict.
Two body checks are deleted, not weakened: a body that promises inline comments it
does not carry, and a body whose footer is preceded by a literal `\n`. Both were
checks on a string the caller built. The caller no longer builds it.
`compose-review` now prints the verdict line itself, and Step 6 prints that. There
is one place a verdict exists; skipping the command does not get you a different
one, it gets you none.
Verified: a payload with `event: APPROVE` and an unreviewed dimension is refused
at the wire; the same findings without a verdict compose to `COMMENT`. A fresh
/review of #6788 called `compose-review`, was told `Verdict: Comment`, and showed
the user "Comment — downgraded from Approve (CI failing: route)" — the presubmit
downgrade applied by code, on a run that did not post.
…ng wolf Dogfooding the Step 3B path — the one topology none of this had been run against — found two defects, and the second is the more important of the two. 1. Eighty-seven kilobytes of chunk prompts, in one response. The briefs moved onto disk for the dimension agents and not for the territory agents. Measured on PR #6606 (5 511 diff lines, 17 chunks): 17 chunk launch prompts of ~5 149 characters each — **87 546 characters** the orchestrator was expected to paste unedited. At a twelfth of that load it had already cut nineteen hundred characters out of a single prompt. Chunk agents get the same split: the brief on disk, and a launch prompt that carries only what cannot live anywhere else — `chunk N of M`, which attributes the territory, and the `offset`/`limit`, which are the lines coverage proves were delivered. 87 546 → 14 789 characters. And `check-coverage` now asks the territory agents the same question it asks the others: did you open your brief? 2. The delivery check failed a correct run — all nine agents of it. It was a substring test: the built prompt had to appear in the launch prompt, contiguously. That is a stricter claim than the skill makes, and both of the differences it fired on were legitimate. The orchestrator had inserted **the one-sentence summary of the change that the skill explicitly tells it to add**, which breaks contiguity by construction — and it had reflowed a hard-wrapped sentence onto one line, which changes not one character of meaning. This is the failure this skill keeps re-learning, and this time it was ours: a gate that fires on a correct run is a gate that gets talked around, and there is a dogfood transcript of a model doing exactly that. The rule the check enforces is now the rule the skill states — **you may add; you may not remove, alter, or reorder** — over whitespace-collapsed lines, in order. Verified against the harness's transcripts of a real Step 3B review of #6766: nine agents (five chunks, issue fidelity, cross-file tracer, test matrix, build & test), 9/9 delivered intact, 9/9 opened their brief, 6-22 successful tool calls each. `check-coverage`: 5/5 chunks, every list empty, exit 0.
…, where they were never reaching the agents
Two changes, and the second found a hole the first would not have.
1. Rules that attach to a path, not to a dimension.
The nine dimensions are domain-blind by design — "find security bugs" is a lens,
not a syllabus — and that holds until a file's failure modes are not guessable
from reading it. A GitHub Actions workflow is the clearest case: it is YAML, so it
reads as configuration, and the reviewer who treats it as configuration misses
every one of its attack classes. Nothing in this review knew to ask whether a
`pull_request_target` job checks out the contributor's head — which is the
difference between a CI file and a remote code execution with the repository's
write token. This repo runs `qwen-autofix.yml`, which posts to pull requests.
`agent-prompt` now appends a checklist for such a file to the brief of every
code-reviewing agent **whose territory actually contains one**. Scoped, because a
rule that fires on every review is a rule that gets skimmed. `/review` runs on
other people's repositories, so the calibration matters as much as the content:
the blockers are the six that are unambiguously wrong; the two that shade into
taste (SHA-pinning, `permissions:`) are Suggestions, exempt the conventions almost
everyone keeps, and are scoped to lines the diff touches. No style rules — a
linter owns those, and the Exclusion Criteria already forbid them.
2. The briefs move out of SKILL.md — and three things turned out never to have
reached an agent at all.
The briefs have been built in code since the roster landed, and SKILL.md still
carried 38 KB of the same prose. Duplication is drift, and a 178 KB skill is
~45 000 tokens in the orchestrator's context on every review — which is itself a
cause of the failure this whole line of work has been chasing. The skill now keeps
what each agent is *for* (a table) and drops what it is *sent* (the command's copy
is the one that arrives).
Doing that surfaced what the code briefs were missing, because the deleted prose
had to go somewhere:
- **The Exclusion Criteria had never reached an agent.** The skill states them at
the end of the document and tells the orchestrator to "apply" them. The agents
do not read the document. The single largest precision control in this review
has been governing nobody, in every run, since it was written.
- **Nor had the anchor rules.** Agents were asked for a snippet and never told
what makes one resolvable: prefer added lines, a removed line cannot be anchored
at all, a bare `}` matches everywhere. `resolve-anchors` was downstream of a
snippet nobody had given the rules to produce.
- **Nor the severity calibration.** `SEVERITY`'s own comment warns that a chunk
agent owns test coverage with nothing to calibrate it and will file "zero test
coverage" as Critical — and then did not include the calibration.
All three are in the briefs now. And two degradations the orchestrator used to be
told to add by hand — and can no longer add, because it does not write these
prompts — are applied by the builder: in cross-repo lightweight mode there is no
tree, so 1b and 1c report at `Confidence: low` rather than asserting a
re-establishment is missing. A false Critical blocks a merge.
Step 3C (the medium-effort inline pass) now *loads* the briefs it needs rather
than carrying them: same text as the high-effort agents get, read when that level
actually runs instead of sitting in every review's context.
SKILL.md: 171 178 → 153 656 bytes.
…ker's blast radius be part of the blocker Two corrections, both from dogfooding the paths that had never been run. 1. An invariant agent was being handed the whole chunk plan. It owns one heavily-rewritten file. Its brief says so, and gives it that file's own slice of the diff. Its *launch prompt* listed every chunk in the review — on PR #6457, all twenty-one reads of a 6 149-line diff, for an agent whose job is one file. The wasted reading is the smaller half. Coverage is computed from the ranges in the launch prompt, so an invariant agent was being credited with having read **every chunk in the review**. One of them could have masked twenty missing chunk agents. It now gets exactly its file's `diffRange`, and nothing else. 2. `permissions: write-all` on a job that runs untrusted code is not a Suggestion. The path rule said it was. Dogfooded against a planted vulnerability, the security agent read that and escalated anyway: "grants maximum token scope to a job that processes untrusted contributor code, amplifying the RCE above". It was right and the flat rule was too coarse. A broad token on a privileged job is not a separate recommendation — it is how far the blocker reaches, and it belongs in that finding, at Critical. On an ordinary job it stays a Suggestion.
qwen-code-ci-bot
requested changes
Jul 14, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
The substance here is excellent — seven real defects found and verified through dogfooding, with transcripts. But the PR body is missing all the required template headings, which makes it harder for the review pipeline and future readers to navigate:
## What this PR does## Why it's needed## Reviewer Test Plan(with### How to verify,### Evidence (Before & After),### Tested on)## Risk & Scope## Linked Issues
Could you add the template structure? The narrative content is great — it just needs to fit under the right headings so reviewers can find the test plan, evidence, and risk assessment without reading the whole body.
Template: .github/pull_request_template.md
中文说明
内容非常好——通过 dogfooding 发现并验证了七个真实缺陷，附有 transcript。但 PR body 缺少所有必需的模板标题，不便于审查流程和后续读者导航：
## What this PR does## Why it's needed## Reviewer Test Plan（含### How to verify、### Evidence (Before & After)、### Tested on）## Risk & Scope## Linked Issues
请补充模板结构。叙述内容很好——只需放到对应的标题下，方便审查者快速找到测试计划、证据和风险评估。
— Qwen Code · qwen3.7-max
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Author
CI: the two failures are flakes, not this branch`Test (ubuntu-latest, Node 22.x)` failed on two tests, and both are timeouts in files this branch does not touch:
Both pass locally on this branch: This branch touches only Failed jobs re-run. 中文说明
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output.
— qwen3.7-max via Qwen Code /review
Comment on lines
311
to
312
| if (payload.state === undefined) { | ||
| problems.push( |
Collaborator
There was a problem hiding this comment.
[Suggestion] structuralProblems does not reject state: null — Concrete cost: a payload with "state": null passes the === undefined guard (null is not undefined). compose then collapses it to {} via ?? ({} as ComposeReviewInput), producing a confusing error downstream instead of the intended "state is missing" message.
Suggested change
| if (payload.state === undefined) { | |
| problems.push( | |
| if (payload.state == null) { | |
| problems.push( |
Loose equality (== null) catches both undefined and null.
— qwen3.7-max via Qwen Code /review
Comment on lines
+144
to
+145
| /** Drop the record. Called by `cleanup`, which owns the rest of the temp files. */ | ||
| export function removePromptRecord(planPath: string): void { |
Collaborator
There was a problem hiding this comment.
[Suggestion] removePromptRecord is exported with a doc comment saying "Called by cleanup" but cleanup.ts never imports or calls it. Cleanup happens via cleanup.ts's prefix-based rmSync sweep. Either have cleanup.ts call this function (making the comment true and cleanup more precise), or correct the doc comment to describe the actual cleanup mechanism.
— qwen3.7-max via Qwen Code /review
| '', | ||
| '```bash', | ||
| `qwen review test-efficacy ${opts.planPath} \\`, | ||
| ` --worktree ${typeof wt === 'string' ? wt : '<worktree>'} \\`, |
Collaborator
There was a problem hiding this comment.
[Suggestion] worktreePath may be relative (e.g., .qwen/tmp/review-pr-6892), but the test-efficacy command embeds it as the --worktree argument. Since the agent's cwd is already the worktree, the relative path would resolve to <worktree>/.qwen/tmp/review-pr-6892 (nonexistent). Use resolve(wt) (already imported from node:path) to produce an absolute path, or emit --worktree ..
— qwen3.7-max via Qwen Code /review
| writeStdoutLine( | ||
| JSON.stringify( | ||
| { posted: false, wouldPost: true, target, event: payload.event }, | ||
| { posted: false, wouldPost: true, target, event }, |
Collaborator
There was a problem hiding this comment.
[Suggestion] Dry-run JSON omits cappedBy — Concrete cost: a user running --dry-run to preview what would be posted sees "event": "COMMENT" with no explanation of why the verdict was capped from APPROVE. The non-dry-run path (line 523) already includes cappedBy; add it here too.
Suggested change
| { posted: false, wouldPost: true, target, event }, | |
| { posted: false, wouldPost: true, target, event, cappedBy }, |
— qwen3.7-max via Qwen Code /review
…equest The repository's own `/review` bot reviewed #6892 — this change reviewing the code that changes it — and filed six Suggestions. Every one of them is real, and two are fail-open holes in the gates this pull request exists to build. They are fixed here, each with a test. - **`submit` accepted `state: null`.** `=== undefined` is not `== null`, so the structural check passed it; `compose`'s `?? {}` then collapsed it to an empty state and would have posted a review whose footer named no model and whose caps came from nowhere. - **`wasDeliveredVerbatim` was vacuously true for an empty `built` prompt.** A zero-byte record is what a partial write leaves behind — and `recordPrompt` swallows its write errors by design, so this is reachable. `readRecordedPrompts` stores it as `''`, not `undefined`, so the "no prompt was built" guard did not catch it, and the loop's body never executed. The roster would have credited a required role to whichever transcript it looked at first. It now fails closed. - **A chunk read across two pages got no credit.** The check asked for a *single* range containing the chunk, and reads of 1-200 and 201-400 are two — so it contradicted the paging instruction the same review had just given, on exactly the oversized chunks where paging is not optional. Ranges are coalesced first. - **Agent 7 was handed relative paths it could not resolve.** `worktreePath` and the plan path are repo-relative in the report, and Agent 7's working directory *is* the worktree — so `--worktree .qwen/tmp/review-pr-6457` resolved to `<worktree>/.qwen/tmp/review-pr-6457`, which does not exist. This was already visible and nobody had read it: in the 29-agent dogfood run, Agent 7 spent its time running `find … -name "*6457*fetch*"`, hunting for a plan it had been handed a path to. Absolute now. - **`removePromptRecord` was dead code with a comment claiming a caller it did not have.** `cleanup.ts` sweeps the prompt directory by prefix instead. Deleted. - **`--dry-run` omitted `cappedBy`.** The point of a dry run is to see what would be posted; `"event": "COMMENT"` with no reason leaves the reader to guess why the Approve went away.
Collaborator
Author
All six findings fixed — and this is the result the PR was arguing forThe repository's own
Zero false positives. Two of the six are defects in the gates themselves, and #5 is a bug whose live failure was in a transcript I had already looked at. That is the argument of this pull request, run against the pull request: the review's value is not in what it says about itself — it is in what its agents actually read. 562 tests in 中文说明仓库自己的
零假阳性。六条里两条是闸门自身的缺陷，第 5 条的失败现场就躺在我早已看过的一份 transcript 里。 这正是本 PR 的论点，作用在本 PR 身上：一次 review 的价值不在它如何自述，而在它的 agent 究竟读了什么。 |
qwen-code-ci-bot
requested changes
Jul 14, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] SKILL.md Step 7 contradiction: unchanged main-path JSON examples still show event/body fields (lines ~786-815), but submit.ts now refuses payloads carrying those fields. An orchestrator following the unchanged instructions would build a payload that submit rejects. The new no-findings path (line ~905) correctly states the new contract, but the main-path examples were not updated.
— qwen3.7-max via Qwen Code /review
Comment on lines
+400
to
+401
| const brief = briefPath(planPath, req.key); | ||
| const opened = agent.successfulCallArgs.some((a) => a.includes(brief)); |
Collaborator
There was a problem hiding this comment.
[Suggestion] unreadBriefs check does not verify the brief file exists on disk before reporting — Concrete cost: if writeBrief silently fails (e.g., inode exhaustion) but recordPrompt succeeds, the coverage report says the agent "never opened its brief" when the brief was never written. Consider adding existsSync(brief) check or a separate missingBriefFiles category.
— qwen3.7-max via Qwen Code /review
Comment on lines
+160
to
+162
| if (report.missingChunks.length > 0) { | ||
| writeStderrLine( | ||
| 'NOTE: a chunk counts as read when an agent was pointed at its lines AND ' + |
Collaborator
There was a problem hiding this comment.
[Suggestion] Redundant NOTE + ERROR output for the same missingChunks condition — Concrete cost: when chunks are missing, the user gets a NOTE explaining why chunks might be missing followed by an ERROR listing which ones. Consider merging the NOTE's context into the ERROR message at line 185.
— qwen3.7-max via Qwen Code /review
The reviewer caught a real contradiction it filed as Critical: submit.ts now
refuses a payload carrying `event`/`body` (those are computed from `state` and
the attached comments), but Step 7's main-path 'Build the review JSON' examples
still showed `"event": "REQUEST_CHANGES"` / `"body"` and routed the verdict
through a copy-it-verbatim step. An orchestrator following the unchanged
instructions would have built exactly the payload submit rejects.
The correct `{commit_id, comments, state}` shape existed lower in the section (the
no-findings branch), added when submit took over composition — but the main-path
examples and the compose-review-then-transcribe bullets above them were never
reconciled. They are now: one payload shape, no verdict in it, `state` handed to
submit, and the inline counts derived from the comments rather than supplied.
Found by the repository's own /review on #6892.
Collaborator
Author
The Critical is real — fixed in
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| It reads the harness's own per-agent transcripts: a record you do not author, are not given the path to, and cannot revise. It reports three failures, and they are not the same: | ||
| **This step runs on both topologies.** It used to live inside Step 3B and be reachable only from there, and it modelled coverage as "an agent whose prompt says `chunk N of M` made a tool call" — which no Step 3A agent's prompt ever says. Run against a real 3A review whose twelve agents each opened the diff, walked both chunks and filed findings, it reported `0/2 chunk(s) reviewed … Nobody read those lines` in the same breath as `16 agent(s) ran; 16 did work`. `compose-review` runs the same computation on the way to the verdict, so that review was capped away from Approve and the body it would have posted to the pull request said nobody had read it. Both sentences cannot be true. Coverage is now the intersection of two things the harness wrote down: the lines each agent was **pointed at** (its launch prompt) and the fact that it **opened the diff** (a successful tool call naming the diff file). | ||
|
|
||
| It reads the harness's own per-agent transcripts: a record you do not author, are not given the path to, and cannot revise. It reports six failures, and they are not the same: |
Collaborator
There was a problem hiding this comment.
[Suggestion] Count mismatch: the text says "six failures" but seven bullet points follow (Agents that never ran, never opened brief, launched blind, not launched with CLI-built prompt, pointed at diff but never opened it, made no tool call, chunks nobody reviewed). check-coverage.ts reports eight distinct categories (blindAgents, rewrittenPrompts, missingRoles, unreadBriefs, unopenedAgents, idleAgents, uncoverableChunks, missingChunks). An orchestrator trusting the count "six" would silently drop one failure class.
Suggested change
| It reads the harness's own per-agent transcripts: a record you do not author, are not given the path to, and cannot revise. It reports six failures, and they are not the same: | |
| It reads the harness's own per-agent transcripts: a record you do not author, are not given the path to, and cannot revise. It reports eight failures, and they are not the same: |
— qwen3.7-max via Qwen Code /review
The repository's /review passed #6892 (no blockers) and filed three Suggestions. All three are real; two are contradictions this PR itself introduced. - **verdictLine printed a dangling colon.** When a would-be Approve was taken away by a presubmit downgrade ALONE — no cap state, `cappedBy` empty, `downgraded` true — the code joined the empty array and produced 'an Approve was NOT available: — downgraded by a presubmit check', a colon over nothing. It now collects the reasons (a cap and a downgrade are both reasons, either can be the only one) and prints the clause only when there is a reason to. The function had no test; it has six now, including this case. - **Step 3D said 'six failures' and listed seven, while check-coverage reports eight.** The count drifted as failure classes were added, and the uncoverable-chunk class had no bullet at all. Now 'eight', with the missing bullet written. - **`submit --review` help still advertised `event` / `body`** as payload fields, which the same command now refuses. Updated to `commit_id / comments / state`. Found by the repository's own /review on #6892 — the third pass, the one that turned CHANGES_REQUESTED into no-blockers.
Collaborator
Author
Third pass: no blockers, three Suggestions — all real, fixed in
|
| # | Finding | Verdict |
|---|---|---|
| 1 | verdictLine has no test; a downgrade-only path could print a dangling colon |
Real, and reproducible. When a would-be Approve is taken away by a presubmit downgrade alone — no cap state, cappedBy empty, downgraded true — the old code joined the empty array: an Approve was NOT available: — downgraded by a presubmit check, a colon over nothing. The reviewer had the mechanism slightly off (it framed it as cappedBy empty + downgrade, which is exactly the trigger) but the instinct was right and the bug was there. Now it collects the reasons — a cap and a downgrade are both reasons, either can be the only one — and prints the clause only when there is one. Six tests added, this case among them. |
| 2 | Step 3D says "six failures", lists seven, check-coverage reports eight |
Real. The count drifted as failure classes were added across this PR, and the uncoverable-chunk class had no bullet at all. Now "eight", with the missing bullet written. |
| 3 | submit --review help still advertises event / body |
Real — same root as pass two's Critical, one layer down. The --help text still listed event/body as payload fields the command now refuses. A user reading --help would build the rejected shape. Updated to commit_id / comments / state. |
Zero false positives across all three passes: 6 + 1 + 3 = ten findings, ten real. Every one was either a fail-open in a gate this PR builds, or a place where a rule was stated once and contradicted somewhere the author did not re-read — which is the failure the whole change is about, and the review reads the whole document, so it keeps finding it.
568 tests in commands/review, green. dist SKILL rebuilt.
中文说明
第三轮:无 blocker,三条 Suggestion,全真,已在 c2cb5b618 修复。 上一轮那条 stale-event/body 的 Critical 消失了(verdict 从 CHANGES_REQUESTED 降到 no-blockers)。三条里两条是本 PR 自己引入的矛盾:
verdictLine无测试,downgrade-only 路径会打印悬空冒号 —— 真且可复现。当一个本该 Approve 的裁决仅因 presubmit 降级(无任何 cap,cappedBy空,downgradedtrue)被拉走时,旧代码 join 了空数组,输出an Approve was NOT available: — downgraded by a presubmit check——冒号后面是空的。审查器把机制说得略偏(它说成cappedBy空 + downgrade,而这恰好就是触发条件),但直觉对、bug 在。现在改成先收集"原因"(cap 和 downgrade 都算原因,任一都可能是唯一),有原因才打印那一句。新增六个测试,含这一例。- Step 3D 写"six failures"却列了七条,
check-coverage实际有八类 —— 真。计数随本 PR 陆续加失败类别而漂移,而 uncoverable-chunk 那一类根本没有 bullet。现在改成"eight",并补上缺的那条。 submit --review的 help 仍宣传event/body—— 真,与上一轮那条 Critical 同源、低一层。--help文本还把event/body列为字段,而这个命令现在会拒绝它们。照着--help走的人会构造出被拒的形状。已改成commit_id / comments / state。
三轮零假阳性:6 + 1 + 3 = 十条 finding,十条全真。 每一条要么是本 PR 所建闸门里的 fail-open,要么是"一条规则在一处声明、在作者没回看的另一处矛盾"——而这正是整个改动要解决的失败,审查器读的是整份文档,所以它一次次抓到。
commands/review 568 测试绿,dist SKILL 已重建。
doudouOUC
reviewed
Jul 15, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Manual review of the coverage / roster / prompt-record / submit changes.
Overall this is solid, well-tested work. The fail-closed reasoning holds up (an empty prompt record makes wasDeliveredVerbatim return false; a missing planPath caps compose-review to COMMENT; submit strips env from caller state so transcripts can't be redirected), the id-based tool-call pairing in transcripts.ts fixes a real "N parallel calls counted as 1" bug, and I confirmed the self-review fixes are present (the verdictLine dangling-colon guard, --dry-run carrying cappedBy, and the stale event/body example purge).
One substantive finding (inline on roster.ts): a heavy file in a Step-3A-sized diff makes the roster demand invariant agents that Step 3A never launches, which falsely blocks an otherwise-complete review. Plus two minor notes — a merge() input-mutation nit and an Agent-0 readsDiff coupling question.
Posting as a COMMENT (not blocking a maintainer PR); the roster/skill mismatch is the one worth resolving before merge.
qwen-code-ci-bot
requested changes
Jul 15, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] submit.test.ts:186 — test 'posts when the user typed --comment' fails (ghMock called 0 times). The authorization gate at submit.ts:170 checks currentSessionId() === '' && args.skillArgs, but in the test environment currentSessionId() returns a non-empty value, so the fallback to args.skillArgs is never reached and the post never happens.
— qwen3.7-max via Qwen Code /review
… agents From a human review of #6892 (doudouOUC). `heavy` is decided independently of topology (lib/heavy.ts): a ~300-line source file with ~120 changed lines clears the rewrite-ratio branch while srcDiffLines stays under 500 — a Step 3A review. The invariant-agent loop in requiredAgents ran in both topologies, so it added invariant-a/b/c to the roster of a 3A review that never launches them; check-coverage then reported them as missingRoles and exit-3'd, and compose-review capped the verdict — an otherwise-complete small PR, falsely blocked. Gate the loop on isTerritoryFanOut. Step 3A's dimension agents each walk the whole diff, so one already sees both ends of a rewritten file; invariant agents are a 3B mechanism for when the diff is carved into territories and no single agent holds the whole file. roster.test.ts now pins the 3A-heavy case. Also, same review: merge() in coverage.ts copied its first tuple and pushes copies, so it no longer mutates a tuple owned by rec.diffReads (harmless today, pure now).
Collaborator
Author
Fourth pass: one real human finding fixed (
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| // half that was not got launched with no diff path at all, and the one that was | ||
| // never launched at all could not be seen by anything that inspects the agents | ||
| // that ran. | ||
| describe('buildRolePrompt — every agent, not just the territory ones', () => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] Misleading describe block name: buildRolePrompt does not exist as a function. Every test inside calls buildRoleBrief. A maintainer grepping for buildRoleBrief tests will not find this block.
Concrete cost: discoverability of the test suite.
Suggested change
| describe('buildRolePrompt — every agent, not just the territory ones', () => { | |
| describe('buildRoleBrief — every agent, not just the territory ones', () => { |
— qwen3.7-max via Qwen Code /review
| it.each([ | ||
| ['neither', {}], | ||
| ['both', { chunk: 13, 'whole-diff': true }], | ||
| ])('rejects a call that names %s of the two modes', (_, extra) => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The mode-rejection test covers only 2 of 8 combinations (neither and both for chunk + whole-diff). The production code now has three mutually exclusive modes (chunk, whole-diff, role), but role+chunk, role+whole-diff, and all three are untested. The message "the two modes" also reflects the old two-mode design.
Concrete cost: a future refactor that breaks role-mode mutual exclusion would not be caught.
Suggested change
| ])('rejects a call that names %s of the two modes', (_, extra) => { | |
| ])( | |
| 'rejects a call that names %s', | |
| (_, extra) => { |
And add cases: ['chunk+role', { chunk: 13, role: '2' }], ['whole-diff+role', { 'whole-diff': true, role: '2' }], ['all three', { chunk: 13, 'whole-diff': true, role: '2' }].
— qwen3.7-max via Qwen Code /review
Comment on lines
+579
to
+583
| } else if (r.downgraded) { | ||
| // The base was not an Approve (a Suggestion-only Comment, say), so there was no | ||
| // Approve to lose — but a presubmit downgrade still moved the event, and the | ||
| // user should see that it did. | ||
| line += ' — downgraded by a presubmit check'; |
Collaborator
There was a problem hiding this comment.
[Suggestion] When baseEvent is REQUEST_CHANGES and a presubmit check downgrades it to COMMENT, the verdict reads "Comment — downgraded by a presubmit check". The operator sees "Comment" and may conclude there are no blockers, when Criticals were found and the review would otherwise have been REQUEST_CHANGES. The code comment says "a Suggestion-only Comment, say" but this branch also fires for REQUEST_CHANGES.
Failure scenario: an operator running submit sees the terminal verdict and acts on it — "Comment" reads as "minor issues, nothing blocking", but the review actually posted Criticals inline.
Suggested fix: distinguish the two cases:
} else if (r.downgraded) {
if (r.baseEvent === 'REQUEST_CHANGES') {
line += ' — Request changes was downgraded to Comment by a presubmit check';
} else {
line += ' — downgraded by a presubmit check';
}
}— qwen3.7-max via Qwen Code /review
|
|
||
| // `verdictLine` is what Step 6 prints — the one place a verdict exists for the | ||
| // user. It had no test, and a review of this change found the reason to want one. | ||
| describe('verdictLine — the terminal verdict, and its dangling colon', () => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The verdictLine test suite covers 6 scenarios but omits baseEvent: 'REQUEST_CHANGES' with downgraded: true. The else if (r.downgraded) branch at compose-review.ts:579 handles this case, and the COMMENT+downgraded test exercises the same branch with a different baseEvent, but the REQUEST_CHANGES origin is untested.
Concrete cost: a regression that mishandles the REQUEST_CHANGES+downgraded case would go undetected.
Suggested change
| describe('verdictLine — the terminal verdict, and its dangling colon', () => { | |
| describe('verdictLine — the terminal verdict, and its dangling colon', () => { | |
| const line = (over: Partial<ComposeReviewResult>): string => | |
| verdictLine({ | |
| event: 'COMMENT', | |
| body: '', | |
| baseEvent: 'COMMENT', | |
| cappedBy: [], | |
| downgraded: false, | |
| ...over, | |
| }); | |
| it('says a Request changes downgraded to Comment was downgraded', () => { | |
| expect( | |
| line({ event: 'COMMENT', baseEvent: 'REQUEST_CHANGES', downgraded: true }), | |
| ).toContain('downgraded'); | |
| }); |
— qwen3.7-max via Qwen Code /review
| ] | ||
| ], | ||
| "state": { | ||
| /* the compose-review state below */ |
Collaborator
There was a problem hiding this comment.
[Suggestion] The JSON example uses /* comment */ inside a ```json code fence. JavaScript-style comments are not valid JSON — a reader copying the example verbatim would get a parse error.
Concrete cost: documentation accuracy; a reader following the example literally cannot produce valid JSON.
Switch the fence to ```jsonc or replace the comment with a valid placeholder.
— qwen3.7-max via Qwen Code /review
…mment Fifth self-review, one behavioural finding among five (the rest are test/doc). verdictLine printed 'Comment — downgraded by a presubmit check' for BOTH a Suggestion-only Comment the presubmit moved and a REQUEST_CHANGES it moved down to Comment. The second is a review with confirmed Criticals posted inline, and 'Comment — downgraded' reads to an operator as 'nothing blocking'. It could not tell them apart from baseEvent alone — a cap may already have softened the RC before the downgrade ran — so ComposeReviewResult now carries downgradedFrom, and verdictLine says 'Request changes, downgraded to Comment … (the blockers are still posted)' for that case. Six verdictLine cases now, including this one. Also from the same review, all confirmed: - agent-prompt.test.ts: the describe block named a function that was renamed (buildRolePrompt -> buildRoleBrief), and the mode-rejection it.each covered 2 of the invalid combinations, not the role-mode ones; now covers all five and drops the stale 'two modes' wording. - SKILL.md Step 7: the review-JSON example used a /* */ comment inside a ```json fence (not valid JSON); switched to ```jsonc with a // comment.
Collaborator
Author
Fifth pass: no blockers, five Suggestions — all real, fixed in
|
| # | Finding | Verdict |
|---|---|---|
| 1 | verdictLine prints Comment — downgraded for a downgraded Request changes too, hiding that Criticals were posted |
Real, and the one that matters. A presubmit downgrade (self-PR, failing CI) moves a REQUEST_CHANGES — a review with confirmed Criticals inline — down to COMMENT. Printed as a bare "Comment — downgraded", an operator reads "nothing blocking". baseEvent alone could not distinguish it (a cap may have softened the RC before the downgrade ran), so ComposeReviewResult now carries downgradedFrom, and the line reads "Request changes, downgraded to Comment by a presubmit check (the blockers are still posted)". A sixth verdictLine test pins it. |
| 2 | describe block named buildRolePrompt — a function renamed to buildRoleBrief |
Real. Renamed. |
| 3 | mode-rejection it.each covered 2 of the invalid combinations, missed the role-mode ones, and said "two modes" |
Real. Now covers all five (none, chunk+whole-diff, chunk+role, whole-diff+role, all three). |
| 4 | verdictLine suite omitted baseEvent: REQUEST_CHANGES + downgraded |
Real — the test for #1. Added. |
| 5 | Step 7 review-JSON example used a /* */ comment inside a ```json fence |
Real. Copying it verbatim gives a JSON parse error; the example already had {placeholder} values, so the honest fence is ```jsonc with a // comment. Switched. |
#1 is worth dwelling on: it is a regression in my previous fix. Last pass I split verdictLine to kill a dangling colon and wrote the new branch thinking only of the Suggestion-only case — the comment even said "a Suggestion-only Comment, say". The review read the branch against every baseEvent that reaches it and found the one I had not: a downgraded blocker, announced as if it were not one. That is the class this whole PR is about — a rule (or here, a display) correct in the case its author pictured and wrong in the one they did not — and it caught it in the fix for the same class, one pass later.
Tally across five passes: 16 findings, 15 real and fixed, 1 false (the pass-four submit.test.ts "failure" that CI and local both showed green). 573 tests green.
中文说明
第五轮:无 blocker,五条 Suggestion,全真,已在 24df1020f 修复。 一条是行为问题,其余是测试/文档。
verdictLine对「被降级的 Request changes」也打印Comment — downgraded,掩盖了已发的 Critical —— 真,且是最要紧的一条。presubmit 降级(self-PR、CI 失败)会把一个有 Critical 内联的REQUEST_CHANGES降到COMMENT;打印成光秃秃的 "Comment — downgraded",operator 会读成「没 blocker」。单靠baseEvent分不出来(cap 可能在降级前已软化了 RC),所以ComposeReviewResult现在带downgradedFrom,该行改为 "Request changes, downgraded to Comment …(the blockers are still posted)"。第六个verdictLine测试钉住它。describe块名叫buildRolePrompt(已改名buildRoleBrief)—— 真,已改。- mode-rejection
it.each只覆盖 2 种非法组合、漏了 role-mode、消息还写 "two modes" —— 真,现覆盖全部五种。 verdictLine测试漏了baseEvent: REQUEST_CHANGES+downgraded—— 真,即 pre-release: fix ci #1 的测试,已补。- Step 7 的 JSON 示例在
```jsonfence 里用了/* */注释 —— 真,照抄会 parse 错;示例本就有{占位符},故改成```jsonc+//。
#1 值得多说一句:它是我上一轮修复的回归。 上一轮我拆 verdictLine 去消除悬空冒号,写新分支时只想着 Suggestion-only 那种情况——注释甚至写着 "a Suggestion-only Comment, say"。审查器把这个分支对着每一个会到达它的 baseEvent 都验了一遍,找到了我没想到的那个:一个被降级的 blocker,被当成非 blocker 播报。这正是整个 PR 讲的那类失败——一条规则(这里是一处显示)在作者设想的情况里对、在他没设想的那个里错——而它在「修同一类失败的那次修复」里,晚一轮,抓到了它。
五轮合计:16 条 finding,15 真已修,1 假(第四轮那条 submit.test.ts "失败",CI 与本地都显示绿)。573 测试绿。
…lution Follows 7c499d1, which resolved the doudouOUC roster finding (a heavy file in a Step-3A diff must not demand invariant agents — gate the loop on the topology) and the merge() purity nit. This carries the rest of the same round: - `roster.ts` requires Agent 0 only for a positive PR number. `!== undefined` let `null`/`0`/`''` through. Note the reviewer's suggested `typeof === 'number'` is wrong for this codebase — `fetch-pr` writes the number as a *string* — so the guard accepts a numeric string too, or every real PR review would lose Agent 0. A table test pins both directions. - `transcripts.ts` matches the diff path as a whole JSON string value, so `…/diff.txt.bak` no longer counts as reading `…/diff.txt`. It also documents why FIFO is right for a chronological transcript. - `agent-prompt.ts` scopes path rules to `--file` only for invariant roles — a whole-diff reviewsCode agent passed `--file` would otherwise lose the rules for every other file — and guards each chunk element in `diffReadingBlock` like `chunkFrom`, so a corrupted chunk errors legibly instead of emitting `offset=NaN`. - `compose-review.ts` stops double-wrapping `cov.missingRoles` / `cov.rewrittenPrompts`, which coverage.ts already writes self-explanatory. - `agent-briefs.ts` JSDoc said "Two do not" read the diff; only Build & Test does not. - The agent-prompt size-bound test now covers `test-matrix`. The empty-prompt guard, the paged-read coverage, the verdictLine dangling-colon and the submit help text were all already handled by earlier commits on the branch; those threads are answered without a code change.
Collaborator
Author
Automated-review round: dispositionFollow-up to the human review, covering the Fixed:
Fixed — but not as suggested: Already handled by earlier commits on the branch — no change, verified:
Not taken (reasoned):
577 review tests green; lint/format/tsc clean on the changed files. 🤖 Reviewed & applied with Claude Code · Opus 4.8 (1M context) |
doudouOUC
approved these changes
Jul 15, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed at 619f336e1 — all three findings from my earlier review are resolved, and CI Test (ubuntu) is green. Approving.
My findings — all addressed:
- Heavy file in a Step-3A diff (
roster.ts) — fixed in7c499d193. TheheavyFilesloop is now gated onisTerritoryFanOut, so invariant agents are demanded only in the 3B topology that actually launches them; a newroster.test.tscase pins the 3A-heavy behavior. The rationale is sound — 3A dimension agents each walk the whole diff, so they already see both ends of a file. merge()input mutation (coverage.ts) — fixed in7c499d193: it seeds and pushes tuple copies, so the function is now pure.- Agent 0
readsDiffcoupling (agent-briefs.ts) — confirmed intentional (issue fidelity has to judge the stated intent against what the diff actually does), and the stale "Two do not read the diff" JSDoc was corrected to "One". Acceptable as deliberate fail-closed behavior.
Additional self-review fixes (24df1020f, 619f336e1) also verified:
- The
downgradedFromverdict-line fix is a genuinely good catch — aREQUEST_CHANGESdowngraded toCOMMENTby a presubmit check no longer reads as "nothing blocking" when blockers are posted inline. isPositivePrNumber(hardens the Agent-0 gate againstnull/0/''/junk while accepting the numeric-string formfetch-prwrites), thediffReadingBlockNaN guard, theinvariant--gated path-rules scoping, the quoted-path match intranscripts.ts(fixes the…/diff.txt.bakfalse-positive, with no false negative for the instructedread_filepath), and themissingRoles/rewrittenPromptslabel de-dup all look correct and are each pinned by a test.
No outstanding concerns. Nice work turning each finding around with a test to pin it. LGTM 🎉
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Not reviewed: chunk 1, chunk 2, chunk 3, chunk 4, chunk 5, chunk 6, chunk 7, chunk 8, chunk 9, chunk 10, chunk 11, chunk 12, chunk 13, chunk 14, chunk 15, chunk 16, chunk 17, chunk 18, chunk 19, chunk 20, chunk 21, chunk 22, chunk 23, chunk 24 — no agent reported covering these; nobody read them.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Thanks for the PR! Template looks good ✓ — all required sections present and filled in with real content. Problem: Observed, not theoretical. Seven specific defects identified across six rounds of dogfooding against real pull requests of this repo, each measured against the harness's per-agent transcripts. The coverage bug produced contradictory output ( Direction: Strongly aligned. The core thesis — "a rule stated in prose is a rule that will eventually not be followed; the fix is to move it into code that can say no" — is the right architectural direction for an automated review skill. This is the same lesson the skill has already applied to the review target, posting gate, and coverage report, now extended to the four remaining gaps: agent prompts, the roster, the terminal verdict, and rules that never reached agents. No CHANGELOG reference but the area is directly within the skill's reliability mandate. Size: This PR touches one core-path file ( Approach: The scope feels right for the problem. Each change maps to a named, demonstrated defect. The architecture is clean: briefs on disk (not in prompts), coverage as line-range intersection (topology-blind), roster derived from the plan (not caller-supplied), verdict computed at submit time (refusing hand-written event/body), and delivery verified by subsequence check (not substring). Tests are thorough — 452 passed in the review test suite — with each test tied to a specific observed failure. No drive-by refactors or scope creep. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必需章节均已填写，内容真实。 问题： 已观测到的缺陷，非理论性加固。通过在本仓库真实 PR 上进行六轮 dogfood 测试，发现了七个具体缺陷，每个都通过 harness 的 per-agent transcript 进行了量化验证。coverage bug 产生了矛盾的输出（"0/2 chunks reviewed" 与 "16 agents ran; 16 did work" 并存），Agent 0 在一次真实 review 中完全未被启动，编排器在传递 prompt 前对其进行了改写。before/after 证据具体且可验证。 方向： 高度对齐。核心论点——"用散文写下的规则最终不会被遵守；解法是把它变成能说「不」的代码"——是自动化 review skill 的正确架构方向。CHANGELOG 中无直接引用，但该领域直接关系到 skill 的可靠性。 规模： 本 PR 涉及一个核心路径文件（ 方案： 范围与问题匹配。每项改动对应一个已证实的缺陷。架构清晰：brief 落盘、coverage 取行范围交集（拓扑无关）、roster 从 plan 推导、裁决在 submit 时计算、交付通过子序列检查验证。452 个测试通过，每个测试关联一个已观测的故障。无顺手重构或范围蔓延。进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Code ReviewNo critical blockers found. The implementation is clean, well-structured, and thoroughly tested. Architecture — the move from prose-enforced rules to code-enforced invariants is applied consistently across every layer: prompt recording ( Prompt recording — Coverage — the topology-blind coverage model (intersection of pointed-at lines and diff-opened ranges) elegantly solves the Step 3A/3B discrepancy. The Transcript parsing — the switch from stack-based to ID-based call-response pairing is correct. The FIFO fallback for anonymous (un-ID'd) calls matches JSONL's chronological ordering. Submit authorization — reading the raw user-typed argument string and parsing it in Verdict composition — Path rules — the GitHub Actions security checklist is well-calibrated: scoped to agents whose territory contains workflow files, distinguishes blockers (Critical) from recommendations (Suggestion), and correctly handles the Tests — 581 tests across 29 files, all passing. Each test is tied to a specific observed failure from dogfooding. The test for Minor observation (not a blocker): the test TestingUnit tests (581 passed, 0 failed): Real-scenario testing: not performed in this triage. This PR changes internal review skill infrastructure (prompt building, coverage computation, verdict composition) — verifying it end-to-end requires running 中文说明代码审查未发现关键阻塞问题。实现干净、结构良好、测试充分。 架构 — 从散文强制到代码强制的转变一致地应用于每一层：prompt 记录、coverage 计算、roster 推导、交付验证和裁决合成。每个模块独立可测，接口清晰。 Prompt 记录 — Coverage — 拓扑无关的 coverage 模型（指向行与 diff 打开范围的交集）优雅地解决了了 Step 3A/3B 不一致问题。 Transcript 解析 — 从基于栈到基于 ID 的调用-响应对匹配是正确的。匿名调用的 FIFO 回退匹配 JSONL 的时间顺序。当没有 Submit 授权 — 在 裁决合成 — 测试 — 29 个文件中 581 个测试全部通过。每个测试关联一个 dogfood 中发现的具体故障。 小观察（非阻塞）： 当 测试单元测试（581 通过，0 失败）： 真实场景测试： 本次审查中未执行。本 PR 变更的是内部 review skill 基础设施——端到端验证需要通过完整 Qwen Code TUI 运行 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage; the defects are real, the fix is correct, and the test suite proves it. This PR addresses a genuine reliability crisis in the The architecture is the right one. Every layer that was previously enforced by prose is now enforced by code: briefs live on disk where the orchestrator can't truncate them, coverage is the intersection of two independently-recorded artifacts (what the agent was pointed at and what it opened), the roster is derived from the plan rather than supplied by the caller, and the verdict is computed at submit time from the same state Step 6 showed the user. The key insight — that a rule stated in prose is a rule that will eventually not be followed — is applied consistently and without exception. 581 tests pass, each tied to a specific observed failure. The test for The PR is large (~2,900 production lines) but not bloated — each of the seven defects requires its own module, and they're interconnected through the coverage check. Splitting would have been artificial and would have left intermediate states where some defects were fixed and others weren't. One minor observation: the test Ship it. ✅ 中文说明置信度：5/5 — 各阶段均无问题；缺陷真实存在，修复正确，测试套件已证明。 本 PR 解决了 架构是正确的。之前由散文强制的每一层现在都由代码强制：brief 存在磁盘上（编排器无法截断），coverage 是两个独立记录工件的交集（agent 被指向的内容和它打开的内容），roster 从 plan 推导而非由调用方提供，裁决在 submit 时从 Step 6 展示给用户的同一 state 计算。关键洞察——"用散文写下的规则最终不会被遵守"——被一致且无例外地应用。 581 个测试通过，每个关联一个已观测的故障。PR 规模较大（约 2,900 生产行）但不臃肿——每个缺陷需要独立模块，且通过 coverage 检查互连。拆分将是人为的。 一个小建议： 可以合并 ✅ — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
approved these changes
Jul 15, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
This was referenced Jul 15, 2026
wenshao
added a commit
that referenced
this pull request
Jul 16, 2026
…inds Reported from a real session: `npm run dev:daemon`, `/review 6998 --comment` in the web shell, and the run died on Missing required argument: chunk with a help screen for a command that has no `--role` at all. The daemon was running the checkout — the skill it loaded is the current one, and says `--role 0` — but the skill shells out to `qwen review agent-prompt …`, and `qwen` on that machine is `/usr/bin/qwen` → a v0.19.10 global install whose `agent-prompt` predates #6892 entirely. The skill and the CLI it was talking to were different programs. The skill assumed `qwen` on PATH is the build running it. That holds for a single install and breaks for exactly the people most likely to run a dev daemon. It is also invisible when it breaks: the error names an argument, not a version. So the entry is passed down instead of rediscovered. `scripts/cli-entry.js` is the executable entry and the one thing that knows its own path, so it publishes it as `QWEN_CODE_CLI` (`||=`, so the relaunch into dist/cli.js keeps pointing callers back at the wrapper with the shebang, not at itself). `daemon-dev.js` sets it too — the dev daemon is started as `node scripts/dev.js` and never passes through the wrapper, which is why this bit there first. `getShellContextEnvVars` passes it to every shell subprocess, beside the session and project-dir vars that are already handed down for the same reason. The skill's 23 command sites now read `"${QWEN_CODE_CLI:-qwen}" review …`; the fallback keeps hosts that do not export it on the old behaviour. PATH was the other candidate and was rejected: prepending a shim dir means writing an executable at spawn time and overriding `PATH` in an env that `normalizePathEnvForWindows` has already normalised — a `Path`/`PATH` collision on Windows in exchange for saving one variable. The env var is isolated in `shellContextEnv.test.ts` the way the session id already is: the CLI now exports it to every shell it spawns, so `npm test` run from inside a qwen session inherits it, and the exact-equality assertion would have failed on a variable the test never set. Verified by running the suite with it set.
wenshao
added a commit
to wenshao/qwen-code
that referenced
this pull request
Jul 18, 2026
…ne call (QwenLM#7033) * fix(review): name a rewritten launch as itself, and leave nothing to hand-assemble Dogfooded on a real 3A review of a live PR, and the run talked its way past the gate: compose-review printed: Verdict: Comment — an Approve was NOT available: a dimension nobody reviewed the run's next thought: "the compose-review flagged reverse audit as unreviewed (transcript visibility issue — the reverse audit did run substantively with two dry rounds). Let me proceed." the run then reported, and saved: Verdict: Approve The gap was right and its wording was wrong, and the wording is what let the run dismiss it. Two auditors HAD run — 16 and 23 tool calls each — and both HAD opened their brief. What had actually happened is that the orchestrator skipped `--findings` and hand-wrote their launches, keeping only the brief pointer, so no agent was launched with the prompt the CLI built. The gap said "no agent was launched with it that opened its brief", which is false as written, and "a transcript visibility issue" is what a reader concludes from a message that does not describe what happened. So the floor tells the four shapes apart instead of collapsing them into one boolean, and each says what happened and what to do: not-built — the step was skipped; `agent-prompt --role <r>` never ran not-launched — the prompt was built and nothing was launched with it rewritten — an agent ran and opened its brief, but no agent got the built prompt: the launch was written by hand instead of pasted brief-unread — an agent got the built prompt and never opened the brief `rewritten` is the one that just happened, and it is now un-dismissable: it concedes the agent ran and read its brief, and names the orchestrator's own edit as the defect. And the path that produced it is gone: `--findings` is now REQUIRED for a role that takes findings. There is no bare-block-plus-hand-assembly path left — the command refuses, and prints one block to paste. An early reverse-audit round with nothing confirmed yet passes an empty file, which the command renders as "Nothing is confirmed yet". SKILL: Step 4/5 say `--findings` is required. Step 6 gains the second half of the lesson — you may not overrule the line compose-review gives you; a cap you can explain is still a cap, and the fix is to make the step verifiable and re-run, not to keep the verdict you preferred. Step 8's report interpolates the verdict out of the composed JSON (`jq -r .event`) instead of typing it, because the terminal is prose and the archive is forever. * fix(review): call the CLI that is running, not whatever `qwen` PATH finds Reported from a real session: `npm run dev:daemon`, `/review 6998 --comment` in the web shell, and the run died on Missing required argument: chunk with a help screen for a command that has no `--role` at all. The daemon was running the checkout — the skill it loaded is the current one, and says `--role 0` — but the skill shells out to `qwen review agent-prompt …`, and `qwen` on that machine is `/usr/bin/qwen` → a v0.19.10 global install whose `agent-prompt` predates QwenLM#6892 entirely. The skill and the CLI it was talking to were different programs. The skill assumed `qwen` on PATH is the build running it. That holds for a single install and breaks for exactly the people most likely to run a dev daemon. It is also invisible when it breaks: the error names an argument, not a version. So the entry is passed down instead of rediscovered. `scripts/cli-entry.js` is the executable entry and the one thing that knows its own path, so it publishes it as `QWEN_CODE_CLI` (`||=`, so the relaunch into dist/cli.js keeps pointing callers back at the wrapper with the shebang, not at itself). `daemon-dev.js` sets it too — the dev daemon is started as `node scripts/dev.js` and never passes through the wrapper, which is why this bit there first. `getShellContextEnvVars` passes it to every shell subprocess, beside the session and project-dir vars that are already handed down for the same reason. The skill's 23 command sites now read `"${QWEN_CODE_CLI:-qwen}" review …`; the fallback keeps hosts that do not export it on the old behaviour. PATH was the other candidate and was rejected: prepending a shim dir means writing an executable at spawn time and overriding `PATH` in an env that `normalizePathEnvForWindows` has already normalised — a `Path`/`PATH` collision on Windows in exchange for saving one variable. The env var is isolated in `shellContextEnv.test.ts` the way the session id already is: the CLI now exports it to every shell it spawns, so `npm test` run from inside a qwen session inherits it, and the exact-equality assertion would have failed on a variable the test never set. Verified by running the suite with it set. * fix(review): point the dev daemon's CLI at the source it is running, not dist Verifying the previous commit on a real `npm run dev:daemon` caught it doing a smaller version of the bug it fixes. The daemon runs the TypeScript **source** through tsx; `cli-entry.js` runs `dist/cli.js`. Pointing QWEN_CODE_CLI there traded "the subprocess is a whole major version behind" for "the subprocess is however stale the last build was" — measured on the box that reported this, dist was **105 source files** behind the daemon. Same bug, smaller hat. `scripts/dev.js` is the entry that runs what the daemon itself runs, so the dev daemon points there. It gains a shebang and the exec bit, which is what lets a caller invoke it as `"${QWEN_CODE_CLI}" review …` without knowing it needs node — the same shape `cli-entry.js` already has for the published path. Verified end to end on a headless box: started the dev daemon, read /proc/<pid>/environ (QWEN_CODE_CLI=<repo>/scripts/dev.js, -rwxr-xr-x), and ran the command that started this whole thread. Before: `Missing required argument: chunk`. After: `agent-prompt: --role 0 needs a plan with prNumber and ownerRepo` — the role is understood, and the complaint is about the fixture, which is the correct answer. * fix(review): say what a missing brief proves, once, to the reader who can act on it A role with no recorded prompt proves one thing: the brief never reached an agent. The roster check claimed more than that — "no prompt was built for it (`agent-prompt --role 0` never ran)" — and on QwenLM#7012 it said that about all twelve dimensions of a review that had just posted two Criticals with line numbers. The agents were in the same comment the gate was calling empty. Both failures are real and neither is the other. An orchestrator that writes the launch by hand gets an agent that runs, reads the diff and finds things, having never seen the severity bar, the finding format or this project's rules — all of which live in the brief it was never given. That is worth blocking on. It is not "nobody looked", and a check may not report the reading it cannot see. Three changes, one shape: - The per-role text says the brief never reached an agent, and that the dimension was reviewed "if at all" from a prompt the run wrote for itself. It no longer speaks for the agent's existence. - Every role briefless collapses to one line. It is one failure — the run did not use the prompt builder — and saying it twelve times buries the fact that explains all twelve. - The public body drops the internal command. `agent-prompt --role 2` is not something a PR author can run; on QwenLM#7012 fourteen lines of it were the whole CHANGES_REQUESTED while the findings sat inline below the fold. The call survives in check-coverage's stderr, where the orchestrator reads it, and the role number is already in each label. check-coverage no longer leads with a count: the collapsed line covers the whole roster, so "1 required brief" would undercount it by the size of the review. Behaviour is unchanged — the gate fires on exactly the same runs and still caps the verdict. Only the sentence changes, and only where it was overclaiming or talking to the wrong reader. * fix(review): name the directory the missing briefs were missing from "The prompt builder never ran" and "the prompt builder ran against a different --plan" arrive at this check as the same thing — an absent file — and they are fixed differently. Nothing in the error told them apart. The record directory hangs off the plan path as given, so a relative --plan resolves against the caller's cwd, and the skill runs Steps 2-6 from inside the worktree it just created. Two cwds, one relative path, two directories. Proven locally: the same `--plan .qwen/tmp/p.json` from a repo root and from a worktree under it yields two record dirs. That is not a reason to resolve the path differently — resolving a relative path against the cwd is what a relative path means, and the mismatch mostly fails loudly, because the plan is not in the worktree either and the read errors. It is a reason to print where it looked. One line, on stderr, where the orchestrator reads it; the PR author gets no path to a temp directory. * feat(review): build the whole roster in one call, because compliance decays per call The launch prompts are already small — a role line, the brief pointer, the diff reads — and it did not save the run that stopped building them. Dogfooded on one PR, the same environment went from a clean review to "no prompt was built for any of twelve roles" over three reviews in a day. The per-agent form asks the orchestrator for ~30 build-then-launch round trips on a large review, and that is a compliance cost paid per agent, per review, forever; what decays under repetition eventually decayed. `agent-prompt --roster` builds every prompt the plan requires — chunk agents, dimension agents, invariants — in one call: one labelled block per agent, each recorded under the key `check-coverage` will look it up by. The list is `requiredAgents(plan)`, the same list the coverage gate reads, so what gets built is exactly what gets checked; a key the two derive differently is refused at build time rather than surfacing later as "brief never reached an agent" on a compliant run. The blocks are separated by lines that are visibly not prompt text, and a block copied lazily — separator included — still passes the add-only delivery check. That is load-bearing: if honest-but-sloppy copying read as a rewrite, the gate would punish exactly the behaviour this call exists to buy. The per-agent forms stay, for rebuilding a single prompt after Step 3D names a gap. Step 4/5 verify and reverse-audit are untouched: they are built per round, with the findings folded in. SKILL.md's Step 3A and 3B now ask for the roster once instead of one call per agent, and check-coverage's missing-brief error names the one-call fix first. * fix(review): close the review's five consistency gaps in the CLI-pinning story Review feedback on this PR found five places where the fix stopped short of its own thesis. All five, addressed: 1. Four copyable SKILL.md commands had missed the QWEN_CODE_CLI sweep — `pr-context` (lightweight mode), `cleanup` (cache hit), `capture-local --file` (file-path reviews), `agent-prompt --whole-diff` (Agent 8). On a skewed host those modes died exactly the way the motivating run did. All four now carry the prefix; the remaining bare mentions are prose. 2. check-coverage's own stderr recommended recovery with a bare `qwen` — the message is the interface the orchestrator acts on, and on a skewed host the recommended recovery reproduced the skew. All four recommendation sites now print the prefixed form, and the rebuild hint covers `--chunk <id>`, which a missing chunk agent needs and `--role` cannot express. 3. Ambient inheritance could silently re-point an entry at another session's CLI. A dev daemon started from inside another qwen session's shell — the usual dogfooding flow — inherited that session's QWEN_CODE_CLI through `??`/`||=` and called the OUTER build: the same skew, one level up, and silent. Every entry now stamps itself unconditionally; nested sessions each call their own build. The `||=` comment in cli-entry.js also claimed a relaunch hazard that does not exist (the relaunch child runs dist/cli.js and never re-executes the wrapper) — the comment now states the real reason. 4. The third dogfooding entry point was still unpinned: `npm run dev` and `npm start` published nothing, so a /review from a plain dev TUI fell back to PATH. `scripts/dev.js` now stamps the variable in the env it spawns with — which also covers the daemon, since daemon-dev launches serve through it, and the daemon's own deferring copy is gone (one writer, not two). `scripts/start.js` does the same and gains the shebang and exec bit that make it callable as the entry it now names. 5. `"${VAR:-fallback}"` is POSIX parameter expansion, which cmd.exe passes through literally and PowerShell rejects. The skill was already POSIX-bound (Step 0 pipes through `tee`); the requirement is now total, and SKILL.md says so where the variable is introduced: on Windows, run the review from git-bash. The unconditional stamp is pinned by a test that inherits a foreign QWEN_CODE_CLI and asserts the spawned child gets this checkout's dev.js; flipping the assignment back to `??` turns exactly that test red. * fix(review): finish the two-register split, and pin the last unpinned entry Round-2 review feedback: five more places where this PR's own rules were not yet applied to itself. The Agent 7 brief handed its subagent a bare `qwen`. Its two fenced command blocks (`build-test`, `test-efficacy`) are the one call site where a SUBAGENT shells out to the review CLI — reachable by neither the SKILL.md sweep nor the stderr hints. Its shell gets QWEN_CODE_CLI exactly as the orchestrator's does, so the standard prefix works verbatim; without it, an old PATH global likely lacks these subcommands entirely, wedging the agent between its mandate (no hand-run builds) and a command that does not exist. A test now rejects any line-initial bare `qwen review` in that brief. The Step 4/5 gap texts and the blind-agent line carried remediation commands into the posted body — the register §4 stripped from missingRoles, surviving in the sibling paths, and partly ADDED by this PR (the rewritten texts). Each gap is now two sentences for two readers: `gap` (author-facing, no internal commands, rendered under `Not reviewed:`) and `fix` (orchestrator-facing, printed by compose-review to stderr as `FIX:` lines, carried on the result as `remediation`). The four-shape precision is intact — it moved channels, not content — and tests pin both directions: the body may not contain `agent-prompt`/`--findings`, and the remediation must. Pinning start.js exposed a stdout contamination: check-build-status.js printed "Checking build status..." to stdout ahead of every child, and start.js is now an entry whose stdout callers consume — `review parse-args --stdin | tee` would write a plan file whose first line is not JSON. The checker's status lines go to stderr with its warnings; `./scripts/start.js --version` now emits the version alone. Also from review: the all-briefless hint no longer points at role labels the collapsed line does not carry, and start.js's stamp gets the same test dev.js has — inherit a foreign QWEN_CODE_CLI, assert the spawned child gets this checkout's entry. * fix(review): isolate the env var this PR exports, and give every gap its FIX Two findings from the bot review of the previous commit. The shellContextEnv suite isolated QWEN_CODE_SESSION_ID and QWEN_CODE_CLI but not QWEN_CODE_PROJECT_DIR — the third variable the CLI exports to every shell, and the one this suite's own per-session tests assign without cleanup. Reproduced: run the suite with it set, as any `npm test` from inside a qwen session does, and exactly the two `.toEqual()` exact-match tests fail on a key the test never set. Same isolation, same shape, and it retires the in-file leak too. The remediation channel covered blind agents and the Step 4/5 gaps and stopped there: missing briefs, rewritten launches, unread briefs and never-opened diffs still reached the body with no FIX line beside them. A body disclosure with no repair command is how QwenLM#7012's orchestrator got to "the agents clearly did their job" — the whole reason the channel exists. Each category now pushes one remediation line (missing briefs point at `--roster`; the relaunch-shaped ones say relaunch with the same printed prompt), and a test pins the pair for a roster gap: the body says "brief never reached an agent" with no command in it, and the remediation names the roster call. * fix(review): retire the last two overclaims the round-3 review found Two sentences, same class, both this branch's own thesis applied to itself. A chunk agent that ran on a hand-written prompt while its chunk was never built landed in the body as "no prompt was built for it (`agent-prompt` never ran for this chunk)" — an internal command on the author-facing surface, one line per chunk on a 3B replay of the QwenLM#7012 shape. The label now says what happened in the author's register (ran on a prompt the run wrote itself; the brief never reached it); the rebuild command already rides the rewritten-launches remediation line on stderr. And the Step 4/5 `not-built` texts still said "no auditor ran" / "no verifier ran" — the one residual of the overclaim this branch exists to retire. `not-built` is decided before the transcripts are consulted: a run that skipped the builder and hand-wrote the launch leaves no brief on disk whose open could be looked for, so such an auditor is invisible to the check, and "no auditor ran" claims sight it does not have. Both texts now use the roster wording: what a missing record proves (no agent was launched with a prompt this skill builds), then what it costs ("ran, if at all, without the method its brief carries"). The Delivery docstring records why. Tests pin the new sentences positively and negatively; the register pin (no `agent-prompt`/`--chunk` in a body label) guards the first one. * test(review): make the every-gap-has-a-FIX claim true, and pin the partial stderr shape Round-5 review caught a test whose title outran its body: "every coverage gap … has a FIX" exercised only the missing-roles path, so dropping the remediation push for unread briefs — or rewritten launches, or never-opened diffs — failed nothing. That is the exact disclosure-without-repair state the channel exists to prevent, asserted by a test that could not see it. The title now claims what the test covers, and a sibling test covers the rest: one plan, three defects — a chunk agent on a hand-written prompt, one that never opened its brief, one that never opened the diff — asserting each category's FIX line and that none of the three drags a command into the body. Between the blind-agent test, the missing-roles test and this one, every category that discloses is now asserted to repair; mutation-checked by deleting each push in turn, one red test each. Also from the review: the missing-briefs stderr had handler coverage only for the all-briefless collapse. The partial shape — one role missing, the rest briefed — reached stderr through no test, so a formatting regression there (a broken join, a lost --roster hint, a garbled Looked-in path) would ship unseen. A second handler test pins it: the per-role detail, the rebuild hints, and the record-dir line, with the collapse text asserted absent. * fix(review): close the round-5 findings — entry contracts, gap reach, repair loops A GPT-5 review pass filed twenty-eight findings against this branch. Nineteen were real and are fixed here; two were refuted with evidence (the scripts test suite IS in CI: `test:ci` runs `npm run test:scripts`); the rest are recorded follow-ups of documented floor designs. Entry contracts. The standalone package launches through a shim that carries the bundled Node and announces itself via QWEN_CODE_LAUNCHER_PATH — stamping cli-entry.js there handed subprocesses a `#!/usr/bin/env node` script on hosts that may have no system Node; the shim is now preferred, with a test. The variable also predates this branch with a second meaning: desktop tooling sets it to a vendored dist/cli.js — a module path, no shebang — which a POSIX shell would run as a shell script; getShellContextEnvVars now drops a shebang-less script (and only a script: a native binary needs none), restoring the bare `qwen` fallback for those hosts. And both dev launchers read a signal-killed child (`code === null`) as exit 0 — a killed gate command reported green; both now re-raise the signal, with close(null, 'SIGKILL') regressions. The production entry's stamp gets the test only the dev entries had. Gap reach. `not-launched` said the pass "did not run" — but a hand-written launch that never opened the brief lands in that shape too, so it now uses the certification language the other shapes got. The roster check judged only the FIRST transcript matching a built prompt, so a failed attempt masked the compliant relaunch that the remediation itself prescribes — all matches are consulted now. An agent flagged rewritten is no longer also flagged unopened (contradictory repairs for one agent), and the all-briefless collapse no longer coexists with one "none was built" line per chunk transcript. Repair loops. Every rebuild command the run prints is now executable as written — plan, selector, and `--rules` included, because a rebuild without the rules file writes a rules-free brief that every delivery check still passes; the verify variant stops inviting the empty findings file that is only legitimate for a reverse-audit round. check-coverage prints exact selectors beside the human labels. Idle agents and unread chunks get FIX lines too, and a handler test pins the boundary: every FIX on stderr, before the verdict, never in the JSON. SKILL.md Step 6 now says what FIX lines are for: one bounded repair round, recompose, then the cap stands. Roster integrity. The output is self-checking against the 30 000-character shell truncation the skill itself documents — numbered blocks, an end-of-roster line, and SKILL.md redirects it to a file read back paged. A PR-controlled filename can no longer forge a block boundary: control characters flatten to spaces in the label and the launch prompt, and a test pins the separator count. The jq interpolation in the report template is gone — the verdict line is copied from Step 6's output, not recomputed by a binary the host may not have. The findings read-error no longer advises omitting a flag another guard requires. * fix(review): filter by overwriting, not omitting — the spread carries what the record drops The shebang filter fixed the wrong layer. It omitted QWEN_CODE_CLI from the record getShellContextEnvVars returns — but every spawn site composes the child env as `{...process.env, ...vars}`, so a key omitted from the additive record arrives anyway, inherited through the spread. On exactly the hosts the filter was written for (desktop tooling setting the variable to a shebang-less vendored dist/cli.js), the value leaked through and every `"${QWEN_CODE_CLI:-qwen}"` in the skill died on exit 126 — where before this branch those hosts ran bare `qwen` and worked. The fix is the pattern this same function already documents for the agent/prompt IDs: write an EMPTY string, which overwrites the inherited value through the spread, and which the consumer's `:-` expansion treats exactly like unset. The test comment that justified omission — "an empty string would shadow the fallback" — was true only of the colon-less `${VAR-qwen}` form and is corrected where it stood, so the reasoning that produced the bug does not outlive it. The tests now assert on the channel the bug lived in: composing `{...process.env, ...getShellContextEnvVars()}` and reading the child env — for the shebang-less case, the unreadable-path case, and the pass-through case. Reverting the overwrite to an omission turns exactly the two filter tests red. Verified end-to-end: with the desktop shape in the parent env, a child shell resolves `"${QWEN_CODE_CLI:-qwen}"` to the PATH `qwen` again. Also from the same review: the two adjacent `missingReceipts` blocks in compose-review are one block now (disclosure and repair cannot drift apart), and the `Exact selectors:` line says a rebuild of an already-built role is idempotent, so the over-prescription cannot make an operator hesitate. * fix(review): reunite roleLabel with the doc comment the selectorOf insertion orphaned The insertion left roleLabel's one-line JSDoc stranded above selectorOf, stacked on top of the new function's own — a maintainer chasing a wrong-label bug would have edited the rebuild-flags function. Each doc sits on its function again. * fix(review): close the round-9 findings — convergence, injectivity, and the claims a record can carry Fourteen findings from a GPT-5 review of the previous head; twelve fixed here, one was already fixed in the commit the review missed, one re-recorded as the standing roster-design follow-up. Repair loops now converge. Coverage accumulated every historical failed transcript, so the relaunch its own FIX line prescribes ADDED a transcript while the failed one kept its flag — ok stayed false, the same FIX printed forever. A failed attempt is now superseded by a compliant attempt at the same target (same chunk served verbatim with the diff opened; same built prompt delivered to an agent that opened its brief), and a rewritten agent is not also told to relaunch the prompt that was the defect. One transcript, one credit. Pasting the whole roster output to a single agent produced one transcript that verbatim-contains every block, matched every requirement independently, and certified an N-agent fan-out with one reader (reproduced upstream: roster 8, agents 1, ok true). Requirements now claim distinct transcripts; the paste-all run fails with a sentence that names the mistake. Records claim only what they prove. "Its brief never reached an agent" said more than a missing record can see (the builder may have run against another --plan spelling); it now reads "no record shows its brief reaching an agent". The rewritten texts claimed the brief's method never arrived — but that shape is DETECTED by the brief being opened; they now state exactly that, and that the launch was not the built one. A zero-byte record (a torn write) no longer counts as built anywhere: one predicate serves the collapse, the roster loop and the chunk lookup. Entries the shell can actually run. The shebang filter now also requires the execute bit (a 0644 script passes the header check and dies on EACCES), and cli-entry consumes QWEN_CODE_LAUNCHER_PATH at stamp time — the serve/mcp fast path never reached the branch that deleted it, so a standalone daemon leaked the outer shim into every child, where a different checkout would republish it as its own entry. Inputs a PR cannot weaponize, commands an operator can run. The invariant brief interpolated the raw PR-controlled filename into the file the agent is told is the whole of its instructions — display sinks now flatten control characters and the functional read argument is JSON-quoted. Agent 7 no longer receives the review rules its own workflow forbids it (SKILL.md: deterministic commands, not code review). The verifier refuses an empty findings file — a vacuous pass that cleared the delivery floor while ruling on nothing — while the early reverse-audit round keeps it. FIX lines carry the run's real plan path instead of a `<plan>` placeholder that pastes as a shell redirection, the roster truncation hint names --file and --rules, and composed.json persists the exact verdictLine so the archived report copies rather than reconstructs it — event and cappedBy alone cannot express a presubmit downgrade. Every new behaviour is pinned: convergence, paste-all refusal, and the zero-byte collapse are mutation-checked (disabling each turns exactly its test red); the exec-bit, brief-injection, launcher-consumption and verdictLine contracts each carry a direct test. 1 236 tests across the affected suites. * fix(review): close the three paths the round-11 review found still open The Step 4/5 FIX lines still carried a literal `--plan <plan>`. Round 9 substituted the real path into compose-review's own remediation strings and check-coverage's hints, and left the one builder both Step 4/5 gaps flow through — `rebuildFix` — untouched: its output reached stderr through verificationGaps with the placeholder intact, and a literal `<plan>` pasted into a POSIX shell parses as input redirection, so the one repair round Step 6 prescribes could never run there. The push sites now substitute the plan path verificationGaps was handed, and the test that pins the fix text asserts no literal `<plan>` survives anywhere in the remediation. A lightweight cross-repo review can now be REQUIRED to run Agent 0. plan-diff takes `--pr <n> --repo <owner/repo>` — passed only after pr-context succeeds, so the pair's presence doubles as the context-availability signal — and writes the identity into the plan; the roster requires role 0 wherever the full identity is present, not only in worktree mode (fetch-pr always writes both fields, so PR-worktree behavior is unchanged). Half an identity is refused: a roster demanding an agent nobody can brief would wedge the run. SKILL.md's lightweight capture block carries the flags and the when-not-to-pass-them rule. And the path-inertness boundary is one function with a wider net: `inertPath` now flattens every control character (a terminal escape in a filename must not reach a terminal), the separator glyph, and the backtick — which could close the Markdown code span the path is rendered inside and let the tail of a PR-controlled filename run as markup in the brief the agent treats as authoritative. The roster label and launch-prompt sites that had their own narrower regexes now share it. The injection test's hostile filename gained a backtick and an ESC sequence, and asserts the rendered heading carries exactly the span's own backtick pair and no control bytes, while the JSON-quoted functional read argument still round-trips the raw path. Each fix is mutation-checked: reverting the substitution, re-gating the roster on worktree mode, and narrowing inertPath each turn exactly one test red. * fix(review): bind the receipt to what was delivered, and match what actually assigns Three review-integrity holes from the round-12 review, each with a reproduction, each fixed at the layer the reproduction named. The verify receipt could be satisfied by a partial delivery. The record was deliberately the findings-free launch block, so one key could serve every shard by the add-only rule — and that same rule let a caller build with a real findings file, launch the agent with only the recorded tail, and clear the gate while no verifier ever saw a finding. The record is now the EXACT printed prompt, findings folded in, keyed per findings-content digest (`verify--<sha>`, `reverse-audit--chunk-N--<sha>`); the delivery side collects the whole key family with the documented floor of one. Tail-only delivery matches nothing; each shard verifies against its own list; shard records no longer share a key, so none clobbers another. The injective roster matching was greedy, and greedy rejects valid assignments. With transcript T1 containing blocks A+B and T2 containing only A, first-come claiming took T1 for A and reported B missing — a compliant repair permanently capped by transcript filename order. The claim set is now a maximum bipartite matching (Kuhn's augmenting paths), seeded on the edges where the transcript also opened the requirement's brief and extended over all verbatim edges, so a requirement reports missing only when no injective completion exists at all. A rules-free rebuild could silently strip the brief. The launch prompt only points at the brief, so rebuilding a rules-bearing role without --rules left the recorded launch byte-identical while the project rules vanished from the one file the agent treats as authoritative — every delivery check kept passing. writeBrief now refuses the downgrade at the single choke point both build paths pass through, with the escape hatch named (delete the record dir to start over deliberately). All three are mutation-checked: regressing the record to findings-free, the matching to greedy, or disabling the downgrade guard each turns its own test red. 704 review tests green. * docs(review): let the docs and comments claim only what the new record design does The round-13 review caught the drift this branch's own thesis forbids: two SKILL.md sentences still described the findings-free record the previous commit retired — an orchestrator reasoning from them would conclude a findings-less delivery still matches, precisely the bypass that commit closed. Both now state the new contract: the record is the exact printed block, keyed per findings digest, and a launch that drops the list matches no record. And the matching comment claimed more than Kuhn guarantees: phase-2 augmentation can displace an opened match onto an unopened edge to enlarge the matching, so an unread flag describes the assignment, not an impossibility. The comment now says so, and why cardinality is the right thing to maximize. * docs(review): finish retiring the findings-free record from every sentence that described it Round 15 found the three survivors round 13 missed — all in code, not SKILL.md: the findingsSection docstring (all three of its clauses false since the digest-key commit), the findings field doc ('Printed, not recorded'), and the --findings --help text, which told an operator the exact opposite of what the command now does. Each now states the new contract: the findings are part of the recorded prompt, keyed per digest, and a launch that drops them matches no record. Also from the same review: the plan-path substitution uses a function replacer, so a path containing $& or $` cannot be misrendered as a replacement pattern. Practically unreachable for .qwen/tmp paths; closed because it costs four characters. * docs(review): the actually-last sentence describing the findings-free record Round 16 counted one survivor of the sweep the previous commit's title claimed complete: the acceptsFindings jsdoc in agent-briefs.ts, present-tense, whose '(see runAgentPrompt)' pointed at a function whose own comment says the opposite. It now states the digest-key contract like its siblings, and a whole-tree grep for present-tense descriptions of the retired design comes back empty. * test(review): pin the idle and missing-chunk FIX lines to the remediation channel Round-18 review: the two remediation pushes added for the every-gap-has-a-FIX rule had no test of their own — deleting either failed nothing, leaving a body disclosure whose repair could silently vanish, the exact state the channel exists to prevent. The idle-plan test now asserts the relaunch FIX; the blind-plan test, whose chunks nobody reads, now asserts the chunks-nobody-read FIX beside the blind one. Both mutation-checked: deleting each push turns exactly one test red. * fix(review): quote the plan path in every printed repair, and test the executable shebang-less shape Round-21 review, three items. The plan path is now single-quoted at all seven sites that print it into a repair command — a workspace path containing a space split the copy-pasted FIX at the space, exactly the operator moment the lines exist for; the earlier uniformity deferral ends here, uniformly. PlanDiffResult declares prNumber/ownerRepo so a refactor away from the conditional spread cannot silently drop the fields the roster's Agent-0 requirement reads. And the filter gains the test its primary target deserved: an EXECUTABLE shebang-less .js (the desktop vendored bundle shape) is rejected by the header read itself — the existing 0644 fixture never reached that branch, so a regression in the byte read would have passed every test. * fix(review): shell-quote the plan path properly — an apostrophe is not rarer than a space Round-22 review: the bare '…' wrap from the previous commit closed at an embedded apostrophe, so ~/Documents/John's Projects broke where it had worked unquoted — one breakage class traded for another instead of both closed. A shared shellQuotePath (the same '\'' dance as utils/standalone-update.ts) now serves all six repair-printing sites, and a test drives verificationGaps from a plan under an apostrophe directory, asserting the escaped form and rejecting the naive wrap. * fix(review): quote the --file selector, un-dead the spawn guard, test the half-identity Round-24/25 reviews, four small items. selectorOf now shell-quotes the --file path — the same copy-paste contract the --plan quoting just earned, on the one selector that carries a path. RULES_MARKER moves above writeBrief's JSDoc, which it had been silently stealing. The check-build-status test's reject guard was dead (execFile always delivers string stdout, so an ENOENT resolved and the empty-stdout assertion passed on a script that never ran) — it now rejects on spawn-level errors, which carry string codes, while non-zero exits still resolve. And the roster's ownerRepo guard gets the independent test it never had: a plan with prNumber but no ownerRepo requires no Agent 0, since the brief builder cannot serve half an identity.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
/reviewnow proves it read the diff, builds every agent's prompt in code, and computes its own verdict. Seven defects were found across six rounds of dogfooding against real pull requests of this repo, each one measured against the harness's own per-agent transcripts — what each agent was actually launched with, and what it actually did — rather than against what the run said about itself.The verdict a small pull request got was a lie. Coverage was attributed by one question: did an agent whose launch prompt says
chunk N of Mmake a successful tool call? No Step 3A prompt ever says that — there, every dimension agent walks the whole diff. Run against a real 3A review whose twelve agents each opened the diff, walked both chunks and filed findings,check-coveragereturnedCoverage: 0/2 chunk(s) reviewed. 16 agent(s) ran; 16 did workfollowed byERROR: 2 chunk(s) were not reviewed. Nobody read those lines.Both sentences cannot be true.compose-reviewruns the same computation on the way to the verdict, so that review was capped away from Approve and told, in the body it would have posted to the pull request, that nobody had read it. Step 3A is the topology most pull requests get; the only reason it never blew up is thatcheck-coveragelived inside Step 3B and was never reached from 3A — two bugs cancelling. Coverage is now the intersection of two things the harness wrote down: the lines each agent was pointed at, and the fact that it opened the diff. Topology-blind.Half the fan-out was still being launched blind.
agent-promptbuilt the territory agents' prompts and left the rest to prose. All three whole-diff agents of a real 3B run — cross-file tracer, test-coverage matrix, build & test — got a prompt naming no diff file at all. The test-coverage matrix was told to "Read the diff chunks" and given no path to read them from; it read the post-change source instead, which on a deletion shows it nothing.Agent 0 was never launched, and nothing could tell. Every check in the skill asks a question of an agent that ran. An agent that does not run leaves no transcript to ask, so an omission is invisible precisely because it is an omission.
check-coveragenow derives a roster from the plan — which the caller does not write — and names every required agent that never ran, with the exactagent-promptcall that builds it.The prompt was rewritten on the way to the agent, and then it grew too big to carry at all. One run invoked
agent-promptcorrectly for all five chunks and then paraphrased what it printed — keeping theread_fileline (so every other check passed it) and dropping the rule against reciting a stock sentence, the half-read warning, and the project's review rules. The next, with the briefs welded into the launch prompt, delivered 2 893 characters of a 4 652-character prompt, then read the check's exit-3, concluded "the agents clearly did their job", skippedcompose-review, and filed an Approve it had written itself. A 17-chunk Step 3B review would have asked the orchestrator to paste 87 546 characters in one response. So the brief now goes where the diff already goes — on disk, read by the agent — and the launch prompt drops to ~500-800 characters. Whether the agent read its brief is a tool call the harness recorded, not a hope.The verdict was the model's to write.
compose-reviewcomputed the event and body, and the skill then told the orchestrator to "copy event/body verbatim into the review JSON" — a transcription, into a document the model writes, of a decision the CLI had already made.submitcomposes now. It takes the findings and derives everything that follows, including how many blockers there are, by counting the**[Critical]**/**[Suggestion]**prefixes of the comments actually attached rather than believing a number typed beside them. A payload carryingevent/bodyis refused.SKILL.mddrops 171 → 154 KB, and three things had never reached an agent at all. The briefs were duplicated between the skill and the code; the skill now keeps what each agent is for and drops what it is sent. Removing the prose surfaced that the Exclusion Criteria (the largest precision control in the review) had never been delivered to any agent — the skill states them at the end of the document and tells the orchestrator to apply them, and the agents do not read the document. Nor had the anchor rules (resolve-anchorswas downstream of a snippet nobody had told the agents how to produce), nor the severity calibration thatSEVERITY's own comment says is needed.Path-scoped rules:
.github/workflows. The dimensions are domain-blind by design, which holds until a file's failure modes are not guessable from reading it. A workflow is YAML, so it reads as configuration, and the reviewer who treats it as configuration misses every one of its attack classes — including apull_request_targetjob that checks out the contributor's head, which is the difference between a CI file and a remote code execution with the repository's write token. This repo runsqwen-autofix.yml, which posts to pull requests. A checklist is now appended to the brief of every code-reviewing agent whose territory actually contains a matching file.Why it's needed
The line running through every defect is the one this skill has now re-learned seven times: a rule stated in prose is a rule that will eventually not be followed, and the fix is to move it into code that can say no. The skill had already applied that to the review target, the posting gate, the verdict composition and the coverage report — and left it unapplied in the four places that turned out to matter most: the agents' prompts, the roster of who must exist, the terminal verdict, and the rules the agents were never actually sent.
Concretely, before this PR: a Step 3A review could not reach Approve and would have published a false claim; the only agents covering cross-file tracing, cross-chunk removed-behaviour and the test matrix had never opened the diff; issue fidelity could be skipped silently; and a run that skipped
compose-reviewcould print — and post — a verdict of its own.Two of the fixes were themselves caught by dogfooding and fixed in this PR, which is the point of the method:
Reviewer Test Plan
How to verify
Nothing here is verified by a review saying it went well. Each run below was checked afterwards by reading the harness's per-agent transcripts (
<projectDir>/subagents/<sessionId>/agent-*.jsonl, written at launch and not retconnable) and asking, per agent: is the prompt it was launched with byte-for-byte the one the CLI built, and did it open what it was pointed at?To reproduce any of them:
Unit tests for the whole surface:
npx vitest run --root packages/cli src/commands/review # 557 passed npx vitest run --root packages/core src/skills/bundled-skills npx tsc --noEmit -p packages/cli/tsconfig.jsonEvidence (Before & After)
The live bug, replayed against a real Step 3A review's transcripts (twelve agents that each opened the diff, walked both chunks and filed findings):
The prompt-rewrite check, replayed against the Step 3B run that paraphrased its prompts — all five chunk agents named, which nothing in the run could see before, because a paraphrase keeps the diff path:
The roster, replayed against the run that never launched issue fidelity:
The carrying problem, on a real 17-chunk PR: 17 chunk launch prompts × ~5 149 chars = 87 546 characters in one response → 14 789 after the briefs moved to disk.
End-to-end runs, all verified from the transcripts:
/review 6847/review 6766/review 6457return, but the 60-second reconnect timer is armed unconditionally forty lines away, so it logs "giving up" every minute and never retries./review --effort high(local) on a foreign project with a planted vulnerabilitypull_request_target+ PR-head checkout ✅ Critical (with thepostinstallattack chain);${{ pull_request.title }}intorun:✅ Critical (plus a$GITHUB_OUTPUTinjection I had not planted); unpinned third-party action ✅ Suggestion;actions/checkout@v4correctly not reported; a planted falsy-value bug ✅ Critical with an exact repro. Zero false positives; no noise on the non-workflow file./review 6788compose-review, was toldVerdict: Comment, and showed the user "Comment — downgraded from Approve (CI failing: route)" — the presubmit downgrade applied by code, on a run that did not post.The posting gate, at the wire:
Tested on
Environment (optional)
Repo build (
npm run build && npm run bundle) driven through a real TUI in tmux, modelqwen3.7-max. All dogfood runs against live pull requests of this repository; the foreign-project run against a throwaway git repo outside the tree.Risk & Scope
SKILL.mdintopackages/cli/src/commands/review/lib/agent-briefs.ts, so a change to what an agent is told is now a code change, not a prose edit. That is the intent — the prose did not survive the trip — but it does mean the skill and the code must not drift back apart. The skill now states which one is authoritative.--commentposting has not been driven against a live pull request. It is covered by unit tests and a dry run (above), andsubmitnow composes the verdict itself, but the end-to-end write path is unexercised. Also out of scope: the Step 4 verification agents and Step 5 reverse-audit agents still get orchestrator-written prompts and are not in the roster.submitpayload changes shape —{commit_id, comments, state}, with noevent/body, and a payload carrying them is refused rather than silently overruled.SKILL.mdis updated in the same commit; nothing outside this skill calls it.packages/web-shellthat was present onmainand brokenpm cifor every open pull request. Fixed onmainby fix(web-shell): remove duplicate useWebShellPortalRoot import in ChatEditor #6890 and merged into this branch; the failure was never this branch's.Linked Issues
None.
中文说明
这个 PR 做了什么
让
/review能证明自己读过 diff、用代码生成每一个 agent 的 prompt、自己算出裁决。在本仓库的真实 PR 上做了六轮 dogfood，找出七个缺陷——每一个都是拿 harness 自己记录的 subagent transcript（每个 agent 实际被什么 prompt 启动、实际做了什么）测出来的，而不是听这次 run 自述。小 PR 拿到的裁决是假的。 coverage 只认 launch prompt 里的
chunk N of M来归属，而 Step 3A 从来没有 agent 的 prompt 会说这句（那条路上每个维度 agent 都走整个 diff）。拿一次真实的 3A review（12 个 agent 全读了 diff、走完两个 chunk、产出真 finding）去跑闸门，它输出Coverage: 0/2 chunk(s) reviewed. 16 agent(s) ran; 16 did work，紧接着ERROR: 2 chunk(s) were not reviewed. Nobody read those lines.两句话不可能同时为真。而compose-review在通往裁决的路上跑的是同一段计算——所以那次完美的 review 被封顶、拿不到 Approve，并且在它准备发到 PR 上的正文里写着「没人读过」。3A 是绝大多数 PR 走的拓扑；它至今没爆的唯一原因是check-coverage埋在 Step 3B 里、3A 根本够不到它——两个 bug 互相掩护。现在 coverage 取的是 harness 写下的两件事的交集：agent 被指到的行，以及它真的打开过 diff。与拓扑无关。扇出的另一半仍然是瞎的。
agent-prompt只焊死了 territory agent 的 prompt，其余交给散文。一次真实 3B run 的三个 whole-diff agent——cross-file tracer、test-coverage matrix、build & test——prompt 里一个 diff 路径都没有。test-matrix 被要求「Read the diff chunks」却没给它路径，于是它跑去读了改后源码——而删除的行在改后文件里根本不存在。Agent 0 压根没启动，而且没人看得见。 skill 里每一个检查问的都是「跑过的 agent 怎么样」。没跑的 agent 不留 transcript，所以遗漏因为是遗漏所以不可见。现在
check-coverage从 plan 推出 roster（调用方写不了 plan），点名每一个该跑没跑的 agent，并附上生成它的确切命令。prompt 在送达途中被改写，然后大到根本搬不动。 一次 run 正确调用了
agent-prompt（5 个 chunk 全调了），然后把输出改写了——保留了read_file那行（于是所有其他检查都放行），删掉了「不要照念现成句子」、删掉了半读警告、把项目 rules 换成自己写的三句话。下一次，brief 被焊进 launch prompt 后，它只交付了 4652 字符里的 2893 字符，然后读到闸门的 exit-3，自我说服「the agents clearly did their job」，跳过compose-review，自己写了个 Approve。一个 17-chunk 的 3B review 会要求编排器在一个 response 里粘贴 87,546 字符——这种事不会发生。所以 brief 去了 diff 早就去的地方：磁盘上，由 agent 自己读；launch prompt 降到 ~500–800 字符。而「agent 到底读没读 brief」从一句但愿，变成 transcript 里一次可查的工具调用。裁决曾经是模型自己写的。
compose-review早就在算 event 和 body 了，然后 skill 让编排器「把 event/body 逐字抄进 review JSON」——把 CLI 已经做出的决定，转抄进一份模型自己写的文档。现在submit自己 compose。 它只收 findings，其余全部现算——包括有几个 blocker：从实际挂上去的 comment 的**[Critical]**/**[Suggestion]**前缀数出来，而不是相信旁边写的那个数字。payload 里还带event/body→ 直接拒绝。SKILL.md从 171KB 降到 154KB——而且发现三样东西从来没送到过 agent 手里。 brief 在 skill 和代码里各有一份；skill 现在只留「这个 agent 是干嘛的」，删掉「发给它的是什么」。删的过程逼出了：Exclusion Criteria 从未到达任何 agent（整个 review 最大的精度控制——skill 在文末写着它、叫编排器去 apply，而 agent 根本不读那份文档）；anchor 规则也没有（resolve-anchors一直在消费没人教过怎么产出的片段）；severity 校准也没有（SEVERITY常量的注释自己写着需要它）。按路径生效的规则：
.github/workflows。 维度是刻意「不认领域」的——直到某个文件的失效模式光看它本身猜不出来。workflow 是 YAML，读起来像配置；把它当配置读的人，会漏掉它的每一个攻击面——包括pull_request_target去 checkout 贡献者的 head，那是「一个 CI 文件」和「拿着仓库写权限的远程代码执行」之间的区别。本仓库自己就跑着qwen-autofix.yml，会往 PR 发东西。 现在这份清单只会附加给territory 里真的有该文件的 code-reviewing agent。为什么需要
贯穿每一个缺陷的是同一条、这个 skill 已经重新学会七次的教训：凡是用散文写下的规则，最终都会被无视；解法是把它变成能说「不」的代码。 skill 早就把这条用在了 review 目标、发布闸门、裁决合成和覆盖率报告上——唯独漏了最要紧的四处：agent 自己的 prompt、「该有谁」这份名单、终端里的裁决，以及那些从来没真正发给 agent 的规则。
这个 PR 里有两个修复是 dogfood 抓出我自己写的 bug，这正是这套方法的意义：
审查者验证计划
这里没有一件事是靠「这次 review 说自己跑得很好」来验证的。下面每一次 run 都是事后去读 harness 的 per-agent transcript（
<projectDir>/subagents/<sessionId>/agent-*.jsonl，启动那一刻写下、无法事后追改），逐个 agent 地问：它被启动时拿到的 prompt，是不是 CLI 构建的那份、一字不差？它有没有打开它被指向的东西？复现任何一次：
单元测试：
npx vitest run --root packages/cli src/commands/review # 557 通过 npx tsc --noEmit -p packages/cli/tsconfig.json那个实伤，拿真实 3A review 的 transcript 回放（12 个 agent 各自打开了 diff、走完两个 chunk、产出了真 finding）：
改写检测，拿那次真的改写了 prompt 的 3B run 回放——5 个 chunk agent 全部被点名（改写保留了 diff 路径，所以这在以前对整个 run 都是隐形的）。
roster，拿那次没启动 Agent 0 的 run 回放——
ERROR: 9 required agent(s) never ran — Agent 0: Issue fidelity …。搬运问题，在一个真实的 17-chunk PR 上：17 × ~5,149 = 87,546 字符 → brief 落盘后 14,789。
端到端 run（全部由 transcript 验证）：
/review 6847/review 6766/review 6457return了，但 60 秒的重连定时器在四十行外无条件被排上——于是它每分钟打印一次「giving up」，却永不重试。/review --effort high（本地）跑在一个外部项目上，里面种了已知漏洞pull_request_target+ checkout PR head ✅ Critical（连postinstall攻击链都对）；${{ pull_request.title }}注入run:✅ Critical（还多抓到一个我没种的$GITHUB_OUTPUT注入）；未 pin 的第三方 action ✅ Suggestion；actions/checkout@v4正确地没报；种下的 falsy 逻辑 bug ✅ Critical，带精确复现。零假阳性；非 workflow 文件上没有噪音。/review 6788compose-review，拿到Verdict: Comment，给用户显示 "Comment — downgraded from Approve (CI failing: route)"——presubmit 降级由代码施加，且该次 run 未发布。发布闸门，在出网前：
风险与范围
SKILL.md挪进了packages/cli/src/commands/review/lib/agent-briefs.ts，所以「改变一个 agent 被告知什么」现在是改代码、不是改散文。这正是意图（散文没能活着送到），但也意味着 skill 和代码不能再漂回两份。skill 里已经写明哪一份是权威。--comment真发布没有在活的 PR 上跑过。它有单元测试和 dry-run 覆盖（见上），submit现在也自己算裁决，但端到端的写路径未被驱动过。同样在范围外：Step 4 的验证 agent 和 Step 5 的 reverse-audit agent 仍然拿编排器手写的 prompt，且不在 roster 里。submit的 payload 换了形状——{commit_id, comments, state}，没有event/body，带了就直接拒绝（而不是静默覆盖）。SKILL.md在同一个 commit 里同步更新；这个 skill 之外没有别的东西调用它。packages/web-shell里一个重复 import——它在main上就存在，会让每一个开着的 PR 的npm ci失败。main已由 fix(web-shell): remove duplicate useWebShellPortalRoot import in ChatEditor #6890 修复并合入本分支；这个失败自始至终与本分支无关。关联 issue
无。