From c4e75ae538ab15c3a6200e873a0bbf0c52d1fc1f Mon Sep 17 00:00:00 2001 From: zernie Date: Wed, 9 Sep 2026 21:37:34 +0500 Subject: [PATCH] fix: read a space-separated tool list, so a working fence stops auditing as ineffective (#221) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: read a space-separated tool list, so a working fence stops auditing as ineffective Claude Code documents three equivalent spellings of `allowed-tools:` / `disallowed-tools:` / a subagent's `tools:` — a comma-separated string, a SPACE-separated string, and a YAML list. `splitList` in core/frontmatter-read.ts split on `,` alone, so the space-separated form arrived as ONE token matching no built-in name. Reported with a reproduction by @vlad-ryzhkov in #217: on a 22-skill harness, changing only the separator moved Safety 81 -> 80 -> 81, reversibly. Reproduced here before changing anything — three of the seven documented shapes came back as a single token: `WebFetch WebSearch`, `Bash(git add *) Bash(git commit *)`, and `"Read Write Glob"` (a YAML-quoted scalar whose VALUE is a space-separated list, which is what most skills in the wild write). TWO SURFACES, AND THE SECOND IS WORSE THAN THE REPORTED ONE. #217 asks about a skill's fence, where `allowed-tools:` only pre-approves, so a mis-split MISLEADS: a fence Claude Code really enforces was reported as closing no lethal-trifecta leg, naming as still-supplied the very tools the author had just denied. But a SUBAGENT's `tools:` is the allowlist the PreToolUse rail denies against, and it reads the same `splitList`. Measured on `tools: Read Grep Glob`, pre-fix: Read DENY · Grep DENY · Bash DENY — the joined token matched nothing, so the agent was denied EVERY tool, including the three it was explicitly granted. #217 raised this path as an open question ("I have not worked out whether anything downstream depends on that"); it did. THE NAIVE FIX IS WORSE THAN THE BUG. `split(/[,\s]+/)` shreds `Bash(git push *)` into `Bash(git`, `push`, `*)`, and `bashGrantIsUnbounded()` answers "unbounded" when it cannot recognise a grant — so a BOUNDED grant would begin reading as an unbounded one. That trades a false-clean verdict for a false-exposed one, which is the worse side of the trade core/lethal-trifecta.ts already reasons about, for a reason the author cannot see anywhere in their file. So this is a tokenizer: whitespace separates only at paren depth 0. Quotes are stripped per token AFTER splitting, never treated as a delimiter — the outer quotes of `"Read Write Glob"` are YAML syntax, and treating them as token boundaries would keep exactly this bug for every skill that quotes its list. An unclosed `(` keeps the remainder as one token rather than fragmenting it into garbage tool names. The tokenizer is the one @vlad-ryzhkov proposed, with his six-row table plus irregular spacing and an unbalanced paren as tests. Every new test is mutation-proven with the patch verified landed: separator back to `,` only (pre-fix) -> the 4 new tests fail whitespace splits at ANY depth (the naive fix) -> the parens test fails One home, so the surfaces cannot drift: `frontmatterList` is the single reader behind scan, adopt, and the rail. Closes #217 Co-Authored-By: vlad-ryzhkov <211849729+vlad-ryzhkov@users.noreply.github.com> Co-Authored-By: Claude Opus 5 * fix: a parenthesis inside a quoted string is a character, not a bracket Follow-up within #217, found by the Codex review bot on the first cut of the tokenizer. The depth counter read every `(` as structure, so a grant carrying a literal unmatched paren in its own command left depth at 1 after the grant closed and swallowed everything after it: tools: Bash(printf '( %s' foo) Read -> ["Bash(printf '( %s' foo) Read"] one token On a subagent that is the same failure this PR exists to fix: the rail denies `Read`, a tool the author granted. Reproduced before changing anything. Quote state now gates the DEPTH COUNTER ONLY, never the splitting. That asymmetry is the fix, not an accident: if being inside quotes also suppressed whitespace splitting, a quoted list would collapse back to one token and reintroduce #217 itself for every skill that writes `allowed-tools: "Read Write Glob"`. Backslash escapes are honoured outside single quotes, per shell rules. MUTATION-PROVEN BOTH WAYS, each with the patch verified landed: depth counted inside quotes (the pre-fix state) -> the new test fails quotes suppress splitting too (the over-fix) -> the new test fails 🔴 AND THE SECOND MUTATION EXPOSED A DEFECT IN THE TEST FIRST. Its original counterweight asserted the quoted list on a VALID YAML block and stayed GREEN under the widening mutation — because js-yaml strips the quotes before `splitList` ever runs, so no quote character was present and the mutation could not bite. Measured, then moved onto a MALFORMED block, where the regex salvage path hands the quotes through: that is the only path on which the property is observable at all. A green mutation is a finding about the test. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Co-authored-by: vlad-ryzhkov <211849729+vlad-ryzhkov@users.noreply.github.com> --- CLAUDE.md | 4 +- CLAUDE.md.spec.ts | 2 +- docs/rules/disallowed-tools-contract.md | 6 +- .../claude-code/agent-runtime.test.ts | 18 ++++ src/core/frontmatter-read.test.ts | 91 +++++++++++++++++++ src/core/frontmatter-read.ts | 80 ++++++++++++++-- src/scan.test.ts | 17 ++++ 7 files changed, 207 insertions(+), 11 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 547e6b94..bd11835e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,4 +1,4 @@ - + # CLAUDE.md @@ -224,7 +224,7 @@ BUILD + TOOLING + GENERATED: - `src/core/frontmatter.ts` — Frontmatter-mode parser: `vigiles: enforce:` YAML frontmatter rules in markdown (Level 1 adoption) - `src/core/frontmatter.test.ts` — Frontmatter parser test suite (node:test) - `src/core/frontmatter-read.ts` — Lenient frontmatter reader — ONE reader for the SKILL.md/subagent --- block, shared by scan + the PreToolUse rail (agent-runtime), replacing three divergent hand-parsers (scan's readField +… -- `src/core/frontmatter-read.test.ts` — Lenient-reader suite (vitest): valid YAML scalars + flow array, comma-list split, absent→null vs present-empty→[], block-scalar + next-line-quoted, malformed YAML → malformed:true AND salvages a… +- `src/core/frontmatter-read.test.ts` — Lenient-reader suite (vitest): valid YAML scalars + flow array, the comma / space / YAML-list split (whitespace separates only outside parens, #217), absent→null vs present-empty→[], block-scalar + next-line-quoted, malformed YAML → malformed:true AND salvages a… - `src/core/hook-normalize.ts` — Hook settings normalization — the typed boundary (parse-don't-validate) the audit hook detectors read. normalizeHooks(raw) parses the raw settings.hooks `unknown` ONCE into a typed… - `src/core/hook-normalize.test.ts` — Hook-normalize suite (vitest): flattens the CC nested shape, reads the Codex flat shape, carries matcher null when absent, drops empty/non-string commands + non-object entries, returns [] for… - `action.yml` — GitHub Action — a composite action over the published `npx vigiles` CLI (NOT a node20 entry pointing at an uncommitted dist/): maps every input to a real CLI flag, sets the `valid` output via $GITHUB_OUTPUT, and supports `version: local` so the repo dogfoods it via `uses: ./`. See docs/cli.md and the `prod-grade-gha-cli` rule. diff --git a/CLAUDE.md.spec.ts b/CLAUDE.md.spec.ts index 6d98ebc1..4a8bb194 100644 --- a/CLAUDE.md.spec.ts +++ b/CLAUDE.md.spec.ts @@ -345,7 +345,7 @@ BUILD + TOOLING + GENERATED: "src/core/frontmatter-read.ts": "Lenient frontmatter reader — ONE reader for the SKILL.md/subagent --- block, shared by scan + the PreToolUse rail (agent-runtime), replacing three divergent hand-parsers (scan's readField +…", "src/core/frontmatter-read.test.ts": - "Lenient-reader suite (vitest): valid YAML scalars + flow array, comma-list split, absent→null vs present-empty→[], block-scalar + next-line-quoted, malformed YAML → malformed:true AND salvages a…", + "Lenient-reader suite (vitest): valid YAML scalars + flow array, the comma / space / YAML-list split (whitespace separates only outside parens, #217), absent→null vs present-empty→[], block-scalar + next-line-quoted, malformed YAML → malformed:true AND salvages a…", "src/core/hook-normalize.ts": "Hook settings normalization — the typed boundary (parse-don't-validate) the audit hook detectors read. normalizeHooks(raw) parses the raw settings.hooks `unknown` ONCE into a typed…", "src/core/hook-normalize.test.ts": diff --git a/docs/rules/disallowed-tools-contract.md b/docs/rules/disallowed-tools-contract.md index f62b8a1f..10781354 100644 --- a/docs/rules/disallowed-tools-contract.md +++ b/docs/rules/disallowed-tools-contract.md @@ -49,8 +49,10 @@ The block-list **inverts** the allow check, so the FP-safe set is different from ## Scope -Subagent frontmatter (`agents/*.md` `disallowedTools:`), both the comma-list and -inline-array forms. +Subagent frontmatter (`agents/*.md` `disallowedTools:`) in every spelling Claude +Code accepts — a comma-separated string, a space-separated string, or a YAML +list. Whitespace separates tokens only outside parentheses, so a bounded grant +like `Bash(git push *)` stays one entry. > **Not the same field as a skill's `disallowed-tools:`.** A skill's fence is > kebab-case and lives in `SKILL.md`; it is read by diff --git a/src/adapters/claude-code/agent-runtime.test.ts b/src/adapters/claude-code/agent-runtime.test.ts index b0cf43a2..dee8ef08 100644 --- a/src/adapters/claude-code/agent-runtime.test.ts +++ b/src/adapters/claude-code/agent-runtime.test.ts @@ -964,3 +964,21 @@ test("agent-hook CLI: PreToolUse(Task) with an unknown subagent activates nothin cleanupTmpDir(dir); } }); + +test("a SPACE-separated tools: contract grants those tools at the rail (#217)", () => { + // The second, worse half of #217, on the surface where the contract is + // ENFORCED rather than reported. A skill's `allowed-tools:` only pre-approves, + // so a mis-split there misleads an audit; a SUBAGENT's `tools:` is the + // allowlist this rail denies against — so when the space-separated spelling + // arrived as the single token "Read Grep Glob", `includes(tool)` matched + // nothing and the agent was denied EVERY tool, including the three it was + // explicitly granted. Measured before the fix: Read DENY, Grep DENY, Bash DENY. + const md = + "---\nname: reader\ndescription: reads\ntools: Read Grep Glob\n---\nbody\n"; + const allowed = parseAgentTools(md); + assert.deepEqual(allowed, ["Read", "Grep", "Glob"]); + assert.equal(decidePreToolUse(allowed, "Read").allow, true); + assert.equal(decidePreToolUse(allowed, "Grep").allow, true); + // …and the fence still holds for what was NOT granted. + assert.equal(decidePreToolUse(allowed, "Bash").allow, false); +}); diff --git a/src/core/frontmatter-read.test.ts b/src/core/frontmatter-read.test.ts index c300a7d7..bd08a206 100644 --- a/src/core/frontmatter-read.test.ts +++ b/src/core/frontmatter-read.test.ts @@ -29,6 +29,97 @@ test("a comma-list tool value splits", () => { assert.deepEqual(frontmatterList(fm, "tools"), ["Read", "Grep", "Bash"]); }); +test("every separator Claude Code documents splits the same way (#217)", () => { + // Claude Code accepts a comma-separated string, a SPACE-separated string, or a + // YAML list, and treats all three as equivalent. Until #217 only two of the + // three worked here: a space-separated fence arrived as one token matching no + // built-in, so a fence the harness really enforces audited as closing nothing. + // Reported with this table by @vlad-ryzhkov, measured against a real harness. + const read = (v: string): string[] | null => + frontmatterList( + readFrontmatter(`---\nname: a\ndisallowed-tools: ${v}\n---\n`), + "disallowed-tools", + ); + assert.deepEqual(read("WebFetch, WebSearch"), ["WebFetch", "WebSearch"]); + assert.deepEqual(read("WebFetch WebSearch"), ["WebFetch", "WebSearch"]); + assert.deepEqual(read("WebFetch, WebSearch ,Bash"), [ + "WebFetch", + "WebSearch", + "Bash", + ]); + // A YAML-quoted scalar whose VALUE is a space-separated list — common in the + // wild, and the shape that made this bug invisible: the quotes are YAML syntax, + // not token boundaries, so they are stripped and the value still splits. + assert.deepEqual(read('"Read Write Glob"'), ["Read", "Write", "Glob"]); +}); + +test("whitespace splits ONLY outside parens, so a bounded grant survives (#217)", () => { + // The half that makes the naive `split(/[,\s]+/)` wrong: `Bash(git push *)` must + // stay ONE token. Shredded, it matches no grant, and `bashGrantIsUnbounded()` + // answers "unbounded" when it cannot recognise one — so the fix for a + // false-CLEAN verdict would have bought a false-EXPOSED one. + const read = (v: string): string[] | null => + frontmatterList( + readFrontmatter(`---\nname: a\nallowed-tools: ${v}\n---\n`), + "allowed-tools", + ); + assert.deepEqual(read("Bash(git add *) Bash(git commit *)"), [ + "Bash(git add *)", + "Bash(git commit *)", + ]); + assert.deepEqual(read("Bash(git push *), WebFetch"), [ + "Bash(git push *)", + "WebFetch", + ]); + assert.deepEqual(read("[Read, Grep]"), ["Read", "Grep"]); + // An UNCLOSED paren keeps the rest as one token rather than fragmenting it into + // garbage tool names — the conservative side of a shape we cannot parse. + assert.deepEqual(read("Bash(git push Read"), ["Bash(git push Read"]); +}); + +test("a parenthesis INSIDE a quoted string is a character, not a bracket (#217)", () => { + // Found by the Codex review bot on the first cut of this tokenizer. A literal + // unmatched paren inside a grant's own command left depth at 1 after the grant + // closed, so every following token was swallowed into it — and on a SUBAGENT + // that denies a tool the author granted, the exact failure this function + // exists to fix. Measured before the fix: one token, `Read` lost. + const read = (v: string): string[] | null => + frontmatterList( + readFrontmatter(`---\nname: a\ntools: ${v}\n---\n`), + "tools", + ); + assert.deepEqual(read("Bash(printf '( %s' foo) Read"), [ + "Bash(printf '( %s' foo)", + "Read", + ]); + // Balanced parens inside quotes self-corrected even before the fix; pinned so + // the common `git commit -m "feat(api): …"` shape cannot regress either. + assert.deepEqual(read('Bash(git commit -m "feat(api): x") Read'), [ + 'Bash(git commit -m "feat(api): x")', + "Read", + ]); + // 🔴 THE COUNTERWEIGHT, and why quotes gate ONLY the depth counter: if being + // inside quotes also suppressed SPLITTING, a quoted list would collapse back + // to one token — reintroducing the very bug #217 reports. + // + // It must be asserted on a MALFORMED block, and that is the whole point. On + // valid YAML js-yaml strips the quotes before `splitList` ever runs, so the + // value arrives as bare `Read Write Glob` and NO quote character is present — + // measured, after the first version of this assertion used a valid block and + // stayed green under the widening mutation, carrying zero information. Quotes + // reach the splitter only down the regex SALVAGE path, so that is where the + // property lives. (`desc:` below carries an unescaped `: ` — invalid YAML.) + const salvaged = frontmatterList( + readFrontmatter( + '---\nname: a\ndesc: Use the foo: bar tool\ntools: "Read Write Glob"\n---\n', + ), + "tools", + ); + assert.deepEqual(salvaged, ["Read", "Write", "Glob"]); + // …and the same value on the valid-YAML path, where the quotes are gone by then. + assert.deepEqual(read('"Read Write Glob"'), ["Read", "Write", "Glob"]); +}); + test("absent list key → null (inherits all); present-but-empty → [] (no tools)", () => { const none = readFrontmatter("---\nname: a\n---\n"); assert.equal(frontmatterList(none, "tools"), null); diff --git a/src/core/frontmatter-read.ts b/src/core/frontmatter-read.ts index d8fc048b..1fea4d53 100644 --- a/src/core/frontmatter-read.ts +++ b/src/core/frontmatter-read.ts @@ -146,11 +146,79 @@ function salvageList(block: string, key: string): string[] | null { return splitList(match[1]); } -/** Split a comma list or inline-array string into trimmed, de-quoted tokens. */ +/** + * Split a tool-list string into trimmed, de-quoted tokens. + * + * WHITESPACE IS A SEPARATOR, BUT ONLY AT PAREN DEPTH 0 — and that qualifier is + * the whole rule. Claude Code documents three equivalent spellings of + * `allowed-tools:`/`disallowed-tools:` (a comma-separated string, a + * SPACE-separated string, a YAML list), and its own documented example is + * space-separated with spaces INSIDE the tokens: + * + * allowed-tools: Bash(git add *) Bash(git commit *) Bash(git status *) + * + * Until #217 this split on `,` alone, so a space-separated fence arrived as ONE + * token that matches no built-in name. That failed in the direction that misleads: + * a fence the harness really enforces was reported as closing no lethal-trifecta + * leg, so an author who wrote a correct fence was told it did nothing. Reported + * with a reproduction by @vlad-ryzhkov, measured on a 22-skill harness where the + * separator alone moved Safety 81 → 80 → 81. + * + * THE NAIVE FIX IS WORSE THAN THE BUG, which is why this is a tokenizer and not + * `split(/[,\s]+/)`: that shreds `Bash(git push *)` into `Bash(git`, `push`, `*)`, + * and since `bashGrantIsUnbounded()` answers "unbounded" when it cannot recognise + * a grant, a BOUNDED grant would start reading as an unbounded one. That is the + * false-exposed side of the trade `core/lethal-trifecta.ts` already reasons about + * — a scarier verdict for a reason the author cannot see anywhere in their file. + * + * Quotes are stripped per token AFTER splitting, never treated as a delimiter: + * `allowed-tools: "Read Write Glob"` is a YAML-quoted scalar whose VALUE is a + * space-separated list, and Claude Code splits it. Treating the quotes as token + * boundaries would keep exactly the bug this fixes for every skill that quotes + * its list. + */ function splitList(raw: string): string[] { - return raw - .replace(/^\[|\]$/g, "") - .split(",") - .map((t) => t.trim().replace(/^["']|["']$/g, "")) - .filter((t) => t.length > 0); + const out: string[] = []; + let token = ""; + let depth = 0; + let quote: '"' | "'" | null = null; + let escaped = false; + const flush = (): void => { + const t = token.trim().replace(/^["']|["']$/g, ""); + if (t.length > 0) out.push(t); + token = ""; + }; + for (const ch of raw.trim().replace(/^\[|\]$/g, "")) { + if (escaped) { + escaped = false; + token += ch; + continue; + } + if (ch === "\\" && quote !== "'") { + escaped = true; + token += ch; + continue; + } + if (quote === null && (ch === '"' || ch === "'")) quote = ch; + else if (quote === ch) quote = null; + // Depth tracks STRUCTURE, so a parenthesis inside a quoted string is a + // character, not a bracket. Without this, `Bash(printf '( %s' foo) Read` + // leaves depth at 1 after the grant closes and swallows `Read` into the + // same token — on a subagent that denies a tool the author granted, which + // is the exact failure this function exists to fix. + else if (quote === null) { + if (ch === "(") depth++; + else if (ch === ")") depth = Math.max(0, depth - 1); + } + // Quotes deliberately do NOT suppress splitting: `allowed-tools: "Read + // Write Glob"` is a YAML-quoted scalar whose VALUE is a space-separated + // list, and Claude Code splits it. The quotes come off per token below. + if (depth === 0 && (ch === "," || /\s/.test(ch))) { + flush(); + continue; + } + token += ch; + } + flush(); + return out; } diff --git a/src/scan.test.ts b/src/scan.test.ts index 2ef7703c..e780b1c2 100644 --- a/src/scan.test.ts +++ b/src/scan.test.ts @@ -1745,6 +1745,23 @@ test("a disallowed-tools that CLOSES a leg clears the finding", () => { cleanupTmpDir(dir); }); +test("the SPACE-separated spelling of that same fence also clears it (#217)", () => { + const dir = makeTmpDir("scan-trifecta-fenced-spaces"); + // Byte-for-byte the fence above with the commas removed — the spelling Claude + // Code documents and honours. It used to read as ONE token named + // "WebFetch WebSearch Bash", matching no built-in, so every leg still counted as + // supplied and the author was told their working fence closed nothing (#217). + write( + dir, + "skills/fenced/SKILL.md", + "---\nname: fenced\ndescription: A model-invocable skill that fences off the network entirely\ndisallowed-tools: WebFetch WebSearch Bash\n---\n# fenced\n", + ); + const r = scanPlugin(dir); + assert.equal(r.skills.find((s) => s.name === "fenced")?.trifecta, null); + assert.equal(r.trifectaFindings.length, 0); + cleanupTmpDir(dir); +}); + test("a PARTIAL disallowed-tools closes no leg and names the suppliers still standing", () => { const dir = makeTmpDir("scan-trifecta-partial-fence"); // Denying `Read` alone leaves Grep/Glob/Bash on the private-data leg — an author