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)
This commit is contained in:
viettranx committed 2026-04-01 19:31:02 +07:00
1 parent d334a2082f
commit da240a5e09
7 files changed
+191 -3

No files matched your search

@@ -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
@@ -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)
}
}
+1
View File
@@ -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.",
+1
View File
@@ -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.",
+1
View File
@@ -185,6 +185,7 @@
"unsavedChangesTitle": "草稿尚未生效",
"unsavedChangesDescription": "下方验证卡片反映的是最近一次已保存的池配置。请先保存这个草稿,再使用证据验证 round robin。",
"draftBadge": "草稿待保存",
"providerChangedWarning": "提供者已更改 — 池路由配置将在保存时重置。",
"savedPoolOnlyHint": "实时证据仍然反映最近一次已保存的池配置。",
"verificationTitle": "如何验证",
"verificationDescription": "证据来自该 agent 最近的 LLM span。发送几次请求后刷新此页,即可对比配置好的池和实际使用的别名。",
@@ -64,13 +64,18 @@ export function AgentOverviewTab({ agent, onUpdate, heartbeat, onManageCodexPool
const handleSave = async () => {
setSaving(true);
try {
const updatedOtherConfig = {
const updatedOtherConfig: Record<string, unknown> = {
...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
/>
<ChatGPTOAuthRoutingSummarySection agent={agent} onManage={onManageCodexPool} />
{provider !== agent.provider && !!otherCfg.chatgpt_oauth_routing && (
<p className="text-xs text-amber-600 dark:text-amber-400 -mt-2 px-1">
{t("chatgptOAuthRouting.providerChangedWarning")}
</p>
)}
{/* Memory — always visible, per-agent overrides */}
<MemorySection
+11 -2
View File
@@ -72,8 +72,17 @@ export function StepProvider({ onComplete, existingProvider }: StepProviderProps
setLoading(true);
setError("");
try {
const res = await http.get<{ providers: ProviderData[] }>("/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;