Constrain built-skill allowed-tools to the approved plan

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)"]);
+});

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<string>();
+  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 };
+}

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<LiveBuild> {
       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);

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",