feat: enhance text protocol conversion and advanced custom routing - #5825
Conversation
…anced custom models
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
WalkthroughThis PR centralizes request/response conversion in ChangesCore conversion, routing, billing, and handler updates
Web UI and operations
Estimated code review effort: 5 (Critical) | ~180 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Calcium-Ion
changed the title
Calcium-Ion
changed the title
Calcium-Ion
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/default/src/features/channels/lib/advanced-custom.ts (1)
301-323: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecompute upstream defaults when changing a route group path.
updateGroupIncomingPathonly changesincoming_pathand possibly resetsconverter; it leaves the oldupstream_pathandauth. Changing a native group from OpenAI Chat to Claude/Gemini can save a route with the new incoming path but stale upstream/auth defaults.Suggested fix
if (!groupRouteIndexes.has(routeIndex)) return route const converter = route.converter || 'none' + const nextConverter = isAdvancedCustomIncomingPathAllowed( + resolvedIncomingPath, + converter + ) + ? converter + : 'none' + const defaults = getAdvancedCustomConverterDefaults( + nextConverter, + resolvedIncomingPath + ) return { ...route, incoming_path: resolvedIncomingPath, - converter: isAdvancedCustomIncomingPathAllowed( - resolvedIncomingPath, - converter - ) - ? converter - : 'none', + converter: nextConverter, + upstream_path: defaults.upstream_path, + auth: defaults.auth, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/channels/lib/advanced-custom.ts` around lines 301 - 323, `updateGroupIncomingPath` only updates the route’s incoming_path, so changing a group’s path can leave stale upstream_path and auth from the previous preset. Update the logic in `updateGroupIncomingPath` (and any helper it uses for native route defaults) to recompute the full route defaults for the selected group, including upstream_path, converter, and auth, based on the matching entries in the advanced route definitions.
🟠 Major comments (29)
dto/channel_settings.go-291-296 (1)
291-296: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPersist the defaulted converter back to the route.
Line 291 copies
c.Routes[i], so theadvancedCustomConverterNonedefault on Line 296 is only used during validation. Later callers that receive the matched route still see an emptyConverter.Proposed fix
- route := c.Routes[i] - route.IncomingPath = strings.TrimSpace(route.IncomingPath) - upstreamPath := strings.TrimSpace(route.UpstreamPath) - route.Converter = strings.TrimSpace(route.Converter) + route := &c.Routes[i] + route.IncomingPath = strings.TrimSpace(route.IncomingPath) + route.UpstreamPath = strings.TrimSpace(route.UpstreamPath) + upstreamPath := route.UpstreamPath + route.Converter = strings.TrimSpace(route.Converter) if route.Converter == "" { route.Converter = advancedCustomConverterNone }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dto/channel_settings.go` around lines 291 - 296, The route validation logic in the channel settings flow is only defaulting Converter on a local copy of c.Routes[i], so the empty value is never persisted back to the stored route. Update the route-handling code in the validation/matching path that trims IncomingPath, UpstreamPath, and Converter so the default advancedCustomConverterNone is written back into c.Routes[i] (or otherwise returned in the matched route object) before continuing, ensuring later callers see the defaulted Converter value.dto/channel_settings.go-136-139 (1)
136-139: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrefer exact path matches before Gemini generate→stream fallback.
Line 137 calls
matchAdvancedCustomIncomingPath, where a configured:generateContentroute also matches:streamGenerateContent. If a generic generate route appears before a stream-specific route, streaming requests are captured by the generate route before the exact stream route is inspected.Proposed direction
func (c *AdvancedCustomConfig) MatchPathForModel(requestPath string, model string) (AdvancedCustomRoute, bool) { if c == nil { return AdvancedCustomRoute{}, false } model = strings.TrimSpace(model) + + for _, route := range c.Routes { + if matchAdvancedCustomIncomingPathTemplate(strings.TrimSpace(route.IncomingPath), requestPath) && + matchAdvancedCustomRouteModel(route.Models, model) { + return route, true + } + } + for _, route := range c.Routes { if matchAdvancedCustomIncomingPath(strings.TrimSpace(route.IncomingPath), requestPath) && matchAdvancedCustomRouteModel(route.Models, model) { return route, true } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dto/channel_settings.go` around lines 136 - 139, The route lookup in the channel settings matching flow is too permissive because `matchAdvancedCustomIncomingPath` lets a `:generateContent` route match `:streamGenerateContent`, so a generic route can win before a stream-specific one is checked. Update the matching logic in the route selection path around `Routes` iteration to prefer exact incoming-path matches first, then allow the Gemini generate→stream fallback only if no exact stream route matches, keeping the existing model check via `matchAdvancedCustomRouteModel`.service/billing_usage.go-154-193 (1)
154-193: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPopulate normalized Gemini
InputTokens.
usageFromGeminiBillingUsage()computes the provider-normalized input total but leavesInputTokensat zero. Downstream,PostTextConsumeQuota()only writesother["input_tokens_total"]whenbillingUsage.InputTokens > 0, so Gemini billing-usage paths never emit that field after this refactor.Suggested fix
func usageFromGeminiBillingUsage(billingUsage *dto.BillingUsage) *dto.Usage { metadata := *billingUsage.GeminiUsageMetadata promptTokens := metadata.PromptTokenCount + metadata.ToolUsePromptTokenCount usage := &dto.Usage{ - PromptTokens: promptTokens, + PromptTokens: promptTokens, + InputTokens: promptTokens, CompletionTokens: metadata.CandidatesTokenCount + metadata.ThoughtsTokenCount, TotalTokens: metadata.TotalTokenCount, UsageSemantic: dto.BillingUsageSemanticGemini, UsageSource: dto.BillingUsageSourceGeminiChat, BillingUsage: dto.CloneBillingUsage(billingUsage),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/billing_usage.go` around lines 154 - 193, The Gemini normalization in usageFromGeminiBillingUsage leaves Usage.InputTokens unset even though it already computes the normalized prompt total; set InputTokens on the returned dto.Usage from the same provider-normalized input token value used for prompt usage. Keep the change localized to usageFromGeminiBillingUsage and ensure PostTextConsumeQuota can see a non-zero BillingUsage.InputTokens so it emits input_tokens_total for Gemini paths.relay/channel/gemini/relay-gemini.go-40-62 (1)
40-62: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCount non-text Gemini parts in the fallback usage estimator.
Line 45 only includes
part.Text. When Gemini omitsUsageMetadata, tool-call / function-response outputs fall through toResponseText2Usage(...)as an empty string, so these responses can be billed with0completion tokens even though the model produced output.Suggested fix
func geminiResponseUsageText(response *dto.GeminiChatResponse) string { if response == nil { return "" } var text strings.Builder for _, candidate := range response.Candidates { for _, part := range candidate.Content.Parts { - if part.Text != "" { - text.WriteString(part.Text) - } + switch { + case part.Text != "": + text.WriteString(part.Text) + case part.FunctionCall != nil: + if payload, err := common.Marshal(part.FunctionCall); err == nil { + text.Write(payload) + } + case part.FunctionResponse != nil: + if payload, err := common.Marshal(part.FunctionResponse); err == nil { + text.Write(payload) + } + } } } return text.String() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@relay/channel/gemini/relay-gemini.go` around lines 40 - 62, The fallback usage estimator in geminiResponseUsageText only concatenates part.Text, so non-text Gemini parts are dropped and can produce zero completion tokens when UsageMetadata is missing. Update geminiResponseUsageText and/or buildUsageFromGeminiResponse to account for tool-call and function-response parts from GeminiChatResponse.Candidates.Content.Parts, converting those outputs into a non-empty fallback string before calling ResponseText2Usage so usage is estimated from all model output, not just text.service/relayconvert/internal/oai_chat/to_claude_messages_req.go-33-44 (1)
33-44: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject malformed request shapes instead of panicking.
Line 41 and Line 219 both use unchecked type assertions on client JSON. A payload like
tools[].function.parameters.type: {}orstop: [1]will panic inside conversion instead of returning a normal validation error.Proposed fix
for _, tool := range textRequest.Tools { if params, ok := tool.Function.Parameters.(map[string]any); ok { claudeTool := dto.Tool{ Name: tool.Function.Name, Description: tool.Function.Description, } claudeTool.InputSchema = make(map[string]interface{}) - if params["type"] != nil { - claudeTool.InputSchema["type"] = params["type"].(string) + if rawType, exists := params["type"]; exists { + typeName, ok := rawType.(string) + if !ok { + return nil, fmt.Errorf("tool %q has non-string schema type", tool.Function.Name) + } + claudeTool.InputSchema["type"] = typeName }if textRequest.Stop != nil { switch stop := textRequest.Stop.(type) { case string: claudeRequest.StopSequences = []string{stop} case []interface{}: stopSequences := make([]string, 0) for _, item := range stop { - stopSequences = append(stopSequences, item.(string)) + value, ok := item.(string) + if !ok { + return nil, fmt.Errorf("stop sequences must be strings") + } + stopSequences = append(stopSequences, value) } claudeRequest.StopSequences = stopSequences } }Also applies to: 212-220
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/oai_chat/to_claude_messages_req.go` around lines 33 - 44, The conversion in toClaudeMessagesReq should not panic on malformed client JSON; replace the unchecked type assertions in the tools loop and stop handling with safe type checks and return a validation error when fields like parameters.type or stop have the wrong shape. Update the conversion paths around the tool parsing in toClaudeMessagesReq and the stop-field handling near the later conversion block so they validate types before assigning into dto.Tool/InputSchema or Claude request fields.service/relayconvert/internal/oai_chat/to_gemini_chat_req.go-61-68 (1)
61-68: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOnly skip
ApplyThinkingConfigwhen a thinking override was actually provided.Line 63 sets
adaptorWithExtraBody = truefor anyextra_body.googleobject on non--nothinkingmodels. If the caller only sendsimage_config, Lines 151-153 still skipsharedgemini.ApplyThinkingConfig, so the normal default/model thinking behavior disappears for an unrelated option.Proposed fix
if googleBody, ok := extraBody["google"].(map[string]interface{}); ok { if !strings.HasSuffix(upstreamModelName, "-nothinking") { - adaptorWithExtraBody = true if _, hasErrorParam := googleBody["thinkingConfig"]; hasErrorParam { return nil, errors.New("extra_body.google.thinkingConfig is not supported, use extra_body.google.thinking_config instead") } @@ if hasThinkingConfig { + adaptorWithExtraBody = true if geminiRequest.GenerationConfig.ThinkingConfig == nil { geminiRequest.GenerationConfig.ThinkingConfig = &tempThinkingConfig } else {Also applies to: 104-116, 151-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/oai_chat/to_gemini_chat_req.go` around lines 61 - 68, The thinking-config guard in to_gemini_chat_req.go is too broad: `adaptorWithExtraBody` is being set for any `extra_body.google` payload, which causes `sharedgemini.ApplyThinkingConfig` to be skipped even when only unrelated fields like `image_config` are present. Update the logic around `googleBody`, `thinkingConfig`, and the `ApplyThinkingConfig` call so the skip only happens when an actual thinking override is supplied (for example, `thinking_config` or the legacy `thinkingConfig` error path), and keep the default/model thinking behavior for other extra body options.service/relayconvert/internal/oai_chat/to_claude_messages_req.go-229-235 (1)
229-235: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the normalized role when the incoming role is empty.
Line 231 writes
"user"back intotextRequest.Messages[i], butfmtMessage.Roleon Line 234 still reads from the stalemessagerange copy. Empty-role messages therefore stay empty informatMessages, which can produce invalid Claude roles later.Proposed fix
for i, message := range textRequest.Messages { - if message.Role == "" { - textRequest.Messages[i].Role = "user" - } + role := message.Role + if role == "" { + role = "user" + textRequest.Messages[i].Role = role + } fmtMessage := dto.Message{ - Role: message.Role, + Role: role, Content: message.Content, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/oai_chat/to_claude_messages_req.go` around lines 229 - 235, The normalized role is being written back to textRequest.Messages[i] in formatMessages, but dto.Message.Role still uses the stale loop variable message, so empty roles remain empty. Update formatMessages to populate fmtMessage.Role from the normalized value in textRequest.Messages[i] (or otherwise re-read the updated slice element) so messages with missing roles become "user" before building Claude messages.service/relayconvert/internal/oai_chat/to_gemini_chat_req.go-311-358 (1)
311-358: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the text after the last inline markdown image.
Once the loop starts consuming
segments, it keeps slicingtext, but the remaining tail is never appended after the final image. Inputs likebefore  afterdrop the trailing" after"completely.Proposed fix
for { startIdx := strings.Index(text, "![") if startIdx == -1 { break @@ parts = append(parts, imgPart) text = text[closeIdx+1:] } + if hasMarkdownImage && text != "" { + parts = append(parts, dto.GeminiPart{ + Text: text, + }) + } if !hasMarkdownImage { parts = append(parts, dto.GeminiPart{ Text: part.Text,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/oai_chat/to_gemini_chat_req.go` around lines 311 - 358, The inline markdown image parsing in toGeminiChatReq is discarding the trailing text after the last `` segment. Update the loop that slices `text` to preserve any remaining tail after the final image by appending the leftover text to `parts` once no more image matches are found, using the existing `text`, `parts`, and `hasMarkdownImage` handling in `toGeminiChatReq`.service/relayconvert/internal/shared/claude/cache.go-3-8 (1)
3-8: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNormalize the overflow path as well.
When
tokens5m + tokens1h > totalTokens, Lines 4-8 still return a split whose sum exceedstotalTokens. That makes the “normalized” result inconsistent and can overcount cache-creation tokens downstream.Proposed fix
func NormalizeCacheCreationSplit(totalTokens int, tokens5m int, tokens1h int) (int, int) { + if totalTokens <= 0 { + return 0, 0 + } + if tokens1h < 0 { + tokens1h = 0 + } + if tokens1h > totalTokens { + return 0, totalTokens + } remainder := totalTokens - tokens5m - tokens1h if remainder < 0 { - remainder = 0 + return totalTokens - tokens1h, tokens1h } return tokens5m + remainder, tokens1h }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/shared/claude/cache.go` around lines 3 - 8, Normalize the overflow path in NormalizeCacheCreationSplit: when tokens5m + tokens1h exceeds totalTokens, the returned split must be clamped so the sum never goes above totalTokens. Update the logic in NormalizeCacheCreationSplit to reduce the 5m and/or 1h values proportionally or by a clear priority rule, instead of only zeroing the remainder, so the normalized result is always bounded by totalTokens.dto/gemini.go-486-493 (1)
486-493: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve billing-only Gemini metadata here.
GetUsageMetadata()currently returnsnilunlessHasUsageMetadatais set or token counters are non-zero. That drops responses built in memory with onlyUsageMetadata.BillingUsage, and both the Gemini relay path andservice/relayconvert/response_registry.gorely on this accessor before deciding whether to fall back to estimated usage. The result is that preserved upstream billing data can be discarded on the way into quota/billing normalization.Suggested fix
func (r *GeminiChatResponse) GetUsageMetadata() *GeminiUsageMetadata { if r == nil { return nil } - if r.HasUsageMetadata || HasGeminiUsageMetadataTokens(&r.UsageMetadata) { + if r.HasUsageMetadata || HasGeminiUsageMetadataTokens(&r.UsageMetadata) || r.UsageMetadata.BillingUsage != nil { return &r.UsageMetadata } return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dto/gemini.go` around lines 486 - 493, GetUsageMetadata in GeminiChatResponse is filtering out billing-only metadata by only returning UsageMetadata when HasUsageMetadata is set or token counters are present. Update this accessor to preserve any populated UsageMetadata, including BillingUsage, so the Gemini relay path and service/relayconvert/response_registry.go can keep upstream billing data instead of falling back to estimated usage. Keep the nil receiver guard, but remove the token-only gating and make the check rely on the metadata field itself.service/relayconvert/internal/oai_responses/to_claude_messages_req.go-196-217 (1)
196-217: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject non-image/non-PDF inputs here.
Lines 196-216 route
input_file,input_audio, andinput_videothrough Claude'simage/documentblocks only. Anything that is not a PDF becomesimage, so MP3/MP4/CSV uploads are serialized with an invalid Claude block type and will fail upstream.Suggested fix
- if strings.HasPrefix(mimeType, "application/pdf") { - claudePart.Type = "document" - } else { - claudePart.Type = "image" - } + normalized := strings.ToLower(mimeType) + switch { + case strings.HasPrefix(normalized, "image/"): + claudePart.Type = "image" + case normalized == "application/pdf": + claudePart.Type = "document" + default: + return nil, fmt.Errorf("mime type %q is not supported for Claude input", mimeType) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/oai_responses/to_claude_messages_req.go` around lines 196 - 217, The media conversion in toClaudeMessagesReq is incorrectly treating every non-PDF `input_file`, `input_audio`, and `input_video` as an `image`, which sends invalid Claude block types upstream. Update the switch handling around `ContentPartToFileSource` and `relaymedia.ResolveBase64Data` to only allow supported image/PDF inputs for `dto.ClaudeMediaMessage`, and explicitly reject or skip non-image/non-PDF MIME types (such as audio, video, or generic files) instead of mapping them to `image`.service/relayconvert/internal/oai_responses/to_gemini_chat_req.go-178-186 (1)
178-186: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate invalid
json_schemaerrors.Once line 178 switches Gemini into JSON response mode, an unmarshal failure on lines 183-185 should reject the request. Returning
nilhere silently drops the schema and forwards a less constrained request than the client asked for.Suggested fix
var jsonSchema dto.FormatJsonSchema if err := common.Unmarshal(responseFormat.JsonSchema, &jsonSchema); err != nil { - return nil + return fmt.Errorf("invalid text.format.json_schema: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/oai_responses/to_gemini_chat_req.go` around lines 178 - 186, The JSON response setup in toGeminiChatReq should not silently ignore an invalid json_schema after setting ResponseMimeType in the Gemini request. In the logic around responseFormat.JsonSchema and common.Unmarshal, change the unmarshal failure path to return an error instead of nil so the request is rejected; keep the fix localized to the schema handling in toGeminiChatReq and preserve the existing valid-schema flow.service/relayconvert/internal/gemini_chat/to_oai_chat_req.go-33-34 (1)
33-34: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep tool-call IDs stable across the whole converted request.
toolCallsis reset for each Gemini content block, butFunctionResponsereuseslen(toolCalls)to buildToolCallId. That means a later tool response usually points atcall_0instead of the earlier assistant tool call, so the OpenAI message sequence loses the tool/result pairing.Also applies to: 61-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/gemini_chat/to_oai_chat_req.go` around lines 33 - 34, The tool-call ID generation in toOAIChatReq is unstable because toolCalls is recreated for each Gemini content block, so FunctionResponse ends up reusing len(toolCalls) and producing call_0 for later tool results. Update the conversion logic in toOAIChatReq (including the FunctionCall and FunctionResponse handling paths) to use a request-wide counter or preserved mapping so each tool call gets a stable, unique ToolCallId across all content blocks and the assistant/tool message pairing remains intact.service/relayconvert/internal/jsonutil/stringify.go-9-14 (1)
9-14: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't fall back to Go formatting from a JSON helper.
fmt.Sprintf("%v", v)can return strings likemap[a:b], which are not valid JSON. This helper is used to serialize function arguments and tool responses on the Gemini→OpenAI path, so a marshal failure here turns into a malformed upstream payload instead of a conversion error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/jsonutil/stringify.go` around lines 9 - 14, The ToJSONString helper currently falls back to fmt.Sprintf("%v", v) on marshal failure, which can emit non-JSON output and leak malformed payloads into the Gemini→OpenAI conversion path. Update ToJSONString in stringif y.go to avoid Go formatting entirely: when common.Marshal fails, return a JSON-safe result or propagate the failure through the surrounding conversion flow so callers can surface an error instead of using invalid serialized data. Keep the fix localized to ToJSONString and any immediate callers that depend on it for function arguments or tool responses.service/relayconvert/internal/claude_messages/to_oai_chat_req.go-149-160 (1)
149-160: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve assistant text when the Claude message also contains
tool_use.This branch keeps
toolCallsbut discardsmediaMessageswhenever both exist. Claude assistant turns can interleave text and tool calls, and OpenAI assistant messages support sending both, so the current conversion silently drops prompt-relevant text.Proposed fix
if len(toolCalls) > 0 { openAIMessage.SetToolCalls(toolCalls) } - if len(mediaMessages) > 0 && len(toolCalls) == 0 { + if len(mediaMessages) > 0 { openAIMessage.SetMediaContent(mediaMessages) }Also applies to: 199-204
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/claude_messages/to_oai_chat_req.go` around lines 149 - 160, The conversion logic in to_oai_chat_req is dropping assistant text whenever tool_use is present because the toolCalls branch wins over mediaMessages. Update the Claude-to-OpenAI mapping so mixed assistant turns keep both text and tool calls: preserve the accumulated mediaMessages alongside toolCalls instead of discarding them, and ensure the same handling is applied in the companion branch around the later media/tool_use conversion path. Use the existing dto.ToolCallRequest, dto.MediaContent, and message-building flow to locate and adjust the merge logic.service/relayconvert/internal/gemini_chat/to_oai_chat_req.go-51-60 (1)
51-60: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOnly map Gemini
file_datatoimage_urlfor image MIME types.This converter tags every
FileDatapart as OpenAIimage_url, but the shared Gemini helpers in this PR explicitly allow non-image MIME types such as PDF, audio, and video. Those requests will be turned into invalid OpenAI chat content instead of being rejected or converted via a supported path.Proposed fix
} else if part.FileData != nil { + if !strings.HasPrefix(part.FileData.MimeType, "image/") { + return nil, fmt.Errorf("unsupported Gemini file mime type for OpenAI chat conversion: %s", part.FileData.MimeType) + } mediaContent := dto.MediaContent{ Type: "image_url", ImageUrl: &dto.MessageImageUrl{ Url: part.FileData.FileUri,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/gemini_chat/to_oai_chat_req.go` around lines 51 - 60, The Gemini-to-OpenAI converter in to_oai_chat_req should not map every FileData part to dto.MediaContent{Type: "image_url"}; update the FileData handling branch to inspect part.FileData.MimeType and only create an image_url payload for image MIME types. For non-image MIME types (for example PDF, audio, video), either reject the part with a clear error or route it through a supported conversion path, so the logic in the file_data branch of the converter does not produce invalid OpenAI chat content.service/relayconvert/internal/gemini_chat/to_oai_chat_resp.go-72-74 (1)
72-74: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon't relabel image-only prompt tokens as text tokens.
This fallback fires whenever text/audio are zero, even if
ImageTokensis already populated. For image-only prompts it duplicates the full prompt total intoTextTokens, which corrupts the modality breakdown used downstream for usage/billing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/gemini_chat/to_oai_chat_resp.go` around lines 72 - 74, The fallback in the usage normalization logic is too broad and relabels image-only prompts as text tokens. Update the check in the gemini chat response conversion path around the usage details handling so it only copies `PromptTokens` into `TextTokens` when there are truly no modality-specific tokens set, and explicitly exclude cases where `ImageTokens` is already populated. Use the `usage.PromptTokensDetails` fields in `to_oai_chat_resp` to preserve the correct modality breakdown for downstream billing.service/relayconvert/internal/oai_chat/to_gemini_chat_resp.go-57-83 (1)
57-83: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve OpenAI reasoning content as Gemini
Thoughtparts.Both converters ignore
ReasoningContent, and the stream precheck also drops reasoning-only chunks as “empty”. That breaks round-tripping withservice/relayconvert/internal/gemini_chat/to_oai_chat_resp.go, which already maps Gemini thought parts into OpenAI reasoning content.Also applies to: 97-109, 165-195
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/oai_chat/to_gemini_chat_resp.go` around lines 57 - 83, The OpenAI-to-Gemini conversion in toGeminiChatResp currently drops ReasoningContent, so update the response mapping to preserve it as Gemini Thought parts alongside text and function calls. Add the same handling in the stream precheck/empty-chunk path and any other affected converter blocks referenced in this diff so reasoning-only messages are not discarded. Use the existing structures in choice.Message, dto.GeminiPart, and the related converter logic to keep round-tripping compatible with to_oai_chat_resp.go.service/relayconvert/internal/oai_chat/to_claude_messages_resp.go-281-290 (1)
281-290: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winProcess the current delta before deferring close on
finish_reason.This early return happens before
chosenChoice.Deltais converted. If an upstream sends final content/tool data in the same chunk asfinish_reasonand postpones usage to a later chunk, that last delta is silently discarded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/oai_chat/to_claude_messages_resp.go` around lines 281 - 290, The finish_reason handling in toClaudeMessagesResp is returning too early in the doneChunk branch, which drops any final Delta content/tool data in the same chunk. In to_claude_messages_resp.go, update the logic around chosenChoice.FinishReason so the current chosenChoice.Delta is converted and appended before deferring closure for usage, and only return early after processing that delta when openAIResponse.Usage is still nil. Keep the fix localized to the doneChunk handling in toClaudeMessagesResp.service/relayconvert/internal/oai_chat/to_claude_messages_resp.go-134-171 (1)
134-171: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe first streaming tool-call chunk drops every tool after index 0.
The
SendResponseCount == 1branch selects a singletoolCalland never iterates the rest ofopenAIResponse.Choices[0].Delta.ToolCalls. If the opener chunk carries multiple parallel tool calls, only the first Claudetool_useblock is emitted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/oai_chat/to_claude_messages_resp.go` around lines 134 - 171, The first streaming tool-call chunk only emits the first tool call and ignores any additional parallel calls. Update the SendResponseCount == 1 handling in to_claude_messages_resp.go to iterate over openAIResponse.Choices[0].Delta.ToolCalls (and the fallback GetFirstToolCall path if needed) and append a Claude content_block_start for each tool call, preserving each tool’s ID, Function.Name, and Arguments instead of hardcoding a single toolCall at index 0.service/relayconvert/internal/gemini_chat/to_oai_chat_resp.go-86-87 (1)
86-87: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTrack tool-call finish state per candidate, not per response.
isToolCallis declared outside the candidate loop and never reset. After the first tool-call candidate, every later choice is forced totool_callseven when that candidate only contains plain text.Also applies to: 158-178
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/gemini_chat/to_oai_chat_resp.go` around lines 86 - 87, The tool-call finish state is being carried across candidates because isToolCall is initialized before the candidate loop and reused in toOAIChatResp, which causes later candidates to inherit tool_calls incorrectly. Update the candidate processing logic in toOAIChatResp so the tool-call check is computed and reset per candidate (and likewise for the related response-building path mentioned in the later block), using each candidate’s own finish/parts data instead of a shared flag.service/relayconvert/internal/oai_chat/to_claude_messages_resp.go-42-48 (1)
42-48: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon't double-count cached prompt tokens in synthesized Claude usage.
PromptTokensis already the OpenAI-side total, including cached prompt details. Copying it intoInputTokensand also populatingCacheReadInputTokens/CacheCreationInputTokensinflates Anthropic usage whenever cache fields are present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/oai_chat/to_claude_messages_resp.go` around lines 42 - 48, The Claude usage synthesis in toClaudeMessagesResp is double-counting cached prompt tokens by setting ClaudeUsage.InputTokens from oaiUsage.PromptTokens while also filling CacheReadInputTokens and CacheCreationInputTokens. Update the mapping in this function so InputTokens reflects only the non-cached prompt portion when PromptTokensDetails is present, and keep the cache fields as the explicit cached amounts; use the existing oaiUsage and ClaudeUsage symbols to adjust the conversion logic without inflating billingUsage.service/relayconvert/internal/claude_messages/to_oai_chat_resp.go-124-142 (1)
124-142: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAccumulate Claude text/thinking blocks instead of overwriting them.
This loop replaces
responseTextandthinkingContenton each matching block, so a Claude message with multipletextorthinkingparts only returns the last one after conversion.Suggested fix
- for _, message := range claudeResponse.Content { + var textBuilder strings.Builder + var thinkingBuilder strings.Builder + for _, message := range claudeResponse.Content { switch message.Type { case "tool_use": args, _ := common.Marshal(message.Input) tools = append(tools, dto.ToolCallResponse{ @@ case "thinking": if message.Thinking != nil { - thinkingContent = *message.Thinking + thinkingBuilder.WriteString(*message.Thinking) } case "text": - responseText = message.GetText() + textBuilder.WriteString(message.GetText()) } } + responseText = textBuilder.String() + thinkingContent = thinkingBuilder.String()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/claude_messages/to_oai_chat_resp.go` around lines 124 - 142, The loop in to_oai_chat_resp.go overwrites responseText and thinkingContent for each Claude content block, so only the last text/thinking segment survives. Update the claudeResponse.Content handling in the conversion logic to accumulate all "text" blocks into responseText and all "thinking" blocks into thinkingContent instead of replacing prior values, while keeping the existing tool_use handling unchanged.service/relayconvert/internal/oai_responses/to_oai_chat_resp.go-131-140 (1)
131-140: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep absent responses usage nil.
When
resp.Usageisnil, this helper still returns&dto.Usage{}. That makes a responses payload with no accounting data look like a zero-token usage record, and registry callers will treat it as present. Please returnnilfornilinput here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/oai_responses/to_oai_chat_resp.go` around lines 131 - 140, The UsageFromResponsesUsage helper currently turns a nil input into an empty dto.Usage, which makes absent responses usage look present. Update UsageFromResponsesUsage to return nil immediately when src is nil, and keep the existing field cloning/population logic only for non-nil inputs so registry callers can distinguish missing usage from a real zero-valued record.service/relayconvert/internal/oai_responses/to_oai_chat_resp.go-201-207 (1)
201-207: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't fall back to reasoning text as assistant content.
This fallback appends every non-empty
c.Text, includingreasoning/summary_textitems. A chat→responses→chat round-trip with empty assistant content but non-empty reasoning will come back with the reasoning duplicated into bothMessage.ContentandMessage.ReasoningContent.Possible fix
- for _, out := range resp.Output { - for _, c := range out.Content { - if c.Text != "" { - sb.WriteString(c.Text) - } - } - } + for _, out := range resp.Output { + if out.Type != responsesOutputTypeMessage { + continue + } + if out.Role != "" && out.Role != "assistant" { + continue + } + for _, c := range out.Content { + if c.Type == "output_text" && c.Text != "" { + sb.WriteString(c.Text) + } + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/oai_responses/to_oai_chat_resp.go` around lines 201 - 207, The chat-content reconstruction in toOAIChatResp is incorrectly appending every non-empty Output.Content.Text, which can pull in reasoning/summary text as assistant content. Update the loop in the chat response builder to only collect actual assistant message text and explicitly skip reasoning-related content types so Message.Content is not duplicated from Message.ReasoningContent on round-trips.service/relayconvert/internal/oai_chat/to_oai_responses_resp.go-111-153 (1)
111-153: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTreat empty chat usage as absent.
dto.OpenAITextResponse.Usageis a value, so an omitted upstreamusagearrives here as an all-zero struct. Returning a non-nil*dto.Usagefor that case turns “usage missing” into “0 tokens”, and downstream registry code will treat it as a real accounting snapshot. On multi-hop conversions that can wipe preservedBillingUsage/token totals with zeros.Possible fix
func UsageFromChatUsage(src *dto.Usage) *dto.Usage { - usage := &dto.Usage{} - if src == nil { - return usage - } + if src == nil { + return nil + } + if !dto.HasOpenAIUsageTokens(src) && + src.BillingUsage == nil && + src.Cost == 0 && + src.PromptTokensDetails == (dto.InputTokenDetails{}) && + src.CompletionTokenDetails == (dto.OutputTokenDetails{}) && + src.ClaudeCacheCreation5mTokens == 0 && + src.ClaudeCacheCreation1hTokens == 0 { + return nil + } + usage := &dto.Usage{}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/internal/oai_chat/to_oai_responses_resp.go` around lines 111 - 153, Treat an all-zero chat usage as absent in UsageFromChatUsage: when src is non-nil but every usage field is zero, return nil instead of constructing an empty dto.Usage. Update the conversion logic in UsageFromChatUsage to detect this “empty upstream usage” case before populating BillingUsage, token counts, or details, so downstream code in the relay conversion path preserves existing accounting data instead of overwriting it with zeros.service/relayconvert/request_registry.go-325-344 (1)
325-344: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid running Responses→Gemini preprocessing twice.
For the direct OpenAI Responses→Gemini registry path,
prepareRequestForStepprepares the request before the step, thenconvertOpenAIResponsesRequestToGeminiChatprepares it again. Keep this normalization in one layer to avoid duplicated/non-idempotent transformations.🐛 Proposed fix
func convertOpenAIResponsesRequestToGeminiChat(c *gin.Context, info *relaycommon.RelayInfo, request any) (any, error) { responsesRequest, err := oairesponses.OpenAIResponsesRequestFromAny(request) if err != nil { return nil, err } - - prepared, err := oairesponses.PrepareOpenAIResponsesRequest(*responsesRequest) - if err != nil { - return nil, err - } - return oairesponses.OpenAIResponsesRequestToGeminiChat(c, &prepared, info) + return oairesponses.OpenAIResponsesRequestToGeminiChat(c, responsesRequest, info) }Also applies to: 475-485
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/request_registry.go` around lines 325 - 344, Avoid double-normalizing OpenAI Responses requests on the direct Responses-to-Gemini path: `prepareRequestForStep` is already calling `oairesponses.PrepareOpenAIResponsesRequest`, and `convertOpenAIResponsesRequestToGeminiChat` should not prepare the same payload again. Update one of these layers so the normalization happens only once, keeping the registry path in `request_registry.go` and the Gemini converter consistent for both the direct flow and the shared helper used elsewhere.service/relayconvert/response_registry.go-468-503 (1)
468-503: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSuppress nil stream conversions before returning a result.
StreamResponseOpenAI2Geminican returnnilfor empty leading stream chunks, butexecuteStatelessStreamResponseSpecstill wraps that as a successfulResponseResultwithValue == nil. That can make callers write an invalid/empty stream payload instead of skipping the chunk.🐛 Proposed fix
var err error current, usage, err = step.ConvertStream(c, info, current) if err != nil { return nil, err } + if len(streamValuesFromAny(current)) == 0 { + return nil, nil + } resultSteps = append(resultSteps, ResponseStep{ Converter: step.ID,Also applies to: 908-914
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/response_registry.go` around lines 468 - 503, `executeStatelessStreamResponseSpec` currently returns a successful `ResponseResult` even when a stream converter like `StreamResponseOpenAI2Gemini` produces a nil `current` for empty leading chunks. Update the loop in `executeStatelessStreamResponseSpec` to detect a nil stream conversion result and suppress it by returning no result for that chunk instead of wrapping it in `ResponseResult`; keep the existing step validation and error handling intact, and ensure callers can skip empty stream payloads rather than emitting an invalid nil value.service/relayconvert/request_compat.go-47-48 (1)
47-48: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply Responses→Gemini preprocessing in the compatibility wrapper.
Line 48 bypasses the preparation that the registry applies before OpenAI Responses→Gemini conversion, so direct callers of this public wrapper can get different request normalization than
ConvertRequest.🐛 Proposed fix
import ( + "errors" + "github.com/QuantumNous/new-api/dto" relaycommon "github.com/QuantumNous/new-api/relay/common" @@ func OpenAIResponsesRequestToGeminiChat(c *gin.Context, req *dto.OpenAIResponsesRequest, info *relaycommon.RelayInfo) (*dto.GeminiChatRequest, error) { - return oairesponses.OpenAIResponsesRequestToGeminiChat(c, req, info) + if req == nil { + return nil, errors.New("OpenAI responses request is nil") + } + prepared, err := oairesponses.PrepareOpenAIResponsesRequest(*req) + if err != nil { + return nil, err + } + return oairesponses.OpenAIResponsesRequestToGeminiChat(c, &prepared, info) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/relayconvert/request_compat.go` around lines 47 - 48, The OpenAIResponsesRequestToGeminiChat compatibility wrapper bypasses the same preprocessing that ConvertRequest applies before Responses-to-Gemini conversion, so direct callers can get inconsistent normalization. Update OpenAIResponsesRequestToGeminiChat in request_compat.go to run the same Responses→Gemini preprocessing path used by the registry (via the relevant oairesponses/registry helper) before delegating to the Gemini conversion, while keeping the existing function signature and delegation structure intact.
🟡 Minor comments (3)
web/default/src/features/usage-logs/components/dialogs/details-dialog.tsx-1030-1035 (1)
1030-1035: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHide decorative billing-path icons from assistive tech.
These icons are redundant with the adjacent text label, so add
aria-hidden='true'. As per coding guidelines, decorative icons should be hidden from assistive tech.- <Monitor className='size-3 text-blue-500' /> + <Monitor className='size-3 text-blue-500' aria-hidden='true' /> ... - <Cloud className='size-3 text-emerald-500' /> + <Cloud className='size-3 text-emerald-500' aria-hidden='true' />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/usage-logs/components/dialogs/details-dialog.tsx` around lines 1030 - 1035, Hide the decorative billing-path icons in the usage log details dialog from assistive tech by adding aria-hidden='true' to the Monitor and Cloud icon renders inside the details dialog component. Update the conditional icon block in details-dialog.tsx so the icons remain visually shown next to the label but are excluded from accessibility APIs.Source: Coding guidelines
web/default/src/features/channels/components/dialogs/advanced-custom-editor-dialog.tsx-762-773 (1)
762-773: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLocalize incoming-path option labels.
Line 769 renders
option.labeldirectly in the new route group selector. Wrap it witht()like the other user-facing labels in this component. As per coding guidelines, React UI text must support i18n withuseTranslation()andt().- <span>{option.label}</span> + <span>{t(option.label)}</span>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/channels/components/dialogs/advanced-custom-editor-dialog.tsx` around lines 762 - 773, The incoming-path option label in the advanced custom editor dialog is still rendered as raw UI text. Update the SelectItem rendering in advanced-custom-editor-dialog.tsx so the option label is passed through the existing i18n flow using useTranslation() and t(), matching the other localized labels in this component; keep option.value as-is and only localize the displayed label text.Source: Coding guidelines
web/default/src/features/channels/lib/advanced-custom.ts-724-731 (1)
724-731: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd regex syntax validation to match backend validation.
The frontend only rejects empty regex patterns but the backend also validates regex syntax with
regexp.Compile(). Patterns likere:[currently pass frontend validation but fail at runtime. Add a try-catch block to test the regex pattern before save, using the same validation as the backend:Backend validation (Go)
// ./dto/channel_settings.go lines 383-387 if pattern == "" { return fmt.Errorf("regex is empty") } if _, err := regexp.Compile(pattern); err != nil { return fmt.Errorf("regex is invalid") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/channels/lib/advanced-custom.ts` around lines 724 - 731, The regex validation in advanced-custom.ts only checks for empty patterns, so it should also validate regex syntax to match backend behavior. In the model validation flow where getAdvancedCustomModelRuleKind and getAdvancedCustomRegexModelPattern are used, add a try-catch around compiling the extracted pattern and return a validation message when compilation fails, alongside the existing empty-pattern check. Keep the check in the same save/validation path so invalid regexes like malformed patterns are rejected before persistence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49aa5a81-8b09-464e-a37f-547640f89e26
📒 Files selected for processing (98)
controller/model_list_test.godto/billing_usage.godto/billing_usage_test.godto/channel_settings.godto/channel_settings_test.godto/claude.godto/gemini.godto/gemini_response_test.godto/openai_response.gomain.gomiddleware/distributor.gomodel/ability.gomodel/channel_cache.gomodel/pricing.gomodel/pricing_endpoint_test.gorelay/channel/advancedcustom/adaptor.gorelay/channel/advancedcustom/adaptor_test.gorelay/channel/ali/adaptor.gorelay/channel/api_request.gorelay/channel/aws/adaptor.gorelay/channel/claude/adaptor.gorelay/channel/claude/relay-claude.gorelay/channel/claude/relay_claude_test.gorelay/channel/gemini/adaptor.gorelay/channel/gemini/relay-gemini-native.gorelay/channel/gemini/relay-gemini.gorelay/channel/gemini/relay_gemini_usage_test.gorelay/channel/gemini/relay_responses.gorelay/channel/openai/adaptor.gorelay/channel/openai/chat_via_responses.gorelay/channel/openai/helper.gorelay/channel/openai/relay-openai.gorelay/channel/openai/responses_via_chat.gorelay/channel/vertex/adaptor.gorelay/chat_completions_via_responses.gorelay/claude_handler.gorelay/common/relay_utils.gorelay/common/relay_utils_test.gorelay/gemini_handler.goservice/billing_usage.goservice/convert.goservice/convert_test.goservice/relayconvert/internal/claude_messages/to_oai_chat_req.goservice/relayconvert/internal/claude_messages/to_oai_chat_resp.goservice/relayconvert/internal/gemini_chat/to_oai_chat_req.goservice/relayconvert/internal/gemini_chat/to_oai_chat_resp.goservice/relayconvert/internal/jsonutil/stringify.goservice/relayconvert/internal/matcher/regex.goservice/relayconvert/internal/media/media.goservice/relayconvert/internal/meta/relay_info.goservice/relayconvert/internal/oai_chat/to_claude_messages_req.goservice/relayconvert/internal/oai_chat/to_claude_messages_resp.goservice/relayconvert/internal/oai_chat/to_claude_messages_resp_test.goservice/relayconvert/internal/oai_chat/to_gemini_chat_req.goservice/relayconvert/internal/oai_chat/to_gemini_chat_resp.goservice/relayconvert/internal/oai_chat/to_gemini_chat_resp_test.goservice/relayconvert/internal/oai_chat/to_oai_responses_policy.goservice/relayconvert/internal/oai_chat/to_oai_responses_req.goservice/relayconvert/internal/oai_chat/to_oai_responses_req_test.goservice/relayconvert/internal/oai_chat/to_oai_responses_resp.goservice/relayconvert/internal/oai_chat/to_oai_responses_resp_test.goservice/relayconvert/internal/oai_chat/to_oai_responses_stream_resp.goservice/relayconvert/internal/oai_responses/req_helpers.goservice/relayconvert/internal/oai_responses/to_claude_messages_req.goservice/relayconvert/internal/oai_responses/to_gemini_chat_req.goservice/relayconvert/internal/oai_responses/to_gemini_chat_req_preprocess.goservice/relayconvert/internal/oai_responses/to_oai_chat_req.goservice/relayconvert/internal/oai_responses/to_oai_chat_req_test.goservice/relayconvert/internal/oai_responses/to_oai_chat_resp.goservice/relayconvert/internal/oai_responses/to_oai_chat_resp_test.goservice/relayconvert/internal/oai_responses/to_oai_chat_stream_resp.goservice/relayconvert/internal/shared/claude/cache.goservice/relayconvert/internal/shared/claude/tool_choice.goservice/relayconvert/internal/shared/gemini/request.goservice/relayconvert/internal/shared/gemini/schema.goservice/relayconvert/media.goservice/relayconvert/request_compat.goservice/relayconvert/request_registry.goservice/relayconvert/request_registry_test.goservice/relayconvert/response_compat.goservice/relayconvert/response_registry.goservice/relayconvert/response_registry_test.goservice/relayconvert/text_converter_registry.goservice/relayconvert/text_converter_registry_test.goservice/request_converter.goservice/text_quota.goservice/text_quota_test.goweb/default/src/features/channels/components/dialogs/advanced-custom-editor-dialog.tsxweb/default/src/features/channels/lib/advanced-custom.tsweb/default/src/features/channels/types.tsweb/default/src/features/usage-logs/components/dialogs/details-dialog.tsxweb/default/src/features/usage-logs/types.tsweb/default/src/i18n/locales/en.jsonweb/default/src/i18n/locales/fr.jsonweb/default/src/i18n/locales/ja.jsonweb/default/src/i18n/locales/ru.jsonweb/default/src/i18n/locales/vi.jsonweb/default/src/i18n/locales/zh.json
# Conflicts: # relay/common/relay_utils_test.go # web/default/src/features/channels/components/dialogs/advanced-custom-editor-dialog.tsx # web/default/src/features/channels/lib/advanced-custom.ts
- estimate Gemini completion tokens locally when billable usageMetadata is prompt-only but output content was received (e.g. client aborts the stream before the final chunk), and rebuild the attached billing_usage as estimated so settlement does not bill zero output tokens - guard NewClaudeMessagesBillingUsage against all-zero ClaudeUsage, matching the OpenAI/Gemini constructors, so a zero billing_usage cannot override a non-zero top-level usage during settlement - cache compiled advanced-custom route model regexes; they run on the request hot path and were recompiled per request - move the effectiveBillingUsage remap to PostTextConsumeQuota only, and document that calculateTextQuotaSummary expects remapped usage - document the updatePricingLock -> channelSyncLock lock ordering that InitChannelCache/CacheUpdateChannel rely on, and the aux-struct pitfall in GeminiChatResponse.UnmarshalJSON
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
.github/workflows/electron-build.yml (1)
136-136: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueOptional: replace
softprops/action-gh-releasewithgh releaseCLI.zizmor flags this as functionality already provided by the runner via the GitHub CLI, removing a third-party action dependency. Low priority given the current pinned SHA is already reasonably secure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/electron-build.yml at line 136, Replace the release step that uses softprops/action-gh-release in the Electron build workflow with the GitHub CLI release flow instead. Update the release job/step to use gh release so the existing workflow logic still creates/uploads the release artifacts without depending on the third-party action, and keep the surrounding release configuration in the same workflow block.Source: Linters/SAST tools
.github/workflows/release.yml (2)
67-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional:
softprops/action-gh-releaseduplicates runner-providedghfunctionality.zizmor notes this functionality is already available via the pre-installed
ghCLI, so the third-party action could be replaced with agh release create/gh release uploadscript step. Current usage works fine; this is a stylistic simplification only.Also applies to: 122-122, 179-179
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml at line 67, The release workflow still relies on softprops/action-gh-release in multiple places, but this can be simplified by using the runner’s built-in gh CLI instead. Update the release job steps that currently use the action to equivalent gh release create and gh release upload commands, keeping the existing release behavior and tags/assets handling intact.Source: Linters/SAST tools
29-29: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable default caching on
setup-bun/setup-goin the release jobs.Static analysis flags cache-poisoning risk: these actions enable dependency caching by default. For a release pipeline that produces publicly distributed binaries, a cache poisoned by an earlier (possibly untrusted) workflow run sharing the same cache key could get restored here.
🔒️ Proposed fix: disable caching for release builds
- uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6.5.0 with: go-version: '>=1.25.1' + cache: falseApply similarly to the
oven-sh/setup-bunsteps if it exposes a cache-disable input.Also applies to: 51-51, 88-88, 111-111, 146-146, 168-168
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml at line 29, The release workflow’s setup steps are leaving dependency caching enabled by default, which should be disabled for all release jobs. Update the `setup-bun` and `setup-go` usages in the release workflow to explicitly turn off caching via their cache-disable inputs or equivalent flags, and apply the same change consistently to every matching setup step in the workflow so the release pipeline never restores shared caches.Source: Linters/SAST tools
relay/channel/gemini/relay-gemini.go (1)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate magic number
1400for the image-token completion estimate heuristic.Consider extracting this into a shared named constant (e.g.
geminiImageCompletionTokenEstimate) to avoid drift between the two call sites.Also applies to: 183-183
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@relay/channel/gemini/relay-gemini.go` at line 55, The Gemini image completion estimate currently uses the same magic number at multiple call sites, which risks divergence over time. Extract the shared heuristic value from the code paths in the Gemini relay logic (including the completion token assignment in the relevant Gemini helper/methods) into a named constant such as geminiImageCompletionTokenEstimate, and replace both literal uses with that constant so the estimate stays consistent in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/electron-build.yml:
- Line 136: Replace the release step that uses softprops/action-gh-release in
the Electron build workflow with the GitHub CLI release flow instead. Update the
release job/step to use gh release so the existing workflow logic still
creates/uploads the release artifacts without depending on the third-party
action, and keep the surrounding release configuration in the same workflow
block.
In @.github/workflows/release.yml:
- Line 67: The release workflow still relies on softprops/action-gh-release in
multiple places, but this can be simplified by using the runner’s built-in gh
CLI instead. Update the release job steps that currently use the action to
equivalent gh release create and gh release upload commands, keeping the
existing release behavior and tags/assets handling intact.
- Line 29: The release workflow’s setup steps are leaving dependency caching
enabled by default, which should be disabled for all release jobs. Update the
`setup-bun` and `setup-go` usages in the release workflow to explicitly turn off
caching via their cache-disable inputs or equivalent flags, and apply the same
change consistently to every matching setup step in the workflow so the release
pipeline never restores shared caches.
In `@relay/channel/gemini/relay-gemini.go`:
- Line 55: The Gemini image completion estimate currently uses the same magic
number at multiple call sites, which risks divergence over time. Extract the
shared heuristic value from the code paths in the Gemini relay logic (including
the completion token assignment in the relevant Gemini helper/methods) into a
named constant such as geminiImageCompletionTokenEstimate, and replace both
literal uses with that constant so the estimate stays consistent in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7bcd7d3-71c5-4331-ba76-fd69c9f4f16e
📒 Files selected for processing (15)
.github/workflows/docker-build.yml.github/workflows/docker-image-branch.yml.github/workflows/electron-build.yml.github/workflows/release.ymldto/billing_usage.godto/billing_usage_test.godto/channel_settings.godto/channel_settings_test.godto/gemini.gomodel/channel_cache.gomodel/pricing.gorelay/channel/gemini/relay-gemini.gorelay/channel/gemini/relay_gemini_usage_test.goservice/text_quota.goservice/text_quota_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- dto/billing_usage_test.go
- .github/workflows/docker-build.yml
- model/pricing.go
- dto/gemini.go
- dto/billing_usage.go
- service/text_quota_test.go
- model/channel_cache.go
- dto/channel_settings_test.go
- dto/channel_settings.go
# Conflicts: # web/default/src/features/usage-logs/components/dialogs/details-dialog.tsx # web/default/src/i18n/locales/en.json # web/default/src/i18n/locales/fr.json # web/default/src/i18n/locales/ja.json # web/default/src/i18n/locales/ru.json # web/default/src/i18n/locales/vi.json # web/default/src/i18n/locales/zh.json
* upstream/main: (56 commits) fix: list only channel models in unset price models tab fix: harden unset price models tab batch copy, feedback, and memo equality feat: add unset price models tab to model pricing settings (QuantumNous#6124) feat: enhance model search functionality with status and sync filters fix: bound uncached remainder by prompt-max(cached,write) and forward compact prompt_cache_key feat: bill OpenAI cache_write_tokens at cache-creation price with zero clamp feat: enhance text protocol conversion and advanced custom routing (QuantumNous#5825) fix: adjust margin for StatusBadge component in logs columns feat: enhance stale instance handling and update theme colors feat: update theme colors feat(timing): add timing metrics display for stream logs and enhance localization revert: restore StatusBadge horizontal padding revert: undo t0ng7u UI design-system refactor ✨ feat(web): polish themed data views and add task log details feat(image): enhance image stream handling with client disconnect logic and billing adjustments fix(billing): improve quota handling and error reporting for pre-consume operations fix(billing): reject saturated pre-consume quota 🐛 fix: Fontsource asset resolution across workspace layouts fix: sync codex field (QuantumNous#6018) chore(deps): bump golang.org/x/crypto from 0.51.0 to 0.52.0 (QuantumNous#6096) ...
…uantumNous#5825) * refactor: consolidate relay protocol converters * refactor relayconvert text converters * feat: refine relay converters and advanced custom routing * refactor: enhance logging and add thought signature handling for Gemini requests * refactor: enhance channel cache and pricing endpoint handling for advanced custom models * feat: preserve billing usage semantics * feat: add protocol-aware billing usage * Delete useless files * chore: update action versions in workflow files * chore: update Docker action versions in workflow files * fix: harden billing usage settlement and hot-path route matching - estimate Gemini completion tokens locally when billable usageMetadata is prompt-only but output content was received (e.g. client aborts the stream before the final chunk), and rebuild the attached billing_usage as estimated so settlement does not bill zero output tokens - guard NewClaudeMessagesBillingUsage against all-zero ClaudeUsage, matching the OpenAI/Gemini constructors, so a zero billing_usage cannot override a non-zero top-level usage during settlement - cache compiled advanced-custom route model regexes; they run on the request hot path and were recompiled per request - move the effectiveBillingUsage remap to PostTextConsumeQuota only, and document that calculateTextQuotaSummary expects remapped usage - document the updatePricingLock -> channelSyncLock lock ordering that InitChannelCache/CacheUpdateChannel rely on, and the aux-struct pitfall in GeminiChatResponse.UnmarshalJSON
PR QuantumNous#6177（隐藏未定价模型）只更新了 controller/model_list_test.go，漏了上游 model/pricing_endpoint_test.go（来自 PR QuantumNous#5825）——后者用未定价模型验证 endpoint 类型推断， 依赖「未定价模型出现在 GetPricing」。PR 后非自用模式下这些模型被排除，7 个测试挂。 在共享 helper resetPricingEndpointTestTables 临时开启自用模式（带还原），让未定价测试模型 仍以默认档暴露、复刻 PR 前可见性；endpoint 推断断言不受影响。合并上游若其补了同一测试需 留意此处。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…uantumNous#5825) * refactor: consolidate relay protocol converters * refactor relayconvert text converters * feat: refine relay converters and advanced custom routing * refactor: enhance logging and add thought signature handling for Gemini requests * refactor: enhance channel cache and pricing endpoint handling for advanced custom models * feat: preserve billing usage semantics * feat: add protocol-aware billing usage * Delete useless files * chore: update action versions in workflow files * chore: update Docker action versions in workflow files * fix: harden billing usage settlement and hot-path route matching - estimate Gemini completion tokens locally when billable usageMetadata is prompt-only but output content was received (e.g. client aborts the stream before the final chunk), and rebuild the attached billing_usage as estimated so settlement does not bill zero output tokens - guard NewClaudeMessagesBillingUsage against all-zero ClaudeUsage, matching the OpenAI/Gemini constructors, so a zero billing_usage cannot override a non-zero top-level usage during settlement - cache compiled advanced-custom route model regexes; they run on the request hot path and were recompiled per request - move the effectiveBillingUsage remap to PostTextConsumeQuota only, and document that calculateTextQuotaSummary expects remapped usage - document the updatePricingLock -> channelSyncLock lock ordering that InitChannelCache/CacheUpdateChannel rely on, and the aux-struct pitfall in GeminiChatResponse.UnmarshalJSON
…uantumNous#5825) * refactor: consolidate relay protocol converters * refactor relayconvert text converters * feat: refine relay converters and advanced custom routing * refactor: enhance logging and add thought signature handling for Gemini requests * refactor: enhance channel cache and pricing endpoint handling for advanced custom models * feat: preserve billing usage semantics * feat: add protocol-aware billing usage * Delete useless files * chore: update action versions in workflow files * chore: update Docker action versions in workflow files * fix: harden billing usage settlement and hot-path route matching - estimate Gemini completion tokens locally when billable usageMetadata is prompt-only but output content was received (e.g. client aborts the stream before the final chunk), and rebuild the attached billing_usage as estimated so settlement does not bill zero output tokens - guard NewClaudeMessagesBillingUsage against all-zero ClaudeUsage, matching the OpenAI/Gemini constructors, so a zero billing_usage cannot override a non-zero top-level usage during settlement - cache compiled advanced-custom route model regexes; they run on the request hot path and were recompiled per request - move the effectiveBillingUsage remap to PostTextConsumeQuota only, and document that calculateTextQuotaSummary expects remapped usage - document the updatePricingLock -> channelSyncLock lock ordering that InitChannelCache/CacheUpdateChannel rely on, and the aux-struct pitfall in GeminiChatResponse.UnmarshalJSON
1 participant
Important
📝 变更描述 / Description
本 PR 整理并统一了文本协议转换链，将 OpenAI Chat、OpenAI Responses、Claude Messages、Gemini Chat 的 request / response 转换集中到
service/relayconvert，通过统一 registry 声明转换方向、质量、请求转换、响应转换和多步转换链路。原本散落在 channel 和 service 层的纯 DTO 转换逻辑被收拢到来源协议对应的内部包中，channel 层主要保留 HTTP、SSE、鉴权和分发逻辑。同时增强了 Advanced Custom 渠道能力：支持同一入口路径按客户端
model分流，支持re:正则模型匹配，允许/v1/responses同时路由到 OpenAI Chat 和 Gemini Chat；前端编辑器改为按入口路径分组，增加排序、兜底提示、converter 默认上游配置自动填充和更清晰的 converter 展示。计费侧新增协议感知的
billing_usage容器，用于在协议转换后保留真实上游 usage 语义。最终响应仍保持客户端入口协议的 usage 格式，但会额外携带 OpenAI、Claude 或 Gemini 的原始计费 usage，便于下游按真实协议模式计费。用量日志也增加了 billing path 展示，能区分普通上游返回、billing_usage 以及 estimated billing_usage。此外，Advanced Custom 的
/v1/modelsendpoint 能力不再按渠道类型硬编码，而是根据实际配置的入口路径和模型匹配规则推断，并修正了启动时 pricing 预热早于 channel cache 初始化导致初始 endpoint 不准确的问题。🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
已执行过的主要验证：