feat(review): build every Step 5 auditor of a round in one call, identity first - #7150
Merged
Conversation
…tity first Post-merge dogfood named the last hand-assembly that survives the pipeline, with the transcript to prove both of its shapes. The per-chunk reverse-audit form asked for one build-and-capture round trip per chunk per round, and the run answered with `for i in 1..10; do agent-prompt … | head -5; done` — it SAMPLED each build instead of capturing it, never possessed a single full prompt, hand-reconstructed all ten launches, and every one was flagged rewritten: a full repair round spent recovering from a shortcut that saved nothing. `--all-chunks` is the same medicine --roster was for Step 3: one call, one labelled numbered block per chunk, an end-of-round marker for the truncation self-check, records keyed per chunk per findings digest, and a header that says outright what the for-loop got wrong — never sample this output; the text IS the deliverable. SKILL.md's Step 5 3B carries the call, the redirect-and-page protocol, and the sampling ban with the measured cost attached. And the one edit that same run made to a prompt it fully possessed was at the head: it swapped the verifier's identity line for a context sentence of its own. The findings fold now leaves the identity line FIRST — a context wrap lands above it instead of replacing it, and the delivery check keeps its anchor. The record remains the exact printed prompt either way. The findings-free-tail attack pin is rebuilt for the new layout (a launch carrying the block without the findings section still matches no record), and the batch is mutation-checked three ways: cross-wiring every block to one chunk's ranges, recording less than what was printed, and demoting the identity line each turn exactly one test red — the first of those needed a two-chunk assertion, because checking only the first block cannot see a batch built entirely from it.
qwen-code-ci-bot
requested changes
Jul 18, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Hi @wenshao — thanks for the PR! The code changes look interesting, but the PR body doesn't follow the pull request template. Could you update it to match?
Missing sections:
- ## What this PR does (current heading is
## What) - ## Why it's needed — motivation / user-facing benefit
- ## Reviewer Test Plan — with
### How to verify,### Evidence (Before & After), and### Tested ontable - ## Risk & Scope — main risk, out-of-scope, breaking changes
- ## Linked Issues — closes/references
The template is there to help reviewers find what they need quickly and to ensure consistency. Once updated, re-run triage with @qwen-code /triage.
中文说明
@wenshao 你好——感谢 PR！代码改动看起来很有意思，但 PR 正文没有按照 PR 模板 填写，能否更新一下？
缺少的部分：
- ## What this PR does（当前标题是
## What） - ## Why it's needed — 动机 / 用户可见的价值
- ## Reviewer Test Plan — 包含
### How to verify、### Evidence (Before & After)、### Tested on表格 - ## Risk & Scope — 主要风险、不在范围内的内容、破坏性变更
- ## Linked Issues — 关联的 issue
模板的目的是帮助 reviewer 快速找到需要的信息，并保持一致性。更新后可以用 @qwen-code /triage 重新跑一次。
— Qwen Code · qwen3.7-max
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: Observed — the dogfood run transcript ( Direction: Aligned. The Size: ~213 production logic lines in Approach: Scope feels right. The three guard clauses ( 中文说明感谢贡献！ 模板完整 ✓ 问题： 已观测到 — dogfood run 记录（ 方向： 对齐。 规模： 方案： 范围合理。四个新提交进一步加强： — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Code ReviewCode review: clean pass on the updated diff, no blockers. The four new commits (
Guard clauses remain comprehensive: Reuse check: Tests127/127 pass (up from 117 in the previous review). New tests since last review:
CI: all required checks green on 中文说明代码审查代码审查：对更新后的 diff 无阻塞问题。 四个新提交在原实现基础上增加了大量加固：
测试127/127 通过（比上次审查的 117 增加 10 个）。新增测试覆盖：rules-key 碰撞、空 plan 守卫、chunkIdsProblem 验证、 CI 在 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 — Clean across every stage; well-motivated fix for a documented failure mode with tight implementation, thorough hardening, and comprehensive tests. This PR solves a real problem with evidence from a dogfood run. The The code is straightforward for its domain — every guard clause, every extracted helper, every test assertion traces back to a documented failure. Nothing speculative, nothing over-engineered. If I had to maintain this in six months, the comments and test names would tell me exactly why each piece exists. Approving. ✅ 中文说明置信度：5/5 — 各阶段均无问题；针对已记录失败模式的精确修复，实现紧凑，加固充分，测试全面。 这个 PR 解决了 dogfood run 中有证据的真实问题。 批准。✅ — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
previously approved these changes
Jul 18, 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. ✅
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. |
wenshao
commented
Jul 18, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen-latest-series-invite-beta-v92 via Qwen Code /review
Review caught the asymmetry: the single-chunk path throws on a chunk it cannot use, while the batch filter silently emptied the list and printed '0 auditors required this round' with a valid end marker, recording nothing — a zero-coverage round wearing a receipt. Same corruption, same refusal now, with a test driving a plan whose every chunk id is a string.
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. 1 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output.
— qwen3.7-max via Qwen Code /review
wenshao
commented
Jul 18, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— Codex GPT-5 via Qwen Code /review
…d rules-blind brief keys Three holes in the Step 5 batch, all caught in review: - A malformed or duplicated chunk id shrank the round instead of stopping it: [13, "x", 15] printed a complete-looking two-auditor round with one territory silently gone, and a duplicate id keyed two territories to one record -- the second never audited, under an end marker that says the round is whole. The batch now refuses exactly what coverage's readPlan refuses, via one shared definition (chunkIdsProblem in diff-plan.ts), before writing any brief, record or stdout block. - --all-chunks was validated only inside the findings-role branch, so --roster --all-chunks, --whole-diff --all-chunks, a bare --chunk 13 --all-chunks, and --all-chunks alone all ran the other mode and exited 0 with the batch silently dropped. Every combination is now ruled on at the primary-mode boundary. - The findings digest keyed the brief by findings alone: a round rebuilt with corrected rules kept its key, so the corrected brief landed at the same path an earlier agent had already opened, and delivery credited that old transcript with reading rules it never saw. The digest now hashes a JSON tuple of the findings and the effective rules, so a rules change is a fresh key, a fresh brief, a fresh launch. Plus the two review-suggested regression pins: an empty findings file still builds one auditor per chunk, each carrying the early-round framing, and --rules lands in every brief of the batch.
yiliang114
previously approved these changes
Jul 18, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
LGTM. Well-structured hardening that addresses real dogfooded failures.
What's good
--all-chunksfollows the--rosterpattern exactly — one call, labelled numbered blocks, end marker, same truncation self-check. Thefor i in …; do agent-prompt … | head -5; donesampling failure that motivated this is well-documented in the comments and tested against.findingsDigestincluding rules is a real fix — a round rebuilt with corrected rules kept the old key, so the corrected brief landed at the same path the first-round agent had already opened. The JSON tuple[content, rules ?? null]correctly distinguishes no-rules from empty-rules.foldFindingsidentity-first is a clever defense — the one hand-edit a real run made to a fully possessed prompt was replacing the role line at the head. With identity first, a context wrap lands above it instead of replacing it.chunkIdsProblemextraction — one definition shared between coverage'sreadPlanand the batch builder, preventing[13, "x", 15]from printing a complete-looking two-auditor round with one territory silently gone.- Test coverage is thorough — bad chunk IDs (all-bad, one-bad, duplicate), empty findings, rules propagation into every brief, mode conflict guards (
--roster,--whole-diff,--chunk, bare), and the key-identity test proving rules-less and rules-bearing builds get different keys.
Minor suggestions (non-blocking)
-
foldFindingsassumes the first line is always the identity line (You are review agent …). This holds today, but ifbuildRoleLaunchPrompt's output format ever changes, the split could silently misplace the findings section. A one-line assertion or comment pinning the invariant would future-proof it. -
The
runAllChunkscastreport.chunks as DiffChunk[]is safe given the plan JSON shape, butchunkIdsProblemonly validatesid— other properties (diffRange,files) could be malformed. Low risk since downstream code would fail loudly, but worth noting.
— Qwen Code review
ytahdn
reviewed
Jul 18, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. 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
…ts on The fold assumed line one of the launch prompt IS the identity line, which holds today because buildRoleLaunchPrompt writes it first — but a future prompt shape that moves it would make the split silently rebuild the buried-identity layout the fold exists to prevent. Refuse loudly instead: folding under the wrong first line is corruption, not a formatting choice. Verified by mutation: rewording the identity line in buildRoleLaunchPrompt now turns every batch test red with the fold's own error, instead of shipping prompts with the findings misplaced.
Collaborator
Author
|
Re: review 4728122658 — thanks for the approval; both suggestions answered:
|
…s refused The id-validation tests all pass a populated chunks[], so the guard ahead of them (missing or empty chunks[]) had no test of its own: deleted or inverted, nothing went red. Covers both shapes — chunks: [] and the key absent entirely — because an inverted condition still throws on the empty array and only the missing key exposes it.
ytahdn
approved these changes
Jul 18, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
LGTM ✅
— qwen3.7-max via Qwen Code /review
wenshao
commented
Jul 18, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen-latest-series-invite-beta-v77 via Qwen Code /review
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
|
Re: review 4728189158 — the "CI failing: route" downgrade caught a superseded run, not a failure: |
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 18, 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 18, 2026
wenshao
added a commit
that referenced
this pull request
Jul 18, 2026
…ftover
classifyCi already asks "did this name run" per NAME -- the routing
workflows re-dispatch a name several times per commit -- but it judged
failure per RUN: any single failing conclusion pushed its name into
failedCheckNames, including the cancelled runs those very re-dispatches
displace and the failed attempt a flaky job leaves behind when re-run to
green. Two real reviews were downgraded from Approve over exactly that
("CI failing: route" at #7150; route, review-pr, review-config and four
more at #7171), each on a commit whose every live check was green on the
PR page.
Failure is now judged by the name's latest verdict, ordered by the runs'
own timestamps -- the same evidence GitHub's PR page shows a human.
Skipped/neutral/stale runs stay non-verdicts, so a re-dispatch that
skipped cannot erase a real failure beside it; a name whose only run was
cancelled still fails, because nothing superseded it; and a re-run that
fails after a success fails, because latest wins in both directions. On
equal or absent timestamps the first-listed run keeps the name (the API
lists newest first).
pull Bot
pushed a commit
to bit-cook/qwen-code
that referenced
this pull request
Jul 18, 2026
…ftover (QwenLM#7183) * fix(review): judge a CI check by its name's latest run, not by any leftover classifyCi already asks "did this name run" per NAME -- the routing workflows re-dispatch a name several times per commit -- but it judged failure per RUN: any single failing conclusion pushed its name into failedCheckNames, including the cancelled runs those very re-dispatches displace and the failed attempt a flaky job leaves behind when re-run to green. Two real reviews were downgraded from Approve over exactly that ("CI failing: route" at QwenLM#7150; route, review-pr, review-config and four more at QwenLM#7171), each on a commit whose every live check was green on the PR page. Failure is now judged by the name's latest verdict, ordered by the runs' own timestamps -- the same evidence GitHub's PR page shows a human. Skipped/neutral/stale runs stay non-verdicts, so a re-dispatch that skipped cannot erase a real failure beside it; a name whose only run was cancelled still fails, because nothing superseded it; and a re-run that fails after a success fails, because latest wins in both directions. On equal or absent timestamps the first-listed run keeps the name (the API lists newest first). * test(review): pin the started_at fallback in verdictStamp The fallback branch had no test: with it dropped, two same-name runs whose completed_at is null collapse to insertion order. The winning run is listed second on purpose -- listed first, the fixture passes with or without the fallback (first-seen keeps the name on equal stamps) and pins nothing; listed second, the success can win only through its started_at.
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
Adds
agent-prompt --all-chunks, which builds every Step 5 reverse-audit prompt of a round in one call — one labelled, numbered block per chunk (auditor k of N), anend of roundmarker for the truncation self-check, and per-chunk records keyed by findings digest — and reorders the findings fold so the agent's identity line leads the printed prompt, with the findings section beneath it. SKILL.md's Step 5 (3B) switches to the batch call with the redirect-and-page protocol and an explicit ban on sampling builder output.Why it's needed
Both changes come from the transcript of the first post-#7033 dogfood run (
/review 7121), which named the last hand-assembly that survives the review pipeline, in both of its observed shapes. The per-chunk form asked the orchestrator for one build-and-capture round trip per chunk per round, and the real run answered withfor i in 1..10; do agent-prompt … | head -5; done— it sampled each build instead of capturing it, never possessed a single full prompt, hand-reconstructed all ten launches, and every one was flaggedrewritten, costing a full repair round to recover from a shortcut that saved nothing. And the one edit that run made to a prompt it fully possessed was at the head: it swapped the verifier's identity line for a context sentence of its own — with the identity line first, a context wrap lands above it instead of replacing it, and the delivery check keeps its anchor.Reviewer Test Plan
How to verify
Run the batch form through the real CLI against any 3B plan (or the synthetic one below) and confirm the output contract and records:
Expected: a header stating
N auditors required this roundwith the no-sampling rule, one───── auditor k of N — chunk <id> ─────block per chunk each ending in the same brief-pointer/read shape as the single form, a closing───── end of round — N auditors ─────line, and<plan>-prompts/reverse-audit--chunk-<id>--<digest>.txtrecords whose content is byte-identical to the printed blocks. Guards:--all-chunkswith--role verifyrefuses (not a per-chunk role); with--chunkrefuses (contradiction). For the reorder: build--role verify --findings <f>and confirm the printed prompt starts withYou are review agent `verify`and the findings section sits beneath it.Evidence (Before & After)
Before (run 7121 transcript): the orchestrator's actual command was
for i in 1 2 3 4 5 6 7 8 9 10; do … agent-prompt --plan … --role reverse-audit --chunk $i --findings … | head -5; echo "---"; done— ten five-line samples, zero possessed prompts, ten hand-reconstructed launches, all flaggedrewrittenby compose-review, one full repair round. After (this PR, live run on a 3-chunk plan): one call prints3 auditors required this round …, blocksauditor 1..3 of 3,end of round — 3 auditors, and recordsreverse-audit--chunk-{1,2,3}--20613df321c6.txtland beside the plan.Tested on
Risk & Scope
Main risk is the record/delivery contract for batch-built rounds; it is pinned by tests and mutation-checked three ways (cross-wiring every block to one chunk's ranges, recording less than what was printed, and demoting the identity line each turn exactly one test red — the first needed a two-chunk assertion, because checking only the first block cannot see a batch built entirely from it). The reorder changes the printed layout for findings roles, so the findings-free-tail attack pin was rebuilt for the new shape: a launch carrying the block without the findings section still matches no record. Out of scope: verify sharding (single-key, unchanged), the Step 3 roster (already batched), and any delivery-gate semantics — 707 review tests pass unchanged. No breaking changes;
--chunk <id>remains for single rebuilds.Linked Issues
Follow-up to #7033 (the delivery-gate hardening this closes the loop on) and sibling of #7143 (the other friction found by the same dogfood run).
中文说明
这个 PR 做了什么
新增
agent-prompt --all-chunks:一次调用构建 Step 5 一整轮的全部 reverse-audit prompt——每 chunk 一个带编号的标签块(auditor k of N)、end of round截断自检标记、按 chunk × findings 摘要分 key 的记录;并把 findings 折叠改为身份行前置(findings 段在其下方)。SKILL.md Step 5(3B)同步改为批量调用 + 重定向分页协议 + 明令禁止对 builder 输出抽样。为什么需要
两处修改都取自 #7033 合入后首次 dogfood(
/review 7121)的 transcript——它点名了 review 管道里仅存的手工拼装环节的两种实测形状。逐 chunk 形式要求每 chunk 每轮一次「构建→捕获」往返,真实 run 的答卷是for i in 1..10; do agent-prompt … | head -5; done:对每次构建抽样而非捕获,从未持有任何完整 prompt,10 个 launch 全部手工重建,每一个都被标为rewritten,整轮修复花在一个毫无收益的捷径上。而同一 run 对全文在手的 prompt 做的唯一编辑发生在头部——把 verifier 的身份行换成自己的背景句;身份行前置后,背景包裹只能落在其上方,交付检查保住第一锚行。评审验证方案
如何验证:对任意 3B plan 跑
node scripts/dev.js review agent-prompt --plan <plan> --role reverse-audit --all-chunks --findings <f>,确认:头部声明N auditors required this round并带禁抽样规则;每 chunk 一个───── auditor k of N — chunk <id> ─────块;───── end of round ─────收尾;<plan>-prompts/reverse-audit--chunk-<id>--<digest>.txt记录与打印块逐字节一致。守卫:--all-chunks配--role verify拒绝,配--chunk拒绝。身份行前置:构建--role verify --findings <f>,打印件以You are review agent `verify`开头,findings 段在其下。前后对照:修前(run 7121 transcript)= 十次五行抽样、零持有、十个手工重建 launch、全部 rewritten、一整轮修复;修后(3-chunk plan 实跑)= 一次调用、3 个编号块 + end 标记、3 份 digest-key 记录落盘。
风险与范围:主风险是批量轮次的记录/交付契约,已由测试钉住并三路变异验证(跨接范围/少记录/身份行降位各红一测,第一路需双 chunk 断言)。折叠布局变更后重建了 findings 缺失攻击钉子。不在范围:verify 分片、Step 3 roster、交付门语义(707 测试原样通过)。无破坏性变更;单块重建仍用
--chunk <id>。关联:#7033 的收尾 follow-up;与 #7143 同源(同一次 dogfood 发现的另一摩擦点)。