From fc589bce17b7a481cddb63136e024b83014bf505 Mon Sep 17 00:00:00 2001 From: Long Li Date: Sun, 23 Aug 2026 15:43:57 +0900 Subject: [PATCH] fix(codex): address routed catalog review feedback --- .../openai_codex_models_handler_test.go | 18 +++- .../service/openai_codex_model_metadata.go | 42 ++++++-- .../openai_codex_model_metadata_test.go | 101 ++++++++++++++++++ .../service/openai_codex_models_service.go | 66 +++++++++--- .../openai_codex_models_service_test.go | 75 ++++++++++--- 5 files changed, 267 insertions(+), 35 deletions(-) diff --git a/backend/internal/handler/openai_codex_models_handler_test.go b/backend/internal/handler/openai_codex_models_handler_test.go index ea6537ae33..e47852409b 100644 --- a/backend/internal/handler/openai_codex_models_handler_test.go +++ b/backend/internal/handler/openai_codex_models_handler_test.go @@ -298,6 +298,23 @@ func TestCodexModelsFailsOverFromRetryableUpstreamStatus(t *testing.T) { } } +// Scenario: an API-key upstream without /models is excluded only for this discovery request. +func TestCodexModelsFailsOverWhenAPIKeyModelsEndpointIsUnavailable(t *testing.T) { + for _, status := range []int{http.StatusNotFound, http.StatusMethodNotAllowed} { + t.Run(http.StatusText(status), func(t *testing.T) { + handler, upstream, groupID := newCodexModelsFailoverTestHandler(status) + recorder := performCodexModelsRequest(t, handler, groupID) + + if got, want := upstream.calls(), []int64{1, 2}; !equalInt64Slices(got, want) { + t.Fatalf("upstream account calls: got %v, want %v", got, want) + } + if recorder.Code != http.StatusOK { + t.Fatalf("status: got %d, want %d; body=%s", recorder.Code, http.StatusOK, recorder.Body.String()) + } + }) + } +} + func TestCodexModelsFailsOverFromUpstreamTransportError(t *testing.T) { handler, upstream, groupID := newCodexModelsFailoverTestHandler(http.StatusServiceUnavailable) upstream.firstErr = &net.OpError{ @@ -334,7 +351,6 @@ func TestCodexModelsDoesNotFailOverFromPermanentUpstreamStatus(t *testing.T) { http.StatusBadRequest, http.StatusUnauthorized, http.StatusForbidden, - http.StatusNotFound, 600, } for _, status := range statuses { diff --git a/backend/internal/service/openai_codex_model_metadata.go b/backend/internal/service/openai_codex_model_metadata.go index 71651b3271..c779a05ab9 100644 --- a/backend/internal/service/openai_codex_model_metadata.go +++ b/backend/internal/service/openai_codex_model_metadata.go @@ -45,13 +45,8 @@ func groupCodexModelMetadata( } } } - if explicitClaims && codexExplicitModelTargetsConflict(accounts, modelID) { - return codexModelMetadataOverride{ - reasoningConflict: true, - inputModalitiesConflict: true, - }, true - } - + explicitTargetsConflict := explicitClaims && codexExplicitModelTargetsConflictForPlatform(accounts, platform, modelID) + publicAlias := upstreamModel != modelID candidates := make([]UpstreamModelMetadata, 0) for i := range accounts { account := &accounts[i] @@ -70,8 +65,17 @@ func groupCodexModelMetadata( } lookupModel = account.GetMappedModel(upstreamModel) } + if strings.TrimSpace(lookupModel) != modelID { + publicAlias = true + } metadata, ok := account.GetUpstreamModelMetadata(lookupModel) if !ok { + if explicitTargetsConflict { + return codexModelMetadataOverride{ + reasoningConflict: true, + inputModalitiesConflict: true, + }, true + } return codexModelMetadataOverride{}, false } candidates = append(candidates, metadata) @@ -79,7 +83,12 @@ func groupCodexModelMetadata( if len(candidates) == 0 { return codexModelMetadataOverride{}, false } - return intersectUpstreamModelMetadata(modelID, candidates), true + metadata := intersectUpstreamModelMetadata(modelID, candidates) + if publicAlias { + metadata.DisplayName = modelID + metadata.Description = configuredCodexCustomDescription + } + return metadata, true } func codexExplicitModelTargetsConflict(accounts []Account, modelID string) bool { @@ -96,6 +105,23 @@ func codexExplicitModelTargetsConflict(accounts []Account, modelID string) bool return len(targets) > 1 } +func codexExplicitModelTargetsConflictForPlatform(accounts []Account, platform, modelID string) bool { + targets := make(map[string]struct{}) + for i := range accounts { + account := &accounts[i] + if account.Platform != platform { + continue + } + mappedModel, matched := account.ResolveMappedModel(modelID) + mappedModel = strings.TrimSpace(mappedModel) + if !matched || mappedModel == "" { + continue + } + targets[mappedModel] = struct{}{} + } + return len(targets) > 1 +} + func intersectUpstreamModelMetadata(modelID string, candidates []UpstreamModelMetadata) codexModelMetadataOverride { result := codexModelMetadataOverride{UpstreamModelMetadata: UpstreamModelMetadata{ID: strings.TrimSpace(modelID)}} for _, candidate := range candidates { diff --git a/backend/internal/service/openai_codex_model_metadata_test.go b/backend/internal/service/openai_codex_model_metadata_test.go index 4baa47c10c..eac3a59ab8 100644 --- a/backend/internal/service/openai_codex_model_metadata_test.go +++ b/backend/internal/service/openai_codex_model_metadata_test.go @@ -135,6 +135,107 @@ func TestBuildCodexModelsManifestForGroupIntersectsSyncedAccountMetadata(t *test require.EqualValues(t, 128_000, models[0]["context_window"]) } +// Scenario: the same public alias may target different models on one platform when complete snapshots can be intersected. +func TestBuildCodexModelsManifestForGroupIntersectsDifferentMappedTargetsWithoutLeakingAlias(t *testing.T) { + t.Parallel() + + const groupID int64 = 739 + reasoning := true + newAccount := func(id int64, target, displayName, description string, levels, modalities []string, contextWindow int64) Account { + account := Account{ + ID: id, Platform: PlatformOpenAI, Type: AccountTypeAPIKey, + Credentials: map[string]any{ + "base_url": "https://provider.example/v1", + "model_mapping": map[string]any{"my-coder": target}, + }, + } + account.SetUpstreamModelMetadataSnapshot(UpstreamModelMetadataSnapshot{Models: map[string]UpstreamModelMetadata{ + target: { + ID: target, DisplayName: displayName, Description: description, Reasoning: &reasoning, + SupportedReasoningLevels: levels, + InputModalities: modalities, + ContextWindow: contextWindow, + }, + }}) + return account + } + openAIAccount := newAccount( + 31, + "gpt-5.6-sol", + "GPT-5.6 Sol", + "OpenAI upstream model", + []string{"low", "medium", "high", "xhigh"}, + []string{"text", "image"}, + 272_000, + ) + arkAccount := newAccount( + 32, + "glm-5.3", + "GLM 5.3", + "Ark upstream model", + []string{"low", "medium", "high"}, + []string{"text"}, + 1_000_000, + ) + + for _, accounts := range [][]Account{{openAIAccount, arkAccount}, {arkAccount, openAIAccount}} { + svc := &GatewayService{accountRepo: codexModelsVisibilityAccountRepo{byGroup: map[int64][]Account{ + groupID: accounts, + }}} + body, err := svc.BuildCodexModelsManifestForGroup( + context.Background(), &Group{ID: groupID, Platform: PlatformOpenAI}, "", []string{"my-coder"}, + ) + require.NoError(t, err) + models := decodeCodexManifestModels(t, body) + require.Len(t, models, 1) + require.Equal(t, "my-coder", models[0]["slug"]) + require.Equal(t, "my-coder", models[0]["display_name"]) + require.Equal(t, "Custom model routed through Sub2API.", models[0]["description"]) + require.Equal(t, []string{"low", "medium", "high"}, effortsFromManifestModel(t, models[0])) + require.Equal(t, []any{"text"}, models[0]["input_modalities"]) + require.EqualValues(t, 272_000, models[0]["context_window"]) + } +} + +// Scenario: a Composite alias claimed across platforms remains ambiguous and fails closed. +func TestBuildCodexModelsManifestForGroupKeepsCrossPlatformAliasAmbiguityClosed(t *testing.T) { + t.Parallel() + + const groupID int64 = 740 + reasoning := true + newAccount := func(id int64, platform, target string) Account { + account := Account{ + ID: id, Platform: platform, Type: AccountTypeAPIKey, + Credentials: map[string]any{"model_mapping": map[string]any{"shared-alias": target}}, + } + account.SetUpstreamModelMetadataSnapshot(UpstreamModelMetadataSnapshot{Models: map[string]UpstreamModelMetadata{ + target: { + ID: target, DisplayName: target, Reasoning: &reasoning, + SupportedReasoningLevels: []string{"low", "high"}, + InputModalities: []string{"text", "image"}, + ContextWindow: 128_000, + }, + }}) + return account + } + svc := &GatewayService{accountRepo: codexModelsVisibilityAccountRepo{byGroup: map[int64][]Account{ + groupID: { + newAccount(33, PlatformOpenAI, "gpt-5.6-sol"), + newAccount(34, PlatformGrok, "grok-4.6"), + }, + }}} + + body, err := svc.BuildCodexModelsManifestForGroup( + context.Background(), &Group{ID: groupID, Platform: PlatformComposite}, "", []string{"shared-alias"}, + ) + require.NoError(t, err) + models := decodeCodexManifestModels(t, body) + require.Len(t, models, 1) + require.Equal(t, "shared-alias", models[0]["display_name"]) + require.Empty(t, effortsFromManifestModel(t, models[0])) + require.Equal(t, []any{"text"}, models[0]["input_modalities"]) +} + func TestBuildCodexModelsManifestForGroupDoesNotAdvertiseNoneWhenAccountReasoningConflicts(t *testing.T) { t.Parallel() diff --git a/backend/internal/service/openai_codex_models_service.go b/backend/internal/service/openai_codex_models_service.go index e0715889fa..cb58591aad 100644 --- a/backend/internal/service/openai_codex_models_service.go +++ b/backend/internal/service/openai_codex_models_service.go @@ -109,21 +109,28 @@ func (s *OpenAIGatewayService) BuildGroupConfiguredCodexModelsManifest( return nil, false, nil } - configuredModels, err := s.groupConfiguredCodexModelIDs(ctx, group.ID) + accounts, err := s.accountRepo.ListSchedulableByGroupID(ctx, group.ID) if err != nil { return nil, false, fmt.Errorf("load group configured Codex models: %w", err) } + configuredModels := openAIConfiguredCodexModelIDs(accounts) if len(configuredModels) == 0 { return nil, false, nil } - body, err := BuildCodexModelsManifest(nil) + body, err := buildCodexModelsManifestForAccounts( + PlatformOpenAI, + configuredModels, + accounts, + nil, + true, + ) if err != nil { return nil, false, fmt.Errorf("initialize group configured Codex models: %w", err) } body, _, err = mergeConfiguredCodexModelsManifest( body, - configuredModels, + nil, group.ModelsListConfig.Models, group.CustomModelsListEnabled(), ) @@ -187,7 +194,10 @@ func (s *OpenAIGatewayService) groupConfiguredCodexModelIDs(ctx context.Context, if err != nil { return nil, err } + return openAIConfiguredCodexModelIDs(accounts), nil +} +func openAIConfiguredCodexModelIDs(accounts []Account) []string { seen := make(map[string]struct{}) models := make([]string, 0) for i := range accounts { @@ -208,11 +218,12 @@ func (s *OpenAIGatewayService) groupConfiguredCodexModelIDs(ctx context.Context, } } sort.Strings(models) - return models, nil + return models } const ( configuredCodexModelPriority = 50 + configuredCodexCustomDescription = "Custom model routed through Sub2API." configuredCodexFallbackContext = 272_000 configuredCodexDeepSeekV4Context = 1_000_000 configuredCodexGrokContext = 500_000 @@ -304,7 +315,7 @@ func newConfiguredCodexModelDescriptor(modelID string) configuredCodexModelDescr descriptor := configuredCodexModelDescriptor{ Slug: modelID, DisplayName: modelID, - Description: "Custom model routed through Sub2API.", + Description: configuredCodexCustomDescription, DefaultReasoningLevel: &noReasoningLevel, SupportedReasoningLevels: []configuredCodexReasoningLevel{ {Effort: "none", Description: configuredCodexReasoningLevelDescription("none")}, @@ -652,6 +663,22 @@ func (s *GatewayService) BuildCodexModelsManifestForGroup( compositeRoutesAvailable = false } } + return buildCodexModelsManifestForAccounts( + effectivePlatform, + modelIDs, + accounts, + compositeRoutes, + compositeRoutesAvailable, + ) +} + +func buildCodexModelsManifestForAccounts( + effectivePlatform string, + modelIDs []string, + accounts []Account, + compositeRoutes []CompositeModelRoute, + compositeRoutesAvailable bool, +) ([]byte, error) { imageInputModels := make(map[string]bool, len(modelIDs)) metadataModels := codexCatalogMetadataModels( effectivePlatform, @@ -717,6 +744,10 @@ func buildCodexModelsManifest( if metadata, ok := modelMetadata[modelID]; ok { applyUpstreamModelMetadataToCodexDescriptor(&descriptor, metadata) } + if metadataModelID != modelID { + descriptor.DisplayName = modelID + descriptor.Description = configuredCodexCustomDescription + } models = append(models, descriptor) } return json.Marshal(struct { @@ -1135,12 +1166,13 @@ func (e *codexModelsManifestUpstreamError) Error() string { return e.err.Error() func (e *codexModelsManifestUpstreamError) Unwrap() error { return e.err } // IsRetryableCodexModelsManifestError reports whether another selected account -// may succeed without changing the request. Configuration and upstream 4xx -// responses, except 429 and ChatGPT-backend 401, are intentionally not -// retried. A manifest 401 from the ChatGPT Codex backend reflects the selected -// OAuth account's upstream token rather than the client request (the client's -// own API key was already validated locally), so a different account may still -// serve the manifest. Custom API key upstreams keep the old no-failover 401 +// may succeed without changing the request. API key upstream 404/405 responses +// mean that the selected account does not expose a model-discovery endpoint, so +// another account may still serve the manifest. Other upstream 4xx responses, +// except 429 and ChatGPT-backend 401, are intentionally not retried. A manifest +// 401 from the ChatGPT Codex backend reflects the selected OAuth account's +// upstream token rather than the client request (the client's own API key was +// already validated locally). Custom API key upstreams keep the no-failover 401 // behavior because their /models auth semantics are not authoritative for the // account. func IsRetryableCodexModelsManifestError(err error) bool { @@ -1148,6 +1180,14 @@ func IsRetryableCodexModelsManifestError(err error) bool { return errors.As(err, &upstreamErr) && upstreamErr.retryable } +func isRetryableCodexModelsManifestStatus(statusCode int, useAPIKeyUpstream bool) bool { + return (useAPIKeyUpstream && + (statusCode == http.StatusNotFound || statusCode == http.StatusMethodNotAllowed)) || + (statusCode == http.StatusUnauthorized && !useAPIKeyUpstream) || + statusCode == http.StatusTooManyRequests || + (statusCode >= http.StatusInternalServerError && statusCode < 600) +} + func isRetryableCodexModelsManifestTransportError(err error) bool { if err == nil || errors.Is(err, context.Canceled) { return false @@ -1575,9 +1615,7 @@ func (s *OpenAIGatewayService) fetchCodexModelsManifestUpstream(ctx context.Cont statusCode: resp.StatusCode, headers: resp.Header.Clone(), body: body, - retryable: (resp.StatusCode == http.StatusUnauthorized && !request.useAPIKeyUpstream) || - resp.StatusCode == http.StatusTooManyRequests || - (resp.StatusCode >= http.StatusInternalServerError && resp.StatusCode < 600), + retryable: isRetryableCodexModelsManifestStatus(resp.StatusCode, request.useAPIKeyUpstream), } } diff --git a/backend/internal/service/openai_codex_models_service_test.go b/backend/internal/service/openai_codex_models_service_test.go index 99d96f0b6d..4ba6de646e 100644 --- a/backend/internal/service/openai_codex_models_service_test.go +++ b/backend/internal/service/openai_codex_models_service_test.go @@ -623,8 +623,8 @@ func TestBuildCodexModelsManifestForGroupUsesMappedTargetMetadataForCompositeAli models := decodeCodexManifestModels(t, body) require.Len(t, models, 1) require.Equal(t, "reasoning-alias", models[0]["slug"]) - require.Equal(t, "Claude Opus 4.8", models[0]["display_name"]) - require.Equal(t, "Claude coding and reasoning model routed through Sub2API.", models[0]["description"]) + require.Equal(t, "reasoning-alias", models[0]["display_name"]) + require.Equal(t, "Custom model routed through Sub2API.", models[0]["description"]) require.Equal(t, []string{"low", "medium", "high", "xhigh", "max"}, effortsFromManifestModel(t, models[0])) } @@ -816,6 +816,29 @@ func TestBuildGroupConfiguredCodexModelsManifestUsesAdministratorConfiguration(t t.Parallel() const groupID int64 = 77 + reasoning := true + arkAccount := Account{ + Platform: PlatformOpenAI, + Type: AccountTypeAPIKey, + Credentials: map[string]any{ + "model_mapping": map[string]any{ + "glm-5.3": "glm-5.3", + "gpt-image-2": "gpt-image-2", + }, + }, + } + arkAccount.SetUpstreamModelMetadataSnapshot(UpstreamModelMetadataSnapshot{Models: map[string]UpstreamModelMetadata{ + "glm-5.3": { + ID: "glm-5.3", + DisplayName: "GLM 5.3", + Description: "Ark coding model", + Reasoning: &reasoning, + DefaultReasoningLevel: "medium", + SupportedReasoningLevels: []string{"low", "medium", "high"}, + InputModalities: []string{"text"}, + ContextWindow: 1_000_000, + }, + }}) svc := &OpenAIGatewayService{accountRepo: codexModelsVisibilityAccountRepo{ byGroup: map[int64][]Account{ groupID: { @@ -823,15 +846,7 @@ func TestBuildGroupConfiguredCodexModelsManifestUsesAdministratorConfiguration(t Platform: PlatformOpenAI, Type: AccountTypeOAuth, }, - { - Platform: PlatformOpenAI, - Type: AccountTypeAPIKey, - Credentials: map[string]any{ - "model_mapping": map[string]any{ - "glm-5.3": "glm-5.3", - }, - }, - }, + arkAccount, }, }, }} @@ -840,7 +855,14 @@ func TestBuildGroupConfiguredCodexModelsManifestUsesAdministratorConfiguration(t manifest, configured, err := svc.BuildGroupConfiguredCodexModelsManifest(context.Background(), group, "") require.NoError(t, err) require.True(t, configured) - require.Equal(t, []string{"glm-5.3"}, codexManifestModelSlugs(t, manifest.Body)) + models := decodeCodexManifestModels(t, manifest.Body) + require.Len(t, models, 1) + require.Equal(t, "glm-5.3", models[0]["slug"]) + require.Equal(t, "GLM 5.3", models[0]["display_name"]) + require.Equal(t, []string{"low", "medium", "high"}, effortsFromManifestModel(t, models[0])) + require.Equal(t, "medium", models[0]["default_reasoning_level"]) + require.Equal(t, []any{"text"}, models[0]["input_modalities"]) + require.EqualValues(t, 1_000_000, models[0]["context_window"]) require.Equal(t, codexModelsManifestBodyETag(manifest.Body), manifest.ETag) notModified, configured, err := svc.BuildGroupConfiguredCodexModelsManifest( @@ -1089,6 +1111,35 @@ func TestIsRetryableCodexModelsManifestTransportError(t *testing.T) { } } +func TestIsRetryableCodexModelsManifestStatus(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + statusCode int + useAPIKeyUpstream bool + retryable bool + }{ + {name: "api key 404", statusCode: http.StatusNotFound, useAPIKeyUpstream: true, retryable: true}, + {name: "api key 405", statusCode: http.StatusMethodNotAllowed, useAPIKeyUpstream: true, retryable: true}, + {name: "oauth 404", statusCode: http.StatusNotFound}, + {name: "oauth 405", statusCode: http.StatusMethodNotAllowed}, + {name: "api key 401", statusCode: http.StatusUnauthorized, useAPIKeyUpstream: true}, + {name: "oauth 401", statusCode: http.StatusUnauthorized, retryable: true}, + {name: "api key 400", statusCode: http.StatusBadRequest, useAPIKeyUpstream: true}, + {name: "api key 403", statusCode: http.StatusForbidden, useAPIKeyUpstream: true}, + {name: "rate limited", statusCode: http.StatusTooManyRequests, retryable: true}, + {name: "server error", statusCode: http.StatusServiceUnavailable, retryable: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.retryable, isRetryableCodexModelsManifestStatus(tt.statusCode, tt.useAPIKeyUpstream)) + }) + } +} + func newCodexModelsAPIKeyTestService(upstream HTTPUpstream) *OpenAIGatewayService { return &OpenAIGatewayService{ cfg: &config.Config{Security: config.SecurityConfig{URLAllowlist: config.URLAllowlistConfig{