diff --git a/docs/design/2026-07-20-skills-default-disabled.md b/docs/design/2026-07-20-skills-default-disabled.md new file mode 100644 index 00000000000..353ce5bd4b9 --- /dev/null +++ b/docs/design/2026-07-20-skills-default-disabled.md @@ -0,0 +1,37 @@ +# Overridable default-disabled skills + +## Problem + +`skills.disabled` is a case-insensitive union across settings scopes. That makes it a hard denylist: a project cannot enable a skill disabled by user or system settings. This is correct for policy, but it cannot represent a skill that should start off and remain available for project opt-in. + +## Settings + +Add two case-insensitive union lists while keeping `skills.disabled` unchanged: + +| Setting | Meaning | +| ------------------------ | ------------------------------------------------------- | +| `skills.disabled` | Hard disable. Always wins and preserves existing locks. | +| `skills.defaultDisabled` | Disabled unless explicitly enabled. | +| `skills.enabled` | Explicit opt-in; cannot override `skills.disabled`. | + +Effective disables are `disabled + (defaultDisabled - enabled)`. An explicit `enabled` list is used instead of replacement semantics so enabling one inherited default does not replace unrelated defaults. + +## Runtime and persistence + +One CLI-local resolver computes the effective disabled names and whether each disabled skill is `hard` or `default`. Existing runtime consumers continue reading the effective set through `Config.getDisabledSkillNames()`; core skill discovery and execution APIs do not change. + +The `/skills` picker and daemon toggle apply the same rules: + +- enabling removes a workspace hard disable and adds the canonical name to workspace `skills.enabled` only when needed; +- disabling removes the workspace opt-in and adds the canonical name to workspace `skills.disabled`; +- higher-scope `skills.disabled` entries remain locked; +- unrelated and unavailable skill entries are preserved. + +Workspace skill status adds a disable reason and optional lock scope so clients can distinguish a hard lock from an overridable default. The daemon-local and ACP status paths both read the same CLI-local resolver. + +## Scope + +- No skill is added to `defaultDisabled` by this change. +- `disable-model-invocation` and managed-skill ACP operations are unchanged. +- Existing `skills.disabled` configuration remains compatible. +- Changes are limited to settings, the two existing toggle surfaces, workspace skill status, their wire types, documentation, and focused tests. diff --git a/docs/design/daemon-skill-toggle.md b/docs/design/daemon-skill-toggle.md index 760bb26cb95..a22c2531bb4 100644 --- a/docs/design/daemon-skill-toggle.md +++ b/docs/design/daemon-skill-toggle.md @@ -16,7 +16,7 @@ The response contains the canonical skill name, requested state, whether persist ## Semantics -The API changes only workspace `skills.disabled`. Skill lookup is case-insensitive, but the canonical discovered name is persisted. Updating one target removes target duplicates and case variants without deleting orphan entries for unavailable skills. A second identical request is a no-op. +The API changes workspace `skills.disabled` and `skills.enabled` as needed. Skill lookup is case-insensitive, but the canonical discovered name is persisted. Enabling a default-disabled skill writes an explicit opt-in; disabling it removes the opt-in and writes a hard workspace disable. Updating one target removes target duplicates and case variants without deleting orphan entries for unavailable skills. A second identical request is a no-op. The route rejects states the CLI panel cannot toggle: @@ -28,9 +28,9 @@ The route rejects states the CLI panel cannot toggle: The scope lock check and workspace read-modify-write happen inside the daemon's per-workspace settings lock. A failed write stops before refresh and event publication. -## `skills.disabled` versus `disable-model-invocation` +## Skill availability versus `disable-model-invocation` -`skills.disabled` is an operator setting merged as a case-insensitive union across scopes. It removes matching skill slash commands and model-visible skill entries, and execution-time validation rejects the skill. The daemon endpoint writes the workspace member of this union. +`skills.disabled` is an operator hard denylist merged as a case-insensitive union across scopes. `skills.defaultDisabled` supplies overridable defaults and `skills.enabled` supplies explicit opt-ins, with `disabled > enabled > defaultDisabled` precedence. Effective disables remove matching skill slash commands and model-visible skill entries, and execution-time validation rejects the skill. The daemon endpoint writes the workspace members of `disabled` and `enabled`. `disable-model-invocation` is SKILL.md metadata. It hides a skill from model invocation while preserving direct user invocation. The existing managed-skill ACP operation edits that metadata and is intentionally not reused by this API. @@ -42,21 +42,21 @@ The scope lock check and workspace read-modify-write happen inside the daemon's 4. If an ACP child is live, invoke `qwen/control/workspace/skills/refresh`. 5. The child reloads workspace-scope settings and refreshes every active session, including busy sessions. 6. Each session reloads its own workspace settings, rebuilds and pushes `available_commands_update`, and notifies SkillManager consumers. -7. Publish the existing workspace `settings_changed` event for `skills.disabled`. +7. Publish the existing workspace `settings_changed` event for each changed skill-settings key. An in-flight model request cannot be rewritten. Subsequent skill execution checks, command snapshots, and model contexts read the new state. ## Downstream consumers -- Settings merge: system defaults, user, workspace, and system `skills.disabled` form the effective disabled-name set. -- Workspace status: ACP and daemon-local skill mapping expose disabled state and false-only `userInvocable`. +- Settings merge: system defaults, user, workspace, and system lists form the effective disabled-name set with `disabled > enabled > defaultDisabled` precedence. +- Workspace status: ACP and daemon-local skill mapping expose disabled state, disablement reason, lock scope, and false-only `userInvocable`. - Slash commands: available-command construction removes disabled skills and sends updated command metadata to daemon clients. - Model context: SkillManager change listeners refresh the Skill tool description and available-skill context. - Execution validation: the Skill tool re-reads the disabled-name provider before invocation, so later calls are rejected immediately. - Extension state: inactive extension skills remain non-toggleable even when they are not disabled by settings. - Daemon cache: the cached live-child skill snapshot is invalidated after persistence so later GET requests cannot replay stale state. - SDK consumers: both primary-workspace and workspace-qualified clients share the response and error contract. -- Events: existing `settings_changed` consumers observe the committed `skills.disabled` value; there is no new event type. +- Events: existing `settings_changed` consumers observe each committed `skills.disabled` or `skills.enabled` value; there is no new event type. ## Failure behavior diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index 63311ac1647..1dedac3e5f9 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -2557,9 +2557,9 @@ SSE event (workspace-scoped): `tool_toggled` with `{toolName, enabled, originato Capability tag: `workspace_skill_toggle`. The workspace-qualified form is `POST /workspaces/:workspace/skills/:name/enable`. -Toggle a loaded, user-invocable skill through the workspace `skills.disabled` list, matching the CLI `/skills` panel's Space-key behavior. Lookup is case-insensitive, while persistence and the response use the skill's canonical name. Existing disabled entries for skills that are no longer loaded are preserved, and duplicate/case-variant entries for the target are collapsed. A disable entry inherited from system defaults, user, or system scope locks the skill: workspace scope cannot override the merged union. +Toggle a loaded, user-invocable skill through the workspace skill settings, matching the CLI `/skills` panel's Space-key behavior. Lookup is case-insensitive, while persistence and the response use the skill's canonical name. Enabling a `skills.defaultDisabled` skill adds a workspace `skills.enabled` opt-in; disabling removes that opt-in and adds a workspace `skills.disabled` entry. Existing entries for skills that are no longer loaded are preserved, and duplicate/case-variant entries for the target are collapsed. A hard-disable entry inherited from system defaults, user, or system scope locks the skill: workspace scope cannot override it. -This is different from the ACP `qwen/skills/setEnabled` managed-skill operation and the `disable-model-invocation` frontmatter field. `skills.disabled` removes the skill from slash-command/model availability and rejects later skill execution. `disable-model-invocation: true` keeps direct user invocation available and only hides the skill from model invocation. +This is different from the ACP `qwen/skills/setEnabled` managed-skill operation and the `disable-model-invocation` frontmatter field. Effective skill availability follows `skills.disabled` > `skills.enabled` > `skills.defaultDisabled`. Both hard and default disables remove the skill from slash-command/model availability and reject later skill execution. `disable-model-invocation: true` keeps direct user invocation available and only hides the skill from model invocation. Request: @@ -2590,7 +2590,7 @@ Errors: - `404 {code: 'skill_not_found'}` — no loaded skill matches the name. - `409 {code: 'skill_not_toggleable', reason: 'not_user_invocable' | 'inactive_extension' | 'locked', lockedScope?: 'system' | 'user' | 'systemDefaults'}` — the CLI panel would not allow the target to be toggled. `lockedScope` is present only when `reason` is `locked`. -The mutation reuses the workspace-scoped `settings_changed` event with `key: 'skills.disabled'`; it does not add a new event type. +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/init` diff --git a/docs/users/configuration/settings.md b/docs/users/configuration/settings.md index 79801afc8c3..d2c9e611875 100644 --- a/docs/users/configuration/settings.md +++ b/docs/users/configuration/settings.md @@ -494,9 +494,13 @@ execute when typed. Controls which [Skills](../features/skills) are exposed to the model. -| Setting | Type | Description | Default | -| ----------------- | ---------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------- | -| `skills.disabled` | array of strings | Skill names to hide. Matched case-insensitively against the skill name. Hidden skills do not appear in `` or as `/` slash commands. **Merged as a union** across user/project/system scopes, so a project cannot remove entries defined in user or system settings. | `undefined` | +| Setting | Type | Description | Default | +| ------------------------ | ---------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------- | +| `skills.disabled` | array of strings | Hard-disabled skill names. Matched case-insensitively and **merged as a union** across settings scopes, so project settings cannot override a user or system entry. Hidden skills do not appear in `` or as `/` slash commands. | `undefined` | +| `skills.defaultDisabled` | array of strings | Skill names that start disabled but can be opted into through `skills.enabled`. Matched case-insensitively and merged as a union across settings scopes. | `undefined` | +| `skills.enabled` | array of strings | Explicit opt-ins that override matching `skills.defaultDisabled` entries. Matched case-insensitively and merged as a union across settings scopes. This setting cannot override `skills.disabled`. | `undefined` | + +The precedence is `skills.disabled` > `skills.enabled` > `skills.defaultDisabled`. For example, a user can put a skill in `defaultDisabled` and a project can add the same name to `enabled`; a hard `disabled` entry at any scope still wins. #### mcp diff --git a/docs/users/qwen-serve.md b/docs/users/qwen-serve.md index 55c931727c4..a6d8381cda0 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 writes only workspace `skills.disabled`, rejects unknown, hidden, inactive-extension, higher-scope-locked, and untrusted targets, and immediately refreshes active ACP sessions. 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. 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. `GET /workspace/env` and `GET /workspace/preflight` always answer with `initialized: true` regardless of ACP state. `env` never consults ACP diff --git a/packages/acp-bridge/src/status.ts b/packages/acp-bridge/src/status.ts index 00815536401..db6f5d60270 100644 --- a/packages/acp-bridge/src/status.ts +++ b/packages/acp-bridge/src/status.ts @@ -435,6 +435,8 @@ export interface ServeWorkspaceSkillStatus extends ServeStatusCell { description: string; level: ServeSkillLevel; modelInvocable: boolean; + disabledReason?: 'hard' | 'default' | 'inactive_extension'; + lockedScope?: 'system' | 'user' | 'systemDefaults'; userInvocable?: false; installedPath?: string; argumentHint?: string; diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index 3ce805ae286..81e505e1dab 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -198,6 +198,7 @@ import { buildDisabledSkillNamesProvider, loadCliConfig, } from '../config/config.js'; +import { resolveSkillSettings } from '../config/skill-settings.js'; import { createWorkspaceMemoryExtractionErrorLogger, shouldSuppressRememberErrorDetails, @@ -5445,7 +5446,22 @@ class QwenAgent implements Agent { } try { - const disabled = config.getDisabledSkillNames(); + const resolved = resolveSkillSettings( + loadSettings(this.workspaceCwd(config), { + consumeCorruptionEnvVars: false, + skipLoadEnvironment: true, + }), + ); + const disablements = new Map( + Array.from(config.getDisabledSkillNames(), (name) => { + const normalizedName = name.trim().toLowerCase(); + return [ + normalizedName, + resolved.disablements.get(normalizedName) ?? + ({ reason: 'hard' } as const), + ] as const; + }), + ); try { await config.getExtensionManager().refreshCache(); } catch (error) { @@ -5461,7 +5477,7 @@ class QwenAgent implements Agent { const skillsByKey = new Map( skills.map((skill) => [ `${skill.level}:${skill.extensionName ?? ''}:${skill.name}`, - mapSkillConfigToStatus(skill, disabled, { + mapSkillConfigToStatus(skill, disablements, { disabled: isInactiveExtensionSkill(skill, inactiveSkillRefs), }), ]), @@ -5485,7 +5501,7 @@ class QwenAgent implements Agent { level: 'extension', extensionName, }, - disabled, + disablements, { disabled: true }, ), ); diff --git a/packages/cli/src/config/config.integration.test.ts b/packages/cli/src/config/config.integration.test.ts index 3ea492cd4a4..ecf28a0e748 100644 --- a/packages/cli/src/config/config.integration.test.ts +++ b/packages/cli/src/config/config.integration.test.ts @@ -428,8 +428,15 @@ describe('Configuration Integration Tests', () => { describe('buildDisabledSkillNamesProvider', async () => { const { buildDisabledSkillNamesProvider } = await import('./config.js'); - function fakeSettings(disabled: unknown) { - return { merged: { skills: { disabled } } } as never; + function fakeSettings( + disabled: unknown, + defaultDisabled?: unknown, + enabled?: unknown, + ) { + return { + merged: { skills: { disabled, defaultDisabled, enabled } }, + forScope: () => ({ settings: { skills: {} } }), + } as never; } it('returns a normalized set from a normal array', () => { @@ -470,4 +477,12 @@ describe('buildDisabledSkillNamesProvider', async () => { ); expect(provider()).toEqual(new Set(['keep'])); }); + + it('applies explicit enables between defaults and hard disables', () => { + const provider = buildDisabledSkillNamesProvider( + fakeSettings(['hard'], ['soft', 'hard'], ['SOFT', 'HARD']), + ); + + expect(provider()).toEqual(new Set(['hard'])); + }); }); diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index b4dcbab5022..fe34600b79a 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -93,6 +93,7 @@ import { validateMaxWallTimeSetting, } from '../utils/runBudget.js'; import { detectSystemLanguage } from '../i18n/index.js'; +import { resolveSkillSettings } from './skill-settings.js'; const debugLogger = createDebugLogger('CONFIG'); @@ -1456,7 +1457,7 @@ function parseMcpConfig( * Builds the live-read closure for `Config.getDisabledSkillNames()`. * * The returned function reads through `loadedSettings.merged` on every - * call, so `LoadedSettings.setValue('skills.disabled', ...)` invocations + * call, so `LoadedSettings` skill-setting mutations * are reflected without rebuilding `Config`. The closure is over the * `LoadedSettings` instance, NOT over its `.merged` snapshot — that * distinction matters because `LoadedSettings.setValue` replaces the @@ -1471,24 +1472,7 @@ function parseMcpConfig( export function buildDisabledSkillNamesProvider( loadedSettings: LoadedSettings, ): () => ReadonlySet { - return () => { - // Defensive: settings.json is user-editable, so the `disabled` slot - // could be a non-array (e.g. `"disabled": "all"` or `"disabled": 42`) - // OR an array containing non-strings (e.g. `[42, null]`). The `??` - // fallback only catches `null`/`undefined`, so we MUST also guard - // against non-array values before `.filter()` — otherwise calling - // `"all".filter` throws `TypeError: list.filter is not a function` - // and bricks every skill invocation (validateToolParams + execute - // both call this provider without a try/catch). - const raw = loadedSettings.merged.skills?.disabled; - const list = Array.isArray(raw) ? raw : []; - return new Set( - list - .filter((n): n is string => typeof n === 'string') - .map((n) => n.trim().toLowerCase()) - .filter(Boolean), - ); - }; + return () => resolveSkillSettings(loadedSettings).disabledNames; } export async function loadCliConfig( @@ -1507,8 +1491,8 @@ export async function loadCliConfig( /** * Live-read provider for the set of disabled skill names. Forwarded to * `ConfigParameters` so that `Config.getDisabledSkillNames()` reflects - * `LoadedSettings.merged.skills?.disabled` even after `setValue` - * mutations within the same process. + * effective skill availability even after `setValue` mutations within the + * same process. * * Callers MUST close over the live `LoadedSettings` instance, NOT over * the `settings: Settings` snapshot passed as the first argument here — diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index e01219b6b63..bdab79b60f4 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -1999,6 +1999,32 @@ const SETTINGS_SCHEMA = { showInDialog: false, mergeStrategy: MergeStrategy.UNION, }, + defaultDisabled: { + type: 'array', + label: 'Default Disabled Skills', + category: 'Advanced', + requiresRestart: false, + default: undefined as string[] | undefined, + description: + 'Skill names disabled by default unless explicitly enabled through ' + + 'skills.enabled. Matched case-insensitively and UNION-merged across ' + + 'settings scopes. skills.disabled always wins.', + showInDialog: false, + mergeStrategy: MergeStrategy.UNION, + }, + enabled: { + type: 'array', + label: 'Enabled Skills', + category: 'Advanced', + requiresRestart: false, + default: undefined as string[] | undefined, + description: + 'Explicit opt-ins that override matching skills.defaultDisabled ' + + 'entries. Matched case-insensitively and UNION-merged across settings ' + + 'scopes. Cannot override skills.disabled.', + showInDialog: false, + mergeStrategy: MergeStrategy.UNION, + }, directories: { type: 'array', label: 'Skill Directories', diff --git a/packages/cli/src/config/skill-settings.test.ts b/packages/cli/src/config/skill-settings.test.ts new file mode 100644 index 00000000000..81e482492ec --- /dev/null +++ b/packages/cli/src/config/skill-settings.test.ts @@ -0,0 +1,228 @@ +/** + * @license + * Copyright 2026 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import { SettingScope } from './settings.js'; +import { + computeWorkspaceSkillListUpdates, + resolveSkillSettings, + updateWorkspaceSkillSettingLists, +} from './skill-settings.js'; + +function fakeSettings({ + merged, + system = {}, + systemDefaults = {}, + user = {}, + workspace = {}, +}: { + merged: Record; + system?: Record; + systemDefaults?: Record; + user?: Record; + workspace?: Record; +}) { + const byScope = { + [SettingScope.System]: system, + [SettingScope.SystemDefaults]: systemDefaults, + [SettingScope.User]: user, + [SettingScope.Workspace]: workspace, + }; + return { + merged: { skills: merged }, + forScope: (scope: SettingScope) => ({ + settings: { skills: byScope[scope] }, + }), + } as never; +} + +describe('resolveSkillSettings', () => { + it('lets a workspace opt-in override a user default case-insensitively', () => { + const result = resolveSkillSettings( + fakeSettings({ + merged: { + defaultDisabled: [' Review ', 'plan'], + enabled: ['REVIEW'], + }, + user: { defaultDisabled: [' Review ', 'plan'] }, + workspace: { enabled: ['REVIEW'] }, + }), + ); + + expect(result.disabledNames).toEqual(new Set(['plan'])); + expect(result.enabledNames).toEqual(new Set(['review'])); + expect(result.disablements.get('plan')).toEqual({ reason: 'default' }); + expect(result.disablements.has('review')).toBe(false); + }); + + it('keeps hard disables authoritative and reports their lock scope', () => { + const result = resolveSkillSettings( + fakeSettings({ + merged: { + disabled: ['review'], + defaultDisabled: ['review'], + enabled: ['REVIEW'], + }, + user: { disabled: ['Review'] }, + }), + ); + + expect(result.disabledNames).toEqual(new Set(['review'])); + expect(result.disablements.get('review')).toEqual({ + reason: 'hard', + lockedScope: 'user', + }); + }); + + it('ignores malformed and empty list entries', () => { + const result = resolveSkillSettings( + fakeSettings({ + merged: { + disabled: 'all', + defaultDisabled: [null, 42, ' ', 'Valid'], + enabled: false, + }, + }), + ); + + expect(result.disabledNames).toEqual(new Set(['valid'])); + }); +}); + +describe('updateWorkspaceSkillSettingLists', () => { + it('persists a canonical opt-in for a default-disabled skill', () => { + expect( + updateWorkspaceSkillSettingLists( + { disabled: ['orphan'], enabled: [] }, + 'Review', + true, + true, + ), + ).toEqual({ disabled: ['orphan'], enabled: ['Review'] }); + }); + + it('keeps unrelated entries and canonicalizes matching entries', () => { + expect( + updateWorkspaceSkillSettingLists( + { + disabled: ['orphan', ' REVIEW ', 'review'], + enabled: ['other', 'ReViEw'], + }, + 'review', + false, + true, + ), + ).toEqual({ disabled: ['orphan', 'review'], enabled: ['other'] }); + }); + + it('does not add a redundant opt-in for an ordinary skill', () => { + expect( + updateWorkspaceSkillSettingLists( + { disabled: ['review'], enabled: [] }, + 'review', + true, + false, + ), + ).toEqual({ disabled: [], enabled: [] }); + }); + + it('does not reorder an already canonical hard disable', () => { + expect( + updateWorkspaceSkillSettingLists( + { disabled: ['review', 'orphan'], enabled: [] }, + 'review', + false, + false, + ), + ).toEqual({ disabled: ['review', 'orphan'], enabled: [] }); + }); +}); + +describe('computeWorkspaceSkillListUpdates', () => { + it('preserves orphaned workspace disables the picker does not manage', () => { + // 'orphan' is not a loaded skill (different branch, uninstalled extension, + // deleted skills dir), so it must survive untouched even though only + // 'review' is toggled. + const result = computeWorkspaceSkillListUpdates( + ['orphan', 'review'], + new Set(), + [], + [ + { + name: 'review', + wasEnabled: false, + isEnabled: true, + defaultDisabled: false, + }, + ], + ); + + expect(result.disabled).toEqual(['orphan']); + expect(result.disabledChanged).toBe(true); + expect(result.enabledChanged).toBe(false); + }); + + it('drops locked higher-scope entries so they are not re-emitted', () => { + // 'locked' is disabled at a higher scope; the picker must not re-emit it at + // workspace scope. Toggling 'review' on provides a genuine change so the + // write path is exercised while 'orphan' is still preserved. + const result = computeWorkspaceSkillListUpdates( + ['locked', 'orphan', 'review'], + new Set(['locked']), + [], + [ + { + name: 'review', + wasEnabled: false, + isEnabled: true, + defaultDisabled: false, + }, + ], + ); + + expect(result.disabled).toEqual(['orphan']); + expect(result.disabledChanged).toBe(true); + }); + + it('reports no change when nothing toggled and lists already match', () => { + const result = computeWorkspaceSkillListUpdates( + ['orphan'], + new Set(), + [], + [ + { + name: 'review', + wasEnabled: true, + isEnabled: true, + defaultDisabled: false, + }, + ], + ); + + expect(result.disabled).toEqual(['orphan']); + expect(result.disabledChanged).toBe(false); + expect(result.enabledChanged).toBe(false); + }); + + it('records an explicit opt-in when enabling a default-disabled skill', () => { + const result = computeWorkspaceSkillListUpdates( + [], + new Set(), + [], + [ + { + name: 'Review', + wasEnabled: false, + isEnabled: true, + defaultDisabled: true, + }, + ], + ); + + expect(result.enabled).toEqual(['Review']); + expect(result.enabledChanged).toBe(true); + }); +}); diff --git a/packages/cli/src/config/skill-settings.ts b/packages/cli/src/config/skill-settings.ts new file mode 100644 index 00000000000..aae32d82487 --- /dev/null +++ b/packages/cli/src/config/skill-settings.ts @@ -0,0 +1,202 @@ +/** + * @license + * Copyright 2026 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { SettingScope, type LoadedSettings } from './settings.js'; + +export interface SkillDisablement { + reason: 'hard' | 'default'; + lockedScope?: 'system' | 'user' | 'systemDefaults'; +} + +export type SkillSettingListKey = 'disabled' | 'defaultDisabled' | 'enabled'; + +export interface ResolvedSkillSettings { + disabledNames: ReadonlySet; + defaultDisabledNames: ReadonlySet; + enabledNames: ReadonlySet; + disablements: ReadonlyMap; +} + +interface WorkspaceSkillSettingLists { + disabled: string[]; + enabled: string[]; +} + +export function normalizeSkillNames(value: unknown): Set { + if (!Array.isArray(value)) return new Set(); + return new Set( + value + .filter((name): name is string => typeof name === 'string') + .map((name) => name.trim().toLowerCase()) + .filter(Boolean), + ); +} + +export function skillSettingStrings( + settings: LoadedSettings, + scope: SettingScope, + key: SkillSettingListKey, +): string[] { + const value = settings.forScope(scope).settings.skills?.[key]; + return Array.isArray(value) + ? value.filter((name): name is string => typeof name === 'string') + : []; +} + +export function resolveSkillSettings( + settings: LoadedSettings, +): ResolvedSkillSettings { + const hardDisabled = normalizeSkillNames(settings.merged.skills?.disabled); + const defaultDisabled = normalizeSkillNames( + settings.merged.skills?.defaultDisabled, + ); + const enabled = normalizeSkillNames(settings.merged.skills?.enabled); + const disablements = new Map(); + + for (const name of defaultDisabled) { + if (!enabled.has(name)) disablements.set(name, { reason: 'default' }); + } + + const lockedScopes = [ + [SettingScope.SystemDefaults, 'systemDefaults'], + [SettingScope.User, 'user'], + [SettingScope.System, 'system'], + ] as const; + const lockedByName = new Map< + string, + NonNullable + >(); + for (const [scope, label] of lockedScopes) { + for (const name of normalizeSkillNames( + skillSettingStrings(settings, scope, 'disabled'), + )) { + lockedByName.set(name, label); + } + } + + for (const name of hardDisabled) { + const lockedScope = lockedByName.get(name); + disablements.set(name, { + reason: 'hard', + ...(lockedScope ? { lockedScope } : {}), + }); + } + + return { + disabledNames: new Set(disablements.keys()), + defaultDisabledNames: defaultDisabled, + enabledNames: enabled, + disablements, + }; +} + +function updateTarget( + names: string[], + skillName: string, + include: boolean, +): string[] { + const normalizedName = skillName.trim().toLowerCase(); + const next: string[] = []; + let found = false; + for (const name of names) { + if (name.trim().toLowerCase() !== normalizedName) { + next.push(name); + } else if (include && !found) { + next.push(skillName); + found = true; + } + } + if (include && !found) next.push(skillName); + return next; +} + +export function updateWorkspaceSkillSettingLists( + lists: WorkspaceSkillSettingLists, + skillName: string, + enabled: boolean, + defaultDisabled: boolean, +): WorkspaceSkillSettingLists { + const normalizedName = skillName.trim().toLowerCase(); + const hadExplicitEnable = lists.enabled.some( + (name) => name.trim().toLowerCase() === normalizedName, + ); + + if (enabled) { + return { + disabled: updateTarget(lists.disabled, skillName, false), + enabled: + defaultDisabled || hadExplicitEnable + ? updateTarget(lists.enabled, skillName, true) + : lists.enabled, + }; + } + + return { + disabled: updateTarget(lists.disabled, skillName, true), + enabled: updateTarget(lists.enabled, skillName, false), + }; +} + +export interface WorkspaceSkillListToggle { + name: string; + wasEnabled: boolean; + isEnabled: boolean; + /** Record an explicit `skills.enabled` opt-in when enabling this skill. */ + defaultDisabled: boolean; +} + +export interface WorkspaceSkillListUpdates { + disabled: string[]; + enabled: string[]; + disabledChanged: boolean; + enabledChanged: boolean; +} + +/** + * Computes the workspace `skills.disabled` / `skills.enabled` lists the skills + * picker should persist after a set of toggle changes. + * + * The seed lists are the workspace's current entries. Locked skills (disabled + * at a higher scope) are dropped from the seed so we never re-emit redundant + * entries the higher scope already enforces. Orphaned entries — workspace + * disables for skills not currently loaded (a different git branch, an + * uninstalled extension, a deleted skills dir) — are preserved verbatim: only + * the toggled, currently-loaded skills passed in `toggles` mutate the lists. + * That preservation is load-bearing; the orphan case is pinned by a test in + * `skill-settings.test.ts`. + */ +export function computeWorkspaceSkillListUpdates( + workspaceDisabled: readonly string[], + lockedNames: ReadonlySet, + workspaceEnabled: readonly string[], + toggles: readonly WorkspaceSkillListToggle[], +): WorkspaceSkillListUpdates { + const previousDisabled = workspaceDisabled.filter( + (name) => !lockedNames.has(name.trim().toLowerCase()), + ); + const previousEnabled = [...workspaceEnabled]; + let next: WorkspaceSkillSettingLists = { + disabled: previousDisabled, + enabled: previousEnabled, + }; + for (const toggle of toggles) { + if (toggle.wasEnabled === toggle.isEnabled) continue; + next = updateWorkspaceSkillSettingLists( + next, + toggle.name, + toggle.isEnabled, + toggle.defaultDisabled, + ); + } + return { + disabled: next.disabled, + enabled: next.enabled, + disabledChanged: + JSON.stringify(previousDisabled) !== JSON.stringify(next.disabled), + enabledChanged: + JSON.stringify(previousEnabled) !== JSON.stringify(next.enabled), + }; +} diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index 9ff9fa3b786..054b727c95a 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -525,7 +525,13 @@ 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-skill', 'inherited-opt-in'], + enabled: ['INHERITED-OPT-IN'], + }, + }), ); const originalCreateServeApp = serverModule.createServeApp; @@ -534,14 +540,8 @@ describe('workspace skill settings persistence', () => { Parameters[2] >['persistDisabledSkills'] | undefined; - let persistDisabledTools: - | Parameters< - typeof workspaceServiceRuntime.createDaemonWorkspaceService - >[0]['persistDisabledTools'] - | undefined; vi.spyOn(serverModule, 'createServeApp').mockImplementation((...args) => { persistDisabledSkills = args[2]?.persistDisabledSkills; - persistDisabledTools = args[2]?.persistDisabledTools; return originalCreateServeApp(...args); }); handle = await runQwenServe( @@ -556,13 +556,21 @@ describe('workspace skill settings persistence', () => { ); await handle.runtimeReady; expect(persistDisabledSkills).toBeDefined(); - expect(persistDisabledTools).toBeDefined(); + await expect( + persistDisabledSkills!(workspace, 'inherited-opt-in', true), + ).resolves.toEqual({ + changed: false, + disabled: ['orphan', ' ReViEw ', 'review'], + }); await expect( persistDisabledSkills!(workspace, 'review', false), ).resolves.toEqual({ changed: true, disabled: ['orphan', 'review'], + settingsChanges: [ + { key: 'skills.disabled', value: ['orphan', 'review'] }, + ], }); await expect( persistDisabledSkills!(workspace, 'review', false), @@ -578,42 +586,113 @@ describe('workspace skill settings persistence', () => { await expect( persistDisabledSkills!(workspace, 'review', true), ).resolves.toMatchObject({ changed: true }); + await expect( + persistDisabledSkills!(workspace, 'opt-in-skill', true), + ).resolves.toEqual({ + changed: true, + disabled: ['orphan', 'alpha', 'beta'], + settingsChanges: [{ key: 'skills.enabled', value: ['opt-in-skill'] }], + }); const saved = JSON.parse( fs.readFileSync(path.join(workspace, '.qwen', 'settings.json'), 'utf8'), - ) as { skills: { disabled: string[] } }; + ) as { skills: { disabled: string[]; enabled: string[] } }; expect(saved.skills.disabled).toEqual(['orphan', 'alpha', 'beta']); + expect(saved.skills.enabled).toEqual(['opt-in-skill']); + await expect( + persistDisabledSkills!(workspace, 'locked-skill', true), + ).rejects.toMatchObject({ reason: 'locked', lockedScope: 'user' }); + }); + + it('produces both skills.disabled and skills.enabled changes when enabling a workspace-hard-disabled default-disabled skill', async () => { + workspace = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), 'qws-skill-dual-')), + ); + qwenHome = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), 'qws-skill-dual-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: ['dual-skill'] }, + }), + ); + fs.writeFileSync( + path.join(qwenHome, 'settings.json'), + JSON.stringify({ + skills: { defaultDisabled: ['dual-skill'] }, + }), + ); + + const originalCreateServeApp = serverModule.createServeApp; + let persistDisabledSkills: + | NonNullable< + Parameters[2] + >['persistDisabledSkills'] + | undefined; + let persistDisabledTools: + | Parameters< + typeof workspaceServiceRuntime.createDaemonWorkspaceService + >[0]['persistDisabledTools'] + | undefined; + vi.spyOn(serverModule, 'createServeApp').mockImplementation((...args) => { + persistDisabledSkills = args[2]?.persistDisabledSkills; + persistDisabledTools = args[2]?.persistDisabledTools; + 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(persistDisabledSkills).toBeDefined(); + expect(persistDisabledTools).toBeDefined(); + + await expect( + persistDisabledSkills!(workspace, 'dual-skill', true), + ).resolves.toEqual({ + changed: true, + disabled: [], + settingsChanges: [ + { key: 'skills.disabled', value: undefined }, + { key: 'skills.enabled', value: ['dual-skill'] }, + ], + }); + + const saved = JSON.parse( + fs.readFileSync(path.join(workspace, '.qwen', 'settings.json'), 'utf8'), + ) as { skills: { disabled?: string[]; enabled: string[] } }; + expect(saved.skills.disabled).toBeUndefined(); + expect(saved.skills.enabled).toEqual(['dual-skill']); const setValue = vi.spyOn( settingsRuntime.LoadedSettings.prototype, 'setValue', ); - for (const persist of [ - (assertGenerationOpen: () => void) => - persistDisabledSkills!( - workspace, - 'guarded-skill', - false, - assertGenerationOpen, - ), - (assertGenerationOpen: () => void) => - persistDisabledTools!( - workspace, - 'guarded-tool', - false, - assertGenerationOpen, - ), - ]) { - const assertGenerationOpen = vi.fn(); - await persist(assertGenerationOpen); - expect(setValue.mock.calls).toHaveLength(1); - expect(setValue.mock.calls[0]?.[3]).toBe(assertGenerationOpen); - setValue.mockClear(); - } + const setValues = vi.spyOn( + settingsRuntime.LoadedSettings.prototype, + 'setValues', + ); - await expect( - persistDisabledSkills!(workspace, 'locked-skill', true), - ).rejects.toMatchObject({ reason: 'locked', lockedScope: 'user' }); + const skillGuard = vi.fn(); + await persistDisabledSkills!(workspace, 'guarded-skill', false, skillGuard); + expect(setValues.mock.calls).toHaveLength(1); + expect(setValues.mock.calls[0]?.[2]).toBe(skillGuard); + + const toolGuard = vi.fn(); + await persistDisabledTools!(workspace, 'guarded-tool', false, toolGuard); + expect(setValue.mock.calls).toHaveLength(1); + expect(setValue.mock.calls[0]?.[3]).toBe(toolGuard); }); }); diff --git a/packages/cli/src/serve/run-qwen-serve.ts b/packages/cli/src/serve/run-qwen-serve.ts index 6335e545fe7..37d196ecb9a 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -3443,70 +3443,76 @@ async function runQwenServeImpl( ) => withSettingsLock(workspace, async () => { assertGenerationOpen?.(); + const { + resolveSkillSettings, + skillSettingStrings, + updateWorkspaceSkillSettingLists, + } = await import('../config/skill-settings.js'); const fresh = loadSettingsForPersistence(workspace); const normalizedName = skillName.trim().toLowerCase(); - const disabledNames = (value: unknown): string[] => - Array.isArray(value) - ? value.filter( - (entry): entry is string => typeof entry === 'string', - ) - : []; - const lockedScopes = [ - ['system', fresh.system.settings.skills?.disabled], - ['user', fresh.user.settings.skills?.disabled], - ['systemDefaults', fresh.systemDefaults.settings.skills?.disabled], - ] as const; - for (const [scope, names] of lockedScopes) { - if ( - disabledNames(names).some( - (name) => name.trim().toLowerCase() === normalizedName, - ) - ) { - throw new runtime.WorkspaceSkillNotToggleableError( - skillName, - 'locked', - scope, - ); - } + const resolved = resolveSkillSettings(fresh); + const disablement = resolved.disablements.get(normalizedName); + if (disablement?.reason === 'hard' && disablement.lockedScope) { + throw new runtime.WorkspaceSkillNotToggleableError( + skillName, + 'locked', + disablement.lockedScope, + ); } - const workspaceDisabled = disabledNames( - fresh.workspace.settings.skills?.disabled, + const workspaceDisabled = skillSettingStrings( + fresh, + WORKSPACE_SETTING_SCOPE, + 'disabled', ); - const next: string[] = []; - let found = false; - let changed = false; - for (const name of workspaceDisabled) { - if (name.trim().toLowerCase() !== normalizedName) { - next.push(name); - continue; - } - if (enabled) { - changed = true; - continue; - } - if (!found) { - next.push(skillName); - found = true; - if (name !== skillName) changed = true; - } else { - changed = true; - } + const workspaceEnabled = skillSettingStrings( + fresh, + WORKSPACE_SETTING_SCOPE, + 'enabled', + ); + const next = updateWorkspaceSkillSettingLists( + { disabled: workspaceDisabled, enabled: workspaceEnabled }, + skillName, + enabled, + resolved.defaultDisabledNames.has(normalizedName) && + !resolved.enabledNames.has(normalizedName), + ); + const settingsChanges: Array<{ + key: 'skills.disabled' | 'skills.enabled'; + value: string[] | undefined; + }> = []; + if ( + JSON.stringify(next.disabled) !== JSON.stringify(workspaceDisabled) + ) { + settingsChanges.push({ + key: 'skills.disabled', + value: next.disabled.length > 0 ? next.disabled : undefined, + }); } - if (!enabled && !found) { - next.push(skillName); - changed = true; + if (JSON.stringify(next.enabled) !== JSON.stringify(workspaceEnabled)) { + settingsChanges.push({ + key: 'skills.enabled', + value: next.enabled.length > 0 ? next.enabled : undefined, + }); + } + if (settingsChanges.length === 0) { + return { changed: false, disabled: workspaceDisabled }; } - if (!changed) return { changed: false, disabled: workspaceDisabled }; assertGenerationOpen?.(); - fresh.setValue( - WORKSPACE_SETTING_SCOPE, - 'skills.disabled', - next.length > 0 ? next : undefined, + fresh.setValues( + settingsChanges.map((change) => ({ + scope: WORKSPACE_SETTING_SCOPE, + ...change, + })), + undefined, assertGenerationOpen, ); - return { changed: true, disabled: next }; + return { + changed: true, + disabled: next.disabled, + settingsChanges, + }; }); const persistSettingFn = ( workspace: string, 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 41a1e657b16..8eb1c494988 100644 --- a/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts +++ b/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts @@ -1426,7 +1426,7 @@ describe('createDaemonWorkspaceService', () => { }); }); - it('publishes the reduced disabled list when enabling a skill', async () => { + it('publishes an explicit enabled override for a default-disabled skill', async () => { const publishWorkspaceEvent = vi.fn(); const svc = createDaemonWorkspaceService( makeDeps({ @@ -1434,6 +1434,7 @@ describe('createDaemonWorkspaceService', () => { persistDisabledSkills: vi.fn().mockResolvedValue({ changed: true, disabled: ['orphan'], + settingsChanges: [{ key: 'skills.enabled', value: ['review'] }], }), invokeWorkspaceCommand: vi.fn().mockResolvedValue({ sessionsRefreshed: 1, @@ -1454,8 +1455,8 @@ describe('createDaemonWorkspaceService', () => { expect(publishWorkspaceEvent).toHaveBeenCalledWith({ type: 'settings_changed', data: { - key: 'skills.disabled', - value: ['orphan'], + key: 'skills.enabled', + value: ['review'], scope: 'workspace', }, originatorClientId: 'client-1', @@ -1635,6 +1636,7 @@ describe('createDaemonWorkspaceService', () => { queryWorkspaceStatus: statusQuery( skillStatus({ status: 'disabled', + disabledReason: 'inactive_extension', level: 'extension', extensionName: 'review-ext', }), @@ -1650,6 +1652,73 @@ describe('createDaemonWorkspaceService', () => { expect(persistDisabledSkills).not.toHaveBeenCalled(); }); + it('rejects a legacy inactive extension skill with no disabledReason and not disabled by settings', async () => { + await withIsolatedWorkspace(async ({ workspace }) => { + const persistDisabledSkills = vi.fn(); + const svc = createDaemonWorkspaceService( + makeDeps({ + boundWorkspace: workspace, + queryWorkspaceStatus: statusQuery( + skillStatus({ + status: 'disabled', + disabledReason: undefined, + level: 'extension', + extensionName: 'review-ext', + }), + ), + persistDisabledSkills, + }), + ); + await expect( + svc.setWorkspaceSkillEnabled(makeCtx(), 'review', true), + ).rejects.toMatchObject({ + reason: 'inactive_extension', + }); + expect(persistDisabledSkills).not.toHaveBeenCalled(); + }); + }); + + it('allows toggling a legacy extension skill that is disabled by settings', async () => { + await withIsolatedWorkspace(async ({ workspace }) => { + await writeJson( + path.join(workspace, SETTINGS_DIRECTORY_NAME, 'settings.json'), + { skills: { disabled: ['review'] } }, + ); + const persistDisabledSkills = vi.fn().mockResolvedValue({ + changed: true, + disabled: [], + }); + const svc = createDaemonWorkspaceService( + makeDeps({ + boundWorkspace: workspace, + queryWorkspaceStatus: statusQuery( + skillStatus({ + status: 'disabled', + disabledReason: undefined, + level: 'extension', + extensionName: 'review-ext', + }), + ), + persistDisabledSkills, + isChannelLive: () => false, + }), + ); + await expect( + svc.setWorkspaceSkillEnabled(makeCtx(), 'review', true), + ).resolves.toMatchObject({ + skillName: 'review', + enabled: true, + changed: true, + }); + expect(persistDisabledSkills).toHaveBeenCalledWith( + workspace, + 'review', + true, + undefined, + ); + }); + }); + it('does not refresh or publish when persistence fails', async () => { const invokeWorkspaceCommand = vi.fn(); 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 16d78eb9ffa..f9f7fc5d39a 100644 --- a/packages/cli/src/serve/workspace-service/index.ts +++ b/packages/cli/src/serve/workspace-service/index.ts @@ -47,6 +47,7 @@ import { import { MCP_RESTART_SERVER_DEADLINE_MS } from '@qwen-code/acp-bridge/mcpTimeouts'; import { loadSettings } from '../../config/settings.js'; +import { resolveSkillSettings } from '../../config/skill-settings.js'; import { getWorkspaceTrustStatus } from '../../config/trustedFolders.js'; import { buildPermissionSettings } from '../../config/permission-settings.js'; import { @@ -749,17 +750,20 @@ export function createDaemonWorkspaceService( ); } - const disabled = loadBoundSettings().merged.skills?.disabled; - const disabledNames = new Set( - (Array.isArray(disabled) ? disabled : []) - .filter((name): name is string => typeof name === 'string') - .map((name) => name.trim().toLowerCase()) - .filter(Boolean), - ); + const needsLegacyInactiveCheck = + skill.level === 'extension' && + skill.status === 'disabled' && + skill.disabledReason === undefined; + const disabledBySettings = + needsLegacyInactiveCheck && + resolveSkillSettings(loadBoundSettings(true)).disabledNames.has( + normalizedName, + ); if ( skill.level === 'extension' && skill.status === 'disabled' && - !disabledNames.has(normalizedName) + (skill.disabledReason === 'inactive_extension' || + (skill.disabledReason === undefined && !disabledBySettings)) ) { throw new WorkspaceSkillNotToggleableError( skill.name, @@ -812,16 +816,24 @@ export function createDaemonWorkspaceService( } assertActiveGeneration(); - publishWorkspaceEvent({ - type: 'settings_changed', - data: { - key: 'skills.disabled', + const settingsChanges = persisted.settingsChanges ?? [ + { + key: 'skills.disabled' as const, value: persisted.disabled.length > 0 ? persisted.disabled : undefined, - scope: 'workspace', }, - originatorClientId: ctx.originatorClientId, - }); + ]; + for (const change of settingsChanges) { + publishWorkspaceEvent({ + type: 'settings_changed', + data: { + key: change.key, + value: change.value, + scope: 'workspace', + }, + originatorClientId: ctx.originatorClientId, + }); + } } return { diff --git a/packages/cli/src/serve/workspace-service/types.ts b/packages/cli/src/serve/workspace-service/types.ts index c0ca29bf839..9b6b6a05890 100644 --- a/packages/cli/src/serve/workspace-service/types.ts +++ b/packages/cli/src/serve/workspace-service/types.ts @@ -203,7 +203,7 @@ export interface DaemonWorkspaceService { enabled: boolean, ): Promise<{ toolName: string; enabled: boolean }>; - /** Toggle a skill in the workspace's skills.disabled settings list. */ + /** Toggle a skill in the workspace skill settings. */ setWorkspaceSkillEnabled( ctx: WorkspaceRequestContext, skillName: string, @@ -345,6 +345,10 @@ export interface WorkspaceSkillToggleResult { export interface PersistDisabledSkillResult { changed: boolean; disabled: string[]; + settingsChanges?: Array<{ + key: 'skills.disabled' | 'skills.enabled'; + value: string[] | undefined; + }>; } export type WorkspaceSkillNotToggleableReason = diff --git a/packages/cli/src/serve/workspace-skills-mapping.test.ts b/packages/cli/src/serve/workspace-skills-mapping.test.ts index 7c439c0b3a0..cd936cb2b38 100644 --- a/packages/cli/src/serve/workspace-skills-mapping.test.ts +++ b/packages/cli/src/serve/workspace-skills-mapping.test.ts @@ -59,21 +59,27 @@ describe('mapSkillConfigToStatus', () => { it('marks a settings-disabled skill as disabled', () => { const status = mapSkillConfigToStatus( makeSkill({ name: 'internal' }), - new Set(['internal']), + new Map([['internal', { reason: 'hard', lockedScope: 'user' }]]), ); expect(status.status).toBe('disabled'); expect(status.modelInvocable).toBe(true); expect(status.name).toBe('internal'); + expect(status.disabledReason).toBe('hard'); + expect(status.lockedScope).toBe('user'); }); it('marks a forced-disabled skill as disabled', () => { - const status = mapSkillConfigToStatus(makeSkill(), new Set(), { - disabled: true, - }); + const status = mapSkillConfigToStatus( + makeSkill(), + new Map([['review', { reason: 'hard', lockedScope: 'user' }]]), + { disabled: true }, + ); expect(status.status).toBe('disabled'); expect(status.modelInvocable).toBe(true); + expect(status.disabledReason).toBe('inactive_extension'); + expect(status).not.toHaveProperty('lockedScope'); }); it('surfaces optional model and extensionName only when present', () => { diff --git a/packages/cli/src/serve/workspace-skills-mapping.ts b/packages/cli/src/serve/workspace-skills-mapping.ts index f3ebebd2c68..fc3dd0cca11 100644 --- a/packages/cli/src/serve/workspace-skills-mapping.ts +++ b/packages/cli/src/serve/workspace-skills-mapping.ts @@ -6,6 +6,7 @@ import type { SkillConfig } from '@qwen-code/qwen-code-core'; import type { ServeWorkspaceSkillStatus } from '@qwen-code/acp-bridge/status'; +import type { SkillDisablement } from '../config/skill-settings.js'; /** * Maps a `SkillConfig` (as `SkillManager.listSkills()` returns) to the @@ -16,18 +17,25 @@ import type { ServeWorkspaceSkillStatus } from '@qwen-code/acp-bridge/status'; */ export function mapSkillConfigToStatus( skill: SkillConfig, - disabledSkillNames: ReadonlySet = new Set(), + disablements: ReadonlyMap = new Map(), opts: { disabled?: boolean } = {}, ): ServeWorkspaceSkillStatus { - const userDisabled = disabledSkillNames.has(skill.name.toLowerCase()); + const disablement = disablements.get(skill.name.toLowerCase()); + const disabledReason = opts.disabled + ? 'inactive_extension' + : disablement?.reason; const modelInvocable = skill.disableModelInvocation !== true; return { kind: 'skill', - status: opts.disabled || userDisabled ? 'disabled' : 'ok', + status: disabledReason ? 'disabled' : 'ok', name: skill.name, description: skill.description, level: skill.level, modelInvocable, + ...(disabledReason ? { disabledReason } : {}), + ...(!opts.disabled && disablement?.lockedScope + ? { lockedScope: disablement.lockedScope } + : {}), ...(skill.userInvocable === false ? { userInvocable: false as const } : {}), installedPath: skill.filePath, ...(skill.argumentHint ? { argumentHint: skill.argumentHint } : {}), diff --git a/packages/cli/src/serve/workspace-skills-status.test.ts b/packages/cli/src/serve/workspace-skills-status.test.ts index fba7ecd18a8..2c036f3bbb9 100644 --- a/packages/cli/src/serve/workspace-skills-status.test.ts +++ b/packages/cli/src/serve/workspace-skills-status.test.ts @@ -94,7 +94,12 @@ describe('createWorkspaceSkillsStatusProvider', () => { await fsp.mkdir(path.join(workspace, '.qwen'), { recursive: true }); await fsp.writeFile( path.join(workspace, '.qwen', 'settings.json'), - JSON.stringify({ skills: { disabled: ['disabled'] } }), + JSON.stringify({ + skills: { + defaultDisabled: ['disabled', 'enabled'], + enabled: ['ENABLED'], + }, + }), ); const provider = createWorkspaceSkillsStatusProvider(); @@ -109,11 +114,112 @@ describe('createWorkspaceSkillsStatusProvider', () => { { name: 'disabled', status: 'disabled', + disabledReason: 'default', installedPath: '/skills/disabled/SKILL.md', }, ]); }); + it('marks hard-disabled skills with a hard disable reason', async () => { + vi.spyOn(SkillManager.prototype, 'listSkills').mockResolvedValueOnce([ + { + name: 'enabled', + description: 'Enabled skill', + body: 'Visible', + filePath: '/skills/enabled/SKILL.md', + level: 'project', + }, + { + name: 'disabled', + description: 'Disabled skill', + body: 'Hidden', + filePath: '/skills/disabled/SKILL.md', + level: 'project', + }, + ]); + const workspace = await fsp.mkdtemp( + path.join(os.tmpdir(), 'qwen-skills-hard-disabled-'), + ); + await fsp.mkdir(path.join(workspace, '.qwen'), { recursive: true }); + await fsp.writeFile( + path.join(workspace, '.qwen', 'settings.json'), + JSON.stringify({ + skills: { + disabled: ['disabled'], + }, + }), + ); + const provider = createWorkspaceSkillsStatusProvider(); + + const status = await provider(workspace); + + expect(status.skills).toMatchObject([ + { + name: 'enabled', + status: 'ok', + installedPath: '/skills/enabled/SKILL.md', + }, + { + name: 'disabled', + status: 'disabled', + disabledReason: 'hard', + installedPath: '/skills/disabled/SKILL.md', + }, + ]); + // A workspace-scope hard disable is not locked by a higher scope. + const hardDisabled = status.skills.find((s) => s.name === 'disabled'); + expect(hardDisabled?.lockedScope).toBeUndefined(); + }); + + it('resolves disablements in safe mode (status matches execution)', async () => { + vi.spyOn(SkillManager.prototype, 'listSkills').mockResolvedValueOnce([ + { + name: 'available', + description: 'Available skill', + body: 'Visible', + filePath: '/skills/available/SKILL.md', + level: 'bundled', + }, + { + name: 'blocked', + description: 'Blocked skill', + body: 'Hidden', + filePath: '/skills/blocked/SKILL.md', + level: 'bundled', + }, + ]); + const workspace = await fsp.mkdtemp( + path.join(os.tmpdir(), 'qwen-skills-safe-mode-'), + ); + await fsp.mkdir(path.join(workspace, '.qwen'), { recursive: true }); + await fsp.writeFile( + path.join(workspace, '.qwen', 'settings.json'), + JSON.stringify({ + skills: { + disabled: ['blocked'], + }, + }), + ); + const saved = process.env['QWEN_CODE_SAFE_MODE']; + process.env['QWEN_CODE_SAFE_MODE'] = '1'; + try { + const provider = createWorkspaceSkillsStatusProvider(); + + const status = await provider(workspace); + + expect(status.skills).toMatchObject([ + { name: 'available', status: 'ok' }, + { name: 'blocked', status: 'disabled', disabledReason: 'hard' }, + ]); + } finally { + if (saved === undefined) { + delete process.env['QWEN_CODE_SAFE_MODE']; + } else { + process.env['QWEN_CODE_SAFE_MODE'] = saved; + } + } + }); + it('does not read workspace-level disabled skills when untrusted', async () => { vi.spyOn(SkillManager.prototype, 'listSkills').mockResolvedValueOnce([ { diff --git a/packages/cli/src/serve/workspace-skills-status.ts b/packages/cli/src/serve/workspace-skills-status.ts index 9ed2e9ffd2e..663847b8008 100644 --- a/packages/cli/src/serve/workspace-skills-status.ts +++ b/packages/cli/src/serve/workspace-skills-status.ts @@ -37,6 +37,7 @@ import { STATUS_SCHEMA_VERSION } from '@qwen-code/acp-bridge/status'; import { loadSettings } from '../config/settings.js'; import { writeStderrLine } from '../utils/stdioHelpers.js'; import { mapSkillConfigToStatus } from './workspace-skills-mapping.js'; +import { resolveSkillSettings } from '../config/skill-settings.js'; export interface WorkspaceSkillsStatusProvider { (workspaceCwd: string): Promise; @@ -105,13 +106,22 @@ async function buildWorkspaceSkillsStatus( skillManager = new SkillManager(shim as Config); managers.set(workspaceCwd, skillManager); } - const disabled = readDisabledSkillNames(workspaceCwd, workspaceTrusted); + const disablements = resolveSkillSettings( + loadSettings(workspaceCwd, { + consumeCorruptionEnvVars: false, + skipLoadEnvironment: !workspaceTrusted, + skipWorkspaceSettings: !workspaceTrusted, + workspaceTrusted, + }), + ).disablements; const skills = await skillManager.listSkills(); return { v: STATUS_SCHEMA_VERSION, workspaceCwd, initialized: true, - skills: skills.map((skill) => mapSkillConfigToStatus(skill, disabled)), + skills: skills.map((skill) => + mapSkillConfigToStatus(skill, disablements), + ), }; } catch (error) { const message = error instanceof Error ? error.message : String(error); @@ -133,22 +143,3 @@ async function buildWorkspaceSkillsStatus( }; } } - -function readDisabledSkillNames( - workspaceCwd: string, - workspaceTrusted: boolean, -): ReadonlySet { - const raw = loadSettings(workspaceCwd, { - consumeCorruptionEnvVars: false, - skipLoadEnvironment: !workspaceTrusted, - skipWorkspaceSettings: !workspaceTrusted, - workspaceTrusted, - }).merged.skills?.disabled; - if (!Array.isArray(raw)) return new Set(); - return new Set( - raw - .filter((name): name is string => typeof name === 'string') - .map((name) => name.trim().toLowerCase()) - .filter(Boolean), - ); -} diff --git a/packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx b/packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx index 6374ebedefc..f8b82f670af 100644 --- a/packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx +++ b/packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx @@ -32,6 +32,11 @@ import type { } from '@qwen-code/qwen-code-core'; import type { LoadedSettings } from '../../../config/settings.js'; import { SettingScope } from '../../../config/settings.js'; +import { + computeWorkspaceSkillListUpdates, + resolveSkillSettings, + skillSettingStrings, +} from '../../../config/skill-settings.js'; import { t } from '../../../i18n/index.js'; import { levelLabel } from '../../utils/skill-level-label.js'; import type { UseHistoryManagerReturn } from '../../hooks/useHistoryManager.js'; @@ -153,9 +158,8 @@ export function SkillsManagerDialog({ // settings snapshot at open time — using `useMemo` keyed on `settings` // would re-derive on every parent re-render and could thrash the // `selectedKeys` derivation below. - const initialWorkspaceDisabled = useMemo( - () => - new Set(normalizeNames(namesFromScope(settings, SettingScope.Workspace))), + const initialResolved = useMemo( + () => resolveSkillSettings(settings), [settings], ); const higher = useMemo(() => buildHigherDisabled(settings), [settings]); @@ -198,16 +202,25 @@ export function SkillsManagerDialog({ [allSkills, higher.set], ); - // Initial selection: every unlocked skill that the workspace has NOT - // disabled. Checked = enabled. + const initialSelectedKeys = useMemo( + () => + new Set( + unlockedSkills + .filter( + (skill) => !initialResolved.disabledNames.has(lower(skill.name)), + ) + .map((skill) => skill.name), + ), + [initialResolved, unlockedSkills], + ); + + // Initial selection: every effectively enabled, unlocked skill. + // Checked = enabled. const [selectedKeys, setSelectedKeys] = useState(null); useEffect(() => { if (selectedKeys !== null || unlockedSkills.length === 0) return; - const initial = unlockedSkills - .filter((s) => !initialWorkspaceDisabled.has(lower(s.name))) - .map((s) => s.name); - setSelectedKeys(initial); - }, [unlockedSkills, initialWorkspaceDisabled, selectedKeys]); + setSelectedKeys([...initialSelectedKeys]); + }, [unlockedSkills, initialSelectedKeys, selectedKeys]); const filteredUnlocked = useMemo(() => { const normalizedQuery = query.trim().toLowerCase(); @@ -292,60 +305,48 @@ export function SkillsManagerDialog({ } const selected = new Set(selectedKeys ?? []); - // workspace disabled = unlocked skills NOT in the selection. - // Locked names are intentionally excluded so we don't write redundant - // entries the higher scope is already enforcing. - const previousWorkspace = namesFromScope(settings, SettingScope.Workspace); - // Only string entries can be re-emitted with their original casing. - // A stray non-string survived the namesFromScope `Array.isArray` guard - // but would crash `lower()` (`.trim is not a function`). - const previousStrings = previousWorkspace.filter( - (n): n is string => typeof n === 'string', - ); - const previousMap = new Map(previousStrings.map((n) => [lower(n), n])); - const nextDisabled: string[] = []; - // Preserve workspace entries that don't correspond to any currently- - // loaded skill (e.g. from a different git branch, uninstalled - // extension, deleted .qwen/skills/ directory). Without this, opening - // /skills and pressing Esc would silently drop orphaned entries and - // the user's prior disable setting would vanish if the skill later - // reappears (branch switch, extension reinstall). - // - // Use `allSkills` (not `unlockedSkills`) as the "known" set so that - // skills disabled at a higher scope (locked) are NOT treated as - // orphans and re-emitted — that would violate invariant #2 (locked - // names never appear in the workspace write). - const allKnownLower = new Set(allSkills.map((s) => lower(s.name))); - for (const prev of previousStrings) { - if (!allKnownLower.has(lower(prev))) { - nextDisabled.push(prev); - } - } - for (const s of unlockedSkills) { - if (selected.has(s.name)) continue; - const existing = previousMap.get(lower(s.name)); - nextDisabled.push(existing ?? s.name); - } - - // Skip the disk write + refresh roundtrip when the on-disk state - // already matches what we'd write. Comparing normalized lists keeps - // whitespace/case-only edits in the JSON file from being treated as - // changes. `previousWorkspace` includes only workspace-scope entries - // (matching what we're about to write) — locked entries from higher - // scopes are not in this list, so they don't affect the comparison. - const prevNormalized = normalizeNames(previousWorkspace).sort(); - const nextNormalized = normalizeNames(nextDisabled).sort(); - const unchanged = - prevNormalized.length === nextNormalized.length && - prevNormalized.every((n, i) => n === nextNormalized[i]); - if (unchanged) return 'ok'; + const workspaceDisabled = namesFromScope( + settings, + SettingScope.Workspace, + ).filter((name): name is string => typeof name === 'string'); + const lockedNames = new Set(lockedSkills.map((skill) => lower(skill.name))); + const { disabled, enabled, disabledChanged, enabledChanged } = + computeWorkspaceSkillListUpdates( + workspaceDisabled, + lockedNames, + skillSettingStrings(settings, SettingScope.Workspace, 'enabled'), + unlockedSkills.map((skill) => ({ + name: skill.name, + wasEnabled: initialSelectedKeys.has(skill.name), + isEnabled: selected.has(skill.name), + defaultDisabled: + initialResolved.defaultDisabledNames.has(lower(skill.name)) && + !initialResolved.enabledNames.has(lower(skill.name)), + })), + ); + if (!disabledChanged && !enabledChanged) return 'ok'; try { - settings.setValue( - SettingScope.Workspace, - 'skills.disabled', - nextDisabled.length > 0 ? nextDisabled : undefined, - ); + settings.setValues([ + ...(disabledChanged + ? [ + { + scope: SettingScope.Workspace, + key: 'skills.disabled', + value: disabled.length > 0 ? disabled : undefined, + }, + ] + : []), + ...(enabledChanged + ? [ + { + scope: SettingScope.Workspace, + key: 'skills.enabled', + value: enabled.length > 0 ? enabled : undefined, + }, + ] + : []), + ]); } catch (e) { addItem( { @@ -394,7 +395,9 @@ export function SkillsManagerDialog({ return 'ok'; }, [ addItem, - allSkills, + initialResolved, + initialSelectedKeys, + lockedSkills, reloadCommands, selectedKeys, settings, diff --git a/packages/sdk-typescript/src/daemon/types.ts b/packages/sdk-typescript/src/daemon/types.ts index 5f8768dc681..8c65667c714 100644 --- a/packages/sdk-typescript/src/daemon/types.ts +++ b/packages/sdk-typescript/src/daemon/types.ts @@ -1413,6 +1413,8 @@ export interface DaemonWorkspaceSkillStatus extends DaemonStatusCell { description: string; level: DaemonSkillLevel; modelInvocable: boolean; + disabledReason?: 'hard' | 'default' | 'inactive_extension'; + lockedScope?: 'system' | 'user' | 'systemDefaults'; userInvocable?: false; installedPath?: string; argumentHint?: string; diff --git a/packages/vscode-ide-companion/schemas/settings.schema.json b/packages/vscode-ide-companion/schemas/settings.schema.json index 19686c34a13..79b9f17873c 100644 --- a/packages/vscode-ide-companion/schemas/settings.schema.json +++ b/packages/vscode-ide-companion/schemas/settings.schema.json @@ -903,6 +903,20 @@ "type": "string" } }, + "defaultDisabled": { + "description": "Skill names disabled by default unless explicitly enabled through skills.enabled. Matched case-insensitively and UNION-merged across settings scopes. skills.disabled always wins.", + "type": "array", + "items": { + "type": "string" + } + }, + "enabled": { + "description": "Explicit opt-ins that override matching skills.defaultDisabled entries. Matched case-insensitively and UNION-merged across settings scopes. Cannot override skills.disabled.", + "type": "array", + "items": { + "type": "string" + } + }, "directories": { "description": "Additional directories to scan for skills (SKILL.md files). Entries should be absolute paths or ~-prefixed; relative paths resolve against the working directory. Each directory is scanned one level deep for subdirectories containing a SKILL.md file. Skills from these directories are loaded at user level, after the default user skill directories; a custom skill with the same name as one in the default user directories will not override it. Only point this at trusted locations, since skills can define hooks and commands.", "type": "array",