refactor(integrations): migrate A2A SDK to 1.1.2 - #488
Conversation
67ce331 to
8be36a4
Compare
8be36a4 to
a846ec5
Compare
Blocking: per-alias
|
| alias | /.well-known/agent-card.json |
POST /message:stream |
|---|---|---|
weather-agent (first registered) |
200 | 200 |
uuid-weather |
200 | 404 |
billing-agent |
200 | 404 |
uuid-billing |
200 | 404 |
Impact: the REST streaming endpoint works for exactly one peer, and never for a UUID alias. A single-peer gateway still loses its UUID alias. This is the endpoint advertised in README.md:611, examples/a2a_gateway/README.md, examples/a2a_gateway/01_basic_gateway.py:90,99, and examples/run_agent.py:983.
Why CI is green
Every test in test_server.py uses one peer (build_server(), line 42), and the only multi-alias test — test_uuid_peer_alias_serves_the_same_agent_card (line 106) — hits the card route, which is in the group that isn't shadowed. Nothing exercises a second peer's REST route.
Test to reproduce
Add to tests/integrations/a2a/gateway/test_server.py (reuses the existing FakeExecutor and make_peer):
def build_multi_peer_server() -> GatewayServer:
weather = make_peer("uuid-weather", "Weather Agent", "Gets weather info")
billing = make_peer("uuid-billing", "Billing Agent", "Answers billing questions")
return GatewayServer(
peers={"weather-agent": weather, "billing-agent": billing},
peers_by_uuid={weather.id: weather, billing.id: billing},
gateway_url="http://localhost:10000",
port=10000,
executor_factory=lambda _slug: FakeExecutor(),
)
@pytest_asyncio.fixture
async def multi_peer_client() -> AsyncIterator[httpx.AsyncClient]:
transport = ASGITransport(app=build_multi_peer_server()._build_app())
async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
yield client
async def test_rest_binding_is_reachable_for_every_peer_and_alias(
multi_peer_client: httpx.AsyncClient,
) -> None:
"""Every peer must serve the REST route the gateway docs advertise.
A gateway hosting more than one peer is the normal case, and each peer is
addressable by slug and by UUID, so all four routes have to answer.
"""
aliases = ("weather-agent", "uuid-weather", "billing-agent", "uuid-billing")
reached_handler = {}
for alias in aliases:
response = await multi_peer_client.post(
f"/agents/{alias}/message:stream",
headers={"A2A-Version": "1.0"},
json={
"message": {
"messageId": "message-1",
"role": "ROLE_USER",
"parts": [{"text": "Hello"}],
}
},
)
reached_handler[alias] = response.status_code
assert reached_handler == dict.fromkeys(aliases, 200)On this branch it fails with:
Differing items:
{'uuid-weather': 404} != {'uuid-weather': 200}
{'billing-agent': 404} != {'billing-agent': 200}
{'uuid-billing': 404} != {'uuid-billing': 200}
(weather-agent: 200 is the one matching item.)
Suggested fix
This gateway namespaces peers by path and isn't multi-tenant, so the tenant mounts can be dropped — in server.py:119:
rest_routes.extend(
route
for route in create_rest_routes(
handler,
enable_v0_3_compat=True,
path_prefix=f"/agents/{alias}",
)
# Each call also returns a catch-all "/{tenant}" mount for multi-tenant
# hosts. Peers here are namespaced by path, and the first mount would
# shadow every later peer's REST routes.
if not isinstance(route, Mount)
)plus from starlette.routing import BaseRoute, Mount, Route.
Verified: with that change the new test passes and the rest of the suite is unaffected (36 passed).
Related, separate from the above: /v1/ is the legacy REST path
Worth fixing while in here. Under a2a-sdk 1.1.2 the two REST paths map to different protocol versions:
| Path | Protocol |
|---|---|
/agents/<peer>/message:stream |
1.0 |
/agents/<peer>/v1/message:stream |
v0.3 compat |
Posting with A2A-Version: 1.0 to the /v1/ path returns:
400 — A2A version '1.0' is not supported by this handler. Expected version '0.3'.
So the /v1 prefix reads like a newer route but is the legacy binding. examples/a2a_gateway/README.md currently tells 1.0 clients to "use the versioned route" /v1/message:stream with a SendMessageRequest wrapper — a 1.0 client needs /agents/<peer>/message:stream instead. Same path appears in README.md:611, examples/a2a_gateway/01_basic_gateway.py:90,99, and examples/run_agent.py:983.
Also note test_rest_stream_runs_through_upstream_handler covers /message:stream, so the /v1 path the docs advertise has no test.
🤖 Review generated with Claude Code
Follow-up: secondary findingsSeparate from the routing bug above. Each of these was reproduced by running it against 1.
|
| peers | top-level routes |
|---|---|
| 1 | 53 |
| 2 | 105 |
| 5 | 261 |
| 50 | 2601 |
Two aliases × (1 card + 1 legacy card + 1 JSON-RPC + 23 REST). Starlette matches linearly, so every request on a 50-peer gateway walks a 2601-entry list, and each peer also carries 2 handlers + 2 task stores. A single parameterized /agents/{peer_id} route set with peer resolution inside the executor would collapse this and fix item 3 at the same time.
7. Timeout path logs success
gateway/adapter.py:326-331. _await_response swallows TimeoutError, so the else: branch runs anyway:
WARNING A2A response timed out: room=room-1 task=b16ba967… timeout=0.01s
INFO A2A request completed: room=room-1 task=b16ba967…
Smaller items
protocol.py:94—is_terminal_state()is dead code; no caller insrc,tests, orexamples.server.py:42—self.peers_by_uuidis now write-only; aliasing is derived frompeer.idin_build_appinstead.integrations/a2a/adapter.py:174—getattr(event, "HasField", lambda _name: False)("message").StreamResponsealways hasHasField, and all six_handle_eventtests pass real protobuf events, so nothing motivates the guard. It's also a trap:MagicMock().HasField("message")is truthy, so a future mock-based test would silently take the message branch. Plainevent.HasField("message")reads as what's meant.gateway/types.py:86—def _message(self, content: str):is missing its return type.server.py:147—_handle_list_peersre-importsJSONResponsealthough it's already imported at line 20; the signature is also(_request: Any) -> Anywhere(request: Request) -> JSONResponseis available.gateway/adapter.py:298—_establish_requestruns before thetryat line 306, so peer-not-found and room-creation failures skip the structuredlogger.exceptionthe rest of the method provides.CLAUDE.md:209-211— the A2A key-files table still lists onlytypes.py; the newprotocol.pyandconfig.pyaren't there, andresponse_timeout_sisn't documented outside the README snippet.test_adapter.py:80—test_timeout_is_adapter_configurationmostly asserts the constructor stored its argument.test_timeout_returns_terminal_failureis the one carrying the weight.
Verified as fine
Worth recording so they don't get re-litigated:
- Dropping
_list_peers_page_with_retryis safe. The Fern client's_should_retryalready covers{429, 408, 409}and 5xx with jittered backoff honoringRetry-After/RateLimit-Reset, andDEFAULT_REQUEST_OPTIONS = {"max_retries": 3}is threaded through. Fewer attempts than the old five, but it respects server-supplied backoff, which the hand-rolled loop did not. - Gateway cancellation is correct.
ActiveTask.cancelcallsself._producer_task.cancel()beforeagent_executor.cancel, so_execute_a2a'sCancelledErrorhandler runs andpending_task'sfinallyreleases the room slot. raise InternalError()/raise UnsupportedOperationError()in the examples is valid — in 1.1.2 these derive fromA2AError(Exception), so dropping theServerErrorwrapper is right.PendingA2ATask's lock +doneguard correctly makefail/complete/cancelidempotent and mutually exclusive.
🤖 Review generated with Claude Code
amit-gazal-thenvoi
left a comment
There was a problem hiding this comment.
Request changes
The A2A 1.1.2 migration is the right direction — leaning on the upstream route factories and deleting ~3200 lines of hand-rolled JSON-RPC/SSE is a clear win, protocol.py's reducer is well factored, and the aliasing test in test_protocol.py is the kind that catches real regressions. One blocker and a few correctness issues before it lands.
Everything below was reproduced by running it against a2a-sdk==1.1.2, not read off the diff.
Blocking
- Per-alias
Mount("/{tenant}")shadows the REST binding for every peer after the first, so/agents/<peer>/message:streamworks for exactly one peer and never for a UUID alias. Repro test + one-line fix inline onserver.py.
Should fix before merge
GET /agents/<peer>/tasksreturns conversation content unauthenticated — new exposure vs. the pre-PR server._handle_eventlost itstry/finally, stranding a completedtask_idon the room when delivery fails.- Per-alias task stores make a task invisible across a peer's own aliases (surfaces once the mount bug is fixed).
- The new
httpx.AsyncClientis never closed;cleanup_all()is the hook, notstop().
Smaller — _task_cache leak, contradictory timeout log, ~52 routes per peer, dead is_terminal_state() / peers_by_uuid, the getattr(HasField) guard, missing return type, redundant import.
Not inline-able
CLAUDE.md:209-211— the A2A key-files table still lists onlytypes.py; the newprotocol.pyandconfig.pyaren't there, andresponse_timeout_sisn't documented outside the README snippet.- Docs point 1.0 clients at the wrong path: under 1.1.2,
/agents/<peer>/v1/message:streamis the v0.3 binding (A2A-Version: 1.0there returns400 - A2A version '1.0' is not supported by this handler. Expected version '0.3'). 1.0 clients need/agents/<peer>/message:stream. AffectsREADME.md:611,examples/a2a_gateway/README.md,examples/a2a_gateway/01_basic_gateway.py:90,99,examples/run_agent.py:983.test_rest_stream_runs_through_upstream_handlercovers/message:stream, so the documented/v1path has no test. test_adapter.py—test_timeout_is_adapter_configurationmostly asserts the constructor stored its argument;test_timeout_returns_terminal_failureis the one carrying the weight.
Verified as fine, so they don't get re-litigated: dropping _list_peers_page_with_retry is safe (Fern's _should_retry already covers {429, 408, 409} + 5xx with Retry-After-aware backoff, max_retries=3 threaded through); gateway cancellation is correct (ActiveTask.cancel cancels the producer before calling agent_executor.cancel, so pending_task's finally releases the slot); raise InternalError() in the examples is valid since 1.1.2 errors derive from A2AError(Exception); and PendingA2ATask's lock + done guard correctly make the terminal transitions idempotent.
Detail on each of these is in the two comments above.
🤖 Review generated with Claude Code
…le review findings
- Drop the upstream multi-tenant "/{tenant}" mount and the unauthenticated
task/push-config REST routes: peers are namespaced by path, so the first
alias's mount shadowed every later alias's REST binding, and the task
routes disclosed room content with no auth. The gateway now serves only
the messaging endpoints and cards.
- Share one request handler (and task store) per peer across its slug and
UUID aliases, so tasks stay visible on both and the route table shrinks.
- Restore the terminal-task finally: a failed Band delivery still persists
the terminal task event and releases the room's task_id.
- Release owned clients on shutdown: A2AAdapter.cleanup_all closes the A2A
client and its httpx transport; the gateway's dead stop() hook is now
cleanup_all so Agent.stop() actually stops the hosted server.
- Report timeouts truthfully (no "completed" log after a timeout), log
request-setup failures with the same structure as execution failures,
reclaim resubscribed tasks from the cache on room cleanup, drop the
write-only peers_by_uuid parameter and dead is_terminal_state helper,
and fix small typing/import nits.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011Y2bdwA8CxTkD9SXxfU8et
…e migration coverage Self-review round before peer re-review: - Close the JSON-RPC half of the unauthenticated task surface: the dispatcher served ListTasks/push-config/extended-card for every caller under one shared task-store identity. The gateway now allowlists the messaging methods plus per-task operations gated by the server-minted task UUID (ALLOWED_JSONRPC_METHODS), in both 1.0 and v0.3 spellings. - Stop the gateway server by asking uvicorn to exit instead of cancelling serve(): cancellation skipped shutdown and leaked the listening socket (rebind failed with EADDRINUSE). - Recognize pre-migration terminal state names on rehydration (TERMINAL_TASK_STATE_NAMES), so rooms with 0.x history don't resubscribe to finished tasks. - Fail in-flight gateway requests on cleanup_all, and reject a concurrent room request without leaking the internal room id to the remote client. - Close the resubscribe stream deterministically (aclosing) and use real proto presence checks for status messages. - Fix the three newly-authored gateway curl examples (all failed against the shipped server: v0.3 payload shapes on the 1.0 parser, and the /v1/ route mislabeled as the 1.0 REST binding), the mixed README extra, and the demo orchestrator's shared-client close in discover_peer. - Cap a2a-sdk <2 at all pin sites. - Restore coverage the migration dropped: auth-plumbing identity, history converter round trip, remote/failed-task error paths, context continuity, bootstrap gating, legacy terminal values, resubscribe failure tolerance, PendingA2ATask terminal idempotency, and the closed JSON-RPC method surface. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Y2bdwA8CxTkD9SXxfU8et
Self-review round (25db472)Ran a deep verification pass over the full diff before requesting re-review. Everything below was reproduced empirically before fixing: Security — JSON-RPC task exposure (completes the earlier REST fix): the REST filtering in f4985f3 closed only half the surface. The JSON-RPC dispatcher still served Socket leak on stop: cancelling Docs that didn't work: all three newly-authored gateway curl commands failed against the shipped server (v0.3 payload shapes fed to the 1.0 parser → Hardening: pre-migration (0.x) terminal state names recognized on rehydration; Coverage restored (dropped by the test rewrite, behaviors still live): auth-plumbing identity into 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Migration itself checks out — I verified every changed call against a real a2a-sdk==1.1.2 install (ClientFactory.create_from_url, send_message(SendMessageRequest), subscribe(SubscribeToTaskRequest(id=...)), async Client.close(), new_text_message kwargs, TaskUpdater.start_work/complete/failed/cancel, RequestContext id generation, and InternalError/UnsupportedOperationError now being real exceptions). Locally: 4279 passed / 82 skipped, ruff clean, pyrefly 0 errors, README markdown-docs green.
One blocking item below (the a2a_gateway extra can't import, one line to fix), one question, one bit of praise.
Summary
a2a-sdk>=1.1.2,<2using official route factories, helpers, and task lifecycle APIs.Retry-After-aware backoff.Validation
uv run pytest tests/ --ignore=tests/integration/ --ignore=tests/e2e/ -q --no-covuv run ruff check .uv run ruff format --check .uv run pyrefly checka2a_gatewayanda2a_gateway_demoextras.