fix(desktop): isolate invalid optional MCP tools - #3366
Conversation
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
深入 review 后需要修改再合入。
两个阻断问题：
- optional MCP tool 的过滤只验证单个 tool，无法保证最终 capability manifest 仍满足单 offer 64 tools、总 manifest 56 KiB 等协议约束，candidate 仍会因完整注册失败。
- AI SDK
jsonSchema()默认没有validate，当前 fallback 会直接放行参数；这使 Desktop 与 CodeMode 的 JSON Schema 执行契约不一致，非法参数会在 admission 后发往 MCP server。
建议以完整 manifest 作为唯一验证单位，确定性地加入 optional tool；并抽取 Runtime 共用的 JSON Schema validation adapter。另请将无 production change 对应的 rate-limit test 拆出本 PR。
| try { | ||
| decodeClientCapabilityReplaceInput({ | ||
| registrationId: "desktop_capability_validation", | ||
| offers: [capabilityOffer({ ...group, tools: [tool] }, hostPathAccess)], |
There was a problem hiding this comment.
这里每次只对一个 tool 构造 offer 并 decode，但最终会把整个 MCP group 一次性注册。协议的 CLIENT_CAPABILITY_MAX_TOOLS_PER_OFFER（64）和 56 KiB manifest 限制都是跨 tool 的：65 个各自合法的 MCP tool，或 3 个各约 20 KiB 的 schema，都会分别在最终注册时失败。请按最终完整 manifest 做增量校验并确定性地省略不能加入的 optional tool，否则 candidate 仍会被 optional MCP 工具拖垮。
| typeof parameters.jsonSchema === "object" && | ||
| !Array.isArray(parameters.jsonSchema) | ||
| ) { | ||
| return value; |
There was a problem hiding this comment.
buildMcpTools() 用的是 AI SDK jsonSchema(descriptor.inputSchema)；AI SDK 将 validate 定义为 optional，默认正是 undefined。因此这个分支会把例如缺少 required 字段的参数直接放行，并在 options.accept() 后调用 MCP。CodeMode 已对同一类 schema 用 Ajv fallback 验证。请复用/抽取那条 Runtime 级 validation path，而不是在 Desktop 绕过 schema 的执行契约。
| }); | ||
| }); | ||
|
|
||
| test('a rate-limited root Turn releases admission before regenerate', async () => { |
There was a problem hiding this comment.
本 PR 没有对应的 Runtime Host production change；该 test 在 main 上也应成立，且与 optional MCP schema isolation 无关。请从此 PR 移除，若确有独立 rate-limit regression，再用 focused PR 提交，避免继续扩张较重的 execution-host suite。
|
Addressed the requested changes in 2bdd7f9. Optional MCP tools are now admitted deterministically only when the complete manifest decodes successfully, covering the 64-tool and 56 KiB limits. Desktop now uses the Runtime Ajv JSON Schema adapter before accept/MCP dispatch, with a regression test proving invalid arguments are neither accepted nor invoked. The unrelated rate-limit Runtime Host test has been removed. |
d593184 to
2bdd7f9
Compare
|
修复 Windows package check 的自动更新失败：electron-builder 的 �xtraMetadata.version 只更新应用包内 manifest，不会更新 PE ProductVersion。提交 209ffe0 在构建 version-bumped installer 时临时写入 Desktop manifest，构建结束后（含失败路径）恢复原文件，使 ProductVersion 与 latest.yml 一致。请重新检查 Windows package job。 |
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Independent review of exact head 60c26e80b9779d8d729467b9a0fed6f28f6f8b15.
三问
- 要解决什么：一条非法的可选 MCP 工具会让整次 Client Capability replace 失败，连 Browser / Computer Use 等必选 offer 一起带走。
- 怎么解决：只给
desktop_mcp开invalidToolPolicy: "omit"；prepareCapabilityGroups按完整 selected manifest 调decodeClientCapabilityReplaceInput，能过才收下这把工具；必选 group 在 omit 循环前先 decode，失败直接抛。调度时用抽出来的validateJsonSchemaInput在accept()之前验参。 - 奥卡姆：隔离本身是小而准的。同一 PR 里的 Windows autoupdate 打包/校验改写是另一件事（分支还叫
fix/rate-limit-session-recovery）。
隔离判定边界
「无效」在这里等于「这把工具会让 Host 拒掉 replace」，不是 Ajv 能不能编译 schema。
- 会拦：单 offer 64 把、总 256、manifest 56KiB、单 schema 32KiB、keyword allowlist、未解析的本地
$ref、根 schema 不是type: "object"、重复 identity。 - 贪心前缀：排在后面、协议上合法的工具，可能只因为预算被丢掉。非法工具不会污染后面的兄弟（UDS 测试：
mcp_invalid再mcp_valid）。 patternProperties/not/if/then/else/ OpenAPInullable这类常见 JSON Schema 会被 omit。这是 Host 协议的门，不是这 PR 新发明的；相对 Host 不是误判，相对「MCP 服务器认为有效」会静默少工具。- Ajv 不在 admission。协议过了就会 offer；调用时 compile/校验失败 fail-closed。
compileJsonSchema返回undefined时仍会放行输入。
降级
- 必选（browser / CU / settings / rive）：fail-closed。
- 可选 MCP：omit 后会话继续，空 group 丢掉，
onInvalidTool→reportError不抛。对那把坏工具是 fail-closed，对会话是 fail-open。可选能力这样是对的。 - 调度：非法参数在
accept()前拒绝。MCP + AI SDKjsonSchema()缺validate那条旧洞，这版 fallback 到 Ajv，测试用{}打 requiredquery能拦住。
CI（按 path filter）
期望：test（永远）、audit（动了 packages/runtime/package.json）、package（runtime-host-boot.ts + windows scripts）。不会跑 windows_sandbox_*。
test：completed / failure —format:check挂在scripts/package-windows-autoupdate-next.mjsaudit：completed / successpackage：仍 in_progress
旧的 jackwener REQUEST_CHANGES（bf667a1e）那两条——按单工具 decode、以及 jsonSchema() 无 validate 放行参数——在这个 head 上已经补上。本条是 COMMENT，不是 REQUEST_CHANGES。
NO-GO。 有未闭 [P2]，且 required test 不是 completed+success。未 approve，未合。
| // `extraMetadata` only changes the packaged app manifest, so temporarily | ||
| // update every source manifest for the build and restore them even when | ||
| // electron-builder fails. | ||
| const [originalRootManifest, originalDesktopManifest, originalCliManifest] = |
There was a problem hiding this comment.
[P2] 这四份 Windows autoupdate 脚本（package-windows-autoupdate-next.mjs / verify-windows-autoupdate.mjs / verify-windows-harness.test.mjs / verify-windows-x64.mjs）跟「隔离无效的可选 MCP 工具」不是一件事。这里会在 next-installer 构建期间把 root / desktop / CLI 三份 package.json 的 version 写盘，再在 finally 里还原；硬杀构建时 restore 走不到，工作树会停在被改过的 version 上。
同一处也是 required test 红灯的唯一原因：format:check 挂在这文件第 94 行。隔离逻辑本身没挂 format。所以不是「格式坏了顺手修一下就能合」——夹带的改动既超出了本 PR 声明的范围，又是门禁红的原因。
请把这四份 Windows 脚本从本 PR 摘出去，单独开 PR。不要只在这里 biome format 后继续带着它们。
| selectedOptionalTools.set(group, tools); | ||
| onInvalidTool?.( | ||
| new Error( | ||
| `Invalid optional Desktop capability tool omitted: ${group.offerId}/${tool.name}`, |
There was a problem hiding this comment.
[P3] 这里的判定单位是「整份 selected manifest 还能不能 decode」，不是「这把工具自己非法」。贪心前缀对 64/56KiB 是对的，非法工具也不会污染后面的兄弟。
但任何 decode 失败——包括第 65 把完全合法的工具、或只是把 manifest 顶过 56KiB——都会打成 Invalid optional Desktop capability tool omitted。文案会把预算溢出说成无效。
Host keyword allowlist 比 JSON Schema 窄（patternProperties / not / if/then/else / OpenAPI nullable），这些 MCP 工具会被 omit。相对 Host 这不是误判；相对 MCP 服务器会静默少工具。Ajv 仍只在 dispatch，不在 admission。
| /** Validate an input against a provider JSON Schema when the schema is compilable. */ | ||
| export function validateJsonSchemaInput(schema: unknown, input: unknown): unknown { | ||
| const validator = compileJsonSchema(schema); | ||
| if (!validator || validator(input)) return input; |
There was a problem hiding this comment.
[P3] if (!validator || validator(input)) return input：compileJsonSchema 对非 object / 数组 / null 返回 undefined 时，输入直接过。这是从 ai-sdk-backend 原样抽出来的。
admission 已经要求根 schema 是 object，所以已 offer 的 MCP 工具通常走不到这条。compile 抛错会被 parseToolArguments 包成非法参数，那条是 fail-closed。真正的洞只剩「编不出 validator 就放行」。Desktop 现在和 CodeMode 共用这条路径，收紧的话两边一起收。
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for tightening optional MCP capability isolation. I reviewed exact head 60c26e80b9779d8d729467b9a0fed6f28f6f8b15 and found one additional final-manifest budget edge that is separate from the current threads. The branch is currently conflicting and its exact-head test check is failing, so I have not treated it as merge-ready. Please do push back if services are guaranteed to have a separately reserved budget—the author may have protocol context that is not visible from this lane.
中文摘要
感谢收紧可选 MCP capability 的隔离。这里还有一个与现有线程不同的最终 manifest 预算边界：预筛只算 offers，而真实 replace 还会携带 services。当前分支冲突且 exact-head test 失败。若 services 有单独预留预算，也欢迎直接反驳并补充协议上下文。
AI-assisted review disclosure: Codex ran an independent analysis lane; Astro-Han independently verified the exact head, production path, and severity, and owns this review.
| const validateSelectedGroups = () => { | ||
| const selected = selectedGroups(); | ||
| if (selected.length === 0) return; | ||
| decodeClientCapabilityReplaceInput({ |
There was a problem hiding this comment.
[P2] Suggestion (category ① — normal MCP capability registration at the manifest-size boundary): could the optional-tool selector validate the exact payload that ClientCapabilityChannel.replace() will send? This preflight only includes registrationId + offers, while the real provider always adds the scheduled-task native service (and may add OAuth services). The protocol applies the 56 KiB limit to the combined decoded offers + services document. Therefore a selected offer set can pass here but cross the limit once production services are added; the final replace then still rejects the whole candidate and required Desktop capabilities fail to register, which is the global failure this PR is trying to isolate. I suggest passing the actual service descriptors into this bounded selection (or sharing one final-manifest builder) and adding an offers-only-pass / offers-plus-service-fail boundary regression. Please push back if another layer reserves those service bytes before this function runs.
|
@Astro-Han Thanks for pointing out the final-manifest budget boundary. I merged the latest apache/main and resolved the conflict in packages/runtime-host/src/protocol/index.ts. Since main had advanced the compatibility epoch to 105, this PR’s Client Capability tuple-schema change now uses epoch 106 while preserving the mainline history. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The budget boundary is closed correctly — you did more than your comment claims. prepareCapabilityGroups now runs the trial validation through decodeClientCapabilityReplaceInput with serviceOffers attached, which is exactly the payload a real replace carries, and the new test pins it with 32 padded services against a group that fits without them. The pre-check and the real call now measure the same thing. Doing the required groups first, before any optional tool is admitted, keeps required capabilities fail-closed; the comment says so and the code matches.
Moving serviceOffers above the group preparation and freezing it so services() returns the same array the budget was computed against is the right ordering.
P1 — the epoch needs renumbering again. The branch claims 106, but main is at 109 now (packages/runtime-host/src/protocol/index.ts), and #3368 is approved with 110. Please take the next free number when you rebase, and put the changelog entry after the newest one rather than at 106's position. Nothing else blocks this.
P3, no action needed unless it bothers you. The trial loop re-encodes the whole selection for every optional tool — selectedGroups() rebuilds each offer and decodeClientCapabilityReplaceInput re-validates the full manifest, plus a [...tools, tool] copy per iteration. That's quadratic in the number of optional tools. It runs once at provider construction and MCP tool counts are small, so it isn't worth restructuring now; I'm noting it only so it isn't a surprise if someone later points a large MCP server at this path.
Rebase onto current main, take the free epoch, and I'll approve.
0bec64e to
35b4100
Compare
|
@Astro-Han Addressed the remaining P1: rebased onto the current apache/main, advanced the compatibility epoch to 111 (main was 109 and #3368 uses 110), and moved the changelog entry after the newest epoch entry. Updated the affected fixtures/tests for the current dynamic/onDiagnostic capability API. Validation passes: root typecheck, Desktop typecheck, build:test, format:check, lint, and 30 focused Desktop Runtime Host capability/UDS tests. The P3 quadratic-construction note is understood and left unchanged as suggested. Please take another look and approve/merge when ready. |
35b4100 to
a5d77c9
Compare
|
@Astro-Han, I have addressed the review feedback and pushed the updated branch.
Verification passed:
Could you please take another look and merge PR #3366 when ready? Thank you! |
Keep the Desktop Runtime Host candidate usable when one optional MCP tool publishes an invalid schema, while retaining fail-closed behavior for required native capabilities. Cover AI SDK JSON Schema invocation and rate-limit terminal admission recovery. Generated-by: Codex
Validate optional MCP tools against the complete capability manifest, reuse Runtime JSON Schema validation before admission, and remove unrelated Runtime Host coverage. Generated-by: Codex
Use a temporary Desktop manifest version while building the Windows auto-update installer so the PE product version matches the update feed, then restore the source manifest. Generated-by: Codex
a5d77c9 to
f088ba6
Compare
3 participants
Summary
Refs #3302
An invalid optional MCP tool schema previously caused the entire Desktop Runtime Host capability candidate to fail. This PR now:
This PR intentionally scopes itself to the MCP capability-isolation portion of #3302.
Verification
npm --workspace @maka/runtime run build- passednpm --workspace @maka/desktop run build:main- passednpm --workspace @maka/runtime run typecheck- passednpm --workspace @maka/runtime-host run typecheck- passednpm run lint- passednpm run format:check- passednpm run typecheck/ Desktop typecheck currently fail on unrelated UI API mismatches (settledText,conversationKey,unlockAutoFollow, andtrailingAction).npx knip --workspace apps/desktopcurrently reports three existing unused PiP exports and onetaskkillconfiguration hint.AI use
Tool(s) and scope: Codex contributed to issue analysis, implementation, tests, code review, and PR wording. I reviewed the changes and remain the human contributor of record.
Checklist
Does this PR entail a change in behavior?