diff --git a/README.md b/README.md index 88ca94c..93d8689 100644 --- a/README.md +++ b/README.md @@ -301,6 +301,14 @@ Write a rule with at least one command it must match (`block`) and one it must n } ``` +`label` is the tool name: one name (`"Bash"`), or an alternation of two or more in braces +(`"{Bash,PowerShell}"`), and `*` works as in a glob (`"mcp__github__*"`). A single name in +braces (`"{Bash}"`) is not an alternation: its braces are read literally, so it never matches +`Bash`. `guardrails add` writes it without the braces and says so, and `guardrails validate` +warns about it. Only a plain tool name is rewritten: anything else in one-value braces, such +as a wildcard or a negation (`"{*}"`, `"{!Bash}"`), is only warned about, because without the +braces it could match almost every tool call. + Validate it against Guard's own evaluator, then add it: ```console diff --git a/src/__tests__/label-braces.test.ts b/src/__tests__/label-braces.test.ts new file mode 100644 index 0000000..dc1c90e --- /dev/null +++ b/src/__tests__/label-braces.test.ts @@ -0,0 +1,160 @@ +import { describe, expect, it } from "vitest"; +import { correctSingleBraceLabels, findSingleBraceLabels } from "../core/label-braces.js"; +import { createCompiledLabelMatcher } from "../engine/matchers.js"; + +describe("the trap is real", () => { + it("`{Bash}` does not match Bash, while `Bash` and `{Bash,PowerShell}` do", () => { + expect(createCompiledLabelMatcher("{Bash}")("Bash")).toBe(false); + expect(createCompiledLabelMatcher("Bash")("Bash")).toBe(true); + expect(createCompiledLabelMatcher("{Bash,PowerShell}")("Bash")).toBe(true); + }); +}); + +describe("findSingleBraceLabels", () => { + it("finds a one-element brace group in every composition arm", () => { + const match = { + any_of: [{ kind: "execute_tool", label: "{Bash}" }], + all_of: [{ kind: "execute_tool", label: "mcp__{github}__*" }], + none_of: [{ kind: "execute_tool", label: "{Read}" }], + }; + expect(findSingleBraceLabels(match)).toEqual([ + { path: "match.any_of[0].label", label: "{Bash}", corrected: "Bash", rewrite: true }, + { + path: "match.all_of[0].label", + label: "mcp__{github}__*", + corrected: "mcp__github__*", + rewrite: true, + }, + { path: "match.none_of[0].label", label: "{Read}", corrected: "Read", rewrite: true }, + ]); + }); + + it.each([ + ["a plain value", "Bash"], + ["an alternation", "{Bash,PowerShell}"], + ["an alternation with an empty branch", "{Bash,}"], + ["a numeric range", "tool{1..3}"], + ["an escaped brace", "\\{Bash\\}"], + ["a glob", "mcp__*"], + ["an empty group", "{}"], + ["an empty group in a label", "mcp__{}x"], + ["a group of spaces", "{ }"], + ["a brace inside a bracket expression", "[{a}]"], + ["a group closed by an escaped brace", "{Bash\\}"], + ])("leaves %s alone", (_name, label) => { + expect(findSingleBraceLabels({ any_of: [{ kind: "execute_tool", label }] })).toEqual([]); + }); + + it("corrects every one-element group in one label", () => { + expect(findSingleBraceLabels({ any_of: [{ label: "mcp__{x}__{y}" }] })).toEqual([ + { + path: "match.any_of[0].label", + label: "mcp__{x}__{y}", + corrected: "mcp__x__y", + rewrite: true, + }, + ]); + }); + + it("trims spaces around the value", () => { + expect(findSingleBraceLabels({ any_of: [{ label: "{ Bash }" }] })).toEqual([ + { path: "match.any_of[0].label", label: "{ Bash }", corrected: "Bash", rewrite: true }, + ]); + }); + + it("unwraps nested one-element groups completely", () => { + expect(findSingleBraceLabels({ any_of: [{ label: "{{Bash}}" }] })).toEqual([ + { path: "match.any_of[0].label", label: "{{Bash}}", corrected: "Bash", rewrite: true }, + ]); + }); + + it("flags a group next to an escaped bracket", () => { + expect(findSingleBraceLabels({ any_of: [{ label: "\\[{a}\\]" }] })).toEqual([ + { path: "match.any_of[0].label", label: "\\[{a}\\]", corrected: "\\[a\\]", rewrite: true }, + ]); + }); + + it("unwraps a one-element group nested in an alternation", () => { + // picomatch reads `{a,{b}}` as `a` or a literal `{b}`. + expect(findSingleBraceLabels({ any_of: [{ label: "{a,{b}}" }] })).toEqual([ + { path: "match.any_of[0].label", label: "{a,{b}}", corrected: "{a,b}", rewrite: true }, + ]); + }); + + it.each([ + ["{*}", "*"], + ["{mcp__*}", "mcp__*"], + ["{Bas?}", "Bas?"], + ["{[BP]ash}", "[BP]ash"], + ["{Bash}{*}", "Bash*"], + ["{!Bash}", "!Bash"], + ["{!(Bash)}", "!(Bash)"], + ["{!}", "!"], + ["{@(Bash|Edit)}", "@(Bash|Edit)"], + ["{+(Bash)}", "+(Bash)"], + ["{(\\w)+}", "(\\w)+"], + ["{\\S\\S\\S\\S}", "\\S\\S\\S\\S"], + ["{Bash|Edit}", "Bash|Edit"], + ])("finds %s but marks it not to be rewritten, because %s is not a plain tool name", (label, corrected) => { + expect(findSingleBraceLabels({ any_of: [{ label }] })).toEqual([ + { path: "match.any_of[0].label", label, corrected, rewrite: false }, + ]); + }); + + it("stays linear on a long run of unclosed brackets and braces", () => { + const start = performance.now(); + findSingleBraceLabels({ any_of: [{ label: "[".repeat(50_000) }] }); + findSingleBraceLabels({ any_of: [{ label: "{".repeat(50_000) }] }); + expect(performance.now() - start).toBeLessThan(1_000); + }); + + it("is total on shapes that are not a match object", () => { + for (const value of [undefined, null, 1, "x", [], { any_of: "x" }, { any_of: [null, 1] }]) { + expect(findSingleBraceLabels(value)).toEqual([]); + } + }); +}); + +describe("correctSingleBraceLabels", () => { + it("returns a corrected copy and leaves the input unchanged", () => { + const match = { any_of: [{ kind: "execute_tool", label: "{Bash}", detail_contains: ["x"] }] }; + const { match: corrected, fixes } = correctSingleBraceLabels(match); + expect(corrected).toEqual({ + any_of: [{ kind: "execute_tool", label: "Bash", detail_contains: ["x"] }], + }); + expect(match.any_of[0]?.label).toBe("{Bash}"); + expect(fixes).toHaveLength(1); + }); + + it("leaves a wildcard group as written and reports it as a warning", () => { + const match = { + any_of: [ + { kind: "execute_tool", label: "{*}", detail_contains: ["x"] }, + { kind: "execute_tool", label: "{Bash}", detail_contains: ["y"] }, + ], + }; + const { match: corrected, fixes, warnings } = correctSingleBraceLabels(match); + expect(corrected).toEqual({ + any_of: [ + { kind: "execute_tool", label: "{*}", detail_contains: ["x"] }, + { kind: "execute_tool", label: "Bash", detail_contains: ["y"] }, + ], + }); + expect(fixes.map((f) => f.label)).toEqual(["{Bash}"]); + expect(warnings.map((f) => f.label)).toEqual(["{*}"]); + }); + + it("returns the input itself when only a wildcard group is found", () => { + const match = { any_of: [{ kind: "execute_tool", label: "{*}" }] }; + const result = correctSingleBraceLabels(match); + expect(result.match).toBe(match); + expect(result.fixes).toEqual([]); + expect(result.warnings).toHaveLength(1); + }); + + it("returns the input itself when there is nothing to correct", () => { + const match = { any_of: [{ kind: "execute_tool", label: "Bash" }] }; + expect(correctSingleBraceLabels(match)).toEqual({ match, fixes: [], warnings: [] }); + expect(correctSingleBraceLabels(match).match).toBe(match); + }); +}); diff --git a/src/__tests__/rules-command.test.ts b/src/__tests__/rules-command.test.ts index 3c7b316..4ad625a 100644 --- a/src/__tests__/rules-command.test.ts +++ b/src/__tests__/rules-command.test.ts @@ -1099,6 +1099,246 @@ describe("a guardrail that would match everything is named, not silently accepte }); }); +describe("a one-element brace alternation in `label` is named, not left inert", () => { + // `{Bash,PowerShell}` is an alternation. `{Bash}` is not: picomatch reads a brace group + // with no comma as literal braces, so the label only matches a tool literally named + // `{Bash}`, and the guardrail never fires. + const BRACED = { + ...USER_RULE, + match: { + any_of: [{ kind: "execute_tool", label: "{Bash}", detail_contains: ["./deploy.sh"] }], + }, + }; + const BRACED_CORPUS_RULE = { + ...BRACED, + description: "Synthetic, with a one-element brace alternation.", + fixtures: { + block: [{ tool: "Bash", command: "./deploy.sh prod" }], + allow: [{ tool: "Bash", command: "ls -la" }], + }, + }; + + it("validate with a file warns on `{Bash}` and gives the corrected spelling", async () => { + const h = harness({ "/tmp/b.json": JSON.stringify(BRACED_CORPUS_RULE) }); + // The block fixture is a matching command, and the braced label still misses it. + expect(await runRules(["validate", "/tmp/b.json"], h.io)).toBe(1); + expect(h.out()).toContain("block fixture ./deploy.sh prod"); + expect(h.out()).toContain("FAIL"); + expect(h.out()).toContain("`{Bash}`"); + expect(h.out()).toContain('write `"Bash"`'); + expect(h.out()).toContain("match.any_of[0].label"); + }); + + it("validate with a file reports the warning under --json", async () => { + const h = harness({ "/tmp/b.json": JSON.stringify(BRACED_CORPUS_RULE) }); + await runRules(["validate", "/tmp/b.json", "--json"], h.io); + const j = JSON.parse(h.out()) as Record; + expect(j.warnings).toEqual([ + { path: "match.any_of[0].label", label: "{Bash}", corrected: "Bash", rewrite: true }, + ]); + }); + + it("validate with no file warns on an installed guardrail that uses it", async () => { + const h = harness({ [RULES]: JSON.stringify([BRACED]) }); + expect(await runRules(["validate"], h.io)).toBe(0); + expect(h.out()).toContain("local.no-deploy-friday"); + expect(h.out()).toContain("`{Bash}`"); + expect(h.out()).toContain('write `"Bash"`'); + }); + + it("validate with no file still warns when another installed guardrail is invalid", async () => { + const h = harness({ + [RULES]: JSON.stringify([BRACED, { ...USER_RULE_OWN_CATEGORY, defaultAction: "ask" }]), + }); + expect(await runRules(["validate"], h.io)).toBe(1); + expect(h.out()).toContain("require_approval"); + expect(h.out()).toContain("`{Bash}`"); + }); + + it("validate with no file keeps the warning under --json when another guardrail is invalid", async () => { + const h = harness({ + [RULES]: JSON.stringify([BRACED, { ...USER_RULE_OWN_CATEGORY, defaultAction: "ask" }]), + }); + expect(await runRules(["validate", "--json"], h.io)).toBe(1); + const j = JSON.parse(h.out()) as Record; + expect(j.warnings).toEqual([ + { + guardrail: "local.no-deploy-friday", + path: "match.any_of[0].label", + label: "{Bash}", + corrected: "Bash", + rewrite: true, + }, + ]); + }); + + it("add rewrites `{Bash}` to `Bash` and says so", async () => { + const h = harness({ "/tmp/b.json": JSON.stringify(BRACED) }); + expect(await runRules(["add", "/tmp/b.json"], h.io)).toBe(0); + const stored = JSON.parse(h.raw(RULES) as string) as (typeof USER_RULE)[]; + expect(stored[0]?.match.any_of[0]?.label).toBe("Bash"); + expect(stored).toEqual([USER_RULE]); + expect(h.out()).toContain("Rewrote"); + expect(h.out()).toContain("`{Bash}` to `Bash`"); + }); + + it("add reports the rewrite under --json", async () => { + const h = harness({ "/tmp/b.json": JSON.stringify(BRACED) }); + await runRules(["add", "/tmp/b.json", "--json"], h.io); + const j = JSON.parse(h.out()) as Record; + expect(j).toMatchObject({ ok: true, changed: true, guardrail: "local.no-deploy-friday" }); + expect(j.rewrote).toEqual([ + { path: "match.any_of[0].label", label: "{Bash}", corrected: "Bash", rewrite: true }, + ]); + }); + + describe("a wildcard in one-value braces is warned about and never rewritten", () => { + // Unwrapping `{*}` would turn a guardrail that never fires into one that matches every + // tool call, so it is left as written. + const STAR = { + ...USER_RULE, + match: { + any_of: [{ kind: "execute_tool", label: "{*}", detail_contains: ["./deploy.sh"] }], + }, + }; + const STAR_CORPUS_RULE = { + ...STAR, + description: "Synthetic, with a wildcard in one-value braces.", + fixtures: { + block: [{ tool: "Bash", command: "./deploy.sh prod" }], + allow: [{ tool: "Bash", command: "ls -la" }], + }, + }; + const WARNING = { + path: "match.any_of[0].label", + label: "{*}", + corrected: "*", + rewrite: false, + }; + + it("add writes `{*}` unchanged and warns with the spelling", async () => { + const h = harness({ "/tmp/s.json": JSON.stringify(STAR) }); + expect(await runRules(["add", "/tmp/s.json"], h.io)).toBe(0); + expect(JSON.parse(h.raw(RULES) as string)).toEqual([STAR]); + expect(h.out()).not.toContain("Rewrote"); + expect(h.out()).toContain("WARNING"); + expect(h.out()).toContain("`{*}`"); + expect(h.out()).toContain('`"*"`'); + expect(h.out()).toContain("not rewritten"); + }); + + it("add still prints the warning under --quiet", async () => { + const h = harness({ "/tmp/s.json": JSON.stringify(STAR) }); + expect(await runRules(["add", "/tmp/s.json", "--quiet"], h.io)).toBe(0); + expect(h.out()).toContain("WARNING"); + expect(h.out()).toContain("`{*}`"); + expect(h.out()).not.toContain("Validated."); + }); + + it("add writes a negation in one-value braces unchanged and warns", async () => { + const negated = { + ...USER_RULE, + match: { + any_of: [{ kind: "execute_tool", label: "{!Bash}", detail_contains: ["./deploy.sh"] }], + }, + }; + const h = harness({ "/tmp/n.json": JSON.stringify(negated) }); + expect(await runRules(["add", "/tmp/n.json"], h.io)).toBe(0); + expect(JSON.parse(h.raw(RULES) as string)).toEqual([negated]); + expect(h.out()).not.toContain("Rewrote"); + expect(h.out()).toContain("WARNING"); + }); + + it("add reports it under --json as a warning, not a rewrite", async () => { + const h = harness({ "/tmp/s.json": JSON.stringify(STAR) }); + expect(await runRules(["add", "/tmp/s.json", "--json"], h.io)).toBe(0); + expect(JSON.parse(h.raw(RULES) as string)).toEqual([STAR]); + const j = JSON.parse(h.out()) as Record; + expect(j).not.toHaveProperty("rewrote"); + expect(j.warnings).toEqual([WARNING]); + }); + + it("add rewrites `{Bash}` and only warns on `{*}` in the same guardrail", async () => { + const mixed = { + ...USER_RULE, + match: { + any_of: [ + { kind: "execute_tool", label: "{*}", detail_contains: ["./deploy.sh"] }, + { kind: "execute_tool", label: "{Bash}", detail_contains: ["./release.sh"] }, + ], + }, + }; + const h = harness({ "/tmp/m.json": JSON.stringify(mixed) }); + expect(await runRules(["add", "/tmp/m.json", "--json"], h.io)).toBe(0); + const stored = JSON.parse(h.raw(RULES) as string) as (typeof mixed)[]; + expect(stored[0]?.match.any_of.map((c) => c.label)).toEqual(["{*}", "Bash"]); + const j = JSON.parse(h.out()) as Record; + expect((j.rewrote as { label: string }[]).map((f) => f.label)).toEqual(["{Bash}"]); + expect((j.warnings as { label: string }[]).map((f) => f.label)).toEqual(["{*}"]); + }); + + it("validate with a file warns and leaves the file unchanged", async () => { + const text = JSON.stringify(STAR_CORPUS_RULE); + const h = harness({ "/tmp/s.json": text }); + await runRules(["validate", "/tmp/s.json"], h.io); + expect(h.out()).toContain("WARNING"); + expect(h.out()).toContain("`{*}`"); + expect(h.raw("/tmp/s.json")).toBe(text); + expect(h.writes).toEqual([]); + }); + + it("validate with a file reports it under --json", async () => { + const h = harness({ "/tmp/s.json": JSON.stringify(STAR_CORPUS_RULE) }); + await runRules(["validate", "/tmp/s.json", "--json"], h.io); + expect((JSON.parse(h.out()) as Record).warnings).toEqual([WARNING]); + }); + + it("validate with no file warns on an installed guardrail and leaves it unchanged", async () => { + const text = JSON.stringify([STAR]); + const h = harness({ [RULES]: text }); + expect(await runRules(["validate"], h.io)).toBe(0); + expect(h.out()).toContain("WARNING local.no-deploy-friday"); + expect(h.out()).toContain("`{*}`"); + expect(h.raw(RULES)).toBe(text); + expect(h.writes).toEqual([]); + }); + + it("validate with no file reports it under --json", async () => { + const h = harness({ [RULES]: JSON.stringify([STAR]) }); + expect(await runRules(["validate", "--json"], h.io)).toBe(0); + expect((JSON.parse(h.out()) as Record).warnings).toEqual([ + { guardrail: "local.no-deploy-friday", ...WARNING }, + ]); + }); + }); + + it("leaves a real alternation alone, in add and in validate", async () => { + const alternation = { + ...USER_RULE, + match: { + any_of: [ + { kind: "execute_tool", label: "{Bash,PowerShell}", detail_contains: ["./deploy.sh"] }, + ], + }, + }; + const added = harness({ "/tmp/a.json": JSON.stringify(alternation) }); + expect(await runRules(["add", "/tmp/a.json"], added.io)).toBe(0); + expect(JSON.parse(added.raw(RULES) as string)).toEqual([alternation]); + expect(added.out()).not.toContain("Rewrote"); + + const installed = harness({ [RULES]: JSON.stringify([alternation]) }); + expect(await runRules(["validate"], installed.io)).toBe(0); + expect(installed.out()).not.toContain("WARNING"); + }); + + it("a rejected rule's example says what `label` accepts", async () => { + const h = harness({ "/tmp/x.json": JSON.stringify({ ...USER_RULE, category: "made-up" }) }); + expect(await runRules(["add", "/tmp/x.json"], h.io)).toBe(1); + expect(h.out()).toContain("`label` is one tool name"); + expect(h.out()).toContain("{Bash,PowerShell}"); + }); +}); + /** * The `--json` KEY CONTRACT. * diff --git a/src/commands/rules.ts b/src/commands/rules.ts index 6f90bc8..b30e6b2 100644 --- a/src/commands/rules.ts +++ b/src/commands/rules.ts @@ -38,6 +38,11 @@ import { SHIPPED_CATALOG } from "../core/catalog.js"; import { parseConfig, updateConfigText } from "../core/config.js"; import { inspectConfig } from "../core/config-report.js"; import { compileAllowlist, evaluateCall } from "../core/evaluate.js"; +import { + correctSingleBraceLabels, + findSingleBraceLabels, + singleBraceWarning, +} from "../core/label-braces.js"; import { buildGuardSpanContext } from "../core/normalize.js"; import { guardDir } from "../core/paths.js"; import { blockFixtureCommands } from "../core/rule-fixtures.js"; @@ -995,7 +1000,10 @@ const CORRECTED_EXAMPLE = ` { "match": { "any_of": [ { "kind": "execute_tool", "label": "Bash", "detail_contains": ["./deploy.sh"] } ]} - }`; + } + +\`label\` is one tool name ("Bash") or an alternation of two or more ("{Bash,PowerShell}"). +A single name in braces ("{Bash}") is not an alternation: its braces are read literally.`; function cmdAdd(io: SetupIO, flags: Flags, files: Files, deps: RulesDeps, path: string): number { const r = new Reporter(io, flags, "add"); @@ -1009,7 +1017,23 @@ function cmdAdd(io: SetupIO, flags: Flags, files: Files, deps: RulesDeps, path: return r.fail(`${path} is not valid JSON: ${(error as Error).message}. Nothing was written.`); } // A file holding a one-element array is what someone copying `guardrails.json` produces. - const candidate = Array.isArray(raw) && raw.length === 1 ? raw[0] : raw; + let candidate = Array.isArray(raw) && raw.length === 1 ? raw[0] : raw; + + // `{Bash}` is accepted by every check and then never matches `Bash`, so the label is written + // without the braces and the output says so. A group that is not a plain tool name, such as + // `{*}`, is only warned about: unwrapping it could turn a guardrail that never fires into one + // that matches every tool call. + let rewrote: ReturnType = []; + let braceWarnings: ReturnType = []; + if (candidate !== null && typeof candidate === "object" && !Array.isArray(candidate)) { + const rec = candidate as Record; + const corrected = correctSingleBraceLabels(rec.match); + braceWarnings = corrected.warnings; + if (corrected.fixes.length > 0) { + candidate = { ...rec, match: corrected.match }; + rewrote = corrected.fixes; + } + } const { rule, errors } = validateUserRule(candidate); if (rule === undefined) { @@ -1045,10 +1069,25 @@ function cmdAdd(io: SetupIO, flags: Flags, files: Files, deps: RulesDeps, path: io.writeFileAtomic(files.rulesPath, `${JSON.stringify([...existing, candidate], null, 2)}\n`); const active = views.filter((v) => v.enabled).length + 1; r.say( + ...rewrote.map( + (f) => + `Rewrote ${f.path} from \`${f.label}\` to \`${f.corrected}\`: a brace group with one value is not an alternation, and picomatch would have read the braces literally.`, + ), + ...braceWarnings.map((f) => `WARNING: ${singleBraceWarning(f)}`), `Validated. Added ${rule.id} (${displayAction(rule.defaultAction)}) to ${files.rulesPath}.`, `${active} guardrails active.`, ); - return r.ok({ changed: true, guardrail: rule.id }); + // `--quiet` drops the lines above, but a guardrail that was written and will never fire is + // worth one line even then. + if (flags.quiet && !flags.json) { + for (const f of braceWarnings) io.writeStdout(`WARNING: ${singleBraceWarning(f)}\n`); + } + return r.ok({ + changed: true, + guardrail: rule.id, + ...(rewrote.length > 0 ? { rewrote } : {}), + ...(braceWarnings.length > 0 ? { warnings: braceWarnings } : {}), + }); } function cmdRemove(io: SetupIO, flags: Flags, files: Files, deps: RulesDeps, id: string): number { @@ -1117,19 +1156,36 @@ function cmdValidate( // question, and it is the one a user asks most often. if (path === undefined) { const user = loadUserRules(files.rulesText); + // A valid guardrail can still be inert: `{Bash}` loads and then never matches `Bash`. + const warnings = user.valid.flatMap((rule) => + findSingleBraceLabels(rule.match).map((found) => ({ guardrail: rule.id, ...found })), + ); if (flags.json) { return user.invalid.length === 0 - ? r.ok({ valid: user.valid.length, invalid: [] }) - : r.fail("invalid guardrails", { invalid: user.invalid }); + ? r.ok({ + valid: user.valid.length, + invalid: [], + ...(warnings.length > 0 ? { warnings } : {}), + }) + : r.fail("invalid guardrails", { + invalid: user.invalid, + ...(warnings.length > 0 ? { warnings } : {}), + }); } + const warningLines = warnings.map( + (w) => ` WARNING ${w.guardrail}: ${singleBraceWarning(w)}`, + ); if (user.invalid.length === 0) { - r.say(`Valid. ${user.valid.length} of your own guardrails in ${files.rulesPath}.`); + r.say( + `Valid. ${user.valid.length} of your own guardrails in ${files.rulesPath}.`, + ...warningLines, + ); return r.ok({ valid: user.valid.length }); } io.writeStdout( `${user.invalid.length} of your guardrails ${user.invalid.length === 1 ? "is" : "are"} invalid and NOT running:\n${user.invalid .map((b) => ` ${b.id}: ${b.reason}`) - .join("\n")}\n`, + .join("\n")}\n${warningLines.map((line) => `${line}\n`).join("")}`, ); return 1; } @@ -1187,11 +1243,13 @@ function cmdValidate( }); } const failed = rows.filter((x) => !x.pass).length; + const braces = findSingleBraceLabels(rule.match); + const warnings = braces.length > 0 ? { warnings: braces } : {}; if (flags.json) { return failed === 0 - ? r.ok({ id: rule.id, action: rule.defaultAction, fixtures: rows }) - : r.fail("fixtures failed", { id: rule.id, fixtures: rows }); + ? r.ok({ id: rule.id, action: rule.defaultAction, fixtures: rows, ...warnings }) + : r.fail("fixtures failed", { id: rule.id, fixtures: rows, ...warnings }); } // A fixture path written with backslashes is answered differently depending on @@ -1218,6 +1276,7 @@ function cmdValidate( // own evaluator. " Schema checked with the same `parseRule` CI runs. Fixtures were run through the", " guard's own evaluator — the one that will run on your machine.", + ...braces.flatMap((found) => ["", ` WARNING: ${singleBraceWarning(found)}`]), ...(backslashFixtures.length > 0 ? [ "", diff --git a/src/core/label-braces.ts b/src/core/label-braces.ts new file mode 100644 index 0000000..8135c32 --- /dev/null +++ b/src/core/label-braces.ts @@ -0,0 +1,134 @@ +/** + * One-element brace groups in a guardrail's `label`. + * + * `label` is a picomatch pattern. `{Bash,PowerShell}` is an alternation and matches either + * name. A brace group with no comma and no `..` range, such as `{Bash}`, is not: picomatch + * reads it as literal braces, so the label matches only a tool literally named `{Bash}` + * and the guardrail does not fire on `Bash`. Nothing about such a rule is invalid, so the loader + * accepts it and lists it as enabled. + * + * This module finds those groups and spells the label without the braces. `guardrails add` + * writes the corrected label and says so; `guardrails validate` names it as a warning. + * Matching itself is unchanged. + * + * Only a group whose value is a plain tool name (letters, digits, `_`, `.`, `:`, `-`) is + * rewritten. Any other value, such as a wildcard, a negation, an extended glob or a regex + * escape (`{*}`, `{mcp__*}`, `{!Bash}`, `{@(A|B)}`, `{(\w)+}`), is warned about and left as + * written: without the braces the label could go from matching no tool to matching every + * tool the pattern covers, which can block or allow every call. + * + * Pure, and total on any input. + */ + +/** One label that holds a one-element brace group. */ +export interface SingleBraceLabel { + /** Where the label is, as `match.[].label`. */ + readonly path: string; + /** The label as written. */ + readonly label: string; + /** The same label with each one-element group's braces removed. */ + readonly corrected: string; + /** + * Whether `guardrails add` writes `corrected` in place of `label`. False when an unwrapped + * value is not a plain tool name, because the correction could widen what the guardrail + * matches. + */ + readonly rewrite: boolean; +} + +/** A value that names one tool and holds no pattern syntax. Only these are rewritten. */ +const PLAIN_NAME = /^[\p{L}\p{N}_.:-]+$/u; + +/** The three composition arms a condition can sit in. */ +const ARMS = ["any_of", "all_of", "none_of"] as const; + +/** + * A bracket expression (`[…]`, matched first and kept as is, because a `{` inside it is a + * character in a class), or a `{…}` group holding no comma and no nested brace. Neither + * brace may be escaped with a backslash. The bracket body stops at the next `[`, so a run of + * unclosed `[` is scanned once rather than once per `[`. + */ +const BRACKET_OR_BRACE_GROUP = /(? { + const value = inner?.trim(); + if (value === undefined || value === "" || value.includes("..")) return group; + if (!PLAIN_NAME.test(value)) rewrite = false; + return value; + }); + if (next === current) break; + current = next; + } + return current === label ? undefined : { corrected: current, rewrite }; +} + +/** A JSON object that is not an array. */ +function isRecord(value: unknown): value is Record { + return value !== null && typeof value === "object" && !Array.isArray(value); +} + +/** Every label in `match` that holds a one-element brace group, in arm and index order. */ +export function findSingleBraceLabels(match: unknown): SingleBraceLabel[] { + const found: SingleBraceLabel[] = []; + if (!isRecord(match)) return found; + for (const arm of ARMS) { + const conditions = match[arm]; + if (!Array.isArray(conditions)) continue; + conditions.forEach((condition, index) => { + if (!isRecord(condition) || typeof condition.label !== "string") return; + const result = correctLabel(condition.label); + if (result === undefined) return; + found.push({ path: `match.${arm}[${index}].label`, label: condition.label, ...result }); + }); + } + return found; +} + +/** + * `match` with every rewritable label corrected, plus what was rewritten (`fixes`) and what + * was found but left as written because a rewrite would widen it (`warnings`). + * + * Returns a new object when something changed and the input itself when nothing did. The + * input is never modified. + */ +export function correctSingleBraceLabels(match: unknown): { + match: unknown; + fixes: SingleBraceLabel[]; + warnings: SingleBraceLabel[]; +} { + const found = findSingleBraceLabels(match); + const fixes = found.filter((f) => f.rewrite); + const warnings = found.filter((f) => !f.rewrite); + if (fixes.length === 0 || !isRecord(match)) return { match, fixes, warnings }; + const corrected: Record = { ...match }; + for (const arm of ARMS) { + const conditions = match[arm]; + if (!Array.isArray(conditions)) continue; + corrected[arm] = conditions.map((condition) => { + if (!isRecord(condition) || typeof condition.label !== "string") return condition; + const result = correctLabel(condition.label); + return result?.rewrite ? { ...condition, label: result.corrected } : condition; + }); + } + return { match: corrected, fixes, warnings }; +} + +/** The sentence `guardrails add` and `guardrails validate` print for one finding. */ +export function singleBraceWarning(found: SingleBraceLabel): string { + const head = `${found.path} is \`${found.label}\`: a brace group with one value is not an alternation, so picomatch reads the braces literally`; + if (found.rewrite) { + return `${head}; write \`${JSON.stringify(found.corrected)}\` instead (use \`{A,B}\` only for two or more values).`; + } + return `${head} and this guardrail never fires. Without the braces it is \`${JSON.stringify(found.corrected)}\`, a pattern that can match many tools, so it was not rewritten; write that only if every tool it matches should match.`; +}