fix(review): give cancelled runs an accurate fallback body instead of the failure comment - #10114
Merged
Conversation
… the failure comment 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 (#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 #9729, run 32875478404 was run-cancelled two minutes into the review with no successor run, so the #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 #10109
github-actions
Bot
added
the
review/self-reported
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Gate pass — the problem is verified end-to-end against the API, not just claimed.
Moving on to code review. 🔍 中文说明通过门禁 —— 问题已在 API 层面逐条核实，不只是作者的单方面陈述。
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewMy independent take before reading the diff: the gate admits Verified against the workflow around the change:
No blockers, no convention issues. The comment blocks are dense, but that matches this file's incident-log house style. One safety note for a workflow-changing PR: the review/triage pipelines run on Test evidence — the PR's own CI (per policy, PR code is never executed in this unattended run)The behavioural claim here — which body posts for a cancelled review-pr — is pinned by the executable step-level tests, which run the step's actual bash with the new env value and assert the posted comment; the gate semantics they assume ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The author also reports mutation-testing the new branch (neutralizing it turns the new tests red) — that is the author's claim, attributed as such; the executable assertions above are the pin this review relies on. 中文说明代码审查读 diff 前的独立判断：gate 有意放行 结合改动周围的工作流核实：
无阻塞项，无规范问题。注释块较密，但与本文件的"事故日志"风格一致。对工作流改动型 PR 的一个安全说明：review/triage 流水线跑在 测试证据——PR 自身的 CI（按策略，无人值守运行中从不执行 PR 代码）本处的行为主张——被取消的 review-pr 发布哪条正文——由可执行步骤级测试锁定：它们用新的 env 值运行步骤的真实 bash，并断言所发评论；其假设的 gate 语义（ 作者另报告了对新分支的变异测试（打掉分支则新测试变红）——此为作者陈述，仅作转述；本审查依赖的是上述可执行断言这一锁定。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 5/5 — the incident is verified end-to-end against the API, the fix is the minimal shape of the change, and the tests execute the step's real bash rather than paraphrasing it. What I'm actually thinking: this is how a fix for this pipeline should look. The PR doesn't just assert the incident — it names the run, the job timestamps, and the false comment, and every one of those checks out when pulled from the API independently. The direction question (why not just silence cancels?) is answered before it's asked, with the #9255 history that makes The one thing I can't attest yet is the suite result — 中文说明置信度：5/5 —— 事故已在 API 层面端到端核实，修复是该改动的最小形态，测试执行的是步骤的真实 bash 而非转述。 真实想法：这条流水线的问题就该这样修。PR 不只是声称事故——点名了 run、job 时间戳和那条错误评论，独立从 API 拉取后条条对得上。方向问题（为什么不直接静默取消？）在被提出之前就已回答，并带上了让 唯一尚不能背书的是套件结果——运行改动测试文件的 — 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. |
doudouOUC
reviewed
Aug 26, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Code review
PR #10114 — fix(review): give cancelled runs an accurate fallback body instead of the failure comment
Files: 2 | +73/−8
Author: @wenshao | Fixes: #10109
What changed
-
.github/workflows/qwen-code-pr-review.yml(+27/−4): ThePost fallback commentstep wiresREVIEW_PR_RESULT: ${{ needs.review-pr.result }}into its environment and branches on it. A cancelled review-pr posts a body that says "was cancelled" instead of "did not complete successfully / pipeline failed / retried automatically". Theelsebranch keeps the original failure body byte-for-byte unchanged. -
scripts/tests/qwen-pr-review-workflow.test.js(+46/−4): The executable step harness injectsREVIEW_PR_RESULT(defaultfailure); a new test runs the real bash withREVIEW_PR_RESULT=cancelledand asserts the marker prefix, the cancelled-specific wording, the absence of all three failure claims, and the presence of the dedup anchor + retry instruction. A wiring pin test locks the env declaration and theifbranch line.
Verification
- Cross-job dedup preserved: The
[workflow logs](${RUN_URL})markdown link is present in both bodies — the dedup matchesactions/runs/<id>), anchored on the closing paren. The run-URL pin in the test now counts both bodies. set -usafety: The step runs underset -uo pipefail; a droppedREVIEW_PR_RESULTenv wiring fails the step loudly instead of silently reverting to the false body.- No regression on other paths: The
elsebranch is the exact original body string. The gate is untouched. The in-job fallback step is unchanged. - Harness default of
failure: Every pre-existing executable test stays on the unchanged path. - Mutation-verified: The author reports neutralizing the cancelled branch turns both new tests red.
Risk assessment
- Self-interference: The review/triage pipelines run on
pull_request_target, which evaluates the workflow from the base ref — this PR's own pipeline ran themainversion, so there is no self-interference. - Mid-review merge: The PR merged
mainmid-review; the effective diff againstmainis line-for-line identical to the pre-merge one, and the singlemaincommit pulled in (#9980, providers) touches none of the files reviewed here.
Conclusion
No issues found. The fix is minimal, correct, and well-tested — one env wiring, one branch, one accurate body for both cancellation flavors. The triage already gave this 5/5 confidence with an approve-on-green posture. I concur.
Verdict: COMMENT — no changes requested.
Collaborator
|
test connectivity |
doudouOUC
approved these changes
Aug 26, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Review
Reviewed commit 407f463 — +73/−8 across 2 files (.github/workflows/qwen-code-pr-review.yml, scripts/tests/qwen-pr-review-workflow.test.js).
This is a tight, well-documented fix for issue #10109. The change is minimal and correct:
Workflow: The Post fallback comment step wires in REVIEW_PR_RESULT: ${{ needs.review-pr.result }} and branches on its value. For a cancelled review-pr, it posts an accurate body saying the run was cancelled (by an operator, an upstream event, or a job-level timeout) — without the false "pipeline failed / retried automatically" claims. The else branch preserves the original failure body unchanged. The set -uo pipefail shell option keeps the -u guard that fails the step loudly if the env wiring is ever dropped.
Tests: The new tests cover the env wiring (text pin on REVIEW_PR_RESULT), the bash branch condition, and the full end-to-end bash step — verifying that the cancelled body has the correct content, excludes the failure claims, and preserves the )-anchored run URL for cross-job dedup. The failure path is also verified to be unchanged. The runFallbackStep harness defaults to REVIEW_PR_RESULT=failure, preserving existing test behavior.
No issues found. The design correctly handles the indistinguishable-in-needs flavors (job-level timeout vs. plain cancel) with one accurate body, and the test coverage is mutation-verified per the author report.
Note: Build and test could not run on this machine (Windows git fetch to github.com is blocked by a hosts entry). The change is a pure workflow + test change, both exercised by the PR's own CI and the mutation-verified test suite.
qwen-code-ci-bot
approved these changes
Aug 26, 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
approved these changes
Aug 26, 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! ✅
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.
中文说明
未发现问题。LGTM！✅
未检查（工具限制，非阻断）：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.22.0)
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.
Problem
#9716 stopped the fallback comment on superseded review runs, but a run cancelled without a successor still posts the full failure comment — "Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically…". For a cancellation none of that is true: nothing failed and nothing retries automatically. Evidence: PR #9729, run 32875478404 —
review-prstarted 17:36:09, was run-cancelled 17:38:01, andfallback-commentposted the failure body at 17:39:38; no successor run existed until the next morning, so the supersede guard correctly did not match.Fixes #10109.
Why not silence the cancelled case
The gate admits
needs.review-pr.result == 'cancelled'on purpose (#9255): a review-pr that dies to its own job-leveltimeout-minutesis auto-CANCELLED by GitHub (failure()false), which opens neither the failure-only gate nor the in-job step — going silent on every cancel would leave a timed-out review unexplained again. And the two flavors are not separable inneeds: a job-level timeout and a mid-review cancel landing after the upstream chain finished both arrive as review-prcancelledwithauthorize/delay-automatic-reviewgreen.Change
The gate is untouched. The
Post fallback commentstep now wires inREVIEW_PR_RESULT: ${{ needs.review-pr.result }}and branches on it: a cancelled review-pr posts one body accurate for both flavors —— keeping the
)-anchored run-URL markdown link the cross-job dedup matches and the retry instruction the timeout flavor needs, while every other path keeps the existing failure body. The step runs underset -u, so a dropped env wiring fails the step loudly instead of silently reverting cancelled runs to the false body.Tests
scripts/tests/qwen-pr-review-workflow.test.js: the executable harness now setsREVIEW_PR_RESULT(defaultfailure); a new step-bash test asserts the cancelled case posts the marker-headed cancellation body without the "did not complete successfully / pipeline failed / retried automatically" claims while keepingactions/runs/<id>)and the retry instruction, and that the failure path is unchanged; a text pin locks the env wiring and the branch; the run-URL-link pin now counts both bodies.中文版
问题
#9716 堵住了 superseded(被后继 run 顶替)时的 fallback 评论,但一个没有后继的被取消 run 仍会发布完整失败评论——"Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically…"。对取消而言这些话都不成立:没有东西失败,也不会自动重试。证据:PR #9729,run 32875478404——
review-pr17:36:09 启动、17:38:01 被 run 级取消,fallback-comment17:39:38 发布了失败评论;直到次日早晨才有后继 run,supersede 守卫正确地没有匹配。Fixes #10109。
为什么不能对 cancelled 直接静默
gate 放行
needs.review-pr.result == 'cancelled'是有意为之(#9255):review-pr 触发自身 job 级timeout-minutes时会被 GitHub 自动标为 CANCELLED(failure()为 false),failure-only 的 gate 和 job 内步骤都不会触发——对所有取消一律静默会让超时的 review 重新变得无解释。而且两种情形在needs里不可区分:job 级超时、以及上游链完成后落在 review 中途的取消,都表现为 review-prcancelled且authorize/delay-automatic-review为绿。改动
gate 不变。
Post fallback comment步骤接入REVIEW_PR_RESULT: ${{ needs.review-pr.result }}并据此分支:review-pr 被取消时发布一条对两种情形都准确的正文(见上文英文引文),保留跨 job 去重所锚定的)结尾 run-URL markdown 链接、以及超时情形需要的重试指引;其余路径维持原失败正文。步骤在set -u下运行,env 接线一旦被删会让步骤大声失败,而不是悄悄退回错误正文。测试
scripts/tests/qwen-pr-review-workflow.test.js:可执行 harness 现在注入REVIEW_PR_RESULT(默认failure);新增真实 bash 步骤测试断言 cancelled 情形发布带 marker 的取消正文、不含 "did not complete successfully / pipeline failed / retried automatically" 措辞、保留actions/runs/<id>)与重试指引,且 failure 路径不变;文本 pin 锁定 env 接线与分支;run-URL 链接 pin 改为对两个正文计数。