mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-03 05:14:52 +00:00
0afb48b1ea
* fix(safe-mode): preserve caller-supplied top-tier MCP servers Safe mode is meant to distrust LOCAL/ambient state (settings.json, extensions, project .mcp.json) so a user can isolate which local customization is misbehaving. It was also unconditionally dropping topTierMcpServers -- the caller-supplied servers from an ACP session/new's mcpServers field or --mcp-config -- which are an explicit, per-invocation argument, not ambient local state. The real gate turned out to be in packages/core/src/config/config.ts's Config.getMcpServers() (the accessor mcp-client-manager.ts actually reads for discovery), not just the mcpServers assembly in loadCliConfig -- fixed both so the raw field and the accessor agree. Also guarded the hot-reload path (hot-reload.ts) with the same bare/safe-mode check, so a live settings.json edit can't smuggle local servers into an already-running bare/safe-mode session. Fixes #7819 * docs: clarify safe-mode's MCP servers note distinguishes local vs caller-supplied Per CONTRIBUTING.md guideline 5 (docs for user-facing changes). * fix(safe-mode): apply allowedMcpServers to top-tier servers, skip pendingMcpServers I/O under safe mode Address Copilot's review of PR #7827: - getMcpServers() now runs the safe-mode top-tier map through the same allowedMcpServers filter as the non-safe-mode path -- safe mode is not an exemption from a session's own --allowed-mcp-server-names upper bound (High severity finding, confirmed real). - Re-added safeMode to pendingMcpServers' skip condition in loadCliConfig. Functionally a no-op either way (top-tier servers are never gated, #4615), but skips getMcpApprovals' local file read entirely under safe mode instead of doing a no-op read -- safe mode shouldn't touch local/ambient state at all, not even harmlessly (Medium severity finding). * fix(safe-mode): actually run MCP discovery for surviving top-tier servers Config.getMcpServers() reporting a top-tier server as configured isn't enough on its own -- something has to actually connect to it and register its tools with the model. That's a separate gate in initialize(): startMcpDiscoveryInBackground() was skipped outright whenever isSafeMode() was true, written back when getMcpServers() always returned {} under safe mode (so skipping discovery was a harmless no-op). Left unpatched after the earlier fix, a caller-supplied top-tier server survives getMcpServers() but is never actually discovered/connected -- confirmed live against a real ACP session before this commit: the agent reported the tool as "not configured" even though Config.getMcpServers() already returned it. Found by actually running the fix end-to-end against a live ACP session (qwen --acp --safe-mode + a real stdio MCP fixture server) instead of relying on unit tests of Config in isolation. After this commit the same live session correctly discovers and calls the caller-supplied tool. Checks getMcpServers() (not topTierMcpServers directly) so the allowedMcpServers filter still applies -- no discovery is kicked off if the only top-tier server present is filtered out. * fix(safe-mode): also run MCP discovery for surviving bare-mode top-tier servers The discovery-kickoff gate in Config.initialize() special-cased safe mode (skip only when there's nothing to discover) but left bare mode's half of the same guard unconditional (!this.getBareMode()), even though loadCliConfig feeds top-tier MCP servers into bare mode's mcpServers assembly exactly the way it does safe mode's topTierMcpServers field. A bare-mode session with a caller-supplied server (qwen --bare --mcp-config, or ACP session/new under bare mode) had that server reported as configured by getMcpServers() but never actually connected/discovered — the same stranded-server regression already fixed for safe mode in this PR, just the bare-mode twin of it. Found by an automated review pass on PR #7827 after the safe-mode fix had already landed. Added the same three-case regression coverage (present / nothing supplied / filtered out by allowedMcpServers) mirroring the existing safe-mode tests, using mcpServers (not topTierMcpServers) since bare mode's "local sources dropped" guarantee lives entirely in the CLI-layer assembly, not a core-level short-circuit. * refactor(safe-mode): simplify the bare/safe discovery-gate condition (!bare || has) && (!safe || has) reduces by distributivity to !(bare || safe) || has, so factor the has-servers check into a single hasMcpServers computed once, instead of calling getMcpServers() (which allocates and filters) twice. Suggested by an automated review pass on PR #7827, commit 25ddf8b. No behavior change — same 23 discovery-gate tests pass unmodified. * fix(safe-mode): stop reading settings.mcp.allowed/excluded under safe mode allowedMcpServers/excludedMcpServers assembly in loadCliConfig() only guarded the settings-sourced branch with `!bareMode`, missing `!safeMode` — so a local settings.json mcp.allowed/excluded list (LOCAL/ambient state, same category as settings.mcpServers itself, which safe mode already drops) was still read under safe mode. Combined with getMcpServers()'s own allowedMcpServers filter (added earlier in this PR for the --allowed-mcp-server-names case), a settings.json mcp.allowed list narrower than the caller's own top-tier servers would silently filter them back out — defeating the guarantee this PR exists to provide, via the filter's source rather than the mcpServers map directly. The argv.allowedMcpServerNames branch is unaffected: that's an explicit per-invocation argument, not local state, so it still applies under safe mode same as topTierMcpServers itself. Found by an automated review pass (doudouOUC, CHANGES_REQUESTED) on PR #7827. Regression test confirmed red before the fix (session-supplied server silently filtered out) and green after. Full targeted vitest run (packages/cli config/: 1018 passed, same 3 pre-existing Windows-only extension-file-watcher failures as before, unrelated), tsc --noEmit, eslint, prettier --check all clean. * fix(safe-mode): stop reading settings.mcp.allowed/excluded on hot-reload too Same class of bug as the previous commit's loadCliConfig fix, found by an automated review pass on the SAME PR: recomputeMcpGating (hot-reload.ts) reads settings.merged.mcp.allowed/excluded unconditionally, with no bare/safe guard of its own. registerMcpHotReload's existing bare/safe guard only covered the servers map (`next`), not the admission lists computed right after it — so a live settings.json edit narrowing mcp.allowed during an already-running safe/bare session would flow straight into setAllowedMcpServers and silently filter the caller's top-tier server out of getMcpServers() mid-session. Same stranded-server outcome as the boot-time bug, reached through the gating list's SOURCE instead of the mcpServers map. Fix: under bare/safe mode, skip recomputeMcpGating entirely and build the gating directly from only the CLI --allowed-mcp-server-names bound (explicit, per-invocation, not local state — same treatment as topTierMcpServers itself); excluded/pending are irrelevant once nothing but the never-gated top-tier servers can be present. Regression tests (safe mode + bare mode) confirmed red before the fix (setAllowedMcpServers called with the settings-sourced list) and green after (called with the CLI bound, undefined here). Full targeted vitest run (packages/cli config/: 1020 passed, same 3 pre-existing Windows-only extension-file-watcher failures as before this PR touched anything, unrelated), tsc --noEmit, eslint, prettier --check all clean. * fix(safe-mode): stop reading settings.mcp.allowed/excluded on ACP reload too Third instance of the same bug class found by an automated review pass on this PR: reloadWorkspaceMcpDiscovery (packages/cli/src/acp-integration/ acpAgent.ts) — the ACP control-endpoint reload path (workspaceMcpReload), distinct from registerMcpHotReload's settings-file-watcher path fixed in the previous commit — called assembleMcpServers(settings.merged.mcpServers, ...) and recomputeMcpGating(settings, ...) unconditionally, per live Config in liveConfigs, with no bare/safe guard. A workspaceMcpReload request against an already-running safe/bare session would fold local mcpServers/mcp.allowed/excluded back in, silently stranding or filtering the caller's own top-tier server mid-session — same outcome as the two prior fixes, reached through a third independent reload path. Fix: per-config (liveConfigs holds a Set of potentially differently-moded Configs — the base config, active session configs, and the discovery config), skip assembleMcpServers/recomputeMcpGating under bare/safe mode and build servers/gating directly from that config's own getTopTierMcpServers()/getCliAllowedMcpServerNames() — same treatment as the other two fixes. Regression test (packages/cli/src/acp-integration/acpAgent.test.ts) confirmed red before the fix (settings-sourced 'local' server leaked into reinitializeMcpServers alongside the caller's 'probe') and green after. Also added getBareMode/isSafeMode mocks (defaulting false) to the two pre-existing Config-shaped mocks in this describe block that didn't have them — reloadWorkspaceMcpDiscovery now calls these unconditionally per config, which would otherwise throw "not a function" against any mock missing them, even for a normal-mode test. Full targeted vitest run (packages/cli/src/acp-integration/acpAgent.test.ts: 318 passed), tsc --noEmit, eslint, prettier --check all clean. * test(safe-mode): add bare-mode counterpart for the workspaceMcpReload guard Suggested by an automated review pass on PR #7827: the previous commit's regression test for reloadWorkspaceMcpDiscovery only exercised isSafeMode: true, leaving the bare-mode half of config.getBareMode() || config.isSafeMode() unverified at this layer — unlike the hot-reload.ts tests, which already cover both modes for both the servers map and the admission lists. A future change narrowing that guard to isSafeMode() only would go undetected here. Confirmed red before the fix (temporarily reverted acpAgent.ts to the prior commit): settings-sourced 'local' leaked into reinitializeMcpServers alongside the caller's 'probe', same as the safe-mode case. Green with the fix restored. Full targeted vitest run (acp-integration/acpAgent.test.ts: 319 passed), tsc --noEmit, eslint, prettier --check all clean. |
||
|---|---|---|
| .. | ||
| configuration | ||
| extension | ||
| features | ||
| ide-integration | ||
| reference | ||
| support | ||
| _meta.ts | ||
| common-workflow.md | ||
| integration-github-action.md | ||
| integration-jetbrains.md | ||
| integration-vscode.md | ||
| integration-zed.md | ||
| overview.md | ||
| quickstart.md | ||
| qwen-serve-deploy-local.md | ||
| qwen-serve.md | ||