fix(mcp): preserve concurrent MCP settings during initial creation (fixes #1371) by easonLiangWorldedtech · Pull Request #1380 · Zoo-Code-Org/Zoo-Code

easonLiangWorldedtech · 2026-08-27T01:42:05Z

Tracking issue: #1385

Summary

Fixes #1371 (part of epic #1375).

MCP settings initialization has a check-then-create race: getMcpSettingsFilePath() did fileExistsAtPath() followed by an unconditional fs.writeFile of the empty stub. Two VS Code windows (separate extension-host processes) racing at startup both see the file as absent, and the second blind write truncates the first window's config to the 122-byte stub — silent data loss of all configured MCP servers.

Changes

src/services/mcp/McpHub.ts — the stub creation in getMcpSettingsFilePath() now goes through safeWriteJson with a merge callback:
- the merge read happens under the existing advisory lock, so any config written by a concurrent process after the existence check is visible to it;
- if the on-disk content has a proper mcpServers object, it is preserved; otherwise the default stub is written;
- the fast path is unchanged (file exists → no write at all), so no new writes, no mtime churn, no watcher-triggered MCP reloads.
src/services/mcp/__tests__/McpHub.spec.ts:
- regression test reproduces the interleaving: the existence check sees an absent file, but the locked read sees a concurrent window's config; asserts the creation write carries that config, not the stub. (This test fails against the old raw fs.writeFile implementation.)
- the safeWriteJson spec mock now honors options.merge (it previously ignored options, which masked merge semantics for every test in this file).

Notes

safeWriteJson was already the established pattern for this file (used by the add/remove-server flows); this change removes the one remaining raw fs.writeFile in McpHub.
No suppression count changes; eslint --max-warnings=0 clean on both touched files.
pnpm --dir src exec vitest run services/mcp/__tests__/McpHub.spec.ts → 64/64 passing.

This is an auto-generated comment: release notes by coderabbit.ai -->

Summary by CodeRabbit

Bug Fixes
- Improved reliability when creating MCP settings.
- Preserves settings added concurrently instead of overwriting them.

end of auto-generated comment: release notes by coderabbit.ai -->

Update (CodeRabbit-sync from trial #1413): head `9dd9825c4` — mcp_settings merge array guard, spec-mock production error parity, unknown-safe error.code read (trial addenda `178e6f4` + `4fc14c4`). Review context: trial PR #1413.

Review-gate re-trigger (2026-08-30): empty commit 046b78c (no code change) re-runs CI and CodeRabbit current-head review under the org new PR review gate; the code head remains 9dd9825.

…ixes Zoo-Code-Org#1371) getMcpSettingsFilePath() created the default mcp_settings.json with a check-then-write: fileExistsAtPath() followed by an unconditional fs.writeFile of the empty stub. Two windows racing at startup both saw the file as absent, and the second blind write truncated the first window's config to the 122-byte stub. The stub write now goes through safeWriteJson with a merge callback: the read happens under the advisory lock, and any config already on disk (written by a concurrent process after the existence check) is preserved instead of clobbered. The fast path (file exists -> no write) is unchanged, so no watcher-triggered reloads or write amplification. Test: regression test reproduces the interleaving (existence check sees absent file, locked read sees the concurrent config) and asserts the creation write carries the concurrent config, not the stub. The safeWriteJson spec mock now honors options.merge.

coderabbitai · 2026-08-27T01:43:19Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: c98035e8-3db4-4be8-b9f2-72e71185ff98

📥 Commits

Reviewing files that changed from the base of the PR and between 046b78c and 3519428.

📒 Files selected for processing (1)

src/services/mcp/__tests__/McpHub.settingsCreation.integration.spec.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📜 Recent review details

⚠️ CI failures not shown inline (2)

GitHub Actions: Changed-code mutation testing / 0_mutation-diff.txt: fix(mcp): preserve concurrent MCP settings during initial creation (fixes #1371)

Conclusion: failure

View job details

##[group]Run node scripts/stryker-diff.mjs ci --base "$BASE_SHA" --head "$HEAD_SHA"
 �[36;1mnode scripts/stryker-diff.mjs ci --base "$BASE_SHA" --head "$HEAD_SHA"�[0m
 shell: /usr/bin/bash -e {0}
 env:
   PNPM_HOME: /home/runner/setup-pnpm/node_modules/.bin
   STORE_PATH: /home/runner/setup-pnpm/node_modules/.bin/store/v10
   BASE_SHA: 0d937c050810ebc305b34368bf6d6ae8c1f89966
   HEAD_SHA: cdb2958d877a369c505094ab578754d07c5f2799
 ##[endgroup]
 Mutation gate failed: extension has 1009 changed executable lines (limit 500). Split the PR or obtain a maintainer-reviewed narrow exclusion.
 ##[error]Process completed with exit code 1.

GitHub Actions: Changed-code mutation testing / mutation-diff: fix(mcp): preserve concurrent MCP settings during initial creation (fixes #1371)

Conclusion: failure

View job details

##[group]Run node scripts/stryker-diff.mjs ci --base "$BASE_SHA" --head "$HEAD_SHA"
 �[36;1mnode scripts/stryker-diff.mjs ci --base "$BASE_SHA" --head "$HEAD_SHA"�[0m
 shell: /usr/bin/bash -e {0}
 env:
   PNPM_HOME: /home/runner/setup-pnpm/node_modules/.bin
   STORE_PATH: /home/runner/setup-pnpm/node_modules/.bin/store/v10
   BASE_SHA: 0d937c050810ebc305b34368bf6d6ae8c1f89966
   HEAD_SHA: cdb2958d877a369c505094ab578754d07c5f2799
 ##[endgroup]
 Mutation gate failed: extension has 1009 changed executable lines (limit 500). Split the PR or obtain a maintainer-reviewed narrow exclusion.
 ##[error]Process completed with exit code 1.

🧰 Additional context used

📓 Path-based instructions (6)

Check persistence and lifecycle invariants: awaited atomic writes, rollback or explicit partial-failure behavior, cross-window state consistency, stale listeners/watchers, cancellation, idempotency, and safe restart/resume without lost or d...