Skip to content

fix(invariant): no_secret_echo only fails on string-valued suspect-named fields (#1713)#1714

Merged
bokelley merged 1 commit into
mainfrom
bokelley/issue-1713-no-secret-echo
May 12, 2026
Merged

fix(invariant): no_secret_echo only fails on string-valued suspect-named fields (#1713)#1714
bokelley merged 1 commit into
mainfrom
bokelley/issue-1713-no-secret-echo

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Closes #1713.

The actual BidMachine root cause

Investigating #1707/#1711 surfaced something unexpected: BidMachine's sync_accounts context.no_secret_echo failures aren't caused by Zod .strict() codegen lag. I verified published @adcp/sdk tarballs at 5.25.1, 6.12.0, and current main — every one uses .passthrough() on SyncAccountsResponseSchema.accounts[]. Zod silently accepts the authorization field.

The rejection happens inside context.no_secret_echo's name dragnet at src/lib/testing/storyboard/default-invariants.ts:184-190:

if (v !== null && typeof v === 'object') {
  for (const [key, inner] of Object.entries(v as Record<string, unknown>)) {
    if (SUSPECT_PROPERTY_NAMES.has(key.toLowerCase())) {
      return `contains suspect property name "${key}"`;
    }
    stack.push(inner);
  }
}

The dragnet trips on field NAME alone (authorization, api_key, apikey, bearer, x-api-key) regardless of value. BidMachine's response carries authorization with a structured object value (per the spec — compliance/cache/3.0.11/property/validation-result.json declares authorization as a structured authorization-validation payload, and the additionalProperties: true on sync_accounts_response.accounts[] permits seller extensions). The structured value isn't a credential leak; the dragnet false-positives.

The fix

findSecretEcho now requires both a suspect name AND a non-empty string value:

if (
  SUSPECT_PROPERTY_NAMES.has(key.toLowerCase()) &&
  typeof inner === 'string' &&
  inner.length > 0
) {
  return `contains suspect property name "${key}" with string value`;
}
stack.push(inner);

Object/array values on suspect-named fields pass through to the recursive walk, which still scans nested strings via BEARER_TOKEN_PATTERN and caller-supplied secret literals. The actual leak shapes (bearer tokens at any depth, caller secret echoes, bare credential strings on suspect-named fields) all stay caught.

Tests

Five new cases in describe('default-invariants: context.no_secret_echo (widened whole-body scan)'):

# Case Result
1 Suspect name (authorization) carrying an OBJECT value — the BidMachine case passes ✓
2 Suspect name (api_key) carrying an ARRAY value passes ✓
3 Suspect name carrying an empty string passes ✓
4 Suspect name carrying a non-empty string — existing dragnet preserved fails ✓
5 Bearer literal nested INSIDE a structured suspect-named object fails (regression guard) ✓

113/113 invariant tests pass.

Why this matters for the coordinated stance

  • adcp-client#1709 (merged via fix(runner): attribute Zod schema rejects to response_schema (#1709) #1712) addresses Zod-reject misattribution (different path — when Zod DOES reject, attribute to response_schema).
  • adcp-client#1707 — dynamic schema fetch + strict→passthrough + codegen regen. Real architectural cleanup but does not unblock BidMachine (verified the published SDK already uses passthrough).
  • adcp-client#1711 (fgranata) — BidMachine's report. Their "Zod .strict() codegen lag" diagnosis was incorrect for the published SDK; this PR is the actual unblocker for the 27-point delta (63/128 comply vs 45/59 CLI). Will close once fgranata retests.

So the order BidMachine cares about:

  1. fix(invariant): no_secret_echo only fails on string-valued suspect-named fields (#1713) #1714 (this PR) — closes BidMachine's immediate symptom.
  2. Schema URLs and Zod validators are baked at SDK build time — drift from agent's advertised adcp_version #1707 — independent architectural cleanup; lower urgency now.

Test plan

  • npm run build clean
  • npm run format:check clean
  • node --test test/lib/storyboard-default-invariants.test.js — 113/113 pass (5 new + all existing)
  • After merge, BidMachine retest should show sync_accounts failures resolved.

Coordinated stance: #1685 (the SDK is a witness, not a translator). Same anti-pattern as #1702 / #1705 / #1712: the SDK fabricated a credential-leak signal against a structured non-credential field the spec legally permits.

🤖 Generated with Claude Code

…med fields (#1713)

The default context.no_secret_echo invariant flagged any response field
whose name was in SUSPECT_PROPERTY_NAMES (authorization, api_key,
apikey, bearer, x-api-key) regardless of the field's value. This
over-rejected spec-legitimate structured fields — notably the
authorization object on validation-result.json (structured authorization-
validation payload, not a credential) and any seller-side extension
fields named authorization under sync_accounts_response.accounts[]
(additionalProperties: true).

Symptom: BidMachine's sync_accounts failures in
adcontextprotocol/adcp#4419 all surfaced as context.no_secret_echo.
Diagnosis in #4419 pointed at Zod .strict() codegen lag, but
verification of published SDK tarballs (5.25.1 / 6.12.0 / current)
shows SyncAccountsResponseSchema.accounts[] already uses .passthrough().
Zod silently accepts the authorization field; the NAME dragnet in
this invariant is the actual rejection.

Fix: narrow findSecretEcho so the suspect-name check only fires when
the field VALUE is a non-empty string. Structured object/array values
on suspect-named fields pass through to the recursive walk, which still
scans nested strings against BEARER_TOKEN_PATTERN and caller-supplied
secret literals. The actual leak shapes the invariant was designed to
catch remain caught.

Five new test cases:
- Suspect name with object value passes (the BidMachine case)
- Suspect name with array value passes
- Suspect name with empty string passes
- Suspect name with non-empty string value still fails
- Bearer literal nested inside structured suspect-named object still
  fails via the value-scan regression guard

113/113 invariant tests pass.

Sequencing:
- #1709 (merged via #1712) addresses Zod-reject misattribution
  (different path); this fix addresses the case where Zod ACCEPTS but
  the invariant over-rejects on field name.
- #1707 (dynamic schema fetch + strict→passthrough) remains real
  architectural cleanup but does NOT unblock BidMachine.
- #1711 (fgranata): the "Zod .strict() codegen lag" diagnosis was
  incorrect for the published SDK; this fix is the actual unblocker.

Coordinated stance: #1685 (the SDK is a witness, not a translator).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley bokelley merged commit 110b49e into main May 12, 2026
10 checks passed
@bokelley bokelley deleted the bokelley/issue-1713-no-secret-echo branch May 12, 2026 11:55
@fgranata
Copy link
Copy Markdown

Following up on your guinea-pig invitation. Ran our prod against @adcp/sdk@7.1.0 (the version with #1714's findSecretEcho fix) using npx -y -p '@adcp/sdk@7.1.0' adcp storyboard run bm-prod ... since @adcp/client@latest is still on the 5.25.1 compat shim.

Empirically confirms your analysis. Two probes against https://adcp.bidmachine.io/adcp/mcp:

Storyboard Result
media_buy_seller (+ 10 sub-storyboards) 47 pass / 3 fail / 9 skip
sales_non_guaranteed 26 pass / 3 fail / 8 skip

account_setup is cleared — not in the failure list with @adcp/sdk@7.1.0. Score should jump once your grading infra picks up 7.1.0. Will you re-run automatically, or should we wait?

Three remaining failures we captured locally (each independent of #1714):

  1. media_buy_seller/delivery_reporting/simulate_delivery — our comply_test_controller returns UNKNOWN_SCENARIO (we hadn't wired this scenario). Shipping the dispatch now.
  2. media_buy_seller/inventory_list_targeting/get_products_brief — our sandbox product emits channels: ["mobile_app"] which isn't in the canonical MediaChannel enum. One-line fix to ["display"].
  3. sales_non_guaranteed/create_media_buy — our handler reads bid_price only from packages[0]; multi-package case silently drops packages[1]. Refactoring handler + mapper.

Shipping all three this afternoon. When grading retests against 7.1.0, would love a per-storyboard failure-list dump so we can diff against this local capture. Thanks again — the #1714 diagnostic trail made triangulation easy.

bokelley added a commit that referenced this pull request May 12, 2026
…#1708) (#1715)

Locks the post-7.1.0 attribution invariants so future refactors of
comply()'s extractFailures can't silently reintroduce the BidMachine
misattribution shape (adcp#4419).

What's locked:

1. A storyboard step carrying both a synthesized response_schema failure
   (prepended by the runner per #1709 / PR #1712) and an assertion entry
   surfaces validation.check === 'response_schema' in
   ComplianceResult.failures — never 'assertion'. The attribution that
   was silently broken pre-7.1.0 (Zod rejects fell through to the next
   invariant, canonically context.no_secret_echo).

2. Skipped-invariant markers (passed: true entries the runner emits when
   short-circuiting invariants downstream of a schema failure per #1712)
   are correctly filtered out — only failed validations surface in
   `failures`. A future change that included passed: true entries would
   crowd out the real failure.

3. A clean BidMachine-shape response (structured authorization field
   passing no_secret_echo per #1713 / PR #1714) produces zero failures
   through the aggregation layer.

4. Multi-storyboard aggregation preserves per-storyboard
   (storyboard_id, step_id, validation.check) tuples.

5/6. Clean pass paths (no failures, skipped steps) produce empty failures.

API change (minor): extractFailures (previously file-internal) is now
exported from src/lib/testing/compliance/comply.ts so the regression
test can call it directly with synthetic StoryboardResult fixtures.
Functionally identical; just visibility.

Scope correction relative to the original #1708 framing: the
"cross-evaluator divergence" symptom was version-driven (different
@adcp/sdk versions hitting #1713 and #1709 differently), not a true
parity gap. Both root causes shipped in 7.1.0; this test is the
durable guard for the aggregation-layer invariants those fixes
depend on.

7/7 tests pass.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot mentioned this pull request May 12, 2026
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.

no_secret_echo invariant flags spec-legitimate field names on structured-value fields

2 participants