From 531c5a28c5ac2f35faf8dcb7c797f17b51fd9021 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8F=B6=E5=85=AC?= Date: Fri, 7 Aug 2026 12:10:48 +0800 Subject: [PATCH 1/9] feat(daemon): add batch skill toggle API --- docs/design/daemon-skill-batch-toggle.md | 63 +++++++ .../developers/daemon/13-sdk-daemon-client.md | 13 ++ docs/developers/qwen-serve-protocol.md | 45 ++++- docs/users/qwen-serve.md | 2 +- packages/cli/src/serve/capabilities.ts | 1 + .../src/serve/routes/workspace-skills.test.ts | 124 ++++++++++++- .../cli/src/serve/routes/workspace-skills.ts | 165 ++++++++++++++++++ packages/cli/src/serve/server.test.ts | 1 + .../serve/workspace-qualified-rest.test.ts | 41 +++++ .../sdk-typescript/src/daemon/DaemonClient.ts | 44 +++++ packages/sdk-typescript/src/daemon/index.ts | 3 + packages/sdk-typescript/src/daemon/types.ts | 20 +++ packages/sdk-typescript/src/index.ts | 3 + .../test/unit/DaemonClient.test.ts | 79 +++++++++ 14 files changed, 601 insertions(+), 3 deletions(-) create mode 100644 docs/design/daemon-skill-batch-toggle.md diff --git a/docs/design/daemon-skill-batch-toggle.md b/docs/design/daemon-skill-batch-toggle.md new file mode 100644 index 00000000000..de36ed2d099 --- /dev/null +++ b/docs/design/daemon-skill-batch-toggle.md @@ -0,0 +1,63 @@ +# Daemon Skill batch toggle + +## Problem + +Remote Skill managers can toggle only one Skill per request. Closing several +Skills therefore requires client-side request orchestration and provides no +single response that records all target outcomes. + +## API + +Add collection-level mutation routes: + +- `POST /workspace/skills/enable` +- `POST /workspaces/:workspace/skills/enable` + +The request body is: + +```json +{ + "skillNames": ["review", "deploy"], + "enabled": false +} +``` + +`skillNames` is a non-empty string array with at most 100 entries. Names are +trimmed and deduplicated case-insensitively while preserving first-seen order. +The response is best-effort: valid targets are toggled in order, and failures +for individual targets are returned without preventing later targets from +being attempted. + +```json +{ + "enabled": false, + "results": [ + { + "skillName": "review", + "enabled": false, + "changed": true, + "activation": "applied", + "sessionsRefreshed": 1, + "sessionsFailed": 0 + } + ], + "errors": [ + { + "skillName": "missing", + "code": "skill_not_found", + "error": "Skill not found: missing" + } + ] +} +``` + +Malformed requests still fail as a whole with HTTP 400. Workspace trust, +authentication, client identity, and generation ownership use the same gates +as the single-Skill route. + +## Compatibility + +Advertise `workspace_skill_batch_toggle` separately from +`workspace_skill_toggle`. Clients must pre-flight the new capability before +calling the collection route. The existing single-Skill route and response +remain unchanged. diff --git a/docs/developers/daemon/13-sdk-daemon-client.md b/docs/developers/daemon/13-sdk-daemon-client.md index 7ed73138eb0..3235345ba80 100644 --- a/docs/developers/daemon/13-sdk-daemon-client.md +++ b/docs/developers/daemon/13-sdk-daemon-client.md @@ -154,6 +154,19 @@ await client Pre-flight `capabilities.features.includes('workspace_skill_toggle')`. The typed `DaemonSkillToggleResult` reports the canonical `skillName`, whether disk state `changed`, activation state (`applied`, `deferred`, or `partial`), and refreshed/failed session counts. `DaemonWorkspaceSkillStatus.userInvocable` is an optional false-only field; absence means the skill is user-invocable. +For batch changes, pre-flight `workspace_skill_batch_toggle` and call either client shape with the same contract: + +```ts +await client.setWorkspaceSkillsEnabled(['review', 'deploy'], false, { + clientId: 'dashboard-1', +}); +await client + .workspaceByCwd('/work/secondary') + .setWorkspaceSkillsEnabled(['review', 'deploy'], true); +``` + +`DaemonSkillBatchToggleResult` contains ordered successful `results` and per-target `errors`; one invalid target does not stop later targets. + Workspace display names are optional presentation metadata. Pre-flight `capabilities.features.includes('workspace_display_name')`; workspace ids and canonical paths remain the only selectors, and duplicate display names are valid. ```ts diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index 3ea52f19eea..4b3a2f01a38 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -181,6 +181,7 @@ registry. Clients **must** gate UI off `features`, not off `mode` (per design 'mcp_server_runtime_mutation', 'workspace_file_read', 'workspace_file_bytes', 'workspace_file_write', 'session_approval_mode_control', 'workspace_tool_toggle', 'workspace_skill_toggle', + 'workspace_skill_batch_toggle', 'workspace_settings', 'workspace_init', 'workspace_mcp_restart', 'session_recap', 'session_generation', 'session_btw', 'session_shell_command', 'mcp_workspace_pool', 'mcp_pool_restart', @@ -243,7 +244,7 @@ registry. Clients **must** gate UI off `features`, not off `mode` (per design `session_info` advertises `GET /workspace/:id/session-info` and its `/workspaces/:workspace/session-info` twin. The response aggregates persisted active and archived session counts without hydrating list metadata. It is an explicit O(n) disk scan and must not be polled; clients should treat `truncated: true` as a lower-bound result. -`session_approval_mode_control`, `workspace_tool_toggle`, `workspace_skill_toggle`, `workspace_init`, and `workspace_mcp_restart` advertise the mutation control routes documented below. They are strict-gated by the mutation gate (a daemon configured without a bearer token rejects them with 401 `token_required`). Older daemons return `404`; pre-flight each tag before exposing the corresponding affordance. +`session_approval_mode_control`, `workspace_tool_toggle`, `workspace_skill_toggle`, `workspace_skill_batch_toggle`, `workspace_init`, and `workspace_mcp_restart` advertise the mutation control routes documented below. They are strict-gated by the mutation gate (a daemon configured without a bearer token rejects them with 401 `token_required`). Older daemons return `404`; pre-flight each tag before exposing the corresponding affordance. `mcp_guardrails` (issue [#4175](https://github.com/QwenLM/qwen-code/issues/4175) PR 14) covers the MCP budget surface: the `clientCount` / `clientBudget` / `budgetMode` / `budgets[]` fields on `GET /workspace/mcp`, the `disabledReason` field on per-server cells, and the `--mcp-client-budget` / `--mcp-budget-mode` CLI flags. Older daemons omit the new fields entirely; SDK clients pre-flight this tag before relying on `budgets[]` semantics. The registry descriptor also carries `modes: ['warn', 'enforce']` for future feature-modes exposure — for now, clients infer mode from the snapshot's `budgetMode` field. Server refusal under `enforce` mode is deterministic by `Object.entries(mcpServers)` declaration order; a future scope-precedence layer (if qwen-code adopts one) would shift this to "lowest-precedence first" to mirror claude-code's `plugin < user < project < local` convention. @@ -2671,6 +2672,48 @@ Errors: The mutation reuses the workspace-scoped `settings_changed` event for each changed key (`skills.disabled` and/or `skills.enabled`); it does not add a new event type. Workspace skill status cells include optional `disabledReason: 'hard' | 'default' | 'inactive_extension'` and `lockedScope: 'system' | 'user' | 'systemDefaults'` fields. +#### `POST /workspace/skills/enable` + +Capability tag: `workspace_skill_batch_toggle`. The workspace-qualified form is `POST /workspaces/:workspace/skills/enable`. + +Toggle up to 100 loaded Skills in one request. The route applies the same validation, persistence, and live-session refresh semantics as the single-Skill route. Names are trimmed and deduplicated case-insensitively while preserving first-seen order. Processing is best-effort and ordered: a target failure is recorded in `errors` and does not prevent later targets from being attempted. + +Request: + +```json +{ + "skillNames": ["review", "deploy"], + "enabled": false +} +``` + +Response (200): + +```json +{ + "enabled": false, + "results": [ + { + "skillName": "review", + "enabled": false, + "changed": true, + "activation": "applied", + "sessionsRefreshed": 2, + "sessionsFailed": 0 + } + ], + "errors": [ + { + "skillName": "missing", + "code": "skill_not_found", + "error": "Skill not found: missing" + } + ] +} +``` + +Target errors use `skill_not_found`, `skill_not_toggleable`, `skill_inactive_extension`, or `skill_toggle_failed`. Malformed requests return HTTP 400 with `invalid_skill_names`, `invalid_skill_name`, or `invalid_enabled_flag`. Authentication, workspace trust, client identity, and runtime-generation failures still fail the whole request through the standard route gates. + #### `POST /workspace/init` Capability tag: `workspace_init`. Pure file IO — no ACP roundtrip, **no LLM invocation**. diff --git a/docs/users/qwen-serve.md b/docs/users/qwen-serve.md index 8f4b872f3fc..069bd80779e 100644 --- a/docs/users/qwen-serve.md +++ b/docs/users/qwen-serve.md @@ -192,7 +192,7 @@ idle daemon returns `initialized: false` with an empty snapshot. Once a session is alive they switch to `initialized: true` and surface the real state. -To mirror the CLI `/skills` panel remotely, call `POST /workspace/skills/:name/enable` with `{ "enabled": true | false }` after checking the `workspace_skill_toggle` capability. The route updates workspace `skills.disabled` and `skills.enabled` as needed, rejects unknown, hidden, inactive-extension, higher-scope-locked, and untrusted targets, and immediately refreshes active ACP sessions. Enabling a `skills.defaultDisabled` skill writes a canonical opt-in to `skills.enabled`; a hard `skills.disabled` entry inherited from a higher scope still cannot be overridden. Skill status cells expose `disabledReason` (`hard`, `default`, or `inactive_extension`) and an optional `lockedScope`. A `deferred` response means the setting was saved while no ACP child was running; it will apply when the child starts. `skills.disabled` disables both manual and model use, unlike `disable-model-invocation: true`, which keeps direct `/skill-name` invocation available. +To mirror the CLI `/skills` panel remotely, call `POST /workspace/skills/:name/enable` with `{ "enabled": true | false }` after checking the `workspace_skill_toggle` capability. To change several Skills, check `workspace_skill_batch_toggle` and call `POST /workspace/skills/enable` with `{ "skillNames": ["review", "deploy"], "enabled": false }`; its response separates successful `results` from per-target `errors`. The routes update workspace `skills.disabled` and `skills.enabled` as needed, reject unknown, hidden, inactive-extension, higher-scope-locked, and untrusted targets, and immediately refresh active ACP sessions. Enabling a `skills.defaultDisabled` skill writes a canonical opt-in to `skills.enabled`; a hard `skills.disabled` entry inherited from a higher scope still cannot be overridden. Skill status cells expose `disabledReason` (`hard`, `default`, or `inactive_extension`) and an optional `lockedScope`. A `deferred` response means the setting was saved while no ACP child was running; it will apply when the child starts. `skills.disabled` disables both manual and model use, unlike `disable-model-invocation: true`, which keeps direct `/skill-name` invocation available. `GET /workspace/env` and `GET /workspace/preflight` always answer with `initialized: true` regardless of ACP state. `env` never consults ACP diff --git a/packages/cli/src/serve/capabilities.ts b/packages/cli/src/serve/capabilities.ts index bedf3d200cd..2ea74998605 100644 --- a/packages/cli/src/serve/capabilities.ts +++ b/packages/cli/src/serve/capabilities.ts @@ -170,6 +170,7 @@ export const SERVE_CAPABILITY_REGISTRY = { // (`tools.disabled` is consulted at `Config` construction time). workspace_tool_toggle: { since: 'v1' }, workspace_skill_toggle: { since: 'v1' }, + workspace_skill_batch_toggle: { since: 'v1' }, workspace_skill_manage: { since: 'v1' }, workspace_settings: { since: 'v1' }, // `GET /workspace/permissions` is always available when this tag is diff --git a/packages/cli/src/serve/routes/workspace-skills.test.ts b/packages/cli/src/serve/routes/workspace-skills.test.ts index f14f543a18d..f6bd5054789 100644 --- a/packages/cli/src/serve/routes/workspace-skills.test.ts +++ b/packages/cli/src/serve/routes/workspace-skills.test.ts @@ -7,6 +7,10 @@ import request from 'supertest'; import { describe, expect, it, vi } from 'vitest'; import type { WorkspaceRuntime } from '../workspace-registry.js'; import { WorkspaceSkillManagementError } from '../workspace-skill-management.js'; +import { + WorkspaceSkillNotFoundError, + WorkspaceSkillNotToggleableError, +} from '../workspace-service/types.js'; import { registerWorkspaceSkillsRoutes } from './workspace-skills.js'; function createHarness() { @@ -20,6 +24,16 @@ function createHarness() { scope: 'global', deleted: true, }); + const setWorkspaceSkillEnabled = vi.fn( + async (_ctx: unknown, skillName: string, enabled: boolean) => ({ + skillName: skillName.toLowerCase(), + enabled, + changed: true, + activation: 'applied' as const, + sessionsRefreshed: 1, + sessionsFailed: 0, + }), + ); const app = express(); app.use(express.json({ limit: '10mb' })); registerWorkspaceSkillsRoutes(app, { @@ -29,6 +43,7 @@ function createHarness() { workspaceService: { installWorkspaceSkill, deleteWorkspaceSkill, + setWorkspaceSkillEnabled, }, } as unknown as WorkspaceRuntime, mutate: () => (_req: Request, _res: Response, next: NextFunction) => next(), @@ -36,7 +51,12 @@ function createHarness() { sendBridgeError: vi.fn(), parseAndValidateClientId: () => 'client-1', }); - return { app, installWorkspaceSkill, deleteWorkspaceSkill }; + return { + app, + installWorkspaceSkill, + deleteWorkspaceSkill, + setWorkspaceSkillEnabled, + }; } describe('workspace Skill management routes', () => { @@ -134,4 +154,106 @@ describe('workspace Skill management routes', () => { expect(response.body.code).toBe('invalid_skill_name'); expect(harness.deleteWorkspaceSkill).not.toHaveBeenCalled(); }); + + it('toggles a deduplicated Skill batch and returns per-target errors', async () => { + const harness = createHarness(); + harness.setWorkspaceSkillEnabled.mockImplementation( + async (_ctx: unknown, skillName: string, enabled: boolean) => { + if (skillName === 'missing') { + throw new WorkspaceSkillNotFoundError(skillName); + } + if (skillName === 'locked') { + throw new WorkspaceSkillNotToggleableError( + skillName, + 'locked', + 'user', + ); + } + return { + skillName: skillName.toLowerCase(), + enabled, + changed: true, + activation: 'applied' as const, + sessionsRefreshed: 1, + sessionsFailed: 0, + }; + }, + ); + + const response = await request(harness.app) + .post('/workspace/skills/enable') + .send({ + skillNames: [' Review ', 'review', 'missing', 'locked'], + enabled: false, + }); + + expect(response.status).toBe(200); + expect(response.body).toEqual({ + enabled: false, + results: [ + { + skillName: 'review', + enabled: false, + changed: true, + activation: 'applied', + sessionsRefreshed: 1, + sessionsFailed: 0, + }, + ], + errors: [ + { + skillName: 'missing', + code: 'skill_not_found', + error: 'Skill not found: missing', + }, + { + skillName: 'locked', + code: 'skill_not_toggleable', + error: 'Skill locked is locked by user settings', + reason: 'locked', + lockedScope: 'user', + }, + ], + }); + expect(harness.setWorkspaceSkillEnabled).toHaveBeenCalledTimes(3); + expect(harness.setWorkspaceSkillEnabled).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ + route: 'POST /workspace/skills/enable', + originatorClientId: 'client-1', + }), + 'Review', + false, + ); + }); + + it('validates Skill batch request shape before calling the service', async () => { + const harness = createHarness(); + + const empty = await request(harness.app) + .post('/workspace/skills/enable') + .send({ skillNames: [], enabled: false }); + const tooMany = await request(harness.app) + .post('/workspace/skills/enable') + .send({ + skillNames: Array.from({ length: 101 }, (_, i) => `s${i}`), + enabled: false, + }); + const blank = await request(harness.app) + .post('/workspace/skills/enable') + .send({ skillNames: [' '], enabled: false }); + const invalidFlag = await request(harness.app) + .post('/workspace/skills/enable') + .send({ skillNames: ['review'], enabled: 'no' }); + + expect(empty.status).toBe(400); + expect(empty.body.code).toBe('invalid_skill_names'); + expect(tooMany.status).toBe(400); + expect(tooMany.body.code).toBe('invalid_skill_names'); + expect(blank.status).toBe(400); + expect(blank.body.code).toBe('invalid_skill_name'); + expect(invalidFlag.status).toBe(400); + expect(invalidFlag.body.code).toBe('invalid_enabled_flag'); + expect(harness.setWorkspaceSkillEnabled).not.toHaveBeenCalled(); + }); }); diff --git a/packages/cli/src/serve/routes/workspace-skills.ts b/packages/cli/src/serve/routes/workspace-skills.ts index 54882312dc8..7f83250039d 100644 --- a/packages/cli/src/serve/routes/workspace-skills.ts +++ b/packages/cli/src/serve/routes/workspace-skills.ts @@ -11,6 +11,7 @@ import { parseAndValidateWorkspaceClientId, } from '../server/request-helpers.js'; import { + isGenerationClosedError, requireTrustedWorkspaceRuntime, resolveWorkspaceRuntimeFromParam, } from '../workspace-route-runtime.js'; @@ -25,6 +26,14 @@ import { type WorkspaceSkillInstallRequest, type WorkspaceSkillScope, } from '../workspace-skill-management.js'; +import { + WorkspaceSkillNotFoundError, + WorkspaceSkillNotToggleableError, + type DaemonWorkspaceService, + type WorkspaceRequestContext, +} from '../workspace-service/types.js'; + +const MAX_WORKSPACE_SKILL_BATCH_SIZE = 100; interface RegisterWorkspaceSkillsRoutesDeps { workspaceRuntime: WorkspaceRuntime; @@ -76,6 +85,107 @@ function parseSkillToggleRequest( return { skillName, enabled }; } +function parseSkillBatchToggleRequest( + req: Request, + res: Response, + safeBody: (req: Request) => Record, +): { skillNames: string[]; enabled: boolean } | undefined { + const body = safeBody(req); + const rawSkillNames = body['skillNames']; + if ( + !Array.isArray(rawSkillNames) || + rawSkillNames.length === 0 || + rawSkillNames.length > MAX_WORKSPACE_SKILL_BATCH_SIZE || + !rawSkillNames.every((name) => typeof name === 'string') + ) { + res.status(400).json({ + error: `\`skillNames\` must be a non-empty string array (max ${MAX_WORKSPACE_SKILL_BATCH_SIZE})`, + code: 'invalid_skill_names', + }); + return undefined; + } + + const skillNames: string[] = []; + const seen = new Set(); + for (const rawSkillName of rawSkillNames as string[]) { + const skillName = rawSkillName.trim(); + if (skillName.length === 0) { + res.status(400).json({ + error: 'Skill names must not be empty', + code: 'invalid_skill_name', + }); + return undefined; + } + if (skillName.length > MAX_WORKSPACE_SKILL_NAME_LENGTH) { + res.status(400).json({ + error: `Skill name exceeds ${MAX_WORKSPACE_SKILL_NAME_LENGTH}-character limit`, + code: 'invalid_skill_name', + }); + return undefined; + } + const normalizedName = skillName.toLowerCase(); + if (seen.has(normalizedName)) continue; + seen.add(normalizedName); + skillNames.push(skillName); + } + + const enabled = body['enabled']; + if (typeof enabled !== 'boolean') { + res.status(400).json({ + error: '`enabled` is required and must be a boolean', + code: 'invalid_enabled_flag', + }); + return undefined; + } + return { skillNames, enabled }; +} + +async function setWorkspaceSkillsEnabled( + service: DaemonWorkspaceService, + ctx: WorkspaceRequestContext, + skillNames: readonly string[], + enabled: boolean, +) { + const results = []; + const errors = []; + for (const skillName of skillNames) { + try { + results.push( + await service.setWorkspaceSkillEnabled(ctx, skillName, enabled), + ); + } catch (error) { + if (isGenerationClosedError(error)) throw error; + if (error instanceof WorkspaceSkillNotFoundError) { + errors.push({ + skillName: error.skillName, + code: 'skill_not_found' as const, + error: error.message, + }); + continue; + } + if (error instanceof WorkspaceSkillNotToggleableError) { + errors.push({ + skillName: error.skillName, + code: + error.reason === 'inactive_extension' + ? ('skill_inactive_extension' as const) + : ('skill_not_toggleable' as const), + error: error.message, + reason: error.reason, + ...(error.lockedScope ? { lockedScope: error.lockedScope } : {}), + }); + continue; + } + errors.push({ + skillName, + code: 'skill_toggle_failed' as const, + error: error instanceof Error ? error.message : String(error), + }); + } + } + return { enabled, results, errors }; +} + function parseSkillScope( value: unknown, res: Response, @@ -165,6 +275,7 @@ export function registerWorkspaceSkillsRoutes( deps.workspaceRuntime.workspaceCwd, ); const route = 'POST /workspace/skills/:name/enable'; + const batchRoute = 'POST /workspace/skills/enable'; app.post( '/workspace/skills/install', deps.mutate({ strict: true }), @@ -220,6 +331,28 @@ export function registerWorkspaceSkillsRoutes( } }, ); + app.post( + '/workspace/skills/enable', + deps.mutate({ strict: true }), + async (req, res) => { + if (!requireTrustedWorkspaceRuntime(deps.workspaceRuntime, res)) return; + const input = parseSkillBatchToggleRequest(req, res, deps.safeBody); + if (!input) return; + const clientId = deps.parseAndValidateClientId(req, res); + if (clientId === null) return; + try { + const result = await setWorkspaceSkillsEnabled( + deps.workspaceRuntime.workspaceService, + buildWorkspaceCtx(batchRoute, clientId), + input.skillNames, + input.enabled, + ); + res.status(200).json(result); + } catch (err) { + deps.sendBridgeError(res, err, { route: batchRoute }); + } + }, + ); app.post( '/workspace/skills/:name/enable', deps.mutate({ strict: true }), @@ -252,6 +385,7 @@ export function registerWorkspaceQualifiedSkillsRoutes( > & { workspaceRegistry: WorkspaceRegistry }, ): void { const route = 'POST /workspaces/:workspace/skills/:name/enable'; + const batchRoute = 'POST /workspaces/:workspace/skills/enable'; app.post( '/workspaces/:workspace/skills/install', deps.mutate({ strict: true }), @@ -323,6 +457,37 @@ export function registerWorkspaceQualifiedSkillsRoutes( } }, ); + app.post( + '/workspaces/:workspace/skills/enable', + deps.mutate({ strict: true }), + async (req, res) => { + const runtime = resolveWorkspaceRuntimeFromParam( + deps.workspaceRegistry, + req, + res, + ); + if (!runtime || !requireTrustedWorkspaceRuntime(runtime, res)) return; + const input = parseSkillBatchToggleRequest(req, res, deps.safeBody); + if (!input) return; + const clientId = parseAndValidateWorkspaceClientId( + req, + res, + runtime.bridge, + ); + if (clientId === null) return; + try { + const result = await setWorkspaceSkillsEnabled( + runtime.workspaceService, + createBuildWorkspaceCtx(runtime.workspaceCwd)(batchRoute, clientId), + input.skillNames, + input.enabled, + ); + res.status(200).json(result); + } catch (err) { + deps.sendBridgeError(res, err, { route: batchRoute }); + } + }, + ); app.post( '/workspaces/:workspace/skills/:name/enable', deps.mutate({ strict: true }), diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 1c498649b22..d21c0649427 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -449,6 +449,7 @@ const EXPECTED_STAGE1_FEATURES = [ 'session_approval_mode_control', 'workspace_tool_toggle', 'workspace_skill_toggle', + 'workspace_skill_batch_toggle', 'workspace_skill_manage', 'workspace_permissions', 'workspace_trust', diff --git a/packages/cli/src/serve/workspace-qualified-rest.test.ts b/packages/cli/src/serve/workspace-qualified-rest.test.ts index f2248e9f4cc..b99cf93e2a1 100644 --- a/packages/cli/src/serve/workspace-qualified-rest.test.ts +++ b/packages/cli/src/serve/workspace-qualified-rest.test.ts @@ -970,6 +970,47 @@ describe('workspace-qualified core REST', () => { sessionsFailed: 0, }); + const batch = await request(h.app) + .post(`/workspaces/${encodeURIComponent(h.secondaryId)}/skills/enable`) + .set('Authorization', 'Bearer secret') + .set('X-Qwen-Client-Id', 'client-1') + .set('Host', host()) + .send({ skillNames: ['review', 'deploy'], enabled: false }); + expect(batch.status).toBe(200); + expect(batch.body).toEqual({ + enabled: false, + results: [ + { + skillName: 'review', + enabled: false, + changed: true, + activation: 'deferred', + sessionsRefreshed: 0, + sessionsFailed: 0, + }, + { + skillName: 'deploy', + enabled: false, + changed: true, + activation: 'deferred', + sessionsRefreshed: 0, + sessionsFailed: 0, + }, + ], + errors: [], + }); + expect( + h.secondaryWorkspaceService.setWorkspaceSkillEnabled, + ).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ + workspaceCwd: h.secondaryCwd, + originatorClientId: 'client-1', + }), + 'review', + false, + ); + vi.mocked( h.secondaryWorkspaceService.setWorkspaceSkillEnabled, ).mockRejectedValueOnce(new WorkspaceSkillNotFoundError('missing')); diff --git a/packages/sdk-typescript/src/daemon/DaemonClient.ts b/packages/sdk-typescript/src/daemon/DaemonClient.ts index 83d513386d5..047368d1dab 100644 --- a/packages/sdk-typescript/src/daemon/DaemonClient.ts +++ b/packages/sdk-typescript/src/daemon/DaemonClient.ts @@ -161,6 +161,7 @@ import type { DaemonRuntimeMcpAddResult, DaemonRuntimeMcpRemoveResult, DaemonToolToggleResult, + DaemonSkillBatchToggleResult, DaemonSkillToggleResult, DaemonSkillInstallRequest, DaemonSkillMutationResult, @@ -3142,6 +3143,36 @@ export class DaemonClient { ); } + /** + * Toggle up to 100 user-invocable skills and return every target outcome. + * + * Pre-flight + * `caps.features.includes('workspace_skill_batch_toggle')` before calling. + */ + async setWorkspaceSkillsEnabled( + skillNames: readonly string[], + enabled: boolean, + opts?: { clientId?: string }, + ): Promise { + return await this.fetchWithTimeout( + `${this.baseUrl}/workspace/skills/enable`, + { + method: 'POST', + headers: this.headers( + { 'Content-Type': 'application/json' }, + opts?.clientId, + ), + body: JSON.stringify({ skillNames, enabled }), + }, + async (res) => { + if (!res.ok) { + throw await this.failOnError(res, 'POST /workspace/skills/enable'); + } + return (await res.json()) as DaemonSkillBatchToggleResult; + }, + ); + } + installWorkspaceSkill( request: DaemonSkillInstallRequest, ): Promise { @@ -5836,6 +5867,19 @@ export class WorkspaceDaemonClient { ); } + setWorkspaceSkillsEnabled( + skillNames: readonly string[], + enabled: boolean, + opts?: { clientId?: string }, + ): Promise { + return this.post( + '/skills/enable', + 'POST /workspaces/:workspace/skills/enable', + { skillNames, enabled }, + opts?.clientId, + ); + } + restartMcpServer( serverName: string, opts?: { clientId?: string; entryIndex?: number | '*'; timeoutMs?: number }, diff --git a/packages/sdk-typescript/src/daemon/index.ts b/packages/sdk-typescript/src/daemon/index.ts index f29b11abdae..0d6ac52d766 100644 --- a/packages/sdk-typescript/src/daemon/index.ts +++ b/packages/sdk-typescript/src/daemon/index.ts @@ -392,6 +392,9 @@ export type { DaemonRuntimeMcpAddResult, DaemonRuntimeMcpRemoveResult, DaemonToolToggleResult, + DaemonSkillBatchToggleError, + DaemonSkillBatchToggleErrorCode, + DaemonSkillBatchToggleResult, DaemonSkillToggleActivation, DaemonSkillToggleResult, DaemonSkillScope, diff --git a/packages/sdk-typescript/src/daemon/types.ts b/packages/sdk-typescript/src/daemon/types.ts index d2ec3803580..eba58655dd3 100644 --- a/packages/sdk-typescript/src/daemon/types.ts +++ b/packages/sdk-typescript/src/daemon/types.ts @@ -2497,6 +2497,26 @@ export interface DaemonSkillToggleResult { sessionsFailed: number; } +export type DaemonSkillBatchToggleErrorCode = + | 'skill_not_found' + | 'skill_not_toggleable' + | 'skill_inactive_extension' + | 'skill_toggle_failed'; + +export interface DaemonSkillBatchToggleError { + skillName: string; + code: DaemonSkillBatchToggleErrorCode; + error: string; + reason?: 'not_user_invocable' | 'inactive_extension' | 'locked'; + lockedScope?: 'system' | 'user' | 'systemDefaults'; +} + +export interface DaemonSkillBatchToggleResult { + enabled: boolean; + results: DaemonSkillToggleResult[]; + errors: DaemonSkillBatchToggleError[]; +} + export type DaemonSkillScope = 'workspace' | 'global'; export type DaemonSkillInstallSource = diff --git a/packages/sdk-typescript/src/index.ts b/packages/sdk-typescript/src/index.ts index 203c4a6b988..4caa4c2f010 100644 --- a/packages/sdk-typescript/src/index.ts +++ b/packages/sdk-typescript/src/index.ts @@ -94,6 +94,9 @@ export { type DaemonSettingsReloadedData, type DaemonSettingsReloadedEvent, type DaemonToolToggleResult, + type DaemonSkillBatchToggleError, + type DaemonSkillBatchToggleErrorCode, + type DaemonSkillBatchToggleResult, type DaemonSkillToggleActivation, type DaemonSkillToggleResult, type DaemonSkillScope, diff --git a/packages/sdk-typescript/test/unit/DaemonClient.test.ts b/packages/sdk-typescript/test/unit/DaemonClient.test.ts index 7334f94a598..378385b6b72 100644 --- a/packages/sdk-typescript/test/unit/DaemonClient.test.ts +++ b/packages/sdk-typescript/test/unit/DaemonClient.test.ts @@ -4040,6 +4040,85 @@ describe('DaemonClient', () => { }); }); + describe('setWorkspaceSkillsEnabled', () => { + const response = { + enabled: false, + results: [ + { + skillName: 'review', + enabled: false, + changed: true, + activation: 'applied', + sessionsRefreshed: 2, + sessionsFailed: 0, + }, + ], + errors: [], + }; + + it('POSTs the Skill names, flag, and client id', async () => { + const { fetch, calls } = recordingFetch(() => + jsonResponse(200, response), + ); + const client = new DaemonClient({ baseUrl: 'http://daemon', fetch }); + + await expect( + client.setWorkspaceSkillsEnabled(['review', 'deploy'], false, { + clientId: 'client-1', + }), + ).resolves.toEqual(response); + expect(calls[0]).toMatchObject({ + url: 'http://daemon/workspace/skills/enable', + method: 'POST', + body: JSON.stringify({ + skillNames: ['review', 'deploy'], + enabled: false, + }), + }); + expect(calls[0]?.headers['x-qwen-client-id']).toBe('client-1'); + }); + + it('supports the workspace-qualified helper', async () => { + const { fetch, calls } = recordingFetch(() => + jsonResponse(200, response), + ); + const client = new DaemonClient({ baseUrl: 'http://daemon', fetch }); + + await client + .workspaceByCwd('/tmp/work space') + .setWorkspaceSkillsEnabled(['review', 'deploy'], false, { + clientId: 'client-2', + }); + + expect(calls[0]).toMatchObject({ + url: 'http://daemon/workspaces/%2Ftmp%2Fwork%20space/skills/enable', + method: 'POST', + body: JSON.stringify({ + skillNames: ['review', 'deploy'], + enabled: false, + }), + }); + expect(calls[0]?.headers['x-qwen-client-id']).toBe('client-2'); + }); + + it('passes request-level errors through', async () => { + const { fetch } = recordingFetch(() => + jsonResponse(400, { + error: '`skillNames` must be a non-empty string array (max 100)', + code: 'invalid_skill_names', + }), + ); + const client = new DaemonClient({ baseUrl: 'http://daemon', fetch }); + + await expect( + client.setWorkspaceSkillsEnabled([], false), + ).rejects.toMatchObject({ + status: 400, + body: expect.objectContaining({ code: 'invalid_skill_names' }), + }); + }); + }); + describe('workspace Skill management', () => { it('uploads a Skill package', async () => { const response = { From ff35624370aa5700903bb3ab5390e91087e357bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8F=B6=E5=85=AC?= Date: Fri, 7 Aug 2026 13:20:01 +0800 Subject: [PATCH 2/9] test(serve): update capability integration baseline --- integration-tests/cli/qwen-serve-routes.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/integration-tests/cli/qwen-serve-routes.test.ts b/integration-tests/cli/qwen-serve-routes.test.ts index 20d15f6f571..95d6371d580 100644 --- a/integration-tests/cli/qwen-serve-routes.test.ts +++ b/integration-tests/cli/qwen-serve-routes.test.ts @@ -364,6 +364,7 @@ describe('qwen serve — capabilities envelope', () => { 'session_approval_mode_control', 'workspace_tool_toggle', 'workspace_skill_toggle', + 'workspace_skill_batch_toggle', 'workspace_skill_manage', 'workspace_settings', 'workspace_permissions', From 1ba3021adce612a30155b7047f1bc3babe83ca34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8F=B6=E5=85=AC?= Date: Fri, 7 Aug 2026 16:43:44 +0800 Subject: [PATCH 3/9] fix(daemon): apply skill batches atomically --- docs/design/daemon-skill-batch-toggle.md | 16 +- .../developers/daemon/13-sdk-daemon-client.md | 2 +- docs/developers/qwen-serve-protocol.md | 12 +- docs/users/qwen-serve.md | 2 +- .../src/serve/daemon-status-provider.test.ts | 4 + .../src/serve/routes/workspace-skills.test.ts | 119 +++++++--- .../cli/src/serve/routes/workspace-skills.ts | 137 ++++-------- packages/cli/src/serve/run-qwen-serve.test.ts | 75 +++++++ packages/cli/src/serve/run-qwen-serve.ts | 90 ++++++++ packages/cli/src/serve/server.test.ts | 16 ++ packages/cli/src/serve/server.ts | 8 + .../cli/src/serve/server/error-response.ts | 29 +-- .../serve/workspace-qualified-rest.test.ts | 55 ++++- .../__tests__/facade.test.ts | 203 ++++++++++++++++++ .../cli/src/serve/workspace-service/index.ts | 175 +++++++++++++++ .../cli/src/serve/workspace-service/types.ts | 80 +++++++ packages/sdk-typescript/src/daemon/index.ts | 1 + packages/sdk-typescript/src/daemon/types.ts | 14 +- packages/sdk-typescript/src/index.ts | 1 + .../test/unit/DaemonClient.test.ts | 6 +- 20 files changed, 855 insertions(+), 190 deletions(-) diff --git a/docs/design/daemon-skill-batch-toggle.md b/docs/design/daemon-skill-batch-toggle.md index de36ed2d099..6f80e1abf6f 100644 --- a/docs/design/daemon-skill-batch-toggle.md +++ b/docs/design/daemon-skill-batch-toggle.md @@ -24,21 +24,23 @@ The request body is: `skillNames` is a non-empty string array with at most 100 entries. Names are trimmed and deduplicated case-insensitively while preserving first-seen order. -The response is best-effort: valid targets are toggled in order, and failures -for individual targets are returned without preventing later targets from -being attempted. +The response is best-effort for expected target errors: valid targets are +validated against one status snapshot, persisted in one locked write, and +applied with one live-session refresh. Unknown, hidden, inactive-extension, +and locked targets are returned without blocking the valid targets. Unexpected +persistence and runtime-generation failures fail the whole request. ```json { "enabled": false, + "activation": "applied", + "sessionsRefreshed": 1, + "sessionsFailed": 0, "results": [ { "skillName": "review", "enabled": false, - "changed": true, - "activation": "applied", - "sessionsRefreshed": 1, - "sessionsFailed": 0 + "changed": true } ], "errors": [ diff --git a/docs/developers/daemon/13-sdk-daemon-client.md b/docs/developers/daemon/13-sdk-daemon-client.md index 3235345ba80..c265342ab03 100644 --- a/docs/developers/daemon/13-sdk-daemon-client.md +++ b/docs/developers/daemon/13-sdk-daemon-client.md @@ -165,7 +165,7 @@ await client .setWorkspaceSkillsEnabled(['review', 'deploy'], true); ``` -`DaemonSkillBatchToggleResult` contains ordered successful `results` and per-target `errors`; one invalid target does not stop later targets. +`DaemonSkillBatchToggleResult` contains ordered successful `results`, per-target `errors`, and batch-level activation/session-refresh counts. The daemon persists valid targets together and refreshes active sessions once; one expected target error does not block other valid targets. Workspace display names are optional presentation metadata. Pre-flight `capabilities.features.includes('workspace_display_name')`; workspace ids and canonical paths remain the only selectors, and duplicate display names are valid. diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index 4b3a2f01a38..5b28aac8060 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -2676,7 +2676,7 @@ The mutation reuses the workspace-scoped `settings_changed` event for each chang Capability tag: `workspace_skill_batch_toggle`. The workspace-qualified form is `POST /workspaces/:workspace/skills/enable`. -Toggle up to 100 loaded Skills in one request. The route applies the same validation, persistence, and live-session refresh semantics as the single-Skill route. Names are trimmed and deduplicated case-insensitively while preserving first-seen order. Processing is best-effort and ordered: a target failure is recorded in `errors` and does not prevent later targets from being attempted. +Toggle up to 100 loaded Skills in one request. Names are trimmed and deduplicated case-insensitively while preserving first-seen order. The daemon validates against one Skill status snapshot, persists all valid changes in one locked settings write, and refreshes active sessions once. Processing is best-effort for expected target errors: an unknown, hidden, inactive-extension, or locked target is recorded in `errors` without preventing other valid targets from being applied. Unexpected persistence or runtime-generation failures still fail the whole request. Request: @@ -2692,14 +2692,14 @@ Response (200): ```json { "enabled": false, + "activation": "applied", + "sessionsRefreshed": 2, + "sessionsFailed": 0, "results": [ { "skillName": "review", "enabled": false, - "changed": true, - "activation": "applied", - "sessionsRefreshed": 2, - "sessionsFailed": 0 + "changed": true } ], "errors": [ @@ -2712,7 +2712,7 @@ Response (200): } ``` -Target errors use `skill_not_found`, `skill_not_toggleable`, `skill_inactive_extension`, or `skill_toggle_failed`. Malformed requests return HTTP 400 with `invalid_skill_names`, `invalid_skill_name`, or `invalid_enabled_flag`. Authentication, workspace trust, client identity, and runtime-generation failures still fail the whole request through the standard route gates. +Target errors use `skill_not_found`, `skill_not_toggleable`, or `skill_inactive_extension`. Malformed requests return HTTP 400 with `invalid_skill_names`, `invalid_skill_name`, or `invalid_enabled_flag`. Authentication, workspace trust, client identity, unexpected persistence failures, and runtime-generation failures fail the whole request through the standard route gates. Batch-level `activation`, `sessionsRefreshed`, and `sessionsFailed` describe the single live-session refresh shared by all changed results. #### `POST /workspace/init` diff --git a/docs/users/qwen-serve.md b/docs/users/qwen-serve.md index 069bd80779e..c6edc4b282f 100644 --- a/docs/users/qwen-serve.md +++ b/docs/users/qwen-serve.md @@ -192,7 +192,7 @@ idle daemon returns `initialized: false` with an empty snapshot. Once a session is alive they switch to `initialized: true` and surface the real state. -To mirror the CLI `/skills` panel remotely, call `POST /workspace/skills/:name/enable` with `{ "enabled": true | false }` after checking the `workspace_skill_toggle` capability. To change several Skills, check `workspace_skill_batch_toggle` and call `POST /workspace/skills/enable` with `{ "skillNames": ["review", "deploy"], "enabled": false }`; its response separates successful `results` from per-target `errors`. The routes update workspace `skills.disabled` and `skills.enabled` as needed, reject unknown, hidden, inactive-extension, higher-scope-locked, and untrusted targets, and immediately refresh active ACP sessions. Enabling a `skills.defaultDisabled` skill writes a canonical opt-in to `skills.enabled`; a hard `skills.disabled` entry inherited from a higher scope still cannot be overridden. Skill status cells expose `disabledReason` (`hard`, `default`, or `inactive_extension`) and an optional `lockedScope`. A `deferred` response means the setting was saved while no ACP child was running; it will apply when the child starts. `skills.disabled` disables both manual and model use, unlike `disable-model-invocation: true`, which keeps direct `/skill-name` invocation available. +To mirror the CLI `/skills` panel remotely, call `POST /workspace/skills/:name/enable` with `{ "enabled": true | false }` after checking the `workspace_skill_toggle` capability. To change several Skills, check `workspace_skill_batch_toggle` and call `POST /workspace/skills/enable` with `{ "skillNames": ["review", "deploy"], "enabled": false }`; its response separates successful `results` from per-target `errors`, persists valid targets together, and refreshes active ACP sessions once. The routes update workspace `skills.disabled` and `skills.enabled` as needed and reject unknown, hidden, inactive-extension, higher-scope-locked, and untrusted targets. Enabling a `skills.defaultDisabled` skill writes a canonical opt-in to `skills.enabled`; a hard `skills.disabled` entry inherited from a higher scope still cannot be overridden. Skill status cells expose `disabledReason` (`hard`, `default`, or `inactive_extension`) and an optional `lockedScope`. A `deferred` response means the setting was saved while no ACP child was running; it will apply when the child starts. `skills.disabled` disables both manual and model use, unlike `disable-model-invocation: true`, which keeps direct `/skill-name` invocation available. `GET /workspace/env` and `GET /workspace/preflight` always answer with `initialized: true` regardless of ACP state. `env` never consults ACP diff --git a/packages/cli/src/serve/daemon-status-provider.test.ts b/packages/cli/src/serve/daemon-status-provider.test.ts index 0c7c4ae07d5..e0632f353a3 100644 --- a/packages/cli/src/serve/daemon-status-provider.test.ts +++ b/packages/cli/src/serve/daemon-status-provider.test.ts @@ -54,6 +54,10 @@ function makeWorkspaceServiceWithProvider( isChannelLive: opts.isChannelLive ?? (() => false), persistDisabledTools: async () => {}, persistDisabledSkills: async () => ({ changed: false, disabled: [] }), + persistDisabledSkillsBatch: async () => ({ + outcomes: [], + settingsChanges: [], + }), queryWorkspaceStatus: opts.queryWorkspaceStatus ?? noopQueryWorkspaceStatus, invokeWorkspaceCommand: async () => { throw new Error('not wired'); diff --git a/packages/cli/src/serve/routes/workspace-skills.test.ts b/packages/cli/src/serve/routes/workspace-skills.test.ts index f6bd5054789..d5667af8a36 100644 --- a/packages/cli/src/serve/routes/workspace-skills.test.ts +++ b/packages/cli/src/serve/routes/workspace-skills.test.ts @@ -5,12 +5,10 @@ import express, { } from 'express'; import request from 'supertest'; import { describe, expect, it, vi } from 'vitest'; +import { sendBridgeError } from '../server/error-response.js'; import type { WorkspaceRuntime } from '../workspace-registry.js'; import { WorkspaceSkillManagementError } from '../workspace-skill-management.js'; -import { - WorkspaceSkillNotFoundError, - WorkspaceSkillNotToggleableError, -} from '../workspace-service/types.js'; +import type { WorkspaceSkillBatchToggleResult } from '../workspace-service/types.js'; import { registerWorkspaceSkillsRoutes } from './workspace-skills.js'; function createHarness() { @@ -34,6 +32,24 @@ function createHarness() { sessionsFailed: 0, }), ); + const setWorkspaceSkillsEnabled = vi.fn( + async ( + _ctx: unknown, + skillNames: readonly string[], + enabled: boolean, + ): Promise => ({ + enabled, + activation: 'applied', + sessionsRefreshed: 1, + sessionsFailed: 0, + results: skillNames.map((skillName) => ({ + skillName: skillName.toLowerCase(), + enabled, + changed: true, + })), + errors: [], + }), + ); const app = express(); app.use(express.json({ limit: '10mb' })); registerWorkspaceSkillsRoutes(app, { @@ -44,11 +60,12 @@ function createHarness() { installWorkspaceSkill, deleteWorkspaceSkill, setWorkspaceSkillEnabled, + setWorkspaceSkillsEnabled, }, } as unknown as WorkspaceRuntime, mutate: () => (_req: Request, _res: Response, next: NextFunction) => next(), safeBody: (req) => req.body as Record, - sendBridgeError: vi.fn(), + sendBridgeError, parseAndValidateClientId: () => 'client-1', }); return { @@ -56,6 +73,7 @@ function createHarness() { installWorkspaceSkill, deleteWorkspaceSkill, setWorkspaceSkillEnabled, + setWorkspaceSkillsEnabled, }; } @@ -157,28 +175,27 @@ describe('workspace Skill management routes', () => { it('toggles a deduplicated Skill batch and returns per-target errors', async () => { const harness = createHarness(); - harness.setWorkspaceSkillEnabled.mockImplementation( - async (_ctx: unknown, skillName: string, enabled: boolean) => { - if (skillName === 'missing') { - throw new WorkspaceSkillNotFoundError(skillName); - } - if (skillName === 'locked') { - throw new WorkspaceSkillNotToggleableError( - skillName, - 'locked', - 'user', - ); - } - return { - skillName: skillName.toLowerCase(), - enabled, - changed: true, - activation: 'applied' as const, - sessionsRefreshed: 1, - sessionsFailed: 0, - }; - }, - ); + harness.setWorkspaceSkillsEnabled.mockResolvedValueOnce({ + enabled: false, + activation: 'applied', + sessionsRefreshed: 1, + sessionsFailed: 0, + results: [{ skillName: 'review', enabled: false, changed: true }], + errors: [ + { + skillName: 'missing', + code: 'skill_not_found', + error: 'Skill not found: missing', + }, + { + skillName: 'locked', + code: 'skill_not_toggleable', + error: 'Skill locked is locked by user settings', + reason: 'locked', + lockedScope: 'user', + }, + ], + }); const response = await request(harness.app) .post('/workspace/skills/enable') @@ -190,14 +207,14 @@ describe('workspace Skill management routes', () => { expect(response.status).toBe(200); expect(response.body).toEqual({ enabled: false, + activation: 'applied', + sessionsRefreshed: 1, + sessionsFailed: 0, results: [ { skillName: 'review', enabled: false, changed: true, - activation: 'applied', - sessionsRefreshed: 1, - sessionsFailed: 0, }, ], errors: [ @@ -215,14 +232,12 @@ describe('workspace Skill management routes', () => { }, ], }); - expect(harness.setWorkspaceSkillEnabled).toHaveBeenCalledTimes(3); - expect(harness.setWorkspaceSkillEnabled).toHaveBeenNthCalledWith( - 1, + expect(harness.setWorkspaceSkillsEnabled).toHaveBeenCalledWith( expect.objectContaining({ route: 'POST /workspace/skills/enable', originatorClientId: 'client-1', }), - 'Review', + ['Review', 'missing', 'locked'], false, ); }); @@ -245,6 +260,18 @@ describe('workspace Skill management routes', () => { const invalidFlag = await request(harness.app) .post('/workspace/skills/enable') .send({ skillNames: ['review'], enabled: 'no' }); + const nonString = await request(harness.app) + .post('/workspace/skills/enable') + .send({ skillNames: ['review', 42], enabled: false }); + const tooLong = await request(harness.app) + .post('/workspace/skills/enable') + .send({ skillNames: ['x'.repeat(257)], enabled: false }); + const exactLimit = await request(harness.app) + .post('/workspace/skills/enable') + .send({ + skillNames: Array.from({ length: 100 }, (_, i) => `s${i}`), + enabled: false, + }); expect(empty.status).toBe(400); expect(empty.body.code).toBe('invalid_skill_names'); @@ -254,6 +281,28 @@ describe('workspace Skill management routes', () => { expect(blank.body.code).toBe('invalid_skill_name'); expect(invalidFlag.status).toBe(400); expect(invalidFlag.body.code).toBe('invalid_enabled_flag'); - expect(harness.setWorkspaceSkillEnabled).not.toHaveBeenCalled(); + expect(nonString.status).toBe(400); + expect(nonString.body.code).toBe('invalid_skill_names'); + expect(tooLong.status).toBe(400); + expect(tooLong.body.code).toBe('invalid_skill_name'); + expect(exactLimit.status).toBe(200); + expect(harness.setWorkspaceSkillsEnabled).toHaveBeenCalledTimes(1); + }); + + it('fails the whole batch when the workspace generation closes', async () => { + const harness = createHarness(); + harness.setWorkspaceSkillsEnabled.mockRejectedValueOnce( + Object.assign(new Error('closed'), { + code: 'workspace_generation_closed', + }), + ); + + const response = await request(harness.app) + .post('/workspace/skills/enable') + .send({ skillNames: ['review', 'deploy'], enabled: false }); + + expect(response.status).toBe(503); + expect(response.body.code).toBe('workspace_runtime_unavailable'); + expect(harness.setWorkspaceSkillsEnabled).toHaveBeenCalledOnce(); }); }); diff --git a/packages/cli/src/serve/routes/workspace-skills.ts b/packages/cli/src/serve/routes/workspace-skills.ts index 7f83250039d..c42fbdd795f 100644 --- a/packages/cli/src/serve/routes/workspace-skills.ts +++ b/packages/cli/src/serve/routes/workspace-skills.ts @@ -11,7 +11,6 @@ import { parseAndValidateWorkspaceClientId, } from '../server/request-helpers.js'; import { - isGenerationClosedError, requireTrustedWorkspaceRuntime, resolveWorkspaceRuntimeFromParam, } from '../workspace-route-runtime.js'; @@ -26,13 +25,6 @@ import { type WorkspaceSkillInstallRequest, type WorkspaceSkillScope, } from '../workspace-skill-management.js'; -import { - WorkspaceSkillNotFoundError, - WorkspaceSkillNotToggleableError, - type DaemonWorkspaceService, - type WorkspaceRequestContext, -} from '../workspace-service/types.js'; - const MAX_WORKSPACE_SKILL_BATCH_SIZE = 100; interface RegisterWorkspaceSkillsRoutesDeps { @@ -46,6 +38,28 @@ interface RegisterWorkspaceSkillsRoutesDeps { ) => string | undefined | null; } +function rejectSkillNameTooLong(skillName: string, res: Response): boolean { + if (skillName.length <= MAX_WORKSPACE_SKILL_NAME_LENGTH) return false; + res.status(400).json({ + error: `Skill name exceeds ${MAX_WORKSPACE_SKILL_NAME_LENGTH}-character limit`, + code: 'invalid_skill_name', + }); + return true; +} + +function parseEnabledFlag( + body: Record, + res: Response, +): { enabled: boolean } | undefined { + const enabled = body['enabled']; + if (typeof enabled === 'boolean') return { enabled }; + res.status(400).json({ + error: '`enabled` is required and must be a boolean', + code: 'invalid_enabled_flag', + }); + return undefined; +} + function parseSkillToggleRequest( req: Request, res: Response, @@ -67,22 +81,9 @@ function parseSkillToggleRequest( }); return undefined; } - if (skillName.length > MAX_WORKSPACE_SKILL_NAME_LENGTH) { - res.status(400).json({ - error: `Skill name exceeds ${MAX_WORKSPACE_SKILL_NAME_LENGTH}-character limit`, - code: 'invalid_skill_name', - }); - return undefined; - } - const enabled = safeBody(req)['enabled']; - if (typeof enabled !== 'boolean') { - res.status(400).json({ - error: '`enabled` is required and must be a boolean', - code: 'invalid_enabled_flag', - }); - return undefined; - } - return { skillName, enabled }; + if (rejectSkillNameTooLong(skillName, res)) return undefined; + const flag = parseEnabledFlag(safeBody(req), res); + return flag ? { skillName, enabled: flag.enabled } : undefined; } function parseSkillBatchToggleRequest( @@ -116,74 +117,15 @@ function parseSkillBatchToggleRequest( }); return undefined; } - if (skillName.length > MAX_WORKSPACE_SKILL_NAME_LENGTH) { - res.status(400).json({ - error: `Skill name exceeds ${MAX_WORKSPACE_SKILL_NAME_LENGTH}-character limit`, - code: 'invalid_skill_name', - }); - return undefined; - } + if (rejectSkillNameTooLong(skillName, res)) return undefined; const normalizedName = skillName.toLowerCase(); if (seen.has(normalizedName)) continue; seen.add(normalizedName); skillNames.push(skillName); } - const enabled = body['enabled']; - if (typeof enabled !== 'boolean') { - res.status(400).json({ - error: '`enabled` is required and must be a boolean', - code: 'invalid_enabled_flag', - }); - return undefined; - } - return { skillNames, enabled }; -} - -async function setWorkspaceSkillsEnabled( - service: DaemonWorkspaceService, - ctx: WorkspaceRequestContext, - skillNames: readonly string[], - enabled: boolean, -) { - const results = []; - const errors = []; - for (const skillName of skillNames) { - try { - results.push( - await service.setWorkspaceSkillEnabled(ctx, skillName, enabled), - ); - } catch (error) { - if (isGenerationClosedError(error)) throw error; - if (error instanceof WorkspaceSkillNotFoundError) { - errors.push({ - skillName: error.skillName, - code: 'skill_not_found' as const, - error: error.message, - }); - continue; - } - if (error instanceof WorkspaceSkillNotToggleableError) { - errors.push({ - skillName: error.skillName, - code: - error.reason === 'inactive_extension' - ? ('skill_inactive_extension' as const) - : ('skill_not_toggleable' as const), - error: error.message, - reason: error.reason, - ...(error.lockedScope ? { lockedScope: error.lockedScope } : {}), - }); - continue; - } - errors.push({ - skillName, - code: 'skill_toggle_failed' as const, - error: error instanceof Error ? error.message : String(error), - }); - } - } - return { enabled, results, errors }; + const flag = parseEnabledFlag(body, res); + return flag ? { skillNames, enabled: flag.enabled } : undefined; } function parseSkillScope( @@ -212,13 +154,7 @@ function parseSkillInstallRequest( }); return undefined; } - if (name.trim().length > MAX_WORKSPACE_SKILL_NAME_LENGTH) { - res.status(400).json({ - error: `Skill name exceeds ${MAX_WORKSPACE_SKILL_NAME_LENGTH}-character limit`, - code: 'invalid_skill_name', - }); - return undefined; - } + if (rejectSkillNameTooLong(name.trim(), res)) return undefined; const scope = parseSkillScope(body['scope'], res); if (!scope) return undefined; const rawSource = body['source']; @@ -341,12 +277,12 @@ export function registerWorkspaceSkillsRoutes( const clientId = deps.parseAndValidateClientId(req, res); if (clientId === null) return; try { - const result = await setWorkspaceSkillsEnabled( - deps.workspaceRuntime.workspaceService, - buildWorkspaceCtx(batchRoute, clientId), - input.skillNames, - input.enabled, - ); + const result = + await deps.workspaceRuntime.workspaceService.setWorkspaceSkillsEnabled( + buildWorkspaceCtx(batchRoute, clientId), + input.skillNames, + input.enabled, + ); res.status(200).json(result); } catch (err) { deps.sendBridgeError(res, err, { route: batchRoute }); @@ -476,8 +412,7 @@ export function registerWorkspaceQualifiedSkillsRoutes( ); if (clientId === null) return; try { - const result = await setWorkspaceSkillsEnabled( - runtime.workspaceService, + const result = await runtime.workspaceService.setWorkspaceSkillsEnabled( createBuildWorkspaceCtx(runtime.workspaceCwd)(batchRoute, clientId), input.skillNames, input.enabled, diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index bc1d761fbf7..343aa9850af 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -780,6 +780,81 @@ describe('workspace skill settings persistence', () => { expect(setValue.mock.calls).toHaveLength(1); expect(setValue.mock.calls[0]?.[3]).toBe(toolGuard); }); + + it('persists a Skill batch with one settings write and per-target lock outcomes', async () => { + workspace = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), 'qws-skill-batch-')), + ); + qwenHome = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), 'qws-skill-batch-home-')), + ); + previousQwenHome = process.env['QWEN_HOME']; + process.env['QWEN_HOME'] = qwenHome; + settingsRuntime.resetHomeEnvBootstrapForTesting(); + fs.mkdirSync(path.join(workspace, '.qwen'), { recursive: true }); + fs.writeFileSync( + path.join(workspace, '.qwen', 'settings.json'), + JSON.stringify({ skills: { disabled: ['orphan'] } }), + ); + fs.writeFileSync( + path.join(qwenHome, 'settings.json'), + JSON.stringify({ skills: { disabled: ['locked-skill'] } }), + ); + + const originalCreateServeApp = serverModule.createServeApp; + let persistDisabledSkillsBatch: + | NonNullable< + Parameters[2] + >['persistDisabledSkillsBatch'] + | undefined; + vi.spyOn(serverModule, 'createServeApp').mockImplementation((...args) => { + persistDisabledSkillsBatch = args[2]?.persistDisabledSkillsBatch; + return originalCreateServeApp(...args); + }); + handle = await runQwenServe( + { + port: 0, + hostname: '127.0.0.1', + mode: 'http-bridge', + workspace, + serveWebShell: false, + }, + { bridge: makeRuntimeBridge() }, + ); + await handle.runtimeReady; + expect(persistDisabledSkillsBatch).toBeDefined(); + const setValues = vi.spyOn( + settingsRuntime.LoadedSettings.prototype, + 'setValues', + ); + + const result = await persistDisabledSkillsBatch!( + workspace, + ['review', 'alpha', 'locked-skill'], + false, + ); + + expect(result.outcomes).toHaveLength(3); + expect(result.outcomes[0]).toEqual({ + skillName: 'review', + changed: true, + }); + expect(result.outcomes[1]).toEqual({ + skillName: 'alpha', + changed: true, + }); + expect(result.outcomes[2]).toMatchObject({ + skillName: 'locked-skill', + error: { reason: 'locked', lockedScope: 'user' }, + }); + expect(result.settingsChanges).toEqual([ + { + key: 'skills.disabled', + value: ['orphan', 'review', 'alpha'], + }, + ]); + expect(setValues).toHaveBeenCalledOnce(); + }); }); /** diff --git a/packages/cli/src/serve/run-qwen-serve.ts b/packages/cli/src/serve/run-qwen-serve.ts index 275e58081ca..a5118fcd8e4 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -389,6 +389,8 @@ type RunQwenServeOptions = Omit & { }; type WorkspaceSettingsWrite = import('./workspace-service/types.js').WorkspaceSettingsWrite; +type PersistDisabledSkillsBatchResult = + import('./workspace-service/types.js').PersistDisabledSkillsBatchResult; type ChannelWebhookConfigRuntime = { loadChannelsConfig: typeof import('../commands/channel/runtime.js').loadChannelsConfig; parseChannelWebhookConfig: typeof import('../commands/channel/config-utils.js').parseChannelWebhookConfig; @@ -3763,6 +3765,90 @@ async function runQwenServeImpl( settingsChanges, }; }); + const persistDisabledSkillsBatchFn = ( + workspace: string, + skillNames: readonly string[], + enabled: boolean, + assertGenerationOpen?: () => void, + ): Promise => + withSettingsLock(workspace, async () => { + assertGenerationOpen?.(); + const { + resolveSkillSettings, + skillSettingStrings, + updateWorkspaceSkillSettingLists, + } = await import('../config/skill-settings.js'); + const fresh = loadSettingsForPersistence(workspace); + const resolved = resolveSkillSettings(fresh); + const initialDisabled = skillSettingStrings( + fresh, + WORKSPACE_SETTING_SCOPE, + 'disabled', + ); + const initialEnabled = skillSettingStrings( + fresh, + WORKSPACE_SETTING_SCOPE, + 'enabled', + ); + let next = { disabled: initialDisabled, enabled: initialEnabled }; + const outcomes: PersistDisabledSkillsBatchResult['outcomes'] = []; + + for (const skillName of skillNames) { + const normalizedName = skillName.trim().toLowerCase(); + const disablement = resolved.disablements.get(normalizedName); + if (disablement?.reason === 'hard' && disablement.lockedScope) { + outcomes.push({ + skillName, + error: new runtime.WorkspaceSkillNotToggleableError( + skillName, + 'locked', + disablement.lockedScope, + ), + }); + continue; + } + const updated = updateWorkspaceSkillSettingLists( + next, + skillName, + enabled, + resolved.defaultDisabledNames.has(normalizedName) && + !resolved.enabledNames.has(normalizedName), + ); + const changed = + JSON.stringify(updated.disabled) !== + JSON.stringify(next.disabled) || + JSON.stringify(updated.enabled) !== JSON.stringify(next.enabled); + next = updated; + outcomes.push({ skillName, changed }); + } + + const settingsChanges: PersistDisabledSkillsBatchResult['settingsChanges'] = + []; + if (JSON.stringify(next.disabled) !== JSON.stringify(initialDisabled)) { + settingsChanges.push({ + key: 'skills.disabled', + value: next.disabled.length > 0 ? next.disabled : undefined, + }); + } + if (JSON.stringify(next.enabled) !== JSON.stringify(initialEnabled)) { + settingsChanges.push({ + key: 'skills.enabled', + value: next.enabled.length > 0 ? next.enabled : undefined, + }); + } + if (settingsChanges.length > 0) { + assertGenerationOpen?.(); + fresh.setValues( + settingsChanges.map((change) => ({ + scope: WORKSPACE_SETTING_SCOPE, + ...change, + })), + undefined, + assertGenerationOpen, + ); + } + return { outcomes, settingsChanges }; + }); const persistSettingFn = ( workspace: string, scope: import('../config/settings.js').SettingScope, @@ -3949,6 +4035,7 @@ async function runQwenServeImpl( isChannelLive: () => bridge.isChannelLive(), persistDisabledTools: persistDisabledToolsFn, persistDisabledSkills: persistDisabledSkillsFn, + persistDisabledSkillsBatch: persistDisabledSkillsBatchFn, persistSetting: persistSettingFn, persistSettings: persistSettingsFn, preheatAcpChild: () => bridge.preheat(), @@ -4355,6 +4442,7 @@ async function runQwenServeImpl( preheatAcpChild: () => secondaryBridge.preheat(), persistDisabledTools: persistDisabledToolsFn, persistDisabledSkills: persistDisabledSkillsFn, + persistDisabledSkillsBatch: persistDisabledSkillsBatchFn, persistSetting: persistSettingFn, persistSettings: persistSettingsFn, reloadDaemonEnv: (workspace, assertGenerationOpen) => @@ -4886,6 +4974,7 @@ async function runQwenServeImpl( preheatAcpChild: () => wsBridge.preheat(), persistDisabledTools: persistDisabledToolsFn, persistDisabledSkills: persistDisabledSkillsFn, + persistDisabledSkillsBatch: persistDisabledSkillsBatchFn, persistSetting: persistSettingFn, persistSettings: persistSettingsFn, reloadDaemonEnv: (workspace, assertGenerationOpen) => @@ -5505,6 +5594,7 @@ async function runQwenServeImpl( clientMcpSenderRegistry, persistDisabledTools: persistDisabledToolsFn, persistDisabledSkills: persistDisabledSkillsFn, + persistDisabledSkillsBatch: persistDisabledSkillsBatchFn, persistSetting: persistSettingFn, persistSettings: persistSettingsFn, sessionArtifactsPersistenceAvailable: diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index d21c0649427..87221dc485d 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -17617,6 +17617,22 @@ describe('createServeApp', () => { expect(res.body.code).toBe('token_required'); }); + it('requires the strict bearer-auth mutation gate for Skill batches', async () => { + const bridge = fakeBridge(); + const persistDisabledSkillsBatch = vi.fn(); + const app = createServeApp(baseOpts, undefined, { + bridge, + persistDisabledSkillsBatch, + }); + const res = await request(app) + .post('/workspace/skills/enable') + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({ skillNames: ['review'], enabled: false }); + expect(res.status).toBe(401); + expect(res.body.code).toBe('token_required'); + expect(persistDisabledSkillsBatch).not.toHaveBeenCalled(); + }); + it('validates skill names and the enabled body', async () => { const bridge = fakeBridge(); const app = createServeApp(tokenOpts, undefined, { diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 8ea44491a7f..7e9cc1cc23e 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -516,6 +516,7 @@ export interface ServeAppDeps { enabled: boolean, ) => Promise; persistDisabledSkills?: DaemonWorkspaceServiceDeps['persistDisabledSkills']; + persistDisabledSkillsBatch?: DaemonWorkspaceServiceDeps['persistDisabledSkillsBatch']; contextFilename?: string; persistSetting?: ( workspace: string, @@ -1077,6 +1078,13 @@ export function createServeApp( 'setWorkspaceSkillEnabled requires persistDisabledSkills in ServeAppDeps', ); }), + persistDisabledSkillsBatch: + deps.persistDisabledSkillsBatch ?? + (async () => { + throw new Error( + 'setWorkspaceSkillsEnabled requires persistDisabledSkillsBatch in ServeAppDeps', + ); + }), queryWorkspaceStatus: (method, idle) => bridge.queryWorkspaceStatus(method, idle), invokeWorkspaceCommand: (method, params, invokeOpts) => diff --git a/packages/cli/src/serve/server/error-response.ts b/packages/cli/src/serve/server/error-response.ts index 70b5cedfd69..6abc88616fb 100644 --- a/packages/cli/src/serve/server/error-response.ts +++ b/packages/cli/src/serve/server/error-response.ts @@ -51,10 +51,7 @@ import { TotalSessionLimitExceededError, } from '../acp-session-bridge.js'; import type { DaemonLogger } from '../daemon-logger.js'; -import { - WorkspaceSkillNotFoundError, - WorkspaceSkillNotToggleableError, -} from '../workspace-service/types.js'; +import { mapWorkspaceSkillToggleError } from '../workspace-service/types.js'; import { sendGenerationClosedError } from '../workspace-route-runtime.js'; import { DaemonDrainingError } from './session-archive.js'; @@ -187,25 +184,11 @@ export function sendBridgeError( }); return; } - if (err instanceof WorkspaceSkillNotFoundError) { - res.status(404).json({ - error: err.message, - code: 'skill_not_found', - skillName: err.skillName, - }); - return; - } - if (err instanceof WorkspaceSkillNotToggleableError) { - res.status(409).json({ - error: err.message, - code: - err.reason === 'inactive_extension' - ? 'skill_inactive_extension' - : 'skill_not_toggleable', - skillName: err.skillName, - reason: err.reason, - ...(err.lockedScope ? { lockedScope: err.lockedScope } : {}), - }); + const skillError = mapWorkspaceSkillToggleError(err); + if (skillError) { + res + .status(skillError.code === 'skill_not_found' ? 404 : 409) + .json(skillError); return; } if (err instanceof InvalidSessionTranscriptCursorError) { diff --git a/packages/cli/src/serve/workspace-qualified-rest.test.ts b/packages/cli/src/serve/workspace-qualified-rest.test.ts index b99cf93e2a1..34301fbc47b 100644 --- a/packages/cli/src/serve/workspace-qualified-rest.test.ts +++ b/packages/cli/src/serve/workspace-qualified-rest.test.ts @@ -153,6 +153,20 @@ function makeWorkspaceService(label: string): DaemonWorkspaceService { sessionsRefreshed: 0, sessionsFailed: 0, })), + setWorkspaceSkillsEnabled: vi.fn( + async (_ctx, skillNames: readonly string[], enabled) => ({ + enabled, + activation: 'deferred' as const, + sessionsRefreshed: 0, + sessionsFailed: 0, + results: skillNames.map((skillName) => ({ + skillName, + enabled, + changed: true, + })), + errors: [], + }), + ), initWorkspace: vi.fn(async (ctx) => ({ path: `${ctx.workspaceCwd}/QWEN.md`, action: 'created' as const, @@ -979,35 +993,31 @@ describe('workspace-qualified core REST', () => { expect(batch.status).toBe(200); expect(batch.body).toEqual({ enabled: false, + activation: 'deferred', + sessionsRefreshed: 0, + sessionsFailed: 0, results: [ { skillName: 'review', enabled: false, changed: true, - activation: 'deferred', - sessionsRefreshed: 0, - sessionsFailed: 0, }, { skillName: 'deploy', enabled: false, changed: true, - activation: 'deferred', - sessionsRefreshed: 0, - sessionsFailed: 0, }, ], errors: [], }); expect( - h.secondaryWorkspaceService.setWorkspaceSkillEnabled, - ).toHaveBeenNthCalledWith( - 2, + h.secondaryWorkspaceService.setWorkspaceSkillsEnabled, + ).toHaveBeenCalledWith( expect.objectContaining({ workspaceCwd: h.secondaryCwd, originatorClientId: 'client-1', }), - 'review', + ['review', 'deploy'], false, ); @@ -1034,6 +1044,18 @@ describe('workspace-qualified core REST', () => { .send({ enabled: false }); expect(invalidClient.status).toBe(400); expect(invalidClient.body.code).toBe('invalid_client_id'); + + const invalidBatchClient = await request(h.app) + .post(`/workspaces/${encodeURIComponent(h.secondaryId)}/skills/enable`) + .set('Authorization', 'Bearer secret') + .set('X-Qwen-Client-Id', 'forged-client') + .set('Host', host()) + .send({ skillNames: ['review'], enabled: false }); + expect(invalidBatchClient.status).toBe(400); + expect(invalidBatchClient.body.code).toBe('invalid_client_id'); + expect( + h.secondaryWorkspaceService.setWorkspaceSkillsEnabled, + ).toHaveBeenCalledTimes(1); } finally { await fsp.rm(h.scratch, { recursive: true, force: true }); } @@ -1052,6 +1074,19 @@ describe('workspace-qualified core REST', () => { .send({ enabled: false }); expect(res.status).toBe(403); expect(res.body.code).toBe('untrusted_workspace'); + + const batch = await request(untrusted.app) + .post( + `/workspaces/${encodeURIComponent(untrusted.secondaryId)}/skills/enable`, + ) + .set('Authorization', 'Bearer secret') + .set('Host', host()) + .send({ skillNames: ['review'], enabled: false }); + expect(batch.status).toBe(403); + expect(batch.body.code).toBe('untrusted_workspace'); + expect( + untrusted.secondaryWorkspaceService.setWorkspaceSkillsEnabled, + ).not.toHaveBeenCalled(); } finally { await fsp.rm(untrusted.scratch, { recursive: true, force: true }); } diff --git a/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts b/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts index ed77aa1f2cd..9528db0002d 100644 --- a/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts +++ b/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts @@ -111,6 +111,7 @@ import { WorkspaceVoiceError } from '../../../services/voice-service.js'; import { WorkspacePermissionRulesSessionRequiredError, WorkspaceSkillNotFoundError, + WorkspaceSkillNotToggleableError, WorkspaceSettingsPartialPersistError, } from '../types.js'; import type { @@ -136,6 +137,10 @@ function makeDeps( changed: true, disabled: [], }), + persistDisabledSkillsBatch: vi.fn().mockResolvedValue({ + outcomes: [], + settingsChanges: [], + }), queryWorkspaceStatus: vi .fn() .mockImplementation((_method: string, idle: () => unknown) => @@ -2035,6 +2040,204 @@ describe('createDaemonWorkspaceService', () => { }); }); + describe('setWorkspaceSkillsEnabled', () => { + const skills = [ + { + kind: 'skill' as const, + status: 'ok' as const, + name: 'review', + description: 'Review changed code', + level: 'bundled' as const, + modelInvocable: true, + }, + { + kind: 'skill' as const, + status: 'ok' as const, + name: 'deploy', + description: 'Deploy code', + level: 'bundled' as const, + modelInvocable: true, + }, + { + kind: 'skill' as const, + status: 'ok' as const, + name: 'hidden', + description: 'Hidden skill', + level: 'bundled' as const, + modelInvocable: true, + userInvocable: false, + }, + { + kind: 'skill' as const, + status: 'disabled' as const, + name: 'inactive', + description: 'Inactive extension skill', + level: 'extension' as const, + extensionName: 'demo', + modelInvocable: true, + disabledReason: 'inactive_extension' as const, + }, + { + kind: 'skill' as const, + status: 'disabled' as const, + name: 'locked', + description: 'Locked skill', + level: 'bundled' as const, + modelInvocable: true, + disabledReason: 'hard' as const, + lockedScope: 'user' as const, + }, + ]; + + it('persists and refreshes once while preserving ordered target outcomes', async () => { + const queryWorkspaceStatus = vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }); + const persistDisabledSkillsBatch = vi.fn().mockResolvedValue({ + outcomes: [ + { skillName: 'review', changed: true }, + { + skillName: 'locked', + error: new WorkspaceSkillNotToggleableError( + 'locked', + 'locked', + 'user', + ), + }, + { skillName: 'deploy', changed: true }, + ], + settingsChanges: [ + { key: 'skills.disabled', value: ['review', 'deploy'] }, + ], + }); + const invokeWorkspaceCommand = vi.fn().mockResolvedValue({ + sessionsRefreshed: 2, + sessionsFailed: 0, + }); + const publishWorkspaceEvent = vi.fn(); + const svc = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus, + persistDisabledSkillsBatch, + invokeWorkspaceCommand, + publishWorkspaceEvent, + isChannelLive: () => true, + }), + ); + + const result = await svc.setWorkspaceSkillsEnabled( + makeCtx({ originatorClientId: 'client-1' }), + ['Review', 'missing', 'hidden', 'inactive', 'locked', 'deploy'], + false, + ); + + expect(queryWorkspaceStatus).toHaveBeenCalledOnce(); + expect(persistDisabledSkillsBatch).toHaveBeenCalledOnce(); + expect(persistDisabledSkillsBatch).toHaveBeenCalledWith( + '/workspace', + ['review', 'locked', 'deploy'], + false, + undefined, + ); + expect(invokeWorkspaceCommand).toHaveBeenCalledOnce(); + expect(result).toEqual({ + enabled: false, + activation: 'applied', + sessionsRefreshed: 2, + sessionsFailed: 0, + results: [ + { skillName: 'review', enabled: false, changed: true }, + { skillName: 'deploy', enabled: false, changed: true }, + ], + errors: [ + { + skillName: 'missing', + code: 'skill_not_found', + error: 'Skill not found: missing', + }, + { + skillName: 'hidden', + code: 'skill_not_toggleable', + error: 'Skill hidden is not toggleable: not_user_invocable', + reason: 'not_user_invocable', + }, + { + skillName: 'inactive', + code: 'skill_inactive_extension', + error: 'Skill inactive is not toggleable: inactive_extension', + reason: 'inactive_extension', + }, + { + skillName: 'locked', + code: 'skill_not_toggleable', + error: 'Skill locked is locked by user settings', + reason: 'locked', + lockedScope: 'user', + }, + ], + }); + expect(publishWorkspaceEvent).toHaveBeenCalledOnce(); + }); + + it('fails the whole batch when persistence fails unexpectedly', async () => { + const invokeWorkspaceCommand = vi.fn(); + const svc = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch: vi + .fn() + .mockRejectedValue(new Error('disk full')), + invokeWorkspaceCommand, + isChannelLive: () => true, + }), + ); + + await expect( + svc.setWorkspaceSkillsEnabled(makeCtx(), ['review'], false), + ).rejects.toThrow('disk full'); + expect(invokeWorkspaceCommand).not.toHaveBeenCalled(); + }); + + it('returns validation errors without persisting when no target is valid', async () => { + const persistDisabledSkillsBatch = vi.fn(); + const svc = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch, + }), + ); + + await expect( + svc.setWorkspaceSkillsEnabled( + makeCtx(), + ['missing', 'hidden', 'inactive'], + false, + ), + ).resolves.toMatchObject({ + results: [], + errors: [ + { skillName: 'missing', code: 'skill_not_found' }, + { skillName: 'hidden', code: 'skill_not_toggleable' }, + { skillName: 'inactive', code: 'skill_inactive_extension' }, + ], + }); + expect(persistDisabledSkillsBatch).not.toHaveBeenCalled(); + }); + }); + describe('requestWorkspaceTrustChange', () => { it('publishes trust_change_requested with originatorClientId', async () => { const publishWorkspaceEvent = vi.fn(); diff --git a/packages/cli/src/serve/workspace-service/index.ts b/packages/cli/src/serve/workspace-service/index.ts index 1964438cd26..9d097862ae3 100644 --- a/packages/cli/src/serve/workspace-service/index.ts +++ b/packages/cli/src/serve/workspace-service/index.ts @@ -69,6 +69,7 @@ import { } from '../workspace-skill-management.js'; import { + mapWorkspaceSkillToggleError, WorkspacePermissionRulesSessionRequiredError, WorkspaceSkillNotFoundError, WorkspaceSkillNotToggleableError, @@ -84,7 +85,10 @@ import type { WorkspaceVoiceSettingsUpdate, WorkspaceAcpPreheatResult, WorkspaceAcpStatusResult, + WorkspaceSkillBatchToggleResult, + WorkspaceSkillToggleError, WorkspaceSkillToggleResult, + PersistDisabledSkillsBatchResult, WorkspaceSkillInstallRequest, WorkspaceSkillMutationResult, WorkspaceSkillScope, @@ -103,6 +107,10 @@ export type { WorkspaceVoiceSettingsUpdate, WorkspaceAcpPreheatResult, WorkspaceAcpStatusResult, + WorkspaceSkillBatchToggleResult, + WorkspaceSkillBatchToggleItem, + WorkspaceSkillToggleError, + WorkspaceSkillToggleErrorCode, WorkspaceSkillToggleResult, WorkspaceSkillToggleActivation, EnvReloadResult, @@ -113,6 +121,7 @@ export { WorkspacePermissionRulesSessionRequiredError, WorkspaceSkillNotFoundError, WorkspaceSkillNotToggleableError, + mapWorkspaceSkillToggleError, } from './types.js'; // --------------------------------------------------------------------------- @@ -221,6 +230,7 @@ export function createDaemonWorkspaceService( isChannelLive, persistDisabledTools, persistDisabledSkills, + persistDisabledSkillsBatch, persistSetting, persistSettings, skillInstallEnv, @@ -921,6 +931,171 @@ export function createDaemonWorkspaceService( }; }, + async setWorkspaceSkillsEnabled( + ctx: WorkspaceRequestContext, + requestedSkillNames: readonly string[], + enabled: boolean, + ): Promise { + assertActiveGeneration(); + const status = await getWorkspaceSkillsStatus(); + const skillsByName = new Map< + string, + ServeWorkspaceSkillsStatus['skills'][number] + >(); + for (const skill of status.skills) { + const normalizedName = skill.name.trim().toLowerCase(); + if (!skillsByName.has(normalizedName)) { + skillsByName.set(normalizedName, skill); + } + } + const disabledNames = resolveSkillSettings( + loadBoundSettings(true), + ).disabledNames; + const targets: Array< + | { requestedName: string; skillName: string } + | { requestedName: string; error: WorkspaceSkillToggleError } + > = []; + + for (const requestedName of requestedSkillNames) { + const normalizedName = requestedName.trim().toLowerCase(); + const skill = skillsByName.get(normalizedName); + let domainError: unknown; + if (!skill) { + domainError = new WorkspaceSkillNotFoundError(requestedName); + } else if (skill.userInvocable === false) { + domainError = new WorkspaceSkillNotToggleableError( + skill.name, + 'not_user_invocable', + ); + } else { + const legacyInactive = + skill.level === 'extension' && + skill.status === 'disabled' && + skill.disabledReason === undefined && + !disabledNames.has(normalizedName); + if ( + skill.level === 'extension' && + skill.status === 'disabled' && + (skill.disabledReason === 'inactive_extension' || legacyInactive) + ) { + domainError = new WorkspaceSkillNotToggleableError( + skill.name, + 'inactive_extension', + ); + } + } + + if (domainError) { + const error = mapWorkspaceSkillToggleError(domainError); + if (!error) throw domainError; + targets.push({ requestedName, error }); + } else { + targets.push({ requestedName, skillName: skill!.name }); + } + } + + const validSkillNames = targets.flatMap((target) => + 'skillName' in target ? [target.skillName] : [], + ); + const persisted: PersistDisabledSkillsBatchResult = + validSkillNames.length > 0 + ? await persistDisabledSkillsBatch( + boundWorkspace, + validSkillNames, + enabled, + assertGenerationOpen, + ) + : { outcomes: [], settingsChanges: [] }; + assertActiveGeneration(); + const persistedByName = new Map( + persisted.outcomes.map((outcome) => [ + outcome.skillName.trim().toLowerCase(), + outcome, + ]), + ); + const results: WorkspaceSkillBatchToggleResult['results'] = []; + const errors: WorkspaceSkillBatchToggleResult['errors'] = []; + for (const target of targets) { + if ('error' in target) { + errors.push(target.error); + continue; + } + const outcome = persistedByName.get( + target.skillName.trim().toLowerCase(), + ); + if (!outcome) { + throw new Error( + `Missing persisted Skill batch outcome: ${target.skillName}`, + ); + } + if ('error' in outcome) { + const error = mapWorkspaceSkillToggleError(outcome.error); + if (!error) throw outcome.error; + errors.push(error); + } else { + results.push({ + skillName: outcome.skillName, + enabled, + changed: outcome.changed, + }); + } + } + + const changed = results.some((result) => result.changed); + const channelLive = isChannelLive?.() ?? false; + let activation: WorkspaceSkillBatchToggleResult['activation'] = + channelLive ? 'applied' : 'deferred'; + let sessionsRefreshed = 0; + let sessionsFailed = 0; + if (changed) { + invalidateWorkspaceSkillsSnapshot(); + if (channelLive) { + try { + const refreshed = + await invokeWorkspaceCommand( + SERVE_CONTROL_EXT_METHODS.workspaceSkillsRefresh, + { cwd: boundWorkspace, reason: 'settings' }, + ); + assertActiveGeneration(); + sessionsRefreshed = refreshed.sessionsRefreshed; + sessionsFailed = refreshed.sessionsFailed; + if (sessionsFailed > 0) activation = 'partial'; + } catch (err) { + if ( + err instanceof SessionNotFoundError || + err instanceof BridgeChannelClosedError + ) { + activation = 'deferred'; + } else { + activation = 'partial'; + sessionsFailed = 1; + writeStderrLine( + `qwen serve: workspace skill refresh failed: ${err instanceof Error ? err.message : String(err)}`, + ); + } + } + invalidateWorkspaceSkillsSnapshot(); + } + assertActiveGeneration(); + for (const change of persisted.settingsChanges) { + publishWorkspaceEvent({ + type: 'settings_changed', + data: { ...change, scope: 'workspace' }, + originatorClientId: ctx.originatorClientId, + }); + } + } + + return { + enabled, + activation, + sessionsRefreshed, + sessionsFailed, + results, + errors, + }; + }, + async installWorkspaceSkill( _ctx: WorkspaceRequestContext, request: WorkspaceSkillInstallRequest, diff --git a/packages/cli/src/serve/workspace-service/types.ts b/packages/cli/src/serve/workspace-service/types.ts index 9b6b6a05890..f17c6afcd43 100644 --- a/packages/cli/src/serve/workspace-service/types.ts +++ b/packages/cli/src/serve/workspace-service/types.ts @@ -210,6 +210,13 @@ export interface DaemonWorkspaceService { enabled: boolean, ): Promise; + /** Toggle multiple skills with one settings write and one session refresh. */ + setWorkspaceSkillsEnabled( + ctx: WorkspaceRequestContext, + skillNames: readonly string[], + enabled: boolean, + ): Promise; + /** Install a project- or user-level Skill from a bounded package. */ installWorkspaceSkill( ctx: WorkspaceRequestContext, @@ -342,6 +349,34 @@ export interface WorkspaceSkillToggleResult { sessionsFailed: number; } +export type WorkspaceSkillToggleErrorCode = + | 'skill_not_found' + | 'skill_not_toggleable' + | 'skill_inactive_extension'; + +export interface WorkspaceSkillToggleError { + skillName: string; + code: WorkspaceSkillToggleErrorCode; + error: string; + reason?: WorkspaceSkillNotToggleableReason; + lockedScope?: 'system' | 'user' | 'systemDefaults'; +} + +export interface WorkspaceSkillBatchToggleItem { + skillName: string; + enabled: boolean; + changed: boolean; +} + +export interface WorkspaceSkillBatchToggleResult { + enabled: boolean; + activation: WorkspaceSkillToggleActivation; + sessionsRefreshed: number; + sessionsFailed: number; + results: WorkspaceSkillBatchToggleItem[]; + errors: WorkspaceSkillToggleError[]; +} + export interface PersistDisabledSkillResult { changed: boolean; disabled: string[]; @@ -351,6 +386,18 @@ export interface PersistDisabledSkillResult { }>; } +export type PersistDisabledSkillsBatchOutcome = + | { skillName: string; changed: boolean } + | { skillName: string; error: WorkspaceSkillNotToggleableError }; + +export interface PersistDisabledSkillsBatchResult { + outcomes: PersistDisabledSkillsBatchOutcome[]; + settingsChanges: Array<{ + key: 'skills.disabled' | 'skills.enabled'; + value: string[] | undefined; + }>; +} + export type WorkspaceSkillNotToggleableReason = | 'not_user_invocable' | 'inactive_extension' @@ -378,6 +425,31 @@ export class WorkspaceSkillNotToggleableError extends Error { } } +export function mapWorkspaceSkillToggleError( + error: unknown, +): WorkspaceSkillToggleError | undefined { + if (error instanceof WorkspaceSkillNotFoundError) { + return { + skillName: error.skillName, + code: 'skill_not_found', + error: error.message, + }; + } + if (error instanceof WorkspaceSkillNotToggleableError) { + return { + skillName: error.skillName, + code: + error.reason === 'inactive_extension' + ? 'skill_inactive_extension' + : 'skill_not_toggleable', + error: error.message, + reason: error.reason, + ...(error.lockedScope ? { lockedScope: error.lockedScope } : {}), + }; + } + return undefined; +} + /** Discriminated union for MCP server restart outcomes. */ export type RestartMcpServerResult = | { serverName: string; restarted: true; durationMs: number } @@ -471,6 +543,14 @@ export interface DaemonWorkspaceServiceDeps { assertGenerationOpen?: () => void, ) => Promise; + /** Persist multiple skill changes under one settings lock. */ + persistDisabledSkillsBatch: ( + workspace: string, + skillNames: readonly string[], + enabled: boolean, + assertGenerationOpen?: () => void, + ) => Promise; + persistSetting?: ( workspace: string, scope: SettingScope, diff --git a/packages/sdk-typescript/src/daemon/index.ts b/packages/sdk-typescript/src/daemon/index.ts index 0d6ac52d766..5e28af44eda 100644 --- a/packages/sdk-typescript/src/daemon/index.ts +++ b/packages/sdk-typescript/src/daemon/index.ts @@ -394,6 +394,7 @@ export type { DaemonToolToggleResult, DaemonSkillBatchToggleError, DaemonSkillBatchToggleErrorCode, + DaemonSkillBatchToggleItem, DaemonSkillBatchToggleResult, DaemonSkillToggleActivation, DaemonSkillToggleResult, diff --git a/packages/sdk-typescript/src/daemon/types.ts b/packages/sdk-typescript/src/daemon/types.ts index eba58655dd3..40f61ded225 100644 --- a/packages/sdk-typescript/src/daemon/types.ts +++ b/packages/sdk-typescript/src/daemon/types.ts @@ -2500,8 +2500,7 @@ export interface DaemonSkillToggleResult { export type DaemonSkillBatchToggleErrorCode = | 'skill_not_found' | 'skill_not_toggleable' - | 'skill_inactive_extension' - | 'skill_toggle_failed'; + | 'skill_inactive_extension'; export interface DaemonSkillBatchToggleError { skillName: string; @@ -2513,10 +2512,19 @@ export interface DaemonSkillBatchToggleError { export interface DaemonSkillBatchToggleResult { enabled: boolean; - results: DaemonSkillToggleResult[]; + activation: DaemonSkillToggleActivation; + sessionsRefreshed: number; + sessionsFailed: number; + results: DaemonSkillBatchToggleItem[]; errors: DaemonSkillBatchToggleError[]; } +export interface DaemonSkillBatchToggleItem { + skillName: string; + enabled: boolean; + changed: boolean; +} + export type DaemonSkillScope = 'workspace' | 'global'; export type DaemonSkillInstallSource = diff --git a/packages/sdk-typescript/src/index.ts b/packages/sdk-typescript/src/index.ts index 4caa4c2f010..54b83297ada 100644 --- a/packages/sdk-typescript/src/index.ts +++ b/packages/sdk-typescript/src/index.ts @@ -96,6 +96,7 @@ export { type DaemonToolToggleResult, type DaemonSkillBatchToggleError, type DaemonSkillBatchToggleErrorCode, + type DaemonSkillBatchToggleItem, type DaemonSkillBatchToggleResult, type DaemonSkillToggleActivation, type DaemonSkillToggleResult, diff --git a/packages/sdk-typescript/test/unit/DaemonClient.test.ts b/packages/sdk-typescript/test/unit/DaemonClient.test.ts index 378385b6b72..0723b420ab0 100644 --- a/packages/sdk-typescript/test/unit/DaemonClient.test.ts +++ b/packages/sdk-typescript/test/unit/DaemonClient.test.ts @@ -4043,14 +4043,14 @@ describe('DaemonClient', () => { describe('setWorkspaceSkillsEnabled', () => { const response = { enabled: false, + activation: 'applied', + sessionsRefreshed: 2, + sessionsFailed: 0, results: [ { skillName: 'review', enabled: false, changed: true, - activation: 'applied', - sessionsRefreshed: 2, - sessionsFailed: 0, }, ], errors: [], From 960df141c2996da762a07b40be8c8d5d4dbe5874 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8F=B6=E5=85=AC?= Date: Fri, 7 Aug 2026 13:43:54 +0000 Subject: [PATCH 4/9] test(daemon): pin Skill batch toggle contracts and fix docs examples --- docs/design/daemon-skill-batch-toggle.md | 9 +- docs/developers/qwen-serve-protocol.md | 7 +- packages/cli/src/serve/run-qwen-serve.test.ts | 49 +++- .../__tests__/facade.test.ts | 272 ++++++++++++++++++ .../cli/src/serve/workspace-service/index.ts | 3 + .../test/unit/DaemonClient.test.ts | 1 + .../test/unit/daemon-public-surface.test.ts | 10 + 7 files changed, 347 insertions(+), 4 deletions(-) diff --git a/docs/design/daemon-skill-batch-toggle.md b/docs/design/daemon-skill-batch-toggle.md index 6f80e1abf6f..e31bd3622c1 100644 --- a/docs/design/daemon-skill-batch-toggle.md +++ b/docs/design/daemon-skill-batch-toggle.md @@ -17,7 +17,7 @@ The request body is: ```json { - "skillNames": ["review", "deploy"], + "skillNames": ["review", "deploy", "missing"], "enabled": false } ``` @@ -34,13 +34,18 @@ persistence and runtime-generation failures fail the whole request. { "enabled": false, "activation": "applied", - "sessionsRefreshed": 1, + "sessionsRefreshed": 2, "sessionsFailed": 0, "results": [ { "skillName": "review", "enabled": false, "changed": true + }, + { + "skillName": "deploy", + "enabled": false, + "changed": true } ], "errors": [ diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index 15dd82cb63c..604428b4654 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -2686,7 +2686,7 @@ Request: ```json { - "skillNames": ["review", "deploy"], + "skillNames": ["review", "deploy", "missing"], "enabled": false } ``` @@ -2704,6 +2704,11 @@ Response (200): "skillName": "review", "enabled": false, "changed": true + }, + { + "skillName": "deploy", + "enabled": false, + "changed": true } ], "errors": [ diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index 2448f107e39..7739bce8ac9 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -798,7 +798,12 @@ describe('workspace skill settings persistence', () => { ); fs.writeFileSync( path.join(qwenHome, 'settings.json'), - JSON.stringify({ skills: { disabled: ['locked-skill'] } }), + JSON.stringify({ + skills: { + disabled: ['locked-skill'], + defaultDisabled: ['opt-in'], + }, + }), ); const originalCreateServeApp = serverModule.createServeApp; @@ -854,6 +859,48 @@ describe('workspace skill settings persistence', () => { }, ]); expect(setValues).toHaveBeenCalledOnce(); + + const savedAfterDisable = JSON.parse( + fs.readFileSync(path.join(workspace, '.qwen', 'settings.json'), 'utf8'), + ) as { skills: { disabled: string[]; enabled?: string[] } }; + expect(savedAfterDisable.skills.disabled).toEqual([ + 'orphan', + 'review', + 'alpha', + ]); + expect(savedAfterDisable.skills.enabled).toBeUndefined(); + const savedUser = JSON.parse( + fs.readFileSync(path.join(qwenHome, 'settings.json'), 'utf8'), + ) as { skills: { disabled: string[]; enabled?: string[] } }; + expect(savedUser.skills.disabled).toEqual(['locked-skill']); + expect(savedUser.skills.enabled).toBeUndefined(); + + const enableResult = await persistDisabledSkillsBatch!( + workspace, + ['opt-in'], + true, + ); + + expect(enableResult.outcomes).toEqual([ + { skillName: 'opt-in', changed: true }, + ]); + expect(enableResult.settingsChanges).toEqual([ + { + key: 'skills.enabled', + value: ['opt-in'], + }, + ]); + expect(setValues).toHaveBeenCalledTimes(2); + + const savedAfterEnable = JSON.parse( + fs.readFileSync(path.join(workspace, '.qwen', 'settings.json'), 'utf8'), + ) as { skills: { disabled: string[]; enabled: string[] } }; + expect(savedAfterEnable.skills.disabled).toEqual([ + 'orphan', + 'review', + 'alpha', + ]); + expect(savedAfterEnable.skills.enabled).toEqual(['opt-in']); }); }); diff --git a/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts b/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts index 9528db0002d..97cdbde671f 100644 --- a/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts +++ b/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts @@ -2087,6 +2087,15 @@ describe('createDaemonWorkspaceService', () => { disabledReason: 'hard' as const, lockedScope: 'user' as const, }, + { + kind: 'skill' as const, + status: 'disabled' as const, + name: 'legacy-inactive', + description: 'Legacy inactive extension skill', + level: 'extension' as const, + extensionName: 'demo', + modelInvocable: true, + }, ]; it('persists and refreshes once while preserving ordered target outcomes', async () => { @@ -2143,6 +2152,10 @@ describe('createDaemonWorkspaceService', () => { undefined, ); expect(invokeWorkspaceCommand).toHaveBeenCalledOnce(); + expect(invokeWorkspaceCommand).toHaveBeenCalledWith( + 'qwen/control/workspace/skills/refresh', + { cwd: '/workspace', reason: 'settings' }, + ); expect(result).toEqual({ enabled: false, activation: 'applied', @@ -2180,10 +2193,20 @@ describe('createDaemonWorkspaceService', () => { ], }); expect(publishWorkspaceEvent).toHaveBeenCalledOnce(); + expect(publishWorkspaceEvent).toHaveBeenCalledWith({ + type: 'settings_changed', + data: { + key: 'skills.disabled', + value: ['review', 'deploy'], + scope: 'workspace', + }, + originatorClientId: 'client-1', + }); }); it('fails the whole batch when persistence fails unexpectedly', async () => { const invokeWorkspaceCommand = vi.fn(); + const publishWorkspaceEvent = vi.fn(); const svc = createDaemonWorkspaceService( makeDeps({ queryWorkspaceStatus: vi.fn().mockResolvedValue({ @@ -2196,6 +2219,7 @@ describe('createDaemonWorkspaceService', () => { .fn() .mockRejectedValue(new Error('disk full')), invokeWorkspaceCommand, + publishWorkspaceEvent, isChannelLive: () => true, }), ); @@ -2204,6 +2228,7 @@ describe('createDaemonWorkspaceService', () => { svc.setWorkspaceSkillsEnabled(makeCtx(), ['review'], false), ).rejects.toThrow('disk full'); expect(invokeWorkspaceCommand).not.toHaveBeenCalled(); + expect(publishWorkspaceEvent).not.toHaveBeenCalled(); }); it('returns validation errors without persisting when no target is valid', async () => { @@ -2236,6 +2261,253 @@ describe('createDaemonWorkspaceService', () => { }); expect(persistDisabledSkillsBatch).not.toHaveBeenCalled(); }); + + it('rejects a legacy inactive extension skill like the single-toggle path', async () => { + const persistDisabledSkillsBatch = vi.fn(); + const svc = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch, + }), + ); + + await expect( + svc.setWorkspaceSkillsEnabled(makeCtx(), ['legacy-inactive'], false), + ).resolves.toMatchObject({ + results: [], + errors: [ + { + skillName: 'legacy-inactive', + code: 'skill_inactive_extension', + reason: 'inactive_extension', + }, + ], + }); + expect(persistDisabledSkillsBatch).not.toHaveBeenCalled(); + }); + + it('reports partial activation when the shared batch refresh fails', async () => { + const failedSessions = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch: vi.fn().mockResolvedValue({ + outcomes: [{ skillName: 'review', changed: true }], + settingsChanges: [{ key: 'skills.disabled', value: ['review'] }], + }), + invokeWorkspaceCommand: vi.fn().mockResolvedValue({ + sessionsRefreshed: 1, + sessionsFailed: 1, + }), + isChannelLive: () => true, + }), + ); + await expect( + failedSessions.setWorkspaceSkillsEnabled(makeCtx(), ['review'], false), + ).resolves.toMatchObject({ + activation: 'partial', + sessionsRefreshed: 1, + sessionsFailed: 1, + }); + + const unexpectedError = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch: vi.fn().mockResolvedValue({ + outcomes: [{ skillName: 'review', changed: true }], + settingsChanges: [{ key: 'skills.disabled', value: ['review'] }], + }), + invokeWorkspaceCommand: vi + .fn() + .mockRejectedValue(new Error('network timeout')), + isChannelLive: () => true, + }), + ); + await expect( + unexpectedError.setWorkspaceSkillsEnabled(makeCtx(), ['review'], false), + ).resolves.toMatchObject({ + activation: 'partial', + sessionsRefreshed: 0, + sessionsFailed: 1, + }); + }); + + it('defers batch refresh when sessions or the channel disappear', async () => { + const missingSession = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch: vi.fn().mockResolvedValue({ + outcomes: [{ skillName: 'review', changed: true }], + settingsChanges: [{ key: 'skills.disabled', value: ['review'] }], + }), + invokeWorkspaceCommand: vi + .fn() + .mockRejectedValue(new SessionNotFoundError('session-1')), + isChannelLive: () => true, + }), + ); + await expect( + missingSession.setWorkspaceSkillsEnabled(makeCtx(), ['review'], false), + ).resolves.toMatchObject({ + activation: 'deferred', + sessionsRefreshed: 0, + sessionsFailed: 0, + }); + + const closedChannel = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch: vi.fn().mockResolvedValue({ + outcomes: [{ skillName: 'review', changed: true }], + settingsChanges: [{ key: 'skills.disabled', value: ['review'] }], + }), + invokeWorkspaceCommand: vi + .fn() + .mockRejectedValue( + new BridgeChannelClosedError('mid-request (batch toggle)'), + ), + isChannelLive: () => true, + }), + ); + await expect( + closedChannel.setWorkspaceSkillsEnabled(makeCtx(), ['review'], false), + ).resolves.toMatchObject({ + activation: 'deferred', + sessionsRefreshed: 0, + sessionsFailed: 0, + }); + + const deadChannelCommand = vi.fn(); + const deadChannel = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch: vi.fn().mockResolvedValue({ + outcomes: [{ skillName: 'review', changed: true }], + settingsChanges: [{ key: 'skills.disabled', value: ['review'] }], + }), + invokeWorkspaceCommand: deadChannelCommand, + isChannelLive: () => false, + }), + ); + await expect( + deadChannel.setWorkspaceSkillsEnabled(makeCtx(), ['review'], false), + ).resolves.toMatchObject({ + activation: 'deferred', + sessionsRefreshed: 0, + sessionsFailed: 0, + }); + expect(deadChannelCommand).not.toHaveBeenCalled(); + }); + + it('does not refresh or publish when every batch target is unchanged', async () => { + const invokeWorkspaceCommand = vi.fn(); + const publishWorkspaceEvent = vi.fn(); + const svc = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch: vi.fn().mockResolvedValue({ + outcomes: [ + { skillName: 'review', changed: false }, + { skillName: 'deploy', changed: false }, + ], + settingsChanges: [], + }), + invokeWorkspaceCommand, + publishWorkspaceEvent, + isChannelLive: () => true, + }), + ); + + await expect( + svc.setWorkspaceSkillsEnabled(makeCtx(), ['review', 'deploy'], false), + ).resolves.toMatchObject({ + activation: 'applied', + sessionsRefreshed: 0, + }); + expect(invokeWorkspaceCommand).not.toHaveBeenCalled(); + expect(publishWorkspaceEvent).not.toHaveBeenCalled(); + }); + + it('drops the cached skill snapshot after a changed batch like the single-toggle path', async () => { + const beforeStatus = { + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }; + const afterStatus = { + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills: [ + { + kind: 'skill', + status: 'disabled', + name: 'review', + description: 'Review changed code', + level: 'bundled', + modelInvocable: true, + }, + ], + }; + const queryWorkspaceStatus = vi + .fn() + .mockResolvedValueOnce(beforeStatus) + .mockResolvedValueOnce(afterStatus); + const svc = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus, + persistDisabledSkillsBatch: vi.fn().mockResolvedValue({ + outcomes: [{ skillName: 'review', changed: true }], + settingsChanges: [{ key: 'skills.disabled', value: ['review'] }], + }), + isChannelLive: () => false, + }), + ); + + await expect(svc.getWorkspaceSkillsStatus(makeCtx())).resolves.toEqual( + beforeStatus, + ); + await svc.setWorkspaceSkillsEnabled(makeCtx(), ['review'], false); + await expect(svc.getWorkspaceSkillsStatus(makeCtx())).resolves.toEqual( + afterStatus, + ); + expect(queryWorkspaceStatus).toHaveBeenCalledTimes(2); + }); }); describe('requestWorkspaceTrustChange', () => { diff --git a/packages/cli/src/serve/workspace-service/index.ts b/packages/cli/src/serve/workspace-service/index.ts index 9d097862ae3..599043574d6 100644 --- a/packages/cli/src/serve/workspace-service/index.ts +++ b/packages/cli/src/serve/workspace-service/index.ts @@ -1058,6 +1058,9 @@ export function createDaemonWorkspaceService( ); assertActiveGeneration(); sessionsRefreshed = refreshed.sessionsRefreshed; + // `reason: 'settings'` never touches skill caches, so + // `configsFailed` is structurally 0 here — folding it in would only + // conflate two different failures behind one count. sessionsFailed = refreshed.sessionsFailed; if (sessionsFailed > 0) activation = 'partial'; } catch (err) { diff --git a/packages/sdk-typescript/test/unit/DaemonClient.test.ts b/packages/sdk-typescript/test/unit/DaemonClient.test.ts index 0723b420ab0..7d5e13d0d75 100644 --- a/packages/sdk-typescript/test/unit/DaemonClient.test.ts +++ b/packages/sdk-typescript/test/unit/DaemonClient.test.ts @@ -4075,6 +4075,7 @@ describe('DaemonClient', () => { enabled: false, }), }); + expect(calls[0]?.headers['content-type']).toBe('application/json'); expect(calls[0]?.headers['x-qwen-client-id']).toBe('client-1'); }); diff --git a/packages/sdk-typescript/test/unit/daemon-public-surface.test.ts b/packages/sdk-typescript/test/unit/daemon-public-surface.test.ts index 2e110db031f..38e14d51fe5 100644 --- a/packages/sdk-typescript/test/unit/daemon-public-surface.test.ts +++ b/packages/sdk-typescript/test/unit/daemon-public-surface.test.ts @@ -77,6 +77,10 @@ import type { DaemonSessionDiedEvent, DaemonSessionEvent, DaemonSessionRecapResult, + DaemonSkillBatchToggleError, + DaemonSkillBatchToggleErrorCode, + DaemonSkillBatchToggleItem, + DaemonSkillBatchToggleResult, DaemonSessionRecordingDegradedData, DaemonSessionRecordingDegradedEvent, DaemonSessionUpdateData, @@ -291,6 +295,12 @@ describe('public SDK entry — typed daemon event surface (#4217)', () => { expectTypeOf().not.toBeNever(); expectTypeOf().not.toBeNever(); expectTypeOf().not.toBeNever(); + // Batch Skill toggle surface: clients type the result of + // `client.setWorkspaceSkillsEnabled(...)` through the published entry. + expectTypeOf().not.toBeNever(); + expectTypeOf().not.toBeNever(); + expectTypeOf().not.toBeNever(); + expectTypeOf().not.toBeNever(); // `GET /daemon/status` report surface (PR 5174 client coverage): the // envelope plus the sub-shapes UI dashboards need to type against. expectTypeOf().not.toBeNever(); From 8df9605820ecbfab28e9c17c33016fafa2e079ff Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 7 Aug 2026 17:55:48 +0000 Subject: [PATCH 5/9] test(daemon): pin Skill batch toggle mutants flagged in review --- .../src/serve/routes/workspace-skills.test.ts | 11 + packages/cli/src/serve/run-qwen-serve.test.ts | 30 ++ packages/cli/src/serve/server.test.ts | 37 +++ .../serve/workspace-qualified-rest.test.ts | 52 ++++ .../__tests__/facade.test.ts | 262 +++++++++++++++++- .../test/unit/DaemonClient.test.ts | 29 ++ 6 files changed, 408 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/serve/routes/workspace-skills.test.ts b/packages/cli/src/serve/routes/workspace-skills.test.ts index d5667af8a36..75ae30dd671 100644 --- a/packages/cli/src/serve/routes/workspace-skills.test.ts +++ b/packages/cli/src/serve/routes/workspace-skills.test.ts @@ -273,6 +273,13 @@ describe('workspace Skill management routes', () => { enabled: false, }); + const missingNames = await request(harness.app) + .post('/workspace/skills/enable') + .send({ enabled: false }); + const nonArrayNames = await request(harness.app) + .post('/workspace/skills/enable') + .send({ skillNames: 'review', enabled: false }); + expect(empty.status).toBe(400); expect(empty.body.code).toBe('invalid_skill_names'); expect(tooMany.status).toBe(400); @@ -283,6 +290,10 @@ describe('workspace Skill management routes', () => { expect(invalidFlag.body.code).toBe('invalid_enabled_flag'); expect(nonString.status).toBe(400); expect(nonString.body.code).toBe('invalid_skill_names'); + expect(missingNames.status).toBe(400); + expect(missingNames.body.code).toBe('invalid_skill_names'); + expect(nonArrayNames.status).toBe(400); + expect(nonArrayNames.body.code).toBe('invalid_skill_names'); expect(tooLong.status).toBe(400); expect(tooLong.body.code).toBe('invalid_skill_name'); expect(exactLimit.status).toBe(200); diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index 7739bce8ac9..461f39ddeae 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -860,6 +860,17 @@ describe('workspace skill settings persistence', () => { ]); expect(setValues).toHaveBeenCalledOnce(); + const noopResult = await persistDisabledSkillsBatch!( + workspace, + ['review'], + false, + ); + expect(noopResult.outcomes).toEqual([ + { skillName: 'review', changed: false }, + ]); + expect(noopResult.settingsChanges).toEqual([]); + expect(setValues).toHaveBeenCalledOnce(); + const savedAfterDisable = JSON.parse( fs.readFileSync(path.join(workspace, '.qwen', 'settings.json'), 'utf8'), ) as { skills: { disabled: string[]; enabled?: string[] } }; @@ -901,6 +912,25 @@ describe('workspace skill settings persistence', () => { 'alpha', ]); expect(savedAfterEnable.skills.enabled).toEqual(['opt-in']); + + const guard = vi.fn(); + await persistDisabledSkillsBatch!(workspace, ['guarded'], false, guard); + expect(setValues.mock.calls.at(-1)?.[2]).toBe(guard); + expect(guard).toHaveBeenCalled(); + + const blockingGuard = vi.fn(() => { + throw new Error('generation closed'); + }); + const writesBefore = setValues.mock.calls.length; + await expect( + persistDisabledSkillsBatch!( + workspace, + ['guarded-too'], + false, + blockingGuard, + ), + ).rejects.toThrow('generation closed'); + expect(setValues.mock.calls).toHaveLength(writesBefore); }); }); diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 87221dc485d..ca73e8ada9b 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -17828,6 +17828,43 @@ describe('createServeApp', () => { expect(res.status).toBe(403); expect(res.body.code).toBe('untrusted_workspace'); }); + + it('rejects Skill batch writes to an untrusted primary workspace', async () => { + const persistDisabledSkillsBatch = vi.fn(); + const app = createServeApp(tokenOpts, undefined, { + bridge: fakeBridge(), + persistDisabledSkillsBatch, + }); + const res = await auth( + request(app).post('/workspace/skills/enable'), + ).send({ skillNames: ['review'], enabled: false }); + expect(res.status).toBe(403); + expect(res.body.code).toBe('untrusted_workspace'); + expect(persistDisabledSkillsBatch).not.toHaveBeenCalled(); + }); + + it('rejects an unknown workspace client id before Skill batch persistence', async () => { + const persistDisabledSkillsBatch = vi.fn(); + const app = createServeApp(tokenOpts, undefined, { + bridge: fakeBridge({ + workspaceSkillsImpl: async () => ({ + v: 1, + workspaceCwd: WS_BOUND, + initialized: true, + skills: [reviewSkill], + }), + }), + boundWorkspace: WS_BOUND, + persistDisabledSkillsBatch, + primaryWorkspaceTrusted: true, + }); + const res = await auth(request(app).post('/workspace/skills/enable')) + .set('X-Qwen-Client-Id', 'forged-client') + .send({ skillNames: ['review'], enabled: false }); + expect(res.status).toBe(400); + expect(res.body.code).toBe('invalid_client_id'); + expect(persistDisabledSkillsBatch).not.toHaveBeenCalled(); + }); }); describe('POST /session/:id/permission/:requestId', () => { diff --git a/packages/cli/src/serve/workspace-qualified-rest.test.ts b/packages/cli/src/serve/workspace-qualified-rest.test.ts index 34301fbc47b..cd639449bf8 100644 --- a/packages/cli/src/serve/workspace-qualified-rest.test.ts +++ b/packages/cli/src/serve/workspace-qualified-rest.test.ts @@ -1056,6 +1056,58 @@ describe('workspace-qualified core REST', () => { expect( h.secondaryWorkspaceService.setWorkspaceSkillsEnabled, ).toHaveBeenCalledTimes(1); + + const badBatchBody = await request(h.app) + .post(`/workspaces/${encodeURIComponent(h.secondaryId)}/skills/enable`) + .set('Authorization', 'Bearer secret') + .set('X-Qwen-Client-Id', 'client-1') + .set('Host', host()) + .send({ skillNames: [], enabled: false }); + expect(badBatchBody.status).toBe(400); + expect(badBatchBody.body.code).toBe('invalid_skill_names'); + expect( + h.secondaryWorkspaceService.setWorkspaceSkillsEnabled, + ).toHaveBeenCalledTimes(1); + + const enableBatch = await request(h.app) + .post(`/workspaces/${encodeURIComponent(h.secondaryId)}/skills/enable`) + .set('Authorization', 'Bearer secret') + .set('X-Qwen-Client-Id', 'client-1') + .set('Host', host()) + .send({ skillNames: ['review'], enabled: true }); + expect(enableBatch.status).toBe(200); + expect(enableBatch.body).toMatchObject({ + enabled: true, + results: [{ skillName: 'review', enabled: true, changed: true }], + }); + expect( + h.secondaryWorkspaceService.setWorkspaceSkillsEnabled, + ).toHaveBeenCalledWith( + expect.objectContaining({ + workspaceCwd: h.secondaryCwd, + originatorClientId: 'client-1', + }), + ['review'], + true, + ); + expect( + h.secondaryWorkspaceService.setWorkspaceSkillsEnabled, + ).toHaveBeenCalledTimes(2); + + vi.mocked( + h.secondaryWorkspaceService.setWorkspaceSkillsEnabled, + ).mockRejectedValueOnce(new Error('disk full')); + const failedBatch = await request(h.app) + .post(`/workspaces/${encodeURIComponent(h.secondaryId)}/skills/enable`) + .set('Authorization', 'Bearer secret') + .set('X-Qwen-Client-Id', 'client-1') + .set('Host', host()) + .send({ skillNames: ['review'], enabled: false }); + expect(failedBatch.status).toBe(500); + expect(failedBatch.body.error).toBe('disk full'); + expect( + h.secondaryWorkspaceService.setWorkspaceSkillsEnabled, + ).toHaveBeenCalledTimes(3); } finally { await fsp.rm(h.scratch, { recursive: true, force: true }); } diff --git a/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts b/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts index 97cdbde671f..019b001629e 100644 --- a/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts +++ b/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts @@ -2204,6 +2204,60 @@ describe('createDaemonWorkspaceService', () => { }); }); + it('orders results and errors by request targets, not persist outcomes', async () => { + const svc = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch: vi.fn().mockResolvedValue({ + outcomes: [ + { skillName: 'deploy', changed: true }, + { + skillName: 'locked', + error: new WorkspaceSkillNotToggleableError( + 'locked', + 'locked', + 'user', + ), + }, + { skillName: 'review', changed: true }, + ], + settingsChanges: [], + }), + isChannelLive: () => false, + }), + ); + + const result = await svc.setWorkspaceSkillsEnabled( + makeCtx(), + ['review', 'locked', 'missing', 'deploy'], + false, + ); + + expect(result.results).toEqual([ + { skillName: 'review', enabled: false, changed: true }, + { skillName: 'deploy', enabled: false, changed: true }, + ]); + expect(result.errors).toEqual([ + { + skillName: 'locked', + code: 'skill_not_toggleable', + error: 'Skill locked is locked by user settings', + reason: 'locked', + lockedScope: 'user', + }, + { + skillName: 'missing', + code: 'skill_not_found', + error: 'Skill not found: missing', + }, + ]); + }); + it('fails the whole batch when persistence fails unexpectedly', async () => { const invokeWorkspaceCommand = vi.fn(); const publishWorkspaceEvent = vi.fn(); @@ -2263,7 +2317,85 @@ describe('createDaemonWorkspaceService', () => { }); it('rejects a legacy inactive extension skill like the single-toggle path', async () => { - const persistDisabledSkillsBatch = vi.fn(); + await withIsolatedWorkspace(async ({ workspace }) => { + const persistDisabledSkillsBatch = vi.fn(); + const svc = createDaemonWorkspaceService( + makeDeps({ + boundWorkspace: workspace, + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: workspace, + initialized: true, + skills, + }), + persistDisabledSkillsBatch, + }), + ); + + await expect( + svc.setWorkspaceSkillsEnabled(makeCtx(), ['legacy-inactive'], false), + ).resolves.toMatchObject({ + results: [], + errors: [ + { + skillName: 'legacy-inactive', + code: 'skill_inactive_extension', + reason: 'inactive_extension', + }, + ], + }); + expect(persistDisabledSkillsBatch).not.toHaveBeenCalled(); + }); + }); + + it('allows batch-toggling a legacy extension skill disabled by settings', async () => { + await withIsolatedWorkspace(async ({ workspace }) => { + await writeJson( + path.join(workspace, SETTINGS_DIRECTORY_NAME, 'settings.json'), + { skills: { disabled: ['legacy-inactive'] } }, + ); + const persistDisabledSkillsBatch = vi.fn().mockResolvedValue({ + outcomes: [{ skillName: 'legacy-inactive', changed: true }], + settingsChanges: [{ key: 'skills.disabled', value: undefined }], + }); + const svc = createDaemonWorkspaceService( + makeDeps({ + boundWorkspace: workspace, + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: workspace, + initialized: true, + skills, + }), + persistDisabledSkillsBatch, + isChannelLive: () => false, + }), + ); + + await expect( + svc.setWorkspaceSkillsEnabled(makeCtx(), ['legacy-inactive'], true), + ).resolves.toMatchObject({ + enabled: true, + results: [ + { skillName: 'legacy-inactive', enabled: true, changed: true }, + ], + errors: [], + }); + expect(persistDisabledSkillsBatch).toHaveBeenCalledWith( + workspace, + ['legacy-inactive'], + true, + undefined, + ); + }); + }); + + it('passes enabled:true through to persistence, results, and events', async () => { + const publishWorkspaceEvent = vi.fn(); + const persistDisabledSkillsBatch = vi.fn().mockResolvedValue({ + outcomes: [{ skillName: 'review', changed: true }], + settingsChanges: [{ key: 'skills.disabled', value: undefined }], + }); const svc = createDaemonWorkspaceService( makeDeps({ queryWorkspaceStatus: vi.fn().mockResolvedValue({ @@ -2273,22 +2405,86 @@ describe('createDaemonWorkspaceService', () => { skills, }), persistDisabledSkillsBatch, + publishWorkspaceEvent, + isChannelLive: () => false, }), ); - await expect( - svc.setWorkspaceSkillsEnabled(makeCtx(), ['legacy-inactive'], false), - ).resolves.toMatchObject({ - results: [], - errors: [ - { - skillName: 'legacy-inactive', - code: 'skill_inactive_extension', - reason: 'inactive_extension', - }, - ], + const result = await svc.setWorkspaceSkillsEnabled( + makeCtx({ originatorClientId: 'client-1' }), + ['review'], + true, + ); + + expect(persistDisabledSkillsBatch).toHaveBeenCalledWith( + '/workspace', + ['review'], + true, + undefined, + ); + expect(result).toMatchObject({ + enabled: true, + results: [{ skillName: 'review', enabled: true, changed: true }], + }); + expect(publishWorkspaceEvent).toHaveBeenCalledOnce(); + expect(publishWorkspaceEvent).toHaveBeenCalledWith({ + type: 'settings_changed', + data: { + key: 'skills.disabled', + value: undefined, + scope: 'workspace', + }, + originatorClientId: 'client-1', + }); + }); + + it('publishes one settings_changed event per settingsChanges entry in order', async () => { + const publishWorkspaceEvent = vi.fn(); + const svc = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch: vi.fn().mockResolvedValue({ + outcomes: [{ skillName: 'review', changed: true }], + settingsChanges: [ + { key: 'skills.disabled', value: undefined }, + { key: 'skills.enabled', value: ['review'] }, + ], + }), + publishWorkspaceEvent, + isChannelLive: () => false, + }), + ); + + await svc.setWorkspaceSkillsEnabled( + makeCtx({ originatorClientId: 'client-1' }), + ['review'], + true, + ); + + expect(publishWorkspaceEvent).toHaveBeenCalledTimes(2); + expect(publishWorkspaceEvent).toHaveBeenNthCalledWith(1, { + type: 'settings_changed', + data: { + key: 'skills.disabled', + value: undefined, + scope: 'workspace', + }, + originatorClientId: 'client-1', + }); + expect(publishWorkspaceEvent).toHaveBeenNthCalledWith(2, { + type: 'settings_changed', + data: { + key: 'skills.enabled', + value: ['review'], + scope: 'workspace', + }, + originatorClientId: 'client-1', }); - expect(persistDisabledSkillsBatch).not.toHaveBeenCalled(); }); it('reports partial activation when the shared batch refresh fails', async () => { @@ -2462,6 +2658,46 @@ describe('createDaemonWorkspaceService', () => { expect(publishWorkspaceEvent).not.toHaveBeenCalled(); }); + it('refreshes and publishes when any batch target changed', async () => { + const invokeWorkspaceCommand = vi.fn().mockResolvedValue({ + sessionsRefreshed: 1, + sessionsFailed: 0, + }); + const publishWorkspaceEvent = vi.fn(); + const svc = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch: vi.fn().mockResolvedValue({ + outcomes: [ + { skillName: 'review', changed: false }, + { skillName: 'deploy', changed: true }, + ], + settingsChanges: [{ key: 'skills.disabled', value: ['deploy'] }], + }), + invokeWorkspaceCommand, + publishWorkspaceEvent, + isChannelLive: () => true, + }), + ); + + await expect( + svc.setWorkspaceSkillsEnabled(makeCtx(), ['review', 'deploy'], false), + ).resolves.toMatchObject({ + activation: 'applied', + results: [ + { skillName: 'review', enabled: false, changed: false }, + { skillName: 'deploy', enabled: false, changed: true }, + ], + }); + expect(invokeWorkspaceCommand).toHaveBeenCalledOnce(); + expect(publishWorkspaceEvent).toHaveBeenCalledOnce(); + }); + it('drops the cached skill snapshot after a changed batch like the single-toggle path', async () => { const beforeStatus = { v: 1, diff --git a/packages/sdk-typescript/test/unit/DaemonClient.test.ts b/packages/sdk-typescript/test/unit/DaemonClient.test.ts index 7d5e13d0d75..b4996580e2e 100644 --- a/packages/sdk-typescript/test/unit/DaemonClient.test.ts +++ b/packages/sdk-typescript/test/unit/DaemonClient.test.ts @@ -4102,6 +4102,35 @@ describe('DaemonClient', () => { expect(calls[0]?.headers['x-qwen-client-id']).toBe('client-2'); }); + it('POSTs enabled:true unchanged on the primary and qualified helpers', async () => { + const { fetch, calls } = recordingFetch(() => + jsonResponse(200, response), + ); + const client = new DaemonClient({ baseUrl: 'http://daemon', fetch }); + + await client.setWorkspaceSkillsEnabled(['review', 'deploy'], true); + await client + .workspaceByCwd('/tmp/work space') + .setWorkspaceSkillsEnabled(['review', 'deploy'], true); + + expect(calls[0]).toMatchObject({ + url: 'http://daemon/workspace/skills/enable', + method: 'POST', + body: JSON.stringify({ + skillNames: ['review', 'deploy'], + enabled: true, + }), + }); + expect(calls[1]).toMatchObject({ + url: 'http://daemon/workspaces/%2Ftmp%2Fwork%20space/skills/enable', + method: 'POST', + body: JSON.stringify({ + skillNames: ['review', 'deploy'], + enabled: true, + }), + }); + }); + it('passes request-level errors through', async () => { const { fetch } = recordingFetch(() => jsonResponse(400, { From e8df05367e8413d28d820bb1141d0f9a625462a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8F=B6=E5=85=AC?= Date: Sat, 8 Aug 2026 09:52:29 +0800 Subject: [PATCH 6/9] test(daemon): cover Skill batch toggle edge cases --- .../src/serve/routes/workspace-skills.test.ts | 47 +++++++- packages/cli/src/serve/run-qwen-serve.test.ts | 19 +++ .../serve/workspace-qualified-rest.test.ts | 38 +++++- .../__tests__/facade.test.ts | 113 ++++++++++++++++-- .../test/unit/DaemonClient.test.ts | 12 +- 5 files changed, 207 insertions(+), 22 deletions(-) diff --git a/packages/cli/src/serve/routes/workspace-skills.test.ts b/packages/cli/src/serve/routes/workspace-skills.test.ts index d5667af8a36..e03ff29e827 100644 --- a/packages/cli/src/serve/routes/workspace-skills.test.ts +++ b/packages/cli/src/serve/routes/workspace-skills.test.ts @@ -11,7 +11,10 @@ import { WorkspaceSkillManagementError } from '../workspace-skill-management.js' import type { WorkspaceSkillBatchToggleResult } from '../workspace-service/types.js'; import { registerWorkspaceSkillsRoutes } from './workspace-skills.js'; -function createHarness() { +function createHarness(options?: { + trusted?: boolean; + rejectClientId?: boolean; +}) { const installWorkspaceSkill = vi.fn().mockResolvedValue({ skillName: 'demo-skill', scope: 'workspace', @@ -55,7 +58,7 @@ function createHarness() { registerWorkspaceSkillsRoutes(app, { workspaceRuntime: { workspaceCwd: '/workspace', - trusted: true, + trusted: options?.trusted ?? true, workspaceService: { installWorkspaceSkill, deleteWorkspaceSkill, @@ -66,7 +69,16 @@ function createHarness() { mutate: () => (_req: Request, _res: Response, next: NextFunction) => next(), safeBody: (req) => req.body as Record, sendBridgeError, - parseAndValidateClientId: () => 'client-1', + parseAndValidateClientId: (_req, res) => { + if (options?.rejectClientId) { + res.status(400).json({ + error: 'Invalid client id', + code: 'invalid_client_id', + }); + return null; + } + return 'client-1'; + }, }); return { app, @@ -248,6 +260,9 @@ describe('workspace Skill management routes', () => { const empty = await request(harness.app) .post('/workspace/skills/enable') .send({ skillNames: [], enabled: false }); + const nonArray = await request(harness.app) + .post('/workspace/skills/enable') + .send({ skillNames: 'review', enabled: false }); const tooMany = await request(harness.app) .post('/workspace/skills/enable') .send({ @@ -275,6 +290,8 @@ describe('workspace Skill management routes', () => { expect(empty.status).toBe(400); expect(empty.body.code).toBe('invalid_skill_names'); + expect(nonArray.status).toBe(400); + expect(nonArray.body.code).toBe('invalid_skill_names'); expect(tooMany.status).toBe(400); expect(tooMany.body.code).toBe('invalid_skill_names'); expect(blank.status).toBe(400); @@ -289,6 +306,30 @@ describe('workspace Skill management routes', () => { expect(harness.setWorkspaceSkillsEnabled).toHaveBeenCalledTimes(1); }); + it('trust-gates Skill batches before parsing or resolving the client id', async () => { + const harness = createHarness({ trusted: false }); + + const response = await request(harness.app) + .post('/workspace/skills/enable') + .send({ skillNames: ['review'], enabled: false }); + + expect(response.status).toBe(403); + expect(response.body.code).toBe('untrusted_workspace'); + expect(harness.setWorkspaceSkillsEnabled).not.toHaveBeenCalled(); + }); + + it('stops Skill batches when client-id validation rejects the request', async () => { + const harness = createHarness({ rejectClientId: true }); + + const response = await request(harness.app) + .post('/workspace/skills/enable') + .send({ skillNames: ['review'], enabled: false }); + + expect(response.status).toBe(400); + expect(response.body.code).toBe('invalid_client_id'); + expect(harness.setWorkspaceSkillsEnabled).not.toHaveBeenCalled(); + }); + it('fails the whole batch when the workspace generation closes', async () => { const harness = createHarness(); harness.setWorkspaceSkillsEnabled.mockRejectedValueOnce( diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index 7739bce8ac9..4a213b63277 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -833,10 +833,12 @@ describe('workspace skill settings persistence', () => { 'setValues', ); + const generationGuard = vi.fn(); const result = await persistDisabledSkillsBatch!( workspace, ['review', 'alpha', 'locked-skill'], false, + generationGuard, ); expect(result.outcomes).toHaveLength(3); @@ -859,6 +861,12 @@ describe('workspace skill settings persistence', () => { }, ]); expect(setValues).toHaveBeenCalledOnce(); + expect( + generationGuard.mock.invocationCallOrder.filter( + (order) => order < setValues.mock.invocationCallOrder[0]!, + ), + ).toHaveLength(2); + expect(setValues.mock.calls[0]?.[2]).toBe(generationGuard); const savedAfterDisable = JSON.parse( fs.readFileSync(path.join(workspace, '.qwen', 'settings.json'), 'utf8'), @@ -901,6 +909,17 @@ describe('workspace skill settings persistence', () => { 'alpha', ]); expect(savedAfterEnable.skills.enabled).toEqual(['opt-in']); + + const unchanged = await persistDisabledSkillsBatch!( + workspace, + ['review'], + false, + ); + expect(unchanged).toEqual({ + outcomes: [{ skillName: 'review', changed: false }], + settingsChanges: [], + }); + expect(setValues).toHaveBeenCalledTimes(2); }); }); diff --git a/packages/cli/src/serve/workspace-qualified-rest.test.ts b/packages/cli/src/serve/workspace-qualified-rest.test.ts index 34301fbc47b..aa83eafeff5 100644 --- a/packages/cli/src/serve/workspace-qualified-rest.test.ts +++ b/packages/cli/src/serve/workspace-qualified-rest.test.ts @@ -989,22 +989,22 @@ describe('workspace-qualified core REST', () => { .set('Authorization', 'Bearer secret') .set('X-Qwen-Client-Id', 'client-1') .set('Host', host()) - .send({ skillNames: ['review', 'deploy'], enabled: false }); + .send({ skillNames: ['review', 'deploy'], enabled: true }); expect(batch.status).toBe(200); expect(batch.body).toEqual({ - enabled: false, + enabled: true, activation: 'deferred', sessionsRefreshed: 0, sessionsFailed: 0, results: [ { skillName: 'review', - enabled: false, + enabled: true, changed: true, }, { skillName: 'deploy', - enabled: false, + enabled: true, changed: true, }, ], @@ -1018,9 +1018,35 @@ describe('workspace-qualified core REST', () => { originatorClientId: 'client-1', }), ['review', 'deploy'], - false, + true, ); + const invalidBatch = await request(h.app) + .post(`/workspaces/${encodeURIComponent(h.secondaryId)}/skills/enable`) + .set('Authorization', 'Bearer secret') + .set('Host', host()) + .send({ skillNames: 'review', enabled: true }); + expect(invalidBatch.status).toBe(400); + expect(invalidBatch.body.code).toBe('invalid_skill_names'); + expect( + h.secondaryWorkspaceService.setWorkspaceSkillsEnabled, + ).toHaveBeenCalledTimes(1); + + vi.mocked( + h.secondaryWorkspaceService.setWorkspaceSkillsEnabled, + ).mockRejectedValueOnce( + Object.assign(new Error('closed'), { + code: 'workspace_generation_closed', + }), + ); + const failedBatch = await request(h.app) + .post(`/workspaces/${encodeURIComponent(h.secondaryId)}/skills/enable`) + .set('Authorization', 'Bearer secret') + .set('Host', host()) + .send({ skillNames: ['review'], enabled: true }); + expect(failedBatch.status).toBe(503); + expect(failedBatch.body.code).toBe('workspace_runtime_unavailable'); + vi.mocked( h.secondaryWorkspaceService.setWorkspaceSkillEnabled, ).mockRejectedValueOnce(new WorkspaceSkillNotFoundError('missing')); @@ -1055,7 +1081,7 @@ describe('workspace-qualified core REST', () => { expect(invalidBatchClient.body.code).toBe('invalid_client_id'); expect( h.secondaryWorkspaceService.setWorkspaceSkillsEnabled, - ).toHaveBeenCalledTimes(1); + ).toHaveBeenCalledTimes(2); } finally { await fsp.rm(h.scratch, { recursive: true, force: true }); } diff --git a/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts b/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts index 97cdbde671f..468989c85f6 100644 --- a/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts +++ b/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts @@ -2107,7 +2107,7 @@ describe('createDaemonWorkspaceService', () => { }); const persistDisabledSkillsBatch = vi.fn().mockResolvedValue({ outcomes: [ - { skillName: 'review', changed: true }, + { skillName: 'deploy', changed: false }, { skillName: 'locked', error: new WorkspaceSkillNotToggleableError( @@ -2116,11 +2116,9 @@ describe('createDaemonWorkspaceService', () => { 'user', ), }, - { skillName: 'deploy', changed: true }, - ], - settingsChanges: [ - { key: 'skills.disabled', value: ['review', 'deploy'] }, + { skillName: 'review', changed: true }, ], + settingsChanges: [{ key: 'skills.disabled', value: ['review'] }], }); const invokeWorkspaceCommand = vi.fn().mockResolvedValue({ sessionsRefreshed: 2, @@ -2163,7 +2161,7 @@ describe('createDaemonWorkspaceService', () => { sessionsFailed: 0, results: [ { skillName: 'review', enabled: false, changed: true }, - { skillName: 'deploy', enabled: false, changed: true }, + { skillName: 'deploy', enabled: false, changed: false }, ], errors: [ { @@ -2197,7 +2195,7 @@ describe('createDaemonWorkspaceService', () => { type: 'settings_changed', data: { key: 'skills.disabled', - value: ['review', 'deploy'], + value: ['review'], scope: 'workspace', }, originatorClientId: 'client-1', @@ -2291,6 +2289,107 @@ describe('createDaemonWorkspaceService', () => { expect(persistDisabledSkillsBatch).not.toHaveBeenCalled(); }); + it('allows enabling a legacy extension skill disabled by settings', async () => { + await withIsolatedWorkspace(async ({ workspace }) => { + await writeJson( + path.join(workspace, SETTINGS_DIRECTORY_NAME, 'settings.json'), + { skills: { disabled: ['legacy-inactive'] } }, + ); + const persistDisabledSkillsBatch = vi.fn().mockResolvedValue({ + outcomes: [{ skillName: 'legacy-inactive', changed: true }], + settingsChanges: [{ key: 'skills.disabled', value: undefined }], + }); + const svc = createDaemonWorkspaceService( + makeDeps({ + boundWorkspace: workspace, + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: workspace, + initialized: true, + skills, + }), + persistDisabledSkillsBatch, + isChannelLive: () => false, + }), + ); + + await expect( + svc.setWorkspaceSkillsEnabled(makeCtx(), ['legacy-inactive'], true), + ).resolves.toMatchObject({ + enabled: true, + results: [ + { skillName: 'legacy-inactive', enabled: true, changed: true }, + ], + errors: [], + }); + expect(persistDisabledSkillsBatch).toHaveBeenCalledWith( + workspace, + ['legacy-inactive'], + true, + undefined, + ); + }); + }); + + it('publishes both setting changes from an enabling batch', async () => { + const publishWorkspaceEvent = vi.fn(); + const svc = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch: vi.fn().mockResolvedValue({ + outcomes: [ + { skillName: 'review', changed: true }, + { skillName: 'deploy', changed: true }, + ], + settingsChanges: [ + { key: 'skills.disabled', value: undefined }, + { key: 'skills.enabled', value: ['deploy'] }, + ], + }), + publishWorkspaceEvent, + isChannelLive: () => false, + }), + ); + + await expect( + svc.setWorkspaceSkillsEnabled( + makeCtx({ originatorClientId: 'client-1' }), + ['review', 'deploy'], + true, + ), + ).resolves.toMatchObject({ + enabled: true, + results: [ + { skillName: 'review', enabled: true, changed: true }, + { skillName: 'deploy', enabled: true, changed: true }, + ], + }); + expect(publishWorkspaceEvent).toHaveBeenCalledTimes(2); + expect(publishWorkspaceEvent).toHaveBeenNthCalledWith(1, { + type: 'settings_changed', + data: { + key: 'skills.disabled', + value: undefined, + scope: 'workspace', + }, + originatorClientId: 'client-1', + }); + expect(publishWorkspaceEvent).toHaveBeenNthCalledWith(2, { + type: 'settings_changed', + data: { + key: 'skills.enabled', + value: ['deploy'], + scope: 'workspace', + }, + originatorClientId: 'client-1', + }); + }); + it('reports partial activation when the shared batch refresh fails', async () => { const failedSessions = createDaemonWorkspaceService( makeDeps({ diff --git a/packages/sdk-typescript/test/unit/DaemonClient.test.ts b/packages/sdk-typescript/test/unit/DaemonClient.test.ts index 7d5e13d0d75..aa35dcc0547 100644 --- a/packages/sdk-typescript/test/unit/DaemonClient.test.ts +++ b/packages/sdk-typescript/test/unit/DaemonClient.test.ts @@ -4042,14 +4042,14 @@ describe('DaemonClient', () => { describe('setWorkspaceSkillsEnabled', () => { const response = { - enabled: false, + enabled: true, activation: 'applied', sessionsRefreshed: 2, sessionsFailed: 0, results: [ { skillName: 'review', - enabled: false, + enabled: true, changed: true, }, ], @@ -4063,7 +4063,7 @@ describe('DaemonClient', () => { const client = new DaemonClient({ baseUrl: 'http://daemon', fetch }); await expect( - client.setWorkspaceSkillsEnabled(['review', 'deploy'], false, { + client.setWorkspaceSkillsEnabled(['review', 'deploy'], true, { clientId: 'client-1', }), ).resolves.toEqual(response); @@ -4072,7 +4072,7 @@ describe('DaemonClient', () => { method: 'POST', body: JSON.stringify({ skillNames: ['review', 'deploy'], - enabled: false, + enabled: true, }), }); expect(calls[0]?.headers['content-type']).toBe('application/json'); @@ -4087,7 +4087,7 @@ describe('DaemonClient', () => { await client .workspaceByCwd('/tmp/work space') - .setWorkspaceSkillsEnabled(['review', 'deploy'], false, { + .setWorkspaceSkillsEnabled(['review', 'deploy'], true, { clientId: 'client-2', }); @@ -4096,7 +4096,7 @@ describe('DaemonClient', () => { method: 'POST', body: JSON.stringify({ skillNames: ['review', 'deploy'], - enabled: false, + enabled: true, }), }); expect(calls[0]?.headers['x-qwen-client-id']).toBe('client-2'); From 895234b30196907a8bbd38cbb6c104bce38d1257 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sat, 8 Aug 2026 03:31:46 +0000 Subject: [PATCH 7/9] docs(daemon): clarify Skill batch toggle contract notes from review Co-authored-by: Qwen-Coder --- docs/design/daemon-skill-batch-toggle.md | 8 +++++++- docs/developers/daemon/13-sdk-daemon-client.md | 2 +- docs/developers/qwen-serve-protocol.md | 4 ++-- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/docs/design/daemon-skill-batch-toggle.md b/docs/design/daemon-skill-batch-toggle.md index e31bd3622c1..c4e2ad03bab 100644 --- a/docs/design/daemon-skill-batch-toggle.md +++ b/docs/design/daemon-skill-batch-toggle.md @@ -58,6 +58,10 @@ persistence and runtime-generation failures fail the whole request. } ``` +`results` and `errors` each preserve request order within their own array; +the response does not reconstruct the original mixed ordering, so clients +re-match targets by `skillName`. + Malformed requests still fail as a whole with HTTP 400. Workspace trust, authentication, client identity, and generation ownership use the same gates as the single-Skill route. @@ -67,4 +71,6 @@ as the single-Skill route. Advertise `workspace_skill_batch_toggle` separately from `workspace_skill_toggle`. Clients must pre-flight the new capability before calling the collection route. The existing single-Skill route and response -remain unchanged. +remain unchanged. The collection routes are HTTP-only: the ACP +`_qwen/workspace/skills` dispatch surface stays read-only, matching the +single-Skill toggle. diff --git a/docs/developers/daemon/13-sdk-daemon-client.md b/docs/developers/daemon/13-sdk-daemon-client.md index c265342ab03..38ca21c4e6e 100644 --- a/docs/developers/daemon/13-sdk-daemon-client.md +++ b/docs/developers/daemon/13-sdk-daemon-client.md @@ -165,7 +165,7 @@ await client .setWorkspaceSkillsEnabled(['review', 'deploy'], true); ``` -`DaemonSkillBatchToggleResult` contains ordered successful `results`, per-target `errors`, and batch-level activation/session-refresh counts. The daemon persists valid targets together and refreshes active sessions once; one expected target error does not block other valid targets. +`DaemonSkillBatchToggleResult` contains ordered successful `results`, per-target `errors`, and batch-level activation/session-refresh counts. The daemon persists valid targets together and refreshes active sessions once; one expected target error does not block other valid targets. The method throws only on a non-200 response; a 200 does not mean every target was applied, so always inspect `errors` before treating the batch as successful. Workspace display names are optional presentation metadata. Pre-flight `capabilities.features.includes('workspace_display_name')`; workspace ids and canonical paths remain the only selectors, and duplicate display names are valid. diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index 604428b4654..433eb8fd1bb 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -2680,7 +2680,7 @@ The mutation reuses the workspace-scoped `settings_changed` event for each chang Capability tag: `workspace_skill_batch_toggle`. The workspace-qualified form is `POST /workspaces/:workspace/skills/enable`. -Toggle up to 100 loaded Skills in one request. Names are trimmed and deduplicated case-insensitively while preserving first-seen order. The daemon validates against one Skill status snapshot, persists all valid changes in one locked settings write, and refreshes active sessions once. Processing is best-effort for expected target errors: an unknown, hidden, inactive-extension, or locked target is recorded in `errors` without preventing other valid targets from being applied. Unexpected persistence or runtime-generation failures still fail the whole request. +Toggle up to 100 loaded Skills in one request; the cap counts the raw `skillNames` entries before deduplication. Names are trimmed and deduplicated case-insensitively while preserving first-seen order. The daemon validates against one Skill status snapshot, persists all valid changes in one locked settings write, and refreshes active sessions once. Processing is best-effort for expected target errors: an unknown, hidden, inactive-extension, or locked target is recorded in `errors` without preventing other valid targets from being applied. Unexpected persistence or runtime-generation failures still fail the whole request. Request: @@ -2721,7 +2721,7 @@ Response (200): } ``` -Target errors use `skill_not_found`, `skill_not_toggleable`, or `skill_inactive_extension`. Malformed requests return HTTP 400 with `invalid_skill_names`, `invalid_skill_name`, or `invalid_enabled_flag`. Authentication, workspace trust, client identity, unexpected persistence failures, and runtime-generation failures fail the whole request through the standard route gates. Batch-level `activation`, `sessionsRefreshed`, and `sessionsFailed` describe the single live-session refresh shared by all changed results. +Target errors use `skill_not_found`, `skill_not_toggleable`, or `skill_inactive_extension`. Malformed requests return HTTP 400 with `invalid_skill_names`, `invalid_skill_name`, or `invalid_enabled_flag`. Authentication, workspace trust, client identity, unexpected persistence failures, and runtime-generation failures fail the whole request through the standard route gates. Batch-level `activation`, `sessionsRefreshed`, and `sessionsFailed` describe the single live-session refresh shared by all changed results. `activation` reports the refresh attempt rather than the outcome: a batch in which no target changed (for example, every target errored) still answers `applied` when a session is live, matching the single-Skill no-op response, so derive what actually changed from each result's `changed` flag and the `errors` array. #### `POST /workspace/init` From 7f59a228373d687d185437a89c35481752d609e9 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sat, 8 Aug 2026 12:17:14 +0000 Subject: [PATCH 8/9] test(daemon): pin Skill batch toggle cap semantics and SDK surface shape --- .../src/serve/routes/workspace-skills.test.ts | 10 +++++++ .../test/unit/daemon-public-surface.test.ts | 28 +++++++++++++++---- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/serve/routes/workspace-skills.test.ts b/packages/cli/src/serve/routes/workspace-skills.test.ts index 75ae30dd671..a48722d739f 100644 --- a/packages/cli/src/serve/routes/workspace-skills.test.ts +++ b/packages/cli/src/serve/routes/workspace-skills.test.ts @@ -254,6 +254,14 @@ describe('workspace Skill management routes', () => { skillNames: Array.from({ length: 101 }, (_, i) => `s${i}`), enabled: false, }); + // The cap counts raw entries before deduplication (contract stated in + // docs/developers/qwen-serve-protocol.md), so duplicates cannot bypass it. + const duplicatesOverCap = await request(harness.app) + .post('/workspace/skills/enable') + .send({ + skillNames: Array.from({ length: 101 }, () => 'review'), + enabled: false, + }); const blank = await request(harness.app) .post('/workspace/skills/enable') .send({ skillNames: [' '], enabled: false }); @@ -284,6 +292,8 @@ describe('workspace Skill management routes', () => { expect(empty.body.code).toBe('invalid_skill_names'); expect(tooMany.status).toBe(400); expect(tooMany.body.code).toBe('invalid_skill_names'); + expect(duplicatesOverCap.status).toBe(400); + expect(duplicatesOverCap.body.code).toBe('invalid_skill_names'); expect(blank.status).toBe(400); expect(blank.body.code).toBe('invalid_skill_name'); expect(invalidFlag.status).toBe(400); diff --git a/packages/sdk-typescript/test/unit/daemon-public-surface.test.ts b/packages/sdk-typescript/test/unit/daemon-public-surface.test.ts index 38e14d51fe5..62d98224c06 100644 --- a/packages/sdk-typescript/test/unit/daemon-public-surface.test.ts +++ b/packages/sdk-typescript/test/unit/daemon-public-surface.test.ts @@ -19,6 +19,7 @@ import { // `src/daemon/index.ts` but not re-exported by the published entry" // gap that two-layer SDK re-exports are easy to drift on. import type { + DaemonClient, DaemonClientEvictedData, DaemonClientEvictedEvent, DaemonChannelControlState, @@ -295,12 +296,27 @@ describe('public SDK entry — typed daemon event surface (#4217)', () => { expectTypeOf().not.toBeNever(); expectTypeOf().not.toBeNever(); expectTypeOf().not.toBeNever(); - // Batch Skill toggle surface: clients type the result of - // `client.setWorkspaceSkillsEnabled(...)` through the published entry. - expectTypeOf().not.toBeNever(); - expectTypeOf().not.toBeNever(); - expectTypeOf().not.toBeNever(); - expectTypeOf().not.toBeNever(); + // Batch Skill toggle surface: type-only imports are erased at vitest + // runtime, so the prototype check is the fence that actually executes + // here; the shape assertions pin the contract for any tsc pass. + expect(typeof Public.DaemonClient.prototype.setWorkspaceSkillsEnabled).toBe( + 'function', + ); + expectTypeOf< + Awaited> + >().toEqualTypeOf(); + expectTypeOf().toEqualTypeOf<{ + skillName: string; + enabled: boolean; + changed: boolean; + }>(); + expectTypeOf().toEqualTypeOf<{ + skillName: string; + code: DaemonSkillBatchToggleErrorCode; + error: string; + reason?: 'not_user_invocable' | 'inactive_extension' | 'locked'; + lockedScope?: 'system' | 'user' | 'systemDefaults'; + }>(); // `GET /daemon/status` report surface (PR 5174 client coverage): the // envelope plus the sub-shapes UI dashboards need to type against. expectTypeOf().not.toBeNever(); From f0dca845ca6dc69c50c869f5758b01dc2afbf987 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sat, 8 Aug 2026 18:32:42 +0000 Subject: [PATCH 9/9] test(daemon): pin Skill batch toggle mutants flagged in round-5 review --- .../__tests__/facade.test.ts | 97 +++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts b/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts index 019b001629e..db6637d7936 100644 --- a/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts +++ b/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts @@ -2285,6 +2285,34 @@ describe('createDaemonWorkspaceService', () => { expect(publishWorkspaceEvent).not.toHaveBeenCalled(); }); + it('fails the whole batch when a persisted outcome is missing for a valid target', async () => { + const invokeWorkspaceCommand = vi.fn(); + const publishWorkspaceEvent = vi.fn(); + const svc = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch: vi.fn().mockResolvedValue({ + outcomes: [{ skillName: 'review', changed: true }], + settingsChanges: [], + }), + invokeWorkspaceCommand, + publishWorkspaceEvent, + isChannelLive: () => true, + }), + ); + + await expect( + svc.setWorkspaceSkillsEnabled(makeCtx(), ['review', 'deploy'], false), + ).rejects.toThrow('Missing persisted Skill batch outcome: deploy'); + expect(invokeWorkspaceCommand).not.toHaveBeenCalled(); + expect(publishWorkspaceEvent).not.toHaveBeenCalled(); + }); + it('returns validation errors without persisting when no target is valid', async () => { const persistDisabledSkillsBatch = vi.fn(); const svc = createDaemonWorkspaceService( @@ -2296,6 +2324,7 @@ describe('createDaemonWorkspaceService', () => { skills, }), persistDisabledSkillsBatch, + isChannelLive: () => true, }), ); @@ -2306,6 +2335,9 @@ describe('createDaemonWorkspaceService', () => { false, ), ).resolves.toMatchObject({ + activation: 'applied', + sessionsRefreshed: 0, + sessionsFailed: 0, results: [], errors: [ { skillName: 'missing', code: 'skill_not_found' }, @@ -2744,6 +2776,71 @@ describe('createDaemonWorkspaceService', () => { ); expect(queryWorkspaceStatus).toHaveBeenCalledTimes(2); }); + + it('does not retain a status snapshot read while a batch settings refresh is in flight', async () => { + const refresh = deferred<{ + sessionsRefreshed: number; + sessionsFailed: number; + }>(); + const beforeStatus = { + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }; + const afterStatus = { + ...beforeStatus, + skills: [ + { + kind: 'skill', + status: 'disabled', + name: 'review', + description: 'Review changed code', + level: 'bundled', + modelInvocable: true, + }, + ], + }; + const queryWorkspaceStatus = vi + .fn() + .mockResolvedValueOnce(beforeStatus) + .mockResolvedValueOnce(beforeStatus) + .mockResolvedValueOnce(afterStatus); + const invokeWorkspaceCommand = vi.fn( + () => refresh.promise, + ) as unknown as InvokeWorkspaceCommandFn; + const svc = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus, + persistDisabledSkillsBatch: vi.fn().mockResolvedValue({ + outcomes: [{ skillName: 'review', changed: true }], + settingsChanges: [{ key: 'skills.disabled', value: ['review'] }], + }), + invokeWorkspaceCommand, + isChannelLive: () => true, + }), + ); + + const toggle = svc.setWorkspaceSkillsEnabled( + makeCtx(), + ['review'], + false, + ); + await vi.waitFor(() => + expect(invokeWorkspaceCommand).toHaveBeenCalledOnce(), + ); + + await expect(svc.getWorkspaceSkillsStatus(makeCtx())).resolves.toEqual( + beforeStatus, + ); + refresh.resolve({ sessionsRefreshed: 1, sessionsFailed: 0 }); + await toggle; + + await expect(svc.getWorkspaceSkillsStatus(makeCtx())).resolves.toEqual( + afterStatus, + ); + expect(queryWorkspaceStatus).toHaveBeenCalledTimes(3); + }); }); describe('requestWorkspaceTrustChange', () => {