Fix Zod object detection logic #468
Conversation
Previously, this check was `value instanceof ZodType`, which breaks if the user is using a different version of Zod than the library. Instead, do the check using the structure of the input object rather than its prototype chain. Additionally, this adds a vendored version of Zod to use for tests, which ensures that regressions like this would be caught next time.
|
Well I don't think it's an either/or, personally. Fixing the API is technically a breaking change, which might be fine but probably shouldn't be rushed out. Whereas this is a straight bugfix, which imo should get released ASAP as a patch release to stop anyone else hitting the issue. |
src/server/mcp.ts
Outdated
| function isZodRawShape(obj: unknown): obj is ZodRawShape { | ||
| if (typeof obj !== "object" || obj === null) return false; | ||
|
|
||
| const isEmptyObject = z.object({}).strict().safeParse(obj).success; |
There was a problem hiding this comment.
haven't tested it, would it potentially also cause an issue with a different version of Zod?
- simpler test for empty object - only test public zod properties (parse, safeParse) Confirmed that vendored-library tests still pass. Note that you need the --testPathIgnorePatterns to avoid the __tests__ directory (this is what was breaking the build): $ npm test -- --testPathIgnorePatterns="vendor"
|
@geelen thanks a bunch for this - I've tweaked the heuristic type test, removed the dependency on Also agree that a real fix means a different API - essential considering the more complex/type-unsniffable tool configs that are sure to follow (starting with |
Previously (in #309), this check was
value instanceof ZodType, which breaks if the user is using a different version of Zod than the library. Instead, do the check using the structure of the input object rather than its prototype chain.Additionally, this adds a vendored version of Zod to use for tests, which ensures that regressions like this would be caught next time.
Types of changes
Checklist
Note: In general, having two loosely structured arguments that are both optional and so can occur in the same position of a function call (i.e.
server.tool(name, params, cb)andserver.tool(name, annotations, cb)is really brittle. This PR improves things so the version of Zod the downstream user has installed no longer breaks things, but I would strongly recommend removing the ambiguity from the API.It would mean that in order to declare a tool with no params but with annotations, you'd write
server.tool(name, {}, annotations, cb). Those four extra characters seem a small price to pay for eliminating an entire class of errors like this.