From 2e54e6af05032e0c9fb4984fad72ab22d7dced77 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 14 Sep 2026 16:10:40 +0000 Subject: [PATCH] Cap tool response sizes, and fix what measuring them uncovered An MCP tool result is spent from the caller's context window, and nothing here bounded one. Measured against the live services, the sizes are driven by the ID asked about rather than by anything the tool decides: reactome_events_hierarchy 87,996 chars ~22,000 tokens reactome_query (Metabolism) 61,336 chars ~15,300 tokens reactome_pathway_contained_events 23,413 chars ~5,900 tokens A single call could crowd out the conversation it was meant to inform. The Cypher tool has had a total-size cap since it was written; the REST tools had nothing. **A cap, applied once.** `MAX_TOOL_RESPONSE_CHARS` (40,000, matching the Cypher default, overridable by env) is enforced in the wrapper every handler already passes through -- a per-tool guard is one somebody forgets to add to the fifty-seventh tool. The cut is announced in the text: a model that cannot see it was truncated will report the partial answer as the whole one. **The root cause in events_hierarchy.** It capped at three top-level pathways and then recursed every descendant of each. It now takes `max_depth` (default 3) and `top_level_limit`, and says where it stopped and which tool goes deeper. 86 KB -> 16 KB. **reactome_query.** Pretty-printed JSON cost ~23% in whitespace no model needs. And truncating a JSON dump hands back invalid JSON while still spending the whole budget, so an object too large to return is now described instead -- field names, shapes and sizes, with a pointer to the `attribute` argument. Metabolism: 43,457 characters of severed object -> 1,327 characters of usable map. Three further bugs fell out of this, each found by checking the fix rather than by assuming it: - **`reactome_query`'s `attribute` argument had never worked.** `/data/query/{id}/{attribute}` answers `text/plain`, the client asks for `application/json`, and the service returns HTTP 406. Found because the summary above advises using `attribute` -- advice that would have been wrong. It now uses the text path that `contentClient.getText` already had. - **The test harness never ran zod.** `fake.invoke()` passed raw arguments straight to handlers, so defaults never materialised and invalid input was never rejected. A test could pass while the real server requested `/data/eventsHierarchy/undefined`, and no test had ever exercised the `nonEmptyString` validation added last week. The harness now parses through the registered schema, and four tests cover validation that was previously untestable. - **The sweep's "empty body" check counted lines.** Compact JSON is one very long line, so a complete answer was reported as empty. It now checks length too. Lint caught two `String(unknown)` calls in the new summariser -- "[object Object]" waiting to happen, the same quiet wrongness as the field-path bugs. 41 -> 81 tests. Coverage thresholds raised to match. Live sweep: 53 tools called, 16 content expectations checked, no suspicious output. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 7 +- scripts/sweep-live.mjs | 8 +- src/config.ts | 19 ++++ src/response-limits.ts | 52 +++++++++ src/tools/index.ts | 94 +++++++++++++++-- src/tools/pathway.ts | 54 ++++++++-- tests/helpers/fake-server.ts | 20 +++- tests/response-limits.test.ts | 192 ++++++++++++++++++++++++++++++++++ vitest.config.ts | 8 +- 9 files changed, 425 insertions(+), 29 deletions(-) create mode 100644 src/response-limits.ts create mode 100644 tests/response-limits.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index e44351c..abc7825 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,11 +7,15 @@ All notable changes to this project are documented here. This project adheres to ### Fixed - **Ten tools read field paths the Reactome services never return.** Each called the right endpoint and reported success, so nothing flagged them: `contentClient.get` asserts `T`, it does not verify it, and 51 of 56 tools had no test. `search_suggest` and `search_spellcheck` expected `{suggestions: []}` where the API returns a bare array; `entity_component_of` expected `Complex` objects where the API returns one entry per relationship type with parallel `names`/`stIds`/`schemaClasses` arrays; `participants` expected `stId` where the API returns `peDbId`; the four interactor tools read `score` one level above where it lives, throwing on `.toFixed`; `analysis_found_entities` read `mapsTo[].identifier` where the API returns `ids[]`. **`search_diagram` had never returned an answer** — it shared the grouped-results helper, but that endpoint returns a flat `entries` array, so every call threw `result.results is not iterable`. - **Two tools dropped data silently, with no `undefined` to give it away.** `participants` never rendered external identifiers at all — the endpoint returns `refEntities` (an array), never a singular `referenceEntity`, so UniProt accessions were simply absent. `search_facets` returned nothing but its heading: each facet is an object with an `available` list, so `.length` on it was `undefined` and every section was skipped as falsy. +- **`reactome_query`'s `attribute` argument had never worked.** `/data/query/{id}/{attribute}` responds with `text/plain`, and the client asks for `application/json`, so every attribute request came back HTTP 406. It now uses the text path. +- **`events_hierarchy` walked the entire tree.** It capped at three top-level pathways but then recursed every descendant, so those three rendered ~86 KB. It now takes `max_depth` (default 3) and `top_level_limit`, and says where it stopped — ~86 KB down to ~16 KB. +- **`reactome_query` returned pretty-printed JSON**, which cost ~23% more tokens for whitespace no model needs. Objects too large to return whole are now described — field names, shapes and sizes, with a pointer to the `attribute` argument — instead of being truncated into invalid JSON. Metabolism goes from 43,457 characters of severed object to 1,327 characters of usable map. - **`events_hierarchy` failed every call that did not name a species.** Its default was `"Homo sapiens"`, which that endpoint answers with HTTP 500; `"9606"` returns 200. - A failed Neo4j driver close rejected with nobody listening — `process.once` was handed an `async` function. - `fetchWithRetry` rethrew `lastError`, typed `unknown`, so a non-`Error` rejection reached callers as something they could not read `.message` off. ### Added +- **A cap on how much text one tool may return** (`MAX_TOOL_RESPONSE_CHARS`, default 40,000; override by env). Every tool result is spent from the caller's context window, and the size of several of these is driven by the ID asked about rather than by anything the tool decides: `reactome_events_hierarchy` rendered ~86 KB (~22,000 tokens) in a single call and `reactome_query` on Metabolism ~60 KB. The cap is applied once, in the wrapper every tool handler already passes through, rather than in 56 places — and it announces the cut rather than truncating silently, because a model that cannot see it was truncated reports the partial answer as the whole one. - **Spec Kit.** `.specify/` with a constitution written from failures this repository actually had, and `specs/` for design decisions. Spec 001 records what was found about response shapes; spec 002 states the transport and hosting question rather than answering it. The Spec Kit skills under `.claude/skills/` are tracked deliberately — people clone this repository and point an agent at it. - **A live sweep.** `npm run sweep` calls all 53 tools against the live services. It checks for `undefined`, `[object Object]` and empty bodies, and — because marker-grepping cannot see a field that was dropped cleanly — asserts expected content for 16 tools whose arguments are known to return data. Runs weekly and on demand, not in CI, since a red run there can mean Reactome changed rather than this repository did. - **ESLint with type-aware rules, and Prettier.** Plus coverage reporting with a ratchet threshold, and a `tsconfig.eslint.json` that finally includes `tests/` — which had been in no TypeScript project at all. @@ -21,7 +25,8 @@ All notable changes to this project are documented here. This project adheres to ### Changed - **zod 4**, **neo4j-driver 6**, **vitest 5**, **@types/node 26**. `z.record(v)` now requires an explicit key type; that was the only breaking change reaching this code. - CI now runs lint, format check, typecheck, **build** and coverage. The build had never run in CI. -- Test suite: 41 → 64 tests. +- **The test harness now runs zod validation.** `fake.invoke()` called handlers with raw arguments, so schemas never ran: defaults never materialised and invalid input was never rejected. No test had ever exercised the `nonEmptyString` validation, and a test could pass while the real server requested `/data/eventsHierarchy/undefined`. +- Test suite: 41 → 81 tests. - **TypeScript 5.9 → 7**, via the side-by-side arrangement the TypeScript team documents. The build and typecheck run TypeScript 7 (installed as the alias `typescript-7`); the package named `typescript` stays at 6.0.3 because that is the newest typescript-eslint supports. Builds go from ~2.6s to ~0.38s, and `npm run check` typechecks with both compilers so they cannot diverge silently. TypeScript 6 stopped auto-including `@types/*`, so `tsconfig.json` now names `"types": ["node"]` — the lint config already did. diff --git a/scripts/sweep-live.mjs b/scripts/sweep-live.mjs index 1932a2e..cfcd153 100644 --- a/scripts/sweep-live.mjs +++ b/scripts/sweep-live.mjs @@ -231,9 +231,11 @@ async function main() { suspicious.push([tool.name, hits.join(", "), line.trim().slice(0, 100)]); } else if (SERVICE_ERROR.test(text.trim())) { serviceErrors.push([tool.name, text.trim().slice(0, 120)]); - } else if (text.trim().split("\n").filter(Boolean).length <= 1) { - // A single line is a heading with no body -- either genuinely empty, or - // a section that was skipped because a field was read at the wrong path. + } else if (text.trim().split("\n").filter(Boolean).length <= 1 && text.trim().length < 400) { + // A single SHORT line is a heading with no body -- either genuinely + // empty, or a section skipped because a field was read at the wrong + // path. The length check matters: reactome_query returns compact JSON as + // one very long line, which is a full answer, not an empty one. suspicious.push([tool.name, "empty body", text.trim().slice(0, 100)]); } else { const missing = (EXPECT[tool.name] ?? []).filter(needle => !text.includes(needle)); diff --git a/src/config.ts b/src/config.ts index d5f3767..da83915 100644 --- a/src/config.ts +++ b/src/config.ts @@ -60,3 +60,22 @@ export const CYPHER_QUERY_TIMEOUT_MS = parsePositiveInt( process.env.CYPHER_QUERY_TIMEOUT_MS, 30_000 ); + +/** + * Backstop on how much text one tool may return. + * + * Every tool result is spent from the model's context window, and several here + * can be far larger than they look: `reactome_query` on Metabolism renders + * ~60 KB (~15k tokens) and `reactome_events_hierarchy` ~86 KB (~22k tokens), + * because the size is driven by the ID that was asked about rather than by + * anything the tool decides. A single call could crowd out the conversation it + * was meant to inform. + * + * This is a backstop, not a target -- tools should page or summarise long + * before reaching it. It matches the Cypher tool's existing total-size default, + * which had this guard from the start while the REST tools had none. + */ +export const MAX_TOOL_RESPONSE_CHARS = parsePositiveInt( + process.env.MAX_TOOL_RESPONSE_CHARS, + 40_000 +); diff --git a/src/response-limits.ts b/src/response-limits.ts new file mode 100644 index 0000000..061d1e7 --- /dev/null +++ b/src/response-limits.ts @@ -0,0 +1,52 @@ +import { MAX_TOOL_RESPONSE_CHARS } from "./config.js"; + +/** + * Truncate a tool response that would otherwise eat the caller's context. + * + * The cut is announced in the text rather than made silently: a model that + * cannot see it was truncated will report the partial answer as the whole one, + * which is the same class of quiet wrongness as a formatter reading the wrong + * field. The note names the tool so the model can narrow the request. + */ +export function capToolText(text: string, toolName: string, max = MAX_TOOL_RESPONSE_CHARS): string { + if (text.length <= max) return text; + + const notice = + `\n\n---\n` + + `*Truncated: ${toolName} returned ${text.length.toLocaleString()} characters, ` + + `over the ${max.toLocaleString()}-character limit. ` + + `${(text.length - max).toLocaleString()} characters were dropped. ` + + `Narrow the request — ask about a specific pathway or entity rather than a whole species or top-level pathway.*`; + + // Cut at a line boundary where one is close by, so the visible text does not + // end mid-token and read as corrupt. + const head = text.slice(0, Math.max(0, max - notice.length)); + const lastNewline = head.lastIndexOf("\n"); + const body = + lastNewline > head.length - 500 && lastNewline > 0 ? head.slice(0, lastNewline) : head; + + return body + notice; +} + +/** Apply the cap to an MCP tool result, leaving non-text content untouched. */ +export function capToolResult(result: unknown, toolName: string): unknown { + if (!result || typeof result !== "object") return result; + const r = result as { content?: Array<{ type?: string; text?: string }> }; + if (!Array.isArray(r.content)) return result; + + // Cap on the total across blocks: two 30 KB blocks cost the caller the same + // as one 60 KB block. + let budget = MAX_TOOL_RESPONSE_CHARS; + const total = r.content.reduce((n, b) => n + (typeof b.text === "string" ? b.text.length : 0), 0); + if (total <= budget) return result; + + const content = r.content.map(block => { + if (typeof block.text !== "string") return block; + if (budget <= 0) return { ...block, text: "" }; + const text = capToolText(block.text, toolName, budget); + budget -= text.length; + return { ...block, text }; + }); + + return { ...r, content: content.filter(b => b.text !== "") }; +} diff --git a/src/tools/index.ts b/src/tools/index.ts index 461ae15..9770199 100644 --- a/src/tools/index.ts +++ b/src/tools/index.ts @@ -13,6 +13,8 @@ import { registerInteractorTools } from "./interactors.js"; import { registerCypherTools } from "./cypher.js"; import { isNeo4jConfigured } from "../clients/neo4j.js"; import { withNewRequestContext } from "../context.js"; +import { capToolResult } from "../response-limits.js"; +import { MAX_TOOL_RESPONSE_CHARS } from "../config.js"; /** * Wrap `server.tool` so every handler runs inside a fresh request context. @@ -20,7 +22,15 @@ import { withNewRequestContext } from "../context.js"; * log line emitted during that invocation — giving one correlation handle * across tool → client → retry → error log. */ -function installRequestContextWrapper(server: McpServer) { +/** + * Wrap every tool handler once, at registration. + * + * Two things ride here: a fresh request context so log lines from one call can + * be grepped together, and a cap on how much text the call may return. The cap + * belongs at this single point rather than in 56 handlers -- a per-tool guard + * is a guard somebody forgets to add to the fifty-seventh. + */ +function installToolWrapper(server: McpServer) { const original = server.tool.bind(server); // The SDK's tool() is an overloaded method; we only ever call the 4-arg // form (name, description, schema, handler). Keep the wrapper permissive. @@ -29,15 +39,20 @@ function installRequestContextWrapper(server: McpServer) { if (typeof handler !== "function") { return (original as (...a: unknown[]) => unknown)(...args); } - const wrapped = (params: unknown) => - withNewRequestContext(() => (handler as (p: unknown) => unknown)(params)); + const toolName = typeof args[0] === "string" ? args[0] : "tool"; + const wrapped = async (params: unknown) => { + const result = await withNewRequestContext(() => + (handler as (p: unknown) => unknown)(params) + ); + return capToolResult(result, toolName); + }; const nextArgs = [...args.slice(0, -1), wrapped]; return (original as (...a: unknown[]) => unknown)(...nextArgs); }; } export function registerAllTools(server: McpServer) { - installRequestContextWrapper(server); + installToolWrapper(server); registerAnalysisTools(server); registerPathwayTools(server); @@ -55,6 +70,45 @@ export function registerAllTools(server: McpServer) { registerUtilityTools(server); } +/** + * Describe a database object that is too large to return, so the caller can ask + * again for the part it wants via `reactome_query`'s `attribute` argument. + */ +function summariseLargeObject( + id: string, + result: Record, + totalChars: number +): string { + const describe = (value: unknown): string => { + if (Array.isArray(value)) return `array of ${value.length}`; + if (value === null) return "null"; + if (typeof value === "object") return "object"; + // Not String(value): these come off an untyped JSON object, and String() + // on one renders "[object Object]" -- the same quiet wrongness this repo + // has shipped before. + const text = JSON.stringify(value) ?? typeof value; + return text.length > 60 ? `${typeof value}, ${text.length} chars` : text; + }; + + const entries = Object.entries(result) + .map(([key, value]) => [key, describe(value), JSON.stringify(value)?.length ?? 0] as const) + .sort((a, b) => b[2] - a[2]); + + return [ + `## ${typeof result.displayName === "string" ? result.displayName : id}`, + "", + `This object is ${totalChars.toLocaleString()} characters — too large to return whole ` + + `(limit ${MAX_TOOL_RESPONSE_CHARS.toLocaleString()}). Its fields are listed below.`, + "", + `**Ask for one field** with \`reactome_query\` and the \`attribute\` argument, ` + + `e.g. \`{ id: "${id}", attribute: "${entries[0]?.[0] ?? "displayName"}" }\`.`, + "", + "| field | contents | size |", + "| --- | --- | ---: |", + ...entries.map(([key, shape, size]) => `| \`${key}\` | ${shape} | ${size.toLocaleString()} |`), + ].join("\n"); +} + function registerUtilityTools(server: McpServer) { // Get species list server.tool( @@ -261,14 +315,36 @@ function registerUtilityTools(server: McpServer) { attribute: nonEmptyString.optional().describe("Specific attribute to retrieve (optional)"), }, async ({ id, attribute }) => { - const endpoint = attribute - ? `/data/query/${encodeURIComponent(id)}/${encodeURIComponent(attribute)}` - : `/data/query/enhanced/${encodeURIComponent(id)}`; + // A single attribute comes back as text/plain, not JSON. Asking for it + // with Accept: application/json is answered with HTTP 406, so this + // argument had never worked -- every attribute request failed. + if (attribute) { + const value = await contentClient.getText( + `/data/query/${encodeURIComponent(id)}/${encodeURIComponent(attribute)}` + ); + return { + content: [{ type: "text", text: `**${attribute}** of ${id}:\n\n${value}` }], + }; + } - const result = await contentClient.get>(endpoint); + const result = await contentClient.get>( + `/data/query/enhanced/${encodeURIComponent(id)}` + ); + + // Compact, not indented. This endpoint returns whole database objects -- + // Metabolism is ~48 KB pretty-printed. Dropping the indentation saves + // ~23%, and a model does not need the whitespace. + const json = JSON.stringify(result); + if (json.length <= MAX_TOOL_RESPONSE_CHARS) { + return { content: [{ type: "text", text: json }] }; + } + // Too big to return whole. Truncating would hand back invalid JSON and + // still spend the whole budget, so describe the object's shape instead + // and point at the `attribute` argument this tool already accepts. A map + // of what is available is worth more than 40 KB of a severed object. return { - content: [{ type: "text", text: JSON.stringify(result, null, 2) }], + content: [{ type: "text", text: summariseLargeObject(id, result, json.length) }], }; } ); diff --git a/src/tools/pathway.ts b/src/tools/pathway.ts index 1fed1f7..7e16790 100644 --- a/src/tools/pathway.ts +++ b/src/tools/pathway.ts @@ -58,14 +58,29 @@ function formatPathway(pathway: Pathway | Event): string { return lines.join("\n"); } -function formatEventHierarchy(event: EventHierarchy, indent = 0): string[] { +/** + * Render a hierarchy node and its descendants, to `maxDepth` levels. + * + * The depth limit is the point of this function. Without one it walked the + * whole tree: three top-level human pathways rendered ~86 KB, roughly 22,000 + * tokens spent on a single call, most of it reactions nobody asked about. + */ +function formatEventHierarchy(event: EventHierarchy, indent = 0, maxDepth = 3): string[] { const prefix = " ".repeat(indent); const lines = [`${prefix}- **${event.name}** (${event.stId}) [${event.type}]`]; - if (event.children) { - for (const child of event.children) { - lines.push(...formatEventHierarchy(child, indent + 1)); - } + const children = event.children ?? []; + if (children.length === 0) return lines; + + if (indent >= maxDepth) { + lines.push( + `${prefix} - *(${children.length} more below this level — use reactome_pathway_contained_events on ${event.stId})*` + ); + return lines; + } + + for (const child of children) { + lines.push(...formatEventHierarchy(child, indent + 1, maxDepth)); } return lines; @@ -285,8 +300,26 @@ export function registerPathwayTools(server: McpServer) { .describe( "Species taxonomy ID (e.g. 9606). Names are accepted by the API but are unreliable here -- prefer the ID." ), + max_depth: z + .number() + .int() + .min(1) + .max(10) + .optional() + .default(3) + .describe( + "How many levels of the tree to render (default 3). Deeper trees get large fast." + ), + top_level_limit: z + .number() + .int() + .min(1) + .max(30) + .optional() + .default(3) + .describe("How many top-level pathways to render (default 3)."), }, - async ({ species }) => { + async ({ species, max_depth, top_level_limit }) => { const hierarchy = await contentClient.get( `/data/eventsHierarchy/${encodeURIComponent(species)}` ); @@ -297,14 +330,13 @@ export function registerPathwayTools(server: McpServer) { "", ]; - // Show first 3 top-level pathways with their immediate children - hierarchy.slice(0, 3).forEach(top => { - lines.push(...formatEventHierarchy(top, 0)); + hierarchy.slice(0, top_level_limit).forEach(top => { + lines.push(...formatEventHierarchy(top, 0, max_depth)); lines.push(""); }); - if (hierarchy.length > 3) { - lines.push(`... and ${hierarchy.length - 3} more top-level pathways`); + if (hierarchy.length > top_level_limit) { + lines.push(`... and ${hierarchy.length - top_level_limit} more top-level pathways`); } lines.push( diff --git a/tests/helpers/fake-server.ts b/tests/helpers/fake-server.ts index 5deddac..a235d47 100644 --- a/tests/helpers/fake-server.ts +++ b/tests/helpers/fake-server.ts @@ -1,4 +1,5 @@ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { z, type ZodTypeAny } from "zod"; export interface CapturedTool { name: string; @@ -39,10 +40,27 @@ export function createFakeServer() { return [...tools.keys()]; } + /** + * Call a tool the way the SDK does: validate the arguments against the + * registered zod schema first, then hand the parsed result to the handler. + * + * Calling the handler with raw params -- which this helper used to do -- + * skips zod entirely, so defaults never materialise and invalid input is + * never rejected. A test could then pass while the real server returned + * `/data/eventsHierarchy/undefined`, and none of the `nonEmptyString` + * validation was exercised at all. + */ function invoke(name: string, params: Record = {}) { const tool = tools.get(name); if (!tool) throw new Error(`tool not registered: ${name}`); - return tool.handler(params); + + const shape = tool.schema as Record; + const parsed = + shape && Object.keys(shape).length > 0 + ? (z.object(shape).parse(params) as Record) + : params; + + return tool.handler(parsed); } function readResource(uri: string) { diff --git a/tests/response-limits.test.ts b/tests/response-limits.test.ts new file mode 100644 index 0000000..c84bc0e --- /dev/null +++ b/tests/response-limits.test.ts @@ -0,0 +1,192 @@ +/** + * Response size is a correctness concern for an MCP server, not a cosmetic + * one: every character a tool returns is spent from the caller's context + * window. Before these limits, one `reactome_events_hierarchy` call rendered + * ~86 KB (~22,000 tokens) and `reactome_query` on Metabolism ~60 KB, because + * the size is driven by the ID asked about rather than by anything the tool + * decides. + */ +import { describe, it, expect, vi, beforeEach, afterEach, type MockInstance } from "vitest"; +import { capToolText, capToolResult } from "../src/response-limits.js"; +import { MAX_TOOL_RESPONSE_CHARS } from "../src/config.js"; +import { createFakeServer, textOf, calledUrl } from "./helpers/fake-server.js"; +import { registerPathwayTools } from "../src/tools/pathway.js"; + +function jsonResponse(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { "content-type": "application/json" }, + }); +} + +describe("capToolText", () => { + it("leaves text under the limit exactly as it was", () => { + const text = "a".repeat(100); + expect(capToolText(text, "some_tool", 1000)).toBe(text); + }); + + it("leaves text at exactly the limit alone", () => { + const text = "a".repeat(1000); + expect(capToolText(text, "some_tool", 1000)).toBe(text); + }); + + it("announces the cut rather than making it silently", () => { + const capped = capToolText("a".repeat(5000), "reactome_query", 1000); + + // A model that cannot see it was truncated reports the partial answer as + // the whole one. + expect(capped).toContain("Truncated"); + expect(capped).toContain("reactome_query"); + expect(capped).toContain("5,000"); + expect(capped.length).toBeLessThanOrEqual(1000); + }); + + it("names how much was dropped and what to do instead", () => { + const capped = capToolText("x".repeat(3000), "reactome_events_hierarchy", 1000); + expect(capped).toContain("Narrow the request"); + }); + + it("prefers to cut at a line boundary", () => { + const text = Array.from({ length: 200 }, (_, i) => `line ${i} of some content here`).join("\n"); + const capped = capToolText(text, "t", 900); + const body = capped.split("\n\n---\n")[0] ?? ""; + // The visible body should not end mid-line. + expect(text.split("\n")).toContain(body.split("\n").at(-1)); + }); +}); + +describe("capToolResult", () => { + it("passes a small result through untouched", () => { + const result = { content: [{ type: "text", text: "small" }] }; + expect(capToolResult(result, "t")).toBe(result); + }); + + it("caps a result over the limit", () => { + const result = { + content: [{ type: "text", text: "a".repeat(MAX_TOOL_RESPONSE_CHARS + 5000) }], + }; + const capped = capToolResult(result, "t") as { content: Array<{ text: string }> }; + + expect(capped.content[0]!.text.length).toBeLessThanOrEqual(MAX_TOOL_RESPONSE_CHARS); + expect(capped.content[0]!.text).toContain("Truncated"); + }); + + it("budgets across blocks, since two large blocks cost the same as one", () => { + const half = MAX_TOOL_RESPONSE_CHARS; + const result = { + content: [ + { type: "text", text: "a".repeat(half) }, + { type: "text", text: "b".repeat(half) }, + ], + }; + const capped = capToolResult(result, "t") as { content: Array<{ text: string }> }; + const total = capped.content.reduce((n, b) => n + b.text.length, 0); + + expect(total).toBeLessThanOrEqual(MAX_TOOL_RESPONSE_CHARS); + }); + + it("leaves a result with no text content alone", () => { + const result = { content: [{ type: "image", data: "..." }] }; + expect(capToolResult(result, "t")).toBe(result); + }); + + it("does not throw on a malformed result", () => { + expect(() => capToolResult(null, "t")).not.toThrow(); + expect(() => capToolResult({ nope: true }, "t")).not.toThrow(); + }); +}); + +describe("events hierarchy depth", () => { + let fetchSpy: MockInstance; + const fake = createFakeServer(); + registerPathwayTools(fake.server); + + beforeEach(() => { + fetchSpy = vi.spyOn(globalThis, "fetch"); + }); + afterEach(() => { + fetchSpy.mockRestore(); + }); + + /** A tree deep enough that rendering all of it would be the old behaviour. */ + function deepTree(depth: number, breadth = 2): unknown { + const node = (level: number, path: string): unknown => ({ + stId: `R-HSA-${path}`, + name: `Level ${level} node ${path}`, + type: "Pathway", + children: + level >= depth + ? undefined + : Array.from({ length: breadth }, (_, i) => node(level + 1, `${path}${i}`)), + }); + return [node(0, "0")]; + } + + it("stops at the requested depth instead of walking the whole tree", async () => { + fetchSpy.mockResolvedValueOnce(jsonResponse(deepTree(8))); + + const text = textOf(await fake.invoke("reactome_events_hierarchy", { max_depth: 2 })); + + expect(text).toContain("Level 0"); + expect(text).toContain("Level 2"); + expect(text).not.toContain("Level 4"); + // And says why it stopped, pointing at the tool that goes deeper. + expect(text).toContain("reactome_pathway_contained_events"); + }); + + it("renders a deeper tree when asked", async () => { + fetchSpy.mockResolvedValueOnce(jsonResponse(deepTree(8))); + + const text = textOf(await fake.invoke("reactome_events_hierarchy", { max_depth: 5 })); + expect(text).toContain("Level 5"); + expect(text).not.toContain("Level 7"); + }); + + it("defaults to the taxonomy ID, which is the form that endpoint answers", async () => { + fetchSpy.mockResolvedValueOnce(jsonResponse(deepTree(1))); + + await fake.invoke("reactome_events_hierarchy", {}); + // "Homo sapiens" is answered with HTTP 500 by this endpoint; "9606" is not. + expect(calledUrl(fetchSpy.mock.calls)).toContain("9606"); + }); +}); + +describe("argument validation", () => { + const fake = createFakeServer(); + registerPathwayTools(fake.server); + + // These assertions were impossible until the fake server started running the + // zod schema: it called handlers with raw params, so validation never ran. + + it("rejects a blank id rather than requesting /data/query/enhanced/", () => { + expect(() => fake.invoke("reactome_get_pathway", { id: " " })).toThrow(); + }); + + it("rejects an empty id", () => { + expect(() => fake.invoke("reactome_get_pathway", { id: "" })).toThrow(); + }); + + it("trims a padded id instead of sending the padding", async () => { + const fetchSpy = vi.spyOn(globalThis, "fetch").mockResolvedValueOnce( + jsonResponse({ + dbId: 1, + stId: "R-HSA-109582", + displayName: "Hemostasis", + schemaClass: "Pathway", + }) + ); + try { + await fake.invoke("reactome_get_pathway", { id: " R-HSA-109582 " }); + const url = calledUrl(fetchSpy.mock.calls); + expect(url).toContain("R-HSA-109582"); + expect(url).not.toContain("%20"); + } finally { + fetchSpy.mockRestore(); + } + }); + + it("rejects a max_depth outside the allowed range", () => { + expect(() => fake.invoke("reactome_events_hierarchy", { max_depth: 99 })).toThrow(); + expect(() => fake.invoke("reactome_events_hierarchy", { max_depth: 0 })).toThrow(); + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index bc882ee..bc00228 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -16,10 +16,10 @@ export default defineConfig({ // tools still have no test, which is how a token-parsing bug and nine // wrong field paths all shipped unnoticed. thresholds: { - lines: 50, - functions: 46, - branches: 42, - statements: 50, + lines: 52, + functions: 48, + branches: 45, + statements: 53, }, }, },