Skip to content

fix: preserve ProductSchema ZodObject ergonomics#1972

Merged
bokelley merged 9 commits into
mainfrom
bokelley/productschema-dx-breakage
May 24, 2026
Merged

fix: preserve ProductSchema ZodObject ergonomics#1972
bokelley merged 9 commits into
mainfrom
bokelley/productschema-dx-breakage

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Summary

Fixes #1970 by preserving ProductSchema as a ZodObject when the generated schema is only intersecting the real object shape with opaque Record<string, unknown> variant markers.

What changed

  • Extends the Zod codegen post-process to unwrap z.union([RecordUnknown...]).and(z.object(...)) into the object side when every union arm is a redundant Record<string, unknown> marker.
  • Regenerates ProductSchema as a plain z.object(...), restoring .extend(), .omit(), and .pick() for downstream consumers.
  • Adds runtime and compile-time regression coverage for ProductSchema composition helpers.

Validation

  • npm run build:lib
  • npm run typecheck
  • NODE_ENV=test node --test-timeout=60000 --test-force-exit --test test/lib/zod-schemas.test.js test/lib/inline-enum-arrays.test.js
  • npm run ci:codegen-strict
  • npm run ci:docs-check

Review

Reviewed by JavaScript/codegen, DX/docs, and code-review expert agents. The initial unrelated inline-enums.generated.ts churn was removed after review; the final PR scope is limited to the ProductSchema codegen fix and regression tests.

@bokelley bokelley marked this pull request as ready for review May 24, 2026 10:50
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sound fix. Blocked on the changeset.

The change itself is right. V1ProductNamedFormatReferenceSchema and V2ProductInlineFormatDeclarationsSchema at src/lib/types/schemas.generated.ts:513,515 are both z.record(z.string(), z.unknown()) — opaque markers that accept anything. The z.union([V1, V2]).and(z.object(...)) wrap was a TS-side artifact that bought zero runtime discrimination and cost .extend/.omit/.pick/.shape. Unwrapping to the object side is runtime-equivalent and restores the surface docs/ZOD-SCHEMAS.md:107 already advertises. The new pass is correctly conservative: every union arm must be a literal or aliased z.record(z.string(), z.unknown()) before it unwraps, so future arms with real shape are safe.

Must fix before merge

  1. Add a changeset. src/lib/types/schemas.generated.ts is in the published surface; adopters observe typeof ProductSchema flipping from ZodIntersection to ZodObject. Patch bump is the right shape — DX restoration of a documented helper, no validation behavior change. CI's Check for changeset is failing on this PR; the project's own rule in CLAUDE.md § "When to Create a Changeset" is explicit. Run npm run changeset and pick patch.

Things I checked

  • unwrapRecordUnionIntersections at scripts/generate-zod-from-ts.ts:566-608 — balanced-bracket scan with quote-skipping is symmetric with the existing unwrapRecordIntersections parser. Union-end-must-be-) guard at L578 prevents misfire on bare z.union([...]) not followed by .and(.
  • collectRedundantRecordSchemaNames at L544 — pre-scans the file once for aliased markers (V1ProductNamedFormatReferenceSchema, V2ProductInlineFormatDeclarationsSchema, plus the four Fixed/MultiSize/Responsive/None aliases that back SizeModeMutexSchema). Detection is correct for the current ts-to-zod output.
  • Pass ordering at L284-299 — new Pass 3 sits between record-first unwrap and never-union strip. Non-overlapping inputs; no interference.
  • Generated diff at src/lib/types/schemas.generated.ts:7901,7993 is the only expected change. Verified grep -nE 'z\.union\(\[.*\]\)\.and\(z\.object' returns zero remaining inline matches on the regenerated file.
  • Type test at src/type-tests/zod-schema-ergonomics.type-test.ts and runtime test at test/lib/zod-schemas.test.js:22-38 are belt-and-braces against codegen regression. Good shape.

Follow-ups (non-blocking — file as issues)

  • Named-alias intersection has the same DX trap. SizeModeMutexSchema = z.union([FixedSchema, MultiSizeSchema, ResponsiveSchema, NoneSchema]) at src/lib/types/schemas.generated.ts:632 is a union of four record-only aliases — structurally identical to the V1/V2 case. CanonicalFormatDisplayTagSchema (L634), CanonicalFormatImageSchema (L701), and CanonicalFormatHTML5BannerSchema (L734) are SizeModeMutexSchema.and(z.object({...})) and still lose .shape/.extend. Pre-existing, not a regression — but the same shape, and easy to extend the new pass to substitute the named alias if every arm of its right-hand union is opaque. Worth a follow-up issue.
  • Alias regex at scripts/generate-zod-from-ts.ts:546 requires exact whitespace + trailing semicolon. \s+ and an optional semicolon would future-proof for near-zero cost; if ts-to-zod ever line-wraps long declarations, alias detection silently goes dark.
  • JSDoc on ProductSchema. With the union name no longer visible in the schema type, IntelliSense loses its V1-vs-V2 hint. A one-line @remarks on the regenerated schema pointing at format_ids vs inline formats as the discrimination signal would help adopters who land on the type without the docs.

Minor nits (non-blocking)

  1. Runtime test claims more than it proves. test/lib/zod-schemas.test.js:22-38 types .omit as a function but doesn't actually invoke it. The type-test covers the compile side, so combined coverage is fine — but the test name says "exposes helpers" and the body only validates two of the three. Either invoke .omit({ description: true }) or rename.

Request changes for the changeset. Drop one in and this lands as a clean patch.

@bokelley bokelley changed the title [codex] Preserve ProductSchema ZodObject ergonomics fix: preserve ProductSchema ZodObject ergonomics May 24, 2026
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codegen fix is sound. Block is on the missing changeset — Check for changeset CI gate is FAILURE and scripts/generate-zod-from-ts.ts + src/lib/types/schemas.generated.ts both ship published behavior.

MUST FIX

1. Missing changeset. scripts/generate-zod-from-ts.ts (+237/-3) regenerates src/lib/types/schemas.generated.ts (in the files field of package.json). The named-union pass introduced in 742f0d6 collapses SizeModeMutexSchema.and(z.object(...)) for CanonicalFormatDisplayTagSchema/CanonicalFormatImageSchema/CanonicalFormatHTML5BannerSchema — that's a wire-shape delta for adopters who imported those .d.ts types as ZodIntersection. CI surfaces this; treat the failing check as the block. Per CLAUDE.md § "Critical rules": "ALWAYS create a changeset for ANY library/CLI code change before pushing a PR."

A patch is enough — this restores documented ZodObject ergonomics (.extend()/.omit()/.pick()/.shape) that 8.1.0-beta.* regressed. Mention #1970 and that the named-union arm of the fix extends to CanonicalFormat* schemas. npm run changeset, commit, push.

Things I checked

  • Information-preservation. V1ProductNamedFormatReferenceSchema, V2ProductInlineFormatDeclarationsSchema (schemas.generated.ts:513, 515) and all four SizeMode arms — FixedSchema, MultiSizeSchema, ResponsiveSchema, NoneSchema (L624-630) — are bare z.record(z.string(), z.unknown()). Stripping the intersection on top of .passthrough() is runtime-equivalent. Not a witness-not-translator violation.
  • Conservative guard. collectRedundantRecordUnionSchemaNames (generate-zod-from-ts.ts:561) requires .every(member => isRedundantRecordMember(...)) — a future spec version that adds real fields to a marker arm will correctly leave the intersection in place. The doc comment at L640-644 names that contract.
  • Pass ordering. The new passes 3 and 4 sit between unwrapRecordIntersections and stripNeverUnionIntersections and don't compete with postProcessForPassthrough.
  • Test coverage. Runtime gate at test/lib/zod-schemas.test.js:24-40 plus the .d.ts regex regression at L46-54. Type-test at src/type-tests/zod-schema-ergonomics.type-test.ts exercises .extend(), .omit(), .pick() against ProductSchema.
  • code-reviewer: sound-with-caveats, no blockers.
  • Package-lock bump from 7.10.1 → 8.1.0-beta.7 is sync-with-main, not a manual package.json edit.

Follow-ups (non-blocking — file as issues)

  • Word-boundary match in unwrapNamedRecordUnionIntersections. content.startsWith(\${name}.and(`, i)at generate-zod-from-ts.ts:655 is not identifier-aware. No collision today, but the first time codegen producesMyFixedSchemaalongside a redundantFixedSchemaunion, the scanner positioned two characters in will match the suffix and corrupt output. Cheap fix: requirei === 0 || !/[A-Za-z0-9_$]/.test(content[i-1])` before accepting the match.
  • Pass 4 has no targeted ergonomics test. src/type-tests/zod-schema-ergonomics.type-test.ts only exercises ProductSchema (Pass 3 inline-union case). Add one .extend() / .omit() / .pick() assertion against CanonicalFormatDisplayTagSchema (or one of the other named-union cases) so a future TS serializer that prints ZodIntersection<typeof SizeModeMutexSchema, ZodObject<...>> instead of the fully expanded form doesn't slip past the d.ts regex.
  • Loud-fail on unbalanced parens. readBalancedBody returning undefined is swallowed at generate-zod-from-ts.ts:676 — content is preserved, but a malformed ts-to-zod payload should crash the codegen rather than emit silently, mirroring the z.any() stub guard elsewhere in this file.

Minor nits (non-blocking)

  1. The named-union doc comment is load-bearing. generate-zod-from-ts.ts:636-645 explains the "marker-only era" contract — keep it. If you add the recursive union-of-unions collection later, update the comment in the same diff so the invariant doesn't drift from the code.

Ship it once the changeset lands and CI is green.

Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Restores ZodObject ergonomics without changing validation behavior — the marker-only union arms (Record<string, unknown>) are structurally vacuous against the existing .passthrough(), so z.union([Record...]).and(z.object({...}).passthrough())z.object({...}).passthrough() for parse semantics. Witness, not translator — wire behavior is unchanged; only the JS API surface comes back.

Things I checked

  • Pass 3 (unwrapRecordUnionIntersections, scripts/generate-zod-from-ts.ts:593-635) bails correctly via members.every(isRedundantRecordMember) — a single non-marker arm in a discriminated union leaves the intersection intact.
  • All four union arms in SizeModeMutexSchema (schemas.generated.ts:624-632) and both arms of the ProductSchema union (schemas.generated.ts:513,515) are top-level z.record(z.string(), z.unknown()) — the unwrap precondition holds.
  • Pass order: record-and → record-then-and → union-then-and → named-union-then-and → never-union. Passes inspect distinct syntactic anchors; pass 4 re-collects union names after pass 3 mutation, so the ordering is robust.
  • Changeset present, patch is the right shape — this is a regression restore, not a new method surface. ProductSchema shipped with .extend/.omit/.pick before the ts-to-zod V1/V2 marker split landed.
  • z.infer<typeof ProductSchema> is structurally identical pre/post: Record<string,unknown> & T ≡ T when T has typed keys plus passthrough index signature. No type narrowing on adopters.
  • Type test at src/type-tests/zod-schema-ergonomics.type-test.ts plus the .d.ts regression grep at test/lib/zod-schemas.test.js:44-53 cover the API surface and the codegen invariant respectively.
  • Package-lock sync (7.10.1 → 8.1.0-beta.7) is bringing the lockfile in line with the release-PR-driven package.json, not a manual version bump. PR doesn't touch package.json.

Follow-ups (non-blocking — file as issues)

  1. Identifier-boundary guard in pass 4 at scripts/generate-zod-from-ts.ts:656. content.startsWith(\${name}.and(`, i)does no left-boundary check. Currently safe —SizeModeMutexSchemais the only collected name and nothing in the generated file ends with that string — but if a future upstream schema is namedBarSizeModeMutexSchemaorXSizeModeMutexSchema, the loop would emit the prefix character and drop the rest of the identifier. One-line fix: gate the match on (i === 0 || !/[A-Za-z0-9_$]/.test(content[i-1]))`. Latent, not loaded — file it before the next upstream marker-schema rename.
  2. Pass 4 fallback at scripts/generate-zod-from-ts.ts:677-678 is silent recovery. When readBalancedBody returns undefined (malformed input at EOF), the branch emits just the name and resumes inside the unclosed argument list. Either throw new Error (codegen-input malformation is a real bug) or emit the full ${name}.and( substring so output round-trips byte-for-byte.
  3. Type-test coverage gap at src/type-tests/zod-schema-ergonomics.type-test.ts. Asserts .extend/.omit/.pick only on ProductSchema. The same transform also fixes CanonicalFormatDisplayTagSchema, CanonicalFormatImageSchema, CanonicalFormatHTML5BannerSchema — add type-level assertions for those three so a future regression there is caught at compile time rather than only by the runtime .d.ts grep.

Minor nits (non-blocking)

  1. .d.ts grep at test/lib/zod-schemas.test.js:46 is shape-fragile. The pattern z\.ZodIntersection<z\.ZodUnion<readonly \[z\.ZodRecord depends on Zod v4's exact emitter format (the readonly tuple prefix in particular). If Zod adjusts how it stringifies tuple element types, the test goes vacuously green. Pair it with a positive assertion — confirm ProductSchema: z.ZodObject (or similar) appears in the declarations — so the test fails closed if the regression returns under a different format.
  2. Pass 3 docstring at scripts/generate-zod-from-ts.ts:585-592 should mention the z.object( shape gate explicitly, matching pass 4's wording. The current text describes the inline pattern but the body-prefix check at line 619 is what makes the unwrap safe — call that constraint out so the next editor doesn't broaden the right-hand side.

code-reviewer and javascript-protocol-expert converged independently on the same Major finding (latent identifier-boundary at line 656) and the same verdict (sound-with-caveats). Patch type is defensible.

Approving on the strength of the validation-equivalence audit plus the pass-3 every-bail discriminator-safety check. The line-656 guard is worth a follow-up PR before the next upstream marker-schema rename — cheap to add, prevents a silent codegen corruption that wouldn't surface until the next regen.

aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes May 24, 2026
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Runtime-safe ergonomics restoration on the SDK's most-composed schema — the four union arms (FixedSchema, MultiSizeSchema, ResponsiveSchema, NoneSchema) and the two product variants (V1ProductNamedFormatReferenceSchema, V2ProductInlineFormatDeclarationsSchema) are all z.record(z.string(), z.unknown()) markers, so the intersection collapse cannot let new inputs through. .extend() / .omit() / .pick() / .shape are the right shape for adopter code, and the regression-grep in test/lib/zod-schemas.test.js:46 locks the wire-side ZodObject contract.

Things I checked

  • scripts/generate-zod-from-ts.ts:455-682readBalancedBody, splitTopLevelCommaList, and the two new unwrap passes. Quote/escape handling is correct; bracket depth is preserved across well-formed regex character classes ([a-z0-9], [a-z0-9-]* etc.), which is all ts-to-zod actually emits today.
  • Pass ordering: postProcessRecordIntersections runs before postProcessForPassthrough in the main pipeline. The new passes match z.object( (without .passthrough()) — correct against ts-to-zod's raw output. Passthrough is re-applied later. No doubled passthrough.
  • Over-match guard: unwrapRecordUnionIntersections only fires when every union arm is a redundant record marker AND the .and(...) body starts with z.object(. Real discriminated unions with z.never() / sibling-field constraints survive untouched (Pass 5 still handles those separately).
  • Idempotency: re-running on already-unwrapped output finds no z.union([RecordUnknown…]).and(…) and no-ops. writeFileIfChanged strips the timestamp before comparison, so re-runs don't churn the file.
  • Generated diff (src/lib/types/schemas.generated.ts:631, 660, 731, 765, 7901): four schemas correctly unwrapped to z.object({...}).passthrough(). No other schemas affected.
  • MCP toJSONSchema impact: allOf:[{type:object,addProps:true},{props:…}] collapses to plain {type:object,props:…,addProps:true}. Strict improvement for tool registration.

Follow-ups (non-blocking)

  • Changeset prose understates the type-level effect. Runtime validation is unchanged — true. But z.infer<typeof ProductSchema> narrows from (V1 | V2) & ProductShape to ProductShape. Adopters constructing Product literals with arbitrary extra keys may newly hit excess-property errors, and keyof Product narrows. Add one sentence to .changeset/product-schema-zodobject-ergonomics.md noting the z.infer narrowing, or bump to minor. (Lean toward keeping patch + adding the note — the new shape is the intended one.)
  • Identifier-boundary check in unwrapNamedRecordUnionIntersections (scripts/generate-zod-from-ts.ts:580-595). content.startsWith(\${name}.and(`, i)doesn't verify the byte beforeiisn't an identifier character. Today'sunionSchemaNamesset is effectively{SizeModeMutexSchema}with no near-collisions, so safe. But if a future schema addsOptionalSizeModeMutexSchemaalongsideSizeModeMutexSchema— or any name that's a suffix of another in the same set — the loop silently corrupts output. One-line fix:i === 0 || !/[A-Za-z0-9_$]/.test(content[i-1])` before accepting.
  • Broaden the .d.ts regression-grep. test/lib/zod-schemas.test.js:46 catches ZodIntersection<ZodUnion<readonly [ZodRecord… (the prior shape) but won't fire on object-first intersections like ZodIntersection<ZodObject<…>, ZodUnion<ZodRecord…>>. Either generalize to /z\.ZodIntersection<.*z\.ZodRecord/ or add a positive assertion: ProductSchema._def.typeName === 'ZodObject'.
  • Extend the type test to the three CanonicalFormat*Schema variants named in the changeset (src/type-tests/zod-schema-ergonomics.type-test.ts currently exercises only ProductSchema). Three one-liners lock the contract for the whole set the codegen now restores.
  • LIMITATION: comment near readBalancedBody mirroring the one on postProcessForPassthrough (lines 317-320). No template-literal / regex / // / /* */ handling — fine for current ts-to-zod output, fragile for future output. Pre-existing risk in other passes too; worth documenting in one place.

Approving on the strength of the conservative match conditions (every-arm check + z.object( prefix gate) plus the .d.ts regression-grep that fires file-wide. The named-union identifier-boundary nit is the one I'd land before the next codegen schema is added to that set.

bokelley added a commit that referenced this pull request May 24, 2026
Address two reviewer follow-ups on #1972 (`code-reviewer` and
`javascript-protocol-expert` converged on the same nit independently):

1. Left identifier-boundary check in `unwrapNamedRecordUnionIntersections`.
   `content.startsWith(${name}.and(, i)` previously matched suffixes —
   `SizeModeMutexSchema` collected today, but a future
   `FooSizeModeMutexSchema` would silently corrupt output by emitting
   the prefix character and then the inner schema body. Gate on
   `i === 0 || !/[A-Za-z0-9_$]/.test(content[i-1])`.

2. Loud-fail when `readBalancedBody` returns undefined. Previously
   swallowed silently (advanced by name length, dropping content);
   now throws so a malformed ts-to-zod payload crashes codegen.

Also extends `zod-schema-ergonomics.type-test.ts` with `.extend()` /
`.omit()` / `.pick()` assertions against `CanonicalFormatDisplayTagSchema`,
`CanonicalFormatImageSchema`, and `CanonicalFormatHTML5BannerSchema` —
the three Pass 4 targets the original PR only covered via runtime
`.d.ts` grep.

Regen brings inline-enums and schemas.generated forward.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley bokelley force-pushed the bokelley/productschema-dx-breakage branch from e8a0daf to 9515db4 Compare May 24, 2026 12:42
bokelley added a commit that referenced this pull request May 24, 2026
Address two reviewer follow-ups on #1972 (`code-reviewer` and
`javascript-protocol-expert` converged on the same nit independently):

1. Left identifier-boundary check in `unwrapNamedRecordUnionIntersections`.
   `content.startsWith(${name}.and(, i)` previously matched suffixes —
   `SizeModeMutexSchema` collected today, but a future
   `FooSizeModeMutexSchema` would silently corrupt output by emitting
   the prefix character and then the inner schema body. Gate on
   `i === 0 || !/[A-Za-z0-9_$]/.test(content[i-1])`.

2. Loud-fail when `readBalancedBody` returns undefined. Previously
   swallowed silently (advanced by name length, dropping content);
   now throws so a malformed ts-to-zod payload crashes codegen.

Also extends `zod-schema-ergonomics.type-test.ts` with `.extend()` /
`.omit()` / `.pick()` assertions against `CanonicalFormatDisplayTagSchema`,
`CanonicalFormatImageSchema`, and `CanonicalFormatHTML5BannerSchema` —
the three Pass 4 targets the original PR only covered via runtime
`.d.ts` grep.

Regen brings inline-enums and schemas.generated forward.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley bokelley force-pushed the bokelley/productschema-dx-breakage branch from 9515db4 to 4967d26 Compare May 24, 2026 12:47
aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes May 24, 2026
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Approving on the strength of safe-unwrap plus a self-disarming guard. Witness, not translator invariant holds — the post-processor only collapses union arms that are provably opaque (z.record(z.string(), z.unknown()) with no modifiers), so wire validation is byte-equivalent to the prior intersection. The remaining .passthrough() already accepts the same unknown keys the marker arms would have.

Things I checked

  • All six marker schemas are bare z.record(z.string(), z.unknown()) with no .refine/.transform/.optional — confirmed at src/lib/types/schemas.generated.ts:513-515,624-632. So isRedundantRecordMember correctness holds against current codegen output.
  • Identifier-boundary guard at scripts/generate-zod-from-ts.ts:658 covers the left side; the literal .and( in startsWith covers the right side (a name like FooSizeModeMutexSchema.and( cannot collide with SizeModeMutexSchema.and(). The 9515db4d fix is load-bearing.
  • Fail-closed branches: loud throw on unbalanced .and( at :674, byte-for-byte passthrough at :686 when RHS isn't z.object(...). If a future spec revision thickens a marker arm, collectRedundantRecordUnionSchemaNames stops collecting it and the intersection naturally reappears — self-disarming, which is the right design.
  • Regression guard at test/lib/zod-schemas.test.js:46-53 greps the built .d.ts for z.ZodIntersection<z.ZodUnion<readonly [z.ZodRecord — catches any codegen change that re-introduces the shape. Type-test at src/type-tests/zod-schema-ergonomics.type-test.ts gates .extend/.omit/.pick on the four affected schemas at compile time.
  • AdCP wire conformance unchanged: SizeModeMutex and V1/V2 Product arm unions are TypeScript-side discriminator scaffolds over sibling fields. adcp#4844 explicitly allows both format_ids and format_options simultaneously, so a runtime "exactly one" constraint would actively contradict the spec. The unwrap doesn't weaken anything that was enforced.
  • format_schema URI+digest and extractPublisherFormats/scopePublisherFormats live in src/lib/v2/format-schema/ and src/lib/v2/publisher-catalog/ and don't intersect the marker-arm code path.
  • package.json is already at 8.1.0-beta.8 in main — the version.ts and lockfile diffs are regenerated artifacts, not a manual version edit.

Follow-ups (non-blocking — file as issues)

  • Changeset is probably minor, not patch. Adopters get new capability — .extend/.omit/.pick/.shape on ProductSchema and three CanonicalFormat* schemas threw at compile time on every prior release. The changeset prose hedges with "Restore… again" but no prior release ever exposed these on these schemas. Adopters scanning a patch changelog will not look here for new compose powers. Worth a bump.
  • The inline-enums.generated.ts churn the PR body claims was "removed after review" still shows 264 additions in the diff. Looks like organic regen output for the new CanonicalFormat* family and a few schema renames — not the cosmetic drift you'd want to clean up — but the PR description and reality disagree. Either update the body or pull the remaining diff out.

Minor nits (non-blocking)

  1. Tighten collectRedundantRecordSchemaNames regex. scripts/generate-zod-from-ts.ts:549-550 accepts trailing \s*;?, so a future const Foo = z.record(z.string(), z.unknown()).optional(); would get classified as a redundant record marker and trigger downstream unwrap of Foo.and(z.object(...)). No current trigger — purely latent. Anchor to a ; (or EOL) after the closing ).
  2. isRedundantRecordMember is whitespace-sensitive. :558-560 compares against the literal z.record(z.string(), z.unknown()). If ts-to-zod ever emits z.record( z.string(), z.unknown() ) (with internal spacing), the member silently falls through to "not redundant" and the unwrap quietly stops applying. Fail-closed direction, but worth a normalize step or a one-line comment naming the assumption.
  3. Regression grep is exact-anchored. test/lib/zod-schemas.test.js:46-53 matches the literal z.ZodIntersection<z.ZodUnion<readonly [z.ZodRecord shape. A codegen reformat that line-breaks inside the generic would silently bypass it. Consider adding a positive instanceof z.ZodObject runtime assertion on ProductSchema as a second-line check.

Safe to merge.

@bokelley
Copy link
Copy Markdown
Contributor Author

Issue #1979 consolidates three non-blocking hardening items from review of this PR that overlap its diff. Lightweight enough to fold before merge if convenient — otherwise they'll resurface on #1979 post-merge.

  1. .d.ts regression grep is shape-fragile (test/lib/zod-schemas.test.js:46) — pair the negative grep with a positive _def.typeName === 'ZodObject' assertion so the test fails closed if Zod adjusts tuple stringification.
  2. Pass 3/4 docstrings missing z.object( shape gate (scripts/generate-zod-from-ts.ts:585-592, :636-645) — note the body-prefix constraint in the docstring so a future editor doesn't accidentally broaden the right-hand side.
  3. unionSchemaNames one-level limit in collectRedundantRecordUnionSchemaNames — pre-existing, low urgency; a TODO comment is sufficient if you touch the area.

Posted by the triage bot from #1979.


Generated by Claude Code

bokelley added a commit that referenced this pull request May 24, 2026
Address two reviewer follow-ups on #1972 (`code-reviewer` and
`javascript-protocol-expert` converged on the same nit independently):

1. Left identifier-boundary check in `unwrapNamedRecordUnionIntersections`.
   `content.startsWith(${name}.and(, i)` previously matched suffixes —
   `SizeModeMutexSchema` collected today, but a future
   `FooSizeModeMutexSchema` would silently corrupt output by emitting
   the prefix character and then the inner schema body. Gate on
   `i === 0 || !/[A-Za-z0-9_$]/.test(content[i-1])`.

2. Loud-fail when `readBalancedBody` returns undefined. Previously
   swallowed silently (advanced by name length, dropping content);
   now throws so a malformed ts-to-zod payload crashes codegen.

Also extends `zod-schema-ergonomics.type-test.ts` with `.extend()` /
`.omit()` / `.pick()` assertions against `CanonicalFormatDisplayTagSchema`,
`CanonicalFormatImageSchema`, and `CanonicalFormatHTML5BannerSchema` —
the three Pass 4 targets the original PR only covered via runtime
`.d.ts` grep.

Regen brings inline-enums and schemas.generated forward.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley bokelley force-pushed the bokelley/productschema-dx-breakage branch from 4967d26 to 0c5b181 Compare May 24, 2026 12:51
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restores ZodObject ergonomics on ProductSchema and three CanonicalFormat*Schema exports without changing runtime validation. Right fix — the unwrapped union arms are pure Record<string, unknown> markers and .passthrough() already accepts the same unknown keys. Holding on --approve only because ci:codegen-strict is red on an unrelated drift in the checked-in generated file.

Things I checked

  • SizeModeMutexSchema arms are all z.record(z.string(), z.unknown())schemas.generated.ts:624-630. V1ProductNamedFormatReferenceSchema / V2ProductInlineFormatDeclarationsSchema likewise — :513,:515. Unwrap loses zero runtime validation; the marker arms enforce nothing the surviving .passthrough() object doesn't already accept.
  • All four target schemas regenerate as plain z.object(...).passthrough()CanonicalFormatDisplayTagSchema, CanonicalFormatImageSchema, CanonicalFormatHTML5BannerSchema, ProductSchema. .shape is restored, MCP tool registration unblocked.
  • Identifier-boundary guard at scripts/generate-zod-from-ts.ts:658 closes the FooSizeModeMutexSchema suffix-match risk. Right side is implicitly protected by requiring the literal .and( suffix in startsWith.
  • Loud-throw at :674-677 is correctly shaped — a collected union name followed by .and( with no balanced body is malformedness, not "case we forgot," so panic beats producing corrupt TS.
  • Forward-compat collector is sound: isRedundantRecordMember (:557-560) only accepts the literal z.record(z.string(), z.unknown()) or names resolving to one. The day size_mode becomes a real discriminator (z.literal('fixed') etc.), collectRedundantRecordUnionSchemaNames skips the union and the intersection correctly survives — JSDoc at :641-645 matches behavior.
  • Quoted-string handling in readBalancedBody (:469-485) and splitTopLevelCommaList (:510-526) actually strengthens the LIMITATION at :317-320 — passes 3 and 4 won't be fooled by .and(z.record(...)) substrings inside string literals the way the regex pass at :288 could be.
  • Pass 4 re-runs the collectors against post-pass-3 content (:648-649). Safe because pass 3 only rewrites .and(...) wrappers and emits z.object(...) bodies — it does not introduce or delete named const X = z.union([...]) declarations, so the collected sets are stable.
  • Type-test gate at src/type-tests/zod-schema-ergonomics.type-test.ts plus the .d.ts grep in test/lib/zod-schemas.test.js:44-53 cover the output-side regression.
  • Changeset present (patch) and correctly scoped — restoring documented ergonomics is a patch.

Blocker for merge (not for the code)

  • ci:codegen-strict is failing on src/lib/types/schemas.generated.ts:135 — checked-in has email: z.email().max(254).optional() but regen produces email: z.email().optional(). The PR's generated file is stale relative to the source. Regenerate cleanly against the current ts-to-zod pin and recommit. Not a code-review issue — the codegen logic in this PR is unrelated to that line — but the merge button stays grey until it's green.

Follow-ups (non-blocking — file as issues)

  • Add a fixture-string unit test for unwrapRecordUnionIntersections and unwrapNamedRecordUnionIntersections under scripts/__tests__/. The .d.ts grep catches the output, but if ts-to-zod's formatting drifts (extra whitespace, different argument order), the new passes can silently stop matching and the regression test will pass accidentally because nothing gets emitted as an intersection in the first place. Cases worth covering: marker-only union, mixed union with one real z.object arm (must NOT unwrap), identifier-boundary suffix collision (FooSizeModeMutexSchema), .and( with non-z.object RHS (must preserve byte-for-byte per :684-687).
  • stripNeverUnionIntersections (:704+) still uses inline string scanning. Worth refactoring onto the new readBalancedBody helpers for consistency once this lands.
  • splitTopLevelCommaList and readBalancedBody don't skip regex literals. No bug today — ts-to-zod doesn't emit regexes inside z.union / z.and argument lists — but a latent footgun. One-line guard is cheap insurance.
  • Brief comment at :648-649 explaining why re-collection after pass 3 is sound (collectors only key on named const X = z.union(...) declarations, which pass 3 doesn't touch).

Minor nits

  1. PR description vs diff. The summary says "the initial unrelated inline-enums.generated.ts churn was removed after review," but the diff still carries 264 additions in that file. Those additions are explainable — they're the regen artifacts of the 8.1.0-beta.6 → 8.1.0-beta.8 version bump and they belong in this PR — but the wording reads like they shouldn't be there. Worth tightening the description so a reviewer doesn't burn time auditing them.

Ship it once ci:codegen-strict is green.

@bokelley
Copy link
Copy Markdown
Contributor Author

All three items from #1979 folded in commit fd68ac6:

  1. Runtime _def.typeName assertiontest/lib/zod-schemas.test.js:44 test is now async and adds a _def.typeName === 'ZodObject' check alongside the negative .d.ts grep; fails closed regardless of how Zod serializes the type in declarations.
  2. Docstring body-prefix notes — both unwrapRecordUnionIntersections and unwrapNamedRecordUnionIntersections now document the z.object( constraint explicitly.
  3. collectRedundantRecordUnionSchemaNames TODO — one-level limit noted with a comment; no behavior change.

Pre-push validation passed (format ✓, typecheck ✓, build ✓). Full test suite will run in CI.


Triaged by Claude Code. Session: https://claude.ai/code/session_01768uE6DmJzUwwq4tpb4hqZ


Generated by Claude Code

Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The intersection arms being stripped are vacuously typed Record<string, unknown> markers, so peeling them off restores ZodObject ergonomics with byte-identical runtime semantics.

Things I checked

  • Collector cannot false-positive. collectRedundantRecordUnionSchemaNames (scripts/generate-zod-from-ts.ts:562-583) requires every top-level union arm to be either the literal z.record(z.string(), z.unknown()) or a name that resolves to that exact literal via collectRedundantRecordSchemaNames (:547-554). Single pass — no transitive resolution, so an arm with real fields short-circuits .every(...) and the union stays out of the strip set.
  • Identifier-boundary guard at :657-658 correctly excludes the JS continuation set ([A-Za-z0-9_$]), preventing a future FooSizeModeMutexSchema from shadowing SizeModeMutexSchema. Right boundary is implicit via the literal .and( suffix.
  • Loud-fail at :671-677 throws on a collected name followed by an unbalanced .and(. ts-to-zod produces balanced output by construction; this is fail-closed for the day codegen drifts under us.
  • Pass ordering is correct. Pass 3 (inline unions) leaves SizeModeMutexSchema's own declaration intact because the union is followed by ; rather than .and( — falls through the result += content.substring(unionStart, unionEnd) branch at :626-627. Pass 4 re-scans the file and finds the named declaration.
  • Runtime equivalence. z.union([record(string, unknown), ...]).and(z.object({...}).passthrough()) accepts/produces the same set as z.object({...}).passthrough() alone — the record side is a strict superset of plain objects, and .passthrough() already preserves unknown keys.
  • Drift-safety. If a future spec turns FixedSchema into a real z.object(...), it drops out of recordSchemaNames, SizeModeMutexSchema drops out of unionSchemaNames, and the consuming schema correctly stays a ZodIntersection. The .d.ts grep test at test/lib/zod-schemas.test.js:44-53 plus the type-tests would surface a regression as a loud failure.
  • Changeset present and right level. patch is defensible — the prior ZodIntersection<ZodUnion<...Record<string,unknown>...>, ZodObject<...>> was never a usable type (.extend/.omit/.pick/.shape all failed; instanceof z.ZodIntersection narrowing on vacuous arms validates nothing). No manual package.json version edit.
  • inline-enums.generated.ts churn (+264 lines) is regen drift from spec additions (BillingNotSupportedDetails, CanonicalFormat*, CreativePurgedWebhook, WebhookActivityRecord, WholesaleFeedWebhook, etc.) — all cross-check against the upstream types in core.generated.ts/tools.generated.ts. The PR-description note that "unrelated churn was removed" referred to manual edits; the residual is the codegen consequence of regenerating after the schema cache moved forward.

Follow-ups (non-blocking — file as issues)

  • Tighten the codegen comment at scripts/generate-zod-from-ts.ts:277. It says ZodIntersection loses .shape access "needed by MCP SDK for tool registration." True in general for *RequestSchema exports surfaced via TOOL_REQUEST_SCHEMAS (src/lib/utils/tool-request-schemas.ts:27-50), but none of the four schemas this PR fixes (ProductSchema, CanonicalFormatDisplayTagSchema, CanonicalFormatHTML5BannerSchema, CanonicalFormatImageSchema) are MCP tool inputs. The real bug is adopter composition. Add "and by adopters composing derived payload schemas" so the comment matches reality.
  • Expand the changeset prose. Name the four affected schemas explicitly and add an "no action required unless you were annotating these as ZodIntersection<...> or narrowing with instanceof z.ZodIntersection" line, so 7.10 → 8.1-beta consumers don't have to grep to learn whether they're affected.
  • Dead exports. After Pass 4 strips SizeModeMutexSchema.and(...) and z.union([V1Product..., V2Product...]).and(...), the named marker schemas (V1ProductNamedFormatReferenceSchema, V2ProductInlineFormatDeclarationsSchema, SizeModeMutexSchema, FixedSchema/MultiSizeSchema/ResponsiveSchema/NoneSchema) appear unreferenced at runtime. Harmless — kept for backward-compat of consumer imports — but a future cleanup pass could drop them once no external import sites remain.

Approving on the strength of the runtime-equivalence proof, the fail-safe drift behavior, and the durable .d.ts regression grep.

aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes May 24, 2026
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Real bug, narrow fix, defense-in-depth on the parser. ProductSchema back to ZodObject restores .extend() / .omit() / .pick() / .shape — exactly what adopters and the MCP SDK tool-registration path read.

Things I checked

  • Runtime equivalence — SizeModeMutexSchema is four arms of z.record(z.string(), z.unknown()), the inline Product union is two V* markers of the same shape. Intersecting any of them with z.object({...}).passthrough() contributes zero new validation; the unwrap is a syntactic refactor, not a semantic relaxation. .passthrough() rides along on the RHS because andBody.body is the verbatim inner text of .and(...).
  • Guard symmetry — andBody.body.trimStart().startsWith('z.object(') at scripts/generate-zod-from-ts.ts:683 blocks unwrapping past any chained-but-not-z.object RHS, and falls through to the byte-for-byte preserve at L691. Non-object RHS survives intact.
  • Identifier-boundary guard at L663 — FooSizeModeMutexSchema.and( won't match SizeModeMutexSchema. The .and( suffix requirement at L661 forms the implicit right boundary. Good.
  • Loud-fail at L679 — throw rather than silent corruption when a collected union name is followed by .and( with no balanced body. Right call; the alternative was data-shaped lies in generated output.
  • One-level-only TODO at L577 — collectRedundantRecordUnionSchemaNames doesn't recurse into nested z.union(...) arms. Today's schemas.generated.ts has no such shape, so it's a forward-defensive note, not a present gap.
  • Regression coverage — test/lib/zod-schemas.test.js:51 asserts _def.typeName === 'ZodObject', plus a .d.ts grep for z.ZodIntersection<z.ZodUnion<readonly [z.ZodRecord that fails closed if codegen drifts. The runtime .extend() / .omit() / .pick() calls in the same test would also throw if the schema flipped back to intersection — belt and suspenders. The new src/type-tests/zod-schema-ergonomics.type-test.ts extends the gate to tsc --noEmit time.
  • Wire impact — schemas.generated.ts flips four schemas (ProductSchema, CanonicalFormatDisplayTagSchema, CanonicalFormatImageSchema, CanonicalFormatHTML5BannerSchema) from intersection to plain z.object({...}).passthrough(). Patch changeset is the right shape — restoring documented ergonomics, no validation behavior change.
  • package-lock.json 7.10.1 → 8.1.0-beta.8 and src/lib/version.ts beta.6 → beta.8 are rebase churn against main (already at beta.8). src/lib/version.ts is auto-generated by sync-version.ts — not a manual edit.
  • inline-enums.generated.ts churn is regeneration drift from rebase, not authored content — the PR body's claim that it "was removed after review" is stale, but the file is generated, so it's noise rather than risk.

Follow-ups (non-blocking — file as issues)

  • Land the TODO at scripts/generate-zod-from-ts.ts:577. If a future spec emits OuterMutexSchema = z.union([SizeModeMutexSchema, OtherMutexSchema]), Pass 4 silently misses it and the intersection re-appears in ProductSchema-shaped schemas. The runtime + .d.ts regression tests would catch it, but the codegen should fold rather than rely on the tests as the safety net.
  • Consider widening the .d.ts regex in test/lib/zod-schemas.test.js:57 to a less Zod-internal-formatting-coupled assertion — the readonly [...] substring is byte-tight against ts-to-zod's current emit. The accompanying runtime _def.typeName check covers the real fail-closed concern, so this is belt-tightening, not a gap.

Safe to merge.

bokelley added a commit that referenced this pull request May 24, 2026
Address two reviewer follow-ups on #1972 (`code-reviewer` and
`javascript-protocol-expert` converged on the same nit independently):

1. Left identifier-boundary check in `unwrapNamedRecordUnionIntersections`.
   `content.startsWith(${name}.and(, i)` previously matched suffixes —
   `SizeModeMutexSchema` collected today, but a future
   `FooSizeModeMutexSchema` would silently corrupt output by emitting
   the prefix character and then the inner schema body. Gate on
   `i === 0 || !/[A-Za-z0-9_$]/.test(content[i-1])`.

2. Loud-fail when `readBalancedBody` returns undefined. Previously
   swallowed silently (advanced by name length, dropping content);
   now throws so a malformed ts-to-zod payload crashes codegen.

Also extends `zod-schema-ergonomics.type-test.ts` with `.extend()` /
`.omit()` / `.pick()` assertions against `CanonicalFormatDisplayTagSchema`,
`CanonicalFormatImageSchema`, and `CanonicalFormatHTML5BannerSchema` —
the three Pass 4 targets the original PR only covered via runtime
`.d.ts` grep.

Regen brings inline-enums and schemas.generated forward.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley bokelley force-pushed the bokelley/productschema-dx-breakage branch from 8cac2ad to 832bb38 Compare May 24, 2026 13:40
aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes May 24, 2026
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Root-caused at codegen instead of bolting on a parallel ProductObjectSchema — the right shape. Adopters following docs/ZOD-SCHEMAS.md:107 compile again with zero migration.

Things I checked

  • Codegen fail-closed contract. Pass 3 (scripts/generate-zod-from-ts.ts:619-627) and Pass 4 (:683-692) both unwrap only when the .and(...) RHS starts with z.object(. Non-object RHS preserved byte-for-byte. If a future spec adds real fields to a marker arm, collectRedundantRecordUnionSchemaNames stops collecting and the schema correctly remains a ZodIntersection. Witness, not translator.
  • Identifier-boundary guard. :663 correctly rejects FooSizeModeMutexSchema when only SizeModeMutexSchema is in the collected set. The loud-fail throw at :679 is the right call — readBalancedBody only returns undefined on truncated ts-to-zod output; crashing beats silent corruption.
  • Wire/runtime equivalence. z.union([z.record(z.string(), z.unknown())...]).and(z.object({...}).passthrough()) and z.object({...}).passthrough() accept the same key shapes and validate the same known fields. The marker arms contributed only unknown keys that .passthrough() already handles. z.infer<typeof ProductSchema> is unchanged.
  • Output diff. Exactly 4 schemas flipped in src/lib/types/schemas.generated.ts: ProductSchema (V1/V2 marker arms), CanonicalFormatDisplayTagSchema / CanonicalFormatImageSchema / CanonicalFormatHTML5BannerSchema (SizeModeMutex marker arms). Consistent with the marker-only-union era.
  • inline-enums diff is a correct downstream consequence, not drift. scripts/generate-inline-enum-arrays.ts:207 early-returns for def.type !== 'object', so pre-fix the three Canonical formats' shapes were never enumerated. Post-fix they emit. The PR body wording is fine.
  • Test gates. Type-test fails at tsc --noEmit, runtime constructor-name probe fails at node:test. constructor.name === 'ZodObject' is the Zod-version-stable probe (Zod 4 dropped _def.typeName — that's load-bearing).
  • Changeset present, patch matches behavior. Runtime identical, type-level signature changes restore docs-correctness.

Follow-ups (non-blocking — file as issues)

  • Tighten the record-marker regex. scripts/generate-zod-from-ts.ts:549-555 accepts trailing .optional() because ;? permits zero chars. No current generated file triggers this, but add a (?![.\w]) lookahead so const Foo = z.record(z.string(), z.unknown()).optional(); can never be collected as a bare marker.
  • Lift constructor.name === 'ZodObject' to the three Canonical formats, not just ProductSchema. test/lib/zod-schemas.test.js:52 is the Zod-version-stable gate; the .d.ts regex at :58 is best-effort. Belt-and-suspenders on the runtime side is cheap.
  • Changeset prose could lead with the symptom. Adopters who hit Property 'extend' does not exist on type 'ZodIntersection<...>' won't grep for "marker arms." Worth a follow-up rewrite at release time that names .extend() / .omit() / .pick(), links #1970, and notes the ZodIntersection<...> → ZodObject<...> type-signature change. Trivially, this and .changeset/object-schema-intersections.md could consolidate in the rendered note.
  • Type-test enumeration. src/type-tests/zod-schema-ergonomics.type-test.ts covers the 4 schemas affected today. Worth a top-of-file comment pointing future maintainers at Pass 4 as the authoritative list — if codegen ever produces additional marker-only-union schemas, the type-test should grow to match.

Minor nits (non-blocking)

  1. One-level-only TODO at scripts/generate-zod-from-ts.ts:577 is correctly documented as a scope limit. Watch item if a future spec layers marker unions (union([marker_union, RecordUnknown])). The .d.ts regex would catch the resulting ZodIntersection<ZodUnion<...ZodRecord shape today.
  2. src/lib/version.ts:69 generatedAt moves backward five minutes due to regen ordering. Cosmetic; not worth a separate commit.

Approving on the strength of the fail-closed contract plus the dual compile-time + runtime gates.

bokelley and others added 5 commits May 24, 2026 10:49
Address two reviewer follow-ups on #1972 (`code-reviewer` and
`javascript-protocol-expert` converged on the same nit independently):

1. Left identifier-boundary check in `unwrapNamedRecordUnionIntersections`.
   `content.startsWith(${name}.and(, i)` previously matched suffixes —
   `SizeModeMutexSchema` collected today, but a future
   `FooSizeModeMutexSchema` would silently corrupt output by emitting
   the prefix character and then the inner schema body. Gate on
   `i === 0 || !/[A-Za-z0-9_$]/.test(content[i-1])`.

2. Loud-fail when `readBalancedBody` returns undefined. Previously
   swallowed silently (advanced by name length, dropping content);
   now throws so a malformed ts-to-zod payload crashes codegen.

Also extends `zod-schema-ergonomics.type-test.ts` with `.extend()` /
`.omit()` / `.pick()` assertions against `CanonicalFormatDisplayTagSchema`,
`CanonicalFormatImageSchema`, and `CanonicalFormatHTML5BannerSchema` —
the three Pass 4 targets the original PR only covered via runtime
`.d.ts` grep.

Regen brings inline-enums and schemas.generated forward.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
claude and others added 4 commits May 24, 2026 10:49
- Pair negative .d.ts intersection grep with runtime _def.typeName===ZodObject
  assertion in zod-schemas.test.js so the test fails closed if Zod adjusts
  its tuple stringification.
- Note the z.object( body-prefix constraint in both unwrap docstrings so a
  future editor doesn't accidentally broaden the right-hand side.
- Add TODO comment on the one-level limit in collectRedundantRecordUnionSchemaNames.

Refs #1979

https://claude.ai/code/session_01768uE6DmJzUwwq4tpb4hqZ
Triage commit fd68ac6 added a `ProductSchema._def.typeName === 'ZodObject'`
assertion based on the reviewer's follow-up suggestion. The reviewer's
suggested probe was written against Zod 3 — Zod 4 dropped `_def.typeName`
in favor of `_def.type` ('object' lowercase) and the codebase imports
Zod 4. The assertion was failing because `_def.typeName` is `undefined`.

Use `constructor.name === 'ZodObject'` instead — stable across both Zod
major versions and the same closed-fail behavior the reviewer asked for.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley bokelley force-pushed the bokelley/productschema-dx-breakage branch from 832bb38 to be3f7f6 Compare May 24, 2026 14:54
@bokelley bokelley enabled auto-merge (squash) May 24, 2026 14:54
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codegen-only fix. Right shape: only unwraps when the RHS of .and( begins with z.object(, preserves byte-for-byte otherwise, and throws on unbalanced parens rather than silently corrupting output.

Things I checked

  • Parser correctness for current ts-to-zod output: quoted-string and balanced-bracket handling in readBalancedBody (scripts/generate-zod-from-ts.ts:464) and splitTopLevelCommaList (:509) is sufficient given the constrained input domain. Template/regex literals aren't emitted inside marker-only union arms today.
  • Generated output: ProductSchema = z.object( at src/lib/types/schemas.generated.ts:7901; CanonicalFormatDisplayTagSchema = z.object( at :634; SizeModeMutexSchema at :632 stays as a named z.union([...]) because its arms are real named object schemas, not redundant Record<string, unknown> markers — Pass 4 correctly leaves it alone.
  • Left-identifier boundary at :669 is sufficient to prevent FooSizeModeMutexSchema matching as SizeModeMutexSchema; the literal .and( suffix provides the right boundary.
  • Pass ordering: Pass 3 (inline) cannot create input that Pass 4 (named) misreads. Pass 4 re-collecting recordSchemaNames is wasteful, not incorrect.
  • Patch changeset is the right tier. No runtime-behavior change; the emitted Zod type narrows from ZodIntersection<ZodUnion<...>, ZodObject> to ZodObject, and z.infer<typeof ProductSchema> is identical across both forms. Practical break surface is zero — this restores ergonomics adopters were already using.
  • Runtime test schemas.ProductSchema.constructor.name === 'ZodObject' is the right cross-Zod-version probe (Zod 4 dropped _def.typeName); the .d.ts grep at test/lib/zod-schemas.test.js:60 catches future regressions in emitted types.
  • The generatedAt regression in src/lib/version.ts:69 (12:44:56Z12:39:59Z) is benign — scripts/sync-version.ts:243 writes new Date().toISOString() from the rebase-machine clock, not from source state.

Follow-ups (non-blocking — file as issues)

  1. Regex-literal handling in readBalancedBody. ts-to-zod doesn't currently emit z.string().regex(/.../) inside intersection arms, but the existing skipQuotedOrRegexLiteral helper (:781) handles regex — the new scanner at :464 doesn't. Two scanners, two behaviours. The moment a spec adds a regex'd field inside a marker-only union arm, depth tracking goes out of sync. Unify on the existing helper.
  2. Inverse test. The grep at test/lib/zod-schemas.test.js:60 only asserts the unwrap fired. There's no test that a non-redundant intersection (one arm carries a real typed field) survives as ZodIntersection. Add a fixture-driven test against generate-zod-from-ts.ts directly — would catch a Pass 3/4 broadening that silently over-unwraps.
  3. Orphan-declaration sweep. No collected unionSchemaNames declaration becomes unused today because the only collected unions in this corpus are inline. The moment a named union is composed exclusively of redundant Record markers, Pass 4 rewrites every call site and leaves the declaration as a dead export. Either drop export for collected names, or sweep zero-reference declarations after Pass 4.
  4. Asymmetric fail-closed posture. Pass 4 throws at :685 on unbalanced .and(; Pass 3 silently falls through at :614. The silent path is safe (writes one char, advances) but the inconsistency is the kind of thing that gets cargo-culted on the next pass. Pick one.

Minor nits (non-blocking)

  1. EOS escape edge. scripts/generate-zod-from-ts.ts:480 reads the backslash, then unconditionally appends content[i] even if i >= content.length, which appends the string "undefined" to body. Cheap fix: if (i >= content.length) break; after the backslash branch. Cannot fire on current ts-to-zod output but the guard is one line.
  2. The TODO at :583. one-level only — arms that are themselves z.union(...) are not collected is the kind of TODO that should fail loudly the moment ts-to-zod's output drifts. Consider throwing if a nested union arm is encountered so the next codegen change surfaces here rather than producing a quietly-not-unwrapped schema.

Worth a note: this is the third pass-based string rewriter in scripts/generate-zod-from-ts.ts. The fourth one tips into ts-morph-is-cheaper-than-debugging-this territory — file a tracking issue before someone adds Pass 6.

LGTM. Follow-ups noted above — file #1 before this becomes a debugging session in six months. Patch changeset is right; the SDK's wire behaviour is unchanged.

@bokelley bokelley merged commit eb0d260 into main May 24, 2026
11 checks passed
@bokelley bokelley deleted the bokelley/productschema-dx-breakage branch May 24, 2026 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DX: ProductSchema shape change in 8.1 silently breaks .extend()/.omit()/.pick() consumers

2 participants