fix(sequentialthinking): restore nextThoughtNeeded in the advertised inputSchema required array - #4652
Conversation
…inputSchema required array commit 1cdf806 (modelcontextprotocol#3533) wrapped nextThoughtNeeded in a z.preprocess-based coercedBoolean to fix a real footgun (string "false" coercing to true). zod's toJSONSchema(..., { io: "input" }) treats a z.preprocess()-wrapped field's input type as unknown, so it silently drops that field from the emitted required array, even though it carries no .optional(). A client that builds its call arguments from the advertised schema then omits nextThoughtNeeded and gets a -32602 Invalid params from the runtime validator, which still requires it. Rebuild coercedBoolean as a transform on an explicit z.union([z.boolean(), z.string()]) instead of z.preprocess. The union gives toJSONSchema a concrete input type to report, so nextThoughtNeeded stays in required, while parse behavior (including the case-insensitive string coercion modelcontextprotocol#3533 added) is unchanged. Fixes modelcontextprotocol#4651
|
#4655 (AbhiPra24, opened 71 minutes after this one) fixes the same nextThoughtNeeded/required drift with the same mechanism (drop z.preprocess for a union that keeps a real Zod type, so toJSONSchema stops dropping the field), plus a broader scope (also fixes #4575's version drift) and a full integration test suite this PR doesn't have. One behavioral difference worth flagging either way: this PR rejects a malformed nextThoughtNeeded string (anything besides "true"/"false") with a validation error, #4655's fallback silently coerces it to false. Happy to stand down in favor of #4655 if that's the one you'd rather take forward. Update: #4669 (re2zero, opened two days after this one) landed the identical fix, same file, same mechanism, no test suite and no version-drift fix, so it does not change the comparison above. |
Root cause
Commit
1cdf806d(#3533) wrappednextThoughtNeededin az.preprocess()-basedcoercedBooleanto fix a real footgun (the string"false"coercing totrue).zod'stoJSONSchema(..., { io: "input" })treats az.preprocess()-wrapped field's input type asunknown, so it silently drops that field from the emittedrequiredarray, even though the field carries no.optional(). The advertised schema then saysnextThoughtNeededis optional while the runtime validator still requires it, so any client that builds its call arguments from the declared schema gets-32602 Invalid params.Invariant
The advertised
inputSchema.requiredarray and the runtime validator agree on which fields are mandatory forsequentialthinking.Fix
Rebuild
coercedBooleanas a.transform()on an explicitz.union([z.boolean(), z.string()])instead ofz.preprocess(). The union givestoJSONSchemaa concrete input type to report, sonextThoughtNeededstays inrequired, while parse behavior (including the case-insensitive string coercion #3533 added) is unchanged.Verification
node:22-slim), clean checkout of76d64c8(currentmain): a live stdio MCP session (initialize→tools/list→tools/call) built its call arguments strictly from the advertisedrequiredarray. Fails on main withMCP error -32602: ... expected boolean, received undefined at nextThoughtNeeded; passes on this branch, andtools/listnow reportsrequired: ["thought","nextThoughtNeeded","thoughtNumber","totalThoughts"].nextThoughtNeeded: "FALSE"(the case fix(sequential-thinking): use z.coerce for number and boolean params #3533 fixed): still coerces tofalseon this branch, so the original footgun stays fixed.npm run build(tsc) andnpm test(vitest, 14/14) both green on this branch.lib.ts(SequentialThinkingServer), notindex.ts(0% coverage before and after this change, pernpm test's own coverage report), becauseindex.tscallsrunServer()unconditionally at module load, so it isn't imported by the existing tests. I verified the fix with a live protocol session instead of adding a unit test, rather than invent a new import-safe seam (exporting the schema, guarding therunServer()call) that has no precedent anywhere else in this repo's ~30 other reference servers. Happy to add that seam if you'd like a permanent regression test here.Fixes #4651