From cf9b35d544f7e88590f39d76d4c63cf5a51fd3dc Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Mon, 7 Sep 2026 16:02:01 -0700 Subject: [PATCH] Fix skill workflow confirmations and behavior fixtures AI assistance: Codex, under maintainer direction. --- skill/SKILL.src.md | 2 +- skill/reference/new-work.md | 2 +- tests/skill-behavior-harness.test.mjs | 130 +++++++++++++++++- tests/skill-behavior/README.md | 69 ++++++++-- tests/skill-behavior/assertions.mjs | 16 +++ tests/skill-behavior/harness.mjs | 121 +++++++++------- tests/skill-behavior/scenarios.test.mjs | 10 +- .../skill-behavior/workflow-contract.test.mjs | 11 +- 8 files changed, 284 insertions(+), 77 deletions(-) diff --git a/skill/SKILL.src.md b/skill/SKILL.src.md index 0637f4789..f989e758c 100644 --- a/skill/SKILL.src.md +++ b/skill/SKILL.src.md @@ -22,7 +22,7 @@ Core principles: 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 resolving analysis and direction, read [reference/craft-floor.md](reference/craft-floor.md) immediately before any UI edit, including small refinements. It carries the quality floor, the absolute bans, and the reflexes no detector catches. Do not load it for planning-only work. -**Launcher unavailable:** If refused, missing, or failed, **first send the user a message** that context loading did not run. Then read existing PRODUCT.md and DESIGN.md without inventing missing context, follow the applicable steps 2–3, and perform the requested work through permitted tools. Launcher failure alone does not block otherwise-permitted edits. +**Launcher unavailable:** On refusal or failure, send a separate message **before the next tool call**: “Context loading did not run; I’ll read the existing project context directly.” Then read existing PRODUCT.md and DESIGN.md without inventing missing context, follow applicable steps 2–3, and continue through permitted tools. This applies to planning and editing; launcher failure alone does not block either. ## How to design diff --git a/skill/reference/new-work.md b/skill/reference/new-work.md index 675aa02ec..50049b0b1 100644 --- a/skill/reference/new-work.md +++ b/skill/reference/new-work.md @@ -15,7 +15,7 @@ A section, component, feature, or state inside an established surface inherits t ## 2. Ask what will change the work -Ask one round of two or three related questions through the structured question tool when available. Skip settled facts; a precise request may need only a compact confirmation. +Before implementation, get the user's answer through the structured question tool when available. Ask two or three related questions; a precise request needs only a compact confirmation. Skip settled facts, not the confirmation: DESIGN.md settles the visual world, not this surface's purpose or concept. - **Persuade:** who must act, what they should believe, which real proof, content, or assets earn that belief. - **Operate:** the task, information, important states, frequency, constraints. diff --git a/tests/skill-behavior-harness.test.mjs b/tests/skill-behavior-harness.test.mjs index 6ffe5c0ee..5a60a9119 100644 --- a/tests/skill-behavior-harness.test.mjs +++ b/tests/skill-behavior-harness.test.mjs @@ -3,8 +3,85 @@ import assert from 'node:assert/strict'; import fs from 'node:fs'; import path from 'node:path'; import { MockLanguageModelV3 } from 'ai/test'; -import { prepareWorkspace, cleanupWorkspace, makeTools, runTurn, SKILL_BODY } from './skill-behavior/harness.mjs'; -import { assertPlanningFallbackWarning } from './skill-behavior/assertions.mjs'; +import { prepareWorkspace, cleanupWorkspace, makeTools, runTurn, fileLoaded, SKILL_BODY } from './skill-behavior/harness.mjs'; +import { assertPlanningFallbackWarning, assertNewWorkLifecycle } from './skill-behavior/assertions.mjs'; + +it('new-work requires approval and a brief before code, then documents the finished redesign', () => { + const ask = { name: 'ask_user_question' }; + const brief = { name: 'bash', mutatedPaths: ['.impeccable/surfaces/current-html.md'] }; + const page = { name: 'write', mutatedPaths: ['current.html'] }; + const design = { name: 'write', mutatedPaths: ['DESIGN.md'] }; + const check = (toolCalls) => assertNewWorkLifecycle({ toolCalls }, { target: 'current.html', redesign: true }); + assert.doesNotThrow(() => check([ask, brief, page, design])); + assert.doesNotThrow(() => check([ask, brief, page, design, page, design])); + assert.throws(() => check([ask, brief]), /did not produce/); + assert.throws(() => check([brief, page, ask, design]), /user answer/); + assert.throws(() => check([ask, page, brief, design]), /surface brief before/); + assert.throws(() => check([ask, brief, design, page]), /finished build/); + assert.throws(() => check([ask, brief, page, design, page]), /finished build/); +}); + +it('stages resolved references independently of the source skill', async () => { + const workspace = prepareWorkspace(); + try { + const base = path.join(workspace, '.claude/skills/impeccable'); + assert.equal(fs.lstatSync(base).isSymbolicLink(), false); + const { tools } = makeTools(workspace); + const critique = await tools.read.execute({ path: '.claude/skills/impeccable/reference/critique.md' }); + assert.match(critique, /Use the ask_user_question tool\./); + assert.doesNotMatch(critique, /\{\{ask_instruction\}\}|\{\{scripts_path\}\}|/); + for (const role of ['finish-reviewer', 'documenter']) { + const reference = await tools.read.execute({ path: `.claude/skills/impeccable/reference/degraded/${role}.md` }); + assert.match(reference, /This harness has no subagent capability/); + assert.doesNotMatch(reference, /\{\{scripts_path\}\}|/); + } + const shellRead = await tools.bash.execute({ command: 'cat .claude/skills/impeccable/reference/critique.md' }); + assert.ok(shellRead.includes(critique), 'shell and read tools must see the same resolved reference'); + assert.match(await tools.write.execute({ path: '.claude/skills/impeccable/reference/critique.md', contents: 'bad' }), /^Error:/); + } finally { + cleanupWorkspace(workspace); + } +}); + +it('reference-loading evidence requires content, not a failed read or a filename mention', async () => { + const workspace = prepareWorkspace(); + try { + const ref = '.claude/skills/impeccable/reference/polish.md'; + const denied = makeTools(workspace, {}, {}, { denyBash: true }); + await denied.tools.bash.execute({ command: `cat ${ref}` }); + assert.equal(fileLoaded(denied.trace, 'polish.md'), false); + await denied.tools.read.execute({ path: 'missing/polish.md' }); + assert.equal(fileLoaded(denied.trace, 'polish.md'), false); + const allowed = makeTools(workspace); + await allowed.tools.bash.execute({ command: `printf '%s' '${ref}'` }); + assert.equal(fileLoaded(allowed.trace, 'polish.md'), false); + await allowed.tools.bash.execute({ command: `cat ${ref}` }); + assert.equal(fileLoaded(allowed.trace, 'polish.md'), true); + await denied.tools.read.execute({ path: ref }); + assert.equal(fileLoaded(denied.trace, 'polish.md'), true); + } finally { + cleanupWorkspace(workspace); + } +}); + +it('headless behavior shells disable unattended decision pages and omit provider credentials', async () => { + const workspace = prepareWorkspace(); + try { + const { tools } = makeTools(workspace, { OPENAI_API_KEY: 'synthetic-secret', IMPECCABLE_QUESTION_DISABLED: '0' }); + const result = await tools.bash.execute({ command: 'node -e \'console.log(JSON.stringify({disabled:process.env.IMPECCABLE_QUESTION_DISABLED,hasKey:!!process.env.OPENAI_API_KEY}))\'' }); + assert.match(result, /"disabled":"1"/); + assert.match(result, /"hasKey":false/); + assert.doesNotMatch(result, /synthetic-secret/); + if (process.env.IMPECCABLE_BIN) { + const question = await tools.bash.execute({ command: '.claude/skills/impeccable/scripts/impeccable serve-question --start --payload nonexistent.json' }); + assert.match(question, /^exit=2\n/); + assert.match(question, /use the structured question tool instead/); + assert.equal(fs.existsSync(path.join(workspace, '.impeccable/questions')), false); + } + } finally { + cleanupWorkspace(workspace); + } +}); it('planning fallback requires an assistant warning between the denial and context reads', () => { const call = { role: 'assistant', content: [{ type: 'tool-call', toolCallId: 'context', toolName: 'bash', input: { command: '.claude/skills/impeccable/scripts/impeccable context' } }] }; @@ -47,6 +124,40 @@ it('DeepSeek gets an explicit output ceiling instead of the compatibility SDK de } }); +it('optional diagnostics retain tool evidence when a provider turn fails', async () => { + const workspace = prepareWorkspace({ files: { 'PRODUCT.md': 'Synthetic product context.' } }); + const previous = process.env.IMPECCABLE_SKILL_BEHAVIOR_TRACE_DIR; + const traceDir = path.join(workspace, 'diagnostics'); + process.env.IMPECCABLE_SKILL_BEHAVIOR_TRACE_DIR = traceDir; + try { + let calls = 0; + const model = new MockLanguageModelV3({ + modelId: 'claude-sonnet-5', + doGenerate: async () => { + if (calls++ === 0) return { + content: [{ type: 'tool-call', toolCallId: 'read-product', toolName: 'read', input: JSON.stringify({ path: 'PRODUCT.md' }) }], + finishReason: { unified: 'tool-calls', raw: 'tool-calls' }, + usage: { inputTokens: { total: 1 }, outputTokens: { total: 1 } }, + warnings: [], + }; + throw new Error('synthetic provider failure'); + }, + }); + await assert.rejects(runTurn({ workspace, model, userPrompt: 'Synthetic diagnostic test.' }), /synthetic provider failure/); + const files = fs.readdirSync(traceDir); + assert.equal(files.length, 1); + const diagnostic = JSON.parse(fs.readFileSync(path.join(traceDir, files[0]), 'utf8')); + assert.equal(diagnostic.status, 'failed'); + assert.match(diagnostic.error, /synthetic provider failure/); + assert.equal(diagnostic.trace.toolCalls.length, 1); + assert.equal(fileLoaded(diagnostic.trace, 'PRODUCT.md'), true); + } finally { + if (previous === undefined) delete process.env.IMPECCABLE_SKILL_BEHAVIOR_TRACE_DIR; + else process.env.IMPECCABLE_SKILL_BEHAVIOR_TRACE_DIR = previous; + cleanupWorkspace(workspace); + } +}); + it('loaded-skill metadata resolves to the staged launcher and readable references', async () => { const workspace = prepareWorkspace(); try { @@ -96,18 +207,31 @@ it('context-only routing tools reject shell searches and compound commands befor for (const command of [ 'find / -name routing.md', '.claude/skills/impeccable/scripts/impeccable context; echo bad > index.html', + '.claude/skills/impeccable/scripts/impeccable context --target index.html; echo bad > index.html', + '.claude/skills/impeccable/scripts/impeccable context --target "$(echo bad > index.html)"', 'echo bad > index.html', ]) { assert.match(await tools.bash.execute({ command }), /^Error:/); } assert.equal(fs.readFileSync(path.join(workspace, 'index.html'), 'utf8'), 'before'); - assert.equal(trace.bashCommands.length, 3, 'rejected attempts remain observable'); + assert.equal(trace.bashCommands.length, 5, 'rejected attempts remain observable'); assert.ok(trace.toolCalls.every((call) => call.mutatedPaths.length === 0)); } finally { cleanupWorkspace(workspace); } }); +it('successful-loader controls accept a workspace-relative target', { skip: !process.env.IMPECCABLE_BIN }, async () => { + const workspace = prepareWorkspace({ files: { 'index.html': '' } }); + try { + const { tools } = makeTools(workspace, {}, {}, { contextOnlyBash: true }); + assert.match(await tools.bash.execute({ command: '.claude/skills/impeccable/scripts/impeccable context --target index.html' }), /^exit=0\n/); + assert.match(await tools.bash.execute({ command: '.claude/skills/impeccable/scripts/impeccable context --target ../outside.html' }), /^Error:/); + } finally { + cleanupWorkspace(workspace); + } +}); + it('context-only routing tools keep project writes observable but protect the staged skill', async () => { const workspace = prepareWorkspace({ files: { 'index.html': 'before' } }); try { diff --git a/tests/skill-behavior/README.md b/tests/skill-behavior/README.md index fbbc70e0b..2860e1c71 100644 --- a/tests/skill-behavior/README.md +++ b/tests/skill-behavior/README.md @@ -2,8 +2,8 @@ LLM-backed scenarios that verify how the impeccable skill drives context, command-reference, new-work, and native-platform loading. Each scenario runs -against one current model from each supported provider (Anthropic, OpenAI, -Google, DeepSeek). +against the default Anthropic, OpenAI, and Google models. DeepSeek remains +available through `IMPECCABLE_SKILL_BEHAVIOR_MODELS`. These are the tests you re-run when you refactor anything in SKILL.md's `## Setup` section. They fail when the agent stops following the loading @@ -25,7 +25,7 @@ skipped, not failed. Also requires the engine binary (`bun run fetch:engine`, or `IMPECCABLE_BIN`). The staged skill dir ships the launcher (`scripts/impeccable`); the harness exports `IMPECCABLE_BIN` into every bash call the agent makes, so the launcher -resolves the binary in both symlink and copy mode without a download. Without a +resolves the binary in the generated fixture without a download. Without a binary the suites skip. To run a single scenario against one model: @@ -39,13 +39,14 @@ IMPECCABLE_SKILL_BEHAVIOR_MODELS=claude-sonnet-5 IMPECCABLE_SKILL_BEHAVIOR_VERBO Each scenario: -1. `prepareWorkspace()` mints a temp dir, symlinks the canonical skill - into `/.claude/skills/impeccable` (so its launcher is at - `.claude/skills/impeccable/scripts/impeccable`), and optionally writes - `PRODUCT.md` / `DESIGN.md` fixtures. +1. `prepareWorkspace()` uses the production transformer to build current source + into an independent `/.claude/skills/impeccable`. References have + resolved placeholders and generated degraded reviewer/documenter files. + Host-specific blocks are omitted: this is a neutral API harness, not an exact + Claude/Codex/Gemini host simulation. It optionally seeds project fixtures. 2. `runTurn()` inlines `SKILL.md` (placeholders neutralized) as the - system prompt and runs Vercel AI SDK `generateText` with four - workspace-scoped tools: `bash`, `read`, `write`, `list`, and a fake + system prompt and runs Vercel AI SDK `generateText` with five + tools: `bash`, `read`, `write`, `list`, and a fake provider-neutral `ask_user_question` backed by a deterministic simulated user. 3. The tools record every call into a `trace` that the test asserts on. 4. For scenario 4, a second `runTurn` reuses turn 1's `responseMessages` @@ -53,6 +54,50 @@ Each scenario: The trace is the source of truth, not the model's free-form reply. +File tools are workspace-scoped; bash is a real host shell, **not a security +sandbox**. Use disposable synthetic fixtures. Shell helpers do not inherit +provider API keys/auth tokens; model calls still use the parent's keys. The +harness always sets `IMPECCABLE_QUESTION_DISABLED=1` for shell calls so real +decision pages cannot wait for a nonexistent browser user. The engine returns +its genuine structured-question fallback; browser decisions have separate E2E. + +Set `IMPECCABLE_SKILL_BEHAVIOR_TRACE_DIR=` to retain per-turn JSON +with model, prompt, tool results, response ordering, usage, and finish reason. +These are local diagnostic artifacts; inspect before sharing. Successful reads +or full reference content in shell output count as loading; filename mentions, +denied commands, and failed reads do not. + +Context-only controls permit the real launcher with an optional workspace-relative +`--target`; compound commands remain rejected. The target form is part of the +skill's Setup contract, not a launcher failure. + +## Release investigation (2026-09-07) + +The initial release sweep reported 68/81 passes. Do not interpret its 13 failed +assertions as 13 demonstrated product regressions. The harness staged raw +references with unresolved placeholders, omitted generated degraded roles, and +allowed unanswered browser decisions. Its redesign assertion was also stale: +current `new-work.md` requires a surface brief **before code**, and DESIGN.md +**at finish**, from the built world. The corrected lifecycle checks retain +approval, brief, implementation, and final documentation requirements; missing +artifacts now have their own error instead of being called premature edits. +The historical tables below retain their original measurements and methods. + +Focused launcher-fallback verification on the corrected fixture: + +| Default model | Old fallback paragraph | Explicit pre-tool warning paragraph | +|---|---:|---:| +| `claude-sonnet-5` | 2/3 | 3/3 | +| `gpt-5.6-terra` | 3/3 | 3/3 | +| `gemini-3.7-flash` | 1/3 | 3/3 | + +The three cases are denied editing, successful-loader control, and denied +planning. The old-paragraph failures were warning order, not refused edits. +An intermediate candidate run scored 8/9 because the control rejected valid +`context --target index.html`; after correcting that allowlist, the full focused +rerun passed 9/9. This is one measured run per variant, not a reliability estimate +or an all-workflow pass. Broader routing and workflow results remain separate. + ## Scenarios | # | Setup | Assertion | @@ -368,9 +413,9 @@ IMPECCABLE_QUESTION_DISABLED=1 CI=1 IMPECCABLE_SKILL_BEHAVIOR_MODELS=deepseek-v4 --test-name-pattern="bolder refinement" tests/skill-behavior/workflow-contract.test.mjs ``` -Keep `--test-timeout` at 300000. A tighter cap turns claude-sonnet-5's slower -runs into timeouts that look like failures. Set `IMPECCABLE_QUESTION_DISABLED=1` -and `CI=1` so `impeccable serve-question` cannot open a browser window on the host. Pipe +Use the suite's current 900000ms timeout for full workflow cases; the 300000ms +example above is historical. The harness now disables decision pages itself. +Pipe to a file rather than `tail`; node prints the failing-test summary at the end, and truncating it costs you the per-model attribution. diff --git a/tests/skill-behavior/assertions.mjs b/tests/skill-behavior/assertions.mjs index 3e48012fa..bc11ed9e1 100644 --- a/tests/skill-behavior/assertions.mjs +++ b/tests/skill-behavior/assertions.mjs @@ -1,5 +1,21 @@ import assert from 'node:assert/strict'; +export function assertNewWorkLifecycle(trace, { target, redesign = false }) { + const calls = trace.toolCalls; + const writes = (call, file) => (call.mutatedPaths || []).includes(file); + const implementation = calls.findIndex((call) => writes(call, target)); + const question = calls.findIndex((call) => call.name === 'ask_user_question'); + const brief = calls.findIndex((call) => (call.mutatedPaths || []).some((file) => file.startsWith('.impeccable/surfaces/'))); + assert.ok(implementation >= 0, `new-work did not produce the requested artifact: ${target}`); + assert.ok(question >= 0 && question < implementation, 'implementation must follow a user answer'); + assert.ok(brief >= 0 && brief < implementation, 'the direction contract must be recorded in a surface brief before implementation'); + if (redesign) { + const lastImplementation = calls.findLastIndex((call) => writes(call, target)); + const documentation = calls.findLastIndex((call) => writes(call, 'DESIGN.md')); + assert.ok(documentation > lastImplementation, 'redesign must record DESIGN.md from the finished build, after the last page edit'); + } +} + export const LAUNCHER_FAILURE_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; export function assertPlanningFallbackWarning(responseMessages) { diff --git a/tests/skill-behavior/harness.mjs b/tests/skill-behavior/harness.mjs index d9e0bc922..b725315ca 100644 --- a/tests/skill-behavior/harness.mjs +++ b/tests/skill-behavior/harness.mjs @@ -1,9 +1,9 @@ /** - * Sandboxed scenario runner for skill-behavior tests. + * Synthetic-workspace scenario runner for skill-behavior tests. * * Each scenario: * 1. Creates a temp workspace. - * 2. Symlinks the real .claude/skills/impeccable into the workspace so + * 2. Builds a neutral .claude/skills/impeccable into the workspace so * the launcher (`scripts/impeccable`) resolves from the canonical path * the skill references, and points it at an engine binary. * 3. Optionally writes PRODUCT.md / DESIGN.md fixtures. @@ -15,8 +15,8 @@ * messages (so multi-turn scenarios can append to them). * * The harness deliberately mirrors the live-mode E2E pattern: real LLM, - * no mocks, but tightly bounded execution surface so we observe the routing - * behavior of the skill without paying for full-fledged design work. + * no mocked model. File tools are workspace-scoped; bash is a real host shell, + * not a security sandbox. Run only against disposable synthetic fixtures. */ import { generateText, stepCountIs, tool } from 'ai'; import { z } from 'zod'; @@ -28,12 +28,35 @@ import { spawn } from 'node:child_process'; import { fileURLToPath } from 'node:url'; import { getProviderOptions } from './providers.mjs'; import { ENGINE_MISSING_MESSAGE, findEngineBinary } from '../lib/engine-bin.mjs'; +import { readSourceFiles, compileProviderBlocks, replacePlaceholders, stripRuleMarkers } from '../../scripts/lib/utils.js'; +import { createTransformer } from '../../scripts/lib/transformers/factory.js'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); const REPO_ROOT = path.resolve(__dirname, '..', '..'); -const SKILL_SOURCE_DIR = path.join(REPO_ROOT, 'skill'); const MAX_BASH_OUTPUT_BYTES = 200_000; +function renderNeutral(content) { + return stripRuleMarkers(replacePlaceholders(compileProviderBlocks(content, []) + .replaceAll('{{ask_instruction}}', 'Use the ask_user_question tool.') + .replaceAll('{{model}}', 'the assistant'), 'dsh')) + .replaceAll('{{scripts_path}}', '.claude/skills/impeccable/scripts') + .replaceAll('{{command_hint}}', 'command'); +} + +// Use the production builder so fallback reviewer/documenter references exist. +// Generic tool names are shared by the API providers; host-specific blocks are +// deliberately absent. Exact provider transforms have separate loader tests. +const sourceSkills = readSourceFiles(REPO_ROOT).skills.map((skill) => ({ + ...skill, + body: renderNeutral(skill.body), + references: skill.references.map((ref) => ({ ...ref, content: renderNeutral(ref.content) })), + agents: skill.agents.map((agent) => ({ ...agent, body: renderNeutral(agent.body) })), +})); +const stageSkill = createTransformer({ + provider: 'skill-behavior', placeholderProvider: 'dsh', providerTags: [], + configDir: '.claude', displayName: 'Behavior fixture', +}); + function snapshotWorkspaceFiles(root) { const snapshot = new Map(); const walk = (dir, relDir = '') => { @@ -66,24 +89,7 @@ function changedPaths(before, after) { * is provider-neutral when inlined. */ function loadSkillBody() { - let md = fs.readFileSync(path.join(SKILL_SOURCE_DIR, 'SKILL.src.md'), 'utf8'); - // Strip frontmatter. - if (md.startsWith('---')) { - const end = md.indexOf('\n---', 3); - if (end !== -1) md = md.slice(end + 4).trimStart(); - } - // The source uses placeholders that the build step replaces per-provider. - // For the test harness we want a single body that works for any provider, - // and the scripts the skill references live at .claude/skills/impeccable/ - // (the workspace symlink), so hard-code those values. - md = md - .replaceAll('{{model}}', 'the assistant') - .replaceAll('{{command_prefix}}', '/') - .replaceAll('{{ask_instruction}}', 'Use the ask_user_question tool.') - .replaceAll('{{config_file}}', 'AGENTS.md') - .replaceAll('{{scripts_path}}', '.claude/skills/impeccable/scripts') - .replaceAll('{{command_hint}}', 'command'); - return md.trim(); + return sourceSkills[0].body.trim(); } // This provider-neutral fixture assumes a loaded skill with a known base @@ -96,36 +102,26 @@ export const SKILL_BODY = `Base directory for this skill (workspace-relative): . /** * Create a temp workspace and prepopulate it. * - * - `.claude/skills/impeccable` is symlinked at the SOURCE skill dir (not - * the built `.claude/skills/impeccable/`) so the test exercises whatever - * is in `skill/` right now, without needing `bun run build` to refresh - * the harness output dirs. The trade-off: reference files surface their - * raw `{{placeholders}}`, but the assertions only check tool calls, not - * their content. + * - Compile current source into an independent fixture distribution. Shell + * and read tools see the same resolved references, including degraded roles. * - `files` lets the test seed PRODUCT.md / DESIGN.md (or anything else). - * - `skillVersion` switches from symlink to a real COPY of the skill dir and - * writes a `SKILL.md` carrying that version. `impeccable context` reads its + * - `skillVersion` adds a `SKILL.md` version. `impeccable context` reads its * own version from that sibling file, so this is required for any scenario * that exercises the update-check path (the source dir has only SKILL.src.md). * * The launcher in the staged scripts dir needs an engine binary. Every bash * call the agent makes gets `IMPECCABLE_BIN` (tests/lib/engine-bin.mjs: * `IMPECCABLE_BIN` or `skill/scripts/bin/-/`), which the launcher - * honors first, so the symlink and copy modes both work without a download. + * honors first, so the staged skill works without a download. */ export const ENGINE_BIN = findEngineBinary(); export { ENGINE_MISSING_MESSAGE }; export function prepareWorkspace({ files = {}, skillVersion = null } = {}) { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'impeccable-skill-test-')); - const skillDest = path.join(dir, '.claude', 'skills', 'impeccable'); - fs.mkdirSync(path.join(dir, '.claude', 'skills'), { recursive: true }); - if (skillVersion) { - fs.cpSync(SKILL_SOURCE_DIR, skillDest, { recursive: true }); - fs.writeFileSync(path.join(skillDest, 'SKILL.md'), `---\nname: impeccable\nversion: ${skillVersion}\n---\n\nbody\n`); - } else { - fs.symlinkSync(SKILL_SOURCE_DIR, skillDest, 'dir'); - } + stageSkill(sourceSkills, dir, { skillsVersion: skillVersion || '' }); + fs.renameSync(path.join(dir, 'skill-behavior', '.claude'), path.join(dir, '.claude')); + fs.rmdirSync(path.join(dir, 'skill-behavior')); for (const [name, contents] of Object.entries(files)) { const target = path.join(dir, name); fs.mkdirSync(path.dirname(target), { recursive: true }); @@ -157,11 +153,22 @@ function safeResolve(root, userPath) { return resolved; } +function isContextOnlyCommand(workspace, command) { + const match = command.trim().match(/^\.claude\/skills\/impeccable\/scripts\/impeccable context(?: --target (?:"([a-zA-Z0-9_./ -]+)"|'([a-zA-Z0-9_./ -]+)'|([a-zA-Z0-9_./-]+)))?$/); + if (!match) return false; + const target = match[1] ?? match[2] ?? match[3]; + return target === undefined || (!target.startsWith('-') && typeof safeResolve(workspace, target) === 'string'); +} + function execBash(workspace, command, timeoutMs = 20_000, extraEnv = {}) { return new Promise((resolve) => { + // Model credentials belong to generateText, not to child image helpers. + // Real decision pages have browser E2E; this suite has a structured user. + const shellEnv = Object.fromEntries(Object.entries({ ...process.env, ...extraEnv }) + .filter(([name]) => !/(?:^|_)(?:API_KEY|AUTH_TOKEN|ACCESS_TOKEN)$/.test(name))); const proc = spawn('bash', ['-lc', command], { cwd: workspace, - env: { ...process.env, ...(ENGINE_BIN ? { IMPECCABLE_BIN: ENGINE_BIN } : {}), ...extraEnv }, + env: { ...shellEnv, ...(ENGINE_BIN ? { IMPECCABLE_BIN: ENGINE_BIN } : {}), IMPECCABLE_QUESTION_DISABLED: '1' }, }); let stdout = ''; let stderr = ''; @@ -225,6 +232,10 @@ function defaultSimulatedAnswer(question) { } export function makeTools(workspace, extraEnv = {}, simulatedUser = {}, { contextOnlyBash = false, denyBash = false } = {}) { + const referenceDir = path.join(workspace, '.claude/skills/impeccable/reference'); + const references = fs.readdirSync(referenceDir, { recursive: true }) + .filter((file) => file.endsWith('.md')) + .map((file) => ({ file: file.split(path.sep).join('/'), content: fs.readFileSync(path.join(referenceDir, file), 'utf8').trim() })); const trace = { toolCalls: [], bashCommands: [], @@ -248,7 +259,7 @@ export function makeTools(workspace, extraEnv = {}, simulatedUser = {}, { contex const tools = { bash: tool({ description: contextOnlyBash - ? 'Only `.claude/skills/impeccable/scripts/impeccable context` is allowed here. Use read/list for workspace files and skill references; write remains available for requested edits.' + ? 'Only `.claude/skills/impeccable/scripts/impeccable context` with an optional `--target ` is allowed here. Use read/list for files and references; write remains available for requested edits.' : 'Run a bash command in the workspace root. Use this to invoke skill commands (e.g. `.claude/skills/impeccable/scripts/impeccable context`).', inputSchema: z.object({ command: z.string().describe('The bash command to execute.'), @@ -265,14 +276,16 @@ export function makeTools(workspace, extraEnv = {}, simulatedUser = {}, { contex } // 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') { - const out = 'Error: only `.claude/skills/impeccable/scripts/impeccable context` is allowed. Use read/list for files; references live at .claude/skills/impeccable/reference/.'; + if (contextOnlyBash && !isContextOnlyCommand(workspace, command)) { + const out = 'Error: only `.claude/skills/impeccable/scripts/impeccable context` with an optional workspace-relative `--target` is allowed. Use read/list for files; references live at .claude/skills/impeccable/reference/.'; trace.bashOutputs.push(out); return out; } const before = snapshotWorkspaceFiles(workspace); const res = await execBash(workspace, command, 20_000, extraEnv); call.mutatedPaths = changedPaths(before, snapshotWorkspaceFiles(workspace)); + call.loadedFiles = references.filter(({ content }) => content && res.stdout.includes(content)) + .map(({ file }) => `.claude/skills/impeccable/reference/${file}`); const head = `exit=${res.exitCode}`; const body = (res.stdout ? `stdout:\n${res.stdout}` : '') + (res.stderr ? `\nstderr:\n${res.stderr}` : ''); const out = `${head}\n${body}${res.truncated ? '\n[output truncated]' : ''}`; @@ -295,6 +308,7 @@ export function makeTools(workspace, extraEnv = {}, simulatedUser = {}, { contex if (stat.isDirectory()) return `Path is a directory: ${p}. Use list instead.`; const contents = fs.readFileSync(resolved, 'utf8'); call.succeeded = true; + call.loadedFiles = [p]; return contents; }, }), @@ -308,7 +322,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 || denyBash) && path.relative(workspace, resolved).split(path.sep)[0] === '.claude') { + if (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 }); @@ -392,6 +406,13 @@ export async function runTurn({ workspace, model, userPrompt, priorMessages = [] ...priorMessages, { role: 'user', content: userPrompt }, ]; + const traceDir = process.env.IMPECCABLE_SKILL_BEHAVIOR_TRACE_DIR; + const tracePath = traceDir && path.join(traceDir, `${path.basename(workspace)}-${crypto.randomUUID()}.json`); + const saveTrace = (details) => { + if (!tracePath) return; + fs.mkdirSync(traceDir, { recursive: true }); + fs.writeFileSync(tracePath, JSON.stringify({ model: model.modelId, userPrompt, trace, ...details }, null, 2)); + }; let result; const controller = new AbortController(); const timer = setTimeout( @@ -405,6 +426,7 @@ export async function runTurn({ workspace, model, userPrompt, priorMessages = [] system: SKILL_BODY, messages, tools, + onStepFinish: tracePath ? (step) => saveTrace({ status: 'in-progress', lastStepMessages: step.response.messages }) : undefined, stopWhen: [stepCountIs(maxSteps)], // Real client-side deadline on the provider call: without it a stalled // stream wedges the whole sweep with no tally. @@ -420,12 +442,15 @@ export async function runTurn({ workspace, model, userPrompt, priorMessages = [] }); } catch (err) { const reason = controller.signal.aborted ? ` (aborted after ${timeoutMs}ms client-side timeout)` : ''; + saveTrace({ status: 'failed', error: `${String(err)}${reason}` }); throw new Error(`LLM behavior turn failed before completing${reason}: ${String(err)}`, { cause: err }); } finally { clearTimeout(timer); } const generatedResponseMessages = result.responseMessages ?? result.response?.messages ?? []; const responseMessages = [...messages, ...generatedResponseMessages]; + saveTrace({ status: 'completed', responseMessages, + finishReason: result.finishReason, steps: result.steps.length, usage: result.usage }); return { trace, text: result.text ?? '', @@ -451,8 +476,12 @@ export function readsMatching(trace, substring) { * True if the agent loaded a file by Read OR by a bash `cat` (some models * stream multiple files via bash to save tool calls). */ +export function callLoadedFile(call, filename) { + return (call.loadedFiles || []).some((file) => file === filename || file.endsWith(`/${filename}`)); +} + export function fileLoaded(trace, filename) { - return readsMatching(trace, filename).length > 0 || bashCommandsMatching(trace, filename).length > 0; + return trace.toolCalls.some((call) => callLoadedFile(call, filename)); } export function summarizeTrace(trace) { diff --git a/tests/skill-behavior/scenarios.test.mjs b/tests/skill-behavior/scenarios.test.mjs index 6cd2cdc5b..ed1f30f54 100644 --- a/tests/skill-behavior/scenarios.test.mjs +++ b/tests/skill-behavior/scenarios.test.mjs @@ -22,6 +22,7 @@ import { bashCommandsMatching, readsMatching, fileLoaded, + callLoadedFile, summarizeTrace, ENGINE_BIN, ENGINE_MISSING_MESSAGE, @@ -57,14 +58,9 @@ function logTrace(label, scenario, model, trace, extras = {}) { } function loadedBeforeImplementationWrite(trace, filename) { - const needle = filename.toLowerCase(); - const loadIndex = trace.toolCalls.findIndex(({ name, input }) => { - if (name === 'read') return input?.path?.toLowerCase().includes(needle); - if (name === 'bash') return input?.command?.toLowerCase().includes(needle); - return false; - }); + const loadIndex = trace.toolCalls.findIndex((call) => callLoadedFile(call, filename)); const writeIndex = trace.toolCalls.findIndex( - ({ name, input }) => name === 'write' && /\.(html?|css|svelte|jsx?|tsx?)$/i.test(input?.path ?? ''), + ({ mutatedPaths = [] }) => mutatedPaths.some((file) => /\.(html?|css|svelte|jsx?|tsx?)$/i.test(file)), ); return loadIndex >= 0 && (writeIndex < 0 || loadIndex < writeIndex); } diff --git a/tests/skill-behavior/workflow-contract.test.mjs b/tests/skill-behavior/workflow-contract.test.mjs index 93dc38716..2ed8c7f33 100644 --- a/tests/skill-behavior/workflow-contract.test.mjs +++ b/tests/skill-behavior/workflow-contract.test.mjs @@ -17,6 +17,7 @@ import { ENGINE_MISSING_MESSAGE, } from './harness.mjs'; import { detectProvider, getModel, hasKey, resolveModelList, PROVIDERS } from './providers.mjs'; +import { assertNewWorkLifecycle } from './assertions.mjs'; import { PRODUCT_MD_SAMPLE, DESIGN_MD_SAMPLE } from './fixtures.mjs'; const LEGACY_DESIGN = `# Design @@ -150,17 +151,16 @@ for (const modelId of resolveModelList()) { maxSteps: 22, }); const question = firstCall(trace, ({ name }) => name === 'ask_user_question'); - const implementation = firstMutation(trace, /\.(?:html?|astro|svelte|jsx?|tsx?)$/i); assert.ok(fileLoaded(trace, 'new-work.md'), `new-work.md was not loaded.\n${workflowTraceMessage(trace)}`); assert.ok(question >= 0, `task concept was never put to the user.\n${workflowTraceMessage(trace)}`); - assert.ok(implementation > question, `implementation began before the attended concept checkpoint.\n${workflowTraceMessage(trace)}`); + assertNewWorkLifecycle(trace, { target: 'index.html' }); assert.equal(fs.existsSync(path.join(workspace, 'index.html')), true, 'new-work must still produce the requested artifact'); } finally { cleanupWorkspace(workspace); } }); - it('redesign replaces DESIGN before touching the existing page', async () => { + it('redesign approves and records the direction before code, then documents the built world', async () => { const workspace = prepareWorkspace({ files: { 'PRODUCT.md': PRODUCT_MD_SAMPLE, @@ -176,12 +176,9 @@ for (const modelId of resolveModelList()) { maxSteps: 26, }); const question = firstCall(trace, ({ name }) => name === 'ask_user_question'); - const designWrite = firstMutation(trace, /(^|\/)DESIGN\.md$/i); - const implementation = firstMutation(trace, /(^|\/)current\.html$/i); assert.ok(fileLoaded(trace, 'new-work.md'), `redesign did not route through new-work.\n${workflowTraceMessage(trace)}`); assert.ok(question >= 0, `replacement world was not put to the user.\n${workflowTraceMessage(trace)}`); - assert.ok(designWrite > question, `replacement DESIGN.md must follow user choice.\n${workflowTraceMessage(trace)}`); - assert.ok(implementation > designWrite, `redesign touched the page before replacing DESIGN.md.\n${workflowTraceMessage(trace)}`); + assertNewWorkLifecycle(trace, { target: 'current.html', redesign: true }); const design = fs.readFileSync(path.join(workspace, 'DESIGN.md'), 'utf8'); assert.notEqual(design.trim(), LEGACY_DESIGN.trim(), 'redesign preserved the old visual world verbatim'); } finally {