Repository navigation
feat(server): send structuredContent as JSON text when a tool omits content - #3153
mattzcarey wants to merge 1 commit into
Conversation
…ontent
A McpServer.registerTool result that carries structuredContent but no
content now gets a single text block holding the serialized JSON of
structuredContent, as the tools spec recommends ("a tool that returns
structured content SHOULD also return the serialized JSON in a
TextContent block"). Previously such a result went out with
content: []. A result that supplies content, even an empty array, is
sent unchanged.
To match, successful results of a tool with an outputSchema may omit
content in TypeScript.
Covered by an e2e requirement on every transport and both eras, and by
fixtures in the conformance everything-server for the
tools-call-structured-content scenario.
🦋 Changeset detectedLatest commit: 820f72a The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked how the new withStructuredContentText step interacts with the existing SEP-2106 projection in projectCallToolResult.
- Array and primitive
structuredContentdoes not get a second text block:appendTextFallbackForNonObjectskips results that already carry atextblock, and the helper runs first. - On the 2025 era the mirrored text holds the unwrapped value while
structuredContentis wrapped as{ result: ... }for non-object-root schemas. That is the same shape the existing auto-append already produces, so I did not treat it as a regression.
Extended reasoning...
The change adds a small helper to the McpServer tools/call path in packages/server/src/server/mcp.ts and makes content optional on ToolResultWithStructuredContent, with docs, a changeset, conformance fixtures and an e2e requirement alongside. It touches no auth, crypto or permission code, but it changes wire output for every tool result that has structuredContent and no content. I did not approve because the inline findings include a behaviour change in how mixed-family results are rejected and a public typing gap on RegisteredTool.update, and every changed path is covered by the repo-wide CODEOWNERS entry.
| [K in keyof CallToolResult as K extends 'content' | 'structuredContent' | 'isError' ? never : K]: CallToolResult[K]; | ||
| } & { | ||
| content: CallToolResult['content']; | ||
| content?: CallToolResult['content']; |
There was a problem hiding this comment.
🟡 (optional) Tool authors who swap a handler with RegisteredTool.update({ callback }) get a compile error for exactly the typed handlers this change tells them they can write. Making content optional at packages/server/src/server/mcp.ts:1401 widens the typed result, but update at mcp.ts:1526 still takes ToolCallback<StandardSchemaWithJSON>, whose result is plain CallToolResult with required content. Fix: make every place that accepts a tool callback accept the same result type, e.g. type update's callback to return ToolResultFor<...> with optional content, or state the exception in the docs.
Why this was flagged
Trigger: a handler typed with the documented annotation, e.g. const cb = (): ToolResultFor<typeof outputSchema> => ({ structuredContent: { data: 'x' } }), is passed to tool.update({ callback: cb }). update's callback is ToolCallback<StandardSchemaWithJSON> at packages/server/src/server/mcp.ts:1526; its Output parameter defaults to undefined, so ToolResultFor at mcp.ts:1421-1425 resolves to CallToolResult, where content is required. With content?: at mcp.ts:1401 the typed result is no longer assignable. On base the same annotated function had required content and was assignable, so the dismissal's "not touched by this diff" misses that the diff changes the source type. At runtime update stores the callback at mcp.ts:1002-1004 and the same tools/call path adds the text block, so only the type rejects it. docs/servers/tools.md:125 says "When outputSchema is set you may leave out content" with no exception for update. Remedy: widen the callback type at mcp.ts:1526.
Verification: nit — triggers when a handler whose return type is ToolResultFor<typeof outputSchema> is passed to RegisteredTool.update({ callback }). packages/server/src/server/mcp.ts:1526 still requires content, which line 1401 now makes optional, so the callback is rejected by update. It was assignable on base. No overload of update accepts the typed result.
Finding ID: f-4b14fb37
| if ((result as { content?: unknown }).content !== undefined || result.structuredContent === undefined) { | ||
| return result; | ||
| } | ||
| return { ...result, content: [{ type: 'text', text: JSON.stringify(result.structuredContent) }] }; |
There was a problem hiding this comment.
🟡 (optional) A tool result that omits content but carries structuredContent plus a task, inputRequests or requestState key is now sent as a normal success instead of being rejected. withStructuredContentText adds content before normalizeContentlessToolResult runs (server.ts:576), and that guard only fires while content === undefined. Fix: leave the result untouched when any key in TOOL_RESULT_FOREIGN_FAMILY_KEYS is present, so the existing loud rejection still applies on both eras.
Why this was flagged
Trigger: a registerTool handler returns { structuredContent: {...}, task: {...} } with no content, arriving through the tools/call handler at packages/server/src/server/mcp.ts:310. withStructuredContentText only checks content !== undefined and structuredContent === undefined, so it adds a text block. The foreign-family guards at packages/core-internal/src/wire/resultFamilies.ts:18-19 and packages/core-internal/src/wire/rev2025-11-25/buildSchemas.ts:2245-2259 both return early once content is defined. On the base branch the same result stays content-less and fails with "Invalid tools/call result: content is required when the body carries 'task' ..."; after the change the client receives a success with a text block. The paragraph this PR edits at skills/mcp-typescript-sdk-upgrade-to-v2/references/behavioral-changes.md:204-206 still says such a body "is still rejected loudly".
Verification: Triggers when a registerTool handler returns structuredContent together with a task, inputRequests or requestState key and no content. packages/server/src/server/mcp.ts:1343-1347: withStructuredContentText never looks at foreign-family keys. normalizeContentlessToolResult is a no-op once content !== undefined. A loud -32602 turns into a success that carries a stray key.
Finding ID: f-b052db8b
| `McpServer` gives such a result a JSON text block of its `structuredContent`; any other | ||
| content-less handler result is normalized to `content: []` before it reaches the |
There was a problem hiding this comment.
🟡 nit (optional): Two new statements about content-less tool results say something the code does not do, so upgrade-skill readers get wrong expectations. withStructuredContentText (mcp.ts:1343) is not gated on outputSchema or isError, so a schema-less or error result with structuredContent also gets the text block, not content: []. Fix: make the words match the helper and the codec, or gate the helper on successful outputSchema results, which covers the 2 sites listed. Same instruction at 2 sites (behavioral-changes.md:203, test/e2e/requirements.ts:426). The second says content: [] is sent unchanged, but textFallback.ts:22 appends text for array values. [also at: skills/mcp-typescript-sdk-upgrade-to-v2/references/behavioral-changes.md:204 - nit: Readers of the v2 upgrade guide are told a content-less result outside the typed outputSchema case goes out as content: [], which is no longer true.; +2 more]
Why this was flagged
A JavaScript or cast handler on a tool with no outputSchema returns { structuredContent: { a: 1 } } with no content, through tools/call at packages/server/src/server/mcp.ts:310. withStructuredContentText (mcp.ts:1343-1347) checks only content !== undefined and structuredContent === undefined, so it adds [{type:'text', text:'{"a":1}'}]; the same happens for an isError: true result carrying structuredContent. behavioral-changes.md:202-203 says only successful outputSchema results get the text block and "any other content-less handler result is normalized to content: []". Separately, test/e2e/requirements.ts:426 says a handler that supplies content, "even an empty array, is sent unchanged"; with an array or primitive structuredContent and content: [], appendTextFallbackForNonObject (packages/core-internal/src/wire/textFallback.ts:22) still appends a text block. On the base the skill text matched the code. No safeguard applies: these are prose claims, and the new tests only cover object values with content: [].
Verification: nit — triggers when a handler that TypeScript would not allow to omit content (JavaScript or a cast) returns structuredContent without content from a schema-less tool, or with isError: true. packages/server/src/server/mcp.ts:1343-1348 — withStructuredContentText never looks at outputSchema or isError.
Finding ID: f-05cb9ce6
| // Structured content (tools-call-structured-content scenario). The handlers | ||
| // omit `content`: McpServer sends the serialized JSON of structuredContent | ||
| // as a text block, which is what the scenario's text-mirror check verifies. |
There was a problem hiding this comment.
🟡 nit (optional): Two comments added here run against the repo's brief-comments principle. The inline comment at test/conformance/src/everythingServer.ts:748-750 spans three lines, and the module-private helper withStructuredContentText carries a JSDoc block at packages/server/src/server/mcp.ts:1342. Fix: cut the inline comment to one line and turn the private helper's JSDoc into a one-line // comment, which covers the 2 sites listed. Same instruction at 2 sites (test/conformance/src/everythingServer.ts:750, packages/server/src/server/mcp.ts:1342). [also at: test/conformance/src/everythingServer.ts:750 - nit: CLAUDE.md principle 8 (inline comments one line, JSDoc only on the public API): the added comment above test_structured_content runs three lines and explains why the handlers omit content, and packages/server/src/server/mcp.ts adds a JSDoc block to the non-exported helper withStructuredContentText.]
Why this was flagged
No runtime input triggers this; it is a convention check on comments the diff adds. The comment at test/conformance/src/everythingServer.ts:748-750 is a three-line inline comment explaining why the handlers omit content. The /** ... */ block at packages/server/src/server/mcp.ts:1342 documents withStructuredContentText, which is declared with plain function and is not exported. The instruction asks for one-line inline comments and JSDoc only on the public API, with the reasoning kept in the commit message or PR description. Neither line existed on the base branch. No lint rule enforces this, so nothing else catches it.
Verification: Triggering condition: none at runtime; this is a violation of a written repository instruction by comments the diff adds. test/conformance/src/everythingServer.ts:748-750 is a three-line // inline comment. packages/server/src/server/mcp.ts:1342 is a /** ... */ JSDoc block on withStructuredContentText, declared at line 1343 as a plain function with no export.
Finding ID: f-22701457
A
McpServer.registerToolresult that hasstructuredContentbut nocontentnow gets one text block holding the serialized JSON ofstructuredContent. It used to go out withcontent: []. A result that suppliescontent, even an empty array, is unchanged, and successful results of a tool with anoutputSchemamay now omitcontentin TypeScript.Why:
contentstill see the result.content: []gave them nothing.structuredContent(SEP-2106); object results were the gap.Stacked on #2449 (types), which makes
structuredContenttype-checked againstoutputSchema; review that first. Covered by a new e2e requirement on every transport and both eras, and by fixtures in the conformance everything-server for a newtools-call-structured-contentconformance scenario.