fix(zod): parse extracted definitions in their property position - #2463
fix(zod): parse extracted definitions in their property position#2463cmun2 wants to merge 16 commits into
Conversation
A Zod v3 schema ending in .nullable().optional() that is reached from more
than one field was encoded two different ways in one document: inline it came
out clean, while the definitions entry the second reference points at kept an
anyOf: [{ not: {} }, ...] wrapper.
zodToJsonSchema materialises a definition because some property $refs it, but
parsed it with currentPath set to the definition and propertyPath left
undefined. parseOptionalDef branches on exactly that field, so the same Zod
node took the standalone branch and gained the wrapper.
not is outside the subset strict Structured Outputs accepts, and the v3 path
is the one helper path that does not run toStrictJsonSchema(), so it reached
the request body unchecked.
parseOptionalDef is the only reader of propertyPath in the vendored converter,
so no other parser is affected.
Fixes openai#2462
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecf273d4a3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Review on openai#2463 caught this: with the definitions loop now passing propertyPath, parseOptionalDef takes its property branch, which returns parseDef's result directly instead of falling back to {}. When the inner type produces no schema the element became undefined, and parseTupleDef filtered it out of items while minItems and maxItems still described the original arity. items is positional, so dropping an entry shifts every later element onto the wrong index. For zod.tuple([emptySlot, zod.string()]) the generated schema demanded a string at index 0, rejecting [123, 'valid'] that Zod accepts and accepting ['valid', 123] that Zod rejects. Substitute an unconstrained {} instead of filtering. That keeps positions aligned and constrains nothing, which is what the element already said. This also fixes the same drop for a tuple under an object property, where parseObjectDef has always set propertyPath - that case was broken before openai#2463.
Review on openai#2463 caught this: with the definitions loop now passing propertyPath, parseOptionalDef takes its property branch, which returns parseDef's result directly instead of falling back to {}. When the inner type produces no schema the element became undefined, and parseTupleDef filtered it out of items while minItems and maxItems still described the original arity. items is positional, so dropping an entry shifts every later element onto the wrong index. For zod.tuple([emptySlot, zod.string()]) the generated schema demanded a string at index 0, rejecting [123, 'valid'] that Zod accepts and accepting ['valid', 123] that Zod rejects. Substitute an unconstrained {} instead of filtering. That keeps positions aligned and constrains nothing, which is what the element already said. This also fixes the same drop for a tuple under an object property, where parseObjectDef has always set propertyPath - that case was broken before openai#2463.
d5166dc to
1a26b13
Compare
Review caught that the previous commit marked every materialized definition as
a property. Only some are: a definition referenced from an array item, or one
the caller supplied outright, was parsed with a propertyPath it never had, so
parseOptionalDef dropped its standalone anyOf: [{ not: {} }, ...] encoding, and
in a tuple it returned undefined instead of {} and parseTupleDef filtered that
positional entry out while minItems and maxItems still described the arity.
Seen now records the propertyPath in effect where a def was first reached, and
the definitions loop reuses it: property context only for definitions that came
from a property.
Strict mode keeps the property encoding for every definition including supplied
ones, because not is not in the subset strict Structured Outputs accepts, so the
standalone form is not representable there. ZodTuple, the shape that makes
positions matter, is rejected before conversion in that mode.
## Summary GitHub omits `workflow_run.pull_requests` for external-fork runs, and querying the upstream repository's commit-association endpoint returns no pull requests for those fork commits. That leaves the required `Castiron / budget-only change` and `Castiron / custom-code budget` contexts permanently expected even when the candidate workflow succeeds. - Centralize trusted Python pull-request association in `custom_code_report.py` and reuse it from trusted report generation, comment publication, and budget evaluation. - Resolve missing associations from the authenticated source run's `head_repository`, including legitimately renamed forks, then independently re-fetch every candidate PR from `openai/openai-node`. - Apply equivalent validation in both privileged JavaScript publishers: required commit statuses and failure-report comments. - Refresh the candidate workflow's pinned reporter SHA-256 after the trusted reporter change. ## Security model Fork-side associations and PR numbers are discovery hints, never authorization. The trusted paths: 1. Re-fetch the workflow run from the upstream Actions API and verify its repository, immutable candidate SHA, workflow path, completion, and run attempt as applicable. 2. Strictly validate the source repository's owner/name syntax and consistency with authenticated `head_repository` metadata; reject traversal-like components and spoofed identities. 3. Re-fetch each hinted PR from the upstream repository and require an open PR with the exact candidate SHA, the exact source head repository, the intended upstream repository and `main` base ref, and exactly one valid current association. 4. Require the current `main` base SHA wherever budget evaluation or status publication needs freshness; preserve existing stale-run behavior and merge-group validation. The existing trusted `workflow_run`/main-checkout boundary, bare Git object store, candidate-artifact isolation, least-privilege job permissions, merge-queue protections, and exact required status names remain unchanged. No candidate workflow definition, mutable ref, contributor artifact, or fork-supplied PR number is trusted. ## Affected contributor PRs - #2463: live fork workflow run `32877584723` has `pull_requests: []`; the updated resolver correctly finds its upstream PR through `cmun2/openai-node`. - #2444 and #2431: independently reproduced the same fork-only association behavior; both PRs merged while this fix was being prepared. ## Verification - `env PYTHONDONTWRITEBYTECODE=1 python3 -m unittest discover -s scripts/castiron -p 'test_custom_code*.py'` — 60 tests pass, with one pre-existing skip. - `go run github.com/rhysd/actionlint/cmd/actionlint@latest .github/workflows/castiron-custom-code.yml .github/workflows/castiron-custom-code-comment.yml` — both workflows pass actionlint v1.7.12. - `ruff format --check scripts/castiron/custom_code_report.py scripts/castiron/custom_code_budget.py scripts/castiron/test_custom_code_report.py scripts/castiron/test_custom_code_budget.py`. - Ruff lint passes when ignoring only the same pre-existing baseline rule findings; `git diff --check` passes. - Executable publisher and Python regression coverage includes external forks with empty run associations, fork-side lookup, same-repository and renamed-fork PRs, malformed/spoofed repositories, unrelated source heads, ambiguous/duplicate/invalid associations, stale heads/bases/run attempts, exact required contexts, and merge groups.
Review caught that the strict-mode branch recreated the drop for callers of the
exported converter. The comment justified it by saying ZodTuple is rejected
before conversion, but that rejection lives in assertSupportedZodV3Schema,
which the helpers call and zodToJsonSchema does not. zodToJsonSchema is
exported, so openaiStrictMode with a tuple definition reaches the converter
directly.
Fix the drop at its source rather than reasoning about which callers can
produce one. items is positional and minItems/maxItems come from the declared
arity, so an element that parses to undefined becomes an unconstrained {}
instead of being filtered out. That holds for every position: supplied
definition, strict or not, under an object property, and with a rest element.
Without it the generated document accepted and rejected the opposite arrays
from the Zod schema it came from -- rejecting [123, 'valid'] that Zod accepts
as [0, 'valid'], accepting ['valid', 123] that Zod rejects.
HAYDEN-OAI
left a comment
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Two remaining schema-conversion regressions are called out inline.
Two more containers were losing entries, and a definition was losing its
description. Both come from the same place: strict mode was handed a property
context for every definition, which changes how everything nested inside it
parses, and parseOptionalDef returns undefined rather than {} in that branch.
A union dropped its unconstrained anyOf entry, an array's items became
undefined and vanished in serialization, and the exported converter reaches
both without the helpers' unsupported-schema check.
Drop the strict-mode context entirely. The reason it was there was that not is
outside the subset strict Structured Outputs accepts, and that is now handled by
rewriting the finished definition -- anyOf: [{ not: {} }, X] reduces to X, an
identity -- instead of changing how its contents parse.
parseOptionalDef still cannot return undefined for a definition being
materialized: something already holds a to it, and parseDef only attaches
.describe() text to a schema it actually got, so the annotation disappeared
before the outer ?? {} could run. It now falls back to {} under forceResolution,
which is set only on that path, so a plain optional property is still omitted
the way parseObjectDef expects.
Reverting parsers/optional.ts, parsers/tuple.ts or zodToJsonSchema.ts fails 1, 1
and 15 of the new tests respectively.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9b04296a7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… optional Three more places where a definition came out wrong, all from the same two mistakes. The never-branch reduction ran on the definition's root object only. A description sits beside the generated anyOf, so the lone-key guard skipped it and left not in strict output; and a container supplied through schemaDefinitions holds its optional elements nested, where the reduction never looked. It now walks the whole materialized definition and carries annotation siblings onto the result. The property context was a path prefix, which matches the entire subtree, so a union or array inside a property-derived definition was parsed as though it sat directly in a property and lost the entries it holds by branch or by position. The treatment such a definition actually needs is one thing -- drop the outer optional wrapper -- so that is now done directly, and nothing below it is touched. The wrapper's own .describe() is reapplied, as it would be inline. parsers/optional.ts is back to its original form: unwrapping at the definition makes the fallback it carried unnecessary. Containers checked as supplied definitions, strict and not, at one and two levels of nesting: tuple, union, array, record, intersection, object, an object holding a tuple, and an array of tuples. Every one is byte-identical to clean origin/main apart from the intended differences, and no not survives anywhere in strict output.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1104eb244
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…e path
Three regressions from the last commit.
The reduction recursed into every object-valued child, so a default, const,
enum or examples payload shaped like a schema was rewritten -- a declared
default of { anyOf: [{ not: {} }, { value: 'kept' }] } came out as
{ value: 'kept' }. It now descends only through keywords whose value is a
schema, a list of schemas or a map of schemas, the same line toStrictJsonSchema
draws.
Unwrapping the outer optional took the definition off the normal parseDef path,
so the public override hook never saw the ZodOptional and markdownDescription
was dropped -- the manual restoration copied description only. The def goes
through parseDef unchanged again; the outer wrapper is now dropped afterwards by
the same never-branch identity the strict reduction uses, applied at the root
only for a property-derived definition. override and addMeta behave exactly as
before.
Containers checked as supplied definitions, strict and not, at one and two
levels: tuple, union, array, record, intersection, object, an object holding a
tuple, an array of tuples, and an optional carrying a default. Every one is
byte-identical to clean origin/main apart from the intended differences.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83f0ddfe33
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Two more, both in the collapse itself.
Spreading the siblings over the branch let an outer validation keyword win. An
override returning { anyOf: [{ not: {} }, { type: 'string', maxLength: 3 }],
maxLength: 5 } means both limits apply; the merge produced maxLength: 5 alone
and the document started accepting four-character strings. Only annotations move
across now -- they constrain nothing, and addMeta is what puts them there -- and
a union sitting beside a validation keyword is left standing rather than merged.
Rebuilding a schema map assigned each entry into a plain object, so a
__proto__ key reached the inherited setter: the entry vanished and the supplied
schema became the object's prototype. The map is built with a null prototype and
spread back at the end.
Both fail against the previous head.
tsc --noEmit flagged the three new override callbacks: the local JsonSchema helper in this file is not the converter's JsonSchema7Type, and casting the whole callback to never hid that rather than fixing it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb862fffc6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
A shared .default() makes parseDefaultDef emit default beside the union, and default was missing from the annotation set, so the collision guard left the whole schema standing and the not survived. JSON Schema files both default and examples under annotations; neither narrows what a document accepts, so both move across the collapse like description does. They are still not descended into -- their values are literal JSON, and a default shaped like a schema is left exactly as declared. The strict helpers reject ZodDefault before conversion, so zodResponseFormat never reaches this; the exported converter with openaiStrictMode does, which is where it was measured.
Castiron custom code✅ No new custom-code files detected. 32 mixed files remain; 0 existing customizations changed. Compared 32 existing customizations unchanged
A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 32890823669 --repo openai/openai-node \
--name castiron-custom-code-32890823669-1 --dir /tmp/castiron-custom-code-32890823669-1
git apply --stat /tmp/castiron-custom-code-32890823669-1/custom-code.patch
cat /tmp/castiron-custom-code-32890823669-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin cc532b3f173e64c0dbc9975516bbf4f6195c9b21 d7c5503dfe78a94db50b9f07da2f2d7bd5695a1f
python3 scripts/castiron/custom_code_report.py report \
--base cc532b3f173e64c0dbc9975516bbf4f6195c9b21 \
--head d7c5503dfe78a94db50b9f07da2f2d7bd5695a1f --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-d7c5503dfe78
cat /tmp/castiron-custom-code-d7c5503dfe78/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
HAYDEN-OAI
left a comment
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Two remaining schema-conversion regressions are called out inline.
| refs.openaiStrictMode ? | ||
| // `not` is outside the subset strict Structured Outputs accepts, at any | ||
| // depth (see `toStrictJsonSchema` in `lib/transform`). | ||
| (collapseNeverBranchesDeep(materialized) as JsonSchema7Type) |
There was a problem hiding this comment.
[P2] Rewrite references when collapsing optional definitions
With the exported converter's default $refStrategy: 'root', this rewrite removes the anyOf/1 path after parseDef has already generated references into it:
const shared = z.string().min(2);
const maybe = z.object({ first: shared, second: shared }).optional();
const schema = zodToJsonSchema(z.object({ value: z.number() }), {
openaiStrictMode: true,
definitions: { Maybe: maybe },
});schema.definitions.Maybe.properties.second.$ref remains #/definitions/Maybe/anyOf/1/properties/first, but Maybe now directly owns properties, so that pointer no longer resolves. On the base revision the same reference resolves correctly. $refStrategy: 'relative' likewise retains stale traversal depths after the schema moves.
Please rewrite affected references when collapsing wrappers, or preserve stable paths for referenced schema nodes.
There was a problem hiding this comment.
Fixed in c4e6637ea22c5b6908da53717f1048de005bdefb. Where collapsing would leave a pointer dangling, the wrapper stays. A redundant anyOf still means what it says; a broken $ref does not, and rewriting pointers would have to handle relative depths as well.
Your reproduction now keeps Maybe as { anyOf: [{ not: {} }, …] }, and #/definitions/Maybe/anyOf/1/properties/first resolves — the regression follows the pointer through the document rather than just comparing its text.
| if (!isPlainObject(value)) { | ||
| return value; | ||
| } | ||
| const walked: Record<string, unknown> = { ...value }; |
There was a problem hiding this comment.
[P2] Reject schema accessors without invoking them
This object spread and the following Object.entries(value) each invoke enumerable accessors on materialized schemas returned by the exported converter's public override callback:
let calls = 0;
const definition = z.string();
const custom = { type: 'object' };
Object.defineProperty(custom, 'properties', {
enumerable: true,
get() {
calls += 1;
return { value: { type: 'string' } };
},
});
zodToJsonSchema(z.object({ value: z.string() }), {
openaiStrictMode: true,
definitions: { D: definition },
override: (def, _refs, _seen, forceResolution) =>
forceResolution && def === definition._def ? custom : ignoreOverride,
});calls is 0 on the base revision and 2 on this head. A throwing accessor now aborts conversion, while a side-effecting accessor executes before its value is validated. The existing strict-root boundary explicitly rejects accessors without invoking them.
Please inspect own property descriptors and reject accessors, or snapshot data-descriptor values once before traversing.
There was a problem hiding this comment.
Fixed in c4e6637ea22c5b6908da53717f1048de005bdefb. The walk reads own data descriptors and leaves an object carrying any accessor exactly as it is, so nothing installed by an override is invoked. Your counter stays at 0, matching the base revision.
Both are on top of a merge with origin/main: Castiron / baseline consistency pins a hash of scripts/castiron/custom_code_report.py, which moved twice since this branch was cut.
I also cleared the CI lint failure. oxlint rejects Node 23 when loading the TypeScript config, which is why I could not run it locally before; on Node 22 it reproduces, and oxlint, oxfmt --check and tsc --noEmit are now clean for every changed file.
Verified: 41 focused tests, 844 across tests/helpers, full suite 5,115 — 0 regressions.
Two from review and the CI lint failure, in one pass on top of main. Collapsing the wrapper removes an anyOf/1 segment from every JSON pointer below it, and references were generated against the uncollapsed shape, so a $ref aiming inside was left dangling and relative references shifted a level. Where that would happen the wrapper now stays: a redundant anyOf still means what it says, a broken $ref does not. The deep walk spread and enumerated materialized schemas, which invoked enumerable accessors an override may have installed -- caller code running before anything validated it, twice per schema, and a throwing getter taking the conversion down. It reads own data descriptors now and leaves an object carrying any accessor exactly as it is. Lint could not run locally before -- oxlint rejects Node 23 when loading the TypeScript config -- so this also clears what CI found: no-use-before-define, no-thenable on the keyword table, and the non-null assertions, forEach, sort, reduce and spread patterns in the tests. oxlint, oxfmt --check and tsc --noEmit are all clean for the changed files. Merged origin/main as well: Castiron pins a hash of custom_code_report.py, which moved twice since this branch was cut.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4e6637ea2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| continue; | ||
| } | ||
| const child = descriptor.value; | ||
| if (key === '$ref' && typeof child === 'string' && child.includes(wrapperPath)) { |
There was a problem hiding this comment.
When openaiStrictMode uses the default root ref strategy and a supplied definition contains a nested standalone optional—such as z.array(z.object({ a: shared, b: shared }).optional())—the generated second-property ref targets #/definitions/Nested/items/anyOf/1/properties/a. The deep collapse removes that nested anyOf/1, but this guard checks only the definition-root path #/definitions/Nested/anyOf/1, so it accepts the rewrite and emits a dangling $ref. Track each collapsed wrapper path, including nested and relative paths, before committing the rewritten definition.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3b3d2446d23f410ca70eb70e543e21c92ee8aca1, by removing the thing that produced it rather than guarding it.
Reducing not at every depth in strict mode was scope I added; issue #2462 is about a schema shared between two fields being encoded twice, which is the property-derived case and needs only the outer wrapper dropped. The deep reduction had to walk arbitrary caller-supplied objects, and six of the findings on this PR came from that one addition — literal payloads, accessors, colliding constraints, nested pointers.
The collapse now applies to a property-derived definition, at its root, and only where no reference aims inside it. Nothing below a definition is touched and no other definition is rewritten, so nested wrappers are never removed and the pointer you described cannot arise.
What that gives up: a standalone optional supplied through schemaDefinitions keeps its anyOf: [{ not: {} }, …] under openaiStrictMode. Clean main does the same, so it is unfixed rather than broken, and the tests asserting otherwise are removed with it.
| continue; | ||
| } | ||
| const child = descriptor.value; | ||
| if (key === '$ref' && typeof child === 'string' && child.includes(wrapperPath)) { |
There was a problem hiding this comment.
When a default or examples payload contains a literal $ref string matching the wrapper path, this unrestricted recursive scan mistakes it for an active schema reference and restores the uncollapsed definition. For example, an optional object definition whose valid default is { $ref: '#/definitions/Maybe/anyOf/1' } retains the unsupported { not: {} } branch under openaiStrictMode. Fresh evidence after the earlier default fix is that the new pointer guard descends into literal annotations even though the collapse walker correctly does not; restrict this scan to JSON Schema child positions.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3b3d2446d23f410ca70eb70e543e21c92ee8aca1. The pointer guard no longer scans literal payloads: it descends only through JSON Schema keywords, so a reference-shaped string inside a default is data and does not block anything. It is also a much smaller surface now — the guard covers a single root-level collapse rather than a rewrite at every depth.
Verified across all three: 36 focused tests, full suite 5,110 — 0 regressions, and oxlint, oxfmt --check and tsc --noEmit clean for every changed file. The lint failures CI reported are fixed too; oxlint rejects Node 23 when loading the TypeScript config, which is why I could not run it locally before.
| isPlainObject(first['not']) && | ||
| Object.keys(first['not'] as object).length === 0; |
There was a problem hiding this comment.
When an override returns an anyOf whose first branch has an enumerable accessor—for example, a throwing getter for not—collapseNeverBranchesDeep initially leaves that branch unchanged because dataProperties rejects it, but this direct lookup then invokes the getter anyway and aborts conversion. The same problem occurs for accessors inside schema maps traversed with Object.entries; inspect descriptors throughout nested override output and leave any container with accessors untouched rather than executing caller code.
AGENTS.md reference: AGENTS.md:L98-L101
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3b3d2446d23f410ca70eb70e543e21c92ee8aca1. The deep walk is gone, so nothing traverses override output. The one remaining rewrite reads own data descriptors through a single helper and returns the schema untouched if any key on it — or on the branch it would collapse into — is an accessor. Your nested-getter case leaves the counter at 0.
Issue openai#2462 is one thing: a schema shared between two fields is encoded twice, differently, and one of the two carries not. That is the property-derived case, and it needs the outer optional wrapper dropped from the extracted definition -- nothing more. Everything beyond that came from me widening the scope, and it was what kept breaking: reducing not at every depth in strict mode has to walk arbitrary caller-supplied objects, which meant not rewriting literal payloads, not invoking accessors, not merging constraints, and not stranding pointers at any nesting level. Six of the review findings came from that one addition. So it is gone. A definition that came from a property has its wrapper collapsed at the root, and only where no reference aims inside it. Nothing below a definition is touched, and no other definition is rewritten at all. What that gives up: a standalone optional supplied through schemaDefinitions keeps its anyOf: [{ not: {} }, ...] under openaiStrictMode. That is what clean main does too, so it is unfixed rather than broken, and the five tests asserting otherwise are removed with it. The narrower shape still covers every reported case: standalone encoding kept for non-property definitions, tuple positions kept everywhere including strict, union branches intact, description and markdownDescription preserved, override still called for the wrapper, references still resolving at any depth, and no accessor invoked. oxlint, oxfmt --check and tsc --noEmit are clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b3d2446d2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // touched, and the wrapper stays wherever removing it would strand a | ||
| // pointer generated against the uncollapsed shape. | ||
| let finished: JsonSchema7Type = materialized; | ||
| if (originatedInsideProperty(refs.seen.get(def)) && isPlainObject(materialized)) { |
There was a problem hiding this comment.
When an optional nullable schema is both supplied through schemaDefinitions and used as an object property, its pre-seeded Seen entry has no propertyPath, and returning the initial $ref never updates that entry. This condition therefore treats the definition as standalone and preserves anyOf: [{ not: {} }, ...]; Zod v3 strict helpers then send an unsupported schema to Structured Outputs. Fresh evidence beyond the earlier supplied-but-unused case is that this definition is actually the property's $ref target, so record its first real reference context before materialization.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ed2f4aba1a3c6a1fc7a88fc3438851bddb13a834. parseDef now records the context of the first real reference on a pre-seeded entry, alongside the path the definition itself lives at, and originatedInsideProperty compares against that. A schema supplied through schemaDefinitions and pointed at by a property materializes as { type: 'string', nullable: true }.
You are right that this is in scope — it is the filed inconsistency, just reached through a supplied definition rather than an extracted one.
| } | ||
| carried[key] = dataValue(schema, key); | ||
| } | ||
| return { ...second, ...carried }; |
There was a problem hiding this comment.
When the override hook returns a property-derived optional definition whose second anyOf branch has an enumerable getter, this spread executes the getter and a throwing getter aborts conversion. Fresh evidence after the earlier accessor guard is that the new check covers the schema and first branch but never inspects second; verify that the surviving branch contains only data properties before spreading it.
AGENTS.md reference: AGENTS.md:L98-L101
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ed2f4aba1a3c6a1fc7a88fc3438851bddb13a834. The guard checked the schema and the discarded branch but not the one being spread. It checks all three now, and your getter is never read.
| // pointer generated against the uncollapsed shape. | ||
| let finished: JsonSchema7Type = materialized; | ||
| if (originatedInsideProperty(refs.seen.get(def)) && isPlainObject(materialized)) { | ||
| const collapsed = collapseNeverBranch(materialized) as JsonSchema7Type; |
There was a problem hiding this comment.
When a shared property is declared in the also-supported order z.string().optional().nullable(), its inline occurrence strips the inner optional wrapper because propertyPath propagates through parseNullableDef, but forced definition materialization produces anyOf: [{ anyOf: [{ not: {} }, ...] }, { type: 'null' }]. This call only examines the definition root, so the nested unsupported not survives and strict Zod v3 helpers send a schema Structured Outputs rejects. Fresh evidence after narrowing the earlier deep reduction is that optionality need not be the outermost Zod wrapper; normalize it through transparent nullable wrappers while still avoiding arbitrary descendant containers.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ed2f4aba1a3c6a1fc7a88fc3438851bddb13a834. The collapse steps through one wrapper: anyOf: [X, { type: 'null' }], the spelling parseNullableDef emits, which is transparent by construction. Nothing else — no positional or alternative container — is descended into, so this does not reopen the surface the narrowing closed.
z.string().optional().nullable() supplied as a definition now materializes as { anyOf: [{ type: 'string' }, { type: 'null' }] }, matching how the property spells it inline.
Verified across all three: 39 focused tests, full suite 5,113 — 0 regressions, with oxlint, oxfmt --check and tsc --noEmit clean for every changed file.
Both are the same inconsistency wearing different clothes, so they belong in
scope rather than outside it.
A definition supplied through definitions is pre-seeded before anything is
parsed, so its Seen entry has no context, and returning the initial $ref never
filled one in. When that definition is what a property points at, it was treated
as standalone and kept its not. parseDef now records the context of the first
real reference, and the entry carries that alongside the path the definition
itself lives at.
Optionality is not always the outermost wrapper: z.string().optional().nullable()
puts the union inside. Inline, propertyPath reaches through parseNullableDef and
the inner wrapper is dropped, so the extracted definition has to match. The
collapse steps through one nullable spelling -- anyOf: [X, { type: 'null' }] --
which is transparent by construction, and through nothing else.
Also closes an accessor gap: the guard checked the schema and the discarded
branch but not the one being spread.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed2f4aba1a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const wrapperPath = [...definitionPath, 'anyOf', '1'].join('/'); | ||
| if (collapsed !== materialized && !referencesWrapperPath(materialized, wrapperPath)) { |
There was a problem hiding this comment.
When a property-derived definition such as z.object({ first: leaf, second: leaf }).optional().nullable() is pre-seeded through definitions with the default root ref strategy, forced resolution can create a $ref to #/definitions/Maybe/anyOf/0/anyOf/1/.... The collapse removes that inner optional wrapper, but this guard checks only #/definitions/Maybe/anyOf/1, misses the reference, and emits a dangling pointer. Fresh evidence after the earlier nested-collapse fix is the newly supported nullable descent in collapseNeverBranch while this code still constructs only the outer wrapper path; track and check the actual inner path removed by the rewrite.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6a31c399fe2b644abed1315c3102ef4a21f6b386. collapseNeverBranch now reports each wrapper path it actually removed, and every one is checked before the rewrite is kept — so stepping through a nullable is covered rather than only the outer anyOf/1.
Your case keeps the wrapper: #/definitions/Maybe/anyOf/0/anyOf/1/properties/first still resolves. The regression collects every $ref in the definition and follows each one through the document rather than matching on text.
| */ | ||
| const dataValue = (value: Record<string, unknown>, key: string): unknown => { | ||
| const descriptor = Object.getOwnPropertyDescriptor(value, key); | ||
| return descriptor && 'value' in descriptor ? descriptor.value : undefined; |
There was a problem hiding this comment.
When the public override hook returns a property-derived schema with a non-enumerable own anyOf property, JSON serialization would omit that union, but dataValue treats it as present and the collapse can replace the object with its surviving branch—for example, changing an emitted {} into { type: 'string' }. The helper promises an own enumerable data property, so also require descriptor.enumerable before using its value to ensure normalization operates on the properties actually emitted.
AGENTS.md reference: AGENTS.md:L98-L101
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6a31c399fe2b644abed1315c3102ef4a21f6b386. dataValue requires descriptor.enumerable as well now, so a non-enumerable anyOf is treated as absent — which is what JSON.stringify does with it. An override returning { type: 'string' } with a hidden anyOf materializes as { type: 'string' }, matching its own serialization.
Verified: 44 focused tests, full suite 5,115 — 0 regressions, and oxlint, oxfmt --check, tsc --noEmit clean for every changed file.
…le keys
Stepping through a nullable puts the optional wrapper a level down, so the
pointer to guard is anyOf/0/anyOf/1 rather than anyOf/1. The collapse now
reports each path it actually removed and every one of them is checked before
the rewrite is kept.
dataValue promised an own enumerable data property but only checked for a data
descriptor. A non-enumerable anyOf is absent from what JSON.stringify emits, so
treating it as present let the collapse replace an emitted {} with a branch that
was never in the document.
💡 Codex Reviewopenai-node/src/_vendor/zod-to-json-schema/zodToJsonSchema.ts Lines 383 to 384 in 6a31c39 Delay collapse until references from later definitions are known
When a property-derived optional definition is processed before another definition and both contain the same nested schema, materializing the later definition can create a AGENTS.md reference: AGENTS.md:L61-L63 Preserve the first non-property reference context
For a pre-seeded definition first referenced outside any property, AGENTS.md reference: AGENTS.md:L61-L63 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
A definition materialized later can add a $ref into a branch an earlier
collapse removed, so the decision cannot be made while the loop is still
running. Candidates are collected during materialization and committed
afterwards, checked against the finished root and the finished definition map.
The scan is handed { definitions } rather than the map itself, so the walk
recognises it as a map of schemas -- its own keys are names, not keywords, and
it was never descending into them.
Also fixes the sentinel for recording a pre-seeded definition's first reference.
propertyPath is legitimately undefined for a reference that was not inside a
property, so using it to mean 'not yet recorded' let a later property reference
overwrite the real first one, and the definition was then collapsed as though it
had come from a property. referencePath is the sentinel now.
|
Both findings from the latest review, addressed in Delay collapse until references from later definitions are knownCollapse candidates are collected during materialization and committed after the loop, checked against the finished root and the finished definition map. Your case keeps the wrapper: with One thing the fix needed beyond the timing: the scan is handed Preserve the first non-property reference context
Your example keeps its standalone encoding: Verified across both: 46 focused tests, full suite 5,117 — 0 regressions, |
2 participants
Summary
Fixes #2462.
A Zod v3 schema ending in
.nullable().optional()that is reached from morethan one field is encoded two different ways in one document: inline it comes
out clean, and the
definitionsentry the second reference points at keeps ananyOf: [{ "not": {} }, …]wrapper.notis outside the subset strictStructured Outputs accepts —
src/lib/transform.tslists it as unsupported —but only the v4 and Standard Schema helpers run
toStrictJsonSchema(), so onthe v3 path it reaches the request body unchecked.
Root Cause
zodToJsonSchemamaterializes a definition because some property$refs it,then parses it with
currentPathset to the definition andpropertyPathleftundefined:parseOptionalDefbranches onpropertyPath: inside a property it parses theinner type directly, otherwise it wraps in
anyOf: [{ not: {} }, inner].parseObjectDefsets the field when it descends into a property, so the inlineoccurrence takes the first branch and the extracted definition — the same Zod
node — takes the second.
A definition only exists because a property refers to it, so parsing it as
though it were at the root is what produces the divergence.
parseOptionalDefis the only reader ofpropertyPathin the vendoredconverter (
Refs.tsdeclares it,parsers/object.tswrites it,parsers/optional.tsreads it), so no other parser can be affected by theomission or by this change.
Changes
zodToJsonSchema.ts: passpropertyPathalongsidecurrentPathwhen parsingan extracted definition, so the wrapper parsers see the position the
definition is actually referenced from.
tests/helpers/zod-shared-optional-definitions.test.ts: 20 tests — the sharedcase across
zodResponseFormat/zodTextFormat/zodFunction/zodResponsesFunction, aschemaDefinitionsentry that needs no sharing atall, the non-strict Realtime path, and controls for shapes that must not
change.
One line of behaviour, +8 −1 in
src/.Testing
Comparing the two runs test-by-test rather than by count: 20 added, 0 removed,
0 changed status. The 115 pre-existing failures are missing optional peer
dependencies in this environment (
ws,undici) plus unrelated suites(bedrock, realtime-websocket, x509, ecosystem-cli, oxlint-config); none are
under
tests/helpers/orsrc/_vendor/.With the source change reverted and the new tests kept, 14 of the 20 fail —
they exercise the change rather than restating it.
tsc --noEmitreports nothing for either changed file.Blast radius
16 schema shapes × the 3 strict helpers, dumped before and after and compared as
JSON:
.nullable().optional()— string, object, array, enum, ×3 fields, nested"not": {}count 18 → 0.nullable()without.optional(),.nullable().optional()used once, deep nesting, no sharingNotes
zod-shared-optional-definitions.test.tsfollows the one-file-per-topicconvention of the existing
tests/helpers/zod-*.test.tsfiles. Happy to fold itinto
tests/helpers/zod.test.tsinstead if that is preferred.I could not run
./scripts/formator./scripts/linthere —oxfmtandoxlintreject Node v23 when loading the TypeScript config. Please flag anyformatting the tooling wants and I will fix it.