Skip to content

fix: preserve schema helper types#1981

Merged
bokelley merged 1 commit into
mainfrom
bokelley/schema-dx-audit
May 24, 2026
Merged

fix: preserve schema helper types#1981
bokelley merged 1 commit into
mainfrom
bokelley/schema-dx-audit

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Summary

  • collapse marker-only union/object generated schemas back to ZodObject so ProductSchema and canonical format schemas keep .shape/.extend()/.omit()/.pick() helper access
  • preserve exact known-key request/input schema maps while keeping dynamic string lookup support
  • keep TS7056-bounded object schemas typed enough for customToolFor handler inference, including preview_creative/update_media_buy fields and optional account shapes

Validation

  • npm run typecheck
  • npm run build:lib
  • NODE_ENV=test node --test-timeout=60000 --test-force-exit --test test/generate-zod-object-intersections.test.js test/lib/zod-schemas.test.js
  • npm run check:adopter-types
  • rg marker-union ZodIntersection signature count: 0
  • expert re-review: code-reviewer no blockers; JavaScript/DX no blockers after follow-up fixes

Follow-ups

  • separate DX audit items remain for brand-rights shape coverage, well-known anyOf/oneOf weakening, and mutating-tool exactness guards.

Comment thread src/type-tests/zod-object-intersections.type-test.ts Fixed
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. Follow-ups noted below. The collapse pass fires at the right layer — generator post-processing, not adopter code — and only when union arms are provably opaque (z.record(z.string(), z.unknown()) recursively, including named refs like SizeModeMutexSchema). Runtime validation set is unchanged because union([opaqueRecord, ...]).and(realObject) ≡ realObject; the win is purely type-side helper restoration.

Things I checked

  • Runtime equivalence on all four collapsed schemas. V1ProductNamedFormatReferenceSchema, V2ProductInlineFormatDeclarationsSchema, FixedSchema, MultiSizeSchema, ResponsiveSchema, NoneSchema, and the SizeModeMutexSchema they feed all resolve to z.record(z.string(), z.unknown()) in the current generated file. Dropping them from the runtime expression is a true no-op on validation. The "stops firing when markers gain real fields" test at test/generate-zod-object-intersections.test.js:101-110 is the load-bearing guard — once V1/V2 grow real fields, the collapse stops and maintainers have to think.
  • Cycle / depth correctness in the new pass. isOpaqueRecordMarkerExpression and isOpaqueMarkerUnion each carry a visiting set (scripts/generate-zod-from-ts.ts:185, 211); splitTopLevelList tracks ()/{}/[] depth and skips quoted/regex literals; rewriteLeadingMarkerUnionObjectAnd only rewrites when the tail is exactly .and(<object-shape>) at depth 0, so discriminated-union response intersections like CreateMediaBuyResponseSchema are preserved.
  • Pass ordering. postProcessForPassthroughpostProcessMarkerUnionObjectIntersectionspostProcessObjectIntersections → TS7056 annotations (scripts/generate-zod-from-ts.ts:1199-1276). Marker collapse runs before object-intersection merge so the latter can pick up the now-flat object form.
  • Typed dictionary coverage. KnownToolRequestSchemas (src/lib/utils/tool-request-schemas.ts:33-89) enumerates every key currently in TOOL_REQUEST_SCHEMAS. The fallback [toolName: string]: z.ZodObject<any> | undefined makes the dynamic cast at src/lib/server/create-adcp-server.ts:3455 legal without as unknown as. WithOptionalAccountShape<TShape> correctly preserves per-field types after the account required→optional flip.
  • TS7056 annotation shape. Switching PreviewCreativeRequestSchema and UpdateMediaBuyRequestSchema to objectShape: true emits the dual z.ZodObject<{ [K in keyof T]-?: z.ZodType<T[K], T[K]> }, any> & z.ZodType<T & Record<string, unknown>, ...> annotation (schemas.generated.ts:6073, 9003) — .shape access plus Record<string, unknown> widening, both preserved.
  • Changeset adequacy for wire impact. patch is right for the runtime-visible change. Type narrowing on the exported maps is strictly additive against Partial<Record<string, z.ZodType>> — old read-side uses still type-check because ZodObject extends ZodType.

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

  • Changeset prose understates the typed-map narrowing. .changeset/product-schema-object-helpers.md only mentions ProductSchema and canonical formats. Adopters who explicitly assigned the map to Record<string, z.ZodObject<any>> (no Partial, no | undefined) will see a TS error from the new readonly index signature. One-line fix on their side, but worth a sentence in the changelog so they're not surprised.
  • JSDoc on KnownToolRequestSchemas and on the narrowing behavior of TOOL_INPUT_SHAPES[toolName]. Adopters will currently discover this by IDE autocomplete. One sentence on each closes the loop.
  • Don't-replicate hint on the TS7056 annotation. The verbose dual annotation in schemas.generated.ts:6073, 9003 is what an LLM sees in a hover and may try to mimic when authoring a new schema. The comment at scripts/generate-zod-from-ts.ts:102-112 explains why it exists; a single sentence pointing at TS7056_SCHEMAS as the opt-in mechanism would head off the cargo-culting.

Minor nits (non-blocking)

  1. Redundant update_rights line. src/lib/schemas/index.ts:51update_rights: typeof schemas.UpdateRightsRequestSchema.shape is already provided by the mapped half at line 48 (update_rights is a KnownToolRequestSchemas key). Harmless intersection of the same type with itself. Only creative_approval genuinely needs the additional intersection. Drop the line.
  2. Type-test could @ts-expect-error past void. src/type-tests/zod-object-intersections.type-test.ts:21-29productPick, productOmit, productExtend are void-referenced. Asserting on a field-access of those (productPick.shape.product_id) would more directly prove the helpers return ZodObjects rather than ZodIntersections. Non-blocking — the existing assertions already fail at compile time if the helpers disappear.

Notable choice: pinning the collapse to opaque-record arms specifically (rather than any union arm with no real schema) means the pass intentionally won't fire on the day V1/V2 grow real validators. That's the right shape — fail-loud at the seam, don't translate.

Approving.

@bokelley bokelley enabled auto-merge (squash) May 24, 2026 14:30
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. Follow-ups noted below. Restoring .shape/.extend/.omit/.pick on ProductSchema and the canonical-format schemas without changing what the SDK accepts on the wire is the right shape — both arms of the collapsed unions resolve to z.record(z.string(), z.unknown()), so union ∩ object.passthrough() and object.passthrough() accept the identical value set. Witness, not translator, intact.

Things I checked

  • Wire-shape invariant. V1ProductNamedFormatReferenceSchema, V2ProductInlineFormatDeclarationsSchema, and all four SizeModeMutex arms (Fixed / MultiSize / Responsive / None) are literal z.record(z.string(), z.unknown()) in src/lib/types/schemas.generated.ts. They carry no fields and impose no validation beyond "is an object" — which .passthrough() on the object half already enforces. safeParse returns identical results before and after.
  • Spec conformance. The V1/V2 product split is an at-least-one-of presence constraint on format_ids/format_options (both optional on the combined Product, coexisting via dual emission per the 7.9→7.10 notes), not a discriminated union. Same for size-mode mutex — spec prose at src/lib/types/core.generated.ts:3665, not a structural enforcer. SDK was never enforcing either discriminator; this PR doesn't change that.
  • Safety net. isOpaqueMarkerUnion (scripts/generate-zod-from-ts.ts:876-901) requires arms.every(...) to be opaque records — unanimity, not majority. If a future spec rev materializes a real field on V2 product, the arm stops being opaque, .every fails, the intersection is preserved verbatim. Fail-loud on regression.
  • Ordering. postProcessMarkerUnionObjectIntersections runs before postProcessObjectIntersections (scripts/generate-zod-from-ts.ts:1209-1216). Marker pass strips the .union(...).and(...) wrapper, leaving a bare object; the object-intersection pass then sees no top-level .and() for these schemas. No undo.
  • MCP runtime contract. The new TS7056 annotation z.ZodObject<{[K in keyof T]-?: z.ZodType<T[K], T[K]>}, any> & z.ZodType<T & Record<string, unknown>, ...> is type-only — RHS is still a plain z.object({...}).passthrough() call. TOOL_REQUEST_SCHEMAS.preview_creative.shape returns the real ZodRawShape that server.registerTool expects. No as any smuggling.
  • Known vs. fallback resolution. Readonly<KnownToolRequestSchemas> & { readonly [toolName: string]: z.ZodObject<any> | undefined } resolves known keys to exact types before the index fallback. The @ts-expect-error assertions at src/type-tests/zod-object-intersections.type-test.ts:47, 54, 58, 105 lock this.
  • Changeset. patch is correct — no runtime behavior change, no public API removed, helper access restored.

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

  • KnownToolRequestSchemas drift is silent. The intersection at src/lib/utils/tool-request-schemas.ts:99-101 accepts any new runtime entry via the string index. A tool added to TOOL_REQUEST_SCHEMAS but missed in KnownToolRequestSchemas passes tsc and silently widens to z.ZodObject<any> | undefined at the call site — adopters lose .shape.<field> narrowing with no error. Add a satisfies check or an exhaustive keyof assertion (e.g., a never-typed _exhaustive: keyof KnownToolRequestSchemas extends keyof typeof TOOL_REQUEST_SCHEMAS ? ... : never) so drift fails the build. The ci:schema-check reference at src/lib/schemas/index.ts:74 covers map presence, not type-map membership.
  • Opaque-marker detection is a literal string compare. scripts/generate-zod-from-ts.ts:859 matches exactly z.record(z.string(), z.unknown()). One ts-to-zod output tweak (whitespace, member order) silently disables the collapse, helpers regress, and nothing fails — .parse() still works. Worth a post-generation invariant: assert ProductSchema._def.typeName === 'ZodObject' (or typeof ProductSchema.shape === 'object') at build time so a regression fires loudly.
  • Pre-existing size-mode oneOf gap. The SDK has never enforced the Fixed/MultiSize/Responsive/None mutex on canonical formats; that lives in spec prose. This PR makes the gap more visible (no longer hidden behind a meaningless .and()) but doesn't widen it. If the working group wants SDK-side enforcement, that's separate codegen work — materialize the size-mode discriminator into real z.object arms with required-field disjointness.

Minor nits (non-blocking)

  1. ZodObject<..., any> policy slot. scripts/generate-zod-from-ts.ts:118 — the any second-type-param is the unknown-keys-policy slot. The schema is .passthrough(), but 'passthrough' isn't part of Zod's public type surface, so any is the only sound value. One-line comment on this would save the next reader a Zod source spelunk.
  2. splitTopLevelList bias. scripts/generate-zod-from-ts.ts:822-833 — returning undefined on an empty part means "trailing comma → not a marker union → leave intact." Conservative-correct but worth a one-line comment so a future maintainer doesn't try to be helpful.
  3. Fallback shape assertion in type-test. src/type-tests/zod-object-intersections.type-test.ts:95-99 exercises the string-index fallback but doesn't assert the inferred type. A const _expected: z.ZodObject<any> | undefined = runtimeRequestSchema; would lock the fallback shape so accidental widening of known keys to the fallback breaks the test.

Safe to merge.

@bokelley bokelley merged commit 94ba961 into main May 24, 2026
11 checks passed
@bokelley bokelley deleted the bokelley/schema-dx-audit branch May 24, 2026 14:47
bokelley added a commit that referenced this pull request May 24, 2026
* fix: preserve ProductSchema ZodObject ergonomics

* chore: sync package lock versions

* fix: unwrap named record union schema markers

* chore: add ProductSchema DX changeset

* fix: identifier-boundary guard + loud-fail in named-union unwrap

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>

* test(codegen): fold #1979 hardening items before merge

- 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

* fix(test): use ZodObject constructor name, not Zod 3 _def.typeName

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>

* chore: regen schemas after rebase onto main with #1975

* chore: regen schemas after rebase onto main with #1981

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

1 participant