Repository navigation
fix(mcp): preserve unindented examples in Codex TOML - #1651
Conversation
|
No confident findings in the specified PR diff. The testing record satisfies the review criteria: it documents typecheck, lint, focused unit tests, and a built real-CLI E2E run covering inject, update, and removal. The reported Windows limitations and unchanged-base failure are not blocking findings. Review was read-only; I did not run, build, or install anything from the PR. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Approving at 42a6ef4f. Header-like lines are now masked whether or not they are indented, using the same complete-prefix check #1626 added, so an unindented [mcp_servers.…] inside a multiline value is no longer taken for a table.
Built and run from source, base 9f9f36b4 vs this head, isolated home and Codex home:
"""/''' example with an unindented [mcp_servers.team-mcp], fresh config
base: inject/update/remove exit 0, team server skipped each time
head: added / updated / removed; after remove the file equals the original
example above a managed block
base: update exit 0 but leaves the string unterminated (codex mcp list: failed to load configuration); remove exit 1
head: update keeps the example; remove leaves the file equal to the original
managed server value holding a [projects.example] line, team drops the server
base: removed, config left invalid
head: removed, valid TOML
codex mcp list (codex-cli 0.165.0) on the head configs: lists team-mcp
The masking agrees with a TOML parser on escaped quotes, ''' holding """, """" closings, comments with quotes and CRLF.
- [P3 nit] The prefix parse now runs for every
[-led line; injecting 5 servers into a 2,000-line config went from 0.18 s to 0.39 s.
An unindented table example inside a Codex TOML multiline string (for example in `developer_instructions`) is no longer taken for a real table: `mcp inject` no longer skips or overwrites the server, and update and remove keep the member's text. A table-like line inside a managed server's value no longer leaves the config invalid when the server is removed. Header-like lines are masked with the complete-prefix check from Tencent#1626, indented or not.
Summary
An unindented table example inside a Codex TOML multiline string is mistaken for a real table. For example,
mcp injectcan replace[mcp_servers.team-mcp]insidedeveloper_instructions, destroying the closing quotes and the member's instructions. A table-like line inside a server value can also truncate the block when it is removed.Check the TOML prefix before recognizing unindented headers too, using the same guard introduced for indented headers in #1626. Keep the original text for splicing.
Test plan
npm run typecheckandnpm run lintpassed.npx vitest run src/__tests__/mcp-reconcile.test.ts src/__tests__/local-agent-mcp.test.ts -t '^(?!.*symlinked Codex project config)(?!.*allows retrying uninstall of codex user after a manifest write fails)(?!.*keeps Codex config and ownership when .* cannot read the config).*(Codex|codex|spliceCodexBlock)' --maxWorkers=2 --reporter=dot --silent).npm run test:e2e -- src/__tests__/e2e/mcp-command-errors.test.ts --maxWorkers=1 --reporter=dotbuilt the CLI and passed all 13 tests on Windows. It injects, updates and removes a server while preserving both indented and unindented examples in member instructions.npx vitest run --maxWorkers=4 --reporter=dot --silent --bail=1) stopped after 85 passed and 12 skipped atworkflow-workspace.test.ts's dirty-file mode test; the same test fails on the unchanged base9f9f36b4on this machine. The full MCP test file also stopped on WindowsEPERMcreating a symlink after 44 passed and 7 skipped; symlink privileges are unavailable here.Related
#1626 explicitly left unindented multiline examples as a separate case. Dotted-key MCP declarations remain outside this change.