diff --git a/scripts/lib/plugin-paths.js b/scripts/lib/plugin-paths.js index a99982340..8ad448c42 100644 --- a/scripts/lib/plugin-paths.js +++ b/scripts/lib/plugin-paths.js @@ -1,5 +1,6 @@ import fs from 'fs'; import path from 'path'; +import { generateYamlFrontmatter, parseFrontmatter } from './utils.js'; /** * Rewrite project-relative script paths for the plugin subtree (issue #523). @@ -26,15 +27,19 @@ export const CLAUDE_PROJECT_SCRIPTS_PATH = '.claude/skills/impeccable/scripts'; export const PLUGIN_SCRIPTS_PATH = '/scripts'; -// The project-path rule pre-approves a path inside the user's project, the -// one place the plugin must NOT run scripts from. No replacement rule -// exists: a wildcard pattern such as `node */skills/impeccable/scripts/*` -// auto-approves any same-shaped path anywhere on disk, and frontmatter has -// no variable bound to the loaded plugin root (CLAUDE_PLUGIN_ROOT is -// hook-only). The plugin copy drops the rule and script runs go through -// the normal Bash confirmation. +// Claude Code requires user consent to activate a skill whose frontmatter +// declares allowed-tools; non-interactive hosts (`claude -p`) cannot provide +// it and the skill silently degrades (issue #736). The plugin copy drops the +// key via structured frontmatter rewrite, not regex. export const PROJECT_ALLOWED_TOOLS_LINE = ` - Bash(${CLAUDE_PROJECT_SCRIPTS_PATH}/impeccable *)\n`; +function stripAllowedToolsFrontmatter(content) { + const { frontmatter, body } = parseFrontmatter(content); + if (frontmatter['allowed-tools'] === undefined) return content; + delete frontmatter['allowed-tools']; + return `${generateYamlFrontmatter(frontmatter)}\n${body}`; +} + // Setup step 1's second sentence names the project path as the fallback // when the runtime reports no base directory. A plugin install has no // working project fallback (that path is the bug this rewrite exists to @@ -72,10 +77,7 @@ export const AGENT_EMBED_FALLBACK = * unit suite can pin every rewrite without a build. */ export function rewritePluginMarkdown(content) { - return content - // Order matters: the allowed-tools line contains the project path, so - // remove it before the generic path replacement rewrites it into a - // line the removal no longer matches. + return stripAllowedToolsFrontmatter(content) .replaceAll(PROJECT_ALLOWED_TOOLS_LINE, '') .replaceAll(SETUP_FALLBACK_TEXT, SETUP_PLUGIN_TEXT) .replaceAll(CLAUDE_PROJECT_SCRIPTS_PATH, PLUGIN_SCRIPTS_PATH) @@ -154,10 +156,17 @@ export function verifyPluginSkillRewrite(skillMdPath) { 'scripts/lib/plugin-paths.js (issue #523); update SETUP_FALLBACK_TEXT to the new wording.', ); } + if (parseFrontmatter(content).frontmatter['allowed-tools'] !== undefined) { + throw new Error( + `Plugin rewrite drift: ${skillMdPath} still declares allowed-tools in frontmatter. ` + + 'The plugin copy must drop the entire block so non-interactive sessions can activate ' + + 'the skill (issue #736); update the removal in scripts/lib/plugin-paths.js.', + ); + } if (/Bash\((?:node |[^)]*scripts\/impeccable)/.test(content)) { throw new Error( `Plugin rewrite drift: ${skillMdPath} still pre-approves an engine launcher or node script path. ` + - "SKILL.src.md's allowed-tools entry no longer matches the removal in " + + 'A stray Bash(...) entry survived the allowed-tools removal in ' + 'scripts/lib/plugin-paths.js (issue #523); the plugin ships no launcher pre-approval.', ); } diff --git a/scripts/lib/transformers/providers.js b/scripts/lib/transformers/providers.js index 9a7591c17..5a72fb36b 100644 --- a/scripts/lib/transformers/providers.js +++ b/scripts/lib/transformers/providers.js @@ -29,7 +29,9 @@ export const PROVIDERS = { providerTags: ['claude-code', 'claude'], configDir: '.claude', displayName: 'Claude Code', - frontmatterFields: ['user-invocable', 'argument-hint', 'license', 'compatibility', 'metadata', 'allowed-tools'], + frontmatterFields: ['user-invocable', 'argument-hint', 'license', 'compatibility', 'metadata'], + // allowed-tools omitted: Claude Code blocks skill activation in non-interactive + // sessions when the field is present (issue #736). Other providers keep it. agentFormat: 'claude-md', emitHooks: 'claude', // Project-local Claude Code hooks live in `.claude/settings.json`. diff --git a/tests/plugin-paths.test.js b/tests/plugin-paths.test.js index ad6212822..e4aa2deef 100644 --- a/tests/plugin-paths.test.js +++ b/tests/plugin-paths.test.js @@ -15,6 +15,7 @@ import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; import fs from 'fs'; import os from 'os'; import path from 'path'; +import { parseFrontmatter } from '../scripts/lib/utils.js'; import { rewritePluginMarkdown, rewritePluginAgentMarkdown, @@ -66,20 +67,24 @@ describe('rewritePluginMarkdown', () => { ); }); - test('removes the node pre-approval instead of widening it', () => { - const frontmatter = [ + test('removes the entire allowed-tools frontmatter block', () => { + const input = [ + '---', + 'name: impeccable', 'allowed-tools:', ' - Bash(npx impeccable *)', ' - Bash(.claude/skills/impeccable/scripts/impeccable *)', + 'license: Apache 2.0', '---', '', + 'Body text.', ].join('\n'); - const output = rewritePluginMarkdown(frontmatter); - // The generic path rewrite alone would leave Bash(/scripts/impeccable *), - // a dead literal, and any wildcard replacement would auto-approve - // same-shaped paths outside the plugin. The line must go entirely. + const output = rewritePluginMarkdown(input); + expect(output).not.toMatch(/^allowed-tools:/m); expect(output).not.toContain('scripts/impeccable *'); - expect(output).toContain(' - Bash(npx impeccable *)\n---'); + expect(output).not.toContain('npx impeccable'); + expect(output).toContain('license: Apache 2.0'); + expect(output).toContain('Body text.'); }); test('drops the project-path fallback clause from Setup step 1', () => { @@ -256,9 +261,6 @@ describe('verifyPluginSkillRewrite', () => { }; const goodSkill = [ - 'allowed-tools:', - ' - Bash(.claude/skills/impeccable/scripts/impeccable *)', - '', '1. Run `/scripts/impeccable context` once per session, where `` is the ' + "loaded base directory the runtime reports for this skill; keep cwd at the user's project. " + 'That base directory resolves every `.claude/skills/impeccable/scripts/impeccable ` command in this skill ' + @@ -269,6 +271,7 @@ describe('verifyPluginSkillRewrite', () => { test('accepts a correctly rewritten SKILL.md', () => { const p = writeSkill(rewritePluginMarkdown(goodSkill)); expect(() => verifyPluginSkillRewrite(p)).not.toThrow(); + expect(fs.readFileSync(p, 'utf-8')).not.toMatch(/^allowed-tools:/m); }); test('fails the build when the Setup fallback sentence no longer matched', () => { @@ -280,22 +283,31 @@ describe('verifyPluginSkillRewrite', () => { }); test('fails the build when a launcher pre-approval survives the removal', () => { - const reworded = goodSkill.replace( - 'Bash(.claude/skills/impeccable/scripts/impeccable *)', - 'Bash(.claude/skills/impeccable/scripts/impeccable.cmd *)', + const p = writeSkill( + rewritePluginMarkdown(goodSkill) + '\n - Bash(/scripts/impeccable.cmd *)\n', ); - const p = writeSkill(rewritePluginMarkdown(reworded)); expect(() => verifyPluginSkillRewrite(p)).toThrow(/pre-approves an engine launcher/); }); + test('fails the build when allowed-tools frontmatter survives the removal', () => { + const p = writeSkill([ + '---', + 'name: impeccable', + 'allowed-tools:', + ' - Bash(npx impeccable *)', + 'license: Apache 2.0', + '---', + '', + rewritePluginMarkdown(goodSkill), + ].join('\n')); + expect(() => verifyPluginSkillRewrite(p)).toThrow(/allowed-tools/); + }); + test('fails the build when a legacy node pre-approval survives', () => { - // The Node-era line is gone from SKILL.src.md, but a copy that still - // carries one must fail the same way as a surviving launcher line. + // A copy that still carries a node pre-approval must fail the same way as + // a surviving launcher line, even outside an allowed-tools block. const p = writeSkill( - rewritePluginMarkdown(goodSkill).replace( - 'allowed-tools:\n', - 'allowed-tools:\n - Bash(node /scripts/*)\n', - ), + rewritePluginMarkdown(goodSkill) + '\n - Bash(node /scripts/*)\n', ); expect(() => verifyPluginSkillRewrite(p)).toThrow(/pre-approves an engine launcher or node script path/); }); @@ -310,3 +322,14 @@ describe('verifyPluginSkillRewrite', () => { expect(() => verifyPluginSkillRewrite(p)).toThrow(/still contains the project-relative scripts path/); }); }); + +describe('SKILL.src.md frontmatter', () => { + test('keeps allowed-tools in source for non-Claude providers (issue #736)', () => { + const src = fs.readFileSync( + path.join(import.meta.dirname, '../skill/SKILL.src.md'), + 'utf-8', + ); + const { frontmatter } = parseFrontmatter(src); + expect(frontmatter['allowed-tools']).toBeDefined(); + }); +});