fix(capabilities): default conversion tracking supported_targets#2042
Conversation
dcddad0 to
a239e41
Compare
There was a problem hiding this comment.
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 constwould have widened-then-narrowed toreadonlyand 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-121explains 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_peris 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 declaredconversion_trackingfor the dedup field and never thought about targets. - Specialism universality.
cost_peris the conservative default for most surfaces, butsales-broadcast-tvandsales-catalog-drivenadopters 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== nullcheck). 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)
- 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.
There was a problem hiding this comment.
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-986projection —== nullcorrectly handlesnullandundefined; spread preserves siblings- Sibling capability blocks (
audience_targeting,content_standards,supported_optimization_metrics,frequency_capping) pass through verbatim atfrom-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); existing1853-1818-capability-projection.mdupdated to drop the "deferred" caveat
Blocking
-
Spec-omission semantic.
core.generated.ts:20048says 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 howlegacy*helpers narrow semantics without hiding the choice. -
Semver claim wrong.
default-conversion-supported-target.mdispatch. Any adopter who declaresconversion_trackingwithoutsupported_targetsships a different wire response after upgrade — visible to every buyer integration.minorat minimum.
Major (fix while in the diff regardless)
from-platform.ts:985—as ['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 ofct.supported_targets. Useas const(consistent with the pattern at L1004) or cast to the wider array.
Minor (non-blocking)
- Empty-array test missing.
[] == nullis 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. - JSDoc at
capabilities.ts:115-121doesn'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.
a239e41 to
998cef7
Compare
998cef7 to
cfc276b
Compare
There was a problem hiding this comment.
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— thehasOwnProperty.calldiscrimination 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 onstructuredContent(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_targetingprojection a few lines up — no analogous default. Same shape of block, different convention. The PR breaks the local convention silently.
Fix options (ranked)
- Default to nothing. Drop the projection. Pass
rawConversionTrackingthrough untouched. Adopters who genuinely supportcost_perdeclare it. Witness-faithful and one-line revert. - 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. - Keep the default, but file an issue on
adcontextprotocol/adcpfirst and get the WG to either (a) change the spec so block presence impliescost_per, or (b) bless framework-level projection. Don't bake SDK opinion ahead of the spec — that's the triage rule fromCLAUDE.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)
as Array<'cost_per'>cast atfrom-platform.ts:988. Asymmetric with the schema union('cost_per' | 'per_ad_spend' | 'maximize_value')[]. Use['cost_per'] as constorsatisfies NonNullable<typeof rawConversionTracking>['supported_targets']— widens correctly without lying about the literal tuple shape.- Test asserts on in-memory object, not the wire.
server-decisioning-capability-projections.test.js:139readsresult.structuredContentdirectly. For a contract test that promises "preserve omission semantics on the wire," round-trip throughJSON.parse(JSON.stringify(...))and assert the key is absent, notundefined. (This is also the test that would have caught the footgun in fix option 1.) - Pre-existing
conversion_tracking projectstest 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. - 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.
There was a problem hiding this comment.
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 atsrc/lib/server/decisioning/runtime/from-platform.ts:984correctly separates omission from explicitundefined. Safe against prototype-pollution andObject.create(null)inputs.- Three new tests at
test/server-decisioning-capability-projections.test.js:94-140cover the three meaningful branches: omitted →['cost_per'], explicit array → preserved, explicitsupported_targets: undefined→ preserved. - Type narrowing
Array<'cost_per'>atfrom-platform.ts:987is assignable to the wider wire enum('cost_per' | 'per_ad_spend' | 'maximize_value')[]atsrc/lib/types/core.generated.ts:20048. - JSDoc at
src/lib/server/decisioning/capabilities.ts:115-122names 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:
- Why
minorand notmajor? The bump treats this as additive, but it changes the wire response for adopters who relied on omission semantics. The bind-to-MUST rejecthalf is the part that matters — buyers readingsupported_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. - Is the right seam the SDK or the schema? If
cost_peris the implicit floor for any conversion-tracking seller, that belongs as a JSON-schema default onmedia_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 parallelsupported_billing: ['agent']default atfrom-platform.ts:1067is schema-driven (minItems: 1would fail validation without it); this one is a domain assumption with no schema backing. Worth a parallel issue onadcontextprotocol/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)
- Type assertion at
from-platform.ts:987.['cost_per'] as Array<'cost_per'>reads heavier than['cost_per'] as constand conveys the same narrowing. Either spelling works.
Holding for the changeset-bump + spec-coordination answer.
Summary
media_buy.conversion_tracking.supported_targetsto["cost_per"]when conversion tracking is declared without an explicit target list.supported_targetsarrays unchanged.supported_targetsportion is no longer deferred.Closes #1818.
Validation
npm run buildNODE_ENV=test node --test-timeout=60000 --test test/server-decisioning-capability-projections.test.js test/lib/capability-rollups.test.jsnpm run typecheck