From 8dfa7613bece3f032bb874cb4f55045a8b5137cd Mon Sep 17 00:00:00 2001 From: ytahdn <1294726970@qq.com> Date: Fri, 3 Jul 2026 14:58:46 +0800 Subject: [PATCH] fix(serve): respect disabled skill settings (#6223) * fix(serve): respect disabled skills in status * test(serve): align skill disabled status coverage --------- Co-authored-by: ytahdn --- .../cli/src/acp-integration/acpAgent.test.ts | 39 +++++++++++ packages/cli/src/acp-integration/acpAgent.ts | 3 +- .../acp-integration/session/Session.test.ts | 63 +++++++++++++++++ .../src/acp-integration/session/Session.ts | 68 +++++++++++-------- .../serve/workspace-skills-mapping.test.ts | 15 +++- .../cli/src/serve/workspace-skills-mapping.ts | 4 +- .../src/serve/workspace-skills-status.test.ts | 44 ++++++++++++ .../cli/src/serve/workspace-skills-status.ts | 15 +++- .../webui/src/daemon/session/mappers.test.ts | 8 +++ packages/webui/src/daemon/session/mappers.ts | 8 ++- 10 files changed, 231 insertions(+), 36 deletions(-) diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index 4d94909355..98daa37552 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -1606,6 +1606,23 @@ describe('QwenAgent MCP SSE/HTTP support', () => { skillRoot: '/secret', hooks: { pre: ['secret-hook'] }, }, + { + name: 'manual-only', + description: 'Manual only', + level: 'project', + argumentHint: '[topic]', + disableModelInvocation: true, + body: 'manual secret body', + filePath: '/manual/SKILL.md', + }, + { + name: 'disabled-skill', + description: 'Disabled by settings', + level: 'project', + disableModelInvocation: false, + body: 'disabled secret body', + filePath: '/disabled/SKILL.md', + }, ]); mockConfig = { ...mockConfig, @@ -1635,6 +1652,9 @@ describe('QwenAgent MCP SSE/HTTP support', () => { isMcpServerDisabled: vi .fn() .mockImplementation((name: string) => name === 'disabled'), + getDisabledSkillNames: vi + .fn() + .mockReturnValue(new Set(['disabled-skill'])), getSkillManager: vi.fn().mockReturnValue({ listSkills }), getAuthType: vi.fn().mockReturnValue('qwen'), getAllConfiguredModels: vi.fn().mockReturnValue([ @@ -1808,9 +1828,28 @@ describe('QwenAgent MCP SSE/HTTP support', () => { argumentHint: '[path]', modelInvocable: true, }, + { + kind: 'skill', + status: 'ok', + name: 'manual-only', + description: 'Manual only', + level: 'project', + argumentHint: '[topic]', + modelInvocable: false, + }, + { + kind: 'skill', + status: 'disabled', + name: 'disabled-skill', + description: 'Disabled by settings', + level: 'project', + modelInvocable: true, + }, ], }); expect(JSON.stringify(skills)).not.toContain('secret skill body'); + expect(JSON.stringify(skills)).not.toContain('manual secret body'); + expect(JSON.stringify(skills)).not.toContain('disabled secret body'); expect(JSON.stringify(skills)).not.toContain('/secret'); expect(JSON.stringify(skills)).not.toContain('secret-hook'); diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index 59a2ddecf2..53d86a1991 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -3891,12 +3891,13 @@ class QwenAgent implements Agent { } try { + const disabled = config.getDisabledSkillNames(); const skills = await skillManager.listSkills(); return { v: STATUS_SCHEMA_VERSION, workspaceCwd: this.workspaceCwd(config), initialized: true, - skills: skills.map(mapSkillConfigToStatus), + skills: skills.map((skill) => mapSkillConfigToStatus(skill, disabled)), }; } catch (error) { return { diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 2af66114f9..0bf16ccac6 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -463,6 +463,7 @@ describe('Session', () => { .mockReturnValue(mockBackgroundShellRegistry), getMonitorRegistry: vi.fn().mockReturnValue(mockMonitorRegistry), getFileHistoryService: vi.fn().mockReturnValue(mockFileHistoryService), + getDisabledSkillNames: vi.fn().mockReturnValue(new Set()), } as unknown as Config; mockClient = { @@ -1571,6 +1572,68 @@ describe('Session', () => { ); }); + it('omits skills disabled in settings from availableSkills and details', async () => { + getAvailableCommandsSpy.mockResolvedValueOnce([ + { + name: 'disabled-command', + description: 'Disabled slash skill', + kind: 'skill', + skillDetail: { + name: 'disabled-command', + description: 'Disabled slash skill', + body: 'Hidden instructions', + level: 'project', + }, + }, + ]); + mockConfig.getDisabledSkillNames = vi + .fn() + .mockReturnValue(new Set(['disabled-skill', 'disabled-command'])); + mockConfig.getSkillManager = vi.fn().mockReturnValue({ + listSkills: vi.fn().mockResolvedValue([ + { + name: 'enabled-skill', + description: 'Enabled skill', + body: 'Visible instructions', + filePath: '/skills/enabled-skill/SKILL.md', + level: 'project', + }, + { + name: 'disabled-skill', + description: 'Disabled skill', + body: 'Hidden instructions', + filePath: '/skills/disabled-skill/SKILL.md', + level: 'project', + }, + ]), + }); + + await session.sendAvailableCommandsUpdate(); + + const update = vi + .mocked(mockClient.sessionUpdate) + .mock.calls.map(([call]) => call) + .find( + (call) => call.update.sessionUpdate === 'available_commands_update', + ) as { + update: { + availableCommands: Array<{ name: string }>; + _meta: { + availableSkills: string[]; + availableSkillDetails: Array<{ name: string }>; + }; + }; + }; + expect( + update.update.availableCommands.map((command) => command.name), + ).not.toContain('disabled-command'); + const meta = update.update._meta; + expect(meta.availableSkills).toEqual(['enabled-skill']); + expect(meta.availableSkillDetails.map((detail) => detail.name)).toEqual([ + 'enabled-skill', + ]); + }); + it('swallows errors and does not throw', async () => { getAvailableCommandsSpy.mockRejectedValueOnce( new Error('Command discovery failed'), diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 1f887c606f..2c556d616a 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -710,36 +710,44 @@ export async function buildAvailableCommandsSnapshot( 'acp', settings, ); + const disabledSkillNames = config.getDisabledSkillNames(); - const availableCommands: AvailableCommand[] = slashCommands.map((cmd) => { - const acceptsInput = - cmd.acceptsInput ?? - (cmd.kind !== CommandKind.BUILT_IN || - cmd.completion != null || - cmd.argumentHint != null || - (cmd.subCommands != null && cmd.subCommands.length > 0)); - return { - name: cmd.name, - description: cmd.description, - input: acceptsInput ? { hint: cmd.argumentHint ?? '' } : null, - _meta: { - argumentHint: cmd.argumentHint, - source: cmd.source, - sourceLabel: cmd.sourceLabel, - supportedModes: getEffectiveSupportedModes(cmd), - subcommands: getCommandSubcommandNames(cmd), - modelInvocable: cmd.modelInvocable === true, - // Carry aliases so a channel consumer (which only sees the wire snapshot, - // not the command registry) can recognize an aliased command and avoid - // tagging it. _meta is ACP's extension point; omitted when there are none - // so command entries without aliases stay byte-identical on the wire. - ...(cmd.altNames && cmd.altNames.length > 0 - ? { altNames: cmd.altNames } - : {}), - }, - }; + const visibleSlashCommands = slashCommands.filter((cmd) => { + if (cmd.kind !== CommandKind.SKILL || !cmd.skillDetail) return true; + return !disabledSkillNames.has(cmd.skillDetail.name.toLowerCase()); }); + const availableCommands: AvailableCommand[] = visibleSlashCommands.map( + (cmd) => { + const acceptsInput = + cmd.acceptsInput ?? + (cmd.kind !== CommandKind.BUILT_IN || + cmd.completion != null || + cmd.argumentHint != null || + (cmd.subCommands != null && cmd.subCommands.length > 0)); + return { + name: cmd.name, + description: cmd.description, + input: acceptsInput ? { hint: cmd.argumentHint ?? '' } : null, + _meta: { + argumentHint: cmd.argumentHint, + source: cmd.source, + sourceLabel: cmd.sourceLabel, + supportedModes: getEffectiveSupportedModes(cmd), + subcommands: getCommandSubcommandNames(cmd), + modelInvocable: cmd.modelInvocable === true, + // Carry aliases so a channel consumer (which only sees the wire snapshot, + // not the command registry) can recognize an aliased command and avoid + // tagging it. _meta is ACP's extension point; omitted when there are none + // so command entries without aliases stay byte-identical on the wire. + ...(cmd.altNames && cmd.altNames.length > 0 + ? { altNames: cmd.altNames } + : {}), + }, + }; + }, + ); + let availableSkills: string[] | undefined; const skillDetailsByName = new Map< string, @@ -748,7 +756,9 @@ export async function buildAvailableCommandsSnapshot( try { const skillManager = config.getSkillManager(); if (skillManager) { - const skills = await skillManager.listSkills(); + const skills = (await skillManager.listSkills()).filter( + (skill) => !disabledSkillNames.has(skill.name.toLowerCase()), + ); availableSkills = skills.map((skill) => skill.name); for (const skill of skills) { skillDetailsByName.set(skill.name, { @@ -765,7 +775,7 @@ export async function buildAvailableCommandsSnapshot( debugLogger.error('Error loading available skills:', error); } - for (const command of slashCommands) { + for (const command of visibleSlashCommands) { if (command.kind !== CommandKind.SKILL || !command.skillDetail) { continue; } diff --git a/packages/cli/src/serve/workspace-skills-mapping.test.ts b/packages/cli/src/serve/workspace-skills-mapping.test.ts index 33f73acb49..f8d6dad38f 100644 --- a/packages/cli/src/serve/workspace-skills-mapping.test.ts +++ b/packages/cli/src/serve/workspace-skills-mapping.test.ts @@ -34,16 +34,27 @@ describe('mapSkillConfigToStatus', () => { }); }); - it('marks a disable-model-invocation skill as disabled', () => { + it('keeps a disable-model-invocation skill available for manual use', () => { const status = mapSkillConfigToStatus( makeSkill({ name: 'internal', disableModelInvocation: true }), ); - expect(status.status).toBe('disabled'); + expect(status.status).toBe('ok'); expect(status.modelInvocable).toBe(false); expect(status.name).toBe('internal'); }); + it('marks a settings-disabled skill as disabled', () => { + const status = mapSkillConfigToStatus( + makeSkill({ name: 'internal' }), + new Set(['internal']), + ); + + expect(status.status).toBe('disabled'); + expect(status.modelInvocable).toBe(true); + expect(status.name).toBe('internal'); + }); + it('surfaces optional model and extensionName only when present', () => { expect(mapSkillConfigToStatus(makeSkill())).not.toHaveProperty('model'); expect(mapSkillConfigToStatus(makeSkill())).not.toHaveProperty( diff --git a/packages/cli/src/serve/workspace-skills-mapping.ts b/packages/cli/src/serve/workspace-skills-mapping.ts index 97e13080e3..2a7355bffa 100644 --- a/packages/cli/src/serve/workspace-skills-mapping.ts +++ b/packages/cli/src/serve/workspace-skills-mapping.ts @@ -16,11 +16,13 @@ import type { ServeWorkspaceSkillStatus } from '@qwen-code/acp-bridge/status'; */ export function mapSkillConfigToStatus( skill: SkillConfig, + disabledSkillNames: ReadonlySet = new Set(), ): ServeWorkspaceSkillStatus { + const userDisabled = disabledSkillNames.has(skill.name.toLowerCase()); const modelInvocable = skill.disableModelInvocation !== true; return { kind: 'skill', - status: modelInvocable ? 'ok' : 'disabled', + status: userDisabled ? 'disabled' : 'ok', name: skill.name, description: skill.description, level: skill.level, diff --git a/packages/cli/src/serve/workspace-skills-status.test.ts b/packages/cli/src/serve/workspace-skills-status.test.ts index 7ee0ecab81..fb2f16ff77 100644 --- a/packages/cli/src/serve/workspace-skills-status.test.ts +++ b/packages/cli/src/serve/workspace-skills-status.test.ts @@ -5,6 +5,9 @@ */ import { describe, it, expect, vi, afterEach } from 'vitest'; +import * as fsp from 'node:fs/promises'; +import * as os from 'node:os'; +import * as path from 'node:path'; const mockWriteStderrLine = vi.hoisted(() => vi.fn()); vi.mock('../utils/stdioHelpers.js', () => ({ @@ -63,6 +66,47 @@ describe('createWorkspaceSkillsStatusProvider', () => { expect(mockWriteStderrLine.mock.calls[0][0]).toContain('boom'); }); + it('marks skills disabled in workspace settings', 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-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', + }, + { + name: 'disabled', + status: 'disabled', + }, + ]); + }); + it('reuses one SkillManager per workspace across calls', async () => { const listSpy = vi.spyOn(SkillManager.prototype, 'listSkills'); const provider = createWorkspaceSkillsStatusProvider(); diff --git a/packages/cli/src/serve/workspace-skills-status.ts b/packages/cli/src/serve/workspace-skills-status.ts index 8d0b333e73..7a417b01d7 100644 --- a/packages/cli/src/serve/workspace-skills-status.ts +++ b/packages/cli/src/serve/workspace-skills-status.ts @@ -34,6 +34,7 @@ import { SkillManager, isSafeModeEnv } from '@qwen-code/qwen-code-core'; import type { Config } from '@qwen-code/qwen-code-core'; import type { ServeWorkspaceSkillsStatus } from '@qwen-code/acp-bridge/status'; 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'; @@ -89,12 +90,13 @@ async function buildWorkspaceSkillsStatus( skillManager = new SkillManager(shim as Config); managers.set(workspaceCwd, skillManager); } + const disabled = readDisabledSkillNames(workspaceCwd); const skills = await skillManager.listSkills(); return { v: STATUS_SCHEMA_VERSION, workspaceCwd, initialized: true, - skills: skills.map(mapSkillConfigToStatus), + skills: skills.map((skill) => mapSkillConfigToStatus(skill, disabled)), }; } catch (error) { const message = error instanceof Error ? error.message : String(error); @@ -116,3 +118,14 @@ async function buildWorkspaceSkillsStatus( }; } } + +function readDisabledSkillNames(workspaceCwd: string): ReadonlySet { + const raw = loadSettings(workspaceCwd, false).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/webui/src/daemon/session/mappers.test.ts b/packages/webui/src/daemon/session/mappers.test.ts index 332e19127c..ef76b292be 100644 --- a/packages/webui/src/daemon/session/mappers.test.ts +++ b/packages/webui/src/daemon/session/mappers.test.ts @@ -176,6 +176,14 @@ describe('mapWorkspaceSkills', () => { name: 'deep-research', description: '', level: 'bundled', + modelInvocable: false, + }, + { + kind: 'skill', + status: 'disabled', + name: 'disabled-skill', + description: 'Disabled in settings', + level: 'project', modelInvocable: true, }, ], diff --git a/packages/webui/src/daemon/session/mappers.ts b/packages/webui/src/daemon/session/mappers.ts index 042198ebb2..3d9a4df44e 100644 --- a/packages/webui/src/daemon/session/mappers.ts +++ b/packages/webui/src/daemon/session/mappers.ts @@ -181,7 +181,11 @@ export function mapWorkspaceSkills( } { if (!status) return { commands: [], skills: [] }; - const commands = status.skills.map((skill) => ({ + const availableSkills = status.skills.filter( + (skill) => skill.status === 'ok', + ); + + const commands = availableSkills.map((skill) => ({ name: skill.name, description: skill.description || '', ...(skill.argumentHint ? { argumentHint: skill.argumentHint } : {}), @@ -195,7 +199,7 @@ export function mapWorkspaceSkills( return { commands, - skills: status.skills.map((skill) => skill.name), + skills: availableSkills.map((skill) => skill.name), }; }