Skip to content

fix: expose brand verification input schemas#1985

Merged
bokelley merged 5 commits into
mainfrom
bokelley/brand-rights-schema-dx
May 24, 2026
Merged

fix: expose brand verification input schemas#1985
bokelley merged 5 commits into
mainfrom
bokelley/brand-rights-schema-dx

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Summary

  • expose TOOL_INPUT_SCHEMAS for full generated request schemas alongside raw TOOL_INPUT_SHAPES
  • add customToolForSchema() for union/intersection-shaped custom tools like verify_brand_claim
  • distribute request envelope/object union intersections in generated schemas so brand-claim handlers keep discriminated-union inference
  • add brand discovery/verification helper coverage and a changeset

Validation

  • npm run typecheck
  • npm run build:lib
  • npm run check:adopter-types
  • NODE_ENV=test node --test-timeout=60000 --test-force-exit --test test/generate-zod-object-intersections.test.js test/lib/adcp-3-0-blockers.test.js

Note: JavaScript/DX expert subagents were attempted locally, but both failed with quota errors before returning review. I completed a manual JS/DX pass and added runtime coverage for the full-schema helper.

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 shape/schema split is the right shape — verify_brand_claim is a tagged union and a raw .shape would silently strip the claim_type → claim correlation, so a separate TOOL_INPUT_SCHEMAS + customToolForSchema path is the honest fix.

Things I checked

  • Rewrite is mathematically equivalent. Envelope.and(z.union([A, B, C, D]))z.union([Envelope.merge(A), …]) under distribution over union, and canSafelyMerge (scripts/generate-zod-from-ts.ts:731) bails when an arm collides with the envelope. The "conflicting arms" test exercises that path.
  • .passthrough() is preserved. Both envelope (schemas.generated.ts:2155) and all four arms (:2241,:2250,:2263,:2274) are .passthrough(), and Zod's .merge adopts the right operand's unknownKeys, so the merged form still passes through unknown keys. Wire validation unchanged.
  • Pipeline ordering is right. Object/union runs after marker-union collapse and before plain object-and merging at scripts/generate-zod-from-ts.ts:1288. The opposite order would leave the request schema untouched.
  • No public-API regression. update_rights removed from the explicit TOOL_INPUT_SHAPES overrides but still in TOOL_REQUEST_SCHEMAS (src/lib/utils/tool-request-schemas.ts:191) → still reaches consumers via the implicit spread. The #667 TOOL_INPUT_SHAPES test still asserts it.
  • verify_brand_claim is intentionally absent from TOOL_INPUT_SHAPES. Type-test at src/type-tests/zod-object-intersections.type-test.ts:138 confirms customToolFor('verify_brand_claim', …) fails at compile time.
  • Spec coherence. Matches upstream discriminator: { propertyName: "claim_type" } + allOf:[envelope] + oneOf:[4 variants] from adcontextprotocol/adcp dist/schemas/3.1.0-beta.3/brand/verify-brand-claim-request.json. SDK still a witness — no fabricated fields, no defaulted adcp_major_version, no hoisted claim_type.
  • Runtime coverage. test/lib/adcp-3-0-blockers.test.js:656-670 asserts a valid subsidiary claim parses and a misshapen sibling-variant payload is rejected.

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

  • Changeset type is patch; the audit rule says new public exports → minor. TOOL_INPUT_SCHEMAS and customToolForSchema are genuinely new symbols on @adcp/sdk/schemas. Defensible as a patch since the framing is "verify_brand_claim couldn't be registered before — here's the fix" but worth tightening on the next changeset.
  • Compile-time error on customToolFor('verify_brand_claim', …) says "property does not exist," not "use customToolForSchema." dx-expert's suggested fix is a branded-never slot on the verify_brand_claim key with a __useInstead message string. Highest-leverage change for coding-agent buildability — agents seeing "property does not exist" tend to fabricate the shape inline rather than search for the sibling helper.
  • 38 Response schemas in schemas.generated.ts (e.g. :5881, :6753, :8045) use the same envelope.passthrough().and(z.union([Success, Error])) shape and would get the same .shape/discriminated-union ergonomics from this post-processor. The name.endsWith('RequestSchema') filter is the right scope for this PR — widening to responses is a separate change.
  • customToolForSchema uses z.input<TSchema>, matching customToolFor. Handlers receive post-parse data from MCP validation, so z.output / z.infer is technically more correct. Symmetric with the existing helper for now; worth confirming the MCP SDK contract before flipping either.
  • customToolForSchema is not mentioned in docs/llms.txt or docs/TYPE-SUMMARY.md. Add a line so the generated-from-llms surface picks it up.

Minor nits (non-blocking)

  1. Dead guard at scripts/generate-zod-from-ts.ts:1052. arms?.lengthunionArmsForExpression already returns undefined for empty unions via splitTopLevelList (:826). Harmless, slightly misleading.
  2. "Keeps conflicting arms as intersections" test uses a single-arm union. A two-arm union with the conflict on one arm would exercise the early-return path inside the loop more directly. Single-arm passes today but doesn't pin the loop semantics.
  3. void name; on customToolForSchema (src/lib/schemas/index.ts:184). Document the forward-compat rationale the way customToolFor does at :160-163, or drop the parameter.

Approved.

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. Schema rewrite is wire-equivalent under .passthrough() and canSafelyMerge enforces disjoint fields, so the distribution preserves both runtime validation and TS narrowing on claim_type.

Things I checked

  • postProcessObjectUnionIntersections (scripts/generate-zod-from-ts.ts:1000-1069) is the only path that produces the rewrite, scoped to top-level <Object>.and(z.union([...])) on *RequestSchema exports, gated per-arm by canSafelyMerge (line 731). Single schema actually changes: VerifyBrandClaimRequestSchema (src/lib/types/schemas.generated.ts:8134).
  • Order dependency is clean. postProcessObjectIntersections runs after but finds no top-level .and( and no Schema.and( to re-enter — verified against rewriteTopLevelObjectAnds (line 1091) and rewriteNamedObjectAnds (line 1124).
  • update_rights removal from explicit overrides in TOOL_INPUT_SHAPES (src/lib/schemas/index.ts:117-119) is safe: still covered by the ...TOOL_REQUEST_SCHEMAS spread (src/lib/utils/tool-request-schemas.ts:191). Behavior unchanged at runtime.
  • search_brands / verify_brand_claim / verify_brand_claims are NOT in TOOL_REQUEST_SCHEMAS, so the explicit additions in both TOOL_INPUT_SHAPES and TOOL_INPUT_SCHEMAS are load-bearing — not dead writes.
  • @ts-expect-error at scripts/check-adopter-types.ts:231 pins the contract that verify_brand_claim is union-shaped and must go through customToolForSchema. The type test at src/type-tests/zod-object-intersections.type-test.ts:113-121 proves discriminated narrowing on args.claim_type still works through z.input<ZodUnion<[Merge1, Merge2, ...]>>.
  • AdCP 3.0 spec models verify_brand_claim as a true discriminated union (subsidiary/parent/property/trademark each carrying a distinct claim payload). The rewrite preserves that semantic.
  • Changeset present, tests cover both happy path and envelope-arm conflict negative path.

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

  1. Bump should be minor, not patch. Two new public exports (TOOL_INPUT_SCHEMAS, customToolForSchema) are new features, not bug fixes. Per CLAUDE.md house rule: minor = "New features, non-breaking changes." Edit .changeset/brand-rights-input-schemas.md before the Release PR runs, or it'll ship as 7.10.1 when it should be 7.11.0.
  2. canSafelyMerge doesn't check unknownKeys mode. Today every component in the matched expression uses .passthrough(), so .merge and .and are observationally identical. If a future schema mixes .passthrough() and .strict(), the rewrite would silently flip strictness — .merge's right-hand side wins, while .and would intersect both rulesets. Worth a guard at scripts/generate-zod-from-ts.ts:731 before the post-processor matches a non-passthrough schema.
  3. Request-only scope is a deliberate choice — document it. name.endsWith('RequestSchema') at scripts/generate-zod-from-ts.ts:1015 means UpdateRightsResponseSchema (src/lib/types/schemas.generated.ts:8131) and other Response schemas with the same Envelope.and(z.union([Success, Error])) shape don't get the same treatment. That's correct — handlers receive request types, not responses — but a one-line JSDoc on postProcessObjectUnionIntersections saying so would prevent a future maintainer from broadening the scope without thinking.
  4. TOOL_INPUT_SHAPES vs TOOL_INPUT_SCHEMAS is one letter of meaning. Names are visually near-identical and IDE autocomplete won't distinguish them. Rename window is open since the surface is brand new; consider TOOL_INPUT_UNION_SCHEMAS / customToolForUnion (or similar disambiguator-by-purpose) before adopters land on the existing names.
  5. JSDoc on the union-shaped schema itself. VerifyBrandClaimRequestSchema (src/lib/types/schemas.generated.ts:8134) is silent on the registration helper. A /** Union-shaped — register via customToolForSchema. */ comment emitted from the post-processor at codegen time would catch readers who land on the schema first and the docstring second.

Minor nits (non-blocking)

  1. Test name drift. test/generate-zod-object-intersections.test.js:139 is called "keeps conflicting arms as intersections" but actually exercises envelope-vs-arm conflict — the rewriter never merges arms with each other. Rename to "keeps envelope-arm conflicts as intersections" so future readers don't misread coverage.
  2. update_rights dropped from prose. The inline list at src/lib/schemas/index.ts:84-89 used to call out update_rights by name and now doesn't. Behavior unchanged (spread still covers it), but the docstring reads like it was removed. Add it back to the inline list.

fix: title on a feature-adding PR is the third one in two weeks — the changeset workflow can't read PR titles, but the human reviewers can. Worth a private note.

Approving on the strength of the wire-equivalence proof plus the deliberate Request-only scoping plus the @ts-expect-error contract that pins the union case to customToolForSchema.

…-schema-dx

# Conflicts:
#	src/lib/types/schemas.generated.ts
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 post-processor is the right primitive — Envelope.and(z.union([A, B]))z.union([Envelope.merge(A), Envelope.merge(B)]) is what restores discriminator-correlated validation for verify_brand_claim, and scoping the rewrite to *RequestSchema plus canSafelyMerge() keeps the blast radius contained. The dual map (TOOL_INPUT_SHAPES for shape-compatible tools, TOOL_INPUT_SCHEMAS for union/intersection request schemas) is the right shape — adopters get a single import surface without weakening the variant-correlated payload.

Things I checked

  • scripts/generate-zod-from-ts.ts:1015name.endsWith('RequestSchema') correctly scopes the rewrite. UpdateRightsResponseSchema at the bottom of schemas.generated.ts still uses .and(z.union([Success, Error])) and stays untouched, which is what we want — Success/Error envelopes are not a discriminated request union.
  • scripts/generate-zod-from-ts.ts:1045-1064 — depth tracking, skipQuotedOrRegexLiteral, the trailing-content guard, and the canSafelyMerge gate. Conflict-on-shared-field path falls back to the original .and(...), which the new test at test/generate-zod-object-intersections.test.js:143-157 exercises.
  • src/lib/types/schemas.generated.ts:8134 — exactly one line of behavior change. Previous form was loose: AdCPVersionEnvelopeSchema.passthrough() plus an independent union evaluation would accept {claim_type: 'subsidiary', claim: {parent_domain: 'x'}} because the envelope absorbs the stray field and the union arm validates claim_type separately. New form forces the sibling claim payload to be witnessed against the same arm's variant — the runtime test at test/lib/adcp-3-0-blockers.test.js:660-672 confirms both the parse-success and parse-fail directions.
  • src/lib/schemas/index.ts:62-71ToolInputSchemas type is correct. Removing update_rights from the explicit override is safe because KnownToolRequestSchemas['update_rights'] retains the narrow key via the TOOL_REQUEST_SCHEMAS spread at line 131. update_rights is framework-registered at create-adcp-server.ts:771 — the old override was vestigial.
  • src/lib/schemas/index.ts:174-186z.input<TSchema> flows the discriminated-union type through to the handler. The @ts-expect-error site at scripts/check-adopter-types.ts:230 (customToolFor rejected for the union-shaped tool) is load-bearing — it's how adopters learn to use customToolForSchema for verify_brand_claim.
  • Wire/types unchanged. core.generated.ts and tools.generated.ts are untouched; this is a pure SDK validation/typing tightening, not a witness-not-translator violation.

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

  • Changeset is patch; should be minor. Two new public exports (TOOL_INPUT_SCHEMAS, customToolForSchema) plus a runtime validation behavior change (previously-accepted cross-variant payloads now reject). Per the CLAUDE.md table that is minor. Worth a changelog line so adopters running stricter verify_brand_claim traffic upgrade with eyes open.
  • z.discriminatedUnion('claim_type', ...) instead of z.union. Every arm pins claim_type to a distinct literal. Switching the post-processor (or a follow-on pass) to emit discriminatedUnion gets you oneOf + discriminator in the JSON Schema MCP serves on tools/list — LLM clients pick the variant deterministically instead of guessing across anyOf branches — plus the no-fallback validation path. Same applies anywhere else the codegen produces literal-keyed unions.
  • tools/list snapshot test. The new runtime tests prove safeParse works on the SDK side. We do not have a test that asserts what the buyer sees when MCP serializes the union to JSON Schema. An in-memory transport + listTools() assert that verify_brand_claim.inputSchema.anyOf (or oneOf after the discriminatedUnion follow-up) is well-formed would catch a future SDK regression that breaks tool discovery.
  • verify_brand_claim annotation gap. Does not appear in MUTATING_TASKS and the diff does not set annotations.readOnlyHint. If it is read-only by design, mark it explicitly so the wrapper code at create-adcp-server.ts:5018-5026 carries the right metadata.

Minor nits (non-blocking)

  1. src/lib/schemas/index.ts:6-11 JSDoc. "tools like creative_approval, search_brands, and brand verification tasks that ship as customTools extensions because the spec models them outside the SDK's framework-registered surface" — the spec does model them (they have generated request schemas); what makes them custom-only is that the SDK chose not to wire framework registration. Reword to "...because the SDK does not wire framework registration for them." Also "two convenience helpers" at line 11 has four bullets under it — update the count.
  2. src/lib/schemas/index.ts:122-128 JSDoc on TOOL_INPUT_SCHEMAS. Worth a one-liner that it is a strict superset of TOOL_REQUEST_SCHEMAS so adopters know they can collapse TOOL_REQUEST_SCHEMAS.update_rightsTOOL_INPUT_SCHEMAS.update_rights into a single import.

The PR description note that JS/DX subagents failed locally — both ran clean from here and converged with code-reviewer and ad-tech-protocol-expert on the same read: ship it, but the changeset is the wrong size and discriminatedUnion is the better long-term target.

Approving on the strength of the discriminator-correlated runtime test and the conservative post-processor scope.

@bokelley bokelley merged commit c9a2e63 into main May 24, 2026
11 checks passed
@bokelley bokelley deleted the bokelley/brand-rights-schema-dx branch May 24, 2026 20:21
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