Merge pull request #129: drill-down required params

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<string, unknown>): Record<string, unknown> {
+  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. */

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<Record<string, unknown>>;
 }
 
-function mockUpstream(name: string, payloads: Record<string, string> = PAYLOADS): Upstream {
+function mockUpstream(
+  name: string,
+  payloads: Record<string, string> = PAYLOADS,
+  schema: unknown = STRICT_SCHEMA,
+): Upstream {
   const calls: Array<Record<string, unknown>> = [];
   const conn = {
     name,
@@ -90,7 +108,7 @@ function mockUpstream(name: string, payloads: Record<string, string> = 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<string, string> } = {}) {
+function setup(opts: { gated?: boolean; payloads?: Record<string, string>; 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<string, string> } = {}
     { 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<string, unknown>;
+    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<string, unknown>)['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<string, unknown>;
+    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<string, unknown>;
+    // 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);

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<string, unknown>; additionalProperties?: unknown };
+  inputSchema?: {
+    type?: string;
+    properties?: Record<string, unknown>;
+    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;