feat(mcp): reconcile MCP servers live on settings change - #5561
Conversation
|
@yiliang114 I have created a MR with thorough testing. Please take a look. |
|
@water-in-stone This PR currently has merge conflicts. Could you please pull the latest changes from |
4ad32ea to
d52b6d2
Compare
|
I have resolve all the conflicts. Please take a look. @yiliang114 |
|
@qwen-code /triage |
|
Thanks for the PR, @water-in-stone! Template looks good ✓ — all sections filled with real content, bilingual, videos attached. On direction: this is squarely aligned with #3696 sub-task 3 and solves a genuine user pain point — losing conversation context every time you tweak an MCP server config is a terrible workflow. The incremental reconcile machinery already existed; this PR closes the gap of having no runtime entry point to drive it. CHANGELOG has no direct reference but the area (hot-reload, MCP config) is core to the roadmap. On approach: the scope feels right for what it sets out to do. The six-part breakdown (A: Config setters + reconcile, B: SettingsWatcher wiring, D: mid-session approval modal, E: One observation: the Since the last triage pass, three follow-up commits landed: hardened teardown/reconcile, bounded admission with unavailable-reason explanations (Part F), and an integration-test harness fix. These tighten the implementation without expanding scope. Moving on to code review and testing. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有章节均有实质内容，中英双语，附有视频。 方向：与 #3696 子任务 3 完全对齐，解决了用户真实痛点——每次改 MCP 配置都要重启丢对话上下文。增量 reconcile 机制已有，本 PR 补的是运行时入口。CHANGELOG 无直接引用但该领域（热更新、MCP 配置）属于路线图核心。 方案：范围与目标匹配。六部分拆解（A: Config setter + reconcile, B: SettingsWatcher 接线, D: 中途审批弹窗, E: 一个观察： 上次 triage 后新增三个提交：加固 teardown/reconcile、有界准入与不可用原因解释（Part F）、集成测试 harness 修复。这些收紧了实现，没有扩大范围。进入代码审查和测试 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal vs. actual (re-run): If I were implementing MCP hot-reload from scratch, my approach would closely mirror this PR: Config runtime setters (can't reuse The PR goes beyond in several good ways: No bugs found. Both mock issues from the previous pass ( Reuse check: the PR correctly reuses Testing
tmux capture — CLI smoke test: CLI starts cleanly, responds with a valid answer, no errors or warnings (the React DevTools notice is informational only). No regressions observed. CI status: Ubuntu Test job passes. macOS/Windows jobs skip due to fork authorization (expected for cross-repo PRs). Lint and CodeQL pass. Full hot-reload E2E (edit settings mid-session → server reconnects) can't be meaningfully tested in tmux without a real MCP server binary that completes the handshake. The 24 hot-reload unit tests + 109 mcp-client-manager tests cover the logic paths well (reconcile gate, coalescing, admission list ordering, fingerprint diff, pending-approval skip, unavailable reasons). 中文说明代码审查独立方案对比实际实现（复跑）： 如果从零实现 MCP 热更新，我的方案会和本 PR 高度一致。PR 在几个方面超出了预期： 未发现 bug。 上次 review 的两个 mock 问题（gemini.test.tsx 的 测试结果
CI 状态： Ubuntu Test 通过。macOS/Windows 因 fork 授权跳过（跨仓库 PR 正常现象）。Lint 和 CodeQL 通过。 — Qwen Code · qwen3.7-max |
ReflectionStepping back: this PR is well-executed end-to-end. The motivation is clear (MCP config edits shouldn't cost you your conversation), the implementation is well-scoped (reuses existing reconcile machinery, adds just enough new surface), and the testing is thorough — 663+ tests across 10 suites, all passing. The code reads like someone who thought through the failure modes: reconcile-in-progress coalescing, fingerprint-based config change detection, ordered admission-list updates, The two mock bugs from the previous pass are fixed. The tangential Verdict: approve. ✅ 中文说明反思整体而言：这个 PR 从头到尾执行得很好。动机清晰（MCP 配置修改不应丢失对话上下文），实现范围合理（复用现有 reconcile 机制，只增加必要的新接口），测试充分——10 个测试套件共 663+ 个测试全部通过。 代码读起来像是作者认真考虑过失败模式：reconcile 合并守卫、指纹配置变更检测、有序准入列表更新、 两个 mock bug 均已修复。 结论：批准。 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM — well-scoped implementation that reuses existing reconcile machinery. One test mock needs a getResourceRegistry stub (one-liner fix in mcp-client-manager.test.ts), but the feature logic itself is solid. ✅
|
Heads-up: CI is red on this PR — the Test job fails on all three platforms (macOS / Ubuntu / Windows), so it's deterministic, not flaky. (failing run) 1. The new hot-reload wiring in Failing cases: 2. In your new test Both are just incomplete mocks for the new code paths — the production code itself looks fine. Once the two test doubles stub those methods, the Test job should pass. 👍 中文版提醒一下：这个 PR 的 CI 是红的 —— Test 任务在三个平台（macOS / Ubuntu / Windows）上全部失败，所以是稳定复现，不是 flaky。（失败的 run） 1.
失败用例： 2. 在你新增的测试 两处都只是新代码路径的 mock 不完整 —— 生产代码本身没问题。把这两个测试桩补全后，Test 任务就能通过。👍 |
🔍 Maintainer verification — live hot-reload works on Linux, but one stale test reds CI (2-line fix)I ran an independent real-TUI verification on Linux (the PR description marks Linux
Method
Results — live behaviors (all ✅)
EvidenceAdd → Modify → Remove (process-lifecycle proof; pids are real OS pids of the spawned mock servers): Part D — mid-session approval modal (fired by editing workspace Reject → Part E — A/B vs base (
|
| 组件 | 说明 |
|---|---|
| PR 构建 | head d52b6d271 → npm ci + npm run bundle → 真实 dist/cli.js（已校验 commit） |
| Base 构建 | 8f8ed0d7c（本 PR 父提交），用于 A/B |
| MCP server | 参数化 stdio mock；每次启动把自己的 OS pid 追加到 per-server 日志，从而用进程生命周期证明"改 = 重启"与"无关 server 不动" |
| 驱动 | tmux 下真实 TUI，隔离 QWEN_HOME，受信任 workspace；会话运行期间就地编辑 settings |
结果 —— 实时行为（全部 ✅）
| 场景 | 操作 | 观察（无重启） |
|---|---|---|
| 增 | 编辑 user settings.json 加 beta |
/mcp → 2 个 server，alpha ✓ + beta ✓；alpha pid 不变，beta pid 新增 |
| 改 | 改 alpha 的 env（hash 变） |
两者仍 ✓；alpha pid 日志多出第 2 条（重启），beta pid 不变（无「0 工具」空窗） |
| 删 | 移除 beta |
/mcp → 回到 1 个 server（仅 alpha） |
| 门控批准（D） | 中途加 workspace 范围的 gamma |
弹出审批框 → 批准 → gamma ✓ connected，子进程已起，mcpApprovals.json = approved |
| 门控拒绝 + 重弹（D） | 拒绝 gamma，再编辑其配置 |
拒绝 → 记为 rejected；改配置（hash 变）→ 对已拒绝的 server 重新弹框 |
/mcp 原因（E） |
查看被拒绝的门控 server | 行显示 ✗ rejected — edit config to re-approve（warning 黄，SGR 38;5;223），归到 Workspace Settings；该 server 不显示 "Run qwen --debug …" 提示 |
证据
增/改/删的进程生命周期证明（pid 为 mock server 真实 OS pid）：
加 beta: /mcp = 2 (alpha ✓, beta ✓) alpha pid 4186338(1条) beta pid 4187721(新)
改 alpha: /mcp = 2 (alpha ✓, beta ✓) alpha pid 4186338→4188611(重启) beta 4187721(不变)
删 beta: /mcp = 1 (alpha ✓)
A/B 对比 base（8f8ed0d7c）：同样的"加 beta"编辑，base 的 /mcp 仍是 1 个 server，且从不启动 beta 进程 —— 证明实时 reconcile 是新行为。
⚠️ 阻塞项 —— mcp-client-manager.test.ts 变红（CI 三平台均失败）
跑 PR 自己列的命令：
Test Files 1 failed | 7 passed (8)
Tests 1 failed | 464 passed (465)
✗ pool path skips a gated server pending approval (#4615, sub-task 3)
→ this.cliConfig.getResourceRegistry is not a function
这是过期 mock，非运行时 bug。该新测试按 #5544 之前的 pool.acquire() 签名编写，而 #5544（已并入本 PR 的 base e5c01aa35）让共享池 discovery 路径 (a) 调用 getResourceRegistry()、(b) 给 pool.acquire(...) 传第 6 个 resourceRegistry 参数。文件里其它池测试都已同步更新（stub 了 getResourceRegistry、断言 6 个 expect.anything()），唯独这个新测试漏了：它的 mock 缺 getResourceRegistry，且断言仍是 5 个参数。
本地加 2 行后该文件 105/105 全绿：
isMcpServerPendingApproval: (name: string) => name === 'gated',
+ getResourceRegistry: () => ({}),
} as unknown as Config;
@@
expect.anything(),
+ expect.anything(), // 第 6 个参数 = resourceRegistry（#5544 引入）
);该测试的安全断言（acquire 只对 'ok' 调用一次、不对 'gated'；McpClient 不被 spawn）本就反映正确行为，我也在真实 TUI 里验证了对应的用户可见行为（未审批的门控 server 在批准前不会启动）。所以产品代码没问题，测试需要 rebase 到 #5544 的签名。
说明：我的真实运行走的是单会话 discovery 路径（默认 TUI）；这个红测试专门覆盖池路径。我确认其红因即上面的过期 mock，且其意图与我实测的单会话行为一致。
合并状态
MERGEABLE（无冲突），但 BLOCKED：REVIEW_REQUIRED 且跨平台 Test 任务红（同一根因）。建议请作者更新这一个测试（2 行），之后 CI 应转绿。
Verified locally against isolated mock MCP servers + a mock LLM under tmux; no external services or real credentials. The 2-line fix above was applied only in a throwaway worktree to confirm the diagnosis, then reverted.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Heads-up: CI Test jobs are currently red on all three platforms (ubuntu/macOS/windows, Node 22.x). A static review can't see runtime test results and the failure detail wasn't retrievable from the run logs here — please check the failing Test jobs directly (it may be the getResourceRegistry mock the triage bot flagged, now interacting with the ResourceRegistry that landed on main separately, or a rebase artifact). The one static finding is inline below.
— claude-opus-4-8[1m] via /qreview
| // `discoverMcpToolsForServerInternal` disconnects the stale client | ||
| // before reconnecting with the freshly-read config, so pushing the | ||
| // name is sufficient — no explicit teardown needed here. | ||
| const currentId = this.connectionFingerprints.get(name); |
There was a problem hiding this comment.
[Suggestion] The connectionFingerprints invariant this guard relies on isn't upheld by all connect paths, so a live config edit can be silently ignored.
This branch only reconnects when currentId !== undefined && currentId !== connectionIdOf(...), and the comment states connectionFingerprints "is set on every successful connect." But two connect paths never set it:
- the bulk
discoverAllMcpToolsloop (~line 1110) — reached by/memory refreshand extension reload (tool-registry.ts:490/506), andQWEN_CODE_LEGACY_MCP_BLOCKING=1; - the
readResourcelazy-connect (~line 2707).
After any of those connect a server, its fingerprint is undefined, so the currentId !== undefined short-circuit treats it as "unchanged" — a subsequent in-place config edit (command/url/env/headers) is silently dropped and the server keeps running on the stale config, with no error. (The default boot path uses discoverAllMcpToolsIncremental → discoverMcpToolsForServerInternal, which does set the fingerprint, so a fresh session is fine; the gap is the post-/memory refresh / extension-reload / lazy-connect case.)
Fix either by recording the fingerprint at those two connect sites (this.connectionFingerprints.set(name, connectionIdOf(name, config)) after the successful connect()/discover()), or by inverting the guard so an undefined fingerprint on a CONNECTED client triggers a reconnect (fail-safe — a spurious reconnect is recoverable, a dropped config change is invisible). A test that connects via the bulk path, then edits the config and asserts a reconnect, would lock this in (the current fingerprint-reconnect test only connects via the incremental path first).
— claude-opus-4-8[1m] via /qreview
There was a problem hiding this comment.
Thanks for your comment. This bug has been fixed in the latest code.
There was a problem hiding this comment.
Thanks for the quick follow-up. I checked the latest head (d9f5d819): the bulk discovery path now records the fingerprint after connect()/discover(), the readResource lazy-spawn path records it after connect, and the new regression tests cover both cases. That addresses the stale-config concern from this thread for me.
One small note for later: treating undefined on a connected client as a fail-safe reconnect would still make the invariant more defensive, but with the known connect paths covered I do not think that needs to block this PR.
CI failure analysis — root cause: two test-mock omissionsThe three Test jobs (ubuntu / macOS / windows, Node 22.x) all fail identically and deterministically (same 7 tests on every OS — not flaky). Both failures are 1.
|
CI build failure: root causeThe two failing Cause 1 —
|
d413cf0 to
d9f5d81
Compare
|
I also compared this with Claude Code's MCP lifecycle implementation. My read is that this PR is not a direct port from Claude Code: Claude Code manages MCP mostly through the React/AppState path ( The main stale-config concern around A few non-blocking design follow-ups I would still keep in mind:
Separate from the implementation review: the latest CI still has |
| "diff": "^7.0.0", | ||
| "dotenv": "^17.1.0", | ||
| "express": "^5.2.1", | ||
| "fast-deep-equal": "^3.1.3", |
There was a problem hiding this comment.
[P2] One thing I noticed here: this adds a direct packages/cli dependency, but package-lock.json was not updated. I checked in a temporary worktree, and npm install --package-lock-only --ignore-scripts adds the missing packages/cli.dependencies.fast-deep-equal entry. Could we include that lockfile update as well?
There was a problem hiding this comment.
Thank you. It has been fixed in the latest code
| }) => { | ||
| // 受门控（#4615）但未审批的 server 被 discovery 跳过，不会进入连接/认证流程， | ||
| // 审批原因优先展示。 | ||
| const awaitingApproval = |
There was a problem hiding this comment.
[P3] Since we already compute awaitingApproval, might be worth using it in the action list below too. The row now correctly shows needs approval / rejected, but a disconnected gated server still offers Reconnect, and non-disabled servers still offer Authenticate. Those actions will not really progress while approvalState is set because discovery skips pending/rejected servers, so hiding them until approval would make the detail view match the status text.
There was a problem hiding this comment.
Thank you. It has been fixed in the latest code
79e9dd9 to
aa23dcb
Compare
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Thanks for the follow-up updates. I re-checked the latest head and the previously discussed items look addressed; the MCP hot-reload path looks good to me. Leaving the remaining CI jobs to finish before merge.
aa23dcb to
a55e371
Compare
✅ Re-verification at
|
| Piece | Detail |
|---|---|
| PR build | head a55e3713 → npm ci + npm run bundle → real dist/cli.js (commit verified) |
| Driver | real TUI under tmux, isolated HOME, mock OpenAI for boot; Linux x64 / Node 22. settings.json edited in place while the session runs |
| MCP servers | parameterized stdio mock; each appends its OS pid to a per-server log on every spawn → "modify = restart" and "unrelated untouched" are provable by process lifecycle, and a per-method log is the ground truth for each (re)handshake |
Unit tests — the CI blocker is gone
| Suite | Result | Note |
|---|---|---|
gemini.test.tsx |
✅ 25 | the getTopTierMcpServers stub fix (CI cause #1) |
mcp-client-manager.test.ts |
✅ 107 | the getResourceRegistry mock fix (CI cause #2) |
| CLI side (hot-reload, mcpApprovals, settingsSchema, settingsWatcher, useMcpApproval, ServerListStep, gemini) | ✅ 149 | all 7 files |
| Core side (config, mcp-client-manager, coreToolScheduler, errors) | ✅ 585 | all 4 files |
| Total | ✅ 734 passed, 0 failed | the PR's exact listed command + the two it touches |
Live behaviors (all ✅, no session restart)
| Scenario | What I did | Observed |
|---|---|---|
| Add | add beta to user settings.json |
/mcp 1→2 servers, beta ✓ connected; alpha pid unchanged, beta new pid |
| Modify | change alpha's env (config hash changes) |
alpha restarts (pid 997292→998621, 2nd handshake); beta untouched — no "0 tools" gap |
| Remove | drop beta |
beta process exits; /mcp 2→1 server; alpha untouched |
| Irrelevant edit (the gate) | change theme only |
no server restarted — the serversChanged || gatingChanged gate bails, so unrelated edits don't churn MCP |
| Part D — approval | add a workspace-scope gamma |
gamma not spawned (gated); the mid-session approval modal pops |
| Part D — re-prompt | reject gamma, then edit its config |
reject → recorded; editing the config (hash changes) re-fires the modal for the previously-rejected server (no nag on a settled rejection) |
| Approve | approve gamma |
gamma spawns + ✓ connected; approval persisted (hash 2b30ee86… rejected → ce8a83cc… approved) |
| Part E — gated reason | /mcp for rejected gamma |
row shows ✗ rejected — edit config to re-approve (warning color), grouped under Workspace Settings; no "--debug" footer for it |
| Part E — real failure | add broken (bad command) |
row shows ✗ disconnected with the "Run qwen --debug to see error logs" footer — a genuine failure is still distinguished from a gated skip |
Evidence
Add → Modify → Remove (pids are real OS pids of the spawned mock servers; handshake counts from the per-method log):
boot: /mcp = 1 server (alpha ✓) alpha pid 997292 (1 handshake)
add beta: /mcp = 2 servers (alpha ✓, beta ✓) alpha pid 997292 UNCHANGED beta pid 998339 (new)
modify alpha:/mcp = 2 servers (alpha ✓, beta ✓) alpha pid 997292→998621 RESTARTED beta pid 998339 UNTOUCHED
remove beta: /mcp = 1 server (alpha ✓) beta process 998339 EXITED
per-server handshakes (initialize): alpha ×2 (boot + modify) beta ×1 (add, then torn down) gamma ×1 (only after approval)
Part D — mid-session approval modal (fired by editing workspace .qwen/settings.json):
Untrusted MCP server in .qwen/settings.json
This workspace declares an MCP server. Approving lets Qwen Code start it and run its tools.
Approval is bound to this exact configuration — if .qwen/settings.json changes, you will be asked again.
gamma node …/mock-mcp.mjs (stdio) [env: MOCK_LABEL, MOCK_PIDLOG, MOCK_METHODLOG]
› 1. Approve this server 2. Approve all pending servers in this workspace 3. Reject (esc)
Reject → mcpApprovals.json records gamma: rejected; gamma never spawns/handshakes. Edit its config → modal re-fires (hash rebind). Approve → gamma ✓ connected, approved persisted with the new hash.
Part E — /mcp distinguishes "gated" from "failed":
User MCPs
alpha · ✓ connected
broken · ✗ disconnected ← genuine failure …
Workspace Settings
gamma · ✗ rejected — edit config to re-approve ← … gated skip (warning color, no footer)
※ Run qwen --debug to see error logs ← footer present ONLY because of `broken`
📝 Observations for merge (non-blocking)
- Security stance holds. The deliberate "settings win" choice means a runtime
settings.jsonedit can widen MCP admission beyond the startup--allowed-mcp-server-names— but I confirmed the#4615approval gate is preserved on the hot-reload path: a workspace/project-scope server never auto-connects, a rejected one stays rejected until its config hash changes, and the pool path skips pending servers too. - Test hygiene nit:
ServerListStep.test.tsxemits a Reactact(...)warning (a state update outsideact) — cosmetic, tests pass; worth a tidy-up but not a blocker. - Scope is as advertised: LSP runtime reconnect (Part C),
/reload,clearAllCaches(), and theneedsRefreshnotification are explicitly out of this PR; I did not exercise them.
Verdict (verification side): live behavior matches the description on all of Parts A/B/D/E, the CI blocker I previously flagged is resolved, and the PR's full unit suite is green (734/734). From the runtime-verification side this looks merge-ready; the final call is the maintainers'.
🇨🇳 中文版（点击展开）
✅ 在 a55e3713 上的复验 —— MCP 运行时热更新端到端可用，且我此前指出的 CI 阻塞已修复
我在当前 head(rebase + 测试 mock 修复之后)对该 PR 做了一次独立的 Linux 真实 TUI 验证,确认两件事:
- 功能端到端可用(打包后的 CLI):实时 增 / 改 / 删、中途审批弹窗(Part D)、
/mcp原因文案(Part E)均与描述完全一致。 - CI 阻塞已解除。 我此前定位的两处测试 mock 缺失(CI 分析)——
gemini.test.tsx缺getTopTierMcpServers、新增的mcp-client-manager.test.tsmock 缺getResourceRegistry—— 均已修复;PR 列出的完整测试套件本地全绿(734 个)。
本条取代我早先的验证记录(那是在一个已被 rebase 掉的 head 上)。
方法
| 部分 | 细节 |
|---|---|
| PR 构建 | head a55e3713 → npm ci + npm run bundle → 真实 dist/cli.js(已核对 commit) |
| 驱动 | tmux 下的真实 TUI,隔离 HOME,mock OpenAI 用于启动;Linux x64 / Node 22。会话运行期间原地编辑 settings.json |
| MCP server | 参数化 stdio mock;每次 spawn 都把自己的 OS pid 追加到各自日志 → "改=重启"、"无关 server 不动" 可由进程生命周期证明,另有 per-method 日志作为每次(重)握手的 ground truth |
单元测试 —— CI 阻塞已消除
| 套件 | 结果 | 说明 |
|---|---|---|
gemini.test.tsx |
✅ 25 | getTopTierMcpServers stub 修复(CI 原因 #1) |
mcp-client-manager.test.ts |
✅ 107 | getResourceRegistry mock 修复(CI 原因 #2) |
| CLI 侧(hot-reload、mcpApprovals、settingsSchema、settingsWatcher、useMcpApproval、ServerListStep、gemini) | ✅ 149 | 7 个文件 |
| Core 侧(config、mcp-client-manager、coreToolScheduler、errors) | ✅ 585 | 4 个文件 |
| 合计 | ✅ 734 通过,0 失败 | PR 列出的命令 + 它改动的两个文件 |
实时行为(全部 ✅,无需重启会话)
| 场景 | 操作 | 观察 |
|---|---|---|
| 新增 | 在用户 settings.json 加 beta |
/mcp 1→2 个,beta ✓ connected;alpha pid 不变,beta 新 pid |
| 修改 | 改 alpha 的 env(配置 hash 变化) |
alpha 重启(pid 997292→998621,二次握手);beta 不动 —— 无「0 工具」空窗 |
| 删除 | 移除 beta |
beta 进程退出;/mcp 2→1 个;alpha 不动 |
| 无关编辑(门控) | 仅改 theme |
无 server 重启 —— serversChanged || gatingChanged 门控直接 bail,无关编辑不会扰动 MCP |
| Part D — 审批 | 新增 workspace 作用域 的 gamma |
gamma 未启动(受门控);中途弹出审批框 |
| Part D — 重新提示 | 先拒绝 gamma,再编辑其配置 |
拒绝被记录;编辑配置(hash 变化)→ 对此前被拒绝的 server 重新弹框(对已定的拒绝不再骚扰) |
| 批准 | 批准 gamma |
gamma **启动 + ✓ connected**;审批持久化(hash 2b30ee86…拒绝 →ce8a83cc…` 批准) |
| Part E — 门控原因 | 对被拒的 gamma 看 /mcp |
该行显示 ✗ rejected — edit config to re-approve(warning 色),归在 Workspace Settings 下;不为它显示 "--debug" 提示 |
| Part E — 真实失败 | 新增 broken(坏命令) |
该行显示 ✗ disconnected 并带 "Run qwen --debug to see error logs" 提示 —— 真实失败仍与门控跳过区分开 |
证据
增 → 改 → 删(pid 是 spawn 出来的 mock server 的真实 OS pid;握手计数来自 per-method 日志):
启动: /mcp = 1 个 (alpha ✓) alpha pid 997292 (1 次握手)
加 beta: /mcp = 2 个 (alpha ✓, beta ✓) alpha pid 997292 不变 beta pid 998339 (新)
改 alpha: /mcp = 2 个 (alpha ✓, beta ✓) alpha pid 997292→998621 重启 beta pid 998339 不动
删 beta: /mcp = 1 个 (alpha ✓) beta 进程 998339 已退出
per-server 握手(initialize): alpha ×2(启动 + 修改) beta ×1(新增,随后被拆除) gamma ×1(仅批准后)
Part D —— 中途审批弹窗(由编辑 workspace .qwen/settings.json 触发):
Untrusted MCP server in .qwen/settings.json
This workspace declares an MCP server. Approving lets Qwen Code start it and run its tools.
Approval is bound to this exact configuration — if .qwen/settings.json changes, you will be asked again.
gamma node …/mock-mcp.mjs (stdio) [env: MOCK_LABEL, MOCK_PIDLOG, MOCK_METHODLOG]
› 1. Approve this server 2. Approve all pending servers in this workspace 3. Reject (esc)
拒绝 → mcpApprovals.json 记 gamma: rejected,gamma 从不启动/握手。编辑其配置 → 弹窗重新触发(hash 重新绑定)。批准 → gamma ✓ connected,以新 hash 持久化为 approved。
Part E —— /mcp 区分「门控」与「失败」:
User MCPs
alpha · ✓ connected
broken · ✗ disconnected ← 真实失败 …
Workspace Settings
gamma · ✗ rejected — edit config to re-approve ← … 门控跳过(warning 色,无提示)
※ Run qwen --debug to see error logs ← 仅因为 broken 才出现的底部提示
📝 合并参考(非阻塞)
- 安全姿态成立。 "settings 优先"是有意为之:运行时编辑
settings.json可能让 MCP 准入超出启动时的--allowed-mcp-server-names;但我确认热更新路径上#4615审批门控仍然有效 —— workspace/project 作用域的 server 绝不自动连,被拒的保持拒绝直到其配置 hash 变化,共享池路径同样跳过 pending。 - 测试小瑕疵:
ServerListStep.test.tsx触发了一个 Reactact(...)警告(state 更新未包在act内)—— 纯属表面,测试通过,值得顺手清理但不阻塞合并。 - 范围如描述: LSP 运行时重连(Part C)、
/reload、clearAllCaches()、needsRefresh通知都明确不在本 PR 内,我未验证它们。
结论(验证视角): 实时行为在 Parts A/B/D/E 上均与描述一致,我此前指出的 CI 阻塞已解除,PR 完整单测全绿(734/734)。从运行时验证角度看具备合并条件;最终决定权在维护者。
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Thanks for the follow-up updates. I re-checked the latest head (a55e371): the previously discussed lockfile, mock, approval UI, and connection fingerprint concerns are addressed, and Lint is now green. The MCP hot-reload implementation looks good to me; leaving the remaining Test and CodeQL jobs to finish before merge.
| // Also drop its prompts. Unlike `ToolRegistry.disconnectServer`, this | ||
| // config-driven removal path never cleaned up the prompt registry, so a | ||
| // removed/changed server leaked its prompts across a hot-reload. | ||
| this.cliConfig.getPromptRegistry().removePromptsByServer(serverName); |
There was a problem hiding this comment.
[Suggestion] Thanks for adding removePromptsByServer here to close the prompt leak. ToolRegistry.disconnectServer and disableMcpServer also call removeResourcesByServer as the third cleanup step — this config-driven path should match that triple cleanup to prevent MCP resources from lingering in listMcpResources after a hot-reload removes or changes a server.
| this.cliConfig.getPromptRegistry().removePromptsByServer(serverName); | |
| this.cliConfig.getPromptRegistry().removePromptsByServer(serverName); | |
| this.cliConfig.getResourceRegistry().removeResourcesByServer(serverName); |
Same applies to discoverMcpToolsForServerInternal's finally block (line 1357) where removePromptsByServer was also added.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks for your comment. This bug has been fixed in the latest code.
| } | ||
| this.clients.delete(name); | ||
| this.connectionFingerprints.delete(name); | ||
| this.toolRegistry.removeMcpToolsByServer(name); |
There was a problem hiding this comment.
[Suggestion] The addRuntimeMcpServer replace branch calls removeMcpToolsByServer but is missing both removePromptsByServer and removeResourcesByServer. The PR correctly adds prompt cleanup in removeServer, discoverMcpToolsForServerInternal, and removeRuntimeMcpServer, but this replace branch (pool path here and legacy path below) and the error rollback path were missed — prompts and resources from a replaced or failed runtime server leak across hot-reloads.
| this.toolRegistry.removeMcpToolsByServer(name); | |
| this.toolRegistry.removeMcpToolsByServer(name); | |
| this.cliConfig.getPromptRegistry().removePromptsByServer(name); | |
| this.cliConfig.getResourceRegistry().removeResourcesByServer(name); |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks for your comment. This bug has been fixed in the latest code.
| : t(server.status)} | ||
| : awaitingApproval | ||
| ? server.approvalState === 'rejected' | ||
| ? t('rejected — edit config to re-approve') |
There was a problem hiding this comment.
[Suggestion] The string 'rejected — edit config to re-approve' is passed through t() here and in ServerDetailStep.tsx, but is missing from all locale files under packages/cli/src/i18n/locales/. Other strings in the same ternary chain ('needs approval', 'needs authentication') have entries in every locale. Without locale entries, non-English users see the raw English key.
Please add the translation to each locale file following the pattern of the adjacent 'needs approval' entry.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks for your comment. This bug has been fixed in the latest code.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
[Critical] removeResourcesByServer missing from multiple teardown paths; removePromptsByServer + removeResourcesByServer missing from addRuntimeMcpServer replacement branch
ToolRegistry.disconnectServer, disableMcpServer, and reconnectMcpServer all clean the full trio (tools + prompts + resources). But several lower-level paths are incomplete:
addRuntimeMcpServerreplacement (lines 2907, 2920): callsremoveMcpToolsByServeronly — missing bothremovePromptsByServerandremoveResourcesByServer- Spawn-failure catch (line 2980): calls
removeMcpToolsByServeronly — missing both removeServer(line 2499): tools + prompts — missingremoveResourcesByServerdiscoverMcpToolsForServerInternalfinally (line 1357): tools + prompts — missingremoveResourcesByServerremoveRuntimeMcpServer(line 3072): tools + prompts — missingremoveResourcesByServer
When a user deletes or config-changes a server that exposes MCP resources via hot-reload, stale resource entries remain in the ResourceRegistry pointing at a closed client.
Add this.cliConfig.getResourceRegistry().removeResourcesByServer(name) at lines 1357, 2499, and 3072. Add both removePromptsByServer and removeResourcesByServer at lines 2907, 2920, and 2980.
| // Also drop its prompts. Unlike `ToolRegistry.disconnectServer`, this | ||
| // config-driven removal path never cleaned up the prompt registry, so a | ||
| // removed/changed server leaked its prompts across a hot-reload. | ||
| this.cliConfig.getPromptRegistry().removePromptsByServer(serverName); |
There was a problem hiding this comment.
[Critical] removeResourcesByServer is missing here and at two other teardown paths where this PR added removePromptsByServer.
ToolRegistry.disconnectServer, disableMcpServer, and reconnectMcpServer all clean the full trio (tools + prompts + resources). But this path and lines 1357 and 3072 only clean tools + prompts.
When a user deletes a server that exposes MCP resources via hot-reload, stale resource entries remain in the ResourceRegistry pointing at a closed client.
| this.cliConfig.getPromptRegistry().removePromptsByServer(serverName); | |
| this.cliConfig.getPromptRegistry().removePromptsByServer(serverName); | |
| this.cliConfig.getResourceRegistry().removeResourcesByServer(serverName); |
Apply the same one-line addition at discoverMcpToolsForServerInternal (line 1357) and removeRuntimeMcpServer (line 3072) as well.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks for your comment. This bug has been fixed in the latest code.
| `[mcp-hot-reload] reconcile failed: ${err instanceof Error ? (err.stack ?? err.message) : String(err)}`, | ||
| ); | ||
| throw err; | ||
| } finally { |
There was a problem hiding this comment.
[Suggestion] When pass 1 throws and a coalesced call B had set mcpReconcilePending = true, the finally block only resets mcpReconcileInProgress — leaving mcpReconcilePending stuck at true. The next successful call C enters the while drain loop and runs an unnecessary extra reconcile pass.
| } finally { | |
| } finally { | |
| this.mcpReconcileInProgress = false; | |
| this.mcpReconcilePending = false; | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks for your comment. This bug has been fixed in the latest code.
| // model a missing tool belongs to a server removed this session. Diff the | ||
| // merged map (not just `servers`) so a server still provided by an | ||
| // extension is not falsely flagged as removed. Re-added names self-heal. | ||
| const prevEffective = new Set(Object.keys(this.getMcpServers() ?? {})); |
There was a problem hiding this comment.
[Suggestion] prevEffective is computed from getMcpServers() which applies the allowedMcpServers filter. But in the hot-reload path, setAllowedMcpServers is called before reinitializeMcpServers (see hot-reload.ts lines 169-171). So prevEffective reads the OLD server map through the NEW (already-narrowed) allow-list.
When the allow-list narrows, servers that were previously connected but are now filtered out never appear in prevEffective, so they never enter recentlyRemovedMcpServers. The tool-not-found message for such a server falls through to the generic Levenshtein suggestion instead of explaining the server was removed/filtered this session.
Consider computing prevEffective from getSettingsMcpServers() (raw, ungated), or snapshotting the effective set before the gating setters are called.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks for your comment. This bug has been fixed in the latest code.
| // server's registered prefix. The trailing `__` makes the match exact at a | ||
| // server boundary (so `foo` does not match a `foobar` server). Truncation | ||
| // (>63-char names) is the rare case we let fall through. | ||
| const prefixOf = (server: string): string => |
There was a problem hiding this comment.
[Suggestion] The prefix match unknownToolName.startsWith(prefixOf(s)) can produce a false positive when one server name contains another after sanitization. For example, if servers foo and foo__bar both exist, tool mcp__foo__bar__baz (belonging to server foo, tool bar__baz) starts with prefix mcp__foo__bar__ (the prefix for server foo__bar). Since find() returns the first match, the wrong server is attributed depending on iteration order.
Sort the candidate server names by name length descending before find(), so longer (more specific) prefixes match first:
| const prefixOf = (server: string): string => | |
| const prefixOf = (server: string): string => | |
| `mcp__${server}__`.replace(/[^a-zA-Z0-9_.-]/g, '_'); | |
| // (B) Removed this session — precise, names the server. | |
| const removed = (this.config.getRecentlyRemovedMcpServers?.() ?? []) | |
| .slice() | |
| .sort((a, b) => b.length - a.length); |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks for your comment. This bug has been fixed in the latest code.
… unavailable (QwenLM#3696) (QwenLM#5561) - K: treat the startup --allowed-mcp-server-names flag as an immutable upper bound — a runtime settings edit may narrow MCP admission within it but never widen beyond it; with no flag, settings fully drive admission. - H: preserve an explicit `mcp.allowed: []` as deny-all (don't collapse to undefined / allow-all), matching boot semantics, and make mcpGatingEqual distinguish absent (allow-all) from [] (deny-all) so the change reconciles. - B: classify why an MCP server is unavailable (removed / not_allowed / excluded / pending_approval) and route the tool-not-found message to the right recovery action; track removals against the gating-independent merged map (dropping the prev-effective snapshot param). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
f65939c to
bb10b31
Compare
✅ Fresh re-verification at
|
| # | Live edit | [MCP_HOT_RELOAD] trace |
Process evidence (ps -C node) |
|---|---|---|---|
| 1 | ADD user server gamma |
prev=[alpha, beta] next=[alpha, beta, gamma] → final statuses=[alpha=connected, beta=connected, gamma=connected] |
alpha/beta PIDs unchanged, only gamma spawned (no "0‑tools" gap) ✅ |
| 2 | MODIFY alpha (add env → fingerprint change) |
serversChanged=true → final statuses=[…all connected] |
only alpha respawned (new PID); beta/gamma keep PIDs ✅ |
| 3 | REMOVE beta |
prev=[alpha, beta, gamma] next=[alpha, gamma] → final statuses=[alpha=connected, gamma=connected] |
beta process killed ✅ |
| 4 | non‑MCP edit (ui.hideTips) |
no MCP-relevant change (servers + gating unchanged) — skipping reconcile |
all MCP PIDs unchanged — semantic‑diff guard works ✅ |
| 5 | ADD workspace‑scope delta (gated) |
gating next: … pending=[delta] · serversChanged=true gatingChanged=true · final statuses=[…delta=disconnected] · emitting mcp-pending-approval-changed: [delta] |
approval modal pops mid‑session, no restart ✅ |
| 6 | Approve delta |
— | delta connects; ~/.qwen/mcpApprovals.json ⇒ delta: { hash, status: "approved" } keyed by workspace root ✅ |
Tool re‑discovery (edit #2): /mcp → drill into alpha shows Tools: 2 tools (mcp__alpha__alpha_echo + mcp__alpha__alpha_extra) — up from 1, confirming the reconnect re‑fetched the new tool set.
No‑restart proof: the launcher PID stayed alive the whole session (255s across all 6 edits) and [MCP_HOT_RELOAD] registered MCP hot-reload listener on SettingsWatcher appears exactly once — a single continuous process, never re‑spawned.
Mid‑session approval modal (real TUI):
Untrusted MCP server in .qwen/settings.json
…
delta node …/mcp-server.mjs delta (stdio)
› 1. Approve this server
2. Approve all pending servers in this workspace
3. Reject (esc)
Final /mcp state:
3 servers
User MCPs
alpha · ✓ connected
gamma · ✓ connected
Workspace Settings
delta · ✓ connected
Unit suites touched by the PR (clean npm ci env)
| side | suites | result |
|---|---|---|
| CLI | hot-reload · mcpApprovals · settingsWatcher · settingsSchema · useMcpApproval · ServerListStep |
125 passed (6 files) |
| Core | config · mcp-client-manager · coreToolScheduler · errors |
596 passed (4 files) |
Verdict
All hot‑reload paths behave correctly on a live TUI: add → connect, remove → disconnect, modify → reconnect (re‑discover tools), unchanged → keep (no churn), non‑MCP edit → skip, gated workspace server → approval modal → persisted approval — all without restarting the CLI. The semantic‑diff gate avoids needless reconnects, and only the genuinely‑affected server respawns. Behavior matches the PR description and the unit suites are green. LGTM as a merge reference. 👍
Note: I've verified this PR several times during its review (CI‑red triage and earlier heads); this is a fresh confirmation at the current head cbdac574c.
中文版（点击展开）
✅ 在当前 head cbdac574c 的重新验证 —— MCP 热重载端到端可用（真实 Ink TUI，通过 tmux 驱动）
在当前 head 上用真实交互式 TUI（从源码构建、tmux 驱动）做了重新验证，不只是单测。一个手写的确定性 stdio MCP server（换行 JSON‑RPC，按 argv/env 参数化，使得改一次配置既改变它的 fingerprint 又改变它暴露的工具数量）让每条 reconcile 路径都可观测。QWEN_DEBUG_LOG_FILE=1 暴露 [MCP_HOT_RELOAD] 轨迹；ps -C node 证明到底哪个子进程真的重启。
环境： Linux · Node v22.22.2 · 通过 npm ci 构建（PR 新增 fast-deep-equal 依赖，硬链接 node_modules 会缺它）· node packages/cli（逐文件 dist）· 隔离的 HOME + 独立 workspace cwd · 启动时两个 user‑scope server（alpha、beta）。
实时结果 —— TUI 运行时直接改 settings.json
| # | 实时编辑 | [MCP_HOT_RELOAD] 轨迹 |
进程证据（ps -C node） |
|---|---|---|---|
| 1 | 新增 user server gamma |
prev=[alpha, beta] next=[alpha, beta, gamma] → final statuses=[alpha=connected, beta=connected, gamma=connected] |
alpha/beta 的 PID 不变，只 spawn 了 gamma（没有“0 工具”空窗）✅ |
| 2 | 修改 alpha（加 env → fingerprint 变化） |
serversChanged=true → final statuses=[…全部 connected] |
只有 alpha 重启（新 PID）；beta/gamma 保持 PID ✅ |
| 3 | 删除 beta |
prev=[alpha, beta, gamma] next=[alpha, gamma] → final statuses=[alpha=connected, gamma=connected] |
beta 进程被杀掉 ✅ |
| 4 | 非 MCP 编辑（ui.hideTips） |
no MCP-relevant change (servers + gating unchanged) — skipping reconcile |
所有 MCP PID 不变 —— 语义 diff 守卫生效 ✅ |
| 5 | 新增 workspace‑scope delta（受审批门控） |
gating next: … pending=[delta] · serversChanged=true gatingChanged=true · final statuses=[…delta=disconnected] · emitting mcp-pending-approval-changed: [delta] |
审批 弹窗在会话中弹出，无需重启 ✅ |
| 6 | 批准 delta |
— | delta 连接成功；~/.qwen/mcpApprovals.json ⇒ delta: { hash, status: "approved" }，按 workspace root 归键 ✅ |
工具重新发现（编辑 #2）： /mcp → 进入 alpha 显示 Tools: 2 tools（mcp__alpha__alpha_echo + mcp__alpha__alpha_extra）—— 从 1 个变 2 个，证明重连重新抓取了新工具集。
无重启证明： launcher 进程整段会话一直存活（6 次编辑跨越 255s），且 [MCP_HOT_RELOAD] registered MCP hot-reload listener on SettingsWatcher 只出现一次 —— 单一连续进程，从未重启。
会话内审批弹窗（真实 TUI）：
Untrusted MCP server in .qwen/settings.json
…
delta node …/mcp-server.mjs delta (stdio)
› 1. Approve this server
2. Approve all pending servers in this workspace
3. Reject (esc)
最终 /mcp 状态：
3 servers
User MCPs
alpha · ✓ connected
gamma · ✓ connected
Workspace Settings
delta · ✓ connected
PR 涉及的单测套件（干净的 npm ci 环境）
| 侧 | 套件 | 结果 |
|---|---|---|
| CLI | hot-reload · mcpApprovals · settingsWatcher · settingsSchema · useMcpApproval · ServerListStep |
125 通过（6 文件） |
| Core | config · mcp-client-manager · coreToolScheduler · errors |
596 通过（4 文件） |
结论
所有热重载路径在真实 TUI 上行为正确：新增 → 连接、删除 → 断开、修改 → 重连（重新发现工具）、未变 → 保持（无抖动）、非 MCP 编辑 → 跳过、受门控的 workspace server → 审批弹窗 → 持久化批准 —— 全部无需重启 CLI。语义 diff 门控避免了无谓重连，只有真正受影响的 server 重启。行为与 PR 描述一致，单测全绿。作为 merge 参考，LGTM。 👍
说明：本 PR 在评审期间我已多次验证过（包括 CI 红的排查和更早的 head）；这是在当前 head cbdac574c 的一次最新确认。
|
Heads-up: this is failing in the merge queue, which is why the PR's own checks look green — the failing job only runs on the Failing check: The session child spawns ( Likely root cause is the new pending-approval gate this PR adds to the shared-pool path in // runDiscoverAllMcpToolsViaPool, before building desiredIds
if (cliConfig.isMcpServerPendingApproval?.(name)) continue;
Pulling it out of the queue for now so it stops re-failing the same job — re-queue once this is sorted. |
| // server boundary (so `foo` does not match a `foobar` server). Truncation | ||
| // (>63-char names) is the rare case we let fall through. | ||
| const prefixOf = (server: string): string => | ||
| `mcp__${server}__`.replace(/[^a-zA-Z0-9_.-]/g, '_'); |
There was a problem hiding this comment.
[Suggestion] prefixOf mirrors generateValidName's per-char sanitization but omits its 63-char truncation (first 28 + ___ + last 32). For server names where the full mcp__${name}__ prefix exceeds 63 characters, startsWith(prefixOf(s)) will never match because the registered tool name is truncated at registration time but the reconstructed prefix here is not.
The code comment acknowledges truncation as "the rare case we let fall through", but the fallthrough consequence is a misleading error message — the model receives "no MCP server providing it is currently configured" when the server IS configured and admitted.
| `mcp__${server}__`.replace(/[^a-zA-Z0-9_.-]/g, '_'); | |
| let p = `mcp__${server}__`.replace(/[^a-zA-Z0-9_.-]/g, '_'); | |
| if (p.length > 63) { | |
| p = p.slice(0, 28) + '___' + p.slice(-32); | |
| } | |
| return p; |
— qwen3.7-max via Qwen Code /review
Hot-reload MCP servers when settings.json changes (issue QwenLM#3696 sub-task 3): editing mcpServers / mcp.allowed / mcp.excluded now connects, disconnects, or restarts only the affected servers in place, without restarting the session or losing conversation context. - Part A: Config runtime setters + reinitializeMcpServers incremental reconcile; align the shared-pool path with the QwenLM#4615 pending-approval gate - Part B: SettingsWatcher subscriber (hotReload.ts), gated on a mcpServers + gating-list diff; flip the three MCP schema keys to hot-reloadable - Part D: re-fire the approval modal for a gated server left pending by an edit - Part E: /mcp shows why a gated server was skipped (pending / rejected) - Record connection fingerprints on the bulk and lazy-connect paths so an edit to a server first connected via those paths is not silently dropped - Design doc (en/zh) incl. the admission-stance boundary clarification Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> # Conflicts: # packages/cli/src/config/settingsSchema.test.ts # Conflicts: # packages/cli/src/ui/components/mcp/steps/ServerDetailStep.tsx # Conflicts: # packages/cli/src/gemini.tsx
Hot-reload MCP servers when settings.json changes (issue QwenLM#3696 sub-task 3): editing mcpServers / mcp.allowed / mcp.excluded now connects, disconnects, or restarts only the affected servers in place, without restarting the session or losing conversation context. - Part A: Config runtime setters + reinitializeMcpServers incremental reconcile; align the shared-pool path with the QwenLM#4615 pending-approval gate - Part B: SettingsWatcher subscriber (hotReload.ts), gated on a mcpServers + gating-list diff; flip the three MCP schema keys to hot-reloadable - Part D: re-fire the approval modal for a gated server left pending by an edit - Part E: /mcp shows why a gated server was skipped (pending / rejected) - Record connection fingerprints on the bulk and lazy-connect paths so an edit to a server first connected via those paths is not silently dropped - Design doc (en/zh) incl. the admission-stance boundary clarification Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address reviewer findings on the MCP hot-reload changes: - Extract purgeServerRegistries() and use it at every teardown path, fixing the discovery-timeout handler which leaked prompts/resources (only tools were purged) for a server that stalled tools/list past the timeout. - Surface reconcile failures via AppEvent.LogError so a failed settings edit is visible to the user, not just under --debug. - Make a single-session config edit to a discovery filter (trust / includeTools / excludeTools) reconnect the server so discover() re-applies it: connectionIdOf stays transport-only; add singleSessionConnectedKeyOf and rename connectionFingerprints -> connectedConfigKeys. - Make a coalesced reinitializeMcpServers await the in-flight pass + its drain (store mcpReconcilePromise) so the caller no longer emits approval events / logs "complete" before its change is applied; coalesced callers share the failure. - Assert removeResourcesByServer in the fingerprint-change tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… unavailable (QwenLM#3696) (QwenLM#5561) - K: treat the startup --allowed-mcp-server-names flag as an immutable upper bound — a runtime settings edit may narrow MCP admission within it but never widen beyond it; with no flag, settings fully drive admission. - H: preserve an explicit `mcp.allowed: []` as deny-all (don't collapse to undefined / allow-all), matching boot semantics, and make mcpGatingEqual distinguish absent (allow-all) from [] (deny-all) so the change reconciles. - B: classify why an MCP server is unavailable (removed / not_allowed / excluded / pending_approval) and route the tool-not-found message to the right recovery action; track removals against the gating-independent merged map (dropping the prev-effective snapshot param). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…able reasons (Part F) Reflect the K/H/B changes in the sub-task 3 design doc: add Part F (CLI --allowed-mcp-server-names as an immutable upper bound, mcp.allowed: [] as deny-all, and getMcpServerUnavailableReason routing the tool-not-found message), and fix the now-superseded "settings can widen beyond the startup allowlist" admission-stance note and verification item 11. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The pool/daemon discovery path now honors QwenLM#4615 pending-approval gating, so the workspace-scoped MCP servers the amplification suite declares in .qwen/settings.json are skipped as pending and never spawn (the suite timed out waiting for grandchildren). Add approveWorkspaceMcpServers() to the harness (keyed by the realpath workspace to match the daemon's canonicalized --workspace) and pre-approve the fixtures before boot, mirroring simple-mcp-server.test.ts.
8df319a
bb10b31 to
8df319a
Compare
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM on this head. The substantive threads are resolved: the startup --allowed-mcp-server-names upper bound holds across hot-reload, the triple registry cleanup (tools + prompts + resources) is centralized in purgeServerRegistries and now covers the discovery-timeout path, and package-lock.json carries the new fast-deep-equal dep.
One non-blocking follow-up, already captured in @doudouOUC's thread on mcp-client-manager.ts:1584: single-session reconcile reconnects on trust/includeTools/excludeTools edits via singleSessionConnectedKeyOf, but the pool path still keys on the transport-only connectionIdOf — so in shared-pool / qwen serve mode a hot-reload that only changes those filters is a no-op until restart. Narrow (daemon + filter-only edit), and the fix is just mirroring the single-session key into the pool diff. Fine to land as a fast-follow under #3696, together with the approval-state UI / reconcile-failure test coverage the bots flagged.
Leaving the in-flight Test jobs to finish before merge.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM on this head. The substantive threads are resolved: the startup --allowed-mcp-server-names upper bound holds across hot-reload, the triple registry cleanup (tools + prompts + resources) is centralized in purgeServerRegistries and now covers the discovery-timeout path, and package-lock.json carries the new fast-deep-equal dep.
One non-blocking follow-up, already captured in @doudouOUC's thread on mcp-client-manager.ts:1584: single-session reconcile reconnects on trust/includeTools/excludeTools edits via singleSessionConnectedKeyOf, but the pool path still keys on the transport-only connectionIdOf — so in shared-pool / qwen serve mode a hot-reload that only changes those filters is a no-op until restart. Narrow (daemon + filter-only edit), and the fix is just mirroring the single-session key into the pool diff. Fine to land as a fast-follow under #3696, together with the approval-state UI / reconcile-failure test coverage the bots flagged.
Leaving the in-flight Test jobs to finish before merge.
Duplicate of my approval above — dismissing this one.
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
7 participants
… (#3696)
What this PR does
Implements MCP server runtime hot-reload (Issue #3696, sub-task 3): editing
mcpServers(ormcp.allowed/mcp.excluded) insettings.jsonnow connects/disconnects/restarts only the affected MCP servers in place,without restarting the session or losing conversation context. The existing incremental reconcile already
handled "touch only what changed"; this PR adds the missing runtime entry point on
Config(Part A) and theSettingsWatchertrigger that drives it (Part B). It also aligns the shared-pool discovery path with thesingle-session approval gate, surfaces the mid-session approval modal when a hot-reload leaves a gated server
pending (Part D), and shows in
/mcpwhy a gated server was skipped for approval (Part E).Why it's needed
Before this change, adding/removing/editing an MCP server (or installing an extension) required restarting the
whole CLI session, which drops the conversation context — a poor workflow when iterating on MCP config. The
reconcile machinery existed but had no way to receive updated settings at runtime (
ConfigfrozemcpServersat construction;
addMcpServers()throws post-init) and no subscriber on the watcher. This PR closes that gap,and while doing so fixes two adjacent gaps: a gated server invalidated by an edit was silently left disconnected
with no approval prompt, and
/mcprendered a bare "Disconnected" that didn't distinguish a real connectionfailure from "you rejected / it's awaiting approval".
Reviewer Test Plan
How to verify
npm run devin a trusted workspace that has at least one MCP server configured..qwen/settings.json: (a) add a new MCP server → it connects and itstools appear without a restart; (b) change an existing server's command/URL → it disconnects and reconnects with
the new config, unrelated servers stay connected (no "0 tools" gap); (c) remove a server → it disconnects and
its tools/prompts are removed.
workspace/projectscope) approval: edit a gated server's config → it goespendingand theapproval modal pops mid-session (Part D). Approve → it connects; reject → it stays disconnected.
/mcpvisibility (Part E): for a rejected/pending gated server, the row shows "rejected — edit config tore-approve" / "needs approval" (warning color) instead of a bare "Disconnected", and the "Run qwen --debug to
see error logs" footer no longer appears for it; a genuinely failed connection still shows that footer.
npx vitest run packages/cli/src/config/hotReload.test.ts packages/cli/src/config/mcpApprovals.test.ts packages/cli/src/config/settingsSchema.test.ts packages/cli/src/config/settingsWatcher.test.ts packages/cli/src/ui/hooks/useMcpApproval.test.ts packages/cli/src/ui/components/mcp/steps/ServerListStep.test.tsx packages/core/src/config/config.test.ts packages/core/src/tools/mcp-client-manager.test.ts— all green.Evidence (Before & After)
Before: editing
settings.jsonMCP servers had no effect until a full session restart; a gated server editedmid-session was left "Disconnected" with no prompt and no reason. After: the affected servers reconcile live,
the approval modal re-fires for newly-pending gated servers, and
/mcpstates the skip reason.Add MCP
20260621-add-MCP.mp4
Modify MCP
6.21.mp4
Delete MCP
20260621-delete-MCP.mp4
Tested on
Environment (optional)
Local
npm run dev(core/CLI loaded from source via the tsx loader) on macOS; unit tests via vitest.Risk & Scope
mcpServers/mcp.allowed/mcp.excludedtorequiresRestart: falsechanges the watcher's suppression behavior for those keys (all three are
showInDialog: false, so the settingsdialog's restart prompt is unaffected; blast radius is the watcher path only). Deliberate stance: a runtime
settings.jsonedit may widen MCP admission beyond the startup--allowed-mcp-server-namesallowlist; the#4615pending-approval gate never yields, including on the shared-pool path./reloadcommand (TypeError in Authentication Selection Interface #5),clearAllCaches()(Are you interested in AI Terminal? #4) and theneedsRefreshUI notification (OpenAI API Error: 401 Incorecct API Key provided #6); Windows/Linux notexercised locally; the documented
getTargetDir()vsgetWorkingDir()key mismatch (risk B) is unchangedexcept that Part E's new read uses the write-side
getWorkingDir().(
~/.qwen/mcpApprovals.json, keyed per project) is unchanged.Linked Issues
Progress on #3696 (sub-task 2 of 6: MCP/LSP server runtime re-initialization ).
中文说明
What this PR does
实现 MCP server 运行时热更新(Issue #3696,sub-task 3):在
settings.json里增删改mcpServers(或mcp.allowed/mcp.excluded)后,只对受影响的 MCP server原地连上/断开/重连,无需重启会话、不丢对话上下文。"只动有变化的部分"的增量 reconcile 代码里早已具备,本 PR 补上
Config上缺失的运行时入口(Part A)与驱动它的SettingsWatcher触发(Part B)。同时把共享池 discovery路径对齐到单会话的审批门控,在热更新把某 gated server 打成 pending 时弹出中途审批弹窗(Part D),并在
/mcp显示gated server 因审批被跳过的原因(Part E)。
Why it's needed
改动前,增删改 MCP server(或装扩展)都必须重启整个 CLI 会话,对话上下文随之丢失 —— 在迭代 MCP
配置时体验很差。reconcile 机制虽已存在,但运行时无法接收更新后的 settings(
Config在构造时冻结了mcpServers,启动后调addMcpServers()会抛错),且 watcher 上没有订阅者。本 PR补齐这个缺口,并顺带修掉两个相邻缺陷:被编辑而失效的 gated server 此前会被静默地留在 disconnected
且不弹审批框;
/mcp此前只渲染裸的 "Disconnected",无法区分"真正连接失败"与"被我拒绝 / 待审批"。Reviewer Test Plan
How to verify
npm run dev。.qwen/settings.json:(a) 新增一个 MCP server → 无需重启即连上、工具出现;(b) 改某个已有server 的命令/URL → 它按新配置断开重连,其余 server 保持连接(无「0 工具」空窗);(c) 删除某 server → 它断开,且其
tools/prompts 被移除。
workspace/projectscope)审批:编辑某 gated server 的配置 → 它转为pending且中途弹出审批框(Part D)。批准 → 连上;拒绝 → 保持断开。
/mcp可见性(Part E):对 rejected/pending 的 gated server,该行显示 "rejected — edit config to re-approve" /"needs approval"(warning 颜色)而非裸的 "Disconnected",且底部 "Run qwen --debug to see error logs"
提示不再为它出现;真正连接失败的 server 仍会显示该提示。
npx vitest run packages/cli/src/config/hotReload.test.ts packages/cli/src/config/mcpApprovals.test.ts packages/cli/src/config/settingsSchema.test.ts packages/cli/src/config/settingsWatcher.test.ts packages/cli/src/ui/hooks/useMcpApproval.test.ts packages/cli/src/ui/components/mcp/steps/ServerListStep.test.tsx packages/core/src/config/config.test.ts packages/core/src/tools/mcp-client-manager.test.ts—— 全绿。Evidence (Before & After)
改动前:编辑
settings.json的 MCP server 在重启会话前毫无效果;中途被编辑的 gated server 会留在 "Disconnected"且无提示、无原因。改动后:受影响的 server 实时 reconcile,新转 pending 的 gated server 会重新弹审批框,
/mcp会说明被跳过的原因。
Tested on
Environment (optional)
macOS 上
npm run dev(core/CLI 经 tsx loader 从源码加载);单测经 vitest。Risk & Scope
mcpServers/mcp.allowed/mcp.excluded翻成requiresRestart: false,改变了watcher 对这三个 key 的抑制行为(三者均
showInDialog: false,故设置对话框的重启提示不受影响;blast radius 仅限watcher 路径)。刻意取向:一次运行时
settings.json编辑可以把 MCP 准入放宽到启动--allowed-mcp-server-namesallowlist 之外;但
#4615的 pending 审批门控绝不让步,包括共享池路径。/reload命令(TypeError in Authentication Selection Interface #5)、
clearAllCaches()(Are you interested in AI Terminal? #4) 与needsRefreshUI 通知(OpenAI API Error: 401 Incorecct API Key provided #6);Windows/Linux 未在本地验证;已记录的getTargetDir()vsgetWorkingDir()key 不一致(风险 B)未改动,只是 Part E 的新读取统一用写入端的getWorkingDir()。~/.qwen/mcpApprovals.json,按项目 key分区)不变。