-
Notifications
You must be signed in to change notification settings - Fork 482
feat(adka2a): add http.Handler constructors for simplified A2A server setup #460
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
… setup
This adds three new functions to the adka2a package:
- NewJSONRPCHandler: Returns a ready-to-use http.Handler for A2A JSON-RPC
- NewServeMux: Returns a complete http.ServeMux with agent card and invoke endpoints
- NewRequestHandler: Returns a transport-agnostic handler for custom transports
These functions provide a more ergonomic API that eliminates the need for
users to import and understand the a2asrv package for common use cases.
Before:
```go
executor := adka2a.NewExecutor(config)
requestHandler := a2asrv.NewHandler(executor)
mux.Handle("/invoke", a2asrv.NewJSONRPCHandler(requestHandler))
mux.Handle(a2asrv.WellKnownAgentCardPath, a2asrv.NewStaticAgentCardHandler(card))
```
After:
```go
mux := adka2a.NewServeMux(adka2a.HandlerConfig{
ExecutorConfig: config,
AgentCard: card,
})
```
Fixes google#419
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @majiayu000, 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 developer experience for setting up A2A (Agent-to-Agent) servers by introducing high-level Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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.
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 convenient http.Handler constructors that significantly simplify setting up an A2A server. The new layered API (NewServeMux, NewJSONRPCHandler, NewRequestHandler) is well-designed, providing flexibility for different use cases. The code is well-documented and includes a comprehensive set of unit tests. The example usage in examples/a2a/main.go clearly demonstrates the improvement in ergonomics.
My review includes a few suggestions to further improve the code:
- Refactoring to reduce code duplication in the new handlers.
- Improving test robustness by handling potential errors and adding more thorough verification.
Overall, this is a great contribution that improves the developer experience.
| executor := NewExecutor(config.ExecutorConfig) | ||
| requestHandler := a2asrv.NewHandler(executor, opts...) | ||
| return a2asrv.NewJSONRPCHandler(requestHandler) |
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.
To improve maintainability and reduce code duplication, NewJSONRPCHandler can be simplified by calling NewRequestHandler. Both functions currently duplicate the logic for creating an executor.
| executor := NewExecutor(config.ExecutorConfig) | |
| requestHandler := a2asrv.NewHandler(executor, opts...) | |
| return a2asrv.NewJSONRPCHandler(requestHandler) | |
| return a2asrv.NewJSONRPCHandler(NewRequestHandler(config, opts...)) |
| t.Errorf("agent card endpoint returned status %d, want %d", rec.Code, http.StatusOK) | ||
| } | ||
|
|
||
| body, _ := io.ReadAll(rec.Body) |
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.
It's a good practice to handle errors, even in tests. The error from io.ReadAll is currently ignored. If io.ReadAll fails, json.Unmarshal will likely fail with a less informative error message. Please check the error to make the test more robust.
body, err := io.ReadAll(rec.Body)
if err != nil {
t.Fatalf("failed to read response body: %v", err)
}| // The returned handler should be usable with different transports | ||
| // Wrap it with JSON-RPC transport to verify | ||
| jsonrpcHandler := a2asrv.NewJSONRPCHandler(handler) | ||
| if jsonrpcHandler == nil { | ||
| t.Fatal("a2asrv.NewJSONRPCHandler() returned nil") | ||
| } |
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 test verifies that NewRequestHandler and a2asrv.NewJSONRPCHandler don't return nil, but it doesn't actually test if the resulting handler works. To make this test more meaningful, consider adding an HTTP request to verify that the handler responds correctly, similar to what's done in TestNewJSONRPCHandler.
// The returned handler should be usable with different transports
// Wrap it with JSON-RPC transport to verify
jsonrpcHandler := a2asrv.NewJSONRPCHandler(handler)
if jsonrpcHandler == nil {
t.Fatal("a2asrv.NewJSONRPCHandler() returned nil")
}
// Verify the handler responds to JSON-RPC requests
reqBody := `{"jsonrpc":"2.0","method":"message/send","params":{"message":{"role":"user","parts":[{"type":"text","text":"hello"}]}},"id":"1"}`
req := httptest.NewRequest(http.MethodPost, "/a2a/invoke", strings.NewReader(reqBody))
req.Header.Set("Content-Type", "application/json")
rec := httptest.NewRecorder()
jsonrpcHandler.ServeHTTP(rec, req)
if rec.Code != http.StatusOK {
t.Errorf("handler returned status %d, want %d", rec.Code, http.StatusOK)
}
Summary
This PR adds convenient
http.Handlerconstructors to theadka2apackage, addressing the feedback in #419 and providing a more ergonomic alternative to PR #459.New Functions
NewJSONRPCHandlerhttp.Handlerfor A2A JSON-RPC transportNewServeMuxhttp.ServeMuxwith both agent card and invoke endpointsNewRequestHandlerDesign Rationale
The key insight from the review of #459 was that returning
a2asrv.RequestHandlerdoesn't provide enough value—users still need to importa2asrvand wrap it themselves. This PR takes a different approach:Complete abstraction:
NewJSONRPCHandlerandNewServeMuxreturn standard library types (http.Handler/*http.ServeMux), so users don't need to know abouta2asrvat all.Layered API: Three functions at different abstraction levels:
NewServeMux- Highest level, one-liner server setupNewJSONRPCHandler- Mid level, for custom routingNewRequestHandler- Low level, for custom transportsConsistency with adkrest: Follows the same pattern as
adkrest.NewHandler.Before/After Comparison
Before (current):
After (with this PR):
Test Plan
examples/a2a/main.goto use new APIRelated Issues
Fixes #419
Supersedes #459
🤖 Generated with Claude Code