From 24bdaa6ca0333b91369ac50b23e929f83af01c3a Mon Sep 17 00:00:00 2001 From: Adam Malczewski Date: Wed, 3 Jun 2026 16:33:50 +0900 Subject: fix(config): emit local permission patterns after global ones in merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gemini review caught a precedence-inversion bug in mergePermissions: when a nested permission group exists in BOTH global and local configs, the previous `{ ...existing, ...value }` spread updated an overridden pattern IN PLACE, leaving its original (global) insertion slot. Since configToRuleset flattens patterns in iteration order and evaluate() uses findLast (last match wins), a more-general global pattern declared lower (e.g. "*") would sit AFTER the local override and silently shadow it. Example: global bash { "npm test"=allow, "*"=ask } + local bash { "npm test"=deny } resolved "npm test" to "ask" instead of "deny". Fix: drop global patterns the local block also defines, keep remaining global patterns in order, then append ALL local patterns last — reproducing a clean "global rules then local rules" concatenation so local always wins. Adds a regression test asserting order and evaluation outcome. --- packages/core/src/config/loader.ts | 37 +++++++++++++++++++++++++------- packages/core/tests/config/merge.test.ts | 17 +++++++++++++++ 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/packages/core/src/config/loader.ts b/packages/core/src/config/loader.ts index 1e5f151..66f798b 100644 --- a/packages/core/src/config/loader.ts +++ b/packages/core/src/config/loader.ts @@ -105,13 +105,17 @@ export function loadConfig(dir: string): DispatchConfig { * - A key present only in one side is carried over verbatim. * - A key whose value is a string on either side: local replaces global. * - A key that is a nested `{ pattern -> action }` object on BOTH sides is - * merged pattern-by-pattern (local patterns override global patterns of the - * same name; non-conflicting patterns from both survive). + * merged pattern-by-pattern: global patterns the local block does NOT also + * define come first (original order), then EVERY local pattern is appended + * last (overriding any same-named global pattern). * - * Global groups are emitted before local-only groups, and global patterns - * before local patterns within a shared group. This ordering matters because - * `configToRuleset` flattens in iteration order and `evaluate` uses `findLast` - * (last match wins) — so local rules naturally win. + * Emitting all local patterns after the global ones is essential, not + * cosmetic: `configToRuleset` flattens patterns in iteration order and + * `evaluate` uses `findLast` (last match wins). If an overridden pattern were + * updated in place, a more-general global pattern (e.g. "*") could remain AFTER + * it and silently shadow the local override. Appending local patterns last + * reproduces a clean "global rules then local rules" concatenation so local + * always wins. */ function mergePermissions( global: DispatchConfig["permissions"], @@ -124,8 +128,25 @@ function mergePermissions( for (const [key, value] of Object.entries(local)) { const existing = result[key]; if (existing !== undefined && typeof existing !== "string" && typeof value !== "string") { - // Both nested objects — merge patterns, local wins on conflicts. - result[key] = { ...existing, ...value }; + // Both nested objects — merge patterns so that ALL local patterns + // are emitted AFTER the global ones. This matters because + // `configToRuleset` flattens patterns in insertion order and + // `evaluate` uses `findLast` (last match wins): a naive + // `{ ...existing, ...value }` would update an overridden pattern + // IN PLACE, leaving a more-general global pattern (e.g. "*") sitting + // AFTER it and silently shadowing the local override. We therefore + // drop any global pattern that the local block also defines, keep the + // remaining global patterns in their original order, then append every + // local pattern last — reproducing a clean "global rules then local + // rules" concatenation where local always wins. + const merged: Record = {}; + for (const [pattern, action] of Object.entries(existing)) { + if (!(pattern in value)) merged[pattern] = action; + } + for (const [pattern, action] of Object.entries(value)) { + merged[pattern] = action; + } + result[key] = merged; } else { // Local string, brand-new key, or a string/object type mismatch: // local replaces global wholesale. diff --git a/packages/core/tests/config/merge.test.ts b/packages/core/tests/config/merge.test.ts index b362628..b9e4bbb 100644 --- a/packages/core/tests/config/merge.test.ts +++ b/packages/core/tests/config/merge.test.ts @@ -157,6 +157,23 @@ describe("mergeConfigs — permissions", () => { const ruleset = configToRuleset(merged); expect(evaluate("bash", "anything", ruleset).action).toBe("allow"); }); + + // Regression: a SPECIFIC local override must not be shadowed by a more + // GENERAL global pattern (e.g. "*") that happened to be declared lower in + // the global block. `evaluate` uses findLast, so every local pattern must be + // emitted AFTER all global patterns of the same group. + it("specific local override beats a general global wildcard regardless of declaration order", () => { + const merged = mergeConfigs( + { permissions: { bash: { "npm test": "allow", "*": "ask" } } }, + { permissions: { bash: { "npm test": "deny" } } }, + ); + // Local "npm test" must be emitted after global "*". + expect(Object.keys(merged.permissions.bash as object)).toEqual(["*", "npm test"]); + const ruleset = configToRuleset(merged); + expect(evaluate("bash", "npm test", ruleset).action).toBe("deny"); + // And the inherited global wildcard still applies to other commands. + expect(evaluate("bash", "rm -rf /", ruleset).action).toBe("ask"); + }); }); // ─── loadConfig (filesystem integration) ───────────────────────── -- cgit v1.2.3