feat(serve): approval / tools / init / MCP-restart mutation routes (#4175 Wave 4 PR 17)#4282
Merged
Conversation
Contributor
📋 Review SummaryThis PR (#4282) implements Wave 4 PR 17 from issue #4175, adding four strict-gated mutation control routes to 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds four strict-gated mutation control routes to qwen serve (session approval-mode change, workspace tool toggle, workspace QWEN.md init, MCP server restart) so remote clients can modify daemon runtime posture. Includes a new core TrustGateError typed exception, a new disabledTools Config field that gates ToolRegistry.register*, five new typed daemon events with SDK reducer + guards, four new SDK methods, and supporting docs / capability tags.
Changes:
- Core:
TrustGateErrorclass +Config.disabledToolsskip-register set wired throughToolRegistry. - Serve bridge & routes: 4 new HTTP routes,
broadcastWorkspaceEventhelper,WorkspaceInitConflictError,persistApprovalMode/persistDisabledToolscallbacks, ACP control extMethods +McpClientManager.isServerDiscovering. - SDK / docs:
DAEMON_APPROVAL_MODES, 5 typed events + reducer state, 4DaemonClientmethods, capability tags, protocol doc section, drift-detector test.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/config/config.ts | Adds TrustGateError, disabledTools set + getter; thrown by trust gate. |
| packages/core/src/tools/tool-registry.ts | Skip-register gate consulting Config.getDisabledTools(). |
| packages/core/src/tools/tool-registry.test.ts | Tests for disabled-tool skip + next-spawn semantics. |
| packages/core/src/tools/mcp-client-manager.ts | Public isServerDiscovering helper. |
| packages/cli/src/config/config.ts | Wires settings.tools.disabled into ConfigParameters.disabledTools. |
| packages/cli/src/config/settingsSchema.ts | New tools.disabled settings entry. |
| packages/cli/src/serve/server.ts | 4 new HTTP routes + TrustGateError / WorkspaceInitConflictError mapping in sendBridgeError. |
| packages/cli/src/serve/httpAcpBridge.ts | Bridge implementations of the 4 mutation routes, broadcastWorkspaceEvent, persist callbacks, WorkspaceInitConflictError. |
| packages/cli/src/serve/runQwenServe.ts | Wires production persistApprovalMode / persistDisabledTools callbacks. |
| packages/cli/src/serve/status.ts | Adds SERVE_CONTROL_EXT_METHODS + maps TrustGateError.name to auth_env_error. |
| packages/cli/src/serve/capabilities.ts | Advertises 4 new capability tags. |
| packages/cli/src/serve/index.ts | Re-exports new symbols. |
| packages/cli/src/serve/server.test.ts | Route tests + fake bridge extensions. |
| packages/cli/src/serve/httpAcpBridge.test.ts | Bridge tests for tool toggle / init. |
| packages/cli/src/serve/status.test.ts | Tests TrustGateError → auth_env_error classification. |
| packages/cli/src/acp-integration/acpAgent.ts | ACP extMethod handlers for approval-mode and MCP restart. |
| packages/cli/src/acp-integration/approvalMode.test.ts | New drift detector test for the 3-source approval-mode contract. |
| packages/sdk-typescript/src/daemon/DaemonClient.ts | 4 new HTTP helpers. |
| packages/sdk-typescript/src/daemon/types.ts | 4 new result types + DAEMON_APPROVAL_MODES tuple. |
| packages/sdk-typescript/src/daemon/events.ts | 5 new event types, guards, reducer state, reducer cases. |
| packages/sdk-typescript/src/daemon/index.ts | Re-exports of new types/values. |
| packages/sdk-typescript/src/index.ts | Top-level re-exports. |
| packages/sdk-typescript/test/unit/DaemonClient.test.ts | Tests for the 4 new client helpers. |
| integration-tests/cli/qwen-serve-routes.test.ts | Extends EXPECTED_STAGE1_FEATURES mirror with 4 new + 3 backfilled tags. |
| docs/users/qwen-serve.md | User-facing bullet for the 4 routes. |
| docs/developers/qwen-serve-protocol.md | ~150-line protocol section for the mutation surface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wenshao
reviewed
May 18, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Overall: structure, layering (route → bridge → ACP child), capability advertising, and the new TrustGateError cross-bundle handling are well thought out. But the PR has one CI blocker and three correctness issues that I think should land before this merges. Details inline; medium/low items I'd be fine deferring to a follow-up.
Blockers
- CI is red on Lint + Test (mac/linux/windows) because
packages/cli/src/acp-integration/approvalMode.test.tsimports@qwen-code/sdk, but the CLI package has no such dependency and the tsconfig has no path mapping for it. The drift detector belongs inpackages/sdk-typescript/test/unit/where the SDK→core dependency already exists — that also makes the test exercise a meaningful cross-package contract.
Correctness (high)
persistDisabledToolsreads UNION-merged settings and writes the result back into the workspace scope, baking entries from higher scopes (User/System) into the workspace file on the first toggle.setSessionApprovalModewithpersist: truesilently returnspersisted: false(HTTP 200) when nopersistApprovalModecallback is wired — inconsistent withsetWorkspaceToolEnabled, which throws clearly in the same situation./workspace/initoverwrites a whitespace-onlyQWEN.mdwithaction: 'created', destroying the user's content withoutforce: true.
API shape (medium)
clientIdparameter position is inconsistent across the four newDaemonClientmutation helpers — once this ships in the SDK the shape is hard to walk back.
Aside: PR description claims npm run typecheck --workspace packages/cli — clean, which contradicts the CI typecheck failure. Likely a stale local dist/symlink masked the missing dep — worth a clean re-verification.
No Copilot-flagged items omitted — overlapping observations are noted on the inline threads. Nice work on the rest; the soft-refusal-as-200 contract, runtime guards for every new event, and the name-based TrustGateError cross-bundle pattern are all good calls.
wenshao
requested changes
May 18, 2026
doudouOUC
added a commit
that referenced
this pull request
May 18, 2026
Addresses 5 critical / 4 high / 2 medium items from #4282 review. CI blocker (wenshao H1) - Move `approvalMode.test.ts` from `packages/cli/src/acp-integration/` to `packages/sdk-typescript/test/unit/approval-mode-drift.test.ts`. The CLI package has no `@qwen-code/sdk` dep and the tsconfig has no path mapping for it, so `tsc --build` failed `Cannot find module '@qwen-code/sdk'` on Lint + Test (mac/linux/windows). The SDK package is the right host: it already depends on `@qwen-code/qwen-code-core`, and the test pins the SDK ↔ core contract directly. Also drop the tautological `APPROVAL_MODES contains every ApprovalMode enum value` check — `APPROVAL_MODES` is defined as `Object.values(ApprovalMode)` in core, so that assertion can never fire. Critical (gpt-5.5 via wenshao /review) - C1 (`initWorkspace` path traversal): `getCurrentGeminiMdFilename()` is settings-controlled. A daemon configured with `context.fileName: "../outside.md"` could resolve outside `boundWorkspace` and let this strict-gated mutation create or truncate a file outside the workspace boundary. Resolve and verify the joined path stays within `boundWorkspace`; reject otherwise. - C2 (`X-Qwen-Client-Id` forgery): the 3 workspace mutation routes (`/workspace/init`, `/workspace/tools/:name/enable`, `/workspace/mcp/:server/restart`) accepted any syntactically valid client id and stamped it onto fan-out events without checking `bridge.knownClientIds()`. Mirrors the inline validation pattern PR 16 already uses for `/workspace/memory` and `/workspace/agents`. Add `parseAndValidateWorkspaceClientId` shared helper in `server.ts` (collapses with PR 16's pattern when the Wave-4-wide DRY refactor lands). - C3 (MCP restart budget under-count): the pre-check used `accounting.total >= budget`, but enforce-mode capacity is reserved by `tryReserveSlot` via `reservedSlots` (which counts configured + in-flight + disconnected slot holders). `total` only counts CONNECTED, so a restart on a budget-saturated workspace passed the pre-check while the manager refused internally and the route reported `restarted: true`. Mirror the manager's policy by checking `reservedSlots.length`. - C4 (false `restarted: true` on broken MCP): `discoverMcpToolsForServer` catches reconnect/discovery errors internally (logs and resolves void), so the route reported `restarted: true` while the server stayed disconnected. After the call, verify the live `getMCPServerStatus(name)` is `MCPServerStatus.CONNECTED`; throw a structured JSON-RPC error otherwise. New typed bridge error `McpServerRestartFailedError` → HTTP 502 with `errorKind: 'protocol_error'`. - C5 (unknown MCP server falls through as 500): the agent-side `RequestError.resourceNotFound` was not specially handled by `sendBridgeError`, so a typo in the server name returned 500 indistinguishable from an internal daemon failure. Re-raise with structured `data.errorKind: 'mcp_server_not_found'`; bridge re-instantiates as `McpServerNotFoundError`; route maps to a stable 404 with `code: 'mcp_server_not_found'` and `serverName` in the body. High (wenshao) - H2 (`persistDisabledTools` scope leak): the callback read `fresh.merged.tools?.disabled` (UNION across System / SystemDefaults / User / Workspace) and wrote the result back into `SettingScope.Workspace`, copying entries from higher scopes into the workspace file on the first toggle. Subsequent removals at the originating scope (e.g. User) would no longer take effect. Read from the WORKSPACE-scope `LoadedSettings` only via `fresh.forScope(SettingScope.Workspace).settings.tools?.disabled`. - H3 (silent persist no-op): `setSessionApprovalMode` with `persist: true` returned HTTP 200 + `persisted: false` when no `persistApprovalMode` callback was wired, indistinguishable from "hook ran but failed" or genuine `persisted: true`. Throw asymmetrically with the sibling `setWorkspaceToolEnabled` (which already throws in the same situation). - H4 (whitespace-only init clobber): `/workspace/init` overwrote a whitespace-only `QWEN.md` with `action: 'created'` despite `force` not being passed, destroying the user's whitespace content (template, half-written init, intentional newline) without a signal. Treat existing-and-whitespace-only as a no-op; return `action: 'noop'` and skip the write. Adds `'noop'` to the discriminator union on `DaemonInitWorkspaceResult` and the `workspace_initialized` event payload. Medium - M1 (SDK `clientId` position consistency): the four new mutation helpers placed `clientId` inconsistently (4th vs 3rd vs 2nd). Fold `clientId` into the trailing options bag for all four. Matches the existing `context: { clientId }` argument the bridge layer already uses internally; reduces caller boilerplate for callers that always stamp clientId for audit. - M2 (dead `instanceof String` branch): drop the no-op `instanceof String` clause in `setSessionApprovalMode`'s wire-error reconstruction — `Error.message` is always a primitive string. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text Report |
Summary
Adds 4 strict-gated mutation control routes to
qwen serveso remote TUI / channels / web / IDE clients can change a daemon's runtime posture without touching the host CLI:POST /session/:id/approval-mode— switch a live session's approval mode (plan/default/auto-edit/yolo); optionalpersist: truealso writes to workspace settingsPOST /workspace/tools/:name/enable— toggle a tool name intools.disabled(skip-register, distinct frompermissions.deny)POST /workspace/init— scaffold an emptyQWEN.md(mechanical only — does NOT call the model; clients that want AI-fill follow up withPOST /session/:id/prompt)POST /workspace/mcp/:server/restart— restart a single MCP server with a PR 14 v1 budget pre-checkAll four are strict-gated by the PR 15 mutation gate (
401 token_requiredon no-token loopback defaults), accept theX-Qwen-Client-Idaudit header (PR 7), and emitoriginatorClientId-stamped SSE events.Closes the Wave 4 PR 17 deliverable from #4175. All three dependencies (PR 12 ✅ / PR 14 v1 ✅ / PR 15 ✅) are on main.
Why
Wave 3 read-only routes let remote clients see daemon state; Wave 4 mutation routes let them change it. PR 17 lands the next four of those mutation routes — narrow and well-scoped — with two cross-cutting hardening pieces:
TrustGateErrortyped class in core, so the bridge can map untrusted-folder rejection toerrorKind: 'auth_env_error'(PR 13 taxonomy) without regex-matching message textdisabledToolsworkspace setting, a new skip-register primitive in core'sToolRegistrythat's distinct from the existingpermissions.deny(which keeps the tool registered and rejects invocation). Both built-ins and MCP-discovered tools flow throughToolRegistry.registerTool, so gating there covers every registration pathWhat's in each commit
489fcd7abfeat(core): introduce TrustGateError for setApprovalModemapDomainErrorToErrorKindrecognizes viaerr.name(notinstanceof, to survive cross-package bundling)c48439e00feat(core): add disabledTools workspace settingConfig.disabledToolsSet + ToolRegistry skip-register gate; CLI wiressettings.tools.disabled(UNION merge across scopes)9f243f478feat(serve): add session approval-mode mutation routeapproval_mode_changedevent + drift detector. Wire-level trust-gate translation: ACP child throwsRequestErrorwithdata.errorKind: 'trust_gate', bridge re-instantiatesTrustGateError, route emits 403 +auth_env_errorb7fd92077feat(serve): add workspace tool toggle routebroadcastWorkspaceEventhelper +tool_toggledevent. Unknown tool names accepted (forward-looking — pre-disable a not-yet-installed MCP tool); next-spawn semantics documented18b08b9a5feat(serve): add workspace init routeWorkspaceInitConflictError+workspace_initializedevent. Whitespace-only target treated as absent (matches local/initslash command)f383ef3e5feat(serve): add MCP server restart route with budget guardMcpClientManager.isServerDiscovering+ soft-skip decision tree (in_flight / disabled / budget_would_exceed) + 2 new SSE events. Soft refusals are 200 OK with structured reason; hard errors (unknown server, no live ACP child) escalate to 4xx12a19b77edocs(serve): mutation control routes protocol sectionStrict invariants enforced
mutate({ strict: true })— no-token loopback callers get401 {code: 'token_required'}instead of silent passthroughX-Qwen-Client-Id— audit chain from PR 7 propagates into every emitted event viaoriginatorClientIdtool_toggled/workspace_initialized/mcp_server_restarted/mcp_server_restart_refusedare workspace-scoped (fan-out to every live session SSE bus);approval_mode_changedis session-scoped (mode change is local to one session'sConfig)TrustGateErrorasRequestError(-32003, msg, {errorKind: 'trust_gate'}); bridge detects the structureddata.errorKindand re-instantiatesTrustGateErrorso the route'ssendBridgeErrorhandler can matchinstanceofand return 403 +errorKind: 'auth_env_error'asKnownDaemonEventruntime guards added for all 5 new events — without this the events would be silently filed asunrecognizedKnownEventCountin the SDK reducer instead of reaching the typed reducer cases. Pinned byisApprovalModeChangedData/isToolToggledData/isWorkspaceInitializedData/isMcpServerRestartedData/isMcpServerRestartRefusedDatavalidatorsdisabledTools"next-spawn" semantics — pinned by a regression test that registers a tool, builds a freshConfigwith the tool disabled, registers again, and asserts the new registry skips while the old one is unaffectedTest plan
npm run typecheck --workspace packages/core— cleannpm run typecheck --workspace packages/cli— cleannpm run typecheck --workspace packages/sdk-typescript— cleannpx vitest run packages/cli/src/serve/ packages/cli/src/acp-integration/ packages/sdk-typescript/test/unit packages/core/src/tools/tool-registry.test.ts packages/core/src/config/config.test.ts— 1474/1474 passed across 45 filesEngineering principles checklist
KnownDaemonEventunion extends; older SDKs see new events asunknown)qwen serveStage 1 routes / SDK behavior preservedApprovalMode; runtime guards for every new typed event)Coordination notes
broadcastWorkspaceEventhelper naming: Mirrors PR 21 feat(serve): auth device-flow route (#4175 Wave 4 PR 21) #4255's planned name. PR 16 (feat(serve): workspace memory and agents CRUD (#4175 Wave 4 PR 16) #4249, merged 2026-05-18) shipped a siblingpublishWorkspaceEventhelper for memory/agent events; the post-merge fold-in to consolidate the two onto a single helper is tracked as a Wave 4 follow-upworkspace_memory/workspace_agents(PR 16) andworkspace_file_read(PR 19) intointegration-tests/cli/qwen-serve-routes.test.ts. Those tags exist inserver.test.ts'sEXPECTED_STAGE1_FEATURESon origin/main but were missed in the E2E mirror; folded in here so the integration test stays green/tools enable|disableslash command (PR 17 is daemon-only); follow-updisabledToolstoggling for live sessions without restart (current "next-spawn" semantics is the documented contract)🤖 Generated with Qwen Code