diff --git a/scripts/lib/plugin-paths.js b/scripts/lib/plugin-paths.js index 8ad448c42..17f8e78d3 100644 --- a/scripts/lib/plugin-paths.js +++ b/scripts/lib/plugin-paths.js @@ -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 `` 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 ``; + * 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 = '/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 ` 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 `"/scripts/impeccable" ` command in this skill and its references resolves against that base directory.'; + 'Every `"${CLAUDE_SKILL_DIR}/scripts/impeccable" ` 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 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}' : ''; return stripAllowedToolsFrontmatter(content) .replaceAll(PROJECT_ALLOWED_TOOLS_LINE, '') .replaceAll(SETUP_FALLBACK_TEXT, SETUP_PLUGIN_TEXT) - .replaceAll(CLAUDE_PROJECT_SCRIPTS_PATH, PLUGIN_SCRIPTS_PATH) - // expands to a real path at run time, and an unquoted + .replaceAll(CLAUDE_PROJECT_SCRIPTS_PATH, isSkillEntrypoint ? PLUGIN_SCRIPTS_PATH : `${baseDir}/scripts`) + .replaceAll('', 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 \/scripts\/([^\s`"]+)/g, 'node "/scripts/$1"') - // The engine launcher is the command itself now (`/scripts/impeccable `, + .replace(/node (\$\{CLAUDE_SKILL_DIR\}|)\/scripts\/([^\s`"]+)/g, 'node "$1/scripts/$2"') + // The engine launcher is the command itself (`${CLAUDE_SKILL_DIR}/scripts/impeccable `, // 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(/(?\/scripts\/impeccable(\.cmd)?(?=[\s`])/g, '"/scripts/impeccable$1"'); + .replace( + /(?)\/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 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('')) { + throw new Error( + `Plugin rewrite drift: ${skillMdPath} still contains the 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); } } diff --git a/skill/SKILL.src.md b/skill/SKILL.src.md index 72d35f7c9..c87bbf4bc 100644 --- a/skill/SKILL.src.md +++ b/skill/SKILL.src.md @@ -18,7 +18,7 @@ Core principles: ## Setup -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 `{{scripts_path}}/impeccable ` 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 `. It loads PRODUCT.md, DESIGN.md, the matching surface brief, and native-platform guidance when applicable; follow its directives and do not rerun it. +1. Run `/scripts/impeccable context` once per session, where `` 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 ` 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 `. 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. 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. 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. diff --git a/tests/plugin-paths.test.js b/tests/plugin-paths.test.js index e4aa2deef..df94c59f6 100644 --- a/tests/plugin-paths.test.js +++ b/tests/plugin-paths.test.js @@ -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 `` form + * rewrite uses `${CLAUDE_SKILL_DIR}` only in SKILL.md; raw references keep + * the explicit `` 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 "/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 "/scripts/live.mjs"'); - expect(output).toContain('node "/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 `/scripts/impeccable context` once, then `/scripts/impeccable.cmd doctor --json`; ' + 'already quoted: `"/scripts/impeccable" hooks on`.', ); - expect(output).toContain('`"/scripts/impeccable" context`'); - expect(output).toContain('`"/scripts/impeccable.cmd" doctor --json`'); - expect(output).not.toContain('""'); + 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 /scripts/context.mjs` once per session. ' + 'Already quoted: `node "/scripts/detect.mjs"`.'; expect(rewritePluginMarkdown(input)).toBe( - 'Run `node "/scripts/context.mjs"` once per session. ' + - 'Already quoted: `node "/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 `.'; const output = rewritePluginMarkdown(input); expect(output).toContain( - 'Every `"/scripts/impeccable" ` command in this skill and its references resolves against that base directory.', + 'Every `"${CLAUDE_SKILL_DIR}/scripts/impeccable" ` 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 "/scripts/context.mjs"`.', + 'Run `node "${CLAUDE_SKILL_DIR}/scripts/context.mjs"`.', ); expect(fs.readFileSync(path.join(root, 'reference/live.md'), 'utf-8')).toBe( 'node "/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 `/scripts/impeccable.cmd context`.'; + const output = rewritePluginMarkdown(input, { isSkillEntrypoint: false }); + expect(output).toBe('Run `"/scripts/impeccable" context` or `"/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}' : '', 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(/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 /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}', '')); + expect(() => verifyPluginSkillRewrite(p)).toThrow(/still contains the 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/); + }); +}); diff --git a/tests/skill-behavior-harness.test.mjs b/tests/skill-behavior-harness.test.mjs index 90b0235ac..f73884a24 100644 --- a/tests/skill-behavior-harness.test.mjs +++ b/tests/skill-behavior-harness.test.mjs @@ -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 { diff --git a/tests/skill-behavior/README.md b/tests/skill-behavior/README.md index 9db7678cd..db6c8150d 100644 --- a/tests/skill-behavior/README.md +++ b/tests/skill-behavior/README.md @@ -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) diff --git a/tests/skill-behavior/harness.mjs b/tests/skill-behavior/harness.mjs index 224226abe..c954059ad 100644 --- a/tests/skill-behavior/harness.mjs +++ b/tests/skill-behavior/harness.mjs @@ -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, diff --git a/tests/skill-behavior/scenarios.test.mjs b/tests/skill-behavior/scenarios.test.mjs index 2607ed849..97a513791 100644 --- a/tests/skill-behavior/scenarios.test.mjs +++ b/tests/skill-behavior/scenarios.test.mjs @@ -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': 'Fieldnotes

Fieldnotes

A calmer place for your notes.

', + } }); + 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 {