From b2b5b5e7de8375bd1f2775d8429704fee8e6d556 Mon Sep 17 00:00:00 2001 From: Jinzhe Date: Thu, 16 Jul 2026 18:08:00 +0800 Subject: [PATCH] Address Codex design-shotgun review feedback --- design-shotgun/SKILL.md | 4 +- design-shotgun/SKILL.md.tmpl | 4 +- scripts/resolvers/design.ts | 211 +++++++++++++++++---------- test/codex-e2e.test.ts | 52 +++++++ test/gen-skill-docs.test.ts | 63 +++++++- test/helpers/codex-session-runner.ts | 8 +- 6 files changed, 249 insertions(+), 93 deletions(-) diff --git a/design-shotgun/SKILL.md b/design-shotgun/SKILL.md index 3386d18fa..fc4043052 100644 --- a/design-shotgun/SKILL.md +++ b/design-shotgun/SKILL.md @@ -1006,8 +1006,8 @@ curl -s -o /dev/null -w "%{http_code}" http://localhost:3000 2>/dev/null || echo ``` If a local site is running AND the user referenced a URL or said something like "I don't -like how this looks," screenshot the current page and use `$D evolve` instead of -`$D variants` to generate improvement variants from the existing design. +like how this looks," capture the current page and use the screenshot-evolution path in +Step 3c instead of generating unrelated variants from text alone. **AskUserQuestion with pre-filled context:** Pre-fill what you inferred from the codebase, DESIGN.md, and office-hours output. Then ask for what's missing. Frame as ONE question diff --git a/design-shotgun/SKILL.md.tmpl b/design-shotgun/SKILL.md.tmpl index 4d8f0bb2f..7db001da2 100644 --- a/design-shotgun/SKILL.md.tmpl +++ b/design-shotgun/SKILL.md.tmpl @@ -127,8 +127,8 @@ curl -s -o /dev/null -w "%{http_code}" http://localhost:3000 2>/dev/null || echo ``` If a local site is running AND the user referenced a URL or said something like "I don't -like how this looks," screenshot the current page and use `$D evolve` instead of -`$D variants` to generate improvement variants from the existing design. +like how this looks," capture the current page and use the screenshot-evolution path in +Step 3c instead of generating unrelated variants from text alone. **AskUserQuestion with pre-filled context:** Pre-fill what you inferred from the codebase, DESIGN.md, and office-hours output. Then ask for what's missing. Frame as ONE question diff --git a/scripts/resolvers/design.ts b/scripts/resolvers/design.ts index f4cd2208d..5edf7f403 100644 --- a/scripts/resolvers/design.ts +++ b/scripts/resolvers/design.ts @@ -789,19 +789,47 @@ export function generateDesignSetup(ctx: TemplateContext): string { if (ctx.host === 'codex' && ctx.skillName === 'design-shotgun') { return `## CODEX IMAGE SETUP (run this check BEFORE generating design variants) -This skill is running as \`gstack-design-shotgun\` in Codex. Use Codex's native -\`image_gen\` tool for visual mockups. +This skill is running as \`gstack-design-shotgun\` in Codex. Invoke the documented +\`$imagegen\` skill for mockup generation and editing. Its default built-in mode uses +Codex's \`image_gen\` tool and does not require \`OPENAI_API_KEY\`. -Do NOT run the gstack design binary for this skill: -- no \`$D generate\` -- no \`$D variants\` -- no \`$D evolve\` -- no OpenAI API key setup -- no \`OPENAI_API_KEY\` lookup +Resolve both local helpers. \`$B\` captures an existing page for screenshot evolution. +\`$D\` is allowed only for the local comparison-board commands \`compare\` and \`serve\`: -Design artifacts still belong in \`~/.gstack/projects/$SLUG/designs/\`. The -\`image_gen\` tool may write images elsewhere first; copy the returned image files -into the design session directory before building the comparison board.`; +\`\`\`bash +_ROOT=$(git rev-parse --show-toplevel 2>/dev/null) +D="" +[ -n "$_ROOT" ] && [ -x "$_ROOT/${ctx.paths.localSkillRoot}/design/dist/design" ] && D="$_ROOT/${ctx.paths.localSkillRoot}/design/dist/design" +[ -z "$D" ] && D="${ctx.paths.designDir}/design" +if [ -x "$D" ]; then + echo "DESIGN_BOARD_READY: $D" +else + echo "DESIGN_BOARD_NOT_AVAILABLE" +fi +B="" +[ -n "$_ROOT" ] && [ -x "$_ROOT/${ctx.paths.localSkillRoot}/browse/dist/browse" ] && B="$_ROOT/${ctx.paths.localSkillRoot}/browse/dist/browse" +[ -z "$B" ] && B="${ctx.paths.browseDir}/browse" +if [ -x "$B" ]; then + echo "BROWSE_READY: $B" +else + echo "BROWSE_NOT_AVAILABLE" +fi +\`\`\` + +If \`BROWSE_NOT_AVAILABLE\` while evolving an existing page, ask the user for a +screenshot or stop that evolution path. New-image exploration can still proceed. + +If \`DESIGN_BOARD_NOT_AVAILABLE\`, show the generated images inline and collect +feedback with AskUserQuestion. Do not create a platform-specific static-board fallback. + +Codex generation restrictions: +- no \`$D generate\`, \`$D variants\`, \`$D evolve\`, \`$D iterate\`, or \`$D check\` +- no \`$imagegen\` CLI fallback +- no OpenAI API key setup or \`OPENAI_API_KEY\` lookup + +Design artifacts still belong in \`~/.gstack/projects/$SLUG/designs/\`. Copy the +paths returned by \`$imagegen\` into the design session directory before building +the comparison board.`; } return `## DESIGN SETUP (run this check BEFORE any design mockup command) @@ -929,46 +957,51 @@ Reference the saved mockup in the design doc or plan.`; export function generateDesignShotgunGeneration(ctx: TemplateContext): string { if (ctx.host === 'codex') { - return `### Step 3c: Codex Native Image Generation + return `### Step 3c: Codex \`$imagegen\` Generation -**Codex host rule:** This skill is running as \`gstack-design-shotgun\`. Use the -host-native \`image_gen\` tool directly. Do NOT run \`$D generate\`, \`$D variants\`, -\`$D evolve\`, or any command that requires \`OPENAI_API_KEY\`. +**Codex host rule:** Invoke the documented \`$imagegen\` skill once per confirmed +variant. Keep it in default built-in mode so it uses the host \`image_gen\` tool. +Do not invoke \`image_gen\` as an undocumented command, use \`scripts/image_gen.py\`, +or run any \`$D\` generation command. -**If evolving from a screenshot** (user said "I don't like THIS"), take ONE screenshot -first: +**If evolving from a screenshot** (user said "I don't like THIS"), first confirm +CODEX IMAGE SETUP printed \`BROWSE_READY\`, navigate \`$B\` to the target URL when needed, and +capture ONE current-state screenshot: \`\`\`bash $B screenshot "$_DESIGN_DIR/current.png" \`\`\` -Use that screenshot as visual context in the \`image_gen\` prompt when the tool -supports image editing or image reference input. If the tool only accepts text, -describe the screenshot's visible layout, palette, hierarchy, and the requested -change before generating. +Load \`$_DESIGN_DIR/current.png\` with the host \`view_image\` tool so it is visible +in conversation context. Then invoke \`$imagegen\` in edit mode once per variant, +using the screenshot as the edit target. State the invariant that the product, +content, and interaction purpose stay recognizable while the confirmed visual +direction changes. If \`BROWSE_NOT_AVAILABLE\`, do not invent a screenshot from text. -**Generate one image per variant.** For each confirmed direction, call the -\`image_gen\` tool with a complete prompt that includes: +**For a new design**, invoke \`$imagegen\` in generate mode once per variant. Each +invocation must use the default built-in mode and a complete prompt containing: -- the product/screen brief -- the variant-specific direction +- \`Use case: ui-mockup\` +- the product/screen brief and variant-specific direction - audience and job-to-be-done - visible states and edge cases that must appear - design-system constraints from DESIGN.md or the taste profile - hard output requirement: a polished UI mockup screenshot, no logos, no tiny unreadable body text, no decorative filler -Run these calls in parallel if the host supports parallel tool calls. Otherwise run -them sequentially and show each result as it lands. +Run the independent \`$imagegen\` invocations in parallel when the host supports it. +Otherwise run them sequentially and show each result as it lands. If the built-in +path fails or is unavailable, report the failure and stop; do not switch to the CLI +fallback or ask for \`OPENAI_API_KEY\`. -**Save returned images into the design session directory.** The \`image_gen\` tool -returns or writes image files under the Codex generated-images directory. Copy each -returned image path to the canonical design session path: +**Save returned images into the design session directory.** Copy each path returned +by \`$imagegen\` from the Codex generated-images directory to the canonical design +session path: \`\`\`bash -cp "" "$_DESIGN_DIR/variant-A.png" -cp "" "$_DESIGN_DIR/variant-B.png" -cp "" "$_DESIGN_DIR/variant-C.png" +cp "<$imagegen returned path for variant A>" "$_DESIGN_DIR/variant-A.png" +cp "<$imagegen returned path for variant B>" "$_DESIGN_DIR/variant-B.png" +cp "<$imagegen returned path for variant C>" "$_DESIGN_DIR/variant-C.png" ls -lh "$_DESIGN_DIR"/variant-*.png \`\`\` @@ -982,20 +1015,19 @@ After generation completes: 1. Show every generated image inline so the user sees all variants at once. 2. Report status: "All {N} variants generated. {successes} succeeded, {failures} failed." 3. For any failures: report explicitly with the error. Do NOT silently skip. -4. If zero variants succeeded: stop and say \`image_gen\` did not return usable images. - Do NOT fall back to \`$D generate\` on Codex. +4. If zero variants succeeded: stop and say \`$imagegen\` did not return usable images. + Do NOT fall back to the gstack design binary on Codex. 5. Proceed to Step 4 (comparison board or inline feedback fallback). -**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: +**Build a portable image list for the comparison board.** Include every successful +variant without relying on shell glob behavior: \`\`\`bash -setopt +o nomatch 2>/dev/null || true # zsh compat -_IMAGES=$(ls "$_DESIGN_DIR"/variant-*.png 2>/dev/null | tr '\\n' ',' | sed 's/,$//') +_IMAGES=$(find "$_DESIGN_DIR" -maxdepth 1 -type f -name 'variant-*.png' -print 2>/dev/null | sort | awk 'BEGIN { sep="" } { printf "%s%s", sep, $0; sep="," }') echo "IMAGES: $_IMAGES" \`\`\` -Use \`$_IMAGES\` for any comparison-board generation.`; +Use \`$_IMAGES\` for comparison-board generation.`; } return `### Step 3c: Parallel Generation @@ -1077,58 +1109,75 @@ export function generateDesignShotgunLoop(ctx: TemplateContext): string { if (ctx.host === 'codex') { return `### Comparison Board + Feedback Loop -Codex path: do not use \`$D compare\` or \`$D serve\`. Build a lightweight local board -from the images generated by \`image_gen\`, then collect feedback directly. +The gstack design binary remains useful here because \`compare\` and \`serve\` are +local board operations. They do not generate images or require an OpenAI API key. -Create a static comparison board: +**If \`DESIGN_BOARD_READY\`:** Create the board with the portable gstack board path: \`\`\`bash -node - "$_DESIGN_DIR" <<'NODE' -const fs = require('fs'); -const path = require('path'); -const dir = process.argv[2]; -const files = fs.readdirSync(dir).filter(f => /^variant-[A-Z]\\.png$/.test(f)).sort(); -const cards = files.map(file => { - const label = file.match(/^variant-([A-Z])\\.png$/)?.[1] || file; - return \`

Variant \${label}

Variant \${label}
\`; -}).join('\\n'); -const html = \` - -Design Shotgun Variants - -

Design Shotgun Variants

-
\${cards}
\`; -fs.writeFileSync(path.join(dir, 'design-board.html'), html); -console.log(path.join(dir, 'design-board.html')); -NODE -open "file://$_DESIGN_DIR/design-board.html" 2>/dev/null || true +$D compare --images "$_IMAGES" --output "$_DESIGN_DIR/design-board.html" --serve \`\`\` -Show each variant inline, then ask for feedback: +This generates the board HTML, starts the cross-platform HTTP server on a random +port, and opens the user's default browser. Run it in the background while the user +interacts with the board. Parse \`BOARD_URL: http://127.0.0.1:N/boards//\` from +stderr; the URL already includes the per-board path. -"I've opened a comparison board with the generated variants. Which variant do you -prefer, and what should change?" +Use AskUserQuestion only as the blocking wait mechanism. Include \`BOARD_URL\` so the +user can reopen the board, and ask them to submit feedback there. Do not use +AskUserQuestion as a second variant chooser while the board is available. -Use AskUserQuestion when available. If AskUserQuestion is unavailable, ask in prose -and wait for the user's typed answer. +After the user responds, check the board feedback files: -**After receiving feedback:** Output a clear summary confirming what was understood: +\`\`\`bash +if [ -f "$_DESIGN_DIR/feedback.json" ]; then + echo "SUBMIT_RECEIVED" + cat "$_DESIGN_DIR/feedback.json" +elif [ -f "$_DESIGN_DIR/feedback-pending.json" ]; then + echo "REGENERATE_RECEIVED" + cat "$_DESIGN_DIR/feedback-pending.json" + rm "$_DESIGN_DIR/feedback-pending.json" +else + echo "NO_FEEDBACK_FILE" +fi +\`\`\` -"Here's what I understood from your feedback: -PREFERRED: Variant [X] -YOUR NOTES: [comments] -DIRECTION: [overall] +The feedback JSON has this shape: +\`\`\`json +{ + "preferred": "A", + "ratings": { "A": 4, "B": 3, "C": 2 }, + "comments": { "A": "Love the spacing" }, + "overall": "Go with A, bigger CTA", + "regenerated": false +} +\`\`\` -Is this right?" +**If \`feedback-pending.json\` is found:** +1. Read \`regenerateAction\` (\`"different"\`, \`"match"\`, \`"more_like_B"\`, + \`"remix"\`, or custom text) and any \`remixSpec\`. +2. Invoke \`$imagegen\` once per replacement variant in default built-in mode. For + \`more_like\` or \`remix\`, first load the selected variant PNGs with \`view_image\` + and use edit/reference mode. For \`different\`, generate from the updated brief. +3. Copy every returned image into its canonical \`variant-.png\` path and + rebuild \`$_IMAGES\` with the portable loop from Step 3d. +4. Rebuild the board locally: + \`$D compare --images "$_IMAGES" --output "$_DESIGN_DIR/design-board.html"\` +5. Reload the same daemon board: + \`curl -s -X POST "\${BOARD_URL}api/reload" -H 'Content-Type: application/json' -d '{"html":"$_DESIGN_DIR/design-board.html"}'\` +6. AskUserQuestion again with the same board URL and repeat until + \`feedback.json\` appears. -Use AskUserQuestion or prose confirmation before saving. +Never use \`$D iterate\`, \`$D variants\`, or the \`$imagegen\` CLI fallback for +regeneration on Codex. + +**If \`DESIGN_BOARD_NOT_AVAILABLE\` or the board server fails:** Show every variant +inline, then use AskUserQuestion to ask which variant the user prefers and what should +change. This fallback is host-native and does not use \`open\`, \`start\`, or +\`xdg-open\` directly. + +**After receiving feedback (any path):** Output a clear summary confirming what was +understood, then use AskUserQuestion to verify before proceeding. **Save the approved choice:** \`\`\`bash diff --git a/test/codex-e2e.test.ts b/test/codex-e2e.test.ts index 2f2817f90..62626013f 100644 --- a/test/codex-e2e.test.ts +++ b/test/codex-e2e.test.ts @@ -55,6 +55,13 @@ if (!evalsEnabled) { // Codex E2E touchfiles — keyed by test name, same pattern as E2E_TOUCHFILES const CODEX_E2E_TOUCHFILES: Record = { 'codex-discover-skill': ['codex/**', '.agents/skills/**', 'test/helpers/codex-session-runner.ts'], + 'codex-design-shotgun-routing': [ + 'design-shotgun/**', + '.agents/skills/gstack-design-shotgun/**', + 'scripts/resolvers/design.ts', + 'scripts/resolvers/index.ts', + 'test/helpers/codex-session-runner.ts', + ], 'codex-review-findings': ['review/**', '.agents/skills/gstack-review/**', 'codex/**', 'test/helpers/codex-session-runner.ts'], }; @@ -159,6 +166,51 @@ describeCodex('Codex E2E', () => { ).toBe(true); }, 120_000); + testIfSelected('codex-design-shotgun-routing', async () => { + const skillDir = path.join(testWorktree, '.agents', 'skills', 'gstack-design-shotgun'); + + const result = await runCodexSkill({ + skillDir, + prompt: `Use the gstack-design-shotgun skill for this routing smoke test. +Do not generate images, run shell commands, modify files, or ask questions. Read the +Codex-specific workflow and state exactly: +1. which skill handles each generated or edited variant; +2. which command captures an existing page for screenshot evolution; +3. which command creates and serves the comparison board; +4. whether OPENAI_API_KEY is required. +Keep the answer under 150 words.`, + timeoutMs: 120_000, + cwd: testWorktree, + skillName: 'gstack-design-shotgun', + homeParent: path.join(testWorktree, '.codex-e2e-homes'), + model: 'gpt-5.4', + }); + + logCodexCost('codex-design-shotgun-routing', result); + + if (result.exitCode === 124 || result.exitCode === 137) { + console.warn(`codex-design-shotgun-routing: Codex timed out (exit ${result.exitCode}) - skipping assertions`); + recordCodexE2E('codex-design-shotgun-routing', result, true); + return; + } + + const output = result.output; + const passed = result.exitCode === 0 + && /\$imagegen|imagegen/i.test(output) + && /\$B\s+screenshot/i.test(output) + && /\$D\s+compare/i.test(output) + && /OPENAI_API_KEY/i.test(output) + && /not required|does not require|no API key|without.*API key/i.test(output); + recordCodexE2E('codex-design-shotgun-routing', result, passed); + + expect(result.exitCode).toBe(0); + expect(output).toMatch(/\$imagegen|imagegen/i); + expect(output).toMatch(/\$B\s+screenshot/i); + expect(output).toMatch(/\$D\s+compare/i); + expect(output).toMatch(/OPENAI_API_KEY/i); + expect(output).toMatch(/not required|does not require|no API key|without.*API key/i); + }, 180_000); + // Validates that Codex can invoke the gstack-review skill, run a diff-based // code review, and produce structured review output with findings/issues. // Accepts Codex timeout (exit 124/137) as non-failure since that's a CLI perf issue. diff --git a/test/gen-skill-docs.test.ts b/test/gen-skill-docs.test.ts index 8e50e7eeb..dae41bdf3 100644 --- a/test/gen-skill-docs.test.ts +++ b/test/gen-skill-docs.test.ts @@ -1797,17 +1797,66 @@ describe('Codex generation (--host codex)', () => { expect(reviewContent).not.toContain('CODEX_REVIEWS'); }); - test('Codex design-shotgun uses image_gen instead of gstack design binary', () => { + test('Codex design-shotgun routes generation through $imagegen and keeps local helpers', () => { const content = fs.readFileSync(path.join(AGENTS_DIR, 'gstack-design-shotgun', 'SKILL.md'), 'utf-8'); expect(content).toContain('gstack-design-shotgun'); + expect(content).toContain('$imagegen'); expect(content).toContain('image_gen'); - expect(content).toContain('host-native'); - expect(content).toContain('no OpenAI API key setup'); - expect(content).toContain('no `OPENAI_API_KEY` lookup'); + expect(content).toContain('default built-in mode'); + expect(content).toContain('$B screenshot "$_DESIGN_DIR/current.png"'); + expect(content).toContain('Load `$_DESIGN_DIR/current.png` with the host `view_image` tool'); + expect(content).toContain('DESIGN_BOARD_READY: $D'); + expect(content).toContain('BROWSE_READY: $B'); + expect(content).toContain('$D compare --images "$_IMAGES"'); + expect(content).toContain('no OpenAI API key setup or `OPENAI_API_KEY` lookup'); expect(content).not.toContain('Run: {$D path} generate'); - expect(content).not.toContain('$D compare --images'); - expect(content).not.toContain('DESIGN_READY: $D'); - expect(content).not.toContain('DESIGN_NOT_AVAILABLE'); + expect(content).not.toContain('$D evolve --screenshot'); + expect(content).not.toContain('node - "$_DESIGN_DIR"'); + expect(content).not.toContain('open "file://$_DESIGN_DIR/design-board.html"'); + expect(content).not.toContain('$HOME$GSTACK_DESIGN'); + expect(content).not.toContain('$HOME$GSTACK_BROWSE'); + }); + + test('Codex design-shotgun setup resolves browser and board binaries', () => { + const content = fs.readFileSync(path.join(AGENTS_DIR, 'gstack-design-shotgun', 'SKILL.md'), 'utf-8'); + const sectionStart = content.indexOf('## CODEX IMAGE SETUP'); + const fenceStart = content.indexOf('```bash\n', sectionStart) + '```bash\n'.length; + const fenceEnd = content.indexOf('\n```', fenceStart); + expect(sectionStart).toBeGreaterThanOrEqual(0); + expect(fenceStart).toBeGreaterThan(sectionStart); + expect(fenceEnd).toBeGreaterThan(fenceStart); + + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-codex-design-setup-')); + try { + const designDir = path.join(tempDir, 'design', 'dist'); + const browseDir = path.join(tempDir, 'browse', 'dist'); + fs.mkdirSync(designDir, { recursive: true }); + fs.mkdirSync(browseDir, { recursive: true }); + fs.writeFileSync(path.join(designDir, 'design'), '#!/bin/sh\nexit 0\n'); + fs.writeFileSync(path.join(browseDir, 'browse'), '#!/bin/sh\nexit 0\n'); + fs.chmodSync(path.join(designDir, 'design'), 0o755); + fs.chmodSync(path.join(browseDir, 'browse'), 0o755); + + const result = Bun.spawnSync(['bash', '-c', content.slice(fenceStart, fenceEnd)], { + cwd: tempDir, + env: { + ...process.env, + HOME: tempDir, + GSTACK_DESIGN: designDir, + GSTACK_BROWSE: browseDir, + }, + stdout: 'pipe', + stderr: 'pipe', + }); + + expect(result.exitCode).toBe(0); + const output = result.stdout.toString(); + expect(output).toContain(`DESIGN_BOARD_READY: ${path.join(designDir, 'design')}`); + expect(output).toContain(`BROWSE_READY: ${path.join(browseDir, 'browse')}`); + expect(result.stderr.toString()).toBe(''); + } finally { + fs.rmSync(tempDir, { recursive: true, force: true }); + } }); test('--host codex --dry-run freshness', () => { diff --git a/test/helpers/codex-session-runner.ts b/test/helpers/codex-session-runner.ts index 404aa6bb2..1dae29f27 100644 --- a/test/helpers/codex-session-runner.ts +++ b/test/helpers/codex-session-runner.ts @@ -144,6 +144,8 @@ export async function runCodexSkill(opts: { cwd?: string; // Working directory skillName?: string; // Skill name for installation (default: dirname) sandbox?: string; // Sandbox mode (default: 'read-only') + homeParent?: string; // Parent for isolated HOME (default: OS temp dir) + model?: string; // Optional stable model override for evals }): Promise { const { skillDir, @@ -152,6 +154,8 @@ export async function runCodexSkill(opts: { cwd, skillName, sandbox = 'read-only', + homeParent = os.tmpdir(), + model, } = opts; const startTime = Date.now(); @@ -174,7 +178,8 @@ export async function runCodexSkill(opts: { } // Set up temp HOME with skill installed - const tempHome = fs.mkdtempSync(path.join(os.tmpdir(), 'codex-e2e-')); + fs.mkdirSync(homeParent, { recursive: true }); + const tempHome = fs.mkdtempSync(path.join(homeParent, 'codex-e2e-')); const realHome = os.homedir(); try { @@ -201,6 +206,7 @@ export async function runCodexSkill(opts: { // Build codex exec command const args = ['exec', prompt, '--json', '-s', sandbox]; + if (model) args.push('-m', model); // Spawn codex with temp HOME so it discovers our installed skill. // Hermetic scrub (test/helpers/hermetic-env.ts) with codex's auth surface