Skip to content

test(storyboards): mirror canonical storyboard source#5243

Merged
bokelley merged 4 commits into
mainfrom
bokelley/4907-status
Jun 2, 2026
Merged

test(storyboards): mirror canonical storyboard source#5243
bokelley merged 4 commits into
mainfrom
bokelley/4907-status

Conversation

@bokelley

@bokelley bokelley commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Mirror the current published SDK storyboard compliance bundle into static/compliance/source as the repo-owned canonical source for Phase 3: move storyboard scenarios into spec repo as canonical contract #5016.
  • Add source-authority linting, index membership checks, overlay regeneration, and CI wiring so local/CI storyboards grade against the repo source rather than a stale SDK cache.
  • Pin the local runner dependency to @adcp/sdk@8.1.0-beta.19 and add compatibility fixes for the stricter 3.1 RC generated types.
  • Fix the sales training agent response shape required by the updated canonical storyboards: get_products.cache_scope, wholesale feed unchanged probes, filters.is_fixed_price, and seeded filters.pricing_currencies behavior.

Closes #5016.
Closes #5228.

Validation

  • npm run test:compliance-source-authority
  • bash scripts/overlay-compliance-cache.sh
  • npm run test:storyboard-validations-paths
  • npm run test:storyboard-context-output-paths
  • npm run test:storyboard-sample-request-schema
  • npm run test:storyboard-response-schema
  • npm run test:storyboard-doc-parity
  • npm run test:storyboard-check-enum
  • npm run build:compliance
  • node scripts/lint-compliance-source-authority.cjs --dist-latest
  • npm run typecheck
  • precommit hook: test:unit, test:test-dynamic-imports, test:callapi-state-change, typecheck
  • bash scripts/run-storyboards-matrix.sh

Storyboard matrix:

/signals: ✓ 107 clean, 122 steps
/sales: ✓ 89 clean, 418 steps
/governance: ✓ 103 clean, 171 steps
/creative: ✓ 101 clean, 195 steps
/creative-builder: ✓ 99 clean, 170 steps
/brand: ✓ 106 clean, 97 steps

Pre-push also passed version sync, storyboard matrix, schema-link convention, and Mintlify broken-links validation.

bokelley added 2 commits June 1, 2026 05:02
# Conflicts:
#	package-lock.json
#	package.json
#	scripts/overlay-compliance-cache.sh
#	scripts/run-storyboards-matrix.sh
#	server/src/training-agent/comply-test-controller.ts
#	server/src/training-agent/product-factory.ts
#	server/src/training-agent/task-handlers.ts
@bokelley bokelley marked this pull request as ready for review June 1, 2026 10:21
aao-release-bot[bot]
aao-release-bot Bot previously approved these changes Jun 1, 2026

@aao-release-bot aao-release-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Follow-ups noted below. Right shape: canonical source under static/compliance/source/, generated artifacts under dist/compliance/{version}/, a lint that fails closed in both directions, and the overlay script clears the SDK cache before applying source so deletes can't survive. Matches the existing schema authority pattern.

Things I checked

  • lintSourceAuthority rules and walker in scripts/lint-compliance-source-authority.cjs:352-394 — generated artifacts (domains/, index.json) are banned in source, canonical top-level set enforced, phases:-bearing YAML count required so an empty mirror trips source_empty.
  • lintBuiltMirror + lintIndexMembership at scripts/lint-compliance-source-authority.cjs:396-430,310-350 — byte-identical compare via SHA-256, three-way membership compare against index.json's universal / protocols / specialisms arrays.
  • assertComplianceSourceAuthority and assertComplianceBuiltMirror hook points in scripts/build-compliance.cjs:128-144,547,559-565 — source lint runs before the build, mirror lint runs after buildTo writes index.json. if (require.main === module) main() at the foot preserves CLI behavior while exposing buildTo/generateIndex for tests.
  • scripts/overlay-compliance-cache.sh:147-156rm -rf is guarded by exact-string match [ "${SRC}" = "dist/compliance/latest" ], same conditional used elsewhere in the script. Order is correct: build → clear DST canonical dirs → overlay → run mirror lint against DST.
  • server/src/training-agent/task-handlers.ts:2666,2672,3011 context echo — static/schemas/source/core/context.json defines context as "echoed unchanged in responses" and get-products-response.json has context as a first-class response field. Conformant, not drift.
  • Changeset tier — patch is correct per docs/reference/versioning.mdx:148-166: "The conformance suite … versions independently and is patch-level by default." No static/schemas/source/** change, no task/error-code change.
  • Forward-drift allowlist pattern — every new entry carries the same expiry pointer ("Remove when 3.1.0-rc.4 schema lands"), the new test at tests/lint-storyboard-validations-paths.test.cjs:414-435 asserts each entry still reproduces the underlying violation, so stale entries fail closed. Matches the shrink-only convention already documented in tests/storyboard-sample-request-schema-allowlist.json's $comment.
  • CI wiring in .github/workflows/build-check.yml:38-45 — runs after npm run build, before the dist-assets assertion.

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

  • lintBuiltMirror ignores stray top-level entries in the target. Only the five CANONICAL_TOP_LEVEL_DIRS plus domains/ and index.json are inspected. An extra leftover directory in the mirror (e.g. an abandoned cache subtree from a previous bundle layout) silently survives. Tighten by asserting the target's top-level set is exactly {...CANONICAL_TOP_LEVEL_DIRS, 'domains', 'index.json'}.
  • lintIndexMembership will false-positive the first time a specialism references a protocol without a directory. scripts/lint-compliance-source-authority.cjs:322-336 derives expected.protocols from listDirs(protocols/), but scripts/build-compliance.cjs:discoverProtocols synthesizes protocol ids from specialism protocol: fields. Today every reference has a baseline dir so the sets agree; the first orphan reference trips index_membership_drift. Either reuse discoverProtocols or take the union.
  • overlay-compliance-cache.sh rm-rf list is hard-coded. If the SDK's bundled cache ever grows a new generated subtree (a future index/, per-version dir, etc.), it'll survive the cleanup and mask drift. Discovery beats a hand-maintained list here.
  • The AAO Verified (Spec)AgenticAdvertising.org Verified (Spec) rename is half-applied. Lines 28 and 65 of docs/building/verification/storyboards-vs-scenarios.mdx flip the name; ~20 other sites including docs/building/verification/conformance.mdx:16,25,27,29,31,198,232 and the canonical /docs/building/verification/aao-verified page still say AAO Verified. Either revert in this PR (cosmetic isn't worth the inconsistency) or sweep separately. Notable that this brand rename rode in on a patch-tier changeset with no mention in the changeset body.
  • The comply() link disappeared from the badge-issuance sentence. Pre-PR line 28 said the badge "is issued by running these YAML files through comply() and observing every assertion pass" with a hyperlink — clean operational definition. Post-PR it routes authority through AAO's heartbeat without naming the reproducible CLI. Restore the comply() link.
  • Rollout-order paragraph at docs/building/verification/storyboards-vs-scenarios.mdx:42-48 uses lowercase "must". If this is meant to bind the working group, make it RFC 2119 MUST and cross-link from docs/reference/versioning.mdx. If it's operational guidance, lowercase "should" reads cleaner.

Minor nits (non-blocking)

  1. contextEcho shape mismatch with prevailing pattern. server/src/training-agent/task-handlers.ts:2665 uses req.context ? { context: req.context } : {}; every other context-echo site in the same file uses ...(req.context !== undefined && { context: req.context }). Match the existing pattern so falsy-but-present context values echo identically across handlers.
  2. PR title doesn't follow the conventional-commits format used in recent history. [codex] Mirror canonical storyboard source vs e.g. fix(release): …, chore: …. Cosmetic.

Safe to merge.

# Conflicts:
#	server/src/training-agent/task-handlers.ts
aao-release-bot[bot]
aao-release-bot Bot previously approved these changes Jun 2, 2026

@aao-release-bot aao-release-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Follow-ups noted below. Source-authority shift is the right shape: protocols/, specialisms/, test-kits/, test-vectors/, universal/ as authored source vs. domains/ and index.json as generated, mechanically enforced by scripts/lint-compliance-source-authority.cjs running pre-build, post-build, and post-overlay. That's the load-bearing piece.

Things I checked

  • scripts/lint-compliance-source-authority.cjs:166-208lintSourceAuthority rejects domains/ and index.json in source and requires the five canonical top-level dirs. Correct split.
  • scripts/lint-compliance-source-authority.cjs:210-244lintBuiltMirror SHA-256s every source file against its mirror under dist/compliance/<target>/<dir>/, plus asserts domains/ is byte-for-byte equal to protocols/. Drift, missing-in-mirror, and stale-in-mirror all surface.
  • scripts/build-compliance.cjs:548,560-565 — source-authority lint runs before main build, built-mirror lint runs at the end of buildTo. Both fail-closed. The if (require.main === module) main() guard plus module.exports is safe — no existing requirer relies on side-effects.
  • scripts/overlay-compliance-cache.sh:147-156rm -rf of canonical entries before overlay is gated to SRC == "dist/compliance/latest", then the new lint runs against $DST. Stale SDK-cache entries can't survive a rename or delete from source.
  • .github/workflows/build-check.yml:38-44 — new lint wired into CI after build. Five storyboard lints chained into one job step.
  • .changeset/5016-canonical-storyboard-source.mdpatch is defensible because zero files under static/compliance/source/** are modified in this PR (the canonical mirror was already published via @adcp/sdk@8.1.0-beta.19). Worth saying so in the changeset body so future readers don't assume new wire constraints.
  • server/src/training-agent/task-handlers.ts:2744,3095contextEcho is additive and context is already declared on get-products-response.json as core/context.json. Not a smuggled wire change.
  • Test-plan checklist in the PR body — every storyboard matrix path green (/signals, /sales, /governance, /creative, /creative-builder, /brand) plus the new test:compliance-source-authority ran clean. The stale-allowlist-entry tests added in tests/lint-storyboard-{context-output,validations}-paths.test.cjs are in the validation list and passed, which confirms the lint's path resolver still can't traverse the polymorphism the allowlist documents — see below.

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

  • Allowlist reason field is mis-titled. The 19 entries in scripts/storyboard-validations-paths-allowlist.json and the 3 in scripts/storyboard-context-output-paths-allowlist.json are labeled "Forward schema drift from the @adcp/sdk@8.1.0-beta.19 storyboard mirror (#5016). Remove when the matching 3.1.0-rc.4 schema changes land in static/schemas/source." But signal_targeting_rules, signal_targeting_groups, signal_ref.scope, format_option_id, task_completion.media_buy_id, and the audit_observations[].* paths are all already present in static/schemas/source/. The real gap is the static path resolver — oneOf-arm traversal, details.claimed_value.* nested-object resolution, core/context.json extension-point fall-through. Same shape as the legacy idempotency_key entry this PR removed. Re-classify with per-entry specificity (e.g. static_path_resolver_limitation: oneOf-arm, core/context.json extension point) so the "remove when" condition is actionable. As-is, a future maintainer will grep, find the field already in the schema, and either delete the entry (re-introducing a lint failure) or leave a misleading reason in perpetuity.
  • enum@/scenario allowlist entries in tests/storyboard-sample-request-schema-allowlist.json are Ajv if/then const noise, not missing scenarios. compliance/comply-test-controller-request.json already defines force_creative_purged, query_directed_audit_observation, query_edited_audit_observation, force_upstream_unavailable, and seed_measurement_catalog. The errors are Ajv reporting each non-matching if.const branch in the cascading allOf as an enum-class failure. Either filter that error class at the validator, or rewrite the reason to say so explicitly.
  • Governance rename is buried. docs/building/verification/storyboards-vs-scenarios.mdx changes "AdCP Verified (Spec)" → "AgenticAdvertising.org Verified (Spec)" in two places, with the claim that AAO issues the qualifier when its compliance heartbeat runs the storyboards. That's a governance/brand change about who runs and stamps conformance — affects every implementer's marketing surface. Move to a separate PR with its own changeset and AAO sign-off rather than landing inside a tooling PR titled "Mirror canonical storyboard source."
  • domains/ alias has no sunset. The lint enforces domains/ is byte-for-byte equal to protocols/, which is the right shape today, but it's now permanent generated debt. Either declare a target version for removal or call it out in storyboards-vs-scenarios.mdx as permanent compatibility scaffolding so future contributors don't have to re-derive the rationale.
  • normalizeThreeZeroIdempotencyDates only covers one of the dates in the storyboard. server/tests/manual/run-storyboards.ts:244-260 rewrites 2026-06-01 and 2026-06-30 to 2099, but the current static/compliance/source/universal/idempotency.yaml flight dates are 2026-07-01..2026-09-30 and 2026-07-01..2026-07-31. Only the 2026-09-30 end-time branch matches today (2026-06-02). The 2026-06-* branches are dead code; the July dates are unnormalized. The 3.0-compat run will start failing on past dates after 2026-07-01 unless the date table is updated to match the file actually in tree.

Minor nits (non-blocking)

  1. yamlHasPhases is a regex against raw text. scripts/lint-compliance-source-authority.cjs:103-105 uses /^phases:\s*$/m — false-positives on a multi-line string containing exactly that line at column 0, false-negatives on phases: [] inline. A js-yaml.load + Array.isArray(doc.phases) check is one line and removes the foot-gun.
  2. listYamlBasenames accepts .yml, generateIndex only emits .yaml. scripts/lint-compliance-source-authority.cjs:159-165 strips both extensions; scripts/build-compliance.cjs only lists .yaml. A future .yml under universal/ would trigger index_membership_drift for a reason that's actually two listing rules disagreeing inside this PR's own scripts. Pick one.
  3. SRC override in scripts/overlay-compliance-cache.sh bypasses both cleanup and lint. Both the rm -rf block and the trailing lint invocation are gated to SRC == "dist/compliance/latest". A non-default SRC would overlay onto the SDK cache with neither safety net. Either drop the override (the script is single-purpose) or run cleanup+lint unconditionally.

Safe to merge once the allowlist reason wording lands as a follow-up.

@bokelley bokelley changed the title [codex] Mirror canonical storyboard source test(storyboards): mirror canonical storyboard source Jun 2, 2026

@aao-release-bot aao-release-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. New linter wires the source-of-truth boundary into both build and overlay, and the empty-allowlist test pattern keeps the suppressions honest.

Things I checked

  • scripts/lint-compliance-source-authority.cjs:75-95compareDirectories sha256s every file, catches drift either direction, walkFiles returns sorted output. Violation order is deterministic.
  • scripts/lint-compliance-source-authority.cjs:131-170lintIndexMembership builds expected from listDirs/listYamlBasenames and actual from index.protocols[*].id. Latent divergence with discoverProtocols noted below.
  • scripts/overlay-compliance-cache.sh:147-154rm -rf $DST/<dir> is bounded by the [ -z "$DST" ] || [ ! -d "$DST" ] guard at lines 45-48; $DST only resolves under node_modules/@adcp/sdk/compliance/cache/<version>. Safe.
  • scripts/build-compliance.cjs:790-797if (require.main === module) main() plus exported buildTo/generateIndex is the right shape for the test harness; no other in-repo caller requires this file with side-effect expectations.
  • server/src/training-agent/task-handlers.ts:2744req.context echo is conformant with static/schemas/source/media-buy/get-products-response.json:339-341 (refs core/context.json). Not an undocumented runtime convention.
  • The idempotency_key allowlist removal in scripts/storyboard-context-output-paths-allowlist.json pairs with universal/idempotency.yaml's create_media_buy_initial step no longer capturing it.
  • tests/lint-storyboard-context-output-paths.test.cjs:213-237 + tests/lint-storyboard-validations-paths.test.cjs:414-438 — the stale-allowlist test re-runs the lint with the allowlist disabled per entry. Right pattern; allowlists rot otherwise.
  • .changeset/5016-canonical-storyboard-source.md is patch. Harness-additive per playbook §"Conformance harness". Sound.

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

  • lintIndexMembership should mirror discoverProtocols' union. scripts/build-compliance.cjs:174-198 synthesizes protocols from listDirs(protocols/) ∪ {s.protocol for s in specialisms}, but scripts/lint-compliance-source-authority.cjs:147 only does listDirs. The first contributor who lands a specialism whose protocol: has no matching protocols/<id>/ directory will trip a phantom index_membership_drift on a green build. Today every specialism resolves cleanly (brand/creative/governance/media-buy/signals/sponsored-intelligence, all exist as dirs), so this is latent — but the linter shouldn't itself drift from the generator it's checking.
  • yamlHasPhases regex won't match phases: [] inline. scripts/lint-compliance-source-authority.cjs:99. Tighten /^phases:\s*$/m to /^phases\s*:/m.
  • normalizeThreeZeroIdempotencyDates is keyed to the 3.0 SDK cache dates, not the current source bundle. server/tests/manual/run-storyboards.ts:244-261. A one-line comment stating that pins it against a future source-date refresh silently no-opping.
  • @adcp/sdk release cadence vs. spec-CI overlay window. Per the rollout doc, downstream consumers grade against the bundled snapshot until the next SDK ships. Worth a tracking issue on keeping that lag short for stricter storyboard changes — ad-tech-protocol-expert flagged this as the load-bearing hazard of the source-authority migration.

Minor nits (non-blocking)

  1. req.context ? { context: req.context } : {} echoes {} for a truthy empty-object request. server/src/training-agent/task-handlers.ts:2744. Hold off tightening unless a storyboard actually exercises the empty case.
  2. PR body overstates the dep change. "Pin the local runner dependency to @adcp/sdk@8.1.0-beta.19" — package.json only adds the new test:compliance-source-authority script; the SDK was already at ^8.1.0-beta.19 on main. A drifted summary line, not a code issue.

Approving on the strength of the source-vs-mirror gate plus the stale-allowlist tests.

@bokelley bokelley merged commit ba7410c into main Jun 2, 2026
33 checks passed
@bokelley bokelley deleted the bokelley/4907-status branch June 2, 2026 09:47
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.

training-agent: add 3.1 cache_scope to get_products responses Phase 3: move storyboard scenarios into spec repo as canonical contract

1 participant