Merge pull request #130: smoke _resultId regex

The drill-down checks matched /_resultId:\s*(\S+)/ — the legacy paginator's
JSON spelling. The content pipeline writes _resultId="pm-abc", so the regex
found nothing: "Has _resultId for drill-down" reported a false failure on
every run, and the drill-down tests that used the same regex to extract an id
skipped themselves as "not large enough for section-split". Both were
invisible in a green suite.

One shared extractResultId() in the smoke harness now accepts either
spelling and returns the bare id, so the ad-hoc character strip on the
captured group goes away too. proxy-pipeline's check block reports 21/21
instead of 20 with a phantom failure.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JQr5Z9gYrqBQXTGBBuemZ2

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/mcp-client.ts

@@ -261,6 +261,21 @@ export async function isMcplocalRunning(): Promise<boolean> {
   }
 }
 
+/**
+ * Pull the drill-down id out of a paginated result or TOC.
+ *
+ * The instruction is written `_resultId="pm-abc"`, while the legacy paginator
+ * emits it as JSON (`"_resultId": "pm-abc"`). A regex matching only the JSON
+ * form silently found nothing in the current format, so drill-down checks
+ * either reported a false failure or skipped themselves as "not large enough
+ * for section-split" — both invisible in a green run. Accept either spelling
+ * and return the bare id.
+ */
+export function extractResultId(text: string): string | null {
+  const match = /_resultId"?\s*[:=]\s*"?(pm-[a-zA-Z0-9]+)/.exec(text);
+  return match?.[1] ?? null;
+}
+
 /**
  * Run an mcpctl CLI command and return stdout.
  */

src/mcplocal/tests/smoke/prompt-drilldown.test.ts

@@ -7,7 +7,7 @@
  * Requires: mcplocal running on localhost:3200, mcpd at https://mcpctl.ad.itaz.eu
  */
 import { describe, it, expect, beforeAll, afterAll } from 'vitest';
-import { SmokeMcpSession, isMcplocalRunning } from './mcp-client.js';
+import { SmokeMcpSession, isMcplocalRunning, extractResultId } from './mcp-client.js';
 
 const PROJECT_NAME = 'smoke-data';
 
@@ -90,9 +90,8 @@ describe('Smoke: Prompt section drill-down', () => {
         console.log(`  TOC preview: ${text.slice(0, 200)}...`);
 
         // Extract _resultId
-        const match = /_resultId:\s*(pm-[a-z0-9]+)/.exec(text);
-        if (match) {
-          const resultId = match[1];
+        const resultId = extractResultId(text);
+        if (resultId !== null) {
           // Extract first section id from TOC
           const sectionMatch = /\[([^\]]+)\]/.exec(text);
           if (sectionMatch) {

src/mcplocal/tests/smoke/proxy-pipeline.test.ts

@@ -17,7 +17,7 @@
 import { describe, it, expect, beforeAll, afterAll } from 'vitest';
 import { writeFile, mkdir, rm } from 'node:fs/promises';
 import { join, resolve } from 'node:path';
-import { SmokeMcpSession, isMcplocalRunning, mcpctl } from './mcp-client.js';
+import { SmokeMcpSession, isMcplocalRunning, mcpctl, extractResultId } from './mcp-client.js';
 import { ChatReporter } from './reporter.js';
 
 const PROJECT_NAME = 'smoke-data';
@@ -286,8 +286,7 @@ describe('Smoke: ProxyModel pipeline', () => {
       chat.check('Response is manageable size', text.length, (v) => v < 20_000);
 
       if (text.includes('_resultId')) {
-        const match = text.match(/_resultId:\s*(\S+)/);
-        chat.check('_resultId is present', !!match, (v) => v === true);
+        chat.check('_resultId is present', extractResultId(text) !== null, (v) => v === true);
       } else {
         chat.info('Content small enough — no pagination needed');
       }
@@ -307,14 +306,12 @@ describe('Smoke: ProxyModel pipeline', () => {
       });
       const text = result.content[0]?.text ?? '';
 
-      const match = text.match(/_resultId:\s*(\S+)/);
-      if (!match) {
+      const resultId = extractResultId(text);
+      if (resultId === null) {
         chat.info('Content not large enough for pagination — skip drill-down');
         return;
       }
 
-      const resultId = match[1]!.replace(/[^a-zA-Z0-9-]/g, '');
-
       const sectionResult = await chat.callTool('smoke-aws-docs_read_documentation', {
         url: 'https://docs.aws.amazon.com/AmazonS3/latest/userguide/Welcome.html',
         _resultId: resultId,
@@ -401,8 +398,7 @@ describe('Smoke: ProxyModel pipeline', () => {
       chat.check('Response is manageable size', text.length, (v) => v < 20_000);
 
       if (text.includes('_resultId')) {
-        const match = text.match(/_resultId:\s*(\S+)/);
-        chat.check('Has _resultId for drill-down', !!match, (v) => v === true);
+        chat.check('Has _resultId for drill-down', extractResultId(text) !== null, (v) => v === true);
       }
     }, 60_000);
 
@@ -420,14 +416,12 @@ describe('Smoke: ProxyModel pipeline', () => {
       });
       const text = result.content[0]?.text ?? '';
 
-      const match = text.match(/_resultId:\s*(\S+)/);
-      if (!match) {
+      const resultId = extractResultId(text);
+      if (resultId === null) {
         chat.info('Content not large enough for section-split');
         return;
       }
 
-      const resultId = match[1]!.replace(/[^a-zA-Z0-9-]/g, '');
-
       const sectionResult = await chat.callTool('smoke-aws-docs_read_documentation', {
         url: 'https://docs.aws.amazon.com/AmazonS3/latest/userguide/Welcome.html',
         _resultId: resultId,

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;