Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
isJSONRPCResponse looks right.
isCallToolResult I think is missing something: CallToolResultSchema.content has .default([]) (schemas.ts:1374), so isCallToolResult({}) returns true but the original {} still has no .content — v.content.map(...) throws after the guard passes. Test at line 82 locks this in. The spec has content required.
Two ways to fix:
Option 1 — guard-level check (probably the right scope here):
export function isCallToolResult(v: unknown): v is CallToolResult {
if (typeof v !== 'object' || v === null || !('content' in v)) return false;
return CallToolResultSchema.safeParse(v).success;
}Plus flip the test: expect(isCallToolResult({})).toBe(false).
Option 2 — drop .default([]) from the schema:
// schemas.ts:1374
content: z.array(ContentBlockSchema), // was: .default([])Aligns Zod/TS/spec, fixes the guard for free. The .default([]) predates v2 (carried over from v1's types.ts when #1680 reorganized exports), so it's not a recent design decision we'd be unwinding. Would force tool authors returning only structuredContent to write content: [] explicitly. Maybe a follow-up if we decide that's fine; Option 1 is the safer scope for this PR.
Nit: migration.md example imports from /server in a client context, probably want /client.
Tks, completely missed that this sets a default. Went with Option 1 for now, as I think Option 2 would require more considerations and other SDKs do it in one way or another. We could do this in a follow-up indeed and see how far back is this compatibility for. |
| */ | ||
| export const isCallToolResult = (value: unknown): value is CallToolResult => { | ||
| if (typeof value !== 'object' || value === null || !('content' in value)) return false; | ||
| return CallToolResultSchema.safeParse(value).success; | ||
| }; | ||
|
|
||
| /** | ||
| * Checks if a value is a valid {@linkcode TaskAugmentedRequestParams}. | ||
| * @param value - The value to check. |
There was a problem hiding this comment.
🔴 The isCallToolResult guard has two opposing defects stemming from the same root cause — the pre-flight check \!('content' in value) combined with using Zod's output type (z.infer) as the predicate target instead of the input type (z.input). First, a false negative: isCallToolResult({ structuredContent: { key: 'value' } }) returns false because the pre-flight rejects objects lacking a content key, but CallToolResultSchema.safeParse({ structuredContent: { key: 'value' } }).success is true (Zod applies the .default([]) for content), so the guard contradicts the schema it wraps and will incorrectly reject valid structuredContent-only tool results from MCP-spec-conformant servers. Second, a residual unsound narrowing: isCallToolResult({ content: undefined }) returns true (because 'content' in value is true even when the value is undefined), TypeScript then narrows value.content to ContentBlock[] (non-optional), but at runtime value.content is still undefined, so any post-guard access like value.content.forEach(...) throws a TypeError with no TypeScript warning. The fix is to remove the pre-flight check and use z.input<typeof CallToolResultSchema> as the type predicate, which correctly makes content optional in the narrowed type and aligns the guard's semantics with the schema.
Extended reasoning...
What the bug is and how it manifests
isCallToolResult was added in this PR to let callers validate tool call results at runtime without accessing the now-internal Zod schemas. The implementation adds a pre-flight check \!('content' in value) before delegating to CallToolResultSchema.safeParse(value).success. This creates two opposing defects:
- False negative: Objects where
contentis absent (e.g.,{ structuredContent: { result: 42 } }) are rejected by the pre-flight check and never reachsafeParse, even though the schema accepts them (becausecontentis declared with.default([])). - Unsound narrowing: Objects where
contentis present butundefined(e.g.,{ content: undefined }) pass the pre-flight check ('content' in { content: undefined }istruein JavaScript),safeParsereturns success (Zod treatsundefinedas missing and applies the default), the guard returnstrue, but TypeScript narrowsvalue.contenttoContentBlock[]while at runtime it is stillundefined.
The specific code path that triggers each defect
False negative — isCallToolResult({ structuredContent: { key: 'value' } }):
typeof value === 'object' && value \!== nullpasses.\!('content' in value)—'content' in { structuredContent: ... }isfalse, so\!falseistrue— returnsfalseimmediately.CallToolResultSchema.safeParse({ structuredContent: { key: 'value' } }).successistrue(Zod appliescontentdefault[]).- Guard result (
false) contradicts schema result (true).
Unsound narrowing — isCallToolResult({ content: undefined }):
- Pre-flight passes because the
contentkey exists in the object. CallToolResultSchema.safeParse({ content: undefined })returns{ success: true, data: { content: [] } }(Zod applies the default to the parsed output, not the original value).- Guard returns
true; TypeScript narrowsvaluetoCallToolResultwithcontent: ContentBlock[](required, becausez.inferof a schema with.defaultproduces a non-optional field in the output type). - At runtime,
value.contentis stillundefined.value.content.forEach(...)throwsTypeError.
Why existing checks do not prevent these
The test suite covers { content: [], structuredContent: { key: 'value' } } (content explicitly present) and {} (empty object), but does NOT test the structuredContent-only case { structuredContent: { key: 'value' } }. The root cause is that value is CallToolResult uses z.infer<typeof CallToolResultSchema> (the Zod output type), which has content: ContentBlock[] as required (because .default guarantees it exists in the output). The guard operates on the input value, where content can be absent or undefined.
Regarding the refutation that { content: undefined } is implausible: this is true for JSON-derived inputs. However, this does not invalidate the false negative (bug_001), which is independently real and practically significant.
Impact
The false negative is the more consequential defect. The MCP spec allows tools that declare an outputSchema to return only structuredContent without content in the wire payload. The backwards-compatibility rule "content is always present" is guidance for server implementations, not a Zod validation constraint — the schema explicitly uses .default([]), making content optional in the INPUT type. Any client code that gates downstream processing on isCallToolResult will silently fail to recognize these valid responses.
Step-by-step proof of the false negative
const v: unknown = { structuredContent: { result: 42 } };
console.log(isCallToolResult(v)); // false (incorrect)
console.log(CallToolResultSchema.safeParse(v).success); // true
// data = { content: [], structuredContent: { result: 42 } }How to fix
Remove the pre-flight \!('content' in value) check and change the type predicate to z.input<typeof CallToolResultSchema>:
export const isCallToolResult = (value: unknown): value is z.input<typeof CallToolResultSchema> =>
CallToolResultSchema.safeParse(value).success;z.input makes content optional (content?: ContentBlock[]) in the narrowed type, which accurately reflects the wire shape and forces callers to handle the absent/empty case. The guard then perfectly tracks CallToolResultSchema.safeParse(value).success with no false negatives and no unsound narrowing.
|
|
||
| ## 12. Experimental: `TaskCreationParams.ttl` no longer accepts `null` | ||
|
|
||
| `TaskCreationParams.ttl` changed from `z.union([z.number(), z.null()]).optional()` to `z.number().optional()`. Per the MCP spec, `null` TTL (unlimited lifetime) is only valid in server responses (`Task.ttl`), not in client requests. Omit `ttl` to let the server decide. |
There was a problem hiding this comment.
🟡 The migration table in docs/migration-SKILL.md (and the code example in docs/migration.md) presents isCallToolResult(value) as a direct behavioral replacement for CallToolResultSchema.safeParse(value).success, but the two diverge for inputs that lack a content key (e.g., {} or { structuredContent: {...} }) due to the guard's pre-flight \!('content' in value) check. Any v1 code that relied on Zod's content: [] default to accept objects without an explicit content field will silently stop entering the if-branch after following this migration guide.
Extended reasoning...
What the bug is
The migration table maps CallToolResultSchema.safeParse(value).success → isCallToolResult(value) as if they are semantically equivalent. They are not, because isCallToolResult adds a pre-flight guard that safeParse does not have:
export const isCallToolResult = (value: unknown): value is CallToolResult => {
if (typeof value \!== 'object' || value === null || \!('content' in value)) return false;
return CallToolResultSchema.safeParse(value).success;
};The \!('content' in value) check causes the guard to return false for any object that lacks a content property — even if Zod would have accepted it via its content: z.array(...).default([]) default.
The specific code path that triggers it
For value = {}:
CallToolResultSchema.safeParse({}).success→true(Zod applies thedefault([])and yields{ content: [] })isCallToolResult({})→false(pre-flight\!('content' in {})istrue, guard short-circuits)
The test suite itself documents this explicitly: 'returns false for an empty object (content is required)'. The divergence was intentional — the pre-check prevents unsound type narrowing where value.content would be typed as ContentBlock[] at compile-time but be undefined at runtime. That design decision is sound, but it means the guard is stricter than safeParse, and the migration docs should say so.
Why existing code doesn't prevent it
The migration guide presents the two forms in a plain equivalence table with no qualification. There is no note warning that inputs lacking a content key behave differently. A reader will reasonably conclude they are semantically equivalent.
Impact
A v1 user whose code looked like:
// v1: passes for arbitrary external data that has no 'content' key
if (CallToolResultSchema.safeParse(rawExternalData).success) { ... }After following the guide and switching to isCallToolResult(rawExternalData), the branch is silently skipped for any rawExternalData object that lacks a content field. No TypeScript error is produced. In practice, well-formed MCP protocol values always include content, so the real-world risk is low — but it is still a documentation inaccuracy that could catch edge-case users.
Step-by-step proof
- v1:
CallToolResultSchema.safeParse({}).success === true(Zod fillscontent: []) - v2:
isCallToolResult({}) === false(guard's\!('content' in {})fires) - Code that previously entered the if-branch no longer does — no compiler warning, no runtime exception.
- Same divergence occurs for
{ structuredContent: { key: 'value' } }:safeParseaccepts it (Zod supplies default content),isCallToolResultrejects it (nocontentkey).
How to fix
Add a note to the migration table clarifying that isCallToolResult is stricter than raw safeParse: it requires content to be present as a property in the input object, whereas the Zod schema accepts objects without it by applying the default([]). Something like:
Note:
isCallToolResultadditionally requirescontentto be present as a key in the input; objects that satisfy the schema only via Zod'scontent: []default (e.g.,{}) will returnfalse. In practice this only matters for data not originating from the MCP protocol.
| | `JSONRPCError` | `JSONRPCErrorResponse` | | ||
| | `JSONRPCErrorSchema` | `JSONRPCErrorResponseSchema` | | ||
| | `isJSONRPCError` | `isJSONRPCErrorResponse` | | ||
| | `isJSONRPCResponse` | `isJSONRPCResultResponse` | | ||
| | `isJSONRPCResponse` | `isJSONRPCResultResponse` (see note below) | | ||
| | `ResourceReferenceSchema` | `ResourceTemplateReferenceSchema` | | ||
| | `ResourceReference` | `ResourceTemplateReference` | | ||
| | `IsomorphicHeaders` | Use Web Standard `Headers` | | ||
| | `AuthInfo` (from `server/auth/types.js`) | `AuthInfo` (now re-exported by `@modelcontextprotocol/client` and `@modelcontextprotocol/server`) | | ||
|
|
||
| All other types and schemas exported from `@modelcontextprotocol/sdk/types.js` retain their original names — import them from `@modelcontextprotocol/client` or `@modelcontextprotocol/server`. | ||
|
|
||
| > **Note on `isJSONRPCResponse`:** v1's `isJSONRPCResponse` was a deprecated alias that only checked for *result* responses (it was equivalent to `isJSONRPCResultResponse`). v2 removes the deprecated alias and introduces a **new** `isJSONRPCResponse` with corrected semantics — it checks for *any* response (either result or error). If you are migrating v1 code that used `isJSONRPCResponse`, rename it to `isJSONRPCResultResponse` to preserve the original behavior. Use the new `isJSONRPCResponse` only when you want to match both result and error responses. | ||
|
|
||
| **Before (v1):** | ||
|
|
||
| ```typescript |
There was a problem hiding this comment.
🟡 The closing sentence in migration.md's 'Removed type aliases' section still says "All other types and schemas exported from @modelcontextprotocol/sdk/types.js retain their original names — import them from @modelcontextprotocol/client or @modelcontextprotocol/server", where the word "schemas" implies Zod schemas like CallToolResultSchema are still importable from the v2 packages. This PR updated migration-SKILL.md to explicitly state Zod schemas are no longer public API, but left migration.md unchanged, creating a direct contradiction between the two official migration guides. Fix by changing "types and schemas" to "types" and adding a note that Zod schemas are now internal.
Extended reasoning...
What the bug is and how it manifests
This PR updates migration-SKILL.md line 100 to clearly distinguish between TypeScript type symbols and Zod schemas: "All other type symbols from @modelcontextprotocol/sdk/types.js retain their original names. Zod schemas (e.g., CallToolResultSchema, ListToolsResultSchema) are no longer part of the public API — they are internal to the SDK." However, migration.md line ~503 (in the 'Removed type aliases and deprecated exports' section) was not updated and still reads: "All other types and schemas exported from @modelcontextprotocol/sdk/types.js retain their original names — import them from @modelcontextprotocol/client or @modelcontextprotocol/server." The word "schemas" combined with the directive to import from the v2 packages directly implies Zod schemas like CallToolResultSchema are still publicly available.
The specific code path that triggers it
A developer migrating from v1 to v2 reads migration.md (the primary human-readable migration guide). They reach the 'Removed type aliases' section and see the closing sentence promising that "types and schemas" retain their names and can be imported from @modelcontextprotocol/client or @modelcontextprotocol/server. They then write import { CallToolResultSchema } from '@modelcontextprotocol/client' — which fails at build time with a module-not-found / missing-export error because Zod schemas are not part of the v2 public API.
Why existing code doesn't prevent it
The PR correctly updated migration-SKILL.md but overlooked the corresponding sentence in migration.md. Although migration.md section 11 separately advises removing schema imports when they were only used as request() arguments, the blanket closing statement in the 'Removed type aliases' section overrides that guidance by implying schemas are still importable for other purposes. Verification confirms that neither @modelcontextprotocol/client nor @modelcontextprotocol/server export CallToolResultSchema or other Zod schemas in their index.ts files.
What the impact would be
A developer following only migration.md — the primary migration guide — would confidently conclude that CallToolResultSchema is still available and spend time debugging a build-time module-not-found error, or file a bug report assuming the SDK is broken. The two official migration documents now contradict each other on a significant v2 behavioral change (Zod schemas becoming internal), which undermines developer trust in the documentation.
Step-by-step proof
- Developer reads
migration.md'Removed type aliases' section. - They see: "All other types and schemas exported from
@modelcontextprotocol/sdk/types.jsretain their original names — import them from@modelcontextprotocol/clientor@modelcontextprotocol/server." - They write:
import { CallToolResultSchema } from '@modelcontextprotocol/client'; - Build fails:
Module '@modelcontextprotocol/client' has no exported member 'CallToolResultSchema'. - If they instead read
migration-SKILL.md, they would have seen the correct warning: "Zod schemas are no longer part of the public API."
How to fix
Change line ~503 in migration.md from:
All other types and schemas exported from `@modelcontextprotocol/sdk/types.js` retain their original names — import them from `@modelcontextprotocol/client` or `@modelcontextprotocol/server`.
To match the wording in migration-SKILL.md:
All other **type** symbols from `@modelcontextprotocol/sdk/types.js` retain their original names — import them from `@modelcontextprotocol/client` or `@modelcontextprotocol/server`. **Zod schemas** (e.g., `CallToolResultSchema`, `ListToolsResultSchema`) are no longer part of the public API — they are internal to the SDK. For runtime validation, use type guard functions like `isCallToolResult` instead.
2 participants
Add
isJSONRPCResponseandisCallToolResulttype guard methods to the public API.Motivation and Context
Two gaps in the v2 public API were identified during migration testing:
isJSONRPCResponse— v1 had a deprecatedisJSONRPCResponsethat was a misnamed alias for result-only checking. v2 dropped it but didn't replace it. Users who need to check whether a value is any JSON-RPC response (result or error) had to combine two guards:isJSONRPCResultResponse(v) || isJSONRPCErrorResponse(v). The newisJSONRPCResponseguard validates against the unionJSONRPCResponseSchemadirectly, with corrected semantics.isCallToolResult— In v1, users could importCallToolResultSchemaand call.safeParse()to validate tool call results at runtime. In v2 the Zod schemas are no longer part of the public API, leaving no way to perform this validation. The newisCallToolResultguard fills this gap.How Has This Been Tested?
New test file
packages/core/src/types/guards.test.tswith tests covering:isJSONRPCResponseisJSONRPCResultResponse || isJSONRPCErrorResponseacross a range of inputsCallToolResultshapes (minimal, with content, with isError, with structuredContent)All 488 tests pass, typecheck and lint are clean.
Breaking Changes
None. This is a purely additive change.
Types of changes
Checklist
Additional context
Files changed:
packages/core/src/types/guards.ts— AddedisJSONRPCResponseandisCallToolResulttype guards with JSDocpackages/core/src/exports/public/index.ts— Exported both new guardspackages/core/src/types/guards.test.ts— New test filedocs/migration.md— Added note clarifying v1's deprecatedisJSONRPCResponsevs v2's new one; addedisCallToolResultmigration exampledocs/migration-SKILL.md— Updated rename table, corrected claim about schema exports, addedisCallToolResultmapping table