Repository navigation
fix(amp): smart-mode tool name fixes + deep-mode response repair - #2896
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 replaces the global oauthToolRenameReverseMap with a per-request reverse map to prevent incorrect tool name restoration in mixed-case scenarios. This ensures that only tool names explicitly renamed during the request are reversed in the response, fixing a bug where original TitleCase names were being corrupted. A suggestion was made to pre-allocate the reverseMap in remapOAuthToolNames to improve efficiency by avoiding reallocations.
|
edlsh
changed the title
|
@luispater can you check this PR? Thanks. |
|
Quick verification update: this is still reproducible on current upstream The PR branch ( So the branch is still fixing the current upstream behavior, not a stale issue. |
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Summary
Solid fix for the mixed-case OAuth tool-name anti-fingerprinting remap: reverse mapping is now per-request, so only tools that were actually forward-renamed get restored. This prevents TitleCase tools like Bash from being incorrectly lowercased when a different tool in the same request (e.g. glob) triggers a rename.
Key findings
- ✅
remapOAuthToolNamesnow produces a per-request reverse map; reverse functions only touch entries present in that map. - ✅ Call sites are updated across non-stream, stream (both passthrough and translated paths), and
count_tokens. - ✅ Good regression tests for the Amp mixed-case scenario, including the SSE line path.
Test plan
- Not run locally (review only).
- PR description reports
go test ./internal/runtime/executor/...; CIbuildis green.
This is an automated Codex review result and still requires manual verification by a human reviewer.
…named remapOAuthToolNames renames lowercase client-sent tools (e.g. `glob` → `Glob`) to Claude Code equivalents on OAuth requests to avoid tool-name fingerprinting. The reverse pass previously ran against a *global* reverse map and rewrote every tool_use block whose name matched any value in oauthToolRenameMap — regardless of what the client actually sent. For clients that send mixed casing (notably Amp CLI — `Bash`, `Read`, `Grep`, `Task` alongside `glob`, `skill`, etc.) this corrupted the response. Any forward rename in the request set the "renamed" flag, which then unconditionally lowercased every `Bash` in the response to `bash`. Amp's tool registry has `Bash`, not `bash`, so it rejected the tool_use with `tool "bash" is not allowed for smart mode` and tool execution failed. Fix: `remapOAuthToolNames` now returns a per-request map keyed on the upstream (TitleCase) name valued with the original client-sent name. The reverse functions take this map and only touch entries in it. Names the client sent in TitleCase pass through untouched in both directions. - Change remapOAuthToolNames signature from `([]byte, bool)` to `([]byte, map[string]string)`; populate at every rename site (tools[], tool_choice.name, message tool_use, tool_reference, nested tool_reference inside tool_result). - Change reverseRemapOAuthToolNames and reverseRemapOAuthToolNamesFromStreamLine to accept and consume the per-request map; remove the global oauthToolRenameReverseMap. - Update all three executor call sites (Execute, ExecuteStream direct passthrough, ExecuteStream translated) + count_tokens. - Add regression tests for the mixed-case scenario in both the non-streaming and SSE code paths.
Address Gemini code review suggestion: the reverseMap can contain at most len(oauthToolRenameMap) entries, so pre-allocating avoids reallocations as entries are added.
422bb1c to
fd45dec
Compare
edlsh
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd45dece7f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@luispater quick update: I’ve been working with @edlsh over X/Twitter DMs and testing this PR locally against Amp CLI. I pulled the latest PR branch, rebuilt
I also checked the CLIProxyAPI logs directly. With this PR, the repaired completed event includes the expected output items/function calls, and Amp continues the turn correctly. The latest SSE framing follow-up also looks good to me: repaired multiline payloads are emitted with Focused tests pass locally: I did see a few normal tool execution errors from the model guessing nonexistent paths, but not proxy formatting errors. The remaining full-suite From my local testing and log inspection, this PR looks good for the Amp smart-mode and deep-mode failures. |
|
|
Still error, how can we fix it @ben-vargas @Amoodaa @edlsh |
|
@0xkaisa - I don't know why this hasn't been merged/fixed, I'm maintaining a fork branch here and keeping it up to date - this is what I'm using: https://github.com/ben-vargas/ai-cli-proxy-api/tree/fix-amp-patch That branch is on v6.10.5 with Amp fixes in this PR. |
Sorry for the wait, we've been swamped with issues, PRs, and other chores lately. As a maintainer, if everything looks good to you, I'll merge this right away and release a new version. |
|
@ben-vargas BTW, due to well-known reasons, the core team has lost all Claude subscriptions. This has significantly impacted the review progress of all Claude related PRs. We sincerely apologize for the inconvenience. |
|
Thanks @luispater - Wasn't sure if this was being ignored on purpose or just delayed in the workload... obviously the latter, so no worries and thanks for the attention! |
…-map fix(amp): smart-mode tool name fixes + deep-mode response repair
…e-per-request-map
ghost
mentioned this pull request
5 participants
Summary
This consolidates the Amp smart-mode and deep-mode tool fixes into one PR.
globtool casing in proxied responses.response.completedwith an emptyresponse.outputby reconstructing it from priorresponse.output_item.doneevents.Amp Coverage
Smart mode:
globtool casing fix that was previously in fix(amp): preserve tool calls in proxied streams #2766.Deep mode:
gpt-5.4high reasoning: before the fix, reasoning appeared but the final answer was blank; after running the fixed build, the final answer appeared correctly.Closes #2766.
Testing
go build ./internal/api/modules/amp/... ./sdk/api/handlers/openai/... ./internal/runtime/executor/...go test ./internal/api/modules/amp/... ./sdk/api/handlers/openai/... ./internal/runtime/executor/...go test -run "TestForwardResponsesStream|TestNormalizeAmpToolNames|TestRewriteStreamChunk" -v ./internal/api/modules/amp/... ./sdk/api/handlers/openai/...Note:
TestMultiSourceSecret_CacheEmptyResultininternal/api/modules/amp/secret_test.gois a pre-existing failure on the parent commit as well.