ci(autofix): run heavy autofix jobs on the ECS pool with hosted fallback - #8603
Conversation
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.
|
Re-run on
Moving on to code review. 🔍 中文说明由
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewBefore reading the diff, my independent plan for "move the heavy jobs to the ECS pool" was: route the three jobs through the fleet's repository-guarded expression with hosted fallback, relax the hosted-only environment check while asserting pool membership, provision Node unconditionally, handle persistent-runner state (workspace residue, planted git config, /tmp collisions, token handling on a shared host), and pin the routing in the contract test. The PR matches that plan and goes deeper on the hardening than a first pass would — I found no simpler path it missed, and no blocker. Verified against the head:
No correctness or security blockers. Two non-blocking items carried over from the maintainer's own round 3, both explicitly the author's call: the Testing evidence — the PR's own CI, read via the API (PR code is never executed in this review)All checks on the reviewed commit are settled green; the skipped macOS/Windows/integration legs are merge-queue-only by design in
The one claim neither static review nor this CI can settle — that scheduled legs actually land on 中文说明代码审查读 diff 之前,我对"把重型 job 迁到 ECS 池"的独立方案是:三个 job 走 runner 池统一的、带仓库守卫的路由表达式并保留托管回退,放宽仅限托管的环境检查同时断言池成员身份,无条件安装 Node,处理持久 runner 的状态问题(工作区残留、被植入的 git 配置、/tmp 冲突、共享主机上的 token 处理),并把路由钉进契约测试。PR 与该方案一致,加固深度还超过首轮预期——我没有找到它遗漏的更简路径,也没有发现阻塞项。已在当前 head 上核实:
无正确性或安全阻塞项。两个非阻塞遗留项来自维护者自己的第 3 轮、均明确由作者酌定:同一池子上 测试证据——经 API 读取本 PR 自己的 CI(本审查不执行 PR 代码)被审 commit 上所有检查均已结束且为绿;macOS/Windows/集成腿按 静态审查与 CI 都无法定论的唯一主张——定时 leg 真的落到 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
Confidence: 4/5 — clean review across every stage; the only open items are two non-blocking nits the maintainer already raised and left to the author's discretion. Stepping back: the PR matches my independent proposal and exceeds it where it matters — the persistent-pool hardening (allowlist git-config sanitize, ownership restore, run-scoped 0700 workdirs, one-shot host-scoped push credentials, runner-name assertion) is exactly what secret-bearing jobs need before living on shared persistent machines, and every block is pinned by the contract test so it can't silently rot. Three rounds of maintainer review have already shaken out the real defects — the Process note, since this re-run exists because of it: the merge block was the bot's own 中文说明置信度:4/5 —— 每个阶段都干净;仅有的未决项是维护者已经提出、交由作者酌定的两个非阻塞小项。 退一步看:PR 与我的独立方案一致,并在关键处超出预期——持久池加固(allowlist 式 git 配置清理、所有权恢复、按 run 隔离的 0700 工作目录、一次性限定主机的 push 凭据、runner 名称断言)正是携带 secret 的 job 住进共享持久机器之前所需要的,且每一块都被契约测试钉住,不会悄悄退化。维护者的三轮评审已经把真正的缺陷都筛了出来—— 流程说明(本次重跑正因它而起):挡住合并的是 bot 自己的一条 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-autofix.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| 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"]'') }}' |
There was a problem hiding this comment.
[Critical] R1-1: These jobs now run on the shared persistent ecs-qwen pool as the non-root runner user, but — unlike every sibling workflow on this pool — none of the three moved jobs restores workspace ownership before checkout (qwen-autofix.yml has zero chown/ownership handling). Applies to all three jobs routed by this expression (issue-autofix, build-cli, review-address).
Failure scenario: containerized jobs on the same pool leave root-owned files in the reused workspace — npm-cache.yml runs actions/checkout + npm ci as root inside node:22-bookworm on these exact labels, and the qwen-triage container lanes chown -R root:root their state. When one of these jobs next lands on that registration, its checkout dies on the root-owned .git (pinned actions/checkout source: tryClean EACCES → rmRF EACCES → step fails), and npm ci on root-owned node_modules fails the same way; a build-cli failure cascades to every review-address leg via the empty-base_sha validation. The state persists, so reruns keep failing until ownership is fixed manually. Unreachable on ubuntu-latest. Sibling precedent: qwen-triage.yml:460, serve-ab.yml:63, sdk-java.yml:72/135, ci.yml:163/549/935, qwen-code-pr-review.yml:370.
Suggested fix: before the checkout in all three jobs, add the same self-hosted-only ownership-restore step the siblings use (chown the workspace to the runner uid with a sudo -n fallback, chmod -R u+rwX, warn-not-fail) — ideally identical step text so pool hygiene stays uniform.
中文说明
这三个 job 现在以非 root 的 runner 用户身份跑在共享的持久化 ecs-qwen 池上,但与该池上所有同类 workflow 不同,checkout 前都没有工作区属主恢复步骤(qwen-autofix.yml 中没有任何 chown/属主处理)。适用于由此表达式路由的三个 job(issue-autofix、build-cli、review-address)。
失败场景:同一池上的容器化 job 会在复用的工作区留下 root 属主文件——npm-cache.yml 在这些标签上以 root 身份在 node:22-bookworm 容器内运行 actions/checkout + npm ci,qwen-triage 的容器 lane 也会 chown -R root:root。当这些 job 下一次落到该注册 runner 上时,checkout 会在 root 属主的 .git 上失败(固定版本的 actions/checkout 源码:tryClean EACCES → rmRF EACCES → 步骤失败),root 属主的 node_modules 上的 npm ci 同样失败;build-cli 失败会通过空 base_sha 校验级联到所有 review-address leg。该状态持久存在,重试会一直失败,直到人工修复属主。在 ubuntu-latest 上不可达。同类先例:qwen-triage.yml:460、serve-ab.yml:63、sdk-java.yml:72/135、ci.yml:163/549/935、qwen-code-pr-review.yml:370。
修复建议:在三个 job 的 checkout 前,添加与同类 workflow 一致的、仅自托管执行的属主恢复步骤(以 runner uid chown 工作区,sudo -n 兜底,chmod -R u+rwX,告警但不失败)——最好使用完全相同的步骤文本,保持池内卫生处理一致。
— qwen3.8-max via Qwen Code /review (v0.21.6)
There was a problem hiding this comment.
Addressed in abe8168: all three ECS-routed jobs (issue-autofix, build-cli, review-address) now run a 'Restore workspace ownership' step before checkout, chown-ing the reused workspace back to the runner uid/gid (with a sudo fallback), matching the sibling pool workflows.
| # 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"]'') }}' |
There was a problem hiding this comment.
[Critical] R1-2: The moved jobs never sanitize the reused workspace's local git config before secret-bearing git operations. Other pool consumers execute human-authored code as the runner user without a container (ci.yml's same-repo/write+ lanes), and such code can plant git exec knobs — core.fsmonitor, filter.*.smudge, diff.external, includeIf — in the shared workspace's .git/config, where they persist across runs. qwen-triage.yml:483-514 built a dedicated allowlist-unset-all step for exactly this vector on this same pool; these jobs have no equivalent. Same class applies to issue-autofix (line 665).
Failure scenario: review-address's 'Prepare branch and feedback' step carries GITHUB_TOKEN: secrets.CI_DEV_BOT_PAT while running git status / git restore / git fetch / git checkout -B / git merge-tree against the shared workspace — a planted core.fsmonitor fires on git status, a planted filter.*.smudge on git checkout, both inside the PAT-bearing step. A write+ author's planted knob therefore executes inside a step holding the bot PAT, escalating from "internal author" to the dev-bot identity the fleet's trust gates key on. On hosted runners this was unreachable (fresh VM per job); this routing change makes it reachable.
Suggested fix: before the first secret-bearing git operation in issue-autofix and review-address, add qwen-triage's git-config sanitization pattern (allowlisted git config --local --unset-all of every non-plumbing key plus a symlink-aware hook sweep) — ideally extracted into a shared composite action so the two workflows cannot drift.
中文说明
迁移后的 job 在携带 secret 的 git 操作之前,从不清理复用工作区的本地 git 配置。该池上另有消费者以 runner 用户身份、无容器地执行人类编写的代码(ci.yml 的同仓 write+ lane),这类代码可以在共享工作区的 .git/config 中埋入 git 执行开关——core.fsmonitor、filter.*.smudge、diff.external、includeIf——并跨运行持久存在。qwen-triage.yml:483-514 正是为同一池上的这一向量专门构建了白名单式 unset-all 步骤;这些 job 没有等价处理。同类问题同样适用于 issue-autofix(第 665 行)。
失败场景:review-address 的 'Prepare branch and feedback' 步骤携带 GITHUB_TOKEN: secrets.CI_DEV_BOT_PAT,同时对共享工作区运行 git status / git restore / git fetch / git checkout -B / git merge-tree——埋入的 core.fsmonitor 会在 git status 时触发,埋入的 filter.*.smudge 会在 git checkout 时触发,两者都发生在携带 PAT 的步骤内部。于是 write+ 作者埋入的开关会在持有 bot PAT 的步骤内执行,从"内部作者"提权为整套信任体系所依赖的 dev-bot 身份。在托管 runner 上这不可达(每个 job 都是全新 VM);本次路由改动使其变得可达。
修复建议:在 issue-autofix 和 review-address 首个携带 secret 的 git 操作之前,加入 qwen-triage 的 git 配置清理模式(对每个非管道类 key 做白名单式 git config --local --unset-all,并清理 hook 符号链接)——最好抽成共享 composite action,避免两个 workflow 漂移。
— qwen3.8-max via Qwen Code /review (v0.21.6)
There was a problem hiding this comment.
Addressed in abe8168: a 'Sanitize workspace git config' step now unsets exec knobs (core.fsmonitor, filter.*.smudge, diff.external, includeIf, hooks) from the reused workspace's local config — the qwen-triage allowlist-unset-all pattern — before the PAT-bearing git steps in issue-autofix and review-address.
| # 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"]'') }}' |
There was a problem hiding this comment.
[Critical] R1-3: issue-autofix's fixed WORKDIR: '/tmp/autofix' assumes the machine isolation hosted runners guaranteed; the persistent pool does not provide it. This host carries 25 ecs-qwen registrations sharing one OS-global /tmp, and issue-phase concurrency groups are unique per run (needs.route.outputs.issue_number is only ever set from workflow_dispatch input, so on schedule/issues events the group is qwen-autofix-issue-<run_id>) — no two issue-phase runs are ever serialized against each other.
Failure scenario: an issues:labeled run and a scheduled tick (or two issues labeled close together) overlap within the 180-minute window and land on two registrations of the same host: both rm -rf and rewrite the same /tmp/autofix candidates.json/decision.json/pr-title.txt. 'Read decision' then reads the other run's decision, and the PAT-bearing 'Publish PR' step can claim, develop, and push for the wrong issue. review-address avoids this by construction via per-PR paths; the issue phase has no equivalent.
WORKDIR: '/tmp/autofix-${{ github.run_id }}'Suggested fix: give each run a private workdir (as above), keeping the existing rm -rf/mkdir reset.
中文说明
issue-autofix 固定的 WORKDIR: '/tmp/autofix' 依赖托管 runner 保证的机器隔离;持久化池并不提供这种隔离。本机有 25 个 ecs-qwen 注册 runner 共享同一个操作系统级 /tmp,而 issue 阶段的并发组按运行唯一(needs.route.outputs.issue_number 只会从 workflow_dispatch 输入设置,因此 schedule/issues 事件下组名为 qwen-autofix-issue-<run_id>)——任何两个 issue 阶段运行之间都不会串行。
失败场景:一次 issues:labeled 运行与一次定时触发(或短时间内两个 issue 被打标签)在 180 分钟窗口内重叠,且落到同一台机器的两个注册 runner 上:两者都会 rm -rf 并重写同一个 /tmp/autofix 下的 candidates.json/decision.json/pr-title.txt。'Read decision' 会读到另一次运行的 decision,携带 PAT 的 'Publish PR' 步骤可能为错误的 issue 认领、开发并推送。review-address 通过按 PR 的路径从构造上避免了此问题;issue 阶段没有等价处理。
修复建议:为每次运行提供私有工作目录(如上所示),保留现有的 rm -rf/mkdir 重置。
— qwen3.8-max via Qwen Code /review (v0.21.6)
There was a problem hiding this comment.
Addressed in abe8168: issue-autofix's WORKDIR is now per-run (/tmp/autofix-${{ github.run_id }}), so overlapping issue-phase runs on two registrations of the same host no longer share one /tmp/autofix.
| # 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"]'') }}' |
There was a problem hiding this comment.
[Critical] R1-4: Moving issue-autofix to the persistent pool leaves local autofix/issue-* branches from earlier attempts alive in the reused workspace's .git. 'Reset autofix workspace' wipes only /tmp/autofix; actions/checkout cleans untracked files but never deletes refs; there is no branch -D / update-ref -d anywhere in the job. On hosted runners every run was a fresh VM, so this state never existed.
Failure scenario: run 1 creates local branch autofix/issue-N (SKILL.md develop step) and then dies at the verify gate, times out, or fails publish; 'Withdraw claim on failure' removes autofix/in-progress and invites re-adding autofix/approved (workflow_dispatch with issue_number also retries). When the retry lands on the same registration, git switch -c autofix/issue-N fails with "branch already exists" (probe-verified, exit 128). Probe-verified adaptations: the agent checks out the stale branch and pushes the stale failed-attempt commits into the new PR, or writes failure.md → 'Withdraw claim' permanently labels the issue autofix/skip, killing automation for a healthy issue. review-address avoids this via git checkout -B; the issue phase has no equivalent. (Distinct from untracked-file leftovers, which actions/checkout's clean does remove — refs survive it.)
Suggested fix: drop stale local branches deterministically in the workflow after checkout, e.g. extend 'Reset autofix workspace' with:
git for-each-ref --format='%(refname:short)' 'refs/heads/autofix/issue-*' \
| xargs -r -n 1 git branch -D 2>/dev/null || true中文说明
将 issue-autofix 迁到持久化池后,早前尝试留下的本地 autofix/issue-* 分支会继续存活在复用工作区的 .git 中。'Reset autofix workspace' 只清理 /tmp/autofix;actions/checkout 会清理未跟踪文件但从不删除引用;整个 job 中没有任何 branch -D / update-ref -d。在托管 runner 上每次运行都是全新 VM,这种状态从未存在。
失败场景:运行 1 创建本地分支 autofix/issue-N(SKILL.md develop 步骤),随后在验证关卡失败、超时或发布失败;'Withdraw claim on failure' 移除 autofix/in-progress 并提示重新添加 autofix/approved(带 issue_number 的 workflow_dispatch 同样会重试)。当重试落到同一注册 runner 上时,git switch -c autofix/issue-N 因 "branch already exists" 失败(已用探针验证,exit 128)。探针验证的两种演化:agent 检出旧分支,把上次失败的提交推入新 PR;或者写出 failure.md → 'Withdraw claim' 给 issue 永久打上 autofix/skip 标签,使一个健康 issue 的自动化就此终止。review-address 通过 git checkout -B 避免了此问题;issue 阶段没有等价处理。(与未跟踪文件残留不同——actions/checkout 的 clean 会清掉未跟踪文件,但引用会存活。)
修复建议:在 workflow 中确定性地清理旧本地分支,例如在 'Reset autofix workspace' 后追加上面的命令。
— qwen3.8-max via Qwen Code /review (v0.21.6)
There was a problem hiding this comment.
Addressed in abe8168: after checkout the job sweeps stale local autofix/issue-* branches (git branch -D), so a retry landing on the same registration can't hit 'branch already exists' or push a failed attempt's stale commits.
| 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"]'') }}' |
There was a problem hiding this comment.
[Suggestion] R1-5: The routing expression tests only github.repository, which is QwenLM/qwen-code for fork-PR events too — so fork PR events do not fall back to hosted as the added comment claims, and these are the only PR-context-reachable jobs routed to the persistent pool without the head-repo / write+-author guard every sibling workflow adds for PR-triggered ECS routing (ci.yml:66, serve-ab.yml:60, sdk-java.yml:55/132, qwen-triage.yml:82/424). Applies to all three moved jobs.
Failure scenario: an external author opens a fork PR; pr-self-report-label.yml applies review/self-reported, firing pull_request: labeled against this workflow with github.repository still QwenLM/qwen-code, so build-cli (and review-address whenever any bot PR is a scan target) route onto the persistent pool from a workflow file read from the fork's merge ref. Impact is bounded today — route's label gate admits only autofix/takeover among pull_request events, fork PR events carry no secrets, checkouts stay trusted-base pinned, and the live write+ rechecks hold — but the defense rests entirely on those downstream gates rather than on runs-on, diverging from the fleet convention.
Suggested fix: either extend the guard for PR-context events (e.g. additionally require github.event.pull_request.head.repo.full_name == github.repository or a write+ author_association when the event is in the pull_request family), or correct the comment to state that fork-PR events are gated downstream, not by runs-on.
中文说明
路由表达式只检查 github.repository,而 fork PR 事件下它同样是 QwenLM/qwen-code——因此 fork PR 事件并不会像新增注释所说的那样回退到托管 runner,而且这些是唯一在 PR 上下文可达、却没有头仓库/write+ 作者守卫就路由到持久化池的 job——同类 workflow 的 PR 触发 ECS 路由都带此守卫(ci.yml:66、serve-ab.yml:60、sdk-java.yml:55/132、qwen-triage.yml:82/424)。适用于三个迁移的 job。
失败场景:外部作者开一个 fork PR;pr-self-report-label.yml 打上 review/self-reported 标签,触发针对本 workflow 的 pull_request: labeled 事件,此时 github.repository 仍是 QwenLM/qwen-code,于是 build-cli(以及只要有 bot PR 是扫描目标时的 review-address)会从 fork 合并引用读取的 workflow 文件出发,路由到持久化池。当前影响有界——route 的标签关卡在 pull_request 事件中只放行 autofix/takeover,fork PR 事件不携带 secret,checkout 固定在可信基,write+ 实时复核也成立——但防线完全依赖这些下游关卡而非 runs-on,与整套 runner 池的惯例不一致。
修复建议:要么为 PR 上下文事件扩展守卫(例如事件属于 pull_request 家族时额外要求 github.event.pull_request.head.repo.full_name == github.repository 或 write+ author_association),要么修正注释,说明 fork PR 事件由下游关卡把关而非 runs-on。
— qwen3.8-max via Qwen Code /review (v0.21.6)
There was a problem hiding this comment.
Addressed in abe8168: the runs-on expression now carries the PR-family guard — same-repo head OR OWNER/MEMBER/COLLABORATOR author — mirroring ci.yml's classify_pr convention, so fork-PR events from non-write authors fall back to ubuntu-latest. The runs-on comments that wrongly claimed forks already fell back were corrected too.
| # 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"]'') }}' |
There was a problem hiding this comment.
[Suggestion] R1-6: 'Publish PR' (line ~1302) writes CI_DEV_BOT_PAT into the origin remote URL via git remote set-url origin "https://x-access-token:${GITHUB_TOKEN}@github.com/…", and nothing resets it afterwards. On hosted runners the VM died within minutes; on the persistent pool the workspace survives, so a push-capable bot PAT sits in the workspace's .git/config until the next job's checkout wipes it.
Failure scenario: after every successful issue fix, the bot PAT remains on disk on the shared host; any subsequent step/job on that registration, or any foothold on the host, can read .git/config and act as qwen-code-dev-bot (push branches, open PRs, label issues) — the identity the fleet's trust gates key on. Nothing currently reads it, so this is residue/hygiene rather than an open hole.
Suggested fix: after the push, restore the credential-free remote (git remote set-url origin "https://github.com/${REPO}.git"), or authenticate transiently (e.g. git -c http.extraheader=…) so no secret is written to config. Same fix needed in review-address's 'Push and report' (see the twin comment there).
中文说明
'Publish PR'(约第 1302 行)通过 git remote set-url origin "https://x-access-token:${GITHUB_TOKEN}@github.com/…" 把 CI_DEV_BOT_PAT 写入 origin 远端 URL,之后没有任何重置。在托管 runner 上 VM 几分钟内就销毁;在持久化池上工作区会保留,于是可推送的 bot PAT 会留在工作区的 .git/config 中,直到下一个 job 的 checkout 将其清除。
失败场景:每次 issue 修复成功后,bot PAT 都残留在共享主机的磁盘上;该注册 runner 上后续的任何步骤/job,或主机上的任何立足点,都能读取 .git/config 并以 qwen-code-dev-bot 身份行动(推送分支、开 PR、打标签)——这正是整套信任体系所依赖的身份。当前没有任何步骤会读取它,所以这是残留/卫生问题而非开放漏洞。
修复建议:推送后恢复无凭据的远端(git remote set-url origin "https://github.com/${REPO}.git"),或改用临时认证(如 git -c http.extraheader=…),不把 secret 写入配置。review-address 的 'Push and report' 需要同样的修复(见该处的对应评论)。
— qwen3.8-max via Qwen Code /review (v0.21.6)
There was a problem hiding this comment.
Addressed: the push passes the token inline (git push "https://x-access-token:...@github.com/..."), not via git remote set-url, so the PAT is never written into the reused workspace's .git/config; the http.extraheader helper is also unset. Verified zero git remote set-url occurrences remain.
| # 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"]'') }}' |
There was a problem hiding this comment.
[Suggestion] R1-6: 'Push and report' (line ~3905) writes CI_DEV_BOT_PAT into the origin remote URL via git remote set-url origin "https://x-access-token:${GITHUB_TOKEN}@github.com/…", and nothing resets it afterwards. On hosted runners the VM died within minutes; on the persistent pool the workspace survives, so a push-capable bot PAT sits in the workspace's .git/config until the next job's checkout wipes it.
Failure scenario: after every successful address round, the bot PAT remains on disk on the shared host; any subsequent step/job on that registration, or any foothold on the host, can read .git/config and act as qwen-code-dev-bot (push branches, open PRs, label issues) — the identity the fleet's trust gates key on. Nothing currently reads it, so this is residue/hygiene rather than an open hole.
Suggested fix: after the push, restore the credential-free remote (git remote set-url origin "https://github.com/${REPO}.git"), or authenticate transiently (e.g. git -c http.extraheader=…) so no secret is written to config. Same fix needed in issue-autofix's 'Publish PR' (see the twin comment there).
中文说明
'Push and report'(约第 3905 行)通过 git remote set-url origin "https://x-access-token:${GITHUB_TOKEN}@github.com/…" 把 CI_DEV_BOT_PAT 写入 origin 远端 URL,之后没有任何重置。在托管 runner 上 VM 几分钟内就销毁;在持久化池上工作区会保留,于是可推送的 bot PAT 会留在工作区的 .git/config 中,直到下一个 job 的 checkout 将其清除。
失败场景:每次处理轮成功后,bot PAT 都残留在共享主机的磁盘上;该注册 runner 上后续的任何步骤/job,或主机上的任何立足点,都能读取 .git/config 并以 qwen-code-dev-bot 身份行动(推送分支、开 PR、打标签)——这正是整套信任体系所依赖的身份。当前没有任何步骤会读取它,所以这是残留/卫生问题而非开放漏洞。
修复建议:推送后恢复无凭据的远端(git remote set-url origin "https://github.com/${REPO}.git"),或改用临时认证(如 git -c http.extraheader=…),不把 secret 写入配置。issue-autofix 的 'Publish PR' 需要同样的修复(见该处的对应评论)。
— qwen3.8-max via Qwen Code /review (v0.21.6)
There was a problem hiding this comment.
Addressed: same as the 'Publish PR' note — the push uses an inline https://x-access-token URL and never persists the PAT to .git/config (no git remote set-url anywhere), and http.extraheader is unset.
| case "${RUNNER_ENVIRONMENT}" in | ||
| github-hosted) ;; | ||
| github-hosted|self-hosted) ;; |
There was a problem hiding this comment.
[Suggestion] R1-7: The widened runner-environment guard — the change that makes ECS-routed runs pass this check at all — is pinned by no test (identical change at line 2829). Probe: reverting both case patterns to the exact pre-PR text (github-hosted) ;;) leaves the contract suite 112/112 green.
Failure scenario: a future edit reverting this pattern makes every ECS-routed issue-autofix and review-address run die at 'Check runner environment' with "Unsupported runner environment: self-hosted" — silently re-killing the entire routing this PR introduces while the suite stays green (it pins only that the step receives RUNNER_ENVIRONMENT and that runner.environment == 'self-hosted' appears nowhere).
Suggested fix: in the ECS test, assert the accepted set, e.g.
expect(
workflow.match(
/case "\$\{RUNNER_ENVIRONMENT\}" in\n\s+github-hosted\|self-hosted\) ;;/g,
),
).toHaveLength(2);中文说明
放宽后的 runner 环境检查——正是让 ECS 路由的运行能通过此检查的改动——没有任何测试钉住(第 2829 行有相同改动)。探针验证:把两处 case 模式恢复为 PR 前的原文(github-hosted) ;;),契约测试套件仍为 112/112 全绿。
失败场景:未来某次编辑把该模式改回去,会让每一次 ECS 路由的 issue-autofix 和 review-address 运行在 'Check runner environment' 处以 "Unsupported runner environment: self-hosted" 失败——在套件保持全绿的情况下,悄悄废掉本 PR 引入的全部路由(套件只钉住该步骤接收 RUNNER_ENVIRONMENT,以及 runner.environment == 'self-hosted' 不出现)。
修复建议:在 ECS 测试中断言接受的集合,例如上面的代码。
— qwen3.8-max via Qwen Code /review (v0.21.6)
There was a problem hiding this comment.
Addressed: the contract test now pins the accepted set — it asserts exactly two github-hosted|self-hosted) ;; case patterns (one per job that carries the step; build-cli has none). Reverting either to the hosted-only pattern now fails the suite.
| workflow.match(/- name: 'Set up Node.js'[\s\S]*?(?=\n[ ]{6}- name: ')/g) ?? | ||
| []; |
There was a problem hiding this comment.
[Suggestion] R1-8: The removal of the if: runner.environment == 'github-hosted' condition from all three Node-setup steps (so setup-node provisions Node 22.x on the ECS pool too) is not pinned — this capture is consumed only by assertions on what the steps contain, never that they are unconditional. Probe: re-adding the condition to all three steps leaves npm run test:scripts at 942 passed / 0 failed, while a comparator mutation (node-version: '22.x' → '20.x') fails — the suite discriminates the recipe but not the conditionality.
Failure scenario: a future edit re-adding the condition skips setup-node on every ECS-routed run, leaving the three heavy jobs dependent on whatever Node the pool image happens to ship instead of the guaranteed 22.x — while the suite stays green (toHaveLength(3) and the recipe toContain assertions are all satisfied by the conditional step).
Suggested fix: in keeps the Node setup recipe identical across the autofix jobs, add per captured step:
expect(step).not.toContain("runner.environment == 'github-hosted'");中文说明
从三个 Node 安装步骤移除 if: runner.environment == 'github-hosted' 条件(使 setup-node 也在 ECS 池上提供 Node 22.x)这一改动没有被钉住——此捕获只被"步骤包含什么"的断言消费,从未断言它们是无条件的。探针验证:给三个步骤重新加回该条件,npm run test:scripts 仍为 942 过 / 0 败;而对照突变(node-version: '22.x' → '20.x')会失败——套件能区分配方,却不能区分条件性。
失败场景:未来某次编辑重新加回该条件,会使每次 ECS 路由运行跳过 setup-node,三个重型 job 只能依赖池镜像恰好自带的 Node,而非有保证的 22.x——同时套件保持全绿(toHaveLength(3) 与配方的 toContain 断言对带条件的步骤同样满足)。
修复建议:在 keeps the Node setup recipe identical across the autofix jobs 中,对每个捕获的步骤追加上面的断言。
— qwen3.8-max via Qwen Code /review (v0.21.6)
There was a problem hiding this comment.
Addressed: the Node-recipe test now asserts each of the three setup steps does NOT contain runner.environment == 'github-hosted', so re-adding the hosted-only if fails the suite.
| const heavyJobRunsOn = { | ||
| 'issue-autofix': | ||
| workflow.match( | ||
| /\n {2}issue-autofix:[\s\S]*?(?=\n[ ]{2}# ==========)/, | ||
| )?.[0] ?? '', |
There was a problem hiding this comment.
[Suggestion] R1-9: These three job-block extractions duplicate regexes that already exist as live constants in this same file: issueAutofixJob (lines 50-52, asserted at 1759-1762/1896) is byte-identical to the issue-autofix extraction here, and buildCliJob (5344-5346) / addressJob (copies at 1291 and 5349) are byte-identical to the build-cli / review-address extractions.
Failure scenario: a structural change to the workflow (the # ========== banner after issue-autofix renamed, or a job appended after review-address — the EOF anchor documented at line 1288 only works while it is the last job) must now be propagated across 2-3 independent regex copies per job; a partially-updated or silently widened slice keeps toContain(ecsRunsOn) green while pinning less than it claims.
Suggested fix: hoist buildCliJob/addressJob to file-scope constants alongside issueAutofixJob and reuse them in both tests:
const heavyJobRunsOn = {
'issue-autofix': issueAutofixJob,
'build-cli': buildCliJob,
'review-address': addressJob,
};中文说明
这三个 job 块提取重复了本文件中已存在且仍在使用的正则:issueAutofixJob(第 50-52 行,在 1759-1762/1896 处被断言)与此处的 issue-autofix 提取逐字节相同;buildCliJob(5344-5346)/addressJob(1291 与 5349 两份拷贝)与 build-cli/review-address 提取逐字节相同。
失败场景:workflow 的结构性变化(issue-autofix 后的 # ========== 横幅被改名,或 review-address 之后追加新 job——第 1288 行注释说明 EOF 锚点只在它是最后一个 job 时成立)现在必须传播到每个 job 的 2-3 份独立正则拷贝;部分更新或被悄悄放宽的切片仍能让 toContain(ecsRunsOn) 保持绿色,实际钉住的范围却小于其声称。
修复建议:把 buildCliJob/addressJob 提升为文件级常量,与 issueAutofixJob 并列,并在两个测试中复用(见上面的代码)。
— qwen3.8-max via Qwen Code /review (v0.21.6)
There was a problem hiding this comment.
Addressed: heavyJobRunsOn now reuses the module-level extractions issueAutofixJob / buildCliJob / reviewAddressJob instead of duplicating the job-block regexes.
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
Review —
|
- 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-<run_id>): 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.
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： 🤖 Addressed round-1 review feedback on the ECS routing change. All four Critical findings and all five Suggestions (six inline comments) are resolved in code; nothing is declined or deferred. [Critical] R1-1 — shared-pool workspace ownership (rc:3723852612): added the fleet's [Critical] R1-2 — planted git exec knobs in the reused workspace (rc:3723852618): added a [Critical] R1-3 — shared [Critical] R1-4 — stale [Suggestion] R1-5 — fork-PR events routed to ECS / wrong comment (rc:3723852640): implemented the guard option (not just a comment fix). The runs-on expression now requires, for [Suggestion] R1-6 (twin comments rc:3723852645, rc:3723852653) — bot PAT persisted in the origin remote URL on the shared pool: [Suggestion] R1-7 — widened runner-environment guard unpinned (rc:3723852657): the contract test now asserts the accepted set [Suggestion] R1-8 — conditional Node setup unpinned (rc:3723852660): the Node-setup lockstep test now asserts each captured step does NOT contain [Suggestion] R1-9 — duplicated job-block regexes (rc:3723852665): Failed check — Conflict notes: no base conflict ( VerificationCommands actually run, on the final committed tree (
中文说明🤖 已处理 ECS 路由变更的第 1 轮评审反馈。四条 Critical 与五条 Suggestion（六条行内评论）全部在代码中解决；没有拒绝或搁置项。 [Critical] R1-1——共享池工作区属主问题（rc:3723852612）：在三个 ECS 路由 job（ [Critical] R1-2——复用工作区中被埋入的 git 执行开关（rc:3723852618）：在 [Critical] R1-3——并发的 issue 阶段运行共享 [Critical] R1-4——复用 [Suggestion] R1-5——fork PR 事件被路由到 ECS／注释错误（rc:3723852640）：采用守卫方案（而非仅改注释）。runs-on 表达式现在要求 [Suggestion] R1-6（成对评论 rc:3723852645、rc:3723852653）——bot PAT 残留在共享池的 origin 远端 URL 中： [Suggestion] R1-7——放宽后的 runner 环境守卫无测试钉住（rc:3723852657）：契约测试现断言接受集合 [Suggestion] R1-8——条件化 Node 安装无测试钉住（rc:3723852660）：Node 安装同步测试现断言每个捕获的步骤不包含 [Suggestion] R1-9——重复的 job 块正则（rc:3723852665）： 失败检查—— 冲突说明：无基分支冲突（ 验证实际执行的命令，除注明外均在最终提交树（
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
Review:
|
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM, no blockers. The move to the ECS pool is properly guarded: runs-on routes self-hosted only for same-repo + (non-PR-family or same-repo head or write+ author), mirroring ci.yml classify_pr, so external/fork PR code stays on hosted; review-address targets are write+-gated. The self-hosted hardening is correct and thoughtful: git-config allowlist sanitize removes planted exec knobs (core.hooksPath/fsmonitor/filter/diff.external/includeIf) and sweeps symlinked hooks; push uses a transient token URL instead of persisting the PAT to .git/config on the shared host; per-run WORKDIR (/tmp/autofix-<run_id>) stops concurrent-run clobbering; stale autofix branches dropped. Non-blocking P3s: (1) 'Restore workspace ownership' sudo -n fallback will just warn on pools without NOPASSWD sudo (same gap as the hk update incident) — checkout may then fail on root-owned leftovers; (2) the transient push token is briefly visible in the process list (Actions masks logs) — acceptable, note it.
…-ecs # Conflicts: # scripts/tests/qwen-autofix-workflow.test.js
…tion 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.
…/qwen-code into ci/autofix-heavy-jobs-ecs
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： AutoFix review round — ECS routing hardening (round 2)Addressed both review comments: the full initial review and the follow-up on Blocking items (follow-up review)1. Sanitize step was the bypassable trusted-lane variant — fixed. 2. 3. No contract tests for the hygiene steps — fixed. New tests pin, per Non-blocking items done in the same PR
Declined / deferred (with reasons)
VerificationCommands actually run in this round (on the committed state):
中文说明AutoFix 评审轮次 —— ECS 路由加固（第 2 轮）本轮处理了两条评审意见：最初的完整评审，以及针对加固 commit 的后续评审。三个 blocking 项全部完成；非 blocking 的卫生（hygiene）项也一并完成。Commit： Blocking 项（后续评审）1. sanitize 步骤用的是可被绕过的 trusted-lane 变体 —— 已修复。 2. 3. 卫生步骤没有契约测试 —— 已修复。 新测试钉住：每个重任务 job 里 ownership restore 和 sanitize 都存在，且都排在它们所保护的 checkout 之前；共享 action 的加固顺序（worktree-config → unset extension → 清扫 → hooks）与封闭性（免疫 global 配置的解析、只在 git 目录内删除、unlink 分支），与 同一 PR 中一并完成的非 blocking 项
拒绝 / 延后（附理由）
验证本轮实际执行的命令（针对提交后的状态）：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
Review —
|
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.
|
🔴 Blocking issue fixed in What was wrong: the sanitize logic ran as a local composite action ( Fix (matches your suggestion):
Verified: 115 tests in the autofix contract file, 945 across Medium / Minor (tmp-leak age sweep, credential-helper instead of argv PAT, 🔴 阻塞问题已在 |
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.
|
🟡 Medium security items fixed in PAT no longer in The token enters neither the reused workspace's Pool assertion. /tmp + .git hygiene. Verified: 115 autofix contract tests, 945 across Left as follow-ups (Minor/style, non-blocking): 🟡 Medium 安全项已在 |
Review —
|
| Claim | Result |
|---|---|
ecs-qwen-* runner-name assertion matches the real pool |
✅ every member is ecs-qwen-runner-64c-N (gh api /repos/QwenLM/qwen-code/actions/runners) — the new hard-fail won't kill every ECS run |
| Routing expression precedence | ✅ && binds tighter than ||, so it reads (repo && !disabled && (non-PR-event || same-repo-head || write+)) && ecs || hosted; same shape as qwen-triage.yml:347 |
BRANCH_PREFIX reachable in Drop stale autofix branches |
✅ workflow-level env (L100) — the sweep can't degenerate to refs/heads/* |
/tmp/autofix-review-<pr> left PR-scoped (not run-scoped) |
✅ safe — qwen-pr-head-write-<pr> with cancel-in-progress: false serialises every writer of that PR |
24 h -mmin floor can't reap a live workdir |
✅ above both caps (180 / 300 min) |
stale=true early-exit path |
✅ gates all downstream steps (steps.prepare.outputs.stale != 'true'), so the agent step doesn't run after it |
🔴 High — the transient credential helper is not transient on error paths
The helper is written into the reused workspace's .git/config, and the matching --unset-all is skipped on every failure path:
Publish PR(L1457):git pushfailing aborts the step underbash -e, before the L1459 unset.Prepare branch and feedback(L3401): the preflight-failure branchexit 0s at L3403–3411 and never reaches the L3412 unset — and that is the expected path for a fork PR without allow-edits, not a rare one.Push and report(L4305): fourexit 1paths inside the retry loop bypass the L4360 unset.
What's left behind is a !sh helper that answers any host and any protocol with password=${GITHUB_TOKEN}, sitting in _work/qwen-code/qwen-code/.git/config — which on a self-hosted runner is shared with every other workflow that lands on that machine. The step comment says "credential.* is off the sanitize allowlist, so a leftover helper is swept next run", and that's true — but only for these three autofix jobs. ci.yml and qwen-code-pr-review.yml land in the same workspace and sweep nothing.
Exploitability today is low (checkout authenticates via http.*.extraheader, so the helper is rarely consulted), but the fix is smaller than the caveat:
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}"-c never touches .git/config, argv still holds only the literal ${GITHUB_TOKEN} (the helper's own shell expands it from env), and host-scoping means it can't answer a non-GitHub URL. For the retry loop, wrap it once — git_auth() { git -c credential."https://github.com".helper='…' "$@"; } — and call git_auth push / git_auth fetch. If you'd rather keep git config, then at minimum add trap 'git config --local --unset-all credential.helper || true' EXIT immediately after setting it, which covers all three sites.
🟠 Medium
1. setup-node + cache: 'npm' on ECS contradicts ci.yml on the same pool. ci.yml:246–264 deliberately splits: hosted gets actions/setup-node with the npm cache, self-hosted gets Use pre-installed Node.js (self-hosted) plus Configure persistent npm cache (self-hosted) writing NPM_CONFIG_CACHE=${HOME}/.cache/qwen-code/npm. This PR makes setup-node unconditional with cache: 'npm' on that same pool, so the ECS runs now round-trip a large tarball through the Actions cache service, and the restore into ~/.npm can collide with a concurrent job on the same host. Mirroring ci.yml's split (or just dropping cache: 'npm' on the self-hosted branch) keeps one fleet policy instead of two.
2. Capacity — this relocates the saturation rather than removing it. The pool has 19 online ecs-qwen runners, and ci.yml routes every same-repo PR build onto the same label. After this change heavy autofix can park 1 (issue) + 1 (build-cli) + 5 (address legs @ timeout-minutes: 300) = up to 7 of 19 for as long as 5 h each — the hosted-pool shape, minus hosted's elastic overflow. There are runners carrying ecs without ecs-qwen (…‑26/27/28/29/32/33); routing the agent jobs onto a disjoint label would make it structurally impossible for them to starve CI. If you'd rather not re-label now, please at least watch ECS queue depth (not just disk/health) on the first few scans.
3. Sanitize allowlist keeps all of remote. and extensions.. The block's stated criterion is "only plumbing that carries no command", but remote.<n>.uploadpack / receivepack are command strings and remote.<n>.url accepts the ext::<cmd> transport. actions/checkout re-establishes only remote.origin.*, so a planted remote.evil.url = ext::sh -c … survives the sweep. Nothing in this workflow fetches a non-origin remote today, so it's a hole in the invariant rather than a live vector — narrowing to remote\.[^.]+\.(url|fetch|pushurl) and rejecting non-https:// URLs would close it.
4. The hardening test covers 1 of the 3 sanitize copies. sanitizeStep is extracted from issueAutofixJob only, so hardens the shared git-config sanitize action against the verified bypasses stays green even if build-cli's or review-address's copy silently loses GIT_CONFIG_GLOBAL=/dev/null or the worktree-config ordering. That's ~45 lines of security-critical shell duplicated 3× here (plus a 4th near-copy in qwen-triage.yml) — please loop the hardening assertions over all three slices, or simply assert the three blocks are byte-identical. Related: the comments say "The composite action keeps a known-safe allowlist" and "the shared git-config sanitize action" — there is no composite action; it's inlined (correctly so, since it must precede checkout). Worth rewording so the next reader doesn't go looking for it.
🟡 Low
rm -rffallback contradicts its own comment. The else branch'srm -f "$RAW_HOOKS" 2>/dev/null || rm -rf "$RAW_HOOKS"does exactly the "recursive delete of a planted path" the comment two lines above calls "far worse". Separately, ifhooksPathever resolves to the git dir itself, the$GIT_DIR_ABS/prefix test fails and the fallbackrm -rfs the whole.git. Dropping therm -rfand warning instead matches the stated intent.- The
/tmpsweep misses the dir it replaces.-name 'autofix-*'doesn't match the legacy fixed/tmp/autofix, so exactly one stale dir per runner lingers forever.-name 'autofix*'covers it. git gc --prune=nowruns unconditionally on every heavy job.--prune=nowforces a full repack rather than the threshold-driven--auto;git gc --auto --prune=nowgets the reclamation without paying a repack on every run of up to 7 concurrent jobs.- Comment nit: "a cancelled run leaves the dir to the next run's reset" —
if: always()does run on cancellation within the grace period, so the cleanup normally fires. - PR body says the contract file passes "112 tests"; it's 115 on the head.
Nice work
The parts that are easy to get wrong are right: Restore workspace ownership and Sanitize workspace git config both precede the checkout they protect (and are pinned that way by test), the sweep is allowlist-shaped rather than denylist-shaped, hooks resolve with GIT_CONFIG_GLOBAL/GIT_CONFIG_SYSTEM neutralised, -type f -o -type l catches symlinked hooks, the docker info preflight fails in seconds instead of after npm ci, sudo -n can't hang on a password prompt, and the runner.name assertion is a genuine second lock behind the label routing. The negative assertions in the test (not.toContain("runner.environment == 'github-hosted'"), not.toContain('sudo apt-get'), short jobs pinned to ubuntu-latest) are the right shape — they fail on the specific regressions this change makes possible.
中文说明
总体结论
方向正确,最难的部分(持久 workspace 的卫生处理)做得很细致。合并前建议修 1 个问题,另有 2 个容量/一致性决策值得现在就定。契约测试在 PR head 上全绿(115 passed)。
已实测验证(而非假设)的点
| 论断 | 结果 |
|---|---|
ecs-qwen-* runner 名断言与真实池子一致 |
✅ 所有成员都是 ecs-qwen-runner-64c-N(gh api /repos/QwenLM/qwen-code/actions/runners)——新增的硬失败不会打死所有 ECS 运行 |
| 路由表达式优先级 | ✅ && 优先于 ||,实际语义是 (repo && !disabled && (非 PR 事件 || 同仓 head || write+)) && ecs || hosted;与 qwen-triage.yml:347 同型 |
Drop stale autofix branches 里 BRANCH_PREFIX 可见 |
✅ 定义在 workflow 级 env(L100)——sweep 不会退化成 refs/heads/* |
/tmp/autofix-review-<pr> 仍是 PR 维度(非 run 维度) |
✅ 安全——qwen-pr-head-write-<pr> 且 cancel-in-progress: false,串行化了该 PR 的所有写入方 |
24 小时 -mmin 下限不会误删活跃 workdir |
✅ 高于两个 job 上限(180 / 300 分钟) |
stale=true 提前退出路径 |
✅ 后续所有步骤都被 steps.prepare.outputs.stale != 'true' 挡掉,agent 步骤不会在其后执行 |
🔴 High —— 临时 credential helper 在错误路径上并不"临时"
helper 被写进复用的 workspace 的 .git/config,而对应的 --unset-all 在每条失败路径上都被跳过:
Publish PR(L1457):git push失败会在bash -e下中止步骤,走不到 L1459 的 unset。Prepare branch and feedback(L3401): preflight 失败分支在 L3403–3411 直接exit 0,永远到不了 L3412 的 unset——而对于没勾 allow-edits 的 fork PR,这是预期路径,不是罕见路径。Push and report(L4305): 重试循环里的 4 条exit 1都绕过了 L4360 的 unset。
残留下来的是一个对任意 host、任意协议都回答 password=${GITHUB_TOKEN} 的 !sh helper,躺在 _work/qwen-code/qwen-code/.git/config 里——在自托管 runner 上,这个目录被落到同一台机器的所有其他 workflow 共享。步骤注释写的"credential.* 不在 sanitize 白名单里,所以下次运行会被清掉"是对的,但只对这三个 autofix job 成立;ci.yml 和 qwen-code-pr-review.yml 会落到同一个 workspace,而它们什么都不清。
目前可利用性不高(checkout 走 http.*.extraheader 认证,helper 很少被调用),但修复比这段免责说明还短:
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}"-c 完全不碰 .git/config;argv 里依旧只有字面量 ${GITHUB_TOKEN}(由 helper 自己的 shell 从环境展开);按 host 限定则保证它不会应答非 GitHub 的 URL。重试循环里包一层即可——git_auth() { git -c credential."https://github.com".helper='…' "$@"; },然后调用 git_auth push / git_auth fetch。如果更想保留 git config 写法,那至少在设置之后紧跟 trap 'git config --local --unset-all credential.helper || true' EXIT,一次覆盖三处。
🟠 Medium
1. ECS 上的 setup-node + cache: 'npm' 与同一池子上的 ci.yml 相矛盾。 ci.yml:246–264 是刻意拆开的:托管用 actions/setup-node 带 npm 缓存,自托管用 Use pre-installed Node.js (self-hosted) 加 Configure persistent npm cache (self-hosted)(写 NPM_CONFIG_CACHE=${HOME}/.cache/qwen-code/npm)。本 PR 在同一池子上把 setup-node 变成无条件且带 cache: 'npm',于是 ECS 运行要通过 Actions 缓存服务来回搬一个大 tarball,而其向 ~/.npm 的 restore 还可能与同机并发 job 相撞。沿用 ci.yml 的拆分(或仅在自托管分支去掉 cache: 'npm')可以保持一套 fleet 策略而不是两套。
2. 容量 —— 这是把饱和搬了个地方,而不是消除它。 池子有 19 台在线 ecs-qwen runner,而 ci.yml 把所有同仓 PR 构建也路由到同一标签。改动后,重型 autofix 可以占住 1(issue)+ 1(build-cli)+ 5(address leg,timeout-minutes: 300)= 19 台里最多 7 台,每台最长 5 小时——正是托管池的那个形状,而且没有托管池的弹性溢出。池子里存在带 ecs 但不带 ecs-qwen 的 runner(…‑26/27/28/29/32/33);把 agent job 路由到一个不相交的标签,可以从结构上杜绝它们饿死 CI。若暂不想改标签,合并后头几次扫描请至少同时盯 ECS 的排队深度(不只是磁盘/健康)。
3. sanitize 白名单整段保留了 remote. 和 extensions.。 这段代码自述的标准是"只保留不携带命令的管道配置",但 remote.<n>.uploadpack / receivepack 本身就是命令字符串,而 remote.<n>.url 接受 ext::<cmd> 传输方式。actions/checkout 只会重建 remote.origin.*,因此被埋的 remote.evil.url = ext::sh -c … 能活过这次清扫。当前 workflow 里没有任何步骤会 fetch 非 origin 的 remote,所以这是"不变式有洞"而非可用攻击面——收紧为 remote\.[^.]+\.(url|fetch|pushurl) 并拒绝非 https:// 的 URL 即可闭合。
4. 加固测试只覆盖了 3 份 sanitize 副本中的 1 份。 sanitizeStep 只从 issueAutofixJob 里抽取,因此即便 build-cli 或 review-address 的副本悄悄丢掉 GIT_CONFIG_GLOBAL=/dev/null 或 worktree-config 的顺序,hardens the shared git-config sanitize action against the verified bypasses 仍然绿。这是约 45 行安全关键 shell 在本文件里复制了 3 份(qwen-triage.yml 里还有第 4 份近似副本)——建议把加固断言对三个切片都跑一遍,或干脆断言这三段逐字节相同。相关地:注释里写的 "The composite action keeps a known-safe allowlist"、"the shared git-config sanitize action" 并不存在——它是内联的(而且内联是对的,因为必须先于 checkout)。建议改写措辞,免得后来者去找一个不存在的 composite action。
🟡 Low
rm -rf兜底与自己的注释相矛盾。 else 分支的rm -f "$RAW_HOOKS" 2>/dev/null || rm -rf "$RAW_HOOKS"做的正是上面两行注释里称为"糟糕得多"的"对被埋路径递归删除"。另外,如果hooksPath恰好解析到 git dir 自身,$GIT_DIR_ABS/前缀判断会失败,兜底的rm -rf会把整个.git删掉。去掉rm -rf只告警,更贴合原意。/tmp清扫漏掉了它所替换的那个目录。-name 'autofix-*'匹配不到旧的固定路径/tmp/autofix,于是每台 runner 会永久留下一个陈旧目录。-name 'autofix*'即可覆盖。git gc --prune=now在每个重型 job 上无条件执行。--prune=now会强制完整 repack,而不是按阈值触发的--auto;git gc --auto --prune=now同样能回收磁盘,却不必在最多 7 个并发 job 的每次运行上都付一次 repack。- 注释小问题: "a cancelled run leaves the dir to the next run's reset" ——
if: always()在取消时(宽限期内)是会执行的,所以清理步骤通常仍会跑。 - PR 正文写契约文件通过 "112 tests",在 head 上实际是 115。
做得好的地方
容易做错的部分都做对了:Restore workspace ownership 和 Sanitize workspace git config 都排在它们所保护的 checkout 之前(并被测试钉死顺序);清扫用白名单而非黑名单;hooks 解析时中和了 GIT_CONFIG_GLOBAL/GIT_CONFIG_SYSTEM;-type f -o -type l 能抓到符号链接形式的 hook;docker info 预检把失败提前到几秒内而不是 npm ci 之后;sudo -n 不会卡在密码提示上;runner.name 断言是标签路由之外真正独立的第二道锁。测试里的反向断言(not.toContain("runner.environment == 'github-hosted'")、not.toContain('sudo apt-get')、短 job 钉死 ubuntu-latest)形状也对——它们恰好会在本次改动新引入的那几种回归上失败。
…eview fixes 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.<n>.(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.
|
R4 items addressed in 🔴 High — credential helper is now genuinely transient (your exact fix). All three push paths ( The salvage loop wraps it once as 🟠 Medium 3 — allowlist narrowed. The sanitize sweep's 🟠 Medium 4 — coverage fixed. The hardening test now asserts all three inlined sanitize copies are byte-identical and runs its assertions on them (previously one of three), renamed to reflect that it's inlined, and the stale "composite action" comments are reworded. 🟡 Low — all done: dropped the recursive Deferred as decisions, not code (flagging, not dropping):
Verified: 115 autofix contract tests, 945 across R4 已在 |
…/qwen-code into ci/autofix-heavy-jobs-ecs
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #8603Outcome: the two remaining small Minor items are implemented; all prior-round fixes re-verified; one Minor item deferred and one declined with evidence. This round triaged the full review posted after the last evaluation. The blocking and medium findings had already been fixed by earlier commits on this branch; that was re-verified against the current tree rather than assumed. No base merge was performed ( Feedback triage🔴 Blocking — sanitize composite action invoked before 🟡 Medium (3 items) — already fixed in
Inline threads (10) — every "Addressed in …" claim was re-verified against the current tree and holds: ownership restore ×3 jobs, inline sanitize ×3 jobs, per-run 🟢 Minor items:
Pre-merge check — the suggested Verification
Note: the first full 中文说明Autofix 轮次总结 — PR #8603结果:实现了剩余的两个小的 Minor 项;重新验证了此前各轮的修复;一项 Minor 推迟处理,一项附证据拒绝。 本轮对上次评估之后发布的完整 review 做了分类处理。阻塞级和中等级问题此前已由本分支上的提交修复;本轮对照当前代码树重新核实,而非凭空采信。未执行 base 合并( 反馈分类🔴 阻塞 — sanitize composite action 在 🟡 中等(3 项) — 已在
内联线程(10 条) — 每条 “Addressed in …” 的声明都对照当前代码树重新核实且成立:三个 job 的所有权恢复、三个 job 的内联 sanitize、按 run 隔离的 🟢 次要项:
合并前验证 — 建议的 验证
说明:首次完整运行 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
Round 2 —
|
| Round 1 | Status |
|---|---|
| 🔴 H1 credential helper stranded on error paths | Closed. All three sites are now one-shot git -c credential."https://github.com".helper=…, with a git_auth wrapper covering the retry loop's push and fetch. I checked the key parses as intended (section=credential, subsection=https://github.com, key=helper) and the URL scope matches both https://github.com/${REPO}.git and the fork URL. No git config --local credential.helper remains, so there is no error path left to strand one — and the test pins its absence. |
🟠 M3 remote. allowlisted wholesale |
Closed, and better than I asked for. Narrowing to remote\.[^.]+\.(url|fetch|pushurl) also sweeps remote.origin.uploadpack, which closes the one consumer I was worried about (actions/checkout's own git fetch origin). I grepped the head: no git command in the workflow consumes a named remote — every network op passes an explicit https://github.com/… URL — so the residual ext:: URL vector has nothing left to fire it. My "also validate the URL scheme" suggestion is unnecessary given that. |
| 🟠 M4 hardening test covered 1 of 3 sanitize copies | Closed, more strongly than suggested. sanitizeSteps[1]/[2] pinned byte-identical to [0], so every assertion in that test now covers all three copies rather than just issue-autofix's. |
🟡 L1 rm -rf fallback contradicted its comment |
Correct. rm -f on a directory fails → warning, planted path left alone, core.hooksPath unset from local config, empty .git/hooks restored. The hooksPath == git dir case now warns instead of wiping .git. |
🟡 L2 /tmp sweep missed the legacy dir |
Closed — -name 'autofix*'. |
🟡 L4 always()-on-cancellation comment |
Closed, and the replacement text is more precise than what I asked for — it now names the one case that really does abandon a dir (hard runner kill) and which reclaim path covers it. |
| Misleading "composite action" comments | Fixed in all copies. |
Unprompted and good: (umask 077; mkdir -p "${WORKDIR}") replacing mkdir + chmod 700. That closes a TOCTOU window I missed — on a world-writable shared /tmp the old form left the dir at 0755 between the two calls, long enough for another tenant to open a handle inside it. The subshell keeps the umask from leaking into the rest of the step, and the test pins not.toContain('chmod 700') so it can't regress.
🔴 Still open — git gc --auto detaches, and --prune=now is unsafe concurrently
git gc --auto --prune=now --quiet 2>/dev/null || true # L793, L3150gc.autoDetach defaults to true, so adding --auto changed gc from synchronous to backgrounded:
gc.autoDetach — Make
git gc --autoreturn immediately and run in the background if the system supports it. Default is true. —git-config(1)
And that is exactly the combination git warns about:
--prune=nowprunes loose objects regardless of their age and increases the risk of corruption if another process is writing to the repository concurrently; see "NOTES" below. —git-gc(1)
Before 243cde4609 the gc owned the repo for the duration of its own step — no other git process was running, so --prune=now was safe. Now it detaches and races everything that follows in a job with timeout-minutes: 300: the PR-branch checkout, the agent's own commits across the whole run, and the merge/retry loop. The 2-week gc.pruneExpire margin that normally makes a concurrent gc tolerable is precisely what --prune=now removes.
This is my fault for suggesting --auto --prune=now without noting the detach. One flag keeps both properties:
git -c gc.autoDetach=false gc --auto --prune=now --quiet 2>/dev/null || true--auto still means it usually no-ops — it fires around gc.autoPackLimit (~50 packs, so roughly every ~50 runs on a fetch-depth: 1 workspace) — so forcing it synchronous costs nothing on the common path and only pays the repack on the run that actually needed one.
🟡 Minor — two assertions lost their git anchor
expect(workflow).toContain('push --no-verify --dry-run "https://github.com/${HEAD_REPO}.git" HEAD:"${BRANCH}"');
expect(publishPrStep).toContain('push --no-verify "https://github.com/${REPO}.git" "${BRANCH}"');A regression back to a bare git push … with no -c credential… prefix still satisfies both, and the companion toContain('credential."https://github.com".helper') only proves the helper appears somewhere in the step — not that it is on that push. pushAndReportStep is pinned properly (/if git_auth push …/); these two could get the same treatment, e.g. matching the two-line form or asserting the git -c credential."https://github.com".helper prefix immediately precedes.
↩️ Correction to my round-1 M2
I wrote that runners carrying ecs without ecs-qwen could serve as a disjoint pool for the agent jobs. That's wrong: ecs-qwen ⊂ ecs. Online labels today are 15× ecs-qwen,ecs, 4× ecs-qwen,ecs,ecs-update-64c, 6× ecs — so routing to ecs is a superset, not isolation. Isolating heavy autofix from CI would need a new label on the runner side, which is outside this PR. The only in-repo lever is max-parallel, and its current 5 is a documented measured choice, so I'd leave it and just watch ECS queue depth on the first scans after merge — as your risk section already proposes.
Still open from round 1 (noting, not re-litigating)
- 🟠 M1 —
setup-node+cache: 'npm'on ECS vsci.yml:246–264's deliberateNPM_CONFIG_CACHE=${HOME}/.cache/qwen-code/npmsplit on the same pool. - 🟡 L5 — PR body still says the contract file passes "112 tests"; it's 115.
Everything else from round 1 is resolved. With the gc.autoDetach=false fix this is merge-ready from my side.
中文说明
第 2 轮 —— a0e88fc036 → 3be747dd29
复核了两次增量(243cde4609 + 17ddaf6af3)。head 上契约套件全绿(115 passed)。还剩一项未解决,而且是我第 1 轮的建议引入的 —— 下面如实说明。
✅ 已确认修复
| 第 1 轮 | 状态 |
|---|---|
| 🔴 H1 credential helper 在错误路径上残留 | 已闭合。 三处都改成一次性的 git -c credential."https://github.com".helper=…,重试循环用 git_auth 包装器同时覆盖 push 和 fetch。我核对了这个 key 的解析符合预期(section=credential、subsection=https://github.com、key=helper),URL 作用域对 https://github.com/${REPO}.git 和 fork URL 都匹配。代码里已无 git config --local credential.helper,因此不存在会残留它的错误路径——测试也钉住了它的消失。 |
🟠 M3 remote. 整段进白名单 |
已闭合,而且比我要求的更彻底。 收紧为 remote\.[^.]+\.(url|fetch|pushurl) 顺带把 remote.origin.uploadpack 也清掉了,这恰好堵住了我担心的那个唯一消费者(actions/checkout 自己的 git fetch origin)。我在 head 上 grep 过:workflow 里没有任何 git 命令消费具名 remote——所有网络操作都传显式 https://github.com/… URL——所以残留的 ext:: URL 向量已无处触发。我提的"再校验 URL scheme"因此没必要。 |
| 🟠 M4 加固测试只覆盖 3 份副本中的 1 份 | 已闭合,比建议的更强。 sanitizeSteps[1]/[2] 被钉为与 [0] 逐字节相同,该测试里的每条断言现在覆盖全部三份副本,而不只是 issue-autofix 那份。 |
🟡 L1 rm -rf 兜底与注释矛盾 |
正确。 对目录执行 rm -f 会失败 → 告警,被埋路径原样保留,core.hooksPath 从 local config 移除,空的 .git/hooks 被重建。hooksPath == git dir 的情况现在只告警,不再抹掉 .git。 |
🟡 L2 /tmp 清扫漏掉旧目录 |
已闭合 —— -name 'autofix*'。 |
🟡 L4 always() 与取消的注释 |
已闭合,而且替换后的措辞比我要求的更精确——它点明了真正会遗留目录的那一种情况(runner 硬杀),以及由哪条回收路径兜底。 |
| 误导性的 "composite action" 注释 | 三份副本均已修正。 |
主动做的、且做得好: 用 (umask 077; mkdir -p "${WORKDIR}") 取代 mkdir + chmod 700。这堵上了我漏掉的一个 TOCTOU 窗口——在全局可写的共享 /tmp 上,旧写法会让目录在两条命令之间短暂处于 0755,足够另一个租户在其中拿到句柄。子 shell 保证 umask 不会泄漏到该步骤的其余部分,测试也用 not.toContain('chmod 700') 钉住了不可回退。
🔴 仍未解决 —— git gc --auto 会 detach,而 --prune=now 在并发下不安全
git gc --auto --prune=now --quiet 2>/dev/null || true # L793、L3150gc.autoDetach 默认为 true,因此加上 --auto 把 gc 从同步变成了后台执行:
gc.autoDetach —— 让
git gc --auto立即返回并在后台运行(若系统支持)。默认为 true。 ——git-config(1)
而这正是 git 明确警告的组合:
--prune=now会无视对象年龄一律裁剪 loose object,并在另一进程并发写入仓库时提高损坏风险;参见下文 "NOTES"。 ——git-gc(1)
在 243cde4609 之前,gc 是同步的——它在自己那一步里独占仓库,没有其他 git 进程在跑,所以 --prune=now 是安全的。现在它 detach 后会与该 job 后续的一切并发,而这个 job 的 timeout-minutes: 300:PR 分支 checkout、agent 在整个运行期内自己的提交、以及 merge/重试循环。让并发 gc 通常还能容忍的那个两周 gc.pruneExpire 边界,恰恰就是 --prune=now 去掉的东西。
这是我建议 --auto --prune=now 时没提 detach 造成的。加一个 flag 即可两者兼得:
git -c gc.autoDetach=false gc --auto --prune=now --quiet 2>/dev/null || true--auto 依旧意味着它多数时候空转——大约在 gc.autoPackLimit(约 50 个 pack,即 fetch-depth: 1 的 workspace 上大致每 ~50 次运行)才触发一次——所以强制同步在常见路径上不花任何代价,只有真正需要 repack 的那一次才付出成本。
🟡 小问题 —— 两条断言丢了 git 锚点
expect(workflow).toContain('push --no-verify --dry-run "https://github.com/${HEAD_REPO}.git" HEAD:"${BRANCH}"');
expect(publishPrStep).toContain('push --no-verify "https://github.com/${REPO}.git" "${BRANCH}"');若回退成不带 -c credential… 前缀的裸 git push …,这两条仍然通过;而配套的 toContain('credential."https://github.com".helper') 只能证明 helper 出现在该步骤的某处,不能证明它挂在这条 push 上。pushAndReportStep 钉得是对的(/if git_auth push …/),这两条可以照同样方式处理,例如匹配两行形式,或断言紧邻的 git -c credential."https://github.com".helper 前缀。
↩️ 更正我第 1 轮的 M2
我写了"带 ecs 但不带 ecs-qwen 的 runner 可作为 agent job 的不相交池子"。这是错的:ecs-qwen ⊂ ecs。当前在线标签为 15× ecs-qwen,ecs、4× ecs-qwen,ecs,ecs-update-64c、6× ecs——所以路由到 ecs 是超集,不是隔离。要把重型 autofix 与 CI 隔离,需要在 runner 侧加新标签,不在本 PR 范围内。仓库内唯一的杠杆是 max-parallel,而当前的 5 是有实测依据的既定选择,所以我建议保持不动,合并后头几次扫描盯一下 ECS 排队深度即可——这也正是你风险章节已经提出的做法。
第 1 轮遗留(仅记录,不再展开)
- 🟠 M1 —— ECS 上的
setup-node+cache: 'npm',对比ci.yml:246–264在同一池子上刻意采用的NPM_CONFIG_CACHE=${HOME}/.cache/qwen-code/npm拆分方案。 - 🟡 L5 —— PR 正文仍写契约文件通过 "112 tests",实际是 115。
第 1 轮其余问题均已解决。把 gc.autoDetach=false 补上之后,在我这边就可以合并了。
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.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： 🤖 Autofix review round — no changes needed this round. Why no changesThis round's feedback re-delivered the R4 review plus the round summaries already posted for it. Every R4 finding was verified against the current head ( R4 findings re-verified at the current head
Still open — maintainer decisions, not code
Verification run this round (nothing committed)
中文说明🤖 Autofix 评审轮次 —— 本轮无需改动。 为什么没有改动本轮收到的反馈是 R4 评审及其已发布的轮次总结的再次投递。所有 R4 发现都已对照当前 head( 已在当前 head 上复核的 R4 发现
仍然开放 —— 维护者决策,不是代码问题
本轮实际执行的验证(未提交任何内容)
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
Round-2 open items addressed in 🔴 🟡 Assertion anchors — tightened. The dry-run preflight and Noting (no action needed):
Verified: 115 autofix contract tests, 945 across Round-2 遗留项已在 |
Round 3 —
|
Note on the bot round at
|
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 194 passed · 0 failed · 194 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：194 通过 · 0 失败 · 194 总计 Verification reportPR 8603 — ci(autofix): run heavy autofix jobs on the ECS pool with hosted fallbackVerdict: 中文摘要
ScopeCentral claim: the three heavy jobs ( Central claim — A/B tableHarness
36/36 scenario cells at HEAD resolve to the expected label set; 3/3 BASE cells are statically hosted; the three expressions are byte-identical across the jobs. Short jobs ( "Runs there" half (witness
Setup-node: BASE carries Secondary claim 1 — hygiene steps behave as claimedSanitize step ( Workdir lifecycle ( PAT handling ( tmux install (same harness): HEAD fails fast with a named Secondary claim 2 — the new tests are load-bearing (mutation matrix)Witness
No survivors: every reverted hunk is caught by exactly the test the commit says pins it. Base A/A: base's own suite is 112/112 green at base (head adds exactly the 3 new tests). Reviewer Test Plan walkthrough
Findings (non-blocking)F1 (Low) — sanitize scrubs local config only; global/system config poisoning is out of scope. A prior pool job running human-authored code as the runner user can write F2 (Informational) — the F3 (Informational) — kill-switch comparison is case-sensitive ( Not covered
MethodologyEnvironment: the CI verify container ( Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round at
|
| Point | Decision | Reason |
|---|---|---|
Round-2 🔴 gc --auto detach + 🟡 push-auth assertion anchors |
Already fixed | Both landed in 1e35b582ce and were empirically verified in round 3: the git -c gc.autoDetach=false gc --auto --prune=now A/B confirmed foreground execution on git 2.47.3 (including with a planted maintenance.autoDetach=true), and both re-anchored regexes were checked against the real workflow text. Nothing further to do. |
🟠 M1 — setup-node + cache: 'npm' on ECS vs ci.yml's NPM_CONFIG_CACHE=${HOME}/.cache/qwen-code/npm split on the same pool |
Deferred (follow-up) | The reviewer explicitly classified this as non-blocking — "fine to land as-is and reconcile later; it's a consistency/efficiency point, not a correctness one". It stays deferred exactly as recorded in round 2: reconcile the three autofix jobs with ci.yml's self-hosted node/cache pattern (including its documented nodejs.org-through-egress-proxy consideration) in a fleet-consistency follow-up. Not silently dropped. |
| 🟡 L5 — PR body says "112 tests"; the suite actually has 115 | Cannot be fixed from this checkout | Verified at this head: the contract suite genuinely has 115 tests (115 passed). The stale "112" figure exists only in the PR body on GitHub; this autofix mode has no GitHub write access (the workflow owns all network writes), and nothing in the repository contains the figure. The PR body itself needs the one-word correction — a prior round reported it corrected, but the reviewer still sees 112. |
| Process note 1 — the loop's dedup silently absorbed a follow-up Critical | Noted as a follow-up | "No code impact" per the reviewer, and the fix target is the loop's feedback dedup — a comment posted after a fix commit must never be able to close the item it is objecting to. That is separate from this PR's diff, which is already LGTM'd, so it is recorded here rather than added to a late review round. |
Process note 2 — merge blocked by a stale CHANGES_REQUESTED |
Needs a maintainer/workflow GitHub action | The blocking review is qwen-code-ci-bot's on 1c565927b7 — 17 commits behind the current head — whose own body says it stopped before producing findings ("Not reviewed: … stopped before round 3 by the review time budget"). Unblocking requires either dismissing that review or a fresh review pass against 1e35b582ce; both are GitHub-side actions outside this checkout's reach. |
Verification
npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/qwen-autofix-workflow.test.jsat1e35b582ce— 115 passed (evidence for the L5 real test count).- No code changes this round, so no build/typecheck/lint run was required;
git statusis clean and HEAD is still the LGTM'd1e35b582ce.
中文说明
1e35b582ce 上的 autofix 轮次 —— 无需改动
新反馈(第 3 轮评审及两条流程观察)已完成分诊;评审者 LGTM 的 head 保持不变。本轮没有失败的 check,也没有持续发红的 check。
新反馈的分诊
| 条目 | 决定 | 理由 |
|---|---|---|
第 2 轮 🔴 gc --auto detach + 🟡 push 认证断言锚点 |
已修复 | 两者都已在 1e35b582ce 落地,并在第 3 轮得到实测验证:git -c gc.autoDetach=false gc --auto --prune=now 的 A/B 在 git 2.47.3 上确认前台执行(包括在被埋 maintenance.autoDetach=true 的情况下),两条重新锚定的正则也都对照 workflow 真实文本核对过。无需再做任何事。 |
🟠 M1 —— ECS 上的 setup-node + cache: 'npm',对比 ci.yml 在同一池子上的 NPM_CONFIG_CACHE=${HOME}/.cache/qwen-code/npm 拆分 |
缓办(后续跟进) | 评审者已明确将其定为非阻塞——"就这样合入、之后再统一也完全可以;这是一致性/效率问题,不是正确性问题"。维持与第 2 轮记录完全一致的缓办:在后续的全局一致性改动中,把三个 autofix job 与 ci.yml 的 self-hosted node/缓存模式对齐(包括其注释中记录的 nodejs.org 经出口代理不可达的考量)。不会被静默丢弃。 |
| 🟡 L5 —— PR 正文写 "112 tests",实际套件有 115 个 | 无法在本 checkout 内修复 | 已在此 head 上验证:契约套件确实有 115 个测试(115 passed)。过时的 "112" 数字只存在于 GitHub 上的 PR 正文里;本 autofix 模式没有 GitHub 写权限(所有网络写操作由 workflow 负责),且仓库内任何文件都不含该数字。需要修正的是 PR 正文本身——此前某轮曾报告已更正,但评审者仍然看到 112。 |
| 流程观察 1 —— 回路的去重静默吞掉了一个后续 Critical | 记录为后续跟进 | 按评审者所述"对代码没有影响",且修复目标是回路的反馈去重逻辑——发布时间晚于修复 commit 的评论,不应该有能力关闭它正在反对的那个条目。那与本 PR 的 diff 无关,而 diff 已经 LGTM,因此记录在此,而不是塞进后期的评审轮次。 |
流程观察 2 —— 一条过期的 CHANGES_REQUESTED 挡住了合并 |
需要维护者/workflow 执行 GitHub 操作 | 阻塞合并的评审是 qwen-code-ci-bot 基于 1c565927b7 提交的——落后当前 head 17 个 commit——其正文自己写明它在产出结论之前就停止了("Not reviewed: … stopped before round 3 by the review time budget")。解除阻塞需要 dismiss 掉那条评审,或者针对 1e35b582ce 重新跑一轮评审;两者都是本 checkout 够不到的 GitHub 侧操作。 |
验证
- 在
1e35b582ce上运行npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/qwen-autofix-workflow.test.js—— 115 通过(作为 L5 真实测试数的证据)。 - 本轮没有代码改动,因此无需运行 build/typecheck/lint;
git status干净,HEAD 仍为 LGTM 的1e35b582ce。
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。
🧠 Handled by Qwen Code · model/模型 qwen3.8-max
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Re-reviewed the expanded revision. The update applies the same self-hosted hardening uniformly across issue-autofix, build-cli, and review-address (restore workspace ownership, git-config allowlist sanitize incl. symlinked-hook sweep, Node setup on self-hosted) and adds an always() per-run WORKDIR cleanup (rm -rf /tmp/autofix-<run_id>) so the shared pool's /tmp doesn't accumulate — bounded to this run's dir, safe. The runs-on routing guard (same-repo + write+/same-repo-head for PR-family, else hosted) is consistent on all three jobs. No new P0/P1; security posture is more uniform than the first revision. Prior P3s stand (sudo -n fallback warns on pools without NOPASSWD; transient push token briefly in process list).
github-actions
Bot
added
the
skip-changelog-auto
|
Released in v0.21.7. |
4 participants
What this PR does
Routes the three heavy Qwen AutoFix jobs — the issue-fixing agent, the review CLI bundle build, and the review-feedback address agent — off GitHub-hosted runners and onto the persistent self-hosted ECS pool, using the same repository-guarded routing expression as the rest of the fleet. Forks of this repository, and any run where the maintainer ECS kill-switch is enabled, automatically fall back to
ubuntu-latest. To make this work it relaxes the runner-environment check that previously hard-failed these jobs on anything other than a GitHub-hosted runner, and provisions Node on the self-hosted pool instead of skipping setup there. The short-lived router/scan/ack/retry jobs and the separate PR-review workflow are left unchanged.Why it's needed
GitHub-hosted runners are a shared, capped resource for the organization (roughly 20 concurrent jobs). The review-feedback address job alone can hold up to five of those slots for as long as 300 minutes each on a single scheduled scan, and together with CI this saturates the pool. When that happens, the PR-review workflow's hosted-only jobs — the delayed automatic-review timer, fork authorization, and the
/resolveconflict job — queue up and reviews stall. The ECS pool had ample idle capacity (24 runners online, only ~2 busy during the stall) that these jobs were not using. Moving them there frees hosted capacity for review and CI without reducing AutoFix throughput.Reviewer Test Plan
How to verify
This is a CI-infrastructure change; the definitive behavior is observable once the workflow runs after merge. Locally I verified: the AutoFix workflow contract test file passes (115 tests), the full
npm run test:scriptssuite passes (945 passed, 9 skipped), the workflow parses cleanly and passes yamllint, and the routing/contract test now pins the exact ECS routing expression including the repository guard and the hosted fallback so neither can be dropped silently.After merge, a reviewer should confirm: on the next scheduled AutoFix scan the address legs are picked up by
ecs-qwen-*runners (notubuntu-latest), the hosted pool is no longer saturated, and the PR-review delayed-review/authorize jobs start promptly. To roll back without a revert, set the repository variableMAINTAINER_ECS_RUNNER_DISABLEDtotrue(or run on a fork), which routes these jobs back toubuntu-latest.Evidence (Before & After)
Non-UI change: N/A. Test evidence —
Test Files 46 passed (46),Tests 945 passed | 9 skipped (954)fromnpm run test:scripts; yamllint exit 0 on the modified workflow.Tested on
macOS: ran the contract/script test suites locally. Linux: the jobs themselves execute on Linux self-hosted runners after merge — validated via contract tests, docker/sandbox availability already proven in production by the repo-hygiene workflow on the same pool, but the actual post-merge job run is not yet observed.
Environment (optional)
Contract tests via
npm run test:scripts; no local sandbox execution. Docker/sandbox-image behavior on the ECS pool is inferred from the repo-hygiene workflow, which already runs the sandbox image there.Risk & Scope
--rmwith no fixed name so concurrent legs on one machine do not collide. Recommend watching ECS runner disk/health for the first few scans after merge.resolve-pr/review-prjobs are untouched.Linked Issues
No linked issue. Observed stall: ~20 concurrent GitHub-hosted jobs saturated the org pool on 2026-08-05 with nine AutoFix address legs running, while PR-review delayed/authorize jobs queued behind them.
中文说明
本 PR 做了什么
把 Qwen AutoFix 的三个重型 job——issue 修复 agent、review CLI 产物构建、review 反馈处理 agent——从 GitHub 托管 runner 迁到持久化的自托管 ECS 池,使用与整套 runner 池一致的、带仓库守卫的路由表达式。本仓库的 fork、以及开启了 maintainer ECS 停用开关的运行,都会自动回退到
ubuntu-latest。为此放宽了原本在非托管 runner 上直接 fail 的 runner 环境检查,并改为在自托管池上安装 Node(原来会跳过)。短耗时的路由/扫描/应答/重试 job,以及独立的 PR-review workflow,保持不变。为什么需要
GitHub 托管 runner 对组织而言是共享且有上限的资源(约 20 个并发 job)。仅 review 反馈处理 job 在一次定时扫描里就可能占用多达 5 个槽位、每个最长 300 分钟,叠加 CI 后会把池子打满。一旦发生,PR-review workflow 里只能用托管 runner 的 job——延迟自动评审计时器、fork 授权、
/resolve冲突 job——就会排队,评审被卡住。而 ECS 池当时有大量空闲容量(24 台在线、卡顿时仅约 2 台 busy)却没被这些 job 利用。迁过去后,既释放了托管容量给评审和 CI,又不降低 AutoFix 吞吐。审阅者测试计划
如何验证
这是 CI 基础设施改动;确定性行为要在合并后 workflow 运行时才能观察。本地我已验证:AutoFix workflow 契约测试文件通过(115 个),完整
npm run test:scripts通过(945 过、9 跳过),workflow 能被正常解析并通过 yamllint;路由/契约测试现在会精确钉住新的 ECS 路由表达式(含仓库守卫与托管回退),两者都不能被悄悄删掉。合并后,审阅者应确认:下一次定时 AutoFix 扫描时,处理 leg 由
ecs-qwen-*runner(而非ubuntu-latest)接走;托管池不再被打满;PR-review 的延迟评审/授权 job 能及时启动。如需不回滚代码就回退,把仓库变量MAINTAINER_ECS_RUNNER_DISABLED设为true(或在 fork 上运行),这些 job 即回到ubuntu-latest。前后对比证据
非 UI 改动:N/A。测试证据——
npm run test:scripts输出Test Files 46 passed (46)、Tests 945 passed | 9 skipped (954);改动后的 workflow yamllint 退出码 0。测试环境
macOS:本地跑了契约/脚本测试套件。Linux:这些 job 合并后在 Linux 自托管 runner 上执行——已通过契约测试验证,docker/sandbox 镜像可用性也已被 repo-hygiene workflow 在同一池子上生产验证,但合并后的真实 job 运行尚未观察到。
环境(可选)
契约测试经
npm run test:scripts;本地未执行 sandbox。ECS 池上的 docker/sandbox 镜像行为依据 repo-hygiene workflow 推断——它已在该池子上运行 sandbox 镜像。风险与范围
--rm且无固定名,同机并发 leg 不会互相冲突。建议合并后头几次扫描观察 ECS runner 的磁盘/健康状态。resolve-pr/review-prjob 未改动。关联 Issue
无关联 issue。观察到的卡顿:2026-08-05,约 20 个 GitHub 托管并发 job 打满组织池子,其中 9 个 AutoFix 处理 leg 在跑,而 PR-review 的延迟/授权 job 在其后排队。