fix(skill): enforce invocation policy before load
This commit is contained in:
@@ -93,7 +93,15 @@ export function apply(ctx: Context, config: Config = {}): void {
|
|||||||
if (!isSkillName(args.name)) {
|
if (!isSkillName(args.name)) {
|
||||||
throw new Error(`invalid skill name "${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) {
|
if (!skill) {
|
||||||
throw new Error(`skill "${args.name}" is unknown or no longer available`)
|
throw new Error(`skill "${args.name}" is unknown or no longer available`)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -385,4 +385,56 @@ describe('dsh-tool-skill', () => {
|
|||||||
if (unknownBlock?.type !== 'text') throw new Error('expected text tool result')
|
if (unknownBlock?.type !== 'text') throw new Error('expected text tool result')
|
||||||
expect(unknownBlock.text).toContain('skill "missing" is unknown or no longer available')
|
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.')
|
||||||
|
}
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -1264,23 +1264,40 @@ export function createTuiChat(
|
|||||||
appendNotice('Skills are not available in this session.', 'warning')
|
appendNotice('Skills are not available in this session.', 'warning')
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
skills.get(name, { cwd, signal: skillAbort.signal }).then(
|
const lookup = { cwd, signal: skillAbort.signal }
|
||||||
(skill) => {
|
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 (disposed) return
|
||||||
if (skill === undefined) {
|
const summary = summaries.find(skill => skill.name === name)
|
||||||
|
if (summary === undefined) {
|
||||||
appendNotice(`Unknown skill: ${name}`, 'warning')
|
appendNotice(`Unknown skill: ${name}`, 'warning')
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
if (!isSkillUserInvocable(skill)) {
|
if (!isSkillUserInvocable(summary)) {
|
||||||
appendNotice(`Skill "${name}" is not available for user invocation.`, 'warning')
|
appendNotice(`Skill "${name}" is not available for user invocation.`, 'warning')
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
deliver(renderSkillInvocation(skill, instructions))
|
skills.get(name, lookup).then(
|
||||||
},
|
(skill) => {
|
||||||
(error: unknown) => {
|
if (disposed) return
|
||||||
if (disposed) return
|
if (skill === undefined) {
|
||||||
appendNotice(`Skill "${name}" failed to load: ${errorChain(error)}`, 'error')
|
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,
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -3890,17 +3890,51 @@ describe('skill slash command', () => {
|
|||||||
await dispose(result)
|
await dispose(result)
|
||||||
})
|
})
|
||||||
|
|
||||||
it('rejects exact invocation of skills disabled for users', async () => {
|
it('checks user policy before loading and rechecks the loaded definition', async () => {
|
||||||
const result = await setup({ configureContext: withSkills })
|
const get = vi.fn((name: string) => Promise.resolve<SkillDefinition | undefined>({
|
||||||
|
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<SkillSummary[]>([
|
||||||
|
{
|
||||||
|
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('/skill:model-only-skill')
|
||||||
result.terminal.send('\r')
|
result.terminal.send('\r')
|
||||||
await tick()
|
await tick()
|
||||||
result.terminal.send('/skill:trusted-only-skill')
|
result.terminal.send('/skill:policy-race-skill')
|
||||||
result.terminal.send('\r')
|
result.terminal.send('\r')
|
||||||
await tick()
|
await tick()
|
||||||
expect(result.agent.sent).toEqual([])
|
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 "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)
|
await dispose(result)
|
||||||
})
|
})
|
||||||
|
|
||||||
@@ -3964,22 +3998,31 @@ describe('skill slash command', () => {
|
|||||||
result.terminal.send('\r')
|
result.terminal.send('\r')
|
||||||
await tick()
|
await tick()
|
||||||
expect(result.terminal.output).toContain('failed to load')
|
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)
|
await dispose(result)
|
||||||
})
|
})
|
||||||
|
|
||||||
it('drops skill list and lookup results that settle after disposal', async () => {
|
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 pendingGet: Array<{ resolve: (value: SkillDefinition | undefined) => void; reject: (error: unknown) => void }> = []
|
||||||
const result = await setup({
|
const result = await setup({
|
||||||
configureContext: async (ctx) => {
|
configureContext: async (ctx) => {
|
||||||
ctx.provide('tools', { get() { return undefined } } as never)
|
ctx.provide('tools', { get() { return undefined } } as never)
|
||||||
ctx.provide('skills', {
|
ctx.provide('skills', {
|
||||||
list: () => new Promise<SkillSummary[]>((resolve) => { pendingList.push(resolve) }),
|
list: () => {
|
||||||
|
listCalls += 1
|
||||||
|
if (listCalls === 1) return Promise.resolve<SkillSummary[]>([])
|
||||||
|
if (listCalls === 2) {
|
||||||
|
return Promise.resolve<SkillSummary[]>([{ name: 'demo-skill', description: 'demo', source: 'runtime', provider: 'runtime' }])
|
||||||
|
}
|
||||||
|
return new Promise<SkillSummary[]>((resolve) => { resolvePendingList = resolve })
|
||||||
|
},
|
||||||
get: () => new Promise<SkillDefinition | undefined>((resolve, reject) => { pendingGet.push({ resolve, reject }) }),
|
get: () => new Promise<SkillDefinition | undefined>((resolve, reject) => { pendingGet.push({ resolve, reject }) }),
|
||||||
} as never)
|
} as never)
|
||||||
},
|
},
|
||||||
})
|
})
|
||||||
|
await tick()
|
||||||
result.terminal.send('/skill:demo-skill')
|
result.terminal.send('/skill:demo-skill')
|
||||||
result.terminal.send('\r')
|
result.terminal.send('\r')
|
||||||
await tick()
|
await tick()
|
||||||
@@ -3988,12 +4031,10 @@ describe('skill slash command', () => {
|
|||||||
await tick()
|
await tick()
|
||||||
await dispose(result)
|
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[0]?.resolve({ name: 'demo-skill', description: 'late', source: 'runtime', provider: 'runtime', content: 'late body' })
|
||||||
pendingGet[1]?.reject(new Error('late failure'))
|
|
||||||
await tick()
|
await tick()
|
||||||
expect(result.agent.sent).toEqual([])
|
expect(result.agent.sent).toEqual([])
|
||||||
expect(result.terminal.output).not.toContain('late failure')
|
|
||||||
expect(result.terminal.output).not.toContain('late body')
|
expect(result.terminal.output).not.toContain('late body')
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
Reference in New Issue
Block a user