Conversation
Summary of ChangesHello @tchapacan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the client's communication capabilities by integrating a new REST client. This addition provides an alternative, HTTP-based transport layer to interact with agents, complementing the existing JSON-RPC. The change involves the creation of dedicated REST transport classes, a shared utility for parsing Server-Sent Events, and updates to the client factory to support the new transport by default. The overall impact is increased flexibility and broader compatibility for client-agent interactions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new REST transport client, which is a significant and well-structured feature. The implementation in src/client/transports/rest_transport.ts is solid, and the refactoring of Server-Sent Events (SSE) parsing logic into a reusable sse_parser.ts module is a great improvement for code maintainability. The parameterization of end-to-end tests to cover both JSON-RPC and REST transports is also a commendable effort to ensure robustness. My review identifies a high-severity bug in getTaskPushNotificationConfig that could lead to incorrect behavior, and a medium-severity issue in setTaskPushNotificationConfig concerning redundant data in the request body. Addressing these points will enhance the correctness and clarity of the new REST client.
d868812 to
cbb56da
Compare
|
Hello @ishymko ! Following our last discussion in this PR #142, this my proposition for REST support in the client. To summarize :
Not sure about the error handling, but this might be where the backward compability is for the jsonrpc, so I've simplified and be more straightforward for the rest one, so LMK if that's ok this way. |
ishymko
left a comment
There was a problem hiding this comment.
Hello again @tchapacan, thank you for a quick follow-up with the client part of HTTP+JSON/REST transport!
Left a few minor comments and should be good to go, well done 👍
I choose to go with only camelCase for the client, LMK if it's ok (I know server support both, but this is to be more aligned with the spec)
Yep, agree.
Not sure about the error handling, but this might be where the backward compability is for the jsonrpc, so I've simplified and be more straightforward for the rest one, so LMK if that's ok this way.
Looks good to me.
88a9fec to
67ee60f
Compare
|
Helo @ishymko , 2 open questions crossed my mind while re-reading the PR.
What's your opinion regarding those 2? |
ishymko
left a comment
There was a problem hiding this comment.
Hi @tchapacan, added some final minor comments and answered questions.
On your questions:
- Great question. I believe it's not fully accurate in the spec now, spec operates on
/v1...absolute paths, so strictly speaking combining them with something likehttps://example.com/a2a/rest/should erase the path on the base URL according to the rules most libraries use which is going to result inhttps://example.com/v1...witha2a/restremoved. I think it makes sense to protect from this to be more developer friendly. Let's trim trailing slashes from the base URL provided by the user before concatenating, something like.replace(/\/+$/, ""). - Another great question. I tend to agree that we should relax strictness for better compatibility with implementations "in the wild". Created #264, let's address it separately.
2b2d48f to
b4aac37
Compare
b4aac37 to
f5f9bb6
Compare
|
Helo @ishymko , I've tried to fixed all the comment and rebase LMK if that's good. I have added as well the trim of the trailing slash for the endpoint and tests for it. According to this job https://github.com/a2aproject/a2a-js/actions/runs/20286295879/job/58260634940?pr=258 it seems that the token used might not have the correct scope, or might be a permission issue, maybe because coming from a fork ? for the case insensitive comparison #264 I can have a look later if no one yet plan to work on it |
🧪 Code Coverage
Generated by coverage-comment.yml |
ishymko
left a comment
There was a problem hiding this comment.
Thank you @tchapacan, LGTM! 🚀
Sorry for confusion with the failing coverage workflow, this was fixed in #270 and it passes on your PR now.
And of course your contribution is going to be welcome on #264 💯
🤖 I have created a release *beep* *boop* --- ## [0.3.7](v0.3.6...v0.3.7) (2025-12-17) ### Features * add rest client ([#258](#258)) ([96be3a1](96be3a1)) * remove EventEmitter dependency to support Edge Runtime ([#219](#219)) ([6c76fef](6c76fef)), closes [#218](#218) ### Bug Fixes * export transport agnostic errors from client ([#272](#272)) ([23cd42e](23cd42e)) * pass ServerCallContext to getAuthenticatedExtendedAgentCard for REST ([#274](#274)) ([89b141b](89b141b)), closes [#137](#137) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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:
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, fixes #175 🦕
Release-As: 0.3.7