diff --git a/.agents/skills/impeccable/reference/critique.md b/.agents/skills/impeccable/reference/critique.md index 82cae84ed..cdf30e095 100644 --- a/.agents/skills/impeccable/reference/critique.md +++ b/.agents/skills/impeccable/reference/critique.md @@ -1,6 +1,6 @@ ### Purpose -Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive/backlog for future commands. +Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive of that run. ### Hard Invariants @@ -97,7 +97,7 @@ Codex failure accounting: final Run Notes must include target slug, ignore list, Synthesize both assessments into a single report. Do NOT simply concatenate. Weave the findings together, noting where the LLM review and detector agree, where the detector caught issues the LLM missed, and where detector findings are false positives. -The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is only an archive/backlog for later commands. +The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is an archive of that run. Codex final-answer note: `$impeccable critique` produces a report artifact, so the final chat response should intentionally exceed the usual concise close-out style. Do not title the final response "Critique Summary" unless the user explicitly asked for a summary. @@ -219,7 +219,7 @@ Skip this step if the Setup slug was null (vague or root-level target). IMPECCABLE_CRITIQUE_META='{"target":"","total_score":,"max_score":,"na_heuristics":"","p0_count":,"p1_count":}' \ node .agents/skills/impeccable/scripts/critique-storage.mjs write "" ``` - `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. + `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. For a local file target, the helper also records an exact content fingerprint so polish can distinguish the assessed bytes from later edits without relying on Git state or timestamps. The helper prints the absolute path it wrote. Leave that file on disk. Polish closes it; this run does not. 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. diff --git a/.agents/skills/impeccable/reference/polish.md b/.agents/skills/impeccable/reference/polish.md index a9fd13935..cc38587b5 100644 --- a/.agents/skills/impeccable/reference/polish.md +++ b/.agents/skills/impeccable/reference/polish.md @@ -29,10 +29,10 @@ Use the feature yourself at the surface's representative sizes: desktop and mobi If a prior critique exists, use it as one input: ```bash -node .agents/skills/impeccable/scripts/critique-storage.mjs latest "" +node .agents/skills/impeccable/scripts/critique-storage.mjs latest "" --json ``` -Exit 0 returns the latest snapshot; incorporate relevant P0/P1 findings and name the snapshot read. Exit 2 means none exists. Perform an independent pass either way. +Exit 0 returns JSON with the latest snapshot's `body` and an exact `snapshot_file` identity. Retain `snapshot_file` until the end of the pass. For a local file target, the helper compares the file's exact current content fingerprint with the fingerprint captured by critique. Unchanged staged, unstaged, or untracked content remains current; any byte change, deletion, or replacement with a non-file closes the backlog it identified while preserving its trend history and exits 2. A URL target has no local fingerprint and remains current until explicitly closed. When current, incorporate relevant P0/P1 findings from `body` and name the snapshot read. Exit 2 means none exists or the target changed. Perform an independent pass either way. ## 3. Triage @@ -95,3 +95,11 @@ Walk the complete path again with mouse, keyboard, and touch where applicable. C Follow the quality guidance supplied by `context.mjs` and hooks, then run any other relevant QA commands. Context requests a manual scan only when no automatic detector is active; never add another detector pass. Fix real defects and document only narrow intentional exceptions. A clean scan does not replace visual judgment. Finish with a source diff: remove accidental churn, orphaned code, redundant values, and temporary artifacts. Ship only when the feature is functionally complete and consistently finished across the path. + +When this pass clears every Priority Issue it took from a snapshot, close that snapshot: + +```bash +node .agents/skills/impeccable/scripts/critique-storage.mjs close "" "" +``` + +This closes only the snapshot this pass actually processed; if a newer critique landed meanwhile, its backlog stays live. Do not close when no snapshot was read, when `snapshot_file` was not retained, or when Priority Issues remain. diff --git a/.agents/skills/impeccable/reference/routing.md b/.agents/skills/impeccable/reference/routing.md index 405bfc629..fd0937d2d 100644 --- a/.agents/skills/impeccable/reference/routing.md +++ b/.agents/skills/impeccable/reference/routing.md @@ -8,7 +8,7 @@ Reason over the signals; there is no score to obey: - `setup.hasDesign` false while `setup.hasCode` true → `document` (capture the visual system). - `critique.latest` is `null` → the project has never been critiqued; for a set-up project with a real surface, offering `$impeccable critique ` is a strong default. -- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog), or re-run `critique` if the snapshot looks stale. +- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog and closes it when stale or cleared). - `git.changedFiles` pointing at one surface → scope `audit` or `polish` to those files specifically, naming them. - `devServer.running` true → `live` is available for in-browser iteration; if false, don't lead with `live`. **`live` and the bundled `detect.mjs` are web-only.** If `setup.platform` is `ios`, `android`, or `adaptive`, don't lead with either; the browser overlay and the HTML rule engine don't apply to native app code. - Otherwise group by intent (build new / improve what's there / iterate visually), tailored to the current surface and `setup.platform`. diff --git a/.agents/skills/impeccable/scripts/critique-storage.mjs b/.agents/skills/impeccable/scripts/critique-storage.mjs index f23fded37..86d297390 100644 --- a/.agents/skills/impeccable/scripts/critique-storage.mjs +++ b/.agents/skills/impeccable/scripts/critique-storage.mjs @@ -16,8 +16,9 @@ * CLI entry points (called from skill instructions): * node critique-storage.mjs slug * node critique-storage.mjs write - * node critique-storage.mjs latest + * node critique-storage.mjs latest [--json] * node critique-storage.mjs trend [limit] + * node critique-storage.mjs close * * 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 @@ -27,6 +28,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { createHash } from 'node:crypto'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { getCritiqueDir } from './lib/impeccable-paths.mjs'; import { slugFromTarget } from './lib/target-slug.mjs'; @@ -50,6 +52,45 @@ export function nowFilenameStamp(date = new Date()) { return iso.replace(/[:.]/g, '-').replace(/-\d+Z$/, 'Z'); } +/** + * Return an exact content fingerprint for a local file target. URLs and + * non-files return null because their content is not available here. + * + * The fingerprint deliberately describes bytes, not Git state or mtimes: + * critique often assesses an uncommitted file, and a later polish run should + * inherit that backlog when the bytes are unchanged regardless of staging. + */ +function resolveLocalTargetPath(target, { cwd = process.cwd() } = {}) { + if (!target || /^https?:\/\//i.test(target)) return null; + return path.isAbsolute(target) ? path.resolve(target) : path.resolve(cwd, target); +} + +function resolveTargetIdentity(target, { cwd = process.cwd() } = {}) { + if (!target || typeof target !== 'string') return null; + if (/^https?:\/\//i.test(target)) { + try { + const url = new URL(target); + const pathname = url.pathname.replace(/\/+$/, '') || '/'; + return `url:${url.origin}${pathname}`; + } catch { + return null; + } + } + const filePath = resolveLocalTargetPath(target, { cwd }); + return filePath ? `file:${filePath}` : null; +} + +export function fingerprintTarget(target, { cwd = process.cwd() } = {}) { + const filePath = resolveLocalTargetPath(target, { cwd }); + if (!filePath) return null; + try { + if (!fs.statSync(filePath).isFile()) return null; + return `sha256:${createHash('sha256').update(fs.readFileSync(filePath)).digest('hex')}`; + } catch { + return null; + } +} + /** * Write a snapshot for `slug`. `meta` carries the small structured frontmatter * keys read back by readTrend(). `body` is the human-readable critique @@ -62,14 +103,27 @@ export function writeSnapshot({ slug, meta, body, cwd = process.cwd(), now = new 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; + const contents = `${front}\n${body.trim()}\n`; + + // A second critique can finish in the same UTC second. Use exclusive + // creation and a fixed-width suffix so concurrent writers cannot replace + // history and lexical ordering still keeps collision entries newest. + for (let collision = 0; collision <= 9999; collision += 1) { + const suffix = collision === 0 ? '' : `~${String(collision).padStart(4, '0')}`; + const filePath = path.join(dir, `${timestamp}${suffix}__${slug}.md`); + try { + fs.writeFileSync(filePath, contents, { encoding: 'utf-8', flag: 'wx' }); + return filePath; + } catch (error) { + if (error?.code !== 'EEXIST') throw error; + } + } + throw new Error(`Too many critique snapshots for ${slug} at ${timestamp}`); } function serializeFrontmatter(obj) { @@ -98,6 +152,8 @@ function parseFrontmatter(text) { try { value = JSON.parse(value); } catch { /* leave as-is */ } } else if (/^-?\d+$/.test(value)) { value = Number(value); + } else if (value === 'true' || value === 'false') { + value = value === 'true'; } out[key] = value; } @@ -107,7 +163,7 @@ function parseFrontmatter(text) { /** * Return snapshot files matching `suffix`, sorted oldest → newest. */ -const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z__.+\.md$/; +const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z(?:~\d{4})?__.+\.md$/; function listSnapshots(suffix, cwd) { const dir = getCritiqueDir(cwd); @@ -118,24 +174,105 @@ function listSnapshots(suffix, cwd) { .map((f) => path.join(dir, f)); } -function readLatestSnapshotMatching(suffix, cwd) { - const filePath = listSnapshots(suffix, cwd).at(-1); +function readSnapshot(filePath) { if (!filePath) return null; const body = fs.readFileSync(filePath, 'utf-8'); return { path: filePath, body, meta: parseFrontmatter(body) }; } +function snapshotTargetIdentity(snapshot) { + const targetPath = snapshot?.meta.target_path; + return snapshot?.meta.target_identity + || (targetPath ? `file:${targetPath}` : null); +} + +function readNewestSnapshot(slug, { cwd = process.cwd() } = {}) { + return readSnapshot(listSnapshots(`__${slug}.md`, cwd).at(-1)); +} + +function readNewestSnapshotForIdentity( + slug, + targetIdentity, + { cwd = process.cwd() } = {}, +) { + const matches = listSnapshots(`__${slug}.md`, cwd) + .map(readSnapshot) + .filter((snapshot) => snapshotTargetIdentity(snapshot) === targetIdentity); + return matches.at(-1) || null; +} + /** * 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() } = {}) { - return readLatestSnapshotMatching(`__${slug}.md`, cwd); + const latest = readNewestSnapshot(slug, { cwd }); + return latest?.meta.closed === true ? null : latest; +} + +/** + * Mark one exact snapshot closed without deleting the score history consumed + * by `trend`. Exact identity matters: a newer critique may land after polish + * reads its backlog, and that newer snapshot must remain live. `snapshotFile` + * may be the absolute path returned by readLatestSnapshot() or the basename + * emitted by `latest --json`. Returns the path marked closed, or null. + */ +export function closeSnapshot(snapshotFile, { cwd = process.cwd() } = {}) { + if (!snapshotFile || typeof snapshotFile !== 'string') return null; + const dir = path.resolve(getCritiqueDir(cwd)); + const snapshotPath = path.isAbsolute(snapshotFile) + ? path.resolve(snapshotFile) + : path.resolve(dir, snapshotFile); + const filename = path.basename(snapshotPath); + if ( + path.dirname(snapshotPath) !== dir + || !SNAPSHOT_FILENAME.test(filename) + ) return null; + + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) return null; + snapshot = readSnapshot(snapshotPath); + } catch { + return null; + } + if (!snapshot || snapshot.meta.closed === true) return null; + const closedBody = snapshot.body.replace( + /^(---\r?\n[\s\S]*?)(\r?\n---)/, + '$1\nclosed: true$2', + ); + if (closedBody === snapshot.body) { + throw new Error(`Cannot close snapshot without frontmatter: ${snapshot.path}`); + } + fs.writeFileSync(snapshot.path, closedBody, 'utf-8'); + return snapshot.path; } /** Return the most recent snapshot across all targets, or null. */ export function readLatestSnapshotAcrossTargets({ cwd = process.cwd() } = {}) { - return readLatestSnapshotMatching('.md', cwd); + const snapshots = listSnapshots('.md', cwd).map(readSnapshot); + const identifiedSlugs = new Set( + snapshots + .filter((snapshot) => snapshotTargetIdentity(snapshot)) + .map((snapshot) => snapshot.meta.slug), + ); + const latestByTarget = new Map(); + for (const snapshot of snapshots) { + if (!snapshot?.meta.slug) continue; + // Slugs are lossy: distinct targets such as foo/bar and foo-bar can share + // one. Keep each known identity's latest open/closed state independent so + // closing one target cannot hide another target's live backlog. Once a + // slug has any identity-aware snapshot, its older legacy records are no + // longer independently routable and must not resurface as zombie work. + const targetIdentity = snapshotTargetIdentity(snapshot); + if (!targetIdentity && identifiedSlugs.has(snapshot.meta.slug)) continue; + const streamKey = targetIdentity || `slug:${snapshot.meta.slug}`; + latestByTarget.set(streamKey, snapshot); + } + return [...latestByTarget.values()] + .filter((snapshot) => snapshot.meta.closed !== true) + .sort((a, b) => a.path.localeCompare(b.path)) + .at(-1) || null; } /** @@ -153,9 +290,13 @@ export function readTrend(slug, { limit = 5, cwd = process.cwd() } = {}) { // Accept either a ready slug or a concrete target (path/URL) everywhere, so // callers never have to run the slug step separately. Anything containing a // path or URL marker is resolved through slugFromTarget. +function isReadySlug(value) { + return /^[a-z0-9-]+$/.test(value || '') && !value.includes('/'); +} + function coerceSlug(value) { if (!value) return null; - if (/^[a-z0-9-]+$/.test(value) && !value.includes('/')) return value; + if (isReadySlug(value)) return value; return slugFromTarget(value); } @@ -181,14 +322,124 @@ function main(argv) { if (metaArg) { try { meta = JSON.parse(metaArg); } catch { /* ignore */ } } + // The helper, not caller-provided metadata, owns the target fingerprint. + // This makes the snapshot describe the exact file bytes critique saw. + delete meta.target_fingerprint; + delete meta.target_path; + delete meta.target_identity; + const targetIdentity = resolveTargetIdentity(slugArg); + if (targetIdentity) meta.target_identity = targetIdentity; + const targetFingerprint = fingerprintTarget(slugArg); + if (targetFingerprint) { + meta.target_fingerprint = targetFingerprint; + meta.target_path = resolveLocalTargetPath(slugArg); + } const out = writeSnapshot({ slug, meta, body: raw }); process.stdout.write(`${out}\n`); return; } case 'latest': { - const latest = readLatestSnapshot(coerceSlug(args[0])); - if (!latest) { process.exit(2); } - process.stdout.write(latest.body); + const target = args[0]; + const format = args[1]; + const slug = coerceSlug(target); + if (!slug || (format && format !== '--json')) { + process.stderr.write('usage: latest [--json]\n'); + process.exit(1); + } + const targetFingerprint = fingerprintTarget(target); + const targetPath = resolveLocalTargetPath(target); + const targetIdentity = resolveTargetIdentity(target); + const readySlug = isReadySlug(target); + const newestForSlug = readNewestSnapshot(slug); + if (!newestForSlug) { process.exit(2); } + + // Concrete targets select the newest snapshot for their exact identity, + // not merely the newest filename for a lossy slug. This keeps distinct + // targets such as foo/bar and foo-bar from hiding each other's backlog. + const exactSnapshot = readNewestSnapshotForIdentity(slug, targetIdentity); + let latest = exactSnapshot; + if (!latest && !readySlug) { + // Legacy snapshots have no identity. Preserve their old explicit + // path/URL behavior only when no known target identity was selected. + latest = readNewestSnapshotForIdentity(slug, null); + } + if (!latest) latest = newestForSlug; + if (latest.meta.closed === true) { process.exit(2); } + + const recordedTargetPath = latest.meta.target_path; + const recordedTargetIdentity = snapshotTargetIdentity(latest); + const matchingIdentity = recordedTargetIdentity === targetIdentity; + + // Bare slugs remain a supported lookup mode, including for URL + // snapshots. But when a same-named local file exists, the request is + // ambiguous unless that exact file owns the snapshot identity. + if (readySlug && !recordedTargetIdentity) { + process.stderr.write( + 'ambiguous legacy snapshot target; use an explicit ./path or full URL\n', + ); + process.exit(2); + } + if (readySlug && targetPath && fs.existsSync(targetPath) && !matchingIdentity) { + process.stderr.write( + 'ambiguous snapshot slug; use an explicit ./path or remove the local name collision\n', + ); + process.exit(2); + } + + const concreteTarget = !readySlug || matchingIdentity; + if (concreteTarget && recordedTargetIdentity && !matchingIdentity) { + process.exit(2); + } + const concreteLocalTarget = concreteTarget && targetPath; + if (concreteLocalTarget && latest.meta.target_fingerprint !== targetFingerprint) { + closeSnapshot(latest.path); + process.exit(2); + } + if (format === '--json') { + process.stdout.write(JSON.stringify({ + snapshot_file: path.basename(latest.path), + body: latest.body, + }, null, 2) + '\n'); + } else { + process.stdout.write(latest.body); + } + return; + } + case 'close': { + const [slugArg, snapshotFile, ...extra] = args; + const slug = coerceSlug(slugArg); + if (!slug || !snapshotFile || extra.length > 0) { + process.stderr.write('usage: close \n'); + process.exit(1); + } + if ( + path.basename(snapshotFile) !== snapshotFile + || !SNAPSHOT_FILENAME.test(snapshotFile) + || !snapshotFile.endsWith(`__${slug}.md`) + ) process.exit(2); + + // A slug and filename are not enough to prove ownership because two + // distinct targets can normalize to the same slug. Modern snapshots + // carry a canonical identity, so require the supplied resolved target + // to match it before allowing the exact snapshot to be closed. Legacy + // snapshots without identity retain their historical close behavior. + const snapshotPath = path.join(getCritiqueDir(process.cwd()), snapshotFile); + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) process.exit(2); + snapshot = readSnapshot(snapshotPath); + } catch { + process.exit(2); + } + const recordedTargetIdentity = snapshotTargetIdentity(snapshot); + if ( + recordedTargetIdentity + && recordedTargetIdentity !== resolveTargetIdentity(slugArg) + ) process.exit(2); + + const closed = closeSnapshot(snapshotFile); + if (!closed) { process.exit(2); } + process.stdout.write(`${closed}\n`); return; } case 'trend': { @@ -197,7 +448,7 @@ function main(argv) { return; } default: - process.stderr.write('usage: critique-storage.mjs [args]\n'); + process.stderr.write('usage: critique-storage.mjs [args]\n'); process.exit(1); } } diff --git a/.claude/skills/impeccable/reference/critique.md b/.claude/skills/impeccable/reference/critique.md index 637e73fca..8c09ba2e6 100644 --- a/.claude/skills/impeccable/reference/critique.md +++ b/.claude/skills/impeccable/reference/critique.md @@ -1,6 +1,6 @@ ### Purpose -Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive/backlog for future commands. +Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive of that run. ### Hard Invariants @@ -84,7 +84,7 @@ After Assessment B returns usable CLI findings, reuse them. Do not rerun `detect Synthesize both assessments into a single report. Do NOT simply concatenate. Weave the findings together, noting where the LLM review and detector agree, where the detector caught issues the LLM missed, and where detector findings are false positives. -The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is only an archive/backlog for later commands. +The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is an archive of that run. Structure your feedback as a design director would: @@ -197,7 +197,7 @@ Skip this step if the Setup slug was null (vague or root-level target). IMPECCABLE_CRITIQUE_META='{"target":"","total_score":,"max_score":,"na_heuristics":"","p0_count":,"p1_count":}' \ node .claude/skills/impeccable/scripts/critique-storage.mjs write "" ``` - `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. + `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. For a local file target, the helper also records an exact content fingerprint so polish can distinguish the assessed bytes from later edits without relying on Git state or timestamps. The helper prints the absolute path it wrote. Leave that file on disk. Polish closes it; this run does not. 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. diff --git a/.claude/skills/impeccable/reference/polish.md b/.claude/skills/impeccable/reference/polish.md index 9db878697..77fe219ed 100644 --- a/.claude/skills/impeccable/reference/polish.md +++ b/.claude/skills/impeccable/reference/polish.md @@ -29,10 +29,10 @@ Use the feature yourself at the surface's representative sizes: desktop and mobi If a prior critique exists, use it as one input: ```bash -node .claude/skills/impeccable/scripts/critique-storage.mjs latest "" +node .claude/skills/impeccable/scripts/critique-storage.mjs latest "" --json ``` -Exit 0 returns the latest snapshot; incorporate relevant P0/P1 findings and name the snapshot read. Exit 2 means none exists. Perform an independent pass either way. +Exit 0 returns JSON with the latest snapshot's `body` and an exact `snapshot_file` identity. Retain `snapshot_file` until the end of the pass. For a local file target, the helper compares the file's exact current content fingerprint with the fingerprint captured by critique. Unchanged staged, unstaged, or untracked content remains current; any byte change, deletion, or replacement with a non-file closes the backlog it identified while preserving its trend history and exits 2. A URL target has no local fingerprint and remains current until explicitly closed. When current, incorporate relevant P0/P1 findings from `body` and name the snapshot read. Exit 2 means none exists or the target changed. Perform an independent pass either way. ## 3. Triage @@ -95,3 +95,11 @@ Walk the complete path again with mouse, keyboard, and touch where applicable. C Follow the quality guidance supplied by `context.mjs` and hooks, then run any other relevant QA commands. Context requests a manual scan only when no automatic detector is active; never add another detector pass. Fix real defects and document only narrow intentional exceptions. A clean scan does not replace visual judgment. Finish with a source diff: remove accidental churn, orphaned code, redundant values, and temporary artifacts. Ship only when the feature is functionally complete and consistently finished across the path. + +When this pass clears every Priority Issue it took from a snapshot, close that snapshot: + +```bash +node .claude/skills/impeccable/scripts/critique-storage.mjs close "" "" +``` + +This closes only the snapshot this pass actually processed; if a newer critique landed meanwhile, its backlog stays live. Do not close when no snapshot was read, when `snapshot_file` was not retained, or when Priority Issues remain. diff --git a/.claude/skills/impeccable/reference/routing.md b/.claude/skills/impeccable/reference/routing.md index da49eb993..9d1b9c005 100644 --- a/.claude/skills/impeccable/reference/routing.md +++ b/.claude/skills/impeccable/reference/routing.md @@ -8,7 +8,7 @@ Reason over the signals; there is no score to obey: - `setup.hasDesign` false while `setup.hasCode` true → `document` (capture the visual system). - `critique.latest` is `null` → the project has never been critiqued; for a set-up project with a real surface, offering `/impeccable critique ` is a strong default. -- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog), or re-run `critique` if the snapshot looks stale. +- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog and closes it when stale or cleared). - `git.changedFiles` pointing at one surface → scope `audit` or `polish` to those files specifically, naming them. - `devServer.running` true → `live` is available for in-browser iteration; if false, don't lead with `live`. **`live` and the bundled `detect.mjs` are web-only.** If `setup.platform` is `ios`, `android`, or `adaptive`, don't lead with either; the browser overlay and the HTML rule engine don't apply to native app code. - Otherwise group by intent (build new / improve what's there / iterate visually), tailored to the current surface and `setup.platform`. diff --git a/.claude/skills/impeccable/scripts/critique-storage.mjs b/.claude/skills/impeccable/scripts/critique-storage.mjs index f23fded37..86d297390 100644 --- a/.claude/skills/impeccable/scripts/critique-storage.mjs +++ b/.claude/skills/impeccable/scripts/critique-storage.mjs @@ -16,8 +16,9 @@ * CLI entry points (called from skill instructions): * node critique-storage.mjs slug * node critique-storage.mjs write - * node critique-storage.mjs latest + * node critique-storage.mjs latest [--json] * node critique-storage.mjs trend [limit] + * node critique-storage.mjs close * * 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 @@ -27,6 +28,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { createHash } from 'node:crypto'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { getCritiqueDir } from './lib/impeccable-paths.mjs'; import { slugFromTarget } from './lib/target-slug.mjs'; @@ -50,6 +52,45 @@ export function nowFilenameStamp(date = new Date()) { return iso.replace(/[:.]/g, '-').replace(/-\d+Z$/, 'Z'); } +/** + * Return an exact content fingerprint for a local file target. URLs and + * non-files return null because their content is not available here. + * + * The fingerprint deliberately describes bytes, not Git state or mtimes: + * critique often assesses an uncommitted file, and a later polish run should + * inherit that backlog when the bytes are unchanged regardless of staging. + */ +function resolveLocalTargetPath(target, { cwd = process.cwd() } = {}) { + if (!target || /^https?:\/\//i.test(target)) return null; + return path.isAbsolute(target) ? path.resolve(target) : path.resolve(cwd, target); +} + +function resolveTargetIdentity(target, { cwd = process.cwd() } = {}) { + if (!target || typeof target !== 'string') return null; + if (/^https?:\/\//i.test(target)) { + try { + const url = new URL(target); + const pathname = url.pathname.replace(/\/+$/, '') || '/'; + return `url:${url.origin}${pathname}`; + } catch { + return null; + } + } + const filePath = resolveLocalTargetPath(target, { cwd }); + return filePath ? `file:${filePath}` : null; +} + +export function fingerprintTarget(target, { cwd = process.cwd() } = {}) { + const filePath = resolveLocalTargetPath(target, { cwd }); + if (!filePath) return null; + try { + if (!fs.statSync(filePath).isFile()) return null; + return `sha256:${createHash('sha256').update(fs.readFileSync(filePath)).digest('hex')}`; + } catch { + return null; + } +} + /** * Write a snapshot for `slug`. `meta` carries the small structured frontmatter * keys read back by readTrend(). `body` is the human-readable critique @@ -62,14 +103,27 @@ export function writeSnapshot({ slug, meta, body, cwd = process.cwd(), now = new 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; + const contents = `${front}\n${body.trim()}\n`; + + // A second critique can finish in the same UTC second. Use exclusive + // creation and a fixed-width suffix so concurrent writers cannot replace + // history and lexical ordering still keeps collision entries newest. + for (let collision = 0; collision <= 9999; collision += 1) { + const suffix = collision === 0 ? '' : `~${String(collision).padStart(4, '0')}`; + const filePath = path.join(dir, `${timestamp}${suffix}__${slug}.md`); + try { + fs.writeFileSync(filePath, contents, { encoding: 'utf-8', flag: 'wx' }); + return filePath; + } catch (error) { + if (error?.code !== 'EEXIST') throw error; + } + } + throw new Error(`Too many critique snapshots for ${slug} at ${timestamp}`); } function serializeFrontmatter(obj) { @@ -98,6 +152,8 @@ function parseFrontmatter(text) { try { value = JSON.parse(value); } catch { /* leave as-is */ } } else if (/^-?\d+$/.test(value)) { value = Number(value); + } else if (value === 'true' || value === 'false') { + value = value === 'true'; } out[key] = value; } @@ -107,7 +163,7 @@ function parseFrontmatter(text) { /** * Return snapshot files matching `suffix`, sorted oldest → newest. */ -const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z__.+\.md$/; +const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z(?:~\d{4})?__.+\.md$/; function listSnapshots(suffix, cwd) { const dir = getCritiqueDir(cwd); @@ -118,24 +174,105 @@ function listSnapshots(suffix, cwd) { .map((f) => path.join(dir, f)); } -function readLatestSnapshotMatching(suffix, cwd) { - const filePath = listSnapshots(suffix, cwd).at(-1); +function readSnapshot(filePath) { if (!filePath) return null; const body = fs.readFileSync(filePath, 'utf-8'); return { path: filePath, body, meta: parseFrontmatter(body) }; } +function snapshotTargetIdentity(snapshot) { + const targetPath = snapshot?.meta.target_path; + return snapshot?.meta.target_identity + || (targetPath ? `file:${targetPath}` : null); +} + +function readNewestSnapshot(slug, { cwd = process.cwd() } = {}) { + return readSnapshot(listSnapshots(`__${slug}.md`, cwd).at(-1)); +} + +function readNewestSnapshotForIdentity( + slug, + targetIdentity, + { cwd = process.cwd() } = {}, +) { + const matches = listSnapshots(`__${slug}.md`, cwd) + .map(readSnapshot) + .filter((snapshot) => snapshotTargetIdentity(snapshot) === targetIdentity); + return matches.at(-1) || null; +} + /** * 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() } = {}) { - return readLatestSnapshotMatching(`__${slug}.md`, cwd); + const latest = readNewestSnapshot(slug, { cwd }); + return latest?.meta.closed === true ? null : latest; +} + +/** + * Mark one exact snapshot closed without deleting the score history consumed + * by `trend`. Exact identity matters: a newer critique may land after polish + * reads its backlog, and that newer snapshot must remain live. `snapshotFile` + * may be the absolute path returned by readLatestSnapshot() or the basename + * emitted by `latest --json`. Returns the path marked closed, or null. + */ +export function closeSnapshot(snapshotFile, { cwd = process.cwd() } = {}) { + if (!snapshotFile || typeof snapshotFile !== 'string') return null; + const dir = path.resolve(getCritiqueDir(cwd)); + const snapshotPath = path.isAbsolute(snapshotFile) + ? path.resolve(snapshotFile) + : path.resolve(dir, snapshotFile); + const filename = path.basename(snapshotPath); + if ( + path.dirname(snapshotPath) !== dir + || !SNAPSHOT_FILENAME.test(filename) + ) return null; + + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) return null; + snapshot = readSnapshot(snapshotPath); + } catch { + return null; + } + if (!snapshot || snapshot.meta.closed === true) return null; + const closedBody = snapshot.body.replace( + /^(---\r?\n[\s\S]*?)(\r?\n---)/, + '$1\nclosed: true$2', + ); + if (closedBody === snapshot.body) { + throw new Error(`Cannot close snapshot without frontmatter: ${snapshot.path}`); + } + fs.writeFileSync(snapshot.path, closedBody, 'utf-8'); + return snapshot.path; } /** Return the most recent snapshot across all targets, or null. */ export function readLatestSnapshotAcrossTargets({ cwd = process.cwd() } = {}) { - return readLatestSnapshotMatching('.md', cwd); + const snapshots = listSnapshots('.md', cwd).map(readSnapshot); + const identifiedSlugs = new Set( + snapshots + .filter((snapshot) => snapshotTargetIdentity(snapshot)) + .map((snapshot) => snapshot.meta.slug), + ); + const latestByTarget = new Map(); + for (const snapshot of snapshots) { + if (!snapshot?.meta.slug) continue; + // Slugs are lossy: distinct targets such as foo/bar and foo-bar can share + // one. Keep each known identity's latest open/closed state independent so + // closing one target cannot hide another target's live backlog. Once a + // slug has any identity-aware snapshot, its older legacy records are no + // longer independently routable and must not resurface as zombie work. + const targetIdentity = snapshotTargetIdentity(snapshot); + if (!targetIdentity && identifiedSlugs.has(snapshot.meta.slug)) continue; + const streamKey = targetIdentity || `slug:${snapshot.meta.slug}`; + latestByTarget.set(streamKey, snapshot); + } + return [...latestByTarget.values()] + .filter((snapshot) => snapshot.meta.closed !== true) + .sort((a, b) => a.path.localeCompare(b.path)) + .at(-1) || null; } /** @@ -153,9 +290,13 @@ export function readTrend(slug, { limit = 5, cwd = process.cwd() } = {}) { // Accept either a ready slug or a concrete target (path/URL) everywhere, so // callers never have to run the slug step separately. Anything containing a // path or URL marker is resolved through slugFromTarget. +function isReadySlug(value) { + return /^[a-z0-9-]+$/.test(value || '') && !value.includes('/'); +} + function coerceSlug(value) { if (!value) return null; - if (/^[a-z0-9-]+$/.test(value) && !value.includes('/')) return value; + if (isReadySlug(value)) return value; return slugFromTarget(value); } @@ -181,14 +322,124 @@ function main(argv) { if (metaArg) { try { meta = JSON.parse(metaArg); } catch { /* ignore */ } } + // The helper, not caller-provided metadata, owns the target fingerprint. + // This makes the snapshot describe the exact file bytes critique saw. + delete meta.target_fingerprint; + delete meta.target_path; + delete meta.target_identity; + const targetIdentity = resolveTargetIdentity(slugArg); + if (targetIdentity) meta.target_identity = targetIdentity; + const targetFingerprint = fingerprintTarget(slugArg); + if (targetFingerprint) { + meta.target_fingerprint = targetFingerprint; + meta.target_path = resolveLocalTargetPath(slugArg); + } const out = writeSnapshot({ slug, meta, body: raw }); process.stdout.write(`${out}\n`); return; } case 'latest': { - const latest = readLatestSnapshot(coerceSlug(args[0])); - if (!latest) { process.exit(2); } - process.stdout.write(latest.body); + const target = args[0]; + const format = args[1]; + const slug = coerceSlug(target); + if (!slug || (format && format !== '--json')) { + process.stderr.write('usage: latest [--json]\n'); + process.exit(1); + } + const targetFingerprint = fingerprintTarget(target); + const targetPath = resolveLocalTargetPath(target); + const targetIdentity = resolveTargetIdentity(target); + const readySlug = isReadySlug(target); + const newestForSlug = readNewestSnapshot(slug); + if (!newestForSlug) { process.exit(2); } + + // Concrete targets select the newest snapshot for their exact identity, + // not merely the newest filename for a lossy slug. This keeps distinct + // targets such as foo/bar and foo-bar from hiding each other's backlog. + const exactSnapshot = readNewestSnapshotForIdentity(slug, targetIdentity); + let latest = exactSnapshot; + if (!latest && !readySlug) { + // Legacy snapshots have no identity. Preserve their old explicit + // path/URL behavior only when no known target identity was selected. + latest = readNewestSnapshotForIdentity(slug, null); + } + if (!latest) latest = newestForSlug; + if (latest.meta.closed === true) { process.exit(2); } + + const recordedTargetPath = latest.meta.target_path; + const recordedTargetIdentity = snapshotTargetIdentity(latest); + const matchingIdentity = recordedTargetIdentity === targetIdentity; + + // Bare slugs remain a supported lookup mode, including for URL + // snapshots. But when a same-named local file exists, the request is + // ambiguous unless that exact file owns the snapshot identity. + if (readySlug && !recordedTargetIdentity) { + process.stderr.write( + 'ambiguous legacy snapshot target; use an explicit ./path or full URL\n', + ); + process.exit(2); + } + if (readySlug && targetPath && fs.existsSync(targetPath) && !matchingIdentity) { + process.stderr.write( + 'ambiguous snapshot slug; use an explicit ./path or remove the local name collision\n', + ); + process.exit(2); + } + + const concreteTarget = !readySlug || matchingIdentity; + if (concreteTarget && recordedTargetIdentity && !matchingIdentity) { + process.exit(2); + } + const concreteLocalTarget = concreteTarget && targetPath; + if (concreteLocalTarget && latest.meta.target_fingerprint !== targetFingerprint) { + closeSnapshot(latest.path); + process.exit(2); + } + if (format === '--json') { + process.stdout.write(JSON.stringify({ + snapshot_file: path.basename(latest.path), + body: latest.body, + }, null, 2) + '\n'); + } else { + process.stdout.write(latest.body); + } + return; + } + case 'close': { + const [slugArg, snapshotFile, ...extra] = args; + const slug = coerceSlug(slugArg); + if (!slug || !snapshotFile || extra.length > 0) { + process.stderr.write('usage: close \n'); + process.exit(1); + } + if ( + path.basename(snapshotFile) !== snapshotFile + || !SNAPSHOT_FILENAME.test(snapshotFile) + || !snapshotFile.endsWith(`__${slug}.md`) + ) process.exit(2); + + // A slug and filename are not enough to prove ownership because two + // distinct targets can normalize to the same slug. Modern snapshots + // carry a canonical identity, so require the supplied resolved target + // to match it before allowing the exact snapshot to be closed. Legacy + // snapshots without identity retain their historical close behavior. + const snapshotPath = path.join(getCritiqueDir(process.cwd()), snapshotFile); + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) process.exit(2); + snapshot = readSnapshot(snapshotPath); + } catch { + process.exit(2); + } + const recordedTargetIdentity = snapshotTargetIdentity(snapshot); + if ( + recordedTargetIdentity + && recordedTargetIdentity !== resolveTargetIdentity(slugArg) + ) process.exit(2); + + const closed = closeSnapshot(snapshotFile); + if (!closed) { process.exit(2); } + process.stdout.write(`${closed}\n`); return; } case 'trend': { @@ -197,7 +448,7 @@ function main(argv) { return; } default: - process.stderr.write('usage: critique-storage.mjs [args]\n'); + process.stderr.write('usage: critique-storage.mjs [args]\n'); process.exit(1); } } diff --git a/.cursor/skills/impeccable/reference/critique.md b/.cursor/skills/impeccable/reference/critique.md index 5f5026768..2ba946344 100644 --- a/.cursor/skills/impeccable/reference/critique.md +++ b/.cursor/skills/impeccable/reference/critique.md @@ -1,6 +1,6 @@ ### Purpose -Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive/backlog for future commands. +Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive of that run. ### Hard Invariants @@ -84,7 +84,7 @@ After Assessment B returns usable CLI findings, reuse them. Do not rerun `detect Synthesize both assessments into a single report. Do NOT simply concatenate. Weave the findings together, noting where the LLM review and detector agree, where the detector caught issues the LLM missed, and where detector findings are false positives. -The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is only an archive/backlog for later commands. +The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is an archive of that run. Structure your feedback as a design director would: @@ -197,7 +197,7 @@ Skip this step if the Setup slug was null (vague or root-level target). IMPECCABLE_CRITIQUE_META='{"target":"","total_score":,"max_score":,"na_heuristics":"","p0_count":,"p1_count":}' \ node .cursor/skills/impeccable/scripts/critique-storage.mjs write "" ``` - `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. + `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. For a local file target, the helper also records an exact content fingerprint so polish can distinguish the assessed bytes from later edits without relying on Git state or timestamps. The helper prints the absolute path it wrote. Leave that file on disk. Polish closes it; this run does not. 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. diff --git a/.cursor/skills/impeccable/reference/polish.md b/.cursor/skills/impeccable/reference/polish.md index 7af88f60c..2c1c31e80 100644 --- a/.cursor/skills/impeccable/reference/polish.md +++ b/.cursor/skills/impeccable/reference/polish.md @@ -29,10 +29,10 @@ Use the feature yourself at the surface's representative sizes: desktop and mobi If a prior critique exists, use it as one input: ```bash -node .cursor/skills/impeccable/scripts/critique-storage.mjs latest "" +node .cursor/skills/impeccable/scripts/critique-storage.mjs latest "" --json ``` -Exit 0 returns the latest snapshot; incorporate relevant P0/P1 findings and name the snapshot read. Exit 2 means none exists. Perform an independent pass either way. +Exit 0 returns JSON with the latest snapshot's `body` and an exact `snapshot_file` identity. Retain `snapshot_file` until the end of the pass. For a local file target, the helper compares the file's exact current content fingerprint with the fingerprint captured by critique. Unchanged staged, unstaged, or untracked content remains current; any byte change, deletion, or replacement with a non-file closes the backlog it identified while preserving its trend history and exits 2. A URL target has no local fingerprint and remains current until explicitly closed. When current, incorporate relevant P0/P1 findings from `body` and name the snapshot read. Exit 2 means none exists or the target changed. Perform an independent pass either way. ## 3. Triage @@ -95,3 +95,11 @@ Walk the complete path again with mouse, keyboard, and touch where applicable. C Follow the quality guidance supplied by `context.mjs` and hooks, then run any other relevant QA commands. Context requests a manual scan only when no automatic detector is active; never add another detector pass. Fix real defects and document only narrow intentional exceptions. A clean scan does not replace visual judgment. Finish with a source diff: remove accidental churn, orphaned code, redundant values, and temporary artifacts. Ship only when the feature is functionally complete and consistently finished across the path. + +When this pass clears every Priority Issue it took from a snapshot, close that snapshot: + +```bash +node .cursor/skills/impeccable/scripts/critique-storage.mjs close "" "" +``` + +This closes only the snapshot this pass actually processed; if a newer critique landed meanwhile, its backlog stays live. Do not close when no snapshot was read, when `snapshot_file` was not retained, or when Priority Issues remain. diff --git a/.cursor/skills/impeccable/reference/routing.md b/.cursor/skills/impeccable/reference/routing.md index 0568954d5..074998baa 100644 --- a/.cursor/skills/impeccable/reference/routing.md +++ b/.cursor/skills/impeccable/reference/routing.md @@ -8,7 +8,7 @@ Reason over the signals; there is no score to obey: - `setup.hasDesign` false while `setup.hasCode` true → `document` (capture the visual system). - `critique.latest` is `null` → the project has never been critiqued; for a set-up project with a real surface, offering `/impeccable critique ` is a strong default. -- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog), or re-run `critique` if the snapshot looks stale. +- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog and closes it when stale or cleared). - `git.changedFiles` pointing at one surface → scope `audit` or `polish` to those files specifically, naming them. - `devServer.running` true → `live` is available for in-browser iteration; if false, don't lead with `live`. **`live` and the bundled `detect.mjs` are web-only.** If `setup.platform` is `ios`, `android`, or `adaptive`, don't lead with either; the browser overlay and the HTML rule engine don't apply to native app code. - Otherwise group by intent (build new / improve what's there / iterate visually), tailored to the current surface and `setup.platform`. diff --git a/.cursor/skills/impeccable/scripts/critique-storage.mjs b/.cursor/skills/impeccable/scripts/critique-storage.mjs index f23fded37..86d297390 100644 --- a/.cursor/skills/impeccable/scripts/critique-storage.mjs +++ b/.cursor/skills/impeccable/scripts/critique-storage.mjs @@ -16,8 +16,9 @@ * CLI entry points (called from skill instructions): * node critique-storage.mjs slug * node critique-storage.mjs write - * node critique-storage.mjs latest + * node critique-storage.mjs latest [--json] * node critique-storage.mjs trend [limit] + * node critique-storage.mjs close * * 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 @@ -27,6 +28,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { createHash } from 'node:crypto'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { getCritiqueDir } from './lib/impeccable-paths.mjs'; import { slugFromTarget } from './lib/target-slug.mjs'; @@ -50,6 +52,45 @@ export function nowFilenameStamp(date = new Date()) { return iso.replace(/[:.]/g, '-').replace(/-\d+Z$/, 'Z'); } +/** + * Return an exact content fingerprint for a local file target. URLs and + * non-files return null because their content is not available here. + * + * The fingerprint deliberately describes bytes, not Git state or mtimes: + * critique often assesses an uncommitted file, and a later polish run should + * inherit that backlog when the bytes are unchanged regardless of staging. + */ +function resolveLocalTargetPath(target, { cwd = process.cwd() } = {}) { + if (!target || /^https?:\/\//i.test(target)) return null; + return path.isAbsolute(target) ? path.resolve(target) : path.resolve(cwd, target); +} + +function resolveTargetIdentity(target, { cwd = process.cwd() } = {}) { + if (!target || typeof target !== 'string') return null; + if (/^https?:\/\//i.test(target)) { + try { + const url = new URL(target); + const pathname = url.pathname.replace(/\/+$/, '') || '/'; + return `url:${url.origin}${pathname}`; + } catch { + return null; + } + } + const filePath = resolveLocalTargetPath(target, { cwd }); + return filePath ? `file:${filePath}` : null; +} + +export function fingerprintTarget(target, { cwd = process.cwd() } = {}) { + const filePath = resolveLocalTargetPath(target, { cwd }); + if (!filePath) return null; + try { + if (!fs.statSync(filePath).isFile()) return null; + return `sha256:${createHash('sha256').update(fs.readFileSync(filePath)).digest('hex')}`; + } catch { + return null; + } +} + /** * Write a snapshot for `slug`. `meta` carries the small structured frontmatter * keys read back by readTrend(). `body` is the human-readable critique @@ -62,14 +103,27 @@ export function writeSnapshot({ slug, meta, body, cwd = process.cwd(), now = new 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; + const contents = `${front}\n${body.trim()}\n`; + + // A second critique can finish in the same UTC second. Use exclusive + // creation and a fixed-width suffix so concurrent writers cannot replace + // history and lexical ordering still keeps collision entries newest. + for (let collision = 0; collision <= 9999; collision += 1) { + const suffix = collision === 0 ? '' : `~${String(collision).padStart(4, '0')}`; + const filePath = path.join(dir, `${timestamp}${suffix}__${slug}.md`); + try { + fs.writeFileSync(filePath, contents, { encoding: 'utf-8', flag: 'wx' }); + return filePath; + } catch (error) { + if (error?.code !== 'EEXIST') throw error; + } + } + throw new Error(`Too many critique snapshots for ${slug} at ${timestamp}`); } function serializeFrontmatter(obj) { @@ -98,6 +152,8 @@ function parseFrontmatter(text) { try { value = JSON.parse(value); } catch { /* leave as-is */ } } else if (/^-?\d+$/.test(value)) { value = Number(value); + } else if (value === 'true' || value === 'false') { + value = value === 'true'; } out[key] = value; } @@ -107,7 +163,7 @@ function parseFrontmatter(text) { /** * Return snapshot files matching `suffix`, sorted oldest → newest. */ -const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z__.+\.md$/; +const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z(?:~\d{4})?__.+\.md$/; function listSnapshots(suffix, cwd) { const dir = getCritiqueDir(cwd); @@ -118,24 +174,105 @@ function listSnapshots(suffix, cwd) { .map((f) => path.join(dir, f)); } -function readLatestSnapshotMatching(suffix, cwd) { - const filePath = listSnapshots(suffix, cwd).at(-1); +function readSnapshot(filePath) { if (!filePath) return null; const body = fs.readFileSync(filePath, 'utf-8'); return { path: filePath, body, meta: parseFrontmatter(body) }; } +function snapshotTargetIdentity(snapshot) { + const targetPath = snapshot?.meta.target_path; + return snapshot?.meta.target_identity + || (targetPath ? `file:${targetPath}` : null); +} + +function readNewestSnapshot(slug, { cwd = process.cwd() } = {}) { + return readSnapshot(listSnapshots(`__${slug}.md`, cwd).at(-1)); +} + +function readNewestSnapshotForIdentity( + slug, + targetIdentity, + { cwd = process.cwd() } = {}, +) { + const matches = listSnapshots(`__${slug}.md`, cwd) + .map(readSnapshot) + .filter((snapshot) => snapshotTargetIdentity(snapshot) === targetIdentity); + return matches.at(-1) || null; +} + /** * 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() } = {}) { - return readLatestSnapshotMatching(`__${slug}.md`, cwd); + const latest = readNewestSnapshot(slug, { cwd }); + return latest?.meta.closed === true ? null : latest; +} + +/** + * Mark one exact snapshot closed without deleting the score history consumed + * by `trend`. Exact identity matters: a newer critique may land after polish + * reads its backlog, and that newer snapshot must remain live. `snapshotFile` + * may be the absolute path returned by readLatestSnapshot() or the basename + * emitted by `latest --json`. Returns the path marked closed, or null. + */ +export function closeSnapshot(snapshotFile, { cwd = process.cwd() } = {}) { + if (!snapshotFile || typeof snapshotFile !== 'string') return null; + const dir = path.resolve(getCritiqueDir(cwd)); + const snapshotPath = path.isAbsolute(snapshotFile) + ? path.resolve(snapshotFile) + : path.resolve(dir, snapshotFile); + const filename = path.basename(snapshotPath); + if ( + path.dirname(snapshotPath) !== dir + || !SNAPSHOT_FILENAME.test(filename) + ) return null; + + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) return null; + snapshot = readSnapshot(snapshotPath); + } catch { + return null; + } + if (!snapshot || snapshot.meta.closed === true) return null; + const closedBody = snapshot.body.replace( + /^(---\r?\n[\s\S]*?)(\r?\n---)/, + '$1\nclosed: true$2', + ); + if (closedBody === snapshot.body) { + throw new Error(`Cannot close snapshot without frontmatter: ${snapshot.path}`); + } + fs.writeFileSync(snapshot.path, closedBody, 'utf-8'); + return snapshot.path; } /** Return the most recent snapshot across all targets, or null. */ export function readLatestSnapshotAcrossTargets({ cwd = process.cwd() } = {}) { - return readLatestSnapshotMatching('.md', cwd); + const snapshots = listSnapshots('.md', cwd).map(readSnapshot); + const identifiedSlugs = new Set( + snapshots + .filter((snapshot) => snapshotTargetIdentity(snapshot)) + .map((snapshot) => snapshot.meta.slug), + ); + const latestByTarget = new Map(); + for (const snapshot of snapshots) { + if (!snapshot?.meta.slug) continue; + // Slugs are lossy: distinct targets such as foo/bar and foo-bar can share + // one. Keep each known identity's latest open/closed state independent so + // closing one target cannot hide another target's live backlog. Once a + // slug has any identity-aware snapshot, its older legacy records are no + // longer independently routable and must not resurface as zombie work. + const targetIdentity = snapshotTargetIdentity(snapshot); + if (!targetIdentity && identifiedSlugs.has(snapshot.meta.slug)) continue; + const streamKey = targetIdentity || `slug:${snapshot.meta.slug}`; + latestByTarget.set(streamKey, snapshot); + } + return [...latestByTarget.values()] + .filter((snapshot) => snapshot.meta.closed !== true) + .sort((a, b) => a.path.localeCompare(b.path)) + .at(-1) || null; } /** @@ -153,9 +290,13 @@ export function readTrend(slug, { limit = 5, cwd = process.cwd() } = {}) { // Accept either a ready slug or a concrete target (path/URL) everywhere, so // callers never have to run the slug step separately. Anything containing a // path or URL marker is resolved through slugFromTarget. +function isReadySlug(value) { + return /^[a-z0-9-]+$/.test(value || '') && !value.includes('/'); +} + function coerceSlug(value) { if (!value) return null; - if (/^[a-z0-9-]+$/.test(value) && !value.includes('/')) return value; + if (isReadySlug(value)) return value; return slugFromTarget(value); } @@ -181,14 +322,124 @@ function main(argv) { if (metaArg) { try { meta = JSON.parse(metaArg); } catch { /* ignore */ } } + // The helper, not caller-provided metadata, owns the target fingerprint. + // This makes the snapshot describe the exact file bytes critique saw. + delete meta.target_fingerprint; + delete meta.target_path; + delete meta.target_identity; + const targetIdentity = resolveTargetIdentity(slugArg); + if (targetIdentity) meta.target_identity = targetIdentity; + const targetFingerprint = fingerprintTarget(slugArg); + if (targetFingerprint) { + meta.target_fingerprint = targetFingerprint; + meta.target_path = resolveLocalTargetPath(slugArg); + } const out = writeSnapshot({ slug, meta, body: raw }); process.stdout.write(`${out}\n`); return; } case 'latest': { - const latest = readLatestSnapshot(coerceSlug(args[0])); - if (!latest) { process.exit(2); } - process.stdout.write(latest.body); + const target = args[0]; + const format = args[1]; + const slug = coerceSlug(target); + if (!slug || (format && format !== '--json')) { + process.stderr.write('usage: latest [--json]\n'); + process.exit(1); + } + const targetFingerprint = fingerprintTarget(target); + const targetPath = resolveLocalTargetPath(target); + const targetIdentity = resolveTargetIdentity(target); + const readySlug = isReadySlug(target); + const newestForSlug = readNewestSnapshot(slug); + if (!newestForSlug) { process.exit(2); } + + // Concrete targets select the newest snapshot for their exact identity, + // not merely the newest filename for a lossy slug. This keeps distinct + // targets such as foo/bar and foo-bar from hiding each other's backlog. + const exactSnapshot = readNewestSnapshotForIdentity(slug, targetIdentity); + let latest = exactSnapshot; + if (!latest && !readySlug) { + // Legacy snapshots have no identity. Preserve their old explicit + // path/URL behavior only when no known target identity was selected. + latest = readNewestSnapshotForIdentity(slug, null); + } + if (!latest) latest = newestForSlug; + if (latest.meta.closed === true) { process.exit(2); } + + const recordedTargetPath = latest.meta.target_path; + const recordedTargetIdentity = snapshotTargetIdentity(latest); + const matchingIdentity = recordedTargetIdentity === targetIdentity; + + // Bare slugs remain a supported lookup mode, including for URL + // snapshots. But when a same-named local file exists, the request is + // ambiguous unless that exact file owns the snapshot identity. + if (readySlug && !recordedTargetIdentity) { + process.stderr.write( + 'ambiguous legacy snapshot target; use an explicit ./path or full URL\n', + ); + process.exit(2); + } + if (readySlug && targetPath && fs.existsSync(targetPath) && !matchingIdentity) { + process.stderr.write( + 'ambiguous snapshot slug; use an explicit ./path or remove the local name collision\n', + ); + process.exit(2); + } + + const concreteTarget = !readySlug || matchingIdentity; + if (concreteTarget && recordedTargetIdentity && !matchingIdentity) { + process.exit(2); + } + const concreteLocalTarget = concreteTarget && targetPath; + if (concreteLocalTarget && latest.meta.target_fingerprint !== targetFingerprint) { + closeSnapshot(latest.path); + process.exit(2); + } + if (format === '--json') { + process.stdout.write(JSON.stringify({ + snapshot_file: path.basename(latest.path), + body: latest.body, + }, null, 2) + '\n'); + } else { + process.stdout.write(latest.body); + } + return; + } + case 'close': { + const [slugArg, snapshotFile, ...extra] = args; + const slug = coerceSlug(slugArg); + if (!slug || !snapshotFile || extra.length > 0) { + process.stderr.write('usage: close \n'); + process.exit(1); + } + if ( + path.basename(snapshotFile) !== snapshotFile + || !SNAPSHOT_FILENAME.test(snapshotFile) + || !snapshotFile.endsWith(`__${slug}.md`) + ) process.exit(2); + + // A slug and filename are not enough to prove ownership because two + // distinct targets can normalize to the same slug. Modern snapshots + // carry a canonical identity, so require the supplied resolved target + // to match it before allowing the exact snapshot to be closed. Legacy + // snapshots without identity retain their historical close behavior. + const snapshotPath = path.join(getCritiqueDir(process.cwd()), snapshotFile); + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) process.exit(2); + snapshot = readSnapshot(snapshotPath); + } catch { + process.exit(2); + } + const recordedTargetIdentity = snapshotTargetIdentity(snapshot); + if ( + recordedTargetIdentity + && recordedTargetIdentity !== resolveTargetIdentity(slugArg) + ) process.exit(2); + + const closed = closeSnapshot(snapshotFile); + if (!closed) { process.exit(2); } + process.stdout.write(`${closed}\n`); return; } case 'trend': { @@ -197,7 +448,7 @@ function main(argv) { return; } default: - process.stderr.write('usage: critique-storage.mjs [args]\n'); + process.stderr.write('usage: critique-storage.mjs [args]\n'); process.exit(1); } } diff --git a/.gemini/skills/impeccable/reference/critique.md b/.gemini/skills/impeccable/reference/critique.md index a56b6dd6b..5d3a22a8c 100644 --- a/.gemini/skills/impeccable/reference/critique.md +++ b/.gemini/skills/impeccable/reference/critique.md @@ -1,6 +1,6 @@ ### Purpose -Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive/backlog for future commands. +Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive of that run. ### Hard Invariants @@ -84,7 +84,7 @@ After Assessment B returns usable CLI findings, reuse them. Do not rerun `detect Synthesize both assessments into a single report. Do NOT simply concatenate. Weave the findings together, noting where the LLM review and detector agree, where the detector caught issues the LLM missed, and where detector findings are false positives. -The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is only an archive/backlog for later commands. +The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is an archive of that run. Structure your feedback as a design director would: @@ -197,7 +197,7 @@ Skip this step if the Setup slug was null (vague or root-level target). IMPECCABLE_CRITIQUE_META='{"target":"","total_score":,"max_score":,"na_heuristics":"","p0_count":,"p1_count":}' \ node .gemini/skills/impeccable/scripts/critique-storage.mjs write "" ``` - `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. + `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. For a local file target, the helper also records an exact content fingerprint so polish can distinguish the assessed bytes from later edits without relying on Git state or timestamps. The helper prints the absolute path it wrote. Leave that file on disk. Polish closes it; this run does not. 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. diff --git a/.gemini/skills/impeccable/reference/polish.md b/.gemini/skills/impeccable/reference/polish.md index 877a83c84..e25ac153e 100644 --- a/.gemini/skills/impeccable/reference/polish.md +++ b/.gemini/skills/impeccable/reference/polish.md @@ -29,10 +29,10 @@ Use the feature yourself at the surface's representative sizes: desktop and mobi If a prior critique exists, use it as one input: ```bash -node .gemini/skills/impeccable/scripts/critique-storage.mjs latest "" +node .gemini/skills/impeccable/scripts/critique-storage.mjs latest "" --json ``` -Exit 0 returns the latest snapshot; incorporate relevant P0/P1 findings and name the snapshot read. Exit 2 means none exists. Perform an independent pass either way. +Exit 0 returns JSON with the latest snapshot's `body` and an exact `snapshot_file` identity. Retain `snapshot_file` until the end of the pass. For a local file target, the helper compares the file's exact current content fingerprint with the fingerprint captured by critique. Unchanged staged, unstaged, or untracked content remains current; any byte change, deletion, or replacement with a non-file closes the backlog it identified while preserving its trend history and exits 2. A URL target has no local fingerprint and remains current until explicitly closed. When current, incorporate relevant P0/P1 findings from `body` and name the snapshot read. Exit 2 means none exists or the target changed. Perform an independent pass either way. ## 3. Triage @@ -95,3 +95,11 @@ Walk the complete path again with mouse, keyboard, and touch where applicable. C Follow the quality guidance supplied by `context.mjs` and hooks, then run any other relevant QA commands. Context requests a manual scan only when no automatic detector is active; never add another detector pass. Fix real defects and document only narrow intentional exceptions. A clean scan does not replace visual judgment. Finish with a source diff: remove accidental churn, orphaned code, redundant values, and temporary artifacts. Ship only when the feature is functionally complete and consistently finished across the path. + +When this pass clears every Priority Issue it took from a snapshot, close that snapshot: + +```bash +node .gemini/skills/impeccable/scripts/critique-storage.mjs close "" "" +``` + +This closes only the snapshot this pass actually processed; if a newer critique landed meanwhile, its backlog stays live. Do not close when no snapshot was read, when `snapshot_file` was not retained, or when Priority Issues remain. diff --git a/.gemini/skills/impeccable/reference/routing.md b/.gemini/skills/impeccable/reference/routing.md index a09c24de9..890693c8c 100644 --- a/.gemini/skills/impeccable/reference/routing.md +++ b/.gemini/skills/impeccable/reference/routing.md @@ -8,7 +8,7 @@ Reason over the signals; there is no score to obey: - `setup.hasDesign` false while `setup.hasCode` true → `document` (capture the visual system). - `critique.latest` is `null` → the project has never been critiqued; for a set-up project with a real surface, offering `/impeccable critique ` is a strong default. -- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog), or re-run `critique` if the snapshot looks stale. +- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog and closes it when stale or cleared). - `git.changedFiles` pointing at one surface → scope `audit` or `polish` to those files specifically, naming them. - `devServer.running` true → `live` is available for in-browser iteration; if false, don't lead with `live`. **`live` and the bundled `detect.mjs` are web-only.** If `setup.platform` is `ios`, `android`, or `adaptive`, don't lead with either; the browser overlay and the HTML rule engine don't apply to native app code. - Otherwise group by intent (build new / improve what's there / iterate visually), tailored to the current surface and `setup.platform`. diff --git a/.gemini/skills/impeccable/scripts/critique-storage.mjs b/.gemini/skills/impeccable/scripts/critique-storage.mjs index f23fded37..86d297390 100644 --- a/.gemini/skills/impeccable/scripts/critique-storage.mjs +++ b/.gemini/skills/impeccable/scripts/critique-storage.mjs @@ -16,8 +16,9 @@ * CLI entry points (called from skill instructions): * node critique-storage.mjs slug * node critique-storage.mjs write - * node critique-storage.mjs latest + * node critique-storage.mjs latest [--json] * node critique-storage.mjs trend [limit] + * node critique-storage.mjs close * * 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 @@ -27,6 +28,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { createHash } from 'node:crypto'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { getCritiqueDir } from './lib/impeccable-paths.mjs'; import { slugFromTarget } from './lib/target-slug.mjs'; @@ -50,6 +52,45 @@ export function nowFilenameStamp(date = new Date()) { return iso.replace(/[:.]/g, '-').replace(/-\d+Z$/, 'Z'); } +/** + * Return an exact content fingerprint for a local file target. URLs and + * non-files return null because their content is not available here. + * + * The fingerprint deliberately describes bytes, not Git state or mtimes: + * critique often assesses an uncommitted file, and a later polish run should + * inherit that backlog when the bytes are unchanged regardless of staging. + */ +function resolveLocalTargetPath(target, { cwd = process.cwd() } = {}) { + if (!target || /^https?:\/\//i.test(target)) return null; + return path.isAbsolute(target) ? path.resolve(target) : path.resolve(cwd, target); +} + +function resolveTargetIdentity(target, { cwd = process.cwd() } = {}) { + if (!target || typeof target !== 'string') return null; + if (/^https?:\/\//i.test(target)) { + try { + const url = new URL(target); + const pathname = url.pathname.replace(/\/+$/, '') || '/'; + return `url:${url.origin}${pathname}`; + } catch { + return null; + } + } + const filePath = resolveLocalTargetPath(target, { cwd }); + return filePath ? `file:${filePath}` : null; +} + +export function fingerprintTarget(target, { cwd = process.cwd() } = {}) { + const filePath = resolveLocalTargetPath(target, { cwd }); + if (!filePath) return null; + try { + if (!fs.statSync(filePath).isFile()) return null; + return `sha256:${createHash('sha256').update(fs.readFileSync(filePath)).digest('hex')}`; + } catch { + return null; + } +} + /** * Write a snapshot for `slug`. `meta` carries the small structured frontmatter * keys read back by readTrend(). `body` is the human-readable critique @@ -62,14 +103,27 @@ export function writeSnapshot({ slug, meta, body, cwd = process.cwd(), now = new 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; + const contents = `${front}\n${body.trim()}\n`; + + // A second critique can finish in the same UTC second. Use exclusive + // creation and a fixed-width suffix so concurrent writers cannot replace + // history and lexical ordering still keeps collision entries newest. + for (let collision = 0; collision <= 9999; collision += 1) { + const suffix = collision === 0 ? '' : `~${String(collision).padStart(4, '0')}`; + const filePath = path.join(dir, `${timestamp}${suffix}__${slug}.md`); + try { + fs.writeFileSync(filePath, contents, { encoding: 'utf-8', flag: 'wx' }); + return filePath; + } catch (error) { + if (error?.code !== 'EEXIST') throw error; + } + } + throw new Error(`Too many critique snapshots for ${slug} at ${timestamp}`); } function serializeFrontmatter(obj) { @@ -98,6 +152,8 @@ function parseFrontmatter(text) { try { value = JSON.parse(value); } catch { /* leave as-is */ } } else if (/^-?\d+$/.test(value)) { value = Number(value); + } else if (value === 'true' || value === 'false') { + value = value === 'true'; } out[key] = value; } @@ -107,7 +163,7 @@ function parseFrontmatter(text) { /** * Return snapshot files matching `suffix`, sorted oldest → newest. */ -const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z__.+\.md$/; +const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z(?:~\d{4})?__.+\.md$/; function listSnapshots(suffix, cwd) { const dir = getCritiqueDir(cwd); @@ -118,24 +174,105 @@ function listSnapshots(suffix, cwd) { .map((f) => path.join(dir, f)); } -function readLatestSnapshotMatching(suffix, cwd) { - const filePath = listSnapshots(suffix, cwd).at(-1); +function readSnapshot(filePath) { if (!filePath) return null; const body = fs.readFileSync(filePath, 'utf-8'); return { path: filePath, body, meta: parseFrontmatter(body) }; } +function snapshotTargetIdentity(snapshot) { + const targetPath = snapshot?.meta.target_path; + return snapshot?.meta.target_identity + || (targetPath ? `file:${targetPath}` : null); +} + +function readNewestSnapshot(slug, { cwd = process.cwd() } = {}) { + return readSnapshot(listSnapshots(`__${slug}.md`, cwd).at(-1)); +} + +function readNewestSnapshotForIdentity( + slug, + targetIdentity, + { cwd = process.cwd() } = {}, +) { + const matches = listSnapshots(`__${slug}.md`, cwd) + .map(readSnapshot) + .filter((snapshot) => snapshotTargetIdentity(snapshot) === targetIdentity); + return matches.at(-1) || null; +} + /** * 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() } = {}) { - return readLatestSnapshotMatching(`__${slug}.md`, cwd); + const latest = readNewestSnapshot(slug, { cwd }); + return latest?.meta.closed === true ? null : latest; +} + +/** + * Mark one exact snapshot closed without deleting the score history consumed + * by `trend`. Exact identity matters: a newer critique may land after polish + * reads its backlog, and that newer snapshot must remain live. `snapshotFile` + * may be the absolute path returned by readLatestSnapshot() or the basename + * emitted by `latest --json`. Returns the path marked closed, or null. + */ +export function closeSnapshot(snapshotFile, { cwd = process.cwd() } = {}) { + if (!snapshotFile || typeof snapshotFile !== 'string') return null; + const dir = path.resolve(getCritiqueDir(cwd)); + const snapshotPath = path.isAbsolute(snapshotFile) + ? path.resolve(snapshotFile) + : path.resolve(dir, snapshotFile); + const filename = path.basename(snapshotPath); + if ( + path.dirname(snapshotPath) !== dir + || !SNAPSHOT_FILENAME.test(filename) + ) return null; + + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) return null; + snapshot = readSnapshot(snapshotPath); + } catch { + return null; + } + if (!snapshot || snapshot.meta.closed === true) return null; + const closedBody = snapshot.body.replace( + /^(---\r?\n[\s\S]*?)(\r?\n---)/, + '$1\nclosed: true$2', + ); + if (closedBody === snapshot.body) { + throw new Error(`Cannot close snapshot without frontmatter: ${snapshot.path}`); + } + fs.writeFileSync(snapshot.path, closedBody, 'utf-8'); + return snapshot.path; } /** Return the most recent snapshot across all targets, or null. */ export function readLatestSnapshotAcrossTargets({ cwd = process.cwd() } = {}) { - return readLatestSnapshotMatching('.md', cwd); + const snapshots = listSnapshots('.md', cwd).map(readSnapshot); + const identifiedSlugs = new Set( + snapshots + .filter((snapshot) => snapshotTargetIdentity(snapshot)) + .map((snapshot) => snapshot.meta.slug), + ); + const latestByTarget = new Map(); + for (const snapshot of snapshots) { + if (!snapshot?.meta.slug) continue; + // Slugs are lossy: distinct targets such as foo/bar and foo-bar can share + // one. Keep each known identity's latest open/closed state independent so + // closing one target cannot hide another target's live backlog. Once a + // slug has any identity-aware snapshot, its older legacy records are no + // longer independently routable and must not resurface as zombie work. + const targetIdentity = snapshotTargetIdentity(snapshot); + if (!targetIdentity && identifiedSlugs.has(snapshot.meta.slug)) continue; + const streamKey = targetIdentity || `slug:${snapshot.meta.slug}`; + latestByTarget.set(streamKey, snapshot); + } + return [...latestByTarget.values()] + .filter((snapshot) => snapshot.meta.closed !== true) + .sort((a, b) => a.path.localeCompare(b.path)) + .at(-1) || null; } /** @@ -153,9 +290,13 @@ export function readTrend(slug, { limit = 5, cwd = process.cwd() } = {}) { // Accept either a ready slug or a concrete target (path/URL) everywhere, so // callers never have to run the slug step separately. Anything containing a // path or URL marker is resolved through slugFromTarget. +function isReadySlug(value) { + return /^[a-z0-9-]+$/.test(value || '') && !value.includes('/'); +} + function coerceSlug(value) { if (!value) return null; - if (/^[a-z0-9-]+$/.test(value) && !value.includes('/')) return value; + if (isReadySlug(value)) return value; return slugFromTarget(value); } @@ -181,14 +322,124 @@ function main(argv) { if (metaArg) { try { meta = JSON.parse(metaArg); } catch { /* ignore */ } } + // The helper, not caller-provided metadata, owns the target fingerprint. + // This makes the snapshot describe the exact file bytes critique saw. + delete meta.target_fingerprint; + delete meta.target_path; + delete meta.target_identity; + const targetIdentity = resolveTargetIdentity(slugArg); + if (targetIdentity) meta.target_identity = targetIdentity; + const targetFingerprint = fingerprintTarget(slugArg); + if (targetFingerprint) { + meta.target_fingerprint = targetFingerprint; + meta.target_path = resolveLocalTargetPath(slugArg); + } const out = writeSnapshot({ slug, meta, body: raw }); process.stdout.write(`${out}\n`); return; } case 'latest': { - const latest = readLatestSnapshot(coerceSlug(args[0])); - if (!latest) { process.exit(2); } - process.stdout.write(latest.body); + const target = args[0]; + const format = args[1]; + const slug = coerceSlug(target); + if (!slug || (format && format !== '--json')) { + process.stderr.write('usage: latest [--json]\n'); + process.exit(1); + } + const targetFingerprint = fingerprintTarget(target); + const targetPath = resolveLocalTargetPath(target); + const targetIdentity = resolveTargetIdentity(target); + const readySlug = isReadySlug(target); + const newestForSlug = readNewestSnapshot(slug); + if (!newestForSlug) { process.exit(2); } + + // Concrete targets select the newest snapshot for their exact identity, + // not merely the newest filename for a lossy slug. This keeps distinct + // targets such as foo/bar and foo-bar from hiding each other's backlog. + const exactSnapshot = readNewestSnapshotForIdentity(slug, targetIdentity); + let latest = exactSnapshot; + if (!latest && !readySlug) { + // Legacy snapshots have no identity. Preserve their old explicit + // path/URL behavior only when no known target identity was selected. + latest = readNewestSnapshotForIdentity(slug, null); + } + if (!latest) latest = newestForSlug; + if (latest.meta.closed === true) { process.exit(2); } + + const recordedTargetPath = latest.meta.target_path; + const recordedTargetIdentity = snapshotTargetIdentity(latest); + const matchingIdentity = recordedTargetIdentity === targetIdentity; + + // Bare slugs remain a supported lookup mode, including for URL + // snapshots. But when a same-named local file exists, the request is + // ambiguous unless that exact file owns the snapshot identity. + if (readySlug && !recordedTargetIdentity) { + process.stderr.write( + 'ambiguous legacy snapshot target; use an explicit ./path or full URL\n', + ); + process.exit(2); + } + if (readySlug && targetPath && fs.existsSync(targetPath) && !matchingIdentity) { + process.stderr.write( + 'ambiguous snapshot slug; use an explicit ./path or remove the local name collision\n', + ); + process.exit(2); + } + + const concreteTarget = !readySlug || matchingIdentity; + if (concreteTarget && recordedTargetIdentity && !matchingIdentity) { + process.exit(2); + } + const concreteLocalTarget = concreteTarget && targetPath; + if (concreteLocalTarget && latest.meta.target_fingerprint !== targetFingerprint) { + closeSnapshot(latest.path); + process.exit(2); + } + if (format === '--json') { + process.stdout.write(JSON.stringify({ + snapshot_file: path.basename(latest.path), + body: latest.body, + }, null, 2) + '\n'); + } else { + process.stdout.write(latest.body); + } + return; + } + case 'close': { + const [slugArg, snapshotFile, ...extra] = args; + const slug = coerceSlug(slugArg); + if (!slug || !snapshotFile || extra.length > 0) { + process.stderr.write('usage: close \n'); + process.exit(1); + } + if ( + path.basename(snapshotFile) !== snapshotFile + || !SNAPSHOT_FILENAME.test(snapshotFile) + || !snapshotFile.endsWith(`__${slug}.md`) + ) process.exit(2); + + // A slug and filename are not enough to prove ownership because two + // distinct targets can normalize to the same slug. Modern snapshots + // carry a canonical identity, so require the supplied resolved target + // to match it before allowing the exact snapshot to be closed. Legacy + // snapshots without identity retain their historical close behavior. + const snapshotPath = path.join(getCritiqueDir(process.cwd()), snapshotFile); + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) process.exit(2); + snapshot = readSnapshot(snapshotPath); + } catch { + process.exit(2); + } + const recordedTargetIdentity = snapshotTargetIdentity(snapshot); + if ( + recordedTargetIdentity + && recordedTargetIdentity !== resolveTargetIdentity(slugArg) + ) process.exit(2); + + const closed = closeSnapshot(snapshotFile); + if (!closed) { process.exit(2); } + process.stdout.write(`${closed}\n`); return; } case 'trend': { @@ -197,7 +448,7 @@ function main(argv) { return; } default: - process.stderr.write('usage: critique-storage.mjs [args]\n'); + process.stderr.write('usage: critique-storage.mjs [args]\n'); process.exit(1); } } diff --git a/.github/skills/impeccable/reference/critique.md b/.github/skills/impeccable/reference/critique.md index 7038a321d..7eda2789f 100644 --- a/.github/skills/impeccable/reference/critique.md +++ b/.github/skills/impeccable/reference/critique.md @@ -1,6 +1,6 @@ ### Purpose -Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive/backlog for future commands. +Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive of that run. ### Hard Invariants @@ -84,7 +84,7 @@ After Assessment B returns usable CLI findings, reuse them. Do not rerun `detect Synthesize both assessments into a single report. Do NOT simply concatenate. Weave the findings together, noting where the LLM review and detector agree, where the detector caught issues the LLM missed, and where detector findings are false positives. -The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is only an archive/backlog for later commands. +The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is an archive of that run. Structure your feedback as a design director would: @@ -197,7 +197,7 @@ Skip this step if the Setup slug was null (vague or root-level target). IMPECCABLE_CRITIQUE_META='{"target":"","total_score":,"max_score":,"na_heuristics":"","p0_count":,"p1_count":}' \ node .github/skills/impeccable/scripts/critique-storage.mjs write "" ``` - `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. + `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. For a local file target, the helper also records an exact content fingerprint so polish can distinguish the assessed bytes from later edits without relying on Git state or timestamps. The helper prints the absolute path it wrote. Leave that file on disk. Polish closes it; this run does not. 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. diff --git a/.github/skills/impeccable/reference/polish.md b/.github/skills/impeccable/reference/polish.md index 0e115f981..3b857f52c 100644 --- a/.github/skills/impeccable/reference/polish.md +++ b/.github/skills/impeccable/reference/polish.md @@ -29,10 +29,10 @@ Use the feature yourself at the surface's representative sizes: desktop and mobi If a prior critique exists, use it as one input: ```bash -node .github/skills/impeccable/scripts/critique-storage.mjs latest "" +node .github/skills/impeccable/scripts/critique-storage.mjs latest "" --json ``` -Exit 0 returns the latest snapshot; incorporate relevant P0/P1 findings and name the snapshot read. Exit 2 means none exists. Perform an independent pass either way. +Exit 0 returns JSON with the latest snapshot's `body` and an exact `snapshot_file` identity. Retain `snapshot_file` until the end of the pass. For a local file target, the helper compares the file's exact current content fingerprint with the fingerprint captured by critique. Unchanged staged, unstaged, or untracked content remains current; any byte change, deletion, or replacement with a non-file closes the backlog it identified while preserving its trend history and exits 2. A URL target has no local fingerprint and remains current until explicitly closed. When current, incorporate relevant P0/P1 findings from `body` and name the snapshot read. Exit 2 means none exists or the target changed. Perform an independent pass either way. ## 3. Triage @@ -95,3 +95,11 @@ Walk the complete path again with mouse, keyboard, and touch where applicable. C Follow the quality guidance supplied by `context.mjs` and hooks, then run any other relevant QA commands. Context requests a manual scan only when no automatic detector is active; never add another detector pass. Fix real defects and document only narrow intentional exceptions. A clean scan does not replace visual judgment. Finish with a source diff: remove accidental churn, orphaned code, redundant values, and temporary artifacts. Ship only when the feature is functionally complete and consistently finished across the path. + +When this pass clears every Priority Issue it took from a snapshot, close that snapshot: + +```bash +node .github/skills/impeccable/scripts/critique-storage.mjs close "" "" +``` + +This closes only the snapshot this pass actually processed; if a newer critique landed meanwhile, its backlog stays live. Do not close when no snapshot was read, when `snapshot_file` was not retained, or when Priority Issues remain. diff --git a/.github/skills/impeccable/reference/routing.md b/.github/skills/impeccable/reference/routing.md index 7099b1db8..bfc064188 100644 --- a/.github/skills/impeccable/reference/routing.md +++ b/.github/skills/impeccable/reference/routing.md @@ -8,7 +8,7 @@ Reason over the signals; there is no score to obey: - `setup.hasDesign` false while `setup.hasCode` true → `document` (capture the visual system). - `critique.latest` is `null` → the project has never been critiqued; for a set-up project with a real surface, offering `/impeccable critique ` is a strong default. -- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog), or re-run `critique` if the snapshot looks stale. +- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog and closes it when stale or cleared). - `git.changedFiles` pointing at one surface → scope `audit` or `polish` to those files specifically, naming them. - `devServer.running` true → `live` is available for in-browser iteration; if false, don't lead with `live`. **`live` and the bundled `detect.mjs` are web-only.** If `setup.platform` is `ios`, `android`, or `adaptive`, don't lead with either; the browser overlay and the HTML rule engine don't apply to native app code. - Otherwise group by intent (build new / improve what's there / iterate visually), tailored to the current surface and `setup.platform`. diff --git a/.github/skills/impeccable/scripts/critique-storage.mjs b/.github/skills/impeccable/scripts/critique-storage.mjs index f23fded37..86d297390 100644 --- a/.github/skills/impeccable/scripts/critique-storage.mjs +++ b/.github/skills/impeccable/scripts/critique-storage.mjs @@ -16,8 +16,9 @@ * CLI entry points (called from skill instructions): * node critique-storage.mjs slug * node critique-storage.mjs write - * node critique-storage.mjs latest + * node critique-storage.mjs latest [--json] * node critique-storage.mjs trend [limit] + * node critique-storage.mjs close * * 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 @@ -27,6 +28,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { createHash } from 'node:crypto'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { getCritiqueDir } from './lib/impeccable-paths.mjs'; import { slugFromTarget } from './lib/target-slug.mjs'; @@ -50,6 +52,45 @@ export function nowFilenameStamp(date = new Date()) { return iso.replace(/[:.]/g, '-').replace(/-\d+Z$/, 'Z'); } +/** + * Return an exact content fingerprint for a local file target. URLs and + * non-files return null because their content is not available here. + * + * The fingerprint deliberately describes bytes, not Git state or mtimes: + * critique often assesses an uncommitted file, and a later polish run should + * inherit that backlog when the bytes are unchanged regardless of staging. + */ +function resolveLocalTargetPath(target, { cwd = process.cwd() } = {}) { + if (!target || /^https?:\/\//i.test(target)) return null; + return path.isAbsolute(target) ? path.resolve(target) : path.resolve(cwd, target); +} + +function resolveTargetIdentity(target, { cwd = process.cwd() } = {}) { + if (!target || typeof target !== 'string') return null; + if (/^https?:\/\//i.test(target)) { + try { + const url = new URL(target); + const pathname = url.pathname.replace(/\/+$/, '') || '/'; + return `url:${url.origin}${pathname}`; + } catch { + return null; + } + } + const filePath = resolveLocalTargetPath(target, { cwd }); + return filePath ? `file:${filePath}` : null; +} + +export function fingerprintTarget(target, { cwd = process.cwd() } = {}) { + const filePath = resolveLocalTargetPath(target, { cwd }); + if (!filePath) return null; + try { + if (!fs.statSync(filePath).isFile()) return null; + return `sha256:${createHash('sha256').update(fs.readFileSync(filePath)).digest('hex')}`; + } catch { + return null; + } +} + /** * Write a snapshot for `slug`. `meta` carries the small structured frontmatter * keys read back by readTrend(). `body` is the human-readable critique @@ -62,14 +103,27 @@ export function writeSnapshot({ slug, meta, body, cwd = process.cwd(), now = new 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; + const contents = `${front}\n${body.trim()}\n`; + + // A second critique can finish in the same UTC second. Use exclusive + // creation and a fixed-width suffix so concurrent writers cannot replace + // history and lexical ordering still keeps collision entries newest. + for (let collision = 0; collision <= 9999; collision += 1) { + const suffix = collision === 0 ? '' : `~${String(collision).padStart(4, '0')}`; + const filePath = path.join(dir, `${timestamp}${suffix}__${slug}.md`); + try { + fs.writeFileSync(filePath, contents, { encoding: 'utf-8', flag: 'wx' }); + return filePath; + } catch (error) { + if (error?.code !== 'EEXIST') throw error; + } + } + throw new Error(`Too many critique snapshots for ${slug} at ${timestamp}`); } function serializeFrontmatter(obj) { @@ -98,6 +152,8 @@ function parseFrontmatter(text) { try { value = JSON.parse(value); } catch { /* leave as-is */ } } else if (/^-?\d+$/.test(value)) { value = Number(value); + } else if (value === 'true' || value === 'false') { + value = value === 'true'; } out[key] = value; } @@ -107,7 +163,7 @@ function parseFrontmatter(text) { /** * Return snapshot files matching `suffix`, sorted oldest → newest. */ -const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z__.+\.md$/; +const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z(?:~\d{4})?__.+\.md$/; function listSnapshots(suffix, cwd) { const dir = getCritiqueDir(cwd); @@ -118,24 +174,105 @@ function listSnapshots(suffix, cwd) { .map((f) => path.join(dir, f)); } -function readLatestSnapshotMatching(suffix, cwd) { - const filePath = listSnapshots(suffix, cwd).at(-1); +function readSnapshot(filePath) { if (!filePath) return null; const body = fs.readFileSync(filePath, 'utf-8'); return { path: filePath, body, meta: parseFrontmatter(body) }; } +function snapshotTargetIdentity(snapshot) { + const targetPath = snapshot?.meta.target_path; + return snapshot?.meta.target_identity + || (targetPath ? `file:${targetPath}` : null); +} + +function readNewestSnapshot(slug, { cwd = process.cwd() } = {}) { + return readSnapshot(listSnapshots(`__${slug}.md`, cwd).at(-1)); +} + +function readNewestSnapshotForIdentity( + slug, + targetIdentity, + { cwd = process.cwd() } = {}, +) { + const matches = listSnapshots(`__${slug}.md`, cwd) + .map(readSnapshot) + .filter((snapshot) => snapshotTargetIdentity(snapshot) === targetIdentity); + return matches.at(-1) || null; +} + /** * 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() } = {}) { - return readLatestSnapshotMatching(`__${slug}.md`, cwd); + const latest = readNewestSnapshot(slug, { cwd }); + return latest?.meta.closed === true ? null : latest; +} + +/** + * Mark one exact snapshot closed without deleting the score history consumed + * by `trend`. Exact identity matters: a newer critique may land after polish + * reads its backlog, and that newer snapshot must remain live. `snapshotFile` + * may be the absolute path returned by readLatestSnapshot() or the basename + * emitted by `latest --json`. Returns the path marked closed, or null. + */ +export function closeSnapshot(snapshotFile, { cwd = process.cwd() } = {}) { + if (!snapshotFile || typeof snapshotFile !== 'string') return null; + const dir = path.resolve(getCritiqueDir(cwd)); + const snapshotPath = path.isAbsolute(snapshotFile) + ? path.resolve(snapshotFile) + : path.resolve(dir, snapshotFile); + const filename = path.basename(snapshotPath); + if ( + path.dirname(snapshotPath) !== dir + || !SNAPSHOT_FILENAME.test(filename) + ) return null; + + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) return null; + snapshot = readSnapshot(snapshotPath); + } catch { + return null; + } + if (!snapshot || snapshot.meta.closed === true) return null; + const closedBody = snapshot.body.replace( + /^(---\r?\n[\s\S]*?)(\r?\n---)/, + '$1\nclosed: true$2', + ); + if (closedBody === snapshot.body) { + throw new Error(`Cannot close snapshot without frontmatter: ${snapshot.path}`); + } + fs.writeFileSync(snapshot.path, closedBody, 'utf-8'); + return snapshot.path; } /** Return the most recent snapshot across all targets, or null. */ export function readLatestSnapshotAcrossTargets({ cwd = process.cwd() } = {}) { - return readLatestSnapshotMatching('.md', cwd); + const snapshots = listSnapshots('.md', cwd).map(readSnapshot); + const identifiedSlugs = new Set( + snapshots + .filter((snapshot) => snapshotTargetIdentity(snapshot)) + .map((snapshot) => snapshot.meta.slug), + ); + const latestByTarget = new Map(); + for (const snapshot of snapshots) { + if (!snapshot?.meta.slug) continue; + // Slugs are lossy: distinct targets such as foo/bar and foo-bar can share + // one. Keep each known identity's latest open/closed state independent so + // closing one target cannot hide another target's live backlog. Once a + // slug has any identity-aware snapshot, its older legacy records are no + // longer independently routable and must not resurface as zombie work. + const targetIdentity = snapshotTargetIdentity(snapshot); + if (!targetIdentity && identifiedSlugs.has(snapshot.meta.slug)) continue; + const streamKey = targetIdentity || `slug:${snapshot.meta.slug}`; + latestByTarget.set(streamKey, snapshot); + } + return [...latestByTarget.values()] + .filter((snapshot) => snapshot.meta.closed !== true) + .sort((a, b) => a.path.localeCompare(b.path)) + .at(-1) || null; } /** @@ -153,9 +290,13 @@ export function readTrend(slug, { limit = 5, cwd = process.cwd() } = {}) { // Accept either a ready slug or a concrete target (path/URL) everywhere, so // callers never have to run the slug step separately. Anything containing a // path or URL marker is resolved through slugFromTarget. +function isReadySlug(value) { + return /^[a-z0-9-]+$/.test(value || '') && !value.includes('/'); +} + function coerceSlug(value) { if (!value) return null; - if (/^[a-z0-9-]+$/.test(value) && !value.includes('/')) return value; + if (isReadySlug(value)) return value; return slugFromTarget(value); } @@ -181,14 +322,124 @@ function main(argv) { if (metaArg) { try { meta = JSON.parse(metaArg); } catch { /* ignore */ } } + // The helper, not caller-provided metadata, owns the target fingerprint. + // This makes the snapshot describe the exact file bytes critique saw. + delete meta.target_fingerprint; + delete meta.target_path; + delete meta.target_identity; + const targetIdentity = resolveTargetIdentity(slugArg); + if (targetIdentity) meta.target_identity = targetIdentity; + const targetFingerprint = fingerprintTarget(slugArg); + if (targetFingerprint) { + meta.target_fingerprint = targetFingerprint; + meta.target_path = resolveLocalTargetPath(slugArg); + } const out = writeSnapshot({ slug, meta, body: raw }); process.stdout.write(`${out}\n`); return; } case 'latest': { - const latest = readLatestSnapshot(coerceSlug(args[0])); - if (!latest) { process.exit(2); } - process.stdout.write(latest.body); + const target = args[0]; + const format = args[1]; + const slug = coerceSlug(target); + if (!slug || (format && format !== '--json')) { + process.stderr.write('usage: latest [--json]\n'); + process.exit(1); + } + const targetFingerprint = fingerprintTarget(target); + const targetPath = resolveLocalTargetPath(target); + const targetIdentity = resolveTargetIdentity(target); + const readySlug = isReadySlug(target); + const newestForSlug = readNewestSnapshot(slug); + if (!newestForSlug) { process.exit(2); } + + // Concrete targets select the newest snapshot for their exact identity, + // not merely the newest filename for a lossy slug. This keeps distinct + // targets such as foo/bar and foo-bar from hiding each other's backlog. + const exactSnapshot = readNewestSnapshotForIdentity(slug, targetIdentity); + let latest = exactSnapshot; + if (!latest && !readySlug) { + // Legacy snapshots have no identity. Preserve their old explicit + // path/URL behavior only when no known target identity was selected. + latest = readNewestSnapshotForIdentity(slug, null); + } + if (!latest) latest = newestForSlug; + if (latest.meta.closed === true) { process.exit(2); } + + const recordedTargetPath = latest.meta.target_path; + const recordedTargetIdentity = snapshotTargetIdentity(latest); + const matchingIdentity = recordedTargetIdentity === targetIdentity; + + // Bare slugs remain a supported lookup mode, including for URL + // snapshots. But when a same-named local file exists, the request is + // ambiguous unless that exact file owns the snapshot identity. + if (readySlug && !recordedTargetIdentity) { + process.stderr.write( + 'ambiguous legacy snapshot target; use an explicit ./path or full URL\n', + ); + process.exit(2); + } + if (readySlug && targetPath && fs.existsSync(targetPath) && !matchingIdentity) { + process.stderr.write( + 'ambiguous snapshot slug; use an explicit ./path or remove the local name collision\n', + ); + process.exit(2); + } + + const concreteTarget = !readySlug || matchingIdentity; + if (concreteTarget && recordedTargetIdentity && !matchingIdentity) { + process.exit(2); + } + const concreteLocalTarget = concreteTarget && targetPath; + if (concreteLocalTarget && latest.meta.target_fingerprint !== targetFingerprint) { + closeSnapshot(latest.path); + process.exit(2); + } + if (format === '--json') { + process.stdout.write(JSON.stringify({ + snapshot_file: path.basename(latest.path), + body: latest.body, + }, null, 2) + '\n'); + } else { + process.stdout.write(latest.body); + } + return; + } + case 'close': { + const [slugArg, snapshotFile, ...extra] = args; + const slug = coerceSlug(slugArg); + if (!slug || !snapshotFile || extra.length > 0) { + process.stderr.write('usage: close \n'); + process.exit(1); + } + if ( + path.basename(snapshotFile) !== snapshotFile + || !SNAPSHOT_FILENAME.test(snapshotFile) + || !snapshotFile.endsWith(`__${slug}.md`) + ) process.exit(2); + + // A slug and filename are not enough to prove ownership because two + // distinct targets can normalize to the same slug. Modern snapshots + // carry a canonical identity, so require the supplied resolved target + // to match it before allowing the exact snapshot to be closed. Legacy + // snapshots without identity retain their historical close behavior. + const snapshotPath = path.join(getCritiqueDir(process.cwd()), snapshotFile); + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) process.exit(2); + snapshot = readSnapshot(snapshotPath); + } catch { + process.exit(2); + } + const recordedTargetIdentity = snapshotTargetIdentity(snapshot); + if ( + recordedTargetIdentity + && recordedTargetIdentity !== resolveTargetIdentity(slugArg) + ) process.exit(2); + + const closed = closeSnapshot(snapshotFile); + if (!closed) { process.exit(2); } + process.stdout.write(`${closed}\n`); return; } case 'trend': { @@ -197,7 +448,7 @@ function main(argv) { return; } default: - process.stderr.write('usage: critique-storage.mjs [args]\n'); + process.stderr.write('usage: critique-storage.mjs [args]\n'); process.exit(1); } } diff --git a/.grok/skills/impeccable/reference/critique.md b/.grok/skills/impeccable/reference/critique.md index 4da7a7a72..cdba748fb 100644 --- a/.grok/skills/impeccable/reference/critique.md +++ b/.grok/skills/impeccable/reference/critique.md @@ -1,6 +1,6 @@ ### Purpose -Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive/backlog for future commands. +Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive of that run. ### Hard Invariants @@ -84,7 +84,7 @@ After Assessment B returns usable CLI findings, reuse them. Do not rerun `detect Synthesize both assessments into a single report. Do NOT simply concatenate. Weave the findings together, noting where the LLM review and detector agree, where the detector caught issues the LLM missed, and where detector findings are false positives. -The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is only an archive/backlog for later commands. +The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is an archive of that run. Structure your feedback as a design director would: @@ -197,7 +197,7 @@ Skip this step if the Setup slug was null (vague or root-level target). IMPECCABLE_CRITIQUE_META='{"target":"","total_score":,"max_score":,"na_heuristics":"","p0_count":,"p1_count":}' \ node .grok/skills/impeccable/scripts/critique-storage.mjs write "" ``` - `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. + `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. For a local file target, the helper also records an exact content fingerprint so polish can distinguish the assessed bytes from later edits without relying on Git state or timestamps. The helper prints the absolute path it wrote. Leave that file on disk. Polish closes it; this run does not. 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. diff --git a/.grok/skills/impeccable/reference/polish.md b/.grok/skills/impeccable/reference/polish.md index e5ff9c4b5..59f33805d 100644 --- a/.grok/skills/impeccable/reference/polish.md +++ b/.grok/skills/impeccable/reference/polish.md @@ -29,10 +29,10 @@ Use the feature yourself at the surface's representative sizes: desktop and mobi If a prior critique exists, use it as one input: ```bash -node .grok/skills/impeccable/scripts/critique-storage.mjs latest "" +node .grok/skills/impeccable/scripts/critique-storage.mjs latest "" --json ``` -Exit 0 returns the latest snapshot; incorporate relevant P0/P1 findings and name the snapshot read. Exit 2 means none exists. Perform an independent pass either way. +Exit 0 returns JSON with the latest snapshot's `body` and an exact `snapshot_file` identity. Retain `snapshot_file` until the end of the pass. For a local file target, the helper compares the file's exact current content fingerprint with the fingerprint captured by critique. Unchanged staged, unstaged, or untracked content remains current; any byte change, deletion, or replacement with a non-file closes the backlog it identified while preserving its trend history and exits 2. A URL target has no local fingerprint and remains current until explicitly closed. When current, incorporate relevant P0/P1 findings from `body` and name the snapshot read. Exit 2 means none exists or the target changed. Perform an independent pass either way. ## 3. Triage @@ -95,3 +95,11 @@ Walk the complete path again with mouse, keyboard, and touch where applicable. C Follow the quality guidance supplied by `context.mjs` and hooks, then run any other relevant QA commands. Context requests a manual scan only when no automatic detector is active; never add another detector pass. Fix real defects and document only narrow intentional exceptions. A clean scan does not replace visual judgment. Finish with a source diff: remove accidental churn, orphaned code, redundant values, and temporary artifacts. Ship only when the feature is functionally complete and consistently finished across the path. + +When this pass clears every Priority Issue it took from a snapshot, close that snapshot: + +```bash +node .grok/skills/impeccable/scripts/critique-storage.mjs close "" "" +``` + +This closes only the snapshot this pass actually processed; if a newer critique landed meanwhile, its backlog stays live. Do not close when no snapshot was read, when `snapshot_file` was not retained, or when Priority Issues remain. diff --git a/.grok/skills/impeccable/reference/routing.md b/.grok/skills/impeccable/reference/routing.md index 4878532c5..e2f8f3617 100644 --- a/.grok/skills/impeccable/reference/routing.md +++ b/.grok/skills/impeccable/reference/routing.md @@ -8,7 +8,7 @@ Reason over the signals; there is no score to obey: - `setup.hasDesign` false while `setup.hasCode` true → `document` (capture the visual system). - `critique.latest` is `null` → the project has never been critiqued; for a set-up project with a real surface, offering `/impeccable critique ` is a strong default. -- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog), or re-run `critique` if the snapshot looks stale. +- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog and closes it when stale or cleared). - `git.changedFiles` pointing at one surface → scope `audit` or `polish` to those files specifically, naming them. - `devServer.running` true → `live` is available for in-browser iteration; if false, don't lead with `live`. **`live` and the bundled `detect.mjs` are web-only.** If `setup.platform` is `ios`, `android`, or `adaptive`, don't lead with either; the browser overlay and the HTML rule engine don't apply to native app code. - Otherwise group by intent (build new / improve what's there / iterate visually), tailored to the current surface and `setup.platform`. diff --git a/.grok/skills/impeccable/scripts/critique-storage.mjs b/.grok/skills/impeccable/scripts/critique-storage.mjs index f23fded37..86d297390 100644 --- a/.grok/skills/impeccable/scripts/critique-storage.mjs +++ b/.grok/skills/impeccable/scripts/critique-storage.mjs @@ -16,8 +16,9 @@ * CLI entry points (called from skill instructions): * node critique-storage.mjs slug * node critique-storage.mjs write - * node critique-storage.mjs latest + * node critique-storage.mjs latest [--json] * node critique-storage.mjs trend [limit] + * node critique-storage.mjs close * * 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 @@ -27,6 +28,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { createHash } from 'node:crypto'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { getCritiqueDir } from './lib/impeccable-paths.mjs'; import { slugFromTarget } from './lib/target-slug.mjs'; @@ -50,6 +52,45 @@ export function nowFilenameStamp(date = new Date()) { return iso.replace(/[:.]/g, '-').replace(/-\d+Z$/, 'Z'); } +/** + * Return an exact content fingerprint for a local file target. URLs and + * non-files return null because their content is not available here. + * + * The fingerprint deliberately describes bytes, not Git state or mtimes: + * critique often assesses an uncommitted file, and a later polish run should + * inherit that backlog when the bytes are unchanged regardless of staging. + */ +function resolveLocalTargetPath(target, { cwd = process.cwd() } = {}) { + if (!target || /^https?:\/\//i.test(target)) return null; + return path.isAbsolute(target) ? path.resolve(target) : path.resolve(cwd, target); +} + +function resolveTargetIdentity(target, { cwd = process.cwd() } = {}) { + if (!target || typeof target !== 'string') return null; + if (/^https?:\/\//i.test(target)) { + try { + const url = new URL(target); + const pathname = url.pathname.replace(/\/+$/, '') || '/'; + return `url:${url.origin}${pathname}`; + } catch { + return null; + } + } + const filePath = resolveLocalTargetPath(target, { cwd }); + return filePath ? `file:${filePath}` : null; +} + +export function fingerprintTarget(target, { cwd = process.cwd() } = {}) { + const filePath = resolveLocalTargetPath(target, { cwd }); + if (!filePath) return null; + try { + if (!fs.statSync(filePath).isFile()) return null; + return `sha256:${createHash('sha256').update(fs.readFileSync(filePath)).digest('hex')}`; + } catch { + return null; + } +} + /** * Write a snapshot for `slug`. `meta` carries the small structured frontmatter * keys read back by readTrend(). `body` is the human-readable critique @@ -62,14 +103,27 @@ export function writeSnapshot({ slug, meta, body, cwd = process.cwd(), now = new 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; + const contents = `${front}\n${body.trim()}\n`; + + // A second critique can finish in the same UTC second. Use exclusive + // creation and a fixed-width suffix so concurrent writers cannot replace + // history and lexical ordering still keeps collision entries newest. + for (let collision = 0; collision <= 9999; collision += 1) { + const suffix = collision === 0 ? '' : `~${String(collision).padStart(4, '0')}`; + const filePath = path.join(dir, `${timestamp}${suffix}__${slug}.md`); + try { + fs.writeFileSync(filePath, contents, { encoding: 'utf-8', flag: 'wx' }); + return filePath; + } catch (error) { + if (error?.code !== 'EEXIST') throw error; + } + } + throw new Error(`Too many critique snapshots for ${slug} at ${timestamp}`); } function serializeFrontmatter(obj) { @@ -98,6 +152,8 @@ function parseFrontmatter(text) { try { value = JSON.parse(value); } catch { /* leave as-is */ } } else if (/^-?\d+$/.test(value)) { value = Number(value); + } else if (value === 'true' || value === 'false') { + value = value === 'true'; } out[key] = value; } @@ -107,7 +163,7 @@ function parseFrontmatter(text) { /** * Return snapshot files matching `suffix`, sorted oldest → newest. */ -const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z__.+\.md$/; +const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z(?:~\d{4})?__.+\.md$/; function listSnapshots(suffix, cwd) { const dir = getCritiqueDir(cwd); @@ -118,24 +174,105 @@ function listSnapshots(suffix, cwd) { .map((f) => path.join(dir, f)); } -function readLatestSnapshotMatching(suffix, cwd) { - const filePath = listSnapshots(suffix, cwd).at(-1); +function readSnapshot(filePath) { if (!filePath) return null; const body = fs.readFileSync(filePath, 'utf-8'); return { path: filePath, body, meta: parseFrontmatter(body) }; } +function snapshotTargetIdentity(snapshot) { + const targetPath = snapshot?.meta.target_path; + return snapshot?.meta.target_identity + || (targetPath ? `file:${targetPath}` : null); +} + +function readNewestSnapshot(slug, { cwd = process.cwd() } = {}) { + return readSnapshot(listSnapshots(`__${slug}.md`, cwd).at(-1)); +} + +function readNewestSnapshotForIdentity( + slug, + targetIdentity, + { cwd = process.cwd() } = {}, +) { + const matches = listSnapshots(`__${slug}.md`, cwd) + .map(readSnapshot) + .filter((snapshot) => snapshotTargetIdentity(snapshot) === targetIdentity); + return matches.at(-1) || null; +} + /** * 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() } = {}) { - return readLatestSnapshotMatching(`__${slug}.md`, cwd); + const latest = readNewestSnapshot(slug, { cwd }); + return latest?.meta.closed === true ? null : latest; +} + +/** + * Mark one exact snapshot closed without deleting the score history consumed + * by `trend`. Exact identity matters: a newer critique may land after polish + * reads its backlog, and that newer snapshot must remain live. `snapshotFile` + * may be the absolute path returned by readLatestSnapshot() or the basename + * emitted by `latest --json`. Returns the path marked closed, or null. + */ +export function closeSnapshot(snapshotFile, { cwd = process.cwd() } = {}) { + if (!snapshotFile || typeof snapshotFile !== 'string') return null; + const dir = path.resolve(getCritiqueDir(cwd)); + const snapshotPath = path.isAbsolute(snapshotFile) + ? path.resolve(snapshotFile) + : path.resolve(dir, snapshotFile); + const filename = path.basename(snapshotPath); + if ( + path.dirname(snapshotPath) !== dir + || !SNAPSHOT_FILENAME.test(filename) + ) return null; + + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) return null; + snapshot = readSnapshot(snapshotPath); + } catch { + return null; + } + if (!snapshot || snapshot.meta.closed === true) return null; + const closedBody = snapshot.body.replace( + /^(---\r?\n[\s\S]*?)(\r?\n---)/, + '$1\nclosed: true$2', + ); + if (closedBody === snapshot.body) { + throw new Error(`Cannot close snapshot without frontmatter: ${snapshot.path}`); + } + fs.writeFileSync(snapshot.path, closedBody, 'utf-8'); + return snapshot.path; } /** Return the most recent snapshot across all targets, or null. */ export function readLatestSnapshotAcrossTargets({ cwd = process.cwd() } = {}) { - return readLatestSnapshotMatching('.md', cwd); + const snapshots = listSnapshots('.md', cwd).map(readSnapshot); + const identifiedSlugs = new Set( + snapshots + .filter((snapshot) => snapshotTargetIdentity(snapshot)) + .map((snapshot) => snapshot.meta.slug), + ); + const latestByTarget = new Map(); + for (const snapshot of snapshots) { + if (!snapshot?.meta.slug) continue; + // Slugs are lossy: distinct targets such as foo/bar and foo-bar can share + // one. Keep each known identity's latest open/closed state independent so + // closing one target cannot hide another target's live backlog. Once a + // slug has any identity-aware snapshot, its older legacy records are no + // longer independently routable and must not resurface as zombie work. + const targetIdentity = snapshotTargetIdentity(snapshot); + if (!targetIdentity && identifiedSlugs.has(snapshot.meta.slug)) continue; + const streamKey = targetIdentity || `slug:${snapshot.meta.slug}`; + latestByTarget.set(streamKey, snapshot); + } + return [...latestByTarget.values()] + .filter((snapshot) => snapshot.meta.closed !== true) + .sort((a, b) => a.path.localeCompare(b.path)) + .at(-1) || null; } /** @@ -153,9 +290,13 @@ export function readTrend(slug, { limit = 5, cwd = process.cwd() } = {}) { // Accept either a ready slug or a concrete target (path/URL) everywhere, so // callers never have to run the slug step separately. Anything containing a // path or URL marker is resolved through slugFromTarget. +function isReadySlug(value) { + return /^[a-z0-9-]+$/.test(value || '') && !value.includes('/'); +} + function coerceSlug(value) { if (!value) return null; - if (/^[a-z0-9-]+$/.test(value) && !value.includes('/')) return value; + if (isReadySlug(value)) return value; return slugFromTarget(value); } @@ -181,14 +322,124 @@ function main(argv) { if (metaArg) { try { meta = JSON.parse(metaArg); } catch { /* ignore */ } } + // The helper, not caller-provided metadata, owns the target fingerprint. + // This makes the snapshot describe the exact file bytes critique saw. + delete meta.target_fingerprint; + delete meta.target_path; + delete meta.target_identity; + const targetIdentity = resolveTargetIdentity(slugArg); + if (targetIdentity) meta.target_identity = targetIdentity; + const targetFingerprint = fingerprintTarget(slugArg); + if (targetFingerprint) { + meta.target_fingerprint = targetFingerprint; + meta.target_path = resolveLocalTargetPath(slugArg); + } const out = writeSnapshot({ slug, meta, body: raw }); process.stdout.write(`${out}\n`); return; } case 'latest': { - const latest = readLatestSnapshot(coerceSlug(args[0])); - if (!latest) { process.exit(2); } - process.stdout.write(latest.body); + const target = args[0]; + const format = args[1]; + const slug = coerceSlug(target); + if (!slug || (format && format !== '--json')) { + process.stderr.write('usage: latest [--json]\n'); + process.exit(1); + } + const targetFingerprint = fingerprintTarget(target); + const targetPath = resolveLocalTargetPath(target); + const targetIdentity = resolveTargetIdentity(target); + const readySlug = isReadySlug(target); + const newestForSlug = readNewestSnapshot(slug); + if (!newestForSlug) { process.exit(2); } + + // Concrete targets select the newest snapshot for their exact identity, + // not merely the newest filename for a lossy slug. This keeps distinct + // targets such as foo/bar and foo-bar from hiding each other's backlog. + const exactSnapshot = readNewestSnapshotForIdentity(slug, targetIdentity); + let latest = exactSnapshot; + if (!latest && !readySlug) { + // Legacy snapshots have no identity. Preserve their old explicit + // path/URL behavior only when no known target identity was selected. + latest = readNewestSnapshotForIdentity(slug, null); + } + if (!latest) latest = newestForSlug; + if (latest.meta.closed === true) { process.exit(2); } + + const recordedTargetPath = latest.meta.target_path; + const recordedTargetIdentity = snapshotTargetIdentity(latest); + const matchingIdentity = recordedTargetIdentity === targetIdentity; + + // Bare slugs remain a supported lookup mode, including for URL + // snapshots. But when a same-named local file exists, the request is + // ambiguous unless that exact file owns the snapshot identity. + if (readySlug && !recordedTargetIdentity) { + process.stderr.write( + 'ambiguous legacy snapshot target; use an explicit ./path or full URL\n', + ); + process.exit(2); + } + if (readySlug && targetPath && fs.existsSync(targetPath) && !matchingIdentity) { + process.stderr.write( + 'ambiguous snapshot slug; use an explicit ./path or remove the local name collision\n', + ); + process.exit(2); + } + + const concreteTarget = !readySlug || matchingIdentity; + if (concreteTarget && recordedTargetIdentity && !matchingIdentity) { + process.exit(2); + } + const concreteLocalTarget = concreteTarget && targetPath; + if (concreteLocalTarget && latest.meta.target_fingerprint !== targetFingerprint) { + closeSnapshot(latest.path); + process.exit(2); + } + if (format === '--json') { + process.stdout.write(JSON.stringify({ + snapshot_file: path.basename(latest.path), + body: latest.body, + }, null, 2) + '\n'); + } else { + process.stdout.write(latest.body); + } + return; + } + case 'close': { + const [slugArg, snapshotFile, ...extra] = args; + const slug = coerceSlug(slugArg); + if (!slug || !snapshotFile || extra.length > 0) { + process.stderr.write('usage: close \n'); + process.exit(1); + } + if ( + path.basename(snapshotFile) !== snapshotFile + || !SNAPSHOT_FILENAME.test(snapshotFile) + || !snapshotFile.endsWith(`__${slug}.md`) + ) process.exit(2); + + // A slug and filename are not enough to prove ownership because two + // distinct targets can normalize to the same slug. Modern snapshots + // carry a canonical identity, so require the supplied resolved target + // to match it before allowing the exact snapshot to be closed. Legacy + // snapshots without identity retain their historical close behavior. + const snapshotPath = path.join(getCritiqueDir(process.cwd()), snapshotFile); + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) process.exit(2); + snapshot = readSnapshot(snapshotPath); + } catch { + process.exit(2); + } + const recordedTargetIdentity = snapshotTargetIdentity(snapshot); + if ( + recordedTargetIdentity + && recordedTargetIdentity !== resolveTargetIdentity(slugArg) + ) process.exit(2); + + const closed = closeSnapshot(snapshotFile); + if (!closed) { process.exit(2); } + process.stdout.write(`${closed}\n`); return; } case 'trend': { @@ -197,7 +448,7 @@ function main(argv) { return; } default: - process.stderr.write('usage: critique-storage.mjs [args]\n'); + process.stderr.write('usage: critique-storage.mjs [args]\n'); process.exit(1); } } diff --git a/.hermes/skills/impeccable/reference/critique.md b/.hermes/skills/impeccable/reference/critique.md index a862ca951..8caa3e422 100644 --- a/.hermes/skills/impeccable/reference/critique.md +++ b/.hermes/skills/impeccable/reference/critique.md @@ -1,6 +1,6 @@ ### Purpose -Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive/backlog for future commands. +Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive of that run. ### Hard Invariants @@ -84,7 +84,7 @@ After Assessment B returns usable CLI findings, reuse them. Do not rerun `detect Synthesize both assessments into a single report. Do NOT simply concatenate. Weave the findings together, noting where the LLM review and detector agree, where the detector caught issues the LLM missed, and where detector findings are false positives. -The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is only an archive/backlog for later commands. +The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is an archive of that run. Structure your feedback as a design director would: @@ -197,7 +197,7 @@ Skip this step if the Setup slug was null (vague or root-level target). IMPECCABLE_CRITIQUE_META='{"target":"","total_score":,"max_score":,"na_heuristics":"","p0_count":,"p1_count":}' \ node .hermes/skills/impeccable/scripts/critique-storage.mjs write "" ``` - `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. + `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. For a local file target, the helper also records an exact content fingerprint so polish can distinguish the assessed bytes from later edits without relying on Git state or timestamps. The helper prints the absolute path it wrote. Leave that file on disk. Polish closes it; this run does not. 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. diff --git a/.hermes/skills/impeccable/reference/polish.md b/.hermes/skills/impeccable/reference/polish.md index b89846eab..1a040886a 100644 --- a/.hermes/skills/impeccable/reference/polish.md +++ b/.hermes/skills/impeccable/reference/polish.md @@ -29,10 +29,10 @@ Use the feature yourself at the surface's representative sizes: desktop and mobi If a prior critique exists, use it as one input: ```bash -node .hermes/skills/impeccable/scripts/critique-storage.mjs latest "" +node .hermes/skills/impeccable/scripts/critique-storage.mjs latest "" --json ``` -Exit 0 returns the latest snapshot; incorporate relevant P0/P1 findings and name the snapshot read. Exit 2 means none exists. Perform an independent pass either way. +Exit 0 returns JSON with the latest snapshot's `body` and an exact `snapshot_file` identity. Retain `snapshot_file` until the end of the pass. For a local file target, the helper compares the file's exact current content fingerprint with the fingerprint captured by critique. Unchanged staged, unstaged, or untracked content remains current; any byte change, deletion, or replacement with a non-file closes the backlog it identified while preserving its trend history and exits 2. A URL target has no local fingerprint and remains current until explicitly closed. When current, incorporate relevant P0/P1 findings from `body` and name the snapshot read. Exit 2 means none exists or the target changed. Perform an independent pass either way. ## 3. Triage @@ -95,3 +95,11 @@ Walk the complete path again with mouse, keyboard, and touch where applicable. C Follow the quality guidance supplied by `context.mjs` and hooks, then run any other relevant QA commands. Context requests a manual scan only when no automatic detector is active; never add another detector pass. Fix real defects and document only narrow intentional exceptions. A clean scan does not replace visual judgment. Finish with a source diff: remove accidental churn, orphaned code, redundant values, and temporary artifacts. Ship only when the feature is functionally complete and consistently finished across the path. + +When this pass clears every Priority Issue it took from a snapshot, close that snapshot: + +```bash +node .hermes/skills/impeccable/scripts/critique-storage.mjs close "" "" +``` + +This closes only the snapshot this pass actually processed; if a newer critique landed meanwhile, its backlog stays live. Do not close when no snapshot was read, when `snapshot_file` was not retained, or when Priority Issues remain. diff --git a/.hermes/skills/impeccable/reference/routing.md b/.hermes/skills/impeccable/reference/routing.md index a439765ef..6a60e322e 100644 --- a/.hermes/skills/impeccable/reference/routing.md +++ b/.hermes/skills/impeccable/reference/routing.md @@ -8,7 +8,7 @@ Reason over the signals; there is no score to obey: - `setup.hasDesign` false while `setup.hasCode` true → `document` (capture the visual system). - `critique.latest` is `null` → the project has never been critiqued; for a set-up project with a real surface, offering `/impeccable critique ` is a strong default. -- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog), or re-run `critique` if the snapshot looks stale. +- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog and closes it when stale or cleared). - `git.changedFiles` pointing at one surface → scope `audit` or `polish` to those files specifically, naming them. - `devServer.running` true → `live` is available for in-browser iteration; if false, don't lead with `live`. **`live` and the bundled `detect.mjs` are web-only.** If `setup.platform` is `ios`, `android`, or `adaptive`, don't lead with either; the browser overlay and the HTML rule engine don't apply to native app code. - Otherwise group by intent (build new / improve what's there / iterate visually), tailored to the current surface and `setup.platform`. diff --git a/.hermes/skills/impeccable/scripts/critique-storage.mjs b/.hermes/skills/impeccable/scripts/critique-storage.mjs index f23fded37..86d297390 100644 --- a/.hermes/skills/impeccable/scripts/critique-storage.mjs +++ b/.hermes/skills/impeccable/scripts/critique-storage.mjs @@ -16,8 +16,9 @@ * CLI entry points (called from skill instructions): * node critique-storage.mjs slug * node critique-storage.mjs write - * node critique-storage.mjs latest + * node critique-storage.mjs latest [--json] * node critique-storage.mjs trend [limit] + * node critique-storage.mjs close * * 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 @@ -27,6 +28,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { createHash } from 'node:crypto'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { getCritiqueDir } from './lib/impeccable-paths.mjs'; import { slugFromTarget } from './lib/target-slug.mjs'; @@ -50,6 +52,45 @@ export function nowFilenameStamp(date = new Date()) { return iso.replace(/[:.]/g, '-').replace(/-\d+Z$/, 'Z'); } +/** + * Return an exact content fingerprint for a local file target. URLs and + * non-files return null because their content is not available here. + * + * The fingerprint deliberately describes bytes, not Git state or mtimes: + * critique often assesses an uncommitted file, and a later polish run should + * inherit that backlog when the bytes are unchanged regardless of staging. + */ +function resolveLocalTargetPath(target, { cwd = process.cwd() } = {}) { + if (!target || /^https?:\/\//i.test(target)) return null; + return path.isAbsolute(target) ? path.resolve(target) : path.resolve(cwd, target); +} + +function resolveTargetIdentity(target, { cwd = process.cwd() } = {}) { + if (!target || typeof target !== 'string') return null; + if (/^https?:\/\//i.test(target)) { + try { + const url = new URL(target); + const pathname = url.pathname.replace(/\/+$/, '') || '/'; + return `url:${url.origin}${pathname}`; + } catch { + return null; + } + } + const filePath = resolveLocalTargetPath(target, { cwd }); + return filePath ? `file:${filePath}` : null; +} + +export function fingerprintTarget(target, { cwd = process.cwd() } = {}) { + const filePath = resolveLocalTargetPath(target, { cwd }); + if (!filePath) return null; + try { + if (!fs.statSync(filePath).isFile()) return null; + return `sha256:${createHash('sha256').update(fs.readFileSync(filePath)).digest('hex')}`; + } catch { + return null; + } +} + /** * Write a snapshot for `slug`. `meta` carries the small structured frontmatter * keys read back by readTrend(). `body` is the human-readable critique @@ -62,14 +103,27 @@ export function writeSnapshot({ slug, meta, body, cwd = process.cwd(), now = new 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; + const contents = `${front}\n${body.trim()}\n`; + + // A second critique can finish in the same UTC second. Use exclusive + // creation and a fixed-width suffix so concurrent writers cannot replace + // history and lexical ordering still keeps collision entries newest. + for (let collision = 0; collision <= 9999; collision += 1) { + const suffix = collision === 0 ? '' : `~${String(collision).padStart(4, '0')}`; + const filePath = path.join(dir, `${timestamp}${suffix}__${slug}.md`); + try { + fs.writeFileSync(filePath, contents, { encoding: 'utf-8', flag: 'wx' }); + return filePath; + } catch (error) { + if (error?.code !== 'EEXIST') throw error; + } + } + throw new Error(`Too many critique snapshots for ${slug} at ${timestamp}`); } function serializeFrontmatter(obj) { @@ -98,6 +152,8 @@ function parseFrontmatter(text) { try { value = JSON.parse(value); } catch { /* leave as-is */ } } else if (/^-?\d+$/.test(value)) { value = Number(value); + } else if (value === 'true' || value === 'false') { + value = value === 'true'; } out[key] = value; } @@ -107,7 +163,7 @@ function parseFrontmatter(text) { /** * Return snapshot files matching `suffix`, sorted oldest → newest. */ -const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z__.+\.md$/; +const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z(?:~\d{4})?__.+\.md$/; function listSnapshots(suffix, cwd) { const dir = getCritiqueDir(cwd); @@ -118,24 +174,105 @@ function listSnapshots(suffix, cwd) { .map((f) => path.join(dir, f)); } -function readLatestSnapshotMatching(suffix, cwd) { - const filePath = listSnapshots(suffix, cwd).at(-1); +function readSnapshot(filePath) { if (!filePath) return null; const body = fs.readFileSync(filePath, 'utf-8'); return { path: filePath, body, meta: parseFrontmatter(body) }; } +function snapshotTargetIdentity(snapshot) { + const targetPath = snapshot?.meta.target_path; + return snapshot?.meta.target_identity + || (targetPath ? `file:${targetPath}` : null); +} + +function readNewestSnapshot(slug, { cwd = process.cwd() } = {}) { + return readSnapshot(listSnapshots(`__${slug}.md`, cwd).at(-1)); +} + +function readNewestSnapshotForIdentity( + slug, + targetIdentity, + { cwd = process.cwd() } = {}, +) { + const matches = listSnapshots(`__${slug}.md`, cwd) + .map(readSnapshot) + .filter((snapshot) => snapshotTargetIdentity(snapshot) === targetIdentity); + return matches.at(-1) || null; +} + /** * 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() } = {}) { - return readLatestSnapshotMatching(`__${slug}.md`, cwd); + const latest = readNewestSnapshot(slug, { cwd }); + return latest?.meta.closed === true ? null : latest; +} + +/** + * Mark one exact snapshot closed without deleting the score history consumed + * by `trend`. Exact identity matters: a newer critique may land after polish + * reads its backlog, and that newer snapshot must remain live. `snapshotFile` + * may be the absolute path returned by readLatestSnapshot() or the basename + * emitted by `latest --json`. Returns the path marked closed, or null. + */ +export function closeSnapshot(snapshotFile, { cwd = process.cwd() } = {}) { + if (!snapshotFile || typeof snapshotFile !== 'string') return null; + const dir = path.resolve(getCritiqueDir(cwd)); + const snapshotPath = path.isAbsolute(snapshotFile) + ? path.resolve(snapshotFile) + : path.resolve(dir, snapshotFile); + const filename = path.basename(snapshotPath); + if ( + path.dirname(snapshotPath) !== dir + || !SNAPSHOT_FILENAME.test(filename) + ) return null; + + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) return null; + snapshot = readSnapshot(snapshotPath); + } catch { + return null; + } + if (!snapshot || snapshot.meta.closed === true) return null; + const closedBody = snapshot.body.replace( + /^(---\r?\n[\s\S]*?)(\r?\n---)/, + '$1\nclosed: true$2', + ); + if (closedBody === snapshot.body) { + throw new Error(`Cannot close snapshot without frontmatter: ${snapshot.path}`); + } + fs.writeFileSync(snapshot.path, closedBody, 'utf-8'); + return snapshot.path; } /** Return the most recent snapshot across all targets, or null. */ export function readLatestSnapshotAcrossTargets({ cwd = process.cwd() } = {}) { - return readLatestSnapshotMatching('.md', cwd); + const snapshots = listSnapshots('.md', cwd).map(readSnapshot); + const identifiedSlugs = new Set( + snapshots + .filter((snapshot) => snapshotTargetIdentity(snapshot)) + .map((snapshot) => snapshot.meta.slug), + ); + const latestByTarget = new Map(); + for (const snapshot of snapshots) { + if (!snapshot?.meta.slug) continue; + // Slugs are lossy: distinct targets such as foo/bar and foo-bar can share + // one. Keep each known identity's latest open/closed state independent so + // closing one target cannot hide another target's live backlog. Once a + // slug has any identity-aware snapshot, its older legacy records are no + // longer independently routable and must not resurface as zombie work. + const targetIdentity = snapshotTargetIdentity(snapshot); + if (!targetIdentity && identifiedSlugs.has(snapshot.meta.slug)) continue; + const streamKey = targetIdentity || `slug:${snapshot.meta.slug}`; + latestByTarget.set(streamKey, snapshot); + } + return [...latestByTarget.values()] + .filter((snapshot) => snapshot.meta.closed !== true) + .sort((a, b) => a.path.localeCompare(b.path)) + .at(-1) || null; } /** @@ -153,9 +290,13 @@ export function readTrend(slug, { limit = 5, cwd = process.cwd() } = {}) { // Accept either a ready slug or a concrete target (path/URL) everywhere, so // callers never have to run the slug step separately. Anything containing a // path or URL marker is resolved through slugFromTarget. +function isReadySlug(value) { + return /^[a-z0-9-]+$/.test(value || '') && !value.includes('/'); +} + function coerceSlug(value) { if (!value) return null; - if (/^[a-z0-9-]+$/.test(value) && !value.includes('/')) return value; + if (isReadySlug(value)) return value; return slugFromTarget(value); } @@ -181,14 +322,124 @@ function main(argv) { if (metaArg) { try { meta = JSON.parse(metaArg); } catch { /* ignore */ } } + // The helper, not caller-provided metadata, owns the target fingerprint. + // This makes the snapshot describe the exact file bytes critique saw. + delete meta.target_fingerprint; + delete meta.target_path; + delete meta.target_identity; + const targetIdentity = resolveTargetIdentity(slugArg); + if (targetIdentity) meta.target_identity = targetIdentity; + const targetFingerprint = fingerprintTarget(slugArg); + if (targetFingerprint) { + meta.target_fingerprint = targetFingerprint; + meta.target_path = resolveLocalTargetPath(slugArg); + } const out = writeSnapshot({ slug, meta, body: raw }); process.stdout.write(`${out}\n`); return; } case 'latest': { - const latest = readLatestSnapshot(coerceSlug(args[0])); - if (!latest) { process.exit(2); } - process.stdout.write(latest.body); + const target = args[0]; + const format = args[1]; + const slug = coerceSlug(target); + if (!slug || (format && format !== '--json')) { + process.stderr.write('usage: latest [--json]\n'); + process.exit(1); + } + const targetFingerprint = fingerprintTarget(target); + const targetPath = resolveLocalTargetPath(target); + const targetIdentity = resolveTargetIdentity(target); + const readySlug = isReadySlug(target); + const newestForSlug = readNewestSnapshot(slug); + if (!newestForSlug) { process.exit(2); } + + // Concrete targets select the newest snapshot for their exact identity, + // not merely the newest filename for a lossy slug. This keeps distinct + // targets such as foo/bar and foo-bar from hiding each other's backlog. + const exactSnapshot = readNewestSnapshotForIdentity(slug, targetIdentity); + let latest = exactSnapshot; + if (!latest && !readySlug) { + // Legacy snapshots have no identity. Preserve their old explicit + // path/URL behavior only when no known target identity was selected. + latest = readNewestSnapshotForIdentity(slug, null); + } + if (!latest) latest = newestForSlug; + if (latest.meta.closed === true) { process.exit(2); } + + const recordedTargetPath = latest.meta.target_path; + const recordedTargetIdentity = snapshotTargetIdentity(latest); + const matchingIdentity = recordedTargetIdentity === targetIdentity; + + // Bare slugs remain a supported lookup mode, including for URL + // snapshots. But when a same-named local file exists, the request is + // ambiguous unless that exact file owns the snapshot identity. + if (readySlug && !recordedTargetIdentity) { + process.stderr.write( + 'ambiguous legacy snapshot target; use an explicit ./path or full URL\n', + ); + process.exit(2); + } + if (readySlug && targetPath && fs.existsSync(targetPath) && !matchingIdentity) { + process.stderr.write( + 'ambiguous snapshot slug; use an explicit ./path or remove the local name collision\n', + ); + process.exit(2); + } + + const concreteTarget = !readySlug || matchingIdentity; + if (concreteTarget && recordedTargetIdentity && !matchingIdentity) { + process.exit(2); + } + const concreteLocalTarget = concreteTarget && targetPath; + if (concreteLocalTarget && latest.meta.target_fingerprint !== targetFingerprint) { + closeSnapshot(latest.path); + process.exit(2); + } + if (format === '--json') { + process.stdout.write(JSON.stringify({ + snapshot_file: path.basename(latest.path), + body: latest.body, + }, null, 2) + '\n'); + } else { + process.stdout.write(latest.body); + } + return; + } + case 'close': { + const [slugArg, snapshotFile, ...extra] = args; + const slug = coerceSlug(slugArg); + if (!slug || !snapshotFile || extra.length > 0) { + process.stderr.write('usage: close \n'); + process.exit(1); + } + if ( + path.basename(snapshotFile) !== snapshotFile + || !SNAPSHOT_FILENAME.test(snapshotFile) + || !snapshotFile.endsWith(`__${slug}.md`) + ) process.exit(2); + + // A slug and filename are not enough to prove ownership because two + // distinct targets can normalize to the same slug. Modern snapshots + // carry a canonical identity, so require the supplied resolved target + // to match it before allowing the exact snapshot to be closed. Legacy + // snapshots without identity retain their historical close behavior. + const snapshotPath = path.join(getCritiqueDir(process.cwd()), snapshotFile); + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) process.exit(2); + snapshot = readSnapshot(snapshotPath); + } catch { + process.exit(2); + } + const recordedTargetIdentity = snapshotTargetIdentity(snapshot); + if ( + recordedTargetIdentity + && recordedTargetIdentity !== resolveTargetIdentity(slugArg) + ) process.exit(2); + + const closed = closeSnapshot(snapshotFile); + if (!closed) { process.exit(2); } + process.stdout.write(`${closed}\n`); return; } case 'trend': { @@ -197,7 +448,7 @@ function main(argv) { return; } default: - process.stderr.write('usage: critique-storage.mjs [args]\n'); + process.stderr.write('usage: critique-storage.mjs [args]\n'); process.exit(1); } } diff --git a/.kiro/skills/impeccable/reference/critique.md b/.kiro/skills/impeccable/reference/critique.md index 1dfa1e2da..594224a64 100644 --- a/.kiro/skills/impeccable/reference/critique.md +++ b/.kiro/skills/impeccable/reference/critique.md @@ -1,6 +1,6 @@ ### Purpose -Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive/backlog for future commands. +Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive of that run. ### Hard Invariants @@ -84,7 +84,7 @@ After Assessment B returns usable CLI findings, reuse them. Do not rerun `detect Synthesize both assessments into a single report. Do NOT simply concatenate. Weave the findings together, noting where the LLM review and detector agree, where the detector caught issues the LLM missed, and where detector findings are false positives. -The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is only an archive/backlog for later commands. +The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is an archive of that run. Structure your feedback as a design director would: @@ -197,7 +197,7 @@ Skip this step if the Setup slug was null (vague or root-level target). IMPECCABLE_CRITIQUE_META='{"target":"","total_score":,"max_score":,"na_heuristics":"","p0_count":,"p1_count":}' \ node .kiro/skills/impeccable/scripts/critique-storage.mjs write "" ``` - `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. + `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. For a local file target, the helper also records an exact content fingerprint so polish can distinguish the assessed bytes from later edits without relying on Git state or timestamps. The helper prints the absolute path it wrote. Leave that file on disk. Polish closes it; this run does not. 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. diff --git a/.kiro/skills/impeccable/reference/polish.md b/.kiro/skills/impeccable/reference/polish.md index fee049ce1..3bf1ecfd3 100644 --- a/.kiro/skills/impeccable/reference/polish.md +++ b/.kiro/skills/impeccable/reference/polish.md @@ -29,10 +29,10 @@ Use the feature yourself at the surface's representative sizes: desktop and mobi If a prior critique exists, use it as one input: ```bash -node .kiro/skills/impeccable/scripts/critique-storage.mjs latest "" +node .kiro/skills/impeccable/scripts/critique-storage.mjs latest "" --json ``` -Exit 0 returns the latest snapshot; incorporate relevant P0/P1 findings and name the snapshot read. Exit 2 means none exists. Perform an independent pass either way. +Exit 0 returns JSON with the latest snapshot's `body` and an exact `snapshot_file` identity. Retain `snapshot_file` until the end of the pass. For a local file target, the helper compares the file's exact current content fingerprint with the fingerprint captured by critique. Unchanged staged, unstaged, or untracked content remains current; any byte change, deletion, or replacement with a non-file closes the backlog it identified while preserving its trend history and exits 2. A URL target has no local fingerprint and remains current until explicitly closed. When current, incorporate relevant P0/P1 findings from `body` and name the snapshot read. Exit 2 means none exists or the target changed. Perform an independent pass either way. ## 3. Triage @@ -95,3 +95,11 @@ Walk the complete path again with mouse, keyboard, and touch where applicable. C Follow the quality guidance supplied by `context.mjs` and hooks, then run any other relevant QA commands. Context requests a manual scan only when no automatic detector is active; never add another detector pass. Fix real defects and document only narrow intentional exceptions. A clean scan does not replace visual judgment. Finish with a source diff: remove accidental churn, orphaned code, redundant values, and temporary artifacts. Ship only when the feature is functionally complete and consistently finished across the path. + +When this pass clears every Priority Issue it took from a snapshot, close that snapshot: + +```bash +node .kiro/skills/impeccable/scripts/critique-storage.mjs close "" "" +``` + +This closes only the snapshot this pass actually processed; if a newer critique landed meanwhile, its backlog stays live. Do not close when no snapshot was read, when `snapshot_file` was not retained, or when Priority Issues remain. diff --git a/.kiro/skills/impeccable/reference/routing.md b/.kiro/skills/impeccable/reference/routing.md index 26efca80e..918048a00 100644 --- a/.kiro/skills/impeccable/reference/routing.md +++ b/.kiro/skills/impeccable/reference/routing.md @@ -8,7 +8,7 @@ Reason over the signals; there is no score to obey: - `setup.hasDesign` false while `setup.hasCode` true → `document` (capture the visual system). - `critique.latest` is `null` → the project has never been critiqued; for a set-up project with a real surface, offering `/impeccable critique ` is a strong default. -- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog), or re-run `critique` if the snapshot looks stale. +- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog and closes it when stale or cleared). - `git.changedFiles` pointing at one surface → scope `audit` or `polish` to those files specifically, naming them. - `devServer.running` true → `live` is available for in-browser iteration; if false, don't lead with `live`. **`live` and the bundled `detect.mjs` are web-only.** If `setup.platform` is `ios`, `android`, or `adaptive`, don't lead with either; the browser overlay and the HTML rule engine don't apply to native app code. - Otherwise group by intent (build new / improve what's there / iterate visually), tailored to the current surface and `setup.platform`. diff --git a/.kiro/skills/impeccable/scripts/critique-storage.mjs b/.kiro/skills/impeccable/scripts/critique-storage.mjs index f23fded37..86d297390 100644 --- a/.kiro/skills/impeccable/scripts/critique-storage.mjs +++ b/.kiro/skills/impeccable/scripts/critique-storage.mjs @@ -16,8 +16,9 @@ * CLI entry points (called from skill instructions): * node critique-storage.mjs slug * node critique-storage.mjs write - * node critique-storage.mjs latest + * node critique-storage.mjs latest [--json] * node critique-storage.mjs trend [limit] + * node critique-storage.mjs close * * 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 @@ -27,6 +28,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { createHash } from 'node:crypto'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { getCritiqueDir } from './lib/impeccable-paths.mjs'; import { slugFromTarget } from './lib/target-slug.mjs'; @@ -50,6 +52,45 @@ export function nowFilenameStamp(date = new Date()) { return iso.replace(/[:.]/g, '-').replace(/-\d+Z$/, 'Z'); } +/** + * Return an exact content fingerprint for a local file target. URLs and + * non-files return null because their content is not available here. + * + * The fingerprint deliberately describes bytes, not Git state or mtimes: + * critique often assesses an uncommitted file, and a later polish run should + * inherit that backlog when the bytes are unchanged regardless of staging. + */ +function resolveLocalTargetPath(target, { cwd = process.cwd() } = {}) { + if (!target || /^https?:\/\//i.test(target)) return null; + return path.isAbsolute(target) ? path.resolve(target) : path.resolve(cwd, target); +} + +function resolveTargetIdentity(target, { cwd = process.cwd() } = {}) { + if (!target || typeof target !== 'string') return null; + if (/^https?:\/\//i.test(target)) { + try { + const url = new URL(target); + const pathname = url.pathname.replace(/\/+$/, '') || '/'; + return `url:${url.origin}${pathname}`; + } catch { + return null; + } + } + const filePath = resolveLocalTargetPath(target, { cwd }); + return filePath ? `file:${filePath}` : null; +} + +export function fingerprintTarget(target, { cwd = process.cwd() } = {}) { + const filePath = resolveLocalTargetPath(target, { cwd }); + if (!filePath) return null; + try { + if (!fs.statSync(filePath).isFile()) return null; + return `sha256:${createHash('sha256').update(fs.readFileSync(filePath)).digest('hex')}`; + } catch { + return null; + } +} + /** * Write a snapshot for `slug`. `meta` carries the small structured frontmatter * keys read back by readTrend(). `body` is the human-readable critique @@ -62,14 +103,27 @@ export function writeSnapshot({ slug, meta, body, cwd = process.cwd(), now = new 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; + const contents = `${front}\n${body.trim()}\n`; + + // A second critique can finish in the same UTC second. Use exclusive + // creation and a fixed-width suffix so concurrent writers cannot replace + // history and lexical ordering still keeps collision entries newest. + for (let collision = 0; collision <= 9999; collision += 1) { + const suffix = collision === 0 ? '' : `~${String(collision).padStart(4, '0')}`; + const filePath = path.join(dir, `${timestamp}${suffix}__${slug}.md`); + try { + fs.writeFileSync(filePath, contents, { encoding: 'utf-8', flag: 'wx' }); + return filePath; + } catch (error) { + if (error?.code !== 'EEXIST') throw error; + } + } + throw new Error(`Too many critique snapshots for ${slug} at ${timestamp}`); } function serializeFrontmatter(obj) { @@ -98,6 +152,8 @@ function parseFrontmatter(text) { try { value = JSON.parse(value); } catch { /* leave as-is */ } } else if (/^-?\d+$/.test(value)) { value = Number(value); + } else if (value === 'true' || value === 'false') { + value = value === 'true'; } out[key] = value; } @@ -107,7 +163,7 @@ function parseFrontmatter(text) { /** * Return snapshot files matching `suffix`, sorted oldest → newest. */ -const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z__.+\.md$/; +const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z(?:~\d{4})?__.+\.md$/; function listSnapshots(suffix, cwd) { const dir = getCritiqueDir(cwd); @@ -118,24 +174,105 @@ function listSnapshots(suffix, cwd) { .map((f) => path.join(dir, f)); } -function readLatestSnapshotMatching(suffix, cwd) { - const filePath = listSnapshots(suffix, cwd).at(-1); +function readSnapshot(filePath) { if (!filePath) return null; const body = fs.readFileSync(filePath, 'utf-8'); return { path: filePath, body, meta: parseFrontmatter(body) }; } +function snapshotTargetIdentity(snapshot) { + const targetPath = snapshot?.meta.target_path; + return snapshot?.meta.target_identity + || (targetPath ? `file:${targetPath}` : null); +} + +function readNewestSnapshot(slug, { cwd = process.cwd() } = {}) { + return readSnapshot(listSnapshots(`__${slug}.md`, cwd).at(-1)); +} + +function readNewestSnapshotForIdentity( + slug, + targetIdentity, + { cwd = process.cwd() } = {}, +) { + const matches = listSnapshots(`__${slug}.md`, cwd) + .map(readSnapshot) + .filter((snapshot) => snapshotTargetIdentity(snapshot) === targetIdentity); + return matches.at(-1) || null; +} + /** * 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() } = {}) { - return readLatestSnapshotMatching(`__${slug}.md`, cwd); + const latest = readNewestSnapshot(slug, { cwd }); + return latest?.meta.closed === true ? null : latest; +} + +/** + * Mark one exact snapshot closed without deleting the score history consumed + * by `trend`. Exact identity matters: a newer critique may land after polish + * reads its backlog, and that newer snapshot must remain live. `snapshotFile` + * may be the absolute path returned by readLatestSnapshot() or the basename + * emitted by `latest --json`. Returns the path marked closed, or null. + */ +export function closeSnapshot(snapshotFile, { cwd = process.cwd() } = {}) { + if (!snapshotFile || typeof snapshotFile !== 'string') return null; + const dir = path.resolve(getCritiqueDir(cwd)); + const snapshotPath = path.isAbsolute(snapshotFile) + ? path.resolve(snapshotFile) + : path.resolve(dir, snapshotFile); + const filename = path.basename(snapshotPath); + if ( + path.dirname(snapshotPath) !== dir + || !SNAPSHOT_FILENAME.test(filename) + ) return null; + + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) return null; + snapshot = readSnapshot(snapshotPath); + } catch { + return null; + } + if (!snapshot || snapshot.meta.closed === true) return null; + const closedBody = snapshot.body.replace( + /^(---\r?\n[\s\S]*?)(\r?\n---)/, + '$1\nclosed: true$2', + ); + if (closedBody === snapshot.body) { + throw new Error(`Cannot close snapshot without frontmatter: ${snapshot.path}`); + } + fs.writeFileSync(snapshot.path, closedBody, 'utf-8'); + return snapshot.path; } /** Return the most recent snapshot across all targets, or null. */ export function readLatestSnapshotAcrossTargets({ cwd = process.cwd() } = {}) { - return readLatestSnapshotMatching('.md', cwd); + const snapshots = listSnapshots('.md', cwd).map(readSnapshot); + const identifiedSlugs = new Set( + snapshots + .filter((snapshot) => snapshotTargetIdentity(snapshot)) + .map((snapshot) => snapshot.meta.slug), + ); + const latestByTarget = new Map(); + for (const snapshot of snapshots) { + if (!snapshot?.meta.slug) continue; + // Slugs are lossy: distinct targets such as foo/bar and foo-bar can share + // one. Keep each known identity's latest open/closed state independent so + // closing one target cannot hide another target's live backlog. Once a + // slug has any identity-aware snapshot, its older legacy records are no + // longer independently routable and must not resurface as zombie work. + const targetIdentity = snapshotTargetIdentity(snapshot); + if (!targetIdentity && identifiedSlugs.has(snapshot.meta.slug)) continue; + const streamKey = targetIdentity || `slug:${snapshot.meta.slug}`; + latestByTarget.set(streamKey, snapshot); + } + return [...latestByTarget.values()] + .filter((snapshot) => snapshot.meta.closed !== true) + .sort((a, b) => a.path.localeCompare(b.path)) + .at(-1) || null; } /** @@ -153,9 +290,13 @@ export function readTrend(slug, { limit = 5, cwd = process.cwd() } = {}) { // Accept either a ready slug or a concrete target (path/URL) everywhere, so // callers never have to run the slug step separately. Anything containing a // path or URL marker is resolved through slugFromTarget. +function isReadySlug(value) { + return /^[a-z0-9-]+$/.test(value || '') && !value.includes('/'); +} + function coerceSlug(value) { if (!value) return null; - if (/^[a-z0-9-]+$/.test(value) && !value.includes('/')) return value; + if (isReadySlug(value)) return value; return slugFromTarget(value); } @@ -181,14 +322,124 @@ function main(argv) { if (metaArg) { try { meta = JSON.parse(metaArg); } catch { /* ignore */ } } + // The helper, not caller-provided metadata, owns the target fingerprint. + // This makes the snapshot describe the exact file bytes critique saw. + delete meta.target_fingerprint; + delete meta.target_path; + delete meta.target_identity; + const targetIdentity = resolveTargetIdentity(slugArg); + if (targetIdentity) meta.target_identity = targetIdentity; + const targetFingerprint = fingerprintTarget(slugArg); + if (targetFingerprint) { + meta.target_fingerprint = targetFingerprint; + meta.target_path = resolveLocalTargetPath(slugArg); + } const out = writeSnapshot({ slug, meta, body: raw }); process.stdout.write(`${out}\n`); return; } case 'latest': { - const latest = readLatestSnapshot(coerceSlug(args[0])); - if (!latest) { process.exit(2); } - process.stdout.write(latest.body); + const target = args[0]; + const format = args[1]; + const slug = coerceSlug(target); + if (!slug || (format && format !== '--json')) { + process.stderr.write('usage: latest [--json]\n'); + process.exit(1); + } + const targetFingerprint = fingerprintTarget(target); + const targetPath = resolveLocalTargetPath(target); + const targetIdentity = resolveTargetIdentity(target); + const readySlug = isReadySlug(target); + const newestForSlug = readNewestSnapshot(slug); + if (!newestForSlug) { process.exit(2); } + + // Concrete targets select the newest snapshot for their exact identity, + // not merely the newest filename for a lossy slug. This keeps distinct + // targets such as foo/bar and foo-bar from hiding each other's backlog. + const exactSnapshot = readNewestSnapshotForIdentity(slug, targetIdentity); + let latest = exactSnapshot; + if (!latest && !readySlug) { + // Legacy snapshots have no identity. Preserve their old explicit + // path/URL behavior only when no known target identity was selected. + latest = readNewestSnapshotForIdentity(slug, null); + } + if (!latest) latest = newestForSlug; + if (latest.meta.closed === true) { process.exit(2); } + + const recordedTargetPath = latest.meta.target_path; + const recordedTargetIdentity = snapshotTargetIdentity(latest); + const matchingIdentity = recordedTargetIdentity === targetIdentity; + + // Bare slugs remain a supported lookup mode, including for URL + // snapshots. But when a same-named local file exists, the request is + // ambiguous unless that exact file owns the snapshot identity. + if (readySlug && !recordedTargetIdentity) { + process.stderr.write( + 'ambiguous legacy snapshot target; use an explicit ./path or full URL\n', + ); + process.exit(2); + } + if (readySlug && targetPath && fs.existsSync(targetPath) && !matchingIdentity) { + process.stderr.write( + 'ambiguous snapshot slug; use an explicit ./path or remove the local name collision\n', + ); + process.exit(2); + } + + const concreteTarget = !readySlug || matchingIdentity; + if (concreteTarget && recordedTargetIdentity && !matchingIdentity) { + process.exit(2); + } + const concreteLocalTarget = concreteTarget && targetPath; + if (concreteLocalTarget && latest.meta.target_fingerprint !== targetFingerprint) { + closeSnapshot(latest.path); + process.exit(2); + } + if (format === '--json') { + process.stdout.write(JSON.stringify({ + snapshot_file: path.basename(latest.path), + body: latest.body, + }, null, 2) + '\n'); + } else { + process.stdout.write(latest.body); + } + return; + } + case 'close': { + const [slugArg, snapshotFile, ...extra] = args; + const slug = coerceSlug(slugArg); + if (!slug || !snapshotFile || extra.length > 0) { + process.stderr.write('usage: close \n'); + process.exit(1); + } + if ( + path.basename(snapshotFile) !== snapshotFile + || !SNAPSHOT_FILENAME.test(snapshotFile) + || !snapshotFile.endsWith(`__${slug}.md`) + ) process.exit(2); + + // A slug and filename are not enough to prove ownership because two + // distinct targets can normalize to the same slug. Modern snapshots + // carry a canonical identity, so require the supplied resolved target + // to match it before allowing the exact snapshot to be closed. Legacy + // snapshots without identity retain their historical close behavior. + const snapshotPath = path.join(getCritiqueDir(process.cwd()), snapshotFile); + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) process.exit(2); + snapshot = readSnapshot(snapshotPath); + } catch { + process.exit(2); + } + const recordedTargetIdentity = snapshotTargetIdentity(snapshot); + if ( + recordedTargetIdentity + && recordedTargetIdentity !== resolveTargetIdentity(slugArg) + ) process.exit(2); + + const closed = closeSnapshot(snapshotFile); + if (!closed) { process.exit(2); } + process.stdout.write(`${closed}\n`); return; } case 'trend': { @@ -197,7 +448,7 @@ function main(argv) { return; } default: - process.stderr.write('usage: critique-storage.mjs [args]\n'); + process.stderr.write('usage: critique-storage.mjs [args]\n'); process.exit(1); } } diff --git a/.opencode/skills/impeccable/reference/critique.md b/.opencode/skills/impeccable/reference/critique.md index 6f05e1a2d..b8ef2d3f7 100644 --- a/.opencode/skills/impeccable/reference/critique.md +++ b/.opencode/skills/impeccable/reference/critique.md @@ -1,6 +1,6 @@ ### Purpose -Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive/backlog for future commands. +Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive of that run. ### Hard Invariants @@ -84,7 +84,7 @@ After Assessment B returns usable CLI findings, reuse them. Do not rerun `detect Synthesize both assessments into a single report. Do NOT simply concatenate. Weave the findings together, noting where the LLM review and detector agree, where the detector caught issues the LLM missed, and where detector findings are false positives. -The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is only an archive/backlog for later commands. +The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is an archive of that run. Structure your feedback as a design director would: @@ -197,7 +197,7 @@ Skip this step if the Setup slug was null (vague or root-level target). IMPECCABLE_CRITIQUE_META='{"target":"","total_score":,"max_score":,"na_heuristics":"","p0_count":,"p1_count":}' \ node .opencode/skills/impeccable/scripts/critique-storage.mjs write "" ``` - `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. + `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. For a local file target, the helper also records an exact content fingerprint so polish can distinguish the assessed bytes from later edits without relying on Git state or timestamps. The helper prints the absolute path it wrote. Leave that file on disk. Polish closes it; this run does not. 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. diff --git a/.opencode/skills/impeccable/reference/polish.md b/.opencode/skills/impeccable/reference/polish.md index d46ea12c9..8c41d2f83 100644 --- a/.opencode/skills/impeccable/reference/polish.md +++ b/.opencode/skills/impeccable/reference/polish.md @@ -29,10 +29,10 @@ Use the feature yourself at the surface's representative sizes: desktop and mobi If a prior critique exists, use it as one input: ```bash -node .opencode/skills/impeccable/scripts/critique-storage.mjs latest "" +node .opencode/skills/impeccable/scripts/critique-storage.mjs latest "" --json ``` -Exit 0 returns the latest snapshot; incorporate relevant P0/P1 findings and name the snapshot read. Exit 2 means none exists. Perform an independent pass either way. +Exit 0 returns JSON with the latest snapshot's `body` and an exact `snapshot_file` identity. Retain `snapshot_file` until the end of the pass. For a local file target, the helper compares the file's exact current content fingerprint with the fingerprint captured by critique. Unchanged staged, unstaged, or untracked content remains current; any byte change, deletion, or replacement with a non-file closes the backlog it identified while preserving its trend history and exits 2. A URL target has no local fingerprint and remains current until explicitly closed. When current, incorporate relevant P0/P1 findings from `body` and name the snapshot read. Exit 2 means none exists or the target changed. Perform an independent pass either way. ## 3. Triage @@ -95,3 +95,11 @@ Walk the complete path again with mouse, keyboard, and touch where applicable. C Follow the quality guidance supplied by `context.mjs` and hooks, then run any other relevant QA commands. Context requests a manual scan only when no automatic detector is active; never add another detector pass. Fix real defects and document only narrow intentional exceptions. A clean scan does not replace visual judgment. Finish with a source diff: remove accidental churn, orphaned code, redundant values, and temporary artifacts. Ship only when the feature is functionally complete and consistently finished across the path. + +When this pass clears every Priority Issue it took from a snapshot, close that snapshot: + +```bash +node .opencode/skills/impeccable/scripts/critique-storage.mjs close "" "" +``` + +This closes only the snapshot this pass actually processed; if a newer critique landed meanwhile, its backlog stays live. Do not close when no snapshot was read, when `snapshot_file` was not retained, or when Priority Issues remain. diff --git a/.opencode/skills/impeccable/reference/routing.md b/.opencode/skills/impeccable/reference/routing.md index 9d6caa879..b84af9cdb 100644 --- a/.opencode/skills/impeccable/reference/routing.md +++ b/.opencode/skills/impeccable/reference/routing.md @@ -8,7 +8,7 @@ Reason over the signals; there is no score to obey: - `setup.hasDesign` false while `setup.hasCode` true → `document` (capture the visual system). - `critique.latest` is `null` → the project has never been critiqued; for a set-up project with a real surface, offering `/impeccable critique ` is a strong default. -- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog), or re-run `critique` if the snapshot looks stale. +- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog and closes it when stale or cleared). - `git.changedFiles` pointing at one surface → scope `audit` or `polish` to those files specifically, naming them. - `devServer.running` true → `live` is available for in-browser iteration; if false, don't lead with `live`. **`live` and the bundled `detect.mjs` are web-only.** If `setup.platform` is `ios`, `android`, or `adaptive`, don't lead with either; the browser overlay and the HTML rule engine don't apply to native app code. - Otherwise group by intent (build new / improve what's there / iterate visually), tailored to the current surface and `setup.platform`. diff --git a/.opencode/skills/impeccable/scripts/critique-storage.mjs b/.opencode/skills/impeccable/scripts/critique-storage.mjs index f23fded37..86d297390 100644 --- a/.opencode/skills/impeccable/scripts/critique-storage.mjs +++ b/.opencode/skills/impeccable/scripts/critique-storage.mjs @@ -16,8 +16,9 @@ * CLI entry points (called from skill instructions): * node critique-storage.mjs slug * node critique-storage.mjs write - * node critique-storage.mjs latest + * node critique-storage.mjs latest [--json] * node critique-storage.mjs trend [limit] + * node critique-storage.mjs close * * 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 @@ -27,6 +28,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { createHash } from 'node:crypto'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { getCritiqueDir } from './lib/impeccable-paths.mjs'; import { slugFromTarget } from './lib/target-slug.mjs'; @@ -50,6 +52,45 @@ export function nowFilenameStamp(date = new Date()) { return iso.replace(/[:.]/g, '-').replace(/-\d+Z$/, 'Z'); } +/** + * Return an exact content fingerprint for a local file target. URLs and + * non-files return null because their content is not available here. + * + * The fingerprint deliberately describes bytes, not Git state or mtimes: + * critique often assesses an uncommitted file, and a later polish run should + * inherit that backlog when the bytes are unchanged regardless of staging. + */ +function resolveLocalTargetPath(target, { cwd = process.cwd() } = {}) { + if (!target || /^https?:\/\//i.test(target)) return null; + return path.isAbsolute(target) ? path.resolve(target) : path.resolve(cwd, target); +} + +function resolveTargetIdentity(target, { cwd = process.cwd() } = {}) { + if (!target || typeof target !== 'string') return null; + if (/^https?:\/\//i.test(target)) { + try { + const url = new URL(target); + const pathname = url.pathname.replace(/\/+$/, '') || '/'; + return `url:${url.origin}${pathname}`; + } catch { + return null; + } + } + const filePath = resolveLocalTargetPath(target, { cwd }); + return filePath ? `file:${filePath}` : null; +} + +export function fingerprintTarget(target, { cwd = process.cwd() } = {}) { + const filePath = resolveLocalTargetPath(target, { cwd }); + if (!filePath) return null; + try { + if (!fs.statSync(filePath).isFile()) return null; + return `sha256:${createHash('sha256').update(fs.readFileSync(filePath)).digest('hex')}`; + } catch { + return null; + } +} + /** * Write a snapshot for `slug`. `meta` carries the small structured frontmatter * keys read back by readTrend(). `body` is the human-readable critique @@ -62,14 +103,27 @@ export function writeSnapshot({ slug, meta, body, cwd = process.cwd(), now = new 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; + const contents = `${front}\n${body.trim()}\n`; + + // A second critique can finish in the same UTC second. Use exclusive + // creation and a fixed-width suffix so concurrent writers cannot replace + // history and lexical ordering still keeps collision entries newest. + for (let collision = 0; collision <= 9999; collision += 1) { + const suffix = collision === 0 ? '' : `~${String(collision).padStart(4, '0')}`; + const filePath = path.join(dir, `${timestamp}${suffix}__${slug}.md`); + try { + fs.writeFileSync(filePath, contents, { encoding: 'utf-8', flag: 'wx' }); + return filePath; + } catch (error) { + if (error?.code !== 'EEXIST') throw error; + } + } + throw new Error(`Too many critique snapshots for ${slug} at ${timestamp}`); } function serializeFrontmatter(obj) { @@ -98,6 +152,8 @@ function parseFrontmatter(text) { try { value = JSON.parse(value); } catch { /* leave as-is */ } } else if (/^-?\d+$/.test(value)) { value = Number(value); + } else if (value === 'true' || value === 'false') { + value = value === 'true'; } out[key] = value; } @@ -107,7 +163,7 @@ function parseFrontmatter(text) { /** * Return snapshot files matching `suffix`, sorted oldest → newest. */ -const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z__.+\.md$/; +const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z(?:~\d{4})?__.+\.md$/; function listSnapshots(suffix, cwd) { const dir = getCritiqueDir(cwd); @@ -118,24 +174,105 @@ function listSnapshots(suffix, cwd) { .map((f) => path.join(dir, f)); } -function readLatestSnapshotMatching(suffix, cwd) { - const filePath = listSnapshots(suffix, cwd).at(-1); +function readSnapshot(filePath) { if (!filePath) return null; const body = fs.readFileSync(filePath, 'utf-8'); return { path: filePath, body, meta: parseFrontmatter(body) }; } +function snapshotTargetIdentity(snapshot) { + const targetPath = snapshot?.meta.target_path; + return snapshot?.meta.target_identity + || (targetPath ? `file:${targetPath}` : null); +} + +function readNewestSnapshot(slug, { cwd = process.cwd() } = {}) { + return readSnapshot(listSnapshots(`__${slug}.md`, cwd).at(-1)); +} + +function readNewestSnapshotForIdentity( + slug, + targetIdentity, + { cwd = process.cwd() } = {}, +) { + const matches = listSnapshots(`__${slug}.md`, cwd) + .map(readSnapshot) + .filter((snapshot) => snapshotTargetIdentity(snapshot) === targetIdentity); + return matches.at(-1) || null; +} + /** * 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() } = {}) { - return readLatestSnapshotMatching(`__${slug}.md`, cwd); + const latest = readNewestSnapshot(slug, { cwd }); + return latest?.meta.closed === true ? null : latest; +} + +/** + * Mark one exact snapshot closed without deleting the score history consumed + * by `trend`. Exact identity matters: a newer critique may land after polish + * reads its backlog, and that newer snapshot must remain live. `snapshotFile` + * may be the absolute path returned by readLatestSnapshot() or the basename + * emitted by `latest --json`. Returns the path marked closed, or null. + */ +export function closeSnapshot(snapshotFile, { cwd = process.cwd() } = {}) { + if (!snapshotFile || typeof snapshotFile !== 'string') return null; + const dir = path.resolve(getCritiqueDir(cwd)); + const snapshotPath = path.isAbsolute(snapshotFile) + ? path.resolve(snapshotFile) + : path.resolve(dir, snapshotFile); + const filename = path.basename(snapshotPath); + if ( + path.dirname(snapshotPath) !== dir + || !SNAPSHOT_FILENAME.test(filename) + ) return null; + + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) return null; + snapshot = readSnapshot(snapshotPath); + } catch { + return null; + } + if (!snapshot || snapshot.meta.closed === true) return null; + const closedBody = snapshot.body.replace( + /^(---\r?\n[\s\S]*?)(\r?\n---)/, + '$1\nclosed: true$2', + ); + if (closedBody === snapshot.body) { + throw new Error(`Cannot close snapshot without frontmatter: ${snapshot.path}`); + } + fs.writeFileSync(snapshot.path, closedBody, 'utf-8'); + return snapshot.path; } /** Return the most recent snapshot across all targets, or null. */ export function readLatestSnapshotAcrossTargets({ cwd = process.cwd() } = {}) { - return readLatestSnapshotMatching('.md', cwd); + const snapshots = listSnapshots('.md', cwd).map(readSnapshot); + const identifiedSlugs = new Set( + snapshots + .filter((snapshot) => snapshotTargetIdentity(snapshot)) + .map((snapshot) => snapshot.meta.slug), + ); + const latestByTarget = new Map(); + for (const snapshot of snapshots) { + if (!snapshot?.meta.slug) continue; + // Slugs are lossy: distinct targets such as foo/bar and foo-bar can share + // one. Keep each known identity's latest open/closed state independent so + // closing one target cannot hide another target's live backlog. Once a + // slug has any identity-aware snapshot, its older legacy records are no + // longer independently routable and must not resurface as zombie work. + const targetIdentity = snapshotTargetIdentity(snapshot); + if (!targetIdentity && identifiedSlugs.has(snapshot.meta.slug)) continue; + const streamKey = targetIdentity || `slug:${snapshot.meta.slug}`; + latestByTarget.set(streamKey, snapshot); + } + return [...latestByTarget.values()] + .filter((snapshot) => snapshot.meta.closed !== true) + .sort((a, b) => a.path.localeCompare(b.path)) + .at(-1) || null; } /** @@ -153,9 +290,13 @@ export function readTrend(slug, { limit = 5, cwd = process.cwd() } = {}) { // Accept either a ready slug or a concrete target (path/URL) everywhere, so // callers never have to run the slug step separately. Anything containing a // path or URL marker is resolved through slugFromTarget. +function isReadySlug(value) { + return /^[a-z0-9-]+$/.test(value || '') && !value.includes('/'); +} + function coerceSlug(value) { if (!value) return null; - if (/^[a-z0-9-]+$/.test(value) && !value.includes('/')) return value; + if (isReadySlug(value)) return value; return slugFromTarget(value); } @@ -181,14 +322,124 @@ function main(argv) { if (metaArg) { try { meta = JSON.parse(metaArg); } catch { /* ignore */ } } + // The helper, not caller-provided metadata, owns the target fingerprint. + // This makes the snapshot describe the exact file bytes critique saw. + delete meta.target_fingerprint; + delete meta.target_path; + delete meta.target_identity; + const targetIdentity = resolveTargetIdentity(slugArg); + if (targetIdentity) meta.target_identity = targetIdentity; + const targetFingerprint = fingerprintTarget(slugArg); + if (targetFingerprint) { + meta.target_fingerprint = targetFingerprint; + meta.target_path = resolveLocalTargetPath(slugArg); + } const out = writeSnapshot({ slug, meta, body: raw }); process.stdout.write(`${out}\n`); return; } case 'latest': { - const latest = readLatestSnapshot(coerceSlug(args[0])); - if (!latest) { process.exit(2); } - process.stdout.write(latest.body); + const target = args[0]; + const format = args[1]; + const slug = coerceSlug(target); + if (!slug || (format && format !== '--json')) { + process.stderr.write('usage: latest [--json]\n'); + process.exit(1); + } + const targetFingerprint = fingerprintTarget(target); + const targetPath = resolveLocalTargetPath(target); + const targetIdentity = resolveTargetIdentity(target); + const readySlug = isReadySlug(target); + const newestForSlug = readNewestSnapshot(slug); + if (!newestForSlug) { process.exit(2); } + + // Concrete targets select the newest snapshot for their exact identity, + // not merely the newest filename for a lossy slug. This keeps distinct + // targets such as foo/bar and foo-bar from hiding each other's backlog. + const exactSnapshot = readNewestSnapshotForIdentity(slug, targetIdentity); + let latest = exactSnapshot; + if (!latest && !readySlug) { + // Legacy snapshots have no identity. Preserve their old explicit + // path/URL behavior only when no known target identity was selected. + latest = readNewestSnapshotForIdentity(slug, null); + } + if (!latest) latest = newestForSlug; + if (latest.meta.closed === true) { process.exit(2); } + + const recordedTargetPath = latest.meta.target_path; + const recordedTargetIdentity = snapshotTargetIdentity(latest); + const matchingIdentity = recordedTargetIdentity === targetIdentity; + + // Bare slugs remain a supported lookup mode, including for URL + // snapshots. But when a same-named local file exists, the request is + // ambiguous unless that exact file owns the snapshot identity. + if (readySlug && !recordedTargetIdentity) { + process.stderr.write( + 'ambiguous legacy snapshot target; use an explicit ./path or full URL\n', + ); + process.exit(2); + } + if (readySlug && targetPath && fs.existsSync(targetPath) && !matchingIdentity) { + process.stderr.write( + 'ambiguous snapshot slug; use an explicit ./path or remove the local name collision\n', + ); + process.exit(2); + } + + const concreteTarget = !readySlug || matchingIdentity; + if (concreteTarget && recordedTargetIdentity && !matchingIdentity) { + process.exit(2); + } + const concreteLocalTarget = concreteTarget && targetPath; + if (concreteLocalTarget && latest.meta.target_fingerprint !== targetFingerprint) { + closeSnapshot(latest.path); + process.exit(2); + } + if (format === '--json') { + process.stdout.write(JSON.stringify({ + snapshot_file: path.basename(latest.path), + body: latest.body, + }, null, 2) + '\n'); + } else { + process.stdout.write(latest.body); + } + return; + } + case 'close': { + const [slugArg, snapshotFile, ...extra] = args; + const slug = coerceSlug(slugArg); + if (!slug || !snapshotFile || extra.length > 0) { + process.stderr.write('usage: close \n'); + process.exit(1); + } + if ( + path.basename(snapshotFile) !== snapshotFile + || !SNAPSHOT_FILENAME.test(snapshotFile) + || !snapshotFile.endsWith(`__${slug}.md`) + ) process.exit(2); + + // A slug and filename are not enough to prove ownership because two + // distinct targets can normalize to the same slug. Modern snapshots + // carry a canonical identity, so require the supplied resolved target + // to match it before allowing the exact snapshot to be closed. Legacy + // snapshots without identity retain their historical close behavior. + const snapshotPath = path.join(getCritiqueDir(process.cwd()), snapshotFile); + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) process.exit(2); + snapshot = readSnapshot(snapshotPath); + } catch { + process.exit(2); + } + const recordedTargetIdentity = snapshotTargetIdentity(snapshot); + if ( + recordedTargetIdentity + && recordedTargetIdentity !== resolveTargetIdentity(slugArg) + ) process.exit(2); + + const closed = closeSnapshot(snapshotFile); + if (!closed) { process.exit(2); } + process.stdout.write(`${closed}\n`); return; } case 'trend': { @@ -197,7 +448,7 @@ function main(argv) { return; } default: - process.stderr.write('usage: critique-storage.mjs [args]\n'); + process.stderr.write('usage: critique-storage.mjs [args]\n'); process.exit(1); } } diff --git a/.pi/skills/impeccable/reference/critique.md b/.pi/skills/impeccable/reference/critique.md index dcf4a98b0..a70248baf 100644 --- a/.pi/skills/impeccable/reference/critique.md +++ b/.pi/skills/impeccable/reference/critique.md @@ -1,6 +1,6 @@ ### Purpose -Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive/backlog for future commands. +Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive of that run. ### Hard Invariants @@ -84,7 +84,7 @@ After Assessment B returns usable CLI findings, reuse them. Do not rerun `detect Synthesize both assessments into a single report. Do NOT simply concatenate. Weave the findings together, noting where the LLM review and detector agree, where the detector caught issues the LLM missed, and where detector findings are false positives. -The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is only an archive/backlog for later commands. +The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is an archive of that run. Structure your feedback as a design director would: @@ -197,7 +197,7 @@ Skip this step if the Setup slug was null (vague or root-level target). IMPECCABLE_CRITIQUE_META='{"target":"","total_score":,"max_score":,"na_heuristics":"","p0_count":,"p1_count":}' \ node .pi/skills/impeccable/scripts/critique-storage.mjs write "" ``` - `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. + `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. For a local file target, the helper also records an exact content fingerprint so polish can distinguish the assessed bytes from later edits without relying on Git state or timestamps. The helper prints the absolute path it wrote. Leave that file on disk. Polish closes it; this run does not. 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. diff --git a/.pi/skills/impeccable/reference/polish.md b/.pi/skills/impeccable/reference/polish.md index fb26c7fca..ece8f886b 100644 --- a/.pi/skills/impeccable/reference/polish.md +++ b/.pi/skills/impeccable/reference/polish.md @@ -29,10 +29,10 @@ Use the feature yourself at the surface's representative sizes: desktop and mobi If a prior critique exists, use it as one input: ```bash -node .pi/skills/impeccable/scripts/critique-storage.mjs latest "" +node .pi/skills/impeccable/scripts/critique-storage.mjs latest "" --json ``` -Exit 0 returns the latest snapshot; incorporate relevant P0/P1 findings and name the snapshot read. Exit 2 means none exists. Perform an independent pass either way. +Exit 0 returns JSON with the latest snapshot's `body` and an exact `snapshot_file` identity. Retain `snapshot_file` until the end of the pass. For a local file target, the helper compares the file's exact current content fingerprint with the fingerprint captured by critique. Unchanged staged, unstaged, or untracked content remains current; any byte change, deletion, or replacement with a non-file closes the backlog it identified while preserving its trend history and exits 2. A URL target has no local fingerprint and remains current until explicitly closed. When current, incorporate relevant P0/P1 findings from `body` and name the snapshot read. Exit 2 means none exists or the target changed. Perform an independent pass either way. ## 3. Triage @@ -95,3 +95,11 @@ Walk the complete path again with mouse, keyboard, and touch where applicable. C Follow the quality guidance supplied by `context.mjs` and hooks, then run any other relevant QA commands. Context requests a manual scan only when no automatic detector is active; never add another detector pass. Fix real defects and document only narrow intentional exceptions. A clean scan does not replace visual judgment. Finish with a source diff: remove accidental churn, orphaned code, redundant values, and temporary artifacts. Ship only when the feature is functionally complete and consistently finished across the path. + +When this pass clears every Priority Issue it took from a snapshot, close that snapshot: + +```bash +node .pi/skills/impeccable/scripts/critique-storage.mjs close "" "" +``` + +This closes only the snapshot this pass actually processed; if a newer critique landed meanwhile, its backlog stays live. Do not close when no snapshot was read, when `snapshot_file` was not retained, or when Priority Issues remain. diff --git a/.pi/skills/impeccable/reference/routing.md b/.pi/skills/impeccable/reference/routing.md index 2125286b4..f3d9ab71b 100644 --- a/.pi/skills/impeccable/reference/routing.md +++ b/.pi/skills/impeccable/reference/routing.md @@ -8,7 +8,7 @@ Reason over the signals; there is no score to obey: - `setup.hasDesign` false while `setup.hasCode` true → `document` (capture the visual system). - `critique.latest` is `null` → the project has never been critiqued; for a set-up project with a real surface, offering `/impeccable critique ` is a strong default. -- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog), or re-run `critique` if the snapshot looks stale. +- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog and closes it when stale or cleared). - `git.changedFiles` pointing at one surface → scope `audit` or `polish` to those files specifically, naming them. - `devServer.running` true → `live` is available for in-browser iteration; if false, don't lead with `live`. **`live` and the bundled `detect.mjs` are web-only.** If `setup.platform` is `ios`, `android`, or `adaptive`, don't lead with either; the browser overlay and the HTML rule engine don't apply to native app code. - Otherwise group by intent (build new / improve what's there / iterate visually), tailored to the current surface and `setup.platform`. diff --git a/.pi/skills/impeccable/scripts/critique-storage.mjs b/.pi/skills/impeccable/scripts/critique-storage.mjs index f23fded37..86d297390 100644 --- a/.pi/skills/impeccable/scripts/critique-storage.mjs +++ b/.pi/skills/impeccable/scripts/critique-storage.mjs @@ -16,8 +16,9 @@ * CLI entry points (called from skill instructions): * node critique-storage.mjs slug * node critique-storage.mjs write - * node critique-storage.mjs latest + * node critique-storage.mjs latest [--json] * node critique-storage.mjs trend [limit] + * node critique-storage.mjs close * * 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 @@ -27,6 +28,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { createHash } from 'node:crypto'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { getCritiqueDir } from './lib/impeccable-paths.mjs'; import { slugFromTarget } from './lib/target-slug.mjs'; @@ -50,6 +52,45 @@ export function nowFilenameStamp(date = new Date()) { return iso.replace(/[:.]/g, '-').replace(/-\d+Z$/, 'Z'); } +/** + * Return an exact content fingerprint for a local file target. URLs and + * non-files return null because their content is not available here. + * + * The fingerprint deliberately describes bytes, not Git state or mtimes: + * critique often assesses an uncommitted file, and a later polish run should + * inherit that backlog when the bytes are unchanged regardless of staging. + */ +function resolveLocalTargetPath(target, { cwd = process.cwd() } = {}) { + if (!target || /^https?:\/\//i.test(target)) return null; + return path.isAbsolute(target) ? path.resolve(target) : path.resolve(cwd, target); +} + +function resolveTargetIdentity(target, { cwd = process.cwd() } = {}) { + if (!target || typeof target !== 'string') return null; + if (/^https?:\/\//i.test(target)) { + try { + const url = new URL(target); + const pathname = url.pathname.replace(/\/+$/, '') || '/'; + return `url:${url.origin}${pathname}`; + } catch { + return null; + } + } + const filePath = resolveLocalTargetPath(target, { cwd }); + return filePath ? `file:${filePath}` : null; +} + +export function fingerprintTarget(target, { cwd = process.cwd() } = {}) { + const filePath = resolveLocalTargetPath(target, { cwd }); + if (!filePath) return null; + try { + if (!fs.statSync(filePath).isFile()) return null; + return `sha256:${createHash('sha256').update(fs.readFileSync(filePath)).digest('hex')}`; + } catch { + return null; + } +} + /** * Write a snapshot for `slug`. `meta` carries the small structured frontmatter * keys read back by readTrend(). `body` is the human-readable critique @@ -62,14 +103,27 @@ export function writeSnapshot({ slug, meta, body, cwd = process.cwd(), now = new 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; + const contents = `${front}\n${body.trim()}\n`; + + // A second critique can finish in the same UTC second. Use exclusive + // creation and a fixed-width suffix so concurrent writers cannot replace + // history and lexical ordering still keeps collision entries newest. + for (let collision = 0; collision <= 9999; collision += 1) { + const suffix = collision === 0 ? '' : `~${String(collision).padStart(4, '0')}`; + const filePath = path.join(dir, `${timestamp}${suffix}__${slug}.md`); + try { + fs.writeFileSync(filePath, contents, { encoding: 'utf-8', flag: 'wx' }); + return filePath; + } catch (error) { + if (error?.code !== 'EEXIST') throw error; + } + } + throw new Error(`Too many critique snapshots for ${slug} at ${timestamp}`); } function serializeFrontmatter(obj) { @@ -98,6 +152,8 @@ function parseFrontmatter(text) { try { value = JSON.parse(value); } catch { /* leave as-is */ } } else if (/^-?\d+$/.test(value)) { value = Number(value); + } else if (value === 'true' || value === 'false') { + value = value === 'true'; } out[key] = value; } @@ -107,7 +163,7 @@ function parseFrontmatter(text) { /** * Return snapshot files matching `suffix`, sorted oldest → newest. */ -const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z__.+\.md$/; +const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z(?:~\d{4})?__.+\.md$/; function listSnapshots(suffix, cwd) { const dir = getCritiqueDir(cwd); @@ -118,24 +174,105 @@ function listSnapshots(suffix, cwd) { .map((f) => path.join(dir, f)); } -function readLatestSnapshotMatching(suffix, cwd) { - const filePath = listSnapshots(suffix, cwd).at(-1); +function readSnapshot(filePath) { if (!filePath) return null; const body = fs.readFileSync(filePath, 'utf-8'); return { path: filePath, body, meta: parseFrontmatter(body) }; } +function snapshotTargetIdentity(snapshot) { + const targetPath = snapshot?.meta.target_path; + return snapshot?.meta.target_identity + || (targetPath ? `file:${targetPath}` : null); +} + +function readNewestSnapshot(slug, { cwd = process.cwd() } = {}) { + return readSnapshot(listSnapshots(`__${slug}.md`, cwd).at(-1)); +} + +function readNewestSnapshotForIdentity( + slug, + targetIdentity, + { cwd = process.cwd() } = {}, +) { + const matches = listSnapshots(`__${slug}.md`, cwd) + .map(readSnapshot) + .filter((snapshot) => snapshotTargetIdentity(snapshot) === targetIdentity); + return matches.at(-1) || null; +} + /** * 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() } = {}) { - return readLatestSnapshotMatching(`__${slug}.md`, cwd); + const latest = readNewestSnapshot(slug, { cwd }); + return latest?.meta.closed === true ? null : latest; +} + +/** + * Mark one exact snapshot closed without deleting the score history consumed + * by `trend`. Exact identity matters: a newer critique may land after polish + * reads its backlog, and that newer snapshot must remain live. `snapshotFile` + * may be the absolute path returned by readLatestSnapshot() or the basename + * emitted by `latest --json`. Returns the path marked closed, or null. + */ +export function closeSnapshot(snapshotFile, { cwd = process.cwd() } = {}) { + if (!snapshotFile || typeof snapshotFile !== 'string') return null; + const dir = path.resolve(getCritiqueDir(cwd)); + const snapshotPath = path.isAbsolute(snapshotFile) + ? path.resolve(snapshotFile) + : path.resolve(dir, snapshotFile); + const filename = path.basename(snapshotPath); + if ( + path.dirname(snapshotPath) !== dir + || !SNAPSHOT_FILENAME.test(filename) + ) return null; + + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) return null; + snapshot = readSnapshot(snapshotPath); + } catch { + return null; + } + if (!snapshot || snapshot.meta.closed === true) return null; + const closedBody = snapshot.body.replace( + /^(---\r?\n[\s\S]*?)(\r?\n---)/, + '$1\nclosed: true$2', + ); + if (closedBody === snapshot.body) { + throw new Error(`Cannot close snapshot without frontmatter: ${snapshot.path}`); + } + fs.writeFileSync(snapshot.path, closedBody, 'utf-8'); + return snapshot.path; } /** Return the most recent snapshot across all targets, or null. */ export function readLatestSnapshotAcrossTargets({ cwd = process.cwd() } = {}) { - return readLatestSnapshotMatching('.md', cwd); + const snapshots = listSnapshots('.md', cwd).map(readSnapshot); + const identifiedSlugs = new Set( + snapshots + .filter((snapshot) => snapshotTargetIdentity(snapshot)) + .map((snapshot) => snapshot.meta.slug), + ); + const latestByTarget = new Map(); + for (const snapshot of snapshots) { + if (!snapshot?.meta.slug) continue; + // Slugs are lossy: distinct targets such as foo/bar and foo-bar can share + // one. Keep each known identity's latest open/closed state independent so + // closing one target cannot hide another target's live backlog. Once a + // slug has any identity-aware snapshot, its older legacy records are no + // longer independently routable and must not resurface as zombie work. + const targetIdentity = snapshotTargetIdentity(snapshot); + if (!targetIdentity && identifiedSlugs.has(snapshot.meta.slug)) continue; + const streamKey = targetIdentity || `slug:${snapshot.meta.slug}`; + latestByTarget.set(streamKey, snapshot); + } + return [...latestByTarget.values()] + .filter((snapshot) => snapshot.meta.closed !== true) + .sort((a, b) => a.path.localeCompare(b.path)) + .at(-1) || null; } /** @@ -153,9 +290,13 @@ export function readTrend(slug, { limit = 5, cwd = process.cwd() } = {}) { // Accept either a ready slug or a concrete target (path/URL) everywhere, so // callers never have to run the slug step separately. Anything containing a // path or URL marker is resolved through slugFromTarget. +function isReadySlug(value) { + return /^[a-z0-9-]+$/.test(value || '') && !value.includes('/'); +} + function coerceSlug(value) { if (!value) return null; - if (/^[a-z0-9-]+$/.test(value) && !value.includes('/')) return value; + if (isReadySlug(value)) return value; return slugFromTarget(value); } @@ -181,14 +322,124 @@ function main(argv) { if (metaArg) { try { meta = JSON.parse(metaArg); } catch { /* ignore */ } } + // The helper, not caller-provided metadata, owns the target fingerprint. + // This makes the snapshot describe the exact file bytes critique saw. + delete meta.target_fingerprint; + delete meta.target_path; + delete meta.target_identity; + const targetIdentity = resolveTargetIdentity(slugArg); + if (targetIdentity) meta.target_identity = targetIdentity; + const targetFingerprint = fingerprintTarget(slugArg); + if (targetFingerprint) { + meta.target_fingerprint = targetFingerprint; + meta.target_path = resolveLocalTargetPath(slugArg); + } const out = writeSnapshot({ slug, meta, body: raw }); process.stdout.write(`${out}\n`); return; } case 'latest': { - const latest = readLatestSnapshot(coerceSlug(args[0])); - if (!latest) { process.exit(2); } - process.stdout.write(latest.body); + const target = args[0]; + const format = args[1]; + const slug = coerceSlug(target); + if (!slug || (format && format !== '--json')) { + process.stderr.write('usage: latest [--json]\n'); + process.exit(1); + } + const targetFingerprint = fingerprintTarget(target); + const targetPath = resolveLocalTargetPath(target); + const targetIdentity = resolveTargetIdentity(target); + const readySlug = isReadySlug(target); + const newestForSlug = readNewestSnapshot(slug); + if (!newestForSlug) { process.exit(2); } + + // Concrete targets select the newest snapshot for their exact identity, + // not merely the newest filename for a lossy slug. This keeps distinct + // targets such as foo/bar and foo-bar from hiding each other's backlog. + const exactSnapshot = readNewestSnapshotForIdentity(slug, targetIdentity); + let latest = exactSnapshot; + if (!latest && !readySlug) { + // Legacy snapshots have no identity. Preserve their old explicit + // path/URL behavior only when no known target identity was selected. + latest = readNewestSnapshotForIdentity(slug, null); + } + if (!latest) latest = newestForSlug; + if (latest.meta.closed === true) { process.exit(2); } + + const recordedTargetPath = latest.meta.target_path; + const recordedTargetIdentity = snapshotTargetIdentity(latest); + const matchingIdentity = recordedTargetIdentity === targetIdentity; + + // Bare slugs remain a supported lookup mode, including for URL + // snapshots. But when a same-named local file exists, the request is + // ambiguous unless that exact file owns the snapshot identity. + if (readySlug && !recordedTargetIdentity) { + process.stderr.write( + 'ambiguous legacy snapshot target; use an explicit ./path or full URL\n', + ); + process.exit(2); + } + if (readySlug && targetPath && fs.existsSync(targetPath) && !matchingIdentity) { + process.stderr.write( + 'ambiguous snapshot slug; use an explicit ./path or remove the local name collision\n', + ); + process.exit(2); + } + + const concreteTarget = !readySlug || matchingIdentity; + if (concreteTarget && recordedTargetIdentity && !matchingIdentity) { + process.exit(2); + } + const concreteLocalTarget = concreteTarget && targetPath; + if (concreteLocalTarget && latest.meta.target_fingerprint !== targetFingerprint) { + closeSnapshot(latest.path); + process.exit(2); + } + if (format === '--json') { + process.stdout.write(JSON.stringify({ + snapshot_file: path.basename(latest.path), + body: latest.body, + }, null, 2) + '\n'); + } else { + process.stdout.write(latest.body); + } + return; + } + case 'close': { + const [slugArg, snapshotFile, ...extra] = args; + const slug = coerceSlug(slugArg); + if (!slug || !snapshotFile || extra.length > 0) { + process.stderr.write('usage: close \n'); + process.exit(1); + } + if ( + path.basename(snapshotFile) !== snapshotFile + || !SNAPSHOT_FILENAME.test(snapshotFile) + || !snapshotFile.endsWith(`__${slug}.md`) + ) process.exit(2); + + // A slug and filename are not enough to prove ownership because two + // distinct targets can normalize to the same slug. Modern snapshots + // carry a canonical identity, so require the supplied resolved target + // to match it before allowing the exact snapshot to be closed. Legacy + // snapshots without identity retain their historical close behavior. + const snapshotPath = path.join(getCritiqueDir(process.cwd()), snapshotFile); + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) process.exit(2); + snapshot = readSnapshot(snapshotPath); + } catch { + process.exit(2); + } + const recordedTargetIdentity = snapshotTargetIdentity(snapshot); + if ( + recordedTargetIdentity + && recordedTargetIdentity !== resolveTargetIdentity(slugArg) + ) process.exit(2); + + const closed = closeSnapshot(snapshotFile); + if (!closed) { process.exit(2); } + process.stdout.write(`${closed}\n`); return; } case 'trend': { @@ -197,7 +448,7 @@ function main(argv) { return; } default: - process.stderr.write('usage: critique-storage.mjs [args]\n'); + process.stderr.write('usage: critique-storage.mjs [args]\n'); process.exit(1); } } diff --git a/.qoder/skills/impeccable/reference/critique.md b/.qoder/skills/impeccable/reference/critique.md index 8faa84eb1..333678a85 100644 --- a/.qoder/skills/impeccable/reference/critique.md +++ b/.qoder/skills/impeccable/reference/critique.md @@ -1,6 +1,6 @@ ### Purpose -Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive/backlog for future commands. +Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive of that run. ### Hard Invariants @@ -84,7 +84,7 @@ After Assessment B returns usable CLI findings, reuse them. Do not rerun `detect Synthesize both assessments into a single report. Do NOT simply concatenate. Weave the findings together, noting where the LLM review and detector agree, where the detector caught issues the LLM missed, and where detector findings are false positives. -The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is only an archive/backlog for later commands. +The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is an archive of that run. Structure your feedback as a design director would: @@ -197,7 +197,7 @@ Skip this step if the Setup slug was null (vague or root-level target). IMPECCABLE_CRITIQUE_META='{"target":"","total_score":,"max_score":,"na_heuristics":"","p0_count":,"p1_count":}' \ node .qoder/skills/impeccable/scripts/critique-storage.mjs write "" ``` - `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. + `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. For a local file target, the helper also records an exact content fingerprint so polish can distinguish the assessed bytes from later edits without relying on Git state or timestamps. The helper prints the absolute path it wrote. Leave that file on disk. Polish closes it; this run does not. 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. diff --git a/.qoder/skills/impeccable/reference/polish.md b/.qoder/skills/impeccable/reference/polish.md index b392fe8fd..2a1c855d8 100644 --- a/.qoder/skills/impeccable/reference/polish.md +++ b/.qoder/skills/impeccable/reference/polish.md @@ -29,10 +29,10 @@ Use the feature yourself at the surface's representative sizes: desktop and mobi If a prior critique exists, use it as one input: ```bash -node .qoder/skills/impeccable/scripts/critique-storage.mjs latest "" +node .qoder/skills/impeccable/scripts/critique-storage.mjs latest "" --json ``` -Exit 0 returns the latest snapshot; incorporate relevant P0/P1 findings and name the snapshot read. Exit 2 means none exists. Perform an independent pass either way. +Exit 0 returns JSON with the latest snapshot's `body` and an exact `snapshot_file` identity. Retain `snapshot_file` until the end of the pass. For a local file target, the helper compares the file's exact current content fingerprint with the fingerprint captured by critique. Unchanged staged, unstaged, or untracked content remains current; any byte change, deletion, or replacement with a non-file closes the backlog it identified while preserving its trend history and exits 2. A URL target has no local fingerprint and remains current until explicitly closed. When current, incorporate relevant P0/P1 findings from `body` and name the snapshot read. Exit 2 means none exists or the target changed. Perform an independent pass either way. ## 3. Triage @@ -95,3 +95,11 @@ Walk the complete path again with mouse, keyboard, and touch where applicable. C Follow the quality guidance supplied by `context.mjs` and hooks, then run any other relevant QA commands. Context requests a manual scan only when no automatic detector is active; never add another detector pass. Fix real defects and document only narrow intentional exceptions. A clean scan does not replace visual judgment. Finish with a source diff: remove accidental churn, orphaned code, redundant values, and temporary artifacts. Ship only when the feature is functionally complete and consistently finished across the path. + +When this pass clears every Priority Issue it took from a snapshot, close that snapshot: + +```bash +node .qoder/skills/impeccable/scripts/critique-storage.mjs close "" "" +``` + +This closes only the snapshot this pass actually processed; if a newer critique landed meanwhile, its backlog stays live. Do not close when no snapshot was read, when `snapshot_file` was not retained, or when Priority Issues remain. diff --git a/.qoder/skills/impeccable/reference/routing.md b/.qoder/skills/impeccable/reference/routing.md index 4ac03b63a..4ba1d7455 100644 --- a/.qoder/skills/impeccable/reference/routing.md +++ b/.qoder/skills/impeccable/reference/routing.md @@ -8,7 +8,7 @@ Reason over the signals; there is no score to obey: - `setup.hasDesign` false while `setup.hasCode` true → `document` (capture the visual system). - `critique.latest` is `null` → the project has never been critiqued; for a set-up project with a real surface, offering `/impeccable critique ` is a strong default. -- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog), or re-run `critique` if the snapshot looks stale. +- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog and closes it when stale or cleared). - `git.changedFiles` pointing at one surface → scope `audit` or `polish` to those files specifically, naming them. - `devServer.running` true → `live` is available for in-browser iteration; if false, don't lead with `live`. **`live` and the bundled `detect.mjs` are web-only.** If `setup.platform` is `ios`, `android`, or `adaptive`, don't lead with either; the browser overlay and the HTML rule engine don't apply to native app code. - Otherwise group by intent (build new / improve what's there / iterate visually), tailored to the current surface and `setup.platform`. diff --git a/.qoder/skills/impeccable/scripts/critique-storage.mjs b/.qoder/skills/impeccable/scripts/critique-storage.mjs index f23fded37..86d297390 100644 --- a/.qoder/skills/impeccable/scripts/critique-storage.mjs +++ b/.qoder/skills/impeccable/scripts/critique-storage.mjs @@ -16,8 +16,9 @@ * CLI entry points (called from skill instructions): * node critique-storage.mjs slug * node critique-storage.mjs write - * node critique-storage.mjs latest + * node critique-storage.mjs latest [--json] * node critique-storage.mjs trend [limit] + * node critique-storage.mjs close * * 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 @@ -27,6 +28,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { createHash } from 'node:crypto'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { getCritiqueDir } from './lib/impeccable-paths.mjs'; import { slugFromTarget } from './lib/target-slug.mjs'; @@ -50,6 +52,45 @@ export function nowFilenameStamp(date = new Date()) { return iso.replace(/[:.]/g, '-').replace(/-\d+Z$/, 'Z'); } +/** + * Return an exact content fingerprint for a local file target. URLs and + * non-files return null because their content is not available here. + * + * The fingerprint deliberately describes bytes, not Git state or mtimes: + * critique often assesses an uncommitted file, and a later polish run should + * inherit that backlog when the bytes are unchanged regardless of staging. + */ +function resolveLocalTargetPath(target, { cwd = process.cwd() } = {}) { + if (!target || /^https?:\/\//i.test(target)) return null; + return path.isAbsolute(target) ? path.resolve(target) : path.resolve(cwd, target); +} + +function resolveTargetIdentity(target, { cwd = process.cwd() } = {}) { + if (!target || typeof target !== 'string') return null; + if (/^https?:\/\//i.test(target)) { + try { + const url = new URL(target); + const pathname = url.pathname.replace(/\/+$/, '') || '/'; + return `url:${url.origin}${pathname}`; + } catch { + return null; + } + } + const filePath = resolveLocalTargetPath(target, { cwd }); + return filePath ? `file:${filePath}` : null; +} + +export function fingerprintTarget(target, { cwd = process.cwd() } = {}) { + const filePath = resolveLocalTargetPath(target, { cwd }); + if (!filePath) return null; + try { + if (!fs.statSync(filePath).isFile()) return null; + return `sha256:${createHash('sha256').update(fs.readFileSync(filePath)).digest('hex')}`; + } catch { + return null; + } +} + /** * Write a snapshot for `slug`. `meta` carries the small structured frontmatter * keys read back by readTrend(). `body` is the human-readable critique @@ -62,14 +103,27 @@ export function writeSnapshot({ slug, meta, body, cwd = process.cwd(), now = new 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; + const contents = `${front}\n${body.trim()}\n`; + + // A second critique can finish in the same UTC second. Use exclusive + // creation and a fixed-width suffix so concurrent writers cannot replace + // history and lexical ordering still keeps collision entries newest. + for (let collision = 0; collision <= 9999; collision += 1) { + const suffix = collision === 0 ? '' : `~${String(collision).padStart(4, '0')}`; + const filePath = path.join(dir, `${timestamp}${suffix}__${slug}.md`); + try { + fs.writeFileSync(filePath, contents, { encoding: 'utf-8', flag: 'wx' }); + return filePath; + } catch (error) { + if (error?.code !== 'EEXIST') throw error; + } + } + throw new Error(`Too many critique snapshots for ${slug} at ${timestamp}`); } function serializeFrontmatter(obj) { @@ -98,6 +152,8 @@ function parseFrontmatter(text) { try { value = JSON.parse(value); } catch { /* leave as-is */ } } else if (/^-?\d+$/.test(value)) { value = Number(value); + } else if (value === 'true' || value === 'false') { + value = value === 'true'; } out[key] = value; } @@ -107,7 +163,7 @@ function parseFrontmatter(text) { /** * Return snapshot files matching `suffix`, sorted oldest → newest. */ -const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z__.+\.md$/; +const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z(?:~\d{4})?__.+\.md$/; function listSnapshots(suffix, cwd) { const dir = getCritiqueDir(cwd); @@ -118,24 +174,105 @@ function listSnapshots(suffix, cwd) { .map((f) => path.join(dir, f)); } -function readLatestSnapshotMatching(suffix, cwd) { - const filePath = listSnapshots(suffix, cwd).at(-1); +function readSnapshot(filePath) { if (!filePath) return null; const body = fs.readFileSync(filePath, 'utf-8'); return { path: filePath, body, meta: parseFrontmatter(body) }; } +function snapshotTargetIdentity(snapshot) { + const targetPath = snapshot?.meta.target_path; + return snapshot?.meta.target_identity + || (targetPath ? `file:${targetPath}` : null); +} + +function readNewestSnapshot(slug, { cwd = process.cwd() } = {}) { + return readSnapshot(listSnapshots(`__${slug}.md`, cwd).at(-1)); +} + +function readNewestSnapshotForIdentity( + slug, + targetIdentity, + { cwd = process.cwd() } = {}, +) { + const matches = listSnapshots(`__${slug}.md`, cwd) + .map(readSnapshot) + .filter((snapshot) => snapshotTargetIdentity(snapshot) === targetIdentity); + return matches.at(-1) || null; +} + /** * 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() } = {}) { - return readLatestSnapshotMatching(`__${slug}.md`, cwd); + const latest = readNewestSnapshot(slug, { cwd }); + return latest?.meta.closed === true ? null : latest; +} + +/** + * Mark one exact snapshot closed without deleting the score history consumed + * by `trend`. Exact identity matters: a newer critique may land after polish + * reads its backlog, and that newer snapshot must remain live. `snapshotFile` + * may be the absolute path returned by readLatestSnapshot() or the basename + * emitted by `latest --json`. Returns the path marked closed, or null. + */ +export function closeSnapshot(snapshotFile, { cwd = process.cwd() } = {}) { + if (!snapshotFile || typeof snapshotFile !== 'string') return null; + const dir = path.resolve(getCritiqueDir(cwd)); + const snapshotPath = path.isAbsolute(snapshotFile) + ? path.resolve(snapshotFile) + : path.resolve(dir, snapshotFile); + const filename = path.basename(snapshotPath); + if ( + path.dirname(snapshotPath) !== dir + || !SNAPSHOT_FILENAME.test(filename) + ) return null; + + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) return null; + snapshot = readSnapshot(snapshotPath); + } catch { + return null; + } + if (!snapshot || snapshot.meta.closed === true) return null; + const closedBody = snapshot.body.replace( + /^(---\r?\n[\s\S]*?)(\r?\n---)/, + '$1\nclosed: true$2', + ); + if (closedBody === snapshot.body) { + throw new Error(`Cannot close snapshot without frontmatter: ${snapshot.path}`); + } + fs.writeFileSync(snapshot.path, closedBody, 'utf-8'); + return snapshot.path; } /** Return the most recent snapshot across all targets, or null. */ export function readLatestSnapshotAcrossTargets({ cwd = process.cwd() } = {}) { - return readLatestSnapshotMatching('.md', cwd); + const snapshots = listSnapshots('.md', cwd).map(readSnapshot); + const identifiedSlugs = new Set( + snapshots + .filter((snapshot) => snapshotTargetIdentity(snapshot)) + .map((snapshot) => snapshot.meta.slug), + ); + const latestByTarget = new Map(); + for (const snapshot of snapshots) { + if (!snapshot?.meta.slug) continue; + // Slugs are lossy: distinct targets such as foo/bar and foo-bar can share + // one. Keep each known identity's latest open/closed state independent so + // closing one target cannot hide another target's live backlog. Once a + // slug has any identity-aware snapshot, its older legacy records are no + // longer independently routable and must not resurface as zombie work. + const targetIdentity = snapshotTargetIdentity(snapshot); + if (!targetIdentity && identifiedSlugs.has(snapshot.meta.slug)) continue; + const streamKey = targetIdentity || `slug:${snapshot.meta.slug}`; + latestByTarget.set(streamKey, snapshot); + } + return [...latestByTarget.values()] + .filter((snapshot) => snapshot.meta.closed !== true) + .sort((a, b) => a.path.localeCompare(b.path)) + .at(-1) || null; } /** @@ -153,9 +290,13 @@ export function readTrend(slug, { limit = 5, cwd = process.cwd() } = {}) { // Accept either a ready slug or a concrete target (path/URL) everywhere, so // callers never have to run the slug step separately. Anything containing a // path or URL marker is resolved through slugFromTarget. +function isReadySlug(value) { + return /^[a-z0-9-]+$/.test(value || '') && !value.includes('/'); +} + function coerceSlug(value) { if (!value) return null; - if (/^[a-z0-9-]+$/.test(value) && !value.includes('/')) return value; + if (isReadySlug(value)) return value; return slugFromTarget(value); } @@ -181,14 +322,124 @@ function main(argv) { if (metaArg) { try { meta = JSON.parse(metaArg); } catch { /* ignore */ } } + // The helper, not caller-provided metadata, owns the target fingerprint. + // This makes the snapshot describe the exact file bytes critique saw. + delete meta.target_fingerprint; + delete meta.target_path; + delete meta.target_identity; + const targetIdentity = resolveTargetIdentity(slugArg); + if (targetIdentity) meta.target_identity = targetIdentity; + const targetFingerprint = fingerprintTarget(slugArg); + if (targetFingerprint) { + meta.target_fingerprint = targetFingerprint; + meta.target_path = resolveLocalTargetPath(slugArg); + } const out = writeSnapshot({ slug, meta, body: raw }); process.stdout.write(`${out}\n`); return; } case 'latest': { - const latest = readLatestSnapshot(coerceSlug(args[0])); - if (!latest) { process.exit(2); } - process.stdout.write(latest.body); + const target = args[0]; + const format = args[1]; + const slug = coerceSlug(target); + if (!slug || (format && format !== '--json')) { + process.stderr.write('usage: latest [--json]\n'); + process.exit(1); + } + const targetFingerprint = fingerprintTarget(target); + const targetPath = resolveLocalTargetPath(target); + const targetIdentity = resolveTargetIdentity(target); + const readySlug = isReadySlug(target); + const newestForSlug = readNewestSnapshot(slug); + if (!newestForSlug) { process.exit(2); } + + // Concrete targets select the newest snapshot for their exact identity, + // not merely the newest filename for a lossy slug. This keeps distinct + // targets such as foo/bar and foo-bar from hiding each other's backlog. + const exactSnapshot = readNewestSnapshotForIdentity(slug, targetIdentity); + let latest = exactSnapshot; + if (!latest && !readySlug) { + // Legacy snapshots have no identity. Preserve their old explicit + // path/URL behavior only when no known target identity was selected. + latest = readNewestSnapshotForIdentity(slug, null); + } + if (!latest) latest = newestForSlug; + if (latest.meta.closed === true) { process.exit(2); } + + const recordedTargetPath = latest.meta.target_path; + const recordedTargetIdentity = snapshotTargetIdentity(latest); + const matchingIdentity = recordedTargetIdentity === targetIdentity; + + // Bare slugs remain a supported lookup mode, including for URL + // snapshots. But when a same-named local file exists, the request is + // ambiguous unless that exact file owns the snapshot identity. + if (readySlug && !recordedTargetIdentity) { + process.stderr.write( + 'ambiguous legacy snapshot target; use an explicit ./path or full URL\n', + ); + process.exit(2); + } + if (readySlug && targetPath && fs.existsSync(targetPath) && !matchingIdentity) { + process.stderr.write( + 'ambiguous snapshot slug; use an explicit ./path or remove the local name collision\n', + ); + process.exit(2); + } + + const concreteTarget = !readySlug || matchingIdentity; + if (concreteTarget && recordedTargetIdentity && !matchingIdentity) { + process.exit(2); + } + const concreteLocalTarget = concreteTarget && targetPath; + if (concreteLocalTarget && latest.meta.target_fingerprint !== targetFingerprint) { + closeSnapshot(latest.path); + process.exit(2); + } + if (format === '--json') { + process.stdout.write(JSON.stringify({ + snapshot_file: path.basename(latest.path), + body: latest.body, + }, null, 2) + '\n'); + } else { + process.stdout.write(latest.body); + } + return; + } + case 'close': { + const [slugArg, snapshotFile, ...extra] = args; + const slug = coerceSlug(slugArg); + if (!slug || !snapshotFile || extra.length > 0) { + process.stderr.write('usage: close \n'); + process.exit(1); + } + if ( + path.basename(snapshotFile) !== snapshotFile + || !SNAPSHOT_FILENAME.test(snapshotFile) + || !snapshotFile.endsWith(`__${slug}.md`) + ) process.exit(2); + + // A slug and filename are not enough to prove ownership because two + // distinct targets can normalize to the same slug. Modern snapshots + // carry a canonical identity, so require the supplied resolved target + // to match it before allowing the exact snapshot to be closed. Legacy + // snapshots without identity retain their historical close behavior. + const snapshotPath = path.join(getCritiqueDir(process.cwd()), snapshotFile); + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) process.exit(2); + snapshot = readSnapshot(snapshotPath); + } catch { + process.exit(2); + } + const recordedTargetIdentity = snapshotTargetIdentity(snapshot); + if ( + recordedTargetIdentity + && recordedTargetIdentity !== resolveTargetIdentity(slugArg) + ) process.exit(2); + + const closed = closeSnapshot(snapshotFile); + if (!closed) { process.exit(2); } + process.stdout.write(`${closed}\n`); return; } case 'trend': { @@ -197,7 +448,7 @@ function main(argv) { return; } default: - process.stderr.write('usage: critique-storage.mjs [args]\n'); + process.stderr.write('usage: critique-storage.mjs [args]\n'); process.exit(1); } } diff --git a/.rovodev/skills/impeccable/reference/critique.md b/.rovodev/skills/impeccable/reference/critique.md index b544990c9..a5247bf1f 100644 --- a/.rovodev/skills/impeccable/reference/critique.md +++ b/.rovodev/skills/impeccable/reference/critique.md @@ -1,6 +1,6 @@ ### Purpose -Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive/backlog for future commands. +Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive of that run. ### Hard Invariants @@ -84,7 +84,7 @@ After Assessment B returns usable CLI findings, reuse them. Do not rerun `detect Synthesize both assessments into a single report. Do NOT simply concatenate. Weave the findings together, noting where the LLM review and detector agree, where the detector caught issues the LLM missed, and where detector findings are false positives. -The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is only an archive/backlog for later commands. +The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is an archive of that run. Structure your feedback as a design director would: @@ -197,7 +197,7 @@ Skip this step if the Setup slug was null (vague or root-level target). IMPECCABLE_CRITIQUE_META='{"target":"","total_score":,"max_score":,"na_heuristics":"","p0_count":,"p1_count":}' \ node .rovodev/skills/impeccable/scripts/critique-storage.mjs write "" ``` - `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. + `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. For a local file target, the helper also records an exact content fingerprint so polish can distinguish the assessed bytes from later edits without relying on Git state or timestamps. The helper prints the absolute path it wrote. Leave that file on disk. Polish closes it; this run does not. 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. diff --git a/.rovodev/skills/impeccable/reference/polish.md b/.rovodev/skills/impeccable/reference/polish.md index b18f133ad..c3c97006e 100644 --- a/.rovodev/skills/impeccable/reference/polish.md +++ b/.rovodev/skills/impeccable/reference/polish.md @@ -29,10 +29,10 @@ Use the feature yourself at the surface's representative sizes: desktop and mobi If a prior critique exists, use it as one input: ```bash -node .rovodev/skills/impeccable/scripts/critique-storage.mjs latest "" +node .rovodev/skills/impeccable/scripts/critique-storage.mjs latest "" --json ``` -Exit 0 returns the latest snapshot; incorporate relevant P0/P1 findings and name the snapshot read. Exit 2 means none exists. Perform an independent pass either way. +Exit 0 returns JSON with the latest snapshot's `body` and an exact `snapshot_file` identity. Retain `snapshot_file` until the end of the pass. For a local file target, the helper compares the file's exact current content fingerprint with the fingerprint captured by critique. Unchanged staged, unstaged, or untracked content remains current; any byte change, deletion, or replacement with a non-file closes the backlog it identified while preserving its trend history and exits 2. A URL target has no local fingerprint and remains current until explicitly closed. When current, incorporate relevant P0/P1 findings from `body` and name the snapshot read. Exit 2 means none exists or the target changed. Perform an independent pass either way. ## 3. Triage @@ -95,3 +95,11 @@ Walk the complete path again with mouse, keyboard, and touch where applicable. C Follow the quality guidance supplied by `context.mjs` and hooks, then run any other relevant QA commands. Context requests a manual scan only when no automatic detector is active; never add another detector pass. Fix real defects and document only narrow intentional exceptions. A clean scan does not replace visual judgment. Finish with a source diff: remove accidental churn, orphaned code, redundant values, and temporary artifacts. Ship only when the feature is functionally complete and consistently finished across the path. + +When this pass clears every Priority Issue it took from a snapshot, close that snapshot: + +```bash +node .rovodev/skills/impeccable/scripts/critique-storage.mjs close "" "" +``` + +This closes only the snapshot this pass actually processed; if a newer critique landed meanwhile, its backlog stays live. Do not close when no snapshot was read, when `snapshot_file` was not retained, or when Priority Issues remain. diff --git a/.rovodev/skills/impeccable/reference/routing.md b/.rovodev/skills/impeccable/reference/routing.md index 86debb2c7..8fc3b945c 100644 --- a/.rovodev/skills/impeccable/reference/routing.md +++ b/.rovodev/skills/impeccable/reference/routing.md @@ -8,7 +8,7 @@ Reason over the signals; there is no score to obey: - `setup.hasDesign` false while `setup.hasCode` true → `document` (capture the visual system). - `critique.latest` is `null` → the project has never been critiqued; for a set-up project with a real surface, offering `/impeccable critique ` is a strong default. -- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog), or re-run `critique` if the snapshot looks stale. +- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog and closes it when stale or cleared). - `git.changedFiles` pointing at one surface → scope `audit` or `polish` to those files specifically, naming them. - `devServer.running` true → `live` is available for in-browser iteration; if false, don't lead with `live`. **`live` and the bundled `detect.mjs` are web-only.** If `setup.platform` is `ios`, `android`, or `adaptive`, don't lead with either; the browser overlay and the HTML rule engine don't apply to native app code. - Otherwise group by intent (build new / improve what's there / iterate visually), tailored to the current surface and `setup.platform`. diff --git a/.rovodev/skills/impeccable/scripts/critique-storage.mjs b/.rovodev/skills/impeccable/scripts/critique-storage.mjs index f23fded37..86d297390 100644 --- a/.rovodev/skills/impeccable/scripts/critique-storage.mjs +++ b/.rovodev/skills/impeccable/scripts/critique-storage.mjs @@ -16,8 +16,9 @@ * CLI entry points (called from skill instructions): * node critique-storage.mjs slug * node critique-storage.mjs write - * node critique-storage.mjs latest + * node critique-storage.mjs latest [--json] * node critique-storage.mjs trend [limit] + * node critique-storage.mjs close * * 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 @@ -27,6 +28,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { createHash } from 'node:crypto'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { getCritiqueDir } from './lib/impeccable-paths.mjs'; import { slugFromTarget } from './lib/target-slug.mjs'; @@ -50,6 +52,45 @@ export function nowFilenameStamp(date = new Date()) { return iso.replace(/[:.]/g, '-').replace(/-\d+Z$/, 'Z'); } +/** + * Return an exact content fingerprint for a local file target. URLs and + * non-files return null because their content is not available here. + * + * The fingerprint deliberately describes bytes, not Git state or mtimes: + * critique often assesses an uncommitted file, and a later polish run should + * inherit that backlog when the bytes are unchanged regardless of staging. + */ +function resolveLocalTargetPath(target, { cwd = process.cwd() } = {}) { + if (!target || /^https?:\/\//i.test(target)) return null; + return path.isAbsolute(target) ? path.resolve(target) : path.resolve(cwd, target); +} + +function resolveTargetIdentity(target, { cwd = process.cwd() } = {}) { + if (!target || typeof target !== 'string') return null; + if (/^https?:\/\//i.test(target)) { + try { + const url = new URL(target); + const pathname = url.pathname.replace(/\/+$/, '') || '/'; + return `url:${url.origin}${pathname}`; + } catch { + return null; + } + } + const filePath = resolveLocalTargetPath(target, { cwd }); + return filePath ? `file:${filePath}` : null; +} + +export function fingerprintTarget(target, { cwd = process.cwd() } = {}) { + const filePath = resolveLocalTargetPath(target, { cwd }); + if (!filePath) return null; + try { + if (!fs.statSync(filePath).isFile()) return null; + return `sha256:${createHash('sha256').update(fs.readFileSync(filePath)).digest('hex')}`; + } catch { + return null; + } +} + /** * Write a snapshot for `slug`. `meta` carries the small structured frontmatter * keys read back by readTrend(). `body` is the human-readable critique @@ -62,14 +103,27 @@ export function writeSnapshot({ slug, meta, body, cwd = process.cwd(), now = new 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; + const contents = `${front}\n${body.trim()}\n`; + + // A second critique can finish in the same UTC second. Use exclusive + // creation and a fixed-width suffix so concurrent writers cannot replace + // history and lexical ordering still keeps collision entries newest. + for (let collision = 0; collision <= 9999; collision += 1) { + const suffix = collision === 0 ? '' : `~${String(collision).padStart(4, '0')}`; + const filePath = path.join(dir, `${timestamp}${suffix}__${slug}.md`); + try { + fs.writeFileSync(filePath, contents, { encoding: 'utf-8', flag: 'wx' }); + return filePath; + } catch (error) { + if (error?.code !== 'EEXIST') throw error; + } + } + throw new Error(`Too many critique snapshots for ${slug} at ${timestamp}`); } function serializeFrontmatter(obj) { @@ -98,6 +152,8 @@ function parseFrontmatter(text) { try { value = JSON.parse(value); } catch { /* leave as-is */ } } else if (/^-?\d+$/.test(value)) { value = Number(value); + } else if (value === 'true' || value === 'false') { + value = value === 'true'; } out[key] = value; } @@ -107,7 +163,7 @@ function parseFrontmatter(text) { /** * Return snapshot files matching `suffix`, sorted oldest → newest. */ -const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z__.+\.md$/; +const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z(?:~\d{4})?__.+\.md$/; function listSnapshots(suffix, cwd) { const dir = getCritiqueDir(cwd); @@ -118,24 +174,105 @@ function listSnapshots(suffix, cwd) { .map((f) => path.join(dir, f)); } -function readLatestSnapshotMatching(suffix, cwd) { - const filePath = listSnapshots(suffix, cwd).at(-1); +function readSnapshot(filePath) { if (!filePath) return null; const body = fs.readFileSync(filePath, 'utf-8'); return { path: filePath, body, meta: parseFrontmatter(body) }; } +function snapshotTargetIdentity(snapshot) { + const targetPath = snapshot?.meta.target_path; + return snapshot?.meta.target_identity + || (targetPath ? `file:${targetPath}` : null); +} + +function readNewestSnapshot(slug, { cwd = process.cwd() } = {}) { + return readSnapshot(listSnapshots(`__${slug}.md`, cwd).at(-1)); +} + +function readNewestSnapshotForIdentity( + slug, + targetIdentity, + { cwd = process.cwd() } = {}, +) { + const matches = listSnapshots(`__${slug}.md`, cwd) + .map(readSnapshot) + .filter((snapshot) => snapshotTargetIdentity(snapshot) === targetIdentity); + return matches.at(-1) || null; +} + /** * 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() } = {}) { - return readLatestSnapshotMatching(`__${slug}.md`, cwd); + const latest = readNewestSnapshot(slug, { cwd }); + return latest?.meta.closed === true ? null : latest; +} + +/** + * Mark one exact snapshot closed without deleting the score history consumed + * by `trend`. Exact identity matters: a newer critique may land after polish + * reads its backlog, and that newer snapshot must remain live. `snapshotFile` + * may be the absolute path returned by readLatestSnapshot() or the basename + * emitted by `latest --json`. Returns the path marked closed, or null. + */ +export function closeSnapshot(snapshotFile, { cwd = process.cwd() } = {}) { + if (!snapshotFile || typeof snapshotFile !== 'string') return null; + const dir = path.resolve(getCritiqueDir(cwd)); + const snapshotPath = path.isAbsolute(snapshotFile) + ? path.resolve(snapshotFile) + : path.resolve(dir, snapshotFile); + const filename = path.basename(snapshotPath); + if ( + path.dirname(snapshotPath) !== dir + || !SNAPSHOT_FILENAME.test(filename) + ) return null; + + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) return null; + snapshot = readSnapshot(snapshotPath); + } catch { + return null; + } + if (!snapshot || snapshot.meta.closed === true) return null; + const closedBody = snapshot.body.replace( + /^(---\r?\n[\s\S]*?)(\r?\n---)/, + '$1\nclosed: true$2', + ); + if (closedBody === snapshot.body) { + throw new Error(`Cannot close snapshot without frontmatter: ${snapshot.path}`); + } + fs.writeFileSync(snapshot.path, closedBody, 'utf-8'); + return snapshot.path; } /** Return the most recent snapshot across all targets, or null. */ export function readLatestSnapshotAcrossTargets({ cwd = process.cwd() } = {}) { - return readLatestSnapshotMatching('.md', cwd); + const snapshots = listSnapshots('.md', cwd).map(readSnapshot); + const identifiedSlugs = new Set( + snapshots + .filter((snapshot) => snapshotTargetIdentity(snapshot)) + .map((snapshot) => snapshot.meta.slug), + ); + const latestByTarget = new Map(); + for (const snapshot of snapshots) { + if (!snapshot?.meta.slug) continue; + // Slugs are lossy: distinct targets such as foo/bar and foo-bar can share + // one. Keep each known identity's latest open/closed state independent so + // closing one target cannot hide another target's live backlog. Once a + // slug has any identity-aware snapshot, its older legacy records are no + // longer independently routable and must not resurface as zombie work. + const targetIdentity = snapshotTargetIdentity(snapshot); + if (!targetIdentity && identifiedSlugs.has(snapshot.meta.slug)) continue; + const streamKey = targetIdentity || `slug:${snapshot.meta.slug}`; + latestByTarget.set(streamKey, snapshot); + } + return [...latestByTarget.values()] + .filter((snapshot) => snapshot.meta.closed !== true) + .sort((a, b) => a.path.localeCompare(b.path)) + .at(-1) || null; } /** @@ -153,9 +290,13 @@ export function readTrend(slug, { limit = 5, cwd = process.cwd() } = {}) { // Accept either a ready slug or a concrete target (path/URL) everywhere, so // callers never have to run the slug step separately. Anything containing a // path or URL marker is resolved through slugFromTarget. +function isReadySlug(value) { + return /^[a-z0-9-]+$/.test(value || '') && !value.includes('/'); +} + function coerceSlug(value) { if (!value) return null; - if (/^[a-z0-9-]+$/.test(value) && !value.includes('/')) return value; + if (isReadySlug(value)) return value; return slugFromTarget(value); } @@ -181,14 +322,124 @@ function main(argv) { if (metaArg) { try { meta = JSON.parse(metaArg); } catch { /* ignore */ } } + // The helper, not caller-provided metadata, owns the target fingerprint. + // This makes the snapshot describe the exact file bytes critique saw. + delete meta.target_fingerprint; + delete meta.target_path; + delete meta.target_identity; + const targetIdentity = resolveTargetIdentity(slugArg); + if (targetIdentity) meta.target_identity = targetIdentity; + const targetFingerprint = fingerprintTarget(slugArg); + if (targetFingerprint) { + meta.target_fingerprint = targetFingerprint; + meta.target_path = resolveLocalTargetPath(slugArg); + } const out = writeSnapshot({ slug, meta, body: raw }); process.stdout.write(`${out}\n`); return; } case 'latest': { - const latest = readLatestSnapshot(coerceSlug(args[0])); - if (!latest) { process.exit(2); } - process.stdout.write(latest.body); + const target = args[0]; + const format = args[1]; + const slug = coerceSlug(target); + if (!slug || (format && format !== '--json')) { + process.stderr.write('usage: latest [--json]\n'); + process.exit(1); + } + const targetFingerprint = fingerprintTarget(target); + const targetPath = resolveLocalTargetPath(target); + const targetIdentity = resolveTargetIdentity(target); + const readySlug = isReadySlug(target); + const newestForSlug = readNewestSnapshot(slug); + if (!newestForSlug) { process.exit(2); } + + // Concrete targets select the newest snapshot for their exact identity, + // not merely the newest filename for a lossy slug. This keeps distinct + // targets such as foo/bar and foo-bar from hiding each other's backlog. + const exactSnapshot = readNewestSnapshotForIdentity(slug, targetIdentity); + let latest = exactSnapshot; + if (!latest && !readySlug) { + // Legacy snapshots have no identity. Preserve their old explicit + // path/URL behavior only when no known target identity was selected. + latest = readNewestSnapshotForIdentity(slug, null); + } + if (!latest) latest = newestForSlug; + if (latest.meta.closed === true) { process.exit(2); } + + const recordedTargetPath = latest.meta.target_path; + const recordedTargetIdentity = snapshotTargetIdentity(latest); + const matchingIdentity = recordedTargetIdentity === targetIdentity; + + // Bare slugs remain a supported lookup mode, including for URL + // snapshots. But when a same-named local file exists, the request is + // ambiguous unless that exact file owns the snapshot identity. + if (readySlug && !recordedTargetIdentity) { + process.stderr.write( + 'ambiguous legacy snapshot target; use an explicit ./path or full URL\n', + ); + process.exit(2); + } + if (readySlug && targetPath && fs.existsSync(targetPath) && !matchingIdentity) { + process.stderr.write( + 'ambiguous snapshot slug; use an explicit ./path or remove the local name collision\n', + ); + process.exit(2); + } + + const concreteTarget = !readySlug || matchingIdentity; + if (concreteTarget && recordedTargetIdentity && !matchingIdentity) { + process.exit(2); + } + const concreteLocalTarget = concreteTarget && targetPath; + if (concreteLocalTarget && latest.meta.target_fingerprint !== targetFingerprint) { + closeSnapshot(latest.path); + process.exit(2); + } + if (format === '--json') { + process.stdout.write(JSON.stringify({ + snapshot_file: path.basename(latest.path), + body: latest.body, + }, null, 2) + '\n'); + } else { + process.stdout.write(latest.body); + } + return; + } + case 'close': { + const [slugArg, snapshotFile, ...extra] = args; + const slug = coerceSlug(slugArg); + if (!slug || !snapshotFile || extra.length > 0) { + process.stderr.write('usage: close \n'); + process.exit(1); + } + if ( + path.basename(snapshotFile) !== snapshotFile + || !SNAPSHOT_FILENAME.test(snapshotFile) + || !snapshotFile.endsWith(`__${slug}.md`) + ) process.exit(2); + + // A slug and filename are not enough to prove ownership because two + // distinct targets can normalize to the same slug. Modern snapshots + // carry a canonical identity, so require the supplied resolved target + // to match it before allowing the exact snapshot to be closed. Legacy + // snapshots without identity retain their historical close behavior. + const snapshotPath = path.join(getCritiqueDir(process.cwd()), snapshotFile); + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) process.exit(2); + snapshot = readSnapshot(snapshotPath); + } catch { + process.exit(2); + } + const recordedTargetIdentity = snapshotTargetIdentity(snapshot); + if ( + recordedTargetIdentity + && recordedTargetIdentity !== resolveTargetIdentity(slugArg) + ) process.exit(2); + + const closed = closeSnapshot(snapshotFile); + if (!closed) { process.exit(2); } + process.stdout.write(`${closed}\n`); return; } case 'trend': { @@ -197,7 +448,7 @@ function main(argv) { return; } default: - process.stderr.write('usage: critique-storage.mjs [args]\n'); + process.stderr.write('usage: critique-storage.mjs [args]\n'); process.exit(1); } } diff --git a/.trae-cn/skills/impeccable/reference/critique.md b/.trae-cn/skills/impeccable/reference/critique.md index 52c17fc8e..bd9ae241b 100644 --- a/.trae-cn/skills/impeccable/reference/critique.md +++ b/.trae-cn/skills/impeccable/reference/critique.md @@ -1,6 +1,6 @@ ### Purpose -Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive/backlog for future commands. +Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive of that run. ### Hard Invariants @@ -84,7 +84,7 @@ After Assessment B returns usable CLI findings, reuse them. Do not rerun `detect Synthesize both assessments into a single report. Do NOT simply concatenate. Weave the findings together, noting where the LLM review and detector agree, where the detector caught issues the LLM missed, and where detector findings are false positives. -The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is only an archive/backlog for later commands. +The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is an archive of that run. Structure your feedback as a design director would: @@ -197,7 +197,7 @@ Skip this step if the Setup slug was null (vague or root-level target). IMPECCABLE_CRITIQUE_META='{"target":"","total_score":,"max_score":,"na_heuristics":"","p0_count":,"p1_count":}' \ node .trae-cn/skills/impeccable/scripts/critique-storage.mjs write "" ``` - `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. + `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. For a local file target, the helper also records an exact content fingerprint so polish can distinguish the assessed bytes from later edits without relying on Git state or timestamps. The helper prints the absolute path it wrote. Leave that file on disk. Polish closes it; this run does not. 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. diff --git a/.trae-cn/skills/impeccable/reference/polish.md b/.trae-cn/skills/impeccable/reference/polish.md index 56789b001..d983907a3 100644 --- a/.trae-cn/skills/impeccable/reference/polish.md +++ b/.trae-cn/skills/impeccable/reference/polish.md @@ -29,10 +29,10 @@ Use the feature yourself at the surface's representative sizes: desktop and mobi If a prior critique exists, use it as one input: ```bash -node .trae-cn/skills/impeccable/scripts/critique-storage.mjs latest "" +node .trae-cn/skills/impeccable/scripts/critique-storage.mjs latest "" --json ``` -Exit 0 returns the latest snapshot; incorporate relevant P0/P1 findings and name the snapshot read. Exit 2 means none exists. Perform an independent pass either way. +Exit 0 returns JSON with the latest snapshot's `body` and an exact `snapshot_file` identity. Retain `snapshot_file` until the end of the pass. For a local file target, the helper compares the file's exact current content fingerprint with the fingerprint captured by critique. Unchanged staged, unstaged, or untracked content remains current; any byte change, deletion, or replacement with a non-file closes the backlog it identified while preserving its trend history and exits 2. A URL target has no local fingerprint and remains current until explicitly closed. When current, incorporate relevant P0/P1 findings from `body` and name the snapshot read. Exit 2 means none exists or the target changed. Perform an independent pass either way. ## 3. Triage @@ -95,3 +95,11 @@ Walk the complete path again with mouse, keyboard, and touch where applicable. C Follow the quality guidance supplied by `context.mjs` and hooks, then run any other relevant QA commands. Context requests a manual scan only when no automatic detector is active; never add another detector pass. Fix real defects and document only narrow intentional exceptions. A clean scan does not replace visual judgment. Finish with a source diff: remove accidental churn, orphaned code, redundant values, and temporary artifacts. Ship only when the feature is functionally complete and consistently finished across the path. + +When this pass clears every Priority Issue it took from a snapshot, close that snapshot: + +```bash +node .trae-cn/skills/impeccable/scripts/critique-storage.mjs close "" "" +``` + +This closes only the snapshot this pass actually processed; if a newer critique landed meanwhile, its backlog stays live. Do not close when no snapshot was read, when `snapshot_file` was not retained, or when Priority Issues remain. diff --git a/.trae-cn/skills/impeccable/reference/routing.md b/.trae-cn/skills/impeccable/reference/routing.md index 4d37a18b6..aa6dae335 100644 --- a/.trae-cn/skills/impeccable/reference/routing.md +++ b/.trae-cn/skills/impeccable/reference/routing.md @@ -8,7 +8,7 @@ Reason over the signals; there is no score to obey: - `setup.hasDesign` false while `setup.hasCode` true → `document` (capture the visual system). - `critique.latest` is `null` → the project has never been critiqued; for a set-up project with a real surface, offering `/impeccable critique ` is a strong default. -- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog), or re-run `critique` if the snapshot looks stale. +- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog and closes it when stale or cleared). - `git.changedFiles` pointing at one surface → scope `audit` or `polish` to those files specifically, naming them. - `devServer.running` true → `live` is available for in-browser iteration; if false, don't lead with `live`. **`live` and the bundled `detect.mjs` are web-only.** If `setup.platform` is `ios`, `android`, or `adaptive`, don't lead with either; the browser overlay and the HTML rule engine don't apply to native app code. - Otherwise group by intent (build new / improve what's there / iterate visually), tailored to the current surface and `setup.platform`. diff --git a/.trae-cn/skills/impeccable/scripts/critique-storage.mjs b/.trae-cn/skills/impeccable/scripts/critique-storage.mjs index f23fded37..86d297390 100644 --- a/.trae-cn/skills/impeccable/scripts/critique-storage.mjs +++ b/.trae-cn/skills/impeccable/scripts/critique-storage.mjs @@ -16,8 +16,9 @@ * CLI entry points (called from skill instructions): * node critique-storage.mjs slug * node critique-storage.mjs write - * node critique-storage.mjs latest + * node critique-storage.mjs latest [--json] * node critique-storage.mjs trend [limit] + * node critique-storage.mjs close * * 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 @@ -27,6 +28,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { createHash } from 'node:crypto'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { getCritiqueDir } from './lib/impeccable-paths.mjs'; import { slugFromTarget } from './lib/target-slug.mjs'; @@ -50,6 +52,45 @@ export function nowFilenameStamp(date = new Date()) { return iso.replace(/[:.]/g, '-').replace(/-\d+Z$/, 'Z'); } +/** + * Return an exact content fingerprint for a local file target. URLs and + * non-files return null because their content is not available here. + * + * The fingerprint deliberately describes bytes, not Git state or mtimes: + * critique often assesses an uncommitted file, and a later polish run should + * inherit that backlog when the bytes are unchanged regardless of staging. + */ +function resolveLocalTargetPath(target, { cwd = process.cwd() } = {}) { + if (!target || /^https?:\/\//i.test(target)) return null; + return path.isAbsolute(target) ? path.resolve(target) : path.resolve(cwd, target); +} + +function resolveTargetIdentity(target, { cwd = process.cwd() } = {}) { + if (!target || typeof target !== 'string') return null; + if (/^https?:\/\//i.test(target)) { + try { + const url = new URL(target); + const pathname = url.pathname.replace(/\/+$/, '') || '/'; + return `url:${url.origin}${pathname}`; + } catch { + return null; + } + } + const filePath = resolveLocalTargetPath(target, { cwd }); + return filePath ? `file:${filePath}` : null; +} + +export function fingerprintTarget(target, { cwd = process.cwd() } = {}) { + const filePath = resolveLocalTargetPath(target, { cwd }); + if (!filePath) return null; + try { + if (!fs.statSync(filePath).isFile()) return null; + return `sha256:${createHash('sha256').update(fs.readFileSync(filePath)).digest('hex')}`; + } catch { + return null; + } +} + /** * Write a snapshot for `slug`. `meta` carries the small structured frontmatter * keys read back by readTrend(). `body` is the human-readable critique @@ -62,14 +103,27 @@ export function writeSnapshot({ slug, meta, body, cwd = process.cwd(), now = new 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; + const contents = `${front}\n${body.trim()}\n`; + + // A second critique can finish in the same UTC second. Use exclusive + // creation and a fixed-width suffix so concurrent writers cannot replace + // history and lexical ordering still keeps collision entries newest. + for (let collision = 0; collision <= 9999; collision += 1) { + const suffix = collision === 0 ? '' : `~${String(collision).padStart(4, '0')}`; + const filePath = path.join(dir, `${timestamp}${suffix}__${slug}.md`); + try { + fs.writeFileSync(filePath, contents, { encoding: 'utf-8', flag: 'wx' }); + return filePath; + } catch (error) { + if (error?.code !== 'EEXIST') throw error; + } + } + throw new Error(`Too many critique snapshots for ${slug} at ${timestamp}`); } function serializeFrontmatter(obj) { @@ -98,6 +152,8 @@ function parseFrontmatter(text) { try { value = JSON.parse(value); } catch { /* leave as-is */ } } else if (/^-?\d+$/.test(value)) { value = Number(value); + } else if (value === 'true' || value === 'false') { + value = value === 'true'; } out[key] = value; } @@ -107,7 +163,7 @@ function parseFrontmatter(text) { /** * Return snapshot files matching `suffix`, sorted oldest → newest. */ -const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z__.+\.md$/; +const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z(?:~\d{4})?__.+\.md$/; function listSnapshots(suffix, cwd) { const dir = getCritiqueDir(cwd); @@ -118,24 +174,105 @@ function listSnapshots(suffix, cwd) { .map((f) => path.join(dir, f)); } -function readLatestSnapshotMatching(suffix, cwd) { - const filePath = listSnapshots(suffix, cwd).at(-1); +function readSnapshot(filePath) { if (!filePath) return null; const body = fs.readFileSync(filePath, 'utf-8'); return { path: filePath, body, meta: parseFrontmatter(body) }; } +function snapshotTargetIdentity(snapshot) { + const targetPath = snapshot?.meta.target_path; + return snapshot?.meta.target_identity + || (targetPath ? `file:${targetPath}` : null); +} + +function readNewestSnapshot(slug, { cwd = process.cwd() } = {}) { + return readSnapshot(listSnapshots(`__${slug}.md`, cwd).at(-1)); +} + +function readNewestSnapshotForIdentity( + slug, + targetIdentity, + { cwd = process.cwd() } = {}, +) { + const matches = listSnapshots(`__${slug}.md`, cwd) + .map(readSnapshot) + .filter((snapshot) => snapshotTargetIdentity(snapshot) === targetIdentity); + return matches.at(-1) || null; +} + /** * 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() } = {}) { - return readLatestSnapshotMatching(`__${slug}.md`, cwd); + const latest = readNewestSnapshot(slug, { cwd }); + return latest?.meta.closed === true ? null : latest; +} + +/** + * Mark one exact snapshot closed without deleting the score history consumed + * by `trend`. Exact identity matters: a newer critique may land after polish + * reads its backlog, and that newer snapshot must remain live. `snapshotFile` + * may be the absolute path returned by readLatestSnapshot() or the basename + * emitted by `latest --json`. Returns the path marked closed, or null. + */ +export function closeSnapshot(snapshotFile, { cwd = process.cwd() } = {}) { + if (!snapshotFile || typeof snapshotFile !== 'string') return null; + const dir = path.resolve(getCritiqueDir(cwd)); + const snapshotPath = path.isAbsolute(snapshotFile) + ? path.resolve(snapshotFile) + : path.resolve(dir, snapshotFile); + const filename = path.basename(snapshotPath); + if ( + path.dirname(snapshotPath) !== dir + || !SNAPSHOT_FILENAME.test(filename) + ) return null; + + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) return null; + snapshot = readSnapshot(snapshotPath); + } catch { + return null; + } + if (!snapshot || snapshot.meta.closed === true) return null; + const closedBody = snapshot.body.replace( + /^(---\r?\n[\s\S]*?)(\r?\n---)/, + '$1\nclosed: true$2', + ); + if (closedBody === snapshot.body) { + throw new Error(`Cannot close snapshot without frontmatter: ${snapshot.path}`); + } + fs.writeFileSync(snapshot.path, closedBody, 'utf-8'); + return snapshot.path; } /** Return the most recent snapshot across all targets, or null. */ export function readLatestSnapshotAcrossTargets({ cwd = process.cwd() } = {}) { - return readLatestSnapshotMatching('.md', cwd); + const snapshots = listSnapshots('.md', cwd).map(readSnapshot); + const identifiedSlugs = new Set( + snapshots + .filter((snapshot) => snapshotTargetIdentity(snapshot)) + .map((snapshot) => snapshot.meta.slug), + ); + const latestByTarget = new Map(); + for (const snapshot of snapshots) { + if (!snapshot?.meta.slug) continue; + // Slugs are lossy: distinct targets such as foo/bar and foo-bar can share + // one. Keep each known identity's latest open/closed state independent so + // closing one target cannot hide another target's live backlog. Once a + // slug has any identity-aware snapshot, its older legacy records are no + // longer independently routable and must not resurface as zombie work. + const targetIdentity = snapshotTargetIdentity(snapshot); + if (!targetIdentity && identifiedSlugs.has(snapshot.meta.slug)) continue; + const streamKey = targetIdentity || `slug:${snapshot.meta.slug}`; + latestByTarget.set(streamKey, snapshot); + } + return [...latestByTarget.values()] + .filter((snapshot) => snapshot.meta.closed !== true) + .sort((a, b) => a.path.localeCompare(b.path)) + .at(-1) || null; } /** @@ -153,9 +290,13 @@ export function readTrend(slug, { limit = 5, cwd = process.cwd() } = {}) { // Accept either a ready slug or a concrete target (path/URL) everywhere, so // callers never have to run the slug step separately. Anything containing a // path or URL marker is resolved through slugFromTarget. +function isReadySlug(value) { + return /^[a-z0-9-]+$/.test(value || '') && !value.includes('/'); +} + function coerceSlug(value) { if (!value) return null; - if (/^[a-z0-9-]+$/.test(value) && !value.includes('/')) return value; + if (isReadySlug(value)) return value; return slugFromTarget(value); } @@ -181,14 +322,124 @@ function main(argv) { if (metaArg) { try { meta = JSON.parse(metaArg); } catch { /* ignore */ } } + // The helper, not caller-provided metadata, owns the target fingerprint. + // This makes the snapshot describe the exact file bytes critique saw. + delete meta.target_fingerprint; + delete meta.target_path; + delete meta.target_identity; + const targetIdentity = resolveTargetIdentity(slugArg); + if (targetIdentity) meta.target_identity = targetIdentity; + const targetFingerprint = fingerprintTarget(slugArg); + if (targetFingerprint) { + meta.target_fingerprint = targetFingerprint; + meta.target_path = resolveLocalTargetPath(slugArg); + } const out = writeSnapshot({ slug, meta, body: raw }); process.stdout.write(`${out}\n`); return; } case 'latest': { - const latest = readLatestSnapshot(coerceSlug(args[0])); - if (!latest) { process.exit(2); } - process.stdout.write(latest.body); + const target = args[0]; + const format = args[1]; + const slug = coerceSlug(target); + if (!slug || (format && format !== '--json')) { + process.stderr.write('usage: latest [--json]\n'); + process.exit(1); + } + const targetFingerprint = fingerprintTarget(target); + const targetPath = resolveLocalTargetPath(target); + const targetIdentity = resolveTargetIdentity(target); + const readySlug = isReadySlug(target); + const newestForSlug = readNewestSnapshot(slug); + if (!newestForSlug) { process.exit(2); } + + // Concrete targets select the newest snapshot for their exact identity, + // not merely the newest filename for a lossy slug. This keeps distinct + // targets such as foo/bar and foo-bar from hiding each other's backlog. + const exactSnapshot = readNewestSnapshotForIdentity(slug, targetIdentity); + let latest = exactSnapshot; + if (!latest && !readySlug) { + // Legacy snapshots have no identity. Preserve their old explicit + // path/URL behavior only when no known target identity was selected. + latest = readNewestSnapshotForIdentity(slug, null); + } + if (!latest) latest = newestForSlug; + if (latest.meta.closed === true) { process.exit(2); } + + const recordedTargetPath = latest.meta.target_path; + const recordedTargetIdentity = snapshotTargetIdentity(latest); + const matchingIdentity = recordedTargetIdentity === targetIdentity; + + // Bare slugs remain a supported lookup mode, including for URL + // snapshots. But when a same-named local file exists, the request is + // ambiguous unless that exact file owns the snapshot identity. + if (readySlug && !recordedTargetIdentity) { + process.stderr.write( + 'ambiguous legacy snapshot target; use an explicit ./path or full URL\n', + ); + process.exit(2); + } + if (readySlug && targetPath && fs.existsSync(targetPath) && !matchingIdentity) { + process.stderr.write( + 'ambiguous snapshot slug; use an explicit ./path or remove the local name collision\n', + ); + process.exit(2); + } + + const concreteTarget = !readySlug || matchingIdentity; + if (concreteTarget && recordedTargetIdentity && !matchingIdentity) { + process.exit(2); + } + const concreteLocalTarget = concreteTarget && targetPath; + if (concreteLocalTarget && latest.meta.target_fingerprint !== targetFingerprint) { + closeSnapshot(latest.path); + process.exit(2); + } + if (format === '--json') { + process.stdout.write(JSON.stringify({ + snapshot_file: path.basename(latest.path), + body: latest.body, + }, null, 2) + '\n'); + } else { + process.stdout.write(latest.body); + } + return; + } + case 'close': { + const [slugArg, snapshotFile, ...extra] = args; + const slug = coerceSlug(slugArg); + if (!slug || !snapshotFile || extra.length > 0) { + process.stderr.write('usage: close \n'); + process.exit(1); + } + if ( + path.basename(snapshotFile) !== snapshotFile + || !SNAPSHOT_FILENAME.test(snapshotFile) + || !snapshotFile.endsWith(`__${slug}.md`) + ) process.exit(2); + + // A slug and filename are not enough to prove ownership because two + // distinct targets can normalize to the same slug. Modern snapshots + // carry a canonical identity, so require the supplied resolved target + // to match it before allowing the exact snapshot to be closed. Legacy + // snapshots without identity retain their historical close behavior. + const snapshotPath = path.join(getCritiqueDir(process.cwd()), snapshotFile); + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) process.exit(2); + snapshot = readSnapshot(snapshotPath); + } catch { + process.exit(2); + } + const recordedTargetIdentity = snapshotTargetIdentity(snapshot); + if ( + recordedTargetIdentity + && recordedTargetIdentity !== resolveTargetIdentity(slugArg) + ) process.exit(2); + + const closed = closeSnapshot(snapshotFile); + if (!closed) { process.exit(2); } + process.stdout.write(`${closed}\n`); return; } case 'trend': { @@ -197,7 +448,7 @@ function main(argv) { return; } default: - process.stderr.write('usage: critique-storage.mjs [args]\n'); + process.stderr.write('usage: critique-storage.mjs [args]\n'); process.exit(1); } } diff --git a/.trae/skills/impeccable/reference/critique.md b/.trae/skills/impeccable/reference/critique.md index 75d5063f5..420271085 100644 --- a/.trae/skills/impeccable/reference/critique.md +++ b/.trae/skills/impeccable/reference/critique.md @@ -1,6 +1,6 @@ ### Purpose -Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive/backlog for future commands. +Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive of that run. ### Hard Invariants @@ -84,7 +84,7 @@ After Assessment B returns usable CLI findings, reuse them. Do not rerun `detect Synthesize both assessments into a single report. Do NOT simply concatenate. Weave the findings together, noting where the LLM review and detector agree, where the detector caught issues the LLM missed, and where detector findings are false positives. -The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is only an archive/backlog for later commands. +The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is an archive of that run. Structure your feedback as a design director would: @@ -197,7 +197,7 @@ Skip this step if the Setup slug was null (vague or root-level target). IMPECCABLE_CRITIQUE_META='{"target":"","total_score":,"max_score":,"na_heuristics":"","p0_count":,"p1_count":}' \ node .trae/skills/impeccable/scripts/critique-storage.mjs write "" ``` - `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. + `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. For a local file target, the helper also records an exact content fingerprint so polish can distinguish the assessed bytes from later edits without relying on Git state or timestamps. The helper prints the absolute path it wrote. Leave that file on disk. Polish closes it; this run does not. 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. diff --git a/.trae/skills/impeccable/reference/polish.md b/.trae/skills/impeccable/reference/polish.md index ffd4ea340..4e09fe076 100644 --- a/.trae/skills/impeccable/reference/polish.md +++ b/.trae/skills/impeccable/reference/polish.md @@ -29,10 +29,10 @@ Use the feature yourself at the surface's representative sizes: desktop and mobi If a prior critique exists, use it as one input: ```bash -node .trae/skills/impeccable/scripts/critique-storage.mjs latest "" +node .trae/skills/impeccable/scripts/critique-storage.mjs latest "" --json ``` -Exit 0 returns the latest snapshot; incorporate relevant P0/P1 findings and name the snapshot read. Exit 2 means none exists. Perform an independent pass either way. +Exit 0 returns JSON with the latest snapshot's `body` and an exact `snapshot_file` identity. Retain `snapshot_file` until the end of the pass. For a local file target, the helper compares the file's exact current content fingerprint with the fingerprint captured by critique. Unchanged staged, unstaged, or untracked content remains current; any byte change, deletion, or replacement with a non-file closes the backlog it identified while preserving its trend history and exits 2. A URL target has no local fingerprint and remains current until explicitly closed. When current, incorporate relevant P0/P1 findings from `body` and name the snapshot read. Exit 2 means none exists or the target changed. Perform an independent pass either way. ## 3. Triage @@ -95,3 +95,11 @@ Walk the complete path again with mouse, keyboard, and touch where applicable. C Follow the quality guidance supplied by `context.mjs` and hooks, then run any other relevant QA commands. Context requests a manual scan only when no automatic detector is active; never add another detector pass. Fix real defects and document only narrow intentional exceptions. A clean scan does not replace visual judgment. Finish with a source diff: remove accidental churn, orphaned code, redundant values, and temporary artifacts. Ship only when the feature is functionally complete and consistently finished across the path. + +When this pass clears every Priority Issue it took from a snapshot, close that snapshot: + +```bash +node .trae/skills/impeccable/scripts/critique-storage.mjs close "" "" +``` + +This closes only the snapshot this pass actually processed; if a newer critique landed meanwhile, its backlog stays live. Do not close when no snapshot was read, when `snapshot_file` was not retained, or when Priority Issues remain. diff --git a/.trae/skills/impeccable/reference/routing.md b/.trae/skills/impeccable/reference/routing.md index 57c81ff86..ae69f516a 100644 --- a/.trae/skills/impeccable/reference/routing.md +++ b/.trae/skills/impeccable/reference/routing.md @@ -8,7 +8,7 @@ Reason over the signals; there is no score to obey: - `setup.hasDesign` false while `setup.hasCode` true → `document` (capture the visual system). - `critique.latest` is `null` → the project has never been critiqued; for a set-up project with a real surface, offering `/impeccable critique ` is a strong default. -- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog), or re-run `critique` if the snapshot looks stale. +- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog and closes it when stale or cleared). - `git.changedFiles` pointing at one surface → scope `audit` or `polish` to those files specifically, naming them. - `devServer.running` true → `live` is available for in-browser iteration; if false, don't lead with `live`. **`live` and the bundled `detect.mjs` are web-only.** If `setup.platform` is `ios`, `android`, or `adaptive`, don't lead with either; the browser overlay and the HTML rule engine don't apply to native app code. - Otherwise group by intent (build new / improve what's there / iterate visually), tailored to the current surface and `setup.platform`. diff --git a/.trae/skills/impeccable/scripts/critique-storage.mjs b/.trae/skills/impeccable/scripts/critique-storage.mjs index f23fded37..86d297390 100644 --- a/.trae/skills/impeccable/scripts/critique-storage.mjs +++ b/.trae/skills/impeccable/scripts/critique-storage.mjs @@ -16,8 +16,9 @@ * CLI entry points (called from skill instructions): * node critique-storage.mjs slug * node critique-storage.mjs write - * node critique-storage.mjs latest + * node critique-storage.mjs latest [--json] * node critique-storage.mjs trend [limit] + * node critique-storage.mjs close * * 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 @@ -27,6 +28,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { createHash } from 'node:crypto'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { getCritiqueDir } from './lib/impeccable-paths.mjs'; import { slugFromTarget } from './lib/target-slug.mjs'; @@ -50,6 +52,45 @@ export function nowFilenameStamp(date = new Date()) { return iso.replace(/[:.]/g, '-').replace(/-\d+Z$/, 'Z'); } +/** + * Return an exact content fingerprint for a local file target. URLs and + * non-files return null because their content is not available here. + * + * The fingerprint deliberately describes bytes, not Git state or mtimes: + * critique often assesses an uncommitted file, and a later polish run should + * inherit that backlog when the bytes are unchanged regardless of staging. + */ +function resolveLocalTargetPath(target, { cwd = process.cwd() } = {}) { + if (!target || /^https?:\/\//i.test(target)) return null; + return path.isAbsolute(target) ? path.resolve(target) : path.resolve(cwd, target); +} + +function resolveTargetIdentity(target, { cwd = process.cwd() } = {}) { + if (!target || typeof target !== 'string') return null; + if (/^https?:\/\//i.test(target)) { + try { + const url = new URL(target); + const pathname = url.pathname.replace(/\/+$/, '') || '/'; + return `url:${url.origin}${pathname}`; + } catch { + return null; + } + } + const filePath = resolveLocalTargetPath(target, { cwd }); + return filePath ? `file:${filePath}` : null; +} + +export function fingerprintTarget(target, { cwd = process.cwd() } = {}) { + const filePath = resolveLocalTargetPath(target, { cwd }); + if (!filePath) return null; + try { + if (!fs.statSync(filePath).isFile()) return null; + return `sha256:${createHash('sha256').update(fs.readFileSync(filePath)).digest('hex')}`; + } catch { + return null; + } +} + /** * Write a snapshot for `slug`. `meta` carries the small structured frontmatter * keys read back by readTrend(). `body` is the human-readable critique @@ -62,14 +103,27 @@ export function writeSnapshot({ slug, meta, body, cwd = process.cwd(), now = new 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; + const contents = `${front}\n${body.trim()}\n`; + + // A second critique can finish in the same UTC second. Use exclusive + // creation and a fixed-width suffix so concurrent writers cannot replace + // history and lexical ordering still keeps collision entries newest. + for (let collision = 0; collision <= 9999; collision += 1) { + const suffix = collision === 0 ? '' : `~${String(collision).padStart(4, '0')}`; + const filePath = path.join(dir, `${timestamp}${suffix}__${slug}.md`); + try { + fs.writeFileSync(filePath, contents, { encoding: 'utf-8', flag: 'wx' }); + return filePath; + } catch (error) { + if (error?.code !== 'EEXIST') throw error; + } + } + throw new Error(`Too many critique snapshots for ${slug} at ${timestamp}`); } function serializeFrontmatter(obj) { @@ -98,6 +152,8 @@ function parseFrontmatter(text) { try { value = JSON.parse(value); } catch { /* leave as-is */ } } else if (/^-?\d+$/.test(value)) { value = Number(value); + } else if (value === 'true' || value === 'false') { + value = value === 'true'; } out[key] = value; } @@ -107,7 +163,7 @@ function parseFrontmatter(text) { /** * Return snapshot files matching `suffix`, sorted oldest → newest. */ -const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z__.+\.md$/; +const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z(?:~\d{4})?__.+\.md$/; function listSnapshots(suffix, cwd) { const dir = getCritiqueDir(cwd); @@ -118,24 +174,105 @@ function listSnapshots(suffix, cwd) { .map((f) => path.join(dir, f)); } -function readLatestSnapshotMatching(suffix, cwd) { - const filePath = listSnapshots(suffix, cwd).at(-1); +function readSnapshot(filePath) { if (!filePath) return null; const body = fs.readFileSync(filePath, 'utf-8'); return { path: filePath, body, meta: parseFrontmatter(body) }; } +function snapshotTargetIdentity(snapshot) { + const targetPath = snapshot?.meta.target_path; + return snapshot?.meta.target_identity + || (targetPath ? `file:${targetPath}` : null); +} + +function readNewestSnapshot(slug, { cwd = process.cwd() } = {}) { + return readSnapshot(listSnapshots(`__${slug}.md`, cwd).at(-1)); +} + +function readNewestSnapshotForIdentity( + slug, + targetIdentity, + { cwd = process.cwd() } = {}, +) { + const matches = listSnapshots(`__${slug}.md`, cwd) + .map(readSnapshot) + .filter((snapshot) => snapshotTargetIdentity(snapshot) === targetIdentity); + return matches.at(-1) || null; +} + /** * 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() } = {}) { - return readLatestSnapshotMatching(`__${slug}.md`, cwd); + const latest = readNewestSnapshot(slug, { cwd }); + return latest?.meta.closed === true ? null : latest; +} + +/** + * Mark one exact snapshot closed without deleting the score history consumed + * by `trend`. Exact identity matters: a newer critique may land after polish + * reads its backlog, and that newer snapshot must remain live. `snapshotFile` + * may be the absolute path returned by readLatestSnapshot() or the basename + * emitted by `latest --json`. Returns the path marked closed, or null. + */ +export function closeSnapshot(snapshotFile, { cwd = process.cwd() } = {}) { + if (!snapshotFile || typeof snapshotFile !== 'string') return null; + const dir = path.resolve(getCritiqueDir(cwd)); + const snapshotPath = path.isAbsolute(snapshotFile) + ? path.resolve(snapshotFile) + : path.resolve(dir, snapshotFile); + const filename = path.basename(snapshotPath); + if ( + path.dirname(snapshotPath) !== dir + || !SNAPSHOT_FILENAME.test(filename) + ) return null; + + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) return null; + snapshot = readSnapshot(snapshotPath); + } catch { + return null; + } + if (!snapshot || snapshot.meta.closed === true) return null; + const closedBody = snapshot.body.replace( + /^(---\r?\n[\s\S]*?)(\r?\n---)/, + '$1\nclosed: true$2', + ); + if (closedBody === snapshot.body) { + throw new Error(`Cannot close snapshot without frontmatter: ${snapshot.path}`); + } + fs.writeFileSync(snapshot.path, closedBody, 'utf-8'); + return snapshot.path; } /** Return the most recent snapshot across all targets, or null. */ export function readLatestSnapshotAcrossTargets({ cwd = process.cwd() } = {}) { - return readLatestSnapshotMatching('.md', cwd); + const snapshots = listSnapshots('.md', cwd).map(readSnapshot); + const identifiedSlugs = new Set( + snapshots + .filter((snapshot) => snapshotTargetIdentity(snapshot)) + .map((snapshot) => snapshot.meta.slug), + ); + const latestByTarget = new Map(); + for (const snapshot of snapshots) { + if (!snapshot?.meta.slug) continue; + // Slugs are lossy: distinct targets such as foo/bar and foo-bar can share + // one. Keep each known identity's latest open/closed state independent so + // closing one target cannot hide another target's live backlog. Once a + // slug has any identity-aware snapshot, its older legacy records are no + // longer independently routable and must not resurface as zombie work. + const targetIdentity = snapshotTargetIdentity(snapshot); + if (!targetIdentity && identifiedSlugs.has(snapshot.meta.slug)) continue; + const streamKey = targetIdentity || `slug:${snapshot.meta.slug}`; + latestByTarget.set(streamKey, snapshot); + } + return [...latestByTarget.values()] + .filter((snapshot) => snapshot.meta.closed !== true) + .sort((a, b) => a.path.localeCompare(b.path)) + .at(-1) || null; } /** @@ -153,9 +290,13 @@ export function readTrend(slug, { limit = 5, cwd = process.cwd() } = {}) { // Accept either a ready slug or a concrete target (path/URL) everywhere, so // callers never have to run the slug step separately. Anything containing a // path or URL marker is resolved through slugFromTarget. +function isReadySlug(value) { + return /^[a-z0-9-]+$/.test(value || '') && !value.includes('/'); +} + function coerceSlug(value) { if (!value) return null; - if (/^[a-z0-9-]+$/.test(value) && !value.includes('/')) return value; + if (isReadySlug(value)) return value; return slugFromTarget(value); } @@ -181,14 +322,124 @@ function main(argv) { if (metaArg) { try { meta = JSON.parse(metaArg); } catch { /* ignore */ } } + // The helper, not caller-provided metadata, owns the target fingerprint. + // This makes the snapshot describe the exact file bytes critique saw. + delete meta.target_fingerprint; + delete meta.target_path; + delete meta.target_identity; + const targetIdentity = resolveTargetIdentity(slugArg); + if (targetIdentity) meta.target_identity = targetIdentity; + const targetFingerprint = fingerprintTarget(slugArg); + if (targetFingerprint) { + meta.target_fingerprint = targetFingerprint; + meta.target_path = resolveLocalTargetPath(slugArg); + } const out = writeSnapshot({ slug, meta, body: raw }); process.stdout.write(`${out}\n`); return; } case 'latest': { - const latest = readLatestSnapshot(coerceSlug(args[0])); - if (!latest) { process.exit(2); } - process.stdout.write(latest.body); + const target = args[0]; + const format = args[1]; + const slug = coerceSlug(target); + if (!slug || (format && format !== '--json')) { + process.stderr.write('usage: latest [--json]\n'); + process.exit(1); + } + const targetFingerprint = fingerprintTarget(target); + const targetPath = resolveLocalTargetPath(target); + const targetIdentity = resolveTargetIdentity(target); + const readySlug = isReadySlug(target); + const newestForSlug = readNewestSnapshot(slug); + if (!newestForSlug) { process.exit(2); } + + // Concrete targets select the newest snapshot for their exact identity, + // not merely the newest filename for a lossy slug. This keeps distinct + // targets such as foo/bar and foo-bar from hiding each other's backlog. + const exactSnapshot = readNewestSnapshotForIdentity(slug, targetIdentity); + let latest = exactSnapshot; + if (!latest && !readySlug) { + // Legacy snapshots have no identity. Preserve their old explicit + // path/URL behavior only when no known target identity was selected. + latest = readNewestSnapshotForIdentity(slug, null); + } + if (!latest) latest = newestForSlug; + if (latest.meta.closed === true) { process.exit(2); } + + const recordedTargetPath = latest.meta.target_path; + const recordedTargetIdentity = snapshotTargetIdentity(latest); + const matchingIdentity = recordedTargetIdentity === targetIdentity; + + // Bare slugs remain a supported lookup mode, including for URL + // snapshots. But when a same-named local file exists, the request is + // ambiguous unless that exact file owns the snapshot identity. + if (readySlug && !recordedTargetIdentity) { + process.stderr.write( + 'ambiguous legacy snapshot target; use an explicit ./path or full URL\n', + ); + process.exit(2); + } + if (readySlug && targetPath && fs.existsSync(targetPath) && !matchingIdentity) { + process.stderr.write( + 'ambiguous snapshot slug; use an explicit ./path or remove the local name collision\n', + ); + process.exit(2); + } + + const concreteTarget = !readySlug || matchingIdentity; + if (concreteTarget && recordedTargetIdentity && !matchingIdentity) { + process.exit(2); + } + const concreteLocalTarget = concreteTarget && targetPath; + if (concreteLocalTarget && latest.meta.target_fingerprint !== targetFingerprint) { + closeSnapshot(latest.path); + process.exit(2); + } + if (format === '--json') { + process.stdout.write(JSON.stringify({ + snapshot_file: path.basename(latest.path), + body: latest.body, + }, null, 2) + '\n'); + } else { + process.stdout.write(latest.body); + } + return; + } + case 'close': { + const [slugArg, snapshotFile, ...extra] = args; + const slug = coerceSlug(slugArg); + if (!slug || !snapshotFile || extra.length > 0) { + process.stderr.write('usage: close \n'); + process.exit(1); + } + if ( + path.basename(snapshotFile) !== snapshotFile + || !SNAPSHOT_FILENAME.test(snapshotFile) + || !snapshotFile.endsWith(`__${slug}.md`) + ) process.exit(2); + + // A slug and filename are not enough to prove ownership because two + // distinct targets can normalize to the same slug. Modern snapshots + // carry a canonical identity, so require the supplied resolved target + // to match it before allowing the exact snapshot to be closed. Legacy + // snapshots without identity retain their historical close behavior. + const snapshotPath = path.join(getCritiqueDir(process.cwd()), snapshotFile); + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) process.exit(2); + snapshot = readSnapshot(snapshotPath); + } catch { + process.exit(2); + } + const recordedTargetIdentity = snapshotTargetIdentity(snapshot); + if ( + recordedTargetIdentity + && recordedTargetIdentity !== resolveTargetIdentity(slugArg) + ) process.exit(2); + + const closed = closeSnapshot(snapshotFile); + if (!closed) { process.exit(2); } + process.stdout.write(`${closed}\n`); return; } case 'trend': { @@ -197,7 +448,7 @@ function main(argv) { return; } default: - process.stderr.write('usage: critique-storage.mjs [args]\n'); + process.stderr.write('usage: critique-storage.mjs [args]\n'); process.exit(1); } } diff --git a/.vibe/skills/impeccable/reference/critique.md b/.vibe/skills/impeccable/reference/critique.md index 0c65a923c..82b05ed43 100644 --- a/.vibe/skills/impeccable/reference/critique.md +++ b/.vibe/skills/impeccable/reference/critique.md @@ -1,6 +1,6 @@ ### Purpose -Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive/backlog for future commands. +Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive of that run. ### Hard Invariants @@ -84,7 +84,7 @@ After Assessment B returns usable CLI findings, reuse them. Do not rerun `detect Synthesize both assessments into a single report. Do NOT simply concatenate. Weave the findings together, noting where the LLM review and detector agree, where the detector caught issues the LLM missed, and where detector findings are false positives. -The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is only an archive/backlog for later commands. +The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is an archive of that run. Structure your feedback as a design director would: @@ -197,7 +197,7 @@ Skip this step if the Setup slug was null (vague or root-level target). IMPECCABLE_CRITIQUE_META='{"target":"","total_score":,"max_score":,"na_heuristics":"","p0_count":,"p1_count":}' \ node .vibe/skills/impeccable/scripts/critique-storage.mjs write "" ``` - `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. + `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. For a local file target, the helper also records an exact content fingerprint so polish can distinguish the assessed bytes from later edits without relying on Git state or timestamps. The helper prints the absolute path it wrote. Leave that file on disk. Polish closes it; this run does not. 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. diff --git a/.vibe/skills/impeccable/reference/polish.md b/.vibe/skills/impeccable/reference/polish.md index 7e7d52c3f..ae81c4510 100644 --- a/.vibe/skills/impeccable/reference/polish.md +++ b/.vibe/skills/impeccable/reference/polish.md @@ -29,10 +29,10 @@ Use the feature yourself at the surface's representative sizes: desktop and mobi If a prior critique exists, use it as one input: ```bash -node .vibe/skills/impeccable/scripts/critique-storage.mjs latest "" +node .vibe/skills/impeccable/scripts/critique-storage.mjs latest "" --json ``` -Exit 0 returns the latest snapshot; incorporate relevant P0/P1 findings and name the snapshot read. Exit 2 means none exists. Perform an independent pass either way. +Exit 0 returns JSON with the latest snapshot's `body` and an exact `snapshot_file` identity. Retain `snapshot_file` until the end of the pass. For a local file target, the helper compares the file's exact current content fingerprint with the fingerprint captured by critique. Unchanged staged, unstaged, or untracked content remains current; any byte change, deletion, or replacement with a non-file closes the backlog it identified while preserving its trend history and exits 2. A URL target has no local fingerprint and remains current until explicitly closed. When current, incorporate relevant P0/P1 findings from `body` and name the snapshot read. Exit 2 means none exists or the target changed. Perform an independent pass either way. ## 3. Triage @@ -95,3 +95,11 @@ Walk the complete path again with mouse, keyboard, and touch where applicable. C Follow the quality guidance supplied by `context.mjs` and hooks, then run any other relevant QA commands. Context requests a manual scan only when no automatic detector is active; never add another detector pass. Fix real defects and document only narrow intentional exceptions. A clean scan does not replace visual judgment. Finish with a source diff: remove accidental churn, orphaned code, redundant values, and temporary artifacts. Ship only when the feature is functionally complete and consistently finished across the path. + +When this pass clears every Priority Issue it took from a snapshot, close that snapshot: + +```bash +node .vibe/skills/impeccable/scripts/critique-storage.mjs close "" "" +``` + +This closes only the snapshot this pass actually processed; if a newer critique landed meanwhile, its backlog stays live. Do not close when no snapshot was read, when `snapshot_file` was not retained, or when Priority Issues remain. diff --git a/.vibe/skills/impeccable/reference/routing.md b/.vibe/skills/impeccable/reference/routing.md index aa0d1f969..f8bbdafb8 100644 --- a/.vibe/skills/impeccable/reference/routing.md +++ b/.vibe/skills/impeccable/reference/routing.md @@ -8,7 +8,7 @@ Reason over the signals; there is no score to obey: - `setup.hasDesign` false while `setup.hasCode` true → `document` (capture the visual system). - `critique.latest` is `null` → the project has never been critiqued; for a set-up project with a real surface, offering `/impeccable critique ` is a strong default. -- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog), or re-run `critique` if the snapshot looks stale. +- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog and closes it when stale or cleared). - `git.changedFiles` pointing at one surface → scope `audit` or `polish` to those files specifically, naming them. - `devServer.running` true → `live` is available for in-browser iteration; if false, don't lead with `live`. **`live` and the bundled `detect.mjs` are web-only.** If `setup.platform` is `ios`, `android`, or `adaptive`, don't lead with either; the browser overlay and the HTML rule engine don't apply to native app code. - Otherwise group by intent (build new / improve what's there / iterate visually), tailored to the current surface and `setup.platform`. diff --git a/.vibe/skills/impeccable/scripts/critique-storage.mjs b/.vibe/skills/impeccable/scripts/critique-storage.mjs index f23fded37..86d297390 100644 --- a/.vibe/skills/impeccable/scripts/critique-storage.mjs +++ b/.vibe/skills/impeccable/scripts/critique-storage.mjs @@ -16,8 +16,9 @@ * CLI entry points (called from skill instructions): * node critique-storage.mjs slug * node critique-storage.mjs write - * node critique-storage.mjs latest + * node critique-storage.mjs latest [--json] * node critique-storage.mjs trend [limit] + * node critique-storage.mjs close * * 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 @@ -27,6 +28,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { createHash } from 'node:crypto'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { getCritiqueDir } from './lib/impeccable-paths.mjs'; import { slugFromTarget } from './lib/target-slug.mjs'; @@ -50,6 +52,45 @@ export function nowFilenameStamp(date = new Date()) { return iso.replace(/[:.]/g, '-').replace(/-\d+Z$/, 'Z'); } +/** + * Return an exact content fingerprint for a local file target. URLs and + * non-files return null because their content is not available here. + * + * The fingerprint deliberately describes bytes, not Git state or mtimes: + * critique often assesses an uncommitted file, and a later polish run should + * inherit that backlog when the bytes are unchanged regardless of staging. + */ +function resolveLocalTargetPath(target, { cwd = process.cwd() } = {}) { + if (!target || /^https?:\/\//i.test(target)) return null; + return path.isAbsolute(target) ? path.resolve(target) : path.resolve(cwd, target); +} + +function resolveTargetIdentity(target, { cwd = process.cwd() } = {}) { + if (!target || typeof target !== 'string') return null; + if (/^https?:\/\//i.test(target)) { + try { + const url = new URL(target); + const pathname = url.pathname.replace(/\/+$/, '') || '/'; + return `url:${url.origin}${pathname}`; + } catch { + return null; + } + } + const filePath = resolveLocalTargetPath(target, { cwd }); + return filePath ? `file:${filePath}` : null; +} + +export function fingerprintTarget(target, { cwd = process.cwd() } = {}) { + const filePath = resolveLocalTargetPath(target, { cwd }); + if (!filePath) return null; + try { + if (!fs.statSync(filePath).isFile()) return null; + return `sha256:${createHash('sha256').update(fs.readFileSync(filePath)).digest('hex')}`; + } catch { + return null; + } +} + /** * Write a snapshot for `slug`. `meta` carries the small structured frontmatter * keys read back by readTrend(). `body` is the human-readable critique @@ -62,14 +103,27 @@ export function writeSnapshot({ slug, meta, body, cwd = process.cwd(), now = new 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; + const contents = `${front}\n${body.trim()}\n`; + + // A second critique can finish in the same UTC second. Use exclusive + // creation and a fixed-width suffix so concurrent writers cannot replace + // history and lexical ordering still keeps collision entries newest. + for (let collision = 0; collision <= 9999; collision += 1) { + const suffix = collision === 0 ? '' : `~${String(collision).padStart(4, '0')}`; + const filePath = path.join(dir, `${timestamp}${suffix}__${slug}.md`); + try { + fs.writeFileSync(filePath, contents, { encoding: 'utf-8', flag: 'wx' }); + return filePath; + } catch (error) { + if (error?.code !== 'EEXIST') throw error; + } + } + throw new Error(`Too many critique snapshots for ${slug} at ${timestamp}`); } function serializeFrontmatter(obj) { @@ -98,6 +152,8 @@ function parseFrontmatter(text) { try { value = JSON.parse(value); } catch { /* leave as-is */ } } else if (/^-?\d+$/.test(value)) { value = Number(value); + } else if (value === 'true' || value === 'false') { + value = value === 'true'; } out[key] = value; } @@ -107,7 +163,7 @@ function parseFrontmatter(text) { /** * Return snapshot files matching `suffix`, sorted oldest → newest. */ -const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z__.+\.md$/; +const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z(?:~\d{4})?__.+\.md$/; function listSnapshots(suffix, cwd) { const dir = getCritiqueDir(cwd); @@ -118,24 +174,105 @@ function listSnapshots(suffix, cwd) { .map((f) => path.join(dir, f)); } -function readLatestSnapshotMatching(suffix, cwd) { - const filePath = listSnapshots(suffix, cwd).at(-1); +function readSnapshot(filePath) { if (!filePath) return null; const body = fs.readFileSync(filePath, 'utf-8'); return { path: filePath, body, meta: parseFrontmatter(body) }; } +function snapshotTargetIdentity(snapshot) { + const targetPath = snapshot?.meta.target_path; + return snapshot?.meta.target_identity + || (targetPath ? `file:${targetPath}` : null); +} + +function readNewestSnapshot(slug, { cwd = process.cwd() } = {}) { + return readSnapshot(listSnapshots(`__${slug}.md`, cwd).at(-1)); +} + +function readNewestSnapshotForIdentity( + slug, + targetIdentity, + { cwd = process.cwd() } = {}, +) { + const matches = listSnapshots(`__${slug}.md`, cwd) + .map(readSnapshot) + .filter((snapshot) => snapshotTargetIdentity(snapshot) === targetIdentity); + return matches.at(-1) || null; +} + /** * 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() } = {}) { - return readLatestSnapshotMatching(`__${slug}.md`, cwd); + const latest = readNewestSnapshot(slug, { cwd }); + return latest?.meta.closed === true ? null : latest; +} + +/** + * Mark one exact snapshot closed without deleting the score history consumed + * by `trend`. Exact identity matters: a newer critique may land after polish + * reads its backlog, and that newer snapshot must remain live. `snapshotFile` + * may be the absolute path returned by readLatestSnapshot() or the basename + * emitted by `latest --json`. Returns the path marked closed, or null. + */ +export function closeSnapshot(snapshotFile, { cwd = process.cwd() } = {}) { + if (!snapshotFile || typeof snapshotFile !== 'string') return null; + const dir = path.resolve(getCritiqueDir(cwd)); + const snapshotPath = path.isAbsolute(snapshotFile) + ? path.resolve(snapshotFile) + : path.resolve(dir, snapshotFile); + const filename = path.basename(snapshotPath); + if ( + path.dirname(snapshotPath) !== dir + || !SNAPSHOT_FILENAME.test(filename) + ) return null; + + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) return null; + snapshot = readSnapshot(snapshotPath); + } catch { + return null; + } + if (!snapshot || snapshot.meta.closed === true) return null; + const closedBody = snapshot.body.replace( + /^(---\r?\n[\s\S]*?)(\r?\n---)/, + '$1\nclosed: true$2', + ); + if (closedBody === snapshot.body) { + throw new Error(`Cannot close snapshot without frontmatter: ${snapshot.path}`); + } + fs.writeFileSync(snapshot.path, closedBody, 'utf-8'); + return snapshot.path; } /** Return the most recent snapshot across all targets, or null. */ export function readLatestSnapshotAcrossTargets({ cwd = process.cwd() } = {}) { - return readLatestSnapshotMatching('.md', cwd); + const snapshots = listSnapshots('.md', cwd).map(readSnapshot); + const identifiedSlugs = new Set( + snapshots + .filter((snapshot) => snapshotTargetIdentity(snapshot)) + .map((snapshot) => snapshot.meta.slug), + ); + const latestByTarget = new Map(); + for (const snapshot of snapshots) { + if (!snapshot?.meta.slug) continue; + // Slugs are lossy: distinct targets such as foo/bar and foo-bar can share + // one. Keep each known identity's latest open/closed state independent so + // closing one target cannot hide another target's live backlog. Once a + // slug has any identity-aware snapshot, its older legacy records are no + // longer independently routable and must not resurface as zombie work. + const targetIdentity = snapshotTargetIdentity(snapshot); + if (!targetIdentity && identifiedSlugs.has(snapshot.meta.slug)) continue; + const streamKey = targetIdentity || `slug:${snapshot.meta.slug}`; + latestByTarget.set(streamKey, snapshot); + } + return [...latestByTarget.values()] + .filter((snapshot) => snapshot.meta.closed !== true) + .sort((a, b) => a.path.localeCompare(b.path)) + .at(-1) || null; } /** @@ -153,9 +290,13 @@ export function readTrend(slug, { limit = 5, cwd = process.cwd() } = {}) { // Accept either a ready slug or a concrete target (path/URL) everywhere, so // callers never have to run the slug step separately. Anything containing a // path or URL marker is resolved through slugFromTarget. +function isReadySlug(value) { + return /^[a-z0-9-]+$/.test(value || '') && !value.includes('/'); +} + function coerceSlug(value) { if (!value) return null; - if (/^[a-z0-9-]+$/.test(value) && !value.includes('/')) return value; + if (isReadySlug(value)) return value; return slugFromTarget(value); } @@ -181,14 +322,124 @@ function main(argv) { if (metaArg) { try { meta = JSON.parse(metaArg); } catch { /* ignore */ } } + // The helper, not caller-provided metadata, owns the target fingerprint. + // This makes the snapshot describe the exact file bytes critique saw. + delete meta.target_fingerprint; + delete meta.target_path; + delete meta.target_identity; + const targetIdentity = resolveTargetIdentity(slugArg); + if (targetIdentity) meta.target_identity = targetIdentity; + const targetFingerprint = fingerprintTarget(slugArg); + if (targetFingerprint) { + meta.target_fingerprint = targetFingerprint; + meta.target_path = resolveLocalTargetPath(slugArg); + } const out = writeSnapshot({ slug, meta, body: raw }); process.stdout.write(`${out}\n`); return; } case 'latest': { - const latest = readLatestSnapshot(coerceSlug(args[0])); - if (!latest) { process.exit(2); } - process.stdout.write(latest.body); + const target = args[0]; + const format = args[1]; + const slug = coerceSlug(target); + if (!slug || (format && format !== '--json')) { + process.stderr.write('usage: latest [--json]\n'); + process.exit(1); + } + const targetFingerprint = fingerprintTarget(target); + const targetPath = resolveLocalTargetPath(target); + const targetIdentity = resolveTargetIdentity(target); + const readySlug = isReadySlug(target); + const newestForSlug = readNewestSnapshot(slug); + if (!newestForSlug) { process.exit(2); } + + // Concrete targets select the newest snapshot for their exact identity, + // not merely the newest filename for a lossy slug. This keeps distinct + // targets such as foo/bar and foo-bar from hiding each other's backlog. + const exactSnapshot = readNewestSnapshotForIdentity(slug, targetIdentity); + let latest = exactSnapshot; + if (!latest && !readySlug) { + // Legacy snapshots have no identity. Preserve their old explicit + // path/URL behavior only when no known target identity was selected. + latest = readNewestSnapshotForIdentity(slug, null); + } + if (!latest) latest = newestForSlug; + if (latest.meta.closed === true) { process.exit(2); } + + const recordedTargetPath = latest.meta.target_path; + const recordedTargetIdentity = snapshotTargetIdentity(latest); + const matchingIdentity = recordedTargetIdentity === targetIdentity; + + // Bare slugs remain a supported lookup mode, including for URL + // snapshots. But when a same-named local file exists, the request is + // ambiguous unless that exact file owns the snapshot identity. + if (readySlug && !recordedTargetIdentity) { + process.stderr.write( + 'ambiguous legacy snapshot target; use an explicit ./path or full URL\n', + ); + process.exit(2); + } + if (readySlug && targetPath && fs.existsSync(targetPath) && !matchingIdentity) { + process.stderr.write( + 'ambiguous snapshot slug; use an explicit ./path or remove the local name collision\n', + ); + process.exit(2); + } + + const concreteTarget = !readySlug || matchingIdentity; + if (concreteTarget && recordedTargetIdentity && !matchingIdentity) { + process.exit(2); + } + const concreteLocalTarget = concreteTarget && targetPath; + if (concreteLocalTarget && latest.meta.target_fingerprint !== targetFingerprint) { + closeSnapshot(latest.path); + process.exit(2); + } + if (format === '--json') { + process.stdout.write(JSON.stringify({ + snapshot_file: path.basename(latest.path), + body: latest.body, + }, null, 2) + '\n'); + } else { + process.stdout.write(latest.body); + } + return; + } + case 'close': { + const [slugArg, snapshotFile, ...extra] = args; + const slug = coerceSlug(slugArg); + if (!slug || !snapshotFile || extra.length > 0) { + process.stderr.write('usage: close \n'); + process.exit(1); + } + if ( + path.basename(snapshotFile) !== snapshotFile + || !SNAPSHOT_FILENAME.test(snapshotFile) + || !snapshotFile.endsWith(`__${slug}.md`) + ) process.exit(2); + + // A slug and filename are not enough to prove ownership because two + // distinct targets can normalize to the same slug. Modern snapshots + // carry a canonical identity, so require the supplied resolved target + // to match it before allowing the exact snapshot to be closed. Legacy + // snapshots without identity retain their historical close behavior. + const snapshotPath = path.join(getCritiqueDir(process.cwd()), snapshotFile); + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) process.exit(2); + snapshot = readSnapshot(snapshotPath); + } catch { + process.exit(2); + } + const recordedTargetIdentity = snapshotTargetIdentity(snapshot); + if ( + recordedTargetIdentity + && recordedTargetIdentity !== resolveTargetIdentity(slugArg) + ) process.exit(2); + + const closed = closeSnapshot(snapshotFile); + if (!closed) { process.exit(2); } + process.stdout.write(`${closed}\n`); return; } case 'trend': { @@ -197,7 +448,7 @@ function main(argv) { return; } default: - process.stderr.write('usage: critique-storage.mjs [args]\n'); + process.stderr.write('usage: critique-storage.mjs [args]\n'); process.exit(1); } } diff --git a/plugin/skills/impeccable/reference/critique.md b/plugin/skills/impeccable/reference/critique.md index d298da3eb..31e48effc 100644 --- a/plugin/skills/impeccable/reference/critique.md +++ b/plugin/skills/impeccable/reference/critique.md @@ -1,6 +1,6 @@ ### Purpose -Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive/backlog for future commands. +Resolve one stable target, run two independent assessments, synthesize a design critique, persist a snapshot, and ask the user what to improve next. The chat response is the primary deliverable; the snapshot is an archive of that run. ### Hard Invariants @@ -84,7 +84,7 @@ After Assessment B returns usable CLI findings, reuse them. Do not rerun `detect Synthesize both assessments into a single report. Do NOT simply concatenate. Weave the findings together, noting where the LLM review and detector agree, where the detector caught issues the LLM missed, and where detector findings are false positives. -The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is only an archive/backlog for later commands. +The chat response is the primary user-facing deliverable. Present the full structured critique below in chat; do not replace it with a summary and a link. The persisted snapshot is an archive of that run. Structure your feedback as a design director would: @@ -197,7 +197,7 @@ Skip this step if the Setup slug was null (vague or root-level target). IMPECCABLE_CRITIQUE_META='{"target":"","total_score":,"max_score":,"na_heuristics":"","p0_count":,"p1_count":}' \ node "/scripts/critique-storage.mjs" write "" ``` - `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. + `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. For a local file target, the helper also records an exact content fingerprint so polish can distinguish the assessed bytes from later edits without relying on Git state or timestamps. The helper prints the absolute path it wrote. Leave that file on disk. Polish closes it; this run does not. 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. diff --git a/plugin/skills/impeccable/reference/polish.md b/plugin/skills/impeccable/reference/polish.md index 3a6eeb63f..25041740f 100644 --- a/plugin/skills/impeccable/reference/polish.md +++ b/plugin/skills/impeccable/reference/polish.md @@ -29,10 +29,10 @@ Use the feature yourself at the surface's representative sizes: desktop and mobi If a prior critique exists, use it as one input: ```bash -node "/scripts/critique-storage.mjs" latest "" +node "/scripts/critique-storage.mjs" latest "" --json ``` -Exit 0 returns the latest snapshot; incorporate relevant P0/P1 findings and name the snapshot read. Exit 2 means none exists. Perform an independent pass either way. +Exit 0 returns JSON with the latest snapshot's `body` and an exact `snapshot_file` identity. Retain `snapshot_file` until the end of the pass. For a local file target, the helper compares the file's exact current content fingerprint with the fingerprint captured by critique. Unchanged staged, unstaged, or untracked content remains current; any byte change, deletion, or replacement with a non-file closes the backlog it identified while preserving its trend history and exits 2. A URL target has no local fingerprint and remains current until explicitly closed. When current, incorporate relevant P0/P1 findings from `body` and name the snapshot read. Exit 2 means none exists or the target changed. Perform an independent pass either way. ## 3. Triage @@ -95,3 +95,11 @@ Walk the complete path again with mouse, keyboard, and touch where applicable. C Follow the quality guidance supplied by `context.mjs` and hooks, then run any other relevant QA commands. Context requests a manual scan only when no automatic detector is active; never add another detector pass. Fix real defects and document only narrow intentional exceptions. A clean scan does not replace visual judgment. Finish with a source diff: remove accidental churn, orphaned code, redundant values, and temporary artifacts. Ship only when the feature is functionally complete and consistently finished across the path. + +When this pass clears every Priority Issue it took from a snapshot, close that snapshot: + +```bash +node "/scripts/critique-storage.mjs" close "" "" +``` + +This closes only the snapshot this pass actually processed; if a newer critique landed meanwhile, its backlog stays live. Do not close when no snapshot was read, when `snapshot_file` was not retained, or when Priority Issues remain. diff --git a/plugin/skills/impeccable/reference/routing.md b/plugin/skills/impeccable/reference/routing.md index df9a63096..e5b59d1c8 100644 --- a/plugin/skills/impeccable/reference/routing.md +++ b/plugin/skills/impeccable/reference/routing.md @@ -8,7 +8,7 @@ Reason over the signals; there is no score to obey: - `setup.hasDesign` false while `setup.hasCode` true → `document` (capture the visual system). - `critique.latest` is `null` → the project has never been critiqued; for a set-up project with a real surface, offering `/impeccable critique ` is a strong default. -- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog), or re-run `critique` if the snapshot looks stale. +- `critique.latest` with a low `score` or non-zero `p0` / `p1` → `polish` (it reads that snapshot as its backlog and closes it when stale or cleared). - `git.changedFiles` pointing at one surface → scope `audit` or `polish` to those files specifically, naming them. - `devServer.running` true → `live` is available for in-browser iteration; if false, don't lead with `live`. **`live` and the bundled `detect.mjs` are web-only.** If `setup.platform` is `ios`, `android`, or `adaptive`, don't lead with either; the browser overlay and the HTML rule engine don't apply to native app code. - Otherwise group by intent (build new / improve what's there / iterate visually), tailored to the current surface and `setup.platform`. diff --git a/plugin/skills/impeccable/scripts/critique-storage.mjs b/plugin/skills/impeccable/scripts/critique-storage.mjs index f23fded37..86d297390 100644 --- a/plugin/skills/impeccable/scripts/critique-storage.mjs +++ b/plugin/skills/impeccable/scripts/critique-storage.mjs @@ -16,8 +16,9 @@ * CLI entry points (called from skill instructions): * node critique-storage.mjs slug * node critique-storage.mjs write - * node critique-storage.mjs latest + * node critique-storage.mjs latest [--json] * node critique-storage.mjs trend [limit] + * node critique-storage.mjs close * * 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 @@ -27,6 +28,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { createHash } from 'node:crypto'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { getCritiqueDir } from './lib/impeccable-paths.mjs'; import { slugFromTarget } from './lib/target-slug.mjs'; @@ -50,6 +52,45 @@ export function nowFilenameStamp(date = new Date()) { return iso.replace(/[:.]/g, '-').replace(/-\d+Z$/, 'Z'); } +/** + * Return an exact content fingerprint for a local file target. URLs and + * non-files return null because their content is not available here. + * + * The fingerprint deliberately describes bytes, not Git state or mtimes: + * critique often assesses an uncommitted file, and a later polish run should + * inherit that backlog when the bytes are unchanged regardless of staging. + */ +function resolveLocalTargetPath(target, { cwd = process.cwd() } = {}) { + if (!target || /^https?:\/\//i.test(target)) return null; + return path.isAbsolute(target) ? path.resolve(target) : path.resolve(cwd, target); +} + +function resolveTargetIdentity(target, { cwd = process.cwd() } = {}) { + if (!target || typeof target !== 'string') return null; + if (/^https?:\/\//i.test(target)) { + try { + const url = new URL(target); + const pathname = url.pathname.replace(/\/+$/, '') || '/'; + return `url:${url.origin}${pathname}`; + } catch { + return null; + } + } + const filePath = resolveLocalTargetPath(target, { cwd }); + return filePath ? `file:${filePath}` : null; +} + +export function fingerprintTarget(target, { cwd = process.cwd() } = {}) { + const filePath = resolveLocalTargetPath(target, { cwd }); + if (!filePath) return null; + try { + if (!fs.statSync(filePath).isFile()) return null; + return `sha256:${createHash('sha256').update(fs.readFileSync(filePath)).digest('hex')}`; + } catch { + return null; + } +} + /** * Write a snapshot for `slug`. `meta` carries the small structured frontmatter * keys read back by readTrend(). `body` is the human-readable critique @@ -62,14 +103,27 @@ export function writeSnapshot({ slug, meta, body, cwd = process.cwd(), now = new 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; + const contents = `${front}\n${body.trim()}\n`; + + // A second critique can finish in the same UTC second. Use exclusive + // creation and a fixed-width suffix so concurrent writers cannot replace + // history and lexical ordering still keeps collision entries newest. + for (let collision = 0; collision <= 9999; collision += 1) { + const suffix = collision === 0 ? '' : `~${String(collision).padStart(4, '0')}`; + const filePath = path.join(dir, `${timestamp}${suffix}__${slug}.md`); + try { + fs.writeFileSync(filePath, contents, { encoding: 'utf-8', flag: 'wx' }); + return filePath; + } catch (error) { + if (error?.code !== 'EEXIST') throw error; + } + } + throw new Error(`Too many critique snapshots for ${slug} at ${timestamp}`); } function serializeFrontmatter(obj) { @@ -98,6 +152,8 @@ function parseFrontmatter(text) { try { value = JSON.parse(value); } catch { /* leave as-is */ } } else if (/^-?\d+$/.test(value)) { value = Number(value); + } else if (value === 'true' || value === 'false') { + value = value === 'true'; } out[key] = value; } @@ -107,7 +163,7 @@ function parseFrontmatter(text) { /** * Return snapshot files matching `suffix`, sorted oldest → newest. */ -const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z__.+\.md$/; +const SNAPSHOT_FILENAME = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z(?:~\d{4})?__.+\.md$/; function listSnapshots(suffix, cwd) { const dir = getCritiqueDir(cwd); @@ -118,24 +174,105 @@ function listSnapshots(suffix, cwd) { .map((f) => path.join(dir, f)); } -function readLatestSnapshotMatching(suffix, cwd) { - const filePath = listSnapshots(suffix, cwd).at(-1); +function readSnapshot(filePath) { if (!filePath) return null; const body = fs.readFileSync(filePath, 'utf-8'); return { path: filePath, body, meta: parseFrontmatter(body) }; } +function snapshotTargetIdentity(snapshot) { + const targetPath = snapshot?.meta.target_path; + return snapshot?.meta.target_identity + || (targetPath ? `file:${targetPath}` : null); +} + +function readNewestSnapshot(slug, { cwd = process.cwd() } = {}) { + return readSnapshot(listSnapshots(`__${slug}.md`, cwd).at(-1)); +} + +function readNewestSnapshotForIdentity( + slug, + targetIdentity, + { cwd = process.cwd() } = {}, +) { + const matches = listSnapshots(`__${slug}.md`, cwd) + .map(readSnapshot) + .filter((snapshot) => snapshotTargetIdentity(snapshot) === targetIdentity); + return matches.at(-1) || null; +} + /** * 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() } = {}) { - return readLatestSnapshotMatching(`__${slug}.md`, cwd); + const latest = readNewestSnapshot(slug, { cwd }); + return latest?.meta.closed === true ? null : latest; +} + +/** + * Mark one exact snapshot closed without deleting the score history consumed + * by `trend`. Exact identity matters: a newer critique may land after polish + * reads its backlog, and that newer snapshot must remain live. `snapshotFile` + * may be the absolute path returned by readLatestSnapshot() or the basename + * emitted by `latest --json`. Returns the path marked closed, or null. + */ +export function closeSnapshot(snapshotFile, { cwd = process.cwd() } = {}) { + if (!snapshotFile || typeof snapshotFile !== 'string') return null; + const dir = path.resolve(getCritiqueDir(cwd)); + const snapshotPath = path.isAbsolute(snapshotFile) + ? path.resolve(snapshotFile) + : path.resolve(dir, snapshotFile); + const filename = path.basename(snapshotPath); + if ( + path.dirname(snapshotPath) !== dir + || !SNAPSHOT_FILENAME.test(filename) + ) return null; + + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) return null; + snapshot = readSnapshot(snapshotPath); + } catch { + return null; + } + if (!snapshot || snapshot.meta.closed === true) return null; + const closedBody = snapshot.body.replace( + /^(---\r?\n[\s\S]*?)(\r?\n---)/, + '$1\nclosed: true$2', + ); + if (closedBody === snapshot.body) { + throw new Error(`Cannot close snapshot without frontmatter: ${snapshot.path}`); + } + fs.writeFileSync(snapshot.path, closedBody, 'utf-8'); + return snapshot.path; } /** Return the most recent snapshot across all targets, or null. */ export function readLatestSnapshotAcrossTargets({ cwd = process.cwd() } = {}) { - return readLatestSnapshotMatching('.md', cwd); + const snapshots = listSnapshots('.md', cwd).map(readSnapshot); + const identifiedSlugs = new Set( + snapshots + .filter((snapshot) => snapshotTargetIdentity(snapshot)) + .map((snapshot) => snapshot.meta.slug), + ); + const latestByTarget = new Map(); + for (const snapshot of snapshots) { + if (!snapshot?.meta.slug) continue; + // Slugs are lossy: distinct targets such as foo/bar and foo-bar can share + // one. Keep each known identity's latest open/closed state independent so + // closing one target cannot hide another target's live backlog. Once a + // slug has any identity-aware snapshot, its older legacy records are no + // longer independently routable and must not resurface as zombie work. + const targetIdentity = snapshotTargetIdentity(snapshot); + if (!targetIdentity && identifiedSlugs.has(snapshot.meta.slug)) continue; + const streamKey = targetIdentity || `slug:${snapshot.meta.slug}`; + latestByTarget.set(streamKey, snapshot); + } + return [...latestByTarget.values()] + .filter((snapshot) => snapshot.meta.closed !== true) + .sort((a, b) => a.path.localeCompare(b.path)) + .at(-1) || null; } /** @@ -153,9 +290,13 @@ export function readTrend(slug, { limit = 5, cwd = process.cwd() } = {}) { // Accept either a ready slug or a concrete target (path/URL) everywhere, so // callers never have to run the slug step separately. Anything containing a // path or URL marker is resolved through slugFromTarget. +function isReadySlug(value) { + return /^[a-z0-9-]+$/.test(value || '') && !value.includes('/'); +} + function coerceSlug(value) { if (!value) return null; - if (/^[a-z0-9-]+$/.test(value) && !value.includes('/')) return value; + if (isReadySlug(value)) return value; return slugFromTarget(value); } @@ -181,14 +322,124 @@ function main(argv) { if (metaArg) { try { meta = JSON.parse(metaArg); } catch { /* ignore */ } } + // The helper, not caller-provided metadata, owns the target fingerprint. + // This makes the snapshot describe the exact file bytes critique saw. + delete meta.target_fingerprint; + delete meta.target_path; + delete meta.target_identity; + const targetIdentity = resolveTargetIdentity(slugArg); + if (targetIdentity) meta.target_identity = targetIdentity; + const targetFingerprint = fingerprintTarget(slugArg); + if (targetFingerprint) { + meta.target_fingerprint = targetFingerprint; + meta.target_path = resolveLocalTargetPath(slugArg); + } const out = writeSnapshot({ slug, meta, body: raw }); process.stdout.write(`${out}\n`); return; } case 'latest': { - const latest = readLatestSnapshot(coerceSlug(args[0])); - if (!latest) { process.exit(2); } - process.stdout.write(latest.body); + const target = args[0]; + const format = args[1]; + const slug = coerceSlug(target); + if (!slug || (format && format !== '--json')) { + process.stderr.write('usage: latest [--json]\n'); + process.exit(1); + } + const targetFingerprint = fingerprintTarget(target); + const targetPath = resolveLocalTargetPath(target); + const targetIdentity = resolveTargetIdentity(target); + const readySlug = isReadySlug(target); + const newestForSlug = readNewestSnapshot(slug); + if (!newestForSlug) { process.exit(2); } + + // Concrete targets select the newest snapshot for their exact identity, + // not merely the newest filename for a lossy slug. This keeps distinct + // targets such as foo/bar and foo-bar from hiding each other's backlog. + const exactSnapshot = readNewestSnapshotForIdentity(slug, targetIdentity); + let latest = exactSnapshot; + if (!latest && !readySlug) { + // Legacy snapshots have no identity. Preserve their old explicit + // path/URL behavior only when no known target identity was selected. + latest = readNewestSnapshotForIdentity(slug, null); + } + if (!latest) latest = newestForSlug; + if (latest.meta.closed === true) { process.exit(2); } + + const recordedTargetPath = latest.meta.target_path; + const recordedTargetIdentity = snapshotTargetIdentity(latest); + const matchingIdentity = recordedTargetIdentity === targetIdentity; + + // Bare slugs remain a supported lookup mode, including for URL + // snapshots. But when a same-named local file exists, the request is + // ambiguous unless that exact file owns the snapshot identity. + if (readySlug && !recordedTargetIdentity) { + process.stderr.write( + 'ambiguous legacy snapshot target; use an explicit ./path or full URL\n', + ); + process.exit(2); + } + if (readySlug && targetPath && fs.existsSync(targetPath) && !matchingIdentity) { + process.stderr.write( + 'ambiguous snapshot slug; use an explicit ./path or remove the local name collision\n', + ); + process.exit(2); + } + + const concreteTarget = !readySlug || matchingIdentity; + if (concreteTarget && recordedTargetIdentity && !matchingIdentity) { + process.exit(2); + } + const concreteLocalTarget = concreteTarget && targetPath; + if (concreteLocalTarget && latest.meta.target_fingerprint !== targetFingerprint) { + closeSnapshot(latest.path); + process.exit(2); + } + if (format === '--json') { + process.stdout.write(JSON.stringify({ + snapshot_file: path.basename(latest.path), + body: latest.body, + }, null, 2) + '\n'); + } else { + process.stdout.write(latest.body); + } + return; + } + case 'close': { + const [slugArg, snapshotFile, ...extra] = args; + const slug = coerceSlug(slugArg); + if (!slug || !snapshotFile || extra.length > 0) { + process.stderr.write('usage: close \n'); + process.exit(1); + } + if ( + path.basename(snapshotFile) !== snapshotFile + || !SNAPSHOT_FILENAME.test(snapshotFile) + || !snapshotFile.endsWith(`__${slug}.md`) + ) process.exit(2); + + // A slug and filename are not enough to prove ownership because two + // distinct targets can normalize to the same slug. Modern snapshots + // carry a canonical identity, so require the supplied resolved target + // to match it before allowing the exact snapshot to be closed. Legacy + // snapshots without identity retain their historical close behavior. + const snapshotPath = path.join(getCritiqueDir(process.cwd()), snapshotFile); + let snapshot; + try { + if (!fs.lstatSync(snapshotPath).isFile()) process.exit(2); + snapshot = readSnapshot(snapshotPath); + } catch { + process.exit(2); + } + const recordedTargetIdentity = snapshotTargetIdentity(snapshot); + if ( + recordedTargetIdentity + && recordedTargetIdentity !== resolveTargetIdentity(slugArg) + ) process.exit(2); + + const closed = closeSnapshot(snapshotFile); + if (!closed) { process.exit(2); } + process.stdout.write(`${closed}\n`); return; } case 'trend': { @@ -197,7 +448,7 @@ function main(argv) { return; } default: - process.stderr.write('usage: critique-storage.mjs [args]\n'); + process.stderr.write('usage: critique-storage.mjs [args]\n'); process.exit(1); } }