feat(review): build the Step 4 verifier and Step 5 reverse-audit prompts in code - #6942
Merged
Conversation
…pts in code The last change moved Step 3's agent prompts into code — the diff path, the brief, the roster, the delivery check — because a prompt the orchestrator retypes is one that drifts, and dogfooding proved every drift. Step 4 (verify) and Step 5 (reverse audit) were left composing their prompts from prose. They carry the densest methodology in the skill, and it is the methodology most costly to drop: - the verifier's one-way, quote-the-contradiction bar on rejecting a Critical, and its documented-intent gate — the exact rule a run skipped when it auto-posted a false "this PR now leaks AWS/GitHub tokens" Critical over a rationale three lines up in the diff; - the reverse auditor's gaps-only focus and substantive receipt. Both are now `qwen review agent-prompt --role verify` / `--role reverse-audit` — built in code, written to a brief file the agent reads, so the launch prompt the orchestrator carries is short and the method cannot be paraphrased away. The verifier is a new brief kind (`output: 'verdicts'`): it gets the Exclusion Criteria but not the finding format, because it rules on findings rather than filing them. A Step 3B reverse auditor takes `--chunk <id>` so it reads one chunk's range, not the whole 5 800-line diff — the range that made it the most context-starved agent in the pipeline. The orchestrator still supplies the one input that changes per launch — the shard's findings for the verifier, the cumulative finding list for the auditor — above the verbatim brief, exactly as a dimension agent gets its one-line change summary. NOT in this change, and called out so it is not mistaken for done: these agents run after Step 3D, so the coverage gate's roster does not reach them. Whether the verifier and auditor actually ran and read their briefs is not yet checked from the transcripts the way Step 3's agents are. That check is the next step.
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.
Hey @wenshao — the PR content looks solid, but the body is missing several required sections from the PR template:
## Why it's needed— present as "Why these two, specifically" but needs the template heading name## Reviewer Test Plan— "Verified" covers testing but is missing the required subsections:### How to verify,### Evidence (Before & After),### Tested ontable## Risk & Scope— not present (main risk, out of scope, breaking changes)## Linked Issues— not present (this is a follow-up to #6892 but no issue link)
Could you update the PR body to use the template headings? The substance is already there — it's just a matter of fitting it into the expected structure so reviewers can find what they need.
中文说明
@wenshao — PR 内容本身很扎实，但 body 缺少模板中几个必需的小节：
## Why it's needed— 内容在"Why these two, specifically"里，但需要用模板的标题名## Reviewer Test Plan— "Verified"涵盖了测试，但缺少必需的子节：### How to verify、### Evidence (Before & After)、### Tested on表格## Risk & Scope— 未提供（主要风险、不在范围内、破坏性变更）## Linked Issues— 未提供（这是 #6892 的后续，但没有 issue 链接）
请按照模板更新 PR body。内容已经有了，只是需要放到预期的结构里，方便 reviewer 查找。
— 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
|
Updated the body to follow the PR template — added |
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. Not reviewed: chunk 1, chunk 2 — no agent reported covering these; nobody read them.
— qwen3.7-max via Qwen Code /review
Comment on lines
+834
to
+836
| } else if (opts.chunk !== undefined) { | ||
| // A Step 3B reverse-audit agent owns one chunk's territory, the same as its | ||
| // Step 3 counterpart. Give it that chunk's range, not the whole diff — a |
Collaborator
There was a problem hiding this comment.
[Suggestion] The brief for reverse-audit includes diffReadingBlock (via buildRoleBrief at line 619), which emits read_file calls for every chunk in the plan — with the instruction "Walk it chunk by chunk." But the launch prompt here scopes a per-chunk reverse-audit agent to one chunk's range.
For --role reverse-audit --chunk 14 on a 20-chunk plan, the agent receives its brief first (20 chunk reads) and then the launch prompt (1 chunk read). The brief's "walk it chunk by chunk" undermines the per-chunk scoping this code was designed to enforce.
Failure scenario: A Step 3B reverse-audit agent follows its brief (which it's told is authoritative) and reads all 20 chunks, consuming the context budget the per-chunk design exists to preserve.
Suggested change
| } else if (opts.chunk !== undefined) { | |
| // A Step 3B reverse-audit agent owns one chunk's territory, the same as its | |
| // Step 3 counterpart. Give it that chunk's range, not the whole diff — a | |
| } else if (opts.chunk !== undefined) { | |
| // A Step 3B reverse-audit agent owns one chunk's territory, the same as its | |
| // Step 3 counterpart. Give it that chunk's range, not the whole diff — a | |
| // reverse auditor handed a 5 800-line diff is the most context-starved agent | |
| // in the pipeline, on exactly the PRs where the reverse audit matters most. | |
| // NOTE: when a role gets scoped reads here, its brief should NOT also include | |
| // the full diffReadingBlock — consider gating diffReadingBlock in buildRoleBrief | |
| // on the role not having chunk-scoped reads. |
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Good catch — this was a real bug, and it happened to be found by this PR's own reverse-audit agent dogfooding the change. Fixed in 0db5cd9: diffReadingBlock now takes an optional chunk id and, when given one, reads that chunk alone (dropping the "walk it chunk by chunk" line), and buildRoleBrief threads the chunk through — exactly as an invariant agent's brief is already scoped to its one file. Brief and launch prompt now agree on the one chunk. Regression test: the scoped brief reads chunk 14 only, the whole-diff (3A) brief still walks every chunk.
Comment on lines
+899
to
+901
| if (hasChunk && args.role !== 'reverse-audit') { | ||
| bad( | ||
| `--chunk combines with --role only for reverse-audit (a Step 3B per-chunk ` + |
Collaborator
There was a problem hiding this comment.
[Suggestion] The chunk-acceptance guard hardcodes args.role !== 'reverse-audit' with no corresponding declarative field on the Brief interface. A future maintainer adding a per-chunk role would need to know to update this guard, with no signal from the type system.
Failure scenario: Someone adds a new per-chunk role, creates a valid BRIEFS entry, calls --role new-role --chunk 5, and gets rejected with an error naming the old special case.
Consider adding an acceptsChunk?: boolean field to Brief, setting it on the reverse-audit entry, and changing this guard to if (hasChunk && !BRIEFS[args.role as RoleId]?.acceptsChunk).
— qwen3.7-max via Qwen Code /review
Comment on lines
+841
to
+843
| throw new Error( | ||
| `agent-prompt: --role ${role} --chunk ${opts.chunk}: the plan has no ` + | ||
| `chunk ${opts.chunk} (it has ${allChunks.map((x) => x.id).join(', ')}).`, |
Collaborator
There was a problem hiding this comment.
[Suggestion] The error path for a non-existent chunk id has no test. The happy-path test at line 425 passes chunk: 14 which exists in the PLAN fixture; no test exercises this throw.
Failure scenario: If the error message were wrong (listed wrong available chunks, or the condition were inverted), no test would catch it.
Consider adding:
it('rejects --role reverse-audit --chunk N when the plan has no such chunk', () => {
expect(() =>
buildRoleLaunchPrompt(PLAN, 'reverse-audit', '/t/ra.brief.md', { chunk: 999 }),
).toThrow(/the plan has no chunk 999/);
});— qwen3.7-max via Qwen Code /review
Comment on lines
+959
to
+963
| key = args.file | ||
| ? `${role}--${args.file}` | ||
| : typeof args.chunk === 'number' | ||
| ? `${role}--chunk-${args.chunk}` | ||
| : role; |
Collaborator
There was a problem hiding this comment.
[Suggestion] The record key derivation ${role}--chunk-${chunk} is untested through runAgentPrompt. Existing tests call buildRoleLaunchPrompt directly (bypassing the guard and key derivation) or test other roles' keys.
Failure scenario: If the ternary were reordered, the delivery check downstream would fail to find the recorded prompt by key and silently skip the coverage check.
Consider adding a handler-level test that calls the handler with { plan, role: 'reverse-audit', chunk: 14 } and asserts the record has key reverse-audit--chunk-14.
— qwen3.7-max via Qwen Code /review
Comment on lines
+425
to
+427
| it('accepts --role reverse-audit --chunk N — the one legal role+chunk combo', () => { | ||
| // A Step 3B reverse-audit agent owns one chunk's territory. The guard lets that | ||
| // one through, and the launch prompt reads exactly that chunk's range — not the |
Collaborator
There was a problem hiding this comment.
[Suggestion] This acceptance test calls buildRoleLaunchPrompt directly, bypassing the guard in runAgentPrompt. No test confirms the guard actually allows --role reverse-audit --chunk N through to the prompt-building step.
Failure scenario: A refactor of the guard logic (e.g., back to count-based check) would still pass the rejection tests and this output test, but at runtime runAgentPrompt would reject the one valid combo — and Step 3B reverse-audit would silently never launch.
Consider adding a test through the handler:
expect(() =>
(agentPromptCommand.handler as (a: unknown) => void)({
plan: planPath,
role: 'reverse-audit',
chunk: 14,
}),
).not.toThrow();— qwen3.7-max via Qwen Code /review
Dogfooding this PR, the /review skill's own reverse-audit agent found a defect in it. A Step 3B reverse auditor is launched `--role reverse-audit --chunk N` so it reads one chunk's range, not the whole diff — that scoping is the whole point: a reverse auditor handed a 5 800-line diff is the most context-starved agent in the pipeline, on exactly the PRs where the reverse audit matters most. The launch prompt scoped correctly. The brief did not: `buildRoleBrief` gave every diff-reading role the full `diffReadingBlock`, which emits a read for every chunk in the plan and says "walk it chunk by chunk". The agent is told its brief is authoritative and that nothing in the launch message replaces it — so it would read the whole diff the `--chunk` design exists to spare it. The brief and the launch prompt disagreed on the one thing the feature is about. `diffReadingBlock` now takes an optional chunk id and, when given one, reads that chunk alone — the same range the launch prompt reads — and drops the "walk it chunk by chunk" instruction. `buildRoleBrief` threads the chunk through, exactly as an invariant agent's brief is already scoped to its one file. Which role may be launched per-chunk is now declared on the brief (`acceptsChunk`), not hardcoded in the command guard as `role !== 'reverse-audit'`. A new per-chunk role is a data change in agent-briefs, and the guard reads the same field the brief builder does. Tests added for the fix and the coverage the review flagged: the scoped brief reads one chunk not all; the handler accepts the one legal role+chunk combo and keys its record `reverse-audit--chunk-N`; a non-existent chunk id is rejected by name rather than emitting an unusable read.
Collaborator
Author
|
Thanks for the review — all five addressed in 0db5cd9. Notably the substantive one (F1) was caught by this PR's own reverse-audit agent while dogfooding the change, which is the strongest signal the feature works.
589 review tests pass (was 586), review |
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. Not reviewed: chunk 1, chunk 2, chunk 3 — no agent reported covering these; nobody read them.
— qwen3.7-max via Qwen Code /review
Comment on lines
+939
to
+940
| `--chunk combines with --role only for reverse-audit (a Step 3B per-chunk ` + | ||
| `auditor); role "${args.role}" does not take --chunk.`, |
Collaborator
There was a problem hiding this comment.
[Suggestion] The error message hardcodes "reverse-audit" while the guard uses BRIEFS[role]?.acceptsChunk — a data-driven flag. The acceptsChunk JSDoc says "a new per-chunk role is a data change here, not a name hardcoded in the guard" but this error message violates that promise.
Concrete cost: if a future role sets acceptsChunk: true, the guard correctly allows it through, but if someone passes --chunk with a different non-accepting role, the error still says "only for reverse-audit" — misleading the caller about which roles accept chunks.
Suggested change
| `--chunk combines with --role only for reverse-audit (a Step 3B per-chunk ` + | |
| `auditor); role "${args.role}" does not take --chunk.`, | |
| `--chunk combines with --role only for roles that set acceptsChunk ` + | |
| `(currently: reverse-audit); role "${args.role}" does not take --chunk.`, |
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in a3e38f4. Good consistency catch — the guard reads acceptsChunk but the message still said "only for reverse-audit". It now derives the list from the briefs (Object.keys(BRIEFS).filter(r => BRIEFS[r].acceptsChunk)), so the message can never allow a role the text denies.
Comment on lines
+997
to
+1001
| key = args.file | ||
| ? `${role}--${args.file}` | ||
| : typeof args.chunk === 'number' | ||
| ? `${role}--chunk-${args.chunk}` | ||
| : role; |
Collaborator
There was a problem hiding this comment.
[Suggestion] The record key ternary gives args.file precedence over args.chunk, but --file is not validated for non-invariant roles. Passing --role reverse-audit --chunk 14 --file foo.ts (which the guard allows) produces key reverse-audit--foo.ts instead of reverse-audit--chunk-14, overwriting a prior recording of the same file and defeating the delivery check.
Concrete cost: the guard validates --chunk against acceptsChunk but leaves --file unvalidated — an asymmetric gap. Any non-invariant role that receives --file silently gets a file-keyed record when the brief actually reads the whole diff (or a chunk), producing a key that implies file-scoping when the content does not match.
Consider either rejecting --file on non-invariant roles in the guard, or prefixing the namespaces (${role}--file--${file} vs ${role}--chunk--${chunk}) so they cannot collide.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in a3e38f4 — real asymmetry. --file is now rejected on any non-invariant role (and on --whole-diff), symmetric with the --chunk/acceptsChunk guard. --role reverse-audit --chunk 14 --file foo.ts no longer keys reverse-audit--foo.ts over a file the agent never reads; it is rejected. Test added.
Comment on lines
+863
to
+866
| const rangeOf = (c: DiffChunk) => ({ | ||
| offset: c.startLine - 1, | ||
| limit: c.endLine - c.startLine + 1, | ||
| }); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The chunk-to-{offset, limit} mapping is now computed in three places in this file: diffReadingBlock (inline .map), this rangeOf helper, and invariantDiffRange. All three compute offset = startLine - 1, limit = endLine - startLine + 1.
Concrete cost: if the formula needs to change (off-by-one fix, adapting to a read_file behavior change), the edit must land in three sites. Lifting rangeOf to module scope where diffReadingBlock could share it is the one-line cleanup.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in a3e38f4. It was actually five sites, not three (buildChunkLaunchPrompt and invariantFileBlock too). All five now call one module-scope diffWindow(startLine, endLine) helper, so an off-by-one fix lands once. Tests prove the emitted offsets/limits are unchanged.
| // orchestrator retypes each run. The rules pinned here are the ones a paraphrase | ||
| // would have dropped — and one of them (the documented-intent gate) is the exact | ||
| // rule a real run skipped when it auto-posted a false "leaks tokens" Critical. | ||
| describe('verify and reverse-audit briefs — the Step 4/5 methodology, in code', () => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The verify role is tested via buildRoleBrief and buildRoleLaunchPrompt directly, but never through agentPromptCommand.handler. The reverse-audit+chunk handler test covers the handler's I/O path (brief write, record key, prompt record), but a handler-level call with { role: 'verify' } would exercise the output: 'verdicts' branch of tail() through file I/O and confirm the record key is 'verify'.
Concrete cost: the handler's role branch is generic and low-risk, but it is the one new role whose full handler path is not driven end-to-end.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in a3e38f4. Added a handler-level test driving { role: "verify" } end-to-end: it does not throw, keys the record verify, and the brief it writes carries the verdict branch of tail() (Exclusion Criteria, no finding format / no Anchor).
…window math
A second reverse-audit pass — the /review skill run on the previous commit — found
four gaps in it. All four are addressed here.
The guard was made data-driven (`!BRIEFS[role]?.acceptsChunk`) but its error message
still said "only for reverse-audit". If a second role ever sets `acceptsChunk`, the
guard would allow it while the message denied it. The message now names the set it
read from the briefs, so it cannot drift from what the guard enforces.
The record key derived from `--file` with no guard on it, while `--chunk` was guarded.
`--role reverse-audit --chunk 14 --file foo.ts` was accepted and keyed
`reverse-audit--foo.ts` — a file the agent never reads, colliding with and masking a
real file-keyed record. `--file` is the invariant agent's one scoping input; it is now
rejected on any role that is not an invariant agent, and on `--whole-diff`, closing the
asymmetry with the `--chunk` guard.
The 1-based-line-range → `{offset, limit}` arithmetic (`startLine - 1`,
`endLine - startLine + 1`) was written out at five sites. An off-by-one fix, or a
change in how `read_file` windows, would have had to land in all five. It is now one
`diffWindow(startLine, endLine)` helper the five call.
Tests: the message names the derived set; `--file` is rejected on a non-invariant role
and on `--whole-diff`; and the verify role — the one new role whose full handler path
was untested — is driven end-to-end through the handler, keyed `verify`, with the
verdict branch of its brief (Exclusion Criteria, no finding format).
Collaborator
Author
|
Round 2 of the self-review (the
592 review tests pass (589→592), review Two rounds of the skill reviewing its own PR have now found and fixed nine issues in it — one a real bug (the per-chunk brief F1), the rest precision and coverage. That recursion is the strongest evidence the coverage-in-code approach works: the review can no longer quietly skip its own agents, so it catches itself. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Thanks for the PR! Template looks good ✓ — all required sections present, bilingual body included. Problem: this is a Direction: aligned. This is a direct follow-up to #6892 which established the pattern of encoding review agent prompts in code. The same reasoning applies — the review pipeline's trustworthiness depends on methods that survive paraphrase, and code is the only format that cannot be paraphrased away. Size: 272 production logic lines ( Approach: scope feels right. Two new roles added to the existing Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必要章节均存在，包含双语正文。 问题：这是 方向：对齐。这是 #6892 的直接后续，该 PR 建立了将 review agent prompt 编码进代码的模式。同样的推理适用 — review 流水线的可信度取决于能经受住转述的方法，而代码是唯一无法被转述掉的格式。 规模：272 行生产逻辑（ 方案：范围合理。两个新 role 加入现有的 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Code ReviewIndependent proposal before reading the diff: to encode the Step 4 verifier and Step 5 reverse-audit prompts in code, I'd add the two roles to The PR does exactly this, with several good design choices:
No correctness bugs, no security issues, no convention violations. The code follows the established pattern from #6892 exactly. Test ResultsUnit tests for the changed file: 90/90 passing in TypeScript: clean ( Full review suite: 591/592 passing. The 1 failure is in CI: all relevant checks green (Test ubuntu-latest Node 22.x ✅, web-shell E2E Smoke ✅, Classify PR ✅, route ✅). 中文说明代码审查在读 diff 之前的独立方案：要将 Step 4 verifier 和 Step 5 reverse-audit 的 prompt 编码进代码，我会把两个 role 加入 PR 完全做到了这些，并有几个好的设计选择：
没有正确性 bug，没有安全问题，没有违反项目约定。代码完全遵循 #6892 建立的模式。 测试结果变更文件的单测：90/90 通过（ TypeScript：干净（ 完整 review 套件：591/592 通过。1 个失败在 CI：所有相关检查绿色（Test ubuntu-latest Node 22.x ✅，web-shell E2E Smoke ✅，Classify PR ✅，route ✅）。 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 — Clean across every stage; would merge without hesitation. This is a focused, well-motivated follow-up to #6892 that continues the pattern of encoding review agent methods in code. The two agents being encoded — the verifier and the reverse auditor — are the ones whose method decides whether the review is trustworthy, and whose method is most costly to drop under context pressure. The dogfood evidence (a run auto-posting a false "leaks tokens" Critical because it paraphrased away the documented-intent gate) makes the case concrete. The implementation matches what I'd propose independently, with better design choices: declarative The prior CHANGES_REQUESTED on this PR was a template check (Stage 1a) that no longer applies — the PR body now has all required sections. Approving. ✅ 中文说明信心度：5/5 — 各阶段均干净；毫不犹豫即可合并。 这是 #6892 的一个聚焦、动机充分的后续，延续了将 review agent 方法编码进代码的模式。被编码的两个 agent — verifier 和 reverse auditor — 是其「方法」决定 review 是否可信的 agent，也是上下文压力下最不能丢的方法。dogfood 证据（一次运行因为转述掉了 documented-intent gate 而自动发了假的"泄露 token"Critical）使这一论点具体化。 实现与我独立提出的方案一致，并有更好的设计选择：声明式 此 PR 之前的 CHANGES_REQUESTED 是一个模板检查（Stage 1a），现已不再适用 — PR 正文现在包含所有必要章节。 批准。✅ — 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. ✅
doudouOUC
approved these changes
Jul 15, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Review summary — APPROVE
Reviewed at a3e38f40d. A clean, well-scoped follow-up to #6892: it moves the Step 4 (verify) and Step 5 (reverse-audit) methodology out of SKILL.md prose and into code, exactly mirroring what #6892 did for Step 3. No Criticals.
Verified locally
packages/cli→npx vitest run src/commands/review/agent-prompt.test.ts— 90/90 pass.- Full
src/commands/reviewsuite — 587 pass. The 5 failures are all intest-efficacy*(a macOSsafeRmWithinsymlinkrmSyncquirk + git-worktree integration timeouts) and are untouched by this PR. - The four changed files typecheck clean (the remaining
tscerrors are stale core.d.tsfrom an unbuilt tree — e.g.unquoteCStylePath, which is exported in core source).
Correctness spot-checks (all pass)
- Roster is unaffected.
requiredAgents()builds the roster from explicitadd()calls, not by iteratingBRIEFS, so the newverify/reverse-auditroles do not leak into the Step 3D coverage gate — matches the PR description. - Chunk-window math is right: chunk 14 (lines 4025–4200) →
offset=4024, limit=176. - The verify-brief tests bind to real constants (
EXCLUSIONS= "What is NOT a finding";FINDING_FORMAT= "Anchor:"), so "Exclusion Criteria yes, finding-format no" is a genuine assertion, not a tautology. - Guard matrix behaves:
--role reverse-audit --chunk Nallowed;--role verify --chunk Nrejected;--filerejected on non-invariant roles and on--whole-diff; a non-existent chunk id throws by name (in the brief builder first). - No information lost in
SKILL.md: worktreeworking_dirrule, Agent 0 issue-evidence hand-off, and the "remove rejected / low-confidence goes to terminal only" paragraph are all preserved.
Non-blocking notes
- One inline Suggestion below: the bare
--chunkmode is the one primary mode that silently ignores a stray--fileinstead of rejecting it. No harm today (the key ischunk-<id>), purely a consistency gap with the guards this PR just tightened. verifystill reads the whole diff per shard, and verify shards share one record key — both already called out as out-of-scope in the PR body, with no functional impact yet.
Nice dogfooding loop: every earlier /review bot Suggestion is closed by a later commit with a regression test. LGTM.
| `role "${role}" does not take --file.`, | ||
| ); | ||
| } | ||
| } else if (!hasChunk) { |
Collaborator
There was a problem hiding this comment.
[Suggestion — non-blocking] This bare---chunk branch is the one primary mode that doesn't reject a stray --file. --chunk 14 --file foo.ts (no --role) is accepted and --file is silently dropped, whereas a3e38f4 made both the role mode and --whole-diff reject it loudly.
No real harm: the record key in this branch is chunk-<id> and never incorporates --file, so there is no collision/masking risk (the thing the --file guard was closing for the role case). It is purely a consistency gap — a typo'd scoping flag is silently ignored here instead of producing the clear error the other two modes now give.
Could close the asymmetry with if (hasFile) bad('--chunk owns a whole chunk; it does not take --file.'), or defer to a follow-up.
This was referenced Jul 15, 2026
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
Follow-up to #6892. That PR moved the
/reviewskill's Step 3 agent prompts into code — the diff path, the brief, the roster, the delivery check — so the orchestrator no longer retypes them. This PR does the same for Step 4 (verify) and Step 5 (reverse audit): both are now built byqwen review agent-prompt --role verify/--role reverse-auditand written to a brief file the agent reads, instead of being composed from prose inSKILL.md. The verifier is a new brief kind (output: 'verdicts') that receives the Exclusion Criteria but not the finding format, because it rules on findings rather than files them. A Step 3B reverse auditor takes--chunk <id>so it reads one chunk's line range instead of the whole diff.SKILL.mdStep 4/5 now call the commands and supply only the one input that changes per launch (the shard's findings for the verifier, the cumulative finding list for the auditor).Why it's needed
The verifier and reverse auditor are the two agents whose method — not just their target — decides whether the review is trustworthy, and it is the method most costly to drop under context pressure. The verifier carries a one-way bar on rejecting a Critical (quote the code that contradicts it, else downgrade rather than drop) and a documented-intent gate (a rationale in the diff does not make a real harm safe) — the exact rule a dogfood run skipped when it auto-posted a false "this PR now leaks AWS/GitHub tokens" Critical over a rationale sitting three lines up in the same diff. The reverse auditor carries a gaps-only focus and a substantive receipt requirement so it proves it read the code rather than restating the diff. When those rules live in prose the orchestrator paraphrases, they are the first thing to erode; in code they cannot be paraphrased away. The
--chunkrange for 3B fixes the auditor otherwise being the most context-starved agent in the pipeline (it was reading the whole 5 800-line diff).Reviewer Test Plan
How to verify
Run the review unit suite and the CLI surface directly.
cd packages/cli && npx vitest run src/commands/review→ 586 passing across 29 files;npx tsc --noEmit -p packages/cli/tsconfig.jsonfiltered tocommands/review→ clean. To exercise the new command surface:qwen review agent-prompt --plan <plan.json> --role verifyemits a short launch prompt plus averify.brief.mdcarrying the reject-Critical bar and Exclusion Criteria (no finding format);--role reverse-audit --chunk <id>restricts the auditor to that chunk's offset/limit range; the illegal combination--role verify --chunk Nis rejected with--chunk combines with --role only for reverse-audit.Evidence (Before & After)
Non-UI change (a CLI subcommand and skill prose), so no before/after screenshots. Behavioral evidence from a dogfood on a real 3A review: the orchestrator adopted the commands (
--role verify×5,--role reverse-audit×6 across rounds, with no fallback to hand-written prompts), and every launch that matched a written brief opened that brief in the harness transcript (verify + reverse auditor, 3/3). Before this PR the same two steps were prose inSKILL.mdthat the orchestrator paraphrased each run.Tested on
Environment (optional)
Linux dev box; repo-built CLI (
npm run build && npm run bundle) driven under tmux against a live PR. Unit suite vianpx vitest.Risk & Scope
/reviewskill (one CLI subcommand's argument handling + brief content, andSKILL.mdStep 4/5 text). No runtime path outsideqwen reviewis touched. The verifier's stricter reject-Critical bar could in principle keep a genuinely-wrong Critical that a reviewer would otherwise have dropped — mitigated by the downgrade-not-drop rule and by this being the intended, documented policy.agent-promptinvocations are unchanged.Linked Issues
Follow-up to #6892 (no issue to close).
中文说明
这个 PR 做了什么
#6892 的后续。那个 PR 把
/reviewskill 的 Step 3 agent prompt 搬进了代码——diff 路径、brief、roster、投递核对——让 orchestrator 不再手打它们。这个 PR 对 Step 4(verify)和 Step 5(reverse audit)做同样的事:两者现在都由qwen review agent-prompt --role verify/--role reverse-audit构建、写进一个 brief 文件让 agent 去读,而不再是SKILL.md里用散文拼出来的。verifier 是一种新的 brief 类型(output: 'verdicts'):它拿到 Exclusion Criteria,但不拿 finding format——因为它是裁决 finding,不是提交 finding。Step 3B 的 reverse auditor 接受--chunk <id>,只读某个 chunk 的行范围,而不是整份 diff。SKILL.mdStep 4/5 现在调用命令,只提供每次 launch 唯一变化的那份输入(verifier 拿本分片的 findings,auditor 拿累积的 finding 列表)。为什么需要
verifier 和 reverse auditor 是两个「方法」决定 review 可不可信的 agent——不只是「看哪里」,而是「怎么看」,也是上下文压力下最不能丢的方法。verifier 携带推翻 Critical 的单向高门槛(必须引用与之矛盾的代码,否则降级而不是删掉)和一道 documented-intent gate(diff 里写了 rationale 不代表真实危害就安全了)——这正是某次 dogfood 跳过的规则:它自动发了一条假 Critical 说「这个 PR 现在会泄露 AWS/GitHub token」,而解释就在同一份 diff 往上三行。reverse auditor 携带「只找缺口」的聚焦和 substantive receipt 要求,以证明自己真读了代码、而非复述 diff。这些规则一旦活在「orchestrator 会转述」的散文里,就是第一个被磨掉的东西;放进代码,就无法被转述掉。3B 的
--chunk范围修掉了 auditor 此前是整条流水线里上下文最匮乏的 agent 这个问题(它原本要读整份 5 800 行的 diff)。Reviewer 测试计划
如何验证
运行 review 单测套件并直接验证 CLI 表面。
cd packages/cli && npx vitest run src/commands/review→ 29 个文件 586 通过;npx tsc --noEmit -p packages/cli/tsconfig.json过滤到commands/review→ 干净。验证新命令表面:qwen review agent-prompt --plan <plan.json> --role verify输出一段简短的 launch prompt 外加一份verify.brief.md,内含推翻 Critical 的门槛和 Exclusion Criteria(不含 finding format);--role reverse-audit --chunk <id>把 auditor 限制在该 chunk 的 offset/limit 范围;非法组合--role verify --chunk N会被拒绝并提示--chunk combines with --role only for reverse-audit。证据(前后对比)
非 UI 改动(一个 CLI 子命令 + skill 散文),因此没有前后截图。来自真实 3A review dogfood 的行为证据:orchestrator 采用了这两个命令(
--role verify×5、--role reverse-audit×6 多轮,没有回退到手写 prompt),且每一个能对上落盘 brief 的 launch,在 harness transcript 里都真的打开了那份 brief(verify + reverse auditor,3/3)。在这个 PR 之前,这两步是SKILL.md里的散文,orchestrator 每次运行都会转述。测试平台
环境(可选)
Linux 开发机;仓库构建的 CLI(
npm run build && npm run bundle)在 tmux 下针对真实 PR 运行。单测通过npx vitest。风险与范围
/reviewskill(一个 CLI 子命令的参数处理 + brief 内容,以及SKILL.mdStep 4/5 文本)。不触及qwen review之外的任何运行时路径。verifier 更严的推翻-Critical 门槛,原则上可能保留一条 reviewer 本会删掉的、确实错误的 Critical——由「降级而非删除」规则缓解,且这本就是有意为之、有文档记录的策略。agent-prompt调用不变。关联 Issue
#6892 的后续(无需关闭的 issue)。