fix(sequentialthinking): restore nextThoughtNeeded in the advertised inputSchema required array - #4652
Open
AmirF194 wants to merge 1 commit into
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
Author
|
#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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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