From a8067b74bf92c5704ce9040168185da15ae7cf4e Mon Sep 17 00:00:00 2001 From: wuwangzhang1216 Date: Mon, 3 Aug 2026 23:10:29 -0400 Subject: [PATCH] Constrain built-skill allowed-tools to the approved plan `allowed-tools` is the capability grant on an installed SKILL.md. The user approves it on the plan-review screen, but the builder's create turn is an LLM call whose returned list was written into the frontmatter verbatim, so a skill could be installed carrying tools the reviewer never approved. Set-membership is not the right check. The builder's contract deliberately lets the agent tighten the grant to the steps it actually emitted (approved `Bash(gh *)` -> submitted `Bash(gh pr list)`). Comparing strings rejects every such narrowing and falls back to the broader approved pattern, disabling the one behaviour the contract asks for. Decide pattern subsumption instead. common/allowed-tools.ts is pure and errs toward refusal: anything unparseable, ambiguous, or not provably covered is treated as not covered. Bash(gh pr list) <= Bash(gh *) kept (narrowing) Bash(*) !> Bash(gh *) dropped (escalation) Bash(gh *) <= Bash kept (bare name is unrestricted) Bash !> Bash(gh *) dropped (drops an approved restriction) Argument patterns treat `*` as the only wildcard and everything else as literal, so a regex metacharacter in a command cannot silently widen the match. Tool names compare case-insensitively -- a case slip should cost a narrowing rather than widen anything -- while argument patterns stay case-sensitive, because shell commands are. When nothing survives, the approved patterns are re-asserted rather than emitting an empty list: an omitted `allowed-tools` means "use the agent's default set", which may well be wider than what the user approved. Fixes #8. --- common/allowed-tools.test.ts | 146 +++++++++++++++++++++++++++ common/allowed-tools.ts | 168 +++++++++++++++++++++++++++++++ electron/skillbuilder/builder.ts | 21 +++- package.json | 2 +- 4 files changed, 332 insertions(+), 5 deletions(-) create mode 100644 common/allowed-tools.test.ts create mode 100644 common/allowed-tools.ts diff --git a/common/allowed-tools.test.ts b/common/allowed-tools.test.ts new file mode 100644 index 0000000..1f8e108 --- /dev/null +++ b/common/allowed-tools.test.ts @@ -0,0 +1,146 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + constrainAllowedTools, + isSubsumedBy, + parseToolPattern, +} from "./allowed-tools"; + +test("tool patterns parse into a tool name and an optional argument pattern", () => { + // A bare tool name places no restriction on arguments. + assert.deepEqual(parseToolPattern("Read"), { tool: "Read", arg: null }); + // The common gated-shell shape. + assert.deepEqual(parseToolPattern("Bash(gh *)"), { tool: "Bash", arg: "gh *" }); + // Surrounding whitespace is incidental formatting, never part of the grant. + assert.deepEqual(parseToolPattern(" Bash( gh * ) "), { tool: "Bash", arg: "gh *" }); + // A command may legitimately contain parentheses; only the outermost pair delimits. + assert.deepEqual(parseToolPattern("Bash(echo (hi))"), { tool: "Bash", arg: "echo (hi)" }); + // Unparseable input is not a grant — callers must drop it rather than guess. + assert.equal(parseToolPattern(""), null); + assert.equal(parseToolPattern(" "), null); + assert.equal(parseToolPattern("(gh *)"), null); +}); + +test("narrowing an approved pattern is preserved", () => { + // THE REGRESSION THIS MODULE EXISTS FOR. The builder's contract explicitly allows the + // agent to tighten allowed-tools to the steps it actually emitted. A plain string + // comparison would reject every one of these, silently discarding the narrowing and + // falling back to the broader approved pattern. + assert.equal(isSubsumedBy("Bash(gh pr list)", "Bash(gh *)"), true); + assert.equal(isSubsumedBy("Bash(gh pr *)", "Bash(gh *)"), true); + assert.equal(isSubsumedBy("Bash(gh issue create --title x)", "Bash(gh *)"), true); + // Self-subsumption: an unchanged pattern is trivially within itself. + assert.equal(isSubsumedBy("Bash(gh *)", "Bash(gh *)"), true); + assert.equal(isSubsumedBy("Read", "Read"), true); +}); + +test("broadening past the approved pattern is rejected", () => { + // The whole point of the gate: the model must not hand itself a wider shell. + assert.equal(isSubsumedBy("Bash(*)", "Bash(gh *)"), false); + assert.equal(isSubsumedBy("Bash(rm -rf /)", "Bash(gh *)"), false); + assert.equal(isSubsumedBy("Bash(git push)", "Bash(gh *)"), false); + // Subsumption is directional — a wider pattern is not covered by a narrower one. + assert.equal(isSubsumedBy("Bash(gh *)", "Bash(gh pr *)"), false); + // A different tool is never covered, however similar the argument pattern. + assert.equal(isSubsumedBy("Write", "Read"), false); + assert.equal(isSubsumedBy("Bash(gh *)", "Shell(gh *)"), false); +}); + +test("a bare tool name is the unrestricted form of that tool", () => { + // `Bash` places no argument restriction, so it covers any gated form of itself. + assert.equal(isSubsumedBy("Bash(gh *)", "Bash"), true); + assert.equal(isSubsumedBy("Bash(*)", "Bash"), true); + // …and conversely, dropping the restriction is a broadening, so it must be refused. + assert.equal(isSubsumedBy("Bash", "Bash(gh *)"), false); +}); + +test("argument patterns match literally except for `*`", () => { + // Regex metacharacters in a command must not silently widen the match: `a.b` is a + // literal dot, so `axb` is a different command and must not be treated as covered. + assert.equal(isSubsumedBy("Bash(echo a.b)", "Bash(echo a.b)"), true); + assert.equal(isSubsumedBy("Bash(echo axb)", "Bash(echo a.b)"), false); + assert.equal(isSubsumedBy("Bash(echo a+b)", "Bash(echo a+b)"), true); + // `*` is the only wildcard, and it may appear anywhere in the pattern. + assert.equal(isSubsumedBy("Bash(gh pr list --json x)", "Bash(gh * --json *)"), true); + assert.equal(isSubsumedBy("Bash(gh pr list --yaml x)", "Bash(gh * --json *)"), false); +}); + +test("tool names compare case-insensitively, argument patterns do not", () => { + // Tool names are identifiers — a case slip should not silently drop a valid narrowing. + assert.equal(isSubsumedBy("bash(gh pr list)", "Bash(gh *)"), true); + // Shell arguments are case-sensitive: `GH` is not the `gh` CLI. + assert.equal(isSubsumedBy("Bash(GH pr list)", "Bash(gh *)"), false); +}); + +test("an approved literal cannot partially match the wildcard sentinel", () => { + // The submitted `*` is substituted with a sentinel before matching. If that sentinel + // were multi-character, an approved pattern whose literal segment is a PREFIX of it + // would match part of the substitution and wrongly report the grant as covered — + // failing OPEN, the one direction this must never fail in. These are the cases that + // caught it; they only pass while the sentinel is a single character. + assert.equal(isSubsumedBy("Bash(a*)", "Bash(a@@skill*)"), false); + assert.equal(isSubsumedBy("Bash(x*)", "Bash(x@@*)"), false); + assert.equal(isSubsumedBy("Bash(*)", "Bash(@@skill-recorder-wildcard*)"), false); + // A literal that merely starts the same way is not coverage either. + assert.equal(isSubsumedBy("Bash(deploy *)", "Bash(deploy-prod *)"), false); +}); + +test("constrain keeps the covered patterns and reports the rest", () => { + // The realistic case: the agent narrows one step correctly and invents another. + const result = constrainAllowedTools( + ["Bash(gh pr list)", "Bash(rm -rf /)", "Read"], + ["Bash(gh *)", "Read", "Write"], + ); + assert.deepEqual(result.allowed, ["Bash(gh pr list)", "Read"]); + assert.deepEqual(result.dropped, ["Bash(rm -rf /)"]); +}); + +test("constrain falls back to the approved set rather than emitting an empty list", () => { + // An omitted `allowed-tools` means "use the agent's DEFAULT set", which can be WIDER + // than what the human approved. So when nothing survives, we must re-assert the + // approved patterns instead of shipping an empty (= unrestricted-by-default) list. + const rejected = constrainAllowedTools(["Bash(rm -rf /)"], ["Bash(gh *)"]); + assert.deepEqual(rejected.allowed, ["Bash(gh *)"]); + assert.deepEqual(rejected.dropped, ["Bash(rm -rf /)"]); + + // The agent declining to restate the tools is not a narrowing — keep what was approved. + const empty = constrainAllowedTools([], ["Bash(gh *)"]); + assert.deepEqual(empty.allowed, ["Bash(gh *)"]); + assert.deepEqual(empty.dropped, []); +}); + +test("an approved-empty plan stays on the default set", () => { + // The plan declared no `allowed-tools`, so the human approved "whatever the agent + // normally has". We cannot prove an explicit pattern is narrower than an unknown + // default set, so nothing is accepted and the frontmatter stays omitted. + const result = constrainAllowedTools(["Bash(gh *)"], []); + assert.deepEqual(result.allowed, []); + assert.deepEqual(result.dropped, ["Bash(gh *)"]); +}); + +test("constrain is robust to blank and duplicate entries", () => { + // Blank entries carry no grant and must not survive into the frontmatter. + const result = constrainAllowedTools( + ["Bash(gh pr list)", " ", "Bash(gh pr list)"], + ["Bash(gh *)"], + ); + assert.deepEqual(result.allowed, ["Bash(gh pr list)"]); + assert.deepEqual(result.dropped, []); +}); + +test("patterns that differ only in spelling collapse to one entry", () => { + // Same grant, different whitespace or tool-name casing — emitting all of them would + // put visibly redundant lines in the SKILL.md frontmatter. The first spelling wins. + const result = constrainAllowedTools( + ["Bash(gh pr list)", "Bash( gh pr list )", "bash(gh pr list)"], + ["Bash(gh *)"], + ); + assert.deepEqual(result.allowed, ["Bash(gh pr list)"]); + + // Argument patterns stay case-sensitive, so these are genuinely different grants and + // must not be collapsed — only the one the plan covers survives. + const cased = constrainAllowedTools(["Bash(gh pr)", "Bash(GH pr)"], ["Bash(gh *)"]); + assert.deepEqual(cased.allowed, ["Bash(gh pr)"]); + assert.deepEqual(cased.dropped, ["Bash(GH pr)"]); +}); diff --git a/common/allowed-tools.ts b/common/allowed-tools.ts new file mode 100644 index 0000000..24ce7c5 --- /dev/null +++ b/common/allowed-tools.ts @@ -0,0 +1,168 @@ +/** + * `allowed-tools` is the capability grant on a built `SKILL.md`: a list of tool + * patterns (`Bash(gh *)`, `Read`, `Write`) the skill may use. The human approves that + * list on the plan-review screen, and the builder's *create* turn is an LLM call that + * may return its own list — so the plan gate only means something if the submitted + * list is provably **no wider** than the approved one. + * + * The builder's contract deliberately lets the agent *tighten* the grant to the steps + * it actually emitted (approved `Bash(gh *)` → submitted `Bash(gh pr list)`). That + * rules out a plain string comparison: set-membership would reject every narrowing and + * silently fall back to the broader approved pattern, quietly disabling the one + * behaviour the contract asks for. So this module decides **pattern subsumption**: + * does the submitted pattern grant anything the approved pattern does not? + * + * Everything here is pure and deterministic so the trust boundary is unit-testable in + * isolation, and it errs toward refusal: anything unparseable, ambiguous, or not + * provably covered is treated as *not* covered. + */ + +/** A parsed `allowed-tools` entry. `arg === null` means the bare, unrestricted form. */ +export interface ToolPattern { + tool: string; + arg: string | null; +} + +/** + * Split `Tool(argument pattern)` / `Tool` into its parts, or null when the text is not + * a usable grant. Only the outermost parentheses delimit, so a command may contain its + * own — `Bash(echo (hi))` yields the argument `echo (hi)`. + */ +export function parseToolPattern(raw: string): ToolPattern | null { + const text = raw.trim(); + if (!text) return null; + + const open = text.indexOf("("); + if (open < 0) { + // A bare name with a stray closing paren is malformed, not a grant. + return text.includes(")") ? null : { tool: text, arg: null }; + } + // The argument pattern must close at the very end; anything else is malformed. + if (!text.endsWith(")")) return null; + + const tool = text.slice(0, open).trim(); + if (!tool) return null; + return { tool, arg: text.slice(open + 1, -1).trim() }; +} + +/** Characters that must survive as literals when an argument pattern becomes a regex. */ +const REGEX_METACHARACTERS = /[.*+?^${}()|[\]\\]/g; + +/** + * Stands in for the submitted pattern's own `*` while it is tested against the approved + * pattern. + * + * It **must be a single character**. A multi-character sentinel can be partially matched + * by an approved pattern whose literal segment happens to be a prefix of it — e.g. with + * a `@@wildcard@@` sentinel, `Bash(a*)` would be reported as covered by `Bash(a@@w*)`, + * because the `includes` guard below only catches the *whole* sentinel. That fails open, + * which is the one direction this module must never fail in. A single character cannot + * be partially matched: an approved literal segment either contains it (refused below) + * or cannot touch it at all. + * + * U+E000 is in a Unicode private-use area, so no real tool pattern contains it. Built + * from a code point rather than an escape so the source stays plain ASCII. + */ +const WILDCARD_SENTINEL = String.fromCodePoint(0xe000); + +function escapeRegExp(text: string): string { + return text.replace(REGEX_METACHARACTERS, "\\$&"); +} + +/** Compile an argument pattern, treating `*` as the only wildcard and the rest as literal. */ +function argumentMatcher(pattern: string): RegExp { + const source = pattern.split("*").map(escapeRegExp).join("[\\s\\S]*"); + return new RegExp(`^${source}$`); +} + +/** + * True when every command `child` admits is also admitted by `parent`. + * + * `child`'s own wildcards become a sentinel before matching, so a `*` in `child` is only + * covered where `parent` also has a `*` at that position: `gh pr *` ⊆ `gh *`, but + * `*` ⊄ `gh *` — which is exactly the escalation to refuse. + */ +function argumentSubsumes(child: string, parent: string): boolean { + if (child === parent) return true; + // Refuse rather than reason about input that already contains the sentinel. + if (child.includes(WILDCARD_SENTINEL) || parent.includes(WILDCARD_SENTINEL)) return false; + return argumentMatcher(parent).test(child.split("*").join(WILDCARD_SENTINEL)); +} + +/** + * True when `child` grants nothing beyond `parent` — i.e. `child` is a safe narrowing + * (or an exact restatement) of the approved `parent`. + * + * Tool names compare case-insensitively: they are identifiers, and a case slip should + * cost a valid narrowing rather than silently widen anything. Argument patterns stay + * case-sensitive, because shell commands are (`GH` is not the `gh` CLI). + */ +export function isSubsumedBy(child: string, parent: string): boolean { + const narrow = parseToolPattern(child); + const broad = parseToolPattern(parent); + if (!narrow || !broad) return false; + if (narrow.tool.toLowerCase() !== broad.tool.toLowerCase()) return false; + // A bare approved tool carries no argument restriction, so it covers any gated form. + if (broad.arg === null) return true; + // The reverse drops a restriction the human approved — a broadening. + if (narrow.arg === null) return false; + return argumentSubsumes(narrow.arg, broad.arg); +} + +/** + * A spelling-insensitive key for the same grant, so `Bash(gh *)` and `Bash( gh * )` do + * not both reach the frontmatter. Mirrors the comparison rules in {@link isSubsumedBy}: + * the tool name folds case, the argument pattern does not. + */ +function canonicalKey(pattern: ToolPattern): string { + return pattern.arg === null + ? pattern.tool.toLowerCase() + : `${pattern.tool.toLowerCase()}(${pattern.arg})`; +} + +export interface ConstrainedAllowedTools { + /** The patterns to write into the skill's frontmatter. */ + allowed: string[]; + /** Submitted patterns refused because the approved plan does not cover them. */ + dropped: string[]; +} + +/** + * Constrain an agent-submitted `allowed-tools` list to what the human actually approved. + * Narrowing is kept, broadening is dropped and reported for logging. + * + * When nothing survives, this re-asserts the **approved** patterns instead of emitting + * an empty list: an omitted `allowed-tools` means "use the agent's default set", which + * may well be wider than what was approved — so an empty result would turn a refused + * escalation into an unrestricted skill. An `approved` list that is itself empty is the + * one case that stays empty: the human approved the default set, and no explicit pattern + * can be proven narrower than a set whose contents we do not know. + */ +export function constrainAllowedTools( + submitted: readonly string[], + approved: readonly string[], +): ConstrainedAllowedTools { + const approvedPatterns = approved.map((t) => t.trim()).filter(Boolean); + const seen = new Set(); + const allowed: string[] = []; + const dropped: string[] = []; + + for (const raw of submitted) { + const text = raw.trim(); + // A blank entry carries no grant, so it is neither kept nor worth reporting. + if (!text) continue; + const parsed = parseToolPattern(text); + // Unparseable text has no canonical form; key it by its own spelling so it is still + // reported once rather than silently collapsed with something else. + const key = parsed ? canonicalKey(parsed) : text; + if (seen.has(key)) continue; + seen.add(key); + if (approvedPatterns.some((pattern) => isSubsumedBy(text, pattern))) allowed.push(text); + else dropped.push(text); + } + + if (allowed.length === 0 && approvedPatterns.length > 0) { + return { allowed: approvedPatterns, dropped }; + } + return { allowed, dropped }; +} diff --git a/electron/skillbuilder/builder.ts b/electron/skillbuilder/builder.ts index 6041f0f..bbaa4cb 100644 --- a/electron/skillbuilder/builder.ts +++ b/electron/skillbuilder/builder.ts @@ -15,6 +15,7 @@ import { type SkillPlan, type SkillSubmission, } from "../../common/skill"; +import { constrainAllowedTools } from "../../common/allowed-tools"; import { unresolvedTokens } from "../../common/values"; import type { SkillBuildInput, SkillBuildProgress } from "../../common/ipc"; import { AgentBuilder, type BaseLive } from "../builders/agent-builder"; @@ -172,13 +173,25 @@ export class SkillBuilder extends AgentBuilder { if (unknownTokens.length) { log.warn(`skill body references unknown value tokens: ${unknownTokens.map((t) => `{{${t}}}`).join(", ")}`); } - // The frontmatter comes from the edited plan (authoritative); only the body is - // the agent's generated prose. allowed-tools may be tightened by the agent to the - // final steps, but never emptied below what the plan declared. + // The frontmatter comes from the edited plan (authoritative); only the body is the + // agent's generated prose. allowed-tools is the capability grant the user approved + // on the plan screen, so the agent may narrow it to the steps it actually emitted + // but must never widen it: anything the approved plan does not cover is dropped + // here rather than written into an installed skill's frontmatter. + const { allowed: allowedTools, dropped: refusedTools } = constrainAllowedTools( + submission.allowedTools, + plan.allowedTools, + ); + if (refusedTools.length) { + log.warn( + "skill requested allowed-tools outside the approved plan; dropped: " + + refusedTools.join(", "), + ); + } const finalSubmission: SkillSubmission = { name: plan.name, description: plan.description, - allowedTools: submission.allowedTools.length ? submission.allowedTools : plan.allowedTools, + allowedTools, body: submission.body, }; const built = toBuiltSkill(sessionId, plan.architecture, finalSubmission, plan); diff --git a/package.json b/package.json index 7aa16a2..46c880e 100644 --- a/package.json +++ b/package.json @@ -14,7 +14,7 @@ "build": "tsc --noEmit && vite build", "typecheck": "tsc --noEmit", "typecheck:evals": "tsc --noEmit -p evals/tsconfig.json", - "test": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs --test common/audio.test.ts common/microphone.test.ts common/narration.test.ts electron/recording-controls-bounds.test.ts electron/recording-privacy.test.ts electron/recorder/controller.test.ts electron/frames/extractor.test.ts electron/narration/audio-analysis.test.ts electron/narration/analyze-gate.test.ts electron/narration/transcribe.test.ts electron/narration/whisper.test.ts electron/sessions.test.ts electron/debug-bundle.test.ts scripts/compliance.test.mjs", + "test": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs --test common/allowed-tools.test.ts common/audio.test.ts common/microphone.test.ts common/narration.test.ts electron/recording-controls-bounds.test.ts electron/recording-privacy.test.ts electron/recorder/controller.test.ts electron/frames/extractor.test.ts electron/narration/audio-analysis.test.ts electron/narration/analyze-gate.test.ts electron/narration/transcribe.test.ts electron/narration/whisper.test.ts electron/sessions.test.ts electron/debug-bundle.test.ts scripts/compliance.test.mjs", "eval": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs evals/run.ts", "eval:builder": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs evals/builder/run.ts", "eval:skill": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs evals/skillbuilder/run.ts",