From ed6a28b65e8e622d1c410cae905c5d844999c252 Mon Sep 17 00:00:00 2001 From: Charlie-1909 <283177329+Charlie-1909@users.noreply.github.com> Date: Thu, 20 Aug 2026 22:24:23 +0200 Subject: [PATCH 1/3] fix(codex): avoid creating an empty default provider --- .../settings/acp-agent-settings.test.tsx | 110 +++++++++++++- .../settings/acp-agent-settings.tsx | 137 ++++++++++-------- 2 files changed, 179 insertions(+), 68 deletions(-) diff --git a/src/components/settings/acp-agent-settings.test.tsx b/src/components/settings/acp-agent-settings.test.tsx index 460fd36b9d..8b47f006ad 100644 --- a/src/components/settings/acp-agent-settings.test.tsx +++ b/src/components/settings/acp-agent-settings.test.tsx @@ -1217,6 +1217,106 @@ describe("materializeClaudeHardeningFlags — save-time toggle defaults", () => }) }) +describe("patchCodexConfigTomlText — issue #520 provider creation", () => { + function importantProviderShape(configTomlText: string) { + return parseTomlDocument(configTomlText) as { + model_provider?: string + model_providers?: Record> + features?: { responses_websockets_v2?: boolean } + } + } + + it("does not create a provider for an empty API base URL", () => { + const parsed = importantProviderShape( + patchCodexConfigTomlText("", { apiBaseUrl: "" }) + ) + expect(parsed.model_provider).toBeUndefined() + expect(parsed.model_providers).toBeUndefined() + }) + + it("updates only the global WebSocket feature without a provider", () => { + const parsed = importantProviderShape( + patchCodexConfigTomlText("", { + modelProvider: "codeg", + supportsWebsockets: true, + }) + ) + expect(parsed.model_provider).toBeUndefined() + expect(parsed.model_providers).toBeUndefined() + expect(parsed.features?.responses_websockets_v2).toBe(true) + }) + + it("does not materialize the synthetic codeg provider", () => { + const parsed = importantProviderShape( + patchCodexConfigTomlText("", { modelProvider: "codeg" }) + ) + expect(parsed.model_provider).toBeUndefined() + expect(parsed.model_providers).toBeUndefined() + }) + + it("creates the managed provider for a non-empty API base URL", () => { + const parsed = importantProviderShape( + patchCodexConfigTomlText("", { + modelProvider: "codeg", + apiBaseUrl: "https://new.example/v1", + }) + ) + expect(parsed.model_provider).toBe("codeg") + expect(parsed.model_providers?.codeg).toMatchObject({ + base_url: "https://new.example/v1", + name: "codeg", + wire_api: "responses", + requires_openai_auth: true, + }) + }) + + it("updates the already selected provider without replacing it", () => { + const result = patchCodexConfigTomlText( + [ + 'model_provider = "custom"', + "", + "[model_providers.custom]", + 'base_url = "https://old.example/v1"', + 'keep = "user-value"', + ].join("\n"), + { apiBaseUrl: "https://new.example/v1" } + ) + const parsed = importantProviderShape(result) + expect(parsed.model_provider).toBe("custom") + expect(parsed.model_providers?.custom).toMatchObject({ + base_url: "https://new.example/v1", + keep: "user-value", + }) + expect(parsed.model_providers?.codeg).toBeUndefined() + }) + + it("switches to an explicitly requested provider with a non-empty URL", () => { + const result = patchCodexConfigTomlText( + [ + 'model_provider = "custom"', + "", + "[model_providers.custom]", + 'base_url = "https://old.example/v1"', + 'keep = "user-value"', + ].join("\n"), + { + modelProvider: "codeg", + apiBaseUrl: "https://new.example/v1", + } + ) + const parsed = importantProviderShape(result) + expect(parsed.model_provider).toBe("codeg") + expect(parsed.model_providers?.codeg).toMatchObject({ + base_url: "https://new.example/v1", + name: "codeg", + wire_api: "responses", + }) + expect(parsed.model_providers?.custom).toMatchObject({ + keep: "user-value", + }) + }) +}) + describe("patchCodexConfigTomlText — codeg's requires_openai_auth default", () => { /** Read `model_providers.codeg.requires_openai_auth` back out of a result. */ function authFlagOf(configTomlText: string): boolean | undefined { @@ -1265,10 +1365,6 @@ describe("patchCodexConfigTomlText — codeg's requires_openai_auth default", () ).toBe(true) }) - it("seeds a brand-new provider from an empty config", () => { - expect(authFlagOf(patchCodexConfigTomlText("", patch))).toBe(true) - }) - it("stands down for a provider using actor authorization", () => { const toml = [ BOUND_PROVIDER, @@ -1285,6 +1381,12 @@ describe("patchCodexConfigTomlText — codeg's requires_openai_auth default", () const ENTRY = ENTRY_POINTS[0].patch + it("seeds a brand-new provider from a non-empty API base URL", () => { + expect( + authFlagOf(patchCodexConfigTomlText("", ENTRY_POINTS[0].patch)) + ).toBe(true) + }) + it("preserves user comments around the managed provider", () => { const toml = [ "# my hand-written codex config", diff --git a/src/components/settings/acp-agent-settings.tsx b/src/components/settings/acp-agent-settings.tsx index 4439af1080..bbdc68c78b 100644 --- a/src/components/settings/acp-agent-settings.tsx +++ b/src/components/settings/acp-agent-settings.tsx @@ -2890,29 +2890,53 @@ function patchCodexAuthJsonText( } } +interface CodexConfigTomlPatch { + apiBaseUrl?: string + model?: string + modelProvider?: string + modelReasoningEffort?: string + supportsWebsockets?: boolean + skills?: boolean + serviceTierFast?: boolean +} + +/** + * Resolve which provider a patch should write to, distinguishing a real + * selection from the `codeg` display fallback (issue #520). Returns "" when + * no provider should be created or touched at all. + */ +function resolveCodexProviderForPatch( + configTomlText: string, + patch: CodexConfigTomlPatch +): string { + const configuredProvider = + extractCodexTomlImportantValues(configTomlText).modelProvider.trim() + const requestedProvider = patch.modelProvider?.trim() ?? "" + const hasApiBaseUrl = + typeof patch.apiBaseUrl === "string" && patch.apiBaseUrl.trim() !== "" + if (hasApiBaseUrl && requestedProvider) return requestedProvider + if (configuredProvider) return configuredProvider + if (hasApiBaseUrl) return CODEX_DEFAULT_MODEL_PROVIDER + return "" +} + export function patchCodexConfigTomlText( configTomlText: string, - patch: { - apiBaseUrl?: string - model?: string - modelProvider?: string - modelReasoningEffort?: string - supportsWebsockets?: boolean - skills?: boolean - serviceTierFast?: boolean - } + patch: CodexConfigTomlPatch ): string { let nextTomlText = configTomlText - if (typeof patch.modelProvider === "string") { - const modelProvider = patch.modelProvider.trim() - if (modelProvider) { - nextTomlText = updateTomlRootStringKey( - nextTomlText, - "model_provider", - modelProvider - ) - nextTomlText = ensureCodexProviderDefaults(nextTomlText, modelProvider) - } + const initialProvider = + extractCodexTomlImportantValues(configTomlText).modelProvider.trim() + const provider = resolveCodexProviderForPatch(configTomlText, patch) + if (provider && provider !== initialProvider) { + nextTomlText = updateTomlRootStringKey( + nextTomlText, + "model_provider", + provider + ) + } + if (typeof patch.modelProvider === "string" && provider) { + nextTomlText = ensureCodexProviderDefaults(nextTomlText, provider) } if (typeof patch.model === "string") { nextTomlText = updateTomlRootStringKey(nextTomlText, "model", patch.model) @@ -2927,46 +2951,22 @@ export function patchCodexConfigTomlText( reasoningEffort ) } - if (typeof patch.apiBaseUrl === "string") { - const tomlValues = extractCodexTomlImportantValues(nextTomlText) - const modelProvider = - patch.modelProvider?.trim() || - tomlValues.modelProvider.trim() || - CODEX_DEFAULT_MODEL_PROVIDER - if (!tomlValues.modelProvider.trim() && patch.apiBaseUrl.trim()) { - nextTomlText = updateTomlRootStringKey( - nextTomlText, - "model_provider", - modelProvider - ) - } + if (typeof patch.apiBaseUrl === "string" && provider) { nextTomlText = patchCodexProviderBaseUrl( nextTomlText, - modelProvider, + provider, patch.apiBaseUrl ) - nextTomlText = ensureCodexProviderDefaults(nextTomlText, modelProvider) - } - if (typeof patch.supportsWebsockets === "boolean") { - const tomlValues = extractCodexTomlImportantValues(nextTomlText) - const modelProvider = - patch.modelProvider?.trim() || - tomlValues.modelProvider.trim() || - CODEX_DEFAULT_MODEL_PROVIDER - if (!tomlValues.modelProvider.trim()) { - nextTomlText = updateTomlRootStringKey( - nextTomlText, - "model_provider", - modelProvider - ) - } + nextTomlText = ensureCodexProviderDefaults(nextTomlText, provider) + } + if (typeof patch.supportsWebsockets === "boolean" && provider) { nextTomlText = patchCodexProviderField( nextTomlText, - modelProvider, + provider, "supports_websockets", `supports_websockets = ${patch.supportsWebsockets ? "true" : "false"}` ) - nextTomlText = ensureCodexProviderDefaults(nextTomlText, modelProvider) + nextTomlText = ensureCodexProviderDefaults(nextTomlText, provider) } const normalizedTomlValues = extractCodexTomlImportantValues(nextTomlText) if (normalizedTomlValues.model.trim()) { @@ -2981,17 +2981,25 @@ export function patchCodexConfigTomlText( "model_reasoning_effort", normalizedTomlValues.modelReasoningEffort ) - const activeProvider = - normalizedTomlValues.modelProvider.trim() || CODEX_DEFAULT_MODEL_PROVIDER - const shouldEnableFeature = Boolean( - normalizedTomlValues.providerSupportsWebsockets[activeProvider] - ) - nextTomlText = upsertTomlSectionBooleanKey( - nextTomlText, - "features", - "responses_websockets_v2", - shouldEnableFeature ? true : null - ) + const activeProvider = normalizedTomlValues.modelProvider.trim() + if (activeProvider) { + const shouldEnableFeature = Boolean( + normalizedTomlValues.providerSupportsWebsockets[activeProvider] + ) + nextTomlText = upsertTomlSectionBooleanKey( + nextTomlText, + "features", + "responses_websockets_v2", + shouldEnableFeature ? true : null + ) + } else if (typeof patch.supportsWebsockets === "boolean") { + nextTomlText = upsertTomlSectionBooleanKey( + nextTomlText, + "features", + "responses_websockets_v2", + patch.supportsWebsockets ? true : null + ) + } if (typeof patch.skills === "boolean") { nextTomlText = upsertTomlSectionBooleanKey( nextTomlText, @@ -5918,7 +5926,7 @@ export function AcpAgentSettings() { codexModelList: codexList, codexAuthJsonText: nextAuthJsonText, codexConfigTomlText: nextConfigTomlText, - codexModelProvider: CODEX_DEFAULT_MODEL_PROVIDER, + codexModelProvider: synced.modelProvider, codexProviderOptions: synced.providerOptions, envText: patchEnvText(current.envText, { OPENAI_API_KEY: apiKey, @@ -7056,7 +7064,8 @@ export function AcpAgentSettings() { return } - // "api_key" or "model_provider": ensure model_provider = "codeg" in toml + // "api_key" or "model_provider": keep any already-selected provider; + // the "codeg" fallback is a display value, not a request to create one. const nextConfigTomlText = patchCodexConfigTomlText( selectedDraft.codexConfigTomlText, { modelProvider: CODEX_DEFAULT_MODEL_PROVIDER } @@ -7080,7 +7089,7 @@ export function AcpAgentSettings() { apiBaseUrl: synced.apiBaseUrl, apiKey: synced.apiKey ?? current.apiKey, model: synced.model, - codexModelProvider: CODEX_DEFAULT_MODEL_PROVIDER, + codexModelProvider: synced.modelProvider, codexProviderOptions: synced.providerOptions, codexReasoningEffort: synced.reasoningEffort, codexSupportsWebsockets: synced.supportsWebsockets, From c185df0607ccc74d230c6e99ca5f1edf8365f32c Mon Sep 17 00:00:00 2001 From: Charlie-1909 <283177329+Charlie-1909@users.noreply.github.com> Date: Thu, 20 Aug 2026 22:25:16 +0200 Subject: [PATCH 2/3] test(codex): cover providerless config persistence --- src-tauri/src/commands/acp.rs | 83 +++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/src-tauri/src/commands/acp.rs b/src-tauri/src/commands/acp.rs index bf2ad04631..4fba9b77ff 100644 --- a/src-tauri/src/commands/acp.rs +++ b/src-tauri/src/commands/acp.rs @@ -14874,6 +14874,89 @@ wire_api = "chat" }); } + #[test] + fn codex_persist_keeps_a_providerless_config_providerless() { + let dir = tempfile::tempdir().expect("tempdir"); + temp_env::with_var("CODEX_HOME", Some(dir.path()), || { + let patch = serde_json::json!({ + "apiBaseUrl": "", + "apiKey": "", + "model": "gpt-5-codex", + "env": { "KEEP": "1" } + }) + .to_string(); + persist_codex_local_config(Some(&patch)).expect("persist must succeed"); + + let written = std::fs::read_to_string(dir.path().join("config.toml")) + .expect("read back"); + let parsed = written.parse::().expect("must parse"); + let root = parsed.as_table().expect("root table"); + assert_eq!(root.get("model").and_then(toml::Value::as_str), Some("gpt-5-codex")); + assert!(!root.contains_key("model_provider")); + assert!(!root.contains_key("model_providers")); + }); + } + + #[test] + fn codex_persist_does_not_activate_a_dormant_codeg_table() { + let dir = tempfile::tempdir().expect("tempdir"); + temp_env::with_var("CODEX_HOME", Some(dir.path()), || { + let config_path = dir.path().join("config.toml"); + std::fs::write( + &config_path, + "[model_providers.codeg]\nbase_url = \"\"\nkeep = \"user-value\"\n", + ) + .expect("seed config.toml"); + let patch = serde_json::json!({ + "apiBaseUrl": "", + "apiKey": "", + "model": "gpt-5-codex", + "env": {} + }) + .to_string(); + + persist_codex_local_config(Some(&patch)).expect("persist must succeed"); + + let written = std::fs::read_to_string(&config_path).expect("read back"); + let parsed = written.parse::().expect("must parse"); + let root = parsed.as_table().expect("root table"); + assert!(!root.contains_key("model_provider")); + let codeg = root + .get("model_providers") + .and_then(toml::Value::as_table) + .and_then(|providers| providers.get("codeg")) + .and_then(toml::Value::as_table) + .expect("dormant codeg table must survive"); + assert_eq!(codeg.get("keep").and_then(toml::Value::as_str), Some("user-value")); + assert!(!codeg.contains_key("requires_openai_auth")); + assert!(!codeg.contains_key("wire_api")); + }); + } + + #[test] + fn codex_cascade_keeps_a_providerless_config_providerless() { + let dir = tempfile::tempdir().expect("tempdir"); + temp_env::with_var("CODEX_HOME", Some(dir.path()), || { + cascade_update_agent_config( + AgentType::Codex, + "", + "", + &BTreeMap::new(), + &CodexModelAction::Set("gpt-5-codex".to_string()), + None, + ) + .expect("cascade must succeed"); + + let written = std::fs::read_to_string(dir.path().join("config.toml")) + .expect("read back"); + let parsed = written.parse::().expect("must parse"); + let root = parsed.as_table().expect("root table"); + assert_eq!(root.get("model").and_then(toml::Value::as_str), Some("gpt-5-codex")); + assert!(!root.contains_key("model_provider")); + assert!(!root.contains_key("model_providers")); + }); + } + #[test] fn cursor_fingerprint_tracks_cli_config_changes() { // Cursor's ~/.cursor/cli-config.json (permission rules / sandbox) is From 17d3ebf75cd2b5de0ca92bbbb9e244407d44334d Mon Sep 17 00:00:00 2001 From: Charlie-1909 <283177329+Charlie-1909@users.noreply.github.com> Date: Thu, 20 Aug 2026 22:27:35 +0200 Subject: [PATCH 3/3] fix(codex): skip provider writes without an API URL --- src-tauri/src/commands/acp.rs | 194 +++++++++++++++++++--------------- 1 file changed, 109 insertions(+), 85 deletions(-) diff --git a/src-tauri/src/commands/acp.rs b/src-tauri/src/commands/acp.rs index 4fba9b77ff..a6583abf70 100644 --- a/src-tauri/src/commands/acp.rs +++ b/src-tauri/src/commands/acp.rs @@ -2630,6 +2630,31 @@ fn ensure_codex_provider_auth_default(provider_table: &mut toml::map::Map, + api_base_url: Option<&str>, +) -> Option { + table + .get("model_provider") + .and_then(toml::Value::as_str) + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(str::to_string) + .or_else(|| { + api_base_url + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(|_| "codeg".to_string()) + }) +} + /// OpenCode reads config from `$XDG_CONFIG_HOME/opencode` (falling back to /// `~/.config/opencode`) and credentials from `$XDG_DATA_HOME/opencode` /// (falling back to `~/.local/share/opencode`) on every platform. codeg must @@ -3232,52 +3257,53 @@ fn persist_codex_local_config(config_patch_json: Option<&str>) -> Result<(), Acp } } - let provider_name = table - .get("model_provider") - .and_then(|value| value.as_str()) - .map(str::trim) - .filter(|value| !value.is_empty()) - .map(str::to_string) - .unwrap_or_else(|| "codeg".to_string()); - table.insert( - "model_provider".to_string(), - toml::Value::String(provider_name.clone()), - ); + let api_base_url = trim_non_empty(api_base_url); + let provider_name = codex_provider_name_for_write(table, api_base_url.as_deref()); - let providers_item = table - .entry("model_providers".to_string()) - .or_insert_with(|| toml::Value::Table(toml::map::Map::new())); - if !providers_item.is_table() { - *providers_item = toml::Value::Table(toml::map::Map::new()); - } - let providers = providers_item - .as_table_mut() - .ok_or_else(|| AcpError::protocol("invalid model_providers table"))?; - let provider_item = providers - .entry(provider_name.clone()) - .or_insert_with(|| toml::Value::Table(toml::map::Map::new())); - if !provider_item.is_table() { - *provider_item = toml::Value::Table(toml::map::Map::new()); - } - let provider_table = provider_item - .as_table_mut() - .ok_or_else(|| AcpError::protocol("invalid model provider table"))?; - match trim_non_empty(api_base_url) { - Some(base_url) => { - provider_table.insert("base_url".to_string(), toml::Value::String(base_url)); + if let Some(provider_name) = provider_name { + table.insert( + "model_provider".to_string(), + toml::Value::String(provider_name.clone()), + ); + + let providers_item = table + .entry("model_providers".to_string()) + .or_insert_with(|| toml::Value::Table(toml::map::Map::new())); + if !providers_item.is_table() { + *providers_item = toml::Value::Table(toml::map::Map::new()); + } + let providers = providers_item + .as_table_mut() + .ok_or_else(|| AcpError::protocol("invalid model_providers table"))?; + let provider_item = providers + .entry(provider_name.clone()) + .or_insert_with(|| toml::Value::Table(toml::map::Map::new())); + if !provider_item.is_table() { + *provider_item = toml::Value::Table(toml::map::Map::new()); + } + let provider_table = provider_item + .as_table_mut() + .ok_or_else(|| AcpError::protocol("invalid model provider table"))?; + match api_base_url.as_deref() { + Some(base_url) => { + provider_table.insert( + "base_url".to_string(), + toml::Value::String(base_url.to_string()), + ); + } + None => { + provider_table.remove("base_url"); + } } - None => { - provider_table.remove("base_url"); + if provider_name == "codeg" { + provider_table.insert("name".to_string(), toml::Value::String("codeg".to_string())); + provider_table.insert( + "wire_api".to_string(), + toml::Value::String("responses".to_string()), + ); + ensure_codex_provider_auth_default(provider_table); } } - if provider_name == "codeg" { - provider_table.insert("name".to_string(), toml::Value::String("codeg".to_string())); - provider_table.insert( - "wire_api".to_string(), - toml::Value::String("responses".to_string()), - ); - ensure_codex_provider_auth_default(provider_table); - } if env.is_empty() { table.remove("env"); @@ -9110,51 +9136,49 @@ fn cascade_update_agent_config( .ok_or_else(|| AcpError::protocol("codex config root must be a TOML table"))?; table.remove("api_base_url"); - let provider_name = table - .get("model_provider") - .and_then(|value| value.as_str()) - .map(str::trim) - .filter(|value| !value.is_empty()) - .map(str::to_string) - .unwrap_or_else(|| "codeg".to_string()); - table.insert( - "model_provider".to_string(), - toml::Value::String(provider_name.clone()), - ); + let provider_name = codex_provider_name_for_write(table, Some(api_url)); - let providers_item = table - .entry("model_providers".to_string()) - .or_insert_with(|| toml::Value::Table(toml::map::Map::new())); - if !providers_item.is_table() { - *providers_item = toml::Value::Table(toml::map::Map::new()); - } - let providers = providers_item - .as_table_mut() - .ok_or_else(|| AcpError::protocol("invalid model_providers table"))?; - let provider_item = providers - .entry(provider_name.clone()) - .or_insert_with(|| toml::Value::Table(toml::map::Map::new())); - if !provider_item.is_table() { - *provider_item = toml::Value::Table(toml::map::Map::new()); - } - let provider_table = provider_item - .as_table_mut() - .ok_or_else(|| AcpError::protocol("invalid model provider table"))?; - if api_url.trim().is_empty() { - provider_table.remove("base_url"); - } else { - provider_table.insert( - "base_url".to_string(), - toml::Value::String(api_url.to_string()), - ); - } - if provider_name == "codeg" { - provider_table.insert("name".to_string(), toml::Value::String("codeg".to_string())); - provider_table.insert( - "wire_api".to_string(), - toml::Value::String("responses".to_string()), + if let Some(provider_name) = provider_name { + table.insert( + "model_provider".to_string(), + toml::Value::String(provider_name.clone()), ); - ensure_codex_provider_auth_default(provider_table); + + let providers_item = table + .entry("model_providers".to_string()) + .or_insert_with(|| toml::Value::Table(toml::map::Map::new())); + if !providers_item.is_table() { + *providers_item = toml::Value::Table(toml::map::Map::new()); + } + let providers = providers_item + .as_table_mut() + .ok_or_else(|| AcpError::protocol("invalid model_providers table"))?; + let provider_item = providers + .entry(provider_name.clone()) + .or_insert_with(|| toml::Value::Table(toml::map::Map::new())); + if !provider_item.is_table() { + *provider_item = toml::Value::Table(toml::map::Map::new()); + } + let provider_table = provider_item + .as_table_mut() + .ok_or_else(|| AcpError::protocol("invalid model provider table"))?; + if api_url.trim().is_empty() { + provider_table.remove("base_url"); + } else { + provider_table.insert( + "base_url".to_string(), + toml::Value::String(api_url.to_string()), + ); + } + if provider_name == "codeg" { + provider_table + .insert("name".to_string(), toml::Value::String("codeg".to_string())); + provider_table.insert( + "wire_api".to_string(), + toml::Value::String("responses".to_string()), + ); + ensure_codex_provider_auth_default(provider_table); + } } match codex_model { CodexModelAction::Set(model) => {