fix(ci): give each job its own proxy wrapper directory - #7951
Merged
Conversation
The PR review job and the containerised triage jobs all built their gh/git proxy wrappers in one fixed directory under RUNNER_TEMP. On the shared self-hosted runner that path outlives a job, and the triage containers write it as root through the RUNNER_TEMP bind mount. Once that happened, the review job — running as the unprivileged runner user — could neither overwrite the wrapper nor remove the root-owned directory holding it, so every review scheduled on that runner died with EACCES while writing the wrapper, before the review itself had started. Each run now creates a private wrapper directory and removes it on exit, and the container jobs keep theirs on the container's own disk so they stop leaving root-owned state on the host.
Collaborator
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. A specific runner ( Direction: aligned — this is CI plumbing that fixes a real breakage in the review/triage pipeline. It doesn't touch auth, sandbox, model selection, telemetry, release, or any public contract. CHANGELOG: no direct reference (CI-only change), and the area is clearly in scope. Size: not applicable — two workflow files, +27/-7, no core paths. Approach: the scope is exactly right. Risk: no elevated risk signals — neither changed file matches the high-risk paths from the revert-history analysis. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到，而非理论性。某台特定 runner（ 方向：对齐——这是修复 review/triage 流水线真实故障的 CI 管道改动，不触及 auth、sandbox、模型选择、telemetry、release 或任何公开契约。CHANGELOG：无直接引用（仅 CI 改动），该领域显然在范围内。 规模：不适用——两个 workflow 文件，+27/-7，未触及核心路径。 方案：范围恰好。用 风险：无升级风险信号——两个改动文件均未命中 revert 历史分析中的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
Code reviewIndependent proposal first: faced with "a fixed wrapper path gets poisoned by root-owned files via the The implementation is correct on the points that matter:
No correctness, security, or regression concerns; no convention violations. Comments explain the non-obvious "why" and are justified. TestingThis is the unattended CI path, so no PR code is executed here and there is no local tmux run (2c is out of scope on CI, and the change has no user-visible surface — CI on
One row per check name (latest run); skipped checks omitted. / 每个检查名一行（取最新一次运行），省略 skipped。 The Linux unit test is running again on a fresh CI run; the macOS/Windows tests and the integration/verify/tmux/web-shell lanes are skipped, consistent with a gated CI-only change. The author reports the extracted step scripts pass On the earlier red that a prior finalize pass recorded here: that failure was in The product sandboxed lanes ( 中文说明代码审查先给出独立方案：面对"固定 wrapper 路径被 root 属主文件经 实现在关键点上正确：
无正确性、安全性或回归问题；无规范违反。注释解释了非显而易见的"为什么"，物有所值。 测试这是无人值守 CI 路径，因此此处不执行任何 PR 代码，也没有本地 tmux 运行（2c 在 CI 上不适用，且改动无用户可见界面—— Linux 单元测试正在新一次 CI 运行中重跑；macOS/Windows 测试及 integration/verify/tmux/web-shell 通道均被跳过，与受门控的仅 CI 改动一致。作者称抽取出的步骤脚本通过 关于此前 finalize 在此记录的转红：该失败发生在 产品沙箱通道（ — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage; the fix is minimal, correct, and matches my independent proposal. Approval is deferred only so CI can land green on the reviewed commit. Stepping back: this is a tight, well-motivated fix. The problem is real and observed (a poisoned runner, not a hypothesis), the root cause is correctly identified, and the change does the minimum needed — unique per-run wrapper dirs plus cleanup, and container jobs moved off the host bind mount. Nothing in the diff is extraneous. The The one thing I can't verify from here is the runtime behaviour on a poisoned runner — that's inherent to CI infrastructure and confirms post-merge (reviews on Verdict: approve. CI ( 中文说明置信度：5/5 —— 每个阶段都干净；修复最小、正确，且与我的独立方案一致。仅推迟批准，等 CI 在被审 commit 上转绿。 退一步看：这是一个紧凑、动机充分的修复。问题真实且已观测（被污染的 runner，而非假设），根因定位正确，改动只做了必需的事——每次运行独有的 wrapper 目录加清理，以及把容器 job 移出宿主 bind mount。diff 中没有多余内容。用 我在此无法验证的唯一一点是在被污染 runner 上的运行时行为——这是 CI 基础设施的固有属性，需合入后确认（ 结论：批准。CI（ — Qwen Code · qwen3.8-max-preview Reviewed at |
Contributor
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. |
doudouOUC
approved these changes
Jul 28, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed at HEAD 806931f. Approve — code-level review, every claim below verified against the HEAD tree, not the PR description.
- The root cause analysis is consistent with the code. At HEAD, exactly three sites build the wrapper directory, and they had all shared the literal
${RUNNER_TEMP:-/tmp}/qwen-network-bin: the review job (host, unprivileged runner user) and the two triage container jobs (node:22-bookworm, root,RUNNER_TEMPbind-mounted). The poisoning mechanism — root-owned dir on the host that the host job can neither overwrite norrm— follows directly. - The container fix actually escapes the bind mount. Both triage containers declare only
image+options: '--init', no volume mounts beyond the runner defaults, somktemp -d /tmp/qwen-network-bin.XXXXXXlands on the container's own writable layer and dies with the container. No cleanup trap is needed there, and none is pretended. - The review-job trap is correct in all three orderings.
PROXY_BINis initialised empty before the trap is installed;proxy_binislocaltoconfigure_qwen_networkwhile thePROXY_BIN="$proxy_bin"assignment escapes to the step shell (the function is called directly, not in a subshell); the[ -z "$PROXY_BIN" ] ||guard makes the trap a no-op when the step dies beforemktempruns; andrm -rfof a per-runmktempdir cannot change the step's exit status. Failure-before-creation, failure-after-creation, and success all behave. - No other consumer of the fixed path exists.
git grep qwen-network-binacross the entire HEAD tree hits only the three rewritten sites — no test, script, or later workflow step expects the old fixed location, andPATHwas always exported within the same step shell, so per-run naming changes nothing downstream. - The red
Test (ubuntu-latest, Node 22.x)check is pre-existing flake, classified from check identity and diff, not log claims. The 13 failures are all inpackages/core(write-file.test.ts×10,openaiLogger.test.ts×2,gitWorktreeService.symlinks.integ.test.ts×1) with fixed-/tmp-path collision signatures (EEXIST: /tmp/qwen-wt-sibling, files under the shared/tmp/qwen-code-test-rootvanishing mid-test) — none of which a two-file workflow-YAML diff can influence. Two unrelated PRs in the same time window failed the same job on different flaky tests (e.g. aclipboardUtilshook timeout), confirming suite instability, not a PR regression. A rerun should clear it; the fixed-path collisions in those core tests are, fittingly, the same class of bug this PR fixes in CI.
Non-blocking observation, already acknowledged in the PR: the remaining fixed root-owned dirs the container jobs create under RUNNER_TEMP (tmux-results, verify-results, verify-context) are written only ever as root by container jobs, so they collide with nothing today — but they are the same pattern one new host-side consumer away from the same landmine.
中文总结
在 HEAD 806931f 上完成代码审查，批准（所有结论均对照 HEAD 代码树验证，而非复述 PR 描述）。
- 根因分析与代码一致：全树恰好三处构建 wrapper 目录，此前共享同一固定路径；容器 job 以 root 通过 bind mount 写宿主，宿主上的 review job 以非特权用户运行，EACCES 污染机制成立。
- 容器侧修复真正脱离了 bind mount：两个 triage 容器仅声明 image +
--init，无额外挂载，mktemp -d /tmp/...落在容器自身磁盘，随容器销毁，无需清理 trap。 - review job 的 trap 三种时序均正确：
PROXY_BIN先置空再装 trap；函数直接调用（非子 shell），赋值可逃逸到步骤 shell；[ -z ]守卫覆盖目录创建前失败；rm -rf不影响退出码。 - 全树
git grep确认固定路径无其他消费者，PATH 本就只在同一步骤 shell 内导出，改为每次运行命名无下游影响。 - 红色的 Test job 为预存在 flaky（依据 check 身份与 diff 分类）：13 个失败全在
packages/core，为固定/tmp路径并发冲突特征，两个 workflow YAML 的 diff 无法影响；同时段两个不相关 PR 在同一 job 上因不同的 flaky 测试失败，证实是套件级不稳定。重跑即可；巧合的是这些 core 测试的固定路径冲突正是本 PR 在 CI 里修复的同类问题。
非阻塞观察（PR 已自行说明）：容器 job 在 RUNNER_TEMP 下仍创建的固定目录（tmux-results/verify-results/verify-context）目前只由 root 容器 job 写入、无冲突，但属同一模式，未来若有宿主侧消费者会踩同样的雷。
Collaborator
|
@qwen-code /triage |
Collaborator
Author
ReviewThe diagnosis is right and the review-job half of the fix is solid. The triage half has a regression: What the PR doesReplaces the fixed Verified independently:
Critical — the wrapper directory becomes unreachable for the agent in both triage jobs
with The failure is silent, not loud. Bash's PATH search skips an unsearchable directory and falls through to the next entry, so That defeats the wrappers' only job. The parent step does Since proxy_bin="$(mktemp -d /tmp/qwen-network-bin.XXXXXX)"
chmod 755 "$proxy_bin"or just The PR review job is unaffected — no Minor
Follow-up (out of scope, worth filing)The fix closes one instance of the bind-mount poisoning class, not the class. The containerised jobs still write fixed root-owned paths into StyleComments are the right kind — they explain why the fixed path was a landmine rather than restating the code, and they will survive the next person who wonders why this is not just Verdict: blocked on the |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: the code-quality pass, the performance pass, the test-coverage pass, the open-ended audit (attacker mindset), the open-ended audit (oncall mindset), the open-ended audit (maintainer mindset), the removed-behavior audit, the cross-file consistency pass, the build-and-test check — its prompt was built, but no agent on record was launched with it.
中文说明
已审查。 建议见行内评论。 未审查：代码质量检查、性能检查、测试覆盖检查、开放式审计（攻击者视角）、开放式审计（值班排障视角）、开放式审计（后续维护者视角）、删除行为审计、跨文件一致性检查、构建与测试验证——它的 prompt 已构建，但没有任何 agent 有记录用它启动过。
— qwen3.7-max via Qwen Code /review
| # non-container jobs (the PR review) can neither overwrite nor | ||
| # delete, breaking every review scheduled on that runner. Keep | ||
| # them on the container's own disk, under a per-run directory. | ||
| proxy_bin="$(mktemp -d /tmp/qwen-network-bin.XXXXXX)" |
Collaborator
There was a problem hiding this comment.
[Suggestion] No test asserts that the triage workflow's proxy wrappers use container-local /tmp rather than RUNNER_TEMP. — Failure scenario: if a future edit reverts this line to ${RUNNER_TEMP:-/tmp}/qwen-network-bin.XXXXXX (matching the review workflow's pattern, which looks like the more defensive choice), the original bug recurs — root-owned files on the host via the RUNNER_TEMP bind mount cause EACCES for every subsequent non-container review job on that runner.
The triage test file (scripts/tests/qwen-triage-workflow.test.js) has zero assertions on proxy_bin, configure_qwen_network, or network-bin, despite asserting RUNNER_TEMP paths for 22 other concerns — showing the established pattern for guarding path choices.
Suggested change
| proxy_bin="$(mktemp -d /tmp/qwen-network-bin.XXXXXX)" | |
| proxy_bin="$(mktemp -d /tmp/qwen-network-bin.XXXXXX)" # container-local; NOT $RUNNER_TEMP (host bind mount) |
中文说明
[Suggestion] 没有测试断言 triage workflow 的代理脚本包装器使用的是容器内的 /tmp 而不是 RUNNER_TEMP。—— 失败场景：如果未来的编辑将此行改回 ${RUNNER_TEMP:-/tmp}/qwen-network-bin.XXXXXX（与 review workflow 的模式一致，看起来更像更稳妥的选择），原来的 bug 就会重现 —— 通过 RUNNER_TEMP bind mount 在宿主机上留下 root 所属的文件，导致该 runner 上所有后续的非容器 review job 出现 EACCES。
triage 测试文件 (scripts/tests/qwen-triage-workflow.test.js) 对 proxy_bin、configure_qwen_network 或 network-bin 没有任何断言，尽管它对 22 个其他路径使用了 RUNNER_TEMP 断言 —— 说明已有现成的路径保护模式可以复用。
— qwen3.7-max via Qwen Code /review
Collaborator
|
Released in v0.21.1. |
Collaborator
|
This is just the release bot confirming PR #7951 ( Acknowledged. ✅ ✅ completed |
doudouOUC
pushed a commit
to doudouOUC/qwen-code
that referenced
this pull request
Jul 29, 2026
…ger (QwenLM#7965) Post-merge measurement of QwenLM#7917, one day in: 9 eligible PRs, two considered-and-declined mentions (both correct calls), zero positive recommendations. The one clear behavioural candidate — QwenLM#7947, bounded reads of large text files — wrote "Not verified: Windows and Linux manual runs (author tested on macOS only)" in its own Stage 2 comment and never named a lane. That is the failure shape worth fixing: the judgement-based rule ("when neither static review nor 2b substantiates it") failed exactly where the comment had already written the gap down in so many words. The model judged that pending CI would cover it; a green suite proves the tests pass, not that the untested behaviour holds. So the trigger is now textual, not judgemental: before posting, grep your own draft. A sentence of the shape "not verified", "author tested on one platform only", or "author's claim, not independently re-run" IS the trigger — the 2b-bis line is that same sentence with the remedy attached, and omitting it means telling the maintainer what is missing while withholding the one command that would supply it. Pending CI does not lift the trigger. The two legitimate skip cases (nothing behavioural to settle; author lacks write) are unchanged, and the rule is ordered before them so they read as outs from the requirement, not the requirement as an out from them. The trigger phrases are verbatim from real comments: "not verified" and "author tested on macOS only" from QwenLM#7947, "author's claim, not independently re-run" from QwenLM#7951. Mutation-verified 3/3: dropping the trigger paragraph, moving it after the skip cases, and dropping the pending-CI sentence each turn the test red. n=1 is thin evidence for a behavioural rule change — but this rule is text-matching, not probability-weighing, so it cannot overfit to the sample that motivated it. Co-authored-by: wenshao <wenshao@example.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
The PR review job and the two containerised triage jobs each build a pair of proxy wrappers for
ghandgitbefore handing control to the agent, and all three built them in the same fixed directory underRUNNER_TEMP. This change gives every run its own private wrapper directory, removed again when the step exits. The container jobs additionally keep their wrappers on the container's own disk instead of the bind-mounted runner temp directory.Why it's needed
On the shared self-hosted runners
RUNNER_TEMPoutlives an individual job, and the triage jobs that run inside a container write into it as root through the bind mount. Once one of those jobs had run on a given runner, the fixed wrapper path on that host was owned by root. The review job runs directly on the host as the unprivileged runner user, so from then on it could neither overwrite the wrapper file nor delete the root-owned directory holding it — the runner's own temp cleanup cannot remove it either. Every review scheduled onto that runner then failed withPermission deniedwhile writing the wrapper, roughly twenty seconds in and before the review had started, and the fallback comment told the author to retry — which failed the same way whenever the retry landed on the same runner.Runner
ecs-qwen-runner-sg-12was in that state today; reviews on the other runners were unaffected, which is why this looked intermittent rather than broken.Note that this stops new runners from being poisoned, but the leftover root-owned directory on an already-affected runner still has to be removed by hand on the host, since nothing running in CI has the privileges to delete it.
Reviewer Test Plan
How to verify
The failure is reproducible locally without CI. Create a directory owned by an unprivileged user, then, as root, create the shared wrapper directory and file inside it the way a container job does. Acting as the unprivileged user, writing the wrapper to that fixed path fails with exactly the error seen in CI, and an attempt to clean it up fails too:
With the change, the same unprivileged user creates its own directory next to the poisoned one and writes, marks executable and runs the wrapper successfully, then removes it — the stale root-owned directory no longer matters.
Worth confirming on review: the cleanup runs on both the success and failure paths and does not alter the step's exit status, including when the step fails before the wrapper directory is ever created. I checked all three cases (success with a directory, failure before one exists, and failure after) and the exit codes are preserved with no directory left behind. The extracted step scripts also still pass
bash -nandshellcheck, and both workflow files still parse as YAML.An end-to-end confirmation is simply that reviews scheduled onto the affected runner start completing again.
Evidence (Before & After)
N/A — CI-only change with no user-visible surface.
Tested on
Risk & Scope
Linked Issues
None.
中文说明
这个 PR 做了什么
PR review job 和两个跑在容器里的 triage job，在把控制权交给 agent 之前都会各自生成一对
gh和git的代理 wrapper，而三者都把它们生成在RUNNER_TEMP下同一个固定目录里。这个改动让每次运行使用自己私有的 wrapper 目录，并在步骤结束时删除。容器里的 job 额外改为把 wrapper 放在容器自身的磁盘上，而不是 bind mount 进来的 runner 临时目录。为什么需要
在共享的自托管 runner 上，
RUNNER_TEMP的生命周期长于单个 job，而跑在容器里的 triage job 会通过 bind mount 以 root 身份往里写。只要某台 runner 上跑过一次这样的 job，该宿主上那个固定的 wrapper 路径就归 root 所有。review job 直接以非特权的 runner 用户在宿主上运行，从那以后它既无法覆盖 wrapper 文件，也无法删除持有该文件的 root 属主目录 —— runner 自带的临时目录清理同样删不掉。此后调度到这台 runner 上的每一次 review 都会在写 wrapper 时以Permission denied失败，大约二十秒就挂掉，此时 review 根本还没开始；而 fallback 评论会让作者重试 —— 只要重试再次落到同一台 runner，就会以同样的方式失败。今天 runner
ecs-qwen-runner-sg-12就处于这个状态；其他 runner 上的 review 不受影响，所以这个问题看起来是偶发的，而不是彻底坏掉。需要说明的是，这个改动能防止新的 runner 被污染，但已经受影响的 runner 上残留的 root 属主目录仍然需要在宿主上手工删除，因为 CI 里运行的任何东西都没有权限删掉它。
Reviewer Test Plan
如何验证
这个失败不依赖 CI，本地即可复现。先建一个属于非特权用户的目录，然后以 root 身份、按照容器 job 的方式在其中创建那个共享的 wrapper 目录和文件。再以非特权用户的身份往这个固定路径写 wrapper，就会得到与 CI 中完全一致的报错，而且尝试清理同样会失败：
应用改动之后，同一个非特权用户会在被污染的目录旁边创建属于自己的目录，成功写入 wrapper、赋予可执行权限并运行，随后将其删除 —— 残留的 root 属主目录不再有影响。
评审时值得确认的点：清理在成功和失败两条路径上都会执行，并且不会改变步骤的退出码，包括步骤在 wrapper 目录创建之前就失败的情况。我验证了全部三种情形（成功且已创建目录、目录创建前失败、目录创建后失败），退出码均被保留，且没有目录残留。抽取出来的步骤脚本仍然通过
bash -n和shellcheck，两个 workflow 文件也仍然能正常解析为 YAML。端到端的确认方式很简单：调度到受影响 runner 上的 review 重新开始正常完成。
Evidence (Before & After)
N/A —— 仅涉及 CI，没有用户可见的界面。
测试环境
风险与范围
关联 Issue
无。