Skip to content

feat(a2a): implement A2A transport and server bridge (Track 3)#22

Open
MichielDean wants to merge 15 commits into
mainfrom
feat/a2a-transport-track3
Open

feat(a2a): implement A2A transport and server bridge (Track 3)#22
MichielDean wants to merge 15 commits into
mainfrom
feat/a2a-transport-track3

Conversation

@MichielDean
Copy link
Copy Markdown
Collaborator

Summary

Completes Track 3: A2A transport implementation for both caller and server sides.

Caller side (adcp module)

  • A2aCaller — JSON-RPC tool dispatch over A2A protocol with SSE streaming, SSRF-safe redirect policy, response size limits, and idempotency key
  • A2aConnectionManager — credential-isolated client cache using HMAC cache hash; protected headers (Authorization, Cookie) stripped from cache keys but forwarded to agent-card discovery; exact-key eviction on transport errors
  • ProtocolClient — A2A dispatch path wired with computeCacheHash, evict-and-retry on transient failures

Server side (adcp-server module)

  • A2aServlet — Jakarta servlet bridge for A2A JSON-RPC; AsyncContext-based SSE streaming; writerLock guards concurrent writes; exactly-once asyncContext.complete() via AtomicBoolean CAS; completeAsync holds writerLock to eliminate write-after-complete race
  • A2aAgentExecutor — adapts A2A message requests to AdcpPlatform.handleTool; plumbs ServerCallContext state as request headers (primitive values only)
  • A2aAuthProvider — pluggable authentication SPI
  • A2aServerBuilder — fluent builder wiring AgentExecutor + RequestHandler

Security hardening (5 audit cycles + 3 code review cycles)

  • SSRF: redirect-never HttpClient, DNS pre-validation, AgentCard URL pinning to validated origin
  • Input bounds: request body cap (1 MB), method/message ID/tool name length limits, args scan cap, adcp_version field length cap
  • Auth isolation: per-credential HMAC cache hash prevents cross-tenant client sharing; raw secrets never encoded into cache key strings
  • SSE concurrency: completeAsync holds writerLock to close the write-after-complete race; subscription backpressure via request(SSE_PREFETCH) / request(1)
  • Log safety: control-character stripping in all user-controlled log fields

Dependencies

  • Added a2a-java-sdk 1.0.0.CR1; upgrade to GA deferred to a follow-up (expected minimal changeset)

ROADMAP

  • Track 3 marked complete
  • A2A SDK version decision documented

Caller side (adcp module):
- A2aCaller: JSON-RPC tool dispatch over A2A protocol with SSE streaming,
  SSRF-safe redirect policy, response size limits, and idempotency key
- A2aConnectionManager: credential-isolated client cache using HMAC cache
  hash; protected headers (Authorization, Cookie) are stripped from cache
  keys but forwarded to agent-card discovery; exact-key eviction on retry
- ProtocolClient: A2A dispatch path with computeCacheHash, evict-and-retry

Server side (adcp-server module):
- A2aServlet: Jakarta servlet bridge for A2A JSON-RPC; AsyncContext-based
  SSE streaming with writerLock guarding concurrent writes and exactly-once
  asyncContext.complete() via AtomicBoolean CAS
- A2aAgentExecutor: adapts A2A message requests to AdcpPlatform.handleTool;
  plumbs ServerCallContext state as request headers (primitives only)
- A2aAuthProvider: pluggable authentication SPI
- A2aServerBuilder: fluent builder wiring AgentExecutor + RequestHandler

Security hardening (5 audit cycles + 3 code review cycles):
- SSRF: redirect-never HttpClient, DNS pre-validation, AgentCard URL pinning
- Input bounds: request body cap (1 MB), method length, message ID length,
  tool name length, args scan limit, adcp_version field length
- Auth isolation: per-credential cache hash prevents cross-tenant sharing;
  raw secrets never appear in cache key strings
- SSE concurrency: completeAsync holds writerLock to eliminate write-after-
  complete race; subscription backpressure via request(SSE_PREFETCH)/request(1)
- Log safety: control-character stripping in all user-controlled log fields

Dependencies: a2a-java-sdk 1.0.0.CR1 (upgrade to GA deferred)
ROADMAP.md: mark Track 3 complete, document a2a-java-sdk version decision

jira-issue: N/A

Co-authored-by: Copilot <[email protected]>
@MichielDean
Copy link
Copy Markdown
Collaborator Author

I know it said to wait for a full 1.0.0 release, but I think this CR1 release is likely close enough for now and we likely won't have any major breaking changes and we can punt the full 1.0.0 upgrade to later. This will allow us to close track 3 for now.

Lockfiles regenerated to match CI resolution. The a2a-java-sdk 1.0.0.CR1
transitive dependency tree resolved differently on Linux (CI) vs macOS
(local), causing the lock-drift check to fail.

jira-issue: N/A

Co-authored-by: Copilot <[email protected]>
@MichielDean MichielDean marked this pull request as ready for review May 20, 2026 22:23
@MichielDean MichielDean requested a review from bokelley as a code owner May 20, 2026 22:23
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Request changes — narrow block. The only thing keeping this from approval is a missing .changeset/*.md. Expert verdicts came back clean: security-reviewer no HIGH/MEDIUM with all five hardening claims verified line-by-line; ad-tech-protocol-expert sound-with-caveats; code-reviewer no Blockers. The implementation is well-built — 5 audit cycles show in the diff.

Why this blocks

.changeset/ has only README.md and config.json. This PR adds a new public transport surface on two published artifacts — adcp (org.adcontextprotocol.adcp.transport.a2a.A2aCaller, A2aConnectionManager) and adcp-server (A2aServlet, A2aServerBuilder, A2aAgentExecutor, A2aAuthProvider). Per .changeset/README.md and D18, that is the textbook adopter-visible change. changeset-check is green because npx changeset status succeeds on zero entries — the workflow is informational, the policy is not. Run npx changeset, pick adcp + adcp-server at minor, and ship the entry alongside.

Things I checked

  • D10 alignment. a2a-java-sdk:1.0.0.CR1 in gradle/libs.versions.toml is the explicit pinned version per D10 — not drift. Upgrade to GA is a straight version bump per the decision row.
  • D9 alignment. No drift off mcp-core:1.1.2 + mcp-json-jackson2:1.1.2. No mcp bundle artifact slipped in.
  • D2 / D7 / D3. JDK 21 features used (no *Async mirror), jakarta.servlet throughout, org.adcontextprotocol.adcp.{transport.a2a,server.a2a} sub-packages match the surface convention.
  • *Request / *Response invariant. ad-tech-protocol-expert confirmed no new public records violate the naming guard.
  • No Optional<T> on the public surface. Confirmed by both reviewers.
  • SSRF posture (A2aCaller, A2aConnectionManager). security-reviewer walked the path: AdcpHttpClient.sendDnsPinResolver.resolveAndPinRedirect.NEVER → body cap; StrictSsrfPolicy denies loopback / link-local / site-local / multicast / IPv4-mapped-v6 / CGN / ULA / 6to4-private / Teredo-private / NAT64-private. AgentCard URL pinning at A2aConnectionManager.java:359-366 overrides url + supportedInterfaces from the validated baseUri — hostile card cannot redirect subsequent JSON-RPC POSTs.
  • Cache-key isolation (A2aConnectionManager.java:77). HMAC-SHA256 with a per-process random key via ProtocolClient.java:252; raw secrets never enter the key. Protected headers (Authorization, Cookie, Proxy-Authorization) stripped from cache keys but forwarded to agent-card discovery. A2aConnectionManagerTest.java:110-116 asserts Authorization absent from the key — the right assertion shape.
  • SSE concurrency (A2aServlet.java). writerLock guards every writer touch; completed.get() checked inside the lock at :362 before any write; completeAsync CAS-flips completed inside the same lock at :379. Write-after-complete is closed.
  • Input caps. Request body 1 MB at A2aServlet.java:188 enforced before parse; method/message-id/tool-name length caps + control-character strip throughout; adcp_version capped at 20; parts-/history-scan capped at 20.
  • Build. ./gradlew build green; lockfile regen across all eight modules looks like an honest sweep, not drift.
  • Tests. A2aServletTest.java (556 LoC, largest-file rule) covers body-cap, malformed JSON, missing method, unknown method — solid edge-case shape. @SuppressWarnings("deprecation") at :54 is the symptom of the unauth-provider footgun called out below.

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

  1. A2aServerBuilder.ensureStarted() lifecycle (adcp-server/.../A2aServerBuilder.java:75). mainEventBusProcessor is started but never closed. Adopters who hot-reload or call build() more than once leak the event bus + queue manager + task store per call. Either return a Closeable server wrapper, or move the processor's lifecycle to a parent component.

  2. extractCallContextHeaders projects state into headers (adcp-server/.../A2aAgentExecutor.java:179). ServerCallContext.getState() carries non-HTTP-header data; flattening it into AdcpContext.headers() (primitive-only filter or not) means tool handlers see internal context entries as if they came from the wire. Allowlist the keys, or namespace them, or surface them on AdcpContext as a distinct field.

  3. AgentCard well-known path (adcp/.../A2aConnectionManager.java:356). Hand-rolled /.well-known/agent.json; current A2A spec is /.well-known/agent-card.json. The pinned SDK likely already has a discovery helper — delegate to it rather than hand-rolling the URL, and you stop paying this tax on every spec revision.

  4. Idempotency-Key not first-class. A2aCaller.callTool forwards a generic headers map (A2aCaller.java:111); AdcpContext (AdcpContext.java:18-22) has no idempotencyKey field. PR description claims plumbing, but the canonical header name isn't enforced at either end. Either honor the IETF draft header name in A2aCaller + surface it on AdcpContext, or drop the claim from the description.

  5. adcp_version extracted from args, not metadata (A2aAgentExecutor.java:138-171). Tool-name discriminator lives in metadata.adcp_tool_name on the caller side; version lives in args on the server side. Cross-transport invariant check against the MCP path would be worth filing. Also the major >= 3 gate at :156-158 is a magic number without a citation.

  6. Transport-error retry walk doesn't check e itself (adcp/.../ProtocolClient.java:169). isTransportError walks getCause() but skips e. A2aCaller.java:122 throws timeout ProtocolError with cause == null, so the timeout path never retries — likely the opposite of intent.

  7. Hard-coded 30s RESPONSE_TIMEOUT_SECONDS (A2aCaller.java:122). Ignores CallToolOptions.timeout. MCP path honors it; A2A doesn't. v0.1 limitation but file it.

  8. Deprecated single-arg A2aServlet constructor (A2aServlet.java:91-97). Wires an unauthenticated provider. @Deprecated is the right marker, but it's also the copy-paste path from A2aServerBuilderTest. Drop it before GA, or require an explicit A2aAuthProvider.unauthenticated() sentinel so the call-site reads as a deliberate opt-out.

  9. Unbounded objectMapper.valueToTree(first) fallback (A2aCaller.java:235-238). Last-ditch path when DataPart and TextPart scans both miss. Bounded upstream by the SSE chunk cap, so impact is small, but gate by MAX_CONTENT_LENGTH or throw ProtocolError and stop encoding "unknown" into a tree.

Minor nits (non-blocking)

  1. JSONRPC literal string repeated in A2aServerBuilder.java:96-102 — pull the constant from A2aConnectionManager.JSONRPC_TRANSPORT or shared A2aConstants.
  2. A2aServlet.writeError(response, SC_OK, …) (A2aServlet.java:171) returns HTTP 200 with a JSON-RPC error body — correct per JSON-RPC 2.0, inconsistent with the 4xx paths above. Pick a convention or document the split.
  3. HttpAgentCardLoader.load log level (A2aConnectionManager.java:330). Non-2xx responses currently debug; warn would give adopters a signal when an agent URL is wrong without spamming on transient network failures.

Add the changeset and the verdict flips to approve. Sensible v0.1 baseline; the audit-cycle claims hold up under scrutiny.

This comment was marked as resolved.

- Move extractVersion() inside try block in A2aAgentExecutor so
  VersionUnsupportedError is caught and reported via emitter.fail()
- Validate toolName in A2aCaller upfront (non-null, non-blank, length,
  control chars); use original value for outbound request, safeToolName
  only in log/error strings
- Add AdcpHttpClient.newHttpClientBuilder() transport-agnostic alias;
  update A2aConnectionManager to use it instead of newMcpClientBuilder()
- Fix Javadoc link in A2aServerBuilder: DefaultRequestHandler -> RequestHandler

jira-issue: N/A

Co-authored-by: Copilot <[email protected]>

This comment was marked as resolved.

github-actions[bot]
github-actions Bot previously approved these changes May 20, 2026
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Solid implementation of Track 3. SSRF closure via HttpAgentCardLoader.normalize pinning url + supportedInterfaces to the validated baseUri (A2aConnectionManager.java:359-392) is the right shape, and the SSE writerLock + AtomicBoolean completed CAS inside the lock (A2aServlet.java:377-383) is the correct exactly-once machinery. code-reviewer, security-reviewer, and ad-tech-protocol-expert all came back with no Critical/Blocker — Mediums and Minors only.

Things I checked

  • D-decision audit. D2 (no *Async, JDK 21) — clean. D7 (jakarta only) — A2aServlet is on jakarta.servlet.*. D9 (mcp-core + mcp-json-jackson2 1.1.2) — unchanged, no jackson3 leak via the A2A bundle. D10 — modified in-PR (see Follow-up 1). D18 — Conventional Commits clean.
  • Lockfile diff. Seven gradle.lockfiles touched. No Bouncy Castle (D2 honored), no jackson3 pull from the A2A transitive tree.
  • Largest-file rule. Read A2aConnectionManager.java (414), A2aServlet.java (390), A2aCaller.java (313), A2aAgentExecutor.java (199 — borderline), A2aServletTest.java (556). Findings cited inline below.
  • Per-process random HMAC for cache hashing (ProtocolClient.java:241-245) — prevents ghp_*-style token reversal from heap dumps. Right call.
  • Storyboard CI still in progress at the time of review.

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

  1. D10 governance. ROADMAP.md:22 flips D10 from "keep types in-tree until ≥1.0.0" to "depend on 1.0.0.CR1 directly" in-PR. Per CONTRIBUTING.md, changing a confirmed decision starts with an issue. Technical rationale (Beta1→CR1 is bug-fix only, package layout stable) holds, but this should have an explicit governance trail rather than landing bundled with the implementation. Flagging for maintainer sign-off — not blocking, since the change is documented openly in the PR body and the row itself.

  2. specs/a2a-binding.md is missing. AdCP-over-A2A wire shape is invented in code: adcp_tool_name metadata key, TextPart(toolName) duplicated as first part, version envelope merged into DataPart args (not metadata), extractResponse precedence DataPart→TextPart→tree-of-first-part, history-scan of up to 20 messages on terminal tasks. None of this is in specs/. Without a binding doc, the TS / Python SDKs diverge silently. mcp-prototype-findings.md set the precedent — A2A should match.

  3. Cross-tenant cache collision on shared OAuth clientId. ProtocolClient.computeTokenHash() at L283-L286 keys oauthClientCredentials on "cc:" + clientId only; two tenants sharing the same clientId but different clientSecret collide. basicAuth (username + ":" + password) collides at ("a:b", "c") ≡ ("a", "b:c"). Include the secret in the HMAC input (delimiter-separated or HMAC-per-field). Edge-case for single-tenant adopters, real gap for shared-IdP multi-tenant.

  4. A2aAgentExecutor.extractCallContextHeaders missing ProtectedHeaders filter (A2aAgentExecutor.java:173-188). Client side filters extraHeaders through ProtectedHeaders.isProtected() before sending; server side copies ServerCallContext.state into AdcpContext.headers() with no symmetric filter. If an adopter's A2aAuthProvider stashes Authorization / Cookie in state, those leak into tool implementations.

  5. A2aServlet JSON-RPC body is parsed by Gson with no depth bound. 1 MB byte cap is enforced (readRequestBody, L183-L197) but Gson defaults to depth 1000 and is not configured by JsonUtil. A 1 MB crafted-nested payload OOM-multiplies into ~16 MB of wrapper allocations. Not code-exec — DoS multiplier. Either depth-limit JsonReader or transcode through AdcpObjectMapperFactory (which has StreamReadConstraints) before re-handing to Gson.

  6. A2aConnectionManager.evictOldest holds cacheLock during client.close() (A2aConnectionManager.java:120-121, 200-209). A blocking socket teardown pins the global lock; concurrent getOrConnect() across all 32 stripes queues behind it. Collect evictees locally, release the lock, then close.

  7. Test coverage gap on the SSE concurrency machinery. A2aServletTest.AsyncStreamingRequestHandler (L529-L555) submits two events sequentially from one thread and closes. The writerLock + completed CAS guard exists to serialize onError / onTimeout against in-flight onNext — none of which is exercised. The PR claims this race is closed; tests don't validate the claim. Worth at least one race-test before this code goes load-bearing.

  8. A2aCaller synchronous-delivery guard is dead (A2aCaller.java:128-131). If callbacks fired synchronously, the latch is already at 0, so the getCount() > 0 && (latestMessage|task|failure != null) predicate is unreachable in the synchronous-delivery case. In the only scenario it could fire (async, callback path broken), force-counting-down masks the bug. Remove or convert to an assertion.

  9. extractResponse history-scan is too forgiving (A2aCaller.java:182-189). A task in TASK_STATE_COMPLETED with no status.message is server-misbehavior; walking up to 20 history messages risks returning a stale prior-turn payload. Scope the scan to non-terminal states.

  10. AgentCard skills=List.of() may fail A2A schema validation. A2aServerBuilder.buildAgentCard (L102) and HttpAgentCardLoader.fallbackCard (L410) both emit empty skills. A2A 0.3+ requires at least one. Stub adcp.invoke until codegen surfaces skill metadata.

  11. A2aServerBuilder.build() wires Runnable::run for both executors (L84-85). Tool execution runs on the servlet container's request thread — fine if the adopter pairs this with a virtual-thread container per D2, worth a Javadoc note pointing at Thread.ofVirtual().factory() for adopters who aren't.

  12. No changeset. New public packages (transport.a2a, server.a2a) plus a new a2a-java-sdk coordinate. changeset-check CI passed because the workflow runs changeset status informationally — but per .changeset/README.md ("when in doubt, add one"), a new transport surface isn't in doubt. Add a minor for adcp + adcp-server.

Minor nits (non-blocking)

  1. Stripe selection uses signed mod. A2aConnectionManager.java:90(hashCode & 0x7FFFFFFF) % STRIPE_COUNT distributes weakly across near-identical keys. Prefer Math.floorMod(cacheKey.hashCode(), STRIPE_COUNT).

  2. A2aServlet sets status / content-type after startAsync (A2aServlet.java:247-253). Move the header block above startAsync so subscribers invoked synchronously inside publisher.subscribe(...) always observe the right headers.

  3. Single-arg A2aServlet(RequestHandler) is correctly deprecated and Javadoc-warned as test-only, but production safety rides entirely on adopters reading the doc. Worth an explicit opt-in (-Dadcp.a2a.allow-unauthenticated=true) so a misconfigured prod deploy fails fast instead of accepting everyone.

Interesting that a PR advertising "5 audit cycles + 3 code review cycles" lands with zero concurrent-write tests on the writerLock machinery it's claiming to harden — worth closing that loop before this gets load-bearing.

LGTM. Ship it once storyboard CI goes green. D10 governance and specs/a2a-binding.md are the two follow-ups worth catching the maintainer on; the rest can roll into Track-3 follow-up PRs.

Add com.google.code.gson:gson 2.14.0 as an explicit implementation
dependency in adcp-server so A2aServlet's Gson usage is not reliant
on transitive resolution through the A2A SDK.

jira-issue: N/A

Co-authored-by: Copilot <[email protected]>

This comment was marked as resolved.

github-actions[bot]
github-actions Bot previously approved these changes May 20, 2026
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Approving. The Track 3 wire-up is the right shape: caller-side A2aConnectionManager with HMAC-keyed cache, server-side A2aServlet with writerLock-guarded SSE writes, and ProtocolClient evict-and-retry — and the D10 pin at 1.0.0.CR1 matches the amended ROADMAP row authorizing CR1.

Things I checked

  • D10-aligned. gradle/libs.versions.toml:24 sets a2a-sdk = "1.0.0.CR1"; ROADMAP D10 (line 22) explicitly authorizes CR1 with 1.0.0 final as a straight version bump. No drift.
  • D9-aligned. Still on mcp-core:1.1.2 + mcp-json-jackson2:1.1.2. No jackson3 leak through the A2A artifacts.
  • D7-aligned. jakarta.servlet.* everywhere; no javax surface.
  • D2-aligned. No *Async mirror methods, no Bouncy Castle.
  • SSE concurrency invariant. A2aServlet#writerLock covers both response.getWriter().write(...) and the completed.compareAndSet in completeAsync (A2aServlet.java:361-367, 377-383). The write-after-complete race the description claims to close is in fact closed.
  • Cache-key isolation. ProtocolClient#computeCacheHash HMACs token + sorted extraHeaders with \0/=/\n delimiters keyed by a per-process SecureRandom key. Raw secrets never appear in the cache-key string. CR/LF in extraHeaders is rejected upstream at AgentConfig.validateExtraHeaders, so the \n delimiter is unambiguous.
  • SSRF posture. HttpAgentCardLoader#normalize unconditionally overrides card.url() and supportedInterfaces with the validated baseUri (A2aConnectionManager.java:365-366), and the underlying HttpClient uses followRedirects(NEVER). JSON-RPC traffic cannot be redirected to internal addresses via a hostile agent card.
  • Body cap. A2aServlet#readRequestBody enforces the 1 MB cap before out.write (A2aServlet.java:189-193); max overshoot bounded by buffer.length.
  • Auth opt-in. A2aServerBuilder.build() returns a RequestHandler, not a servlet — adopters must instantiate A2aServlet themselves, so the @Deprecated no-arg unauthenticated constructor is opt-in.

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

  1. AgentCard URL pinning over-pins. A2aConnectionManager.java:365-366 replaces card.url() and supportedInterfaces with the bare baseUri. That forces all JSON-RPC traffic to baseUri root and breaks any agent that legitimately advertises a path-suffixed endpoint (e.g. https://host/agents/seller/jsonrpc). The SSRF concern is real but the fix is broader than it needs to be — pin the origin to baseUri, allow the path the card declares. (ad-tech-protocol-expert flagged this as the highest-priority finding.)
  2. Caller/server wire-shape asymmetry. Caller sends metadata["adcp_tool_name"] + TextPart(toolName) + DataPart(args) (A2aCaller.java:152-162); server reads metadata["adcp_args"] first (which the caller never writes), then falls back to TextPart+DataPart (A2aAgentExecutor.java:108-118). Pick one canonical shape and land specs/a2a-binding.md before GA — right now the Java SDK is unilaterally defining the wire convention.
  3. subRef race vs. async onSubscribe. A2aServlet.java:294-296 assumes Flow.Publisher.subscribe invokes onSubscribe synchronously. If a publisher dispatches onSubscribe on its own executor, onTimeout/onError can run first, cancelSubscription(subRef) returns null, and the later-arriving subscription leaks. Cancel inside onSubscribe if completed.get() is already true.
  4. Runnable::run inline executor. A2aServerBuilder.java:84-85 wires both executors to Runnable::run, so any blocking work in AdcpPlatform.handleTool runs on the servlet request thread and stalls the SSE writer. At minimum add a javadoc warning; ideally accept an Executor parameter.
  5. No changeset. ~700 LoC of new public API (A2aCaller, A2aConnectionManager, A2aServlet, A2aServerBuilder, A2aAuthProvider, A2aAgentExecutor) ships with no .changeset/*.md. CI doesn't fail (changeset-check.yml lacks --exit-code) and no artifact is published yet, but the v1.0 release notes will be the poorer for it.
  6. Test coverage gap. 6 happy-path tests for the 390-LOC servlet (A2aServletTest.java). No coverage for: writerLock contention, onTimeout mid-stream, body at MAX_REQUEST_BYTES + 1, async onSubscribe. Add at least one publisher-firing-onComplete-before-onSubscribe-returns case to lock in the race fix when Follow-up 3 lands.
  7. Dual JSON-mapper exposure. A2aServlet:104 uses com.google.gson.JsonParser for envelope parsing while the rest of the SDK is on Jackson. implementation(libs.gson) keeps it off the public surface, but introducing a second parser for one envelope is a notable choice. Replace with Jackson readTree or document why.

Minor nits (non-blocking)

  1. Dead synchronous-delivery guard. A2aCaller.java:124-131 — every state-setting branch (MessageEvent, terminal task, failure.compareAndSet) already calls countDown(). The extra countDown() is unreachable as a recovery path; the comment misrepresents what protection it provides. Drop or rewrite.
  2. Stack-trace leak in error log. A2aAgentExecutor.java:70 logs the full exception chain; if Jackson surfaces a fragment of the attacker-controlled request, that lands in logs verbatim. Prefer e.getClass().getName() + sanitizeErrorMessage(e.getMessage()).
  3. SSE timeout not configurable. A2aServlet.java:66 SSE_STREAM_TIMEOUT_SECONDS = 300 is private static final. Expose via constructor or init-param so adopters with long-running tools can tune.
  4. evict(URI) dead equals(prefix) branch. A2aConnectionManager.java:131-135buildCacheKey always appends #cacheHash, so no key equals agentUri.toString() alone. Harmless; trim or comment.
  5. Discovery Authorization silently dropped. AdcpHttpClient strips protected headers from outbound discovery; adopters whose /.well-known/agent.json requires auth will silently fall back to the synthetic card. Document the behavior.

Approving on the strength of the SSE concurrency invariant plus the unconditional AgentCard URL override closing the C-2 SSRF surface. The "5 audit cycles + 3 code review cycles" framing in the description sets a notable bar for what counts as a security-hardened PR around here.

- Normalize Accept header to lowercase (Locale.ROOT) in wantsStreaming()
  so Accept: Text/Event-Stream (or any other casing) correctly enables
  SSE streaming per RFC 7231 case-insensitivity rules
- Add explicit implementation deps for a2a-java-sdk-client-transport-jsonrpc
  (JSONRPCTransport, JSONRPCTransportConfigBuilder) and a2a-java-sdk-http-client
  (JdkA2AHttpClient) to adcp/build.gradle.kts; add catalog aliases for both

jira-issue: N/A

Co-authored-by: Copilot <[email protected]>

This comment was marked as resolved.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Request changes on the missing changeset. The code is sound; the block is a release-notes gap, not a defect.

Things I checked

  • D10 update (ROADMAP.md:22) is intentional governance — pin moves from "wait for 1.0 GA" to 1.0.0.CR1 with rationale (Beta1→CR1 delta is bug fixes only, in-tree fallback would have been throwaway). Aligned, not drift.
  • D9 still on mcp-core:1.1.2 + mcp-json-jackson2:1.1.2. No swap to the mcp bundle artifact.
  • D7 jakarta-only: A2aServlet imports jakarta.servlet.*, compileOnly(libs.jakarta.servlet.api). Confirmed.
  • D2 invariants: no *Async mirrors, no Bouncy Castle, JDK 21 features in use.
  • *Request / *Response naming invariant: holds across the new surface.
  • @Nullable over Optional<T>: confirmed across A2aAgentExecutor, ProtocolClient.
  • SSRF posture (caller): JdkA2AHttpClient is built on the AdcpHttpClient builder which sets followRedirects(NEVER). HttpAgentCardLoader.normalize (A2aConnectionManager.java:359-392) unconditionally pins url and supportedInterfaces to baseUri regardless of what the remote card declares — closes the obvious bypass.
  • Cache-key cross-tenant isolation: HMAC-keyed via per-process random in ProtocolClient.computeCacheHash (ProtocolClient.java:241-269). filterProtected strips Authorization/Cookie before buildCacheKey (A2aConnectionManager.java:77-78). No raw secret in the key.
  • SSE write-after-complete: completeAsync CAS is under writerLock (A2aServlet.java:377-383); writeStreamingResponse re-checks completed.get() inside the same lock (line 361-364). Race is closed.
  • Caller-side version envelope is merged at ProtocolClient.callTool:111 via VersionEnvelope.mergeInto(args, version) before dispatch — A2aCaller receives merged args. Looked like a gap on first read; it isn't.
  • code-reviewer: SOUND-WITH-CAVEATS, no Blockers.
  • security-reviewer: SOUND-WITH-CAVEATS, no High findings.
  • ad-tech-protocol-expert: SOUND-WITH-CAVEATS.

MUST FIX

  1. No changeset for an adopter-visible PR. Adds two new public packages (org.adcontextprotocol.adcp.transport.a2a.* and org.adcontextprotocol.adcp.server.a2a.*) across two of the eight published artifacts plus pulls in a new top-level dep (a2a-java-sdk:1.0.0.CR1). Per .changeset/README.md and CONTRIBUTING.md, this is the canonical "adopter-visible behavior" case. The changeset-check job passes because npx changeset status doesn't exit non-zero on empty — that's a CI gap, not permission. Add a minor changeset for adcp + adcp-server describing the A2A transport addition, and call out the D10 pin so the v0.3 release notes carry it.

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

  • Caller-side tasks/cancel fire-and-forget on buyer abort. ROADMAP §7.x explicitly requires it (ROADMAP.md:93). A2aCaller currently only times out the latch (A2aCaller.java:133); no POST on interrupt/timeout. Track for v0.4.
  • DNS TOCTOU between ProtocolClient.validateUrl probe and JDK HttpClient re-resolve. Acknowledged in the comment at ProtocolClient.java:217-221. DnsPinResolver already exists; the A2A path doesn't yet flow through it. Decide before v0.4 whether to thread the pinned resolver through JdkA2AHttpClient or accept the JDK's re-resolve.
  • Streaming SSE has no event-count or byte-budget cap. A2aServlet.stream honors the 300s timeout but does not bound total bytes written. A misbehaving AgentEmitter can stream unbounded data.
  • A2aCaller.extractFromParts fallback bypasses MAX_CONTENT_LENGTH. A2aCaller.java:246-256: when neither DataPart nor TextPart matches in the scan window, the valueToTree/treeToValue path runs without the size guard. Bounded by Jackson stream constraints but inconsistent with the rest of the file.
  • A2aServlet.doPost catch-all logs nothing. Line 172. The 500 envelope reaches the client; the operator gets no stack. Add log.error("A2A request handling failed", e) on this branch and on the IO branch at 163-169.
  • A2aConnectionManager.close() holds cacheLock during closeQuietly for every cached client. A slow Client.close() blocks every concurrent getOrConnect. Snapshot values, clear+flag closed under the lock, close outside.
  • A2aServerBuilder uses caller-thread executors and unbounded in-memory stores by default (Runnable::run, in-memory task/queue/notification stores). Javadoc warns about the stores but not the executors. Strengthen before v0.3.
  • CR1 → 1.0 final transition. D10 calls this "a straight version bump." Surface area is small but isolated to A2aServlet.java:302/347/354; be ready for wrapper-class rename at GA.

Minor nits (non-blocking)

  1. A2aConnectionManager.java:90cacheKey.hashCode() & 0x7FFFFFFF) % STRIPE_COUNT is correct; Math.floorMod(cacheKey.hashCode(), STRIPE_COUNT) reads cleaner.
  2. A2aCaller.java:128-131 — the synchronous-delivery latch guard is a no-op when the latch is already at 0; the comment admits it. Delete the dead branch.
  3. A2aConnectionManager.java:213-216MAX_HEADERS=50 silently truncates rather than rejects. A caller passing 60 routing headers gets the last 10 dropped and the connection still proceeds with potentially-wrong routing. Reject is the safer default.
  4. A2aServlet.java:90-97 — deprecated single-arg constructor allocates a new UnauthenticatedUser ServerCallContext lambda per request. Hoist or static-field it. The WARNING Javadoc reads stronger than the @Deprecated annotation — since="0.1" would help.
  5. A2aCaller.java:220-221, 240-241 — Jackson e.getMessage() is logged unsanitized at debug level. Source-input excerpts from a malicious agent can include CRLF; inconsistent with the rest of the file's sanitize-then-log discipline.

Five audit cycles plus three code-review cycles show in the diff — concurrency model is the right shape and the SSRF defenses are real. Add the changeset and this lands.

- Bump slf4j to 2.0.17 in version catalog to match lockfile resolution;
  update adcp-cli/gradle.lockfile accordingly
- Fix buildAgentCardUri() to use origin (scheme+authority) rather than
  appending to the full agent URI path — A2A Agent Card is always at
  /.well-known/agent.json on the origin root
- Enforce MAX_CONTENT_LENGTH in bytes (UTF-8) not chars in A2aCaller:
  DataPart uses writeValueAsBytes; TextPart uses getBytes(UTF_8).length
- Set completed=true inside writeTimeoutResponse() writerLock block
  (non-committed path) to close the race where an in-flight onNext()
  could write SSE data after the timeout error was sent

jira-issue: N/A

Co-authored-by: Copilot <[email protected]>

This comment was marked as resolved.

github-actions[bot]
github-actions Bot previously approved these changes May 20, 2026
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Approving. Track 3 lands clean — caller + server + SSE streaming with five audit cycles' worth of hardening visible in the diff, and the D10 amendment (1.0.0.CR1 direct dep, drop in-tree fallback) is documented in the same PR with reasonable rationale.

Things I checked

  • D-decisions. D2 (no *Async mirrors): clean. D7 (jakarta-only): clean — jakarta.servlet.* throughout, compileOnly(libs.jakarta.servlet.api) in adcp-server/build.gradle.kts:25. D9 (mcp-core:1.1.2 + mcp-json-jackson2): unchanged. D10: amended in ROADMAP.md from "in-tree until 1.0.0 stable" to "depend on 1.0.0.CR1, skip in-tree fallback" — author authority, rationale recorded in the table row. gradle/libs.versions.toml:24 matches.
  • SSRF posture. A2aConnectionManager.HttpAgentCardLoader.normalize (A2aConnectionManager.java:357-389) pins url and supportedInterfaces to the validated baseUri; DefaultClientFactory (A2aConnectionManager.java:300-305) wraps JdkA2AHttpClient with followRedirects(NEVER). The previously-known SSRF-via-AgentCard-redirect bypass is closed for the two fields the A2A client routes on by default. See follow-up note on additionalInterfaces below.
  • Auth isolation. ProtocolClient.computeCacheHash HMAC-SHA256s credentials with a per-process random key (ProtocolClient.java:241-269); A2aConnectionManager.buildCacheKey (A2aConnectionManager.java:179-198) URL-encodes header k/v so =/& injection is closed; ProtectedHeaders strips Authorization/Cookie from cache keys but sanitizedAll is forwarded to the agent-card probe. Cross-tenant client reuse is correctly impossible.
  • SSE concurrency. writerLock + AtomicBoolean completed + AtomicLong sequence in A2aServlet.java:261-399. writeStreamingResponse, writeFinalStreamingResponse, writeTimeoutResponse, and completeAsync all hold the lock across the completed check + write/complete. The lock-gap and write-after-complete races flagged in earlier review rounds are closed.
  • Input validation. Body 1 MB cap at A2aServlet.java:189-203 before parse; method 128-char cap; tool-name 256-char cap with control-char stripping in both caller (A2aCaller.java:80-91) and executor (A2aAgentExecutor.java:84-104); JSON-RPC id rejects booleans and structured ids per the 2.0 spec.
  • Defense-in-depth. AdcpObjectMapperFactory and A2aCaller both call deactivateDefaultTyping() — Jackson gadget surface explicitly closed. SchemaBundle.load and AdcpSchemaValidator.loadSchema reject .. and absolute paths.
  • Naming invariant. No new SDK-defined response records ship in the diff; A2A wrapper types (SendMessageResponse, etc.) are upstream. *Request builds / *Response doesn't is preserved by virtue of having no new responses.
  • Tests. Happy-path coverage for A2aServlet (sync + async-streaming via SubmissionPublisher), A2aCaller (response extraction), and A2aConnectionManager (cache isolation, exact-key eviction, post-close behavior). Concurrency-stress and inline-delivery publisher cases are not covered — see follow-up.

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

  1. Server error mapping is not A2A-structured. A2aAgentExecutor.errorMessage (A2aAgentExecutor.java:129-142) returns emitter.fail(...) with a TextPart containing {"error":code,"message":msg} JSON. A2A clients written against the spec expect structured JSONRPCError/A2AError codes (e.g. InvalidRequestError, InternalError — both already imported in A2aServlet.java). Java↔Java round-trips work because A2aCaller.extractResponse reads the TextPart back, but TS/Python A2A clients will see "task succeeded with weird content." Map AdcpError.codeA2AError subclasses before claiming cross-SDK interop.
  2. additionalInterfaces is not pinned in normalize. A2aConnectionManager.java:357-389 overrides url and supportedInterfaces but inherits everything else from the remote card via AgentCard.builder(card). If the A2A SDK's Client.builder(...) ever consults additionalInterfaces for routing under load-balancing or capability negotiation, that's a partial SSRF bypass. Verify against the 1.0.0.CR1 Client implementation and either pin or null those fields too.
  3. Cross-SDK wire-shape verification. A2aCaller.buildRequest (A2aCaller.java:154-164) sends metadata.adcp_tool_name + parts=[TextPart(toolName), DataPart(args)]. Confirm @adcp/sdk 7.x uses the identical metadata key and part ordering before the v0.4 beta tag. The PR description doesn't list mock-server smoke validation against the TS implementation; this needs to happen before claiming Track 3 interop-complete.
  4. SSE recursion under synchronous publishers. A2aServlet.java:307-313subscription.request(1) is called from onNext outside writerLock. With a synchronous Flow.Publisher (e.g. anything wired through A2aServerBuilder's Runnable::run executors at A2aServerBuilder.java:84-85), each request(1) re-enters onNext on the same thread. The intrinsic lock is reentrant so no deadlock — but stack grows ~5 frames per event. Default SubmissionPublisher is async via FJP so the production path is fine; the dev/test in-memory path is the failure mode. Trampoline pattern (counter + drain after lock release) would fix it cleanly.
  5. Concurrency-stress test coverage. A2aServletTest exercises happy-path SubmissionPublisher only. No tests drive writeFinalStreamingResponse concurrent with writeStreamingResponse, no inline-delivery publisher, no timeout-mid-stream, no cancel-during-onNext. Given the writer-lock + CAS invariants are the entire correctness story of the streaming path, at least one race-driving test belongs here.
  6. A2aServerBuilder Runnable::run executors. A2aServerBuilder.java:84-85 hardcodes synchronous executors with no override knob. Combine with (4) and any adopter who follows the builder path gets the recursion mode. Expose executor(Executor compute, Executor io) setters; consider rejecting Runnable::run outside an explicit dev/test mode.
  7. Deprecated unauth constructor ships in the artifact. A2aServlet.java:90-97 is @Deprecated with javadoc warning but otherwise indistinguishable from the production constructor at call-site review. Either @Deprecated(forRemoval = true, since = "0.1") with a target removal version, or move the no-auth shortcut into adcp-testing as A2aDevServlet. Deprecation alone is too polite for a "accepts everything as UnauthenticatedUser" constructor.

Minor nits (non-blocking)

  1. writeTimeoutResponse pattern is asymmetric. A2aServlet.java:344-348 uses if (completed.get()) return; completed.set(true); where the other two terminal paths (writeFinalStreamingResponse:366, completeAsync:395) use compareAndSet(false, true). Both correct under the writerLock, but symmetric CAS reads cleaner.
  2. HTTP header names are case-insensitive. A2aConnectionManager.buildCacheKey:188 orders via new TreeMap<>(sanitizedHeaders) — case-sensitive. A caller passing x-tenant vs X-Tenant gets distinct cache entries. Normalize on insertion in sanitizeHeaders.
  3. Server-side response size cap. A2aAgentExecutor.java:63 calls objectMapper.writeValueAsString(response) unbounded; caller side caps at 10 MB (A2aCaller.MAX_CONTENT_LENGTH). Mirror it.
  4. Catch-all swallows the throwable. A2aServlet.java:178-181 returns InternalError("Internal error") with no log.error(...). Server-side debugging is impaired.
  5. 413 vs 400 for oversize body. A2aServlet.java:169-175 reports body-cap IOExceptions as 400 "Invalid request body"; should be 413 Payload Too Large.

Safe to merge — fast-follow (1) and (4) before the v0.4 beta tag, and (3) before claiming interop.

@github-actions
Copy link
Copy Markdown

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

This is an automated message from the Argus AI review workflow.

This comment was marked as resolved.

…ly stripping them

Stripping control characters from tool names before dispatch was a
security concern: a caller could send 'foo\x00bar' and end up executing
'foobar', silently changing which tool runs. Both the metadata and
TextPart extraction paths now throw InvalidRequestError if the capped
name contains any ISO control character, matching the same policy as
A2aCaller. The validated name is safe to use directly in logs.

jira-issue: N/A

Co-authored-by: Copilot <[email protected]>
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Request changes. The architecture is right and the security work is real, but security-reviewer flagged the deprecated single-arg A2aServlet(RequestHandler) constructor as High — an adopter who copies a quickstart and skips the A2aAuthProvider ships a fully unauthenticated production server. Fix that and the rest is approve-track.

Must fix (blocking)

  1. A2aServlet.java:91-97 — deprecated unauthenticated constructor is too easy to copy into production. @Deprecated plus a strongly-worded javadoc is not enough; the constructor still compiles and produces a ServerCallContext of UnauthenticatedUser.INSTANCE for every request. Replace with a clearly-named static factory A2aServlet.unauthenticatedForTesting(handler) and delete the public unauthenticated constructor — or, at minimum, fail-fast at servlet init() unless -Dadcp.allowUnauthenticated=true is set. The named-factory variant matches the shape adopters use elsewhere ("InsecureClientForTesting") and removes the IDE-autocomplete trap. Fail-closed beats fail-open on the auth surface.

Things I checked

  • D2 — no *Async mirror methods, no Bouncy Castle on the A2A path, JDK 21 baseline preserved.
  • D7 — jakarta.servlet.* only; servlet API is compileOnly, adopter brings their own container. Clean.
  • D9 — mcp-core:1.1.2 + mcp-json-jackson2:1.1.2 unchanged in gradle/libs.versions.toml.
  • D10 — row updated in-PR (ROADMAP.md:22) to authorize the a2a-sdk = "1.0.0.CR1" pin. The original "keep in-tree until ≥1.0.0" plan is dropped with a stated reason (CR1→1.0 final is a straight version bump, no API reshuffle on the surfaces this PR touches). Governance change is the right way to handle it — D-row updated alongside the code that depends on it.
  • D16 — design decision landed in the existing ROADMAP.md D-row, not a new docs/adr/ directory.
  • D18 — commit history is Conventional Commits; changeset-check job green.
  • CLAUDE.md naming invariants — *Request / *Response separation respected on the new surface; @Nullable (JSpecify) used in place of Optional<T>; SLF4J for logging.
  • SSRF posture on A2aConnectionManager.java:295-396 — origin-pinning to validated baseUri plus followRedirects(NEVER) plus 4 KiB AgentCard body cap with a bounded streaming read. security-reviewer confirmed Jackson default-typing is deactivated on both inbound and outbound mappers and that protected headers (Authorization/Cookie/Proxy-Authorization) are stripped from the cache key.
  • 1 MB request body cap on A2aServlet.readRequestBody (A2aServlet.java:189-203) is enforced before write, not after read.
  • SSE writerLock pattern (A2aServlet.java:338-399) — completed CAS + writerLock does close the obvious write-after-complete race on the happy path; code-reviewer and security-reviewer both verified.

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

  • AdCP-over-A2A wire convention is undocumented. A2aCaller.buildRequest (A2aCaller.java:154-164) and A2aAgentExecutor.extractToolName / extractArgs (A2aAgentExecutor.java:92-127) encode the AdCP tool name into Message.metadata["adcp_tool_name"] plus a leading TextPart, with args in a DataPart. The TS SDK uses the same keys, so this is de facto cross-SDK protocol surface. Write it down in specs/a2a-wire.md (per D16) before a third party tries to interop.
  • A2aServerBuilder defaults DefaultRequestHandler executors to Runnable::run (A2aServerBuilder.java:84-85). Long-running tool dispatch will run on the servlet container thread under async dispatch, defeating the streaming design. Expose Executor parameters on the builder; default to a virtual-thread executor (D2 makes that free).
  • Exception swallowing in A2aServlet.doPost catch chain (A2aServlet.java:166-181). JsonProcessingException, the generic Exception branch, and the recoverable IOException branch log nothing — operators see opaque 500s with no stack trace. Add log.warn(...) / log.error(...) with the throwable.
  • Stream-path exception leaves async context hung (A2aServlet.java:130-181). If publisher.subscribe(...) throws synchronously after startAsync fires, the outer catch (Exception) calls writeError on the already-async-started response without asyncContext.complete(). The async-context timeout listener will eventually clean up at 300s, but that's a 5-minute hang on a non-conforming publisher. Wrap subscribe in its own try/catch and route the failure through writeFinalStreamingResponse.
  • A2aAgentExecutor.extractArgs reports bad arg shapes as internal_error (A2aAgentExecutor.java:109-127). A non-object adcp_args (array, string) throws IllegalArgumentException from Jackson and is caught by the broad outer handler at :69 — should be InvalidRequestError so the caller knows it's their bug.
  • DNS-rebinding TOCTOU window on the JSON-RPC client. A2aConnectionManager.java:309-322 reuses the JSON-RPC JdkA2AHttpClient across the connection cache lifetime; only AgentCard fetch revalidates DNS. URL-pinning to baseUri blunts the redirect vector but not the DNS swap. Out of scope for v0.1 alpha; flag for the L1 hardening pass.
  • Prompt-injection forwarding. A2aCaller.extractFromParts (A2aCaller.java:197-260) returns remote-agent text/data parts to the AdCP layer with no provenance fencing. Add an @implNote documenting that callers must fence agent output before feeding it to an LLM.
  • Missing changeset. Pre-v0.3 alpha (D6) means it's not enforced, and changeset-check is green — but Track 3 adds public API surface to adcp and adcp-server and should accumulate a minor changeset now so the v0.3 publish notes are complete.

Minor nits (non-blocking)

  1. writeTimeoutResponse sets completed=true before the write that may throw. A2aServlet.java:344-359. If the write fails, the outer completeAsync CAS fails and asyncContext.complete() is skipped. Async-context leak on the timeout-plus-write-failure path. Move completed.set(true) to after the successful write and flush.
  2. putIfAbsent on lowercased header key drops the second value silently. A2aConnectionManager.java:191. If the caller passes both X-Tenant=a and x-tenant=b, one is dropped without a log line.
  3. Raw e.getMessage() logged without control-char stripping in three spots. A2aConnectionManager.java:283,352, A2aCaller.java:222,243. Defense in depth — strip control chars at the placeholder.
  4. A2aServletTest.java:529-555 uses Thread.sleep(25). Flaky under loaded CI. Drive the publisher synchronously from the test thread after the subscribe latch fires.
  5. A2aAgentExecutorTest.java has no coverage for the AdcpError / VersionUnsupportedError emission paths (A2aAgentExecutor.java:65-72). Happy path and cancel are covered; the error branches are not.

writerLock and the completed CAS are the right shape; the security work across five audit cycles is visible in the diff. Resolve the deprecated-constructor block and this is ready to ship.

@github-actions
Copy link
Copy Markdown

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

This is an automated message from the Argus AI review workflow.

This comment was marked as resolved.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Holding the merge. Two security-High findings with concrete attack paths plus a missing changeset on an adopter-visible PR. The shape of the work is right — caller-side SSRF posture, HMAC-keyed credential isolation, SSE concurrency, *Request/*Response invariant via upstream a2a-java, JDK 21 native primitives, D9 still on mcp-core:1.1.2 + mcp-json-jackson2:1.1.2, no *Async mirrors, no Bouncy Castle — but the JSON-RPC leg quietly bypasses the SSRF posture the PR description claims, and an unauthenticated production constructor sits on the public surface guarded only by a Javadoc warning.

MUST FIX (blocking)

  1. DNS rebinding on the JSON-RPC leg. A2aConnectionManager.java:309-311 wraps adcpHttpClient.newHttpClientBuilder().build() in a raw JdkA2AHttpClient. The builder only sets connectTimeout and followRedirects(NEVER) — it does not carry DnsPinResolver and does not re-validate addresses on the request-time DNS resolution. The agent-card fetch is hardened through AdcpHttpClient.get(...) and the URL is then pinned to the validated baseUri (good), but every subsequent JSON-RPC POST re-resolves the hostname through the JDK HttpClient. An attacker controlling evil.example.com returns a public IP for the card fetch (passes SSRF), then 169.254.169.254 for the JSON-RPC POST. followRedirects=NEVER doesn't help — there is no redirect; DNS itself is the bypass. The PR description's SSRF claim — "redirect-never HttpClient, DNS pre-validation, AgentCard URL pinning to validated origin" — does not hold for the request leg. Fix: install a JDK 21 HttpClient.Resolver or interceptor that calls DnsPinResolver.validateAddress per resolution, or route A2A JSON-RPC through AdcpHttpClient.send() rather than via JdkA2AHttpClient directly. ProtocolClient.java:217-221 already acknowledges this TOCTOU window for MCP — A2A inherits the same gap but the PR claims it doesn't.

  2. Deprecated unauthenticated constructor on the production-jar public surface. A2aServlet.java:91. The single-arg A2aServlet(handler) constructs every request as UnauthenticatedUser.INSTANCE with empty scopes. @Deprecated is a comment, not a guardrail — it survives copy-paste from a quickstart, it survives gradle dependencies, and a Spring Boot autoconfig wiring it would expose every registered tool without auth. Fix: delete the constructor from adcp-server and move the test-only form to adcp-testing as something like A2aTestServlets.unauthenticated(handler). Production adopters then physically cannot reach the unauth path without depending on a test module. agentic-product-architect flagged the same.

  3. Missing changeset on adopter-visible PR. .changeset/ contains only README.md + config.json. This PR adds new public types in adcp (A2aCaller, A2aConnectionManager) and adcp-server (A2aServlet, A2aServerBuilder, A2aAuthProvider, A2aAgentExecutor), a new public method AdcpHttpClient.newHttpClientBuilder(), and pulls 6 new compile-classpath deps onto the starter. D18 — npx changeset, minor on adcp and adcp-server, body explaining the A2A surface and the a2a-sdk CR1 pin. The changeset-check job is informational per .changeset/README.md; passing CI is not the answer.

Things I checked

  • D9-aligned: gradle/libs.versions.toml still pins mcp-sdk = "1.1.2" with mcp-core + mcp-json-jackson2 — no jackson3 contamination via the mcp bundle artifact.
  • D10: rewritten in this PR to permit 1.0.0.CR1. Defensible — the rewrite is expanding the option set, not contradicting it — but bundling the governance change with the implementation means reviewers can't disagree with the D-row separately. For future D-rewrites, land them in a standalone PR. ad-tech-protocol-expert called this a "calculated gamble" and noted Quarkiverse projects have shipped CR→Final renames before; keep an escape clause for Message.parts, Task.status, AgentEmitter.sendMessage.
  • D7: jakarta.servlet.* everywhere, no javax imports.
  • D2: no *Async mirror methods added; no Bouncy Castle; JDK 21 Ed25519 not touched here.
  • *Request / *Response invariant: the public surface added here is A2aCaller, A2aConnectionManager, A2aServlet, A2aServerBuilder, A2aAuthProvider, A2aAgentExecutor — no new request/response records. Wire-shape records come from upstream a2a-java.
  • @Nullable vs Optional<T>: @Nullable consistently used (A2aServerBuilder.java:36-39, A2aAgentExecutor.java:80,110,144). No Optional<T> returns on the public surface.
  • Per-process HMAC key in ProtocolClient.java:241-245 is the right primitive — SecureRandom, 32 bytes, never persisted, never logged. security-reviewer confirmed.
  • A2aServlet.doPost exception handling at L166-L182: fails-closed correctly. Body-cap IOException at L169-L175 writes the generic error only if uncommitted. Catch-all Exception returns InternalError("Internal error") with no e.getMessage() — no stack/detail leak to client.
  • A2aCaller.callTool synchronous-delivery latch guard at L130-L133 — the recent commit 9c3416e correctly restricts the count-down to terminal results. No early count-down on in-progress tasks.

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

  1. A2aConnectionManager.buildCacheKey duplicate-header precedence. L189-L192. putIfAbsent over a LinkedHashMap keeps the first-iterated value, so {X-Tenant:a, x-tenant:b} and {x-tenant:b, X-Tenant:a} produce different cache keys. The fix isn't at the cache-key boundary — reject duplicate header names case-insensitively in sanitizeHeaders at L217-L233 and the ambiguity goes away.
  2. A2aServerBuilder.buildAgentCard() hardcodes skills(List.of()) at L102. AdcpPlatform already exposes supportedTools() / toolDescriptions() / toolSchemas() — the MCP side projects them; A2A drops them. Agents discovering this server via /.well-known/agent.json see a card claiming the agent does nothing. Map each supported tool to an AgentSkill (id = tool name, description = toolDescriptions().get(name)).
  3. A2A wire-shape vs. TS conformance oracle. ad-tech-protocol-expert could not verify metadata.adcp_tool_name + [TextPart(toolName), DataPart(args)] matches what @adcp/sdk 7.2.0's transports/a2a/A2AClient.ts puts on the wire. A2A 0.3.x has MessageSendConfiguration.skillId as a first-class field, and the leading-TextPart fallback in A2aAgentExecutor.extractToolName (L92-L105) is fragile — A2A in the wild routinely uses an opening TextPart for human framing. Confirm against the TS oracle before v0.4 freezes the shape.
  4. Version envelope location. adcp_major_version + adcp_version stuffed into tool args then args.remove(...) on the server (A2aAgentExecutor.java:53-55). Collides with additionalProperties=false tool schemas. The symmetric location is message.metadata, alongside adcp_tool_name.
  5. Confused-deputy via extractCallContextHeaders (A2aAgentExecutor.java:179-194). Lifts arbitrary entries from ServerCallContext.state into the AdcpContext headers map. If an auth provider stores attacker-influenced state, the tool sees it indistinguishable from a trusted claim. Namespace-prefix state-derived headers (e.g. state:foo) or allowlist, or carry principal/tenant via a typed ScopedValue per the D2 commentary.
  6. A2aAgentExecutor.extractResponse history scan (A2aCaller.java:184-192) walks newest→oldest and returns the first non-empty parts list. Picks up intermediate status-update messages instead of terminal output. Filter on ROLE_AGENT and prefer task.status.message before history.
  7. ServerCallContext returned from A2aAuthProvider. Pragmatic at L0 — adopters need UnauthenticatedUser, AgentInterface.CURRENT_PROTOCOL_VERSION anyway — but every adopter's auth code now imports org.a2aproject.sdk.server.*. Swapping a2a-java later means a breaking change for every adopter. Wrap with an AdcpCallContext record before 1.0 GA; after is breaking.
  8. adcp-spring-boot-starter/gradle.lockfile gained 22 lines with no source change in that module — A2A SDK + gson + protobuf + jakarta CDI now on the starter classpath. Verify A2A is intended as default, not opt-in, in the starter — fat-jar size for MCP-only adopters.
  9. SchemaBundle.load/exists and AdcpSchemaValidator.loadSchemaResource use contains(\"..\") to reject path traversal. Overly broad — rejects legitimate filenames like `foo..bar.json`. Split on `/` and reject any segment equal to `..`. AdCP-controlled schema filenames, so unlikely to hit in practice.

Minor nits (non-blocking)

  1. Hard-coded timeouts. A2aServlet.SSE_STREAM_TIMEOUT_SECONDS = 300 (L66) and A2aCaller.RESPONSE_TIMEOUT_SECONDS = 30 (L42). Production deployments will want to tune. Constructor parameter or system property.
  2. TOOL_NAME_KEY = \"adcp_tool_name\" duplicated between A2aCaller.java:43 and A2aAgentExecutor.java:34. Extract to a shared constant under transport.a2a.

Re-request review once (1), (2), and (3) are addressed. The rest of the surface is in good shape.

@github-actions
Copy link
Copy Markdown

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

This is an automated message from the Argus AI review workflow.

… sanitize debug logs

Bump catalog jackson version from 2.18.2 to 2.20.1 to match what the
A2A SDK dependency resolves in lockfiles; regenerate all lockfiles.

buildCacheKey() used putIfAbsent when normalizing header names to
lowercase, making value selection non-deterministic for case-insensitive
duplicate keys from non-ordered input maps. Now pre-sorts entries by
original key via TreeMap before lowercasing, so the alphabetically-last
original key always wins — deterministic regardless of input map type.

Two debug log statements were logging raw exception messages from
remote A2A responses, which can contain untrusted payload content or
control characters. Both now pass through sanitizeErrorText/sanitizeLogText
to strip control chars and truncate before logging.

jira-issue: N/A

Co-authored-by: Copilot <[email protected]>

This comment was marked as resolved.

…hable evict branch

computeCacheHash() was hashing all extraHeaders including protected ones
(Authorization, Cookie, etc.) that AdcpHttpClient strips before every
request. This fragmented the connection cache unnecessarily — agents
differing only in their Authorization header would get separate cache
entries even though they'd make identical requests. Now filters out
ProtectedHeaders.isProtected() entries before hashing.

evict(URI agentUri) had an unreachable key.equals(prefix) branch since
buildCacheKey() always produces 'agentUri#cacheHash[?headers]' — the
bare agentUri string can never equal a cache key. Simplified to a single
startsWith(agentUri + "#") predicate with a clarifying comment.

jira-issue: N/A

Co-authored-by: Copilot <[email protected]>
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Request changes. The AdCP-over-A2A wire framing in A2aCaller.buildRequest and A2aAgentExecutor.extractArgs diverges from the AdCP spec's normative A2A binding and from both sister SDKs (@adcp/sdk and adcp) — a Java client cannot interop with TS/Python A2A servers, and vice versa.

MUST FIX — wire-shape divergence from the AdCP A2A binding

The AdCP spec (adcontextprotocol/adcpdocs/building/by-layer/L0/a2a-guide.mdx, see lines 96–104, 132–146, 162–170) pins the request shape to a single DataPart:

parts: [{ data: { skill: "<toolName>", parameters: { ... } } }]

Both sister SDKs encode this way. The TS SDK at adcontextprotocol/adcp-client/src/lib/protocols/a2a.ts:512-520 builds exactly:

parts: [{ kind: 'data', data: { skill: toolName, parameters: parameters } }]

No metadata.adcp_tool_name. No leading TextPart(toolName). The Python SDK matches.

adcp/src/main/java/org/adcontextprotocol/adcp/transport/a2a/A2aCaller.java:154-164 sends instead:

Message.builder()
    .metadata(Map.of("adcp_tool_name", toolName))
    .parts(new TextPart(toolName), new DataPart(args))

where args is the merged map (including the adcp_major_version / adcp_version envelope, which has no counterpart on the wire in TS/Python — the version envelope is a Java-side invention here).

Symmetrically, adcp-server/src/main/java/org/adcontextprotocol/adcp/server/a2a/A2aAgentExecutor.java:79-127 reads tool name from metadata.adcp_tool_name first and args from metadata.adcp_args or raw DataPart.data. A TS/Python client's spec-conformant { skill, parameters } payload lands at handleTool with top-level skill/parameters keys instead of the parameters dict — schema validation fails on every cross-stack call.

The whole point of this SDK per CLAUDE.md is "full L0–L3 parity with @adcp/sdk (TypeScript) and adcp (Python)." That parity fails on its first transport.

Fix: mirror the canonical shape on both caller and server. Send/read a single DataPart with data: { skill: <toolName>, parameters: <args> }. Remove the adcp_tool_name metadata key, the leading TextPart(toolName), and the in-args version envelope from the A2A path — or run them through the spec authors before keeping them.

Things I checked

  • code-reviewer: sound-with-caveats, no blockers. Concurrency in A2aServlet.stream() is correct — writerLock guards every writer path and completed.compareAndSet(false, true) makes asyncContext.complete() exactly-once (A2aServlet.java:248-398). A2aConnectionManager.getOrConnect() DCL under stripe semaphore + cacheLock is sound (A2aConnectionManager.java:79-129). buildCacheKey() lowercases header names with deterministic last-write-wins and URL-encodes both name and value (:179-207). The recent latch-guard fix in A2aCaller.callTool correctly checks isTerminal(latestTask.get()) to avoid premature countDown on in-progress tasks (A2aCaller.java:121-138). No Optional<T> returns on the public surface; no *Response.builder().
  • security-reviewer: sound-with-caveats, no High findings. SSRF posture holds: JdkA2AHttpClient(adcpHttpClient.newHttpClientBuilder().build()) is Redirect.NEVER, HttpAgentCardLoader.normalize unconditionally pins card.url and card.supportedInterfaces to the validated baseUri regardless of what the remote card declares (A2aConnectionManager.java:174-181). ProtocolClient.computeCacheHash() uses a per-process random HMAC-SHA256 key from a static final byte[32] populated by SecureRandom at class-load (ProtocolClient.java:241-245) — not reversible from heap dumps. Body cap 1 MB, method cap 128, id-string truncation 128, tool name cap 256 with control-char rejection on both sides.
  • D-decision audit. D2 (no *Async): clean. D3 (package layout org.adcontextprotocol.adcp.transport.a2a / .server.a2a): clean. D7 (jakarta only): A2aServlet imports jakarta.servlet.*. D9 (mcp-core 1.1.2): untouched. D10 is updated in this PR from "keep types in-tree until ≥ 1.0.0" to "pin a2a-java-sdk 1.0.0.CR1" — legitimate as a coordinated governance change, but the row's "Beta1→CR1 delta is bug fixes and dep bumps only, no API reshuffling" justification doesn't hold against the CR1 release notes (feat: Add 0.3 protocol version compatibility layer #805, feat: add url and preferredTransport fields to AgentCard for v0.3 compatibility #863). The implementation is actually correct because of the CR1 additions, but soften the D10 wording before the next ROADMAP pass.

Follow-ups (file once the wire-shape block is resolved)

  • AgentCard URL pinning is incomplete. HttpAgentCardLoader.normalize (A2aConnectionManager.java:174-218) pins url and supportedInterfaces but passes provider.url, documentationUrl, iconUrl, additionalInterfaces[*].url through unmodified. Not a JSON-RPC-dispatch SSRF today, but a defense-in-depth gap — pin or drop them.
  • A2aServlet has no logger. Every catch (Exception e) path returns a generic InternalError with zero operational signal (A2aServlet.java:178-181). Add SLF4J and log.warn with sanitized requestId/method.
  • Deprecated A2aServlet(handler) constructor wires UnauthenticatedUser with no runtime signal. @Deprecated is not load-bearing. Add a one-shot log.warn on first request, or gate behind a system property adcp.a2a.allowUnauthenticated=true so a forgotten dev wiring fails closed.
  • A2aAgentExecutor.extractCallContextHeaders silently drops non-primitive ServerCallContext.state entries. Either String.valueOf() unconditionally or document the contract loudly in A2aAuthProvider Javadoc.
  • A2aServerBuilder.build() uses Runnable::run for both executor slots so tool execution runs on the request thread. Document or replace with a bounded pool before v1.0 GA.
  • A2aAgentExecutor plumbs state into AdcpContext.headers without CRLF validation. Caller side enforces it; server side should too.
  • Missing changeset on adopter-visible PR. Non-blocking right now — repo has shipped zero artifacts and changeset-check CI passed — but the adcp and adcp-server public surface adds new types; add one before the first publish.

Real work in here. The concurrency and SSRF posture are clean and the audit-cycle commits show in the code. The wire-shape divergence is the only thing standing between this and approval — fix that and the rest is follow-ups.

@github-actions
Copy link
Copy Markdown

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

This is an automated message from the Argus AI review workflow.

This comment was marked as resolved.

ClientCallContext, MessageSendParams, BiConsumer, and Consumer were
imported but not directly referenced — the A2aMessageClient lambda
interface uses them internally. Removing to keep -Xlint:all clean.

jira-issue: N/A

Co-authored-by: Copilot <[email protected]>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Hold. The AgentCard.normalize() SSRF guard (C-2) is incomplete — it pins url + supportedInterfaces but leaves at least one other URL-bearing field on the rebuilt card to whatever the remote agent ships, which re-opens the exact SSRF surface the C-2 comment claims to close. Block is on the v0.1 SSRF baseline (DNS pin + address-guards + redirect: manual + AgentCard URL pinning); the rest of the PR is strong.

Must fix (blocking)

  1. AgentCard normalize() is not comprehensive — SSRF re-open via untrusted card fields (adcp/src/main/java/org/adcontextprotocol/adcp/transport/a2a/A2aConnectionManager.java:373-406). Client.builder(agentCard).clientConfig(setUseClientPreference(true)) (L325-333) lets the A2A client consult fields on the card for transport selection. normalize() only overwrites url and supportedInterfaces. security-reviewer H1: confirm whether AgentCard in a2a-java 1.0.0.CR1 carries an additionalInterfaces (or equivalent) array that the JSON-RPC transport will consult with useClientPreference(true) — if it does, a malicious card setting additionalInterfaces: [{transport:"JSONRPC", url:"http://169.254.169.254/..."}] reaches the metadata endpoint past the DNS pin. Also explicitly null/empty iconUrl, documentationUrl, provider, securitySchemes on the rebuilt card since the same untrusted body controls them.

  2. normalize() transport-downgrade inconsistency (same file, L390-391). supportedInterfaces is force-pinned to [JSONRPC] for SSRF, but preferredTransport is only set when blank. A remote card advertising preferredTransport=GRPC plus a GRPC interface will keep preferredTransport=GRPC while supportedInterfaces is silently rewritten to [JSONRPC]getInterface(preferredTransport) then mismatches. Force preferredTransport=JSONRPC (and apply the same to fallbackCard for symmetry), or log a warning when stripping a non-JSONRPC preference.

Things I checked

  • D10 update is consistent — ROADMAP row at ROADMAP.md:22 flips from in-tree fallback to a2a-java 1.0.0.CR1, matching the code at gradle/libs.versions.toml:24. The CR1→GA bump path is reasonable for JBoss-cadence releases.
  • D2/D7/D9/D16 unchanged. No *Async mirrors, jakarta-only servlet imports (compileOnly per adcp-server/build.gradle.kts:28), MCP still on mcp-core:1.1.2, no new ADRs.
  • *Request/*Response invariant intact — A2aServerBuilder is a builder for a model class, not a response record.
  • @Nullable used throughout A2aServerBuilder, A2aAgentExecutor. No Optional<T> returns on the public surface.
  • A2aServlet exactly-once protocol (A2aServlet.java:393-399, 362-372, 374-384, 338-360): lock-on-writerLock + CAS on completed is sound. onNext reads completed under the same lock; no write-after-complete race.
  • A2aConnectionManager double-checked locking around cacheLock + connectStripes is correct; no double-create for the same key. evict(URI)'s startsWith(agentUri + "#") cannot match unrelated URIs since # is the unique terminator.
  • extractVersion(args) is called at A2aAgentExecutor.java:53 before the args.remove(...) at L54-55 — ordering correct.
  • Cache-key collision surface (ProtocolClient.java:267-272, A2aConnectionManager.java:178-206): HMAC-SHA256 with per-process key, header keys lowercased + URL-encoded + TreeMap-sorted. Sound for realistic header inputs.
  • Path-traversal guards in SchemaBundle.java and AdcpSchemaValidator.java catch the relevant vectors (.., leading /).
  • AdcpObjectMapperFactory.deactivateDefaultTyping() is the canonical Jackson gadget defense; belt-and-suspenders since the Jackson default is already off.
  • Largest-file rule: read A2aServlet.java (406), A2aConnectionManager.java (428), A2aCaller.java (316), A2aAgentExecutor.java (205), A2aServletTest.java (sampled).

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

  • adcp_tool_name over A2A is not yet a documented AdCP cross-language convention. ad-tech-protocol-expert flagged that the constant lives only at A2aCaller.java:43, with no specs/ or docs/ doc defining the wire shape. Cross-check against @adcp/sdk (TS) A2A caller before v1.0 GA; if TS encodes differently this PR breaks interop. Move the convention into specs/a2a-wire.md per D16.
  • A2aServerBuilder.buildAgentCard() declares skills: [] (A2aServerBuilder.java:102). Fine for v0.4 beta but populate from AdcpPlatform.registeredTools() (specialism declaration) before v1.0 GA — that is exactly what A2A AgentSkill exists for.
  • Adopter-visible PR, no .changeset/*.md. changeset-check CI passes only because no Java module is registered as a changesets package (package.json lists only adcp-sdk-java-tools, which is on the ignore list). Per .changeset/README.md this PR is exactly the case that needs a minor bump on adcp + adcp-server. Wire the Java modules into changesets and add the entry — until that happens, the changeset-check workflow has no teeth on the SDK itself.
  • computeCacheHash separator ambiguity (ProtocolClient.java:267-272). Header-name grammar in HTTP forbids =, but AgentConfig.extraHeaders() is an unvalidated Map. Length-prefix or validate against the token grammar.
  • extractCallContextHeaders namespace (A2aAgentExecutor.java:179-194). Coerces ServerCallContext.state to String and hands to AdcpContext.headers as if they were caller-controlled. Namespace-prefix (e.g. a2a_state:) or document the contract.
  • A2aServlet deprecated single-arg constructor lacks forRemoval=true (A2aServlet.java:91). Add forRemoval=true and consider fail-fast in init() when no auth provider is wired.
  • A2aServletTest has no concurrent-write race test. Author can land that as a follow-up issue.

Minor nits (non-blocking)

  1. Sync-delivery guard in A2aCaller.callTool (A2aCaller.java:130-133) is effectively a defensive no-op — every callback path already calls completion.countDown() itself. The comment overstates its role; either drop it or recomment as "defensive no-op."
  2. Retry-success swallows the original transport error silently (ProtocolClient.java:147-165, 187-203). Add log.debug("... recovered after retry, original: {}", original.getMessage()) so operators can see flapping.
  3. A2aServlet catch-all Exception (A2aServlet.java:178-181) returns 500 with no log. Add log.error("Unhandled A2A servlet error", e) so production bugs aren't invisible.

Once normalize() pins all untrusted URL-bearing fields and the preferredTransport inconsistency is closed, this is ready to ship.

@github-actions
Copy link
Copy Markdown

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

This is an automated message from the Argus AI review workflow.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

LGTM. Follow-ups noted below. The SSRF posture (agent-card URL pinning in A2aConnectionManager.normalize at adcp/src/main/java/org/adcontextprotocol/adcp/transport/a2a/A2aConnectionManager.java:373) and the SSE concurrency discipline in A2aServlet (writerLock + AtomicBoolean CAS across all four write paths) are the right shapes for this surface.

Things I checked

  • D10-aligned. gradle/libs.versions.toml:24 pins a2a-sdk = "1.0.0.CR1" exactly per the ROADMAP row; the CR1 → 1.0.0 final bump is on-roadmap, not a blocker here.
  • D2-aligned. No *Async mirror methods on the public surface. No Bouncy Castle. JDK 21 baseline preserved.
  • D7-aligned. adcp-server uses jakarta.servlet.* at compile time, compileOnly per the existing convention.
  • *Request / *Response invariant — confirmed sound by ad-tech-protocol-expert; no new *Response record has a .builder().
  • @Nullable vs Optional<T> — clean across the new surface (A2aServerBuilder, A2aAgentExecutor, A2aConnectionManager).
  • Credential cache isolation. ProtocolClient.computeCacheHash (adcp/src/main/java/org/adcontextprotocol/adcp/transport/ProtocolClient.java:253) HMAC-SHA256s the token with a per-process random key; "anonymous" collides only with other token-less agents; filterProtected correctly excludes Authorization from the cache key while leaving it in the per-call header map.
  • A2A SSE write-after-complete raceA2aServlet.completeAsync (line 393) and writeFinalStreamingResponse (line 362) both hold writerLock during the CAS-and-complete; no path can write to the response after asyncContext.complete() returns.
  • Outbound auth precedence. ProtocolClient.callTool line 100-108 filters caller extraHeaders through ProtectedHeaders before adding SDK-resolved auth — callers cannot override Authorization on the wire.
  • Changeset. None in the diff. Pre-v0.3 per D6 and the .changeset/README.md exemptions; Check for changeset CI is green.
  • CI. ./gradlew build (JDK 21), Check for changeset, commitlint, check / check, GitGuardian all green at review time.

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

  1. A2aServerBuilder leaks the MainEventBusProcessor on rebuild. adcp-server/src/main/java/org/adcontextprotocol/adcp/server/a2a/A2aServerBuilder.java:76 calls mainEventBusProcessor.ensureStarted(), but neither the builder nor the returned DefaultRequestHandler is AutoCloseable. Hot-reload, test cycles, and multi-tenant boot will accumulate background work with no documented shutdown path. Either return a holder that implements close() or document the adopter-side teardown sequence in the class Javadoc.

  2. Version-extraction divergence between A2A and MCP server paths. A2aAgentExecutor.extractVersion (adcp-server/src/main/java/org/adcontextprotocol/adcp/server/a2a/A2aAgentExecutor.java:144) returns null when adcp_major_version is absent; the MCP equivalent AdcpServerBuilder.extractVersion (line 198 of that file) falls back to the server's configured default. Same protocol, two semantics — identical clients will see different AdcpContext.version() depending on transport. The A2A executor isn't even constructor-injected with the server's default. Mirror the MCP fallback.

  3. A2aServerBuilder.buildAgentCard hardcodes .skills(List.of()). adcp-server/src/main/java/org/adcontextprotocol/adcp/server/a2a/A2aServerBuilder.java:102. Empty skills means discovery returns no tools — the CreativeBuilderPlatform doesn't advertise missing tools rule (ROADMAP.md §7.x, 7.0.0 line) is satisfied by accident. Plumb the AdcpPlatform's registered tool list into AgentSkill entries so discovery actually works.

  4. A2aAgentExecutor.extractArgs returns 500 on a non-map adcp_args. Line 115 does objectMapper.convertValue(..., LinkedHashMap.class); if a peer sends adcp_args as an array/string the resulting IllegalArgumentException falls into the generic Exception catch and emits internal_error. Adopters see a 500 for what is really a 400. Add an instanceof Map precheck and throw InvalidRequestError.

  5. A2aServlet body-cap IOException is silently mapped to 400 with no log breadcrumb. Line 169-175. Operators investigating rejected uploads get no SLF4J signal. Add a single log.debug before writeError so the cap is observable.

  6. Unauthenticated single-arg A2aServlet constructor is fenced only by Javadoc. Line 91. Once Spring Boot autoconfig lands and starts auto-wiring, a careless adopter could ship the deprecated convenience constructor to prod. Consider either (a) an explicit A2aAuthProvider.allowAnonymous() opt-in sentinel, or (b) a WARN at servlet init when UnauthenticatedUser is wired against a non-loopback bind.

  7. A2aConnectionManager.normalize overwrites supportedInterfaces blanket. Line 373-406. SSRF reason for URL pinning is sound, but if an agent legitimately advertises GRPC alongside JSONRPC on the same origin, the SDK forces JSONRPC. Acceptable for v0.1 (JSONRPC-only); add a TODO so this gets revisited when GRPC lands.

  8. extractCallContextHeaders doesn't validate header values. adcp-server/src/main/java/org/adcontextprotocol/adcp/server/a2a/A2aAgentExecutor.java:184-193. Adopters whose A2aAuthProvider stuffs raw HTTP headers into ServerCallContext.state will pass attacker-controlled X-Forwarded-* through to tool handlers without the CRLF / control-char filter that A2aConnectionManager.sanitizeHeaders applies. Reuse that filter on the way in.

Minor nits (non-blocking)

  1. A2aCaller.java:127-129 comment. The comment says "this countDown() is a no-op" but in the failure.get() != null window from a truly async error path the countDown is real (just harmless). Tighten the wording.

  2. A2aConnectionManager.java:192 header-case dedup. The comment is correct ("alphabetically-last original key wins among case-insensitive duplicates") but counterintuitive — worth a unit test asserting the X-Tenant vs x-tenant collision shape so the invariant doesn't silently regress.

  3. ProtocolClient.java:242-246 per-process HMAC key. Worth a one-line Javadoc noting connection caches do not persist across JVM restarts so adopters don't expect warm-cache behavior on rolling restarts.

Five audit cycles plus three review rounds is visible — the credential cache hash, header normalization, and SSE concurrency are all sitting in places that take a while to reach without that scrutiny.

Approving.

@github-actions
Copy link
Copy Markdown

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

This is an automated message from the Argus AI review workflow.

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.

3 participants