feat: Add GRPC support #44
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @danielblignaut, 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 expands the A2A JS SDK's communication capabilities by introducing a gRPC transport layer. This change provides a more performant and strongly-typed alternative to the existing JSON-RPC, leveraging Protocol Buffers for service and message definitions. It also improves overall code quality and maintainability through new linting and formatting tools, and enhances type safety across the codebase.
Highlights
- New gRPC Transport Implementation: Introduced a gRPC transport layer for the A2A JS SDK, enabling strongly typed, streaming communication between agents and clients. This provides an alternative to the existing JSON-RPC transport.
- Protocol Buffer Integration: Added
buf.gen.yamlto configurebuffor generating gRPC client and server code from thea2a.protospecification. This includes generated TypeScript interfaces and classes for the A2A service and standard Google protobuf types (Empty,Struct,Timestamp). - Core Dependency Additions: Incorporated
@grpc/grpc-js,@protobuf-ts/runtime, and@protobuf-ts/runtime-rpcas core dependencies to support the new gRPC functionality. BiomeJS has also been added as a development dependency for code formatting and linting. - Enhanced Type Safety and Code Quality: Updated
tsconfig.jsonto enable stricter TypeScript checks (strict,strictNullChecks) and introduced abiome.jsonconfiguration for consistent code style and linting across the project. - Comprehensive Documentation and Example: Included a detailed
README.mdwithin the gRPC transport directory, explaining its overview, usage, service methods, authentication, type conversion, and error handling. Anexample.tsfile is also provided to demonstrate how to set up and use the A2A gRPC service.
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.
Code Review
This pull request introduces gRPC support, a significant and well-structured feature. The code is clean, and the inclusion of documentation and examples is excellent. I've provided feedback focused on improving error handling, validation, and the implementation of stubbed methods to enhance the robustness of the new transport layer.
| async listTaskPushNotificationConfig( | ||
| call: ServerUnaryCall< | ||
| ListTaskPushNotificationConfigRequest, | ||
| ListTaskPushNotificationConfigResponse | ||
| >, | ||
| callback: sendUnaryData<ListTaskPushNotificationConfigResponse>, | ||
| ): Promise<void> { | ||
| try { | ||
| // For now, we'll return a single config if it exists | ||
| const taskId = extractTaskIdFromParent(call.request.parent); | ||
| const params: TaskIdParams = { id: taskId }; | ||
|
|
||
| try { | ||
| const config = | ||
| await requestHandler.getTaskPushNotificationConfig(params); | ||
| const response: ListTaskPushNotificationConfigResponse = { | ||
| configs: [convertPushNotificationConfigToGrpc(config)], | ||
| nextPageToken: "", | ||
| }; | ||
| callback(null, response); | ||
| } catch (error) { | ||
| // If no config exists, return empty list | ||
| const response: ListTaskPushNotificationConfigResponse = { | ||
| configs: [], | ||
| nextPageToken: "", | ||
| }; | ||
| callback(null, response); | ||
| } | ||
| } catch (error) { | ||
| callback(handleError(error)); | ||
| } |
There was a problem hiding this comment.
The implementation for listTaskPushNotificationConfig swallows errors from requestHandler.getTaskPushNotificationConfig and returns an empty list. Modify the error handling to only catch 'config not found' scenarios and propagate other errors.
| function convertPushNotificationConfigToGrpc( | ||
| config: A2ATaskPushNotificationConfig, | ||
| ): TaskPushNotificationConfig { | ||
| const pushConfig = config.pushNotificationConfig; | ||
| return { | ||
| name: `tasks/${config.taskId}/pushNotificationConfigs/${pushConfig.id}`, | ||
| pushNotificationConfig: { | ||
| id: pushConfig.id || "", | ||
| url: pushConfig.url, | ||
| token: pushConfig.token || "", | ||
| authentication: pushConfig.authentication | ||
| ? { | ||
| schemes: pushConfig.authentication.schemes || [], | ||
| credentials: pushConfig.authentication.credentials || "", | ||
| } | ||
| : undefined, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The resource name for the TaskPushNotificationConfig is constructed using pushConfig.id. Add validation to ensure pushConfig.id is a non-empty string before constructing the name to avoid invalid resource names.
| async deleteTaskPushNotificationConfig( | ||
| _call: ServerUnaryCall<DeleteTaskPushNotificationConfigRequest, Empty>, | ||
| callback: sendUnaryData<Empty>, | ||
| ): Promise<void> { | ||
| try { | ||
| // The current interface doesn't support deletion, so we'll return success | ||
| // This might need to be implemented in the request handler | ||
| callback(null, {}); | ||
| } catch (error) { | ||
| callback(handleError(error)); | ||
| } | ||
| }, |
There was a problem hiding this comment.
The deleteTaskPushNotificationConfig method is implemented as a stub that always returns success. Return an UNIMPLEMENTED gRPC status code to signal that the feature is not available.
async deleteTaskPushNotificationConfig(
_call: ServerUnaryCall<DeleteTaskPushNotificationConfigRequest, Empty>,
callback: sendUnaryData<Empty>,
): Promise<void> {
callback({
code: status.UNIMPLEMENTED,
message: "deleteTaskPushNotificationConfig is not implemented",
});
},| function convertMessageSendParamsToInternal(request: SendMessageRequest): MessageSendParams { | ||
| return { | ||
| message: convertGrpcMessageToInternal(request.request!), | ||
| configuration: request.configuration ? convertGrpcMessageConfigurationToInternal(request.configuration) : undefined, | ||
| metadata: request.metadata ? structToObject(request.metadata) : undefined, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The non-null assertion operator (!) is used on request.request. Add an explicit check for null or undefined for more robust code.
function convertMessageSendParamsToInternal(request: SendMessageRequest): MessageSendParams {
if (!request.request) {
throw A2AError.invalidParams("request field is missing in SendMessageRequest");
}
return {
message: convertGrpcMessageToInternal(request.request),
configuration: request.configuration ? convertGrpcMessageConfigurationToInternal(request.configuration) : undefined,
metadata: request.metadata ? structToObject(request.metadata) : undefined,
};
}| } as DataPart; | ||
| } | ||
|
|
||
| throw new Error("Invalid part type"); |
There was a problem hiding this comment.
Throwing a generic Error for an "Invalid part type" will result in a gRPC INTERNAL error status. Throw an A2AError.invalidParams instead to provide a more informative INVALID_ARGUMENT status to the client.
throw A2AError.invalidParams("Invalid part type");| }] | ||
| ); | ||
|
|
||
| server.bindAsync('0.0.0.0:50051', credentials, callback); |
There was a problem hiding this comment.
The callback variable used in this TLS example is not defined. Replace it with a callback function, similar to the insecure example above it.
| const cancelUpdate: TaskStatusUpdateEvent = { | ||
| kind: 'status-update', | ||
| taskId, | ||
| contextId: "", |
There was a problem hiding this comment.
The contextId is hardcoded to an empty string. Use the actual contextId of the task being cancelled for consistency and correctness. Consider updating the AgentExecutor.cancelTask signature to receive the RequestContext or the contextId.
| contextId: "", | |
| contextId: "", // TODO: This should be the actual contextId of the task |
| "strict": true, | ||
| "strictNullChecks": true, | ||
| "strictPropertyInitialization": false, |
There was a problem hiding this comment.
Consider enabling strictPropertyInitialization to ensure class properties are initialized either in the constructor or with a default value.
1 participant
Description
This is a draft PR to solicit feedback on adding GRPC support to the JS sdk. It comes with a few side-effects, mainly due to the usage / restrictions on the
timostamm-protobuf-tsgenerator.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 #<issue_number_goes_here> 🦕