feat(compliance): cover vendor_metric optimization goals#4972
Conversation
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Clean encoding of the AdCP 3.1 vendor_metric contract — capability, target-kind, and reporting-coherence checks mirror the schema's normative wording, and the storyboard's INVALID_REQUEST vs TERMS_REJECTED framing matches the error-code scoping.
Things I checked
- Schema-vs-implementation coherence: the three preconditions in
static/schemas/source/core/optimization-goal.json:183map 1:1 onto the validator atserver/src/training-agent/task-handlers.ts:1998-2068. Capability key uses(vendor.domain, brand_id, metric_id)— symmetric withvendor_metric_values,reporting_capabilities.vendor_metrics, andcommitted_metrics. - Validator does not fire on unrelated goal kinds — early returns at L2021 (
goal.kind !== 'vendor_metric') and L2023 (!key). Sits afterproductMapbuild, mirrors the existingmetric_optimizationvalidator's shape directly above. - Storyboard fixture (
static/compliance/source/protocols/media-buy/scenarios/vendor_metric_optimization_flow.yaml:79-85) matchesvendor-metric-optimization.jsonrequired fields; sample requests correctly omitcommitted_at(response-stamped perpackage.json:74). - Test helper at
server/tests/unit/training-agent.test.ts:104-105unwrapserrors[0]intoresult, soresult.code/result.fieldassertions in the four new tests read the right path. - Empty-frontmatter changeset matches established repo precedent (
.changeset/4321-prospect-on-conflict-do-nothing.md,.changeset/4348-pipes-github-connection-state-lag.md) — non-package-bumping changes. ad-tech-protocol-expert: sound-with-caveats — three preconditions matchoptimization-goal.json:183,INVALID_REQUESTcorrect pererror-code.json:90scoping, noTERMS_REJECTEDconflict.code-reviewer: no blockers; control flow consistent with the existingmetric_optimizationvalidator pattern.
Follow-ups (non-blocking — file as issues)
- Storyboard skips the SHOULD-level discovery precondition. The narrative at L26-31 names three preconditions but only two are exercised. The third (
metric_idSHOULD appear in vendor'smeasurement.metrics[]) tightens to MUST at the next minor — worth a forward-compat phase now so sellers that hard-fail on catalog miss get flagged at the 3.2 boundary. - No target-less positive case. Validator at L2038-L2052 correctly bypasses
supported_targetswhentargetis omitted (target-less = maximize within budget), but the storyboard never exercises that branch. Add a positive phase that omitstargetso implementations get coverage on the "optional target" semantics. vendorMetricKeylowercases domain but not brand_id (task-handlers.ts:170-178). Brand-ref schema is silent on brand_id case; case-equal would be the safer reconciliation rule and matchesvendor_metric_valuessymmetry. Non-blocking — current behavior is defensible.
Minor nits (non-blocking)
- Unknown product_id masked as capability miss.
task-handlers.ts:2012-2014— whenpkg.product_idresolves to no product,productis undefined and the goal is rejected as "not in supported_metrics" instead of the laterPRODUCT_NOT_FOUND. Matches the existingmetric_optimizationvalidator's precedent at L1953/L1963, so it's consistent, not a regression — flagging only because a comment naming the masking would prevent the next reviewer from re-discovering it. - Target validated only when
kindis a string.task-handlers.ts:2038-2052—target: {}ortarget: nullsilently bypasses thesupported_targetscheck. Schema validation should catch malformed shapes earlier, but the metric-kind validators above probe types more defensively. Cheap to tighten. - Negative-test fragility on bid_price floors.
findProductWithVendorMetric(training-agent.test.ts:1893-1908) picks the first matching product and the first pricing option. If publisher fixture reordering ever pushes a fixed-floor ormin_spend_per_packagepricing variant to position 0, the bid_price=5.0 / budget=10000 in the three negative tests could trip a different validator first. Tests assert onresult.fieldso the regression would surface, but the helper should prefer a known-floor template the wayfindProductWithMetriccallers already navigate.
Approving on the strength of schema-conformant precondition logic plus the storyboard's clean error-envelope framing. The two unexercised paths (discovery SHOULD, target-less positive) are worth a follow-up issue but not load-bearing for shipping the contract test.
There was a problem hiding this comment.
Mostly clean. Holding for a question on the seller-level capability gate path and an error-code consistency question.
Things I checked
task-handlers.ts:2026-2117— vendor_metric validation block. Order of checks (capability → target → committed → reportable) is correct.vendorMetricKeybrand_id strictness matchesstatic/schemas/source/core/vendor-metric-optimization.jsonuniqueness key. Reportability check at:2096-2098correctly returns false whenreporting_capabilities.vendor_metricsis undefined. Every dereference is optional-chained.task-handlers.ts:3402-3405— seller-level rollupvendor_metric_optimization: { supported_targets: ['threshold_rate'] }. Themedia_buycapabilities block instatic/schemas/source/protocol/get-adcp-capabilities-response.jsonisadditionalProperties: true(line 665), so this is permitted — but the field is not defined in the schema. Sibling rollupsupported_optimization_metrics(line 547) is the established pattern.training-agent.test.ts:1890-2263— five tests cover five distinct failure paths. Each asserts a distincterrors[0].fieldJSONPath. No overlap that would mask a regression.vendor_metric_optimization_flow.yaml— committed_metrics shape (scope: vendor, vendor, metric_id) matchesmedia-buy/package-request.json:151-169. JSONPath-lite field assertions conform tocore/error.json#field. Changeset present (empty-frontmatter pattern matches2487-image-size-cap-for-c2pa-manifest.mdand friends — established convention for build/test-only changes).- PR description test plan — all change-path commands listed without unchecked items. The two reported "unrelated repo drift" items are pre-existing and not on the path this PR changes.
Open questions (would flip to approve)
-
Seller-level capability gate. The storyboard's
requires_capability.pathismedia_buy.vendor_metric_optimization.supported_targets. That path is not currently defined instatic/schemas/source/protocol/get-adcp-capabilities-response.json— onlysupported_optimization_metricsexists at the seller level. The agent emits the extension field attask-handlers.ts:3402, andadditionalProperties: truepermits it. Two paths forward:- Land a parallel schema PR adding
vendor_metric_optimization(withsupported_targets) to themedia_buycapabilities block — so any seller, not just this training agent, knows what to publish. The storyboard then gates on a real schema-defined field. - Or gate on the product-level path that IS in the schema (
vendor_metric_optimization.supported_metrics[].supported_targetspercore/vendor-metric-optimization.json:23-30), and drop the seller-level emission.
As written, the gate works for this agent because the agent publishes the extension, but no other conformant seller has a wire-defined signal to publish that would make them match the gate — they'd grade
not_applicableforever. Is a schema PR queued? - Land a parallel schema PR adding
-
Error-code consistency with sibling capability checks. The new code at
task-handlers.ts:2060and:2076returnsTERMS_REJECTEDfor "goal value not in declared product capability." The sibling checks for the same shape —reach_unit not in supported_reach_unitsat:1999,view_duration_seconds not in supported_view_durationsat:2015— returnINVALID_REQUEST. Perenums/error-code.json:138,TERMS_REJECTEDis scoped tomeasurement_termsrejection. The reporting-coherence checks at:2086and:2099are defensiblyTERMS_REJECTED; the two capability checks above them read more likeINVALID_REQUESTto match sibling precedent. Is the uniformTERMS_REJECTEDchoice deliberate?
Follow-ups (non-blocking — file as issues)
- Brief-relevance heuristic at
task-handlers.ts:1485-1488matches only "attention" or "vendor" substrings.core/vendor-metric-optimization.json:5calls out emissions and brand-lift as core use cases — sustainability briefs ("emissions", "scope3") and lift briefs ("brand lift") miss the vendor-metric bonus. Tightening to those keywords generalizes discovery for non-attention vendor metrics. - Storyboard narrative parity with unit tests. The fourth runtime check at
task-handlers.ts:2096-2107("optimizable but not reportable") has unit coverage attraining-agent.test.ts:2184but no corresponding negative phase in the storyboard. Either add a 4th negative phase, or note that thecommitted_metricsschema (media-buy/package-request.json:107) may already constrain this and the runtime check is belt-and-suspenders. vendor_metric_optimizationapplies to guaranteed and non-guaranteed products atproduct-factory.ts:565, unlikemetric_optimizationwhich gates on non-guaranteed (:406). Probably fine but worth a one-line decision.
Minor nits (non-blocking)
- Handler error message at
task-handlers.ts:2076says "vendor_metric_optimization.supported_targets" — the true per-product path isvendor_metric_optimization.supported_metrics[i].supported_targets. The shortened path is fine in user-facing prose but reinforces the seller-level-rollup framing flagged in question 1 above.
Notable choice on TERMS_REJECTED for capability mismatches — worth confirming before this code becomes the precedent the next vendor-extension PR copies from.
|
Argus follow-up addressed in Open questions:
Follow-ups/nits addressed:
Split out as follow-up issue:
One item intentionally unchanged from the earlier dismissed review: |
There was a problem hiding this comment.
Request changes on the changeset. Schema-source change without a versioned changeset ships the new seller capability untracked at the protocol version surface.
Blocking
.changeset/vendor-metric-optimization-storyboard.mdhas empty frontmatter on a schema-source-touching PR. The diff editsstatic/schemas/source/protocol/get-adcp-capabilities-response.json(lines 569-588 — new seller-levelvendor_metric_optimizationblock). Every prior PR that added an analogous seller-level capability rollup used"adcontextprotocol": minor—.changeset/4651-supported-optimization-metrics-capability.mdand.changeset/4639-conversion-tracking-supported-targets.md. The empty-frontmatter precedent cited invendor-metric-accountability-storyboard.mdwas storyboard-only with no schema-source edit, so it doesn't carry. Fix: set frontmatter to"adcontextprotocol": minor. The schema addition is additive (optional field,additionalProperties: true), sominoris the right type — notmajor.
Things I checked
- Validator control flow at
server/src/training-agent/task-handlers.ts:2021-2125— nested loops index correctly, each failure short-circuits the request, ordering mirrors the siblingmetric_optimizationblock and matches the per-step negative-path expectations in the storyboard. vendorMetricKeycanonicalization (task-handlers.ts:159-182) — lowercasesdomainonly;metric_idandbrand_idare schema-pinned lowercase viavendor-metric-id.json/brand.json, so the asymmetry is correct.- Schema enum parity — seller-level
supported_targetsenum["cost_per","threshold_rate"]matches product-levelstatic/schemas/source/core/vendor-metric-optimization.json:29exactly. - Error-code choice —
TERMS_REJECTEDfor capability and reporting-coherence misses tracksdocs/media-buy/media-buys/optimization-reporting.mdx:79-81;INVALID_REQUESTfor malformedtarget.kindfollows the structural-validation convention. Right shape. - Test honesty — five negative-path tests plus two positive paths, all assert
errors[0].fieldand code. PR description's validation block is fully checked; nothing claimed-but-unverified. - 3.0-compat — additive optional field with
additionalProperties: true, no risk to 3.0 storyboard matrix.
Follow-ups (non-blocking — file as issues)
- Storyboard gate over-narrows.
requires_capability.contains: "threshold_rate"atvendor_metric_optimization_flow.yaml:13-15markscost_per-only sellersnot_applicablefor the entire scenario, including the target-less positive path and three of the four negative paths that don't depend onthreshold_rate. Either split the scenario or gate only the unsupported-target-kind step onthreshold_rate. - Seller-level surface omits
supported_metricsrollup. Buyers can't discover which(vendor, metric_id)pairs any product supports without walking the catalog.additionalProperties: truekeeps the door open; track for a follow-up minor.
Minor nits (non-blocking)
- Misleading brief_relevance string at
task-handlers.ts:1517. WhenchannelScore === 0 && keywordScore === 0butvendorMetricScore > 0, the response reads "Matches keywords only with vendor-metric optimization." — there were no keyword matches. Render as "Matches vendor-metric optimization." in that branch. findProductWithVendorMetric()attraining-agent.test.ts:1893-1913depends on a single publisher. Pinnacle (publishers.ts:36-44) is the only profile carryingvendor_metric_optimization. If that fixture loses or renamesattention_score, every vendor-metric test throws. The seeded-fixture path viaseedVendorMetricProductalready exists — consider routing the capability-positive tests through it too.VENDOR_METRIC_OPTIMIZATION_BRIEF_TERMSranking widens brief results to vendor-metric products even with zero channel/keyword score. Intentional per the discovery test, but worth a one-line doc comment so future ranking edits don't strip it.
Ship it once the changeset frontmatter is set to "adcontextprotocol": minor.
|
Argus follow-up pass is pushed in e243e88. Addressed:
Follow-ups tracked / intentionally left:
Validation:
|
|
The Seller-level Triaged by Claude Code. Session: https://claude.ai/code/${CLAUDE_CODE_REMOTE_SESSION_ID} Generated by Claude Code |
|
Issue #4988 proposes adding a seller-level Generated by Claude Code |
|
Confirmed as tracked follow-up — folding #4988 into this PR is not the right call. Three reasons:
#4988 stays open with Generated by Claude Code |
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Validator preconditions, storyboard assertions, and the seller-level rollup line up cleanly — capability → target shape → target support → committed presence → reportability, in that order, with field paths that match what tests and YAML actually assert.
Things I checked
- Validator order and field paths at
server/src/training-agent/task-handlers.ts~L2030-2140 match the storyboard'serrors[0].fieldassertions one-for-one (packages[0].optimization_goals[0].metric_idatvendor_metric_optimization_flow.yaml:380,…target.kindat:440,packages[0].committed_metricsat:495and:556) and the unit-test expectations atserver/tests/unit/training-agent.test.ts:2188,:2217,:2299,:2326. vendorMetricKeyattask-handlers.ts:175-184normalizes domain casing on both sides of the comparison and treats missingbrand_idas''consistently, so avendor: { domain }-only goal matches avendor: { domain }-only supported entry. No false-negative path on this key.- Seller-level rollup parity:
handleGetAdcpCapabilitiesattask-handlers.ts:3453returnsvendor_metric_optimization: { supported_targets: ['threshold_rate'] }; v6 platform atv6-sales-platform.ts:191declares the same; schema enum atget-adcp-capabilities-response.json:577-586permits exactlycost_perandthreshold_rate. On the wire and in code they agree. - Schema change is purely additive optional with
additionalProperties: truemirroring siblingsupported_optimization_metricsatget-adcp-capabilities-response.json:547-568.minorchangeset is the right shape — non-breaking capability surface. - Publisher fixture at
publishers.ts:33-43addsattention_scoreto bothvendorMetricsandvendorMetricOptimization.supported_metrics[]— has to land in both for the validator's two-sided coherence check to clear, and it does. TERMS_REJECTEDfor capability/coherence rejections is normatively right perdocs/media-buy/media-buys/optimization-reporting.mdx:79-81,vendor-metric-optimization.json:5, andoptimization-goal.json:183— distinct from theINVALID_REQUESTused by sibling reach/cpcv shape-check flows, which is the intended distinction (terms vs. shape).- Six new unit tests assert
code,field, and message substring per path — not just smoke-tested happy paths.
Follow-ups (non-blocking — file as issues)
error-code.json:138still describesTERMS_REJECTEDnarrowly as 'Buyer-proposed measurement_terms were rejected.' This PR's normative basis for using it on vendor-metric optimization-goal preconditions is sound, but an implementor reading just the enum description will reasonably pickINVALID_REQUEST. Broaden the enum description to cover vendor-metric optimization-goal rejections.- Pre-existing docs drift on
docs/protocol/get_adcp_capabilities.mdx: the page documents neithersupported_optimization_metricsnor the newvendor_metric_optimizationrollup. Not caused by this PR but amplified by it. - Malformed vendor_metric goal with missing
vendor.domainormetric_idcontinues silently via theif (!key) continueguard attask-handlers.ts:2089. If no downstream validator catches it, the buy lands. Consider returningINVALID_REQUESTwithfield: …optimization_goals[j].vendor/.metric_idfor symmetry with the target-shape branch.
Minor nits (non-blocking)
brief_relevancereads awkward at three matches. With channel+keyword+vendor-metric all positive, the joined string attask-handlers.ts:1527producesMatches 1 channel(s) and keywords and vendor-metric optimization.Twoands. Oxford-list join would tighten it.vendorMetricKeydoesn't trim domain.task-handlers.ts:181lowercases but doesn't.trim(). No current fixture has whitespace; cheap insurance against fixture drift.
LGTM. Follow-ups noted below.
|
Additional expert-review fixes pushed in 6a81b34. Addressed before merge:
Validation:
Note: local npm run typecheck currently fails on existing Express 5 route-param typings across unrelated route files (for example server/src/http.ts and server/src/routes/*); none of those errors are in this PR's touched files. I did not include that unrelated cleanup here. |
|
Updates noted. For reviewers: the Generated by Claude Code |
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Three preconditions land where the spec puts them: capability + target-kind via TERMS_REJECTED, malformed payload via INVALID_REQUEST. Seller-level rollup mirrors the supported_optimization_metrics precedent.
Things I checked
- Validation ordering in
server/src/training-agent/task-handlers.ts:2082-2205. INVALID_REQUEST for shape misses (missingvendor.domain, missingmetric_id, missingtarget.kindwhentargetpresent, non-positivetarget.value); TERMS_REJECTED for capability and reporting-coherence misses. Line drawn at the same place aserror-code.jsondescription. vendorMetricKey()collision behavior (absentbrand_idvs"") at task-handlers.ts:181 — intentional, matchesscripts/lint-vendor-metric-uniqueness.cjs:70and the test attests/lint-vendor-metric-uniqueness.test.cjs:31. Not a bug.- Schema-vs-docs coherence: seller-level
supported_targetsenum[cost_per, threshold_rate]inget-adcp-capabilities-response.json:578-581matches the product-level enum atcore/vendor-metric-optimization.json:29and the worked example atdocs/media-buy/media-buys/optimization-reporting.mdx:102.maximize_valuecorrectly omitted (event-goal-only). additionalProperties: trueon the new object — matches the convention used byaudience_management,conversion_tracking,frequency_capping. Forward-compat for a likely seller-levelsupported_metrics[]rollup later.minorchangeset classification. Repo is at3.1.0-beta.3; additive optional field plus a validation path that only triggers onkind: "vendor_metric"(3.1-new) cannot break a buyer that wasn't already using the new goal kind. Correct.- Storyboard
requires_capability: media_buy.vendor_metric_optimization(YAML L12-14) — gates against the seller-level rollup this PR just added. Right gate. - Test isolation.
findProductWithVendorMetric()reads frombuildCatalog();seedVendorMetricProduct()uses uniqueproduct_ids per call; each test gets a freshserver = createTrainingAgentServer(DEFAULT_CTX). No state leakage acrossit.eachcases.
Follow-ups (non-blocking — file as issues)
- Docs lag the schema.
docs/reference/whats-new-in-3-1.mdx:139anddocs/media-buy/media-buys/optimization-reporting.mdx:64still describe only the per-productvendor_metric_optimization. The seller-level rollup added atget-adcp-capabilities-response.json:569-588is on the wire but not in the prose. Mirror thesupported_optimization_metricsrow in the optimization-reporting table. - Fourth check is stricter than the docs normate.
task-handlers.ts:2191-2202rejects when acommitted_metricis not inreporting_capabilities.vendor_metrics. Spec atoptimization-reporting.mdx:79-81documents only goal↔capability and goal↔committed_metrics. The check is defensible (the seller can't commit to a metric it doesn't list as reportable), but the storyboard'sreject_unreportable_vendor_metricphase bakes it in as MUST. Either document this as a fourth precondition or widen the storyboard to allow seller acceptance. TERMS_REJECTEDenum description still scoped to measurement_terms.static/schemas/source/enums/error-code.json:138still reads measurement_terms-only. After this PR, the code returns it for vendor-metric capability and coherence misses too. Widen the description in a follow-up so downstream sellers reading only the enum aren't surprised.- No guaranteed-delivery test for vendor_metric_optimization.
product-factory.ts:565-568explicitly does not gate vendor_metric_optimization ondelivery_type— that's the design claim. Every seeded fixture in the test file usesdelivery_type: 'non_guaranteed'. One guaranteed-delivery case would lock the claim.
Minor nits (non-blocking)
- Empty-string
targetKindslips the supported_targets check.task-handlers.ts:2163usesif (targetKind). Schema validation upstream rejectskind: "", but the defensive layer has a gap. Tighten the L2141 guard to also reject empty strings, or useif (typeof targetKind === 'string' && targetKind.length > 0)at L2163. target.value === 0is rejected but untested.task-handlers.ts:2153usestargetValue <= 0. Theit.eachattraining-agent.test.ts:2550only covers-1. Add avalue: 0case to close the boundary.inferVendorMetricBriefTermsregex asymmetry.task-handlers.ts:1037matches/(attention|focus)/but only adds the termattention. Works in practice (the metric_id token gets matched separately), but the asymmetry is a foot-gun if the identity tokens change later.brief_relevancegrammar. Three-way join attask-handlers.ts:1554producesMatches X channel(s) and keywords and vendor-metric optimization.— pre-existing pattern, twoands is ugly. Pre-existing.
Safe to merge.
Closes #4933
Summary
Adds compliance coverage for the AdCP 3.1
optimization_goals[].kind: "vendor_metric"contract. The new media-buy storyboard verifies discovery ofvendor_metric_optimization.supported_metrics[], positive create-media-buy acceptance when capability and reporting-coherence preconditions are met, and negative paths for unsupported vendor metrics, unsupported target kinds, and missing vendor-scopecommitted_metrics[].The training agent now publishes vendor-metric optimization capability on both capability surfaces, ranks vendor-metric-capable products higher for relevant briefs, and enforces that vendor-metric goals are both optimizable and reportable before accepting a media buy.
Expert Review
Ran protocol, ad-tech protocol, and code-review expert passes. Follow-up fixes addressed their findings:
TERMS_REJECTEDis used for vendor-metric capability/reporting-coherence rejection paths.media_buy.vendor_metric_optimization.supported_targetscontainingthreshold_rate.errors[0].field.reporting_capabilities.vendor_metrics.Validation
npm run test:storyboard-validations-pathsvendor_metric_optimization_flow.yamlnpm run build:compliancenpx vitest run server/tests/unit/training-agent.test.ts --config server/vitest.config.tsnpm run typechecknpm run test:storyboardsnpm run test:storyboards:3.0-compattest:unit,test:test-dynamic-imports,test:callapi-state-change,typecheckKnown unrelated repo drift remains in the full global lints:
test:storyboard-sample-request-schemareportsprotocols/creative/scenarios/native_in_feed.yaml, andtest:storyboard-context-output-pathsreportsspecialisms/sales-guaranteed/index.yaml.