-
Notifications
You must be signed in to change notification settings - Fork 109
feat: implement server http+json #142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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. |
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. |
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. |
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)?
|
e5aaa8b to
8821ab8
Compare
91bd2c2 to
bc4fbe9
Compare
|
Helo @ishymko, to summarize :
If the camelCase/snake-case input is to flaky and add some unecessary complexity according to the next version of the spec that will authorize only camelCase, I can remove it no worry. |
ishymko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tchapacan, let's keep supporting both, I like it, I think it's reasonable given that the work was done already and it works in other SDKs now. As long as this adapter logic is not exported we're fine.
Left a few cleanup-like comments. I don't want to block on TCK SSE test failure as long as manual testing (via switching to per-session event) succeeds.
I am going to start updating the doc for upcoming release, will include this part as well (as it's easier to do everything together) and will tag you on the PR.
b72a614 to
09d03b1
Compare
|
helo @ishymko, to summarize latest change :
I will follow-up the issue I have opened here a2aproject/a2a-tck#99 and take the responsibility to create an another PR later, to reintroduce the transports method in the github workflow for the capabilities When the spec will evolve and if it make sense to restrict it to only camelCase as input and output, I can create an another PR in that sense if necessary. Thanks for all the time you took reviewing it! If you need help for the client transport part, I'll be happy to help as well ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @tchapacan for this contribution and your due diligence during review process! Good action items on TCK side emerged from it.
Some final nit comments and I am going to merge it for the upcoming release (current plan is to push a 0.3.6 version this week). I don't think it's critical to include client support in the same release as they are independent.
As for client transport, your help will be greatly appreciated!
09d03b1 to
d5a5033
Compare
d5a5033 to
7b3d0eb
Compare
|
No problem @ishymko always a pleasure to help and thanks for your review! Will follow-up my issue in TCK. I have fixed all your comment and sync rebase from main. Yes I don't see any issue to add server and client in different release, no problem! I will create an another PR then for the client part. I see a lot of job has already been done in term of refacto and pattern in place. I suppose we will need to add a And wire everything together following same pattern as for the |
|
@tchapacan thanks again, merging the PR! 🚀 As for the client support: correct, the same approach should be followed for REST. See reference PRs for JSON-RPC: #198, #169. Note that current JSON-RPC transport implementation has some compatibility hacks to be used from the old client, there is no need for it for HTTP+JSON. There is no TCK-like thing for client, but we have e2e.spec.ts to test both client and server assembled like external users would do it (using Could you please comment on #137 that you're planning to open a PR for client support so that I can "assign" this issue to you to avoid duplicated effort from the community? Thank you! |
# Description Follow-up for unreleased #142. [The spec](https://a2a-protocol.org/latest/specification/) uses [`HTTP+JSON/REST`](https://a2a-protocol.org/latest/specification/#11-httpjsonrest-protocol-binding) wording and other SDKs use just REST in all names (i.e. [Python](https://github.com/a2aproject/a2a-python/blob/main/src/a2a/server/request_handlers/rest_handler.py) and [Golang](https://github.com/a2aproject/a2a-go/blob/release/spec-v1/a2asrv/rest.go)).
🤖 I have created a release *beep* *boop* --- ## [0.3.6](v0.3.5...v0.3.6) (2025-12-10) ### Features * add support for extendedAgentCard on client side ([#234](#234)) ([3073376](3073376)) * Add support for extension headers on client side ([#227](#227)) ([8c57002](8c57002)) * implement client interceptors ([#223](#223)) ([5694c22](5694c22)) * Implement extended card support on server side ([#197](#197)) ([45014ac](45014ac)) * implement server http+json ([#142](#142)) ([f20e662](f20e662)) * introduce AgentCardResolver ([#225](#225)) ([ddaf7de](ddaf7de)) * introduce transport agnostic client ([#198](#198)) ([94a9848](94a9848)) * server side support for extensions ([5ef7396](5ef7396)) * support authentication on server side ([#195](#195)) ([9872d93](9872d93)) ### Bug Fixes * handle errors occurred in non-blocking sendMessage ([#187](#187)) ([e55c0f4](e55c0f4)) ### Miscellaneous Chores * set version to 0.3.6 ([#191](#191)) ([3f8cea0](3f8cea0)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: Ivan Shymko <ishymko@google.com>
# Description Following continuation of this PR #142, this one is related to the implementation of the REST mode support for the client part of **a2a-js** sdk. Following the same pattern as other sdk (agnostic transport). ---- 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: - [x] Follow the [`CONTRIBUTING` Guide](https://github.com/google-a2a/a2a-js/blob/main/CONTRIBUTING.md). - [x] Make your Pull Request title in the <https://www.conventionalcommits.org/> specification. - Important Prefixes for [release-please](https://github.com/googleapis/release-please): - `fix:` which represents bug fixes, and correlates to a [SemVer](https://semver.org/) patch. - `feat:` represents a new feature, and correlates to a SemVer minor. - `feat!:`, or `fix!:`, `refactor!:`, etc., which represent a breaking change (indicated by the `!`) and will result in a SemVer major. - [x] Ensure the tests and linter pass - [ ] Appropriate docs were updated (if necessary) Fixes #137, fixes #175 🦕 Release-As: 0.3.7 Co-authored-by: Ivan Shymko <ishymko@google.com>
Description
Add HTTP+REST ServerSide 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.Re #137 🦕
Release-As: 0.3.6