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}
| * @param value - The value to check. | ||
| * | ||
| * @returns True if the value is a valid {@linkcode CallToolResult}, false otherwise. | ||
| */ | ||
| export const isCallToolResult = (value: unknown): value is CallToolResult => CallToolResultSchema.safeParse(value).success; | ||
|
|
||
| /** |
There was a problem hiding this comment.
🔴 The isCallToolResult guard has unsound type narrowing: it returns true for {} because CallToolResultSchema.content uses .default([]), so Zod fills in content during parsing but the guard narrows the original input value not the parsed output. After the guard passes, TypeScript believes value.content is ContentBlock[] while at runtime it is undefined, causing TypeError on any .map() or .length access with no TypeScript warning. Fix by using z.input for the narrowed type, or making content optional in the type predicate signature.
Extended reasoning...
What the bug is and how it manifests
isCallToolResult is implemented as CallToolResultSchema.safeParse(value).success. The CallToolResult TypeScript type is Flatten<z.infer>, which uses Zod output type inference. Because CallToolResultSchema.content is defined as z.array(ContentBlockSchema).default([]), the output type has content: ContentBlock[] as non-optional, since the default guarantees it is always present in the parsed result. The guard therefore declares value is CallToolResult, asserting content is ContentBlock[].
The specific code path that triggers it
When value = {} is passed: (1) CallToolResultSchema.safeParse({}) returns { success: true, data: { content: [] } } -- the default is applied to the parsed output object, not to the original value. (2) The guard returns true. (3) TypeScript narrows value to CallToolResult, reporting value.content as ContentBlock[]. (4) At runtime, value is still {}, so value.content is undefined.
Why existing code does not prevent it
The test file documents this behavior with it('returns true for a minimal valid result', () => { expect(isCallToolResult({})).toBe(true); }). The test passes, but it does not verify what TypeScript infers about value.content after the guard. The other guards in this file (isJSONRPCResultResponse, isJSONRPCErrorResponse) do not share this problem because their schemas do not have fields with .default() on non-optional properties -- their required fields must be present in the input.
Impact
Any code that accesses value.content after the guard without a null-check will throw a TypeError at runtime despite TypeScript reporting no type error. This is a silent regression that violates TypeScript type safety. Example: if (isCallToolResult(result)) { result.content.forEach(block => console.log(block)); } throws TypeError: Cannot read properties of undefined.
Step-by-step proof
- const value: unknown = {}. 2. isCallToolResult(value) returns true (Zod applies content:[] default in parsed output). 3. TypeScript narrows value to CallToolResult, so value.content is typed as ContentBlock[]. 4. value.content.length -- TypeScript: no error. Runtime: TypeError: Cannot read properties of undefined (reading 'length').
How to fix
Option 1 (preferred): Change the type predicate to use the Zod INPUT type so content is optional in the narrowed type: export const isCallToolResult = (value: unknown): value is z.input => CallToolResultSchema.safeParse(value).success. Option 2: Return the safeParsed data output to callers instead of narrowing the original value. Option 3: Annotate the narrowed type with content?: ContentBlock[] to reflect that the original input may not have the field.
| import { CallToolResultSchema } from '@modelcontextprotocol/sdk/types.js'; | ||
| if (CallToolResultSchema.safeParse(value).success) { /* ... */ } | ||
|
|
||
| // v2: use the type guard | ||
| import { isCallToolResult } from '@modelcontextprotocol/server'; | ||
| if (isCallToolResult(value)) { /* ... */ } | ||
| ``` | ||
|
|
||
| ### Client list methods return empty results for missing capabilities | ||
|
|
There was a problem hiding this comment.
🟡 The isCallToolResult migration example in docs/migration.md (line 453) imports from @modelcontextprotocol/server, but the surrounding context is migrating client.callTool() usage -- a purely client-side concern. Since isCallToolResult is re-exported by both packages, a client-only user following this example would add an unnecessary server dependency. The import should use @modelcontextprotocol/client to match the client-focused context.
Extended reasoning...
What the bug is and how it manifests
In docs/migration.md, the code example for the isCallToolResult migration (lines 449-458) shows import { isCallToolResult } from '@modelcontextprotocol/server'. This example appears directly within the section documenting migration of client.callTool() -- a client-side API. The import recommends the server package to client-only users.
The specific code path that triggers it
The problematic example sits in the section titled 'Protocol.request(), ctx.mcpReq.send(), and Client.callTool() no longer take a schema parameter'. The preceding examples all show client.callTool() usage. The isCallToolResult guard is presented as the replacement for CallToolResultSchema.safeParse() when validating results returned by client.callTool() -- definitively a client-side operation.
Why existing code does not prevent it
isCallToolResult lives in @modelcontextprotocol/core/public/index.ts and is re-exported by BOTH packages/client/src/index.ts and packages/server/src/index.ts via export * from '@modelcontextprotocol/core/public'. Neither package exclusively owns the symbol, so there is no compile-time or lint-time signal that the wrong package is being cited in the docs.
What the impact would be
A developer who only has @modelcontextprotocol/client installed (the common case for tool-call consumers) will either (a) add an unnecessary @modelcontextprotocol/server dependency that brings in transitive server-side code, or (b) be confused when they cannot find the import in their existing dependencies. The docs/migration-SKILL.md companion document does not include a concrete import path for isCallToolResult, so migration.md is the primary source a migrating user will rely on.
How to fix it
Change line 453 from import { isCallToolResult } from '@modelcontextprotocol/server' to import { isCallToolResult } from '@modelcontextprotocol/client'. This aligns with the client-side context of the surrounding client.callTool() examples and with section 3 of the same document, which already advises importing from whichever package you depend on.
Step-by-step proof
- Developer has @modelcontextprotocol/client installed; they do NOT have @modelcontextprotocol/server.
- They migrate their client.callTool() code and reach the isCallToolResult example in migration.md.
- They copy import { isCallToolResult } from '@modelcontextprotocol/server' verbatim.
- Their bundler/TypeScript compiler reports an unresolved module error for @modelcontextprotocol/server.
- To silence the error, they run npm install @modelcontextprotocol/server -- an entirely unnecessary dependency for a client-only project.
- Alternatively, they waste time searching for why @modelcontextprotocol/client does not export isCallToolResult (it does -- the doc just points them to the wrong package).
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.
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