feat(util): add Claude tool id and tool_use mapping helpers#1927
feat(util): add Claude tool id and tool_use mapping helpers#1927shenshuoyaoyouguang wants to merge 1 commit intorouter-for-me:mainfrom
Conversation
Summary of ChangesHello, 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 improves the robustness and compatibility of the Claude translator by addressing two key areas: ensuring tool IDs meet strict formatting requirements and maintaining accurate tool name mappings throughout the translation process. These changes prevent errors in tool-using conversations and provide a more consistent experience when interacting with various AI providers through the Claude interface. 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. Changelog
Activity
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
|
![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 important fixes for handling Claude tool IDs by sanitizing them and preserving tool name mappings across different translators. The changes are well-supported by new regression tests.
My review identified a recurring issue where a utility function, util.MapToolName, is used incorrectly. While it doesn't seem to cause a bug at the moment due to how the function behaves on lookup failure, it makes the code confusing and fragile. I've left comments with suggestions to simplify and correct this usage for better clarity and robustness.
internal/translator/antigravity/claude/antigravity_claude_request.go
Outdated
Show resolved
Hide resolved
internal/translator/gemini-cli/claude/gemini-cli_claude_request.go
Outdated
Show resolved
Hide resolved
![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: 0d0ccd04f3
ℹ️ 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".
shenshuoyaoyouguang
force-pushed
the
pr/claude-tool-id-sanitize
branch
from
0d0ccd0 to
cc418bb
Compare
shenshuoyaoyouguang
changed the title
|
Narrowed this PR to internal/util/** only so it complies with ranslator-path-guard. The translator-specific follow-up is tracked in #1930. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces two useful helper functions for handling Claude-specific tool IDs and mappings, along with corresponding tests. The SanitizeClaudeToolID function correctly sanitizes tool IDs, and ToolUseNameMapFromClaudeRequest effectively extracts tool use mappings from request payloads. The changes are well-implemented and tested. I have one suggestion to simplify the implementation of ToolUseNameMapFromClaudeRequest by leveraging more of gjson's pathing capabilities.
| if len(rawJSON) == 0 || !gjson.ValidBytes(rawJSON) { | ||
| return nil | ||
| } | ||
|
|
||
| messages := gjson.GetBytes(rawJSON, "messages") | ||
| if !messages.Exists() || !messages.IsArray() { | ||
| return nil | ||
| } | ||
|
|
||
| out := map[string]string{} | ||
| messages.ForEach(func(_, message gjson.Result) bool { | ||
| contents := message.Get("content") | ||
| if !contents.IsArray() { | ||
| return true | ||
| } | ||
|
|
||
| contents.ForEach(func(_, content gjson.Result) bool { | ||
| if content.Get("type").String() != "tool_use" { | ||
| return true | ||
| } | ||
|
|
||
| toolUseID := strings.TrimSpace(content.Get("id").String()) | ||
| toolName := strings.TrimSpace(content.Get("name").String()) | ||
| if toolUseID == "" || toolName == "" { | ||
| return true | ||
| } | ||
|
|
||
| if _, exists := out[toolUseID]; !exists { | ||
| out[toolUseID] = toolName | ||
| } | ||
| return true | ||
| }) | ||
| return true | ||
| }) | ||
|
|
||
| if len(out) == 0 { | ||
| return nil | ||
| } | ||
| return out |
There was a problem hiding this comment.
This function can be simplified by using a gjson path query to directly select the tool_use objects. This avoids nested loops and several checks, making the code more concise and potentially more performant.
if !gjson.ValidBytes(rawJSON) {
return nil
}
out := map[string]string{}
gjson.GetBytes(rawJSON, `messages.#.content.#[type=="tool_use"]`).ForEach(func(_, content gjson.Result) bool {
toolUseID := strings.TrimSpace(content.Get("id").String())
toolName := strings.TrimSpace(content.Get("name").String())
if toolUseID == "" || toolName == "" {
return true
}
if _, exists := out[toolUseID]; !exists {
out[toolUseID] = toolName
}
return true
})
if len(out) == 0 {
return nil
}
return out
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces two useful helper functions for handling Claude tool IDs and tool use mappings, along with corresponding tests. The implementation is solid. I've provided a couple of suggestions to refactor the code to be more idiomatic and to improve the test coverage and maintainability.
| func TestSanitizeClaudeToolID_ReplacesInvalidCharacters(t *testing.T) { | ||
| got := SanitizeClaudeToolID("fs.readFile:temp@1") | ||
| if got != "fs_readFile_temp_1" { | ||
| t.Fatalf("SanitizeClaudeToolID returned %q", got) | ||
| } | ||
| } | ||
|
|
||
| func TestSanitizeClaudeToolID_GeneratesFallbackForEmptyResult(t *testing.T) { | ||
| got := SanitizeClaudeToolID("!!!") | ||
| if got == "" { | ||
| t.Fatal("expected non-empty fallback id") | ||
| } | ||
| if !regexp.MustCompile(`^[a-zA-Z0-9_-]+$`).MatchString(got) { | ||
| t.Fatalf("fallback id %q does not match Claude regex", got) | ||
| } | ||
| } |
There was a problem hiding this comment.
The tests are good, but they could be improved by using a single table-driven test. This would make it easier to add more test cases in the future, would cover more edge cases (like empty input or already-valid IDs), and would avoid recompiling the regex in a loop.
func TestSanitizeClaudeToolID(t *testing.T) {
claudeToolIDRegex := regexp.MustCompile(`^[a-zA-Z0-9_-]+$`)
tests := []struct {
name string
input string
expected string
fallbackExpected bool
}{
{"replaces invalid characters", "fs.readFile:temp@1", "fs_readFile_temp_1", false},
{"valid id is unchanged", "valid-id_123", "valid-id_123", false},
{"generates fallback for all-invalid input", "!!!", "", true},
{"generates fallback for empty input", "", "", true},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := SanitizeClaudeToolID(tt.input)
if tt.fallbackExpected {
if got == "" {
t.Error("expected fallback ID, but got empty string")
}
} else {
if got != tt.expected {
t.Errorf("got %q, want %q", got, tt.expected)
}
}
if !claudeToolIDRegex.MatchString(got) {
t.Errorf("output %q does not match Claude tool ID regex", got)
}
})
}
}| messages := gjson.GetBytes(rawJSON, "messages") | ||
| if !messages.Exists() || !messages.IsArray() { | ||
| return nil | ||
| } | ||
|
|
||
| out := map[string]string{} | ||
| messages.ForEach(func(_, message gjson.Result) bool { | ||
| contents := message.Get("content") | ||
| if !contents.IsArray() { | ||
| return true | ||
| } | ||
|
|
||
| contents.ForEach(func(_, content gjson.Result) bool { | ||
| if content.Get("type").String() != "tool_use" { | ||
| return true | ||
| } | ||
|
|
||
| toolUseID := strings.TrimSpace(content.Get("id").String()) | ||
| toolName := strings.TrimSpace(content.Get("name").String()) | ||
| if toolUseID == "" || toolName == "" { | ||
| return true | ||
| } | ||
|
|
||
| if _, exists := out[toolUseID]; !exists { | ||
| out[toolUseID] = toolName | ||
| } | ||
| return true | ||
| }) | ||
| return true | ||
| }) |
There was a problem hiding this comment.
The current implementation with nested ForEach loops can be simplified by using a gjson path query to directly select all tool_use content parts. This makes the code more concise and idiomatic when using the gjson library.
out := map[string]string{}
gjson.GetBytes(rawJSON, `messages.#.content.#[type=="tool_use"]`).ForEach(func(_, toolUse gjson.Result) bool {
toolUseID := strings.TrimSpace(toolUse.Get("id").String())
toolName := strings.TrimSpace(toolUse.Get("name").String())
if toolUseID == "" || toolName == "" {
return true
}
if _, exists := out[toolUseID]; !exists {
out[toolUseID] = toolName
}
return true
})
|
Update Note: This PR is now a deliberately scoped-down util-only version, keeping only reusable, testable helper changes under internal/util/. The translator-side wiring modifications mentioned in previous reviews are reasonable directions, but due to repo rules .github/workflows/pr-path-guard.yml directly blocking public PR changes to internal/translator/, that portion has been split out to issue #1930 for follow-up by the maintenance team. Current PR business goal: deliver general helpers in a mergeable, reusable form first, avoiding continued policy gate blocks. Review Navigation: Current PR is the deliberately scoped-down util-only version, keeping only reusable helpers under internal/util/**; old translator-side comments mostly correspond to subsequent work already split to issue #1930, no longer applicable to current diff. Current CI has passed. |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Summary: Thanks for narrowing this down. The util-level additions look reasonable, but the branch is currently not mergeable against main.
Blocking:
This PR still adds internal/util/claude_tool_id.go, but main already contains SanitizeClaudeToolID. Rebasing onto current main reproduces an add/add conflict in that file, so the PR cannot be merged as-is.
Please rebase and drop or resolve that duplicated file so the remaining diff is only the new tests plus ToolUseNameMapFromClaudeRequest.
Test plan:
go test ./internal/util
Attempted merge of main into the PR branch and reproduced the conflict in internal/util/claude_tool_id.go.
2 participants
Summary
Why this PR was narrowed
The repository path guard rejects pull requests that modify internal/translator/.
To keep business value moving without fighting repository policy, this PR now contains only reusable helper functions under internal/util/.
The translator-specific follow-up has been moved to issue #1930 for the maintenance team.
Business value
Tests