feat: implement server http+json #142
Conversation
tchapacan
force-pushed
the
feat/implement-http+json
branch
from
ab133cc to
530d5db
Compare
|
Thanks a lot @tchapacan for looking into this! Appreciate your effort to align this proposal with the existing infrastructure. I actually think that current way of wiring it up with express followed in this repo worth changing to make it more modular and explicit, i.e. const app = express();
// const a2aRequestHandler = ...
// const agentCard = ...
app.use('/.well-known/agent-card.json', agentCardHandler(agentCard));
app.use('/a2a/jsonrpc', jsonRpcHandler(a2aRequestHandler));
app.use('/a2a/api', restHandler(a2aRequestHandler));This is just a sketch, but the idea is to provide ready to use routers which can be registered independently by the integration code, currently In addition, current JSON-RPC implementation uses framework-agnostic Similar applies to client, JSON-RPC specific implementation has to be factored out into some sort of a "transport" abstraction which can be implemented for REST as well (see Python). I'll try to prepare this as well. And last but not least, we're setting up conformance tests from here: #159. Once SUT agent is ready, it'll be possible to validate REST transport as well. |
|
Hello @ishymko ! No problem hope it helped a little, at least starting the job for REST support, and opening the discussion! Yep, I like it too! It seems more modular and easier to understand/maintain in the end. And going with some modular middleware can be compatible with other framework such as fastify see this https://github.com/fastify/fastify-express. I was thinking about going with the framework agnostic too, following the jsonRcpTransportHandler. But in the end I choose to leverage fully of express framework which does everything we need (routing, method, etc..), only caveats was the route like Agree for the client too, I’m not really happy with my implementation even if it was working, but it deserves some love to make it cleaner. Good for the conformance test in the CI! Once this will be implemented, you can take advantage of this draft PR to test it. If you need any help don’t hesitate, on the client part or server part, depending of my time I’ll be happy to help! |
# Description As a preparation step for REST support proposed in #142, extract Express.js handlers from `A2AExpressApp` to allow assembling your own app using Express.js API directly for specifying individual paths and middlewares, rather than wrapping it in `A2AExpressApp`: ```ts // const a2aRequestHandler = . . . const app = express(); // Minimal example: app.use(`/${AGENT_CARD_PATH}`, agentCardHandler({ agentCardProvider: a2aRequestHandler })); app.use(jsonRpcHandler({ requestHandler: a2aRequestHandler })); // Advanced example: app.use(myCustomMiddleware()); app.use('/.well-known/custom-agent-card.json', myCustomAgentCardMiddleware, agentCardHandler({ agentCardProvider: a2aRequestHandler })); app.use('/a2a/jsonrpc', myJsonRpcMiddleware, jsonRpcHandler({ requestHandler: a2aRequestHandler })); app.use('/a2a/api', restHandler({ requestHandler: a2aRequestHandler })); ``` No logic changes to existing handlers, they are moved as is. Also `A2AExpressApp` is switched to use new handlers without behavior changes. **Note:** new functions **are not** exported via `index.ts` yet, it will be done together with updating docs and examples once REST support is done. Re #137
|
Hi @tchapacan, thank you for you patience! At this point server-side implementation should be good to proceed as #162 is merged and we also have TCK tests running. Could you please merge with main and factor out the server side support in the same fashion as in #162? As you proposed in the description, splitting server and client here indeed will help a lot, so let's start with the server as client is not yet switched to multi-transport types. Also please make sure to integrate with TCK tests. It should probably work with minimal changes to CLI arguments when SUT agent is updated with REST transport in agent card. |
tchapacan
force-pushed
the
feat/implement-http+json
branch
4 times, most recently
from
71285cd to
2e758e7
Compare
|
Hello @ishymko! Thanks for following it up with #162 and the TCK testing, it seems clear! I've rebased on main and tried to implement the server-side REST support following the same pattern as #162. Here's what I have done: Server Side REST support:
TCK adaptation:
Something I would like to highlight while I was testing my implementation with the TCK test : I encountered an issue: the TCK cli for REST requests use snake_case (e.g., Implement a transformation middleware in (
Even with this modification, I have some inconsistency between my local run of a2a-tck which are passing : And the one in the CI that are failing example => https://github.com/a2aproject/a2a-js/actions/runs/19397175092/job/55498766100?pr=142 Might need your help about this to understand what I'm doing wrong ? Performance Impact:
With middleware : Without middleware : I'm open to different approaches here and would like your guidance for the next steps:
Let me know what you think would be best! Happy to adjust the implementation, reorganize the code (convention naming etc..) and tests following your recommendations. But first I need a little help related to the a2a-tck issue I'm having. |
tchapacan
force-pushed
the
feat/implement-http+json
branch
5 times, most recently
from
0746260 to
3343f3a
Compare
|
Thank you @tchapacan! This is a very solid work! I'll get to review it properly once we have the testing sorted out, but I agree with the general approach. On TCK (modified example This should also make use of additional interfaces you defined in the agent card, as transport specific base URLs should be discovered automatically from it without defining them explicitly as SUT URL. As for the differences between CI run and local run: are you sure you're running against the same version of TCK locally? CI uses On snake_case/camelCase |
|
Hi @tchapacan, as a heads up we've recently merged ESLint and Prettier integration (#183, #185) which caused substantial diffs across the whole codebase. Those are only formatting and "unused imports" kind of changes, so you should be good with just resolving all the conflicts in favor of your local state and applying |
# Description Currently `A2AClient` defined in `client.ts` uses JSON-RPC types in its interface, i.e. `sendMessage` uses [`SendMessageResponse`](https://github.com/a2aproject/a2a-js/blob/e7e8f35d5d5356d30a13131dea0caff97be8cd53/src/client/client.ts#L207) which contains [JSON-RPC fields like `id` and `jsonrpc` version](https://github.com/a2aproject/a2a-js/blob/e7e8f35d5d5356d30a13131dea0caff97be8cd53/src/types.ts#L2177-L2190). It was also the case for Python SDK which required introducing a new [client.py](https://github.com/a2aproject/a2a-python/blob/main/src/a2a/client/client.py) keeping the old one as [legacy.py](https://github.com/a2aproject/a2a-python/blob/main/src/a2a/client/legacy.py) for backward compatibility (see [#348](a2aproject/a2a-python#348)). As a first step of introducing a transport-agnostic client, extract JSON-RPC specific logic into a transport abstraction and switch existing `A2AClient` to it in a backward compatible way to take advantage of existing tests. **Note:** new types are not exported from `index.ts`, this will be done once new client is ready. # Changes 1. Define `A2ATransport` abstraction without using JSON-RPC types. 2. Implement `JsonRpcTransport` from existing `A2AClient`. 3. Use new `JsonRpcTransport` from `A2AClient` - new transports are **not going** to be added into this client as it uses JSON-RPC types. This is done in a backward compatible way, existing behavior is preserved: returning JSON-RPC errors as objects, populating JSON-RPC specific fields. 4. Define shared transport-agnostic errors for A2A specific errors defined in [the spec](https://a2a-protocol.org/latest/specification/#82-a2a-specific-errors). Re #137, #142
|
Hello @ishymko Thanks for the heads-up! Will do for the rebase and the linting no problem about that! I'm able to reproduce the behavior of the tck CI run by using the correct tag 0.3.0.beta3 locally! I missed the details to switch to the correct tag my bad sorry! I was able to dig a little further and it seems the object message send by the tck cli is not same as the one expected by the server. Using the Following the cli arguments you recommended and by performing some adjustment into the agentcard by following more rigorously this part of the spec, here what I got now :
Using tag 0.3.0.beta3 (failing):
Using main branch (all good):
I will remove the transformation middleware in my next commits/push and try my best to propose some relevant type for REST, I'll work on it in the next days. |
tchapacan
force-pushed
the
feat/implement-http+json
branch
6 times, most recently
from
0a8cc83 to
fde96cd
Compare
|
Hello @tchapacan! Great catch on main vs 0.3.0.beta3! Seems like indeed it was using incorrect payload ( Line 125 in 9d72082 undefined.find and it got fixed in main. I don't see any new releases created in TCK, setting main in the workflow is fragile, as it'll be floating, can we set it to the fix commit directly for now (actions/checkout should support it)?
|
tchapacan
force-pushed
the
feat/implement-http+json
branch
2 times, most recently
from
e5aaa8b to
8821ab8
Compare
tchapacan
force-pushed
the
feat/implement-http+json
branch
from
8821ab8 to
b66a396
Compare
|
Hello @ishymko! I have sticked to this commit for the tck ref in the workflow while waiting for a new tag there, no problem it works very well! I have removed the "magic" middleware, and tried to implement some custom snake-case types for REST (added in For an express api generally I like to define my schema using zod library, infer my types from the schema (feature from zod) and perform the request validation using the schema. Some transformation can be done as well. But I didn't want to introduce an external library, and doing it natively for the complexity we have seems acceptable. If that's ok for you, I was thinking about extracting some part of the logic into the LMK if it's ok to switch this PR to open for review as we are now or if it still need some work before. I suppose documentation will have to be updated as well, but don't know if we do it in this PR or in an another one, I can of course help! |
|
Hello @tchapacan! Thank you very much for all your work and being responsive!
Unfortunately we can't modify
Yes, exactly!
Sounds reasonable to me! We can always change it later if manual validation becomes too complex.
Sounds good to me, let's try to do it.
Feel free to do it once you're ready with the items above and assign me as a reviewer. On the documentation, yes, for now let's skip it, I was planning to do it closer to the next version release to keep the main readme consistent. One can always switch tags to see relevant documentation, but given significant changes, don't want to confuse people with the main page readme opened by default which reflects unreleased version (and I don't want to maintain a dedicated branch for this as well). |
tchapacan
force-pushed
the
feat/implement-http+json
branch
from
d2f8516 to
d7de73d
Compare
tchapacan
force-pushed
the
feat/implement-http+json
branch
from
d7de73d to
c1455ab
Compare
|
Hello @ishymko! You are welcome, no problem always happy to stay up to date and contribute/help when possible! To summarize latest change :
Make sense for the docs, It can be done later (just before a future release). I'm opening the PR for review but it seems I can't add you as reviewer, so poking you here. Don't hesitate to challenge it, specifically convention naming or code organization, I'm open for any changes. Let's not forget the version of tck as well that is sticked into the github workflow currently to this commit, if it goes in I can open a small issue for that to avoid It's forgotten later. |
tchapacan
force-pushed
the
feat/implement-http+json
branch
from
c1455ab to
56ee1a0
Compare
There was a problem hiding this comment.
Hello @tchapacan, thanks for wiring up everything together, this is a solid work!
First pass done, added a few comments on potential reshuffling things a bit, also let's double check SSE/streaming implementation as for some reason it fails TCK test (the one we don't run now, you'll find more details in the specific comment).
Let's not forget the version of tck as well that is sticked into the github workflow currently to this commit, if it goes in I can open a small issue for that to avoid It's forgotten later.
Sounds good, let's create a follow-up once this one is merged, thank you!
| # TODO once we have the TCK for 0.4.0 we will need to look at the branch to decide which TCK version to run. | ||
| # Tag of the TCK | ||
| TCK_VERSION: 0.3.0.beta3 | ||
| TCK_VERSION: 601bf0d6f63baafb079c2ab23f53d73ce03e5461 |
There was a problem hiding this comment.
Let's add a comment here why it was done, maybe referencing this PR comment(s) with our discussion?
| }) | ||
| ); | ||
| router.use(`/${agentCardPath}`, agentCardHandler({ agentCardProvider: this.requestHandler })); | ||
| router.use(httpRestHandler({ requestHandler: this.requestHandler })); |
There was a problem hiding this comment.
Let's remove it from the A2AExpressApp to avoid registering unexpected new routes by default. We're going to move to the new handlers to be explicit and deprecate the A2AExpressApp.
| if (err instanceof SyntaxError && 'body' in err) { | ||
| const a2aError = A2AError.parseError('Invalid JSON payload.'); | ||
| return res.status(400).json({ | ||
| error: a2aError.toJSONRPCError(), |
There was a problem hiding this comment.
We shouldn't be using JSON-RPC errors here, current v0.3 spec doesn't specify the payload of HTTP+JSON errors, but using JSON-RPC still doesn't feel right to me. Let's follow Python implementation for now.
The v1.0 is going to specify it, see in the draft here and here. We will support it when getting ready for 1.0 (and when the final spec version is published).
Applies to other places using toJSONRPCError().
| /** | ||
| * @deprecated Use httpRestHandler instead. | ||
| */ | ||
| export function createHttpRestRouter(requestHandler: A2ARequestHandler): Router { | ||
| return httpRestHandler({ requestHandler }) as Router; | ||
| } |
There was a problem hiding this comment.
Looks like a leftover, can we remove it?
| const ROUTE_PATTERN = { | ||
| MESSAGE_ACTION: /^\/v1\/message:(send|stream)$/i, | ||
| TASK_ACTION: /^\/v1\/tasks\/([^/:]+):([a-z]+)$/i, | ||
| } as const; |
There was a problem hiding this comment.
Let's consider simplifying it so that single router.get/route.post handles one method to make the logic more explicit and avoid advanced regex matching. Code duplication between similar "groups" of actions can be extracted into helper methods.
| export type { JsonRpcHandlerOptions } from './json_rpc_handler.js'; | ||
| export { agentCardHandler } from './agent_card_handler.js'; | ||
| export type { AgentCardHandlerOptions, AgentCardProvider } from './agent_card_handler.js'; | ||
| export { httpRestHandler, restErrorHandler } from './http_rest_handler.js'; |
There was a problem hiding this comment.
restErrorHandler should stay internal, it's a part of httpRestHandler.
| export { httpRestHandler, restErrorHandler } from './http_rest_handler.js'; | |
| export { httpRestHandler } from './http_rest_handler.js'; |
There was a problem hiding this comment.
Can we actually introduce a class here similar to JSON-RPC which will perform the type conversion and some validation (like for history length and also capabilities check)? This way it should be possible to shrink down the express part more, the purpose of express part will be to create a transport specific object (i.e. assemble data from body and query params) and pass it down here, where it's going to be converted into the "domain" object, validated and processed.
| } | ||
|
|
||
| // ============================================================================ | ||
| // Server-Sent Events (SSE) Support |
There was a problem hiding this comment.
Is it possible to reuse SSE code from jsonrpc_transport_handler.ts? There is an issue for this #175. If it gets too troublesome we can tackle it later, either way as long as it's not exported we can reorganize any time.
| } | ||
|
|
||
| // ============================================================================ | ||
| // Server-Sent Events (SSE) Support |
There was a problem hiding this comment.
I've tried to run TCK capabilities tests and got SSE/streaming for REST specifically (it passes for JSON-RPC). Currently we don't run capabilities in CI, only mandatory.
Failures:
FAILED tests/optional/capabilities/test_streaming_methods.py::test_message_stream_basic - AssertionError: assert 'taskId' in {'context_id': '065ae425-f162-4ab0-b618-18e7c29bcce3', 'final': False, 'kind': 'status-update', 'status': {'message': ... 'text', 'text': 'Processing your question'}], ...}, 'state': 'working', 'timestamp': '2025-11-26T10:45:11.634Z'}, ...}
FAILED tests/optional/capabilities/test_streaming_methods.py::test_sse_event_format_compliance - tck.transport.base_client.TransportError: [REST] Unexpected error in REST streaming: Event loop is closed
I run it against your branch (TCK against the commit set for CI in this PR) by:
cd tck && npm run tck:sut-agentuv run ./run_tck.py --sut-url http://localhost:41241/a2a/jsonrpc --category capabilities --transports jsonrpc,rest
Could you please check it? If it's some HTTP+JSON implementation bug and you'll be able to get it working, let's include them in CI (there are only two failures in this category). If it's a bug in TCK we can take another look and maybe report it in their repo.
There was a problem hiding this comment.
Can be moved to /transports I think, i.e. under a new subcatalog transports/rest together with the other file.
2 participants
Description
Add HTTP+REST ServerIde Transport Support
Some part of the code has been generated using generative AI to speed up the development, I have reviewed it carefully, but this should be taken into account into the review. Keep in mind this is my first time contributing to the repo and this is an attempt to help a2a-js support REST (HTTP+JSON) as per describe in the a2a documentation. I'll be happy to refacto it during review process, don't hesitate to challenge it.
Thank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
CONTRIBUTINGGuide.fix:which represents bug fixes, and correlates to a SemVer patch.feat:represents a new feature, and correlates to a SemVer minor.feat!:, orfix!:,refactor!:, etc., which represent a breaking change (indicated by the!) and will result in a SemVer major.Fixes #137 🦕