From 0e5c6cbe17dd2528a12afbeec6d7830bd8167918 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Thu, 13 Aug 2026 01:06:33 -0400 Subject: [PATCH 1/4] Keep critique's report out of the question's message The critique report and the AskUserQuestion call shipped in one assistant message, so the report stayed hidden until the user answered the picker and the command read as if it had never run. Reorder critique's persistence steps so the temp-file cleanup runs after the report and trend line are sent. That cleanup now ends the message carrying the report, leaving the questions to open a fresh one. Both critique.md and overdrive.md state the constraint and why it exists, so the ordering is not an unexplained sequence a model can optimize away. Overdrive additionally moves its direction descriptions inside the question options, where the user is actually reading them. Also fix the ask_instruction splices. The placeholder is a complete sentence, but five call sites spliced it mid-sentence and shipped text like "stop and STOP and call the AskUserQuestion tool to clarify. before expanding it". Every call site is now sentence-initial and the twelve lowercase provider values are capitalized to match, with a comment in utils.js pinning the contract. Record a workflow-contract baseline for the current model lineup. The two failures seen while validating this change are pre-existing: bolder refinement fails on deepseek-v4-flash identically with bolder.md reverted to HEAD, and redesign replaces DESIGN is flaky on assertions driven by new-work.md, which this change does not touch. Prepared with AI assistance (Claude Code). Co-Authored-By: Claude Opus 5 (1M context) --- scripts/lib/utils.js | 24 +++++++------- skill/reference/bolder.md | 2 +- skill/reference/critique.md | 10 +++--- skill/reference/distill.md | 2 +- skill/reference/document.md | 2 +- skill/reference/extract.md | 2 +- skill/reference/overdrive.md | 2 +- skill/reference/quieter.md | 2 +- tests/lib/utils.test.js | 2 +- tests/skill-behavior/README.md | 57 ++++++++++++++++++++++++++++++++++ 10 files changed, 83 insertions(+), 22 deletions(-) diff --git a/scripts/lib/utils.js b/scripts/lib/utils.js index f3fe77367..a3fc3c03d 100644 --- a/scripts/lib/utils.js +++ b/scripts/lib/utils.js @@ -465,31 +465,33 @@ export const PROVIDER_PLACEHOLDERS = { 'cursor': { model: 'the model', config_file: '.cursorrules', - ask_instruction: 'ask the user directly to clarify what you cannot infer.', + ask_instruction: 'Ask the user directly to clarify what you cannot infer.', command_prefix: '/' }, 'gemini': { model: 'Gemini', config_file: 'GEMINI.md', - ask_instruction: 'ask the user directly to clarify what you cannot infer.', + ask_instruction: 'Ask the user directly to clarify what you cannot infer.', command_prefix: '/' }, 'codex': { model: 'GPT', config_file: 'AGENTS.md', + // Every {{ask_instruction}} call site is sentence-initial, so each value is a + // complete capitalized sentence. Keep it that way when adding a provider. ask_instruction: "STOP and use Codex's structured user-input/question tool when available; if unavailable, ask directly in chat to clarify what you cannot infer.", command_prefix: '$' }, 'agents': { model: 'the model', config_file: '.github/copilot-instructions.md', - ask_instruction: 'ask the user directly to clarify what you cannot infer.', + ask_instruction: 'Ask the user directly to clarify what you cannot infer.', command_prefix: '/' }, 'kiro': { model: 'Claude', config_file: '.kiro/settings.json', - ask_instruction: 'ask the user directly to clarify what you cannot infer.', + ask_instruction: 'Ask the user directly to clarify what you cannot infer.', command_prefix: '/' }, opencode: { @@ -501,31 +503,31 @@ export const PROVIDER_PLACEHOLDERS = { 'pi': { model: 'the model', config_file: 'AGENTS.md', - ask_instruction: 'ask the user directly to clarify what you cannot infer.', + ask_instruction: 'Ask the user directly to clarify what you cannot infer.', command_prefix: '/' }, 'qoder': { model: 'the model', config_file: 'AGENTS.md', - ask_instruction: 'ask the user directly to clarify what you cannot infer.', + ask_instruction: 'Ask the user directly to clarify what you cannot infer.', command_prefix: '/' }, 'trae': { model: 'the model', config_file: 'RULES.md', - ask_instruction: 'ask the user directly to clarify what you cannot infer.', + ask_instruction: 'Ask the user directly to clarify what you cannot infer.', command_prefix: '/' }, 'rovo-dev': { model: 'Rovo Dev', config_file: 'AGENTS.md', - ask_instruction: 'ask the user directly to clarify what you cannot infer.', + ask_instruction: 'Ask the user directly to clarify what you cannot infer.', command_prefix: '/' }, 'vibe': { model: 'Mistral', config_file: 'AGENTS.md', - ask_instruction: 'ask the user directly to clarify what you cannot infer.', + ask_instruction: 'Ask the user directly to clarify what you cannot infer.', command_prefix: '/' }, 'grok': { @@ -537,7 +539,7 @@ export const PROVIDER_PLACEHOLDERS = { 'antigravity': { model: 'Gemini', config_file: 'AGENTS.md', - ask_instruction: 'ask the user directly to clarify what you cannot infer.', + ask_instruction: 'Ask the user directly to clarify what you cannot infer.', command_prefix: '/' }, 'hermes': { @@ -546,7 +548,7 @@ export const PROVIDER_PLACEHOLDERS = { // for harnesses without a vendor-fixed assistant name. model: 'the model', config_file: 'AGENTS.md', - ask_instruction: 'ask the user directly to clarify what you cannot infer.', + ask_instruction: 'Ask the user directly to clarify what you cannot infer.', command_prefix: '/' } }; diff --git a/skill/reference/bolder.md b/skill/reference/bolder.md index aff6667b1..e06aa3212 100644 --- a/skill/reference/bolder.md +++ b/skill/reference/bolder.md @@ -6,7 +6,7 @@ An open direction round owns the word first: "bolder" said while a direction dec ## Scope is sovereign -"Everything else stays" is a literal instruction. Touch only the named target. Do not restyle its neighbors, do not migrate the page to a new idea, do not add colors, fonts, radii, shadows, or system primitives the surface does not already own. If the existing system genuinely cannot express the direction, stop and {{ask_instruction}} before expanding it, naming the exact addition and the job it would do. +"Everything else stays" is a literal instruction. Touch only the named target. Do not restyle its neighbors, do not migrate the page to a new idea, do not add colors, fonts, radii, shadows, or system primitives the surface does not already own. If the existing system genuinely cannot express the direction, do not expand it on your own. {{ask_instruction}} Name the exact addition and the job it would do. ## Why it reads flat diff --git a/skill/reference/critique.md b/skill/reference/critique.md index 726fc1657..b1b96a8f4 100644 --- a/skill/reference/critique.md +++ b/skill/reference/critique.md @@ -221,15 +221,13 @@ Skip this step if the Setup slug was null (vague or root-level target). ``` `max_score` is the applicable maximum from the heuristic table (40 when every heuristic applied), so a later run can tell a renormalized total from a full one. The helper prints the absolute path it wrote. -3. **Delete the temp body file** after the write attempt completes, whether the write succeeded or failed. If deletion fails, mention `temp-file cleanup failed: ` briefly in the final output, but do not block the critique. - -4. **Read the trend** for context: +3. **Read the trend** for context: ```bash node {{scripts_path}}/critique-storage.mjs trend "" 5 ``` This returns a JSON array of the last 5 frontmatter entries (including the one you just wrote). -5. **Append a single line to the user-visible output**, after the report and before the questions: +4. **Append a single line to the user-visible output**, after the report and before the questions: > **Trend for `` (last 5 runs): 24 → 28 → 32 → 29 → 32 (out of 40)** > Wrote `.impeccable/critique/`. @@ -238,12 +236,16 @@ Skip this step if the Setup slug was null (vague or root-level target). If this is the first run for the slug, the trend is just one score; say so: "First run for this target, no trend yet." +5. **Send the report and trend line to the user, then delete the temp body file**, whether the write succeeded or failed. The order is deliberate: the deletion is the last thing in the message that carries the report, so the questions below open a fresh one. Do not hold the report back to bundle it with the questions. If deletion fails, mention `temp-file cleanup failed: ` briefly in the final output, but do not block the critique. + This is fire-and-forget. Do not show the user the helper's JSON output; only the human-readable trend line and the written path. Failures here should not block the rest of the flow; print the error and move on. ### Ask the User **After presenting findings**, use targeted questions based on what was actually found. {{ask_instruction}} These answers will shape the action plan. +The report must already be sent before you ask. A structured question blocks the message it rides in until the user answers, so a report bundled with the questions stays invisible until they have answered, and the critique reads as if it never ran. The temp-file cleanup that closed the persistence step ends the report's message; ask once it returns, and put nothing but the questions in that turn. + Ask questions along these lines (adapt to the specific findings; do NOT ask generic questions): 1. **Priority direction**: Based on the issues found, ask which category matters most to the user right now. For example: "I found problems with visual hierarchy, color usage, and information overload. Which area should we tackle first?" Offer the top 2-3 issue categories as options. diff --git a/skill/reference/distill.md b/skill/reference/distill.md index 85c4ea69c..bc0f6e964 100644 --- a/skill/reference/distill.md +++ b/skill/reference/distill.md @@ -21,7 +21,7 @@ Analyze what makes the design feel complex or cluttered: - What can be removed, hidden, or combined? - What's the 20% that delivers 80% of value? -If any of these are unclear from the codebase, {{ask_instruction}} +If any of these are unclear from the codebase, do not guess. {{ask_instruction}} **CRITICAL**: Simplicity is not about removing features. It's about removing obstacles between users and their goals. Every element should justify its existence. diff --git a/skill/reference/document.md b/skill/reference/document.md index ce7f19287..5a7f348b7 100644 --- a/skill/reference/document.md +++ b/skill/reference/document.md @@ -68,7 +68,7 @@ Omit irrelevant sections rather than filling them with invented rules. Put respo - An existing `DESIGN.md` is stale (the design has drifted). - Before a large redesign, to capture the current state as a reference. -If a `DESIGN.md` already exists, **do not silently overwrite it**. Show the user the existing file and {{ask_instruction}} whether to refresh, overwrite, or merge. +If a `DESIGN.md` already exists, **do not silently overwrite it**. Show the user the existing file, then {{ask_instruction}} The choice is refresh, overwrite, or merge. ## Two paths diff --git a/skill/reference/extract.md b/skill/reference/extract.md index 154b9d3aa..0f49cfbaa 100644 --- a/skill/reference/extract.md +++ b/skill/reference/extract.md @@ -6,7 +6,7 @@ Identify reusable patterns, components, and design tokens, then extract and cons Find the design system, component library, or shared UI directory. Understand its structure: component organization, naming conventions, design token structure, import/export conventions. -**CRITICAL**: If no design system exists, {{ask_instruction}} before creating one. Understand the preferred location and structure first. +**CRITICAL**: If no design system exists, do not create one yet. {{ask_instruction}} Understand the preferred location and structure first. ## Step 2: Identify Patterns diff --git a/skill/reference/overdrive.md b/skill/reference/overdrive.md index bc4ffd79f..474100a30 100644 --- a/skill/reference/overdrive.md +++ b/skill/reference/overdrive.md @@ -14,7 +14,7 @@ Push an interface past conventional limits. This isn't just about visual effects This command has the highest potential to misfire. Do NOT jump straight into implementation. You MUST: 1. **Think through 2-3 different directions**: consider different techniques, levels of ambition, and aesthetic approaches. For each direction, briefly describe what the result would look and feel like. -2. **{{ask_instruction}}** to present these directions and get the user's pick before writing any code. Explain trade-offs (browser support, performance cost, complexity). +2. **Get the user's pick before writing any code.** {{ask_instruction}} Carry each direction's description and its trade-offs (browser support, performance cost, complexity) inside the option itself, so the user is choosing between things they can read. A structured question blocks the message it rides in until the user answers, so directions written alongside the question stay invisible while the user is being asked to choose between them. 3. Only proceed with the direction the user confirms. Skipping this step risks building something embarrassing that needs to be thrown away. diff --git a/skill/reference/quieter.md b/skill/reference/quieter.md index 7bba66e76..8483d305d 100644 --- a/skill/reference/quieter.md +++ b/skill/reference/quieter.md @@ -28,7 +28,7 @@ Analyze what makes the design feel too intense: - What's working? (Don't throw away good ideas) - What's the core message? (Preserve what matters) -If any of these are unclear from the codebase, {{ask_instruction}} +If any of these are unclear from the codebase, do not guess. {{ask_instruction}} **CRITICAL**: "Quieter" doesn't mean boring or generic. It means refined and easier on the eyes. Think luxury, not laziness. diff --git a/tests/lib/utils.test.js b/tests/lib/utils.test.js index ed3fb0a87..d7e5d6ed8 100644 --- a/tests/lib/utils.test.js +++ b/tests/lib/utils.test.js @@ -614,7 +614,7 @@ describe('replacePlaceholders', () => { expect(result).toBe('STOP and call the AskUserQuestion tool to clarify.'); const cursorResult = replacePlaceholders('{{ask_instruction}}', 'cursor'); - expect(cursorResult).toBe('ask the user directly to clarify what you cannot infer.'); + expect(cursorResult).toBe('Ask the user directly to clarify what you cannot infer.'); }); test('should replace {{available_commands}} with command list', () => { diff --git a/tests/skill-behavior/README.md b/tests/skill-behavior/README.md index 63f18c2f1..c21d3da1d 100644 --- a/tests/skill-behavior/README.md +++ b/tests/skill-behavior/README.md @@ -63,6 +63,63 @@ an initialized natural build request, replacement-world redesign, and scope-pres refinement. It checks question order and context/artifact writes rather than only reference-file loading. +## Workflow-contract baseline (2026-08-13, current lineup) + +Measured on `claude-sonnet-5`, `gpt-5.6-luna`, `gemini-3.5-flash`, and +`deepseek-v4-flash` while checking whether an `{{ask_instruction}}` rewrite had +regressed anything. Two of the four workflow-contract scenarios fail for reasons +that predate that change. Treat both as the known floor; a regression is a +failure beyond these. + +| Scenario | claude-sonnet-5 | gpt-5.6-luna | gemini-3.5-flash | deepseek-v4-flash | +|---|---|---|---|---| +| attended fresh init | not measured | not measured | not measured | not measured | +| initialized natural build | not measured | not measured | not measured | not measured | +| redesign replaces DESIGN | flaky | not measured | not measured | not measured | +| bolder refinement | not measured | pass | pass | **fail** | + +`not measured` means exactly that: the cell was never run in isolation on this +lineup. Only the two failing scenarios were scoped per model, because the +investigation was about whether a specific edit had regressed them. The rows are +worth keeping anyway, since a scenario absent from the table is easy to mistake +for a scenario that passed. + +**`bolder refinement`, deepseek-v4-flash.** The model runs `context.mjs`, reads +`bolder.md`, `craft-floor.md`, and `current.html`, then ends its turn without +editing anything: empty `writePaths`, no `ask_user_question` call, well short of +the 16-step cap. Confirmed identical on HEAD with `bolder.md` reverted, so it is +not a skill-text problem. Same shape as the gpt-5.4-mini scenario 6/7 failures +below: the model consumes the references and then declines to act. + +**`redesign replaces DESIGN`, flaky.** It has failed on two different assertions +across runs (`designWrite > question` and `implementation > designWrite`), and on +one run claude-sonnet-5 exhausted the 300s per-test timeout instead of asserting. +The traces never load `document.md`; the ordering under test comes from +`new-work.md`. Re-run before believing a single red result here. Which model +produced which failure was not pinned down, so the row records only that the +scenario is unstable. + +The `bolder` claude-sonnet-5 cell is unmeasured for a specific reason: the scoped +run that produced this table used a 180s cap, which sonnet exceeded. That is a +timeout, not a failure, and it is why the guidance below insists on 300000. + +### Scoping a run while investigating + +Both files honor `--test-name-pattern`, which is much cheaper than a full sweep +when bisecting one scenario: + +```bash +IMPECCABLE_QUESTION_DISABLED=1 CI=1 IMPECCABLE_SKILL_BEHAVIOR_MODELS=deepseek-v4-flash \ + node --test --test-timeout=300000 --test-force-exit \ + --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 `serve-question.mjs` cannot open a browser window on the host. 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. + ## Baseline state (2026-05-20, previous cheap tier) > **Historical record.** The default models are now `claude-sonnet-5`, From d4e1b0902fad59fcd52afa4bb5f8798a8c0a6b35 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Thu, 13 Aug 2026 13:24:29 -0400 Subject: [PATCH 2/4] Enforce the ask_instruction sentence-initial contract Review on #576 caught document.md:71 splicing {{ask_instruction}} after "then", which is the same defect this branch set out to fix. Rendered for Codex it produced "Show the user the existing file, then STOP and use Codex's structured user-input/question tool...". The line now starts a new sentence. The comment added to PROVIDER_PLACEHOLDERS asserted the contract without enforcing it, which is exactly how four reference files shipped the splice in the first place. validateAskInstructionSites() in build.js now checks every call site and fails the build on a mid-sentence interpolation, and the comment points at the gate instead of asking authors to remember. Prepared with AI assistance (Claude Code). Co-Authored-By: Claude Opus 5 (1M context) --- scripts/build.js | 57 ++++++++++++++++++++++++++++++++++++- scripts/lib/utils.js | 6 ++-- skill/reference/document.md | 2 +- 3 files changed, 61 insertions(+), 4 deletions(-) diff --git a/scripts/build.js b/scripts/build.js index 94257eb85..5d3e07cac 100644 --- a/scripts/build.js +++ b/scripts/build.js @@ -371,6 +371,57 @@ function validateSkillProse(rootDir) { return errors; } +/** + * Validate that every `{{ask_instruction}}` interpolation starts a sentence. + * + * The placeholder's per-provider values are complete capitalized sentences + * ("STOP and call the AskUserQuestion tool to clarify."), so a call site that + * splices it mid-sentence ships malformed guidance to every provider at once: + * `stop and STOP and call the AskUserQuestion tool to clarify. before expanding + * it`. Four reference files shipped exactly that before this gate existed, and + * a comment in PROVIDER_PLACEHOLDERS asking authors to keep the contract is + * what failed to prevent it. + * + * Returns the number of validation errors. Build fails if > 0. + */ +function validateAskInstructionSites(rootDir) { + const dir = path.join(rootDir, 'skill', 'reference'); + const token = '{{ask_instruction}}'; + let errors = 0; + let sites = 0; + + if (!fs.existsSync(dir)) return 0; + + for (const file of fs.readdirSync(dir)) { + if (path.extname(file) !== '.md') continue; + const rel = path.join('skill/reference', file); + fs.readFileSync(path.join(dir, file), 'utf-8') + .split('\n') + .forEach((line, i) => { + let idx = line.indexOf(token); + while (idx !== -1) { + sites++; + // Bold/italic markers may sit between the punctuation and the token. + const before = line.slice(0, idx).replace(/[*_`]+\s*$/, '').trimEnd(); + if (before !== '' && !/[.!?:]$/.test(before)) { + console.error(` ❌ ${rel}:${i + 1}: ${token} is spliced mid-sentence`); + console.error(` ...${before.slice(-60)} ${token}`); + console.error(` Provider values are full sentences. Start a new one.`); + errors++; + } + idx = line.indexOf(token, idx + 1); + } + }); + } + + if (errors === 0) { + console.log(`✓ ask_instruction call sites: ${sites} sentence-initial`); + } else { + console.error(`\n❌ ${errors} of ${sites} {{ask_instruction}} site(s) spliced mid-sentence.`); + } + return errors; +} + /** * Validate that every hand-authored HTML page carries the shared site header. * The partial is stamped with `` so drift is loud. @@ -738,7 +789,11 @@ async function build() { // that has no technical reading. Hardening repetition is intentionally allowed. const skillProseErrors = validateSkillProse(ROOT_DIR); - if (countErrors > 0 || versionErrors > 0 || manifestShapeErrors > 0 || proseErrors > 0 || skillProseErrors > 0) { + // Placeholder values are full sentences; a mid-sentence splice ships broken + // guidance to every provider at once. + const askSiteErrors = validateAskInstructionSites(ROOT_DIR); + + if (countErrors > 0 || versionErrors > 0 || manifestShapeErrors > 0 || proseErrors > 0 || skillProseErrors > 0 || askSiteErrors > 0) { process.exit(1); } diff --git a/scripts/lib/utils.js b/scripts/lib/utils.js index a3fc3c03d..f74da76f5 100644 --- a/scripts/lib/utils.js +++ b/scripts/lib/utils.js @@ -477,8 +477,10 @@ export const PROVIDER_PLACEHOLDERS = { 'codex': { model: 'GPT', config_file: 'AGENTS.md', - // Every {{ask_instruction}} call site is sentence-initial, so each value is a - // complete capitalized sentence. Keep it that way when adding a provider. + // Each value is a complete capitalized sentence, because every + // {{ask_instruction}} call site is sentence-initial. That is enforced by + // validateAskInstructionSites() in scripts/build.js, not left to authors: + // four reference files had already spliced the placeholder mid-sentence. ask_instruction: "STOP and use Codex's structured user-input/question tool when available; if unavailable, ask directly in chat to clarify what you cannot infer.", command_prefix: '$' }, diff --git a/skill/reference/document.md b/skill/reference/document.md index 5a7f348b7..90b963b1a 100644 --- a/skill/reference/document.md +++ b/skill/reference/document.md @@ -68,7 +68,7 @@ Omit irrelevant sections rather than filling them with invented rules. Put respo - An existing `DESIGN.md` is stale (the design has drifted). - Before a large redesign, to capture the current state as a reference. -If a `DESIGN.md` already exists, **do not silently overwrite it**. Show the user the existing file, then {{ask_instruction}} The choice is refresh, overwrite, or merge. +If a `DESIGN.md` already exists, **do not silently overwrite it**. Show the user the existing file first. {{ask_instruction}} The choice is refresh, overwrite, or merge. ## Two paths From ebc63f071a3fd41dd0bfb3c31a889acdd9768ac2 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Thu, 13 Aug 2026 14:14:44 -0400 Subject: [PATCH 3/4] Fix critique's close on the right mechanism The earlier fix in this branch was built on a wrong diagnosis. It assumed a structured question hides any prose sharing its message, so it split report and question across two turns. A controlled check showed prose before a question renders fine; what hides a report is emitting it AFTER the question. The split therefore fixed nothing and introduced a worse failure: a turn that ends on the report is a turn that ends, and the questions never arrived at all. Persistence returns to main's ordering, byte for byte, and the boundary prose is gone. What replaces it is a position rule: the question is the last thing in the response. The trace test added here found two failures beyond the reported one. Critique can fail to land in three ways, and they are now all asserted: 1. Question emitted before the report, hiding it behind the picker. 2. No close at all: no questions and no skip line, so polish inherits nothing. 3. Report authored into the persistence heredoc and never written to chat, leaving a perfect snapshot and a user who sees nothing. Mode 3 predates this branch entirely. Persistence step 1 now says the temp file is an archive copy, not delivery. The Codex final-question gate is promoted out of its fence, where it was stripped for three of four providers, and the skip branch is now a countable threshold (fewer than 3 Priority Issues) rather than a judgment call. Known floor, recorded in the suite README: gpt-5.6-luna passes 1 run in 6 and deepseek-v4-flash is flaky. claude-sonnet-5 and gemini-3.5-flash are consistent. Prepared with AI assistance (Claude Code). Co-Authored-By: Claude Opus 5 (1M context) --- skill/reference/critique.md | 20 ++-- tests/skill-behavior/README.md | 42 +++++++- .../skill-behavior/workflow-contract.test.mjs | 99 +++++++++++++++++++ 3 files changed, 149 insertions(+), 12 deletions(-) diff --git a/skill/reference/critique.md b/skill/reference/critique.md index b1b96a8f4..4c1928fa7 100644 --- a/skill/reference/critique.md +++ b/skill/reference/critique.md @@ -12,6 +12,8 @@ Resolve one stable target, run two independent assessments, synthesize a design - Viewable targets require browser inspection when available. - Any local server started only for critique visualization must run in the background, have a recorded stop method, and be stopped before final reporting unless the user asks to keep it. - Do not claim a user-visible overlay exists unless script injection succeeded and the detector ran in the page. +- The question is the LAST thing in the response. Write the entire report out first, then ask; nothing follows the question. Prose emitted after a structured question is withheld until the user answers it, so a report written after the question reads as if the critique never ran. +- A run that ends with neither the targeted questions nor a literal `Questions skipped: ` line is an incomplete run. The report is not the finish; the close is. ### Setup @@ -210,6 +212,8 @@ Skip this step if the Setup slug was null (vague or root-level target). 1. **Write the body to a temp file** so you can pipe it to the helper. Use the full critique report (heuristic table, design-specificity verdict, priority issues, persona red flags, minor observations, and questions), but stop before the "Ask the User" / "Recommended Actions" sections that come later. + This file is an archive copy, not delivery. The same report must still be written out in the chat response. Authoring it into a heredoc, storing it, and moving on produces a run with nothing the user can see: the snapshot is for later commands, the chat response is for the person who asked. + Codex: exclude Run Notes from the temp body file; Run Notes are final-chat only because persistence, trend read, and temp cleanup happen after the snapshot write. @@ -221,13 +225,15 @@ Skip this step if the Setup slug was null (vague or root-level target). ``` `max_score` is the applicable maximum from the heuristic table (40 when every heuristic applied), so a later run can tell a renormalized total from a full one. The helper prints the absolute path it wrote. -3. **Read the trend** for context: +3. **Delete the temp body file** after the write attempt completes, whether the write succeeded or failed. If deletion fails, mention `temp-file cleanup failed: ` briefly in the final output, but do not block the critique. + +4. **Read the trend** for context: ```bash node {{scripts_path}}/critique-storage.mjs trend "" 5 ``` This returns a JSON array of the last 5 frontmatter entries (including the one you just wrote). -4. **Append a single line to the user-visible output**, after the report and before the questions: +5. **Append a single line to the user-visible output**, after the report and before the questions: > **Trend for `` (last 5 runs): 24 → 28 → 32 → 29 → 32 (out of 40)** > Wrote `.impeccable/critique/`. @@ -236,15 +242,13 @@ Skip this step if the Setup slug was null (vague or root-level target). If this is the first run for the slug, the trend is just one score; say so: "First run for this target, no trend yet." -5. **Send the report and trend line to the user, then delete the temp body file**, whether the write succeeded or failed. The order is deliberate: the deletion is the last thing in the message that carries the report, so the questions below open a fresh one. Do not hold the report back to bundle it with the questions. If deletion fails, mention `temp-file cleanup failed: ` briefly in the final output, but do not block the critique. - This is fire-and-forget. Do not show the user the helper's JSON output; only the human-readable trend line and the written path. Failures here should not block the rest of the flow; print the error and move on. ### Ask the User **After presenting findings**, use targeted questions based on what was actually found. {{ask_instruction}} These answers will shape the action plan. -The report must already be sent before you ask. A structured question blocks the message it rides in until the user answers, so a report bundled with the questions stays invisible until they have answered, and the critique reads as if it never ran. The temp-file cleanup that closed the persistence step ends the report's message; ask once it returns, and put nothing but the questions in that turn. +Ask in the same message that carries the report, with the report written out first and the question last. Do not split the two across turns: a turn that ends on the report is a turn that ends, and the questions never arrive. Order within the message is what matters, because prose emitted after a structured question is withheld until the user answers. Ask questions along these lines (adapt to the specific findings; do NOT ask generic questions): @@ -260,11 +264,9 @@ Ask questions along these lines (adapt to the specific findings; do NOT ask gene - Every question must reference specific findings from the report. Never ask generic "who is your audience?" questions. - Keep it to 2-4 questions maximum. Respect the user's time. - Offer concrete options, not open-ended prompts. -- If findings are straightforward (e.g., only 1-2 clear issues), skip questions and go directly to Recommended Actions. +- Skipping is allowed only when the report listed **fewer than 3 Priority Issues**. Count them; do not judge the findings "straightforward" by feel. At 3 or more, the questions are required. - -Codex final-question gate: The user-visible response must either include the targeted questions or explicitly say `Questions skipped: ` because the findings were straightforward. Each question must include 2-3 concrete answer options tied to the actual critique findings. Do not end with only open-ended questions. - +**Final-question gate.** The user-visible response must either include the targeted questions or carry the literal line `Questions skipped: ` naming the count that permitted the skip. Each question must include 2-3 concrete answer options tied to the actual critique findings. Do not end with only open-ended questions, and do not end with neither: stopping after the report, having asked nothing and printed no skip line, is the most common way this command fails. ### Recommended Actions diff --git a/tests/skill-behavior/README.md b/tests/skill-behavior/README.md index c21d3da1d..4ab1104b5 100644 --- a/tests/skill-behavior/README.md +++ b/tests/skill-behavior/README.md @@ -59,9 +59,18 @@ The trace is the source of truth, not the model's free-form reply. | 15 | same iOS fixture; prompt is `/impeccable audit` | agent loads `reference/audit.native.md` (the Commands-table native variant, routed instead of `audit.md`) | The workflow-contract file adds end-to-end assertions for attended fresh init, -an initialized natural build request, replacement-world redesign, and scope-preserving bolder -refinement. It checks question order and context/artifact writes rather than -only reference-file loading. +an initialized natural build request, replacement-world redesign, scope-preserving bolder +refinement, and critique's closing question. It checks question order and +context/artifact writes rather than only reference-file loading. + +`critique closes with the question or an explicit skip line` is a regression +guard, not a routing check. A critique that prints its report and then stops, +asking nothing and printing no `Questions skipped: ` line, is an +incomplete run: the close is half the deliverable, and `polish` downstream has +no priorities to inherit without it. The fixture page is deliberately broken +enough to put the report past the three-Priority-Issue threshold, so the run +cannot reach the skip branch on merit. The assertion is deliberately loose about +*how* the run closes, because either close is valid; what it forbids is neither. ## Workflow-contract baseline (2026-08-13, current lineup) @@ -77,6 +86,7 @@ failure beyond these. | initialized natural build | not measured | not measured | not measured | not measured | | redesign replaces DESIGN | flaky | not measured | not measured | not measured | | bolder refinement | not measured | pass | pass | **fail** | +| critique closes | pass (2 of 2) | **fail (1 of 6)** | pass (2 of 2) | flaky (1 of 2) | `not measured` means exactly that: the cell was never run in isolation on this lineup. Only the two failing scenarios were scoped per model, because the @@ -91,6 +101,32 @@ the 16-step cap. Confirmed identical on HEAD with `bolder.md` reverted, so it is not a skill-text problem. Same shape as the gpt-5.4-mini scenario 6/7 failures below: the model consumes the references and then declines to act. +**`critique closes`, gpt-5.6-luna.** Five runs while tuning the instruction text +produced one pass. It fails in three distinct ways, which is why the scenario +asserts on emission order rather than only on the presence of a question: + +1. *No close.* Report lands, no question, no skip line. The failure this + scenario was written for. +2. *Question before report.* The question is emitted first and the report after + it, so the report is withheld until the user answers. Observed directly, and + the reason the invariant is stated as a position rule ("the question is the + LAST thing in the response") rather than as prose order. +3. *Report never spoken.* The full report is authored into the persistence + heredoc, archived, and never written to chat. The snapshot is perfect and the + user sees nothing. This is why persistence step 1 says the temp file is an + archive copy, not delivery. + +claude-sonnet-5 and gemini-3.5-flash pass consistently. deepseek-v4-flash has +hit mode 3 once in two runs, so it is flaky here, not clean. Strengthening the +instruction text moved luna from consistently failing to occasionally passing, +and further prose tuning stopped paying. + +Read the counts in the table as what they are: small samples on a nondeterministic +system, gathered while the instruction text was being changed between runs. They +say the close is reliable on the two strongest models and unreliable on the two +cheapest ones. They do not support a finer claim than that. Re-measure rather +than assuming when the lineup changes. + **`redesign replaces DESIGN`, flaky.** It has failed on two different assertions across runs (`designWrite > question` and `implementation > designWrite`), and on one run claude-sonnet-5 exhausted the 300s per-test timeout instead of asserting. diff --git a/tests/skill-behavior/workflow-contract.test.mjs b/tests/skill-behavior/workflow-contract.test.mjs index 5f83e7fc3..13c04d2b4 100644 --- a/tests/skill-behavior/workflow-contract.test.mjs +++ b/tests/skill-behavior/workflow-contract.test.mjs @@ -37,6 +37,55 @@ body { background: var(--legacy-beige); color: #3c3833; font-family: Arial, sans
Operational since 1987
`; +// Deliberately broken enough that any honest critique lists three or more +// Priority Issues, so the run cannot reach the "fewer than 3" skip branch by +// merit. Low contrast, an icon-tile stack, a kicker over the heading, dead +// hierarchy, and a placeholder CTA. +const FLAWED_PAGE = ` + +
+

INTRODUCING

+

Harbor Desk

+

A platform that helps teams do more of what matters, faster.

+

Lightning Fast

Blazing performance.

+

Rock Solid

Enterprise grade.

+

Fully Secure

Bank level security.

+ +
+`; + +/** + * Flatten assistant output into ordered parts. + * + * `generateText` only returns `text` for the FINAL step, which is empty when a + * turn ends on a tool call. Reading the report out of that field silently tests + * nothing. Walking responseMessages instead preserves emission order, which is + * the point: critique's invariant is that report prose precedes the question + * inside the message, since prose after a structured question is withheld until + * the user answers. + */ +function assistantParts(responseMessages) { + const parts = []; + for (const message of responseMessages) { + if (message.role !== 'assistant') continue; + const content = message.content; + if (typeof content === 'string') { + parts.push({ kind: 'text', value: content }); + continue; + } + for (const part of content ?? []) { + if (part.type === 'text') parts.push({ kind: 'text', value: part.text ?? '' }); + else if (part.type === 'tool-call') parts.push({ kind: 'tool', value: part.toolName ?? '' }); + } + } + return parts; +} + function firstCall(trace, predicate) { return trace.toolCalls.findIndex(predicate); } @@ -164,5 +213,55 @@ for (const modelId of resolveModelList()) { cleanupWorkspace(workspace); } }); + + // Regression guard for the failure mode that shipped in PR #576: the report + // landed and the run then stopped, asking nothing and printing no skip + // line. The close is the deliverable's other half, so a critique that ends + // on the report is incomplete. Asserted on the trace rather than on prose + // because the model's own account of why it skipped is not evidence. + it('critique closes with the question or an explicit skip line', async () => { + const workspace = prepareWorkspace({ + files: { + 'PRODUCT.md': PRODUCT_MD_SAMPLE, + 'DESIGN.md': DESIGN_MD_SAMPLE, + 'current.html': FLAWED_PAGE, + }, + }); + try { + const { trace, responseMessages } = await runTurn({ + workspace, + model, + userPrompt: '/impeccable critique current.html', + maxSteps: 30, + }); + assert.ok(fileLoaded(trace, 'critique.md'), `critique.md was not loaded.\n${workflowTraceMessage(trace)}`); + + const parts = assistantParts(responseMessages); + const allText = parts.filter((p) => p.kind === 'text').map((p) => p.value).join('\n'); + const reportPattern = /priority issue|heuristic|design health/i; + assert.match(allText, reportPattern, `no report reached the user.\n${workflowTraceMessage(trace)}`); + + const askIndex = parts.findIndex((p) => p.kind === 'tool' && p.value === 'ask_user_question'); + const skipped = /Questions skipped:/i.test(allText); + assert.ok( + askIndex >= 0 || skipped, + `critique ended without the questions and without a "Questions skipped: " line.\n` + + `This is the PR #576 regression: the report is not the finish, the close is.\n${workflowTraceMessage(trace)}`, + ); + + // The ordering invariant. Only meaningful when a question was actually + // asked; a skip-line close has nothing to order against. + if (askIndex >= 0) { + const reportIndex = parts.findIndex((p) => p.kind === 'text' && reportPattern.test(p.value)); + assert.ok( + reportIndex >= 0 && reportIndex < askIndex, + `the question was emitted before the report text, so the report stays hidden until the user answers.\n` + + `${workflowTraceMessage(trace)}`, + ); + } + } finally { + cleanupWorkspace(workspace); + } + }); }); } From 121602079c2f0d33e0961c286dae680516606ab9 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Thu, 13 Aug 2026 17:03:19 -0400 Subject: [PATCH 4/4] Deliver the report as its own step; retune the lineup Two failures the trace test found were structural, not model quirks. critique.md described the report's format and then went straight to writing a temp file, with no step saying to output the report. gemini-3.6-flash and luna both responded by bundling heredoc, snapshot write, trend read, and cleanup into one bash call and stopping, leaving a perfect archive nobody had read. A "Deliver the Report" step now precedes persistence, and persistence describes itself as a copy of what was already sent. gemini-3.6-flash failed three consecutive runs before this and its failures afterward all show the report reaching chat. The close is also step 6 of the persistence list rather than a section after it, since the same shape is what fixed delivery. Lineup: gpt-5.6-luna and deepseek-v4-flash out, gpt-5.6-terra in at reasoningEffort high (IMPECCABLE_SKILL_BEHAVIOR_EFFORT overrides), gemini 3.5 to 3.6. Provider options resolve from the model object inside the harness so the 21 runTurn call sites are untouched. Verified the effort actually reaches the API rather than being silently dropped. The Gemini bump was not cosmetic: 3.5-flash passed critique closes twice and 3.6-flash then failed three times against identical text. A version bump inside one family changed the outcome, so the README now treats cross-version carryover as unmeasured. Known floor, recorded: critique closes is flaky on gemini-3.6-flash, 1 run in 3. Two structural attempts moved it from consistently failing to intermittently passing and then stopped paying. claude-sonnet-5 and gpt-5.6-terra are clean. Prepared with AI assistance (Claude Code). Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 2 +- skill/reference/critique.md | 12 +++- tests/skill-behavior/README.md | 100 ++++++++++++++++++----------- tests/skill-behavior/harness.mjs | 5 ++ tests/skill-behavior/providers.mjs | 44 +++++++++++-- 5 files changed, 120 insertions(+), 43 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 993e0e133..dc3dae89a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -198,7 +198,7 @@ IMPECCABLE_SKILL_BEHAVIOR_MODELS=gemini-3.5-flash bun run test:skill-behavior IMPECCABLE_SKILL_BEHAVIOR_VERBOSE=1 bun run test:skill-behavior # dump per-scenario trace JSON to stderr (use when iterating) ``` -**Every provider, every run.** The lineup is `DEFAULT_MODELS` in `tests/skill-behavior/providers.mjs`, currently `claude-sonnet-5`, `gpt-5.6-luna`, `gemini-3.5-flash`, and `deepseek-v4-flash`. **Don't substitute Claude alone**: many of the most useful findings come from divergence between providers. +**Frontier tiers, more than one family.** The lineup is `DEFAULT_MODELS` in `tests/skill-behavior/providers.mjs`, currently `claude-sonnet-5` and `gemini-3.6-flash`. `gpt-5.6-luna` and `deepseek-v4-flash` were dropped in 2026-08: below the frontier tier they fail scenarios for model-floor reasons rather than skill-text defects, and a suite that is always red is a suite nobody reads. **Don't substitute Claude alone**: many of the most useful findings come from divergence between families, so keep at least two. The dropped models stay selectable via `IMPECCABLE_SKILL_BEHAVIOR_MODELS` when a Setup or routing change warrants a wider sweep. **Auth** lives in repo-root `.env` (copied from `~/code/impeccable-evals/.env`, gitignored). Providers skip cleanly when their key is unset; they don't fail. diff --git a/skill/reference/critique.md b/skill/reference/critique.md index 4c1928fa7..f96cbbfe7 100644 --- a/skill/reference/critique.md +++ b/skill/reference/critique.md @@ -204,6 +204,14 @@ Codex Run Notes are final-chat only. Do not include this section in the persiste - Prioritize ruthlessly. If everything is important, nothing is. - Don't soften criticism. Developers need honest feedback to ship great design. +### Deliver the Report + +Write the full report into the chat response now, before any persistence work. This is the deliverable; everything below it is bookkeeping. + +Do this first because the alternative is the most common way this command fails: the report gets composed once, straight into the persistence heredoc, and the run ends with a perfect archive nobody has read. Composing it into a file is not delivering it. If the report exists only in `.impeccable/critique/`, the run produced nothing. + +Persistence is not the end of the run. After it, the response continues with the trend line and the close. + ### Persist the Snapshot Once the report above is finalized, write it to `.impeccable/critique/` so the user can refer back, and so `{{command_prefix}}impeccable polish` can pick up the priority issues without a copy-paste. @@ -212,7 +220,7 @@ Skip this step if the Setup slug was null (vague or root-level target). 1. **Write the body to a temp file** so you can pipe it to the helper. Use the full critique report (heuristic table, design-specificity verdict, priority issues, persona red flags, minor observations, and questions), but stop before the "Ask the User" / "Recommended Actions" sections that come later. - This file is an archive copy, not delivery. The same report must still be written out in the chat response. Authoring it into a heredoc, storing it, and moving on produces a run with nothing the user can see: the snapshot is for later commands, the chat response is for the person who asked. + This is a copy of the report you already delivered above, for later commands to read. It is not delivery. If you find yourself composing the report for the first time inside this heredoc, you have skipped Deliver the Report; go back and send it. Codex: exclude Run Notes from the temp body file; Run Notes are final-chat only because persistence, trend read, and temp cleanup happen after the snapshot write. @@ -242,6 +250,8 @@ Skip this step if the Setup slug was null (vague or root-level target). If this is the first run for the slug, the trend is just one score; say so: "First run for this target, no trend yet." +6. **Close the run.** Go to Ask the User below and emit the questions, or the `Questions skipped: ` line when the count allows it. The run is not complete until you do. Persistence is bookkeeping and cleanup is not an ending; stopping here leaves the user with a report and no way forward, and leaves `{{command_prefix}}impeccable polish` with no priorities to inherit. + This is fire-and-forget. Do not show the user the helper's JSON output; only the human-readable trend line and the written path. Failures here should not block the rest of the flow; print the error and move on. ### Ask the User diff --git a/tests/skill-behavior/README.md b/tests/skill-behavior/README.md index 4ab1104b5..1e904baab 100644 --- a/tests/skill-behavior/README.md +++ b/tests/skill-behavior/README.md @@ -15,6 +15,7 @@ contract. bun run test:skill-behavior IMPECCABLE_SKILL_BEHAVIOR_VERBOSE=1 bun run test:skill-behavior # dump per-scenario traces IMPECCABLE_SKILL_BEHAVIOR_MODELS=claude-sonnet-5 bun run test:skill-behavior # scope to one model +IMPECCABLE_SKILL_BEHAVIOR_EFFORT=xhigh bun run test:skill-behavior # OpenAI reasoning effort (default: high) ``` Requires `.env` at repo root with at least one of `ANTHROPIC_API_KEY`, @@ -74,25 +75,40 @@ cannot reach the skip branch on merit. The assertion is deliberately loose about ## Workflow-contract baseline (2026-08-13, current lineup) -Measured on `claude-sonnet-5`, `gpt-5.6-luna`, `gemini-3.5-flash`, and -`deepseek-v4-flash` while checking whether an `{{ask_instruction}}` rewrite had -regressed anything. Two of the four workflow-contract scenarios fail for reasons -that predate that change. Treat both as the known floor; a regression is a -failure beyond these. +Measured while checking whether an `{{ask_instruction}}` rewrite had regressed +anything. -| Scenario | claude-sonnet-5 | gpt-5.6-luna | gemini-3.5-flash | deepseek-v4-flash | +**The last two columns are no longer in the default lineup.** `gpt-5.6-luna` and +`deepseek-v4-flash` were dropped in 2026-08 for being below the frontier tier: +they fail scenarios by stopping mid-run or archiving a report without stating +it, which is model-floor behavior rather than a skill-text defect. Their columns +stay here because they are the record of what a weaker model does with this text, +and that is the useful part. Reproduce with +`IMPECCABLE_SKILL_BEHAVIOR_MODELS=gpt-5.6-luna,deepseek-v4-flash`. + +Against the current default lineup, two cells are the known floor: +`redesign replaces DESIGN` is flaky, and `critique closes` is flaky on +gemini-3.6-flash. A regression is a failure beyond those two. + +| Scenario | claude-sonnet-5 | gpt-5.6-terra | gemini-3.6-flash | luna / deepseek (dropped) | |---|---|---|---|---| | attended fresh init | not measured | not measured | not measured | not measured | | initialized natural build | not measured | not measured | not measured | not measured | | redesign replaces DESIGN | flaky | not measured | not measured | not measured | -| bolder refinement | not measured | pass | pass | **fail** | -| critique closes | pass (2 of 2) | **fail (1 of 6)** | pass (2 of 2) | flaky (1 of 2) | +| bolder refinement | not measured | not measured | pass (on 3.5) | luna pass, deepseek **fail** | +| critique closes | pass (2 of 2) | pass (2 of 2) | **flaky (1 of 3)** | luna **fail (1 of 6)**, deepseek flaky | + +Gemini cells marked `on 3.5` were measured on the superseded `gemini-3.5-flash` +and have not been re-run on 3.6. That distinction is not pedantic. `critique +closes` passed twice on 3.5-flash, then failed three times in a row on 3.6-flash +against identical instruction text, and only passed once the report delivery step +was made explicit. A version bump inside one family changed the outcome, so treat +cross-version carryover as unmeasured rather than inherited. `not measured` means exactly that: the cell was never run in isolation on this -lineup. Only the two failing scenarios were scoped per model, because the -investigation was about whether a specific edit had regressed them. The rows are -worth keeping anyway, since a scenario absent from the table is easy to mistake -for a scenario that passed. +lineup. Only the scenarios under investigation were scoped per model. The rows +are worth keeping anyway, since a scenario absent from the table is easy to +mistake for a scenario that passed. **`bolder refinement`, deepseek-v4-flash.** The model runs `context.mjs`, reads `bolder.md`, `craft-floor.md`, and `current.html`, then ends its turn without @@ -101,31 +117,44 @@ the 16-step cap. Confirmed identical on HEAD with `bolder.md` reverted, so it is not a skill-text problem. Same shape as the gpt-5.4-mini scenario 6/7 failures below: the model consumes the references and then declines to act. -**`critique closes`, gpt-5.6-luna.** Five runs while tuning the instruction text -produced one pass. It fails in three distinct ways, which is why the scenario -asserts on emission order rather than only on the presence of a question: +**`critique closes`: the three ways a critique fails to land.** The scenario +asserts emission order, not just the presence of a question, because the command +fails in three distinct ways and only one of them was the reported bug: -1. *No close.* Report lands, no question, no skip line. The failure this - scenario was written for. +1. *No close.* Report lands, no question, no skip line. `polish` downstream + inherits nothing. 2. *Question before report.* The question is emitted first and the report after - it, so the report is withheld until the user answers. Observed directly, and - the reason the invariant is stated as a position rule ("the question is the - LAST thing in the response") rather than as prose order. -3. *Report never spoken.* The full report is authored into the persistence - heredoc, archived, and never written to chat. The snapshot is perfect and the - user sees nothing. This is why persistence step 1 says the temp file is an - archive copy, not delivery. + it, so the report is withheld until the user answers. Observed directly on + gpt-5.6-luna, and the reason the invariant is a position rule ("the question + is the LAST thing in the response") rather than a statement about prose order. +3. *Report never spoken.* The report is authored straight into the persistence + heredoc, archived, and never written to chat. A perfect snapshot and a user + who sees nothing. -claude-sonnet-5 and gemini-3.5-flash pass consistently. deepseek-v4-flash has -hit mode 3 once in two runs, so it is flaky here, not clean. Strengthening the -instruction text moved luna from consistently failing to occasionally passing, -and further prose tuning stopped paying. +Mode 3 is the one worth understanding, because it was structural rather than a +model quirk. `critique.md` described the report's format and then went directly +to writing a temp file, with no step that said to output the report. Both +gemini-3.6-flash and luna responded by bundling heredoc, snapshot write, trend +read, and cleanup into a single bash call and stopping. The `Deliver the Report` +section exists to close that gap, and it worked: gemini-3.6-flash failed three +consecutive runs before it, and its failures afterward all show the report +reaching chat. -Read the counts in the table as what they are: small samples on a nondeterministic -system, gathered while the instruction text was being changed between runs. They -say the close is reliable on the two strongest models and unreliable on the two -cheapest ones. They do not support a finer claim than that. Re-measure rather -than assuming when the lineup changes. +**Mode 1 is not fixed on gemini-3.6-flash.** It passes 1 run in 3 on the final +text. Two structural attempts were made and neither settled it: the close was +promoted into Hard Invariants with a printable `Questions skipped: ` +string, then made step 6 of the persistence list so it would sit inside the +numbered flow rather than after it (the shape that fixed mode 3). Both moved it +from consistently failing to intermittently passing, and further prose tuning +was not paying, so it stopped. claude-sonnet-5 and gpt-5.6-terra are clean. +Treat this cell as the known floor and re-measure rather than tuning blindly: +the next useful move is probably a structural one, such as making the close +something the run cannot syntactically finish without, not another paragraph. + +Read the counts here as what they are: small samples on a nondeterministic +system, several of them gathered while the instruction text was still changing +between runs. They support "the close works on the current lineup" and not much +finer than that. Re-measure rather than assuming when the lineup changes. **`redesign replaces DESIGN`, flaky.** It has failed on two different assertions across runs (`designWrite > question` and `implementation > designWrite`), and on @@ -158,9 +187,8 @@ and truncating it costs you the per-model attribution. ## Baseline state (2026-05-20, previous cheap tier) -> **Historical record.** The default models are now `claude-sonnet-5`, -> `gpt-5.6-luna`, `gemini-3.5-flash`, and `deepseek-v4-flash`. The table below -> was measured on an older cheap tier +> **Historical record.** The default models are now `claude-sonnet-5` and +> `gemini-3.6-flash`. The table below was measured on an older cheap tier > (`claude-haiku-4-5` / `gpt-5.4-mini`) and is kept as the historical record. > Re-measure on the current lineup and update this section; the stronger > models are expected to clear the scenario 6/7 routing failures that the old diff --git a/tests/skill-behavior/harness.mjs b/tests/skill-behavior/harness.mjs index 94a8f1871..e9f4187b1 100644 --- a/tests/skill-behavior/harness.mjs +++ b/tests/skill-behavior/harness.mjs @@ -26,6 +26,7 @@ import os from 'node:os'; import path from 'node:path'; import { spawn } from 'node:child_process'; import { fileURLToPath } from 'node:url'; +import { getProviderOptions } from './providers.mjs'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); const REPO_ROOT = path.resolve(__dirname, '..', '..'); @@ -348,6 +349,10 @@ export async function runTurn({ workspace, model, userPrompt, priorMessages = [] messages, tools, stopWhen: [stepCountIs(maxSteps)], + // 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. + providerOptions: getProviderOptions(model?.modelId ?? ''), }); } catch (err) { throw new Error(`LLM behavior turn failed before completing: ${String(err)}`, { cause: err }); diff --git a/tests/skill-behavior/providers.mjs b/tests/skill-behavior/providers.mjs index 97be64c68..d6bf0dc8b 100644 --- a/tests/skill-behavior/providers.mjs +++ b/tests/skill-behavior/providers.mjs @@ -88,12 +88,46 @@ export function getModel(modelId) { } /** - * Default model lineup. These are current models, but intentionally the - * economical members of each family: this test is about routing/loading - * behavior, not design-output quality. - * Override with IMPECCABLE_SKILL_BEHAVIOR_MODELS=claude-foo,gpt-bar. + * Per-model provider options, merged into generateText by the harness. + * + * gpt-5.6-terra is a reasoning model, and at the provider's default effort it + * is not the tier this suite is meant to measure. Setup and routing behavior is + * exactly the kind of multi-step instruction-following that reasoning effort + * moves, so pin it high rather than inherit whatever the default happens to be. + * Override with IMPECCABLE_SKILL_BEHAVIOR_EFFORT=xhigh. */ -export const DEFAULT_MODELS = ['claude-sonnet-5', 'gpt-5.6-luna', 'gemini-3.5-flash', 'deepseek-v4-flash']; +export function getProviderOptions(modelId) { + let provider; + try { + provider = detectProvider(modelId); + } catch { + // Resolved from a live model object rather than the lineup, so an id this + // module does not recognize is not an error; it just gets no options. + return undefined; + } + if (provider === 'openai') { + const effort = process.env.IMPECCABLE_SKILL_BEHAVIOR_EFFORT || 'high'; + return { openai: { reasoningEffort: effort } }; + } + return undefined; +} + +/** + * Default model lineup. Frontier tiers only. + * + * gpt-5.6-luna and deepseek-v4-flash were dropped in 2026-08: below the + * frontier tier, they fail scenarios for reasons that are model-floor behavior + * rather than skill-text defects (stopping mid-run, archiving a report without + * ever stating it), and a permanently red suite teaches everyone to ignore it. + * + * They stay selectable, and running a wider sweep deliberately is still worth + * doing when Setup or routing text changes in a way that could go wrong in an + * unfamiliar direction. Divergence between families is what surfaces the + * non-obvious failures; the cheap tier just could not tell divergence from + * its own floor: + * IMPECCABLE_SKILL_BEHAVIOR_MODELS=gpt-5.6-luna,deepseek-v4-flash + */ +export const DEFAULT_MODELS = ['claude-sonnet-5', 'gpt-5.6-terra', 'gemini-3.6-flash']; export function resolveModelList() { const override = process.env.IMPECCABLE_SKILL_BEHAVIOR_MODELS;