mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-07-09 17:19:02 +00:00
feat(mcp): reconcile MCP servers live on settings change (#5561)
* feat(mcp): reconcile MCP servers live on settings change Hot-reload MCP servers when settings.json changes (issue #3696 sub-task 3): editing mcpServers / mcp.allowed / mcp.excluded now connects, disconnects, or restarts only the affected servers in place, without restarting the session or losing conversation context. - Part A: Config runtime setters + reinitializeMcpServers incremental reconcile; align the shared-pool path with the #4615 pending-approval gate - Part B: SettingsWatcher subscriber (hotReload.ts), gated on a mcpServers + gating-list diff; flip the three MCP schema keys to hot-reloadable - Part D: re-fire the approval modal for a gated server left pending by an edit - Part E: /mcp shows why a gated server was skipped (pending / rejected) - Record connection fingerprints on the bulk and lazy-connect paths so an edit to a server first connected via those paths is not silently dropped - Design doc (en/zh) incl. the admission-stance boundary clarification Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> # Conflicts: # packages/cli/src/config/settingsSchema.test.ts # Conflicts: # packages/cli/src/ui/components/mcp/steps/ServerDetailStep.tsx # Conflicts: # packages/cli/src/gemini.tsx * feat(mcp): reconcile MCP servers live on settings change Hot-reload MCP servers when settings.json changes (issue #3696 sub-task 3): editing mcpServers / mcp.allowed / mcp.excluded now connects, disconnects, or restarts only the affected servers in place, without restarting the session or losing conversation context. - Part A: Config runtime setters + reinitializeMcpServers incremental reconcile; align the shared-pool path with the #4615 pending-approval gate - Part B: SettingsWatcher subscriber (hotReload.ts), gated on a mcpServers + gating-list diff; flip the three MCP schema keys to hot-reloadable - Part D: re-fire the approval modal for a gated server left pending by an edit - Part E: /mcp shows why a gated server was skipped (pending / rejected) - Record connection fingerprints on the bulk and lazy-connect paths so an edit to a server first connected via those paths is not silently dropped - Design doc (en/zh) incl. the admission-stance boundary clarification Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(mcp): harden hot-reload teardown and reconcile (review follow-ups) Address reviewer findings on the MCP hot-reload changes: - Extract purgeServerRegistries() and use it at every teardown path, fixing the discovery-timeout handler which leaked prompts/resources (only tools were purged) for a server that stalled tools/list past the timeout. - Surface reconcile failures via AppEvent.LogError so a failed settings edit is visible to the user, not just under --debug. - Make a single-session config edit to a discovery filter (trust / includeTools / excludeTools) reconnect the server so discover() re-applies it: connectionIdOf stays transport-only; add singleSessionConnectedKeyOf and rename connectionFingerprints -> connectedConfigKeys. - Make a coalesced reinitializeMcpServers await the in-flight pass + its drain (store mcpReconcilePromise) so the caller no longer emits approval events / logs "complete" before its change is applied; coalesced callers share the failure. - Assert removeResourcesByServer in the fingerprint-change tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(mcp): bound hot-reload MCP admission and explain why servers are unavailable (#3696) (#5561) - K: treat the startup --allowed-mcp-server-names flag as an immutable upper bound — a runtime settings edit may narrow MCP admission within it but never widen beyond it; with no flag, settings fully drive admission. - H: preserve an explicit `mcp.allowed: []` as deny-all (don't collapse to undefined / allow-all), matching boot semantics, and make mcpGatingEqual distinguish absent (allow-all) from [] (deny-all) so the change reconciles. - B: classify why an MCP server is unavailable (removed / not_allowed / excluded / pending_approval) and route the tool-not-found message to the right recovery action; track removals against the gating-independent merged map (dropping the prev-effective snapshot param). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(mcp): document hot-reload admission bound, deny-all, and unavailable reasons (Part F) Reflect the K/H/B changes in the sub-task 3 design doc: add Part F (CLI --allowed-mcp-server-names as an immutable upper bound, mcp.allowed: [] as deny-all, and getMcpServerUnavailableReason routing the tool-not-found message), and fix the now-superseded "settings can widen beyond the startup allowlist" admission-stance note and verification item 11. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(serve): pre-approve gated MCP servers in daemon baseline harness The pool/daemon discovery path now honors #4615 pending-approval gating, so the workspace-scoped MCP servers the amplification suite declares in .qwen/settings.json are skipped as pending and never spawn (the suite timed out waiting for grandchildren). Add approveWorkspaceMcpServers() to the harness (keyed by the realpath workspace to match the daemon's canonicalized --workspace) and pre-approve the fixtures before boot, mirroring simple-mcp-server.test.ts. --------- Co-authored-by: heyang.why <heyang.why@alibaba-inc.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Heyang Wang
GitHub
parent
ecdf470bb6
commit
1344f34147
41 changed files with 3351 additions and 121 deletions
666
docs/design/hot-reload/mcp-runtime-reinitialization.md
Normal file
666
docs/design/hot-reload/mcp-runtime-reinitialization.md
Normal file
|
|
@ -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<string, MCPServerConfig> | 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<string, MCPServerConfig> | undefined): Promise<void> {
|
||||
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<string, MCPServerConfig> | 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.
|
||||
|
|
@ -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<string, MCPServerConfig>,
|
||||
): Record<string, string> {
|
||||
const approvalsPath = path.join(workspaceCwd, '.qwen', 'mcpApprovals.json');
|
||||
const project: Record<string, { hash: string; status: 'approved' }> = {};
|
||||
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 <pid>`. Returns megabytes (rounded
|
||||
* to 1 decimal). Returns NaN if the process is gone or `ps` errored — call
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
1
package-lock.json
generated
1
package-lock.json
generated
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
465
packages/cli/src/config/hot-reload.test.ts
Normal file
465
packages/cli/src/config/hot-reload.test.ts
Normal file
|
|
@ -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<string, MCPServerConfig> | 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);
|
||||
}
|
||||
});
|
||||
});
|
||||
241
packages/cli/src/config/hot-reload.ts
Normal file
241
packages/cli/src/config/hot-reload.ts
Normal file
|
|
@ -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<string, MCPServerConfig> | undefined,
|
||||
b: Record<string, MCPServerConfig> | 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<string, MCPServerConfig>,
|
||||
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<string, MCPServerConfig> | 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);
|
||||
}
|
||||
});
|
||||
}
|
||||
|
|
@ -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([]);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<string, MCPServerConfig>,
|
||||
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;
|
||||
|
|
|
|||
|
|
@ -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]) => {
|
||||
|
|
|
|||
|
|
@ -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<string, MCPServerConfig>,
|
||||
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,
|
||||
|
|
|
|||
|
|
@ -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', () => {
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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ó',
|
||||
|
|
|
|||
|
|
@ -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',
|
||||
|
|
|
|||
|
|
@ -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',
|
||||
|
|
|
|||
|
|
@ -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':
|
||||
|
|
|
|||
|
|
@ -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':
|
||||
|
|
|
|||
|
|
@ -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',
|
||||
|
|
|
|||
|
|
@ -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: 'Выполняется',
|
||||
|
|
|
|||
|
|
@ -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: '執行中',
|
||||
|
|
|
|||
|
|
@ -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: '运行中',
|
||||
|
|
|
|||
|
|
@ -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<MCPManagementDialogProps> = ({
|
|||
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<MCPManagementDialogProps> = ({
|
|||
(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<MCPManagementDialogProps> = ({
|
|||
isDisabled,
|
||||
hasOAuthTokens,
|
||||
requiresAuth,
|
||||
approvalState,
|
||||
});
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -40,15 +40,20 @@ export const ServerDetailStep: React.FC<ServerDetailStepProps> = ({
|
|||
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<ServerDetailStepProps> = ({
|
|||
});
|
||||
}
|
||||
|
||||
// 只在服务器未禁用且已断开连接时显示"重新连接"选项
|
||||
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<ServerDetailStepProps> = ({
|
|||
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<ServerDetailStepProps> = ({
|
|||
}
|
||||
|
||||
return result;
|
||||
}, [server, onViewResources]);
|
||||
}, [server, onViewResources, awaitingApproval]);
|
||||
|
||||
useKeypress(
|
||||
(key) => {
|
||||
|
|
@ -165,9 +175,13 @@ export const ServerDetailStep: React.FC<ServerDetailStepProps> = ({
|
|||
{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)}
|
||||
</Text>
|
||||
</Box>
|
||||
</Box>
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
<ServerListStep
|
||||
servers={[gatedServer('blocked', 'rejected')]}
|
||||
onSelect={vi.fn()}
|
||||
/>,
|
||||
);
|
||||
expect(lastFrame()).toContain('rejected — edit config to re-approve');
|
||||
});
|
||||
|
||||
it('shows "needs approval" for a pending gated server', () => {
|
||||
const { lastFrame } = render(
|
||||
<ServerListStep
|
||||
servers={[gatedServer('waiting', 'pending')]}
|
||||
onSelect={vi.fn()}
|
||||
/>,
|
||||
);
|
||||
expect(lastFrame()).toContain('needs approval');
|
||||
});
|
||||
|
||||
it('does not show the debug-log hint for an approval-skipped server', () => {
|
||||
const { lastFrame } = render(
|
||||
<ServerListStep
|
||||
servers={[gatedServer('blocked', 'rejected')]}
|
||||
onSelect={vi.fn()}
|
||||
/>,
|
||||
);
|
||||
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(
|
||||
<ServerListStep servers={[failed]} onSelect={vi.fn()} />,
|
||||
);
|
||||
expect(lastFrame()).toContain('see error logs');
|
||||
expect(lastFrame()).not.toContain('needs approval');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -109,14 +109,19 @@ export const ServerListStep: React.FC<ServerListStepProps> = ({
|
|||
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<ServerListStepProps> = ({
|
|||
{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)}
|
||||
</Text>
|
||||
{/* 显示无效工具警告 */}
|
||||
{!!server.invalidToolCount && server.invalidToolCount > 0 && (
|
||||
|
|
@ -176,8 +185,11 @@ export const ServerListStep: React.FC<ServerListStepProps> = ({
|
|||
</Box>
|
||||
))}
|
||||
|
||||
{/* 提示信息 */}
|
||||
{servers.some((s) => s.status === 'disconnected' && !s.isDisabled) && (
|
||||
{/* 提示信息：仅针对"真正连接失败"的 server；被门控跳过（awaiting
|
||||
approval）不是错误，不提示查日志 */}
|
||||
{servers.some(
|
||||
(s) => s.status === 'disconnected' && !s.isDisabled && !s.approvalState,
|
||||
) && (
|
||||
<Box marginTop={1}>
|
||||
<Text color={theme.status.warning}>
|
||||
※ {t('Run qwen --debug to see error logs')}
|
||||
|
|
|
|||
|
|
@ -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';
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -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<string, MCPServerConfig> = {};
|
||||
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.
|
||||
|
|
|
|||
|
|
@ -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<PendingMcpServer[]>([]);
|
||||
|
||||
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);
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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<void>((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<void>((_, 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<void>((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);
|
||||
|
|
|
|||
|
|
@ -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<string, MCPServerConfig>;
|
||||
/**
|
||||
* 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<string, MCPServerConfig>;
|
||||
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<string, MCPServerConfig> | 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<string>();
|
||||
private readonly topTierMcpServers:
|
||||
| Record<string, MCPServerConfig>
|
||||
| undefined;
|
||||
private readonly runtimeMcpServers = new Map<string, MCPServerConfig>();
|
||||
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<void> | 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<string, MCPServerConfig> | 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<string, MCPServerConfig> | 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<string, MCPServerConfig> {
|
||||
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<string, MCPServerConfig> | 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<string, MCPServerConfig> | 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<string, MCPServerConfig> | undefined,
|
||||
): Promise<void> {
|
||||
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
|
||||
|
|
|
|||
|
|
@ -2989,6 +2989,179 @@ describe('CoreToolScheduler', () => {
|
|||
);
|
||||
});
|
||||
|
||||
describe('MCP tool-not-found messaging', () => {
|
||||
const makeScheduler = (opts: {
|
||||
mcpServers?: Record<string, unknown>;
|
||||
removed?: string[];
|
||||
// Per-server admission reason for configured-but-unavailable servers.
|
||||
reasons?: Record<string, 'not_allowed' | 'excluded' | 'pending_approval'>;
|
||||
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
|
||||
|
|
|
|||
|
|
@ -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__<server>__<tool>` 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();
|
||||
|
|
|
|||
|
|
@ -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<void> }
|
||||
).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',
|
||||
|
|
|
|||
|
|
@ -390,6 +390,20 @@ export class McpClientManager {
|
|||
private isReconnecting: Map<string, boolean> = new Map();
|
||||
private serverDiscoveryPromises: Map<string, Promise<void>> = 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<string, string>();
|
||||
|
||||
/**
|
||||
* 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<string, ConnectionId>();
|
||||
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
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
};
|
||||
|
||||
|
|
|
|||
|
|
@ -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', () => {
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue