fix(review): keep the blocker in a COMMENT body every softening path reaches - #9416
Open
wenshao wants to merge 3 commits into
Open
fix(review): keep the blocker in a COMMENT body every softening path reaches#9416wenshao wants to merge 3 commits into
wenshao wants to merge 3 commits into
Conversation
…reaches
`compose-review` renders the body copy of an unanchorable blocker on a
COMMENT only when that COMMENT stands where a REQUEST_CHANGES would have
been — the body copy is its ONLY copy, and softening the event must never
erase it. The condition listing which softenings qualify was written as an
enumeration of the two flags known at the time:
if (downgradedFrom === 'Request changes' || criticalsUnverified)
A third path shipped past it. The findings-file `— [unverified]` tag softens
a Request changes at the event line, and it sets neither flag: not
`downgradedFrom` (only the presubmit carve-out sets that) and not
`criticalsUnverified` (only the verification-delivery gate sets that). So a
run whose coverage was PROVEN and whose verifier ran posted a
239-character body — the opener, the tag disclosure, the footer — with the
blocker nowhere in it, while the verdict line and the artifact both counted
it.
The condition is derived now rather than enumerated: `baseEvent` is the row
before every cap and downgrade, so `baseEvent === 'REQUEST_CHANGES' &&
event === 'COMMENT'` asks the question the clause is actually about, and
answers it for softening paths that do not exist yet. Same closure the
module applied to the deferral channel after #9095.
The regression test builds the isolating shape — covered plan, verifier on
record, findings file still carrying a tag — and asserts both flags the old
condition read are unset while the blocker rides the body. Reverting to the
enumeration reddens it; so does dropping the clause.
Collaborator
|
|
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. A real bot run posted a 239-character COMMENT body — opener plus unverified-tag disclosure — while Direction: aligned. The clause's own comment already states the property — "softening the event must never erase it" — this change makes the condition ask the question the clause is about instead of listing the flags known when it was written. Same closure the module applied to the deferral channel after #9095. Size: not core paths ( Approach: minimal — one condition swap, a comment explaining the derivation, and one test pinning the isolating shape. No drive-by changes. The alternative I'd consider — teaching the findings-tag path to set one of the existing flags — would push the enumeration problem one hop down and blur what those flags mean; deriving from Risk: no elevated risk signals — no high-risk path matches. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 已观测到，不是理论问题。一次真实的 bot 运行发出了 239 字符的 COMMENT 正文——只有开头与未验证标记披露——而 方向： 对齐。clause 自己的注释早已声明该性质——"软化事件不得抹除 blocker"——本改动让条件直接提出 clause 真正关心的问题，而不是列举写代码时已知的标记。与 #9095 之后对 deferral 通道采用的闭合方式一致。 规模： 非核心路径（ 方案： 最小改动——一处条件替换、一段解释推导的注释、一个钉住隔离形状的测试。无顺手改动。备选方案——让 findings 标记路径去设置已有标记之一——只会把枚举问题推到下一跳并模糊这两个标记各自的含义；从 风险： 无升级风险信号——未命中任何高风险路径。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewIndependent proposal first, before the diff: a condition that keeps enumerating softening flags will always lag the flag-setters, so I would derive the clause from the event history — The derivation checks out against every existing path:
The new test pins the isolating shape — coverage proven, verifier on record, findings file still tagged, one body Critical — and asserts the full machine shape ( No correctness, security, or convention issues found; the expanded comment matches this file's dense-rationale house style. TestingUnattended CI run — no local build or test execution of PR code. Evidence below is the PR's own CI on the reviewed commit, fetched via the API: Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the end-to-end claim: 中文说明代码审查先给独立方案再看 diff：一个持续枚举软化标记的条件永远追不上标记的设置者，所以我会从事件历史推导该 clause—— 推导在既有路径上逐一成立：
新测试钉住了隔离形状——覆盖已证明、验证者在案、findings 文件仍带标记、一个 body Critical——并断言完整机器形状（ 未发现正确性、安全性或规范问题；扩写的注释符合本文件密集说理的风格。 测试无人值守 CI 运行——不在本地构建或执行 PR 代码。下方证据来自被审 commit 的 PR 自身 CI（经 API 获取）： — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 5/5 — observed failure, minimal fix, a condition I verified is equivalent on every existing path, and a test that cannot pass under the old code. Stepping back: this is exactly the kind of fix the module's own history asks for. Clause 7 was an enumeration written when two softening paths existed; a third shipped past it and silently erased a blocker's only copy from a posted review. The fix states the property — a COMMENT standing where a REQUEST_CHANGES would have been — as a derivation from The verdict is approve, but the unit suite on the reviewed commit is still in flight — approving now would attest to a result that does not exist yet. Approval is deferred until CI lands green on this commit (marker below); the finalize job performs it, and withholds it if anything lands red or the head moves. 中文说明置信度：5/5 —— 已观测到的故障、最小修复、经逐路径验证与既有路径等价的条件，以及在旧代码下不可能通过的测试。 退一步看：这正是该模块自身历史所要求的那类修复。clause 7 是只有两条软化路径时写下的枚举；第三条路径绕过它上线，把 blocker 的唯一副本从发出的评论中悄悄抹掉。修复把该性质——"站在 REQUEST_CHANGES 位置上的 COMMENT"——表述为从 结论是批准，但被审 commit 上的单元测试仍在运行——现在批准等于为一个尚不存在的结果背书。批准推迟到 CI 在该 commit 上变绿后执行（见下方标记），由 finalize 任务完成；若有检查变红或 head 移动则不会执行。 — Qwen Code · qwen3.8-max Reviewed at |
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.
Not explored to full depth (tool budget reached): "agent 3b": ran the new regression test — blocked: the worktree has no node_modules and the parent checkout's are absent too; npm ci + prerequisite build skipped as dis…; "agent 6a": run the new vitest case ( compose-review.test.ts ) to confirm green — worktree has no node_modules and a full install + workspace build exceeded the tool budg….
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 3b"：ran the new regression test — blocked: the worktree has no node_modules and the parent checkout's are absent too; npm ci + prerequisite build skipped as dis…；"agent 6a"：run the new vitest case ( compose-review.test.ts ) to confirm green — worktree has no node_modules and a full install + workspace build exceeded the tool budg…。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Comment on lines
+3587
to
+3589
| '- **File:** src/pay.ts:42\n' + | ||
| '- **Issue:** off-by-one in the retry cap\n' + | ||
| '- **Severity:** Critical — [unverified]\n', |
Collaborator
There was a problem hiding this comment.
[Suggestion] The test re-writes the TAGGED findings fixture inline, but it already exists in this same file as TAGGED (~lines 6999-7002) with the findingsFile() helper (~lines 6993-6997). The content is byte-for-byte identical; the filename difference is immaterial (dir is a fresh mkdtempSync per test, and composeReview reads only the explicit findingsPath). — Failure scenario: if the tag format ever changes, the tag-cap describe block's TAGGED constant gets updated and this inline copy can be forgotten → the regression test exercises an input that no longer matches what the review loop actually writes.
Witness: probe — hoisted findingsFile()/TAGGED to file scope and switched this test to findingsPath: findingsFile(TAGGED); 345/345 pass (coveredPlan() is already file-scoped, so there is precedent).
Suggested fix: hoist findingsFile() and TAGGED to file scope (next to coveredPlan()), then use findingsPath: findingsFile(TAGGED) here and drop the inline join/writeFileSync block.
中文说明
该测试以内联方式重写了一份 TAGGED findings 夹具，但同一文件中已存在 TAGGED（约 6999-7002 行）及 findingsFile() 辅助函数（约 6993-6997 行）。两者内容逐字节相同；文件名差异无关紧要（dir 是每个测试新建的 mkdtempSync 目录，composeReview 只读取显式传入的 findingsPath）。失败场景：若 tag 格式日后变更，tag-cap describe 块中的 TAGGED 常量会被更新，而这份内联副本可能被遗忘 → 该回归测试将使用与评审循环实际写入不再匹配的输入。
证据：探针 —— 将 findingsFile()/TAGGED 提升至文件作用域并把本测试改为 findingsPath: findingsFile(TAGGED)，345/345 全部通过（coveredPlan() 已是文件作用域，有先例）。
建议修复：将 findingsFile() 与 TAGGED 提升至文件作用域（放在 coveredPlan() 旁），此处改用 findingsPath: findingsFile(TAGGED)，并删除内联的 join/writeFileSync 块。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Comment on lines
+3305
to
3307
| if (baseEvent === 'REQUEST_CHANGES' && event === 'COMMENT') { | ||
| clauses.push(...bodyCriticalBlock); | ||
| } |
Collaborator
There was a problem hiding this comment.
[Suggestion] The presubmit recovery arm (lines 2153-2157) still enumerates only 1); may reasonably be deferred to a follow-up. — Failure scenario: a run whose findings file still carries criticalsUnverified — the same enumeration disease this clause derives around. When findings-tag softening (findingsUnverifiedAtCompose, which sets neither legacy flag) and presubmit's downgradeRequestChanges both hold, the arm matches neither disjunct, so the presubmit reasons never reach the body or the verdict line — exactly the silent vanishing its own comment forbids. Pre-existing (identical at HEAD— [unverified] at compose time (verifier clean) plus downgradeRequestChanges: true (self-PR / failing CI) → downgraded/downgradedFrom stay null → the "Downgraded from Request changes to Comment: " sentence and its reasons are lost from both the posted body and the terminal verdict line.
Witness: probe — PROBE-A (tag path): downgraded:false, downgradedFrom:null, bodyHasPresubmitReason:false, while the criticalsUnverified control twin in the identical run posts the reasons; a one-line derived fix flips PROBE-A to downgraded:true with the reasons in the body, and all 345 tests stay green with it.
Suggested fix: (event === 'REQUEST_CHANGES' || (baseEvent === 'REQUEST_CHANGES' && event === 'COMMENT')) && downgradeRequestChanges, plus a sibling regression test mirroring the one at ~line 3531 with a tagged findings file instead of an absent verifier.
中文说明
presubmit 恢复分支（约 2153-2157 行）仍只枚举 criticalsUnverified —— 与本子句以推导替代的枚举同病。当 findings 标签软化（findingsUnverifiedAtCompose，两个旧标志均不设置）与 presubmit 的 downgradeRequestChanges 同时成立时，该分支两个析取项都不匹配，presubmit 的理由既进不了正文也进不了裁决行 —— 正是其自身注释所禁止的「静默消失」。属既有问题（HEAD~1 相同）；可合理地推迟到后续 PR。失败场景：一次 compose 时 findings 文件仍带 — [unverified]（验证器干净）且 presubmit 判定 downgradeRequestChanges: true（自身 PR / CI 失败）的运行 → downgraded/downgradedFrom 保持 null → 「Downgraded from Request changes to Comment: 」一句及其理由同时从发出的正文与终端裁决行中丢失。
证据：探针 —— PROBE-A（tag 路径）：downgraded:false, downgradedFrom:null, bodyHasPresubmitReason:false，而同一运行中 criticalsUnverified 对照组能正常发出理由；一行推导式修复即可将 PROBE-A 翻转为 downgraded:true 且理由进入正文，且全部 345 个测试仍为绿。
建议修复：改为 (event === 'REQUEST_CHANGES' || (baseEvent === 'REQUEST_CHANGES' && event === 'COMMENT')) && downgradeRequestChanges，并仿照约 3531 行的测试补一个以带标签 findings 文件替代「验证器缺席」的姊妹回归测试。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| expect(r.event).toBe('COMMENT'); | ||
| // Neither flag the old condition listed is set on this path. | ||
| expect(r.downgradedFrom).toBeNull(); | ||
| expect(r.cappedBy).toContain('findings-unverified-at-compose'); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The test pins only one of the two flags its comment claims absent: it never asserts criticalsUnverified is false. All five assertions are satisfiable with criticalsUnverified === true (both cap names are pushed independently, compose-review.ts ~2040-2043), and the sibling test 'softens a Request changes whose blockers are non-deterministic' (~line 7058) pins exactly this premise under the same harness. — Failure scenario: a future drift flipping criticalsUnverified true in this fixture (digest keying in verificationGaps/recordStep45, the coveredPlan transcript shape) leaves the test green — and would also leave it green against the reverted old condition (that flag satisfies || criticalsUnverified) → the 239-character-body bug this PR fixes regresses with no red test.
Witness: probe — reverted condition + criticalsUnverified flipped true passes all five original assertions; adding the suggested assertion turns it red (expected [ 'unreviewed-dimension', …(2) ] to not include 'criticals-unverified'); with the PR's code it passes.
Suggested change
| expect(r.cappedBy).toContain('findings-unverified-at-compose'); | |
| expect(r.cappedBy).toContain('findings-unverified-at-compose'); | |
| expect(r.cappedBy).not.toContain('criticals-unverified'); |
中文说明
该测试只钉住了其注释声称缺席的两个标志之一：它从未断言 criticalsUnverified 为 false。当 criticalsUnverified === true 时全部五条断言仍可满足（两个 cap 名称是独立推入的，见 compose-review.ts 约 2040-2043 行），而姊妹测试「softens a Request changes whose blockers are non-deterministic」（约 7058 行）在同一夹具下恰好钉住了这一前提。失败场景：未来某次漂移使该夹具中 criticalsUnverified 变为 true（verificationGaps/recordStep45 的摘要键控、coveredPlan 的 transcript 形态）时，测试仍然为绿 —— 而且对回退后的旧条件同样为绿（该标志满足 || criticalsUnverified）→ 本 PR 修复的 239 字符正文问题会在没有任何红色测试的情况下回归。
证据：探针 —— 回退条件并把 criticalsUnverified 翻为 true 后，原有五条断言全部通过；加上建议的断言后变红（expected [ 'unreviewed-dimension', …(2) ] to not include 'criticals-unverified'）；在 PR 代码下该断言通过。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Comment on lines
+3305
to
3306
| if (baseEvent === 'REQUEST_CHANGES' && event === 'COMMENT') { | ||
| clauses.push(...bodyCriticalBlock); |
Collaborator
There was a problem hiding this comment.
[Suggestion] This widening falsifies the pre-existing comment at compose-review.test.ts:6734-6737, which still asserts the pre-fix behavior: "(clause 7 rides on criticalsUnverified); the findings-file tag route caps without that flag, so its body carries no blocker and cannot reach the cut at all." Post-fix, the tag route's body DOES carry the blocker (this PR's own new test asserts it) and CAN reach the trim cut — the same file now carries two contradictory descriptions of the tag route. Anchored here because the comment sits outside the diff hunks and this condition change is what invalidates it. — Failure scenario: a maintainer extending the truncation oracles reads the stale comment, concludes a tag-route body can never contain a blocker and never reach the trim cut, and reasons about keep/trim ranking — or "simplifies" an oracle — from pre-fix semantics.
Witness: probe — a tag-route body with a 70k-char blocker reaches the trim cut (r.bodyTrim.truncated === true) under the fix; reverting clause 7 to the pre-fix condition flips the probe (no blocker posted, no truncation).
Suggested fix: rewrite that comment paragraph to the post-fix semantics, e.g. (clause 7 rides on the softened RC→COMMENT transition); the findings-file tag route softens the same way since that fix, so its body carries the blocker too and can reach the cut.
中文说明
本次放宽使 compose-review.test.ts:6734-6737 处的既有注释失真：该注释仍断言修复前的行为 ——「(clause 7 rides on criticalsUnverified); the findings-file tag route caps without that flag, so its body carries no blocker and cannot reach the cut at all.」。修复后，tag 路径的正文确实会携带阻断项（本 PR 自己的新测试就是这样断言的），也确实能到达裁剪切口 —— 同一文件现在对该路径携带了两处相互矛盾的描述。锚定在此是因为该注释位于 diff 块之外，而使其失效的正是这处条件变更。失败场景：维护者扩展截断预言时读到这条陈旧注释，认定 tag 路径正文永远不会包含阻断项、也永远不会到达裁剪切口，从而基于修复前的语义推理 keep/trim 排序，甚至「简化」某个预言。
证据：探针 —— 修复后，携带 7 万字符阻断项的 tag 路径正文会到达裁剪切口（r.bodyTrim.truncated === true）；将 clause 7 回退为修复前条件后探针翻转（阻断项未发出、无截断）。
建议修复：将该注释段落改写为修复后的语义，例如：(clause 7 rides on the softened RC→COMMENT transition); the findings-file tag route softens the same way since that fix, so its body carries the blocker too and can reach the cut.
— qwen3.8-max via Qwen Code /review (v0.21.13)
Collaborator
Author
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
Collaborator
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
Collaborator
Collaborator
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #9416Triage: four inline [Suggestion] findings from the automated reviewer (COMMENTED review; no Critical/Request-changes findings, no failed checks, Findings and dispositions
Conflict notes
VerificationCommit:
中文说明Autofix 评审轮次 — PR #9416分诊结果：来自自动评审器的四条行内 [Suggestion] 发现（COMMENTED 评审；无 Critical/Request-changes 发现，无失败检查项， 各发现的处置
冲突说明
验证提交：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
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 explored to full depth (tool budget reached): "agent 1c": none — no checks were cut short..
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/compose-review.ts:3305 — [probe] verdictLine's first branch still enumerates the two unverified-family caps instead of the derived RC→COMMENT transition
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 1c"：none — no checks were cut short.。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Comment on lines
+2159
to
2160
| // A softening cap moved the event first, but the presubmit | ||
| // still ruled: without this arm its reasons (self-PR, failing CI) would |
Collaborator
There was a problem hiding this comment.
[Suggestion] The derived treatment this diff gives the RC presubmit arm is not carried to the sibling arm directly above it: if (event === 'APPROVE' && downgradeApprove) (~line 2150) still tests the post-cap event. The APPROVE→COMMENT cap (~line 2111) runs before the presubmit arms, so any cap that softens a zero-finding Approve first (contextUnavailable, an unreviewed dimension, a cannot-tell existing Critical, a surviving findings-file tag) fails arm 1's gate — and arm 2's baseEvent === 'REQUEST_CHANGES' gate fails too — leaving downgraded/downgradedFrom false/null, so the presubmit reasons (self-PR, failing CI) vanish from both the body and the verdict line. The behavior predates this diff and its reachability is unchanged (hence Suggestion); it is in scope as the fourth member of the predicate family this diff converts from enumeration to derivation, and because the comment this diff rewrites now asserts the invariant — "without this arm its reasons … would silently vanish" — that the chain's other arm still violates. — Failure scenario: a self-PR review whose run also trips any cap over a zero-finding verdict → the event is softened to COMMENT before the arms run → no "
Witness (probe in the real vitest harness):
capped: event=COMMENT baseEvent=APPROVE downgraded=false downgradedFrom=null cappedBy=["context-unavailable"]
verdictLine: Verdict: Comment — an Approve was NOT available: the PR's existing discussion could not be read
body: no downgrade clause, no 'self-PR'
control: downgraded=true downgradedFrom=Approve
verdictLine: … — an Approve was NOT available: a presubmit check failed
body: ⚠️ Downgraded from Approve to Comment: self-PR.
Applying the derived gate flips the capped arm to downgraded=true downgradedFrom=Approve with both surfaces restored; the full suite stays green.
Suggested fix (two locations — arm 1's condition, plus a regression test pairing a capped zero-finding run with downgradeApprove: true):
if (
(event === 'APPROVE' ||
(baseEvent === 'APPROVE' && event === 'COMMENT')) &&
downgradeApprove
) {
downgraded = true;
downgradedFrom = 'Approve';
}中文说明
本 diff 给 RC presubmit 分支的推导式处理没有传导到它正上方的姊妹分支：if (event === 'APPROVE' && downgradeApprove)（约 2150 行）仍在检测 cap 之后的 event。APPROVE→COMMENT 的 cap（约 2111 行）先于 presubmit 分支执行，因此任何先把零发现 Approve 软化的 cap（contextUnavailable、未审查维度、cannot-tell 的既有 Critical、findings 文件残留标签）都会使分支 1 的门槛失败——分支 2 的 baseEvent === 'REQUEST_CHANGES' 门槛同样失败——downgraded/downgradedFrom 保持 false/null，presubmit 的理由（自身 PR、CI 失败）从正文与裁决行两处同时消失。该行为在本 diff 之前已存在且可达性未变（故为 Suggestion）；它属于本 diff 从枚举转为推导的谓词家族的第四个成员，且本 diff 重写的注释现在断言了「没有该分支其理由会静默消失」这一不变量——链条的另一分支仍然违反它。失败场景：一次自身 PR 的评审在零发现裁决上同时触发任一 cap → event 在分支执行前被软化为 COMMENT → 正文没有「
证据（真实 vitest 探针）：cap 组 downgraded=false downgradedFrom=null cappedBy=["context-unavailable"]，裁决行只剩「an Approve was NOT available: the PR's existing discussion could not be read」，正文无降级子句、无 'self-PR'；同一 presubmit 的无 cap 对照组 downgraded=true downgradedFrom=Approve，两处均正常渲染。应用推导式门槛后 cap 组翻转为 downgraded=true，两处恢复；全套测试保持绿。
建议修复（两处——分支 1 的条件，外加一个把「带 cap 的零发现运行 + downgradeApprove: true」配对的回归测试）：
if (
(event === 'APPROVE' ||
(baseEvent === 'APPROVE' && event === 'COMMENT')) &&
downgradeApprove
) {
downgraded = true;
downgradedFrom = 'Approve';
}— qwen3.8-max via Qwen Code /review (v0.21.13)
doudouOUC
reviewed
Aug 18, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Review
Summary
Minimal fix for a real bug: clause 7 in compose-review enumerated two softening flags (downgradedFrom === 'Request changes' || criticalsUnverified), and a third path — the findings-file — [unverified] tag — shipped past both, causing a blocker's only copy to silently vanish from the posted COMMENT body. The fix replaces the enumeration with the derivation baseEvent === 'REQUEST_CHANGES' && event === 'COMMENT' and carries the same derivation to the presubmit reasons arm.
Correctness
The new condition is equivalent to the old on every existing path:
downgradedFrom === 'Request changes'is only set by the presubmit carve-out, wherebaseEvent === 'REQUEST_CHANGES'andevent === 'COMMENT'— both hold under the new condition.criticalsUnverifiedrequirescriticalsNeedingVerify ≥ 1, which impliesbodyCriticals.length > 0, which forcesbaseEvent === 'REQUEST_CHANGES'. The event is alreadyCOMMENT(the verification cap softened it). So both hold under the new condition.- The findings-tag path:
baseEventisREQUEST_CHANGES,eventisCOMMENT, and neither old flag is set. This is the exact gap the PR closes.
The new condition strictly adds coverage for the findings-tag path and any future softening path, while preserving all existing behavior. The bodyCriticalBlock is empty when bodyCriticals is empty, so the widened condition is harmless when no blockers exist.
Production code
compose-review.ts line 3291 — clause 7 condition: the comment now correctly explains the derivation rationale and the observed failure mode. The old comment named the two paths; the new one states the invariant.
compose-review.ts line 2153 — presubmit reasons arm: same derivation fix. The previous condition criticalsUnverified was also an enumeration that missed the findings-tag + presubmit combined path. The new condition event === 'COMMENT' is correct because the presubmit reasons block is only rendered when downgraded is true and downgradeReasons is non-empty.
Test quality
All four tests mentioned in the PR description are present and well-constructed:
-
"keeps the presubmit downgrade reasons when the findings-tag cap also holds" — pins the combined path (presubmit + findings-tag). Would have failed under the old condition because the presubmit reasons arm required
criticalsUnverified. -
"keeps the body Criticals when the FINDINGS-TAG cap softens the event" — pins the isolating shape. Asserts
downgradedFrom: null,cappedBy: [findings-unverified-at-compose], notcriticals-unverified. This is the test that cannot pass under the old code. The additionalcappedByassertion (added in commit 2) addresses the previous review finding about pinning only one flag. -
"keeps the body Criticals when the unverified cap softens the event" — pre-existing regression test for the
criticals-unverifiedpath. -
Body-limit test comment — updated to reflect that the findings-tag path now also carries blockers.
The findingsFile() helper and TAGGED constant are properly hoisted to module scope (addressing R1-1 from the previous review).
Previous review findings
All five findings from the two earlier review rounds have been addressed:
| Finding | Status |
|---|---|
| R1-1: TAGGED fixture inline | ✅ Fixed — moved to module scope |
| R1-2: Presubmit arm enumeration | ✅ Fixed — derived from event, new test added |
| R1-3: Only one flag pinned | ✅ Fixed — cappedBy negation added |
| R1-4: Comment falsified by widening | ✅ Fixed — comment updated |
| R2-1: Presubmit arm not carried | ✅ Fixed — same derivation applied to the presubmit reasons arm |
Verdict
✅ Looks good. The problem is real (observed in production), the fix is minimal and correct, the tests genuinely pin the new behavior, and all previous review feedback has been incorporated. No correctness, security, or convention issues remain.
— Reviewed at 12aabd4bdfd90611b675a997db325e1cee208549
Collaborator
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： PR #9416 — review round 2 summaryFeedback and decisions
Changes
Conflict notesNone ( Verification
中文说明PR #9416 — 评审第 2 轮总结反馈与决定
变更
冲突说明无（ 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
approved these changes
Aug 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! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.8-max via Qwen Code /review (v0.21.13)
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
4 participants
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
Makes
compose-reviewrender the body copy of a blocker on every COMMENT that stands where a REQUEST_CHANGES would have been, instead of on the two softening paths that were listed by name.Clause 7 has always meant "this COMMENT replaced a Request changes, so the blocker's only copy must still reach the author". It asked that question as an enumeration of the flags known when it was written:
It now asks it as a derivation:
baseEventis the row before every cap and downgrade, so this covers the softening paths that exist and the ones that do not exist yet.Why it's needed
A third softening path shipped past the enumeration. The findings-file
— [unverified]tag softens a Request changes at the event line, and it sets neither flag the condition read — notdowngradedFrom(only the presubmit carve-out sets that), and notcriticalsUnverified(only the verification-delivery gate sets that).So a run whose coverage was proven and whose verifier ran, holding one unanchorable blocker, posted this — 239 characters, no blocker anywhere in it:
while
deferredCount, the verdict line and the saved artifact all counted the blocker. The body copy is the only copy an unanchorable Critical has: nothing else on the PR page carries it. This is the failure the clause's own comment names — "softening the event must never erase it" — reached through the one door the condition did not name.Same closure the module applied to the deferral channel after #9095: state the property, do not enumerate its instances.
Reviewer Test Plan
How to verify
— [unverified], onebodyCriticalsentry →baseEventisREQUEST_CHANGES,eventisCOMMENT,downgradedFromisnull,cappedBycontainsfindings-unverified-at-compose, and the body carries**[Critical]** …. That is the new test.cd packages/cli && npx vitest run src/commands/review/Evidence (Before & After)
N/A — pipeline behaviour, no TUI change. Before: the blocker was absent from the posted body on this path (probe output above). After: it rides, disclosed as unverified like its siblings.
Tested on
Environment (optional)
Unit tests per package; typecheck and the review suites verified in a clean detached worktree. Both mutations of the new condition — reverting it to the enumeration, and dropping the clause — redden the tests that pin them.
Risk & Scope
Linked Issues
Found while reviewing #9247.
中文说明
本 PR 做了什么
让
compose-review在任何「本该是 REQUEST_CHANGES 却被软化为 COMMENT」的情形下都渲染阻断项的正文副本，而不是只覆盖被逐个列名的两条路径。clause 7 想问的一直是「这条 COMMENT 是否顶替了一个 Request changes」，但它此前用枚举当时已知的标志来回答：
downgradedFrom === 'Request changes' || criticalsUnverified。现在改为推导：baseEvent === 'REQUEST_CHANGES' && event === 'COMMENT'——baseEvent是所有 cap 与 downgrade 之前的那一行，因此既覆盖现有路径，也覆盖尚不存在的路径。为什么需要
第三条软化路径绕过了该枚举：findings 文件仍带
— [unverified]标签会在 event 处把 Request changes 软化为 COMMENT，而它两个标志都不设——downgradedFrom只由 presubmit 分支设置，criticalsUnverified只由验证交付闸设置。于是一次覆盖已被证明、验证器也确实运行过、且持有一条不可锚定阻断项的运行，发出的正文只有 239 字符，其中没有任何阻断项，而
deferredCount、裁决行与已保存工件都把它计入了。不可锚定的 Critical 只有正文这一份副本，PR 页面上别无他处承载。这正是该子句注释自己所说的失败——「softening the event must never erase it」——从它没有点名的那扇门进来。与 #9095 之后对延后通道所做的闭合同理：陈述性质，而不是枚举其实例。
评审验证计划
见上方英文部分：隔离形态（覆盖已证明 + 验证器在案 + findings 仍带标签 + 一条 body Critical）、两条既有路径的回归、以及套件命令。两个变异（改回枚举、删掉该子句）都会让对应测试变红。
风险与范围
关联 Issue
在评审 #9247 期间发现。