Skip to content

fix(installer): stop clobbering foreign and JSONC MCP server config#441

Merged
latekvo merged 12 commits into
mainfrom
fix/installer-preserve-foreign-mcp-config
Jul 12, 2026
Merged

fix(installer): stop clobbering foreign and JSONC MCP server config#441
latekvo merged 12 commits into
mainfrom
fix/installer-preserve-foreign-mcp-config

Conversation

@latekvo

@latekvo latekvo commented Jun 30, 2026

Copy link
Copy Markdown
Member

Summary

The MCP config installer (packages/argent-installer/src/mcp-configs.ts) is supposed to only ever touch the argent entry in a user's editor MCP config. Two confirmed defects broke that contract and silently corrupted unrelated configuration. This PR fixes both across every backend (JSON, JSONC, and TOML) and adds regression coverage.

Defect A — uninstall over-prunes unrelated config

writeJsonOrRemove(value) ran the whole config through pruneEmptyConfig, which recursively drops any empty object/array anywhere in the tree, and then wrote the pruned result. So when remove() deleted the argent entry and wrote back, other MCP servers lost their args: [] / env: {} keys, and any sibling user key containing an empty object/array was deleted. The TOML-backed Codex config had the identical bug in writeTomlOrRemove.

Repro: a Cursor .cursor/mcp.json with
mcpServers: { argent: {...}, other: { command: "other-bin", args: [], env: {} } }, userSettings: { theme: "dark", overrides: {} }

  • Observed after Cursor.remove(path): other becomes just { command: "other-bin" } (its args/env gone) and userSettings.overrides is stripped.
  • Expected: other and userSettings are preserved byte-for-byte; only argent is removed.

Defect B — clobbering a JSONC config on write

.vscode/mcp.json is JSONC (VS Code allows comments and trailing commas), but the VS Code adapter read it with readJson, whose catch { return {} } turns any non-strict-JSON file into {}. write then persisted only { servers: { argent } }, destroying every pre-existing user server (and comments). The same flaw affected the other readJson → writeJson adapters — most importantly Cursor and Kiro, which are comment-tolerant VS Code forks whose mcp.json legitimately contains comments, plus Claude Code, Windsurf, and Gemini (strict JSON, but a hand-added comment would have wiped the file). The Zed and opencode adapters were already hardened with readJsonc + editJsoncFile.

Repro: a Cursor/Kiro mcp.json containing a // comment and mcpServers: { myserver: {...} }

  • Observed after the adapter's write(path, getMcpEntry()): myserver and the comment are gone.
  • Expected: myserver and the comment survive; argent is added alongside.

Fixes

  • A: writeJsonOrRemove no longer deep-prunes. It writes data unchanged, and deletes the file only when the top-level object has no own keys. Each remover collapses just its own now-empty argent container along the argent path (new deleteIfEmpty helper) before writing, so a config that held only argent still results in file deletion. Foreign servers and user keys are preserved exactly. writeTomlOrRemove received the identical non-deep-pruning treatment so the Codex TOML config is protected the same way (pruneEmptyConfig is removed entirely). Applied to the Cursor / Claude Code / Windsurf / Gemini / Kiro / Codex removers, the Cursor allowlist remover, and removeClaudePermission.
  • B: the JSONC-safe write path (read with readJsonc, mutate via editJsoncFile — path-targeted edits that preserve comments and foreign servers) now covers VS Code (servers key) and the mcpServers-keyed adapters Cursor, Kiro, Claude Code, Windsurf, and Gemini, matching the Zed/opencode adapters. write, remove, and hasArgentEntry all go through the JSONC path per adapter so a commented file round-trips cleanly on install and uninstall. JSONC is a superset of JSON, so this is safe for the strict-JSON adapters and unifies the write path; each adapter keeps its own key structure, value shape (e.g. Claude Code's type: "stdio"), and config location. The allowlist helpers (Cursor's permissions.json, and the autoApprove/alwaysAllow/trust toggles) are left on readJson/writeJson — they are a separate concern and never clobber (they early-return when the argent entry is absent), so they stay out of scope.

Tests

Added to the describe("installer preserves foreign MCP config") block in packages/argent-installer/test/mcp-configs.test.ts. Each new test fails on the original code and passes with the fix:

  • Defect A (JSON): Cursor.remove keeps a foreign server's empty env/args and a sibling user key.
  • Defect A (TOML): Codex.remove keeps a foreign server's args = [] and a sibling empty table, and deletes the file when argent was the only content.
  • Defect B (JSONC): parametrized write test over Cursor, Claude Code, Windsurf, Gemini, Kiro — a hand-authored comment and a pre-existing foreign server both survive an argent write (alongside the existing VS Code case).

Full installer suite: 309 passed. tsc --noEmit (src + tests), eslint, and prettier all clean.

latekvo added 4 commits June 30, 2026 18:18
The MCP config installer's uninstall/write paths violated the "only touch
argent" contract in two ways:

Defect A (uninstall over-prunes unrelated config): writeJsonOrRemove ran
the config through pruneEmptyConfig, which recursively dropped every empty
object/array anywhere in the tree. After remove() deleted the argent entry
and wrote back, other MCP servers lost their args:[] / env:{} keys and any
sibling user key holding an empty object/array was deleted.

Defect B (VS Code clobbers JSONC config): .vscode/mcp.json is JSONC (VS
Code allows comments and trailing commas), but the VS Code adapter read it
with readJson, whose catch returns {}; write then persisted only the argent
entry, destroying every pre-existing user server and comment.

Fix A: writeJsonOrRemove now writes data unchanged and only deletes the
file when the top-level object has no own keys; each remover collapses just
its own emptied argent container (via deleteIfEmpty) so a config that held
only argent still results in file deletion. No more deep prune.

Fix B: the VS Code adapter now reads with readJsonc and applies write/
remove via editJsoncFile, mirroring the Zed and opencode adapters, so
comments and foreign servers survive.
writeTomlOrRemove still ran the old recursive pruneEmptyConfig, the
exact "only touch argent" violation the JSON adapters were just fixed
for — silently stripping a foreign Codex server's args = [] and any
sibling empty table. Mirror writeJsonOrRemove: only delete the file
when it has no keys at all, and have codexAdapter.remove() collapse
its own emptied mcp_servers table via deleteIfEmpty first.
@latekvo latekvo marked this pull request as ready for review July 1, 2026 22:58
latekvo added 2 commits July 2, 2026 12:45
…nfigs

editJsoncFile (which six adapters' write() now route through) wrote the file
with no trailing newline, unlike the JSON/TOML writers. A fresh `argent init`
therefore created .cursor/mcp.json etc. without a final newline (git "No newline
at end of file", newline-requiring linters). Append one only when the file did
not exist before, so an existing file's own formatting is preserved untouched.
…ers (JSONC)

The foreign-config preservation suite covered the `write` path for comment
survival but only tested `remove` against strict-JSON Cursor. Add a full
install→uninstall round-trip on a hand-commented JSONC file for VS Code and the
five { mcpServers } JSONC adapters (Cursor / Claude Code / Windsurf / Gemini /
Kiro), asserting the user's comment and foreign server survive uninstall, argent
is gone, and the file is kept when a foreign server remains — the remove side of
Defect B.

@hubgan hubgan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the installer changes and drove the write/remove paths end-to-end across all backends — both defects are genuinely fixed: foreign servers and comments are preserved on install and uninstall for JSON, JSONC, and TOML, and typecheck/lint/the full suite are clean. A few inline notes below — one scope gap in the allowlist helpers that the "only touch argent" contract doesn't yet cover (pre-existing, but newly reachable via update), plus a couple of behaviors that are correct but not pinned by a test.

Comment thread packages/argent-installer/src/mcp-configs.ts Outdated
Comment thread packages/argent-installer/src/mcp-configs.ts Outdated
Comment thread packages/argent-installer/src/utils.ts Outdated
Comment thread packages/argent-installer/test/mcp-configs.test.ts
latekvo added 2 commits July 7, 2026 16:12
The allowlist helpers were the one surface still on the readJson -> writeJson
path this PR migrated everywhere else, and it clobbers foreign config:

  - cursorAdapter.addAllowlist read ~/.cursor/permissions.json with readJson,
    whose `catch { return {} }` discards a hand-commented or multi-rule file, and
    rewrote it with only { mcpAllowlist }, dropping every foreign rule and
    comment. Cursor is a comment-tolerant VS Code fork, and this became newly
    reachable through `update`: now that hasArgentEntry reads mcp.json with
    readJsonc, a commented .cursor/mcp.json is detected as configured, so update
    calls addAllowlist and touches permissions.json.
  - addClaudePermission had the same unconditional read-write shape on
    .claude/settings.json — a comment there would drop the user's other
    permissions on the next write.

Route all four allowlist/permission helpers (Cursor add/removeAllowlist,
add/removeClaudePermission) through readJsonc / editJsoncFile, matching the
adapters this PR already migrated. editJsoncFile preserves comments and foreign
keys, prunes only the edited path's empty ancestors, and deletes the file when
the document collapses to {} — so removeAllowlist / removeClaudePermission keep
the exact file-deletion semantics of the old deleteIfEmpty + writeJsonOrRemove
chain while also working on commented files. This unifies every JSON write on
editJsoncFile and lets writeJsonOrRemove (and the readJson/writeJson imports)
go; deleteIfEmpty stays for the TOML-backed Codex config.

Adds regression tests that fail on the pre-fix code (Cursor and Claude add +
install/uninstall round-trip preserve a comment and a foreign rule/permission),
plus pins three behaviors the review flagged as correct-but-untested: write()
leaves an existing no-trailing-newline file untouched, re-write() replaces a
stale env key rather than merging it, and remove() keeps the file and an empty
top-level sibling when argent was the only server.
… configs

Routing addClaudePermission / cursor.addAllowlist through editJsoncFile lost the
trailing-newline normalization the old writeJson gave for free on an empty or
whitespace-only *existing* config: editJsoncFile only appended a newline when the
file did not exist before, but a whitespace-only file (readJsoncFileRaw
substitutes it with "{}") is synthesized fresh just like an absent file, so it
was written back without a final newline — the exact git "No newline at end of
file" nit the fresh-create newline handling was added to prevent.

readJsoncFileRaw now reports whether the prior content was empty/whitespace, and
editJsoncFile appends the newline for any synthesized document (fresh file or
empty-existing) while still leaving a file with real content on its own EOL.
This also drops a redundant fs.existsSync call.

Adds pinning tests for this and three behaviors the review confirmed correct but
unpinned: removeClaudePermission keeps a permissions.deny sibling when argent was
the sole allow rule (the nested-ancestor partial-prune boundary), addClaudePermission
preserves a UTF-8 BOM, and Cursor removeAllowlist prunes an emptied mcpAllowlist
while keeping the file for a foreign top-level key.

@hubgan hubgan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few inline notes below: one test-coverage gap on the update/hasArgentEntry path plus a couple of cleanups. No blocking correctness issues — the foreign-server and JSONC/comment preservation itself holds up.

Comment thread packages/argent-installer/src/mcp-configs.ts
Comment thread packages/argent-installer/src/mcp-configs.ts
Comment thread packages/argent-installer/src/mcp-configs.ts Outdated
Comment thread packages/argent-installer/src/utils.ts
latekvo added 4 commits July 10, 2026 13:05
# Conflicts:
#	packages/argent-installer/src/mcp-configs.ts
…tection

- editJsoncFile / readJsonc JSDoc no longer steer callers to the removed
  writeJsonOrRemove or claim it is Zed-only; document it as the shared
  MCP-config write path (strict-JSON adapters included) and keep writeJson
  for whole-document rewrites that must never delete the file (~/.claude.json).
- Add a findConfiguredAdapterScopes test pinning that a commented (JSONC)
  config is detected as configured — the behavior transition from strict
  readJson (which parsed it to {} and skipped it). Fails on a readJson regression.
@latekvo latekvo merged commit f1679fe into main Jul 12, 2026
5 checks passed
@latekvo latekvo deleted the fix/installer-preserve-foreign-mcp-config branch July 12, 2026 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants