Skip to content

fix(review): stop the fallback comment from firing on superseded runs #9716

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 4 commits into from
Aug 22, 2026
Merged

fix(review): stop the fallback comment from firing on superseded runs #9716

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
26d2484
fix(review): stop the fallback comment from firing on superseded runs
wenshao Aug 22, 2026
49e9207
test(review): pin the fallback gate's upstream-cancel conjuncts
wenshao Aug 22, 2026
36b9a5b
test(review): assert the bare cancelled disjunct is absent from the gate
wenshao Aug 22, 2026
46cec25
test(review): pin the cancelled-check count; credit the PR-state check
wenshao Aug 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 19 additions & 7 deletions .github/workflows/qwen-code-pr-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
[
Expand All @@ -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') ||
Comment on lines +1940 to +1942

@wenshao wenshao Aug 22, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

needs.authorize.result == 'failure' ||
needs.review-config.result == 'failure' ||
needs.delay-automatic-review.result == 'failure' ||
Expand Down
16 changes: 13 additions & 3 deletions scripts/tests/qwen-pr-review-workflow.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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') ||",
);
Comment on lines +3080 to +3084

@wenshao wenshao Aug 22, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

expect(job.if).toContain("needs.authorize.result == 'failure'");
expect(job.if).toContain("needs.review-config.result == 'failure'");
expect(job.if).toContain(
Expand Down
Loading