refactor(host)!: retire the skill.invoke RPC for the gesture boundary
Invocation is an ordinary session.prompt again: the pre-step gesture boundary makes it deterministic host-side for every front end, so the dedicated RPC (handler, wire schema, error codes, client face, fixtures) and ui-skill's claim machinery are net deletions. The menu keeps decision 21 exactly — a pick lands literal /name text — plus the user-only marker from skill.list's modelInvocable flag.
This commit is contained in:
Yichen Jiang
@@ -269,207 +269,6 @@ describe('skill.list', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('skill.invoke', () => {
|
||||
/** Provider with one user-only and one model-only skill, both loadable. */
|
||||
function registerInvokeSkills(ctx: Context): void {
|
||||
const summaries = [
|
||||
{
|
||||
name: 'user-only', description: 'User-only',
|
||||
invocation: { modelInvocable: false, userInvocable: true },
|
||||
source: 'custom', provider: 'probe', rank: 0, locator: null,
|
||||
resourceBase: { kind: 'directory', path: '/proj/.agents/skills/user-only' },
|
||||
},
|
||||
{
|
||||
name: 'model-only', description: 'Model-only',
|
||||
invocation: { modelInvocable: true, userInvocable: false },
|
||||
source: 'custom', provider: 'probe', rank: 0, locator: null,
|
||||
},
|
||||
] as const
|
||||
ctx.skills.registerProvider(() => ({
|
||||
name: 'probe',
|
||||
list: () => Promise.resolve(summaries.map(summary => ({ ...summary }))),
|
||||
get: candidate => Promise.resolve({
|
||||
...summaries.find(summary => summary.name === candidate.name)!,
|
||||
content: 'Follow the probe instructions.',
|
||||
}),
|
||||
}))
|
||||
}
|
||||
|
||||
/** Agent stub whose session carries a project cwd and whose followup records the injected message. */
|
||||
function invokableAgent(ctx: Context): { agent: Agent; followup: ReturnType<typeof vi.fn> } {
|
||||
const session = ctx.sessions.create(undefined, { meta: { cwd: '/proj' } })
|
||||
const inbox = new Inbox(session, { inserted: () => {}, discarded: () => {}, claimed: () => {} })
|
||||
const followup = vi.fn()
|
||||
const agent = { id: session.id, session, inbox, status: 'idle', ctx, followup } as unknown as Agent
|
||||
ctx.agents.register(agent)
|
||||
return { agent, followup }
|
||||
}
|
||||
|
||||
const live = () => new AbortController().signal
|
||||
|
||||
it('injects a user-invocable skill as a user message with the invocation source', async () => {
|
||||
const ctx = await harness()
|
||||
registerInvokeSkills(ctx)
|
||||
const api = createApiProxy(ctx, DEFAULTS)
|
||||
const { agent, followup } = invokableAgent(ctx)
|
||||
const value = expectOk(await api.skills.invoke(request({
|
||||
sessionId: agent.id, name: 'user-only', text: 'and check the fixture',
|
||||
}), live()))
|
||||
expect(value).toEqual({ accepted: true })
|
||||
expect(followup).toHaveBeenCalledTimes(1)
|
||||
const message = followup.mock.calls[0]?.[0] as UserMessage
|
||||
expect(message.source).toEqual({ kind: 'skill-invocation', name: 'user-only', args: 'and check the fixture' })
|
||||
expect(message.content).toHaveLength(1)
|
||||
const text = (message.content[0] as { text: string }).text
|
||||
expect(text).toContain('<skill_content name="user-only">')
|
||||
expect(text).toContain('Base directory for this skill: /proj/.agents/skills/user-only')
|
||||
expect(text).toContain('Follow the probe instructions.')
|
||||
expect(text.endsWith('\n\nand check the fixture')).toBe(true)
|
||||
})
|
||||
|
||||
it('omits args from the source and content when no text rides the invocation', async () => {
|
||||
const ctx = await harness()
|
||||
registerInvokeSkills(ctx)
|
||||
const api = createApiProxy(ctx, DEFAULTS)
|
||||
const { agent, followup } = invokableAgent(ctx)
|
||||
expectOk(await api.skills.invoke(request({ sessionId: agent.id, name: 'user-only' }), live()))
|
||||
const message = followup.mock.calls[0]?.[0] as UserMessage
|
||||
expect(message.source).toEqual({ kind: 'skill-invocation', name: 'user-only' })
|
||||
const text = (message.content[0] as { text: string }).text
|
||||
expect(text.endsWith('</skill_content>')).toBe(true)
|
||||
})
|
||||
|
||||
it('rejects a skill the user may not invoke', async () => {
|
||||
const ctx = await harness()
|
||||
registerInvokeSkills(ctx)
|
||||
const api = createApiProxy(ctx, DEFAULTS)
|
||||
const { agent, followup } = invokableAgent(ctx)
|
||||
const error = expectErr(await api.skills.invoke(request({ sessionId: agent.id, name: 'model-only' }), live()))
|
||||
expect(error.code).toBe('skill-not-invocable')
|
||||
expect(followup).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('rechecks user policy on the loaded definition (list/get race)', async () => {
|
||||
const ctx = await harness()
|
||||
// The provider flips the skill user-invocable in list but user-disabled
|
||||
// in get — the window a provider change between the two collects opens.
|
||||
ctx.skills.registerProvider(() => ({
|
||||
name: 'flipping',
|
||||
list: () => Promise.resolve([{
|
||||
name: 'flipper', description: 'Race probe',
|
||||
invocation: { modelInvocable: false, userInvocable: true },
|
||||
source: 'custom', provider: 'flipping', rank: 0, locator: null,
|
||||
}]),
|
||||
get: () => Promise.resolve({
|
||||
name: 'flipper', description: 'Race probe',
|
||||
invocation: { modelInvocable: false, userInvocable: false },
|
||||
source: 'custom', provider: 'flipping',
|
||||
content: 'Must never inject.',
|
||||
}),
|
||||
}))
|
||||
const api = createApiProxy(ctx, DEFAULTS)
|
||||
const { agent, followup } = invokableAgent(ctx)
|
||||
const error = expectErr(await api.skills.invoke(request({ sessionId: agent.id, name: 'flipper' }), live()))
|
||||
expect(error.code).toBe('skill-not-invocable')
|
||||
expect(followup).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('reports skill-not-found when the summary wins but the load returns nothing', async () => {
|
||||
const ctx = await harness()
|
||||
ctx.skills.registerProvider(() => ({
|
||||
name: 'vanishing',
|
||||
list: () => Promise.resolve([{
|
||||
name: 'ghost', description: 'Vanishes on load',
|
||||
invocation: { modelInvocable: false, userInvocable: true },
|
||||
source: 'custom', provider: 'vanishing', rank: 0, locator: null,
|
||||
}]),
|
||||
get: () => Promise.resolve(undefined),
|
||||
}))
|
||||
const api = createApiProxy(ctx, DEFAULTS)
|
||||
const { agent, followup } = invokableAgent(ctx)
|
||||
const error = expectErr(await api.skills.invoke(request({ sessionId: agent.id, name: 'ghost' }), live()))
|
||||
expect(error.code).toBe('skill-not-found')
|
||||
expect(followup).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('rejects an unknown or invalid skill name', async () => {
|
||||
const ctx = await harness()
|
||||
registerInvokeSkills(ctx)
|
||||
const api = createApiProxy(ctx, DEFAULTS)
|
||||
const { agent } = invokableAgent(ctx)
|
||||
const missing = expectErr(await api.skills.invoke(request({ sessionId: agent.id, name: 'absent-skill' }), live()))
|
||||
expect(missing.code).toBe('skill-not-found')
|
||||
const invalid = expectErr(await api.skills.invoke(request({ sessionId: agent.id, name: 'Not A Name' }), live()))
|
||||
expect(invalid.code).toBe('skill-not-found')
|
||||
})
|
||||
|
||||
it('folds a loader failure into a structured internal error', async () => {
|
||||
const ctx = await harness()
|
||||
ctx.skills.registerProvider(() => ({
|
||||
name: 'exploding',
|
||||
list: () => Promise.resolve([{
|
||||
name: 'grenade', description: 'Loader throws',
|
||||
invocation: { modelInvocable: false, userInvocable: true },
|
||||
source: 'custom', provider: 'exploding', rank: 0, locator: null,
|
||||
}]),
|
||||
get: () => Promise.reject(new Error('disk exploded')),
|
||||
}))
|
||||
const api = createApiProxy(ctx, DEFAULTS)
|
||||
const { agent, followup } = invokableAgent(ctx)
|
||||
const error = expectErr(await api.skills.invoke(request({ sessionId: agent.id, name: 'grenade' }), live()))
|
||||
expect(error.code).toBe('internal')
|
||||
expect(error.message).toContain('skill invocation failed')
|
||||
expect(followup).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('refuses to start a turn the caller already abandoned', async () => {
|
||||
const ctx = await harness()
|
||||
registerInvokeSkills(ctx)
|
||||
const api = createApiProxy(ctx, DEFAULTS)
|
||||
const { agent, followup } = invokableAgent(ctx)
|
||||
const abort = new AbortController()
|
||||
abort.abort()
|
||||
const error = expectErr(await api.skills.invoke(request({ sessionId: agent.id, name: 'user-only' }), abort.signal))
|
||||
expect(error.code).toBe('cancelled')
|
||||
expect(followup).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('surfaces a followup refusal as agent-busy', async () => {
|
||||
const ctx = await harness()
|
||||
registerInvokeSkills(ctx)
|
||||
const api = createApiProxy(ctx, DEFAULTS)
|
||||
const { agent, followup } = invokableAgent(ctx)
|
||||
followup.mockImplementation(() => { throw new Error('inbox closed') })
|
||||
const error = expectErr(await api.skills.invoke(request({ sessionId: agent.id, name: 'user-only' }), live()))
|
||||
expect(error.code).toBe('agent-busy')
|
||||
})
|
||||
|
||||
it('refuses a cwd-less session with the skill.list stance', async () => {
|
||||
const ctx = await harness()
|
||||
registerInvokeSkills(ctx)
|
||||
const api = createApiProxy(ctx, DEFAULTS)
|
||||
const session = ctx.sessions.create(undefined)
|
||||
const inbox = new Inbox(session, { inserted: () => {}, discarded: () => {}, claimed: () => {} })
|
||||
const followup = vi.fn()
|
||||
ctx.agents.register({ id: session.id, session, inbox, status: 'idle', ctx, followup } as unknown as Agent)
|
||||
const error = expectErr(await api.skills.invoke(request({ sessionId: session.id, name: 'user-only' }), live()))
|
||||
expect(error.code).toBe('internal')
|
||||
expect(error.message).toContain('has no project cwd')
|
||||
expect(followup).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('fails loud with internal when the skill registry is not mounted', async () => {
|
||||
const ctx = await harness({ skills: false })
|
||||
const api = createApiProxy(ctx, DEFAULTS)
|
||||
const session = ctx.sessions.create(undefined, { meta: { cwd: '/proj' } })
|
||||
const inbox = new Inbox(session, { inserted: () => {}, discarded: () => {}, claimed: () => {} })
|
||||
ctx.agents.register({ id: session.id, session, inbox, status: 'idle', ctx, followup: vi.fn() } as unknown as Agent)
|
||||
const error = expectErr(await api.skills.invoke(request({ sessionId: session.id, name: 'user-only' }), live()))
|
||||
expect(error.code).toBe('internal')
|
||||
expect(error.message).toContain('skill registry is absent')
|
||||
})
|
||||
})
|
||||
|
||||
describe('host/commands-changed frame', () => {
|
||||
it('broadcasts on registry change', async () => {
|
||||
const ctx = await harness()
|
||||
|
||||
@@ -86,7 +86,7 @@ function scriptedApi(overrides: {
|
||||
execute: r => ok(r, { matched: false }),
|
||||
...overrides.commands,
|
||||
},
|
||||
skills: { list: r => ok(r, { skills: [] }), invoke: r => ok(r, { accepted: true as const }), ...overrides.skills },
|
||||
skills: { list: r => ok(r, { skills: [] }), ...overrides.skills },
|
||||
goals: {
|
||||
create: err,
|
||||
edit: err,
|
||||
|
||||
@@ -198,9 +198,6 @@ function fakeApi(overrides: Partial<{ muxFrames: MuxFrame[]; hostFrames: HostFra
|
||||
async list(request) {
|
||||
return { rpcId: request.rpcId, result: { ok: true, value: { skills: [{ name: 'commit-helper', description: 'Git commits', modelInvocable: true }] } } }
|
||||
},
|
||||
async invoke(request) {
|
||||
return { rpcId: request.rpcId, result: { ok: true, value: { accepted: true as const } } }
|
||||
},
|
||||
},
|
||||
goals: {
|
||||
async create(request) {
|
||||
@@ -385,8 +382,6 @@ describe('unary round trip (handler ⇄ client, no network)', () => {
|
||||
expect(miss.result).toEqual({ ok: true, value: { matched: false } })
|
||||
const skills = await c.skills.list({ sessionId: 's' as never })
|
||||
expect(skills.result).toEqual({ ok: true, value: { skills: [{ name: 'commit-helper', description: 'Git commits', modelInvocable: true }] } })
|
||||
const invoked = await c.skills.invoke({ sessionId: 's' as never, name: 'commit-helper', text: 'go' })
|
||||
expect(invoked.result).toEqual({ ok: true, value: { accepted: true } })
|
||||
})
|
||||
|
||||
it('lets command.execute finish after the 30-second default unary deadline', async () => {
|
||||
|
||||
@@ -31,7 +31,7 @@ import {
|
||||
commandDescriptorSchema, commandExecuteRequestSchema, commandExecuteValueSchema,
|
||||
commandListRequestSchema, commandListValueSchema,
|
||||
} from '../src/api/commands.schema.ts'
|
||||
import { skillEntrySchema, skillInvokeRequestSchema, skillInvokeValueSchema, skillListRequestSchema, skillListValueSchema } from '../src/api/skills.schema.ts'
|
||||
import { skillEntrySchema, skillListRequestSchema, skillListValueSchema } from '../src/api/skills.schema.ts'
|
||||
import { hostFrameSchema, muxFrameSchema, askUserQuestionItemSchema } from '../src/api/events.schema.ts'
|
||||
import { approvalRequestIdSchema, approvalResponsePayloadSchema } from '../src/api/approvals.schema.ts'
|
||||
import { askUserQuestionAnswerSchema, questionResponsePayloadSchema } from '../src/api/questions.schema.ts'
|
||||
@@ -74,8 +74,6 @@ describe('rpcErrorSchema', () => {
|
||||
expect(rpcErrorSchema.parse({ code: 'queue-item-not-found', message: 'm', details: { itemId: 'i' } }).code).toBe('queue-item-not-found')
|
||||
expect(rpcErrorSchema.parse({ code: 'command-error', message: 'm', details: {} }).code).toBe('command-error')
|
||||
expect(rpcErrorSchema.parse({ code: 'unknown-command', message: 'm', details: {} }).code).toBe('unknown-command')
|
||||
expect(rpcErrorSchema.parse({ code: 'skill-not-found', message: 'm', details: { name: 'n' } }).code).toBe('skill-not-found')
|
||||
expect(rpcErrorSchema.parse({ code: 'skill-not-invocable', message: 'm', details: { name: 'n' } }).code).toBe('skill-not-invocable')
|
||||
expect(rpcErrorSchema.parse({ code: 'title-invalid', message: 'm', details: { sessionId: 's' } }).code).toBe('title-invalid')
|
||||
expect(rpcErrorSchema.parse({ code: 'internal', message: 'm', details: {} }).code).toBe('internal')
|
||||
})
|
||||
@@ -83,7 +81,6 @@ describe('rpcErrorSchema', () => {
|
||||
it('rejects a known code with missing details', () => {
|
||||
expect(() => rpcErrorSchema.parse({ code: 'agent-busy', message: 'm', details: {} })).toThrow()
|
||||
expect(() => rpcErrorSchema.parse({ code: 'title-invalid', message: 'm', details: {} })).toThrow()
|
||||
expect(() => rpcErrorSchema.parse({ code: 'skill-not-invocable', message: 'm', details: {} })).toThrow()
|
||||
expect(() => rpcErrorSchema.parse({ code: 'command-error', message: 'm' })).toThrow()
|
||||
expect(() => rpcErrorSchema.parse({ code: 'nope', message: 'm', details: {} })).toThrow()
|
||||
})
|
||||
@@ -408,19 +405,6 @@ describe('skills domain schemas', () => {
|
||||
// modelInvocable is required wire data: an entry without it fails.
|
||||
expect(() => skillEntrySchema.parse({ name: 'n', description: 'd' })).toThrow()
|
||||
})
|
||||
|
||||
it('validates the invoke request/value pair', () => {
|
||||
expect(skillInvokeRequestSchema.parse({ sessionId: 's1', name: 'user-only' }))
|
||||
.toEqual({ sessionId: 's1', name: 'user-only' })
|
||||
expect(skillInvokeRequestSchema.parse({ sessionId: 's1', name: 'user-only', text: 'check it' }).text)
|
||||
.toBe('check it')
|
||||
expect(() => skillInvokeRequestSchema.parse({ sessionId: 's1', name: '' })).toThrow()
|
||||
expect(() => skillInvokeRequestSchema.parse({ name: 'user-only' })).toThrow()
|
||||
// A blank trailing text is refused at the wire boundary, not by client courtesy.
|
||||
expect(() => skillInvokeRequestSchema.parse({ sessionId: 's1', name: 'user-only', text: '' })).toThrow()
|
||||
expect(skillInvokeValueSchema.parse({ accepted: true })).toEqual({ accepted: true })
|
||||
expect(() => skillInvokeValueSchema.parse({ accepted: false })).toThrow()
|
||||
})
|
||||
})
|
||||
|
||||
describe('goals domain schemas', () => {
|
||||
|
||||
Reference in New Issue
Block a user