Prechádzať zdrojové kódy

refactor: simplify task tool subagent filtering (#7165)

Co-authored-by: Aiden Cline <63023139+rekram1-node@users.noreply.github.com>
M. Adel Alhashemi 8 mesiacov pred
rodič
commit
34c9d106ee

+ 8 - 32
packages/opencode/src/session/prompt.ts

@@ -37,7 +37,7 @@ import { SessionSummary } from "./summary"
 import { NamedError } from "@opencode-ai/util/error"
 import { fn } from "@/util/fn"
 import { SessionProcessor } from "./processor"
-import { TaskTool, filterSubagents, TASK_DESCRIPTION } from "@/tool/task"
+import { TaskTool } from "@/tool/task"
 import { Tool } from "@/tool/tool"
 import { PermissionNext } from "@/permission/next"
 import { SessionStatus } from "./status"
@@ -383,7 +383,7 @@ export namespace SessionPrompt {
           sessionID: sessionID,
           abort,
           callID: part.callID,
-          extra: { userInvokedAgents: [task.agent] },
+          extra: { bypassAgentCheck: true },
           async metadata(input) {
             await Session.updatePart({
               ...part,
@@ -545,11 +545,9 @@ export namespace SessionPrompt {
         abort,
       })
 
-      // Track agents explicitly invoked by user via @ autocomplete
-      const userInvokedAgents = msgs
-        .filter((m) => m.info.role === "user")
-        .flatMap((m) => m.parts.filter((p) => p.type === "agent") as MessageV2.AgentPart[])
-        .map((p) => p.name)
+      // Check if user explicitly invoked an agent via @ in this turn
+      const lastUserMsg = msgs.findLast((m) => m.info.role === "user")
+      const bypassAgentCheck = lastUserMsg?.parts.some((p) => p.type === "agent") ?? false
 
       const tools = await resolveTools({
         agent,
@@ -557,7 +555,7 @@ export namespace SessionPrompt {
         model,
         tools: lastUser.tools,
         processor,
-        userInvokedAgents,
+        bypassAgentCheck,
       })
 
       if (step === 1) {
@@ -646,7 +644,7 @@ export namespace SessionPrompt {
     session: Session.Info
     tools?: Record<string, boolean>
     processor: SessionProcessor.Info
-    userInvokedAgents: string[]
+    bypassAgentCheck: boolean
   }) {
     using _ = log.time("resolveTools")
     const tools: Record<string, AITool> = {}
@@ -656,7 +654,7 @@ export namespace SessionPrompt {
       abort: options.abortSignal!,
       messageID: input.processor.message.id,
       callID: options.toolCallId,
-      extra: { model: input.model, userInvokedAgents: input.userInvokedAgents },
+      extra: { model: input.model, bypassAgentCheck: input.bypassAgentCheck },
       agent: input.agent.name,
       metadata: async (val: { title?: string; metadata?: any }) => {
         const match = input.processor.partFromToolCall(options.toolCallId)
@@ -800,28 +798,6 @@ export namespace SessionPrompt {
       tools[key] = item
     }
 
-    // Regenerate task tool description with filtered subagents
-    if (tools.task) {
-      const all = await Agent.list().then((x) => x.filter((a) => a.mode !== "primary"))
-      const filtered = filterSubagents(all, input.agent.permission)
-
-      // If no subagents are permitted, remove the task tool entirely
-      if (filtered.length === 0) {
-        delete tools.task
-      } else {
-        const description = TASK_DESCRIPTION.replace(
-          "{agents}",
-          filtered
-            .map((a) => `- ${a.name}: ${a.description ?? "This subagent should only be called manually by the user."}`)
-            .join("\n"),
-        )
-        tools.task = {
-          ...tools.task,
-          description,
-        }
-      }
-    }
-
     return tools
   }
 

+ 20 - 18
packages/opencode/src/tool/task.ts

@@ -12,35 +12,37 @@ import { defer } from "@/util/defer"
 import { Config } from "../config/config"
 import { PermissionNext } from "@/permission/next"
 
-export { DESCRIPTION as TASK_DESCRIPTION }
-
-export function filterSubagents(agents: Agent.Info[], ruleset: PermissionNext.Ruleset) {
-  return agents.filter((a) => PermissionNext.evaluate("task", a.name, ruleset).action !== "deny")
-}
+const parameters = z.object({
+  description: z.string().describe("A short (3-5 words) description of the task"),
+  prompt: z.string().describe("The task for the agent to perform"),
+  subagent_type: z.string().describe("The type of specialized agent to use for this task"),
+  session_id: z.string().describe("Existing Task session to continue").optional(),
+  command: z.string().describe("The command that triggered this task").optional(),
+})
 
-export const TaskTool = Tool.define("task", async () => {
+export const TaskTool = Tool.define("task", async (ctx) => {
   const agents = await Agent.list().then((x) => x.filter((a) => a.mode !== "primary"))
+
+  // Filter agents by permissions if agent provided
+  const caller = ctx?.agent
+  const accessibleAgents = caller
+    ? agents.filter((a) => PermissionNext.evaluate("task", a.name, caller.permission).action !== "deny")
+    : agents
+
   const description = DESCRIPTION.replace(
     "{agents}",
-    agents
+    accessibleAgents
       .map((a) => `- ${a.name}: ${a.description ?? "This subagent should only be called manually by the user."}`)
       .join("\n"),
   )
   return {
     description,
-    parameters: z.object({
-      description: z.string().describe("A short (3-5 words) description of the task"),
-      prompt: z.string().describe("The task for the agent to perform"),
-      subagent_type: z.string().describe("The type of specialized agent to use for this task"),
-      session_id: z.string().describe("Existing Task session to continue").optional(),
-      command: z.string().describe("The command that triggered this task").optional(),
-    }),
-    async execute(params, ctx) {
+    parameters,
+    async execute(params: z.infer<typeof parameters>, ctx) {
       const config = await Config.get()
 
-      const userInvokedAgents = (ctx.extra?.userInvokedAgents ?? []) as string[]
-      // Skip permission check when invoked from a command subtask (user already approved by invoking the command)
-      if (!ctx.extra?.bypassAgentCheck && !userInvokedAgents.includes(params.subagent_type)) {
+      // Skip permission check when user explicitly invoked via @ or command subtask
+      if (!ctx.extra?.bypassAgentCheck) {
         await ctx.ask({
           permission: "task",
           patterns: [params.subagent_type],

+ 8 - 148
packages/opencode/test/permission-task.test.ts

@@ -1,147 +1,9 @@
 import { describe, test, expect } from "bun:test"
-import type { Agent } from "../src/agent/agent"
-import { filterSubagents } from "../src/tool/task"
 import { PermissionNext } from "../src/permission/next"
 import { Config } from "../src/config/config"
 import { Instance } from "../src/project/instance"
 import { tmpdir } from "./fixture/fixture"
 
-describe("filterSubagents - permission.task filtering", () => {
-  const createRuleset = (rules: Record<string, "allow" | "deny" | "ask">): PermissionNext.Ruleset =>
-    Object.entries(rules).map(([pattern, action]) => ({
-      permission: "task",
-      pattern,
-      action,
-    }))
-
-  const mockAgents = [
-    { name: "general", mode: "subagent", permission: [], options: {} },
-    { name: "code-reviewer", mode: "subagent", permission: [], options: {} },
-    { name: "orchestrator-fast", mode: "subagent", permission: [], options: {} },
-    { name: "orchestrator-slow", mode: "subagent", permission: [], options: {} },
-  ] as Agent.Info[]
-
-  test("returns all agents when permissions config is empty", () => {
-    const result = filterSubagents(mockAgents, [])
-    expect(result).toHaveLength(4)
-    expect(result.map((a) => a.name)).toEqual(["general", "code-reviewer", "orchestrator-fast", "orchestrator-slow"])
-  })
-
-  test("excludes agents with explicit deny", () => {
-    const ruleset = createRuleset({ "code-reviewer": "deny" })
-    const result = filterSubagents(mockAgents, ruleset)
-    expect(result).toHaveLength(3)
-    expect(result.map((a) => a.name)).toEqual(["general", "orchestrator-fast", "orchestrator-slow"])
-  })
-
-  test("includes agents with explicit allow", () => {
-    const ruleset = createRuleset({
-      "code-reviewer": "allow",
-      general: "deny",
-    })
-    const result = filterSubagents(mockAgents, ruleset)
-    expect(result).toHaveLength(3)
-    expect(result.map((a) => a.name)).toEqual(["code-reviewer", "orchestrator-fast", "orchestrator-slow"])
-  })
-
-  test("includes agents with ask permission (user approval is runtime behavior)", () => {
-    const ruleset = createRuleset({
-      "code-reviewer": "ask",
-      general: "deny",
-    })
-    const result = filterSubagents(mockAgents, ruleset)
-    expect(result).toHaveLength(3)
-    expect(result.map((a) => a.name)).toEqual(["code-reviewer", "orchestrator-fast", "orchestrator-slow"])
-  })
-
-  test("includes agents with undefined permission (default allow)", () => {
-    const ruleset = createRuleset({
-      general: "deny",
-    })
-    const result = filterSubagents(mockAgents, ruleset)
-    expect(result).toHaveLength(3)
-    expect(result.map((a) => a.name)).toEqual(["code-reviewer", "orchestrator-fast", "orchestrator-slow"])
-  })
-
-  test("supports wildcard patterns with deny", () => {
-    const ruleset = createRuleset({ "orchestrator-*": "deny" })
-    const result = filterSubagents(mockAgents, ruleset)
-    expect(result).toHaveLength(2)
-    expect(result.map((a) => a.name)).toEqual(["general", "code-reviewer"])
-  })
-
-  test("supports wildcard patterns with allow", () => {
-    const ruleset = createRuleset({
-      "*": "allow",
-      "orchestrator-fast": "deny",
-    })
-    const result = filterSubagents(mockAgents, ruleset)
-    expect(result).toHaveLength(3)
-    expect(result.map((a) => a.name)).toEqual(["general", "code-reviewer", "orchestrator-slow"])
-  })
-
-  test("supports wildcard patterns with ask", () => {
-    const ruleset = createRuleset({
-      "orchestrator-*": "ask",
-    })
-    const result = filterSubagents(mockAgents, ruleset)
-    expect(result).toHaveLength(4)
-    expect(result.map((a) => a.name)).toEqual(["general", "code-reviewer", "orchestrator-fast", "orchestrator-slow"])
-  })
-
-  test("longer pattern takes precedence over shorter pattern", () => {
-    const ruleset = createRuleset({
-      "orchestrator-*": "deny",
-      "orchestrator-fast": "allow",
-    })
-    const result = filterSubagents(mockAgents, ruleset)
-    expect(result).toHaveLength(3)
-    expect(result.map((a) => a.name)).toEqual(["general", "code-reviewer", "orchestrator-fast"])
-  })
-
-  test("edge case: all agents denied", () => {
-    const ruleset = createRuleset({ "*": "deny" })
-    const result = filterSubagents(mockAgents, ruleset)
-    expect(result).toHaveLength(0)
-    expect(result).toEqual([])
-  })
-
-  test("edge case: mixed patterns with multiple wildcards", () => {
-    const ruleset = createRuleset({
-      "*": "ask",
-      "orchestrator-*": "deny",
-      "orchestrator-fast": "allow",
-    })
-    const result = filterSubagents(mockAgents, ruleset)
-    expect(result).toHaveLength(3)
-    expect(result.map((a) => a.name)).toEqual(["general", "code-reviewer", "orchestrator-fast"])
-  })
-
-  test("hidden: true does not affect filtering (hidden only affects autocomplete)", () => {
-    const agents = [
-      { name: "general", mode: "subagent", hidden: true, permission: [], options: {} },
-      { name: "code-reviewer", mode: "subagent", hidden: false, permission: [], options: {} },
-      { name: "orchestrator", mode: "subagent", permission: [], options: {} },
-    ] as Agent.Info[]
-
-    const result = filterSubagents(agents, [])
-    expect(result).toHaveLength(3)
-    expect(result.map((a) => a.name)).toEqual(["general", "code-reviewer", "orchestrator"])
-  })
-
-  test("hidden: true agents can be filtered by permission.task deny", () => {
-    const agents = [
-      { name: "general", mode: "subagent", hidden: true, permission: [], options: {} },
-      { name: "orchestrator-coder", mode: "subagent", hidden: true, permission: [], options: {} },
-    ] as Agent.Info[]
-
-    const ruleset = createRuleset({ general: "deny" })
-    const result = filterSubagents(agents, ruleset)
-    expect(result).toHaveLength(1)
-    expect(result.map((a) => a.name)).toEqual(["orchestrator-coder"])
-  })
-})
-
 describe("PermissionNext.evaluate for permission.task", () => {
   const createRuleset = (rules: Record<string, "allow" | "deny" | "ask">): PermissionNext.Ruleset =>
     Object.entries(rules).map(([pattern, action]) => ({
@@ -277,12 +139,6 @@ describe("PermissionNext.disabled for task tool", () => {
 
 // Integration tests that load permissions from real config files
 describe("permission.task with real config files", () => {
-  const mockAgents = [
-    { name: "general", mode: "subagent", permission: [], options: {} },
-    { name: "code-reviewer", mode: "subagent", permission: [], options: {} },
-    { name: "orchestrator-fast", mode: "subagent", permission: [], options: {} },
-  ] as Agent.Info[]
-
   test("loads task permissions from opencode.json config", async () => {
     await using tmp = await tmpdir({
       git: true,
@@ -300,8 +156,10 @@ describe("permission.task with real config files", () => {
       fn: async () => {
         const config = await Config.get()
         const ruleset = PermissionNext.fromConfig(config.permission ?? {})
-        const result = filterSubagents(mockAgents, ruleset)
-        expect(result.map((a) => a.name)).toEqual(["general", "orchestrator-fast"])
+        // general and orchestrator-fast should be allowed, code-reviewer denied
+        expect(PermissionNext.evaluate("task", "general", ruleset).action).toBe("allow")
+        expect(PermissionNext.evaluate("task", "orchestrator-fast", ruleset).action).toBe("allow")
+        expect(PermissionNext.evaluate("task", "code-reviewer", ruleset).action).toBe("deny")
       },
     })
   })
@@ -323,8 +181,10 @@ describe("permission.task with real config files", () => {
       fn: async () => {
         const config = await Config.get()
         const ruleset = PermissionNext.fromConfig(config.permission ?? {})
-        const result = filterSubagents(mockAgents, ruleset)
-        expect(result.map((a) => a.name)).toEqual(["general", "code-reviewer"])
+        // general and code-reviewer should be ask, orchestrator-* denied
+        expect(PermissionNext.evaluate("task", "general", ruleset).action).toBe("ask")
+        expect(PermissionNext.evaluate("task", "code-reviewer", ruleset).action).toBe("ask")
+        expect(PermissionNext.evaluate("task", "orchestrator-fast", ruleset).action).toBe("deny")
       },
     })
   })