feat(core): forward bounded MCP tool arguments to the AUTO-mode classifier - #10352
Conversation
…ifier
DiscoveredMCPTool never overrode toAutoClassifierInput, so every MCP call
reached the classifier as `Tool: mcp__server__tool / Arguments: {}`. Told
to err on the side of blocking, the classifier rejected most of them on
the name alone, which made AUTO mode unusable with MCP and pushed users
toward blanket `mcp__server` allow rules that skip the classifier.
The projection now carries the server name, the server-side tool name,
the server's self-reported annotations, and a bounded copy of the
arguments: 2,000 chars per string, a 16,000-char shared budget, depth and
entry caps, with every cut marked in place and flagged via
`arguments_truncated`. The classifier system prompt explains how to read
the projection — arguments are the evidence for exfiltration and
external-write rules, annotations are unverified, truncation is never a
reason to relax.
`permissions.autoMode.mcp.forwardArguments: false` restores the
name-only projection for deployments whose classifier runs against a
different provider than the main model.
Claude-Session: https://claude.ai/code/session_01YX3fw1haWj6KD5saknQFb6
|
Thanks for the PR! Template looks good ✓ Problem: observed, and the mechanism checks out against the code. The base class Direction: aligned. AUTO-mode usability is a real gap, and giving the classifier the arguments is the only way its data-exfiltration / external-write rules can apply at all. I checked the privacy argument: the classifier runs on Size: core paths touched ( Approach: the scope feels right. The override plugs into the existing Risk: no elevated risk signals (no match against the revert-correlated path list). Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：真实存在，且机制与代码一致。基类 方向：对齐。AUTO 模式的可用性是真实缺口，把参数交给分类器是其数据外泄 / 外部写入规则能够生效的唯一途径。隐私论点我核实过：分类器使用 规模：触及核心路径（ 方案：范围合理。override 直接接入现有 风险：无升级风险信号（未命中与 revert 相关的高风险路径清单）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe approach matches what I'd have proposed independently: override No blockers. Two non-blocking notes:
Test evidence — the PR's own CI on
|
| Check | Conclusion |
|---|---|
| Test (ubuntu-latest, Node 22.x) | ❌ failure (checkout — pre-existing runner infra, see above) |
| Test (macos-latest, Node 22.x) | ⏭️ skipped (cascade) |
| Test (windows-latest, Node 22.x) | ⏭️ skipped (cascade) |
| Integration Tests (CLI, No Sandbox) | ⏭️ skipped (cascade) |
| web-shell E2E Smoke (ubuntu-latest, Node 22.x) | ⏭️ skipped (cascade) |
| Classify PR / label / precheck-pr | ✅ success |
| Desktop Shell (ubuntu-22.04 / windows-2022) | ✅ success |
| Secret scan (TruffleHog) | ✅ success |
| Dependency CVE audit | ✅ success |
Unsettled behavioural claim
The unit tests pin the projection and its wiring, but the PR's central claim — that the classifier now allows benign MCP calls (and still blocks exfiltration-shaped ones), making AUTO mode usable with MCP tools — is behavioural, and nothing in CI exercises a real classifier verdict; the manual verification in the description is the author's claim, not independently run here. Sandboxed verification would settle this: @qwen-code /verify — an A/B run against the base build proving benign mcp__* calls flip from block to allow while a .env-shaped payload stays blocked. (Author has write access, so a maintainer can trigger the lane directly.)
中文说明
代码审查：实现与我独立设想的方案一致——在 DiscoveredMCPTool 上覆写 toAutoClassifierInput，新增一个带上限的投影模块，复用现有 projectFunctionArgs → buildClassifierContents 管线，transcript 零改动。端到端核实了接线：测试用的构造参数位置正确（config 第 8 位、annotations 第 12 位），设置项 permissions.autoMode.mcp.forwardArguments 经 getAutoModeSettings() 直达，分类器模型解析（getFastModel() ?? getModel()）支撑同 provider 的隐私论点，SAFE_TOOL_ALLOWLIST 确认 mcp__* 必然进入分类器。投影本身正确：共享 16k 预算 + 单串/深度/条目上限，每处截断原地标记并有顶层 arguments_truncated 旗标，annotations 白名单只保留规范定义的四个键，容忍非 JSON 值。prompt 与两处文档修改均与代码吻合（含 edit 预览 80→300 的更正）。
无阻塞项。两条非阻塞建议：代码实际转发四个 annotation 键（ANNOTATION_KEYS 含 idempotentHint），但 prompt 行和 auto-mode.md 只列了三个，建议对齐；描述称 settingsSchema.test.ts 覆盖新设置，但该文件无改动——schema 条目本身没有直接测试（运行时路径由 mcp-tool.test.ts 的 config mock 覆盖），手写同步的 settings.schema.json 依赖 CI 的 schema 一致性门禁，而该门禁在下面的运行中未能执行到。
测试证据：Test (ubuntu-latest, Node 22.x) 在 actions/checkout 阶段失败——PR 代码一行都未执行：共享 runner 上遗留的 root 所有 .qwen 目录导致工作区清理 EACCES，checkout 中止。这是既有的 runner 基础设施噪音，与本 PR 无关（diff 不可能在 checkout 阶段引发 EACCES）。macOS/Windows、集成测试、web-shell 冒烟随之跳过——本提交目前没有任何绿色测试证据，需要重跑失败任务或推新提交。其余已运行的检查（分类/标签/预检、桌面壳、密钥扫描、依赖 CVE）全部通过。
未定论的行为性声明：单元测试固定了投影与接线，但"分类器因此放行良性 MCP 调用（并仍拦截外泄形态的调用）、AUTO 模式对 MCP 工具变得可用"这一核心声明是行为性的，CI 没有真实分类器裁决的证据；描述中的手动验证是作者声明，本次无人值守运行未独立执行。沙盒验证可以定论：@qwen-code /verify —— 与 base 构建 A/B 对照，证明良性 mcp__* 调用由拦截翻转为放行、而 .env 形态的载荷仍被拦截。（作者有 write 权限，maintainer 可直接触发。）
— Qwen Code · qwen3.8-max
Reviewed at 851a26a136e9fe7a87e99e2eef99b6db07a4b7d3 · re-run with @qwen-code /triage
|
Confidence: 3/5 — the code review is genuinely clean, but this commit has no green CI evidence (the unit suite died in checkout on leaked runner files before executing anything) and the behavioural claim is unsettled, so I'm deferring rather than approving. Stepping back: this is a well-built PR. The problem is real and code-verified (the classifier literally receives What holds it back is evidence, not code:
@qqqys you have write access — re-running the failed 中文说明整体来看这是一个完成度很高的 PR：问题真实且经代码核实（今天分类器对每个 挡住它的是证据而非代码：
@qqqys 你有 write 权限——重跑失败的 — Qwen Code · qwen3.8-max Reviewed at |
…ry key visible Addresses review round 1 on QwenLM#10352: - Build projected objects with a null prototype so an argument key named `__proto__` stays an own, visible property instead of vanishing through the Object.prototype setter (R1-1). - Cap `server` / `tool` names at 200 chars inside the shared budget, strip control characters, and flag cuts with `name_truncated` (R1-2). - Charge the budget at serialized cost (encoded length plus pretty-print line overhead), truncate keys like values, charge every marker, and stop container iteration once the budget is spent, so the pretty-printed payload the classifier receives stays within the budget plus one marker per nesting level (R1-3, R1-4). - Pick a collision-free key for the remainder marker (R1-6). - Unify marker forms to `…[truncated N chars]` / `[omitted: …]` and say so in the classifier prompt and docs (R1-5). - Update the base-class `toAutoClassifierInput` docstring to mention the MCP override (R1-7). - Bound rendered historical actions in the classifier transcript: 4,000 chars each, 40,000 in aggregate, newest kept first, older ones reduced to their tool name plus an omission marker (R1-8). Claude-Session: https://claude.ai/code/session_01YX3fw1haWj6KD5saknQFb6
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.22.2)
…orwards `ANNOTATION_KEYS` forwards four keys, but the classifier system prompt and the auto-mode doc enumerated only three: a server could assert `idempotentHint` and the classifier would see a key the prompt never marked as self-reported and unverified, right beside the rule that annotations never justify allowing an action on their own. Nothing pinned that key either — removing `'idempotentHint'` from `ANNOTATION_KEYS` left the whole suite green. The projection test now passes all four keys and asserts all four echo back through the exact-match `toEqual`, and a new sibling test keeps the boolean-only filter live (a non-boolean `idempotentHint` must not reach the prompt), which the widened test would otherwise have stopped covering. Verified by mutation: dropping `'idempotentHint'` from `ANNOTATION_KEYS` reds the projection test, dropping it from the prompt enumeration reds the `MCP guidance` test, and relaxing the boolean filter to a `!== undefined` check reds the new filter test. Claude-Session: https://claude.ai/code/session_018dYE4LwSMeMPFchXk5UBdM
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/mcp-classifier-input.test.ts:159 — [probe] No triple key-collision test; a while → if mutant in uniqueKey survives the suite
中文说明
已审查——无阻断问题。 建议见行内评论。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
…list The four hand-copied `expect(prompt).toMatch(/xHint/)` assertions duplicated `ANNOTATION_KEYS`, so they only caught a key *removed* from the projection. A key added to `ANNOTATION_KEYS` was forwarded by `projectAnnotations` immediately while the prompt never named it, and the suite stayed green -- the classifier would then receive an annotation key the prompt never marked as unverified. This is the drift that had to be repaired by hand for `idempotentHint` one commit ago. Export the list and iterate it so the guard works in both directions. Claude-Session: https://claude.ai/code/session_01Y7nLadH7zFM6B6bJZ2Vfzk
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.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.22.2)
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 explored to full depth (tool budget reached): "agent test-matrix": none — no check was cut short..
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/mcp-tool.ts:1016 — [probe] no cloned-tool test pins the classifier projection's cliConfig/annotations propagation (asFullyQualifiedTool)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent test-matrix"：none — no check was cut short.。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
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 explored to full depth (tool budget reached): "agent 3b": run packages/core mcp-classifier-input.test.ts (worktree has no node_modules/vitest installed; a monorepo install + build to execute one file was beyond this le….
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 3b"：run packages/core mcp-classifier-input.test.ts (worktree has no node_modules/vitest installed; a monorepo install + build to execute one file was beyond this le…。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Maintainer verification — built and exercised end-to-endTested Harness. Two full builds (PR head and merge-base), each bundled to 1. The behavioural claim holds, and the failure it fixes is worse than the PR saysIdentical user request, identical tool, identical everything — only the argument text differs. 3 runs per cell.
On merge-base the classifier is not conservative, it is uncorrelated: it blocked the harmless reminder 2/3 and allowed the credential dump 3/3, and the MCP server received The wire, same run, same call:
Annotations arrive verbatim from the server's 2. The bounds hold under hostile input30 adversarial payloads against the built projection, measured on
Worst case observed is 1.03× the 16,000-char budget at 148 ms, including a 10 MB string, 100k keys, a depth-5000 chain, escape-only strings, and a 4^7 wide+deep bomb. Nothing threw, nothing ran long. The two deferred probe items from the bot rounds also check out on the built code: Truncation is visible, in place and flagged, on a real 60 KB argument:
3. Forwarding attacker-controlled text into the classifier prompt is containedThe PR opens a surface that did not exist before: MCP argument content now lands inside the classifier prompt. I sent a payload that tries to forge a new prompt section and a fake pending-call block.
4. Both halves of the PR are load-bearingI blanked the new
The projection alone fixes the visible cases. The prompt line is what closes the padded case — 2,100 chars of benign standup notes with the secrets pushed past the 2,000-char string cap leaked 2/3 without it and 0/3 with it, on a payload where the classifier can literally only see 5. The opt-out works — with one gap
F1 — This fallback is pre-existing (the same leak reproduces on merge-base) and the PR's new 4,000-char historical cap actually bounds it for the first time, so it is not a regression. It is worth a line anyway because this PR is what turns it into a promise a deployment can rely on. One-line fix: treat an unresolvable 6. Smaller notes (none blocking)
7. Repo hygiene
The LimitationsLinux only. One live classifier model — VerdictThe behavioural claim reproduces on the real binary and is stronger than the PR body claims. The bounds are genuine, the injection surface it opens is closed, and the counterfactual shows the prompt change is not decoration. F1 is worth a follow-up line (or a commit here, it is one line); nothing I found blocks the merge. 中文版维护者验证 —— 完整构建并端到端跑通测试对象为当前 head 验证环境。 两套完整构建（PR head 与 merge-base），各自 bundle 出 1. 行为主张成立，而且它修的问题比 PR 描述的更严重用户请求相同、工具相同、其他一切相同——只有参数文本不同。每格跑 3 次。 在 merge-base 上，分类器不是"保守"，而是与调用无关：它把无害的提醒拦了 2/3，却把凭据外泄放行了 3/3，而且这 3 次 MCP server 都实际收到了 Annotations 原样来自 server 的 2. 上限在恶意输入下守得住30 组对抗性 payload 打在构建产物的投影上，测量口径是 最坏观测值是 16,000 字符预算的 1.03 倍，148 ms，其中包括 10 MB 字符串、10 万个 key、深度 5000 的链、全转义字符串、以及 4^7 的宽+深炸弹。没有抛异常，没有跑飞。bot 几轮里延后的两个 probe 项在构建产物上也都成立： 在真实的 60 KB 参数上，截断标记就地可见并被打上标志。 3. 把攻击者可控文本送进分类器 prompt 这件事是收得住的本 PR 打开了一个此前不存在的面：MCP 参数内容现在会进入分类器 prompt。我构造了一个试图伪造新 prompt 段落和假的 pending-call 块的 payload。
4. PR 的两半都是承重的我把构建产物里新增的 只有投影就能修好"看得见"的那些情况。但真正堵住"填充绕过"的是新增的 prompt 行——2,100 字符良性站会记录、把密钥推到 2,000 字符上限之外，没有那行时泄漏 2/3，有那行时 0/3；而在这个 payload 上，分类器能看到的只有 5. Opt-out 有效——但有一个缺口把 F1 —— 对于工具已经不在 registry 里的历史条目， 这个回退是既有行为（同样的泄漏在 merge-base 上也能复现），而且本 PR 新增的 4,000 字符历史上限反而是第一次给它加了界，所以不是回归。之所以仍然值得写一句，是因为正是这个 PR 让它变成了一个部署方会去依赖的承诺。一行修复：把无法解析的 6. 其他小项（都不阻断）
7. 仓库层面的检查
PR 上的 局限仅 Linux。只有一个可用的真实分类器模型 —— 结论行为主张在真实二进制上复现，而且比 PR 描述的更有说服力。上限是真的，它打开的注入面是收住的，反事实实验说明 prompt 改动不是装饰。F1 值得一个后续（或者就在这个 PR 里改，只要一行）；我找到的东西没有一项阻断合并。 |
Two gaps the maintainer verification found in the classifier projection.
The `permissions.autoMode.mcp.forwardArguments` opt-out lives on the tool
object, so a history entry whose MCP server was removed from settings (or
a session resumed without it) had nothing left to express it:
`projectFunctionArgs` fell back to the raw arguments and forwarded a prior
call's payload — secrets included — into the classifier prompt unbounded.
An `mcp__*` name the registry cannot resolve now projects to the same `{}`
an opted-out MCP tool produces, as does a resolved MCP tool whose
projection threw: for MCP arguments the fallback must not be the unbounded
one. Unknown non-MCP tools keep passing their args through.
A non-object payload (an array, a bare string) projected to `{}` with no
`arguments_truncated` flag, so dropped content read as a call that
genuinely had no arguments — the one place the module broke its own
"omitted content is never presented as absent" invariant. Absent params
stay unflagged; anything else is marked.
Claude-Session: https://claude.ai/code/session_01NkW1J2aBKcsKS62dkPcWbT
|
Thanks — that verification is far beyond what I could produce locally, and the A/B matrix is a better statement of the problem than my PR body was. I have rewritten the body's "blocked most MCP calls outright" line to state the uncorrelated-in-both-directions shape your matrix showed. The two findings: F1 — fixed ( F2 — fixed (same commit). A non-object payload (array, bare string, number) now sets F3 (non-MCP half) and F4 left as-is: the generic Local verification on On the merge state: the 中文说明感谢这份验证——远超我本地能做到的程度，那个 A/B 矩阵对问题的刻画也比我 PR 正文里的说法更准确（正文那句"blocked most MCP calls outright"我会一并改掉）。两个发现： F1 已修（ F2 已修（同一提交）：非对象 payload（数组、裸字符串、数字）现在会置 F3（非 MCP 的那一半）与 F4 保持现状：通用 本地验证（ 关于合并状态：那条 |
Re-verified at
|
| check | result |
|---|---|
core permissions + MCP projection + mcp-tool suites |
983 passed |
packages/cli settingsSchema.test.ts |
47 passed |
tsc --noEmit core + cli |
clean |
eslint + prettier --check on the 11 changed .ts files |
clean |
merge of current origin/main |
auto-merges; merged tree typechecks, 983 tests pass |
| adversarial projection suite (30 payloads) | worst case 1.03× budget, 148 ms, no throw |
Both findings are closed and I could not turn up anything new. The only thing standing between this and a merge is the stale round-5 CHANGES_REQUESTED and a CI re-run.
中文版
在 19bf27af 上重新验证
针对新 head 重新构建并重跑了整套验证环境。merge-base 未变（36de6eb），所以 base 那几组也是重跑的，而不是沿用上次结果。
这一轮 base 组比上一轮更一边倒：凭据外泄再次 3/3 放行，而这次连良性那条也被放行——因此两轮合计，merge-base 对外泄 payload 是 6/6 放行（每次密钥都实际送达 MCP server），对无害 payload 是 4/6 放行。新 head 在两个方向上 6/6 正确。其余各组全部复现：注入收敛、密钥填充到上限之外、60 KB 截断标记、readOnlyHint: true 的 server，以及 forwardArguments: false opt-out。
F1 —— 已修，同一复现路径确认
之前会泄漏的那条完整序列——带密钥调用 slackbot.post_message，从 settings.json 移除 slackbot，qwen --resume，再调用一个仍然配置着的 server——现在在分类器请求里是 Prior action: mcp__slackbot__post_message({})，被移除 server 的 payload 在整个请求体中都不存在。
我单独核了这个守卫的影响范围，因为 startsWith('mcp__') 看起来是个很宽的判据：它覆盖 history 以及未注册的 pending 调用；能把 500k 字符的 payload 收敛到 33 个字符；能接住"已解析但投影抛异常"的 MCP tool（F3 中真正要紧的那一半，感谢一并处理）；同时未知的非 MCP 工具仍与此前完全一致地透传原始参数。最后这条是关键的反向验证：修复被限定在 MCP 一侧，没有悄悄让分类器对别的东西失明。
F2 —— 已修，且两处修复都被真正钉住
非对象契约的行为与描述一致：undefined / null / {} 不置位，数组 / 裸字符串 / 数字置 arguments_truncated。false 与 NaN 也会置位，这是保守读法，没问题。
突变检查我自己独立跑了一遍，没有直接采信。移除 startsWith('mcp__') 守卫恰好红一个测试（drops the arguments of an MCP call whose tool left the registry，其余 982 仍绿）；还原截断标志恰好红两个（更新后的 projects non-object inputs… 加新增的 flags a non-object payload…，其余 981 绿）。两次突变都没有波及其他用例，因此这两个测试确实在钉它们声称的行为，而不是碰巧通过。
关于 F3 的非 MCP 那一半和 F4：同意，保持现状。MCP 一侧的回退才是要紧的那个，且已覆盖；通用 catch 与 pending 调用重复都是既有行为、与本次改动正交，为此扩大 diff 只会让审查更难而不是更安全。要跟踪的话挂到 #10353 是合适的。
CI
19bf27af 上 Test (ubuntu-latest, Node 22.x) 是红的，原因在 runner。
三个失败，没有一个位于本 PR 改动的文件里：一个 wall-clock 预算超了 1.5%（1014.66 < 1000），另外两个是 5 秒超时，而那两个 suite 报告的 collect 时间分别是 760 秒和 172 秒，实际测试时间只有 21 秒和 19 秒。main 现在在同一个 job 上也是红的——run 33242953180 失败在 pendingPromptVersion.test.ts > bumps queue version for pending-prompt queue events，同一个测试，比本 PR 这次运行早 27 分钟，同时还有 recall-scan-latency 和两个 DOM 测试；main 上的 run 33241200951 还有另外八个同类失败。在本地 19bf27af 上，core 与 webui 那两个文件都通过（548 与 7 个用例）；vscode-companion 那个 bundle 测试在我这里因为第三个无关原因失败（@qwen-code/web-shell 未构建），在 merge-base 上同样失败。等机器空一点重跑应该就能过。
19bf27af 上的仓库层面检查
| 检查项 | 结果 |
|---|---|
core permissions + MCP 投影 + mcp-tool 测试 |
983 通过 |
packages/cli settingsSchema.test.ts |
47 通过 |
tsc --noEmit core + cli |
干净 |
对 11 个改动 .ts 文件跑 eslint + prettier --check |
干净 |
合入当前 origin/main |
自动合并成功；合并后类型检查通过，983 个测试通过 |
| 对抗性投影套件（30 组 payload） | 最坏 1.03 倍预算、148 ms、无异常 |
两个发现都已关闭，我也没能再找出新的问题。现在挡在合并前面的只剩那条过期的第 5 轮 CHANGES_REQUESTED 和一次 CI 重跑。
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Approving on the strength of the two verification runs above (#issuecomment-5460961644 at 19ef2cf, #issuecomment-5461571484 at 19bf27af).
Both findings I raised are fixed and independently mutation-checked: the mcp__* fail-closed guard is correctly scoped (history and pending call, throwing projections included; unknown non-MCP tools unchanged), and the non-object payload now flags arguments_truncated while genuinely-absent params stay unflagged. The behavioural claim reproduces on the real binary against a real MCP server with a live classifier — 6/6 correct in both directions at the new head, against 6/6 allow on the credential-exfiltration payload at merge-base. Bounds hold under 30 adversarial payloads (worst case 1.03× budget, 148 ms), the injection surface the projection opens is contained, and the counterfactual shows the prompt line is load-bearing rather than decorative.
This clears the round-5 CHANGES_REQUESTED, whose finding was fixed by 19ef2cf. The red Test (ubuntu-latest, Node 22.x) is a loaded-runner flake — three failures, none in a file this PR touches, and main is red on the same job with one of the identical tests; it needs a re-run, not a code change.
中文说明
基于上面两轮验证（19ef2cf 的 #issuecomment-5460961644 与 19bf27af 的 #issuecomment-5461571484）批准。
我提出的两个发现都已修复并经过独立突变检查：mcp__* fail-closed 守卫范围正确（覆盖 history 与 pending 调用、含投影抛异常的情形；未知的非 MCP 工具行为不变），非对象 payload 现在会置 arguments_truncated，而真正没有参数的情况不置位。行为主张在真实二进制 + 真实 MCP server + 真实分类器模型下复现——新 head 两个方向 6/6 正确，而 merge-base 对凭据外泄 payload 是 6/6 放行。30 组对抗性 payload 下上限成立（最坏 1.03 倍预算、148 ms），投影打开的注入面是收住的，反事实实验说明新增的 prompt 行是承重的而非装饰。
这解除第 5 轮的 CHANGES_REQUESTED，该轮的发现已由 19ef2cf 修复。红掉的 Test (ubuntu-latest, Node 22.x) 是 runner 负载导致的 flake——三个失败没有一个在本 PR 改动的文件里，且 main 在同一个 job 上也是红的、其中一个测试完全相同；需要的是重跑，而不是改代码。
Stale: all eight round-1 findings were addressed in f595cbd, 7ec7c4c and 6fdba2b, which land after this review's commit b24ddaa. Independently verified against the built code at 19bf27a (see the verification comments on this PR): a __proto__ argument key stays visible as an own key; server/tool names are charged against the shared budget (1 MB names project to 541 chars); the key-budget and remainder-marker cases hold across 30 adversarial payloads, worst case 1.03x the 16,000-char budget at 148 ms; the triple key collision keeps all three real values with the marker on a fresh key; and the transcript now carries the aggregate cap this round asked for (40 large MCP calls stay inside 40,000 chars).
|
@qqqys — code-wise this is done from my side; the only thing left is CI, and it will not settle on its own. Could you rebase onto current
A regression introduced by a PR fails the same tests every time. A set that reshuffles on each attempt, made of wall-clock budgets missed by 1.2×–3.4× and 5–15 s timeouts, is a saturated runner — and none of those files appear in I cannot force another attempt: GitHub refuses to re-run
Heads-up: 中文说明@qqqys —— 代码这边我这里已经没有问题了，只剩 CI，而且它自己不会好。能否 rebase 到当前
PR 引入的回归每次会挂同样的测试。每次都换一批、且都是 wall-clock 预算超出 1.2×–3.4× 和 5–15 秒超时，这是 runner 过载；而且这些文件没有一个出现在 我没法再强制触发一次：GitHub 拒绝重跑
提醒一下：本仓库开启了 |
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.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/core/src/permissions/classifier-transcript.ts:277 — [probe] throwing-projection route of the fail-closed {} fallback is untested (D7-1)packages/core/src/permissions/classifier-transcript.ts:196 — [probe] budget-elision line re-emits the raw unbounded model-emitted tool name (D7-2)packages/core/src/permissions/classifier-transcript.ts:189 — [probe] budget-elision output unbounded in elided-action count under parallel tool calls (D7-3)
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
Correction to my previous comment — @qqqys please don't push yet, it would not help. I asked for a rebase on the premise that a fresh
Three different runners, three cancellations at exactly the timeout, each with This is repo-wide, not this PR.
Nothing here changes the verification: the code is good, both findings are fixed, and my approval stands (a push now would only dismiss it for nothing). Flagging it here so the CI ownership side sees it — this is currently blocking every PR, not just this one. 中文说明更正我上一条评论 —— @qqqys 先别 push，push 也没用。 我当时请你 rebase，前提是"新的
三台不同的 runner，三次都恰好在超时点被取消，且孤儿进程清理时 这是全仓库的问题，不是本 PR。
这些都不影响验证结论：代码没问题，两个发现都已修复，我的 approve 继续有效（现在 push 只会白白让它失效）。在这里标记出来是为了让 CI 侧看到——它目前挡的不只是这一个 PR。 |
4 participants
What this PR does
In AUTO mode, every tool call that is not fast-pathed goes to the LLM classifier together with a projection of its arguments. MCP tools never had a projection, so the classifier saw
Tool: mcp__server__toolfollowed byArguments: {}and had to decide on the name alone. This PR gives discovered MCP tools a real projection: the server name, the server-side tool name, the server's self-reported annotations (readOnlyHint/destructiveHint/idempotentHint/openWorldHint) and a bounded copy of the call arguments.The bound is deterministic and always visible to the classifier: each string (value or key) is cut at 2,000 characters, server/tool names at 200, the whole payload shares a 16,000-character budget charged at serialized cost (so the pretty-printed form the classifier receives stays within it), nesting is capped at 8 levels and arrays/objects at 64 entries, and iteration stops as soon as the budget is spent. Every cut is replaced in place by a marker (
…[truncated N chars]or[omitted: …]) and the projection carries top-levelarguments_truncated: true/name_truncated: trueflags whenever anything was removed. Projected objects are built with a null prototype so a key named__proto__stays visible. Historical actions in the classifier transcript are capped too (4,000 chars each, 40,000 in aggregate, newest first), since they now carry real MCP payloads.The classifier system prompt gains one Environment line explaining how to read an
mcp__*call: the arguments are what the agent is about to send to a third-party process, so they are the evidence for the data-exfiltration and external-write rules; the annotations are asserted by the server and unverified, so they may add suspicion but never justify an allow on their own; truncation markers mean content was omitted, and omitted content is never evidence of safety.A new setting,
permissions.autoMode.mcp.forwardArguments(defaulttrue), restores the previous name-only projection for deployments whose classifier runs against a different provider than the main model. The docs for auto mode are updated to describe the actual projection and the opt-out; the FAQ line that claimed MCP tool authors could opt in viatoAutoClassifierInput(they cannot — it is a method on qwen-code's tool class) is replaced, and the stale "80 characters" figure for edit previews is corrected to the 300 the code uses.Why it's needed
The classifier is instructed to err on the side of blocking, so with no arguments to look at it blocked many MCP calls outright; three consecutive blocks then trip the denial tracker and drop the user into manual approval. Over-blocking is only the visible half. A name-only verdict is not conservative, it is uncorrelated with the payload, and it fails in both directions: the maintainer verification on this PR (comment) drove the real binary against a real MCP server and a live classifier model and recorded the harmless reminder blocked 2/3 while a call carrying
AWS_SECRET_ACCESS_KEY/DATABASE_URL/STRIPE_SECRETwas allowed 3/3, with the server receiving the secrets every time. So AUTO mode did not work with MCP tools in either sense, and the only documented escape was apermissions.allow: ["mcp__server"]rule — which removes the classifier from the path entirely, the least safe option available.The original fail-closed choice (#4151 review) was made to avoid leaking secrets from MCP arguments into the classifier LLM. That concern does not hold in the default configuration: the arguments are the main model's own function-call output and have already been sent to the same provider. What the name-only projection actually removed was the classifier's ability to tell
slack.post_message({text: "deploy done"})fromslack.post_message({text: <contents of .env>}). Both Claude Code (k=vprojection, uncapped) and Codex's auto-review reviewer (full arguments, 16k-token string cap with<guardian_truncated/>markers) forward MCP arguments for exactly this reason; this PR follows the capped variant.Reviewer Test Plan
How to verify
Unit tests cover the projection and its wiring:
cd packages/core && npx vitest run src/tools/mcp-classifier-input.test.ts src/tools/mcp-tool.test.ts src/permissions/classifier-transcript.test.ts src/permissions/classifier-prompts/system-prompt.test.ts(169 tests).mcp-classifier-input.test.tschecks the string / budget / depth / entry caps and the marker text;mcp-tool.test.tschecks that aDiscoveredMCPToolforwards server, tool, annotations and arguments, tolerates a config withoutgetAutoModeSettings, and returns the name-only sentinel whenforwardArgumentsisfalse;classifier-transcript.test.tsdrives a realDiscoveredMCPToolthroughbuildClassifierContentsand asserts the pending-call prompt contains the server, tool, annotation and argument text;system-prompt.test.tsasserts the new guidance is in the prompt.cd packages/cli && npx vitest run src/config/settingsSchema.test.tscovers the new setting.Manually: start
qwenwith any MCP server attached (the bundlednode-replserver works), switch to AUTO mode with Shift+Tab, and ask for something that calls an MCP tool. Before this PR the call is blocked with a classifier reason that only names the tool; after it, the call is judged on its arguments and a benign call is allowed. Setpermissions.autoMode.mcp.forwardArguments: falseand repeat to see the name-only behaviour return.Evidence (Before & After)
Before — classifier payload for an MCP call (from
buildClassifierContents):After:
Tested on
Environment (optional)
Linux, unit tests only (
npm run typecheck,eslintandprettier --checkon the changed files, vitest per package).Risk & Scope
permissions.autoMode.mcp.forwardArguments: false. The 16k-character budget adds at most a few thousand tokens to a classifier request for a large call.destructiveHintis forwarded as context but does not yet force manual approval (Claude Code does this; planned as a follow-up), and server-levelmcp__serverallow rules are still not stripped on AUTO entry the wayBash(*)is (also a follow-up). Trusted servers (trust: true) keep bypassing the classifier as before.true; existingpermissions.allowrules for MCP tools keep bypassing the classifier.Linked Issues
Part of #10353 (PR 1 of 3). Follow-up to #4151 (AUTO mode), which introduced the name-only default for MCP tools.
中文说明
这个 PR 做了什么
在 AUTO 模式下，没有走快速通道的每个工具调用都会连同其参数投影一起交给 LLM 分类器。MCP 工具从来没有自己的投影，所以分类器看到的是
Tool: mcp__server__tool加上Arguments: {}，只能凭名字判断。本 PR 给发现的 MCP 工具补上真正的投影：服务器名、服务器侧的工具名、服务器自报的 annotations（readOnlyHint/destructiveHint/idempotentHint/openWorldHint），以及一份有上限的参数副本。上限是确定性的，且分类器始终可见：每个字符串（值或键）截到 2,000 字符，server/tool 名截到 200，整个载荷共享 16,000 字符预算，按序列化成本计费（分类器实际收到的 pretty-print 形式不会超出），嵌套深度上限 8 层、数组/对象条目上限 64，预算耗尽即停止遍历。每一处截断都原地替换为标记（
…[truncated N chars]或[omitted: …]），并且只要有任何内容被删掉，投影顶层就带arguments_truncated: true/name_truncated: true。投影对象用空原型构建，名为__proto__的键仍然可见。分类器 transcript 中的历史动作同样设上限（单条 4,000 字符、总量 40,000，优先保留最新），因为它们现在携带真实的 MCP 载荷。分类器 system prompt 新增一行 Environment 说明如何读
mcp__*调用：参数就是 agent 即将发给第三方进程的内容，是数据外泄和外部系统写入规则的判断依据；annotations 是服务器自己声明、未经验证的，可以增加怀疑但不能单独作为放行理由；截断标记表示内容被省略，省略的内容永远不能视为安全。新增设置
permissions.autoMode.mcp.forwardArguments（默认true），用于分类器与主模型不在同一 provider 的部署，可恢复之前只传名字的投影。auto mode 文档更新为描述实际的投影和退出方式；FAQ 中"MCP 工具作者可以通过覆盖toAutoClassifierInput来 opt-in"的说法（做不到——那是 qwen-code 工具类上的方法）被替换，edit 预览"80 字符"的过时数字也改为代码实际使用的 300。为什么需要
分类器被要求"宁可拦截"，因此在没有参数可看时，它把很多 MCP 调用直接拦下；连续三次拦截触发 denial tracker，用户被丢回手动审批。但过度拦截只是能看见的那一半。仅凭名字的判定不是"保守"，而是与 payload 无关，并且会向两个方向出错：本 PR 的维护者验证（评论）用真实二进制、真实 MCP server 和真实分类器模型跑出的结果是——无害的提醒被拦 2/3，而携带
AWS_SECRET_ACCESS_KEY/DATABASE_URL/STRIPE_SECRET的调用3/3 全部放行，且 MCP server 每次都收到了这些密钥。所以 AUTO 模式对 MCP 工具在两个意义上都不可用，而文档给的唯一出路是permissions.allow: ["mcp__server"]规则——这会把分类器整个从路径上移除，是所有选项里最不安全的一个。最初的 fail-closed 选择（#4151 评审）是为了避免 MCP 参数里的密钥泄漏给分类器 LLM。这个顾虑在默认配置下不成立：参数是主模型自己的 function-call 输出，已经发给了同一个 provider。只传名字真正去掉的，是分类器区分
slack.post_message({text: "deploy done"})和slack.post_message({text: <.env 的内容>})的能力。Claude Code（k=v投影，无上限）和 Codex 的 auto-review 审批器（完整参数，字符串 16k token 上限并带<guardian_truncated/>标记）都出于同样的理由传递 MCP 参数；本 PR 采用带上限的方案。审查测试计划
如何验证
单元测试覆盖投影及其接线：
cd packages/core && npx vitest run src/tools/mcp-classifier-input.test.ts src/tools/mcp-tool.test.ts src/permissions/classifier-transcript.test.ts src/permissions/classifier-prompts/system-prompt.test.ts（169 个用例）。mcp-classifier-input.test.ts检查字符串 / 预算 / 深度 / 条目上限和标记文本；mcp-tool.test.ts检查DiscoveredMCPTool会转发 server、tool、annotations 和 arguments，能容忍没有getAutoModeSettings的 config，并在forwardArguments为false时返回只传名字的哨兵值；classifier-transcript.test.ts让真实的DiscoveredMCPTool走一遍buildClassifierContents，断言待分类调用的 prompt 包含 server、tool、annotation 和参数文本；system-prompt.test.ts断言新的指引在 prompt 里。cd packages/cli && npx vitest run src/config/settingsSchema.test.ts覆盖新设置。手动验证：挂任意一个 MCP 服务器启动
qwen（自带的node-repl服务器即可），Shift+Tab 切到 AUTO 模式，请求一个会调用 MCP 工具的任务。本 PR 之前该调用会被拦截，分类器给出的理由只有工具名；之后调用按参数判断，良性调用被放行。把permissions.autoMode.mcp.forwardArguments设为false再试一次，可以看到只传名字的行为恢复。证据（前后对比）
之前——一次 MCP 调用的分类器 payload（来自
buildClassifierContents）：之后：
测试环境
环境（可选）
Linux，仅单元测试（
npm run typecheck，对改动文件运行eslint和prettier --check，按包运行 vitest）。风险与范围
permissions.autoMode.mcp.forwardArguments: false退出。16k 字符预算对一次大调用最多给分类器请求增加几千 token。destructiveHint作为上下文转发，但尚未强制人工审批（Claude Code 是这样做的；计划作为后续 PR），服务器级mcp__serverallow 规则在进入 AUTO 时仍然不会像Bash(*)那样被剥离（同样是后续）。受信任的服务器（trust: true）和以前一样继续绕过分类器。true；已有的 MCP 工具permissions.allow规则继续绕过分类器。关联 Issue
#10353 的一部分（三个 PR 中的第 1 个）。#4151（AUTO 模式）的后续，该 PR 引入了 MCP 工具只传名字的默认行为。