Skip to content

feat: add transport diagnostics hook#2274

Merged
bokelley merged 2 commits into
mainfrom
issue-2272
Jun 25, 2026
Merged

feat: add transport diagnostics hook#2274
bokelley merged 2 commits into
mainfrom
issue-2272

Conversation

@bokelley

@bokelley bokelley commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes #2272.

Adds a public onTransportActivity diagnostics hook for outbound MCP and A2A transport requests. Events include request/response/failure phases, HTTP status, duration, method, sanitized URL, safe headers, bounded redacted body snippets, and operation/task/context correlation fields.

The hook is wired through ADCPMultiAgentClient, task execution, governance checks, cached MCP calls, native tasks/list, and A2A calls while preserving the official protocol SDK clients.

Also updates undici from 6.25.0 to 6.27.0 so the PR passes the repo high-severity npm audit gate.

Safety

  • Redacts sensitive headers including auth, cookies, API keys, signatures, and MCP session IDs.
  • Redacts body secrets, idempotency keys, private keys, and URL query/userinfo data.
  • Avoids reading binary/SSE bodies and bounds response snippets.
  • Async handlers are awaited by the diagnostics scope without blocking network start or surfacing handler failures to protocol calls.

Validation

  • npm run build:lib
  • NODE_ENV=test node --test-timeout=60000 --test-force-exit --test test/transport-diagnostics-e2e.test.js test/lib/transport-diagnostics.test.js test/server-a2a-submitted-end-to-end.test.js test/lib/mcp-oauth-connection-cache.test.js
  • npm run test:all
  • npm audit --audit-level=high
  • npx commitlint --from origin/main --to HEAD --verbose

@bokelley bokelley enabled auto-merge (squash) June 25, 2026 15:57

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean observability layer that holds the witness-not-translator line — reads a response.clone() and returns the original byte-for-byte, so the diagnostics path can never reshape what the protocol caller sees. Opt-in (no-op when onTransportActivity is unset), correctly composed under the official MCP/A2A SDK fetch slots.

Things I checked

  • Witness invariant holds. wrapFetchWithTransportDiagnostics returns upstream(input, init)'s response untouched; the only read is of response.clone() in responseBodySnippet. No fabricated fields, no normalization of the returned payload. ad-tech-protocol-expert: sound. javascript-protocol-expert: sound-with-caveats.
  • SSE stays intact. isDiagnosticTextContentType returns false for text/event-stream, so responseBodySnippet bails before .clone() — MCP StreamableHTTP streams are never teed or drained. Same SSE-skip discipline as rawResponseCapture.ts / responseSizeLimit.ts.
  • Composition order is right. In mcp.ts:543 and 912, signing wraps diagnosticFetch as upstream, so diagnostics observes the fully-signed request. That makes redaction of Signature / Signature-Input load-bearing — and SENSITIVE_KEY_RE catches both (signature at ^…- boundary). mcp-session-id redacted on request and response paths.
  • Header model is allowlist-only. Only SAFE_HEADER_NAMES + correlation headers are emitted; everything else is dropped or redacted. authorization/cookie/set-cookie/x-api-key/x-adcp-auth/proxy-authorization cannot slip through.
  • listMCPTasks 3→4 args — sole non-test caller TaskExecutor.ts:1353 updated, new options param defaults to {}. No broken call sites.
  • ALS plumbingemitTransportActivity pushes the handler promise onto slot.pending synchronously during the fetch; withTransportDiagnostics's finally awaits Promise.allSettled(slot.pending). Handler rejections are swallowed (.then(noop, noop)) so an observability hook can't alter protocol behavior.
  • Changeset .changeset/clean-taxis-observe.md is minor — correct. Purely additive: new exports (sanitizeTransportHeaders, sanitizeTransportUrl, TransportActivity* types), one optional config field. No wire change, no breaking SDK surface.

Follow-ups (non-blocking — file as issues)

  • Redaction is a weaker fork of redactCredentialPatterns. security-reviewer flagged it and it checks out: src/lib/server/redact.ts:85 rule #5 is an unlabeled ≥32-char token catch-all that sanitizeDiagnosticText dropped. Net effect — a secret value under a benign JSON key ({"value":"sk_live_…"}, a PEM block, a bare JWT) or a raw token interpolated into an error string survives, because the JSON path only redacts on key-name match and the text path only catches Bearer/Basic/URL/labeled-pair shapes. Canonical credential names are redacted, so this isn't a leak of the common case — but before these events feed any sink a partially-trusted operator can read, port rule #5 (plus PEM -----BEGIN/JWT patterns) into sanitizeDiagnosticText and apply it on the JSON-value path. Reuse redactCredentialPatterns directly so the two redactors don't drift.
  • Two missing regression tests for the headline-risk paths: (a) a text/event-stream response asserting no body read + original stream still consumable, and (b) a >64KB streaming body hitting the reader.cancel() truncation branch while the SDK still drains the original. Both code-reviewer and javascript-protocol-expert asked for these. A direct assertion that Signature/Signature-Input headers redact would pin the load-bearing regex against future SENSITIVE_KEY_RE edits.

Minor nits (non-blocking)

  1. Empty content-type defaults to text. isDiagnosticTextContentType returns true on !lower (transportDiagnostics.ts), so a bodied response with no content-type gets cloned and read. Bounded to 64KB and a conformant MCP server always labels SSE, so it won't hit streaming — but a one-line comment on the intent, or flipping the unknown default to skip, is the safer posture for a transport observer.
  2. Static HMAC salt. fingerprintDiagnosticValue keys on the hardcoded 'adcp-transport-diagnostics'. The raw idempotency_key never leaves the process (only idempotencyKeyHash), which is fine — idempotency keys aren't credentials. But a per-process random salt would remove the cross-deployment correlation oracle at zero cost to in-process correlation. Worth it only if these events ever reach a shared multi-tenant sink.

Approving on the strength of the witness-clean clone-and-return and the correct after-signing composition order. The redaction catch-all is the one follow-up I'd land before flipping these events on for an untrusted sink — but it doesn't block the hook shipping behind an opt-in flag.

@bokelley bokelley merged commit 918067d into main Jun 25, 2026
32 checks passed
@bokelley bokelley deleted the issue-2272 branch June 25, 2026 16:01

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Clean additive observability hook that holds the SDK's witness-not-translator line — it emits sanitized copies, never mutates the protocol response or fabricates a field.

Things I checked

  • Redaction is load-bearing, not theater. The diagnostic wrapper is the innermost network fetch in both transports, so it sees the fully-assembled request with auth attached — a2a.ts:303 injects Authorization/x-adcp-auth before networkFetch, mcp.ts:912 sits diagnosticFetch under the RFC 9421 signing fetch. authorization, cookie, proxy-authorization, set-cookie, x-adcp-auth, x-api-key, mcp-session-id are all in SENSITIVE_HEADER_NAMES. signature/signature-input aren't in the safe allowlist and don't start with x-, so they drop. security-reviewer: no High-severity leak in the deployed config.
  • Header emission is an allowlist, not a denylist (transportDiagnostics.ts sanitizeTransportHeaders). Unknown sensitive headers (dpop, x-amz-security-token, x-goog-api-key) are dropped, not emitted. Fail-closed beats fail-open — right default for a surface that logs.
  • SSE is never teed. responseBodySnippet checks isDiagnosticTextContentType and returns undefined for text/event-stream before response.clone() is reached. MCP streamable-HTTP streams are untouched; only status/headers fire. Confirmed by both code-reviewer and javascript-protocol-expert.
  • response.clone() doesn't corrupt the consumer. Same proven pattern already shipping in wrapFetchWithCapture ("clone before reading so the SDK still gets a consumable body"); original Response returned unconsumed. Diagnostic clone reads the size-limited stream (wrapper sits outside wrapFetchWithSizeLimit), so a hostile body can't blow memory through diagnostics.
  • ALS slot lifecycle is sound. withTransportDiagnostics awaits Promise.allSettled(slot.pending) in finally; handlers run as .then(()=>{}, ()=>{}) so a throwing handler can't reach the protocol caller. Cached MCP connections resolve the current call's slot at request time, not connection-build time — the E2E test asserts two reused-connection calls emit distinct operationId/idempotencyKeyHash.
  • protocols/index.ts (+210/-171) is a mechanical reindent. The entire callTool body is wrapped one level deeper inside withTransportDiagnostics({...}, () => withSpan(...)). In-process MCP, OAuth client-credentials refresh, the callMCPToolWithTasks + 401 retry, the A2A retry, and the final else throw 'Unsupported protocol' are byte-identical. Capability-priming forwards onTransportActivity/transportActivityContext on the recursive callTool. No logic changed.
  • Changeset present, minor, correct. .changeset/clean-taxis-observe.md — new opt-in config + additive exports (sanitizeTransportHeaders, sanitizeTransportUrl, TransportActivity* types). Additive = minor. package.json version is untouched (9.0.0 both sides); the lockfile bump beta.31→9.0.0 just resyncs a stale lockfile on main, not a manual version edit.
  • Official SDKs preserved. The wrapper is a typeof fetch decorator into StreamableHTTPClientTransport / A2AClient — no custom HTTP or SSE reimplementation.

Follow-ups (non-blocking — file as issues)

  • durationMs on a streamed (SSE) tool is time-to-headers, not time-to-completion, because response_received fires when upstream() resolves. A one-line doc note on the TransportActivity.durationMs field would stop a consumer reading it as end-to-end latency.
  • Body-key redaction gaps (security-reviewer, defense-in-depth): bearer/assertion/dpop/session[_-]?id/client_assertion aren't in SENSITIVE_KEY_RE. None appear in normal AdCP request/response bodies — auth is header-borne — so no concrete leak today. Worth adding for parity with the CTX-METADATA-SAFETY.md ethos if an adopter ever embeds a secret under such a key.

Minor nits (non-blocking)

  1. Constant HMAC salt for the idempotency fingerprint. fingerprintDiagnosticValue salts with the literal 'adcp-transport-diagnostics'. Fine for correlation (one-way, raw key independently redacted), but a per-process random salt would give cross-deployment unlinkability if that ever matters.
  2. emitTransportActivity re-reads the slot via getStore() rather than receiving it. Safe today — the wrapper only runs inside withTransportDiagnostics — but passing the slot explicitly would be more robust against a future caller emitting outside the scope and silently dropping the pending promise.

The diagnostics-enabled token-exchange path is the one body that legitimately carries client_secret, and ClientCredentialsFlow.ts:199 builds its own fetch that never reads the ALS slot — so it's never observed. Nicely fenced.

Safe to merge.

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.

Expose transport-level diagnostics hook for ADCP client calls

1 participant