Skip to content

spec(compliance): drop redundant assertion modules, use bundled 5.9 defaults (#2639)#2769

Merged
bokelley merged 5 commits intomainfrom
bokelley/drop-redundant-assertions
Apr 22, 2026
Merged

spec(compliance): drop redundant assertion modules, use bundled 5.9 defaults (#2639)#2769
bokelley merged 5 commits intomainfrom
bokelley/drop-redundant-assertions

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

@bokelley bokelley commented Apr 22, 2026

Summary

Closes the end state for #2639. @adcp/client@5.9.1 auto-registers three bundled cross-step assertion defaults (context.no_secret_echo, idempotency.conflict_no_payload_leak, governance.denial_blocks_mutation) on any @adcp/client/testing import, with coverage that matches or exceeds what this repo was shipping locally. Deletes the last local override (context.no_secret_echo) now that upstream fixes land.

Timeline

  • #2771 already deleted two of the three local assertion modules when the 5.9.0 defaults shipped, but kept a local stricter context.no_secret_echo because the 5.9.0 default was a no-op for structured auth options (tracked as adcp-client#751).
  • adcp-client#752 (5.9.1) walks structured auth discriminated-union objects to extract leaf secrets (bearer token, basic-auth password, OAuth access/refresh tokens, OAuth-CC client_id/client_secret, $ENV:VAR resolution) and adds registerAssertion(spec, { override: true }) for consumers who want stricter per-repo semantics.
  • adcp-client#753 (5.9.1) widens the default further: whole-body recursive walk (not just .context), bearer-token regex, suspect property names at any depth, options.test_kit.auth.api_key pickup.
  • This PR deletes the now-redundant local override and bumps the dep to ^5.9.1.

What changes

Deleted:

  • server/src/compliance/assertions/context-no-secret-echo.ts
  • server/src/compliance/assertions/index.ts

Side-effect imports removed from:

  • server/src/services/storyboards.ts
  • server/tests/manual/run-storyboards.ts
  • server/tests/manual/run-one-storyboard.ts
  • server/tests/manual/storyboard-smoke.ts

Other:

  • .gitignore — drops the now-dead /dist/compliance/assertions/ exclusion (no sources → no tsc output → no shadowing of the compliance-tarball path).
  • static/compliance/source/universal/idempotency.yaml — comment refreshed to point at the bundled 5.9+ defaults and call out the registerAssertion(spec, { override: true }) escape hatch.
  • package.json@adcp/client bumped ^5.9.0^5.9.1.

Net diff: 202 lines deleted, ~14 added.

Coverage equivalence (what the SDK default now does vs the deleted local)

The SDK 5.9.1 context.no_secret_echo is a strict superset:

Feature Deleted local SDK 5.9.1
Bearer-token literal regex ✓ (identical)
Suspect property names at any depth (authorization, api_key, apikey, bearer, x-api-key) ✓ (identical)
Whole-body recursive walk
options.test_kit.auth.api_key
options.auth_token / .auth / .secrets[]
Structured auth object walk (discriminated union — bearer/basic/oauth/oauth_cc)
$ENV:VAR resolution to runtime value
SECRET_MIN_LENGTH for verbatim-value matching 8 16

The one scope shift worth flagging: the SDK floor moved from 8 → 16 chars for verbatim secret-value matching (the deliberate FP-reduction rationale is in the SDK comments). Real tokens clear 16 trivially; the property-name check is the primary gate for short fixture values. Consumers staging sub-16-char credentials can re-register a stricter version via registerAssertion(spec, { override: true }).

Expert review

  • Code review: "ship it" — deletion clean, import chain intact, 5.9.1 auto-registration verified at runtime, no orphaned references.
  • Security review: "safe to ship" — SDK 5.9.1 is strict superset of deleted coverage; the 16-char floor is the right FP/coverage tradeoff given real-world credential shapes; no residual attack surface.

Verification

  • node -e "console.log(require('@adcp/client/testing').listAssertions())" → all three default ids present on a bare import.
  • npx tsc --project server/tsconfig.json --noEmit — clean.
  • npx vitest run server/tests/unit/storyboards.test.ts server/tests/unit/collection-lists-storyboard.test.ts — 15/15 passing.
  • Full CI: 13/14 pass + 1 skipped Mintlify. Storyboards (legacy + framework dispatch) both pass.

#2639 final topology

  • Spec (adcp) owns invariant ids and YAML invariants: [...] wiring on specialisms.
  • SDK (@adcp/client) owns the registry, runner hooks, default-invariants implementations (auto-registered on /testing import), and the { override: true } escape hatch.
  • adcp repo no longer needs any local assertion code — just declares invariants: [...] on storyboards.

Follow-ups (tracked separately)

🤖 Generated with Claude Code

…efaults (#2639)

@adcp/client@5.9 bundles and auto-registers the three cross-step assertions
this repo was shipping as local modules:

- idempotency.conflict_no_payload_leak
- context.no_secret_echo
- governance.denial_blocks_mutation

They register on `@adcp/client/testing` import via the upstream
default-invariants side effect, so every runStoryboard / comply() call
resolves them without explicit loading.

Delete the local modules, their test, the side-effect imports that were
loading them, and the .gitignore entry that kept the tsc output out of
the published spec-tarball path. Bump @adcp/client to ^5.9.0. Update the
comment on universal/idempotency.yaml to point at the bundled defaults
instead of the old "import the module first" instruction.

Closes #2665 (in-band default registration was the chosen path over a
sibling @adcp/compliance-assertions package). Completes the end state for
#2639 — spec owns the invariant ids and YAML wiring; SDK owns the
implementations, registry, and auto-registration.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Original note flagged the bundled 5.9 defaults as narrower; upstream #753
restores allowlist-based conflict checking and whole-body secret scanning
before this merges, so the narrower-than-local state is transient.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley added a commit to adcontextprotocol/adcp-client that referenced this pull request Apr 22, 2026
Security review on adcontextprotocol/adcp#2769 flagged both
idempotency.conflict_no_payload_leak and context.no_secret_echo as
shipping materially narrower semantics than the local versions they
replace. Widens both to restore coverage before #2769 merges.

idempotency.conflict_no_payload_leak:
  Before: denylist of 5 field names (payload, stored_payload,
  request_body, original_request, original_response). Trivially
  bypassed by a seller inlining budget / product_id / account_id at
  the adcp_error root, which turns key-reuse into a read oracle.
  After: allowlist of 7 envelope keys (code, message, status,
  retry_after, correlation_id, request_id, operation_id). Any other
  top-level property on the adcp_error envelope fails with a sorted
  list of leaked field names.

context.no_secret_echo:
  Before: scanned only response.context; missed credentials echoed
  into error.message, audit fields, debug blocks.
  After:
    - Full-body recursive walk (not just .context)
    - Bearer-token literal regex /\bbearer\s+[A-Za-z0-9._~+/=-]{10,}/i
    - Suspect property name match at any depth: authorization,
      api_key, apikey, bearer, x-api-key (case-insensitive)
    - Pick up options.test_kit.auth.api_key as a verbatim-secret
      source alongside auth_token / auth / secrets[]
    - Reuses #752's extractAuthSecrets for the structured auth union,
      including $ENV:VAR resolution via pushCredentialValue
    - Centralises the SECRET_MIN_LENGTH (16) guard in addIfSecret so
      every source gets the same floor

governance.denial_blocks_mutation is untouched.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley added a commit to adcontextprotocol/adcp-client that referenced this pull request Apr 22, 2026
…753)

Security review on adcontextprotocol/adcp#2769 flagged both
idempotency.conflict_no_payload_leak and context.no_secret_echo as
shipping materially narrower semantics than the local versions they
replace. Widens both to restore coverage before #2769 merges.

idempotency.conflict_no_payload_leak:
  Before: denylist of 5 field names (payload, stored_payload,
  request_body, original_request, original_response). Trivially
  bypassed by a seller inlining budget / product_id / account_id at
  the adcp_error root, which turns key-reuse into a read oracle.
  After: allowlist of 7 envelope keys (code, message, status,
  retry_after, correlation_id, request_id, operation_id). Any other
  top-level property on the adcp_error envelope fails with a sorted
  list of leaked field names.

context.no_secret_echo:
  Before: scanned only response.context; missed credentials echoed
  into error.message, audit fields, debug blocks.
  After:
    - Full-body recursive walk (not just .context)
    - Bearer-token literal regex /\bbearer\s+[A-Za-z0-9._~+/=-]{10,}/i
    - Suspect property name match at any depth: authorization,
      api_key, apikey, bearer, x-api-key (case-insensitive)
    - Pick up options.test_kit.auth.api_key as a verbatim-secret
      source alongside auth_token / auth / secrets[]
    - Reuses #752's extractAuthSecrets for the structured auth union,
      including $ENV:VAR resolution via pushCredentialValue
    - Centralises the SECRET_MIN_LENGTH (16) guard in addIfSecret so
      every source gets the same floor

governance.denial_blocks_mutation is untouched.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley and others added 3 commits April 21, 2026 21:54
…nt-assertions

# Conflicts:
#	server/src/compliance/assertions/context-no-secret-echo.ts
#	server/src/compliance/assertions/index.ts
#	server/src/services/storyboards.ts
#	server/tests/manual/run-one-storyboard.ts
#	server/tests/manual/run-storyboards.ts
#	server/tests/manual/storyboard-smoke.ts
…1 walks structured auth (#2639)

#2771 added server/src/compliance/assertions/context-no-secret-echo.ts
as a stricter override of the SDK default while upstream was a no-op
for structured `auth` options (adcp-client#751). That's fixed in
@adcp/client 5.9.1 (#752 walks structured auth + #753 widens to full-
body scan + suspect-property-name match + bearer-token regex). The
local override's coverage is now a subset of the SDK default.

- Delete server/src/compliance/assertions/ — 5.9.1 auto-registers the
  three cross-step defaults on any @adcp/client/testing import.
- Drop the side-effect imports in services/storyboards.ts and the three
  manual-test runner scripts.
- Drop the /dist/compliance/assertions/ .gitignore entry that only
  existed while tsc output was shadowing the compliance-tarball path.
- Bump @adcp/client to ^5.9.1.
- Refresh universal/idempotency.yaml comment to point at the bundled
  defaults.

Consumers who need stricter per-repo checks can override via the new
registerAssertion(spec, { override: true }) option (adcp-client#752).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cape hint

- Changeset: acknowledge the SDK default's 16-char verbatim-secret floor
  (vs local's 8), called out by security-review as the one residual scope
  shift. Real credentials clear 16; the property-name check is the
  primary gate for short fixture values. Consumers can override per #752.
- universal/idempotency.yaml comment: mention the
  `registerAssertion(spec, { override: true })` escape hatch so downstream
  implementers know where to reach for stricter per-repo checks.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley bokelley merged commit c507b9b into main Apr 22, 2026
13 checks passed
bokelley added a commit that referenced this pull request Apr 22, 2026
…l-asset templates)

Folds origin/main into the release-docs branch. Adds:

- Item 18: asset_type discriminator (#2776), refine[] prefixed ids + optional action (#2775), SI context→intent rename (#2774), governance plan scoping contract (#2777).
- Item 19: url-asset url.format relaxed from uri to uri-template (#2801) so AdCP macros like {SKU}/{DEVICE_ID}/{MEDIA_BUY_ID} validate at sync time.
- Breaking-changes table: asset_type const, refine[] product_id/proposal_id, SI context→intent, url-asset uri-template.
- Migration notes: asset_type on payloads, refine rename, SI field rename, url-asset no-op guidance.

Local compliance-assertion modules superseded by bundled @adcp/client 5.9 defaults (#2769) — accepted the upstream delete on merge.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

1 participant