feat(review): prove Step 4 (verify) and Step 5 (reverse audit) actually ran - #6965
Merged
Conversation
…ly ran `check-coverage` proves Step 3 was done, from the harness's own transcripts. But it runs at Step 3D — before verify and reverse audit exist — so its roster never reaches them, and a run could skip Step 4 or Step 5 wholesale, or launch agents that never opened their brief, and nothing would see it. That is the same silent-omission failure the coverage gate was built for, one pipeline stage later. Their count is not in the plan (verify shards on the finding count; the reverse audit loops until it goes dry), so there is no exact roster to check. What there is is a floor, and `compose-review` is the place to check it: it runs only at high effort — the only effort at which verify and reverse audit run at all — and it already recomputes coverage from the transcripts on the way to the verdict. `verificationGaps` asks two questions of the same records: - **Reverse audit** — required on every high-effort review, because it is the pass that looks for what Step 3 missed, and a verdict that never ran it cannot certify the diff complete, least of all a clean one. At least one auditor must have run and opened its brief (3A records it under `reverse-audit`, 3B under `reverse-audit--chunk-N`). - **Verify** — required once the review has findings, because an unverified finding must not become a public blocker. Keyed to inline findings: a review that confirmed nothing has nothing to verify, and deterministic `[build]`/`[test]` findings are pre-confirmed and skip verification by design. A gap is named in `unreviewedDimensions` and caps the verdict exactly like a dimension nobody reviewed — an Approve drops to Comment, the gap is disclosed in the body, the findings still post. The highest-value catch is a clean, zero-finding review that never ran its reverse audit and would otherwise have approved. Nothing is passed in and nothing can turn it off: the proof is the intersection of the prompt the CLI recorded building and the harness's transcript of an agent that ran it and read the brief — two artifacts, neither authored by the orchestrator. Tests: verificationGaps over real transcript/record fixtures (reverse audit ran / skipped / built-but-brief-unread / 3B per-chunk key; verify required-when-findings / not-when-clean / brief-unread), and compose-review integration (the cap reaches the verdict and the body).
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed and well-documented. The coverage pipeline ( Direction: aligned. This is internal review-quality infrastructure — making the Size: 5 files changed. Production logic: ~114 lines ( Approach: minimal and consistent with the existing pattern. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的真实缺口。覆盖率管线（#6771 的 方向：对齐。这是内部审查质量基础设施——让 规模：5 个文件变更。生产逻辑：~114 行（ 方案：最小化且与现有模式一致。 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Code review: clean. The The test updates are well-considered: Test results: All 605 tests pass across 29 test files, matching the PR's claim. TypeScript typecheck is clean. 中文说明代码审查： 干净。
测试更新考虑周全： 测试结果： 605 个测试全部通过（29 个测试文件），与 PR 声明一致。TypeScript 类型检查干净。 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 — Clean across every stage. The problem is a documented gap in an ongoing series, the solution is one function and one call site built entirely on existing infrastructure, 605 tests pass, typecheck is clean. This is the kind of PR that makes a pipeline trustworthy. The test suite is thorough: fixture-driven tests covering ran/skipped/built-but-not-launched/brief-unread for both steps, per-chunk reverse auditor recognition, and No concerns. Ships the feature cleanly, the before/after behavior matches what the PR promised, and the code is straightforward enough that a future maintainer won't need the author to explain it. 中文说明信心：5/5 — 每个阶段都干净。问题是 ongoing 系列中有记录的缺口，方案是一个函数和一个调用点，完全构建在现有基础设施上，605 个测试通过，类型检查干净。 这是让管线值得信赖的那种 PR。 测试套件全面：fixture 驱动的测试覆盖了两步的跑了/跳过/构建但未启动/brief 未读场景，per-chunk reverse auditor 识别，以及 无顾虑。干净地交付了功能，before/after 行为与 PR 承诺一致，代码足够直接，未来维护者不需要作者解释。 — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
previously 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. ✅
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. |
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.
— qwen3.7-max via Qwen Code /review
| // review; verify is required once the review has findings to verify. | ||
| const verification = verificationGaps( | ||
| input.planPath, | ||
| { postsFindings: criticalsInline + suggestionsInline > 0 }, |
Collaborator
There was a problem hiding this comment.
[Suggestion] postsFindings excludes bodyCriticals — a review whose only confirmed findings are body Criticals (unanchorable or 422-relocated) skips the verifier requirement while still posting REQUEST_CHANGES with unverified findings.
Failure scenario: A Critical cannot be anchored → becomes a body Critical. criticalsInline=0, suggestionsInline=0 → postsFindings=false → no verifier required → REQUEST_CHANGES with unverified findings. The PR description acknowledges this as "the known edge this floor does not catch."
Suggested change
| { postsFindings: criticalsInline + suggestionsInline > 0 }, | |
| { postsFindings: criticalsInline + suggestionsInline + bodyCriticals.length > 0 }, |
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in dfdba1c — good catch, this closes the edge the description flagged. postsFindings now counts body Criticals too, minus the deterministic [build]/[test] ones (pre-confirmed, skip verification by design, and they carry their source tag so they are recognisable). Since any body Critical makes the event REQUEST_CHANGES and caps never soften an RC, this only ever affects the disclosure: a real unanchorable Critical is now told it was unverified; a [build]-only review is not (that disclosure would be false). Two compose-review tests pin both directions.
Comment on lines
+278
to
+282
| const verification = verificationGaps( | ||
| input.planPath, | ||
| { postsFindings: criticalsInline + suggestionsInline > 0 }, | ||
| input.env, | ||
| ); |
Collaborator
There was a problem hiding this comment.
[Suggestion] verificationGaps() re-reads the plan, transcripts, and recorded prompts that coverageFromTranscripts() already loaded ~70 lines earlier in the same try block. This doubles filesystem I/O on every high-effort review (plan JSON parse, transcript directory scan + JSON parse of each agent's JSONL, prompt record directory scan).
Concrete cost: each transcript is read from disk and every line JSON-parsed twice. With 23+ chunk agents on real PRs, each with multi-KB JSONL transcripts, this scales linearly. Also, if the second read throws after the first succeeded, the shared catch block appends a contradictory "cannot show that any of the diff was read" message alongside the specific coverage data already pushed.
Suggested change
| const verification = verificationGaps( | |
| input.planPath, | |
| { postsFindings: criticalsInline + suggestionsInline > 0 }, | |
| input.env, | |
| ); | |
| const verification = verificationGaps( | |
| input.planPath, | |
| { postsFindings: criticalsInline + suggestionsInline > 0 }, | |
| input.env, | |
| { records: cov.records, built: cov.built }, | |
| ); |
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
The contradictory-catch half is fixed in dfdba1c: verificationGaps now runs in its own try/catch, so a read failure there reports itself instead of wearing the coverage block's "cannot show the diff was read" over a coverage result that just succeeded. On the double read: I kept it deliberately. coverageFromTranscripts returns the computed report, not the raw records, and threading the raw {records, built} out of it (and an optional param into both functions) trades a clean standalone verificationGaps for an optimisation of a few KB-sized file reads that run twice per review — this file's stated preference is clarity over that. Happy to do the sharing refactor if you'd rather.
| return records.some( | ||
| (r) => | ||
| wasDeliveredVerbatim(r.launchPrompt, b) && | ||
| r.successfulCallArgs.some((a) => a.includes(brief)), |
Collaborator
There was a problem hiding this comment.
[Suggestion] Brief-path check uses bare a.includes(brief) substring match, but the diff-path check in parseTranscript already learned to use JSON.stringify: a bare substring credits …/diff.txt.bak for …/diff.txt. The brief check did not apply the same lesson.
Failure scenario: a sibling file whose path has the brief path as a strict prefix (e.g. reverse-audit.brief.md.bak) exists, and an agent opens that file instead of the brief. a.includes(brief) still matches, ranAndReadBrief returns true, the verification gap clears, and composeReview approves a review whose agent never opened its brief.
Suggested change
| r.successfulCallArgs.some((a) => a.includes(brief)), | |
| r.successfulCallArgs.some((a) => a.includes(JSON.stringify(brief))), |
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in dfdba1c — applied the parseTranscript diff-path lesson here. Both brief-open checks in this file (this Step 4/5 one and the roster's unreadBriefs) now match the whole quoted JSON value (JSON.stringify(brief)), so …/x.brief.md.bak no longer counts as …/x.brief.md.
| // `reverse-audit--chunk-N`, one per chunk. The floor is one: at least one auditor | ||
| // ran and read its brief. | ||
| const reverseKeys = [...built.keys()].filter( | ||
| (k) => k === 'reverse-audit' || /^reverse-audit--chunk-\d+$/.test(k), |
Collaborator
There was a problem hiding this comment.
[Suggestion] The regex /^reverse-audit--chunk-\d+$/ is a hand-duplicated mirror of the key-construction logic in agent-prompt.ts (${role}--chunk-${args.chunk}). No shared constant or type links them.
Concrete cost: someone refactors the key format in agent-prompt.ts (changes separator, renames chunk). The regex silently stops matching per-chunk keys. A review that ran a full per-chunk reverse audit fails the floor check with "no auditor ran", capping a correct review to COMMENT. No test fails because coverage tests use hardcoded key strings.
Suggested fix: export the key-format logic as a shared function or extract the separator --chunk- into a constant both sites import.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in dfdba1c. The match now keys off the role name and the universal -- key separator (k === "reverse-audit" || k.startsWith("reverse-audit--")) instead of the exact --chunk-<n> shape, so a change to how the chunk suffix is spelled no longer silently drops every per-chunk key and caps a correct review. A shared key-format module across agent-prompt/roster/prompt-record would be the fuller fix, but that is a wider refactor than this PR.
| `You are review agent \`${key}\`.\n` + | ||
| `read_file(file_path="${brief}")\n` + | ||
| `read_file(file_path="${DIFF}")`; | ||
| writeFileSync(join(d, `${key}.txt`), launch); |
Collaborator
There was a problem hiding this comment.
[Suggestion] This test helper writes prompt records as ${key}.txt (raw), while the sibling check-coverage.test.ts correctly uses ${encodeURIComponent(key)}.txt matching production code in prompt-record.ts.
Concrete cost: for current keys (verify, reverse-audit) this is a no-op since hyphens are not percent-encoded. But if a future role name with a character that encodeURIComponent transforms is added, this writes my role.txt while production reads my%20role.txt, causing a silent test mismatch. The inconsistency between the two test files means a copy-paste from the wrong one propagates the bug.
Suggested change
| writeFileSync(join(d, `${key}.txt`), launch); | |
| writeFileSync(join(d, `${encodeURIComponent(key)}.txt`), launch); |
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in dfdba1c — the fixture now records under encodeURIComponent(key), matching prompt-record.ts. A no-op for todays keys, but it stops a future role name from being written to a file the reader never looks for.
| expect(r.gaps.join(' ')).not.toMatch(/verification/); | ||
| }); | ||
|
|
||
| it('flags a verifier built but whose agent never opened its brief', () => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The verify side tests opensBrief: false but not launch: false, unlike the reverse-audit side which tests both failure modes symmetrically.
Concrete cost: launch: false exercises a different intersection in ranAndReadBrief — built.get('verify') returns content, but records.some(…) finds zero matching transcripts because none were written. Both produce the same gap message, but they fail at different points in the boolean expression. A regression in the transcript-matching half would go undetected.
Suggested fix: add a test mirroring the reverse-audit launch: false case:
it('flags a verifier whose prompt was built but never launched', () => {
const p = plan();
step45(p, 'reverse-audit');
step45(p, 'verify', { launch: false });
const r = verificationGaps(p, { postsFindings: true }, ENV);
expect(r.gaps.join(' ')).toMatch(/verification — its prompt was built/);
});— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in dfdba1c — added the launch: false verify case, mirroring the reverse-audit side. It exercises the transcript-matching term of ranAndReadBrief (built record present, no matching transcript), which opensBrief: false does not reach.
Six findings from the /review skill's pass over this PR, all addressed. The verify floor keyed on inline findings alone, so a non-deterministic Critical that could not be anchored — a body Critical — posted under REQUEST_CHANGES without ever demanding a verifier. It now counts body Criticals too, minus the deterministic `[build]`/`[test]` ones, which are pre-confirmed and skip verification by design (and carry their source tag, so they are recognisable). A review whose only finding is a real unanchorable Critical is now told it was not verified; one whose only finding is a build failure is not — that disclosure would be false. The brief-open check matched the brief path as a bare substring, the trap `parseTranscript` already learned to avoid for the diff path: `…/x.brief.md.bak` would have counted as `…/x.brief.md`. Both this file's brief checks — the Step 4/5 one and the roster's — now match the whole quoted JSON value. The per-chunk reverse-audit key was recognised by a regex hardcoding the `--chunk-<n>` shape; it now keys off the role name and the universal `--` separator, so a change to how the suffix is spelled does not silently drop every per-chunk key and cap a correct review. `verificationGaps` now runs in its own try, so a read failure there reports itself rather than wearing the coverage block's "cannot show the diff was read" message over a coverage result that succeeded a line above it. Tests: the fixture helper records prompts under the percent-encoded key, matching production; a `launch: false` verify case covers the transcript-matching half of the floor that the `opensBrief: false` case does not; and two compose-review cases pin the body-Critical verify rule (a non-deterministic one demands a verifier, a `[build]` one does not).
Collaborator
Author
|
Thanks for the review — the bot approved, and all six inline Suggestions are addressed in dfdba1c.
608 review tests pass (605→608), review |
Collaborator
Author
ReviewVerdict: LGTM with minor notes. The gate closes the hole it names, the keys it floors on are exactly what production records, every failure path I could construct fails closed, and the new tests have teeth (mutation-checked below). The notes are doc-staleness and one missing regression test — nothing blocking. What I verified (ran, not just read)
Notes
Test planReproduced as claimed: |
Collaborator
Author
Review — verified on Linux at head
|
Collaborator
Author
|
@qwen-code /triage |
…ef arm Two non-blocking notes from the verification review. The `verificationGaps` doc comment still said the verify floor was "keyed to INLINE findings" — one commit stale. It is `opts.postsFindings`, decided by the caller, which now counts non-deterministic body Criticals and excludes deterministic `[build]`/`[test]` ones. The comment now says that. The `.bak` tightening of the brief-open check had a regression test for the Step 4/5 arm but not the Step 3 roster arm they share. Added one: a roster agent that opened `<brief>.bak` is not credited with opening the brief.
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed and concrete. The PR names the silent failure precisely — a zero-finding high-effort review could skip the reverse-audit pass wholesale and still certify the diff clean. The "before" behavior is reproducible from the existing harness (swap main's Direction: aligned. This is the natural closure of the #6892/#6942 work that moved Step 3/4/5 prompts into code. Prompts existing in code ≠ agents ran; this PR closes that gap for the two steps whose omission is most costly and least visible. Follows the existing coverage-gate pattern ( Size: 5 files, ~386 production lines ( Approach: minimal. One new function ( One acknowledged edge the PR is transparent about: inline counts are bare numbers, so a review whose only inline findings are pre-confirmed Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 已观测到且可复现。PR 精确地命名了这个静默失败——一个零发现的 high-effort review 可以整步跳过 reverse audit,仍然判定 diff 为 clean。"之前"行为可以从现有 harness 复现(把 main 的 方向： 对齐。这是 #6892/#6942 (把 Step 3/4/5 prompt 搬进代码) 的自然闭环。prompt 存在于代码 ≠ agent 跑了;本 PR 为「遗漏代价最大、最难看见」的两步补上这个缺口。沿用 规模： 5 文件,约 386 生产行 ( 方案： 最小。 PR 坦诚承认一个边界:inline 计数只是数字,所以「唯一 inline 发现是预确认 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Author
|
Thanks for the deep verify — the before/after swap and the production-writer→reader E2E are exactly the checks I'd want on this. All six notes handled:
609 review tests pass (+1 for the .bak case), |
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Code review: clean. Two things worth noting: 1. The 2. The The reverse-audit key matching ( No critical issues found. No AGENTS.md violations. TestingNon-UI change (coverage computation + verdict cap) — tested via the fixture-driven test suite. The new tests exercise real transcript and prompt-record files, not mocks: Full review suite: 609 tests across 29 files, all passing. 中文说明代码审查： 干净。 两个值得注意的点: 1. 2. 未发现 Critical 问题。未违反 AGENTS.md。 测试： 非 UI 改动(覆盖率计算 + 判定 cap),通过 fixture 驱动的测试套件验证。新测试使用真实 transcript 和 prompt-record 文件,非 mock。完整 review 套件:29 文件 609 测试全部通过。 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 — Clean across every stage. The problem is a documented gap in an ongoing series (#6892, #6942), the solution is one function and one call site built entirely on existing infrastructure, every failure path I tested fails closed, and the tests cover every shape the PR describes. The PR does exactly what it says: closes the last hole in the What I'd want as a maintainer: the function is testable in isolation (and is, with 11 new Approving. ✅ 中文说明信心: 5/5 — 各阶段均干净。问题是已有系列 (#6892、#6942) 中已记录的缺口,方案是一个函数加一处调用点,完全基于现有基础设施,我测试的每条失败路径均 fail closed,测试覆盖了 PR 描述的每种形态。 PR 完全做到了它声称的事:补上 作为 maintainer 我想看到的:函数可独立测试(也确实是,11 个新 批准 ✅ — 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.
Approve. Reviewed the full diff, ran the changed test files locally, and traced the load-bearing assumptions through the codebase.
Correctness — verified
- Not a dead switch:
agent-promptrecords theverify/reverse-audit/reverse-audit--chunk-Nkeys and writes their briefs, soverificationGapshas real, differently-authored artifacts to intersect. - The gap is real:
requiredAgents()never emits these roles, so the Step-3D coverage roster genuinely cannot reach them — this closes an actual hole rather than duplicating an existing check. - The unconditional reverse-audit cap is safe:
compose-reviewruns high-effort-only (SKILL.md gates Step 4/5 and the verdict to high effort; low/medium passes emit no verdict and never call it). - Fail-open: whenever
postsFindingsis true,c>=1/s>=1, sobaseEventis already non-APPROVE — the verify gap can only add a disclosure, never flip a real blocker into an approval. - No false-negative on the real reverse-audit flow:
wasDeliveredVerbatimpermits the prepended cumulative finding list Step 5 adds above the pasted prompt. - Step 6 / Step 7 stay consistent:
submitcalls the samecomposeReviewwith the full state includingplanPath, so the printed verdict and the posted one are the same computation. - The
.bakfix mirrorsparseTranscript's diff-pathJSON.stringifypattern and is now applied to both brief-open checks (Step 4/5 and the roster'sunreadBriefs).
Tests / CI
The two changed test files pass 93/93 locally (check-coverage 38, compose-review 55). The unrelated test-efficacy symlink failure reproduces on main (a macOS rmSync symlink behavior), and the unquoteCStylePath tsc error is a stale core dist/.d.ts in an untouched file (diff-plan.ts) — the symbol is exported from source and clears on a core rebuild. The six findings from the PR's own /review pass were all addressed in dfdba1c49.
Non-blocking nits (optional follow-ups)
- The
[build]/[test]exclusion regex is unanchored, so a real non-deterministic Critical whose prose contains the literal tag would be misclassified as pre-confirmed. Cosmetic only — it suppresses the "unverified" disclosure while the finding still posts as REQUEST_CHANGES; it can never produce a false Approve. Could tighten to match theSource:tag context. - When transcripts are unavailable, the coverage block and
verificationGapseach emit a cap line for the same root cause — two lines, both true and both capping correctly, but slightly redundant in the public body.
Nice work — the highest-value catch (a clean, zero-finding review that skipped its reverse audit and would otherwise have approved) is genuinely closed.
chinesepowered
pushed a commit
to chinesepowered/qwen-code
that referenced
this pull request
Jul 16, 2026
…ompt (QwenLM#6994) * feat(review): fold findings into the verify/reverse-audit prompt so it can't be paraphrased Dogfooding the merged Step 4/5 coverage gate (QwenLM#6965) on a real 3A review turned up the weakness it was built to expose. The orchestrator delivered every Step 3 prompt verbatim — but PARAPHRASED the Step 4 (verify) and Step 5 (reverse-audit) ones: it added a round number, inserted its own one-line summary, and truncated the recorded line telling the agent the brief is the whole of its instructions. The gate caught it (the launch prompt was not the recorded one) and capped the verdict, even though the agents had opened their briefs and done real work. The root cause is the hand-assembly step. Step 3 is a clean paste; Step 4/5 told the orchestrator to "paste verbatim AND prepend the findings list", and constructing that prepend is where the wording drifted. So this removes the assembly step: `agent-prompt --role verify|reverse-audit --findings <file>` folds the list in and prints one block to paste. There is nothing left to hand-build, so nothing to reword. The findings are printed, not recorded. The record stays the findings-free launch block, so the per-shard (verify) / per-round (reverse-audit) key keeps matching by the add-only delivery rule — a printed `<findings>\n\n<block>` still contains the recorded `<block>` in order — without baking a different findings list into each record. The folded section restates that the brief is authoritative, which is the exact sentence the hand-assembly used to truncate. `--findings` is declared on the brief (`acceptsFindings`, like `acceptsChunk`) and rejected on any other role, on a bare chunk agent, and on `--whole-diff`. SKILL Step 4/5 now write the findings to a file, pass `--findings`, and paste the whole block verbatim — no round number, no rewording. Tests: the verifier/auditor get the findings folded above a findings-free record that still passes the delivery check; an empty findings file tells a first-round auditor nothing is confirmed yet; the record is byte-identical with or without `--findings` (the shared key is unaffected); and the guard rejects the flag everywhere it does not belong. * fix(review): derive the no-role --findings message, clean up temp dirs, test verify-empty Three findings from the /review pass over this PR. The "--findings with no role" error hardcoded `--role verify / --role reverse-audit`, while the wrong-role branch two above it derives the list from `acceptsFindings`. A future role with that flag would leave this one telling a stale story. It now derives the same way (the `--chunk` guard already set this precedent). The `--findings` test block created temp dirs and never removed them — the one block in the file without try/finally cleanup. It now tracks each dir and sweeps them in an `afterEach`. `findingsSection('verify', '')` — a verifier handed an empty findings file — had no test; only the reverse auditor's empty branch did. Added the symmetric case: it prints "nothing to verify", not the auditor's "nothing is confirmed yet". * test(review): cover --role reverse-audit --chunk N --findings F, the 3B path The one valid flag triple was untested: reverse-audit declares both acceptsChunk and acceptsFindings, and Step 5 3B launches `--role reverse-audit --chunk N --findings <cumulative>` per chunk per round — the real production path. Added a case: the findings fold above the chunk-scoped prompt, the printed prompt reads that chunk's range (not another's), and the record is that chunk's block, findings-free, keyed `reverse-audit--chunk-N`, and still passes the delivery check. The code was already correct; nothing failed. Now it is pinned. * test(review): assert each findingsSection role does NOT get the other's framing The verify and reverse-audit non-empty tests each asserted their own framing heading but not the absence of the other's, so a swap of the two branches in findingsSection would have passed both. Added the mirror assertions: the verifier's prompt must not say "Already confirmed", and the auditor's must not say "The findings you are ruling on". The swap is now caught. * fix(review): make findingsSection reject a role it has no framing for findingsSection matched `verify` and fell through to the reverse auditor's framing for everything else. A future role that sets acceptsFindings would pass every guard, reach here, and silently receive "Already confirmed — do not re-report" — wrong for any role not hunting gaps. The branches are now explicit (`verify`, `reverse-audit`) with a throwing default, the same reasoning the no-role guard message already follows. Exported and tested: a role it does not frame throws by name.
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
Closes the last hole in the
/reviewskill's coverage story.check-coverage(#6771) proves Step 3 was done, from the harness's own per-agent transcripts. But it runs at Step 3D — before Step 4 (verify) and Step 5 (reverse audit) exist — so its roster never reaches them, and a run could skip either step wholesale, or launch agents that never opened their brief, and nothing would see it. This addsverificationGaps, called bycompose-review(which runs only at high effort, the only effort at which verify and reverse audit run at all), to prove those two steps ran the same waycheck-coverageproves Step 3: from the intersection of the prompt the CLI recorded building and the harness's transcript of an agent that ran it and read the brief.A missing step is named in
unreviewedDimensionsand caps the verdict exactly like a dimension nobody reviewed: an Approve drops to Comment, the gap is disclosed in the review body, and the findings still post. Nothing is passed in and nothing turns it off.Why it's needed
Follow-up to #6892 and #6942, which moved the Step 3/4/5 agent prompts into code so the review can no longer drift. This closes the loop: those prompts existing in code does not prove the agents ran. The two steps this checks are the ones whose omission is most costly and least visible — the reverse audit is the pass that looks for what Step 3 missed, so a clean, zero-finding review that skips it and approves is exactly the silent failure the whole coverage gate was built to stop, one pipeline stage later. Their agent count is not derivable from the plan (verify shards on the finding count; the reverse audit loops until it goes dry), so there is no exact roster — but there is a decidable floor: at least one reverse auditor on every high-effort review, and at least one verifier once the review has findings to verify.
Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review→ 605 passing (29 files);npx tsc --noEmit -p packages/cli/tsconfig.jsonfiltered tocommands/review→ clean. The new behavior is exercised by fixture-driven tests that write real transcript and prompt-record files:verificationGapsover a reverse audit that ran / was skipped / was built but whose agent never opened its brief / a 3B per-chunk auditor, and a verifier required-when-findings / not-when-clean / brief-unread; pluscompose-reviewintegration proving the gap caps the verdict and is disclosed in the body.Evidence (Before & After)
Non-UI change (a coverage computation and a verdict cap), so no screenshots. Behaviorally: before, a high-effort review that never launched a reverse auditor could compose an Approve; after,
compose-reviewcaps it to Comment withNot reviewed: reverse audit — no auditor ran (…)in the body. A review posting findings with no verifier now disclosesverification — the review posts findings, but no verifier ran.Tested on
Environment (optional)
Linux;
npx vitest/npx tsc. The review vitest suite runs againstsrc(no core dist dependency).Risk & Scope
/reviewskill — one new function inlib/coverage.tsand one call incompose-review.ts;check-coverage(Step 3D) is untouched, so nothing changes before Step 4/5. The verify floor counts the anchored findings plus non-deterministic body Criticals, and excludes deterministic[build]/[test]findings (pre-confirmed, skip verification by design) — so a real unanchorable Critical is required to have a verifier, while a[build]-only review is not. The residual edge (fails open, disclosure-only): inline finding counts are bare numbers, so a review whose only inline findings are pre-confirmed[test]Suggestions would get a spurious "not verified" disclosure — it can never move the event. The reverse-audit floor (unconditional at high effort) always applies.unreviewedDimensionswhen a scope is left outstanding.Linked Issues
Follow-up to #6892 and #6942 (no issue to close).
中文说明
这个 PR 做了什么
补上
/reviewskill 覆盖率故事的最后一个洞。check-coverage(#6771)从 harness 自己的 per-agent transcript 证明 Step 3 做了。但它在 Step 3D 运行——早于 Step 4(verify)和 Step 5(reverse audit)存在——所以它的 roster 够不到这两步,一个 run 可以整步跳过 Step 4/5,或启动从不打开 brief 的 agent,而无人能察觉。本 PR 加verificationGaps,由compose-review(只在 high effort 运行,而 verify/reverse-audit 也只在 high effort 存在)调用,用和check-coverage证明 Step 3 相同的方式证明这两步跑了:CLI 记录构建的 prompt 与 harness 中「某 agent 用它启动并读了 brief」的 transcript 的交集。缺失的步骤会被写进
unreviewedDimensions,像「无人审查的维度」一样 cap 判定:Approve 降为 Comment,body 里披露缺口,findings 照发。不传参、无法关闭。为什么需要
#6892、#6942 的后续——那两个 PR 把 Step 3/4/5 的 agent prompt 搬进代码,让审查不再漂移。这个 PR 闭环:prompt 在代码里,不代表 agent 跑了。这里检查的两步,是「遗漏代价最大、最难看见」的:reverse audit 是寻找 Step 3 漏掉之物的那一遍,所以一个零发现的 clean review 跳过它就 Approve,正是整个覆盖率 gate 要拦的静默失败,只是晚了一个流水线阶段。它们的 agent 数量无法从 plan 推出(verify 按发现数分片;reverse audit 循环到枯竭),所以没有精确 roster——但有可判定的下限:每个 high-effort review 至少一个 reverse auditor,有 findings 时至少一个 verifier。
Reviewer 测试计划
如何验证
cd packages/cli && npx vitest run src/commands/review→ 605 通过(29 文件);npx tsc --noEmit -p packages/cli/tsconfig.json过滤到commands/review→ 干净。新行为由 fixture 驱动的测试覆盖(真实写出 transcript 和 prompt-record 文件):verificationGaps对 reverse audit 跑了 / 被跳过 / 构建了但 agent 没打开 brief / 3B per-chunk auditor,以及 verifier 有发现时必需 / 无发现时不必需 / brief 未读;外加compose-review集成测试证明缺口 cap 判定并在 body 披露。证据(前后对比)
非 UI 改动(覆盖率计算 + 判定 cap),无截图。行为上:之前,一个从未启动 reverse auditor 的 high-effort review 能组成 Approve;之后,
compose-review把它 cap 成 Comment,body 含Not reviewed: reverse audit — no auditor ran (…)。发 findings 却无 verifier 的 review 现在披露verification — the review posts findings, but no verifier ran。测试平台
环境(可选)
Linux;
npx vitest/npx tsc。review vitest 套件跑src(不依赖 core dist)。风险与范围
/reviewskill——lib/coverage.ts一个新函数 +compose-review.ts一处调用;check-coverage(Step 3D)不动,Step 4/5 之前的一切不变。verify 下限 = 已锚定发现 + 非确定性 body Criticals,排除确定性[build]/[test](预确认、按设计跳过验证)——所以真·不可锚定 Critical 会要求 verifier,而[build]-only 的 review 不会。残留边界(fail-open、仅披露):inline 计数是裸数字,「唯一 inline 发现是预确认[test]Suggestion」的 review 会收到一句多余的「未验证」披露——但它永远不改变 event。reverse-audit 下限(high effort 无条件)始终生效。unreviewedDimensions披露。关联 Issue
#6892、#6942 的后续(无需关闭的 issue)。