Skip to content

fix(capabilities): default conversion tracking supported_targets#2042

Merged
bokelley merged 2 commits into
mainfrom
adcp-client-1818-finish
May 27, 2026
Merged

fix(capabilities): default conversion tracking supported_targets#2042
bokelley merged 2 commits into
mainfrom
adcp-client-1818-finish

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Summary

Closes #1818.

Validation

  • npm run build
  • NODE_ENV=test node --test-timeout=60000 --test test/server-decisioning-capability-projections.test.js test/lib/capability-rollups.test.js
  • npm run typecheck
  • pre-push validation hook

@bokelley bokelley force-pushed the adcp-client-1818-finish branch from dcddad0 to a239e41 Compare May 27, 2026 03:44
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.

Clean fix. Closing the deferred slice of #1818 with a default projection at the same composition seam that already handles the other media_buy.* rich blocks is the right shape.

Things I checked

  • Witness-not-translator doesn't apply here — this is server-side composition (SDK helping the adopter compose get_adcp_capabilities), not buyer-side response reading. Different direction, different rule.
  • from-platform.ts:982-986 — null-safe (!= null && supported_targets == null), non-mutating (spread), preserves explicit arrays unchanged. Tested both branches.
  • as ['cost_per'] cast is correct against the generated ('cost_per' | 'per_ad_spend' | 'maximize_value')[] target. as const would have widened-then-narrowed to readonly and broken assignment.
  • Folding into the existing #1818/#1853 changeset (still minor, deferral note rewritten in place) is the right move — same release event, same issue lineage. No new changeset needed.
  • JSDoc on capabilities.ts:115-121 explains the default and the explicit-opt-in escape hatch loudly enough that an adopter reading the typed surface won't be surprised.
  • Schema-side: cost_per is in the AdCP-allowed enum (core.generated.ts), so the wire shape is conformant.

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

  • Coupling check. The schema docstring says "sellers MUST reject goals with unlisted target kinds." Defaulting ['cost_per'] onto the wire commits the adopter's handler to accepting cost-per goals end-to-end. The JSDoc + changeset cover this in prose; consider mirroring it in 8.1.0-beta migration notes so it's not a silent surprise for an adopter who declared conversion_tracking for the dedup field and never thought about targets.
  • Specialism universality. cost_per is the conservative default for most surfaces, but sales-broadcast-tv and sales-catalog-driven adopters that attribute on post-log counts (no per-impression cost math) might legitimately not compute it. Worth a quick poll of v8-line adopters on those specialisms before the minor ships.
  • Empty-array edge case. Current logic treats supported_targets: [] as "explicit and preserved" (it falls through the == null check). That's defensible — distinguishes "I opted out" from "I never set it" — but it's asymmetric with the schema's "omitted = no targets guaranteed" semantics. A one-line test pinning [] → [] (not ['cost_per']) keeps a future contributor from quietly flipping the check to ??.

Minor nits (non-blocking)

  1. Changeset test count. .changeset/1853-1818-capability-projection.md:41 — "5 new integration tests on the projection" reads as cumulative across the minor (3 from #1853 + 2 here). Correct under that reading; slightly ambiguous on a casual scan.

LGTM. Follow-ups noted below.

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.

Holding. The supported_targets omission has a normative spec semantic the projection overrides without spec backing, and the changeset claims patch for a wire-visible default flip.

Things I checked

  • from-platform.ts:982-986 projection — == null correctly handles null and undefined; spread preserves siblings
  • Sibling capability blocks (audience_targeting, content_standards, supported_optimization_metrics, frequency_capping) pass through verbatim at from-platform.ts:981, 987-989 — this PR breaks that established pattern
  • Tests cover default-when-omitted and preserve-when-explicit; basePlatform fixture is sound
  • Changeset present (default-conversion-supported-target.md, patch); existing 1853-1818-capability-projection.md updated to drop the "deferred" caveat

Blocking

  1. Spec-omission semantic. core.generated.ts:20048 says omission means "sellers MAY accept specific target kinds but buyers should not rely on it" — the spec's explicit best-effort signal. Projecting ['cost_per'] collapses that to "sellers MUST reject goals with unlisted target kinds" (same field). The framework knows the adopter declared the block; it does not know their attribution pipeline computes cost-per. The original deferral reasoning ("value_field support isn't tracked per-product today, so a full rollup isn't mechanical") applies the same way to cost_per — cost-per attribution is the conversion-tracking pipeline, not a free derivation off block presence. If we want a default, surface it as an adopter-side helper (defaultConversionTrackingTargets()) the adopter opts into — mirrors how legacy* helpers narrow semantics without hiding the choice.

  2. Semver claim wrong. default-conversion-supported-target.md is patch. Any adopter who declares conversion_tracking without supported_targets ships a different wire response after upgrade — visible to every buyer integration. minor at minimum.

Major (fix while in the diff regardless)

  • from-platform.ts:985as ['cost_per'] over-narrows to a fixed-length tuple. Field type is ('cost_per' | 'per_ad_spend' | 'maximize_value')[]. Compiles via tuple-to-array assignability but leaks a misleading literal type to readers of ct.supported_targets. Use as const (consistent with the pattern at L1004) or cast to the wider array.

Minor (non-blocking)

  1. Empty-array test missing. [] == null is false, so [] survives — the spec-conformant best-effort escape hatch under the proposed default. Worth a third test pinning [] → [] so the contract is load-bearing in the suite, not just in the JSDoc.
  2. JSDoc at capabilities.ts:115-121 doesn't mention the [] escape hatch. Reads as if the only opt-out is a non-default specific array. One line on [] closes the loop for adopters who want the spec's omission semantic back.

If there is an adcp# issue or spec change that elevates cost_per to a normative floor across all conversion-tracking sellers, point at it and the projection is fine — but the current spec text reads the other way.

@bokelley bokelley force-pushed the adcp-client-1818-finish branch from a239e41 to 998cef7 Compare May 27, 2026 03:52
@bokelley bokelley force-pushed the adcp-client-1818-finish branch from 998cef7 to cfc276b Compare May 27, 2026 03:55
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.

Blocking. Defaulting supported_targets: ['cost_per'] when the adopter didn't declare it fabricates a wire claim that the AdCP spec explicitly reserves to mean "no guarantee" — the framework is no longer a witness.

Why this is a block

The wire schema text for media_buy.conversion_tracking.supported_targets is normative (src/lib/types/tools.generated.ts:20661):

"Buyers should only submit event-kind optimization goals whose target.kind is listed here — sellers MUST reject goals with unlisted target kinds. When omitted, only target-less event goals (maximize conversion count within budget) are guaranteed; sellers MAY accept specific target kinds but buyers should not rely on it."

Omission is a defined wire state with a defined buyer-side contract. The PR overrides that semantics for every adopter who declared conversion_tracking to advertise supported_action_sources / supported_event_types / multi_source_event_dedup but never wired up a cost_per goal evaluator. cost_per requires bid-time goal evaluation against ingested events with cost attribution — a pipeline orthogonal to event ingestion itself. Buyers will submit target.kind: 'cost_per' against the projected claim; sellers will be forced to reject under the "MUST reject unlisted kinds" rule the spec is warding off. The SDK becomes the mislister.

The opt-out (supported_targets: undefined discriminated via hasOwnProperty.call) is the wrong shape for a wire-semantic distinction. TypeScript-land treats {x: undefined}{}JSON.stringify drops it, object spread preserves it, Zod .optional() strips it, exactOptionalPropertyTypes is off by default in most adopter tsconfigs. Adopters using any of those builders silently lose the opt-out. Footgun.

The 'every conversion-tracking seller can compute cost_per' premise in the changeset is an ecosystem claim, not a spec fact. The audience_targeting block — the prior-art neighbor at from-platform.ts:981 — does no analogous projection. The framework's prior convention is witness-only; this PR breaks the convention without WG cover.

Things I checked

  • from-platform.ts:981-989 — the hasOwnProperty.call discrimination is correctly implemented for the in-memory branch but doesn't survive any standard adopter serialization path.
  • capabilities.ts:106-124 — JSDoc is honest about what the projection does; the question is whether the projection should exist.
  • Spec text in tools.generated.ts:20661 — explicit and normative on omission semantics.
  • Changeset (default-conversion-supported-target.md, minor) — correctly typed for the impact if the behavior were sound; the behavior is the problem, not the changeset.
  • Tests at server-decisioning-capability-projections.test.js:94-140 — assert on structuredContent (in-memory) not on a JSON round-trip, so the "preserve undefined" path is only proven against the in-memory branch, not the wire.
  • audience_targeting projection a few lines up — no analogous default. Same shape of block, different convention. The PR breaks the local convention silently.

Fix options (ranked)

  1. Default to nothing. Drop the projection. Pass rawConversionTracking through untouched. Adopters who genuinely support cost_per declare it. Witness-faithful and one-line revert.
  2. Default ['cost_per'] only when a registered platform hook proves the seller has an event-goal evaluator — presence of the evaluator code is the witness, not the presence of an adjacent capability block.
  3. Keep the default, but file an issue on adcontextprotocol/adcp first and get the WG to either (a) change the spec so block presence implies cost_per, or (b) bless framework-level projection. Don't bake SDK opinion ahead of the spec — that's the triage rule from CLAUDE.md ("spec → mock → SDK").

Option 1 is the cheapest path back to the original deferred-rollup posture. The #1818 changeset already explained why this was deferred; reinstate that.

Minor (non-blocking, if you pursue a different shape)

  1. as Array<'cost_per'> cast at from-platform.ts:988. Asymmetric with the schema union ('cost_per' | 'per_ad_spend' | 'maximize_value')[]. Use ['cost_per'] as const or satisfies NonNullable<typeof rawConversionTracking>['supported_targets'] — widens correctly without lying about the literal tuple shape.
  2. Test asserts on in-memory object, not the wire. server-decisioning-capability-projections.test.js:139 reads result.structuredContent directly. For a contract test that promises "preserve omission semantics on the wire," round-trip through JSON.parse(JSON.stringify(...)) and assert the key is absent, not undefined. (This is also the test that would have caught the footgun in fix option 1.)
  3. Pre-existing conversion_tracking projects test at L77-92 now flows through the new default. Worth a deliberate assertion either way (['cost_per'] if you keep the default, absent if you don't) so the baseline test locks the contract.
  4. Changeset wording. "best-effort target signal" reads ambiguously. The actual semantic the spec gives omission is "no advertised targets, buyers fall back to target-less event goals." Tighten if you keep any version of this.

Happy to re-review on either a revert to the deferred posture or a smaller projection scoped to a registered evaluator hook. Will pull ad-tech-protocol-expert back through whichever shape lands.

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.

Mechanics are clean. Holding on the protocol-default question — the SDK is now claiming a MUST reject contract on adopters' behalf, and the changeset doesn't say so.

Things I checked

  • Object.prototype.hasOwnProperty.call(...) discriminator at src/lib/server/decisioning/runtime/from-platform.ts:984 correctly separates omission from explicit undefined. Safe against prototype-pollution and Object.create(null) inputs.
  • Three new tests at test/server-decisioning-capability-projections.test.js:94-140 cover the three meaningful branches: omitted → ['cost_per'], explicit array → preserved, explicit supported_targets: undefined → preserved.
  • Type narrowing Array<'cost_per'> at from-platform.ts:987 is assignable to the wider wire enum ('cost_per' | 'per_ad_spend' | 'maximize_value')[] at src/lib/types/core.generated.ts:20048.
  • JSDoc at src/lib/server/decisioning/capabilities.ts:115-122 names the default, the rationale, and the opt-out spelling. The opt-out test lands the contract.
  • Changeset present, named, and prose-matched to the rest of the 8.1.0-beta.N adoption sweep.

Open question — wire semantics

The wire spec at src/lib/types/core.generated.ts:20046 is load-bearing on omission: "When omitted, only target-less event goals (maximize conversion count within budget) are guaranteed; sellers MAY accept specific target kinds but buyers should not rely on it. Sellers MUST reject goals with unlisted target kinds."

After this PR, every conversion-tracking adopter who never set supported_targets ships ['cost_per'] on the wire and binds themselves to a MUST reject contract on every other target kind. The opt-out only triggers when the key is present in the object literal — adopters who simply left the property off don't have it, and that's the majority case. That is a silent wire-behavior change on upgrade for existing adopters.

Two questions:

  1. Why minor and not major? The bump treats this as additive, but it changes the wire response for adopters who relied on omission semantics. The bind-to-MUST reject half is the part that matters — buyers reading supported_targets: ['cost_per'] are entitled to assume the seller will reject everything else, even if today the seller would have happily accepted target-less goals.
  2. Is the right seam the SDK or the schema? If cost_per is the implicit floor for any conversion-tracking seller, that belongs as a JSON-schema default on media_buy.conversion_tracking.supported_targets — every SDK gets it for free, the spec carries the load-bearing claim, and adopters don't get a witness/translator boundary blurred at the SDK layer. The parallel supported_billing: ['agent'] default at from-platform.ts:1067 is schema-driven (minItems: 1 would fail validation without it); this one is a domain assumption with no schema backing. Worth a parallel issue on adcontextprotocol/adcp.

Notable: the changeset paragraph this PR is amending out — "intentionally deferred… adopters declare that field manually until the spec gives us a per-product signal" — argued the case for keeping the SDK out of this exact seam. The PR reverses that decision without saying what changed.

If the answer is "we own the spec, we're shipping this from the SDK side ahead of the schema change", fine — say so in the changeset prose, bump to major to flag the wire-behavior shift, and file the parallel adcp issue so the schema catches up and other SDKs converge.

Minor nits (non-blocking)

  1. Type assertion at from-platform.ts:987. ['cost_per'] as Array<'cost_per'> reads heavier than ['cost_per'] as const and conveys the same narrowing. Either spelling works.

Holding for the changeset-bump + spec-coordination answer.

@bokelley bokelley merged commit df047c2 into main May 27, 2026
15 checks passed
@bokelley bokelley deleted the adcp-client-1818-finish branch May 27, 2026 09:37
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.

Auto-derive seller-level rollup capabilities (supported_optimization_metrics, supported_targets) from product catalog

1 participant