Critique persistence: per-run snapshots, ignore list, polish reads as signal (#153)

* critique-storage: new helper for per-run snapshot persistence Adds skill/scripts/critique-storage.mjs with: - slugFromTarget(): mechanically derive a stable slug from a resolved file path or URL (NOT from the user's natural-language phrasing), so the same target lands in the same stream across runs even when dev-server ports drift or the user phrases it differently. - writeSnapshot(): writes .impeccable/critique/<timestamp>__<slug>.md with a small YAML frontmatter (timestamp, slug, target, total_score, p0_count, p1_count) plus the report body. - readLatestSnapshot(): newest snapshot for a slug, used by polish. - readTrend(): last N frontmatter entries for a slug, used by critique to print the score trend line. - readIgnoreList(): non-empty non-comment lines from ignore.md, the ONLY input critique consumes from prior runs. No separate index.json. The snapshot files are the single source of truth; trend reader globs them and parses frontmatter. Deleting a snapshot removes it from the trend cleanly with no orphan rows. CRITIQUE_DIR constant + getCritiqueDir / getCritiqueIgnorePath added to impeccable-paths.mjs alongside the existing live-dir helpers. 19 unit tests in tests/critique-storage.test.mjs cover slug stability, URL and file inputs, round-trip read/write, trend filtering by slug, and ignore-list parsing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * critique: persist snapshot per run, respect ignore.md Two new steps wired into the critique flow: - Setup: Resolve Target and Load Ignore List. Before gathering assessments, resolve the user's natural-language target ("the homepage") to a concrete artifact, compute the slug via critique-storage.mjs, and read ignore.md. Matching findings drop silently from the report. This is the only prior-run input critique consumes; anchoring on prior findings would defeat independent assessment. - Persist the Snapshot. After the report is finalized (before Ask the User), write it to .impeccable/critique/<ts>__<slug>.md with structured frontmatter, then surface a one-line trend ("Trend for index-astro: 24 → 28 → 32") and the written path. First run says "no trend yet". Persistence is fire-and-forget; failures print and move on rather than blocking the rest of the flow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * polish: read latest matching critique as fix backlog When polish is invoked after critique on the same target, the critique's P0/P1 findings are the right backlog; don't re-derive them. Adds a Setup step that resolves the target, computes the slug via critique-storage.mjs slug, and reads the latest matching snapshot via critique-storage.mjs latest. Found → use those P0/P1 items as the polish backlog and mention the snapshot path. Not found → proceed independently from a clean slate. Explicitly does NOT read snapshots for other targets (cross-target context is pollution). Explicitly does NOT cascade to atomic moves (bolder, quieter, clarify, animate, etc.); those act on a specific selection where the page-level critique would be noise. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * gitignore: .impeccable/critique/, opt ignore.md back in Per-run critique snapshots are local artifacts (same precedent as .impeccable/live/sessions/), but ignore.md carries user-curated deferrals that may be worth sharing across a team. Negate-pattern keeps it trackable while the snapshot files stay local. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * polish: reframe prior critique as additional signal, not backlog Three corrections to the previous polish.md addition: - "Polish is usually invoked after critique" is wrong; people polish without ever running critique. Dropped the presumption. - "This is the only command that auto-reads prior critique" leaks cross-command scope into polish's reference file. Dropped. - Treating critique findings as THE polish backlog biased polish to only fix what critique flagged, skipping its own checklist. The critique is one input among many; fold its P0/P1 items into the polish list, then do the normal pass. Now lives as a short item 4 in Pre-Polish Assessment ("Pull in any prior critique — optional signal") instead of a top-level Setup section. Less prominent, doesn't presume invocation order. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * critique-storage: drop the ignore subcommand, read ignore.md directly The ignore-list helper did nothing the model can't do inline: read a markdown file, skip blank and #-prefix lines. It added a tool roundtrip for no real value. Other helpers earn their keep by doing work the model can't trivially do (path normalization, filename generation, glob + frontmatter parsing); ignore-list did not. Removed: - `ignore` CLI subcommand - readIgnoreList() module export + its tests - getCritiqueIgnorePath() from impeccable-paths.mjs (now dead code) Critique.md step 3 now just says "read .impeccable/critique/ignore.md if it exists" and explains the format inline. Simpler. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * critique-storage: caller meta cannot override timestamp or slug Spotted by Cursor Bugbot on the PR. writeSnapshot built frontmatter as { timestamp, slug, ...meta } so a caller-supplied meta blob (parsed from the IMPECCABLE_CRITIQUE_META env var) could silently clobber the computed timestamp and slug. The filename keeps the computed values, so the frontmatter would drift from the filename and readTrend would attribute scores to wrong timestamps with no visible error. Swap to { ...meta, timestamp, slug } so internal values always win. Add a regression test that passes corrupt meta and asserts the frontmatter still matches the filename. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-09-12 06:06:37 +03:00 · 2026-05-13 00:06:02 -07:00
parent c32daaf3b0
commit 93a13f9882
58 changed files with 4196 additions and 14 deletions
@@ -1,5 +1,23 @@
 > **Additional context needed**: what the interface is trying to accomplish.

+### Setup: Resolve Target and Load Ignore List
+
+Before gathering assessments, do two small bookkeeping steps. They cost almost nothing and they're what makes critique iterative across runs.
+
+1. **Resolve the primary artifact.** The user's phrasing ("the homepage", "the pricing flow") is not stable enough to track across runs. Resolve it to a concrete file path or URL: the same one you'd already need to scan code or open in a browser. Examples:
+   - "the homepage" → `site/pages/index.astro` (or `http://localhost:3000/` if you're inspecting live)
+   - "the settings modal" → the primary component file (e.g., `src/components/Settings.tsx`)
+   - "this page" → the URL or the page's source file
+   Prefer the source file path over the dev-server URL when both exist; ports drift between runs (`bun dev` vs `bun preview`), file paths don't.
+
+2. **Compute the slug.** Run:
+   ```bash
+   node .github/skills/impeccable/scripts/critique-storage.mjs slug "<resolved-path-or-url>"
+   ```
+   Keep the printed slug. It identifies this target's stream across runs. If the command exits non-zero ("no stable slug for input"), skip persistence for this run and tell the user; the trend won't update but the critique still goes ahead.
+
+3. **Read the ignore list** at `.impeccable/critique/ignore.md` if it exists. Plain markdown; each non-empty, non-comment line is something the user has marked as "do not re-raise" (deferred tradeoffs, designer-intended deviations, detector false-positives the user accepts). When a finding's text matches a line here (case-insensitive substring against rule name or snippet), **drop it silently**. Do not mention it in the report. This is the ONLY input critique consumes from prior runs; anchoring on prior findings would defeat the point of independent assessment.
+
 ### Gather Assessments

 Launch two independent assessments. **Neither may see the other's output.** This isolation is what makes the combined score honest. Running both in one head silently anchors them to each other; do not shortcut it for cost, speed, or context-size reasons.
@@ -164,6 +182,36 @@ Provocative questions that might unlock better solutions:
 - Prioritize ruthlessly. If everything is important, nothing is.
 - Don't soften criticism. Developers need honest feedback to ship great design.

+### Persist the Snapshot
+
+Once the report above is finalized, write it to `.impeccable/critique/` so the user can refer back, and so `/impeccable polish` can pick up the priority issues without a copy-paste.
+
+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 report (heuristic table, anti-patterns verdict, priority issues, persona red flags) but stop before the "Ask the User" / "Recommended Actions" sections that come later.
+
+2. **Pass the structured metadata** through `IMPECCABLE_CRITIQUE_META` (JSON), then run the write command:
+   ```bash
+   IMPECCABLE_CRITIQUE_META='{"target":"<user phrasing>","total_score":<n>,"p0_count":<n>,"p1_count":<n>}' \
+     node .github/skills/impeccable/scripts/critique-storage.mjs write <slug> <body-file>
+   ```
+   The helper prints the absolute path it wrote.
+
+3. **Read the trend** for context:
+   ```bash
+   node .github/skills/impeccable/scripts/critique-storage.mjs trend <slug> 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:
+
+   > **Trend for `<slug>` (last 5 runs): 24 → 28 → 32 → 29 → 32**
+   > Wrote `.impeccable/critique/<filename>`.
+
+   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."
+
+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 the user directly to clarify what you cannot infer. These answers will shape the action plan.
@@ -35,7 +35,14 @@ Understand the current state and goals before touching anything:
   - Loading and transition smoothness
   - Information architecture and flow drift (does this feature reveal complexity the way neighboring features do?)

-4. **Triage cosmetic vs functional**: Classify each issue as **cosmetic** (looks off, doesn't impede the user) or **functional** (breaks, blocks, or confuses the experience). When polish time is tight, functional issues ship first; cosmetic ones can land in a follow-up. Quality should be consistent; never perfect one corner while leaving another rough.
+4. **Pull in any prior critique** (optional signal): If `/impeccable critique` has been run on the same target, its priority issues are a useful prior for what to address first. Resolve the target to a file path or URL, then:
+   ```bash
+   slug=$(node .github/skills/impeccable/scripts/critique-storage.mjs slug "<resolved>")
+   node .github/skills/impeccable/scripts/critique-storage.mjs latest "$slug"
+   ```
+   Exit 0 with body = found; fold the P0/P1 items into your polish list and mention the snapshot path so the user sees what you read. Exit 2 = no snapshot, continue without it. The critique is one input among many. Do your own pass either way.
+
+5. **Triage cosmetic vs functional**: Classify each issue as **cosmetic** (looks off, doesn't impede the user) or **functional** (breaks, blocks, or confuses the experience). When polish time is tight, functional issues ship first; cosmetic ones can land in a follow-up. Quality should be consistent; never perfect one corner while leaving another rough.

 **CRITICAL**: Polish is the last step, not the first. Don't polish work that's not functionally complete.

@@ -0,0 +1,226 @@
+#!/usr/bin/env node
+/**
+ * Critique persistence helper.
+ *
+ * Each run of /impeccable critique writes a per-target snapshot to
+ *   .impeccable/critique/<timestamp>__<slug>.md
+ * with a small YAML frontmatter carrying the score + P0/P1 counts.
+ *
+ * /impeccable polish reads the latest matching snapshot at start as its
+ * fix backlog. No other skill auto-reads critique output.
+ *
+ * The slug is derived mechanically from the *resolved* primary artifact
+ * (file path or URL), never from the user's natural-language phrasing.
+ * Slug stability across runs is what lets the trend display work.
+ *
+ * CLI entry points (called from skill instructions):
+ *   node critique-storage.mjs slug <resolved-target>
+ *   node critique-storage.mjs write <slug> <snapshot-body-file>
+ *   node critique-storage.mjs latest <slug>
+ *   node critique-storage.mjs trend <slug> [limit]
+ *
+ * Note: there is intentionally no `ignore` subcommand. ignore.md is a plain
+ * markdown file; the model reads it directly with its file-read tool. This
+ * helper only exists for operations the model can't trivially do inline
+ * (normalizing paths, generating filenames, globbing + parsing frontmatter).
+ */
+
+import fs from 'node:fs';
+import path from 'node:path';
+import { getCritiqueDir } from './impeccable-paths.mjs';
+
+const SLUG_MAX = 50;
+
+/**
+ * Mechanically derive a slug from a resolved target. Returns null if the
+ * input doesn't look like a stable identifier (empty, project root, etc).
+ *
+ * Accepts file paths and URLs. The model resolves "the homepage" to a
+ * concrete artifact before calling this — we never slug a natural-language
+ * phrase.
+ */
+export function slugFromTarget(resolved, { cwd = process.cwd() } = {}) {
+  if (!resolved || typeof resolved !== 'string') return null;
+  const trimmed = resolved.trim();
+  if (!trimmed) return null;
+
+  // URL
+  if (/^https?:\/\//i.test(trimmed)) {
+    let url;
+    try { url = new URL(trimmed); } catch { return null; }
+    const hostPath = `${url.hostname}${url.pathname}`;
+    return kebab(hostPath);
+  }
+
+  // File path. Make it project-relative so two devs critiquing the same
+  // checkout get the same slug regardless of where their repo is cloned.
+  const abs = path.isAbsolute(trimmed) ? trimmed : path.resolve(cwd, trimmed);
+  let rel = path.relative(cwd, abs);
+  // If the target is outside cwd, fall back to the basename so we still
+  // produce a stable slug (vs the absolute path, which would include
+  // home dirs / usernames).
+  if (rel.startsWith('..') || path.isAbsolute(rel)) {
+    rel = path.basename(abs);
+  }
+  if (!rel || rel === '.' || rel === '') return null;
+  return kebab(rel);
+}
+
+function kebab(s) {
+  const slug = s
+    .toLowerCase()
+    .replace(/[/\\.]+/g, '-')
+    .replace(/[^a-z0-9-]+/g, '-')
+    .replace(/-+/g, '-')
+    .replace(/^-|-$/g, '');
+  if (!slug) return null;
+  // Cap from the tail — the tail (filename) is more identifying than the
+  // top-level directory.
+  return slug.length <= SLUG_MAX ? slug : slug.slice(slug.length - SLUG_MAX).replace(/^-/, '');
+}
+
+/**
+ * Filename-safe UTC ISO timestamp: hyphens for separators, trailing Z.
+ * Plain colons aren't allowed on Windows filesystems.
+ */
+export function nowFilenameStamp(date = new Date()) {
+  const iso = date.toISOString();           // 2026-05-12T18:30:00.123Z
+  return iso.replace(/[:.]/g, '-').replace(/-\d+Z$/, 'Z');
+}
+
+/**
+ * Write a snapshot for `slug`. `meta` carries the small structured frontmatter
+ * keys read back by readTrend(). `body` is the human-readable critique
+ * report (everything below the frontmatter).
+ *
+ * Returns the absolute path written.
+ */
+export function writeSnapshot({ slug, meta, body, cwd = process.cwd(), now = new Date() }) {
+  if (!slug) throw new Error('writeSnapshot requires a slug');
+  const dir = getCritiqueDir(cwd);
+  fs.mkdirSync(dir, { recursive: true });
+  const timestamp = nowFilenameStamp(now);
+  const filePath = path.join(dir, `${timestamp}__${slug}.md`);
+  // Spread `meta` first so internally computed `timestamp` and `slug`
+  // always win. Otherwise a caller-supplied meta blob (parsed from the
+  // IMPECCABLE_CRITIQUE_META env var) could clobber them, leaving the
+  // filename in disagreement with its frontmatter and corrupting trends.
+  const front = serializeFrontmatter({ ...meta, timestamp, slug });
+  fs.writeFileSync(filePath, `${front}\n${body.trim()}\n`, 'utf-8');
+  return filePath;
+}
+
+function serializeFrontmatter(obj) {
+  const lines = ['---'];
+  for (const [key, value] of Object.entries(obj)) {
+    if (value === undefined || value === null) continue;
+    const str = typeof value === 'string' ? value : String(value);
+    // Quote strings that contain : or # to keep parsing simple.
+    const needsQuotes = typeof value === 'string' && /[:#]/.test(str);
+    lines.push(`${key}: ${needsQuotes ? JSON.stringify(str) : str}`);
+  }
+  lines.push('---');
+  return lines.join('\n');
+}
+
+function parseFrontmatter(text) {
+  const match = text.match(/^---\r?\n([\s\S]*?)\r?\n---/);
+  if (!match) return {};
+  const out = {};
+  for (const line of match[1].split(/\r?\n/)) {
+    const colon = line.indexOf(':');
+    if (colon < 0) continue;
+    const key = line.slice(0, colon).trim();
+    let value = line.slice(colon + 1).trim();
+    if (/^".*"$/.test(value)) {
+      try { value = JSON.parse(value); } catch { /* leave as-is */ }
+    } else if (/^-?\d+$/.test(value)) {
+      value = Number(value);
+    }
+    out[key] = value;
+  }
+  return out;
+}
+
+/**
+ * Return all snapshot files for `slug`, sorted oldest → newest.
+ */
+function listSnapshotsForSlug(slug, cwd) {
+  const dir = getCritiqueDir(cwd);
+  if (!fs.existsSync(dir)) return [];
+  const suffix = `__${slug}.md`;
+  return fs.readdirSync(dir)
+    .filter((f) => f.endsWith(suffix))
+    .sort()
+    .map((f) => path.join(dir, f));
+}
+
+/**
+ * Return the most recent snapshot for `slug`, or null. Polish reads this
+ * to find its fix backlog when the slug matches.
+ */
+export function readLatestSnapshot(slug, { cwd = process.cwd() } = {}) {
+  const all = listSnapshotsForSlug(slug, cwd);
+  if (!all.length) return null;
+  const latest = all[all.length - 1];
+  const body = fs.readFileSync(latest, 'utf-8');
+  return { path: latest, body, meta: parseFrontmatter(body) };
+}
+
+/**
+ * Return the last `limit` snapshots' frontmatter, oldest → newest.
+ * Critique appends a one-line trend to its output using this.
+ */
+export function readTrend(slug, { limit = 5, cwd = process.cwd() } = {}) {
+  const all = listSnapshotsForSlug(slug, cwd);
+  const slice = all.slice(-limit);
+  return slice.map((file) => parseFrontmatter(fs.readFileSync(file, 'utf-8')));
+}
+
+// ---- CLI ---------------------------------------------------------------
+
+function main(argv) {
+  const [cmd, ...args] = argv;
+  switch (cmd) {
+    case 'slug': {
+      const slug = slugFromTarget(args[0]);
+      if (!slug) { process.stderr.write('no stable slug for input\n'); process.exit(1); }
+      process.stdout.write(`${slug}\n`);
+      return;
+    }
+    case 'write': {
+      const [slug, bodyFile] = args;
+      if (!slug || !bodyFile) { process.stderr.write('usage: write <slug> <body-file>\n'); process.exit(1); }
+      const raw = fs.readFileSync(bodyFile, 'utf-8');
+      // The body file may be a full report. The caller passes the meta as
+      // a JSON object on stdin if it wants structured frontmatter; otherwise
+      // we write with minimal metadata.
+      let meta = {};
+      const metaArg = process.env.IMPECCABLE_CRITIQUE_META;
+      if (metaArg) {
+        try { meta = JSON.parse(metaArg); } catch { /* ignore */ }
+      }
+      const out = writeSnapshot({ slug, meta, body: raw });
+      process.stdout.write(`${out}\n`);
+      return;
+    }
+    case 'latest': {
+      const latest = readLatestSnapshot(args[0]);
+      if (!latest) { process.exit(2); }
+      process.stdout.write(latest.body);
+      return;
+    }
+    case 'trend': {
+      const rows = readTrend(args[0], { limit: args[1] ? Number(args[1]) : 5 });
+      process.stdout.write(JSON.stringify(rows, null, 2) + '\n');
+      return;
+    }
+    default:
+      process.stderr.write('usage: critique-storage.mjs <slug|write|latest|trend> [args]\n');
+      process.exit(1);
+  }
+}
+
+if (import.meta.url === `file://${process.argv[1]}`) {
+  main(process.argv.slice(2));
+}
@@ -3,6 +3,7 @@ import path from 'node:path';

 export const IMPECCABLE_DIR = '.impeccable';
 export const LIVE_DIR = 'live';
+export const CRITIQUE_DIR = 'critique';

 export function getImpeccableDir(cwd = process.cwd()) {
  return path.join(cwd, IMPECCABLE_DIR);
@@ -96,6 +97,10 @@ export function getLiveAnnotationsDir(cwd = process.cwd()) {
  return path.join(getLiveDir(cwd), 'annotations');
 }

+export function getCritiqueDir(cwd = process.cwd()) {
+  return path.join(getImpeccableDir(cwd), CRITIQUE_DIR);
+}
+
 export function getLegacyLiveAnnotationsDir(cwd = process.cwd()) {
  return path.join(cwd, '.impeccable-live', 'annotations');
 }