fix(zod): keep every declared tuple position - #2512
Merged
Merged
Conversation
`parseTupleDef` builds `items` with a `map` that drops any element whose
parser returns `undefined`, while `minItems` and `maxItems` are taken from
`def.items.length`. `z.tuple([z.void(), z.string()])` therefore converts to
an array that requires exactly two entries but describes only one position,
and the string element is described at index 0 where the void element sits.
Keeping an unconstrained `{}` for such an element preserves the alignment and
constrains nothing, which is what the element already said.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cmun2
added a commit
to cmun2/openai-node
that referenced
this pull request
Aug 28, 2026
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>
1 task
HAYDEN-OAI
approved these changes
Sep 3, 2026
HAYDEN-OAI
left a comment
HAYDEN-OAI
left a comment
Contributor
There was a problem hiding this comment.
Reviewed tuple indexing and rest-element handling. The empty-schema fallback preserves declared positions without changing ordinary tuples. No substantive findings.
Static review; no tests executed.
Contributor
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 33822316129 --repo openai/openai-node \
--name castiron-custom-code-33822316129-1 --dir /tmp/castiron-custom-code-33822316129-1
git apply --stat /tmp/castiron-custom-code-33822316129-1/custom-code.patch
cat /tmp/castiron-custom-code-33822316129-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin dde19c5c72280516fdfd4e0f4fe987fd1a3eced1 a9fcd2137fd1b8433673a914f85aab8494113b10
python3 scripts/castiron/custom_code_report.py report \
--base dde19c5c72280516fdfd4e0f4fe987fd1a3eced1 \
--head a9fcd2137fd1b8433673a914f85aab8494113b10 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-a9fcd2137fd1
cat /tmp/castiron-custom-code-a9fcd2137fd1/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
HAYDEN-OAI
enabled auto-merge
September 4, 2026 00:22
HAYDEN-OAI
disabled auto-merge
September 4, 2026 00:29
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes being requested
parseTupleDefin the vendored Zod v3 converter buildsitemswith amapthat drops any element whose parser returnsundefined, whileminItemsandmaxItemsare taken fromdef.items.length. The two disagree whenever an element produces no schema.The array-form
itemskeyword 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, whereadditionalItemsthen 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 #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
parseObjectDefhas always setpropertyPath. The repro above needs no property at all and reproduces onmainas it stands. #2463 no longer touchesparseDef.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.mainat222f3d7and come from optional dependencies that are not installed in this environment (@aws-sdk/*,@smithy/*,ws)../scripts/lintand./scripts/format: clean for both files.