From 28204720dcb6f3d42d1015432fff0acecef10b3a Mon Sep 17 00:00:00 2001 From: Aiden Cline <63023139+rekram1-node@users.noreply.github.com> Date: Tue, 12 May 2026 23:55:45 -0500 Subject: [PATCH] temporarily revert: preserve permission ordering by accepting a layered array (#27258) --- packages/opencode/src/agent/agent.ts | 6 +- packages/opencode/src/config/config.ts | 25 +-- packages/opencode/src/config/permission.ts | 8 - packages/opencode/test/config/config.test.ts | 178 +----------------- .../opencode/test/permission-task.test.ts | 25 +-- 5 files changed, 18 insertions(+), 224 deletions(-) diff --git a/packages/opencode/src/agent/agent.ts b/packages/opencode/src/agent/agent.ts index 74ca1a402b..423a513180 100644 --- a/packages/opencode/src/agent/agent.ts +++ b/packages/opencode/src/agent/agent.ts @@ -1,5 +1,4 @@ import { Config } from "@/config/config" -import { ConfigPermission } from "@/config/permission" import { Provider } from "@/provider/provider" import { ModelID, ProviderID } from "../provider/schema" import { generateObject, streamObject, type ModelMessage } from "ai" @@ -118,10 +117,7 @@ export const layer = Layer.effect( }, }) - // Convert permission layers to rulesets and merge them - // Each layer's rules come after the previous, so later configs override earlier ones - const layers = ConfigPermission.toLayers(cfg.permission) - const user = Permission.merge(...layers.map((p) => Permission.fromConfig(p))) + const user = Permission.fromConfig(cfg.permission ?? {}) const agents: Record = { build: { diff --git a/packages/opencode/src/config/config.ts b/packages/opencode/src/config/config.ts index 78bb83ed8e..545e48e64d 100644 --- a/packages/opencode/src/config/config.ts +++ b/packages/opencode/src/config/config.ts @@ -55,16 +55,6 @@ function mergeConfigConcatArrays(target: Info, source: Info): Info { if (target.instructions && source.instructions) { merged.instructions = Array.from(new Set([...target.instructions, ...source.instructions])) } - // Accumulate permission layers for later merging as rulesets. - // This preserves the ordering semantics: later rules override earlier rules. - // Each layer keeps the raw shape the user wrote on disk; consumers should use - // ConfigPermission.toLayers to normalise. - if (source.permission) { - merged.permission = [ - ...ConfigPermission.toLayers(target.permission), - ...ConfigPermission.toLayers(source.permission), - ] - } return merged } @@ -238,12 +228,7 @@ export const Info = Schema.Struct({ description: "Additional instruction files or patterns to include", }), layout: Schema.optional(ConfigLayout.Layout).annotate({ description: "@deprecated Always uses stretch layout." }), - permission: Schema.optional( - Schema.Union([ConfigPermission.Info, Schema.mutable(Schema.Array(ConfigPermission.Info))]), - ).annotate({ - description: - "Permission configuration. Accepts a single object (per-tool action map) or an array of layered configs; arrays are merged in order so later layers override earlier ones.", - }), + permission: Schema.optional(ConfigPermission.Info), tools: Schema.optional(Schema.Record(Schema.String, Schema.Boolean)), attachment: Schema.optional(ConfigAttachment.Info).annotate({ description: "Attachment processing configuration, including image size limits and resizing behavior", @@ -719,12 +704,11 @@ export const layer = Layer.effect( } if (Flag.OPENCODE_PERMISSION) { - const envPermission = JSON.parse(Flag.OPENCODE_PERMISSION) as ConfigPermission.Info - result.permission = [...ConfigPermission.toLayers(result.permission), envPermission] + result.permission = mergeDeep(result.permission ?? {}, JSON.parse(Flag.OPENCODE_PERMISSION)) } if (result.tools) { - const perms: ConfigPermission.Info = {} + const perms: Record = {} for (const [tool, enabled] of Object.entries(result.tools)) { const action: ConfigPermission.Action = enabled ? "allow" : "deny" if (tool === "write" || tool === "edit" || tool === "patch") { @@ -733,8 +717,7 @@ export const layer = Layer.effect( } perms[tool] = action } - // Tools permissions come before other permissions (they can be overridden) - result.permission = [perms, ...ConfigPermission.toLayers(result.permission)] + result.permission = mergeDeep(perms, result.permission ?? {}) } if (!result.username) result.username = os.userInfo().username diff --git a/packages/opencode/src/config/permission.ts b/packages/opencode/src/config/permission.ts index c780d24436..1092ae2b7e 100644 --- a/packages/opencode/src/config/permission.ts +++ b/packages/opencode/src/config/permission.ts @@ -56,11 +56,3 @@ export const Info = InputSchema.pipe( ).annotate({ identifier: "PermissionConfig" }) type _Info = Schema.Schema.Type export type Info = { -readonly [K in keyof _Info]: _Info[K] } - -// Top-level config accepts either a single permission object or an array of -// layered configs. Internal merging produces arrays; this helper normalises -// either shape into the array form expected by consumers. -export function toLayers(value: Info | Info[] | undefined): Info[] { - if (!value) return [] - return Array.isArray(value) ? value : [value] -} diff --git a/packages/opencode/test/config/config.test.ts b/packages/opencode/test/config/config.test.ts index a2e4391777..90e78efcdb 100644 --- a/packages/opencode/test/config/config.test.ts +++ b/packages/opencode/test/config/config.test.ts @@ -3,9 +3,7 @@ import { Effect, Layer, Option } from "effect" import { NodeFileSystem, NodePath } from "@effect/platform-node" import { Config } from "@/config/config" import { ConfigManaged } from "@/config/managed" -import { ConfigPermission } from "@/config/permission" import { ConfigParse } from "../../src/config/parse" -import { Permission } from "../../src/permission" import { EffectFlock } from "@opencode-ai/core/util/effect-flock" import { Instance } from "../../src/project/instance" @@ -278,40 +276,6 @@ test("updates global config and omits empty shell key in json", async () => { } }) -test("global config update preserves single-object permission shape on disk", async () => { - await using tmp = await tmpdir({ - init: async (dir) => { - await Filesystem.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - $schema: "https://opencode.ai/config.json", - shell: "bash", - permission: { bash: "ask" }, - }), - ) - }, - }) - - const prev = Global.Path.config - ;(Global.Path as { config: string }).config = tmp.path - await clear(true) - - try { - // Updating an unrelated key must not rewrite `permission` from object to array form. - await saveGlobal({ shell: "zsh" }) - - const written = await Filesystem.readJson<{ permission?: unknown; shell?: string }>( - path.join(tmp.path, "opencode.json"), - ) - expect(written.shell).toBe("zsh") - expect(Array.isArray(written.permission)).toBe(false) - expect(written.permission).toEqual({ bash: "ask" }) - } finally { - ;(Global.Path as { config: string }).config = prev - await clear(true) - } -}) - test("updates global config and omits empty shell key in jsonc", async () => { await using tmp = await tmpdir({ init: async (dir) => { @@ -1749,10 +1713,7 @@ test("permission config preserves user key order", async () => { directory: tmp.path, fn: async () => { const config = await load() - // load() goes through the merge pipeline, producing the layered array form - expect(config.permission).toHaveLength(1) - const perm = (config.permission as ConfigPermission.Info[])[0] - expect(Object.keys(perm)).toEqual([ + expect(Object.keys(config.permission!)).toEqual([ "*", "edit", "write", @@ -1768,129 +1729,6 @@ test("permission config preserves user key order", async () => { }) }) -// Global bash "rm *" deny is inherited, but user's top-level "*" ask comes after and overrides it -test("user top-level catchall overrides inherited bash rules", async () => { - await using tmp = await tmpdir({ - init: async (dir) => { - await Filesystem.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - $schema: "https://opencode.ai/config.json", - permission: { - bash: { "rm *": "deny" }, - }, - }), - ) - const opencodeDir = path.join(dir, ".opencode") - await fs.mkdir(opencodeDir, { recursive: true }) - await Filesystem.write( - path.join(opencodeDir, "opencode.json"), - JSON.stringify({ - $schema: "https://opencode.ai/config.json", - permission: { - "*": "ask", - bash: { "ls *": "allow" }, - }, - }), - ) - }, - }) - await WithInstance.provide({ - directory: tmp.path, - fn: async () => { - const config = await load() - const layers = ConfigPermission.toLayers(config.permission) - const ruleset = Permission.merge(...layers.map((p) => Permission.fromConfig(p))) - - expect(Permission.evaluate("bash", "rm -rf /", ruleset).action).toBe("ask") - expect(Permission.evaluate("bash", "ls -la", ruleset).action).toBe("allow") - expect(Permission.evaluate("bash", "echo hello", ruleset).action).toBe("ask") - }, - }) -}) - -// No top-level catchall, so global bash "rm *" deny is preserved -test("inherited bash rules apply when no user top-level catchall", async () => { - await using tmp = await tmpdir({ - init: async (dir) => { - await Filesystem.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - $schema: "https://opencode.ai/config.json", - permission: { - bash: { "rm *": "deny" }, - }, - }), - ) - const opencodeDir = path.join(dir, ".opencode") - await fs.mkdir(opencodeDir, { recursive: true }) - await Filesystem.write( - path.join(opencodeDir, "opencode.json"), - JSON.stringify({ - $schema: "https://opencode.ai/config.json", - permission: { - bash: { "ls *": "allow" }, - }, - }), - ) - }, - }) - await WithInstance.provide({ - directory: tmp.path, - fn: async () => { - const config = await load() - const layers = ConfigPermission.toLayers(config.permission) - const ruleset = Permission.merge(...layers.map((p) => Permission.fromConfig(p))) - - expect(Permission.evaluate("bash", "rm -rf /", ruleset).action).toBe("deny") - expect(Permission.evaluate("bash", "ls -la", ruleset).action).toBe("allow") - }, - }) -}) - -// User's bash "*" catchall overrides global "rm *" deny -test("user bash catchall overrides inherited bash rules", async () => { - await using tmp = await tmpdir({ - init: async (dir) => { - await Filesystem.write( - path.join(dir, "opencode.json"), - JSON.stringify({ - $schema: "https://opencode.ai/config.json", - permission: { - bash: { "rm *": "deny" }, - }, - }), - ) - const opencodeDir = path.join(dir, ".opencode") - await fs.mkdir(opencodeDir, { recursive: true }) - await Filesystem.write( - path.join(opencodeDir, "opencode.json"), - JSON.stringify({ - $schema: "https://opencode.ai/config.json", - permission: { - bash: { "*": "ask", "ls *": "allow" }, - }, - }), - ) - }, - }) - await WithInstance.provide({ - directory: tmp.path, - fn: async () => { - const config = await load() - const layers = ConfigPermission.toLayers(config.permission) - const ruleset = Permission.merge(...layers.map((p) => Permission.fromConfig(p))) - - expect(Permission.evaluate("bash", "rm -rf /", ruleset).action).toBe("ask") - expect(Permission.evaluate("bash", "ls -la", ruleset).action).toBe("allow") - expect(Permission.evaluate("bash", "echo hello", ruleset).action).toBe("ask") - - // Non-bash permissions should use the top-level "*" rule - expect(Permission.evaluate("read", "foo.txt", ruleset).action).toBe("ask") - }, - }) -}) - test("config parser preserves permission order while rejecting unknown top-level keys", () => { const config = ConfigParse.schema( Config.Info, @@ -1904,8 +1742,7 @@ test("config parser preserves permission order while rejecting unknown top-level "test", ) - // ConfigParse.schema preserves the raw shape the user wrote - expect(Object.keys(config.permission as ConfigPermission.Info)).toEqual(["bash", "*", "edit"]) + expect(Object.keys(config.permission!)).toEqual(["bash", "*", "edit"]) try { ConfigParse.schema(Config.Info, { invalid_field: true }, "test") throw new Error("expected config parse to fail") @@ -2742,12 +2579,11 @@ test("parseManagedPlist parses permission rules", async () => { ), "test:mobileconfig", ) - const perm = config.permission as ConfigPermission.Info - expect(perm?.["*"]).toBe("ask") - expect(perm?.grep).toBe("allow") - expect(perm?.webfetch).toBe("ask") - expect(perm?.["~/.ssh/*"]).toBe("deny") - const bash = perm?.bash as Record + expect(config.permission?.["*"]).toBe("ask") + expect(config.permission?.grep).toBe("allow") + expect(config.permission?.webfetch).toBe("ask") + expect(config.permission?.["~/.ssh/*"]).toBe("deny") + const bash = config.permission?.bash as Record expect(bash?.["rm -rf *"]).toBe("deny") expect(bash?.["curl *"]).toBe("deny") }) diff --git a/packages/opencode/test/permission-task.test.ts b/packages/opencode/test/permission-task.test.ts index 77ceda8a4c..64b93bb8bc 100644 --- a/packages/opencode/test/permission-task.test.ts +++ b/packages/opencode/test/permission-task.test.ts @@ -1,7 +1,6 @@ import { afterEach, describe, test, expect } from "bun:test" import { Permission } from "../src/permission" import { Config } from "@/config/config" -import { ConfigPermission } from "@/config/permission" import { Instance } from "../src/project/instance" import { WithInstance } from "../src/project/with-instance" import { disposeAllInstances, tmpdir } from "./fixture/fixture" @@ -164,9 +163,7 @@ describe("permission.task with real config files", () => { directory: tmp.path, fn: async () => { const config = await load() - const ruleset = Permission.merge( - ...ConfigPermission.toLayers(config.permission).map((p) => Permission.fromConfig(p)), - ) + const ruleset = Permission.fromConfig(config.permission ?? {}) // general and orchestrator-fast should be allowed, code-reviewer denied expect(Permission.evaluate("task", "general", ruleset).action).toBe("allow") expect(Permission.evaluate("task", "orchestrator-fast", ruleset).action).toBe("allow") @@ -191,9 +188,7 @@ describe("permission.task with real config files", () => { directory: tmp.path, fn: async () => { const config = await load() - const ruleset = Permission.merge( - ...ConfigPermission.toLayers(config.permission).map((p) => Permission.fromConfig(p)), - ) + const ruleset = Permission.fromConfig(config.permission ?? {}) // general and code-reviewer should be ask, orchestrator-* denied expect(Permission.evaluate("task", "general", ruleset).action).toBe("ask") expect(Permission.evaluate("task", "code-reviewer", ruleset).action).toBe("ask") @@ -218,9 +213,7 @@ describe("permission.task with real config files", () => { directory: tmp.path, fn: async () => { const config = await load() - const ruleset = Permission.merge( - ...ConfigPermission.toLayers(config.permission).map((p) => Permission.fromConfig(p)), - ) + const ruleset = Permission.fromConfig(config.permission ?? {}) expect(Permission.evaluate("task", "general", ruleset).action).toBe("allow") expect(Permission.evaluate("task", "code-reviewer", ruleset).action).toBe("deny") // Unspecified agents default to "ask" @@ -247,9 +240,7 @@ describe("permission.task with real config files", () => { directory: tmp.path, fn: async () => { const config = await load() - const ruleset = Permission.merge( - ...ConfigPermission.toLayers(config.permission).map((p) => Permission.fromConfig(p)), - ) + const ruleset = Permission.fromConfig(config.permission ?? {}) // Verify task permissions expect(Permission.evaluate("task", "general", ruleset).action).toBe("allow") @@ -287,9 +278,7 @@ describe("permission.task with real config files", () => { directory: tmp.path, fn: async () => { const config = await load() - const ruleset = Permission.merge( - ...ConfigPermission.toLayers(config.permission).map((p) => Permission.fromConfig(p)), - ) + const ruleset = Permission.fromConfig(config.permission ?? {}) // Last matching rule wins - "*" deny is last, so all agents are denied expect(Permission.evaluate("task", "general", ruleset).action).toBe("deny") @@ -320,9 +309,7 @@ describe("permission.task with real config files", () => { directory: tmp.path, fn: async () => { const config = await load() - const ruleset = Permission.merge( - ...ConfigPermission.toLayers(config.permission).map((p) => Permission.fromConfig(p)), - ) + const ruleset = Permission.fromConfig(config.permission ?? {}) // Evaluate uses findLast - "general" allow comes after "*" deny expect(Permission.evaluate("task", "general", ruleset).action).toBe("allow")