diff --git a/packages/opencode/src/agent/agent.ts b/packages/opencode/src/agent/agent.ts index 142440087..b1430314f 100644 --- a/packages/opencode/src/agent/agent.ts +++ b/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", }, diff --git a/packages/opencode/src/agent/subagent-permissions.ts b/packages/opencode/src/agent/subagent-permissions.ts index 56da42626..b1b99b484 100644 --- a/packages/opencode/src/agent/subagent-permissions.ts +++ b/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", ), diff --git a/packages/opencode/src/tool/task.ts b/packages/opencode/src/tool/task.ts index dac340184..b0a866c90 100644 --- a/packages/opencode/src/tool/task.ts +++ b/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 = [ diff --git a/packages/opencode/test/agent/agent.test.ts b/packages/opencode/test/agent/agent.test.ts index 67071bce7..1df95b5c0 100644 --- a/packages/opencode/test/agent/agent.test.ts +++ b/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")) diff --git a/packages/opencode/test/agent/plan-mode-subagent-bypass.test.ts b/packages/opencode/test/agent/plan-mode-subagent-bypass.test.ts index de0e2cd46..a58a5ddf2 100644 --- a/packages/opencode/test/agent/plan-mode-subagent-bypass.test.ts +++ b/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, }), )