fix(translator): correctly map stop_reason in response translations #805
Conversation
This commit fixes an issue where coding tools (Claude Code, OpenCode) would stop after a single turn because the stop_reason was not being correctly mapped from the upstream provider. - Gemini: Correctly map 'MAX_TOKENS' to 'max_tokens' and 'STOP'/'FINISH_REASON_UNSPECIFIED'/'UNKNOWN' to 'end_turn'. - Gemini CLI: Applied the same logic as the standard Gemini translator. - Codex: Propagate the 'stop_reason' directly from the upstream response if available, falling back to 'tool_use' or 'end_turn'. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Summary of ChangesHello @HsnSaboor, 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 primarily addresses critical issues in how various translators handle 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.
Pull request overview
This PR fixes multi-turn conversation functionality for coding tools (Claude Code, OpenCode) by correctly mapping the stop_reason field from upstream API responses to Claude API values. Additionally, it introduces function name sanitization to ensure compatibility with Gemini/Vertex AI naming requirements.
Key changes:
- Updated Gemini, Gemini-CLI, and Codex translators to properly map upstream finish reasons to Claude API
stop_reasonvalues (end_turn,max_tokens,tool_use) - Added
SanitizeFunctionNameutility to enforce Gemini/Vertex AI function naming constraints (64 char limit, valid character set, proper prefix) - Fixed schema placeholder logic to prevent adding placeholders to top-level schemas
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/util/util.go | Added SanitizeFunctionName utility with regex-based character validation, length truncation, and prefix enforcement |
| internal/util/sanitize_test.go | Comprehensive test coverage for SanitizeFunctionName with edge cases and boundary conditions |
| internal/util/gemini_schema.go | Skip placeholder addition for top-level schemas to prevent unnecessary required fields |
| internal/util/gemini_schema_test.go | Updated test expectation to include placeholder field in nested object schemas |
| internal/translator/gemini/claude/gemini_claude_response.go | Map Gemini finish reasons to Claude stop_reason values (MAX_TOKENS→max_tokens, STOP→end_turn, etc.) |
| internal/translator/gemini/claude/gemini_claude_request.go | Apply function name sanitization to tool_use and tool_result function names |
| internal/translator/gemini-cli/claude/gemini-cli_claude_response.go | Map Gemini-CLI finish reasons to Claude stop_reason values |
| internal/translator/gemini-cli/claude/gemini-cli_claude_request.go | Apply function name sanitization to tool_use and tool_result function names |
| internal/translator/codex/claude/codex_claude_response.go | Prioritize upstream stop_reason from response before falling back to tool_use detection |
| internal/translator/codex/claude/codex_claude_request.go | Integrate SanitizeFunctionName into shortenNameIfNeeded and buildShortNameMap |
| internal/translator/antigravity/claude/antigravity_claude_request.go | Apply function name sanitization and fix tool_result ID parsing (slice to -1 instead of -2) |
Comments suppressed due to low confidence (1)
internal/translator/codex/claude/codex_claude_request.go:323
- The makeUnique function can potentially produce names exceeding 64 characters after adding suffixes. Since SanitizeFunctionName is called on the candidate before entering makeUnique, and makeUnique appends suffixes without re-sanitizing, the final result could exceed the 64 character limit. Consider applying SanitizeFunctionName to the final tmp value before returning it from makeUnique, or ensure the result is truncated to 64 characters after appending the suffix.
makeUnique := func(cand string) string {
if _, ok := used[cand]; !ok {
return cand
}
base := cand
for i := 1; ; i++ {
suffix := "_" + strconv.Itoa(i)
allowed := limit - len(suffix)
if allowed < 0 {
allowed = 0
}
tmp := base
if len(tmp) > allowed {
tmp = tmp[:allowed]
}
tmp = tmp + suffix
if _, ok := used[tmp]; !ok {
return tmp
}
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 correctly addresses the issue of mapping stop_reason for multi-turn conversations, which is a crucial fix. I also appreciate the move to centralize function name sanitization logic into a new SanitizeFunctionName utility function.
However, I've found a critical bug in the logic for parsing function names from tool_use_id in the Gemini and Antigravity translators. The change from len(toolCallIDs)-2 to len(toolCallIDs)-1 seems incorrect and will lead to malformed function names. I've left detailed comments on the affected files.
Additionally, I have a few suggestions for refactoring that could improve code clarity and reduce duplication, particularly in codex_claude_request.go and util.go, and simplify the new stop_reason logic. Please see the specific comments for details.
| toolCallIDs := strings.Split(toolCallID, "-") | ||
| if len(toolCallIDs) > 1 { | ||
| funcName = strings.Join(toolCallIDs[0:len(toolCallIDs)-2], "-") | ||
| rawFuncName = strings.Join(toolCallIDs[0:len(toolCallIDs)-1], "-") |
There was a problem hiding this comment.
There appears to be a bug in how the function name is extracted from toolCallID. The tool_use_id is generated in other parts of the codebase (e.g., gemini-cli_claude_response.go:212) with the format funcName-timestamp-counter. To extract just funcName, you need to strip the last two components (timestamp and counter).
The previous logic strings.Join(toolCallIDs[0:len(toolCallIDs)-2], "-") was correct for this format. The new logic strings.Join(toolCallIDs[0:len(toolCallIDs)-1], "-") will incorrectly include the timestamp in the function name (e.g., funcName-timestamp). This will likely cause issues with tool use.
Please revert this part of the change to use -2.
| rawFuncName = strings.Join(toolCallIDs[0:len(toolCallIDs)-1], "-") | |
| rawFuncName = strings.Join(toolCallIDs[0:len(toolCallIDs)-2], "-") |
| toolCallIDs := strings.Split(toolCallID, "-") | ||
| if len(toolCallIDs) > 1 { | ||
| funcName = strings.Join(toolCallIDs[0:len(toolCallIDs)-1], "-") | ||
| rawFuncName = strings.Join(toolCallIDs[0:len(toolCallIDs)-1], "-") |
There was a problem hiding this comment.
There appears to be a bug in how the function name is extracted from toolCallID. The tool_use_id is generated in gemini-cli_claude_response.go:212 with the format funcName-timestamp-counter. To extract just funcName, you need to strip the last two components (timestamp and counter).
The previous logic strings.Join(toolCallIDs[0:len(toolCallIDs)-2], "-") was correct for this format. The new logic strings.Join(toolCallIDs[0:len(toolCallIDs)-1], "-") will incorrectly include the timestamp in the function name (e.g., funcName-timestamp). This will likely cause issues with tool use.
Please revert this part of the change to use -2.
| rawFuncName = strings.Join(toolCallIDs[0:len(toolCallIDs)-1], "-") | |
| rawFuncName = strings.Join(toolCallIDs[0:len(toolCallIDs)-2], "-") |
| toolCallIDs := strings.Split(toolCallID, "-") | ||
| if len(toolCallIDs) > 1 { | ||
| funcName = strings.Join(toolCallIDs[0:len(toolCallIDs)-1], "-") | ||
| rawFuncName = strings.Join(toolCallIDs[0:len(toolCallIDs)-1], "-") |
There was a problem hiding this comment.
There appears to be a bug in how the function name is extracted from toolCallID. The tool_use_id is generated in gemini_claude_response.go with the format funcName-timestamp-counter. To extract just funcName, you need to strip the last two components (timestamp and counter).
The previous logic strings.Join(toolCallIDs[0:len(toolCallIDs)-2], "-") was correct for this format. The new logic strings.Join(toolCallIDs[0:len(toolCallIDs)-1], "-") will incorrectly include the timestamp in the function name (e.g., funcName-timestamp). This will likely cause issues with tool use.
Please revert this part of the change to use -2.
| rawFuncName = strings.Join(toolCallIDs[0:len(toolCallIDs)-1], "-") | |
| rawFuncName = strings.Join(toolCallIDs[0:len(toolCallIDs)-2], "-") |
| baseCandidate := func(n string) string { | ||
| const limit = 64 | ||
| if len(n) <= limit { | ||
| return n | ||
| return util.SanitizeFunctionName(n) | ||
| } | ||
| if strings.HasPrefix(n, "mcp__") { | ||
| idx := strings.LastIndex(n, "__") | ||
| if idx > 0 { | ||
| cand := "mcp__" + n[idx+2:] | ||
| if len(cand) > limit { | ||
| cand = cand[:limit] | ||
| } | ||
| return cand | ||
| return util.SanitizeFunctionName(cand) | ||
| } | ||
| } | ||
| return n[:limit] | ||
| return util.SanitizeFunctionName(n) | ||
| } |
There was a problem hiding this comment.
The logic inside this baseCandidate local function is identical to the shortenNameIfNeeded function in the same file. To avoid code duplication and improve maintainability, you can replace this function definition with baseCandidate := shortenNameIfNeeded. This also removes the need for the local limit constant.
| switch finish.String() { | ||
| case "MAX_TOKENS": | ||
| stopReason = "max_tokens" | ||
| case "STOP", "FINISH_REASON_UNSPECIFIED", "UNKNOWN": | ||
| stopReason = "end_turn" | ||
| default: | ||
| stopReason = "end_turn" | ||
| } |
There was a problem hiding this comment.
The logic in this switch statement can be simplified. The stopReason is already initialized to "end_turn", so the cases for "STOP", "FINISH_REASON_UNSPECIFIED", "UNKNOWN", and the default case are redundant. You can replace the switch with a simple if statement to check for "MAX_TOKENS".
if finish.String() == "MAX_TOKENS" {
stopReason = "max_tokens"
}| switch finish.String() { | ||
| case "MAX_TOKENS": | ||
| stopReason = "max_tokens" | ||
| case "STOP", "FINISH_REASON_UNSPECIFIED", "UNKNOWN": | ||
| stopReason = "end_turn" | ||
| default: | ||
| stopReason = "end_turn" | ||
| } |
There was a problem hiding this comment.
The logic in this switch statement can be simplified. The stopReason is already initialized to "end_turn", so the cases for "STOP", "FINISH_REASON_UNSPECIFIED", "UNKNOWN", and the default case are redundant. You can replace the switch with a simple if statement to check for "MAX_TOKENS".
if finish.String() == "MAX_TOKENS" {
stopReason = "max_tokens"
}| if len(sanitized) > 0 { | ||
| first := sanitized[0] | ||
| if !((first >= 'a' && first <= 'z') || (first >= 'A' && first <= 'Z') || first == '_') { | ||
| // If it starts with an allowed character but not allowed at the beginning (digit, dot, colon, dash), | ||
| // we must prepend an underscore. | ||
|
|
||
| // To stay within the 64-character limit while prepending, we must truncate first. | ||
| if len(sanitized) >= 64 { | ||
| sanitized = sanitized[:63] | ||
| } | ||
| sanitized = "_" + sanitized | ||
| } | ||
| } else { | ||
| sanitized = "_" | ||
| } |
There was a problem hiding this comment.
This block of code can be simplified. Since you check for an empty name at the beginning of the function, sanitized will never be empty if name is not. This means the if len(sanitized) > 0 check is redundant, and the else block is unreachable (dead code). You can remove these checks for cleaner code.
first := sanitized[0]
if !((first >= 'a' && first <= 'z') || (first >= 'A' && first <= 'Z') || first == '_') {
// If it starts with an allowed character but not allowed at the beginning (digit, dot, colon, dash),
// we must prepend an underscore.
// To stay within the 64-character limit while prepending, we must truncate first.
if len(sanitized) >= 64 {
sanitized = sanitized[:63]
}
sanitized = "_" + sanitized
}
HsnSaboor
changed the title
…implification - Fixed critical bug in tool ID parsing logic in Gemini/Antigravity translators: using index -2 instead of -1 to correctly strip timestamp and counter. - Refactored Codex request translator to use helper, removing duplicated code. - Simplified in util.go by removing redundant length check. - Simplified stop reason mapping logic in Gemini and Gemini CLI response translators.
1 participant
Summary
This PR fixes an issue where coding tools (Claude Code, OpenCode) would stop after a single turn because the stop_reason was not being correctly mapped from the upstream provider.
Test plan
finishReasontostop_reason.🤖 Generated with Claude Code