fix: config ordering issue

This commit is contained in:
Aiden Cline 2026-04-24 22:59:33 -04:00
parent 5cd178ba70
commit bcb37c0d91
3 changed files with 18 additions and 34 deletions

View file

@ -288,18 +288,8 @@ function expand(pattern: string): string {
} }
export function fromConfig(permission: ConfigPermission.Info) { export function fromConfig(permission: ConfigPermission.Info) {
// Sort top-level keys so wildcard permissions (`*`, `mcp_*`) come before
// specific ones. Combined with `findLast` in evaluate(), this gives the
// intuitive semantic "specific tool rules override the `*` fallback"
// regardless of the user's JSON key order. Sub-pattern order inside a
// single permission key is preserved — only top-level keys are sorted.
const entries = Object.entries(permission).sort(([a], [b]) => {
const aWild = a.includes("*")
const bWild = b.includes("*")
return aWild === bWild ? 0 : aWild ? -1 : 1
})
const ruleset: Ruleset = [] const ruleset: Ruleset = []
for (const [key, value] of entries) { for (const [key, value] of Object.entries(permission)) {
if (typeof value === "string") { if (typeof value === "string") {
ruleset.push({ permission: key, action: value, pattern: "*" }) ruleset.push({ permission: key, action: value, pattern: "*" })
continue continue

View file

@ -1501,10 +1501,8 @@ test("permission config canonicalises known keys first, preserves rest-key inser
// todowrite, external_directory are declared in `config/permission.ts`), // todowrite, external_directory are declared in `config/permission.ts`),
// followed by rest keys in the user's insertion order. // followed by rest keys in the user's insertion order.
// //
// Rule precedence is NOT affected by this reordering: `Permission.fromConfig` // Permission.fromConfig preserves this effective order, and permission
// sorts wildcards before specifics before iterating. See the // evaluation uses last-match-wins precedence.
// "fromConfig - specific key beats wildcard regardless of JSON key order"
// test in test/permission/next.test.ts for the behavioural guarantee.
await using tmp = await tmpdir({ await using tmp = await tmpdir({
init: async (dir) => { init: async (dir) => {
await Filesystem.write( await Filesystem.write(

View file

@ -130,49 +130,45 @@ test("fromConfig - does not expand tilde in middle of path", () => {
// Top-level wildcard-vs-specific precedence semantics. // Top-level wildcard-vs-specific precedence semantics.
// //
// fromConfig sorts top-level keys so wildcard permissions (containing "*") // fromConfig preserves top-level key order. Combined with `findLast` in
// come before specific permissions. Combined with `findLast` in evaluate(), // evaluate(), later matching rules win.
// this gives the intuitive semantic "specific tool rules override the `*`
// fallback", regardless of the order the user wrote the keys in their JSON.
// //
// Sub-pattern order inside a single permission key (e.g. `bash: { "*": "allow", "rm": "deny" }`) // Sub-pattern order inside a single permission key (e.g. `bash: { "*": "allow", "rm": "deny" }`)
// still depends on insertion order — only top-level keys are sorted. // also depends on insertion order.
test("fromConfig - specific key beats wildcard regardless of JSON key order", () => { test("fromConfig - top-level key order controls wildcard precedence", () => {
const wildcardFirst = Permission.fromConfig({ "*": "deny", bash: "allow" }) const wildcardFirst = Permission.fromConfig({ "*": "deny", bash: "allow" })
const specificFirst = Permission.fromConfig({ bash: "allow", "*": "deny" }) const specificFirst = Permission.fromConfig({ bash: "allow", "*": "deny" })
// Both orderings produce the same ruleset
expect(wildcardFirst).toEqual(specificFirst)
// And both evaluate bash → allow (bash rule wins over * fallback)
expect(Permission.evaluate("bash", "ls", wildcardFirst).action).toBe("allow") expect(Permission.evaluate("bash", "ls", wildcardFirst).action).toBe("allow")
expect(Permission.evaluate("bash", "ls", specificFirst).action).toBe("allow") expect(Permission.evaluate("bash", "ls", specificFirst).action).toBe("deny")
})
test("fromConfig - regression: trailing wildcard overrides earlier specific rule", () => {
const ruleset = Permission.fromConfig({ bash: "ask", "*": "allow" })
expect(Permission.evaluate("bash", "glab", ruleset).action).toBe("allow")
}) })
test("fromConfig - wildcard acts as fallback for permissions with no specific rule", () => { test("fromConfig - wildcard acts as fallback for permissions with no specific rule", () => {
const ruleset = Permission.fromConfig({ bash: "allow", "*": "ask" }) const ruleset = Permission.fromConfig({ "*": "ask", bash: "allow" })
expect(Permission.evaluate("edit", "foo.ts", ruleset).action).toBe("ask") expect(Permission.evaluate("edit", "foo.ts", ruleset).action).toBe("ask")
expect(Permission.evaluate("bash", "ls", ruleset).action).toBe("allow") expect(Permission.evaluate("bash", "ls", ruleset).action).toBe("allow")
}) })
test("fromConfig - top-level ordering: wildcards first, specifics after", () => { test("fromConfig - preserves top-level ordering", () => {
const ruleset = Permission.fromConfig({ const ruleset = Permission.fromConfig({
bash: "allow", bash: "allow",
"*": "ask", "*": "ask",
edit: "deny", edit: "deny",
"mcp_*": "allow", "mcp_*": "allow",
}) })
// wildcards (* and mcp_*) come before specifics (bash, edit) expect(ruleset.map((r) => r.permission)).toEqual(["bash", "*", "edit", "mcp_*"])
const permissions = ruleset.map((r) => r.permission)
expect(permissions.slice(0, 2).sort()).toEqual(["*", "mcp_*"])
expect(permissions.slice(2)).toEqual(["bash", "edit"])
}) })
test("fromConfig - sub-pattern insertion order inside a tool key is preserved (only top-level sorts)", () => { test("fromConfig - sub-pattern insertion order inside a tool key is preserved", () => {
// Sub-patterns within a single tool key use the documented "`*` first, // Sub-patterns within a single tool key use the documented "`*` first,
// specific patterns after" convention (findLast picks specifics). The // specific patterns after" convention (findLast picks specifics). The
// top-level sort must not touch sub-pattern ordering. // top-level order must not affect sub-pattern ordering.
const ruleset = Permission.fromConfig({ bash: { "*": "deny", "git *": "allow" } }) const ruleset = Permission.fromConfig({ bash: { "*": "deny", "git *": "allow" } })
expect(ruleset.map((r) => r.pattern)).toEqual(["*", "git *"]) expect(ruleset.map((r) => r.pattern)).toEqual(["*", "git *"])
// * fallback for unknown commands // * fallback for unknown commands