diff --git a/packages/core/src/permission.ts b/packages/core/src/permission.ts index ec8038f713..b4738abe2b 100644 --- a/packages/core/src/permission.ts +++ b/packages/core/src/permission.ts @@ -19,15 +19,7 @@ export type Ruleset = typeof Ruleset.Type const EDIT_TOOLS = ["edit", "write", "apply_patch"] export function evaluate(permission: string, pattern: string, ...rulesets: Ruleset[]): Rule { - return ( - rulesets - .flat() - .findLast((rule) => Wildcard.match(permission, rule.permission) && Wildcard.match(pattern, rule.pattern)) ?? { - action: "ask", - permission, - pattern: "*", - } - ) + return select(permission, pattern, rulesets.flat())?.rule ?? { action: "ask", permission, pattern: "*" } } export function merge(...rulesets: Ruleset[]): Ruleset { @@ -38,8 +30,52 @@ export function disabled(tools: string[], ruleset: Ruleset): Set { return new Set( tools.filter((tool) => { const permission = EDIT_TOOLS.includes(tool) ? "edit" : tool - const rule = ruleset.findLast((rule) => Wildcard.match(permission, rule.permission)) - return rule?.pattern === "*" && rule.action === "deny" + if ( + ruleset.some( + (rule) => Wildcard.match(permission, rule.permission) && rule.pattern !== "*" && rule.action !== "deny", + ) + ) { + return false + } + return evaluate(permission, "*", ruleset).action === "deny" }), ) } + +function select(permission: string, pattern: string, ruleset: Ruleset) { + return ruleset.reduce((best, rule, index) => { + if (!Wildcard.match(permission, rule.permission) || !Wildcard.match(pattern, rule.pattern)) return best + const next = { rule, index, permission: specificity(rule.permission), pattern: specificity(rule.pattern) } + if (!best) return next + return compare(next, best) >= 0 ? next : best + }, undefined) +} + +type Selected = { + rule: Rule + index: number + permission: Specificity + pattern: Specificity +} + +type Specificity = { + wildcard: number + literal: number +} + +function specificity(pattern: string): Specificity { + return { + wildcard: [...pattern.matchAll(/[?*]/g)].length, + literal: pattern.replace(/[?*]/g, "").length, + } +} + +function compare(a: Selected, b: Selected) { + return ( + b.permission.wildcard - a.permission.wildcard || + a.permission.literal - b.permission.literal || + b.pattern.wildcard - a.pattern.wildcard || + a.pattern.literal - b.pattern.literal || + a.index - b.index + ) +} diff --git a/packages/opencode/test/permission-task.test.ts b/packages/opencode/test/permission-task.test.ts index 1ee8b5488e..27f5bea79c 100644 --- a/packages/opencode/test/permission-task.test.ts +++ b/packages/opencode/test/permission-task.test.ts @@ -55,7 +55,7 @@ describe("Permission.evaluate for permission.task", () => { expect(Permission.evaluate("task", "code-reviewer", globalRuleset).action).toBe("ask") }) - test("later rules take precedence (last match wins)", () => { + test("more specific rules take precedence", () => { const ruleset = createRuleset({ "orchestrator-*": "deny", "orchestrator-fast": "allow", @@ -73,8 +73,9 @@ describe("Permission.evaluate for permission.task", () => { describe("Permission.disabled for task tool", () => { // Note: The `disabled` function checks if a TOOL should be completely removed from the tool list. - // It only disables a tool when there's a rule with `pattern: "*"` and `action: "deny"`. - // It does NOT evaluate complex subagent patterns - those are handled at runtime by `evaluate`. + // It only disables a tool when every possible call is denied. Specific allow + // or ask patterns keep the tool available; runtime evaluation handles the + // individual subagent patterns. const createRuleset = (rules: Record): Permission.Ruleset => Object.entries(rules).map(([pattern, action]) => ({ permission: "task", @@ -82,26 +83,22 @@ describe("Permission.disabled for task tool", () => { action, })) - test("task tool is disabled when global deny pattern exists (even with specific allows)", () => { - // When "*": "deny" exists, the task tool is disabled because the disabled() function - // only checks for wildcard deny patterns - it doesn't consider that specific subagents might be allowed + test("task tool is not disabled when global deny has specific allows", () => { const ruleset = createRuleset({ "orchestrator-*": "allow", "*": "deny", }) const disabled = Permission.disabled(["task", "bash", "read"], ruleset) - // The task tool IS disabled because there's a pattern: "*" with action: "deny" - expect(disabled.has("task")).toBe(true) + expect(disabled.has("task")).toBe(false) }) - test("task tool is disabled when global deny pattern exists (even with ask overrides)", () => { + test("task tool is not disabled when global deny has specific asks", () => { const ruleset = createRuleset({ "orchestrator-*": "ask", "*": "deny", }) const disabled = Permission.disabled(["task"], ruleset) - // The task tool IS disabled because there's a pattern: "*" with action: "deny" - expect(disabled.has("task")).toBe(true) + expect(disabled.has("task")).toBe(false) }) test("task tool is disabled when global deny pattern exists", () => { @@ -111,14 +108,12 @@ describe("Permission.disabled for task tool", () => { }) test("task tool is NOT disabled when only specific patterns are denied (no wildcard)", () => { - // The disabled() function only disables tools when pattern: "*" && action: "deny" - // Specific subagent denies don't disable the task tool - those are handled at runtime + // Specific subagent denies don't disable the task tool - those are handled at runtime. const ruleset = createRuleset({ "orchestrator-*": "deny", general: "deny", }) const disabled = Permission.disabled(["task"], ruleset) - // The task tool is NOT disabled because no rule has pattern: "*" with action: "deny" expect(disabled.has("task")).toBe(false) }) @@ -127,16 +122,12 @@ describe("Permission.disabled for task tool", () => { expect(disabled.has("task")).toBe(false) }) - test("task tool is NOT disabled when last wildcard pattern is allow", () => { - // Last matching rule wins - if wildcard allow comes after wildcard deny, tool is enabled + test("task tool is NOT disabled when wildcard deny has a specific allow", () => { const ruleset = createRuleset({ "*": "deny", "orchestrator-coder": "allow", }) const disabled = Permission.disabled(["task"], ruleset) - // The disabled() function uses findLast and checks if the last matching rule - // has pattern: "*" and action: "deny". In this case, the last rule matching - // "task" permission has pattern "orchestrator-coder", not "*", so not disabled expect(disabled.has("task")).toBe(false) }) }) @@ -234,8 +225,7 @@ describe("permission.task with real config files", () => { const disabled = Permission.disabled(["bash", "edit", "task"], ruleset) expect(disabled.has("bash")).toBe(false) expect(disabled.has("edit")).toBe(false) - // task is NOT disabled because disabled() uses findLast, and the last rule - // matching "task" permission is {pattern: "general", action: "allow"}, not pattern: "*" + // task is NOT disabled because the specific allow leaves at least one subagent available. expect(disabled.has("task")).toBe(false) }), { @@ -254,21 +244,18 @@ describe("permission.task with real config files", () => { ) it.instance( - "task tool disabled when global deny comes last in config", + "specific task allows beat global deny regardless of order", () => Effect.gen(function* () { const config = yield* load 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") - expect(Permission.evaluate("task", "code-reviewer", ruleset).action).toBe("deny") + expect(Permission.evaluate("task", "general", ruleset).action).toBe("allow") + expect(Permission.evaluate("task", "code-reviewer", ruleset).action).toBe("allow") expect(Permission.evaluate("task", "unknown", ruleset).action).toBe("deny") - // Since "*": "deny" is the last rule, disabled() finds it with findLast - // and sees pattern: "*" with action: "deny", so task is disabled const disabled = Permission.disabled(["task"], ruleset) - expect(disabled.has("task")).toBe(true) + expect(disabled.has("task")).toBe(false) }), { git: true, @@ -285,20 +272,17 @@ describe("permission.task with real config files", () => { ) it.instance( - "task tool NOT disabled when specific allow comes last in config", + "task tool NOT disabled when global deny has a specific allow", () => Effect.gen(function* () { const config = yield* load const ruleset = Permission.fromConfig(config.permission ?? {}) - // Evaluate uses findLast - "general" allow comes after "*" deny expect(Permission.evaluate("task", "general", ruleset).action).toBe("allow") // Other agents still denied by the earlier "*" deny expect(Permission.evaluate("task", "code-reviewer", ruleset).action).toBe("deny") - // disabled() uses findLast and checks if the last rule has pattern: "*" with action: "deny" - // In this case, the last rule is {pattern: "general", action: "allow"}, not pattern: "*" - // So the task tool is NOT disabled (even though most subagents are denied) + // The task tool remains available because the specific allow leaves one subagent callable. const disabled = Permission.disabled(["task"], ruleset) expect(disabled.has("task")).toBe(false) }), diff --git a/packages/opencode/test/permission/next.test.ts b/packages/opencode/test/permission/next.test.ts index 1b09c36afd..201f60edb0 100644 --- a/packages/opencode/test/permission/next.test.ts +++ b/packages/opencode/test/permission/next.test.ts @@ -129,9 +129,8 @@ test("fromConfig - does not expand tilde in middle of path", () => { expect(result).toEqual([{ permission: "external_directory", pattern: "/some/~/path", action: "allow" }]) }) -// Permission precedence follows config insertion order. `evaluate()` uses the -// last matching rule, so later config entries intentionally override earlier -// entries even when a wildcard appears after a specific permission. +// Permission precedence follows specificity. Later config entries only override +// earlier entries when the matching rules are equally specific. test("fromConfig - preserves top-level config key order", () => { const wildcardFirst = Permission.fromConfig({ "*": "deny", bash: "allow" }) @@ -141,7 +140,7 @@ test("fromConfig - preserves top-level config key order", () => { expect(specificFirst.map((r) => r.permission)).toEqual(["bash", "*"]) expect(Permission.evaluate("bash", "ls", wildcardFirst).action).toBe("allow") - expect(Permission.evaluate("bash", "ls", specificFirst).action).toBe("deny") + expect(Permission.evaluate("bash", "ls", specificFirst).action).toBe("allow") }) test("fromConfig - wildcard acts as fallback when it appears before specifics", () => { @@ -288,7 +287,7 @@ test("evaluate - wildcard pattern match", () => { expect(result.action).toBe("allow") }) -test("evaluate - last matching rule wins", () => { +test("evaluate - more specific pattern wins over wildcard", () => { const result = Permission.evaluate("bash", "rm", [ { permission: "bash", pattern: "*", action: "allow" }, { permission: "bash", pattern: "rm", action: "deny" }, @@ -296,12 +295,12 @@ test("evaluate - last matching rule wins", () => { expect(result.action).toBe("deny") }) -test("evaluate - last matching rule wins (wildcard after specific)", () => { +test("evaluate - specific pattern wins when wildcard appears later", () => { const result = Permission.evaluate("bash", "rm", [ { permission: "bash", pattern: "rm", action: "deny" }, { permission: "bash", pattern: "*", action: "allow" }, ]) - expect(result.action).toBe("allow") + expect(result.action).toBe("deny") }) test("evaluate - glob pattern match", () => { @@ -309,7 +308,7 @@ test("evaluate - glob pattern match", () => { expect(result.action).toBe("allow") }) -test("evaluate - last matching glob wins", () => { +test("evaluate - more specific glob wins", () => { const result = Permission.evaluate("edit", "src/components/Button.tsx", [ { permission: "edit", pattern: "src/*", action: "deny" }, { permission: "edit", pattern: "src/components/*", action: "allow" }, @@ -317,12 +316,12 @@ test("evaluate - last matching glob wins", () => { expect(result.action).toBe("allow") }) -test("evaluate - order matters for specificity", () => { +test("evaluate - specific glob wins when broader glob appears later", () => { const result = Permission.evaluate("edit", "src/components/Button.tsx", [ { permission: "edit", pattern: "src/components/*", action: "allow" }, { permission: "edit", pattern: "src/*", action: "deny" }, ]) - expect(result.action).toBe("deny") + expect(result.action).toBe("allow") }) test("evaluate - unknown permission returns ask", () => { @@ -373,12 +372,12 @@ test("evaluate - exact match at end wins over earlier wildcard", () => { expect(result.action).toBe("deny") }) -test("evaluate - wildcard at end overrides earlier exact match", () => { +test("evaluate - earlier exact match wins over wildcard at end", () => { const result = Permission.evaluate("bash", "/bin/rm", [ { permission: "bash", pattern: "/bin/rm", action: "deny" }, { permission: "bash", pattern: "*", action: "allow" }, ]) - expect(result.action).toBe("allow") + expect(result.action).toBe("deny") }) // wildcard permission tests @@ -433,12 +432,32 @@ test("evaluate - wildcard permission fallback for unknown tool", () => { expect(result.action).toBe("ask") }) -test("evaluate - later wildcard permission can override earlier specific permission", () => { +test("evaluate - specific permission wins over later wildcard permission", () => { const result = Permission.evaluate("bash", "rm", [ { permission: "bash", pattern: "*", action: "allow" }, { permission: "*", pattern: "*", action: "deny" }, ]) - expect(result.action).toBe("deny") + expect(result.action).toBe("allow") +}) + +test("evaluate - bash deny beats wildcard ask regardless of order", () => { + const wildcardFirst = Permission.fromConfig({ + bash: { + "*": "ask", + "git checkout": "deny", + "git checkout *": "deny", + }, + }) + const wildcardLast = Permission.fromConfig({ + bash: { + "git checkout": "deny", + "git checkout *": "deny", + "*": "ask", + }, + }) + + expect(Permission.evaluate("bash", "git checkout -- backend/db/schema.py", wildcardFirst).action).toBe("deny") + expect(Permission.evaluate("bash", "git checkout -- backend/db/schema.py", wildcardLast).action).toBe("deny") }) test("evaluate - merges multiple rulesets", () => { diff --git a/packages/web/src/content/docs/agents.mdx b/packages/web/src/content/docs/agents.mdx index 53048b7927..7cdf838f49 100644 --- a/packages/web/src/content/docs/agents.mdx +++ b/packages/web/src/content/docs/agents.mdx @@ -535,7 +535,7 @@ This can take a glob pattern. ``` And you can also use the `*` wildcard to manage permissions for all commands. -Since the last matching rule takes precedence, put the `*` wildcard first and specific rules after. +The most specific matching rule takes precedence, so specific command rules beat the `*` wildcard regardless of order. ```json title="opencode.json" {8} { @@ -622,7 +622,7 @@ Control which subagents an agent can invoke via the Task tool with `permission.t When set to `deny`, the subagent is removed from the Task tool description entirely, so the model won't attempt to invoke it. :::tip -Rules are evaluated in order, and the **last matching rule wins**. In the example above, `orchestrator-planner` matches both `*` (deny) and `orchestrator-*` (allow), but since `orchestrator-*` comes after `*`, the result is `allow`. +Rules are evaluated by specificity. In the example above, `orchestrator-planner` matches both `*` (deny) and `orchestrator-*` (allow), but `orchestrator-*` is more specific, so the result is `allow`. ::: :::tip diff --git a/packages/web/src/content/docs/permissions.mdx b/packages/web/src/content/docs/permissions.mdx index 19182e2749..d70692f650 100644 --- a/packages/web/src/content/docs/permissions.mdx +++ b/packages/web/src/content/docs/permissions.mdx @@ -68,7 +68,7 @@ For most permissions, you can use an object to apply different actions based on } ``` -Rules are evaluated by pattern match, with the **last matching rule winning**. A common pattern is to put the catch-all `"*"` rule first, and more specific rules after it. +Rules are evaluated by pattern match, with the **most specific matching rule winning**. If two matching rules are equally specific, the later rule wins. ### Wildcards