fix(permission): disabled() hides tools with per-subagent allow rules (#401)

packages/opencode/src/permission/next.ts

@@ -257,15 +257,29 @@ export namespace PermissionNext {
 
   const EDIT_TOOLS = ["edit", "write", "patch", "multiedit"]
 
-export function disabled(tools: string[], ruleset: Ruleset): Set<string> {
+/** Determines which tools should be hidden from an agent's tool list.
+   * A tool is visible if ANY allow rule exists for its permission (any pattern),
+   * even if the wildcard default is deny. This handles configs like:
+   *   { "task": { "*": "deny", "ops": "allow" } }
+   * where the tool should be visible because at least one subagent is allowed.
+   * 
+   * Note: This only controls tool visibility. Runtime permission enforcement
+   * is handled by evaluate() + ask(), which check specific patterns.
+   * A tool may be visible but denied for a specific subagent at runtime. */
+  export function disabled(tools: string[], ruleset: Ruleset): Set<string> {
     const result = new Set<string>()
     for (const tool of tools) {
       const permission = EDIT_TOOLS.includes(tool) ? "edit" : tool
-      // Use evaluate() to check if the tool would be denied
-      // This ensures disabled() and evaluate() use the same logic
-      const rule = evaluate(permission, "*", ruleset)
-      if (rule.action === "deny") {
-        result.add(tool)
+      const wildcardResult = evaluate(permission, "*", ruleset)
+
+      if (wildcardResult.action === "deny") {
+        // Check for any non-wildcard allow rules before disabling
+        const hasSpecificAllow = ruleset.some(
+          (rule) => Wildcard.match(permission, rule.permission) &&
+                    rule.action === "allow" &&
+                    rule.pattern !== "*"
+        )
+        if (!hasSpecificAllow) result.add(tool)
       }
     }
     return result

packages/opencode/test/permission-task.test.ts

@@ -82,18 +82,23 @@ describe("PermissionNext.disabled for task tool", () => {
       action,
     }))
 
-  test("task tool is disabled when wildcard deny exists", () => {
-    // When "*": "deny" exists, the tool is disabled regardless of specific patterns
-    // Our implementation: wildcard deny disables the task tool
+  test("task tool is visible when wildcard deny has specific allow", () => {
+    // With the new semantic: tools are visible if ANY specific allow rule exists
+    // Config: { "*": "deny", "orchestrator-*": "allow" }
+    // Since "orchestrator-*": "allow" is a specific pattern (not "*"),
+    // the tool is visible (not disabled)
     const ruleset = createRuleset({
       "*": "deny",
       "orchestrator-*": "allow",
     })
     const disabled = PermissionNext.disabled(["task"], ruleset)
-    expect(disabled.has("task")).toBe(true)
+    expect(disabled.has("task")).toBe(false)
   })
 
-  test("task tool is disabled when wildcard deny exists (even with specific ask)", () => {
+  test("task tool is disabled when wildcard deny with no specific allow", () => {
+    // Config: { "*": "deny", "orchestrator-*": "ask" }
+    // Since "orchestrator-*": "ask" is not "allow", there's no specific allow rule
+    // So the tool IS disabled
     const ruleset = createRuleset({
       "*": "deny",
       "orchestrator-*": "ask",
@@ -125,19 +130,17 @@ describe("PermissionNext.disabled for task tool", () => {
     expect(disabled.has("task")).toBe(false)
   })
 
-  test("task tool IS disabled when specific allow comes before wildcard deny", () => {
-    // With pure last-match-wins, we must put specific patterns AFTER wildcard for last-match-wins behavior
-    // Config order: [{orchestrator-coder,allow}, {*,deny}]
-    // Backwards iteration: check index 1 first → "*" matches → deny
-    // disabled() uses evaluate(permission, "*", ruleset) - evaluates with pattern "*"
-    // "task" doesn't match "orchestrator-coder" exactly, so it falls through to "*"
+  test("task tool is visible when wildcard deny has specific allow", () => {
+    // With the new semantic: tools are visible if ANY specific allow rule exists
+    // Config: { "*": "deny", "orchestrator-coder": "allow" }
+    // Since "orchestrator-coder": "allow" is a specific pattern (not "*"),
+    // the tool is visible (not disabled)
     const ruleset = createRuleset({
       "*": "deny",
       "orchestrator-coder": "allow",
     })
     const disabled = PermissionNext.disabled(["task"], ruleset)
-    // With pure last-match-wins, "*" is last in the array (index 1), so it wins
-    expect(disabled.has("task")).toBe(true)
+    expect(disabled.has("task")).toBe(false)
   })
 })
 
@@ -256,11 +259,13 @@ test("mixed permission config with task and other tools", async () => {
         expect(PermissionNext.evaluate("edit", "*", ruleset).action).toBe("ask")
 
         // Verify disabled tools
+        // Config: task: { "*": "deny", general: "allow" }
+        // Since "general": "allow" is a specific pattern (not "*"),
+        // the task tool is visible (not disabled)
         const disabled = PermissionNext.disabled(["bash", "edit", "task"], ruleset)
         expect(disabled.has("bash")).toBe(false)
         expect(disabled.has("edit")).toBe(false)
-        // disabled() evaluates with pattern "*", "task" matches "*" deny rule → disabled
-        expect(disabled.has("task")).toBe(true)
+        expect(disabled.has("task")).toBe(false)
       },
     })
   })
@@ -296,7 +301,7 @@ test("mixed permission config with task and other tools", async () => {
     })
   })
 
-  test("task tool IS disabled when wildcard deny has last-match priority", async () => {
+  test("task tool is visible when wildcard deny has specific allow", async () => {
     await using tmp = await tmpdir({
       git: true,
       config: {
@@ -315,13 +320,14 @@ test("mixed permission config with task and other tools", async () => {
         const ruleset = PermissionNext.fromConfig(config.permission ?? {})
 
         // With pure last-match-wins, "*" deny is last matching rule, so it wins
+        // This affects runtime evaluation, not visibility
         expect(PermissionNext.evaluate("task", "general", ruleset).action).toBe("deny")
         expect(PermissionNext.evaluate("task", "code-reviewer", ruleset).action).toBe("deny")
 
-        // disabled() uses findLast() - last match is {pattern: "*", action: "deny"}
-        // So the task tool IS disabled
+        // disabled() checks for any specific allow pattern (not "*")
+        // Since "general": "allow" exists, the tool is visible (not disabled)
         const disabled = PermissionNext.disabled(["task"], ruleset)
-        expect(disabled.has("task")).toBe(true)
+        expect(disabled.has("task")).toBe(false)
       },
     })
   })

packages/opencode/test/permission/next.test.ts

@@ -13,7 +13,7 @@ test("fromConfig - string value becomes wildcard rule", () => {
 
 test("fromConfig - object value converts to rules array", () => {
   const result = PermissionNext.fromConfig({ bash: { "*": "allow", rm: "deny" } })
-  // With findLast() behavior, specific patterns come AFTER wildcards to override them
+  // With last-match-wins behavior, specific patterns come AFTER wildcards to override them
   expect(result).toEqual([
     { permission: "bash", pattern: "*", action: "allow" },  // 1 wildcard
     { permission: "bash", pattern: "rm", action: "deny" },  // 0 wildcards
@@ -26,7 +26,7 @@ test("fromConfig - mixed string and object values", () => {
     edit: "allow",
     webfetch: "ask",
   })
-  // With findLast() behavior, specific patterns come AFTER wildcards to override them
+  // With last-match-wins behavior, specific patterns come AFTER wildcards to override them
   expect(result).toEqual([
     { permission: "bash", pattern: "*", action: "allow" },  // 0 fixed chars
     { permission: "bash", pattern: "rm", action: "deny" },  // 2 fixed chars
@@ -134,7 +134,7 @@ test("merge - empty ruleset does nothing", () => {
   expect(result).toEqual([{ permission: "bash", pattern: "*", action: "allow" }])
 })
 
-test("merge - preserves order for findLast() precedence", () => {
+test("merge - preserves order for last-match-wins precedence", () => {
   const result = PermissionNext.merge(
     [
       { permission: "edit", pattern: "src/*", action: "allow" },
@@ -317,7 +317,7 @@ test("evaluate - specific permission comes after wildcard", () => {
     { permission: "*", pattern: "*", action: "deny" },
     { permission: "bash", pattern: "*", action: "allow" },
   ])
-  // With findLast(), the last matching rule ("bash" allow) takes precedence
+  // With last-match-wins, the last matching rule ("bash" allow) takes precedence
   expect(result.action).toBe("allow")
 })
 
@@ -326,7 +326,7 @@ test("evaluate - wildcard permission before specific allow", () => {
     { permission: "*", pattern: "*", action: "deny" },
     { permission: "edit", pattern: "src/*", action: "allow" },
   ])
-  // With findLast(), the last matching rule ("edit" allow) takes precedence
+  // With last-match-wins, the last matching rule ("edit" allow) takes precedence
   expect(result.action).toBe("allow")
 })
 
@@ -422,19 +422,16 @@ test("disabled - does not disable when action is ask", () => {
   expect(result.size).toBe(0)
 })
 
-test("disabled - does not disable when wildcard deny comes before specific allow", () => {
-  // With findLast(), both disabled() and evaluate() use last-match precedence
-  // If wildcard deny comes first, tool is NOT disabled because findLast() ignores it
-  // This ensures disabled() and evaluate() are consistent - no security vulnerability
+test("disabled - does not disable when wildcard deny before specific allow", () => {
   const result = PermissionNext.disabled(
     ["bash"],
     [
       { permission: "bash", pattern: "*", action: "deny" },
       { permission: "bash", pattern: "echo *", action: "allow" },
     ],
   )
-  // With findLast(), the last matching rule is the wildcard deny, so bash IS disabled
-  expect(result.has("bash")).toBe(true)
+  // Tool should NOT be disabled because there is an allow rule for this permission
+  expect(result.has("bash")).toBe(false)
 })
 
 test("disabled - does not disable when specific deny before wildcard allow", () => {
@@ -445,7 +442,7 @@ test("disabled - does not disable when specific deny before wildcard allow", ()
       { permission: "bash", pattern: "*", action: "allow" },
     ],
   )
-  // With findLast(), the last matching rule is wildcard allow, so bash is NOT disabled
+  // With last-match-wins, the last matching rule is wildcard allow, so bash is NOT disabled
   expect(result.has("bash")).toBe(false)
 })
 
@@ -472,21 +469,114 @@ test("disabled - wildcard permission denies all tools", () => {
 
 test("disabled - specific allow overrides wildcard deny", () => {
   // With wildcard matching, "*" matches everything including "bash", "edit", "read"
-  // But with findLast(), the last matching rule ({ bash, *, allow }) overrides the wildcard deny
+  // The last matching rule ({ bash, *, allow }) overrides the wildcard deny
   const result = PermissionNext.disabled(
     ["bash", "edit", "read"],
     [
       { permission: "*", pattern: "*", action: "deny" },
       { permission: "bash", pattern: "*", action: "allow" },
     ],
   )
-  // With findLast(), bash is NOT disabled because the last matching rule for bash is the wildcard allow
+  // bash is NOT disabled because the last matching rule for bash is the wildcard allow
   expect(result.has("bash")).toBe(false)
   // edit and read ARE disabled because their last matching rule is the wildcard deny
   expect(result.has("edit")).toBe(true)
   expect(result.has("read")).toBe(true)
 })
 
+test("disabled - tool with per-subagent deny/allow is visible", () => {
+  // Tests the exact bug from issue #401
+  const result = PermissionNext.disabled(
+    ["task"],
+    PermissionNext.fromConfig({
+      task: { "*": "deny", "ops": "allow", "developer": "allow" },
+    }),
+  )
+  // Tool should be visible because at least one subagent (ops, developer) is allowed
+  expect(result.has("task")).toBe(false)
+})
+
+test("disabled - tool with deny-only config is hidden", () => {
+  const result = PermissionNext.disabled(
+    ["task"],
+    PermissionNext.fromConfig({
+      task: { "*": "deny" },
+    }),
+  )
+  // Tool should be hidden because no subagent is allowed
+  expect(result.has("task")).toBe(true)
+})
+
+test("disabled - edit tool with deny and glob allow is visible", () => {
+  const result = PermissionNext.disabled(
+    ["edit"],
+    [
+      { permission: "edit", pattern: "*", action: "deny" },
+      { permission: "edit", pattern: "src/*", action: "allow" },
+    ],
+  )
+  // Edit should be visible because src/* allow exists
+  expect(result.has("edit")).toBe(false)
+})
+
+test("disabled - consistency with evaluate for wildcard deny-only", () => {
+  // If disabled() says tool is disabled, evaluate() must also deny
+  const ruleset = PermissionNext.fromConfig({
+    task: { "*": "deny" },
+  })
+  const disabled = PermissionNext.disabled(["task"], ruleset)
+  const evalResult = PermissionNext.evaluate("task", "*", ruleset)
+  expect(disabled.has("task")).toBe(true)
+  expect(evalResult.action).toBe("deny")
+  // Verify consistency
+  expect(disabled.has("task")).toBe(evalResult.action === "deny")
+})
+
+test("disabled - consistency with evaluate for per-subagent allow", () => {
+  // If disabled() says tool is NOT disabled, evaluate() should allow for at least one pattern
+  const ruleset = PermissionNext.fromConfig({
+    task: { "*": "deny", "ops": "allow" },
+  })
+  const disabled = PermissionNext.disabled(["task"], ruleset)
+  const evalForWildcard = PermissionNext.evaluate("task", "*", ruleset)
+  const evalForOps = PermissionNext.evaluate("task", "ops", ruleset)
+  expect(disabled.has("task")).toBe(false)
+  expect(evalForWildcard.action).toBe("deny") // default is deny
+  expect(evalForOps.action).toBe("allow") // ops specifically allowed
+})
+
+test("disabled - tool with all deny rules regardless of pattern", () => {
+  const result = PermissionNext.disabled(
+    ["bash"],
+    [
+      { permission: "bash", pattern: "*", action: "deny" },
+      { permission: "bash", pattern: "rm", action: "deny" },
+    ],
+  )
+  // All rules are deny, tool should be disabled
+  expect(result.has("bash")).toBe(true)
+})
+
+test("disabled - wildcard permission collision: visible but denied at runtime", () => {
+  // A tool with a specific allow rule but wildcard permission deny:
+  // disabled() shows the tool (hasAllow = true), but evaluate() denies for
+  // patterns not specifically allowed. This is by design — visibility shows
+  // "this tool can be used for SOME subagent", enforcement checks per-action.
+  // Note: Order matters for last-match-wins - wildcard must come before specific rules
+  const ruleset: PermissionNext.Ruleset = [
+    { permission: "*", pattern: "*", action: "deny" },
+    { permission: "task", pattern: "*", action: "deny" },
+    { permission: "task", pattern: "ops", action: "allow" },
+  ]
+  const disabled = PermissionNext.disabled(["task"], ruleset)
+  // Tool is visible because task+ops allow rule exists
+  expect(disabled.has("task")).toBe(false)
+  // But evaluate() denies for non-ops subagents
+  expect(PermissionNext.evaluate("task", "developer", ruleset).action).toBe("deny")
+  // And evaluate() allows for ops specifically
+  expect(PermissionNext.evaluate("task", "ops", ruleset).action).toBe("allow")
+})
+
 test("disabled and evaluate consistency - Security check", () => {
   // This test ensures disabled() and evaluate() use the same matching logic
   // Both prioritize exact pattern matches over wildcard pattern matches
@@ -541,7 +631,7 @@ test("disabled and evaluate consistency - wildcard permission first", () => {
   const disabled = PermissionNext.disabled(["bash"], ruleset)
   const evalResult = PermissionNext.evaluate("bash", "rm", ruleset)
 
-  // With findLast(), last match is bash allow
+  // With last-match-wins, last match is bash allow
   expect(disabled.has("bash")).toBe(false) // bash is NOT disabled
   expect(evalResult.action).toBe("allow") // bash is allowed