Repository navigation
Conversation
- v1 protobuf-flavored types throughout (Part $case unions, TaskState/Role enums, AgentExecutionEvent wrapping, SendMessageRequest) - new src/types-v1.ts helpers for part/message construction + legacy-part coercion - FileTaskStore implements v1 TaskStore.list() with filtering/pagination - AgentCard builder emits supportedInterfaces[] incl. dual-stack JSONRPC 1.0+0.3 - client normalizes both card generations into transport fallback list - inbound extraction handles both v0.3 and v1 part shapes tsc clean; matrix 470/514 pass (remaining failures = tests asserting v0.3 shapes)
Boots a real SDK 1.x JSON-RPC handler (legacyCompat enabled) on loopback and exercises both client paths: A. v1 outbound -> real SDK server B. legacy v0.3 outbound -> same server (v0.3-only card) C. optional live check against a production v0.3 peer Usage: npx tsx scripts/verify-e2e.mts
Additional local v0.3 verificationTo distinguish a v0.3-shaped card on the v1 SDK from a genuine legacy peer, I created an isolated fixture pinned to
This supplies the missing local backward-compatibility evidence. The live Prax check remains environmental and is still blocked while Prax is offline. |
There was a problem hiding this comment.
Internal Code Review — requesting-code-review
Second independent perspective (superpowers code-reviewer), separate from CodeRabbit's bot-layer review (see the AI Review Summary for the full reviewer matrix including CodeRabbit findings).
Prior to this review, the reviewer's principal independently verified the PR's core claim: built this PR locally (npm install + npm run build, 0 tsc errors), reran the test suite (514/514 passed, matching the PR description), and ran a live handshake against a real A2A v1.0 server (ericabouaf/claude-a2a, GitHub HEAD build) using this PR's own a2a-send.mjs client — the handshake succeeded end-to-end (discover + message/send round trip). This review covers code quality/architecture only.
Strengths
- Well-designed dual-stack architecture — v0.3 vs v1.0 card detection is sound. Legacy detection via absent
supportedInterfaces/protocolVersionprefix cleanly separates the two paths. - Robust part conversion (
types-v1.ts) —toV1Parthandles v0.3 TextPart/FilePart/DataPart, plain strings, and already-v1 protobuf parts, with a pragmatic JSON-stringify fallback for unknown shapes. - Backward-compatible card generation — advertising both v1.0 and v0.3 JSONRPC on the same URL (
agent-card.ts) lets legacy peers negotiate down without a second fetch. - Adaptive transport selection (
transport-fallback.ts) — composite scoring (success rate × latency) with a sliding window is a mature design. - Comprehensive test coverage — 27 new tests in
compat-matrix.test.tscovering v0.3/v1.0 cards, message formats, auth headers, transport fallback, and edge cases (empty parts, missing fields, forward-compat unknown fields). - Clear separation of concerns — the legacy v0.3 path (
sendViaLegacyJsonRpc) is fully isolated from the SDK v1 path.
Issues
Critical (Must Fix)
None identified — protocol negotiation is sound and the test suite is thorough.
Important (Should Fix)
-
src/client.ts:332-334— Silent fallback when no transport interfaces are found masks a malformed Agent Card instead of surfacing it.- If both the v0.3 and v1.0 interface-building loops push nothing, the code falls back to
pushEndpoint(legacyCard.url, "JSONRPC")without verifyinglegacyCard.urlexists. - Impact: a peer that advertises no interfaces at all (empty/malformed card) silently defaults to a JSONRPC transport that may not exist, producing a confusing downstream error instead of a clear "no transport interfaces" failure.
- Suggested fix: only fall back when
legacyCard.urlis actually present; otherwise return an explicit error (e.g.{ ok: false, statusCode: 500, response: { error: "Agent Card has no service URL and no supported interfaces" } }).
- If both the v0.3 and v1.0 interface-building loops push nothing, the code falls back to
-
src/client.ts:298-300—cardIsLegacyOR-condition is partially redundant and worth a comment.supportedIfaces.length === 0 || protocolVersion.startsWith("0.")— the primary signal (supportedIfaces.length === 0) is already sufficient and robust; the second condition adds defensive coverage for a field (protocolVersion) that may be optional per spec, but the rationale isn't documented.- Not a bug (a v1.0 card with
protocolVersionomitted still correctly falls through sincesupportedIfaces.length > 0), but a short comment on the dual-condition rationale would help future maintainers.
-
src/client.ts:305-312— v0.3 → v1 field-name mapping (additionalInterfaces[].transport→protocolBinding) works but isn't called out explicitly, which can confuse a maintainer reading only one side of the mapping.- Suggested: an inline comment naming both field names side-by-side where the mapping happens (lines 326-330).
Minor (Nice to Have)
src/client.ts:311,326-328—legacyCard.additionalInterfacesis typed as an array but not runtime-validated; a peer sending a non-array value here is silently treated as falsy/empty. AnArray.isArray()guard would make this explicit rather than incidental.src/types-v1.ts:468-477—toV1Part's fallback (JSON.stringify) for unrecognized v0.3 part shapes doesn't explicitly preserve fields likemetadata/annotationsif a v0.3 part were to carry them. Low impact — current v0.3 parts are limited to text/file/data, which are all handled.src/client.ts:339-341— the "adaptive ordering when we have stats" comment doesn't mention that adaptive ordering is skipped entirely when no transport has any recorded stats yet (falls back to staticorderTransports). Behavior is correct; comment could be more precise.
Recommendations
- Document the v0.3 wire format (
LEGACY_METHOD_MESSAGE_SENDand friends) with a pointer to where the v0.3 spec constants are anchored (@a2a-js/sdk/compat/v0_3), for future maintainers. - An E2E test against a real (not mocked) v0.3 peer would complement the existing mock-based test suite — this PR's own
scripts/verify-e2e.mtsalready does something like this manually; formalizing it as CI-runnable (with a fixture peer instead of a hardcoded path) would be valuable. - Consider basic instrumentation on transport fallback outcomes (attempts/success per transport per peer) to observe negotiation behavior in production over time.
Assessment
Ready to merge? With fixes
Reasoning: The dual-stack negotiation is well-architected and the independently-verified live handshake against a real v1.0 peer confirms it works end-to-end. No Critical issues found in this review. See the AI Review Summary for the full findings list including CodeRabbit's independently-verified findings.
| if (allInterfaces.length === 0) { | ||
| pushEndpoint(legacyCard.url, "JSONRPC"); | ||
| } |
There was a problem hiding this comment.
Important: this fallback pushes legacyCard.url as a JSONRPC endpoint whenever no interfaces were found in either card generation, without checking legacyCard.url actually exists. A peer with a malformed/empty card silently gets routed to a transport that may not exist, instead of a clear "no transport interfaces" error. Suggest gating this on legacyCard.url being present, else returning an explicit error.
| const cardIsLegacy = | ||
| supportedIfaces.length === 0 || | ||
| String(cardAny.protocolVersion ?? "").startsWith("0."); |
There was a problem hiding this comment.
Minor clarity note: supportedIfaces.length === 0 already fully covers legacy detection (a v1.0 card always has supportedInterfaces); the protocolVersion.startsWith("0.") OR-branch is defensive for a field that's optional per spec. Not a bug, but a short comment here on why both conditions exist would help future readers.
| // Build transport list, normalizing BOTH card generations: | ||
| // v0.3: url + preferredTransport + additionalInterfaces[] | ||
| // v1: supportedInterfaces[] ({ url, protocolBinding }) | ||
| const legacyCard = agentCard as unknown as { | ||
| url?: string; | ||
| preferredTransport?: string; | ||
| additionalInterfaces?: { url?: string; transport?: string }[]; | ||
| }; |
There was a problem hiding this comment.
The v0.3 → v1 field mapping (additionalInterfaces[].transport → protocolBinding) works correctly where it's applied (lines 326-330 below), but isn't called out explicitly at the mapping site — an inline comment there naming both field names side-by-side would reduce the chance of a future edit breaking the mapping without realizing it's a cross-version rename.
DrumRobot
left a comment
DrumRobot
left a comment
There was a problem hiding this comment.
CodeRabbit CLI Review (local) — bot-layer substitute
This repository isn't connected to a CodeRabbit organization the reviewer's principal can access, so no cloud walkthrough was available. Per this workflow's policy, coderabbit review --agent -t all was run locally against this PR's worktree as a bot-layer substitute (separate from the Internal Code Review, which is an independent second perspective from a different engine).
CodeRabbit CLI reported 4 findings. Each was verified against the code at HEAD (6ad2716) before inclusion here:
| Finding | File | Verdict |
|---|---|---|
params.status filtering should treat omitted status as UNSPECIFIED |
src/task-store.ts:148-153 |
False positive — ListTasksRequest.status is a non-optional protobuf3 scalar field (status: TaskState, verified against @a2a-js/sdk's type declarations), so it can never be undefined; the existing check (params.status !== TaskState.TASK_STATE_UNSPECIFIED) is already correct. No change needed. |
Raw Uint8Array/Buffer handling before base64 encoding |
src/executor.ts:1668-1675 |
Valid — see inline comment below. |
| Hardcoded peer-file path in verify script | scripts/verify-e2e.mts:81-82 |
Valid, low impact — developer-machine-specific path in a local verification script (not shipped code); the missing-file case already degrades gracefully to SKIP (no token). An env-var override with this path as fallback would make it portable for other contributors. |
Invalid/non-absolute agentCard.url can throw during register() |
src/agent-card.ts:43 |
Valid, low impact — new URL(jsonRpcUrl).origin throws uncaught if agentCard.url is configured but not a valid absolute URL. Validating (or a try/catch fallback to fallbackUrl) would turn a misconfiguration into a clear error instead of an uncaught exception. |
Assessment
One valid finding worth fixing before merge (executor.ts, inline below); the other two valid findings are low-impact polish items; one was a false positive on verification.
| if (v1Content.$case === "raw") { | ||
| const bytes = | ||
| Buffer.isBuffer(v1Content.value) | ||
| ? (v1Content.value as Buffer).toString("base64") | ||
| : String(v1Content.value ?? ""); | ||
| results.push({ kind: "file", file: { bytes, mimeType, name: filename } }); | ||
| return results; | ||
| } |
There was a problem hiding this comment.
CodeRabbit CLI finding, verified: Buffer.isBuffer(v1Content.value) is false for a plain (non-Buffer) Uint8Array. In that branch, String(v1Content.value) produces a comma-separated decimal string (e.g. "1,2,3"), not base64 — this would corrupt the payload if the underlying gRPC/protobuf client ever hands back a plain Uint8Array instead of a Node Buffer for a bytes field. Suggest converting through Buffer.from(...) for any Uint8Array before the base64 encode, regardless of whether it is specifically a Buffer instance.
AliceLJY
left a comment
AliceLJY
left a comment
There was a problem hiding this comment.
Thanks for the thorough spike. I won't repeat DrumRobot's points. I checked this statically against @a2a-js/sdk@1.0.1 and found two regressions in the direction that matters most for existing users: current peers calling into an upgraded gateway.
-
Inbound v0.3 is not enabled. In SDK 1.0.1,
agentCardHandleronly serves the compat card (withurl/protocolVersion) throughlegacyAgentCardRouter, andjsonRpcHandleronly routesmessage/sendtoLegacyJsonRpcTransportHandler, whenlegacyCompat: { enabled: true }is passed.index.ts:506and:516pass neither. Result:- A peer still on current master gets a card with no
url. - A raw v0.3
message/sendgets-32601 Invalid method.
Listing
protocolVersion: "0.3"insupportedInterfacesdoesn't help, because v0.3 clients don't read that field. The compat-matrix tests that usedmessage/sendwere rewritten toSendMessage, so nothing covers this path any more. Please enablelegacyCompaton the card and JSON-RPC handlers (and REST, if you intend to support it), and add tests that (a) fetch the card with noA2A-Versionheader and asserturlis present, and (b) POST a v0.3message/sendand get a result. - A peer still on current master gets a card with no
-
agentIdrouting between two upgraded gateways is lost.client.ts:257now putsagentIdinmessage.metadata, which is correct. ButpickAgentId(executor.ts:56-60) still reads only the top-levelmsg.agentId, so the receiver always falls back to the default agent. Please readmetadata.agentIdtoo, and add a round-trip test. -
Minor: the description says the suites need
--test-force-exit, but thepackage.jsontestscript is unchanged, and that is what CI runs. Please confirm plainnpm testfinishes on Node 22 and 25, or update the script.
Closes #95.
What
Migrates the gateway client from
@a2a-js/sdk0.3.x to 1.0.1 while keeping full backward compatibility with v0.3 peers:12eca55— close gateway executor resources on shutdown (fixes the exit-hang observed in the test matrix; see Interop failure with A2A v1.0 peers: client-side message/send hangs (one-directional) — proposal: dual-protocol support via @a2a-js/sdk 1.x #95 discussion)1de58aa— core type migration to SDK 1.0.1 (~60 type errors resolved: protobuf enums,$casePart unions, wrapped execution events, v1 Agent Cards,TaskStore.list(),SendMessageRequest)7826453— dual-stack negotiation: agent card advertisessupportedInterfaces[]with JSONRPC v1.0 first, v0.3 second for legacy negotiation; client auto-detects peer version from the fetched card and speaks v1 (SendMessage) or falls back to a direct legacy v0.3 JSON-RPC path (sendViaLegacyJsonRpc) since SDK 1.x transports reject v0.3 cards3ee7bdf—scripts/verify-e2e.mts, a manual dual-stack smoke test that boots a real SDK 1.x handler withlegacyCompatenabledVerification
npx tsc --noEmit: cleanscripts/verify-e2e.mts):Notes
compat/v0_3was chosen over a hard v1 cutover so existing 0.3 peers keep working.message/sendshape; itsmessage/streamworks. Documented in Interop failure with A2A v1.0 peers: client-side message/send hangs (one-directional) — proposal: dual-protocol support via @a2a-js/sdk 1.x #95 for attribution clarity.