mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-14 15:16:35 +03:00
Merge pull request #576 from pbakaus/fix/ask-instruction-message-boundary
Make critique's report and close actually land
This commit is contained in:
Paul Bakaus
@@ -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.
|
||||
|
||||
|
||||
+56
-1
@@ -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 `<!-- site-header v1 -->` 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);
|
||||
}
|
||||
|
||||
|
||||
+15
-11
@@ -465,31 +465,35 @@ 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',
|
||||
// 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: '$'
|
||||
},
|
||||
'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 +505,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 +541,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 +550,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: '/'
|
||||
}
|
||||
};
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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: <reason>` line is an incomplete run. The report is not the finish; the close is.
|
||||
|
||||
### Setup
|
||||
|
||||
@@ -202,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.
|
||||
@@ -210,6 +220,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 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>
|
||||
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.
|
||||
</codex>
|
||||
@@ -238,12 +250,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."
|
||||
|
||||
6. **Close the run.** Go to Ask the User below and emit the questions, or the `Questions skipped: <reason>` 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
|
||||
|
||||
**After presenting findings**, use targeted questions based on what was actually found. {{ask_instruction}} These answers will shape the action plan.
|
||||
|
||||
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):
|
||||
|
||||
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.
|
||||
@@ -258,11 +274,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>
|
||||
Codex final-question gate: The user-visible response must either include the targeted questions or explicitly say `Questions skipped: <reason>` 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.
|
||||
</codex>
|
||||
**Final-question gate.** The user-visible response must either include the targeted questions or carry the literal line `Questions skipped: <reason>` 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
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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 first. {{ask_instruction}} The choice is refresh, overwrite, or merge.
|
||||
|
||||
## Two paths
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
@@ -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`,
|
||||
@@ -59,15 +60,135 @@ 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: <reason>` 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)
|
||||
|
||||
Measured while checking whether an `{{ask_instruction}}` rewrite had regressed
|
||||
anything.
|
||||
|
||||
**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 | 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 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
|
||||
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.
|
||||
|
||||
**`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. `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 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.
|
||||
|
||||
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.
|
||||
|
||||
**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: <reason>`
|
||||
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
|
||||
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`,
|
||||
> `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
|
||||
|
||||
@@ -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 });
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -37,6 +37,55 @@ body { background: var(--legacy-beige); color: #3c3833; font-family: Arial, sans
|
||||
<footer data-untouched="footer">Operational since 1987</footer>
|
||||
</body></html>`;
|
||||
|
||||
// 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 = `<!doctype html>
|
||||
<html><head><style>
|
||||
body { background:#f4f4f5; color:#b9b9c0; font-family: Arial, sans-serif; font-size:15px; }
|
||||
h1, h2, h3, p { font-size:15px; font-weight:400; margin:8px 0; }
|
||||
.tile { width:48px; height:48px; background:#e6e6ea; border-radius:12px; }
|
||||
.card { border:1px solid #e6e6ea; border-radius:12px; padding:16px; }
|
||||
</style></head><body>
|
||||
<main>
|
||||
<p class="kicker">INTRODUCING</p>
|
||||
<h1>Harbor Desk</h1>
|
||||
<p>A platform that helps teams do more of what matters, faster.</p>
|
||||
<section class="card"><div class="tile"></div><h3>Lightning Fast</h3><p>Blazing performance.</p></section>
|
||||
<section class="card"><div class="tile"></div><h3>Rock Solid</h3><p>Enterprise grade.</p></section>
|
||||
<section class="card"><div class="tile"></div><h3>Fully Secure</h3><p>Bank level security.</p></section>
|
||||
<button style="background:#e6e6ea;color:#c9c9d0;border:none;padding:8px 12px">Learn More</button>
|
||||
</main>
|
||||
</body></html>`;
|
||||
|
||||
/**
|
||||
* 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: <reason>" 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);
|
||||
}
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user