-
Notifications
You must be signed in to change notification settings - Fork 482
feat: adka2a should provide a http.Handler #459
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
base: main
Are you sure you want to change the base?
feat: adka2a should provide a http.Handler #459
Conversation
Add NewHandler function that creates and returns an http.Handler for the A2A JSON-RPC API. This follows the same pattern as the adkrest package, providing a convenient way to create an A2A handler without having to manually create the executor and wrap it with a2asrv handlers. Fixes google#419 Signed-off-by: majiayu000 <1835304752@qq.com>
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 enhances the 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 a new http.Handler for the A2A JSON-RPC API, simplifying the setup for consumers of this package. The implementation is clean and follows existing patterns. My review includes a minor suggestion to improve the test code by removing a redundant check. Overall, the changes look good and improve the library's API.
server/adka2a/handler_test.go
Outdated
| if _, ok := handler.(http.Handler); !ok { | ||
| t.Fatal("NewHandler() did not return an http.Handler") | ||
| } |
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.
server/adka2a/handler.go
Outdated
| func NewHandler(config HandlerConfig) http.Handler { | ||
| executor := NewExecutor(config.ExecutorConfig) | ||
| reqHandler := a2asrv.NewHandler(executor, config.A2AOptions...) | ||
| return a2asrv.NewJSONRPCHandler(reqHandler) |
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 very opinionated towards JSONRPC, I will revisit this PR after we confirm the intention behind the issue reported. Thank you for your contribution
|
@baptmont Updated the change to be transport-agnostic: |
|
I think it's a very shallow utility, which is close to a simple re-export of a function provided by a2a SDK. Developers will need to import |
This addresses the review feedback by providing a higher-level API that
eliminates the need for users to import and understand the a2asrv package.
Changes:
- NewJSONRPCHandler: Returns http.Handler directly (recommended for most users)
- NewServeMux: Returns complete http.ServeMux with agent card + invoke endpoints
- NewRequestHandler: Kept for custom transport layers (gRPC, etc.)
- Updated HandlerConfig to include optional AgentCard field
- Added comprehensive tests for all new functions
- Updated example to use the simplified NewServeMux API
Before (required understanding a2asrv):
```go
executor := adka2a.NewExecutor(config)
requestHandler := a2asrv.NewHandler(executor)
mux.Handle("/invoke", a2asrv.NewJSONRPCHandler(requestHandler))
mux.Handle(a2asrv.WellKnownAgentCardPath, a2asrv.NewStaticAgentCardHandler(card))
```
After (one-liner):
```go
mux := adka2a.NewServeMux(adka2a.HandlerConfig{
ExecutorConfig: config,
AgentCard: card,
})
```
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
|
@yarolegovich Thanks for the feedback! I've refactored the implementation to address your concern. The key insight from your comment was that returning In this update, I've taken a different approach with a layered API:
The value proposition:
// Before: 5 lines, requires a2asrv import
mux := http.NewServeMux()
mux.Handle(a2asrv.WellKnownAgentCardPath, a2asrv.NewStaticAgentCardHandler(card))
executor := adka2a.NewExecutor(config)
requestHandler := a2asrv.NewHandler(executor)
mux.Handle("/invoke", a2asrv.NewJSONRPCHandler(requestHandler))
// After: 1 call, no a2asrv import needed
mux := adka2a.NewServeMux(adka2a.HandlerConfig{...})
handler := adka2a.NewJSONRPCHandler(config,
a2asrv.WithTaskStore(customDB),
a2asrv.WithPushNotifications(store, sender),
)This follows the same pattern as |
Virt10n01
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.
locale?
Diff.txt
Summary
This PR adds convenient
http.Handlerconstructors to theadka2apackage, addressing the feedback in #419.New Functions
NewJSONRPCHandlerhttp.HandlerNewServeMux*http.ServeMuxNewRequestHandlera2asrv.RequestHandlerDesign Rationale
The key insight from the initial review was that returning
a2asrv.RequestHandlerdoesn't provide enough value—users still need to importa2asrvand wrap it themselves. This revision 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
🤖 Generated with Claude Code