fix: expose brand verification input schemas#1985
Conversation
There was a problem hiding this comment.
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, andcanSafelyMerge(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.mergeadopts the right operand'sunknownKeys, 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_rightsremoved from the explicitTOOL_INPUT_SHAPESoverrides but still inTOOL_REQUEST_SCHEMAS(src/lib/utils/tool-request-schemas.ts:191) → still reaches consumers via the implicit spread. The#667 TOOL_INPUT_SHAPEStest still asserts it. verify_brand_claimis intentionally absent fromTOOL_INPUT_SHAPES. Type-test atsrc/type-tests/zod-object-intersections.type-test.ts:138confirmscustomToolFor('verify_brand_claim', …)fails at compile time.- Spec coherence. Matches upstream
discriminator: { propertyName: "claim_type" } + allOf:[envelope] + oneOf:[4 variants]fromadcontextprotocol/adcpdist/schemas/3.1.0-beta.3/brand/verify-brand-claim-request.json. SDK still a witness — no fabricated fields, no defaultedadcp_major_version, no hoistedclaim_type. - Runtime coverage.
test/lib/adcp-3-0-blockers.test.js:656-670asserts 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_SCHEMASandcustomToolForSchemaare genuinely new symbols on@adcp/sdk/schemas. Defensible as apatchsince 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 "usecustomToolForSchema."dx-expert's suggested fix is a branded-neverslot on theverify_brand_claimkey with a__useInsteadmessage 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 sameenvelope.passthrough().and(z.union([Success, Error]))shape and would get the same.shape/discriminated-union ergonomics from this post-processor. Thename.endsWith('RequestSchema')filter is the right scope for this PR — widening to responses is a separate change. customToolForSchemausesz.input<TSchema>, matchingcustomToolFor. Handlers receive post-parse data from MCP validation, soz.output/z.inferis technically more correct. Symmetric with the existing helper for now; worth confirming the MCP SDK contract before flipping either.customToolForSchemais not mentioned indocs/llms.txtordocs/TYPE-SUMMARY.md. Add a line so the generated-from-llms surface picks it up.
Minor nits (non-blocking)
- Dead guard at
scripts/generate-zod-from-ts.ts:1052.arms?.length—unionArmsForExpressionalready returnsundefinedfor empty unions viasplitTopLevelList(:826). Harmless, slightly misleading. - "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.
void name;oncustomToolForSchema(src/lib/schemas/index.ts:184). Document the forward-compat rationale the waycustomToolFordoes at:160-163, or drop the parameter.
Approved.
There was a problem hiding this comment.
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*RequestSchemaexports, gated per-arm bycanSafelyMerge(line 731). Single schema actually changes:VerifyBrandClaimRequestSchema(src/lib/types/schemas.generated.ts:8134).- Order dependency is clean.
postProcessObjectIntersectionsruns after but finds no top-level.and(and noSchema.and(to re-enter — verified againstrewriteTopLevelObjectAnds(line 1091) andrewriteNamedObjectAnds(line 1124). update_rightsremoval from explicit overrides inTOOL_INPUT_SHAPES(src/lib/schemas/index.ts:117-119) is safe: still covered by the...TOOL_REQUEST_SCHEMASspread (src/lib/utils/tool-request-schemas.ts:191). Behavior unchanged at runtime.search_brands/verify_brand_claim/verify_brand_claimsare NOT inTOOL_REQUEST_SCHEMAS, so the explicit additions in bothTOOL_INPUT_SHAPESandTOOL_INPUT_SCHEMASare load-bearing — not dead writes.@ts-expect-errorat scripts/check-adopter-types.ts:231 pins the contract thatverify_brand_claimis union-shaped and must go throughcustomToolForSchema. The type test at src/type-tests/zod-object-intersections.type-test.ts:113-121 proves discriminated narrowing onargs.claim_typestill works throughz.input<ZodUnion<[Merge1, Merge2, ...]>>.- AdCP 3.0 spec models
verify_brand_claimas a true discriminated union (subsidiary/parent/property/trademark each carrying a distinctclaimpayload). 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)
- Bump should be
minor, notpatch. 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. canSafelyMergedoesn't checkunknownKeysmode. Today every component in the matched expression uses.passthrough(), so.mergeand.andare observationally identical. If a future schema mixes.passthrough()and.strict(), the rewrite would silently flip strictness —.merge's right-hand side wins, while.andwould intersect both rulesets. Worth a guard at scripts/generate-zod-from-ts.ts:731 before the post-processor matches a non-passthrough schema.- Request-only scope is a deliberate choice — document it.
name.endsWith('RequestSchema')at scripts/generate-zod-from-ts.ts:1015 meansUpdateRightsResponseSchema(src/lib/types/schemas.generated.ts:8131) and other Response schemas with the sameEnvelope.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 onpostProcessObjectUnionIntersectionssaying so would prevent a future maintainer from broadening the scope without thinking. TOOL_INPUT_SHAPESvsTOOL_INPUT_SCHEMASis 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; considerTOOL_INPUT_UNION_SCHEMAS/customToolForUnion(or similar disambiguator-by-purpose) before adopters land on the existing names.- 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)
- Test name drift.
test/generate-zod-object-intersections.test.js:139is 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. update_rightsdropped from prose. The inline list at src/lib/schemas/index.ts:84-89 used to call outupdate_rightsby 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
There was a problem hiding this comment.
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:1015—name.endsWith('RequestSchema')correctly scopes the rewrite.UpdateRightsResponseSchemaat the bottom ofschemas.generated.tsstill 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 thecanSafelyMergegate. Conflict-on-shared-field path falls back to the original.and(...), which the new test attest/generate-zod-object-intersections.test.js:143-157exercises.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 validatesclaim_typeseparately. New form forces the siblingclaimpayload to be witnessed against the same arm's variant — the runtime test attest/lib/adcp-3-0-blockers.test.js:660-672confirms both the parse-success and parse-fail directions.src/lib/schemas/index.ts:62-71—ToolInputSchemastype is correct. Removingupdate_rightsfrom the explicit override is safe becauseKnownToolRequestSchemas['update_rights']retains the narrow key via theTOOL_REQUEST_SCHEMASspread at line 131.update_rightsis framework-registered atcreate-adcp-server.ts:771— the old override was vestigial.src/lib/schemas/index.ts:174-186—z.input<TSchema>flows the discriminated-union type through to the handler. The@ts-expect-errorsite atscripts/check-adopter-types.ts:230(customToolForrejected for the union-shaped tool) is load-bearing — it's how adopters learn to usecustomToolForSchemaforverify_brand_claim.- Wire/types unchanged.
core.generated.tsandtools.generated.tsare 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 beminor. 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 isminor. Worth a changelog line so adopters running stricterverify_brand_claimtraffic upgrade with eyes open. z.discriminatedUnion('claim_type', ...)instead ofz.union. Every arm pinsclaim_typeto a distinct literal. Switching the post-processor (or a follow-on pass) to emitdiscriminatedUniongets youoneOf+discriminatorin the JSON Schema MCP serves ontools/list— LLM clients pick the variant deterministically instead of guessing acrossanyOfbranches — plus the no-fallback validation path. Same applies anywhere else the codegen produces literal-keyed unions.tools/listsnapshot test. The new runtime tests provesafeParseworks 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 thatverify_brand_claim.inputSchema.anyOf(oroneOfafter the discriminatedUnion follow-up) is well-formed would catch a future SDK regression that breaks tool discovery.verify_brand_claimannotation gap. Does not appear inMUTATING_TASKSand the diff does not setannotations.readOnlyHint. If it is read-only by design, mark it explicitly so the wrapper code atcreate-adcp-server.ts:5018-5026carries the right metadata.
Minor nits (non-blocking)
src/lib/schemas/index.ts:6-11JSDoc. "tools likecreative_approval,search_brands, and brand verification tasks that ship ascustomToolsextensions 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.src/lib/schemas/index.ts:122-128JSDoc onTOOL_INPUT_SCHEMAS. Worth a one-liner that it is a strict superset ofTOOL_REQUEST_SCHEMASso adopters know they can collapseTOOL_REQUEST_SCHEMAS.update_rights→TOOL_INPUT_SCHEMAS.update_rightsinto 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.
Summary
TOOL_INPUT_SCHEMASfor full generated request schemas alongside rawTOOL_INPUT_SHAPEScustomToolForSchema()for union/intersection-shaped custom tools likeverify_brand_claimValidation
npm run typechecknpm run build:libnpm run check:adopter-typesNODE_ENV=test node --test-timeout=60000 --test-force-exit --test test/generate-zod-object-intersections.test.js test/lib/adcp-3-0-blockers.test.jsNote: 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.