From 65368f609d851a9e832d8dd176903df9a690a635 Mon Sep 17 00:00:00 2001 From: Andrew Suffield Date: Tue, 12 May 2026 18:09:06 -0400 Subject: [PATCH] fix: preserve permission ordering by accepting a layered array (#23214) Co-authored-by: Andrew Suffield Co-authored-by: Aiden Cline --- packages/opencode/src/agent/agent.ts | 6 +- packages/opencode/src/config/config.ts | 22 ++- 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, 221 insertions(+), 18 deletions(-) diff --git a/packages/opencode/src/agent/agent.ts b/packages/opencode/src/agent/agent.ts index 423a513180..74ca1a402b 100644 --- a/packages/opencode/src/agent/agent.ts +++ b/packages/opencode/src/agent/agent.ts @@ -1,4 +1,5 @@ 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" @@ -117,7 +118,10 @@ export const layer = Layer.effect( }, }) - const user = Permission.fromConfig(cfg.permission ?? {}) + // 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 agents: Record = { build: { diff --git a/packages/opencode/src/config/config.ts b/packages/opencode/src/config/config.ts index 545e48e64d..91eeab47e4 100644 --- a/packages/opencode/src/config/config.ts +++ b/packages/opencode/src/config/config.ts @@ -55,6 +55,13 @@ 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 } @@ -228,7 +235,12 @@ 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(ConfigPermission.Info), + 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.", + }), 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", @@ -704,11 +716,12 @@ export const layer = Layer.effect( } if (Flag.OPENCODE_PERMISSION) { - result.permission = mergeDeep(result.permission ?? {}, JSON.parse(Flag.OPENCODE_PERMISSION)) + const envPermission = JSON.parse(Flag.OPENCODE_PERMISSION) as ConfigPermission.Info + result.permission = [...ConfigPermission.toLayers(result.permission), envPermission] } if (result.tools) { - const perms: Record = {} + const perms: ConfigPermission.Info = {} for (const [tool, enabled] of Object.entries(result.tools)) { const action: ConfigPermission.Action = enabled ? "allow" : "deny" if (tool === "write" || tool === "edit" || tool === "patch") { @@ -717,7 +730,8 @@ export const layer = Layer.effect( } perms[tool] = action } - result.permission = mergeDeep(perms, result.permission ?? {}) + // Tools permissions come before other permissions (they can be overridden) + result.permission = [perms, ...ConfigPermission.toLayers(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 1092ae2b7e..c780d24436 100644 --- a/packages/opencode/src/config/permission.ts +++ b/packages/opencode/src/config/permission.ts @@ -56,3 +56,11 @@ 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 90e78efcdb..a2e4391777 100644 --- a/packages/opencode/test/config/config.test.ts +++ b/packages/opencode/test/config/config.test.ts @@ -3,7 +3,9 @@ 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" @@ -276,6 +278,40 @@ 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) => { @@ -1713,7 +1749,10 @@ test("permission config preserves user key order", async () => { directory: tmp.path, fn: async () => { const config = await load() - expect(Object.keys(config.permission!)).toEqual([ + // 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([ "*", "edit", "write", @@ -1729,6 +1768,129 @@ 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, @@ -1742,7 +1904,8 @@ test("config parser preserves permission order while rejecting unknown top-level "test", ) - expect(Object.keys(config.permission!)).toEqual(["bash", "*", "edit"]) + // ConfigParse.schema preserves the raw shape the user wrote + expect(Object.keys(config.permission as ConfigPermission.Info)).toEqual(["bash", "*", "edit"]) try { ConfigParse.schema(Config.Info, { invalid_field: true }, "test") throw new Error("expected config parse to fail") @@ -2579,11 +2742,12 @@ test("parseManagedPlist parses permission rules", async () => { ), "test:mobileconfig", ) - 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 + 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(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 64b93bb8bc..77ceda8a4c 100644 --- a/packages/opencode/test/permission-task.test.ts +++ b/packages/opencode/test/permission-task.test.ts @@ -1,6 +1,7 @@ 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" @@ -163,7 +164,9 @@ describe("permission.task with real config files", () => { directory: tmp.path, fn: async () => { const config = await load() - const ruleset = Permission.fromConfig(config.permission ?? {}) + const ruleset = Permission.merge( + ...ConfigPermission.toLayers(config.permission).map((p) => Permission.fromConfig(p)), + ) // 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") @@ -188,7 +191,9 @@ describe("permission.task with real config files", () => { directory: tmp.path, fn: async () => { const config = await load() - const ruleset = Permission.fromConfig(config.permission ?? {}) + const ruleset = Permission.merge( + ...ConfigPermission.toLayers(config.permission).map((p) => Permission.fromConfig(p)), + ) // 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") @@ -213,7 +218,9 @@ describe("permission.task with real config files", () => { directory: tmp.path, fn: async () => { const config = await load() - const ruleset = Permission.fromConfig(config.permission ?? {}) + const ruleset = Permission.merge( + ...ConfigPermission.toLayers(config.permission).map((p) => Permission.fromConfig(p)), + ) expect(Permission.evaluate("task", "general", ruleset).action).toBe("allow") expect(Permission.evaluate("task", "code-reviewer", ruleset).action).toBe("deny") // Unspecified agents default to "ask" @@ -240,7 +247,9 @@ describe("permission.task with real config files", () => { directory: tmp.path, fn: async () => { const config = await load() - const ruleset = Permission.fromConfig(config.permission ?? {}) + const ruleset = Permission.merge( + ...ConfigPermission.toLayers(config.permission).map((p) => Permission.fromConfig(p)), + ) // Verify task permissions expect(Permission.evaluate("task", "general", ruleset).action).toBe("allow") @@ -278,7 +287,9 @@ describe("permission.task with real config files", () => { directory: tmp.path, fn: async () => { const config = await load() - const ruleset = Permission.fromConfig(config.permission ?? {}) + const ruleset = Permission.merge( + ...ConfigPermission.toLayers(config.permission).map((p) => Permission.fromConfig(p)), + ) // Last matching rule wins - "*" deny is last, so all agents are denied expect(Permission.evaluate("task", "general", ruleset).action).toBe("deny") @@ -309,7 +320,9 @@ describe("permission.task with real config files", () => { directory: tmp.path, fn: async () => { const config = await load() - const ruleset = Permission.fromConfig(config.permission ?? {}) + const ruleset = Permission.merge( + ...ConfigPermission.toLayers(config.permission).map((p) => Permission.fromConfig(p)), + ) // Evaluate uses findLast - "general" allow comes after "*" deny expect(Permission.evaluate("task", "general", ruleset).action).toBe("allow")