Просмотр исходного кода

fix(host): harden skill.invoke at the enforcement boundary

Review fixes: recheck isUserInvocable on the loaded definition (list and
get collect independently, so a provider change between them could swap in
a user-disabled body — the skill-tool execute template's second check);
thread the carrier signal through the lookup and refuse an abandoned
caller's turn as cancelled; fold lookup/loader failures into the
structured internal error the list face already uses; refuse cwd-less
sessions with the skill.list stance; and reject blank trailing text at the
wire schema instead of relying on client trimming.
Yichen Jiang 2 месяцев назад
Родитель
Сommit
c4c2355b50

+ 44 - 18
packages/host/apiproxy/src/api-proxy.ts

@@ -2390,32 +2390,58 @@ export function createApiProxy(ctx: Context, defaults: ApiProxyDefaults): ApiPro
         }
       },
 
-      async invoke(request) {
+      async invoke(request, signal) {
         const { sessionId, name, text } = request.payload
         const resolved = await turnAgentFor<{ accepted: true }>(request, sessionId)
         if ('refused' in resolved) return resolved.refused
         const agent = resolved.agent
+        if (agent.session.header.cwd === undefined) {
+          // Same stance as skill.list: a cwd-less header is a pre-project
+          // legacy log, and skill discovery has no root to resolve against.
+          return err(request, { code: 'internal', message: `session "${sessionId}" has no project cwd`, details: {} })
+        }
         const skillRegistry = ctx.get('skills')
         if (skillRegistry === undefined) {
           return err(request, { code: 'internal', message: 'skill registry is absent: this deployment does not mount @deepseek-ai/dsh-skill in its composition (cordis.yml or explicit assembly)', details: {} })
         }
-        const lookup = { cwd: agent.session.header.cwd }
-        // isSkillName guards the registry contract; an ill-formed name is
-        // indistinguishable from an absent one for the caller.
-        const summary = isSkillName(name)
-          ? (await skillRegistry.list(lookup)).find(skill => skill.name === name)
-          : undefined
-        if (summary === undefined) {
-          return err(request, { code: 'skill-not-found', message: `skill "${name}" is unknown in this workspace`, details: { name } })
-        }
-        // The operation boundary owns user-invocation policy: client menus
-        // filtering their candidates is an affordance, not enforcement.
-        if (!isUserInvocable(summary)) {
-          return err(request, { code: 'skill-not-invocable', message: `skill "${name}" is not available for user invocation`, details: { name } })
-        }
-        const skill = await skillRegistry.get(name, lookup)
-        if (skill === undefined) {
-          return err(request, { code: 'skill-not-found', message: `skill "${name}" is unknown in this workspace`, details: { name } })
+        const lookup = { cwd: agent.session.header.cwd, signal }
+        let skill
+        try {
+          // isSkillName guards the registry contract; an ill-formed name is
+          // indistinguishable from an absent one for the caller.
+          const summary = isSkillName(name)
+            ? (await skillRegistry.list(lookup)).find(candidate => candidate.name === name)
+            : undefined
+          if (summary === undefined) {
+            return err(request, { code: 'skill-not-found', message: `skill "${name}" is unknown in this workspace`, details: { name } })
+          }
+          // The operation boundary owns user-invocation policy: client menus
+          // filtering their candidates is an affordance, not enforcement.
+          if (!isUserInvocable(summary)) {
+            return err(request, { code: 'skill-not-invocable', message: `skill "${name}" is not available for user invocation`, details: { name } })
+          }
+          const loaded = await skillRegistry.get(name, lookup)
+          if (loaded === undefined) {
+            return err(request, { code: 'skill-not-found', message: `skill "${name}" is unknown in this workspace`, details: { name } })
+          }
+          // Recheck on the loaded definition (the skill-tool execute template):
+          // list and get collect independently, so a provider change between
+          // the two awaits can swap the winning candidate for a user-disabled
+          // one — the boundary must judge what it actually injects.
+          if (!isUserInvocable(loaded)) {
+            return err(request, { code: 'skill-not-invocable', message: `skill "${name}" is not available for user invocation`, details: { name } })
+          }
+          skill = loaded
+        } catch (error: unknown) {
+          if (signal.aborted) {
+            return err(request, { code: 'cancelled', message: 'skill invocation cancelled', details: {} })
+          }
+          return err(request, { code: 'internal', message: `skill invocation failed: ${String(error)}`, details: {} })
+        }
+        if (signal.aborted) {
+          // The caller already gave up (unary deadline or navigation): a turn
+          // it will never observe must not start.
+          return err(request, { code: 'cancelled', message: 'skill invocation cancelled', details: {} })
         }
         const body = renderSkillContent(skill)
         const source: SkillInvocationSource = { kind: 'skill-invocation', name, ...text === undefined ? {} : { args: text } }

+ 5 - 2
packages/host/apiproxy/src/api/skills.schema.ts

@@ -27,11 +27,14 @@ export const skillListValueSchema = z.object({
   skills: z.array(skillEntrySchema),
 }) satisfies z.ZodType<Wire<ResponseValue<'skill.list'>>>
 
-/** skill.invoke request payload. */
+/**
+ * skill.invoke request payload. `text` is the user's trailing message; a
+ * blank one stays off the wire (the boundary, not client courtesy, refuses it).
+ */
 export const skillInvokeRequestSchema = z.object({
   sessionId: sessionIdSchema,
   name: z.string().min(1),
-  text: z.string().optional(),
+  text: z.string().min(1).optional(),
 }) satisfies z.ZodType<Wire<RequestPayload<'skill.invoke'>>>
 
 /** skill.invoke response value. */

+ 7 - 3
packages/host/apiproxy/src/api/skills.ts

@@ -29,9 +29,13 @@ export interface SkillsApi {
    * Injects one user-invocable skill into the addressed agent as a user-role
    * message (the canonical `<skill_content>` rendering, with `text` appended
    * when present) and starts a turn. The host enforces user-invocation policy
-   * here: a model-only or unknown name is refused regardless of what a client
-   * menu offered. Session-backed subagents reject with `agent-busy`.
+   * here — on the discovery summary and again on the loaded definition, so a
+   * catalog change between the two lookups cannot slip a user-disabled body
+   * through — a model-only or unknown name is refused regardless of what a
+   * client menu offered. The carrier's request signal aborts the skill
+   * lookup and refuses injection once the caller has given up (`cancelled`).
+   * Session-backed subagents reject with `agent-busy`.
    */
-  invoke(request: RpcRequest<{ sessionId: SessionId; name: string; text?: string }>):
+  invoke(request: RpcRequest<{ sessionId: SessionId; name: string; text?: string }>, signal: AbortSignal):
   Promise<RpcResponse<{ accepted: true }>>
 }

+ 1 - 1
packages/host/apiproxy/src/fetch/handler.ts

@@ -109,7 +109,7 @@ const UNARY_ROUTES: UnaryRoutes = {
   'command.list': { schema: commandListRequestSchema, invoke: (api, r) => api.commands.list(r) },
   'command.execute': { schema: commandExecuteRequestSchema, invoke: (api, r, signal) => api.commands.execute(r, signal) },
   'skill.list': { schema: skillListRequestSchema, invoke: (api, r) => api.skills.list(r) },
-  'skill.invoke': { schema: skillInvokeRequestSchema, invoke: (api, r) => api.skills.invoke(r) },
+  'skill.invoke': { schema: skillInvokeRequestSchema, invoke: (api, r, signal) => api.skills.invoke(r, signal) },
   'goal.create': { schema: goalCreateRequestSchema, invoke: (api, r) => api.goals.create(r) },
   'goal.edit': { schema: goalEditRequestSchema, invoke: (api, r) => api.goals.edit(r) },
   'goal.pause': { schema: goalPauseRequestSchema, invoke: (api, r) => api.goals.pause(r) },

+ 97 - 7
packages/host/apiproxy/tests/api-proxy-commands.spec.ts

@@ -305,6 +305,8 @@ describe('skill.invoke', () => {
     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)
@@ -312,7 +314,7 @@ describe('skill.invoke', () => {
     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
@@ -330,7 +332,7 @@ describe('skill.invoke', () => {
     registerInvokeSkills(ctx)
     const api = createApiProxy(ctx, DEFAULTS)
     const { agent, followup } = invokableAgent(ctx)
-    expectOk(await api.skills.invoke(request({ sessionId: agent.id, name: 'user-only' })))
+    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
@@ -342,39 +344,127 @@ describe('skill.invoke', () => {
     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' })))
+    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' })))
+    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' })))
+    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' })))
+    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' })))
+    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')
   })

+ 2 - 0
packages/host/apiproxy/tests/rpc-schemas.spec.ts

@@ -416,6 +416,8 @@ describe('skills domain schemas', () => {
       .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()
   })