fix(zod): parse extracted definitions in their property position - #2463
fix(zod): parse extracted definitions in their property position#2463cmun2 wants to merge 28 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".
An accessor on `$ref` was already enough to leave the wrappers standing, but an accessor on a keyword that carries schemas hides everything under it. A `properties` getter reads as absent, the scan never sees the subtree, and a reference inside it survives only in what `JSON.stringify` produces. The three reasons a wrapper has to stay -- a relative reference, a `toJSON`, and now any accessor on `$ref` or a schema-bearing keyword -- are one traversal rather than three, since this runs over the whole definitions map. Descriptors are read for the keys a node has rather than every keyword it could have. 201 shared optional definitions convert in 17ms against 102ms before the reference index; the generalized guard accounts for the difference from 7ms and is what makes the hidden subtree visible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed in The guard is now over That generalization costs something on the path you flagged earlier: 201 shared optional definitions convert in 17ms rather than the 7ms the Regression: an override whose 53 focused tests, full suite 5,131 passed with the same 99 pre-existing failures this environment has without the change. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b36ec32108
ℹ️ 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 throughNullable = nullableBranchIndex(branches); | ||
| if (throughNullable !== -1) { | ||
| const inner = elementValue(branches, throughNullable); |
There was a problem hiding this comment.
When the exported converter targets openApi3, a shared property declared as z.string().optional().nullable() is materialized as { anyOf: [{ not: {} }, { type: 'string' }], nullable: true }, because parseNullableDef uses an OpenAPI nullable sibling rather than the anyOf: [X, null] shape recognized here. The subsequent collapse rejects the nullable sibling as a non-annotation, so the referenced definition retains not and differs from the inline { type: 'string', nullable: true } occurrence. Recognize this OpenAPI nullable form and collapse its inner optional union as well.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
The OpenAPI targets spell a nullable as a `nullable` sibling rather than an
`anyOf` branch, so a shared `z.string().optional().nullable()` materialized as
`{ anyOf: [{ not: {} }, { type: 'string' }], nullable: true }` while the inline
occurrence was `{ type: 'string', nullable: true }` -- the same divergence this
change exists to remove, still present on the base revision for that target.
The collapse rejected the sibling because it is not an annotation. The union is
an identity, so moving it onto the branch produces the inline encoding exactly.
It is carried only when the branch says nothing about `nullable` itself, since
merging over an existing value would be choosing between two claims.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed in
The union is an identity, so moving the sibling onto the branch produces the inline encoding exactly. It is carried only when the branch says nothing about Regression added for 54 focused tests, full suite 5,132 passed with the same 99 pre-existing failures this environment has without the change. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 421d6f1609
ℹ️ 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 someSchemaNode = (value: unknown, matches: (node: Record<string, unknown>) => boolean): boolean => { | ||
| if (Array.isArray(value)) { | ||
| // By index: an `override` can hand back an array of accessor entries, and |
There was a problem hiding this comment.
When an override-produced schema list has its own or inherited toJSON, this array branch returns before calling matches, so wrappersMustStay never sees the hook. JSON.stringify does invoke toJSON on arrays; if it emits a $ref into a pending definition's anyOf/1 branch, the collapse leaves a dangling reference in the serialized schema. Fresh evidence beyond the prior object-hook fix is that arrays take this early-return path, so serialization hooks must be checked before traversing their elements.
AGENTS.md reference: AGENTS.md:L98-L101
Useful? React with 👍 / 👎.
| const isOpenApiNullable = | ||
| key === 'nullable' && Object.getOwnPropertyDescriptor(second, 'nullable') === undefined; | ||
| if (!ANNOTATION_KEYWORDS.has(key) && !isOpenApiNullable) { | ||
| return schema; |
There was a problem hiding this comment.
When a shared property has redundant nullable wrappers, such as z.string().nullable().optional().nullable() with target openApi3, forced definition resolution produces { anyOf: [{ not: {} }, { type: 'string', nullable: true }], nullable: true }. Because the surviving branch already owns nullable, this condition refuses the rewrite, whereas the inline occurrence is { type: 'string', nullable: true }; the extracted definition therefore retains the unwanted not. Unlike the covered optional().nullable() case, this wrapper order reaches the collision path, so identical nullable values should be coalesced.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
| const branches = dataValue(schema, 'anyOf'); | ||
| // Accessor entries are left untouched: every read below -- destructuring | ||
| // included -- would otherwise run caller code from `override`. | ||
| if (!isInspectableBranchList(branches)) { | ||
| return schema; |
There was a problem hiding this comment.
When a reused property schema is a pipeline or intersection whose branches are nullable optionals—for example, z.string().nullable().optional().pipe(z.string().nullable().optional())—the inline property drops the optional wrappers, but forced definition resolution emits them inside allOf entries. This root-only check therefore leaves anyOf: [{ not: {} }, …] beneath allOf, and strict Zod v3 helpers serialize a schema that toStrictJsonSchema rejects. Fresh evidence after limiting traversal to nullable wrappers is that allOf is also produced by supported Zod parsers around the property-level optionality; normalize these generated conjunctive branches while retaining the existing reference-path checks.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
| const collapsed = | ||
| aliasWasReferenced && originatedInsideProperty(seen) && isPlainObject(materialized) | ||
| ? (collapseNeverBranch(materialized, removedWrappers) as JsonSchema7Type) | ||
| : materialized; |
There was a problem hiding this comment.
When schemaDefinitions lists an optional Leaf before a Container whose property uses Leaf, Leaf is materialized while its pre-seeded Seen entry still has no property context, so this condition leaves its not wrapper and the loop marks it processed. Materializing Container later records the first real property reference, but Leaf is never reconsidered; reversing the two definition keys changes the output, and strict helpers can therefore send an unsupported not solely because of object insertion order. Reprocess a definition when its previously missing reference context is populated, or defer context-dependent materialization until references are known.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
| wrapperSegments.push([...prefix, 'anyOf', '1']); | ||
| return { ...second, ...carried }; |
There was a problem hiding this comment.
When another schema contains $ref: '#/definitions/Maybe/anyOf/0' (or a descendant such as /anyOf/0/not), collapsing this optional definition removes that valid false-schema target. Only the surviving /anyOf/1 path is recorded here, so the later reference scan misses the branch that is actually discarded and commits a schema with a dangling pointer. Fresh evidence beyond the existing nested-wrapper checks is that both branches disappear from their old paths during collapse, not only branch 1; references into branch 0 must also keep the wrapper in place.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
…ition exists
Four of the five from the latest review; the fifth is noted on the pull request.
The collapse removes an `anyOf` entirely, so a reference into branch 0 -- the
`{ not: {} }` that matches nothing, a valid target -- loses its path just as one
into branch 1 does. Only branch 1 was recorded, so those references were missed.
`JSON.stringify` calls `toJSON` on an array as readily as on an object, and the
array case returned before the predicate ran, so a hook on a schema list was
never seen.
An OpenAPI `nullable` sibling was refused whenever the surviving branch already
had one, which is the redundant-wrapper spelling of the same value. Identical
values coalesce; different ones are still two claims and are left alone.
Whether a definition collapses depended on how far the loop had got: a
definition materialized before the property that references it had no context
yet and was never revisited, so `definitions` insertion order changed the
output. The decision now runs after every definition exists.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Four of the five from the latest review are fixed in References into the discarded branch. You are right that both branches leave their old paths. Only Array serialization hooks. The array case returned before the predicate ran, so a Duplicate OpenAPI nullable. The sibling was refused whenever the surviving branch already had Definition order. This one was the most useful to receive. Whether a definition collapsed depended on how far the loop had got — one materialized before the property that references it had no context yet and was never revisited, so Not in this commit: optionals inside generated Closing it means widening the property-context rule that an earlier round in this review narrowed on purpose, and I would rather do that as its own change than fold it into four unrelated fixes and hand you a fifth round of regressions from it. If you would prefer it here, say so and I will add it; if the nested definitions should instead inherit the context of the property they were hoisted out of, that is the smaller version and I will do that. 58 focused tests, full suite 5,136 passed with the same 99 pre-existing failures this environment has without the change. Reverting the source with the tests kept fails all four new cases. 201 shared optional definitions still convert in 16ms. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a84d9c42fe
ℹ️ 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".
| // identity the strict reduction applies, so it is done afterwards on the | ||
| // finished schema. The def itself still goes through `parseDef`, so | ||
| // `override` and `.describe()` behave exactly as they did. | ||
| const materialized = parseDef(def, { ...refs, currentPath: definitionPath }, true) ?? {}; |
There was a problem hiding this comment.
When definitions places an optional Leaf before a Container whose property references it, Leaf is force-resolved here before Container records its new Seen.propertyPath/referencePath. A public override that branches on the supplied seen context therefore materializes a different Leaf when those two definition keys are reversed; the later pass only applies collapseNeverBranch and never reruns the override. Fresh evidence beyond the resolved built-in ordering case is that the hook result itself remains cached before contexts are final, so reference discovery must precede context-sensitive override materialization.
AGENTS.md reference: AGENTS.md:L52-L57
Useful? React with 👍 / 👎.
| if (!stranded) { | ||
| definitions[key] = collapsed; |
There was a problem hiding this comment.
When the force-resolution override returns a schema object such as custom = { anyOf: [{ not: {} }, survivor] } for a property-derived optional definition, parseDef returns custom itself but this assignment substitutes the newly allocated collapsed object. The base revision stored the exact hook result in definitions, so consumers of the exported converter that associate state through a WeakMap or otherwise rely on hook-result identity now observe result.definitions[name] !== custom; avoid replacing override-owned objects or otherwise preserve the prior identity contract.
AGENTS.md reference: AGENTS.md:L52-L57
Useful? React with 👍 / 👎.
| // pointer names: a relative reference, whose depth is part of its meaning, | ||
| // and a serialization hook, which can produce a reference the scan cannot | ||
| // see without running caller code. | ||
| const unsafeToMove = wrappersMustStay(main) || wrappersMustStay({ definitions }); |
There was a problem hiding this comment.
When definitions has no property-derived optional candidate but an override returns a deeply nested valid schema, such as a long chain of not subschemas, this unconditional wrappersMustStay({ definitions }) walk recursively visits every level and can throw RangeError: Maximum call stack size exceeded. The base revision returned such a definition without this traversal, and the safety result is unused when pendingCollapses remains empty; determine the candidates first and skip the scan when there is nothing to rewrite, while using a non-recursive traversal when a scan is required.
AGENTS.md reference: AGENTS.md:L98-L101
Useful? React with 👍 / 👎.
An `override` can return a schema of any depth, and the safety scans walked the whole document whether or not a collapse was waiting on the answer. A long chain of `not` subschemas took the conversion down with a stack overflow on a definition the base revision returned without traversing anything. The candidates are decided first, and the scans run only when one exists. The walk is an explicit stack rather than recursion, so depth is bounded by memory rather than by the call stack when a scan is required after all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Stack overflow fixed in Deep override schemas. You are right that the scans ran whether or not anything depended on them. The candidates are decided first now, and the scans only run when a collapse is pending; the walk is an explicit stack rather than recursion, so a chain that reaches it later is bounded by memory rather than the call stack. Two regressions: a deep Override result identity. I do not think this one can be fixed while the change does what it is for. The definition being rewritten is the override's object — That leaves a real question, which is yours rather than mine: should a definition an Deferring override materialization. Same shape of question. Running the hook after reference contexts are final would make a context-sensitive 60 focused tests, full suite 5,138 passed with the same 99 pre-existing failures this environment has without the change. 201 shared optional definitions still convert in 17ms. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5710e9208
ℹ️ 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".
| } | ||
|
|
||
| if (seenItem && !forceResolution) { | ||
| if (seenItem.referencePath === undefined) { |
There was a problem hiding this comment.
When a non-preseeded schema is first reached outside a property and reused later inside one (or vice versa), its newly created Seen entry has no referencePath, so the second encounter satisfies this condition and overwrites the original propertyPath. With extract-to-root, for example, placing a shared optional first in an array branch and later in an object property incorrectly classifies the array-origin definition as property-derived and collapses its standalone encoding; reversing the occurrences changes the output. Initialize referencePath for ordinary newly seen entries, or otherwise restrict this update to entries pre-seeded from definitions.
AGENTS.md reference: AGENTS.md:L52-L57
Useful? React with 👍 / 👎.
| for (const { key, collapsed, wrapperPaths } of pendingCollapses) { | ||
| const stranded = | ||
| unsafeToMove || | ||
| wrapperPaths.some((wrapperPath) => emittedRefs.some((ref) => ref.includes(wrapperPath))); |
There was a problem hiding this comment.
With N shared optional definitions, this still compares each candidate's wrapper paths against all O(N) emitted references, so the normal no-stranded-reference case remains O(N²). Fresh evidence after the earlier report is that references are now collected once, but emittedRefs.some(...) is still nested inside wrapperPaths.some(...) for every pending collapse; build a prefix index or test all wrapper paths during one reference traversal.
AGENTS.md reference: AGENTS.md:L119-L123
Useful? React with 👍 / 👎.
| // standalone definition the caller asked for by name, and rewriting it | ||
| // would change a schema nothing referenced. | ||
| if ( | ||
| !aliasWasReferenced(seen, definitionPath) || |
There was a problem hiding this comment.
When a caller supplies an optional schema through definitions but selects $refStrategy: 'none', using the same schema in a property does not emit a reference to that definition—the property is parsed inline. Nevertheless, the property encounter replaces the pre-seeded Seen entry, this condition classifies the supplied definition as referenced, and its standalone anyOf: [{ not: {} }, …] form is collapsed. The base revision preserves that explicitly supplied, unreferenced definition, so gate this rewrite on an actual emitted reference rather than path context alone.
AGENTS.md reference: AGENTS.md:L52-L57
Useful? React with 👍 / 👎.
…rences Whether an extracted definition is the encoding a property asked for was read off `refs.seen`, which answers a different question. It is keyed by the Zod def, so two names for one schema shared a single entry; it records the first context reached, so definition order and branch order decided the answer; and it records reads that emitted no `$ref` at all, so a `$refStrategy` that inlines everything still looked referenced. The emitted document answers it directly: collect every `$ref` once, with whether it sits under a `properties` map, and collapse a definition only when every reference to it came from a property. A definition reached from both a property and an array branch now keeps its wrapper regardless of which branch is written first, `'none'` collapses nothing, and of two aliases only the one a property points at is rewritten. The same pass indexes the pointer prefixes the stranding check needs, so that check is a set lookup rather than a scan of every reference per candidate. Collapse candidacy is computed first, so a schema with no shared optionals never walks the document at all. Converting 3200 shared optionals: 109ms -> 65ms, with no change on schemas that collapse nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed What changedWhether an extracted definition is the encoding a property asked for was read off The finished document answers it directly. Every The three findingsPreserve the first context for ordinary seen entries — a shared optional used in an array branch and in an object property now produces the same definition regardless of which branch is written first. Pinned by a test that fails on the previous revision. Require an emitted reference before collapsing — with Index wrapper paths instead of rescanning every ref — the same traversal now indexes the pointer prefixes the stranding check needs, so that check is a set lookup rather than a scan of every reference per candidate. Collapse candidacy is computed before any scan, so a schema with no shared optionals never walks the document.
Scaling from 1600 to 3200 goes from 2.68x to 2.05x. Median of 7 samples. A 1000-property schema that collapses nothing measures 3.24 ms before and 3.05 ms after, so the reordering costs nothing on schemas the rewrite does not apply to. The alias case falls out of the same rule rather than needing its own check: given Re-verifying the change requestEach of the three repros from the change request, run on
A Verification
Still openThree findings from the previous rounds are unaddressed on purpose, with the reasoning already posted in their threads: normalizing optionals inside generated |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad5bffc9d0
ℹ️ 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 pendingCollapses = candidates.filter( | ||
| ({ path }) => referencedFromProperty.has(path) && !referencedElsewhere.has(path), | ||
| ); |
There was a problem hiding this comment.
When a reused optional field is a recursive lazy union whose array branch points back to the same optional schema, the definition's self-$ref under items is collected with insideProperty: false. This condition consequently excludes the definition even though its external reference sites are object properties, leaving the outer anyOf: [{ not: {} }, …] intact; Zod v3 strict helpers then send the unsupported not keyword to Structured Outputs. Internal self-references should not classify the definition as having a standalone call site.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
| const ref = dataValue(current, '$ref'); | ||
| if (typeof ref === 'string') { | ||
| found.push({ ref: normalizedRef(ref), insideProperty }); | ||
| } |
There was a problem hiding this comment.
When the jsonSchema2019-09 converter's override output contains a valid $recursiveRef into a candidate such as #/definitions/Maybe/anyOf/1, this collector ignores it because it recognizes only $ref; wrappersMustStay misses the same keyword. A normal property $ref can therefore make Maybe eligible for collapse, removing the recursive reference's target and emitting a dangling pointer. Index $recursiveRef alongside ordinary references, or conservatively keep wrappers when it is present.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
| const normalizedRef = (ref: string): string => { | ||
| try { | ||
| return decodeURIComponent(ref); | ||
| } catch { | ||
| return ref; | ||
| } |
There was a problem hiding this comment.
When a Zod v3 strict helper receives a schemaDefinitions name containing literal percent-escape text, such as A%2FB, the converter initially emits #/definitions/A%2FB while the candidate path retains that raw spelling. Decoding here changes only the emitted ref to #/definitions/A/B, so the definition is not recognized as property-referenced and retains its unsupported not; afterward escapeSchemaDefinitionRefs correctly rewrites the actual wire ref to #/definitions/A%252FB, but it is too late to reconsider the collapse. Fresh evidence beyond encoded override refs is that helper-generated refs are escaped only after this converter returns, so comparison must use the same escaped representation or distinguish generated raw refs.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
| const ref = dataValue(current, '$ref'); | ||
| if (typeof ref === 'string') { | ||
| found.push({ ref: normalizedRef(ref), insideProperty }); | ||
| } |
There was a problem hiding this comment.
When override output beneath a property declares a nested $id and uses $ref: '#/definitions/Maybe' to reference that nested resource's own definition, this collector treats the raw fragment as though it targeted the document-root Maybe. An explicitly supplied but otherwise unreferenced top-level optional definition with that name is consequently misclassified as property-referenced and collapsed, even though no emitted reference names it. Track $id resource scopes while resolving local refs, or conservatively avoid this rewrite when nested resources are present.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
| const collapsedInner = collapseNeverBranch(inner, wrapperSegments, [ | ||
| ...prefix, | ||
| 'anyOf', | ||
| String(throughNullable), | ||
| ]); |
There was a problem hiding this comment.
When a force-resolution override for a property-derived optional definition returns a deeply nested chain of nullable wrappers ending in anyOf: [{ not: {} }, X], this recursive call consumes one JavaScript stack frame per wrapper and eventually throws RangeError before the iterative reference scan runs. Fresh evidence after the deep-scan fix is that candidate normalization itself remains recursive, so an arbitrarily deep override schema still crashes conversion whereas the base revision returned it unchanged; descend and rebuild with an explicit stack instead.
AGENTS.md reference: AGENTS.md:L98-L101
Useful? React with 👍 / 👎.
Three things came out now that the decision is read off the emitted document rather than inferred from `refs.seen`. `Seen.propertyPath` and `Seen.referencePath` are written and never read. They existed to carry the reference context forward; the emitted `$ref` carries it instead. `Refs.ts` and `parseDef.ts` go back to their original contents, so the converter's shared dispatch is untouched by this PR. A recursive definition whose branch points back at itself was being read as a call site outside a property, which kept the wrapper the inline occurrence does not have. A definition describing itself does not decide which encoding it owes; it still counts as a pointer for the stranding check, where removing a wrapper strands a self-reference exactly as it would an external one. The tuple positioning fix is a separate, pre-existing bug -- it reproduces with no part of this change applied -- and is now openai#2512. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed The recursive self-referenceReading the decision off the emitted document introduced this, and it is a real regression against the previous revision: a definition whose own branch points back at itself was counted as a call site outside a property, so it kept a wrapper the inline occurrence does not have. A definition describing itself is not a second call site asking for a different encoding, so it no longer decides which encoding is owed. It still counts as a pointer for the stranding check — removing a wrapper strands a self-reference into it exactly as it would an external one. Pinned by a test asserting the definition and the inline occurrence agree for Narrowing
The tuple positioning fix moved to #2512. It reproduces with no part of this change applied — Where the remaining findings standRe-running each open thread's own repro against
Four are outstanding and I have not implemented them, with the reasoning in their threads: Verification
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06c5d3d697
ℹ️ 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".
| if (!ANNOTATION_KEYWORDS.has(key) && !isOpenApiNullable) { | ||
| return null; | ||
| } | ||
| carried[key] = dataValue(schema, key); |
There was a problem hiding this comment.
When an override returns a collapsible schema whose outer annotation is undefined, such as { anyOf: [{ not: {} }, { type: 'string', description: 'inner' }], description: undefined }, the outer property would be omitted during JSON serialization, leaving the branch description intact. Copying it into carried instead overwrites the surviving branch's value, so the collapsed wire schema loses that metadata. Skip annotation values that serialization omits rather than carrying them onto the branch.
AGENTS.md reference: AGENTS.md:L58-L60
Useful? React with 👍 / 👎.
| for (const key of Object.keys(definitions)) { | ||
| const ownerPath = pathOf(key); | ||
| for (const { ref, insideProperty } of collectRefs(definitions[key])) { | ||
| add(ref, insideProperty, ownerPath); |
There was a problem hiding this comment.
When a reused property schema is a container such as outer = z.array(inner) and inner is optional, the first inline outer occurrence strips inner's optional wrapper, while materializing outer later can extract inner behind an items $ref. This loop scans every definition with insideProperty: false, so that reference is classified as standalone even when every reference to outer comes from an object property; inner consequently retains anyOf: [{ not: {} }, …] and the Zod v3 strict helper sends an unsupported schema. Propagate the referring definitions' property/standalone contexts through the definition-reference graph before deciding the collapse.
AGENTS.md reference: AGENTS.md:L61-L63
Useful? React with 👍 / 👎.
`carriedSiblings` moved every annotation on the wrapper onto the surviving
branch, including one whose value is `undefined`. JSON has no such value, so
the wrapper's key would have been dropped on serialization and the branch's
own value was what reached the wire. Copying it deleted that value instead of
moving one the wrapper stated: an override supplying
`{ anyOf: [{ not: {} }, { type: 'string', description: 'inner' }],
description: undefined }` lost the branch description, which the base
revision kept.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed in
Not reproducible as a regression at Intentionally not applied at 63 tests in the file pass. Full suite 5141 passed, with the 18 failing files identical to the parent commit and caused by optional dependencies absent in this environment. |
HAYDEN-OAI
left a comment
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
One correctness issue in the reference-safety scan; details inline.
Static review; no tests executed.
| if (kind === 'map' && isPlainObject(child)) { | ||
| for (const name of Object.keys(child)) { | ||
| const entry = dataValue(child, name); |
There was a problem hiding this comment.
[P2] Treat schema-map accessors as hidden references
dataValue(child, name) returns undefined for an accessor, and this traversal never checks the schema-map container itself. With a and b sharing z.string().nullable().optional() and the options name: 'p', nameStrategy: 'duplicate-ref', and $refStrategy: 'extract-to-root', an override for a third property can return:
{
type: 'object',
properties: {
get x() {
return { $ref: '#/definitions/p_properties_a/anyOf/1' };
},
},
}Both collectRefs() and wrappersMustStay() miss this reference, so the new code collapses definitions.p_properties_a. JSON.stringify() then invokes the getter and emits a $ref to the removed anyOf/1 branch. The base revision preserves that branch, so this introduces a dangling reference in a previously valid serialized schema.
Please treat accessor-backed schema-map entries as hidden references before allowing the collapse. The same guard should cover map-level serialization hooks and accessor-backed schema-list entries without invoking caller code.
HAYDEN-OAI
left a comment
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
This fixes a supported API use case: sharing nullable optional schemas should not introduce unsupported not into strict Structured Outputs.
For scope, please focus the implementation on schemas generated for the Zod v3 strict helpers, preserving existing non-strict conversion and caller-supplied override behavior. Broader reference normalization, override identity changes, and override execution ordering can stay outside this PR. The current implementation extends the general converter substantially beyond the property-context change described in the PR body; please update the description to match the final approach.
The current head still has the dangling-reference regression described inline. Please resolve that within the narrower scope before merging. For caller-supplied schema forms whose rewrite cannot be established as safe, preserving the previous output is sufficient; this fix does not need to expand the converter's general schema support.
Static review of 74868d8211eb388cc170fb94e5896e2b8bba5d84; no tests run.
- [x] I understand that this repository is auto-generated and my pull
request may not be merged
## Changes being requested
`parseTupleDef` in the vendored Zod v3 converter builds `items` with a
`map` that drops any element whose parser returns `undefined`, while
`minItems` and `maxItems` are taken from `def.items.length`. The two
disagree whenever an element produces no schema.
```ts
zodToJsonSchema(z.tuple([z.void(), z.string()]));
```
```jsonc
// before
{ "type": "array", "items": [{ "type": "string" }], "minItems": 2, "maxItems": 2 }
// after
{ "type": "array", "items": [{}, { "type": "string" }], "minItems": 2, "maxItems": 2 }
```
The array-form `items` keyword is positional, so before this change the
schema requires exactly two entries and describes the *string* element
at index 0 — the position the void element occupies. A value the Zod
schema accepts is rejected by the schema generated from it, and vice
versa. The same shift happens with a rest element, where
`additionalItems` then applies one position too early.
Keeping an unconstrained `{}` for such an element preserves the
alignment and constrains nothing, which is what the element was already
saying. Zod v4 describes both positions here, so this also brings the v3
output back in line with v4.
Tuples whose elements all produce schemas are unaffected; a test pins
that.
## Additional context & links
Split out of openai#2463. It surfaced there as a regression an earlier
revision of that branch introduced, and while fixing it I found the same
drop happens without any of that change — a tuple under an object
property already hit it, because `parseObjectDef` has always set
`propertyPath`. The repro above needs no property at all and reproduces
on `main` as it stands. openai#2463 no longer touches `parseDef.ts`, so the
path that first exposed this is gone from it entirely and the two are
independent.
Scoped to one function in
`src/_vendor/zod-to-json-schema/parsers/tuple.ts`.
**Testing**
- `pnpm vitest run tests/helpers/zod-tuple-positions.test.ts` — 4
passed.
- With the source change reverted and the tests kept, 3 of the 4 fail.
The fourth is a control asserting that ordinary tuples do not change, so
it passes either way.
- Full suite: 5353 passed. The 26 failing files are identical to those
on `main` at `222f3d7` and come from optional dependencies that are not
installed in this environment (`@aws-sdk/*`, `@smithy/*`, `ws`).
- `./scripts/lint` and `./scripts/format`: clean for both files.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Hayden <hayden@openai.com>
…accessors Two changes, both narrowing. The rewrite now runs only when `openaiStrictMode` is set. The divergence it fixes is that `not` reaches Structured Outputs, which is a constraint of that path alone; every other caller of the converter keeps the output it had. Ten non-strict shapes -- shared optionals, supplied `definitions`, relative references, `'none'`, recursion, `openApi3`, aliases, tuples -- were dumped before and after and are byte-identical to the base revision. `hasHiddenReference` now also looks one level into a schema map or list. An accessor on `properties.x` rather than on `properties` itself passed the old check, and the walk below then read `undefined` and stopped, so the reference the getter returns was never seen and `JSON.stringify` emitted it into a branch this rewrite had already removed. Whether such a getter would in fact return a reference is not knowable without running caller code, so its presence is treated as one and the schema is left alone. That is enough to drop the self-reference bookkeeping added earlier: a recursive definition materializes a shape that differs from the inline occurrence by more than the wrapper, so it now keeps what it had rather than being reasoned about. The `openApi3` and non-strict Realtime cases go with it -- they were general converter behaviour rather than this fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8467c52c60
ℹ️ 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".
| if (value === undefined) { | ||
| continue; |
There was a problem hiding this comment.
When an override supplies a collapsible wrapper with a function- or symbol-valued annotation (for example, default: () => value) and the surviving branch has its own annotation, JSON serialization would omit the wrapper value and preserve the branch value. This condition skips only undefined, so the omitted value is carried onto the branch, overwrites its annotation, and is then itself omitted from the serialized schema. Fresh evidence beyond the earlier undefined fix is that JSON also omits function and symbol values from object properties; apply the same filtering to all three types.
AGENTS.md reference: AGENTS.md:L98-L101
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0d4afe0. Correct, and it is the same defect the undefined condition was written for — JSON.stringify drops a function or a symbol from an object property exactly as it drops undefined, so the wrapper never states one either.
Reproduced with { anyOf: [{ not: {} }, { type: 'string', description: 'inner' }], description: () => 'fromWrapper' } on a property-derived optional definition: definitions.p_properties_a serialized as { type: 'string' } at 8467c52, and is { type: 'string', description: 'inner' } at 0d4afe0. The symbol case behaves identically.
The condition now skips all three, and the existing regression is parameterized over them, so it fails on two of the three with the source change reverted.
|
Fixed in The dangling reference. Scope. The rewrite now runs only when That gate made the general-converter work removable rather than fixable. The self-reference bookkeeping is gone: a recursive definition materializes a shape differing from the inline occurrence by more than the wrapper, so it now keeps what it had. The Still declining to rewrite, and preserving previous output, for: relative references, The description has been rewritten to match. 62 tests in the file pass; the two new ones fail with this change reverted. Full suite 6,253 passed — the 2 failures are |
The wrapper's annotation was skipped only when it held `undefined`, but
serialization drops a function or a symbol from an object property just
the same. An `override` supplying a collapsible wrapper with such a value
carried it onto the surviving branch, overwrote the branch's own
annotation, and was then itself omitted from the serialized schema, so
the branch lost a value it did state.
With `{ anyOf: [{ not: {} }, { type: 'string', description: 'inner' }],
description: () => 'fromWrapper' }`, `definitions.p_properties_a`
serialized as `{ type: 'string' }`; it is now
`{ type: 'string', description: 'inner' }`, matching the `undefined`
case. The existing regression covers all three values.
## Summary - Capture the original `parameters` schema once in `zodFunction()` and `zodResponsesFunction()`, and use that same reference for both JSON Schema generation and argument parsing. - Add a focused public-entrypoint regression covering Zod v3, v4, and v4 Mini through Chat Completions and Responses, in both parsing and streaming modes. - Preserve callbacks, schema-object identity, serialization, and public types. No generated files, dependencies, or parser implementations are changed. ## Reproduction Create a tool using a six-character string schema, then reuse its options object by assigning a four-character schema to `options.parameters`. On main, the existing tool still sends the six-character JSON Schema, but its parser follows the replacement schema: it rejects valid original arguments and accepts replacement-only arguments. A newly constructed tool correctly uses the replacement. The fix binds each tool's parser to the schema used when that tool was constructed, matching the existing behavior of the Standard Schema helpers. ## Validation - New 72-case regression: 24 failures and 48 passing controls before the source change; all 72 pass afterward. - Full canonical handwritten suite: 7,872 tests passed across 206 files. - Built-package CJS/ESM parsing and CJS streaming checks pass on exact Node 22.0.0, Node 24.19.0, and Node 26.7.0, including retained-tool and newly-created-tool acceptance/rejection controls. - All 651 published declaration files are byte-identical to main. Repository TypeScript 6, published-source TypeScript 4.9/6, CJS/ESM build, formatting, canonical lint, and `git diff --check` pass. - `publint` passes with the same pre-existing vendored-export warning as main. - Independent adversarial review checks single-read getters, parser receiver/schema identity, callback identity, unchanged serialized tool definitions, validation, and public parse/stream regression coverage. The generated resource suite and remaining platform integrations are left to CI. This is separate from openai#2463's extracted-definition conversion work.
2 participants
Summary
Fixes #2462.
Sharing a
.nullable().optional()Zod v3 schema across more than one field makes the Zod v3 strict helpers emitanyOf: [{ "not": {} }, …]into the extracted definition, while the inline occurrence of the same schema is emitted without it.notis outside the subset strict Structured Outputs accepts —src/lib/transform.tslists it as unsupported — so a supported way of writing the schema produces a request the API rejects.Approach
zodToJsonSchemamaterializes a definition because some property$refs it, then parses it withcurrentPathset to the definition andpropertyPathleftundefined.parseOptionalDefbranches onpropertyPath: inside a property it parses the inner type directly, otherwise it wraps inanyOf: [{ not: {} }, inner]. The inline occurrence takes the first branch and the extracted definition takes the second.When
openaiStrictModeis set, a definition that every emitted reference reaches from inside a property is rewritten to the encoding the inline occurrence gets. The decision is read off the finished document rather than inferred fromrefs.seen, which is keyed by Zod def and so cannot tell two names for one schema apart.Everything outside the strict helpers is unchanged. Ten non-strict shapes — shared optionals, supplied
definitions,$refStrategy: 'relative','none', recursion,openApi3, aliases, tuples — were dumped before and after and are byte-identical to the base revision.Where it declines to rewrite
The rewrite fires only where it can be shown to preserve meaning. Each of these keeps the previous output:
toJSONanywhere reachable, which can emit a pointer the scan cannot seeVerification
tests/helpers/zod-shared-optional-definitions.test.ts— 62 passed. With the source change reverted, the tests asserting the new behaviour fail; the rest are controls that pass either way.ecosystem-cloudflare-credential-lifecycle-securityand fail identically with this change reverted — a filesystem-inode limitation of this container../scripts/lint,./scripts/format,tsc --noEmit— clean for both changed files.