From 26d2484ad16e5ba24e46c967b9826ea612893a52 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 22 Aug 2026 17:43:38 +0800 Subject: [PATCH 1/4] fix(review): stop the fallback comment from firing on superseded runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/workflows/qwen-code-pr-review.yml | 26 +++++++++++++++++------ 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 1a4acdc500a..b0e84a992f2 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -1903,12 +1903,22 @@ 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 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 +1931,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' || From 49e92078fba3cf6f764056f3a87ed6ca7e7d97d1 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 22 Aug 2026 18:39:32 +0800 Subject: [PATCH 2/4] test(review): pin the fallback gate's upstream-cancel conjuncts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- scripts/tests/qwen-pr-review-workflow.test.js | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 55f2a27c767..b7ca13c98fb 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -3069,9 +3069,19 @@ 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') ||", + ); expect(job.if).toContain("needs.authorize.result == 'failure'"); expect(job.if).toContain("needs.review-config.result == 'failure'"); expect(job.if).toContain( From 36b9a5b699ad2155505c4ea0521dd089d0a7e8cf Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 22 Aug 2026 20:23:43 +0800 Subject: [PATCH 3/4] test(review): assert the bare cancelled disjunct is absent from the gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/workflows/qwen-code-pr-review.yml | 14 +++++++++----- scripts/tests/qwen-pr-review-workflow.test.js | 7 +++++++ 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index b0e84a992f2..f3ea68f59ec 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -1914,11 +1914,15 @@ jobs: # `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 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. + # 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, 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: [ diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index b7ca13c98fb..31ae1bd5b61 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -3082,6 +3082,13 @@ describe('fallback comment resilience (PR #8894 incident class)', () => { " needs.authorize.result != 'cancelled' &&\n" + " needs.delay-automatic-review.result != 'cancelled') ||", ); + // ...and the bare disjunct must be ABSENT, not just the compound one + // present: a merge-conflict resolution keeping both sides re-adds + // `== 'cancelled' ||` beside the intact compound clause, the gate opens + // 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' ||"); expect(job.if).toContain("needs.authorize.result == 'failure'"); expect(job.if).toContain("needs.review-config.result == 'failure'"); expect(job.if).toContain( From 46cec2565138a6d9a5a568ffb0297fa17e368625 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 22 Aug 2026 21:41:20 +0800 Subject: [PATCH 4/4] test(review): pin the cancelled-check count; credit the PR-state check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/workflows/qwen-code-pr-review.yml | 8 +++++--- scripts/tests/qwen-pr-review-workflow.test.js | 16 +++++++++------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index f3ea68f59ec..386dda209c0 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -1916,9 +1916,11 @@ jobs: # 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, 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 (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 diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 31ae1bd5b61..5102240d07a 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -3082,13 +3082,15 @@ describe('fallback comment resilience (PR #8894 incident class)', () => { " needs.authorize.result != 'cancelled' &&\n" + " needs.delay-automatic-review.result != 'cancelled') ||", ); - // ...and the bare disjunct must be ABSENT, not just the compound one - // present: a merge-conflict resolution keeping both sides re-adds - // `== 'cancelled' ||` beside the intact compound clause, the gate opens - // 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' ||"); + // ...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(