Skip to content

Commit 4ebd2b8

Browse files
committed
fix(client): isolate per-tool outputSchema compile failures (PR #2249 review)
Addresses claude-bot review on PR #2249: assertSchemaSafeToCompile (SEP-2106 SSRF / composition-DoS guard) throws inside getValidator(), and cacheToolMetadata() eagerly compiled every advertised tool's outputSchema with no per-tool error handling. A single tool advertising a non-local $ref or an over-budget schema therefore rejected the entire listTools() call, leaving the client unable to list or call ANY tool from that server. - catch compilation per-tool in cacheToolMetadata(); store the error in a new _toolOutputValidatorErrors map instead of letting it propagate. - surface the scoped error from callTool() only when the offending tool is actually called (clear, descriptive ProtocolError). - integration test: one tool with a non-local $ref outputSchema does not break listTools() or the use of a sibling good tool; the bad tool errors only on call. Note: the second review comment (experimental/tasks callToolStream truthiness) is stale \u2014 the tasks feature was removed in c8d7401 before this branch, no callToolStream exists, and no truthiness structuredContent checks remain.
1 parent 5e39b25 commit 4ebd2b8

2 files changed

Lines changed: 82 additions & 3 deletions

File tree

packages/client/src/client/client.ts

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,13 @@ export class Client extends Protocol<ClientContext> {
224224
private _instructions?: string;
225225
private _jsonSchemaValidator: jsonSchemaValidator;
226226
private _cachedToolOutputValidators: Map<string, JsonSchemaValidator<unknown>> = new Map();
227+
/**
228+
* Tools whose advertised `outputSchema` could not be compiled into a validator (e.g. it tripped
229+
* the SEP-2106 safety guards — a non-local `$ref` or an over-budget schema). The error is stored
230+
* per-tool and surfaced only when that tool is called, so one malformed tool definition does not
231+
* break `listTools()` or the use of every other tool from the same server.
232+
*/
233+
private _toolOutputValidatorErrors: Map<string, Error> = new Map();
227234
private _listChangedDebounceTimers: Map<string, ReturnType<typeof setTimeout>> = new Map();
228235
private _pendingListChangedConfig?: ListChangedHandlers;
229236
private _enforceStrictCapabilities: boolean;
@@ -812,6 +819,17 @@ export class Client extends Protocol<ClientContext> {
812819
async callTool(params: CallToolRequest['params'], options?: RequestOptions): Promise<CallToolResult> {
813820
const result = await this._requestWithSchema({ method: 'tools/call', params }, CallToolResultSchema, options);
814821

822+
// If the tool advertised an outputSchema that failed to compile (e.g. a SEP-2106 safety-guard
823+
// rejection), surface that error now — scoped to this tool — rather than silently skipping
824+
// output validation.
825+
const validatorError = this._toolOutputValidatorErrors.get(params.name);
826+
if (validatorError) {
827+
throw new ProtocolError(
828+
ProtocolErrorCode.InvalidParams,
829+
`Tool ${params.name} has an output schema that could not be compiled: ${validatorError.message}`
830+
);
831+
}
832+
815833
// Check if the tool has an outputSchema
816834
const validator = this.getToolOutputValidator(params.name);
817835
if (validator) {
@@ -858,12 +876,19 @@ export class Client extends Protocol<ClientContext> {
858876
*/
859877
private cacheToolMetadata(tools: Tool[]): void {
860878
this._cachedToolOutputValidators.clear();
879+
this._toolOutputValidatorErrors.clear();
861880

862881
for (const tool of tools) {
863-
// If the tool has an outputSchema, create and cache the validator
882+
// If the tool has an outputSchema, create and cache the validator. Compilation can throw
883+
// (invalid schema, or a SEP-2106 safety-guard rejection); scope that failure to the
884+
// offending tool rather than letting it reject the whole listTools() call.
864885
if (tool.outputSchema) {
865-
const toolValidator = this._jsonSchemaValidator.getValidator(tool.outputSchema as JsonSchemaType);
866-
this._cachedToolOutputValidators.set(tool.name, toolValidator);
886+
try {
887+
const toolValidator = this._jsonSchemaValidator.getValidator(tool.outputSchema as JsonSchemaType);
888+
this._cachedToolOutputValidators.set(tool.name, toolValidator);
889+
} catch (error) {
890+
this._toolOutputValidatorErrors.set(tool.name, error instanceof Error ? error : new Error(String(error)));
891+
}
867892
}
868893
}
869894
}

test/integration/test/client/client.test.ts

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1940,6 +1940,60 @@ describe('outputSchema validation', () => {
19401940
);
19411941
});
19421942

1943+
/***
1944+
* Test: A single tool with an uncompilable outputSchema does not break listTools()
1945+
* or the use of other tools (SEP-2106 safety-guard isolation).
1946+
*/
1947+
test('isolates a tool whose outputSchema fails to compile from the rest of the server', async () => {
1948+
const server = new Server({ name: 'test-server', version: '1.0.0' }, { capabilities: { tools: {} } });
1949+
1950+
server.setRequestHandler('initialize', async request => ({
1951+
protocolVersion: request.params.protocolVersion,
1952+
capabilities: { tools: {} },
1953+
serverInfo: { name: 'test-server', version: '1.0.0' }
1954+
}));
1955+
1956+
server.setRequestHandler('tools/list', async () => ({
1957+
tools: [
1958+
{
1959+
name: 'bad-tool',
1960+
description: 'advertises a non-local $ref the SEP-2106 guard rejects',
1961+
inputSchema: { type: 'object', properties: {} },
1962+
// Non-same-document $ref: assertSchemaSafeToCompile throws when this is compiled.
1963+
outputSchema: { $ref: 'https://evil.example/schema.json' }
1964+
},
1965+
{
1966+
name: 'good-tool',
1967+
description: 'a normal tool that must remain usable',
1968+
inputSchema: { type: 'object', properties: {} },
1969+
outputSchema: { type: 'object', properties: { ok: { type: 'boolean' } }, required: ['ok'] }
1970+
}
1971+
]
1972+
}));
1973+
1974+
server.setRequestHandler('tools/call', async request => {
1975+
if (request.params.name === 'good-tool') {
1976+
return { content: [], structuredContent: { ok: true } };
1977+
}
1978+
return { content: [], structuredContent: { irrelevant: true } };
1979+
});
1980+
1981+
const client = new Client({ name: 'test-client', version: '1.0.0' });
1982+
const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair();
1983+
await Promise.all([client.connect(clientTransport), server.connect(serverTransport)]);
1984+
1985+
// listTools() must NOT reject just because one tool's schema is uncompilable.
1986+
const listed = await client.listTools();
1987+
expect(listed.tools.map(t => t.name).toSorted()).toEqual(['bad-tool', 'good-tool']);
1988+
1989+
// The good tool is fully usable.
1990+
const good = await client.callTool({ name: 'good-tool' });
1991+
expect(good.structuredContent).toEqual({ ok: true });
1992+
1993+
// The bad tool surfaces a scoped, descriptive error only when it is called.
1994+
await expect(client.callTool({ name: 'bad-tool' })).rejects.toThrow(/output schema that could not be compiled/i);
1995+
});
1996+
19431997
/***
19441998
* Test: Handle Tools Without outputSchema Normally
19451999
*/

0 commit comments

Comments
 (0)