feat(serve): MCP client guardrails (#4175 Wave 3 PR 14)#4247
Merged
Conversation
Adds an in-process MCP client counter, slot-reservation enforcement at all 3 spawn sites (discoverAllMcpTools / discoverAllMcpToolsIncremental / readResource), new `--mcp-client-budget=N` + `--mcp-budget-mode={enforce,warn,off}` CLI flags forwarded to the ACP child via env, and additive `clientCount` / `clientBudget` / `budgetMode` / `budgets[]` fields plus `disabledReason: 'budget'` tagging on `GET /workspace/mcp`.
Always-on capability tag `mcp_guardrails` with `modes: ['warn', 'enforce']` so SDK clients can pre-flight refusal semantics. Typed SSE push events (`mcp_budget_warning` / `mcp_child_refused_batch`) intentionally deferred to a small follow-up PR — the snapshot already exposes `budgets[0].status: 'warning'|'error'` + `refusedCount` so operator visibility isn't blocked.
Contributor
📋 Review SummaryThis PR implements MCP client budget guardrails to prevent per-workspace MCP fan-out issues by introducing a counter-based slot reservation system with three enforcement modes ( 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds MCP client guardrails for qwen serve, introducing per-workspace budget configuration, accounting, enforcement/warning modes, serve capability metadata, status payload extensions, SDK type mirrors, tests, and documentation.
Changes:
- Adds MCP budget mode/config/accounting and refusal handling in
McpClientManager. - Adds serve CLI flags, capability tag,
/workspace/mcpbudget fields, and ACP status mapping. - Updates SDK types, protocol docs, user docs, and tests for the new guardrail surface.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
packages/sdk-typescript/src/daemon/types.ts |
Mirrors new MCP budget/status fields in SDK daemon types. |
packages/core/src/tools/mcp-client-manager.ts |
Implements MCP client accounting, slot reservation, budget modes, and refusal tracking. |
packages/core/src/tools/mcp-client-manager.test.ts |
Adds unit coverage for budget accounting and enforcement behavior. |
packages/cli/src/serve/types.ts |
Adds serve option types for MCP budget flags. |
packages/cli/src/serve/status.ts |
Extends serve MCP status shapes with budget fields and idle defaults. |
packages/cli/src/serve/server.test.ts |
Tests capability metadata and route passthrough for new MCP budget fields. |
packages/cli/src/serve/runQwenServe.ts |
Forwards MCP budget settings to the ACP child environment. |
packages/cli/src/serve/capabilities.ts |
Registers the always-on mcp_guardrails capability descriptor. |
packages/cli/src/commands/serve.ts |
Adds CLI parsing, validation, and logging for MCP budget flags. |
packages/cli/src/acp-integration/acpAgent.ts |
Builds /workspace/mcp budget cells and budget-refused per-server status. |
docs/users/qwen-serve.md |
Documents user-facing MCP budget flags and behavior. |
docs/developers/qwen-serve-protocol.md |
Documents protocol capability and /workspace/mcp budget payload extensions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Summary
Adds an in-process MCP client counter, slot-reservation enforcement at all 3 spawn sites (
discoverAllMcpTools/discoverAllMcpToolsIncremental/readResourcelazy spawn), new--mcp-client-budget=N+--mcp-budget-mode={enforce,warn,off}CLI flags forwarded to the ACP child via env vars, additiveclientCount/clientBudget/budgetMode/budgets[]fields plusdisabledReason: 'budget'tagging onGET /workspace/mcp, and always-on capability tagmcp_guardrailswithmodes: ['warn', 'enforce'].A workspace declaring 30 MCP servers in
mcpServersno longer starts 30 clients per session with zero operator control — set--mcp-client-budget=10 --mcp-budget-mode=warnto measure first, flip toenforceonce telemetry confirms the cap is right.Round 3 review (@wenshao) caught a critical scope-naming bug. The actual semantics of PR 14 v1 are per-session, not per-workspace:
acpAgent.newSessionConfig()constructs a freshConfig+ToolRegistry+McpClientManagerfor every ACP session, and each manager independently readsQWEN_SERVE_MCP_CLIENT_BUDGET. So--mcp-client-budget=10with 5 concurrent sessions caps at 5 × 10 = 50 live MCP clients across the daemon, NOT 10.Pragmatic v1 path adopted (alternative to a workspace-shared-manager refactor): rewrite docs + protocol surface to be honest about per-session semantics. The snapshot cell now emits
scope: 'session'(widened type accepts'workspace'for future PR 23). Wave 5 PR 23 (shared MCP pool) will introduce a workspace-scoped manager and addscope: 'workspace'cells for true cross-session aggregation. PR 14 v1 is the in-process counter + soft enforcement foundation that PR 23 builds on.Scope
This PR (v1) ships the foundation: counter, slot reservation, three-mode enforcement, snapshot extensions, capability tag, SDK type mirror, telemetry hook (
recordStartupEvent('mcp_budget_decision', ...)), stderr boot breadcrumb mirroring PR 15's--require-authstyle, full docs.Deferred to PR 14b (small follow-up): typed SSE push events
mcp_budget_warning(hysteresis 75% / 37.5% like PR 10'sslow_client_warning) +mcp_child_refused_batch(one coalesced event per discovery pass). Needs a new workspace-level ACP notification kind (child→daemon→fan-out to all session buses) which is its own protocol decision. The snapshot in v1 already exposesbudgets[0].status: 'warning'|'error'+refusedCount+ per-serverdisabledReason: 'budget', so operator visibility isn't blocked.Design highlights
packages/core/src/tools/mcp-client-manager.ts— source of truth, not a daemon-side polled cache (avoids races against the multi-emissionmcp-client-updateevent). Bridge reads via the existingqwen/status/workspace/mcpACP ext-method.Set<string>— survives reconnect /runWithDiscoveryTimeoutdeletion / health-monitor restart. Released only onremoveServer(config-gone / mid-session disable) anddisconnectServer(explicit operator intent). Reconnect's internalclient.disconnect()doesn't release.await client.connect()—Promise.all(discoveryPromises)cannot interleave a second connect past the cap at any microtask boundary.enforcemode refuses;warnmode over-reserves so accounting reflects the configured set (operators seeliveCount > budgetin the snapshot).offis pure observability — no reservation.subprocessCount = stdio + websocket(excludessdkin-process,sse,http) — the value PR 1'spgrep -Pbaseline harness can cross-check against (cross-check itself ships with PR 14b).Object.entries(mcpServers)). Forward-compat note in protocol doc: if qwen-code adopts a scope hierarchy later (claude-code usesplugin < user < project < local), refusal switches to lowest-precedence-first.Prior art (cited in protocol doc + this PR description)
filterMcpServersByPolicyreturningblocked: string[](src/services/mcp/config.ts) — same shape as our refusal surface so operators already recognize the wire format.--mcp-*flag family (--mcp-config,--mcp-debug,--strict-mcp-config) — naming consistent with--mcp-client-budget. Orthogonal to claude-code'sMCP_SERVER_CONNECTION_BATCH_SIZE(concurrency cap, not total cap).packages/opencode/src/cli/heap.ts— single-thresholdarmedboolean hysteresis primitive (the PR 14b dual-threshold version evolves it).stdioCount,sseCount,httpCountontengu_mcp_server_connection_succeeded).Engineering principles checklist
offwhen budget unset; old clients ignoremcp_guardrails+ new payload fields.--mcp-client-budget. Even when set, default mode iswarn(no refusal).warnships first;enforceopt-in this PR; default flip from warn→enforce is a future PR after operator telemetry.McpClientManagerreverts to no-counter, no-budget; no schema breaks; env-var passthrough becomes no-op.Review history (4 rounds)
597f011e6ba3e3febddiscoverAllMcpTools+readResource(Critical),readBudgetFromEnvenforce-without-budget fail-open, dedupmcp_budget_decisiontelemetry,BudgetExhaustedError.liveCount→reservedCountrename, accounting-failure log-level bump2b1836a4ereadResourcemissingisMcpServerDisabledgate, workspacerefusedCountfilter for disabled-after-refused, extractMCP_BUDGET_WARN_FRACTIONconstant, doc clarifications ontcp/createTransportdivergence + slot-retention design + TOCTOU invariantb4f7b2c23discoverMcpToolsForServerInternal(closes 3 sibling threads with one weReserved fix)Still open (intentionally):
tcptransport mapper vscreateTransport(pre-existing core inconsistency; cc @wenshao for direction on (a) implement WS in createTransport vs (b) droptcpfrom MCPServerConfig)reservedSlots[]on the wire for slot-leak debugging (additive, deferred to PR 14b alongside the typed event surface)Engineering principles checklist
offwhen budget unset; old clients ignoremcp_guardrails+ new payload fields.--mcp-client-budget. Even when set, default mode iswarn(no refusal).warnships first;enforceopt-in this PR; default flip from warn→enforce is a future PR after operator telemetry.McpClientManagerreverts to no-counter, no-budget; no schema breaks; env-var passthrough becomes no-op.Test plan
npm run typecheck— clean across 4 workspaces (cli, core, sdk-typescript, webui)npx eslinton touched files — cleannpx vitest run— 631/631 focused tests pass across 20 files (44 core mcp-client-manager + 151 serve + 50 acpAgent + rest)qwen serve --mcp-client-budget=2 --mcp-budget-mode=warnagainst 5-server fixture → per-sessionclientCount: 5,budgets[0].scope: 'session',status: 'warning'; restart--mcp-budget-mode=enforce→ 2 connect, 3 servers taggeddisabledReason: 'budget',budgets[0].status: 'error'withrefusedCount: 3🤖 Generated with Qwen Code