fix(tck): enqueue Task before TaskStatusUpdateEvent in SUT agent - #1165
fix(tck): enqueue Task before TaskStatusUpdateEvent in SUT agent#1165kuangmi-bit wants to merge 7 commits into
Conversation
🧪 Code Coverage (vs
|
|
Gentle ping on review. This is a small, TCK-unblocking server-side change (part of #666): |
… TCK SUT Two changes that together let the 1.0 TCK exercise the JSON-RPC SUT: 1. create_jsonrpc_routes now registers both the exact rpc_url and its trailing-slash variant. HTTP clients (httpx in particular) normalize an empty request path to a trailing slash, so POST /a2a/jsonrpc/ was previously 404 even though /a2a/jsonrpc worked. This is a protocol compatibility fix: the spec does not mandate one spelling over the other, and a 404 on the trailing-slash form breaks any client that does not strip it. 2. tck/sut_agent.py now enqueues the Task itself (via new_task_from_user_message) before emitting TaskStatusUpdateEvents. The SDK's active-task machinery requires this ordering (InvalidAgentResponseError otherwise), and the 1.0 TCK CORE-SEND tests assert it. Verified against a2a-tck 1.0.0.alpha2 (jsonrpc, must level): 53 failed -> 6 failed before this change, with the remaining failures being SUT feature gaps (artifacts) and one SDK error-code mapping gap, not transport issues.
…card
Two more 1.0 compatibility fixes surfaced by running the REST and gRPC
rows of the TCK:
- protocolBinding 'REST' -> 'HTTP+JSON': the 1.0 TCK's protocol binding
map only recognizes JSONRPC / GRPC / HTTP+JSON. The old name made the
whole REST transport untestable ("No usable transports after filtering").
- gRPC interface url 'http://localhost:50051' -> 'localhost:50051': the
gRPC client treats the url as a channel target; the http:// prefix
fails DNS resolution in grpcio.
Verified against a2a-tck 1.0.0.alpha2 (must level):
- jsonrpc: 6 failed / 67 passed
- http_json (REST): 5 failed / 61 passed
- grpc: 7 failed / 48 passed
Remaining failures are SUT feature gaps (artifact-carrying responses,
MessageResponse variants) plus two status/error-code mappings.
f49d8e4 to
860bd65
Compare
mykytanetipa
left a comment
mykytanetipa
left a comment
There was a problem hiding this comment.
Retracting my earlier approval - it was submitted in error while I was reviewing itk PRs in parallel. Switching to request-changes; see the points below.
| ), | ||
| Route( | ||
| path=f'{rpc_url}/', | ||
| endpoint=dispatcher.handle_requests, | ||
| methods=['POST'], | ||
| ), |
There was a problem hiding this comment.
major: adding a duplicated route is a behavioral change for existing clients that rely on public create_jsonrpc_routes, I would avoid this.
the route mismatch issue should be fixed from the clients side e.g. by using follow_redirects=True on the httpx.Client.
| ) | ||
| ), | ||
| Route( | ||
| path=f'{rpc_url}/', |
There was a problem hiding this comment.
will introduce malformed unreachable path in case of rpc_url='/'
…route
Review feedback (mykytanetipa): the duplicated trailing-slash route was a
behavioral change for callers of create_jsonrpc_routes and broke rpc_url='/'
(f'{rpc_url}/' -> '//', used by the FastAPI mount).
Revert the server-side route; the mismatch is a client redirect-following
issue: Starlette's default redirect_slashes 307s the trailing-slash variant,
and 307 preserves method+body, so following it is safe for JSON-RPC POSTs.
The factory-created httpx client now sets follow_redirects=True (users
supplying their own client keep their policy).
Regression tests: default client follows redirects; custom client policy
respected.
Signed-off-by: kuangmi-bit <kuangmi@gmail.com>
|
@mykytanetipa — both points are right, thank you for catching them. Reworked per your direction (head What changed
One honest caveatThe original 53→6 TCK improvement came from the server answering the trailing-slash spelling directly. The client that actually hits that spelling in the TCK run is the a2a-tck harness, not the SDK: The task-enqueue fix in the SUT ( |
|
@mykytanetipa — gentle nudge for a re-review when you have a moment. Both points from your 08-20 review are addressed at head
Per your earlier guidance, the test-harness half of the fix (httpx |
mykytanetipa
left a comment
mykytanetipa
left a comment
There was a problem hiding this comment.
not as important, but isn't the TCK's JSON-RPC client constructed directly rather than through a2a's ClientFactory? (https://github.com/a2aproject/a2a-tck/blob/5996b79f9cefa6fc390980e383e358a66fb9e49e/tck/transport/jsonrpc_client.py#L81-L85)
| httpx_client = config.httpx_client or httpx.AsyncClient( | ||
| follow_redirects=True | ||
| ) |
There was a problem hiding this comment.
This is a behavioral change for existing clients.
The default now follows all redirects, not only the intended /x -> /x/ case. A server that previously returned a 301/302 (which the client used to surface as a redirect response) will now be transparently followed, and for 301/302 httpx downgrades POST -> GET and drops the body. Affects existing client that depended on seeing the 3xx e.g. custom redirect handling, or asserting on status codes.
| endpoint=dispatcher.handle_requests, | ||
| methods=['POST'], | ||
| ) | ||
| ), |
There was a problem hiding this comment.
minor: trailing comma, please remove
| # variant of the JSON-RPC/REST endpoint (Starlette's default | ||
| # redirect_slashes). 307 preserves method and body, so this is safe | ||
| # for POST payloads and keeps the client robust to either spelling. | ||
| httpx_client = config.httpx_client or httpx.AsyncClient( |
There was a problem hiding this comment.
major: do we need to change the default here? Anyone who wants redirect-following can already opt in today without this PR:
ClientFactory(ClientConfig(httpx_client=httpx.AsyncClient(follow_redirects=True)))
This is what I ment previously by saying "the route mismatch issue should be fixed from the clients side"
Per review: changing the default redirect policy in ClientFactory is a behavioral change for existing clients. Revert to the previous default (opt-in via httpx_client=httpx.AsyncClient(follow_redirects=True)). The harness-side follow for the trailing-slash case lands in a2a-tck (follow redirects in harness HTTP clients). Also drop the trailing comma flagged as minor. Keep the Task-enqueue fix in tck/sut_agent.py.
kuangmi-bit
changed the title
|
@mykytanetipa — thanks for both rounds of review, and good catch on the TCK client construction. Reworked at head What changed
On your 08-30 questionCorrect — the TCK's JSON-RPC client is constructed directly (
So the remaining diff is a 4-line SUT correctness fix with no public-API impact. Appreciate a re-review when you have a moment. |
## Summary The JSON-RPC and HTTP+JSON harness clients construct `httpx.Client` without following redirects (`follow_redirects` defaults to `False` in httpx), and both POST to `"/"` (i.e. `base_url + "/"`). When a SUT's framework redirects the trailing-slash variant — Starlette's default `redirect_slashes` answers it with a **307**, which preserves method and body — the harness treats the SUT as unreachable even though the endpoint answers at the canonical path. This is the harness-side half of a2aproject/a2a-python#1165 (SDK client already follows redirects; the TCK's own clients didn't). ## Change - `tck/transport/jsonrpc_client.py` — `follow_redirects=True` on the httpx client - `tck/transport/http_json_client.py` — `follow_redirects=True` on the httpx client Verification: `make lint` clean, `make unit-test` 253 passed. Signed-off-by: kuangmi-bit <kuangmi@gmail.com>
| protocol_version='1.0.0', | ||
| ), | ||
| AgentInterface( | ||
| url=f'http://localhost:{grpc_port}', |
There was a problem hiding this comment.
wont this break current TCK CI on a2a 0.3.x (e.g. assertions like https://github.com/a2aproject/a2a-tck/blob/0.3.0.beta3/tests/optional/capabilities/test_agent_card_optional.py#L286)?
This will leave CI both not working on 0.3.x and 1.x.x, I think we should submit the full 1.x.x migration work at the same time (i.e. with TCK_VERSION v1) but it would include more changes. (please provide explanation, I may lack context on the TCK migration plan)
|
@mykytanetipa — thank you, you're right. PR description updated at head
No SDK client defaults are touched. Appreciate a re-review when you have a moment. |
|
@mykytanetipa — gentle nudge for a re-review when you have a moment. Since your last COMMENTED rounds (08-30/08-31), both points are addressed at head
The 08-20 CHANGES_REQUESTED was noted as an approval retraction ("submitted in error"), so this is just a request for a fresh look at the narrowed scope. Thanks for the review bandwidth. |
2 participants
Summary
Single-file fix to the TCK SUT (
tck/sut_agent.py) that unblocks the A2A 1.0 TCK conformance run (part of #666):Enqueue the Task before emitting
TaskStatusUpdateEvent. The 1.0 SDK's active-task machinery requires the Task object to be enqueued first (it raisesInvalidAgentResponseErrorotherwise), and the 1.0 TCKCORE-SEND-*requirements assert this ordering. When the request context does not yet carry a task, the SUT now creates one vianew_task_from_user_messageand enqueues it before processing the message.AgentInterface metadata aligned with the 1.0 spec: the HTTP interface now reports
protocol_binding='HTTP+JSON'(the 1.0 binding name) instead of the legacy'REST', and the gRPC interface URL is a barehost:porttarget (no scheme prefix), matching what the SDK's gRPC channel expects.Verification
Ran the 1.0 TCK (
a2a-tcktag1.0.0.alpha2, jsonrpc transport, must level) against the SUT locally:The remaining 6 failures are not transport issues — they are SUT feature gaps (artifact-carrying responses, DM-ART-001) and one SDK error-code mapping gap (
ContentTypeNotSupportedErrorreported asParseError). Those are tracked as follow-ups; this PR removes the transport-level blockers.Scope notes
ClientFactorybehavior is identical tomain.Related
follow_redirects=True)