mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-11 21:57:14 +03:00
Clarify launcher-refusal fallback and correct behavior tests (#756)
* Clarify permitted work after launcher refusal Correct behavior-test skill metadata and DeepSeek output limits; document the measured remaining Gemini warning-order failure and hook download trust boundary. AI assistance: Codex, under pbakaus direction. * Clarify behavior harness host-modeling scope AI assistance: Codex, under pbakaus direction. * Preserve planning-only scope after launcher refusal Clarify applicable setup steps and cover denied-launcher planning. Retain the observed playbook-read failure under issue #744 rather than weakening its assertion. AI assistance: Codex, under pbakaus direction. * Test planning fallback warning order Require an assistant warning after context launcher denial and before fallback context reads. Cover silent, late, and unrelated warnings with deterministic tests; retain the observed Sonnet omission under #744. AI assistance: Codex, under pbakaus direction.
This commit is contained in:
Paul Bakaus
@@ -397,6 +397,8 @@ Installed hook surfaces:
|
||||
|
||||
Every command goes through the launcher shipped in the skill's `scripts/` directory (`impeccable`, or `impeccable.cmd` on Windows), guarded so a missing launcher is a silent no-op. The launcher runs the engine binary that ships next to it, or downloads the pinned version once into `~/.impeccable/bin/`. No Node or other runtime is required for the hook or the skill.
|
||||
|
||||
In Claude Code, installed command hooks run independently of model-tool approval. The first edit or Stop event can therefore download and cache the engine even if the session denies the model's launcher command. Review installed hooks before unattended runs; to disable all Claude Code hooks for a run, pass `--settings '{"disableAllHooks": true}'`. See [Claude Code's hook security guidance](https://code.claude.com/docs/en/hooks#security-considerations).
|
||||
|
||||
The installer preserves unrelated hook entries and settings. If a hook manifest is malformed, install/update aborts by default; rerun with `--force` to back up the malformed file as `.bak` and replace it.
|
||||
|
||||
On an interactive `install`/`update`, Impeccable explains the hook and offers to install it (default yes). Your choice is remembered per-developer in the gitignored `.impeccable/config.local.json`, so you are not asked again; `--no-hooks` skips it for that run without recording anything. Hook lifecycle settings live under the `hook` key of `.impeccable/config.json`; detector ignores live under `detector`, shared by `/impeccable hooks` and `npx impeccable detect`.
|
||||
|
||||
+4
-2
@@ -18,9 +18,11 @@ Core principles:
|
||||
|
||||
## Setup
|
||||
|
||||
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 -->
|
||||
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. <!-- 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 -->
|
||||
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. <!-- rule:skill-craft-floor-load -->
|
||||
|
||||
**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.
|
||||
|
||||
## How to design
|
||||
|
||||
|
||||
@@ -2,7 +2,66 @@ import { it } from 'node:test';
|
||||
import assert from 'node:assert/strict';
|
||||
import fs from 'node:fs';
|
||||
import path from 'node:path';
|
||||
import { prepareWorkspace, cleanupWorkspace, makeTools } from './skill-behavior/harness.mjs';
|
||||
import { MockLanguageModelV3 } from 'ai/test';
|
||||
import { prepareWorkspace, cleanupWorkspace, makeTools, runTurn, SKILL_BODY } from './skill-behavior/harness.mjs';
|
||||
import { assertPlanningFallbackWarning } from './skill-behavior/assertions.mjs';
|
||||
|
||||
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' } }] };
|
||||
const denial = { role: 'tool', content: [{ type: 'tool-result', toolCallId: 'context', toolName: 'bash', output: { type: 'text', value: 'Error: Bash permission denied by the host. This command was not executed.' } }] };
|
||||
const warning = { role: 'assistant', content: 'Context loading did not run because the launcher was denied.' };
|
||||
const read = { role: 'assistant', content: [{ type: 'tool-call', toolCallId: 'read', toolName: 'read', input: { path: 'PRODUCT.md' } }] };
|
||||
assert.doesNotThrow(() => assertPlanningFallbackWarning([call, denial, warning, read]));
|
||||
assert.doesNotThrow(() => assertPlanningFallbackWarning([call, denial, { role: 'assistant', content: [{ type: 'text', text: warning.content }, ...read.content] }]));
|
||||
for (const messages of [
|
||||
[call, denial, read], // Silent continuation.
|
||||
[call, denial, read, warning], // Final-only disclosure.
|
||||
[warning, call, denial, read], // Not a response to the actual denial.
|
||||
[call, denial, { ...warning, role: 'user' }, read],
|
||||
[call, { ...denial, content: [{ ...denial.content[0], toolCallId: 'unrelated' }] }, warning, read],
|
||||
]) {
|
||||
assert.throws(() => assertPlanningFallbackWarning(messages), assert.AssertionError);
|
||||
}
|
||||
});
|
||||
|
||||
it('DeepSeek gets an explicit output ceiling instead of the compatibility SDK default', async () => {
|
||||
const workspace = prepareWorkspace();
|
||||
try {
|
||||
for (const modelId of ['deepseek-v4-flash', 'claude-sonnet-5']) {
|
||||
const model = new MockLanguageModelV3({
|
||||
modelId,
|
||||
doGenerate: {
|
||||
content: [{ type: 'text', text: 'done' }],
|
||||
finishReason: { unified: 'stop', raw: 'stop' },
|
||||
usage: { inputTokens: { total: 1 }, outputTokens: { total: 1 } },
|
||||
warnings: [],
|
||||
},
|
||||
});
|
||||
await runTurn({ workspace, model, userPrompt: 'Test the harness.', maxSteps: 1 });
|
||||
const request = model.doGenerateCalls[0];
|
||||
assert.equal(request.maxOutputTokens, modelId.startsWith('deepseek-') ? 16_384 : undefined);
|
||||
assert.ok(request.prompt.some((message) => message.role === 'system' && message.content === SKILL_BODY));
|
||||
}
|
||||
} finally {
|
||||
cleanupWorkspace(workspace);
|
||||
}
|
||||
});
|
||||
|
||||
it('loaded-skill metadata resolves to the staged launcher and readable references', async () => {
|
||||
const workspace = prepareWorkspace();
|
||||
try {
|
||||
const baseDir = SKILL_BODY.match(/^Base directory for this skill \(workspace-relative\): (.+)$/m)?.[1];
|
||||
assert.ok(baseDir, 'the host must supply the skill directory separately from its instructions');
|
||||
assert.ok(fs.statSync(path.join(workspace, baseDir, 'scripts/impeccable')).isFile());
|
||||
const { tools, trace } = makeTools(workspace, {}, {}, { denyBash: true });
|
||||
await tools.read.execute({ path: `${baseDir}/reference/polish.md` });
|
||||
await tools.read.execute({ path: `${baseDir}/reference/craft-floor.md` });
|
||||
assert.ok(trace.toolCalls.every((call) => call.succeeded));
|
||||
assert.ok(SKILL_BODY.includes('<skill-base-dir>/scripts/impeccable context'), 'metadata must not rewrite away the path-resolution behavior under test');
|
||||
} finally {
|
||||
cleanupWorkspace(workspace);
|
||||
}
|
||||
});
|
||||
|
||||
it('denied-launcher tools reject every shell attempt without executing or modifying the skill', async () => {
|
||||
const workspace = prepareWorkspace({ files: { 'index.html': 'before' } });
|
||||
|
||||
@@ -75,7 +75,7 @@ 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 |
|
||||
| 19 | tiny spacing edit with PRODUCT.md + DESIGN.md; Bash denied, a real-loader success control, and a denied-launcher planning-only case | edits require successful playbook/craft-floor reads and a pre-edit denial warning; planning stays read-only and skips craft-floor |
|
||||
|
||||
## Setup launcher-failure branch (2026-09-06, PR #750)
|
||||
|
||||
@@ -98,6 +98,67 @@ 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.
|
||||
|
||||
### Refusal follow-up (2026-09-06, #744)
|
||||
|
||||
The provider-neutral harness models a loaded skill with a known base directory
|
||||
using synthetic workspace-relative host metadata, not each provider's exact
|
||||
generated prompt. The source instructions and `<skill-base-dir>` resolution
|
||||
remain under test; reference reads still have to succeed. Provider transforms
|
||||
and plugin loading have separate path/loader tests. The harness also sets an
|
||||
explicit 16,384-token response ceiling for DeepSeek: the Anthropic-compatible
|
||||
SDK otherwise treats that model as unknown and caps it at 4,096. Truncation
|
||||
still fails the scenario; this changes the test runner, not the shipped skill.
|
||||
|
||||
The unchanged-source baseline with directory metadata passed 5/8 focused
|
||||
cases: Sonnet skipped craft-floor in its successful-launcher control, OpenAI
|
||||
stopped without editing after denial, and Gemini warned only after editing.
|
||||
DeepSeek passed both cases. An initial candidate got OpenAI to edit but still
|
||||
warned late on Sonnet, OpenAI, and Gemini; DeepSeek's denial response truncated.
|
||||
All four successful-launcher controls passed that candidate.
|
||||
|
||||
The pre-review candidate separates the fallback from the long first step, says to
|
||||
send the warning first and continue through permitted tools, and clarifies
|
||||
that craft-floor also applies to small refinements. Setup grows by 18
|
||||
whitespace-separated words; the description is unchanged. Sonnet and OpenAI
|
||||
passed both final cases, as did DeepSeek with the explicit output ceiling.
|
||||
Gemini still warned after the edit; its control passed. The final result is
|
||||
7/8; the warning-order assertion remains unchanged.
|
||||
|
||||
Review follow-up: the fallback now says to follow the **applicable** steps
|
||||
2–3, preserving step 3's planning-only exclusion (20 added Setup words overall).
|
||||
A new denied-launcher planning case requires a real plan, no mutations or
|
||||
craft-floor read, and successful context/target/playbook reads. On Sonnet,
|
||||
the editing denial and successful-loader cases both passed again. The planning
|
||||
run stayed read-only and skipped craft-floor, but failed because it did not
|
||||
read `polish.md`. That assertion remains intact: this is another reference-loading
|
||||
gap under #744, not a green planning result. Other providers were not rerun for
|
||||
this wording-only review clarification.
|
||||
|
||||
The planning case also checks the response-message sequence: the launcher must
|
||||
actually be denied, then an assistant warning must precede the first fallback
|
||||
PRODUCT.md or DESIGN.md read. Deterministic tests reject silent continuation,
|
||||
final-only warnings, warnings before denial, and user-authored warnings. This
|
||||
checks disclosure even when no editing occurs; the editing cases retain their
|
||||
existing pre-edit warning assertion.
|
||||
|
||||
One focused Sonnet rerun with this guard read the playbook and produced a
|
||||
read-only plan without craft-floor, but omitted the launcher warning entirely.
|
||||
The strengthened assertion correctly failed that run; #744 remains open for
|
||||
the behavior failure rather than treating this coverage fix as a skill fix.
|
||||
|
||||
These are single samples per case and candidate, not reliability estimates.
|
||||
This API harness starts with the skill loaded and readable references. It
|
||||
does not measure activation, reproduce Windows command parsing, or establish
|
||||
fallback behavior when the host also denies required file reads or writes.
|
||||
Keep #744 open; evaluate activation separately with #375.
|
||||
|
||||
To repeat only these cases (provider keys and an engine binary required):
|
||||
|
||||
```sh
|
||||
IMPECCABLE_SKILL_BEHAVIOR_MODELS=claude-sonnet-5,gpt-5.6-terra,gemini-3.7-flash,deepseek-v4-flash \
|
||||
node --test --test-name-pattern='scenario 19:' tests/skill-behavior/scenarios.test.mjs
|
||||
```
|
||||
|
||||
## Workflow-advice baseline (2026-09-05, PR #737)
|
||||
|
||||
The four cases in scenarios 16-18 are new; prior scenario results do not
|
||||
|
||||
@@ -0,0 +1,24 @@
|
||||
import assert from 'node:assert/strict';
|
||||
|
||||
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) {
|
||||
const blocks = responseMessages.flatMap((message) =>
|
||||
(typeof message.content === 'string' ? [{ type: 'text', text: message.content }] : message.content)
|
||||
.map((block) => ({ ...block, role: message.role })),
|
||||
);
|
||||
const contextCalls = new Set(blocks.filter((block) => block.role === 'assistant'
|
||||
&& block.type === 'tool-call' && block.toolName === 'bash'
|
||||
&& /impeccable\s+context\b/.test(block.input?.command ?? '')).map((block) => block.toolCallId));
|
||||
const denialIndex = blocks.findIndex((block) => block.role === 'tool'
|
||||
&& block.type === 'tool-result' && contextCalls.has(block.toolCallId)
|
||||
&& block.output?.type === 'text' && /Bash permission denied by the host/.test(block.output.value));
|
||||
assert.ok(denialIndex >= 0, 'must observe the context launcher denial in the response sequence');
|
||||
const warningIndex = blocks.findIndex((block, index) => index > denialIndex
|
||||
&& block.role === 'assistant' && block.type === 'text' && LAUNCHER_FAILURE_WARNING.test(block.text));
|
||||
const contextReadIndex = blocks.findIndex((block, index) => index > denialIndex
|
||||
&& block.role === 'assistant' && block.type === 'tool-call' && block.toolName === 'read'
|
||||
&& /(?:^|\/)(?:PRODUCT|DESIGN)\.md$/.test(block.input?.path ?? ''));
|
||||
assert.ok(warningIndex > denialIndex && contextReadIndex > warningIndex,
|
||||
'planning fallback must warn after denial and before reading project context, not only in the final response');
|
||||
}
|
||||
@@ -86,7 +86,12 @@ function loadSkillBody() {
|
||||
return md.trim();
|
||||
}
|
||||
|
||||
export const SKILL_BODY = loadSkillBody();
|
||||
// This provider-neutral fixture assumes a loaded skill with a known base
|
||||
// directory, not an exact copy of each host's transformed prompt. Claude's
|
||||
// loader supplies a base-directory prefix; here it is workspace-relative
|
||||
// because the file tools reject absolute paths. Provider rewrite/loader
|
||||
// contracts are tested separately, not established by these behavior cases.
|
||||
export const SKILL_BODY = `Base directory for this skill (workspace-relative): .claude/skills/impeccable\n\n${loadSkillBody()}`;
|
||||
|
||||
/**
|
||||
* Create a temp workspace and prepopulate it.
|
||||
@@ -404,6 +409,10 @@ export async function runTurn({ workspace, model, userPrompt, priorMessages = []
|
||||
// Real client-side deadline on the provider call: without it a stalled
|
||||
// stream wedges the whole sweep with no tally.
|
||||
abortSignal: controller.signal,
|
||||
// The Anthropic-compatible adapter does not recognize DeepSeek and
|
||||
// otherwise caps each response at 4096 tokens, truncating valid tool
|
||||
// continuations. Keep an explicit ceiling; length remains a test failure.
|
||||
maxOutputTokens: model?.modelId?.startsWith('deepseek-') ? 16_384 : undefined,
|
||||
// Resolved from the model object so the 21 runTurn call sites stay
|
||||
// unchanged. Reasoning models run at the provider default otherwise,
|
||||
// which is not the tier this suite is meant to measure.
|
||||
|
||||
@@ -27,6 +27,7 @@ import {
|
||||
ENGINE_MISSING_MESSAGE,
|
||||
} from './harness.mjs';
|
||||
import { detectProvider, getModel, hasKey, resolveModelList, PROVIDERS } from './providers.mjs';
|
||||
import { assertPlanningFallbackWarning, LAUNCHER_FAILURE_WARNING } from './assertions.mjs';
|
||||
import {
|
||||
PRODUCT_MD_SAMPLE,
|
||||
PRODUCT_MD_SAMPLE_NO_REGISTER,
|
||||
@@ -682,10 +683,9 @@ for (const modelId of resolveModelList()) {
|
||||
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 warningIndex = assistantBlocks.findIndex((block) => block.type === 'text' && LAUNCHER_FAILURE_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');
|
||||
@@ -695,6 +695,34 @@ for (const modelId of resolveModelList()) {
|
||||
});
|
||||
}
|
||||
|
||||
it('scenario 19: denied launcher keeps planning-only work read-only without craft-floor', async () => {
|
||||
const workspace = prepareWorkspace({ files: {
|
||||
'PRODUCT.md': PRODUCT_MD_SAMPLE,
|
||||
'DESIGN.md': DESIGN_MD_SAMPLE,
|
||||
'index.html': '<!doctype html><html><body><button style="padding:2px 4px">New note</button></body></html>',
|
||||
} });
|
||||
try {
|
||||
const { trace, text, stepTexts, finishReason, responseMessages } = await runTurn({
|
||||
workspace,
|
||||
model,
|
||||
userPrompt: '/impeccable polish index.html. Inspect the button spacing and propose a short plan only. Do not edit any files or implement the plan yet.',
|
||||
maxSteps: 12,
|
||||
denyBash: true,
|
||||
});
|
||||
logTrace('S19', 'denied-launcher-planning', modelId, trace, { finishReason, text: stepTexts.join('\n') });
|
||||
assert.notEqual(finishReason, 'length', 'a truncated response is not a completed plan');
|
||||
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');
|
||||
assert.deepEqual(readsMatching(trace, 'craft-floor.md'), [], 'planning-only work must not load the editing floor');
|
||||
assertAdviceOnly(trace, text);
|
||||
assertPlanningFallbackWarning(responseMessages);
|
||||
for (const filename of ['PRODUCT.md', 'DESIGN.md', 'index.html', 'reference/polish.md']) {
|
||||
assert.ok(trace.toolCalls.some((call) => call.name === 'read' && call.succeeded && (call.input.path === filename || call.input.path.endsWith(`/${filename}`))), `${filename} must actually be read`);
|
||||
}
|
||||
} 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