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 <ytahdn@gmail.com>
2026-07-09 17:19:02 +00:00 · 2026-07-03 14:58:46 +08:00 · 2026-07-03 14:58:46 +08:00 · 8dfa7613be
commit 8dfa7613be
parent 0633b8a985
10 changed files with 231 additions and 36 deletions
--- 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');

--- 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 {
--- 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<string>()),
    } 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'),
--- 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;
    }
--- 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(
--- 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<string> = 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,
--- 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();
--- 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<string> {
+  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),
+  );
+}
--- 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,
        },
      ],
--- 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),
  };
 }