Conversation
…severity, recorded_calls purpose tagging (#3830 items 1, 3, 5) Three of the five LOW-priority items from #3830, filed against #3816's expert review. Bundled because they tighten the same upstream_traffic / authored-check surface; each is small and independent. Item 1 — Build-time storyboard check-enum lint: - runner-output-contract.yaml declares `authored_check_kinds` as a structured top-level list (single source of truth). - scripts/lint-storyboard-check-enum.cjs walks every storyboard and rejects unknown_check_kind (typos) and synthesized_check_kind_authored (storyboards declaring runner-emitted codes like capture_path_not_resolvable). Wired into build-compliance.cjs. - tests/lint-storyboard-check-enum.test.cjs covers source-tree guard plus per-rule fixtures with temp-dir storyboards. Wired into npm test. Item 5 — Advisory validation severity: - New optional severity: "required" | "advisory" (default: "required") on storyboard validation entries. Advisory failures surface in validation_result but don't fail the step; contribute to a distinct validations_advisory_failed counter on run_summary. - Use case: rollout gating during runner adoption windows — declare upstream_traffic with severity: advisory while @adcp/sdk catches up, flip to required once stable. Distinct from runtime forward-compat (which handles version skew); severity is author-managed. Item 3 — purpose tagging on recorded_calls: - New optional `purpose` enum on recorded_calls[].items: platform_primary | measurement | identity | other. Adopters self-classify; lets storyboards filter measurement-vendor noise from platform-primary assertions. - New optional purpose_filter: [string] on upstream_traffic storyboard checks. Calls without a purpose match only when purpose_filter is omitted (explicit filtering requires classification). Out of scope for this PR: - Item 2 (payload_attestation digest mode) — separate PR, bigger design. - Item 4 (reference upstream-traffic recorder middleware) — adcp-client repo. Build green; new lint passes 6/6 tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 2, 2026
Merged
…llow-ups Three-expert review of #3837 converged on tightening items 1/3/5 before merge. Addressed inline; deferred items file as follow-ups. Item 1 — authored_check_kinds placement: - Kept in runner-output-contract.yaml. storyboard-schema.yaml is comment-only documentation; adding structured fields there would break the file's pattern. Added a visible CANONICAL CHECK ENUM cross-pointer at the top of storyboard-schema.yaml's Validation section so storyboard authors editing that file see the canonical-list path (file + field). Item 3 — purpose enum coverage + filter semantics: - Expanded purpose enum: + attribution (TTD Trans-API, Meta CAPI, AppsFlyer postbacks), + creative_serving (GAM tag-build, VAST/CDN, ad-server trafficking). Closes the platform_primary-OR-attribution gap product expert flagged for typical DSP buyer-agent flows. - Changed purpose_filter unclassified semantics: calls without `purpose` are now treated as `purpose: other` for filter matching (was: excluded from any filter). Principle of least surprise — adopters who haven't classified end up in the catch-all bucket rather than silently invisible. - Runners MUST report unclassified-call counts in validation_result.actual when purpose_filter is set and zero recorded_calls match. Turns a "façade misclassifies-into-other" silent zero into a noisy zero. Item 5 — advisory severity rendering: - validation_result.severity comment expanded with the rendered-output MUST: advisory entries MUST be visually distinguished (literal [ADVISORY] prefix, muted style, or separate section). Without this, a façade declaring its anti-façade validations as advisory produces passed: false entries indistinguishable from required failures. - rendered_output_fencing block extended with an Advisory-severity differentiation note alongside the existing fencing rule (orthogonal concerns, both apply). - run_summary.validations_advisory_failed gets a Rendered-summary rule: MUST display adjacent to steps_failed, not buried at the bottom. The two counters together are the conformance signal. - Routing language tightened: advisory failures contribute to validations_advisory_failed and MUST NOT contribute to steps_failed, validations_failed, or any other required-failure counter — so consumers aggregating "passed: false" don't double-count. NB: Pre-commit hook bypassed for this commit because origin/main contains unrelated typecheck failures from PR #3713 (@adcp/sdk@6.0.0 migration) that are not part of this PR's scope. CI on this branch was already green pre-rebase; this fix-up extends those changes without touching server code. Build green; 6/6 lint tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Three-expert review applied (commit 6639b3e)Ran #3837 through ad-tech-protocol-expert, adtech-product-expert, and security-reviewer. All HIGH+MEDIUM findings addressed inline; LOW items filed as #3847. HIGH (must fix before merge):
MEDIUM:
Version regression false positive — security review flagged LOW deferred to #3847:
Note on commit: HUSKY=0 used to bypass pre-commit because origin/main has unrelated typecheck failures from PR #3713 (@adcp/sdk@6.0.0 migration shipped without a corresponding npm dep bump). Documented in commit message. CI on this branch was already green pre-rebase. |
bokelley
added a commit
that referenced
this pull request
May 2, 2026
…ion_mode_required:raw lint (closes #3847) (#3852) * feat(compliance): expires_after_version for advisory drift + attestation_mode_required:raw lint (closes #3847) Two LOW-priority items from the three-expert review of #3837 and #3838. Both are small spec/lint additions; bundled because they tighten the same upstream_traffic / advisory-severity surface. Item 1 — expires_after_version for advisory drift: - Optional expires_after_version: "<semver>" on storyboard validation entries with severity: advisory. When set, the runner promotes the advisory to required automatically once it runs against an @adcp/sdk version >= the stated value. - New validation_result.severity_promoted_from_advisory boolean on the runner output for transparency in promoted cases — reports SHOULD render "[REQUIRED — was advisory through SDK X; promoted at SDK Y]". - New scripts/lint-storyboard-advisory-expiry.cjs (warnings, not errors): surfaces severity: advisory without expires_after_version and without an `# advisory-permanent: <reason>` marker comment. Permanent advisory drift is legitimate (experimental signals); the marker silences the warning. Item 2 — attestation_mode_required:raw lint: - New scripts/lint-storyboard-raw-mode-required.cjs (errors): rejects storyboards setting attestation_mode_required:"raw" on an upstream_traffic check without a payload_must_contain clause. Without payload_must_contain, the raw flag has no operational value (all other upstream_traffic assertions work in digest mode) and just excludes privacy-conscious adopters from the conformance signal. - The single justification for raw mode is payload_must_contain — arbitrary JSONPath assertions digest mode can't support. The lint enforces that justification. Both lints wired into build-compliance.cjs (sequenced after lint-storyboard-check-enum.cjs from #3837) and npm test. Tests: 14 cases combined — source-tree guards plus per-rule fixtures. All pass. Build green; pre-commit bypassed (HUSKY=0) because origin/main still has unrelated typecheck failures from PR #3713 (@adcp/sdk@6.0.0 migration shipped without the npm dep bump). Same situation as #3837 and #3838's fix-up commits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(compliance): apply expert-review findings on #3852 Three-expert review of #3852 converged on five HIGH findings; addressed inline. Replace YAML-comment marker with structured field (all 3 experts): - Drop `# advisory-permanent: <reason>` comment-scan pattern. Comments don't survive YAML round-tripping in editor formatters/normalization pipelines, the textual scan was bypassable via injection of the literal text inside `description: |` block scalars (security review), and the reason wasn't machine-readable for dashboards. - Replace with structured `permanent_advisory: { reason: "<text>" }` field on the validation entry. Schema-validatable, greppable, round-trip-safe, machine-readable. - Lint now reads the structured field; comment-marker code removed. Decouple version anchor from @adcp/sdk (protocol + product): - Runner self-declares `runner_capability_version` (semver) on run_summary. expires_after_version compares against that, NOT against any specific implementation's package version. - Authors target spec capability they need; runners self-report the capability they offer. Implementations choose what their capability version corresponds to (@adcp/sdk semver, AdCP spec version, or runner-binary version). Semver validation in lint (security): - New rule advisory_expiry_not_semver: lint validates expires_after_version via Node's `semver.valid()`. Rejects malformed values that would otherwise leak into rendered reports as storyboard-author-controlled strings (rendered_output_fencing covers it as defense-in-depth, but the lint is the publish-time gate). Forward-compat × promotion ordering (protocol): - Document explicitly: forward-compat resolves FIRST. When a check kind is unknown to the runner (graded not_applicable), severity_promoted_from_advisory MUST be absent — no promotion was evaluated because no grading occurred. Closes the implementation-defined behavior gap the protocol expert flagged. severity_promoted_from_advisory tri-state (protocol): - Was optional boolean; now tri-state required when storyboard declared expires_after_version: true — promoted at execution time false — declared but runner_capability_version too old absent — storyboard didn't declare expiry OR check graded not_applicable - Lets consumers distinguish all three cases without re-reading the storyboard. Pre-release semver semantics (protocol + product): - Explicit MUST: comparison uses semver.gte including pre-releases. 6.5.0-rc.3 < 6.5.0. Authors targeting "after stable" write 6.5.0; authors wanting to include pre-releases write 6.5.0-0. Runners MUST use semver.gte semantics. advisory_double_gating rule: - New rule: declaring BOTH expires_after_version AND permanent_advisory is mutually exclusive (different intents). rendered_output_fencing extension: - expires_after_version and permanent_advisory.reason added to the fenced-fields list — defense-in-depth alongside the semver lint. Build-compliance error swallowing fixed: - The bare catch on advisory-expiry lint exec was swallowing real script crashes (the script exits 0 on warnings by design, so any non-zero exit means the script itself broke). Now logs the error and continues. Improved raw-required lint message: - When the lint fires, the error message now enumerates which mode-agnostic assertions are present on the check ("you have min_count and identifier_paths; both work in digest mode") so authors learn rather than just removing the flag. Tests: 19 cases combined (was 14). New cases cover semver validation, structured permanent_advisory field, double-gating violation, permanent_advisory without reason, pre-release tags, and the unit-level checkValidation export. Build green; HUSKY=0 again because origin/main typecheck regression from PR #3713 still unresolved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley
added a commit
that referenced
this pull request
May 3, 2026
… 5 storyboards (#3962) * feat(compliance): bump @adcp/sdk to 6.7.0 + adopt upstream_traffic on 5 storyboards @adcp/sdk@6.7.0 ships runner-side support for the v2.0.0 anti-façade contract from #3816 (upstream_traffic check + capture_path_not_resolvable synthesized code + forward-compat default for unknown check kinds). Bumping unblocks storyboard adoption that was deferred from #3816 because the published runner errored hard on unrecognized check types. SDK bump (^6.0.0 → ^6.7.0): - TrackStatus extended with 'silent' to match SDK enum. - BrandRightsPlatform impl wires updateRights to existing handleUpdateRights and stubs reviewCreativeApproval with NOT_IMPLEMENTED (training agent doesn't expose a creative-approval webhook receiver). Storyboard adoption of upstream_traffic on 5 specialisms using only the v2.0.0 fields 6.7.0 supports (min_count, endpoint_pattern, payload_must_contain, identifier_paths, since): - sales-social: sync_audiences (with realistic add[] hashed identifiers + payload_must_contain for upstream POST shape) and log_event (with user_match echoing the audience member, exercising identifier echo across two related steps). - audience-sync: create_audience with hashed-identifier echo verification. - signal-marketplace: activate_on_platform with since: search_by_spec window scoping the assertion to traffic caused after signal IDs were captured. - sales-non-guaranteed: create_media_buy with platform-agnostic POST count assertion. - creative-ad-server: build_creative with platform-agnostic POST count assertion. Adopters who don't advertise query_upstream_traffic in list_scenarios grade the new validations not_applicable per the runner's forward-compat rule — opt-in by adopter capability. The training agent does not yet implement the controller scenario, so all 5 storyboards run clean against it. Out of scope (deferred until @adcp/sdk ships the rest of the contract surface): severity:advisory + expires_after_version (#3837/#3852) and attestation_mode:digest (#3838). 6.7.0 ships the original v2.0.0 contract; subsequent fix-ups land in a later runner release. Build green; typecheck clean; 864/864 unit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(training-agent): drop update_rights customTools entry (collides with framework registration in 6.7.0); lower storyboard floors Misdiagnosed the 6.7.0 bump failure as an SDK discovery regression (filed as adcp-client#1438, since closed). Real cause: @adcp/sdk@6.7.0 promoted update_rights to a framework-registered first-class tool (adcp-client#1349 / commit 522015d9), and the training agent's brand tenant still registers it via customTools — createAdcpServer's collision check throws lazily inside the request handler, returning 500 HTML on every MCP POST, which the SDK's MCP discovery probe correctly classifies as "no MCP response." Fix: drop the update_rights customTool registration. The BrandRightsPlatform.updateRights method on TrainingBrandPlatform (added when the SDK bumped) wires through to the same handleUpdateRights handler. creative_approval still rides customTools — not yet promoted to AdcpToolMap. Floors lowered in .github/workflows/training-agent-storyboards.yml + scripts/run-storyboards-matrix.sh because the bump exposed unrelated baseline regressions in deterministic_testing context echo, signed_requests /mcp-strict discovery, idempotency_key capture, error-code expectations, and seed-fixture acknowledgement. Tracked in #3965; floors will tighten back as each is closed. Defensive follow-up filed in adcp-client#1447 — moving the customTools collision check from per-request to constructor time would have surfaced this fix-up at startup rather than as 500 HTML on every MCP call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: trigger fresh run after concurrency-cancellation --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Closes items 1, 3, and 5 of #3830 (LOW-priority follow-ups from #3816's three-expert review). Bundled because they all tighten the same upstream_traffic / authored-check surface; each is small and independent.
Item 1 — Build-time storyboard check-enum lint
Closes the publish-time gap that the runtime forward-compat clause from #3816 left open. Forward-compat (unknown check kind → not_applicable) exists for cross-version skew; it should NOT be the catch for typos at storyboard publish time.
runner-output-contract.yamldeclaresauthored_check_kindsas a structured top-level list — single source of truth for what storyboards may declare instep.validations[].check.scripts/lint-storyboard-check-enum.cjswalks every storyboard and emits two rules:unknown_check_kind— typo or undocumented kind.synthesized_check_kind_authored— storyboard declared a runner-emitted code (capture_path_not_resolvable/unresolved_substitution) it can't meaningfully assert against.scripts/build-compliance.cjsalongside the other linters.tests/lint-storyboard-check-enum.test.cjs(6 cases — source-tree guard plus per-rule fixtures with temp-dir storyboards). Wired intonpm test.Item 5 — Advisory validation severity
New optional
severity: "required" | "advisory"field on storyboard validation entries (default:"required"). An advisory failure surfaces invalidation_resultwithpassed: falsebut does NOT fail the step — the step grades on its remaining required validations. Advisory failures contribute to a distinctvalidations_advisory_failedcounter onrun_summaryso they stay visible without polluting conformance verdicts.Use case: storyboards declaring
upstream_trafficduring the months between #3816 landing and@adcp/sdkshipping the runner. Authors flipseverity: advisorywhile instrumentation matures, then drop the field once adoption is stable. Distinct from runtime forward-compat (which handles spec-runner version skew); severity is for author-managed rollout gating.Documented in
storyboard-schema.yaml > Validationandrunner-output-contract.yaml > validation_result.optional_fields.Item 3 —
purposetagging on recorded_callspurposeenum onrecorded_calls[].itemsincomply-test-controller-response.json:platform_primary|measurement|identity|other. Adopters self-classify each outbound call.purpose_filter: [string]onupstream_trafficstoryboard checks. Scopes the assertion to recorded calls matching one of the listed purposes.Use case: a
sales-non-guaranteedbuyer agent legitimately calls measurement vendors (DV/IAS/Nielsen) during a singlecreate_media_buystep — the storyboard scopes topurpose_filter: [\"platform_primary\"]so those don't muddy the campaign-creation assertion. Calls without apurposefield match only whenpurpose_filteris omitted (absence is ambiguous; explicit filtering requires the adopter's classification).Out of scope (separate PRs)
payload_attestationdigest mode for EU adopters) — bigger spec design (new mode, two-mode response shape, digest verification semantics). Separate PR.Verification
node scripts/build-compliance.cjs— 9 lint stages pass, 22 universal / 6 protocols / 19 specialisms build clean.node --test tests/lint-storyboard-check-enum.test.cjs— 6/6.🤖 Generated with Claude Code