fix(codex): preserve nested MCP env tables when updating config

[yukaidi1220]

Summary

This fixes Codex MCP config updates when ~/.codex/config.toml already contains nested MCP env tables such as [mcp_servers.node_repl.env].

Before this change, updating MCP services could break existing Codex config in two ways:

• writing invalid TOML while handling env
• leaving both [mcp_servers.<id>.env] and env = {...} for the same service, which caused duplicate key errors

In practice, this showed up during MCP installation in Codex on Windows.

What changed

• stopped using direct object writes for mcp_servers.*.env updates
• switched MCP section updates to section-based replacement for the target service
• expanded section matching so nested child sections like [mcp_servers.node_repl.env] are updated or removed together with the parent MCP service section
• added a regression test covering existing node_repl.env config

Why

Codex may already contain MCP entries that use nested env tables.
The previous update path could rewrite those entries into an incompatible shape, which led to TOML parse failures and duplicate key errors.

This change keeps the update behavior predictable for MCP config while avoiding corruption of existing nested env tables.

Verification

Ran:

corepack pnpm exec vitest run tests/unit/utils/code-tools/codex-platform.test.ts tests/unit/utils/code-tools/codex-configure.test.ts tests/unit/utils/code-tools/codex-extra-fields.test.ts tests/unit/utils/code-tools/toml-parser-refactor.test.ts

## Result:

4 test files passed
41 tests passed

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Preserve nested MCP env tables during config updates

🐞 Bug fix

Walkthroughs

Description

• Refactored MCP service config updates to preserve nested env tables
• Replaced individual field edits with section-based replacement approach
• Enhanced section regex to match and remove nested child sections together
• Added regression test for existing node_repl.env config preservation

Diagram

flowchart LR
  A["MCP Service Update"] --> B["upsertMcpSection"]
  B --> C["createMcpSectionRegex"]
  C --> D["Match parent and nested sections"]
  D --> E["renderMcpSection"]
  E --> F["Format inline tables"]
  F --> G["Updated TOML content"]

Loading

File Changes

1. src/utils/code-tools/codex-toml-updater.ts 🐞 Bug fix +105/-21

Refactor MCP config updates with section-based replacement

• Replaced individual editToml calls with new upsertMcpSection function for atomic updates
• Added createMcpSectionRegex to match parent and nested MCP sections together
• Implemented renderMcpSection to generate properly formatted TOML sections
• Added helper functions for TOML formatting: formatInlineTable, formatTomlArray,
 escapeTomlString
• Simplified deleteCodexMcpService to use the new regex helper

src/utils/code-tools/codex-toml-updater.ts

---

2. tests/unit/utils/code-tools/codex-platform.test.ts 🧪 Tests +42/-0

Add test for nested env table preservation

• Added regression test for preserving existing nested [mcp_servers.node_repl.env] tables
• Verifies that nested env sections are not converted to inline table format
• Ensures new MCP services can be added without corrupting existing nested env config

tests/unit/utils/code-tools/codex-platform.test.ts

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. SSE url gets deleted ✓ Resolved 🐞 Bug ≡ Correctness

Description

For SSE MCP services (detected via existingService.url), the updater rewrites the entire
[mcp_servers.] section using a renderer that only emits managed fields, so it drops url and any
other user/unknown keys (e.g., retries, max_connections, or parsed extraFields), breaking
service configs and causing silent data loss. This contradicts the function’s contract to preserve
unmanaged fields and will corrupt existing configurations during batch update runs.

Code

src/utils/code-tools/codex-toml-updater.ts[R302-324]

Evidence

The updater both claims it must preserve unmanaged fields like url and uses existingService.url
to detect SSE services, implying url must survive updates, but the rewrite path
(upsertMcpSection → renderMcpSection) replaces the whole [mcp_servers.] block while only
outputting command/args/env/startup_timeout_sec, so url is necessarily removed. Additionally,
the codebase parses and models unknown MCP keys into extraFields, yet the new renderer does not
emit them, meaning any update will drop these keys. Because configureCodexMcp() calls
batchUpdateCodexMcpServices(finalServices) over the merged list (including existing services), the
full-section rewrite is triggered for real user configs, causing the losses described.

src/utils/code-tools/codex-toml-updater.ts[180-210]
src/utils/code-tools/codex-toml-updater.ts[302-324]
src/utils/code-tools/codex-configure.ts[205-229]
src/utils/code-tools/codex.ts[45-53]
src/utils/code-tools/codex.ts[510-535]
src/utils/code-tools/codex-toml-updater.ts[284-324]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`upsertCodexMcpService()` detects SSE services via `existingService.url` and the updater contract says unmanaged fields (like `url`) must be preserved, but the section rewrite path (`upsertMcpSection` → `renderMcpSection`) replaces the entire `[mcp_servers.<id>]` block while only emitting managed keys (`command/args/env/startup_timeout_sec`). As a result, `url` and any other user/unknown MCP keys (including those parsed into `extraFields`, such as `retries` or `max_connections`) are silently deleted during updates, breaking SSE services and causing data loss.
## Issue Context
- The updater explicitly claims it preserves unmanaged fields like `url`.
- SSE services are detected via `existingService.url`, so dropping `url` makes these services unusable.
- `parseCodexConfig()` collects unknown MCP keys into `extraFields`, indicating they are intended to survive round-trips.
- `configureCodexMcp()` calls `batchUpdateCodexMcpServices(finalServices)` over the merged service list (including existing ones), so the rewrite affects real user configs during batch updates.
## Fix Focus Areas
- src/utils/code-tools/codex-toml-updater.ts[190-213]
- src/utils/code-tools/codex-toml-updater.ts[284-324]
- src/utils/code-tools/codex.ts[510-535]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. readCodexConfig mock uses any ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The new test mocks readCodexConfig using as any, weakening TypeScript’s type guarantees and
potentially masking incorrect config shapes. This violates the strict typing requirement for changed
TypeScript code.

Code

tests/unit/utils/code-tools/codex-platform.test.ts[R154-158]

Evidence

PR Compliance ID 2 requires maintaining strict TypeScript typing and avoiding weakening typing via
any in changed code. The added test code explicitly casts the mocked return value to any via `}
as any)`.

CLAUDE.md: Maintain Strict TypeScript Typing With Explicit Type Definitions: CLAUDE.md: Maintain Strict TypeScript Typing With Explicit Type Definitions: CLAUDE.md: Maintain Strict TypeScript Typing With Explicit Type Definitions: CLAUDE.md: Maintain Strict TypeScript Typing With Explicit Type Definitions: CLAUDE.md: Maintain Strict TypeScript Typing With Explicit Type Definitions: CLAUDE.md: Maintain Strict TypeScript Typing With Explicit Type Definitions: CLAUDE.md: Maintain Strict TypeScript Typing With Explicit Type Definitions: CLAUDE.md: Maintain Strict TypeScript Typing With Explicit Type Definitions
tests/unit/utils/code-tools/codex-platform.test.ts[154-158]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A new test uses `as any` when mocking `readCodexConfig`, which weakens strict typing and can hide incorrect mock shapes.
## Issue Context
`readCodexConfig()` returns `CodexConfigData | null`. The mock can be typed by returning a complete `CodexConfigData` object (e.g., set `model`/`modelProvider` to `null`) or by using a safer typed helper rather than `as any`.
## Fix Focus Areas
- tests/unit/utils/code-tools/codex-platform.test.ts[154-158]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Env values path-mangled ✓ Resolved 🐞 Bug ≡ Correctness

Description

All string env values are passed through normalizeTomlPath(), which replaces backslashes and
collapses repeated slashes. This can corrupt non-path env values (e.g., UNC paths \\server\share,
regex strings, or other backslash-sensitive values).

Code

src/utils/code-tools/codex-toml-updater.ts[R334-337]

Evidence

The new env/table formatter normalizes every string via normalizeTomlPath(), and that function is
documented/implemented as a Windows path normalizer that replaces backslashes and collapses
slashes—behavior that is not safe for arbitrary env strings.

src/utils/code-tools/codex-toml-updater.ts[326-372]
src/utils/platform.ts[126-139]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`formatInlineTableValue()` and `formatTomlArray()` apply `normalizeTomlPath()` to every string. `normalizeTomlPath()` is explicitly a Windows-path normalizer (backslashes → forward slashes, collapsing duplicates), so using it on arbitrary env values can change semantics.
### Issue Context
This is especially risky for values where backslashes are meaningful (UNC paths, regex patterns, JSON escape sequences).
### Fix approach
- Only apply `normalizeTomlPath()` when the value is known to be a filesystem path (e.g., `command`, `args`, or specific env keys like `SYSTEMROOT`), or when it matches a Windows-path heuristic (`/^[A-Za-z]:\\/` or starts with `\\`).
- Otherwise, keep the original string and rely on TOML escaping/quoting.
### Fix Focus Areas
- src/utils/code-tools/codex-toml-updater.ts[334-372]
- src/utils/platform.ts[126-139]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Section regex can overmatch ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new MCP section regex requires table headers to start immediately after \n[ with no leading
whitespace, so indented headers may not be recognized as boundaries. In that case, deleting or
upserting one MCP service can accidentally consume subsequent unrelated sections.

Code

src/utils/code-tools/codex-toml-updater.ts[R276-281]

Evidence

The regex in the updater assumes section headers begin at column 0, while other code in the repo
explicitly trims whitespace when detecting section headers, indicating indentation is a plausible
input shape; thus the updater regex can miss boundaries and overmatch.

src/utils/code-tools/codex-toml-updater.ts[276-282]
src/utils/code-tools/codex-uninstaller.ts[338-349]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`createMcpSectionRegex()` uses `\n?\[mcp_servers...` and a lookahead boundary `(?=\n\[ ...)`, which fails to detect section boundaries when headers are indented (e.g., `"\n  [section]"`). That can cause over-deletion/over-replacement.
### Issue Context
Other code in the repo already uses `line.trim().startsWith('[')` to handle possible indentation when scanning TOML sections.
### Fix approach
- Allow optional whitespace before `[` in both the section start and boundary, e.g. `\n?\s*\[` and boundary `(?=\n\s*\[ ...)`.
- Alternatively, avoid regex scanning and instead parse TOML and/or implement a line-based section remover using `trim()`.
### Fix Focus Areas
- src/utils/code-tools/codex-toml-updater.ts[276-282]
- src/utils/code-tools/codex-uninstaller.ts[338-349]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[yukaidi1220]

Addressed the follow-up review items in the latest commits.

Changes included:

• preserved existing SSE fields such as url and other unmanaged keys during MCP updates
• avoided normalizing URL-like strings as Windows paths
• tightened MCP section matching so indented section headers do not cause overmatching
• replaced as any in the new test setup with a typed mock config helper

Re-ran the related tests and they pass.

[WitMiao]

ci fail @yukaidi1220