fix(hooks): close four trust-boundary holes in hook execution - #8396
fix(hooks): close four trust-boundary holes in hook execution#8396wenshao wants to merge 30 commits into
Conversation
The URL whitelist and DNS-level SSRF checks validate only the initial URL, but undici's default redirect:'follow' would re-POST the hook payload (prompts, tool inputs, session data) to any 307/308 target and connect to blocked metadata/private ranges on 30x — bypassing every guard the module exists to enforce. Pass redirect:'manual' so a 3xx lands in the existing non-2xx non-blocking error path. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The workspace merge already strips security.allowPrivateNetworkHooks so a repository cannot self-grant an SSRF relaxation, but the sibling whitelist security.allowedHttpHookUrls was still honored from workspace scope — letting a repo replace the user's hook-payload whitelist (e.g. with "*") and exfiltrate hook data past the boundary the user configured. Strip both; user/system scopes keep working. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Fix #7527 stripped INTERNAL_SECRET_ENV_VARS (daemon tokens, private ACP capability) from hook child environments, but two other env-construction paths read raw process.env with no denylist: settings resolution (envVarResolver, baking values into hook commands/URLs at settings load) and HTTP hook allowedEnvVars interpolation into URLs/headers sent over the network (envInterpolator). A repo-controlled settings file or hook config could name QWEN_SERVER_TOKEN and exfiltrate the daemon bearer token. Both paths now refuse those names; placeholders stay unresolved (children get a sanitized env anyway) or interpolate to empty. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Project subagents (.qwen/agents/*.md) and project skills (.qwen/skills/) are discovered regardless of folder trust — fine for instructions that only influence the model. But their frontmatter hooks are repo-supplied code execution, and both registration paths (addAgentHooks on subagent spawn, registerSkillHooks on skill invocation) ran unconditionally — so a malicious repo in an UNTRUSTED folder could get arbitrary commands registered for the session by inducing a subagent spawn or skill invoke, bypassing the folder-trust gate Config.getProjectHooks() applies to the same hooks declared in settings.json. Both paths now skip registration with a warning when the folder is untrusted; user-level configs are unaffected, matching the user-hooks trust semantics. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…lver copy Follow-up to 38754ef — the resolver exists in two copies (cli and core, already drifted apart on circular-reference handling). The core copy feeds extensionManager manifest resolution; a third-party extension could name QWEN_SERVER_TOKEN the same way a repo settings file could. Apply the identical denylist so the two copies agree on security semantics. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@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 冲突，直到移除标签或达到轮次上限。移除 |
|
Re-run at the new head ( Template looks good ✓ Problem: Still real holes, not theoretical hardening — each has a concrete attack narrative (redirect-to-metadata SSRF, a workspace replacing the user's hook whitelist, one settings entry exfiltrating the daemon bearer token, frontmatter hooks firing in an untrusted folder) and a unit test asserting the gated negative. The maintainer's own review traced each fix's completeness (single call sites for Direction: Aligned. Closing trust-boundary holes where repo-controlled config meets code execution / network egress is core mission, and the fixes extend existing semantics ( Size: Core paths are touched ( Approach: Scope feels right for what it became — four headline fixes plus the increments the maintainer explicitly asked for (intersection instead of stripping, Risk: No elevated risk signals — none of the changed files match the revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明在新 head（ 模板完整 ✓ 问题：仍是真实漏洞而非理论性加固——每个都有具体攻击路径（重定向到元数据的 SSRF、workspace 替换用户的 hook 白名单、一条 settings 配置外泄 daemon bearer token、不可信文件夹中触发 frontmatter hooks），且各有断言"被门禁的否定面"的单元测试。维护者本人的评审逐一追踪了每个修复的完整性（ 方向：对齐。在"仓库可控配置"与"代码执行/网络出口"交界处关闭信任边界漏洞属于核心使命，且修复是扩展已有语义（ 规模：触及核心路径（ 方案：就其现状而言范围合理——四个标题修复，加上维护者明确要求的增量（用求交替代剥离、显式决定 风险：无升级风险信号——改动文件均未命中与回滚相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent proposal first (title + motivation only): disable redirects at the client (
No correctness blockers, no security holes. The residue is non-blocking and mostly items the maintainer review already raised and the author deliberately answered: Files changed (30 of 36 shown)
TestingUnattended CI run — I did not build or execute any PR code; the evidence below is the PR's own CI on the reviewed commit, read via the API. The unit tests are load-bearing by construction: they assert the gated negatives (
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the remaining efficacy question: Not verified: I did not re-run the unit suite or the author's mutation checks myself (this run never executes PR code) — the "mutation-checked gates" and "tested on macOS" statements are the author's claims, consistent with but not substituted by the green CI above. Windows/macOS CLI test jobs are gated/skipped at this stage. 中文说明代码审查先写独立方案（仅凭标题与动机）：在客户端禁用重定向（
无正确性阻塞、无安全漏洞。遗留项均非阻塞，且多为维护者评审已提出、作者已有意回应过的： 测试无人值守的 CI 运行——我未构建或执行任何 PR 代码；以下证据是 PR 自身在被审 commit 上的 CI，经 API 读取。单元测试在构造上即承重：断言被门禁的否定面（客户端收到 沙箱验证可以收尾剩余的"有效性"问题： 未验证：我未重跑单元测试或作者的 mutation 验证（本运行从不执行 PR 代码）——"门禁做过 mutation 验证"与"在 macOS 上测试"是作者声明，与上面的绿色 CI 一致，但不以其替代。Windows/macOS 的 CLI 测试 job 在此阶段被门禁/跳过。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review; the cap is pure policy — ~730 production-logic lines in core paths trip the Stage 0 maintainer-awareness escalation, and that path never auto-approves regardless of how clean the stages looked. Stepping back: this is the version of the PR I'd want to merge. The implementation matches my independent proposal on all four holes and exceeds it exactly where the maintainer's review asked for more — intersection instead of stripping keeps the narrowing use case, the Why defer instead of approve: the core-size escalation policy requires a human maintainer's sign-off on a change this size, and that is a rule I apply mechanically rather than waive on vibes — including the vibe that here it's arguably satisfied already, since the author is a maintainer and @doudouOUC approved exactly this commit a few hours ago. I'm not registering an approval vote from this run; that's the point of the defer. ⏸️ Deferring to @doudouOUC (most recent human reviewer; @wenshao as author/maintainer) — the review itself found nothing blocking, and an approving review already stands on this exact head, so this is the policy's final human checkpoint rather than an open question: merge if you're satisfied, or say what you'd like changed. Needs a human call on this one. 中文说明置信度：3/5 —— 审查本身干净；封顶纯因策略——核心路径约 730 行生产逻辑触发了 Stage 0 的"维护者知悉"升级，该路径无论各阶段多干净都不自动批准。 退一步看：这是我会愿意合并的版本。实现在四个漏洞上都与我的独立方案一致，并且恰好在维护者评审要求更多的地方超出了它——用求交替代剥离保留了"收窄"用法， 为什么是 defer 而非 approve：核心规模升级策略要求这个体量的改动由人类维护者签核，这是一条我机械执行、不凭感觉豁免的规则——哪怕这里的感觉是"其实已经满足了"：作者本人是维护者，且 @doudouOUC 几小时前刚在这个 commit 上批准。本轮不投批准票；这正是 defer 的意义。 ⏸️ 转交 @doudouOUC（最近的人类评审；@wenshao 为作者/维护者）——审查本身未发现阻塞项，且这个 head 上已有一份批准评审，因此这是策略意义上的人类终检，而非悬而未决的问题：满意即合并，或指出想改的地方。这一处需要人来拍板。 — Qwen Code · qwen3.8-max Reviewed at |
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x), Post Coverage Comment (ubuntu-latest, 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x), Post Coverage Comment (ubuntu-latest, 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review — no code change this roundTriage. The only feedback newer than the last evaluation is two failed CI The failed checks are not caused by this PR's code. The workflow's own Local verification of the PR (all green). I reproduced the PR's behavior in
I also ran the full Conclusion. No evidence-backed code defect exists to fix, so no change is 中文说明Autofix 复查 —— 本轮不做代码改动分类。 相对上次评估，唯一的最新反馈是两个失败的 CI 检查（ 失败的检查并非由本 PR 的代码引起。 工作流自身的 base 更新步骤已经验证了这一点并记录在 PR 上：同样的两个检查在当前 main 上是通过的，已通过 update-branch 将 main 合入本分支，并重新运行了 CI。 本 PR 的本地验证（全部通过）。 我在 checkout 中复现了 PR 的行为，并运行了可信的必需检查：
我还运行了 结论。 不存在有证据支撑、可修复的代码缺陷，因此不做任何改动。为了强制重跑 CI 而凭空制造一处改动，只会是一个投机性的空操作。分支已经合入了当前 main 并触发了新一轮 CI；独立的 CI 门禁仍是最终裁决依据。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): 1385 tests green — this review observed 16960, 18998 passed.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：1385 tests green — this review observed 16960, 18998 passed。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| if (INTERNAL_SECRET_ENV_VARS.includes(varName)) { | ||
| return match; | ||
| } |
There was a problem hiding this comment.
[Critical] The INTERNAL_SECRET_ENV_VARS denylist is checked with a case-sensitive Array.includes(), but process.env is case-insensitive on Windows. A repo-controlled settings/hook config referencing a mixed-case spelling ($qwen_server_token) bypasses the denylist (the list holds uppercase constants) yet still resolves the daemon bearer token from process.env on win32 — the match regexes accept any casing. The same defect exists at packages/cli/src/utils/envVarResolver.ts:39 and packages/core/src/hooks/envInterpolator.ts:74. — Failure scenario: on Windows (a supported platform; Node documents process.env as case-insensitive there), a user who exported QWEN_SERVER_TOKEN opens a folder whose .qwen/settings.json hook command / MCP arg / HTTP-hook allowedEnvVars references $qwen_server_token; includes('qwen_server_token') is false, process.env['qwen_server_token'] returns the canonically-cased value, and the token is baked into the resolved command/URL/header and leaves the process — the exact exfiltration this PR closes on Linux/macOS stays open on Windows. A probe confirmed resolveEnvVarsInString('$qwen_server_token') returns the secret when both casings are in process.env, and flips to the blocked placeholder under a case-insensitive comparison.
Fix (spans all three sites, so a regular block rather than one-click): compare case-insensitively via a shared helper in sanitize-child-env.ts:
const INTERNAL_SECRET_ENV_VARS_UPPER = new Set(
INTERNAL_SECRET_ENV_VARS.map((v) => v.toUpperCase()),
);
export const isInternalSecretEnvVar = (name: string): boolean =>
INTERNAL_SECRET_ENV_VARS_UPPER.has(name.toUpperCase());then call if (isInternalSecretEnvVar(varName)) { … } at all three sites.
中文说明
INTERNAL_SECRET_ENV_VARS 拒绝列表用大小写敏感的 Array.includes() 检查，但 process.env 在 Windows 上大小写不敏感。仓库可控的 settings/hook 配置只要用混合大小写写法（$qwen_server_token）就能绕过拒绝列表（列表里全是大写常量），却仍能在 win32 上从 process.env 解析出 daemon bearer token——匹配正则接受任意大小写。同样的缺陷还存在于 packages/cli/src/utils/envVarResolver.ts:39 与 packages/core/src/hooks/envInterpolator.ts:74。失败场景：在 Windows（受支持平台；Node 官方文档说明此处 process.env 大小写不敏感）上，已导出 QWEN_SERVER_TOKEN 的用户打开一个文件夹，其 .qwen/settings.json 的 hook 命令 / MCP 参数 / HTTP-hook allowedEnvVars 引用 $qwen_server_token；includes('qwen_server_token') 为 false，process.env['qwen_server_token'] 返回规范大小写的值，token 被烤进解析后的命令/URL/请求头并离开进程——本 PR 在 Linux/macOS 上关闭的外泄路径在 Windows 上仍然敞开。探针已确认：当 process.env 中同时存在两种大小写时 resolveEnvVarsInString('$qwen_server_token') 返回 secret，改为大小写不敏感比较后恢复为被拦截的占位符。修复（涉及三处）：在 sanitize-child-env.ts 中用大小写不敏感的共享 helper（isInternalSecretEnvVar，对列表做 toUpperCase() 后用 Set 查找），并在三处调用点替换。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { INTERNAL_SECRET_ENV_VARS } from '@qwen-code/qwen-code-core'; |
There was a problem hiding this comment.
[Critical] This import pulls INTERNAL_SECRET_ENV_VARS from @qwen-code/qwen-code-core — the whole package index. fast-path-settings.ts statically imports this resolver and is dynamically imported by the serve fast-path bootstrap before runQwenServe, so evaluating it now pulls the entire core module graph into the fast path. — Failure scenario: every qwen serve fast-path invocation now evaluates the whole core graph before the HTTP listener is handed to runQwenServe; the first runQwenServe call lands ~3831 ms after tryRunServeFastPath (this import alone costs ~3.6 s — no throw, the call is simply late) versus <1 s at base, so both tests in src/serve/fast-path-open.test.ts deterministically time out their 1000 ms vi.waitFor (measured netNew: fails in isolation on this PR, passes in isolation on the merge base) — and every cold qwen serve startup pays the ~3.6 s the fast path's deferred-import design exists to avoid.
Fix: don't import the whole core index here — consume the constant from a narrow leaf subpath (expose/consume a subpath export for utils/sanitize-child-env.js, as the CLI vitest config already does for other core subpaths), or inline the short frozen list in this file so the fast-path module graph stays free of the core index.
中文说明
此 import 从 @qwen-code/qwen-code-core（整个包索引）引入 INTERNAL_SECRET_ENV_VARS。fast-path-settings.ts 静态引入本解析器，而 serve 快速路径引导会在 runQwenServe 之前动态引入 fast-path-settings.ts，因此求值本文件会把整个 core 模块图拖进快速路径。失败场景：每次 qwen serve 快速路径调用现在都会在把 HTTP listener 交给 runQwenServe 之前求值整个 core 图；第一次 runQwenServe 调用落在 tryRunServeFastPath 之后约 3831 ms（仅这个 import 就耗约 3.6 s——不抛错，只是迟到），而 base 上 <1 s，于是 src/serve/fast-path-open.test.ts 的两个测试都会确定性地超过其 1000 ms vi.waitFor（实测为净新增：在本 PR 上单独运行失败，在合并基上单独运行通过）——并且每次冷启动 qwen serve 都要付出这约 3.6 s，而快速路径的延迟引入设计正是为了避免它。修复：不要在此引入整个 core 索引——从窄的叶子子路径引入该常量（为 utils/sanitize-child-env.js 暴露/使用子路径导出，CLI 的 vitest 配置已对其他 core 子路径这样做），或在本文件内联这个简短的冻结列表，使快速路径模块图不依赖 core 索引。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| settings.security; | ||
| const { | ||
| allowPrivateNetworkHooks: _strippedFlag, | ||
| allowedHttpHookUrls: _strippedUrls, |
There was a problem hiding this comment.
[Suggestion] This PR newly strips security.allowedHttpHookUrls from workspace scope, but getSettingsWarnings() still only warns about allowPrivateNetworkHooks — so the whitelist strip is silent. — Concrete cost: a user who sets security.allowedHttpHookUrls in workspace .qwen/settings.json gets no startup diagnostic (a colleague setting allowPrivateNetworkHooks in the same file does); the whitelist is silently discarded and HTTP hooks fall back to the user-scope list or, if that is empty, the schema default (allow all), while the operator believes their configured boundary is in effect. The asymmetry is created by this diff — before it, the workspace whitelist was honored.
Fix: add a parallel warning in getSettingsWarnings() that fires when workspaceFile.originalSettings.security?.allowedHttpHookUrls !== undefined (naming both keys), and update the comment above that block to mention both settings.
中文说明
本 PR 新增从 workspace scope 剥离 security.allowedHttpHookUrls，但 getSettingsWarnings() 仍然只对 allowPrivateNetworkHooks 告警——因此白名单的剥离是静默的。具体代价：在 workspace .qwen/settings.json 中设置 security.allowedHttpHookUrls 的用户不会得到任何启动诊断（而在同一文件里设置 allowPrivateNetworkHooks 的同事会得到）；白名单被静默丢弃，HTTP hooks 回退到 user scope 列表，或在其为空时回退到 schema 默认（放行所有），而操作者以为自己配置的边界仍生效。这种不对称是本 diff 造成的——在此之前 workspace 白名单是被接受的。修复：在 getSettingsWarnings() 中增加一条并行告警，当 workspaceFile.originalSettings.security?.allowedHttpHookUrls !== undefined 时触发（并点名两个键），同时更新该告警块上方的注释以同时提及两个设置。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| expect(unregisterSpy).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('does not register hooks for a project-level subagent in an untrusted folder', async () => { |
There was a problem hiding this comment.
[Suggestion] No test covers a non-project-level subagent in an untrusted folder, so the config.level === 'project' conjunct of the new gate is unguarded — the mutation to plain !runtimeContext.isTrustedFolder() survives the whole file. Both new tests use level: 'project'; the pre-existing dispose test uses level: 'session' but never spies isTrustedFolder (which defaults to true). The sister change in skill.ts ships exactly this case ("registers hooks for a user-level skill regardless of folder trust"). — Concrete cost: if a future edit drops the level check, user-owned agents from ~/.qwen/agents (SubagentLevel includes user/extension/builtin) that declare frontmatter hooks would silently have those hooks ignored in an untrusted workspace, and the suite would not catch it.
Fix: mirror the skill.ts test —
it('registers hooks for a user-level subagent regardless of folder trust', async () => {
const addAgentHooksSpy = vi.fn().mockReturnValue(vi.fn());
vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({
getRegistry: () => ({ addAgentHooks: addAgentHooksSpy }),
} as unknown as ReturnType<Config['getHookSystem']>);
vi.spyOn(mockConfig, 'isTrustedFolder').mockReturnValue(false);
const result = await manager.createAgentHeadless(
{ ...baseConfig, level: 'user', hooks: { /* one command hook */ } },
mockConfig,
);
expect(addAgentHooksSpy).toHaveBeenCalledTimes(1);
await result.dispose();
});中文说明
没有测试覆盖"不可信文件夹下的非 project 级 subagent"，因此新门禁中 config.level === 'project' 这一合取项无人守护——把它突变为单纯的 !runtimeContext.isTrustedFolder() 后整个测试文件仍能通过。两个新测试都用 level: 'project'；既有的 dispose 测试用 level: 'session' 但从不 spy isTrustedFolder（其默认为 true）。skill.ts 中的姊妹改动恰好带了这个用例（"无论文件夹信任与否都为用户级 skill 注册 hooks"）。具体代价：如果未来某次编辑删掉了 level 检查，来自 ~/.qwen/agents（SubagentLevel 含 user/extension/builtin）、声明了 frontmatter hooks 的用户自有 agent，会在不可信 workspace 中被静默忽略 hooks，而测试套件无法捕获。修复：仿照 skill.ts 的测试，补一个 level: 'user' + isTrustedFolder 返回 false、并断言 addAgentHooksSpy 被调用一次的用例。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
…, workspace warning (#8396)
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed[Critical] Case-sensitive denylist bypass on Windows (rc:3700141961)Decision: Fixed. The Changes:
[Critical] Fast-path import pulls whole core module graph (rc:3700141963)Decision: Fixed. The CLI's Changes:
[Suggestion] Missing warning for stripped allowedHttpHookUrls (rc:3700141965)Decision: Fixed. This PR strips Changes:
[Suggestion] Missing test for non-project-level subagent in untrusted folder (rc:3700141968)Decision: Fixed. No test covered a non- Changes:
Verification
中文说明已处理的审查反馈[Critical] Windows 上大小写敏感的拒绝列表可被绕过 (rc:3700141961)决定： 已修复。
变更：
[Critical] 快速路径 import 拉入整个 core 模块图 (rc:3700141963)决定： 已修复。 CLI 的 变更：
[Suggestion] 缺少对已剥离的 allowedHttpHookUrls 的告警 (rc:3700141965)决定： 已修复。 本 PR 从 workspace scope 剥离 变更：
[Suggestion] 缺少不可信文件夹下非 project 级 subagent 的测试 (rc:3700141968)决定： 已修复。 没有测试覆盖不可信文件夹下的非 变更：
验证
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/模型 |
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. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Test Plan (not a blocker): 1385 tests green — this review observed 16964, 19001 passed.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 Test Plan（非阻断）：1385 tests green — this review observed 16964, 19001 passed。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| it('should block internal secrets regardless of casing (Windows process.env)', () => { | ||
| process.env['QWEN_SERVER_TOKEN'] = 'daemon-secret'; | ||
|
|
||
| const result = resolveEnvVarsInString( | ||
| 'curl https://x/t=$qwen_server_token', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The casing regression test is vacuous on the platform CI runs on. It sets only the uppercase key process.env['QWEN_SERVER_TOKEN'] but references the lowercase $qwen_server_token; on Linux process.env is case-sensitive, so even without the isInternalSecretEnvVar guard the lowercase lookup is undefined and every assertion still passes. A probe (removing the guard) confirmed the uppercase test fails but this casing test still passes — the exact round-1 Critical (case-sensitive denylist) would ship green on Linux CI. — Failure scenario: remove .toUpperCase() from isInternalSecretEnvVar (or the guard entirely) and all three casing tests stay green on Linux.
| it('should block internal secrets regardless of casing (Windows process.env)', () => { | |
| process.env['QWEN_SERVER_TOKEN'] = 'daemon-secret'; | |
| const result = resolveEnvVarsInString( | |
| 'curl https://x/t=$qwen_server_token', | |
| ); | |
| it('should block internal secrets regardless of casing (Windows process.env)', () => { | |
| process.env['QWEN_SERVER_TOKEN'] = 'daemon-secret'; | |
| process.env['qwen_server_token'] = 'daemon-secret'; | |
| const result = resolveEnvVarsInString( | |
| 'curl https://x/t=$qwen_server_token', | |
| ); |
The same gap exists in the parallel copies packages/cli/src/utils/envVarResolver.test.ts and packages/core/src/hooks/envInterpolator.test.ts — set the lowercase key there too (for the interpolator, so the whitelisted lookup would otherwise resolve it). Alternatively, add one platform-independent unit test for isInternalSecretEnvVar in packages/core/src/utils/sanitize-child-env.test.ts.
中文说明
大小写回归测试在 CI 运行的平台（Linux）上是空转的。测试只设置了大写键 process.env['QWEN_SERVER_TOKEN']，却引用小写的 $qwen_server_token；Linux 上 process.env 大小写敏感，因此即使没有 isInternalSecretEnvVar 守卫，小写查找也返回 undefined，所有断言依然通过。探针（移除守卫）确认：大写测试会失败，但本大小写测试仍然通过——第一轮的那个 Critical（大小写敏感的拒绝列表）在 Linux CI 上会绿灯通过。失败场景：从 isInternalSecretEnvVar 移除 .toUpperCase()（或整个守卫），三个大小写测试在 Linux 上仍全部为绿。同样的问题还存在于 packages/cli/src/utils/envVarResolver.test.ts 与 packages/core/src/hooks/envInterpolator.test.ts——请在那里也设置小写键（对 interpolator，使白名单查找否则会解析出它）。或者，在 packages/core/src/utils/sanitize-child-env.test.ts 中为 isInternalSecretEnvVar 增加一个平台无关的单元测试。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { isInternalSecretEnvVar } from '@qwen-code/qwen-code-core/sanitizeChildEnv'; |
There was a problem hiding this comment.
[Suggestion] This file is a near-byte-identical copy of packages/core/src/utils/envVarResolver.ts (only the import path and one doc-comment clause differ — the doc comments have already drifted in this PR). This change applied the same isInternalSecretEnvVar guard and the same two tests to both copies. — Concrete cost: the next resolver change (a new denylist entry, a regex fix) must be applied to both files; missing one silently leaves the CLI path unpatched while core is fixed, or vice versa.
The './sanitizeChildEnv' subpath-export mechanism this PR adds could equally export the resolver. Consolidate by mirroring that entry in packages/core/package.json exports (e.g. "./envVarResolver" → ./dist/src/utils/envVarResolver.js), adding the matching alias in packages/cli/tsconfig.json and packages/cli/vitest.config.ts, deleting this copy and its test, and pointing the two CLI import sites (settings.ts, fast-path-settings.ts) at core.
中文说明
本文件与 packages/core/src/utils/envVarResolver.ts 几乎逐字节相同（仅 import 路径与一处文档注释不同——文档注释在本 PR 中已经漂移）。本次改动把相同的 isInternalSecretEnvVar 守卫与相同的两个测试同时应用到两份拷贝。具体代价：下一次解析器改动（新增拒绝列表项、正则修复）必须同时改两个文件；漏改其一会在 core 已修复时让 CLI 路径悄悄保持未打补丁（或反之）。本 PR 新增的 './sanitizeChildEnv' 子路径导出机制同样可以导出该解析器，建议合并：在 packages/core/package.json 的 exports 中仿照该条增加 "./envVarResolver"，在 packages/cli/tsconfig.json 与 packages/cli/vitest.config.ts 增加对应别名，删除本拷贝及其测试，并把两处 CLI 引入点（settings.ts、fast-path-settings.ts）指向 core。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| function stripWorkspaceHookSecurityOverrides(settings: Settings): Settings { | ||
| const { allowPrivateNetworkHooks, allowedHttpHookUrls } = | ||
| settings.security ?? {}; |
There was a problem hiding this comment.
[Suggestion] The set of workspace-stripped hook-security fields is now encoded independently in two functions that must be kept in sync by hand: stripWorkspaceHookSecurityOverrides strips the pair via destructuring, and getSettingsWarnings re-declares the same pair as two separate if blocks. This PR is the second field added to the pattern, so the recurrence is demonstrated, not hypothetical. — Failure scenario: for the next such field, adding it to the strip but forgetting getSettingsWarnings leaves it silently stripped with no warning; adding the warning but forgetting the strip tells the user it "is ignored" while it is still honored from workspace scope (the exact hole this PR class closes). No test couples the two field sets, so either divergence ships green.
Drive both from one constant, e.g.:
const WORKSPACE_STRIPPED_SECURITY_FIELDS = [
'allowPrivateNetworkHooks',
'allowedHttpHookUrls',
] as const;with stripWorkspaceHookSecurityOverrides deleting each named key from security and getSettingsWarnings iterating the same array to emit warnings.
中文说明
需要从 workspace 剥离的 hook 安全字段集合，现在被独立编码在两个必须手工保持同步的函数里：stripWorkspaceHookSecurityOverrides 通过解构剥离这两个字段，而 getSettingsWarnings 又把同样的两个字段重写为两个独立的 if 块。本 PR 是向该模式添加的第二个字段，说明这种重复是现实存在的，而非假设。失败场景：对下一个此类字段，若只加进剥离函数而漏了 getSettingsWarnings，它会被静默剥离、没有任何告警；若只加告警而漏了剥离，用户会被告知该设置"被忽略"，但它实际上仍从 workspace 作用域生效（正是本 PR 要关闭的那类漏洞）。没有任何测试把两个函数的字段集合耦合起来，因此任一方向的偏离都会绿灯通过。建议用同一个常量驱动两者：stripWorkspaceHookSecurityOverrides 从 security 中删除其中每个具名键，getSettingsWarnings 遍历同一数组来发出告警。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| if ( | ||
| workspaceFile.rawJson !== undefined && | ||
| workspaceFile.originalSettings.security?.allowedHttpHookUrls !== undefined | ||
| ) { | ||
| warningSet.add( | ||
| `Warning: security.allowedHttpHookUrls in workspace settings (${workspaceFile.path}) is ignored. This setting is only honored from User, System, or SystemDefaults scope settings.`, |
There was a problem hiding this comment.
[Suggestion] This change makes a workspace-scoped security.allowedHttpHookUrls silently stripped (with the warning above), but it is undocumented — while the sibling security.allowPrivateNetworkHooks documents the identical restriction. In docs/users/features/hooks.md the sibling's scope rule is documented ("only honored from User, System, and SystemDefaults settings scopes … ignored and logged as a warning"), but the page's only mention of allowedHttpHookUrls advises users to "pair this flag with a whitelist" with no scope caveat. The settings-schema description (settingsSchema.ts) likewise omits the "workspace values are ignored" note the sibling carries. — Failure scenario: a user who follows the docs/schema and puts the whitelist in Workspace settings gets a non-working whitelist plus a warning, and neither the docs page nor the schema explains why this key behaves like its sibling.
Extend the scope bullet in docs/users/features/hooks.md to cover security.allowedHttpHookUrls (and add the same scope note where the whitelist is mentioned), and append the same scope note to the allowedHttpHookUrls description in packages/cli/src/config/settingsSchema.ts, re-syncing packages/vscode-ide-companion/schemas/settings.schema.json.
中文说明
本改动让 workspace 作用域的 security.allowedHttpHookUrls 被静默剥离（并触发上面的告警），但没有相应文档——而姊妹设置 security.allowPrivateNetworkHooks 对相同限制有文档。在 docs/users/features/hooks.md 中，姊妹设置的作用域规则有说明（"仅从 User、System、SystemDefaults 设置作用域生效……被忽略并记录告警"），但该页对 allowedHttpHookUrls 的唯一提及却建议用户"将此标志与白名单配对使用"，没有任何作用域提示。设置 schema 描述（settingsSchema.ts）同样缺少姊妹设置所带的"workspace 值被忽略"说明。失败场景：用户按文档/schema 把白名单放进 workspace 设置，会得到一个不生效的白名单加一条告警，而文档页与 schema 都没有解释为什么这个键的行为与姊妹设置一致。建议把 docs/users/features/hooks.md 中的作用域说明扩展到 security.allowedHttpHookUrls（并在提到白名单处补上同样的作用域提示），同时在 packages/cli/src/config/settingsSchema.ts 的 allowedHttpHookUrls 描述中追加同样的作用域说明，并重新同步 packages/vscode-ide-companion/schemas/settings.schema.json。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix review-feedback summary — PR #8396 (round 2)All four inline findings from the round-2 review were verified against the code and resolved in code in commit Findings and dispositions
Note on the review-body observation (non-blocker)The review noted the PR Test Plan says "1385 tests green" while it observed 16964/19001. The counts differ because the plan quoted a focused subset; the full verification list below records what this round actually ran. Environment noteThis autofix shell runs inside a Qwen Code session that exports VerificationCommands actually run against the final tree (commit
中文说明Autofix 审查反馈处理总结 — PR #8396（第 2 轮）第 2 轮审查中的四条行内发现均已对照代码核实，并在提交 发现与处理结果
关于审查正文中的观察项（非阻断）审查指出 PR Test Plan 写的是 "1385 tests green"，而其观察到 16964/19001。数字不同是因为计划中引用的是聚焦子集；下方的完整验证清单记录了本轮实际运行的内容。 环境说明本 autofix shell 运行在一个导出了 验证针对最终代码树（提交
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/模型 |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： 🤖 Reviewed the latest feedback on PR #8396 (head Why, point by point
No code changes were made and no commit was created this round. 中文说明🤖 已审阅 PR #8396（HEAD 为 逐点说明
本轮未做任何代码改动，也未创建任何提交。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 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 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 3 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not reviewed: reverse audit — stopped before round 4 by the review time budget. Not reviewed: verification and reverse audit — each ran and opened its brief, but neither was launched with the prompt the CLI built — the launches were written by hand, so the posted findings cannot be counted as verified, and what the agents were actually asked is not what this skill certifies. Test Plan (not a blocker): 1385 tests green — this review observed 17658, 19229, 475 passed.
中文说明
已审查。 建议见行内评论。 3 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 未审查：反向审计——评审时间预算不足，未能开始第 4 轮。 未审查：验证与反向审计——两者都运行并打开了各自的 brief，但都不是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的，发布的发现不能算作已验证，agent 实际被要求做的也不是本 skill 所认证的内容。 Test Plan（非阻断）：1385 tests green — this review observed 17658, 19229, 475 passed。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const homeIsProjectRoot = | ||
| path.resolve(runtimeContext.getProjectRoot()) === | ||
| path.resolve(os.homedir()); |
There was a problem hiding this comment.
[Critical] R13-3: Worktree-isolation spawn bypasses the home-root trust gate. The gate reads runtimeContext.getProjectRoot(), but the worktree-isolation / working_dir spawn paths rebind that getter to the worktree path (agent.ts:2977, workflow-orchestrator.ts:1086, InProcessBackend.ts:521), while agent listing — which decides that repo agents surface at 'user' level at home — reads this.config.getProjectRoot(). In an untrusted folder whose project root IS the home directory, spawning a 'user'-surfaced repo agent with isolation: 'worktree' makes homeIsProjectRoot false → trustedAgentLevel true → addAgentHooks registers repo-supplied hooks session-wide despite isTrustedFolder() false. There is no trust gate on the isolation provisioning path. — Failure scenario: user runs with $HOME as project root (dotfiles repo / CI container), folder untrusted; repo .qwen/agents/evil.md (hooks frontmatter) surfaces at 'user' level; spawning it with isolation: 'worktree' registers those hooks for every event session-wide — the exact code execution this gate blocks on the normal path. Probe-verified mirroring the real rebind shape: worktree-rebind arm → addAgentHooks called once; no-isolation arm → 0; reading this.config.getProjectRoot() instead flips the rebind arm to 0 with all 14 gate tests still green. Residual sibling entrance of the R12-6 fix (direct spawn is gated correctly).
| const homeIsProjectRoot = | |
| path.resolve(runtimeContext.getProjectRoot()) === | |
| path.resolve(os.homedir()); | |
| const homeIsProjectRoot = | |
| path.resolve(this.config.getProjectRoot()) === | |
| path.resolve(os.homedir()); |
Apply the same rebind-proof root source to the skill-side gate (see the R13-9 thread on skill.ts).
中文说明
[Critical] R13-3：worktree 隔离派生绕过了 home-root 信任门禁。门禁读取 runtimeContext.getProjectRoot()，但 worktree 隔离 / working_dir 派生路径会把该 getter 重绑为 worktree 路径（agent.ts:2977、workflow-orchestrator.ts:1086、InProcessBackend.ts:521），而 agent 列表逻辑（决定 home 下仓库 agent 以 'user' 级出现的那一侧）读的是 this.config.getProjectRoot()。在项目根目录就是主目录且文件夹不受信时，用 isolation: 'worktree' 派生一个以 'user' 级出现的仓库 agent，会使 homeIsProjectRoot 为 false → trustedAgentLevel 为 true → 尽管 isTrustedFolder() 为 false，addAgentHooks 仍把仓库提供的 hooks 注册到整个会话。隔离准备路径上没有任何信任门禁。—— 失败场景：用户以 $HOME 为项目根（dotfiles 仓库 / CI 容器）、文件夹不受信；仓库 .qwen/agents/evil.md（frontmatter 带 hooks）以 'user' 级出现；以 isolation: 'worktree' 派生它，这些 hooks 就会在整个会话内为每个事件注册——正是该门禁在普通路径上所阻止的代码执行。已按真实重绑形态探针验证：worktree 重绑分支 → addAgentHooks 被调用一次；无隔离分支 → 0 次；改读 this.config.getProjectRoot() 后重绑分支翻转为 0，且 14 个门禁测试全部保持绿色。属 R12-6 修复的残留同类入口（直接派生已被正确门禁拦截）。
修复：从 per-agent 覆盖无法重绑的根来源计算 homeIsProjectRoot——例如 this.config.getProjectRoot()（manager 自身的 config，与列表侧及 skill.ts 一致），并对 skill 侧门禁（见 skill.ts 上的 R13-9 线程）使用同一来源。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const homeIsProjectRoot = | ||
| path.resolve(this.config.getProjectRoot()) === path.resolve(os.homedir()); |
There was a problem hiding this comment.
[Critical] R13-9: Skill-side sibling entrance of the R13-3 bypass. This gate reads this.config.getProjectRoot() — but in a subagent context this.config IS the per-agent override config whose getProjectRoot is rebound (worktree isolation agent.ts:2977; working_dir pin InProcessBackend.ts:521): rebuildToolRegistryOnOverride builds a fresh SkillTool on the override while the shared SkillManager keeps the skill's 'user' level. When a worktree-isolated / working_dir-pinned subagent invokes a repo skill surfaced at 'user' level (home-root topology, untrusted folder), homeIsProjectRoot = worktree path === homedir → false → sideEffectsGated false despite isTrustedFolder() false. — Failure scenario: untrusted folder whose project root IS home; repo skills surface at 'user' level; top-level invocation gates correctly, but the model spawns a subagent with isolation: 'worktree' and invokes the skill inside it → repo-supplied hooks register on the PARENT session's hooks manager (SessionHooksManager.addSessionHook has no dedupe; getHookSystem/getSessionId are inherited from the base config) and allowedTools become session-wide permission auto-approvals. Probe-verified through the real SkillTool.execute(): top-level arm → 0 registrations; worktree-rebind arm → registerSkillHooks once + addSessionAllowRule once with ['Bash(curl *)']; flips when the home-root detection is made rebind-proof. Note: the R13-3 fix (read this.config.getProjectRoot()) does NOT close this path — this gate already reads it, and it is the rebound value.
Suggested fix (spans locations): don't compute home-root shadowing from a per-agent-rebindable getter — propagate SkillManager's home-root detection instead (e.g. a flag on SkillConfig set at collection time, or gate any 'user'-level skill whose filePath resolves inside the repository tree), and use the same source of truth in SkillCommandLoader.ts:164 and subagent-manager.ts:939.
中文说明
[Critical] R13-9：R13-3 绕过的 skill 侧同类入口。此门禁读取 this.config.getProjectRoot()——但在 subagent 上下文中，this.config 正是那个 getProjectRoot 被重绑过的 per-agent 覆盖 config（worktree 隔离见 agent.ts:2977；working_dir 固定见 InProcessBackend.ts:521）：rebuildToolRegistryOnOverride 会在该覆盖 config 上构建新的 SkillTool，而共享的 SkillManager 保持该 skill 的 'user' 级不变。当 worktree 隔离 / working_dir 固定的 subagent 调用一个以 'user' 级出现的仓库 skill（home 为项目根、文件夹不受信）时，homeIsProjectRoot = worktree 路径 === 主目录 → false → 尽管 isTrustedFolder() 为 false，sideEffectsGated 仍为 false。—— 失败场景：项目根即主目录的不受信文件夹；仓库 skill 以 'user' 级出现；顶层调用被正确门禁拦截，但模型派生一个 isolation: 'worktree' 的 subagent 并在其中调用该 skill → 仓库提供的 hooks 注册到父会话的 hooks 管理器（SessionHooksManager.addSessionHook 无去重；getHookSystem/getSessionId 继承自基础 config），allowedTools 成为整个会话的权限自动批准。已通过真实 SkillTool.execute() 探针验证：顶层分支 → 0 次注册；worktree 重绑分支 → registerSkillHooks 一次 + addSessionAllowRule 一次（['Bash(curl *)']）；使 home-root 判定不受重绑影响后翻转。注意：R13-3 的修复（改读 this.config.getProjectRoot()）无法关闭此路径——本门禁读的本来就是它，而它正是被重绑的值。
修复建议（跨位置）：不要从可被 per-agent 重绑的 getter 计算 home-root 影子判定——改为传播 SkillManager 的 home-root 判定（例如在收集期为 SkillConfig 设置一个标志，或对任何 filePath 落在仓库树内的 'user' 级 skill 施加门禁），并在 SkillCommandLoader.ts:164 与 subagent-manager.ts:939 使用同一事实来源。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const nonAscii = /[\u0080-\uFFFF]/; | ||
| if (nonAscii.test(outerPattern) || nonAscii.test(innerPattern)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R13-4: hookUrlPatternCovers fail-closes on chars ≥ U+0080 but lets ASCII line terminators \n (0x0A) and \r (0x0D) through, which diverge from the runtime regex (no s flag, strict $, . cannot cross terminators). Probe-verified: hookUrlPatternCovers('https://corp.com/*', 'https://corp.com/*\n') → true, while new UrlValidator(['https://corp.com/*\n']).isAllowed('https://corp.com/x\n') is true but new UrlValidator(['https://corp.com/*']).isAllowed('https://corp.com/x\n') is false — so such a workspace entry survives narrowWorkspaceHookSecurityOverrides and displaces the higher-scope whitelist. Today's sole consumer neutralizes it (httpHookRunner → new URL()/fetch() apply WHATWG normalization, which strips \t\n\r — probe-verified, no host escape), so no widened network destination is reachable now — Concrete cost: latent widening for any future consumer that matches raw strings without normalization, plus a policy-integrity hit today (the user's list is displaced by a newline-quirked subset, so plain-URL hooks the user allowed stop validating). — Suggested fix:
const unsafe = /[\u0000-\u001f\u007f\u0080-\uFFFF]/;
if (unsafe.test(outerPattern) || unsafe.test(innerPattern)) {
return false;
}(mirrors the existing non-ASCII fail-closed gate; all 41 existing urlValidator tests stay green with it).
中文说明
[Suggestion] R13-4：hookUrlPatternCovers 对 ≥ U+0080 的字符 fail closed，但放过了 ASCII 行终止符 \n（0x0A）与 \r（0x0D），而它们与运行时正则（无 s 标志、严格 $、. 不能跨越行终止符）存在语义分歧。已用探针验证：hookUrlPatternCovers('https://corp.com/*', 'https://corp.com/*\n') → true，而 new UrlValidator(['https://corp.com/*\n']).isAllowed('https://corp.com/x\n') 为 true、new UrlValidator(['https://corp.com/*']).isAllowed('https://corp.com/x\n') 为 false——因此这类 workspace 条目能在 narrowWorkspaceHookSecurityOverrides 中存活并替换更高 scope 的白名单。当前唯一消费者会将其消解（httpHookRunner → new URL()/fetch() 应用 WHATWG 规范化，会剥离 \t\n\r——已探针验证，无主机逃逸），所以目前无可达的放宽目的地——具体代价：对未来任何不做规范化、直接匹配原始字符串的消费者构成潜在放宽；当下则有策略完整性损失（用户列表被一个带换行的子集替换，用户本已放行的普通 URL hooks 反而不再通过校验）。—— 修复建议：把 fail-closed 门禁扩展到全部 ASCII 控制字符（见上方代码块），与现有非 ASCII 门禁同构；现有 41 个 urlValidator 测试在该修复下全部保持绿色。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| if (specifier === '@qwen-code/qwen-code-core/envVarResolver') { | ||
| return { shortCircuit: true, url: '${envVarResolverSrcUrl}', format: 'module' }; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R13-2: The 17 lines added to this file (the envVarResolver subpath source-remap in the embedded ESM loader) are unreachable by every test command in the project — the file sits outside every npm workspace, and every vitest config under integration-tests/ includes only **/*.test.ts-style globs, so nothing collects it. — Concrete cost: if the specifier string, the remapped path, or the shortCircuit shape is wrong, nothing in CI or any suite run goes red; the defect surfaces only as a runtime resolution failure the next time a human manually runs the skill-review-harness after mode (which imports SkillReviewDialog → cli config/settings.js → the envVarResolver subpath), with no gate having ever validated it. — Suggested fix: exercise the remap somewhere a test command collects it (e.g. a scenario or a small *.test.ts under integration-tests/ asserting the loader resolves the subpath to the source file), or attach a manual terminal-capture run of the after mode as PR evidence.
中文说明
[Suggestion] R13-2：本文件新增的 17 行（内嵌 ESM loader 中的 envVarResolver 子路径源码重映射）无法被项目中任何测试命令触达——该文件不在任何 npm workspace 内，而 integration-tests/ 下的每个 vitest 配置只收录 **/*.test.ts 形式的文件，因此没有任何套件会加载它。—— 具体代价：如果 specifier 字符串、重映射路径或 shortCircuit 形态有误，CI 或任何套件运行都不会变红；该缺陷只会在下次有人手动运行 skill-review-harness 的 after 模式时（其导入链为 SkillReviewDialog → cli config/settings.js → envVarResolver 子路径）以运行时解析失败的形式出现，而此前没有任何门禁验证过它。—— 修复建议：在某个会被测试命令收录的位置行使该重映射（例如一个场景，或 integration-tests/ 下一个断言 loader 能把该子路径解析到源文件的小型 *.test.ts），或者把一次 after 模式的手动 terminal-capture 运行结果作为 PR 证据附上。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // secretEnv used to return the variable *name* as the secret — a | ||
| // public constant that makes HMAC verification bypassable. |
There was a problem hiding this comment.
[Suggestion] R13-5: This rationale comment misdescribes the pre-fix behavior. Verified against the merge base: resolveWebhookSecretEnv resolved the variable's VALUE (const envValue = env[envName]; … return envValue;) — it never returned the name. The name-as-secret behavior was this PR's own intermediate denylist branch (commit 9569dbf71), replaced by the throw in 832f82b47 — the same commit that introduced this comment. — Concrete cost: once merged, "used to" reads as the last-shipped behavior, which resolved the value: with QWEN_SERVER_TOKEN set, the live daemon token itself would have become the HMAC secret of a repo-configured webhook — a strictly worse exposure than the "public constant" described here. A future maintainer or incident reviewer using this test as the canonical denylist rationale would understate the historical exposure and could wrongly scope rotation/audit. (The adjacent code comment in config-utils.ts is fine — it describes the hypothetical keep-the-placeholder design, not merge-base history.)
| // secretEnv used to return the variable *name* as the secret — a | |
| // public constant that makes HMAC verification bypassable. | |
| // Before the denylist, secretEnv resolved the variable's *value* into | |
| // the webhook secret — with QWEN_SERVER_TOKEN set, the live daemon token | |
| // itself became the HMAC secret of a repo-configured webhook. |
中文说明
[Suggestion] R13-5：该理由注释误述了修复前的行为。已对合并基核实：resolveWebhookSecretEnv 解析的是变量的值（const envValue = env[envName]; … return envValue;）——从未返回过变量名。把名字当 secret 的行为是本 PR 自己的中间态 denylist 分支（commit 9569dbf71），在 832f82b47 中被改为抛错——正是引入本注释的同一 commit。—— 具体代价：合入后 "used to" 会被读作上一个已发布行为，而那个行为解析的是值：在设置了 QWEN_SERVER_TOKEN 时，活跃的 daemon token 本身会成为仓库所配置 webhook 的 HMAC secret——比这里描述的"公开常量"严重得多的暴露面。未来以本测试作为 denylist 权威理由的维护者或事件复盘者会低估历史暴露面，并可能错误地界定凭据轮换/审计范围。（config-utils.ts 中相邻的代码注释没有问题——它描述的是假设的"保留占位符"设计，而非合并基历史。）
— qwen3.8-max via Qwen Code /review (v0.21.6)
| it('grants allowedTools for a user-level skill regardless of folder trust', async () => { | ||
| vi.mocked(config.isTrustedFolder).mockReturnValue(false); |
There was a problem hiding this comment.
[Suggestion] R13-8: The new allowedTools trust gating describe mirrors the sibling hooks describe for every level/trust case except the home-directory-rooted 'user' skill case — the topology this PR explicitly calls out as breaking the 'user' trust premise. The hooks side pins it with two tests (~612, ~634); this side has zero home-root coverage. Mutation probe (ran): removing (skill.level === 'user' && homeIsProjectRoot) from the shared sideEffectsGated predicate keeps all 7 allowedTools tests green — only the hooks-side test fails. — Concrete cost: today both side effects hang off one shared boolean, so nothing is wrong yet; but the hooks and allowedTools branches are separate if blocks in applySkillSideEffects — if a later change detaches the allowedTools grant from the shared condition without the home-root clause (the exact divergence class this PR already exhibited once, before the gate moved above applySkillAllowedTools), a repo skill in a home-rooted project would silently gain session-wide permission auto-approvals while untrusted, and no test in this PR would go red. — Suggested fix: mirror the two hooks-side home-root tests here: getProjectRoot mocked to os.homedir(), level: 'user', asserting mockAddSessionAllowRule is NOT called when isTrustedFolder returns false and called twice when it returns true.
中文说明
[Suggestion] R13-8：新的 allowedTools trust gating describe 对每个级别/信任组合都与姊妹 hooks describe 对齐，唯独缺少 home 目录为项目根的 'user' 级 skill 场景——而本 PR 明确指出该拓扑会打破 'user' 级的信任前提。hooks 一侧已用两个测试钉住（约 612、634 行）；本侧对 home-root 零覆盖。已跑变异探针：从共享的 sideEffectsGated 谓词中移除 (skill.level === 'user' && homeIsProjectRoot)，全部 7 个 allowedTools 测试保持绿色——只有 hooks 一侧的测试失败。—— 具体代价：当前两种副作用共用同一个布尔量，所以现在还没有问题；但 hooks 与 allowedTools 是 applySkillSideEffects 中两个独立的 if 分支——如果后续改动把 allowedTools 授予从共享条件中拆出且不带 home-root 子句（本 PR 在此前迭代中已经出现过一次的同类分歧），home 为项目根时的仓库 skill 将在文件夹不受信时悄然获得整个会话的权限自动批准，而本 PR 中没有任何测试会变红。—— 修复建议：在此镜像 hooks 一侧的两个 home-root 测试：getProjectRoot mock 为 os.homedir()、level: 'user'，断言 isTrustedFolder 返回 false 时 mockAddSessionAllowRule 不被调用、返回 true 时被调用两次。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| it('does not grant allowedTools for a user-level skill when the project root is the home directory', async () => { | ||
| // SkillManager skips the 'project' level when the project root IS | ||
| // the home directory, so repository-committed skills surface at | ||
| // 'user' level there and must stay gated on folder trust. |
There was a problem hiding this comment.
[Suggestion] R13-10: No test pairs home-root with a trusted folder — the only homedir() use in this file (line ~612, this test) is the untrusted case, so the gate's "trusted folder always wins" disjunct is untested for the home-root combination. The over-gating mutant !(skill.level === 'user' && homeIsProjectRoot) && (isTrustedSkillLevel(skill.level) || this.config?.isTrustedFolder()) — a plausible "home-rooted user skills stay gated" simplification — passes all 35 tests in this file (ran it), yet silently withholds allowedTools auto-approvals from user-level skills when the project root IS home and the folder is explicitly trusted. The sibling gates pin this arm (skill.test.ts home-root-trusted, subagent-manager.test.ts home-root-trusted); the loader is the lone gate without it. — Concrete cost: that regression ships green and surfaces only as a user-visible loss of skill permission grants in that topology. — Suggested fix: add a companion test — getProjectRoot mocked to os.homedir(), isTrustedFolder mocked true, a user-level skill with two allowedTools → expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2).
中文说明
[Suggestion] R13-10：没有测试把 home-root 与受信文件夹组合在一起——本文件中唯一使用 homedir() 的地方（约 612 行，即本测试）是不受信场景，因此门禁中"受信文件夹始终放行"这一分支在 home-root 组合下没有测试覆盖。过度门禁变异 !(skill.level === 'user' && homeIsProjectRoot) && (isTrustedSkillLevel(skill.level) || this.config?.isTrustedFolder())——一个看似合理的"home 为根的 user skill 保持门禁"简化——通过本文件全部 35 个测试（已实际运行），却会在项目根即主目录且文件夹已明确受信时，悄然拒绝 user 级 skill 的 allowedTools 自动批准。姊妹门禁都钉住了该分支（skill.test.ts 的 home-root 受信用例、subagent-manager.test.ts 的 home-root 受信用例）；loader 是三者中唯一没有的。—— 具体代价：该回归会以全绿状态合入，只在该拓扑下以用户可见的 skill 权限授予丢失形式显现。—— 修复建议：补一个配套测试——getProjectRoot mock 为 os.homedir()、isTrustedFolder mock 为 true、带两个 allowedTools 的 user 级 skill → expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2)。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| expect( | ||
| hookUrlPatternCovers('https://corp.com/*', 'https://corp.com/a+b'), | ||
| ).toBe(false); |
There was a problem hiding this comment.
[Suggestion] R13-11: hookUrlPatternCovers fails closed on regex-active characters (? + $ () …) even for patterns with NO \. escape, although compilePattern's escape branch literalizes all of them for unescaped patterns — so coverage of query-string URL entries is provable but rejected, and narrowWorkspaceHookSecurityOverrides silently drops the workspace narrowing for the most common hook-endpoint shape. Probe-verified: hookUrlPatternCovers('https://corp.com/*', 'https://corp.com/ci?branch=main') → false, while new UrlValidator(['https://corp.com/ci?branch=main']).isAllowed('https://corp.com/ci?branch=main') → true and the lookalike 'https://corp.com/cbranch=main' → false, proving literalization; the outer list also admits the entry. Fails safe (never widens) — Concrete cost: user scope ['https://corp.com/*'] + trusted workspace narrowing to ['https://corp.com/ci?branch=main'] → entry filtered out → empty intersection → workspace list deleted → the broader user list stands and the repo author's explicit narrowing silently never applies; the always-on generic warning cannot distinguish successful narrowing from dropped narrowing. The docstring's fail-closed rationale ("the pre-escaped compilePattern branch would read it as raw regex") only holds for patterns containing \.; this pinned a+b assertion has no rationale of its own. — Suggested fix: apply the regexActive fail-closed only when either pattern contains \. (for \.-free patterns the escape branch guarantees literalness, so specials compare as literals); verified the conditioning flips the probe and breaks only this pinned assertion while all other fail-closed tests stay green. Alternatively, document in the docstring/test comment why blanket conservatism is preferred when coverage is provable.
中文说明
[Suggestion] R13-11：hookUrlPatternCovers 对含正则活性字符（? + $ () …）的模式一律 fail closed，即使模式不含任何 \. 转义——而 compilePattern 的转义分支对未转义模式会把这些字符全部字面量化，因此 query 字符串 URL 条目的覆盖关系是可证明的，却被拒绝，narrowWorkspaceHookSecurityOverrides 会对最常见的 hook 端点形态悄悄丢弃 workspace 收窄。已探针验证：hookUrlPatternCovers('https://corp.com/*', 'https://corp.com/ci?branch=main') → false，而 new UrlValidator(['https://corp.com/ci?branch=main']).isAllowed('https://corp.com/ci?branch=main') → true、形似串 'https://corp.com/cbranch=main' → false，证明确实被字面量化；外层列表同样放行该条目。方向安全（从不放宽）—— 具体代价：user scope ['https://corp.com/*'] + 受信 workspace 收窄为 ['https://corp.com/ci?branch=main'] → 条目被过滤 → 交集为空 → workspace 列表被删除 → 更宽的 user 列表保持不变，仓库作者明确的收窄悄悄失效；常开的通用告警无法区分"收窄成功"与"收窄被丢弃"。docstring 的 fail-closed 理由（"pre-escaped 的 compilePattern 分支会把它们当原生正则"）只对含 \. 的模式成立；此处钉住的 a+b 断言没有自己的理由说明。—— 修复建议：仅当任一模式含 \. 时才应用 regexActive fail-closed（不含 \. 时转义分支保证字面性，特殊字符可按字面比较）；已验证该条件化使探针翻转，且仅破坏此钉住的断言，其余 fail-closed 测试全部保持绿色。或者在 docstring/测试注释中说明：在覆盖关系可证明时为何仍偏好一刀切的保守。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // System scope is the final override: admin policy wins over both | ||
| // the user list and any workspace narrowing. | ||
| expect(settings.merged.security?.allowedHttpHookUrls).toEqual([ | ||
| 'https://managed.example.com/*', | ||
| ]); |
There was a problem hiding this comment.
[Suggestion] R13-13: This test never exercises the interaction it names: the workspace entry ['https://hooks.corp.com/ci/*'] is NOT covered by the system list ['https://managed.example.com/*'], so narrowWorkspaceHookSecurityOverrides deletes it before the merge and the assertion passes by merge order alone (system merged last, arrays replace). Mutant verified (ran): deleting the system.security?.allowedHttpHookUrls ?? arm from the coverage chain keeps every test green — the arm is structurally unobservable (whenever system defines the key, the last-wins merge overwrites whatever narrowing produced). No test in this diff pairs a system list with a workspace entry COVERED by it — the only shape where a narrowed workspace list survives to the merge and must then lose to system. — Concrete cost: the documented "System-scope whitelist always takes precedence over the workspace value" guarantee (schema description, warning text) has no load-bearing test; a regression letting a surviving narrowed workspace list outrank system for this key ships green in that untested scenario. — Suggested fix: change the workspace entry to one covered by the system list (e.g. workspace ['https://managed.example.com/ci/*'], user ['https://hooks.corp.com/*'], system ['https://managed.example.com/*']) and still expect ['https://managed.example.com/*'] — the narrowed list then survives to the merge and pins system precedence over it. Optionally drop the dead system ?? arm, or keep it and let this test justify it.
中文说明
[Suggestion] R13-13：本测试从未真正行使它标题所称的交互：workspace 条目 ['https://hooks.corp.com/ci/*'] 并不被 system 列表 ['https://managed.example.com/*'] 覆盖，因此 narrowWorkspaceHookSecurityOverrides 在合并前就把它删除，断言仅靠合并顺序（system 最后合并、数组整体替换）通过。已跑变异：删除覆盖链中的 system.security?.allowedHttpHookUrls ?? 分支，全部测试保持绿色——该分支在结构上不可观测（只要 system 定义了该键，last-wins 合并就会覆盖收窄产生的任何结果）。本 diff 中没有任何测试把 system 列表与一个被其覆盖的 workspace 条目同时设置——那是收窄后的 workspace 列表存活到合并、随后必须输给 system 的唯一形态。—— 具体代价：文档承诺的 "System-scope whitelist always takes precedence over the workspace value"（schema 描述、告警文案）没有任何承重测试；若某个回归让存活的收窄 workspace 列表在该键上压过 system，将在这一未测场景中以全绿状态合入。—— 修复建议：把 workspace 条目改为被 system 列表覆盖的条目（例如 workspace ['https://managed.example.com/ci/*']、user ['https://hooks.corp.com/*']、system ['https://managed.example.com/*']），仍期望 ['https://managed.example.com/*']——收窄列表便能存活到合并，从而钉住 system 对其的优先权。也可选删除已死的 system ?? 分支，或保留它并用本测试为其正名。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // A redirect delivers no payload, so it cannot consume the one | ||
| // execution: both firings must fetch instead of skipping. | ||
| expect(mockFetch).toHaveBeenCalledTimes(2); |
There was a problem hiding this comment.
[Suggestion] R13-14: The once-slot restoration is only pinned for the 302→302 case; the flow the restoration exists for — redirect resolves, the restored slot delivers the payload exactly once, then skips — has no test (no 302→200→third-call sequence anywhere in this file). Mutant verified (ran): gating the restore on the warn slot — if (hookConfig.once && !this.redirectWarnedHooks.has(warnKey)) this.executedOnceHooks.delete(onceKey) — passes all 31 current tests, but a 302→302→200→200 probe fails under it (the slot stays consumed after the second redirect; the payload is never delivered) and passes on the PR code. — Concrete cost: after any second redirect of the same URL the once slot stays consumed under that mutant, so once the URL starts returning 2xx (redirect fixed in place) the hook never fires for the rest of the session — the exact "silently no-op-ing forever" outcome the restore was added to prevent. The inverse regression (also deleting the slot after a post-restore success) re-delivers the once payload on every subsequent event, equally unseen by current tests. — Suggested fix: add a test — first call 302, second call 200 with a JSON payload (assert the payload is delivered, mockFetch called twice), third call must skip (no further fetch; the exhausted-slot output).
中文说明
[Suggestion] R13-14：once 槽位恢复目前只被 302→302 场景钉住；而恢复机制存在的意义——重定向解除后，被恢复的槽位恰好投递一次负载、随后跳过——没有任何测试覆盖（整个文件中不存在 302→200→第三次调用的序列）。已跑变异：把恢复用 warn 槽位门禁化——if (hookConfig.once && !this.redirectWarnedHooks.has(warnKey)) this.executedOnceHooks.delete(onceKey)——通过当前全部 31 个测试，但 302→302→200→200 探针在其下失败（第二次重定向后槽位仍被占用，负载永远不投递），在 PR 代码上则通过。—— 具体代价：在该变异下，同一 URL 经历任意第二次重定向后 once 槽位保持被占用，一旦该 URL 开始返回 2xx（重定向被原地修复），该 hook 在会话剩余时间内永不再触发——正是恢复机制要防止的"悄然永久 no-op"结局。反向回归（在恢复后的成功投递后也删除槽位）会让 once 负载在后续每个事件重复投递，当前测试同样看不见。—— 修复建议：补一个测试——第一次调用 302，第二次调用 200 且带 JSON 负载（断言负载被投递、mockFetch 被调用两次），第三次调用必须跳过（不再 fetch，输出为槽位耗尽路径的结果）。
— qwen3.8-max via Qwen Code /review (v0.21.6)
|
🤖 Addressed the latest review feedback (round 17/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 17/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #8396 (Critical-only tail)Two Critical findings from the automated reviewer, both describing the same Feedback and dispositions[Critical] R13-3 — worktree-isolation spawn bypasses the home-root trust gate ( The spawn-time gate computed [Critical] R13-9 — skill-side sibling entrance of the same bypass ( The skill side-effect gate already read The fix (single source of truth)The home-root shadow is now decided once, at collection time, by the shared
All three gates now consume the flag instead of re-deriving the shadow from a
The flag travels with the skill/agent object, so it is immune to every Tests
Conflict notesNone ( VerificationCommands actually run this round (all on the final tree):
Note: in this particular runner 中文说明Autofix 评审轮次 — PR #8396（仅处理 Critical 阶段）自动评审器提出两个 Critical 发现，描述的是同一类对本 PR 引入的 home-root 反馈与处置[Critical] R13-3 — worktree 隔离派生绕过 home-root 信任门禁（ 派生时门禁原来从 [Critical] R13-9 — 同一绕过的 skill 侧同类入口（ skill 侧副作用门禁用的是 修复（单一事实来源）home-root 影子判定现在只在收集期做一次，由共享 manager 完成——它们是唯一
三个门禁都改为消费该标志，而不是从可被 per-agent 重绑的 getter 重新推导：
标志随 skill/agent 对象传播，因此对所有现有重绑点（ 测试
冲突说明无（ 验证本轮在最终代码树上实际执行的命令：
注：本 runner 环境导出了 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 无冲突。 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/模型 |
|
🤖 AutoFix ran out of time before finishing (timeout (1080000ms)) (attempt 18/100) — it will retry on the next scan. 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/31040664859 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： 🤖 No code changes this round — the one failed check was diagnosed as an environment failure that happened before this PR's code was built or run. Autofix address-review — PR #8396 (no action)Critical-only mode is active; the deferred non-Critical section was left untouched (audit record only, per the round rules). No inline comments, reviews, or issue-level comments were actionable this round. No conflict resolution was requested ( Failed check:
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the case-insensitive env-denylist paths this PR adds are only exercisable on Windows.
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the case-insensitive env-denylist paths this PR adds are only exercisable on Windows。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // Never follow redirects: the whitelist and DNS-level SSRF | ||
| // checks above cover only this URL, and a 307/308 would re-send | ||
| // the hook payload to an unvalidated target. A 3xx response |
There was a problem hiding this comment.
[Critical] R14-1: The whitelist static check regex-tests the RAW hook URL string, but fetch() sends the WHATWG-normalized URL — URL normalization (dot-segment removal) moves the hook payload to a path on a user-whitelisted host that the whitelist does not cover. Same static-check/runtime-consumer divergence class this PR closes for redirects, left open for normalization. — Failure scenario: user-scope whitelist ["https://corp.example.com/api/*"]; a trusted repo configures hook url https://corp.example.com/api/../telemetry/exfil → validate() tests the raw string → allowed; fetch normalizes and POSTs the hook payload (prompts, tool inputs/outputs) to https://corp.example.com/telemetry/exfil, which the whitelist regex does NOT match. Probe-verified end-to-end with a real fetch against a live server; the fix below flips the probe and all 72 hook tests stay green. The userinfo cross-host variant fails closed at fetch (undici rejects credentials in Request construction), so confirmed harm is bounded to normalization-without-credentials destinations — but every path-shaped pattern (…/prefix/*, all documented examples) is bypassable.
// In HttpHookRunner.execute (or UrlValidator.validate): parse once and
// validate the exact string that will be fetched:
const parsed = new URL(url); // invalid URL already fails closed in isBlocked
// run isBlocked + isAllowed against parsed.href, then fetch(parsed.href, ...)中文说明
白名单静态检查对原始 hook URL 字符串做正则匹配，但 fetch() 发送的是 WHATWG 规范化后的 URL——URL 规范化（点段移除）会把 hook 负载投递到用户白名单主机上一个白名单并未覆盖的路径。这与本 PR 为重定向关闭的"静态检查/运行时消费者分歧"是同一类问题，但在规范化方向上仍然敞开。——失败场景：user 白名单 ["https://corp.example.com/api/*"]，受信仓库配置 hook url https://corp.example.com/api/../telemetry/exfil → validate() 对原始字符串判定放行；fetch 规范化后把 hook 负载（prompt、工具输入/输出）POST 到白名单正则并不匹配的 https://corp.example.com/telemetry/exfil。已用真实 fetch 对活服务器端到端探针验证；下方修复可翻转探针且全部 72 个 hook 测试保持绿色。userinfo 跨主机变体在 fetch 处 fail closed（undici 拒绝构造含凭据的 Request），因此已确认的危害限于不含凭据的规范化目的地——但所有路径形态的模式（…/prefix/*，即全部文档示例）都可被绕过。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // Extensions load only from the user-scope extensions directory, so | ||
| // they are in the trusted allowlist and must not regress behind the | ||
| // fail-closed gate. |
There was a problem hiding this comment.
[Critical] R14-2: The new gate's 'extension'-level exemption violates its own fail-closed invariant in the home-root topology. When the project root IS $HOME, ~/.qwen/extensions == <repo>/.qwen/extensions is repo-controlled; unknown extensions are active by default (ExtensionStore.getActivation returns effective 'enabled' with no store policy — no install/consent/state needed), and their skills surface at level 'extension', where isTrustedSkillLevel skips the gate unconditionally — repo-supplied frontmatter allowedTools become session-wide auto-approvals and frontmatter hooks register, despite the untrusted folder. This is the exact topology this PR hardens at 'user' level via homeRootShadow — the tag is applied only when level === 'user', so the identical topology is hardened there and left open here (extensions additionally carry MCP servers, commands, and hooks). The subagent gate's 'extension' arm is the same surface (extensions supply agents). Scope note: pre-PR, allowedTools were granted unconditionally for all skills, so this vector is not newly reachable — the Critical stands on the gate's own stated premise ("only levels that cannot originate from the repository skip the folder-trust gate") being falsified by a topology the gate explicitly models elsewhere. — Failure scenario: victim runs qwen with project root == $HOME, folder untrusted; repo commits .qwen/extensions/evil/ with skills/s/SKILL.md declaring allowedTools: ['Bash(*)'] → chain verified at HEAD: auto-active → level 'extension' → gate skipped → session-wide auto-approval of attacker-chosen tool patterns.
Suggested fix: extend the home-root tag to extension-level skills (inside the extension branch of listSkillsAtLevel, which returns before the current tagging block — if ((level === 'user' || level === 'extension') && isHomeDirectory)), broaden the gate disjunct to level-independent skill.homeRootShadow === true in skill.ts, SkillCommandLoader.ts, and the subagent gate; condition this test on not-home-root-shadowed and add a gated counterpart.
中文说明
新门禁对 'extension' 级的豁免在 home-root 拓扑下违反了其自身的 fail-closed 不变量。当项目根就是 $HOME 时，~/.qwen/extensions == <repo>/.qwen/extensions 是仓库可控的；未知扩展默认处于激活状态（无 store 策略时 ExtensionStore.getActivation 返回 effective 'enabled'——无需安装/同意/状态文件），其 skill 以 'extension' 级出现，而 isTrustedSkillLevel 对该级无条件跳过门禁——仓库提供的 frontmatter allowedTools 成为整个会话的自动批准、frontmatter hooks 被注册，全程没有文件夹信任。这正是本 PR 在 'user' 级用 homeRootShadow 加固的拓扑——标记仅在 level === 'user' 时应用，因此同一拓扑在 'user' 级被加固、在这里却敞开（扩展还额外携带 MCP server、命令与 hooks）。subagent 门禁的 'extension' 分支是同一表面（扩展可提供 agent）。范围说明：PR 之前 allowedTools 对所有 skill 无条件授予，因此该向量并非新近可达——Critical 成立的依据是门禁自身声明的前提（"只有不可能来源于仓库的级别才跳过文件夹信任门禁"）被门禁在别处显式建模的拓扑所证伪。——失败场景：受害者以 $HOME 为项目根运行 qwen、文件夹不受信；仓库提交带 skills/s/SKILL.md（声明 allowedTools: ['Bash(*)']）的 .qwen/extensions/evil/ → 已在 HEAD 逐步核实：自动激活 → 'extension' 级 → 门禁跳过 → 攻击者选定的工具模式获得会话级自动批准。
修复建议：把 home-root 标记扩展到 extension 级 skill（在 listSkillsAtLevel 的 extension 分支内——该分支在当前标记块之前就返回——if ((level === 'user' || level === 'extension') && isHomeDirectory)），并把门禁析取项放宽为与级别无关的 skill.homeRootShadow === true（skill.ts、SkillCommandLoader.ts 与 subagent 门禁同步）；将本测试限定为非 home-root-shadow 场景并补充受门禁的对照测试。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const chunks = outer.split('*'); | ||
| if (chunks.length === 1) { | ||
| return inner === outer; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R14-3: The no-wildcard (exact-URL) branch of hookUrlPatternCovers has zero test coverage — all 20 invocations in urlValidator.test.ts use an outer pattern containing *. — Failure scenario: exact URLs are valid allowedHttpHookUrls entries; if a future edit relaxes this branch to prefix/containment comparison (e.g. inner.startsWith(outer)), hookUrlPatternCovers('https://corp.com/ci', 'https://corp.com/ci-evil') flips to true, the workspace entry survives narrowing, replaces the user's list in the merge, and hook payloads POST to a destination the user's whitelist does not cover — suite-green.
Suggested fix: add cases such as expect(hookUrlPatternCovers('https://corp.com/ci', 'https://corp.com/ci')).toBe(true), expect(hookUrlPatternCovers('https://corp.com/ci', 'https://corp.com/ci-evil')).toBe(false), plus a wildcard inner vs exact outer (must fail closed).
中文说明
hookUrlPatternCovers 的无通配符（精确 URL）分支零测试覆盖——urlValidator.test.ts 中全部 20 处调用的 outer 模式都含 *。——失败场景：精确 URL 是合法的 allowedHttpHookUrls 条目；若未来把该分支放宽为前缀/包含比较（如 inner.startsWith(outer)），hookUrlPatternCovers('https://corp.com/ci', 'https://corp.com/ci-evil') 会翻转为 true，workspace 条目在收窄中存活并替换用户列表，hook 负载被 POST 到用户白名单未覆盖的目的地——且测试全绿。修复建议：补充上述用例（精确相等为 true、前缀污染为 false、通配 inner 对精确 outer 必须 fail closed）。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| if (hookConfig.once) { | ||
| this.executedOnceHooks.delete(onceKey); | ||
| } |
There was a problem hiding this comment.
[Suggestion] R14-4: No test pins that a once hook failing with a non-3xx status (4xx/5xx) still consumes its single execution slot — the 3xx exemption is tested, but the complementary behavior is not (once: true appears only in 200/302 tests; the 500 test uses a non-once hook). — Failure scenario: if a future refactor hoists this.executedOnceHooks.delete(onceKey) out of the 3xx branch to the top of the general failure path (plausible, since 4xx/5xx also deliver no payload), a once hook behind a 4xx/5xx endpoint would re-fire on every subsequent event instead of running once, re-sending its payload indefinitely — suite-green.
Suggested fix: add a test: once: true hook, mock a 500 response, execute twice for the same event, assert mockFetch was called exactly once (slot consumed on non-3xx failure).
中文说明
没有测试钉住：once hook 以非 3xx 状态（4xx/5xx）失败时仍消耗其唯一一次执行额度——3xx 豁免有测试，但其互补行为没有（once: true 只出现在 200/302 测试中；500 测试用的是非 once hook）。——失败场景：若未来重构把 this.executedOnceHooks.delete(onceKey) 从 3xx 分支提升到通用失败路径顶部（4xx/5xx 同样未投递负载，这种改动很自然），位于 4xx/5xx 端点后的 once hook 会在后续每个事件重复触发、无限重发负载——且测试全绿。修复建议：新增测试——once: true hook、mock 500 响应、同一事件执行两次、断言 mockFetch 恰被调用一次。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| if (specifier === '@qwen-code/qwen-code-core') { | ||
| return { shortCircuit: true, url: '${coreSrcUrl}', format: 'module' }; | ||
| } | ||
| if (specifier === '@qwen-code/qwen-code-core/envVarResolver') { |
There was a problem hiding this comment.
[Suggestion] R13-2 (still standing from round 13; re-detected independently this round by the build-test efficacy probe, harness validated): the envVarResolver subpath source-remap added to this file is unreachable by every test command in the project — the file sits outside every npm workspace. — Concrete cost: if the remap specifier or mapped path is wrong (or drifts from core package.json's ./envVarResolver subpath or from scripts/dev.js), an after-mode terminal capture of the SkillReviewDialog flow fails at import time on a fresh checkout, and nothing automated catches the break.
Suggested fix: confirm the terminal-capture harness run for this PR actually executed this path in CI, or exercise the loader remap in a collected test.
中文说明
R13-2（第 13 轮遗留，本轮仍然成立；本轮 build-test 有效性探针独立再次发现，harness 已验证）：本文件新增的 envVarResolver 子路径源码重映射对项目内任何测试命令都不可达——该文件位于所有 npm workspace 之外。——具体代价：若重映射的说明符或目标路径错误（或与 core package.json 的 ./envVarResolver 子路径、scripts/dev.js 发生漂移），新检出上的 after 模式 SkillReviewDialog 终端采集会在 import 时失败，且没有任何自动化手段能捕获该破坏。修复建议：确认本 PR 的 terminal-capture harness 运行在 CI 中确实执行了该路径，或在一个被收集的测试中执行该 loader 重映射。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // The remedy rides a systemMessage. How it surfaces is | ||
| // event-dependent: Stop/SubagentStop show it to the user; other |
There was a problem hiding this comment.
[Suggestion] R14-9: This pinned surfacing contract — "Stop/SubagentStop show it to the user" (repeated near line 462) — is false for SubagentStop: no consumer ever displays a SubagentStop hook's systemMessage. Verified against every SubagentStop consumer at HEAD: runSubagentStopHookLoop (agent.ts) and background-agent-resume.ts read only isBlockingDecision()/shouldStopExecution() and the cap warning; client.ts emits HookSystemMessage only in the Stop path; hookEventHandler.processCommonHookOutputFields only debug-logs it. — Failure scenario: a SubagentStop HTTP hook behind a redirecting LB — the first 3xx burns the one-shot redirectWarnedHooks slot for <url>:SubagentStop and produces a systemMessage no UI ever renders; the user gets no warning and no remedy, ever, while these comments tell maintainers SubagentStop is a user-visible surface (the stated reason the per-event slot keeps SubagentStop separate from PreToolUse). Both the remedy and its surfacing gap are new with this diff (pre-PR redirects were followed, so no warning existed).
Suggested fix: surface the SubagentStop systemMessage where the loop consumes the hook output (mirroring client.ts's Stop path), or correct both comments to say only Stop is user-visible and SubagentStop is debug-only like PreToolUse.
中文说明
R14-9：此处钉住的呈现契约——"Stop/SubagentStop show it to the user"（约第 462 行处重复出现）——对 SubagentStop 不成立：没有任何消费者会展示 SubagentStop hook 的 systemMessage。已在 HEAD 核实全部 SubagentStop 消费者：runSubagentStopHookLoop（agent.ts）与 background-agent-resume.ts 只读取 isBlockingDecision()/shouldStopExecution() 与上限告警；client.ts 仅在 Stop 路径发出 HookSystemMessage；hookEventHandler 只写 debug 日志。——失败场景：位于重定向负载均衡后的 SubagentStop HTTP hook——首个 3xx 会烧掉 <url>:SubagentStop 的一次性 redirectWarnedHooks 槽位并产生一条没有任何 UI 会呈现的 systemMessage；用户永远得不到告警与补救，而这些注释却告诉维护者 SubagentStop 是用户可见表面（也正是 per-event 槽位把 SubagentStop 与 PreToolUse 分开的理由）。补救本身及其呈现缺口都是本 diff 新增（PR 前重定向会被跟随，因此根本不存在该告警）。修复建议：在 hook 输出的消费处（loop 内）呈现 SubagentStop 的 systemMessage（镜像 client.ts 的 Stop 路径），或修正两处注释——只有 Stop 用户可见，SubagentStop 与 PreToolUse 一样仅 debug 可见。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // runtime language past the literal reading used here. A bare `.` is | ||
| // regex-active in that branch too, but only after unescaping: the `\.` | ||
| // sequences it came from are literal dots, so strip them before checking. | ||
| const regexActive = /[+?^${}()|[\]\\]/; |
There was a problem hiding this comment.
[Suggestion] R13-4 (still standing from round 13): hookUrlPatternCovers fail-closes on chars ≥ U+0080 but lets ASCII line terminators \n (0x0A) and \r (0x0D) through, which diverge from the runtime regex semantics (no s flag; $ matches before a trailing newline; . cannot cross terminators) — the same checker/runtime divergence family the PR's other guards close, left open for the ASCII control characters. Settings patterns embedding terminators are rare but possible in hand-edited JSON, and the comparison treats them as ordinary literal text. Fail-closed overall, hence Suggestion.
| const regexActive = /[+?^${}()|[\]\\]/; | |
| const regexActive = /[+?^${}()|[\]\\\n\r]/; |
中文说明
R13-4（第 13 轮遗留，本轮仍然成立）：hookUrlPatternCovers 对 ≥ U+0080 的字符 fail closed，却放行 ASCII 行终止符 \n（0x0A）与 \r（0x0D），而它们与运行时正则语义存在分歧（无 s 标志；$ 可在结尾换行前匹配；. 不能跨越终止符）——与 PR 其他守卫所关闭的"检查器/运行时分歧"同族，唯独对 ASCII 控制字符敞开。settings 模式中嵌入终止符虽罕见但手工编辑 JSON 时可能出现，而比较逻辑把它们当普通字面文本处理。总体 fail-closed，故为 Suggestion。上方 suggestion 将 \n/\r 并入 fail-closed 字符类。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| }), | ||
| ).rejects.toThrow(`${SECRET} is a Qwen-internal secret`); | ||
|
|
||
| // secretEnv used to return the variable *name* as the secret — a |
There was a problem hiding this comment.
[Suggestion] R13-5 (still standing from round 13): this rationale comment misdescribes the pre-fix behavior. Verified against the merge base: resolveWebhookSecretEnv resolved the variable's VALUE (const envValue = env[envName]; … return envValue;) and threw when unset — it never returned the name. — Concrete cost: a maintainer reading this security test gets an inverted history of what the old code did; the comment is the only rationale documentation for the secretEnv denylist.
Suggested fix: reword to describe the actual pre-fix behavior (resolved the variable's value into channel config).
中文说明
R13-5（第 13 轮遗留，本轮仍然成立）：该原理性注释对修复前行为的描述有误。已对合并基核实：resolveWebhookSecretEnv 解析的是变量的值（const envValue = env[envName]; … return envValue;），未设置时抛错——它从不返回变量名。——具体代价：维护者阅读该安全测试时会得到关于旧代码行为的颠倒历史；而该注释是 secretEnv 拒绝列表唯一的原理说明。修复建议：改写为真实的修复前行为（把变量的值解析进 channel 配置）。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| }); | ||
|
|
||
| it('should strip security.allowPrivateNetworkHooks from workspace scope even when trusted', () => { | ||
| it('should strip security.allowPrivateNetworkHooks and security.allowedHttpHookUrls from workspace scope even when trusted', () => { |
There was a problem hiding this comment.
[Suggestion] R13-6 (still standing from round 13): this test title makes a blanket strip claim for security.allowedHttpHookUrls while the same describe block adds narrowing tests — the final behavior is narrow-with-fallback, not strip. — Concrete cost: a future maintainer reading the title concludes workspace allowedHttpHookUrls is always stripped and reasons incorrectly about the merge (e.g. debugging why a trusted workspace's covered entry survived); the title contradicts its own sibling tests.
| it('should strip security.allowPrivateNetworkHooks and security.allowedHttpHookUrls from workspace scope even when trusted', () => { | |
| it('should strip security.allowPrivateNetworkHooks and narrow security.allowedHttpHookUrls against higher scopes in workspace scope even when trusted', () => { |
中文说明
R13-6（第 13 轮遗留，本轮仍然成立）：该测试标题对 security.allowedHttpHookUrls 做了"一律剥离"的断言，而同一 describe 块内新增了收窄（narrowing）测试——最终行为是"收窄并在无存活时回退"，不是剥离。——具体代价：未来维护者读标题会以为 workspace 的 allowedHttpHookUrls 总是被剥离，从而对合并行为做出错误推理（例如排查为何受信 workspace 的被覆盖条目存活了下来）；标题与其姊妹测试自相矛盾。上方 suggestion 给出与真实语义一致的标题。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // A redirect delivers no payload, so it cannot consume the one | ||
| // execution: both firings must fetch instead of skipping. |
There was a problem hiding this comment.
[Suggestion] R13-14 (still standing from round 13): the once-slot restoration is only pinned for the 302→302 case; the flow the restoration exists for — redirect resolves, the restored slot delivers the payload exactly once, then skips — has no test (no 302→200→third-call). — Failure scenario: if the restoration delete is moved or conditioned differently (e.g. removed under the theory that non-2xx should consume), the delivery-then-skip half of the contract is untested, so a regression there ships green while once-hooks either double-deliver or never deliver after a transient redirect.
Suggested fix: add a test: once hook, first execute → 302 (no consumption), mock 200, second execute → payload delivered, third execute → skipped (mockFetch called twice total).
中文说明
R13-14（第 13 轮遗留，本轮仍然成立）：once 槽位恢复只被钉住了 302→302 的情形；恢复机制真正服务的流程——重定向解除后，被恢复的槽位恰好投递一次负载、随后跳过——没有测试（没有 302→200→第三次调用）。——失败场景：若恢复用的 delete 被移动或附加条件（例如以"非 2xx 也应消耗"为由删除），契约中"投递一次后跳过"的一半没有测试保护，相关回归会在测试全绿的情况下发布，once-hook 在瞬时重定向后要么重复投递、要么永不投递。修复建议：新增测试——once hook，第一次执行 → 302（不消耗），mock 200，第二次执行 → 负载投递，第三次执行 → 跳过（mockFetch 共被调用两次）。
— qwen3.8-max via Qwen Code /review (v0.21.6)
|
🤖 AutoFix stopped: this counting window now contains 3 time-budget exhaustions (pushed rounds in between included; this round itself may have failed differently). That is 3 full agent runs that pushed nothing. A human should split or reduce the PR (or raise the agent time budget AND its step backstop together), then comment 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/31060843430 🧠 Handled by Qwen Code · model/模型 |
|
⏸️ Takeover paused: this PR reached its round cap (100/100). Comment 中文说明⏸️ 托管已暂停：本 PR 达到轮次上限（100/100）。评论 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the case-insensitive env-denylist paths this PR adds are only exercisable on Windows.
Test Plan (not a blocker): 1385 tests green — this review observed 19232, 475, 1067, 1455, 2825, 444 passed.
[Critical] R14-1 (prior-round thread, comment 3725050089 at packages/core/src/hooks/httpHookRunner.ts:253) re-checked at dce013c and STILL STANDS: the whitelist check regex-tests the raw interpolated URL string while fetch() sends the WHATWG-normalized form. Probe at this commit: new UrlValidator(['https://corp.example.com/api/*']).validate('https://corp.example.com/api/../telemetry/exfil') returns allowed:true, while the normalized URL (https://corp.example.com/telemetry/exfil) is NOT covered by the whitelist — dot-segment removal delivers the hook payload to a path the whitelist does not cover. The suggested fix (parse once and validate/fetch parsed.href) is not applied.
[Critical] R14-2 (prior-round thread, comment 3725050095 at packages/core/src/tools/skill.test.ts:686) re-checked at dce013c and STILL STANDS: at the home-root topology (~/.qwen == /.qwen), repo-committed extensions load as active by default (ExtensionStore.getActivation returns effective 'enabled' with no store policy) and their skills/agents surface at 'extension' level, which both trust gates exempt; homeRootShadow only tags the 'user' level, so the fail-closed invariant the gates state ('only levels that cannot originate from the repository skip the gate') is violated for this topology.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the case-insensitive env-denylist paths this PR adds are only exercisable on Windows。
Test Plan（非阻断）：1385 tests green — this review observed 19232, 475, 1067, 1455, 2825, 444 passed。
[Critical] R14-1 (prior-round thread, comment 3725050089 at packages/core/src/hooks/httpHookRunner.ts:253) re-checked at dce013c and STILL STANDS: the whitelist check regex-tests the raw interpolated URL string while fetch() sends the WHATWG-normalized form. Probe at this commit: new UrlValidator(['https://corp.example.com/api/*']).validate('https://corp.example.com/api/../telemetry/exfil') returns allowed:true, while the normalized URL (https://corp.example.com/telemetry/exfil) is NOT covered by the whitelist — dot-segment removal delivers the hook payload to a path the whitelist does not cover. The suggested fix (parse once and validate/fetch parsed.href) is not applied.
[Critical] R14-2 (prior-round thread, comment 3725050095 at packages/core/src/tools/skill.test.ts:686) re-checked at dce013c and STILL STANDS: at the home-root topology (~/.qwen == /.qwen), repo-committed extensions load as active by default (ExtensionStore.getActivation returns effective 'enabled' with no store policy) and their skills/agents surface at 'extension' level, which both trust gates exempt; homeRootShadow only tags the 'user' level, so the fail-closed invariant the gates state ('only levels that cannot originate from the repository skip the gate') is violated for this topology.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| * check fails closed. | ||
| */ | ||
| export function isTrustedSkillLevel(level: SkillLevel | undefined): boolean { | ||
| return level === 'user' || level === 'bundled' || level === 'extension'; |
There was a problem hiding this comment.
[Critical] R15-6: The gates exempt 'user'-level skills on the premise that they live in ~/.qwen, but two supported configurations put repository-controlled directories into the 'user' level: (A) settings.skills.directories — including RELATIVE entries, which the code itself resolves against the working directory (skill-manager.ts:930-943); (B) QWEN_HOME resolving inside the project root (relative QWEN_HOME is documented to resolve from cwd), which makes <repo>/<QWEN_HOME>/skills the user-level skills dir. homeRootShadow is only tagged when projectRoot === homedir, so repo-shipped skills surface at 'user' level untagged and skip both gates (skill.ts applySkillSideEffects and SkillCommandLoader.ts:164-168) — repo-authored skill hooks are registered and repo-authored allowedTools granted as session-wide auto-approvals in an untrusted folder.
Failure scenario: probe-verified end-to-end at this commit — with customSkillDirs: ['./skills'], an untrusted folder, and a repo-shipped skills/evil/SKILL.md declaring allowedTools: ['Bash(git push *)'] plus a PreToolUse command hook: listing returned level 'user' with no homeRootShadow; both gates opened (hooks registered, allow rule granted), while the identical content at 'project' level was gated (0 calls). Extending the tag to dirs inside getProjectRoot() flipped both arms. Vector B verified the same way with QWEN_HOME=<repo>/.qhome. The repo cannot inject skills.directories itself (workspace settings are dropped while untrusted) — the indirection needs user/system-scoped config or a victim-side QWEN_HOME; that narrows the exposed population, not the mechanism.
Suggested fix: tag by provenance — in listSkillsAtLevel('user'), set the shadow flag for any skill whose resolved base dir lies inside getProjectRoot() (covers both custom-dir entries and a repo-interior QWEN_HOME); mirror in listSubagentsAtLevel (see the subagent-manager thread). Cross-links: R15-9 (subagent side, same root cause), R15-5 (the over-blocking direction of the same predicate — do not fix by blanket-removing the 'user' exemption).
中文说明
[Critical] 门禁对 'user' 级 skill 的豁免基于“它们位于 ~/.qwen”这一前提，但有两种受支持的配置会把仓库可控目录放进 'user' 级：(A) settings.skills.directories——包括相对路径条目，代码本身会按工作目录解析（skill-manager.ts:930-943）；(B) QWEN_HOME 解析到项目根目录内部（文档明确相对 QWEN_HOME 从 cwd 解析），使 <repo>/<QWEN_HOME>/skills 成为 user 级 skill 目录。homeRootShadow 仅在 projectRoot === homedir 时打标，因此仓库自带的 skill 会以无标记的 'user' 级身份出现，绕过两处门禁（skill.ts 的 applySkillSideEffects 与 SkillCommandLoader.ts:164-168）——在不可信文件夹中注册仓库编写的 skill hooks，并把仓库编写的 allowedTools 授予为全会话自动批准。
失败场景：已在本 commit 端到端探针验证——customSkillDirs: ['./skills']、不可信文件夹、仓库自带 skills/evil/SKILL.md（声明 allowedTools: ['Bash(git push *)'] 与 PreToolUse 命令 hook）：列表返回 level 'user' 且无 homeRootShadow；两处门禁均打开（hooks 已注册、allow 规则已授予），而同样内容在 'project' 级被拦截（0 次调用）。把打标扩展到 getProjectRoot() 内部目录后两个分支均翻转。向量 B 用 QWEN_HOME=<repo>/.qhome 同样验证。仓库自身无法注入 skills.directories（不可信时 workspace 设置被整体丢弃）——该间接路径需要 user/system 作用域配置或受害端的 QWEN_HOME；这只缩小暴露人群，不改变机制本身。
修复建议：按来源打标——在 listSkillsAtLevel('user') 中，对解析后基础目录位于 getProjectRoot() 内部的 skill 设置 shadow 标记（同时覆盖自定义目录条目与仓库内部的 QWEN_HOME）；并在 listSubagentsAtLevel 做镜像处理（见 subagent-manager 的评论）。交叉引用：R15-9（subagent 侧，同一根因）、R15-5（同一判定条件的过度拦截方向——修复时不要整体移除 'user' 豁免）。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // rebinds to the worktree path, opening the gate for exactly the | ||
| // repo-supplied agents the shadow surfaced. | ||
| const trustedAgentLevel = | ||
| (config.level === 'user' && config.homeRootShadow !== true) || |
There was a problem hiding this comment.
[Critical] R15-9: The 'user'-level exemption assumes the user agents directory is ~/.qwen, but Storage.getGlobalQwenDir() honors QWEN_HOME (storage.ts:183-193; relative values resolve against cwd — documented in settings.md), and listSubagentsAtLevel('user') reads getGlobalQwenDir()/agents. When QWEN_HOME resolves inside the project root (e.g. a direnv/devcontainer convention QWEN_HOME="$PWD/.qwen-home"), repo-committed agent files ARE the 'user' level; projectRoot !== homedir, so no homeRootShadow tag is applied, this gate never calls isTrustedFolder(), and HookRegistry.addAgentHooks (which has no gate of its own) registers the repo-supplied hooks session-wide.
Failure scenario: probe-verified at this commit — QWEN_HOME=/test/project/.qwen-home, repo shipping only .qwen-home/agents/helper.md with a PreToolUse command hook, folder untrusted: listing returned level=user, homeRootShadow=undefined; addAgentHooks was called once with isTrustedFolder()=false; the project-level control arm was gated (0 calls). Exempting 'user' only when its file is outside getProjectRoot() flipped the observation. Precondition: the victim's QWEN_HOME must resolve inside the repo (monorepo tooling, CI checkouts, direnv layouts) — the repo cannot set QWEN_HOME itself; that affects trigger likelihood, not the mechanism.
Suggested fix: at listing time, also tag 'user'-level agents whose resolved directory lies inside the project root (or gate any 'user' agent whose filePath resolves under getProjectRoot()). This is the under-blocking twin of R15-5 and the subagent-side mirror of R15-6 — all three share one root cause (level label trusted instead of directory provenance) and the fixes should land together.
中文说明
[Critical] 'user' 级豁免假定 user agents 目录是 ~/.qwen，但 Storage.getGlobalQwenDir() 会尊重 QWEN_HOME（storage.ts:183-193；相对值按 cwd 解析——settings.md 有文档说明），而 listSubagentsAtLevel('user') 读取 getGlobalQwenDir()/agents。当 QWEN_HOME 解析到项目根目录内部（例如 direnv/devcontainer 约定 QWEN_HOME="$PWD/.qwen-home"）时，仓库提交的 agent 文件就是 'user' 级内容；projectRoot !== homedir，因此不会打 homeRootShadow 标记，此门禁根本不会调用 isTrustedFolder()，而 HookRegistry.addAgentHooks（自身没有门禁）会把仓库提供的 hooks 注册为全会话生效。
失败场景：已在本 commit 探针验证——QWEN_HOME=/test/project/.qwen-home，仓库仅携带 .qwen-home/agents/helper.md（含 PreToolUse 命令 hook），文件夹不可信：列表返回 level=user, homeRootShadow=undefined；addAgentHooks 在 isTrustedFolder()=false 时被调用一次；project 级对照分支被拦截（0 次调用）。把 'user' 豁免限定为文件位于 getProjectRoot() 之外后观测翻转。前提条件：受害者的 QWEN_HOME 需解析到仓库内部（monorepo 工具链、CI checkout、direnv 布局）——仓库自身无法设置 QWEN_HOME；这只影响触发概率，不改变机制。
修复建议：在列表阶段，对解析目录位于项目根内部的 'user' 级 agent 同样打标（或对 filePath 解析到 getProjectRoot() 之下的任何 'user' agent 执行门禁）。这是 R15-5 的欠拦截孪生问题、R15-6 的 subagent 侧镜像——三者同根因（信任 level 标签而非目录来源），修复应一起落地。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| it('should never resolve Qwen-internal secrets from process.env', () => { | ||
| process.env['QWEN_SERVER_TOKEN'] = 'daemon-secret'; |
There was a problem hiding this comment.
[Suggestion] R15-1: The PR body's "How to verify" instructs reviewers to run cd packages/cli && npx vitest run src/utils/envVarResolver.test.ts, but this diff DELETES that file (and the module it tested) — the denylist coverage now lives here in the core package, and the new channel-config denylist tests (packages/cli/src/commands/channel/config-utils.test.ts, "internal-secret denylist" describe) are not listed in the plan at all. Probe: the documented CLI command exits 1 with "No test files found".
Concrete cost: a reviewer verifying claim 3 follows the PR's own instructions and the CLI-side half of the denylist verification fails to execute, while the channel-credential path (the one that used to resolve $QWEN_SERVER_TOKEN) has no entry point in the test plan.
Suggested fix: update the PR body — drop the deleted CLI command (the core-side command already listed covers this file) and add cd packages/cli && npx vitest run src/commands/channel/config-utils.test.ts.
中文说明
[Suggestion] PR 正文的 "How to verify" 要求评审者运行 cd packages/cli && npx vitest run src/utils/envVarResolver.test.ts，但本 diff 删除了该文件（及其测试的模块）——denylist 覆盖现在位于 core 包的这个文件中，而新增的 channel 配置 denylist 测试（packages/cli/src/commands/channel/config-utils.test.ts 的 "internal-secret denylist" describe）完全没有列入验证计划。探针：按文档运行该 CLI 命令会以 "No test files found" 退出码 1 结束。
具体代价：评审者按 PR 自身的说明验证第 3 项时，CLI 侧的 denylist 验证无法执行，而 channel 凭据路径（过去会解析 $QWEN_SERVER_TOKEN 的那条）在测试计划中没有入口。
修复建议：更新 PR 正文——删除已删除文件的 CLI 命令（已列出的 core 侧命令覆盖本文件），并补充 cd packages/cli && npx vitest run src/commands/channel/config-utils.test.ts。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| - This setting is **only honored from User, System, and SystemDefaults settings scopes**. A value set in Workspace (project) settings is ignored and logged as a warning, so a cloned repository can never self-grant this bypass. | ||
| - The flag relaxes only the general private/CGNAT/link-local **range** checks. Cloud metadata endpoints stay blocked in every configuration: the `BLOCKED_HOSTS` list is matched literally (`metadata.google.internal`, `metadata.azure.internal`, ...), and the metadata IPs `169.254.169.254` and `100.100.100.200` are blocked in all serialized forms (including IPv4-mapped IPv6 such as `::ffff:a9fe:a9fe`) and after DNS resolution. | ||
| - The `security.allowedHttpHookUrls` whitelist still applies independently. In managed environments, pair this flag with a whitelist so only the intended internal endpoints are reachable. | ||
| - The `security.allowedHttpHookUrls` whitelist still applies independently. In managed environments, pair this flag with a whitelist so only the intended internal endpoints are reachable. Like this flag, the whitelist is **honored from User, System, and SystemDefaults settings scopes**; a value set in Workspace (project) settings can only _narrow_ the User or SystemDefaults whitelist and is logged as a warning: workspace entries that no higher-scope entry covers are dropped, when no higher scope sets a whitelist the workspace value is ignored entirely, and a System-scope whitelist always takes precedence over the workspace value (an empty whitelist means "allow all", so a repository can neither widen where hook payloads may be sent nor establish a whitelist of its own). |
There was a problem hiding this comment.
[Suggestion] R15-2: The PR description says "Both keys are now stripped from workspace scope" (and Risk & Scope says workspaces "must move it to user settings"), but the shipped behavior — this docs line, narrowWorkspaceHookSecurityOverrides in packages/cli/src/config/settings.ts, the schema descriptions, and the narrowing tests — is narrow-only intersection: a trusted workspace may still NARROW a higher-scope whitelist; only uncovered entries are dropped.
Probe: user ['https://hooks.corp.com/*'] + workspace ['https://hooks.corp.com/ci/*'] → effective ['https://hooks.corp.com/ci/*'] (the "should let a trusted workspace whitelist narrow the user whitelist" test passes at this commit).
Concrete cost: a maintainer validating claim 2 reads "stripped" and concludes a repository can never influence the effective whitelist, while the shipped code lets a trusted repo change it by narrowing; conversely, a user following the migration note moves a deliberately narrowing project whitelist to user scope and loses the per-project restriction.
Suggested fix: update the PR body before merge — describe item 2 as narrow-only intersection (workspace entries may only narrow a higher-scope whitelist; uncovered entries dropped; workspace ignored entirely when no higher scope sets one; System precedence), and replace the blanket migration note.
中文说明
[Suggestion] PR 描述写的是 "Both keys are now stripped from workspace scope"（风险与范围一节还要求把工作区白名单"移到 user settings"），但实际实现——本行文档、packages/cli/src/config/settings.ts 的 narrowWorkspaceHookSecurityOverrides、schema 描述以及收窄测试——是“仅收窄”的交集语义：受信任的工作区仍可收窄更高作用域的白名单，只有未被覆盖的条目会被丢弃。
探针：user ['https://hooks.corp.com/*'] + workspace ['https://hooks.corp.com/ci/*'] → 生效 ['https://hooks.corp.com/ci/*']（"should let a trusted workspace whitelist narrow the user whitelist" 测试在本 commit 通过）。
具体代价：维护者验证第 2 项时读到 "stripped"，会以为仓库永远无法影响生效白名单，而实际代码允许受信仓库通过收窄来改变它；反过来，用户照迁移说明把刻意收窄用的项目白名单移到 user 作用域，反而失去按项目粒度的限制。
修复建议：合并前更新 PR 正文——把第 2 项描述为仅收窄交集（workspace 条目只能收窄更高作用域白名单；未覆盖条目被丢弃；更高作用域未设置时 workspace 值被完全忽略；System 优先），并替换一刀切的迁移说明。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| 'Warning: security.allowedHttpHookUrls is not a list and was ignored; HTTP hooks are unrestricted apart from SSRF protection.', | ||
| ); | ||
| } | ||
| return Array.isArray(hookUrls) |
There was a problem hiding this comment.
[Suggestion] R15-3: An array whose entries are ALL non-strings is silently coerced to [] — which the UrlValidator reads as "allow all" — without the warning the adjacent branch adds for the identical outcome. Array.isArray is true, so the warning branch above is skipped; the filter drops every entry; the user's attempted restriction vanishes with no startup warning (the diff's own rationale comment says to "surface the lost restriction instead of silently starting unrestricted").
Failure scenario: probe at this commit — settings { security: { allowedHttpHookUrls: [42, null] } } → getAllowedHttpHookUrls() returned [] with zero warnings mentioning the field (expected 0 to be greater than 0); applying the implied fix flipped the probe and the suite stayed green. Boundary: mixed arrays keep their surviving strings; an explicitly empty [] stays warning-free by design ("If empty, all URLs are allowed").
const filtered = Array.isArray(hookUrls)
? hookUrls.filter((entry): entry is string => typeof entry === 'string')
: [];
if (Array.isArray(hookUrls) && hookUrls.length > 0 && filtered.length === 0) {
resolvedCliConfig.warnings.push(
'Warning: security.allowedHttpHookUrls contains no string entries and was ignored; HTTP hooks are unrestricted apart from SSRF protection.',
);
}
return filtered;中文说明
[Suggestion] 全部条目都不是字符串的数组会被静默强转为 []——UrlValidator 将其读作"全部允许"——却没有相邻分支为同样结果添加的那条告警。Array.isArray 为 true，因此上方的告警分支被跳过；filter 丢弃所有条目；用户试图设置的限制在没有启动告警的情况下消失（diff 自身的注释正写着"surface the lost restriction instead of silently starting unrestricted"）。
失败场景：已在本 commit 探针验证——settings { security: { allowedHttpHookUrls: [42, null] } } → getAllowedHttpHookUrls() 返回 [] 且没有任何提及该字段的告警（expected 0 to be greater than 0）；应用隐含修复后探针翻转、套件保持绿色。边界：混合数组保留存活的字符串；显式空数组 [] 按设计不告警（"If empty, all URLs are allowed"）。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| allowPrivateNetworkHooks: | ||
| bareMode || safeMode | ||
| ? false |
There was a problem hiding this comment.
[Suggestion] R15-10: The malformed-value hardening added four lines above covers only allowedHttpHookUrls; the adjacent allowPrivateNetworkHooks (next line) is still passed through raw, and every consumption site tests truthiness (!allowPrivateNetworkHosts && isBlockedAddress(...) in httpHookRunner.ts:66/92 and urlValidator.ts:126) — so any non-empty string silently ENABLES the private-network relaxation. Settings are env-interpolated whole before merging (resolveEnvVarsInObject runs on system/systemDefaults/user/workspace scopes), env values are always strings, and an unset variable leaves the literal $PLACEHOLDER — also truthy.
Failure scenario: probe at this commit — ALLOW_HOOKS_PRIVATE_IP=false with "security": { "allowPrivateNetworkHooks": "$ALLOW_HOOKS_PRIVATE_IP" } → loadCliConfig returned 'false' (string, truthy), and HttpHookRunner([], 'false') POSTed to a private-range URL, while the boolean-false control arm was blocked by the SSRF guard. Changing the next line to === true flipped the probe. Not attacker-reachable (workspace scope is stripped) — the direction is admin-disables → silently enabled; the docs section this PR edits ("pair this flag with a whitelist") is exactly where an admin would env-reference the flag.
Suggested fix: coerce on the next line — : settings.security?.allowPrivateNetworkHooks === true, — and push a warning into resolvedCliConfig.warnings when the value is present but not a boolean (mirroring the array field's warning).
中文说明
[Suggestion] 上方四行新增的畸形值加固只覆盖了 allowedHttpHookUrls；相邻的 allowPrivateNetworkHooks（下一行）仍原样透传，而所有消费点都做真值判断（httpHookRunner.ts:66/92 与 urlValidator.ts:126 的 !allowPrivateNetworkHosts && isBlockedAddress(...)）——因此任何非空字符串都会静默启用内网放行。settings 在合并前整体做环境变量插值（resolveEnvVarsInObject 作用于 system/systemDefaults/user/workspace 各作用域），环境变量值总是字符串，未设置的变量还会留下字面量 $PLACEHOLDER——同样是真值。
失败场景：已在本 commit 探针验证——ALLOW_HOOKS_PRIVATE_IP=false 且 "security": { "allowPrivateNetworkHooks": "$ALLOW_HOOKS_PRIVATE_IP" } → loadCliConfig 返回 'false'（字符串，真值），HttpHookRunner([], 'false') 成功 POST 到内网地址，而布尔 false 的对照分支被 SSRF 防护拦截。把下一行改为 === true 后探针翻转。攻击者不可达（workspace 作用域已被剥离）——方向是管理员关闭 → 被静默开启；本 PR 编辑的文档小节（"pair this flag with a whitelist"）恰是管理员会用环境变量引用该标志的地方。
修复建议：在下一行强转——: settings.security?.allowPrivateNetworkHooks === true,——并在值存在但不是布尔类型时向 resolvedCliConfig.warnings 推送告警（与数组字段的告警对称）。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| if (level === 'user' && isHomeDirectory) { | ||
| for (const subagent of subagents) { | ||
| subagent.homeRootShadow = true; |
There was a problem hiding this comment.
[Suggestion] R15-5: The tag assumes the user-level agents directory is <projectRoot>/.qwen/agents, but that only holds when Storage.getGlobalQwenDir() is ~/.qwen. When QWEN_HOME redirects the global dir elsewhere (storage.ts:183-186) and the project root is the home directory, genuinely user-supplied agents at $QWEN_HOME/agents are still tagged homeRootShadow: true even though nothing repo-supplied surfaces at that level — the spawn gate then silently drops their hooks in untrusted folders (pre-PR they registered unconditionally). Fail-closed direction: functionality loss only, no security exposure. Mirror instance in skill-manager.ts:1061-1072.
Failure scenario: QWEN_HOME=/custom/qwen, project root == home, folder untrusted — a legitimate user agent at /custom/qwen/agents/helper.md with hooks: frontmatter is listed at 'user' level, tagged, and its hooks are ignored with a debug-only warning.
Suggested fix: key the tag on actual directory coincidence — set homeRootShadow only for entries loaded from a user-level dir that coincides with (or lies inside) the project root. This is the same provenance fix as R15-6/R15-9 and resolves the over- and under-blocking directions together; do not drop the 'user' exemption wholesale (that would trade this bug for the other).
中文说明
[Suggestion] 该标记假定 user 级 agents 目录是 <projectRoot>/.qwen/agents，但这只在 Storage.getGlobalQwenDir() 为 ~/.qwen 时成立。当 QWEN_HOME 把全局目录重定向到别处（storage.ts:183-186）且项目根就是家目录时，位于 $QWEN_HOME/agents 的真正用户自带 agent 仍会被打上 homeRootShadow: true——尽管该层级并没有任何仓库提供的内容出现——spawn 门禁随后会在不可信文件夹中静默丢弃其 hooks（PR 前它们无条件注册）。fail-closed 方向：只是功能损失，无安全暴露。镜像实例在 skill-manager.ts:1061-1072。
失败场景：QWEN_HOME=/custom/qwen、项目根 == 家目录、文件夹不可信——位于 /custom/qwen/agents/helper.md 的合法用户 agent（带 hooks: frontmatter）以 'user' 级列出、被打标，其 hooks 被忽略且只有一条 debug 告警。
修复建议：按实际目录重合来打标——仅对从与项目根重合（或位于其内部）的 user 级目录加载的条目设置 homeRootShadow。这与 R15-6/R15-9 是同一个“按来源判定”的修复，可同时解决过度拦截与欠拦截两个方向；不要整体移除 'user' 豁免（那会把这个 bug 换成另一个）。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| if (level === 'user' && isHomeDirectory) { | ||
| for (const skill of skills) { | ||
| skill.homeRootShadow = true; |
There was a problem hiding this comment.
[Suggestion] R15-8: The home-root shadow tag applies to EVERY 'user'-level skill, including skills loaded from getCustomSkillDirs() directories, which the 'project' level never reads and which therefore cannot be repository-committed skills surfacing through the skipped project level. Running from ~ with the folder untrusted, the user's own custom-dir skills get their hooks silently deferred and allowedTools ignored — pre-PR they applied unconditionally.
Failure scenario: user has settings.skills.directories pointing at an absolute path outside the home (e.g. /srv/shared-team-skills); qwen runs from /home/user with the folder marked untrusted — listSkillsAtLevel('user') tags all user-level skills including the custom-dir ones, and both gates then defer/ignore the user-authored skill's side effects even though its directory can never coincide with a project-level dir.
Suggested fix: tag per origin directory — when collecting 'user'-level skills under isHomeDirectory, set homeRootShadow only for skills whose base dir is one of the provider config dirs (exclude getCustomSkillDirs() origins), or reuse the directory-coincidence predicate from the R15-5/R15-6/R15-9 fix.
中文说明
[Suggestion] home-root shadow 标记会打到每一个 'user' 级 skill 上，包括从 getCustomSkillDirs() 目录加载的 skill——而 'project' 级从不读取这些目录，因此它们不可能是经由被跳过的 project 级浮出的仓库提交 skill。当家目录作为项目根且文件夹不可信时，用户自己的自定义目录 skill 会被静默延迟注册 hooks、忽略 allowedTools——PR 前它们无条件生效。
失败场景：用户的 settings.skills.directories 指向家目录之外的绝对路径（如 /srv/shared-team-skills）；qwen 在 /home/user 运行且文件夹被标记为不可信——listSkillsAtLevel('user') 会给所有 user 级 skill（包括自定义目录的）打标，两处门禁随即延迟/忽略该用户自创 skill 的副作用，尽管其目录永远不可能与 project 级目录重合。
修复建议：按来源目录打标——在 isHomeDirectory 下收集 'user' 级 skill 时，仅对基础目录属于 provider 配置目录的 skill 设置 homeRootShadow（排除 getCustomSkillDirs() 来源），或复用 R15-5/R15-6/R15-9 修复中的目录重合判定。
— qwen3.8-max via Qwen Code /review (v0.21.6)
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts and pushed the branch update. Root causeMain's voice PR #8350 (732f4d8) landed after this branch's last merge. It renamed Semantic, not just textualBoth sides rewrote the workspace security strip. Resolution keeps the PR's function (strict superset) and grafts main's voice strip in; if (
WORKSPACE_STRIPPED_SECURITY_FIELDS.every(
(field) => security[field] === undefined,
) &&
security.allowedInsecureVoiceBaseUrls === undefined
) {
return workspace;
}
const restSecurity = { ...security };
delete restSecurity.allowPrivateNetworkHooks;
delete restSecurity.allowedInsecureVoiceBaseUrls;Call site kept: What is load-bearing
Could not verifyNo build/tests run here. Known breakage in the AUTO-MERGED, non-conflicted 中文说明根因：main 的语音 PR #8350（732f4d8a2）把 settings.ts 的 语义冲突：双方都重写了 workspace 安全剥离逻辑。解决方式：保留本 PR 函数（严格超集），把语音剥离嫁接进去——守卫加 关键点：语音字段必须同时在守卫与剥离两处——否则仅设置该字段的 workspace 会绕过函数泄漏进合并结果，破坏 #8350 的安全属性。语音列表整体剥离；收窄仅适用于 未能验证：未运行构建/测试。已知问题：自动合并（未冲突）的 settings.test.ts 中，main 的测试 |
|
@qwen-code /resolve |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge resolution — PR #8396 (fix/hooks-security ← main)Root causeBoth sides inserted a new alias at the same spot in
That was the only conflict; the remaining 211 commits from main merged cleanly. Textual, not semanticThe two alias additions are independent and both target files exist in the merged tree. The resolution keeps both: '@qwen-code/qwen-code-core/envVarResolver': path.resolve(
__dirname,
'../core/src/utils/envVarResolver.ts',
),
'@qwen-code/qwen-code-core/toolWriteOrigin': path.resolve(
__dirname,
'../core/src/services/tool-write-origin.ts',
),What is load-bearing
What I could not verifyNo build, typecheck, or tests were run. Cross-checks performed instead: 中文说明根因：PR #8396（提交 纯文本冲突，无语义冲突：两个别名互相独立，目标文件均存在，解决方案是两者都保留（见上方代码块）。 关键约束：① 两个子路径别名必须位于通用 未能验证：未运行构建/类型检查/测试。已做的交叉核对：合并结果对 origin/main 的差异与原 PR 统计完全一致（43 个文件，+2922/−525）； |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the case-insensitive env-denylist paths this PR adds are only exercisable on Windows.
Not explored to full depth (tool budget reached): "PR #8396 (QwenLM/qwen-code) security-hardening review,…": none — all checks I started were completed within budget.; "PR #8396 (QwenLM/qwen-code) security-hardening review,…": none — all checks above completed within budget (~24 calls).; "PR #8396 (QwenLM/qwen-code) security-hardening review,…": none** — all checks completed within budget.; "PR #8396 (QwenLM/qwen-code) security-hardening review,…": none — all checks above completed within budget.; "PR #8396 (QwenLM/qwen-code) security-hardening review,…": none — all planned checks completed within budget (~17 of ~52 calls used)., and 16 more.
Test Plan (not a blocker): 1385 tests green — this review observed 494, 1285, 1511, 3393, 555 passed.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the case-insensitive env-denylist paths this PR adds are only exercisable on Windows。
未探索到全部深度（达到工具调用预算）："PR #8396 (QwenLM/qwen-code) security-hardening review,…"：none — all checks I started were completed within budget.；"PR #8396 (QwenLM/qwen-code) security-hardening review,…"：none — all checks above completed within budget (~24 calls).；"PR #8396 (QwenLM/qwen-code) security-hardening review,…"：none** — all checks completed within budget.；"PR #8396 (QwenLM/qwen-code) security-hardening review,…"：none — all checks above completed within budget.；"PR #8396 (QwenLM/qwen-code) security-hardening review,…"：none — all planned checks completed within budget (~17 of ~52 calls used).，另有 16 条。
Test Plan（非阻断）：1385 tests green — this review observed 494, 1285, 1511, 3393, 555 passed。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if (narrowed.length > 0) { | ||
| restSecurity.allowedHttpHookUrls = narrowed; | ||
| } else { | ||
| delete restSecurity.allowedHttpHookUrls; | ||
| } |
There was a problem hiding this comment.
[Critical] The workspace-whitelist narrowing added here breaks a pre-existing, unmodified test — the packages/cli suite is red at this head. narrowWorkspaceHookSecurityOverrides deliberately discards a workspace-scoped security.allowedHttpHookUrls when no higher-scope whitelist exists, but the pre-existing test allowedInsecureVoiceBaseUrls scope handling > should strip and warn about the allowlist from workspace scope (settings.test.ts:3776, assertion at :3797, landed via the main merge 732f4d8) still asserts the old pass-through semantics. — Failure scenario: workspace settings define allowedHttpHookUrls: ['https://hooks.example.com/*'] with no User/System/SystemDefaults whitelist → higherUrls === undefined → narrowed = [] → delete → merged is undefined → AssertionError: expected undefined to deeply equal ['https://hooks.example.com/*']. Measured: npm test --workspace=packages/cli fails exactly this test at this head; the same test passes at the merge base (A/B-verified); a probe flip (asserting toBeUndefined()) passes. The discard is documented, intended behavior — the stale test is the overlooked artifact.
// settings.test.ts:3797 — update the stale assertion to the new semantics:
expect(settings.merged.security?.allowedHttpHookUrls).toBeUndefined();中文说明
此处新增的 workspace 白名单收窄逻辑破坏了一个既有且未被本 PR 修改的测试——packages/cli 测试套件在当前 head 上是红的。narrowWorkspaceHookSecurityOverrides 在更高 scope 未设置白名单时会刻意丢弃 workspace scope 的 security.allowedHttpHookUrls，但既有测试 allowedInsecureVoiceBaseUrls scope handling > should strip and warn about the allowlist from workspace scope（settings.test.ts:3776，断言位于 :3797，经由 main 合并 732f4d8 进入本分支）仍在断言旧的直通合并语义。——失败场景：workspace 设置定义 allowedHttpHookUrls: ['https://hooks.example.com/*'] 且 User/System/SystemDefaults 均未设置白名单 → higherUrls === undefined → narrowed = [] → delete → 合并结果为 undefined → 断言失败。实测：npm test --workspace=packages/cli 在本 head 上恰好失败于该测试；同一测试在合并基上通过（已做 A/B 验证）；探针翻转（改为断言 toBeUndefined()）后通过。丢弃行为本身是有文档记载的预期行为——被遗漏的是这个过时的测试。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // Side effects skipped by the trust gate on first load are applied | ||
| // on re-invocation once the folder becomes trusted (the gate is | ||
| // live), without re-injecting the skill body into context. Drop the | ||
| // entry only once the side effects were actually applied, so | ||
| // still-untrusted re-invocations keep the deferral alive. |
There was a problem hiding this comment.
[Critical] This gate is live in one direction only: untrusted→trusted re-applies deferred repo-supplied side effects, but trusted→untrusted never revokes already-applied ones. Config.isTrustedFolder() live-reads the IDE context on every call (a mid-session revocation flips it instantly), yet registered skill hooks keep executing — unregisterSkillHooks (registerSkillHooks.ts:136) is a no-op stub with zero call sites, HookSystem.clearSessionHooks has zero production callers, HookEventHandler.executeHooks merges session hooks into every event with no trust check, and addSessionAllowRule grants have no removal API. IDE trust revocation only surfaces the non-blocking IdeTrustChangeDialog banner ('Press r to restart'). — Failure scenario: IDE mode, trusted workspace; the user invokes a repo skill whose SKILL.md declares command hooks → hooks register in the shared SessionHooksManager; mid-session the IDE revokes trust → repo-controlled hooks keep executing on every event (background agents inherit the same hook system/sessionId and never surface the banner) and the session-wide allowedTools grants keep auto-approving — the exact surface this security-hardening PR exists to gate. Maintainer ruling needed on whether restart-based revocation is the accepted mechanism (settings-file hooks share the same restart-based pattern) — but this diff introduced liveness into this gate and implemented only the capability-granting half.
// Fix: implement the removal side — track hook IDs per skill at registration,
// implement unregisterSkillHooks for real, and purge session hooks + session
// allow rules from the trust-change listener; or re-check isTrustedFolder()
// in executeHooks before merging session hooks.中文说明
这个门禁只在一个方向上是"活"的：不可信→受信时会重新应用被延迟的仓库侧效果，但受信→不可信时从不撤销已经应用的效果。Config.isTrustedFolder() 每次调用都实时读取 IDE 上下文（会话中途撤销信任会立即翻转其结果），但已注册的 skill hooks 仍会继续执行——unregisterSkillHooks（registerSkillHooks.ts:136）是一个零调用点的空壳 stub，HookSystem.clearSessionHooks 没有任何生产调用者，HookEventHandler.executeHooks 在合并 session hooks 到每个事件时不做任何信任检查，addSessionAllowRule 授予的规则也没有移除 API。IDE 侧的信任撤销只会弹出非阻塞的 IdeTrustChangeDialog 横幅（"按 r 重启"）。——失败场景：IDE 模式、受信工作区；用户调用一个 SKILL.md 声明了 command hooks 的仓库 skill → hooks 注册进共享的 SessionHooksManager；会话中途 IDE 撤销信任 → 仓库可控的 hooks 在每个事件上继续执行（后台 agent 继承同一 hook 系统/sessionId，永远不会看到横幅），会话级 allowedTools 授权也继续自动批准——这正是本安全加固 PR 要门禁的表面。需要维护者裁决"基于重启的撤销"是否为可接受机制（settings 文件 hooks 同样是重启生效模式）——但本 diff 为这个门禁引入了活性，却只实现了授予能力的那一半。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if (specifier === '@qwen-code/qwen-code-core/memoryScopes') { | ||
| return { | ||
| shortCircuit: true, | ||
| url: memoryScopesSourceUrl, | ||
| format: 'module', | ||
| }; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The remap set this hunk extends is still missing the toolWriteOrigin core subpath — the comment's own invariant ('each one cli source imports needs its own remap to the source file') remains violated. Cli source imports @qwen-code/qwen-code-core/toolWriteOrigin at packages/cli/src/serve/bridge-file-system-adapter.ts:62 and packages/cli/src/acp-integration/service/filesystem.ts:25; this same hunk fixed the sibling pre-existing memoryScopes gap while leaving toolWriteOrigin out. — Concrete cost: probe-verified at this head: with no built packages/core/dist, node scripts/dev.js serve --port 0 exits 1 with ERR_MODULE_NOT_FOUND .../dist/src/services/tool-write-origin.js imported from .../bridge-file-system-adapter.ts; adding the remap flips the probe. Affects npm run dev:daemon / npm run dev -- serve on fresh checkouts.
// Fix: mirror the two new remaps —
const toolWriteOriginSourceUrl = pathToFileURL(
join(root, 'packages', 'core', 'src', 'services', 'tool-write-origin.ts'),
).href;
// + interpolate into loaderCode and add an exact-match branch for
// '@qwen-code/qwen-code-core/toolWriteOrigin' returning that URL.中文说明
本 hunk 扩展的 remap 集合仍然遗漏了 toolWriteOrigin 这个 core 子路径——注释自己声明的不变量（"cli 源码引入的每一个子路径都需要一条到源文件的 remap"）仍未满足。cli 源码在 packages/cli/src/serve/bridge-file-system-adapter.ts:62 与 packages/cli/src/acp-integration/service/filesystem.ts:25 引入 @qwen-code/qwen-code-core/toolWriteOrigin；同一个 hunk 修复了同类的既有 memoryScopes 缺口，却漏掉了 toolWriteOrigin。——具体代价：已在本 head 探针验证：在没有构建产物 packages/core/dist 的检出上，node scripts/dev.js serve --port 0 以 ERR_MODULE_NOT_FOUND .../dist/src/services/tool-write-origin.js imported from .../bridge-file-system-adapter.ts 退出（exit 1）；补上该 remap 即可翻转探针。影响全新检出上的 npm run dev:daemon / npm run dev -- serve。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| "@qwen-code/qwen-code-core/envVarResolver": [ | ||
| "../core/src/utils/envVarResolver.ts" | ||
| ], |
There was a problem hiding this comment.
[Suggestion] The new envVarResolver subpath gets resolution entries here, in packages/cli/vitest.config.ts, and in core's exports map — but not in integration-tests/tsconfig.json, whose own comment mandates 'every subpath the program imports needs an explicit entry naming its source file … Keep these in sync with the packages' exports maps'. integration-tests/terminal-capture/skill-review-harness/text-capture.tsx:27 imports cli settings.ts relatively, pulling the new subpath import (settings.ts:34, and config-utils.ts:7) into the tsc -p integration-tests program. — Concrete cost: probe-verified: on a clean checkout (no built dist) tsc -p integration-tests --noEmit reports TS2307: Cannot find module '@qwen-code/qwen-code-core/envVarResolver' at both sites; adding the missing entry removes both errors. With a stale dist present the subpath silently resolves to stale declarations — the two failure modes that comment says these entries exist to prevent. Blast radius is editor TS servers and ad-hoc runs (nothing in CI runs that program today).
// Fix: add to integration-tests/tsconfig.json paths:
"@qwen-code/qwen-code-core/envVarResolver": [
"../packages/core/src/utils/envVarResolver.ts"
]
// (consider `toolWriteOrigin` too — also absent there while imported by cli sources)中文说明
新的 envVarResolver 子路径在此处、packages/cli/vitest.config.ts 以及 core 的 exports map 中都拿到了解析条目——唯独 integration-tests/tsconfig.json 没有，而该文件的注释明确要求"程序引入的每一个子路径都需要一条指名源文件的显式条目……与包的 exports map 保持同步"。integration-tests/terminal-capture/skill-review-harness/text-capture.tsx:27 以相对路径引入 cli 的 settings.ts，把新的子路径引入（settings.ts:34，以及 config-utils.ts:7）拉进了 tsc -p integration-tests 程序。——具体代价：已探针验证：在干净检出（无构建产物）上，tsc -p integration-tests --noEmit 在两处均报 TS2307: Cannot find module '@qwen-code/qwen-code-core/envVarResolver'；补上缺失条目后两个错误消失。若存在过期的 dist，该子路径会悄悄解析到过期的声明文件——正是该注释所说这些条目要避免的两类失败。影响范围是编辑器 TS 服务与临时 typecheck（目前 CI 不运行该程序）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| // Never follow redirects: the whitelist and DNS-level SSRF | ||
| // checks above cover only this URL, and a 307/308 would re-send | ||
| // the hook payload to an unvalidated target. A 3xx response | ||
| // falls into the non-2xx branch below (non-blocking error). |
There was a problem hiding this comment.
[Critical] R14-1 re-checked at d4a76b9 and STILL STANDS: the whitelist static check regex-tests the RAW interpolated hook URL string, but fetch() sends the WHATWG-normalized URL — dot-segment removal moves the hook payload to a path on a user-whitelisted host that the whitelist does not cover. This is the same static-check/runtime-consumer divergence class this PR closes for redirects (as this very comment describes), left open for normalization. — Failure scenario: user-scope whitelist ['https://corp.example.com/api/*']; a trusted repo configures hook url https://corp.example.com/api/../telemetry/exfil → validate() tests the raw string → allowed; fetch normalizes and POSTs the hook payload (prompts, tool inputs/outputs) to https://corp.example.com/telemetry/exfil, which the whitelist regex does NOT match. Probe-verified end-to-end with a real fetch in prior rounds; every path-shaped pattern (…/prefix/* — all documented examples) is bypassable.
// Fix: parse once and validate the exact string that will be fetched:
const parsed = new URL(url); // invalid URLs already fail closed upstream
// run isBlocked + isAllowed against parsed.href, then fetch(parsed.href, ...)中文说明
R14-1 已在 d4a76b9 复查，仍然存在：白名单静态检查对原始插值后的 hook URL 字符串做正则匹配，但 fetch() 发送的是 WHATWG 规范化后的 URL——点段移除会把 hook 负载投递到用户白名单主机上一个白名单并未覆盖的路径。这与本 PR 为重定向关闭的"静态检查/运行时消费者分歧"是同一类问题（正如本条注释所描述），但在规范化方向上仍然敞开。——失败场景：user scope 白名单 ['https://corp.example.com/api/*']；受信仓库配置 hook url 为 https://corp.example.com/api/../telemetry/exfil → validate() 对原始字符串判定放行；fetch 规范化后把 hook 负载（prompt、工具输入/输出）POST 到白名单正则并不匹配的 https://corp.example.com/telemetry/exfil。此前几轮已用真实 fetch 端到端探针验证；所有路径形态的模式（…/prefix/*——即全部文档示例）均可被绕过。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const trustedAgentLevel = | ||
| (config.level === 'user' && config.homeRootShadow !== true) || | ||
| config.level === 'builtin' || | ||
| config.level === 'extension' || | ||
| config.level === 'session'; |
There was a problem hiding this comment.
[Critical] R15-9 re-checked at d4a76b9 and STILL STANDS: the 'user' arm of this allowlist assumes the user agents directory is ~/.qwen, but Storage.getGlobalQwenDir() honors QWEN_HOME (storage.ts:183-193; relative values resolve against cwd) and listSubagentsAtLevel('user') reads getGlobalQwenDir()/agents. When QWEN_HOME resolves inside the project root (e.g. a direnv/devcontainer convention), repo-controlled agents surface at 'user' level WITHOUT the homeRootShadow tag — the tag is set only when path.resolve(projectRoot) === path.resolve(os.homedir()), which that geometry falsifies. — Failure scenario: QWEN_HOME=.qwen-home (relative, inside the repo) → $QWEN_HOME/agents/*.md with frontmatter hooks list at 'user' level untagged → this gate exempts them → repo-supplied agent hooks register in an untrusted folder. Fix (shared root cause with R15-6): base the shadow decision on directory containment — tag per-agent when the resolved agents directory (or loaded filePath) is inside path.resolve(projectRoot), regardless of home equality.
中文说明
R15-9 已在 d4a76b9 复查，仍然存在：此允许清单的 'user' 分支假设用户级 agents 目录是 ~/.qwen，但 Storage.getGlobalQwenDir() 会遵循 QWEN_HOME（storage.ts:183-193；相对值按 cwd 解析），而 listSubagentsAtLevel('user') 读取的是 getGlobalQwenDir()/agents。当 QWEN_HOME 解析到项目根目录内部（例如 direnv/devcontainer 约定）时，仓库可控的 agents 会以 'user' 级别出现且不带 homeRootShadow 标记——该标记只在 path.resolve(projectRoot) === path.resolve(os.homedir()) 时设置，而上述几何形状恰好使其为假。——失败场景：QWEN_HOME=.qwen-home（相对路径，位于仓库内）→ 带 frontmatter hooks 的 $QWEN_HOME/agents/*.md 以未打标的 'user' 级别被列出 → 此门禁将其豁免 → 仓库提供的 agent hooks 在不可信文件夹中被注册。修复（与 R15-6 同根因）：把 shadow 判定改为基于目录包含关系——当解析出的 agents 目录（或加载的 filePath）位于 path.resolve(projectRoot) 内部时逐 agent 打标，而不依赖与 home 目录的相等比较。
— qwen3.8-max via Qwen Code /review (v0.21.10)
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): 1385 tests green — this review observed 19368, 19901 passed.
中文说明
Test Plan（非阻断）：1385 tests green — this review observed 19368, 19901 passed。
— qwen-code via Qwen Code /review (v0.21.11)
| */ | ||
| private appendSystemMessage(merged: HookOutput, output: HookOutput): void { | ||
| if (output.systemMessage !== undefined) { | ||
| merged.systemMessage = merged.systemMessage |
There was a problem hiding this comment.
[Critical] Preserve suppressOutput per producing message. The new concatenation appends every systemMessage, while mergeWithOrLogic independently keeps the last defined suppression value. For example, { systemMessage: 'private diagnostic', suppressOutput: true } followed by { systemMessage: 'visible', suppressOutput: false } aggregates to both messages with suppression disabled; processCommonHookOutputFields then emits the private diagnostic too. Filter suppressed messages before concatenation or retain per-message metadata until emission, and add a mixed suppressed/visible regression test.
[Critical] 请按消息来源分别保留 suppressOutput。新的拼接逻辑会加入每个 systemMessage，但 mergeWithOrLogic 独立采用最后一个已定义的 suppression 值。例如先返回 { systemMessage: 'private diagnostic', suppressOutput: true }，再返回 { systemMessage: 'visible', suppressOutput: false }，聚合结果会包含两条消息且 suppression 被关闭，最终私有诊断也会被输出。请在拼接前过滤被抑制的消息，或保留逐消息元数据直到输出阶段，并增加“抑制消息 + 可见消息”的回归测试。
— qwen-code via Qwen Code /review (v0.21.11)
| merged.systemMessage = merged.systemMessage | ||
| ? [merged.systemMessage, output.systemMessage] | ||
| .filter(Boolean) | ||
| .join('\n') |
There was a problem hiding this comment.
[Critical] Add a final aggregate size limit here. Command and HTTP hook runners cap individual outputs, but this loop concatenates an unbounded number of individually valid messages; a large hook set can therefore create a multi-megabyte systemMessage that is logged and emitted to terminal/ACP consumers as one payload. Cap while appending or truncate the final aggregate deterministically, and cover a many-hook case.
[Critical] 请为最终聚合结果增加总长度上限。命令和 HTTP hook runner 只限制单个输出，而这里可以拼接任意数量的合法消息；大量 hooks 因此能够生成数 MB 的 systemMessage，并作为一个 payload 写入日志及终端/ACP 消费端。请在追加时限制剩余容量，或对最终结果进行确定性截断，并增加多 hook 场景测试。
— qwen-code via Qwen Code /review (v0.21.11)
| if (!response.ok) { | ||
| if (response.status >= 300 && response.status < 400) { | ||
| // A redirect delivers no payload, so it must not consume a | ||
| // once hook's single execution: drop the slot added above so |
There was a problem hiding this comment.
[Critical] Restrict this branch to actual Fetch redirect statuses (301, 302, 303, 307, 308). 304 Not Modified is currently classified as a redirect, gets the redirect warning, and deletes a once hook's execution marker; the supposedly one-shot hook then sends another request on every matching event. Let other non-2xx 3xx statuses use the generic error path and add a 304 regression test.
[Critical] 请把该分支限制为 Fetch 实际识别的重定向状态码（301、302、303、307、308）。当前 304 Not Modified 也会被当作重定向，输出重定向警告并删除 once hook 的执行标记，导致本应只执行一次的 hook 在后续每个匹配事件中再次请求。其他非 2xx 的 3xx 状态应走通用错误路径，并增加 304 回归测试。
— qwen-code via Qwen Code /review (v0.21.11)
|
|
||
| const INTERNAL_SECRET_ENV_VARS_UPPER = new Set( | ||
| INTERNAL_SECRET_ENV_VARS.map((v) => v.toUpperCase()), | ||
| ); |
There was a problem hiding this comment.
[Critical] Include QWEN_CODE_EXTERNAL_TOOL_GUARD_TOKEN in the shared internal-secret denylist. Serve startup intentionally retains this daemon-local credential while fast-path settings are interpolated, and deletes the original only afterward. A trusted workspace setting can therefore reference it and copy the value under another environment key, after which repository hooks or child commands inherit the credential. Cover settings resolution, child-env sanitization, and the serve fast path.
[Critical] 请把 QWEN_CODE_EXTERNAL_TOOL_GUARD_TOKEN 加入共享内部秘密 denylist。serve 启动时会在 fast-path settings 插值期间保留这个 daemon 本地凭证，之后才删除原始变量；因此可信 workspace 设置可以引用它并把值复制到另一个环境变量名，随后仓库 hooks 或子进程命令就能继承该凭证。请覆盖 settings 解析、子进程环境清理和 serve fast path。
— qwen-code via Qwen Code /review (v0.21.11)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Unresolved, please confirm:
- [Critical] R16-2 (thread at packages/core/src/tools/skill.ts:609): one-directional gate liveness — the trusted→untrusted revocation half is factually absent at this head (registered skill hooks keep executing; unregisterSkillHooks is a no-op stub with zero call sites, clearSessionHooks has no production callers, session allow rules have no removal API), but the thread itself awaits a maintainer ruling on whether restart-based revocation is the accepted mechanism (settings-file hooks share the same restart-based pattern, and pre-PR behavior was identical — the diff narrows the grant side without widening the revocation gap). Could not be ruled fixed or defect from code alone.
Not reviewed: reverse audit — reached the 5-round cap without two consecutive dry rounds (rounds 3, 4 and 5 each reported findings; the loop ended at the cap, not by convergence).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the case-insensitive env-denylist paths this PR adds are only exercisable on Windows.
Not reviewed: build-and-test — packages/webui test suite did not run (whole-call budget); test-efficacy probe capped (47 hunk probes / 7 mutants unprobed).
Not explored to full depth (tool budget reached): "agent reverse-audit (round 4)": verify the tool scheduler actually executes same-turn tool calls concurrently (my grep in packages/core/src/core found no tool-execution loop; the finding's inc…; "agent 1c": full packages/cli and packages/core test suites and integration tests were not run — only the test files the PR touches plus package-level typechecks.; "agent reverse-audit (round 2)": Config.isTrustedFolder() semantics (core/src/config/config.ts:7331) not read — trust default in non-IDE mode unverified**; "agent reverse-audit (round 2)": packages/cli suite not run — chunk's tests' green status taken from confirmed finding #1, which attributes the red suite solely to settings.test.ts**; "agent reverse-audit (round 1)": conclusively verifying whether project-level extensions require explicit user consent/enablement before activation (the covering control for the extension arm….
[Critical] R17-6 (re-asserts @wenshao's open blocker, comment 3779654325 at packages/core/src/utils/sanitize-child-env.ts:37 — could not be anchored to a changed line; the denylist entries are unchanged context): QWEN_CODE_EXTERNAL_TOOL_GUARD_TOKEN is absent from INTERNAL_SECRET_ENV_VARS — re-checked at this head and still standing, the denylist is unchanged (exactly QWEN_SERVER_TOKEN, QWEN_DAEMON_TOKEN, and the private ACP capability). Serve startup intentionally retains this daemon-local credential while fast-path settings are interpolated and deletes the original only afterward; a trusted workspace setting can reference it and copy the value under another env key, after which repository hooks or child commands can exfiltrate it. Its three denylisted siblings have the identical lifecycle; this one is omitted, so the invariant this PR canonizes ('Qwen-internal secrets are never substituted into hook commands, URLs, or headers') is false for it. Fix: add 'QWEN_CODE_EXTERNAL_TOOL_GUARD_TOKEN' to INTERNAL_SECRET_ENV_VARS.
中文说明
仅完成部分审查，审查缺口已披露。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未审查：reverse audit — reached the 5-round cap without two consecutive dry rounds (rounds 3, 4 and 5 each reported findings; the loop ended at the cap, not by convergence)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the case-insensitive env-denylist paths this PR adds are only exercisable on Windows。
未审查：build-and-test — packages/webui test suite did not run (whole-call budget); test-efficacy probe capped (47 hunk probes / 7 mutants unprobed)。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 4)"：verify the tool scheduler actually executes same-turn tool calls concurrently (my grep in packages/core/src/core found no tool-execution loop; the finding's inc…；"agent 1c"：full packages/cli and packages/core test suites and integration tests were not run — only the test files the PR touches plus package-level typechecks.；"agent reverse-audit (round 2)"：Config.isTrustedFolder() semantics (core/src/config/config.ts:7331) not read — trust default in non-IDE mode unverified**；"agent reverse-audit (round 2)"：packages/cli suite not run — chunk's tests' green status taken from confirmed finding #1, which attributes the red suite solely to settings.test.ts**；"agent reverse-audit (round 1)"：conclusively verifying whether project-level extensions require explicit user consent/enablement before activation (the covering control for the extension arm…。
[Critical] R17-6 (re-asserts @wenshao's open blocker, comment 3779654325 at packages/core/src/utils/sanitize-child-env.ts:37 — could not be anchored to a changed line; the denylist entries are unchanged context): QWEN_CODE_EXTERNAL_TOOL_GUARD_TOKEN is absent from INTERNAL_SECRET_ENV_VARS — re-checked at this head and still standing, the denylist is unchanged (exactly QWEN_SERVER_TOKEN, QWEN_DAEMON_TOKEN, and the private ACP capability). Serve startup intentionally retains this daemon-local credential while fast-path settings are interpolated and deletes the original only afterward; a trusted workspace setting can reference it and copy the value under another env key, after which repository hooks or child commands can exfiltrate it. Its three denylisted siblings have the identical lifecycle; this one is omitted, so the invariant this PR canonizes ('Qwen-internal secrets are never substituted into hook commands, URLs, or headers') is false for it. Fix: add 'QWEN_CODE_EXTERNAL_TOOL_GUARD_TOKEN' to INTERNAL_SECRET_ENV_VARS.
— qwen3.8-max via Qwen Code /review (v0.21.12)
| if (narrowed.length > 0) { | ||
| restSecurity.allowedHttpHookUrls = narrowed; | ||
| } else { | ||
| delete restSecurity.allowedHttpHookUrls; |
There was a problem hiding this comment.
[Critical] R16-1: The workspace-whitelist narrowing added here breaks a pre-existing, unmodified test — the packages/cli suite is red at this head (re-checked this round; still stands). narrowWorkspaceHookSecurityOverrides deliberately discards a workspace-scoped security.allowedHttpHookUrls when no higher-scope whitelist exists, but the pre-existing test allowedInsecureVoiceBaseUrls scope handling > should strip and warn about the allowlist from workspace scope (settings.test.ts:3776, assertion at :3797) still asserts the old pass-through semantics. The sibling stale test was updated; this one was missed. — Failure scenario: workspace settings define allowedHttpHookUrls: ['https://hooks.example.com/*'] with no User/System/SystemDefaults whitelist → higherUrls === undefined → narrowed = [] → delete → merged is undefined → AssertionError: expected undefined to deeply equal ['https://hooks.example.com/*'] → npm test --workspace=packages/cli and CI fail.
Witness (this round): the test fails in isolation on the PR branch, passes at merge base c396fe3d in the identical environment, and still fails with QWEN_HOME unset (1 failed | 176 passed) — PR-caused, not environmental. The discard is documented, intended behavior; the stale test is the overlooked artifact.
// settings.test.ts:3797 — update the stale assertion to the new semantics:
expect(settings.merged.security?.allowedHttpHookUrls).toBeUndefined();中文说明
[Critical] R16-1：此处新增的 workspace 白名单收窄逻辑破坏了一个既有且未被本 PR 修改的测试——packages/cli 测试套件在当前 head 上是红的（本轮复查确认仍然存在）。narrowWorkspaceHookSecurityOverrides 在更高 scope 未设置白名单时会刻意丢弃 workspace scope 的 security.allowedHttpHookUrls，但既有测试 allowedInsecureVoiceBaseUrls scope handling > should strip and warn about the allowlist from workspace scope（settings.test.ts:3776，断言位于 :3797）仍在断言旧的直通合并语义。同类的另一个过时测试已更新，这一个被遗漏了。——失败场景：workspace 设置定义 allowedHttpHookUrls: ['https://hooks.example.com/*'] 且 User/System/SystemDefaults 均未设置白名单 → higherUrls === undefined → narrowed = [] → delete → 合并结果为 undefined → 断言失败 → npm test --workspace=packages/cli 与 CI 失败。
见证（本轮）：该测试在 PR 分支上单独运行即失败，在合并基 c396fe3d 的相同环境下通过，且移除 QWEN_HOME 后仍然失败（1 failed | 176 passed）——是 PR 导致的，不是环境问题。丢弃行为本身是有文档记载的预期行为，被遗漏的是这个过时的测试。
修复：把 settings.test.ts:3797 的断言更新为新语义 expect(settings.merged.security?.allowedHttpHookUrls).toBeUndefined();。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| export function hookUrlPatternCovers( | ||
| outerPattern: string, | ||
| innerPattern: string, |
There was a problem hiding this comment.
[Critical] R17-1: The whitelist boundary is enforced by regex-testing the RAW URL string — both in validate()/compilePattern and in this new hookUrlPatternCovers gate — but neither models WHATWG URL parsing, so URL-structure tokens bypass it. Probe-verified against this head, three token families (with wildcard-host patterns such as https://*.corp.com/*): (1) userinfo — https://x@evil.com/.corp.com/payload validates allowed: true while new URL(...).hostname is evil.com, where fetch() sends the payload; hookUrlPatternCovers also certifies such entries as legitimate narrowings. (2) fragment / newline — a workspace entry https://evil.com#.corp.com/* is certified as covered (# is not in regexActive), survives narrowWorkspaceHookSecurityOverrides and REPLACES the higher-scope list, matches at runtime, and fetch strips the fragment → payload POSTs to evil.com; the \n axis behaves the same (WHATWG strips \n\r\t). (3) authority/path boundary — https://evil.com/.corp.com/payload and https://evil.com\.corp.com/x are both isAllowed: true with parsed hostname evil.com (backslash folds to / mid-authority), and covers() certifies https://evil.com/.corp.com/* under https://*.corp.com/*. This defeats the guarantee this PR writes into settings.ts, the schema, and the docs — that a workspace may only NARROW where hook payloads may be sent — and subsumes prior-round R14-1 (raw string vs normalized fetch). — Failure scenario: user/system whitelist ['https://*.corp.com/*']; a trusted-but-malicious repo adds the workspace narrowing entry ['https://evil.com#.corp.com/*'] plus an HTTP hook targeting it → the entry survives narrowing, becomes the effective whitelist, and the hook payload (prompts, tool inputs/outputs) POSTs to evil.com.
Witness (probes this round, unmodified PR): userinfo — validate('https://x@evil.com/.corp.com/payload').allowed === true vs hostname evil.com (flip: rejecting URL.username + @ in covers → 5/5 cases pass, all 41 existing tests green). Fragment/newline — covers=true, survivesNarrowing=['https://evil.com#.corp.com/*'], runtime allowed, hostname evil.com; after fail-closed fix on [# \n \r \t \0]: covers=false, controls unchanged. Authority boundary — both URLs isAllowed: true, hostname evil.com; E2E with a real HttpHookRunner + local HTTP server: payload received at /.corp.com/exfil; with a parse-based origin guard: validate: false, 0 requests received. Note: normalizing input to its serialized href does NOT close / and \ — the serialized href still textually matches.
Suggested fix: close the class structurally — parse the hook URL once with new URL() and enforce the pattern against parsed components (reject non-empty username, fragments and C0 control chars; compare hostname/path against the pattern's authority/path split), and lift hookUrlPatternCovers's contract to the same fetch-destination semantics (fail closed on @, #, and control characters in either pattern).
中文说明
[Critical] R17-1：白名单边界靠对原始 URL 字符串做正则测试来执行——validate()/compilePattern 如此，本 PR 新增的 hookUrlPatternCovers 门禁亦如此——但两者都没有建模 WHATWG URL 解析，因此 URL 结构 token 可以绕过它。已在本 head 上用探针验证三类 token（针对 https://*.corp.com/* 这类通配主机模式）：(1) userinfo——https://x@evil.com/.corp.com/payload 校验结果为 allowed: true，而 new URL(...).hostname 是 evil.com，fetch() 实际把负载发往该主机；hookUrlPatternCovers 还会把这类条目认证为合法收窄。(2) fragment / 换行——workspace 条目 https://evil.com#.corp.com/* 被认证为被覆盖（# 不在 regexActive 中），在 narrowWorkspaceHookSecurityOverrides 中存活并替换更高 scope 列表，运行时匹配通过，fetch 剥掉 fragment → 负载 POST 到 evil.com；\n 轴同理（WHATWG 会剥掉 \n\r\t）。(3) authority/path 边界——https://evil.com/.corp.com/payload 与 https://evil.com\.corp.com/x 均 isAllowed: true，解析主机名为 evil.com（反斜杠在 authority 中段被折叠为 /），且 covers() 把 https://evil.com/.corp.com/* 认证在 https://*.corp.com/* 之下。这击穿了本 PR 写入 settings.ts、schema 与文档的保证——workspace 只能收窄 hook 负载的去向——并涵盖上一轮的 R14-1（原始字符串 vs 归一化 fetch）。——失败场景：user/system 白名单 ['https://*.corp.com/*']；受信但恶意的仓库添加 workspace 收窄条目 ['https://evil.com#.corp.com/*'] 及指向它的 HTTP hook → 条目存活收窄、成为生效白名单，hook 负载（prompt、工具输入/输出）POST 到 evil.com。
见证（本轮探针，未修改的 PR）：userinfo——validate('https://x@evil.com/.corp.com/payload').allowed === true 而 hostname 为 evil.com（翻转：拒绝 URL.username 并把 @ 加入 covers 的 fail-closed 集 → 5/5 用例通过，现有 41 个测试全绿）。Fragment/换行——covers=true、survivesNarrowing=['https://evil.com#.corp.com/*']、运行时放行、hostname evil.com；对 [# \n \r \t \0] fail-closed 后 covers=false，对照不变。Authority 边界——两个 URL 均 isAllowed: true、hostname evil.com；用真实 HttpHookRunner + 本地 HTTP server 的端到端：负载到达 /.corp.com/exfil；加上基于解析的 origin 守卫后 validate: false、0 请求到达。注意：把输入归一化为序列化 href 不能关闭 / 与 \——序列化 href 在文本上仍然匹配。
修复建议：以结构化方式关闭整类问题——用 new URL() 解析一次 hook URL，对解析后的分量执行模式匹配（拒绝非空 username、fragment 与 C0 控制字符；按模式的 authority/path 切分比较 hostname/path），并把 hookUrlPatternCovers 的契约提升到同样的 fetch 目的地语义（任一模式含 @、#、控制字符时 fail closed）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| * — are repo-controllable, so callers gate them on folder trust and this | ||
| * check fails closed. | ||
| */ | ||
| export function isTrustedSkillLevel(level: SkillLevel | undefined): boolean { |
There was a problem hiding this comment.
[Critical] R17-2: R14-2 (thread at packages/core/src/tools/skill.test.ts:686) re-checked at this head and STILL STANDS — independently re-detected twice by this round's audit. The 'extension'-level exemption violates the gate's own fail-closed premise in the home-root topology: when the project root IS $HOME, ~/.qwen/extensions == <repo>/.qwen/extensions is repo-controlled; repo-committed extensions are discovered by the default cache refresh and auto-activate with zero user consent (ExtensionStore mints defaultActivation: 'enabled' with no store policy), and their skills/agents surface at level 'extension', where isTrustedSkillLevel skips the folder-trust gate unconditionally — homeRootShadow is only tagged for level === 'user', so the identical topology is hardened there and left open here. Repo-supplied frontmatter allowedTools become session-wide auto-approvals and frontmatter hooks register despite the untrusted folder; the subagent gate's 'extension' arm is the same surface. — Failure scenario: victim runs qwen with project root == $HOME and the folder untrusted; the repo commits .qwen/extensions/evil/ with skills/s/SKILL.md declaring allowedTools: ['Bash(*)'] and hooks → auto-active → level 'extension' → gate skipped → session-wide auto-approval of attacker-chosen tool patterns and hook registration without folder trust.
Witness: traced at HEAD ccf1a7a — skill-manager.ts extension collection feeds getActiveExtensions() entries with level: 'extension' and no homeRootShadow tag (the tagging block conditions on level === 'user' only); extension-store activation resolves effective 'enabled' with no store policy; isTrustedSkillLevel('extension') === true opens both gates (skill.ts:385-388, SkillCommandLoader.ts:164-168). Two independent audit traces this round reproduce the original thread's chain. (not run — E2E activation probe not re-executed this round; mechanism verified by trace.)
Suggested fix: extend the home-root shadow to extension-level entries — tag homeRootShadow on extension-sourced skills/agents during collection when the extensions directory lies inside the project root (inside the extension branch of listSkillsAtLevel, which returns before the current tagging block), and broaden the gate disjuncts to re-gate homeRootShadow === true regardless of level in skill.ts, SkillCommandLoader.ts, and the subagent gate.
中文说明
[Critical] R17-2：R14-2（线程位于 packages/core/src/tools/skill.test.ts:686）在本 head 复查仍然存在——本轮审计独立地两次重新发现。'extension' 级豁免在 home-root 拓扑下违反了门禁自身的 fail-closed 前提：当项目根就是 $HOME 时，~/.qwen/extensions == <repo>/.qwen/extensions 是仓库可控的；仓库提交的扩展会被默认缓存刷新发现，并在零用户同意下自动激活（无 store 策略时 ExtensionStore 铸造 defaultActivation: 'enabled'），其 skill/agent 以 'extension' 级出现，而 isTrustedSkillLevel 对该级无条件跳过文件夹信任门禁——homeRootShadow 只对 level === 'user' 打标，因此同一拓扑在 'user' 级被加固、在这里却敞开。仓库提供的 frontmatter allowedTools 成为全会话自动批准、frontmatter hooks 在不受信文件夹中照常注册；subagent 门禁的 'extension' 分支是同一表面。——失败场景：受害者以 $HOME 为项目根运行 qwen、文件夹不受信；仓库提交带 skills/s/SKILL.md（声明 allowedTools: ['Bash(*)'] 与 hooks）的 .qwen/extensions/evil/ → 自动激活 → 'extension' 级 → 门禁跳过 → 攻击者选定的工具模式获得会话级自动批准、hooks 无信任注册。
见证：已在 HEAD ccf1a7a 逐步追踪——skill-manager.ts 的扩展收集把 getActiveExtensions() 条目以 level: 'extension' 送入且无 homeRootShadow 标记（打标块仅以 level === 'user' 为条件）；extension-store 在无 store 策略时解析为 effective 'enabled'；isTrustedSkillLevel('extension') === true 打开两处门禁（skill.ts:385-388、SkillCommandLoader.ts:164-168）。本轮两条独立审计追踪复现了原线程的链路。（未运行——本轮未重跑端到端激活探针；机制经代码追踪核实。）
修复建议：把 home-root 标记扩展到 extension 级条目——在收集阶段，当扩展目录位于项目根内部时对 extension 来源的 skill/agent 打 homeRootShadow 标记（在 listSkillsAtLevel 的 extension 分支内——该分支在当前打标块之前就返回），并把三处门禁析取项放宽为与级别无关的 homeRootShadow === true 重门禁（skill.ts、SkillCommandLoader.ts、subagent 门禁）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| merged.systemMessage = merged.systemMessage | ||
| ? [merged.systemMessage, output.systemMessage] |
There was a problem hiding this comment.
[Critical] Re-asserts @wenshao's open blocker (comment 3779654313 at :449) — verified still standing at this head; the file has not been modified since. suppressOutput is not preserved per producing message: the new concatenation appends every systemMessage unconditionally, while mergeWithOrLogic independently keeps the LAST defined suppression value (hookAggregator.ts:202-203). — Failure scenario: hook A returns { systemMessage: 'private diagnostic', suppressOutput: true }, hook B returns { systemMessage: 'visible', suppressOutput: false } → the aggregate carries BOTH messages with suppression disabled → processCommonHookOutputFields emits the private diagnostic that hook A explicitly suppressed.
Witness: re-read at HEAD ccf1a7a — appendSystemMessage (hookAggregator.ts:447-455) appends output.systemMessage with no suppressOutput consultation; suppression is last-wins at :202-203; comment-status confirms the file is unchanged since the blocker was filed.
Suggested fix: filter suppressed messages before concatenation, or retain per-message suppression metadata until emission; add a mixed suppressed/visible regression test.
中文说明
[Critical] 重申 @wenshao 的未关闭 blocker（comment 3779654313，位于 :449）——已在本 head 核实仍然存在；该文件自评论以来未被修改。suppressOutput 未按消息来源分别保留：新的拼接会无条件加入每个 systemMessage，而 mergeWithOrLogic 独立采用最后一个已定义的 suppression 值（hookAggregator.ts:202-203）。——失败场景：hook A 返回 { systemMessage: 'private diagnostic', suppressOutput: true }，hook B 返回 { systemMessage: 'visible', suppressOutput: false } → 聚合结果包含两条消息且 suppression 被关闭 → processCommonHookOutputFields 输出了 hook A 明确抑制的私有诊断。
见证：已在 HEAD ccf1a7a 重读——appendSystemMessage（hookAggregator.ts:447-455）追加 output.systemMessage 时不查询 suppressOutput；suppression 在 :202-203 为 last-wins；comment-status 确认该文件自 blocker 提出后未变更。
修复建议：在拼接前过滤被抑制的消息，或保留逐消息元数据直到输出阶段；增加“抑制消息 + 可见消息”的回归测试。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| private appendSystemMessage(merged: HookOutput, output: HookOutput): void { | ||
| if (output.systemMessage !== undefined) { |
There was a problem hiding this comment.
[Critical] Re-asserts @wenshao's open blocker (comment 3779654318 at :452) — verified still standing at this head; the file has not been modified since. No final aggregate size limit: command and HTTP hook runners cap individual outputs (truncateOutput), but this concatenation joins an unbounded number of individually valid messages. — Failure scenario: a session with many hooks on one event, each returning a near-cap (~10KB) systemMessage → the merged systemMessage grows without bound (N × 10KB) and is logged and emitted to terminal/ACP consumers as one payload — unbounded memory and render cost from user/repo-configurable hooks.
Witness: re-read at HEAD ccf1a7a — appendSystemMessage joins with no size bound; no cap exists between aggregation and the consumers (processCommonHookOutputFields is a string pass-through); file unchanged since the blocker was filed.
Suggested fix: cap while appending (track remaining budget in appendSystemMessage) or truncate the final aggregate deterministically; cover a many-hook case in hookAggregator.test.ts.
中文说明
[Critical] 重申 @wenshao 的未关闭 blocker（comment 3779654318，位于 :452）——已在本 head 核实仍然存在；该文件自评论以来未被修改。最终聚合结果无总长度上限：命令和 HTTP hook runner 只限制单个输出（truncateOutput），而此拼接会连接任意数量的合法消息。——失败场景：一个事件上挂大量 hooks、每个返回接近上限（约 10KB）的 systemMessage → 合并后的 systemMessage 无上界增长（N × 10KB），并作为一个 payload 写入日志、发送到终端/ACP 消费端——用户/仓库可配置的 hooks 带来无上界的内存与渲染开销。
见证：已在 HEAD ccf1a7a 重读——appendSystemMessage 拼接时无尺寸限制；聚合与消费端之间无任何上限（processCommonHookOutputFields 是字符串透传）；文件自 blocker 提出后未变更。
修复建议：在追加时限制剩余容量（在 appendSystemMessage 中跟踪剩余配额），或对最终结果做确定性截断；在 hookAggregator.test.ts 增加多 hook 场景测试。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| if (response.status >= 300 && response.status < 400) { | ||
| // A redirect delivers no payload, so it must not consume a |
There was a problem hiding this comment.
[Critical] Re-asserts @wenshao's open blocker (comment 3779654320 at :267) — verified still standing at this head; the branch is unchanged. The redirect branch uses the full 300-399 band, so 304 Not Modified is classified as a redirect: it gets the redirect warning and deletes a once hook's execution marker — the supposedly one-shot hook then sends another request on every matching event. — Failure scenario: a once: true HTTP hook whose endpoint answers 304 (caching proxies / conditional requests) → the once slot is deleted on every event → the hook re-fires on every matching event for the life of the runner, with the redirect warning treatment for a response that redirected nothing.
Witness: re-read at HEAD ccf1a7a — if (response.status >= 300 && response.status < 400) at httpHookRunner.ts:265 with executedOnceHooks.delete(onceKey) at :270 inside the branch; branch unchanged since the blocker was filed.
| if (response.status >= 300 && response.status < 400) { | |
| // A redirect delivers no payload, so it must not consume a | |
| if ([301, 302, 303, 307, 308].includes(response.status)) { | |
| // A redirect delivers no payload, so it must not consume a |
Let other non-2xx 3xx statuses (300/304/…) use the generic error path, and add a 304 regression test.
中文说明
[Critical] 重申 @wenshao 的未关闭 blocker（comment 3779654320，位于 :267）——已在本 head 核实仍然存在；该分支未变更。重定向分支使用完整的 300-399 区间，因此 304 Not Modified 被归类为重定向：它会收到重定向告警并删除 once hook 的执行标记——本应一次性的 hook 之后在每个匹配事件上都会再次发送请求。——失败场景：once: true 的 HTTP hook 的端点应答 304（缓存代理/条件请求）→ once 槽位每个事件都被删除 → 该 hook 在 runner 生命周期内每个匹配事件都重新触发，且对一个并未重定向的响应按重定向告警处理。
见证：已在 HEAD ccf1a7a 重读——httpHookRunner.ts:265 为 if (response.status >= 300 && response.status < 400)，分支内部 :270 为 executedOnceHooks.delete(onceKey)；分支自 blocker 提出后未变更。
修复建议（见上方 suggestion）：把重定向分支限定为实际的 Fetch 重定向状态码（301、302、303、307、308），让其他非 2xx 的 3xx 走通用错误路径，并增加 304 回归测试。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| if (specifier === '@qwen-code/qwen-code-core/memoryScopes') { | ||
| return { | ||
| shortCircuit: true, | ||
| url: memoryScopesSourceUrl, | ||
| format: 'module', | ||
| }; |
There was a problem hiding this comment.
[Suggestion] R16-3: The remap set this hunk extends is still missing the toolWriteOrigin core subpath (re-checked this round; still stands) — the comment's own invariant ('each one cli source imports needs its own remap to the source file') remains violated. Cli source imports @qwen-code/qwen-code-core/toolWriteOrigin in exactly two files (serve/bridge-file-system-adapter.ts:62 → run-qwen-serve.ts:75, and acp-integration/service/filesystem.ts:25); core's exports map resolves it to dist/, which is git-ignored and absent on a fresh checkout. — Concrete cost: on a fresh checkout, npm run dev:daemon / npm run dev -- serve / ACP dev mode die with ERR_MODULE_NOT_FOUND one specifier past memoryScopes; with a stale dist/ the import silently loads built code instead of source — the exact source/dist skew the remap mechanism exists to prevent.
Witness (probe this round): a faithful copy of dev.js's post-PR loader resolves toolWriteOrigin to .../core/dist/src/services/tool-write-origin.js → ERR_MODULE_NOT_FOUND on a fresh-checkout model; adding the remap → LOADED OK. Mechanical sweep: cli source imports exactly 3 core subpaths (envVarResolver ×3, toolWriteOrigin ×2, memoryScopes ×1) — 1 of 3 lacks the remap.
const toolWriteOriginSourceUrl = pathToFileURL(
join(root, 'packages', 'core', 'src', 'services', 'tool-write-origin.ts'),
).href;
// and in the loader, beside the block above:
if (specifier === '@qwen-code/qwen-code-core/toolWriteOrigin') {
return { shortCircuit: true, url: toolWriteOriginSourceUrl, format: 'module' };
}Note: the transcriptRecords/goalWire subpaths consumed through acp-bridge dist (open thread R14-5) remain unresolved by this fix.
中文说明
[Suggestion] R16-3：本 hunk 扩展的 remap 集合仍然缺少 toolWriteOrigin core 子路径（本轮复查确认仍然存在）——注释自带的不变量（“cli source 引入的每一个子路径都需要一个指向源文件的 remap”）依旧被违反。cli source 在两个文件中引入 @qwen-code/qwen-code-core/toolWriteOrigin（serve/bridge-file-system-adapter.ts:62 → run-qwen-serve.ts:75，以及 acp-integration/service/filesystem.ts:25）；core 的 exports map 把它解析到 git-ignored、fresh checkout 上不存在的 dist/。——具体代价：fresh checkout 上 npm run dev:daemon / npm run dev -- serve / ACP dev 模式会在 memoryScopes 之后一个 specifier 处以 ERR_MODULE_NOT_FOUND 失败；带着过期 dist/ 时则悄悄加载构建产物而非源码——正是 remap 机制要避免的 source/dist 偏差。
见证（本轮探针）：dev.js 改动后 loader 的忠实副本把 toolWriteOrigin 解析到 .../core/dist/src/services/tool-write-origin.js → fresh-checkout 模型下 ERR_MODULE_NOT_FOUND；加上 remap 后 → LOADED OK。机械扫描：cli source 恰好引入 3 个 core 子路径（envVarResolver ×3、toolWriteOrigin ×2、memoryScopes ×1）——3 缺 1。
修复：见上方代码块（新增 toolWriteOriginSourceUrl 常量与 loader 分支）。注意：经 acp-bridge dist 消费的 transcriptRecords/goalWire 子路径（未关闭线程 R14-5）此修复不覆盖。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // literal would turn a secret *name* into a public, guessable value. | ||
| if (isInternalSecretEnvVar(envName)) { |
There was a problem hiding this comment.
[Suggestion] R17-7: No test pins the $$-escape-vs-denylist ordering. The new denylist checks in resolveEnvVars and resolveConfigEnvVar are deliberately placed AFTER the if (value.startsWith('$$')) return value.substring(1) escape (resolveWebhookSecretEnv takes a bare name — no escape, fail-closed), but the only escape test uses a non-secret name ('$$literal-token') and all new denylist tests use single-$ references. — Concrete cost: a future refactor consolidating the three duplicated denylist blocks can hoist the check above the escape with every existing test still green; after that, a channel config holding a literal value spelled $$QWEN_SERVER_TOKEN — a previously valid, explicitly-literal configuration — aborts at config-parse time instead of resolving to the literal $QWEN_SERVER_TOKEN.
Witness (probe this round): resolveEnvVars('$$QWEN_SERVER_TOKEN') returns '$QWEN_SERVER_TOKEN' at HEAD. Flip: hoisting a denylist check above the escape makes the probe FAIL while all 54 existing config-utils tests stay green (1 failed | 54 passed under mutation) — the unguarded fragility, demonstrated; mutation reverted, tree clean.
// in the new internal-secret denylist describe:
expect(resolveEnvVars('$$QWEN_SERVER_TOKEN')).toBe('$QWEN_SERVER_TOKEN');
// plus the parseChannelConfig equivalent through a credential field中文说明
[Suggestion] R17-7：没有测试钉住 $$ 转义与内部 secret 拒绝名单的顺序。resolveEnvVars 与 resolveConfigEnvVar 中新增的拒绝检查刻意放在 if (value.startsWith('$$')) return value.substring(1) 转义之后（resolveWebhookSecretEnv 接收裸变量名——无转义、fail-closed），但唯一的转义测试用的是非 secret 名（'$$literal-token'），所有新的拒绝名单测试都用单 $ 引用。——具体代价：未来把三处重复拒绝块合并的重构可以把检查提升到转义之前而所有现有测试仍绿；此后，字面值写作 $$QWEN_SERVER_TOKEN 的 channel 配置——此前合法的显式字面量配置——会在配置解析时中止，而不是解析为字面量 $QWEN_SERVER_TOKEN。
见证（本轮探针）：HEAD 上 resolveEnvVars('$$QWEN_SERVER_TOKEN') 返回 '$QWEN_SERVER_TOKEN'。翻转：把拒绝检查提升到转义之前会使探针失败，而现有 54 个 config-utils 测试全部仍绿（变异下 1 failed | 54 passed）——无防护的脆弱性已被演示；变异已还原，工作区干净。
修复：在新的 internal-secret denylist describe 中增加 expect(resolveEnvVars('$$QWEN_SERVER_TOKEN')).toBe('$QWEN_SERVER_TOKEN')（以及通过凭据字段的 parseChannelConfig 等价用例），钉住转义在两处都先于拒绝名单短路。
— qwen3.8-max via Qwen Code /review (v0.21.12)
qwen-code-dev-bot
added
the
autofix/needs-human
|
🔓 Takeover auto-released: the autofix loop paused on this PR 9 day(s) ago (🤖 AutoFix stopped: this counting window now contains 3 time-budget exhaustions (pushed rounds in between included; this ) and no re-arm followed, so the 中文说明🔓 已自动释放接管：autofix 循环在 9 天前暂停于此 PR（🤖 AutoFix stopped: this counting window now contains 3 time-budget exhaustions (pushed rounds in between included; this ），此后无人重新武装，现移除 |
qwen-code-dev-bot
removed
the
autofix/takeover
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R15-3 all-non-string-array coerced to silent allow-all without warning (config.ts:2348) — already reported (comment 3728764862)
- R13-8 allowedTools trust-gating describe omits the homeRootShadow/worktree topology cases its sibling hooks describe pins (skill.test.ts) — already reported (comments 3722849940, 3725050104)
- R13-2 text-capture.tsx envVarResolver remap unreachable by any test command — already reported (comments 3722849891, 3725050100)
Unresolved, please confirm:
- [Critical] R16-2 one-directional gate liveness (packages/core/src/tools/skill.ts:609 thread): the trusted→untrusted revocation half is factually absent at this head (re-verified this round: unregisterSkillHooks is a no-op stub with zero call sites, cl…
Not reviewed: reverse audit — reached the 5-round cap without two consecutive dry rounds (rounds 3, 4 and 5 each reported findings; the loop ended at the cap, not by convergence).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the case-insensitive env-denylist paths this PR adds are only exercisable on Windows.
Not explored to full depth (tool budget reached): chunk 6: execute packages/core vitest for envInterpolator.test.ts and hookAggregator.test.ts — node_modules is not installed in the review worktree or parent check….
Test Plan (not a blocker): 1385 tests green — this review observed 21293, 20381, 494, 1541, 1597, 3723, 529 passed.
Deferred under the convergence posture (round 18, not a blocker) — recorded, not requested in this round:
scripts/dev.js:110 — [probe] The dev.js subpath remap set is still missing…packages/cli/src/commands/channel/config-utils.ts:38 — [review] No test pins the $$-escape-vs-denylist orderingpackages/core/src/hooks/httpHookRunner.ts:269 — [probe] Once-slot re-arm fires only for 3xx; connection…scripts/dev.js:103 — [probe] Nothing pins the dev.js subpath remap table to cli's…packages/core/src/hooks/httpHookRunner.ts:270 — [probe] Unconditional 3xx re-arm turns a once hook behind a…packages/core/src/skills/skill-manager.ts:1068 — [probe] homeRootShadow over-tags user-level skills loaded from…packages/core/src/skills/skill-manager.test.ts:783 — [probe] New homeRootShadow tests fail whenever QWEN_HOME is set in…packages/core/src/hooks/httpHookRunner.ts:287 — [probe] Redirect-warning systemMessage exceeds the 10,000-char cap…
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未决，请确认：共 1 条（原文未翻译，列表见上方英文部分）。
未审查：reverse audit — reached the 5-round cap without two consecutive dry rounds (rounds 3, 4 and 5 each reported findings; the loop ended at the cap, not by convergence)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the case-insensitive env-denylist paths this PR adds are only exercisable on Windows。
未探索到全部深度（达到工具调用预算）：chunk 6：execute packages/core vitest for envInterpolator.test.ts and hookAggregator.test.ts — node_modules is not installed in the review worktree or parent check…。
Test Plan（非阻断）：1385 tests green — this review observed 21293, 20381, 494, 1541, 1597, 3723, 529 passed。
收敛姿态下延后（第 18 轮，非阻断）——已记录，本轮不要求修改：共 8 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (narrowed.length > 0) { | ||
| restSecurity.allowedHttpHookUrls = narrowed; | ||
| } else { | ||
| delete restSecurity.allowedHttpHookUrls; | ||
| } |
There was a problem hiding this comment.
[Critical] R16-1: The workspace-whitelist narrowing added here breaks a pre-existing, unmodified test — the packages/cli suite is red at this head (re-measured this round; still stands). narrowWorkspaceHookSecurityOverrides deliberately discards a workspace-scoped security.allowedHttpHookUrls when no higher-scope whitelist exists, but the pre-existing test allowedInsecureVoiceBaseUrls scope handling > should strip and warn about the allowlist from workspace scope (settings.test.ts:3776, assertion at :3797) still asserts the old pass-through semantics. The sibling stale test was updated; this one was missed. — Failure scenario: workspace settings define allowedHttpHookUrls: ['https://hooks.example.com/*'] with no User/System/SystemDefaults whitelist → higherUrls === undefined → narrowed = [] → delete → merged is undefined → AssertionError: expected undefined to deeply equal ['https://hooks.example.com/*'] → npm test --workspace=packages/cli and CI fail.
Witness (this round): name-level junit A/B — the test fails in isolation on the PR head (AssertionError: expected undefined to deeply equal [ 'https://hooks.example.com/*' ]) and passes at merge base 259951c5 in the identical environment; it is the only net-new failure of 97 (the other 96 fail identically on base — environmental QWEN_HOME/home-path mismatch, gone under env -u QWEN_HOME). The discard is documented, intended behavior; the stale test is the overlooked artifact. (Raised at Critical over the file-level test-delta hold, which is confounded by that environmental cluster; both sides quoted above.)
| if (narrowed.length > 0) { | |
| restSecurity.allowedHttpHookUrls = narrowed; | |
| } else { | |
| delete restSecurity.allowedHttpHookUrls; | |
| } | |
| expect(settings.merged.security?.allowedHttpHookUrls).toBeUndefined(); |
中文说明
[Critical] R16-1：此处新增的 workspace 白名单收窄逻辑破坏了一个既有且未被本 PR 修改的测试——packages/cli 测试套件在当前 head 上是红的（本轮重新实测，仍然存在）。narrowWorkspaceHookSecurityOverrides 在更高 scope 未设置白名单时会刻意丢弃 workspace scope 的 security.allowedHttpHookUrls，但既有测试 allowedInsecureVoiceBaseUrls scope handling > should strip and warn about the allowlist from workspace scope（settings.test.ts:3776，断言位于 :3797）仍在断言旧的直通合并语义。同类的另一个过时测试已更新，这一个被遗漏了。——失败场景：workspace 设置定义 allowedHttpHookUrls: ['https://hooks.example.com/*'] 且 User/System/SystemDefaults 均未设置白名单 → higherUrls === undefined → narrowed = [] → delete → 合并结果为 undefined → 断言失败 → npm test --workspace=packages/cli 与 CI 失败。
见证（本轮）：测试名级 junit A/B——该测试在 PR 分支上单独运行即失败（AssertionError: expected undefined to deeply equal [ 'https://hooks.example.com/*' ]），在合并基 259951c5 的相同环境下通过；是 97 个失败中唯一净新增的（其余 96 个在 base 上同样失败——属环境性 QWEN_HOME/home 路径不匹配，env -u QWEN_HOME 后消失）。丢弃行为本身是有文档记载的预期行为，被遗漏的是这个过时的测试。（file 级 test-delta hold 被该环境性失败簇混淆，故按测试名级双侧证据恢复为 Critical。）
修复：把 settings.test.ts:3797 的断言更新为新语义（见上方 suggestion）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| export function hookUrlPatternCovers( | ||
| outerPattern: string, | ||
| innerPattern: string, | ||
| ): boolean { |
There was a problem hiding this comment.
[Critical] R17-1: The whitelist boundary is enforced by regex-testing the RAW URL string — both in validate()/compilePattern and in this new hookUrlPatternCovers gate — but neither models WHATWG URL parsing, so URL-structure tokens bypass it. Probe-verified in round 17, three token families (with wildcard-host patterns such as https://*.corp.com/*): (1) userinfo — https://x@evil.com/.corp.com/payload validates allowed: true while new URL(...).hostname is evil.com, where fetch() sends the payload; hookUrlPatternCovers also certifies such entries as legitimate narrowings. (2) fragment / newline — a workspace entry https://evil.com#.corp.com/* is certified as covered (# is not in regexActive), survives narrowWorkspaceHookSecurityOverrides and REPLACES the higher-scope list, matches at runtime, and fetch strips the fragment → payload POSTs to evil.com; the \n axis behaves the same (WHATWG strips \n\r\t). (3) authority/path boundary — https://evil.com/.corp.com/payload and https://evil.com\.corp.com/x are both isAllowed: true with parsed hostname evil.com (backslash folds to / mid-authority), and covers() certifies https://evil.com/.corp.com/* under https://*.corp.com/*. This defeats the guarantee this PR writes into settings.ts, the schema, and the docs — that a workspace may only NARROW where hook payloads may be sent — and subsumes prior-round R14-1. Re-checked this round: the branch code is unchanged since the round-17 probes (only a main-merge commit), and no @/#/control-char guard exists at HEAD. — Failure scenario: user/system whitelist ['https://*.corp.com/*']; a trusted-but-malicious repo adds the workspace narrowing entry ['https://evil.com#.corp.com/*'] plus an HTTP hook targeting it → the entry survives narrowing, becomes the effective whitelist, and the hook payload (prompts, tool inputs/outputs) POSTs to evil.com.
Witness: round-17 probes (branch unchanged since): userinfo — validate('https://x@evil.com/.corp.com/payload').allowed === true vs hostname evil.com (flip: rejecting URL.username + @ in covers → 5/5 cases pass, all 41 existing tests green); fragment — covers=true, entry survives narrowing, runtime allows, hostname evil.com; authority boundary — both URLs isAllowed: true, hostname evil.com; E2E with a real HttpHookRunner + local HTTP server: payload received at /.corp.com/exfil; with a parse-based origin guard: validate: false, 0 requests received.
Suggested fix: close the class structurally — parse the hook URL once with new URL() and enforce the pattern against parsed components (reject non-empty username, fragments and C0 control chars; compare hostname/path against the pattern's authority/path split), and lift hookUrlPatternCovers's contract to the same fetch-destination semantics (fail closed on @, #, and control characters in either pattern). Note: normalizing input to its serialized href does NOT close / and \ — the serialized href still textually matches.
中文说明
[Critical] R17-1：白名单边界靠对原始 URL 字符串做正则测试来执行——validate()/compilePattern 如此，本 PR 新增的 hookUrlPatternCovers 门禁亦如此——但两者都没有建模 WHATWG URL 解析，因此 URL 结构 token 可以绕过它。已在第 17 轮用探针验证三类 token（针对 https://*.corp.com/* 这类通配主机模式）：(1) userinfo——https://x@evil.com/.corp.com/payload 校验结果为 allowed: true，而 new URL(...).hostname 是 evil.com，fetch() 实际把负载发往该主机；hookUrlPatternCovers 还会把这类条目认证为合法收窄。(2) fragment / 换行——workspace 条目 https://evil.com#.corp.com/* 被认证为被覆盖（# 不在 regexActive 中），在收窄中存活并替换更高 scope 列表，运行时匹配通过，fetch 剥掉 fragment → 负载 POST 到 evil.com；\n 轴同理（WHATWG 会剥掉 \n\r\t）。(3) authority/path 边界——https://evil.com/.corp.com/payload 与 https://evil.com\.corp.com/x 均 isAllowed: true，解析主机名为 evil.com（反斜斜杠在 authority 中段被折叠为 /），且 covers() 把 https://evil.com/.corp.com/* 认证在 https://*.corp.com/* 之下。这击穿了本 PR 写入 settings.ts、schema 与文档的保证——workspace 只能收窄 hook 负载的去向——并涵盖上一轮的 R14-1。本轮复查：分支代码自第 17 轮探针以来未变（仅合并 main），HEAD 上仍无 @/#/控制字符守卫。——失败场景：user/system 白名单 ['https://*.corp.com/*']；受信但恶意的仓库添加 workspace 收窄条目 ['https://evil.com#.corp.com/*'] 及指向它的 HTTP hook → 条目存活收窄、成为生效白名单，hook 负载（prompt、工具输入/输出）POST 到 evil.com。
见证：第 17 轮探针（分支此后未变）：userinfo——validate('https://x@evil.com/.corp.com/payload').allowed === true 而 hostname 为 evil.com（翻转：拒绝 URL.username 并把 @ 加入 covers 的 fail-closed 集 → 5/5 用例通过，现有 41 个测试全绿）；fragment——covers=true、条目存活收窄、运行时放行、hostname evil.com；authority 边界——两个 URL 均 isAllowed: true、hostname evil.com；真实 HttpHookRunner + 本地 HTTP server 的端到端：负载到达 /.corp.com/exfil；加上基于解析的 origin 守卫后 validate: false、0 请求到达。
修复建议：以结构化方式关闭整类问题——用 new URL() 解析一次 hook URL，对解析后的分量执行模式匹配（拒绝非空 username、fragment 与 C0 控制字符；按模式的 authority/path 切分比较 hostname/path），并把 hookUrlPatternCovers 的契约提升到同样的 fetch 目的地语义（任一模式含 @、#、控制字符时 fail closed）。注意：把输入归一化为序列化 href 不能关闭 / 与 \——序列化 href 在文本上仍然匹配。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (output.suppressOutput !== undefined) { | ||
| merged.suppressOutput = output.suppressOutput; | ||
| } | ||
| if (output.systemMessage !== undefined) { | ||
| merged.systemMessage = output.systemMessage; | ||
| } | ||
| this.appendSystemMessage(merged, output); |
There was a problem hiding this comment.
[Critical] R17-3: Re-asserts @wenshao's open blocker (comment 3779654313 at :449) — verified still standing at this head; the file has not been modified since. suppressOutput is not preserved per producing message: the new concatenation appends every systemMessage unconditionally (appendSystemMessage, :447-455), while mergeWithOrLogic independently keeps the LAST defined suppression value (these lines). For example, { systemMessage: 'private diagnostic', suppressOutput: true } followed by { systemMessage: 'visible', suppressOutput: false } aggregates to both messages with suppression disabled; processCommonHookOutputFields then emits the private diagnostic too. — Failure scenario: two hooks on one event, the first returning a suppressed diagnostic, the second a visible message with suppressOutput: false → the merged output carries both messages with suppression off → content the producing hook explicitly suppressed reaches the user/ACP consumers.
Witness: code read at HEAD ad00d83 — if (output.suppressOutput !== undefined) { merged.suppressOutput = output.suppressOutput; } (last-defined wins) immediately followed by this.appendSystemMessage(merged, output) appending every message regardless of suppression.
Suggested fix: filter suppressed messages before concatenation (skip the append when output.suppressOutput === true), or retain per-message metadata until emission; add a mixed suppressed/visible regression test.
中文说明
[Critical] R17-3：重申 @wenshao 的未决 blocker（comment 3779654313，位于 :449）——已在本 head 核实仍然存在；该文件自那以后未修改。suppressOutput 未按消息来源分别保留：新的拼接逻辑无条件加入每个 systemMessage（appendSystemMessage，:447-455），而 mergeWithOrLogic 独立采用最后一个已定义的 suppression 值（即这几行）。例如先返回 { systemMessage: 'private diagnostic', suppressOutput: true }，再返回 { systemMessage: 'visible', suppressOutput: false }，聚合结果会包含两条消息且 suppression 被关闭，processCommonHookOutputFields 最终连私有诊断一并输出。——失败场景：同一事件上两个 hooks，第一个返回被抑制的诊断、第二个返回 suppressOutput: false 的可见消息 → 聚合输出携带两条消息且抑制关闭 → 产生方明确抑制的内容到达用户/ACP 消费端。
见证：HEAD ad00d83 代码阅读——last-wins 的 suppressOutput 赋值之后紧跟无条件的 appendSystemMessage。
修复建议：拼接前过滤被抑制的消息（output.suppressOutput === true 时跳过追加），或保留逐消息元数据直到输出阶段；增加“抑制 + 可见”混合回归测试。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| * Append an output's systemMessage to merged, concatenating so a | ||
| * one-shot message from an earlier hook survives later outputs. | ||
| */ | ||
| private appendSystemMessage(merged: HookOutput, output: HookOutput): void { |
There was a problem hiding this comment.
[Critical] R17-4: Re-asserts @wenshao's open blocker (comment 3779654318 at :452) — verified still standing at this head; the file has not been modified since. No final aggregate size limit: command and HTTP hook runners cap individual outputs (truncateOutput), but this concatenation joins an unbounded number of individually valid messages; a large hook set can therefore create a multi-megabyte systemMessage that is logged and emitted to terminal/ACP consumers as one payload. — Failure scenario: a session with many hooks on one event, each returning a near-cap (~10 KB) systemMessage → the merged systemMessage grows without bound → multi-MB payload logged and rendered/emitted in one piece.
Witness: code read at HEAD ad00d83 — appendSystemMessage joins [merged.systemMessage, output.systemMessage] with no length check anywhere in the three merge paths; truncateOutput is applied by individual producers only.
Suggested fix: cap while appending or truncate the final aggregate deterministically (apply truncateOutput to the merged systemMessage after the loop), and cover a many-hook case.
中文说明
[Critical] R17-4：重申 @wenshao 的未决 blocker（comment 3779654318，位于 :452）——已在本 head 核实仍然存在；该文件自那以后未修改。最终聚合没有总长度上限：命令与 HTTP hook runner 只限制单个输出（truncateOutput），而此处拼接可以连接任意数量的合法消息；大量 hooks 因此能生成数 MB 的 systemMessage，并作为单个 payload 写入日志、发送到终端/ACP 消费端。——失败场景：一个事件上挂多个 hooks，每个都返回接近上限（约 10 KB）的 systemMessage → 合并后的 systemMessage 无限增长 → 数 MB 负载被整体记录/渲染/发出。
见证：HEAD ad00d83 代码阅读——appendSystemMessage 在三条合并路径中均无长度检查地拼接；truncateOutput 仅由各个产生方单独应用。
修复建议：在追加时限制剩余容量，或对最终结果做确定性截断（循环结束后对合并值应用 truncateOutput），并增加多 hook 场景测试。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // Per Qwen Code spec: Non-2xx status is a non-blocking error | ||
| // Execution continues, but we log a warning | ||
| if (!response.ok) { | ||
| if (response.status >= 300 && response.status < 400) { |
There was a problem hiding this comment.
[Critical] R17-5: Re-asserts @wenshao's open blocker (comment 3779654320 at :267) — verified still standing at this head; the branch is unchanged. The redirect branch uses the full 300-399 band, so 304 Not Modified is classified as a redirect: it gets the redirect warning and deletes a once hook's execution marker — the supposedly one-shot hook then sends another request on every matching event. — Failure scenario: an endpoint returns 304 Not Modified → the branch deletes the once hook's executedOnceHooks slot → the one-shot hook re-fires on every matching event instead of running exactly once.
Witness: code read at HEAD ad00d83 — if (response.status >= 300 && response.status < 400) (the full band, no 304 exclusion); the slot delete at :269-271 runs for every status in it.
Suggested fix: restrict this branch to actual Fetch redirect statuses (301, 302, 303, 307, 308); let other non-2xx 3xx statuses use the generic error path, and add a 304 regression test.
中文说明
[Critical] R17-5：重申 @wenshao 的未决 blocker（comment 3779654320，位于 :267）——已在本 head 核实仍然存在；该分支未变。重定向分支使用完整的 300-399 区间，因此 304 Not Modified 也被当作重定向：输出重定向警告并删除 once hook 的执行标记——本应只执行一次的 hook 会在后续每个匹配事件中再次发送请求。——失败场景：端点返回 304 Not Modified → 分支删除该 once hook 的 executedOnceHooks 槽位 → 一次性 hook 在每个匹配事件上重复触发，而不是恰好执行一次。
见证：HEAD ad00d83 代码阅读——if (response.status >= 300 && response.status < 400)（完整区间，未排除 304）；:269-271 的槽位删除对该区间内所有状态码执行。
修复建议：把该分支限制为 Fetch 实际识别的重定向状态码（301、302、303、307、308），其他非 2xx 的 3xx 状态走通用错误路径，并增加 304 回归测试。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| const sideEffectsGated = | ||
| (!isTrustedSkillLevel(skill.level) || | ||
| (skill.level === 'user' && skill.homeRootShadow === true)) && | ||
| !this.config.isTrustedFolder(); |
There was a problem hiding this comment.
[Critical] R15-6: The 'user'-level gate exemption is bypassed when QWEN_HOME (or a relative settings.skills.directories entry) resolves inside the project root — re-checked at this head and STILL STANDS (probe-verified this round; previously reported in round 15, comment 3728764835, then dropped from the round-17 ledger). The gates exempt 'user'-level skills on the premise that they live in ~/.qwen, but Storage.getGlobalQwenDir() honors QWEN_HOME (storage.ts:183-194; relative values resolve against cwd) and custom skill dirs are resolved against the working directory (skill-manager.ts:930-943), both surfacing at 'user' level. homeRootShadow is only tagged when path.resolve(projectRoot) === path.resolve(os.homedir()) (skill-manager.ts:1062-1071), which that geometry falsifies — so repo-controlled skills surface at 'user' level UNTAGGED and skip this gate and the SkillCommandLoader gate. The repo cannot inject skills.directories itself (workspace settings are dropped while untrusted), but a victim-side QWEN_HOME needs no config injection at all — direnv/devcontainer conventions set it, and this repo's own automation uses a qwen-home dir pattern. The mirrored gate in packages/cli/src/services/SkillCommandLoader.ts:163-169 has the same shape. — Failure scenario: QWEN_HOME=<repo>/.qwen-home → $QWEN_HOME/skills/evil/SKILL.md with frontmatter hooks + allowedTools: ['Bash(curl *)'] lists at 'user' level untagged → both gates exempt → repo-supplied hooks register and allowedTools grant session-wide auto-approvals in an UNTRUSTED folder.
Witness (probe this round, HEAD ad00d83): [PROBE-R15-6] {"level":"user","filePath":".../.qwen-home/skills/evil/SKILL.md","allowedTools":["Bash(curl *)"],"hasHooks":true} with sideEffectsGated = false in an untrusted folder. Flip check: patching the tag to source-dir containment sets homeRootShadow: true and the probe's bypass assertions fail (patch reverted, tree clean).
Suggested fix: tag by provenance — in listSkillsAtLevel('user'), set the shadow flag for any skill whose resolved base dir lies inside path.resolve(projectRoot) (covers both custom-dir entries and a repo-interior QWEN_HOME); mirror in listSubagentsAtLevel (R15-9).
中文说明
[Critical] R15-6：当 QWEN_HOME（或相对的 settings.skills.directories 条目）解析到项目根目录内部时，'user' 级门禁豁免被绕过——在本 head 复查仍然存在（本轮探针验证；第 15 轮已报告，comment 3728764835，随后从第 17 轮 ledger 中丢失）。门豁免 'user' 级 skill 的前提是它们位于 ~/.qwen，但 Storage.getGlobalQwenDir() 会遵循 QWEN_HOME（storage.ts:183-194；相对值按 cwd 解析），自定义 skill 目录也按工作目录解析（skill-manager.ts:930-943），两者都以 'user' 级出现。homeRootShadow 仅在 path.resolve(projectRoot) === path.resolve(os.homedir()) 时打标（skill-manager.ts:1062-1071），而上述几何形状恰好使其为假——于是仓库可控的 skill 以未打标的 'user' 级出现，同时绕过此门禁与 SkillCommandLoader 门禁。仓库自身无法注入 skills.directories（不受信时 workspace 设置被整体丢弃），但受害端的 QWEN_HOME 无需任何配置注入——direnv/devcontainer 约定会设置它，本仓库自己的自动化也使用 qwen-home 目录模式。packages/cli/src/services/SkillCommandLoader.ts:163-169 的镜像门禁同样如此。——失败场景：QWEN_HOME=<repo>/.qwen-home → $QWEN_HOME/skills/evil/SKILL.md（frontmatter 声明 hooks + allowedTools: ['Bash(curl *)']）以未打标的 'user' 级被列出 → 两处门禁均豁免 → 仓库提供的 hooks 在不受信文件夹中注册、allowedTools 获得会话级自动批准。
见证（本轮探针，HEAD ad00d83）：不受信文件夹下 [PROBE-R15-6] {"level":"user","filePath":".../.qwen-home/skills/evil/SKILL.md","allowedTools":["Bash(curl *)"],"hasHooks":true} 且 sideEffectsGated = false。翻转验证：把打标改为按源目录包含关系后 homeRootShadow: true，探针的绕过断言失败（补丁已还原，工作区干净）。
修复建议：按来源打标——在 listSkillsAtLevel('user') 中，对解析后基础目录位于 path.resolve(projectRoot) 内部的 skill 设置 shadow 标记（同时覆盖自定义目录条目与仓库内部的 QWEN_HOME）；并在 listSubagentsAtLevel 做镜像处理（R15-9）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| const trustedAgentLevel = | ||
| (config.level === 'user' && config.homeRootShadow !== true) || | ||
| config.level === 'builtin' || | ||
| config.level === 'extension' || | ||
| config.level === 'session'; |
There was a problem hiding this comment.
[Critical] R15-9: Subagent side of R15-6 (same root cause) — re-checked at this head and STILL STANDS (probe-verified this round; previously reported in round 15, comments 3728764842 / 3773026779, then dropped from the round-17 ledger). The 'user' arm of this allowlist assumes the user agents directory is ~/.qwen, but Storage.getGlobalQwenDir() honors QWEN_HOME (storage.ts:183-193; relative values resolve against cwd) and listSubagentsAtLevel('user') reads getGlobalQwenDir()/agents (:1368-1371). When QWEN_HOME resolves inside the project root (e.g. a direnv/devcontainer convention), repo-controlled agents surface at 'user' level WITHOUT the homeRootShadow tag — the tag is set only when path.resolve(projectRoot) === path.resolve(os.homedir()) (:1402-1406), which that geometry falsifies — and this gate's 'user' arm trusts them unconditionally. — Failure scenario: QWEN_HOME=.qwen-home (relative, inside the repo) → $QWEN_HOME/agents/evil.md with frontmatter hooks lists at 'user' level untagged → trustedAgentLevel === true → repo-supplied agent hooks register in an untrusted folder.
Witness (probe this round, HEAD ad00d83): [PROBE-R15-9] {"level":"user","filePath":".../.qwen-home/agents/evil-agent.md","hasHooks":true} with trustedAgentLevel = true. Flip check: the mirrored containment patch sets homeRootShadow: true and the probe fails (patch reverted, tree clean).
Suggested fix: mirror the R15-6 containment tag in listSubagentsAtLevel: tag agents whose resolved dir lies inside path.resolve(projectRoot).
中文说明
[Critical] R15-9：R15-6 的 subagent 侧（同一根因）——在本 head 复查仍然存在（本轮探针验证；第 15 轮已报告，comments 3728764842 / 3773026779，随后从第 17 轮 ledger 中丢失）。此允许清单的 'user' 分支假设用户级 agents 目录是 ~/.qwen，但 Storage.getGlobalQwenDir() 会遵循 QWEN_HOME（storage.ts:183-193；相对值按 cwd 解析），而 listSubagentsAtLevel('user') 读取 getGlobalQwenDir()/agents（:1368-1371）。当 QWEN_HOME 解析到项目根目录内部（例如 direnv/devcontainer 约定）时，仓库可控的 agents 会以 'user' 级别出现且不带 homeRootShadow 标记——该标记只在 path.resolve(projectRoot) === path.resolve(os.homedir()) 时设置（:1402-1406），上述几何形状恰好使其为假——而此门禁的 'user' 分支无条件信任它们。——失败场景：QWEN_HOME=.qwen-home（相对路径，位于仓库内）→ $QWEN_HOME/agents/evil.md（frontmatter 声明 hooks）以未打标的 'user' 级被列出 → trustedAgentLevel === true → 仓库提供的 agent hooks 在不受信文件夹中被注册。
见证（本轮探针，HEAD ad00d83）：[PROBE-R15-9] {"level":"user","filePath":".../.qwen-home/agents/evil-agent.md","hasHooks":true} 且 trustedAgentLevel = true。翻转验证：镜像的包含关系打标补丁使 homeRootShadow: true，探针失败（补丁已还原，工作区干净）。
修复建议：在 listSubagentsAtLevel 镜像 R15-6 的包含关系打标：对解析目录位于 path.resolve(projectRoot) 内部的 agent 打标。
— qwen3.8-max via Qwen Code /review (v0.21.13)
4 participants
What this PR does
This PR closes four independent trust-boundary holes in the hook system, all in the area where repository-controlled configuration meets code execution or network egress:
HTTP hooks no longer follow redirects. Previously the URL whitelist and the DNS-level SSRF checks (private ranges, cloud metadata endpoints) were validated only against the initially configured URL, but the underlying client followed redirects automatically — so any 30x response from a configured or compromised endpoint could re-POST the full hook payload (prompts, tool inputs and outputs, session id) to an arbitrary internal or metadata address with no re-validation. Redirects are now disabled; a 3xx response is treated as a hook failure (non-blocking, same as other HTTP errors).
The HTTP-hook URL whitelist can no longer be set from workspace settings. The settings merge already strips the private-network relaxation flag from workspace scope so a repository cannot self-grant an SSRF bypass; the sibling whitelist key was still honored from workspace scope, letting a repository replace or widen the user's whitelist (e.g. with
*) and exfiltrate hook payloads past the boundary the user configured. Both keys are now stripped from workspace scope; user and system scopes are unaffected.Qwen-internal secrets are never substituted into hook commands, URLs, or headers. An earlier fix stripped the daemon tokens and the private ACP capability from hook child-process environments, but two other paths still resolved them from the process environment: environment-variable references in settings files (resolved at load time, before any child-env sanitization applies) and the HTTP hook feature that interpolates whitelisted environment variables into request URLs and headers. A repository-controlled settings file or hook config could name the daemon bearer token and have it sent over the network. All env-var resolution paths now refuse these variables.
Project-level frontmatter hooks require a trusted folder. Hooks declared in the frontmatter of project subagents (
.qwen/agents/) and project skills (.qwen/skills/) were registered unconditionally when the agent was spawned or the skill invoked — even in folders the user explicitly marked untrusted, where the same hooks declared in the repository's settings file would have been blocked. Both registration paths now skip project-level hooks with a warning in untrusted folders; user-level subagents and skills are unaffected, matching the existing user-hook trust semantics.Why it's needed
These were found during a systematic security audit of the hooks module, and each was reproduced end-to-end before fixing:
Reviewer Test Plan
How to verify
Each fix ships with focused unit tests; run them from the repo root:
cd packages/core && npx vitest run src/hooks/httpHookRunner.test.ts— a 302 response is not followed and the redirect target is never contacted (the test asserts the client is invoked with redirect-following disabled).cd packages/cli && npx vitest run src/config/settings.test.ts— workspace-scope whitelist and private-network keys are stripped even when trusted; a user-scope whitelist survives a workspace*entry.cd packages/core && npx vitest run src/hooks/envInterpolator.test.ts src/utils/envVarResolver.test.tsandcd packages/cli && npx vitest run src/utils/envVarResolver.test.ts— internal secret variables are never substituted, even when explicitly whitelisted by config.cd packages/core && npx vitest run src/tools/skill.test.ts src/subagents/subagent-manager.test.ts— project-level frontmatter hooks are not registered when the folder is untrusted, are registered when trusted, and user-level configs register regardless of trust. The two gate tests were mutation-checked: with the gates removed they fail; with them restored all 220 tests in the two files pass.Broader regression:
cd packages/core && npx vitest run src/hooks/ src/subagents/ src/skills/ src/tools/skill.test.ts— 1385 tests green;npm run typecheckclean.Evidence (Before & After)
N/A — non-UI security fixes; verification is the test output above.
Tested on
Environment (optional)
Unit tests via
npx vitest run; no runtime environment required.Risk & Scope
Linked Issues
N/A — found by internal security audit.
中文说明
本 PR 内容
本 PR 关闭了 hook 系统中四个相互独立的信任边界漏洞，全部位于"仓库可控配置"与"代码执行/网络出口"的交界处：
HTTP hooks 不再跟随重定向。 此前 URL 白名单与 DNS 级 SSRF 检查（内网网段、云元数据端点）只针对初始配置的 URL 校验，但底层客户端会自动跟随重定向——任何来自已配置或被攻陷端点的 30x 响应，都可以把完整的 hook 负载（用户 prompt、工具输入输出、session id）重新 POST 到任意内网或元数据地址，且不做任何重新校验。现在重定向被禁用；3xx 响应按 hook 失败处理（非阻塞，与其他 HTTP 错误一致）。
HTTP hook 的 URL 白名单不再接受 workspace 设置。 设置合并逻辑此前已从 workspace scope 剥离"内网放行"开关，防止仓库自我授权 SSRF 绕过；但姊妹白名单键仍可从 workspace scope 生效，仓库可以替换或扩大用户的白名单（例如写成
*），从而越过用户配置的边界外泄 hook 负载。现在两个键都从 workspace scope 剥离；user 与 system scope 不受影响。Qwen 内部 secrets 永远不会被替换进 hook 命令、URL 或请求头。 此前的一次修复已把 daemon token 与私有 ACP capability 从 hook 子进程环境中剥离，但仍有两条路径直接从进程环境解析它们：settings 文件中的环境变量引用（加载时解析，早于任何子进程 env 清洗），以及 HTTP hook 将白名单环境变量插值进请求 URL 与请求头的功能。仓库可控的 settings 文件或 hook 配置只要点名 daemon bearer token，就能让它经网络外泄。现在所有环境变量解析路径都拒绝这些变量。
项目级 frontmatter hooks 需要文件夹受信。 在项目 subagent（
.qwen/agents/）与项目 skill（.qwen/skills/）的 frontmatter 中声明的 hooks，此前在 agent 派生或 skill 调用时无条件注册——即使用户已显式把文件夹标记为不可信，而同样的 hooks 写在仓库 settings 文件里本来会被拦住。现在两条注册路径在不可信文件夹下都会跳过项目级 hooks 并告警；user 级 subagent 与 skill 不受影响，与现有 user hook 信任语义一致。为什么需要
这四个漏洞来自对 hooks 模块的一次系统性安全审计，修复前均已端到端复现：
Reviewer 验证计划
如何验证
每个修复都带有聚焦的单元测试，从仓库根目录运行：
cd packages/core && npx vitest run src/hooks/httpHookRunner.test.ts—— 302 响应不会被跟随，重定向目标从未被请求（测试断言客户端以禁用重定向的方式被调用）。cd packages/cli && npx vitest run src/config/settings.test.ts—— workspace scope 的白名单与内网放行键在受信时也被剥离；user scope 的白名单在 workspace 写入*时仍然存活。cd packages/core && npx vitest run src/hooks/envInterpolator.test.ts src/utils/envVarResolver.test.ts以及cd packages/cli && npx vitest run src/utils/envVarResolver.test.ts—— 即使配置显式把内部 secret 变量列入白名单，也永远不会被替换。cd packages/core && npx vitest run src/tools/skill.test.ts src/subagents/subagent-manager.test.ts—— 不可信文件夹下项目级 frontmatter hooks 不注册，受信时正常注册，user 级配置无论信任与否都注册。这两个门禁测试做过 mutation 验证：去掉门禁它们失败，恢复后两个文件的 220 个测试全部通过。更广的回归：
cd packages/core && npx vitest run src/hooks/ src/subagents/ src/skills/ src/tools/skill.test.ts—— 1385 个测试全绿；npm run typecheck干净。证据（前后对比）
N/A —— 非 UI 的安全修复，验证即上述测试输出。
测试平台
macOS ✅ 已测试；Windows⚠️ 未本地测试（CI 覆盖）；Linux ⚠️ 未本地测试（CI 覆盖）。
环境（可选）
仅通过
npx vitest run运行单元测试，无需运行时环境。风险与范围
关联 Issue
N/A —— 由内部安全审计发现。