diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index d825e228d22..d5c0819f537 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -748,6 +748,16 @@ jobs: # the next on the reused self-hosted workspace (reset in "Clean stale # agent state"). Must match the QWEN_HOME computed there. QWEN_HOME: '${{ runner.temp }}/qwen-home' + # KEEP THE `run` BODY BELOW FREE OF `${{ }}`. A run block containing + # one is evaluated as a single expression template, capped at 21000 + # characters — and this script is far past that. Every value it needs + # from the workflow context is passed as an environment variable, so + # the body is plain bash the runner never templates. See the + # workflow-expression-length test in + # scripts/tests/qwen-pr-review-workflow.test.js. + EVENT_NAME: '${{ github.event_name }}' + EVENT_HEAD_SHA: '${{ github.event.pull_request.head.sha }}' + MAX_TIMEOUT_MINUTES_VAR: '${{ vars.QWEN_REVIEW_MAX_TIMEOUT_MINUTES }}' run: |- set -euo pipefail fail() { @@ -960,7 +970,7 @@ jobs: if [ "$TIMEOUT_MINUTES" -le 5 ]; then fail "timeout_minutes must be greater than 5" fi - MAX_TIMEOUT_MINUTES="${{ vars.QWEN_REVIEW_MAX_TIMEOUT_MINUTES }}" + MAX_TIMEOUT_MINUTES="$MAX_TIMEOUT_MINUTES_VAR" if [ "$TIMEOUT_MINUTES" -gt "$MAX_TIMEOUT_MINUTES" ]; then fail "timeout_minutes must not exceed ${MAX_TIMEOUT_MINUTES} minutes" fi @@ -989,7 +999,7 @@ jobs: if [ "$PR_SIZE_LINES" -le 300 ]; then EFFECTIVE_TIMEOUT_MINUTES=180 else - EFFECTIVE_TIMEOUT_MINUTES="${{ vars.QWEN_REVIEW_MAX_TIMEOUT_MINUTES }}" + EFFECTIVE_TIMEOUT_MINUTES="$MAX_TIMEOUT_MINUTES_VAR" fi echo "PR #${PR_NUMBER} changed ${PR_SIZE_LINES} lines; auto timeout ${EFFECTIVE_TIMEOUT_MINUTES} minutes." else @@ -1056,8 +1066,7 @@ jobs: 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 [ "$EVENT_NAME" = "pull_request_target" ]; then 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 diff --git a/scripts/tests/qwen-pr-review-workflow.test.js b/scripts/tests/qwen-pr-review-workflow.test.js index 3a8e905b9a7..affc71a6c0b 100644 --- a/scripts/tests/qwen-pr-review-workflow.test.js +++ b/scripts/tests/qwen-pr-review-workflow.test.js @@ -2161,3 +2161,49 @@ describe('upstream-timeout headroom (PR 8507 incident)', () => { ); }); }); + +describe('workflow expression length', () => { + // A `run:` body containing `${{ }}` is evaluated as ONE expression template, + // and GitHub caps a single expression at 21000 characters. Blowing that cap + // does not fail a job — it makes the whole workflow file *invalid*, so no + // event triggers it at all and no run is even created for the ones that + // matter. That is how every automatic review and every `@qwen-code /review` + // in this repository silently stopped for ~12h on 2026-08-07: #8648 pushed + // the "Run review" body from 17705 to 22282 characters, and from that merge + // onward the only runs left were startup failures reading + // `Invalid workflow file: … (Line: 751, Col: 14): Exceeded max expression + // length 21000` (e.g. run 31239579253). CI stayed green the whole time — no + // test covered this, which is why it is covered here. + const LIMIT = 21000; + const dir = '.github/workflows'; + const files = readdirSync(dir).filter((f) => /\.ya?ml$/.test(f)); + + it('keeps every templated run block under the limit', () => { + expect(files.length).toBeGreaterThan(0); + const over = []; + for (const file of files) { + const doc = parse(readFileSync(join(dir, file), 'utf8')); + for (const [jobId, job] of Object.entries(doc?.jobs ?? {})) { + for (const step of job?.steps ?? []) { + const body = step?.run; + if (typeof body !== 'string' || !body.includes('${{')) continue; + if (body.length > LIMIT) { + over.push( + `${file} › ${jobId} › ${step.name}: ${body.length} chars`, + ); + } + } + } + } + expect(over).toEqual([]); + }); + + it('keeps the review script free of ${{ }} so its length cannot break it', () => { + // This one body is ~24000 characters — already past the limit — so it stays + // valid only while nothing templates it. Every context value it needs is + // passed through the step's `env:` instead. A single `${{ }}` added back + // here takes the entire workflow down, which the test above would also + // catch; this asserts the actual invariant a contributor has to preserve. + expect(runReviewStep()).not.toContain('${{'); + }); +}); diff --git a/scripts/tests/qwen-resolve-workflow.test.js b/scripts/tests/qwen-resolve-workflow.test.js index d3e0bc6c5c2..b4c541bc283 100644 --- a/scripts/tests/qwen-resolve-workflow.test.js +++ b/scripts/tests/qwen-resolve-workflow.test.js @@ -356,8 +356,13 @@ describe('qwen resolve workflow', () => { expect(contextStep).toContain('timeout=*)'); expect(contextStep).toContain('TIMEOUT_MINUTES="${token#timeout=}"'); expect(runStep).toContain('if [ "${#TIMEOUT_MINUTES}" -gt 3 ]; then'); + // The cap still comes from the repository variable, but reaches the script + // through the step's env: the run body must stay free of `${{ }}` or the + // whole workflow exceeds the 21000-character expression limit and becomes + // invalid. Both halves are asserted so neither can drift alone. + expect(runStep).toContain('MAX_TIMEOUT_MINUTES="$MAX_TIMEOUT_MINUTES_VAR"'); expect(runStep).toContain( - 'MAX_TIMEOUT_MINUTES="${{ vars.QWEN_REVIEW_MAX_TIMEOUT_MINUTES }}"', + "MAX_TIMEOUT_MINUTES_VAR: '${{ vars.QWEN_REVIEW_MAX_TIMEOUT_MINUTES }}'", ); expect(runStep).toContain( 'if [ "$TIMEOUT_MINUTES" -gt "$MAX_TIMEOUT_MINUTES" ]; then', @@ -403,7 +408,10 @@ describe('qwen resolve workflow', () => { expect(sizeGuardArm).toContain('if [ "$PR_SIZE_LINES" -le 300 ]; then'); expect(runStep).toContain('EFFECTIVE_TIMEOUT_MINUTES=180'); expect(runStep).toContain( - 'EFFECTIVE_TIMEOUT_MINUTES="${{ vars.QWEN_REVIEW_MAX_TIMEOUT_MINUTES }}"', + 'EFFECTIVE_TIMEOUT_MINUTES="$MAX_TIMEOUT_MINUTES_VAR"', + ); + expect(runStep).toContain( + "MAX_TIMEOUT_MINUTES_VAR: '${{ vars.QWEN_REVIEW_MAX_TIMEOUT_MINUTES }}'", ); // Slice the small-PR arm so a swap of the two assignments between the // branches fails: unordered containment keeps both texts present. @@ -416,7 +424,7 @@ describe('qwen resolve workflow', () => { runStep.indexOf('else', smallPrStart), ); expect(smallPrArm).toContain('EFFECTIVE_TIMEOUT_MINUTES=180'); - expect(smallPrArm).not.toContain('vars.QWEN_REVIEW_MAX_TIMEOUT_MINUTES'); + expect(smallPrArm).not.toContain('MAX_TIMEOUT_MINUTES_VAR'); expect(runStep).not.toContain('EFFECTIVE_TIMEOUT_MINUTES=210'); expect(runStep).toContain( 'echo "effective_timeout_minutes=$EFFECTIVE_TIMEOUT_MINUTES"', @@ -532,15 +540,26 @@ describe('qwen resolve workflow', () => { it('skips stale automatic review runs before invoking qwen', () => { const runStep = step(reviewJob, 'Run review'); + const staleHeadStart = runStep.indexOf( + 'if [ "$EVENT_NAME" = "pull_request_target" ]; then', + ); + // Without this, a reworded guard makes `indexOf` return -1 and the slice + // below silently degrades instead of failing. + expect(staleHeadStart).toBeGreaterThan(-1); const staleHeadCheck = runStep.slice( - runStep.indexOf( - 'if [ "${{ github.event_name }}" = "pull_request_target" ]; then', - ), + staleHeadStart, runStep.indexOf('PROMPT="/review ${REVIEW_URL}"'), ); + // Both context values arrive as step env so the run body carries no + // `${{ }}` — see the expression-length test in + // qwen-pr-review-workflow.test.js for why that is load-bearing. + expect(runStep).toContain("EVENT_NAME: '${{ github.event_name }}'"); + expect(runStep).toContain( + "EVENT_HEAD_SHA: '${{ github.event.pull_request.head.sha }}'", + ); expect(staleHeadCheck).toContain( - 'EVENT_HEAD_SHA="${{ github.event.pull_request.head.sha }}"', + 'if [ "$CURRENT_HEAD_SHA" != "$EVENT_HEAD_SHA" ]; then', ); expect(runStep).toContain( 'PR_DATA="$(gh pr view "$PR_NUMBER" --repo "$REPO" --json state,headRefOid --jq \'[.state, .headRefOid] | @tsv\')"',