Browse Source

fix(core): prefer specific permission rules

Shoubhit Dash 2 months ago
parent
commit
5530bbe0ac

+ 47 - 11
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<string> {
   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<Selected | undefined>((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
+  )
+}

+ 17 - 33
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<string, "allow" | "deny" | "ask">): 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)
       }),

+ 33 - 14
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", () => {

+ 2 - 2
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

+ 1 - 1
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