From f9ee52265338029a313ea3357539b600960638e4 Mon Sep 17 00:00:00 2001 From: Adam Dalloul <47503782+Adam-Dalloul@users.noreply.github.com> Date: Wed, 9 Sep 2026 20:13:02 -0700 Subject: [PATCH] fix(codex): stop installing an empty codeg provider in the shared config.toml An unrelated switch could bind codeg as codex's model provider with no URL behind it, writing model_provider = "codeg" beside a [model_providers.codeg] whose base_url is "". codex then fails every request at the builder stage, in codeg and in every other client that reads ~/.codex/config.toml, and nothing in the panel undoes it. The WebSocket switch no longer adopts a provider. With none bound it writes only features.responses_websockets_v2, which is what the reader falls back to, so the control still round-trips. Clearing the API base URL no longer creates a provider either; typing one still binds codeg as before. Fixes #520 --- .../settings/acp-agent-settings.test.tsx | 145 +++++++++++++++++- .../settings/acp-agent-settings.tsx | 65 +++++--- 2 files changed, 178 insertions(+), 32 deletions(-) diff --git a/src/components/settings/acp-agent-settings.test.tsx b/src/components/settings/acp-agent-settings.test.tsx index 4532f2ec38..2d08ef9f61 100644 --- a/src/components/settings/acp-agent-settings.test.tsx +++ b/src/components/settings/acp-agent-settings.test.tsx @@ -1307,13 +1307,29 @@ describe("patchCodexConfigTomlText — codeg's requires_openai_auth default", () // The three structured controls that reach ensureCodexProviderDefaults. Each // must behave identically — the bug in issue #406 fired through all of them. + // `seedsFromEmpty` marks the ones that may CREATE the provider: the WebSocket + // toggle carries no URL, so on an unbound config it writes the `[features]` + // key instead of installing an empty provider (#520). const ENTRY_POINTS: Array<{ label: string patch: Parameters[1] + seedsFromEmpty: boolean }> = [ - { label: "API base URL", patch: { apiBaseUrl: "https://new.example/v1" } }, - { label: "WebSocket toggle", patch: { supportsWebsockets: true } }, - { label: "model provider", patch: { modelProvider: "codeg" } }, + { + label: "API base URL", + patch: { apiBaseUrl: "https://new.example/v1" }, + seedsFromEmpty: true, + }, + { + label: "WebSocket toggle", + patch: { supportsWebsockets: true }, + seedsFromEmpty: false, + }, + { + label: "model provider", + patch: { modelProvider: "codeg" }, + seedsFromEmpty: true, + }, ] const BOUND_PROVIDER = [ @@ -1325,7 +1341,7 @@ describe("patchCodexConfigTomlText — codeg's requires_openai_auth default", () 'wire_api = "responses"', ].join("\n") - for (const { label, patch } of ENTRY_POINTS) { + for (const { label, patch, seedsFromEmpty } of ENTRY_POINTS) { describe(`via the ${label} control`, () => { it("keeps an explicit false", () => { const toml = `${BOUND_PROVIDER}\nrequires_openai_auth = false\n` @@ -1343,9 +1359,16 @@ 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( + seedsFromEmpty + ? "seeds a brand-new provider from an empty config" + : "does not seed a provider from an empty config", + () => { + expect(authFlagOf(patchCodexConfigTomlText("", patch))).toBe( + seedsFromEmpty ? true : undefined + ) + } + ) it("stands down for a provider using actor authorization", () => { const toml = [ @@ -1482,6 +1505,114 @@ describe("patchCodexConfigTomlText — codeg's requires_openai_auth default", () }) }) +// config.toml is shared with the codex CLI and the native Codex app. Adopting +// `codeg` as the model provider from a control that carries no URL writes +// `model_provider = "codeg"` beside `[model_providers.codeg] base_url = ""`, and +// codex then fails every request at the builder stage ("stream disconnected +// before completion: builder error") in every client reading that file. Nothing +// in codeg's UI undoes it. See issue #520. +describe("patchCodexConfigTomlText: an unbound config stays unbound", () => { + // What the panel really sends: the draft's provider is "" while nothing is + // bound, and it rides along on the URL and WebSocket patches. + const UNBOUND_DRAFT_PROVIDER = "" + const UNBOUND = 'model = "gpt-5.6-sol"\n' + const BOUND = [ + 'model_provider = "acme"', + "", + "[model_providers.acme]", + 'base_url = "https://acme.example/v1"', + ].join("\n") + + function codegProviderOf(configTomlText: string): { + root: string | undefined + table: unknown + } { + const parsed = parseTomlDocument(configTomlText) as { + model_provider?: unknown + model_providers?: Record + } + return { + root: + typeof parsed.model_provider === "string" + ? parsed.model_provider + : undefined, + table: parsed.model_providers?.codeg, + } + } + + for (const enabled of [true, false]) { + it(`turning WebSockets ${enabled ? "on" : "off"} binds no provider`, () => { + const after = patchCodexConfigTomlText(UNBOUND, { + modelProvider: UNBOUND_DRAFT_PROVIDER, + supportsWebsockets: enabled, + }) + const { root, table } = codegProviderOf(after) + expect(root).toBeUndefined() + expect(table).toBeUndefined() + expect(after).not.toContain("base_url") + }) + } + + // Without a provider the `[features]` key IS the setting, and it is what the + // reader falls back to, so the switch has to keep round-tripping. + it("still moves the WebSocket switch, through the feature key", () => { + const on = patchCodexConfigTomlText(UNBOUND, { + modelProvider: UNBOUND_DRAFT_PROVIDER, + supportsWebsockets: true, + }) + expect(on).toContain("responses_websockets_v2 = true") + expect(extractCodexImportantValues("", on).supportsWebsockets).toBe(true) + + const off = patchCodexConfigTomlText(on, { + modelProvider: UNBOUND_DRAFT_PROVIDER, + supportsWebsockets: false, + }) + expect(off).not.toContain("responses_websockets_v2") + expect(extractCodexImportantValues("", off).supportsWebsockets).toBe(false) + expect(codegProviderOf(off).root).toBeUndefined() + }) + + it("clearing the API base URL binds no provider", () => { + const after = patchCodexConfigTomlText(UNBOUND, { + modelProvider: UNBOUND_DRAFT_PROVIDER, + apiBaseUrl: "", + }) + expect(codegProviderOf(after).table).toBeUndefined() + expect(after).not.toContain("model_providers") + }) + + it("typing an API base URL still binds codeg", () => { + const after = patchCodexConfigTomlText(UNBOUND, { + modelProvider: UNBOUND_DRAFT_PROVIDER, + apiBaseUrl: "https://new.example/v1", + }) + expect(codegProviderOf(after).root).toBe("codeg") + expect(after).toContain('base_url = "https://new.example/v1"') + }) + + it("writes supports_websockets onto the provider the user did bind", () => { + const after = patchCodexConfigTomlText(BOUND, { supportsWebsockets: true }) + const parsed = parseTomlDocument(after) as { + model_provider?: string + model_providers?: Record + } + expect(parsed.model_provider).toBe("acme") + expect(parsed.model_providers?.acme?.supports_websockets).toBe(true) + expect(parsed.model_providers?.codeg).toBeUndefined() + }) + + it("still clears a bound provider's base URL", () => { + const after = patchCodexConfigTomlText(BOUND, { apiBaseUrl: "" }) + const parsed = parseTomlDocument(after) as { + model_provider?: string + model_providers?: Record + } + expect(parsed.model_provider).toBe("acme") + expect(parsed.model_providers?.acme).toBeDefined() + expect(parsed.model_providers?.acme?.base_url).toBeUndefined() + }) +}) + // The panel used to ship a Reasoning Effort dropdown, and the writer rewrote // `model_reasoning_effort` on EVERY patch with "high" as the parse default. So // touching any unrelated control injected a key the user never asked for and diff --git a/src/components/settings/acp-agent-settings.tsx b/src/components/settings/acp-agent-settings.tsx index b046daa8b5..f0fb943c93 100644 --- a/src/components/settings/acp-agent-settings.tsx +++ b/src/components/settings/acp-agent-settings.tsx @@ -3061,44 +3061,59 @@ export function patchCodexConfigTomlText( } if (typeof patch.apiBaseUrl === "string") { const tomlValues = extractCodexTomlImportantValues(nextTomlText) + // Falling back to `codeg` is only right when this patch actually carries a + // URL for it. Clearing the box must not conjure a provider whose entire + // content would be `base_url = ""`. See the websockets arm below. const modelProvider = patch.modelProvider?.trim() || tomlValues.modelProvider.trim() || - CODEX_DEFAULT_MODEL_PROVIDER - if (!tomlValues.modelProvider.trim() && patch.apiBaseUrl.trim()) { - nextTomlText = updateTomlRootStringKey( + (patch.apiBaseUrl.trim() ? CODEX_DEFAULT_MODEL_PROVIDER : "") + if (modelProvider) { + if (!tomlValues.modelProvider.trim() && patch.apiBaseUrl.trim()) { + nextTomlText = updateTomlRootStringKey( + nextTomlText, + "model_provider", + modelProvider + ) + } + nextTomlText = patchCodexProviderBaseUrl( nextTomlText, - "model_provider", - modelProvider + modelProvider, + patch.apiBaseUrl ) + nextTomlText = ensureCodexProviderDefaults(nextTomlText, modelProvider) } - nextTomlText = patchCodexProviderBaseUrl( - nextTomlText, - modelProvider, - patch.apiBaseUrl - ) - nextTomlText = ensureCodexProviderDefaults(nextTomlText, modelProvider) } if (typeof patch.supportsWebsockets === "boolean") { const tomlValues = extractCodexTomlImportantValues(nextTomlText) + // No `|| CODEX_DEFAULT_MODEL_PROVIDER` here. config.toml is shared with the + // codex CLI and the native Codex app, and adopting `codeg` from a switch + // that has nothing to do with providers installs + // `model_provider = "codeg"` + `[model_providers.codeg] base_url = ""` into + // a file the user never pointed at codeg. codex then fails every request at + // the builder stage ("stream disconnected before completion: builder + // error") in every client, and nothing inside codeg undoes it. See #520. const modelProvider = - patch.modelProvider?.trim() || - tomlValues.modelProvider.trim() || - CODEX_DEFAULT_MODEL_PROVIDER - if (!tomlValues.modelProvider.trim()) { - nextTomlText = updateTomlRootStringKey( + patch.modelProvider?.trim() || tomlValues.modelProvider.trim() + if (modelProvider) { + nextTomlText = patchCodexProviderField( nextTomlText, - "model_provider", - modelProvider + modelProvider, + "supports_websockets", + `supports_websockets = ${patch.supportsWebsockets ? "true" : "false"}` + ) + nextTomlText = ensureCodexProviderDefaults(nextTomlText, modelProvider) + } else { + // Unbound: the `[features]` key IS the setting, and it is what the reader + // falls back to. Write it here so the normalization below reads the value + // the switch just moved to instead of the one already on disk. + nextTomlText = upsertTomlSectionBooleanKey( + nextTomlText, + "features", + "responses_websockets_v2", + patch.supportsWebsockets ? true : null ) } - nextTomlText = patchCodexProviderField( - nextTomlText, - modelProvider, - "supports_websockets", - `supports_websockets = ${patch.supportsWebsockets ? "true" : "false"}` - ) - nextTomlText = ensureCodexProviderDefaults(nextTomlText, modelProvider) } const normalizedTomlValues = extractCodexTomlImportantValues(nextTomlText) if (normalizedTomlValues.model.trim()) {