diff --git a/docs/design/daemon-skill-batch-toggle.md b/docs/design/daemon-skill-batch-toggle.md new file mode 100644 index 00000000000..c4e2ad03bab --- /dev/null +++ b/docs/design/daemon-skill-batch-toggle.md @@ -0,0 +1,76 @@ +# 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", "missing"], + "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 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": 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" + } + ] +} +``` + +`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. + +## 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. 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 7ed73138eb0..38ca21c4e6e 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`, 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. ```ts diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index fc996bd1c6f..65e1ec00964 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. @@ -2704,6 +2705,53 @@ 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 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: + +```json +{ + "skillNames": ["review", "deploy", "missing"], + "enabled": false +} +``` + +Response (200): + +```json +{ + "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" + } + ] +} +``` + +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` 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 76e878a7c93..00b79f5e5c8 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`, 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/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', 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/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 f14f543a18d..a48722d739f 100644 --- a/packages/cli/src/serve/routes/workspace-skills.test.ts +++ b/packages/cli/src/serve/routes/workspace-skills.test.ts @@ -5,8 +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 type { WorkspaceSkillBatchToggleResult } from '../workspace-service/types.js'; import { registerWorkspaceSkillsRoutes } from './workspace-skills.js'; function createHarness() { @@ -20,6 +22,34 @@ 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 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, { @@ -29,14 +59,22 @@ function createHarness() { workspaceService: { 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 { app, installWorkspaceSkill, deleteWorkspaceSkill }; + return { + app, + installWorkspaceSkill, + deleteWorkspaceSkill, + setWorkspaceSkillEnabled, + setWorkspaceSkillsEnabled, + }; } describe('workspace Skill management routes', () => { @@ -134,4 +172,158 @@ 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.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') + .send({ + skillNames: [' Review ', 'review', 'missing', 'locked'], + enabled: false, + }); + + expect(response.status).toBe(200); + expect(response.body).toEqual({ + 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', + }, + ], + }); + expect(harness.setWorkspaceSkillsEnabled).toHaveBeenCalledWith( + expect.objectContaining({ + route: 'POST /workspace/skills/enable', + originatorClientId: 'client-1', + }), + ['Review', 'missing', 'locked'], + 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, + }); + // 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 }); + 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, + }); + + 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); + 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); + 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); + 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 54882312dc8..c42fbdd795f 100644 --- a/packages/cli/src/serve/routes/workspace-skills.ts +++ b/packages/cli/src/serve/routes/workspace-skills.ts @@ -25,6 +25,7 @@ import { type WorkspaceSkillInstallRequest, type WorkspaceSkillScope, } from '../workspace-skill-management.js'; +const MAX_WORKSPACE_SKILL_BATCH_SIZE = 100; interface RegisterWorkspaceSkillsRoutesDeps { workspaceRuntime: WorkspaceRuntime; @@ -37,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, @@ -58,22 +81,51 @@ function parseSkillToggleRequest( }); return undefined; } - if (skillName.length > MAX_WORKSPACE_SKILL_NAME_LENGTH) { + if (rejectSkillNameTooLong(skillName, res)) return undefined; + const flag = parseEnabledFlag(safeBody(req), res); + return flag ? { skillName, enabled: flag.enabled } : undefined; +} + +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: `Skill name exceeds ${MAX_WORKSPACE_SKILL_NAME_LENGTH}-character limit`, - code: 'invalid_skill_name', + error: `\`skillNames\` must be a non-empty string array (max ${MAX_WORKSPACE_SKILL_BATCH_SIZE})`, + code: 'invalid_skill_names', }); 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; + + 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 (rejectSkillNameTooLong(skillName, res)) return undefined; + const normalizedName = skillName.toLowerCase(); + if (seen.has(normalizedName)) continue; + seen.add(normalizedName); + skillNames.push(skillName); } - return { skillName, enabled }; + + const flag = parseEnabledFlag(body, res); + return flag ? { skillNames, enabled: flag.enabled } : undefined; } function parseSkillScope( @@ -102,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']; @@ -165,6 +211,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 +267,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 deps.workspaceRuntime.workspaceService.setWorkspaceSkillsEnabled( + 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 +321,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 +393,36 @@ 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 runtime.workspaceService.setWorkspaceSkillsEnabled( + 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/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index e1451523764..2c19bbf575f 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -789,6 +789,158 @@ 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'], + defaultDisabled: ['opt-in'], + }, + }), + ); + + 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(); + + 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[] } }; + 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']); + + 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/run-qwen-serve.ts b/packages/cli/src/serve/run-qwen-serve.ts index cd47d12947b..23385f53b0f 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -399,6 +399,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; @@ -3875,6 +3877,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, @@ -4062,6 +4148,7 @@ async function runQwenServeImpl( isChannelLive: () => bridge.isChannelLive(), persistDisabledTools: persistDisabledToolsFn, persistDisabledSkills: persistDisabledSkillsFn, + persistDisabledSkillsBatch: persistDisabledSkillsBatchFn, persistSetting: persistSettingFn, persistSettings: persistSettingsFn, preheatAcpChild: () => bridge.preheat(), @@ -4470,6 +4557,7 @@ async function runQwenServeImpl( preheatAcpChild: () => secondaryBridge.preheat(), persistDisabledTools: persistDisabledToolsFn, persistDisabledSkills: persistDisabledSkillsFn, + persistDisabledSkillsBatch: persistDisabledSkillsBatchFn, persistSetting: persistSettingFn, persistSettings: persistSettingsFn, reloadDaemonEnv: (workspace, assertGenerationOpen) => @@ -5028,6 +5116,7 @@ async function runQwenServeImpl( preheatAcpChild: () => wsBridge.preheat(), persistDisabledTools: persistDisabledToolsFn, persistDisabledSkills: persistDisabledSkillsFn, + persistDisabledSkillsBatch: persistDisabledSkillsBatchFn, persistSetting: persistSettingFn, persistSettings: persistSettingsFn, reloadDaemonEnv: (workspace, assertGenerationOpen) => @@ -5648,6 +5737,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 40628b068bf..773d76fb6d0 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -459,6 +459,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', @@ -18078,6 +18079,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, { @@ -18273,6 +18290,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/server.ts b/packages/cli/src/serve/server.ts index 6f403aac5d8..00bc553b575 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -518,6 +518,7 @@ export interface ServeAppDeps { enabled: boolean, ) => Promise; persistDisabledSkills?: DaemonWorkspaceServiceDeps['persistDisabledSkills']; + persistDisabledSkillsBatch?: DaemonWorkspaceServiceDeps['persistDisabledSkillsBatch']; contextFilename?: string; persistSetting?: ( workspace: string, @@ -1080,6 +1081,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 f2248e9f4cc..cd639449bf8 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, @@ -970,6 +984,43 @@ 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, + activation: 'deferred', + sessionsRefreshed: 0, + sessionsFailed: 0, + results: [ + { + skillName: 'review', + enabled: false, + changed: true, + }, + { + skillName: 'deploy', + enabled: false, + changed: true, + }, + ], + errors: [], + }); + expect( + h.secondaryWorkspaceService.setWorkspaceSkillsEnabled, + ).toHaveBeenCalledWith( + expect.objectContaining({ + workspaceCwd: h.secondaryCwd, + originatorClientId: 'client-1', + }), + ['review', 'deploy'], + false, + ); + vi.mocked( h.secondaryWorkspaceService.setWorkspaceSkillEnabled, ).mockRejectedValueOnce(new WorkspaceSkillNotFoundError('missing')); @@ -993,6 +1044,70 @@ 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); + + 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 }); } @@ -1011,6 +1126,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..db6637d7936 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,809 @@ 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, + }, + { + 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 () => { + 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(invokeWorkspaceCommand).toHaveBeenCalledWith( + 'qwen/control/workspace/skills/refresh', + { cwd: '/workspace', reason: 'settings' }, + ); + 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(); + expect(publishWorkspaceEvent).toHaveBeenCalledWith({ + type: 'settings_changed', + data: { + key: 'skills.disabled', + value: ['review', 'deploy'], + scope: 'workspace', + }, + originatorClientId: 'client-1', + }); + }); + + 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(); + const svc = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch: vi + .fn() + .mockRejectedValue(new Error('disk full')), + invokeWorkspaceCommand, + publishWorkspaceEvent, + isChannelLive: () => true, + }), + ); + + await expect( + svc.setWorkspaceSkillsEnabled(makeCtx(), ['review'], false), + ).rejects.toThrow('disk full'); + expect(invokeWorkspaceCommand).not.toHaveBeenCalled(); + 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( + makeDeps({ + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch, + isChannelLive: () => true, + }), + ); + + await expect( + svc.setWorkspaceSkillsEnabled( + makeCtx(), + ['missing', 'hidden', 'inactive'], + false, + ), + ).resolves.toMatchObject({ + activation: 'applied', + sessionsRefreshed: 0, + sessionsFailed: 0, + results: [], + errors: [ + { skillName: 'missing', code: 'skill_not_found' }, + { skillName: 'hidden', code: 'skill_not_toggleable' }, + { skillName: 'inactive', code: 'skill_inactive_extension' }, + ], + }); + expect(persistDisabledSkillsBatch).not.toHaveBeenCalled(); + }); + + it('rejects a legacy inactive extension skill like the single-toggle path', async () => { + 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({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch, + publishWorkspaceEvent, + isChannelLive: () => false, + }), + ); + + 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', + }); + }); + + 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('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, + 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); + }); + + 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', () => { 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..599043574d6 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,174 @@ 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; + // `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) { + 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/DaemonClient.ts b/packages/sdk-typescript/src/daemon/DaemonClient.ts index 181f13fc7d9..a842ccd635b 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, @@ -3177,6 +3178,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 { @@ -5871,6 +5902,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 71a07abf0b7..20143cb946b 100644 --- a/packages/sdk-typescript/src/daemon/index.ts +++ b/packages/sdk-typescript/src/daemon/index.ts @@ -400,6 +400,10 @@ export type { DaemonRuntimeMcpAddResult, DaemonRuntimeMcpRemoveResult, DaemonToolToggleResult, + DaemonSkillBatchToggleError, + DaemonSkillBatchToggleErrorCode, + DaemonSkillBatchToggleItem, + DaemonSkillBatchToggleResult, DaemonSkillToggleActivation, DaemonSkillToggleResult, DaemonSkillScope, diff --git a/packages/sdk-typescript/src/daemon/types.ts b/packages/sdk-typescript/src/daemon/types.ts index 246236328fd..f214f1dba3e 100644 --- a/packages/sdk-typescript/src/daemon/types.ts +++ b/packages/sdk-typescript/src/daemon/types.ts @@ -2571,6 +2571,34 @@ export interface DaemonSkillToggleResult { sessionsFailed: number; } +export type DaemonSkillBatchToggleErrorCode = + | 'skill_not_found' + | 'skill_not_toggleable' + | 'skill_inactive_extension'; + +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; + 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 73268e5ed64..dabb7efbb4d 100644 --- a/packages/sdk-typescript/src/index.ts +++ b/packages/sdk-typescript/src/index.ts @@ -101,6 +101,10 @@ export { type DaemonSettingsReloadedData, type DaemonSettingsReloadedEvent, type DaemonToolToggleResult, + type DaemonSkillBatchToggleError, + type DaemonSkillBatchToggleErrorCode, + type DaemonSkillBatchToggleItem, + 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 a6ad516de3d..e75a1c7802d 100644 --- a/packages/sdk-typescript/test/unit/DaemonClient.test.ts +++ b/packages/sdk-typescript/test/unit/DaemonClient.test.ts @@ -4040,6 +4040,115 @@ describe('DaemonClient', () => { }); }); + describe('setWorkspaceSkillsEnabled', () => { + const response = { + enabled: false, + activation: 'applied', + sessionsRefreshed: 2, + sessionsFailed: 0, + results: [ + { + skillName: 'review', + enabled: false, + changed: true, + }, + ], + 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['content-type']).toBe('application/json'); + 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('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, { + 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 = { 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 4fc4e7f56d6..deb4ade417d 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, @@ -78,6 +79,10 @@ import type { DaemonSessionDiedEvent, DaemonSessionEvent, DaemonSessionRecapResult, + DaemonSkillBatchToggleError, + DaemonSkillBatchToggleErrorCode, + DaemonSkillBatchToggleItem, + DaemonSkillBatchToggleResult, DaemonSessionRecordingDegradedData, DaemonSessionRecordingDegradedEvent, DaemonSessionUpdateData, @@ -293,6 +298,27 @@ describe('public SDK entry — typed daemon event surface (#4217)', () => { 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();