fix(compliance): backport AgenticAdvertising.org suite fixture fixes#5231
Conversation
There was a problem hiding this comment.
Request changes. The status → media_buy_status rename in state-machine.yaml is a 3.0.x wire-conformance regression — the matching schema additive-deprecate from #4904 didn't come back with it — and dist/protocol/3.0.14.tgz is now stale against the in-repo dist/compliance/3.0.14/ it bundles.
Blockers
-
field_present: media_buy_statuson a 3.0.x schema that only definesstatus.static/compliance/source/protocols/media-buy/state-machine.yaml:225,261,298switch threeupdate_media_buyvalidations frompath: "status"topath: "media_buy_status". On 3.0.x,static/schemas/source/media-buy/update-media-buy-response.json:18declaresstatus(→media-buy-status.json) and nothing else — zeromedia_buy_statushits anywhere understatic/schemas/source/media-buy/. The companion additive-deprecate from #4904 that introducedmedia_buy_statusalongside the deprecatedstatuslanded onmainbut was not backported. A 3.0.x seller emitting the schema-documented{media_buy_id, status: "paused"}fails all three checks. Two fixes: revert the threepath:strings to"status"and ship a clean patch, or also backport #4904's schema rename and bump the changeset frompatchtominor. Apatchshipping a silent contract change on the conformance bar is the wrong shape.ad-tech-protocol-expert: unsound.code-reviewer: Critical. -
dist/protocol/3.0.14.tgzdesynced from in-repodist/compliance/3.0.14/**.scripts/build-protocol-tarball.cjs:185-189bundlesdist/compliance/{version}/into the signed tarball. The PR rewrites the Q2→Q3 windows indist/compliance/3.0.14/{protocols,domains}/media-buy/{state-machine,scenarios/measurement_terms_rejected}.yamlbut leavesdist/protocol/3.0.14.tgz(sha25680e2aca0…),.sha256, and.siguntouched — and the on-disk sha256 still matches the recorded one, so the tarball is internally consistent with its own old contents. SDK consumers that fetch by tarball get the expired May 2026 windows; consumers reading the unpacked tree get Q3. Re-tar and re-sign 3.0.14, or revert the dist/3.0.14 mutations and let 3.0.15 carry the window fix forward only.
Things I checked
runLintIfAvailablecorrectly no-ops the six lints not yet backported to 3.0.x (lint-compliance-packaged-refs.cjs,lint-vendor-metric-uniqueness.cjs,lint-storyboard-{check-enum,raw-mode-required,upstream-traffic-paths,advisory-expiry}.cjs). Branch-tolerance is the right shape for a backport.- New
stableKeyViolationsandduplicateGeneratedKeyViolationspaths inlintStoryboardIdempotencyare sound.universal/idempotency.yaml's deliberate alias-reuse allowance is preserved. omit_idempotency_key === truereplacing the prior blanketexpect_error === trueskip is the right tightening — it scopes the suppression to the explicit negative vector instead of every error path.- The Q2 → Q3 2026 window shift is load-bearing. Today is 2026-05-31; May 1–31 windows are expired.
required_any_of_toolsfrontmatter parsing matches therequired_toolsshape and forwards throughgenerateIndexcorrectly.is_fixed_price: truefilter is incore/product-filters.json;pricing_options[0].fixed_priceis the discriminator presence-check percore/pricing-option.json.c30 → c28is wire-legal —core/measurement-window.jsondefineswindow_idas freeform with examples only.
Follow-ups (non-blocking — file as issues)
runLintIfAvailabletypo silent-skip. A misspelledscriptNameproduces only a(skipping …)log line forever. Gate by a hardcoded allowlist of known-missing scripts so a future rename onmaindoesn't disable a real lint on 3.0.x silently.- Duplicate-generated-key framing. The new branch is sound, but the prior aliases (
measurement_terms_rejected_{aggressive,relaxed}_terms) were already unique on 3.0.x — no existing collision the fully-qualified rename resolves. Drop the collision framing from the changeset or cite the actual case. published_versionbackfill. Newly-built indexes carrypublished_version; the shippeddist/compliance/3.0.14/index.jsonstill doesn't. Either backfill or document the≤3.0.14fallback toadcp_version.
Minor nits (non-blocking)
c30 → c28inmeasurement_terms_rejected.yaml:125. Wire-legal, but unmentioned in the PR description and the changeset. One-line in-fixture comment would save the next reader the lookup.pricing_options[0].fixed_pricecheck atmeasurement_terms_rejected.yaml:78. Brittle to sellers that don't sort the fixed option to the head ofpricing_options. Pair withis_fixed_price: truefilter assumption explicitly, or assert membership rather than index 0.
The PR description advertises three things and quietly does a fourth. Worth flagging so the next cherry-pick from main doesn't repeat the schema/storyboard split.
Request changes — restore 3.0.x storyboard/schema parity, then resync dist/protocol/3.0.14.tgz.
f025471 to
e80ebcc
Compare
There was a problem hiding this comment.
LGTM. Date forward-port plus a tighter idempotency-key freshness lint, all on a branch where the optional sibling lints don't exist yet. Right shape for a 3.0.x backport.
Things I checked
- The dates were genuinely stale. Today is 2026-05-31; old windows
2026-05-01..2026-05-31were no longer forward-looking. Q3 2026 (2026-07-01..2026-09-30) is the correct shift acrossstatic/compliance/source/protocols/media-buy/scenarios/measurement_terms_rejected.yaml,static/compliance/source/protocols/media-buy/state-machine.yaml, and the fourdist/compliance/3.0.14/**mirror files. - The
expect_error === trueblanket skip removal won't break the build.code-reviewerswept everyexpect_error: truestep on 3.0.x: the only mutating one is the missing-key vector inuniversal/idempotency.yaml:176, which already carriesomit_idempotency_key: true— the new narrower skip flag inscripts/build-compliance.cjs. Net behavior change: stable-key violations now get caught onexpect_errorpaths too. Right direction. runLintIfAvailablefail-closed semantics.scripts/build-compliance.cjs:42-61exits 1 when an available lint script fails for all six wired callers; only thelint-storyboard-advisory-expiry.cjsinvocation passescrashMessageto treat script crashes (not rule violations) as warnings. All six forward-ported scripts are absent on 3.0.x today, so the calls are no-ops until the scripts land — backport-safe.- Idempotency_key freshness lint is grounded in spec.
ad-tech-protocol-expertconfirmed the "$generate:uuid_v4#..." rule is the same MUST fromdocs/building/implementation/security.mdx:380("use a fresh UUID v4 for each request"), and theuniversal/idempotency.yamlcarve-out atscripts/build-compliance.cjs:325lines up with the existing replay-alias convention there. reviewer_checksMUST is real, not invented. The new TERMS_REJECTED idempotency-claim-release MUST atstatic/compliance/source/protocols/media-buy/scenarios/measurement_terms_rejected.yaml:202operationalizessecurity.mdx#idempotencyrule 3 ("Only successful responses are cached") rather than inventing a stricter requirement.measurement_window: "c28"is schema-valid.core/measurement-terms.json:22-35treatsmeasurement_windowas a free-formwindow_idstring;c28is no less valid thanc30. The newfield_presentcheck onproducts[0].pricing_options[0].fixed_pricematchescore/product-filters.json:15semantics.- Changeset present and correct type.
.changeset/aao-compliance-suite-3-0-fixes.md,patchforadcontextprotocol. Right bump for a fixture/lint fix. - No schema source changes.
static/schemas/source/**is untouched; this is fixtures + build-script polish, no wire shape moves.
Follow-ups (non-blocking — file as issues)
required_any_of_toolshas zero downstream consumers today.grep -rn required_any_of_toolsfinds only the new build-script lines — no specialism declares it, no SDK reads it, no schema describes it. Forward-compatible (emitted only when present), but the 3.0.x backport is an odd first carrier for a wire field with no reader. Open a@adcp/sdktracking issue or pull the field forward only.published_version+adcp_versionis a dual-write with no deprecation timer. Both carry full semver perscripts/build-compliance.cjs:504. Fine today, but the comment says "legacy alias kept through 3.x for @adcp/client compatibility" without a 4.x removal note in the spec or a@deprecatedmarker onComplianceIndex. Either annotate the JSON schema or flag the 4.0 migration guide.runLintIfAvailablepath coupling. The existence check uses__dirname/<scriptName>but the exec usescd __dirname/.. && node scripts/<scriptName>. The two will diverge silently if anyone movesbuild-compliance.cjs. Invokenode <scriptPath>directly to keep them in lockstep.
Approving on the strength of the spec-grounded MUSTs (security.mdx:380, rule 3) plus the verified lint-skip coverage of all 16 expect_error steps on 3.0.x. Cut the 3.0.x patch after merge so hosted /compliance/3.0/ serves the corrected windows.
There was a problem hiding this comment.
LGTM. Clean backport with two genuine hardenings on top (stable-key + duplicate-alias detection). Follow-ups noted below.
Things I checked
- All
expect_error: truesteps on 3.0.x are safe under the new lint contract — every one either declaresidempotency_key, points at a non-mutating schema, uses a placeholdertasknot inmutatingTools, or setsomit_idempotency_key: true(onlyuniversal/idempotency.yaml:176). The behavior change atscripts/build-compliance.cjs:298-313doesn't break any existing storyboard. - No stable (non-
$generate:) idempotency keys anywhere instatic/compliance/source/**. The newstableKeyViolationsarray is empty on this branch. - The only duplicate
$generate:uuid_v4#aliasuse is insideuniversal/idempotency.yaml(the replay storyboard), which the new lint hard-allowlists. is_fixed_priceexists onstatic/schemas/source/core/product-filters.jsonon 3.0.x;fixed_priceexists on the pricing-option schemas. The new fixture validates.c28is valid:measurement_windowis unconstrainedstringwith non-normative examples.- All six conditionally-invoked lint scripts (
lint-compliance-packaged-refs.cjsand friends) are absent on 3.0.x, sorunLintIfAvailablecorrectly skips them — this is the intent per the PR description. - state-machine.yaml
field_present: statusassertions are already correct againstupdate-media-buy-response.jsononeOf[0].properties.status— no assertion change was needed, only the dates.
Follow-ups (non-blocking — file as issues)
runLintIfAvailablesilently drops 6 protections on 3.0.x patch releases.scripts/build-compliance.cjs:42-62. Each skip is one stdout line; in CI logs that's easy to miss. Worth either a single aggregate WARN summarizing all skipped lints, or a line in the changeset that 3.0.x patches ship without packaged-ref / vendor-metric-uniqueness / check-enum / raw-mode-required / upstream-traffic-paths / advisory-expiry coverage.- Changeset overstates the state-machine change.
.changeset/aao-compliance-suite-3-0-fixes.mdsays "state-machine assertions match the 3.0.xstatusresponse field" but the diff is purely date moves (2026-05→2026-07). Reviewers reading the changelog will look for assertion changes and find none. Rephrase to "state-machine fixture dates move to a forward-looking flight window; existingstatusassertions remain correct on 3.0.x." - Narrative contradicts reviewer_check in
measurement_terms_rejected.yaml. Narrative says "reusing the prior key against a different body MUST be rejected with IDEMPOTENCY_CONFLICT per spec." Persecurity.mdx#idempotencyrule 3 ("Only successful responses are cached"), a TERMS_REJECTED first call never populates the cache — so the same-key retry MUST succeed, which is exactly what the newreviewer_checksblock oncreate_media_buy_relaxed_terms(lines 201-204) probes. Either drop the IDEMPOTENCY_CONFLICT sentence from the narrative or qualify it to "after a successful first call." On a backport whose whole point is getting this rule right, the narrative shouldn't teach the wrong invariant. - Duplicate-alias escape hatch is hard-coded path equality.
scripts/build-compliance.cjs:333usesrel !== 'universal/idempotency.yaml'. A future per-task replay storyboard outside that file has no escape hatch other than amending the lint. Per-stepshare_idempotency_alias: trueor a top-level allowlist constant would age better. Non-urgent —universal/idempotency.yamlis the only replay storyboard today.
Minor nits (non-blocking)
- Stale
@adcp/sdk@5.xrename.scripts/build-compliance.cjs(the renamed transitional-alias comment): the repo uses@adcp/clienteverywhere (43+ imports acrossserver/,scripts/,docs/), and the newly-addedpublished_versioncomment block correctly references@adcp/client. One comment line was renamed to@adcp/sdk@5.x, leaving the file internally inconsistent. On a 3.0.x backport where the published SDK is still@adcp/client, revert that one line. - Positional assumption in fixed-price check.
measurement_terms_rejected.yaml:78-80:field_presentatproducts[0].pricing_options[0].fixed_priceassumes the first pricing option is the fixed-price one. Theis_fixed_price: truefilter only guarantees at least one option qualifies — agents are free to return auction options first. A JSONPath query (pricing_options[?(@.fixed_price)]) or a description that flags the positional assumption is safer. - Path-separator portability.
scripts/build-compliance.cjs:333—path.relativereturns native separators;rel !== 'universal/idempotency.yaml'would mis-compare on Windows. CI is Linux, so this is a portability nit, not a defect.
ad-tech-protocol-expert: sound-with-caveats — no spec divergence; the only normative issue is the narrative contradiction noted above. code-reviewer: no blockers; the lint hardening is correctly anchored against the existing source tree.
Approving on the strength of the lint hardening plus the verified all-clear sweep across expect_error and $generate: use sites. Ship the patch release once CI confirms the build skips the six absent lints cleanly. The changeset and narrative wording are worth a one-commit polish before tagging — notable that a PR specifically backporting an idempotency rule ships with a narrative that contradicts it.
e80ebcc to
d1afe81
Compare
d1afe81 to
d6db4d4
Compare
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Clean backport — the idempotency narrative rewrite is the load-bearing fix and matches the 3.0.x spec.
Things I checked
scripts/build-compliance.cjs:296— oldif (step.expect_error === true) continue;removed. Newomit_idempotency_key === trueopt-out at L316 is a stricter, intent-declaring surface. Walked everyexpect_error: truestoryboard (15 files); the only one with a key insample_requestisuniversal/idempotency.yaml:457($generate:uuid_v4#replay_key), exempted by therel !== 'universal/idempotency.yaml'carve-out at the duplicate-alias check. Behavior is correct.scripts/build-compliance.cjs:42-62—runLintIfAvailableswallows non-zero exits only whencrashMessageis set, otherwiseprocess.exit(1). Onlylint-storyboard-advisory-expiry.cjspasses acrashMessage, matching the comment at L678-685 ("script exits 0 on rule violations; non-zero exit means script crashed").lintStoryboardIdempotencyrewrite —isGeneratedIdempotencyKeyat L221 matches both bare$generate:uuid_v4and#aliasform. No anchoring bug.readYamlFrontmatterrequired_any_of_toolsvalidation — array, non-empty, each entry{tools: string[≥2], rationale?: string}. Error messages cite path + index. Sound.- YAML date shifts — all three fixtures move Q2 (May 2026) → Q3 (Jul-Sep 2026). start < end, same calendar year, coherent.
measurement_terms_rejected.yamlnarrative — old text claimed key reuse against a different body MUST be rejected with IDEMPOTENCY_CONFLICT after TERMS_REJECTED. That contradictssecurity.mdxrule 3 (only successful responses are cached) — a rejected response leaves no canonical hash to conflict against. New text is the correct contract. The addedreviewer_checksclause requiring a freshmedia_buy_idon the second attempt is the right enforcement signal.c30→c28—static/schemas/source/core/measurement-terms.json:22definesmeasurement_windowas free-form string;c3/c7are examples, not enum members.c28aligns with the 28-day digital reconciliation window. Fixture hygiene win.- Changeset present, descriptively named,
patchis defensible (additive index.json fields, no wire-schema change).
Follow-ups (non-blocking — file as issues)
published_versionandrequired_any_of_toolsare 3.1.0-beta surfaces. Both land in 3.0.x bundles via this PR without a 3.0.xstoryboard-schema.yamldefinition orComplianceIndexschema update on 3.0.x. The parser is dormant until a 3.0.x specialism opts in, but adopters that key offindex.jsonsee new fields without a doc anchor. Consider landing the schema/doc update on 3.0.x in a follow-up patch, or gating the emit on a version check.- Six branch-tolerant lint skips warrant WG sign-off.
lint-compliance-packaged-refs,lint-vendor-metric-uniqueness,lint-storyboard-check-enum(#3830),lint-storyboard-raw-mode-required(#3847 item 2),lint-storyboard-upstream-traffic-paths(#5073),lint-storyboard-advisory-expiry— all silently skip on 3.0.x. That means 3.0.x publishing tolerates check-enum typos, raw-mode misuse, and identifier_path grammar drift that the WG already deemed errors on main. Acceptable for a backport patch; flag the policy decision explicitly.
Minor nits (non-blocking)
runLintIfAvailableparameter shape.crashMessageis implicitly a boolean policy switch (truthy → swallow, falsy → exit 1).{ allowCrash: true, message: '...' }or acrashAdvisoryrename would make the policy harder to misuse for future call sites.scripts/build-compliance.cjs:42.required_any_of_tools.toolshas no internal de-dup. A future author writingtools: ["x", "x", "y"]passes the≥2gate while semantically being a single-tool family.scripts/build-compliance.cjs:121.
Approving on the strength of the idempotency-narrative correction and the lint tightening. Cut the 3.0.x patch after merge so hosted 3.0 serves the fixed successor to 3.0.14.
There was a problem hiding this comment.
Approving. The backport is clean and the narrative correction is load-bearing — the old fixture was teaching a spec error.
Things I checked
omit_idempotency_key: trueis set atstatic/compliance/source/universal/idempotency.yaml:176, so swapping the lint opt-out fromexpect_errortoomit_idempotency_keydoesn't regress the missing-key negative vector. The conflict step keeps a valididempotency_keyand wouldn't trip the lint either.- New narrative on
measurement_terms_rejected.yamlmatchesdocs/building/implementation/security.mdx#idempotencyrule 3 ("Only successful responses are cached"). The old narrative was wrong —TERMS_REJECTEDis an error, the key claim is not retained, the buyer can legitimately retry the same key with corrected terms. Thereviewer_checkscite is well-anchored. measurement_windowis free-form string incore/measurement-terms.json/core/measurement-window.json, not a closed enum —c30→c28is a fixture-content choice, not spec drift.is_fixed_priceis a documented boolean filter incore/product-filters.json:15;pricing_options[].fixed_priceis documented across the pricing-options schemas.duplicateGeneratedKeyViolationscorrectly exemptsuniversal/idempotency.yamlat the populate-and-check site inscripts/build-compliance.cjs. Cross-file alias collisions outside that file get caught on the second occurrence.- Changeset is
patchand the wire surface is unchanged — fixtures + build script + additivepublished_versionfield onindex.json. Right bump.
Follow-ups (non-blocking — file as issues)
runLintIfAvailablecrashMessage branch inscripts/build-compliance.cjsprints to stderr andreturns. The caller comment says script crashes "SHOULD surface — don't silently swallow implementation bugs that mask the lint forever." With theexecSyncerror consumed andmain()continuing, CI exits 0 — the crash is exactly as silent as the comment warns against. On 3.0.x this is academic (the script doesn't exist here). Onmainit isn't. Change toprocess.exitCode = 1so the build keeps running but the run goes red.published_versionis additive and back-compat for@adcp/clientconsumers readingadcp_version, but worth one line in the changeset documenting the new key so the next consumer doesn't discover it by accident.reviewer_checksis established by precedent (universal/idempotency.yaml:486plus this fixture) but isn't declared instoryboard-schema.yaml. Worth declaring formally as a step-level array of strings.
Minor nits (non-blocking)
- Narrative says "same key"; fixture uses two aliases. The
measurement_terms_rejected.yamlnarrative +reviewer_checksdescribe a same-key retry acrossreject_terms→accept_terms, but the executable steps use distinct$generate:uuid_v4#...aggressive_terms/...relaxed_termsaliases — correct, because the new duplicate-alias lint would block same-aliases outsideuniversal/idempotency.yaml. One inline comment on the fixture pointing at the deliberate split would stop a future maintainer from "fixing" the alias mismatch and tripping the lint.
LGTM. Follow-ups noted below.
Summary
Verification
Release