From 1c565927b71ef4198a24ebccd2143d99c8e2347b Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 6 Aug 2026 02:17:53 +0800 Subject: [PATCH 1/8] ci(autofix): run heavy autofix jobs on the ECS pool with hosted fallback The heavy autofix jobs (issue-autofix, build-cli, review-address) were pinned to GitHub-hosted runners, where they hold up to five 300-minute slots per scan. Combined with CI, that saturates the org's ~20 hosted concurrency slots and starves the PR-review workflow's hosted-only jobs (delay timer, fork authorize, /resolve). Route the three heavy jobs to the persistent ecs-qwen pool using the same repository-guarded expression as the rest of the fleet, with an ubuntu-latest fallback for forks of this repo and for MAINTAINER_ECS_RUNNER_DISABLED. This is safe for the fleet's two secret-bearing agent jobs: - Every review-address target is live-gated to write+ (internal) authors at scan AND address time; in-repo PR heads already require push access. - CI_DEV_BOT_PAT is a write-level branch-push token with no privilege escalation (cannot bypass branch protection). - The ECS pool ships docker, and repo-hygiene already runs the sandbox image there in production; sandbox containers use --rm with no fixed name, so concurrent legs on one machine do not collide. Relax the 'Check runner environment' gate to accept self-hosted, and drop the github-hosted condition on 'Set up Node.js' so Node 22 is provisioned on ECS. The contract test is updated to pin the new ECS routing expression (including the repository guard and hosted fallback) instead of the old hosted-only one. --- .github/workflows/qwen-autofix.yml | 30 +++++++++-------- scripts/tests/qwen-autofix-workflow.test.js | 36 +++++++++++++++------ 2 files changed, 43 insertions(+), 23 deletions(-) diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index 7a36897328b..d6a591d7b8f 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -658,7 +658,11 @@ jobs: needs.route.outputs.do_issue == 'true' && (github.event_name != 'schedule' || (needs.review-scan.result == 'success' && needs.review-scan.outputs.has_targets != 'true')) }} - runs-on: 'ubuntu-latest' + # Secret-bearing and executes agent-driven code, but the agent runs inside + # the docker sandbox image (which the ECS pool ships) and only ever writes a + # new branch as the dev-bot - it never executes a foreign author's code. + # Forks of this repo (and MAINTAINER_ECS_RUNNER_DISABLED) fall back to hosted. + 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: 180 concurrency: group: 'qwen-autofix-issue-${{ needs.route.outputs.issue_number || github.run_id }}' @@ -719,16 +723,14 @@ jobs: RUNNER_ENVIRONMENT: '${{ runner.environment }}' run: |- case "${RUNNER_ENVIRONMENT}" in - github-hosted) ;; + github-hosted|self-hosted) ;; *) echo "::error::Unsupported runner environment: ${RUNNER_ENVIRONMENT:-unset}." exit 1 ;; esac - - name: 'Set up Node.js (hosted)' - if: |- - ${{ runner.environment == 'github-hosted' }} + - name: 'Set up Node.js' uses: 'actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e' # v6.4.0 with: node-version: '22.x' @@ -2604,7 +2606,7 @@ jobs: needs: ['route', 'review-scan'] if: |- ${{ needs.review-scan.outputs.has_targets == 'true' }} - runs-on: '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: 30 permissions: contents: 'read' @@ -2618,9 +2620,7 @@ jobs: fetch-depth: 1 persist-credentials: false - - name: 'Set up Node.js (hosted)' - if: |- - ${{ runner.environment == 'github-hosted' }} + - name: 'Set up Node.js' uses: 'actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e' # v6.4.0 with: node-version: '22.x' @@ -2692,7 +2692,11 @@ jobs: needs: ['route', 'review-scan', 'build-cli'] if: |- ${{ needs.review-scan.outputs.has_targets == 'true' }} - runs-on: 'ubuntu-latest' + # Secret-bearing and executes PR code, but every target is live-gated to + # write+ (internal) authors at scan AND address time, and the ECS pool ships + # docker for the sandbox image. Forks of this repo (and + # MAINTAINER_ECS_RUNNER_DISABLED) fall back to hosted. + 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: 300 permissions: contents: 'read' @@ -2822,16 +2826,14 @@ jobs: RUNNER_ENVIRONMENT: '${{ runner.environment }}' run: |- case "${RUNNER_ENVIRONMENT}" in - github-hosted) ;; + github-hosted|self-hosted) ;; *) echo "::error::Unsupported runner environment: ${RUNNER_ENVIRONMENT:-unset}." exit 1 ;; esac - - name: 'Set up Node.js (hosted)' - if: |- - ${{ runner.environment == 'github-hosted' }} + - name: 'Set up Node.js' uses: 'actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e' # v6.4.0 with: node-version: '22.x' diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 910ec4c427a..b279c2e1912 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -36,7 +36,7 @@ const autofixContractsScript = readFileSync(autofixContractsScriptPath, 'utf8'); const autofixRunnerScriptPath = '.qwen/skills/autofix/scripts/run-agent.mjs'; const checkBotCredentialsStep = workflow.match( - /- name: 'Check bot credentials'[\s\S]*?(?=\n[ ]{6}- name: 'Set up Node.js \(hosted\)')/, + /- name: 'Check bot credentials'[\s\S]*?(?=\n[ ]{6}- name: 'Set up Node.js')/, )?.[0] ?? ''; const routeStep = workflow.match( @@ -148,9 +148,8 @@ const installAndBuildSteps = /- name: 'Install dependencies and build'[\s\S]*?(?=\n[ ]{6}- name: ')/g, ) ?? []; const nodeSetupSteps = - workflow.match( - /- name: 'Set up Node.js \(hosted\)'[\s\S]*?(?=\n[ ]{6}- name: ')/g, - ) ?? []; + workflow.match(/- name: 'Set up Node.js'[\s\S]*?(?=\n[ ]{6}- name: ')/g) ?? + []; function readAutofixSkill() { return readFileSync('.qwen/skills/autofix/SKILL.md', 'utf8'); @@ -5061,7 +5060,7 @@ describe('qwen-autofix workflow', () => { expect(pushAndReportStep.length).toBeGreaterThan(0); expect(withdrawClaimStep.length).toBeGreaterThan(0); expect(workflow.indexOf("- name: 'Check bot credentials'")).toBeLessThan( - workflow.indexOf("- name: 'Set up Node.js (hosted)'"), + workflow.indexOf("- name: 'Set up Node.js'"), ); expect(checkBotCredentialsStep).toContain( 'GH_TOKEN="${GITHUB_TOKEN}" gh api user --jq \'.login\'', @@ -5204,12 +5203,31 @@ describe('qwen-autofix workflow', () => { ); }); - it('runs heavy autofix jobs on hosted runners with sandbox images', () => { + it('runs heavy autofix jobs on the ECS pool with hosted fallback', () => { const workflowAndSkill = `${workflow}\n${readAutofixSkill()}`; - expect(workflow).toMatch(/issue-autofix:[\s\S]*?runs-on: 'ubuntu-latest'/); - expect(workflow).toMatch(/review-address:[\s\S]*?runs-on: 'ubuntu-latest'/); - expect(workflow).toMatch(/build-cli:[\s\S]*?runs-on: 'ubuntu-latest'/); + // Each heavy job routes to the persistent ECS pool (every target is + // live-gated to write+ internal authors and the ECS pool ships docker), + // with a hosted fallback for forks of this repo and when ECS routing is + // disabled. Pin the exact expression so neither the repository guard nor + // the hosted fallback can be dropped silently. + const ecsRunsOn = + "runs-on: '${{ (github.repository == ''QwenLM/qwen-code'' && vars.MAINTAINER_ECS_RUNNER_DISABLED != ''true'') && fromJSON(''[\"self-hosted\", \"linux\", \"x64\", \"ecs-qwen\"]'') || fromJSON(''[\"ubuntu-latest\"]'') }}'"; + const heavyJobRunsOn = { + 'issue-autofix': + workflow.match( + /\n {2}issue-autofix:[\s\S]*?(?=\n[ ]{2}# ==========)/, + )?.[0] ?? '', + 'build-cli': + workflow.match( + /\n {2}build-cli:[\s\S]*?(?=\n {2}review-address:)/, + )?.[0] ?? '', + 'review-address': + workflow.match(/\n {2}review-address:[\s\S]*$/)?.[0] ?? '', + }; + for (const runsOn of Object.values(heavyJobRunsOn)) { + expect(runsOn).toContain(ecsRunsOn); + } expect(workflow).not.toContain( '["self-hosted", "linux", "x64", "autofix"]', ); From abe81685ff8463cfc40b5c8bb4b2aaea914661e7 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 5 Aug 2026 23:55:33 +0000 Subject: [PATCH 2/8] ci(autofix): harden ECS-routed heavy jobs for the shared persistent pool - Restore workspace ownership before checkout in issue-autofix, build-cli, and review-address, matching the sibling pool workflows (root-owned leftovers from containerised jobs break checkout/npm ci). - Sanitize the reused workspace's local git config (allowlisted unset-all + symlink-aware hook sweep, qwen-triage's pattern) before the PAT-bearing git steps in issue-autofix and review-address. - Give issue-autofix a per-run WORKDIR (/tmp/autofix-): pool registrations share one /tmp and issue-phase runs never serialize. - Drop stale local autofix/issue-* branches after checkout so a retry on the same registration cannot hit "branch already exists" or push a failed attempt's stale commits. - Extend the ECS routing guard for PR-family events (same-repo head or write+ author), mirroring ci.yml's classify_pr convention, and fix the runs-on comments that claimed fork PRs already fell back. - Stop persisting CI_DEV_BOT_PAT into the origin remote URL: publish pushes authenticate transiently, and the redundant set-url in 'Push and report' is removed (push/fetch already use a URL inline). - Pin all of the above in the workflow contract tests, reusing hoisted job-block constants. --- .github/workflows/qwen-autofix.yml | 127 ++++++++++++++++++-- scripts/tests/qwen-autofix-workflow.test.js | 91 ++++++++------ 2 files changed, 171 insertions(+), 47 deletions(-) diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index d6a591d7b8f..82a25898fe1 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -661,8 +661,10 @@ jobs: # Secret-bearing and executes agent-driven code, but the agent runs inside # the docker sandbox image (which the ECS pool ships) and only ever writes a # new branch as the dev-bot - it never executes a foreign author's code. - # Forks of this repo (and MAINTAINER_ECS_RUNNER_DISABLED) fall back to hosted. - runs-on: '${{ (github.repository == ''QwenLM/qwen-code'' && vars.MAINTAINER_ECS_RUNNER_DISABLED != ''true'') && fromJSON(''["self-hosted", "linux", "x64", "ecs-qwen"]'') || fromJSON(''["ubuntu-latest"]'') }}' + # Forks of this repo (and MAINTAINER_ECS_RUNNER_DISABLED) fall back to + # hosted; PR-family events additionally need a same-repo head or a write+ + # author, mirroring the fleet's ECS routing guard (ci.yml's classify_pr). + 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: 180 concurrency: group: 'qwen-autofix-issue-${{ needs.route.outputs.issue_number || github.run_id }}' @@ -671,12 +673,52 @@ jobs: contents: 'read' env: REPO: '${{ github.repository }}' - WORKDIR: '/tmp/autofix' + # Per-run private dir: this pool carries many registrations sharing one + # OS /tmp, and issue-phase runs never serialize against each other, so + # a fixed path let concurrent runs clobber each other's decision files. + WORKDIR: '/tmp/autofix-${{ github.run_id }}' EVENT_NAME: '${{ github.event_name }}' READY_FOR_AGENT_LABEL: 'status/ready-for-agent' AUTOFIX_APPROVED_LABEL: 'autofix/approved' AUTOFIX_ISSUE_EXCLUDES: 'no:assignee -linked:pr -label:autofix/skip -label:autofix/in-progress -label:status/need-information -label:status/need-retesting sort:created-desc' steps: + # Self-hosted runners reuse the workspace; a prior containerised job + # can leave root-owned, read-only files anywhere in it. Restore + # ownership and write permission unconditionally before checkout. + - name: 'Restore workspace ownership' + run: |- + set -uo pipefail + RUNNER_UID="$(id -u)" + RUNNER_GID="$(id -g)" + if [ "$RUNNER_UID" != "0" ]; then + chown -R "$RUNNER_UID:$RUNNER_GID" "$GITHUB_WORKSPACE" 2>/dev/null || sudo -n chown -R "$RUNNER_UID:$RUNNER_GID" "$GITHUB_WORKSPACE" || echo "::warning::could not restore workspace ownership; checkout may fail on leftover root-owned files" + fi + chmod -R u+rwX "$GITHUB_WORKSPACE" 2>/dev/null || sudo -n chmod -R u+rwX "$GITHUB_WORKSPACE" || echo "::warning::could not restore workspace write permissions; checkout may fail on leftover read-only files" + + # Self-hosted runners keep the workspace between runs, and other pool + # jobs execute human-authored code as the runner user, so a prior job + # can plant git exec knobs (core.fsmonitor, filter.*.smudge, + # diff.external, includeIf, hooks) in the local config that would fire + # inside THIS job's PAT-bearing git steps. Keep a known-safe allowlist + # and unset everything else — the same pattern qwen-triage uses on + # this pool. No-op on a fresh hosted runner. + - name: 'Sanitize workspace git config' + run: |- + set -uo pipefail + # `.git` is a directory in a normal checkout but a gitlink file in + # a worktree; -e covers both, and a missing .git (first run) too. + if [ ! -e .git ]; then + echo "no prior workspace; nothing to sanitize" + exit 0 + fi + git config --local --name-only --list 2>/dev/null \ + | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } \ + | while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done + HOOKS_DIR="$(git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" + # Match -type f OR -type l: a symlinked hook survives a bare + # `-type f` sweep and still fires on the next checkout. + find "$HOOKS_DIR" \( -type f -o -type l \) ! -name '*.sample' -delete 2>/dev/null || true + - name: 'Checkout' uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 with: @@ -688,6 +730,17 @@ jobs: rm -rf "${WORKDIR}" mkdir -p "${WORKDIR}" + # Self-hosted runners keep the workspace's .git across runs, so a + # failed earlier attempt's local branch survives here: the agent's + # branch create then dies "branch already exists", or an adaptation + # checks out the stale line and pushes the failed attempt's commits + # into the new PR. Drop them deterministically (refs survive + # actions/checkout's untracked-file clean). + - name: 'Drop stale autofix branches' + run: |- + git for-each-ref --format='%(refname:short)' 'refs/heads/autofix/issue-*' \ + | xargs -r -n 1 git branch -D 2>/dev/null || true + # Same staging as the review-address job: the verify gate always runs the # trusted checkout's copy of the schema gate, never a working-tree copy. - name: 'Stage trusted schema gate' @@ -1262,7 +1315,7 @@ jobs: uses: 'actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a' # v7.0.1 with: name: 'autofix-issue-artifacts' - path: '/tmp/autofix/' + path: '/tmp/autofix-${{ github.run_id }}/' if-no-files-found: 'ignore' - name: 'Publish PR' @@ -1299,9 +1352,12 @@ jobs: fi BRANCH="autofix/issue-${ISSUE}" git config --local --unset-all http.https://github.com/.extraheader || true - git remote set-url origin "https://x-access-token:${GITHUB_TOKEN}@github.com/${REPO}.git" git config core.hooksPath /dev/null - git push --no-verify origin "${BRANCH}" + # Authenticate the push transiently, never writing the PAT into + # the origin remote: the pool keeps this workspace between runs, + # so a token persisted to .git/config would stay readable on the + # shared host until a later job wiped it. + git push --no-verify "https://x-access-token:${GITHUB_TOKEN}@github.com/${REPO}.git" "${BRANCH}" PR_URL="$(gh pr create --repo "${REPO}" \ --base main --head "${BRANCH}" \ @@ -2606,13 +2662,26 @@ jobs: needs: ['route', 'review-scan'] if: |- ${{ needs.review-scan.outputs.has_targets == 'true' }} - 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'' && (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: 30 permissions: contents: 'read' outputs: base_sha: '${{ steps.meta.outputs.base_sha }}' steps: + # Self-hosted runners reuse the workspace; a prior containerised job + # can leave root-owned, read-only files anywhere in it. Restore + # ownership and write permission unconditionally before checkout. + - name: 'Restore workspace ownership' + run: |- + set -uo pipefail + RUNNER_UID="$(id -u)" + RUNNER_GID="$(id -g)" + if [ "$RUNNER_UID" != "0" ]; then + chown -R "$RUNNER_UID:$RUNNER_GID" "$GITHUB_WORKSPACE" 2>/dev/null || sudo -n chown -R "$RUNNER_UID:$RUNNER_GID" "$GITHUB_WORKSPACE" || echo "::warning::could not restore workspace ownership; checkout may fail on leftover root-owned files" + fi + chmod -R u+rwX "$GITHUB_WORKSPACE" 2>/dev/null || sudo -n chmod -R u+rwX "$GITHUB_WORKSPACE" || echo "::warning::could not restore workspace write permissions; checkout may fail on leftover read-only files" + - name: 'Checkout trusted base' uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 with: @@ -2695,8 +2764,10 @@ jobs: # Secret-bearing and executes PR code, but every target is live-gated to # write+ (internal) authors at scan AND address time, and the ECS pool ships # docker for the sandbox image. Forks of this repo (and - # MAINTAINER_ECS_RUNNER_DISABLED) fall back to hosted. - runs-on: '${{ (github.repository == ''QwenLM/qwen-code'' && vars.MAINTAINER_ECS_RUNNER_DISABLED != ''true'') && fromJSON(''["self-hosted", "linux", "x64", "ecs-qwen"]'') || fromJSON(''["ubuntu-latest"]'') }}' + # MAINTAINER_ECS_RUNNER_DISABLED) fall back to hosted; PR-family events + # additionally need a same-repo head or a write+ author, mirroring the + # fleet's ECS routing guard (ci.yml's classify_pr). + 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: 300 permissions: contents: 'read' @@ -2782,6 +2853,43 @@ jobs: exit 1 fi + # Self-hosted runners reuse the workspace; a prior containerised job + # can leave root-owned, read-only files anywhere in it. Restore + # ownership and write permission unconditionally before checkout. + - name: 'Restore workspace ownership' + run: |- + set -uo pipefail + RUNNER_UID="$(id -u)" + RUNNER_GID="$(id -g)" + if [ "$RUNNER_UID" != "0" ]; then + chown -R "$RUNNER_UID:$RUNNER_GID" "$GITHUB_WORKSPACE" 2>/dev/null || sudo -n chown -R "$RUNNER_UID:$RUNNER_GID" "$GITHUB_WORKSPACE" || echo "::warning::could not restore workspace ownership; checkout may fail on leftover root-owned files" + fi + chmod -R u+rwX "$GITHUB_WORKSPACE" 2>/dev/null || sudo -n chmod -R u+rwX "$GITHUB_WORKSPACE" || echo "::warning::could not restore workspace write permissions; checkout may fail on leftover read-only files" + + # Self-hosted runners keep the workspace between runs, and other pool + # jobs execute human-authored code as the runner user, so a prior job + # can plant git exec knobs (core.fsmonitor, filter.*.smudge, + # diff.external, includeIf, hooks) in the local config that would fire + # inside THIS job's PAT-bearing git steps. Keep a known-safe allowlist + # and unset everything else — the same pattern qwen-triage uses on + # this pool. No-op on a fresh hosted runner. + - name: 'Sanitize workspace git config' + run: |- + set -uo pipefail + # `.git` is a directory in a normal checkout but a gitlink file in + # a worktree; -e covers both, and a missing .git (first run) too. + if [ ! -e .git ]; then + echo "no prior workspace; nothing to sanitize" + exit 0 + fi + git config --local --name-only --list 2>/dev/null \ + | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } \ + | while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done + HOOKS_DIR="$(git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" + # Match -type f OR -type l: a symlinked hook survives a bare + # `-type f` sweep and still fires on the next checkout. + find "$HOOKS_DIR" \( -type f -o -type l \) ! -name '*.sample' -delete 2>/dev/null || true + - name: 'Checkout trusted base' uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 with: @@ -3902,7 +4010,6 @@ jobs: if [[ "${OUTCOME}" == "fixed" ]]; then NEXT_ROUND="$(( ROUND + 1 ))" git config --local --unset-all http.https://github.com/.extraheader || true - git remote set-url origin "https://x-access-token:${GITHUB_TOKEN}@github.com/${REPO}.git" # This step carries the PAT; the branch carries PR-controlled # .husky hooks (hooksPath was pointed there so the AGENT's # commits get checked). A pre-push hook would execute that code diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index b279c2e1912..21ced8eec80 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -50,6 +50,13 @@ const reviewScanJob = const issueAutofixJob = workflow.match(/\n {2}issue-autofix:[\s\S]*?(?=\n[ ]{2}# ==========)/)?.[0] ?? ''; +const buildCliJob = + workflow.match(/\n {2}build-cli:[\s\S]*?(?=\n {2}review-address:)/)?.[0] ?? + ''; +// review-address is the last job in the file, so there is no trailing +// `# ====` separator to anchor on — match to EOF. +const reviewAddressJob = + workflow.match(/\n {2}review-address:[\s\S]*$/)?.[0] ?? ''; const publishPrStep = workflow.match( /- name: 'Publish PR'[\s\S]*?(?=\n[ ]{6}- name: 'Withdraw claim on failure')/, @@ -1285,12 +1292,8 @@ describe('qwen-autofix workflow', () => { // raising it to the target budget, both let one backlog open every agent // run at once — which is the thing the cap exists to prevent, and neither // would fail any other test. - // review-address is the last job in the file, so there is no trailing - // `# ====` separator to anchor on — match to EOF. - const addressJob = - workflow.match(/\n {2}review-address:[\s\S]*$/)?.[0] ?? ''; - expect(addressJob).toContain('matrix:'); - const parallel = Number(addressJob.match(/max-parallel: (\d+)/)?.[1]); + expect(reviewAddressJob).toContain('matrix:'); + const parallel = Number(reviewAddressJob.match(/max-parallel: (\d+)/)?.[1]); const targetBudget = Number( workflow.match(/MAX_TARGETS_PER_SCAN: '(\d+)'/)?.[1], ); @@ -5209,25 +5212,30 @@ describe('qwen-autofix workflow', () => { // Each heavy job routes to the persistent ECS pool (every target is // live-gated to write+ internal authors and the ECS pool ships docker), // with a hosted fallback for forks of this repo and when ECS routing is - // disabled. Pin the exact expression so neither the repository guard nor - // the hosted fallback can be dropped silently. + // disabled. PR-family events additionally need a same-repo head or a + // write+ author — the fleet's ECS routing guard (ci.yml's classify_pr). + // Pin the exact expression so neither the repository guard nor the + // hosted fallback can be dropped silently. const ecsRunsOn = - "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'' && (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\"]'') }}'"; const heavyJobRunsOn = { - 'issue-autofix': - workflow.match( - /\n {2}issue-autofix:[\s\S]*?(?=\n[ ]{2}# ==========)/, - )?.[0] ?? '', - 'build-cli': - workflow.match( - /\n {2}build-cli:[\s\S]*?(?=\n {2}review-address:)/, - )?.[0] ?? '', - 'review-address': - workflow.match(/\n {2}review-address:[\s\S]*$/)?.[0] ?? '', + 'issue-autofix': issueAutofixJob, + 'build-cli': buildCliJob, + 'review-address': reviewAddressJob, }; for (const runsOn of Object.values(heavyJobRunsOn)) { expect(runsOn).toContain(ecsRunsOn); } + // The widened runner-environment guard is what lets ECS-routed runs pass + // 'Check runner environment' at all — pin the accepted set in both jobs + // that carry it (build-cli has no such step): reverting either to the + // hosted-only pattern kills every ECS-routed run at that step while the + // rest of this suite stays green. + expect( + workflow.match( + /case "\$\{RUNNER_ENVIRONMENT\}" in\n\s+github-hosted\|self-hosted\) ;;/g, + ), + ).toHaveLength(2); expect(workflow).not.toContain( '["self-hosted", "linux", "x64", "autofix"]', ); @@ -5326,6 +5334,10 @@ describe('qwen-autofix workflow', () => { // Node bump applied to two of the three jobs) must not ship green. expect(nodeSetupSteps).toHaveLength(3); for (const step of nodeSetupSteps) { + // Unconditional: re-adding the hosted-only `if` skips setup-node on + // every ECS-routed run and leaves the job on whatever Node the pool + // image happens to ship, while every recipe assertion stays green. + expect(step).not.toContain("runner.environment == 'github-hosted'"); expect(step).toContain( 'actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e', ); @@ -5341,21 +5353,15 @@ describe('qwen-autofix workflow', () => { // per scan) before the agent could start. The legs download the shared // artifact instead; only their npm ci remains (the agent and the verify // gate still need node_modules against the PR branch). - const buildCliJob = - workflow.match( - /\n {2}build-cli:[\s\S]*?(?=\n {2}review-address:)/, - )?.[0] ?? ''; - const addressJob = - workflow.match(/\n {2}review-address:[\s\S]*$/)?.[0] ?? ''; const stepOf = (job, name) => job.match( new RegExp(`- name: '${name}'[\\s\\S]*?(?=\\n[ ]{6}- name: |$)`), )?.[0] ?? ''; expect(buildCliJob).toBeTruthy(); - expect(addressJob).toBeTruthy(); + expect(reviewAddressJob).toBeTruthy(); expect(buildCliJob).toContain("needs: ['route', 'review-scan']"); - expect(addressJob).toContain( + expect(reviewAddressJob).toContain( "needs: ['route', 'review-scan', 'build-cli']", ); // An idle tick (no review targets) must not spend a build; the issue @@ -5379,13 +5385,13 @@ describe('qwen-autofix workflow', () => { // steps.meta.outputs.base_sha resolves to '' at runtime and every leg // silently checks out the event-default ref. expect(stepOf(buildCliJob, 'Upload CLI bundle')).toContain("id: 'meta'"); - expect(stepOf(addressJob, 'Checkout trusted base')).toContain( + expect(stepOf(reviewAddressJob, 'Checkout trusted base')).toContain( "ref: '${{ needs.build-cli.outputs.base_sha }}'", ); // The guard must fail the leg LOUD before the checkout: an empty ref // makes actions/checkout fall back to the event default — on // pull_request_review triggers the PR merge ref. - const validateShaStep = stepOf(addressJob, 'Validate bundle SHA'); + const validateShaStep = stepOf(reviewAddressJob, 'Validate bundle SHA'); expect(validateShaStep).toContain( "BASE_SHA: '${{ needs.build-cli.outputs.base_sha }}'", ); @@ -5393,9 +5399,9 @@ describe('qwen-autofix workflow', () => { 'if [[ ! "${BASE_SHA}" =~ ^[0-9a-f]{40}$ ]]; then', ); expect(validateShaStep).toContain('exit 1'); - expect(addressJob.indexOf("- name: 'Validate bundle SHA'")).toBeLessThan( - addressJob.indexOf("- name: 'Checkout trusted base'"), - ); + expect( + reviewAddressJob.indexOf("- name: 'Validate bundle SHA'"), + ).toBeLessThan(reviewAddressJob.indexOf("- name: 'Checkout trusted base'")); // The artifact is the repo-root dist/ only — copy_bundle_assets.js // already gathers every runtime asset under it; packages/*/dist would @@ -5407,12 +5413,12 @@ describe('qwen-autofix workflow', () => { expect(buildCliJob).toContain('retention-days: 1'); expect(buildCliJob).toContain("if-no-files-found: 'error'"); - const restoreStep = stepOf(addressJob, 'Restore CLI bundle'); + const restoreStep = stepOf(reviewAddressJob, 'Restore CLI bundle'); expect(restoreStep).toContain( 'tar -xzf "${RUNNER_TEMP}/cli-dist/qwen-cli-dist.tar.gz"', ); expect(restoreStep).toContain('test -f dist/cli.js'); - const downloadStep = stepOf(addressJob, 'Download CLI bundle'); + const downloadStep = stepOf(reviewAddressJob, 'Download CLI bundle'); expect(downloadStep).toContain("name: 'qwen-autofix-cli-dist'"); // Download directory and restore extract path are one contract — pin // both sides so a rename of either fails this suite. @@ -5420,7 +5426,7 @@ describe('qwen-autofix workflow', () => { // The leg itself never rebuilds the base bundle — that is the entire // point of the fan-out. - const legInstall = stepOf(addressJob, 'Install dependencies'); + const legInstall = stepOf(reviewAddressJob, 'Install dependencies'); expect(legInstall).toContain('npm ci --prefer-offline'); expect(legInstall).not.toContain('npm run build'); expect(legInstall).not.toContain('npm run bundle'); @@ -5463,7 +5469,11 @@ describe('qwen-autofix workflow', () => { it('clears persistent autofix workdirs before agent steps run', () => { expect(resetAutofixWorkspaceSteps).toHaveLength(2); - expect(workflow).toContain("WORKDIR: '/tmp/autofix'"); + // Per-run private dir: pool registrations share one /tmp and issue-phase + // runs never serialize against each other. The artifact upload reads the + // same per-run path — pin both sides so neither is left behind. + expect(workflow).toContain("WORKDIR: '/tmp/autofix-${{ github.run_id }}'"); + expect(workflow).toContain("path: '/tmp/autofix-${{ github.run_id }}/'"); expect(workflow).toContain( "WORKDIR: '/tmp/autofix-review-${{ matrix.target.pr }}'", ); @@ -6366,7 +6376,14 @@ describe('qwen-autofix workflow', () => { // force flag; long options (--no-verify) start with -- and are exempt. expect(workflow).not.toMatch(/\bgit push\b[^\n]* -[a-zA-Z]*f\b/); expect(workflow).not.toMatch(/\bgit push\b[^\n]* \+\S/); - expect(publishPrStep).toContain('git push --no-verify origin "${BRANCH}"'); + expect(publishPrStep).toContain( + 'git push --no-verify "https://x-access-token:${GITHUB_TOKEN}@github.com/${REPO}.git" "${BRANCH}"', + ); + // Neither PAT push may persist the token into the reused workspace's + // .git/config — a `git remote set-url` carrying the PAT survives the job + // on the shared pool. Both authenticate transiently (URL on the command). + expect(publishPrStep).not.toContain('git remote set-url'); + expect(pushAndReportStep).not.toContain('git remote set-url'); expect(pushAndReportStep).toContain( 'git push --no-verify "${PUSH_URL}" HEAD:"${BRANCH}"', ); From 95737f6e242108e6bae1ae34adddf518adeefc1d Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Thu, 6 Aug 2026 01:58:33 +0000 Subject: [PATCH 3/8] ci(autofix): harden ECS pool hygiene with shared hardened sanitize action Review round for the ECS routing of the heavy autofix jobs. The sanitize step shipped in the previous commit was the trusted-lane variant of the qwen-triage sweep, which two verified bypasses defeat (worktree-scoped config carrying core.hooksPath, and a global core.hooksPath steering the hook deletion). Port the hardened variant into a composite action so all three heavy jobs share one copy, and add it to build-cli, whose checkout runs in the same reused workspace and feeds the bundle every PAT-bearing leg executes. Also completes the persistent-pool hygiene the routing depends on: per-run/per-target WORKDIRs now chmod 700 at creation and are removed by an always() teardown step after the artifact upload (nothing else ever deleted them); the stale-branch sweep detaches HEAD first and globs via BRANCH_PREFIX; the artifact upload reads env.WORKDIR instead of a duplicated literal. The tautological runner-environment gate is repurposed as a fail-fast docker preflight (a missing daemon otherwise surfaces only at 'Resolve sandbox image', after npm ci/build), and Install tmux switches to sudo -n so hosts without passwordless sudo fail with a clear message instead of dying on a prompt. The runs-on comments are rewritten to say exactly what they guard: which events carry the same-repo/write+ clause and which rely on the live write+ or label gates, why review-address is an author-permission gate rather than a head-repository gate, and where the pool's docker availability is proven in-repo (qwen-triage's container jobs on the same labels). Contract tests pin all of it: hygiene steps present and ordered before checkout in every heavy job, the hardened sweep's ordering and containment in the shared action, the docker preflight and tmux hardening, the WORKDIR lifecycle, hosted-only short jobs, and generic next-job slice boundaries so a job inserted after review-address can no longer hide inside its test slice. --- .../sanitize-workspace-git-config/action.yml | 67 ++++++++ .github/workflows/qwen-autofix.yml | 160 ++++++++++++------ scripts/tests/qwen-autofix-workflow.test.js | 152 ++++++++++++++++- 3 files changed, 320 insertions(+), 59 deletions(-) create mode 100644 .github/actions/sanitize-workspace-git-config/action.yml diff --git a/.github/actions/sanitize-workspace-git-config/action.yml b/.github/actions/sanitize-workspace-git-config/action.yml new file mode 100644 index 00000000000..3ba54c7b44f --- /dev/null +++ b/.github/actions/sanitize-workspace-git-config/action.yml @@ -0,0 +1,67 @@ +name: 'Sanitize workspace git config' +description: 'Neutralizes git exec vectors (hooks, fsmonitor, filters, includeIf, ...) a prior job could have planted in a persistent self-hosted workspace, so they cannot fire inside later secret-bearing git steps. Hardened form of the qwen-triage allowlist sweep: also contains the worktree-scoped config and global core.hooksPath bypasses. No-op on a fresh hosted runner. Keep in sync with qwen-triage.yml''s hardened ''Clean stale agent state'' step.' + +runs: + using: 'composite' + steps: + - name: 'Sanitize workspace git config' + shell: 'bash' + run: |- + set -uo pipefail + # `.git` is a directory in a normal checkout but a gitlink file in + # a worktree; -e covers both, and a missing .git (first run) too. + if [ ! -e .git ]; then + echo "no prior workspace; nothing to sanitize" + exit 0 + fi + # Worktree-scoped config FIRST: `extensions.worktreeConfig=true` is + # on the allowlist below (it carries no command itself), but it + # activates `.git/config.worktree` — a second config file that + # `git config --local` neither lists nor unsets, and that CAN carry + # core.hooksPath. Verified in qwen-triage: a prior run can set + # `--worktree core.hooksPath=/`, survive the sweep untouched, and + # make the hooks deletion below walk /. Delete the file outright, + # then drop the extension. + rm -f "$(git rev-parse --git-path config.worktree 2>/dev/null || echo /nonexistent)" 2>/dev/null || true + git config --local --unset-all extensions.worktreeConfig 2>/dev/null || true + # Rather than denylist each exec-vector family (which kept missing + # new ones), KEEP a known-safe allowlist and --unset-all everything + # else: this closes the whole class, including knobs not yet + # enumerated. The kept set is only plumbing that carries no command + # — repo format, remote, branch, fetch/gc/pack/index, safe.directory, + # extensions, and submodule url/active/branch (NOT + # submodule.*.update, which can be `!cmd`). actions/checkout + # re-establishes remote/auth afterward. `|| true` on the grep: no + # non-allowlisted keys (the steady state on an already-sanitized + # runner) means grep exits 1, which would kill the step exactly + # when there is nothing to clean. + git config --local --name-only --list 2>/dev/null \ + | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } \ + | while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done + # Belt and braces after the config scrub: only delete inside the + # repository's own git dir. A hooks path resolving anywhere else is + # unlinked, never swept — a recursive delete of a planted path is + # far worse than a stale hook on a runner the pool re-cleans. + # Resolve hooks with global/system config OUT of the way. Verified + # in qwen-triage: with a global core.hooksPath set, `git rev-parse + # --git-path hooks` returns that path, the guard below sees + # "outside the git dir", and a planted `.git/hooks` symlink + # survives untouched. Keep this resolution AFTER the sweep above. + GIT_DIR_ABS="$(git rev-parse --absolute-git-dir 2>/dev/null || echo '')" + HOOKS_DIR="$(GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" + HOOKS_ABS="$(cd "$HOOKS_DIR" 2>/dev/null && pwd -P || echo '')" + if [ -n "$GIT_DIR_ABS" ] && [ -n "$HOOKS_ABS" ] && [ "${HOOKS_ABS#"$GIT_DIR_ABS"/}" != "$HOOKS_ABS" ]; then + # Match -type f OR -type l: a symlinked hook survives a bare + # `-type f` sweep and still fires on the next checkout. + find "$HOOKS_ABS" \( -type f -o -type l \) ! -name '*.sample' -delete 2>/dev/null || true + else + # Resolves outside the git dir (or not at all). Warning and + # walking away would leave a live hook directory that the next + # git command executes, so unlink the ENTRY without descending + # into it and put an empty hooks directory back. + RAW_HOOKS="$(GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" + echo "::warning::hooks path did not resolve inside the git dir (${HOOKS_ABS:-unresolved}); unlinking it." + rm -f "$RAW_HOOKS" 2>/dev/null || rm -rf "$RAW_HOOKS" 2>/dev/null || true + mkdir -p "${GIT_DIR_ABS:-.git}/hooks" 2>/dev/null || true + git config --local --unset-all core.hooksPath 2>/dev/null || true + fi diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index 82a25898fe1..c40f84d07fb 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -659,11 +659,17 @@ jobs: (github.event_name != 'schedule' || (needs.review-scan.result == 'success' && needs.review-scan.outputs.has_targets != 'true')) }} # Secret-bearing and executes agent-driven code, but the agent runs inside - # the docker sandbox image (which the ECS pool ships) and only ever writes a - # new branch as the dev-bot - it never executes a foreign author's code. - # Forks of this repo (and MAINTAINER_ECS_RUNNER_DISABLED) fall back to - # hosted; PR-family events additionally need a same-repo head or a write+ - # author, mirroring the fleet's ECS routing guard (ci.yml's classify_pr). + # the docker sandbox image and only ever writes a new branch as the + # dev-bot — it never executes a foreign author's code. Forks of this repo + # (and MAINTAINER_ECS_RUNNER_DISABLED) fall back to hosted. On + # pull_request / pull_request_review events the ECS route additionally + # needs a same-repo head or a write+ author (ci.yml's pick_runner form); + # the other triggers skip that clause and rely on their own gates + # instead: issues / schedule require autofix/approved plus + # status/ready-for-agent on the issue, and workflow_dispatch rides the + # actor's own write access. Docker availability on this pool is proven + # in-repo by qwen-triage's container jobs, which run on the same + # runner labels. 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: 180 concurrency: @@ -699,25 +705,12 @@ jobs: # jobs execute human-authored code as the runner user, so a prior job # can plant git exec knobs (core.fsmonitor, filter.*.smudge, # diff.external, includeIf, hooks) in the local config that would fire - # inside THIS job's PAT-bearing git steps. Keep a known-safe allowlist - # and unset everything else — the same pattern qwen-triage uses on - # this pool. No-op on a fresh hosted runner. + # inside THIS job's PAT-bearing git steps. The composite action keeps + # a known-safe allowlist and unsets everything else, hardened against + # the worktree-config and global-hooksPath bypasses verified in + # qwen-triage on this pool. No-op on a fresh hosted runner. - name: 'Sanitize workspace git config' - run: |- - set -uo pipefail - # `.git` is a directory in a normal checkout but a gitlink file in - # a worktree; -e covers both, and a missing .git (first run) too. - if [ ! -e .git ]; then - echo "no prior workspace; nothing to sanitize" - exit 0 - fi - git config --local --name-only --list 2>/dev/null \ - | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } \ - | while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done - HOOKS_DIR="$(git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" - # Match -type f OR -type l: a symlinked hook survives a bare - # `-type f` sweep and still fires on the next checkout. - find "$HOOKS_DIR" \( -type f -o -type l \) ! -name '*.sample' -delete 2>/dev/null || true + uses: './.github/actions/sanitize-workspace-git-config' - name: 'Checkout' uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 @@ -729,6 +722,10 @@ jobs: run: |- rm -rf "${WORKDIR}" mkdir -p "${WORKDIR}" + # 0700: the dir holds agent transcripts and decision files; the + # sandbox container runs as this same user, so the tighter mode + # costs the job nothing. + chmod 700 "${WORKDIR}" # Self-hosted runners keep the workspace's .git across runs, so a # failed earlier attempt's local branch survives here: the agent's @@ -738,7 +735,12 @@ jobs: # actions/checkout's untracked-file clean). - name: 'Drop stale autofix branches' run: |- - git for-each-ref --format='%(refname:short)' 'refs/heads/autofix/issue-*' \ + # Detach first: `git branch -D` refuses the currently checked-out + # branch, so a stale autofix branch holding HEAD would otherwise + # silently survive the sweep (actions/checkout normally leaves + # HEAD on the default branch; this makes the sweep unconditional). + git checkout --detach 2>/dev/null || true + git for-each-ref --format='%(refname:short)' "refs/heads/${BRANCH_PREFIX}*" \ | xargs -r -n 1 git branch -D 2>/dev/null || true # Same staging as the review-address job: the verify gate always runs the @@ -782,6 +784,16 @@ jobs: exit 1 ;; esac + # Capability preflight for the persistent pool: this job's agent + # runs inside the docker sandbox, and a missing daemon otherwise + # surfaces only at 'Resolve sandbox image' — after npm ci/build + # has already burned tens of minutes. Fail in seconds instead. + # Hosted runners ship docker; the ECS pool's docker is proven by + # qwen-triage's container jobs on the same labels. + if ! docker info > /dev/null 2>&1; then + echo "::error::docker daemon is not reachable on this runner; the sandboxed agent cannot start." + exit 1 + fi - name: 'Set up Node.js' uses: 'actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e' # v6.4.0 @@ -795,8 +807,13 @@ jobs: if command -v tmux > /dev/null 2>&1; then tmux -V elif command -v sudo > /dev/null 2>&1 && command -v apt-get > /dev/null 2>&1; then - sudo apt-get update -qq - sudo apt-get install -y -qq tmux + # sudo -n: a host without passwordless sudo must fail fast with a + # clear message, not die on a password prompt (the pr-review pool + # steps make the same assumption with `sudo -n ... || ::warning`). + sudo -n apt-get update -qq && sudo -n apt-get install -y -qq tmux || { + echo '::error::tmux is required on the autofix runner and passwordless install failed.' + exit 1 + } else echo '::error::tmux is required on the autofix runner.' exit 1 @@ -1315,7 +1332,7 @@ jobs: uses: 'actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a' # v7.0.1 with: name: 'autofix-issue-artifacts' - path: '/tmp/autofix-${{ github.run_id }}/' + path: '${{ env.WORKDIR }}/' if-no-files-found: 'ignore' - name: 'Publish PR' @@ -1430,6 +1447,14 @@ jobs: gh api -X DELETE "/repos/${REPO}/issues/comments/${COMMENT_ID}" || true fi + # Nothing else removes the per-run WORKDIR; on the persistent pool + # every run would leave its transcripts and decision files behind + # forever. Last step, after every reader including the artifact + # upload; a cancelled run leaves the dir to the next run's reset. + - name: 'Clean up autofix workdir' + if: 'always()' + run: 'rm -rf "${WORKDIR}"' + # =========================================================================== # TAKEOVER COMMAND — the accepted comment command's side effects are the # label toggle plus its own acknowledgements: add on an unlabeled PR @@ -2682,6 +2707,13 @@ jobs: fi chmod -R u+rwX "$GITHUB_WORKSPACE" 2>/dev/null || sudo -n chmod -R u+rwX "$GITHUB_WORKSPACE" || echo "::warning::could not restore workspace write permissions; checkout may fail on leftover read-only files" + # Same sweep as the agent jobs: this checkout runs in a workspace + # other pool jobs have written, and a surviving filter.*.smudge or + # hook fires during it — in the job that builds the CLI bundle every + # review-address leg then executes with a PAT in env. + - name: 'Sanitize workspace git config' + uses: './.github/actions/sanitize-workspace-git-config' + - name: 'Checkout trusted base' uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 with: @@ -2762,11 +2794,20 @@ jobs: if: |- ${{ needs.review-scan.outputs.has_targets == 'true' }} # Secret-bearing and executes PR code, but every target is live-gated to - # write+ (internal) authors at scan AND address time, and the ECS pool ships - # docker for the sandbox image. Forks of this repo (and - # MAINTAINER_ECS_RUNNER_DISABLED) fall back to hosted; PR-family events - # additionally need a same-repo head or a write+ author, mirroring the - # fleet's ECS routing guard (ci.yml's classify_pr). + # write+ (internal) authors at scan AND address time. That is an + # author-permission gate by design, not a head-repository gate: takeover + # engages maintainer fork PRs, and the pattern matches qwen-code-pr-review, + # whose ECS-routed review job also rides its upstream write+ check. The + # job therefore runs host-side (no `container:`): the branch code it + # executes is collaborator-authored — the same trust class ci.yml's + # pick_runner routes onto this pool — and persistent-workspace residue is + # scrubbed by the hygiene steps below. On pull_request / + # pull_request_review events the ECS route additionally needs a same-repo + # head or a write+ author (ci.yml's pick_runner form); issue_comment and + # the other triggers skip that clause and rely on the live write+ gates. + # Forks of this repo (and MAINTAINER_ECS_RUNNER_DISABLED) fall back to + # hosted. Docker availability on this pool is proven in-repo by + # qwen-triage's container jobs, which run on the same runner labels. 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: 300 permissions: @@ -2870,25 +2911,12 @@ jobs: # jobs execute human-authored code as the runner user, so a prior job # can plant git exec knobs (core.fsmonitor, filter.*.smudge, # diff.external, includeIf, hooks) in the local config that would fire - # inside THIS job's PAT-bearing git steps. Keep a known-safe allowlist - # and unset everything else — the same pattern qwen-triage uses on - # this pool. No-op on a fresh hosted runner. + # inside THIS job's PAT-bearing git steps. The composite action keeps + # a known-safe allowlist and unsets everything else, hardened against + # the worktree-config and global-hooksPath bypasses verified in + # qwen-triage on this pool. No-op on a fresh hosted runner. - name: 'Sanitize workspace git config' - run: |- - set -uo pipefail - # `.git` is a directory in a normal checkout but a gitlink file in - # a worktree; -e covers both, and a missing .git (first run) too. - if [ ! -e .git ]; then - echo "no prior workspace; nothing to sanitize" - exit 0 - fi - git config --local --name-only --list 2>/dev/null \ - | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } \ - | while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done - HOOKS_DIR="$(git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" - # Match -type f OR -type l: a symlinked hook survives a bare - # `-type f` sweep and still fires on the next checkout. - find "$HOOKS_DIR" \( -type f -o -type l \) ! -name '*.sample' -delete 2>/dev/null || true + uses: './.github/actions/sanitize-workspace-git-config' - name: 'Checkout trusted base' uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 @@ -2901,6 +2929,10 @@ jobs: run: |- rm -rf "${WORKDIR}" mkdir -p "${WORKDIR}" + # 0700: the dir holds agent transcripts and decision files; the + # sandbox container runs as this same user, so the tighter mode + # costs the job nothing. + chmod 700 "${WORKDIR}" # Stage the schema gate script from the TRUSTED BASE checkout before # "Prepare branch and feedback" switches the working tree to the PR @@ -2940,6 +2972,16 @@ jobs: exit 1 ;; esac + # Capability preflight for the persistent pool: this job's agent + # runs inside the docker sandbox, and a missing daemon otherwise + # surfaces only at 'Resolve sandbox image' — after npm ci/build + # has already burned tens of minutes. Fail in seconds instead. + # Hosted runners ship docker; the ECS pool's docker is proven by + # qwen-triage's container jobs on the same labels. + if ! docker info > /dev/null 2>&1; then + echo "::error::docker daemon is not reachable on this runner; the sandboxed agent cannot start." + exit 1 + fi - name: 'Set up Node.js' uses: 'actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e' # v6.4.0 @@ -2953,8 +2995,13 @@ jobs: if command -v tmux > /dev/null 2>&1; then tmux -V elif command -v sudo > /dev/null 2>&1 && command -v apt-get > /dev/null 2>&1; then - sudo apt-get update -qq - sudo apt-get install -y -qq tmux + # sudo -n: a host without passwordless sudo must fail fast with a + # clear message, not die on a password prompt (the pr-review pool + # steps make the same assumption with `sudo -n ... || ::warning`). + sudo -n apt-get update -qq && sudo -n apt-get install -y -qq tmux || { + echo '::error::tmux is required on the autofix runner and passwordless install failed.' + exit 1 + } else echo '::error::tmux is required on the autofix runner.' exit 1 @@ -4897,3 +4944,12 @@ jobs: gh api --method PATCH "repos/${REPO}/issues/comments/${STATUS_ID}" \ -f body="${BODY}" > /dev/null || echo "::warning::Failed to finalize the autofix status comment on PR #${PR}; continuing." + + # Nothing else removes the per-target WORKDIR; PR numbers only + # increase, so on the persistent pool every addressed PR would leave + # its transcripts and decision files behind forever. Last step, after + # every reader including the artifact upload; a cancelled run leaves + # the dir to the next run's reset. + - name: 'Clean up autofix workdir' + if: 'always()' + run: 'rm -rf "${WORKDIR}"' diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 21ced8eec80..4f702164d27 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -34,6 +34,9 @@ const reviewVerificationRunner = readFileSync( const autofixContractsScriptPath = '.github/scripts/check-autofix-contracts.sh'; const autofixContractsScript = readFileSync(autofixContractsScriptPath, 'utf8'); const autofixRunnerScriptPath = '.qwen/skills/autofix/scripts/run-agent.mjs'; +const sanitizeActionPath = + '.github/actions/sanitize-workspace-git-config/action.yml'; +const sanitizeAction = readFileSync(sanitizeActionPath, 'utf8'); const checkBotCredentialsStep = workflow.match( /- name: 'Check bot credentials'[\s\S]*?(?=\n[ ]{6}- name: 'Set up Node.js')/, @@ -50,13 +53,17 @@ const reviewScanJob = const issueAutofixJob = workflow.match(/\n {2}issue-autofix:[\s\S]*?(?=\n[ ]{2}# ==========)/)?.[0] ?? ''; +// Both slices bound on the GENERIC next-job shape, not the specific job +// that happens to follow today: inserting a job after review-address must +// shrink these slices instead of silently landing the newcomer's `runs-on` +// inside them. review-address is currently last, so it also allows EOF. const buildCliJob = - workflow.match(/\n {2}build-cli:[\s\S]*?(?=\n {2}review-address:)/)?.[0] ?? + workflow.match(/\n {2}build-cli:[\s\S]*?(?=\n {2}[a-z][a-z0-9-]*:\n)/)?.[0] ?? ''; -// review-address is the last job in the file, so there is no trailing -// `# ====` separator to anchor on — match to EOF. const reviewAddressJob = - workflow.match(/\n {2}review-address:[\s\S]*$/)?.[0] ?? ''; + workflow.match( + /\n {2}review-address:[\s\S]*?(?=\n {2}[a-z][a-z0-9-]*:\n|$)/, + )?.[0] ?? ''; const publishPrStep = workflow.match( /- name: 'Publish PR'[\s\S]*?(?=\n[ ]{6}- name: 'Withdraw claim on failure')/, @@ -5245,6 +5252,40 @@ describe('qwen-autofix workflow', () => { expect(workflow).toContain( "RUNNER_ENVIRONMENT: '${{ runner.environment }}'", ); + // The widened environment gate doubles as the fail-fast capability + // preflight in both agent jobs: their agent runs inside the docker + // sandbox, and without the check a missing daemon surfaces only at + // 'Resolve sandbox image' — after npm ci/build. tmux likewise fails + // fast (and self-installs only via passwordless sudo) before npm ci. + const envCheckSteps = + workflow.match( + /- name: 'Check runner environment'[\s\S]*?(?=\n[ ]{6}- name: ')/g, + ) ?? []; + expect(envCheckSteps).toHaveLength(2); + for (const step of envCheckSteps) { + expect(step).toContain('docker info'); + expect(step).toContain('exit 1'); + } + const installTmuxSteps = + workflow.match(/- name: 'Install tmux'[\s\S]*?(?=\n[ ]{6}- name: ')/g) ?? + []; + expect(installTmuxSteps).toHaveLength(2); + for (const step of installTmuxSteps) { + 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'"); + for (const name of ['takeover-command', 'retry-command', 'takeover-ack']) { + const job = + workflow.match( + new RegExp(`\\n {2}${name}:[\\s\\S]*?(?=\\n {2}[a-z][a-z0-9-]*:\\n)`), + )?.[0] ?? ''; + expect(job, `job slice missing: ${name}`).toBeTruthy(); + expect(job).toContain("runs-on: 'ubuntu-latest'"); + } // issue-autofix, build-cli, and review-address each stage the qwen shim // against the workspace bundle. expect(prepareQwenCliSteps).toHaveLength(3); @@ -5470,10 +5511,12 @@ describe('qwen-autofix workflow', () => { it('clears persistent autofix workdirs before agent steps run', () => { expect(resetAutofixWorkspaceSteps).toHaveLength(2); // Per-run private dir: pool registrations share one /tmp and issue-phase - // runs never serialize against each other. The artifact upload reads the - // same per-run path — pin both sides so neither is left behind. + // runs never serialize against each other. Both artifact uploads read + // ${{ env.WORKDIR }} — one source, nothing to drift. expect(workflow).toContain("WORKDIR: '/tmp/autofix-${{ github.run_id }}'"); - expect(workflow).toContain("path: '/tmp/autofix-${{ github.run_id }}/'"); + expect(workflow.match(/path: '\$\{\{ env\.WORKDIR \}\}\/'/g)).toHaveLength( + 2, + ); expect(workflow).toContain( "WORKDIR: '/tmp/autofix-review-${{ matrix.target.pr }}'", ); @@ -5481,6 +5524,24 @@ describe('qwen-autofix workflow', () => { for (const step of resetAutofixWorkspaceSteps) { expect(step).toContain('rm -rf "${WORKDIR}"'); expect(step).toContain('mkdir -p "${WORKDIR}"'); + // 0700: the dirs hold agent transcripts and decision files. + expect(step).toContain('chmod 700 "${WORKDIR}"'); + } + // Per-run/per-target teardown after the artifact upload: nothing else + // removes these dirs on the persistent pool (PR numbers only increase). + const cleanupSteps = + workflow.match( + /- name: 'Clean up autofix workdir'[\s\S]*?(?=\n[ ]{6}- name: '|\n[ ]{2}# ==========|$)/g, + ) ?? []; + expect(cleanupSteps).toHaveLength(2); + for (const step of cleanupSteps) { + expect(step).toContain("if: 'always()'"); + expect(step).toContain('rm -rf "${WORKDIR}"'); + } + for (const job of [issueAutofixJob, reviewAddressJob]) { + expect(job.indexOf("- name: 'Upload run artifacts'")).toBeLessThan( + job.indexOf("- name: 'Clean up autofix workdir'"), + ); } expect(workflow.indexOf("- name: 'Checkout'")).toBeLessThan( workflow.indexOf("- name: 'Reset autofix workspace'"), @@ -5493,6 +5554,83 @@ describe('qwen-autofix workflow', () => { ).toBeLessThan(workflow.indexOf("- name: 'Prepare branch and feedback'")); }); + it('pins the persistent-pool hygiene steps into every heavy job', () => { + const stepOf = (job, name) => + job.match( + new RegExp(`- name: '${name}'[\\s\\S]*?(?=\\n[ ]{6}- name: |$)`), + )?.[0] ?? ''; + const heavyJobs = [ + ['issue-autofix', issueAutofixJob], + ['build-cli', buildCliJob], + ['review-address', reviewAddressJob], + ]; + for (const [name, job] of heavyJobs) { + expect(job, `job slice missing: ${name}`).toBeTruthy(); + // Ownership restore and config sanitize must BOTH precede the + // checkout they protect: leftover root-owned files break + // actions/checkout, and a planted smudge filter or hook fires during + // the checkout itself. Deleting either step — or reordering it after + // the checkout — must not ship green while the routing assertions + // stay green. + for (const stepName of [ + 'Restore workspace ownership', + 'Sanitize workspace git config', + ]) { + expect(job, `${name} missing '${stepName}'`).toContain( + `- name: '${stepName}'`, + ); + expect( + job.indexOf(`- name: '${stepName}'`), + `${name}: '${stepName}' must precede checkout`, + ).toBeLessThan(job.indexOf("- name: 'Checkout")); + } + expect(job).toContain( + "uses: './.github/actions/sanitize-workspace-git-config'", + ); + } + // The issue phase treats "the branch exists" as proof the agent ran, + // so only it sweeps stale autofix/issue-* branches — detached, so + // `git branch -D` can never refuse the checked-out branch, and via + // BRANCH_PREFIX, so renaming the prefix renames the sweep. + const dropStep = stepOf(issueAutofixJob, 'Drop stale autofix branches'); + expect(dropStep).toContain('git checkout --detach'); + expect(dropStep).toContain('"refs/heads/${BRANCH_PREFIX}*"'); + expect(buildCliJob).not.toContain("- name: 'Drop stale autofix branches'"); + }); + + it('hardens the shared git-config sanitize action against the verified bypasses', () => { + // Worktree-scoped config first: extensions.worktreeConfig activates + // .git/config.worktree, which `git config --local` neither lists nor + // unsets and which CAN carry core.hooksPath — pointing the hook + // sweep's recursive delete at /. Then the allowlist sweep, and only + // then the hooks resolution: the ordering IS the containment. + const rmWorktreeCfg = sanitizeAction.indexOf('--git-path config.worktree'); + const unsetExt = sanitizeAction.indexOf( + '--unset-all extensions.worktreeConfig', + ); + const sweep = sanitizeAction.indexOf('--name-only --list'); + const hooks = sanitizeAction.indexOf('--git-path hooks'); + expect(rmWorktreeCfg).toBeGreaterThan(-1); + expect(unsetExt).toBeGreaterThan(rmWorktreeCfg); + expect(sweep).toBeGreaterThan(unsetExt); + expect(hooks).toBeGreaterThan(sweep); + // Hooks resolve with global/system config out of the way (a planted + // global core.hooksPath must not steer the sweep), deletion stays + // inside the repository's own git dir, and an outward-resolving entry + // is unlinked, never descended into. + expect(sanitizeAction).toContain('GIT_CONFIG_GLOBAL=/dev/null'); + expect(sanitizeAction).toContain('GIT_CONFIG_SYSTEM=/dev/null'); + expect(sanitizeAction).toContain('rev-parse --absolute-git-dir'); + expect(sanitizeAction).toContain('unlinking it'); + expect(sanitizeAction).toContain('-type f -o -type l'); + expect(sanitizeAction).toContain( + 'git config --local --unset-all core.hooksPath', + ); + // Provenance link: the action and qwen-triage's hardened step must be + // edited together. + expect(sanitizeAction).toContain('qwen-triage'); + }); + it('runs qwen headless once in each agent step', () => { const qwenSteps = [ assessCandidatesStep, From 7fe220a2db747e9135a4422ed42c6f2bef56a535 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 6 Aug 2026 11:10:51 +0800 Subject: [PATCH 4/8] ci(autofix): inline git-config sanitize step before checkout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The composite sanitize action was invoked as a local action (`uses: './.github/actions/sanitize-workspace-git-config'`) BEFORE `actions/checkout` in all three ECS-routed jobs. A local action resolves from $GITHUB_WORKSPACE, so: - on any clean workspace it hard-fails with "Can't find 'action.yml'" — every hosted-fallback run (forks, MAINTAINER_ECS_RUNNER_DISABLED=true, i.e. the documented rollback path) and any freshly registered ECS runner — and it cannot self-heal, dying before checkout. - on a reused workspace it executes the leftover copy, which for review-address is the previous run's PR branch — pre-checkout execution of PR-controlled content, the exact vector the step exists to prevent. Inline the hardened sanitize script as a plain `run:` step in each job (the qwen-triage 'Clean stale agent state' pattern) and delete the now-consumer-less composite action. Update the contract test, which had pinned the broken ordering, to pin the inline run step instead, and add a generic guard that no local action may precede a checkout. --- .../sanitize-workspace-git-config/action.yml | 67 ------- .github/workflows/qwen-autofix.yml | 180 +++++++++++++++++- scripts/tests/qwen-autofix-workflow.test.js | 51 +++-- 3 files changed, 213 insertions(+), 85 deletions(-) delete mode 100644 .github/actions/sanitize-workspace-git-config/action.yml diff --git a/.github/actions/sanitize-workspace-git-config/action.yml b/.github/actions/sanitize-workspace-git-config/action.yml deleted file mode 100644 index 3ba54c7b44f..00000000000 --- a/.github/actions/sanitize-workspace-git-config/action.yml +++ /dev/null @@ -1,67 +0,0 @@ -name: 'Sanitize workspace git config' -description: 'Neutralizes git exec vectors (hooks, fsmonitor, filters, includeIf, ...) a prior job could have planted in a persistent self-hosted workspace, so they cannot fire inside later secret-bearing git steps. Hardened form of the qwen-triage allowlist sweep: also contains the worktree-scoped config and global core.hooksPath bypasses. No-op on a fresh hosted runner. Keep in sync with qwen-triage.yml''s hardened ''Clean stale agent state'' step.' - -runs: - using: 'composite' - steps: - - name: 'Sanitize workspace git config' - shell: 'bash' - run: |- - set -uo pipefail - # `.git` is a directory in a normal checkout but a gitlink file in - # a worktree; -e covers both, and a missing .git (first run) too. - if [ ! -e .git ]; then - echo "no prior workspace; nothing to sanitize" - exit 0 - fi - # Worktree-scoped config FIRST: `extensions.worktreeConfig=true` is - # on the allowlist below (it carries no command itself), but it - # activates `.git/config.worktree` — a second config file that - # `git config --local` neither lists nor unsets, and that CAN carry - # core.hooksPath. Verified in qwen-triage: a prior run can set - # `--worktree core.hooksPath=/`, survive the sweep untouched, and - # make the hooks deletion below walk /. Delete the file outright, - # then drop the extension. - rm -f "$(git rev-parse --git-path config.worktree 2>/dev/null || echo /nonexistent)" 2>/dev/null || true - git config --local --unset-all extensions.worktreeConfig 2>/dev/null || true - # Rather than denylist each exec-vector family (which kept missing - # new ones), KEEP a known-safe allowlist and --unset-all everything - # else: this closes the whole class, including knobs not yet - # enumerated. The kept set is only plumbing that carries no command - # — repo format, remote, branch, fetch/gc/pack/index, safe.directory, - # extensions, and submodule url/active/branch (NOT - # submodule.*.update, which can be `!cmd`). actions/checkout - # re-establishes remote/auth afterward. `|| true` on the grep: no - # non-allowlisted keys (the steady state on an already-sanitized - # runner) means grep exits 1, which would kill the step exactly - # when there is nothing to clean. - git config --local --name-only --list 2>/dev/null \ - | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } \ - | while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done - # Belt and braces after the config scrub: only delete inside the - # repository's own git dir. A hooks path resolving anywhere else is - # unlinked, never swept — a recursive delete of a planted path is - # far worse than a stale hook on a runner the pool re-cleans. - # Resolve hooks with global/system config OUT of the way. Verified - # in qwen-triage: with a global core.hooksPath set, `git rev-parse - # --git-path hooks` returns that path, the guard below sees - # "outside the git dir", and a planted `.git/hooks` symlink - # survives untouched. Keep this resolution AFTER the sweep above. - GIT_DIR_ABS="$(git rev-parse --absolute-git-dir 2>/dev/null || echo '')" - HOOKS_DIR="$(GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" - HOOKS_ABS="$(cd "$HOOKS_DIR" 2>/dev/null && pwd -P || echo '')" - if [ -n "$GIT_DIR_ABS" ] && [ -n "$HOOKS_ABS" ] && [ "${HOOKS_ABS#"$GIT_DIR_ABS"/}" != "$HOOKS_ABS" ]; then - # Match -type f OR -type l: a symlinked hook survives a bare - # `-type f` sweep and still fires on the next checkout. - find "$HOOKS_ABS" \( -type f -o -type l \) ! -name '*.sample' -delete 2>/dev/null || true - else - # Resolves outside the git dir (or not at all). Warning and - # walking away would leave a live hook directory that the next - # git command executes, so unlink the ENTRY without descending - # into it and put an empty hooks directory back. - RAW_HOOKS="$(GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" - echo "::warning::hooks path did not resolve inside the git dir (${HOOKS_ABS:-unresolved}); unlinking it." - rm -f "$RAW_HOOKS" 2>/dev/null || rm -rf "$RAW_HOOKS" 2>/dev/null || true - mkdir -p "${GIT_DIR_ABS:-.git}/hooks" 2>/dev/null || true - git config --local --unset-all core.hooksPath 2>/dev/null || true - fi diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index a7cdb901af2..8ece0b3944c 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -710,7 +710,65 @@ jobs: # the worktree-config and global-hooksPath bypasses verified in # qwen-triage on this pool. No-op on a fresh hosted runner. - name: 'Sanitize workspace git config' - uses: './.github/actions/sanitize-workspace-git-config' + run: |- + set -uo pipefail + # `.git` is a directory in a normal checkout but a gitlink file in + # a worktree; -e covers both, and a missing .git (first run) too. + if [ ! -e .git ]; then + echo "no prior workspace; nothing to sanitize" + exit 0 + fi + # Worktree-scoped config FIRST: `extensions.worktreeConfig=true` is + # on the allowlist below (it carries no command itself), but it + # activates `.git/config.worktree` — a second config file that + # `git config --local` neither lists nor unsets, and that CAN carry + # core.hooksPath. Verified in qwen-triage: a prior run can set + # `--worktree core.hooksPath=/`, survive the sweep untouched, and + # make the hooks deletion below walk /. Delete the file outright, + # then drop the extension. + rm -f "$(git rev-parse --git-path config.worktree 2>/dev/null || echo /nonexistent)" 2>/dev/null || true + git config --local --unset-all extensions.worktreeConfig 2>/dev/null || true + # Rather than denylist each exec-vector family (which kept missing + # new ones), KEEP a known-safe allowlist and --unset-all everything + # else: this closes the whole class, including knobs not yet + # enumerated. The kept set is only plumbing that carries no command + # — repo format, remote, branch, fetch/gc/pack/index, safe.directory, + # extensions, and submodule url/active/branch (NOT + # submodule.*.update, which can be `!cmd`). actions/checkout + # re-establishes remote/auth afterward. `|| true` on the grep: no + # non-allowlisted keys (the steady state on an already-sanitized + # runner) means grep exits 1, which would kill the step exactly + # when there is nothing to clean. + git config --local --name-only --list 2>/dev/null \ + | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } \ + | while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done + # Belt and braces after the config scrub: only delete inside the + # repository's own git dir. A hooks path resolving anywhere else is + # unlinked, never swept — a recursive delete of a planted path is + # far worse than a stale hook on a runner the pool re-cleans. + # Resolve hooks with global/system config OUT of the way. Verified + # in qwen-triage: with a global core.hooksPath set, `git rev-parse + # --git-path hooks` returns that path, the guard below sees + # "outside the git dir", and a planted `.git/hooks` symlink + # survives untouched. Keep this resolution AFTER the sweep above. + GIT_DIR_ABS="$(git rev-parse --absolute-git-dir 2>/dev/null || echo '')" + HOOKS_DIR="$(GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" + HOOKS_ABS="$(cd "$HOOKS_DIR" 2>/dev/null && pwd -P || echo '')" + if [ -n "$GIT_DIR_ABS" ] && [ -n "$HOOKS_ABS" ] && [ "${HOOKS_ABS#"$GIT_DIR_ABS"/}" != "$HOOKS_ABS" ]; then + # Match -type f OR -type l: a symlinked hook survives a bare + # `-type f` sweep and still fires on the next checkout. + find "$HOOKS_ABS" \( -type f -o -type l \) ! -name '*.sample' -delete 2>/dev/null || true + else + # Resolves outside the git dir (or not at all). Warning and + # walking away would leave a live hook directory that the next + # git command executes, so unlink the ENTRY without descending + # into it and put an empty hooks directory back. + RAW_HOOKS="$(GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" + echo "::warning::hooks path did not resolve inside the git dir (${HOOKS_ABS:-unresolved}); unlinking it." + rm -f "$RAW_HOOKS" 2>/dev/null || rm -rf "$RAW_HOOKS" 2>/dev/null || true + mkdir -p "${GIT_DIR_ABS:-.git}/hooks" 2>/dev/null || true + git config --local --unset-all core.hooksPath 2>/dev/null || true + fi - name: 'Checkout' uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 @@ -2712,7 +2770,65 @@ jobs: # hook fires during it — in the job that builds the CLI bundle every # review-address leg then executes with a PAT in env. - name: 'Sanitize workspace git config' - uses: './.github/actions/sanitize-workspace-git-config' + run: |- + set -uo pipefail + # `.git` is a directory in a normal checkout but a gitlink file in + # a worktree; -e covers both, and a missing .git (first run) too. + if [ ! -e .git ]; then + echo "no prior workspace; nothing to sanitize" + exit 0 + fi + # Worktree-scoped config FIRST: `extensions.worktreeConfig=true` is + # on the allowlist below (it carries no command itself), but it + # activates `.git/config.worktree` — a second config file that + # `git config --local` neither lists nor unsets, and that CAN carry + # core.hooksPath. Verified in qwen-triage: a prior run can set + # `--worktree core.hooksPath=/`, survive the sweep untouched, and + # make the hooks deletion below walk /. Delete the file outright, + # then drop the extension. + rm -f "$(git rev-parse --git-path config.worktree 2>/dev/null || echo /nonexistent)" 2>/dev/null || true + git config --local --unset-all extensions.worktreeConfig 2>/dev/null || true + # Rather than denylist each exec-vector family (which kept missing + # new ones), KEEP a known-safe allowlist and --unset-all everything + # else: this closes the whole class, including knobs not yet + # enumerated. The kept set is only plumbing that carries no command + # — repo format, remote, branch, fetch/gc/pack/index, safe.directory, + # extensions, and submodule url/active/branch (NOT + # submodule.*.update, which can be `!cmd`). actions/checkout + # re-establishes remote/auth afterward. `|| true` on the grep: no + # non-allowlisted keys (the steady state on an already-sanitized + # runner) means grep exits 1, which would kill the step exactly + # when there is nothing to clean. + git config --local --name-only --list 2>/dev/null \ + | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } \ + | while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done + # Belt and braces after the config scrub: only delete inside the + # repository's own git dir. A hooks path resolving anywhere else is + # unlinked, never swept — a recursive delete of a planted path is + # far worse than a stale hook on a runner the pool re-cleans. + # Resolve hooks with global/system config OUT of the way. Verified + # in qwen-triage: with a global core.hooksPath set, `git rev-parse + # --git-path hooks` returns that path, the guard below sees + # "outside the git dir", and a planted `.git/hooks` symlink + # survives untouched. Keep this resolution AFTER the sweep above. + GIT_DIR_ABS="$(git rev-parse --absolute-git-dir 2>/dev/null || echo '')" + HOOKS_DIR="$(GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" + HOOKS_ABS="$(cd "$HOOKS_DIR" 2>/dev/null && pwd -P || echo '')" + if [ -n "$GIT_DIR_ABS" ] && [ -n "$HOOKS_ABS" ] && [ "${HOOKS_ABS#"$GIT_DIR_ABS"/}" != "$HOOKS_ABS" ]; then + # Match -type f OR -type l: a symlinked hook survives a bare + # `-type f` sweep and still fires on the next checkout. + find "$HOOKS_ABS" \( -type f -o -type l \) ! -name '*.sample' -delete 2>/dev/null || true + else + # Resolves outside the git dir (or not at all). Warning and + # walking away would leave a live hook directory that the next + # git command executes, so unlink the ENTRY without descending + # into it and put an empty hooks directory back. + RAW_HOOKS="$(GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" + echo "::warning::hooks path did not resolve inside the git dir (${HOOKS_ABS:-unresolved}); unlinking it." + rm -f "$RAW_HOOKS" 2>/dev/null || rm -rf "$RAW_HOOKS" 2>/dev/null || true + mkdir -p "${GIT_DIR_ABS:-.git}/hooks" 2>/dev/null || true + git config --local --unset-all core.hooksPath 2>/dev/null || true + fi - name: 'Checkout trusted base' uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 @@ -2926,7 +3042,65 @@ jobs: # the worktree-config and global-hooksPath bypasses verified in # qwen-triage on this pool. No-op on a fresh hosted runner. - name: 'Sanitize workspace git config' - uses: './.github/actions/sanitize-workspace-git-config' + run: |- + set -uo pipefail + # `.git` is a directory in a normal checkout but a gitlink file in + # a worktree; -e covers both, and a missing .git (first run) too. + if [ ! -e .git ]; then + echo "no prior workspace; nothing to sanitize" + exit 0 + fi + # Worktree-scoped config FIRST: `extensions.worktreeConfig=true` is + # on the allowlist below (it carries no command itself), but it + # activates `.git/config.worktree` — a second config file that + # `git config --local` neither lists nor unsets, and that CAN carry + # core.hooksPath. Verified in qwen-triage: a prior run can set + # `--worktree core.hooksPath=/`, survive the sweep untouched, and + # make the hooks deletion below walk /. Delete the file outright, + # then drop the extension. + rm -f "$(git rev-parse --git-path config.worktree 2>/dev/null || echo /nonexistent)" 2>/dev/null || true + git config --local --unset-all extensions.worktreeConfig 2>/dev/null || true + # Rather than denylist each exec-vector family (which kept missing + # new ones), KEEP a known-safe allowlist and --unset-all everything + # else: this closes the whole class, including knobs not yet + # enumerated. The kept set is only plumbing that carries no command + # — repo format, remote, branch, fetch/gc/pack/index, safe.directory, + # extensions, and submodule url/active/branch (NOT + # submodule.*.update, which can be `!cmd`). actions/checkout + # re-establishes remote/auth afterward. `|| true` on the grep: no + # non-allowlisted keys (the steady state on an already-sanitized + # runner) means grep exits 1, which would kill the step exactly + # when there is nothing to clean. + git config --local --name-only --list 2>/dev/null \ + | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } \ + | while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done + # Belt and braces after the config scrub: only delete inside the + # repository's own git dir. A hooks path resolving anywhere else is + # unlinked, never swept — a recursive delete of a planted path is + # far worse than a stale hook on a runner the pool re-cleans. + # Resolve hooks with global/system config OUT of the way. Verified + # in qwen-triage: with a global core.hooksPath set, `git rev-parse + # --git-path hooks` returns that path, the guard below sees + # "outside the git dir", and a planted `.git/hooks` symlink + # survives untouched. Keep this resolution AFTER the sweep above. + GIT_DIR_ABS="$(git rev-parse --absolute-git-dir 2>/dev/null || echo '')" + HOOKS_DIR="$(GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" + HOOKS_ABS="$(cd "$HOOKS_DIR" 2>/dev/null && pwd -P || echo '')" + if [ -n "$GIT_DIR_ABS" ] && [ -n "$HOOKS_ABS" ] && [ "${HOOKS_ABS#"$GIT_DIR_ABS"/}" != "$HOOKS_ABS" ]; then + # Match -type f OR -type l: a symlinked hook survives a bare + # `-type f` sweep and still fires on the next checkout. + find "$HOOKS_ABS" \( -type f -o -type l \) ! -name '*.sample' -delete 2>/dev/null || true + else + # Resolves outside the git dir (or not at all). Warning and + # walking away would leave a live hook directory that the next + # git command executes, so unlink the ENTRY without descending + # into it and put an empty hooks directory back. + RAW_HOOKS="$(GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" + echo "::warning::hooks path did not resolve inside the git dir (${HOOKS_ABS:-unresolved}); unlinking it." + rm -f "$RAW_HOOKS" 2>/dev/null || rm -rf "$RAW_HOOKS" 2>/dev/null || true + mkdir -p "${GIT_DIR_ABS:-.git}/hooks" 2>/dev/null || true + git config --local --unset-all core.hooksPath 2>/dev/null || true + fi - name: 'Checkout trusted base' uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index d1115fa1a00..9c747d05ca2 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -34,9 +34,6 @@ const reviewVerificationRunner = readFileSync( const autofixContractsScriptPath = '.github/scripts/check-autofix-contracts.sh'; const autofixContractsScript = readFileSync(autofixContractsScriptPath, 'utf8'); const autofixRunnerScriptPath = '.qwen/skills/autofix/scripts/run-agent.mjs'; -const sanitizeActionPath = - '.github/actions/sanitize-workspace-git-config/action.yml'; -const sanitizeAction = readFileSync(sanitizeActionPath, 'utf8'); const checkBotCredentialsStep = workflow.match( /- name: 'Check bot credentials'[\s\S]*?(?=\n[ ]{6}- name: 'Set up Node.js')/, @@ -64,6 +61,13 @@ const reviewAddressJob = workflow.match( /\n {2}review-address:[\s\S]*?(?=\n {2}[a-z][a-z0-9-]*:\n|$)/, )?.[0] ?? ''; +// The sanitize step is inlined into each heavy job as a `run:` step (a +// local action would need a checkout it is meant to precede). issue-autofix's +// copy is the canonical text for the ordering and hardening assertions below. +const sanitizeStep = + issueAutofixJob.match( + /- name: 'Sanitize workspace git config'[\s\S]*?(?=\n[ ]{6}- name: ')/, + )?.[0] ?? ''; const publishPrStep = workflow.match( /- name: 'Publish PR'[\s\S]*?(?=\n[ ]{6}- name: 'Withdraw claim on failure')/, @@ -5597,9 +5601,13 @@ describe('qwen-autofix workflow', () => { `${name}: '${stepName}' must precede checkout`, ).toBeLessThan(job.indexOf("- name: 'Checkout")); } + // Inlined as a run step, not a local action: a `uses: './...'` + // before checkout fails on a clean runner and executes leftover + // content on a reused one. expect(job).toContain( - "uses: './.github/actions/sanitize-workspace-git-config'", + "- name: 'Sanitize workspace git config'\n run: |-", ); + expect(job).not.toContain("uses: './"); } // The issue phase treats "the branch exists" as proof the agent ran, // so only it sweeps stale autofix/issue-* branches — detached, so @@ -5617,12 +5625,12 @@ describe('qwen-autofix workflow', () => { // unsets and which CAN carry core.hooksPath — pointing the hook // sweep's recursive delete at /. Then the allowlist sweep, and only // then the hooks resolution: the ordering IS the containment. - const rmWorktreeCfg = sanitizeAction.indexOf('--git-path config.worktree'); - const unsetExt = sanitizeAction.indexOf( + const rmWorktreeCfg = sanitizeStep.indexOf('--git-path config.worktree'); + const unsetExt = sanitizeStep.indexOf( '--unset-all extensions.worktreeConfig', ); - const sweep = sanitizeAction.indexOf('--name-only --list'); - const hooks = sanitizeAction.indexOf('--git-path hooks'); + const sweep = sanitizeStep.indexOf('--name-only --list'); + const hooks = sanitizeStep.indexOf('--git-path hooks'); expect(rmWorktreeCfg).toBeGreaterThan(-1); expect(unsetExt).toBeGreaterThan(rmWorktreeCfg); expect(sweep).toBeGreaterThan(unsetExt); @@ -5631,17 +5639,30 @@ describe('qwen-autofix workflow', () => { // global core.hooksPath must not steer the sweep), deletion stays // inside the repository's own git dir, and an outward-resolving entry // is unlinked, never descended into. - expect(sanitizeAction).toContain('GIT_CONFIG_GLOBAL=/dev/null'); - expect(sanitizeAction).toContain('GIT_CONFIG_SYSTEM=/dev/null'); - expect(sanitizeAction).toContain('rev-parse --absolute-git-dir'); - expect(sanitizeAction).toContain('unlinking it'); - expect(sanitizeAction).toContain('-type f -o -type l'); - expect(sanitizeAction).toContain( + expect(sanitizeStep).toContain('GIT_CONFIG_GLOBAL=/dev/null'); + expect(sanitizeStep).toContain('GIT_CONFIG_SYSTEM=/dev/null'); + expect(sanitizeStep).toContain('rev-parse --absolute-git-dir'); + expect(sanitizeStep).toContain('unlinking it'); + expect(sanitizeStep).toContain('-type f -o -type l'); + expect(sanitizeStep).toContain( 'git config --local --unset-all core.hooksPath', ); // Provenance link: the action and qwen-triage's hardened step must be // edited together. - expect(sanitizeAction).toContain('qwen-triage'); + expect(sanitizeStep).toContain('qwen-triage'); + }); + + it('never invokes a local action before checkout', () => { + // A `uses: './...'` local action resolves from $GITHUB_WORKSPACE, so + // it only exists after a checkout — before one it fails on a clean + // runner and executes a leftover copy on a reused one. + for (const block of workflow.split(/\n {2}# ={6,}/)) { + const localUse = block.indexOf("uses: './"); + if (localUse === -1) continue; + const checkout = block.indexOf("uses: 'actions/checkout"); + expect(checkout).toBeGreaterThan(-1); + expect(localUse).toBeGreaterThan(checkout); + } }); it('runs qwen headless once in each agent step', () => { From a0e88fc0362d008d244f8c0c657efeec28fc7069 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 6 Aug 2026 12:01:02 +0800 Subject: [PATCH 5/8] ci(autofix): harden PAT handling and pool isolation on the shared pool Address the Medium findings from the ECS-routing review: - PAT pushes (Publish PR, Push and report, and the fork dry-run preflight) now authenticate via a transient credential helper instead of a token-bearing remote URL. The token enters neither the reused workspace's .git/config nor the process argv, which is world-readable via /proc on this shared host; the helper config holds only a ${GITHUB_TOKEN} reference and is swept by the next sanitize pass. - 'Check runner environment' now asserts the self-hosted runner is an ecs-qwen pool member by name, so a mis-labelled registration cannot silently claim a PAT-bearing 300-minute job. - 'Reset autofix workspace' age-sweeps abandoned /tmp/autofix-* dirs (a hard runner kill skips the always() teardown and run_id never repeats) and prunes the reused workspace's .git. The contract test is updated to pin the tokenless push/fetch URLs, the credential-helper authentication, and the absence of a token-bearing URL. --- .github/workflows/qwen-autofix.yml | 67 ++++++++++++++++++--- scripts/tests/qwen-autofix-workflow.test.js | 20 +++--- 2 files changed, 72 insertions(+), 15 deletions(-) diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index 8ece0b3944c..8e7b3dbe7b7 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -784,6 +784,13 @@ jobs: # sandbox container runs as this same user, so the tighter mode # costs the job nothing. chmod 700 "${WORKDIR}" + # Age-sweep abandoned run-scoped dirs on this shared /tmp: a hard + # runner kill skips the always() teardown and run_id never repeats, + # so nothing else ever reclaims them. + find /tmp -maxdepth 1 -name 'autofix-*' -mmin +1440 -exec rm -rf {} + 2>/dev/null || true + # The reused workspace's .git accumulates unreferenced objects + # across fetch runs on this persistent pool; prune them. + git gc --prune=now --quiet 2>/dev/null || true # Self-hosted runners keep the workspace's .git across runs, so a # failed earlier attempt's local branch survives here: the agent's @@ -834,6 +841,7 @@ jobs: - name: 'Check runner environment' env: RUNNER_ENVIRONMENT: '${{ runner.environment }}' + RUNNER_NAME: '${{ runner.name }}' run: |- case "${RUNNER_ENVIRONMENT}" in github-hosted|self-hosted) ;; @@ -842,6 +850,18 @@ jobs: exit 1 ;; esac + # The label routing pins ecs-qwen, but a mis-labelled registration + # must not silently claim a PAT-bearing 300-minute job — assert the + # pool by name on the self-hosted branch too. + if [[ "${RUNNER_ENVIRONMENT}" == 'self-hosted' ]]; then + case "${RUNNER_NAME}" in + ecs-qwen-*) ;; + *) + echo "::error::self-hosted runner '${RUNNER_NAME}' is not an ecs-qwen pool member; refusing to run here." + exit 1 + ;; + esac + fi # Capability preflight for the persistent pool: this job's agent # runs inside the docker sandbox, and a missing daemon otherwise # surfaces only at 'Resolve sandbox image' — after npm ci/build @@ -1428,11 +1448,15 @@ jobs: BRANCH="autofix/issue-${ISSUE}" git config --local --unset-all http.https://github.com/.extraheader || true git config core.hooksPath /dev/null - # Authenticate the push transiently, never writing the PAT into - # the origin remote: the pool keeps this workspace between runs, - # so a token persisted to .git/config would stay readable on the - # shared host until a later job wiped it. - git push --no-verify "https://x-access-token:${GITHUB_TOKEN}@github.com/${REPO}.git" "${BRANCH}" + # Authenticate the push via a transient credential helper: the + # PAT enters neither the remote URL (persisted to .git/config on + # this reused workspace) nor the process argv (/proc//cmdline + # is world-readable on this shared host). The config holds only a + # ${GITHUB_TOKEN} reference, and credential.* is off the sanitize + # allowlist, so a leftover helper is swept next run. + git config --local credential.helper '!f(){ echo username=x-access-token; echo "password=${GITHUB_TOKEN}"; };f' + git push --no-verify "https://github.com/${REPO}.git" "${BRANCH}" + git config --local --unset-all credential.helper || true PR_URL="$(gh pr create --repo "${REPO}" \ --base main --head "${BRANCH}" \ @@ -3117,6 +3141,13 @@ jobs: # sandbox container runs as this same user, so the tighter mode # costs the job nothing. chmod 700 "${WORKDIR}" + # Age-sweep abandoned run-scoped dirs on this shared /tmp: a hard + # runner kill skips the always() teardown and run_id never repeats, + # so nothing else ever reclaims them. + find /tmp -maxdepth 1 -name 'autofix-*' -mmin +1440 -exec rm -rf {} + 2>/dev/null || true + # The reused workspace's .git accumulates unreferenced objects + # across fetch runs on this persistent pool; prune them. + git gc --prune=now --quiet 2>/dev/null || true # Stage the schema gate script from the TRUSTED BASE checkout before # "Prepare branch and feedback" switches the working tree to the PR @@ -3148,6 +3179,7 @@ jobs: - name: 'Check runner environment' env: RUNNER_ENVIRONMENT: '${{ runner.environment }}' + RUNNER_NAME: '${{ runner.name }}' run: |- case "${RUNNER_ENVIRONMENT}" in github-hosted|self-hosted) ;; @@ -3156,6 +3188,18 @@ jobs: exit 1 ;; esac + # The label routing pins ecs-qwen, but a mis-labelled registration + # must not silently claim a PAT-bearing 300-minute job — assert the + # pool by name on the self-hosted branch too. + if [[ "${RUNNER_ENVIRONMENT}" == 'self-hosted' ]]; then + case "${RUNNER_NAME}" in + ecs-qwen-*) ;; + *) + echo "::error::self-hosted runner '${RUNNER_NAME}' is not an ecs-qwen pool member; refusing to run here." + exit 1 + ;; + esac + fi # Capability preflight for the persistent pool: this job's agent # runs inside the docker sandbox, and a missing daemon otherwise # surfaces only at 'Resolve sandbox image' — after npm ci/build @@ -3354,7 +3398,8 @@ jobs: # and fine-grained PATs are documented as NOT receiving it. # Prove push access NOW, before an agent round is spent, instead # of 403ing at the report step after the work is done. - if ! git push --no-verify --dry-run "https://x-access-token:${GITHUB_TOKEN}@github.com/${HEAD_REPO}.git" HEAD:"${BRANCH}" > /dev/null 2>&1; then + git config --local credential.helper '!f(){ echo username=x-access-token; echo "password=${GITHUB_TOKEN}"; };f' + if ! git push --no-verify --dry-run "https://github.com/${HEAD_REPO}.git" HEAD:"${BRANCH}" > /dev/null 2>&1; then echo "🫥 fork push preflight failed for ${HEAD_REPO} (allow-edits grant or PAT type) — discarding without action or marker" { echo "stale=true" @@ -3364,6 +3409,7 @@ jobs: } >> "${GITHUB_OUTPUT}" exit 0 fi + git config --local --unset-all credential.helper || true else git checkout -B "${BRANCH}" "origin/${BRANCH}" fi @@ -4253,13 +4299,17 @@ jobs: # commits get checked). A pre-push hook would execute that code # with the PAT in env — sever hooks entirely before pushing. git config core.hooksPath /dev/null + # Authenticate via a transient credential helper instead of a + # token-bearing URL: the PAT enters neither .git/config nor the + # process argv on this shared host (see Publish PR). + git config --local credential.helper '!f(){ echo username=x-access-token; echo "password=${GITHUB_TOKEN}"; };f' if [[ "${HEAD_REPO:-${REPO}}" != "${REPO}" ]]; then # Push back to the FORK branch via allow-edits (PAT has push # rights on the upstream, which GitHub extends to the fork's # PR branch when the author ticked the box). - PUSH_URL="https://x-access-token:${GITHUB_TOKEN}@github.com/${HEAD_REPO}.git" + PUSH_URL="https://github.com/${HEAD_REPO}.git" else - PUSH_URL="https://x-access-token:${GITHUB_TOKEN}@github.com/${REPO}.git" + PUSH_URL="https://github.com/${REPO}.git" fi # Salvage a race-lost push instead of discarding the run. The # per-PR head-write concurrency group serialises THIS repo's @@ -4307,6 +4357,7 @@ jobs: PUSH_RACE_MERGED='true' fi done + git config --local --unset-all credential.helper || true CAN_RESOLVE_THREADS='false' if [[ -s "${WORKDIR}/resolved-comments.txt" ]]; then LOCAL_PUSHED_HEAD="$(git rev-parse HEAD)" diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 9c747d05ca2..4a833e4dddc 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -2224,7 +2224,7 @@ describe('qwen-autofix workflow', () => { 'git fetch "https://github.com/${HEAD_REPO}.git" "refs/heads/${BRANCH}"', ); expect(workflow).toContain( - 'PUSH_URL="https://x-access-token:${GITHUB_TOKEN}@github.com/${HEAD_REPO}.git"', + 'PUSH_URL="https://github.com/${HEAD_REPO}.git"', ); expect(workflow).toContain( 'git push --no-verify "${PUSH_URL}" HEAD:"${BRANCH}"', @@ -2233,7 +2233,7 @@ describe('qwen-autofix workflow', () => { // prove push access BEFORE an agent round is spent, discarding // gracefully instead of 403ing at the report step. expect(workflow).toContain( - 'git push --no-verify --dry-run "https://x-access-token:${GITHUB_TOKEN}@github.com/${HEAD_REPO}.git" HEAD:"${BRANCH}"', + 'git push --no-verify --dry-run "https://github.com/${HEAD_REPO}.git" HEAD:"${BRANCH}"', ); expect(workflow).toContain('fork push preflight failed'); // First-pickup engage ack anchors the window when the label path could @@ -6472,7 +6472,7 @@ describe('qwen-autofix workflow', () => { // for ${HEAD_REPO} (empty in the same-repo case → a malformed // `github.com/.git` remote) must not survive. expect(pushAndReportStep).toContain( - 'PUSH_URL="https://x-access-token:${GITHUB_TOKEN}@github.com/${REPO}.git"', + 'PUSH_URL="https://github.com/${REPO}.git"', ); expect(pushAndReportStep).toContain( 'git fetch "${PUSH_URL}" "refs/heads/${BRANCH}"', @@ -6549,13 +6549,19 @@ describe('qwen-autofix workflow', () => { expect(workflow).not.toMatch(/\bgit push\b[^\n]* -[a-zA-Z]*f\b/); expect(workflow).not.toMatch(/\bgit push\b[^\n]* \+\S/); expect(publishPrStep).toContain( - 'git push --no-verify "https://x-access-token:${GITHUB_TOKEN}@github.com/${REPO}.git" "${BRANCH}"', + 'git push --no-verify "https://github.com/${REPO}.git" "${BRANCH}"', ); - // Neither PAT push may persist the token into the reused workspace's - // .git/config — a `git remote set-url` carrying the PAT survives the job - // on the shared pool. Both authenticate transiently (URL on the command). + // Neither PAT push may expose the token — not persisted to .git/config + // (a `git remote set-url`) and not in the process argv (a token-bearing + // URL on the command line, world-readable via /proc on this shared + // host). Both authenticate via a transient credential helper instead, so + // the push/fetch URLs are tokenless. expect(publishPrStep).not.toContain('git remote set-url'); expect(pushAndReportStep).not.toContain('git remote set-url'); + expect(publishPrStep).not.toContain('x-access-token:${GITHUB_TOKEN}@'); + expect(pushAndReportStep).not.toContain('x-access-token:${GITHUB_TOKEN}@'); + expect(publishPrStep).toContain('credential.helper'); + expect(pushAndReportStep).toContain('credential.helper'); expect(pushAndReportStep).toContain( 'git push --no-verify "${PUSH_URL}" HEAD:"${BRANCH}"', ); From 17ddaf6af39c87e5bb44020cdb1a963e3455b654 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 6 Aug 2026 04:39:04 +0000 Subject: [PATCH 6/8] ci(autofix): create autofix workdirs with umask 077 and correct cleanup comments --- .github/workflows/qwen-autofix.yml | 22 ++++++++++++--------- scripts/tests/qwen-autofix-workflow.test.js | 6 ++++-- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index 8e7b3dbe7b7..e0f721bc421 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -779,11 +779,11 @@ jobs: - name: 'Reset autofix workspace' run: |- rm -rf "${WORKDIR}" - mkdir -p "${WORKDIR}" # 0700: the dir holds agent transcripts and decision files; the # sandbox container runs as this same user, so the tighter mode - # costs the job nothing. - chmod 700 "${WORKDIR}" + # costs the job nothing. umask at creation, not mkdir-then-chmod — + # the chmod form leaves a world-readable window on this shared /tmp. + (umask 077; mkdir -p "${WORKDIR}") # Age-sweep abandoned run-scoped dirs on this shared /tmp: a hard # runner kill skips the always() teardown and run_id never repeats, # so nothing else ever reclaims them. @@ -1532,7 +1532,9 @@ jobs: # Nothing else removes the per-run WORKDIR; on the persistent pool # every run would leave its transcripts and decision files behind # forever. Last step, after every reader including the artifact - # upload; a cancelled run leaves the dir to the next run's reset. + # upload. always() covers cancellation too; only a hard runner kill + # abandons the dir, and run_id never repeats — the age sweep in + # Reset autofix workspace reclaims it. - name: 'Clean up autofix workdir' if: 'always()' run: 'rm -rf "${WORKDIR}"' @@ -3136,11 +3138,11 @@ jobs: - name: 'Reset autofix workspace' run: |- rm -rf "${WORKDIR}" - mkdir -p "${WORKDIR}" # 0700: the dir holds agent transcripts and decision files; the # sandbox container runs as this same user, so the tighter mode - # costs the job nothing. - chmod 700 "${WORKDIR}" + # costs the job nothing. umask at creation, not mkdir-then-chmod — + # the chmod form leaves a world-readable window on this shared /tmp. + (umask 077; mkdir -p "${WORKDIR}") # Age-sweep abandoned run-scoped dirs on this shared /tmp: a hard # runner kill skips the always() teardown and run_id never repeats, # so nothing else ever reclaims them. @@ -5190,8 +5192,10 @@ jobs: # Nothing else removes the per-target WORKDIR; PR numbers only # increase, so on the persistent pool every addressed PR would leave # its transcripts and decision files behind forever. Last step, after - # every reader including the artifact upload; a cancelled run leaves - # the dir to the next run's reset. + # every reader including the artifact upload. always() covers + # cancellation too; a dir abandoned by a hard runner kill is reclaimed + # by the next same-PR run's reset, or by the age sweep in Reset + # autofix workspace if that PR is never addressed again. - name: 'Clean up autofix workdir' if: 'always()' run: 'rm -rf "${WORKDIR}"' diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 4a833e4dddc..abdd3db0cab 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -5541,8 +5541,10 @@ describe('qwen-autofix workflow', () => { for (const step of resetAutofixWorkspaceSteps) { expect(step).toContain('rm -rf "${WORKDIR}"'); expect(step).toContain('mkdir -p "${WORKDIR}"'); - // 0700: the dirs hold agent transcripts and decision files. - expect(step).toContain('chmod 700 "${WORKDIR}"'); + // 0700 at creation via umask — mkdir-then-chmod leaves a + // world-readable window on the shared /tmp. + expect(step).toContain('(umask 077; mkdir -p "${WORKDIR}")'); + expect(step).not.toContain('chmod 700'); } // Per-run/per-target teardown after the artifact upload: nothing else // removes these dirs on the persistent pool (PR numbers only increase). From 243cde46095a9ca8b1632bb17fdea480a4cc73fc Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 6 Aug 2026 12:43:31 +0800 Subject: [PATCH 7/8] ci(autofix): scope PAT credential helper to a one-shot git -c, plus review fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the latest review round on the ECS-routed heavy jobs. High — the transient credential helper was not transient: `git config --local credential.helper` wrote into the reused workspace's .git/config and the matching unset was skipped on every error path (bash -e abort, the fork-preflight exit 0, the salvage-loop exit 1s), stranding a helper that answers any host with the PAT where ci.yml / pr-review jobs could read it. All three push paths now use a one-shot, host-scoped `git -c credential."https://github.com".helper=...` (a git_auth wrapper for the salvage loop), so nothing is ever written to .git/config. Medium — narrow the sanitize allowlist's `remote.*` to `remote..(url|fetch|pushurl)` (uploadpack/receivepack are command strings the sweep previously kept), and cover all three inlined sanitize copies in the hardening test with a byte-identical pin (previously one of three), rewording the stale "composite action" references. Low — drop the recursive `rm -rf` hooks fallback that could wipe the git dir itself (warn instead), extend the /tmp sweep glob to the legacy fixed dir (`autofix*`), and make `git gc` threshold-driven (`--auto --prune=now`). The contract test pins the git -c / git_auth form, the absence of any config-written credential helper, and the tokenless URLs throughout. --- .github/workflows/qwen-autofix.yml | 58 ++++++++++----------- scripts/tests/qwen-autofix-workflow.test.js | 50 +++++++++++++----- 2 files changed, 64 insertions(+), 44 deletions(-) diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index 8e7b3dbe7b7..277021f8c96 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -705,7 +705,7 @@ jobs: # jobs execute human-authored code as the runner user, so a prior job # can plant git exec knobs (core.fsmonitor, filter.*.smudge, # diff.external, includeIf, hooks) in the local config that would fire - # inside THIS job's PAT-bearing git steps. The composite action keeps + # inside THIS job's PAT-bearing git steps. It keeps # a known-safe allowlist and unsets everything else, hardened against # the worktree-config and global-hooksPath bypasses verified in # qwen-triage on this pool. No-op on a fresh hosted runner. @@ -740,7 +740,7 @@ jobs: # runner) means grep exits 1, which would kill the step exactly # when there is nothing to clean. git config --local --name-only --list 2>/dev/null \ - | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } \ + | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.[^.]+\.(url|fetch|pushurl)|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } \ | while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done # Belt and braces after the config scrub: only delete inside the # repository's own git dir. A hooks path resolving anywhere else is @@ -765,7 +765,7 @@ jobs: # into it and put an empty hooks directory back. RAW_HOOKS="$(GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" echo "::warning::hooks path did not resolve inside the git dir (${HOOKS_ABS:-unresolved}); unlinking it." - rm -f "$RAW_HOOKS" 2>/dev/null || rm -rf "$RAW_HOOKS" 2>/dev/null || true + rm -f "$RAW_HOOKS" 2>/dev/null || echo "::warning::refusing to recursively delete planted hooks path '$RAW_HOOKS' (a hooksPath resolving to the git dir itself would otherwise wipe .git); leaving it to the pool re-clean." mkdir -p "${GIT_DIR_ABS:-.git}/hooks" 2>/dev/null || true git config --local --unset-all core.hooksPath 2>/dev/null || true fi @@ -787,10 +787,10 @@ jobs: # Age-sweep abandoned run-scoped dirs on this shared /tmp: a hard # runner kill skips the always() teardown and run_id never repeats, # so nothing else ever reclaims them. - find /tmp -maxdepth 1 -name 'autofix-*' -mmin +1440 -exec rm -rf {} + 2>/dev/null || true + find /tmp -maxdepth 1 -name 'autofix*' -mmin +1440 -exec rm -rf {} + 2>/dev/null || true # The reused workspace's .git accumulates unreferenced objects # across fetch runs on this persistent pool; prune them. - git gc --prune=now --quiet 2>/dev/null || true + git gc --auto --prune=now --quiet 2>/dev/null || true # Self-hosted runners keep the workspace's .git across runs, so a # failed earlier attempt's local branch survives here: the agent's @@ -1448,15 +1448,13 @@ jobs: BRANCH="autofix/issue-${ISSUE}" git config --local --unset-all http.https://github.com/.extraheader || true git config core.hooksPath /dev/null - # Authenticate the push via a transient credential helper: the - # PAT enters neither the remote URL (persisted to .git/config on - # this reused workspace) nor the process argv (/proc//cmdline - # is world-readable on this shared host). The config holds only a - # ${GITHUB_TOKEN} reference, and credential.* is off the sanitize - # allowlist, so a leftover helper is swept next run. - git config --local credential.helper '!f(){ echo username=x-access-token; echo "password=${GITHUB_TOKEN}"; };f' - git push --no-verify "https://github.com/${REPO}.git" "${BRANCH}" - git config --local --unset-all credential.helper || true + # Authenticate the push with a one-shot, host-scoped credential + # helper via `git -c`: nothing is written to the reused + # workspace's .git/config (no error path can strand it there), + # argv holds only the literal ${GITHUB_TOKEN} reference, and the + # host scope means it cannot answer a non-GitHub URL. + git -c credential."https://github.com".helper='!f(){ echo username=x-access-token; echo "password=${GITHUB_TOKEN}"; };f' \ + push --no-verify "https://github.com/${REPO}.git" "${BRANCH}" PR_URL="$(gh pr create --repo "${REPO}" \ --base main --head "${BRANCH}" \ @@ -2824,7 +2822,7 @@ jobs: # runner) means grep exits 1, which would kill the step exactly # when there is nothing to clean. git config --local --name-only --list 2>/dev/null \ - | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } \ + | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.[^.]+\.(url|fetch|pushurl)|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } \ | while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done # Belt and braces after the config scrub: only delete inside the # repository's own git dir. A hooks path resolving anywhere else is @@ -2849,7 +2847,7 @@ jobs: # into it and put an empty hooks directory back. RAW_HOOKS="$(GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" echo "::warning::hooks path did not resolve inside the git dir (${HOOKS_ABS:-unresolved}); unlinking it." - rm -f "$RAW_HOOKS" 2>/dev/null || rm -rf "$RAW_HOOKS" 2>/dev/null || true + rm -f "$RAW_HOOKS" 2>/dev/null || echo "::warning::refusing to recursively delete planted hooks path '$RAW_HOOKS' (a hooksPath resolving to the git dir itself would otherwise wipe .git); leaving it to the pool re-clean." mkdir -p "${GIT_DIR_ABS:-.git}/hooks" 2>/dev/null || true git config --local --unset-all core.hooksPath 2>/dev/null || true fi @@ -3061,7 +3059,7 @@ jobs: # jobs execute human-authored code as the runner user, so a prior job # can plant git exec knobs (core.fsmonitor, filter.*.smudge, # diff.external, includeIf, hooks) in the local config that would fire - # inside THIS job's PAT-bearing git steps. The composite action keeps + # inside THIS job's PAT-bearing git steps. It keeps # a known-safe allowlist and unsets everything else, hardened against # the worktree-config and global-hooksPath bypasses verified in # qwen-triage on this pool. No-op on a fresh hosted runner. @@ -3096,7 +3094,7 @@ jobs: # runner) means grep exits 1, which would kill the step exactly # when there is nothing to clean. git config --local --name-only --list 2>/dev/null \ - | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } \ + | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.[^.]+\.(url|fetch|pushurl)|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } \ | while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done # Belt and braces after the config scrub: only delete inside the # repository's own git dir. A hooks path resolving anywhere else is @@ -3121,7 +3119,7 @@ jobs: # into it and put an empty hooks directory back. RAW_HOOKS="$(GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" echo "::warning::hooks path did not resolve inside the git dir (${HOOKS_ABS:-unresolved}); unlinking it." - rm -f "$RAW_HOOKS" 2>/dev/null || rm -rf "$RAW_HOOKS" 2>/dev/null || true + rm -f "$RAW_HOOKS" 2>/dev/null || echo "::warning::refusing to recursively delete planted hooks path '$RAW_HOOKS' (a hooksPath resolving to the git dir itself would otherwise wipe .git); leaving it to the pool re-clean." mkdir -p "${GIT_DIR_ABS:-.git}/hooks" 2>/dev/null || true git config --local --unset-all core.hooksPath 2>/dev/null || true fi @@ -3144,10 +3142,10 @@ jobs: # Age-sweep abandoned run-scoped dirs on this shared /tmp: a hard # runner kill skips the always() teardown and run_id never repeats, # so nothing else ever reclaims them. - find /tmp -maxdepth 1 -name 'autofix-*' -mmin +1440 -exec rm -rf {} + 2>/dev/null || true + find /tmp -maxdepth 1 -name 'autofix*' -mmin +1440 -exec rm -rf {} + 2>/dev/null || true # The reused workspace's .git accumulates unreferenced objects # across fetch runs on this persistent pool; prune them. - git gc --prune=now --quiet 2>/dev/null || true + git gc --auto --prune=now --quiet 2>/dev/null || true # Stage the schema gate script from the TRUSTED BASE checkout before # "Prepare branch and feedback" switches the working tree to the PR @@ -3398,8 +3396,8 @@ jobs: # and fine-grained PATs are documented as NOT receiving it. # Prove push access NOW, before an agent round is spent, instead # of 403ing at the report step after the work is done. - git config --local credential.helper '!f(){ echo username=x-access-token; echo "password=${GITHUB_TOKEN}"; };f' - if ! git push --no-verify --dry-run "https://github.com/${HEAD_REPO}.git" HEAD:"${BRANCH}" > /dev/null 2>&1; then + if ! git -c credential."https://github.com".helper='!f(){ echo username=x-access-token; echo "password=${GITHUB_TOKEN}"; };f' \ + push --no-verify --dry-run "https://github.com/${HEAD_REPO}.git" HEAD:"${BRANCH}" > /dev/null 2>&1; then echo "🫥 fork push preflight failed for ${HEAD_REPO} (allow-edits grant or PAT type) — discarding without action or marker" { echo "stale=true" @@ -3409,7 +3407,6 @@ jobs: } >> "${GITHUB_OUTPUT}" exit 0 fi - git config --local --unset-all credential.helper || true else git checkout -B "${BRANCH}" "origin/${BRANCH}" fi @@ -4299,10 +4296,10 @@ jobs: # commits get checked). A pre-push hook would execute that code # with the PAT in env — sever hooks entirely before pushing. git config core.hooksPath /dev/null - # Authenticate via a transient credential helper instead of a - # token-bearing URL: the PAT enters neither .git/config nor the - # process argv on this shared host (see Publish PR). - git config --local credential.helper '!f(){ echo username=x-access-token; echo "password=${GITHUB_TOKEN}"; };f' + # Authenticate push/fetch with a one-shot, host-scoped credential + # helper via a git_auth wrapper (see Publish PR) — nothing lands + # in .git/config, argv holds only the ${GITHUB_TOKEN} reference. + git_auth() { git -c credential."https://github.com".helper='!f(){ echo username=x-access-token; echo "password=${GITHUB_TOKEN}"; };f' "$@"; } if [[ "${HEAD_REPO:-${REPO}}" != "${REPO}" ]]; then # Push back to the FORK branch via allow-edits (PAT has push # rights on the upstream, which GitHub extends to the fork's @@ -4328,7 +4325,7 @@ jobs: # through to the existing failure path — same as today. PUSH_RACE_MERGED='false' for push_attempt in 1 2 3; do - if git push --no-verify "${PUSH_URL}" HEAD:"${BRANCH}"; then + if git_auth push --no-verify "${PUSH_URL}" HEAD:"${BRANCH}"; then break fi if [[ "${push_attempt}" == 3 ]]; then @@ -4336,7 +4333,7 @@ jobs: exit 1 fi echo "⚠️ push rejected (attempt ${push_attempt}) — branch moved during the run; merging the new head and retrying" - if ! git fetch "${PUSH_URL}" "refs/heads/${BRANCH}"; then + if ! git_auth fetch "${PUSH_URL}" "refs/heads/${BRANCH}"; then echo "::error::could not fetch the moved head (attempt ${push_attempt}) — cannot salvage this push" exit 1 fi @@ -4357,7 +4354,6 @@ jobs: PUSH_RACE_MERGED='true' fi done - git config --local --unset-all credential.helper || true CAN_RESOLVE_THREADS='false' if [[ -s "${WORKDIR}/resolved-comments.txt" ]]; then LOCAL_PUSHED_HEAD="$(git rev-parse HEAD)" diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 4a833e4dddc..c23d6831c2e 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -64,10 +64,19 @@ const reviewAddressJob = // The sanitize step is inlined into each heavy job as a `run:` step (a // local action would need a checkout it is meant to precede). issue-autofix's // copy is the canonical text for the ordering and hardening assertions below. -const sanitizeStep = - issueAutofixJob.match( +const sanitizeStepOf = (job) => + job.match( /- name: 'Sanitize workspace git config'[\s\S]*?(?=\n[ ]{6}- name: ')/, )?.[0] ?? ''; +// All three heavy jobs inline the SAME sanitize step (it must precede the +// checkout it protects, so it cannot be a shared action). The byte-identical +// pin below makes the hardening assertions cover every copy, not one of three. +const sanitizeSteps = [ + sanitizeStepOf(issueAutofixJob), + sanitizeStepOf(buildCliJob), + sanitizeStepOf(reviewAddressJob), +]; +const sanitizeStep = sanitizeSteps[0]; const publishPrStep = workflow.match( /- name: 'Publish PR'[\s\S]*?(?=\n[ ]{6}- name: 'Withdraw claim on failure')/, @@ -2227,13 +2236,13 @@ describe('qwen-autofix workflow', () => { 'PUSH_URL="https://github.com/${HEAD_REPO}.git"', ); expect(workflow).toContain( - 'git push --no-verify "${PUSH_URL}" HEAD:"${BRANCH}"', + 'git_auth push --no-verify "${PUSH_URL}" HEAD:"${BRANCH}"', ); // The allow-edits grant rides the classic-PAT path only — prepare must // prove push access BEFORE an agent round is spent, discarding // gracefully instead of 403ing at the report step. expect(workflow).toContain( - 'git push --no-verify --dry-run "https://github.com/${HEAD_REPO}.git" HEAD:"${BRANCH}"', + 'push --no-verify --dry-run "https://github.com/${HEAD_REPO}.git" HEAD:"${BRANCH}"', ); expect(workflow).toContain('fork push preflight failed'); // First-pickup engage ack anchors the window when the label path could @@ -5619,7 +5628,13 @@ describe('qwen-autofix workflow', () => { expect(buildCliJob).not.toContain("- name: 'Drop stale autofix branches'"); }); - it('hardens the shared git-config sanitize action against the verified bypasses', () => { + it('hardens the inlined git-config sanitize step against the verified bypasses', () => { + // The step is inlined into all three heavy jobs (a shared action cannot + // run before checkout); the copies must stay byte-identical so the + // assertions below hold for every job, not just issue-autofix. + expect(sanitizeSteps[0]).toBeTruthy(); + expect(sanitizeSteps[1]).toBe(sanitizeSteps[0]); + expect(sanitizeSteps[2]).toBe(sanitizeSteps[0]); // Worktree-scoped config first: extensions.worktreeConfig activates // .git/config.worktree, which `git config --local` neither lists nor // unsets and which CAN carry core.hooksPath — pointing the hook @@ -5647,8 +5662,8 @@ describe('qwen-autofix workflow', () => { expect(sanitizeStep).toContain( 'git config --local --unset-all core.hooksPath', ); - // Provenance link: the action and qwen-triage's hardened step must be - // edited together. + // Provenance link: the inlined step and qwen-triage's hardened step must + // be edited together. expect(sanitizeStep).toContain('qwen-triage'); }); @@ -6464,7 +6479,7 @@ describe('qwen-autofix workflow', () => { // git push twice more and the salvage legs execute against a branch // that was already pushed. expect(pushAndReportStep).toMatch( - /if git push --no-verify "\$\{PUSH_URL\}" HEAD:"\$\{BRANCH\}"; then\n\s+break/, + /if git_auth push --no-verify "\$\{PUSH_URL\}" HEAD:"\$\{BRANCH\}"; then\n\s+break/, ); // BOTH push-URL constructions stay pinned — the fork one is pinned by // the fork-plumbing test, and the same-repo one lost its old @@ -6475,7 +6490,7 @@ describe('qwen-autofix workflow', () => { 'PUSH_URL="https://github.com/${REPO}.git"', ); expect(pushAndReportStep).toContain( - 'git fetch "${PUSH_URL}" "refs/heads/${BRANCH}"', + 'git_auth fetch "${PUSH_URL}" "refs/heads/${BRANCH}"', ); // Every failure path in the salvage loop is ::error::-annotated — a // deleted fork branch (or transient network error) must not kill the @@ -6549,7 +6564,7 @@ describe('qwen-autofix workflow', () => { expect(workflow).not.toMatch(/\bgit push\b[^\n]* -[a-zA-Z]*f\b/); expect(workflow).not.toMatch(/\bgit push\b[^\n]* \+\S/); expect(publishPrStep).toContain( - 'git push --no-verify "https://github.com/${REPO}.git" "${BRANCH}"', + 'push --no-verify "https://github.com/${REPO}.git" "${BRANCH}"', ); // Neither PAT push may expose the token — not persisted to .git/config // (a `git remote set-url`) and not in the process argv (a token-bearing @@ -6560,10 +6575,19 @@ describe('qwen-autofix workflow', () => { expect(pushAndReportStep).not.toContain('git remote set-url'); expect(publishPrStep).not.toContain('x-access-token:${GITHUB_TOKEN}@'); expect(pushAndReportStep).not.toContain('x-access-token:${GITHUB_TOKEN}@'); - expect(publishPrStep).toContain('credential.helper'); - expect(pushAndReportStep).toContain('credential.helper'); + expect(publishPrStep).toContain('credential."https://github.com".helper'); + expect(pushAndReportStep).toContain( + 'credential."https://github.com".helper', + ); + // `git -c` never writes the helper into the reused workspace's + // .git/config, so no error path can strand a credential there for the + // next job that lands on this host to read. + expect(publishPrStep).not.toContain('git config --local credential.helper'); + expect(pushAndReportStep).not.toContain( + 'git config --local credential.helper', + ); expect(pushAndReportStep).toContain( - 'git push --no-verify "${PUSH_URL}" HEAD:"${BRANCH}"', + 'git_auth push --no-verify "${PUSH_URL}" HEAD:"${BRANCH}"', ); // Five sites now: both PAT pushes, the PAT-bearing prepare checkout, // AND both no-secret verification checkouts (convention: every host From 1e35b582ce76246fc94514d9dc37a42ebfd708ce Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 6 Aug 2026 13:04:06 +0800 Subject: [PATCH 8/8] ci(autofix): pin gc to the foreground and anchor push auth assertions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review follow-ups: - `git gc --auto --prune=now` detaches by default (gc.autoDetach=true), and --prune=now is unsafe when other processes write concurrently — exactly what follows in these 300-minute jobs. Force it synchronous with `-c gc.autoDetach=false` so the repack still only fires when the --auto thresholds say so, but never races the checkout, agent commits, or merge/retry loop that come after it. - Tighten the two push assertions (dry-run preflight, Publish PR) to require the host-scoped `git -c credential."https://github.com".helper` prefix immediately before the push, instead of a bare `push --no-verify …` match that a `git push` regression would still satisfy. --- .github/workflows/qwen-autofix.yml | 4 ++-- scripts/tests/qwen-autofix-workflow.test.js | 13 +++++++++---- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index f2044215abe..e8a4c4ade43 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -790,7 +790,7 @@ jobs: find /tmp -maxdepth 1 -name 'autofix*' -mmin +1440 -exec rm -rf {} + 2>/dev/null || true # The reused workspace's .git accumulates unreferenced objects # across fetch runs on this persistent pool; prune them. - git gc --auto --prune=now --quiet 2>/dev/null || true + git -c gc.autoDetach=false gc --auto --prune=now --quiet 2>/dev/null || true # Self-hosted runners keep the workspace's .git across runs, so a # failed earlier attempt's local branch survives here: the agent's @@ -3147,7 +3147,7 @@ jobs: find /tmp -maxdepth 1 -name 'autofix*' -mmin +1440 -exec rm -rf {} + 2>/dev/null || true # The reused workspace's .git accumulates unreferenced objects # across fetch runs on this persistent pool; prune them. - git gc --auto --prune=now --quiet 2>/dev/null || true + git -c gc.autoDetach=false gc --auto --prune=now --quiet 2>/dev/null || true # Stage the schema gate script from the TRUSTED BASE checkout before # "Prepare branch and feedback" switches the working tree to the PR diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 9ce61e8006f..26947c4a86e 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -2241,8 +2241,11 @@ describe('qwen-autofix workflow', () => { // The allow-edits grant rides the classic-PAT path only — prepare must // prove push access BEFORE an agent round is spent, discarding // gracefully instead of 403ing at the report step. - expect(workflow).toContain( - 'push --no-verify --dry-run "https://github.com/${HEAD_REPO}.git" HEAD:"${BRANCH}"', + // Require the git -c form, not a bare `git push` (which a plain + // `push --no-verify …` match would still satisfy): the host-scoped + // credential prefix must immediately precede the push. + expect(workflow).toMatch( + /git -c credential\."https:\/\/github\.com"\.helper=[^\n]*\n\s+push --no-verify --dry-run "https:\/\/github\.com\/\$\{HEAD_REPO\}\.git"/, ); expect(workflow).toContain('fork push preflight failed'); // First-pickup engage ack anchors the window when the label path could @@ -6565,8 +6568,10 @@ describe('qwen-autofix workflow', () => { // force flag; long options (--no-verify) start with -- and are exempt. expect(workflow).not.toMatch(/\bgit push\b[^\n]* -[a-zA-Z]*f\b/); expect(workflow).not.toMatch(/\bgit push\b[^\n]* \+\S/); - expect(publishPrStep).toContain( - 'push --no-verify "https://github.com/${REPO}.git" "${BRANCH}"', + // Same anchor as the dry-run: the publish push must carry the + // host-scoped `git -c credential…` prefix, not a bare `git push`. + expect(publishPrStep).toMatch( + /git -c credential\."https:\/\/github\.com"\.helper=[^\n]*\n\s+push --no-verify "https:\/\/github\.com\/\$\{REPO\}\.git"/, ); // Neither PAT push may expose the token — not persisted to .git/config // (a `git remote set-url`) and not in the process argv (a token-bearing