From 2e7c750392d37d0ae14f251731b8336ba6d83a0d Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Wed, 26 Aug 2026 07:45:59 +0800 Subject: [PATCH 1/6] fix(ci): run the autofix scan lane on the persistent pool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit route and review-scan ran on GitHub-hosted runners. On 2026-08-25 a hosted-runner backlog queued route for over 20 minutes — longer than the cron period — so each newer scheduled tick superseded a still-queued one under the af-005 rule, and nine consecutive schedule runs died without scanning while the ecs-qwen pool stood mostly idle. No review-feedback work was dispatched for hours. Move both jobs onto the persistent pool with the exact fork-trust and kill-switch expression the heavy jobs already use; the command jobs stay hosted. Adds the af-148 rationale and routing regression pins. --- .github/scripts/ci-runner-routing.test.mjs | 85 ++++++++++++++++++++- .github/workflows/.size-baseline | 2 +- .github/workflows/qwen-autofix.md | 31 ++++++++ .github/workflows/qwen-autofix.yml | 8 +- scripts/tests/qwen-autofix-workflow.test.js | 12 ++- 5 files changed, 130 insertions(+), 8 deletions(-) diff --git a/.github/scripts/ci-runner-routing.test.mjs b/.github/scripts/ci-runner-routing.test.mjs index 04f030056a8..d8a5e5cca3b 100644 --- a/.github/scripts/ci-runner-routing.test.mjs +++ b/.github/scripts/ci-runner-routing.test.mjs @@ -1,4 +1,5 @@ -// Runner-routing regression guards for ci.yml and serve-ab.yml. +// Runner-routing regression guards for ci.yml, serve-ab.yml, and the +// qwen-autofix.yml scan lane. // // classify_pr carries the routing logic TWICE — the `runs-on` expression // (which selects the classify job's own runner) and the `pick_runner` shell @@ -68,6 +69,13 @@ function evalRunsOn(expression, { ecsDisabled, eventName, sameRepo, assoc }) { /github\.event_name == 'merge_group'/, String(eventName === 'merge_group'), ], + // The review term MUST substitute before the plain pull_request one: + // 'pull_request' is a prefix of 'pull_request_review', so the plain + // pattern would otherwise eat the review term's head. + [ + /github\.event_name != 'pull_request_review'/, + String(eventName !== 'pull_request_review'), + ], [ /github\.event_name != 'pull_request'/, String(eventName !== 'pull_request'), @@ -80,6 +88,7 @@ function evalRunsOn(expression, { ecsDisabled, eventName, sameRepo, assoc }) { /contains\(fromJSON\('\["OWNER","MEMBER","COLLABORATOR"\]'\), github\.event\.pull_request\.author_association\)/, String(TRUSTED.includes(assoc)), ], + [/github\.repository == 'QwenLM\/qwen-code'/, 'true'], ]; let expr = expression.replace(/^\$\{\{\s*/, '').replace(/\s*\}\}$/, ''); for (const [term, value] of substitutions) { @@ -421,3 +430,77 @@ describe('serve-ab.yml runner routing', () => { ); }); }); + +describe('qwen-autofix.yml scan-lane runner routing', () => { + // route and review-scan gate the WHOLE fan-out: while they sit queued no + // review-address leg starts. A hosted-runner backlog queued them past the + // cron period, and the cron supersede rule then starved every scan round + // (2026-08-25) — so pin the lane on the persistent pool, with the + // fork-trust clause and the kill-switch intact. + const autofixDoc = parse( + readFileSync(join(workflowsDir, 'qwen-autofix.yml'), 'utf8'), + ); + // evalRunsOn unwraps the winning fromJSON label to the array it names, so + // compare against arrays, not the ECS/HOSTED string constants above. + const ECS_LABELS = ['self-hosted', 'linux', 'x64', 'ecs-qwen']; + const HOSTED_LABELS = ['ubuntu-latest']; + + for (const jobName of ['route', 'review-scan']) { + const runsOn = String(autofixDoc.jobs[jobName]['runs-on']); + + it(`${jobName} reaches the persistent pool on schedule and dispatch`, () => { + for (const eventName of ['schedule', 'workflow_dispatch']) { + assert.deepEqual( + evalRunsOn(runsOn, { + ecsDisabled: false, + eventName, + sameRepo: false, + assoc: '', + }), + ECS_LABELS, + `${jobName} must scan from the pool on ${eventName}`, + ); + } + }); + + it(`${jobName} keeps untrusted fork PR lanes hosted`, () => { + for (const eventName of ['pull_request', 'pull_request_review']) { + assert.deepEqual( + evalRunsOn(runsOn, { + ecsDisabled: false, + eventName, + sameRepo: false, + assoc: 'NONE', + }), + HOSTED_LABELS, + `${jobName} fork lane (${eventName}) must stay hosted`, + ); + assert.deepEqual( + evalRunsOn(runsOn, { + ecsDisabled: false, + eventName, + sameRepo: true, + assoc: 'NONE', + }), + ECS_LABELS, + `${jobName} same-repo lane (${eventName}) must reach the pool`, + ); + } + }); + + it(`${jobName} obeys the kill-switch on every event`, () => { + for (const eventName of ['schedule', 'workflow_dispatch']) { + assert.deepEqual( + evalRunsOn(runsOn, { + ecsDisabled: true, + eventName, + sameRepo: true, + assoc: 'OWNER', + }), + HOSTED_LABELS, + `kill-switch must win on ${eventName}`, + ); + } + }); + } +}); diff --git a/.github/workflows/.size-baseline b/.github/workflows/.size-baseline index b4a7e5e4128..c4a9509092e 100644 --- a/.github/workflows/.size-baseline +++ b/.github/workflows/.size-baseline @@ -34,7 +34,7 @@ 6495 pr-self-report-label.yml 9646 qwen-autofix-fork-bridge.yml 5942 qwen-autofix-fork-signal.yml -397656 qwen-autofix.yml +401469 qwen-autofix.yml 7061 qwen-ci-flaky-rerun.yml 158010 qwen-code-pr-review.yml 79041 qwen-fleet-shepherd.yml diff --git a/.github/workflows/qwen-autofix.md b/.github/workflows/qwen-autofix.md index 3e014680dd6..b7121b9064d 100644 --- a/.github/workflows/qwen-autofix.md +++ b/.github/workflows/qwen-autofix.md @@ -249,6 +249,7 @@ task-oriented guides — what a maintainer types and what happens next — see: - [145. review-address · Report dry-run / failure — CUMULATIVE timeout breaker — the sibling of the consecutive one above, for the…](#af-145) - [146. review-address · Report dry-run / failure — The agent committed (verify recorded committed=true before any gate could fail),…](#af-146) - [147. review-address · Report dry-run / failure — Same byte-budget hygiene as the English excerpt above. 3000 bytes ≈ 1000 CJK…](#af-147) +- [148. route — Persistent pool, not hosted: a hosted backlog queued route past the cron period, and af-005's…](#af-148) --- @@ -3727,3 +3728,33 @@ forbids HTML in failure.zh.md), but must not be able to open or close a
/ that swallows the closing tag the workflow emits below. ``` + + + +### 148. route — Persistent pool, not hosted: a hosted backlog queued route past the cron period, and af-005's supersede then starved every scan round. + +In `route` and `review-scan`. + +```text +route and review-scan run on the persistent pool, not the +hosted one. They are short trusted base-repo jobs, but they +gate the WHOLE fan-out: while they sit queued, no +review-address leg starts. On 2026-08-25 a hosted-runner +backlog queued route for over 20 minutes — longer than the +cron period — and af-005's newer-tick-supersedes-older rule +then cancelled every still-queued round: nine consecutive +schedule runs died without scanning while the ecs-qwen pool +stood mostly idle. The supersede rule stays — it is right +once route gets a runner in seconds; the fix is taking the +hosted queue out of the critical path. review-scan moves +with it because it shares the gate, and its own hosted waits +delayed every fan-out by the same backlog. Both keep the +fork-trust clause of the sibling lanes: pull_request and +pull_request_review resolve this file from the PR's own +merge commit, so only same-repo heads and write-access +authors may reach the persistent pool; everything else stays +hosted, and the kill-switch wins everywhere. Neither job +checks code out — route only decides phases, and review-scan +only calls the API — so the shared workspace needs no +restore or wipe step here. +``` diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index 513d30ef94c..c1e4f31c3ae 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -200,7 +200,10 @@ jobs: # Full rationale → qwen-autofix.md#af-004 if: |- ${{ github.repository == 'QwenLM/qwen-code' && (github.event_name != 'issue_comment' || (github.event.issue.pull_request && (startsWith(github.event.comment.body, '@qwen-code /takeover') || startsWith(github.event.comment.body, '@qwen-code /retry')))) && (github.event_name != 'pull_request' || github.event.label.name == 'autofix/takeover') && (github.event_name != 'pull_request_review' || github.event.pull_request.state == 'open') }} - runs-on: 'ubuntu-latest' + # Persistent pool, not hosted: a hosted backlog queued route past the cron + # period, and af-005's supersede then starved every scan round. + # Full rationale → qwen-autofix.md#af-148 + runs-on: '${{ (github.repository == ''QwenLM/qwen-code'' && vars.MAINTAINER_ECS_RUNNER_DISABLED != ''true'' && (github.event_name != ''pull_request'' && github.event_name != ''pull_request_review'' || github.event.pull_request.head.repo.full_name == github.repository || contains(fromJSON(''["OWNER","MEMBER","COLLABORATOR"]''), github.event.pull_request.author_association))) && fromJSON(''["self-hosted", "linux", "x64", "ecs-qwen"]'') || fromJSON(''["ubuntu-latest"]'') }}' timeout-minutes: 5 concurrency: # Concurrency is keyed by TARGET, not shared and not fully unique: @@ -2054,7 +2057,8 @@ jobs: needs: 'route' if: |- ${{ needs.route.outputs.do_review == 'true' }} - runs-on: 'ubuntu-latest' + # Same pool decision as route. Full rationale → qwen-autofix.md#af-148 + runs-on: '${{ (github.repository == ''QwenLM/qwen-code'' && vars.MAINTAINER_ECS_RUNNER_DISABLED != ''true'' && (github.event_name != ''pull_request'' && github.event_name != ''pull_request_review'' || github.event.pull_request.head.repo.full_name == github.repository || contains(fromJSON(''["OWNER","MEMBER","COLLABORATOR"]''), github.event.pull_request.author_association))) && fromJSON(''["self-hosted", "linux", "x64", "ecs-qwen"]'') || fromJSON(''["ubuntu-latest"]'') }}' timeout-minutes: 15 # A forced scan can write the same status comment as review-address. # Share its per-PR lock so neither writer can erase the other's state. diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 4edbd3fb952..f4278f0c397 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -9955,10 +9955,14 @@ exit 1 expect(step).toContain('sudo -n apt-get install'); expect(step).not.toContain('sudo apt-get'); } - // "Short jobs stay hosted" is an explicit design decision — only the - // three heavy jobs may route onto the persistent pool. - expect(routeJob).toContain("runs-on: 'ubuntu-latest'"); - expect(reviewScanJob).toContain("runs-on: 'ubuntu-latest'"); + // "Short jobs stay hosted" is the design rule — the carve-out is the + // scan lane: route and review-scan gate the WHOLE fan-out, and a + // hosted-runner backlog queued route past the cron period so the cron + // supersede rule starved every scan round (2026-08-25). They share the + // heavy jobs' exact expression, fork-trust clause and kill-switch + // included. The command jobs stay hosted. + expect(routeJob).toContain(ecsRunsOn); + expect(reviewScanJob).toContain(ecsRunsOn); for (const name of ['takeover-command', 'retry-command', 'takeover-ack']) { const job = workflow.match( From 3bc6adeb5eac4c81084a53a5efc4758ed6c4d253 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Wed, 26 Aug 2026 10:04:23 +0800 Subject: [PATCH 2/6] fix(ci): harden the scan lane for the persistent pool PR review: moving route and review-scan onto the pool exposed their gh calls to the shared, attacker-writable $HOME and left the scan's per-run WORKDIR uncleaned. Mirror the heavy jobs' gh reroute preamble in both steps, give the scan a reaper-visible WORKDIR created with umask 077 plus an always() cleanup step, extend the kill-switch test to all four events, correct the substitution-order comment, and pin the scan lane's no-checkout carve-out premise. --- .github/scripts/ci-runner-routing.test.mjs | 13 ++++--- .github/workflows/.size-baseline | 2 +- .github/workflows/qwen-autofix.md | 12 ++++++- .github/workflows/qwen-autofix.yml | 38 +++++++++++++++++++-- scripts/tests/qwen-autofix-workflow.test.js | 27 +++++++++++++++ 5 files changed, 84 insertions(+), 8 deletions(-) diff --git a/.github/scripts/ci-runner-routing.test.mjs b/.github/scripts/ci-runner-routing.test.mjs index d8a5e5cca3b..7f3e0b7dab1 100644 --- a/.github/scripts/ci-runner-routing.test.mjs +++ b/.github/scripts/ci-runner-routing.test.mjs @@ -69,9 +69,9 @@ function evalRunsOn(expression, { ecsDisabled, eventName, sameRepo, assoc }) { /github\.event_name == 'merge_group'/, String(eventName === 'merge_group'), ], - // The review term MUST substitute before the plain pull_request one: - // 'pull_request' is a prefix of 'pull_request_review', so the plain - // pattern would otherwise eat the review term's head. + // Longest term first as a convention; both patterns are quote-anchored + // (the closing quote is part of each regex), so neither can match + // inside the other and the substitution order is behaviorally inert. [ /github\.event_name != 'pull_request_review'/, String(eventName !== 'pull_request_review'), @@ -489,7 +489,12 @@ describe('qwen-autofix.yml scan-lane runner routing', () => { }); it(`${jobName} obeys the kill-switch on every event`, () => { - for (const eventName of ['schedule', 'workflow_dispatch']) { + for (const eventName of [ + 'schedule', + 'workflow_dispatch', + 'pull_request', + 'pull_request_review', + ]) { assert.deepEqual( evalRunsOn(runsOn, { ecsDisabled: true, diff --git a/.github/workflows/.size-baseline b/.github/workflows/.size-baseline index c4a9509092e..b94732d37f6 100644 --- a/.github/workflows/.size-baseline +++ b/.github/workflows/.size-baseline @@ -34,7 +34,7 @@ 6495 pr-self-report-label.yml 9646 qwen-autofix-fork-bridge.yml 5942 qwen-autofix-fork-signal.yml -401469 qwen-autofix.yml +403605 qwen-autofix.yml 7061 qwen-ci-flaky-rerun.yml 158010 qwen-code-pr-review.yml 79041 qwen-fleet-shepherd.yml diff --git a/.github/workflows/qwen-autofix.md b/.github/workflows/qwen-autofix.md index b7121b9064d..5f9b82493bf 100644 --- a/.github/workflows/qwen-autofix.md +++ b/.github/workflows/qwen-autofix.md @@ -3756,5 +3756,15 @@ authors may reach the persistent pool; everything else stays hosted, and the kill-switch wins everywhere. Neither job checks code out — route only decides phases, and review-scan only calls the API — so the shared workspace needs no -restore or wipe step here. +restore or wipe step here. The pool still leaves two marks +on the lane, both closed in the same change. One: gh reads +its config from the shared, attacker-writable $HOME, so both +steps carry the heavy jobs' gh hardening preamble — a planted +~/.config/gh/config.yml could reroute their gh calls into a +local socket, taking CI_DEV_BOT_PAT with the scan and forged +collaborator-permission answers with route. Two: review-scan +fills a per-run WORKDIR with API dumps that no VM teardown +removes on the pool, so it gets a fixed autofix* per-run path +(the age sweep can reclaim it after a hard kill), an EXIT +trap, and an always() cleanup step mirroring issue-autofix. ``` diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index c1e4f31c3ae..6854308b5dd 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -259,6 +259,15 @@ jobs: COMMENT_PR_AUTHOR: '${{ github.event.issue.user.login }}' HAS_PR_URL: '${{ github.event.issue.pull_request.url }}' run: |- + # gh has its own reroute channels: pin the host, drop planted tokens, + # and point gh at a fresh empty config dir — on the persistent pool + # the shared ~/.config/gh is attacker-writable (config.yml can carry + # http_unix_socket transport reroutes), and a forged collaborator- + # permission response here would open the /takeover and /retry gates. + # Mirrors the heavy jobs' preamble. Full rationale → qwen-autofix.md#af-148 + export GH_HOST=github.com + unset GH_ENTERPRISE_TOKEN GH_TOKEN + export GH_CONFIG_DIR="$(mktemp -d "${RUNNER_TEMP}/autofix-gh-config.XXXXXX")" DO_ISSUE=false DO_REVIEW=false TAKEOVER_ACK='' @@ -2076,6 +2085,11 @@ jobs: enum_failed: '${{ steps.scan.outputs.enum_failed }}' env: REPO: '${{ github.repository }}' + # Per-run home for the scan's API dumps. A fixed autofix* name (not + # mktemp's tmp.*) so the age sweep in the heavy jobs can reclaim it + # after a hard runner kill; cleaned normally by the always() step. + # Full rationale → qwen-autofix.md#af-148 + WORKDIR: '/tmp/autofix-scan-${{ github.run_id }}' steps: - name: 'Scan for PRs with new feedback' id: 'scan' @@ -2087,6 +2101,16 @@ jobs: REVIEW_SENDER: '${{ needs.route.outputs.review_sender }}' DISPATCH_SOURCE: "${{ github.event_name == 'workflow_dispatch' && inputs.source || '' }}" run: |- + # gh reroute hardening, mirroring the heavy jobs: pin the host, drop + # planted tokens, and use a fresh empty config dir — the pool's + # shared ~/.config/gh is attacker-writable, and a config.yml with an + # http_unix_socket reroute would swallow this step's gh calls, + # CI_DEV_BOT_PAT in the Authorization header included. + # Full rationale → qwen-autofix.md#af-148 + export GH_HOST=github.com + unset GH_ENTERPRISE_TOKEN GH_TOKEN + export GH_CONFIG_DIR="$(mktemp -d "${RUNNER_TEMP}/autofix-gh-config.XXXXXX")" + # Every lane that reaches this scan is supposed to hold the PAT: # route now declines the one event GitHub is known to run without # secrets (a fork PR's own review) before it can set do_review. @@ -2102,11 +2126,11 @@ jobs: # markers, cross-check checks and fork state) was the only way to see # a stall, so stalls stayed invisible until someone went looking. FLEET_FILE="$(mktemp)" - trap 'rm -f "${FLEET_FILE}"' EXIT + trap 'rm -f "${FLEET_FILE}"; rm -rf "${WORKDIR}"' EXIT fleet_row() { printf '%s\t%s\t%s\n' "$1" "$2" "$3" >> "${FLEET_FILE}" } - WORKDIR="$(mktemp -d)" + (umask 077; mkdir -p "${WORKDIR}") read_forced_pr_meta() { local attempt meta @@ -3268,6 +3292,16 @@ jobs: echo "targets=${TARGETS}" >> "${GITHUB_OUTPUT}" echo "has_targets=$([[ "${COUNT}" -gt 0 ]] && echo true || echo false)" >> "${GITHUB_OUTPUT}" + # Nothing else removes the per-run WORKDIR; on the persistent pool + # every scan would leave its API dumps behind forever. always() covers + # cancellation (the in-step EXIT trap does not fire on a killed run); + # only a hard runner kill abandons the dir, and the autofix* age sweep + # in the heavy jobs reclaims it. Mirrors issue-autofix's cleanup step. + # Full rationale → qwen-autofix.md#af-148 + - name: 'Clean up scan workdir' + if: 'always()' + run: 'rm -rf "${WORKDIR}"' + # =========================================================================== # REVIEW PHASE (build) — compile the trusted-base CLI bundle ONCE per scan # and fan it out to the address legs as an artifact. Each leg otherwise diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index f4278f0c397..dd30e11f466 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -9963,6 +9963,17 @@ exit 1 // included. The command jobs stay hosted. expect(routeJob).toContain(ecsRunsOn); expect(reviewScanJob).toContain(ecsRunsOn); + // af-148 carve-out premise: neither job checks code out, so the shared + // workspace needs no restore/wipe step. Pin it, so adding a checkout + // forces a deliberate update of the hygiene requirement. + expect(routeJob).not.toContain('actions/checkout'); + expect(reviewScanJob).not.toContain('actions/checkout'); + // The scan's per-run WORKDIR must not outlive the run on the pool: + // 0700 at creation and an always() cleanup step mirroring the heavy + // jobs' teardown. + expect(reviewScanJob).toContain('(umask 077; mkdir -p "${WORKDIR}")'); + expect(reviewScanJob).toContain("- name: 'Clean up scan workdir'"); + expect(reviewScanJob).toContain("if: 'always()'"); for (const name of ['takeover-command', 'retry-command', 'takeover-ack']) { const job = workflow.match( @@ -10741,6 +10752,22 @@ exit 1 expect(step.indexOf(firstGh)).toBeGreaterThan(-1); expect(ghPin).toBeLessThan(step.indexOf(firstGh)); } + // The scan lane shares the persistent pool with those PAT steps + // (af-148), so its gh calls carry the same reroute hardening: a planted + // ~/.config/gh there would send the scan's CI_DEV_BOT_PAT or route's + // collaborator-permission lookups into a local socket. + for (const [step, firstGh] of [ + [routeStep, 'gh api "repos/${REPO}/collaborators'], + [reviewScanJob, 'gh pr view'], + ]) { + const ghPin = step.indexOf('export GH_HOST=github.com'); + expect(ghPin).toBeGreaterThan(-1); + expect(step).toMatch(/unset GH_ENTERPRISE_TOKEN GH_TOKEN/); + expect(step).toContain( + 'export GH_CONFIG_DIR="$(mktemp -d "${RUNNER_TEMP}/autofix-gh-config.XXXXXX")"', + ); + expect(step.indexOf(firstGh)).toBeGreaterThan(ghPin); + } // DRIFT ALARM, NOT A BOUNDARY. The guarantee that a planted channel // cannot reach the privileged work is the `env -i` clean child, pinned // separately below; no regex over source text can be that guarantee, From f9e56f464390f83a4ce2e19b024036ef67f69aac Mon Sep 17 00:00:00 2001 From: wenshao Date: Wed, 26 Aug 2026 13:38:01 +0800 Subject: [PATCH 3/6] fix(ci): close the scan workdir's remaining pool-hygiene gaps Address round 2: pre-clean WORKDIR before creating it (run_id is public; mkdir -p alone accepts a pre-planted dir or symlink on the shared /tmp), move the fleet file inside WORKDIR so the always() step and the autofix* age sweep reclaim it when the EXIT trap cannot, fold the scan lane's gh hardening pins into the PAT-step loop so the preamble has one edit site, and pin what a mutant could previously drop silently: the WORKDIR value's autofix* prefix, the pre-clean ordering, the fleet-file home, and the cleanup step's actual rm -rf command inside its own slice. --- .github/workflows/qwen-autofix.yml | 15 ++++- scripts/tests/qwen-autofix-workflow.test.js | 61 ++++++++++++++------- 2 files changed, 53 insertions(+), 23 deletions(-) diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index 6854308b5dd..e18e06445d0 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -2120,17 +2120,28 @@ jobs: exit 1 fi + # Pre-clean before create, mirroring the heavy jobs: run_id is + # public and sequential, and mkdir -p alone would succeed over a + # dir or symlink pre-planted on the shared pool /tmp — the scan's + # redirects would then land in an attacker-chosen place, and a + # planted bot-prs.json would feed the forced-PR guard below. + rm -rf "${WORKDIR}" + (umask 077; mkdir -p "${WORKDIR}") + # Fleet visibility: every per-PR decision below also records a row so # the run summary shows the WHOLE managed fleet in one table. # Reconstructing this by hand (list bot PRs, regex each one's eval # markers, cross-check checks and fork state) was the only way to see # a stall, so stalls stayed invisible until someone went looking. - FLEET_FILE="$(mktemp)" + # Inside WORKDIR, not mktemp's /tmp/tmp.*: on a cancelled run the + # EXIT trap never fires, and only the always() step and the heavy + # jobs' autofix* age sweep reclaim what this step leaves — neither + # can reach a tmp.* name. + FLEET_FILE="${WORKDIR}/fleet.tsv" trap 'rm -f "${FLEET_FILE}"; rm -rf "${WORKDIR}"' EXIT fleet_row() { printf '%s\t%s\t%s\n' "$1" "$2" "$3" >> "${FLEET_FILE}" } - (umask 077; mkdir -p "${WORKDIR}") read_forced_pr_meta() { local attempt meta diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index dd30e11f466..36384f3adbc 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -9970,10 +9970,29 @@ exit 1 expect(reviewScanJob).not.toContain('actions/checkout'); // The scan's per-run WORKDIR must not outlive the run on the pool: // 0700 at creation and an always() cleanup step mirroring the heavy - // jobs' teardown. - expect(reviewScanJob).toContain('(umask 077; mkdir -p "${WORKDIR}")'); - expect(reviewScanJob).toContain("- name: 'Clean up scan workdir'"); - expect(reviewScanJob).toContain("if: 'always()'"); + // jobs' teardown. The value pins the autofix* prefix — the contract + // with the heavy jobs' age sweep, the only reclaim channel left after + // a hard runner kill. + expect(reviewScanJob).toContain( + "WORKDIR: '/tmp/autofix-scan-${{ github.run_id }}'", + ); + // Pre-clean immediately before create: run_id is public and mkdir -p + // alone would accept a dir or symlink pre-planted on the shared /tmp. + expect(reviewScanJob).toContain( + 'rm -rf "${WORKDIR}"\n (umask 077; mkdir -p "${WORKDIR}")', + ); + // The fleet file lives inside WORKDIR so the always() step and the age + // sweep reclaim it when the EXIT trap cannot (cancelled/killed run). + expect(reviewScanJob).toContain('FLEET_FILE="${WORKDIR}/fleet.tsv"'); + expect(reviewScanJob).not.toContain('FLEET_FILE="$(mktemp)"'); + // Pin the cleanup step's command inside its own slice, not job-wide: a + // no-op cleanup with the right name and if: shipped green before. + const scanCleanupStep = + reviewScanJob.match( + /- name: 'Clean up scan workdir'[\s\S]*?(?=\n[ ]{6}- name: '|\n[ ]{2}# ==========|$)/, + )?.[0] ?? ''; + expect(scanCleanupStep).toContain("if: 'always()'"); + expect(scanCleanupStep).toContain('rm -rf "${WORKDIR}"'); for (const name of ['takeover-command', 'retry-command', 'takeover-ack']) { const job = workflow.match( @@ -10736,10 +10755,17 @@ exit 1 // gh's own env channels are pinned/stripped BEFORE the first gh call in // each PAT step, so a $GITHUB_ENV-planted GH_HOST cannot reroute the // identity check and a planted GH_TOKEN cannot outrank the inline one. + // The scan lane shares the persistent pool with those PAT steps + // (af-148), so route and review-scan carry the same reroute hardening + // in the same loop — one preamble shape, one edit site: a planted + // ~/.config/gh there would send the scan's CI_DEV_BOT_PAT or route's + // collaborator-permission lookups into a local socket. for (const [step, firstGh] of [ [publishPrStep, 'GH_TOKEN="${GITHUB_TOKEN}" gh api user'], [pushAndReportStep, 'GH_TOKEN="${GITHUB_TOKEN}" gh api user'], [prepareStep, 'PR_LIVE="$(gh pr view'], + [routeStep, 'gh api "repos/${REPO}/collaborators'], + [reviewScanJob, 'gh pr view'], ]) { const ghPin = step.indexOf('export GH_HOST=github.com'); expect(ghPin).toBeGreaterThan(-1); @@ -10752,22 +10778,6 @@ exit 1 expect(step.indexOf(firstGh)).toBeGreaterThan(-1); expect(ghPin).toBeLessThan(step.indexOf(firstGh)); } - // The scan lane shares the persistent pool with those PAT steps - // (af-148), so its gh calls carry the same reroute hardening: a planted - // ~/.config/gh there would send the scan's CI_DEV_BOT_PAT or route's - // collaborator-permission lookups into a local socket. - for (const [step, firstGh] of [ - [routeStep, 'gh api "repos/${REPO}/collaborators'], - [reviewScanJob, 'gh pr view'], - ]) { - const ghPin = step.indexOf('export GH_HOST=github.com'); - expect(ghPin).toBeGreaterThan(-1); - expect(step).toMatch(/unset GH_ENTERPRISE_TOKEN GH_TOKEN/); - expect(step).toContain( - 'export GH_CONFIG_DIR="$(mktemp -d "${RUNNER_TEMP}/autofix-gh-config.XXXXXX")"', - ); - expect(step.indexOf(firstGh)).toBeGreaterThan(ghPin); - } // DRIFT ALARM, NOT A BOUNDARY. The guarantee that a planted channel // cannot reach the privileged work is the `env -i` clean child, pinned // separately below; no regex over source text can be that guarantee, @@ -15858,7 +15868,10 @@ exit 1 // Replay the real helper + render block over fixtures. const lines = scan.split('\n'); - const hi = lines.findIndex((l) => l.trim() === 'FLEET_FILE="$(mktemp)"'); + const hi = lines.findIndex( + (l) => l.trim() === 'FLEET_FILE="${WORKDIR}/fleet.tsv"', + ); + expect(hi).toBeGreaterThan(-1); const hj = lines.findIndex((l, i) => i > hi && l.trim() === '}'); const helper = lines.slice(hi, hj + 1).join('\n'); expect(helper).toContain('fleet_row()'); @@ -15880,6 +15893,9 @@ exit 1 [ 'set -uo pipefail', 'SUMMARY_FILE="$(mktemp)"', + // The helper references the job-level WORKDIR (fleet file home); + // its EXIT trap then reclaims this replay dir. + 'WORKDIR="$(mktemp -d)"', helper, 'COUNT=1', "fleet_row 7329 'SELECTED' '1 review + 5 inline new (round 0/5)'", @@ -15915,6 +15931,9 @@ exit 1 [ 'set -uo pipefail', 'SUMMARY_FILE="$(mktemp)"', + // The helper references the job-level WORKDIR (fleet file home); + // its EXIT trap then reclaims this replay dir. + 'WORKDIR="$(mktemp -d)"', helper, 'COUNT=0', render, From ad784af5fe6064579186d8fd0ab7f16c6c8cdb37 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Wed, 26 Aug 2026 08:12:33 +0000 Subject: [PATCH 4/6] fix(ci): pin the age-sweep reclaim consumer and re-bump the size baseline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 review: the reclaim-contract pin covered only the producer side (the scan WORKDIR basename). Pin the consumer — the heavy jobs' age-sweep line — inside both Reset autofix workspace step slices, so narrowing the glob or moving the sweep ships red. Also pin the scan lane's EXIT trap line (the in-step cleanup twin of the always() step), and re-bump .size-baseline to the workflow's actual 404284 bytes, which f9e56f46's 15 lines left under-recorded by 679. --- .github/workflows/.size-baseline | 2 +- scripts/tests/qwen-autofix-workflow.test.js | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/.github/workflows/.size-baseline b/.github/workflows/.size-baseline index b94732d37f6..e998c61491d 100644 --- a/.github/workflows/.size-baseline +++ b/.github/workflows/.size-baseline @@ -34,7 +34,7 @@ 6495 pr-self-report-label.yml 9646 qwen-autofix-fork-bridge.yml 5942 qwen-autofix-fork-signal.yml -403605 qwen-autofix.yml +404284 qwen-autofix.yml 7061 qwen-ci-flaky-rerun.yml 158010 qwen-code-pr-review.yml 79041 qwen-fleet-shepherd.yml diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 36384f3adbc..1d87f9706c2 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -9985,6 +9985,11 @@ exit 1 // sweep reclaim it when the EXIT trap cannot (cancelled/killed run). expect(reviewScanJob).toContain('FLEET_FILE="${WORKDIR}/fleet.tsv"'); expect(reviewScanJob).not.toContain('FLEET_FILE="$(mktemp)"'); + // The EXIT trap is the in-step cleanup twin of the always() step; it + // must remove both the fleet file and the per-run dir. + expect(reviewScanJob).toContain( + 'trap \'rm -f "${FLEET_FILE}"; rm -rf "${WORKDIR}"\' EXIT', + ); // Pin the cleanup step's command inside its own slice, not job-wide: a // no-op cleanup with the right name and if: shipped green before. const scanCleanupStep = @@ -10263,6 +10268,13 @@ exit 1 // world-readable window on the shared /tmp. expect(step).toContain('(umask 077; mkdir -p "${WORKDIR}")'); expect(step).not.toContain('chmod 700'); + // Consumer side of the scan lane's reclaim contract: after a hard + // runner kill neither its EXIT trap nor its always() step fires, so + // this sweep is the only channel left — the glob must keep matching + // the scan WORKDIR basename pinned in the carve-out test. + expect(step).toContain( + "find /tmp -maxdepth 1 -name 'autofix*' -mmin +1440 -exec rm -rf {} + 2>/dev/null || true", + ); } // Per-run/per-target teardown after the artifact upload: nothing else // removes these dirs on the persistent pool (PR numbers only increase). From 590c32a716405ac2d416634b5e37e0b0a75aac35 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Wed, 26 Aug 2026 11:28:14 +0000 Subject: [PATCH 5/6] fix(ci): pin issue_comment in the scan-lane routing tests Co-authored-by: Qwen-Coder --- .github/scripts/ci-runner-routing.test.mjs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/.github/scripts/ci-runner-routing.test.mjs b/.github/scripts/ci-runner-routing.test.mjs index 7f3e0b7dab1..15d5a190f8d 100644 --- a/.github/scripts/ci-runner-routing.test.mjs +++ b/.github/scripts/ci-runner-routing.test.mjs @@ -448,8 +448,15 @@ describe('qwen-autofix.yml scan-lane runner routing', () => { for (const jobName of ['route', 'review-scan']) { const runsOn = String(autofixDoc.jobs[jobName]['runs-on']); - it(`${jobName} reaches the persistent pool on schedule and dispatch`, () => { - for (const eventName of ['schedule', 'workflow_dispatch']) { + it(`${jobName} reaches the persistent pool on schedule, dispatch, and issue_comment`, () => { + // issue_comment is route's /takeover and /retry lane — pin it beside + // the cron and dispatch triggers so a later event-allowlist narrowing + // of the pool clause cannot silently demote it back to hosted. + for (const eventName of [ + 'schedule', + 'workflow_dispatch', + 'issue_comment', + ]) { assert.deepEqual( evalRunsOn(runsOn, { ecsDisabled: false, @@ -492,6 +499,7 @@ describe('qwen-autofix.yml scan-lane runner routing', () => { for (const eventName of [ 'schedule', 'workflow_dispatch', + 'issue_comment', 'pull_request', 'pull_request_review', ]) { From 36f880b317855cc891158e9b1a17ba9013a3a65b Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Wed, 26 Aug 2026 15:42:46 +0000 Subject: [PATCH 6/6] fix(ci): fail loud when the scan lane's gh config dir cannot be minted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit export VAR="$(mktemp -d ...)" reports export's status, not the substitution's: under bash -e a failing mktemp continued with an empty GH_CONFIG_DIR, and gh treats that as unset — falling back to the shared, attacker-writable ~/.config/gh the route and review-scan preambles exist to close. Check the assignment, ::error + exit 1, then export, and pin the abort behaviourally: both steps run under a mktemp that refuses only the gh-config template must fail before any gh call — removing the guard turns the witness red (probed). Also pin the issues trigger in the scan-lane routing matrix: without it an allowlist narrowing of the pool clause demotes issues back to hosted while the suite stays green (probed by mutant). --- .github/scripts/ci-runner-routing.test.mjs | 10 +- .github/workflows/qwen-autofix.yml | 20 ++- scripts/tests/qwen-autofix-workflow.test.js | 138 ++++++++++++++++++-- 3 files changed, 153 insertions(+), 15 deletions(-) diff --git a/.github/scripts/ci-runner-routing.test.mjs b/.github/scripts/ci-runner-routing.test.mjs index 15d5a190f8d..9373e9350c1 100644 --- a/.github/scripts/ci-runner-routing.test.mjs +++ b/.github/scripts/ci-runner-routing.test.mjs @@ -448,14 +448,16 @@ describe('qwen-autofix.yml scan-lane runner routing', () => { for (const jobName of ['route', 'review-scan']) { const runsOn = String(autofixDoc.jobs[jobName]['runs-on']); - it(`${jobName} reaches the persistent pool on schedule, dispatch, and issue_comment`, () => { - // issue_comment is route's /takeover and /retry lane — pin it beside - // the cron and dispatch triggers so a later event-allowlist narrowing - // of the pool clause cannot silently demote it back to hosted. + it(`${jobName} reaches the persistent pool on schedule, dispatch, issue_comment, and issues`, () => { + // issue_comment is route's /takeover and /retry lane, issues its + // label/assign trigger lane for issue-autofix — pin both beside the + // cron and dispatch triggers so a later event-allowlist narrowing of + // the pool clause cannot silently demote either back to hosted. for (const eventName of [ 'schedule', 'workflow_dispatch', 'issue_comment', + 'issues', ]) { assert.deepEqual( evalRunsOn(runsOn, { diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index e18e06445d0..79efa206d1b 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -267,7 +267,15 @@ jobs: # Mirrors the heavy jobs' preamble. Full rationale → qwen-autofix.md#af-148 export GH_HOST=github.com unset GH_ENTERPRISE_TOKEN GH_TOKEN - export GH_CONFIG_DIR="$(mktemp -d "${RUNNER_TEMP}/autofix-gh-config.XXXXXX")" + # `export VAR="$(...)"` reports export's status, not the + # substitution's: a failing mktemp must abort the step, not continue + # with an empty GH_CONFIG_DIR that gh treats as unset — falling back + # to the shared ~/.config/gh this preamble closes. + if ! GH_CONFIG_DIR="$(mktemp -d "${RUNNER_TEMP}/autofix-gh-config.XXXXXX")"; then + echo "::error::could not create gh config dir; refusing to run gh without reroute hardening" + exit 1 + fi + export GH_CONFIG_DIR DO_ISSUE=false DO_REVIEW=false TAKEOVER_ACK='' @@ -2109,7 +2117,15 @@ jobs: # Full rationale → qwen-autofix.md#af-148 export GH_HOST=github.com unset GH_ENTERPRISE_TOKEN GH_TOKEN - export GH_CONFIG_DIR="$(mktemp -d "${RUNNER_TEMP}/autofix-gh-config.XXXXXX")" + # `export VAR="$(...)"` reports export's status, not the + # substitution's: a failing mktemp must abort the step, not continue + # with an empty GH_CONFIG_DIR that gh treats as unset — falling back + # to the shared ~/.config/gh this preamble closes. + if ! GH_CONFIG_DIR="$(mktemp -d "${RUNNER_TEMP}/autofix-gh-config.XXXXXX")"; then + echo "::error::could not create gh config dir; refusing to run gh without reroute hardening" + exit 1 + fi + export GH_CONFIG_DIR # Every lane that reaches this scan is supposed to hold the PAT: # route now declines the one event GitHub is known to run without diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 1d87f9706c2..37e98f947c2 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -10772,21 +10772,36 @@ exit 1 // in the same loop — one preamble shape, one edit site: a planted // ~/.config/gh there would send the scan's CI_DEV_BOT_PAT or route's // collaborator-permission lookups into a local socket. - for (const [step, firstGh] of [ - [publishPrStep, 'GH_TOKEN="${GITHUB_TOKEN}" gh api user'], - [pushAndReportStep, 'GH_TOKEN="${GITHUB_TOKEN}" gh api user'], - [prepareStep, 'PR_LIVE="$(gh pr view'], - [routeStep, 'gh api "repos/${REPO}/collaborators'], - [reviewScanJob, 'gh pr view'], + for (const [step, firstGh, guardedMint] of [ + [publishPrStep, 'GH_TOKEN="${GITHUB_TOKEN}" gh api user', false], + [pushAndReportStep, 'GH_TOKEN="${GITHUB_TOKEN}" gh api user', false], + [prepareStep, 'PR_LIVE="$(gh pr view', false], + [routeStep, 'gh api "repos/${REPO}/collaborators', true], + [reviewScanJob, 'gh pr view', true], ]) { const ghPin = step.indexOf('export GH_HOST=github.com'); expect(ghPin).toBeGreaterThan(-1); expect(step).toMatch(/unset GH_ENTERPRISE_TOKEN GH_TOKEN/); // GH_CONFIG_DIR is PINNED to a fresh throwaway (unsetting it falls // back to the attacker-writable ~/.config/gh with http_unix_socket). - expect(step).toContain( - 'export GH_CONFIG_DIR="$(mktemp -d "${RUNNER_TEMP}/autofix-gh-config.XXXXXX")"', - ); + // route and review-scan additionally guard the mint itself: `export + // VAR="$(mktemp ...)"` reports export's status, not the + // substitution's, so a failing mktemp would sail through bash -e with + // an empty value — gh's unset fallback — unless the assignment is + // checked before export. The behavioural witness below turns red when + // the guard goes; the heavy jobs' older preambles predate the guard + // and are pinned in their current shape. + if (guardedMint) { + const guard = + 'if ! GH_CONFIG_DIR="$(mktemp -d "${RUNNER_TEMP}/autofix-gh-config.XXXXXX")"; then'; + expect(step).toContain(guard); + expect(step).toMatch(/exit 1\n\s*fi\n\s*export GH_CONFIG_DIR\n/); + expect(step.indexOf(guard)).toBeLessThan(step.indexOf(firstGh)); + } else { + expect(step).toContain( + 'export GH_CONFIG_DIR="$(mktemp -d "${RUNNER_TEMP}/autofix-gh-config.XXXXXX")"', + ); + } expect(step.indexOf(firstGh)).toBeGreaterThan(-1); expect(ghPin).toBeLessThan(step.indexOf(firstGh)); } @@ -11009,6 +11024,111 @@ exit 1 rmSync(dir, { recursive: true, force: true }); }); + it('aborts route and review-scan before any gh call when the gh config dir cannot be minted', () => { + // Behavioural witness for the guarded-mint pin above: without the guard, + // `export VAR="$(mktemp ...)"` reports export's status, not the + // substitution's, so a failing mktemp sails through bash -e with an + // empty GH_CONFIG_DIR and the step calls gh against the shared + // ~/.config/gh fallback — the reroute hole the preamble closes. Run each + // step under a mktemp that refuses ONLY the gh-config template (later + // plain mktemp calls must keep working so the unguarded mutant provably + // reaches gh) and a gh stub that records every call: the step must fail + // loud before the first record. Probed: restoring the unguarded export + // makes both cases record a gh call and drop the ::error abort. + const realMktemp = spawnSync('bash', ['-c', 'command -v mktemp'], { + encoding: 'utf8', + }).stdout.trim(); + const cases = [ + { + name: 'route', + // The managed-PR review lane reaches the collaborator-permission + // lookup (the first gh call) fastest. + block: routeStep.match(/run: \|-\n([\s\S]*)$/)?.[1], + env: { + EVENT_NAME: 'pull_request_review', + REPO: 'QwenLM/qwen-code', + PR_BASE_REF: 'main', + PR_HEAD_REPO: 'QwenLM/qwen-code', + PR_AUTHOR: 'qwen-code-dev-bot', + AUTOFIX_BOT: 'qwen-code-dev-bot', + REVIEW_BOT: 'qwen-code-ci-bot', + SENDER_LOGIN: 'qqqys', + }, + }, + { + name: 'review-scan', + // A forced PR reaches `gh pr view` through read_forced_pr_meta. + block: reviewScanJob.match( + /- name: 'Scan for PRs with new feedback'[\s\S]*?run: \|-\n([\s\S]*?)(?=\n {6}- name: ')/, + )?.[1], + env: { + EVENT_NAME: 'schedule', + FORCED_PR: '1234', + REPO: 'QwenLM/qwen-code', + GITHUB_TOKEN: 'pat', + }, + }, + ]; + for (const { name, block, env } of cases) { + expect(block, name).toBeTruthy(); + const dir = mkdtempSync(join(tmpdir(), 'autofix-ghcfg-')); + try { + const bin = join(dir, 'bin'); + mkdirSync(bin); + writeFileSync( + join(bin, 'mktemp'), + [ + '#!/bin/bash', + 'for a in "$@"; do', + ' case "$a" in *autofix-gh-config*) echo "mktemp: simulated failure: $a" >&2; exit 1 ;; esac', + 'done', + `exec '${realMktemp}' "$@"`, + '', + ].join('\n'), + ); + const callsFile = join(dir, 'gh-calls'); + writeFileSync( + join(bin, 'gh'), + `#!/bin/bash\nprintf '%s\\n' "$*" >> '${callsFile}'\nexit 0\n`, + ); + chmodSync(join(bin, 'mktemp'), 0o755); + chmodSync(join(bin, 'gh'), 0o755); + const proc = spawnSync( + 'bash', + [ + '-e', + '-o', + 'pipefail', + '-c', + // The scan's forced-PR retry loop sleeps between attempts on the + // mutant path; keep the probe fast without touching the block. + `sleep() { :; }\n${block.replace(/^ {10}/gm, '')}`, + ], + { + env: { + PATH: `${bin}:${process.env.PATH}`, + RUNNER_TEMP: dir, + WORKDIR: join(dir, 'workdir'), + GITHUB_OUTPUT: join(dir, 'github-output'), + ...env, + }, + encoding: 'utf8', + }, + ); + expect(proc.stdout, name).toContain( + '::error::could not create gh config dir', + ); + expect(proc.status, name).not.toBe(0); + expect( + existsSync(callsFile), + `${name} called gh after a failed gh-config mktemp`, + ).toBe(false); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + } + }); + it('runs both verification gates under a throwaway global git config', () => { // Same incident, the gate-side guard: the gates re-run branch tests on // the HOST, so runner ~/.gitconfig pollution failed tests the branch