diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 1a4acdc500a..386dda209c0 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -1903,12 +1903,28 @@ jobs: # re-runs, which keep the same run id. A review-pr that dies to its own # job-level timeout is auto-CANCELLED by GitHub — result 'cancelled' and # failure() false — which opens neither a failure-only gate nor the in-job - # step, so the gate admits 'cancelled' too; a run-level cancel cancels this - # queued job with it, so a live gate evaluation seeing 'cancelled' is - # overwhelmingly the timeout case, and the residual manual single-job - # cancel just gets a benign retry-guidance comment. The PR number comes - # from the event payload, not the dead job's outputs, which do not survive - # a crash. + # step, so the gate admits 'cancelled' — but only when the upstream chain + # finished. `always()` keeps this job running through a RUN-level cancel + # (it does not die with the run), so a concurrency supersede used to post + # a false "did not complete" while the surviving run was still reviewing: + # 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's gate + # saw review-pr 'cancelled' (run 32558544379) — same-head, so the in-step + # head-moved guard could not catch it. The two cancels are separable in + # `needs`: a job-level timeout cancels review-pr ALONE — authorize and + # delay-automatic-review completed long before — while a run-level cancel + # sweeps the whole chain, so 'cancelled' opens the gate only when neither + # upstream job was itself cancelled. A run-level cancel landing AFTER the + # chain finished (mid-review) still opens the gate: the push-supersede + # flavor is then suppressed by the in-step head-moved guard, the close + # flavor (a `closed`-action run joining the PR-scoped group hours in, the + # head unchanged) 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 + # silent under this rule (the person who cancelled does not need retry + # guidance); a manual cancel of review-pr alone mid-review still posts, + # benign as before. The PR number comes from the event payload, not the + # dead job's outputs, which do not survive a crash. fallback-comment: needs: [ @@ -1921,7 +1937,9 @@ jobs: if: |- always() && (needs.review-pr.result == 'failure' || - needs.review-pr.result == 'cancelled' || + (needs.review-pr.result == 'cancelled' && + needs.authorize.result != 'cancelled' && + needs.delay-automatic-review.result != 'cancelled') || needs.authorize.result == 'failure' || needs.review-config.result == 'failure' || needs.delay-automatic-review.result == 'failure' || diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 55f2a27c767..5102240d07a 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -3069,9 +3069,28 @@ describe('fallback comment resilience (PR #8894 incident class)', () => { expect(job.if).toContain("needs.review-pr.result == 'failure'"); // A review-pr that dies to its own job-level timeout is auto-cancelled // (result 'cancelled', failure() false), which would open neither this - // gate nor the in-job step; a run-level cancel cancels this queued job - // too, so a live evaluation seeing 'cancelled' is the timeout case. - expect(job.if).toContain("needs.review-pr.result == 'cancelled'"); + // gate nor the in-job step — but `always()` keeps this job alive through + // a RUN-level cancel, so a bare 'cancelled' clause posts a false "did + // not complete" from a concurrency-superseded run while its same-head + // twin is still reviewing (PR #9131, run 32558544379 — same head, so + // the in-step head-moved guard cannot catch it). The two cancels differ + // in `needs`: a timeout cancels review-pr ALONE, a run-level cancel + // sweeps the upstream chain too. Pin the full compound clause — the + // grouping included — so reverting either upstream conjunct fails here. + expect(job.if).toContain( + "(needs.review-pr.result == 'cancelled' &&\n" + + " needs.authorize.result != 'cancelled' &&\n" + + " needs.delay-automatic-review.result != 'cancelled') ||", + ); + // ...and that clause must be the ONLY place the gate tests review-pr + // for 'cancelled': a merge-conflict resolution keeping both sides + // re-adds a bare `== 'cancelled'` disjunct beside the intact compound + // clause — in any rendering (bare, parenthesized, respaced) — and the + // gate opens on every cancelled review-pr again while the pin above + // stays green. Counting occurrences catches every rendering. + expect( + job.if.match(/needs\.review-pr\.result == 'cancelled'/g), + ).toHaveLength(1); expect(job.if).toContain("needs.authorize.result == 'failure'"); expect(job.if).toContain("needs.review-config.result == 'failure'"); expect(job.if).toContain(