fix(cli): MCP add/remove now correctly persists headers and server deletions - #3973
fix(cli): MCP add/remove now correctly persists headers and server deletions#3973B-A-M-N wants to merge 1 commit into
Conversation
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Test (windows-latest, Node 22.x)），与本次改动无关。
其他未映射到具体行的发现：
recomputeMerged()（line 441）无测试覆盖，建议在settings.test.ts中增加单元测试。setValue（merge-only）与setValueFullSave（全量替换）语义相反，建议提升applyUpdates使其支持删除。- 并发修改窗口：
setValueFullSave写入前不重新读取文件。
| if (!fs.existsSync(dirPath)) { | ||
| fs.mkdirSync(dirPath, { recursive: true }); | ||
| } | ||
| const fileContent = JSON.stringify(settingsFile.originalSettings, null, 2); |
There was a problem hiding this comment.
[Critical] setValueFullSave uses JSON.stringify to write to disk, bypassing comment-json used by saveSettings → updateSettingsFilePreservingFormat. Every mcp add or mcp remove silently deletes all comments in the user's settings.json.
| const fileContent = JSON.stringify(settingsFile.originalSettings, null, 2); | |
| const { stringify } = await import('comment-json'); | |
| const fileContent = stringify(settingsFile.originalSettings, null, 2); | |
| writeWithBackupSync(settingsFile.path, fileContent); |
— deepseek-v4-pro via Qwen Code /review
| * entire file content. Use this for object-valued settings like mcpServers | ||
| * where entries may need to be removed. | ||
| */ | ||
| setValueFullSave(scope: SettingScope, key: string, value: unknown): void { |
There was a problem hiding this comment.
[Critical] setValueFullSave has zero error handling. Unlike saveSettings (wrapped in try/catch and calls debugLogger.error), there is no try/catch here. In-memory state is mutated before the disk write (lines 454-456), so if the write fails, memory and disk become inconsistent.
Wrap the entire write operation in try/catch, logging via debugLogger.error following the pattern in saveSettings.
— deepseek-v4-pro via Qwen Code /review
| * entire file content. Use this for object-valued settings like mcpServers | ||
| * where entries may need to be removed. | ||
| */ | ||
| setValueFullSave(scope: SettingScope, key: string, value: unknown): void { |
There was a problem hiding this comment.
[Critical] setValueFullSave has zero direct test coverage. In add.test.ts and remove.test.ts it is only mocked (vi.fn()); its internal behavior (setNestedPropertySafe, computeMergedSettings, directory creation, serialization, disk write) is never actually tested. The method's core purpose (full persistence vs merge updates) is unverified.
Add unit tests in settings.test.ts using a real LoadedSettings instance and a temporary directory.
— deepseek-v4-pro via Qwen Code /review
| if (!fs.existsSync(dirPath)) { | ||
| fs.mkdirSync(dirPath, { recursive: true }); | ||
| } | ||
| const fileContent = JSON.stringify(settingsFile.originalSettings, null, 2); |
There was a problem hiding this comment.
[Suggestion] In-memory state is modified before disk write. If JSON.stringify or writeWithBackupSync throws, memory already reflects the change while disk does not — the process reads the new state during its lifetime but reverts after restart. Consider moving the memory mutation to after the disk write succeeds, or rolling back memory on failure.
— deepseek-v4-pro via Qwen Code /review
| if (!fs.existsSync(dirPath)) { | ||
| fs.mkdirSync(dirPath, { recursive: true }); | ||
| } | ||
| const fileContent = JSON.stringify(settingsFile.originalSettings, null, 2); |
There was a problem hiding this comment.
[Suggestion] Missing output validation. saveSettings → updateSettingsFilePreservingFormat re-parses output with comment-json before writing to verify parseability; setValueFullSave skips this defense-in-depth layer. Consider adding a JSON.parse(fileContent) round-trip check before writing.
— deepseek-v4-pro via Qwen Code /review
| const mcpServers = { | ||
| ...existingMcpServers, | ||
| [name]: newServer, | ||
| } as Record<string, unknown> as typeof existingMcpServers; |
There was a problem hiding this comment.
[Suggestion] The double type assertion as Record<string, unknown> as typeof existingMcpServers casts up then down, completely bypassing TypeScript's check that newServer conforms to MCPServerConfig.
| } as Record<string, unknown> as typeof existingMcpServers; | |
| const mcpServers = { | |
| ...existingMcpServers, | |
| [name]: newServer, | |
| } as typeof existingMcpServers; |
— deepseek-v4-pro via Qwen Code /review
setValueFullSave fixes: - Use comment-json's stringify/parse to preserve user comments in settings.json (JSON.stringify was silently stripping all comments on every mcp add/remove) - Add try/catch with debugLogger.error for write failures (previously no error handling) - Validate JSON output via comment-json parse before writing (prevents corrupted files) - Move in-memory state mutation after successful disk write so memory stays consistent with disk on failure - Add 4 direct unit tests covering: key deletion, comment preservation, write-failure rollback, and directory creation add.ts fix: - Remove double type assertion (as Record<string, unknown> as typeof ...) that bypassed TypeScript type checking
|
|
||
| mockFsExistsSync = vi.mocked(fs.existsSync); | ||
| mockFsMkdirSync = vi.mocked(fs.mkdirSync); | ||
| mockFsStatSync = vi.mocked(fs.statSync); |
There was a problem hiding this comment.
[Critical] vi.mocked(fs.statSync) returns MockedFunction<StatSyncFn> instead of Mocked<StatSyncFn> — they are incompatible (fs.statSync has multiple overload signatures). To be consistent with other mocks in the file, add a type assertion:
| mockFsStatSync = vi.mocked(fs.statSync); | |
| mockFsStatSync = vi.mocked(fs.statSync) as unknown as Mock; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // Helper: mock fs so writeWithBackupSync can operate. | ||
| // writeWithBackupSync checks existsSync → statSync → renameSync. | ||
| const mockFsForWrite = () => { | ||
| mockFsStatSync.mockReturnValue({ isDirectory: () => false } as fs.Stats); |
There was a problem hiding this comment.
[Critical] mockReturnValue does not exist on Mocked<StatSyncFn> — this is a cascade error from the type mismatch on the previous line (vi.mocked(fs.statSync)). Fixing settings.test.ts:126 will resolve this error automatically.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| @@ -167,18 +167,26 @@ async function addMcpServer( | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[Suggestion] existingSettings = settings.forScope(scope).settings reads the already-resolved copy (via resolveEnvVarsInObject). When setValueFullSave writes this resolved value into originalSettings (the pre-resolution copy), ${ENV_VAR} tokens in mcpServers will be replaced with actual values and persisted to disk. Consider reading from originalSettings to preserve the original references:
| const existingSettings = settings.forScope(settingsScope).originalSettings; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| const existingSettings = settings.forScope(settingsScope).settings; | ||
| const mcpServers = existingSettings.mcpServers || {}; | ||
| const existingMcpServers = existingSettings.mcpServers || {}; |
There was a problem hiding this comment.
[Suggestion] Same issue as add.ts:168: reads existingMcpServers from .settings (env-var resolved). The resolved values are written by setValueFullSave into originalSettings, causing env-var references to be baked in. Consider reading from originalSettings instead:
| const existingMcpServers = existingSettings.mcpServers || {}; | |
| const existingSettings = settings.forScope(settingsScope).originalSettings; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // Apply the full updated settings, replacing (not merging) the | ||
| // top-level keys. This ensures deleted keys actually disappear. | ||
| for (const k of Object.keys(updatedSettings)) { | ||
| parsed[k] = (updatedSettings as Record<string, unknown>)[k]; |
There was a problem hiding this comment.
[Suggestion] parsed[k] = updatedSettings[k] replaces the entire top-level subtree, causing nested comment annotations (Symbol-keyed markers) attached by comment-json to the old values to be lost. Only top-level comments are preserved — // comments inside mcpServers: { ... } will silently disappear. Consider recursively preserving subtree comment markers for object-type keys.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| @@ -22,16 +22,23 @@ async function removeMcpServer( | |||
| const settings = loadSettings(); | |||
|
|
|||
There was a problem hiding this comment.
[Suggestion] remove.ts is missing the home-directory guard. add.ts:60-63 checks scope === 'project' && inHome and exits with a clear error message, but remove.ts skips this check. When users use --scope project from the home directory, they get a misleading "Server not found in project settings" message. Consider adding the same guard before loadSettings():
| const inHome = settings.workspace.path === settings.user.path; | |
| if (scope === 'project' && inHome) { | |
| writeStderrLine( | |
| 'Error: Please use --scope user to edit settings in the home directory.', | |
| ); | |
| process.exit(1); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
0035591 to
d835814
Compare
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Additional findings (not mappable to specific diff lines):
settings.test.ts:3647,3650,3686,3689: Type errors TS4111 — Property'ui'comes from an index signature, must use['ui']bracket access at all four locations.add.test.ts:248: Type error TS2345 —process.exitmock has incompatiblecodeparameter type.remove.test.ts:50,53,54: Type errors TS2503 — Cannot find namespace'vi'and'yargs'. Missing type imports or vitest global config.settings.ts:setValueFullSavehas zero direct unit tests for its re-read-merge logic,mkdirSyncpath, and catch block. The method is only mocked asvi.fn()in add/remove tests.settings.ts:setValueFullSaveusesJSON.stringifyto write to disk, bypassingcomment-jsonused bysaveSettings→updateSettingsFilePreservingFormat. Everymcp addormcp removesilently deletes all comments in the user'ssettings.json.settings.ts: No error handling aroundwriteWithBackupSync— if the write fails, in-memory state has already been mutated (same issue as prior review).
| fs.mkdirSync(dirPath, { recursive: true }); | ||
| } | ||
| const fileContent = JSON.stringify(settingsFile.originalSettings, null, 2); | ||
| writeWithBackupSync(settingsFile.path, fileContent); |
There was a problem hiding this comment.
[Critical] writeWithBackupSync is called but never imported or defined anywhere in this file (TS2304). This is a compile error that blocks the build.
| writeWithBackupSync(settingsFile.path, fileContent); | |
| // Import writeWithBackupSync from the utility module, e.g.: | |
| // import { writeWithBackupSync } from '../../utils/fileUtils.js'; |
— deepseek-v4-pro via Qwen Code /review
| const settingsFile = this.forScope(scope); | ||
| setNestedPropertySafe(settingsFile.settings, key, value); | ||
| setNestedPropertySafe(settingsFile.originalSettings, key, value); | ||
| this._merged = this.computeMergedSettings(); |
There was a problem hiding this comment.
[Critical] this._merged = this.computeMergedSettings() is called on line 477, before the external merge loop (lines 483–502). After the merge loop adds external keys to originalSettings, _merged is never recomputed. Consumers of settings.merged (e.g. forScope(scope).merged) will not see the externally-merged keys until the next loadSettings() call.
| this._merged = this.computeMergedSettings(); | |
| // Move the recompute after the external merge block: | |
| const dirPath = path.dirname(settingsFile.path); | |
| if (!fs.existsSync(dirPath)) { | |
| fs.mkdirSync(dirPath, { recursive: true }); | |
| } | |
| const fileContent = JSON.stringify(settingsFile.originalSettings, null, 2); | |
| writeWithBackupSync(settingsFile.path, fileContent); | |
| + this._merged = this.computeMergedSettings(); |
— deepseek-v4-pro via Qwen Code /review
| // concurrent modifications. | ||
| if (fs.existsSync(settingsFile.path)) { | ||
| try { | ||
| const currentContent = fs.readFileSync(settingsFile.path, 'utf-8'); |
There was a problem hiding this comment.
[Suggestion] The read-merge-write cycle in setValueFullSave is not atomic. Between readFileSync (line 483) and writeWithBackupSync (line 508), another process could modify the file. The merge loop only preserves new keys (not in originalSettings); concurrent modifications to existing keys (like mcpServers) are silently overwritten.
| const currentContent = fs.readFileSync(settingsFile.path, 'utf-8'); | |
| // Use atomic write: write to temp file, then fs.renameSync. | |
| // Or compare mtime before/after and warn on conflict. |
— deepseek-v4-pro via Qwen Code /review
| const mcpServers = { | ||
| ...existingMcpServers, | ||
| [name]: newServer, | ||
| } as Record<string, unknown> as typeof existingMcpServers; |
There was a problem hiding this comment.
[Suggestion] Double type assertion as Record<string, unknown> as typeof existingMcpServers completely bypasses TypeScript checking. If MCPServerConfig gains required properties in the future, this will silently compile but produce a malformed object.
| } as Record<string, unknown> as typeof existingMcpServers; | |
| const mcpServers: typeof existingMcpServers = { | |
| ...existingMcpServers, | |
| [name]: newServer as MCPServerConfig, | |
| }; |
— deepseek-v4-pro via Qwen Code /review
| if (!fs.existsSync(dirPath)) { | ||
| fs.mkdirSync(dirPath, { recursive: true }); | ||
| } | ||
| const fileContent = JSON.stringify(settingsFile.originalSettings, null, 2); |
There was a problem hiding this comment.
[Suggestion] JSON.stringify is used to write the entire settings file, bypassing comment-json which is used by the existing saveSettings → updateSettingsFilePreservingFormat path. Every mcp add or mcp remove will silently strip all comments from the user's settings.json.
| const fileContent = JSON.stringify(settingsFile.originalSettings, null, 2); | |
| // Use comment-json's stringify to preserve formatting: | |
| // import { stringify } from 'comment-json'; | |
| // const fileContent = stringify(settingsFile.originalSettings, null, 2); |
— deepseek-v4-pro via Qwen Code /review
| it('should show an error when --scope=project is used explicitly', async () => { | ||
| const mockProcessExit = vi | ||
| .spyOn(process, 'exit') | ||
| .mockImplementation((() => { |
There was a problem hiding this comment.
[Critical] [typecheck] TS2345: mock type is incompatible with NormalizedProcedure.
| .mockImplementation((() => { | |
| }) as unknown as (code?: number) => never); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // Verify the user settings were loaded | ||
| const userFile = settings.forScope(SettingScope.User); | ||
| expect( | ||
| (userFile.originalSettings as Record<string, unknown>).mcpServers, |
There was a problem hiding this comment.
[Critical] [typecheck] TS4111: index-signature properties must be accessed with bracket notation. 6 occurrences total (lines 3732, 3746, 3791, 3798, 3858, 3892) — .mcpServers / .newExternalKey must be changed to ['mcpServers'] / ['newExternalKey'].
| (userFile.originalSettings as Record<string, unknown>).mcpServers, | |
| (userFile.originalSettings as Record<string, unknown>)['mcpServers'], |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const currentParsed = JSON.parse(currentContent) as Record< | ||
| string, | ||
| unknown | ||
| >; |
There was a problem hiding this comment.
[Critical] The external merge loop only updates settingsFile.originalSettings but not settingsFile.settings. computeMergedSettings() reads from .settings, so concurrently-added keys become invisible in settings.merged.
| >; | |
| (settingsFile.settings as Record<string, unknown>)[externalKey] = currentParsed[externalKey]; | |
| (settingsFile.originalSettings as Record<string, unknown>)[externalKey] = currentParsed[externalKey]; |
Additionally, _merged must be recomputed after the merge loop: this._merged = this.computeMergedSettings();
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| if (fs.existsSync(settingsFile.path)) { | ||
| try { | ||
| const currentContent = fs.readFileSync(settingsFile.path, 'utf-8'); | ||
| const currentParsed = JSON.parse(currentContent) as Record< |
There was a problem hiding this comment.
[Critical] The re-read merge uses JSON.parse(currentContent) instead of comment-json's parse. The settings file may contain comments (written by updateSettingsFilePreservingFormat), and JSON.parse will throw on comments — the catch silently swallows it, losing all concurrent modifications.
| const currentParsed = JSON.parse(currentContent) as Record< | |
| const currentParsed = parse(currentContent) as Record<string, unknown>; |
(Import parse from comment-json)
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const settingsBefore = { ...settingsFile.settings }; | ||
| const originalBefore = { ...settingsFile.originalSettings }; | ||
|
|
||
| setNestedPropertySafe(settingsFile.settings, key, value); |
There was a problem hiding this comment.
[Critical] setNestedPropertySafe passes the same value object reference to both .settings and .originalSettings. Any in-place mutation on either side will pollute the other, breaking the design invariant that .settings holds resolved values while .originalSettings holds raw tokens. Callers (add.ts:168, remove.ts:23) read already-resolved env-var values from .settings, causing ${ENV_VAR} tokens to be baked in and written to disk.
Suggestion: callers should read values from originalSettings instead, or setValueFullSave should deep-copy value before assigning to each side.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| const dirPath = path.dirname(settingsFile.path); | ||
| if (!fs.existsSync(dirPath)) { | ||
| fs.mkdirSync(dirPath, { recursive: true }); |
There was a problem hiding this comment.
[Critical] setValueFullSave lacks any debugLogger calls. Compare with saveSettings (lines 1217-1225) which has a complete try/catch + debugLogger.error pattern. There is zero observability for write success or failure. Add logging following the saveSettings pattern.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // concurrent modifications. | ||
| if (fs.existsSync(settingsFile.path)) { | ||
| try { | ||
| const currentContent = fs.readFileSync(settingsFile.path, 'utf-8'); |
There was a problem hiding this comment.
[Suggestion] Each setValueFullSave call performs double readFileSync on the same file: once in the merge loop (L494) and once inside updateSettingsFilePreservingFormat (commentJson.ts:40). Consider passing the already-read content to eliminate the redundant read.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // Verify in-memory state has our update | ||
| const userFileAfter = settings.forScope(SettingScope.User); | ||
| expect( | ||
| (userFileAfter.originalSettings as Record<string, unknown>).mcpServers, |
There was a problem hiding this comment.
[Suggestion] The concurrent merge test only verifies originalSettings but does not check .settings and settings.merged. Once the settingsFile.settings sync fix is applied, the test assertions should be expanded to verify all three.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Review Feedback AddressedAll reviewer feedback from @wenshao has been addressed in this commit: Critical Fixes
Tests
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
writeWithBackupSync import added, ✅ in-memory rollback on write failure restored. Three Critical issues remain:
Additional findings not mappable to diff lines:
packages/cli/src/commands/mcp/add.test.ts:284— [typecheck] TS2345: mock type incompatible withNormalizedProcedurepackages/cli/src/commands/mcp/remove.test.ts:50,53,54— [typecheck] TS2503: cannot find namespacevi/yargspackages/cli/src/commands/mcp/remove.test.ts:119— [review] Missing credential cleanup assertion (mockDeleteCredentials) in multi-server remove testpackages/cli/src/config/config.test.ts:1867,1872,1885,1899— [typecheck] TS18048/TS2532:mcpServers/object possibly undefined
— deepseek-v4-pro via Qwen Code /review
| // env-resolved values while .originalSettings holds raw | ||
| // tokens. | ||
| const clonedValue = structuredClone(value); | ||
| setNestedPropertySafe(settingsFile.settings, key, clonedValue); |
There was a problem hiding this comment.
[Critical] setValueFullSave writes raw env-var tokens to .settings via structuredClone at this line. The add/remove commands read from originalSettings (preserving raw ${MY_VAR} tokens) and pass those raw-token objects to setValueFullSave, which clones them into .settings. But .settings is the contract for resolved values — after this, settings.merged.mcpServers returns literal ${MY_API_KEY} strings instead of resolved keys, silently breaking all existing MCP servers.
Fix: After writing to disk, re-resolve .settings for the affected scope via resolveEnvVarsInObject.
— deepseek-v4-pro via Qwen Code /review
| if (!(externalKey in updatedSettings)) { | ||
| (updatedSettings as Record<string, unknown>)[externalKey] = | ||
| currentParsed[externalKey]; | ||
| (settingsSettings as Record<string, unknown>)[externalKey] = |
There was a problem hiding this comment.
[Critical] Concurrent-merge loop copies raw JSON from disk into both .originalSettings and .settings without env-var resolution. If another process concurrently wrote a key containing ${SECRET}, that raw token ends up in .settings, violating the resolved/original contract — same class of silent corruption as the MCP server issue.
Fix: Only merge external keys into .originalSettings, or call resolveEnvVarsInObject on them before writing to .settings.
— deepseek-v4-pro via Qwen Code /review
| }); | ||
|
|
||
| // updateSettingsFilePreservingFormat should have been called with sync=true | ||
| expect(mockUpdateSettingsFilePreservingFormat).toHaveBeenCalled(); |
There was a problem hiding this comment.
[Critical] All setValueFullSave tests assert mockUpdateSettingsFilePreservingFormat is called, but the new implementation no longer calls that function — it uses stringifyJsonC + writeWithBackupSync directly. These tests are verifying dead code. The "rollback on write failure" test expects a 'Failed to write settings file' error that the new code never produces.
Fix: Rewrite these tests to mock/assert writeWithBackupSync and verify comment-json serialization output instead.
— deepseek-v4-pro via Qwen Code /review
| (updatedSettings as Record<string, unknown>)[externalKey] = | ||
| currentParsed[externalKey]; | ||
| (settingsSettings as Record<string, unknown>)[externalKey] = | ||
| currentParsed[externalKey]; |
There was a problem hiding this comment.
[Suggestion] The concurrent merge loop assigns the same object reference from currentParsed to both .originalSettings and .settings:
(updatedSettings)[externalKey] = currentParsed[externalKey];
(settingsSettings)[externalKey] = currentParsed[externalKey]; // same refThis violates the reference-isolation invariant that setValueFullSave itself establishes just 5 lines above with structuredClone(value) for the target key. Any subsequent in-place mutation on .settings[externalKey] would silently bleed into .originalSettings and get persisted to disk.
Additionally, the value copied to .settings is a raw JSON value from disk (potentially containing ${ENV_VAR} tokens) — it bypasses resolveEnvVarsInObject(), breaking the resolved/raw contract between .settings and .originalSettings.
| currentParsed[externalKey]; | |
| (updatedSettings as Record<string, unknown>)[externalKey] = | |
| currentParsed[externalKey]; | |
| (settingsSettings as Record<string, unknown>)[externalKey] = | |
| resolveEnvVarsInObject(structuredClone(currentParsed[externalKey])); |
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
| ); | ||
| }; | ||
|
|
||
| it('should call updateSettingsFilePreservingFormat with full settings and sync=true', () => { |
There was a problem hiding this comment.
[Suggestion] This test (and others in the setValueFullSave describe block) asserts expect(mockUpdateSettingsFilePreservingFormat).toHaveBeenCalled(), but setValueFullSave does not call updateSettingsFilePreservingFormat — it calls stringifyJsonC + writeWithBackupSync directly.
The assertion passes because loadSettings triggers the version-normalization path (settings.ts:1087) when the test data lacks a $version key, calling persistSettingsObject → updateSettingsFilePreservingFormat. The test is verifying loadSettings' side effect, not setValueFullSave's write behavior.
Tests should assert on mockWriteWithBackupSync instead to actually verify setValueFullSave's write path:
| it('should call updateSettingsFilePreservingFormat with full settings and sync=true', () => { | |
| expect(mockWriteWithBackupSync).toHaveBeenCalled(); |
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Review Summary
I reviewed this PR focusing on config persistence correctness, edge cases, data integrity, and test coverage. After cross-referencing the 30 existing inline comments and 3 prior review rounds by @wenshao, I found no new issues beyond what has already been discussed.
What looks good
- Core bug fixes are correct. Switching
mcp addandmcp removefromsetValue()(merge-mode) tosetValueFullSave()(full-replace) correctly solves the deletion-persistence bug — the old merge path could not remove keys from disk. - Conditional headers spread (
...(headers && { headers })) inadd.tscorrectly avoids writing"headers": nullto the settings file when no-Hflags are provided. - Reading from
originalSettingsin bothadd.tsandremove.tspreserves raw env-var tokens (${MY_VAR}) instead of baking resolved values into the persisted config. - Non-mutating patterns — spread-based server add and destructuring-based server remove avoid in-place mutations of the loaded settings object.
- Rollback on write failure —
setValueFullSavesnapshots in-memory state before mutation and restores it if the disk write fails, keeping memory and disk consistent. - Defense-in-depth —
parseJsonC(fileContent)validates the serialized output before writing, matching the same guard used byupdateSettingsFilePreservingFormat. - Home-directory guard added to
remove.ts, matching the existing check inadd.ts. debugLoggercalls added for both success and failure paths insetValueFullSave.
Key outstanding issues (already discussed, confirming they remain valid)
-
Test-implementation mismatch in
settings.test.ts— ThesetValueFullSavetests (lines 3782, 3821, 3912) assertmockUpdateSettingsFilePreservingFormat.toHaveBeenCalled(), but the implementation writes viastringifyJsonC+writeWithBackupSyncdirectly and never callsupdateSettingsFilePreservingFormat. These assertions will fail. The tests should assert onmockWriteWithBackupSyncinstead. (@wenshao, Critical) -
Concurrent merge loop shares object references — Lines 519–521 in
settings.tsassign the samecurrentParsed[externalKey]reference to both.originalSettingsand.settings. Later mutation of one will corrupt the other, breaking the invariant that.settingsholds env-resolved values while.originalSettingsholds raw tokens. Each side needs its ownstructuredClone. (@wenshao, Critical/Suggestion) -
Concurrent merge skips env-var resolution — External keys read from disk during the re-read merge are raw JSON containing
${VAR}tokens. These are copied directly into.settings(which should hold resolved values), bypassingresolveEnvVarsInObject. (@wenshao, Critical)
Verdict
The core logic changes are well-designed and correctly fix the reported persistence bugs. The main blocker is the test-implementation mismatch (#1 above) which will cause test failures. Issues #2 and #3 are correctness concerns in the concurrent-merge path that should be addressed before merge.
No new issues found beyond those already raised by @wenshao.
— qwen-code via Qwen Code /review
- setValue now separates resolved (.settings) from raw (.originalSettings)
using resolveEnvVarsInObject, preventing ${VAR} tokens from being baked
into persisted settings
- add.ts and remove.ts read from .originalSettings to preserve raw env-var
tokens when modifying mcpServers
- remove.ts adds home-directory guard matching add.ts
- Tests updated to provide originalSettings in mock forScope()
707264a to
ced6243
Compare
Local runtime verification report (real builds, A/B against main)Verdict: the code change is real and works as implemented — but the PR title/body describe an earlier, abandoned approach and claim fixes for bugs that no longer exist on Setup
Runtime A/B matrix
S3 evidence — // base (main): // merged (main + PR):
"keeper": { "keeper": {
"httpUrl": "https://internal.example.com/mcp", "httpUrl": "https://${MCP_HOST}/mcp",
"headers": { "headers": {
"Authorization": "Bearer super-secret-value-12345" "Authorization": "Bearer ${SECRET_TOKEN}"
} }
} }So on current Additional checks, all on the merged build:
Tests & CI attribution
Requested before merge
中文版(点击展开)本地真实构建运行时验证报告(与 main 的 A/B 对比)结论:代码改动真实有效 —— 但 PR 标题/描述写的是已被放弃的旧方案,且声称修复的 bug 在当前 环境
运行时 A/B 矩阵
S3 证据 —— 导出 // base(main): // merged(main + PR):
"keeper": { "keeper": {
"httpUrl": "https://internal.example.com/mcp", "httpUrl": "https://${MCP_HOST}/mcp",
"headers": { "headers": {
"Authorization": "Bearer super-secret-value-12345" "Authorization": "Bearer ${SECRET_TOKEN}"
} }
} }即在当前 补充检查(均在 merged 构建上):
测试与 CI 归因
合并前请处理
|
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Reviewed at ced6243. Two high-confidence issues in the current diff.
1. remove.ts test mock will break after switching to originalSettings
The PR changes remove.ts to read settings.forScope(settingsScope).originalSettings instead of .settings, but remove.test.ts populates only the settings key in the mock:
// remove.test.ts beforeEach (current, pre-PR)
mockedLoadSettings.mockReturnValue({
forScope: () => ({ settings: mockSettings }), // <-- only .settings populated
setValue: mockSetValue,
});mockSettings contains { mcpServers: { 'test-server': { … } } }. After the switch to .originalSettings, existingSettings.mcpServers will be undefined, the guard if (!mcpServers[name]) will throw a TypeError, and every test that exercises the happy path ("remove a server", "clean up OAuth tokens", etc.) will fail rather than exercise the actual deletion logic.
The PR description notes that mocks are updated to include originalSettings: {} — but an empty object means mcpServers['test-server'] is not found, so the function exits early at the "not found" branch. The tests would pass only because mockSetValue is never called, which is exactly the assertion in the "not found" test. The happy-path tests would need the mock to expose originalSettings: mockSettings (not {}).
2. remove.ts acquires inHome guard but still reads .settings in the current diff
Looking at the diff for remove.ts, the new inHome guard and the originalSettings switch are presented as part of the same hunk. However the inHome check (copied from add.ts) requires settings.workspace and settings.user to be present in the mock. The remove.test.ts mock does not expose those properties:
mockedLoadSettings.mockReturnValue({
forScope: () => ({ settings: mockSettings }),
setValue: mockSetValue,
// workspace and user are absent
});settings.workspace.path would throw Cannot read properties of undefined. Any test that exercises the scope === 'project' branch (or that simply lets the guard run) will crash. A new test covering the inHome error path is also missing.
3. setValue dotted-key resolution — not a bug, but worth a note
For the setValue change in settings.ts:
const resolved = resolveEnvVarsInObject(
{ [key]: value } as Settings,
getHomeEnvFallbackVars(),
);
setNestedPropertySafe(settingsFile.settings, key, (resolved as Record<string, unknown>)[key]);When key is a dotted path such as "model.name" or "permissions.allow", { [key]: value } creates { "model.name": value } — a flat object with a literal dot in the property name. resolveEnvVarsInObject recurses into the value correctly regardless, and resolved[key] retrieves it by the same literal key, so the resolved value is obtained correctly before setNestedPropertySafe navigates the dot-separated path. The pattern is unusual but not broken for the current call sites. No action needed here.
— claude-sonnet-4-6 via Qwen Code /review
Generated by Claude Code
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Reviewed via Qwen Code /review.
No new blocking findings. The env-var resolution fix (resolveEnvVarsInObject applied before writing to settings) correctly addresses the regression where env-var placeholders were replaced on first write and never re-expanded. The originalSettings split (untouched raw config vs. resolved in-memory copy) is the right model for this. The mcp remove home-directory guard and the shift to reading from originalSettings in mcp add are both consistent with the fix.
Existing review threads from prior rounds have all been addressed or remain minor nits. LGTM from a logic standpoint — defer to the repo's CI and maintainer sign-off for final merge.
Generated by Claude Code
|
|
||
| const existingSettings = settings.forScope(settingsScope).settings; | ||
| const mcpServers = existingSettings.mcpServers || {}; | ||
| if (scope === 'project' && inHome) { |
There was a problem hiding this comment.
The new inHome guard here mirrors the one in add.ts, but remove.test.ts has no test coverage for it. In add.test.ts, the analogous guard is tested (see the 'when in the home directory' describe block around line 233, which covers: default scope in home dir, explicit --scope project rejection, and explicit --scope user passthrough).
Consider adding similar tests to remove.test.ts — at minimum a case where workspace.path === user.path and --scope project is passed, verifying that process.exit(1) is called and setValue is never invoked. Without this, a future refactor that breaks the guard in remove.ts would go undetected.
3 participants
Summary
Fixes MCP server add/remove persistence issues:
setValueFullSave()to write full settings JSON, avoiding merge-semantics bugsChanges
packages/cli/src/config/settings.ts: AddedsetValueFullSave()methodpackages/cli/src/commands/mcp/add.ts: Conditional headers spread, non-mutating server add, usessetValueFullSavepackages/cli/src/commands/mcp/remove.ts: Non-mutating server removal, usessetValueFullSaveTesting