Skip to content

feat(mcp): bound successful tool results - #4916

Merged
Astro-Han merged 1 commit into
apache:mainfrom
liuxiaocs7:feat/bound-mcp-tool-results
Sep 7, 2026
Merged

Astro-Han merged 1 commit into
apache:mainfrom
liuxiaocs7:feat/bound-mcp-tool-results

Conversation

@liuxiaocs7

Copy link
Copy Markdown
Member

Summary

Bound successful MCP Tool Results at the shared McpClientManager.callTool() boundary before they can reach Runtime, IPC, or transcript consumers. Results now fail closed above 256 content blocks, 24 MiB of serialized UTF-8 JSON, depth 32, or 8,192 JSON nodes.

The guard performs an incremental traversal without constructing another full JSON string, rejects non-JSON/cyclic/custom-serializer values, checks raw block metadata conservatively, and rechecks the normalized credential-scrubbed result. Error Tool Results retain their existing bounded-summary path.

Fixes #4915

Verification

  • npm --workspace @maka/core run build
  • npm --workspace @maka/mcp run build
  • npm --workspace @maka/mcp run typecheck
  • npm --workspace @maka/mcp run test:dist — 186 passed
  • npm --workspace @maka/runtime run build
  • npx biome lint packages/mcp/src/index.ts packages/mcp/src/__tests__/manager.test.ts
  • npx biome format packages/mcp/src/index.ts packages/mcp/src/__tests__/manager.test.ts
  • git diff --check

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex implemented and tested the MCP Tool Result budget guard and assisted with review.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — successful MCP Tool Results exceeding the documented structural budgets now fail closed.
  • No

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 6, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 6db8be44c3b04b4bda89c24a6a44f8ae3fd20b2c.

I found no P0–P3 correctness, protocol, resource-boundary, or secret-handling issue in the two-file diff. packages/mcp/src/index.ts:863-890,2401-2551 bounds successful results before output-schema validation and again after normalization and credential scrubbing, covering content blocks, JSON bytes/depth/nodes, cycles, unsupported values, and scrub expansion. The new tests exercise each limit and the validation-order boundary.

The exact-head hosted label and test checks passed. I did not independently run the MCP typecheck or E2E tests because this checkout has no usable local TypeScript executable, and I did not run a separate live untrusted-server smoke test.

Automated review notice: This is an AI-assisted review and does not replace independent human review.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change. Reviewed exact head 6db8be44c3b04b4bda89c24a6a44f8ae3fd20b2c: no remaining P0–P3 findings after two independent AI reviews and coordinating review.

The guard belongs at the shared MCP manager boundary, checks raw content before validation and the normalized/scrubbed result before publication, and preserves the existing bounded error-result path. Independent verification passed 73 focused manager/transport tests, including real HTTP paths; exact-head CI is green. This approval covers the result-publication budget, not a new transport-ingress limit.

AI-assisted review, approved with the user's authorization; not independently human-verified.

中文

感谢修改。已复核精确 head 6db8be4,经两位 AI reviewer 和主审交叉核对,没有剩余 P0–P3 问题。

预算检查位于共享 MCP manager 边界,在校验前检查原始内容、发布前复查规范化和脱敏结果,并保留现有错误结果摘要路径。独立验证的 73 项 manager/transport 定向测试通过,包括真实 HTTP 路径;当前 head 的 CI 已通过。本批准覆盖结果发布预算,不代表新增了传输入口限额。

这是经用户授权提交的 AI 辅助评审,未经独立人工验证。

@Astro-Han
Astro-Han merged commit d6641fc into apache:main Sep 7, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(mcp): bound successful tool results

2 participants