fix(review): stop the fallback comment from firing on superseded runs - #9716
Merged
Conversation
The fallback-comment gate admitted every review-pr result of 'cancelled' on the assumption that a run-level cancel takes the queued fallback job down with it, leaving job-level timeout as the only live source of a cancelled review step. That assumption does not hold: the job is guarded by always(), which keeps it running through a run-level cancel. On PR #9131 a same-head pull_request_target pair started 1s apart, the newer run cancelled the older inside authorize, and the older run posted a false "review did not complete" (run 32558544379) while the surviving run was still reviewing — same head, so the in-step head-moved guard could not catch it. The two cancels are separable through needs: a job-level timeout cancels review-pr alone while its upstream jobs completed long before, whereas a run-level cancel sweeps the whole chain. 'cancelled' now opens the gate only when neither authorize nor delay-automatic-review was itself cancelled.
Collaborator
Collaborator
|
Re-run after three follow-up commits ( Template looks good ✓ Problem: observed, not theoretical. The false "review did not complete" comment on #9131 (the comment, run 32558544379 — re-verified via API this pass: Direction: aligned — this is the repo's own review pipeline, and not posting stale retry guidance is the fallback gate's entire reason to exist. Size: N/A — one workflow file (+25/−7) and its resilience test (+22/−3); no core paths. Approach: minimal. One compound-clause refinement to the Risk: no elevated risk signals (no high-risk path matches). Moving on to code review. 🔍 中文说明本次为追加三个提交（ 模板完整 ✓ 问题： 已观测，非理论。#9131 上那条错误的 "review did not complete" 评论（评论，run 32558544379——本轮已重新经 API 核实： 方向： 对齐——这是仓库自己的评审流水线，不发过期重试提示正是兜底门存在的意义。 规模： 不适用——一个工作流文件（+25/−7）加其韧性测试（+22/−3）；不触及核心路径。 方案： 最小化。 风险： 无升级风险信号（未命中高风险路径）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Code review — re-reviewed at the new head. The gate clause is unchanged since One correction to my previous pass, prompted by the /review round-4 deferred finding that the compound clause "misses a review-config cancel conjunct". I had claimed the authorize check subsumes review-config by dependency propagation. That mechanism is wrong —
The rest holds as before: superseded run (the #9131 shape) closes the gate — authorize or delay is cancelled; a job-level timeout of review-pr still opens it — upstream intact; failure clauses and resolve exclusions untouched; the residual window (run-level cancel after the chain finished) behaves exactly as pre-PR, and for same-head supersedes it is unreachable behind the 10-minute delay wait. The new test pins match the YAML block-scalar rendering byte-for-byte (I checked the indent arithmetic: 8-space source lines render to 2 leading spaces), and the occurrence-count pin catches the merge-conflict re-add of the bare disjunct that a Both /review round-4 deferred findings verified and correctly deferred: the review-config conjunct (above — correctly absent), and the Test evidence — the PR's own CI on the reviewed commit, read via API (this run is static; no PR code executed):
macOS/Windows test legs and integration tests are skipped on this commit by path filters. The one non-green check on the commit — a There is no sandboxed lane to name here: the claim is about Actions gate semantics that only execute on real cancellations — neither Not verified: the new gate firing on a live cancel — observable only the next time a real supersede happens. 中文说明代码审查 —— 在新 head 上复审：门条件子句自 更正上一轮的一处论证（源于 /review 第 4 轮"复合子句缺 review-config 取消联言"的延后发现）：我曾称 authorize 检查经依赖传递覆盖 review-config——机制错误， 其余结论同前：被顶替 run 门关闭；job 级超时候选门照常放行；各 failure 子句与 resolve 排除未动；残余窗口行为与改动前一致，同头顶替被 10 分钟 delay 等待挡在身后。新测试钉与 YAML 块标量渲染逐字节吻合（已核算缩进：8 空格源行渲染为 2 个前导空格），出现次数钉能抓住单靠 toContain 会漏掉的合并冲突回添。该套件经 /review 第 4 轮两条延后发现均已核实、延后处理正确：review-config 联言（见上，正确缺失）； 测试证据 —— 被审提交自身的 CI（经 API 读取，纯静态审查，未执行任何 PR 代码）：上方表格内全部通过；macOS/Windows 单测腿与集成测试被路径过滤跳过。提交上唯一的非绿检查——cancelled 的 无沙箱验证通道可指定：断言关于 Actions 门语义，只在真实取消时执行， 未验证：新门在真实取消下的线上行为——只能等下一次真实顶替发生时观察。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 5/5 — observed incident with linked evidence, the cheapest viable fix, now pinned by tests that fail on any plausible revert, and gate semantics that hold under every trigger shape I could construct. Stepping back: the false comment on #9131 is precisely the failure mode the fallback gate exists to prevent, and the root cause is diagnosed correctly — CI on the reviewed commit is fully green — both PR-CI workflows completed ( 中文说明置信度：5/5 —— 问题有链接为证的真实观测，修复是最廉价的可行方案，现已有对任何合理回退都会失败的测试钉，门语义在我能构造的所有触发形态下均成立。 退一步看：#9131 的误评正是兜底门本应防止的失效模式，根因诊断正确—— 被审提交上 CI 全绿——两个 PR CI 工作流均已完成（ — 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
previously approved these changes
Aug 22, 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 — CI landed green after the review. ✅
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.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
已审查——无阻断问题。 建议见行内评论。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Comment on lines
+1934
to
+1936
| (needs.review-pr.result == 'cancelled' && | ||
| needs.authorize.result != 'cancelled' && | ||
| needs.delay-automatic-review.result != 'cancelled') || |
Collaborator
There was a problem hiding this comment.
[Suggestion] The new upstream-cancel guard on this gate is pinned by no test. The resilience suite in scripts/tests/qwen-pr-review-workflow.test.js (block 'fallback comment resilience (PR #8894 incident class)') promises "reverting any hunk must fail here", but its only cancelled-related assertion — toContain("needs.review-pr.result == 'cancelled'") — survives verbatim inside this new compound clause, so it passes identically for the old and the new gate. If a future change reverts the two != 'cancelled' conjuncts, the exact #9131 regression this PR fixes returns — a concurrency-superseded run posting "review did not complete" while its same-head twin is still reviewing — and the suite stays green. Verified by mutation: Tests 174 passed (174) on the PR gate and Tests 174 passed (174) on a reverted-gate mutant, while a comparator probe pinning the two conjuncts fails on the mutant and passes on the PR gate. The test's rationale comment (~lines 3070–3073) also still asserts the premise this PR falsifies ("a run-level cancel cancels this queued job with it"). Consider pinning the new shape (e.g. toContain("needs.authorize.result != 'cancelled'") and toContain("needs.delay-automatic-review.result != 'cancelled'")) and refreshing that comment.
中文说明
新增的上游取消守卫没有任何测试固定。scripts/tests/qwen-pr-review-workflow.test.js 中的 'fallback comment resilience (PR #8894 incident class)' 测试块声明 "reverting any hunk must fail here"，但其唯一与 cancelled 相关的断言 —— toContain("needs.review-pr.result == 'cancelled'") —— 在新的复合子句中依然逐字存在，因此新旧 gate 都能通过同一条断言。如果未来还原这两个 != 'cancelled' 条件，本 PR 修复的 #9131 误报回归（被并发组顶掉的 run 在同 head 的幸存 run 仍在评审时发出 "review did not complete"）将悄然重现，而测试套件仍是绿色。已通过变异验证：PR gate 下 Tests 174 passed (174)；还原 gate 的变异体同样 Tests 174 passed (174)；而固定这两个条件的对比探针在变异体上失败、在 PR gate 上通过。另外，该测试约第 3070–3073 行的注释仍在断言本 PR 已推翻的前提（"run 级取消会连带取消排队中的 fallback job"）。建议固定新的子句形状（例如 toContain("needs.authorize.result != 'cancelled'") 和 toContain("needs.delay-automatic-review.result != 'cancelled'")），并更新该注释。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Collaborator
Author
There was a problem hiding this comment.
Landed in 49e92078fb.
Pinned the full compound clause — grouping included — rather than the two conjuncts separately, so dropping either != 'cancelled' conjunct or the parenthesization fails the gate test, and rewrote the rationale comment that still asserted the falsified premise (a run-level cancel taking the queued fallback job down with it).
Mutation-verified locally the same way as the review's probe: the gate test fails on a reverted-gate mutant and passes on the PR gate. The two runHealthProbe failures a root shell shows locally are environmental (chmod-based unwritability is void for root) and green in CI.
Review R1-1: the resilience suite's only cancelled-related assertion —
toContain("needs.review-pr.result == 'cancelled'") — matched the old and
the new gate alike, so reverting the two != 'cancelled' conjuncts would
bring back the #9131 false-fallback regression with the suite green.
Pin the full compound clause, grouping included, so dropping either
conjunct (or the parenthesization) fails the test, and rewrite the
rationale comment that still asserted the falsified premise that a
run-level cancel takes the queued fallback job down with it.
Mutation-verified: the gate test fails on a reverted-gate mutant and
passes on the PR gate.
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.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
.github/workflows/qwen-code-pr-review.yml:1917 — [review] Comment overstates the gate: a late run-level cancel still opens it, and suppression relies on the head-moved guard the comment never mentions
中文说明
已审查——无阻断问题。 建议见行内评论。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Comment on lines
+3080
to
+3084
| expect(job.if).toContain( | ||
| "(needs.review-pr.result == 'cancelled' &&\n" + | ||
| " needs.authorize.result != 'cancelled' &&\n" + | ||
| " needs.delay-automatic-review.result != 'cancelled') ||", | ||
| ); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The pin proves the compound clause is present in fallback-comment.if, but nothing proves the old bare needs.review-pr.result == 'cancelled' || disjunct is absent — the replaced assertion would still pass on the new workflow, because that substring survives inside the compound clause. A merge-conflict resolution that keeps both sides of this hunk re-adds the bare disjunct beside the intact compound clause; the gate then opens on every cancelled review-pr again and the false "review did not complete" fallback comment returns on concurrency-superseded runs (the PR #9131 incident class), while this pin and every other assertion stay green. A scratch-tree probe confirmed the mutant: with the bare disjunct re-added the full suite still reports Tests 174 passed (174); adding the negative assertion below makes that mutant fail (expected … not to contain 'needs.review-pr.result == 'cancelled' ||') while the pristine workflow still passes 174/174.
Suggested change
| expect(job.if).toContain( | |
| "(needs.review-pr.result == 'cancelled' &&\n" + | |
| " needs.authorize.result != 'cancelled' &&\n" + | |
| " needs.delay-automatic-review.result != 'cancelled') ||", | |
| ); | |
| expect(job.if).toContain( | |
| "(needs.review-pr.result == 'cancelled' &&\n" + | |
| " needs.authorize.result != 'cancelled' &&\n" + | |
| " needs.delay-automatic-review.result != 'cancelled') ||", | |
| ); | |
| expect(job.if).not.toContain("needs.review-pr.result == 'cancelled' ||"); |
中文说明
这个固定断言只能证明复合子句存在于 fallback-comment.if，但无法证明旧的裸 needs.review-pr.result == 'cancelled' || 析取项不存在——被替换的断言在新工作流上依然会通过，因为该子串仍包含在复合子句内部。如果某次 merge 冲突解决同时保留了本 hunk 的两侧内容，裸析取项就会与完整的复合子句并存；门控将再次在所有被取消的 review-pr 上放行，并发顶替场景下错误的 "review did not complete" 兜底评论会复发（即 PR #9131 事故类别），而本断言和所有其他断言仍全部为绿。临时工作树中的探针确认了该变异体：重新加回裸析取项后完整测试套件仍报告 Tests 174 passed (174)；加入上面的负向断言后该变异体失败（expected … not to contain 'needs.review-pr.result == 'cancelled' ||'），而原始工作流仍 174/174 通过。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Collaborator
Author
There was a problem hiding this comment.
Landed in 36b9a5b699 — applied the suggested negative assertion verbatim, with a comment noting why the negation holds on the intended gate (inside the compound clause the substring is followed by &&).
Also took this round's deferred note in the same commit: the job comment now states that a run-level cancel landing after the upstream chain finished still opens the gate, that the push-supersede flavor is then suppressed by the in-step head-moved guard, and why a same-head twin cannot land that late (its cancel fires at run creation).
Re-verified by mutation: with the bare disjunct re-added beside the intact compound clause the gate test fails on the new negation; the pristine workflow passes the suite at parity with base.
Review R2-1: the compound-clause pin proves presence, not absence — a merge-conflict resolution keeping both sides of the gate hunk re-adds the bare "== 'cancelled' ||" disjunct beside the intact compound clause, reopening the gate on every cancelled review-pr with the suite green. Add the negative assertion; inside the compound clause the substring is followed by ' &&', so it holds on the intended gate. Also take the round-2 deferred note: the job comment now says a run-level cancel landing after the upstream chain finished still opens the gate and is suppressed by the in-step head-moved guard, and why a same-head twin cannot land that late. Mutation-verified: with the bare disjunct re-added the gate test fails on the negation; the pristine workflow passes.
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": live run of scripts/tests/qwen-pr-review-workflow.test.js (worktree has no node_modules ; vitest failed with ERR_MODULE_NOT_FOUND — the new pins were verifie…; "agent 4": none — all reads completed; nothing was cut short.; "agent 1a": none — one check was substituted rather than cut: the full vitest run was replaced by a faithful parse-and-assert simulation of the two changed assertions again….
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 3b"：live run of scripts/tests/qwen-pr-review-workflow.test.js (worktree has no node_modules ; vitest failed with ERR_MODULE_NOT_FOUND — the new pins were verifie…；"agent 4"：none — all reads completed; nothing was cut short.；"agent 1a"：none — one check was substituted rather than cut: the full vitest run was replaced by a faithful parse-and-assert simulation of the two changed assertions again…。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Comment on lines
+1919
to
+1921
| # flavor is then suppressed by the in-step head-moved guard, and a | ||
| # same-head twin cannot land that late — its cancel fires at run | ||
| # creation, seconds in. A manual run-cancel during the delay window goes |
Collaborator
There was a problem hiding this comment.
[Suggestion] The rewritten rationale comment's enumeration of late (post-chain) run-level cancels is incomplete: pull_request_target: closed is a same-head cancel that can land hours into the review. The closed-action run joins the PR-scoped concurrency group with cancel-in-progress and cancels the in-flight run while authorize/delay-automatic-review already show 'success' — so the new compound clause opens the gate and the head-moved guard passes (the head is unchanged). Today the only thing stopping the comment is the in-step pr_state != "OPEN" check, which this sentence never credits. A future maintainer loosening that state check (say, to post informational comments on closed PRs) or refactoring the gate on the strength of this comment re-opens a false "review did not complete" comment on closed PRs; and in a close→reopen race within the fallback job's startup window the state check reads OPEN again, so the comment posts while the reopen's fresh review queues.
Suggested change
| # flavor is then suppressed by the in-step head-moved guard, and a | |
| # same-head twin cannot land that late — its cancel fires at run | |
| # creation, seconds in. A manual run-cancel during the delay window goes | |
| # flavor is then suppressed by the in-step head-moved guard, the close | |
| # flavor by the in-step PR-state check, and a same-head twin cannot land | |
| # that late — its cancel fires at run creation, seconds in. A manual | |
| # run-cancel during the delay window goes |
中文说明
重写后的说明注释对"链条完成之后才到达的 run 级取消"的列举不完整：pull_request_target: closed 是一种同 head 取消，可能在评审进行数小时后才到达。closed 触发的 run 会加入 PR 级并发组且 cancel-in-progress 生效，在 authorize/delay-automatic-review 已为 'success' 时取消正在运行的 run——于是新的复合条件会放行 gate，而 head 对比守卫因 head 未变而放行。今天唯一拦住兜底评论的是步骤内的 pr_state != "OPEN" 检查，但这句话并没有提及它。未来若有人放宽该状态检查（比如想在已关闭的 PR 上发提示评论），或凭这句注释重构 gate，就会在已关闭的 PR 上重新放出错误的 "review did not complete" 评论；而在 fallback job 启动窗口内发生"关闭→重新打开"竞态时，状态检查会再次读到 OPEN，评论照样发出，而重新打开触发的新评审还在排队。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Collaborator
Author
There was a problem hiding this comment.
Landed in 46cec25651 — applied the suggestion with the close flavor spelled out inline (a closed-action run joining the PR-scoped group hours in, head unchanged) so the sentence carries the mechanism, not just the credit. Verified the premises before applying: closed is in the pull_request_target types (line 11) and the in-step pr_state != "OPEN" check is the suppressor. The close→reopen race you note is real but left as is — comment-only scope for this round.
| // on every cancelled review-pr again, and the pin above stays green. | ||
| // (Inside the compound clause the substring is followed by ` &&`, so | ||
| // this negation holds on the intended gate.) | ||
| expect(job.if).not.toContain("needs.review-pr.result == 'cancelled' ||"); |
Collaborator
There was a problem hiding this comment.
[Suggestion] This negative assertion only rejects one rendering of the bare disjunct it exists to catch. If a merge-conflict resolution (or a later edit) re-adds the bare cancelled disjunct beside the intact compound clause in a different rendering — (needs.review-pr.result == 'cancelled') ||, or even the same text with different whitespace — the exact substring checked here never appears, so this negation and the positive pin above both stay green while the gate opens on every cancelled review-pr again: a concurrency-superseded run posts a false "did not complete" comment while its surviving twin is still reviewing — the exact #9131 regression — under a suite that reports the gate as pinned. Verified by probe in a scratch tree at 36b9a5b:
parenthesized re-addition beside intact clause: Tests 174 passed (174) <- regression present, suite green
occurrence-count pin + same mutation: AssertionError: expected [ ...(2) ] to have a length of 1 but got 2
occurrence-count pin + intended gate: green
An occurrence-count pin catches any rendering. Keep the compound-clause pin above (it fails on a reverted conjunct); this replaces only the negation:
Suggested change
| expect(job.if).not.toContain("needs.review-pr.result == 'cancelled' ||"); | |
| expect( | |
| job.if.match(/needs\.review-pr\.result == 'cancelled'/g), | |
| ).toHaveLength(1); |
中文说明
这条否定断言只能拦截它要防的那种裸析取写法。如果合并冲突的解决方式（或后续编辑）在完整的复合条件旁边以不同写法重新加回裸的 cancelled 析取——例如 (needs.review-pr.result == 'cancelled') ||，甚至只是空白不同的相同文本——这里检查的精确子串就不会出现，于是这条否定断言和上方的正向 pin 都保持绿色，而 gate 又会对每一个 cancelled 的 review-pr 放行：被并发顶掉的 run 会在幸存 run 仍在评审时发出错误的 "did not complete" 评论——正是 #9131 的回归——而测试套件却显示 gate 已被钉住。已在 36b9a5b 的 scratch tree 中用探针验证（运行输出见上方英文部分）。改用出现次数 pin 可以拦截任何写法；保留上方的复合条件 pin（它在合取被还原时会失败），只替换这条否定断言。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Collaborator
Author
There was a problem hiding this comment.
Landed in 46cec25651 — replaced the negation with the occurrence-count pin as suggested, keeping the compound-clause pin for the conjuncts.
Mutation-verified live (this closes your agent 3b's ERR_MODULE_NOT_FOUND gap — full vitest run, not simulation): three gate mutants each fail the gate test — bare re-addition, the parenthesized rendering that escaped the old negation, and reverted conjuncts — and the pristine workflow passes at base parity (170 passed + the 2 root-only runHealthProbe failures, green in CI).
Review R3-2: the negative assertion rejected only one rendering of the bare disjunct — a parenthesized or respaced re-addition beside the intact compound clause escaped both pins with the gate reopened on every cancelled review-pr. Replace the negation with an occurrence-count pin: exactly one "needs.review-pr.result == 'cancelled'" in the gate catches any rendering, while the compound pin keeps guarding the conjuncts. Review R3-1: the rationale comment's list of late run-level cancels missed the closed-action flavor — a same-head cancel hours in with the upstream chain green, stopped only by the in-step PR-state check the comment never credited. Credit it. Mutation-verified: bare, parenthesized, and reverted-conjunct gate mutants each fail the gate test; the pristine workflow passes at base parity.
qwen-code-ci-bot
approved these changes
Aug 22, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
.github/workflows/qwen-code-pr-review.yml:1940 — [review] compound clause misses review-config cancel conjunctscripts/tests/qwen-pr-review-workflow.test.js:3072 — [probe] always() gate prefix unpinned by any test
中文说明
无阻断问题。LGTM！✅
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Collaborator
Author
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 514 passed · 0 failed · 514 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：514 通过 · 0 失败 · 514 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #9716 Deep Verification — fix(review): stop the fallback comment from firing on superseded runsVerdict: 中文摘要结论：
Central claim + A/BCentral claim: the The gate strings were extracted verbatim from the parsed YAML of both trees (sha256-quoted in the log; no transcription), and evaluated by two independent implementations (a recursive-descent parser and a lexical-substitution compiler) that agreed on every cell. The base worktree reused the root Exhaustive sweep (the A/B)All 4⁵ = 1024 combinations of the five
The flip set is exactly Reviewer Test Plan, per step
Additional boundary rows: authorize-failure plus run-cancel still posts via the untouched failure clause (S6); a healthy run stays closed (S7). One annotation to step 4: a manual cancel of the whole run mid-review (upstream already succeeded) also opens the gate and posts — identical to base, and the YAML comment's list of late run-level cancels names only the push-supersede and close flavors; not a change introduced by this PR. Premise the fix rests on — and the evidence bound to itThe gate distinguishes the two cancels only if a run-level cancel records the swept upstream jobs as Corrections (to description / commit text — not code-change requests)
FindingsN1 (nit) — the occurrence-count pin is spelling-sensitive; one escaping rendering exists. Matrix row M6 re-adds a respaced bare disjunct ( No higher-severity findings. Specifically checked and not found: flip regressions anywhere outside the intended set (exhaustive), failure-clause behavior change (residue byte-identical), Mutation matrix (test non-vacuity)Production file = the workflow YAML; mutants applied as scratch edits, suite =
All killed mutants failed the intended test ( Targeted gates
Not covered
MethodologyRan in the CI verify container ( Flakiness gate logEvidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
Collaborator
|
Triage re-run completed without a new review. The bot already has a review of its own on 机器人在 The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
yiliang114
approved these changes
Aug 22, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. Verified the gate logic against all four cancellation shapes:
- Superseded run (the #9131 class): run-level cancel sweeps the chain, so
authorize/delay-automatic-reviewland 'cancelled' alongsidereview-pr— the compound clause stays false and no false fallback posts. Thealways()analysis is right: the fallback job survives a run-level cancel, which is exactly why the bare== 'cancelled'disjunct was wrong. - Job-level timeout: upstream jobs completed (or 'skipped' on explicit-trigger runs, which also satisfies
!= 'cancelled'), so the clause opens and the fallback posts unchanged; failure clauses are untouched. - The residual corner — a run-level cancel landing AFTER the chain finished — is correctly argued away by the in-step guards (head-moved for push-supersede, PR-state for the close flavor, same-head twins can't land that late), and the accepted tradeoff (manual run-cancel during the delay window goes silent) is documented.
- The test upgrade pins the exact compound clause including grouping, plus the occurrence count so a merge-conflict resolution re-adding a bare disjunct fails even with the pin intact.
CI is green on head (14 passing checks, none failing).
qqqys
pushed a commit
to qqqys/qwen-code
that referenced
this pull request
Aug 26, 2026
… the failure comment (QwenLM#10114) The fallback-comment gate admits a cancelled review-pr on purpose: a job-level timeout is auto-CANCELLED by GitHub (failure() false), which opens neither the failure-only gate nor the in-job step, so silence there would leave a timed-out review unexplained (QwenLM#9255). But the same 'cancelled' result also arrives when a run or job is cancelled mid-review after the upstream chain finished, and that flavor got the full failure body — "The review pipeline failed before a review could be posted. A transient error is retried automatically…" — none of which is true for a cancellation. On PR QwenLM#9729, run 32875478404 was run-cancelled two minutes into the review with no successor run, so the QwenLM#9716 supersede guard correctly did not match, and the comment read as a pipeline outage to the PR author. The two flavors are not separable in needs — both reach the gate as review-pr 'cancelled' with upstream green — so the fix branches inside the step on the wired-in needs result: a cancelled review-pr now posts one body accurate for both flavors (no failure/auto-retry claims, retry instruction kept for the timeout flavor, run-URL markdown link kept for the cross-job dedup), and everything else keeps the failure body. The step runs under set -u, so a dropped env wiring fails the step loudly instead of silently reverting cancelled runs to the false body. Mutation-verified: neutralizing the cancelled branch fails both new tests; the restored workflow passes the suite at base parity. Fixes QwenLM#10109
Collaborator
|
Released in v0.22.2. |
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
3 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
Teaches the review pipeline's fallback path to tell two kinds of cancellation apart. When a review run is cancelled as a whole — typically because a newer run for the same pull request superseded it in the concurrency group — the "review did not complete" fallback comment is no longer posted. When only the review step itself is cancelled, which is how GitHub reports a job-level timeout, the fallback comment still fires as before.
Why it's needed
The fallback gate assumed a run-level cancel would take the queued fallback job down with it, leaving job-level timeout as the only live source of a cancelled review step. That assumption does not hold: the fallback job is guarded by
always(), which keeps it running through a run-level cancel. On #9131 two same-head runs started one second apart, the newer one cancelled the older inside the authorization step, and the older run posted a false "Qwen Code review did not complete successfully" (the comment, run 32558544379) while the surviving run was still reviewing. The existing head-comparison guard inside the step could not catch it because both runs pointed at the same head commit.The two cases are separable from the dependency results alone: a job-level timeout cancels the review step in isolation, its upstream jobs having completed long before, while a run-level cancel sweeps the whole chain. The gate now admits a cancelled review step only when no upstream job was itself cancelled.
Reviewer Test Plan
How to verify
Replay the gate expression against the four cancellation shapes:
Evidence (Before & After)
Before: #9131 (comment) — a false "review did not complete" posted by the concurrency-cancelled twin of a run that was still reviewing.
After: N/A (CI workflow change; the matrix above is the verification).
Tested on
N/A — CI workflow change, validated by YAML parsing and actionlint (its single finding is pre-existing on an untouched line), plus the scenario matrix above.
Environment (optional)
N/A
Risk & Scope
pull_request_targetruns one second apart in the first place — worth a separate look; this change only stops the misleading comment such twins produce.Linked Issues
Observed on #9131 (false fallback comment from run 32558544379).
中文说明
这个 PR 做了什么
让评审流水线的 fallback 路径区分两类取消：整个 run 被取消（通常是同一 PR 的更新 run 在并发组里顶掉旧 run）时，不再发布 "review did not complete" 兜底评论；只有评审步骤本身被取消（GitHub 对 job 级超时的表现形式）时，兜底评论照常发出。
为什么需要
原有 gate 假设 run 级取消会连带取消排队中的 fallback job，因此现场看到 cancelled 的评审步骤就只剩超时一种来源。该假设不成立：fallback job 由
always()守卫，整个 run 被取消后它仍会继续执行。#9131 上两个相同 head 的 run 相隔 1 秒启动，新 run 在 authorization 阶段取消了旧 run，旧 run 随即发出一条错误的 "Qwen Code review did not complete successfully"（run 32558544379），而幸存的 run 仍在正常评审。步骤内已有的 head 对比守卫无法拦截，因为两个 run 指向同一个 head。两类取消仅凭依赖结果即可区分：job 级超时只取消评审步骤本身（上游 job 早已完成），run 级取消则横扫整条链。新 gate 仅在上游 job 均未被取消时，才为 cancelled 的评审步骤放行。
风险与范围
pull_request_targetrun，值得单独排查；本改动只消除这类孪生 run 产出的误导性评论。