Implement Anthropic Messages API support #496
Conversation
Summary of ChangesHello @ericcurtin, 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 system's API compatibility by implementing full support for the Anthropic Messages API. It establishes a dedicated handler and routing mechanisms to seamlessly integrate Anthropic-formatted requests for chat completions and token counting. By routing these requests through the existing scheduler to 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
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- In
handler_test.go, the subtests inTestWriteAnthropicErrorcallt.Parallel()inside afor _, tt := range testsloop without capturingtt, which can cause data races on the loop variable; addtt := ttinside the loop or removet.Parallel()from the subtests. - In
proxyToBackend, all errors frommodelManager.GetLocalare returned as anot_found_error; consider distinguishing between 'model not found' vs other lookup failures (e.g., storage or internal errors) so the Anthropic error type and HTTP status better reflect the underlying issue. - In
writeAnthropicError,h.logis assumed to be non-nil when callingErrorf; to avoid potential panics if the handler is constructed differently in the future, either guard the log call with a nil check or ensure the Handler always holds a non-nil no-op logger.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `handler_test.go`, the subtests in `TestWriteAnthropicError` call `t.Parallel()` inside a `for _, tt := range tests` loop without capturing `tt`, which can cause data races on the loop variable; add `tt := tt` inside the loop or remove `t.Parallel()` from the subtests.
- In `proxyToBackend`, all errors from `modelManager.GetLocal` are returned as a `not_found_error`; consider distinguishing between 'model not found' vs other lookup failures (e.g., storage or internal errors) so the Anthropic error type and HTTP status better reflect the underlying issue.
- In `writeAnthropicError`, `h.log` is assumed to be non-nil when calling `Errorf`; to avoid potential panics if the handler is constructed differently in the future, either guard the log call with a nil check or ensure the Handler always holds a non-nil no-op logger.
## Individual Comments
### Comment 1
<location> `pkg/anthropic/handler.go:45-46` </location>
<code_context>
+ }
+
+ // Register routes
+ for route, handler := range h.routeHandlers() {
+ h.router.HandleFunc(route, handler)
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Registering routes including the HTTP method with http.ServeMux likely breaks routing.
`http.ServeMux` patterns match only the URL path, not a leading HTTP method string (e.g. `"POST "`). Since `routeHandlers` returns keys like `"POST " + APIPrefix + "/v1/messages"`, these handlers will never match requests (which use paths like `/anthropic/v1/messages`). Register handlers using only the path (e.g. `APIPrefix+"/v1/messages"`) and branch on `r.Method` inside the handler if needed.
</issue_to_address>
### Comment 2
<location> `pkg/anthropic/handler_test.go:67` </location>
<code_context>
+ }
+}
+
+func TestRouteHandlers(t *testing.T) {
+ t.Parallel()
+
</code_context>
<issue_to_address>
**suggestion (testing):** Extend routeHandlers tests to assert handler wiring, not just keys
Currently this only checks that the expected route keys exist. Please also assert that the mapped handlers are non-nil and match the expected functions (e.g., `routes["POST "+APIPrefix+"/v1/messages"] == h.handleMessages` and similarly for the count_tokens route). This will catch cases where a route key is present but wired to the wrong handler.
Suggested implementation:
```golang
import (
"net/http"
"net/http/httptest"
"reflect"
"strings"
"testing"
)
```
```golang
func TestRouteHandlers(t *testing.T) {
t.Parallel()
// TODO: replace this with the real constructor/helper for the anthropic handler.
// For example: h := newTestAnthropicHandler(t) or h := NewHandler(...)
h := newTestAnthropicHandler(t)
routes := h.routeHandlers()
wantRoutes := map[string]http.HandlerFunc{
"POST " + APIPrefix + "/v1/messages": h.handleMessages,
"POST " + APIPrefix + "/v1/messages/count_tokens": h.handleCountTokens,
}
for routeKey, wantHandler := range wantRoutes {
gotHandler, ok := routes[routeKey]
if !ok {
t.Fatalf("route %q not found in routeHandlers", routeKey)
}
if gotHandler == nil {
t.Fatalf("route %q has a nil handler", routeKey)
}
if reflect.ValueOf(gotHandler).Pointer() != reflect.ValueOf(wantHandler).Pointer() {
t.Errorf("route %q wired to wrong handler: got %v, want %v", routeKey, gotHandler, wantHandler)
}
}
}
func TestWriteAnthropicError(t *testing.T) {
t.Parallel()
```
1. Replace the placeholder `newTestAnthropicHandler(t)` call with the actual way you construct an `anthropic` handler in your tests (e.g., `newTestHandler(t)`, `newAnthropicHandler(...)`, or `NewHandler(...)`), ensuring the returned value has methods `routeHandlers()`, `handleMessages`, and `handleCountTokens`.
2. If your handler type or method names differ (for example, `handleCountTokens` is named `handleCountTokensRequest` or the path is different), update the `wantRoutes` map keys and handler references to match the actual API:
- The keys should exactly match the ones used in `routeHandlers()` (e.g., `"POST "+APIPrefix+"/v1/messages"` and the correct count_tokens route key).
- The values should reference the correct handler methods for each route.
3. If `routeHandlers()` returns a type other than `map[string]http.HandlerFunc` (for example, `map[string]http.Handler`), adjust the `wantRoutes` map type and comparison accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
With this change you can do things like: |
![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 successfully implements support for the Anthropic Messages API by adding a new compatibility layer. The changes are well-structured and follow existing patterns in the codebase. I've identified a few areas for improvement, mainly around testing and code clarity. The new anthropic handler lacks test coverage for its core proxying logic, which is important to add for robustness. Additionally, a test case in handler_test.go could panic due to a nil logger. I've also included a suggestion to refactor some duplicated code in the scheduler. Overall, this is a solid implementation that would be even better with these improvements.
ericcurtin
force-pushed
the
add-anthropic
branch
2 times, most recently
from
2ec883d to
430fbaf
Compare
Complete Anthropic Messages API implementation Signed-off-by: Eric Curtin <[email protected]>
ericcurtin
force-pushed
the
add-anthropic
branch
from
430fbaf to
e4c9d54
Compare
|
I removed all the go.* changes, should build I think |
Complete Anthropic Messages API implementation