|
| 1 | +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. |
| 2 | + |
| 3 | +/** |
| 4 | + * #17568 — a refusal for a MISSING required key must also name the UNEXPECTED |
| 5 | + * key the caller actually sent. |
| 6 | + * |
| 7 | + * THE DEFECT, measured on a real Claude Code client against a real service |
| 8 | + * across three recordings. `update_record`'s parameter is `recordId`, which |
| 9 | + * induces callers to send `id` — `id` is what most read/write paths use to mean |
| 10 | + * a record's primary key — and in take v4, 5 of 7 `update_record` call sites |
| 11 | + * sent it. The refusal was: |
| 12 | + * |
| 13 | + * Invalid input: expected string, received undefined at recordId |
| 14 | + * |
| 15 | + * which names the field the caller believes they ALREADY SUPPLIED. So the |
| 16 | + * surface creates the mistake and then confirms it: an agent reads "you are |
| 17 | + * missing a field you know you sent" as a VALUE problem, not a KEY problem, and |
| 18 | + * retries the same shape with a different value forever. That is the same |
| 19 | + * failure class the card cites from its sibling — an error naming the field |
| 20 | + * whose presence the caller has already decided is correct. |
| 21 | + * |
| 22 | + * WHAT THIS FILE PINS, AND WHY IT IS A PIN AND NOT A FIX. The diagnostic is |
| 23 | + * already correct on this tree: #16913 closed every tool's argument shape with |
| 24 | + * `strictToolInput` and gave the record-scoped tools the alias table |
| 25 | + * `RECORD_ID_ALIASES` (`id`, `record_id` → `recordId`), so the compound case now |
| 26 | + * answers BOTH facts in one message. Nothing pinned that, and the green is the |
| 27 | + * intersection of three independent mechanisms, any one of which can regress |
| 28 | + * silently while every existing gate stays green: |
| 29 | + * |
| 30 | + * 1. zod reports BOTH issues for a strict object — the `unrecognized_keys` for |
| 31 | + * `id` and the `invalid_type` for the absent `recordId`. A schema that |
| 32 | + * short-circuits on the first issue drops the half that carries the news. |
| 33 | + * 2. the MCP SDK's `validateToolInput()` joins every issue into one message. |
| 34 | + * A change that renders only `issues[0]` restores the card's defect |
| 35 | + * verbatim. |
| 36 | + * 3. the alias table carries an `id → recordId` entry at all. It is one line |
| 37 | + * in `mcp-http-tools.ts` and reads as decoration next to the tool it |
| 38 | + * annotates. |
| 39 | + * |
| 40 | + * `mcp-http-tools.unknown-argument-keys.test.ts` is the nearest sibling and does |
| 41 | + * NOT cover this: every case there sends a complete set of VALID arguments plus |
| 42 | + * one probe key, so the unexpected-key half never has to coexist with a missing |
| 43 | + * required key. This file is the compound case, which is the one the card |
| 44 | + * measured. |
| 45 | + * |
| 46 | + * ⛔ THE DIRECTION IS THE DIAGNOSTIC ONLY. The ruling on this card refused |
| 47 | + * accepting `id` as an alias for `recordId` — an alias is consumer-side |
| 48 | + * tolerance, it makes the wrong key silently work so the caller never learns the |
| 49 | + * parameter's name, and it widens the accepted set of a published MCP surface |
| 50 | + * permanently. It also refused renaming `recordId` to `id` (a breaking change to |
| 51 | + * a published tool parameter). So the accept set must stay exactly where it is, |
| 52 | + * and the pin has to be able to SEE that: `the accept set is unchanged` below |
| 53 | + * asserts that the `id` spelling never reaches the bridge and that the declared |
| 54 | + * spelling still arrives with the exact arguments sent. A test that only looked |
| 55 | + * for the new sentence would pass just as well against an implementation that |
| 56 | + * had added the alias. |
| 57 | + * |
| 58 | + * MINIMUM ASSERTIONS ON A REFUSAL. There is no ADR-0112 envelope on this wire — |
| 59 | + * MCP answers a rejected `tools/call` as a TOOL ERROR, so the `code` + `status` |
| 60 | + * pair is asserted in this surface's own vocabulary: the protocol code |
| 61 | + * `-32602` (invalid params) carried in the message, and `result.isError: true` |
| 62 | + * with `error` ABSENT, which is this transport's promise that a refusal is |
| 63 | + * returned rather than thrown across the wire. The HTTP status stays `200` |
| 64 | + * because JSON-RPC carries its own outcome; that is pinned too, so a later |
| 65 | + * change cannot turn a tool error into a transport error unnoticed. |
| 66 | + * |
| 67 | + * ⚠️ WHY EVERY KEY ASSERTION USES THE BACKTICK FORM. A bare |
| 68 | + * `expect(text).toContain('id')` is satisfied by the word **Invalid** in |
| 69 | + * `Invalid input` — it passes against the exact defect this file exists to |
| 70 | + * catch. The refusal quotes an offending key as `` `id` ``, so the quoted form |
| 71 | + * is what is asserted, and the ordinary missing-key refusal (nothing offending |
| 72 | + * to quote) is asserted to carry NO backticks at all rather than to lack some |
| 73 | + * particular sentence. |
| 74 | + * |
| 75 | + * ⛔ The prose is deliberately not pinned — only the named subjects: which keys |
| 76 | + * the message names, and in which quoting position. |
| 77 | + */ |
| 78 | + |
| 79 | +import { describe, it, expect, beforeEach } from 'vitest'; |
| 80 | +import { PassThrough } from 'node:stream'; |
| 81 | +import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js'; |
| 82 | + |
| 83 | +import { MCPServerRuntime } from './mcp-server-runtime.js'; |
| 84 | +import type { McpActionBridge, McpDataBridge } from './mcp-http-tools.js'; |
| 85 | + |
| 86 | +/** The protocol code for invalid params — the `code` half of the refusal pin. */ |
| 87 | +const INVALID_PARAMS = '-32602'; |
| 88 | + |
| 89 | +/** The record id from the card's recording, kept so the repro reads as itself. */ |
| 90 | +const RECORD_ID = '_IFp-Bf8yQcrfV8o'; |
| 91 | + |
| 92 | +interface StubBridge extends McpDataBridge, McpActionBridge { |
| 93 | + calls: unknown[][]; |
| 94 | +} |
| 95 | + |
| 96 | +function makeBridge(): StubBridge { |
| 97 | + const calls: unknown[][] = []; |
| 98 | + return { |
| 99 | + calls, |
| 100 | + async listObjects() { calls.push(['listObjects']); return [{ name: 'crm_lead', label: 'Lead' }]; }, |
| 101 | + async describeObject(name: string) { calls.push(['describeObject', name]); return { name, fields: [] }; }, |
| 102 | + async query(object: string, opts: unknown) { calls.push(['query', object, opts]); return { object, records: [] }; }, |
| 103 | + async get(object: string, id: string) { calls.push(['get', object, id]); return { id }; }, |
| 104 | + async aggregate(object: string, opts: unknown) { calls.push(['aggregate', object, opts]); return []; }, |
| 105 | + async create(object: string, data: unknown) { calls.push(['create', object, data]); return { object, id: 'n1' }; }, |
| 106 | + async update(object: string, id: string, data: unknown) { calls.push(['update', object, id, data]); return { object, id }; }, |
| 107 | + async remove(object: string, id: string) { calls.push(['remove', object, id]); return { object, id, success: true }; }, |
| 108 | + async listActions() { calls.push(['listActions']); return [{ name: 'complete_task', objectName: 'crm_lead' }]; }, |
| 109 | + async runAction(name: string, input: unknown) { calls.push(['runAction', name, input]); return { ok: true }; }, |
| 110 | + }; |
| 111 | +} |
| 112 | + |
| 113 | +interface ToolOutcome { |
| 114 | + /** `true` when the call was refused as a tool error. */ |
| 115 | + refused: boolean; |
| 116 | + /** The refusal (or success) text the client actually reads. */ |
| 117 | + text: string; |
| 118 | + /** The HTTP status the JSON-RPC envelope arrived under. */ |
| 119 | + status: number; |
| 120 | + /** A protocol-level error, which this transport promises never to throw. */ |
| 121 | + protocolError: unknown; |
| 122 | +} |
| 123 | + |
| 124 | +let nextId = 1; |
| 125 | + |
| 126 | +async function callTool( |
| 127 | + runtime: MCPServerRuntime, |
| 128 | + bridge: StubBridge, |
| 129 | + name: string, |
| 130 | + args: unknown, |
| 131 | +): Promise<ToolOutcome> { |
| 132 | + const body = { jsonrpc: '2.0', id: nextId++, method: 'tools/call', params: { name, arguments: args } }; |
| 133 | + const req = new Request('http://localhost/api/v1/mcp', { |
| 134 | + method: 'POST', |
| 135 | + headers: { 'content-type': 'application/json', accept: 'application/json, text/event-stream' }, |
| 136 | + body: JSON.stringify(body), |
| 137 | + }); |
| 138 | + const res = await runtime.handleHttpRequest(req, { bridge, parsedBody: body }); |
| 139 | + const json = (await res.json()) as { |
| 140 | + result?: { isError?: boolean; content?: { text?: string }[] }; |
| 141 | + error?: unknown; |
| 142 | + }; |
| 143 | + return { |
| 144 | + refused: json.result?.isError === true || Boolean(json.error), |
| 145 | + text: String(json.result?.content?.map((c) => c.text).join('\n') ?? ''), |
| 146 | + status: res.status, |
| 147 | + protocolError: json.error ?? undefined, |
| 148 | + }; |
| 149 | +} |
| 150 | + |
| 151 | +describe('#17568 a missing required key names the unexpected key that was sent', () => { |
| 152 | + let runtime: MCPServerRuntime; |
| 153 | + let bridge: StubBridge; |
| 154 | + |
| 155 | + beforeEach(() => { |
| 156 | + runtime = new MCPServerRuntime({ name: 't', version: '1.0.0' }); |
| 157 | + bridge = makeBridge(); |
| 158 | + }); |
| 159 | + |
| 160 | + // ── The card's repro, and the named acceptance criterion ────────────────── |
| 161 | + |
| 162 | + it('the reported `id` call is refused naming BOTH the missing `recordId` and the `id` it received', async () => { |
| 163 | + const { refused, text, status, protocolError } = await callTool(runtime, bridge, 'update_record', { |
| 164 | + objectName: 'crm_lead', |
| 165 | + id: RECORD_ID, |
| 166 | + data: { status: 'qualified' }, |
| 167 | + }); |
| 168 | + |
| 169 | + // The refusal pin's `code` + `status` half, in this surface's vocabulary. |
| 170 | + expect(refused).toBe(true); |
| 171 | + expect(text).toContain(INVALID_PARAMS); |
| 172 | + expect(protocolError).toBeUndefined(); |
| 173 | + expect(status).toBe(200); |
| 174 | + |
| 175 | + // Fact one: the declared key that is missing is located. |
| 176 | + expect(text).toContain('recordId'); |
| 177 | + // Fact two — the whole card. The key the caller BELIEVES they supplied is |
| 178 | + // named back to them as a key, so the mistake reads as a KEY mistake and |
| 179 | + // not as a value the caller should vary. Backticked: the bare substring |
| 180 | + // `id` is present in the word "Invalid" even in the defective message. |
| 181 | + expect(text).toContain('`id`'); |
| 182 | + // And the canonical spelling is prescribed as a key, not merely mentioned |
| 183 | + // as the zod path of the missing-key complaint. |
| 184 | + expect(text).toContain('`recordId`'); |
| 185 | + |
| 186 | + // Nothing ran. The refusal is the whole answer. |
| 187 | + expect(bridge.calls).toEqual([]); |
| 188 | + }); |
| 189 | + |
| 190 | + // ── Negative one: the accept set is unchanged ───────────────────────────── |
| 191 | + |
| 192 | + it('the accept set is unchanged — the declared spelling still succeeds with the exact arguments sent', async () => { |
| 193 | + const { refused, status } = await callTool(runtime, bridge, 'update_record', { |
| 194 | + objectName: 'crm_lead', |
| 195 | + recordId: RECORD_ID, |
| 196 | + data: { status: 'qualified' }, |
| 197 | + }); |
| 198 | + expect(refused).toBe(false); |
| 199 | + expect(status).toBe(200); |
| 200 | + // Not "it did not refuse" — the bridge received precisely the call. |
| 201 | + expect(bridge.calls).toEqual([['update', 'crm_lead', RECORD_ID, { status: 'qualified' }]]); |
| 202 | + }); |
| 203 | + |
| 204 | + it('the accept set is unchanged — `id` is never accepted as a spelling of `recordId`', async () => { |
| 205 | + // The counterpart of the case above, and the reason a message-only assertion |
| 206 | + // is not enough: had the refused direction been "fixed" by accepting `id` as |
| 207 | + // an alias, the call would have reached the bridge with the record id. The |
| 208 | + // ruling on this card refused that direction, so the bridge stays untouched. |
| 209 | + await callTool(runtime, bridge, 'update_record', { |
| 210 | + objectName: 'crm_lead', |
| 211 | + id: RECORD_ID, |
| 212 | + data: { status: 'qualified' }, |
| 213 | + }); |
| 214 | + expect(bridge.calls.find((c) => c[0] === 'update')).toBeUndefined(); |
| 215 | + }); |
| 216 | + |
| 217 | + // ── Negative two: no invented unexpected-key clause ─────────────────────── |
| 218 | + |
| 219 | + it('with NEITHER key the ordinary missing-`recordId` refusal quotes no key at all', async () => { |
| 220 | + const { refused, text } = await callTool(runtime, bridge, 'update_record', { |
| 221 | + objectName: 'crm_lead', |
| 222 | + data: { status: 'qualified' }, |
| 223 | + }); |
| 224 | + expect(refused).toBe(true); |
| 225 | + expect(text).toContain(INVALID_PARAMS); |
| 226 | + expect(text).toContain('recordId'); |
| 227 | + // There is no offending key, so nothing is quoted as one. Asserted as the |
| 228 | + // ABSENCE OF ANY quoted key rather than of one particular sentence: an |
| 229 | + // unexpected-key clause invented for a payload that carried no unexpected |
| 230 | + // key would have to quote something, whatever the wording. |
| 231 | + expect(text).not.toContain('`'); |
| 232 | + expect(bridge.calls).toEqual([]); |
| 233 | + }); |
| 234 | + |
| 235 | + // ── The class, not the literal `id`/`recordId` pair ─────────────────────── |
| 236 | + |
| 237 | + it('`record_id` — the other induced spelling — is answered the same way', async () => { |
| 238 | + // The defect class is "the error names the field the caller believes they |
| 239 | + // sent", not one pair of spellings. This case shares no literal with the one |
| 240 | + // above, so it goes red if the seam is ever narrowed to a hard-coded `id`. |
| 241 | + const { refused, text } = await callTool(runtime, bridge, 'update_record', { |
| 242 | + objectName: 'crm_lead', |
| 243 | + record_id: RECORD_ID, |
| 244 | + data: { status: 'qualified' }, |
| 245 | + }); |
| 246 | + expect(refused).toBe(true); |
| 247 | + expect(text).toContain('`record_id`'); |
| 248 | + expect(text).toContain('`recordId`'); |
| 249 | + expect(bridge.calls).toEqual([]); |
| 250 | + }); |
| 251 | + |
| 252 | + // ── Reach: the sibling record-scoped tools, measured rather than assumed ── |
| 253 | + |
| 254 | + it.each(['get_record', 'delete_record'])( |
| 255 | + '%s inherits the same compound refusal', |
| 256 | + async (tool) => { |
| 257 | + const { refused, text } = await callTool(runtime, bridge, tool, { |
| 258 | + objectName: 'crm_lead', |
| 259 | + id: RECORD_ID, |
| 260 | + }); |
| 261 | + expect(refused).toBe(true); |
| 262 | + expect(text).toContain(INVALID_PARAMS); |
| 263 | + expect(text).toContain('recordId'); |
| 264 | + expect(text).toContain('`id`'); |
| 265 | + expect(text).toContain('`recordId`'); |
| 266 | + expect(bridge.calls).toEqual([]); |
| 267 | + }, |
| 268 | + ); |
| 269 | + |
| 270 | + it('`run_action`, where `recordId` is OPTIONAL, names the sent `id` and claims nothing is missing', async () => { |
| 271 | + // The asymmetry is the point, and it is why the reach is pinned per tool |
| 272 | + // rather than swept: `run_action` declares `recordId` optional, so no |
| 273 | + // required key is absent and there is no missing-key half to report. The |
| 274 | + // key mistake is still named, which is what breaks the retry loop. |
| 275 | + const { refused, text } = await callTool(runtime, bridge, 'run_action', { |
| 276 | + actionName: 'complete_task', |
| 277 | + objectName: 'crm_lead', |
| 278 | + id: RECORD_ID, |
| 279 | + }); |
| 280 | + expect(refused).toBe(true); |
| 281 | + expect(text).toContain('`id`'); |
| 282 | + expect(text).toContain('`recordId`'); |
| 283 | + // No invented missing-key complaint: nothing was required and absent. |
| 284 | + expect(text).not.toContain('received undefined'); |
| 285 | + expect(bridge.calls).toEqual([]); |
| 286 | + }); |
| 287 | + |
| 288 | + it('`create_record`, which declares no `recordId`, names `id` and prescribes nothing', async () => { |
| 289 | + // The control on the prescription channel: `recordId` is not a key on this |
| 290 | + // tool, so suggesting it would be wrong. The offending key is still echoed. |
| 291 | + const { refused, text } = await callTool(runtime, bridge, 'create_record', { |
| 292 | + objectName: 'crm_lead', |
| 293 | + id: RECORD_ID, |
| 294 | + data: { status: 'qualified' }, |
| 295 | + }); |
| 296 | + expect(refused).toBe(true); |
| 297 | + expect(text).toContain('`id`'); |
| 298 | + expect(text).not.toContain('recordId'); |
| 299 | + expect(bridge.calls).toEqual([]); |
| 300 | + }); |
| 301 | +}); |
| 302 | + |
| 303 | +// --------------------------------------------------------------------------- |
| 304 | +// The same repro over the wire a desktop MCP host actually uses |
| 305 | +// --------------------------------------------------------------------------- |
| 306 | + |
| 307 | +/** |
| 308 | + * WHY THE STDIO HALF IS HERE. The card was measured on a real client, and this |
| 309 | + * package has already shipped one outage in which the two transports served |
| 310 | + * DIFFERENT surfaces while every per-surface pin stayed green (#8034: `tools/call` |
| 311 | + * answered `-32601` over stdio while HTTP served the full set). The tools are |
| 312 | + * wired by one composition, so the diagnostic should be identical — "should be" |
| 313 | + * is exactly the claim that outage falsified, so it is asserted on the wire. |
| 314 | + */ |
| 315 | +describe('#17568 the same refusal arrives over the stdio transport', () => { |
| 316 | + it('names both facts on a real StdioServerTransport', async () => { |
| 317 | + const runtime = new MCPServerRuntime({ name: 't', version: '1.0.0' }); |
| 318 | + const bridge = makeBridge(); |
| 319 | + await runtime.bridgeDataTools(bridge); |
| 320 | + |
| 321 | + const serverStdin = new PassThrough(); |
| 322 | + const serverStdout = new PassThrough(); |
| 323 | + const transport = new StdioServerTransport(serverStdin, serverStdout); |
| 324 | + await (runtime as unknown as { mcpServer: { connect(t: unknown): Promise<void> } }).mcpServer.connect(transport); |
| 325 | + |
| 326 | + let buffered = ''; |
| 327 | + const waiting = new Map<number, (frame: Record<string, any>) => void>(); |
| 328 | + serverStdout.on('data', (chunk: Buffer | string) => { |
| 329 | + buffered += String(chunk); |
| 330 | + let newline = buffered.indexOf('\n'); |
| 331 | + while (newline >= 0) { |
| 332 | + const line = buffered.slice(0, newline).trim(); |
| 333 | + buffered = buffered.slice(newline + 1); |
| 334 | + newline = buffered.indexOf('\n'); |
| 335 | + if (!line) continue; |
| 336 | + try { |
| 337 | + const frame = JSON.parse(line) as { id?: number }; |
| 338 | + // Not a frame? A host printing to stdout is someone else's defect — |
| 339 | + // skipped rather than failed, as `mcp-stdio-tools.test.ts` does. |
| 340 | + const resolve = typeof frame.id === 'number' ? waiting.get(frame.id) : undefined; |
| 341 | + if (resolve && typeof frame.id === 'number') { |
| 342 | + waiting.delete(frame.id); |
| 343 | + resolve(frame as Record<string, any>); |
| 344 | + } |
| 345 | + } catch { /* not JSON — ignore */ } |
| 346 | + } |
| 347 | + }); |
| 348 | + |
| 349 | + let id = 1; |
| 350 | + const rpc = (method: string, params?: unknown) => |
| 351 | + new Promise<Record<string, any>>((resolve, reject) => { |
| 352 | + const myId = id++; |
| 353 | + const giveUp = setTimeout( |
| 354 | + () => reject(new Error(`stdio: no answer to "${method}" (id ${myId}) within 5s`)), |
| 355 | + 5_000, |
| 356 | + ); |
| 357 | + waiting.set(myId, (frame) => { |
| 358 | + clearTimeout(giveUp); |
| 359 | + resolve(frame); |
| 360 | + }); |
| 361 | + serverStdin.write( |
| 362 | + `${JSON.stringify({ jsonrpc: '2.0', id: myId, method, ...(params ? { params } : {}) })}\n`, |
| 363 | + ); |
| 364 | + }); |
| 365 | + |
| 366 | + try { |
| 367 | + await rpc('initialize', { |
| 368 | + protocolVersion: '2025-06-18', |
| 369 | + capabilities: {}, |
| 370 | + clientInfo: { name: 'issue-17568-pin', version: '0.0.0' }, |
| 371 | + }); |
| 372 | + serverStdin.write(`${JSON.stringify({ jsonrpc: '2.0', method: 'notifications/initialized' })}\n`); |
| 373 | + |
| 374 | + const frame = await rpc('tools/call', { |
| 375 | + name: 'update_record', |
| 376 | + arguments: { objectName: 'crm_lead', id: RECORD_ID, data: { status: 'qualified' } }, |
| 377 | + }); |
| 378 | + |
| 379 | + const text = String(frame.result?.content?.map((c: { text?: string }) => c.text).join('\n') ?? ''); |
| 380 | + expect(frame.result?.isError).toBe(true); |
| 381 | + expect(frame.error).toBeUndefined(); |
| 382 | + expect(text).toContain(INVALID_PARAMS); |
| 383 | + expect(text).toContain('recordId'); |
| 384 | + expect(text).toContain('`id`'); |
| 385 | + expect(text).toContain('`recordId`'); |
| 386 | + expect(bridge.calls.find((c) => c[0] === 'update')).toBeUndefined(); |
| 387 | + } finally { |
| 388 | + await transport.close().catch(() => {}); |
| 389 | + } |
| 390 | + }); |
| 391 | +}); |
0 commit comments