From 0e1263727123b61113233e3e4c8305c77948bb0c Mon Sep 17 00:00:00 2001 From: Michal Date: Sat, 5 Sep 2026 17:58:57 +0100 Subject: [PATCH] fix(mcplocal): stop upstream required params from blocking a drill-down MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A paginated tool result tells the model to call the same tool again with only _resultId/_section. The advertised schema still carried the upstream's own `required` list, so a client that validates arguments rejected exactly that call — "Received tool input did not match expected schema" — and the model concluded pagination was broken and gave up (websearch/fetch_content requires `url`; verified live on llm-model-tester). `required` is ANDed with everything else in a schema, so no property declaration could rescue it. It now becomes an alternation: either the upstream's requirements (a fresh call) or `_resultId` (a re-read, which is answered from cache and never reaches the upstream). Validators that understand anyOf enforce that; naive ones that only read top-level `required` now find none and accept both. Schemas that already carry a combinator get the alternation appended to `allOf`, and a schema with no required params is left untouched. Covered by unit tests on a required-bearing upstream schema plus a smoke assertion that no advertised tool keeps a drill-down-blocking `required`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JQr5Z9gYrqBQXTGBBuemZ2 --- .../proxymodel/plugins/content-pipeline.ts | 54 +++++++++++- .../plugin-content-pipeline-drilldown.test.ts | 83 ++++++++++++++++++- .../tests/smoke/tool-drilldown.test.ts | 29 ++++++- 3 files changed, 159 insertions(+), 7 deletions(-) diff --git a/src/mcplocal/src/proxymodel/plugins/content-pipeline.ts b/src/mcplocal/src/proxymodel/plugins/content-pipeline.ts index a714aa8..8e75382 100644 --- a/src/mcplocal/src/proxymodel/plugins/content-pipeline.ts +++ b/src/mcplocal/src/proxymodel/plugins/content-pipeline.ts @@ -137,9 +137,24 @@ function drillDownInstruction(toolName: string, resultId: string, sections: Sect /** * Declare the drill-down params on a tool's advertised input schema. * - * Only `properties` is extended: `additionalProperties: false` stays as the + * `properties` is extended: `additionalProperties: false` stays as the * upstream set it, because a property listed in `properties` is allowed by * that keyword. Keeping it false preserves the upstream's typo protection. + * + * `required` also has to give way, or the contract is still unsatisfiable. A + * drill-down call carries only _resultId/_section — it never reaches the + * upstream, so the upstream's own required params (websearch/fetch_content + * requires `url`) are meaningless for it. Left in place, a client that + * validates arguments against the schema rejects the very call the stub just + * instructed the model to make ("Received tool input did not match expected + * schema"), and the model concludes pagination is broken. + * + * So the flat `required` becomes an alternation: either the upstream's + * requirements (a fresh call) or `_resultId` (a re-read of a cached result). + * A validator that understands `anyOf` enforces exactly that; a naive one that + * only looks at top-level `required` now finds none and accepts both. The + * upstream still rejects a fresh call that omits its required params, and the + * requirements stay visible to the model inside the alternation. */ function withDrillDownParams(tool: ToolDefinition): ToolDefinition { if (GATE_TOOLS.has(tool.name)) return tool; @@ -171,7 +186,42 @@ function withDrillDownParams(tool: ToolDefinition): ToolDefinition { + 'previous large result. Requires _resultId.', }; - return { ...tool, inputSchema: { ...s, properties: props } }; + return { ...tool, inputSchema: withDrillDownRequired({ ...s, properties: props }) }; +} + +/** + * Rewrite a flat `required` into "upstream's requirements OR a drill-down". + * + * Schemas that already carry a top-level combinator keep it: the alternation + * is appended to `allOf` instead of colliding with the existing `anyOf`/ + * `oneOf`. A schema with no `required` needs nothing — every call already + * validates. + */ +function withDrillDownRequired(s: Record): Record { + const required = s['required']; + const upstreamRequired = Array.isArray(required) + ? required.filter((r): r is string => typeof r === 'string') + : []; + if (upstreamRequired.length === 0) return s; + + const alternation = { + anyOf: [ + { required: upstreamRequired }, + // _resultId alone is a real call too: it re-shows the table of contents. + { required: ['_resultId'] }, + ], + }; + + const next = { ...s }; + delete next['required']; + + if ('anyOf' in next || 'oneOf' in next || 'allOf' in next) { + const existing = Array.isArray(next['allOf']) ? next['allOf'] : []; + next['allOf'] = [...existing, alternation]; + return next; + } + + return { ...next, ...alternation }; } /** Build a single-text-part tool result. */ diff --git a/src/mcplocal/tests/plugin-content-pipeline-drilldown.test.ts b/src/mcplocal/tests/plugin-content-pipeline-drilldown.test.ts index f665785..555c95e 100644 --- a/src/mcplocal/tests/plugin-content-pipeline-drilldown.test.ts +++ b/src/mcplocal/tests/plugin-content-pipeline-drilldown.test.ts @@ -33,6 +33,20 @@ const STRICT_SCHEMA = { $schema: 'http://json-schema.org/draft-07/schema#', }; +/** + * Mirrors websearch/fetch_content: a required param that a drill-down call + * cannot supply, since the drill-down never reaches the upstream. + */ +const REQUIRED_SCHEMA = { + type: 'object', + properties: { + url: { type: 'string' }, + site: { type: 'string' }, + }, + required: ['url'], + additionalProperties: false, +}; + const BIG_PAYLOAD = 'x'.repeat(20_000); /** @@ -74,7 +88,11 @@ interface Upstream { calls: Array>; } -function mockUpstream(name: string, payloads: Record = PAYLOADS): Upstream { +function mockUpstream( + name: string, + payloads: Record = PAYLOADS, + schema: unknown = STRICT_SCHEMA, +): Upstream { const calls: Array> = []; const conn = { name, @@ -90,7 +108,7 @@ function mockUpstream(name: string, payloads: Record = PAYLOADS) tools: Object.keys(payloads).map((n) => ({ name: n, description: `Retrieve ${n}`, - inputSchema: STRICT_SCHEMA, + inputSchema: schema, })), }, }; @@ -120,7 +138,7 @@ function mockMcpdClient(): McpdClient { } as unknown as McpdClient; } -function setup(opts: { gated?: boolean; payloads?: Record } = {}) { +function setup(opts: { gated?: boolean; payloads?: Record; schema?: unknown } = {}) { const router = new McpRouter(); router.setPromptConfig(mockMcpdClient(), 'test-project'); router.setPlugin(createDefaultPlugin({ gated: opts.gated ?? false, providerRegistry: null })); @@ -129,7 +147,7 @@ function setup(opts: { gated?: boolean; payloads?: Record } = {} { complete: async () => '', available: () => false } as unknown as LLMProviderAdapter, new MemoryCache(), ); - const upstream = mockUpstream('unifi-network', opts.payloads ?? PAYLOADS); + const upstream = mockUpstream('unifi-network', opts.payloads ?? PAYLOADS, opts.schema ?? STRICT_SCHEMA); router.addUpstream(upstream.conn); return { router, upstream }; } @@ -187,6 +205,63 @@ describe('content-pipeline drill-down contract', () => { expect(schema['additionalProperties']).toBe(false); }); + it('does not leave the upstream\'s required params blocking a drill-down call', async () => { + // websearch/fetch_content requires `url`. A drill-down carries only + // _resultId/_section, so a client validating against a flat + // `required: ["url"]` rejected the exact call the stub asked for. + const { router } = setup({ schema: REQUIRED_SCHEMA }); + const tool = (await listTools(router)).find((t) => t.name.endsWith('get_devices')); + + const schema = tool!.inputSchema as Record; + expect(schema['required']).toBeUndefined(); + expect(schema['anyOf']).toEqual([ + { required: ['url'] }, + { required: ['_resultId'] }, + ]); + // The upstream's requirement is preserved, not dropped: a fresh call still + // has to carry `url`. + expect((schema['properties'] as Record)['url']).toMatchObject({ type: 'string' }); + }); + + it('serves a drill-down that omits the upstream\'s required params', async () => { + const { router, upstream } = setup({ schema: REQUIRED_SCHEMA }); + await listTools(router); + + const stub = textOf(await callTool(router, { url: 'https://example.com' }, 3)); + const resultId = /_resultId="([^"]+)"/.exec(stub)?.[1]; + const callsAfterFirst = upstream.calls.length; + + const page = textOf(await callTool(router, { _resultId: resultId!, _section: 'page-1' }, 4)); + + expect(page).toContain('x'.repeat(100)); + expect(upstream.calls.length).toBe(callsAfterFirst); + }); + + it('leaves a schema without required params alone', async () => { + const { router } = setup(); + const tool = (await listTools(router)).find((t) => t.name.endsWith('get_devices')); + + const schema = tool!.inputSchema as Record; + expect(schema['anyOf']).toBeUndefined(); + expect(schema['allOf']).toBeUndefined(); + }); + + it('composes with a schema that already carries a combinator', async () => { + const { router } = setup({ + schema: { ...REQUIRED_SCHEMA, oneOf: [{ required: ['site'] }] }, + }); + const tool = (await listTools(router)).find((t) => t.name.endsWith('get_devices')); + + const schema = tool!.inputSchema as Record; + // The upstream's own combinator survives; ours is appended, not merged + // into it. + expect(schema['oneOf']).toEqual([{ required: ['site'] }]); + expect(schema['required']).toBeUndefined(); + expect(schema['allOf']).toEqual([ + { anyOf: [{ required: ['url'] }, { required: ['_resultId'] }] }, + ]); + }); + it('leaves gate tools alone (they never reach the pipeline)', async () => { const { router } = setup({ gated: true }); const tools = await listTools(router); diff --git a/src/mcplocal/tests/smoke/tool-drilldown.test.ts b/src/mcplocal/tests/smoke/tool-drilldown.test.ts index d1c5935..8ccbf7b 100644 --- a/src/mcplocal/tests/smoke/tool-drilldown.test.ts +++ b/src/mcplocal/tests/smoke/tool-drilldown.test.ts @@ -20,9 +20,15 @@ const TOKEN = process.env['SMOKE_MCPTOKEN']; interface Tool { name: string; - inputSchema?: { type?: string; properties?: Record; additionalProperties?: unknown }; + inputSchema?: { + type?: string; + properties?: Record; + additionalProperties?: unknown; + required?: unknown; + }; } + /** Tools served by the gate plugin — intercepted before the pipeline, so exempt. */ const GATE_TOOLS = new Set(['begin_session', 'read_prompts', 'propose_prompt', 'propose_skill']); @@ -97,6 +103,27 @@ describe('Smoke: tool drill-down contract', () => { console.log(` ${strict.length} strict-schema tools keep additionalProperties: false`); }); + it('required params never block a drill-down call', async () => { + if (!available) return; + + // A drill-down carries only _resultId/_section. If the advertised schema + // still demands the upstream's own required params (websearch's `url`), a + // validating client rejects the call the stub just asked for — "Received + // tool input did not match expected schema". + // A top-level `required` is ANDed with everything else in the schema, so + // any entry there is unsatisfiable for a drill-down. The upstream's + // requirements live in an `anyOf` alternation instead. + const blocked = tools + .filter((t) => { + if (GATE_TOOLS.has(t.name)) return false; + const required = t.inputSchema?.required; + return Array.isArray(required) && required.length > 0; + }) + .map((t) => t.name); + + expect(blocked).toEqual([]); + }); + it('gate tools do not advertise drill-down params', async () => { if (!available) return; -- 2.49.1