From da240a5e09a4ff54470eaaf244abbef1e0e52fb5 Mon Sep 17 00:00:00 2001 From: viettranx Date: Wed, 1 Apr 2026 19:31:02 +0700 Subject: [PATCH] fix: pool validation false positive + OAuth step advancement + pool routing sync (#583, #594) - Skip disabled providers in pool graph validation to prevent false "member already exists" errors when stale pool configs linger (#583) - Add retry logic (3 attempts, 1s delay) in setup wizard OAuth flow to handle backend commit race condition (#594) - Reset chatgpt_oauth_routing in agent other_config when provider changes, with amber warning in UI (#583) - Add 3 pool validation tests for disabled provider scenarios - Add providerChangedWarning i18n key (en/vi/zh) --- .../http/chatgpt_oauth_pool_validation.go | 5 + .../chatgpt_oauth_pool_validation_test.go | 161 ++++++++++++++++++ ui/web/src/i18n/locales/en/agents.json | 1 + ui/web/src/i18n/locales/vi/agents.json | 1 + ui/web/src/i18n/locales/zh/agents.json | 1 + .../agent-detail/agent-overview-tab.tsx | 12 +- ui/web/src/pages/setup/step-provider.tsx | 13 +- 7 files changed, 191 insertions(+), 3 deletions(-) diff --git a/internal/http/chatgpt_oauth_pool_validation.go b/internal/http/chatgpt_oauth_pool_validation.go index c1d08fb4..94a42265 100644 --- a/internal/http/chatgpt_oauth_pool_validation.go +++ b/internal/http/chatgpt_oauth_pool_validation.go @@ -110,6 +110,11 @@ func validateChatGPTOAuthProviderCandidate( if provider.ProviderType != store.ProviderChatGPTOAuth { continue } + // Skip disabled providers — their stale pool configs should not + // block validation for active providers. + if !provider.Enabled { + continue + } if currentID != uuid.Nil && provider.ID == currentID { finalProviders = append(finalProviders, *candidate) replaced = true diff --git a/internal/http/chatgpt_oauth_pool_validation_test.go b/internal/http/chatgpt_oauth_pool_validation_test.go index ab96f982..b51925b4 100644 --- a/internal/http/chatgpt_oauth_pool_validation_test.go +++ b/internal/http/chatgpt_oauth_pool_validation_test.go @@ -163,3 +163,164 @@ func TestValidateChatGPTOAuthAgentRoutingAllowsStrategyOnlyOverride(t *testing.T t.Fatalf("validateChatGPTOAuthAgentRouting() error = %v, want nil", err) } } + +// TestValidatePoolGraphIgnoresDisabledProviders verifies that disabled providers' +// stale pool configs do not block validation for active providers. +func TestValidatePoolGraphIgnoresDisabledProviders(t *testing.T) { + providerStore := newMockProviderStore() + tenantID := uuid.New() + ctx := store.WithTenantID(context.Background(), tenantID) + + // Disabled provider that previously owned "codex-work" in its pool. + for _, p := range []*store.LLMProviderData{ + { + BaseModel: store.BaseModel{ID: uuid.New()}, + TenantID: tenantID, + Name: "codex-old", + ProviderType: store.ProviderChatGPTOAuth, + Enabled: false, // disabled + Settings: json.RawMessage(`{ + "codex_pool": { + "strategy": "round_robin", + "extra_provider_names": ["codex-work"] + } + }`), + }, + { + BaseModel: store.BaseModel{ID: uuid.New()}, + TenantID: tenantID, + Name: "codex-work", + ProviderType: store.ProviderChatGPTOAuth, + Enabled: true, + }, + } { + if err := providerStore.CreateProvider(ctx, p); err != nil { + t.Fatalf("CreateProvider() error = %v", err) + } + } + + // New candidate claims "codex-work" — should pass because the old owner is disabled. + candidate := &store.LLMProviderData{ + BaseModel: store.BaseModel{ID: uuid.New()}, + TenantID: tenantID, + Name: "codex-new", + ProviderType: store.ProviderChatGPTOAuth, + Enabled: true, + Settings: json.RawMessage(`{ + "codex_pool": { + "strategy": "primary_first", + "extra_provider_names": ["codex-work"] + } + }`), + } + + if err := validateChatGPTOAuthProviderCandidate(ctx, providerStore, uuid.Nil, candidate); err != nil { + t.Fatalf("validateChatGPTOAuthProviderCandidate() error = %v, want nil (disabled owner should be ignored)", err) + } +} + +// TestValidatePoolGraphRejectsConflictWithEnabledProviders ensures that enabled +// providers still enforce the exclusive-membership rule. +func TestValidatePoolGraphRejectsConflictWithEnabledProviders(t *testing.T) { + providerStore := newMockProviderStore() + tenantID := uuid.New() + ctx := store.WithTenantID(context.Background(), tenantID) + + for _, p := range []*store.LLMProviderData{ + { + BaseModel: store.BaseModel{ID: uuid.New()}, + TenantID: tenantID, + Name: "codex-A", + ProviderType: store.ProviderChatGPTOAuth, + Enabled: true, + Settings: json.RawMessage(`{ + "codex_pool": { + "strategy": "round_robin", + "extra_provider_names": ["codex-work"] + } + }`), + }, + { + BaseModel: store.BaseModel{ID: uuid.New()}, + TenantID: tenantID, + Name: "codex-work", + ProviderType: store.ProviderChatGPTOAuth, + Enabled: true, + }, + } { + if err := providerStore.CreateProvider(ctx, p); err != nil { + t.Fatalf("CreateProvider() error = %v", err) + } + } + + candidate := &store.LLMProviderData{ + BaseModel: store.BaseModel{ID: uuid.New()}, + TenantID: tenantID, + Name: "codex-B", + ProviderType: store.ProviderChatGPTOAuth, + Enabled: true, + Settings: json.RawMessage(`{ + "codex_pool": { + "strategy": "primary_first", + "extra_provider_names": ["codex-work"] + } + }`), + } + + if err := validateChatGPTOAuthProviderCandidate(ctx, providerStore, uuid.Nil, candidate); err == nil { + t.Fatal("validateChatGPTOAuthProviderCandidate() = nil, want membership conflict with enabled provider") + } +} + +// TestValidatePoolGraphAllowsReassignAfterDisable verifies that a member can be +// reassigned to a new pool after the original pool owner is disabled. +func TestValidatePoolGraphAllowsReassignAfterDisable(t *testing.T) { + providerStore := newMockProviderStore() + tenantID := uuid.New() + ctx := store.WithTenantID(context.Background(), tenantID) + + for _, p := range []*store.LLMProviderData{ + { + BaseModel: store.BaseModel{ID: uuid.New()}, + TenantID: tenantID, + Name: "codex-A", + ProviderType: store.ProviderChatGPTOAuth, + Enabled: false, // previously active, now disabled + Settings: json.RawMessage(`{ + "codex_pool": { + "strategy": "round_robin", + "extra_provider_names": ["codex-work"] + } + }`), + }, + { + BaseModel: store.BaseModel{ID: uuid.New()}, + TenantID: tenantID, + Name: "codex-work", + ProviderType: store.ProviderChatGPTOAuth, + Enabled: true, + }, + } { + if err := providerStore.CreateProvider(ctx, p); err != nil { + t.Fatalf("CreateProvider() error = %v", err) + } + } + + candidate := &store.LLMProviderData{ + BaseModel: store.BaseModel{ID: uuid.New()}, + TenantID: tenantID, + Name: "codex-B", + ProviderType: store.ProviderChatGPTOAuth, + Enabled: true, + Settings: json.RawMessage(`{ + "codex_pool": { + "strategy": "priority_order", + "extra_provider_names": ["codex-work"] + } + }`), + } + + if err := validateChatGPTOAuthProviderCandidate(ctx, providerStore, uuid.Nil, candidate); err != nil { + t.Fatalf("validateChatGPTOAuthProviderCandidate() error = %v, want nil (disabled owner should allow reassignment)", err) + } +} diff --git a/ui/web/src/i18n/locales/en/agents.json b/ui/web/src/i18n/locales/en/agents.json index 077de556..c547db5c 100644 --- a/ui/web/src/i18n/locales/en/agents.json +++ b/ui/web/src/i18n/locales/en/agents.json @@ -185,6 +185,7 @@ "unsavedChangesTitle": "Draft changes are not live yet", "unsavedChangesDescription": "The verification cards below reflect the last saved pool. Save this draft before using the evidence to validate round robin.", "draftBadge": "Draft pending", + "providerChangedWarning": "Provider changed — pool routing will be reset on save.", "savedPoolOnlyHint": "Live evidence still reflects the last saved pool.", "verificationTitle": "How verification works", "verificationDescription": "Evidence comes from recent LLM spans for this agent. Refresh this page after a few requests to compare the configured pool with the aliases that were actually used.", diff --git a/ui/web/src/i18n/locales/vi/agents.json b/ui/web/src/i18n/locales/vi/agents.json index ebfae735..7aa2fba8 100644 --- a/ui/web/src/i18n/locales/vi/agents.json +++ b/ui/web/src/i18n/locales/vi/agents.json @@ -185,6 +185,7 @@ "unsavedChangesTitle": "Bản nháp chưa được áp dụng", "unsavedChangesDescription": "Các thẻ xác minh bên dưới đang phản ánh pool đã lưu gần nhất. Hãy lưu bản nháp này trước khi dùng bằng chứng để xác nhận round robin.", "draftBadge": "Bản nháp chờ lưu", + "providerChangedWarning": "Provider đã thay đổi — cấu hình pool routing sẽ bị xóa khi lưu.", "savedPoolOnlyHint": "Bằng chứng trực tiếp vẫn đang phản ánh pool đã lưu gần nhất.", "verificationTitle": "Cách xác minh hoạt động", "verificationDescription": "Bằng chứng đến từ các LLM span gần đây của agent này. Làm mới trang sau vài request để so sánh pool đã cấu hình với các bí danh thực sự được dùng.", diff --git a/ui/web/src/i18n/locales/zh/agents.json b/ui/web/src/i18n/locales/zh/agents.json index 57c769ab..73a43d42 100644 --- a/ui/web/src/i18n/locales/zh/agents.json +++ b/ui/web/src/i18n/locales/zh/agents.json @@ -185,6 +185,7 @@ "unsavedChangesTitle": "草稿尚未生效", "unsavedChangesDescription": "下方验证卡片反映的是最近一次已保存的池配置。请先保存这个草稿，再使用证据验证 round robin。", "draftBadge": "草稿待保存", + "providerChangedWarning": "提供者已更改 — 池路由配置将在保存时重置。", "savedPoolOnlyHint": "实时证据仍然反映最近一次已保存的池配置。", "verificationTitle": "如何验证", "verificationDescription": "证据来自该 agent 最近的 LLM span。发送几次请求后刷新此页，即可对比配置好的池和实际使用的别名。", diff --git a/ui/web/src/pages/agents/agent-detail/agent-overview-tab.tsx b/ui/web/src/pages/agents/agent-detail/agent-overview-tab.tsx index 31fc1bd3..71b73d1b 100644 --- a/ui/web/src/pages/agents/agent-detail/agent-overview-tab.tsx +++ b/ui/web/src/pages/agents/agent-detail/agent-overview-tab.tsx @@ -64,13 +64,18 @@ export function AgentOverviewTab({ agent, onUpdate, heartbeat, onManageCodexPool const handleSave = async () => { setSaving(true); try { - const updatedOtherConfig = { + const updatedOtherConfig: Record = { ...otherCfg, emoji: emoji.trim() || undefined, self_evolve: selfEvolve, skill_evolve: skillEvolve, skill_nudge_interval: skillEvolve ? skillNudgeInterval : undefined, }; + // When the provider changes, clear stale pool routing config so it + // doesn't reference members from the previous provider's pool. + if (provider !== agent.provider) { + delete updatedOtherConfig.chatgpt_oauth_routing; + } const budgetCents = budgetDollars ? Math.round(parseFloat(budgetDollars) * 100) : null; await onUpdate({ display_name: displayName, @@ -129,6 +134,11 @@ export function AgentOverviewTab({ agent, onUpdate, heartbeat, onManageCodexPool /> + {provider !== agent.provider && !!otherCfg.chatgpt_oauth_routing && ( +

+ {t("chatgptOAuthRouting.providerChangedWarning")} +

+ )} {/* Memory — always visible, per-agent overrides */} ("/v1/providers"); - const provider = res.providers?.find((p) => p.provider_type === "chatgpt_oauth" && p.name === name.trim()); + // Retry up to 3 times with 1 s delay — backend may not have committed + // the provider yet when the OAuth polling detects success. + let provider: ProviderData | undefined; + for (let attempt = 0; attempt < 3; attempt++) { + const res = await http.get<{ providers: ProviderData[] }>("/v1/providers"); + provider = res.providers?.find( + (p) => p.provider_type === "chatgpt_oauth" && p.name === name.trim(), + ); + if (provider) break; + if (attempt < 2) await new Promise((r) => setTimeout(r, 1000)); + } if (!provider) { setError(t("provider.errors.oauthProviderNotFound")); return;