fix(cli): persist MCP server removals - #4535
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| }, | ||
| }; | ||
|
|
||
| const result = applyUpdates(original, updates, false, ['mcpServers']); |
There was a problem hiding this comment.
[Suggestion] The only applyUpdates test with replacePath uses a single-segment path ['mcpServers']. However, setValue() passes key.split('.'), so many production callers use multi-segment keys ('ui.theme', 'tools.approvalMode', 'modelProviders.openai.ignoredVersion'). No test verifies that a nested replacePath (2+ segments) correctly replaces only the target subtree while preserving sibling keys at intermediate levels.
Consider adding a test with a multi-segment path:
it('should replace at a nested path while preserving sibling keys', () => {
const original = {
ui: { theme: { color: 'red', mode: 'dark' }, fontSize: 14 },
};
const updates = {
ui: { theme: { color: 'blue' } },
};
const result = applyUpdates(original, updates, false, ['ui', 'theme']);
expect(result).toEqual({
ui: { theme: { color: 'blue' }, fontSize: 14 },
});
});— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in 94436fe. Added coverage for a multi-segment replace path (['ui', 'theme']) that replaces only the nested target object while preserving sibling keys at the intermediate level.
96742bc to
94436fe
Compare
| saveSettings( | ||
| settingsFile, | ||
| createSettingsUpdate(key, value), | ||
| key.split('.'), |
There was a problem hiding this comment.
[Suggestion] setValue now passes key.split('.') as replacePath for ALL callers, changing every write from deep-merge to exact-subtree-replacement at the target key. I verified all 30+ current callers are safe (scalars, arrays, or full replacement objects), but this is a global behavioral change to a widely-used method with no JSDoc update.
A future caller doing setValue(scope, 'tools', { newOption: true }) expecting deep-merge (preserving tools.approvalMode, tools.disabled, etc.) would silently lose sibling keys in the file.
Consider either:
- Scoping
replacePathto only the MCP case (e.g.,const replacePath = key === 'mcpServers' ? key.split('.') : []) - Or documenting the replacement semantics in
setValue's JSDoc so future callers are aware
— qwen3.7-max via Qwen Code /review
| // Use the format-preserving update function | ||
| updateSettingsFilePreservingFormat(settingsFile.path, updates); | ||
| updateSettingsFilePreservingFormat( | ||
| settingsFile.path, |
There was a problem hiding this comment.
[Suggestion] saveSettings calls updateSettingsFilePreservingFormat but does not check its boolean return value. When the settings file has invalid JSON or output validation fails, the function returns false (after logging to stderr), but saveSettings silently returns without error.
Note that the migration path at ~line 944 DOES check the return value and logs via debugLogger. For consistency and debuggability, consider:
const written = updateSettingsFilePreservingFormat(
settingsFile.path,
updates,
false,
replacePath,
);
if (!written) {
debugLogger.error(
`saveSettings: updateSettingsFilePreservingFormat returned false for ${settingsFile.path}`,
);
}— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in b532fda. saveSettings now logs a debugLogger.error when updateSettingsFilePreservingFormat returns false, with a regression test covering the refused-write path.
94436fe to
b532fda
Compare
| continue; | ||
| } | ||
|
|
||
| if ( |
There was a problem hiding this comment.
[Suggestion] This new branch handles the case where the update value is a non-empty object but result[key] is a primitive, null, or an array — but no test in commentJson.test.ts covers this scenario. This is reachable in production if a settings file becomes corrupted or a user manually edits the JSON (e.g., { mcpServers: "broken" } → { mcpServers: { server1: {...} } }).
Consider adding tests like:
applyUpdates({ a: 'string' }, { a: { nested: true } })
// → { a: { nested: true } }
applyUpdates({ a: [1,2] }, { a: { b: 1 } })
// → { a: { b: 1 } }— qwen3.7-max via Qwen Code /review
|
@Jerry2003826 Just wondering what model you are — the Chinese translation showed up as gibberish lol 
|
|
Thanks for catching that ? I updated the PR description and replaced the garbled Chinese section with proper UTF-8 text. The code is unchanged. |
| nested: { | ||
| keep: 1, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
[Suggestion] Both existing replacePath tests pass non-empty objects as the update value ({keep: {command: 'node'}} and {theme: {color: 'blue'}}), so they always hit the applyUpdates({}, value) branch of the ternary at commentJson.ts:113-116. The : value branch (where valueIsObject is false — empty object, primitive, null, or array) is not directly tested.
This is reachable in production: mcp remove with the last server remaining calls setValue(scope, 'mcpServers', {}), and the "logs when setValue persistence is refused" test mocks updateSettingsFilePreservingFormat before applyUpdates runs.
it('should replace with a non-object value at the exact replace path', () => {
const original = {
mcpServers: { a: { command: 'node' }, b: { command: 'python' } },
};
const result = applyUpdates(original, { mcpServers: {} }, false, ['mcpServers']);
expect(result).toEqual({ mcpServers: {} });
});— qwen3.7-max via Qwen Code /review
Local Verification ReportBuilt and tested locally in tmux ( Environment
1. Bug reproduced on baseline (parent commit
|
| Suite | Result |
|---|---|
src/config/settings.test.ts (incl. the new persists removed MCP servers … case) |
111 passed |
src/utils/commentJson.test.ts (incl. new replace-path & prototype-pollution cases) |
18 passed |
src/commands/mcp/remove.test.ts |
4 passed |
| Targeted suites combined | 133 / 133 passed |
I re-ran the same three suites after rebasing the PR onto current origin/main — still 133/133 passing.
Note on the full
npm run test --workspace=packages/clirun: there are 2 unrelated failures insrc/serve/workspaceMemory.test.tsandsrc/serve/workspaceAgents.test.ts. I confirmed those same tests also fail on the PR's parent commit (HEAD~1) when the PR diff is reverted, so they are pre-existing, not introduced here. CI is green on the PR.
4. Static checks
| Check | Result |
|---|---|
npx prettier --check on the 4 changed files |
All files use Prettier code style |
npx eslint on the 4 changed files |
No issues |
npm run typecheck --workspace=packages/cli (tsc --noEmit) |
Clean |
npm run lint --workspace=packages/cli |
Clean |
5. Prototype-pollution mitigation sanity check
Direct call into the updated applyUpdates() with a hostile payload:
const updates = JSON.parse(
'{"safe":true,"__proto__":{"polluted":true},"constructor":{"prototype":{"polluted":true}},"nested":{"prototype":{"polluted":true},"keep":1}}'
);
applyUpdates({}, updates);
// → {"safe":true,"nested":{"keep":1}}
// Object.prototype.polluted === undefined ✅
// Object.keys(Object.prototype) === [] ✅__proto__ / constructor / prototype are filtered at every level; no global pollution.
6. Scope of behavior change
The patch is intentionally narrow: LoadedSettings.setValue() only opts into the new exact-subtree replacement when key === 'mcpServers'. Every other setValue(...) call (including the existing model.name provider-preservation path) still goes through the original deep-merge path. The added replacePath parameter on updateSettingsFilePreservingFormat / applyUpdates is opt-in and defaults to [], so external callers and migrations (sync mode) are unaffected.
Recommendation
LGTM to merge.
- Fix addresses the reported MCP add/remove bug #3718 symptom for
qwen mcp remove. - Regression coverage is meaningful (replacement semantics + sibling preservation + refusal logging + prototype-pollution).
- Lint / typecheck / prettier / CI all green; locally green on rebase against current
main. - No collateral behavior change for non-MCP settings paths.
4 participants
What this PR does
This PR makes settings persistence replace the exact subtree that
LoadedSettings.setValue()is updating, while keeping the existing format-preserving deep merge behavior for surrounding settings.For
qwen mcp remove, that means replacing the top-levelmcpServersobject with the remaining servers instead of deep-merging it back into the old file content and keeping the removed server entry.Why it's needed
When multiple MCP servers exist,
qwen mcp remove <name>updates the in-memory settings to remove the server, but saving uses a deep merge against the existing JSON file. Because object keys not present in the update are preserved by that merge, the deleted MCP server key is written back to disk and the removal does not persist.Reviewer Test Plan
How to verify
A reviewer can reproduce the original failure by starting with a settings file containing two MCP servers, calling
settings.setValue(SettingScope.User, 'mcpServers', { keep: { command: 'node' } }), and observing that the oldremoveserver remains in the written JSON. The new regression test covers that case and asserts the unrelateduisettings are preserved while themcpServerssubtree is replaced exactly.Validated locally with:
Evidence (Before & After)
N/A; this is a non-UI settings persistence fix covered by regression tests.
Tested on
Environment (optional)
Windows 10, Node.js v24.14.1, npm workspace install.
npm run buildhits an existing Windows path-with-spaces issue inpackages/web-templates/build.mjs, so I generated the web-template artifacts via a temporarysubstdrive before runningnpm run typecheck --workspace=packages/clisuccessfully.Risk & Scope
setValue()now persists the target path as an exact replacement, so whole-object updates can remove keys that are intentionally absent from the new value. This matches the in-memory semantics of setting a value and is covered by both the new MCP regression and the existing model.name preservation test.qwen mcp add -Hheaders report in MCP add/remove bug #3718 remains out of scope because the issue thread still needs an exact reproduction command for that behavior.Linked Issues
Refs #3718
中文说明
这个 PR 做了什么
这个 PR 调整了 settings 的持久化逻辑：当
LoadedSettings.setValue()更新某个具体子树时，写回文件时会精确替换这个子树，同时保留周围 settings 现有的格式保留式 deep merge 行为。对
qwen mcp remove来说，这意味着顶层mcpServers会被替换成移除后的剩余 server 列表，而不是和旧文件再次 deep merge，导致已删除的 server key 又被写回。为什么需要
当 settings 里有多个 MCP server 时，
qwen mcp remove <name>会正确更新内存状态，把目标 server 删除。但保存到 JSON 文件时，旧逻辑会和原文件做 deep merge，更新对象里不存在的 key 会被保留下来，所以被删除的 MCP server 又会出现在磁盘文件里，导致 remove 没有真正持久化。Reviewer Test Plan
如何验证
可以用一个包含两个 MCP server 的 settings 文件复现：调用
settings.setValue(SettingScope.User, 'mcpServers', { keep: { command: 'node' } })后，旧逻辑会把被删除的removeserver 仍然写回 JSON。新的回归测试覆盖了这个场景，并断言无关的uisettings 仍被保留，而mcpServers子树会被精确替换。本地已验证 settings、commentJson、mcp remove 相关测试，以及 Prettier、ESLint、CLI lint 和 CLI typecheck。
Evidence (Before & After)
N/A；这是非 UI 的 settings 持久化修复，由回归测试覆盖。
Tested on
Windows 已本地测试；macOS 和 Linux 依赖 CI 覆盖。
Environment
Windows 10，Node.js v24.14.1，npm workspace install。
npm run build在 Windows 路径带空格时会命中packages/web-templates/build.mjs的既有问题，因此我用临时substdrive 生成 web-template artifacts 后，成功运行了npm run typecheck --workspace=packages/cli。Risk & Scope
主要风险是：
setValue()现在会把目标 path 作为精确替换写入，因此 whole-object update 可以删除新值中不存在的 key。这和“设置一个值”的内存语义一致，并由新的 MCP 回归测试以及现有model.nameprovider 保留测试覆盖。#3718 里另一个
qwen mcp add -Hheaders 问题不在本 PR 范围内；issue 线程仍需要更具体的复现命令。没有 breaking change。