skills/pbakaus_impeccable
1
0
Fork 0
mirror of https://github.com/pbakaus/impeccable.git synced 2026-09-11 21:57:14 +03:00
Code Issues Packages Projects Releases Wiki Activity

Fix critique's close on the right mechanism

Browse Source 
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 <codex> 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) <noreply@anthropic.com>
This commit is contained in:
Paul Bakaus
2026-08-13 14:14:44 -04:00
co-authored by Claude Opus 5
parent d4e1b0902f
commit ebc63f071a
3 changed files with 149 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files
skill/reference/critique.md
+11 -9
View File
@@ -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
@@ -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>
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>
@@ -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: <reason>` 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 "<resolved target>" 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 `<slug>` (last 5 runs): 24 → 28 → 32 → 29 → 32 (out of 40)**
> Wrote `.impeccable/critique/<filename>`.
@@ -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: <reason>` 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>
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
tests/skill-behavior/README.md
+39 -3
View File
@@ -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: <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)
@@ -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.
tests/skill-behavior/workflow-contract.test.mjs
+99
View File
@@ -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);
}
});
});
}