feat(storyboards): lint sample_request against schema_ref with ratchet allowlist#2768
Merged
Conversation
…t allowlist (adcp#2763) The check_governance caller drift was the visible symptom of a structural gap: nothing statically validated storyboard sample_requests against the schemas they declare. Fixtures could drift from the schema, and from parallel protocol fixtures, indefinitely — only a runtime Matrix run surfaced it. Add a new storyboard lint that: - walks every step with schema_ref + sample_request, - strips $context.*, $generate:*, $test_kit.* into schema-typed placeholders, - skips negative-test steps (validations assert error_code / http 4xx-5xx or the step carries sample_request_skip_schema: true), - ajv-validates the normalized payload. Ships with a known-issues allowlist (46 entries) fingerprinted by keyword@path:detail so new drift on grandfathered steps still fails. Stale allowlist entries (step fully fixed but entry left behind) also fail. Regenerate with --write-allowlist after a real fix; never hand-edit to silence a new violation. Wired into `npm run test:storyboard-sample-request-schema` and the top-level `test` script. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code review, test strategy, and protocol-domain feedback all landed a
consistent set of issues. Addressing them in one pass.
Must-fix:
- SUBSTITUTION_RE now covers {{runner.*}} and {{prior_step.*}} in addition
to the $-prefix forms. The sales-broadcast-tv URI-format allowlist entry
was a false positive caused by {{runner.webhook_url:...}} not being
recognized as a substitution; it drops out of the allowlist.
- Ratchet reducer gets a direct pure-function unit test (new-drift vs
grandfathered vs stale). Previously the suite only asserted the current
tree was clean — a silent break in reconcileAgainstAllowlist would have
gone undetected.
- resolveSchemaNode now accepts a discriminator (key+value) and picks the
matching oneOf/anyOf branch instead of always taking [0]. discriminatorFor
surfaces the discriminator from a payload object against a composite
schema. Placeholder-only; ajv still does real branch selection.
Cleanup:
- Single shared Ajv instance + per-schema validator cache. Runtime fell
from ~12s to ~0.5s.
- fingerprintError JSON.stringifies non-scalar allowedValue so object/array
const mismatches don't collapse to "[object Object]".
- Walker skips null/empty phase and step ids; YAML parse errors surface as
a separate, non-grandfatherable bucket.
- runner-output-contract.yaml had two top-level `summary:` keys blocking
js-yaml; renamed the output-section one to `run_summary:` to match its
siblings (step_result / validation_result / skip_result).
- Allowlist entries written alphabetically sorted for diff-friendliness.
- --write-allowlist defaults to shrink-only; --allow-grow is required to
add a new entry. Prevents authors regenerating-to-silence unrelated drift.
Allowlist shrank 46 → 44 (false positive gone, yaml-parse-error entry gone).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 22, 2026
bokelley
added a commit
that referenced
this pull request
Apr 22, 2026
…sync_governance fixtures (#2781) First cluster follow-up to the schema lint landed in #2768. Three `create_media_buy` fixtures used `authentication: { scheme: "HMAC-SHA256" }` at push_notification_config. The schema requires `{ schemes: ["HMAC-SHA256"], credentials: "..." }` with `additionalProperties: false` — plural `schemes` is an array, and `credentials` is required. Fixed with HMAC credentials matching the style already in use in sales-broadcast-tv / brand-rights. Six `sync_governance` fixtures registered `governance_agents[]` entries without any `authentication` block. The schema requires it; omitting credentials is a security regression, not cosmetic drift. Fixed with the Bearer-token shape already in use in protocols/media-buy/index.yaml `sync_governance`. Allowlist shrank 44 → 35 (all 9 stale entries removed). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 22, 2026
bokelley
added a commit
that referenced
this pull request
Apr 22, 2026
…p#2763) (#2788) * fix(storyboards): resolve format_id / signal_ids entity-id cluster (adcp#2763) Second cluster follow-up to the schema lint from #2768. Two parts. Lint improvement. `placeholderFor` now recursively synthesizes shape-valid object placeholders for substitutions that land at object-typed schema locations — including oneOf/anyOf discriminated unions. Before this, a substitution like `$context.first_signal_id` landing at a `oneOf` of object variants produced `{}`, which ajv rejected against every branch's `required`. At runtime the substitution resolves to a concrete object captured from a prior step's response, so the lint should treat it as shape-valid. Five false positives across `format_ids`, `format_id`, and `signal_ids` now pass automatically. Fixture fix. `specialisms/creative-template/index.yaml#build/build_multi_format` was missing the required `format_id` on `creative_manifest`. Added `{ agent_url, id }` matching the canonical `core/format-id.json` shape. Allowlist shrinks 35 → 29. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: regression guard for object-typed substitution synthesis Code reviewer on PR #2788 flagged that the new `placeholderFor` / `synthesizeObject` / `firstObjectBranch` code path had only end-to-end coverage via the allowlist shrink. Adds a direct unit test asserting: - plain `type: object` with required fields produces a populated object - discriminated `oneOf` picks the first object branch and honors the const discriminator Keeps the next refactorer honest without having to reason about the whole allowlist. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
bokelley
added a commit
that referenced
this pull request
Apr 22, 2026
…63) (#2798) Third cluster-bundle follow-up to the schema lint landed in #2768, #2781, #2788. Allowlist shrinks 29 → 13. Lint change: isNegativeStep now treats `expect_error: true` as a canonical negative-step marker alongside the existing error_code / http_status_in heuristics. That's the widely-used convention in the suite (20+ sites) and lets reversed_dates and other deliberate error fixtures pass without per-fixture opt-ins. Fixture changes across 14 files. Main themes: - Shape completion (missing required fields): account on state-machine, reason on si_terminate_session, query/uses on get_rights_catalog, external_id on audience-sync add[] entries, event_time on log_event / log_events, event_source_id on sales-social event_logging, package_id on sales-non-guaranteed update_media_buy (with upstream capture), request_type/creative_manifest/assets on preview_display and preview_synced, creatives[] padding on reassign_creative. - Shape corrections: total_budget bare number → { amount, currency } object on two accept_proposal fixtures; flat brand/operator wrapped in accounts[{...}] on deterministic sync_accounts_for_state. - additionalProperty removal: accounts[].brand.name on sales-catalog- driven sync_accounts. - Explicit negative-test opt-in on error-compliance missing_fields (accepts either response or error — doesn't use expect_error). 13 entries remain, all blocked on WG decisions in upstream issues #2774-#2776 or the sales-social/sync_dpa_creative cluster (separate PR). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 22, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a storyboard lint that validates every step's
sample_requestagainst its declaredschema_ref. Closes the structural gap from adcp#2763 — specialism fixtures could drift from their schemas (and from parallel protocol fixtures) because nothing statically validated payloads against the schemas they named. The check_governancecallerdrift fixed in #2765 was the visible symptom; this is the fix that stops the whole class.What the lint does
schema_ref+sample_request$context.*,$generate:*,$test_kit.*) into schema-typed placeholderserror_codeor HTTP 4xx/5xx, or step carriessample_request_skip_schema: true)Ratchet allowlist
Ships with
tests/storyboard-sample-request-schema-allowlist.jsongrandfathering 46 pre-existing drift steps, fingerprinted bykeyword@path:detail. The ratchet:Regenerate with
node scripts/lint-storyboard-sample-request-schema.cjs --write-allowlistafter a real fix. Never hand-edit to silence a new violation.Drift clusters in the allowlist (follow-ups)
The 46 grandfathered entries cluster into these patterns — each becomes its own follow-up PR:
format_idused as string, schema wants{agent_url, id}object (4 fixtures)authenticationongovernance_agents/push_notification_config, includesschemevsschemestypo (6 fixtures)refine[]shape drift inget_products—proposal_id/product_idrejected,id/actionmissing (3 fixtures)contextfield treated as object by storyboards, string by several schemas (3 fixtures)accountrejected as additionalProperty on governance report/audit requests (2 fixtures)total_budget— schema wants object, fixtures use number (2 fixtures)event_time,source,width/height,external_id, etc.Plus one pre-existing YAML parse error in
universal/runner-output-contract.yaml(duplicate key) — separate bug.Test plan
npm run test:storyboard-sample-request-schema— passes (46 grandfathered, 0 new drift, 0 stale)npm run test:unit— 631 passednpm run typecheck— cleanadditionalPropertiesviolation on a grandfathered step fails the lint; removing it passestestscript🤖 Generated with Claude Code