Repository navigation
Conversation
|
This pull request targeted The base branch has been automatically changed to |
![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 request-specific tool name casing normalization in the ResponseRewriter. By extracting tool names directly from the request body, the rewriter can now preserve the client's original casing (e.g., restoring glob to Glob) in both streaming and non-streaming responses. Feedback on this PR highlights an efficiency improvement in collectRequestToolNames to parse the JSON payload once using gjson.ParseBytes instead of calling gjson.GetBytes multiple times, which avoids redundant parsing overhead on large request bodies.
| func collectRequestToolNames(data []byte) map[string]string { | ||
| names := map[string]string{} | ||
| conflicts := map[string]bool{} | ||
| record := func(name string) { | ||
| if name == "" { | ||
| return | ||
| } | ||
| key := strings.ToLower(name) | ||
| if conflicts[key] { | ||
| return | ||
| } | ||
| if existing, exists := names[key]; exists { | ||
| if existing != name { | ||
| names[key] = "" | ||
| conflicts[key] = true | ||
| } | ||
| return | ||
| } | ||
| names[key] = name | ||
| } | ||
|
|
||
| for _, tool := range gjson.GetBytes(data, "tools").Array() { | ||
| record(tool.Get("name").String()) | ||
| } | ||
| if gjson.GetBytes(data, "tool_choice.type").String() == "tool" { | ||
| record(gjson.GetBytes(data, "tool_choice.name").String()) | ||
| } | ||
| if len(names) == 0 { | ||
| return nil | ||
| } | ||
| return names | ||
| } |
There was a problem hiding this comment.
Efficiency Improvement: Avoid parsing JSON multiple times
Using gjson.GetBytes multiple times on the same data byte slice causes gjson to parse the JSON payload from scratch for each call. For large request bodies, this can lead to significant performance overhead.
By parsing the JSON once using gjson.ParseBytes(data) and querying the resulting gjson.Result object, we can avoid redundant parsing and improve performance.
func collectRequestToolNames(data []byte) map[string]string {
if len(data) == 0 {
return nil
}
parsed := gjson.ParseBytes(data)
names := map[string]string{}
conflicts := map[string]bool{}
record := func(name string) {
if name == "" {
return
}
key := strings.ToLower(name)
if conflicts[key] {
return
}
if existing, exists := names[key]; exists {
if existing != name {
names[key] = ""
conflicts[key] = true
}
return
}
names[key] = name
}
for _, tool := range parsed.Get("tools").Array() {
record(tool.Get("name").String())
}
if parsed.Get("tool_choice.type").String() == "tool" {
record(parsed.Get("tool_choice.name").String())
}
if len(names) == 0 {
return nil
}
return names
}
|
Addressed Gemini Code Assist's suggestion by parsing the request JSON once with gjson.ParseBytes in collectRequestToolNames. Re-ran go test ./internal/api/modules/amp, go test ./..., go build ./cmd/server, and git diff --check successfully. |
bd6820f to
01a7cc4
Compare
Merge upstream/main with 6 commits: - fix(logging): log errors during file-backed source cleanup - feat(logging): add HomeAppLogForwarder for application log forwarding - fix(amp): restore response tool casing from request - feat(models): add Claude Opus 4.8 model to registry - Merge pull request router-for-me#3595 - Update AGENTS.md files
…thropic-tool-name-reverse-map
…thropic-tool-name-reverse-map fix(amp): restore response tool casing from request
2 participants
Fixes #3592
Summary
Claude Code tool dispatch is case-sensitive and expects response
tool_use.namevalues to match the tool names it advertised, e.g.Globrather thanglob. When routing Claude Code through the Amp provider alias path (/api/provider/anthropic/v1/messages), a Codex-backed/local provider can stream back a lowercaseglobtool call, which Claude Code rejects with:A global
glob -> Globnormalization would be unsafe because Amp intentionally preserves lowercaseglobfor Amp CLI compatibility (TestNormalizeAmpToolNames_GlobPreserved, introduced byfix(amp): preserve lowercase glob tool name).This PR makes Amp response tool-name normalization request-aware and collision-aware.
Fix
ResponseRewriternow records the exact tool-name casing advertised by the current request from:tools[].nametool_choice.nameDuring response rewriting, it uses that request-local casing before falling back to the existing Amp canonical map.
Behavior:
Glob-> responseglobis rewritten toGlobglob-> response remainsglobGlobandglob-> that lowercased key is treated as conflicted and is not force-remappedglobThe fallback handler now constructs the response rewriter with the sanitized request body on local-provider and model-mapping paths, so streaming and non-streaming responses can use the per-request tool-name map.
Tests
Added/kept coverage for both the reported bug and compatibility boundaries:
TestNormalizeAmpToolNames_GlobPreserved— default Amp behavior still keeps lowercaseglobTestNormalizeAmpToolNames_RequestToolCasing_NonStreaming— request-localGlobcasing fixes non-streamingtool_use.nameTestNormalizeAmpToolNames_RequestToolCasing_Streaming— request-localGlobcasing fixes streamingcontent_block.nameTestResponseRewriter_RequestToolCasingFromBody— verifies the rewriter derives casing fromtools[].nameTestResponseRewriter_LowercaseNativeRequestPreserved— lowercase-native client tools are not TitleCasedTestCollectRequestToolNames_CollisionIgnored— conflictingGlob/globandBash/bashkeys are not blindly remappedTestResponseRewriter_RequestToolCasingFromBody_Streaming— direct streaming rewriter coverageTestFallbackHandler_RequestToolCasing_RewritesStreamingResponse— coversFallbackHandler.WrapHandler -> NewResponseRewriterForRequest -> SSE rewriteValidation:
All pass.
Evidence
The issue was confirmed via packet capture:
tools[].name = "Glob"content_block.name = "glob"No such tool available: globThe fix is scoped to the Amp response rewriter used by provider alias local/model-mapping paths and avoids changing the global lowercase-
globdefault required by Amp CLI.