From 905c5c95a70e2fa2e0acc82c850ae7decf308b90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=98=93=E8=89=AF?= <1204183885@qq.com> Date: Sun, 5 Jul 2026 04:15:30 +0800 Subject: [PATCH] fix(ci): skip stale PR review runs (#6313) * fix(ci): skip stale PR review runs * test(ci): trigger review cancellation * fix(ci): make PR review cancellation interruptible * fix(ci): block stale review comment writes --- .github/workflows/qwen-code-pr-review.yml | 160 ++++++++++++--- scripts/tests/qwen-resolve-workflow.test.js | 206 +++++++++++++++++++- 2 files changed, 341 insertions(+), 25 deletions(-) diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 34eea28157..7b7c4983eb 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -56,12 +56,7 @@ concurrency: ${{ github.event_name == 'pull_request_target' && format('qwen-pr-review-pr-{0}', github.event.pull_request.number) || format('qwen-pr-review-run-{0}', github.run_id) }} - cancel-in-progress: >- - ${{ - github.event_name == 'pull_request_target' && - (github.event.action == 'synchronize' || - github.event.action == 'closed') - }} + cancel-in-progress: "${{ github.event_name == 'pull_request_target' && (github.event.action == 'synchronize' || github.event.action == 'closed') }}" jobs: precheck-pr: @@ -85,7 +80,7 @@ jobs: # this `if` only matches the /review command shape. needs: ['authorize'] if: |- - always() && + !cancelled() && needs.authorize.outputs.should_review == 'true' && ((github.event_name == 'issue_comment' && github.event.issue.pull_request && @@ -106,7 +101,7 @@ jobs: concurrency: group: 'qwen-pr-ack-${{ github.event.issue.number || github.event.pull_request.number }}' cancel-in-progress: false - runs-on: "${{ (github.repository == 'QwenLM/qwen-code' && vars.MAINTAINER_ECS_RUNNER_DISABLED != 'true') && fromJSON('[\"self-hosted\", \"linux\", \"x64\", \"ecs-qwen\"]') || fromJSON('[\"ubuntu-latest\"]') }}" + runs-on: '${{ (github.repository == ''QwenLM/qwen-code'' && vars.MAINTAINER_ECS_RUNNER_DISABLED != ''true'') && fromJSON(''["self-hosted", "linux", "x64", "ecs-qwen"]'') || fromJSON(''["ubuntu-latest"]'') }}' timeout-minutes: 5 permissions: pull-requests: 'write' @@ -150,7 +145,7 @@ jobs: if: |- github.event_name == 'pull_request_target' && github.event.action == 'review_requested' - runs-on: "${{ (github.repository == 'QwenLM/qwen-code' && vars.MAINTAINER_ECS_RUNNER_DISABLED != 'true') && fromJSON('[\"self-hosted\", \"linux\", \"x64\", \"ecs-qwen\"]') || fromJSON('[\"ubuntu-latest\"]') }}" + runs-on: '${{ (github.repository == ''QwenLM/qwen-code'' && vars.MAINTAINER_ECS_RUNNER_DISABLED != ''true'') && fromJSON(''["self-hosted", "linux", "x64", "ecs-qwen"]'') || fromJSON(''["ubuntu-latest"]'') }}' permissions: {} outputs: bot_login: '${{ steps.values.outputs.bot_login }}' @@ -163,7 +158,7 @@ jobs: delay-automatic-review: needs: ['authorize'] if: |- - always() && + !cancelled() && github.event_name == 'pull_request_target' && (github.event.action == 'opened' || github.event.action == 'synchronize') && @@ -214,7 +209,7 @@ jobs: # unrelated comment — to avoid spawning a job per comment. The downstream # `if`s still do the exact command body match; this prefix is just a filter. if: |- - always() && + !cancelled() && (github.event_name != 'pull_request_target' || github.event.action != 'closed') && (github.event_name != 'pull_request_target' || @@ -234,7 +229,7 @@ jobs: # Canonical same-repo guard: this job loads CI_BOT_PAT, so fork-triggered # runs stay on hosted (ephemeral); only in-repo PR events on QwenLM/qwen-code # use the persistent ECS runner. - runs-on: "${{ (github.repository == 'QwenLM/qwen-code' && vars.MAINTAINER_ECS_RUNNER_DISABLED != 'true' && github.event.pull_request && github.event.pull_request.head.repo.full_name == github.repository) && fromJSON('[\"self-hosted\", \"linux\", \"x64\", \"ecs-qwen\"]') || fromJSON('[\"ubuntu-latest\"]') }}" + runs-on: '${{ (github.repository == ''QwenLM/qwen-code'' && vars.MAINTAINER_ECS_RUNNER_DISABLED != ''true'' && github.event.pull_request && github.event.pull_request.head.repo.full_name == github.repository) && fromJSON(''["self-hosted", "linux", "x64", "ecs-qwen"]'') || fromJSON(''["ubuntu-latest"]'') }}' timeout-minutes: 5 permissions: contents: 'read' @@ -320,7 +315,7 @@ jobs: # - reopened/ready_for_review runs immediately # KEEP IN SYNC with ack-review-request.if (explicit-trigger branches). if: |- - always() && + !cancelled() && ((github.event_name == 'workflow_dispatch' && (github.event.inputs.command == 'review' || github.event.inputs.command == '')) || (github.event_name == 'pull_request_target' && @@ -353,7 +348,7 @@ jobs: startsWith(github.event.review.body, format('@qwen-code /review{0}', '\n'))) && needs.authorize.outputs.should_review == 'true')) timeout-minutes: 200 - runs-on: "${{ (github.repository == 'QwenLM/qwen-code' && vars.MAINTAINER_ECS_RUNNER_DISABLED != 'true') && fromJSON('[\"self-hosted\", \"linux\", \"x64\", \"ecs-qwen\"]') || fromJSON('[\"ubuntu-latest\"]') }}" + runs-on: '${{ (github.repository == ''QwenLM/qwen-code'' && vars.MAINTAINER_ECS_RUNNER_DISABLED != ''true'') && fromJSON(''["self-hosted", "linux", "x64", "ecs-qwen"]'') || fromJSON(''["ubuntu-latest"]'') }}' permissions: contents: 'read' pull-requests: 'write' @@ -547,14 +542,103 @@ jobs: local real_gh real_gh="$(command -v gh)" export QWEN_CI_REAL_GH="$real_gh" - { - printf '%s\n' '#!/usr/bin/env bash' - printf '%s\n' '[ -n "${QWEN_CI_HTTPS_PROXY:-}" ] && export HTTPS_PROXY="$QWEN_CI_HTTPS_PROXY"' - printf '%s\n' '[ -n "${QWEN_CI_https_proxy:-}" ] && export https_proxy="$QWEN_CI_https_proxy"' - printf '%s\n' '[ -n "${QWEN_CI_HTTP_PROXY:-}" ] && export HTTP_PROXY="$QWEN_CI_HTTP_PROXY"' - printf '%s\n' '[ -n "${QWEN_CI_http_proxy:-}" ] && export http_proxy="$QWEN_CI_http_proxy"' - printf '%s\n' 'exec "$QWEN_CI_REAL_GH" "$@"' - } > "$proxy_bin/gh" + cat > "$proxy_bin/gh" <<'QWEN_GH_WRAPPER' + #!/usr/bin/env bash + set -euo pipefail + [ -n "${QWEN_CI_HTTPS_PROXY:-}" ] && export HTTPS_PROXY="$QWEN_CI_HTTPS_PROXY" + [ -n "${QWEN_CI_https_proxy:-}" ] && export https_proxy="$QWEN_CI_https_proxy" + [ -n "${QWEN_CI_HTTP_PROXY:-}" ] && export HTTP_PROXY="$QWEN_CI_HTTP_PROXY" + [ -n "${QWEN_CI_http_proxy:-}" ] && export http_proxy="$QWEN_CI_http_proxy" + guard_pr_write() { + local repo="${QWEN_CI_REVIEW_REPO:-}" + local pr_number="${QWEN_CI_REVIEW_PR_NUMBER:-}" + local expected_head="${QWEN_CI_REVIEW_EXPECTED_HEAD_SHA:-}" + if [ -z "$repo" ] || [ -z "$pr_number" ]; then + echo "Blocked PR write: QWEN_CI_REVIEW_REPO and QWEN_CI_REVIEW_PR_NUMBER must be set." >&2 + exit 90 + fi + local pr_data state current_head + if ! pr_data="$("$QWEN_CI_REAL_GH" pr view "$pr_number" --repo "$repo" --json state,headRefOid --jq '[.state, .headRefOid] | @tsv')"; then + echo "Blocked PR write: failed to verify PR #${pr_number} state." >&2 + exit 90 + fi + IFS=$'\t' read -r state current_head <<< "$pr_data" + if [ "$state" != "OPEN" ]; then + echo "Blocked PR write: PR #${pr_number} is ${state}." >&2 + exit 90 + fi + if [ -n "$expected_head" ] && [ "$current_head" != "$expected_head" ]; then + echo "Blocked PR write: PR #${pr_number} moved from ${expected_head} to ${current_head}." >&2 + exit 90 + fi + } + guard_api_write() { + local endpoint="" method="" write_flag=false previous="" + local arg upper_method + for arg in "$@"; do + if [ -n "$previous" ]; then + case "$previous" in + --method|-X) method="$arg" ;; + esac + previous="" + continue + fi + case "$arg" in + --method|-X|--jq|-q|--hostname|-H|--preview|--cache) + previous="$arg" + ;; + --method=*) + method="${arg#--method=}" + ;; + --input|--field|--raw-field|-f|-F) + write_flag=true + previous="$arg" + ;; + --input=*|--field=*|--raw-field=*|-f*|-F*) + write_flag=true + ;; + -*) + ;; + *) + if [ -z "$endpoint" ]; then + endpoint="$arg" + fi + ;; + esac + done + upper_method="$(printf '%s' "$method" | tr '[:lower:]' '[:upper:]')" + if [ -z "$upper_method" ] && [ "$write_flag" = true ]; then + upper_method="POST" + fi + case "$upper_method" in + POST|PUT|PATCH|DELETE) ;; + *) return 0 ;; + esac + case "$endpoint" in + repos/*/pulls/*/reviews|/repos/*/pulls/*/reviews|\ + repos/*/pulls/*/comments|/repos/*/pulls/*/comments|\ + repos/*/issues/*/comments|/repos/*/issues/*/comments|\ + repos/*/issues/comments/*|/repos/*/issues/comments/*) + guard_pr_write + ;; + esac + } + case "${1:-}" in + api) + shift + guard_api_write "$@" + set -- api "$@" + ;; + pr) + case "${2:-}" in + comment|review) + guard_pr_write + ;; + esac + ;; + esac + exec "$QWEN_CI_REAL_GH" "$@" + QWEN_GH_WRAPPER chmod +x "$proxy_bin/gh" fi @@ -604,13 +688,27 @@ jobs: fail "timeout_minutes must not exceed 180 minutes" fi - if ! PR_STATE="$(gh pr view "$PR_NUMBER" --repo "$REPO" --json state --jq '.state')"; then + if ! PR_DATA="$(gh pr view "$PR_NUMBER" --repo "$REPO" --json state,headRefOid --jq '[.state, .headRefOid] | @tsv')"; then fail "Failed to determine state for PR #${PR_NUMBER}." fi + IFS=$'\t' read -r PR_STATE CURRENT_HEAD_SHA <<< "$PR_DATA" if [ "$PR_STATE" != "OPEN" ]; then echo "Skipping: PR #${PR_NUMBER} is ${PR_STATE}." | tee -a "$GITHUB_STEP_SUMMARY" exit 0 fi + EXPECTED_HEAD_SHA="$CURRENT_HEAD_SHA" + if [ "${{ github.event_name }}" = "pull_request_target" ]; then + EVENT_HEAD_SHA="${{ github.event.pull_request.head.sha }}" + if [ "$CURRENT_HEAD_SHA" != "$EVENT_HEAD_SHA" ]; then + echo "Skipping stale review run: event head ${EVENT_HEAD_SHA} is no longer current (current head ${CURRENT_HEAD_SHA})." | tee -a "$GITHUB_STEP_SUMMARY" + exit 0 + fi + EXPECTED_HEAD_SHA="$EVENT_HEAD_SHA" + fi + export QWEN_CI_REVIEW_REPO="$REPO" + export QWEN_CI_REVIEW_PR_NUMBER="$PR_NUMBER" + export QWEN_CI_REVIEW_EXPECTED_HEAD_SHA="$EXPECTED_HEAD_SHA" + echo "expected_head_sha=$EXPECTED_HEAD_SHA" >> "$GITHUB_OUTPUT" PROMPT="/review ${REVIEW_URL}" if [ "$REVIEW_MODE" = "comment" ]; then @@ -683,12 +781,26 @@ jobs: steps.context.outputs.pr_number != '' env: GH_TOKEN: '${{ secrets.CI_BOT_PAT }}' + EXPECTED_HEAD_SHA: "${{ steps.review.outputs.expected_head_sha || '' }}" FAILURE_KIND: "${{ steps.review.outputs.failure_kind || '' }}" FAILURE_REASON: "${{ steps.review.outputs.failure_reason || 'Run review failed. See workflow logs for details.' }}" PR_NUMBER: '${{ steps.context.outputs.pr_number }}' RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}' TIMEOUT_MINUTES: '${{ steps.context.outputs.timeout_minutes }}' run: |- + pr_data="$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json state,headRefOid --jq '[.state, .headRefOid] | @tsv')" || { + echo "Could not verify PR #${PR_NUMBER}; skipping fallback comment." >> "$GITHUB_STEP_SUMMARY" + exit 0 + } + IFS=$'\t' read -r pr_state current_head <<< "$pr_data" + if [ "$pr_state" != "OPEN" ]; then + echo "Skipping fallback comment: PR #${PR_NUMBER} is ${pr_state}." >> "$GITHUB_STEP_SUMMARY" + exit 0 + fi + if [ -n "$EXPECTED_HEAD_SHA" ] && [ "$current_head" != "$EXPECTED_HEAD_SHA" ]; then + echo "Skipping fallback comment: PR #${PR_NUMBER} moved from ${EXPECTED_HEAD_SHA} to ${current_head}." >> "$GITHUB_STEP_SUMMARY" + exit 0 + fi if [ "$FAILURE_KIND" = "timeout" ]; then if [ "$TIMEOUT_MINUTES" -lt 180 ]; then body="**Qwen Code review timed out.** ${FAILURE_REASON} For large PRs, retry with a longer timeout by commenting: \`@qwen-code /review --timeout=180\`. See [workflow logs](${RUN_URL})." @@ -705,7 +817,7 @@ jobs: resolve-pr: needs: ['authorize'] if: |- - always() && + !cancelled() && github.repository == 'QwenLM/qwen-code' && needs.authorize.outputs.should_review == 'true' && ( diff --git a/scripts/tests/qwen-resolve-workflow.test.js b/scripts/tests/qwen-resolve-workflow.test.js index abd17fdd64..3cb54c02c6 100644 --- a/scripts/tests/qwen-resolve-workflow.test.js +++ b/scripts/tests/qwen-resolve-workflow.test.js @@ -4,7 +4,16 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { existsSync, readFileSync } from 'node:fs'; +import { + chmodSync, + existsSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { spawnSync } from 'node:child_process'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; import { describe, expect, it } from 'vitest'; @@ -39,6 +48,65 @@ function step(section, name) { return match?.[0] ?? ''; } +function reviewGhWrapper(runStep) { + const start = runStep.indexOf('cat > "$proxy_bin/gh" <<\'QWEN_GH_WRAPPER\''); + const bodyStart = runStep.indexOf('\n', start) + 1; + const end = runStep.indexOf('\n QWEN_GH_WRAPPER', bodyStart); + return runStep.slice(bodyStart, end).replace(/^ {10}/gm, ''); +} + +function runReviewGhWrapper( + runStep, + args, + prState, + currentHead, + expectedHead = 'head-a', +) { + const tempDir = mkdtempSync(path.join(tmpdir(), 'qwen-review-gh-')); + try { + const wrapperPath = path.join(tempDir, 'gh'); + const realGhPath = path.join(tempDir, 'real-gh'); + const ghLogPath = path.join(tempDir, 'gh.log'); + writeFileSync(wrapperPath, reviewGhWrapper(runStep)); + writeFileSync( + realGhPath, + [ + '#!/usr/bin/env bash', + 'set -euo pipefail', + 'if [ "${1:-}" = "pr" ] && [ "${2:-}" = "view" ]; then', + ' printf "%s\\t%s\\n" "${FAKE_PR_STATE:-OPEN}" "${FAKE_HEAD_SHA:-head-a}"', + ' exit 0', + 'fi', + 'printf "%s\\n" "$*" >> "${FAKE_GH_LOG:?}"', + ].join('\n'), + ); + writeFileSync(ghLogPath, ''); + chmodSync(wrapperPath, 0o755); + chmodSync(realGhPath, 0o755); + + const result = spawnSync(wrapperPath, args, { + encoding: 'utf8', + env: { + ...process.env, + FAKE_GH_LOG: ghLogPath, + FAKE_HEAD_SHA: currentHead, + FAKE_PR_STATE: prState, + QWEN_CI_REAL_GH: realGhPath, + QWEN_CI_REVIEW_EXPECTED_HEAD_SHA: expectedHead, + QWEN_CI_REVIEW_PR_NUMBER: '123', + QWEN_CI_REVIEW_REPO: 'owner/repo', + }, + }); + + return { + ...result, + ghLog: readFileSync(ghLogPath, 'utf8'), + }; + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } +} + describe('qwen resolve workflow', () => { const workflow = readFileSync( path.join(repoRoot, '.github/workflows/qwen-code-pr-review.yml'), @@ -79,6 +147,18 @@ describe('qwen resolve workflow', () => { ); }); + it('keeps synchronize cancellation expression simple for workflow-level concurrency', () => { + const concurrencyStart = workflow.indexOf('\nconcurrency:'); + const concurrency = workflow.slice( + concurrencyStart, + workflow.indexOf('\njobs:', concurrencyStart), + ); + + expect(concurrency).toContain( + "cancel-in-progress: \"${{ github.event_name == 'pull_request_target' && (github.event.action == 'synchronize' || github.event.action == 'closed') }}\"", + ); + }); + it('listens for /resolve comments', () => { expect(workflow).toContain( "github.event.comment.body == '@qwen-code /resolve'", @@ -177,6 +257,118 @@ describe('qwen resolve workflow', () => { ); }); + it('skips stale automatic review runs before invoking qwen', () => { + const runStep = step(reviewJob, 'Run review'); + const staleHeadCheck = runStep.slice( + runStep.indexOf( + 'if [ "${{ github.event_name }}" = "pull_request_target" ]; then', + ), + runStep.indexOf('PROMPT="/review ${REVIEW_URL}"'), + ); + + expect(staleHeadCheck).toContain( + 'EVENT_HEAD_SHA="${{ github.event.pull_request.head.sha }}"', + ); + expect(runStep).toContain( + 'PR_DATA="$(gh pr view "$PR_NUMBER" --repo "$REPO" --json state,headRefOid --jq \'[.state, .headRefOid] | @tsv\')"', + ); + expect(runStep).toContain( + 'IFS=$\'\\t\' read -r PR_STATE CURRENT_HEAD_SHA <<< "$PR_DATA"', + ); + expect(staleHeadCheck).toContain( + 'Skipping stale review run: event head ${EVENT_HEAD_SHA} is no longer current', + ); + expect(staleHeadCheck).toContain('exit 0'); + }); + + it('guards PR review publication against closed or stale PRs', () => { + const runStep = step(reviewJob, 'Run review'); + const fallbackStep = step(reviewJob, 'Post fallback comment on failure'); + + expect(runStep).toContain('guard_pr_write()'); + expect(runStep).toContain( + 'Blocked PR write: PR #${pr_number} is ${state}.', + ); + expect(runStep).toContain( + 'Blocked PR write: PR #${pr_number} moved from ${expected_head} to ${current_head}.', + ); + expect(runStep).toContain('repos/*/pulls/*/reviews'); + expect(runStep).toContain('repos/*/pulls/*/comments'); + expect(runStep).toContain('repos/*/issues/*/comments'); + expect(runStep).toContain('repos/*/issues/comments/*'); + expect(runStep).toContain('QWEN_CI_REVIEW_REPO="$REPO"'); + expect(runStep).toContain('QWEN_CI_REVIEW_PR_NUMBER="$PR_NUMBER"'); + expect(runStep).toContain( + 'QWEN_CI_REVIEW_EXPECTED_HEAD_SHA="$EXPECTED_HEAD_SHA"', + ); + expect(runStep).toContain( + 'echo "expected_head_sha=$EXPECTED_HEAD_SHA" >> "$GITHUB_OUTPUT"', + ); + expect(fallbackStep).toContain('EXPECTED_HEAD_SHA:'); + expect(fallbackStep).toContain( + 'Skipping fallback comment: PR #${PR_NUMBER} is ${pr_state}.', + ); + expect(fallbackStep).toContain( + 'Skipping fallback comment: PR #${PR_NUMBER} moved from ${EXPECTED_HEAD_SHA} to ${current_head}.', + ); + }); + + it('blocks wrapped gh review writes when the PR is closed or stale', () => { + const runStep = step(reviewJob, 'Run review'); + const closedReview = runReviewGhWrapper( + runStep, + ['api', 'repos/owner/repo/pulls/123/reviews', '--input', 'review.json'], + 'CLOSED', + 'head-a', + ); + expect(closedReview.status).toBe(90); + expect(closedReview.stderr).toContain( + 'Blocked PR write: PR #123 is CLOSED', + ); + expect(closedReview.ghLog).toBe(''); + + const staleSummary = runReviewGhWrapper( + runStep, + [ + 'api', + 'repos/owner/repo/issues/comments/456', + '--method', + 'PATCH', + '--input', + 'summary.json', + ], + 'OPEN', + 'head-b', + ); + expect(staleSummary.status).toBe(90); + expect(staleSummary.stderr).toContain( + 'Blocked PR write: PR #123 moved from head-a to head-b', + ); + expect(staleSummary.ghLog).toBe(''); + }); + + it('allows wrapped gh review writes when the PR is still current', () => { + const runStep = step(reviewJob, 'Run review'); + const currentSummary = runReviewGhWrapper( + runStep, + [ + 'api', + 'repos/owner/repo/issues/123/comments', + '--method', + 'POST', + '--input', + 'summary.json', + ], + 'OPEN', + 'head-a', + ); + + expect(currentSummary.status).toBe(0); + expect(currentSummary.ghLog).toContain( + 'api repos/owner/repo/issues/123/comments --method POST --input summary.json', + ); + }); + // Whole-file `toContain` cannot tell which job a guard lives on. Slice the // resolve-pr job so these assertions fail if a future edit drops a guard // specifically from the credentialed conflict-resolution path. Bound the slice @@ -191,6 +383,7 @@ describe('qwen resolve workflow', () => { ? workflow.slice(resolveJobStart) : workflow.slice(resolveJobStart, resolveJobStart + 1 + nextJob); const reviewJob = job(workflow, 'review-pr'); + const delayAutomaticReviewJob = job(workflow, 'delay-automatic-review'); const authorizeJob = job(workflow, 'authorize'); const precheckJob = job(workflow, 'precheck-pr'); @@ -199,6 +392,17 @@ describe('qwen resolve workflow', () => { expect(authorizeJob).toContain("github.event.action != 'closed'"); }); + it('keeps automatic review jobs cancellable by concurrency', () => { + for (const lifecycleJob of [ + authorizeJob, + delayAutomaticReviewJob, + reviewJob, + ]) { + expect(lifecycleJob).toContain('!cancelled() &&'); + expect(lifecycleJob).not.toContain('\n always() &&'); + } + }); + it('does not require fork PR authors to have write permission for automatic review', () => { const authorizeStep = step( authorizeJob,