-
Notifications
You must be signed in to change notification settings - Fork 104
feat: Implement the spec updates for v0.2.4 and v0.2.5 #192
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
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.
Summary of Changes
Hello @fjuma, 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 updates the A2A client and server implementations to align with versions v0.2.4 and v0.2.5 of the A2A specification. The core changes revolve around enhancing push notification management by introducing new API methods for listing and deleting configurations, and refining the AgentCard structure to include protocol versioning and additional interface declarations.
Highlights
- Updated A2A Specification Alignment: The client and server implementations now fully support A2A spec versions
v0.2.4andv0.2.5. This includes updates to theAgentCardstructure to incorporateprotocolVersion,additionalInterfaces, andpreferredTransportfields, ensuring compatibility with the latest protocol definitions. - Enhanced Push Notification Management: New JSON-RPC methods (
tasks/pushNotificationConfig/listandtasks/pushNotificationConfig/delete) have been introduced. These allow agents to programmatically list all active push notification configurations for a given task and delete specific configurations, providing more granular control over task notifications. - Refactored Push Notification Backend: The server-side architecture for push notifications has been significantly modularized. The monolithic
PushNotifierhas been replaced by two distinct interfaces:PushNotificationConfigStorefor managing configuration storage (with an in-memory implementation provided) andPushNotificationSenderfor handling the actual dispatch of notifications. This improves maintainability and extensibility. - Improved Client API for Push Notifications: The
A2AClientnow offers overloaded methods forgetTaskPushNotificationConfigto support specifying a particular configuration ID. Additionally, new client methods have been added forlistTaskPushNotificationConfiganddeleteTaskPushNotificationConfig, providing a complete client-side interface for the new server capabilities. - Standardized Error Handling: Error handling has been refined across the codebase.
A2AServerExceptionnow consistently captures and propagates the underlying cause of exceptions.JSONRPCResponseconstructors have been updated for better type safety, especially for responses withVoidresults. A newPushNotificationNotSupportedErroris introduced for cases where an agent's capabilities do not include push notifications.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request introduces significant feature updates, aligning the codebase with A2A spec versions v0.2.4 and v0.2.5, primarily focusing on enhancing push notification handling with support for multiple configurations per task. The changes include refactoring the PushNotifier with PushNotificationConfigStore and PushNotificationSender, along with adding list and delete operations for push notification configurations. The code is generally well-structured, and the new features are accompanied by a comprehensive set of tests. The review comments suggest minor improvements for code clarity and correctness.
| private PushNotificationConfig getPushNotificationConfig(List<PushNotificationConfig> notificationConfigList, | ||
| String configId) { | ||
| if (configId != null) { | ||
| for (PushNotificationConfig notificationConfig : notificationConfigList) { | ||
| if (configId.equals(notificationConfig.id())) { | ||
| return notificationConfig; | ||
| } | ||
| } | ||
| } | ||
| return notificationConfigList.get(0); | ||
| } |
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.
When a specific configId is provided but not found, the current implementation falls back to returning the first available configuration. This could be misleading for the client, as it might receive a configuration it didn't ask for.
It would be more explicit and less error-prone to throw an error, such as InternalError or a more specific custom error, to indicate that the requested configuration was not found.
| private PushNotificationConfig getPushNotificationConfig(List<PushNotificationConfig> notificationConfigList, | |
| String configId) { | |
| if (configId != null) { | |
| for (PushNotificationConfig notificationConfig : notificationConfigList) { | |
| if (configId.equals(notificationConfig.id())) { | |
| return notificationConfig; | |
| } | |
| } | |
| } | |
| return notificationConfigList.get(0); | |
| } | |
| private PushNotificationConfig getPushNotificationConfig(List<PushNotificationConfig> notificationConfigList, | |
| String configId) throws JSONRPCError { | |
| if (configId != null) { | |
| for (PushNotificationConfig notificationConfig : notificationConfigList) { | |
| if (configId.equals(notificationConfig.id())) { | |
| return notificationConfig; | |
| } | |
| } | |
| throw new InternalError("Push notification config with id '" + configId + "' not found."); | |
| } | |
| return notificationConfigList.get(0); | |
| } |
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.
This is intentional. The configId is optional. The spec says that the server "will return one of the associated configurations if config id is not specified". The python implementation returns the first one in this case, so we've done the same.
| Iterator<PushNotificationConfig> notificationConfigIterator = notificationConfigList.iterator(); | ||
| while (notificationConfigIterator.hasNext()) { | ||
| PushNotificationConfig config = notificationConfigIterator.next(); | ||
| if (config.id().equals(notificationConfig.id())) { | ||
| notificationConfigIterator.remove(); | ||
| break; | ||
| } | ||
| } |
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.
The while loop with an iterator to find and remove an element can be simplified using List.removeIf(). This is more concise and idiomatic in modern Java (8+).
| Iterator<PushNotificationConfig> notificationConfigIterator = notificationConfigList.iterator(); | |
| while (notificationConfigIterator.hasNext()) { | |
| PushNotificationConfig config = notificationConfigIterator.next(); | |
| if (config.id().equals(notificationConfig.id())) { | |
| notificationConfigIterator.remove(); | |
| break; | |
| } | |
| } | |
| notificationConfigList.removeIf(config -> config.id().equals(notificationConfig.id())); |
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.
List.removeIf() will check every item in the list but we don't need to do this. We can simply break from the loop once we've found the right config.
| Iterator<PushNotificationConfig> notificationConfigIterator = notificationConfigList.iterator(); | ||
| while (notificationConfigIterator.hasNext()) { | ||
| PushNotificationConfig config = notificationConfigIterator.next(); | ||
| if (configId.equals(config.id())) { | ||
| notificationConfigIterator.remove(); | ||
| break; | ||
| } | ||
| } |
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.
Similar to the setInfo method, this while loop with an iterator can be simplified by using List.removeIf(). This will make the code more readable and modern.
| Iterator<PushNotificationConfig> notificationConfigIterator = notificationConfigList.iterator(); | |
| while (notificationConfigIterator.hasNext()) { | |
| PushNotificationConfig config = notificationConfigIterator.next(); | |
| if (configId.equals(config.id())) { | |
| notificationConfigIterator.remove(); | |
| break; | |
| } | |
| } | |
| notificationConfigList.removeIf(config -> configId.equals(config.id())); |
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.
Same here, the loop allows us to break once we've found the right push notification config.
| } | ||
| } | ||
|
|
||
| protected void deletePusNotificationConfigInStore(String taskId, String configId) throws Exception { |
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.
There's a typo in the method name deletePusNotificationConfigInStore. It should be deletePushNotificationConfigInStore.
Correcting this will improve readability and consistency. Please also update all call sites of this method.
| protected void deletePusNotificationConfigInStore(String taskId, String configId) throws Exception { | |
| protected void deletePushNotificationConfigInStore(String taskId, String configId) throws Exception { |
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.
Fixed
… align with v0.2.4
… type is Void to be able to handle DeleteTaskPushNotificationConfigResponse correctly
…ication configurations
…ication configurations
kabir
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.
The main things standing out to me are:
a) TCK_VERSION in .github/workflows/run-tck.yml should be updated to v0.2.5
b) We need a v0.2.5 tag of the TCK
Or all PRs will fail the TCK going forward
I've updated the TCK version to v0.2.5. Note that the tck job will fail until the tag is available. |
# Description This PR brings in the updates required to align with the A2A v0.2.4 and v0.2.5 spec versions. - [X] Follow the [`CONTRIBUTING` Guide](../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 pass - [X] Appropriate READMEs were updated (if necessary) Fixes a2aproject#183 🦕
Description
This PR brings in the updates required to align with the A2A v0.2.4 and v0.2.5 spec versions.
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 #183 🦕