Skip to content

fix(codegen): pre-merge allOf[$ref] siblings to unbreak jsts union emission#1783

Merged
bokelley merged 2 commits into
mainfrom
worktree-agent-a7afa670de957eaea
May 16, 2026
Merged

fix(codegen): pre-merge allOf[$ref] siblings to unbreak jsts union emission#1783
bokelley merged 2 commits into
mainfrom
worktree-agent-a7afa670de957eaea

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Closes #1756. Unblocks adcp#4510 (the spec-side schema dedup spike that reverted on this codegen bug).

Summary

JSON Schema objects that mix sibling properties/required with allOf: [{ $ref }] make json-schema-to-typescript emit a broken union ( Base | { variant + duplicated base fields } ) where the intent is Base & { variant }. This is most visible inside oneOf discriminator variants — GetContentStandardsResponse collapsed its success arm to bare ContentStandards, silently dropping the variant's own context and ext fields.

enforceStrictSchema now pre-merges any allOf member that is a single $ref into the parent shape when the parent already declares its own properties/required — variant-level fields win on collision; the base's additionalProperties is inherited only when the variant didn't override it. Local #/$defs/... fragments are left in place (existing Individual*Asset post-processor handles them). vendor-pricing-option-style allOf-only roots stay untouched (jsts handles those correctly today).

This is paired with the sibling PR for #1745 (JSDoc constraint injection) — independent edits to different parts of scripts/generate-types.ts, mergeable in either order.

Knock-on type changes (intentional)

  • BriefAsset / CatalogAsset flatten from Base & { ... } to merged interfaces. Field set is identical; the named base type is no longer referenced via intersection. This matches option 2 in the issue's acceptance criteria ("flattens the allOf into a single merged shape — less ideal but acceptable"). Recovering the Base & { variant } intersection form would be a follow-up polish pass and is out of scope here.
  • GetContentStandardsResponse success arm regains its context? / ext? fields — the bug this PR fixes.
  • CreativeBrief no longer transitively reaches tools.generated.ts; src/lib/index.ts re-exports it from core.generated instead. Existing import sites are unaffected.

Test plan

  • Unit tests for enforceStrictSchema cover three scenarios — vendor-pricing-option-style pass-through, single-variant merge, and a two-variant oneOf ending in a clean discriminated union (test/type-generator-allof-ref-merge.test.js).
  • npm run generate-types regenerates cleanly; diff confined to the four type changes listed above plus their downstream Zod schemas.
  • npm test passes (1 known-flaky perf-test unrelated to this change: request-signing-replay-perf.test.js's has() sub-linear assertion — passes in isolation).
  • npm run format:check passes.
  • Pre-push validation (tsc --noEmit + build:lib) passes.

🤖 Generated with Claude Code

@bokelley

Copy link
Copy Markdown
Contributor Author

Coordinator review (3-lens sweep: code / protocol / testing). One real concern worth surfacing; the rest are nits or follow-ups. Not blocking — flagging for your call before merge.

Real concern: additionalProperties: true leaks through the merge

resolveAllOfRefForMerge returns the raw resolved base (only removeArrayLengthConstraints applied — not enforceStrictSchema). The merge then propagates resolved.additionalProperties directly:

if (strictSchema.additionalProperties === undefined && resolved.additionalProperties !== undefined) {
  strictSchema.additionalProperties = resolved.additionalProperties;
}

creative-brief.json and catalog.json both declare top-level additionalProperties: true. Pre-PR, enforceStrictSchema stripped that at line 253 when emitting the standalone CreativeBrief interface, so CreativeBrief & { asset_type: 'brief' } had no index signature. Post-PR, the merged BriefAsset gains [k: string]: unknown | undefined (core.generated.ts:2158, also at line 2233 for CatalogAsset).

Net: the new merged shape is wider than both the old intersection form and the current standalone CreativeBrief interface. Adopters who relied on the closed shape lose that guarantee.

Two ways to tighten if you want strictness parity:

  • Run enforceStrictSchema(resolved) inside resolveAllOfRefForMerge before returning — the top-level additionalProperties: true strip then fires before the merge sees it.
  • After the merge, re-check: if (strictSchema.additionalProperties === true && !mustPreserveProperties) delete strictSchema.additionalProperties;

Both fix it. Calling enforceStrictSchema on the resolved base also handles nested base allOf chains for free (currently silently dropped — see follow-up #2 below).

If you prefer to ship as-is given the "merged shape is acceptable / option 2" framing in the issue, fine — but worth a code comment noting the index-signature widening is a deliberate side effect of the merge so the next codegen maintainer doesn't get surprised.

Follow-ups (file separately, not blocking)

  1. Test coverage gap: no test pins the additionalProperties propagation behavior either way. A subtest "base with additionalProperties: true does/doesn't leak [k: string] into merged shape" would lock the contract regardless of which direction you go above.
  2. Resolved base's own allOf is silently dropped: merge only inherits properties / required / additionalProperties from resolved. If a future base schema chains its own allOf: [{ $ref }, ...], those intersections vanish. No live targets today (creative-brief and catalog have no root-level allOf). Cheap fix is the same as above — recurse through enforceStrictSchema on resolved before merging.
  3. Collision precedence is documented but not tested: variant-level field winning on collision is asserted in the code comment but not pinned by a test. A one-line subtest (base has name: string, variant has name: { const: 'brief' }, assert variant wins) would prevent regression.
  4. runHarness temp-dir leak on npx tsx crash: .allof-ref-merge-harness-* directories under REPO_ROOT are cleaned in finally, but spawnSync failures throw before — actually wait, no, finally runs after throw. Disregard.

Confidence

CI is effectively green (Test & Build at "Run unit tests ✓", finishing post-test steps). Two-lens checks I ran:

  • BriefAsset / CatalogAsset field set is identical to pre-PR. Verified by diffing the property keys against the old CreativeBrief & { asset_type } form.
  • GetContentStandardsResponse success arm correctly surfaces context? and ext? at tools.generated.ts:10970-10971. That's the bug fix.
  • CreativeBrief is still emitted in core.generated.ts:8487 and re-exported through src/lib/index.ts:546. No consumption-path breakage for adopters who import CreativeBrief from @adcp/sdk.
  • tightenMutualExclusionOneOf runs before the merge (line 241), then merge runs before the not / if-then-else filter (line 398). Sequencing is correct — non-ref allOf members fall through to the existing filter.

Mergeable. The strictness regression on BriefAsset / CatalogAsset is the only judgement call.

@bokelley

Copy link
Copy Markdown
Contributor Author

Addressed the reviewer's additionalProperties widening concern in eab4000.

What changed

resolveAllOfRefForMerge now applies enforceStrictSchema to the resolved base before returning it. Previously it returned the raw schema with only removeArrayLengthConstraints applied, which let the top-level additionalProperties: true flag on creative-brief.json and catalog.json propagate into the merged shape and emit a [k: string]: unknown | undefined index signature on BriefAsset / CatalogAsset. The fix runs the resolved base through the same strict-schema pipeline the parent uses, so the strip happens before the merge.

Recursion verified safe: loadCachedSchema reads a fresh JSON document per call (no shared mutable cache), and AdCP schemas aren't cyclic at the allOf:[{ $ref }] sibling level. Both schemas in question have no top-level allOf, so the new pre-merge pass on the resolved base doesn't even re-enter the same code path here.

Diff scope (tight, as expected):

  • scripts/generate-types.ts: +11 lines (the normalization + comment).
  • src/lib/types/core.generated.ts: removed [k: string]: unknown | undefined from BriefAsset and CatalogAsset.
  • src/lib/types/tools.generated.ts: same two index signatures removed.
  • test/type-generator-allof-ref-merge.test.js: new test pinning that resolved-base additionalProperties: true does NOT propagate. Verified the test fails on the prior resolveAllOfRefForMerge and passes after the fix.

Full test suite passes, format/build clean. The two pre-existing lint errors (prefer-const in storyboard runner, task-map.test.ts parsing) are unrelated and present on the parent commit.

The three "nice-to-have" items you flagged (collision-precedence pin, nested-base allOf test, etc.) are out of scope for this fix — happy to file follow-ups if useful.

bokelley and others added 2 commits May 16, 2026 07:37
…ission (#1756)

JSON Schema objects that mix sibling `properties`/`required` with
`allOf: [{ $ref }]` make `json-schema-to-typescript` emit a broken
union `( Base | { variant + duplicated base fields } )` where the
intent is `Base & { variant }`. Most visible inside `oneOf` discriminator
variants — `GetContentStandardsResponse` collapsed its success arm to
bare `ContentStandards`, silently dropping the variant's own `context`
and `ext` fields.

`enforceStrictSchema` now pre-merges any `allOf` member that is a
single `$ref` into the parent shape when the parent already declares
its own `properties`/`required` — variant-level fields win on
collision; the base's `additionalProperties` is inherited only when
the variant didn't override it. Local `#/$defs/...` fragments are
left in place (existing post-processors handle the Individual*Asset
alias case). `vendor-pricing-option`-style allOf-only roots stay
untouched.

Tests cover the three required scenarios: allOf-only pass-through,
single-variant merge, and a two-variant `oneOf` ending in a clean
discriminated union.

Knock-on type changes:
- `BriefAsset` / `CatalogAsset` flatten from `Base & { ... }` to merged
  interfaces (option 2 in adcp#4510 acceptance criteria).
- `GetContentStandardsResponse` success arm regains its `context?` /
  `ext?` fields.
- `CreativeBrief` no longer transitively reaches `tools.generated.ts`;
  re-exported from `core.generated` via `src/lib/index.ts`.

Unblocks adcp#4510 (schema dedup spike).

Closes #1756

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nalProperties widening

`resolveAllOfRefForMerge` previously returned the raw resolved base with only
`removeArrayLengthConstraints` applied. Because `creative-brief.json` and
`catalog.json` declare top-level `additionalProperties: true`, that flag
propagated into the merged shape and emitted a
`[k: string]: unknown | undefined` index signature on `BriefAsset` and
`CatalogAsset` — wider than either the pre-PR intersection form
(`BaseAsset & { asset_type }`) or the standalone `CreativeBrief` interface.

Apply `enforceStrictSchema` to the resolved base inside
`resolveAllOfRefForMerge` so `additionalProperties: true` at the base's top
level gets stripped before the merge. Recursion terminates: `loadCachedSchema`
reads a fresh JSON document per call (no shared mutable state) and AdCP
schemas aren't cyclic at the `allOf:[{ $ref }]` sibling level.

Adds a regression test pinning that `additionalProperties: true` on the
resolved base does not propagate after the merge.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley bokelley force-pushed the worktree-agent-a7afa670de957eaea branch from eab4000 to b5cc211 Compare May 16, 2026 11:40
@bokelley bokelley merged commit 98b52eb into main May 16, 2026
10 checks passed
@bokelley bokelley deleted the worktree-agent-a7afa670de957eaea branch May 16, 2026 11:48
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.

Codegen handling of allOf+$ref produces broken union types (blocks adcp#4510 schema dedup)

1 participant