diff --git a/packages/skill/tool-skill/src/index.ts b/packages/skill/tool-skill/src/index.ts index ca9d6fea05..9c19aac4de 100644 --- a/packages/skill/tool-skill/src/index.ts +++ b/packages/skill/tool-skill/src/index.ts @@ -93,7 +93,15 @@ export function apply(ctx: Context, config: Config = {}): void { if (!isSkillName(args.name)) { throw new Error(`invalid skill name "${args.name}"`) } - const skill = await ctx.skills.get(args.name, { cwd: exec.agent?.session.header.cwd, signal: exec.signal }) + const lookup = { cwd: exec.agent?.session.header.cwd, signal: exec.signal } + const summary = (await ctx.skills.list(lookup)).find(skill => skill.name === args.name) + if (!summary) { + throw new Error(`skill "${args.name}" is unknown or no longer available`) + } + if (!isModelInvocable(summary)) { + throw new Error(`skill "${args.name}" is not available for model invocation`) + } + const skill = await ctx.skills.get(args.name, lookup) if (!skill) { throw new Error(`skill "${args.name}" is unknown or no longer available`) } diff --git a/packages/skill/tool-skill/tests/tool-skill.spec.ts b/packages/skill/tool-skill/tests/tool-skill.spec.ts index dabd1bfee7..d31b666ee7 100644 --- a/packages/skill/tool-skill/tests/tool-skill.spec.ts +++ b/packages/skill/tool-skill/tests/tool-skill.spec.ts @@ -385,4 +385,56 @@ describe('dsh-tool-skill', () => { if (unknownBlock?.type !== 'text') throw new Error('expected text tool result') expect(unknownBlock.text).toContain('skill "missing" is unknown or no longer available') }) + + it('checks model policy before provider loading and rechecks the loaded definition', async () => { + const home = await tempDir('tool-policy-before-load') + const ctx = await setup(home) + const getCalls: string[] = [] + ctx.skills.registerProvider({ + name: 'policy-probe', + async list() { + return [ + { + name: 'denied-skill', + description: 'Denied skill', + invocation: { modelInvocable: false, userInvocable: true }, + provider: 'policy-probe', + source: 'test', + rank: 1, + locator: 'denied-skill', + }, + { + name: 'policy-race-skill', + description: 'Policy race skill', + invocation: { modelInvocable: true, userInvocable: true }, + provider: 'policy-probe', + source: 'test', + rank: 1, + locator: 'policy-race-skill', + }, + ] + }, + async get(candidate) { + getCalls.push(candidate.name) + return { + ...candidate, + invocation: { modelInvocable: false, userInvocable: true }, + content: 'Instructions must not be disclosed.', + } + }, + }) + + const denied = await ctx.tools.execute({ signal: testToolSignal, callId: CallId('c6'), name: 'skill', arguments: { name: 'denied-skill' } }) + const raced = await ctx.tools.execute({ signal: testToolSignal, callId: CallId('c7'), name: 'skill', arguments: { name: 'policy-race-skill' } }) + + expect(denied.isError).toBe(true) + expect(raced.isError).toBe(true) + expect(getCalls).toEqual(['policy-race-skill']) + for (const result of [denied, raced]) { + const block = result.content[0] + if (block?.type !== 'text') throw new Error('expected text tool result') + expect(block.text).toContain('is not available for model invocation') + expect(block.text).not.toContain('Instructions must not be disclosed.') + } + }) }) diff --git a/packages/ui/tui/src/index.ts b/packages/ui/tui/src/index.ts index 134581764d..3027ace30c 100644 --- a/packages/ui/tui/src/index.ts +++ b/packages/ui/tui/src/index.ts @@ -1264,23 +1264,40 @@ export function createTuiChat( appendNotice('Skills are not available in this session.', 'warning') return } - skills.get(name, { cwd, signal: skillAbort.signal }).then( - (skill) => { + const lookup = { cwd, signal: skillAbort.signal } + const reportFailure = (error: unknown): void => { + if (disposed) return + appendNotice(`Skill "${name}" failed to load: ${errorChain(error)}`, 'error') + } + skills.list(lookup).then( + (summaries) => { if (disposed) return - if (skill === undefined) { + const summary = summaries.find(skill => skill.name === name) + if (summary === undefined) { appendNotice(`Unknown skill: ${name}`, 'warning') return } - if (!isSkillUserInvocable(skill)) { + if (!isSkillUserInvocable(summary)) { appendNotice(`Skill "${name}" is not available for user invocation.`, 'warning') return } - deliver(renderSkillInvocation(skill, instructions)) - }, - (error: unknown) => { - if (disposed) return - appendNotice(`Skill "${name}" failed to load: ${errorChain(error)}`, 'error') + skills.get(name, lookup).then( + (skill) => { + if (disposed) return + if (skill === undefined) { + appendNotice(`Unknown skill: ${name}`, 'warning') + return + } + if (!isSkillUserInvocable(skill)) { + appendNotice(`Skill "${name}" is not available for user invocation.`, 'warning') + return + } + deliver(renderSkillInvocation(skill, instructions)) + }, + reportFailure, + ) }, + reportFailure, ) } diff --git a/packages/ui/tui/tests/tui.spec.ts b/packages/ui/tui/tests/tui.spec.ts index f9871a5981..8777922025 100644 --- a/packages/ui/tui/tests/tui.spec.ts +++ b/packages/ui/tui/tests/tui.spec.ts @@ -3890,17 +3890,51 @@ describe('skill slash command', () => { await dispose(result) }) - it('rejects exact invocation of skills disabled for users', async () => { - const result = await setup({ configureContext: withSkills }) + it('checks user policy before loading and rechecks the loaded definition', async () => { + const get = vi.fn((name: string) => Promise.resolve({ + name, + description: 'Policy race skill', + invocation: { modelInvocable: true, userInvocable: false }, + source: 'runtime', + provider: 'runtime', + content: 'Instructions must not be delivered.', + })) + const result = await setup({ + configureContext: async (ctx) => { + ctx.provide('tools', { get() { return undefined } } as never) + ctx.provide('skills', { + list: () => Promise.resolve([ + { + name: 'model-only-skill', + description: 'Model-only skill', + invocation: { modelInvocable: true, userInvocable: false }, + source: 'runtime', + provider: 'runtime', + }, + { + name: 'policy-race-skill', + description: 'Policy race skill', + invocation: { modelInvocable: true, userInvocable: true }, + source: 'runtime', + provider: 'runtime', + }, + ]), + get, + } as never) + }, + }) result.terminal.send('/skill:model-only-skill') result.terminal.send('\r') await tick() - result.terminal.send('/skill:trusted-only-skill') + result.terminal.send('/skill:policy-race-skill') result.terminal.send('\r') await tick() expect(result.agent.sent).toEqual([]) + expect(get).toHaveBeenCalledTimes(1) + expect(get).toHaveBeenCalledWith('policy-race-skill', expect.objectContaining({ cwd: '/workspace' })) expect(result.terminal.output).toContain('Skill "model-only-skill" is not available for user invocation.') - expect(result.terminal.output).toContain('Skill "trusted-only-skill" is not available for user invocation.') + expect(result.terminal.output).toContain('Skill "policy-race-skill" is not available for user invocation.') + expect(result.terminal.output).not.toContain('Instructions must not be delivered.') await dispose(result) }) @@ -3964,22 +3998,31 @@ describe('skill slash command', () => { result.terminal.send('\r') await tick() expect(result.terminal.output).toContain('failed to load') - expect(result.terminal.output).toContain('get boom') + expect(result.terminal.output).toContain('list boom') await dispose(result) }) it('drops skill list and lookup results that settle after disposal', async () => { - const pendingList: Array<(value: SkillSummary[]) => void> = [] + let listCalls = 0 + let resolvePendingList: ((value: SkillSummary[]) => void) | undefined const pendingGet: Array<{ resolve: (value: SkillDefinition | undefined) => void; reject: (error: unknown) => void }> = [] const result = await setup({ configureContext: async (ctx) => { ctx.provide('tools', { get() { return undefined } } as never) ctx.provide('skills', { - list: () => new Promise((resolve) => { pendingList.push(resolve) }), + list: () => { + listCalls += 1 + if (listCalls === 1) return Promise.resolve([]) + if (listCalls === 2) { + return Promise.resolve([{ name: 'demo-skill', description: 'demo', source: 'runtime', provider: 'runtime' }]) + } + return new Promise((resolve) => { resolvePendingList = resolve }) + }, get: () => new Promise((resolve, reject) => { pendingGet.push({ resolve, reject }) }), } as never) }, }) + await tick() result.terminal.send('/skill:demo-skill') result.terminal.send('\r') await tick() @@ -3988,12 +4031,10 @@ describe('skill slash command', () => { await tick() await dispose(result) - for (const resolve of pendingList) resolve([{ name: 'late', description: 'late', source: 'runtime', provider: 'runtime' }]) + resolvePendingList?.([{ name: 'other-skill', description: 'late', source: 'runtime', provider: 'runtime' }]) pendingGet[0]?.resolve({ name: 'demo-skill', description: 'late', source: 'runtime', provider: 'runtime', content: 'late body' }) - pendingGet[1]?.reject(new Error('late failure')) await tick() expect(result.agent.sent).toEqual([]) - expect(result.terminal.output).not.toContain('late failure') expect(result.terminal.output).not.toContain('late body') }) })