From c0a2fee4f778a272de617b3547dc83cd7e192f36 Mon Sep 17 00:00:00 2001 From: callmeYe <512217680@qq.com> Date: Fri, 14 Aug 2026 09:04:09 +0000 Subject: [PATCH] fix(serve): allow preinstall skill batch toggles (#9139) Treat names absent from the installed Skill snapshot as valid batch targets so their workspace state can be declared before installation. Preserve the existing persistence semantics: an undeclared enable is a no-op, an existing workspace disable is removed, and disable writes the declaration. --- docs/design/daemon-skill-batch-toggle.md | 26 ++++---- .../src/serve/routes/workspace-skills.test.ts | 20 +++--- packages/cli/src/serve/run-qwen-serve.test.ts | 32 ++++++++-- .../__tests__/facade.test.ts | 62 +++++++++++++------ .../cli/src/serve/workspace-service/index.ts | 10 +-- 5 files changed, 99 insertions(+), 51 deletions(-) diff --git a/docs/design/daemon-skill-batch-toggle.md b/docs/design/daemon-skill-batch-toggle.md index c4e2ad03ba..ab2c315186 100644 --- a/docs/design/daemon-skill-batch-toggle.md +++ b/docs/design/daemon-skill-batch-toggle.md @@ -24,11 +24,16 @@ The request body is: `skillNames` is a non-empty string array with at most 100 entries. Names are trimmed and deduplicated case-insensitively while preserving first-seen order. -The response is best-effort 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. +The response is best-effort for expected target errors: installed targets are +validated against one status snapshot, all valid names are persisted in one +locked write, and changes are applied with one live-session refresh. Names +that are not installed remain valid so callers can declare their state before +installation. Enabling one removes a matching workspace `skills.disabled` +entry and is otherwise a no-op, except for the existing `defaultDisabled` +override behavior; disabling one writes `skills.disabled`. Hidden, +inactive-extension, and locked targets are returned without blocking valid +targets. Unexpected persistence and runtime-generation failures fail the whole +request. ```json { @@ -46,15 +51,14 @@ persistence and runtime-generation failures fail the whole request. "skillName": "deploy", "enabled": false, "changed": true - } - ], - "errors": [ + }, { "skillName": "missing", - "code": "skill_not_found", - "error": "Skill not found: missing" + "enabled": false, + "changed": true } - ] + ], + "errors": [] } ``` diff --git a/packages/cli/src/serve/routes/workspace-skills.test.ts b/packages/cli/src/serve/routes/workspace-skills.test.ts index a48722d739..f00d7af910 100644 --- a/packages/cli/src/serve/routes/workspace-skills.test.ts +++ b/packages/cli/src/serve/routes/workspace-skills.test.ts @@ -173,20 +173,18 @@ describe('workspace Skill management routes', () => { expect(harness.deleteWorkspaceSkill).not.toHaveBeenCalled(); }); - it('toggles a deduplicated Skill batch and returns per-target errors', async () => { + it('toggles a deduplicated Skill batch and returns per-target outcomes', async () => { const harness = createHarness(); harness.setWorkspaceSkillsEnabled.mockResolvedValueOnce({ enabled: false, activation: 'applied', sessionsRefreshed: 1, sessionsFailed: 0, - results: [{ skillName: 'review', enabled: false, changed: true }], + results: [ + { skillName: 'review', enabled: false, changed: true }, + { skillName: 'missing', enabled: false, changed: true }, + ], errors: [ - { - skillName: 'missing', - code: 'skill_not_found', - error: 'Skill not found: missing', - }, { skillName: 'locked', code: 'skill_not_toggleable', @@ -216,13 +214,13 @@ describe('workspace Skill management routes', () => { enabled: false, changed: true, }, - ], - errors: [ { skillName: 'missing', - code: 'skill_not_found', - error: 'Skill not found: missing', + enabled: false, + changed: true, }, + ], + errors: [ { skillName: 'locked', code: 'skill_not_toggleable', diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index e6f5e6de89..346956c8a1 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -914,6 +914,30 @@ describe('workspace skill settings persistence', () => { expect(savedUser.skills.disabled).toEqual(['locked-skill']); expect(savedUser.skills.enabled).toBeUndefined(); + const preinstallNoop = await persistDisabledSkillsBatch!( + workspace, + ['future-skill'], + true, + ); + expect(preinstallNoop.outcomes).toEqual([ + { skillName: 'future-skill', changed: false }, + ]); + expect(preinstallNoop.settingsChanges).toEqual([]); + expect(setValues).toHaveBeenCalledOnce(); + + const preinstallEnable = await persistDisabledSkillsBatch!( + workspace, + ['orphan'], + true, + ); + expect(preinstallEnable.outcomes).toEqual([ + { skillName: 'orphan', changed: true }, + ]); + expect(preinstallEnable.settingsChanges).toEqual([ + { key: 'skills.disabled', value: ['review', 'alpha'] }, + ]); + expect(setValues).toHaveBeenCalledTimes(2); + const enableResult = await persistDisabledSkillsBatch!( workspace, ['opt-in'], @@ -929,16 +953,12 @@ describe('workspace skill settings persistence', () => { value: ['opt-in'], }, ]); - expect(setValues).toHaveBeenCalledTimes(2); + expect(setValues).toHaveBeenCalledTimes(3); 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.disabled).toEqual(['review', 'alpha']); expect(savedAfterEnable.skills.enabled).toEqual(['opt-in']); const guard = vi.fn(); 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 db6637d793..aa209c9d0f 100644 --- a/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts +++ b/packages/cli/src/serve/workspace-service/__tests__/facade.test.ts @@ -2098,6 +2098,38 @@ describe('createDaemonWorkspaceService', () => { }, ]; + it('accepts enabling a Skill before installation as an idempotent result', async () => { + const persistDisabledSkillsBatch = vi.fn().mockResolvedValue({ + outcomes: [{ skillName: 'future-skill', changed: false }], + settingsChanges: [], + }); + const svc = createDaemonWorkspaceService( + makeDeps({ + queryWorkspaceStatus: vi.fn().mockResolvedValue({ + v: 1, + workspaceCwd: '/workspace', + initialized: true, + skills, + }), + persistDisabledSkillsBatch, + isChannelLive: () => false, + }), + ); + + await expect( + svc.setWorkspaceSkillsEnabled(makeCtx(), ['future-skill'], true), + ).resolves.toMatchObject({ + results: [{ skillName: 'future-skill', enabled: true, changed: false }], + errors: [], + }); + expect(persistDisabledSkillsBatch).toHaveBeenCalledWith( + '/workspace', + ['future-skill'], + true, + undefined, + ); + }); + it('persists and refreshes once while preserving ordered target outcomes', async () => { const queryWorkspaceStatus = vi.fn().mockResolvedValue({ v: 1, @@ -2108,6 +2140,7 @@ describe('createDaemonWorkspaceService', () => { const persistDisabledSkillsBatch = vi.fn().mockResolvedValue({ outcomes: [ { skillName: 'review', changed: true }, + { skillName: 'missing', changed: true }, { skillName: 'locked', error: new WorkspaceSkillNotToggleableError( @@ -2119,7 +2152,10 @@ describe('createDaemonWorkspaceService', () => { { skillName: 'deploy', changed: true }, ], settingsChanges: [ - { key: 'skills.disabled', value: ['review', 'deploy'] }, + { + key: 'skills.disabled', + value: ['review', 'missing', 'deploy'], + }, ], }); const invokeWorkspaceCommand = vi.fn().mockResolvedValue({ @@ -2147,7 +2183,7 @@ describe('createDaemonWorkspaceService', () => { expect(persistDisabledSkillsBatch).toHaveBeenCalledOnce(); expect(persistDisabledSkillsBatch).toHaveBeenCalledWith( '/workspace', - ['review', 'locked', 'deploy'], + ['review', 'missing', 'locked', 'deploy'], false, undefined, ); @@ -2163,14 +2199,10 @@ describe('createDaemonWorkspaceService', () => { sessionsFailed: 0, results: [ { skillName: 'review', enabled: false, changed: true }, + { skillName: 'missing', 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', @@ -2197,7 +2229,7 @@ describe('createDaemonWorkspaceService', () => { type: 'settings_changed', data: { key: 'skills.disabled', - value: ['review', 'deploy'], + value: ['review', 'missing', 'deploy'], scope: 'workspace', }, originatorClientId: 'client-1', @@ -2225,6 +2257,7 @@ describe('createDaemonWorkspaceService', () => { ), }, { skillName: 'review', changed: true }, + { skillName: 'missing', changed: true }, ], settingsChanges: [], }), @@ -2240,6 +2273,7 @@ describe('createDaemonWorkspaceService', () => { expect(result.results).toEqual([ { skillName: 'review', enabled: false, changed: true }, + { skillName: 'missing', enabled: false, changed: true }, { skillName: 'deploy', enabled: false, changed: true }, ]); expect(result.errors).toEqual([ @@ -2250,11 +2284,6 @@ describe('createDaemonWorkspaceService', () => { reason: 'locked', lockedScope: 'user', }, - { - skillName: 'missing', - code: 'skill_not_found', - error: 'Skill not found: missing', - }, ]); }); @@ -2329,18 +2358,13 @@ describe('createDaemonWorkspaceService', () => { ); await expect( - svc.setWorkspaceSkillsEnabled( - makeCtx(), - ['missing', 'hidden', 'inactive'], - false, - ), + svc.setWorkspaceSkillsEnabled(makeCtx(), ['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' }, ], diff --git a/packages/cli/src/serve/workspace-service/index.ts b/packages/cli/src/serve/workspace-service/index.ts index 599043574d..e88cbee494 100644 --- a/packages/cli/src/serve/workspace-service/index.ts +++ b/packages/cli/src/serve/workspace-service/index.ts @@ -959,10 +959,12 @@ export function createDaemonWorkspaceService( 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) { + targets.push({ requestedName, skillName: requestedName }); + continue; + } + let domainError: unknown; + if (skill.userInvocable === false) { domainError = new WorkspaceSkillNotToggleableError( skill.name, 'not_user_invocable', @@ -990,7 +992,7 @@ export function createDaemonWorkspaceService( if (!error) throw domainError; targets.push({ requestedName, error }); } else { - targets.push({ requestedName, skillName: skill!.name }); + targets.push({ requestedName, skillName: skill.name }); } }