diff --git a/packages/cli/src/services/BundledSkillLoader.test.ts b/packages/cli/src/services/BundledSkillLoader.test.ts index 9c4e205d5e..2323847fdd 100644 --- a/packages/cli/src/services/BundledSkillLoader.test.ts +++ b/packages/cli/src/services/BundledSkillLoader.test.ts @@ -33,16 +33,21 @@ describe('BundledSkillLoader', () => { let mockSkillManager: { listSkills: ReturnType; }; + let mockAddSessionAllowRule: ReturnType; beforeEach(() => { vi.clearAllMocks(); mockSkillManager = { listSkills: vi.fn().mockResolvedValue([]), }; + mockAddSessionAllowRule = vi.fn(); mockConfig = { getSkillManager: vi.fn().mockReturnValue(mockSkillManager), isCronEnabled: vi.fn().mockReturnValue(false), getModel: vi.fn().mockReturnValue(undefined), + getPermissionManager: vi + .fn() + .mockReturnValue({ addSessionAllowRule: mockAddSessionAllowRule }), } as unknown as Config; }); @@ -155,6 +160,38 @@ describe('BundledSkillLoader', () => { }); }); + describe('allowedTools grant', () => { + it('grants allowedTools as session allow rules when the command runs', async () => { + const skill = makeSkill({ allowedTools: ['Bash(git *)', 'Edit'] }); + mockSkillManager.listSkills.mockResolvedValue([skill]); + + const loader = new BundledSkillLoader(mockConfig); + const commands = await loader.loadCommands(signal); + await commands[0].action!( + { invocation: { raw: '/review', args: '' } } as never, + '', + ); + + expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2); + expect(mockAddSessionAllowRule).toHaveBeenNthCalledWith(1, 'Bash(git *)'); + expect(mockAddSessionAllowRule).toHaveBeenNthCalledWith(2, 'Edit'); + }); + + it('does not grant when the bundled skill declares no allowedTools', async () => { + const skill = makeSkill(); // no allowedTools + mockSkillManager.listSkills.mockResolvedValue([skill]); + + const loader = new BundledSkillLoader(mockConfig); + const commands = await loader.loadCommands(signal); + await commands[0].action!( + { invocation: { raw: '/review', args: '' } } as never, + '', + ); + + expect(mockAddSessionAllowRule).not.toHaveBeenCalled(); + }); + }); + it('should return empty array when listSkills throws', async () => { mockSkillManager.listSkills.mockRejectedValue(new Error('load failed')); diff --git a/packages/cli/src/services/BundledSkillLoader.ts b/packages/cli/src/services/BundledSkillLoader.ts index 47d927507b..2bd58a00a2 100644 --- a/packages/cli/src/services/BundledSkillLoader.ts +++ b/packages/cli/src/services/BundledSkillLoader.ts @@ -9,6 +9,7 @@ import { createDebugLogger, appendToLastTextPart, buildSkillLlmContent, + applySkillAllowedTools, } from '@qwen-code/qwen-code-core'; import { dirname } from 'node:path'; import type { ICommandLoader } from './types.js'; @@ -73,6 +74,12 @@ export class BundledSkillLoader implements ICommandLoader { argumentHint: skill.argumentHint, whenToUse: skill.whenToUse, action: async (context, _args): Promise => { + // Auto-approve the skill's declared allowedTools before its body is submitted. + applySkillAllowedTools( + this.config?.getPermissionManager(), + skill.allowedTools, + ); + // Resolve template variables in skill body let body = skill.body; const modelId = this.config?.getModel()?.trim() || ''; diff --git a/packages/cli/src/services/SkillCommandLoader.test.ts b/packages/cli/src/services/SkillCommandLoader.test.ts index 11871a6371..6c2a922c48 100644 --- a/packages/cli/src/services/SkillCommandLoader.test.ts +++ b/packages/cli/src/services/SkillCommandLoader.test.ts @@ -6,7 +6,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { SkillCommandLoader } from './SkillCommandLoader.js'; -import { CommandKind } from '../ui/commands/types.js'; +import { CommandKind, type CommandContext } from '../ui/commands/types.js'; import { buildSkillLlmContent, type Config, @@ -31,15 +31,20 @@ function makeSkillPrompt(body: string): string { describe('SkillCommandLoader', () => { let mockConfig: Config; let mockSkillManager: { listSkills: ReturnType }; + let mockAddSessionAllowRule: ReturnType; beforeEach(() => { vi.clearAllMocks(); mockSkillManager = { listSkills: vi.fn().mockResolvedValue([]), }; + mockAddSessionAllowRule = vi.fn(); mockConfig = { getSkillManager: vi.fn().mockReturnValue(mockSkillManager), getBareMode: vi.fn().mockReturnValue(false), + getPermissionManager: vi + .fn() + .mockReturnValue({ addSessionAllowRule: mockAddSessionAllowRule }), } as unknown as Config; }); @@ -331,4 +336,39 @@ describe('SkillCommandLoader', () => { 'ext-skill', ]); }); + + describe('allowedTools grant', () => { + it('grants allowedTools as session allow rules when the command runs', async () => { + const skill = makeSkill({ + level: 'user', + allowedTools: ['Bash(git *)', 'Edit'], + }); + mockSkillManager.listSkills.mockImplementation( + ({ level }: { level: string }) => + Promise.resolve(level === 'user' ? [skill] : []), + ); + + const loader = new SkillCommandLoader(mockConfig); + const commands = await loader.loadCommands(signal); + await commands[0].action?.({} as CommandContext, ''); + + expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2); + expect(mockAddSessionAllowRule).toHaveBeenNthCalledWith(1, 'Bash(git *)'); + expect(mockAddSessionAllowRule).toHaveBeenNthCalledWith(2, 'Edit'); + }); + + it('does not grant when the skill declares no allowedTools', async () => { + const skill = makeSkill({ level: 'user' }); // no allowedTools + mockSkillManager.listSkills.mockImplementation( + ({ level }: { level: string }) => + Promise.resolve(level === 'user' ? [skill] : []), + ); + + const loader = new SkillCommandLoader(mockConfig); + const commands = await loader.loadCommands(signal); + await commands[0].action?.({} as CommandContext, ''); + + expect(mockAddSessionAllowRule).not.toHaveBeenCalled(); + }); + }); }); diff --git a/packages/cli/src/services/SkillCommandLoader.ts b/packages/cli/src/services/SkillCommandLoader.ts index 9cf5ee168d..fdf4563b24 100644 --- a/packages/cli/src/services/SkillCommandLoader.ts +++ b/packages/cli/src/services/SkillCommandLoader.ts @@ -9,6 +9,7 @@ import { createDebugLogger, appendToLastTextPart, buildSkillLlmContent, + applySkillAllowedTools, } from '@qwen-code/qwen-code-core'; import { dirname } from 'node:path'; import type { ICommandLoader } from './types.js'; @@ -96,6 +97,12 @@ export class SkillCommandLoader implements ICommandLoader { argumentHint: skill.argumentHint, whenToUse: skill.whenToUse, action: async (context, _args): Promise => { + // Auto-approve the skill's declared allowedTools before its body is submitted. + applySkillAllowedTools( + this.config?.getPermissionManager(), + skill.allowedTools, + ); + const body = buildSkillLlmContent( dirname(skill.filePath), skill.body, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 08dffbc78e..87ddd548cc 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -88,7 +88,10 @@ export * from './tools/sdk-control-client-transport.js'; export * from './tools/modifiable-tool.js'; // Selective re-exports of types/utilities from tool files (avoids loading full tool modules) -export { buildSkillLlmContent } from './tools/skill-utils.js'; +export { + buildSkillLlmContent, + applySkillAllowedTools, +} from './tools/skill-utils.js'; // Backward-compatible type re-exports for tool classes removed from eager loading. // These preserve TypeScript type compatibility for downstream consumers. diff --git a/packages/core/src/skills/types.ts b/packages/core/src/skills/types.ts index cd26954a6d..da2356f566 100644 --- a/packages/core/src/skills/types.ts +++ b/packages/core/src/skills/types.ts @@ -36,8 +36,16 @@ export interface SkillConfig { description: string; /** - * Optional list of tool names that this skill is allowed to use. - * For v1, this is informational only (no gating). + * Optional list of tools to auto-approve while this skill is active. + * + * Each entry is a permission rule string in the same syntax as + * `settings.json` `permissions.allow` — e.g. `Bash(git *)`, `Edit`, `Read`, + * `mcp__server__tool`. When the skill is invoked (via the model Skill-tool or + * a user `/` slash command), each entry is added as a session-scoped + * allow rule, so matching tool calls are auto-approved instead of prompting. + * + * This is an additive grant only: it never hides or restricts the tools the + * model can see. Malformed entries are ignored. See `applySkillAllowedTools`. */ allowedTools?: string[]; diff --git a/packages/core/src/tools/skill-utils.test.ts b/packages/core/src/tools/skill-utils.test.ts new file mode 100644 index 0000000000..5dcccb3b82 --- /dev/null +++ b/packages/core/src/tools/skill-utils.test.ts @@ -0,0 +1,63 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi } from 'vitest'; +import { applySkillAllowedTools } from './skill-utils.js'; +import type { PermissionManager } from '../permissions/permission-manager.js'; + +function mockPermissionManager(): { + pm: PermissionManager; + addSessionAllowRule: ReturnType; +} { + const addSessionAllowRule = vi.fn(); + return { + pm: { addSessionAllowRule } as unknown as PermissionManager, + addSessionAllowRule, + }; +} + +describe('applySkillAllowedTools', () => { + it('adds one session allow rule per entry, verbatim and in order', () => { + const { pm, addSessionAllowRule } = mockPermissionManager(); + + applySkillAllowedTools(pm, ['Bash(git *)', 'Edit', 'mcp__server__tool']); + + expect(addSessionAllowRule).toHaveBeenCalledTimes(3); + expect(addSessionAllowRule).toHaveBeenNthCalledWith(1, 'Bash(git *)'); + expect(addSessionAllowRule).toHaveBeenNthCalledWith(2, 'Edit'); + expect(addSessionAllowRule).toHaveBeenNthCalledWith(3, 'mcp__server__tool'); + }); + + it('no-ops when allowedTools is undefined', () => { + const { pm, addSessionAllowRule } = mockPermissionManager(); + applySkillAllowedTools(pm, undefined); + expect(addSessionAllowRule).not.toHaveBeenCalled(); + }); + + it('no-ops when allowedTools is empty', () => { + const { pm, addSessionAllowRule } = mockPermissionManager(); + applySkillAllowedTools(pm, []); + expect(addSessionAllowRule).not.toHaveBeenCalled(); + }); + + it('no-ops without throwing when there is no permission manager', () => { + expect(() => applySkillAllowedTools(null, ['Bash(git *)'])).not.toThrow(); + expect(() => + applySkillAllowedTools(undefined, ['Bash(git *)']), + ).not.toThrow(); + }); + + it('delegates malformed-entry handling to the permission manager (does not pre-filter)', () => { + // The permission manager is the single authority on rule validity; the + // helper forwards every entry and lets addSessionAllowRule log/skip bad + // ones. This keeps validation in one place. + const { pm, addSessionAllowRule } = mockPermissionManager(); + applySkillAllowedTools(pm, ['Bash(unbalanced', 'Read']); + expect(addSessionAllowRule).toHaveBeenCalledTimes(2); + expect(addSessionAllowRule).toHaveBeenNthCalledWith(1, 'Bash(unbalanced'); + expect(addSessionAllowRule).toHaveBeenNthCalledWith(2, 'Read'); + }); +}); diff --git a/packages/core/src/tools/skill-utils.ts b/packages/core/src/tools/skill-utils.ts index fe94d0fc27..27dd1800df 100644 --- a/packages/core/src/tools/skill-utils.ts +++ b/packages/core/src/tools/skill-utils.ts @@ -4,6 +4,8 @@ * SPDX-License-Identifier: Apache-2.0 */ +import type { PermissionManager } from '../permissions/permission-manager.js'; + /** * Builds the LLM-facing content string when a skill body is injected. * Shared between SkillToolInvocation (runtime) and /context (estimation) @@ -12,3 +14,26 @@ export function buildSkillLlmContent(baseDir: string, body: string): string { return `Base directory for this skill: ${baseDir}\nImportant: ALWAYS resolve absolute paths from this base directory when working with skills.\n\n${body}\n`; } + +/** + * Grants a skill's `allowedTools` as session-scoped permission allow rules. + * + * Each entry is a permission rule string in the same syntax as `settings.json` + * `permissions.allow` (e.g. `Bash(git *)`, `Edit`, `mcp__server__tool`) and is + * handed verbatim to the session allow list, so matching tool calls are + * auto-approved for the rest of the session instead of prompting. This is an + * additive grant only — it never hides or restricts the tools the model sees. + * + * No-ops when there is no permission manager or nothing to grant. + */ +export function applySkillAllowedTools( + permissionManager: PermissionManager | null | undefined, + allowedTools: string[] | undefined, +): void { + if (!permissionManager || !allowedTools?.length) { + return; + } + for (const rule of allowedTools) { + permissionManager.addSessionAllowRule(rule); + } +} diff --git a/packages/core/src/tools/skill.test.ts b/packages/core/src/tools/skill.test.ts index 389a66bca3..dc807039d7 100644 --- a/packages/core/src/tools/skill.test.ts +++ b/packages/core/src/tools/skill.test.ts @@ -49,6 +49,7 @@ describe('SkillTool', () => { let skillTool: SkillTool; let mockSkillManager: SkillManager; let changeListeners: Array<() => void>; + let mockAddSessionAllowRule: ReturnType; const mockSkills: SkillConfig[] = [ { @@ -72,6 +73,8 @@ describe('SkillTool', () => { // Setup fake timers vi.useFakeTimers(); + mockAddSessionAllowRule = vi.fn(); + // Create mock config config = { getProjectRoot: vi.fn().mockReturnValue('/test/project'), @@ -80,6 +83,9 @@ describe('SkillTool', () => { getGeminiClient: vi.fn().mockReturnValue(undefined), getModelInvocableCommandsProvider: vi.fn().mockReturnValue(null), getModelInvocableCommandsExecutor: vi.fn().mockReturnValue(null), + getPermissionManager: vi + .fn() + .mockReturnValue({ addSessionAllowRule: mockAddSessionAllowRule }), } as unknown as Config; changeListeners = []; @@ -521,6 +527,36 @@ describe('SkillTool', () => { expect(result.returnDisplay).toBe('Skill for writing and running tests'); }); + it('grants allowedTools as session allow rules on invocation', async () => { + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue({ + ...mockSkills[1], + allowedTools: ['Bash(git *)', 'Edit'], + }); + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'testing' }); + await invocation.execute(); + + expect(mockAddSessionAllowRule).toHaveBeenCalledTimes(2); + expect(mockAddSessionAllowRule).toHaveBeenNthCalledWith(1, 'Bash(git *)'); + expect(mockAddSessionAllowRule).toHaveBeenNthCalledWith(2, 'Edit'); + }); + + it('does not add allow rules when the skill declares no allowedTools', async () => { + // code-review (mockSkills[0]) has no allowedTools field. + vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue( + mockSkills[0], + ); + + const invocation = ( + skillTool as SkillToolWithProtectedMethods + ).createInvocation({ skill: 'code-review' }); + await invocation.execute(); + + expect(mockAddSessionAllowRule).not.toHaveBeenCalled(); + }); + it('should handle skill not found error', async () => { vi.mocked(mockSkillManager.loadSkillForRuntime).mockResolvedValue(null); diff --git a/packages/core/src/tools/skill.ts b/packages/core/src/tools/skill.ts index a873371292..a3dfcdd714 100644 --- a/packages/core/src/tools/skill.ts +++ b/packages/core/src/tools/skill.ts @@ -25,7 +25,7 @@ export interface SkillParams { // Re-export for backward compatibility export { buildSkillLlmContent } from './skill-utils.js'; -import { buildSkillLlmContent } from './skill-utils.js'; +import { buildSkillLlmContent, applySkillAllowedTools } from './skill-utils.js'; /** * Skill tool that enables the model to access skill definitions. @@ -437,6 +437,12 @@ class SkillToolInvocation extends BaseToolInvocation { ); this.onSkillLoaded(this.params.skill); + // Auto-approve the skill's declared allowedTools for the rest of the session. + applySkillAllowedTools( + this.config.getPermissionManager(), + skill.allowedTools, + ); + // Register skill hooks if present debugLogger.debug('Skill hooks check:', { hasHooks: !!skill.hooks,