mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-12 06:06:37 +03:00
Fix: Setup survives a refused launcher (#750)
Preserve Setup context and reference loading after launcher refusal, disclose the failure before editing, and limit Claude skill-directory substitution to SKILL.md. Add plugin-path and denied-launcher behavior regressions. Addresses part of #744 without closing its remaining scope. AI assistance: Cursor on the original contribution; Codex on maintainer-directed follow-up fixes and validation.
This commit is contained in:
Abdul Wahab
+32
-17
@@ -13,10 +13,12 @@ import { generateYamlFrontmatter, parseFrontmatter } from './utils.js';
|
||||
* project's (possibly older) skill copy.
|
||||
*
|
||||
* No literal path survives installation (the plugin cache location varies
|
||||
* per machine and per plugin version), so skill and reference markdown
|
||||
* uses the `<skill-base-dir>` form SKILL.md's Setup step 1 already leads
|
||||
* with: the runtime shows the skill's loaded base directory when it loads
|
||||
* the skill, and scripts resolve against that. Agent files cannot use the
|
||||
* per machine and per plugin version), so the plugin SKILL.md uses
|
||||
* ${CLAUDE_SKILL_DIR} (issue #744): the directory containing SKILL.md,
|
||||
* substituted by Claude Code at load time. Source uses `<skill-base-dir>`;
|
||||
* the rewrite maps it to the host variable only in SKILL.md. References
|
||||
* arrive through ordinary file reads and retain the explicit placeholder;
|
||||
* CLAUDE_SKILL_DIR is not an exported shell variable. Agent files cannot use either
|
||||
* token (a spawned agent never loads SKILL.md) and get the
|
||||
* ${CLAUDE_PLUGIN_ROOT} variable instead; see PLUGIN_AGENT_SCRIPTS_PATH.
|
||||
*/
|
||||
@@ -25,7 +27,7 @@ import { generateYamlFrontmatter, parseFrontmatter } from './utils.js';
|
||||
// Claude Code transformer's configDir (.claude) + skill name (impeccable).
|
||||
export const CLAUDE_PROJECT_SCRIPTS_PATH = '.claude/skills/impeccable/scripts';
|
||||
|
||||
export const PLUGIN_SCRIPTS_PATH = '<skill-base-dir>/scripts';
|
||||
export const PLUGIN_SCRIPTS_PATH = '${CLAUDE_SKILL_DIR}/scripts';
|
||||
|
||||
// Claude Code requires user consent to activate a skill whose frontmatter
|
||||
// declares allowed-tools; non-interactive hosts (`claude -p`) cannot provide
|
||||
@@ -43,13 +45,14 @@ function stripAllowedToolsFrontmatter(content) {
|
||||
// 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
|
||||
// fix), and every instruction in the plugin copy already carries the
|
||||
// token, so the sentence loses its fallback clause.
|
||||
// fix). SKILL.md receives host substitution; references need the directory
|
||||
// resolved by the assistant, so spell out that boundary in the entrypoint.
|
||||
const SETUP_FALLBACK_TEXT =
|
||||
'That base directory resolves every `.claude/skills/impeccable/scripts/impeccable <verb>` command in this skill and its references, ' +
|
||||
'and `.claude/skills/impeccable/scripts` is the fallback only when the runtime reports no base directory.';
|
||||
const SETUP_PLUGIN_TEXT =
|
||||
'Every `"<skill-base-dir>/scripts/impeccable" <verb>` command in this skill and its references resolves against that base directory.';
|
||||
'Every `"${CLAUDE_SKILL_DIR}/scripts/impeccable" <verb>` command in this skill and its references resolves against that base directory. ' +
|
||||
'In reference files, replace the skill-base-dir placeholder with this directory before running commands; it is not a shell variable.';
|
||||
|
||||
// Agent files are subagent system prompts: a spawned agent never loads
|
||||
// SKILL.md, so Setup's <skill-base-dir> token is undefined in the one
|
||||
@@ -76,21 +79,26 @@ export const AGENT_EMBED_FALLBACK =
|
||||
* Rewrite one markdown file's content for the plugin subtree. Pure, so the
|
||||
* unit suite can pin every rewrite without a build.
|
||||
*/
|
||||
export function rewritePluginMarkdown(content) {
|
||||
export function rewritePluginMarkdown(content, { isSkillEntrypoint = true } = {}) {
|
||||
const baseDir = isSkillEntrypoint ? '${CLAUDE_SKILL_DIR}' : '<skill-base-dir>';
|
||||
return stripAllowedToolsFrontmatter(content)
|
||||
.replaceAll(PROJECT_ALLOWED_TOOLS_LINE, '')
|
||||
.replaceAll(SETUP_FALLBACK_TEXT, SETUP_PLUGIN_TEXT)
|
||||
.replaceAll(CLAUDE_PROJECT_SCRIPTS_PATH, PLUGIN_SCRIPTS_PATH)
|
||||
// <skill-base-dir> expands to a real path at run time, and an unquoted
|
||||
.replaceAll(CLAUDE_PROJECT_SCRIPTS_PATH, isSkillEntrypoint ? PLUGIN_SCRIPTS_PATH : `${baseDir}/scripts`)
|
||||
.replaceAll('<skill-base-dir>', baseDir)
|
||||
// ${CLAUDE_SKILL_DIR} expands to a real path at load time, and an unquoted
|
||||
// path with spaces splits before node sees it. Quote every command's
|
||||
// script argument, including the token-form commands SKILL.src.md
|
||||
// script argument, including the host-variable commands SKILL.src.md
|
||||
// carries natively (Setup step 1). Runs after the path replacement so
|
||||
// one pattern covers both origins; already-quoted forms don't match.
|
||||
.replace(/node <skill-base-dir>\/scripts\/([^\s`"]+)/g, 'node "<skill-base-dir>/scripts/$1"')
|
||||
// The engine launcher is the command itself now (`<skill-base-dir>/scripts/impeccable <verb>`,
|
||||
.replace(/node (\$\{CLAUDE_SKILL_DIR\}|<skill-base-dir>)\/scripts\/([^\s`"]+)/g, 'node "$1/scripts/$2"')
|
||||
// The engine launcher is the command itself (`${CLAUDE_SKILL_DIR}/scripts/impeccable <verb>`,
|
||||
// or `impeccable.cmd` on a Windows shell without sh), so the launcher path
|
||||
// is what gets quoted; the verb and its arguments follow unquoted.
|
||||
.replace(/(?<!["\w/])<skill-base-dir>\/scripts\/impeccable(\.cmd)?(?=[\s`])/g, '"<skill-base-dir>/scripts/impeccable$1"');
|
||||
.replace(
|
||||
/(?<!["\w/])(\$\{CLAUDE_SKILL_DIR\}|<skill-base-dir>)\/scripts\/impeccable(\.cmd)?(?=[\s`])/g,
|
||||
'"$1/scripts/impeccable$2"',
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -151,11 +159,18 @@ export function verifyPluginSkillRewrite(skillMdPath) {
|
||||
const content = fs.readFileSync(skillMdPath, 'utf-8');
|
||||
if (!content.includes(SETUP_PLUGIN_TEXT)) {
|
||||
throw new Error(
|
||||
`Plugin rewrite drift: ${skillMdPath} is missing the <skill-base-dir> resolution sentence. ` +
|
||||
`Plugin rewrite drift: ${skillMdPath} is missing the \${CLAUDE_SKILL_DIR} resolution sentence. ` +
|
||||
"SKILL.src.md's Setup step 1 fallback sentence no longer matches the replacement in " +
|
||||
'scripts/lib/plugin-paths.js (issue #523); update SETUP_FALLBACK_TEXT to the new wording.',
|
||||
);
|
||||
}
|
||||
if (content.includes('<skill-base-dir>')) {
|
||||
throw new Error(
|
||||
`Plugin rewrite drift: ${skillMdPath} still contains the <skill-base-dir> token. ` +
|
||||
'Plugin skill markdown must use ${CLAUDE_SKILL_DIR} (issue #744); check replaceAll in ' +
|
||||
'scripts/lib/plugin-paths.js.',
|
||||
);
|
||||
}
|
||||
if (parseFrontmatter(content).frontmatter['allowed-tools'] !== undefined) {
|
||||
throw new Error(
|
||||
`Plugin rewrite drift: ${skillMdPath} still declares allowed-tools in frontmatter. ` +
|
||||
@@ -195,7 +210,7 @@ export function rewritePluginMarkdownTree(dir, rewrite = rewritePluginMarkdown)
|
||||
rewritePluginMarkdownTree(entryPath, rewrite);
|
||||
} else if (entry.name.endsWith('.md')) {
|
||||
const original = fs.readFileSync(entryPath, 'utf-8');
|
||||
const rewritten = rewrite(original);
|
||||
const rewritten = rewrite(original, { isSkillEntrypoint: entry.name === 'SKILL.md' });
|
||||
if (rewritten !== original) fs.writeFileSync(entryPath, rewritten);
|
||||
}
|
||||
}
|
||||
|
||||
+1
-1
@@ -18,7 +18,7 @@ Core principles:
|
||||
|
||||
## Setup
|
||||
|
||||
1. Run `<skill-base-dir>/scripts/impeccable context` once per session, where `<skill-base-dir>` is the loaded base directory the runtime reports for this skill; keep cwd at the user's project. That base directory resolves every `{{scripts_path}}/impeccable <verb>` command in this skill and its references, and `{{scripts_path}}` is the fallback only when the runtime reports no base directory. On a Windows shell without `sh`, call `{{scripts_path}}/impeccable.cmd` instead. The launcher runs a self-contained binary that ships next to it or is downloaded once on first run; no Node or other runtime is required. Pass a named source file or route as `--target <path>`. It loads PRODUCT.md, DESIGN.md, the matching surface brief, and native-platform guidance when applicable; follow its directives and do not rerun it. <!-- rule:skill-setup-context -->
|
||||
1. Run `<skill-base-dir>/scripts/impeccable context` once per session, where `<skill-base-dir>` is the directory that contains this SKILL.md (the skill folder, not a plugin root two levels above it); keep cwd at the user's project. That base directory resolves every `{{scripts_path}}/impeccable <verb>` command in this skill and its references, and `{{scripts_path}}` is the fallback only when the runtime reports no base directory. On a Windows shell without `sh`, call `{{scripts_path}}/impeccable.cmd` instead. The launcher runs a self-contained binary that ships next to it or is downloaded once on first run; no Node or other runtime is required. Pass a named source file or route as `--target <path>`. It loads PRODUCT.md, DESIGN.md, the matching surface brief, and native-platform guidance when applicable; follow its directives and do not rerun it. If the launcher is refused, missing, or fails, tell the user before editing that context loading did not run. Read existing **PRODUCT.md** and **DESIGN.md** without inventing missing context, then continue with steps 2–3. <!-- rule:skill-setup-context -->
|
||||
2. Load the request's playbook: its Commands-table reference for an explicit/implied sub-command, or [reference/new-work.md](reference/new-work.md) for a new surface or replacement visual world. Inspect target and incumbent visual truth before editing. When the app cannot run, start with committed visual-regression goldens or screenshot fixtures; verify target and freshness against current tokens, CSS, components, or assets, resolve conflicts, and compare theme/variant captures. <!-- rule:skill-setup-command-ref --> <!-- rule:skill-setup-read-project -->
|
||||
3. After analysis and direction are resolved, load [reference/craft-floor.md](reference/craft-floor.md) immediately before editing UI. It carries the quality floor, the absolute bans, and the reflexes no detector catches. Do not load it for planning-only work. <!-- rule:skill-craft-floor-load -->
|
||||
|
||||
|
||||
+53
-14
@@ -6,7 +6,8 @@
|
||||
* `.claude/skills/impeccable/scripts`. Run from the plugin cache, that path
|
||||
* points into the user's project: a plugin-only user gets MODULE_NOT_FOUND,
|
||||
* and a dual-install user silently runs the project's older skill copy. The
|
||||
* rewrite swaps every markdown instruction to the `<skill-base-dir>` form
|
||||
* rewrite uses `${CLAUDE_SKILL_DIR}` only in SKILL.md; raw references keep
|
||||
* the explicit `<skill-base-dir>` placeholder. It removes allowed-tools
|
||||
* and drops the node pre-approval: no frontmatter rule can bind approval to
|
||||
* the loaded plugin root, and an unbound wildcard would auto-approve any
|
||||
* same-shaped path anywhere on disk.
|
||||
@@ -15,6 +16,7 @@ import { describe, test, expect, beforeEach, afterEach } from 'bun:test';
|
||||
import fs from 'fs';
|
||||
import os from 'os';
|
||||
import path from 'path';
|
||||
import { execFileSync } from 'node:child_process';
|
||||
import { parseFrontmatter } from '../scripts/lib/utils.js';
|
||||
import {
|
||||
rewritePluginMarkdown,
|
||||
@@ -27,10 +29,10 @@ import {
|
||||
} from '../scripts/lib/plugin-paths.js';
|
||||
|
||||
describe('rewritePluginMarkdown', () => {
|
||||
test('rewrites a script instruction to the quoted skill-base-dir form', () => {
|
||||
test('rewrites a script instruction to the quoted CLAUDE_SKILL_DIR form', () => {
|
||||
const input = 'Run `node .claude/skills/impeccable/scripts/context.mjs` once per session.';
|
||||
expect(rewritePluginMarkdown(input)).toBe(
|
||||
'Run `node "<skill-base-dir>/scripts/context.mjs"` once per session.',
|
||||
'Run `node "${CLAUDE_SKILL_DIR}/scripts/context.mjs"` once per session.',
|
||||
);
|
||||
});
|
||||
|
||||
@@ -41,8 +43,8 @@ describe('rewritePluginMarkdown', () => {
|
||||
].join('\n');
|
||||
const output = rewritePluginMarkdown(input);
|
||||
expect(output).not.toContain(CLAUDE_PROJECT_SCRIPTS_PATH);
|
||||
expect(output).toContain('node "<skill-base-dir>/scripts/live.mjs"');
|
||||
expect(output).toContain('node "<skill-base-dir>/scripts/live-poll.mjs" --reply EVENT_ID done');
|
||||
expect(output).toContain('node "${CLAUDE_SKILL_DIR}/scripts/live.mjs"');
|
||||
expect(output).toContain('node "${CLAUDE_SKILL_DIR}/scripts/live-poll.mjs" --reply EVENT_ID done');
|
||||
});
|
||||
|
||||
test('quotes the engine launcher path and leaves the verb outside the quotes', () => {
|
||||
@@ -50,9 +52,9 @@ describe('rewritePluginMarkdown', () => {
|
||||
'Run `<skill-base-dir>/scripts/impeccable context` once, then `<skill-base-dir>/scripts/impeccable.cmd doctor --json`; ' +
|
||||
'already quoted: `"<skill-base-dir>/scripts/impeccable" hooks on`.',
|
||||
);
|
||||
expect(output).toContain('`"<skill-base-dir>/scripts/impeccable" context`');
|
||||
expect(output).toContain('`"<skill-base-dir>/scripts/impeccable.cmd" doctor --json`');
|
||||
expect(output).not.toContain('""<skill-base-dir>');
|
||||
expect(output).toContain('`"${CLAUDE_SKILL_DIR}/scripts/impeccable" context`');
|
||||
expect(output).toContain('`"${CLAUDE_SKILL_DIR}/scripts/impeccable.cmd" doctor --json`');
|
||||
expect(output).not.toContain('""${CLAUDE_SKILL_DIR}');
|
||||
});
|
||||
|
||||
test('quotes commands already in the skill-base-dir form without double-quoting', () => {
|
||||
@@ -62,8 +64,8 @@ describe('rewritePluginMarkdown', () => {
|
||||
'Run `node <skill-base-dir>/scripts/context.mjs` once per session. ' +
|
||||
'Already quoted: `node "<skill-base-dir>/scripts/detect.mjs"`.';
|
||||
expect(rewritePluginMarkdown(input)).toBe(
|
||||
'Run `node "<skill-base-dir>/scripts/context.mjs"` once per session. ' +
|
||||
'Already quoted: `node "<skill-base-dir>/scripts/detect.mjs"`.',
|
||||
'Run `node "${CLAUDE_SKILL_DIR}/scripts/context.mjs"` once per session. ' +
|
||||
'Already quoted: `node "${CLAUDE_SKILL_DIR}/scripts/detect.mjs"`.',
|
||||
);
|
||||
});
|
||||
|
||||
@@ -96,7 +98,7 @@ describe('rewritePluginMarkdown', () => {
|
||||
'reports no base directory. Pass a named source file or route as `--target <path>`.';
|
||||
const output = rewritePluginMarkdown(input);
|
||||
expect(output).toContain(
|
||||
'Every `"<skill-base-dir>/scripts/impeccable" <verb>` command in this skill and its references resolves against that base directory.',
|
||||
'Every `"${CLAUDE_SKILL_DIR}/scripts/impeccable" <verb>` command in this skill and its references resolves against that base directory.',
|
||||
);
|
||||
// The naive rewrite would keep the fallback clause and name the token as
|
||||
// its own fallback for when there is no base directory to resolve it.
|
||||
@@ -213,7 +215,7 @@ describe('rewritePluginMarkdownTree', () => {
|
||||
rewritePluginMarkdownTree(root);
|
||||
|
||||
expect(fs.readFileSync(path.join(root, 'SKILL.md'), 'utf-8')).toBe(
|
||||
'Run `node "<skill-base-dir>/scripts/context.mjs"`.',
|
||||
'Run `node "${CLAUDE_SKILL_DIR}/scripts/context.mjs"`.',
|
||||
);
|
||||
expect(fs.readFileSync(path.join(root, 'reference/live.md'), 'utf-8')).toBe(
|
||||
'node "<skill-base-dir>/scripts/live.mjs"',
|
||||
@@ -223,6 +225,27 @@ describe('rewritePluginMarkdownTree', () => {
|
||||
);
|
||||
});
|
||||
|
||||
test('raw references retain an explicit base-directory placeholder, including Windows commands', () => {
|
||||
const input = 'Run `.claude/skills/impeccable/scripts/impeccable context` or `<skill-base-dir>/scripts/impeccable.cmd context`.';
|
||||
const output = rewritePluginMarkdown(input, { isSkillEntrypoint: false });
|
||||
expect(output).toBe('Run `"<skill-base-dir>/scripts/impeccable" context` or `"<skill-base-dir>/scripts/impeccable.cmd" context`.');
|
||||
expect(output).not.toContain('${CLAUDE_SKILL_DIR}');
|
||||
expect(rewritePluginMarkdown(output, { isSkillEntrypoint: false })).toBe(output);
|
||||
});
|
||||
|
||||
test('entrypoint and explicitly resolved reference commands work from a cache path with spaces', () => {
|
||||
const skillDir = path.join(root, 'plugin cache', 'skills', 'impeccable');
|
||||
fs.mkdirSync(path.join(skillDir, 'scripts'), { recursive: true });
|
||||
fs.writeFileSync(path.join(skillDir, 'scripts/probe.mjs'), 'console.log(process.argv[2]);');
|
||||
const env = { ...process.env };
|
||||
delete env.CLAUDE_SKILL_DIR;
|
||||
for (const isSkillEntrypoint of [true, false]) {
|
||||
const command = rewritePluginMarkdown('node .claude/skills/impeccable/scripts/probe.mjs resolved', { isSkillEntrypoint })
|
||||
.replaceAll(isSkillEntrypoint ? '${CLAUDE_SKILL_DIR}' : '<skill-base-dir>', skillDir);
|
||||
expect(execFileSync('sh', ['-c', command], { env, encoding: 'utf8' }).trim()).toBe('resolved');
|
||||
}
|
||||
});
|
||||
|
||||
test('applies the agent rewrite when passed for an agents tree', () => {
|
||||
const agentsDir = path.join(root, 'agents');
|
||||
fs.mkdirSync(agentsDir, { recursive: true });
|
||||
@@ -284,7 +307,7 @@ describe('verifyPluginSkillRewrite', () => {
|
||||
|
||||
test('fails the build when a launcher pre-approval survives the removal', () => {
|
||||
const p = writeSkill(
|
||||
rewritePluginMarkdown(goodSkill) + '\n - Bash(<skill-base-dir>/scripts/impeccable.cmd *)\n',
|
||||
rewritePluginMarkdown(goodSkill) + '\n - Bash(${CLAUDE_SKILL_DIR}/scripts/impeccable.cmd *)\n',
|
||||
);
|
||||
expect(() => verifyPluginSkillRewrite(p)).toThrow(/pre-approves an engine launcher/);
|
||||
});
|
||||
@@ -307,7 +330,7 @@ describe('verifyPluginSkillRewrite', () => {
|
||||
// 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) + '\n - Bash(node <skill-base-dir>/scripts/*)\n',
|
||||
rewritePluginMarkdown(goodSkill) + '\n - Bash(node ${CLAUDE_SKILL_DIR}/scripts/*)\n',
|
||||
);
|
||||
expect(() => verifyPluginSkillRewrite(p)).toThrow(/pre-approves an engine launcher or node script path/);
|
||||
});
|
||||
@@ -321,6 +344,11 @@ describe('verifyPluginSkillRewrite', () => {
|
||||
);
|
||||
expect(() => verifyPluginSkillRewrite(p)).toThrow(/still contains the project-relative scripts path/);
|
||||
});
|
||||
|
||||
test('fails the build when the skill-base-dir token survives the rewrite', () => {
|
||||
const p = writeSkill(rewritePluginMarkdown(goodSkill).replace('${CLAUDE_SKILL_DIR}', '<skill-base-dir>'));
|
||||
expect(() => verifyPluginSkillRewrite(p)).toThrow(/still contains the <skill-base-dir> token/);
|
||||
});
|
||||
});
|
||||
|
||||
describe('SKILL.src.md frontmatter', () => {
|
||||
@@ -333,3 +361,14 @@ describe('SKILL.src.md frontmatter', () => {
|
||||
expect(frontmatter['allowed-tools']).toBeDefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe('SKILL.src.md Setup step 1 authoring contract (issue #744)', () => {
|
||||
test('disambiguates the skill-base-dir token', () => {
|
||||
const setup = fs.readFileSync(
|
||||
path.join(import.meta.dirname, '../skill/SKILL.src.md'),
|
||||
'utf-8',
|
||||
).replace(/\r\n?/g, '\n');
|
||||
const step1 = setup.match(/^1\. .+$/m)?.[0] ?? '';
|
||||
expect(step1).toMatch(/skill folder, not a plugin root/);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -4,6 +4,32 @@ import fs from 'node:fs';
|
||||
import path from 'node:path';
|
||||
import { prepareWorkspace, cleanupWorkspace, makeTools } from './skill-behavior/harness.mjs';
|
||||
|
||||
it('denied-launcher tools reject every shell attempt without executing or modifying the skill', async () => {
|
||||
const workspace = prepareWorkspace({ files: { 'index.html': 'before' } });
|
||||
try {
|
||||
const { tools, trace } = makeTools(workspace, {}, {}, { denyBash: true });
|
||||
for (const command of [
|
||||
'.claude/skills/impeccable/scripts/impeccable context',
|
||||
'.claude/skills/impeccable/scripts/impeccable context; echo bad > index.html',
|
||||
'echo bad > index.html',
|
||||
]) {
|
||||
assert.match(await tools.bash.execute({ command }), /permission denied/i);
|
||||
}
|
||||
assert.equal(fs.readFileSync(path.join(workspace, 'index.html'), 'utf8'), 'before');
|
||||
assert.ok(trace.toolCalls.every((call) => call.denied && call.mutatedPaths.length === 0));
|
||||
const skillPath = '.claude/skills/impeccable/reference/polish.md';
|
||||
const before = await tools.read.execute({ path: skillPath });
|
||||
assert.match(await tools.write.execute({ path: skillPath, contents: 'bad' }), /^Error:/);
|
||||
assert.equal(await tools.read.execute({ path: skillPath }), before);
|
||||
await tools.read.execute({ path: 'missing.md' });
|
||||
assert.deepEqual(trace.toolCalls.filter((call) => call.name === 'read').map((call) => call.succeeded), [true, true, false]);
|
||||
await tools.write.execute({ path: 'index.html', contents: 'after' });
|
||||
assert.deepEqual(trace.toolCalls.flatMap((call) => call.mutatedPaths), ['index.html']);
|
||||
} finally {
|
||||
cleanupWorkspace(workspace);
|
||||
}
|
||||
});
|
||||
|
||||
it('context-only routing tools reject shell searches and compound commands before execution', async () => {
|
||||
const workspace = prepareWorkspace({ files: { 'index.html': 'before' } });
|
||||
try {
|
||||
|
||||
@@ -75,6 +75,28 @@ The trace is the source of truth, not the model's free-form reply.
|
||||
| 16 | existing surface, with and without PRODUCT.md; asks where to start | loads `routing.md`, delivers advice, and does not edit project files, start an interview, archive a critique, or run menu scans |
|
||||
| 17 | existing surface; asks whether critique is required before polish | loads `routing.md` and both command references, then delivers advice without executing the playbooks |
|
||||
| 18 | existing surface; explicitly requests polish followed by a next-command recommendation | loads `polish.md` rather than substituting workflow advice for the requested work |
|
||||
| 19 | tiny spacing edit with PRODUCT.md + DESIGN.md; Bash denied, plus a real-loader success control | actually reads playbook and craft floor before editing; denial also requires direct context-file reads and a user-visible warning before the edit |
|
||||
|
||||
## Setup launcher-failure branch (2026-09-06, PR #750)
|
||||
|
||||
Scenario 19 injects a host permission error before any shell command executes,
|
||||
including retries and compound commands. File tools remain available; the
|
||||
staged skill is read-only. Assertions require successful reads, an actual UI
|
||||
edit, unchanged context files, and disclosure before the first write in the
|
||||
assistant message sequence. Failed read attempts and shell commands merely
|
||||
mentioning a reference do not count as loading it. The success control allows
|
||||
only the real context-loader command through Bash and requires exit 0.
|
||||
|
||||
The suite measures continuation after a tool refusal, not Claude's skill
|
||||
activation or plugin substitution. Those are separate loader/path checks.
|
||||
It also does not establish behavior for every missing-binary or runtime error.
|
||||
|
||||
Focused baseline (2026-09-06): both scenario 19 cases passed on
|
||||
`claude-sonnet-5`, one run each. The original wording also continued after
|
||||
denial in the comparison run, but disclosed the failure only in its final
|
||||
summary. This does not reproduce the reporter's complete reference-loading
|
||||
failure or establish a multi-provider pass. An earlier scenario 6 result used
|
||||
attempt-based reference assertions and is not counted as a success control.
|
||||
|
||||
## Workflow-advice baseline (2026-09-05, PR #737)
|
||||
|
||||
|
||||
@@ -219,7 +219,7 @@ function defaultSimulatedAnswer(question) {
|
||||
return 'Use the brief, preserve real operational content, and make the primary decision obvious.';
|
||||
}
|
||||
|
||||
export function makeTools(workspace, extraEnv = {}, simulatedUser = {}, { contextOnlyBash = false } = {}) {
|
||||
export function makeTools(workspace, extraEnv = {}, simulatedUser = {}, { contextOnlyBash = false, denyBash = false } = {}) {
|
||||
const trace = {
|
||||
toolCalls: [],
|
||||
bashCommands: [],
|
||||
@@ -250,6 +250,14 @@ export function makeTools(workspace, extraEnv = {}, simulatedUser = {}, { contex
|
||||
}),
|
||||
execute: async ({ command }) => {
|
||||
const call = record('bash', { command });
|
||||
// Simulate a host refusal, not a process failure. Nothing reaches a
|
||||
// shell, including retries, alternate launchers, and compound commands.
|
||||
if (denyBash) {
|
||||
call.denied = true;
|
||||
const out = 'Error: Bash permission denied by the host. This command was not executed.';
|
||||
trace.bashOutputs.push(out);
|
||||
return out;
|
||||
}
|
||||
// Routing tests need the real context loader, not a general-purpose
|
||||
// shell on the host. Reject before execution (still record attempts).
|
||||
if (contextOnlyBash && command.trim() !== '.claude/skills/impeccable/scripts/impeccable context') {
|
||||
@@ -273,13 +281,16 @@ export function makeTools(workspace, extraEnv = {}, simulatedUser = {}, { contex
|
||||
path: z.string().describe('Workspace-relative file path.'),
|
||||
}),
|
||||
execute: async ({ path: p }) => {
|
||||
record('read', { path: p });
|
||||
const call = record('read', { path: p });
|
||||
call.succeeded = false;
|
||||
const resolved = safeResolve(workspace, p);
|
||||
if (typeof resolved !== 'string') return `Error: ${resolved.error}`;
|
||||
if (!fs.existsSync(resolved)) return `File not found: ${p}`;
|
||||
const stat = fs.statSync(resolved);
|
||||
if (stat.isDirectory()) return `Path is a directory: ${p}. Use list instead.`;
|
||||
return fs.readFileSync(resolved, 'utf8');
|
||||
const contents = fs.readFileSync(resolved, 'utf8');
|
||||
call.succeeded = true;
|
||||
return contents;
|
||||
},
|
||||
}),
|
||||
write: tool({
|
||||
@@ -292,7 +303,7 @@ export function makeTools(workspace, extraEnv = {}, simulatedUser = {}, { contex
|
||||
const call = record('write', { path: p, contents });
|
||||
const resolved = safeResolve(workspace, p);
|
||||
if (typeof resolved !== 'string') return `Error: ${resolved.error}`;
|
||||
if (contextOnlyBash && path.relative(workspace, resolved).split(path.sep)[0] === '.claude') {
|
||||
if ((contextOnlyBash || denyBash) && path.relative(workspace, resolved).split(path.sep)[0] === '.claude') {
|
||||
return 'Error: the staged skill is read-only; edits must target project files.';
|
||||
}
|
||||
fs.mkdirSync(path.dirname(resolved), { recursive: true });
|
||||
@@ -370,8 +381,8 @@ export function makeTools(workspace, extraEnv = {}, simulatedUser = {}, { contex
|
||||
// run is never killed. The timer is unref'd (it must not keep the loop alive
|
||||
// after a healthy turn) and cleared on completion.
|
||||
const TURN_TIMEOUT_MS = Number(process.env.IMPECCABLE_SKILL_BEHAVIOR_TURN_TIMEOUT_MS) || 840_000;
|
||||
export async function runTurn({ workspace, model, userPrompt, priorMessages = [], maxSteps = 8, env = {}, simulatedUser = {}, timeoutMs = TURN_TIMEOUT_MS, contextOnlyBash = false }) {
|
||||
const { tools, trace } = makeTools(workspace, env, simulatedUser, { contextOnlyBash });
|
||||
export async function runTurn({ workspace, model, userPrompt, priorMessages = [], maxSteps = 8, env = {}, simulatedUser = {}, timeoutMs = TURN_TIMEOUT_MS, contextOnlyBash = false, denyBash = false }) {
|
||||
const { tools, trace } = makeTools(workspace, env, simulatedUser, { contextOnlyBash, denyBash });
|
||||
const messages = [
|
||||
...priorMessages,
|
||||
{ role: 'user', content: userPrompt },
|
||||
@@ -409,6 +420,7 @@ export async function runTurn({ workspace, model, userPrompt, priorMessages = []
|
||||
return {
|
||||
trace,
|
||||
text: result.text ?? '',
|
||||
stepTexts: result.steps.map((step) => step.text ?? ''),
|
||||
finishReason: result.finishReason,
|
||||
usage: result.usage,
|
||||
responseMessages,
|
||||
|
||||
@@ -652,6 +652,49 @@ for (const modelId of resolveModelList()) {
|
||||
}
|
||||
});
|
||||
|
||||
for (const denyBash of [true, false]) {
|
||||
it(`scenario 19: ${denyBash ? 'denied launcher' : 'successful launcher control'} loads context and references before editing`, async () => {
|
||||
const workspace = prepareWorkspace({ files: {
|
||||
'PRODUCT.md': PRODUCT_MD_SAMPLE,
|
||||
'DESIGN.md': DESIGN_MD_SAMPLE,
|
||||
'index.html': '<!doctype html><html lang="en"><head><title>Fieldnotes</title><style>body{font:16px system-ui;margin:32px}button{padding:2px 4px}</style></head><body><main><h1>Fieldnotes</h1><p>A calmer place for your notes.</p><button>New note</button></main></body></html>',
|
||||
} });
|
||||
try {
|
||||
const { trace, stepTexts, finishReason, responseMessages } = await runTurn({
|
||||
workspace,
|
||||
model,
|
||||
userPrompt: '/impeccable polish index.html. Keep this pass small: improve the button spacing only, preserving the page content and structure.',
|
||||
maxSteps: 12,
|
||||
denyBash,
|
||||
contextOnlyBash: !denyBash,
|
||||
});
|
||||
const allText = stepTexts.join('\n');
|
||||
logTrace('S19', denyBash ? 'denied-launcher' : 'successful-launcher', modelId, trace, { finishReason, text: allText });
|
||||
assert.notEqual(finishReason, 'length', 'a truncated response is not a completed fallback');
|
||||
if (denyBash) {
|
||||
assert.ok(trace.toolCalls.some((call) => call.name === 'bash' && call.denied && /impeccable\s+context\b/.test(call.input.command)), 'must encounter an actual denied context attempt');
|
||||
} else {
|
||||
assert.ok(trace.bashOutputs.some((out) => out.startsWith('exit=0\n')), 'the control must execute the real context loader successfully');
|
||||
}
|
||||
const writeIndex = trace.toolCalls.findIndex((call) => call.mutatedPaths.includes('index.html'));
|
||||
assert.ok(writeIndex >= 0, 'must continue to the requested edit, not just load references');
|
||||
for (const filename of [...(denyBash ? ['PRODUCT.md', 'DESIGN.md'] : []), 'reference/polish.md', 'reference/craft-floor.md']) {
|
||||
const readIndex = trace.toolCalls.findIndex((call) => call.name === 'read' && call.succeeded && (call.input.path === filename || call.input.path.endsWith(`/${filename}`)));
|
||||
assert.ok(readIndex >= 0 && readIndex < writeIndex, `${filename} must actually be read before editing`);
|
||||
}
|
||||
const warning = /(?:context|launcher|bash)[^.!?\n]{0,160}(?:denied|refused|unavailable|blocked|could(?:n't| not)|cannot|can't|did(?:n't| not)|fail|unable)|(?:denied|refused|unavailable|blocked|could(?:n't| not)|cannot|can't|unable)[^.!?\n]{0,160}(?:context|launcher|bash)/i;
|
||||
const assistantBlocks = responseMessages.filter((message) => message.role === 'assistant')
|
||||
.flatMap((message) => typeof message.content === 'string' ? [{ type: 'text', text: message.content }] : message.content);
|
||||
const warningIndex = assistantBlocks.findIndex((block) => block.type === 'text' && warning.test(block.text));
|
||||
const writeBlockIndex = assistantBlocks.findIndex((block) => block.type === 'tool-call' && block.toolName === 'write');
|
||||
if (denyBash) assert.ok(warningIndex >= 0 && writeBlockIndex > warningIndex, 'must disclose the failed context launcher before editing, not only in the final summary');
|
||||
assert.ok(!trace.toolCalls.some((call) => call.mutatedPaths.some((p) => /(?:^|\/)(?:PRODUCT|DESIGN)\.md$/.test(p))), 'must not fabricate or replace project context');
|
||||
} finally {
|
||||
cleanupWorkspace(workspace);
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
it('scenario 18: explicit command request takes precedence over workflow advice', async () => {
|
||||
const workspace = prepareWorkspace({ files: WORKFLOW_ADVICE_FILES });
|
||||
try {
|
||||
|
||||
Reference in New Issue
Block a user