feat(decisioning): expose AdcpCapabilitiesConfig passthrough slots on DecisioningCapabilities (#2199)#2201
Conversation
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
2320431 to
981b219
Compare
There was a problem hiding this comment.
Right shape on the type surface and the wire output — ad-tech-protocol-expert confirms adcp.supported_versions is a defined string[] member of the capabilities schema and that forwarding the adopter's own self-declaration is not a witness-invariant breach (no upstream is being relayed). But the per-domain override merge ships shallow where the JSDoc and changeset both promise deep-merge, and that silently drops adopter-declared wire data. One fix needed before this is safe.
MUST FIX
-
src/lib/server/decisioning/runtime/from-platform.ts:1174-1180— shallow merge silently drops adopter override keys undermedia_buy.execution. The new construction does a two-level shallow spread:media_buy: { ...(adopterOverrides?.media_buy ?? {}), ...mediaBuyOverrides }
mediaBuyOverridescarries a nestedexecution: { targeting }key (line 1034) whenever the framework derives targeting andhasMediaBuyProjectionis true. When an adopter declares bothcapabilities.targetingandoverrides.media_buy.executionwith sibling keys (execution.frequency, additionalexecution.targeting.*), the spread replaces the adopter's entireexecutionobject with the framework's{ targeting }— the adopter's sibling keys are dropped frommedia_buy.executionon the wire. The downstreamapplyCapabilityOverrides(create-adcp-server.ts:1858) is a recursive deep merge, but it runs after the loss has already happened at construction time, so it can't recover it. This contradicts the PR's own documented contract — theoverridesJSDoc and changeset both call it a "deep-merge passthrough" where "framework-derived values remain authoritative on the keys the projection engine handles," which implies non-overlapping adopter keys survive. They don't. Deep-merge the per-domain blocks (reuse a recursive merge), or at minimum mergeexecutionexplicitly. Same shallow shape applies to thebrandandaccountblocks; smaller blast radius today because those projections emit flat keys, but the seam is wrong everywhere.
Things I checked
- Imports resolve.
MediaBuyFeatures/AccountCapabilities/CreativeCapabilities(utils/capabilities.ts:31/54/97) andAdcpCapabilitiesOverrides(create-adcp-server.ts:1055) all export;Partial<>wrapping matches the existing override-type convention. supported_versionsemit atcreate-adcp-server.ts:6143is clean....(capConfig?.supported_versions?.length && {...})suppresses the empty array, spreading a falsy value into an object literal is a safe no-op, and the schema (tools.generated.ts) allowsadcp.supported_versions?: string[]. Defensive copy ([...]) is consistent with the file.featuresprecedence is correct. Base laid fromcapConfig.features(create-adcp-server.ts:6149-6160),applyCapabilityOverridesoverlays the per-domainmedia_buyblock after (:6196), so framework-derivedaudience_targeting/conversion_tracking/content_standardswin on those three keys and adopter-only keys survive. The new test proves it: adopter setsaudienceTargeting: false, framework forcestrue, wire result istrue.- Gating is sound.
hasAdopterPassthroughs/hasOverridesObjectinclude all five new fields in the emit condition; no declared passthrough is dropped at the gate. The only silent drop is the nested-merge loss above, not a gating miss. - Changeset:
minoris correct for five new optional non-breaking fields. Descriptively named, not the random-slug form.
Follow-ups (non-blocking — file as issues)
media_buy: nulldelete-sentinel is neutralized when a projection fires.AdcpCapabilitiesOverrides.media_buyis typed... | nullandapplyCapabilityOverridestreatsnullas "delete this block." The...(adopterOverrides?.media_buy ?? {})coalesces the null away and the explicitmedia_buy:key overwrites it wheneverhasMediaBuyProjectionis true, so an adopter passingmedia_buy: nullgets the block re-emitted. Lower likelihood than the execution case — fix alongside #1 or call out as out-of-scope.- No format validation on
supported_versions. An adopter can pass a full-semver"3.1.0-beta.2"(a meta-field value the schema docstring explicitly says is not a valid wire value and must normalize to"3.1-beta.2"); the PR forwards verbatim andstring[]response validation won't catch it. A normalization assertion or a doc pointer would harden it.
Minor nits (non-blocking)
- The new test doesn't exercise the collision path.
test/server-decisioning-capability-projections.test.js:247-280setsoverrides.media_buy.execution.targeting.geo_countriesbut never setscapabilities.targeting, somediaBuyOverrideshas noexecutionkey and the shallow merge never collides — the MUST FIX bug passes this test green. Add a case that declares bothcapabilities.targetingand anoverrides.media_buy.executionsibling key, then assert both survive.
Fix #1 and add the collision test. The fix is contained and the rest of the PR is right — happy to re-approve once the merge is deep at that seam.
Closes adcontextprotocol#2199. Expose optional capability passthrough slots on DecisioningCapabilities. Fields: features, creative, account, overrides, and supported_versions. Also emit supported_versions on get_adcp_capabilities.adcp. Add a regression for the full definePlatform projection path.
981b219 to
5acfd7d
Compare
There was a problem hiding this comment.
Approving. Additive optional fields on DecisioningCapabilities that close a real definePlatform reachability gap — the high-level v6 surface now reaches the AdcpCapabilitiesConfig slice the low-level createAdcpServer API always exposed. Framework projections stay authoritative on the keys they own; adopter declarations sit underneath. Right shape.
Things I checked
adcp.supported_versionsis on the wire, not fabricated.ad-tech-protocol-expert: sound — the field is spec-defined attools.generated.ts(supported_versions?: string[]in theadcpblock, provenance adcp#3493), withmajor_versionsdeprecated in its favor. The buyer side already readsadcp.supported_versions(SingleAgentClient.ts,utils/capabilities.ts); this closes the symmetric seller-emit gap. Witness, not translator.- Release-precision shape is correct. Test ships
['3.1'], example ships["3.0","3.1"]— matches the schema's release-precision contract; full semver lives on the separatebuild_versionfield. - Deep-merge precedence is correct and test-covered.
from-platform.ts:...(adopterOverrides ?? {})lays the adopter base, thenmergeCapabilityOverride(adopterOverrides?.media_buy, mediaBuyOverrides)deep-merges framework on top per domain. The test proves it: adopter'skeyword_targets.supported_match_types: ['exact']survives alongside framework-projectedgeo_countries: true, and auto-derivedaudience_targeting: truecorrectly overlays the passthroughaudienceTargeting: falsebase. supported_versionsempty/undefined spread is safe.create-adcp-server.ts:6144—...(capConfig?.supported_versions?.length && {...})no-ops onundefinedand on[]({...0}spreads to nothing). No emit of empty[], no crash.- Changeset present and correctly typed —
minor, additive optional fields, non-breaking.code-reviewer: clean on category. Changeset-vs-wire audit: no removed exports, no param flips, new optional fields only →minoris right. - Type surface is real —
AdcpCapabilitiesConfig.supported_versions,overrides, and the exportedAdcpCapabilitiesOverridesall exist; thecapabilities.tsimport resolves.
Follow-ups (non-blocking — file as issues)
compliance_testingis the one override block not deep-merged.from-platform.ts:1178-1179clobbersadopterOverrides.compliance_testingwholesale instead ofmergeCapabilityOverride-ing it likemedia_buy/brand/account.code-reviewerconfirmed this doesn't lose real intent — the wirecompliance_testingblock has one field (scenarios), and the adopter's real path iscapabilities.compliance_testing.scenarios→declaredCT, which is honored. But the asymmetry reads like an oversight. Either usemergeCapabilityOverridefor symmetry or add a one-line comment explaining why the single-field block is exempt.isPlainObject/deepMergePlainObjectsare byte-for-byte duplicates of the same helpers increate-adcp-server.ts:1870-1892. Two copies of merge semantics drift. Extract to a shared util.supported_versionsis forwarded verbatim, not normalized. The schema docs warn SDKs reading full-semver from bundle metadata MUST normalize to release-precision before emitting. AnormalizeToRelease()pass (or a footgun comment) on this slot would harden it, matching the buyer-side normalization the SDK already documents.
Minor nits (non-blocking)
- Spread idiom is the odd one out.
create-adcp-server.ts:6144uses...(x?.length && {})where the surrounding capability blocks (6184, 6192) use imperativeif (capConfig?.x?.length) { ... }. Behavior is identical; matching the local idiom reads clearer. supported_versionsedge cases aren't test-covered. The happy path and theaudience_targetingprecedence are locked in; an empty-array assertion would pin the no-emit behavior so a future refactor can't regress it to[].
LGTM. Follow-ups noted below.
Closes #2199.
Summary
Adds optional passthrough fields to
DecisioningCapabilities<TConfig>so adopters usingdefinePlatform/createAdcpServerFromPlatformcan declare capability details that were already modeled by the lower-levelAdcpCapabilitiesConfigsurface:features?: Partial<MediaBuyFeatures>creative?: Partial<CreativeCapabilities>account?: Partial<AccountCapabilities>overrides?: AdcpCapabilitiesOverridessupported_versions?: string[]This is the Option A shape from the #2199 triage: one declaration point on the platform, with framework-derived projections still authoritative for fields the framework owns.
Notes
featuresis forwarded as the basemedia_buy.featuresconfig; rich framework projections such asaudience_targeting,conversion_tracking, andcontent_standardsstill force their matching feature booleans totrue.accountis forwarded as the base account block; existingrequireOperatorAuthandsupportedBillingsprojections still overlay that base.overridesare merged before framework-derived per-domain blocks, preserving existing framework precedence.supported_versionsis now emitted onget_adcp_capabilities.adcp.supported_versions; previously it was only used for version-error details.Validation
npm run build:libnpm run typechecknpm audit --audit-level=highnpx prettier --check src/lib/server/create-adcp-server.ts test/server-decisioning-capability-projections.test.js src/lib/server/decisioning/capabilities.ts src/lib/server/decisioning/runtime/from-platform.ts .changeset/2199-decisioning-capabilities-passthrough.mdNODE_ENV=test node --test --test-timeout=60000 test/server-decisioning-capability-projections.test.js