From 03350856ea43662086ce97bc47ae87f889b1eb71 Mon Sep 17 00:00:00 2001 From: Michal Date: Fri, 14 Aug 2026 23:27:36 +0100 Subject: [PATCH] fix(mcplocal): make the paginated-result contract usable by any MCP client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit UniFi tools were unusable from non-Claude agents. A tool result over 2000 chars is replaced with a table of contents and re-read by calling the tool again with _resultId/_section, but those params were never declared on the tool's inputSchema — and unifi-network and my-grafana ship `additionalProperties: false`, so for a client that validates arguments the drill-down call was illegal and the data unreachable. The stub's own wording made it worse: "Use section parameter" names a param that does not exist; sending it fell through to the upstream, re-paginated, and minted a fresh _resultId. An unbounded loop. UniFi took the blame because it is the one server whose results always trip the threshold: get_devices was 11,718 chars, get_clients 136,696 across 19 pages with a 92-char page-1. Grafana and Gitea return compact results. - content-pipeline gains onToolsList, declaring _resultId/_section on every tool it can paginate (gate tools excluded — they are intercepted before the pipeline runs). additionalProperties stays false: a property listed in `properties` is already legal under it, so declaring is enough and the upstream keeps its typo protection. - createDefaultPlugin wired only the gate's onToolsList, so the pipeline's would have been dropped on the floor. Both now chain, gate first. - _resultId without _section re-shows the table of contents instead of forwarding an unknown argument to a strict upstream. - The stub names the tool, the live _resultId and a real section id. - Nested MCP envelopes are collapsed. A server fronting another MCP server returns the inner result wrapped in its own content/structuredContent pair, so the payload arrives two or three times over. A layer is peeled only when it carries nothing the inner value lacks, which keeps it lossless. Live against the sre project: get_devices 11,718 -> 5,210 chars and no longer paginates at all; get_clients 136,696 -> 62,358 across 8 pages instead of 19. - deploy/mcplocal.service shipped MCPLOCAL_MCPD_URL=http://10.0.0.194:3100, which is dead — every working machine had a hand-written drop-in. Points at the k8s ingress now. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01JNXFvxanvM6uiFcb4Mp3xU --- deploy/mcplocal.service | 6 +- .../proxymodel/plugins/content-pipeline.ts | 246 ++++++++++++- .../src/proxymodel/plugins/default.ts | 12 +- .../src/proxymodel/stages/paginate.ts | 4 +- .../src/proxymodel/stages/section-split.ts | 2 +- .../src/proxymodel/stages/summarize-tree.ts | 4 +- src/mcplocal/src/router.ts | 2 +- .../plugin-content-pipeline-drilldown.test.ts | 339 ++++++++++++++++++ .../tests/prompt-section-drilldown.test.ts | 6 +- .../tests/smoke/prompt-drilldown.test.ts | 2 +- .../tests/smoke/proxy-pipeline.test.ts | 2 +- .../tests/smoke/tool-drilldown.test.ts | 109 ++++++ 12 files changed, 710 insertions(+), 24 deletions(-) create mode 100644 src/mcplocal/tests/plugin-content-pipeline-drilldown.test.ts create mode 100644 src/mcplocal/tests/smoke/tool-drilldown.test.ts diff --git a/deploy/mcplocal.service b/deploy/mcplocal.service index fc26b40..8d3216e 100644 --- a/deploy/mcplocal.service +++ b/deploy/mcplocal.service @@ -7,7 +7,11 @@ Type=simple ExecStart=/usr/bin/mcpctl-local Restart=on-failure RestartSec=5 -Environment=MCPLOCAL_MCPD_URL=http://10.0.0.194:3100 +# mcpd now runs on Kubernetes behind this ingress. The previous default, +# http://10.0.0.194:3100, is dead — a fresh install pointed at it and every +# machine that worked did so via a hand-written drop-in. Override per host with: +# systemctl --user edit mcplocal -> Environment=MCPLOCAL_MCPD_URL=... +Environment=MCPLOCAL_MCPD_URL=https://mcpctl.ad.itaz.eu Environment=MCPLOCAL_HTTP_PORT=3200 Environment=MCPLOCAL_HTTP_HOST=127.0.0.1 diff --git a/src/mcplocal/src/proxymodel/plugins/content-pipeline.ts b/src/mcplocal/src/proxymodel/plugins/content-pipeline.ts index 568caf4..a714aa8 100644 --- a/src/mcplocal/src/proxymodel/plugins/content-pipeline.ts +++ b/src/mcplocal/src/proxymodel/plugins/content-pipeline.ts @@ -7,20 +7,37 @@ * - sectionStore management * * This plugin handles: - * 1. onToolCallBefore: intercept section drill-down requests (_resultId + _section params) - * 2. onToolCallAfter: run tool results through the proxymodel pipeline + * 1. onToolsList: declare the drill-down params on every tool it may paginate + * 2. onToolCallBefore: intercept section drill-down requests (_resultId + _section params) + * 3. onToolCallAfter: run tool results through the proxymodel pipeline */ import type { JsonRpcRequest, JsonRpcResponse } from '../../types.js'; -import type { Section } from '../types.js'; +import type { Section, ToolDefinition } from '../types.js'; import type { ProxyModelPlugin, PluginSessionContext } from '../plugin.js'; const SECTION_STORE_TTL_MS = 300_000; // 5 minutes +/** + * Tools owned by the gate plugin. Their calls are intercepted in + * onToolCallBefore and returned before onToolCallAfter ever runs, so their + * results never paginate and they must not advertise drill-down params. + */ +const GATE_TOOLS = new Set(['begin_session', 'read_prompts', 'propose_prompt', 'propose_skill']); + export function createContentPipelinePlugin(): ProxyModelPlugin { return { name: 'content-pipeline', description: 'Content transformation pipeline: paginate, section-split, summarize tool results.', + async onToolsList(tools): Promise { + // The drill-down params are part of this plugin's contract with the + // client, so they belong in the advertised schema. Without them a client + // that validates arguments against inputSchema (and upstreams that set + // `additionalProperties: false`, e.g. unifi-network) can never send + // _resultId/_section, which makes every paginated result unreadable. + return tools.map(withDrillDownParams); + }, + async onToolCallBefore(_toolName, args, request, ctx) { // Intercept section drill-down requests const resultId = args['_resultId'] as string | undefined; @@ -30,6 +47,13 @@ export function createContentPipelinePlugin(): ProxyModelPlugin { return handleSectionDrillDown(request, resultId, section, ctx); } + // _resultId without _section: the caller wants the cached result but did + // not name a section. Re-show the table of contents instead of + // forwarding _resultId upstream, where it is an unknown argument. + if (resultId !== undefined && resultId !== '') { + return handleSectionListing(request, resultId, ctx); + } + return null; }, @@ -50,8 +74,20 @@ export function createContentPipelinePlugin(): ProxyModelPlugin { if (response.error) return response; // Extract text content from the response - const raw = extractTextContent(response); - if (!raw || raw.length <= 2000) return response; + const extracted = extractTextContent(response); + if (extracted === null) return response; + + // Collapse nested MCP envelopes first. A server that proxies another MCP + // server (unifi-network does) returns the inner result wrapped in its own + // content/structuredContent pair, so the same payload arrives two or + // three times over. Unwrapping halves it, which keeps many results under + // the pagination threshold entirely. + const raw = unwrapNestedEnvelopes(extracted); + const unwrapped = raw !== extracted; + + if (raw.length <= 2000) { + return unwrapped ? textResponse(response.id, raw) : response; + } try { const result = await ctx.processContent(toolName, raw, 'toolResult'); @@ -61,7 +97,7 @@ export function createContentPipelinePlugin(): ProxyModelPlugin { const resultId = `pm-${Date.now().toString(36)}`; storeSections(ctx, resultId, result.sections); - const text = `${result.content}\n\n_resultId: ${resultId} — use _resultId and _section parameters to drill into a section.`; + const text = `${result.content}\n\n${drillDownInstruction(toolName, resultId, result.sections)}`; return { jsonrpc: '2.0', id: response.id, @@ -71,21 +107,174 @@ export function createContentPipelinePlugin(): ProxyModelPlugin { // Pipeline ran but no sections — return processed content if it changed if (result.content !== raw) { - return { - jsonrpc: '2.0', - id: response.id, - result: { content: [{ type: 'text', text: result.content }] }, - }; + return textResponse(response.id, result.content); } } catch { - // Pipeline failed — return original response + // Pipeline failed — fall through, but keep the unwrap } + if (unwrapped) return textResponse(response.id, raw); + return response; }, }; } +/** + * Build the drill-down instruction appended to a sectioned result. + * + * Names both parameters exactly as the schema declares them and shows a real + * section id, so a model can copy the call rather than guess at it. Guessing + * used to mean sending a bare `section`, which fell through to the upstream + * and re-paginated with a fresh _resultId — an unbounded loop. + */ +function drillDownInstruction(toolName: string, resultId: string, sections: Section[]): string { + const example = sections[0]?.id ?? 'page-1'; + return `To read one, call ${toolName} again with _resultId="${resultId}" and _section="${example}". ` + + 'Both parameters are required together; pass no other arguments.'; +} + +/** + * Declare the drill-down params on a tool's advertised input schema. + * + * Only `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. + */ +function withDrillDownParams(tool: ToolDefinition): ToolDefinition { + if (GATE_TOOLS.has(tool.name)) return tool; + + const schema = tool.inputSchema; + if (typeof schema !== 'object' || schema === null || Array.isArray(schema)) return tool; + + const s = schema as Record; + if (s['type'] !== undefined && s['type'] !== 'object') return tool; + + const existing = s['properties']; + const props: Record = + typeof existing === 'object' && existing !== null && !Array.isArray(existing) + ? { ...(existing as Record) } + : {}; + + if ('_resultId' in props || '_section' in props) return tool; + + props['_resultId'] = { + type: 'string', + description: + 'Set only when re-reading a large result this tool already returned. ' + + 'Pass the _resultId from that result together with _section.', + }; + props['_section'] = { + type: 'string', + description: + 'Section id to read (e.g. "page-1"), from the table of contents of a ' + + 'previous large result. Requires _resultId.', + }; + + return { ...tool, inputSchema: { ...s, properties: props } }; +} + +/** Build a single-text-part tool result. */ +function textResponse(id: JsonRpcResponse['id'], text: string): JsonRpcResponse { + return { jsonrpc: '2.0', id, result: { content: [{ type: 'text', text }] } }; +} + +/** Maximum envelope layers to peel. Guards against pathological nesting. */ +const MAX_UNWRAP_DEPTH = 5; + +/** + * Collapse nested MCP result envelopes. + * + * A server that fronts another MCP server hands back the inner result still + * wrapped: `{ ...meta, content: [{ type: 'text', text: "" }], + * structuredContent: }`. The payload is then present twice + * — once escaped inside `content`, once parsed in `structuredContent` — and + * the whole thing is itself a string inside the outer result. unifi-network's + * get_clients arrives at 136K this way. + * + * A layer is peeled only when it carries nothing the inner value doesn't + * already have: every sibling key must reappear in the inner value with an + * equal value, and `structuredContent` (if present) must equal it too. That + * makes the collapse lossless; anything else is left alone. + */ +function unwrapNestedEnvelopes(text: string): string { + let current: unknown; + try { + current = JSON.parse(text); + } catch { + return text; // Not JSON — nothing to unwrap. + } + + let peeled = false; + for (let depth = 0; depth < MAX_UNWRAP_DEPTH; depth++) { + const inner = peelEnvelope(current); + if (inner === null) break; + current = inner; + peeled = true; + } + + if (!peeled) return text; + return typeof current === 'string' ? current : JSON.stringify(current, null, 2); +} + +/** Peel one envelope layer, or return null when this value is not a redundant wrapper. */ +function peelEnvelope(value: unknown): unknown | null { + if (typeof value !== 'object' || value === null || Array.isArray(value)) return null; + + const obj = value as Record; + const parts = obj['content']; + if (!Array.isArray(parts) || parts.length === 0) return null; + + // Only text parts can be collapsed; images and resources must survive intact. + const texts: string[] = []; + for (const part of parts) { + if (typeof part !== 'object' || part === null) return null; + const p = part as Record; + if (p['type'] !== 'text' || typeof p['text'] !== 'string') return null; + texts.push(p['text']); + } + + const innerText = texts.join('\n'); + let inner: unknown; + try { + inner = JSON.parse(innerText); + } catch { + // Inner payload is plain text. Collapsing to it is still lossless when the + // envelope adds nothing but a duplicate structuredContent. + inner = innerText; + } + + // structuredContent must not carry anything the inner payload lacks. + if ('structuredContent' in obj && !deepEqual(obj['structuredContent'], inner)) return null; + + // Every other sibling key must already be present, and equal, inside. + for (const [key, v] of Object.entries(obj)) { + if (key === 'content' || key === 'structuredContent') continue; + if (typeof inner !== 'object' || inner === null || Array.isArray(inner)) return null; + if (!deepEqual((inner as Record)[key], v)) return null; + } + + return inner; +} + +/** Structural equality for JSON-shaped values. */ +function deepEqual(a: unknown, b: unknown): boolean { + if (a === b) return true; + if (typeof a !== typeof b) return false; + if (typeof a !== 'object' || a === null || b === null) return false; + + if (Array.isArray(a) || Array.isArray(b)) { + if (!Array.isArray(a) || !Array.isArray(b) || a.length !== b.length) return false; + return a.every((item, i) => deepEqual(item, b[i])); + } + + const ao = a as Record; + const bo = b as Record; + const aKeys = Object.keys(ao); + if (aKeys.length !== Object.keys(bo).length) return false; + return aKeys.every((k) => k in bo && deepEqual(ao[k], bo[k])); +} + /** Extract text content from a tool result response. */ function extractTextContent(response: JsonRpcResponse): string | null { if (!response.result || typeof response.result !== 'object') return null; @@ -145,6 +334,39 @@ function handleSectionDrillDown( }; } +/** Re-show the table of contents for a cached result (no _section given). */ +function handleSectionListing( + request: JsonRpcRequest, + resultId: string, + ctx: PluginSessionContext, +): JsonRpcResponse { + const sections = getSections(ctx, resultId); + if (!sections) { + return { + jsonrpc: '2.0', + id: request.id, + result: { + content: [{ + type: 'text', + text: 'Cached result not found (expired or invalid _resultId). Please re-call the tool without _resultId/_section to get a fresh result.', + }], + }, + }; + } + + const toc = sections.map((s) => `[${s.id}] ${s.title}`).join('\n'); + return { + jsonrpc: '2.0', + id: request.id, + result: { + content: [{ + type: 'text', + text: `${sections.length} sections:\n${toc}\n\nAdd _section="" alongside _resultId="${resultId}" to read one.`, + }], + }, + }; +} + /** Find a section by ID, searching recursively through children. */ function findSection(sections: Section[], id: string): Section | null { for (const section of sections) { diff --git a/src/mcplocal/src/proxymodel/plugins/default.ts b/src/mcplocal/src/proxymodel/plugins/default.ts index c97e7e7..4e39778 100644 --- a/src/mcplocal/src/proxymodel/plugins/default.ts +++ b/src/mcplocal/src/proxymodel/plugins/default.ts @@ -10,6 +10,7 @@ * overlap on hooks, no conflicts arise. */ import type { ProxyModelPlugin } from '../plugin.js'; +import type { ToolDefinition } from '../types.js'; import { createGatePlugin, type GatePluginConfig } from './gate.js'; import { createContentPipelinePlugin } from './content-pipeline.js'; @@ -59,8 +60,15 @@ export function createDefaultPlugin(config: DefaultPluginConfig = {}): ProxyMode if (gate.onInitialize) { plugin.onInitialize = gate.onInitialize.bind(gate); } - if (gate.onToolsList) { - plugin.onToolsList = gate.onToolsList.bind(gate); + // Tools list: gate first (it decides which tools are visible at all), then + // content-pipeline (it annotates whatever survived with drill-down params). + if (gate.onToolsList || pipeline.onToolsList) { + plugin.onToolsList = async (tools, ctx): Promise => { + let acc = tools; + if (gate.onToolsList) acc = await gate.onToolsList(acc, ctx); + if (pipeline.onToolsList) acc = await pipeline.onToolsList(acc, ctx); + return acc; + }; } if (pipeline.onToolCallAfter) { plugin.onToolCallAfter = pipeline.onToolCallAfter.bind(pipeline); diff --git a/src/mcplocal/src/proxymodel/stages/paginate.ts b/src/mcplocal/src/proxymodel/stages/paginate.ts index 33ef52a..7b522e0 100644 --- a/src/mcplocal/src/proxymodel/stages/paginate.ts +++ b/src/mcplocal/src/proxymodel/stages/paginate.ts @@ -37,7 +37,9 @@ const handler: StageHandler = async (content, ctx) => { ).join('\n'); return { - content: `Content split into ${sections.length} pages (${content.length} total chars):\n${toc}\n\nUse section parameter to read a specific page.`, + // No navigation hint here — the caller (content-pipeline / router) appends + // the authoritative _resultId/_section instruction once sections are stored. + content: `Content split into ${sections.length} pages (${content.length} total chars):\n${toc}`, sections, }; }; diff --git a/src/mcplocal/src/proxymodel/stages/section-split.ts b/src/mcplocal/src/proxymodel/stages/section-split.ts index adbd698..e18db77 100644 --- a/src/mcplocal/src/proxymodel/stages/section-split.ts +++ b/src/mcplocal/src/proxymodel/stages/section-split.ts @@ -65,7 +65,7 @@ const handler: StageHandler = async (content, ctx) => { }).join('\n'); return { - content: `${sections.length} sections (${contentType}):\n${toc}\n\nUse section parameter to read a specific section.`, + content: `${sections.length} sections (${contentType}):\n${toc}`, sections, }; }; diff --git a/src/mcplocal/src/proxymodel/stages/summarize-tree.ts b/src/mcplocal/src/proxymodel/stages/summarize-tree.ts index 8528053..b75e419 100644 --- a/src/mcplocal/src/proxymodel/stages/summarize-tree.ts +++ b/src/mcplocal/src/proxymodel/stages/summarize-tree.ts @@ -39,7 +39,7 @@ const handler: StageHandler = async (content, ctx) => { const summary = await cachedSummarize(ctx, ctx.originalContent, maxTokens); return { - content: summary + '\n\nUse section parameter with id "full" to read the complete content.', + content: summary + '\n\nSection "full" holds the complete content.', sections: [{ id: 'full', title: 'Full Content', content: ctx.originalContent }], }; } @@ -56,7 +56,7 @@ const handler: StageHandler = async (content, ctx) => { }).join('\n'); return { - content: `${tree.length} sections:\n${toc}\n\nUse section parameter to read details.`, + content: `${tree.length} sections:\n${toc}`, sections: tree, }; }; diff --git a/src/mcplocal/src/router.ts b/src/mcplocal/src/router.ts index a5411bb..3715a01 100644 --- a/src/mcplocal/src/router.ts +++ b/src/mcplocal/src/router.ts @@ -1191,6 +1191,6 @@ function injectSectionsIntoPromptResponse( if (now - entry.createdAt > SECTION_TTL_MS) store.delete(key); } - const text = `${tocContent}\n\n_resultId: ${resultId} — use _resultId and _section parameters to drill into a section.`; + const text = `${tocContent}\n\nTo read one, request this prompt again with _resultId="${resultId}" and _section="". Both parameters are required together.`; return replacePromptText(response, text); } diff --git a/src/mcplocal/tests/plugin-content-pipeline-drilldown.test.ts b/src/mcplocal/tests/plugin-content-pipeline-drilldown.test.ts new file mode 100644 index 0000000..f665785 --- /dev/null +++ b/src/mcplocal/tests/plugin-content-pipeline-drilldown.test.ts @@ -0,0 +1,339 @@ +/** + * content-pipeline drill-down contract tests. + * + * Regression cover for the defect that made UniFi tools unusable from + * non-Claude MCP clients: a large tool result was replaced with a stub that + * told the caller to pass _resultId/_section, but those parameters were never + * advertised on the tool's inputSchema. Upstreams that declare + * `additionalProperties: false` (unifi-network, my-grafana) therefore made the + * drill-down call invalid for any client that validates against the schema, + * and the stub's own wording ("Use section parameter") pointed at a parameter + * that does not exist — sending it re-paginated and minted a fresh _resultId, + * an unbounded loop. + * + * Driven through a real McpRouter with the default plugin (gate + + * content-pipeline), mirroring project-mcp-endpoint wiring. + */ +import { describe, it, expect, vi } from 'vitest'; +import { McpRouter } from '../src/router.js'; +import type { UpstreamConnection, JsonRpcRequest, JsonRpcResponse } from '../src/types.js'; +import type { McpdClient } from '../src/http/mcpd-client.js'; +import { createDefaultPlugin } from '../src/proxymodel/plugins/default.js'; +import { LLMProviderAdapter } from '../src/proxymodel/llm-adapter.js'; +import { MemoryCache } from '../src/proxymodel/cache.js'; + +/** Mirrors unifi-network: strict schema, and a payload big enough to paginate. */ +const STRICT_SCHEMA = { + type: 'object', + properties: { + targetId: { type: 'string' }, + site: { type: 'string' }, + }, + additionalProperties: false, + $schema: 'http://json-schema.org/draft-07/schema#', +}; + +const BIG_PAYLOAD = 'x'.repeat(20_000); + +/** + * The shape unifi-network actually returns: the inner MCP result is wrapped in + * the server's own content/structuredContent pair, so the payload arrives + * twice — once escaped inside `content`, once parsed in `structuredContent`. + */ +function wrapLikeUnifi(tool: string, inner: unknown): string { + const innerEnvelope = { tool, targetId: 'home', result: inner }; + return JSON.stringify( + { + tool, + targetId: 'home', + content: [{ type: 'text', text: JSON.stringify(innerEnvelope, null, 2) }], + structuredContent: innerEnvelope, + }, + null, + 2, + ); +} + +/** Inner payload stays under the 8000-char page size; wrapped, it does not. */ +const DEVICE_ROWS = Array.from({ length: 30 }, (_, i) => ({ + mac: `0c:ea:14:38:af:${i.toString(16).padStart(2, '0')}`, + name: `Switch ${i} — office floor plan position ${i}`, + model: 'USPM16', + ip: `192.168.1.${i + 10}`, + version: '7.4.1.16850', +})); + +const PAYLOADS: Record = { + get_devices: BIG_PAYLOAD, + get_clients: wrapLikeUnifi('get_clients', { data: DEVICE_ROWS }), + get_alarms: wrapLikeUnifi('get_alarms', { data: [] }), +}; + +interface Upstream { + conn: UpstreamConnection; + calls: Array>; +} + +function mockUpstream(name: string, payloads: Record = PAYLOADS): Upstream { + const calls: Array> = []; + const conn = { + name, + isAlive: vi.fn(() => true), + close: vi.fn(async () => {}), + onNotification: vi.fn(), + send: vi.fn(async (req: JsonRpcRequest): Promise => { + if (req.method === 'tools/list') { + return { + jsonrpc: '2.0', + id: req.id, + result: { + tools: Object.keys(payloads).map((n) => ({ + name: n, + description: `Retrieve ${n}`, + inputSchema: STRICT_SCHEMA, + })), + }, + }; + } + if (req.method === 'tools/call') { + const params = (req.params ?? {}) as Record; + calls.push((params['arguments'] as Record) ?? {}); + const tool = String(params['name'] ?? '').split('/').pop() ?? ''; + return { jsonrpc: '2.0', id: req.id, result: { content: [{ type: 'text', text: payloads[tool] ?? '' }] } }; + } + if (req.method === 'resources/list') return { jsonrpc: '2.0', id: req.id, result: { resources: [] } }; + if (req.method === 'prompts/list') return { jsonrpc: '2.0', id: req.id, result: { prompts: [] } }; + return { jsonrpc: '2.0', id: req.id, error: { code: -32601, message: 'Not found' } }; + }), + } as unknown as UpstreamConnection; + return { conn, calls }; +} + +function mockMcpdClient(): McpdClient { + return { + get: vi.fn(async () => []), + post: vi.fn(async () => ({})), + put: vi.fn(async () => ({})), + delete: vi.fn(async () => {}), + forward: vi.fn(async () => ({ status: 200, body: {} })), + withHeaders: vi.fn(function (this: McpdClient) { return this; }), + } as unknown as McpdClient; +} + +function setup(opts: { gated?: boolean; payloads?: Record } = {}) { + const router = new McpRouter(); + router.setPromptConfig(mockMcpdClient(), 'test-project'); + router.setPlugin(createDefaultPlugin({ gated: opts.gated ?? false, providerRegistry: null })); + router.setProxyModel( + 'default', + { complete: async () => '', available: () => false } as unknown as LLMProviderAdapter, + new MemoryCache(), + ); + const upstream = mockUpstream('unifi-network', opts.payloads ?? PAYLOADS); + router.addUpstream(upstream.conn); + return { router, upstream }; +} + +async function listTools(router: McpRouter, sessionId = 's1') { + await router.route({ jsonrpc: '2.0', id: 1, method: 'initialize' }, { sessionId }); + const res = await router.route({ jsonrpc: '2.0', id: 2, method: 'tools/list' }, { sessionId }); + return (res.result as { tools: Array<{ name: string; inputSchema?: unknown }> }).tools; +} + +function textOf(res: JsonRpcResponse): string { + return (res.result as { content: Array<{ text: string }> }).content[0]!.text; +} + +async function callNamed( + router: McpRouter, + tool: string, + args: Record, + id: number, + sessionId = 's1', +): Promise { + return router.route( + { jsonrpc: '2.0', id, method: 'tools/call', params: { name: `unifi-network/${tool}`, arguments: args } }, + { sessionId }, + ); +} + +async function callTool( + router: McpRouter, + args: Record, + id: number, + sessionId = 's1', +): Promise { + return router.route( + { jsonrpc: '2.0', id, method: 'tools/call', params: { name: 'unifi-network/get_devices', arguments: args } }, + { sessionId }, + ); +} + +describe('content-pipeline drill-down contract', () => { + it('advertises _resultId/_section on a strict-schema upstream tool', async () => { + const { router } = setup(); + const tool = (await listTools(router)).find((t) => t.name.endsWith('get_devices')); + + expect(tool).toBeDefined(); + const schema = tool!.inputSchema as Record; + const props = schema['properties'] as Record; + + expect(props['_resultId']).toMatchObject({ type: 'string' }); + expect(props['_section']).toMatchObject({ type: 'string' }); + // The upstream's own params survive untouched. + expect(props['targetId']).toMatchObject({ type: 'string' }); + // Declaring the params in `properties` is what makes them legal; the + // upstream's strictness is preserved rather than loosened. + expect(schema['additionalProperties']).toBe(false); + }); + + it('leaves gate tools alone (they never reach the pipeline)', async () => { + const { router } = setup({ gated: true }); + const tools = await listTools(router); + + const beginSession = tools.find((t) => t.name === 'begin_session'); + expect(beginSession).toBeDefined(); + const props = (beginSession!.inputSchema as Record)['properties'] as Record; + expect(props['_resultId']).toBeUndefined(); + expect(props['_section']).toBeUndefined(); + }); + + it('stubs a large result with an instruction naming the real parameters', async () => { + const { router } = setup(); + await listTools(router); + + const text = textOf(await callTool(router, {}, 3)); + + expect(text).toContain('_resultId='); + expect(text).toContain('_section='); + // The old wording pointed at a parameter that does not exist. + expect(text).not.toContain('Use section parameter'); + }); + + it('serves the page from cache without a second upstream call', async () => { + const { router, upstream } = setup(); + await listTools(router); + + const stub = textOf(await callTool(router, {}, 3)); + const resultId = /_resultId="([^"]+)"/.exec(stub)?.[1]; + expect(resultId).toBeTruthy(); + + const callsAfterFirst = upstream.calls.length; + const page = textOf(await callTool(router, { _resultId: resultId!, _section: 'page-1' }, 4)); + + expect(page).toContain('x'.repeat(100)); + // Drill-down is answered locally, so the strict upstream never sees the + // params it would reject. + expect(upstream.calls.length).toBe(callsAfterFirst); + }); + + it('re-shows the table of contents when _resultId arrives without _section', async () => { + const { router, upstream } = setup(); + await listTools(router); + + const stub = textOf(await callTool(router, {}, 3)); + const resultId = /_resultId="([^"]+)"/.exec(stub)?.[1]; + const callsAfterFirst = upstream.calls.length; + + const res = textOf(await callTool(router, { _resultId: resultId! }, 4)); + + expect(res).toContain('page-1'); + expect(res).toContain('_section='); + expect(upstream.calls.length).toBe(callsAfterFirst); + }); + + it('reports an expired or unknown _resultId instead of forwarding it', async () => { + const { router, upstream } = setup(); + await listTools(router); + const callsBefore = upstream.calls.length; + + const res = textOf(await callTool(router, { _resultId: 'pm-nope', _section: 'page-1' }, 3)); + + expect(res).toContain('Cached result not found'); + expect(upstream.calls.length).toBe(callsBefore); + }); +}); + +describe('nested MCP envelope collapse', () => { + it('collapses a doubly-wrapped result to its inner payload', async () => { + const { router } = setup(); + await listTools(router); + + const text = textOf(await callNamed(router, 'get_alarms', {}, 3)); + const parsed = JSON.parse(text) as Record; + + // The wrapper's content/structuredContent pair is gone; the payload it + // carried twice now appears once. + expect(parsed['content']).toBeUndefined(); + expect(parsed['structuredContent']).toBeUndefined(); + expect(parsed['tool']).toBe('get_alarms'); + expect(parsed['targetId']).toBe('home'); + expect(parsed['result']).toEqual({ data: [] }); + }); + + it('drops a formerly-paginated result below the pagination threshold', async () => { + const { router } = setup(); + await listTools(router); + + const wrapped = PAYLOADS['get_clients']!; + expect(wrapped.length).toBeGreaterThan(8000); // would paginate as-is + + const text = textOf(await callNamed(router, 'get_clients', {}, 3)); + + // Delivered whole, not as a table of contents. + expect(text).not.toContain('Content split into'); + expect(text).not.toContain('_resultId='); + expect(text.length).toBeLessThan(wrapped.length); + + const parsed = JSON.parse(text) as { result: { data: unknown[] } }; + expect(parsed.result.data).toHaveLength(30); + }); + + it('leaves a wrapper alone when structuredContent disagrees with the text', async () => { + const divergent = JSON.stringify({ + tool: 'get_devices', + content: [{ type: 'text', text: JSON.stringify({ tool: 'get_devices', result: { data: [1] } }) }], + structuredContent: { tool: 'get_devices', result: { data: [1, 2, 3] } }, + }); + const { router } = setup({ payloads: { get_devices: divergent } }); + await listTools(router); + + const text = textOf(await callTool(router, {}, 3)); + + // Collapsing here would silently drop rows, so the envelope survives. + expect(text).toBe(divergent); + }); + + it('leaves a wrapper alone when it carries a key the payload lacks', async () => { + const extraKey = JSON.stringify({ + tool: 'get_devices', + warning: 'partial results — controller unreachable', + content: [{ type: 'text', text: JSON.stringify({ tool: 'get_devices', result: { data: [] } }) }], + }); + const { router } = setup({ payloads: { get_devices: extraKey } }); + await listTools(router); + + expect(textOf(await callTool(router, {}, 3))).toBe(extraKey); + }); + + it('leaves non-text content parts intact', async () => { + const withImage = JSON.stringify({ + tool: 'get_devices', + content: [ + { type: 'text', text: '{"tool":"get_devices"}' }, + { type: 'image', data: 'iVBORw0KGgo=', mimeType: 'image/png' }, + ], + }); + const { router } = setup({ payloads: { get_devices: withImage } }); + await listTools(router); + + expect(textOf(await callTool(router, {}, 3))).toBe(withImage); + }); + + it('leaves plain non-JSON results untouched', async () => { + const { router } = setup({ payloads: { get_devices: 'plain text, no envelope' } }); + await listTools(router); + + expect(textOf(await callTool(router, {}, 3))).toBe('plain text, no envelope'); + }); +}); diff --git a/src/mcplocal/tests/prompt-section-drilldown.test.ts b/src/mcplocal/tests/prompt-section-drilldown.test.ts index 5949e16..19fa261 100644 --- a/src/mcplocal/tests/prompt-section-drilldown.test.ts +++ b/src/mcplocal/tests/prompt-section-drilldown.test.ts @@ -261,9 +261,11 @@ describe('Prompt section drill-down', () => { expect(result.sections).toBeDefined(); expect(result.sections!.length).toBeGreaterThanOrEqual(3); - // TOC should list sections + // TOC should list sections. The stage must NOT name a navigation + // parameter: the caller appends the authoritative _resultId/_section + // instruction, and a bare `section` hint here sends models into a loop. expect(result.content).toContain('sections'); - expect(result.content).toContain('Use section parameter'); + expect(result.content).not.toContain('Use section parameter'); // Original was ~16K, TOC should be much shorter expect(result.content.length).toBeLessThan(largePrompt.length); diff --git a/src/mcplocal/tests/smoke/prompt-drilldown.test.ts b/src/mcplocal/tests/smoke/prompt-drilldown.test.ts index 1917d93..1925611 100644 --- a/src/mcplocal/tests/smoke/prompt-drilldown.test.ts +++ b/src/mcplocal/tests/smoke/prompt-drilldown.test.ts @@ -4,7 +4,7 @@ * Verifies that large prompts served via prompts/get are section-split * and that subsequent calls with _resultId + _section return cached sections. * - * Requires: mcplocal running on localhost:3200, mcpd on 10.0.0.194:3100 + * 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'; diff --git a/src/mcplocal/tests/smoke/proxy-pipeline.test.ts b/src/mcplocal/tests/smoke/proxy-pipeline.test.ts index fe56248..e660252 100644 --- a/src/mcplocal/tests/smoke/proxy-pipeline.test.ts +++ b/src/mcplocal/tests/smoke/proxy-pipeline.test.ts @@ -6,7 +6,7 @@ * * Prerequisites: * - mcplocal running on localhost:3200 - * - mcpd running on 10.0.0.194:3100 + * - mcpd reachable at https://mcpctl.ad.itaz.eu * - smoke-aws-docs server deployed (runtime: python) * * The test suite uses the fixture at fixtures/smoke-data.yaml which diff --git a/src/mcplocal/tests/smoke/tool-drilldown.test.ts b/src/mcplocal/tests/smoke/tool-drilldown.test.ts new file mode 100644 index 0000000..d1c5935 --- /dev/null +++ b/src/mcplocal/tests/smoke/tool-drilldown.test.ts @@ -0,0 +1,109 @@ +/** + * Smoke tests: tool drill-down contract. + * + * A large tool result is replaced with a table of contents and re-read by + * calling the same tool with _resultId + _section. Those two parameters must + * appear in the tool's advertised inputSchema, because upstreams such as + * unifi-network and my-grafana declare `additionalProperties: false` — a + * client validating against the schema cannot send an undeclared parameter, + * which used to make every paginated UniFi result unreadable. + * + * Requires: mcplocal running (localhost:3200) with a project whose servers are + * reachable. Set SMOKE_PROJECT to target a specific project. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { SmokeMcpSession, isMcplocalRunning } from './mcp-client.js'; + +const PROJECT_NAME = process.env['SMOKE_PROJECT'] ?? 'smoke-data'; +/** HTTP-mode mcplocal authenticates every request with an McpToken. */ +const TOKEN = process.env['SMOKE_MCPTOKEN']; + +interface Tool { + name: string; + inputSchema?: { type?: string; properties?: Record; additionalProperties?: 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']); + +describe('Smoke: tool drill-down contract', () => { + let available = false; + let session: SmokeMcpSession; + let tools: Tool[] = []; + + beforeAll(async () => { + available = await isMcplocalRunning(); + if (!available) return; + + session = new SmokeMcpSession(PROJECT_NAME, TOKEN); + await session.initialize(); + await session.sendNotification('notifications/initialized'); + + // Open the gate if the project is gated, so the real catalog is visible. + const gated = await session.send('tools/list') as { tools: Tool[] }; + if (gated.tools.some((t) => t.name === 'begin_session')) { + await session.send('tools/call', { + name: 'begin_session', + arguments: { description: 'Verify the paginated tool-result drill-down contract' }, + }, 180_000); + } + + tools = ((await session.send('tools/list')) as { tools: Tool[] }).tools; + }, 240_000); + + afterAll(async () => { + if (session) await session.close(); + }); + + it('every upstream tool advertises _resultId and _section', async () => { + if (!available) return; + + const upstreamTools = tools.filter((t) => !GATE_TOOLS.has(t.name)); + if (upstreamTools.length === 0) { + console.log(` No upstream tools in project "${PROJECT_NAME}" — skipping`); + return; + } + + const missing = upstreamTools.filter((t) => { + const props = t.inputSchema?.properties; + return props === undefined || props['_resultId'] === undefined || props['_section'] === undefined; + }); + + if (missing.length > 0) { + console.log(` Missing drill-down params: ${missing.map((t) => t.name).join(', ')}`); + } + expect(missing).toEqual([]); + console.log(` ${upstreamTools.length} tools carry the drill-down contract`); + }); + + it('strict upstream schemas stay strict (params are declared, not permitted)', async () => { + if (!available) return; + + // Declaring the params in `properties` is what makes them legal under + // `additionalProperties: false`; loosening it instead would drop the + // upstream's own typo protection. + const strict = tools.filter( + (t) => !GATE_TOOLS.has(t.name) && t.inputSchema?.additionalProperties === false, + ); + if (strict.length === 0) { + console.log(' No strict-schema tools in this project — skipping'); + return; + } + + for (const t of strict) { + expect(t.inputSchema!.properties!['_resultId']).toBeDefined(); + expect(t.inputSchema!.properties!['_section']).toBeDefined(); + } + console.log(` ${strict.length} strict-schema tools keep additionalProperties: false`); + }); + + it('gate tools do not advertise drill-down params', async () => { + if (!available) return; + + for (const t of tools.filter((x) => GATE_TOOLS.has(x.name))) { + const props = t.inputSchema?.properties ?? {}; + expect(props['_resultId']).toBeUndefined(); + expect(props['_section']).toBeUndefined(); + } + }); +});