From 2ad0e34d1c5a2be3c7f3431679f5d0680dd64da8 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 3 Oct 2026 20:00:21 +0000 Subject: [PATCH 1/2] fix(design): never overwrite a paid design image (#1529) Every image the API returns now goes through one persistence path (design/src/persist.ts): mkdir, exclusive `wx` claim that bumps x.png -> x-2.png ... x-999.png, write, fsync, close. A failure after the bytes arrived removes the claimed file and keeps a 0600 recovery copy in the temp dir; empty image data is a failure, never a 0-byte PNG. The API call is the only thing inside each retry loop, so a local save failure never buys a second image (variants, iterate fallback, generate --retry). generate, iterate, evolve and variants (both branches) print the same run fields (requested, saved, selected, failures, recovered) on every exit, with exit 0 / 2 (nothing saved) / 3 (stopped after saving). Each generate --retry attempt is its own saved image listed in `attempts`. compare accepts --images-file and JSON-array --images. The gallery lists bumped names, orders history by mtime then board-images.json, and marks approvals by approved_path. New bin/gstack-design-claim publishes staged images without clobbering; bin/gstack-design-approved resolves an approval record (legacy letter-only records still resolve; a missing approved_path is an error, never a substitute). Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> --- bin/gstack-design-approved | 24 ++ bin/gstack-design-claim | 55 +++++ design/src/approval.ts | 56 +++++ design/src/auth.ts | 19 +- design/src/cli.ts | 65 ++--- design/src/commands.ts | 41 +++- design/src/evolve.ts | 99 ++++---- design/src/gallery.ts | 43 +++- design/src/generate.ts | 134 ++++++----- design/src/image-args.ts | 53 +++++ design/src/iterate.ts | 103 ++++---- design/src/persist.ts | 196 +++++++++++++++ design/src/variants.ts | 357 ++++++++++++++++------------ design/test/approval.test.ts | 61 +++++ design/test/cli-persistence.test.ts | 223 +++++++++++++++++ design/test/gallery.test.ts | 36 ++- design/test/image-args.test.ts | 52 ++++ design/test/models.test.ts | 16 +- design/test/persist.test.ts | 165 +++++++++++++ test/gstack-design-claim.test.ts | 118 +++++++++ 20 files changed, 1537 insertions(+), 379 deletions(-) create mode 100755 bin/gstack-design-approved create mode 100755 bin/gstack-design-claim create mode 100644 design/src/approval.ts create mode 100644 design/src/image-args.ts create mode 100644 design/src/persist.ts create mode 100644 design/test/approval.test.ts create mode 100644 design/test/cli-persistence.test.ts create mode 100644 design/test/image-args.test.ts create mode 100644 design/test/persist.test.ts create mode 100644 test/gstack-design-claim.test.ts diff --git a/bin/gstack-design-approved b/bin/gstack-design-approved new file mode 100755 index 000000000..7c3f360a2 --- /dev/null +++ b/bin/gstack-design-approved @@ -0,0 +1,24 @@ +#!/usr/bin/env bun +// gstack-design-approved — print the absolute path of the image an +// approved.json names. +// +// Usage: gstack-design-approved +// +// Prefers approved_path (relative to approved.json's directory). Legacy +// records with only approved_variant resolve to variant-.png. When the +// named image is missing, exits 1 with "approved image is missing; +// reselect from the board" and never substitutes another image. + +import { resolveApprovedImage } from '../design/src/approval'; + +const file = process.argv[2]; +if (!file) { + console.error('Usage: gstack-design-approved '); + process.exit(1); +} +const resolved = resolveApprovedImage(file); +if (!resolved.ok) { + console.error(resolved.error); + process.exit(1); +} +console.log(resolved.path); diff --git a/bin/gstack-design-claim b/bin/gstack-design-claim new file mode 100755 index 000000000..88a8c9f1f --- /dev/null +++ b/bin/gstack-design-claim @@ -0,0 +1,55 @@ +#!/bin/sh +# gstack-design-claim — publish a staged design image without overwriting. +# +# Usage: gstack-design-claim +# +# Creates with an exclusive, non-overwriting create (set -C +# noclobber, which opens with O_EXCL). When the name is taken it tries +# -2, -3 ... -999, the same rule as +# $D's claimOutputPath (x-2.png bumps to x-2-2.png). Prints the final path. +# On any failure it exits non-zero with problem/cause/fix and leaves the staged +# image in place, so the paid bytes survive. On success the staged copy is +# removed. + +set -u + +fail() { + printf 'gstack-design-claim: %s\ncause: %s\nfix: %s\n' "$1" "$2" "$3" >&2 + exit 1 +} + +[ "$#" -eq 2 ] || fail "expected 2 arguments, got $#" "usage: gstack-design-claim " "pass the staged image path, then the destination path" +src=$1 +dest=$2 + +[ -f "$src" ] && [ -s "$src" ] || fail "staged image $src is missing or empty" "the generating command did not save an image there" "use the outputPath the \$D command printed" + +dir=$(dirname "$dest") +base=$(basename "$dest") +case "$base" in + ?*.*) stem=${base%.*}; ext=".${base##*.}" ;; + *) stem=$base; ext="" ;; +esac + +err=$(mkdir -p "$dir" 2>&1) || fail "cannot create $dir; staged image kept at $src" "$err" "make $dir writable or choose another destination" + +n=1 +candidate=$dest +while :; do + if err=$( (set -C; : > "$candidate") 2>&1 ); then + if err=$(cat "$src" 2>&1 > "$candidate"); then + rm -f "$src" + printf '%s\n' "$candidate" + exit 0 + fi + rm -f "$candidate" + fail "cannot write $candidate; staged image kept at $src" "$err" "free disk space or make $dir writable, then rerun" + fi + if [ -e "$candidate" ] || [ -L "$candidate" ]; then + n=$((n + 1)) + [ "$n" -le 999 ] || fail "$dest and every name through $stem-999$ext already exist; staged image kept at $src" "999 images already use this name" "choose a new destination name or move older images out of $dir" + candidate="$dir/$stem-$n$ext" + continue + fi + fail "cannot create $candidate; staged image kept at $src" "$err" "make $dir writable or choose another destination" +done diff --git a/design/src/approval.ts b/design/src/approval.ts new file mode 100644 index 000000000..400015439 --- /dev/null +++ b/design/src/approval.ts @@ -0,0 +1,56 @@ +/** + * Resolve the image an approval record names. `approved_path` is stored + * relative to the directory holding approved.json, so a copied or relocated + * designs directory still resolves. Records without `approved_path` (written + * before never-overwrite) fall back to `variant-.png`. A + * present `approved_path` whose file is missing is an error: another image is + * never substituted. + */ + +import fs from "fs"; +import path from "path"; + +export type ApprovedImage = { ok: true; path: string; legacy: boolean } | { ok: false; error: string }; + +export interface ApprovalRecord { + approved_variant?: unknown; + approved_path?: unknown; +} + +function missing(imagePath: string): ApprovedImage { + return { ok: false, error: `approved image ${imagePath} is missing; reselect from the board` }; +} + +function readable(imagePath: string): boolean { + try { + fs.accessSync(imagePath, fs.constants.R_OK); + return fs.statSync(imagePath).isFile(); + } catch { + return false; + } +} + +/** The image path a record names, without checking that it exists. */ +export function approvedImagePath(record: ApprovalRecord, recordDir: string): { path: string; legacy: boolean } | null { + if (typeof record.approved_path === "string" && record.approved_path !== "") { + return { path: path.resolve(recordDir, record.approved_path), legacy: false }; + } + if (typeof record.approved_variant === "string" && record.approved_variant !== "") { + return { path: path.join(recordDir, `variant-${record.approved_variant}.png`), legacy: true }; + } + return null; +} + +export function resolveApprovedImage(approvedJsonPath: string): ApprovedImage { + let record: ApprovalRecord; + try { + record = JSON.parse(fs.readFileSync(approvedJsonPath, "utf-8")); + } catch (err: any) { + return { ok: false, error: `cannot read ${approvedJsonPath}: ${err.code || err.message}` }; + } + const named = approvedImagePath(record ?? {}, path.dirname(path.resolve(approvedJsonPath))); + if (!named) { + return { ok: false, error: `${approvedJsonPath} names no approved image; reselect from the board` }; + } + return readable(named.path) ? { ok: true, ...named } : missing(named.path); +} diff --git a/design/src/auth.ts b/design/src/auth.ts index 1202500a9..fc25cbcf4 100644 --- a/design/src/auth.ts +++ b/design/src/auth.ts @@ -119,19 +119,20 @@ export function saveApiKey(key: string): void { } /** - * Get API key or exit with setup instructions. + * Get API key or throw with setup instructions. */ export function requireApiKey(): string { const resolution = resolveApiKeyInfo(); if (!resolution) { - console.error("No OpenAI API key found."); - console.error(""); - console.error("Run: $D setup"); - console.error(" or save to ~/.gstack/openai.json: { \"api_key\": \"sk-...\" }"); - console.error(" or set OPENAI_API_KEY environment variable"); - console.error(""); - console.error("Get a key at: https://platform.openai.com/api-keys"); - process.exit(1); + throw new Error([ + "No OpenAI API key found.", + "", + "Run: $D setup", + " or save to ~/.gstack/openai.json: { \"api_key\": \"sk-...\" }", + " or set OPENAI_API_KEY environment variable", + "", + "Get a key at: https://platform.openai.com/api-keys", + ].join("\n")); } console.error(`Using OpenAI key from ${describeApiKeySource(resolution)}.`); if (resolution.warning) console.error(resolution.warning); diff --git a/design/src/cli.ts b/design/src/cli.ts index b266c6f38..2278b9a65 100644 --- a/design/src/cli.ts +++ b/design/src/cli.ts @@ -12,7 +12,7 @@ * 4. Print result JSON to stdout */ -import { COMMANDS } from "./commands"; +import { COMMANDS, OUTPUT_CONTRACT } from "./commands"; import { generate } from "./generate"; import { checkCommand } from "./check"; import { compare } from "./compare"; @@ -26,6 +26,7 @@ import { generateDesignToCodePrompt } from "./design-to-code"; import { serve } from "./serve"; import { gallery } from "./gallery"; import { normalizeIntFlag } from "./flag-utils"; +import { resolveImagePaths } from "./image-args"; import { daemonStatus as daemonStatusClient, ensureDaemon, @@ -33,6 +34,9 @@ import { shutdownDaemon, } from "./daemon-client"; import { spawn as nodeSpawn } from "child_process"; +import fs from "fs"; +import os from "os"; +import path from "path"; function parseArgs(argv: string[]): { command: string; @@ -75,6 +79,7 @@ function printUsage(): void { console.log(` ${name.padEnd(12)} ${info.description}`); console.log(` ${"".padEnd(12)} ${info.usage}`); } + console.log(`\n${OUTPUT_CONTRACT}`); console.log("\nAuth: ~/.gstack/openai.json, then OPENAI_API_KEY env var"); console.log("If OPENAI_API_KEY matches a current-directory .env file, the source is reported before billing."); console.log("Setup: $D setup"); @@ -107,19 +112,18 @@ async function runSetup(): Promise { // Smoke test console.log("\nRunning smoke test (generating a simple image)..."); - try { - await generate({ - brief: "A simple blue square centered on a white background. Minimal, geometric, clean.", - output: "/tmp/gstack-design-smoke-test.png", - size: "1024x1024", - quality: "low", - }); - console.log("\nSmoke test PASSED. Design generation is working."); - } catch (err: any) { - console.error(`\nSmoke test FAILED: ${err.message}`); + const smoke = await generate({ + brief: "A simple blue square centered on a white background. Minimal, geometric, clean.", + output: path.join(fs.mkdtempSync(path.join(os.tmpdir(), "gstack-design-smoke-")), "smoke-test.png"), + size: "1024x1024", + quality: "low", + }); + if (smoke.exitCode !== 0) { + console.error(`\nSmoke test FAILED: ${smoke.failures.map(f => f.reason).join("; ")}`); console.error("Check your API key and organization verification status."); process.exit(1); } + console.log("\nSmoke test PASSED. Design generation is working."); } async function main(): Promise { @@ -133,7 +137,7 @@ async function main(): Promise { switch (command) { case "generate": - await generate({ + process.exitCode = (await generate({ brief: flags.brief as string, briefFile: flags["brief-file"] as string, output: (flags.output as string) || "/tmp/gstack-mockup.png", @@ -141,7 +145,7 @@ async function main(): Promise { retry: normalizeIntFlag(flags.retry, { name: "retry", def: 0, min: 0 }), size: flags.size as string, quality: flags.quality as string, - }); + })).exitCode; break; case "check": @@ -149,9 +153,7 @@ async function main(): Promise { break; case "compare": { - // Parse --images as glob or multiple files - const imagesArg = flags.images as string; - const images = await resolveImagePaths(imagesArg); + const images = await resolveImagePaths(flags.images as string, flags["images-file"] as string | undefined); const outputPath = (flags.output as string) || "/tmp/gstack-design-board.html"; compare({ images, output: outputPath }); // If --serve flag is set, publish the board. @@ -195,7 +197,7 @@ async function main(): Promise { break; case "variants": - await variants({ + process.exitCode = await variants({ brief: flags.brief as string, briefFile: flags["brief-file"] as string, // #2032: pass the RAW flag through — variants() normalizes at its @@ -209,7 +211,7 @@ async function main(): Promise { break; case "iterate": - await iterate({ + process.exitCode = await iterate({ session: flags.session as string, feedback: flags.feedback as string, output: (flags.output as string) || "/tmp/gstack-iterate.png", @@ -261,7 +263,7 @@ async function main(): Promise { } case "evolve": - await evolve({ + process.exitCode = await evolve({ screenshot: flags.screenshot as string, brief: flags.brief as string, output: (flags.output as string) || "/tmp/gstack-evolved.png", @@ -381,31 +383,6 @@ function openBrowser(url: string): void { } } -/** - * Resolve image paths from a glob pattern or comma-separated list. - */ -async function resolveImagePaths(input: string): Promise { - if (!input) { - console.error("--images is required. Provide glob pattern or comma-separated paths."); - process.exit(1); - } - - // Check if it's a glob pattern - if (input.includes("*")) { - const glob = new Bun.Glob(input); - const paths: string[] = []; - for await (const match of glob.scan({ absolute: true })) { - if (match.endsWith(".png") || match.endsWith(".jpg") || match.endsWith(".jpeg")) { - paths.push(match); - } - } - return paths.sort(); - } - - // Comma-separated or single path - return input.split(",").map(p => p.trim()); -} - // Self-execution shortcut: when invoked with --daemon-mode, this same // binary runs as the persistent design daemon instead of the CLI. Keeps // the production install to a single executable; daemon-client.ts spawns diff --git a/design/src/commands.ts b/design/src/commands.ts index 17d5811a7..76ff9ded7 100644 --- a/design/src/commands.ts +++ b/design/src/commands.ts @@ -16,17 +16,17 @@ export const COMMANDS = new Map([ ["generate", { description: "Generate a UI mockup from a design brief", - usage: "generate --brief \"...\" --output /path.png", + usage: "generate --brief \"...\" --output /path.png [--check --retry N] (never overwrites: bumps to /path-2.png; each --retry attempt is its own saved image; prints outputPath, attempts + run fields)", flags: ["--brief", "--brief-file", "--output", "--check", "--retry", "--size", "--quality"], }], ["variants", { description: "Generate N design variants from a brief", - usage: "variants --brief \"...\" --count 3 --output-dir /path/", + usage: "variants --brief \"...\" --count 3 --output-dir /path/ [--viewports desktop,tablet,mobile] (never overwrites: variant-A.png bumps to variant-A-2.png; prints paths, errors + run fields)", flags: ["--brief", "--brief-file", "--count", "--output-dir", "--size", "--quality", "--viewports"], }], ["iterate", { description: "Iterate on an existing mockup with feedback", - usage: "iterate --session /path/session.json --feedback \"...\" --output /path.png", + usage: "iterate --session /path/session.json --feedback \"...\" --output /path.png (never overwrites; prints outputPath + run fields)", flags: ["--session", "--feedback", "--output"], }], ["check", { @@ -36,8 +36,8 @@ export const COMMANDS = new Map x-2.png, x-2.png -> x-2-2.png), stderr prints +"note: exists; saved to (existing file kept)", and the JSON +reports the actual path. Read paths from the JSON, never from a fixed file name. + +Run fields printed by generate, iterate, evolve and variants (on every exit): + requested images requested saved every saved path, in order + selected outputPath or paths failures [{file, reason}] not saved + recovered [{path, reason}] private recovery copies in the temp dir + + command extra fields readers + generate outputPath sessionFile responseId checkResult design-review, design-shotgun, + attempts[{path, check}] (one per --retry try) plan-design-review + iterate outputPath sessionFile responseId iteration plan-design-review, design-consultation + evolve outputPath sourceScreenshot brief attempts design-shotgun + variants outputDir count succeeded failed paths errors office-hours, plan-design-review, + viewports (--viewports only) design-consultation + compare board HTML; --serve prints {id, url, sourceDir} all board flows (--images-file) + +Exit codes: 0 a selected result exists; 2 nothing was saved; 3 stopped after +saving at least one image but before its selected result (interrupted retry); +1 invalid flags rejected before any API call (no JSON).`; diff --git a/design/src/evolve.ts b/design/src/evolve.ts index a656054b0..69b486849 100644 --- a/design/src/evolve.ts +++ b/design/src/evolve.ts @@ -6,10 +6,10 @@ */ import fs from "fs"; -import path from "path"; import { requireApiKey } from "./auth"; import { receiptedFetch } from "./receipted-fetch"; import { imageRequestBody, modelRejectionHint, visionRequestBody } from "./models"; +import { emitResult, exitCodeFor, newAccounting, persistImage, recordOutcome, type ExitCode } from "./persist"; export interface EvolveOptions { screenshot: string; // Path to current site screenshot @@ -22,37 +22,68 @@ export interface EvolveOptions { * Sends the screenshot as context to the image model with image generation, * asking it to produce a new version incorporating the brief's changes. */ -export async function evolve(options: EvolveOptions): Promise { - const apiKey = requireApiKey(); - const screenshotData = fs.readFileSync(options.screenshot).toString("base64"); +export async function evolve(options: EvolveOptions): Promise { + const acct = newAccounting(1); + let outputPath: string | null = null; - console.error(`Evolving ${options.screenshot} with: "${options.brief}"`); - const startTime = Date.now(); + try { + const apiKey = requireApiKey(); + const screenshotData = fs.readFileSync(options.screenshot).toString("base64"); - // Use the Responses API with both a text prompt referencing the screenshot - // and the image_generation tool to produce the evolved version. - // Since we can't send reference images directly to image_generation, - // we describe the current state in detail first via vision, then generate. + console.error(`Evolving ${options.screenshot} with: "${options.brief}"`); + const startTime = Date.now(); - // Step 1: Analyze current screenshot - const analysis = await analyzeScreenshot(apiKey, screenshotData); - console.error(` Analyzed current design: ${analysis.slice(0, 100)}...`); + // Use the Responses API with both a text prompt referencing the screenshot + // and the image_generation tool to produce the evolved version. + // Since we can't send reference images directly to image_generation, + // we describe the current state in detail first via vision, then generate. - // Step 2: Generate evolved version using analysis + brief - const evolvedPrompt = [ - "Generate a pixel-perfect UI mockup that is an improved version of an existing design.", - "", - "CURRENT DESIGN (what exists now):", - analysis, - "", - "REQUESTED CHANGES:", - options.brief, - "", - "Generate a new mockup that keeps the existing layout structure but applies the requested changes.", - "The result should look like a real production UI. All text must be readable.", - "1536x1024 pixels.", - ].join("\n"); + // Step 1: Analyze current screenshot + const analysis = await analyzeScreenshot(apiKey, screenshotData); + console.error(` Analyzed current design: ${analysis.slice(0, 100)}...`); + // Step 2: Generate evolved version using analysis + brief + const evolvedPrompt = [ + "Generate a pixel-perfect UI mockup that is an improved version of an existing design.", + "", + "CURRENT DESIGN (what exists now):", + analysis, + "", + "REQUESTED CHANGES:", + options.brief, + "", + "Generate a new mockup that keeps the existing layout structure but applies the requested changes.", + "The result should look like a real production UI. All text must be readable.", + "1536x1024 pixels.", + ].join("\n"); + + const imageData = await requestEvolvedImage(apiKey, evolvedPrompt); + const outcome = persistImage(imageData, options.output); + outputPath = recordOutcome(acct, outcome); + if (outcome.ok) { + const elapsed = ((Date.now() - startTime) / 1000).toFixed(1); + console.error(`Generated (${elapsed}s, ${(outcome.bytes / 1024).toFixed(0)}KB) → ${outcome.path}`); + } + } catch (err: any) { + const reason = err?.message || String(err); + console.error(reason); + acct.failures.push({ file: options.output, reason }); + } + + return emitResult({ + outputPath, + sourceScreenshot: options.screenshot, + brief: options.brief, + attempts: acct.saved.map(p => ({ path: p })), + requested: acct.requested, + saved: acct.saved, + selected: outputPath, + failures: acct.failures, + recovered: acct.recovered, + }, exitCodeFor(outputPath !== null, acct.saved.length)); +} + +async function requestEvolvedImage(apiKey: string, evolvedPrompt: string): Promise { const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), 240_000); @@ -85,19 +116,7 @@ export async function evolve(options: EvolveOptions): Promise { if (!imageItem?.result) { throw new Error("No image data in response"); } - - fs.mkdirSync(path.dirname(options.output), { recursive: true }); - const imageBuffer = Buffer.from(imageItem.result, "base64"); - fs.writeFileSync(options.output, imageBuffer); - - const elapsed = ((Date.now() - startTime) / 1000).toFixed(1); - console.error(`Generated (${elapsed}s, ${(imageBuffer.length / 1024).toFixed(0)}KB) → ${options.output}`); - - console.log(JSON.stringify({ - outputPath: options.output, - sourceScreenshot: options.screenshot, - brief: options.brief, - }, null, 2)); + return imageItem.result; } finally { clearTimeout(timeout); } diff --git a/design/src/gallery.ts b/design/src/gallery.ts index 95675559d..5ab08a486 100644 --- a/design/src/gallery.ts +++ b/design/src/gallery.ts @@ -6,6 +6,7 @@ import fs from "fs"; import path from "path"; +import { approvedImagePath } from "./approval"; export interface GalleryOptions { designsDir: string; // ~/.gstack/projects/$SLUG/designs/ @@ -20,6 +21,32 @@ interface SessionData { variants: string[]; // paths to variant PNGs } +const VARIANT_IMAGE = [/^variant-[A-Z](-\d+)?\.png$/i, /^variant-\d+(-\d+)?\.png$/i]; + +/** + * Variant images in a session directory as chronological history, not rounds: + * names not on the current board sort by mtime then name, followed by the + * board's images in board-images.json order. Never-overwrite means one + * letter can appear in several rounds (variant-A.png, variant-A-2.png). + */ +export function listVariantImages(sessionDir: string): string[] { + const found = fs.readdirSync(sessionDir) + .filter(f => VARIANT_IMAGE.some(re => re.test(f))) + .map(f => path.join(sessionDir, f)); + + let board: string[] = []; + try { + const parsed = JSON.parse(fs.readFileSync(path.join(sessionDir, "board-images.json"), "utf-8")); + if (Array.isArray(parsed)) board = parsed.filter((p): p is string => typeof p === "string").map(p => path.resolve(sessionDir, p)); + } catch {} + + const mtime = new Map(found.map(f => [f, fs.statSync(f).mtimeMs])); + const history = found + .filter(f => !board.includes(f)) + .sort((a, b) => (mtime.get(a)! - mtime.get(b)!) || a.localeCompare(b)); + return [...history, ...board.filter(f => found.includes(f))]; +} + export function generateGalleryHtml(designsDir: string): string { const sessions: SessionData[] = []; @@ -44,16 +71,9 @@ export function generateGalleryHtml(designsDir: string): string { } } - // Find variant PNGs - const variants: string[] = []; + let variants: string[] = []; try { - const files = fs.readdirSync(sessionDir); - for (const f of files) { - if (f.match(/variant-[A-Z]\.png$/i) || f.match(/variant-\d+\.png$/i)) { - variants.push(path.join(sessionDir, f)); - } - } - variants.sort(); + variants = listVariantImages(sessionDir); } catch { // Can't read directory, skip } @@ -81,12 +101,13 @@ export function generateGalleryHtml(designsDir: string): string { sessions.sort((a, b) => b.date.localeCompare(a.date)); const sessionCards = sessions.map(session => { + const approvedTarget = session.approved ? approvedImagePath(session.approved, session.dir) : null; const variantImgs = session.variants.map((vPath, i) => { try { const imgData = fs.readFileSync(vPath).toString("base64"); const ext = path.extname(vPath).slice(1) || "png"; const label = path.basename(vPath, `.${ext}`).replace("variant-", ""); - const isApproved = session.approved?.approved_variant === label; + const isApproved = approvedTarget?.path === vPath; return `
Variant ${label} @@ -200,7 +221,7 @@ export function generateGalleryHtml(designsDir: string): string {

Design History

-
${sessions.length} exploration${sessions.length === 1 ? "" : "s"}
+
${sessions.length} exploration${sessions.length === 1 ? "" : "s"} · images oldest to newest; letters do not identify rounds
${sessionCards} diff --git a/design/src/generate.ts b/design/src/generate.ts index 49bd401d0..c7ea9a796 100644 --- a/design/src/generate.ts +++ b/design/src/generate.ts @@ -2,14 +2,13 @@ * Generate UI mockups via OpenAI Responses API with image_generation tool. */ -import fs from "fs"; -import path from "path"; import { requireApiKey } from "./auth"; import { receiptedFetch } from "./receipted-fetch"; import { imageRequestBody, modelRejectionHint } from "./models"; import { parseBrief } from "./brief"; import { createSession, sessionPath } from "./session"; import { checkMockup } from "./check"; +import { emitResult, exitCodeFor, newAccounting, persistImage, recordOutcome, type ExitCode, type RunAccounting } from "./persist"; export interface GenerateOptions { brief?: string; @@ -21,11 +20,21 @@ export interface GenerateOptions { quality?: string; } -export interface GenerateResult { - outputPath: string; +export interface GenerateAttempt { + path: string; sessionFile: string; responseId: string; + check?: { pass: boolean; issues: string }; +} + +export interface GenerateResult extends RunAccounting { + outputPath: string | null; + sessionFile: string | null; + responseId: string | null; checkResult?: { pass: boolean; issues: string }; + attempts: GenerateAttempt[]; + selected: string | null; + exitCode: ExitCode; } /** @@ -86,69 +95,80 @@ async function callImageGeneration( } /** - * Generate a single mockup from a brief. + * Generate a single mockup from a brief. Every --retry attempt is its own paid + * image and is saved under its own claimed name before its quality check. */ export async function generate(options: GenerateOptions): Promise { - const apiKey = requireApiKey(); + const acct = newAccounting(1); + const attempts: GenerateAttempt[] = []; + let selected: GenerateAttempt | null = null; - // Parse the brief - const prompt = options.briefFile - ? parseBrief(options.briefFile, true) - : parseBrief(options.brief!, false); + try { + const apiKey = requireApiKey(); + const prompt = options.briefFile + ? parseBrief(options.briefFile, true) + : parseBrief(options.brief!, false); + const size = options.size || "1536x1024"; + const quality = options.quality || "high"; + const maxRetries = options.retry ?? 0; - const size = options.size || "1536x1024"; - const quality = options.quality || "high"; - const maxRetries = options.retry ?? 0; - - let lastResult: GenerateResult | null = null; - - for (let attempt = 0; attempt <= maxRetries; attempt++) { - if (attempt > 0) { - console.error(`Retry ${attempt}/${maxRetries}...`); - } - - // Generate the image - const startTime = Date.now(); - const { responseId, imageData } = await callImageGeneration(apiKey, prompt, size, quality); - const elapsed = ((Date.now() - startTime) / 1000).toFixed(1); - - // Write to disk - const outputDir = path.dirname(options.output); - fs.mkdirSync(outputDir, { recursive: true }); - const imageBuffer = Buffer.from(imageData, "base64"); - fs.writeFileSync(options.output, imageBuffer); - - // Create session - const session = createSession(responseId, prompt, options.output); - - console.error(`Generated (${elapsed}s, ${(imageBuffer.length / 1024).toFixed(0)}KB) → ${options.output}`); - - lastResult = { - outputPath: options.output, - sessionFile: sessionPath(session.id), - responseId, - }; - - // Quality check if requested - if (options.check) { - const checkResult = await checkMockup(options.output, prompt); - lastResult.checkResult = checkResult; + for (let attempt = 0; attempt <= maxRetries; attempt++) { + if (attempt > 0) { + console.error(`Retry ${attempt}/${maxRetries}...`); + } + + const startTime = Date.now(); + const { responseId, imageData } = await callImageGeneration(apiKey, prompt, size, quality); + const elapsed = ((Date.now() - startTime) / 1000).toFixed(1); + + const outcome = persistImage(imageData, options.output); + const savedPath = recordOutcome(acct, outcome); + if (!savedPath) break; + + const session = createSession(responseId, prompt, savedPath); + const entry: GenerateAttempt = { path: savedPath, sessionFile: sessionPath(session.id), responseId }; + attempts.push(entry); + console.error(`Generated (${elapsed}s, ${outcome.ok ? (outcome.bytes / 1024).toFixed(0) : 0}KB) → ${savedPath}`); + + if (!options.check) { + selected = entry; + break; + } - if (checkResult.pass) { + entry.check = await checkMockup(savedPath, prompt); + if (entry.check.pass) { console.error(`Quality check: PASS`); + selected = entry; break; + } + console.error(`Quality check: FAIL — ${entry.check.issues}`); + if (attempt < maxRetries) { + console.error("Will retry..."); } else { - console.error(`Quality check: FAIL — ${checkResult.issues}`); - if (attempt < maxRetries) { - console.error("Will retry..."); - } + selected = entry; } - } else { - break; } + } catch (err: any) { + const reason = err?.message || String(err); + console.error(reason); + acct.failures.push({ file: options.output, reason }); } - // Output result as JSON to stdout - console.log(JSON.stringify(lastResult, null, 2)); - return lastResult!; + const exitCode = exitCodeFor(selected !== null, acct.saved.length); + const result: GenerateResult = { + outputPath: selected?.path ?? null, + sessionFile: selected?.sessionFile ?? null, + responseId: selected?.responseId ?? null, + ...(selected?.check ? { checkResult: selected.check } : {}), + attempts, + requested: acct.requested, + saved: acct.saved, + selected: selected?.path ?? null, + failures: acct.failures, + recovered: acct.recovered, + exitCode, + }; + const { exitCode: _omit, ...printed } = result; + emitResult(printed, exitCode); + return result; } diff --git a/design/src/image-args.ts b/design/src/image-args.ts new file mode 100644 index 000000000..5662f87ff --- /dev/null +++ b/design/src/image-args.ts @@ -0,0 +1,53 @@ +/** + * Image list parsing for `$D compare`. Accepted forms, in order: + * --images-file JSON array of path strings (lossless; skills use this) + * --images '[...]' JSON array string, only when it starts with `[` and parses as string[] + * --images 'dir/*.png' glob (sorted) + * --images 'a.png,b.png' comma-separated list, or a single path + */ + +import fs from "fs"; + +function parseStringArray(text: string): string[] | null { + try { + const value = JSON.parse(text); + if (Array.isArray(value) && value.every(item => typeof item === "string")) return value; + } catch {} + return null; +} + +export async function resolveImagePaths(input: string | undefined, imagesFile?: string): Promise { + if (imagesFile) { + let text: string; + try { + text = fs.readFileSync(imagesFile, "utf-8"); + } catch (err: any) { + throw new Error(`cannot read --images-file ${imagesFile}: ${err.code || err.message}`); + } + const list = parseStringArray(text); + if (!list) throw new Error(`--images-file ${imagesFile} must contain a JSON array of image path strings`); + return list; + } + + if (!input) { + throw new Error("--images or --images-file is required. Provide a JSON array file, glob pattern or comma-separated paths."); + } + + if (input.trimStart().startsWith("[")) { + const list = parseStringArray(input); + if (list) return list; + } + + if (input.includes("*")) { + const glob = new Bun.Glob(input); + const paths: string[] = []; + for await (const match of glob.scan({ absolute: true })) { + if (match.endsWith(".png") || match.endsWith(".jpg") || match.endsWith(".jpeg")) { + paths.push(match); + } + } + return paths.sort(); + } + + return input.split(",").map(p => p.trim()); +} diff --git a/design/src/iterate.ts b/design/src/iterate.ts index 85b70d4cc..47bf196c8 100644 --- a/design/src/iterate.ts +++ b/design/src/iterate.ts @@ -6,12 +6,11 @@ * with original brief + accumulated feedback in a single prompt. */ -import fs from "fs"; -import path from "path"; import { requireApiKey } from "./auth"; import { receiptedFetch } from "./receipted-fetch"; import { imageRequestBody, modelRejectionHint } from "./models"; import { readSession, updateSession } from "./session"; +import { emitResult, exitCodeFor, newAccounting, persistImage, recordOutcome, type ExitCode } from "./persist"; export interface IterateOptions { session: string; // Path to session JSON file @@ -20,62 +19,64 @@ export interface IterateOptions { } /** - * Iterate on an existing design using session state. + * Iterate on an existing design using session state. One received image means + * one claim; a local save failure never triggers the fallback purchase. */ -export async function iterate(options: IterateOptions): Promise { - const apiKey = requireApiKey(); - const session = readSession(options.session); - - console.error(`Iterating on session ${session.id}...`); - console.error(` Previous iterations: ${session.feedbackHistory.length}`); - console.error(` Feedback: "${options.feedback}"`); - - const startTime = Date.now(); - - // Try multi-turn with previous_response_id first - let success = false; - let responseId = ""; +export async function iterate(options: IterateOptions): Promise { + const acct = newAccounting(1); + let outputPath: string | null = null; + let responseId: string | null = null; + let iteration: number | null = null; try { - const result = await callWithThreading(apiKey, session.lastResponseId, options.feedback); - responseId = result.responseId; + const apiKey = requireApiKey(); + const session = readSession(options.session); - fs.mkdirSync(path.dirname(options.output), { recursive: true }); - fs.writeFileSync(options.output, Buffer.from(result.imageData, "base64")); - success = true; + console.error(`Iterating on session ${session.id}...`); + console.error(` Previous iterations: ${session.feedbackHistory.length}`); + console.error(` Feedback: "${options.feedback}"`); + + const startTime = Date.now(); + let image: { responseId: string; imageData: string }; + try { + image = await callWithThreading(apiKey, session.lastResponseId, options.feedback); + } catch (err: any) { + console.error(` Threading failed: ${err.message}`); + console.error(" Falling back to re-generation with accumulated feedback..."); + const accumulatedPrompt = buildAccumulatedPrompt( + session.originalBrief, + [...session.feedbackHistory, options.feedback] + ); + image = await callFresh(apiKey, accumulatedPrompt); + } + + const outcome = persistImage(image.imageData, options.output); + outputPath = recordOutcome(acct, outcome); + if (outcome.ok) { + responseId = image.responseId; + const elapsed = ((Date.now() - startTime) / 1000).toFixed(1); + console.error(`Generated (${elapsed}s, ${(outcome.bytes / 1024).toFixed(0)}KB) → ${outcome.path}`); + updateSession(session, image.responseId, options.feedback, outcome.path); + iteration = session.feedbackHistory.length + 1; + } } catch (err: any) { - console.error(` Threading failed: ${err.message}`); - console.error(" Falling back to re-generation with accumulated feedback..."); - - // Fallback: re-generate with original brief + all feedback - const accumulatedPrompt = buildAccumulatedPrompt( - session.originalBrief, - [...session.feedbackHistory, options.feedback] - ); - - const result = await callFresh(apiKey, accumulatedPrompt); - responseId = result.responseId; - - fs.mkdirSync(path.dirname(options.output), { recursive: true }); - fs.writeFileSync(options.output, Buffer.from(result.imageData, "base64")); - success = true; + const reason = err?.message || String(err); + console.error(reason); + acct.failures.push({ file: options.output, reason }); } - if (success) { - const elapsed = ((Date.now() - startTime) / 1000).toFixed(1); - const size = fs.statSync(options.output).size; - console.error(`Generated (${elapsed}s, ${(size / 1024).toFixed(0)}KB) → ${options.output}`); - - // Update session - updateSession(session, responseId, options.feedback, options.output); - - console.log(JSON.stringify({ - outputPath: options.output, - sessionFile: options.session, - responseId, - iteration: session.feedbackHistory.length + 1, - }, null, 2)); - } + return emitResult({ + outputPath, + sessionFile: options.session, + responseId, + iteration, + attempts: acct.saved.map(p => ({ path: p })), + requested: acct.requested, + saved: acct.saved, + selected: outputPath, + failures: acct.failures, + recovered: acct.recovered, + }, exitCodeFor(outputPath !== null, acct.saved.length)); } async function callWithThreading( diff --git a/design/src/persist.ts b/design/src/persist.ts new file mode 100644 index 000000000..37b16a121 --- /dev/null +++ b/design/src/persist.ts @@ -0,0 +1,196 @@ +/** + * Persistence for paid design images. Every image the API returns goes through + * persistImage exactly once: mkdir, exclusive claim (never overwrite), write, + * fsync, close. Any failure after the bytes arrived removes the claimed file and + * leaves a private recovery copy in os.tmpdir(), so a paid image is never lost + * or silently replaced. + * + * Every generating command prints the same accounting fields (see + * commands.ts for the $D contract): + * requested number of images the command set out to produce + * saved every image path written, in order + * selected outputPath (single-image commands) or paths (variants) + * failures [{file, reason}] for every image that was not saved + * recovered [{path, reason}] recovery copies written after a save failed + * Exit 0 when a selected result exists, 2 when nothing was saved, 3 when the + * run stopped after saving at least one image but before its selected result. + */ + +import crypto from "crypto"; +import fs from "fs"; +import os from "os"; +import path from "path"; + +export const CLAIM_CAP = 999; + +export interface PersistFs { + mkdirSync(p: string, opts: { recursive: true }): unknown; + openSync(p: string, flags: string, mode?: number): number; + writeSync(fd: number, buf: Buffer, offset: number, length: number): number; + fsyncSync(fd: number): void; + closeSync(fd: number): void; + unlinkSync(p: string): void; +} + +export interface PersistDeps { + fs?: PersistFs; + tmpdir?: string; +} + +export interface Failure { file: string; reason: string } +export interface Recovery { path: string; reason: string } + +export type PersistOutcome = + | { ok: true; path: string; bytes: number } + | { ok: false; failure: Failure; recovered?: Recovery }; + +export interface RunAccounting { + requested: number; + saved: string[]; + failures: Failure[]; + recovered: Recovery[]; +} + +/** `x.png` -> `x-2.png`; `x-2.png` -> `x-2-2.png`. The suffix always appends to the requested stem. */ +export function bumpedName(requested: string, n: number): string { + const ext = path.extname(requested); + return `${requested.slice(0, requested.length - ext.length)}-${n}${ext}`; +} + +/** + * Atomically create the first free name among requested, -2 ... -999 with an + * exclusive `wx` open. Only EEXIST bumps; any other open error throws at once. + */ +export function claimOutputPath(requested: string, fsImpl: PersistFs = fs): { path: string; fd: number } { + for (let n = 1; n <= CLAIM_CAP; n++) { + const candidate = n === 1 ? requested : bumpedName(requested, n); + try { + return { path: candidate, fd: fsImpl.openSync(candidate, "wx") }; + } catch (err: any) { + if (err?.code !== "EEXIST") throw err; + } + } + const capErr = new Error(`${requested} and every name through ${path.basename(bumpedName(requested, CLAIM_CAP))} already exist`); + throw Object.assign(capErr, { code: "EEXIST" }); +} + +function writeAll(fsImpl: PersistFs, fd: number, buf: Buffer): void { + let offset = 0; + while (offset < buf.length) { + offset += fsImpl.writeSync(fd, buf, offset, buf.length - offset); + } +} + +function fixHint(code: string, dir: string): string { + switch (code) { + case "EACCES": + case "EPERM": + case "EROFS": + return `make ${dir} writable or pass a writable --output/--output-dir`; + case "ENOSPC": + case "EDQUOT": + return `free disk space on the volume holding ${dir}, then rerun`; + case "EEXIST": + return `choose a new --output name or move older images out of ${dir}`; + case "ENOENT": + case "ENOTDIR": + return `check that the --output/--output-dir path is a directory you can create`; + default: + return `pass a writable --output/--output-dir`; + } +} + +function describeError(err: any): { code: string; cause: string } { + return { code: err?.code || "ERROR", cause: err?.message || String(err) }; +} + +function writeRecoveryCopy(bytes: Buffer, fsImpl: PersistFs, tmp: string): string { + const stamp = new Date().toISOString().replace(/[-:.]/g, ""); + const recoveryPath = path.join(tmp, `gstack-design-unsaved-${stamp}-${crypto.randomBytes(4).toString("hex")}.png`); + const fd = fsImpl.openSync(recoveryPath, "wx", 0o600); + try { + writeAll(fsImpl, fd, bytes); + fsImpl.closeSync(fd); + } catch (err) { + try { fsImpl.closeSync(fd); } catch {} + try { fsImpl.unlinkSync(recoveryPath); } catch {} + throw err; + } + return recoveryPath; +} + +/** + * Save one received image under `requested` (or its first free -N name). + * Empty image data is a generation failure: no claim, no file. + */ +export function persistImage(imageData: string, requested: string, deps: PersistDeps = {}): PersistOutcome { + const fsImpl = deps.fs ?? fs; + const bytes = Buffer.from(imageData || "", "base64"); + if (bytes.length === 0) { + const reason = "API returned an empty image; nothing was saved"; + console.error(` ✗ ${path.basename(requested)}: ${reason}`); + return { ok: false, failure: { file: requested, reason } }; + } + + let claimed: { path: string; fd: number } | null = null; + try { + fsImpl.mkdirSync(path.dirname(requested), { recursive: true }); + claimed = claimOutputPath(requested, fsImpl); + writeAll(fsImpl, claimed.fd, bytes); + fsImpl.fsyncSync(claimed.fd); + fsImpl.closeSync(claimed.fd); + if (claimed.path !== requested) { + console.error(`note: ${requested} exists; saved to ${claimed.path} (existing file kept)`); + } + return { ok: true, path: claimed.path, bytes: bytes.length }; + } catch (err) { + const target = claimed?.path ?? requested; + if (claimed) { + try { fsImpl.closeSync(claimed.fd); } catch {} + try { fsImpl.unlinkSync(claimed.path); } catch {} + } + const { code, cause } = describeError(err); + const fix = fixHint(code, path.dirname(requested)); + const tmp = deps.tmpdir ?? os.tmpdir(); + let reason: string; + let recovered: Recovery | undefined; + try { + const recoveryPath = writeRecoveryCopy(bytes, fsImpl, tmp); + reason = `cannot save paid image to ${target}: ${code} (${cause}). Image bytes were received; saved a recovery copy to ${recoveryPath}. Fix: ${fix}.`; + recovered = { path: recoveryPath, reason: `${code} (${cause})` }; + } catch (recoveryErr) { + const second = describeError(recoveryErr); + reason = `cannot save paid image to ${target}: ${code} (${cause}). Image bytes were received (${bytes.length} bytes) but the recovery copy in ${tmp} also failed: ${second.code} (${second.cause}). Fix: ${fix}.`; + } + console.error(reason); + return { ok: false, failure: { file: requested, reason }, ...(recovered ? { recovered } : {}) }; + } +} + +export function newAccounting(requested: number): RunAccounting { + return { requested, saved: [], failures: [], recovered: [] }; +} + +/** Fold one persist outcome into the run's accounting; returns the saved path or null. */ +export function recordOutcome(acct: RunAccounting, outcome: PersistOutcome): string | null { + if (outcome.ok) { + acct.saved.push(outcome.path); + return outcome.path; + } + acct.failures.push(outcome.failure); + if (outcome.recovered) acct.recovered.push(outcome.recovered); + return null; +} + +export function exitCodeFor(hasSelected: boolean, savedCount: number): ExitCode { + if (hasSelected) return 0; + return savedCount === 0 ? 2 : 3; +} + +export type ExitCode = 0 | 2 | 3; + +/** Print the command's JSON on stdout and return its exit code for the CLI. */ +export function emitResult(result: Record, exitCode: ExitCode): ExitCode { + console.log(JSON.stringify(result, null, 2)); + return exitCode; +} diff --git a/design/src/variants.ts b/design/src/variants.ts index 5042da98c..7a1e14316 100644 --- a/design/src/variants.ts +++ b/design/src/variants.ts @@ -11,6 +11,15 @@ import { receiptedFetch } from "./receipted-fetch"; import { imageRequestBody, modelRejectionHint } from "./models"; import { parseBrief } from "./brief"; import { normalizeIntFlag } from "./flag-utils"; +import { + emitResult, + exitCodeFor, + newAccounting, + persistImage, + type ExitCode, + type Recovery, + type RunAccounting, +} from "./persist"; export interface VariantsOptions { brief?: string; @@ -37,20 +46,25 @@ const STYLE_VARIATIONS = [ "Use a playful, modern style with asymmetric layout and unexpected color accents.", ]; +export interface VariantResult { + requested: string; + path: string; + success: boolean; + error?: string; + recovered?: Recovery; +} + /** - * Generate a single variant with retry on 429. - * - * Exported for testability. Pass `fetchFn` to inject a stubbed fetch in tests; - * production code uses the global fetch by default. + * Request one variant image, retrying only the API call on 429. Returns the + * base64 image or the final error; nothing here touches the filesystem. */ -export async function generateVariant( +export async function requestVariantImage( apiKey: string, prompt: string, - outputPath: string, size: string, quality: string, fetchFn: typeof globalThis.fetch = globalThis.fetch, -): Promise<{ path: string; success: boolean; error?: string }> { +): Promise<{ imageData: string } | { error: string }> { const maxRetries = 3; const MAX_RETRY_AFTER_MS = 60_000; // cap honored Retry-After to bound stalls let lastError = ""; @@ -59,7 +73,7 @@ export async function generateVariant( try { body = imageRequestBody(prompt, { size, quality }); } catch (err: any) { - return { path: outputPath, success: false, error: err.message }; + return { error: err.message }; } for (let attempt = 0; attempt <= maxRetries; attempt++) { @@ -74,8 +88,9 @@ export async function generateVariant( const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), 240_000); + let response: Response; try { - const response = await receiptedFetch("variants-image-request", "https://api.openai.com/v1/responses", { + response = await receiptedFetch("variants-image-request", "https://api.openai.com/v1/responses", { method: "POST", headers: { "Authorization": `Bearer ${apiKey}`, @@ -84,83 +99,174 @@ export async function generateVariant( body, signal: controller.signal, }, fetchFn); - - clearTimeout(timeout); - - if (response.status === 429) { - lastError = "Rate limited (429)"; - const retryAfter = response.headers.get("retry-after"); - if (retryAfter) { - const trimmed = retryAfter.trim(); - let waitMs: number | null = null; - if (/^\d+$/.test(trimmed)) { - // delta-seconds (RFC 7231) - waitMs = Math.min(Number.parseInt(trimmed, 10) * 1000, MAX_RETRY_AFTER_MS); - } else { - // HTTP-date (RFC 7231) - const dateMs = Date.parse(trimmed); - if (!Number.isNaN(dateMs)) { - waitMs = Math.min(Math.max(0, dateMs - Date.now()), MAX_RETRY_AFTER_MS); - } - } - if (waitMs !== null) { - if (waitMs > 0) { - await new Promise(resolve => setTimeout(resolve, waitMs)); - } - // Honored Retry-After (incl. 0 / past date "retry now") — skip the - // next iteration's leading exponential sleep so we don't double-wait. - skipLeadingDelay = true; - } - } - continue; - } - - if (!response.ok) { - const error = await response.text(); - if (response.status === 403 && error.includes("organization must be verified")) { - return { path: outputPath, success: false, error: "OpenAI organization verification required. Go to https://platform.openai.com/settings/organization to verify." }; - } - return { path: outputPath, success: false, error: `API error (${response.status}): ${error.slice(0, 200)}${modelRejectionHint(response.status, error, "image")}` }; - } - - const data = await response.json() as any; - const imageItem = data.output?.find((item: any) => item.type === "image_generation_call"); - - if (!imageItem?.result) { - return { path: outputPath, success: false, error: "No image data in response" }; - } - - fs.writeFileSync(outputPath, Buffer.from(imageItem.result, "base64")); - return { path: outputPath, success: true }; } catch (err: any) { clearTimeout(timeout); - if (err.name === "AbortError") { - return { path: outputPath, success: false, error: "Timeout (240s)" }; - } + if (err.name === "AbortError") return { error: "Timeout (240s)" }; lastError = err.message; + continue; + } + clearTimeout(timeout); + + if (response.status === 429) { + lastError = "Rate limited (429)"; + const retryAfter = response.headers.get("retry-after"); + if (retryAfter) { + const trimmed = retryAfter.trim(); + let waitMs: number | null = null; + if (/^\d+$/.test(trimmed)) { + // delta-seconds (RFC 7231) + waitMs = Math.min(Number.parseInt(trimmed, 10) * 1000, MAX_RETRY_AFTER_MS); + } else { + // HTTP-date (RFC 7231) + const dateMs = Date.parse(trimmed); + if (!Number.isNaN(dateMs)) { + waitMs = Math.min(Math.max(0, dateMs - Date.now()), MAX_RETRY_AFTER_MS); + } + } + if (waitMs !== null) { + if (waitMs > 0) { + await new Promise(resolve => setTimeout(resolve, waitMs)); + } + // Honored Retry-After (incl. 0 / past date "retry now") — skip the + // next iteration's leading exponential sleep so we don't double-wait. + skipLeadingDelay = true; + } + } + continue; + } + + if (!response.ok) { + const error = await response.text(); + if (response.status === 403 && error.includes("organization must be verified")) { + return { error: "OpenAI organization verification required. Go to https://platform.openai.com/settings/organization to verify." }; + } + return { error: `API error (${response.status}): ${error.slice(0, 200)}${modelRejectionHint(response.status, error, "image")}` }; + } + + try { + const data = await response.json() as any; + const imageItem = data.output?.find((item: any) => item.type === "image_generation_call"); + if (!imageItem?.result) return { error: "No image data in response" }; + return { imageData: imageItem.result }; + } catch (err: any) { + return { error: `Unreadable API response: ${err.message}` }; } } - return { path: outputPath, success: false, error: lastError }; + return { error: lastError }; +} + +/** + * Generate a single variant: one API request (with 429 retry), then exactly + * one persistence attempt on the received bytes. A save failure is reported + * with its path and never triggers another request. + * + * Exported for testability. Pass `fetchFn` to inject a stubbed fetch in tests; + * production code uses the global fetch by default. + */ +export async function generateVariant( + apiKey: string, + prompt: string, + outputPath: string, + size: string, + quality: string, + fetchFn: typeof globalThis.fetch = globalThis.fetch, +): Promise { + const received = await requestVariantImage(apiKey, prompt, size, quality, fetchFn); + if ("error" in received) { + return { requested: outputPath, path: outputPath, success: false, error: received.error }; + } + const outcome = persistImage(received.imageData, outputPath); + if (outcome.ok) return { requested: outputPath, path: outcome.path, success: true }; + return { + requested: outputPath, + path: outputPath, + success: false, + error: outcome.failure.reason, + ...(outcome.recovered ? { recovered: outcome.recovered } : {}), + }; +} + +interface VariantJob { + outputPath: string; + prompt: string; + size: string; + label: string; +} + +/** Launch jobs 1.5s apart, wait for all, and fold them into run accounting. */ +async function runVariantJobs(apiKey: string, quality: string, jobs: VariantJob[]): Promise<{ + acct: RunAccounting; + errors: string[]; +}> { + const promises = jobs.map((job, i) => + new Promise(resolve => setTimeout(resolve, i * 1500)).then(() => { + console.error(` Starting ${job.label}...`); + return generateVariant(apiKey, job.prompt, job.outputPath, job.size, quality); + }) + ); + const results = await Promise.allSettled(promises); + + const acct = newAccounting(jobs.length); + const errors: string[] = []; + results.forEach((result, i) => { + const requested = jobs[i].outputPath; + if (result.status === "fulfilled" && result.value.success) { + const size = fs.statSync(result.value.path).size; + console.error(` ✓ ${path.basename(result.value.path)} (${(size / 1024).toFixed(0)}KB)`); + acct.saved.push(result.value.path); + return; + } + const reason = result.status === "fulfilled" ? result.value.error || "unknown error" : (result.reason as Error).message; + console.error(` ✗ ${path.basename(requested)}: ${reason}`); + errors.push(path.basename(requested)); + acct.failures.push({ file: requested, reason }); + if (result.status === "fulfilled" && result.value.recovered) acct.recovered.push(result.value.recovered); + }); + return { acct, errors }; +} + +function emitVariantsResult(outputDir: string, acct: RunAccounting, errors: string[], extra: Record = {}): ExitCode { + return emitResult({ + outputDir, + ...extra, + count: acct.requested, + succeeded: acct.saved.length, + failed: errors.length, + paths: acct.saved, + errors, + requested: acct.requested, + saved: acct.saved, + selected: acct.saved, + failures: acct.failures, + recovered: acct.recovered, + }, exitCodeFor(acct.saved.length > 0, acct.saved.length)); } /** * Generate N variants with staggered parallel execution. */ -export async function variants(options: VariantsOptions): Promise { - const apiKey = requireApiKey(); - const baseBrief = options.briefFile - ? parseBrief(options.briefFile, true) - : parseBrief(options.brief!, false); +export async function variants(options: VariantsOptions): Promise { + let apiKey: string; + let baseBrief: string; + try { + apiKey = requireApiKey(); + baseBrief = options.briefFile + ? parseBrief(options.briefFile, true) + : parseBrief(options.brief!, false); + } catch (err: any) { + const reason = err?.message || String(err); + console.error(reason); + const acct = newAccounting(0); + acct.failures.push({ file: options.outputDir, reason }); + return emitVariantsResult(options.outputDir, acct, []); + } const quality = options.quality || "high"; - fs.mkdirSync(options.outputDir, { recursive: true }); - // If viewports specified, generate responsive variants instead of style variants if (options.viewports) { - await generateResponsiveVariants(apiKey, baseBrief, options.outputDir, options.viewports, quality); - return; + return generateResponsiveVariants(apiKey, baseBrief, options.outputDir, options.viewports, quality); } // #2032: normalize at the consumption site so every caller (CLI or @@ -177,58 +283,22 @@ export async function variants(options: VariantsOptions): Promise { console.error(`Generating ${count} variants...`); const startTime = Date.now(); - // Staggered parallel: start each call 1.5s apart - const promises: Promise<{ path: string; success: boolean; error?: string }>[] = []; - + const jobs: VariantJob[] = []; for (let i = 0; i < count; i++) { const variation = STYLE_VARIATIONS[i] || ""; - const prompt = variation - ? `${baseBrief}\n\nStyle direction: ${variation}` - : baseBrief; - - const outputPath = path.join(options.outputDir, `variant-${String.fromCharCode(65 + i)}.png`); - - // Stagger: wait 1.5s between launches - const delay = i * 1500; - promises.push( - new Promise(resolve => setTimeout(resolve, delay)) - .then(() => { - console.error(` Starting variant ${String.fromCharCode(65 + i)}...`); - return generateVariant(apiKey, prompt, outputPath, size, quality); - }) - ); + const letter = String.fromCharCode(65 + i); + jobs.push({ + outputPath: path.join(options.outputDir, `variant-${letter}.png`), + prompt: variation ? `${baseBrief}\n\nStyle direction: ${variation}` : baseBrief, + size, + label: `variant ${letter}`, + }); } - const results = await Promise.allSettled(promises); + const { acct, errors } = await runVariantJobs(apiKey, quality, jobs); const elapsed = ((Date.now() - startTime) / 1000).toFixed(1); - - const succeeded: string[] = []; - const failed: string[] = []; - - for (const result of results) { - if (result.status === "fulfilled" && result.value.success) { - const size = fs.statSync(result.value.path).size; - console.error(` ✓ ${path.basename(result.value.path)} (${(size / 1024).toFixed(0)}KB)`); - succeeded.push(result.value.path); - } else { - const error = result.status === "fulfilled" ? result.value.error : (result.reason as Error).message; - const filePath = result.status === "fulfilled" ? result.value.path : "unknown"; - console.error(` ✗ ${path.basename(filePath)}: ${error}`); - failed.push(path.basename(filePath)); - } - } - - console.error(`\n${succeeded.length}/${count} variants generated (${elapsed}s)`); - - // Output structured result to stdout - console.log(JSON.stringify({ - outputDir: options.outputDir, - count, - succeeded: succeeded.length, - failed: failed.length, - paths: succeeded, - errors: failed, - }, null, 2)); + console.error(`\n${acct.saved.length}/${count} variants generated (${elapsed}s)`); + return emitVariantsResult(options.outputDir, acct, errors); } const VIEWPORT_CONFIGS: Record = { @@ -243,55 +313,34 @@ async function generateResponsiveVariants( outputDir: string, viewports: string, quality: string, -): Promise { +): Promise { const viewportList = viewports.split(",").map(v => v.trim().toLowerCase()); const configs = viewportList.map(v => VIEWPORT_CONFIGS[v]).filter(Boolean); if (configs.length === 0) { - console.error(`No valid viewports. Use: desktop, tablet, mobile`); - process.exit(1); + const reason = `No valid viewports. Use: desktop, tablet, mobile`; + console.error(reason); + const acct = newAccounting(viewportList.length); + acct.failures.push({ file: outputDir, reason }); + return emitVariantsResult(outputDir, acct, [], { viewports: viewportList }); } console.error(`Generating responsive variants: ${configs.map(c => c.desc).join(", ")}...`); const startTime = Date.now(); - const promises = configs.map((config, i) => { - const prompt = `${baseBrief}\n\nViewport: ${config.desc}. Adapt the layout for this screen size. ${ + const jobs: VariantJob[] = configs.map(config => ({ + outputPath: path.join(outputDir, `responsive-${config.suffix}.png`), + prompt: `${baseBrief}\n\nViewport: ${config.desc}. Adapt the layout for this screen size. ${ config.suffix === "mobile" ? "Use a single-column layout, larger touch targets, and mobile navigation patterns." : config.suffix === "tablet" ? "Use a responsive layout that works for medium screens." : "" - }`; - const outputPath = path.join(outputDir, `responsive-${config.suffix}.png`); - const delay = i * 1500; + }`, + size: config.size, + label: config.desc, + })); - return new Promise<{ path: string; success: boolean; error?: string }>(resolve => - setTimeout(resolve, delay) - ).then(() => { - console.error(` Starting ${config.desc}...`); - return generateVariant(apiKey, prompt, outputPath, config.size, quality); - }); - }); - - const results = await Promise.allSettled(promises); + const { acct, errors } = await runVariantJobs(apiKey, quality, jobs); const elapsed = ((Date.now() - startTime) / 1000).toFixed(1); - - const succeeded: string[] = []; - for (const result of results) { - if (result.status === "fulfilled" && result.value.success) { - const sz = fs.statSync(result.value.path).size; - console.error(` ✓ ${path.basename(result.value.path)} (${(sz / 1024).toFixed(0)}KB)`); - succeeded.push(result.value.path); - } else { - const error = result.status === "fulfilled" ? result.value.error : (result.reason as Error).message; - console.error(` ✗ ${error}`); - } - } - - console.error(`\n${succeeded.length}/${configs.length} responsive variants generated (${elapsed}s)`); - console.log(JSON.stringify({ - outputDir, - viewports: viewportList, - succeeded: succeeded.length, - paths: succeeded, - }, null, 2)); + console.error(`\n${acct.saved.length}/${configs.length} responsive variants generated (${elapsed}s)`); + return emitVariantsResult(outputDir, acct, errors, { viewports: viewportList }); } diff --git a/design/test/approval.test.ts b/design/test/approval.test.ts new file mode 100644 index 000000000..7a0e877e8 --- /dev/null +++ b/design/test/approval.test.ts @@ -0,0 +1,61 @@ +/** + * approved.json readers: approved_path (relative to approved.json) wins, legacy + * letter-only records still resolve, and a missing approved image is an error, + * never a substitute. + */ + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { spawnSync } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { resolveApprovedImage } from "../src/approval"; + +const BIN = path.join(import.meta.dir, "..", "..", "bin", "gstack-design-approved"); +let dir: string; +beforeEach(() => { dir = fs.mkdtempSync(path.join(os.tmpdir(), "design-approval-")); }); +afterEach(() => { fs.rmSync(dir, { recursive: true, force: true }); }); + +function record(sessionDir: string, data: object): string { + fs.mkdirSync(sessionDir, { recursive: true }); + const file = path.join(sessionDir, "approved.json"); + fs.writeFileSync(file, JSON.stringify(data)); + return file; +} + +describe("resolveApprovedImage", () => { + // Value: protects=pre-upgrade letter-only records keep resolving to variant-.png; fails_when=the legacy fallback is removed; + // why_new=approved_path is new and legacy records exist on disk; seam=none + test("legacy letter-only record resolves by letter", () => { + const file = record(dir, { approved_variant: "B" }); + fs.writeFileSync(path.join(dir, "variant-B.png"), "b"); + expect(resolveApprovedImage(file)).toEqual({ ok: true, path: path.join(dir, "variant-B.png"), legacy: true }); + }); + + // Value: protects=a present-but-missing approved_path errors instead of resolving variant-A.png; fails_when=readers fall back to the letter; + // why_new=eng approval-fallback precedence; seam=none + test("missing approved_path errors and never substitutes the letter's image", () => { + const file = record(dir, { approved_variant: "A", approved_path: "variant-A-2.png" }); + fs.writeFileSync(path.join(dir, "variant-A.png"), "round one"); + const r = resolveApprovedImage(file); + expect(r).toEqual({ ok: false, error: `approved image ${path.join(dir, "variant-A-2.png")} is missing; reselect from the board` }); + const cli = spawnSync(process.execPath, [BIN, file], { encoding: "utf8", timeout: 30_000 }); + expect(cli.status).toBe(1); + expect(cli.stdout).toBe(""); + expect(cli.stderr).toContain("reselect from the board"); + }); + + // Value: protects=approved_path is relative, so a copied or relocated designs directory still resolves; fails_when=an absolute path is stored or resolved against cwd; + // why_new=DX relocation requirement; seam=none + test("relocated designs directory still resolves the approved image", () => { + const original = path.join(dir, "state", "designs", "home-20261003"); + record(original, { approved_variant: "A", approved_path: "variant-A-2.png" }); + fs.writeFileSync(path.join(original, "variant-A-2.png"), "round two"); + const moved = path.join(dir, "elsewhere", "home-20261003"); + fs.cpSync(original, moved, { recursive: true }); + fs.rmSync(path.join(dir, "state"), { recursive: true }); + const cli = spawnSync(process.execPath, [BIN, path.join(moved, "approved.json")], { encoding: "utf8", timeout: 30_000, cwd: os.tmpdir() }); + expect(cli.status, cli.stderr).toBe(0); + expect(cli.stdout.trim()).toBe(path.join(moved, "variant-A-2.png")); + }); +}); diff --git a/design/test/cli-persistence.test.ts b/design/test/cli-persistence.test.ts new file mode 100644 index 000000000..fc4589c51 --- /dev/null +++ b/design/test/cli-persistence.test.ts @@ -0,0 +1,223 @@ +/** + * $D CLI persistence contract with the image API stubbed (no spend): paid + * images are never overwritten, every saved image is reported, a local save + * failure never buys a second image, and JSON is printed on every exit. + */ + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { spawnSync } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const CLI = path.join(import.meta.dir, "..", "src", "cli.ts"); +const RUN_FIELDS = ["requested", "saved", "selected", "failures", "recovered"]; + +let dir: string; +let calls: string; +let preload: string; +const sessions: string[] = []; + +beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "design-cli-persist-")); + calls = path.join(dir, "calls.log"); + preload = path.join(dir, "stub.ts"); + fs.writeFileSync(preload, `import { appendFileSync, existsSync, readFileSync } from "node:fs"; +const seq = (name: string) => (process.env[name] || "").split(",").filter(Boolean); +globalThis.fetch = (async (url: unknown) => { + const pathname = new URL(String(url)).pathname; + const log = process.env.CALLS!; + const prior = existsSync(log) ? readFileSync(log, "utf8").split("\\n").filter(l => l === pathname).length : 0; + appendFileSync(log, pathname + "\\n"); + if (pathname === "/v1/responses") { + const step = seq("IMAGES")[prior] ?? "ok"; + if (step === "fail") return new Response("upstream unavailable", { status: 503 }); + const result = step === "empty" ? "====" : Buffer.from("image " + prior).toString("base64"); + return Response.json({ id: "resp-" + prior, output: [{ type: "image_generation_call", result }] }); + } + const step = seq("CHECKS")[prior] ?? "PASS"; + if (step === "throw") throw Object.assign(new Error("The operation was aborted."), { name: "AbortError" }); + return Response.json({ choices: [{ message: { content: step === "PASS" ? "PASS" : "FAIL: illegible title" } }] }); +}) as typeof fetch; +`); +}); + +afterEach(() => { + for (const s of sessions.splice(0)) fs.rmSync(s, { force: true }); + fs.rmSync(dir, { recursive: true, force: true }); +}); + +function run(args: string[], env: Record = {}) { + const r = spawnSync(process.execPath, ["--no-env-file", "--preload", preload, CLI, ...args], { + encoding: "utf8", + timeout: 60_000, + env: { + PATH: process.env.PATH!, HOME: dir, GSTACK_HOME: path.join(dir, "state"), TMPDIR: dir, + OPENAI_API_KEY: "fixture-not-a-real-key", CALLS: calls, ...env, + }, + }); + const json = JSON.parse(r.stdout); + for (const a of json.attempts ?? []) if (a.sessionFile) sessions.push(a.sessionFile); + return { status: r.status, stderr: r.stderr, json }; +} + +const apiCalls = () => fs.existsSync(calls) ? fs.readFileSync(calls, "utf8").split("\n").filter(l => l === "/v1/responses").length : 0; + +describe("never overwrite", () => { + // Value: protects=a second generate into the same --output keeps the first image and reports the bumped path with the exact notice; + // fails_when=generate writes with writeFileSync again or reports the requested path; why_new=#1529 had no CLI coverage; seam=none + test("generate bumps, prints the notice and leaves the original bytes", () => { + const out = path.join(dir, "mock.png"); + const first = run(["generate", "--brief", "dashboard", "--output", out]); + expect(first.status, first.stderr).toBe(0); + expect(first.json.outputPath).toBe(out); + const second = run(["generate", "--brief", "dashboard", "--output", out]); + const bumped = path.join(dir, "mock-2.png"); + expect(second.status, second.stderr).toBe(0); + expect(second.json.outputPath).toBe(bumped); + expect(second.json.saved).toEqual([bumped]); + expect(second.stderr).toContain(`note: ${out} exists; saved to ${bumped} (existing file kept)`); + expect(fs.readFileSync(out, "utf8")).toBe("image 0"); + expect(fs.readFileSync(bumped, "utf8")).toBe("image 1"); + for (const key of [...RUN_FIELDS, "outputPath", "attempts"]) expect(second.json).toHaveProperty(key); + }); + + // Value: protects=a second variants round keeps round one, reports variant-X-2 paths, and both branches print one key set; + // fails_when=variants overwrites or the --viewports branch drops count/errors/failures; why_new=#1529 overwrite path; seam=none + test("variants (both branches) bump per file with one shared key set", () => { + const outDir = path.join(dir, "designs"); + const r1 = run(["variants", "--brief", "dashboard", "--count", "2", "--output-dir", outDir]); + expect(r1.json.paths).toEqual([path.join(outDir, "variant-A.png"), path.join(outDir, "variant-B.png")]); + const r2 = run(["variants", "--brief", "dashboard", "--count", "2", "--output-dir", outDir]); + expect(r2.status, r2.stderr).toBe(0); + expect(r2.json.paths).toEqual([path.join(outDir, "variant-A-2.png"), path.join(outDir, "variant-B-2.png")]); + expect(r2.stderr).toContain(`note: ${path.join(outDir, "variant-A.png")} exists; saved to ${path.join(outDir, "variant-A-2.png")} (existing file kept)`); + expect(fs.readFileSync(path.join(outDir, "variant-A.png"), "utf8")).toBe("image 0"); + run(["variants", "--brief", "dashboard", "--viewports", "desktop", "--output-dir", outDir]); + const again = run(["variants", "--brief", "dashboard", "--viewports", "desktop", "--output-dir", outDir]); + expect(again.json.paths).toEqual([path.join(outDir, "responsive-desktop-2.png")]); + expect(fs.readFileSync(path.join(outDir, "responsive-desktop.png"), "utf8")).toBe("image 4"); + const keys = (o: object) => Object.keys(o).filter(k => k !== "viewports").sort(); + expect(keys(again.json)).toEqual(keys(r2.json)); + expect(again.json).toMatchObject({ count: 1, succeeded: 1, failed: 0, errors: [], failures: [], viewports: ["desktop"] }); + for (const key of [...RUN_FIELDS, "outputDir", "count", "succeeded", "failed", "paths", "errors"]) expect(r2.json).toHaveProperty(key); + }); +}); + +describe("accounting and exit codes", () => { + // Value: protects=each --retry attempt is its own saved, counted image; fails_when=retries overwrite one --output or attempts drop rejected images; + // why_new=the retry premise was wrong in #1737; seam=none + test("generate --check --retry keeps the rejected and the passing image", () => { + const out = path.join(dir, "retry.png"); + const r = run(["generate", "--brief", "dashboard", "--output", out, "--check", "--retry", "1"], { CHECKS: "FAIL,PASS" }); + expect(r.status, r.stderr).toBe(0); + const second = path.join(dir, "retry-2.png"); + expect(r.json.saved).toEqual([out, second]); + expect(r.json.attempts.map((a: any) => [a.path, a.check.pass])).toEqual([[out, false], [second, true]]); + expect(r.json.outputPath).toBe(second); + expect(fs.existsSync(out) && fs.existsSync(second)).toBe(true); + }); + + // Value: protects=an interrupted retry exits 3 with JSON listing the earlier saved attempt; fails_when=the error escapes as exit 1 without JSON; + // why_new=new exit contract; seam=none + test("interrupted retry exits 3 and still prints every saved path", () => { + const out = path.join(dir, "retry.png"); + const r = run(["generate", "--brief", "dashboard", "--output", out, "--check", "--retry", "2"], { CHECKS: "FAIL", IMAGES: "ok,fail" }); + expect(r.status).toBe(3); + expect(r.json).toMatchObject({ outputPath: null, selected: null, saved: [out], requested: 1 }); + expect(r.json.failures).toHaveLength(1); + expect(r.json.failures[0].reason).toContain("503"); + }); + + // Value: protects=a quality-check timeout after a save still prints partial JSON and exits 3; fails_when=a check exception loses the saved path; + // why_new=DX contract case; seam=none + test("quality-check timeout after a save exits 3 with partial JSON", () => { + const out = path.join(dir, "checked.png"); + const r = run(["generate", "--brief", "dashboard", "--output", out, "--check"], { CHECKS: "throw" }); + expect(r.status).toBe(3); + expect(r.json.saved).toEqual([out]); + expect(r.json.failures[0].reason).toContain("aborted"); + }); + + // Value: protects=zero saved images exit 2 with JSON naming every failure; fails_when=all-fail exits 0 or prints nothing; + // why_new=skills route on this code; seam=none + test("variants with zero successes exits 2 with JSON", () => { + const outDir = path.join(dir, "designs"); + const r = run(["variants", "--brief", "dashboard", "--count", "2", "--output-dir", outDir], { IMAGES: "fail,fail" }); + expect(r.status).toBe(2); + expect(r.json).toMatchObject({ requested: 2, saved: [], paths: [], errors: ["variant-A.png", "variant-B.png"] }); + expect(r.json.failures.map((f: any) => path.basename(f.file))).toEqual(["variant-A.png", "variant-B.png"]); + }); + + // Value: protects=a partial batch exits 0 and names the failed basename; fails_when=one failure fails the whole round; + // why_new=partial accounting is new; seam=none + test("partial variants batch exits 0", () => { + const outDir = path.join(dir, "designs"); + const r = run(["variants", "--brief", "dashboard", "--count", "2", "--output-dir", outDir], { IMAGES: "ok,fail" }); + expect(r.status).toBe(0); + expect(r.json).toMatchObject({ succeeded: 1, failed: 1, paths: [path.join(outDir, "variant-A.png")], errors: ["variant-B.png"] }); + }); + + // Value: protects=an empty image from the API is a failure, never a 0-byte PNG; fails_when=persistImage writes empty bytes; + // why_new=CEO empty-b64 warning; seam=none + test("empty image bytes exit 2 with no file", () => { + const out = path.join(dir, "empty.png"); + const r = run(["generate", "--brief", "dashboard", "--output", out], { IMAGES: "empty" }); + expect(r.status).toBe(2); + expect(fs.existsSync(out)).toBe(false); + expect(r.json.failures[0].reason).toContain("empty image"); + }); + + // Value: protects=a missing API key still prints the run JSON before exiting non-zero; fails_when=the key check exits the process directly; + // why_new=JSON on every exit path; seam=none + test("missing API key prints JSON and exits 2", () => { + const r = run(["evolve", "--screenshot", path.join(dir, "none.png"), "--brief", "calmer", "--output", path.join(dir, "e.png")], { OPENAI_API_KEY: "" }); + expect(r.status).toBe(2); + expect(r.json.failures[0].reason).toContain("No OpenAI API key found"); + for (const key of [...RUN_FIELDS, "outputPath", "attempts"]) expect(r.json).toHaveProperty(key); + }); +}); + +describe("no re-purchase on a local save failure", () => { + // Value: protects=a write failure after a paid response makes exactly one API call per variant and keeps a recovery copy; + // fails_when=the save sits inside the 429 retry loop again; why_new=Codex DX finding 1; seam=none + test("variants (both branches): one API call, recovery copy, exit 2", () => { + const blocker = path.join(dir, "blocker"); + fs.writeFileSync(blocker, "a file, not a directory"); + for (const extra of [["--count", "1"], ["--viewports", "desktop"]]) { + const before = apiCalls(); + const r = run(["variants", "--brief", "dashboard", ...extra, "--output-dir", path.join(blocker, "designs")]); + expect(r.status).toBe(2); + expect(apiCalls() - before).toBe(1); + expect(r.stderr).not.toContain("Rate limited"); + expect(r.json.recovered).toHaveLength(1); + expect(fs.existsSync(r.json.recovered[0].path)).toBe(true); + expect(r.json.failures[0].reason).toContain("cannot save paid image to"); + } + }); + + // Value: protects=iterate makes one claim per run, and a save failure never triggers the fallback purchase; + // fails_when=the write moves back inside the threaded try; why_new=eng loop restructure; seam=none + test("iterate: threaded failure claims once; save failure buys nothing more", () => { + const gen = run(["generate", "--brief", "dashboard", "--output", path.join(dir, "base.png")]); + const session = gen.json.sessionFile; + const out = path.join(dir, "refined.png"); + + const before = apiCalls(); + const fallback = run(["iterate", "--session", session, "--feedback", "bigger", "--output", out], { IMAGES: "ok,fail,ok" }); + expect(fallback.status, fallback.stderr).toBe(0); + expect(apiCalls() - before).toBe(2); + expect(fallback.json.saved).toEqual([out]); + expect(fs.readdirSync(dir).filter(f => f.startsWith("refined"))).toEqual(["refined.png"]); + + const blocker = path.join(dir, "blocker"); + fs.writeFileSync(blocker, "file"); + const mid = apiCalls(); + const failed = run(["iterate", "--session", session, "--feedback", "bigger", "--output", path.join(blocker, "x.png")]); + expect(failed.status).toBe(2); + expect(apiCalls() - mid).toBe(1); + expect(failed.stderr).not.toContain("Falling back"); + expect(failed.json.recovered).toHaveLength(1); + for (const key of [...RUN_FIELDS, "outputPath", "attempts", "sessionFile", "iteration"]) expect(failed.json).toHaveProperty(key); + }); +}); diff --git a/design/test/gallery.test.ts b/design/test/gallery.test.ts index 7eaebc618..d31230955 100644 --- a/design/test/gallery.test.ts +++ b/design/test/gallery.test.ts @@ -3,7 +3,7 @@ */ import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; -import { generateGalleryHtml } from '../src/gallery'; +import { generateGalleryHtml, listVariantImages } from '../src/gallery'; import * as fs from 'fs'; import * as path from 'path'; @@ -137,3 +137,37 @@ describe('Gallery generation', () => { expect(html).toContain('data:image/png;base64,'); }); }); + +describe('Gallery with never-overwrite rounds', () => { + const setMtime = (file: string, seconds: number) => fs.utimesSync(file, seconds, seconds); + + // Value: protects=bumped names (variant-A-2.png) are listed and approved_path marks the exact image, not the letter; + // fails_when=the regex drops -N suffixes or marking falls back to the letter when approved_path exists; why_new=#1529 gallery gap; seam=none + test('lists bumped variants and marks the one approved_path names', () => { + const session = path.join(tmpDir, 'bumped', 'home-20261003'); + fs.mkdirSync(session, { recursive: true }); + createTestPng(path.join(session, 'variant-A.png')); + createTestPng(path.join(session, 'variant-A-2.png')); + fs.writeFileSync(path.join(session, 'approved.json'), JSON.stringify({ approved_variant: 'A', approved_path: 'variant-A-2.png' })); + expect(listVariantImages(session).map(f => path.basename(f)).sort()).toEqual(['variant-A-2.png', 'variant-A.png']); + const html = generateGalleryHtml(path.join(tmpDir, 'bumped')); + expect(html.match(/class="gallery-variant approved"/g)).toHaveLength(1); + expect(html).toMatch(/class="gallery-variant approved">\s*]+alt="Variant A-2"/); + expect(html).toContain('letters do not identify rounds'); + }); + + // Value: protects=history order is mtime then name, and the current board follows in board-images.json order even when files finished out of order; + // fails_when=the gallery sorts by name only or ignores board-images.json; why_new=eng gallery semantics; seam=none + test('orders history by mtime then name, then the board in board-images.json order', () => { + const session = path.join(tmpDir, 'ordered'); + fs.mkdirSync(session, { recursive: true }); + for (const f of ['variant-B.png', 'variant-A.png']) { createTestPng(path.join(session, f)); setMtime(path.join(session, f), 1_000); } + const round2 = ['variant-A-2.png', 'variant-B-2.png', 'variant-C-2.png']; + [3_000, 2_500, 2_000].forEach((t, i) => { createTestPng(path.join(session, round2[i])); setMtime(path.join(session, round2[i]), t); }); + expect(listVariantImages(session).map(f => path.basename(f))).toEqual( + ['variant-A.png', 'variant-B.png', 'variant-C-2.png', 'variant-B-2.png', 'variant-A-2.png']); + fs.writeFileSync(path.join(session, 'board-images.json'), JSON.stringify(round2.map(f => path.join(session, f)))); + expect(listVariantImages(session).map(f => path.basename(f))).toEqual( + ['variant-A.png', 'variant-B.png', 'variant-A-2.png', 'variant-B-2.png', 'variant-C-2.png']); + }); +}); diff --git a/design/test/image-args.test.ts b/design/test/image-args.test.ts new file mode 100644 index 000000000..8be15133b --- /dev/null +++ b/design/test/image-args.test.ts @@ -0,0 +1,52 @@ +/** + * `$D compare` image list input: --images-file and JSON arrays carry any path + * losslessly; comma and glob input keep working. + */ + +import { afterEach, beforeEach, expect, test } from "bun:test"; +import { spawnSync } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { resolveImagePaths } from "../src/image-args"; + +let dir: string; +beforeEach(() => { dir = fs.mkdtempSync(path.join(os.tmpdir(), "design-image-args-")); }); +afterEach(() => { fs.rmSync(dir, { recursive: true, force: true }); }); + +// Value: protects=a path containing a comma and a space reaches compare intact via --images-file; fails_when=the file is split on commas; +// why_new=Lossless path handoff is new; seam=none +test("--images-file keeps a comma+space path and builds the board", () => { + const odd = path.join(dir, "round 2, final"); + fs.mkdirSync(odd); + const images = [path.join(odd, "variant-A-2.png"), path.join(odd, "variant-B.png")]; + for (const img of images) fs.writeFileSync(img, Buffer.from("iVBORw0KGgo=", "base64")); + const list = path.join(odd, "board-images.json"); + fs.writeFileSync(list, JSON.stringify(images)); + const board = path.join(odd, "board.html"); + const r = spawnSync(process.execPath, ["--no-env-file", path.join(import.meta.dir, "..", "src", "cli.ts"), + "compare", "--images-file", list, "--output", board], { encoding: "utf8", timeout: 30_000, env: { PATH: process.env.PATH!, HOME: dir } }); + expect(r.status, r.stderr).toBe(0); + const html = fs.readFileSync(board, "utf8"); + expect(html).toContain('data-variant="A"'); + expect(html).toContain('data-variant="B"'); + expect(html).not.toContain('data-variant="C"'); +}); + +// Value: protects=JSON-array --images input and the unchanged comma form; fails_when=a `[`-prefixed literal path is misparsed or comma input changes; +// why_new=new input form; seam=none +test("--images: JSON array only when it parses; otherwise comma and literal behavior is unchanged", async () => { + expect(await resolveImagePaths('["/a, b/x.png","/c.png"]')).toEqual(["/a, b/x.png", "/c.png"]); + expect(await resolveImagePaths("[draft]/x.png")).toEqual(["[draft]/x.png"]); + expect(await resolveImagePaths("/a.png, /b.png")).toEqual(["/a.png", "/b.png"]); + expect(await resolveImagePaths("[1,2]")).toEqual(["[1", "2]"]); +}); + +// Value: protects=a malformed --images-file fails loudly instead of building an empty board; fails_when=parse errors are swallowed; +// why_new=new flag; seam=none +test("--images-file must hold a JSON array of strings", async () => { + const bad = path.join(dir, "bad.json"); + fs.writeFileSync(bad, '{"paths": []}'); + await expect(resolveImagePaths(undefined, bad)).rejects.toThrow("JSON array"); + await expect(resolveImagePaths(undefined, path.join(dir, "none.json"))).rejects.toThrow("cannot read --images-file"); +}); diff --git a/design/test/models.test.ts b/design/test/models.test.ts index a057e632f..404eb9435 100644 --- a/design/test/models.test.ts +++ b/design/test/models.test.ts @@ -139,15 +139,19 @@ describe("image-generation call sites (Responses API)", () => { test("generate", async () => { stubFetch((u) => ok(u)); const r = await generate({ brief: "a login page", output: path.join(dir, "g.png") }); - fs.rmSync(r.sessionFile, { force: true }); + fs.rmSync(r.sessionFile!, { force: true }); expect(calls).toHaveLength(1); expectImageShape(calls[0]); stubFetch(() => rejected()); - await expect(generate({ brief: "x", output: path.join(dir, "g2.png") })).rejects.toThrow(`${DESIGN_MODEL_ENV}=${OVERRIDE}`); + const failed = await generate({ brief: "x", output: path.join(dir, "g2.png") }); + expect(failed.exitCode).toBe(2); + names(failed.failures[0].reason); // An override that cannot drive gpt-image-2 fails before any request. calls = []; process.env[DESIGN_MODEL_ENV] = "gpt-4o"; - await expect(generate({ brief: "x", output: path.join(dir, "g3.png") })).rejects.toThrow(DESIGN_MODEL_ENV); + const refused = await generate({ brief: "x", output: path.join(dir, "g3.png") }); + expect(refused.exitCode).toBe(2); + expect(refused.failures[0].reason).toContain(DESIGN_MODEL_ENV); expect(calls).toHaveLength(0); }); @@ -177,7 +181,8 @@ describe("image-generation call sites (Responses API)", () => { write(); errors = []; stubFetch(() => rejected()); - await expect(iterate({ session, feedback: "x", output: path.join(dir, "i2.png") })).rejects.toThrow(`${DESIGN_MODEL_ENV}=${OVERRIDE}`); + expect(await iterate({ session, feedback: "x", output: path.join(dir, "i2.png") })).toBe(2); + names(errors.at(-1)!); names(errors.find((e) => e.includes("Threading failed"))!); } finally { fs.rmSync(session, { force: true }); @@ -192,7 +197,8 @@ describe("image-generation call sites (Responses API)", () => { expectImageShape(calls[1]); expect(calls[1].body.input).toContain("a centered card"); stubFetch(() => rejected()); - await expect(evolve({ screenshot: path.join(dir, "shot.png"), brief: "x", output: path.join(dir, "e2.png") })).rejects.toThrow(`${DESIGN_MODEL_ENV}=${OVERRIDE}`); + expect(await evolve({ screenshot: path.join(dir, "shot.png"), brief: "x", output: path.join(dir, "e2.png") })).toBe(2); + names(errors.at(-1)!); names(errors.find((e) => e.includes("Screenshot analysis failed"))!); }); }); diff --git a/design/test/persist.test.ts b/design/test/persist.test.ts new file mode 100644 index 000000000..9cf2c6867 --- /dev/null +++ b/design/test/persist.test.ts @@ -0,0 +1,165 @@ +/** + * persistImage / claimOutputPath: a paid image is never overwritten and never lost. + */ + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import fs from "fs"; +import os from "os"; +import path from "path"; +import { CLAIM_CAP, bumpedName, claimOutputPath, persistImage, type PersistFs } from "../src/persist"; + +const PNG = Buffer.from("fake paid image bytes"); +const B64 = PNG.toString("base64"); + +let dir: string; +let tmp: string; +let stderr: string[]; +const savedError = console.error; + +beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "design-persist-")); + tmp = fs.mkdtempSync(path.join(os.tmpdir(), "design-persist-tmp-")); + stderr = []; + console.error = (...a: unknown[]) => { stderr.push(a.map(String).join(" ")); }; +}); + +afterEach(() => { + console.error = savedError; + fs.rmSync(dir, { recursive: true, force: true }); + fs.rmSync(tmp, { recursive: true, force: true }); +}); + +function failingFs(fail: { op: keyof PersistFs; code: string; when?: (p: unknown) => boolean }): PersistFs { + const wrapped: any = {}; + for (const op of ["mkdirSync", "openSync", "writeSync", "fsyncSync", "closeSync", "unlinkSync"] as const) { + wrapped[op] = (...args: any[]) => { + if (op === fail.op && (!fail.when || fail.when(args[0]))) { + throw Object.assign(new Error(`${fail.code}: simulated ${op} failure`), { code: fail.code }); + } + return (fs as any)[op](...args); + }; + } + return wrapped; +} + +describe("claimOutputPath", () => { + // Value: protects=an existing image is never truncated and the next free -N name is claimed; + // fails_when=the claim opens with "w" instead of "wx" or the bump rule changes; why_new=no claim existed before; seam=none + test("keeps existing files and bumps sequentially", () => { + const requested = path.join(dir, "x.png"); + fs.writeFileSync(requested, "round one"); + const second = claimOutputPath(requested); + fs.closeSync(second.fd); + const third = claimOutputPath(requested); + fs.closeSync(third.fd); + expect([second.path, third.path]).toEqual([path.join(dir, "x-2.png"), path.join(dir, "x-3.png")]); + expect(fs.readFileSync(requested, "utf8")).toBe("round one"); + expect(bumpedName(path.join(dir, "x-2.png"), 2)).toBe(path.join(dir, "x-2-2.png")); + }); + + // Value: protects=parallel writers (variants, shotgun subagents) each get their own file; + // fails_when=the claim is check-then-write instead of an exclusive create; why_new=concurrency was untested; seam=none + test("N concurrent processes claim N distinct files", async () => { + const requested = path.join(dir, "variant-A.png"); + const script = `import { claimOutputPath } from ${JSON.stringify(path.join(import.meta.dir, "../src/persist.ts"))}; +const c = claimOutputPath(process.argv[2]); console.log(c.path);`; + const scriptPath = path.join(tmp, "claim.ts"); + fs.writeFileSync(scriptPath, script); + const procs = Array.from({ length: 8 }, () => + Bun.spawn([process.execPath, scriptPath, requested], { stdout: "pipe", stderr: "pipe" })); + const outs = await Promise.all(procs.map(p => new Response(p.stdout).text())); + await Promise.all(procs.map(p => p.exited)); + const claimed = outs.map(o => o.trim()); + expect(new Set(claimed).size).toBe(8); + expect(fs.readdirSync(dir).length).toBe(8); + }); + + // Value: protects=a permission error fails at once instead of walking 999 names; fails_when=every open error is treated as EEXIST; + // why_new=new behavior; seam=PersistFs fault injection + test("non-EEXIST open errors do not bump", () => { + let opens = 0; + const fsImpl = failingFs({ op: "openSync", code: "EACCES", when: () => { opens++; return true; } }); + expect(() => claimOutputPath(path.join(dir, "x.png"), fsImpl)).toThrow("EACCES"); + expect(opens).toBe(1); + }); + + // Value: protects=the claim stops at -999 with an error; fails_when=the cap is removed (unbounded loop) or off by one; + // why_new=new behavior; seam=PersistFs fault injection + test("stops at the -999 cap", () => { + const fsImpl = failingFs({ op: "openSync", code: "EEXIST" }); + expect(() => claimOutputPath(path.join(dir, "x.png"), fsImpl)).toThrow(`x-${CLAIM_CAP}.png already exist`); + }); +}); + +describe("persistImage", () => { + // Value: protects=the exact bump notice and actual-path reporting scripts rely on; fails_when=the notice text or returned path drifts; + // why_new=bumping is new; seam=none + test("bumps a taken name, prints the notice and leaves the old bytes untouched", () => { + const requested = path.join(dir, "nested", "x.png"); + fs.mkdirSync(path.dirname(requested)); + fs.writeFileSync(requested, "old"); + const outcome = persistImage(B64, requested, { tmpdir: tmp }); + const actual = path.join(dir, "nested", "x-2.png"); + expect(outcome).toEqual({ ok: true, path: actual, bytes: PNG.length }); + expect(stderr).toContain(`note: ${requested} exists; saved to ${actual} (existing file kept)`); + expect(fs.readFileSync(requested, "utf8")).toBe("old"); + expect(fs.readFileSync(actual)).toEqual(PNG); + }); + + // Value: protects=an empty API image is a failure, not a 0-byte PNG; fails_when=the empty-bytes guard is removed; + // why_new=closes the CEO empty-b64 warning; seam=none + test("empty image data is a failure with no claim and no file", () => { + const outcome = persistImage("", path.join(dir, "x.png"), { tmpdir: tmp }); + expect(outcome.ok).toBe(false); + expect(fs.readdirSync(dir)).toEqual([]); + expect(fs.readdirSync(tmp)).toEqual([]); + }); + + // Value: protects=a mid-write ENOSPC removes the partial claimed file and keeps a 0600 recovery copy; fails_when=cleanup or recovery is skipped; + // why_new=new behavior; seam=PersistFs fault injection + test("post-claim write failure removes the claimed file and saves a private recovery copy", () => { + const requested = path.join(dir, "x.png"); + let writes = 0; + const fsImpl = failingFs({ op: "writeSync", code: "ENOSPC", when: () => writes++ === 0 }); + const outcome = persistImage(B64, requested, { tmpdir: tmp, fs: fsImpl }); + expect(outcome.ok).toBe(false); + if (outcome.ok) return; + expect(fs.existsSync(requested)).toBe(false); + const recovery = outcome.recovered!.path; + expect(path.dirname(recovery)).toBe(tmp); + expect(path.basename(recovery)).toMatch(/^gstack-design-unsaved-.*\.png$/); + expect(fs.readFileSync(recovery)).toEqual(PNG); + expect(fs.statSync(recovery).mode & 0o777).toBe(0o600); + expect(outcome.failure.file).toBe(requested); + expect(outcome.failure.reason).toContain(`cannot save paid image to ${requested}: ENOSPC`); + expect(outcome.failure.reason).toContain(`saved a recovery copy to ${recovery}`); + expect(outcome.failure.reason).toContain("Fix: free disk space"); + }); + + // Value: protects=a mkdir/claim failure (EACCES) still recovers the bytes with problem, cause and fix; fails_when=recovery only covers write errors; + // why_new=new behavior; seam=PersistFs fault injection + test("EACCES on the claim recovers the bytes and names the fix", () => { + const requested = path.join(dir, "x.png"); + const fsImpl = failingFs({ op: "openSync", code: "EACCES", when: p => p === requested }); + const outcome = persistImage(B64, requested, { tmpdir: tmp, fs: fsImpl }); + expect(outcome.ok).toBe(false); + if (outcome.ok) return; + expect(outcome.failure.reason).toMatch(/cannot save paid image to .*x\.png: EACCES \(.*\)\. Image bytes were received; saved a recovery copy to .*\. Fix: make .* writable/); + expect(fs.readFileSync(outcome.recovered!.path)).toEqual(PNG); + }); + + // Value: protects=when the recovery copy also fails both errors and the byte count are reported and no recovery is claimed; + // fails_when=the message claims a recovery copy that does not exist; why_new=new behavior; seam=PersistFs fault injection + test("recovery failure reports both errors and the byte count", () => { + const requested = path.join(dir, "x.png"); + const fsImpl = failingFs({ op: "openSync", code: "EROFS" }); + const outcome = persistImage(B64, requested, { tmpdir: tmp, fs: fsImpl }); + expect(outcome.ok).toBe(false); + if (outcome.ok) return; + expect(outcome.recovered).toBeUndefined(); + expect(outcome.failure.reason).toContain(`(${PNG.length} bytes)`); + expect(outcome.failure.reason).toContain("recovery copy"); + expect(outcome.failure.reason.match(/EROFS/g)!.length).toBeGreaterThanOrEqual(2); + expect(outcome.failure.reason).not.toContain("saved a recovery copy"); + }); +}); diff --git a/test/gstack-design-claim.test.ts b/test/gstack-design-claim.test.ts new file mode 100644 index 000000000..2c2cd3328 --- /dev/null +++ b/test/gstack-design-claim.test.ts @@ -0,0 +1,118 @@ +/** + * bin/gstack-design-claim publishes a staged paid image without overwriting. + * One table drives both claim implementations (the sh helper and $D's + * claimOutputPath) so their suffix and cap rules cannot drift apart. + */ + +import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; +import { spawnSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { claimOutputPath } from '../design/src/persist'; + +const BIN = path.resolve(import.meta.dir, '..', 'bin', 'gstack-design-claim'); + +let dir: string; +beforeEach(() => { dir = fs.mkdtempSync(path.join(os.tmpdir(), 'design-claim-')); }); +afterEach(() => { fs.rmSync(dir, { recursive: true, force: true }); }); + +function stage(content = 'paid bytes'): string { + const staged = path.join(fs.mkdtempSync(path.join(dir, 'stage-')), 'variant.png'); + fs.writeFileSync(staged, content); + return staged; +} + +function claimSh(src: string, dest: string) { + return spawnSync('sh', [BIN, src, dest], { encoding: 'utf8', timeout: 30_000 }); +} + +const TABLE: Array<{ name: string; existing: string[]; requested: string; expected: string }> = [ + { name: 'free name', existing: [], requested: 'variant-A.png', expected: 'variant-A.png' }, + { name: 'first bump', existing: ['variant-A.png'], requested: 'variant-A.png', expected: 'variant-A-2.png' }, + { name: 'gap is not reused out of order', existing: ['variant-A.png', 'variant-A-2.png'], requested: 'variant-A.png', expected: 'variant-A-3.png' }, + { name: 'suffix appends to the requested stem', existing: ['x-2.png'], requested: 'x-2.png', expected: 'x-2-2.png' }, + { name: 'only the last extension splits', existing: ['shot.v1.png'], requested: 'shot.v1.png', expected: 'shot.v1-2.png' }, + { name: 'no extension', existing: ['mock'], requested: 'mock', expected: 'mock-2' }, +]; + +describe('claim rule table (sh helper and claimOutputPath agree)', () => { + for (const row of TABLE) { + // Value: protects=the sh helper and $D bump to the same -N name; fails_when=either suffix rule changes alone; + // why_new=the two implementations are new and must stay in lockstep; seam=none + test(row.name, () => { + for (const impl of ['sh', 'ts'] as const) { + const target = fs.mkdtempSync(path.join(dir, `${impl}-`)); + for (const f of row.existing) fs.writeFileSync(path.join(target, f), `existing ${f}`); + let actual: string; + if (impl === 'sh') { + const r = claimSh(stage(), path.join(target, row.requested)); + expect(r.status, r.stderr).toBe(0); + actual = r.stdout.trim(); + } else { + const c = claimOutputPath(path.join(target, row.requested)); + fs.closeSync(c.fd); + actual = c.path; + } + expect({ impl, actual }).toEqual({ impl, actual: path.join(target, row.expected) }); + for (const f of row.existing) expect(fs.readFileSync(path.join(target, f), 'utf8')).toBe(`existing ${f}`); + } + }); + } +}); + +describe('gstack-design-claim', () => { + // Value: protects=publishing copies the staged bytes, prints the final path and never clobbers; fails_when=the helper uses plain cp/mv or > without noclobber; + // why_new=new helper replacing shotgun's cp; seam=none + test('publishes bytes without clobbering and removes the staged copy', () => { + const dest = path.join(dir, 'designs', 'variant-B.png'); + fs.mkdirSync(path.dirname(dest)); + fs.writeFileSync(dest, 'round one'); + const staged = stage('round two'); + const r = claimSh(staged, dest); + expect(r.status, r.stderr).toBe(0); + const final = path.join(dir, 'designs', 'variant-B-2.png'); + expect(r.stdout).toBe(`${final}\n`); + expect(fs.readFileSync(dest, 'utf8')).toBe('round one'); + expect(fs.readFileSync(final, 'utf8')).toBe('round two'); + expect(fs.existsSync(staged)).toBe(false); + }); + + // Value: protects=the -999 cap stops with problem/cause/fix and keeps the staged image; fails_when=the cap is removed or the staged file is deleted on failure; + // why_new=new helper; seam=none + test('stops at the -999 cap and keeps the staged image', () => { + const target = path.join(dir, 'full'); + fs.mkdirSync(target); + fs.writeFileSync(path.join(target, 'x.png'), '1'); + for (let n = 2; n <= 999; n++) fs.writeFileSync(path.join(target, `x-${n}.png`), String(n)); + const staged = stage(); + const r = claimSh(staged, path.join(target, 'x.png')); + expect(r.status).not.toBe(0); + expect(r.stderr).toContain('already exist'); + expect(r.stderr).toMatch(/cause: .+\nfix: .+/); + expect(fs.existsSync(staged)).toBe(true); + expect(fs.readdirSync(target).length).toBe(999); + }); + + // Value: protects=a non-collision error (destination parent is a file) fails at once, explains itself and keeps the staged bytes; + // fails_when=errors are treated as collisions or the staged file is removed; why_new=new helper; seam=none + test('other errors exit non-zero with problem, cause and fix, keeping the staged image', () => { + const blocker = path.join(dir, 'not-a-dir'); + fs.writeFileSync(blocker, 'file'); + const staged = stage(); + const r = claimSh(staged, path.join(blocker, 'variant-A.png')); + expect(r.status).not.toBe(0); + expect(r.stderr).toMatch(/^gstack-design-claim: cannot create .*staged image kept at .*\ncause: .+\nfix: .+/); + expect(fs.readFileSync(staged, 'utf8')).toBe('paid bytes'); + }); + + // Value: protects=an empty or missing staged file is refused instead of published; fails_when=the -s guard is removed; + // why_new=new helper; seam=none + test('refuses a missing or empty staged image', () => { + const empty = stage(''); + const r = claimSh(empty, path.join(dir, 'variant-A.png')); + expect(r.status).not.toBe(0); + expect(r.stderr).toContain('missing or empty'); + expect(fs.existsSync(path.join(dir, 'variant-A.png'))).toBe(false); + }); +}); From f7c1cbeedf9b61630d477c2145f1997b8debfa50 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 3 Oct 2026 20:26:41 +0000 Subject: [PATCH 2/2] fix(design skills): read the paths $D printed, never fixed variant names (#1529) Every design flow now captures each generating $D command's JSON and exit code without set -e, does round accounting before the first board, check or inline view (stopping when nothing was saved), and carries the printed paths forward. Boards are built from board-images.json via compare --images-file after archiving any earlier feedback.json, and approvals map the confirmed letter through that file and record approved_path. design-shotgun subagents generate into a per-run mktemp staging directory and publish with bin/gstack-design-claim instead of a shared /tmp/variant-X.png plus cp. design-html and taste memory resolve approvals with gstack-design-approved; design-review verifies against the printed target path. test/design-printed-paths.test.ts bans fixed variant names, variant globs, variant-{letter} reads, cp/mv of $D output and literal check paths in the generated skills and resolvers (one content-anchored allowlist entry: the design-html discovery probe), and pins the accounting/board marker order. test/design-approval-flow.test.ts runs the generated board and approval blocks through a stubbed two-round flow. --- design-consultation/SKILL.md | 10 +- .../sections/proposal-and-preview.md | 28 +-- .../sections/proposal-and-preview.md.tmpl | 14 +- design-html/SKILL.md | 21 ++- design-html/SKILL.md.tmpl | 13 +- design-review/SKILL.md | 15 +- design-review/SKILL.md.tmpl | 7 +- design-shotgun/SKILL.md | 111 ++++++------ design-shotgun/SKILL.md.tmpl | 80 ++++----- office-hours/SKILL.md | 30 +++- plan-design-review/SKILL.md | 46 +++-- plan-design-review/SKILL.md.tmpl | 17 +- .../sections/review-sections.md | 4 +- .../sections/review-sections.md.tmpl | 4 +- scripts/resolvers/design.ts | 87 +++++---- test/design-approval-flow.test.ts | 97 ++++++++++ test/design-printed-paths.test.ts | 165 ++++++++++++++++++ 17 files changed, 561 insertions(+), 188 deletions(-) create mode 100644 test/design-approval-flow.test.ts create mode 100644 test/design-printed-paths.test.ts diff --git a/design-consultation/SKILL.md b/design-consultation/SKILL.md index 6922787e6..ea3d6829f 100644 --- a/design-consultation/SKILL.md +++ b/design-consultation/SKILL.md @@ -550,12 +550,14 @@ For interactive feedback, use `compare --serve` and its printed HTTP URL; openin If `DESIGN_READY`: the design binary is available for visual mockup generation. Commands: -- `$D generate --brief "..." --output /path.png` — generate a single mockup -- `$D variants --brief "..." --count 3 --output-dir /path/` — generate N style variants -- `$D compare --images "a.png,b.png,c.png" --output /path/board.html --serve` — comparison board + HTTP server +- `$D generate --brief "..." --output /path.png` — generate a single mockup (prints `outputPath`) +- `$D variants --brief "..." --count 3 --output-dir /path/` — generate N style variants (prints `paths`) +- `$D compare --images-file /path/board-images.json --output /path/board.html --serve` — comparison board + HTTP server - `$D serve --html /path/board.html` — serve comparison board and collect feedback via HTTP - `$D check --image /path.png --brief "..."` — vision quality gate - `$D iterate --session /path/session.json --feedback "..." --output /path.png` — iterate + +Image commands never overwrite (a taken name gets `-2`) and always print JSON (`requested`, `saved`, `failures`); exit 0 ready, 2 nothing saved, 3 stopped after saving some. Capture without `set -e`: `_OUT=$($D ...); _RC=$?`. - `$D extract --image /absolute/path.png` — print tokens and automatically update DESIGN.md in the current Git repository; no read-only flag `generate` returns `sessionFile`; `iterate` requires that existing session. `variants` returns `paths` but creates no session: regenerate with an updated brief instead. @@ -662,7 +664,7 @@ fonts [top-3], colors [top-3], layouts [top-3], aesthetics [top-3]. Bias generation toward these unless the user explicitly requests a different direction. Also avoid their strong rejections: [top-3 rejected per dimension]." -**Legacy fallback:** Glob `$GSTACK_STATE_ROOT/projects/$SLUG/designs/**/approved.json` (resolve the root with gstack-paths); Read the five newest. Use explicit feedback only, never infer fonts/colors from variant letters. No usable files: continue without a taste profile. +**Legacy fallback:** Glob `$GSTACK_STATE_ROOT/projects/$SLUG/designs/**/approved.json` (resolve the root with gstack-paths); Read the five newest. To view an approved image, resolve it with `~/.claude/skills/gstack/bin/gstack-design-approved `. Use explicit feedback only, never infer fonts/colors from variant letters. No usable files: continue without a taste profile. **Conflict handling:** If the current user request contradicts a strong persistent signal (e.g., "make it playful" when taste profile strongly prefers minimal), flag diff --git a/design-consultation/sections/proposal-and-preview.md b/design-consultation/sections/proposal-and-preview.md index 090737c77..b4cb37eaa 100644 --- a/design-consultation/sections/proposal-and-preview.md +++ b/design-consultation/sections/proposal-and-preview.md @@ -304,13 +304,17 @@ echo "DESIGN_DIR: $_DESIGN_DIR" Brief: Phase 3 aesthetic/colors/type/spacing/layout plus Phase 1 product context: ```bash -$D variants --brief "" --count 3 --output-dir "$_DESIGN_DIR/" +_OUT=$($D variants --brief "" --count 3 --output-dir "$_DESIGN_DIR/"); _RC=$? +printf '%s\n' "$_OUT"; echo "EXIT: $_RC" ``` -Run quality check on each successful path returned by `variants`; never include failed variants: + +**Round accounting:** names are never overwritten (a taken one is bumped), so use only the printed `saved` paths. Tell the user how many of `requested` paid images were saved and name each `failures` entry. Exit 2 means nothing was saved: report `failures` and stop here (offer Path B or skip); run no `$D check` or board. + +Run quality check on each saved path, starting with the first; never include failed variants: ```bash -$D check --image "$_DESIGN_DIR/variant-A.png" --brief "" +$D check --image "" --brief "" ``` Read JSON, not exit code: `pass: false` means regenerate addressing `issues`, then recheck. `pass: true` with an unavailable/skipped warning is missing automated coverage; disclose it and inspect visually. @@ -321,10 +325,12 @@ Read each accepted PNG inline, then open the board with those paths before invit ### Comparison Board + Feedback Loop -Use the successful, quality-checked paths in this example: + +Write this round's board images (printed paths that passed checks, in order) as a JSON array to `$_DESIGN_DIR/board-images.json` with the Write tool; board letters A, B, C follow that order. Then archive any earlier Submit so it cannot approve these images, and build the board: ```bash -$D compare --images "$_DESIGN_DIR/variant-A.png,$_DESIGN_DIR/variant-B.png,$_DESIGN_DIR/variant-C.png" --output "$_DESIGN_DIR/design-board.html" --serve +[ -f "$_DESIGN_DIR/feedback.json" ] && mv "$_DESIGN_DIR/feedback.json" "$_DESIGN_DIR/feedback-$(date -u +%Y%m%dT%H%M%SZ).json" +$D compare --images-file "$_DESIGN_DIR/board-images.json" --output "$_DESIGN_DIR/design-board.html" --serve ``` This publishes to a persistent daemon, opens the board and exits. Read captured stderr for the startup marker; a PID is not readiness. Exit 0 with `BOARD_URL` means the daemon is serving. Save its full `http://127.0.0.1:N/boards//` URL. Only legacy `--no-daemon` needs a host background task; `SERVE_STARTED: port=N` gives root URL `http://127.0.0.1:N/`. @@ -346,18 +352,18 @@ After the response, read current feedback next to the board HTML: **Board or chat:** revisions regenerate; a final choice needs summary confirmation; skip goes to Phase 6 without a mockup. Ask if no choice/detail; never infer approval from a missing file. Submit with revision notes is a revision. **Regenerate:** -1. Revise the brief, preserving unrelated constraints. Archive this round's feedback files so old Submit cannot approve new images. -2. Run `$D variants` with the new brief (no session). Re-run the quality check and visual self-gate on every new image. -3. Rebuild: `$D compare --images "" --output "$_DESIGN_DIR/design-board.html"`, without `--serve`. +1. Revise the brief, preserving unrelated constraints. Archive this round's feedback files so old Submit cannot approve new images (the board block does this on rebuild). +2. Run `$D variants` with the new brief (no session), with the same capture and round accounting. Re-run the quality check and visual self-gate on every new image (its printed path). +3. Rebuild with the board block above (it rewrites board-images.json), without `--serve`. 4. Reload at the saved URL (keep its per-board path; legacy uses root): `jq -nc --arg html "$_DESIGN_DIR/design-board.html" '{html: $html}' | curl -sS -X POST "${BOARD_URL}api/reload" -H 'Content-Type: application/json' --data-binary @-` 5. Check reload succeeded, then AskUserQuestion at the same URL until a final choice, skip or stop. Failed generation/reload uses the fallback, not another wait. **SERVER FALLBACK:** Nonzero exit or no readiness marker: show each variant inline with Read, then AskUserQuestion: "The comparison board server failed to start. Which variant? Any changes?" Route chat feedback as above. -**After receiving feedback (any path):** summarize PREFERRED, RATINGS, YOUR NOTES, DIRECTION; AskUserQuestion "Is this right?" A confirmed final choice permits Write of `$_DESIGN_DIR/approved.json` with `approved_variant`, `feedback`, `date` (UTC), `screen` (the product page depicted by the chosen mockup), and `branch` (the current `git branch --show-current` result, empty if detached). Use valid JSON, never shell interpolation. This approves the image only; Q-final gates project writes. +**After receiving feedback (any path):** summarize PREFERRED, RATINGS, YOUR NOTES, DIRECTION; AskUserQuestion "Is this right?" A confirmed final choice permits Write of `$_DESIGN_DIR/approved.json` with `approved_variant`, `approved_path` (file name of that letter's entry in this board's `board-images.json`, never the directory listing), `feedback`, `date` (UTC), `screen` (the product page depicted by the chosen mockup), and `branch` (the current `git branch --show-current` result, empty if detached). Use valid JSON, never shell interpolation. This approves the image only; Q-final gates project writes. -After final image confirmation, `$D extract` would write DESIGN.md in a Git repo: run it only in a fresh non-repository scratch directory. Bind `$D` and `APPROVED_IMAGE` to absolute paths: +After final image confirmation, `$D extract` would write DESIGN.md in a Git repo: run it only in a fresh non-repository scratch directory. Bind `$D` and `APPROVED_IMAGE` (the confirmed letter's `board-images.json` entry) to absolute paths: ```bash _EXTRACT_DIR=$(mktemp -d /tmp/gstack-design-extract-XXXXXXXX) || exit 1 @@ -373,7 +379,7 @@ _EXTRACT_DIR=$(mktemp -d /tmp/gstack-design-extract-XXXXXXXX) || exit 1 Compare extracted tokens with the approved image and verified fonts; show discrepancies at Q-final. Empty arrays, an "Unable to extract" mood or command failure → disclose fallback to Phase 3 values, never invent measured tokens. -Late visual changes return to the feedback loop: regenerate, recheck, reconfirm, then extract again. Only `generate` supplies `sessionFile` for `$D iterate --session "" --feedback "" --output "$_DESIGN_DIR/refined.png"`; variants must regenerate. +Late visual changes return to the feedback loop: regenerate, recheck, reconfirm, then extract again. Only `generate` supplies `sessionFile` for `$D iterate --session "" --feedback "" --output "$_DESIGN_DIR/refined.png"`; use its printed `outputPath`. Variants must regenerate. **Plan mode:** Carry the approved mockup paths/tokens into Phase 6's "## Proposed DESIGN.md" plan section. Its Q-final approval governs saving that content; defer the actual DESIGN.md to implementation. diff --git a/design-consultation/sections/proposal-and-preview.md.tmpl b/design-consultation/sections/proposal-and-preview.md.tmpl index f76f9fcb0..42ffefefd 100644 --- a/design-consultation/sections/proposal-and-preview.md.tmpl +++ b/design-consultation/sections/proposal-and-preview.md.tmpl @@ -111,13 +111,17 @@ echo "DESIGN_DIR: $_DESIGN_DIR" Brief: Phase 3 aesthetic/colors/type/spacing/layout plus Phase 1 product context: ```bash -$D variants --brief "" --count 3 --output-dir "$_DESIGN_DIR/" +_OUT=$($D variants --brief "" --count 3 --output-dir "$_DESIGN_DIR/"); _RC=$? +printf '%s\n' "$_OUT"; echo "EXIT: $_RC" ``` -Run quality check on each successful path returned by `variants`; never include failed variants: + +**Round accounting:** names are never overwritten (a taken one is bumped), so use only the printed `saved` paths. Tell the user how many of `requested` paid images were saved and name each `failures` entry. Exit 2 means nothing was saved: report `failures` and stop here (offer Path B or skip); run no `$D check` or board. + +Run quality check on each saved path, starting with the first; never include failed variants: ```bash -$D check --image "$_DESIGN_DIR/variant-A.png" --brief "" +$D check --image "" --brief "" ``` Read JSON, not exit code: `pass: false` means regenerate addressing `issues`, then recheck. `pass: true` with an unavailable/skipped warning is missing automated coverage; disclose it and inspect visually. @@ -128,7 +132,7 @@ Read each accepted PNG inline, then open the board with those paths before invit {{DESIGN_SHOTGUN_LOOP}} -After final image confirmation, `$D extract` would write DESIGN.md in a Git repo: run it only in a fresh non-repository scratch directory. Bind `$D` and `APPROVED_IMAGE` to absolute paths: +After final image confirmation, `$D extract` would write DESIGN.md in a Git repo: run it only in a fresh non-repository scratch directory. Bind `$D` and `APPROVED_IMAGE` (the confirmed letter's `board-images.json` entry) to absolute paths: ```bash _EXTRACT_DIR=$(mktemp -d /tmp/gstack-design-extract-XXXXXXXX) || exit 1 @@ -144,7 +148,7 @@ _EXTRACT_DIR=$(mktemp -d /tmp/gstack-design-extract-XXXXXXXX) || exit 1 Compare extracted tokens with the approved image and verified fonts; show discrepancies at Q-final. Empty arrays, an "Unable to extract" mood or command failure → disclose fallback to Phase 3 values, never invent measured tokens. -Late visual changes return to the feedback loop: regenerate, recheck, reconfirm, then extract again. Only `generate` supplies `sessionFile` for `$D iterate --session "" --feedback "" --output "$_DESIGN_DIR/refined.png"`; variants must regenerate. +Late visual changes return to the feedback loop: regenerate, recheck, reconfirm, then extract again. Only `generate` supplies `sessionFile` for `$D iterate --session "" --feedback "" --output "$_DESIGN_DIR/refined.png"`; use its printed `outputPath`. Variants must regenerate. **Plan mode:** Carry the approved mockup paths/tokens into Phase 6's "## Proposed DESIGN.md" plan section. Its Q-final approval governs saving that content; defer the actual DESIGN.md to implementation. diff --git a/design-html/SKILL.md b/design-html/SKILL.md index 0409f2cfa..0b9a13915 100644 --- a/design-html/SKILL.md +++ b/design-html/SKILL.md @@ -399,13 +399,15 @@ Comparison boards are local HTML files: open them with `open file://...` on macO If `DESIGN_READY`: the design binary is available for visual mockup generation. Commands: -- `$D generate --brief "..." --output /path.png` — generate a single mockup -- `$D variants --brief "..." --count 3 --output-dir /path/` — generate N style variants -- `$D compare --images "a.png,b.png,c.png" --output /path/board.html --serve` — comparison board + HTTP server +- `$D generate --brief "..." --output /path.png` — generate a single mockup (prints `outputPath`) +- `$D variants --brief "..." --count 3 --output-dir /path/` — generate N style variants (prints `paths`) +- `$D compare --images-file /path/board-images.json --output /path/board.html --serve` — comparison board + HTTP server - `$D serve --html /path/board.html` — serve comparison board and collect feedback via HTTP - `$D check --image /path.png --brief "..."` — vision quality gate - `$D iterate --session /path/session.json --feedback "..." --output /path.png` — iterate +Image commands never overwrite (a taken name gets `-2`) and always print JSON (`requested`, `saved`, `failures`); exit 0 ready, 2 nothing saved, 3 stopped after saving some. Capture without `set -e`: `_OUT=$($D ...); _RC=$?`. + **Path rule:** Design artifacts belong in `$GSTACK_STATE_ROOT/projects/$SLUG/designs/`. Use `bin/gstack-paths` (docs/state-root.md). Keep it even if temporary; never substitute .context/, docs/designs/ or another directory. @@ -455,8 +457,17 @@ Now route based on what was found. Check these cases in order: ### Case A: approved.json exists (design-shotgun ran) -If `APPROVED` was found, read it. Extract: approved variant PNG path, user feedback, -screen name. Also read the CEO plan if one exists (it adds strategic context). +If `APPROVED` was found, read it for user feedback and screen name, and resolve the +approved PNG (never guess it from a letter or the newest variant): + +```bash +~/.claude/skills/gstack/bin/gstack-design-approved "" +``` + +It prints the approved image's absolute path. If it prints "approved image ... is +missing; reselect from the board" instead, tell the user exactly that, offer +/design-shotgun to reselect, and do not substitute another image. Also read the CEO +plan if one exists (it adds strategic context). Read `DESIGN.md` if it exists in the repo root. These tokens take priority for system-level values (fonts, brand colors, spacing scale). diff --git a/design-html/SKILL.md.tmpl b/design-html/SKILL.md.tmpl index 6d97772c6..9fea4b7d0 100644 --- a/design-html/SKILL.md.tmpl +++ b/design-html/SKILL.md.tmpl @@ -82,8 +82,17 @@ Now route based on what was found. Check these cases in order: ### Case A: approved.json exists (design-shotgun ran) -If `APPROVED` was found, read it. Extract: approved variant PNG path, user feedback, -screen name. Also read the CEO plan if one exists (it adds strategic context). +If `APPROVED` was found, read it for user feedback and screen name, and resolve the +approved PNG (never guess it from a letter or the newest variant): + +```bash +{{BIN_DIR}}/gstack-design-approved "" +``` + +It prints the approved image's absolute path. If it prints "approved image ... is +missing; reselect from the board" instead, tell the user exactly that, offer +/design-shotgun to reselect, and do not substitute another image. Also read the CEO +plan if one exists (it adds strategic context). Read `DESIGN.md` if it exists in the repo root. These tokens take priority for system-level values (fonts, brand colors, spacing scale). diff --git a/design-review/SKILL.md b/design-review/SKILL.md index 06072d2b8..c2aa3b7ef 100644 --- a/design-review/SKILL.md +++ b/design-review/SKILL.md @@ -736,13 +736,15 @@ Comparison boards are local HTML files: open them with `open file://...` on macO If `DESIGN_READY`: the design binary is available for visual mockup generation. Commands: -- `$D generate --brief "..." --output /path.png` — generate a single mockup -- `$D variants --brief "..." --count 3 --output-dir /path/` — generate N style variants -- `$D compare --images "a.png,b.png,c.png" --output /path/board.html --serve` — comparison board + HTTP server +- `$D generate --brief "..." --output /path.png` — generate a single mockup (prints `outputPath`) +- `$D variants --brief "..." --count 3 --output-dir /path/` — generate N style variants (prints `paths`) +- `$D compare --images-file /path/board-images.json --output /path/board.html --serve` — comparison board + HTTP server - `$D serve --html /path/board.html` — serve comparison board and collect feedback via HTTP - `$D check --image /path.png --brief "..."` — vision quality gate - `$D iterate --session /path/session.json --feedback "..." --output /path.png` — iterate +Image commands never overwrite (a taken name gets `-2`) and always print JSON (`requested`, `saved`, `failures`); exit 0 ready, 2 nothing saved, 3 stopped after saving some. Capture without `set -e`: `_OUT=$($D ...); _RC=$?`. + **Path rule:** Design artifacts belong in `$GSTACK_STATE_ROOT/projects/$SLUG/designs/`. Use `bin/gstack-paths` (docs/state-root.md). Keep it even if temporary; never substitute .context/, docs/designs/ or another directory. @@ -1736,9 +1738,12 @@ For each fixable finding, in impact order: If the gstack designer is available and the finding involves visual layout, hierarchy, or spacing (not just a CSS value fix like wrong color or font-size), generate a target mockup showing what the corrected version should look like: ```bash -$D generate --brief "" --output "$REPORT_DIR/screenshots/finding-NNN-target.png" +_OUT=$($D generate --brief "" --output "$REPORT_DIR/screenshots/finding-NNN-target.png"); _RC=$? +printf '%s\n' "$_OUT"; echo "EXIT: $_RC" ``` +Record the printed `outputPath` for this finding (images are never overwritten, so it may be bumped); later steps use that path. Exit 2: no mockup was saved; skip it for this finding. + Show the user: "Here's the current state (screenshot) and here's what it should look like (mockup). Now I'll fix the source to match." This step is optional — skip for trivial CSS fixes (wrong hex color, missing padding value). Use it for findings where the intended design isn't obvious from the description alone. @@ -1828,7 +1833,7 @@ DESIGN-FIX RISK: After all fixes are applied: 1. Re-run the design audit on all affected pages -2. If target mockups were generated during the fix loop AND `DESIGN_READY`: run `$D verify --mockup "$REPORT_DIR/screenshots/finding-NNN-target.png" --screenshot "$REPORT_DIR/screenshots/finding-NNN-after.png"` to compare the fix result against the target. Include pass/fail in the report. +2. If target mockups were generated during the fix loop AND `DESIGN_READY`: run `$D verify --mockup "" --screenshot "$REPORT_DIR/screenshots/finding-NNN-after.png"` to compare the fix result against the target. Include pass/fail in the report. 3. Compute final design score and AI slop score 4. Recompute the detector count the same way Phase 0 scanned: DOM mode re-dumps the affected pages after reload into `$REPORT_DIR/dom/$RUN_ID/` and rescans; source mode rescans the files you touched. Then delete `$REPORT_DIR/dom/$RUN_ID/` unless the user passed `--keep-dom`. 5. **If final scores are WORSE than baseline:** WARN prominently — something regressed diff --git a/design-review/SKILL.md.tmpl b/design-review/SKILL.md.tmpl index 6d7d70f0e..5b01fd0f0 100644 --- a/design-review/SKILL.md.tmpl +++ b/design-review/SKILL.md.tmpl @@ -179,9 +179,12 @@ For each fixable finding, in impact order: If the gstack designer is available and the finding involves visual layout, hierarchy, or spacing (not just a CSS value fix like wrong color or font-size), generate a target mockup showing what the corrected version should look like: ```bash -$D generate --brief "" --output "$REPORT_DIR/screenshots/finding-NNN-target.png" +_OUT=$($D generate --brief "" --output "$REPORT_DIR/screenshots/finding-NNN-target.png"); _RC=$? +printf '%s\n' "$_OUT"; echo "EXIT: $_RC" ``` +Record the printed `outputPath` for this finding (images are never overwritten, so it may be bumped); later steps use that path. Exit 2: no mockup was saved; skip it for this finding. + Show the user: "Here's the current state (screenshot) and here's what it should look like (mockup). Now I'll fix the source to match." This step is optional — skip for trivial CSS fixes (wrong hex color, missing padding value). Use it for findings where the intended design isn't obvious from the description alone. @@ -271,7 +274,7 @@ DESIGN-FIX RISK: After all fixes are applied: 1. Re-run the design audit on all affected pages -2. If target mockups were generated during the fix loop AND `DESIGN_READY`: run `$D verify --mockup "$REPORT_DIR/screenshots/finding-NNN-target.png" --screenshot "$REPORT_DIR/screenshots/finding-NNN-after.png"` to compare the fix result against the target. Include pass/fail in the report. +2. If target mockups were generated during the fix loop AND `DESIGN_READY`: run `$D verify --mockup "" --screenshot "$REPORT_DIR/screenshots/finding-NNN-after.png"` to compare the fix result against the target. Include pass/fail in the report. 3. Compute final design score and AI slop score 4. Recompute the detector count the same way Phase 0 scanned: DOM mode re-dumps the affected pages after reload into `$REPORT_DIR/dom/$RUN_ID/` and rescans; source mode rescans the files you touched. Then delete `$REPORT_DIR/dom/$RUN_ID/` unless the user passed `--keep-dom`. 5. **If final scores are WORSE than baseline:** WARN prominently — something regressed diff --git a/design-shotgun/SKILL.md b/design-shotgun/SKILL.md index d343c20ca..e3c05fd39 100644 --- a/design-shotgun/SKILL.md +++ b/design-shotgun/SKILL.md @@ -410,13 +410,15 @@ Comparison boards are local HTML files: open them with `open file://...` on macO If `DESIGN_READY`: the design binary is available for visual mockup generation. Commands: -- `$D generate --brief "..." --output /path.png` — generate a single mockup -- `$D variants --brief "..." --count 3 --output-dir /path/` — generate N style variants -- `$D compare --images "a.png,b.png,c.png" --output /path/board.html --serve` — comparison board + HTTP server +- `$D generate --brief "..." --output /path.png` — generate a single mockup (prints `outputPath`) +- `$D variants --brief "..." --count 3 --output-dir /path/` — generate N style variants (prints `paths`) +- `$D compare --images-file /path/board-images.json --output /path/board.html --serve` — comparison board + HTTP server - `$D serve --html /path/board.html` — serve comparison board and collect feedback via HTTP - `$D check --image /path.png --brief "..."` — vision quality gate - `$D iterate --session /path/session.json --feedback "..." --output /path.png` — iterate +Image commands never overwrite (a taken name gets `-2`) and always print JSON (`requested`, `saved`, `failures`); exit 0 ready, 2 nothing saved, 3 stopped after saving some. Capture without `set -e`: `_OUT=$($D ...); _RC=$?`. + **Path rule:** Design artifacts belong in `$GSTACK_STATE_ROOT/projects/$SLUG/designs/`. Use `bin/gstack-paths` (docs/state-root.md). Keep it even if temporary; never substitute .context/, docs/designs/ or another directory. @@ -552,7 +554,7 @@ fonts [top-3], colors [top-3], layouts [top-3], aesthetics [top-3]. Bias generation toward these unless the user explicitly requests a different direction. Also avoid their strong rejections: [top-3 rejected per dimension]." -**Legacy fallback:** Glob `$GSTACK_STATE_ROOT/projects/$SLUG/designs/**/approved.json` (resolve the root with gstack-paths); Read the five newest. Use explicit feedback only, never infer fonts/colors from variant letters. No usable files: continue without a taste profile. +**Legacy fallback:** Glob `$GSTACK_STATE_ROOT/projects/$SLUG/designs/**/approved.json` (resolve the root with gstack-paths); Read the five newest. To view an approved image, resolve it with `~/.claude/skills/gstack/bin/gstack-design-approved `. Use explicit feedback only, never infer fonts/colors from variant letters. No usable files: continue without a taste profile. **Conflict handling:** If the current user request contradicts a strong persistent signal (e.g., "make it playful" when taste profile strongly prefers minimal), flag @@ -576,14 +578,16 @@ _TASTE=$(find "$GSTACK_STATE_ROOT/projects/$SLUG/designs/" -name "approved.json" ``` If prior sessions exist, read each `approved.json` and extract patterns from the -approved variants. Merge these into the taste-profile.json-derived signal — if the +approved variants; resolve each approved image with `~/.claude/skills/gstack/bin/gstack-design-approved ` +(an error means that image is gone: skip it, never substitute another). Merge these into the taste-profile.json-derived signal — if the profile already says "user prefers Geist font" (from aggregated history), the approved.json files add the specific recent approval context. Limit to last 10 sessions. Try/catch JSON parse on each (skip corrupted files). **Updating taste profile after a design-shotgun session:** When the user picks a -variant, call `~/.claude/skills/gstack/bin/gstack-taste-update approved `. When they +variant, call `~/.claude/skills/gstack/bin/gstack-taste-update approved ` (the +`APPROVED_IMAGE` printed when approved.json is saved). When they explicitly reject a variant, call `~/.claude/skills/gstack/bin/gstack-taste-update rejected `. The CLI handles schema migration from approved.json, decay, and conflict flagging. @@ -674,58 +678,55 @@ variable that agents do NOT inherit. Substitute the resolved absolute path (from **Agent prompt template** (one per variant, substitute all `{...}` values): ``` -Generate a design variant and save it. +Generate a design variant and publish it without overwriting any image. Design binary: {absolute path to $D binary} +Claim helper: ~/.claude/skills/gstack/bin/gstack-design-claim Brief: {the full variant-specific brief for this direction} -Output: /tmp/variant-{letter}.png -Final location: {_DESIGN_DIR absolute path}/variant-{letter}.png +Design dir: {_DESIGN_DIR absolute path} -Steps: -1. Run: {$D path} generate --brief "{brief}" --output /tmp/variant-{letter}.png -2. If the command fails with a rate limit error (429 or "rate limit"), wait 5 seconds - and retry. Up to 3 retries. -3. If the output file is missing or empty after the command succeeds, retry once. -4. Copy: cp /tmp/variant-{letter}.png {_DESIGN_DIR}/variant-{letter}.png -5. Quality check: {$D path} check --image {_DESIGN_DIR}/variant-{letter}.png --brief "{brief}" - If quality check fails, retry generation once. -6. Verify: ls -lh {_DESIGN_DIR}/variant-{letter}.png -7. Report exactly one of: - VARIANT_{letter}_DONE: {file size} - VARIANT_{letter}_FAILED: {error description} +Steps (no set -e; never cp or mv an image): +1. Stage in a fresh per-run directory: STAGE=$(mktemp -d /tmp/gstack-variant-XXXXXXXX) +2. Generate: OUT=$({$D path} generate --brief "{brief}" --output "$STAGE/variant.png"); RC=$?; echo "$OUT"; echo "EXIT: $RC" + Read outputPath, saved and failures from the JSON. Exit 2 = nothing saved: on a rate + limit (429 or "rate limit") wait 5 seconds and retry, up to 3 retries. +3. Publish: FINAL=$(~/.claude/skills/gstack/bin/gstack-design-claim "" "{Design dir}/variant-{letter}.png") + It never overwrites and prints the final (possibly bumped) path. Use FINAL from here on. + If it fails, report the error; the staged image stays at . +4. Quality check: {$D path} check --image "" --brief "{brief}" + If it fails, regenerate once (steps 2-3; the first image is kept). +5. Verify: ls -lh "" +6. Report exactly one of: + VARIANT_{letter}_DONE: {every published path, comma-separated, last one is the pick} {file size} + VARIANT_{letter}_FAILED: {error description, plus any published paths} VARIANT_{letter}_RATE_LIMITED: exhausted retries ``` -For the evolve path, replace step 1 with: +For the evolve path, replace the step 2 command with: ``` -{$D path} evolve --screenshot {_DESIGN_DIR}/current.png --brief "{brief}" --output /tmp/variant-{letter}.png +OUT=$({$D path} evolve --screenshot {_DESIGN_DIR}/current.png --brief "{brief}" --output "$STAGE/variant.png"); RC=$? ``` -**Generate to `/tmp/`, then `cp`:** in sandboxed sessions `$D generate --output` under -`~/.gstack/` can abort ("The operation was aborted"), while `/tmp/` works. +**Why staging:** in sandboxed sessions `$D generate --output` under `~/.gstack/` can +abort ("The operation was aborted"), while `/tmp/` works. A fresh `mktemp -d` per run +means no other session's file can be published, and the claim helper never clobbers. ### Step 3d: Results After all agents complete: -1. Read each generated PNG inline (Read tool) so the user sees all variants at once. -2. Report status: "All {N} variants generated in ~{actual time}. {successes} succeeded, - {failures} failed." -3. For any failures: report explicitly with the error. Do NOT silently skip. -4. If zero variants succeeded: fall back to sequential generation (one at a time with - `$D generate`, showing each as it lands). Tell the user: "Parallel generation failed - (likely rate limiting). Falling back to sequential..." -5. Proceed to Step 4 (comparison board). - -**Dynamic image list for comparison board:** When proceeding to Step 4, construct the -image list from whatever variant files actually exist, not a hardcoded A/B/C list: - -```bash -setopt +o nomatch 2>/dev/null || true # zsh compat -_IMAGES=$(ls "$_DESIGN_DIR"/variant-*.png 2>/dev/null | tr '\n' ',' | sed 's/,$//') -``` - -Use `$_IMAGES` in the `$D compare --images` command. + +1. Round accounting first: this round's images are exactly the paths the DONE and FAILED + lines report (never a directory listing; older rounds stay on disk). Tell the user: + "{saved} of {N} paid images saved in ~{actual time}", listing every published path. +2. For any failures: report explicitly with the error. Do NOT silently skip. +3. If zero variants succeeded: fall back to sequential generation, running the agent + steps yourself one variant at a time and showing each as it lands. Tell the user: + "Parallel generation failed (likely rate limiting). Falling back to sequential..." + If that also saves nothing, report the failures and stop: no board. +4. Read each saved image inline (Read tool, the reported paths) so the user sees all variants at once. +5. Proceed to Step 4 with each variant's pick (the last reported path), in letter order, + as this round's board images. ## Step 4: Comparison Board + Feedback Loop @@ -733,8 +734,12 @@ Use `$_IMAGES` in the `$D compare --images` command. Create the comparison board and serve it over HTTP: + +Write this round's board images (printed paths that passed checks, in order) as a JSON array to `$_DESIGN_DIR/board-images.json` with the Write tool; board letters A, B, C follow that order. Then archive any earlier Submit so it cannot approve these images, and build the board: + ```bash -$D compare --images "$_DESIGN_DIR/variant-A.png,$_DESIGN_DIR/variant-B.png,$_DESIGN_DIR/variant-C.png" --output "$_DESIGN_DIR/design-board.html" --serve +[ -f "$_DESIGN_DIR/feedback.json" ] && mv "$_DESIGN_DIR/feedback.json" "$_DESIGN_DIR/feedback-$(date -u +%Y%m%dT%H%M%SZ).json" +$D compare --images-file "$_DESIGN_DIR/board-images.json" --output "$_DESIGN_DIR/design-board.html" --serve ``` Creates HTML and opens the board. **Run it in the background** (host task, or `&` redirecting stdout/stderr to private files in `$_DESIGN_DIR`). Read captured stderr for the startup marker; a PID is not readiness. Missing marker: use the failure fallback below. @@ -793,8 +798,8 @@ the approved variant. 1. Read `regenerateAction` from the JSON (`"different"`, `"match"`, `"more_like_B"`, `"remix"`, or custom text) 2. If `regenerateAction` is `"remix"`, read `remixSpec` (e.g. `{"layout":"A","colors":"B"}`) -3. Generate new variants with `$D iterate` or `$D variants` using updated brief -4. Create new board: `$D compare --images "..." --output "$_DESIGN_DIR/design-board.html"` +3. Generate new variants with `$D iterate` or `$D variants` using updated brief (capture the JSON and do round accounting as for the first round) +4. Rebuild with the board block above (it archives feedback.json and rewrites board-images.json), without `--serve` 5. Reload the board in the user's browser (same tab) — the URL is per-board under daemon mode, so use `` (from the `BOARD_URL:` stderr line) as the base: @@ -828,9 +833,16 @@ Is this right?" Use AskUserQuestion to verify before proceeding. -**Save the approved choice:** +**Save the approved choice.** Map the confirmed letter through this board's `board-images.json` (never the directory listing) and save it: + ```bash -echo '{"approved_variant":"","feedback":"","date":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","screen":"","branch":"'$(git branch --show-current 2>/dev/null)'"}' > "$_DESIGN_DIR/approved.json" +_IMG=$(jq -r --arg v "" '.[($v | explode[0]) - 65] // empty' "$_DESIGN_DIR/board-images.json") +if [ -n "$_IMG" ]; then + echo '{"approved_variant":"","approved_path":"'"$(basename "$_IMG")"'","feedback":"","date":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","screen":"","branch":"'$(git branch --show-current 2>/dev/null)'"}' > "$_DESIGN_DIR/approved.json" + echo "APPROVED_IMAGE: $_IMG" +else + echo "NO_BOARD_IMAGE: is not on this board; reselect from the board" +fi ``` ## Step 5: Feedback Confirmation @@ -854,7 +866,8 @@ Use AskUserQuestion to confirm before saving. Write `approved.json` to `$_DESIGN_DIR/` (handled by the loop above). If invoked from another skill: return the structured feedback for that skill to consume. -The calling skill reads `approved.json` and the approved variant PNG. +The calling skill reads `approved.json` and resolves the approved image with +`~/.claude/skills/gstack/bin/gstack-design-approved "$_DESIGN_DIR/approved.json"`. If standalone, offer next steps via AskUserQuestion: diff --git a/design-shotgun/SKILL.md.tmpl b/design-shotgun/SKILL.md.tmpl index ac3d1ecdd..1845aa8c7 100644 --- a/design-shotgun/SKILL.md.tmpl +++ b/design-shotgun/SKILL.md.tmpl @@ -173,14 +173,16 @@ _TASTE=$(find "$GSTACK_STATE_ROOT/projects/$SLUG/designs/" -name "approved.json" ``` If prior sessions exist, read each `approved.json` and extract patterns from the -approved variants. Merge these into the taste-profile.json-derived signal — if the +approved variants; resolve each approved image with `{{BIN_DIR}}/gstack-design-approved ` +(an error means that image is gone: skip it, never substitute another). Merge these into the taste-profile.json-derived signal — if the profile already says "user prefers Geist font" (from aggregated history), the approved.json files add the specific recent approval context. Limit to last 10 sessions. Try/catch JSON parse on each (skip corrupted files). **Updating taste profile after a design-shotgun session:** When the user picks a -variant, call `{{BIN_DIR}}/gstack-taste-update approved `. When they +variant, call `{{BIN_DIR}}/gstack-taste-update approved ` (the +`APPROVED_IMAGE` printed when approved.json is saved). When they explicitly reject a variant, call `{{BIN_DIR}}/gstack-taste-update rejected `. The CLI handles schema migration from approved.json, decay, and conflict flagging. @@ -271,58 +273,55 @@ variable that agents do NOT inherit. Substitute the resolved absolute path (from **Agent prompt template** (one per variant, substitute all `{...}` values): ``` -Generate a design variant and save it. +Generate a design variant and publish it without overwriting any image. Design binary: {absolute path to $D binary} +Claim helper: {{BIN_DIR}}/gstack-design-claim Brief: {the full variant-specific brief for this direction} -Output: /tmp/variant-{letter}.png -Final location: {_DESIGN_DIR absolute path}/variant-{letter}.png +Design dir: {_DESIGN_DIR absolute path} -Steps: -1. Run: {$D path} generate --brief "{brief}" --output /tmp/variant-{letter}.png -2. If the command fails with a rate limit error (429 or "rate limit"), wait 5 seconds - and retry. Up to 3 retries. -3. If the output file is missing or empty after the command succeeds, retry once. -4. Copy: cp /tmp/variant-{letter}.png {_DESIGN_DIR}/variant-{letter}.png -5. Quality check: {$D path} check --image {_DESIGN_DIR}/variant-{letter}.png --brief "{brief}" - If quality check fails, retry generation once. -6. Verify: ls -lh {_DESIGN_DIR}/variant-{letter}.png -7. Report exactly one of: - VARIANT_{letter}_DONE: {file size} - VARIANT_{letter}_FAILED: {error description} +Steps (no set -e; never cp or mv an image): +1. Stage in a fresh per-run directory: STAGE=$(mktemp -d /tmp/gstack-variant-XXXXXXXX) +2. Generate: OUT=$({$D path} generate --brief "{brief}" --output "$STAGE/variant.png"); RC=$?; echo "$OUT"; echo "EXIT: $RC" + Read outputPath, saved and failures from the JSON. Exit 2 = nothing saved: on a rate + limit (429 or "rate limit") wait 5 seconds and retry, up to 3 retries. +3. Publish: FINAL=$({{BIN_DIR}}/gstack-design-claim "" "{Design dir}/variant-{letter}.png") + It never overwrites and prints the final (possibly bumped) path. Use FINAL from here on. + If it fails, report the error; the staged image stays at . +4. Quality check: {$D path} check --image "" --brief "{brief}" + If it fails, regenerate once (steps 2-3; the first image is kept). +5. Verify: ls -lh "" +6. Report exactly one of: + VARIANT_{letter}_DONE: {every published path, comma-separated, last one is the pick} {file size} + VARIANT_{letter}_FAILED: {error description, plus any published paths} VARIANT_{letter}_RATE_LIMITED: exhausted retries ``` -For the evolve path, replace step 1 with: +For the evolve path, replace the step 2 command with: ``` -{$D path} evolve --screenshot {_DESIGN_DIR}/current.png --brief "{brief}" --output /tmp/variant-{letter}.png +OUT=$({$D path} evolve --screenshot {_DESIGN_DIR}/current.png --brief "{brief}" --output "$STAGE/variant.png"); RC=$? ``` -**Generate to `/tmp/`, then `cp`:** in sandboxed sessions `$D generate --output` under -`~/.gstack/` can abort ("The operation was aborted"), while `/tmp/` works. +**Why staging:** in sandboxed sessions `$D generate --output` under `~/.gstack/` can +abort ("The operation was aborted"), while `/tmp/` works. A fresh `mktemp -d` per run +means no other session's file can be published, and the claim helper never clobbers. ### Step 3d: Results After all agents complete: -1. Read each generated PNG inline (Read tool) so the user sees all variants at once. -2. Report status: "All {N} variants generated in ~{actual time}. {successes} succeeded, - {failures} failed." -3. For any failures: report explicitly with the error. Do NOT silently skip. -4. If zero variants succeeded: fall back to sequential generation (one at a time with - `$D generate`, showing each as it lands). Tell the user: "Parallel generation failed - (likely rate limiting). Falling back to sequential..." -5. Proceed to Step 4 (comparison board). - -**Dynamic image list for comparison board:** When proceeding to Step 4, construct the -image list from whatever variant files actually exist, not a hardcoded A/B/C list: - -```bash -setopt +o nomatch 2>/dev/null || true # zsh compat -_IMAGES=$(ls "$_DESIGN_DIR"/variant-*.png 2>/dev/null | tr '\n' ',' | sed 's/,$//') -``` - -Use `$_IMAGES` in the `$D compare --images` command. + +1. Round accounting first: this round's images are exactly the paths the DONE and FAILED + lines report (never a directory listing; older rounds stay on disk). Tell the user: + "{saved} of {N} paid images saved in ~{actual time}", listing every published path. +2. For any failures: report explicitly with the error. Do NOT silently skip. +3. If zero variants succeeded: fall back to sequential generation, running the agent + steps yourself one variant at a time and showing each as it lands. Tell the user: + "Parallel generation failed (likely rate limiting). Falling back to sequential..." + If that also saves nothing, report the failures and stop: no board. +4. Read each saved image inline (Read tool, the reported paths) so the user sees all variants at once. +5. Proceed to Step 4 with each variant's pick (the last reported path), in letter order, + as this round's board images. ## Step 4: Comparison Board + Feedback Loop @@ -349,7 +348,8 @@ Use AskUserQuestion to confirm before saving. Write `approved.json` to `$_DESIGN_DIR/` (handled by the loop above). If invoked from another skill: return the structured feedback for that skill to consume. -The calling skill reads `approved.json` and the approved variant PNG. +The calling skill reads `approved.json` and resolves the approved image with +`{{BIN_DIR}}/gstack-design-approved "$_DESIGN_DIR/approved.json"`. If standalone, offer next steps via AskUserQuestion: diff --git a/office-hours/SKILL.md b/office-hours/SKILL.md index 3e1a87541..d62225055 100644 --- a/office-hours/SKILL.md +++ b/office-hours/SKILL.md @@ -959,18 +959,26 @@ explore wide across diverse directions. **Step 3: Generate 3 variants** ```bash -$D variants --brief "" --count 3 --output-dir "$_DESIGN_DIR/" +_OUT=$($D variants --brief "" --count 3 --output-dir "$_DESIGN_DIR/"); _RC=$? +printf '%s\n' "$_OUT"; echo "EXIT: $_RC" ``` This generates 3 style variations of the same brief (~40 seconds total). + +**Round accounting (before any board, check or inline view):** `$D` never overwrites; a taken name is bumped (for example `-2`), so use only the paths its JSON printed and carry them as literal paths into later blocks (bash blocks do not share variables). Tell the user how many of `requested` paid images were saved (`saved`) and name each `failures` entry. Route on the exit code: 0 continue with `saved`; 3 continue with `saved` and say the run stopped early; 2 nothing was saved, so report `failures` and stop: do not run `$D compare` or `$D check`. + **Step 4: Show variants inline, then open comparison board** -Show each variant to the user inline first (read the PNGs with Read tool), then +Show each saved image to the user inline first (read the printed paths with Read tool), then create and serve the comparison board: + +Write this round's board images (printed paths that passed checks, in order) as a JSON array to `$_DESIGN_DIR/board-images.json` with the Write tool; board letters A, B, C follow that order. Then archive any earlier Submit so it cannot approve these images, and build the board: + ```bash -$D compare --images "$_DESIGN_DIR/variant-A.png,$_DESIGN_DIR/variant-B.png,$_DESIGN_DIR/variant-C.png" --output "$_DESIGN_DIR/design-board.html" --serve +[ -f "$_DESIGN_DIR/feedback.json" ] && mv "$_DESIGN_DIR/feedback.json" "$_DESIGN_DIR/feedback-$(date -u +%Y%m%dT%H%M%SZ).json" +$D compare --images-file "$_DESIGN_DIR/board-images.json" --output "$_DESIGN_DIR/design-board.html" --serve ``` This publishes the board to the design daemon, opens it in the user's default @@ -987,8 +995,8 @@ with Read and fall back to AskUserQuestion: "Which variant do you prefer? Any fe If the JSON contains `"regenerated": true`: 1. Read `regenerateAction` (or `remixSpec` for remix requests) -2. Generate new variants with `$D iterate` or `$D variants` using updated brief -3. Create new board with `$D compare` +2. Generate new variants with `$D iterate` or `$D variants` using updated brief (capture and round accounting as in Step 3) +3. Rebuild the board with Step 4's board block, without `--serve` 4. POST the new HTML to the running board. Parse the board URL from stderr (`BOARD_URL: http://127.0.0.1:N/boards//` — the daemon path) or fall back to the legacy port (`SERVE_STARTED: port=N` — only emitted under @@ -1000,11 +1008,19 @@ If `"regenerated": false`: proceed with the approved variant. **Step 6: Save approved choice** +Map the confirmed letter through this board's `board-images.json` (never the directory listing) and save it: + ```bash -echo '{"approved_variant":"","feedback":"","date":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","screen":"mockup","branch":"'$(git branch --show-current 2>/dev/null)'"}' > "$_DESIGN_DIR/approved.json" +_IMG=$(jq -r --arg v "" '.[($v | explode[0]) - 65] // empty' "$_DESIGN_DIR/board-images.json") +if [ -n "$_IMG" ]; then + echo '{"approved_variant":"","approved_path":"'"$(basename "$_IMG")"'","feedback":"","date":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","screen":"mockup","branch":"'$(git branch --show-current 2>/dev/null)'"}' > "$_DESIGN_DIR/approved.json" + echo "APPROVED_IMAGE: $_IMG" +else + echo "NO_BOARD_IMAGE: is not on this board; reselect from the board" +fi ``` -Reference the saved mockup in the design doc or plan. +Reference the printed `APPROVED_IMAGE` in the design doc or plan. ## Visual Sketch (UI ideas only) diff --git a/plan-design-review/SKILL.md b/plan-design-review/SKILL.md index a430512e2..01a9faa0e 100644 --- a/plan-design-review/SKILL.md +++ b/plan-design-review/SKILL.md @@ -654,13 +654,15 @@ Comparison boards are local HTML files: open them with `open file://...` on macO If `DESIGN_READY`: the design binary is available for visual mockup generation. Commands: -- `$D generate --brief "..." --output /path.png` — generate a single mockup -- `$D variants --brief "..." --count 3 --output-dir /path/` — generate N style variants -- `$D compare --images "a.png,b.png,c.png" --output /path/board.html --serve` — comparison board + HTTP server +- `$D generate --brief "..." --output /path.png` — generate a single mockup (prints `outputPath`) +- `$D variants --brief "..." --count 3 --output-dir /path/` — generate N style variants (prints `paths`) +- `$D compare --images-file /path/board-images.json --output /path/board.html --serve` — comparison board + HTTP server - `$D serve --html /path/board.html` — serve comparison board and collect feedback via HTTP - `$D check --image /path.png --brief "..."` — vision quality gate - `$D iterate --session /path/session.json --feedback "..." --output /path.png` — iterate +Image commands never overwrite (a taken name gets `-2`) and always print JSON (`requested`, `saved`, `failures`); exit 0 ready, 2 nothing saved, 3 stopped after saving some. Capture without `set -e`: `_OUT=$($D ...); _RC=$?`. + **Path rule:** Design artifacts belong in `$GSTACK_STATE_ROOT/projects/$SLUG/designs/`. Use `bin/gstack-paths` (docs/state-root.md). Keep it even if temporary; never substitute .context/, docs/designs/ or another directory. @@ -758,6 +760,7 @@ planning phase. Generating mockups during planning is the whole point. Allowed commands under this exception: - `mkdir -p "$GSTACK_STATE_ROOT/projects/$SLUG/designs/..."` - `$D generate`, `$D variants`, `$D compare`, `$D iterate`, `$D evolve`, `$D check` +- Writing `board-images.json` and `approved.json`, and archiving `feedback.json`, inside `$_DESIGN_DIR` - `open` (viewing comparison boards in the default browser) First, set up the output directory. Name it after the screen/feature being designed and today's date: @@ -778,13 +781,17 @@ fewer variants and benefits from sequential control; /design-shotgun is the para For each UI screen/section in scope, construct a design brief from the plan's description (and DESIGN.md if present) and generate variants: ```bash -$D variants --brief "" --count 3 --output-dir "$_DESIGN_DIR/" +_OUT=$($D variants --brief "" --count 3 --output-dir "$_DESIGN_DIR/"); _RC=$? +printf '%s\n' "$_OUT"; echo "EXIT: $_RC" ``` -After generation, run a cross-model quality check on each variant: + +**Round accounting (before any check or board):** `$D` never overwrites, so a taken name is bumped (for example `-2`); use only the printed `saved` paths. Tell the user how many of `requested` paid images were saved and name each `failures` entry. Exit 0: continue with `saved`. Exit 2: nothing was saved; report `failures` and stop: no `$D check`, no board. + +Then run a cross-model quality check on each saved path, starting with the first: ```bash -$D check --image "$_DESIGN_DIR/variant-A.png" --brief "" +$D check --image "" --brief "" ``` Flag any variants that fail the quality check. Offer to regenerate failures. @@ -798,8 +805,12 @@ feedback output. Showing mockups inline is a degraded experience. Create the comparison board and serve it over HTTP: + +Write this round's board images (printed paths that passed checks, in order) as a JSON array to `$_DESIGN_DIR/board-images.json` with the Write tool; board letters A, B, C follow that order. Then archive any earlier Submit so it cannot approve these images, and build the board: + ```bash -$D compare --images "$_DESIGN_DIR/variant-A.png,$_DESIGN_DIR/variant-B.png,$_DESIGN_DIR/variant-C.png" --output "$_DESIGN_DIR/design-board.html" --serve +[ -f "$_DESIGN_DIR/feedback.json" ] && mv "$_DESIGN_DIR/feedback.json" "$_DESIGN_DIR/feedback-$(date -u +%Y%m%dT%H%M%SZ).json" +$D compare --images-file "$_DESIGN_DIR/board-images.json" --output "$_DESIGN_DIR/design-board.html" --serve ``` Creates HTML and opens the board. **Run it in the background** (host task, or `&` redirecting stdout/stderr to private files in `$_DESIGN_DIR`). Read captured stderr for the startup marker; a PID is not readiness. Missing marker: use the failure fallback below. @@ -858,8 +869,8 @@ the approved variant. 1. Read `regenerateAction` from the JSON (`"different"`, `"match"`, `"more_like_B"`, `"remix"`, or custom text) 2. If `regenerateAction` is `"remix"`, read `remixSpec` (e.g. `{"layout":"A","colors":"B"}`) -3. Generate new variants with `$D iterate` or `$D variants` using updated brief -4. Create new board: `$D compare --images "..." --output "$_DESIGN_DIR/design-board.html"` +3. Generate new variants with `$D iterate` or `$D variants` using updated brief (capture the JSON and do round accounting as for the first round) +4. Rebuild with the board block above (it archives feedback.json and rewrites board-images.json), without `--serve` 5. Reload the board in the user's browser (same tab) — the URL is per-board under daemon mode, so use `` (from the `BOARD_URL:` stderr line) as the base: @@ -893,9 +904,16 @@ Is this right?" Use AskUserQuestion to verify before proceeding. -**Save the approved choice:** +**Save the approved choice.** Map the confirmed letter through this board's `board-images.json` (never the directory listing) and save it: + ```bash -echo '{"approved_variant":"","feedback":"","date":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","screen":"","branch":"'$(git branch --show-current 2>/dev/null)'"}' > "$_DESIGN_DIR/approved.json" +_IMG=$(jq -r --arg v "" '.[($v | explode[0]) - 65] // empty' "$_DESIGN_DIR/board-images.json") +if [ -n "$_IMG" ]; then + echo '{"approved_variant":"","approved_path":"'"$(basename "$_IMG")"'","feedback":"","date":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","screen":"","branch":"'$(git branch --show-current 2>/dev/null)'"}' > "$_DESIGN_DIR/approved.json" + echo "APPROVED_IMAGE: $_IMG" +else + echo "NO_BOARD_IMAGE: is not on this board; reselect from the board" +fi ``` **Do NOT use AskUserQuestion to ask which variant the user picked.** Read `feedback.json` — it already contains their preferred variant, ratings, comments, and overall feedback. Only use AskUserQuestion to confirm you understood the feedback correctly, never to re-ask what they chose. @@ -1105,11 +1123,11 @@ mkdir -p "$_DESIGN_DIR" _ROOT=$(git rev-parse --show-toplevel 2>/dev/null) D="$_ROOT/.claude/skills/gstack/design/dist/design" [ -x "$D" ] || D=~/.claude/skills/gstack/design/dist/design -"$D" generate --brief "" --output "$_DESIGN_DIR/ideal-.png" -echo "IDEAL_IMAGE: $_DESIGN_DIR/ideal-.png" +_OUT=$("$D" generate --brief "" --output "$_DESIGN_DIR/ideal-.png"); _RC=$? +printf '%s\n' "$_OUT"; echo "EXIT: $_RC" ``` -Show the mockup to the user via the Read tool. This makes the gap between +Exit 0: show the image at the printed `outputPath` (it may be bumped, never overwritten) to the user via the Read tool. Exit 2 or 3: report `failures` and continue text-only. This makes the gap between "what the plan describes" and "what it should look like" visceral, not abstract. If the design binary is not available, skip this and continue with text-based diff --git a/plan-design-review/SKILL.md.tmpl b/plan-design-review/SKILL.md.tmpl index 023217a56..61f9c94c4 100644 --- a/plan-design-review/SKILL.md.tmpl +++ b/plan-design-review/SKILL.md.tmpl @@ -219,6 +219,7 @@ planning phase. Generating mockups during planning is the whole point. Allowed commands under this exception: - `mkdir -p "$GSTACK_STATE_ROOT/projects/$SLUG/designs/..."` - `$D generate`, `$D variants`, `$D compare`, `$D iterate`, `$D evolve`, `$D check` +- Writing `board-images.json` and `approved.json`, and archiving `feedback.json`, inside `$_DESIGN_DIR` - `open` (viewing comparison boards in the default browser) First, set up the output directory. Name it after the screen/feature being designed and today's date: @@ -239,13 +240,17 @@ fewer variants and benefits from sequential control; /design-shotgun is the para For each UI screen/section in scope, construct a design brief from the plan's description (and DESIGN.md if present) and generate variants: ```bash -$D variants --brief "" --count 3 --output-dir "$_DESIGN_DIR/" +_OUT=$($D variants --brief "" --count 3 --output-dir "$_DESIGN_DIR/"); _RC=$? +printf '%s\n' "$_OUT"; echo "EXIT: $_RC" ``` -After generation, run a cross-model quality check on each variant: + +**Round accounting (before any check or board):** `$D` never overwrites, so a taken name is bumped (for example `-2`); use only the printed `saved` paths. Tell the user how many of `requested` paid images were saved and name each `failures` entry. Exit 0: continue with `saved`. Exit 2: nothing was saved; report `failures` and stop: no `$D check`, no board. + +Then run a cross-model quality check on each saved path, starting with the first: ```bash -$D check --image "$_DESIGN_DIR/variant-A.png" --brief "" +$D check --image "" --brief "" ``` Flag any variants that fail the quality check. Offer to regenerate failures. @@ -303,11 +308,11 @@ mkdir -p "$_DESIGN_DIR" _ROOT=$(git rev-parse --show-toplevel 2>/dev/null) D="$_ROOT/.claude/skills/gstack/design/dist/design" [ -x "$D" ] || D=~/.claude/skills/gstack/design/dist/design -"$D" generate --brief "" --output "$_DESIGN_DIR/ideal-.png" -echo "IDEAL_IMAGE: $_DESIGN_DIR/ideal-.png" +_OUT=$("$D" generate --brief "" --output "$_DESIGN_DIR/ideal-.png"); _RC=$? +printf '%s\n' "$_OUT"; echo "EXIT: $_RC" ``` -Show the mockup to the user via the Read tool. This makes the gap between +Exit 0: show the image at the printed `outputPath` (it may be bumped, never overwritten) to the user via the Read tool. Exit 2 or 3: report `failures` and continue text-only. This makes the gap between "what the plan describes" and "what it should look like" visceral, not abstract. If the design binary is not available, skip this and continue with text-based diff --git a/plan-design-review/sections/review-sections.md b/plan-design-review/sections/review-sections.md index 67c4465bb..86525df56 100644 --- a/plan-design-review/sections/review-sections.md +++ b/plan-design-review/sections/review-sections.md @@ -216,7 +216,7 @@ FIX TO 10: Rewrite vague UI descriptions with specific alternatives: - "Hero section" → what makes this hero feel like THIS product? - "Clean, modern UI" → meaningless. Replace with actual design decisions. - "Dashboard with widgets" → what makes this NOT every other dashboard? -If visual mockups were generated in Step 0.5, evaluate them against the AI slop blacklist above. Read each mockup image using the Read tool. Does the mockup fall into generic patterns (3-column grid, centered hero, stock-photo feel)? If so, flag it and offer to regenerate with more specific direction via `$D iterate --feedback "..."`. +If visual mockups were generated in Step 0.5, evaluate them against the AI slop blacklist above. Read each mockup image using the Read tool. Does the mockup fall into generic patterns (3-column grid, centered hero, stock-photo feel)? If so, flag it and offer to regenerate with more specific direction via `$D iterate --feedback "..."`; use its printed `outputPath` for any later view, check or board. **STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. ### Pass 5: Design System Alignment @@ -257,7 +257,7 @@ If mockups were generated in Step 0.5 and review passes changed significant desi AskUserQuestion: "The review passes changed [list major design changes]. Want me to regenerate mockups to reflect the updated plan? This ensures the visual reference matches what we're actually building." -If yes, use `$D iterate` with feedback summarizing the changes, or `$D variants` with an updated brief. Save to the same `$_DESIGN_DIR` directory. +If yes, use `$D iterate` with feedback summarizing the changes, or `$D variants` with an updated brief. Save to the same `$_DESIGN_DIR` directory; nothing is overwritten, so do round accounting and carry the printed paths into any new board (rebuild it with the board block, which rewrites `board-images.json`) or check. ## How to ask questions Follow the AskUserQuestion format from the Preamble above. Additional rules for plan design reviews: diff --git a/plan-design-review/sections/review-sections.md.tmpl b/plan-design-review/sections/review-sections.md.tmpl index 32c06a75a..ee1627124 100644 --- a/plan-design-review/sections/review-sections.md.tmpl +++ b/plan-design-review/sections/review-sections.md.tmpl @@ -79,7 +79,7 @@ FIX TO 10: Rewrite vague UI descriptions with specific alternatives: - "Hero section" → what makes this hero feel like THIS product? - "Clean, modern UI" → meaningless. Replace with actual design decisions. - "Dashboard with widgets" → what makes this NOT every other dashboard? -If visual mockups were generated in Step 0.5, evaluate them against the AI slop blacklist above. Read each mockup image using the Read tool. Does the mockup fall into generic patterns (3-column grid, centered hero, stock-photo feel)? If so, flag it and offer to regenerate with more specific direction via `$D iterate --feedback "..."`. +If visual mockups were generated in Step 0.5, evaluate them against the AI slop blacklist above. Read each mockup image using the Read tool. Does the mockup fall into generic patterns (3-column grid, centered hero, stock-photo feel)? If so, flag it and offer to regenerate with more specific direction via `$D iterate --feedback "..."`; use its printed `outputPath` for any later view, check or board. **STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. ### Pass 5: Design System Alignment @@ -120,7 +120,7 @@ If mockups were generated in Step 0.5 and review passes changed significant desi AskUserQuestion: "The review passes changed [list major design changes]. Want me to regenerate mockups to reflect the updated plan? This ensures the visual reference matches what we're actually building." -If yes, use `$D iterate` with feedback summarizing the changes, or `$D variants` with an updated brief. Save to the same `$_DESIGN_DIR` directory. +If yes, use `$D iterate` with feedback summarizing the changes, or `$D variants` with an updated brief. Save to the same `$_DESIGN_DIR` directory; nothing is overwritten, so do round accounting and carry the printed paths into any new board (rebuild it with the board block, which rewrites `board-images.json`) or check. ## How to ask questions Follow the AskUserQuestion format from the Preamble above. Additional rules for plan design reviews: diff --git a/scripts/resolvers/design.ts b/scripts/resolvers/design.ts index 075aa3d82..0f74c918c 100644 --- a/scripts/resolvers/design.ts +++ b/scripts/resolvers/design.ts @@ -1114,6 +1114,33 @@ ${slopSection} Source: [OpenAI "Designing Delightful Frontends with GPT-5.4"](https://developers.openai.com/blog/designing-delightful-frontends-with-gpt-5-4) (Mar 2026) + gstack design methodology.`; } +const DESIGN_ROUND_ACCOUNTING = ` +**Round accounting (before any board, check or inline view):** \`$D\` never overwrites; a taken name is bumped (for example \`-2\`), so use only the paths its JSON printed and carry them as literal paths into later blocks (bash blocks do not share variables). Tell the user how many of \`requested\` paid images were saved (\`saved\`) and name each \`failures\` entry. Route on the exit code: 0 continue with \`saved\`; 3 continue with \`saved\` and say the run stopped early; 2 nothing was saved, so report \`failures\` and stop: do not run \`$D compare\` or \`$D check\`.`; + +function designBoardBlock(serve: boolean): string { + return ` +Write this round's board images (printed paths that passed checks, in order) as a JSON array to \`$_DESIGN_DIR/board-images.json\` with the Write tool; board letters A, B, C follow that order. Then archive any earlier Submit so it cannot approve these images, and build the board: + +\`\`\`bash +[ -f "$_DESIGN_DIR/feedback.json" ] && mv "$_DESIGN_DIR/feedback.json" "$_DESIGN_DIR/feedback-$(date -u +%Y%m%dT%H%M%SZ).json" +$D compare --images-file "$_DESIGN_DIR/board-images.json" --output "$_DESIGN_DIR/design-board.html"${serve ? ' --serve' : ''} +\`\`\``; +} + +function designApprovalBlock(screen: string): string { + return `Map the confirmed letter through this board's \`board-images.json\` (never the directory listing) and save it: + +\`\`\`bash +_IMG=$(jq -r --arg v "" '.[($v | explode[0]) - 65] // empty' "$_DESIGN_DIR/board-images.json") +if [ -n "$_IMG" ]; then + echo '{"approved_variant":"","approved_path":"'"$(basename "$_IMG")"'","feedback":"","date":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","screen":"${screen}","branch":"'$(git branch --show-current 2>/dev/null)'"}' > "$_DESIGN_DIR/approved.json" + echo "APPROVED_IMAGE: $_IMG" +else + echo "NO_BOARD_IMAGE: is not on this board; reselect from the board" +fi +\`\`\``; +} + export function generateDesignSetup(ctx: TemplateContext): string { return `## DESIGN SETUP (run this check BEFORE any design mockup command) @@ -1140,12 +1167,14 @@ Comparison boards are local HTML files: open them with \`open file://...\` on ma If \`DESIGN_READY\`: the design binary is available for visual mockup generation. Commands: -- \`$D generate --brief "..." --output /path.png\` — generate a single mockup -- \`$D variants --brief "..." --count 3 --output-dir /path/\` — generate N style variants -- \`$D compare --images "a.png,b.png,c.png" --output /path/board.html --serve\` — comparison board + HTTP server +- \`$D generate --brief "..." --output /path.png\` — generate a single mockup (prints \`outputPath\`) +- \`$D variants --brief "..." --count 3 --output-dir /path/\` — generate N style variants (prints \`paths\`) +- \`$D compare --images-file /path/board-images.json --output /path/board.html --serve\` — comparison board + HTTP server - \`$D serve --html /path/board.html\` — serve comparison board and collect feedback via HTTP - \`$D check --image /path.png --brief "..."\` — vision quality gate -- \`$D iterate --session /path/session.json --feedback "..." --output /path.png\` — iterate${ctx.skillName === 'design-consultation' ? ` +- \`$D iterate --session /path/session.json --feedback "..." --output /path.png\` — iterate + +Image commands never overwrite (a taken name gets \`-2\`) and always print JSON (\`requested\`, \`saved\`, \`failures\`); exit 0 ready, 2 nothing saved, 3 stopped after saving some. Capture without \`set -e\`: \`_OUT=$($D ...); _RC=$?\`.${ctx.skillName === 'design-consultation' ? ` - \`$D extract --image /absolute/path.png\` — print tokens and automatically update DESIGN.md in the current Git repository; no read-only flag \`generate\` returns \`sessionFile\`; \`iterate\` requires that existing session. \`variants\` returns \`paths\` but creates no session: regenerate with an updated brief instead.` : ''} @@ -1192,19 +1221,20 @@ explore wide across diverse directions. **Step 3: Generate 3 variants** \`\`\`bash -$D variants --brief "" --count 3 --output-dir "$_DESIGN_DIR/" +_OUT=$($D variants --brief "" --count 3 --output-dir "$_DESIGN_DIR/"); _RC=$? +printf '%s\\n' "$_OUT"; echo "EXIT: $_RC" \`\`\` This generates 3 style variations of the same brief (~40 seconds total). +${DESIGN_ROUND_ACCOUNTING} + **Step 4: Show variants inline, then open comparison board** -Show each variant to the user inline first (read the PNGs with Read tool), then +Show each saved image to the user inline first (read the printed paths with Read tool), then create and serve the comparison board: -\`\`\`bash -$D compare --images "$_DESIGN_DIR/variant-A.png,$_DESIGN_DIR/variant-B.png,$_DESIGN_DIR/variant-C.png" --output "$_DESIGN_DIR/design-board.html" --serve -\`\`\` +${designBoardBlock(true)} This publishes the board to the design daemon, opens it in the user's default browser, and exits; it does not wait for feedback. Read captured stderr for the @@ -1220,8 +1250,8 @@ with Read and fall back to AskUserQuestion: "Which variant do you prefer? Any fe If the JSON contains \`"regenerated": true\`: 1. Read \`regenerateAction\` (or \`remixSpec\` for remix requests) -2. Generate new variants with \`$D iterate\` or \`$D variants\` using updated brief -3. Create new board with \`$D compare\` +2. Generate new variants with \`$D iterate\` or \`$D variants\` using updated brief (capture and round accounting as in Step 3) +3. Rebuild the board with Step 4's board block, without \`--serve\` 4. POST the new HTML to the running board. Parse the board URL from stderr (\`BOARD_URL: http://127.0.0.1:N/boards//\` — the daemon path) or fall back to the legacy port (\`SERVE_STARTED: port=N\` — only emitted under @@ -1233,21 +1263,15 @@ If \`"regenerated": false\`: proceed with the approved variant. **Step 6: Save approved choice** -\`\`\`bash -echo '{"approved_variant":"","feedback":"","date":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","screen":"mockup","branch":"'$(git branch --show-current 2>/dev/null)'"}' > "$_DESIGN_DIR/approved.json" -\`\`\` +${designApprovalBlock('mockup')} -Reference the saved mockup in the design doc or plan.`; +Reference the printed \`APPROVED_IMAGE\` in the design doc or plan.`; } export function generateDesignShotgunLoop(ctx: TemplateContext): string { if (ctx.skillName === 'design-consultation') return `### Comparison Board + Feedback Loop -Use the successful, quality-checked paths in this example: - -\`\`\`bash -$D compare --images "$_DESIGN_DIR/variant-A.png,$_DESIGN_DIR/variant-B.png,$_DESIGN_DIR/variant-C.png" --output "$_DESIGN_DIR/design-board.html" --serve -\`\`\` +${designBoardBlock(true)} This publishes to a persistent daemon, opens the board and exits. Read captured stderr for the startup marker; a PID is not readiness. Exit 0 with \`BOARD_URL\` means the daemon is serving. Save its full \`http://127.0.0.1:N/boards//\` URL. Only legacy \`--no-daemon\` needs a host background task; \`SERVE_STARTED: port=N\` gives root URL \`http://127.0.0.1:N/\`. @@ -1268,23 +1292,21 @@ After the response, read current feedback next to the board HTML: **Board or chat:** revisions regenerate; a final choice needs summary confirmation; skip goes to Phase 6 without a mockup. Ask if no choice/detail; never infer approval from a missing file. Submit with revision notes is a revision. **Regenerate:** -1. Revise the brief, preserving unrelated constraints. Archive this round's feedback files so old Submit cannot approve new images. -2. Run \`$D variants\` with the new brief (no session). Re-run the quality check and visual self-gate on every new image. -3. Rebuild: \`$D compare --images "" --output "$_DESIGN_DIR/design-board.html"\`, without \`--serve\`. +1. Revise the brief, preserving unrelated constraints. Archive this round's feedback files so old Submit cannot approve new images (the board block does this on rebuild). +2. Run \`$D variants\` with the new brief (no session), with the same capture and round accounting. Re-run the quality check and visual self-gate on every new image (its printed path). +3. Rebuild with the board block above (it rewrites board-images.json), without \`--serve\`. 4. Reload at the saved URL (keep its per-board path; legacy uses root): \`jq -nc --arg html "$_DESIGN_DIR/design-board.html" '{html: $html}' | curl -sS -X POST "\${BOARD_URL}api/reload" -H 'Content-Type: application/json' --data-binary @-\` 5. Check reload succeeded, then AskUserQuestion at the same URL until a final choice, skip or stop. Failed generation/reload uses the fallback, not another wait. **SERVER FALLBACK:** Nonzero exit or no readiness marker: show each variant inline with Read, then AskUserQuestion: "The comparison board server failed to start. Which variant? Any changes?" Route chat feedback as above. -**After receiving feedback (any path):** summarize PREFERRED, RATINGS, YOUR NOTES, DIRECTION; AskUserQuestion "Is this right?" A confirmed final choice permits Write of \`$_DESIGN_DIR/approved.json\` with \`approved_variant\`, \`feedback\`, \`date\` (UTC), \`screen\` (the product page depicted by the chosen mockup), and \`branch\` (the current \`git branch --show-current\` result, empty if detached). Use valid JSON, never shell interpolation. This approves the image only; Q-final gates project writes.`; +**After receiving feedback (any path):** summarize PREFERRED, RATINGS, YOUR NOTES, DIRECTION; AskUserQuestion "Is this right?" A confirmed final choice permits Write of \`$_DESIGN_DIR/approved.json\` with \`approved_variant\`, \`approved_path\` (file name of that letter's entry in this board's \`board-images.json\`, never the directory listing), \`feedback\`, \`date\` (UTC), \`screen\` (the product page depicted by the chosen mockup), and \`branch\` (the current \`git branch --show-current\` result, empty if detached). Use valid JSON, never shell interpolation. This approves the image only; Q-final gates project writes.`; return `### Comparison Board + Feedback Loop Create the comparison board and serve it over HTTP: -\`\`\`bash -$D compare --images "$_DESIGN_DIR/variant-A.png,$_DESIGN_DIR/variant-B.png,$_DESIGN_DIR/variant-C.png" --output "$_DESIGN_DIR/design-board.html" --serve -\`\`\` +${designBoardBlock(true)} Creates HTML and opens the board. **Run it in the background** (host task, or \`&\` redirecting stdout/stderr to private files in \`$_DESIGN_DIR\`). Read captured stderr for the startup marker; a PID is not readiness. Missing marker: use the failure fallback below. @@ -1342,8 +1364,8 @@ the approved variant. 1. Read \`regenerateAction\` from the JSON (\`"different"\`, \`"match"\`, \`"more_like_B"\`, \`"remix"\`, or custom text) 2. If \`regenerateAction\` is \`"remix"\`, read \`remixSpec\` (e.g. \`{"layout":"A","colors":"B"}\`) -3. Generate new variants with \`$D iterate\` or \`$D variants\` using updated brief -4. Create new board: \`$D compare --images "..." --output "$_DESIGN_DIR/design-board.html"\` +3. Generate new variants with \`$D iterate\` or \`$D variants\` using updated brief (capture the JSON and do round accounting as for the first round) +4. Rebuild with the board block above (it archives feedback.json and rewrites board-images.json), without \`--serve\` 5. Reload the board in the user's browser (same tab) — the URL is per-board under daemon mode, so use \`\` (from the \`BOARD_URL:\` stderr line) as the base: @@ -1377,10 +1399,7 @@ Is this right?" Use AskUserQuestion to verify before proceeding. -**Save the approved choice:** -\`\`\`bash -echo '{"approved_variant":"","feedback":"","date":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","screen":"","branch":"'$(git branch --show-current 2>/dev/null)'"}' > "$_DESIGN_DIR/approved.json" -\`\`\``; +**Save the approved choice.** ${designApprovalBlock('')}`; } export function generateTasteProfile(ctx: TemplateContext): string { @@ -1410,7 +1429,7 @@ fonts [top-3], colors [top-3], layouts [top-3], aesthetics [top-3]. Bias generation toward these unless the user explicitly requests a different direction. Also avoid their strong rejections: [top-3 rejected per dimension]." -**Legacy fallback:** Glob \`$GSTACK_STATE_ROOT/projects/$SLUG/designs/**/approved.json\` (resolve the root with gstack-paths); Read the five newest. Use explicit feedback only, never infer fonts/colors from variant letters. No usable files: continue without a taste profile. +**Legacy fallback:** Glob \`$GSTACK_STATE_ROOT/projects/$SLUG/designs/**/approved.json\` (resolve the root with gstack-paths); Read the five newest. To view an approved image, resolve it with \`${ctx.paths.binDir}/gstack-design-approved \`. Use explicit feedback only, never infer fonts/colors from variant letters. No usable files: continue without a taste profile. **Conflict handling:** If the current user request contradicts a strong persistent signal (e.g., "make it playful" when taste profile strongly prefers minimal), flag diff --git a/test/design-approval-flow.test.ts b/test/design-approval-flow.test.ts new file mode 100644 index 000000000..ef6e740d8 --- /dev/null +++ b/test/design-approval-flow.test.ts @@ -0,0 +1,97 @@ +/** + * Stubbed two-round design flow: round two lands at bumped names, the board + * block archives round one's Submit, approval maps the letter through this + * board's board-images.json, and a fresh session (relocated directory) + * resolves the approved image. Runs the generated SKILL.md bash blocks with + * the image API stubbed (no spend). + */ + +import { afterEach, beforeEach, expect, test } from 'bun:test'; +import { spawnSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const SKILL = fs.readFileSync(path.join(ROOT, 'plan-design-review/SKILL.md'), 'utf8'); + +function bashBlockAfter(marker: string): string { + const at = SKILL.indexOf(marker); + expect(at).toBeGreaterThan(-1); + return SKILL.slice(at).match(/```bash\n([\s\S]*?)```/)![1]; +} + +const BOARD = bashBlockAfter(''); +const APPROVAL = SKILL.slice(SKILL.indexOf('**Save the approved choice.**')).match(/```bash\n([\s\S]*?)```/)![1]; + +let dir: string; +let designDir: string; +let env: Record; + +beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'design-approval-flow-')); + designDir = path.join(dir, 'state', 'designs', 'home-20261003'); + const preload = path.join(dir, 'stub.ts'); + fs.writeFileSync(preload, `let n = 0; +globalThis.fetch = (async () => Response.json({ id: "r", output: [{ type: "image_generation_call", result: Buffer.from("round image " + (n++)).toString("base64") }] })) as typeof fetch; +`); + const design = path.join(dir, 'design'); + fs.writeFileSync(design, `#!/bin/sh +if [ "$1" = compare ]; then printf '%s\\n' "$*" > "$COMPARE_ARGS"; exit 0; fi +exec ${JSON.stringify(process.execPath)} --no-env-file --preload ${JSON.stringify(preload)} ${JSON.stringify(path.join(ROOT, 'design/src/cli.ts'))} "$@" +`, { mode: 0o755 }); + env = { + PATH: process.env.PATH!, HOME: dir, GSTACK_HOME: path.join(dir, 'gstack'), TMPDIR: dir, + OPENAI_API_KEY: 'fixture-not-a-real-key', D: design, _DESIGN_DIR: designDir, COMPARE_ARGS: path.join(dir, 'compare-args'), + }; +}); + +afterEach(() => { fs.rmSync(dir, { recursive: true, force: true }); }); + +const sh = (script: string) => spawnSync('bash', ['-c', script], { cwd: dir, env, encoding: 'utf8', timeout: 60_000 }); +const variants = () => JSON.parse(sh('"$D" variants --brief "home page" --count 2 --output-dir "$_DESIGN_DIR/"').stdout); +const approve = (letter: string) => sh(APPROVAL.replaceAll('', letter).replaceAll('', 'calmer').replaceAll('', 'home')); + +// Value: protects=round two's approval comes from round two's board, never round one's stale feedback.json or the newest-file listing; +// fails_when=the board block stops archiving feedback.json or approval maps letters through the directory; why_new=eng feedback-binding; seam=none +test('round two board archives round one Submit and approval maps to the bumped image', () => { + const round1 = variants(); + fs.writeFileSync(path.join(designDir, 'board-images.json'), JSON.stringify(round1.paths)); + fs.writeFileSync(path.join(designDir, 'feedback.json'), JSON.stringify({ preferred: 'B', regenerated: false })); + + const round2 = variants(); + expect(round2.paths.map((p: string) => path.basename(p))).toEqual(['variant-A-2.png', 'variant-B-2.png']); + fs.writeFileSync(path.join(designDir, 'board-images.json'), JSON.stringify(round2.paths)); + const board = sh(BOARD); + expect(board.status, board.stderr).toBe(0); + expect(fs.existsSync(path.join(designDir, 'feedback.json'))).toBe(false); + const archived = fs.readdirSync(designDir).filter(f => /^feedback-\d{8}T\d{6}Z\.json$/.test(f)); + expect(archived).toHaveLength(1); + expect(fs.readFileSync(env.COMPARE_ARGS, 'utf8')).toContain(`--images-file ${path.join(designDir, 'board-images.json')}`); + + const approved = approve('A'); + expect(approved.status, approved.stderr).toBe(0); + expect(approved.stdout).toContain(`APPROVED_IMAGE: ${path.join(designDir, 'variant-A-2.png')}`); + const record = JSON.parse(fs.readFileSync(path.join(designDir, 'approved.json'), 'utf8')); + expect(record).toMatchObject({ approved_variant: 'A', approved_path: 'variant-A-2.png', screen: 'home', feedback: 'calmer' }); + + const missing = approve('C'); + expect(missing.stdout).toContain('NO_BOARD_IMAGE'); + expect(JSON.parse(fs.readFileSync(path.join(designDir, 'approved.json'), 'utf8')).approved_path).toBe('variant-A-2.png'); +}); + +// Value: protects=a fresh session in a relocated state root opens round two's approved image, not variant-A.png; +// fails_when=approved_path is stored absolute or readers resolve by letter; why_new=DX relocation + CEO fresh-session recovery; seam=none +test('fresh session resolves the approved bumped image after relocation', () => { + variants(); + const round2 = variants(); + fs.writeFileSync(path.join(designDir, 'board-images.json'), JSON.stringify(round2.paths)); + expect(approve('A').status).toBe(0); + const moved = path.join(dir, 'moved', 'home-20261003'); + fs.cpSync(designDir, moved, { recursive: true }); + fs.rmSync(path.join(dir, 'state'), { recursive: true }); + const r = spawnSync(process.execPath, [path.join(ROOT, 'bin/gstack-design-approved'), path.join(moved, 'approved.json')], { encoding: 'utf8', timeout: 30_000 }); + expect(r.status, r.stderr).toBe(0); + expect(r.stdout.trim()).toBe(path.join(moved, 'variant-A-2.png')); + expect(fs.readFileSync(r.stdout.trim(), 'utf8')).toBe('round image 0'); +}); diff --git a/test/design-printed-paths.test.ts b/test/design-printed-paths.test.ts new file mode 100644 index 000000000..aa16fb8fa --- /dev/null +++ b/test/design-printed-paths.test.ts @@ -0,0 +1,165 @@ +/** + * Tripwire: design skills read the paths `$D` printed for this round, never a + * fixed variant name or a directory glob, because `$D` never overwrites and a + * second round lands at bumped names (variant-A-2.png). Runs on the generated + * SKILL.md files (with their sections) and the resolver sources. + */ + +import { describe, expect, test } from 'bun:test'; +import fs from 'node:fs'; +import path from 'node:path'; + +const ROOT = path.resolve(import.meta.dir, '..'); + +/** The one reasoned exception, anchored on content rather than a line number. */ +const ALLOWLIST = [{ + file: 'design-html/SKILL.md', + line: '_VARIANTS=$(ls -t "$GSTACK_STATE_ROOT/projects/$SLUG/designs/"*/variant-*.png 2>/dev/null | head -1)', + reason: 'cross-session discovery probe: design-html runs no generator, it only detects that earlier rounds exist; its approved image comes from approved.json', +}]; + +const GENERATOR = /(?:\$D|"\$D")\s+(generate|variants|iterate|evolve)\b/; +const CONSUMER = /^\s*(?:\$D|"\$D")\s+(compare|check)\b/m; + +function flowFiles(): string[] { + const out: string[] = []; + for (const entry of fs.readdirSync(ROOT, { withFileTypes: true })) { + if (!entry.isDirectory() || entry.name.startsWith('.') || entry.name === 'node_modules') continue; + const skill = path.join(entry.name, 'SKILL.md'); + if (fs.existsSync(path.join(ROOT, skill))) out.push(skill); + const sections = path.join(entry.name, 'sections'); + if (fs.existsSync(path.join(ROOT, sections))) { + for (const f of fs.readdirSync(path.join(ROOT, sections))) if (f.endsWith('.md')) out.push(path.join(sections, f)); + } + } + for (const f of fs.readdirSync(path.join(ROOT, 'scripts/resolvers'))) if (f.endsWith('.ts')) out.push(path.join('scripts/resolvers', f)); + return out; +} + +function bashLines(text: string): string[] { + return [...text.matchAll(/```bash\n([\s\S]*?)```/g)].flatMap(m => m[1].split('\n')); +} + +function argAfter(line: string, flag: string): string | null { + const m = line.match(new RegExp(`${flag}\\s+("([^"]+)"|(\\S+))`)); + return m ? (m[2] ?? m[3]) : null; +} + +/** Concrete bans only; returns one message per violation. */ +export function scanForBannedReads(file: string, text: string): string[] { + const violations: string[] = []; + const allowed = new Set(ALLOWLIST.filter(a => a.file === file).map(a => a.line.trim())); + const outputs = new Set(); + for (const line of text.split('\n')) { + if (GENERATOR.test(line)) { + const out = argAfter(line, '--output'); + if (out) outputs.add(out); + } + } + text.split('\n').forEach((line, i) => { + const where = `${file}:${i + 1}`; + if (allowed.has(line.trim())) return; + if (/variant-[A-Z]\.png/.test(line)) violations.push(`${where} fixed variant name: ${line.trim()}`); + if (/variant-\*\.png/.test(line)) violations.push(`${where} variant glob: ${line.trim()}`); + if (/variant-\{letter\}/.test(line) && !/gstack-design-claim/.test(line)) violations.push(`${where} variant-{letter} placeholder read: ${line.trim()}`); + const copy = line.match(/(?:^|[\s;&|(`])(?:cp|mv)\s+(.*)$/); + if (copy && (/variant|responsive-|outputPath/.test(copy[1]) || [...outputs].some(o => copy[1].includes(o)))) { + violations.push(`${where} cp/mv of a $D output (publish with gstack-design-claim): ${line.trim()}`); + } + const image = /\$D\s+check\b/.test(line) ? argAfter(line, '--image') : /\$D\s+verify\b/.test(line) ? argAfter(line, '--mockup') : null; + if (image && outputs.has(image)) violations.push(`${where} reads the requested --output instead of the printed path: ${line.trim()}`); + }); + return violations; +} + +const files = flowFiles(); +const read = (f: string) => fs.readFileSync(path.join(ROOT, f), 'utf8'); + +describe('design printed-paths tripwire', () => { + // Value: protects=no skill reads a fixed variant name, glob or requested --output after $D bumps a name; + // fails_when=a template reintroduces variant-A.png, variant-*.png, a cp of a $D output or a literal check path; why_new=#1529; seam=none + test('generated skills and resolvers read only printed paths', () => { + const violations = files.flatMap(f => scanForBannedReads(f, read(f))); + expect(violations).toEqual([]); + }); + + // Value: protects=the allowlist cannot rot into a blanket pass; fails_when=the probe line changes and the entry no longer anchors; + // why_new=content-anchored allowlist; seam=none + test('the allowlist entry still anchors to real content', () => { + for (const a of ALLOWLIST) { + expect(read(a.file).split('\n').map(l => l.trim())).toContain(a.line); + expect(a.reason.length).toBeGreaterThan(20); + } + }); + + // Value: protects=the scanner actually catches each banned form; fails_when=a regex is loosened until it matches nothing; + // why_new=negative controls for the tripwire; seam=none + test('fixtures with each banned form fail', () => { + const cases = [ + '$D compare --images "$_DESIGN_DIR/variant-A.png,$_DESIGN_DIR/variant-B.png"', + '_IMAGES=$(ls "$_DESIGN_DIR"/variant-*.png)', + '5. Quality check: {$D path} check --image {_DESIGN_DIR}/variant-{letter}.png', + 'cp /tmp/variant-B.png "$_DESIGN_DIR/"', + '$D generate --brief "x" --output "$DIR/target.png"\n$D verify --mockup "$DIR/target.png" --screenshot after.png', + '$D generate --brief "x" --output "$DIR/target.png"\n$D check --image "$DIR/target.png" --brief x', + ]; + for (const c of cases) expect(scanForBannedReads('fixture.md', c).length, c).toBeGreaterThan(0); + expect(scanForBannedReads('fixture.md', 'FINAL=$(gstack-design-claim "" "{dir}/variant-{letter}.png")')).toEqual([]); + }); +}); + +describe('design generating flows', () => { + const flows: Record = { + 'office-hours': ['office-hours/SKILL.md'], + 'plan-design-review': ['plan-design-review/SKILL.md'], + 'design-consultation': ['design-consultation/sections/proposal-and-preview.md'], + 'design-shotgun': ['design-shotgun/SKILL.md'], + }; + + for (const [skill, sources] of Object.entries(flows)) { + // Value: protects=round accounting (with the zero-saved stop) comes before the first board or check in each flow; + // fails_when=a template moves the board or check above the accounting marker or drops the stop; why_new=CEO accounting position; seam=none + test(`${skill}: round accounting precedes the first consumer and stops on zero saved`, () => { + const text = sources.map(read).join('\n'); + const accounting = text.indexOf(''); + const board = text.indexOf(''); + const consumer = text.search(CONSUMER); + expect(accounting).toBeGreaterThan(-1); + expect(board).toBeGreaterThan(accounting); + expect(consumer).toBeGreaterThan(accounting); + const span = text.slice(accounting, Math.min(board, consumer)); + expect(span).toMatch(/stop/i); + expect(span).toMatch(/nothing was saved|saves nothing|zero/i); + }); + } + + // Value: protects=generating steps capture JSON and the exit code instead of dying under set -e; + // fails_when=a bare `$D generate ...` line returns; why_new=eng template exit handling; seam=none + test('every generating bash line captures stdout and the exit code', () => { + const bare = files.filter(f => f.endsWith('.md')).flatMap(f => bashLines(read(f)) + .filter(l => GENERATOR.test(l) && !/=\$\(.*\);\s*_?RC=\$\?/.test(l)) + .map(l => `${f}: ${l.trim()}`)); + expect(bare).toEqual([]); + }); + + // Value: protects=every approval writer records approved_path so a bumped round resolves to the right image; + // fails_when=a writer goes back to approved_variant only; why_new=approved-image identity; seam=none + test('every approval-record writer stores approved_path', () => { + const writers = files.flatMap(f => read(f).split('\n') + .filter(l => /approved_variant/.test(l) && (/>\s*"\$_DESIGN_DIR\/approved\.json"/.test(l) || /Write of `\$_DESIGN_DIR\/approved\.json`/.test(l))) + .map(l => ({ f, l }))); + expect(writers.length).toBeGreaterThanOrEqual(3); + for (const { f, l } of writers) expect(l, f).toContain('approved_path'); + }); + + // Value: protects=shotgun subagents stage in a fresh mktemp dir and publish through the no-clobber helper; + // fails_when=the shared /tmp/variant file and cp return; why_new=eng staging requirement; seam=none + test('design-shotgun stages per run and publishes with gstack-design-claim', () => { + const shotgun = read('design-shotgun/SKILL.md'); + const prompt = shotgun.slice(shotgun.indexOf('**Agent prompt template**'), shotgun.indexOf('### Step 3d')); + expect(prompt).toContain('mktemp -d /tmp/'); + expect(prompt).toMatch(/gstack-design-claim ""/); + expect(prompt).not.toMatch(/\/tmp\/variant-/); + expect(prompt).toMatch(/every published path/); + }); +});