Ver código fonte

fix(opencode): let subagents use their own permissions (#31696)

Aiden Cline 2 meses atrás
pai
commit
3ad6923c61

+ 3 - 0
packages/opencode/src/agent/agent.ts

@@ -160,6 +160,9 @@ export const layer = Layer.effect(
               Permission.fromConfig({
                 question: "allow",
                 plan_exit: "allow",
+                task: {
+                  general: "deny",
+                },
                 external_directory: {
                   [path.join(Global.Path.data, "plans", "*")]: "allow",
                 },

+ 4 - 12
packages/opencode/src/agent/subagent-permissions.ts

@@ -1,31 +1,23 @@
 import { PermissionV1 } from "@opencode-ai/core/v1/permission"
-import type { Permission } from "../permission"
 import type { Agent } from "./agent"
 
 /**
  * Build the `permission` ruleset for a subagent's session when it's spawned
  * via the task tool. Combines:
  *
- * 1. The parent **agent's** edit-class deny rules — Plan Mode's file-edit
- *    restriction lives on the agent ruleset, not on the session, so a
- *    subagent that only inherited the parent SESSION's permission would
- *    silently bypass it. (#26514)
- * 2. The parent **session's** deny rules and external_directory rules —
- *    same forwarding the original code already did.
- * 3. Default `todowrite` and `task` denies if the subagent's own ruleset
+ * 1. The parent session's deny rules and external_directory rules.
+ *    Parent agent restrictions only govern that agent; the subagent's own
+ *    permissions determine its capabilities.
+ * 2. Default `todowrite` and `task` denies if the subagent's own ruleset
  *    doesn't already permit them.
  */
 export function deriveSubagentSessionPermission(input: {
   parentSessionPermission: PermissionV1.Ruleset
-  parentAgent: Agent.Info | undefined
   subagent: Agent.Info
 }): PermissionV1.Ruleset {
   const canTask = input.subagent.permission.some((rule) => rule.permission === "task")
   const canTodo = input.subagent.permission.some((rule) => rule.permission === "todowrite")
-  const parentAgentDenies =
-    input.parentAgent?.permission.filter((rule) => rule.action === "deny" && rule.permission === "edit") ?? []
   return [
-    ...parentAgentDenies,
     ...input.parentSessionPermission.filter(
       (rule) => rule.permission === "external_directory" || rule.action === "deny",
     ),

+ 0 - 4
packages/opencode/src/tool/task.ts

@@ -122,12 +122,8 @@ export const TaskTool = Tool.define(
         ? yield* sessions.get(SessionID.make(params.task_id)).pipe(Effect.catchCause(() => Effect.succeed(undefined)))
         : undefined
       const parent = yield* sessions.get(ctx.sessionID)
-      const parentAgent = parent.agent
-        ? yield* agent.get(parent.agent).pipe(Effect.catchCause(() => Effect.succeed(undefined)))
-        : undefined
       const childPermission = deriveSubagentSessionPermission({
         parentSessionPermission: parent.permission ?? [],
-        parentAgent,
         subagent: next,
       })
       const childToolDenies = [

+ 29 - 0
packages/opencode/test/agent/agent.test.ts

@@ -85,6 +85,35 @@ it.instance("plan agent denies edits except .opencode/plans/*", () =>
   }),
 )
 
+it.instance("plan agent denies the general subagent by default", () =>
+  Effect.gen(function* () {
+    const plan = yield* load((svc) => svc.get("plan"))
+    expect(plan).toBeDefined()
+    expect(Permission.evaluate("task", "general", plan!.permission).action).toBe("deny")
+    expect(Permission.evaluate("task", "explore", plan!.permission).action).toBe("allow")
+    expect(Permission.evaluate("task", "custom", plan!.permission).action).toBe("allow")
+  }),
+)
+
+it.instance(
+  "user permission can allow the general subagent from plan mode",
+  () =>
+    Effect.gen(function* () {
+      const plan = yield* load((svc) => svc.get("plan"))
+      expect(plan).toBeDefined()
+      expect(Permission.evaluate("task", "general", plan!.permission).action).toBe("allow")
+    }),
+  {
+    config: {
+      permission: {
+        task: {
+          general: "allow",
+        },
+      },
+    },
+  },
+)
+
 it.instance("explore agent denies edit and write", () =>
   Effect.gen(function* () {
     const explore = yield* load((svc) => svc.get("explore"))

+ 14 - 68
packages/opencode/test/agent/plan-mode-subagent-bypass.test.ts

@@ -1,24 +1,4 @@
 import { PermissionV1 } from "@opencode-ai/core/v1/permission"
-/**
- * Reproducer for opencode issue #26514:
- *
- * In Plan Mode (the `plan` agent), the main agent's edit/write tools are
- * blocked by the plan agent's permission ruleset (`edit: { "*": "deny" }`).
- * However, when the plan agent spawns a subagent via the `task` tool, the
- * subagent retains full file modification capabilities — a security bypass.
- *
- * This test replicates the permission ruleset that would govern a
- * `general` subagent when launched from a `plan` parent session, mirroring
- * the logic in `src/tool/task.ts` (filtered parent permissions ++ runtime
- * subagent agent permissions, evaluated as in `session/prompt.ts`).
- *
- * The expected (secure) behavior is that the subagent inherits the plan
- * mode read-only restriction and `edit`/`write` resolve to `deny`. On
- * origin/dev this assertion fails because the parent **agent** permissions
- * are not propagated to the subagent — only the parent **session**
- * permissions are passed through, and Plan Mode's restrictions live on the
- * agent, not the session.
- */
 import { expect } from "bun:test"
 import { Effect } from "effect"
 import { Agent } from "../../src/agent/agent"
@@ -45,7 +25,7 @@ function testAgent(input: {
 // exercises the actual helper that task.ts uses to build the subagent's
 // session permission, so any regression in that helper trips this test.
 
-it.instance("[#26514] subagent spawned from plan mode inherits read-only restriction (edit denied)", () =>
+it.instance("subagent permissions take precedence over parent agent restrictions", () =>
   Effect.gen(function* () {
     const planAgent = yield* Agent.use.get("plan")
     const generalAgent = yield* Agent.use.get("general")
@@ -57,15 +37,10 @@ it.instance("[#26514] subagent spawned from plan mode inherits read-only restric
     // tool layer — see Permission.disabled / EDIT_TOOLS.)
     expect(Permission.evaluate("edit", "/some/file.ts", planAgent!.permission).action).toBe("deny")
 
-    // Simulate the plan-mode parent session: in real flow the plan
-    // session's `permission` field is empty (Plan Mode lives on the agent
-    // ruleset, not the session). So we pass [] through as the parent
-    // session permission, exactly like the actual code path.
     const parentSessionPermission: PermissionV1.Ruleset = []
 
     const subagentSessionPermission = deriveSubagentSessionPermission({
       parentSessionPermission,
-      parentAgent: planAgent,
       subagent: generalAgent!,
     })
 
@@ -73,40 +48,29 @@ it.instance("[#26514] subagent spawned from plan mode inherits read-only restric
     //   ruleset: Permission.merge(agent.permission, session.permission ?? [])
     const effective = Permission.merge(generalAgent!.permission, subagentSessionPermission)
 
-    expect(Permission.evaluate("edit", "/some/file.ts", effective).action).toBe("deny")
-    expect(Permission.evaluate("edit", "/another/path/index.tsx", effective).action).toBe("deny")
+    expect(Permission.evaluate("edit", "/some/file.ts", effective).action).not.toBe("deny")
+    expect(Permission.disabled(["edit", "write", "apply_patch"], effective)).toEqual(new Set())
   }),
 )
 
-it.instance("[#26514] explore subagent launched from plan mode also stays read-only", () =>
-  // Sibling check: even though `explore` is intrinsically read-only, the
-  // bug surface is the same. Including this case to document that the fix
-  // should propagate the parent **agent** permissions, not just deny edit
-  // when the subagent happens to already deny it.
+it.instance("subagent's own read-only restriction remains effective", () =>
   Effect.gen(function* () {
-    const planAgent = yield* Agent.use.get("plan")
     const explore = yield* Agent.use.get("explore")
-    expect(planAgent).toBeDefined()
     expect(explore).toBeDefined()
 
     const parentSessionPermission: PermissionV1.Ruleset = []
     const subagentSessionPermission = deriveSubagentSessionPermission({
       parentSessionPermission,
-      parentAgent: planAgent,
       subagent: explore!,
     })
     const effective = Permission.merge(explore!.permission, subagentSessionPermission)
 
-    // Already deny — sanity check.
     expect(Permission.evaluate("edit", "/x.ts", effective).action).toBe("deny")
   }),
 )
 
 it.instance(
-  "[#26514] custom user subagent launched from plan mode bypasses Plan Mode read-only",
-  // The most damaging case: a user-defined subagent with default
-  // permissions (allow-by-default, like `general`). The subagent must NOT
-  // be able to edit when the parent agent is `plan`.
+  "custom subagent can explicitly enable edits denied to its parent agent",
   () =>
     Effect.gen(function* () {
       const planAgent = yield* Agent.use.get("plan")
@@ -117,14 +81,13 @@ it.instance(
       const parentSessionPermission: PermissionV1.Ruleset = []
       const subagentSessionPermission = deriveSubagentSessionPermission({
         parentSessionPermission,
-        parentAgent: planAgent,
         subagent: my!,
       })
       const effective = Permission.merge(my!.permission, subagentSessionPermission)
 
-      // BUG: on origin/dev edit resolves to "allow" because the plan
-      // agent's `edit: deny *` rule never reaches the subagent.
-      expect(Permission.evaluate("edit", "/some/file.ts", effective).action).toBe("deny")
+      expect(Permission.evaluate("edit", "/some/file.ts", planAgent!.permission).action).toBe("deny")
+      expect(Permission.evaluate("edit", "/some/file.ts", effective).action).toBe("allow")
+      expect(Permission.disabled(["edit", "write", "apply_patch"], effective)).toEqual(new Set())
     }),
   {
     config: {
@@ -132,29 +95,17 @@ it.instance(
         my_subagent: {
           description: "A user-defined subagent",
           mode: "subagent",
+          permission: {
+            edit: "allow",
+          },
         },
       },
     },
   },
 )
 
-it.effect("[#26700] controller self-restrictions do not erase executor permissions", () =>
+it.effect("subagent self permissions are preserved", () =>
   Effect.sync(() => {
-    const controller = testAgent({
-      name: "controller",
-      mode: "primary",
-      permission: {
-        "*": "deny",
-        read: "deny",
-        bash: "deny",
-        task: {
-          "*": "deny",
-          executor: "allow",
-        },
-        edit: "deny",
-        write: "deny",
-      },
-    })
     const executor = testAgent({
       name: "executor",
       mode: "subagent",
@@ -166,8 +117,7 @@ it.effect("[#26700] controller self-restrictions do not erase executor permissio
           "*": "deny",
           worker: "allow",
         },
-        edit: "deny",
-        write: "deny",
+        edit: "allow",
       },
     })
 
@@ -175,7 +125,6 @@ it.effect("[#26700] controller self-restrictions do not erase executor permissio
       executor.permission,
       deriveSubagentSessionPermission({
         parentSessionPermission: [],
-        parentAgent: controller,
         subagent: executor,
       }),
     )
@@ -184,9 +133,7 @@ it.effect("[#26700] controller self-restrictions do not erase executor permissio
     expect(Permission.evaluate("bash", "git status", effective).action).toBe("allow")
     expect(Permission.evaluate("task", "worker", effective).action).toBe("allow")
     expect(Permission.evaluate("task", "other", effective).action).toBe("deny")
-    expect(Permission.disabled(["edit", "write", "apply_patch"], effective)).toEqual(
-      new Set(["edit", "write", "apply_patch"]),
-    )
+    expect(Permission.disabled(["edit", "write", "apply_patch"], effective)).toEqual(new Set())
   }),
 )
 
@@ -203,7 +150,6 @@ it.effect("subagent inherits parent session deny rules as hard runtime ceilings"
       executor.permission,
       deriveSubagentSessionPermission({
         parentSessionPermission: Permission.fromConfig({ bash: "deny" }),
-        parentAgent: undefined,
         subagent: executor,
       }),
     )