[codex] feat: add business image fallback - #5234
Closed
neta-zjj wants to merge 1 commit into
Closed
Conversation
Contributor
WalkthroughThis PR implements a comprehensive business image generation fallback system that enables model-to-model retry chains with health-based blocking. It introduces configurable fallback plans, Gemini-to-OpenAI request conversion, health tracking via Redis, channel-selection filtering, and specialized UI for configuration management. ChangesBusiness Image Generation Fallback Feature
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
model/channel_cache.go (1)
231-244: ⚖️ Poor tradeoffN+1 query pattern when loading channels.
Each ability triggers a separate
DB.Firstcall. For a model with many channel abilities, this results in N+1 queries.Consider batch-loading all channel IDs upfront with a single
DB.Where("id IN ?", channelIds).Find(&channels)query.🤖 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 `@model/channel_cache.go` around lines 231 - 244, The loop over abilities causes N+1 queries because DB.First is called per ability; instead, before the loop collect all unique ability.ChannelId values that are missing from the channels map, perform a single batch load using DB.Where("id IN ?", ids).Find(&loadedChannels) (mapping results to Channel objects), populate the channels[ChannelId] entries from that batch, and then run the existing filtering/append logic (using filteredAbilities, filter, Channel type) without any per-ability DB calls.controller/relay.go (1)
385-389: 💤 Low valueState restoration occurs only on failure path.
The
deferat lines 385-389 restores context keys, butrelayInfofields (OriginModelName, Request, RelayFormat, etc.) are restored at lines 469-477 which only executes when the loop completes without early success return (line 452). On success,relayInforemains in the modified state.This is likely intentional since successful requests complete the flow, but document this behavior or consider using defer for consistency if callers may inspect
relayInfoafter success.Also applies to: 469-477
🤖 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 `@controller/relay.go` around lines 385 - 389, The code currently restores context keys via a defer (common.SetContextKey, service.SetBusinessFallbackFamily, service.SetBusinessFallbackActive) but restores relayInfo fields (relayInfo.OriginModelName, relayInfo.Request, relayInfo.RelayFormat, etc.) only on the failure/loop-completion path; on success those relayInfo fields remain modified—either move the relayInfo restoration into the same defer so original values are captured and always restored, or add an explicit comment by the relayInfo modifications documenting that leaving them modified on success is intentional; if you choose defer, capture original values of relayInfo.* before mutation and restore them in the defer to guarantee consistent state for callers.
🤖 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.
Inline comments:
In `@controller/relay.go`:
- Around line 556-565: The code creates dto.GeminiPart with a hardcoded MimeType
"image/png" for each image in imageResponse.Data when image.B64Json is present;
update the logic in the block that builds parts (where dto.GeminiPart and
dto.GeminiInlineData are used) to detect the correct MIME type instead of
hardcoding: if image.B64Json contains a data URI prefix (e.g.,
"data:image/...;base64,"), parse the MIME type from that prefix; otherwise,
prefer any content-type/format field available on the upstream image item (e.g.,
image.ContentType or image.Type) and fall back to "application/octet-stream"
only if nothing is available; set dto.GeminiInlineData.MimeType to the detected
value. Ensure the detection handles common types like image/png, image/jpeg,
image/webp and strips the base64 prefix before assigning Data.
In `@middleware/distributor.go`:
- Around line 185-199: The current shouldUseBusinessImageFallbackSelection
function incorrectly treats any Gemini :generateContent URL as an image request;
change it to also inspect the request body for true image indicators before
returning true. In shouldUseBusinessImageFallbackSelection, after the existing
URL checks, read and preserve the request body (use c.GetRawData or io.ReadAll
then restore c.Request.Body) and if Content-Type indicates multipart/form-data
or application/json, parse the body and look for image-specific fields (e.g.,
keys like "image", "image_url", "image_base64", "image_bytes",
"mimeType"/"mime_type") or a binary/multipart part; only return true if such
image markers are present. Ensure the body is restored so downstream handlers
still receive it.
In `@model/channel_cache.go`:
- Around line 276-282: The DB-backed weighted selection loop in
model/channel_cache.go uses "weight <= 0" while the memory-cached path uses
"randomWeight < 0", causing an off-by-one mismatch; update the DB path loop (the
variable weight and the check inside the for over targetAbilities that returns
channels[ability.ChannelId]) to use the same boundary check as the cached path
(use "weight < 0" instead of "weight <= 0") so both selection paths behave
identically for ability.Weight and weightSum calculations.
In `@model/option.go`:
- Around line 600-603: The call to business_fallback.UpdateConfig(value)
currently swallows its error (returns false) so updateOptionMap never sees the
failure; change handling so the UpdateConfig error is propagated instead of
ignored: either change handleConfigUpdate to return an error and propagate it
up, or (simpler) add an explicit case in updateOptionMap that checks for
configName == "business_fallback" and configKey == "config", calls
business_fallback.UpdateConfig(value), and returns that error (or wraps it) so
failures don’t get silently dropped; locate business_fallback.UpdateConfig and
updateOptionMap to implement this error propagation.
In `@relay/channel/gemini/relay-gemini.go`:
- Around line 1724-1727: The current branch that handles
len(openAIResponse.Data) == 0 returns nil for usage, causing inconsistency with
the other no-candidates branch which returns &usage with metadata-derived
tokens; update this branch in relay-gemini.go to build the same usage object
(populate tokens from metadata as done around the earlier no-candidates
handling) and return &usage along with the types.NewOpenAIError call instead of
nil, while still setting the ContextKeyAdminRejectReason
("gemini_no_inline_images") via common.SetContextKey.
In `@setting/business_fallback/config.go`:
- Around line 130-176: validateConfig currently allows overlapping MatchModels
across ig.Families which makes MatchImageBusinessFallbackFamily
non-deterministic; update validateConfig to detect and reject ambiguous matchers
by ensuring deterministic matching: for each family in ig.Families, compare each
MatchModels pattern against all other families' patterns and error when patterns
are identical or one is a prefix pattern that would match the other's exact or
prefix (e.g., "gpt-image-2" vs "gpt-image-2*" or identical patterns), returning
a clear fmt.Errorf referencing the conflicting family ids and patterns;
implement this logic inside validateConfig (using ig.Families and MatchModels)
so ambiguous configurations are rejected at validation time.
In
`@web/default/src/features/system-settings/business-fallback/config-section.tsx`:
- Around line 80-99: The schema's validation messages in the config field
(inside schema / config: z.string().superRefine) are hard-coded English; update
this to use translated strings from useTranslation() by creating the schema
inside the React component (or a factory) so you can call t(...) for the
validation messages (e.g., replace 'JSON must be an object' and 'Invalid JSON
data' / error.message fallback with t(...) keys) and likewise replace the
hard-coded saveLabel with t('Save Changes'); ensure you reference the same i18n
keys used elsewhere and pass t into any schema factory so zod issues use
localized messages.
- Around line 137-152: The submit handler onSubmit currently returns early when
normalized === initialNormalizedRef.current which leaves the form dirty; change
the early-return path to call form.reset({ config:
formatJsonForEditor(normalized, DEFAULT_BUSINESS_FALLBACK_CONFIG) }) (and keep
initialNormalizedRef.current as-is) before returning so the textarea is
normalized/formatted and the dirty state is cleared; locate this logic around
onSubmit, normalizeJsonString, initialNormalizedRef.current and form.reset in
config-section.tsx and apply the change.
---
Nitpick comments:
In `@controller/relay.go`:
- Around line 385-389: The code currently restores context keys via a defer
(common.SetContextKey, service.SetBusinessFallbackFamily,
service.SetBusinessFallbackActive) but restores relayInfo fields
(relayInfo.OriginModelName, relayInfo.Request, relayInfo.RelayFormat, etc.) only
on the failure/loop-completion path; on success those relayInfo fields remain
modified—either move the relayInfo restoration into the same defer so original
values are captured and always restored, or add an explicit comment by the
relayInfo modifications documenting that leaving them modified on success is
intentional; if you choose defer, capture original values of relayInfo.* before
mutation and restore them in the defer to guarantee consistent state for
callers.
In `@model/channel_cache.go`:
- Around line 231-244: The loop over abilities causes N+1 queries because
DB.First is called per ability; instead, before the loop collect all unique
ability.ChannelId values that are missing from the channels map, perform a
single batch load using DB.Where("id IN ?", ids).Find(&loadedChannels) (mapping
results to Channel objects), populate the channels[ChannelId] entries from that
batch, and then run the existing filtering/append logic (using
filteredAbilities, filter, Channel type) without any per-ability DB calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5db93c67-b290-4670-903f-e738a26a9562
📒 Files selected for processing (25)
common/model.gocontroller/relay.gomiddleware/distributor.gomodel/channel_cache.gomodel/option.gorelay/channel/gemini/adaptor.gorelay/channel/gemini/adaptor_image_test.gorelay/channel/gemini/relay-gemini.gorelay/channel/volcengine/constants.gorelay/image_handler.goservice/business_fallback.goservice/business_fallback_test.goservice/channel_select.gosetting/business_fallback/config.gosetting/business_fallback/config_test.goweb/default/src/components/layout/config/system-settings.config.tsweb/default/src/features/system-settings/business-fallback/config-section.tsxweb/default/src/features/system-settings/business-fallback/index.tsxweb/default/src/features/system-settings/business-fallback/section-registry.tsxweb/default/src/features/system-settings/types.tsweb/default/src/features/usage-logs/components/usage-logs-mobile-card.tsxweb/default/src/i18n/static-keys.tsweb/default/src/routeTree.gen.tsweb/default/src/routes/_authenticated/system-settings/business-fallback/$section.tsxweb/default/src/routes/_authenticated/system-settings/business-fallback/index.tsx
Comment on lines
+556
to
+565
| parts := make([]dto.GeminiPart, 0, len(imageResponse.Data)+1) | ||
| for _, image := range imageResponse.Data { | ||
| if image.B64Json != "" { | ||
| parts = append(parts, dto.GeminiPart{ | ||
| InlineData: &dto.GeminiInlineData{ | ||
| MimeType: "image/png", | ||
| Data: image.B64Json, | ||
| }, | ||
| }) | ||
| continue |
Contributor
There was a problem hiding this comment.
Hardcoded MIME type may be incorrect for non-PNG images.
The code hardcodes MimeType: "image/png" but the upstream image could be JPEG, WebP, or another format. This could cause client-side rendering issues.
Consider detecting the MIME type from the base64 data prefix or the upstream response headers.
Proposed fix
+func detectImageMimeType(b64Data string) string {
+ // Common base64 prefixes for image formats
+ switch {
+ case strings.HasPrefix(b64Data, "/9j/"):
+ return "image/jpeg"
+ case strings.HasPrefix(b64Data, "iVBORw0KGgo"):
+ return "image/png"
+ case strings.HasPrefix(b64Data, "UklGR"):
+ return "image/webp"
+ case strings.HasPrefix(b64Data, "R0lGOD"):
+ return "image/gif"
+ default:
+ return "image/png" // fallback
+ }
+}
+
func writeBusinessFallbackGeminiImageResponse(c *gin.Context, capture *businessFallbackCaptureWriter) *types.NewAPIError {
// ... existing code ...
for _, image := range imageResponse.Data {
if image.B64Json != "" {
parts = append(parts, dto.GeminiPart{
InlineData: &dto.GeminiInlineData{
- MimeType: "image/png",
+ MimeType: detectImageMimeType(image.B64Json),
Data: image.B64Json,
},
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| parts := make([]dto.GeminiPart, 0, len(imageResponse.Data)+1) | |
| for _, image := range imageResponse.Data { | |
| if image.B64Json != "" { | |
| parts = append(parts, dto.GeminiPart{ | |
| InlineData: &dto.GeminiInlineData{ | |
| MimeType: "image/png", | |
| Data: image.B64Json, | |
| }, | |
| }) | |
| continue | |
| func detectImageMimeType(b64Data string) string { | |
| // Common base64 prefixes for image formats | |
| switch { | |
| case strings.HasPrefix(b64Data, "/9j/"): | |
| return "image/jpeg" | |
| case strings.HasPrefix(b64Data, "iVBORw0KGgo"): | |
| return "image/png" | |
| case strings.HasPrefix(b64Data, "UklGR"): | |
| return "image/webp" | |
| case strings.HasPrefix(b64Data, "R0lGOD"): | |
| return "image/gif" | |
| default: | |
| return "image/png" // fallback | |
| } | |
| } | |
| func writeBusinessFallbackGeminiImageResponse(c *gin.Context, capture *businessFallbackCaptureWriter) *types.NewAPIError { | |
| // ... existing code ... | |
| parts := make([]dto.GeminiPart, 0, len(imageResponse.Data)+1) | |
| for _, image := range imageResponse.Data { | |
| if image.B64Json != "" { | |
| parts = append(parts, dto.GeminiPart{ | |
| InlineData: &dto.GeminiInlineData{ | |
| MimeType: detectImageMimeType(image.B64Json), | |
| Data: image.B64Json, | |
| }, | |
| }) | |
| continue |
🤖 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 `@controller/relay.go` around lines 556 - 565, The code creates dto.GeminiPart
with a hardcoded MimeType "image/png" for each image in imageResponse.Data when
image.B64Json is present; update the logic in the block that builds parts (where
dto.GeminiPart and dto.GeminiInlineData are used) to detect the correct MIME
type instead of hardcoding: if image.B64Json contains a data URI prefix (e.g.,
"data:image/...;base64,"), parse the MIME type from that prefix; otherwise,
prefer any content-type/format field available on the upstream image item (e.g.,
image.ContentType or image.Type) and fall back to "application/octet-stream"
only if nothing is available; set dto.GeminiInlineData.MimeType to the detected
value. Ensure the detection handles common types like image/png, image/jpeg,
image/webp and strips the base64 prefix before assigning Data.
Comment on lines
+185
to
+199
| func shouldUseBusinessImageFallbackSelection(c *gin.Context) bool { | ||
| if c == nil || c.Request == nil || c.Request.URL == nil { | ||
| return false | ||
| } | ||
| path := c.Request.URL.Path | ||
| if strings.HasPrefix(path, "/v1/images/generations") { | ||
| return true | ||
| } | ||
| if (strings.HasPrefix(path, "/v1beta/models/") || strings.HasPrefix(path, "/v1/models/")) && | ||
| strings.Contains(path, ":generateContent") && | ||
| !strings.Contains(path, ":streamGenerateContent") && | ||
| !strings.Contains(path, ":embedContent") { | ||
| return true | ||
| } | ||
| return false |
Contributor
There was a problem hiding this comment.
Narrow Gemini fallback opt-in to actual image requests.
This returns true for every ...:generateContent Gemini call, so plain text requests to gemini-3.1-flash-image-preview will also enter the business image fallback selection path. That widens the feature beyond the PR contract of “native image requests” and can reroute non-image traffic through image-only fallback chains. Please gate this on request-body image markers instead of URL shape alone.
🤖 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 `@middleware/distributor.go` around lines 185 - 199, The current
shouldUseBusinessImageFallbackSelection function incorrectly treats any Gemini
:generateContent URL as an image request; change it to also inspect the request
body for true image indicators before returning true. In
shouldUseBusinessImageFallbackSelection, after the existing URL checks, read and
preserve the request body (use c.GetRawData or io.ReadAll then restore
c.Request.Body) and if Content-Type indicates multipart/form-data or
application/json, parse the body and look for image-specific fields (e.g., keys
like "image", "image_url", "image_base64", "image_bytes",
"mimeType"/"mime_type") or a binary/multipart part; only return true if such
image markers are present. Ensure the body is restored so downstream handlers
still receive it.
Comment on lines
+276
to
+282
| weight := common.GetRandomInt(int(weightSum)) | ||
| for _, ability := range targetAbilities { | ||
| weight -= int(ability.Weight) + 10 | ||
| if weight <= 0 { | ||
| return channels[ability.ChannelId], nil | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent weighted random selection boundary check.
The DB-backed path uses weight <= 0 (line 279) while the memory-cached path uses randomWeight < 0 (line 203). This off-by-one inconsistency can cause subtly different selection distributions between cached and non-cached modes.
Proposed fix
weight := common.GetRandomInt(int(weightSum))
for _, ability := range targetAbilities {
weight -= int(ability.Weight) + 10
- if weight <= 0 {
+ if weight < 0 {
return channels[ability.ChannelId], nil
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| weight := common.GetRandomInt(int(weightSum)) | |
| for _, ability := range targetAbilities { | |
| weight -= int(ability.Weight) + 10 | |
| if weight <= 0 { | |
| return channels[ability.ChannelId], nil | |
| } | |
| } | |
| weight := common.GetRandomInt(int(weightSum)) | |
| for _, ability := range targetAbilities { | |
| weight -= int(ability.Weight) + 10 | |
| if weight < 0 { | |
| return channels[ability.ChannelId], 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 `@model/channel_cache.go` around lines 276 - 282, The DB-backed weighted
selection loop in model/channel_cache.go uses "weight <= 0" while the
memory-cached path uses "randomWeight < 0", causing an off-by-one mismatch;
update the DB path loop (the variable weight and the check inside the for over
targetAbilities that returns channels[ability.ChannelId]) to use the same
boundary check as the cached path (use "weight < 0" instead of "weight <= 0") so
both selection paths behave identically for ability.Weight and weightSum
calculations.
Comment on lines
+600
to
+603
| if configName == "business_fallback" && configKey == "config" { | ||
| err := business_fallback.UpdateConfig(value) | ||
| return err == nil | ||
| } |
Contributor
There was a problem hiding this comment.
Error from business_fallback.UpdateConfig is silently swallowed.
When UpdateConfig fails, the function returns false, causing updateOptionMap to continue processing. Since business_fallback.config won't match any case in the switch statement, no error is returned. This could leave the in-memory configuration out of sync with the database.
Proposed fix
The handleConfigUpdate function signature would need to return an error, or handle this case specially. A simpler fix is to add the case to the switch statement in updateOptionMap:
func handleConfigUpdate(key, value string) bool {
parts := strings.SplitN(key, ".", 2)
if len(parts) != 2 {
return false // 不是分层配置
}
configName := parts[0]
configKey := parts[1]
- if configName == "business_fallback" && configKey == "config" {
- err := business_fallback.UpdateConfig(value)
- return err == nil
- }
+ if configName == "business_fallback" {
+ return false // Let switch statement handle it
+ }
// ... rest of functionThen add to updateOptionMap switch:
switch key {
+case "business_fallback.config":
+ err = business_fallback.UpdateConfig(value)
case "EmailDomainWhitelist":🤖 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 `@model/option.go` around lines 600 - 603, The call to
business_fallback.UpdateConfig(value) currently swallows its error (returns
false) so updateOptionMap never sees the failure; change handling so the
UpdateConfig error is propagated instead of ignored: either change
handleConfigUpdate to return an error and propagate it up, or (simpler) add an
explicit case in updateOptionMap that checks for configName ==
"business_fallback" and configKey == "config", calls
business_fallback.UpdateConfig(value), and returns that error (or wraps it) so
failures don’t get silently dropped; locate business_fallback.UpdateConfig and
updateOptionMap to implement this error propagation.
Comment on lines
+1724
to
+1727
| if len(openAIResponse.Data) == 0 { | ||
| common.SetContextKey(c, constant.ContextKeyAdminRejectReason, "gemini_no_inline_images") | ||
| return nil, types.NewOpenAIError(errors.New("no images generated"), types.ErrorCodeEmptyResponse, http.StatusInternalServerError) | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent nil usage return.
When there are candidates but no extractable inline images, this returns nil for usage. However, at lines 1690-1702, when there are no candidates, &usage is returned with metadata-derived tokens. For consistency and proper billing accounting, usage should also be returned here.
🔧 Proposed fix
if len(openAIResponse.Data) == 0 {
+ usage := buildUsageFromGeminiMetadata(geminiResponse.UsageMetadata, info.GetEstimatePromptTokens())
common.SetContextKey(c, constant.ContextKeyAdminRejectReason, "gemini_no_inline_images")
- return nil, types.NewOpenAIError(errors.New("no images generated"), types.ErrorCodeEmptyResponse, http.StatusInternalServerError)
+ return &usage, types.NewOpenAIError(errors.New("no images generated"), types.ErrorCodeEmptyResponse, http.StatusInternalServerError)
}🤖 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 1724 - 1727, The current
branch that handles len(openAIResponse.Data) == 0 returns nil for usage, causing
inconsistency with the other no-candidates branch which returns &usage with
metadata-derived tokens; update this branch in relay-gemini.go to build the same
usage object (populate tokens from metadata as done around the earlier
no-candidates handling) and return &usage along with the types.NewOpenAIError
call instead of nil, while still setting the ContextKeyAdminRejectReason
("gemini_no_inline_images") via common.SetContextKey.
Comment on lines
+130
to
+176
| func validateConfig(cfg Config) error { | ||
| ig := cfg.ImageGeneration | ||
| if len(ig.Families) == 0 { | ||
| return errors.New("image_generation.families is required") | ||
| } | ||
| if len(ig.Chains) == 0 { | ||
| return errors.New("image_generation.chains is required") | ||
| } | ||
| for id, family := range ig.Families { | ||
| id = strings.TrimSpace(id) | ||
| if id == "" { | ||
| return errors.New("image_generation.families contains empty family id") | ||
| } | ||
| if strings.TrimSpace(family.SelectModel) == "" { | ||
| return fmt.Errorf("image_generation.families.%s.select_model is required", id) | ||
| } | ||
| if len(family.MatchModels) == 0 { | ||
| return fmt.Errorf("image_generation.families.%s.match_models is required", id) | ||
| } | ||
| for _, model := range family.MatchModels { | ||
| if strings.TrimSpace(model) == "" { | ||
| return fmt.Errorf("image_generation.families.%s.match_models contains empty model", id) | ||
| } | ||
| } | ||
| } | ||
| for family, chain := range ig.Chains { | ||
| if _, ok := ig.Families[family]; !ok { | ||
| return fmt.Errorf("image_generation.chains.%s references unknown source family", family) | ||
| } | ||
| if len(chain) == 0 { | ||
| return fmt.Errorf("image_generation.chains.%s must contain at least one target family", family) | ||
| } | ||
| for _, target := range chain { | ||
| if _, ok := ig.Families[target]; !ok { | ||
| return fmt.Errorf("image_generation.chains.%s references unknown target family %s", family, target) | ||
| } | ||
| } | ||
| } | ||
| for _, family := range ig.Health.MonitoredFamilies { | ||
| if _, ok := ig.Families[family]; !ok { | ||
| return fmt.Errorf("image_generation.health.monitored_families references unknown family %s", family) | ||
| } | ||
| } | ||
| if ig.Health.SuccessRateThreshold < 0 || ig.Health.SuccessRateThreshold > 1 { | ||
| return errors.New("image_generation.health.success_rate_threshold must be between 0 and 1") | ||
| } | ||
| return nil |
Contributor
There was a problem hiding this comment.
Reject or order overlapping match_models definitions.
families is a map, and MatchImageBusinessFallbackFamily() later stops at the first matching family it sees. If an admin config introduces overlapping patterns like prefix:gpt-image-2 and gpt-image-2*, the selected fallback chain becomes map-iteration dependent and can differ across processes. Please make this deterministic here by rejecting ambiguous matchers during validation or by switching to an ordered config shape for family matching.
🤖 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 `@setting/business_fallback/config.go` around lines 130 - 176, validateConfig
currently allows overlapping MatchModels across ig.Families which makes
MatchImageBusinessFallbackFamily non-deterministic; update validateConfig to
detect and reject ambiguous matchers by ensuring deterministic matching: for
each family in ig.Families, compare each MatchModels pattern against all other
families' patterns and error when patterns are identical or one is a prefix
pattern that would match the other's exact or prefix (e.g., "gpt-image-2" vs
"gpt-image-2*" or identical patterns), returning a clear fmt.Errorf referencing
the conflicting family ids and patterns; implement this logic inside
validateConfig (using ig.Families and MatchModels) so ambiguous configurations
are rejected at validation time.
Comment on lines
+80
to
+99
| const schema = z.object({ | ||
| config: z.string().superRefine((value, ctx) => { | ||
| try { | ||
| const parsed = JSON.parse( | ||
| normalizeJsonString(value, DEFAULT_BUSINESS_FALLBACK_CONFIG) | ||
| ) | ||
| if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: 'JSON must be an object', | ||
| }) | ||
| } | ||
| } catch (error: unknown) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: | ||
| (error instanceof Error ? error.message : null) || | ||
| 'Invalid JSON data', | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Localize the remaining editor strings.
The validation messages and saveLabel are still hard-coded English, so this page will stay partially untranslated even though it already uses useTranslation(). Build the schema with translated messages and pass t('Save Changes') here as well.
As per coding guidelines, "All user-facing text content must support i18n using the t() function from useTranslation() in React components".
Also applies to: 156-163
🤖 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/system-settings/business-fallback/config-section.tsx`
around lines 80 - 99, The schema's validation messages in the config field
(inside schema / config: z.string().superRefine) are hard-coded English; update
this to use translated strings from useTranslation() by creating the schema
inside the React component (or a factory) so you can call t(...) for the
validation messages (e.g., replace 'JSON must be an object' and 'Invalid JSON
data' / error.message fallback with t(...) keys) and likewise replace the
hard-coded saveLabel with t('Save Changes'); ensure you reference the same i18n
keys used elsewhere and pass t into any schema factory so zod issues use
localized messages.
Comment on lines
+137
to
+152
| const onSubmit = async (values: FormValues) => { | ||
| const normalized = normalizeJsonString( | ||
| values.config, | ||
| DEFAULT_BUSINESS_FALLBACK_CONFIG | ||
| ) | ||
| if (normalized === initialNormalizedRef.current) { | ||
| return | ||
| } | ||
| await updateOption.mutateAsync({ | ||
| key: 'business_fallback.config', | ||
| value: normalized, | ||
| }) | ||
| initialNormalizedRef.current = normalized | ||
| form.reset({ | ||
| config: formatJsonForEditor(normalized, DEFAULT_BUSINESS_FALLBACK_CONFIG), | ||
| }) |
Contributor
There was a problem hiding this comment.
Clear dirty state after whitespace-only saves.
When normalized === initialNormalizedRef.current, the submit path returns before form.reset(). That leaves the textarea dirty and the save action enabled even though there is nothing to persist.
Proposed fix
const onSubmit = async (values: FormValues) => {
const normalized = normalizeJsonString(
values.config,
DEFAULT_BUSINESS_FALLBACK_CONFIG
)
if (normalized === initialNormalizedRef.current) {
+ form.reset({
+ config: formatJsonForEditor(
+ normalized,
+ DEFAULT_BUSINESS_FALLBACK_CONFIG
+ ),
+ })
return
}
await updateOption.mutateAsync({
key: 'business_fallback.config',
value: normalized,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const onSubmit = async (values: FormValues) => { | |
| const normalized = normalizeJsonString( | |
| values.config, | |
| DEFAULT_BUSINESS_FALLBACK_CONFIG | |
| ) | |
| if (normalized === initialNormalizedRef.current) { | |
| return | |
| } | |
| await updateOption.mutateAsync({ | |
| key: 'business_fallback.config', | |
| value: normalized, | |
| }) | |
| initialNormalizedRef.current = normalized | |
| form.reset({ | |
| config: formatJsonForEditor(normalized, DEFAULT_BUSINESS_FALLBACK_CONFIG), | |
| }) | |
| const onSubmit = async (values: FormValues) => { | |
| const normalized = normalizeJsonString( | |
| values.config, | |
| DEFAULT_BUSINESS_FALLBACK_CONFIG | |
| ) | |
| if (normalized === initialNormalizedRef.current) { | |
| form.reset({ | |
| config: formatJsonForEditor( | |
| normalized, | |
| DEFAULT_BUSINESS_FALLBACK_CONFIG | |
| ), | |
| }) | |
| return | |
| } | |
| await updateOption.mutateAsync({ | |
| key: 'business_fallback.config', | |
| value: normalized, | |
| }) | |
| initialNormalizedRef.current = normalized | |
| form.reset({ | |
| config: formatJsonForEditor(normalized, DEFAULT_BUSINESS_FALLBACK_CONFIG), | |
| }) |
🤖 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/system-settings/business-fallback/config-section.tsx`
around lines 137 - 152, The submit handler onSubmit currently returns early when
normalized === initialNormalizedRef.current which leaves the form dirty; change
the early-return path to call form.reset({ config:
formatJsonForEditor(normalized, DEFAULT_BUSINESS_FALLBACK_CONFIG) }) (and keep
initialNormalizedRef.current as-is) before returning so the textarea is
normalized/formatted and the dirty state is cleared; locate this logic around
onSubmit, normalizeJsonString, initialNormalizedRef.current and form.reset in
config-section.tsx and apply the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
business_fallbackdomain persisted under option keybusiness_fallback.config.gpt-image-2,gemini-3.1-flash-image-preview, anddoubao-seedream-5-0/doubao-seedream-5-0*.channel_id + model_familyhealth fuse logic with Redis minute buckets and in-memory fallback;seedreamremains final fallback only and is not health-monitored.Business Fallbacksystem settings page.Safety Notes
Validation
PATH=/usr/local/go1.26/bin:$PATH GOPROXY=https://goproxy.cn,direct go test ./setting/business_fallback ./service ./relay/channel/gemini ./relay/channel/volcengine ./model ./middleware ./relaybun run typecheckinweb/defaultbun run buildinweb/defaultgit diff --check HEAD~1..HEADEarlier broader Go ranges still showed unrelated existing failures in Claude file conversion tests, relay helper stream scanner tests, and controller SQLite/model-list tests.
Summary by CodeRabbit
Release Notes