fix(review): an unverified Critical must not become a public blocker — soften the Request changes it rides - #7191
Merged
Merged
Conversation
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed, with evidence. Links directly to review 4729338733 on #7166 — a real bot review shipped a Direction: squarely within scope. The review pipeline's verdict computation must enforce its own stated principle — an unverified finding must not become a public blocker. The gap existed because the mechanics enforced "confirmed" on the Approve row only. This closes it on the Request-changes row too. Size: 71 production logic lines (compose-review.ts: +58/-9, coverage.ts: +10/-1, SKILL.md: +3/-3), 258 test lines. No core module paths touched. Approach: the scope feels exactly right — one flag, three fail-closed exits, one event softening, one body carriage widening, one verdict line. No unrelated changes, no drive-by refactors. The manipulation check (both directions) is a nice touch. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的，有证据。 直接关联 #7166 上的 review 4729338733——一条真实 bot review 在外部贡献者的 PR 上发出了 方向：完全在范围内。review pipeline 的 verdict 计算必须执行自己声明的原则——未验证的发现不得成为公开 blocker。缺口存在于机制仅在 Approve 行执行"confirmed"。本 PR 在 Request-changes 行补上。 规模：71 行生产逻辑（compose-review.ts: +58/-9, coverage.ts: +10/-1, SKILL.md: +3/-3），258 行测试。未触及核心模块路径。 方案：范围恰到好处——一个 flag、三个 fail-closed 出口、一个 event 软化、一个正文携带拓宽、一行 verdict。无无关改动，无顺手重构。双向操纵检查是加分项。 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Code ReviewIndependent proposal (before reading the diff): Given the problem — Comparison with the diff: the PR matches this proposal exactly, and does it better in one detail — the Findings: no criticals, no blockers. The implementation is correct across every case I can think of:
The comments throughout are dense but earned — every one explains a why that the code alone cannot convey (the false "leaks tokens" Critical, the #7166 incident, the manipulation check). They're documentation of the design decisions that led to this shape, not narration of what the code does. TestingThis is internal review pipeline logic — All 31 test files green, 756 tests passing — matches the PR's claim exactly. The key new pins:
中文说明代码审查独立方案（阅读 diff 之前）： 给定问题—— 与 diff 对比： PR 完全匹配该方案，且在一个细节上做得更好—— 发现：无阻塞问题。 实现在我能想到的每种情况下都是正确的：
测试这是 review 流水线的内部逻辑—— 31 个测试文件全绿，756 个测试通过——与 PR 声明完全一致。 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage, the fix is correct and the gap it closes is real. This is the kind of PR you hope for from someone who understands the system they're fixing. The problem is not hypothetical — it shipped on #7166, where a The fix closes exactly that gap: The test suite is comprehensive — 756 tests across 31 files, all green. The flipped pin (the test that used to assert "a confirmed Critical still blocks" now asserts "no verifier, no blocker") tells the story of why this change exists. The No concerns. Shipping this. 中文说明置信度: 5/5 — 各阶段均通过，修复正确，所关闭的缺口确实存在。 这是一个了解系统的贡献者提交的修复。问题不是假设——它在 #7166 上实际发生了：一条 修复精确地关闭了这个缺口： 测试套件全面——31 个文件 756 个测试全绿。被翻转的钉子（曾断言"confirmed Critical 仍然阻塞"的测试现在断言"无 verifier 则无 blocker"）讲述了这个变更存在的原因。 没有问题，通过。 — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
approved these changes
Jul 19, 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.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
Collaborator
Code ReviewThe implementation is clean and well-reasoned. Three properties make this a solid fix: Fail-closed at every exit. The Deterministic findings correctly exempted. The Body carriage survives softening. The No correctness bugs, security issues, or regressions found. The code follows project conventions. Test ResultsUnit tests: 761/761 passing across 30 test files in Key test pins verified:
Real-Scenario Testing (tmux)Drove Test 1 is the key one: a body Critical with no verifier → 中文说明代码审查实现干净且推理充分。三个属性使这是一个扎实的修复： 所有出口一律 fail closed。 确定性发现正确豁免。 正文携带在软化后存活。 未发现正确性 bug、安全问题或回归。代码遵循项目规范。 测试结果单元测试：761/761 通过，30 个测试文件， 真实场景测试 (tmux)直接调用 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 This is a textbook fix for a real, documented process failure. The review that shipped a The fail-closed design is right at every seam: three exits, each setting the flag; the double condition preventing false positives on reviews with no Criticals; deterministic 761 unit tests green. Real-scenario tmux test confirms the verdict computation end-to-end. The flipped test name — from "a confirmed Critical still blocks" to "softens an unverified Request changes to Comment — no verifier, no blocker" — tells the story in one line. Approving. ✅ 中文说明Confidence: 5/5 这是一个教科书级的修复，解决了一个真实的、有据可查的流程缺陷。#7166 上那条在披露自己的发现未经验证的同时发出 Fail-closed 设计在每个接缝都正确：三个出口各自设置 flag；双重条件防止无 Critical 的 review 被误标；确定性 761 单元测试全绿。tmux 真实场景测试端到端确认 verdict 计算。被翻转的测试名——从 "a confirmed Critical still blocks" 到 "softens an unverified Request changes to Comment — no verifier, no blocker"——一行讲完故事。 批准。✅ — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
previously approved these changes
Jul 19, 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
reviewed
Jul 19, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Took another pass over this at head 6d9bc91. The change itself is sound — the event selection is internally consistent, it fails closed at all three exits (no plan / verify gap / unreadable transcripts, each tested), and runs that actually ran verify are byte-identical. One doc inconsistency worth fixing, plus a couple of minor notes.
Doc drift — SKILL.md still states the rule this PR reverses
packages/core/src/skills/bundled/review/SKILL.md (Step 7 / submit, ~L826) still reads:
A cap state forbids
APPROVEbut never softens aREQUEST_CHANGES
The new criticals-unverified cap is precisely a cap state that softens a REQUEST_CHANGES → COMMENT. You already qualified the parallel statement in code (compose-review.ts: "A coverage cap never softens…") and updated the Step 6 rules table, so this Step 7 restatement looks like a missed spot — the same "one downstream restatement not updated when an upstream rule gained a state" drift the module header calls out. Suggest matching the qualifier, e.g. "A coverage cap forbids APPROVE but never softens a REQUEST_CHANGES; the one exception is the unverified-blockers cap, which softens it to COMMENT (findings still posted, disclosed as unverified)."
(That line isn't in this PR's diff, so I couldn't anchor it inline — it's L826 of the file at head.)
Minor / optional
- When
criticalsUnverifiedandpresubmit.downgradeRequestChangesboth hold, the RC→COMMENT softening runs first, sodowngradedstaysfalseand the "⚠️ Downgraded from Request changes…: <reasons>" sentence +downgradeReasonsdrop out of the body/verdict. The event isCOMMENTeither way and the combination is rare, but the presubmit reason silently vanishes — worth a conscious call. - Nit: the
verdictLinetest titled "never names a cap on a Request changes … no cap softens it" reads a touch absolute now (the new cap does soften, just toCOMMENT, so it can't co-occur with anevent === REQUEST_CHANGES). The assertion itself is correct. - Optional tests: a direct
verificationGaps(...).unverifiedFindingsassertion, and a "verify on record + reverse-audit absent + Critical → still RC" case (softening is gated on verify only). Both are currently covered only indirectly.
Nice touches: fail-closed at every exit, the deterministic [build]/[test] carve-out, body-Critical survival when the event softens, and the verdict line that explains why a Comment sits over visible [Critical] comments.
Collaborator
Author
|
Re: review 4729799844 — all four taken, in
Suite: 758 green. (Housekeeping note: an earlier version of this commit briefly landed on #7190's branch by mistake and was force-reverted there within minutes — #7190 is back to |
qwen-code-ci-bot
previously approved these changes
Jul 19, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
…-- soften the Request changes it rides The module's header has stated the principle since the false "leaks tokens" Critical: an unverified finding must not become a public blocker. The mechanics only enforced it on the Approve row -- a verify gap capped an Approve to Comment -- while the Request-changes row stood on "a confirmed Critical earned that", with nothing checking CONFIRMED. A real bot review shipped through the gap: a CHANGES_REQUESTED posted onto an external contributor's PR (#7166) over one Critical that the review's own body disclosed as unverified, from a run whose verifier was built and never launched. verificationGaps now returns the structured bit (unverifiedFindings) instead of leaving the verdict to parse gap prose, and composeReview softens a Request changes to Comment when the non-deterministic Criticals it would post cannot be shown verified -- verifier missing, transcripts unreadable, or no plan to check against; fail closed at every exit, because "could not show" and "was not" read the same to the person the blocker is posted at. Deterministic [build]/[test] findings stay pre-confirmed and keep their Request changes. The findings still post either way -- the body copy of an unanchorable blocker survives the softening exactly as it survives the presubmit carve-out -- and the verdict line says why a Comment sits over visible Critical comments. Manipulation check, both directions: a run that wants an Approve gains nothing (the same gap already caps Approve), and a run that wants to block without verifying now cannot.
…he Step 7 restatement Review follow-through: the Step 7 skill text still said a cap never softens a REQUEST_CHANGES -- the exact one-restatement-behind drift the module header warns about; it now carries the coverage qualifier and the one exception. When the unverified cap and presubmit downgradeRequestChanges both held, the softening ran first and the presubmit reasons silently vanished from the body; the downgrade arm now also fires on a softened Request-changes base, so the body's downgrade clause carries the reasons while the verdict line keeps the unverified sentence. Plus a pin that verify on record with the reverse audit absent still blocks -- the softening gates on verify alone.
wenshao
force-pushed
the
fix/unverified-criticals-cap
branch
from
July 19, 2026 04:55
bb24430 to
70e8660
Compare
Contributor
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
previously approved these changes
Jul 19, 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. ✅
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
qqqys
previously approved these changes
Jul 19, 2026
…e an unverified sibling Review catch: the unverified-blockers softening dropped the whole REQUEST_CHANGES even when the review also carried a pre-confirmed [build]/[test] body Critical -- un-blocking a confirmed build failure because its sibling lacked a verifier. The softening now requires zero deterministic body Criticals; the unverified ones stay disclosed either way, and the mixed case is pinned.
wenshao
dismissed stale reviews from qqqys, yiliang114, and qwen-code-ci-bot
via
July 19, 2026 06:03
71a9498
qqqys
approved these changes
Jul 19, 2026
pomelo-nwu
approved these changes
Jul 19, 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
Closes the gap between a principle
compose-reviewhas stated since the false "leaks tokens" Critical — an unverified finding must not become a public blocker — and the mechanics, which only enforced it on the Approve row. A missing verifier capped an Approve to Comment, but the Request-changes row stood on "a confirmed Critical earned that", with nothing checking confirmed.verificationGapsnow returns the structured bit (unverifiedFindings) instead of leaving the verdict to parse gap prose, andcomposeReviewsoftens a Request changes to Comment when the non-deterministic Criticals it would post cannot be shown verified — verifier missing, transcripts unreadable, or no plan to check against; fail closed at every exit, because "could not show" and "was not" read the same to the person the blocker is posted at. Deterministic[build]/[test]findings stay pre-confirmed and keep their Request changes. The findings still post either way — the body copy of an unanchorable blocker survives the softening exactly as it survives the presubmit carve-out — and the verdict line says why a Comment sits over visible**[Critical]**comments:Verdict: Comment — a Request changes was NOT available: its blockers were never verified (they are posted, disclosed as unverified).submitsharescomposeReview, so the posted verdict and the Step 6 line move together. The skill's Step 6 rules table gains the row.Why it's needed
A real bot review shipped through the gap: review 4729338733 on #7166 posted a CHANGES_REQUESTED onto an external contributor's PR from a run whose verifier was built and never launched — the review's own body discloses "the posted findings cannot be counted as verified" beneath the verdict that blocks on them.
I verified that finding by hand afterwards, and it happens to be real — but that is luck, not process: the same mechanics would have posted a false blocker with identical confidence. Manipulation check, both directions: a run that wants an Approve gains nothing here (the same verify gap already caps Approve via
unreviewed-dimension), and a run that wants to block without verifying now cannot.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review— 31 files, 756 tests green.softens an unverified Request changes to Comment — no verifier, no blocker, and its comment tells the story. New pins: body Criticals survive the softening; deterministic-only Request changes stands; a verified Request changes stands; fail-closed with no plan; fail-closed with unreadable transcripts; the exact verdict line.submit-side: the three tests that post blockers now carry averifiedPlan()fixture (recorded verify prompt + transcript that ran it verbatim and opened the brief), so their originalREQUEST_CHANGESassertions still hold — and prove the cap binds only when verification is genuinely absent.Evidence (Before & After)
Before (#7166, live):
CHANGES_REQUESTED+ body "Not reviewed: verification — its prompt was built, but no agent was launched with it, so the posted findings cannot be counted as verified."After, same state:
COMMENT,cappedBy: ["criticals-unverified", …], the same disclosure, the Critical still posted, and the verdict line naming the reason.Tested on
Risk & Scope
Linked Issues
Exhibit: the review posted on #7166 (verified by hand — the finding is real; the process that posted it could not know that).
中文说明
本 PR 做了什么
补上
compose-review头注早已声明的原则——未验证的发现不得成为公开 blocker(自假 "leaks tokens" Critical 事件起)——与机制之间的缺口:机制过去只在 Approve 行执行(verify 缺口把 Approve 压成 Comment),而 Request-changes 行站在"confirmed Critical 挣来的"之上,却没有任何东西检查 confirmed。verificationGaps现在返回结构化位(unverifiedFindings),verdict 不再解析 gap 文案;composeReview在"要发布的非确定性 Critical 无法证明已验证"时把 Request changes 压成 Comment——verifier 缺席、transcripts 不可读、或没有 plan 可查,各出口一律 fail closed:对被 blocker 指向的人来说,"无法证明验过"和"没验过"读起来是一回事。确定性[build]/[test]发现保持预确认,Request changes 不受影响。findings 两种情况都照发——不可锚定 blocker 的正文副本在软化后存活,与 presubmit 降级豁免同一机制——verdict 行说明为何 Comment 之下有可见的**[Critical]**:Verdict: Comment — a Request changes was NOT available: its blockers were never verified (they are posted, disclosed as unverified)。submit与 Step 6 共用composeReview,两端一致。SKILL 的 Step 6 规则表新增该行。为什么需要
一条真实 bot review 从这个缺口穿了过去:#7166 上的 review 4729338733 在一个 verifier 构建了却从未启动的 run 里,对外部贡献者的 PR 发出了 CHANGES_REQUESTED——其正文自己披露 "the posted findings cannot be counted as verified",而 verdict 恰恰阻塞在这些未验证发现之上。
事后我人工验证了那条 finding,它碰巧是真的——但那是运气不是流程:同样的机制会以同样的置信度发布一个假 blocker。双向操纵检查:想骗 Approve 的 run 在此一无所获(同一 verify 缺口早已通过
unreviewed-dimensioncap 掉 Approve);想不验证就阻塞别人的 run,现在做不到了。验证方式
cd packages/cli && npx vitest run src/commands/review—— 31 文件 756 测试全绿。softens an unverified Request changes to Comment — no verifier, no blocker,注释讲清来龙去脉。新钉:body Critical 在软化后存活;纯确定性 RC 不受影响;已验证 RC 不受影响;无 plan fail-closed;transcripts 不可读 fail-closed;verdict 行原文。submit侧:三个发布 blocker 的测试补verifiedPlan()fixture(verify prompt 记录 + 逐字运行且打开 brief 的 transcript),原REQUEST_CHANGES断言保持——同时证明 cap 只在验证真正缺席时生效。证据(前后对照)
修复前(#7166 实况):
CHANGES_REQUESTED+ 正文 "Not reviewed: verification — its prompt was built, but no agent was launched with it, so the posted findings cannot be counted as verified."修复后,同样状态:
COMMENT、cappedBy: ["criticals-unverified", …]、同样的披露、Critical 照发、verdict 行点名原因。风险与范围
关联
展品:#7166 上的那条 review(已人工验证——finding 属实;但发布它的流程无从知道这一点)。