Repository navigation
fix(cli): scrub inherited loader env vars from daemon session subprocesses - #8663
Conversation
…esses Daemon-mode sessions bound to one workspace inherited loader-affecting env vars (NODE_OPTIONS with dev-harness --import hooks, NODE_PATH, preload-class vars) from whatever shell launched the daemon, so subprocesses in another workspace resolved modules through the launching checkout's tree (fixes #8653). Scrub the loader subset of RELOAD_EXCLUDED_KEYS from process.env at the two process boundaries that host sessions: the daemon after freezing its boot env (the frozen copy keeps loader vars so dev-mode ACP children can still boot), and the ACP child after the relaunch/sandbox handoff (the respawned child re-scrubs itself). Fixes #8653
github-actions
Bot
added
the
review/self-reported
E2E test reportReproduction (pre-fix, v0.21.6 global install): daemon launched from workspace Verification (post-fix, built
By-design residual: the ws-a loader executes exactly once with Artifacts: 中文复现（修复前，全局安装 v0.21.6）：从 workspace 验证（修复后，本分支构建产物，同一场景）：ws-b 会话子进程中 设计内的残留：ws-a 的 loader 会在 ws-b 的 ACP 子进程启动时执行恰好一次。 |
|
@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 冲突，直到移除标签或达到轮次上限。移除 |
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. |
A trusted workspace's .env could re-populate the loader-key slots that scrubInheritedLoaderEnv() emptied in the daemon process, because canApplyParsedEnvKey applied RELOAD_EXCLUDED_KEYS only on reloads. Reject the loader subset on every .env application path so one workspace's loader hook cannot reach other workspaces' session subprocesses through the shared daemon env. Also make the scrub return the removed keys and emit a stderr breadcrumb naming them at both boundaries, so a session subprocess missing an inherited var can be traced back to the scrub.
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review feedback round — PR #8663Feedback points and dispositions[Critical] R1-1 (rc:3733638377) — [Suggestion] R1-2 (rc:3733638383) — no test for the ACP-child scrub call site: FIXED. [Suggestion] R1-3 (rc:3733638386) — silent scrub at both boundaries: FIXED. [Suggestion] R1-8 (rc:3733638389) — PATH-prefix follow-up tracking: ESCALATED (not resolvable in this mode). Review verdict (CHANGES_REQUESTED) — "Integration Tests (CLI, No Sandbox) was skipped in CI and did not run locally": ADDRESSED. Conflict notes
VerificationCommands actually run and their results:
中文说明评审反馈轮次 — PR #8663反馈点与处置[Critical] R1-1（rc:3733638377）—— [Suggestion] R1-2（rc:3733638383）—— ACP 子进程剥离调用点无测试：已修复。 [Suggestion] R1-3（rc:3733638386）—— 两个边界上的剥离静默执行：已修复。 [Suggestion] R1-8（rc:3733638389）—— PATH 前缀后续跟踪：已升级处理（本模式下无法完成）。 评审结论（CHANGES_REQUESTED）——“Integration Tests (CLI, No Sandbox) 在 CI 中被跳过且本地未运行”：已处理。 冲突说明
Verification实际执行的命令及结果：
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/模型 |
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Review round summary — PR #8663Commit: Feedback pointsReview verdict: "Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally" — addressedThe reviewer could not complete verification because the CLI integration suite never ran. Ran it locally this round: R2-1 [Critical] Serve fast path applies loader keys before the daemon env freeze — FIXEDConfirmed: R2-3 [Suggestion] Rejected
|
Co-authored-by: Qwen-Coder <[email protected]>
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed — PR #8663 (round 3)All 8 findings from round 3 were verified against the code and addressed in one commit ( Findings and dispositions
Verification
中文说明已处理的评审反馈 — PR #8663（第 3 轮）第 3 轮的全部 8 条发现均已对照代码核实，并在一个提交中处理完毕（ 发现与处理
验证
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/模型 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： 🤖 Autofix review round — no action needed (round 4/100) The feedback window since the last evaluation (2026-08-07T12:20:10Z) contains no reviews, no inline comments, and no issue-level comments. The round-3 review findings at that timestamp (1 Critical + 6 Suggestions) were already addressed by the previous round's commit Why the Serve A/B cancellation is not a code defect
Local surrogate verification at the current head
No code changes were made this round; there are no threads to resolve and no findings to reply to. If the Serve A/B preview is still wanted at the current head, re-running that workflow is sufficient — the workflow's independent CI remains the final verification gate. 中文说明🤖 Autofix 评审轮次 — 无需处理（第 4/100 轮） 自上次评估（2026-08-07T12:20:10Z）以来的反馈窗口内没有评审、没有行内评论、也没有 Issue 级评论。该时间点的第 3 轮评审发现（1 个 Critical + 6 个 Suggestion）已由上一轮的提交 为什么 Serve A/B 被取消不属于代码缺陷
在当前 head 上的本地替代验证
本轮未做任何代码改动；没有需要解决的评审线程，也没有需要回复的发现。如果仍希望在当前 head 上获得 Serve A/B 预览，重新运行该 workflow 即可——workflow 的独立 CI 仍是最终验证关口。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #8663 (review round 4)Commit: All 7 inline findings were resolved in code; the review-level note about never-run integration tests was addressed by actually running the suite. Feedback dispositionsR4-1 [Critical] Case-sensitive loader-key matching — FIXEDVerified the premise end-to-end on this machine before changing code: npm 10.9.8 applies Changes:
R4-2 [Suggestion] settings.env rejections dropped silently — FIXEDAll three R4-3 [Suggestion] False comment premise in fast-path.test.ts — FIXEDApplied the suggested wording. Verified against the code first: R4-4 [Suggestion] No direct settings.env loader-rejection tests — FIXEDAdded R4-5 [Suggestion] Duplicate derived set — FIXEDBoth local R4-6 [Suggestion] collect-then-report boilerplate — FIXED
R4-7 [Suggestion] Fast-path rejections lost under systemd/desktop launches — FIXED
Review-level note: integration tests never run — ADDRESSEDThe bundled-CLI integration suite was built ( Integration test failure diagnosis (environment-only)
Evidence:
Verification
中文说明Autofix 本轮总结 — PR #8663（评审第 4 轮）提交： 7 条行内评审意见全部在代码中解决；评审级别关于"集成测试从未运行"的备注已通过实际运行测试套件解决。 反馈处理R4-1 [Critical] loader 键按大小写精确匹配 — 已修复改动前已在本机端到端验证其前提：npm 10.9.8 对 改动：
R4-2 [Suggestion] settings.env 拒绝被静默丢弃 — 已修复
R4-3 [Suggestion] fast-path.test.ts 中注释前提错误 — 已修复采用建议的措辞。先对照代码核实： R4-4 [Suggestion] 缺少 settings.env loader 拒绝的直接测试 — 已修复新增 R4-5 [Suggestion] 重复的派生集合 — 已修复两处本地 R4-6 [Suggestion] 先收集再报告的样板代码 — 已修复
R4-7 [Suggestion] systemd/桌面启动下快速路径拒绝信息丢失 — 已修复
评审级别备注：集成测试从未运行 — 已处理本轮构建（ 集成测试失败诊断（纯环境问题）
证据：
验证
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/模型 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： 🤖 Autofix review round — no action needed The feedback window since the last evaluation (2026-08-07T15:03:12Z) contains no reviews, no inline comments, and no issue-level comments. The round-4 review findings at that timestamp (1 Critical + 6 Suggestions) were addressed by the previous round's commit Why the Serve A/B cancellation is not a code defect
Local surrogate verification at the current head (
No code changes were made this round; there are no threads to resolve and no findings to reply to. If the Serve A/B preview is still wanted at the current head, re-running that workflow is sufficient — the workflow's independent CI remains the final verification gate. 中文说明🤖 Autofix 评审轮次 — 无需处理 自上次评估（2026-08-07T15:03:12Z）以来的反馈窗口内没有评审、没有行内评论、也没有 Issue 级评论。该时间点的第 4 轮评审发现（1 个 Critical + 6 个 Suggestion）已由上一轮的提交 为什么 Serve A/B 被取消不属于代码缺陷
在当前 head（
本轮未做任何代码改动；没有需要解决的评审线程，也没有需要回复的发现。如果仍希望在当前 head 上获得 Serve A/B 预览，重新运行该 workflow 即可——workflow 的独立 CI 仍是最终验证关口。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
…mon workers npm maps non-leading underscores in npm_config_* keys onto hyphens, so npm_config_node-options injected NODE_OPTIONS exactly like npm_config_node_options while slipping past every loader gate and scrub. Canonicalize case and underscore/hyphen spelling on both sides of the loader-key membership test, covering .env loads, settings.env application, the serve fast path, and the inherited scrubs. Channel daemon workers are spawned with the daemon's pre-scrub base env but are not ACP children, so they never ran the self-scrub; mirror the ACP-child scrub at the worker entry so nothing a worker spawns inherits loader vars into another workspace. Scope the settings.env rejection warning per workspace so a multi-workspace daemon reports every workspace's rejection instead of deduping them all under one label, revert the unread loadServeFastPathEnvironment return value to void, and pin the buildRuntimeEnvironment settings.env gate and the consume-once stash reset with discriminating tests. Co-authored-by: Qwen-Coder <[email protected]>
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed — round 6 (PR #8663)Commit: Findings and decisions
Review-level CHANGES_REQUESTED ("Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally"): the suite now ran locally against the freshly bundled CLI — see Verification. First run showed exactly one failure, proven environmental (the runner's Changed files
Verification
中文说明已处理的评审反馈 — 第 6 轮（PR #8663）提交： 各条发现与决定
评审级 CHANGES_REQUESTED（"Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally"）：该套件已在本地针对新打包的 CLI 运行——见验证部分。首次运行仅有一处失败，已证明为环境问题（runner 的 变更文件
验证
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/模型 |
…ader-env-leak # Conflicts: # packages/cli/src/config/shared-env-keys.test.ts
|
🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 6/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #8663 (address-review)Feedback pointsNone this round. The workflow-prepared feedback contained no reviews, no inline comments, no issue-level comments, and no failed or still-red checks newer than the last evaluation. Work performed: base-conflict resolution (
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Review: loader-env scrub (fix/daemon-session-loader-env-leak)
The core fix is right and the E2E evidence in the PR body is convincing — the daemon/ACP boundaries are the correct places to break the inheritance chain, and deriving RELOAD_EXCLUDED_KEYS from the shared constant removes a real duplication hazard. Findings below are about the edges of the policy, not the mechanism.
Each finding was re-verified against the diff vs merge-base(origin/main, 43a0a73); line references are to the PR head.
P1 (4) — ENV now rejected on the initial .env load (behavior break + per-start stderr noise) · the daemon's own per-workspace .env path rejects silently, contradicting the docs this PR adds · runQwenServe() mutates the host process.env irreversibly despite being an embeddable entry point · LD_LIBRARY_PATH/DYLD_LIBRARY_PATH are search paths, not injection vectors, and the --acp gate scrubs them far outside the daemon.
P2 (4) — docs overclaim vs mcpServers[].env / hooks[].env · missing siblings (ZDOTDIR, DYLD_FALLBACK_*, non-Node runtimes) · fast-path stash overwrites and duplicates · warn-once key mismatch (normalized vs raw .env path).
P3 (3) — writeStderrLine vs writeStderrLineSafe · call sites disagree about who filters · altitude question about sanitizeChildEnv() as the existing choke point.
|
🤖 AutoFix updated a stale base — the fix did not pass verification, but this PR was behind What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31233855628 🧠 Handled by Qwen Code · model/模型 |
|
🔀 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 将重新运行。 |
- report rejected loader keys through the daemon log after boot (per-workspace .env loads were silent once boot stderr was gone) - accumulate serve fast-path rejected keys across loads instead of overwriting, dedupe, and use normalized env file paths for rejection sources - restore scrubbed inherited loader env vars on embedded runQwenServe close() - add regression tests for ENV scope coverage, reporter dedupe, fast-path accumulation, and post-boot daemon-log diagnostics - scope docs: top-level env rejection does not apply to mcpServers[].env / hooks[].env; document serve loader-scrub behavior
|
@qwen-code /triage |
|
@qwen-code /resolve |
…ader-env-leak # Conflicts: # packages/cli/src/serve/fast-path-settings.ts # packages/cli/src/serve/fast-path.test.ts
…boot env restore (#8663) Address round-6 review: DEV joins the hardcoded project-env exclusions so a workspace file cannot disable the daemon's loader-env scrub; the hardcoded tier is enforced case-insensitively (Windows env lookup is case-insensitive) via isHardcodedProjectEnvExclusion at every application gate; runQwenServe's catch restores the scrubbed launch env and detaches the rejection reporter when startup fails after the scrub. Tests gain the matching regressions, home-env hermeticity, source-scoped warning filters, and tmpdir cleanup; the unreachable reload delete-pass loader guard and its vacuous test are removed.
|
🤖 Addressed the latest review feedback (round 10/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 10/100 轮）。改动内容与我反驳保留之处如下： Round-6 review feedback — address summary (PR #8663)Also merged
Feedback dispositions
Review-level note "Not reviewed: build-and-test — Integration Tests (CLI, No VerificationCommands actually run and their results:
中文说明第 6 轮评审反馈处理总结（PR #8663）同时合并了
反馈处理
评审层面的说明"Not reviewed: build-and-test — Integration Tests (CLI, No 验证实际运行的命令及结果：
Base-conflict check · 基分支冲突检查: conflicted with main — resolved in this push. · 与 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/模型 |
|
📊 Takeover milestone — round 10/100, in the current window. Census: 7 pushed fix(es), 3 no-change review(s), 2 timeout(s), 0 rejected attempt(s), 1 other round(s) (crash / model error / gate error / infra), 1 base update(s). This many rounds deserves a human look. Options: keep going (fine — nothing changes), split or reduce the PR if rounds keep accumulating, or release takeover (remove the 中文说明📊 接管里程碑 —— 第 10/100 轮（当前窗口）。统计：推送修复 7 次、审阅无需改动 3 次、超时 2 次、验证拒绝 0 次、其他轮次（崩溃/模型错误/门错误/infra）1 次、base 更新 1 次。 轮次到这个量值得人工看一眼。可选：继续（无需操作）；若轮次持续累积，考虑拆分或缩减 PR；或释放接管（移除 |
|
Qwen Code did not run conflict resolution for this request. PR #8663 does not currently have merge conflicts with main. |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round: no action neededNo code changes were made this round. The only new feedback is @wenshao's independent E2E verification report, which concludes LGTM and explicitly labels all observations "none are merge blockers". Disposition of each item:
Also noted: Critical-only mode is active for this PR, so the deferred non-Critical feedback listed by the workflow is an audit record and was intentionally not acted on. There are no failed or still-red checks, and no inline review threads to resolve or reply to. 中文说明Autofix 审查轮次：无需处理本轮没有做任何代码改动。新增反馈只有 @wenshao 的独立 E2E 验证报告，结论为 LGTM，并且明确标注所有观察项"都不构成合并阻塞"。 各条目的处理如下：
另外说明：本 PR 已进入仅处理 Critical 的模式，工作流列出的延后非 Critical 反馈属于审计记录，本轮按规则不做处理。当前没有失败或持续失败的检查，也没有需要解决或回复的行内审查线程。 Deferred non-Critical feedbackCritical-only mode is active after 10 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明完成 10 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Round-6 follow-up: R6-3 named RELOAD_EXCLUDED_KEYS.has() among the gates a case variant slips, but the hardcoded-tier fix left the reload-only keys (QWEN_SERVER_TOKEN, PATH, HOME, TMPDIR, …) on exact-case matching. On Windows a lowercase twin names the same OS variable, so a mid-session settings.env/.env edit could still rotate the daemon token or move PATH through a case respelling. Fold the reload tier the same way and pin it with a reload-behavior regression test. Also note DEV in the settings.md exclusion docs.
…meticity The source-scoped warning filters fixed the warning-count assertions, but the process.env assertions (e.g. 'never applies entrypoint or trust-anchor keys') still read state a real home .env can pollute: home scope deliberately bypasses the hardcoded exclusions, so a dev machine with QWEN_CLI_ENTRY in ~/.env applies it and fails the test while CI stays green. Redirect HOME/USERPROFILE to an empty temp dir in beforeEach — verified by running the suite with HOME pointed at a poisoned home.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round summary — no action required (PR #8663)What this round received
What was verifiedEach maintainer confirmation was checked against the exact code at HEAD (
OutcomeNothing to change this round: no new findings, no failed checks, and every confirmation matches the code at HEAD. No commit was made; the branch stays at Verification
中文说明Autofix 轮次总结 — 本轮无需任何改动（PR #8663）本轮收到的内容
已核实的内容每条维护者确认都对照 HEAD（
结论本轮无需任何改动：没有新发现，没有失败的检查，每条确认都与 HEAD 上的代码一致。未创建提交；分支保持在 验证
Deferred non-Critical feedbackCritical-only mode is active after 10 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明完成 10 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Approving — reviewed at 97dedd108c. C=0.
All 68 threads on this PR are marked resolved (7 Critical, 35 Suggestion, 26 untagged), and the standing CHANGES_REQUESTED is qwen-code-ci-bot at 23829a47c4 with five commits landed since — including two that are the fixes for its own last two findings. Nothing is actually outstanding.
I did not read the resolved flags as evidence. The seven Criticals here form an escalating series about denylist completeness, and every one of them is a pure predicate, so I re-derived their exact bypass spellings against the real isLoaderEnvKey / isHardcodedProjectEnvExclusion.
Critical audit — 7/7, probed at the exact spellings
| Thread | Verdict at 97dedd108c |
|---|---|
R1-1 initial (non-reload) .env path re-populates loader keys |
fixed — canApplyParsedEnvKey rejects loader keys unconditionally, not just under reload |
R2-1 loadServeFastPathEnvironment bypasses the loader check on the default serve route |
fixed — both application loops now gate on isLoaderEnvKey before the env freeze |
R3-1 npm_config_node_options absent from every list |
fixed — probed true |
R4-1 case-sensitive matching (NPM_CONFIG_NODE_OPTIONS) |
fixed — probed true for upper, lower and Npm_Config_Node_Options |
R5-1 npm underscore↔hyphen equivalence (npm_config_node-options) |
fixed — probed true, including NPM_CONFIG_NODE-OPTIONS |
R6-2 DEV spoofable from a project .env |
fixed — DEV is now a hardcoded project-env exclusion |
| R6-3 hardcoded tier matched exact-case (Windows case-insensitivity) | fixed — probed true for DEV/dev/Dev/dEv |
0 mismatches across the full sweep: all 11 declared INHERITED_LOADER_ENV_KEYS members in upper/lower/original spelling, plus hyphen variants for all five npm_config_* keys, plus the documented-but-unlisted BASH_FUNC_* prefix rule (BASH_FUNC_foo%%, BASH_FUNC_x(), and case-folded bash_func_foo%% all match, while BASHFUNC_foo correctly does not). Negative controls hold too — PATH, HOME, MY_APP_TOKEN and notably npm_config_registry are not swept up, so the widened matching did not become a blanket npm_config_* strip.
One thing worth recording: my probe initially flagged NODE_REPL_EXTERNAL_MODULE as a miss. That was my error, not a gap — it only affects node's interactive REPL, not node <script> launches or npm run lifecycle scripts, so it is not loader-effective for a daemon that spawns scripts. Its absence from the list is correct.
What makes this reviewable
The denylist carries its rationale inline — why each npm config key is a hijack one level up, why ZDOTDIR is the zsh analogue of BASH_ENV, and why a blanket child-env strip was rejected in favour of per-surface gates (trust-gated overrides that must keep working). It also names a known adjacent gap rather than hiding it: the LSP .lsp.json path keeps its own narrower SECURITY_SENSITIVE_ENV_KEYS that is missing BASH_ENV/ENV/npm_config_node_options, with the deferral justified by that surface being behind --experimental-lsp. That is the right call to defer, and the right way to record it — but it is the obvious follow-up: the two lists should converge before LSP leaves experimental, or the same class reopens through a second door.
Tests
206 pass locally at this commit — daemon-worker 78, fast-path 84, environment 23, shared-env-keys 21 — plus process-env-guard 3. CI is green on this head for web-shell smoke and coverage; review-pr is still running.
Caveat on how I ran them: I overlaid this PR's 16 changed files (plus two dependencies from its newer base, acp-channel-fallback.ts and trust-precedence.ts) onto a working checkout of another branch, because full-tree extraction kept stalling in my environment. All five suites resolved and passed, but that is not byte-identical to a clean checkout of 97dedd108c; CI is the authority for the whole-repo result.
中文说明
批准 —— 审查提交 97dedd108c，C=0。
本 PR 全部 68 条线程均标记为已解决（7 Critical、35 Suggestion、26 无标签）；尚存的 CHANGES_REQUESTED 来自 qwen-code-ci-bot 在 23829a47c4，而此后已落地五个提交，其中两个正是针对它最后两条发现的修复。实际没有遗留项。
我没有把 resolved 标记当作证据。这里的七个 Critical 构成一条关于denylist完备性的递进序列，且每一条都是纯谓词，因此我针对真实的 isLoaderEnvKey / isHardcodedProjectEnvExclusion 重新推导了它们的确切绕过拼写。
7/7 全部按确切拼写实测为已修复：R1-1 初始（非 reload）.env 路径现无条件拒绝 loader 键；R2-1 serve 快路径两个应用循环均在 env 冻结前加了 isLoaderEnvKey 门；R3-1 npm_config_node_options 实测 true；R4-1 大小写变体（含 Npm_Config_Node_Options）实测 true；R5-1 下划线↔连字符等价（含 NPM_CONFIG_NODE-OPTIONS）实测 true；R6-2 DEV 已纳入硬编码项目级排除；R6-3 硬编码层已大小写折叠（DEV/dev/Dev/dEv 均 true）。
全量扫描 0 处不符：11 个已声明的 INHERITED_LOADER_ENV_KEYS 成员在大写/小写/原拼写下全部命中，五个 npm_config_* 键的连字符变体全部命中，文档中说明但未列出的 BASH_FUNC_* 前缀规则也命中（含大小写折叠），而 BASHFUNC_foo 正确地不命中。反向对照同样成立——PATH、HOME、MY_APP_TOKEN，尤其是 npm_config_registry 不会被误纳，说明放宽匹配没有变成对 npm_config_* 的一刀切剥离。
一处需记录：我的探针最初把 NODE_REPL_EXTERNAL_MODULE 标为漏项。那是我的错误而非缺口——它只影响 node 的交互式 REPL，不影响 node <script> 启动或 npm run 生命周期脚本，对一个派生脚本的 daemon 而言并非 loader-effective。它不在列表中是正确的。
值得称道之处：denylist 内联记录了理由（为何每个 npm config 键是更上一层的劫持、为何 ZDOTDIR 是 BASH_ENV 的 zsh 对应物、为何拒绝一刀切剥离子进程环境而改为按面加门）。它还主动点名了一处已知的邻接缺口：LSP .lsp.json 路径保留自己更窄的 SECURITY_SENSITIVE_ENV_KEYS，缺少 BASH_ENV/ENV/npm_config_node_options，并以该面处于 --experimental-lsp 之后为由说明了延后。延后是对的，记录方式也是对的——但这是显而易见的后续项：两份列表应在 LSP 脱离实验阶段前收敛，否则同类问题会从第二道门重开。
测试：本提交下本地 206 通过（daemon-worker 78、fast-path 84、environment 23、shared-env-keys 21，另 process-env-guard 3）；CI 在 web-shell smoke 与覆盖率上已绿，review-pr 仍在运行。
关于运行方式的说明：我的环境下整树解包反复卡死，因此把本 PR 的 16 个改动文件（外加其较新 base 上的两个依赖 acp-channel-fallback.ts、trust-precedence.ts）覆盖到另一分支的可用检出上运行。五个套件均正常解析并通过，但这与 97dedd108c 的干净检出并非逐字节一致；全仓库结果以 CI 为准。
|
@qwen-code /triage |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM — approving at 97dedd108c. The whole Critical series on loader-key denylist completeness (initial .env load path, serve fast path, npm_config_node_options with case and underscore↔hyphen variants, DEV spoofing, Windows case-insensitivity) is closed with mutation-probed regression tests, consistent with @doudouOUC's independent probe sweep at this head. The freeze-before-scrub ordering keeps dev-mode boot intact while every session subprocess comes out clean. Remaining PATH-prefix / INIT_CWD residual is explicitly scoped out and tracked as follow-up — non-blocking.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Verified at head 97dedd1: all nine round-6 findings are fixed — DEV is in the hardcoded exclusions so a workspace .env can no longer spoof the dev gate, every exclusion gate goes through the case-folded check, the scrubbed launch env is restored when startup fails, and the tests redirect HOME/USERPROFILE for hermeticity. I also independently walked the boundaries: daemon process.env + frozen baseEnv, the ACP child after relaunch/sandbox handoff, the channel daemon worker, and every .env/settings.env application path rejecting loader keys; the freeze-before-scrub ordering holds, and the DEV exception only affects child boot — children self-scrub before hosting sessions, so protection composes across the relaunch chain. The key list is pinned by exact-array tests, so a silently shrunk list fails. PATH stays out of scope as stated in Risk & Scope. CI green on this head (mac/windows and integration are skipped for same-repo PRs, consistent across all commits). Nothing blocks merge.
✅ Verification report (local, isolated container) — merge-ready44/44 scripted assertions passed, 0 failed; no blocking findings. Advisory evidence for human reviewers — not a review, an approval, or a CI check.
中文摘要结论：merge-ready — 44/44 条脚本化断言全部通过，无阻塞性发现。
Central claim + A/BClaim: daemon/ACP-hosted session subprocesses must not inherit loader-affecting env vars ( Harness (mock-free): daemon launched from ws-a with a poisoned loader env (
Base session env (verbatim): FindingsNo blocking or material findings. The central change is load-bearing: base leaks, head scrubs, same harness both sides.
Targeted gates (head)522 tests passed, 0 failed across all 7 changed/related suites: Vacuity (new tests are load-bearing)
Not covered
MethodologyPR code ran only inside a credential-free container; metadata resolved outside via read-only Evidence images
|
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round: no action takenThis round triaged the feedback newer than the last evaluation (2026-08-08T14:14:15Z) and found nothing actionable:
Critical-only mode is active for this PR after 10 change-producing rounds. The non-Critical feedback listed in the deferred section (the automated reviewer's PR comment) remains open for human follow-up but is excluded from this round's actionable scope per the workflow's ratchet rule, so no code changes, thread resolutions, or comment replies were made for those items. No code changes were made and no commits were added; the PR head remains at the previously verified commit. 中文说明Autofix 审查轮次：无需处理本轮对上次评估（2026-08-08T14:14:15Z）之后的新反馈进行了分类，未发现任何可处理项：
本 PR 在完成 10 个产生改动的轮次后已进入仅处理 Critical 的模式。按工作流的棘轮规则，延后区域中列出的非 Critical 反馈（自动化审查器的 PR 评论）仍保持开放、留待人工跟进，但不属于本轮的可处理范围，因此未针对这些条目做任何代码修改、线程解决或评论回复。 本轮未做任何代码修改，也未新增提交；PR 的 head 仍停留在先前已通过验证的提交上。 Deferred non-Critical feedbackCritical-only mode is active after 10 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明完成 10 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
中文说明
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
…ycle Follow-up to #8663. Its inherited-env denylist closed the NODE_OPTIONS/ NODE_PATH class but left sibling code-execution and TLS-trust-anchor vars that reach the same #8653 cross-workspace outcome — an untrusted workspace `.env` is frozen into daemonRuntimeBaseEnv and distributed to every workspace's session subprocesses. Denylist additions, split by the PR's own tiering: - Scrubbed loader tier (INHERITED_LOADER_ENV_KEYS — scrubbed from the inherited launch env and rejected from every `.env`/settings.env scope), for pure-injection vars with no legitimate operator-shell use: OPENSSL_CONF (startup dlopen of an attacker OpenSSL engine), NODE_REPL_EXTERNAL_MODULE, npm_config_node_gyp, npm_config_init_module. - Reject-from-project-`.env` tier (PROJECT_ENV_HARDCODED_EXCLUSIONS — rejected from project files, preserved from the shell / home `.env`), for vars with a legitimate operator-shell use whose only exposed vector is an untrusted project file: * TLS trust anchors SSL_CERT_FILE, SSL_CERT_DIR, CURL_CA_BUNDLE, REQUESTS_CA_BUNDLE, GIT_SSL_CAINFO (siblings of NODE_EXTRA_CA_CERTS; an attacker CA MITMs a session's git/npm/pip/curl traffic). * git command-execution family GIT_SSH_COMMAND, GIT_EXTERNAL_DIFF, GIT_CONFIG_GLOBAL/SYSTEM/COUNT and the numbered GIT_CONFIG_KEY_<n>/ GIT_CONFIG_VALUE_<n> pairs (matched by prefix). core/utils/git-branches.ts already scrubs these from the repo's own git invocations. * node-gyp interpreter selection NODE_GYP_FORCE_PYTHON, npm_config_python, PYTHON (run as the build Python during native-addon installs). Concurrency: the daemon's process.env scrub/restore and the loader-key rejection reporter were process-global with no guard for concurrent embedded daemons in one process (a documented supported config). The first daemon's close() restored loader vars into the shared env, re-poisoning a still-live sibling's sessions, and dropped its reporter. The scrub is now reference counted (acquireInheritedLoaderEnvScrub — snapshot on first acquire, restore only on last release) and the reporter is cleared only when still active. Test hardening from the same review: pin the daemon-worker scrub breadcrumb (not just key removal); pin the fast-path settings.env case-folded hardcoded-exclusion gate; drain the module-global fast-path stash so the accumulate assertion is order-independent. Docs updated for the new keys.
…eak (QwenLM#8816) * feat(ci): A/B deterministic gate rejections against the pre-round ref A deterministic rejection in the autofix verification gate is only chargeable to the round if the same check passes without the round's commit. The gate charged every red to the fix unconditionally, and run 31276008548 measured what that costs when the premise is false: PR 8614's branch predated QwenLM#8693's tsconfig guard while node_modules came from the post-QwenLM#8693 trusted base, so `npm run build` was equally red at origin/<branch> — 63 minutes of accepted agent work discarded, an 18-minute repair burned on a failure the repair agent is forbidden to touch (it may only amend the round's own fix), thirteen rounds in a row, and the same again on the QwenLM#8616 leg. On rejection the gate now re-runs the failing check at origin/<branch> (the branch as pushed, before the round) in the same environment: - baseline green: today's path exactly — outcome=failed, retryable=true, the repair pass gets its chance. - baseline red too: outcome=failed with preexisting=true and NO retryable. The repair step keys on retryable and is skipped — it cannot reach a failure outside the round's diff by construction — and gate-rejection.md says outright that the branch needs a base update (merge main), which flows into the failure comment as-is. Fail-closed toward today's semantics: any A/B infrastructure problem (missing ref, checkout failure) charges the fix as before, and a restore failure after the baseline run rejects outright since the tree can no longer be trusted. The round's work is still not pushed — this changes the verdict's honesty and cost, not the push policy. Tested by executing the real script in a real two-remote git repo with an npm stub whose failures are keyed by commit SHA: round-caused red (baseline green), pre-existing red (both red), and the untouched green path. Mutation-tested, 3 of 3 caught: skipping the A/B, claiming pre-existing without measuring, and dropping the tree restore. * Address review: bound the A/B to checks it can honestly compare All seven findings verified before fixing; the three Criticals were each a way the A/B compared something other than the check that failed. R1-1 — the contracts check feeds on stdin, which its first run drains; the baseline leg re-ran against EOF and checked an empty file list. R1-3 — the schema check's verdict rides on packages/core/dist, which the core-rebuild guard built from ROUND sources and which, being gitignored, survives the detach. Both checks are now A/B-exempt (run_check_no_ab): their baseline verdicts prove nothing, and their rejections stay where the repair agent can actually act on them. R1-2 — a workspace the round ADDS does not exist at the baseline, and npm exits 1 there with "No workspaces found" (measured; --if-present forgives a missing script, not a missing workspace) — a round-caused failure misread as pre-existing, skipping the one repair that can fix the round's own package. The per-package loop now A/Bs only when the workspace exists at origin/<branch>. R1-4 — a chatty PASSING baseline used to flood the tail -c 3000 evidence window and push the actual failure text out of gate-rejection.md, the sole carrier into the repair feedback, the PR comment, and the next round's LAST_REJECTION. The baseline transcript now goes to a side log and only a FAILING tail is merged back, where it is the evidence. R1-5 — the pre-existing paragraph pushed gate-rejection.md past the report's head -c 3500 cap, truncating the closing fence for branch names past 44 characters. Cap raised to 3900, invariant comment updated with the new arithmetic. R1-6 — preexisting=true had no read site. It now flows verify → Finalize verification → the failure report, whose headline swaps the generic gate clause for "PRE-EXISTING failure … needs a base update (merge main)". R1-7 — the no-round-commit guard was unpinned (deleting it kept all tests green). Now exercised through the core-rebuild path, the one A/B-eligible check that runs before the commit gate. Four new behavioral scenarios (chatty baseline, no-commit round, A/B-exempt checks, round-added workspace) plus workflow pins for the forwarding, the clause, and the cap. Mutation-tested, 4 of 4 caught: schema back to A/B (3 tests), guard dropped, side log reverted, no-commit guard dropped. * Address review round 2: A/B only what it can prove, prove what it claims Ten findings across two rounds, each verified before fixing. The three deepest share one lesson: the A/B is only sound for a check whose inputs travel entirely with the git ref, and whose failure it can IDENTIFY, not merely observe. R2-1 — rc=1 at both legs does not make them the same failure: the branch can fail for reason A while the round fails for reason B, and a baseline infrastructure hiccup is a nonzero exit too. Pre-existing now requires a MATCHING failure identity — tsc diagnostics normalized to file + error code (positions shift with the round's edits), compared via comm(1) on a per-check transcript. No diagnostics on either side means identity cannot be established and the round stays charged. R2-2 / R2-7 — gitignored dist survives the detach carrying the ROUND's build, so any dist-consuming check A/Bs reverted sources against round-built artifacts: package tests (channel-base resolved through dist exports) and typecheck (sdk-typescript resolves core's d.ts — probe-verified three-arm flip). Both are now A/B-exempt, as is lint, leaving `npm run build` — the incident class, and the one check that rebuilds its own inputs from the checked-out sources — as the sole A/B candidate. The workspace-existence guard dissolves with it. R2-3 — the fixture inherited the caller's global git config; a failing global pre-commit hook broke all seven cases. The harness now isolates GIT_CONFIG_GLOBAL/SYSTEM for every git child, and the suite is proven green under a deliberately hostile hooksPath. R2-4 — Finalize verification now selects preexisting from the same attempt whose outcome it selects (repair verification included). R2-5 / R2-8 — the "merge main" advice is now conditional at both layers: the script paragraph states the measured fact and hedges the remedy; the report headline uses the compare the step already ran — behind/diverged gets the base-update clause, an up-to-date branch is told its own pre-round code needs attention. R2-6 — the rejection document now sizes its evidence tail against its preamble (floor 500 bytes, total under the 3900-byte render cap), so the closing fence can no longer be truncated off by a long branch name. R2-9 — dissolved by R2-2: package tests no longer A/B, the guard and its uncovered positive branch are gone. R2-10 — the baseline-evidence merge is now pinned: the pre-existing scenario asserts the baseline leg's own failure line (keyed by its SHA) reaches gate-rejection.md. Eight behavioral scenarios; mutation-tested 5 of 5: identity dropped, typecheck re-enrolled, package tests re-enrolled, evidence merge dropped, fixed tail restored. * Address review round 4: sharpen identity, stage the git failures, sync prose Nine findings, all refinements — the design held, the edges did not. Identity now keeps the diagnostic MESSAGE (file + code collide: two unrelated TS2339s in one file compared equal, skipping a repair that could have shipped — probe-reproduced by the review), and the fixture emits a SHIFTED position on the baseline leg so the position strip is load-bearing instead of decorative (deleting the sed survived every test before; it fails one now). vite/esbuild failures still yield an empty signature by design — documented as the fail-closed limit rather than half-widened. The fail_signature assignments take `|| true`: grep exits 1 on the normal no-match case and survives errexit today only because the caller sits in an if-condition — a future unconditional call site would crash the gate verdict-less. The restore-failure branch is now stageable and staged: the baseline leg recreates (untracked) a file the branch tracks, the checkout back refuses, and the test pins retryable-not-preexisting with the 'could not restore' label. Relaxing the branch to `|| true` fails it. Prose synced to the mechanisms that replaced it: the render-cap invariant restates against the dynamic tail budget (the old 3000-based arithmetic would misguide the next retune), the no-round-commit guard comment names the core rebuild (schema/contracts left the A/B last round), the describe wording counts both A/B-eligible builds, and the pre-existing clauses no longer claim "the repair pass was skipped" — with REPAIR_PREEXISTING forwarded, repair may have RUN; they now state the invariant that is true either way: repair may only amend the round's own fix, so it cannot reach this failure. Mutation-tested, 3 of 3 caught: position strip dropped, message dropped from the identity, restore rejection relaxed. * fix(ci): watchdog silent sandbox hangs and reap the containers they leak Four autofix rounds have died the same way (QwenLM#8663 twice, QwenLM#8761 r3, QwenLM#8763 r4): the agent's last output is the sandbox wrapper's "ContainerName (regular): …" line at docker container entry, then nothing — not one event — until the 2-hour absolute budget kills the round. Four different runners, two image versions: systemic, not a bad machine. Where exactly the container wedges is still unknown (that needs docker state on the runner); what is certain from the logs is the shape — a wedged sandbox produces NOTHING, and a legitimate run is never silent for long (the fleet's longest tolerated quiet is the review pipeline's 10-minute stream-idle window for thinking phases). Two mitigations, each aimed at a measured half of the damage: - run-agent.mjs gains an idle watchdog (QWEN_IDLE_TIMEOUT_MS, default 20 minutes = 2x that longest legitimate silence): zero output for the window kills the agent with a distinct "idle-timeout … the sandbox likely hung at startup" detail, so the failure comment names the right knob and a hung round costs 20 minutes instead of 120. Polled, not reset-per-chunk — a busy stream should not spend its time re-arming timers. - Both sandboxed jobs reap stale qwen-code-* containers at job start: a budget kill reaps the HOST-side docker client, not the container, so every killed sandbox keeps running on the persistent runner — observed directly when a later leg's container-name counter found qwen-code-0.21.8-0 already occupied and picked -1. One job per runner at a time makes any container alive at job start stale by definition. Tested by executing the real run-agent.mjs end to end with stub agents: the hang shape (one line, then silence) dies at the idle window naming the idle limit, and a slow-but-talking agent that outputs every 400ms across a 1500ms window survives to a clean exit — the test that distinguishes a watchdog from a disguised absolute timer. Mutation- tested, 3 of 3 caught: watchdog disabled, last-output tracking dropped (the disguised-timer regression), cleanup dropped from a job. * Address review round 5: the gate's verdict defects and the reaper's live kill Budget-warning round — the five Criticals from both reviewers, no suggestions (each deferred with a recorded reply). fail_signature: `[^\n]*` in an ERE bracket expression does not mean "rest of line" — in POSIX bracket expressions `\` is literal, so it matched "neither backslash nor the letter n" and truncated every tsc message at its first n. Nearly every real message has an early n ("Cannot find name", "is not assignable"), so distinct same-file failures collapsed into identical signatures and a round-caused failure could be labeled pre-existing, skipping the repair. grep is line-oriented: `.*` is exactly the rest of the line. New fixture: two messages differing only after their first n. Pre-existing verdict: the intersection test mislabeled in both directions. A round that ADDS a diagnostic sharing one normalized line with the baseline was called pre-existing (repair skipped for a round-caused, repairable failure); and `comm -12 | grep -q` under `set -eo pipefail` SIGPIPEs comm (exit 141) once the shared output outruns the pipe buffer, charging true pre-existing failures to the round — the exact 18-minute repair waste the gate exists to kill. Pre-existing now means the round's failing set is a SUBSET of the baseline's, and the difference is captured before testing. New fixture: a round adding a second diagnostic to a failing baseline. Restore failure after the baseline leg: was retryable=true with HEAD still detached at the baseline commit — the repair agent works in that very checkout and does no git recovery, so its commit would land on the baseline and be orphaned. Now rejected non-retryable (reject_fix grows a third arg); the next round starts clean from the trusted checkout. The restoreClash test pins the new semantics. Stale-container reap: the premise "a runner runs one job at a time, so any live qwen-code-* container is stale" holds per runner registration, but the filter queries the docker daemon, which is per host — and this pool runs several registrations on one OS. With per-issue/PR serialization only, a concurrent job's sandbox is a substring match away from `docker rm -f`. The reap now takes only provably-dead containers (--filter status=exited/dead, both jobs) and the comment says why a running one is left alone. Preamble printf: the `\`` escapes sat inside a single-quoted format where backslash is literal, so every pre-existing rejection rendered raw backticks instead of code spans (shellcheck SC2016). Backticks need no escaping there. Also syncs the side-log comment to the dynamic tail_budget it actually renders. Verified: scripts suite 140/140 (was 138; the two new fixtures and the rewritten restoreClash test all fail against the pre-fix script), npm run build / typecheck / lint pass, bash -n clean. * Address review round 6: reap the kill's own orphan, tolerate the reaper * Address review: hang-bound the reaper, unblock the kill path, pin the unpinned arms - Wrap every docker call in the stale-container reap with timeout 30: an alive-but-wedged daemon blocks docker ps indefinitely, and the existing || guards only catch nonzero exits, not hangs (R3-1). - Make the kill-path container removal async in run-agent.mjs: the spawnSync blocked the event loop between SIGTERM and the 10s SIGKILL backstop for up to its 30s timeout — in exactly the wedged-daemon scenario the watchdog exists for. The main flow awaits the removal so the leak warning stays deterministic (R3-6). - Split the pre-existing gate clause for an empty CMP_R: a transient compare-API failure is "never measured", not "measured not-behind", and must not assert the branch's own code is at fault (R3-7). - Swap the timeout breaker's closing remedy to the sandbox investigation when every counted timeout was idle, mirroring the round-level split (R3-11). - Tests: pin the budget kill path separately from the idle kill path (R3-3), parameterize the idle-window parse guard over -1/0/NaN (R3-5), add a stderr-only liveness case (R3-12), pin the strict-subset A/B arm via a baseline-superset fixture knob (R3-15), and pin the breaker's current-round idle increment (R3-18). --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-ci-bot <[email protected]> Co-authored-by: qwen-code-dev-bot <[email protected]>
… lifecycle (QwenLM#8763) * fix(cli): extend the QwenLM#8663 loader denylist and harden its scrub lifecycle Follow-up to QwenLM#8663. Its inherited-env denylist closed the NODE_OPTIONS/ NODE_PATH class but left sibling code-execution and TLS-trust-anchor vars that reach the same QwenLM#8653 cross-workspace outcome — an untrusted workspace `.env` is frozen into daemonRuntimeBaseEnv and distributed to every workspace's session subprocesses. Denylist additions, split by the PR's own tiering: - Scrubbed loader tier (INHERITED_LOADER_ENV_KEYS — scrubbed from the inherited launch env and rejected from every `.env`/settings.env scope), for pure-injection vars with no legitimate operator-shell use: OPENSSL_CONF (startup dlopen of an attacker OpenSSL engine), NODE_REPL_EXTERNAL_MODULE, npm_config_node_gyp, npm_config_init_module. - Reject-from-project-`.env` tier (PROJECT_ENV_HARDCODED_EXCLUSIONS — rejected from project files, preserved from the shell / home `.env`), for vars with a legitimate operator-shell use whose only exposed vector is an untrusted project file: * TLS trust anchors SSL_CERT_FILE, SSL_CERT_DIR, CURL_CA_BUNDLE, REQUESTS_CA_BUNDLE, GIT_SSL_CAINFO (siblings of NODE_EXTRA_CA_CERTS; an attacker CA MITMs a session's git/npm/pip/curl traffic). * git command-execution family GIT_SSH_COMMAND, GIT_EXTERNAL_DIFF, GIT_CONFIG_GLOBAL/SYSTEM/COUNT and the numbered GIT_CONFIG_KEY_<n>/ GIT_CONFIG_VALUE_<n> pairs (matched by prefix). core/utils/git-branches.ts already scrubs these from the repo's own git invocations. * node-gyp interpreter selection NODE_GYP_FORCE_PYTHON, npm_config_python, PYTHON (run as the build Python during native-addon installs). Concurrency: the daemon's process.env scrub/restore and the loader-key rejection reporter were process-global with no guard for concurrent embedded daemons in one process (a documented supported config). The first daemon's close() restored loader vars into the shared env, re-poisoning a still-live sibling's sessions, and dropped its reporter. The scrub is now reference counted (acquireInheritedLoaderEnvScrub — snapshot on first acquire, restore only on last release) and the reporter is cleared only when still active. Test hardening from the same review: pin the daemon-worker scrub breadcrumb (not just key removal); pin the fast-path settings.env case-folded hardcoded-exclusion gate; drain the module-global fast-path stash so the accumulate assertion is order-independent. Docs updated for the new keys. * fix(cli): keep the loader-scrub process.env access in the serve guard surface The refcounted acquireInheritedLoaderEnvScrub read/wrote process.env from config/shared-env-keys.ts, which the serve process.env guard does not scan — moving the access out of run-qwen-serve.ts dropped its allowlisted count and failed process-env-guard.test.ts. Pass the env into the coordinator instead so run-qwen-serve.ts still owns the process.env reference (matching the existing scrub helpers), and update the allowlist to the new count. * fix(cli): block GIT_SSH and GIT_CONFIG_PARAMETERS in the project-env denylist Co-authored-by: Qwen-Coder <[email protected]> * fix(cli): extend the project-env denylist across git exec, TLS, and rc-file tiers Close the round-2 review findings: block the remaining git command-execution siblings (GIT_EXEC_PATH, GIT_TEMPLATE_DIR, GIT_ASKPASS, GIT_PROXY_COMMAND, GIT_EDITOR), the npm/pip TLS trust knobs (npm_config_cafile, npm_config_ca, npm_config_strict_ssl, PIP_CERT, GIT_SSL_CAPATH), and the curl/wget rc-file redirects (CURL_HOME, WGETRC) from project .env files. Freeze the numbered GIT_CONFIG_KEY_/VALUE_ pairs on reload together with GIT_CONFIG_COUNT, and sync the qwen-serve.md loader-key enumeration with settings.md. * fix(cli): harden the project-env denylist and nested scrub snapshot (QwenLM#8763) * fix(cli): merge the loader-env scrub snapshot into one pass (QwenLM#8763) acquireInheritedLoaderEnvScrub iterated process.env twice (a snapshot pass, then the scrub); record the originals inside the scrub's single pass instead. Drop the acquire-time snapshot clear, which the release-time clear made unreachable defense, and add tests that kill the previously surviving mutants on the release-time clear, the test-only reset, and the undefined-value guard. * fix(cli): block the round-4 exec-redirect env keys from project files (QwenLM#8763) --------- Co-authored-by: qwen-code-dev-bot <[email protected]> Co-authored-by: Qwen-Coder <[email protected]>
4 participants
What this PR does
Daemon-mode sessions bound to one workspace inherited loader-affecting environment variables (
NODE_OPTIONSwith dev-harness--importregister hooks,NODE_PATH,LD_*/DYLD_*preload vars,BASH_ENV/ENV) from whatever shell launchedqwen serve, and passed them verbatim into every session subprocess. This PR scrubs that loader subset of the existing reload-exclusion list from the process environment at the two process boundaries that host sessions: the daemon, right after it freezes its boot environment, and each ACP child, after the relaunch/sandbox handoff. The frozen daemon boot env deliberately keeps the loader vars so dev-mode ACP children can still boot against the TypeScript source, and a respawned ACP child re-scrubs its own environment after boot, so protection composes across the relaunch chain. The reload-exclusion list now derives from the shared loader-key constant instead of duplicating its nine literals, and regression tests pin the scrub behavior, the exact key list, and the freeze-before-scrub ordering.Why it's needed
In a multi-workspace daemon setup a session working in checkout B spawned subprocesses carrying checkout A's harness state —
NODE_OPTIONS=--import .../qwen-code/node_modules/tsx/... --import /tmp/qwen-dev-*/register.mjs,NODE_PATHandPATHprefixes pointing at checkout A. The hijack was active, not cosmetic:import.meta.resolve('@qwen-code/qwen-code-core')from checkout B resolved into checkout A's tree, and running checkout B's built CLI failed with export errors answered by checkout A's stale source. Beyond confusing failures this is a correctness/safety gap: sessions can silently execute code built from another workspace. The existing.envreload exclusion already names exactly these keys, but the inherited launch environment bypassed it entirely.Reviewer Test Plan
How to verify
npm install && npm run build && npm run bundle).env NODE_OPTIONS="--import file:///tmp/repro/ws-a/register.mjs --expose-gc" NODE_PATH=/tmp/repro/ws-a/node_modules node dist/cli.js serve --port 4171 --hostname 127.0.0.1 --enable-session-shell --token repro-token(aregister.mjsthat logs each execution withprocess.cwd()makes the hijack observable).POST /session/:id/shellwithenv | grep -E 'NODE_OPTIONS|NODE_PATH'plusnode -e "console.log(typeof globalThis.gc)".NODE_OPTIONS/NODE_PATHare unset in the ws-B subprocess,globalThis.gcisundefined, and the ws-A loader never executes withcwd=ws-B. The loader still executes once per process at boot (daemon + each ACP child) — that is required for dev-mode boot and cannot be scrubbed before process start; everything each process subsequently spawns is clean.npm run devinteractive still runs from TypeScript source (the daemon's own children keep the harness loader via the frozen boot env).Evidence (Before & After)
Before (reproduced against v0.21.6): ws-B session subprocess printed
NODE_OPTIONS=--import file:///tmp/qwen-8653/ws-a/fake-harness/register.mjs --expose-gc,NODE_PATH=/tmp/qwen-8653/ws-a/node_modules,typeof gc === 'function', and the ws-A loader logged two executions withcwd=/private/tmp/qwen-8653/ws-b.After (verified against this branch's built bundle, same scenario): ws-B subprocesses have no
NODE_OPTIONS/NODE_PATH,typeof gc === 'undefined', and seven session subprocesses produced zero ws-A loader executions — only one loader execution per process at boot (daemon and each ACP child, both with their own cwd).Tested on
Environment (optional)
E2E verified with
node dist/cli.js serve --enable-session-shellagainst two temp workspaces plus a logging loader hook; unit tests:packages/clienv/serve suites (shared-env-keys, environment, process-env-guard, run-qwen-serve).Risk & Scope
NODE_OPTIONStuning (e.g.--max-old-space-size, OTel--require) is also scrubbed from session subprocesses of daemon/ACP-hosted sessions. ACP children get explicit memory args; this matches the existing policy that these keys never flow through.envreloads.PATHprefixes and informational vars (INIT_CWD,npm_package_json) are intentionally left intact — they cannot hijack module resolution once loader vars are gone, and sessions still need a workingPATH. Channel workers on embedded (createServeApp) runtimes are unchanged.Linked Issues
Fixes #8653
中文说明
本 PR 做了什么
daemon 模式下绑定到某个 workspace 的会话，会从启动
qwen serve的那个 shell 继承 loader 类环境变量（带 dev harness--importregister 钩子的NODE_OPTIONS、NODE_PATH、LD_*/DYLD_*preload 类变量、BASH_ENV/ENV），并原样传给每个会话子进程。本 PR 在宿主会话的两个进程边界上，从process.env中剥离既有 reload 排除列表的 loader 子集：daemon 在冻结启动环境之后立即剥离；每个 ACP 子进程在 relaunch/sandbox 交接之后剥离。daemon 冻结的启动环境刻意保留 loader 变量，以便 dev 模式下的 ACP 子进程仍能从 TypeScript 源码启动；被 respawn 的 ACP 子进程在启动后会再次自行剥离，因此保护在 relaunch 链上可组合。reload 排除列表改为派生自共享的 loader 键常量，不再重复维护那 9 个字面量；回归测试钉住了剥离行为、确切的键列表、以及“先冻结后剥离”的顺序。为什么需要
在多 workspace 的 daemon 场景下，工作在 checkout B 的会话所派生的子进程带着 checkout A 的 harness 状态——
NODE_OPTIONS=--import .../qwen-code/node_modules/tsx/... --import /tmp/qwen-dev-*/register.mjs、指向 checkout A 的NODE_PATH与PATH前缀。这个劫持是真实生效的，不只是看起来不对：从 checkout B 执行import.meta.resolve('@qwen-code/qwen-code-core')会解析进 checkout A 的目录树；运行 checkout B 构建好的 CLI 会因为 import 被 checkout A 的陈旧源码应答而报导出错误。除了令人困惑的失败之外，这也是正确性/安全隐患：会话可能静默执行另一个 workspace 构建出的代码。既有的.envreload 排除列表本来就点名了这些键，但从启动 shell 继承来的环境完全绕过了这一排除。评审者测试计划
如何验证
npm install && npm run build && npm run bundle）。env NODE_OPTIONS="--import file:///tmp/repro/ws-a/register.mjs --expose-gc" NODE_PATH=/tmp/repro/ws-a/node_modules node dist/cli.js serve --port 4171 --hostname 127.0.0.1 --enable-session-shell --token repro-token（用一个会打印每次执行时process.cwd()的register.mjs，可以让劫持可观测）。POST /session/:id/shell执行env | grep -E 'NODE_OPTIONS|NODE_PATH'和node -e "console.log(typeof globalThis.gc)"。NODE_OPTIONS/NODE_PATH未设置，globalThis.gc为undefined，且 ws-A 的 loader 从未以cwd=ws-B执行过。loader 仍会在每个进程启动时执行一次（daemon + 每个 ACP 子进程）——这是 dev 模式启动所必需的，无法在进程启动前剥离；每个进程此后派生的一切都是干净的。npm run dev交互模式仍从 TypeScript 源码运行（daemon 自己的子进程通过冻结的启动环境保留 harness loader）。证据（修复前后）
修复前（在 v0.21.6 上复现）：ws-B 会话子进程打印出
NODE_OPTIONS=--import file:///tmp/qwen-8653/ws-a/fake-harness/register.mjs --expose-gc、NODE_PATH=/tmp/qwen-8653/ws-a/node_modules、typeof gc === 'function'，且 ws-A 的 loader 记录了两次cwd=/private/tmp/qwen-8653/ws-b的执行。修复后（在本分支构建产物上用同一场景验证）：ws-B 子进程没有
NODE_OPTIONS/NODE_PATH，typeof gc === 'undefined'，7 个会话子进程产生零次 ws-A loader 执行——每个进程仅在启动时各执行一次 loader（daemon 与每个 ACP 子进程，cwd 均为各自所在目录）。测试环境
环境（可选）
E2E 用
node dist/cli.js serve --enable-session-shell对两个临时 workspace 加一个会打日志的 loader 钩子进行了验证；单元测试：packages/cli的 env/serve 相关套件（shared-env-keys、environment、process-env-guard、run-qwen-serve）。风险与范围
NODE_OPTIONS调优参数（如--max-old-space-size、OTel 的--require）同样会从 daemon/ACP 宿主会话的会话子进程中被剥离。ACP 子进程有显式的内存参数；这也与既有策略一致——这些键本来就不允许经由.envreload 注入。PATH前缀与信息性变量（INIT_CWD、npm_package_json）有意保留——一旦 loader 变量被剥离，它们就无法再劫持模块解析，而且会话仍然需要可用的PATH。嵌入式（createServeApp）运行时下的 channel worker 行为不变。关联 Issue
Fixes #8653