feat(zod): support schema definitions#1993
Conversation
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
The main implementation is focused and the intended enum-deduplication case works for both Zod v3 and v4. I’m requesting changes for two definition-name edge cases in the new public map that currently generate invalid schemas.
Validated locally at 16b870f0:
pnpm test --runInBand tests/helpers/zod.test.ts— 28/28 passedpnpm lint— passed, including build, types, package checks, and JSR dry run- additional Zod v3/v4/v4-mini runtime probes
The ordinary PR checks are green. OkTest is red for an unrelated generated fixture mismatch (InputTokensDetails.cache_write_tokens), before its compatibility suite runs.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
The core approach is sound and the normal v3/v4 cases are well covered, but I found two correctness gaps in the newly accepted schemaDefinitions names that can produce invalid or semantically wrong schemas. Please address these before merge.
Validation performed:
- focused Zod helper suite: 28/28 passed
- full repository suite: 1,189 passed (1 skipped)
pnpm lintandpnpm build: passed, including type checks, publint, package loading, and JSR dry-run- GitHub build/lint/tests/ecosystem/examples/breaking-change checks: passed
The separate OkTest failure is unrelated: its Node bridge fixtures are missing the newly required cache_write_tokens field, and the run fails before exercising this change.
Duplicate review submitted during a concurrent review; the earlier review contains the same blocking findings.
There was a problem hiding this comment.
The two previously reported name issues are fixed, and the normal enum-deduplication path is sound. However, the public map still accepts arbitrary string keys that produce dangling $refs in two reproducible cases on both Zod v3 and v4. Please either encode/preserve these keys correctly or reject them explicitly, with regression coverage for both converter paths.
Validation at bbd5c857: focused Zod suite 32/32 passed; all full-suite Jest assertions passed (1,201 passed, 1 skipped), although the command exited nonzero afterward because the existing mock-server cleanup passed multiple newline-separated PIDs to kill; full pnpm lint passed (format, ESLint, CJS/ESM and Deno builds, TypeScript, ATTW, publint, JSR dry-run). Current CI and OkTest 237/237 are green.
There was a problem hiding this comment.
Pull request overview
Adds support for supplying named shared Zod schemas to zodResponseFormat() so they can be emitted as JSON Schema definitions and referenced via $ref, reducing duplicated schema output (notably large enums) and helping stay within API schema limits.
Changes:
- Extend
zodResponseFormat()’s third argument to accept an optionalschemaDefinitionsmap and ensure it is not forwarded as an API response-format field. - Implement definition registration/extraction for both Zod v3 (
zod-to-json-schemadefinitionsoption) and Zod v4 (registry metadata), including JSON Pointer + URI escaping for generated$refs. - Add regression tests covering enum deduplication, name collision behavior, JSON Pointer escaping, URI encoding, and
__proto__rejection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/helpers/zod.ts |
Adds schemaDefinitions support for both Zod v3 and v4 schema conversion paths and sanitizes/escapes generated $refs. |
tests/helpers/zod.test.ts |
Adds tests validating definition extraction/deduplication and correct $ref escaping/validation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-reviewed the exact current head (3305aa52) after the prior change request. Both dangling-reference findings are fixed: URI-fragment tokens are now JSON Pointer-escaped and URI-encoded (foo%2Fbar emits foo%252Fbar and resolves to the literal key), and an own __proto__ definition key is rejected before either converter runs. The thread-aware review state shows both prior threads resolved.
I verified the fixes on Zod v3, v4, and v4-mini, including adjacent /, ~, #, spaces, Unicode, constructor, and prototype names. Local validation passed: focused Zod suite 36/36 and full pnpm lint (Prettier, ESLint, CJS/ESM and Deno builds, TypeScript, ATTW, publint, JSR dry-run). Current CI is green across Node 20/22/24/26.2, ecosystem tests, Agents SDK regressions, breaking-change detection, examples, and CodeQL; OkTest is 237/237. No substantive issues remain.
Changes being requested
Fixes #1285.
zodResponseFormat()now accepts an optionalschemaDefinitionsmap in its existing third argument. Callers can name shared Zod schemas that should be extracted into JSON Schemadefinitions, allowing repeated large schemas such as enums to be referenced instead of copied at every use site.The helper previously forwarded response-format props but had no way to pass definitions into schema conversion. Reusing two 200-value enums could therefore emit more than the API's 500-enum limit even though the logical schema only contained 400 unique values.
The implementation keeps the same public option for both supported Zod paths:
zod-to-json-schemadefinitions option.schemaDefinitionsis removed from the outgoing response-format object after conversion, and callers who do not provide it keep the existing generated schema shape.The regression test mirrors the issue's two reused 200-value enums for both Zod v3 and v4. It verifies that only 400 enum values are emitted and that every generated reference resolves.
Additional context & links
definitionsas azodResponseFormatparam #1285pnpm test --runInBand tests/helpers/zod.test.tspnpm lintpnpm test --runInBand