diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index 49ffaa1dffa..e8a4c4ade43 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -658,7 +658,19 @@ 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 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: group: 'qwen-autofix-issue-${{ needs.route.outputs.issue_number || github.run_id }}' @@ -667,12 +679,97 @@ 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. 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. + - 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 + # 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\.[^.]+\.(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 + # 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 || 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 + - name: 'Checkout' uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 with: @@ -682,7 +779,34 @@ 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. 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. + 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 -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 + # 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: |- + # 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 # trusted checkout's copy of the schema gate, never a working-tree copy. @@ -717,18 +841,39 @@ jobs: - name: 'Check runner environment' env: RUNNER_ENVIRONMENT: '${{ runner.environment }}' + RUNNER_NAME: '${{ runner.name }}' run: |- case "${RUNNER_ENVIRONMENT}" in - github-hosted) ;; + github-hosted|self-hosted) ;; *) echo "::error::Unsupported runner environment: ${RUNNER_ENVIRONMENT:-unset}." 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 + # 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 (hosted)' - if: |- - ${{ runner.environment == 'github-hosted' }} + - name: 'Set up Node.js' uses: 'actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e' # v6.4.0 with: node-version: '22.x' @@ -740,8 +885,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 @@ -1260,7 +1410,7 @@ jobs: uses: 'actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a' # v7.0.1 with: name: 'autofix-issue-artifacts' - path: '/tmp/autofix/' + path: '${{ env.WORKDIR }}/' if-no-files-found: 'ignore' - name: 'Publish PR' @@ -1297,9 +1447,14 @@ 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 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}" \ @@ -1372,6 +1527,16 @@ 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. 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}"' + # =========================================================================== # TAKEOVER COMMAND — the accepted comment command's side effects are the # label toggle plus its own acknowledgements: add on an unlabeled PR @@ -2604,13 +2769,91 @@ 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'' && (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" + + # 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' + 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\.[^.]+\.(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 + # 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 || 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 + - name: 'Checkout trusted base' uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 with: @@ -2618,9 +2861,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' @@ -2702,7 +2943,22 @@ 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. 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: contents: 'read' @@ -2788,6 +3044,88 @@ 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. 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. + - 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 + # 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\.[^.]+\.(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 + # 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 || 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 + - name: 'Checkout trusted base' uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 with: @@ -2798,7 +3136,18 @@ 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. 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. + 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 -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 @@ -2830,18 +3179,39 @@ jobs: - name: 'Check runner environment' env: RUNNER_ENVIRONMENT: '${{ runner.environment }}' + RUNNER_NAME: '${{ runner.name }}' run: |- case "${RUNNER_ENVIRONMENT}" in - github-hosted) ;; + github-hosted|self-hosted) ;; *) echo "::error::Unsupported runner environment: ${RUNNER_ENVIRONMENT:-unset}." 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 + # 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 (hosted)' - if: |- - ${{ runner.environment == 'github-hosted' }} + - name: 'Set up Node.js' uses: 'actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e' # v6.4.0 with: node-version: '22.x' @@ -2853,8 +3223,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 @@ -3023,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 + 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" @@ -3917,19 +4293,22 @@ 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 # with the PAT in env — sever hooks entirely before pushing. git config core.hooksPath /dev/null + # 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 # 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 @@ -3948,7 +4327,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 @@ -3956,7 +4335,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 @@ -4805,3 +5184,14 @@ 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. 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 05ea5dd5340..26947c4a86e 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( @@ -50,6 +50,33 @@ 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}[a-z][a-z0-9-]*:\n)/)?.[0] ?? + ''; +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 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')/, @@ -148,9 +175,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'); @@ -1286,12 +1312,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], ); @@ -2211,16 +2233,19 @@ 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}"', + '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://x-access-token:${GITHUB_TOKEN}@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 @@ -5061,7 +5086,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 +5229,36 @@ 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. 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'' && (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': 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"]', ); @@ -5219,6 +5268,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); @@ -5308,6 +5391,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', ); @@ -5323,21 +5410,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 @@ -5361,13 +5442,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 }}'", ); @@ -5375,9 +5456,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/ plus packages/core/dist — // copy_bundle_assets.js already gathers every runtime asset under the @@ -5398,7 +5479,7 @@ 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"', ); @@ -5407,7 +5488,7 @@ describe('qwen-autofix workflow', () => { // settings-schema generator crash with ERR_MODULE_NOT_FOUND — pin the // restore-side assertion. expect(restoreStep).toContain('test -f packages/core/dist/index.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. @@ -5415,7 +5496,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'); @@ -5458,7 +5539,13 @@ 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. Both artifact uploads read + // ${{ env.WORKDIR }} — one source, nothing to drift. + expect(workflow).toContain("WORKDIR: '/tmp/autofix-${{ github.run_id }}'"); + expect(workflow.match(/path: '\$\{\{ env\.WORKDIR \}\}\/'/g)).toHaveLength( + 2, + ); expect(workflow).toContain( "WORKDIR: '/tmp/autofix-review-${{ matrix.target.pr }}'", ); @@ -5466,6 +5553,26 @@ describe('qwen-autofix workflow', () => { for (const step of resetAutofixWorkspaceSteps) { expect(step).toContain('rm -rf "${WORKDIR}"'); expect(step).toContain('mkdir -p "${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). + 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'"), @@ -5478,6 +5585,106 @@ 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")); + } + // 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( + "- 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 + // `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 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 + // sweep's recursive delete at /. Then the allowlist sweep, and only + // then the hooks resolution: the ordering IS the containment. + const rmWorktreeCfg = sanitizeStep.indexOf('--git-path config.worktree'); + const unsetExt = sanitizeStep.indexOf( + '--unset-all extensions.worktreeConfig', + ); + 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); + 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(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 inlined step and qwen-triage's hardened step must + // be edited together. + 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', () => { const qwenSteps = [ assessCandidatesStep, @@ -6277,7 +6484,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 @@ -6285,10 +6492,10 @@ 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}"', + '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 @@ -6361,9 +6568,33 @@ 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}"'); + // 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 + // 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."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