From 42dab7c89d6eaa3a6acefbe68852a8351bb3241c Mon Sep 17 00:00:00 2001 From: "posthog[bot]" <206114724+posthog[bot]@users.noreply.github.com> Date: Sun, 20 Sep 2026 16:55:08 +0000 Subject: [PATCH] fix(mcp): delete the codex server section when the CLI cannot remove it `codex mcp remove` now falls back to editing ~/.codex/config.toml directly, the mirror of the write the OAuth install path already does. A broken codex CLI left the [mcp_servers.posthog] entry in place and reported its own crash as a wizard exception. An absent server is reported as unchanged, and only a failed config.toml edit captures an exception. Generated-By: PostHog Desktop Task-Id: 6071a238-3d65-45c3-93b4-42dba485bc1e --- .../clients/__tests__/codex.test.ts | 67 +++++++++++++- .../clients/codex.ts | 88 +++++++++++++------ 2 files changed, 126 insertions(+), 29 deletions(-) diff --git a/src/steps/add-mcp-server-to-clients/clients/__tests__/codex.test.ts b/src/steps/add-mcp-server-to-clients/clients/__tests__/codex.test.ts index 4ff93c361..9040967c2 100644 --- a/src/steps/add-mcp-server-to-clients/clients/__tests__/codex.test.ts +++ b/src/steps/add-mcp-server-to-clients/clients/__tests__/codex.test.ts @@ -12,6 +12,9 @@ vi.mock('node:fs', () => ({ existsSync: vi.fn(), readFileSync: vi.fn(), rmSync: vi.fn(), + mkdirSync: vi.fn(), + writeFileSync: vi.fn(), + renameSync: vi.fn(), })); vi.mock('../../../../utils/analytics', () => ({ @@ -22,6 +25,8 @@ describe('CodexMCPClient', () => { const spawnSyncMock = spawnSync as Mock; const execSyncMock = execSync as Mock; const readFileSyncMock = fs.readFileSync as Mock; + const existsSyncMock = fs.existsSync as Mock; + const writeFileSyncMock = fs.writeFileSync as Mock; const CODEX_PATH = '/usr/local/bin/codex'; @@ -33,6 +38,8 @@ describe('CodexMCPClient', () => { // implementations, so without this a config.toml fixture set by one test // leaks into the next one's isPluginInstalled() check. readFileSyncMock.mockReturnValue(''); + existsSyncMock.mockReturnValue(false); + writeFileSyncMock.mockReturnValue(undefined); }); describe('isClientSupported', () => { @@ -172,12 +179,68 @@ describe('CodexMCPClient', () => { ); }); - it('returns the failure reason and captures exception on failure', async () => { + it('deletes the config.toml section when the codex CLI fails', async () => { + spawnSyncMock.mockReturnValue({ + error: new Error('spawn /opt/codex/vendor/codex ENOENT'), + }); + existsSyncMock.mockReturnValue(true); + readFileSyncMock.mockReturnValue( + '[mcp_servers.posthog]\nurl = "https://mcp.posthog.com/mcp"\n\n[mcp_servers.other]\nurl = "https://example.com"\n', + ); + const client = new CodexMCPClient(); + await expect(client.removeServer()).resolves.toEqual({ success: true }); + expect(writeFileSyncMock).toHaveBeenCalledWith( + expect.stringContaining('.wizard-tmp'), + '[mcp_servers.other]\nurl = "https://example.com"\n', + ); + expect(analytics.captureException).not.toHaveBeenCalled(); + }); + + it('deletes the config.toml section when the codex CLI is gone', async () => { + execSyncMock.mockImplementation(() => { + throw new Error('not found'); + }); + existsSyncMock.mockReturnValue(true); + readFileSyncMock.mockReturnValue( + '[mcp_servers.posthog-local]\nurl = "http://localhost:8787/mcp"\n', + ); + const client = new CodexMCPClient(); + await expect(client.removeServer(true)).resolves.toEqual({ + success: true, + }); + expect(spawnSyncMock).not.toHaveBeenCalled(); + expect(writeFileSyncMock).toHaveBeenCalledWith( + expect.stringContaining('.wizard-tmp'), + '', + ); + }); + + it('reports an absent server as success without writing', async () => { + spawnSyncMock.mockReturnValue({ status: 1, stderr: 'no such server' }); + existsSyncMock.mockReturnValue(true); + readFileSyncMock.mockReturnValue( + '[mcp_servers.other]\nurl = "https://example.com"\n', + ); + const client = new CodexMCPClient(); + await expect(client.removeServer()).resolves.toEqual({ + success: true, + alreadyInstalled: true, + }); + expect(writeFileSyncMock).not.toHaveBeenCalled(); + expect(analytics.captureException).not.toHaveBeenCalled(); + }); + + it('returns the failure reason and captures exception when the write fails', async () => { spawnSyncMock.mockReturnValue({ status: 1, stderr: 'codex is locked' }); + existsSyncMock.mockReturnValue(true); + readFileSyncMock.mockReturnValue('[mcp_servers.posthog]\nurl = "u"\n'); + writeFileSyncMock.mockImplementation(() => { + throw new Error('EACCES: permission denied'); + }); const client = new CodexMCPClient(); await expect(client.removeServer()).resolves.toEqual({ success: false, - reason: 'codex is locked', + reason: 'EACCES: permission denied', }); expect(analytics.captureException).toHaveBeenCalled(); }); diff --git a/src/steps/add-mcp-server-to-clients/clients/codex.ts b/src/steps/add-mcp-server-to-clients/clients/codex.ts index e4020d54b..759bbcaa1 100644 --- a/src/steps/add-mcp-server-to-clients/clients/codex.ts +++ b/src/steps/add-mcp-server-to-clients/clients/codex.ts @@ -46,6 +46,24 @@ const sectionHeader = (serverName: string): RegExp => 'm', ); +type Section = { headerStart: number; bodyStart: number; bodyEnd: number }; + +/** + * Locate the server's section. Everything up to the next table header belongs + * to this server, so the body ends there, or at the end of the file. + */ +const findSection = (contents: string, serverName: string): Section | null => { + const header = sectionHeader(serverName).exec(contents); + if (!header) return null; + const bodyStart = header.index + header[0].length; + const next = /^\[/m.exec(contents.slice(bodyStart)); + return { + headerStart: header.index, + bodyStart, + bodyEnd: next ? bodyStart + next.index : contents.length, + }; +}; + /** * Set `key = value` inside a section body, inserting it when absent. Scans the * whole body rather than the line after the header: TOML does not care about @@ -176,8 +194,8 @@ export class CodexMCPClient ? fs.readFileSync(configPath, 'utf-8') : ''; - const header = sectionHeader(serverName).exec(contents); - if (!header) { + const section = findSection(contents, serverName); + if (!section) { const gap = contents === '' || contents.endsWith('\n\n') ? '' : '\n'; const pad = contents === '' || contents.endsWith('\n') ? '' : '\n'; this.write( @@ -188,13 +206,8 @@ export class CodexMCPClient return { success: true }; } - // Everything up to the next table header belongs to this server. - const start = header.index + header[0].length; - const rest = contents.slice(start); - const next = /^\[/m.exec(rest); - const end = next ? start + next.index : contents.length; - - const body = contents.slice(start, end); + const { bodyStart, bodyEnd } = section; + const body = contents.slice(bodyStart, bodyEnd); const updated = setKey( setKey(body, 'url', `"${url}"`), 'startup_timeout_sec', @@ -204,7 +217,7 @@ export class CodexMCPClient this.write( configPath, - contents.slice(0, start) + updated + contents.slice(end), + contents.slice(0, bodyStart) + updated + contents.slice(bodyEnd), ); return { success: true }; } catch (error) { @@ -230,31 +243,52 @@ export class CodexMCPClient } removeServer(local?: boolean): Promise { - const binary = this.findCodexBinary(); - if (!binary) - return Promise.resolve({ - success: false, - reason: 'The codex CLI is no longer on your PATH.', - }); - // `local` was ignored here, so `mcp remove --local` reported success while // leaving the posthog-local server in place. const serverName = local ? 'posthog-local' : 'posthog'; - const result = spawnSync(binary, ['mcp', 'remove', serverName], { - encoding: 'utf-8', - }); + const binary = this.findCodexBinary(); - if (result.error || result.status !== 0) { - const reason = redactSecrets( - result.error?.message ?? result.stderr ?? 'codex mcp remove failed', + if (binary) { + const result = spawnSync(binary, ['mcp', 'remove', serverName], { + encoding: 'utf-8', + }); + if (!result.error && result.status === 0) { + return Promise.resolve({ success: true }); + } + } + + // The CLI is absent, or it crashed on something that isn't ours — a codex + // wrapper that cannot spawn its own native binary, for one. The install + // path writes this section itself, so remove it the same way instead of + // leaving the entry behind and reporting a third-party crash as ours. + return Promise.resolve(this.deleteServerSection(serverName)); + } + + /** Delete the `[mcp_servers.]` section, the mirror of writeServerSection. */ + private deleteServerSection(serverName: string): InstallResult { + const configPath = this.configPath(); + try { + if (!fs.existsSync(configPath)) { + return { success: true, alreadyInstalled: true }; + } + const contents = fs.readFileSync(configPath, 'utf-8'); + const section = findSection(contents, serverName); + // Nothing registered, so the requested end state already holds. + if (!section) return { success: true, alreadyInstalled: true }; + + this.write( + configPath, + contents.slice(0, section.headerStart) + + contents.slice(section.bodyEnd), ); + return { success: true }; + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); analytics.captureException( - new Error(`Failed to remove server from Codex CLI: ${reason}`), + new Error(`Codex config.toml server removal failed: ${reason}`), ); - return Promise.resolve({ success: false, reason }); + return { success: false, reason }; } - - return Promise.resolve({ success: true }); } /** The codex marketplace plugin ships skills only — the MCP server needs its own entry. */