fix(mcp): preserve concurrent MCP settings during initial creation (fixes #1371) - #1380
Conversation
…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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughMCP settings initialization now uses merge-aware ChangesMCP settings preservation
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Merge Risk: ⚪ Minimal · up to This localized change preserves concurrent MCP configuration during initial settings creation and includes a passing regression test; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. Full details: Description checkExplanation The description provides issue references, implementation details, regression-test coverage, lint results, and test results. It does not reproduce every template heading or checklist item, but it contains the required information and is mostly complete. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Codecov reported 2 patch lines (1 missing, 1 partial) in the safeWriteJson merge callback. Add the three remaining fallback cases: absent file (merge sees null), existing content without an mcpServers object, and mcpServers present but not an object - all must write the default stub. All changed lines and branches of the merge callback are now covered.
|
Patch coverage gap (Codecov: 2 lines, 1 missing + 1 partial in the merge callback) addressed in 332253a: added the three remaining fallback cases of getMcpSettingsFilePath (absent file, existing content without an mcpServers object, and mcpServers present but not an object). All changed lines and all merge-callback branches are now covered locally (v8 lcov: lines 519/520/521/523 all hit; BRDA outcomes all > 0); 67/67 McpHub spec tests pass. |
github-actions
Bot
added
the
awaiting-review
…, unknown-safe code read The mcp_settings merge callback now requires a plain object (!Array.isArray), so an existing mcpServers: [] is replaced by the empty stub instead of being preserved and later rejected by McpSettingsSchema; the safeWriteJson spec mock mirrors the production merge contract (only ENOENT and SyntaxError are recoverable, any other read failure rejects before the merge callback runs, with an EACCES regression); the error.code read in the mock uses an unknown-safe type guard instead of an object cast. (CodeRabbit findings on trial Zoo-Code-Org#1413).
2 participants
Summary
Fixes #1371 (part of epic #1375).
MCP settings initialization has a check-then-create race:
getMcpSettingsFilePath()didfileExistsAtPath()followed by an unconditionalfs.writeFileof 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 ingetMcpSettingsFilePath()now goes throughsafeWriteJsonwith amergecallback:mcpServersobject, it is preserved; otherwise the default stub is written;src/services/mcp/__tests__/McpHub.spec.ts:fs.writeFileimplementation.)safeWriteJsonspec mock now honorsoptions.merge(it previously ignored options, which masked merge semantics for every test in this file).Notes
- No suppression count changes;
This is an auto-generated comment: release notes by coderabbit.ai -->safeWriteJsonwas already the established pattern for this file (used by the add/remove-server flows); this change removes the one remaining rawfs.writeFileinMcpHub.eslint --max-warnings=0clean on both touched files.pnpm --dir src exec vitest run services/mcp/__tests__/McpHub.spec.ts→ 64/64 passing.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.