fix(codex): preserve literal Windows TOML path keys

[danielmeppiel]

fix(codex): preserve literal Windows TOML path keys

TL;DR

Codex configuration now uses tomlkit for round-trip-safe reads and writes. Valid literal-quoted Windows path keys are accepted and preserved while APM adds or removes MCP servers, eliminating the parser rejection and backslash corruption reported in #2075.

Important

Closes #2075. This changes only Codex TOML handling; no CLI contract or documentation changes are required.

Problem (WHY)

• The legacy toml parser rejected valid Codex keys such as 'C:\Users\me\Documents\Playground', so apm install --global --only mcp skipped the Codex write.
• Whole-document serialization could rewrite literal path keys with compounded escaping, destroying Codex project trust and per-path preferences.
• [!] The stale-server cleanup path used the same lossy parser/writer and broad exception handling, so cleanup could silently skip valid Windows configs.

The regression tests make the behavior deterministic: "Grounding outputs in deterministic tool execution transforms probabilistic generation into verifiable action."

Approach (WHAT)

#	Fix	Principle
1	Parse and serialize Codex config with tomlkit so literal-key spelling and unrelated formatting survive updates.	"Favor small, chainable primitives over monolithic frameworks."
2	Apply the same round-trip path to stale MCP cleanup and narrow failures to parse and I/O errors.	"Grounding outputs in deterministic tool execution transforms probabilistic generation into verifiable action."
3	Exercise real Windows-style TOML text across repeated writes and cleanup.	"agents pattern-match well against concrete structures"

Implementation (HOW)

File	Change
src/apm_cli/adapters/client/codex.py	Replaces lossy toml loading/dumping with tomlkit round-trip documents while retaining parse-failure data-loss protection.
src/apm_cli/integration/mcp_integrator.py	Uses the same round-trip parser for stale-server removal, narrows exception handling, and leaves non-table mcp_servers untouched.
tests/unit/test_codex_adapter_phase3.py	Adds parser and repeated-write regression traps using literal-quoted Windows path keys.
tests/unit/test_mcp_integrator_remove_stale.py	Proves cleanup removes only stale MCP entries and preserves unrelated Windows path content; covers malformed MCP schema without rewriting.
pyproject.toml, uv.lock	Adds tomlkit>=0.13 as the direct round-trip TOML dependency.
scripts/notice-metadata.yaml, NOTICE	Records the dependency's MIT license and upstream attribution.

Diagrams

Legend: Only the mcp_servers table is changed; the dashed stages are the new round-trip-safe path.

flowchart LR
    subgraph Read[Read]
        C[Codex config.toml]
        P[tomlkit parse]
    end
    subgraph Change[Change]
        M[Update mcp_servers only]
    end
    subgraph Write[Write]
        D[tomlkit dumps]
        O[Codex config.toml]
    end
    C --> P
    P --> M
    M --> D
    D --> O
    classDef new stroke-dasharray: 5 5;
    class P,M,D new;

Loading

Trade-offs

• Round-trip dependency over hand-written surgery. Chose tomlkit; rejected regex or text splicing because nested tables, quoted keys, and comments make partial TOML editing unsafe.
• Keep legacy toml dependency for existing consumers. This PR moves only Codex production paths; removing or migrating unrelated test and integration consumers would broaden scope.
• Skip malformed MCP schema rather than coerce it. A non-table mcp_servers value is left byte-for-byte unchanged instead of risking user-config data loss.

Benefits

1. Literal keys containing \U parse successfully on every supported host OS.
2. Two consecutive MCP updates preserve [projects.'c:\...'] and per-path preference text unchanged.
3. Stale cleanup removes the requested server while retaining other servers and unrelated Codex configuration.
4. Parse failures and I/O failures remain safe no-write outcomes without a broad exception catch.

Validation

Regression and relevant suites

uv run --extra dev pytest tests/unit/test_codex_adapter_phase3.py tests/unit/test_codex_adapter_compatibility.py tests/unit/test_mcp_integrator_remove_stale.py tests/unit/integration/test_mcp_integrator.py -q

252 passed in 0.97s

Mutation-break proof using the previous toml read/write path:

2 failed in 1.05s

Full CI lint mirror

All checks passed!
1410 files already formatted
Your code has been rated at 10.00/10
[+] auth-signal lint clean
CI source guards passed
NOTICE is up to date

Scenario Evidence

#	Scenario (user promise)	Principle(s)	Test(s) proving it	Type
1	Install MCP servers with Codex on Windows without rejecting or rewriting literal path keys.	Multi-harness support, DevX (pragmatic as npm)	tests/unit/test_codex_adapter_phase3.py::TestGetCurrentConfig::test_parses_windows_path_in_literal_quoted_key tests/unit/test_codex_adapter_phase3.py::TestUpdateConfig::test_update_preserves_windows_literal_quoted_path_keys (regression-trap for #2075)	unit
2	Remove a stale Codex MCP server without changing project trust or desktop path preferences.	Multi-harness support, DevX (pragmatic as npm)	tests/unit/test_mcp_integrator_remove_stale.py::TestCleanCodexToml::test_preserves_windows_literal_keys_while_removing_stale_server (regression-trap for #2075)	unit
3	Leave an unexpected non-table mcp_servers value untouched instead of corrupting the config.	Secure by default	tests/unit/test_mcp_integrator_remove_stale.py::TestCleanCodexToml::test_skips_non_table_mcp_servers_without_rewriting	unit

How to test

• Put a literal Windows path key under [desktop.open-in-target-preferences.perPath]; get_current_config() should parse it without warning.
• Run two Codex MCP updates; the original literal-key text should remain unchanged and both servers should exist.
• Remove one stale server; only that entry should disappear while trust and preference sections remain intact.
• Run the relevant suite above; all 252 tests should pass.
• Run the CI lint mirror; all seven lint gates should pass.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[Copilot]

Pull request overview

This PR switches Codex TOML parsing/serialization to tomlkit to preserve literal-quoted Windows path keys during MCP server updates and during stale-server cleanup, addressing the data-loss / parse-rejection behavior reported in #2075.

Changes:

• Use tomlkit for Codex config.toml reads/writes to preserve literal key spelling across updates.
• Update stale MCP cleanup for TOML configs to round-trip with tomlkit and skip non-table mcp_servers.
• Add unit regression tests for Windows-style literal-quoted TOML keys and stale cleanup preservation.
• Add tomlkit>=0.13 to dependencies (pyproject.toml, uv.lock).

Show a summary per file

File	Description
src/apm_cli/adapters/client/codex.py	Switch Codex config TOML read/write to tomlkit for round-trip preservation.
src/apm_cli/integration/mcp_integrator.py	Use tomlkit for TOML stale-server cleanup with a mapping guard.
tests/unit/test_codex_adapter_phase3.py	Add regression tests covering Windows literal-quoted path keys and repeated writes.
tests/unit/test_mcp_integrator_remove_stale.py	Add tests ensuring stale cleanup preserves unrelated Windows literal-key content and skips malformed mcp_servers.
pyproject.toml	Add direct dependency on tomlkit>=0.13.
uv.lock	Lockfile update to include tomlkit.

Review details

• Files reviewed: 5/6 changed files
• Comments generated: 3
• Review effort level: Low

[danielmeppiel]

APM Review Panel: needs_rework

The tomlkit round-trip fix is sound; public orchestration coverage and Windows-safe writes must fold before ship.

cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

The core change correctly removes the lossy TOML path that caused #2075. Four real-filesystem tests prove the adapter and cleanup helpers preserve literal Windows keys. The remaining in-scope gap is an empirical test through the public MCPIntegrator.remove_stale() orchestration path, plus deterministic LF writes and safe handling of non-UTF-8 config files.

Aligned with: portable Codex configuration, secure no-write failure behavior, multi-host support, and pragmatic package-manager UX.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	2	1	tomlkit is sound; document the round-trip document contract.
CLI Logging Expert	0	0	0	No logging regressions.
DevX UX Expert	0	2	1	Add public install/cleanup flow coverage.
Supply Chain Security Expert	0	1	2	Config integrity improves; consolidate parsers separately.
OSS Growth Hacker	0	1	1	Record the Windows data-integrity fix in CHANGELOG.
Doc Writer	0	0	1	No Starlight drift; CHANGELOG needs the fix entry.
Test Coverage Expert	0	1	1	Helper tests pass; public orchestration regression trap is missing.

B = blocking-severity findings, R = recommended, N = nits. Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

1. [Test Coverage Expert] (blocking-severity) Add a real-file integration test through MCPIntegrator.remove_stale() using Windows literal path keys, then prove the regression trap with mutation-break.
2. [DevX UX Expert] (blocking-severity) Use write_text_lf in the Codex adapter so tomlkit output is written byte-for-byte on Windows.
3. [DevX UX Expert] Catch UnicodeDecodeError at both Codex TOML read sites to retain safe no-write behavior.
4. [Python Architect] Document that an existing valid config returns a round-trip-preserving TOMLDocument.
5. [OSS Growth Hacker] Add the [BUG] Windows: apm mishandles backslashes in Codex config.toml literal-quoted keys — parser rejects valid TOML, writer corrupts [projects.*] keys (data loss) #2075 fix under CHANGELOG [Unreleased].

Recommendation

Fold the five in-scope items, run the mutation-break gate and full lint contract, then re-run this panel. Consolidating unrelated legacy toml consumers is outside this PR's scope.

---

Full per-persona findings

• Python Architect: clarify the TOMLDocument contract; track unrelated parser consolidation separately.
• CLI Logging Expert: no findings.
• DevX UX Expert: missing public orchestration test; deterministic newline and failure behavior need completion.
• Supply Chain Security Expert: dependency is locked and attributed; dual-parser consolidation belongs in a follow-up.
• OSS Growth Hacker and Doc Writer: add a concise CHANGELOG entry.
• Test Coverage Expert: four real-file tests pass, but the public remove_stale flow lacks the Windows fixture.
• Auth Expert: inactive; no auth surface touched.
• Performance Expert: inactive; no performance surface touched.

_{This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.}

[danielmeppiel]

APM Review Panel: ship_now

PR #2100 adopts tomlkit round-trip writes for Codex configuration, hardens both install and stale-cleanup writes, and proves Windows literal TOML path keys survive both flows unchanged.

cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

All panelists returned zero findings on SHA 2abbeb86e13cca80f89758feccf054215983d1be (CI run 29064448578 green, 300 relevant tests pass). The change is bounded and correct: Codex configuration is loaded and dumped with tomlkit so comments, unrelated settings, and literal Windows path keys survive edits; both install and stale-cleanup use atomic LF-normalized writes at mode 0o600. Integration tests exercise the public install and cleanup orchestration. Both mutation-break probes failed when their production guards were removed and passed after restoration, confirming the regression traps are load-bearing.

Aligned with: Secure by default: atomic writes create private configuration files without a permission race. Portable by manifest: tomlkit preserves Windows backslashes and unrelated TOML content across edits. Pragmatic as npm: public integration paths carry regression coverage.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	0	The round-trip and atomic-write design is cohesive and appropriately bounded.
CLI Logging Expert	0	0	0	Failure warnings are actionable and preserve safe no-write behavior.
DevX UX Expert	0	0	0	Codex install and cleanup preserve user-managed configuration predictably.
Supply Chain Security Expert	0	0	0	Atomic mode-0600 writes address configuration integrity and privacy.
OSS Growth Hacker	0	0	0	The Windows compatibility fix removes a concrete adoption obstacle.
Doc Writer	0	0	0	The CHANGELOG entry accurately captures the user-visible fix.
Test Coverage Expert	0	0	0	Public install and cleanup paths, preservation, errors, and mutation breaks are covered.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Recommendation

Merge immediately. CI is green at SHA 2abbeb86e13cca80f89758feccf054215983d1be (run 29064448578), all 300 relevant tests pass, both mutation-break tests confirm the production guards are load-bearing, all active panelists returned zero findings, and the CHANGELOG entry is in place. No follow-ups required.

---

Full per-persona findings

Python Architect

No findings.

CLI Logging Expert

No findings.

DevX UX Expert

No findings.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

No findings.

Auth Expert -- inactive

The PR does not change authentication behavior, token management, credential resolution, or remote-host authorization.

Doc Writer

No findings.

Test Coverage Expert

No findings.

Performance Expert -- inactive

The PR does not change dependency transport, caching, materialization, parallelism, or an install/update performance hot path.

_{This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.}