Constrain built-skill allowed-tools to the approved plan

[steve wu (wuwangzhang1216)]

Fixes #8.

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:

allowedTools: submission.allowedTools.length ? submission.allowedTools : plan.allowedTools,

So a skill could be installed carrying tools the reviewer never approved.

Why not set-membership

The issue suggests filtering by plan.allowedTools.includes(t). That closes the escalation, but allowed-tools entries are patterns, not fixed strings — and the comment right above this line, plus the builder instructions, deliberately let the agent tighten the grant to the steps it actually emitted. Set-membership rejects every one of those narrowings and falls back to the broader approved pattern:

	approved ["Bash(gh *)"], submitted ["Bash(gh pr list)", "Bash(rm -rf /)"]
current main	["Bash(gh pr list)", "Bash(rm -rf /)"] — rm -rf reaches the frontmatter
set-membership	["Bash(gh *)"] — safe, but the valid narrowing is discarded and the grant ends up wider than the agent asked for
this PR	["Bash(gh pr list)"], dropped ["Bash(rm -rf /)"]

So this decides pattern subsumption instead: does the submitted pattern grant anything the approved pattern does not?

What this adds

common/allowed-tools.ts — pure, no Electron imports, so the trust boundary is unit-testable in isolation. It 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    (a bare name carries no argument restriction)
Bash              ⊄ Bash(gh *)   dropped (drops a restriction the user approved)

A few decisions worth flagging for review:

• * is the only wildcard; everything else is literal, so a regex metacharacter in a command can't silently widen the match (echo axb ⊄ echo a.b).
• Tool names compare case-insensitively, argument patterns don't. Tool names are identifiers, so a case slip should cost a narrowing rather than widen anything; shell arguments stay case-sensitive because GH is not the gh CLI.
• 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 was approved. The one case that stays empty is an approved list that was itself empty: no explicit pattern can be proven narrower than a default set whose contents we don't know.

electron/skillbuilder/builder.ts calls it and logs anything dropped.

Tests

common/allowed-tools.test.ts — 10 cases covering narrowing, escalation, the bare-name form, literal metacharacters, the case-sensitivity split, blank/duplicate entries, and both empty-list fallbacks. Added to npm test.

The narrowing test is the specific regression that set-membership would fail, so it pins the behaviour the contract asks for.

Coverage boundary

The merge logic is fully covered, but the wiring in SkillBuilder.create() is not: it needs a live CopilotClient, and making it unit-testable would mean injecting the client — out of scope here. Worth a follow-up if you'd like the create turn under test end to end.

Validation

• tsc --noEmit — clean (common is in the tsconfig include, so the new files are covered).
• npm test — the 10 new tests pass, no regressions in the rest of the suite.

One note on running the suite locally: electron/recorder/controller.test.ts fails for me on Node 26, unrelated to this change (it fails the same way on a clean tree). Node 26 removed --experimental-transform-types, and strip-only mode rejects the parameter property at controller.ts:111. On Node 24 — what the README and CI use — it's fine. Happy to file that separately if it's useful.

[steve wu (wuwangzhang1216)]

@microsoft-github-policy-service agree