Conversation
bokelley
added a commit
that referenced
this pull request
May 2, 2026
Three-expert review of #3838 converged on tightening the digest mode spec before merge. Addressed inline. oneOf discriminator hardening: - attestation_mode is now required at the recorded_calls[].items level (not buried inside oneOf branches). Hand-written controllers will fail validation cleanly with "missing attestation_mode" rather than hitting ambiguous oneOf-no-branch-matched errors. - attestation_mode uses `const` (not `enum` of one) on each branch so the discriminator dispatch is deterministic. - payload_length now required at the items level too — symmetric across modes so runners can detect adopter-side truncation regardless of attestation choice. Canonicalization gotchas pinned (RFC 8785 / JCS): - Redaction-vs-digest order is now normative: secret-key redaction MUST precede canonicalization MUST precede digest computation. Both payload_digest_sha256 and identifier_match_proofs MUST be computed over the SAME post-redaction canonical bytes — diverging the two surfaces breaks coherence between digest replay and identifier echo. - NaN/Infinity payloads grade not_applicable for digest mode (RFC 8785 forbids them). - Numeric IDs MUST serialize as JSON strings before digest computation (adtech bid payloads carry IDs outside ±2^53 where I-JSON / RFC 7493 number round-tripping diverges across implementations). - Non-JSON content types (form-urlencoded, multipart, plain text) digest the post-redaction raw bytes; identifier_match_proofs MUST be empty for these (token boundaries are not portably defined). Tokenization for identifier_match_proofs (security finding): - Normative for application/json and *+json: controllers MUST scan exactly the JSON string-typed leaf values of the post-redaction canonical body — no substring matching, no word splitting, no case folding, no Unicode normalization. Closes the cross-implementation divergence the security reviewer flagged (adopter A and adopter B would otherwise tokenize differently and produce flakey proofs). - Non-JSON content types: identifier_match_proofs MUST be empty; runner-side identifier_paths assertions targeting those calls grade not_applicable. Synthetic-vectors-only reaffirmed (security finding): - Top-level UpstreamTrafficSuccess description now states that the synthetic-vectors-only requirement applies to digest mode as well as raw, with explicit existence-oracle threat naming. Digest mode reduces what the runner sees, but it doesn't enable testing against production data — a runner with a precomputed digest set would otherwise learn membership of arbitrary identifiers in the adopter's user base. - Request-side attestation_mode description drops the EU/GDPR framing the product expert flagged as legally optimistic; replaces with neutral language ("whether digest mode satisfies a given adopter's data-handling obligations is for that adopter's counsel to determine"). Bounds and trust framing: - identifier_match_proofs gets maxItems: 64 to match request-side cap. - identifier_match_proofs description now states explicitly that "SHA-256 is a privacy mechanism here, not a trust mechanism" and that consumers MUST NOT treat digest-mode passing as cryptographically more trustworthy than raw mode — lifted from the changeset trust-model paragraph into the schema where SDK code-gen consumers see it. NB: HUSKY=0 used to bypass pre-commit because origin/main contains unrelated typecheck failures from PR #3713 (@adcp/sdk@6.0.0 migration shipped without bumping the npm dep) — same situation as the matching fix-up commit on bokelley/check-enum-lint. CI on this branch was already green pre-rebase. Build green; 7/7 schema validation tests; 36/36 example validation tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Three-expert review applied (commit 9c2239c)Ran #3838 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): oneOf discriminator hardening (protocol-expert):
Canonicalization gotchas pinned (protocol+security):
Tokenization for
Synthetic-vectors-only reaffirmed (security):
MEDIUM:
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. |
…traffic (#3830 item 2) The fifth and final LOW-priority item from #3830. Adds opt-in digest mode so adopters under privacy/data-residency obligations (EU GDPR processors, US sandboxes processing production hashed PII) can support upstream_traffic conformance without returning raw outbound payloads to the runner. Privacy boundary: plaintext identifiers never reach the controller (runner sends SHA-256 digests in identifier_value_digests); plaintext payloads never reach the runner in digest mode (controller emits payload_digest_sha256 + identifier_match_proofs[] booleans). Both directions of the controller↔runner boundary stay closed; only SHA-256 digests and presence booleans cross. comply-test-controller-request.json: - attestation_mode: "raw" | "digest" param (default raw). - identifier_value_digests: array of SHA-256 hex (max 64) — runner sends digests of identifier values it wants echo-verified. comply-test-controller-response.json: - attestation_mode field on recorded_calls[] (echoes request; adopters MAY unilaterally downgrade raw→digest per call). - payload_digest_sha256 (RFC 8785 JCS for JSON, raw bytes otherwise). - payload_length (required in digest mode for truncation detection). - identifier_match_proofs[]: per-digest { identifier_value_sha256, found }. - oneOf discriminator on items: RawAttestation vs DigestAttestation. Mixed-mode responses valid (per-call attestation choice). storyboard-schema.yaml: - attestation_mode_required: "raw" on upstream_traffic check (optional; excludes digest-mode adopters when set — use sparingly). - Digest-mode behavior documented: * payload_must_contain arbitrary paths → not_applicable per call * identifier_paths → supported via identifier_match_proofs[] * min_count / endpoint_pattern / purpose_filter → unchanged runner-output-contract.yaml: - upstream_traffic_digest_mode notes block documents per-mode grading, mixed-mode partial coverage, attestation_mode_required escape hatch. Trust model unchanged from #3816: raises bar against unintentional façades, not adversarial. identifier_match_proofs[] is self-reported. Out of scope: runner implementation (adcp-client#1253), adopter recorder middleware (adcp-client#1290 / adcp-client-python#347). Build green; 7/7 schema validation tests; 36/36 example validation tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three-expert review of #3838 converged on tightening the digest mode spec before merge. Addressed inline. oneOf discriminator hardening: - attestation_mode is now required at the recorded_calls[].items level (not buried inside oneOf branches). Hand-written controllers will fail validation cleanly with "missing attestation_mode" rather than hitting ambiguous oneOf-no-branch-matched errors. - attestation_mode uses `const` (not `enum` of one) on each branch so the discriminator dispatch is deterministic. - payload_length now required at the items level too — symmetric across modes so runners can detect adopter-side truncation regardless of attestation choice. Canonicalization gotchas pinned (RFC 8785 / JCS): - Redaction-vs-digest order is now normative: secret-key redaction MUST precede canonicalization MUST precede digest computation. Both payload_digest_sha256 and identifier_match_proofs MUST be computed over the SAME post-redaction canonical bytes — diverging the two surfaces breaks coherence between digest replay and identifier echo. - NaN/Infinity payloads grade not_applicable for digest mode (RFC 8785 forbids them). - Numeric IDs MUST serialize as JSON strings before digest computation (adtech bid payloads carry IDs outside ±2^53 where I-JSON / RFC 7493 number round-tripping diverges across implementations). - Non-JSON content types (form-urlencoded, multipart, plain text) digest the post-redaction raw bytes; identifier_match_proofs MUST be empty for these (token boundaries are not portably defined). Tokenization for identifier_match_proofs (security finding): - Normative for application/json and *+json: controllers MUST scan exactly the JSON string-typed leaf values of the post-redaction canonical body — no substring matching, no word splitting, no case folding, no Unicode normalization. Closes the cross-implementation divergence the security reviewer flagged (adopter A and adopter B would otherwise tokenize differently and produce flakey proofs). - Non-JSON content types: identifier_match_proofs MUST be empty; runner-side identifier_paths assertions targeting those calls grade not_applicable. Synthetic-vectors-only reaffirmed (security finding): - Top-level UpstreamTrafficSuccess description now states that the synthetic-vectors-only requirement applies to digest mode as well as raw, with explicit existence-oracle threat naming. Digest mode reduces what the runner sees, but it doesn't enable testing against production data — a runner with a precomputed digest set would otherwise learn membership of arbitrary identifiers in the adopter's user base. - Request-side attestation_mode description drops the EU/GDPR framing the product expert flagged as legally optimistic; replaces with neutral language ("whether digest mode satisfies a given adopter's data-handling obligations is for that adopter's counsel to determine"). Bounds and trust framing: - identifier_match_proofs gets maxItems: 64 to match request-side cap. - identifier_match_proofs description now states explicitly that "SHA-256 is a privacy mechanism here, not a trust mechanism" and that consumers MUST NOT treat digest-mode passing as cryptographically more trustworthy than raw mode — lifted from the changeset trust-model paragraph into the schema where SDK code-gen consumers see it. NB: HUSKY=0 used to bypass pre-commit because origin/main contains unrelated typecheck failures from PR #3713 (@adcp/sdk@6.0.0 migration shipped without bumping the npm dep) — same situation as the matching fix-up commit on bokelley/check-enum-lint. CI on this branch was already green pre-rebase. Build green; 7/7 schema validation tests; 36/36 example validation tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
9c2239c to
342651a
Compare
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 item 2 of #3830 — the last LOW-priority item from #3816's expert review. (Items 1, 3, 5 in #3837; item 4 in adcp-client#1290 + adcp-client-python#347.)
Why
#3816's product-expert review flagged a binary choice in the original `upstream_traffic` contract: adopters either expose raw payload contents through `query_upstream_traffic` (necessary for `payload_must_contain` and `identifier_paths` echo verification) or grade `not_applicable` and lose the anti-façade signal entirely.
EU adopters under GDPR processor obligations and US adopters whose sandboxes process production hashed PII can't legally return raw payloads to a runner buffer, even for sandbox diagnostics.
Digest mode preserves the load-bearing `identifier_paths` echo verification while keeping plaintext payloads inside the controller.
Privacy boundary
Plaintext identifiers never reach the controller — runner sends SHA-256 digests in `identifier_value_digests`. Plaintext payloads never reach the runner in digest mode — controller emits `payload_digest_sha256` + `identifier_match_proofs[]` booleans instead. Both directions of the controller↔runner boundary stay closed for raw payload data and identifier values; the only crossing artifacts are SHA-256 digests and presence booleans.
Schema additions
`comply-test-controller-request.json`:
`comply-test-controller-response.json`:
`storyboard-schema.yaml`:
`runner-output-contract.yaml`:
Trust model
Unchanged from #3816 — this raises the bar against UNINTENTIONAL façades, not adversarial ones. `identifier_match_proofs[]` is self-reported by the controller; a determined façade can return `found: true` for any digest. Spec consumers MUST NOT treat digest-mode passing as cryptographic proof of adapter behavior, same as raw mode. Documented in the existing `upstream_traffic_threat_model` block.
Out of scope (separate work)
Verification
Note on PR ordering
This PR and #3837 (items 1, 3, 5) both modify `recorded_calls[].items`. Whichever lands second will need a small rebase to add the missing fields from the other (this PR adds `attestation_mode` / digest fields; #3837 adds `purpose`). Rebases are mechanical; no semantic conflict.
🤖 Generated with Claude Code