Fix six bug families in MCP tool schema and result handling - #1259
Fix six bug families in MCP tool schema and result handling#1259hsm207 wants to merge 11 commits into
Conversation
|
I tested this PR at commit The clone and persistence fixes work for ordinary named schemas, but several tools with loose object fields or unions still fall back to an empty model-facing schema. The Freebuff log reports:
The remaining failure path is:
A minimal reproduction is: {
"type": "object",
"properties": {
"project_id": { "type": "string" },
"payload": { "type": "object" }
},
"required": ["project_id", "payload"]
}The approach I tested builds on the idea from PR #921, which previously proposed preserving the raw schema in My follow-up wraps raw MCP schemas with AI SDK's The focused regression test verifies that:
Would it be helpful if I opened a small dependent PR against this PR's branch with the implementation and regression test? |
|
Good work — this is exactly the kind of PR that's easy to evaluate because each fix ships with a regression test that fails on old code and passes on new code.
Main concern: this bundles four independent fixes plus a Overall: correct root causes, in-scope, tested at the right layer. Recommend splitting for future submissions, but this is portable as-is. |
|
Thanks for the catch. The example schema you gave exposed a gap I had not considered, and your pointer to PR #921 was key to building a better fix. Raw MCP schemas now go through the AI SDK's Your finding also surfaced a second gap: for Now, if a tool still fails, its schema is complicated, and let's just deal with it when it gets reported. No dependent PRs needed, but thanks for the offer! |
Cold-boot live testing against an echo MCP server showed that when a tool schema declares a param as a union with an object variant (anyOf/oneOf), the model may emit the object as a JSON-encoded string - unambiguously valid for the union, so nothing downstream fails, and the server receives a string where the model meant an object. The repair is schema-guided: only params whose declared union includes an object variant, and whose value parses as JSON, are decoded; plain strings and string-typed params containing JSON (script sources, file contents) are untouched. The parse result now returns the validated parameters rather than the raw input, so repairs reach the handler (this also stops a latent crash when input is absent). Tests use quwin's loose-schema shape from PR CodebuffAI#1259 follow-up discussion. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
a7c54d7 to
bc5d8e9
Compare
Cold-boot live testing against an echo MCP server showed that when a tool schema declares a param as a union with an object variant (anyOf/oneOf), the model may emit the object as a JSON-encoded string - unambiguously valid for the union, so nothing downstream fails, and the server receives a string where the model meant an object. The repair is schema-guided: only params whose declared union includes an object variant, and whose value parses as JSON, are decoded; plain strings and string-typed params containing JSON (script sources, file contents) are untouched. The parse result now returns the validated parameters rather than the raw input, so repairs reach the handler (this also stops a latent crash when input is absent). Tests use quwin's loose-schema shape from PR CodebuffAI#1259 follow-up discussion. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
When a parameter's schema is a union with an object variant, models sometimes emit the object as a JSON-encoded string. The string is valid for the union, so validation passes and the handler silently receives a string instead of the object the model meant - data loss with no error. Decode schema-guided string-encoded members before validation, and hand the handler what the schema saw (processedParameters) rather than the untouched raw input. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
Tool results replay from history into every later prompt build, and the AI SDK base64-decodes media file parts at build time. Serving a text/plain resource as media therefore dies with "The string contains invalid characters" on every subsequent turn - permanently, since the poisoned message is in history. Non-image binaries (gzip, PDF, ...) went one worse: the OpenAI-compatible converter throws on them, killing the session on replay. Extract the mapping into mcpContentToToolResultOutputs: text resources become json values, only image/* resources stay media, and other binaries degrade to a descriptive json line. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
lodash cloneDeep strips zod v4's non-enumerable _zod engine from schema instances. The clone passes the safeParse smell test but is half-dead: any zod internal touching schema._zod.* detonates with "undefined is not an object", and upstream's ensureJsonSchemaCompatible fallback then reads schema.description outside its own try - so one stripped schema kills the entire agent step at getToolSet instead of degrading a single tool. Add cloneDeepKeepingZod (deep-clones plain data, passes schema instances through by reference) and use it at the tool-definition clone sites: getToolSet's additional-tool-definition loop and executeCustomToolCall's customToolDefinitions write target. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
Converting every custom tool inputSchema to zod and back is lossy:
schemas zod cannot express (e.g. a property typed only
{ "type": "object" }) come back as an empty object schema, and a model
reading an empty argument schema emits {} - a tool call with no
arguments.
serveInputSchema splits the two consumers: the model-facing definition
gets the MCP server's declared JSON Schema verbatim (wrapped in ai's
jsonSchema() pass-through container), while argument validation at call
time keeps the zod conversion, where approximation is recoverable.
Zod-typed inputSchemas keep ensureJsonSchemaCompatible, which now also
logs when it has to fall back instead of failing silently.
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
toolDefinitions live in agent state, which hosts persist, snapshot, and
ship over the wire. mapValues stored the live inputSchema as-is, so zod
instances (cyclic, internals on non-enumerable _zod) ended up in
persisted state: JSON.stringify over that state embeds zod machinery
({"def":{"shape":...}}) instead of the schema the tool actually
declares.
Normalize at the storage site with toTokenCountInputSchema (already used
for the token-count path): converts zod to JSON Schema, copies plain
objects through, and guarantees a top-level type: 'object'.
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
spawn-agent-inline builds the same state-stored toolDefinitions map as loopAgentSteps and had the identical raw-inputSchema leak. Extract toTokenCountInputSchema into util/to-json-schema.ts so both call sites share one implementation (the util location avoids the import cycle through run-agent-step, which re-exports for compatibility). 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
Bring the six bug-backing test files onto this branch: MCP content
mapping, schema storage, prompts schema handling, to-json-schema,
zod-safe-clone, and the OpenAI-compatible converter.
Two ported tests exposed gaps this branch still had, fixed here:
- getMCPToolData converted server schemas to zod before storing them in
persisted state; store the raw JSON Schema verbatim instead.
- The OpenAI-compatible converter threw on non-image file parts, killing
the whole session on replay; degrade to a text placeholder.
One test asserted a zod serializer token ("allOf") instead of the
business contract; it failed identically on the pre-V2 fix tip, so it
was never a stable assertion. Now asserts the params survive into the
description.
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
Per conflict hygiene: new code lives in new files so a future upstream merge touches our modules plus a one-line import in theirs, instead of rewriting regions inside upstream functions. - tool-executor.ts: repairStringEncodedUnionMembers moves to util/repair-string-encoded-union-members.ts (call site unchanged). - client.ts: mcpContentToToolResultOutputs moves to common/src/mcp/content-mapping.ts (call site unchanged). - prompts.ts: serveInputSchema + ensureZodSchema move to tools/serve-input-schema.ts; prompts.ts drops the logger parameter added for the loud-fallback experiment and reverts ensureJsonSchemaCompatible to the upstream shape (452 lines, under the 500-line budget; ensureJsonSchemaCompatible remains upstream's silent-fallback version pending upstream buy-in). Behavior unchanged: full suite 78/78. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
The tmp/ audit tests proved the bugs and drove the fixes; this replaces
them with committed tests at the modules they guard, rewritten to the
test-review checklist: cyclomatic complexity 1, AAA with fresh fixtures
built through small DSL helpers, contractual trigger-outcome names,
single logical outcome per test, no narration comments.
- repair-string-encoded-union-members.test.ts: 4 cases (decode, plain
string passthrough, real object passthrough, JSON-text string param).
- serve-input-schema.test.ts: zod survival + verbatim JSON Schema
serving incl. the bare {type:object} amputation repro.
- call-mcp-tool-resources.test.ts: real stdio MCP server, fresh client
per test; text->json, gzip->descriptive text, png->media.
- json-safe-state.test.ts: loopAgentSteps stores plain JSON Schema in
agent state, no zod def/shape internals.
79 tests green across the 10 regression files.
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <noreply@codebuff.com>
Index the model-facing schema through a typed propertyAt helper and spread the runtime-impl fixture as Record<string, unknown> so the new test files typecheck clean alongside the suite. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
Same-entry-point duplicates removed: union-repair tests consolidated into parse-raw-custom-tool-call.test.ts (the only file with the real- object-passthrough case), loose-schema cases covered once by the quwin repro in prompts-schema-handling, and the three-transport e2e reduced to one wiring guard since the mapping itself is unit-tested next door. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
bc5d8e9 to
059792d
Compare
|
I've force-pushed a rebuilt version of this series. Since there were no reviews yet, nothing is lost — the discussion stays intact. What changed and why: the original branch was a rebase that resurrected old files on top of upstream's rewrite, which made the branch diverge structurally from Verification is stronger than before:
|
|
Independent reproduction confirming the impact of bug families 1, 5, and 6 — filed as #1306 with full logs. Setup: freebuff CLI 0.0.93, Windows, models Repro: with mainstream MCP servers configured ( Controls (same session): zero-param MCP tools ( Happy to re-run the same 4-server × 2-model matrix against this branch if it helps the merge decision. Side note for anyone debugging similar reports: knowing this root cause gives a recognizable signature — an MCP tool that "works" only when called with zero arguments is almost certainly hitting the empty-schema fallback, and this fix is what makes such tools fully usable again. |
|
Cross-control confirmation (2026-09-09): ran the same 4-server x 1-model matrix with |
This PR fixes six bug families in the MCP server integration, including the one reported in #912 (tool
inputSchema.propertiesstripped at registration). I found them by exercising all 13 tools of@modelcontextprotocol/server-everything, the official example MCP server, end-to-end against a source build, and verified every fix two ways: a two-sided regression suite (each test fails on pre-fixmainfor the exact intended reason) and a live end-to-end session.The six bug families
{}. lodashcloneDeepdrops zod v4's non-enumerable_zodengine, and a silent fallback then serves an empty schema. This is MCP Tool inputSchema.properties stripped when registering tools from external MCP servers #912'sproperties: {}and itsexpected string, received undefinederrors. Fixed withcloneDeepKeepingZod.JSON.stringifythrow, so sessions die from the second turn onward. Schemas are now normalized to plain JSON Schema before storage (including the subagent path).The string contains invalid characters. Text resources now stay text.{ "type": "object" }properties (SEP-2106); converting to zod and back stripped them, so models called tools with no arguments. JSON Schemas are now served to the model verbatim via ai'sjsonSchema(), with zod-backed validation at call time.Branch shape
The series is rebuilt from a clean
mainbase: each fix is one small semantic commit, and its new code lives in its own module (util/zod-safe-clone.ts,util/to-json-schema.ts,util/repair-string-encoded-union-members.ts,tools/serve-input-schema.ts,common/src/mcp/content-mapping.ts), leaving one-line import/call-site changes in upstream files. Future rebases againstmainstay small. Two additional seams the regression suite caught during the rebuild — verbatim schema storage ingetMCPToolData, and the non-image file-part degrade in the OpenAI converter — are fixed in the same series.Testing
Each fix has regression tests at its module. The suite was validated two-sided against pre-fix and post-fix trees, includes real-stdio-server tests for the resource mapping, and the whole set was confirmed in a live session exercising all 13 tools of
server-everythingwith no anomalies across multi-turn replay.Note: this series intentionally does not add a warning log when a schema falls back to empty — that would modify an upstream function body. Separately,
ensureJsonSchemaCompatible's catch block readsschema.descriptionon a clone-stripped schema and throwsTypeError(schema._zod.parent), killing the agent step instead of degrading; I'd like to propose that small fix upstream separately, with repro evidence available.