feat: method extensions prototyping #138
Conversation
Summary of ChangesHello @yarolegovich, 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 introduces a foundational framework for method extensions within the 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a well-designed mechanism for method extensions, allowing for custom RPC-style methods to be added to the A2A protocol. The changes are comprehensive, touching both client and server components, and include a clear example. The separation of concerns with Method, Binding, and transport-specific implementations is clean. My review includes a few suggestions to improve robustness, clarity, and error handling, such as preventing a potential panic with safer type assertions and improving diagnostic messages.
| func (m *unaryServerMethod[Arg, Res]) InvokeUnary(ctx context.Context, arg any) (any, error) { | ||
| return m.call(ctx, arg.(*Arg)) | ||
| } |
There was a problem hiding this comment.
The direct type assertion arg.(*Arg) will cause a panic if arg is not of the expected type *Arg. This could be triggered by a malformed request or a bug in a transport implementation, leading to a server crash. It's much safer to use the "comma ok" idiom to check the type and return an error if it's incorrect.
func (m *unaryServerMethod[Arg, Res]) InvokeUnary(ctx context.Context, arg any) (any, error) {
typedArg, ok := arg.(*Arg)
if !ok {
return nil, errors.New("internal error: unexpected argument type")
}
return m.call(ctx, typedArg)
}| typedBinding, ok := binding.(*jsonrpcExtensionBinding) | ||
| if !ok { | ||
| return nil, fmt.Errorf("method %s is not bound to JSON-RPC", method.Name()) | ||
| } |
There was a problem hiding this comment.
The error message here is identical to the one for the check above, which can be misleading. If the binding exists but has the wrong type, the error should reflect that instead of saying the method is not bound.
| typedBinding, ok := binding.(*jsonrpcExtensionBinding) | |
| if !ok { | |
| return nil, fmt.Errorf("method %s is not bound to JSON-RPC", method.Name()) | |
| } | |
| typedBinding, ok := binding.(*jsonrpcExtensionBinding) | |
| if !ok { | |
| return nil, fmt.Errorf("method %s has incorrect JSON-RPC binding type: got %T", method.Name(), binding) | |
| } |
| GetAgentCard(ctx context.Context) (*a2a.AgentCard, error) | ||
|
|
||
| // Invoke calls the provided extension method. | ||
| Invoke(ctx context.Context, client a2aext.Method, arg any) (any, error) |
There was a problem hiding this comment.
For clarity and consistency, consider renaming the client parameter to method. The parameter is of type a2aext.Method, which represents a method definition, not a client instance. This change would align with its usage in implementations and call sites.
| Invoke(ctx context.Context, client a2aext.Method, arg any) (any, error) | |
| Invoke(ctx context.Context, method a2aext.Method, arg any) (any, error) |
| return nil, errNotImplemented | ||
| } | ||
|
|
||
| func (unimplementedTransport) Invoke(ctx context.Context, client a2aext.Method, arg any) (any, error) { |
There was a problem hiding this comment.
For consistency with the Transport interface, please rename the client parameter to method here as well.
| func (unimplementedTransport) Invoke(ctx context.Context, client a2aext.Method, arg any) (any, error) { | |
| func (unimplementedTransport) Invoke(ctx context.Context, method a2aext.Method, arg any) (any, error) { |
| // GetAgentCard returns an extended a2a.AgentCard if configured. | ||
| OnGetExtendedAgentCard(ctx context.Context) (*a2a.AgentCard, error) | ||
|
|
||
| // GetAgentCard returns an extended a2a.AgentCard if configured. |
There was a problem hiding this comment.
This comment appears to be a copy-paste error from OnGetExtendedAgentCard. It should be updated to describe what GetExtensionMethods does.
| // GetAgentCard returns an extended a2a.AgentCard if configured. | |
| // GetExtensionMethods returns all registered extension methods. |
| if !ok { // fail? | ||
| continue |
There was a problem hiding this comment.
The // fail? comment indicates uncertainty. Silently continuing on a binding type mismatch during initialization can hide a configuration error. It would be better to log a warning to make developers aware of the potential issue, as this is part of the server setup.
| if !ok { // fail? | |
| continue | |
| if !ok { | |
| log.Warn(context.Background(), "extension method has incompatible JSON-RPC binding type", "method", method.Name()) | |
| continue | |
| } |
yarolegovich
changed the title
A prototyping attempt to address #129
Some notes:
Still need to think of a good way for do it with gRPC.