diff --git a/docs/design/hot-reload/mcp-runtime-reinitialization.md b/docs/design/hot-reload/mcp-runtime-reinitialization.md new file mode 100644 index 0000000000..22fc35013e --- /dev/null +++ b/docs/design/hot-reload/mcp-runtime-reinitialization.md @@ -0,0 +1,666 @@ +# MCP Runtime Hot-Reload Design: settings-driven incremental reconnect (Issue #3696 Sub-task 3) + +> Note: sub-task 3's original scope is "MCP/LSP" runtime reconnect; this MR ships **MCP only**. LSP keeps just a sketch + TODO in Part C, deferred to a later MR. + +## Context + +Issue #3696 is the umbrella tracking issue for the hot-reload system. Sub-task 1 +(`SettingsWatcher` file-change detection) is merged, but **has no subscriber yet**— +`gemini.tsx:784` starts the watcher, and the [Sub-task 1 design](./settings-change-detection.md) +explicitly left listener wiring to sub-tasks 2–6. Today, adding/removing/editing an MCP server +in `settings.json` (or installing an extension) requires restarting the whole session, losing +conversation context. + +This MR focuses on **MCP** and delivers two things: (a) a runtime entry point that pushes +reloaded settings into the live `Config`; (b) MCP incremental reconnect driven by +`SettingsWatcher`. LSP runtime reconnect belongs to this sub-task but is not implemented here, +leaving only a Part C TODO. + +**Core observation**: the "reconnect by diff" incremental reconcile already exists in the code +(single-session `discoverAllMcpToolsIncremental`, shared-pool `runDiscoverAllMcpToolsViaPool`, +touching only changed servers by their `connectionIdOf` fingerprint). The only gap is that +`Config` cannot update its settings snapshot after startup (`addMcpServers()` throws, +`config.ts:3200`). Adding that runtime entry point is **Part A**; triggering it from the watcher +is **Part B**—that is the entirety of this MR. Two firm trade-offs: reuse the existing +incremental reconcile rather than the full-wipe `restartMcpServers()` (which causes a "0 tools" +gap); and the shared-pool path must add the `isMcpServerPendingApproval` approval gate to match +the single-session path (Part A item 4). See "Architecture" below for the component overview and +"Design" for the step-by-step flow and details. + +--- + +## Architecture + +In one line: **wire the already-existing incremental reconcile onto settings file changes**, and +fill in the trust boundary and UI feedback along the way. The change splits by responsibility +across the CLI / Core packages, decoupled through `Config` methods and one UI event: + +```text + CLI package Core package + ┌──────────────────────────────────────────┐ ┌────────────────────────────────────┐ + │ SettingsWatcher (sub-task 1, merged) │ │ Config │ + │ └─[Part B] hot-reload.ts │ calls │ └─[Part A] reinitializeMcpServers │ + │ when to fire · recompute gating · gate│ ────▶ │ setMcpServers + incr. reconcile│ + │ │ │ (McpClientManager pool/single)│ + │ └─[Part D] useMcpApproval · approval modal │ ◀──── │ └─[Part A④] pool-path pending gate │ + │ mid-session pending → re-prompt │ event │ │ + │ └─[Part E] /mcp status view │ └────────────────────────────────────┘ + │ show "skipped due to approval" reason │ + └──────────────────────────────────────────┘ +``` + +- **Layering principle**: core must not understand `settings.json` / watcher semantics. + "When to fire" belongs to the CLI (Part B), "how to update + reconcile" belongs to Core + (Part A), consistent with sub-task 1; Part B is Part A's sole consumer, interacting only + through `Config` methods. +- **Main path**: settings change → Part B rebuilds the desired list + gating lists, debounced + gate → calls Part A → Core incremental reconcile (including the pool-path approval gate) → + emits `mcp-client-update` to refresh status indicators. +- **Approval branch**: if reconcile leaves a gated server `pending`, Part D triggers the approval + modal via the `McpPendingApprovalChanged` event; the skip reason is surfaced by Part E in the + `/mcp` view. +- **Hard prerequisite**: the three schema keys `mcpServers` / `mcp.allowed` / `mcp.excluded` must + be flipped to hot-reloadable, otherwise the watcher's restart-required suppression gate swallows + MCP-only edits and the whole chain is inert (see the ⚠️ note at the start of "Design"). + +| Part | Responsibility | Layer | Status | +| ----- | ---------------------------------------------------------------------------------------------------------------------------------------------- | ---------- | --------------- | +| **A** | `Config` runtime-updatable MCP config + incremental reconcile + pool-path approval gate | Core | this MR | +| **B** | subscribe watcher, recompute gating, debounced gate, call Part A | CLI | this MR | +| **C** | LSP reinitialize | Core | TODO (later MR) | +| **D** | mid-session pending triggers the approval modal (and fixes missed prompt #6) | CLI | follow-up | +| **E** | `/mcp` shows the "skipped due to approval" reason | CLI | follow-up | +| **F** | admission semantics: CLI allow-list is an upper bound, `mcp.allowed: []` = deny-all, and tool-not-found explains _why_ a server is unavailable | CLI + Core | follow-up | + +"Design" below gives the step-by-step data flow from disk file to live connection, plus each +part's implementation details. + +--- + +## Design + +The diagram below is the full data flow of one settings change from "disk file" to "connection +takes effect" (`[CLI]` = Part B, `[Core]` = Part A, `[sub-task 1]` = the merged watcher): + +```text +① User edits .qwen/settings.json (add/remove/edit mcpServers, or mcp.excluded / mcp.allowed) + │ + ▼ +② [sub-task 1] SettingsWatcher detects the file change + │ · 300ms debounce: coalesce consecutive saves + │ · whole-file semantic diff: notify only if content really changed (self-write / pure formatting → no notify) + ▼ +③ [CLI · Part B] the callback registered by registerMcpHotReload fires (any settings change reaches it) + │ + ├─ a. assembleMcpServers(settings.merged.mcpServers, cwd, topTier) + │ → merge by priority into the full server list `next` (incl. .mcp.json / --mcp-config / session) + ├─ b. recompute the connection-gating lists nextGating = { excluded, allowed, pending } + └─ c. gate: mcpServersEqual(old, next) AND mcpGatingEqual(old, nextGating) both "unchanged" + → early return (ignore theme / skills and other MCP-irrelevant edits) + │ (continue only if mcpServers OR the mcp gating lists changed ↓) + ▼ +④ [CLI→Core] push the gating lists into config first (discovery reads them during reconcile): + config.setExcludedMcpServers / setAllowedMcpServers / setPendingMcpServers + │ + ▼ +⑤ [Core · Part A] config.reinitializeMcpServers(next) + │ (wrapped by a "reconcile in progress" guard to avoid racing with /reload) + ├─ a. setMcpServers(next): replace the settings-layer snapshot (extension / runtime layers untouched) + └─ b. discoverAllMcpToolsIncremental: reconciliation-style incremental reconcile + · compute each server's connectionIdOf fingerprint, compare "desired" vs "online" + · added → connect; removed → disconnect + drop tools/prompts; + fingerprint changed → disconnect + drop old tools/prompts, then reconnect with new config; unchanged → keep + · skip disabled / pending / untrusted dir; emit mcp-client-update + │ + ▼ +⑥ [CLI · Part B] UI wrap-up: mcp-client-update refreshes the MCP status indicators; + (optional) MCP prompts changed → reloadCommands(); set needsRefresh (sub-task 6) +``` + +> **Trigger timing**: `registerMcpHotReload` runs only once at startup (attach the listener, +> return a disposer); the callback it registers is what fires **on every settings change** via the +> watcher (i.e. from step ③ onward)—that is when reconcile actually runs. + +> ⚠️ **Hard prerequisite: three MCP schema keys must be flipped to hot-reloadable (the hidden +> switch in step ②).** The watcher has a "restart-required suppression gate": if **all** keys +> touched by a change are `requiresRestart: true`, it **emits no event**. But `mcpServers` / +> `mcp.allowed` / `mcp.excluded` were all `true`—so an MCP-only edit never fires the callback and +> Part B is inert. This MR **must** flip these **three leaves** to `false`; the parent node `mcp` +> and the startup-only `mcp.serverCommand` stay `true` (matching uses `isRestartRequiredKey` +> longest-prefix match + `flattenSchema`, leaf wins). All three are `showInDialog: false`, so the +> flip does not change the settings dialog's restart prompt; the blast radius is the watcher path only. + +The following describes Part A (Core capabilities), Part B (CLI wiring), Part C (LSP, TODO only in +this MR) in turn. + +### Part A — Core: make Config runtime-updatable for MCP config and trigger incremental reconcile + +**File: `packages/core/src/config/config.ts`** + +1. Add a post-init setter that updates the settings snapshot reconcile reads: + + ```ts + /** + * Runtime (hot-reload) replacement of the settings-layer MCP server map. + * Unlike addMcpServers(), it bypasses the `initialized` guard and is a REPLACE + * (not a merge), so removals take effect. The runtime overlay + * (addRuntimeMcpServer) and extension contributions are unaffected—getMcpServers() + * still layers on top of it. + */ + setMcpServers(servers: Record | undefined): void { + this.mcpServers = servers; + } + ``` + + `getMcpServers()` (`:3128`) already layers extensions + `runtimeMcpServers` on top of + `this.mcpServers`, so replacing only the settings layer is safe for runtime/extension entries. + +2. **Connection-gating lists**: the three name lists that decide whether each MCP server may + connect—`excluded` (blocked), `allowed` (if set, only these connect), `pending` (gated source, + needs user approval before connecting). These are separate from `mcpServers` (server config): + the former governs "**whether** to connect", the latter "**which servers and how**". Add setters + for these three lists that `getMcpServers()` / discovery consult: `setExcludedMcpServers()` + exists (`:3167`); add `setAllowedMcpServers()` (the field is currently `readonly` and used as a + filter inside `getMcpServers()`) plus a setter for the pending-approval set. + +3. Add a lightweight orchestration method: update config first, then drive the existing + incremental reconcile, wrapped by a shared "reconcile in progress" guard so `/reload` + (sub-task 5) and the watcher don't race: + + ```ts + /** + * Apply a new settings-layer MCP map and incrementally reconcile live connections + * (connect added, disconnect removed, restart changed; keep unchanged untouched). + * Calling before initialize() is a safe no-op. + */ + async reinitializeMcpServers(servers: Record | undefined): Promise { + this.setMcpServers(servers); + const registry = this.getToolRegistry(); + await registry.getMcpClientManager().discoverAllMcpToolsIncremental(this); + } + ``` + + `discoverAllMcpToolsIncremental` already checks `isTrustedFolder()`, handles disabled/SDK + servers, and emits `mcp-client-update` to refresh the UI status indicators. Removed server → + release + drop tools/prompts; fingerprint changed → release + re-acquire; unchanged → keep. + +4. **Add the pending-approval check to the shared-pool path** (trust boundary, mandatory in this + MR): the single-session path skips servers pending approval, but when a shared pool exists + `discoverAllMcpToolsIncremental` delegates to `runDiscoverAllMcpToolsViaPool`, and **the pool + path only skips disabled / SDK, not `isMcpServerPendingApproval`** (around + `mcp-client-manager.ts:1461`). Without this fix, in daemon / shared-pool mode a hot-reload that + adds/edits a gated `.mcp.json` / workspace server would acquire a pool connection and spawn the + process **before** the user approves, bypassing the #4615 approval gate. Fix: add the + `isMcpServerPendingApproval` check in the pool path **before building `desiredIds` and before + acquire**, making its admission semantics match the single-session path. + +### Part B — CLI: subscribe SettingsWatcher → MCP reconcile + +**New file: `packages/cli/src/config/hot-reload.ts`**, wired after +`settingsWatcher.startWatching()` (`:785`) in `gemini.tsx`. + +```ts +export function registerMcpHotReload( + watcher: SettingsWatcher, + settings: LoadedSettings, + config: Config, + topTierMcpServers: Record | undefined, +): () => void { + return watcher.addChangeListener(async (events) => { + // Rebuild exactly the way Config boot did—including top-tier (CLI/session) sources. + const next = assembleMcpServers( + settings.merged.mcpServers, + config.getTargetDir(), + topTierMcpServers, + ); + // Recompute the gating lists (excluded/allowed/pending)—[settings at hot-reload time win], + // see the "admission stance" decision below; pending is always recomputed per the #4615 gate. + const nextGating = { + excluded: recomputeExcluded(settings, next), + allowed: recomputeAllowed(settings, next), + pending: recomputePending(settings, next), + }; + // gate: reconcile only if mcpServers OR the mcp gating lists changed; + // if both unchanged, early-return (ignore theme / skills and other MCP-irrelevant edits). + const serversChanged = !mcpServersEqual( + config.getSettingsMcpServers(), + next, + ); + const gatingChanged = !mcpGatingEqual(config.getMcpGating(), nextGating); + if (!serversChanged && !gatingChanged) return; + // Push the gating lists into config before reconcile (discovery inside reinitializeMcpServers reads them). + config.setExcludedMcpServers(nextGating.excluded); + config.setAllowedMcpServers(nextGating.allowed); + config.setPendingMcpServers(nextGating.pending); + await config.reinitializeMcpServers(next); + // Notify UI: MCP prompts changed → reloadCommands(); set needsRefresh (sub-task 6). + }); +} +``` + +> **Admission stance decision (deliberate)**: hot-reload makes **current settings win _within_ the +> startup `--allowed-mcp-server-names` bound** — a runtime edit to `mcp.allowed` / `mcp.excluded` in +> `settings.json` takes effect immediately, but **only narrows admission, never widens it beyond the +> launch flag** (see Part F for the upper-bound rule and the `mcp.allowed: []` semantics). If no +> `--allowed-mcp-server-names` flag was passed, settings fully drive admission. **The pending-approval +> gate (#4615) never yields** regardless: a gated server must always be approved first (Part A item 4). +> +> > _History_: an earlier revision let a runtime settings edit widen admission beyond the startup +> > flag (treating the flag as a mere name-filter convenience). Adversarial review flagged that as a +> > silent loosening of a launch-time boundary; Part F (item K) reverses it — the flag is now an +> > immutable upper bound. + +Reuse existing helpers—**do not** reimplement the merge logic: + +- `assembleMcpServers(settings.mcpServers, cwd, topTierMcpServers)`— + `packages/cli/src/config/mcpServers.ts:27` (matching the Config boot call at + `packages/cli/src/config/config.ts:1812`). +- `SettingsWatcher.addChangeListener` returns an unsubscribe function (`settingsWatcher.ts:253`). +- `config.getSettingsMcpServers()` (`:3124`) as the pre-image for the `mcpServers` diff; + `config.getMcpGating()` as the pre-image for the gating-list diff (a small new getter returning + `{ excluded, allowed, pending }`, paired with Part A's setters). + +The gate uses two small pure functions to narrow the trigger surface (avoid theme / skills and +other irrelevant edits triggering redundant reconcile, consistent with the watcher's own semantic +diff), both **reusing `fast-deep-equal`** (the cli package must promote it from a transitive to a +direct dependency): + +- `mcpServersEqual(a, b)`: object key order irrelevant (eliminates false positives from server / + field ordering), array order sensitive (`args` and other command-argument order has meaning); + `undefined` ≡ `{}`. +- `mcpGatingEqual(a, b)`: `excluded` / `allowed` / `pending` compared as **sets** (sort copies + first); `undefined` ≡ `[]`. It is precisely what lets "edit only `mcp.excluded` / `mcp.allowed`, + leave `mcpServers` untouched" still trigger reconcile—closing the gap where diffing only + `mcpServers` would miss gating changes. + +UI wrap-up refreshes the status indicators via the existing `mcp-client-update` event, setting +`needsRefresh` when needed (sub-task 6). The floor for this sub-task: config-level reconcile +completes + the existing emit refreshes status. + +### Part C — LSP reinitialize (not implemented in this MR, TODO) + +LSP config comes from `.lsp.json` + extension config (**not** `settings.json`), so it is **not +auto-triggered by SettingsWatcher**; its runtime reconnect should be driven manually by the later +`/reload` command (sub-task 5). `NativeLspService` (gated by `--experimental-lsp`) already has +lifecycle methods `discoverAndPrepare` / `start` / `stop`, enough to implement a `reinitialize()` +primitive exposed to `/reload` via `LspClient.reinitialize?()` + `Config.reinitializeLsp()`, +without major changes. + +> **TODO (next MR)**: implement `NativeLspService.reinitialize()` and its exposure via +> `Config.reinitializeLsp()`, with a detailed design in that MR's doc (including that +> `discoverAndPrepare()` first calls `clearServerHandles()`, preventing an incremental diff, so v1 +> uses stop-all → start-all, etc.). **This MR contains no LSP code changes.** + +### Part D — Follow-up: hot-reload triggers the runtime approval modal for gated servers (ties into #4615) + +> This section was added after Parts A/B landed, while debugging "changed a gated server's URL but +> it doesn't reconnect". It fixes the break where "hot-reload marks a gated server pending but the +> UI shows no approval modal", and incidentally fixes a missed prompt caused by the decision logic +> (issue #6 below). + +#### Background: the approval modal was computed only once at startup + +A gated-source server (`project`'s `.mcp.json` and `workspace`'s `.qwen/settings.json`, see +`isGatedMcpScope`) has its user approval **bound to the config hash** (`mcpApprovals.ts`'s +`getState`: no record, or a record whose hash differs from the current config → `pending`). So if a +hot-reload changes a gated server's config (even just `httpUrl`), its hash change invalidates the +old approval and it becomes `pending` again. + +The Part A/B chain handles this **correctly**: `recomputeMcpGating` puts it in `pending`, +`setPendingMcpServers` pushes it to discovery, and reconcile skips it (no connect, state +`disconnected`). But **the UI shows no approval modal**—the root cause is that `useMcpApproval` +(the hook driving the approval modal) computes its queue only **on mount** via +`useEffect(…, [config])`, and the `config` reference is stable across the session → the effect +never re-runs. That is: + +- core marks the server pending (discovery skips it) ✓ +- the UI's approval queue never recomputes → **no modal** ✗ (the user only sees `disconnected`, with no way to approve) + +The two paths are **disconnected** at runtime. + +#### Fix: connect core→UI via an event, hand the decision to the UI + +1. **Add event** `AppEvent.McpPendingApprovalChanged` (`packages/cli/src/utils/events.ts`). Since + `appEvents` is in the CLI layer and `hot-reload.ts` is too, the listener can emit directly, with + **no core change**. + +2. **`hot-reload.ts` emits after reconcile** (placed after `await reinitializeMcpServers`, so + `config.getMcpServers()` already reflects the new map; emit regardless of reconcile + success/failure—a server left pending still needs a user decision). + +3. **`useMcpApproval` extracts `computePending()`**: compute once on mount (existing behavior) + **plus** recompute the queue after subscribing to `McpPendingApprovalChanged` → a non-empty + queue shows the modal. `computePending` recomputes from authoritative sources (the live server + map + the persisted approval file), so already-approved / already-rejected servers are not + re-prompted. + +#### Key design: drive emit by "strict pending", not a name set-difference (issue #6 / A1 decision) + +Note the two predicates are **deliberately different**, which is the heart of this section: + +| Function | Predicate | Use | +| ------------------------------- | ---------------------------------------------- | ----------------------------------------------------- | +| `getPendingGatedMcpServers` | `state !== 'approved'` (**includes rejected**) | feeds discovery: rejected must keep being **skipped** | +| `getPromptableMcpServers` (new) | `state === 'pending'` (**excludes rejected**) | feeds the modal: rejected is **no longer nagged** | + +The initial emit decision used "the name set-difference of `nextGating.pending` vs last time" to +decide whether to show the modal, which had a missed prompt (review issue #6): + +- a **rejected** server stays in the `pending` list because of `!== 'approved'`; +- the user then **re-edits that same server's config** (hash changes → it genuinely becomes + `pending` again and should be re-asked), but its name was "already in" the list → the + set-difference is empty → **no event → missed prompt**. + +A1 fix: use `getPromptableMcpServers(next, cwd)` (strict `=== 'pending'`) to decide emit, handing +the truth of the decision to `computePending`. Effect: + +- after reject, **edit the same server's config** (hash changes) → `pending` again → **re-prompt** ✓ (fixes #6) +- after reject, an **unrelated** edit (hash unchanged) → still `rejected` → not promptable → **no prompt** ✓ +- already `approved` → no prompt; a new undecided gated server → prompt ✓ + +#### reject semantics (confirmed after review) + +`handleMcpApprovalSelect(REJECT)`: persists `rejected` (bound to the current hash), does **not** +call `reconnect`, does **not** touch `config.pendingMcpServers` → discovery keeps skipping → the +server stays `disconnected`. No need to actively tear down the old connection: emit happens after +the `reinitializeMcpServers` await, so by the time the modal appears reconcile has already torn it +down. After a session restart `computePending` reads `rejected` → not enqueued, stays disconnected, +consistent behavior. + +#### Data-flow addendum (continues after ⑥ in the chapter's overview diagram) + +```text +⑥' [CLI · Part D] after reconcile, if a strictly pending gated server exists: + hot-reload → appEvents.emit(McpPendingApprovalChanged) + → useMcpApproval.computePending() recomputes the queue → shows the approval modal + → user approves: approveMcpServerForSession + discoverToolsForServer (connect with new config) + user rejects: persist rejected, stay disconnected +``` + +#### Key files (Part D) + +| File | Change | +| --------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------- | +| `packages/cli/src/utils/events.ts` | add `AppEvent.McpPendingApprovalChanged` | +| `packages/cli/src/config/mcpApprovals.ts` | add `getPromptableMcpServers()` (strict `=== 'pending'`, distinct from the rejected-inclusive `getPendingGatedMcpServers`) | +| `packages/cli/src/config/hot-reload.ts` | after reconcile, decide via `getPromptableMcpServers`; if non-empty, `appEvents.emit(McpPendingApprovalChanged)` | +| `packages/cli/src/ui/hooks/useMcpApproval.ts` | extract `computePending()`; compute once on mount + recompute on the event | + +#### Verification (Part D) + +- `hot-reload.test.ts`: a gated server newly pending → emit; non-gated change → no emit; + **reject→edit config → emit again** (the old name set-difference would be 0 times, locking down + the #6 regression); reject→unrelated edit → no emit. +- `mcpApprovals.test.ts`: the `getPromptableMcpServers` suite—no decision prompts, rejected does + not prompt (vs `getPendingGatedMcpServers` still skipping), re-prompt after hash change, approved + does not prompt. +- `useMcpApproval.test.ts`: a mid-session event makes a new gated server show the modal; an + already-approved one is not re-prompted. + +#### Known issue / retrospective TODO (NOT handled here) + +1. **`getTargetDir()` vs `getWorkingDir()` key mismatch (risk B)**: gating recompute + (`recomputeMcpGating` → `getPendingGatedMcpServers`) uses `config.getTargetDir()` as the + projectRoot, while `useMcpApproval` reads/writes approval using `config.getWorkingDir()`. They + are usually equal; once they diverge (custom cwd, or symlink realpath differences), approval is + written under the cwd-key while gating queries the targetDir-key → **after approve, gating still + skips and never connects**. A pre-existing issue, not introduced by Part D. Recommend unifying on + one root (lean toward `getWorkingDir()`, i.e. the approval write side), or first add an assertion + that they are equal at runtime. + +### Part E — Follow-up: show in `/mcp` why a gated server was skipped for approval + +> This section was added after Part D landed, while debugging "after rejecting a gated server then +> deleting and re-adding it identically, `/mcp` shows Disconnected with no hint". Conclusion first: +> **this is not a record-lifecycle bug; the only defect is that the skip reason is invisible**, so +> we only add visibility and touch no approval-storage / reconcile logic. + +#### Why "no longer prompting" is as-designed + +An approval record is bound to **(projectRoot, serverName, hash)** and is **independent of whether +the server is currently present in config**—nothing deletes a record when a server disappears from +config. Hence: + +- **approved already persists across remove/re-add**: approve (hash H) → delete → re-add + identically (still hash H) → `getState` returns `approved` → silent reconnect. An intentional + convenience. +- **rejected matching that settled rejection on the same "identical re-add" is symmetric and + consistent**: a settled rejection stays in effect while the config hash is unchanged; the only + way to re-surface it is to **edit the config (change the hash)** (i.e. Part D's + `getPromptableMcpServers` strict-pending re-prompt path). + +> Therefore we **deliberately do not introduce "forget the record on removal"**: that would let +> presence transitions mutate persistent decisions, violating the principle that decisions change +> only via hash or explicit action, and creating an approved / rejected asymmetry. + +#### The actual defect and fix (visibility only) + +`/mcp` (`ServerListStep` / `ServerDetailStep`) rendered a bare `Disconnected`, making "I rejected +it / awaiting approval" indistinguishable from "a genuine connection failure", so the user did not +know the recovery path (edit config to change the hash → re-prompt). Fix: add +`approvalState?: 'pending' | 'rejected'` to `MCPServerDisplayInfo`, computed in +`MCPManagementDialog.fetchServerData` using `loadMcpApprovals` + `isGatedMcpScope`, keyed by +**`config.getWorkingDir()`** (left empty for non-gated / approved); the list / detail views, using +the existing `needsAuth` override pattern, show the reason first +(`rejected → "rejected — edit config to re-approve"`, `pending → "needs approval"`, warning +yellow), and exclude these non-error approval-skips from the footer "see error logs" hint. + +> Keying on the write side's `getWorkingDir()` here is exactly the direction recommended by Part D's +> "Known issue 1 (risk B)"—read and write approval with the same root. `hot-reload.ts`'s existing +> gating query still uses `getTargetDir()` (they are equal today); this section does not change its +> behavior. It **does not touch** `mcpApprovals.ts` storage, the `hot-reload.ts` removal/reconnect +> path, and adds no approval action. + +#### Key files (Part E) + +| File | Change | +| --------------------------------------------------------------- | ---------------------------------------------------------------------------------------- | +| `packages/cli/src/ui/components/mcp/types.ts` | `MCPServerDisplayInfo` adds `approvalState?: 'pending' \| 'rejected'` | +| `packages/cli/src/ui/components/mcp/MCPManagementDialog.tsx` | `fetchServerData` computes `approvalState`, keyed by `getWorkingDir()` | +| `packages/cli/src/ui/components/mcp/steps/ServerListStep.tsx` | render the approval reason; exclude approval-skips from the footer "see error logs" hint | +| `packages/cli/src/ui/components/mcp/steps/ServerDetailStep.tsx` | render the approval reason (consistent with the list) | + +#### Verification (Part E) + +- `ServerListStep.test.tsx`: gated `rejected` → shows the re-approve hint text; `pending` → "needs + approval"; an approval-skip does **not** show the "see error logs" hint, while a genuinely failed + connection **still** does. +- Manual: reject a workspace server → `/mcp` shows the reason (not a bare Disconnected) → edit its + config to change the hash → the Part D modal reappears (the existing recovery path, unchanged here). + +### Part F — Follow-up: admission semantics (CLI upper bound, deny-all, unavailable reasons) + +> Added after a third adversarial-review pass on Parts A/B. Three related admission refinements, +> grouped because they share the "which servers may connect, and how do we explain when one can't" +> surface. Items labelled K / H / B after their review threads. + +#### K — the startup `--allowed-mcp-server-names` flag is an immutable upper bound + +Reverses the earlier "settings always win" stance (see the Part B note). At boot, `loadCliConfig` +gives the flag precedence over `settings.mcp.allowed`; but the hot-reload recompute read `allowed` +from settings only, so any settings change silently dropped a launch-time name restriction — +loosening, in-session, a boundary an operator set precisely to constrain which local MCP commands +may run. + +Fix: capture the **flag value alone** as an immutable bound on `Config` +(`cliAllowedMcpServerNames` param → `getCliAllowedMcpServerNames()`; distinct from the mutable +`allowedMcpServers` that hot-reload overwrites). `recomputeMcpGating` then caps the settings-derived +allow-list to it: + +- flag passed + settings has `mcp.allowed` → **intersection** (settings may narrow within the bound); +- flag passed + no settings `mcp.allowed` → the **flag in full**; +- no flag → settings fully drive admission (unchanged). + +So a runtime edit can only ever narrow MCP admission below the launch flag, never widen past it. +`mcp.excluded` still narrows further at discovery time, consistent with "only stricter, never looser". + +#### H — `mcp.allowed: []` is deny-all, consistently across boot and hot-reload + +Boot treats an empty allow-list as deny-all (`getMcpServers()` filters whenever `allowedMcpServers` +is truthy, and `[]` is truthy). The hot-reload recompute used to collapse `[]` → `undefined` +("allow all") — so editing `mcp.allowed` to `[]` expecting deny-all left every server reachable. Fix: +`recomputeMcpGating` preserves `[]` (only an **absent** key yields `undefined`), and `mcpGatingEqual` +distinguishes absent (allow-all) from `[]` (deny-all) for `allowed` — otherwise the change would +compare equal and never reconcile. `excluded` / `pending` keep `undefined ≡ []` (both "no entries"). + +#### B — tool-not-found explains _why_ a server is unavailable + +`getMcpToolUnavailableMessage` previously distinguished only "removed this session" vs "not +configured". With admission gating it now classifies the owning server via a single core API, +`Config.getMcpServerUnavailableReason(name)`, covering every gate: + +| reason | meaning | recovery the message suggests | +| ------------------ | --------------------------------------------- | ------------------------------------------------- | +| `removed` | deleted from the merged config this session | re-add it to settings | +| `not_allowed` | filtered out by `mcp.allowed` / the CLI bound | add it to `mcp.allowed` | +| `excluded` | listed in `mcp.excluded` | remove it from `mcp.excluded` | +| `pending_approval` | gated server awaiting approval (#4615) | approve it (run `/mcp`) | +| _(none)_ | configured & admitted | genuine "tool not found" (disconnected / renamed) | + +Two supporting changes: a private `getMergedMcpServers()` (the merge **without** the allow-list +filter) so "configured" can be told apart from "filtered out"; and removal tracking now diffs that +**gating-independent merged map**, which means a server filtered by a narrowed allow-list is no +longer mis-reported as `removed` (it's `not_allowed`). That also lets the +`prevEffectiveServerNames` snapshot param added for the earlier allow-list-narrowing fix be dropped +— the merged-map diff is unaffected by the gating setters the caller applies just before reconcile. + +#### Key files (Part F) + +| File | Change | +| ----------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `packages/cli/src/config/config.ts` (`loadCliConfig`) | pass the `--allowed-mcp-server-names` flag value alone as `cliAllowedMcpServerNames` | +| `packages/core/src/config/config.ts` | `cliAllowedMcpServerNames` field + `getCliAllowedMcpServerNames()` (K); `getMergedMcpServers()` (unfiltered) + `getMcpServerNames()`; `McpServerUnavailableReason` + `getMcpServerUnavailableReason()` (B); removal tracking diffs the merged map and `reinitializeMcpServers` drops the `prevEffectiveServerNames` param | +| `packages/cli/src/config/hot-reload.ts` | `recomputeMcpGating` caps `allowed` to the boot bound (K) and preserves `[]` (H); `mcpGatingEqual` makes `allowed` absent ≠ `[]` (H) | +| `packages/core/src/core/coreToolScheduler.ts` | `getMcpToolUnavailableMessage` routes per `getMcpServerUnavailableReason` (B) | + +#### Verification (Part F) + +- `hot-reload.test.ts`: **K** — with a startup flag and no settings allow-list, applies the flag in + full; a settings allow-list is capped to the flag (cannot widen) and may narrow within it; without + the flag, settings win unbounded. **H** — `mcp.allowed: []` is pushed through as deny-all; + `mcpGatingEqual` treats `allowed` absent vs `[]` as different (but `excluded` undefined ≡ `[]`). +- `config.test.ts`: `getMcpServerUnavailableReason` returns `not_allowed` / `excluded` / + `pending_approval` / `removed` for each gate, and `undefined` for a configured-admitted or + never-configured server. +- `coreToolScheduler.test.ts`: the tool-not-found message names the right server and recovery action + per reason. + +--- + +## Out of scope (other sub-tasks) + +- **The entire LSP runtime reconnect** (`NativeLspService.reinitialize()` + + `Config.reinitializeLsp()` + wiring)—deferred to a later MR, see Part C's TODO. +- The `/reload` slash command (#5)—calls `config.reinitializeMcpServers(currentSettings)` (the LSP + part wires up once its primitive lands in a later MR) + skill/command reload. +- `clearAllCaches()` (#4) and the `needsRefresh` UI notification (#6). + +## Key files + +| File | Change | +| ----------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `packages/core/src/config/config.ts` | `setMcpServers()`, `setAllowedMcpServers()` + pending setter, `getMcpGating()` (returns `{ excluded, allowed, pending }`), `reinitializeMcpServers()` (with a reconcile-in-progress guard) | +| `packages/core/src/tools/mcp-client-manager.ts` | ① add `removePromptsByServer()` to `removeServer()` and `removeRuntimeMcpServer()`; ② in the shared-pool path `runDiscoverAllMcpToolsViaPool` (`:1461`), add the `isMcpServerPendingApproval` check before building `desiredIds` / before acquire (matching single-session admission); ③ **add fingerprint diff to the single-session path**: a new `connectionFingerprints` map; `discoverAllMcpToolsIncremental` also triggers disconnect+reconnect for a server that is "connected but its `connectionIdOf` fingerprint changed" (aligned with the pool path's `desiredIds`), clearing the map on every teardown path; ④ **clear old tools/prompts before reconnect**: when `discoverMcpToolsForServerInternal` replaces an existing client, `removeMcpToolsByServer` + `removePromptsByServer` before re-discovery—because `disconnect()` doesn't touch the registry and `discover()` only appends/overwrites by name, otherwise tools dropped/renamed by a config change would linger bound to a closed client (and linger on discovery failure too), matching the existing cleanup in `removeServer` / `addRuntimeMcpServer` | +| `packages/cli/src/config/settingsSchema.ts` | **prerequisite**: flip the three keys `mcpServers` (`:274`), `mcp.allowed`, `mcp.excluded` from `requiresRestart: true` to `false`, so the watcher no longer suppresses MCP-only edits; the parent `mcp` and `mcp.serverCommand` stay `true` (see the "Hard prerequisite" note above) | +| `packages/cli/src/config/hot-reload.ts` _(new)_ | `registerMcpHotReload()`: rebuild via `assembleMcpServers(..., topTierMcpServers)`; recompute the gating lists from current settings (see "admission stance decision"); gate via `mcpServersEqual` + `mcpGatingEqual` (built on `fast-deep-equal`); debounce + coalesce-and-recheck | +| `packages/cli/package.json` | promote `fast-deep-equal` from a transitive to a **direct** dependency | +| `packages/cli/src/gemini.tsx` | call `registerMcpHotReload` after `:785`; register the disposer | +| Tests _(alongside the schema flip)_ | `settingsSchema.test.ts` pins the three MCP keys' `requiresRestart` values (incl. `mcp` / `mcp.serverCommand` staying `true`); `settingsWatcher.test.ts` adds two positive regressions ("edit only `mcpServers` / only `mcp.excluded` → still notify"); `settingsUtils.test.ts` uses its **own mock schema**, unrelated to the real flip, no change needed | + +> LSP-related files (`NativeLspService.ts` / `NativeLspClient.ts` / `lsp/types.ts`) are unchanged in this MR, see the Part C TODO. + +## Verification + +### A. Core capability unit tests (core, `config.test.ts` / `mcp-client-manager.test.ts`) + +1. `setMcpServers` is a **replace (not merge)** and takes effect post-init (no longer throws via + the `initialized` guard). +2. `reinitializeMcpServers` calls `setMcpServers` first then `discoverAllMcpToolsIncremental`; + calling before `initialize()` is a **safe no-op** (no throw, no connect). +3. Assert `removeServer()` / `removeRuntimeMcpServer()` now call `removePromptsByServer()` (prompt + leak regression guard). Reuse `mcp-client-manager.test.ts` fixtures (which already import + `connectionIdOf`). + 3b. **Single-session fingerprint diff**: a mock client whose `getStatus()` is always + `CONNECTED`, run `discoverAllMcpToolsIncremental` three times—first connect records the + fingerprint; same config rerun does **not** churn (`connect` still 1×); changing `args` in place + (fingerprint changes) → disconnect+reconnect (`disconnect` 1×, `connect` 2×). Guards that the + single-session path no longer misses "connected but config changed" as a no-op (aligned with the + shared pool's `desiredIds`). Also assert this run calls `removeMcpToolsByServer` + + `removePromptsByServer` for that server before re-discovery—guarding "clear old tools/prompts + before reconnect", preventing tools dropped/renamed by a config change from lingering. + +### A'. watcher↔schema integration guard (cli, `settingsSchema.test.ts` / `settingsWatcher.test.ts`) + +> These two are **high**-severity integration breaks: an MCP-only edit gets swallowed by the +> watcher's restart-required suppression gate, so the Part B callback never fires. There **must** be +> real watcher-layer coverage; directly calling the callback in `hot-reload.test.ts` cannot catch +> this failure. + +3c. **schema pinning** (`settingsSchema.test.ts`): `mcpServers` / `mcp.allowed` / `mcp.excluded` +have `requiresRestart` `false`; the parent `mcp` and `mcp.serverCommand` are `true`. Prevents +someone from flipping MCP keys back to restart-required and silently killing the whole hot-reload. +3d. **real watcher no longer suppresses** (`settingsWatcher.test.ts`, with a real `SettingsWatcher` + +- mock fs): editing only `mcpServers` / only `mcp.excluded` each triggers **one** + `SettingsChangeEvent` (it would be suppressed before the flip). This is the end-to-end regression + guard that the sub-task 3 listener can actually fire. + +### B. Subscriber gate-branch unit tests (cli, `hot-reload.test.ts`) + +Fake a `SettingsWatcher`, covering every gate branch: + +4. **`mcpServers` changes** → call `reinitializeMcpServers` with the **assembled** map (incl. top-tier). +5. **edit only `mcp.excluded` (or `mcp.allowed` / pending), leave `mcpServers` untouched** → + **still** triggers reconcile, and before reconcile already called `setExcludedMcpServers` / + `setAllowedMcpServers` / `setPendingMcpServers`. This verifies the `mcpGatingEqual` branch—the + fixed gap: diffing only `mcpServers` would miss this change. +6. **neither `mcpServers` nor the `mcp` gating lists changed** (e.g. theme / skills edit) → **does + not** call `reinitializeMcpServers` (verifies the early return when both gates are "unchanged"). +7. **two changes fired during an in-flight reconcile** → coalesce-and-recheck runs once more + (re-entrancy). +8. **debounce**: multiple consecutive saves (< 300ms) trigger reconcile **once** (aligned with the + watcher's 300ms debounce). + +### C. gate-helper pure-function unit tests (cli, `hot-reload.test.ts`) + +9. `mcpServersEqual`: different key order, same values → `true`; nested config fields (`args` / + `env` / `headers`) change → `false`; `undefined` vs `{}` → `true`; add/remove a server → + `false`; `args` array order change → `false` (command-argument order has meaning). +10. `mcpGatingEqual`: the three lists compare "order-independent" (`['a','b']` vs `['b','a']` → + `true`); add/remove an item in any list → `false`; `undefined` vs `[]` → `true`. + +### D. Trust-boundary edge cases (cli + core) + +> Both are **high**-severity trust-boundary points. Item 11 verifies the admission bound (Part F +> item K — settings narrow within, never widen beyond, the startup flag); item 12 corresponds to +> Part A item 4 (pool-path pending check). + +11. **Hot-reload admission narrows within — but never widens beyond — the startup flag** (the Part F + item K bound; supersedes the earlier "settings can widen" stance). Start with + `--allowed-mcp-server-names=a,b`; then a settings change sets `mcp.allowed` to `[a, b, c]`. + **Assert**: after reconcile `c` is **still excluded** (capped to the launch bound) while `a` is + admitted; a settings edit narrowing to `[a]` takes effect; with no startup flag, the settings + allow-list wins unbounded. (See Part F → Verification for the full matrix.) + _Guards_: `recomputeMcpGating` intersects the settings allow-list with + `getCliAllowedMcpServerNames()` and never widens past it. + +12. **The pending-approval gate is not bypassed in shared-pool mode** (high risk: connecting a gated + server before approval). In daemon / shared-pool mode (`runDiscoverAllMcpToolsViaPool`), let a + settings hot-reload add/edit a server pending approval (`.mcp.json` / workspace). **Assert**: + before the user approves, it does **not** acquire a pool connection or spawn the process; a + rejected gated server stays disconnected. Compared to the single-session path which already skips + pending, this test guards the pool path. + _Guards_: Part A item 4—the pool path's `isMcpServerPendingApproval` check before building + `desiredIds` / before acquire. + +### E. reconcile edge cases (recommended coverage, verifying "incremental, not full-wipe") + +13. **empty ↔ non-empty**: from 0 servers to 1 (the first), from 1 to 0 (the last) both reconcile + correctly, leaving no residual connection / tools / prompts. +14. **a fingerprint change touches only that one server**: changing a server's `command` / `url` / + `env` / `headers` → only it disconnects+reconnects, **all other connections kept** (verifies no + full-wipe, no "0 tools" gap). +15. **untrusted dir**: when `isTrustedFolder()` is false, hot-reload is a no-op (establishes no + connection). +16. **`mcp.excluded` toggle**: adding an online server to excluded → it disconnects + tools/prompts + cleared; removing it from excluded → it reconnects. diff --git a/integration-tests/cli/_daemon-harness.ts b/integration-tests/cli/_daemon-harness.ts index 5d8e4d85e7..70783494f3 100644 --- a/integration-tests/cli/_daemon-harness.ts +++ b/integration-tests/cli/_daemon-harness.ts @@ -41,6 +41,10 @@ import * as os from 'node:os'; import * as path from 'node:path'; import { fileURLToPath } from 'node:url'; import { DaemonClient, type SubscribeOptions } from '@qwen-code/sdk'; +import { + hashMcpServerConfig, + type MCPServerConfig, +} from '@qwen-code/qwen-code-core'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); @@ -234,6 +238,41 @@ export function writeWorkspaceSettings( return settingsPath; } +/** + * Pre-approve gated (workspace / project scope, #4615) MCP servers for + * `workspaceCwd` so the daemon's `qwen --acp` child connects them instead of + * skipping them as pending-approval. Servers declared in `.qwen/settings.json` + * are workspace-scoped and therefore gated: absent a stored approval, discovery + * skips them BEFORE any spawn, which makes the MCP-amplification suite time out + * waiting for grandchildren that never appear. + * + * Writes a standalone approvals file (NOT the developer's global + * `~/.qwen/mcpApprovals.json`) under the workspace and returns the env that + * points the daemon — and, by inheritance, its acp child — at it. Pass the + * returned env to `spawnDaemon({ env })`. The approval hash binds to the same + * behavioral fields the child hashes (`scope` is provenance-only and excluded), + * so the plain settings config is sufficient. Mirrors the pre-approval pattern + * in `simple-mcp-server.test.ts`. + */ +export function approveWorkspaceMcpServers( + workspaceCwd: string, + servers: Record, +): Record { + const approvalsPath = path.join(workspaceCwd, '.qwen', 'mcpApprovals.json'); + const project: Record = {}; + for (const [name, config] of Object.entries(servers)) { + project[name] = { hash: hashMcpServerConfig(config), status: 'approved' }; + } + // Key by the canonical (realpath) workspace, NOT `path.resolve`: the daemon + // canonicalizes `--workspace` (e.g. macOS `/var` → `/private/var`) and the + // acp child looks approvals up under that resolved path. Keying by the + // un-resolved temp path would miss, leaving the servers pending. + const root = fs.realpathSync(workspaceCwd); + fs.mkdirSync(path.dirname(approvalsPath), { recursive: true }); + fs.writeFileSync(approvalsPath, JSON.stringify({ [root]: project }, null, 2)); + return { QWEN_CODE_MCP_APPROVALS_PATH: approvalsPath }; +} + /** * One-shot RSS read via `ps -o rss= -p `. Returns megabytes (rounded * to 1 decimal). Returns NaN if the process is gone or `ps` errored — call diff --git a/integration-tests/cli/qwen-serve-baseline.test.ts b/integration-tests/cli/qwen-serve-baseline.test.ts index 53eeedb925..628187f508 100644 --- a/integration-tests/cli/qwen-serve-baseline.test.ts +++ b/integration-tests/cli/qwen-serve-baseline.test.ts @@ -46,6 +46,7 @@ import { countDescendants, percentiles, writeWorkspaceSettings, + approveWorkspaceMcpServers, gitHead, makeTempWorkspace, sleep, @@ -386,13 +387,15 @@ async function measureRssAtSessionCount(sessionCount: number): Promise<{ const ws = makeTempWorkspace('mcp'); let daemon: SpawnedDaemon | undefined; try { - writeWorkspaceSettings(ws, { - mcpServers: { - idle1: { command: 'node', args: [IDLE_MCP_PATH] }, - idle2: { command: 'node', args: [IDLE_MCP_PATH] }, - }, - }); - daemon = await spawnDaemon({ workspaceCwd: ws }); + const mcpServers = { + idle1: { command: 'node', args: [IDLE_MCP_PATH] }, + idle2: { command: 'node', args: [IDLE_MCP_PATH] }, + }; + writeWorkspaceSettings(ws, { mcpServers }); + // Workspace-scoped servers are gated (#4615); pre-approve so the + // daemon's acp child connects them instead of skipping as pending. + const env = approveWorkspaceMcpServers(ws, mcpServers); + daemon = await spawnDaemon({ workspaceCwd: ws, env }); await daemon.client.createOrAttachSession({ workspaceCwd: ws }); const at1 = await waitForMcpGrandchildren( @@ -475,13 +478,15 @@ async function measureRssAtSessionCount(sessionCount: number): Promise<{ const ws = makeTempWorkspace('mcp-counter'); let daemon: SpawnedDaemon | undefined; try { - writeWorkspaceSettings(ws, { - mcpServers: { - idle1: { command: 'node', args: [IDLE_MCP_PATH] }, - idle2: { command: 'node', args: [IDLE_MCP_PATH] }, - }, - }); - daemon = await spawnDaemon({ workspaceCwd: ws }); + const mcpServers = { + idle1: { command: 'node', args: [IDLE_MCP_PATH] }, + idle2: { command: 'node', args: [IDLE_MCP_PATH] }, + }; + writeWorkspaceSettings(ws, { mcpServers }); + // Workspace-scoped servers are gated (#4615); pre-approve so the + // daemon's acp child connects them instead of skipping as pending. + const env = approveWorkspaceMcpServers(ws, mcpServers); + daemon = await spawnDaemon({ workspaceCwd: ws, env }); await daemon.client.createOrAttachSession({ workspaceCwd: ws }); // Wait until the OS sees the full pooled set diff --git a/package-lock.json b/package-lock.json index dbf2aeb6b4..5252c71b65 100644 --- a/package-lock.json +++ b/package-lock.json @@ -23255,6 +23255,7 @@ "diff": "^7.0.0", "dotenv": "^17.1.0", "express": "^5.2.1", + "fast-deep-equal": "^3.1.3", "fzf": "^0.5.2", "glob": "^10.5.0", "highlight.js": "^11.11.1", diff --git a/packages/cli/package.json b/packages/cli/package.json index 8b547e9b5b..904da5d859 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -62,6 +62,7 @@ "diff": "^7.0.0", "dotenv": "^17.1.0", "express": "^5.2.1", + "fast-deep-equal": "^3.1.3", "fzf": "^0.5.2", "glob": "^10.5.0", "highlight.js": "^11.11.1", diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 3228e3d7ff..d605faca7f 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -1885,10 +1885,16 @@ export async function loadCliConfig( toolCallCommand: bareMode ? undefined : settings.tools?.callCommand, mcpServerCommand: bareMode ? undefined : settings.mcp?.serverCommand, mcpServers, + topTierMcpServers, pendingMcpServers, allowedMcpServers: allowedMcpServers ? Array.from(allowedMcpServers) : undefined, + // The flag ONLY (not the settings-derived list) — the hot-reload upper + // bound. Undefined when `--allowed-mcp-server-names` was not passed. + cliAllowedMcpServerNames: argv.allowedMcpServerNames + ? argv.allowedMcpServerNames.filter(Boolean) + : undefined, excludedMcpServers: excludedMcpServers ? Array.from(excludedMcpServers) : undefined, diff --git a/packages/cli/src/config/hot-reload.test.ts b/packages/cli/src/config/hot-reload.test.ts new file mode 100644 index 0000000000..bc388bc813 --- /dev/null +++ b/packages/cli/src/config/hot-reload.test.ts @@ -0,0 +1,465 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { + vi, + describe, + it, + expect, + beforeEach, + afterEach, + type Mock, +} from 'vitest'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import * as fs from 'node:fs'; +import type { Config, MCPServerConfig } from '@qwen-code/qwen-code-core'; +import type { LoadedSettings, Settings } from './settings.js'; +import type { + SettingsWatcher, + SettingsChangeListener, +} from './settingsWatcher.js'; +import { + registerMcpHotReload, + mcpServersEqual, + mcpGatingEqual, +} from './hot-reload.js'; +import { + loadMcpApprovals, + resetMcpApprovalsForTesting, +} from './mcpApprovals.js'; +import { appEvents, AppEvent } from '../utils/events.js'; + +// ── Pure helpers ────────────────────────────────────────────────────── + +describe('mcpServersEqual', () => { + it('treats key-order differences as equal', () => { + const a = { x: { command: 'a' }, y: { command: 'b' } }; + const b = { y: { command: 'b' }, x: { command: 'a' } }; + expect(mcpServersEqual(a, b)).toBe(true); + }); + + it('treats undefined and {} as equal', () => { + expect(mcpServersEqual(undefined, {})).toBe(true); + }); + + it('detects a nested config field change', () => { + expect( + mcpServersEqual({ x: { command: 'a' } }, { x: { command: 'b' } }), + ).toBe(false); + }); + + it('detects adding / removing a server', () => { + expect(mcpServersEqual({ x: { command: 'a' } }, {})).toBe(false); + }); + + it('treats args array reorder as NOT equal (arg order is semantic)', () => { + expect( + mcpServersEqual( + { x: { command: 'c', args: ['--a', '--b'] } }, + { x: { command: 'c', args: ['--b', '--a'] } }, + ), + ).toBe(false); + }); +}); + +describe('mcpGatingEqual', () => { + it('is order-insensitive across the three lists', () => { + expect( + mcpGatingEqual({ allowed: ['a', 'b'] }, { allowed: ['b', 'a'] }), + ).toBe(true); + }); + + it('treats undefined and [] as equal', () => { + expect(mcpGatingEqual({ excluded: undefined }, { excluded: [] })).toBe( + true, + ); + }); + + it('detects a member added to any list', () => { + expect(mcpGatingEqual({ pending: ['a'] }, { pending: ['a', 'b'] })).toBe( + false, + ); + expect(mcpGatingEqual({ excluded: [] }, { excluded: ['a'] })).toBe(false); + }); + + it('treats allowed absent (allow-all) and [] (deny-all) as DIFFERENT', () => { + // For `allowed`, undefined ≠ [] — otherwise editing mcp.allowed to [] would + // look like a no-op and the deny-all would never reconcile. + expect(mcpGatingEqual({ allowed: undefined }, { allowed: [] })).toBe(false); + expect(mcpGatingEqual({ allowed: [] }, { allowed: [] })).toBe(true); + expect(mcpGatingEqual({ allowed: ['a'] }, { allowed: ['a'] })).toBe(true); + // excluded keeps undefined ≡ [] (both mean "exclude nothing"). + expect(mcpGatingEqual({ excluded: undefined }, { excluded: [] })).toBe( + true, + ); + }); +}); + +// ── Subscriber gate branches ────────────────────────────────────────── + +interface FakeConfigState { + settingsMcp: Record | undefined; + gating: { excluded?: string[]; allowed?: string[]; pending?: string[] }; + /** Startup `--allowed-mcp-server-names` upper bound (K); default undefined. */ + bootAllowed?: string[]; +} + +function makeFakeConfig(cwd: string, state: FakeConfigState) { + const reinitializeMcpServers = vi.fn(async () => {}); + const setExcludedMcpServers = vi.fn((v: string[]) => { + state.gating.excluded = v; + }); + const setAllowedMcpServers = vi.fn((v: string[] | undefined) => { + state.gating.allowed = v; + }); + const setPendingMcpServers = vi.fn((v: string[] | undefined) => { + state.gating.pending = v; + }); + const config = { + getTargetDir: () => cwd, + getSettingsMcpServers: () => state.settingsMcp, + // Stand-in for the effective (settings + extensions + runtime) map; the + // hot-reload listener snapshots its keys before narrowing the admission + // lists and passes them to reinitializeMcpServers. + getMcpServers: () => state.settingsMcp, + getMcpGating: () => state.gating, + // Default: no startup --allowed-mcp-server-names flag (settings fully win). + // Individual tests override via state.bootAllowed. + getCliAllowedMcpServerNames: () => state.bootAllowed, + setExcludedMcpServers, + setAllowedMcpServers, + setPendingMcpServers, + reinitializeMcpServers, + } as unknown as Config; + return { + config, + reinitializeMcpServers, + setExcludedMcpServers, + setAllowedMcpServers, + setPendingMcpServers, + }; +} + +describe('registerMcpHotReload', () => { + let cwd: string; + let listener: SettingsChangeListener; + let watcher: SettingsWatcher; + let unsubscribe: Mock; + let settings: LoadedSettings; + let merged: Settings; + + beforeEach(() => { + cwd = fs.mkdtempSync(path.join(os.tmpdir(), 'hotreload-')); + // No .mcp.json in cwd → assembleMcpServers yields only settings + topTier. + process.env['QWEN_CODE_MCP_APPROVALS_PATH'] = path.join( + cwd, + 'mcpApprovals.json', + ); + resetMcpApprovalsForTesting(); + + unsubscribe = vi.fn(); + watcher = { + addChangeListener: vi.fn((l: SettingsChangeListener) => { + listener = l; + return unsubscribe; + }), + } as unknown as SettingsWatcher; + + merged = { mcpServers: {}, mcp: {} } as Settings; + settings = { merged } as LoadedSettings; + }); + + afterEach(() => { + delete process.env['QWEN_CODE_MCP_APPROVALS_PATH']; + resetMcpApprovalsForTesting(); + fs.rmSync(cwd, { recursive: true, force: true }); + }); + + it('returns the watcher unsubscribe fn', () => { + const fc = makeFakeConfig(cwd, { settingsMcp: {}, gating: {} }); + const dispose = registerMcpHotReload( + watcher, + settings, + fc.config, + undefined, + ); + expect(watcher.addChangeListener).toHaveBeenCalledOnce(); + dispose(); + expect(unsubscribe).toHaveBeenCalledOnce(); + }); + + it('reconciles with the assembled map (incl. top-tier) on an mcpServers change', async () => { + const fc = makeFakeConfig(cwd, { settingsMcp: {}, gating: {} }); + const topTier = { cliSrv: { command: 'cli' } }; + registerMcpHotReload(watcher, settings, fc.config, topTier); + + merged.mcpServers = { a: { command: 'a' } }; + await listener([]); + + expect(fc.reinitializeMcpServers).toHaveBeenCalledOnce(); + expect(fc.reinitializeMcpServers).toHaveBeenCalledWith({ + a: { command: 'a' }, + cliSrv: { command: 'cli' }, + }); + }); + + it('reconciles on an admission-list-only change (mcp.excluded), servers unchanged', async () => { + const fc = makeFakeConfig(cwd, { + settingsMcp: { a: { command: 'a' } }, + gating: {}, + }); + registerMcpHotReload(watcher, settings, fc.config, undefined); + + // Same servers, but a newly-excluded one. + merged.mcpServers = { a: { command: 'a' } }; + merged.mcp = { excluded: ['a'] }; + await listener([]); + + expect(fc.setExcludedMcpServers).toHaveBeenCalledWith(['a']); + expect(fc.reinitializeMcpServers).toHaveBeenCalledOnce(); + // Admission lists are pushed BEFORE reconcile. + expect(fc.setExcludedMcpServers.mock.invocationCallOrder[0]).toBeLessThan( + fc.reinitializeMcpServers.mock.invocationCallOrder[0], + ); + }); + + // ── H: mcp.allowed [] semantics ────────────────────────────────────── + it('H: an explicit mcp.allowed [] is preserved as deny-all (not collapsed to undefined)', async () => { + const fc = makeFakeConfig(cwd, { + settingsMcp: { a: { command: 'a' } }, + gating: {}, // allow-all before + }); + registerMcpHotReload(watcher, settings, fc.config, undefined); + + merged.mcpServers = { a: { command: 'a' } }; + merged.mcp = { allowed: [] }; // deny all + await listener([]); + + // Reconcile fires (absent → [] is a real change) and [] is pushed through. + expect(fc.reinitializeMcpServers).toHaveBeenCalledOnce(); + expect(fc.setAllowedMcpServers).toHaveBeenCalledWith([]); + }); + + // ── K: startup --allowed-mcp-server-names as an upper bound ─────────── + it('K: with the startup flag and no settings allow-list, applies the flag in full', async () => { + const fc = makeFakeConfig(cwd, { + settingsMcp: { a: { command: 'a' } }, + gating: {}, + bootAllowed: ['a', 'b'], + }); + registerMcpHotReload(watcher, settings, fc.config, undefined); + + merged.mcpServers = { a: { command: 'a' } }; + merged.mcp = {}; // no settings allow-list + await listener([]); + + expect(fc.setAllowedMcpServers).toHaveBeenCalledWith(['a', 'b']); + }); + + it('K: a settings allow-list is capped to the startup flag (cannot widen beyond it)', async () => { + const fc = makeFakeConfig(cwd, { + settingsMcp: { a: { command: 'a' } }, + gating: {}, + bootAllowed: ['a', 'b'], + }); + registerMcpHotReload(watcher, settings, fc.config, undefined); + + merged.mcpServers = { + a: { command: 'a' }, + b: { command: 'b' }, + c: { command: 'c' }, + }; + merged.mcp = { allowed: ['a', 'b', 'c'] }; // tries to widen to c + await listener([]); + + // `c` is outside the launch bound → dropped. + expect(fc.setAllowedMcpServers).toHaveBeenCalledWith(['a', 'b']); + }); + + it('K: a settings allow-list may narrow within the startup flag', async () => { + const fc = makeFakeConfig(cwd, { + settingsMcp: { a: { command: 'a' } }, + gating: {}, + bootAllowed: ['a', 'b'], + }); + registerMcpHotReload(watcher, settings, fc.config, undefined); + + merged.mcpServers = { a: { command: 'a' }, b: { command: 'b' } }; + merged.mcp = { allowed: ['a'] }; + await listener([]); + + expect(fc.setAllowedMcpServers).toHaveBeenCalledWith(['a']); + }); + + it('K: without the startup flag, the settings allow-list wins unbounded', async () => { + const fc = makeFakeConfig(cwd, { + settingsMcp: { a: { command: 'a' } }, + gating: {}, + // no bootAllowed + }); + registerMcpHotReload(watcher, settings, fc.config, undefined); + + merged.mcpServers = { a: { command: 'a' }, x: { command: 'x' } }; + merged.mcp = { allowed: ['x'] }; + await listener([]); + + expect(fc.setAllowedMcpServers).toHaveBeenCalledWith(['x']); + }); + + it('does NOT reconcile when neither servers nor admission lists changed', async () => { + const fc = makeFakeConfig(cwd, { + settingsMcp: { a: { command: 'a' } }, + gating: {}, + }); + registerMcpHotReload(watcher, settings, fc.config, undefined); + + merged.mcpServers = { a: { command: 'a' } }; + merged.mcp = {}; + await listener([]); + + expect(fc.reinitializeMcpServers).not.toHaveBeenCalled(); + expect(fc.setExcludedMcpServers).not.toHaveBeenCalled(); + }); + + it('recomputes admission lists from current settings, not the startup CLI allowlist', async () => { + // Pre-image gating mimics a session started with --allowed-mcp-server-names=a. + const fc = makeFakeConfig(cwd, { + settingsMcp: { a: { command: 'a' } }, + gating: { allowed: ['a'] }, + }); + registerMcpHotReload(watcher, settings, fc.config, undefined); + + // Runtime settings widen the allow-list to include b. + merged.mcpServers = { a: { command: 'a' }, b: { command: 'b' } }; + merged.mcp = { allowed: ['a', 'b'] }; + await listener([]); + + // Settings win: b is now allowed (not pinned to the boot allowlist). + expect(fc.setAllowedMcpServers).toHaveBeenCalledWith(['a', 'b']); + expect(fc.reinitializeMcpServers).toHaveBeenCalledOnce(); + }); + + it('emits McpPendingApprovalChanged when a gated server becomes newly pending', async () => { + const fc = makeFakeConfig(cwd, { settingsMcp: {}, gating: {} }); + registerMcpHotReload(watcher, settings, fc.config, undefined); + + const spy = vi.fn(); + appEvents.on(AppEvent.McpPendingApprovalChanged, spy); + try { + // A workspace-scoped (gated) server with no stored approval → pending. + merged.mcpServers = { ws: { command: 'ws', scope: 'workspace' } }; + await listener([]); + + expect(fc.setPendingMcpServers).toHaveBeenCalledWith(['ws']); + expect(spy).toHaveBeenCalledOnce(); + } finally { + appEvents.off(AppEvent.McpPendingApprovalChanged, spy); + } + }); + + it('does NOT emit McpPendingApprovalChanged for a non-gated server change', async () => { + const fc = makeFakeConfig(cwd, { settingsMcp: {}, gating: {} }); + registerMcpHotReload(watcher, settings, fc.config, undefined); + + const spy = vi.fn(); + appEvents.on(AppEvent.McpPendingApprovalChanged, spy); + try { + // User-scoped (scope unset) server is never gated → never pending. + merged.mcpServers = { a: { command: 'a' } }; + await listener([]); + + expect(fc.reinitializeMcpServers).toHaveBeenCalledOnce(); + expect(spy).not.toHaveBeenCalled(); + } finally { + appEvents.off(AppEvent.McpPendingApprovalChanged, spy); + } + }); + + it('surfaces a user-visible LogError when reconcile throws', async () => { + const fc = makeFakeConfig(cwd, { settingsMcp: {}, gating: {} }); + fc.reinitializeMcpServers.mockRejectedValueOnce( + new Error('reconcile boom'), + ); + registerMcpHotReload(watcher, settings, fc.config, undefined); + + const spy = vi.fn(); + appEvents.on(AppEvent.LogError, spy); + try { + merged.mcpServers = { a: { command: 'a' } }; + // The listener swallows the reconcile error (one bad reload must not crash + // the watcher) but must NOT do so silently. + await listener([]); + + expect(fc.reinitializeMcpServers).toHaveBeenCalledOnce(); + expect(spy).toHaveBeenCalledOnce(); + // Concise, user-facing message — not a raw stack. + expect(String(spy.mock.calls[0][0])).toMatch( + /Failed to reload MCP server settings/, + ); + } finally { + appEvents.off(AppEvent.LogError, spy); + } + }); + + // Regression for review issue #6: a previously *rejected* gated server is + // still listed in `pending` (rejected ⇒ `!== 'approved'`), so a name-diff of + // the pending set would treat a subsequent config edit as "not newly pending" + // and fail to re-prompt. The strict-`pending` promptable check must re-emit. + it('re-emits when an edit invalidates a previously rejected gated server', async () => { + // Prior reconcile listed ws in pending (because it was rejected). + const fc = makeFakeConfig(cwd, { + settingsMcp: { ws: { command: 'ws', scope: 'workspace' } }, + gating: { pending: ['ws'] }, + }); + // The rejection is bound to ws's OLD config hash. + await loadMcpApprovals().setState( + cwd, + 'ws', + { command: 'ws', scope: 'workspace' }, + 'rejected', + ); + registerMcpHotReload(watcher, settings, fc.config, undefined); + + const spy = vi.fn(); + appEvents.on(AppEvent.McpPendingApprovalChanged, spy); + try { + // Edit changes the config → hash no longer matches the rejection → + // strictly `pending` again → must re-prompt. + merged.mcpServers = { ws: { command: 'ws-v2', scope: 'workspace' } }; + await listener([]); + + expect(spy).toHaveBeenCalledOnce(); + } finally { + appEvents.off(AppEvent.McpPendingApprovalChanged, spy); + } + }); + + it('does NOT re-emit for an unrelated edit while a server stays rejected', async () => { + const ws: MCPServerConfig = { command: 'ws', scope: 'workspace' }; + const fc = makeFakeConfig(cwd, { + settingsMcp: { ws }, + gating: { pending: ['ws'] }, + }); + // ws rejected at its CURRENT config hash → stays rejected, not promptable. + await loadMcpApprovals().setState(cwd, 'ws', ws, 'rejected'); + registerMcpHotReload(watcher, settings, fc.config, undefined); + + const spy = vi.fn(); + appEvents.on(AppEvent.McpPendingApprovalChanged, spy); + try { + // Unrelated admission-list change; ws config itself is unchanged. + merged.mcpServers = { ws }; + merged.mcp = { excluded: ['other'] }; + await listener([]); + + expect(fc.reinitializeMcpServers).toHaveBeenCalledOnce(); + expect(spy).not.toHaveBeenCalled(); + } finally { + appEvents.off(AppEvent.McpPendingApprovalChanged, spy); + } + }); +}); diff --git a/packages/cli/src/config/hot-reload.ts b/packages/cli/src/config/hot-reload.ts new file mode 100644 index 0000000000..8b17aec340 --- /dev/null +++ b/packages/cli/src/config/hot-reload.ts @@ -0,0 +1,241 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import equal from 'fast-deep-equal'; +import { + createDebugLogger, + type Config, + getMCPServerStatus, + type MCPServerConfig, +} from '@qwen-code/qwen-code-core'; +import type { LoadedSettings } from './settings.js'; +import type { SettingsWatcher } from './settingsWatcher.js'; +import { assembleMcpServers } from './mcpServers.js'; +import { + getPendingGatedMcpServers, + getPromptableMcpServers, +} from './mcpApprovals.js'; +import { appEvents, AppEvent } from '../utils/events.js'; + +const debugLogger = createDebugLogger('MCP_HOT_RELOAD'); + +/** + * The three connection-admission lists discovery consults to decide whether a + * given MCP server may connect. Distinct from the `mcpServers` config map: + * these govern *whether* to connect, the map governs *which servers and how*. + */ +export interface McpGating { + excluded?: string[]; + allowed?: string[]; + pending?: string[]; +} + +/** + * Whether two `mcpServers` maps are equivalent. `fast-deep-equal` is + * insensitive to object key order (so reordering servers / fields in + * settings.json is not a false positive) but sensitive to array order (so + * `args` order — which is semantically meaningful — is). `undefined` ≡ `{}`. + */ +export function mcpServersEqual( + a: Record | undefined, + b: Record | undefined, +): boolean { + return equal(a ?? {}, b ?? {}); +} + +/** + * Whether two admission-list snapshots are equivalent. `excluded` / `pending` + * are sets (order-irrelevant) where `undefined` ≡ `[]` (both mean "no entries"). + * `allowed` is different: an absent allow-list (`undefined`) means "allow all", + * but an explicit empty allow-list (`[]`) means "deny all" — so for `allowed`, + * absent and empty are NOT equal (otherwise editing `mcp.allowed` to `[]` would + * be treated as a no-op and the deny-all never reconciles). `fast-deep-equal` + * is array-order-sensitive, so sort copies before comparing. + */ +export function mcpGatingEqual(a: McpGating, b: McpGating): boolean { + const norm = (xs: string[] | undefined) => [...(xs ?? [])].sort(); + const allowedEqual = + (a.allowed === undefined) === (b.allowed === undefined) && + equal(norm(a.allowed), norm(b.allowed)); + return ( + equal(norm(a.excluded), norm(b.excluded)) && + allowedEqual && + equal(norm(a.pending), norm(b.pending)) + ); +} + +/** + * Recompute the connection-admission lists from the *current* settings. Runtime + * edits to `mcp.allowed` / `mcp.excluded` take effect immediately, with two + * deliberate rules: + * + * - **`allowed` empty vs absent**: an absent `mcp.allowed` means "allow all" + * (`undefined`); an explicit `mcp.allowed: []` means "deny all" (`[]` is + * preserved, NOT collapsed to `undefined`), matching the boot-time semantics + * of `getMcpServers()` (an empty allow-list filters everything out). + * - **CLI allow-list is an upper bound (K)**: if launched with + * `--allowed-mcp-server-names`, `bootAllowed` is that flag value and the + * settings-derived allow-list is intersected with it — a settings edit may + * narrow within the launch bound but never widen beyond it. With no settings + * allow-list, the boot bound applies in full. Without the flag (`bootAllowed` + * undefined), settings fully drive admission. + * + * The pending list is always recomputed per #4615 so a hot-reload never + * connects an unapproved gated server. + */ +function recomputeMcpGating( + settings: LoadedSettings, + assembled: Record, + cwd: string, + bootAllowed: readonly string[] | undefined, +): McpGating { + // Preserve `[]` (deny-all); only an absent key yields `undefined` (allow-all). + const settingsAllowed = settings.merged.mcp?.allowed?.filter(Boolean); + const excluded = settings.merged.mcp?.excluded?.filter(Boolean); + let allowed = settingsAllowed; + if (bootAllowed) { + allowed = settingsAllowed + ? settingsAllowed.filter((n) => bootAllowed.includes(n)) + : [...bootAllowed]; + } + return { + allowed, + excluded: excluded && excluded.length > 0 ? excluded : undefined, + pending: getPendingGatedMcpServers(assembled, cwd), + }; +} + +/** + * Subscribe the running {@link Config} to settings changes so MCP servers + * reconnect / disconnect / restart without a session restart (issue #3696 + * sub-task 3). Called once at startup, after `settingsWatcher.startWatching()`; + * returns a disposer that unsubscribes. + * + * On each settings change the callback rebuilds the assembled MCP map the same + * way Config boot did (so top-tier CLI/session servers and `.mcp.json` gating + * stay correct), recomputes the admission lists, and only reconciles when the + * servers or the admission lists actually changed — unrelated edits (theme, + * skills, …) are ignored. The watcher already debounces (300ms) and serializes + * its listeners; re-entrancy during an in-flight reconcile is coalesced inside + * `Config.reinitializeMcpServers`. + */ +export function registerMcpHotReload( + watcher: SettingsWatcher, + settings: LoadedSettings, + config: Config, + topTierMcpServers: Record | undefined, +): () => void { + debugLogger.debug('registered MCP hot-reload listener on SettingsWatcher'); + return watcher.addChangeListener(async (events) => { + debugLogger.debug( + `settings change fired (${events.length} event(s)): ${events + .map((e) => `${e.scope}:${e.changeType}`) + .join(', ')}`, + ); + const cwd = config.getTargetDir(); + // Rebuild exactly the way Config boot did — including top-tier + // (CLI / session-injected) servers layered above settings + `.mcp.json`. + const next = assembleMcpServers( + settings.merged.mcpServers, + cwd, + topTierMcpServers, + ); + const nextGating = recomputeMcpGating( + settings, + next, + cwd, + config.getCliAllowedMcpServerNames(), + ); + + const prevServers = config.getSettingsMcpServers(); + const prevGating = config.getMcpGating(); + debugLogger.debug( + `assembled servers: prev=[${Object.keys(prevServers ?? {}).join( + ', ', + )}] next=[${Object.keys(next).join(', ')}]`, + ); + debugLogger.debug( + `gating next: excluded=[${(nextGating.excluded ?? []).join( + ', ', + )}] allowed=[${(nextGating.allowed ?? []).join( + ', ', + )}] pending=[${(nextGating.pending ?? []).join(', ')}]`, + ); + + // Gate: reconcile only if the servers OR the admission lists changed. + // Both unchanged ⇒ this was an MCP-irrelevant edit; bail. + const serversChanged = !mcpServersEqual(prevServers, next); + const gatingChanged = !mcpGatingEqual(prevGating, nextGating); + // Surface the admission lists — a gated server whose config hash changed + // lands in `pending` and is skipped by discovery (left disconnected), which + // is otherwise invisible from the server-name diff above. See #4615. + if (!serversChanged && !gatingChanged) { + debugLogger.debug( + 'no MCP-relevant change (servers + gating unchanged) — skipping reconcile', + ); + return; + } + debugLogger.debug( + `MCP-relevant change detected (serversChanged=${serversChanged} gatingChanged=${gatingChanged}) — reconciling`, + ); + + // Gated servers awaiting a (re-)decision after this edit — strictly + // `pending`, NOT the rejected-inclusive `nextGating.pending`. The startup + // approval dialog (`useMcpApproval`) only computes its queue on mount, so + // without this signal a mid-session pend would be silently skipped by + // discovery with no prompt. We deliberately do NOT diff against the prior + // pending set by name: a server already listed as `pending` because it was + // *rejected* must still re-prompt once an edit invalidates that rejection's + // hash (issue #6 in the hot-reload review). The dialog's `computePending` + // is the authoritative filter; this is only the "re-evaluate" nudge. See + // #4615. + const promptable = getPromptableMcpServers(next, cwd); + + // Push the admission lists BEFORE reconcile — the discovery pass inside + // reinitializeMcpServers reads them to skip excluded / non-allowed / + // pending servers. + config.setExcludedMcpServers(nextGating.excluded ?? []); + config.setAllowedMcpServers(nextGating.allowed); + config.setPendingMcpServers(nextGating.pending); + + try { + await config.reinitializeMcpServers(next); + const finalStatuses = Object.keys(next) + .map((name) => `${name}=${getMCPServerStatus(name)}`) + .join(', '); + debugLogger.debug( + `reinitializeMcpServers resolved; final statuses=[${finalStatuses}]`, + ); + } catch (err) { + // Keep the full stack on the debug channel for diagnosis… + debugLogger.error( + `reinitializeMcpServers threw: ${ + err instanceof Error ? (err.stack ?? err.message) : String(err) + }`, + ); + // …but also surface a concise, user-visible notice. `debugLogger.error` + // only shows under `--debug`, so a failed settings edit would otherwise + // silently do nothing with no indication anything went wrong. `LogError` + // is the same channel the CLI already renders to the user. + appEvents.emit( + AppEvent.LogError, + 'Failed to reload MCP server settings; existing MCP state may be unchanged. Run with --debug for details.', + ); + } + + // Prompt for approval AFTER reconcile, so `config.getMcpServers()` (which + // the dialog reads) already reflects the new map. Emit regardless of + // reconcile success — a server left pending still needs the user's decision. + if (promptable.length > 0) { + debugLogger.debug( + `gated servers awaiting approval → emitting ${AppEvent.McpPendingApprovalChanged}: [${promptable.join( + ', ', + )}]`, + ); + appEvents.emit(AppEvent.McpPendingApprovalChanged); + } + }); +} diff --git a/packages/cli/src/config/mcpApprovals.test.ts b/packages/cli/src/config/mcpApprovals.test.ts index f6dd64b53b..cb9246a12b 100644 --- a/packages/cli/src/config/mcpApprovals.test.ts +++ b/packages/cli/src/config/mcpApprovals.test.ts @@ -12,6 +12,7 @@ import type { MCPServerConfig } from '@qwen-code/qwen-code-core'; import { loadMcpApprovals, getPendingGatedMcpServers, + getPromptableMcpServers, resetMcpApprovalsForTesting, MCP_APPROVALS_FILENAME, } from './mcpApprovals.js'; @@ -231,4 +232,66 @@ describe('mcpApprovals (hash-bound approval store)', () => { expect(pending).toEqual(['ws']); }); }); + + describe('getPromptableMcpServers (strict-pending, drives the dialog)', () => { + const workspaceServer: MCPServerConfig = { + command: 'node', + args: ['ws.js'], + scope: 'workspace', + }; + const userServer: MCPServerConfig = { command: 'node', args: ['user.js'] }; + + it('prompts gated servers with no stored decision, ignores user scope', () => { + const promptable = getPromptableMcpServers( + { proj: server, ws: workspaceServer, usr: userServer }, + projectRoot, + ); + expect(promptable.sort()).toEqual(['proj', 'ws']); + }); + + it('does NOT prompt a rejected server (unlike getPendingGatedMcpServers)', async () => { + await loadMcpApprovals().setState( + projectRoot, + 'ws', + workspaceServer, + 'rejected', + ); + // Same config hash as the rejection ⇒ stays rejected ⇒ not promptable, + // yet still in the gating skip set. + expect( + getPromptableMcpServers({ ws: workspaceServer }, projectRoot), + ).toEqual([]); + expect( + getPendingGatedMcpServers({ ws: workspaceServer }, projectRoot), + ).toEqual(['ws']); + }); + + it('re-prompts a rejected server once an edit changes its config hash', async () => { + await loadMcpApprovals().setState( + projectRoot, + 'ws', + workspaceServer, + 'rejected', + ); + const edited: MCPServerConfig = { + ...workspaceServer, + args: ['ws-v2.js'], + }; + expect(getPromptableMcpServers({ ws: edited }, projectRoot)).toEqual([ + 'ws', + ]); + }); + + it('does NOT prompt an approved server', async () => { + await loadMcpApprovals().setState( + projectRoot, + 'ws', + workspaceServer, + 'approved', + ); + expect( + getPromptableMcpServers({ ws: workspaceServer }, projectRoot), + ).toEqual([]); + }); + }); }); diff --git a/packages/cli/src/config/mcpApprovals.ts b/packages/cli/src/config/mcpApprovals.ts index 283f6a8e81..04e3aef021 100644 --- a/packages/cli/src/config/mcpApprovals.ts +++ b/packages/cli/src/config/mcpApprovals.ts @@ -183,6 +183,36 @@ export function getPendingGatedMcpServers( return pending; } +/** + * Names of gated servers in `mcpServers` whose state is strictly `pending` — + * i.e. awaiting a first decision OR a re-decision because a config edit changed + * the hash their prior decision was bound to. This is what the interactive + * approval dialog should prompt for. + * + * Distinct from {@link getPendingGatedMcpServers}, which is `!== 'approved'` and + * so also includes `rejected` servers: discovery must keep skipping those, but + * the dialog must NOT re-prompt them. Using this stricter set to drive the + * prompt is what lets a config edit re-surface a previously *rejected* server + * (its hash no longer matches → `pending`) without nagging about a settled + * rejection. See issue #4615. + */ +export function getPromptableMcpServers( + mcpServers: Record, + projectRoot: string, +): string[] { + const approvals = loadMcpApprovals(); + const promptable: string[] = []; + for (const [name, config] of Object.entries(mcpServers)) { + if (!isGatedMcpScope(config.scope)) { + continue; + } + if (approvals.getState(projectRoot, name, config) === 'pending') { + promptable.push(name); + } + } + return promptable; +} + export async function saveMcpApprovals(file: { path: string; config: McpApprovalsConfig; diff --git a/packages/cli/src/config/settingsSchema.test.ts b/packages/cli/src/config/settingsSchema.test.ts index b062d863cb..04e6dd56d0 100644 --- a/packages/cli/src/config/settingsSchema.test.ts +++ b/packages/cli/src/config/settingsSchema.test.ts @@ -254,6 +254,29 @@ describe('SettingsSchema', () => { expect(categories).toContain('Advanced'); }); + it('marks MCP reconcile inputs as hot-reloadable but startup-only MCP keys as restart-required (sub-task 3)', () => { + // These three feed the runtime reconcile (mcpServers is the server map; + // mcp.allowed / mcp.excluded are read by mcpGatingEqual), so they MUST be + // hot-reloadable — otherwise the SettingsWatcher suppresses MCP-only + // edits and registerMcpHotReload never fires for the advertised + // add/remove/restart/gating changes. + expect(getSettingsSchema().mcpServers.requiresRestart).toBe(false); + expect(getSettingsSchema().mcp.properties!.allowed.requiresRestart).toBe( + false, + ); + expect(getSettingsSchema().mcp.properties!.excluded.requiresRestart).toBe( + false, + ); + // serverCommand is consumed once at startup (not part of the reconcile + // input), so it stays restart-required. The mcp parent node also stays + // restart-required; the watcher resolves the longest-matching schema key, + // so the flipped leaves win for allowed/excluded edits regardless. + expect( + getSettingsSchema().mcp.properties!.serverCommand.requiresRestart, + ).toBe(true); + expect(getSettingsSchema().mcp.requiresRestart).toBe(true); + }); + it('should have consistent default values for boolean settings', () => { const checkBooleanDefaults = (schema: SettingsSchema) => { Object.entries(schema).forEach(([, definition]) => { diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index fc5ce7cc57..e87cbca3b4 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -275,7 +275,11 @@ const SETTINGS_SCHEMA = { type: 'object', label: 'MCP Servers', category: 'Advanced', - requiresRestart: true, + // Hot-reloadable (sub-task 3): a runtime edit is reconciled live by the + // SettingsWatcher → reinitializeMcpServers path. Marking it + // restart-required would make the watcher suppress MCP-only edits, so the + // listener that drives the reconcile would never fire. + requiresRestart: false, default: {} as Record, description: 'Configuration for MCP servers.', showInDialog: false, @@ -2343,7 +2347,10 @@ const SETTINGS_SCHEMA = { type: 'array', label: 'Allow MCP Servers', category: 'MCP', - requiresRestart: true, + // Hot-reloadable (sub-task 3): read by mcpGatingEqual and re-applied + // live during reconcile. See mcpServers above for why this must not be + // restart-required. + requiresRestart: false, default: undefined as string[] | undefined, description: 'A list of MCP servers to allow.', showInDialog: false, @@ -2353,7 +2360,10 @@ const SETTINGS_SCHEMA = { type: 'array', label: 'Exclude MCP Servers', category: 'MCP', - requiresRestart: true, + // Hot-reloadable (sub-task 3): read by mcpGatingEqual and re-applied + // live during reconcile. See mcpServers above for why this must not be + // restart-required. + requiresRestart: false, default: undefined as string[] | undefined, description: 'A list of MCP servers to exclude.', showInDialog: false, diff --git a/packages/cli/src/config/settingsWatcher.test.ts b/packages/cli/src/config/settingsWatcher.test.ts index e9f4dc8faf..07010b5247 100644 --- a/packages/cli/src/config/settingsWatcher.test.ts +++ b/packages/cli/src/config/settingsWatcher.test.ts @@ -552,6 +552,51 @@ describe('SettingsWatcher', () => { expect(listener).toHaveBeenCalledTimes(1); }); + + it('should notify when only mcpServers changes (sub-task 3 hot-reload)', async () => { + // Regression guard for the watcher↔schema integration: mcpServers is now + // requiresRestart:false, so an MCP-server-only edit must reach the + // listener that drives reinitializeMcpServers. Before the schema flip + // this change was suppressed and the runtime reconcile never fired. + watcher.startWatching(); + const listener = vi.fn(); + watcher.addChangeListener(listener); + + const userFile = settings.forScope(SettingScope.User); + userFile.settings = s({ mcpServers: { foo: { command: 'a' } } }); + + vi.mocked(settings.reloadScopeFromDisk).mockImplementation(() => { + userFile.settings = s({ + mcpServers: { foo: { command: 'a' }, bar: { command: 'b' } }, + }); + }); + + fireAllEvent(0, 'change', '/home/user/.qwen/settings.json'); + await vi.advanceTimersByTimeAsync(SettingsWatcher.DEBOUNCE_MS + 10); + + expect(listener).toHaveBeenCalledTimes(1); + }); + + it('should notify when only mcp.excluded changes (sub-task 3 gating hot-reload)', async () => { + // mcp.excluded / mcp.allowed feed mcpGatingEqual; the leaf is now + // requiresRestart:false even though the mcp parent stays true (the + // watcher resolves the longest-matching schema key, so the leaf wins). + watcher.startWatching(); + const listener = vi.fn(); + watcher.addChangeListener(listener); + + const userFile = settings.forScope(SettingScope.User); + userFile.settings = s({ mcp: { excluded: ['a'] } }); + + vi.mocked(settings.reloadScopeFromDisk).mockImplementation(() => { + userFile.settings = s({ mcp: { excluded: ['a', 'b'] } }); + }); + + fireAllEvent(0, 'change', '/home/user/.qwen/settings.json'); + await vi.advanceTimersByTimeAsync(SettingsWatcher.DEBOUNCE_MS + 10); + + expect(listener).toHaveBeenCalledTimes(1); + }); }); describe('change type classification', () => { diff --git a/packages/cli/src/gemini.test.tsx b/packages/cli/src/gemini.test.tsx index 64a12ec383..6e2c206573 100644 --- a/packages/cli/src/gemini.test.tsx +++ b/packages/cli/src/gemini.test.tsx @@ -235,6 +235,7 @@ describe('gemini.tsx main function', () => { getDebugMode: () => false, getListExtensions: () => false, getMcpServers: () => ({}), + getTopTierMcpServers: () => undefined, initialize: vi.fn(), waitForMcpReady: vi.fn().mockResolvedValue(undefined), getIdeMode: () => false, @@ -361,6 +362,7 @@ describe('gemini.tsx main function', () => { getDebugMode: () => false, getListExtensions: () => false, getMcpServers: () => ({}), + getTopTierMcpServers: () => undefined, initialize: vi.fn().mockResolvedValue(undefined), waitForMcpReady: vi.fn().mockResolvedValue(undefined), getIdeMode: () => false, @@ -466,6 +468,7 @@ describe('gemini.tsx main function', () => { getDebugMode: () => false, getListExtensions: () => false, getMcpServers: () => ({}), + getTopTierMcpServers: () => undefined, initialize: vi.fn().mockImplementation(async () => { initialized = true; }), @@ -792,6 +795,7 @@ describe('gemini.tsx main function', () => { getDebugMode: () => false, getListExtensions: () => false, getMcpServers: () => ({}), + getTopTierMcpServers: () => undefined, initialize: vi.fn().mockResolvedValue(undefined), waitForMcpReady: vi.fn().mockResolvedValue(undefined), getIdeMode: () => false, @@ -925,6 +929,7 @@ describe('gemini.tsx main function kitty protocol', () => { getDebugMode: () => false, getListExtensions: () => false, getMcpServers: () => ({}), + getTopTierMcpServers: () => undefined, initialize: vi.fn(), waitForMcpReady: vi.fn().mockResolvedValue(undefined), getIdeMode: () => false, @@ -1035,6 +1040,7 @@ describe('gemini.tsx main function kitty protocol', () => { getDebugMode: () => false, getListExtensions: () => false, getMcpServers: () => ({}), + getTopTierMcpServers: () => undefined, initialize: vi.fn(), waitForMcpReady: vi.fn().mockResolvedValue(undefined), getIdeMode: () => false, @@ -1121,6 +1127,7 @@ describe('gemini.tsx main function kitty protocol', () => { getDebugMode: () => false, getListExtensions: () => false, getMcpServers: () => ({}), + getTopTierMcpServers: () => undefined, initialize: vi.fn(), waitForMcpReady: vi.fn().mockResolvedValue(undefined), getIdeMode: () => false, diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index 32290859ad..4856daa1b3 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -39,6 +39,7 @@ import { preResolveHomeEnvOverrides, } from './config/settings.js'; import { SettingsWatcher } from './config/settingsWatcher.js'; +import { registerMcpHotReload } from './config/hot-reload.js'; import { initializeI18n, resolveLanguageSetting } from './i18n/index.js'; import { setupStartupWorktree, @@ -597,6 +598,19 @@ export async function main() { ); profileCheckpoint('after_load_cli_config'); + // Subscribe the running Config to settings changes so MCP servers + // reconnect / disconnect / restart without a session restart (#3696, + // sub-task 3). Skipped in bare mode (no watcher). + if (settingsWatcher) { + const disposeMcpHotReload = registerMcpHotReload( + settingsWatcher, + settings, + config, + config.getTopTierMcpServers(), + ); + registerCleanup(disposeMcpHotReload); + } + // Phase D-1: persist the WorktreeSession sidecar so Phase C's restore // machinery on a subsequent `--resume` picks the worktree back up, and // capture any override of a previously-resumed session's worktree so diff --git a/packages/cli/src/i18n/locales/ca.js b/packages/cli/src/i18n/locales/ca.js index bc17ab7537..9145e63235 100644 --- a/packages/cli/src/i18n/locales/ca.js +++ b/packages/cli/src/i18n/locales/ca.js @@ -1544,6 +1544,8 @@ export default { 'No tasks currently running': 'No hi ha cap tasca en execució', 'No entry to show.': 'No hi ha cap entrada per mostrar.', 'needs approval': 'necessita aprovació', + 'rejected — edit config to re-approve': + 'rebutjat — editeu la configuració per tornar a aprovar', 'Background agent needs approval': "L'agent en segon pla necessita aprovació", 'Approve or deny the request above': 'Aprova o denega la sol·licitud de dalt', Running: 'En execució', diff --git a/packages/cli/src/i18n/locales/de.js b/packages/cli/src/i18n/locales/de.js index e6e404aec6..6a1db32650 100644 --- a/packages/cli/src/i18n/locales/de.js +++ b/packages/cli/src/i18n/locales/de.js @@ -1477,6 +1477,8 @@ export default { 'No tasks currently running': 'Derzeit laufen keine Aufgaben', 'No entry to show.': 'Kein Eintrag zum Anzeigen.', 'needs approval': 'wartet auf Genehmigung', + 'rejected — edit config to re-approve': + 'abgelehnt — Konfiguration bearbeiten, um erneut zu genehmigen', 'Background agent needs approval': 'Hintergrund-Agent wartet auf Genehmigung', 'Approve or deny the request above': 'Genehmigen oder lehnen Sie die obige Anfrage ab', diff --git a/packages/cli/src/i18n/locales/en.js b/packages/cli/src/i18n/locales/en.js index b2087c2679..4fb2307ae1 100644 --- a/packages/cli/src/i18n/locales/en.js +++ b/packages/cli/src/i18n/locales/en.js @@ -1923,6 +1923,8 @@ export default { 'No tasks currently running': 'No tasks currently running', 'No entry to show.': 'No entry to show.', 'needs approval': 'needs approval', + 'rejected — edit config to re-approve': + 'rejected — edit config to re-approve', 'Background agent needs approval': 'Background agent needs approval', 'Approve or deny the request above': 'Approve or deny the request above', Running: 'Running', diff --git a/packages/cli/src/i18n/locales/fr.js b/packages/cli/src/i18n/locales/fr.js index 094b684647..060f72c1bc 100644 --- a/packages/cli/src/i18n/locales/fr.js +++ b/packages/cli/src/i18n/locales/fr.js @@ -1541,6 +1541,8 @@ export default { 'No tasks currently running': 'Aucune tâche en cours', 'No entry to show.': 'Aucune entrée à afficher.', 'needs approval': 'nécessite une approbation', + 'rejected — edit config to re-approve': + 'rejeté — modifiez la configuration pour réapprouver', 'Background agent needs approval': "L'agent en arrière-plan nécessite une approbation", 'Approve or deny the request above': diff --git a/packages/cli/src/i18n/locales/ja.js b/packages/cli/src/i18n/locales/ja.js index eac6a2535a..29ea75be28 100644 --- a/packages/cli/src/i18n/locales/ja.js +++ b/packages/cli/src/i18n/locales/ja.js @@ -923,6 +923,7 @@ export default { 'No tasks currently running': '現在実行中のタスクはありません', 'No entry to show.': '表示するエントリはありません。', 'needs approval': '承認待ち', + 'rejected — edit config to re-approve': '拒否済み — 設定を編集して再承認', 'Background agent needs approval': 'バックグラウンドエージェントが承認待ちです', 'Approve or deny the request above': diff --git a/packages/cli/src/i18n/locales/pt.js b/packages/cli/src/i18n/locales/pt.js index c6a8c2f78f..1cd0172e2f 100644 --- a/packages/cli/src/i18n/locales/pt.js +++ b/packages/cli/src/i18n/locales/pt.js @@ -1510,6 +1510,8 @@ export default { 'No tasks currently running': 'Nenhuma tarefa em execução', 'No entry to show.': 'Nenhuma entrada para mostrar.', 'needs approval': 'precisa de aprovação', + 'rejected — edit config to re-approve': + 'rejeitado — edite a configuração para reaprovar', 'Background agent needs approval': 'Agente em segundo plano precisa de aprovação', 'Approve or deny the request above': 'Aprove ou negue a solicitação acima', diff --git a/packages/cli/src/i18n/locales/ru.js b/packages/cli/src/i18n/locales/ru.js index 0c44a4f9fb..fa86e101f5 100644 --- a/packages/cli/src/i18n/locales/ru.js +++ b/packages/cli/src/i18n/locales/ru.js @@ -1425,6 +1425,8 @@ export default { 'No tasks currently running': 'Нет запущенных задач', 'No entry to show.': 'Нет записи для отображения.', 'needs approval': 'требует подтверждения', + 'rejected — edit config to re-approve': + 'отклонено — измените конфигурацию для повторного подтверждения', 'Background agent needs approval': 'Фоновый агент требует подтверждения', 'Approve or deny the request above': 'Подтвердите или отклоните запрос выше', Running: 'Выполняется', diff --git a/packages/cli/src/i18n/locales/zh-TW.js b/packages/cli/src/i18n/locales/zh-TW.js index ae460d6c32..de38c8d6be 100644 --- a/packages/cli/src/i18n/locales/zh-TW.js +++ b/packages/cli/src/i18n/locales/zh-TW.js @@ -1966,6 +1966,7 @@ export default { 'No tasks currently running': '目前沒有正在執行的任務', 'No entry to show.': '沒有可顯示的項目。', 'needs approval': '待審批', + 'rejected — edit config to re-approve': '已拒絕 — 編輯設定以重新審批', 'Background agent needs approval': '背景 agent 等待審批', 'Approve or deny the request above': '請核准或拒絕上方的請求', Running: '執行中', diff --git a/packages/cli/src/i18n/locales/zh.js b/packages/cli/src/i18n/locales/zh.js index a6cdab3cb8..67902453b1 100644 --- a/packages/cli/src/i18n/locales/zh.js +++ b/packages/cli/src/i18n/locales/zh.js @@ -2162,6 +2162,7 @@ export default { 'No tasks currently running': '当前没有正在运行的任务', 'No entry to show.': '没有可显示的条目。', 'needs approval': '待审批', + 'rejected — edit config to re-approve': '已拒绝 — 编辑配置以重新审批', 'Background agent needs approval': '后台 agent 等待审批', 'Approve or deny the request above': '请批准或拒绝上方的请求', Running: '运行中', diff --git a/packages/cli/src/ui/components/mcp/MCPManagementDialog.tsx b/packages/cli/src/ui/components/mcp/MCPManagementDialog.tsx index f17f99b02e..3ade0bc9f5 100644 --- a/packages/cli/src/ui/components/mcp/MCPManagementDialog.tsx +++ b/packages/cli/src/ui/components/mcp/MCPManagementDialog.tsx @@ -34,12 +34,14 @@ import { MCPServerStatus, DiscoveredMCPTool, MCPOAuthTokenStorage, + isGatedMcpScope, type MCPServerConfig, type AnyDeclarativeTool, type DiscoveredMCPPrompt, createDebugLogger, } from '@qwen-code/qwen-code-core'; import { loadSettings, SettingScope } from '../../../config/settings.js'; +import { loadMcpApprovals } from '../../../config/mcpApprovals.js'; import { isToolValid, getToolInvalidReasons } from './utils.js'; import { useTerminalSize } from '../../hooks/useTerminalSize.js'; @@ -75,6 +77,12 @@ export const MCPManagementDialog: React.FC = ({ const promptRegistry = config.getPromptRegistry(); const resourceRegistry = config.getResourceRegistry(); + // Approval state is keyed by the same project root the approval dialog + // writes under — `getWorkingDir()` (see useMcpApproval) — so the lookup + // matches what discovery gated on. + const approvals = loadMcpApprovals(); + const approvalRoot = config.getWorkingDir(); + const serverInfos: MCPServerDisplayInfo[] = []; for (const [name, serverConfig] of Object.entries(mcpServers) as Array< @@ -137,6 +145,17 @@ export const MCPManagementDialog: React.FC = ({ (mcpServerRequiresOAuth.get(name) === true || (Boolean(serverConfig.oauth?.enabled) && !hasOAuthTokens)); + // Why a gated (#4615) server is skipped by discovery: `pending` (awaiting + // a first/renewed approval) or `rejected`. Only gated scopes carry this; + // `approved` (and all non-gated scopes) leave it undefined. + let approvalState: 'pending' | 'rejected' | undefined; + if (isGatedMcpScope(serverConfig.scope)) { + const state = approvals.getState(approvalRoot, name, serverConfig); + if (state !== 'approved') { + approvalState = state; + } + } + serverInfos.push({ name, status, @@ -149,6 +168,7 @@ export const MCPManagementDialog: React.FC = ({ isDisabled, hasOAuthTokens, requiresAuth, + approvalState, }); } diff --git a/packages/cli/src/ui/components/mcp/steps/ServerDetailStep.tsx b/packages/cli/src/ui/components/mcp/steps/ServerDetailStep.tsx index 89c461d4ad..d282ac59e4 100644 --- a/packages/cli/src/ui/components/mcp/steps/ServerDetailStep.tsx +++ b/packages/cli/src/ui/components/mcp/steps/ServerDetailStep.tsx @@ -40,15 +40,20 @@ export const ServerDetailStep: React.FC = ({ onBack, isActive = true, }) => { + // 受门控(#4615)但未审批的 server 被 discovery 跳过,不会进入连接/认证流程, + // 审批原因优先展示。 + const awaitingApproval = + !!server && !server.isDisabled && !!server.approvalState; // 未连接且需要认证时,状态以"需要认证"展示,避免误导用户去排查连接问题。 // requiresAuth 是加载时的快照,状态被实时推到 connected 后不再适用。 const needsAuth = !!server && !server.isDisabled && + !awaitingApproval && !!server.requiresAuth && server.status !== MCPServerStatus.CONNECTED; const statusColor = server - ? server.isDisabled || needsAuth + ? server.isDisabled || awaitingApproval || needsAuth ? 'yellow' : getStatusColor(server.status) : 'gray'; @@ -90,8 +95,13 @@ export const ServerDetailStep: React.FC = ({ }); } - // 只在服务器未禁用且已断开连接时显示"重新连接"选项 - if (!server.isDisabled && server.status === 'disconnected') { + // 只在服务器未禁用且已断开连接时显示"重新连接"选项。受门控但未审批的 server + // 被 discovery 跳过,重连无法推进(仍是 pending/rejected),故隐藏以与状态文案一致。 + if ( + !server.isDisabled && + !awaitingApproval && + server.status === 'disconnected' + ) { result.push({ key: 'reconnect', label: t('Reconnect'), @@ -106,8 +116,8 @@ export const ServerDetailStep: React.FC = ({ value: 'toggle-disable', }); - // 已认证的服务器显示"重新认证",未认证的显示"认证" - if (!server.isDisabled) { + // 已认证的服务器显示"重新认证",未认证的显示"认证"。审批未通过时认证同样无法推进,隐藏。 + if (!server.isDisabled && !awaitingApproval) { result.push({ key: 'authenticate', label: server.hasOAuthTokens ? t('Re-authenticate') : t('Authenticate'), @@ -125,7 +135,7 @@ export const ServerDetailStep: React.FC = ({ } return result; - }, [server, onViewResources]); + }, [server, onViewResources, awaitingApproval]); useKeypress( (key) => { @@ -165,9 +175,13 @@ export const ServerDetailStep: React.FC = ({ {getStatusIcon(server.status)}{' '} {server.isDisabled ? t('disabled') - : needsAuth - ? t('needs authentication') - : t(server.status)} + : awaitingApproval + ? server.approvalState === 'rejected' + ? t('rejected — edit config to re-approve') + : t('needs approval') + : needsAuth + ? t('needs authentication') + : t(server.status)} diff --git a/packages/cli/src/ui/components/mcp/steps/ServerListStep.test.tsx b/packages/cli/src/ui/components/mcp/steps/ServerListStep.test.tsx index cd4b1899ac..fb755967c3 100644 --- a/packages/cli/src/ui/components/mcp/steps/ServerListStep.test.tsx +++ b/packages/cli/src/ui/components/mcp/steps/ServerListStep.test.tsx @@ -77,4 +77,69 @@ describe('ServerListStep', () => { pressKey({ name: 'p', sequence: '\u0010', ctrl: true }); expect(lastFrame()).toContain('❯ first'); }); + + describe('approval reason (gated servers skipped by discovery)', () => { + const gatedServer = ( + name: string, + approvalState: 'pending' | 'rejected', + ): MCPServerDisplayInfo => ({ + name, + status: MCPServerStatus.DISCONNECTED, + source: 'workspace', + config: { scope: 'workspace' }, + toolCount: 0, + promptCount: 0, + resourceCount: 0, + isDisabled: false, + approvalState, + }); + + it('shows "rejected" with the re-approve hint, not a bare "disconnected"', () => { + const { lastFrame } = render( + , + ); + expect(lastFrame()).toContain('rejected — edit config to re-approve'); + }); + + it('shows "needs approval" for a pending gated server', () => { + const { lastFrame } = render( + , + ); + expect(lastFrame()).toContain('needs approval'); + }); + + it('does not show the debug-log hint for an approval-skipped server', () => { + const { lastFrame } = render( + , + ); + expect(lastFrame()).not.toContain('see error logs'); + }); + + it('still shows the debug-log hint for a genuinely failed connection', () => { + const failed: MCPServerDisplayInfo = { + name: 'broken', + status: MCPServerStatus.DISCONNECTED, + source: 'user', + config: {}, + toolCount: 0, + promptCount: 0, + resourceCount: 0, + isDisabled: false, + }; + const { lastFrame } = render( + , + ); + expect(lastFrame()).toContain('see error logs'); + expect(lastFrame()).not.toContain('needs approval'); + }); + }); }); diff --git a/packages/cli/src/ui/components/mcp/steps/ServerListStep.tsx b/packages/cli/src/ui/components/mcp/steps/ServerListStep.tsx index 6f77c00276..661d086d62 100644 --- a/packages/cli/src/ui/components/mcp/steps/ServerListStep.tsx +++ b/packages/cli/src/ui/components/mcp/steps/ServerListStep.tsx @@ -109,14 +109,19 @@ export const ServerListStep: React.FC = ({ const isSelected = groupIndex === currentPosition.groupIndex && itemIndex === currentPosition.itemIndex; + // 受门控(#4615)但未审批的 server:discovery 直接跳过,不会进入 + // 连接/认证流程,所以审批原因优先于"需要认证"展示。 + const awaitingApproval = + !server.isDisabled && !!server.approvalState; // 未连接且需要认证时,状态以"需要认证"展示(requiresAuth 是 // 加载时的快照,状态被实时推到 connected 后不再适用) const needsAuth = !server.isDisabled && + !awaitingApproval && !!server.requiresAuth && server.status !== MCPServerStatus.CONNECTED; const statusColor = - server.isDisabled || needsAuth + server.isDisabled || awaitingApproval || needsAuth ? 'yellow' : getStatusColor(server.status); @@ -156,9 +161,13 @@ export const ServerListStep: React.FC = ({ {getStatusIcon(server.status)}{' '} {server.isDisabled ? t('disabled') - : needsAuth - ? t('needs authentication') - : t(server.status)} + : awaitingApproval + ? server.approvalState === 'rejected' + ? t('rejected — edit config to re-approve') + : t('needs approval') + : needsAuth + ? t('needs authentication') + : t(server.status)} {/* 显示无效工具警告 */} {!!server.invalidToolCount && server.invalidToolCount > 0 && ( @@ -176,8 +185,11 @@ export const ServerListStep: React.FC = ({ ))} - {/* 提示信息 */} - {servers.some((s) => s.status === 'disconnected' && !s.isDisabled) && ( + {/* 提示信息:仅针对"真正连接失败"的 server;被门控跳过(awaiting + approval)不是错误,不提示查日志 */} + {servers.some( + (s) => s.status === 'disconnected' && !s.isDisabled && !s.approvalState, + ) && ( ※ {t('Run qwen --debug to see error logs')} diff --git a/packages/cli/src/ui/components/mcp/types.ts b/packages/cli/src/ui/components/mcp/types.ts index aad74d9fc4..88213a63c4 100644 --- a/packages/cli/src/ui/components/mcp/types.ts +++ b/packages/cli/src/ui/components/mcp/types.ts @@ -56,6 +56,12 @@ export interface MCPServerDisplayInfo { hasOAuthTokens?: boolean; /** 未连接且需要(重新)认证:连接时收到 401,或声明了 OAuth 但无已存 token */ requiresAuth?: boolean; + /** + * 对于受门控(gated,#4615)但尚未审批的 server:discovery 跳过它(保持 + * disconnected)的原因。`pending`=等待首次/重新审批,`rejected`=已被拒绝。 + * 仅 gated scope 有值,其余为 undefined。 + */ + approvalState?: 'pending' | 'rejected'; } /** diff --git a/packages/cli/src/ui/hooks/useMcpApproval.test.ts b/packages/cli/src/ui/hooks/useMcpApproval.test.ts index 7bae0d82e8..0d5feb2982 100644 --- a/packages/cli/src/ui/hooks/useMcpApproval.test.ts +++ b/packages/cli/src/ui/hooks/useMcpApproval.test.ts @@ -12,6 +12,7 @@ import * as path from 'node:path'; import type { Config, MCPServerConfig } from '@qwen-code/qwen-code-core'; import { useMcpApproval } from './useMcpApproval.js'; import { McpApprovalChoice } from '../components/mcp/MCPServerApprovalDialog.js'; +import { appEvents, AppEvent } from '../../utils/events.js'; import { loadMcpApprovals, resetMcpApprovalsForTesting, @@ -144,6 +145,39 @@ describe('useMcpApproval', () => { expect(result.current.isMcpApprovalDialogOpen).toBe(false); }); + it('opens the dialog mid-session when a hot-reload pends a new gated server', () => { + // Starts with no servers — dialog closed. + const servers: Record = {}; + const config = makeConfig(servers); + const { result } = renderHook(() => useMcpApproval(config)); + expect(result.current.isMcpApprovalDialogOpen).toBe(false); + + // A hot-reload adds a gated, unapproved server and signals the change. + servers['c'] = { httpUrl: 'https://c.test', scope: 'project' }; + act(() => { + appEvents.emit(AppEvent.McpPendingApprovalChanged); + }); + + expect(result.current.isMcpApprovalDialogOpen).toBe(true); + expect(result.current.currentMcpApproval?.name).toBe('c'); + }); + + it('does not re-prompt mid-session for an already-approved server', async () => { + const a: MCPServerConfig = { command: 'a', scope: 'project' }; + await loadMcpApprovals().setState(dir, 'a', a, 'approved'); + resetMcpApprovalsForTesting(); + + const config = makeConfig({ a }); + const { result } = renderHook(() => useMcpApproval(config)); + expect(result.current.isMcpApprovalDialogOpen).toBe(false); + + act(() => { + appEvents.emit(AppEvent.McpPendingApprovalChanged); + }); + + expect(result.current.isMcpApprovalDialogOpen).toBe(false); + }); + it('skips servers already decided (only prompts pending)', async () => { const a: MCPServerConfig = { command: 'a', scope: 'project' }; // Pre-approve a. diff --git a/packages/cli/src/ui/hooks/useMcpApproval.ts b/packages/cli/src/ui/hooks/useMcpApproval.ts index 7b8991e92f..11c5956aed 100644 --- a/packages/cli/src/ui/hooks/useMcpApproval.ts +++ b/packages/cli/src/ui/hooks/useMcpApproval.ts @@ -13,6 +13,7 @@ import type { import { isGatedMcpScope } from '@qwen-code/qwen-code-core'; import { loadMcpApprovals } from '../../config/mcpApprovals.js'; import { McpApprovalChoice } from '../components/mcp/MCPServerApprovalDialog.js'; +import { appEvents, AppEvent } from '../../utils/events.js'; export interface PendingMcpServer { name: string; @@ -74,11 +75,14 @@ function summarize(config: MCPServerConfig): string { export const useMcpApproval = (config: Config) => { const [queue, setQueue] = useState([]); - useEffect(() => { + // Recompute the pending-approval queue from the authoritative sources (the + // live server map + persisted approvals). Reused for both the initial mount + // and mid-session recomputes triggered by a settings hot-reload. + const computePending = useCallback((): PendingMcpServer[] => { const servers = config.getMcpServers() ?? {}; const approvals = loadMcpApprovals(); const root = config.getWorkingDir(); - const pending: PendingMcpServer[] = Object.entries(servers) + return Object.entries(servers) .filter(([, c]) => isGatedMcpScope(c.scope)) .filter(([name, c]) => approvals.getState(root, name, c) === 'pending') .map(([name, c]) => ({ @@ -87,9 +91,28 @@ export const useMcpApproval = (config: Config) => { summary: summarize(c), source: sourceLabel(c.scope), })); - setQueue(pending); }, [config]); + // Initial queue at startup. + useEffect(() => { + setQueue(computePending()); + }, [computePending]); + + // A hot-reload may push a gated server into `pending` mid-session (e.g. an + // edit invalidated its hash-bound approval). Re-evaluate so the dialog pops + // immediately instead of only at startup. Servers the user has since + // approved/rejected are filtered out by `computePending` reading the + // persisted file, so this never re-prompts a settled decision. See #4615. + useEffect(() => { + const onPendingChanged = () => { + setQueue(computePending()); + }; + appEvents.on(AppEvent.McpPendingApprovalChanged, onPendingChanged); + return () => { + appEvents.off(AppEvent.McpPendingApprovalChanged, onPendingChanged); + }; + }, [computePending]); + const reconnect = useCallback( (name: string) => { config.approveMcpServerForSession(name); diff --git a/packages/cli/src/utils/events.ts b/packages/cli/src/utils/events.ts index 976f4a751d..5a6275f6a8 100644 --- a/packages/cli/src/utils/events.ts +++ b/packages/cli/src/utils/events.ts @@ -11,6 +11,13 @@ export enum AppEvent { LogError = 'log-error', OauthDisplayMessage = 'oauth-display-message', OauthAuthUrl = 'oauth-auth-url', + /** + * A settings hot-reload changed the set of gated MCP servers pending + * approval (e.g. an edit to a `workspace`/`project`-scoped server invalidated + * its hash-bound approval). Drives the interactive approval dialog to + * re-evaluate mid-session instead of only at startup. See issue #4615. + */ + McpPendingApprovalChanged = 'mcp-pending-approval-changed', } export const appEvents = new EventEmitter(); diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index d989188c08..4ee75a2f87 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -530,6 +530,255 @@ describe('Server Config (config.ts)', () => { }); }); + describe('MCP hot-reload (sub-task 3)', () => { + const srvA: MCPServerConfig = { command: 'a' }; + const srvB: MCPServerConfig = { command: 'b' }; + + it('setMcpServers REPLACES (not merges) and works post-init', async () => { + const config = new Config({ + ...baseParams, + mcpServers: { a: srvA }, + }); + await config.initialize(); + + // addMcpServers would throw post-init; setMcpServers must not. + config.setMcpServers({ b: srvB }); + + const settingsLayer = config.getSettingsMcpServers(); + expect(settingsLayer).toEqual({ b: srvB }); + expect(settingsLayer).not.toHaveProperty('a'); + }); + + it('reinitializeMcpServers is a safe no-op before initialize()', async () => { + const config = new Config({ ...baseParams }); + // No tool registry yet — must not throw and must not connect. + await expect( + config.reinitializeMcpServers({ a: srvA }), + ).resolves.toBeUndefined(); + expect(config.getSettingsMcpServers()).toEqual({ a: srvA }); + }); + + it('records MCP servers removed by a reconcile and self-heals on re-add', async () => { + const config = new Config({ + ...baseParams, + mcpServers: { a: srvA, b: srvB }, + }); + + // Drop `a` → tracked as recently removed. + await config.reinitializeMcpServers({ b: srvB }); + expect(config.getRecentlyRemovedMcpServers()).toEqual(['a']); + + // Drop `b` too → both tracked. + await config.reinitializeMcpServers({}); + expect(config.getRecentlyRemovedMcpServers().sort()).toEqual(['a', 'b']); + + // Re-add `a` → it self-heals out of the set; `b` stays removed. + await config.reinitializeMcpServers({ a: srvA }); + expect(config.getRecentlyRemovedMcpServers()).toEqual(['b']); + }); + + it('classifies a server filtered by a narrowed allow-list as not_allowed (not removed)', async () => { + const config = new Config({ + ...baseParams, + mcpServers: { a: srvA, b: srvB }, + }); + await config.reinitializeMcpServers({ a: srvA, b: srvB }); + + // Narrow the allow-list to just `a` (mirrors editing mcp.allowed). `b` is + // still configured (merged map) but filtered out of the effective map. + config.setAllowedMcpServers(['a']); + + // `b` is NOT "removed" — it's still in config, just not allowed. The + // tool-not-found path can still explain it precisely, with the right + // recovery action (adjust mcp.allowed, not "re-add the server"). + expect(config.getRecentlyRemovedMcpServers()).not.toContain('b'); + expect(config.getMcpServerUnavailableReason('b')).toBe('not_allowed'); + expect(config.getMcpServerUnavailableReason('a')).toBeUndefined(); + }); + + it('classifies excluded / pending / removed servers with the right reason', async () => { + const config = new Config({ + ...baseParams, + mcpServers: { a: srvA, b: srvB, c: srvA }, + }); + await config.reinitializeMcpServers({ a: srvA, b: srvB, c: srvA }); + + config.setExcludedMcpServers(['b']); + config.setPendingMcpServers(['c']); + expect(config.getMcpServerUnavailableReason('b')).toBe('excluded'); + expect(config.getMcpServerUnavailableReason('c')).toBe( + 'pending_approval', + ); + + // Delete `a` from config → removed this session. + await config.reinitializeMcpServers({ b: srvB, c: srvA }); + expect(config.getMcpServerUnavailableReason('a')).toBe('removed'); + // A never-configured name has no reason (falls through to generic). + expect(config.getMcpServerUnavailableReason('ghost')).toBeUndefined(); + }); + + it('reinitializeMcpServers replaces config then drives incremental reconcile', async () => { + const config = new Config({ ...baseParams, mcpServers: { a: srvA } }); + await config.initialize(); + const manager = ( + config.getToolRegistry() as unknown as { + __mcpManagerMock: { discoverAllMcpToolsIncremental: Mock }; + } + ).__mcpManagerMock; + manager.discoverAllMcpToolsIncremental.mockClear(); + + await config.reinitializeMcpServers({ b: srvB }); + + expect(config.getSettingsMcpServers()).toEqual({ b: srvB }); + expect(manager.discoverAllMcpToolsIncremental).toHaveBeenCalledTimes(1); + expect(manager.discoverAllMcpToolsIncremental).toHaveBeenCalledWith( + config, + ); + }); + + it('coalesces a reconcile request that arrives mid-flight into one extra pass', async () => { + const config = new Config({ ...baseParams, mcpServers: { a: srvA } }); + await config.initialize(); + const manager = ( + config.getToolRegistry() as unknown as { + __mcpManagerMock: { discoverAllMcpToolsIncremental: Mock }; + } + ).__mcpManagerMock; + manager.discoverAllMcpToolsIncremental.mockClear(); + + // Make the first pass hang until we release it, so the second call + // arrives while the first is in flight. + let release!: () => void; + manager.discoverAllMcpToolsIncremental.mockImplementationOnce( + () => + new Promise((resolve) => { + release = resolve; + }), + ); + + const first = config.reinitializeMcpServers({ b: srvB }); + // Second call lands mid-flight → coalesced, not a third pass. + const second = config.reinitializeMcpServers({ a: srvA, b: srvB }); + release(); + await Promise.all([first, second]); + + // One in-flight pass + one drained follow-up = exactly 2 passes. + expect(manager.discoverAllMcpToolsIncremental).toHaveBeenCalledTimes(2); + }); + + it('rethrows a failed reconcile and resets the in-progress guard so the next call still runs', async () => { + const config = new Config({ ...baseParams, mcpServers: { a: srvA } }); + await config.initialize(); + const manager = ( + config.getToolRegistry() as unknown as { + __mcpManagerMock: { discoverAllMcpToolsIncremental: Mock }; + } + ).__mcpManagerMock; + manager.discoverAllMcpToolsIncremental.mockClear(); + + // First pass fails — reinitialize must surface the error. + manager.discoverAllMcpToolsIncremental.mockRejectedValueOnce( + new Error('reconcile boom'), + ); + await expect(config.reinitializeMcpServers({ b: srvB })).rejects.toThrow( + 'reconcile boom', + ); + + // Guard must have been reset in `finally`; a subsequent call must not be + // silently coalesced/dropped — it runs a fresh pass. + await config.reinitializeMcpServers({ a: srvA }); + expect(manager.discoverAllMcpToolsIncremental).toHaveBeenCalledTimes(2); + }); + + it('clears the coalesce flag when a reconcile throws, so the next call runs exactly one pass', async () => { + const config = new Config({ ...baseParams, mcpServers: { a: srvA } }); + await config.initialize(); + const manager = ( + config.getToolRegistry() as unknown as { + __mcpManagerMock: { discoverAllMcpToolsIncremental: Mock }; + } + ).__mcpManagerMock; + manager.discoverAllMcpToolsIncremental.mockClear(); + + // Pass 1 hangs until we reject it; while it is in flight a second call + // arrives and is coalesced (sets the pending flag). + let reject!: (e: Error) => void; + manager.discoverAllMcpToolsIncremental.mockImplementationOnce( + () => + new Promise((_, rej) => { + reject = rej; + }), + ); + const first = config.reinitializeMcpServers({ b: srvB }); + const second = config.reinitializeMcpServers({ a: srvA, b: srvB }); + reject(new Error('reconcile boom')); + await expect(first).rejects.toThrow('reconcile boom'); + // The coalesced caller awaits the shared in-flight pass, so it observes + // the SAME failure rather than resolving before its change was applied. + await expect(second).rejects.toThrow('reconcile boom'); + + // The throw must have cleared the pending flag too. A subsequent + // unrelated reconcile must run EXACTLY ONE pass — not an extra stale + // drain pass left over from the coalesced-then-aborted request. + manager.discoverAllMcpToolsIncremental.mockClear(); + await config.reinitializeMcpServers({ a: srvA }); + expect(manager.discoverAllMcpToolsIncremental).toHaveBeenCalledTimes(1); + }); + + it('a coalesced reconcile awaits the in-flight pass + its drain (does not resolve early)', async () => { + const config = new Config({ ...baseParams, mcpServers: { a: srvA } }); + await config.initialize(); + const manager = ( + config.getToolRegistry() as unknown as { + __mcpManagerMock: { discoverAllMcpToolsIncremental: Mock }; + } + ).__mcpManagerMock; + manager.discoverAllMcpToolsIncremental.mockClear(); + + // Pass 1 hangs until released; the second call lands mid-flight. + let release!: () => void; + manager.discoverAllMcpToolsIncremental.mockImplementationOnce( + () => + new Promise((resolve) => { + release = resolve; + }), + ); + + let secondResolved = false; + const first = config.reinitializeMcpServers({ b: srvB }); + const second = config + .reinitializeMcpServers({ a: srvA, b: srvB }) + .then(() => { + secondResolved = true; + }); + + // While pass 1 is still in flight the coalesced caller must NOT have + // resolved — it is chained onto the shared in-flight reconcile, so its + // change has not been applied yet. + await Promise.resolve(); + expect(secondResolved).toBe(false); + + release(); + await Promise.all([first, second]); + expect(secondResolved).toBe(true); + // pass 1 + exactly one drain (for the coalesced change) = 2 passes. + expect(manager.discoverAllMcpToolsIncremental).toHaveBeenCalledTimes(2); + }); + + it('admission-list setters and getMcpGating round-trip', () => { + const config = new Config({ ...baseParams }); + config.setExcludedMcpServers(['x']); + config.setAllowedMcpServers(['y']); + config.setPendingMcpServers(['z']); + expect(config.getMcpGating()).toEqual({ + excluded: ['x'], + allowed: ['y'], + pending: ['z'], + }); + expect(config.getAllowedMcpServers()).toEqual(['y']); + }); + }); + describe('MemoryPressureMonitor isolation', () => { it('returns a distinct monitor for child Configs created via Object.create', async () => { const parent = new Config(baseParams); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 642cc61d3f..e55575a120 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -12,7 +12,7 @@ import * as path from 'node:path'; import process from 'node:process'; // External dependencies -import { ProxyAgent, setGlobalDispatcher } from 'undici'; +import { EnvHttpProxyAgent, setGlobalDispatcher } from 'undici'; // Types import type { @@ -594,6 +594,21 @@ export const DEFAULT_TOOL_OUTPUT_BATCH_BUDGET = 200_000; */ export type McpServerScope = 'project' | 'workspace' | 'system'; +/** + * Why an MCP server's tools are currently unavailable, used to give the model a + * precise tool-not-found recovery action. See + * {@link Config.getMcpServerUnavailableReason}. + * - `removed`: deleted from config this session. + * - `not_allowed`: filtered out by the `mcp.allowed` allow-list. + * - `excluded`: present in the `mcp.excluded` list. + * - `pending_approval`: a gated server awaiting approval (#4615). + */ +export type McpServerUnavailableReason = + | 'removed' + | 'not_allowed' + | 'excluded' + | 'pending_approval'; + /** * Scopes whose servers are checked-in / shareable and therefore untrusted: they * must be approved before the discovery layer connects them. `'system'` @@ -783,6 +798,13 @@ export interface ConfigParameters { toolCallCommand?: string; mcpServerCommand?: string; mcpServers?: Record; + /** + * Session-injected (ACP/IDE) + `--mcp-config` servers that sit above the + * settings layer and `.mcp.json` and are never gated (#4615). Retained so the + * hot-reload subscriber (sub-task 3) can re-assemble the effective map the + * same way boot did. See `assembleMcpServers`. + */ + topTierMcpServers?: Record; lsp?: { enabled?: boolean; }; @@ -863,6 +885,14 @@ export interface ConfigParameters { /** Locale code for resolving localizable extension fields (e.g., 'en', 'zh'). */ locale?: string; allowedMcpServers?: string[]; + /** + * The startup `--allowed-mcp-server-names` CLI flag value, if passed (the + * flag only — NOT the settings-derived allow-list). When present it is an + * immutable upper bound on MCP admission: a hot-reload may narrow within it + * but never widen beyond it. Undefined when the flag was not passed (then + * settings fully drive admission). See issue #3696 sub-task 3. + */ + cliAllowedMcpServerNames?: string[]; excludedMcpServers?: string[]; /** * Names of project-scoped (`.mcp.json`) servers that are NOT yet approved @@ -1252,13 +1282,42 @@ export class Config { private readonly toolCallCommand: string | undefined; private readonly mcpServerCommand: string | undefined; private mcpServers: Record | undefined; + /** + * Names of MCP servers that were present in the effective server map but + * disappeared after a runtime reconcile (hot-reload / `/reload`). Used only + * to give a precise "this MCP server was removed this session" message when + * the model later calls a tool that no longer exists (see + * `CoreToolScheduler.getToolNotFoundMessage`). Self-heals: a name is dropped + * from the set the moment the server reappears in the effective map. + */ + private readonly recentlyRemovedMcpServers = new Set(); + private readonly topTierMcpServers: + | Record + | undefined; private readonly runtimeMcpServers = new Map(); private readonly lspEnabled: boolean; private lspClient?: LspClient; private lspInitializationError?: string; - private readonly allowedMcpServers?: string[]; + private allowedMcpServers?: string[]; + /** Immutable upper bound from `--allowed-mcp-server-names`; see ConfigParameters. */ + private readonly cliAllowedMcpServerNames?: string[]; private excludedMcpServers?: string[]; private pendingMcpServers?: string[]; + /** + * Guards against concurrent MCP reconcile passes (hot-reload watcher vs. + * `/reload`). `SettingsWatcher` serializes its own listeners, but `/reload` + * shares no such lock; without this, two `reinitializeMcpServers` calls could + * interleave their `discoverAllMcpToolsIncremental` passes. See sub-task 3. + */ + private mcpReconcileInProgress = false; + private mcpReconcilePending = false; + /** + * The in-flight reconcile (pass 1 + its coalesced drain loop), exposed so a + * call arriving mid-flight can await the same work instead of returning + * before its coalesced change has actually been applied. Cleared when the + * loop settles. + */ + private mcpReconcilePromise: Promise | undefined; private sessionSubagents: SubagentConfig[]; private userMemory: string; private sdkMode: boolean; @@ -1451,9 +1510,11 @@ export class Config { this.toolCallCommand = params.toolCallCommand; this.mcpServerCommand = params.mcpServerCommand; this.mcpServers = params.mcpServers; + this.topTierMcpServers = params.topTierMcpServers; this.lspEnabled = params.lsp?.enabled ?? false; this.lspClient = params.lspClient; this.allowedMcpServers = params.allowedMcpServers; + this.cliAllowedMcpServerNames = params.cliAllowedMcpServerNames; this.excludedMcpServers = params.excludedMcpServers; this.pendingMcpServers = params.pendingMcpServers; this.sessionSubagents = params.sessionSubagents ?? []; @@ -1632,7 +1693,19 @@ export class Config { const proxyUrl = this.getProxy(); if (proxyUrl) { - setGlobalDispatcher(new ProxyAgent(proxyUrl)); + // Use EnvHttpProxyAgent (not a bare ProxyAgent) so `NO_PROXY` is + // honored. A bare ProxyAgent tunnels EVERY request — including local + // MCP servers reached over `http://localhost:...` — through the proxy, + // which typically can't route back to localhost and fails with an + // opaque `fetch failed`. EnvHttpProxyAgent connects hosts listed in + // `NO_PROXY` (e.g. `localhost,127.0.0.1`) directly while still proxying + // everything else (LLM API calls, remote MCP). The explicit + // `--proxy` / `settings.proxy` value (resolved by `getProxy()`) + // overrides env `http(s)_proxy`; `NO_PROXY` continues to come from the + // environment. See issue #3696 (local MCP + corporate proxy). + setGlobalDispatcher( + new EnvHttpProxyAgent({ httpProxy: proxyUrl, httpsProxy: proxyUrl }), + ); } this.geminiClient = new GeminiClient(this); this.chatRecordingService = this.chatRecordingEnabled @@ -3345,8 +3418,25 @@ export class Config { return this.mcpServers; } - getMcpServers(): Record | undefined { - let mcpServers = { ...(this.mcpServers || {}) }; + /** + * Session-injected + `--mcp-config` ("top-tier") servers captured at boot, so + * the hot-reload subscriber can re-assemble the effective MCP map exactly the + * way boot did. See sub-task 3 and `assembleMcpServers`. + */ + getTopTierMcpServers(): Record | undefined { + return this.topTierMcpServers; + } + + /** + * The merged MCP server map (settings + extensions + runtime overlay) WITHOUT + * any admission filtering. `getMcpServers()` is this map with the + * `allowedMcpServers` filter applied; the unfiltered form is what tells us a + * server is "configured" regardless of allow-list / excluded / pending gating + * (used to classify why a server is unavailable — see + * {@link getMcpServerUnavailableReason}). + */ + private getMergedMcpServers(): Record { + const mcpServers = { ...(this.mcpServers || {}) }; const extensions = this.getActiveExtensions(); for (const extension of extensions) { Object.entries(extension.config.mcpServers || {}).forEach( @@ -3365,6 +3455,12 @@ export class Config { mcpServers[name] = cfg; } + return mcpServers; + } + + getMcpServers(): Record | undefined { + let mcpServers = this.getMergedMcpServers(); + if (this.allowedMcpServers) { mcpServers = Object.fromEntries( Object.entries(mcpServers).filter(([key]) => @@ -3442,6 +3538,222 @@ export class Config { this.mcpServers = { ...this.mcpServers, ...servers }; } + /** + * Replace the settings-layer MCP server map at runtime (hot-reload). + * Unlike {@link addMcpServers}, this bypasses the `initialized` guard and + * REPLACES (not merges) so removals take effect. The runtime overlay + * ({@link addRuntimeMcpServer}) and extension contributions are unaffected — + * {@link getMcpServers} still layers them on top. See sub-task 3. + */ + setMcpServers(servers: Record | undefined): void { + this.mcpServers = servers; + } + + /** + * Replace the allow-list of MCP server names at runtime (hot-reload). When + * set, {@link getMcpServers} only yields servers whose name is in this list. + * `allowedMcpServers` is consulted as a filter inside `getMcpServers()`, so + * without this setter an allow-list edit would silently require a restart. + */ + setAllowedMcpServers(allowed: string[] | undefined): void { + this.allowedMcpServers = allowed; + } + + getAllowedMcpServers(): string[] | undefined { + return this.allowedMcpServers; + } + + /** + * The startup `--allowed-mcp-server-names` upper bound (the CLI flag only), + * or undefined if the flag was not passed. The hot-reload recompute caps the + * settings-derived allow-list to this so a runtime settings edit can narrow + * MCP admission but never widen it beyond what the launch flag permitted. + */ + getCliAllowedMcpServerNames(): string[] | undefined { + return this.cliAllowedMcpServerNames; + } + + /** + * Replace the pending-approval set of gated MCP server names at runtime + * (hot-reload). The discovery layer skips these BEFORE any connection side + * effect, so a hot-reload must recompute them (#4615) lest it connect a + * newly-added but unapproved `.mcp.json`/workspace server. + */ + setPendingMcpServers(pending: string[] | undefined): void { + this.pendingMcpServers = pending; + } + + /** + * Snapshot of the three connection-admission lists consulted by discovery, + * used by the hot-reload subscriber as the pre-image to diff against. Paired + * with {@link setExcludedMcpServers} / {@link setAllowedMcpServers} / + * {@link setPendingMcpServers}. + */ + getMcpGating(): { + excluded?: string[]; + allowed?: string[]; + pending?: string[]; + } { + return { + excluded: this.excludedMcpServers, + allowed: this.allowedMcpServers, + pending: this.pendingMcpServers, + }; + } + + /** + * Names of MCP servers removed from config during this session by a runtime + * reconcile and not since re-added. "Removed" means gone from the merged map + * (settings + extensions + runtime), NOT merely filtered out by an admission + * gate — a server that is still configured but excluded / not-allowed / + * pending is reported via {@link getMcpServerUnavailableReason} instead. + * Consumed by the tool-not-found path. + */ + getRecentlyRemovedMcpServers(): string[] { + return [...this.recentlyRemovedMcpServers]; + } + + /** All configured MCP server names (merged, before admission gating). */ + getMcpServerNames(): string[] { + return Object.keys(this.getMergedMcpServers()); + } + + /** + * Why a given MCP server is currently unavailable (its tools aren't usable), + * or `undefined` if it is configured and admitted (so a missing tool is a + * genuine "not found" / disconnected, not an admission decision). Lets the + * tool-not-found path explain the right recovery action. Covers every + * admission gate: + * - `removed`: deleted from config this session (see + * {@link getRecentlyRemovedMcpServers}). + * - `not_allowed`: filtered out by the `mcp.allowed` allow-list. + * - `excluded`: in the `mcp.excluded` list. + * - `pending_approval`: a gated server awaiting approval (#4615). + */ + getMcpServerUnavailableReason( + serverName: string, + ): McpServerUnavailableReason | undefined { + if (this.recentlyRemovedMcpServers.has(serverName)) return 'removed'; + if (!(serverName in this.getMergedMcpServers())) return undefined; + if ( + this.allowedMcpServers && + !this.allowedMcpServers.includes(serverName) + ) { + return 'not_allowed'; + } + if (this.excludedMcpServers?.includes(serverName)) return 'excluded'; + if (this.isMcpServerPendingApproval(serverName)) return 'pending_approval'; + return undefined; + } + + /** + * Apply a new settings-layer MCP map and incrementally reconcile live + * connections (connect added, disconnect removed, restart changed; unchanged + * servers untouched). Safe no-op before {@link initialize}. A shared + * "reconcile in progress" guard serializes against a concurrent caller (e.g. + * `/reload`): a request arriving mid-flight is coalesced into a single + * follow-up pass so the latest config always wins. See sub-task 3. + */ + async reinitializeMcpServers( + servers: Record | undefined, + ): Promise { + this.debugLogger.debug( + `[mcp-hot-reload] reinitializeMcpServers: servers=[${Object.keys( + servers ?? {}, + ).join( + ', ', + )}] initialized=${this.initialized} inProgress=${this.mcpReconcileInProgress}`, + ); + + // Track which servers were DELETED from config this session (gone from the + // merged map), so the tool-not-found path can say "removed this session" + // vs an admission-gate reason. The merged map is independent of the + // admission gates (allowed/excluded/pending), so the diff is unaffected by + // the gating setters the hot-reload caller applied just before this — no + // pre-gating snapshot needed. Re-added names self-heal. + const prevConfigured = new Set(Object.keys(this.getMergedMcpServers())); + this.setMcpServers(servers); + const nextConfigured = new Set(Object.keys(this.getMergedMcpServers())); + for (const name of nextConfigured) { + this.recentlyRemovedMcpServers.delete(name); + } + for (const name of prevConfigured) { + if (!nextConfigured.has(name)) { + this.recentlyRemovedMcpServers.add(name); + } + } + if (!this.initialized) { + // No tool registry yet — boot-time discovery will pick up the new map. + this.debugLogger.debug( + '[mcp-hot-reload] not initialized yet — deferring to boot-time discovery (no-op)', + ); + return; + } + if (this.mcpReconcileInProgress) { + // Coalesce: a pass is already running. Mark that the desired state + // advanced so its drain loop runs again with the latest config, and + // await that in-flight pass — NOT a resolved promise — so this caller + // does not proceed (e.g. the hot-reload listener emitting approval events + // and logging "complete") before its coalesced change is actually + // reconciled, and so it observes a shared reconcile failure. + this.mcpReconcilePending = true; + this.debugLogger.debug( + '[mcp-hot-reload] reconcile already in flight — coalescing into a follow-up pass', + ); + return this.mcpReconcilePromise ?? Promise.resolve(); + } + this.mcpReconcileInProgress = true; + const registry = this.getToolRegistry(); + // Run pass 1 + its drain loop as a single promise, assigned BEFORE the + // first await so a coalesced caller arriving mid-flight can await it. + const runReconcile = (async () => { + try { + this.debugLogger.debug( + '[mcp-hot-reload] running incremental reconcile (pass 1)', + ); + await registry + .getMcpClientManager() + .discoverAllMcpToolsIncremental(this); + // Drain any change that arrived while this pass was in flight. The pool + // path returns the in-flight promise rather than queuing, so awaiting + // is not enough — re-run once more to pick up the latest config. + let pass = 1; + while (this.mcpReconcilePending) { + this.mcpReconcilePending = false; + pass += 1; + this.debugLogger.debug( + `[mcp-hot-reload] running coalesced incremental reconcile (pass ${pass})`, + ); + await registry + .getMcpClientManager() + .discoverAllMcpToolsIncremental(this); + } + this.debugLogger.debug( + `[mcp-hot-reload] reconcile complete after ${pass} pass(es); live servers=[${Object.keys( + this.getMcpServers() ?? {}, + ).join(', ')}]`, + ); + } catch (err) { + this.debugLogger.error( + `[mcp-hot-reload] reconcile failed: ${err instanceof Error ? (err.stack ?? err.message) : String(err)}`, + ); + throw err; + } finally { + this.mcpReconcileInProgress = false; + // Clear the coalesce flag too: if a pass threw, a pending follow-up + // would otherwise stay stuck `true` and make the next (unrelated) + // reconcile run an extra no-op drain pass. The next real settings + // change re-triggers reconcile anyway. + this.mcpReconcilePending = false; + this.mcpReconcilePromise = undefined; + } + })(); + this.mcpReconcilePromise = runReconcile; + // Propagate failure to this caller (and, via the shared promise, to any + // coalesced callers). Existing callers rely on the throw. + await runReconcile; + } + /** * Add a runtime-only MCP server. Unlike `addMcpServers`, this does NOT * touch `this.mcpServers` (settings layer) and does not enforce the diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 02d77c1e39..1d7c27c976 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -2989,6 +2989,179 @@ describe('CoreToolScheduler', () => { ); }); + describe('MCP tool-not-found messaging', () => { + const makeScheduler = (opts: { + mcpServers?: Record; + removed?: string[]; + // Per-server admission reason for configured-but-unavailable servers. + reasons?: Record; + allToolNames?: string[]; + }) => { + const mockToolRegistry = { + getAllToolNames: () => opts.allToolNames ?? [], + getTool: () => undefined, + ensureTool: async () => undefined, + } as unknown as ToolRegistry; + const mockConfig = { + getToolRegistry: () => mockToolRegistry, + getUseModelRouter: () => false, + getGeminiClient: () => null, + getPermissionsDeny: () => undefined, + isInteractive: () => true, + getMessageBus: vi.fn().mockReturnValue(undefined), + getDisableAllHooks: vi.fn().mockReturnValue(true), + getMcpServerNames: () => Object.keys(opts.mcpServers ?? {}), + getRecentlyRemovedMcpServers: () => opts.removed ?? [], + getMcpServerUnavailableReason: (name: string) => { + if ((opts.removed ?? []).includes(name)) return 'removed'; + if (!(name in (opts.mcpServers ?? {}))) return undefined; + return opts.reasons?.[name]; + }, + } as unknown as Config; + return new CoreToolScheduler({ + config: mockConfig, + getPreferredEditor: () => 'vscode', + onEditorClose: vi.fn(), + }); + }; + + it('names a server removed this session (precise, branch B)', () => { + const scheduler = makeScheduler({ + mcpServers: {}, + removed: ['pangu-server'], + }); + // @ts-expect-error accessing private method + const msg = scheduler.getMcpToolUnavailableMessage( + 'mcp__pangu-server__pangu_search', + ); + expect(msg).toContain('"pangu-server"'); + expect(msg).toContain('removed during this session'); + }); + + it('reports an MCP tool with no configured server (branch A)', () => { + const scheduler = makeScheduler({ mcpServers: {}, removed: [] }); + // @ts-expect-error accessing private method + const msg = scheduler.getMcpToolUnavailableMessage( + 'mcp__ghost__do_thing', + ); + expect(msg).toContain( + 'no MCP server providing it is currently configured', + ); + }); + + it('reports a configured server that lacks the tool', () => { + const scheduler = makeScheduler({ + mcpServers: { 'pangu-server': {} }, + removed: [], + }); + // @ts-expect-error accessing private method + const msg = scheduler.getMcpToolUnavailableMessage( + 'mcp__pangu-server__missing_tool', + ); + expect(msg).toContain('on MCP server "pangu-server"'); + }); + + it('explains a not-allowed server with the allow-list recovery action', () => { + const scheduler = makeScheduler({ + mcpServers: { 'pangu-server': {} }, + reasons: { 'pangu-server': 'not_allowed' }, + }); + // @ts-expect-error accessing private method + const msg = scheduler.getMcpToolUnavailableMessage( + 'mcp__pangu-server__search', + ); + expect(msg).toContain('"pangu-server"'); + expect(msg).toContain('allow-list'); + expect(msg).toContain('mcp.allowed'); + }); + + it('explains an excluded server with the mcp.excluded recovery action', () => { + const scheduler = makeScheduler({ + mcpServers: { 'pangu-server': {} }, + reasons: { 'pangu-server': 'excluded' }, + }); + // @ts-expect-error accessing private method + const msg = scheduler.getMcpToolUnavailableMessage( + 'mcp__pangu-server__search', + ); + expect(msg).toContain('excluded'); + expect(msg).toContain('mcp.excluded'); + }); + + it('explains a pending-approval server with the approval recovery action', () => { + const scheduler = makeScheduler({ + mcpServers: { 'pangu-server': {} }, + reasons: { 'pangu-server': 'pending_approval' }, + }); + // @ts-expect-error accessing private method + const msg = scheduler.getMcpToolUnavailableMessage( + 'mcp__pangu-server__search', + ); + expect(msg).toContain('awaiting approval'); + expect(msg).toContain('/mcp'); + }); + + it('prefers the removed-this-session message over the generic one', () => { + const scheduler = makeScheduler({ + mcpServers: {}, + removed: ['pangu-server'], + }); + // @ts-expect-error accessing private method + const msg = scheduler.getMcpToolUnavailableMessage( + 'mcp__pangu-server__pangu_search', + ); + expect(msg).toContain('removed during this session'); + expect(msg).not.toContain('currently configured'); + }); + + it('returns null for non-MCP names (keeps generic suggestion path)', () => { + const scheduler = makeScheduler({ mcpServers: {}, removed: [] }); + // @ts-expect-error accessing private method + expect(scheduler.getMcpToolUnavailableMessage('list_fils')).toBeNull(); + }); + + it('a prefix server name does not match a longer server (boundary)', () => { + // `foo` must NOT claim a `mcp__foobar__*` tool. + const scheduler = makeScheduler({ mcpServers: {}, removed: ['foo'] }); + // @ts-expect-error accessing private method + const msg = scheduler.getMcpToolUnavailableMessage('mcp__foobar__x'); + expect(msg).not.toContain('removed during this session'); + expect(msg).toContain('no MCP server providing it'); + }); + + it('attributes the tool to the most specific server when names are prefixes', () => { + // Both `foo` and `foo__bar` exist; `mcp__foo__bar__baz` startsWith both + // `mcp__foo__` and `mcp__foo__bar__`. The longer (more specific) server + // must win regardless of iteration order — not the first match. + const scheduler = makeScheduler({ + mcpServers: {}, + removed: ['foo', 'foo__bar'], + }); + // @ts-expect-error accessing private method + const msg = scheduler.getMcpToolUnavailableMessage('mcp__foo__bar__baz'); + expect(msg).toContain('"foo__bar"'); + expect(msg).toContain('removed during this session'); + }); + + it('getToolNotFoundMessage routes MCP names to the MCP branch, others to Levenshtein', async () => { + const scheduler = makeScheduler({ + mcpServers: {}, + removed: ['pangu-server'], + allToolNames: ['list_files', 'read_file'], + }); + // @ts-expect-error accessing private method + const mcpMsg = await scheduler.getToolNotFoundMessage( + 'mcp__pangu-server__pangu_search', + ); + expect(mcpMsg).toContain('removed during this session'); + expect(mcpMsg).not.toContain('Did you mean'); + + // @ts-expect-error accessing private method + const genericMsg = await scheduler.getToolNotFoundMessage('list_fils'); + expect(genericMsg).toContain('Did you mean'); + }); + }); + describe('getToolSuggestion', () => { it('should suggest the top N closest tool names for a typo', () => { // Create mocked tool registry diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 3b5c037b29..69598a49c0 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -1617,11 +1617,84 @@ export class CoreToolScheduler { } } + // MCP tool whose server is gone / unconfigured: explain in MCP terms + // instead of falling through to a Levenshtein suggestion that would surface + // unrelated tools (e.g. "did you mean computer_use__click?"). + const mcpMessage = this.getMcpToolUnavailableMessage(unknownToolName); + if (mcpMessage) { + return mcpMessage; + } + // Standard "not found" message with Levenshtein suggestions const suggestion = this.getToolSuggestion(unknownToolName, topN); return `Tool "${unknownToolName}" not found in registry. Tools must use the exact names that are registered.${suggestion}`; } + /** + * For an `mcp____` name whose tool is not registered, explains + * *why* in MCP terms — the server was removed this session, is not (or no + * longer) configured, or is configured but lacks that tool — instead of + * letting the generic Levenshtein path suggest unrelated tools. Returns null + * for non-MCP names so they keep the existing suggestion behaviour unchanged. + * + * Detection is by prefix-membership against known server names (each + * sanitized the way `generateValidName` builds the registered tool name), + * never by parsing the server back out of the unknown name: the `__` + * separator is ambiguous and long names are truncated, so extraction is + * unreliable. A name we cannot classify falls through to the generic message. + */ + private getMcpToolUnavailableMessage(unknownToolName: string): string | null { + if (!unknownToolName.startsWith('mcp__')) { + return null; + } + // Mirror generateValidName's per-char sanitization when rebuilding a + // 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 => + `mcp__${server}__`.replace(/[^a-zA-Z0-9_.-]/g, '_'); + // When one server name is a prefix of another after sanitization (e.g. + // `foo` vs `foo__bar`), a tool of the longer server also startsWith the + // shorter one's prefix. Match longest-first so the most specific server + // wins, instead of relying on iteration order. + const byPrefixLengthDesc = (a: string, b: string) => + prefixOf(b).length - prefixOf(a).length; + + // Candidate servers: everything still configured (regardless of admission + // state) plus servers removed this session. Longest-prefix-first so the most + // specific server wins when one name is a prefix of another. + const candidates = Array.from( + new Set([ + ...(this.config.getMcpServerNames?.() ?? []), + ...(this.config.getRecentlyRemovedMcpServers?.() ?? []), + ]), + ).sort(byPrefixLengthDesc); + const serverHit = candidates.find((s) => + unknownToolName.startsWith(prefixOf(s)), + ); + + if (!serverHit) { + // No known server owns this prefix → never configured. + return `Tool "${unknownToolName}" not found: no MCP server providing it is currently configured. If you recently removed or renamed an MCP server, its tools are no longer available.`; + } + + // Explain WHY the owning server's tools aren't loaded, with the matching + // recovery action for each admission gate. + switch (this.config.getMcpServerUnavailableReason?.(serverHit)) { + case 'removed': + return `Tool "${unknownToolName}" is unavailable: the MCP server "${serverHit}" was removed during this session, so its tools were unloaded. Re-add it to your settings to use this tool again.`; + case 'not_allowed': + return `Tool "${unknownToolName}" is unavailable: the MCP server "${serverHit}" is not in the allow-list (mcp.allowed / --allowed-mcp-server-names), so its tools are not loaded. Add it to mcp.allowed to use this tool.`; + case 'excluded': + return `Tool "${unknownToolName}" is unavailable: the MCP server "${serverHit}" is excluded (mcp.excluded), so its tools are not loaded. Remove it from mcp.excluded to use this tool.`; + case 'pending_approval': + return `Tool "${unknownToolName}" is unavailable: the MCP server "${serverHit}" is awaiting approval, so its tools are not loaded. Approve it (run /mcp) to use this tool.`; + default: + // Configured and admitted — a genuine "tool not found". + return `Tool "${unknownToolName}" not found on MCP server "${serverHit}". The server may be disconnected, still starting up, or the tool was renamed.`; + } + } + /** Suggests similar tool names using Levenshtein distance. */ private getToolSuggestion(unknownToolName: string, topN = 3): string { const allToolNames = this.toolRegistry.getAllToolNames(); diff --git a/packages/core/src/tools/mcp-client-manager.test.ts b/packages/core/src/tools/mcp-client-manager.test.ts index 0d635b1067..bb395b7e39 100644 --- a/packages/core/src/tools/mcp-client-manager.test.ts +++ b/packages/core/src/tools/mcp-client-manager.test.ts @@ -47,8 +47,8 @@ function mkManager( isTrustedFolder: () => true, getMcpServers: () => ({}), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, getSessionId: () => 'sid-1', @@ -93,8 +93,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ srv: {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, getSessionId: () => 'sid-1', @@ -156,8 +156,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ srvA: {}, srvB: {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, getSessionId: () => 'sid-1', @@ -175,6 +175,108 @@ describe('McpClientManager', () => { expect(acquireSpy).toHaveBeenCalledTimes(2); }); + it('pool path skips a gated server pending approval — no acquire, no spawn (#4615, sub-task 3)', async () => { + // Trust boundary: with a shared pool, discovery routes through the pool + // path. Pre-fix it only checked `isMcpServerDisabled`, so a hot-reload + // adding a pending `.mcp.json`/workspace server would acquire a connection + // (spawning the process) BEFORE the user approved it. The legacy + // single-session path already skips pending; the pool path must match. + const acquireSpy = vi.fn(); + const fakePool = { + acquire: acquireSpy, + releaseSession: vi.fn(), + getBudget: vi.fn().mockReturnValue(undefined), + } as unknown as import('./mcp-transport-pool.js').McpTransportPool; + const mockConfig = { + isTrustedFolder: () => true, + getMcpServers: () => ({ gated: {}, ok: {} }), + getMcpServerCommand: () => undefined, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), + getWorkspaceContext: () => ({}), + getDebugMode: () => false, + getSessionId: () => 'sid-1', + isMcpServerDisabled: () => false, + isMcpServerPendingApproval: (name: string) => name === 'gated', + } as unknown as Config; + const manager = mkManager({ + config: mockConfig, + options: { pool: fakePool }, + }); + await manager.discoverAllMcpTools(mockConfig); + // `ok` is acquired; `gated` is NOT. + expect(acquireSpy).toHaveBeenCalledTimes(1); + expect(acquireSpy).toHaveBeenCalledWith( + 'ok', + {}, + 'sid-1', + expect.anything(), + expect.anything(), + expect.anything(), + ); + expect(McpClient).not.toHaveBeenCalled(); + }); + + it('removeRuntimeMcpServer drops the server prompts and resources (leak regression, sub-task 3)', async () => { + const removePromptsByServer = vi.fn(); + const removeResourcesByServer = vi.fn(); + const removeMcpToolsByServer = vi.fn(); + const mockConfig = { + isTrustedFolder: () => true, + getMcpServers: () => ({}), + getMcpServerCommand: () => undefined, + getPromptRegistry: () => ({ removePromptsByServer }), + getResourceRegistry: () => ({ removeResourcesByServer }), + getWorkspaceContext: () => ({}), + getDebugMode: () => false, + getSessionId: () => 'sid-1', + isMcpServerDisabled: () => false, + getSettingsMcpServers: () => ({}), + removeRuntimeMcpServer: () => true, + } as unknown as Config; + const manager = mkManager({ + config: mockConfig, + toolRegistry: { removeMcpToolsByServer } as unknown as ToolRegistry, + }); + + await manager.removeRuntimeMcpServer('srv', 'client-1'); + + expect(removeMcpToolsByServer).toHaveBeenCalledWith('srv'); + expect(removePromptsByServer).toHaveBeenCalledWith('srv'); + expect(removeResourcesByServer).toHaveBeenCalledWith('srv'); + }); + + it('removeServer (config-driven removal) drops the server prompts and resources (leak regression, sub-task 3)', async () => { + const removePromptsByServer = vi.fn(); + const removeResourcesByServer = vi.fn(); + const removeMcpToolsByServer = vi.fn(); + const mockConfig = { + isTrustedFolder: () => true, + getMcpServers: () => ({}), + getMcpServerCommand: () => undefined, + getPromptRegistry: () => ({ removePromptsByServer }), + getResourceRegistry: () => ({ removeResourcesByServer }), + getWorkspaceContext: () => ({}), + getDebugMode: () => false, + getSessionId: () => 'sid-1', + isMcpServerDisabled: () => false, + } as unknown as Config; + const manager = mkManager({ + config: mockConfig, + toolRegistry: { removeMcpToolsByServer } as unknown as ToolRegistry, + }); + + // `removeServer` is private; exercised here directly (the incremental + // reconcile's removal branch calls it). + await ( + manager as unknown as { removeServer(name: string): Promise } + ).removeServer('srv'); + + expect(removeMcpToolsByServer).toHaveBeenCalledWith('srv'); + expect(removePromptsByServer).toHaveBeenCalledWith('srv'); + expect(removeResourcesByServer).toHaveBeenCalledWith('srv'); + }); + it('stop() awaits in-flight pool discovery before releasing pool connections (W94/W108/W112)', async () => { // Pre-W94 fix: stop() called releaseAllPooledConnections() while // discoverAllMcpToolsViaPool was still mid-flight; the in-flight @@ -211,8 +313,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ srv: {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, getSessionId: () => 'sid-1', @@ -270,8 +372,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({}), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, getSessionId: () => 'sid-1', @@ -316,8 +418,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({}), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, getSessionId: () => 'sid-1', @@ -369,8 +471,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ srv: {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, getSessionId: () => 'sid-1', @@ -428,8 +530,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ srv: {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, getSessionId: () => 'sid-1', @@ -461,8 +563,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ srv: {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, getSessionId: () => 'sid-1', @@ -503,8 +605,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ srv: {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, getSessionId: () => 'sid-1', @@ -564,8 +666,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ srv: {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, getSessionId: () => 'sid-1', @@ -631,8 +733,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ srv: {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, getSessionId: () => 'sid-1', @@ -681,8 +783,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ srv: {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, getSessionId: () => 'sid-1', @@ -723,8 +825,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ srv: {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -749,8 +851,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ 'test-server': {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -783,8 +885,8 @@ describe('McpClientManager', () => { 'without-instructions': {}, }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -812,8 +914,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => false, getMcpServers: () => ({ 'test-server': {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -838,8 +940,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => false, getMcpServers: () => ({ 'test-server': {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -868,8 +970,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ 'pending-server': { scope: 'project' } }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -898,8 +1000,8 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ 'approved-server': { scope: 'project' } }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -930,7 +1032,9 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ 'test-server': {}, 'another-server': {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -966,7 +1070,9 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ 'test-server': {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -998,7 +1104,9 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ 'test-server': {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; @@ -1038,7 +1146,9 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ 'test-server': {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; @@ -1098,7 +1208,9 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ 'test-server': {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; @@ -1167,7 +1279,9 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ 'test-server': {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; @@ -1234,7 +1348,9 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ 'test-server': {} }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; @@ -1283,7 +1399,9 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({}), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, } as unknown as Config; @@ -1320,7 +1438,9 @@ describe('McpClientManager', () => { broken: { command: 'node', args: [], discoveryTimeoutMs: 50 }, }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -1374,7 +1494,9 @@ describe('McpClientManager', () => { disabled: { command: 'node', args: [] }, }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, isMcpServerDisabled: (name: string) => name === 'disabled', @@ -1417,7 +1539,9 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ foo: { command: 'node', args: [] } }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, isMcpServerDisabled: (name: string) => name === 'foo' && disabled, @@ -1445,6 +1569,185 @@ describe('McpClientManager', () => { expect(mockedMcpClient.connect).toHaveBeenCalledTimes(1); }); + it('discoverAllMcpToolsIncremental reconnects a still-connected server when its config fingerprint changes', async () => { + // Single-session reconcile parity with the pool path's `desiredIds` + // diff: editing a live server's config at runtime (here `args`) must + // tear down the stale connection and reconnect with the new config — + // otherwise the server keeps running on the old command/env/url. + // Without fingerprint tracking, an already-connected server fell into + // the no-op else branch and the edit was silently ignored. + const { MCPServerStatus } = await import('./mcp-client.js'); + const mockedMcpClient = { + connect: vi.fn().mockResolvedValue(undefined), + discover: vi.fn().mockResolvedValue(undefined), + disconnect: vi.fn().mockResolvedValue(undefined), + getStatus: vi.fn().mockReturnValue(MCPServerStatus.CONNECTED), + }; + vi.mocked(McpClient).mockReturnValue( + mockedMcpClient as unknown as McpClient, + ); + + const removePromptsByServer = vi.fn(); + const removeResourcesByServer = vi.fn(); + const removeMcpToolsByServer = vi.fn(); + const toolRegistryStub = { + removeMcpToolsByServer, + } as unknown as ToolRegistry; + let args: string[] = []; + const mockConfig = { + isTrustedFolder: () => true, + getMcpServers: () => ({ foo: { command: 'node', args } }), + getMcpServerCommand: () => undefined, + getPromptRegistry: () => + ({ removePromptsByServer }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer }), + getWorkspaceContext: () => ({}) as WorkspaceContext, + getDebugMode: () => false, + isMcpServerDisabled: () => false, + } as unknown as Config; + const manager = mkManager({ + config: mockConfig, + toolRegistry: toolRegistryStub, + }); + + // First pass: connects and records the fingerprint of `args: []`. + await manager.discoverAllMcpToolsIncremental(mockConfig); + expect(mockedMcpClient.connect).toHaveBeenCalledTimes(1); + expect(mockedMcpClient.disconnect).not.toHaveBeenCalled(); + + // Re-running with an identical config must NOT churn the connection + // (fingerprint unchanged → no-op). + await manager.discoverAllMcpToolsIncremental(mockConfig); + expect(mockedMcpClient.connect).toHaveBeenCalledTimes(1); + expect(mockedMcpClient.disconnect).not.toHaveBeenCalled(); + + // Now change the config in place. The fingerprint differs, so the + // still-connected server is disconnected and reconnected. + removeMcpToolsByServer.mockClear(); + removePromptsByServer.mockClear(); + removeResourcesByServer.mockClear(); + args = ['--flag']; + await manager.discoverAllMcpToolsIncremental(mockConfig); + expect(mockedMcpClient.disconnect).toHaveBeenCalledTimes(1); + expect(mockedMcpClient.connect).toHaveBeenCalledTimes(2); + // The OLD config's tools/prompts/resources MUST be purged before + // rediscovery, so a changed server that drops/renames entries doesn't leave + // stale ones registered against the now-closed client. + expect(removeMcpToolsByServer).toHaveBeenCalledWith('foo'); + expect(removePromptsByServer).toHaveBeenCalledWith('foo'); + expect(removeResourcesByServer).toHaveBeenCalledWith('foo'); + }); + + it('reconnects a still-connected server when only a discovery filter (includeTools) changes', async () => { + // trust / includeTools / excludeTools are excluded from connectionIdOf + // (transport identity), but they ARE applied during discover() and baked + // into the registered tools. The single-session reconcile must therefore + // reconnect when they change — otherwise the edit is silently ignored until + // a manual reconnect/restart. + const { MCPServerStatus } = await import('./mcp-client.js'); + const mockedMcpClient = { + connect: vi.fn().mockResolvedValue(undefined), + discover: vi.fn().mockResolvedValue(undefined), + disconnect: vi.fn().mockResolvedValue(undefined), + getStatus: vi.fn().mockReturnValue(MCPServerStatus.CONNECTED), + }; + vi.mocked(McpClient).mockReturnValue( + mockedMcpClient as unknown as McpClient, + ); + + let includeTools: string[] | undefined = undefined; + const mockConfig = { + isTrustedFolder: () => true, + // command/args/env unchanged across passes → transport fingerprint stays + // identical; only the per-session filter changes. + getMcpServers: () => ({ foo: { command: 'node', includeTools } }), + getMcpServerCommand: () => undefined, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getWorkspaceContext: () => ({}) as WorkspaceContext, + getDebugMode: () => false, + isMcpServerDisabled: () => false, + } as unknown as Config; + const manager = mkManager({ config: mockConfig }); + + await manager.discoverAllMcpToolsIncremental(mockConfig); + expect(mockedMcpClient.connect).toHaveBeenCalledTimes(1); + + // Identical config → no churn. + await manager.discoverAllMcpToolsIncremental(mockConfig); + expect(mockedMcpClient.connect).toHaveBeenCalledTimes(1); + expect(mockedMcpClient.disconnect).not.toHaveBeenCalled(); + + // Change ONLY includeTools — connectionIdOf is unchanged, but the + // discovery-aware key differs → reconnect so discover() re-applies it. + includeTools = ['allowed_tool']; + await manager.discoverAllMcpToolsIncremental(mockConfig); + expect(mockedMcpClient.disconnect).toHaveBeenCalledTimes(1); + expect(mockedMcpClient.connect).toHaveBeenCalledTimes(2); + }); + + it('reconnects a server first connected via the bulk path when its config later changes', async () => { + // Regression: the bulk `discoverAllMcpTools` path (reached via legacy + // blocking boot + extension reload) used to connect WITHOUT recording a + // fingerprint. A subsequent `discoverAllMcpToolsIncremental` then saw an + // `undefined` fingerprint on the still-connected server, short-circuited + // the reconcile guard, and silently dropped the edit — the server kept + // running stale config. Both connect paths must record the fingerprint so + // the invariant the guard relies on actually holds. + const { MCPServerStatus } = await import('./mcp-client.js'); + const mockedMcpClient = { + connect: vi.fn().mockResolvedValue(undefined), + discover: vi.fn().mockResolvedValue(undefined), + disconnect: vi.fn().mockResolvedValue(undefined), + getStatus: vi.fn().mockReturnValue(MCPServerStatus.CONNECTED), + }; + vi.mocked(McpClient).mockReturnValue( + mockedMcpClient as unknown as McpClient, + ); + + const removePromptsByServer = vi.fn(); + const removeResourcesByServer = vi.fn(); + const removeMcpToolsByServer = vi.fn(); + const toolRegistryStub = { + removeMcpToolsByServer, + } as unknown as ToolRegistry; + let args: string[] = []; + const mockConfig = { + isTrustedFolder: () => true, + getMcpServers: () => ({ foo: { command: 'node', args } }), + getMcpServerCommand: () => undefined, + getPromptRegistry: () => + ({ removePromptsByServer }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer }), + getWorkspaceContext: () => ({}) as WorkspaceContext, + getDebugMode: () => false, + isMcpServerDisabled: () => false, + } as unknown as Config; + const manager = mkManager({ + config: mockConfig, + toolRegistry: toolRegistryStub, + }); + + // First connect via the BULK path (not the incremental path) — this is the + // path that previously left the fingerprint unset. + await manager.discoverAllMcpTools(mockConfig); + expect(mockedMcpClient.connect).toHaveBeenCalledTimes(1); + expect(mockedMcpClient.disconnect).not.toHaveBeenCalled(); + + // Now edit the config in place and run the incremental reconcile. With the + // fingerprint recorded by the bulk path, the change is detected and the + // server is torn down + reconnected with the new config; without the fix + // the edit would be silently ignored (connect stays at 1). + args = ['--flag']; + await manager.discoverAllMcpToolsIncremental(mockConfig); + expect(mockedMcpClient.disconnect).toHaveBeenCalledTimes(1); + expect(mockedMcpClient.connect).toHaveBeenCalledTimes(2); + expect(removeMcpToolsByServer).toHaveBeenCalledWith('foo'); + expect(removePromptsByServer).toHaveBeenCalledWith('foo'); + expect(removeResourcesByServer).toHaveBeenCalledWith('foo'); + }); + it('discoverAllMcpToolsIncremental records `failed` outcome for swallowed connect errors', async () => { // `discoverMcpToolsForServerInternal` catches connect/discover errors // without re-throwing (best-effort semantics — one broken server @@ -1475,7 +1778,9 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ 'broken-auth': { command: 'node', args: [] } }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -1532,7 +1837,9 @@ describe('McpClientManager', () => { huge: { command: 'node', args: [], discoveryTimeoutMs: 10_000_000 }, }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -1587,7 +1894,9 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ wsServer: { tcp: 'ws://example.test' } }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -1638,7 +1947,9 @@ describe('McpClientManager', () => { slow: { command: 'node', args: [], discoveryTimeoutMs: 100 }, }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -1692,7 +2003,9 @@ describe('McpClientManager', () => { slow: { command: 'node', args: [], discoveryTimeoutMs: 100 }, }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -1758,7 +2071,9 @@ describe('McpClientManager', () => { isTrustedFolder: () => true, getMcpServers: () => ({ srv: { command: 'node', args: [] } }), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -1822,7 +2137,9 @@ describe('McpClientManager — PR 14 guardrails', () => { isTrustedFolder: () => true, getMcpServers: () => servers, getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -2151,7 +2468,9 @@ describe('McpClientManager — PR 14 guardrails', () => { isTrustedFolder: () => true, getMcpServers: () => mcpServers, getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -2292,6 +2611,63 @@ describe('McpClientManager — PR 14 guardrails', () => { expect(getResourceCalled).toBe(false); }); + it('reconnects a server first connected via a readResource lazy spawn when its config later changes', async () => { + // Regression: the lazy-connect path in `readResource` used to connect + // WITHOUT recording a fingerprint, so a server first brought up by a + // resource read would fall into the reconcile guard's `undefined` + // short-circuit and silently ignore a later in-place config edit. + const { MCPServerStatus } = await import('./mcp-client.js'); + let status: unknown = MCPServerStatus.DISCONNECTED; + const mockedMcpClient = { + connect: vi.fn().mockImplementation(async () => { + status = MCPServerStatus.CONNECTED; + }), + discover: vi.fn().mockResolvedValue(undefined), + disconnect: vi.fn().mockResolvedValue(undefined), + getStatus: vi.fn(() => status), + readResource: vi.fn().mockResolvedValue({ contents: [] }), + }; + vi.mocked(McpClient).mockReturnValue( + mockedMcpClient as unknown as McpClient, + ); + + const removePromptsByServer = vi.fn(); + const removeResourcesByServer = vi.fn(); + const removeMcpToolsByServer = vi.fn(); + let args: string[] = []; + const mockConfig = { + isTrustedFolder: () => true, + getMcpServers: () => ({ foo: { command: 'node', args } }), + getMcpServerCommand: () => undefined, + getPromptRegistry: () => + ({ removePromptsByServer }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer }), + getWorkspaceContext: () => ({}) as WorkspaceContext, + getDebugMode: () => false, + isMcpServerDisabled: () => false, + } as unknown as Config; + const manager = mkManager({ + config: mockConfig, + toolRegistry: { removeMcpToolsByServer } as unknown as ToolRegistry, + }); + + // First bring the server up via a lazy resource read (not discovery). + await manager.readResource('foo', 'mcp://foo/doc'); + expect(mockedMcpClient.connect).toHaveBeenCalledTimes(1); + expect(mockedMcpClient.disconnect).not.toHaveBeenCalled(); + + // Edit the config in place + reconcile. The fingerprint recorded by the + // lazy spawn lets the incremental pass detect the change and reconnect; + // without the fix the edit would be silently dropped (connect stays 1). + args = ['--flag']; + await manager.discoverAllMcpToolsIncremental(mockConfig); + expect(mockedMcpClient.disconnect).toHaveBeenCalledTimes(1); + expect(mockedMcpClient.connect).toHaveBeenCalledTimes(2); + expect(removeMcpToolsByServer).toHaveBeenCalledWith('foo'); + expect(removePromptsByServer).toHaveBeenCalledWith('foo'); + expect(removeResourcesByServer).toHaveBeenCalledWith('foo'); + }); + it('readBudgetFromEnv downgrades enforce-without-budget to off (wenshao S4)', async () => { process.env['QWEN_SERVE_MCP_BUDGET_MODE'] = 'enforce'; // No QWEN_SERVE_MCP_CLIENT_BUDGET — silently fail-open pre-fix: @@ -2904,7 +3280,9 @@ describe('McpClientManager — PR 14b push events + hysteresis', () => { isTrustedFolder: () => true, getMcpServers: () => servers, getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}) as PromptRegistry, + getPromptRegistry: () => + ({ removePromptsByServer: vi.fn() }) as unknown as PromptRegistry, + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), getWorkspaceContext: () => ({}) as WorkspaceContext, getDebugMode: () => false, isMcpServerDisabled: () => false, @@ -3430,8 +3808,8 @@ describe('McpClientManager — addRuntimeMcpServer / removeRuntimeMcpServer (T2. isTrustedFolder: () => true, getMcpServers: () => ({}), getMcpServerCommand: () => undefined, - getPromptRegistry: () => ({}), - getResourceRegistry: () => ({}), + getResourceRegistry: () => ({ removeResourcesByServer: vi.fn() }), + getPromptRegistry: () => ({ removePromptsByServer: vi.fn() }), getWorkspaceContext: () => ({}), getDebugMode: () => false, getSessionId: () => 'test-session-1', diff --git a/packages/core/src/tools/mcp-client-manager.ts b/packages/core/src/tools/mcp-client-manager.ts index c882e7294c..4ff7d6dc31 100644 --- a/packages/core/src/tools/mcp-client-manager.ts +++ b/packages/core/src/tools/mcp-client-manager.ts @@ -390,6 +390,20 @@ export class McpClientManager { private isReconnecting: Map = new Map(); private serverDiscoveryPromises: Map> = new Map(); + /** + * Per connected server, the single-session "connected config key" of the + * config it was last connected with — see {@link singleSessionConnectedKeyOf}. + * Single-session path only; pool mode tracks transport identity via + * `pooledConnections[].id` and its own `desiredIds` diff. + * Lets `discoverAllMcpToolsIncremental` detect an in-place config change to an + * already-connected server and reconnect it, instead of leaving it on the + * stale config. Unlike the transport-only `connectionIdOf`, this key also + * covers the discovery-time filters (trust / includeTools / excludeTools) so + * editing those re-applies them. Set on successful connect; cleared on every + * teardown path so a stale key can't mask a later change. + */ + private readonly connectedConfigKeys = new Map(); + /** * Budget bookkeeping. Slots are reserved synchronously by server name * inside the discovery loop BEFORE any `await client.connect()`, so @@ -1102,6 +1116,17 @@ export class McpClientManager { try { await client.connect(); await client.discover(cliConfig); + // Record the fingerprint of the config this client connected with + // so a later `discoverAllMcpToolsIncremental` can detect an + // in-place config change. The single-session reconcile guard + // skips a still-connected server whose fingerprint is `undefined`, + // so a bulk connect (this path is reached via legacy blocking boot + // + extension reload) that omitted this would silently drop a + // subsequent edit — same invariant the per-server path upholds. + this.connectedConfigKeys.set( + name, + this.singleSessionConnectedKeyOf(name, config), + ); this.eventEmitter?.emit('mcp-client-update', this.clients); } catch (error) { // zombie slot leak. @@ -1327,6 +1352,18 @@ export class McpClientManager { ); } finally { this.clients.delete(serverName); + // Purge the OLD config's tools/prompts before rediscovery. `discover()` + // only adds/overwrites by name and never purges, and `disconnect()` + // doesn't touch the registries — so when this path reconnects a server + // whose config CHANGED (the incremental fingerprint-diff branch), any + // tool the new config drops or renames would otherwise linger, + // selectable by the model but bound to the now-closed client. Clearing + // here also keeps the failure path clean (a rediscovery that throws + // leaves no stale entries behind). Mirrors `removeServer` / + // `addRuntimeMcpServer`'s replace branch. Same-config reconnects + // (`/mcp reconnect`, health monitor, OAuth) simply re-register the + // identical set immediately after. + this.purgeServerRegistries(serverName); this.eventEmitter?.emit('mcp-client-update', this.clients); } } @@ -1352,6 +1389,14 @@ export class McpClientManager { try { await client.connect(); await client.discover(cliConfig); + // Record the connected-config key of the config this client is now + // connected with, so the incremental reconcile can detect a later + // in-place config change and reconnect (mirrors the pool path's `conn.id` + // tracking, plus the discovery filters — see singleSessionConnectedKeyOf). + this.connectedConfigKeys.set( + serverName, + this.singleSessionConnectedKeyOf(serverName, serverConfig), + ); // a server that // was refused at a previous discovery pass and is now // successfully (re)connected via this path (e.g. `/mcp @@ -1408,6 +1453,7 @@ export class McpClientManager { } this.releaseSlotName(serverName); this.clients.delete(serverName); + this.connectedConfigKeys.delete(serverName); } // Log the error but don't throw: callers expect best-effort discovery. debugLogger.error( @@ -1527,6 +1573,13 @@ export class McpClientManager { const desiredIds = new Map(); for (const [name, config] of Object.entries(servers)) { if (cliConfig.isMcpServerDisabled(name)) continue; + // Trust boundary (#4615): a gated `.mcp.json`/workspace server pending + // user approval must not be "desired" — otherwise the release loop + // below would keep (or a hot-reload would acquire) a pool connection + // before the user approves. The legacy path already skips pending; the + // pool path must match. Optional-chain is defensive for the daemon + // surface where `isMcpServerPendingApproval` may be absent. + if (cliConfig.isMcpServerPendingApproval?.(name)) continue; if (isSdkMcpServerConfig(config)) continue; desiredIds.set(name, connectionIdOf(name, config)); } @@ -1553,6 +1606,15 @@ export class McpClientManager { ); return; } + // Trust boundary (#4615): never acquire a connection / spawn a + // process for a gated server still pending approval. Mirrors the + // legacy single-session path and the `desiredIds` filter above. + if (cliConfig.isMcpServerPendingApproval?.(name)) { + debugLogger.debug( + `Skipping pending-approval MCP server (pool mode): ${name}`, + ); + return; + } // SDK MCP servers MUST // stay on the legacy McpClientManager path because their // `sendSdkMcpMessage` callback is bound per-session in this @@ -1783,6 +1845,7 @@ export class McpClientManager { await Promise.all(disconnectionPromises); this.clients.clear(); + this.connectedConfigKeys.clear(); this.consecutiveFailures.clear(); this.isReconnecting.clear(); this.serverDiscoveryPromises.clear(); @@ -1829,6 +1892,7 @@ export class McpClientManager { ); } finally { this.clients.delete(serverName); + this.connectedConfigKeys.delete(serverName); this.consecutiveFailures.delete(serverName); this.isReconnecting.delete(serverName); this.serverDiscoveryPromises.delete(serverName); @@ -2149,9 +2213,28 @@ export class McpClientManager { ) { // Disconnected server, try to reconnect serversToUpdate.push(name); + } else { + // Still-connected server: detect an in-place config change + // (command / url / env / headers / oauth, plus the discovery filters + // trust / includeTools / excludeTools) by comparing the connected- + // config key it was connected with against the desired one. This is + // the single-session equivalent of the pool path's `desiredIds` diff + // — without it, editing a live server's config at runtime would leave + // it running on the stale config. `connectedConfigKeys` is set on + // every successful connect, so a CONNECTED client without a recorded + // key is not expected; guard against `undefined` anyway to avoid a + // spurious reconnect of a healthy server. + // `discoverMcpToolsForServerInternal` disconnects the stale client + // before reconnecting with the freshly-read config, so pushing the + // name is sufficient — no explicit teardown needed here. + const currentKey = this.connectedConfigKeys.get(name); + if ( + currentKey !== undefined && + currentKey !== this.singleSessionConnectedKeyOf(name, servers[name]) + ) { + serversToUpdate.push(name); + } } - // Note: Configuration change detection would require comparing - // the old and new config, which is not implemented here } // Update only the servers that need it. Each per-server discover is @@ -2284,10 +2367,12 @@ export class McpClientManager { ); } } - // Drop any tools that registered during the disconnect window. No-op - // if the server hadn't reached `discover()` yet, so it's safe to - // always call. - this.toolRegistry.removeMcpToolsByServer(serverName); + // Drop any tools/prompts/resources that registered during the + // disconnect window. No-op if the server hadn't reached `discover()` + // yet, so it's safe to always call. A server that registered prompts / + // resources but stalled `tools/list` past the timeout would otherwise + // leak them bound to the closed transport. + this.purgeServerRegistries(serverName); // Prevent the discovery `finally` block's `startHealthCheck` from // resurrecting this server: without removing the client entry, // `performHealthCheck` would observe `status !== CONNECTED` for @@ -2300,6 +2385,7 @@ export class McpClientManager { // absent, so the trailing `finally`-block call becomes a no-op. this.stopHealthCheck(serverName); this.clients.delete(serverName); + this.connectedConfigKeys.delete(serverName); // Release the budget slot ONLY if THIS in-flight // discoverMcpToolsForServerInternal call freshly reserved // it. `freshReservations.has(serverName)` distinguishes: @@ -2387,6 +2473,43 @@ export class McpClientManager { return isRemote ? 5_000 : 30_000; } + /** + * The single-session reconnect key for a server config. `connectionIdOf` is + * intentionally transport-only (it excludes the per-session discovery filters + * so the shared pool can reuse a transport across sessions with different + * filters). But the single-session reconcile must ALSO reconnect when only a + * discovery filter changes — `trust`, `includeTools`, `excludeTools` are + * applied during `discover()` and baked into the registered tools, so a + * config edit to them otherwise never takes effect mid-session. Append a + * stable hash of those fields (arrays sorted so order alone doesn't churn). + */ + private singleSessionConnectedKeyOf( + serverName: string, + config: MCPServerConfig, + ): string { + const discovery = JSON.stringify({ + trust: config.trust ?? null, + includeTools: [...(config.includeTools ?? [])].sort(), + excludeTools: [...(config.excludeTools ?? [])].sort(), + }); + return `${connectionIdOf(serverName, config)}|${discovery}`; + } + + /** + * Purge a server's entries from all three registries (tools + prompts + + * resources). Every teardown path must clean all three atomically — a missed + * registry leaves stale entries bound to a closed client (selectable by the + * model, or surfaced by `listMcpResources`). Centralized here so adding a + * future registry is one edit, not a hunt across teardown sites. Callers keep + * their own transport disconnect / map deletes / health-check / status / + * budget handling — only the registry purge is shared. + */ + private purgeServerRegistries(serverName: string): void { + this.toolRegistry.removeMcpToolsByServer(serverName); + this.cliConfig.getPromptRegistry().removePromptsByServer(serverName); + this.cliConfig.getResourceRegistry().removeResourcesByServer(serverName); + } + /** * Removes a server and its tools */ @@ -2404,6 +2527,7 @@ export class McpClientManager { this.stopHealthCheck(serverName); this.consecutiveFailures.delete(serverName); } + this.connectedConfigKeys.delete(serverName); // server gone from config (or disabled mid-session) releases // the budget slot too — operator intent is "this server should not @@ -2417,8 +2541,11 @@ export class McpClientManager { // last-pass startup refusal record. this.dropRefusalEntry(serverName); - // Remove tools for this server from registry - this.toolRegistry.removeMcpToolsByServer(serverName); + // Remove tools, prompts and resources for this server. Unlike + // `ToolRegistry.disconnectServer`, this config-driven removal path never + // cleaned up the prompt/resource registries, so a removed/changed server + // leaked them across a hot-reload. + this.purgeServerRegistries(serverName); // The server has been removed from configuration, so drop it from the // global status registry too — the health pill should no longer count it. @@ -2647,6 +2774,15 @@ export class McpClientManager { ]).finally(() => { if (timeoutId) clearTimeout(timeoutId); }); + // Record the fingerprint of the config this client connected with so a + // later `discoverAllMcpToolsIncremental` can detect an in-place config + // change. The reconcile guard skips a still-connected server whose + // key is `undefined`; without this, a server first brought up by a lazy + // resource read would silently ignore a subsequent edit. + this.connectedConfigKeys.set( + serverName, + this.singleSessionConnectedKeyOf(serverName, serverConfig), + ); // start // the health monitor on a successful lazy spawn. Pre-fix // a lazy-spawned server that later disconnected (crash, @@ -2817,7 +2953,7 @@ export class McpClientManager { /* best effort */ } this.pooledConnections.delete(name); - this.toolRegistry.removeMcpToolsByServer(name); + this.purgeServerRegistries(name); this.stopHealthCheck(name); } const existingClient = this.clients.get(name); @@ -2829,7 +2965,8 @@ export class McpClientManager { /* best effort */ } this.clients.delete(name); - this.toolRegistry.removeMcpToolsByServer(name); + this.connectedConfigKeys.delete(name); + this.purgeServerRegistries(name); // Do NOT releaseSlotName here — the budget slot carries over to // the new entry being spawned. Releasing + not re-reserving would // leave the running server unaccounted in the budget. @@ -2876,6 +3013,10 @@ export class McpClientManager { this.eventEmitter?.emit('mcp-client-update', this.clients); await client.connect(); await client.discover(this.cliConfig); + this.connectedConfigKeys.set( + name, + this.singleSessionConnectedKeyOf(name, config), + ); this.eventEmitter?.emit('mcp-client-update', this.clients); toolCount = this.toolRegistry.getToolsByServer(name).length; } @@ -2887,8 +3028,9 @@ export class McpClientManager { } else if (this.budgetMode !== 'off') { this.releaseSlotName(name); } - // Clean up any partial state (including tools from partial discover) - this.toolRegistry.removeMcpToolsByServer(name); + // Clean up any partial state (including tools/prompts/resources from a + // partial discover) so a failed runtime add leaves nothing behind. + this.purgeServerRegistries(name); this.pooledConnections.delete(name); removeMCPServerStatus(name); const failedClient = this.clients.get(name); @@ -2900,6 +3042,7 @@ export class McpClientManager { } } this.clients.delete(name); + this.connectedConfigKeys.delete(name); this.stopHealthCheck(name); this.eventEmitter?.emit('mcp-client-update', this.clients); @@ -2976,12 +3119,14 @@ export class McpClientManager { this.eventEmitter?.emit('mcp-client-update', this.clients); } - // Cleanup: tool registry, status, health check, diagnostics (mirrors removeServer) - this.toolRegistry.removeMcpToolsByServer(name); + // Cleanup: tool registry, prompts, resources, status, health check, + // diagnostics (mirrors removeServer) + this.purgeServerRegistries(name); removeMCPServerStatus(name); this.stopHealthCheck(name); this.consecutiveFailures.delete(name); this.isReconnecting.delete(name); + this.connectedConfigKeys.delete(name); this.dropRefusalEntry(name); // Release budget slot diff --git a/packages/core/src/tools/mcp-client.ts b/packages/core/src/tools/mcp-client.ts index c19dc67edb..6ce63c81db 100644 --- a/packages/core/src/tools/mcp-client.ts +++ b/packages/core/src/tools/mcp-client.ts @@ -332,7 +332,14 @@ export class McpClient { // it ahead of `updateStatus` guarantees the field is populated // by the time the listener fires. this.lastTransportError = error; - debugLogger.error(`MCP ERROR (${this.serverName}):`, error.toString()); + // Use getErrorMessage (not `error.toString()`) so the underlying + // syscall behind opaque wrappers like undici's `TypeError: fetch + // failed` (e.g. `ECONNREFUSED`, a proxy `502`) is surfaced instead of + // a bare "fetch failed". Critical for diagnosing local-server / + // proxy connectivity issues. + debugLogger.error( + `MCP ERROR (${this.serverName}): ${getErrorMessage(error)}`, + ); this.updateStatus(MCPServerStatus.DISCONNECTED); }; diff --git a/packages/core/src/utils/errors.test.ts b/packages/core/src/utils/errors.test.ts index c896142ec6..36a098ca6a 100644 --- a/packages/core/src/utils/errors.test.ts +++ b/packages/core/src/utils/errors.test.ts @@ -5,7 +5,44 @@ */ import { describe, it, expect } from 'vitest'; -import { isAbortError, isNodeError } from './errors.js'; +import { getErrorMessage, isAbortError, isNodeError } from './errors.js'; + +describe('getErrorMessage cause unwrapping', () => { + it('returns the plain message when there is no cause', () => { + expect(getErrorMessage(new Error('boom'))).toBe('boom'); + }); + + it('surfaces an undici-style fetch-failed AggregateError cause (ECONNREFUSED)', () => { + // undici "TypeError: fetch failed" wraps an AggregateError whose own + // message is empty; the useful detail lives in `.errors[].code`. + const inner = Object.assign( + new Error('connect ECONNREFUSED 127.0.0.1:29900'), + { code: 'ECONNREFUSED' }, + ); + const agg = new AggregateError([inner]); // message === '' + const err = new TypeError('fetch failed', { cause: agg }); + + const msg = getErrorMessage(err); + expect(msg).toContain('fetch failed'); + expect(msg).toContain('ECONNREFUSED'); + }); + + it('surfaces a single Error cause that has a code but empty message', () => { + const cause = Object.assign(new Error(''), { code: 'ECONNREFUSED' }); + const err = new TypeError('fetch failed', { cause }); + expect(getErrorMessage(err)).toBe('fetch failed (cause: ECONNREFUSED)'); + }); + + it('keeps the existing behavior for a cause with a meaningful message', () => { + const err = new Error('outer', { cause: new Error('inner detail') }); + expect(getErrorMessage(err)).toBe('outer (cause: inner detail)'); + }); + + it('does not append a redundant cause equal to the message', () => { + const err = new Error('same', { cause: new Error('same') }); + expect(getErrorMessage(err)).toBe('same'); + }); +}); describe('isAbortError', () => { it('should return true for DOMException-style AbortError', () => { diff --git a/packages/core/src/utils/errors.ts b/packages/core/src/utils/errors.ts index 1791b478c5..114aa8d291 100644 --- a/packages/core/src/utils/errors.ts +++ b/packages/core/src/utils/errors.ts @@ -36,11 +36,55 @@ export function isAbortError(error: unknown): boolean { return false; } +/** + * Best-effort one-line description of an error's `cause`, used to surface the + * underlying syscall behind opaque wrappers like undici's `TypeError: fetch + * failed` (whose own message carries nothing). Returns `undefined` when there + * is no useful detail. + * + * Handles three shapes: + * - `AggregateError` (undici retries multiple addresses, e.g. IPv6 `::1` then + * IPv4 `127.0.0.1`): its own `message` is empty, so unwrap `.errors[]`. + * - a plain `Error` with a Node `code` (e.g. `ECONNREFUSED`) but possibly an + * empty message — prefer `code`, combine with message when both add signal. + * - any other value — stringify. + */ +function describeErrorCause(cause: unknown): string | undefined { + if (cause == null) return undefined; + if (cause instanceof AggregateError && Array.isArray(cause.errors)) { + const inner = cause.errors + .map((e) => describeSingleError(e)) + .filter((s): s is string => Boolean(s)); + if (inner.length > 0) { + return [...new Set(inner)].join('; '); + } + } + return describeSingleError(cause); +} + +function describeSingleError(err: unknown): string | undefined { + if (err instanceof Error) { + const code = (err as { code?: unknown }).code; + const codeStr = typeof code === 'string' ? code : undefined; + const msg = err.message?.trim(); + if (msg && codeStr && !msg.includes(codeStr)) { + return `${codeStr}: ${msg}`; + } + return msg || codeStr || (err.name !== 'Error' ? err.name : undefined); + } + if (err && typeof err === 'object' && 'code' in err) { + const code = (err as { code?: unknown }).code; + if (typeof code === 'string' && code) return code; + } + const str = String(err); + return str && str !== '[object Object]' ? str : undefined; +} + export function getErrorMessage(error: unknown): string { if (error instanceof Error) { - const cause = error.cause; - if (cause instanceof Error && cause.message !== error.message) { - return `${error.message} (cause: ${cause.message})`; + const detail = describeErrorCause(error.cause); + if (detail && detail !== error.message) { + return `${error.message} (cause: ${detail})`; } return error.message; }