diff --git a/.github/pr-assets/1006/codex-pool-inherit-round-robin.png b/.github/pr-assets/1006/codex-pool-inherit-round-robin.png new file mode 100644 index 00000000..0e7484f9 Binary files /dev/null and b/.github/pr-assets/1006/codex-pool-inherit-round-robin.png differ diff --git a/.github/pr-assets/1006/index.html b/.github/pr-assets/1006/index.html new file mode 100644 index 00000000..1fce050d --- /dev/null +++ b/.github/pr-assets/1006/index.html @@ -0,0 +1,75 @@ + + + + + +PR 1006 · Codex pool refactor + pool-aware create_image + + + + +
+

PR 1006 · Codex pool refactor + pool-aware create_image

+
Closes #1001 and #1008. Captures: staging gateway on claw, master tenant, light theme, openai-codex provider configured as a 2-member round_robin pool with openai-codex-2 as a member.
+ +
+

What changed

Chain entries pointing at a Codex OAuth pool now route through the pool's own strategy with internal failover. The outer chain only advances after the pool is fully exhausted.

+

Why it matters

Before: users could accidentally select a pool member in the chain and bypass pool semantics. Now: the dropdown hides pool members and tags owners with an inline Pool chip — mirrors the Create Agent dropdown.

+

Review cue

The inline Pool chip next to openai-codex — and the absence of openai-codex-2 from the list — proves the UX unification. Backend failover is proven by 5 integration scenarios in create_image_pool_chain_test.go.

+
+ +
+

1. Pool-filtered Provider dropdown

+
Red callout marks the openai-codex option tagged with an inline Pool chip. openai-codex-2 (a pool member) is no longer listed — pool routing is reached only by picking the owner, matching the existing Create Agent dropdown pattern.
+
+
+ Implemented + Create Image — Provider Chain dialog, Provider dropdown open +
+ Pool-filtered dropdown with inline Pool chip on owner +
+
+ +
+

2. Backend validation

+
Full test matrix executed on the PR branch at the current HEAD.
+ 
go build ./...                           — ok (PG)
+go build -tags sqliteonly ./...          — ok (Desktop)
+go vet ./...                             — no issues
+go test ./internal/tools/... ./internal/providers/...   — 1599 passed
+Integration: 5 pool-chain scenarios × 5 runs under -race — 25/25 deterministic
+pnpm --dir ui/web tsc --noEmit           — no errors
+
+
+ + diff --git a/.github/pr-assets/1006/pool-dropdown-filtered.png b/.github/pr-assets/1006/pool-dropdown-filtered.png new file mode 100644 index 00000000..ceb997fb Binary files /dev/null and b/.github/pr-assets/1006/pool-dropdown-filtered.png differ diff --git a/docs/02-providers.md b/docs/02-providers.md index d17d26d3..cfae5c8b 100644 --- a/docs/02-providers.md +++ b/docs/02-providers.md @@ -279,7 +279,7 @@ Extended thinking allows LLMs to generate internal reasoning tokens before produ ```mermaid flowchart TD - LEVEL["provider.settings.reasoning_defaults
+ agent other_config.reasoning"] --> CHECK{"Provider
supports thinking?"} + LEVEL["provider.settings.reasoning_defaults
+ agent reasoning_config"] --> CHECK{"Provider
supports thinking?"} CHECK -->|No| SKIP["Skip — normal request"] CHECK -->|Yes| TYPE{"Provider type?"} @@ -668,12 +668,10 @@ Agent override example: ```json { "provider": "openai-codex", - "other_config": { - "reasoning": { - "override_mode": "custom", - "effort": "xhigh", - "fallback": "downgrade" - } + "reasoning_config": { + "override_mode": "custom", + "effort": "xhigh", + "fallback": "downgrade" } } ``` @@ -685,18 +683,18 @@ Routing behavior: - A provider listed in another pool cannot also manage its own pool. - `override_mode: "inherit"` uses the primary provider's `settings.codex_pool`. - `override_mode: "custom"` is limited to routing behavior for that provider-owned pool. -- `primary_first` keeps the preferred account fixed. When saved as a custom override with no extra names, it disables the pool for that agent and keeps the agent on the primary account only. - `round_robin` rotates requests across the preferred account plus the provider-owned extra authenticated OpenAI Codex OAuth accounts. - `priority_order` tries the preferred account first, then drains the provider-owned extra accounts in order. +- Legacy `primary_first` configs are read back as `priority_order`. Existing agent overrides that explicitly saved an empty `extra_provider_names` list still remain single-account-only after migration. - Retryable upstream failures can fall through to the next eligible OpenAI Codex OAuth account in the same request. - Explicit provider names remain explicit. OAuth auth/logout is still provider-scoped. - Runtime observability for one agent is available at `GET /v1/agents/{id}/codex-pool-activity`, which exposes recent routed traces plus per-alias health derived from those traces. Reasoning behavior: - `settings.reasoning_defaults` is provider-owned and reusable across agents. -- `reasoning.override_mode: "inherit"` follows the provider default. -- `reasoning.override_mode: "custom"` stores an agent-local reasoning policy. -- Existing `reasoning` payloads without `override_mode` still behave as custom overrides. +- `reasoning_config.override_mode: "inherit"` follows the provider default. +- `reasoning_config.override_mode: "custom"` stores an agent-local reasoning policy. +- Existing legacy `other_config.reasoning` payloads without `override_mode` still behave as custom overrides. - If no provider default is saved, inherit resolves to reasoning `off`. - Trace metadata surfaces the reasoning `source` so provider-default behavior is no longer implicit. diff --git a/docs/12-extended-thinking.md b/docs/12-extended-thinking.md index 3bb70131..f5e60ae2 100644 --- a/docs/12-extended-thinking.md +++ b/docs/12-extended-thinking.md @@ -8,7 +8,7 @@ Extended thinking allows LLM providers to "think out loud" before producing a fi ## 1. Configuration -The reusable default now lives on the provider in `settings.reasoning_defaults`. Agents consume that default by inheriting it, or store a custom override in `other_config.reasoning`. `thinking_level` remains the backward-compatible coarse shim for older builds. +The reusable default now lives on the provider in `settings.reasoning_defaults`. Agents consume that default by inheriting it, or store a custom override in top-level `reasoning_config`. `thinking_level` remains the backward-compatible coarse shim for older builds. | Level | Behavior | |-------|----------| @@ -35,10 +35,8 @@ The reusable default now lives on the provider in `settings.reasoning_defaults`. ```json { - "other_config": { - "reasoning": { - "override_mode": "inherit" - } + "reasoning_config": { + "override_mode": "inherit" } } ``` @@ -47,13 +45,11 @@ The reusable default now lives on the provider in `settings.reasoning_defaults`. ```json { - "other_config": { - "thinking_level": "high", - "reasoning": { - "override_mode": "custom", - "effort": "xhigh", - "fallback": "downgrade" - } + "thinking_level": "high", + "reasoning_config": { + "override_mode": "custom", + "effort": "xhigh", + "fallback": "downgrade" } } ``` @@ -61,11 +57,11 @@ The reusable default now lives on the provider in `settings.reasoning_defaults`. Rules: - Unset provider defaults and unset agent reasoning both resolve to `off`. - `settings.reasoning_defaults` is provider-owned and reusable across agents. -- `reasoning.override_mode` accepts `inherit|custom`. +- `reasoning_config.override_mode` accepts `inherit|custom`. - `thinking_level` still accepts `off|low|medium|high`. -- `reasoning.effort` accepts `off|auto|none|minimal|low|medium|high|xhigh`. -- `reasoning.fallback` accepts `downgrade|off|provider_default`. -- Existing `reasoning` payloads without `override_mode` are treated as custom overrides for backward compatibility. +- `reasoning_config.effort` accepts `off|auto|none|minimal|low|medium|high|xhigh`. +- `reasoning_config.fallback` accepts `downgrade|off|provider_default`. +- Existing legacy `other_config.reasoning` payloads without `override_mode` are treated as custom overrides for backward compatibility. - Read path resolves provider defaults first, then applies agent inherit/custom semantics, then falls back to legacy `thinking_level`. - Write path keeps a derived coarse `thinking_level` only for custom agent overrides so rollback to older GoClaw builds stays safe. diff --git a/docs/18-http-api.md b/docs/18-http-api.md index b7402696..33047f4c 100644 --- a/docs/18-http-api.md +++ b/docs/18-http-api.md @@ -140,20 +140,18 @@ POST /v1/agents/{id}/wake Response: `{content, run_id, usage?}`. Used by orchestrators (n8n, Paperclip) to trigger agent runs. -### Codex/OpenAI OAuth Routing in `other_config` +### Codex/OpenAI OAuth Routing in `chatgpt_oauth_routing` -For agents whose main `provider` is a `chatgpt_oauth` provider, `other_config.chatgpt_oauth_routing` +For agents whose main `provider` is a `chatgpt_oauth` provider, top-level `chatgpt_oauth_routing` can override or inherit routing behavior while keeping the main `provider` field as the preferred/default account alias. ```json { "provider": "openai-codex", "model": "gpt-5.4", - "other_config": { - "chatgpt_oauth_routing": { - "override_mode": "custom", - "strategy": "round_robin" - } + "chatgpt_oauth_routing": { + "override_mode": "custom", + "strategy": "round_robin" } } ``` @@ -164,10 +162,10 @@ Rules: - A provider listed in another pool cannot also manage its own pool. - `override_mode: "inherit"` tells the agent to follow those provider defaults. - `override_mode: "custom"` stores an agent-local routing override for that provider-owned pool. -- `strategy: "primary_first"` keeps the main `provider` as the preferred account. When saved as a custom override with no extra names, it disables pooling for that agent. - Provider aliases are arbitrary. `openai-codex`, `codex-work`, and `codex-team` are examples, not required prefixes. - `strategy: "round_robin"` rotates requests across the main provider plus the provider-owned extra authenticated OpenAI Codex OAuth providers. - `strategy: "priority_order"` tries the main provider first, then drains the provider-owned extra providers in order. +- Legacy `primary_first` payloads are normalized to `priority_order` on read. Existing agent overrides that explicitly saved `extra_provider_names: []` still remain single-account-only after migration. - Retryable upstream failures can fall through to the next eligible OpenAI Codex OAuth provider in the same request. - Only enabled and authenticated `chatgpt_oauth` providers participate. - Provider-scoped auth remains unchanged: `cmd/auth` and `/v1/auth/chatgpt/{provider}/*` still operate on explicit providers. @@ -209,30 +207,28 @@ Rules: - the final runtime effort is still normalized against the agent's selected model capabilities - if no provider default is saved, inherit mode resolves to reasoning `off` -### Agent reasoning policy in `other_config` +### Agent reasoning policy in `reasoning_config` -Agents can now store capability-aware GPT-5/Codex reasoning intent under `other_config.reasoning`. +Agents can now store capability-aware GPT-5/Codex reasoning intent under top-level `reasoning_config`. ```json { "provider": "openai-codex", "model": "gpt-5.4", - "other_config": { - "reasoning": { - "override_mode": "inherit" - } + "reasoning_config": { + "override_mode": "inherit" } } ``` Rules: -- `reasoning.override_mode` supports `inherit|custom` +- `reasoning_config.override_mode` supports `inherit|custom` - `override_mode: "inherit"` tells the agent to follow `settings.reasoning_defaults` - `override_mode: "custom"` stores an agent-local override; the dashboard also writes a derived `thinking_level` shim for rollback safety - `thinking_level` remains the coarse compatibility shim: `off|low|medium|high` -- `reasoning.effort` supports `off|auto|none|minimal|low|medium|high|xhigh` -- `reasoning.fallback` supports `downgrade|off|provider_default` -- existing `reasoning` payloads without `override_mode` continue to behave as custom overrides +- `reasoning_config.effort` supports `off|auto|none|minimal|low|medium|high|xhigh` +- `reasoning_config.fallback` supports `downgrade|off|provider_default` +- existing legacy `other_config.reasoning` payloads without `override_mode` continue to behave as custom overrides - unset reasoning resolves to `off` - the runtime may normalize unsupported efforts, and the actual decision is surfaced in trace span metadata @@ -266,7 +262,7 @@ Query parameters: - `limit` optional, defaults to `18`, max `50` Response fields: -- `strategy`: effective routing strategy (`primary_first`, `round_robin`, or `priority_order`) +- `strategy`: effective routing strategy (`round_robin` or `priority_order`) - `pool_providers`: configured primary + extra provider aliases in pool order - `stats_sample_size`: number of recent routed `llm_call` spans used to derive runtime health. The server derives health from `max(limit, 120)` recent spans even when `recent_requests` is still capped by the requested `limit`. - `provider_counts`: per-alias routing evidence: diff --git a/docs/project-changelog.md b/docs/project-changelog.md index 24c867d5..36e559d4 100644 --- a/docs/project-changelog.md +++ b/docs/project-changelog.md @@ -33,8 +33,6 @@ Implementation is evidence-backed against the native ChatGPT Responses API event - `plans/260422-1349-goclaw-chatgpt-image-gen/` — plan + phase files. - `plans/reports/researcher-260422-1414-codex-native-image-events.md` — native event schema. ---- - ## 2026-04-20 ### Pipeline: accurate context token tracking + dynamic compaction @@ -96,6 +94,21 @@ Implementation is evidence-backed against the native ChatGPT Responses API event --- +## 2026-04-22 + +### Codex OAuth pool routing strategy cleanup + +**Changes** + +- Removed `primary_first` from the public Codex OAuth routing strategy surface. The API, OpenAPI schema, and web UI now expose only `round_robin` and `priority_order`. +- Legacy `primary_first` and `manual` routing values now normalize to `priority_order` on read in the backend store layer. +- Activity endpoints now default empty/no-pool responses to `priority_order` instead of `primary_first`. +- Agent overrides that explicitly persist `extra_provider_names: []` continue to behave as single-account-only routing after the migration. + +**Docs** + +- Updated `docs/02-providers.md` and `docs/18-http-api.md` to describe the two-strategy model and the compatibility migration. + ## 2026-04-19 ### TTS: Gemini provider + ProviderCapabilities schema engine diff --git a/internal/http/agents.go b/internal/http/agents.go index b1823bcf..376964c3 100644 --- a/internal/http/agents.go +++ b/internal/http/agents.go @@ -35,16 +35,16 @@ type AgentsHandler struct { kgStore store.KnowledgeGraphStore // for import (nil = disabled) episodicStore store.EpisodicStore // for import (nil in SQLite/lite builds) vaultStore store.VaultStore // for vault import (nil = disabled) - toolsReg ToolPreviewLister // for system prompt preview tool resolution (nil = fallback) - skillsLoader SkillPreviewBuilder // for system prompt preview pinned skills (nil = skip) - skillAccessStore store.SkillAccessStore // for system prompt preview skill filtering (nil = skip) + toolsReg ToolPreviewLister // for system prompt preview tool resolution (nil = fallback) + skillsLoader SkillPreviewBuilder // for system prompt preview pinned skills (nil = skip) + skillAccessStore store.SkillAccessStore // for system prompt preview skill filtering (nil = skip) teamStore store.TeamStore // for system prompt preview team context (nil = skip) agentLinkStore store.AgentLinkStore // for system prompt preview delegation targets (nil = skip) - defaultWorkspace string // default workspace path template (e.g. "~/.goclaw/workspace") - dataDir string // resolved data directory (e.g. "~/.goclaw/data") — for team workspace export - msgBus *bus.MessageBus // for cache invalidation events (nil = no events) - summoner *AgentSummoner // LLM-based agent setup (nil = disabled) - isOwner func(string) bool // checks if user ID is a system owner (nil = no owners configured) + defaultWorkspace string // default workspace path template (e.g. "~/.goclaw/workspace") + dataDir string // resolved data directory (e.g. "~/.goclaw/data") — for team workspace export + msgBus *bus.MessageBus // for cache invalidation events (nil = no events) + summoner *AgentSummoner // LLM-based agent setup (nil = disabled) + isOwner func(string) bool // checks if user ID is a system owner (nil = no owners configured) } // NewAgentsHandler creates a handler for agent management endpoints. @@ -205,7 +205,11 @@ func (h *AgentsHandler) handleList(w http.ResponseWriter, r *http.Request) { return } - writeJSON(w, http.StatusOK, map[string]any{"agents": agents}) + publicAgents := make([]store.AgentData, 0, len(agents)) + for i := range agents { + publicAgents = append(publicAgents, canonicalizeAgentForResponse(&agents[i])) + } + writeJSON(w, http.StatusOK, map[string]any{"agents": publicAgents}) } func (h *AgentsHandler) handleCreate(w http.ResponseWriter, r *http.Request) { @@ -306,7 +310,8 @@ func (h *AgentsHandler) handleCreate(w http.ResponseWriter, r *http.Request) { } emitAudit(h.msgBus, r, "agent.created", "agent", req.ID.String()) - writeJSON(w, http.StatusCreated, req) + publicAgent := canonicalizeAgentForResponse(&req) + writeJSON(w, http.StatusCreated, publicAgent) } func (h *AgentsHandler) handleGet(w http.ResponseWriter, r *http.Request) { @@ -328,7 +333,8 @@ func (h *AgentsHandler) handleGet(w http.ResponseWriter, r *http.Request) { return } } - writeJSON(w, http.StatusOK, ag) + publicAgent := canonicalizeAgentForResponse(ag) + writeJSON(w, http.StatusOK, publicAgent) return } @@ -345,7 +351,8 @@ func (h *AgentsHandler) handleGet(w http.ResponseWriter, r *http.Request) { } } - writeJSON(w, http.StatusOK, ag) + publicAgent := canonicalizeAgentForResponse(ag) + writeJSON(w, http.StatusOK, publicAgent) } func (h *AgentsHandler) handleUpdate(w http.ResponseWriter, r *http.Request) { diff --git a/internal/http/agents_codex_pool.go b/internal/http/agents_codex_pool.go index ca2e46d4..a7717107 100644 --- a/internal/http/agents_codex_pool.go +++ b/internal/http/agents_codex_pool.go @@ -201,7 +201,7 @@ func (h *AgentsHandler) handleCodexPoolActivity(w http.ResponseWriter, r *http.R statsLimit := maxInt(limit, codexPoolRuntimeHealthSampleSize) baseProviderType, routing, poolProviders := resolveCodexPoolRouting(r.Context(), h.providers, h.providerReg, agent) - strategy := store.ChatGPTOAuthStrategyPrimaryFirst + strategy := store.ChatGPTOAuthStrategyPriority if routing != nil && routing.Strategy != "" { strategy = routing.Strategy } diff --git a/internal/http/agents_export_marshal.go b/internal/http/agents_export_marshal.go index 436315bd..0dcd2e11 100644 --- a/internal/http/agents_export_marshal.go +++ b/internal/http/agents_export_marshal.go @@ -11,6 +11,62 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/store" ) +func canonicalizeChatGPTOAuthRoutingForResponse(raw json.RawMessage) json.RawMessage { + if len(raw) == 0 { + return nil + } + agent := &store.AgentData{ChatGPTOAuthRouting: raw} + routing := store.PublicChatGPTOAuthRouting(agent.ParseChatGPTOAuthRouting()) + if routing == nil { + return nil + } + out, err := json.Marshal(routing) + if err != nil { + return raw + } + return out +} + +func canonicalizeProviderSettingsForResponse(raw json.RawMessage) json.RawMessage { + if len(raw) == 0 { + return nil + } + var settings map[string]any + if err := json.Unmarshal(raw, &settings); err != nil { + return raw + } + providerSettings := store.ParseChatGPTOAuthProviderSettings(raw) + if providerSettings == nil || providerSettings.CodexPool == nil { + delete(settings, "codex_pool") + } else { + routing := store.PublicChatGPTOAuthRouting(providerSettings.CodexPool) + settings["codex_pool"] = map[string]any{ + "strategy": routing.Strategy, + "extra_provider_names": routing.ExtraProviderNames, + } + } + if len(settings) == 0 { + return nil + } + out, err := json.Marshal(settings) + if err != nil { + return raw + } + return out +} + +func canonicalizeAgentForResponse(ag *store.AgentData) store.AgentData { + clone := *ag + clone.ChatGPTOAuthRouting = canonicalizeChatGPTOAuthRoutingForResponse(ag.ChatGPTOAuthRouting) + return clone +} + +func canonicalizeProviderForResponse(p *store.LLMProviderData) store.LLMProviderData { + clone := *p + clone.Settings = canonicalizeProviderSettingsForResponse(p.Settings) + return clone +} + // addToTar adds a single file to the tar archive with a standard header. func addToTar(tw *tar.Writer, name string, data []byte) error { hdr := &tar.Header{ @@ -97,7 +153,7 @@ func marshalAgentConfig(ag *store.AgentData) ([]byte, error) { SkillNudgeInterval: ag.SkillNudgeInterval, ReasoningConfig: ag.ReasoningConfig, WorkspaceSharing: ag.WorkspaceSharing, - ChatGPTOAuthRouting: ag.ChatGPTOAuthRouting, + ChatGPTOAuthRouting: canonicalizeChatGPTOAuthRoutingForResponse(ag.ChatGPTOAuthRouting), ShellDenyGroups: ag.ShellDenyGroups, KGDedupConfig: ag.KGDedupConfig, }, "", " ") diff --git a/internal/http/chatgpt_oauth_pool_validation.go b/internal/http/chatgpt_oauth_pool_validation.go index 8765ad85..fc8bc267 100644 --- a/internal/http/chatgpt_oauth_pool_validation.go +++ b/internal/http/chatgpt_oauth_pool_validation.go @@ -213,7 +213,7 @@ func validateChatGPTOAuthAgentRouting( } if len(defaultMembers) == 0 { - if routing.Strategy != store.ChatGPTOAuthStrategyPrimaryFirst || len(routing.ExtraProviderNames) > 0 { + if len(routing.ExtraProviderNames) > 0 { return fmt.Errorf("configure OpenAI Codex pool members on provider %q before enabling agent-level routing", providerName) } return nil diff --git a/internal/http/chatgpt_oauth_pool_validation_test.go b/internal/http/chatgpt_oauth_pool_validation_test.go index b51925b4..e3e0f1b8 100644 --- a/internal/http/chatgpt_oauth_pool_validation_test.go +++ b/internal/http/chatgpt_oauth_pool_validation_test.go @@ -164,6 +164,31 @@ func TestValidateChatGPTOAuthAgentRoutingAllowsStrategyOnlyOverride(t *testing.T } } +func TestValidateChatGPTOAuthAgentRoutingAllowsPriorityOrderWithoutProviderPool(t *testing.T) { + providerStore := newMockProviderStore() + tenantID := uuid.New() + ctx := store.WithTenantID(context.Background(), tenantID) + + if err := providerStore.CreateProvider(ctx, &store.LLMProviderData{ + BaseModel: store.BaseModel{ID: uuid.New()}, + TenantID: tenantID, + Name: "openai-codex", + ProviderType: store.ProviderChatGPTOAuth, + Enabled: true, + }); err != nil { + t.Fatalf("CreateProvider() error = %v", err) + } + + routing := &store.ChatGPTOAuthRoutingConfig{ + OverrideMode: store.ChatGPTOAuthOverrideCustom, + Strategy: store.ChatGPTOAuthStrategyPriority, + } + + if err := validateChatGPTOAuthAgentRouting(ctx, providerStore, "openai-codex", routing); err != nil { + 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) { @@ -208,7 +233,7 @@ func TestValidatePoolGraphIgnoresDisabledProviders(t *testing.T) { Enabled: true, Settings: json.RawMessage(`{ "codex_pool": { - "strategy": "primary_first", + "strategy": "priority_order", "extra_provider_names": ["codex-work"] } }`), @@ -261,7 +286,7 @@ func TestValidatePoolGraphRejectsConflictWithEnabledProviders(t *testing.T) { Enabled: true, Settings: json.RawMessage(`{ "codex_pool": { - "strategy": "primary_first", + "strategy": "priority_order", "extra_provider_names": ["codex-work"] } }`), diff --git a/internal/http/openapi_spec.json b/internal/http/openapi_spec.json index f1b7ce8c..c046be4a 100644 --- a/internal/http/openapi_spec.json +++ b/internal/http/openapi_spec.json @@ -373,7 +373,7 @@ "type": "object", "required": ["strategy", "pool_providers", "stats_sample_size", "provider_counts", "recent_requests"], "properties": { - "strategy": { "type": "string", "enum": ["primary_first", "round_robin", "priority_order"] }, + "strategy": { "type": "string", "enum": ["round_robin", "priority_order"] }, "pool_providers": { "type": "array", "items": { "type": "string" } @@ -887,43 +887,36 @@ "provider": { "type": "string", "description": "LLM provider name" }, "model": { "type": "string", "description": "Model ID" }, "system_prompt": { "type": "string" }, - "other_config": { - "type": "object", - "description": "Optional per-agent JSON config.", - "properties": { - "thinking_level": { - "type": "string", - "enum": ["off", "low", "medium", "high"], - "description": "Legacy coarse reasoning shim. Unset means off." - }, - "reasoning": { - "type": "object", - "description": "Capability-aware reasoning policy for GPT-5/Codex models.", - "properties": { - "override_mode": { - "type": "string", - "enum": ["inherit", "custom"] - }, - "effort": { - "type": "string", - "enum": ["off", "auto", "none", "minimal", "low", "medium", "high", "xhigh"] - }, - "fallback": { - "type": "string", - "enum": ["downgrade", "off", "provider_default"] - } - } - }, - "chatgpt_oauth_routing": { - "type": "object", - "description": "Optional agent-side routing override for ChatGPT OAuth providers. The main provider field remains the preferred/default account, while provider settings may supply inherited defaults.", - "properties": { - "override_mode": { "type": "string", "enum": ["inherit", "custom"] }, - "strategy": { "type": "string", "enum": ["manual", "primary_first", "round_robin", "priority_order"] }, - "extra_provider_names": { "type": "array", "items": { "type": "string" } } - } + "other_config": { + "type": "object", + "description": "Optional legacy/extensibility bag. Do not nest reasoning or Codex routing here in new writes." + }, + "reasoning_config": { + "type": "object", + "description": "Capability-aware reasoning policy for GPT-5/Codex models.", + "properties": { + "override_mode": { + "type": "string", + "enum": ["inherit", "custom"] + }, + "effort": { + "type": "string", + "enum": ["off", "auto", "none", "minimal", "low", "medium", "high", "xhigh"] + }, + "fallback": { + "type": "string", + "enum": ["downgrade", "off", "provider_default"] } } + }, + "chatgpt_oauth_routing": { + "type": "object", + "description": "Optional agent-side routing override for ChatGPT OAuth providers. The main provider field remains the preferred/default account, while provider settings may supply inherited defaults.", + "properties": { + "override_mode": { "type": "string", "enum": ["inherit", "custom"] }, + "strategy": { "type": "string", "enum": ["round_robin", "priority_order"] }, + "extra_provider_names": { "type": "array", "items": { "type": "string" } } + } } } }, @@ -959,7 +952,7 @@ "properties": { "strategy": { "type": "string", - "enum": ["manual", "primary_first", "round_robin", "priority_order"] + "enum": ["round_robin", "priority_order"] }, "extra_provider_names": { "type": "array", diff --git a/internal/http/providers.go b/internal/http/providers.go index c0fb57d0..ff8fa177 100644 --- a/internal/http/providers.go +++ b/internal/http/providers.go @@ -35,9 +35,9 @@ type ProvidersHandler struct { cliMu sync.Mutex // serializes Claude CLI provider create to prevent duplicates msgBus *bus.MessageBus sysConfigStore store.SystemConfigStore - tracingStore store.TracingStore // optional: for provider-scoped pool activity - agents store.AgentCRUDStore // optional: for provider pool activity agent lookup - modelReg providers.ModelRegistry // optional: forward-compat model resolver for Anthropic + tracingStore store.TracingStore // optional: for provider-scoped pool activity + agents store.AgentCRUDStore // optional: for provider pool activity agent lookup + modelReg providers.ModelRegistry // optional: forward-compat model resolver for Anthropic } // NewProvidersHandler creates a handler for provider management endpoints. @@ -328,7 +328,11 @@ func (h *ProvidersHandler) handleListProviders(w http.ResponseWriter, r *http.Re maskAPIKey(&providers[i]) } - writeJSON(w, http.StatusOK, map[string]any{"providers": providers}) + publicProviders := make([]store.LLMProviderData, 0, len(providers)) + for i := range providers { + publicProviders = append(publicProviders, canonicalizeProviderForResponse(&providers[i])) + } + writeJSON(w, http.StatusOK, map[string]any{"providers": publicProviders}) } func (h *ProvidersHandler) handleCreateProvider(w http.ResponseWriter, r *http.Request) { @@ -399,7 +403,8 @@ func (h *ProvidersHandler) handleCreateProvider(w http.ResponseWriter, r *http.R emitAudit(h.msgBus, r, "provider.created", "provider", p.ID.String()) maskAPIKey(&p) - writeJSON(w, http.StatusCreated, p) + publicProvider := canonicalizeProviderForResponse(&p) + writeJSON(w, http.StatusCreated, publicProvider) } func (h *ProvidersHandler) handleGetProvider(w http.ResponseWriter, r *http.Request) { @@ -417,7 +422,8 @@ func (h *ProvidersHandler) handleGetProvider(w http.ResponseWriter, r *http.Requ } maskAPIKey(p) - writeJSON(w, http.StatusOK, p) + publicProvider := canonicalizeProviderForResponse(p) + writeJSON(w, http.StatusOK, publicProvider) } func (h *ProvidersHandler) handleUpdateProvider(w http.ResponseWriter, r *http.Request) { diff --git a/internal/http/providers_codex_pool_activity.go b/internal/http/providers_codex_pool_activity.go index 8e9c5fdc..25d51695 100644 --- a/internal/http/providers_codex_pool_activity.go +++ b/internal/http/providers_codex_pool_activity.go @@ -53,7 +53,7 @@ func (h *ProvidersHandler) handleProviderCodexPoolActivity(w http.ResponseWriter const maxPoolCandidates = 20 settings := store.ParseChatGPTOAuthProviderSettings(provider.Settings) poolCandidates := []string{provider.Name} - strategy := store.ChatGPTOAuthStrategyPrimaryFirst + strategy := store.ChatGPTOAuthStrategyPriority if settings != nil && settings.CodexPool != nil { if settings.CodexPool.Strategy != "" { strategy = settings.CodexPool.Strategy @@ -125,7 +125,7 @@ func (h *ProvidersHandler) handleProviderCodexPoolActivity(w http.ResponseWriter func emptyProviderPoolActivityResponse() map[string]any { return map[string]any{ - "strategy": store.ChatGPTOAuthStrategyPrimaryFirst, + "strategy": store.ChatGPTOAuthStrategyPriority, "pool_providers": []string{}, "stats_sample_size": 0, "provider_counts": []codexPoolProviderCount{}, diff --git a/internal/http/providers_codex_pool_activity_test.go b/internal/http/providers_codex_pool_activity_test.go new file mode 100644 index 00000000..12f3af1b --- /dev/null +++ b/internal/http/providers_codex_pool_activity_test.go @@ -0,0 +1,60 @@ +package http + +import ( + "encoding/json" + "reflect" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +func TestEmptyProviderPoolActivityResponseDefaultsToPriorityOrder(t *testing.T) { + got := emptyProviderPoolActivityResponse() + + if got["strategy"] != store.ChatGPTOAuthStrategyPriority { + t.Fatalf("strategy = %v, want %q", got["strategy"], store.ChatGPTOAuthStrategyPriority) + } +} + +func TestCanonicalizeChatGPTOAuthRoutingForResponseMigratesLegacyStrategy(t *testing.T) { + got := canonicalizeChatGPTOAuthRoutingForResponse(json.RawMessage(`{ + "override_mode": "custom", + "strategy": "manual", + "extra_provider_names": [] + }`)) + + var routing map[string]any + if err := json.Unmarshal(got, &routing); err != nil { + t.Fatalf("Unmarshal() error = %v", err) + } + if routing["strategy"] != store.ChatGPTOAuthStrategyPriority { + t.Fatalf("strategy = %v, want %q", routing["strategy"], store.ChatGPTOAuthStrategyPriority) + } +} + +func TestCanonicalizeProviderSettingsForResponseMigratesLegacyPoolStrategy(t *testing.T) { + got := canonicalizeProviderSettingsForResponse(json.RawMessage(`{ + "codex_pool": { + "strategy": "primary_first", + "extra_provider_names": ["codex-work"] + }, + "embedding": { + "enabled": true + } + }`)) + + var settings map[string]any + if err := json.Unmarshal(got, &settings); err != nil { + t.Fatalf("Unmarshal() error = %v", err) + } + pool, ok := settings["codex_pool"].(map[string]any) + if !ok { + t.Fatalf("codex_pool = %#v, want object", settings["codex_pool"]) + } + if pool["strategy"] != store.ChatGPTOAuthStrategyPriority { + t.Fatalf("strategy = %v, want %q", pool["strategy"], store.ChatGPTOAuthStrategyPriority) + } + if !reflect.DeepEqual(pool["extra_provider_names"], []any{"codex-work"}) { + t.Fatalf("extra_provider_names = %#v, want %#v", pool["extra_provider_names"], []any{"codex-work"}) + } +} diff --git a/internal/permissions/policy_test.go b/internal/permissions/policy_test.go index df5d1e45..03d84592 100644 --- a/internal/permissions/policy_test.go +++ b/internal/permissions/policy_test.go @@ -125,6 +125,7 @@ func TestCanAccess_WriteMethods(t *testing.T) { writeMethods := []string{ protocol.MethodChatSend, protocol.MethodSessionsDelete, + protocol.MethodSessionsCompact, protocol.MethodCronCreate, } for _, method := range writeMethods { diff --git a/internal/pipeline/context_stage_integration_test.go b/internal/pipeline/context_stage_integration_test.go index 8b3c60ef..fe55cfde 100644 --- a/internal/pipeline/context_stage_integration_test.go +++ b/internal/pipeline/context_stage_integration_test.go @@ -39,7 +39,7 @@ func buildRealisticToolDefinitions(n int) []providers.ToolDefinition { } tools[i] = providers.ToolDefinition{ Type: "function", - Function: providers.ToolFunctionSchema{ + Function: &providers.ToolFunctionSchema{ Name: "realistic_tool", Description: strings.Repeat( "A realistic tool that performs complex file and system operations. "+ diff --git a/internal/pipeline/context_stage_overhead_test.go b/internal/pipeline/context_stage_overhead_test.go index 1faa5baf..90f5d4fd 100644 --- a/internal/pipeline/context_stage_overhead_test.go +++ b/internal/pipeline/context_stage_overhead_test.go @@ -35,7 +35,7 @@ func fixtureTools(n int) []providers.ToolDefinition { for i := range tools { tools[i] = providers.ToolDefinition{ Type: "function", - Function: providers.ToolFunctionSchema{ + Function: &providers.ToolFunctionSchema{ Name: "tool_fixture", Description: "A fixture tool for testing overhead calculation.", Parameters: map[string]any{"type": "object", "properties": map[string]any{}}, diff --git a/internal/providerresolve/agent_provider.go b/internal/providerresolve/agent_provider.go index 537e6744..9b3e55aa 100644 --- a/internal/providerresolve/agent_provider.go +++ b/internal/providerresolve/agent_provider.go @@ -8,7 +8,7 @@ import ( ) // ResolveConfiguredProvider resolves the provider an agent should actually use. -// It applies ChatGPT OAuth routing from agent other_config when present. +// It applies ChatGPT OAuth routing from the promoted agent routing field when present. func ResolveConfiguredProvider(registry *providers.Registry, agent *store.AgentData) (providers.Provider, error) { if registry == nil || agent == nil { return nil, fmt.Errorf("provider registry unavailable") @@ -31,17 +31,15 @@ func ResolveConfiguredProvider(registry *providers.Registry, agent *store.AgentD } } if routing := store.ResolveEffectiveChatGPTOAuthRouting(providerDefaults, agent.ParseChatGPTOAuthRouting()); routing != nil { - if routing.Strategy != store.ChatGPTOAuthStrategyPrimaryFirst || len(routing.ExtraProviderNames) > 0 { - router := providers.NewChatGPTOAuthRouter( - agent.TenantID, - registry, - agent.Provider, - routing.Strategy, - routing.ExtraProviderNames, - ) - if router != nil && router.HasRegisteredProviders() { - return router, nil - } + router := providers.NewChatGPTOAuthRouter( + agent.TenantID, + registry, + agent.Provider, + routing.Strategy, + routing.ExtraProviderNames, + ) + if router != nil && router.HasRegisteredProviders() { + return router, nil } } diff --git a/internal/providerresolve/agent_provider_test.go b/internal/providerresolve/agent_provider_test.go index fa794a2b..cbb08c9e 100644 --- a/internal/providerresolve/agent_provider_test.go +++ b/internal/providerresolve/agent_provider_test.go @@ -172,11 +172,12 @@ func TestResolveConfiguredProviderKeepsExplicitSingleAccountOverride(t *testing. if err != nil { t.Fatalf("ResolveConfiguredProvider() error = %v", err) } - if _, ok := resolved.(*providers.ChatGPTOAuthRouter); ok { - t.Fatalf("ResolveConfiguredProvider() returned %T, want base Codex provider", resolved) + router, ok := resolved.(*providers.ChatGPTOAuthRouter) + if !ok { + t.Fatalf("ResolveConfiguredProvider() returned %T, want *providers.ChatGPTOAuthRouter", resolved) } - if resolved.Name() != "openai-codex" { - t.Fatalf("resolved.Name() = %q, want %q", resolved.Name(), "openai-codex") + if router.Name() != "openai-codex" { + t.Fatalf("router.Name() = %q, want %q", router.Name(), "openai-codex") } } diff --git a/internal/providers/chatgpt_oauth_router_image.go b/internal/providers/chatgpt_oauth_router_image.go new file mode 100644 index 00000000..c8bd1872 --- /dev/null +++ b/internal/providers/chatgpt_oauth_router_image.go @@ -0,0 +1,90 @@ +package providers + +import ( + "context" + "fmt" + "log/slog" + "strings" +) + +// compile-time assertion: ChatGPTOAuthRouter satisfies NativeImageProvider. +var _ NativeImageProvider = (*ChatGPTOAuthRouter)(nil) + +// GenerateImage implements NativeImageProvider for ChatGPTOAuthRouter. +// It iterates the strategy-ordered pool members, delegating to each member's +// GenerateImage in turn. Failover semantics mirror the Chat/call() path: +// - retryable error (IsRetryableError) → try next member +// - non-retryable error → return immediately +// - all members exhausted → return aggregated error naming every attempted member +// +// Round-robin state advances once per GenerateImage call (via orderedProviders +// advance=true), regardless of which member ultimately serves the response. +// This matches the Chat path semantics documented on call(). +func (p *ChatGPTOAuthRouter) GenerateImage(ctx context.Context, req NativeImageRequest) (*NativeImageResult, error) { + ordered, err := p.orderedProviders(ctx, true) + if err != nil { + return nil, err + } + + if observation := ChatGPTOAuthRoutingObservationFromContext(ctx); observation != nil { + poolProviders := make([]string, 0, len(p.registeredProviders())) + for _, provider := range p.registeredProviders() { + poolProviders = append(poolProviders, provider.Name()) + } + observation.SetPool(p.defaultProviderName, p.strategy, poolProviders) + } + + attempted := make([]string, 0, len(ordered)) + var lastErr error + + for i, provider := range ordered { + // Check context before attempting each member so a pre-cancelled ctx is + // caught even when orderedProviders returns without error. + if ctx.Err() != nil { + return nil, ctx.Err() + } + + np, ok := provider.(NativeImageProvider) + if !ok { + slog.Warn("chatgpt_oauth router image: member has no native image support, skipping", + "provider", provider.Name(), + ) + lastErr = fmt.Errorf("member %s has no native image support", provider.Name()) + attempted = append(attempted, provider.Name()) + continue + } + + if observation := ChatGPTOAuthRoutingObservationFromContext(ctx); observation != nil { + observation.RecordAttempt(provider.Name()) + } + + attempted = append(attempted, provider.Name()) + res, callErr := np.GenerateImage(ctx, req) + if callErr == nil { + if observation := ChatGPTOAuthRoutingObservationFromContext(ctx); observation != nil { + observation.RecordSuccess(provider.Name()) + } + return res, nil + } + + lastErr = callErr + + // Non-retryable error: surface immediately without trying further members. + if !IsRetryableError(callErr) { + return nil, callErr + } + + // Retryable: log and continue to the next member if one exists. + if i < len(ordered)-1 { + slog.Warn("chatgpt_oauth router image failover", + "from", provider.Name(), + "to", ordered[i+1].Name(), + "error", callErr, + ) + } + } + + // All members exhausted (or none implemented NativeImageProvider). + return nil, fmt.Errorf("all pool members failed image generation (%s): %w", + strings.Join(attempted, ", "), lastErr) +} diff --git a/internal/providers/chatgpt_oauth_router_image_test.go b/internal/providers/chatgpt_oauth_router_image_test.go new file mode 100644 index 00000000..88641ae8 --- /dev/null +++ b/internal/providers/chatgpt_oauth_router_image_test.go @@ -0,0 +1,242 @@ +package providers + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/google/uuid" +) + +// imageSSEResponse builds a minimal SSE body that parseNativeImageSSE will accept. +// b64data is base64-encoded image bytes (any non-empty string works for routing tests). +func imageSSEResponse(b64data string) string { + return `data: {"type":"response.output_item.done","item":{"type":"image_generation_call","result":"` + + b64data + `","output_format":"png"}}` + "\n\ndata: [DONE]\n" +} + +// imageTestServer returns a test HTTP server that responds with a successful image SSE body. +// body is called on each request so callers can count hits via a closure. +func imageTestServer(t *testing.T, body func() string) *httptest.Server { + t.Helper() + s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(body())) + })) + t.Cleanup(s.Close) + return s +} + +// retryableImageTestServer returns a test HTTP server that always responds HTTP 429. +func retryableImageTestServer(t *testing.T) *httptest.Server { + t.Helper() + s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "rate limited", http.StatusTooManyRequests) + })) + t.Cleanup(s.Close) + return s +} + +// badRequestImageTestServer returns a test HTTP server that always responds HTTP 400 (non-retryable). +func badRequestImageTestServer(t *testing.T) *httptest.Server { + t.Helper() + s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "bad request", http.StatusBadRequest) + })) + t.Cleanup(s.Close) + return s +} + +// newImageCodexProvider creates a *CodexProvider with retries disabled, pointing at apiBase. +func newImageCodexProvider(name, apiBase string) *CodexProvider { + p := NewCodexProvider(name, &staticTokenSource{token: "token-" + name}, apiBase, "gpt-5.4") + p.retryConfig.Attempts = 1 // disable internal retries so router failover logic is exercised + return p +} + +// imageReq is a minimal valid NativeImageRequest used across image router tests. +var imageReq = NativeImageRequest{Prompt: "a cat", ImageModel: "gpt-image-2"} + +// b64img is a non-empty base64 string used as placeholder image data in test SSE responses. +const b64img = "aW1hZ2VkYXRh" // "imagedata" — not a valid PNG, but parseNativeImageSSE accepts any non-empty b64 + +// TestChatGPTOAuthRouterImage_RoundRobin_RotatesAcrossCalls verifies that 2 successive +// GenerateImage calls each hit a different pool member (round-robin distribution). +func TestChatGPTOAuthRouterImage_RoundRobin_RotatesAcrossCalls(t *testing.T) { + tenantID := uuid.New() + registry := NewRegistry(nil) + + var hitsA, hitsB int + serverA := imageTestServer(t, func() string { hitsA++; return imageSSEResponse(b64img) }) + serverB := imageTestServer(t, func() string { hitsB++; return imageSSEResponse(b64img) }) + + registry.RegisterForTenant(tenantID, newImageCodexProvider("acct-a", serverA.URL)) + registry.RegisterForTenant(tenantID, newImageCodexProvider("acct-b", serverB.URL)) + + router := NewChatGPTOAuthRouter(tenantID, registry, "acct-a", "round_robin", []string{"acct-b"}) + + for i := range 2 { + if _, err := router.GenerateImage(context.Background(), imageReq); err != nil { + t.Fatalf("call %d: GenerateImage failed: %v", i, err) + } + } + if hitsA != 1 { + t.Fatalf("hitsA = %d, want 1", hitsA) + } + if hitsB != 1 { + t.Fatalf("hitsB = %d, want 1", hitsB) + } +} + +// TestChatGPTOAuthRouterImage_FirstRetryable_SecondSucceeds verifies that when member A +// returns HTTP 429 (retryable), the router fails over to member B and returns its result. +func TestChatGPTOAuthRouterImage_FirstRetryable_SecondSucceeds(t *testing.T) { + tenantID := uuid.New() + registry := NewRegistry(nil) + + serverA := retryableImageTestServer(t) + serverB := imageTestServer(t, func() string { return imageSSEResponse(b64img) }) + + registry.RegisterForTenant(tenantID, newImageCodexProvider("acct-a", serverA.URL)) + registry.RegisterForTenant(tenantID, newImageCodexProvider("acct-b", serverB.URL)) + + router := NewChatGPTOAuthRouter(tenantID, registry, "acct-a", "round_robin", []string{"acct-b"}) + + result, err := router.GenerateImage(context.Background(), imageReq) + if err != nil { + t.Fatalf("GenerateImage failed: %v", err) + } + if len(result.Data) == 0 { + t.Fatal("result.Data is empty — expected image bytes from member B") + } +} + +// TestChatGPTOAuthRouterImage_PriorityOrder_FirstFails_SecondSucceeds verifies failover +// under the priority_order strategy: A returns HTTP 429, B succeeds. +func TestChatGPTOAuthRouterImage_PriorityOrder_FirstFails_SecondSucceeds(t *testing.T) { + tenantID := uuid.New() + registry := NewRegistry(nil) + + serverA := retryableImageTestServer(t) + serverB := imageTestServer(t, func() string { return imageSSEResponse(b64img) }) + + registry.RegisterForTenant(tenantID, newImageCodexProvider("acct-a", serverA.URL)) + registry.RegisterForTenant(tenantID, newImageCodexProvider("acct-b", serverB.URL)) + + router := NewChatGPTOAuthRouter(tenantID, registry, "acct-a", "priority_order", []string{"acct-b"}) + + result, err := router.GenerateImage(context.Background(), imageReq) + if err != nil { + t.Fatalf("GenerateImage (priority_order) failed: %v", err) + } + if len(result.Data) == 0 { + t.Fatal("result.Data is empty") + } +} + +// TestChatGPTOAuthRouterImage_NonRetryable_ReturnsImmediately verifies that HTTP 400 +// (non-retryable) is returned immediately without attempting member B. +func TestChatGPTOAuthRouterImage_NonRetryable_ReturnsImmediately(t *testing.T) { + tenantID := uuid.New() + registry := NewRegistry(nil) + + var hitsB int + serverA := badRequestImageTestServer(t) + serverB := imageTestServer(t, func() string { hitsB++; return imageSSEResponse(b64img) }) + + registry.RegisterForTenant(tenantID, newImageCodexProvider("acct-a", serverA.URL)) + registry.RegisterForTenant(tenantID, newImageCodexProvider("acct-b", serverB.URL)) + + router := NewChatGPTOAuthRouter(tenantID, registry, "acct-a", "round_robin", []string{"acct-b"}) + + _, err := router.GenerateImage(context.Background(), imageReq) + if err == nil { + t.Fatal("GenerateImage should have failed on non-retryable HTTP 400") + } + if hitsB != 0 { + t.Fatalf("hitsB = %d, want 0 (B must not be attempted after non-retryable error)", hitsB) + } +} + +// TestChatGPTOAuthRouterImage_AllFail_AggregatedError verifies that when all 3 members +// return retryable errors, the returned error message mentions all member names. +func TestChatGPTOAuthRouterImage_AllFail_AggregatedError(t *testing.T) { + tenantID := uuid.New() + registry := NewRegistry(nil) + + for _, name := range []string{"acct-a", "acct-b", "acct-c"} { + s := retryableImageTestServer(t) + registry.RegisterForTenant(tenantID, newImageCodexProvider(name, s.URL)) + } + + router := NewChatGPTOAuthRouter(tenantID, registry, "acct-a", "priority_order", []string{"acct-b", "acct-c"}) + + _, err := router.GenerateImage(context.Background(), imageReq) + if err == nil { + t.Fatal("GenerateImage should fail when all members fail") + } + errStr := err.Error() + for _, name := range []string{"acct-a", "acct-b", "acct-c"} { + if !strings.Contains(errStr, name) { + t.Fatalf("error %q does not mention member %q", errStr, name) + } + } +} + +// TestChatGPTOAuthRouterImage_ContextCancel_Aborts verifies that a pre-cancelled context +// causes GenerateImage to return a context-derived error. +func TestChatGPTOAuthRouterImage_ContextCancel_Aborts(t *testing.T) { + tenantID := uuid.New() + registry := NewRegistry(nil) + + // retryable server so the router would attempt failover — but context cancels first + serverA := retryableImageTestServer(t) + registry.RegisterForTenant(tenantID, newImageCodexProvider("acct-a", serverA.URL)) + + router := NewChatGPTOAuthRouter(tenantID, registry, "acct-a", "round_robin", nil) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel before the call + + _, err := router.GenerateImage(ctx, imageReq) + if err == nil { + t.Fatal("GenerateImage should fail with cancelled context") + } + if !errors.Is(err, context.Canceled) && !strings.Contains(err.Error(), "context canceled") { + t.Fatalf("expected context cancellation error, got: %v", err) + } +} + +// TestChatGPTOAuthRouterImage_RoundRobinAdvancesPerCall verifies that the round-robin +// counter advances once per GenerateImage call (not per member tried), matching Chat semantics. +// With 2 members: call 1 → A, call 2 → B, call 3 → A again. +func TestChatGPTOAuthRouterImage_RoundRobinAdvancesPerCall(t *testing.T) { + tenantID := uuid.New() + registry := NewRegistry(nil) + + var hitsA, hitsB int + serverA := imageTestServer(t, func() string { hitsA++; return imageSSEResponse(b64img) }) + serverB := imageTestServer(t, func() string { hitsB++; return imageSSEResponse(b64img) }) + + registry.RegisterForTenant(tenantID, newImageCodexProvider("acct-a", serverA.URL)) + registry.RegisterForTenant(tenantID, newImageCodexProvider("acct-b", serverB.URL)) + + router := NewChatGPTOAuthRouter(tenantID, registry, "acct-a", "round_robin", []string{"acct-b"}) + + for i := range 3 { + if _, err := router.GenerateImage(context.Background(), imageReq); err != nil { + t.Fatalf("call %d: GenerateImage failed: %v", i, err) + } + } + // A: calls 1 and 3; B: call 2 + if hitsA != 2 { + t.Fatalf("hitsA = %d, want 2", hitsA) + } + if hitsB != 1 { + t.Fatalf("hitsB = %d, want 1", hitsB) + } +} diff --git a/internal/providers/chatgpt_oauth_router_provider_type.go b/internal/providers/chatgpt_oauth_router_provider_type.go new file mode 100644 index 00000000..14806f72 --- /dev/null +++ b/internal/providers/chatgpt_oauth_router_provider_type.go @@ -0,0 +1,9 @@ +package providers + +// ProviderType implements typedProvider for ChatGPTOAuthRouter. +// Returns "chatgpt_oauth" for log/type-routing purposes. +// Image gen path in create_image.go short-circuits on _native_provider type-assert +// before reading _provider_type, so this is cosmetic for image generation. +func (p *ChatGPTOAuthRouter) ProviderType() string { + return "chatgpt_oauth" +} diff --git a/internal/providers/codex.go b/internal/providers/codex.go index 3ed3a8d2..c5626093 100644 --- a/internal/providers/codex.go +++ b/internal/providers/codex.go @@ -56,6 +56,14 @@ func (p *CodexProvider) WithMiddlewares(mws ...RequestMiddleware) *CodexProvider return p } +// WithRetryConfig overrides the default per-provider retry config. Useful for +// tests and for callers that manage retry semantics at a higher layer (e.g. +// the pool router fails over on single-attempt member errors). +func (p *CodexProvider) WithRetryConfig(rc RetryConfig) *CodexProvider { + p.retryConfig = rc + return p +} + func (p *CodexProvider) Name() string { return p.name } func (p *CodexProvider) DefaultModel() string { return p.defaultModel } func (p *CodexProvider) SupportsThinking() bool { return true } diff --git a/internal/providers/providertest/codex.go b/internal/providers/providertest/codex.go new file mode 100644 index 00000000..6a8a5fd1 --- /dev/null +++ b/internal/providers/providertest/codex.go @@ -0,0 +1,22 @@ +// Package providertest exposes constructors for provider types wired for +// fast, deterministic test runs. Not intended for production use. +package providertest + +import "github.com/nextlevelbuilder/goclaw/internal/providers" + +// staticTokenSource always returns a fixed token. +type staticTokenSource struct{ token string } + +func (s *staticTokenSource) Token() (string, error) { return s.token, nil } + +// NewCodexProviderFast returns a *providers.CodexProvider with Attempts=1 so +// that tests exercising router-level failover don't incur the default 3-attempt +// retry latency. +func NewCodexProviderFast(name, apiBase string) *providers.CodexProvider { + return providers.NewCodexProvider( + name, + &staticTokenSource{token: "tok-" + name}, + apiBase, + "gpt-image-2", + ).WithRetryConfig(providers.RetryConfig{Attempts: 1}) +} diff --git a/internal/store/agent_store.go b/internal/store/agent_store.go index 0103a149..8590d2e4 100644 --- a/internal/store/agent_store.go +++ b/internal/store/agent_store.go @@ -3,6 +3,7 @@ package store import ( "context" "encoding/json" + "slices" "strings" "github.com/google/uuid" @@ -343,10 +344,10 @@ type WorkspaceSharingConfig struct { } const ( - ReasoningSourceUnset = "unset" - ReasoningSourceLegacy = "thinking_level" - ReasoningSourceAdvanced = "reasoning" - ReasoningSourceProviderDefault = "provider_default" + ReasoningSourceUnset = "unset" + ReasoningSourceLegacy = "thinking_level" + ReasoningSourceAdvanced = "reasoning" + ReasoningSourceProviderDefault = "provider_default" // Reasoning fallback constants — canonical definitions in providers package. ReasoningFallbackDowngrade = providers.ReasoningFallbackDowngrade ReasoningFallbackDisable = providers.ReasoningFallbackDisable @@ -457,12 +458,19 @@ func (a *AgentData) ParseChatGPTOAuthRouting() *ChatGPTOAuthRoutingConfig { if explicitOverrideMode { overrideMode = normalizeChatGPTOAuthOverrideMode(raw.OverrideMode) } + extraProviderNames := normalizeProviderNames(raw.ExtraProviderNames) + if explicitExtras && extraProviderNames == nil { + extraProviderNames = []string{} + } return &ChatGPTOAuthRoutingConfig{ OverrideMode: overrideMode, Strategy: normalizeChatGPTOAuthStrategy(raw.Strategy), - ExtraProviderNames: normalizeProviderNames(raw.ExtraProviderNames), + ExtraProviderNames: extraProviderNames, } } + if explicitExtras && routing.ExtraProviderNames == nil { + routing.ExtraProviderNames = []string{} + } if explicitOverrideMode { return routing } @@ -471,7 +479,7 @@ func (a *AgentData) ParseChatGPTOAuthRouting() *ChatGPTOAuthRoutingConfig { return routing } routing.OverrideMode = "" - if routing.Strategy == ChatGPTOAuthStrategyPrimaryFirst && len(routing.ExtraProviderNames) == 0 { + if routing.Strategy == ChatGPTOAuthStrategyPriority && len(routing.ExtraProviderNames) == 0 { return nil } return routing @@ -486,7 +494,10 @@ func normalizeChatGPTOAuthRoutingConfig(cfg *ChatGPTOAuthRoutingConfig) *ChatGPT Strategy: normalizeChatGPTOAuthStrategy(cfg.Strategy), ExtraProviderNames: normalizeProviderNames(cfg.ExtraProviderNames), } - if routing.OverrideMode == "" && routing.Strategy == ChatGPTOAuthStrategyPrimaryFirst && len(routing.ExtraProviderNames) == 0 { + if cfg.ExtraProviderNames != nil && routing.ExtraProviderNames == nil { + routing.ExtraProviderNames = []string{} + } + if routing.OverrideMode == "" && routing.Strategy == ChatGPTOAuthStrategyPriority && len(routing.ExtraProviderNames) == 0 { return nil } return routing @@ -514,12 +525,28 @@ func normalizeChatGPTOAuthStrategy(value string) string { } } +func PublicChatGPTOAuthStrategy(value string) string { + if value == ChatGPTOAuthStrategyRoundRobin { + return ChatGPTOAuthStrategyRoundRobin + } + return ChatGPTOAuthStrategyPriority +} + +func PublicChatGPTOAuthRouting(cfg *ChatGPTOAuthRoutingConfig) *ChatGPTOAuthRoutingConfig { + if cfg == nil { + return nil + } + clone := CloneChatGPTOAuthRoutingConfig(cfg) + clone.Strategy = PublicChatGPTOAuthStrategy(clone.Strategy) + return clone +} + func CloneChatGPTOAuthRoutingConfig(cfg *ChatGPTOAuthRoutingConfig) *ChatGPTOAuthRoutingConfig { if cfg == nil { return nil } clone := *cfg - clone.ExtraProviderNames = append([]string(nil), cfg.ExtraProviderNames...) + clone.ExtraProviderNames = slices.Clone(cfg.ExtraProviderNames) return &clone } @@ -538,14 +565,15 @@ func ResolveEffectiveChatGPTOAuthRouting(defaults, agentRouting *ChatGPTOAuthRou } effective.OverrideMode = "" if normalizedDefaults != nil && len(normalizedDefaults.ExtraProviderNames) > 0 { - if effective.Strategy == ChatGPTOAuthStrategyPrimaryFirst && - len(normalizedAgent.ExtraProviderNames) == 0 { - effective.ExtraProviderNames = nil + if normalizedAgent.ExtraProviderNames != nil && + len(normalizedAgent.ExtraProviderNames) == 0 && + effective.Strategy != ChatGPTOAuthStrategyRoundRobin { + effective.ExtraProviderNames = slices.Clone(normalizedAgent.ExtraProviderNames) } else { - effective.ExtraProviderNames = append([]string(nil), normalizedDefaults.ExtraProviderNames...) + effective.ExtraProviderNames = slices.Clone(normalizedDefaults.ExtraProviderNames) } } - if effective.Strategy == ChatGPTOAuthStrategyPrimaryFirst && + if effective.Strategy == ChatGPTOAuthStrategyPriority && len(effective.ExtraProviderNames) == 0 && normalizedAgent.OverrideMode != ChatGPTOAuthOverrideCustom { return nil diff --git a/internal/store/agent_store_test.go b/internal/store/agent_store_test.go index 0a820366..4a143e1e 100644 --- a/internal/store/agent_store_test.go +++ b/internal/store/agent_store_test.go @@ -179,24 +179,39 @@ func TestParseChatGPTOAuthRoutingNormalizesNames(t *testing.T) { } } -func TestParseChatGPTOAuthRoutingFallsBackToManual(t *testing.T) { - agent := &AgentData{ - ChatGPTOAuthRouting: json.RawMessage(`{ - "strategy": "something_else", - "extra_provider_names": ["openai-codex-backup"] - }`), - } +func TestPublicChatGPTOAuthRoutingMigratesLegacyStrategiesToPriorityOrder(t *testing.T) { + for _, tc := range []struct { + name string + strategy string + }{ + {name: "unknown", strategy: "something_else"}, + {name: "manual", strategy: "manual"}, + {name: "primary_first", strategy: "primary_first"}, + } { + t.Run(tc.name, func(t *testing.T) { + agent := &AgentData{ + ChatGPTOAuthRouting: json.RawMessage(`{ + "strategy": "` + tc.strategy + `", + "extra_provider_names": ["openai-codex-backup"] + }`), + } - got := agent.ParseChatGPTOAuthRouting() - if got == nil { - t.Fatal("ParseChatGPTOAuthRouting() = nil, want config") - } - if got.Strategy != ChatGPTOAuthStrategyPrimaryFirst { - t.Fatalf("Strategy = %q, want %q", got.Strategy, ChatGPTOAuthStrategyPrimaryFirst) + got := agent.ParseChatGPTOAuthRouting() + if got == nil { + t.Fatal("ParseChatGPTOAuthRouting() = nil, want config") + } + public := PublicChatGPTOAuthRouting(got) + if public == nil { + t.Fatal("PublicChatGPTOAuthRouting() = nil, want config") + } + if public.Strategy != ChatGPTOAuthStrategyPriority { + t.Fatalf("Strategy = %q, want %q", public.Strategy, ChatGPTOAuthStrategyPriority) + } + }) } } -func TestParseChatGPTOAuthRoutingManualWithoutExtrasPreservesExplicitSingleAccount(t *testing.T) { +func TestPublicChatGPTOAuthRoutingCanonicalizesSingleAccountOverrideToPriorityOrder(t *testing.T) { agent := &AgentData{ ChatGPTOAuthRouting: json.RawMessage(`{ "strategy": "manual", @@ -211,8 +226,15 @@ func TestParseChatGPTOAuthRoutingManualWithoutExtrasPreservesExplicitSingleAccou if got.OverrideMode != ChatGPTOAuthOverrideCustom { t.Fatalf("OverrideMode = %q, want %q", got.OverrideMode, ChatGPTOAuthOverrideCustom) } - if got.Strategy != ChatGPTOAuthStrategyPrimaryFirst { - t.Fatalf("Strategy = %q, want %q", got.Strategy, ChatGPTOAuthStrategyPrimaryFirst) + public := PublicChatGPTOAuthRouting(got) + if public == nil { + t.Fatal("PublicChatGPTOAuthRouting() = nil, want config") + } + if public.Strategy != ChatGPTOAuthStrategyPriority { + t.Fatalf("Strategy = %q, want %q", public.Strategy, ChatGPTOAuthStrategyPriority) + } + if got.ExtraProviderNames == nil { + t.Fatal("ExtraProviderNames = nil, want explicit empty slice preserved") } } @@ -230,8 +252,12 @@ func TestParseChatGPTOAuthRoutingPreservesExplicitInheritMode(t *testing.T) { if got.OverrideMode != ChatGPTOAuthOverrideInherit { t.Fatalf("OverrideMode = %q, want %q", got.OverrideMode, ChatGPTOAuthOverrideInherit) } - if got.Strategy != ChatGPTOAuthStrategyPrimaryFirst { - t.Fatalf("Strategy = %q, want %q", got.Strategy, ChatGPTOAuthStrategyPrimaryFirst) + public := PublicChatGPTOAuthRouting(got) + if public == nil { + t.Fatal("PublicChatGPTOAuthRouting() = nil, want config") + } + if public.Strategy != ChatGPTOAuthStrategyPriority { + t.Fatalf("Strategy = %q, want %q", public.Strategy, ChatGPTOAuthStrategyPriority) } } @@ -291,22 +317,43 @@ func TestResolveEffectiveChatGPTOAuthRoutingAllowsCustomSingleAccountToDisableDe ExtraProviderNames: []string{"codex-work"}, } override := &ChatGPTOAuthRoutingConfig{ - OverrideMode: ChatGPTOAuthOverrideCustom, - Strategy: ChatGPTOAuthStrategyPrimaryFirst, + OverrideMode: ChatGPTOAuthOverrideCustom, + Strategy: ChatGPTOAuthStrategyPriority, + ExtraProviderNames: []string{}, } got := ResolveEffectiveChatGPTOAuthRouting(defaults, override) if got == nil { t.Fatal("ResolveEffectiveChatGPTOAuthRouting() = nil, want config") } - if got.Strategy != ChatGPTOAuthStrategyPrimaryFirst { - t.Fatalf("Strategy = %q, want %q", got.Strategy, ChatGPTOAuthStrategyPrimaryFirst) + if got.Strategy != ChatGPTOAuthStrategyPriority { + t.Fatalf("Strategy = %q, want %q", got.Strategy, ChatGPTOAuthStrategyPriority) } if len(got.ExtraProviderNames) != 0 { t.Fatalf("ExtraProviderNames = %#v, want empty", got.ExtraProviderNames) } } +func TestResolveEffectiveChatGPTOAuthRoutingRoundRobinEmptyExtrasKeepsDefaults(t *testing.T) { + defaults := &ChatGPTOAuthRoutingConfig{ + Strategy: ChatGPTOAuthStrategyRoundRobin, + ExtraProviderNames: []string{"codex-work"}, + } + override := &ChatGPTOAuthRoutingConfig{ + OverrideMode: ChatGPTOAuthOverrideCustom, + Strategy: ChatGPTOAuthStrategyRoundRobin, + ExtraProviderNames: []string{}, + } + + got := ResolveEffectiveChatGPTOAuthRouting(defaults, override) + if got == nil { + t.Fatal("ResolveEffectiveChatGPTOAuthRouting() = nil, want config") + } + if !reflect.DeepEqual(got.ExtraProviderNames, defaults.ExtraProviderNames) { + t.Fatalf("ExtraProviderNames = %#v, want %#v", got.ExtraProviderNames, defaults.ExtraProviderNames) + } +} + func TestResolveEffectiveChatGPTOAuthRoutingKeepsProviderOwnedMembersForStrategyOverride(t *testing.T) { defaults := &ChatGPTOAuthRoutingConfig{ Strategy: ChatGPTOAuthStrategyRoundRobin, diff --git a/internal/tokencount/count_tool_schemas_test.go b/internal/tokencount/count_tool_schemas_test.go index 50224a9e..34605c9e 100644 --- a/internal/tokencount/count_tool_schemas_test.go +++ b/internal/tokencount/count_tool_schemas_test.go @@ -13,7 +13,7 @@ const testModel = "claude-sonnet-4-5-20250929" func smallTool() providers.ToolDefinition { return providers.ToolDefinition{ Type: "function", - Function: providers.ToolFunctionSchema{ + Function: &providers.ToolFunctionSchema{ Name: "get_time", Description: "Returns the current UTC time.", Parameters: map[string]any{"type": "object", "properties": map[string]any{}}, @@ -25,7 +25,7 @@ func smallTool() providers.ToolDefinition { func largeTool(name string) providers.ToolDefinition { return providers.ToolDefinition{ Type: "function", - Function: providers.ToolFunctionSchema{ + Function: &providers.ToolFunctionSchema{ Name: name, Description: "Reads, writes, and appends content to files in the workspace. " + "Supports binary and text modes. Path must be relative to the active workspace root. " + diff --git a/internal/tools/create_image_pool_chain_test.go b/internal/tools/create_image_pool_chain_test.go new file mode 100644 index 00000000..b01b417f --- /dev/null +++ b/internal/tools/create_image_pool_chain_test.go @@ -0,0 +1,321 @@ +package tools + +// Integration tests for pool-failover-before-chain-fallthrough semantics in create_image. +// These tests exercise the full stack: ExecuteWithChain + wrapPoolProvider + +// CreateImageTool.callProvider, wired through a real ChatGPTOAuthRouter backed +// by mock HTTP servers. They prove the contract from issue #1008 is correct: +// pool member failover happens INSIDE the router before the outer chain advances. +// +// Not duplicated here (already covered at unit level): +// - internal/providers/chatgpt_oauth_router_image_test.go — router failover semantics +// - internal/tools/media_provider_chain_pool_test.go — wrapPoolProvider decisions + +import ( + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + + "github.com/google/uuid" + "github.com/nextlevelbuilder/goclaw/internal/providers" + "github.com/nextlevelbuilder/goclaw/internal/providers/providertest" + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// poolImageSSE returns a minimal SSE body that parseNativeImageSSE accepts. +func poolImageSSE(b64data string) string { + return `data: {"type":"response.output_item.done","item":{"type":"image_generation_call","result":"` + + b64data + `","output_format":"png"}}` + "\n\ndata: [DONE]\n" +} + +// poolSSEServer starts a test server that returns a successful image SSE on each request. +func poolSSEServer(t *testing.T, hits *atomic.Int32) *httptest.Server { + t.Helper() + s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + hits.Add(1) + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(poolImageSSE("aW1hZ2VkYXRh"))) // "imagedata" base64 + })) + t.Cleanup(s.Close) + return s +} + +// pool429Server starts a test server that always returns HTTP 429 (retryable). +func pool429Server(t *testing.T, hits *atomic.Int32) *httptest.Server { + t.Helper() + s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + hits.Add(1) + http.Error(w, "rate limited", http.StatusTooManyRequests) + })) + t.Cleanup(s.Close) + return s +} + +// buildPoolChainRegistry creates a registry and registers each CodexProvider under +// both the master tenant (so ExecuteWithChain's Get resolves it) and the given +// tenantID (so ChatGPTOAuthRouter's GetForTenant resolves pool members). +func buildPoolChainRegistry(tenantID uuid.UUID, members ...*providers.CodexProvider) *providers.Registry { + reg := providers.NewRegistry(nil) + for _, p := range members { + reg.Register(p) // master tenant — found by ExecuteWithChain + reg.RegisterForTenant(tenantID, p) // tenant scope — found by router's GetForTenant + } + return reg +} + +// poolBaseChainEntry returns a chain entry pointing at baseProvider. +// Prompt injected via Params so callProvider can build NativeImageRequest. +func poolBaseChainEntry(baseProvider string) MediaProviderEntry { + return MediaProviderEntry{ + Provider: baseProvider, + Model: "gpt-image-2", + Enabled: true, + Timeout: 10, + MaxRetries: 1, + Params: map[string]any{ + "prompt": "integration test image", + "aspect_ratio": "1:1", + }, + } +} + +// fakeFallbackEntry returns a chain entry for a nativeImageProvider-backed fake +// (already defined in create_image_native_path_test.go in this package). +// Using a native fake avoids credential requirements for the fallback slot. +func fakeFallbackEntry(name string) MediaProviderEntry { + return MediaProviderEntry{ + Provider: name, + Model: "fake-model", + Enabled: true, + Timeout: 10, + MaxRetries: 1, + Params: map[string]any{ + "prompt": "integration test image", + "aspect_ratio": "1:1", + }, + } +} + +// --- Scenario 1 --- +// Chain: [Pool(A retryable, B success), Fallback] +// Expected: result from B; Fallback NOT called. +// Proves issue #1008 fix: pool failover is internal to the router, never leaks to outer chain. +func TestCreateImagePoolChain_PoolMemberFailover_FallbackNotCalled(t *testing.T) { + tenantID := uuid.New() + + var hitsA, hitsB atomic.Int32 + serverA := pool429Server(t, &hitsA) + serverB := poolSSEServer(t, &hitsB) + + baseA := providertest.NewCodexProviderFast("pool-a", serverA.URL) + memberB := providertest.NewCodexProviderFast("pool-b", serverB.URL) + baseA.WithRoutingDefaults("round_robin", []string{"pool-b"}) + + reg := buildPoolChainRegistry(tenantID, baseA, memberB) + + // Fallback fake — should NOT be called. + fallback := &nativeImageProvider{ + name: "fallback-fake", + model: "fake-model", + returnData: []byte("fallback-bytes"), + } + reg.Register(fallback) + + ctx := store.WithTenantID(t.Context(), tenantID) + ctx = WithToolWorkspace(ctx, t.TempDir()) + + chain := []MediaProviderEntry{ + poolBaseChainEntry("pool-a"), + fakeFallbackEntry("fallback-fake"), + } + + tool := NewCreateImageTool(reg) + result, err := ExecuteWithChain(ctx, chain, reg, tool.callProvider) + if err != nil { + t.Fatalf("ExecuteWithChain failed: %v", err) + } + if len(result.Data) == 0 { + t.Error("result.Data empty — expected image bytes from pool member B") + } + if hitsA.Load() == 0 { + t.Error("pool member A was never hit (expected 429)") + } + if hitsB.Load() == 0 { + t.Error("pool member B was never hit (expected success)") + } + // Core assertion from issue #1008: fallback must NOT have been called. + if fallback.calledWith != nil { + t.Errorf("fallback provider was called — pool failover to B should prevent chain fallthrough (issue #1008 regression)") + } +} + +// --- Scenario 2 --- +// Chain: [Pool(A fail, B fail), Fallback success] +// Expected: result from Fallback. +func TestCreateImagePoolChain_PoolExhausted_FallsThroughToFallback(t *testing.T) { + tenantID := uuid.New() + + var hitsA, hitsB atomic.Int32 + serverA := pool429Server(t, &hitsA) + serverB := pool429Server(t, &hitsB) + + baseA := providertest.NewCodexProviderFast("pool-a2", serverA.URL) + memberB := providertest.NewCodexProviderFast("pool-b2", serverB.URL) + baseA.WithRoutingDefaults("round_robin", []string{"pool-b2"}) + + reg := buildPoolChainRegistry(tenantID, baseA, memberB) + + fallback := &nativeImageProvider{ + name: "fallback-fake2", + model: "fake-model", + returnData: []byte("fallback-image-bytes"), + } + reg.Register(fallback) + + ctx := store.WithTenantID(t.Context(), tenantID) + ctx = WithToolWorkspace(ctx, t.TempDir()) + + chain := []MediaProviderEntry{ + poolBaseChainEntry("pool-a2"), + fakeFallbackEntry("fallback-fake2"), + } + + tool := NewCreateImageTool(reg) + result, err := ExecuteWithChain(ctx, chain, reg, tool.callProvider) + if err != nil { + t.Fatalf("ExecuteWithChain failed: %v — expected fallback to succeed", err) + } + if len(result.Data) == 0 { + t.Error("result.Data empty — expected bytes from fallback") + } + if hitsA.Load() == 0 { + t.Error("pool member A was not attempted") + } + if hitsB.Load() == 0 { + t.Error("pool member B was not attempted") + } + if fallback.calledWith == nil { + t.Error("fallback was not called — expected chain fallthrough after pool exhausted") + } +} + +// --- Scenario 3 --- +// Chain: [Pool(A,B) exhausted, Fallback also fails] +// Expected: error surfaces; no panic. +func TestCreateImagePoolChain_AllFail_ErrorSurfaces(t *testing.T) { + tenantID := uuid.New() + + var hitsA, hitsB atomic.Int32 + serverA := pool429Server(t, &hitsA) + serverB := pool429Server(t, &hitsB) + + baseA := providertest.NewCodexProviderFast("pool-a3", serverA.URL) + memberB := providertest.NewCodexProviderFast("pool-b3", serverB.URL) + baseA.WithRoutingDefaults("round_robin", []string{"pool-b3"}) + + reg := buildPoolChainRegistry(tenantID, baseA, memberB) + + // Fallback fake that returns an error. + fallbackErr := &nativeImageProvider{ + name: "fallback-fake3", + model: "fake-model", + returnError: errPoolTestFailure, + } + reg.Register(fallbackErr) + + ctx := store.WithTenantID(t.Context(), tenantID) + ctx = WithToolWorkspace(ctx, t.TempDir()) + + chain := []MediaProviderEntry{ + poolBaseChainEntry("pool-a3"), + fakeFallbackEntry("fallback-fake3"), + } + + tool := NewCreateImageTool(reg) + _, err := ExecuteWithChain(ctx, chain, reg, tool.callProvider) + if err == nil { + t.Fatal("expected error when all providers fail, got nil") + } +} + +// errPoolTestFailure is a sentinel error used for fallback failure simulation. +var errPoolTestFailure = &poolChainTestError{msg: "pool integration test: simulated failure"} + +// poolChainTestError is a simple non-retryable error for testing. +type poolChainTestError struct{ msg string } + +func (e *poolChainTestError) Error() string { return e.msg } + +// --- Scenario 4 --- +// Chain: [Pool(A)] — single-member pool, no routing defaults. +// wrapPoolProvider must NOT wrap; callProvider routes directly to A via native path. +func TestCreateImagePoolChain_SingleMemberPool_NoWrapOverhead(t *testing.T) { + tenantID := uuid.New() + + var hitsA atomic.Int32 + serverA := poolSSEServer(t, &hitsA) + + // Solo provider — no WithRoutingDefaults → wrapPoolProvider returns it unchanged. + soloA := providertest.NewCodexProviderFast("solo-a", serverA.URL) + + reg := buildPoolChainRegistry(tenantID, soloA) + + ctx := store.WithTenantID(t.Context(), tenantID) + ctx = WithToolWorkspace(ctx, t.TempDir()) + + chain := []MediaProviderEntry{poolBaseChainEntry("solo-a")} + + tool := NewCreateImageTool(reg) + result, err := ExecuteWithChain(ctx, chain, reg, tool.callProvider) + if err != nil { + t.Fatalf("single-member pool failed: %v", err) + } + if len(result.Data) == 0 { + t.Error("result.Data empty") + } + if hitsA.Load() == 0 { + t.Error("solo member A was not called") + } +} + +// --- Scenario 5 --- +// Chain: [Pool(A,B) round_robin] — 2 calls must hit different members. +// Verifies RR counter advances once per GenerateImage call (not per member tried). +func TestCreateImagePoolChain_RoundRobin_RotatesAcrossTwoCalls(t *testing.T) { + tenantID := uuid.New() + + var hitsA, hitsB atomic.Int32 + serverA := poolSSEServer(t, &hitsA) + serverB := poolSSEServer(t, &hitsB) + + baseA := providertest.NewCodexProviderFast("rr-a", serverA.URL) + memberB := providertest.NewCodexProviderFast("rr-b", serverB.URL) + baseA.WithRoutingDefaults("round_robin", []string{"rr-b"}) + + reg := buildPoolChainRegistry(tenantID, baseA, memberB) + + ctx := store.WithTenantID(t.Context(), tenantID) + ctx = WithToolWorkspace(ctx, t.TempDir()) + + chain := []MediaProviderEntry{poolBaseChainEntry("rr-a")} + tool := NewCreateImageTool(reg) + + for i := range 2 { + result, err := ExecuteWithChain(ctx, chain, reg, tool.callProvider) + if err != nil { + t.Fatalf("call %d: ExecuteWithChain failed: %v", i+1, err) + } + if len(result.Data) == 0 { + t.Errorf("call %d: result.Data empty", i+1) + } + } + + // With round_robin and 2 members, 2 successful calls must each hit a different member. + if hitsA.Load() != 1 { + t.Errorf("hitsA = %d, want 1 (round-robin should spread 2 calls across 2 members)", hitsA.Load()) + } + if hitsB.Load() != 1 { + t.Errorf("hitsB = %d, want 1 (round-robin should spread 2 calls across 2 members)", hitsB.Load()) + } +} diff --git a/internal/tools/media_provider_chain.go b/internal/tools/media_provider_chain.go index 320b4a53..f6a8e992 100644 --- a/internal/tools/media_provider_chain.go +++ b/internal/tools/media_provider_chain.go @@ -12,6 +12,7 @@ import ( "time" "github.com/nextlevelbuilder/goclaw/internal/providers" + "github.com/nextlevelbuilder/goclaw/internal/store" ) // MediaProviderEntry represents a single provider in an ordered fallback chain. @@ -182,6 +183,11 @@ func ExecuteWithChain( continue } + // Wrap Codex pool-base providers in a ChatGPTOAuthRouter so that + // _native_provider delivers pool-aware image generation to callProvider. + // Solo Codex providers (no routing defaults) pass through unchanged. + p = wrapPoolProvider(ctx, registry, entry.Provider, p) + // credentialProvider is optional — providers that don't expose static // credentials (e.g. OAuth-based CodexProvider) pass nil and each // callProvider falls back to using the provider's Chat() API. @@ -342,6 +348,61 @@ func ResolveProviderType(p providers.Provider) string { return providerTypeFromName(p.Name()) } +// wrapPoolProvider inspects the resolved provider and, when it is a +// *providers.CodexProvider whose RoutingDefaults indicate a multi-member pool +// (round_robin or priority_order strategy), wraps it in a *ChatGPTOAuthRouter. +// The router satisfies NativeImageProvider, enabling pool-aware image generation +// inside callProvider without changing any caller of ExecuteWithChain. +// +// Wrap conditions (all must hold): +// 1. resolved is *providers.CodexProvider +// 2. codex.RoutingDefaults() is non-nil +// 3. strategy is round_robin or priority_order (OR extras ≥ 1) +// 4. tenant UUID is present in ctx (uuid.Nil → safe degrade, return original) +// 5. router.HasRegisteredProviders() is true (broken router guard) +// +// Returns resolved unchanged for every other case. +func wrapPoolProvider(ctx context.Context, reg *providers.Registry, entryProvider string, resolved providers.Provider) providers.Provider { + codex, ok := resolved.(*providers.CodexProvider) + if !ok { + return resolved + } + + defaults := codex.RoutingDefaults() + if defaults == nil { + return resolved + } + + // A pool needs at least one extra member to be worth wrapping; with zero + // extras there is nothing to rotate or fail over to, so keep the bare + // CodexProvider (skip router overhead for solo Codex entries). + if len(defaults.ExtraProviderNames) == 0 { + return resolved + } + + tenantID := store.TenantIDFromContext(ctx) + if tenantID.String() == "00000000-0000-0000-0000-000000000000" { + // No tenant in context — cannot build a scoped router safely. + return resolved + } + + router := providers.NewChatGPTOAuthRouter( + tenantID, + reg, + entryProvider, + defaults.Strategy, + defaults.ExtraProviderNames, + ) + + // Guard: if the router cannot resolve any member, injecting it would break + // the image gen path. Fall back to the bare Codex provider. + if !router.HasRegisteredProviders() { + return resolved + } + + return router +} + // providerTypeFromName infers provider type from naming patterns. // Used as fallback when the provider doesn't carry its DB type. func providerTypeFromName(name string) string { diff --git a/internal/tools/media_provider_chain_pool_test.go b/internal/tools/media_provider_chain_pool_test.go new file mode 100644 index 00000000..8ef01c18 --- /dev/null +++ b/internal/tools/media_provider_chain_pool_test.go @@ -0,0 +1,255 @@ +package tools + +import ( + "context" + "testing" + + "github.com/google/uuid" + "github.com/nextlevelbuilder/goclaw/internal/providers" + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// helpers + +func newCodexWithDefaults(name, strategy string, extras []string) *providers.CodexProvider { + p := providers.NewCodexProvider(name, nil, "", "") + if strategy != "" || len(extras) > 0 { + p = p.WithRoutingDefaults(strategy, extras) + } + return p +} + +func mustTenantCtx() context.Context { + return store.WithTenantID(context.Background(), uuid.New()) +} + +func registryWith(providers_ ...*providers.CodexProvider) *providers.Registry { + reg := providers.NewRegistry(nil) + for _, p := range providers_ { + reg.Register(p) + } + return reg +} + +// TestWrapsWhenCodexHasExtras: Codex with round_robin strategy and extra members +// → should wrap to *ChatGPTOAuthRouter. +func TestWrapsWhenCodexHasExtras(t *testing.T) { + base := newCodexWithDefaults("base", "round_robin", []string{"extra1", "extra2"}) + extra1 := newCodexWithDefaults("extra1", "", nil) + extra2 := newCodexWithDefaults("extra2", "", nil) + reg := registryWith(base, extra1, extra2) + + ctx := mustTenantCtx() + got := wrapPoolProvider(ctx, reg, "base", base) + + if _, ok := got.(*providers.ChatGPTOAuthRouter); !ok { + t.Errorf("wrapPoolProvider() = %T, want *providers.ChatGPTOAuthRouter", got) + } +} + +// TestWrapsWhenPriorityOrderWithMembers: Codex with priority_order + extras → wraps. +func TestWrapsWhenPriorityOrderWithMembers(t *testing.T) { + base := newCodexWithDefaults("base", "priority_order", []string{"extra1"}) + extra1 := newCodexWithDefaults("extra1", "", nil) + reg := registryWith(base, extra1) + + ctx := mustTenantCtx() + got := wrapPoolProvider(ctx, reg, "base", base) + + if _, ok := got.(*providers.ChatGPTOAuthRouter); !ok { + t.Errorf("wrapPoolProvider() = %T, want *providers.ChatGPTOAuthRouter", got) + } +} + +// TestDoesNotWrapSoloCodexNilDefaults: Codex with nil RoutingDefaults → no wrap. +func TestDoesNotWrapSoloCodexNilDefaults(t *testing.T) { + // Not calling WithRoutingDefaults → RoutingDefaults() returns nil. + base := providers.NewCodexProvider("base", nil, "", "") + reg := registryWith(base) + + ctx := mustTenantCtx() + got := wrapPoolProvider(ctx, reg, "base", base) + + if got != base { + t.Errorf("wrapPoolProvider() returned %T, want original *CodexProvider (no wrap)", got) + } +} + +// TestDoesNotWrapPrimaryFirstNoExtras: strategy primary_first (not round_robin/priority_order), +// extras empty → returns provider unchanged. +func TestDoesNotWrapPrimaryFirstNoExtras(t *testing.T) { + base := newCodexWithDefaults("base", "primary_first", []string{}) + reg := registryWith(base) + + ctx := mustTenantCtx() + got := wrapPoolProvider(ctx, reg, "base", base) + + if got != base { + t.Errorf("wrapPoolProvider() with primary_first + no extras: want original provider, got %T", got) + } +} + +// TestDoesNotWrapNonCodex: non-Codex provider (byteplus style) → unchanged. +func TestDoesNotWrapNonCodex(t *testing.T) { + reg := providers.NewRegistry(nil) + fake := &fakeNonCodexProvider{name: "byteplus"} + reg.Register(fake) + + ctx := mustTenantCtx() + got := wrapPoolProvider(ctx, reg, "byteplus", fake) + + if got != fake { + t.Errorf("wrapPoolProvider() returned %T, want original non-Codex provider unchanged", got) + } +} + +// TestFallsBackWhenRouterHasNoRegisteredMembers: extras reference missing providers +// → router has no registered members → return original Codex. +func TestFallsBackWhenRouterHasNoRegisteredMembers(t *testing.T) { + // Only base registered; extra1 and extra2 are NOT in registry. + base := newCodexWithDefaults("base", "round_robin", []string{"missing1", "missing2"}) + reg := registryWith(base) + + ctx := mustTenantCtx() + got := wrapPoolProvider(ctx, reg, "base", base) + + // Router should fall back because HasRegisteredProviders() returns false + // (only base is registered as member, extras missing — router counts base + extras + // but can't resolve extras → members list = [base alone], which IS > 0). + // Per spec: if router.HasRegisteredProviders() == false → return original. + // With base registered and extras missing, registeredProviders() returns [base], + // so HasRegisteredProviders() == true → we get a router. Adjust test to check + // that wrap happens only when extras are actually resolvable (spec says ≥1 extra): + // Since base alone resolves but extra members don't, router.HasRegisteredProviders() + // is true (base is a member). The phase spec says "Wrapped router's + // HasRegisteredProviders() false → return resolved (don't inject broken router)." + // In this case it's NOT false (base resolves as self-member). Router is returned. + // This test verifies the fallback only when zero members resolve. + if _, ok := got.(*providers.ChatGPTOAuthRouter); !ok { + // When extras are missing but base itself is a Codex in the registry, + // HasRegisteredProviders() is true (base counts as a member). + // The router IS valid here, so a router is expected. + t.Errorf("wrapPoolProvider() with missing extras but base present: got %T, want *ChatGPTOAuthRouter (base self-resolves)", got) + } +} + +// TestFallsBackWhenZeroMembersResolve: verifies we return original when the +// router genuinely has NO registered members. +func TestFallsBackWhenZeroMembersResolve(t *testing.T) { + // base NOT registered in registry; extras also missing. + // We pass the base provider directly to wrapPoolProvider but don't register + // it, so GetForTenant won't find it as a Codex — however the router looks up + // the default + extras from the registry, not from the passed provider. + base := newCodexWithDefaults("ghost", "round_robin", []string{"missing1"}) + reg := providers.NewRegistry(nil) // empty registry — nothing registered + + ctx := mustTenantCtx() + got := wrapPoolProvider(ctx, reg, "ghost", base) + + // Router created but HasRegisteredProviders() == false → must return original. + if got != base { + t.Errorf("wrapPoolProvider() with empty registry: want original provider, got %T", got) + } +} + +// TestNoTenantInContext_ReturnsCodex: missing tenant in ctx → safe degrade → original provider. +func TestNoTenantInContext_ReturnsCodex(t *testing.T) { + base := newCodexWithDefaults("base", "round_robin", []string{"extra1"}) + extra1 := newCodexWithDefaults("extra1", "", nil) + reg := registryWith(base, extra1) + + // No tenant in context → TenantIDFromContext returns uuid.Nil. + ctx := context.Background() + got := wrapPoolProvider(ctx, reg, "base", base) + + if got != base { + t.Errorf("wrapPoolProvider() without tenant ctx: want original provider (safe degrade), got %T", got) + } +} + +// TestWrappedRouterSatisfiesNativeImageProvider: wrapped result can be +// type-asserted to NativeImageProvider. +func TestWrappedRouterSatisfiesNativeImageProvider(t *testing.T) { + base := newCodexWithDefaults("base", "round_robin", []string{"extra1"}) + extra1 := newCodexWithDefaults("extra1", "", nil) + reg := registryWith(base, extra1) + + ctx := mustTenantCtx() + got := wrapPoolProvider(ctx, reg, "base", base) + + if _, ok := got.(providers.NativeImageProvider); !ok { + t.Errorf("wrapPoolProvider() result %T does not satisfy NativeImageProvider", got) + } +} + +// TestParamsInjection: _native_provider in ExecuteWithChain callParams is the +// *ChatGPTOAuthRouter, not the bare *CodexProvider. +func TestParamsInjection(t *testing.T) { + base := newCodexWithDefaults("pool_base", "round_robin", []string{"extra1"}) + extra1 := newCodexWithDefaults("extra1", "", nil) + reg := registryWith(base, extra1) + tenantID := uuid.New() + reg.RegisterForTenant(tenantID, base) + reg.RegisterForTenant(tenantID, extra1) + + ctx := store.WithTenantID(context.Background(), tenantID) + + chain := []MediaProviderEntry{{ + Provider: "pool_base", + Model: "gpt-image-1", + Enabled: true, + Timeout: 10, + MaxRetries: 1, + }} + + // capturedNative captures whatever _native_provider lands in callParams. + var capturedNative interface{} + fn := func(fnCtx context.Context, cp credentialProvider, providerName, model string, params map[string]any) ([]byte, *providers.Usage, error) { + capturedNative = params["_native_provider"] + return []byte("ok"), nil, nil + } + + _, err := ExecuteWithChain(ctx, chain, reg, fn) + if err != nil { + t.Fatalf("ExecuteWithChain returned error: %v", err) + } + + if _, ok := capturedNative.(*providers.ChatGPTOAuthRouter); !ok { + t.Errorf("_native_provider = %T, want *providers.ChatGPTOAuthRouter", capturedNative) + } +} + +// TestStrategyPassedThrough: verifies round_robin strategy is preserved in the +// router by checking the router is created (strategy is opaque; tested indirectly +// by confirming the router forms from round_robin vs priority_order inputs). +func TestStrategyPassedThrough(t *testing.T) { + for _, strategy := range []string{"round_robin", "priority_order"} { + t.Run(strategy, func(t *testing.T) { + base := newCodexWithDefaults("base", strategy, []string{"extra1"}) + extra1 := newCodexWithDefaults("extra1", "", nil) + reg := registryWith(base, extra1) + + ctx := mustTenantCtx() + got := wrapPoolProvider(ctx, reg, "base", base) + + if _, ok := got.(*providers.ChatGPTOAuthRouter); !ok { + t.Errorf("strategy %q: wrapPoolProvider() = %T, want *ChatGPTOAuthRouter", strategy, got) + } + }) + } +} + +// fakeNonCodexProvider is a minimal non-Codex provider for testing. +// Implements only the providers.Provider interface — no Codex-specific methods. +type fakeNonCodexProvider struct { + name string +} + +func (f *fakeNonCodexProvider) Name() string { return f.name } +func (f *fakeNonCodexProvider) DefaultModel() string { return "" } +func (f *fakeNonCodexProvider) Chat(_ context.Context, _ providers.ChatRequest) (*providers.ChatResponse, error) { + return nil, nil +} +func (f *fakeNonCodexProvider) ChatStream(_ context.Context, _ providers.ChatRequest, _ func(providers.StreamChunk)) (*providers.ChatResponse, error) { + return nil, nil +} diff --git a/tests/integration/mcp_grant_revoke_test.go b/tests/integration/mcp_grant_revoke_test.go index c236acac..5eb3bae0 100644 --- a/tests/integration/mcp_grant_revoke_test.go +++ b/tests/integration/mcp_grant_revoke_test.go @@ -9,9 +9,9 @@ import ( "sync/atomic" "testing" + "github.com/google/uuid" mcpclient "github.com/mark3labs/mcp-go/client" mcpgo "github.com/mark3labs/mcp-go/mcp" - "github.com/google/uuid" "github.com/nextlevelbuilder/goclaw/internal/mcp" "github.com/nextlevelbuilder/goclaw/internal/store" diff --git a/tests/integration/tts_gemini_live_test.go b/tests/integration/tts_gemini_live_test.go index 316d29d0..333e14b1 100644 --- a/tests/integration/tts_gemini_live_test.go +++ b/tests/integration/tts_gemini_live_test.go @@ -87,6 +87,6 @@ func isServerError(err error) bool { if err == nil { return false } - msg := err.Error() + msg := strings.ToLower(err.Error()) return strings.Contains(msg, "500") || strings.Contains(msg, "503") || strings.Contains(msg, "server error") } diff --git a/ui/web/src/pages/agents/agent-detail/agent-display-utils.ts b/ui/web/src/pages/agents/agent-detail/agent-display-utils.ts index 69d1dce5..a128833a 100644 --- a/ui/web/src/pages/agents/agent-detail/agent-display-utils.ts +++ b/ui/web/src/pages/agents/agent-detail/agent-display-utils.ts @@ -27,6 +27,7 @@ export interface NormalizedChatGPTOAuthRouting { overrideMode: ChatGPTOAuthRoutingOverrideMode; strategy: EffectiveChatGPTOAuthRoutingStrategy; extraProviderNames: string[]; + hasExplicitExtraProviderNames: boolean; } export interface EffectiveChatGPTOAuthRouting { @@ -73,8 +74,9 @@ export function normalizeChatGPTOAuthRouting( return { isExplicit: false, overrideMode: "custom", - strategy: "primary_first", + strategy: "priority_order", extraProviderNames: [], + hasExplicitExtraProviderNames: false, }; } const routing = raw as Record; @@ -96,13 +98,14 @@ export function normalizeChatGPTOAuthRouting( routing.override_mode === "custom" || hasStrategyField || hasExtraProviderField || - strategy !== "primary_first" || + strategy !== "priority_order" || extraProviderNames.length > 0; return { isExplicit, overrideMode, strategy, extraProviderNames, + hasExplicitExtraProviderNames: hasExtraProviderField, }; } @@ -111,7 +114,10 @@ export function hasActiveChatGPTOAuthRouting( routing?: ChatGPTOAuthRoutingConfig | Record | null, ): boolean { const normalized = normalizeChatGPTOAuthRouting(routing); - return normalized.isExplicit && (normalized.strategy !== "primary_first" || normalized.extraProviderNames.length > 0); + return normalized.isExplicit && ( + normalized.strategy === "round_robin" || + normalized.extraProviderNames.length > 0 + ); } export function normalizeChatGPTOAuthRoutingInput( @@ -121,8 +127,9 @@ export function normalizeChatGPTOAuthRoutingInput( return { isExplicit: false, overrideMode: "custom", - strategy: "primary_first", + strategy: "priority_order", extraProviderNames: [], + hasExplicitExtraProviderNames: false, }; } return normalizeChatGPTOAuthRouting(routing); @@ -139,8 +146,9 @@ export function resolveEffectiveChatGPTOAuthRouting( ({ isExplicit: false, overrideMode: "custom", - strategy: "primary_first", + strategy: "priority_order", extraProviderNames: [], + hasExplicitExtraProviderNames: false, } satisfies NormalizedChatGPTOAuthRouting); let source: EffectiveChatGPTOAuthRouting["source"] = "single"; @@ -150,7 +158,7 @@ export function resolveEffectiveChatGPTOAuthRouting( if (normalizedAgent.overrideMode === "inherit") { source = providerDefaults ? "provider_default" : "single"; - strategy = providerDefaults?.strategy ?? "primary_first"; + strategy = providerDefaults?.strategy ?? "priority_order"; extraProviderNames = providerDefaults?.extraProviderNames ?? []; overrideMode = "inherit"; } else if (normalizedAgent.isExplicit) { @@ -167,7 +175,7 @@ export function resolveEffectiveChatGPTOAuthRouting( providerDefaults?.extraProviderNames.length && source === "agent_custom" ) { - if (strategy === "primary_first" && extraProviderNames.length === 0) { + if (normalizedAgent.hasExplicitExtraProviderNames && extraProviderNames.length === 0) { extraProviderNames = []; } else { extraProviderNames = providerDefaults.extraProviderNames; @@ -190,8 +198,7 @@ export function strategyLabelKey( strategy: EffectiveChatGPTOAuthRoutingStrategy, ): string { if (strategy === "round_robin") return "chatgptOAuthRouting.strategy.roundRobin"; - if (strategy === "priority_order") return "chatgptOAuthRouting.strategy.priorityOrder"; - return "chatgptOAuthRouting.strategy.primaryFirst"; + return "chatgptOAuthRouting.strategy.priorityOrder"; } /** Maps route readiness state to badge variant. */ @@ -252,18 +259,13 @@ export function buildAgentOtherConfigWithChatGPTOAuthRouting( if ( providerDefaults || normalized.isExplicit || - normalized.strategy !== "primary_first" || normalized.extraProviderNames.length > 0 ) { const customRouting: Record = { override_mode: "custom", strategy: normalized.strategy, }; - if ( - !providerDefaults || - (normalized.strategy === "primary_first" && - normalized.extraProviderNames.length === 0) - ) { + if (normalized.hasExplicitExtraProviderNames || normalized.extraProviderNames.length > 0) { customRouting.extra_provider_names = normalized.extraProviderNames; } result.chatgpt_oauth_routing = customRouting; diff --git a/ui/web/src/pages/agents/agent-detail/codex-pool-routing-draft-utils.ts b/ui/web/src/pages/agents/agent-detail/codex-pool-routing-draft-utils.ts index be00fd0e..120006cd 100644 --- a/ui/web/src/pages/agents/agent-detail/codex-pool-routing-draft-utils.ts +++ b/ui/web/src/pages/agents/agent-detail/codex-pool-routing-draft-utils.ts @@ -8,16 +8,19 @@ export function buildDraftRouting( savedRouting: NormalizedChatGPTOAuthRouting, ): ChatGPTOAuthRoutingConfig { if (savedRouting.isExplicit) { - return { + const draft: ChatGPTOAuthRoutingConfig = { override_mode: savedRouting.overrideMode, strategy: savedRouting.strategy, - extra_provider_names: savedRouting.extraProviderNames, }; + if (savedRouting.hasExplicitExtraProviderNames || savedRouting.extraProviderNames.length > 0) { + draft.extra_provider_names = savedRouting.extraProviderNames; + } + return draft; } return { override_mode: "inherit", - strategy: "primary_first", + strategy: "priority_order", extra_provider_names: [], }; } @@ -33,5 +36,6 @@ export function routingDraftSignature( override_mode: "custom", strategy: normalized.strategy, extra_provider_names: normalized.extraProviderNames, + has_explicit_extra_provider_names: normalized.hasExplicitExtraProviderNames, }); } diff --git a/ui/web/src/pages/agents/agent-detail/config-sections/chatgpt-oauth-routing-section.tsx b/ui/web/src/pages/agents/agent-detail/config-sections/chatgpt-oauth-routing-section.tsx index 29bc7a74..b303754b 100644 --- a/ui/web/src/pages/agents/agent-detail/config-sections/chatgpt-oauth-routing-section.tsx +++ b/ui/web/src/pages/agents/agent-detail/config-sections/chatgpt-oauth-routing-section.tsx @@ -125,10 +125,17 @@ export function ChatGPTOAuthRoutingSection({ const blockedEntries = selectedEntries.filter((e) => e.routeReadiness === "blocked"); const routerActiveEntries = healthyEntries; - const selectedStrategy: EffectiveChatGPTOAuthRoutingStrategy = + // When the agent inherits from the provider, paint the Traffic Policy + // buttons with the provider's effective strategy so the UI reflects what + // will actually run. Otherwise derive from the draft (custom override). + const draftStrategy: EffectiveChatGPTOAuthRoutingStrategy = value.strategy === "round_robin" || value.strategy === "priority_order" ? value.strategy - : "primary_first"; + : "priority_order"; + const selectedStrategy: EffectiveChatGPTOAuthRoutingStrategy = + mode === "inherit" && defaultRouting + ? defaultRouting.strategy + : draftStrategy; const canEditMembership = canManageProviders && membershipEditable; const canUsePoolStrategies = canManageProviders && @@ -243,16 +250,7 @@ export function ChatGPTOAuthRoutingSection({

{t("chatgptOAuthRouting.strategyLabel")}

-
- +