diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 1e437bf94fe..ac7237e2364 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -2009,9 +2009,18 @@ jobs: # 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. + # guidance); a cancel landing mid-review still posts, but with the + # cancellation body, not the failure one — the step branches on + # REVIEW_PR_RESULT. For a cancel nothing failed and nothing retries + # automatically, so the failure body's claims are false and read as a + # pipeline outage to the PR author (issue #10109: run 32875478404 was + # run-cancelled two minutes into the review, no successor run existed, + # and the full "pipeline failed" comment posted anyway). The cancelled + # case cannot simply go silent: the job-level-timeout flavor — the very + # reason 'cancelled' is admitted here — reaches this job as the same + # result, so one accurate body covers both flavors. The PR number comes + # from the event payload, not the dead job's outputs, which do not + # survive a crash. fallback-comment: needs: [ @@ -2046,6 +2055,7 @@ jobs: env: GH_TOKEN: '${{ secrets.CI_BOT_PAT }}' PR_NUMBER: '${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}' + REVIEW_PR_RESULT: '${{ needs.review-pr.result }}' RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}' run: |- set -uo pipefail @@ -2169,7 +2179,20 @@ jobs: echo "Skipping fallback comment: a bot review of this PR was submitted after this run was created." >> "$GITHUB_STEP_SUMMARY" exit 0 fi - body="**Qwen Code review did not complete successfully.** The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with \`@qwen-code /review\`. See [workflow logs](${RUN_URL})." + # A cancelled review-pr reaches this job through the gate's + # compound clause with two indistinguishable-in-`needs` flavors: + # its own job-level timeout (auto-CANCELLED by GitHub), and a + # run/job cancel landing after the upstream chain finished. For + # neither is the failure body true — nothing failed, nothing is + # retried automatically (issue #10109) — so the cancelled case + # gets one body accurate for both flavors. It keeps the + # `[workflow logs](RUN_URL)` markdown link the cross-job dedup + # anchors on, and the retry instruction the timeout flavor needs. + if [ "$REVIEW_PR_RESULT" = "cancelled" ]; then + body="**Qwen Code review was cancelled before a review could be posted.** Nothing failed and nothing is retried automatically: the run was cancelled — by an operator, an upstream event, or the job exceeding its execution time limit. If you still want a review of this PR, request one with \`@qwen-code /review\`. See [workflow logs](${RUN_URL})." + else + body="**Qwen Code review did not complete successfully.** The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with \`@qwen-code /review\`. See [workflow logs](${RUN_URL})." + fi body="$(printf '%s\n\n%s' "$FALLBACK_MARKER" "$body")" gh pr comment "$PR_NUMBER" \ --repo "$GITHUB_REPOSITORY" \ diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 6fcd4c67c4e..aa31344e023 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -3008,10 +3008,13 @@ describe('fallback comment resilience (PR #8894 incident class)', () => { expect( inJobStep.run.match(/See \[workflow logs\]\(\$\{RUN_URL\}\)\./g), ).toHaveLength(4); - // Same invariant for the fallback job's body: the cross-job dedup - // matches `actions/runs/)`, anchored on the markdown link's closing - // paren — a body that rendered the URL differently would escape it. - expect(step.run).toContain('See [workflow logs](${RUN_URL}).'); + // Same invariant for the fallback job's TWO bodies (failure and + // cancelled): the cross-job dedup matches `actions/runs/)`, anchored + // on the markdown link's closing paren — a body that rendered the URL + // differently would escape it. + expect( + step.run.match(/See \[workflow logs\]\(\$\{RUN_URL\}\)\./g), + ).toHaveLength(2); expect(inJobStep.run).toContain( `body="$(printf '%s\\n\\n%s' "$FALLBACK_MARKER" "$body")"`, ); @@ -3025,6 +3028,16 @@ describe('fallback comment resilience (PR #8894 incident class)', () => { ).toBeLessThan(inJobStep.run.indexOf('gh pr comment')); }); + it('wires the needs result the cancelled-body branch keys on (issue #10109)', () => { + // The step's bash runs under `set -u`, so dropping this env wiring + // fails the step loudly instead of silently reverting every cancelled + // run to the false "pipeline failed" body. + expect(step.env.REVIEW_PR_RESULT).toBe('${{ needs.review-pr.result }}'); + expect(step.run).toContain( + 'if [ "$REVIEW_PR_RESULT" = "cancelled" ]; then', + ); + }); + it('scopes the dedup to the authenticated bot login, resolved dynamically', () => { // upsert-bot-comment.sh protocol: only comments by the authenticated // login are dedup targets, or a participant posting the marker suppresses @@ -3281,6 +3294,7 @@ describe('fallback comment resilience (PR #8894 incident class)', () => { reviews = '[]', runCreated = '', runStartedAttempt = '', + reviewPrResult = 'failure', } = {}, ) { const dir = mkdtempSync(join(tmpdir(), 'fallback-comment-')); @@ -3408,6 +3422,7 @@ describe('fallback comment resilience (PR #8894 incident class)', () => { REVIEWS_JSON: reviews, RUN_CREATED: runCreated, RUN_STARTED_ATTEMPT: runStartedAttempt, + REVIEW_PR_RESULT: reviewPrResult, }, }, ); @@ -3434,6 +3449,33 @@ describe('fallback comment resilience (PR #8894 incident class)', () => { expect(r.posted).toContain('actions/runs/12345'); }); + it('posts the cancellation body, not the failure one, for a cancelled review-pr', () => { + // Issue #10109: a cancelled review-pr reaches the gate two ways — its + // own job-level timeout, and a run/job cancel landing after the + // upstream chain finished (run 32875478404) — and for neither is + // "pipeline failed / retried automatically" true. Silence would regress + // the timeout flavor, so the cancelled case gets its own body. + const cancelled = runFallbackStep('default', { + reviewPrResult: 'cancelled', + }); + expect(cancelled.status).toBe(0); + expect(cancelled.posted.startsWith(`${marker}\n\n`)).toBe(true); + expect(cancelled.posted).toContain('cancelled'); + // The claims the issue calls out must not ride a cancellation... + expect(cancelled.posted).not.toContain('did not complete successfully'); + expect(cancelled.posted).not.toContain('The review pipeline failed'); + expect(cancelled.posted).not.toContain( + 'A transient error is retried automatically', + ); + // ...while the `)`-anchored run URL the cross-job dedup matches and the + // retry instruction (the job-timeout flavor's reader needs it) stay. + expect(cancelled.posted).toContain('actions/runs/12345)'); + expect(cancelled.posted).toContain('@qwen-code /review'); + // The failure path keeps the original body. + const failed = runFallbackStep('default', { reviewPrResult: 'failure' }); + expect(failed.posted).toContain('did not complete successfully'); + }); + it('dedupes on the marker plus this run URL', () => { // Mirrors a prior fallback comment's rendered shape: the run URL is a // markdown link, so the run id is always immediately followed by ')'.