From 1f678a5b81e75ec19342e7326a1c279d559f8984 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 9 Sep 2026 04:20:08 +0000 Subject: [PATCH] fix: resolve workflow gaps exposed by frontier evals Clarify plan-review ordering and fallback modes, preserve deploy readiness gates, honor configured merge methods, correct benchmark and canary contracts, and restore vendored installs on setup failure. Cover recovery with real-shell regressions. Co-Authored-By: OpenAI Codex --- CHANGELOG.md | 3 + benchmark/SKILL.md | 23 ++++--- benchmark/SKILL.md.tmpl | 23 ++++--- canary/SKILL.md | 13 +++- canary/SKILL.md.tmpl | 13 +++- design-review/SKILL.md | 2 +- gstack-upgrade/SKILL.md | 51 +++++++++------ gstack-upgrade/SKILL.md.tmpl | 51 +++++++++------ land-and-deploy/SKILL.md | 6 +- land-and-deploy/SKILL.md.tmpl | 6 +- land-and-deploy/sections/merge-and-deploy.md | 12 ++-- .../sections/merge-and-deploy.md.tmpl | 12 ++-- plan-ceo-review/sections/review-sections.md | 10 +-- .../sections/review-sections.md | 12 ++-- .../sections/review-sections.md.tmpl | 10 ++- plan-devex-review/sections/review-sections.md | 10 +-- plan-eng-review/SKILL.md | 8 +-- plan-eng-review/SKILL.md.tmpl | 8 +-- plan-eng-review/sections/review-sections.md | 41 ++++++------ .../sections/review-sections.md.tmpl | 7 +- scripts/resolvers/design.ts | 2 +- scripts/resolvers/review.ts | 10 +-- setup-deploy/SKILL.md | 9 +++ setup-deploy/SKILL.md.tmpl | 9 +++ test/helpers/carve-guards.ts | 2 +- test/land-and-deploy-postfail.test.ts | 4 +- test/upgrade-setup-recovery.test.ts | 64 +++++++++++++++++++ test/workflow-excerpt.test.ts | 26 ++++++++ 28 files changed, 317 insertions(+), 130 deletions(-) create mode 100644 test/upgrade-setup-recovery.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index c62234229..04cde3526 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ ### Fixed - Native Codex reviews honor the selected model even when the CLI has a separate review-model pin. Claude judges parse text after thinking blocks, and arm judgments omit the temperature setting Fable does not support. +- Frontier judges have enough output budget for thinking and JSON. Workflow evals read generated sections in execution order, without duplicated content. +- Ship and plan reviews keep approval gates with the parent agent, preserve readiness checks, and use project-native test commands. QA scoring and monitoring rules are explicit; Aside drive options wait for a successful readiness probe. +- Benchmark timing uses the navigation entry's actual fields. Deploy setup collects missing configuration, and failed vendored upgrades restore their backup instead of deleting it. ## [1.81.0.0] - 2026-09-06 diff --git a/benchmark/SKILL.md b/benchmark/SKILL.md index dcdd3d30b..2c5d9ded4 100644 --- a/benchmark/SKILL.md +++ b/benchmark/SKILL.md @@ -294,13 +294,13 @@ await closeTab(pg); console.log("GSTACK_STEP_OK"); `NAV=` is the navigation timing entry, `PAINT=` the paint entries (FCP lives here), `LCP=` the largest-contentful-paint start time (`null` if the page emitted no LCP entry within 3s), `RESOURCES=` the 15 slowest resources, `SCRIPTS=` / `CSS=` the bundle inventory, `SUMMARY=` request count, total transfer, and requests by type. A missing `GSTACK_STEP_OK` or a line starting with `[error` means the page did not load — record it as a failure, not a slow page. -Extract key metrics from `NAV=`: +Extract key metrics from the labelled lines (`NAV=` unless stated otherwise): - **TTFB** (Time to First Byte): `responseStart - requestStart` - **FCP** (First Contentful Paint): the `first-contentful-paint` entry in `PAINT=` - **LCP** (Largest Contentful Paint): the `LCP=` line (`null` if the page emitted no LCP entry — record it as missing, not 0) -- **DOM Interactive**: `domInteractive - navigationStart` -- **DOM Complete**: `domComplete - navigationStart` -- **Full Load**: `loadEventEnd - navigationStart` +- **DOM Interactive**: `domInteractive - startTime` +- **DOM Complete**: `domComplete - startTime` +- **Full Load**: `loadEventEnd - startTime` Load times jitter with the network. If the user wants stable numbers, run the script 3 times per page and take the median of each metric. @@ -335,10 +335,12 @@ Save metrics to baseline file: ``` Write to `.gstack/benchmark-reports/baselines/baseline.json`. +Also retain an immutable `{UTC-timestamp}-baseline.json` beside it for trends. Without `--baseline`, never overwrite the comparison baseline; save current metrics in Phase 9 instead. ### Phase 5: Comparison If baseline exists, compare current metrics against it: +Without a baseline, report absolute measurements and budgets only, mark comparison unavailable, and recommend a `--baseline` run. Missing metrics remain N/A. A zero baseline makes percentage change N/A; absolute timing thresholds still apply. ``` PERFORMANCE REPORT — [url] @@ -353,25 +355,27 @@ TTFB 120ms 135ms +15ms OK FCP 450ms 480ms +30ms OK LCP 800ms 1600ms +800ms REGRESSION DOM Interactive 600ms 650ms +50ms OK -DOM Complete 1200ms 1350ms +150ms WARNING +DOM Complete 1200ms 1350ms +150ms OK Full Load 1400ms 2100ms +700ms REGRESSION Total Requests 42 58 +16 WARNING Transfer Size 1.2MB 1.8MB +0.6MB REGRESSION JS Bundle 450KB 720KB +270KB REGRESSION CSS Bundle 85KB 88KB +3KB OK -REGRESSIONS DETECTED: 3 +REGRESSIONS DETECTED: 4 [1] LCP doubled (800ms → 1600ms) — likely a large new image or blocking resource [2] Total transfer +50% (1.2MB → 1.8MB) — check new JS bundles [3] JS bundle +60% (450KB → 720KB) — new dependency or missing tree-shaking + [4] Full load +700ms (1400ms → 2100ms) — inspect the slowest resources ``` **Regression thresholds:** - Timing metrics: >50% increase OR >500ms absolute increase = REGRESSION - Timing metrics: >20% increase = WARNING -- Bundle size: >25% increase = REGRESSION -- Bundle size: >10% increase = WARNING -- Request count: >30% increase = WARNING +- Bundle size and total transfer: >25% increase = REGRESSION +- Bundle size and total transfer: >10% increase = WARNING +- Request count: >30% increase = WARNING (no separate regression threshold) +Apply REGRESSION before WARNING; otherwise OK. Negative deltas are improvements. ### Phase 6: Slowest Resources @@ -395,6 +399,7 @@ RECOMMENDATIONS: ### Phase 7: Performance Budget Check against industry budgets: +For each available metric, FAIL at or above the budget, WARNING from 90% to below 100%, otherwise PASS. Missing metrics are N/A and excluded. Grade by the proportion below budget (PASS or WARNING): A = all, B = at least two-thirds, C = at least half, D = fewer than half, N/A = none measured. ``` PERFORMANCE BUDGET CHECK diff --git a/benchmark/SKILL.md.tmpl b/benchmark/SKILL.md.tmpl index 4a524a5d8..61c79aa94 100644 --- a/benchmark/SKILL.md.tmpl +++ b/benchmark/SKILL.md.tmpl @@ -86,13 +86,13 @@ await closeTab(pg); console.log("GSTACK_STEP_OK"); `NAV=` is the navigation timing entry, `PAINT=` the paint entries (FCP lives here), `LCP=` the largest-contentful-paint start time (`null` if the page emitted no LCP entry within 3s), `RESOURCES=` the 15 slowest resources, `SCRIPTS=` / `CSS=` the bundle inventory, `SUMMARY=` request count, total transfer, and requests by type. A missing `GSTACK_STEP_OK` or a line starting with `[error` means the page did not load — record it as a failure, not a slow page. -Extract key metrics from `NAV=`: +Extract key metrics from the labelled lines (`NAV=` unless stated otherwise): - **TTFB** (Time to First Byte): `responseStart - requestStart` - **FCP** (First Contentful Paint): the `first-contentful-paint` entry in `PAINT=` - **LCP** (Largest Contentful Paint): the `LCP=` line (`null` if the page emitted no LCP entry — record it as missing, not 0) -- **DOM Interactive**: `domInteractive - navigationStart` -- **DOM Complete**: `domComplete - navigationStart` -- **Full Load**: `loadEventEnd - navigationStart` +- **DOM Interactive**: `domInteractive - startTime` +- **DOM Complete**: `domComplete - startTime` +- **Full Load**: `loadEventEnd - startTime` Load times jitter with the network. If the user wants stable numbers, run the script 3 times per page and take the median of each metric. @@ -127,10 +127,12 @@ Save metrics to baseline file: ``` Write to `.gstack/benchmark-reports/baselines/baseline.json`. +Also retain an immutable `{UTC-timestamp}-baseline.json` beside it for trends. Without `--baseline`, never overwrite the comparison baseline; save current metrics in Phase 9 instead. ### Phase 5: Comparison If baseline exists, compare current metrics against it: +Without a baseline, report absolute measurements and budgets only, mark comparison unavailable, and recommend a `--baseline` run. Missing metrics remain N/A. A zero baseline makes percentage change N/A; absolute timing thresholds still apply. ``` PERFORMANCE REPORT — [url] @@ -145,25 +147,27 @@ TTFB 120ms 135ms +15ms OK FCP 450ms 480ms +30ms OK LCP 800ms 1600ms +800ms REGRESSION DOM Interactive 600ms 650ms +50ms OK -DOM Complete 1200ms 1350ms +150ms WARNING +DOM Complete 1200ms 1350ms +150ms OK Full Load 1400ms 2100ms +700ms REGRESSION Total Requests 42 58 +16 WARNING Transfer Size 1.2MB 1.8MB +0.6MB REGRESSION JS Bundle 450KB 720KB +270KB REGRESSION CSS Bundle 85KB 88KB +3KB OK -REGRESSIONS DETECTED: 3 +REGRESSIONS DETECTED: 4 [1] LCP doubled (800ms → 1600ms) — likely a large new image or blocking resource [2] Total transfer +50% (1.2MB → 1.8MB) — check new JS bundles [3] JS bundle +60% (450KB → 720KB) — new dependency or missing tree-shaking + [4] Full load +700ms (1400ms → 2100ms) — inspect the slowest resources ``` **Regression thresholds:** - Timing metrics: >50% increase OR >500ms absolute increase = REGRESSION - Timing metrics: >20% increase = WARNING -- Bundle size: >25% increase = REGRESSION -- Bundle size: >10% increase = WARNING -- Request count: >30% increase = WARNING +- Bundle size and total transfer: >25% increase = REGRESSION +- Bundle size and total transfer: >10% increase = WARNING +- Request count: >30% increase = WARNING (no separate regression threshold) +Apply REGRESSION before WARNING; otherwise OK. Negative deltas are improvements. ### Phase 6: Slowest Resources @@ -187,6 +191,7 @@ RECOMMENDATIONS: ### Phase 7: Performance Budget Check against industry budgets: +For each available metric, FAIL at or above the budget, WARNING from 90% to below 100%, otherwise PASS. Missing metrics are N/A and excluded. Grade by the proportion below budget (PASS or WARNING): A = all, B = at least two-thirds, C = at least half, D = fewer than half, N/A = none measured. ``` PERFORMANCE BUDGET CHECK diff --git a/canary/SKILL.md b/canary/SKILL.md index 60b5d55b8..60d919afe 100644 --- a/canary/SKILL.md +++ b/canary/SKILL.md @@ -570,6 +570,7 @@ console.log("GSTACK_STEP_OK"); Then copy the screenshot out of the printed session directory: `cp "/.jpg" .gstack/canary-reports/baselines/.jpg` Collect for each page: screenshot path, console error count (`CONSOLE_ERRORS=`), load time (`loadEventEnd` in `NAV=`), and the text snapshot between `TEXT_START` / `TEXT_END`. +Also run Phase 3's read-only link check for each monitored page and retain the URLs whose `LINK` status is 404. Repeat the same check each monitoring round; other HEAD failures are unknown, not broken links. Compare console messages by identity, not just count, and retain the text snapshot for evidence when a page's content disappears. Save the baseline manifest to `.gstack/canary-reports/baseline.json`: @@ -582,7 +583,10 @@ Save the baseline manifest to `.gstack/canary-reports/baseline.json`: "/": { "screenshot": "baselines/home.jpg", "console_errors": 0, - "load_time_ms": 450 + "console_error_messages": [], + "load_time_ms": 450, + "broken_links": [], + "text_snapshot": "" } } } @@ -620,11 +624,12 @@ For each page to monitor: Run the Phase 2 read script for each page with the screenshot saved as `pre-.jpg`, then `cp "/pre-.jpg" .gstack/canary-reports/screenshots/`. -Record the console error count and load time for each page. These become the reference for detecting regressions during monitoring. +Save the same manifest schema as Phase 2 to `.gstack/canary-reports/pre-monitor.json`, with the screenshots' actual paths. This is a monitoring-start reference, not evidence of pre-deploy health. Use it when no baseline exists; never overwrite an existing baseline during monitoring. ### Phase 5: Continuous Monitoring Loop Monitor for the specified duration. Every 60 seconds, check each page. Nothing persists between scripts — every check re-opens the page from its URL and captures fresh evidence: +Record the start and deadline. After each full round, wait `max(0, 60 - elapsed-round-seconds)` seconds using the host's wait tool or `sleep`. If a round exceeds 60 seconds, start the next immediately and report the actual cadence; never overlap rounds. Stop at the deadline after the current round. ```bash aside repl ' @@ -655,7 +660,7 @@ After each check, compare results against the baseline (or pre-deploy snapshot): **Don't cry wolf.** Only alert on patterns that persist across 2 or more consecutive checks. A single transient network blip is not an alert. -**If a CRITICAL or HIGH alert is detected**, immediately notify the user via AskUserQuestion: +**After a CRITICAL or HIGH pattern is confirmed on two consecutive checks**, immediately notify the user via AskUserQuestion. A first occurrence is pending, not yet an alert: ``` CANARY ALERT @@ -702,6 +707,7 @@ VERDICT: [DEPLOY IS HEALTHY / DEPLOY HAS ISSUES — details above] ``` Save report to `.gstack/canary-reports/{date}-canary.md` and `.gstack/canary-reports/{date}-canary.json`. +Per-page and overall status: BROKEN if any confirmed CRITICAL alert occurred; otherwise DEGRADED if any confirmed alert occurred; otherwise HEALTHY. Note resolved incidents separately without erasing them from the run's status. JSON fields: `url`, `started_at`, `ended_at`, `status`, `pages` (URL, checks, latest metrics, status), and `alerts` (severity, URL, first_seen, confirmed_at, evidence, resolved). Unconfirmed transients go in a separate `observations` array. Log the result for the review dashboard: @@ -711,6 +717,7 @@ mkdir -p ~/.gstack/projects/$SLUG ``` Write a JSONL entry: `{"skill":"canary","timestamp":"","status":"","url":"","duration_min":,"alerts":}` +Append it to `~/.gstack/projects/$SLUG/canary-history.jsonl`; never overwrite history. ### Phase 7: Baseline Update diff --git a/canary/SKILL.md.tmpl b/canary/SKILL.md.tmpl index 90cea5fe0..daa894dd7 100644 --- a/canary/SKILL.md.tmpl +++ b/canary/SKILL.md.tmpl @@ -82,6 +82,7 @@ console.log("GSTACK_STEP_OK"); Then copy the screenshot out of the printed session directory: `cp "/.jpg" .gstack/canary-reports/baselines/.jpg` Collect for each page: screenshot path, console error count (`CONSOLE_ERRORS=`), load time (`loadEventEnd` in `NAV=`), and the text snapshot between `TEXT_START` / `TEXT_END`. +Also run Phase 3's read-only link check for each monitored page and retain the URLs whose `LINK` status is 404. Repeat the same check each monitoring round; other HEAD failures are unknown, not broken links. Compare console messages by identity, not just count, and retain the text snapshot for evidence when a page's content disappears. Save the baseline manifest to `.gstack/canary-reports/baseline.json`: @@ -94,7 +95,10 @@ Save the baseline manifest to `.gstack/canary-reports/baseline.json`: "/": { "screenshot": "baselines/home.jpg", "console_errors": 0, - "load_time_ms": 450 + "console_error_messages": [], + "load_time_ms": 450, + "broken_links": [], + "text_snapshot": "" } } } @@ -132,11 +136,12 @@ For each page to monitor: Run the Phase 2 read script for each page with the screenshot saved as `pre-.jpg`, then `cp "/pre-.jpg" .gstack/canary-reports/screenshots/`. -Record the console error count and load time for each page. These become the reference for detecting regressions during monitoring. +Save the same manifest schema as Phase 2 to `.gstack/canary-reports/pre-monitor.json`, with the screenshots' actual paths. This is a monitoring-start reference, not evidence of pre-deploy health. Use it when no baseline exists; never overwrite an existing baseline during monitoring. ### Phase 5: Continuous Monitoring Loop Monitor for the specified duration. Every 60 seconds, check each page. Nothing persists between scripts — every check re-opens the page from its URL and captures fresh evidence: +Record the start and deadline. After each full round, wait `max(0, 60 - elapsed-round-seconds)` seconds using the host's wait tool or `sleep`. If a round exceeds 60 seconds, start the next immediately and report the actual cadence; never overlap rounds. Stop at the deadline after the current round. ```bash aside repl ' @@ -167,7 +172,7 @@ After each check, compare results against the baseline (or pre-deploy snapshot): **Don't cry wolf.** Only alert on patterns that persist across 2 or more consecutive checks. A single transient network blip is not an alert. -**If a CRITICAL or HIGH alert is detected**, immediately notify the user via AskUserQuestion: +**After a CRITICAL or HIGH pattern is confirmed on two consecutive checks**, immediately notify the user via AskUserQuestion. A first occurrence is pending, not yet an alert: ``` CANARY ALERT @@ -214,6 +219,7 @@ VERDICT: [DEPLOY IS HEALTHY / DEPLOY HAS ISSUES — details above] ``` Save report to `.gstack/canary-reports/{date}-canary.md` and `.gstack/canary-reports/{date}-canary.json`. +Per-page and overall status: BROKEN if any confirmed CRITICAL alert occurred; otherwise DEGRADED if any confirmed alert occurred; otherwise HEALTHY. Note resolved incidents separately without erasing them from the run's status. JSON fields: `url`, `started_at`, `ended_at`, `status`, `pages` (URL, checks, latest metrics, status), and `alerts` (severity, URL, first_seen, confirmed_at, evidence, resolved). Unconfirmed transients go in a separate `observations` array. Log the result for the review dashboard: @@ -223,6 +229,7 @@ mkdir -p ~/.gstack/projects/$SLUG ``` Write a JSONL entry: `{"skill":"canary","timestamp":"","status":"","url":"","duration_min":,"alerts":}` +Append it to `~/.gstack/projects/$SLUG/canary-history.jsonl`; never overwrite history. ### Phase 7: Baseline Update diff --git a/design-review/SKILL.md b/design-review/SKILL.md index 5b782faa8..232ba4a8c 100644 --- a/design-review/SKILL.md +++ b/design-review/SKILL.md @@ -1421,7 +1421,7 @@ Tie everything to user goals and product objectives. Always suggest specific imp - ALWAYS preserve visited vs unvisited link distinction (visited links must have a different color) - NEVER float headings between paragraphs (heading must be visually closer to the section it introduces than to the preceding section) -**AI Slop blacklist** (the 10 patterns that scream "AI-generated"): +**AI Slop blacklist** (the 11 patterns that scream "AI-generated"): 1. Purple/violet/indigo gradient backgrounds or blue-to-purple color schemes 2. **The 3-column feature grid:** icon-in-colored-circle + bold title + 2-line description, repeated 3x symmetrically. THE most recognizable AI layout. 3. Icons in colored circles as section decoration (SaaS starter template look) diff --git a/gstack-upgrade/SKILL.md b/gstack-upgrade/SKILL.md index 79ec181ee..8c59fa72a 100644 --- a/gstack-upgrade/SKILL.md +++ b/gstack-upgrade/SKILL.md @@ -42,7 +42,7 @@ _AUTO="" echo "AUTO_UPGRADE=$_AUTO" ``` -**If `AUTO_UPGRADE=true` or `AUTO_UPGRADE=1`:** Skip AskUserQuestion. Log "Auto-upgrading gstack v{old} → v{new}..." and proceed directly to Step 2. If `./setup` fails during auto-upgrade, restore from backup (`.bak` directory) and warn the user: "Auto-upgrade failed — restored previous version. Run `/gstack-upgrade` manually to retry." +**If `AUTO_UPGRADE=true` or `AUTO_UPGRADE=1`:** Skip AskUserQuestion. Log "Auto-upgrading gstack v{old} → v{new}..." and proceed directly to Step 2. On setup failure, follow Step 4's install-specific recovery: vendored installs restore their backup; git installs stop with the pre-upgrade commit recorded, without a destructive reset. Never claim restoration unless it actually succeeded. **Otherwise**, use AskUserQuestion: - Question: "gstack **v{new}** is available (you're on v{old}). Upgrade now?" @@ -112,6 +112,7 @@ echo "Install type: $INSTALL_TYPE at $INSTALL_DIR" ``` The install type and directory path printed above will be used in all subsequent steps. +Resolve `INSTALL_DIR` to an absolute path. Carry `INSTALL_TYPE`, `INSTALL_DIR`, `OLD_VERSION`, and later `NEW_VERSION` forward explicitly: if tool calls use fresh shells, reassign them from captured output before running a block. Do not rely on a prior call's working directory or shell variables. ### Step 3: Save old version @@ -119,6 +120,7 @@ Use the install directory from Step 2's output below: ```bash OLD_VERSION=$(cat "$INSTALL_DIR/VERSION" 2>/dev/null || echo "unknown") +echo "OLD_VERSION=$OLD_VERSION" ``` ### Step 4: Upgrade @@ -138,11 +140,18 @@ cd "$INSTALL_DIR" # ~/.gstack/render), so discarding is lossless. git checkout -- 'SKILL.md' '*/SKILL.md' '*/sections/*.md' 2>/dev/null || true git fetch origin -git pull --ff-only --autostash origin main && ./setup && echo "FF_OK" +PRE_UPGRADE_COMMIT=$(git rev-parse HEAD) +echo "PRE_UPGRADE_COMMIT=$PRE_UPGRADE_COMMIT" +if git pull --ff-only --autostash origin main; then + if ./setup; then echo "FF_OK"; else echo "SETUP_FAILED: git update succeeded; stop and inspect setup output (previous commit: $PRE_UPGRADE_COMMIT)" >&2; exit 1; fi +else + echo "FF_REFUSED" +fi ``` If the output ends with `FF_OK`, the upgrade is done — skip the fallback below entirely. +On `SETUP_FAILED`, STOP; keep user changes and report the recovery commit. There is no `.bak` on the git path. Do not enter the divergence fallback merely because setup failed. Enter it only on `FF_REFUSED`, after inspecting the pull error; network/auth failures stop for repair, not reset. **Fallback (ff-only refused — local commits or divergence).** `git reset --hard` DESTROYS things: a clean tree with unpushed local commits still loses @@ -177,10 +186,17 @@ PARENT=$(dirname "$INSTALL_DIR") [ -e "$INSTALL_DIR.bak" ] && { echo "ERROR: stale backup exists at $INSTALL_DIR.bak (from a previous failed upgrade?) — inspect it, salvage/remove it, then re-run." >&2; exit 1; } TMP_DIR=$(mktemp -d) || { echo "ERROR: mktemp failed — aborting upgrade (install untouched)." >&2; exit 1; } git clone --depth 1 https://github.com/garrytan/gstack.git "$TMP_DIR/gstack" || { echo "ERROR: clone failed — aborting upgrade (install untouched)." >&2; rm -rf "$TMP_DIR"; exit 1; } -mv "$INSTALL_DIR" "$INSTALL_DIR.bak" +mv "$INSTALL_DIR" "$INSTALL_DIR.bak" || { rm -rf "$TMP_DIR"; exit 1; } if mv "$TMP_DIR/gstack" "$INSTALL_DIR"; then - cd "$INSTALL_DIR" && ./setup - rm -rf "$INSTALL_DIR.bak" "$TMP_DIR" + if (cd "$INSTALL_DIR" && ./setup); then + rm -rf "$INSTALL_DIR.bak" "$TMP_DIR" + else + rm -rf "$INSTALL_DIR" + mv "$INSTALL_DIR.bak" "$INSTALL_DIR" || { echo "ERROR: restore failed; backup retained." >&2; exit 1; } + rm -rf "$TMP_DIR" + echo "ERROR: setup failed; previous install restored." >&2 + exit 1 + fi else mv "$INSTALL_DIR.bak" "$INSTALL_DIR" echo "ERROR: swap failed — previous install restored; upgrade aborted." >&2 @@ -222,20 +238,19 @@ Tell user: "Removed vendored copy at `$LOCAL_GSTACK` (team mode active — globa **If `LOCAL_GSTACK` is non-empty AND `TEAM_MODE` is NOT `true`:** Update it by copying from the freshly-upgraded primary install (same approach as README vendored install): ```bash -mv "$LOCAL_GSTACK" "$LOCAL_GSTACK.bak" -cp -Rf "$INSTALL_DIR" "$LOCAL_GSTACK" -rm -rf "$LOCAL_GSTACK/.git" -cd "$LOCAL_GSTACK" && ./setup -rm -rf "$LOCAL_GSTACK.bak" +[ -e "$LOCAL_GSTACK.bak" ] && { echo "ERROR: stale vendored backup; inspect it before retrying." >&2; exit 1; } +mv "$LOCAL_GSTACK" "$LOCAL_GSTACK.bak" || exit 1 +if cp -Rf "$INSTALL_DIR" "$LOCAL_GSTACK" && rm -rf "$LOCAL_GSTACK/.git" && (cd "$LOCAL_GSTACK" && ./setup); then + rm -rf "$LOCAL_GSTACK.bak" + echo "LOCAL_SYNC_OK" +else + rm -rf "$LOCAL_GSTACK" + mv "$LOCAL_GSTACK.bak" "$LOCAL_GSTACK" || { echo "ERROR: restore failed; backup retained." >&2; exit 1; } + echo "ERROR: sync failed; previous vendored copy restored." >&2 + exit 1 +fi ``` -Tell user: "Also updated vendored copy at `$LOCAL_GSTACK` — commit `.claude/skills/gstack/` when you're ready." - -If `./setup` fails, restore from backup and warn the user: -```bash -rm -rf "$LOCAL_GSTACK" -mv "$LOCAL_GSTACK.bak" "$LOCAL_GSTACK" -``` -Tell user: "Sync failed — restored previous version at `$LOCAL_GSTACK`. Run `/gstack-upgrade` manually to retry." +Only on `LOCAL_SYNC_OK`, tell user: "Also updated vendored copy at `$LOCAL_GSTACK` — commit `.claude/skills/gstack/` when you're ready." Otherwise stop and report the recovery outcome; do not continue migrations or announce success. ### Step 4.75: Run version migrations diff --git a/gstack-upgrade/SKILL.md.tmpl b/gstack-upgrade/SKILL.md.tmpl index 158f3e052..84fd586e5 100644 --- a/gstack-upgrade/SKILL.md.tmpl +++ b/gstack-upgrade/SKILL.md.tmpl @@ -39,7 +39,7 @@ _AUTO="" echo "AUTO_UPGRADE=$_AUTO" ``` -**If `AUTO_UPGRADE=true` or `AUTO_UPGRADE=1`:** Skip AskUserQuestion. Log "Auto-upgrading gstack v{old} → v{new}..." and proceed directly to Step 2. If `./setup` fails during auto-upgrade, restore from backup (`.bak` directory) and warn the user: "Auto-upgrade failed — restored previous version. Run `/gstack-upgrade` manually to retry." +**If `AUTO_UPGRADE=true` or `AUTO_UPGRADE=1`:** Skip AskUserQuestion. Log "Auto-upgrading gstack v{old} → v{new}..." and proceed directly to Step 2. On setup failure, follow Step 4's install-specific recovery: vendored installs restore their backup; git installs stop with the pre-upgrade commit recorded, without a destructive reset. Never claim restoration unless it actually succeeded. **Otherwise**, use AskUserQuestion: - Question: "gstack **v{new}** is available (you're on v{old}). Upgrade now?" @@ -109,6 +109,7 @@ echo "Install type: $INSTALL_TYPE at $INSTALL_DIR" ``` The install type and directory path printed above will be used in all subsequent steps. +Resolve `INSTALL_DIR` to an absolute path. Carry `INSTALL_TYPE`, `INSTALL_DIR`, `OLD_VERSION`, and later `NEW_VERSION` forward explicitly: if tool calls use fresh shells, reassign them from captured output before running a block. Do not rely on a prior call's working directory or shell variables. ### Step 3: Save old version @@ -116,6 +117,7 @@ Use the install directory from Step 2's output below: ```bash OLD_VERSION=$(cat "$INSTALL_DIR/VERSION" 2>/dev/null || echo "unknown") +echo "OLD_VERSION=$OLD_VERSION" ``` ### Step 4: Upgrade @@ -135,11 +137,18 @@ cd "$INSTALL_DIR" # ~/.gstack/render), so discarding is lossless. git checkout -- 'SKILL.md' '*/SKILL.md' '*/sections/*.md' 2>/dev/null || true git fetch origin -git pull --ff-only --autostash origin main && ./setup && echo "FF_OK" +PRE_UPGRADE_COMMIT=$(git rev-parse HEAD) +echo "PRE_UPGRADE_COMMIT=$PRE_UPGRADE_COMMIT" +if git pull --ff-only --autostash origin main; then + if ./setup; then echo "FF_OK"; else echo "SETUP_FAILED: git update succeeded; stop and inspect setup output (previous commit: $PRE_UPGRADE_COMMIT)" >&2; exit 1; fi +else + echo "FF_REFUSED" +fi ``` If the output ends with `FF_OK`, the upgrade is done — skip the fallback below entirely. +On `SETUP_FAILED`, STOP; keep user changes and report the recovery commit. There is no `.bak` on the git path. Do not enter the divergence fallback merely because setup failed. Enter it only on `FF_REFUSED`, after inspecting the pull error; network/auth failures stop for repair, not reset. **Fallback (ff-only refused — local commits or divergence).** `git reset --hard` DESTROYS things: a clean tree with unpushed local commits still loses @@ -174,10 +183,17 @@ PARENT=$(dirname "$INSTALL_DIR") [ -e "$INSTALL_DIR.bak" ] && { echo "ERROR: stale backup exists at $INSTALL_DIR.bak (from a previous failed upgrade?) — inspect it, salvage/remove it, then re-run." >&2; exit 1; } TMP_DIR=$(mktemp -d) || { echo "ERROR: mktemp failed — aborting upgrade (install untouched)." >&2; exit 1; } git clone --depth 1 https://github.com/garrytan/gstack.git "$TMP_DIR/gstack" || { echo "ERROR: clone failed — aborting upgrade (install untouched)." >&2; rm -rf "$TMP_DIR"; exit 1; } -mv "$INSTALL_DIR" "$INSTALL_DIR.bak" +mv "$INSTALL_DIR" "$INSTALL_DIR.bak" || { rm -rf "$TMP_DIR"; exit 1; } if mv "$TMP_DIR/gstack" "$INSTALL_DIR"; then - cd "$INSTALL_DIR" && {{SETUP_COMMAND}} - rm -rf "$INSTALL_DIR.bak" "$TMP_DIR" + if (cd "$INSTALL_DIR" && {{SETUP_COMMAND}}); then + rm -rf "$INSTALL_DIR.bak" "$TMP_DIR" + else + rm -rf "$INSTALL_DIR" + mv "$INSTALL_DIR.bak" "$INSTALL_DIR" || { echo "ERROR: restore failed; backup retained." >&2; exit 1; } + rm -rf "$TMP_DIR" + echo "ERROR: setup failed; previous install restored." >&2 + exit 1 + fi else mv "$INSTALL_DIR.bak" "$INSTALL_DIR" echo "ERROR: swap failed — previous install restored; upgrade aborted." >&2 @@ -219,20 +235,19 @@ Tell user: "Removed vendored copy at `$LOCAL_GSTACK` (team mode active — globa **If `LOCAL_GSTACK` is non-empty AND `TEAM_MODE` is NOT `true`:** Update it by copying from the freshly-upgraded primary install (same approach as README vendored install): ```bash -mv "$LOCAL_GSTACK" "$LOCAL_GSTACK.bak" -cp -Rf "$INSTALL_DIR" "$LOCAL_GSTACK" -rm -rf "$LOCAL_GSTACK/.git" -cd "$LOCAL_GSTACK" && {{SETUP_COMMAND}} -rm -rf "$LOCAL_GSTACK.bak" +[ -e "$LOCAL_GSTACK.bak" ] && { echo "ERROR: stale vendored backup; inspect it before retrying." >&2; exit 1; } +mv "$LOCAL_GSTACK" "$LOCAL_GSTACK.bak" || exit 1 +if cp -Rf "$INSTALL_DIR" "$LOCAL_GSTACK" && rm -rf "$LOCAL_GSTACK/.git" && (cd "$LOCAL_GSTACK" && {{SETUP_COMMAND}}); then + rm -rf "$LOCAL_GSTACK.bak" + echo "LOCAL_SYNC_OK" +else + rm -rf "$LOCAL_GSTACK" + mv "$LOCAL_GSTACK.bak" "$LOCAL_GSTACK" || { echo "ERROR: restore failed; backup retained." >&2; exit 1; } + echo "ERROR: sync failed; previous vendored copy restored." >&2 + exit 1 +fi ``` -Tell user: "Also updated vendored copy at `$LOCAL_GSTACK` — commit `.claude/skills/gstack/` when you're ready." - -If `./setup` fails, restore from backup and warn the user: -```bash -rm -rf "$LOCAL_GSTACK" -mv "$LOCAL_GSTACK.bak" "$LOCAL_GSTACK" -``` -Tell user: "Sync failed — restored previous version at `$LOCAL_GSTACK`. Run `/gstack-upgrade` manually to retry." +Only on `LOCAL_SYNC_OK`, tell user: "Also updated vendored copy at `$LOCAL_GSTACK` — commit `.claude/skills/gstack/` when you're ready." Otherwise stop and report the recovery outcome; do not continue migrations or announce success. ### Step 4.75: Run version migrations diff --git a/land-and-deploy/SKILL.md b/land-and-deploy/SKILL.md index 76373b298..5a16096a7 100644 --- a/land-and-deploy/SKILL.md +++ b/land-and-deploy/SKILL.md @@ -710,7 +710,7 @@ gh pr checks --json name,state,status,conclusion Parse the output: 1. If any required checks are **FAILING**: **STOP.** "CI is failing on this PR. Here are the failing checks: {list}. Fix these before deploying — I won't merge code that hasn't passed CI." 2. If required checks are **PENDING**: Tell the user "CI is still running. I'll wait for it to finish." Proceed to Step 3. -3. If all checks pass (or no required checks): Tell the user "CI passed." Skip Step 3, go to Step 4. +3. If all checks pass (or no required checks): Tell the user "CI passed." Skip only Step 3's wait loop; continue to Step 3.4, then Step 3.5 before merging. Also check for merge conflicts: ```bash @@ -730,7 +730,7 @@ gh pr checks --watch --fail-fast Record the CI wait time for the deploy report. -If CI passes within the timeout: Tell the user "CI passed after {duration}. Moving to readiness checks." Continue to Step 4. +If CI passes within the timeout: Tell the user "CI passed after {duration}. Moving to readiness checks." Continue to Step 3.4, then Step 3.5 before merging. If CI fails: **STOP.** "CI failed. Here's what broke: {failures}. This needs to pass before I can merge." If timeout (15 min): **STOP.** "CI has been running for over 15 minutes — that's unusual. Check the GitHub Actions tab to see if something is stuck." @@ -936,7 +936,7 @@ git push origin If the revert has conflicts: "The revert has merge conflicts — this can happen if other changes landed on {base} after your merge. You'll need to resolve the conflicts manually. The merge commit SHA is `` — run `git revert ` to try again." If the base branch has push protections: "This repo has branch protections, so I can't push the revert directly. I'll create a revert PR instead — merge it to roll back." -Then create a revert PR: `gh pr create --title 'revert: '` +Keep the local revert commit. Create a new branch at that commit (`git switch -c "revert/pr--"`), push it with `git push -u origin HEAD`, then create the revert PR with `gh pr create --base --title 'revert: '`. Report rollback as pending until this PR merges and deploys, not REVERTED. After a successful revert: Tell the user "Revert pushed to {base}. The deploy should roll back automatically once CI passes. Keep an eye on the site to confirm." Note the revert commit SHA and continue to Step 9 with status REVERTED. diff --git a/land-and-deploy/SKILL.md.tmpl b/land-and-deploy/SKILL.md.tmpl index f91d530de..65c2a04b7 100644 --- a/land-and-deploy/SKILL.md.tmpl +++ b/land-and-deploy/SKILL.md.tmpl @@ -159,7 +159,7 @@ gh pr checks --json name,state,status,conclusion Parse the output: 1. If any required checks are **FAILING**: **STOP.** "CI is failing on this PR. Here are the failing checks: {list}. Fix these before deploying — I won't merge code that hasn't passed CI." 2. If required checks are **PENDING**: Tell the user "CI is still running. I'll wait for it to finish." Proceed to Step 3. -3. If all checks pass (or no required checks): Tell the user "CI passed." Skip Step 3, go to Step 4. +3. If all checks pass (or no required checks): Tell the user "CI passed." Skip only Step 3's wait loop; continue to Step 3.4, then Step 3.5 before merging. Also check for merge conflicts: ```bash @@ -179,7 +179,7 @@ gh pr checks --watch --fail-fast Record the CI wait time for the deploy report. -If CI passes within the timeout: Tell the user "CI passed after {duration}. Moving to readiness checks." Continue to Step 4. +If CI passes within the timeout: Tell the user "CI passed after {duration}. Moving to readiness checks." Continue to Step 3.4, then Step 3.5 before merging. If CI fails: **STOP.** "CI failed. Here's what broke: {failures}. This needs to pass before I can merge." If timeout (15 min): **STOP.** "CI has been running for over 15 minutes — that's unusual. Check the GitHub Actions tab to see if something is stuck." @@ -383,7 +383,7 @@ git push origin If the revert has conflicts: "The revert has merge conflicts — this can happen if other changes landed on {base} after your merge. You'll need to resolve the conflicts manually. The merge commit SHA is `` — run `git revert ` to try again." If the base branch has push protections: "This repo has branch protections, so I can't push the revert directly. I'll create a revert PR instead — merge it to roll back." -Then create a revert PR: `gh pr create --title 'revert: '` +Keep the local revert commit. Create a new branch at that commit (`git switch -c "revert/pr--"`), push it with `git push -u origin HEAD`, then create the revert PR with `gh pr create --base --title 'revert: '`. Report rollback as pending until this PR merges and deploys, not REVERTED. After a successful revert: Tell the user "Revert pushed to {base}. The deploy should roll back automatically once CI passes. Keep an eye on the site to confirm." Note the revert commit SHA and continue to Step 9 with status REVERTED. diff --git a/land-and-deploy/sections/merge-and-deploy.md b/land-and-deploy/sections/merge-and-deploy.md index 5259dea94..4ae5ee426 100644 --- a/land-and-deploy/sections/merge-and-deploy.md +++ b/land-and-deploy/sections/merge-and-deploy.md @@ -7,8 +7,10 @@ Record the start timestamp for timing data. Also record which merge path is take Try auto-merge first (respects repo merge settings and merge queues): +Resolve `MERGE_METHOD` from Deploy Configuration, checking GitHub's allowed methods via `gh api repos/{owner}/{repo} --jq '{squash: .allow_squash_merge, merge: .allow_merge_commit, rebase: .allow_rebase_merge}'`. With no configured method, prefer squash, then merge, then rebase among allowed methods. If a configured method is disallowed or no method is allowed, stop and ask. Set `MERGE_FLAG` to exactly `--squash`, `--merge`, or `--rebase` accordingly. + ```bash -gh pr merge --squash --auto --delete-branch +gh pr merge "$MERGE_FLAG" --auto --delete-branch ``` If `--auto` succeeds: record `MERGE_PATH=auto`. This means the repo has auto-merge enabled @@ -29,16 +31,16 @@ the flow is unaffected — but do not report the second one as "auto-merge is di before this step runs. ```bash -gh pr merge --squash --delete-branch +gh pr merge "$MERGE_FLAG" --delete-branch ``` If direct merge succeeds: record `MERGE_PATH=direct`. Tell the user: "PR merged successfully. The branch has been cleaned up." -If the merge fails with a permission error: **STOP.** "I don't have permission to merge this PR. You'll need a maintainer to merge it, or check your repo's branch protection rules." +On any failure, run the state check below first. Only if it confirms the PR is still OPEN with no auto-merge request should a permission error stop the workflow. ### 4a-postfail: Post-failure PR-state check -**Universal invariant:** after ANY non-zero exit from `gh pr merge`, query authoritative PR state before retrying or stopping. Do NOT retry `gh pr merge`. Related: cli/cli#3442, cli/cli#13380. +**Universal invariant:** after ANY non-zero exit from `gh pr merge`, query authoritative PR state before retrying or stopping. Do NOT retry blindly. The only permitted retry is the one direct attempt described above, after readback confirms OPEN with no auto-merge request and the original error is one of the two documented auto-merge rejections. All other failures use the branches below. Related: cli/cli#3442, cli/cli#13380. ```bash gh pr view --json state,mergeCommit,mergedAt,mergedBy @@ -98,7 +100,7 @@ Three outcomes — never read a failed check as a clean branch: - **Exit 0, one ref line** — the branch survived: the failed merge command never reached its `--delete-branch` half. If `` is the BASE repository, OFFER deletion, confirm-first (matching the worktree-cleanup posture above): "The remote branch `` still exists in `` — the failed merge never ran its --delete-branch half. Delete it?" Only on confirmation: `git push "https://github.com/.git" --delete ""`. If `` is a FORK, do not offer deletion — the branch belongs to the contributor and the maintainer typically has no push rights there; report instead: "The branch lives on the contributor's fork `` — leaving it to them." If a local branch of the same name exists, offer `git branch -d ""` alongside (`-d`, never `-D` — a non-fast-forwarded local branch is the user's call). - **Non-zero exit** — the check ITSELF failed (network, auth). Tell the user: "Couldn't verify remote branch state — leaving it alone." and skip the deletion offer entirely; a failed check is unknown state, not a clean branch. -Record `MERGE_PATH=direct`, then continue to §4a (CI auto-deploy detection). +Record `MERGE_PATH=direct`, then continue to §4b (CI auto-deploy detection). **If `state == "OPEN"`:** diff --git a/land-and-deploy/sections/merge-and-deploy.md.tmpl b/land-and-deploy/sections/merge-and-deploy.md.tmpl index 22841505a..d951bdb39 100644 --- a/land-and-deploy/sections/merge-and-deploy.md.tmpl +++ b/land-and-deploy/sections/merge-and-deploy.md.tmpl @@ -5,8 +5,10 @@ Record the start timestamp for timing data. Also record which merge path is take Try auto-merge first (respects repo merge settings and merge queues): +Resolve `MERGE_METHOD` from Deploy Configuration, checking GitHub's allowed methods via `gh api repos/{owner}/{repo} --jq '{squash: .allow_squash_merge, merge: .allow_merge_commit, rebase: .allow_rebase_merge}'`. With no configured method, prefer squash, then merge, then rebase among allowed methods. If a configured method is disallowed or no method is allowed, stop and ask. Set `MERGE_FLAG` to exactly `--squash`, `--merge`, or `--rebase` accordingly. + ```bash -gh pr merge --squash --auto --delete-branch +gh pr merge "$MERGE_FLAG" --auto --delete-branch ``` If `--auto` succeeds: record `MERGE_PATH=auto`. This means the repo has auto-merge enabled @@ -27,16 +29,16 @@ the flow is unaffected — but do not report the second one as "auto-merge is di before this step runs. ```bash -gh pr merge --squash --delete-branch +gh pr merge "$MERGE_FLAG" --delete-branch ``` If direct merge succeeds: record `MERGE_PATH=direct`. Tell the user: "PR merged successfully. The branch has been cleaned up." -If the merge fails with a permission error: **STOP.** "I don't have permission to merge this PR. You'll need a maintainer to merge it, or check your repo's branch protection rules." +On any failure, run the state check below first. Only if it confirms the PR is still OPEN with no auto-merge request should a permission error stop the workflow. ### 4a-postfail: Post-failure PR-state check -**Universal invariant:** after ANY non-zero exit from `gh pr merge`, query authoritative PR state before retrying or stopping. Do NOT retry `gh pr merge`. Related: cli/cli#3442, cli/cli#13380. +**Universal invariant:** after ANY non-zero exit from `gh pr merge`, query authoritative PR state before retrying or stopping. Do NOT retry blindly. The only permitted retry is the one direct attempt described above, after readback confirms OPEN with no auto-merge request and the original error is one of the two documented auto-merge rejections. All other failures use the branches below. Related: cli/cli#3442, cli/cli#13380. ```bash gh pr view --json state,mergeCommit,mergedAt,mergedBy @@ -96,7 +98,7 @@ Three outcomes — never read a failed check as a clean branch: - **Exit 0, one ref line** — the branch survived: the failed merge command never reached its `--delete-branch` half. If `` is the BASE repository, OFFER deletion, confirm-first (matching the worktree-cleanup posture above): "The remote branch `` still exists in `` — the failed merge never ran its --delete-branch half. Delete it?" Only on confirmation: `git push "https://github.com/.git" --delete ""`. If `` is a FORK, do not offer deletion — the branch belongs to the contributor and the maintainer typically has no push rights there; report instead: "The branch lives on the contributor's fork `` — leaving it to them." If a local branch of the same name exists, offer `git branch -d ""` alongside (`-d`, never `-D` — a non-fast-forwarded local branch is the user's call). - **Non-zero exit** — the check ITSELF failed (network, auth). Tell the user: "Couldn't verify remote branch state — leaving it alone." and skip the deletion offer entirely; a failed check is unknown state, not a clean branch. -Record `MERGE_PATH=direct`, then continue to §4a (CI auto-deploy detection). +Record `MERGE_PATH=direct`, then continue to §4b (CI auto-deploy detection). **If `state == "OPEN"`:** diff --git a/plan-ceo-review/sections/review-sections.md b/plan-ceo-review/sections/review-sections.md index 62e5ab0bc..784f16a52 100644 --- a/plan-ceo-review/sections/review-sections.md +++ b/plan-ceo-review/sections/review-sections.md @@ -306,12 +306,14 @@ Branch on the echoed `CODEX_MODE`: - **`model_unusable`** — authed but the account cannot use gstack's selected Codex model (#2477: HTTP 400 on every call). Relay the probe's HINT lines, tell the user the one-line fix (set `GSTACK_CODEX_MODEL=` or pass an explicit `-c model=...` override), and fall back to the Claude subagent path. The ~10s round trip is cached for 1h; timeouts fail open to `ready`. - **`ready`** — run the Codex pass below. -When the mode is `ready`, `not_installed`, or `not_authed`, print one line so the off-switch +On `under_codex`, no in-host substitute is defined here: skip this outside-voice section and continue to the required outputs. Do not invoke Codex again or label a self-review as independent. + +For all other non-disabled modes (`ready`, `not_installed`, `not_authed`, `broken_install`, `model_unusable`), print one line so the off-switch stays discoverable: "Running the outside voice automatically (standard step). Disable: `gstack-config set codex_reviews disabled`." -**Construct the plan review prompt** (for `ready`, `not_installed`, and `not_authed` — skip only on `disabled`). +**Construct the plan review prompt** for every remaining mode, including all Claude fallback modes (skip on `disabled` or `under_codex`). Read the plan file being reviewed (the file the user pointed this review at, or the branch -diff scope). If a CEO plan document was written in Step 0D-POST, read that too — it contains +diff scope). If a CEO plan document from an earlier `/plan-ceo-review` Step 0D-POST is available, read that too — it contains the scope decisions and vision. Construct this prompt (substitute the actual plan content — if plan content exceeds 30KB, @@ -357,7 +359,7 @@ CODEX SAYS (plan review — outside voice): - Timeout: "Codex timed out after 5 minutes." Fall back to the Claude subagent below. - Empty response: "Codex returned no response." Fall back to the Claude subagent below. -**If `CODEX_MODE: not_installed` or `not_authed` (or Codex errored at runtime):** +**If `CODEX_MODE: not_installed`, `not_authed`, `broken_install`, or `model_unusable` (or Codex errored at runtime):** Dispatch via the Agent tool with `run_in_background: false` (subagents default to background since Claude Code v2.1.198; the findings must land before the workflow continues). The subagent has fresh context and no conversation bias — but it is the SAME model family, not an outside model; weigh its agreement accordingly. Bound it the same way as Codex: cap the dispatch at a 5-minute timeout so "never blocking" diff --git a/plan-design-review/sections/review-sections.md b/plan-design-review/sections/review-sections.md index e533a3326..4338e59f4 100644 --- a/plan-design-review/sections/review-sections.md +++ b/plan-design-review/sections/review-sections.md @@ -44,6 +44,8 @@ matches a past learning, display: This makes the compounding visible. The user should see that gstack is getting smarter on their codebase over time. +**Pass protocol (1-6):** Record the initial 0-10 score. Every `FIX TO 10` below is a proposal: ask about each issue, wait for approval, then edit the plan and re-rate that pass with the reason for the score change. A declined fix remains documented and lowers the final score. Never edit first and ask afterward. Pass 7 is the unscored decision register, not a seventh rating. + ### Pass 1: Information Architecture Rate 0-10: Does the plan define what the user sees first, second, third? FIX TO 10: Add information hierarchy to the plan. Include ASCII diagram of screen/page structure and navigation flow. Apply "constraint worship" — if you can only show 3 things, which 3? @@ -74,8 +76,6 @@ Apply time-horizon design: 5-sec visceral, 5-min behavioral, 5-year reflective. **STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. ### Pass 4: AI Slop Risk -Rate 0-10: Does the plan describe specific, intentional UI — or generic patterns? -FIX TO 10: Rewrite vague UI descriptions with specific alternatives. ### Design Hard Rules @@ -136,7 +136,7 @@ FIX TO 10: Rewrite vague UI descriptions with specific alternatives. - ALWAYS preserve visited vs unvisited link distinction (visited links must have a different color) - NEVER float headings between paragraphs (heading must be visually closer to the section it introduces than to the preceding section) -**AI Slop blacklist** (the 10 patterns that scream "AI-generated"): +**AI Slop blacklist** (the 11 patterns that scream "AI-generated"): 1. Purple/violet/indigo gradient backgrounds or blue-to-purple color schemes 2. **The 3-column feature grid:** icon-in-colored-circle + bold title + 2-line description, repeated 3x symmetrically. THE most recognizable AI layout. 3. Icons in colored circles as section decoration (SaaS starter template look) @@ -150,6 +150,10 @@ FIX TO 10: Rewrite vague UI descriptions with specific alternatives. 11. system-ui or `-apple-system` as the PRIMARY display/body font — the "I gave up on typography" signal. Pick a real typeface. 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. + +**Pass 4 evaluation:** Rate 0-10: Does the plan describe specific, intentional UI, or generic patterns? Record each hard-rejection hit and litmus YES/NO with evidence. An unresolved hard rejection caps this pass below 8 (not design-complete); it does not automatically set the score to 0. Litmus answers support findings, not a separate numeric score. + +FIX TO 10: Rewrite vague UI descriptions with specific alternatives: - "Cards with icons" → what differentiates these from every SaaS template? - "Hero section" → what makes this hero feel like THIS product? - "Clean, modern UI" → meaningless. Replace with actual design decisions. @@ -179,6 +183,7 @@ Surface ambiguities that will haunt implementation: ``` If visual mockups were generated in Step 0.5, reference them as evidence when surfacing unresolved decisions. A mockup makes decisions concrete — e.g., "Your approved mockup shows a sidebar nav, but the plan doesn't specify mobile behavior. What happens to this sidebar on 375px?" Each decision = one AskUserQuestion with recommendation + WHY + alternatives. Edit the plan with each decision as it's made. +**STOP.** Wait for each answer before editing or advancing. Record unanswered decisions as unresolved. ### Post-Pass: Update Mockups (if generated) @@ -614,4 +619,3 @@ Use AskUserQuestion to present the next step. Include only applicable options: * One sentence max per option. * After each pass, pause and wait for feedback. * Rate before and after each pass for scannability. - diff --git a/plan-design-review/sections/review-sections.md.tmpl b/plan-design-review/sections/review-sections.md.tmpl index 8b9c0c4bc..9804c958a 100644 --- a/plan-design-review/sections/review-sections.md.tmpl +++ b/plan-design-review/sections/review-sections.md.tmpl @@ -6,6 +6,8 @@ {{LEARNINGS_SEARCH}} +**Pass protocol (1-6):** Record the initial 0-10 score. Every `FIX TO 10` below is a proposal: ask about each issue, wait for approval, then edit the plan and re-rate that pass with the reason for the score change. A declined fix remains documented and lowers the final score. Never edit first and ask afterward. Pass 7 is the unscored decision register, not a seventh rating. + ### Pass 1: Information Architecture Rate 0-10: Does the plan define what the user sees first, second, third? FIX TO 10: Add information hierarchy to the plan. Include ASCII diagram of screen/page structure and navigation flow. Apply "constraint worship" — if you can only show 3 things, which 3? @@ -36,10 +38,12 @@ Apply time-horizon design: 5-sec visceral, 5-min behavioral, 5-year reflective. **STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. ### Pass 4: AI Slop Risk -Rate 0-10: Does the plan describe specific, intentional UI — or generic patterns? -FIX TO 10: Rewrite vague UI descriptions with specific alternatives. {{DESIGN_HARD_RULES}} + +**Pass 4 evaluation:** Rate 0-10: Does the plan describe specific, intentional UI, or generic patterns? Record each hard-rejection hit and litmus YES/NO with evidence. An unresolved hard rejection caps this pass below 8 (not design-complete); it does not automatically set the score to 0. Litmus answers support findings, not a separate numeric score. + +FIX TO 10: Rewrite vague UI descriptions with specific alternatives: - "Cards with icons" → what differentiates these from every SaaS template? - "Hero section" → what makes this hero feel like THIS product? - "Clean, modern UI" → meaningless. Replace with actual design decisions. @@ -69,6 +73,7 @@ Surface ambiguities that will haunt implementation: ``` If visual mockups were generated in Step 0.5, reference them as evidence when surfacing unresolved decisions. A mockup makes decisions concrete — e.g., "Your approved mockup shows a sidebar nav, but the plan doesn't specify mobile behavior. What happens to this sidebar on 375px?" Each decision = one AskUserQuestion with recommendation + WHY + alternatives. Edit the plan with each decision as it's made. +**STOP.** Wait for each answer before editing or advancing. Record unanswered decisions as unresolved. ### Post-Pass: Update Mockups (if generated) @@ -220,4 +225,3 @@ Use AskUserQuestion to present the next step. Include only applicable options: * One sentence max per option. * After each pass, pause and wait for feedback. * Rate before and after each pass for scannability. - diff --git a/plan-devex-review/sections/review-sections.md b/plan-devex-review/sections/review-sections.md index f4d428b49..2ff064a2b 100644 --- a/plan-devex-review/sections/review-sections.md +++ b/plan-devex-review/sections/review-sections.md @@ -292,12 +292,14 @@ Branch on the echoed `CODEX_MODE`: - **`model_unusable`** — authed but the account cannot use gstack's selected Codex model (#2477: HTTP 400 on every call). Relay the probe's HINT lines, tell the user the one-line fix (set `GSTACK_CODEX_MODEL=` or pass an explicit `-c model=...` override), and fall back to the Claude subagent path. The ~10s round trip is cached for 1h; timeouts fail open to `ready`. - **`ready`** — run the Codex pass below. -When the mode is `ready`, `not_installed`, or `not_authed`, print one line so the off-switch +On `under_codex`, no in-host substitute is defined here: skip this outside-voice section and continue to the required outputs. Do not invoke Codex again or label a self-review as independent. + +For all other non-disabled modes (`ready`, `not_installed`, `not_authed`, `broken_install`, `model_unusable`), print one line so the off-switch stays discoverable: "Running the outside voice automatically (standard step). Disable: `gstack-config set codex_reviews disabled`." -**Construct the plan review prompt** (for `ready`, `not_installed`, and `not_authed` — skip only on `disabled`). +**Construct the plan review prompt** for every remaining mode, including all Claude fallback modes (skip on `disabled` or `under_codex`). Read the plan file being reviewed (the file the user pointed this review at, or the branch -diff scope). If a CEO plan document was written in Step 0D-POST, read that too — it contains +diff scope). If a CEO plan document from an earlier `/plan-ceo-review` Step 0D-POST is available, read that too — it contains the scope decisions and vision. Construct this prompt (substitute the actual plan content — if plan content exceeds 30KB, @@ -343,7 +345,7 @@ CODEX SAYS (plan review — outside voice): - Timeout: "Codex timed out after 5 minutes." Fall back to the Claude subagent below. - Empty response: "Codex returned no response." Fall back to the Claude subagent below. -**If `CODEX_MODE: not_installed` or `not_authed` (or Codex errored at runtime):** +**If `CODEX_MODE: not_installed`, `not_authed`, `broken_install`, or `model_unusable` (or Codex errored at runtime):** Dispatch via the Agent tool with `run_in_background: false` (subagents default to background since Claude Code v2.1.198; the findings must land before the workflow continues). The subagent has fresh context and no conversation bias — but it is the SAME model family, not an outside model; weigh its agreement accordingly. Bound it the same way as Codex: cap the dispatch at a 5-minute timeout so "never blocking" diff --git a/plan-eng-review/SKILL.md b/plan-eng-review/SKILL.md index fbb8974e9..ecf85e93d 100644 --- a/plan-eng-review/SKILL.md +++ b/plan-eng-review/SKILL.md @@ -671,7 +671,7 @@ If none was produced (user may have cancelled), proceed with standard review. Before reviewing anything, answer these questions: 1. **What existing code already partially or fully solves each sub-problem?** Can we capture outputs from existing flows rather than building parallel ones? 2. **What is the minimum set of changes that achieves the stated goal?** Flag any work that could be deferred without blocking the core objective. Be ruthless about scope creep. -3. **Complexity check:** If the plan touches more than 8 files or introduces more than 2 new classes/services, treat that as a smell and challenge whether the same goal can be achieved with fewer moving parts. +3. **Complexity check:** If the plan touches 8+ files or introduces 2+ new classes/services, treat that as a smell and challenge whether the same goal can be achieved with fewer moving parts. 4. **Search check:** For each architectural pattern, infrastructure component, or concurrency approach the plan introduces, research through Aside (Web research runs in Aside, above), one read-only request per pattern: - Does the runtime/framework have a built-in? Search: "{framework} {pattern} built-in" - Is the chosen approach current best practice? Search: "{pattern} best practice {current year}" @@ -687,9 +687,9 @@ Before reviewing anything, answer these questions: If the plan rolls a custom solution where a built-in exists, flag it as a scope reduction opportunity. Annotate recommendations with **[Layer 1]**, **[Layer 2]**, **[Layer 3]**, or **[EUREKA]** (see preamble's Search Before Building section). If you find a eureka moment — a reason the standard approach is wrong for this case — present it as an architectural insight. 5. **TODOS cross-reference:** Read `TODOS.md` if it exists. Are any deferred items blocking this plan? Can any deferred items be bundled into this PR without expanding scope? Does this plan create new work that should be captured as a TODO? -5. **Completeness check:** Is the plan doing the complete version or a shortcut? With AI-assisted coding, the cost of completeness (100% test coverage, full edge case handling, complete error paths) is 10-100x cheaper than with a human team. If the plan proposes a shortcut that saves human-hours but only saves minutes with CC+gstack, recommend the complete version. Boil the ocean. +6. **Completeness check:** Is the plan doing the complete version or a shortcut? With AI-assisted coding, the cost of completeness (100% test coverage, full edge case handling, complete error paths) is 10-100x cheaper than with a human team. If the plan proposes a shortcut that saves human-hours but only saves minutes with CC+gstack, recommend the complete version. Boil the ocean. -6. **Distribution check:** If the plan introduces a new artifact type (CLI binary, library package, container image, mobile app), does it include the build/publish pipeline? Code without distribution is code nobody can use. Check: +7. **Distribution check:** If the plan introduces a new artifact type (CLI binary, library package, container image, mobile app), does it include the build/publish pipeline? Code without distribution is code nobody can use. Check: - Is there a CI/CD workflow for building and publishing the artifact? - Are target platforms defined (linux/darwin/windows, amd64/arm64)? - How will users download or install it (GitHub Releases, package manager, container registry)? @@ -699,7 +699,7 @@ If the complexity check triggers (8+ files or 2+ new classes/services), STOP bef **STOP.** Do NOT proceed to Section 1 (Architecture review), edit the plan file with a proposed scope reduction, or call ExitPlanMode until the user responds. Naming the 80% solution in chat prose and continuing — or loading the AskUserQuestion schema via ToolSearch and then never invoking it — is the failure mode this gate exists to prevent. -If the complexity check does not trigger, present your Step 0 findings and proceed directly to Section 1. +If the complexity check does not trigger, present your Step 0 findings and enter Review Sections: run Prior Learnings and Confidence Calibration, then Section 1. Always work through the full interactive review: one section at a time (Architecture → Code Quality → Tests → Performance) with at most 8 top issues per section. diff --git a/plan-eng-review/SKILL.md.tmpl b/plan-eng-review/SKILL.md.tmpl index c01058358..f1f231b01 100644 --- a/plan-eng-review/SKILL.md.tmpl +++ b/plan-eng-review/SKILL.md.tmpl @@ -126,7 +126,7 @@ If a design doc exists, read it. Use it as the source of truth for the problem s Before reviewing anything, answer these questions: 1. **What existing code already partially or fully solves each sub-problem?** Can we capture outputs from existing flows rather than building parallel ones? 2. **What is the minimum set of changes that achieves the stated goal?** Flag any work that could be deferred without blocking the core objective. Be ruthless about scope creep. -3. **Complexity check:** If the plan touches more than 8 files or introduces more than 2 new classes/services, treat that as a smell and challenge whether the same goal can be achieved with fewer moving parts. +3. **Complexity check:** If the plan touches 8+ files or introduces 2+ new classes/services, treat that as a smell and challenge whether the same goal can be achieved with fewer moving parts. 4. **Search check:** For each architectural pattern, infrastructure component, or concurrency approach the plan introduces, research through Aside (Web research runs in Aside, above), one read-only request per pattern: - Does the runtime/framework have a built-in? Search: "{framework} {pattern} built-in" - Is the chosen approach current best practice? Search: "{pattern} best practice {current year}" @@ -142,9 +142,9 @@ Before reviewing anything, answer these questions: If the plan rolls a custom solution where a built-in exists, flag it as a scope reduction opportunity. Annotate recommendations with **[Layer 1]**, **[Layer 2]**, **[Layer 3]**, or **[EUREKA]** (see preamble's Search Before Building section). If you find a eureka moment — a reason the standard approach is wrong for this case — present it as an architectural insight. 5. **TODOS cross-reference:** Read `TODOS.md` if it exists. Are any deferred items blocking this plan? Can any deferred items be bundled into this PR without expanding scope? Does this plan create new work that should be captured as a TODO? -5. **Completeness check:** Is the plan doing the complete version or a shortcut? With AI-assisted coding, the cost of completeness (100% test coverage, full edge case handling, complete error paths) is 10-100x cheaper than with a human team. If the plan proposes a shortcut that saves human-hours but only saves minutes with CC+gstack, recommend the complete version. Boil the ocean. +6. **Completeness check:** Is the plan doing the complete version or a shortcut? With AI-assisted coding, the cost of completeness (100% test coverage, full edge case handling, complete error paths) is 10-100x cheaper than with a human team. If the plan proposes a shortcut that saves human-hours but only saves minutes with CC+gstack, recommend the complete version. Boil the ocean. -6. **Distribution check:** If the plan introduces a new artifact type (CLI binary, library package, container image, mobile app), does it include the build/publish pipeline? Code without distribution is code nobody can use. Check: +7. **Distribution check:** If the plan introduces a new artifact type (CLI binary, library package, container image, mobile app), does it include the build/publish pipeline? Code without distribution is code nobody can use. Check: - Is there a CI/CD workflow for building and publishing the artifact? - Are target platforms defined (linux/darwin/windows, amd64/arm64)? - How will users download or install it (GitHub Releases, package manager, container registry)? @@ -154,7 +154,7 @@ If the complexity check triggers (8+ files or 2+ new classes/services), STOP bef **STOP.** Do NOT proceed to Section 1 (Architecture review), edit the plan file with a proposed scope reduction, or call ExitPlanMode until the user responds. Naming the 80% solution in chat prose and continuing — or loading the AskUserQuestion schema via ToolSearch and then never invoking it — is the failure mode this gate exists to prevent. -If the complexity check does not trigger, present your Step 0 findings and proceed directly to Section 1. +If the complexity check does not trigger, present your Step 0 findings and enter Review Sections: run Prior Learnings and Confidence Calibration, then Section 1. Always work through the full interactive review: one section at a time (Architecture → Code Quality → Tests → Performance) with at most 8 top issues per section. diff --git a/plan-eng-review/sections/review-sections.md b/plan-eng-review/sections/review-sections.md index 1147665e8..0beac9457 100644 --- a/plan-eng-review/sections/review-sections.md +++ b/plan-eng-review/sections/review-sections.md @@ -44,20 +44,7 @@ matches a past learning, display: This makes the compounding visible. The user should see that gstack is getting smarter on their codebase over time. -### 1. Architecture review -Evaluate: -* Overall system design and component boundaries. -* Dependency graph and coupling concerns. -* Data flow patterns and potential bottlenecks. -* Scaling characteristics and single points of failure. -* Security architecture (auth, data access, API boundaries). -* Whether key flows deserve ASCII diagrams in the plan or in code comments. -* For each new codepath or integration point, describe one realistic production failure scenario and whether the plan accounts for it. -* **Distribution architecture:** If this introduces a new artifact (binary, package, container), how does it get built, published, and updated? Is the CI/CD pipeline part of the plan or deferred? - -For each issue found in this section, call AskUserQuestion individually. One issue per call. Present options, state your recommendation, explain WHY. Do NOT batch multiple issues into one AskUserQuestion. Use the preamble's AskUserQuestion Format section. The AskUserQuestion call is a tool_use, not prose — call the tool directly. - -**STOP.** Do NOT proceed to the next review section, edit the plan file with the proposed fix, or call ExitPlanMode until the user responds. An issue with an "obvious fix" is still an issue and still needs explicit user approval before it lands in the plan. Loading the AskUserQuestion schema via ToolSearch and then writing the recommendation as chat prose is the failure mode this gate exists to prevent. +**Plan-review evidence:** Apply the calibration gate below before Section 1. For proposed work, quote the motivating plan requirement (plan file:line); verify it against existing interfaces where applicable. Do not require nonexistent future code or describe a proposed regression as an observed one. Code-specific examples apply when critiquing existing code. Put suppressed findings in a `Suppressed findings` appendix to the review report. ## Confidence Calibration @@ -121,6 +108,21 @@ confirms it IS a real issue, that is a calibration event. Your initial confidenc too low. Log the corrected pattern as a learning so future reviews catch it with higher confidence. +### 1. Architecture review +Evaluate: +* Overall system design and component boundaries. +* Dependency graph and coupling concerns. +* Data flow patterns and potential bottlenecks. +* Scaling characteristics and single points of failure. +* Security architecture (auth, data access, API boundaries). +* Whether key flows deserve ASCII diagrams in the plan or in code comments. +* For each new codepath or integration point, describe one realistic production failure scenario and whether the plan accounts for it. +* **Distribution architecture:** If this introduces a new artifact (binary, package, container), how does it get built, published, and updated? Is the CI/CD pipeline part of the plan or deferred? + +For each issue found in this section, call AskUserQuestion individually. One issue per call. Present options, state your recommendation, explain WHY. Do NOT batch multiple issues into one AskUserQuestion. Use the preamble's AskUserQuestion Format section. The AskUserQuestion call is a tool_use, not prose — call the tool directly. + +**STOP.** Do NOT proceed to the next review section, edit the plan file with the proposed fix, or call ExitPlanMode until the user responds. An issue with an "obvious fix" is still an issue and still needs explicit user approval before it lands in the plan. Loading the AskUserQuestion schema via ToolSearch and then writing the recommendation as chat prose is the failure mode this gate exists to prevent. + ### 2. Code quality review Evaluate: * Code organization and module structure. @@ -387,12 +389,14 @@ Branch on the echoed `CODEX_MODE`: - **`model_unusable`** — authed but the account cannot use gstack's selected Codex model (#2477: HTTP 400 on every call). Relay the probe's HINT lines, tell the user the one-line fix (set `GSTACK_CODEX_MODEL=` or pass an explicit `-c model=...` override), and fall back to the Claude subagent path. The ~10s round trip is cached for 1h; timeouts fail open to `ready`. - **`ready`** — run the Codex pass below. -When the mode is `ready`, `not_installed`, or `not_authed`, print one line so the off-switch +On `under_codex`, no in-host substitute is defined here: skip this outside-voice section and continue to the required outputs. Do not invoke Codex again or label a self-review as independent. + +For all other non-disabled modes (`ready`, `not_installed`, `not_authed`, `broken_install`, `model_unusable`), print one line so the off-switch stays discoverable: "Running the outside voice automatically (standard step). Disable: `gstack-config set codex_reviews disabled`." -**Construct the plan review prompt** (for `ready`, `not_installed`, and `not_authed` — skip only on `disabled`). +**Construct the plan review prompt** for every remaining mode, including all Claude fallback modes (skip on `disabled` or `under_codex`). Read the plan file being reviewed (the file the user pointed this review at, or the branch -diff scope). If a CEO plan document was written in Step 0D-POST, read that too — it contains +diff scope). If a CEO plan document from an earlier `/plan-ceo-review` Step 0D-POST is available, read that too — it contains the scope decisions and vision. Construct this prompt (substitute the actual plan content — if plan content exceeds 30KB, @@ -438,7 +442,7 @@ CODEX SAYS (plan review — outside voice): - Timeout: "Codex timed out after 5 minutes." Fall back to the Claude subagent below. - Empty response: "Codex returned no response." Fall back to the Claude subagent below. -**If `CODEX_MODE: not_installed` or `not_authed` (or Codex errored at runtime):** +**If `CODEX_MODE: not_installed`, `not_authed`, `broken_install`, or `model_unusable` (or Codex errored at runtime):** Dispatch via the Agent tool with `run_in_background: false` (subagents default to background since Claude Code v2.1.198; the findings must land before the workflow continues). The subagent has fresh context and no conversation bias — but it is the SAME model family, not an outside model; weigh its agreement accordingly. Bound it the same way as Codex: cap the dispatch at a 5-minute timeout so "never blocking" @@ -947,4 +951,3 @@ Use AskUserQuestion with only the applicable options: ## Unresolved decisions If the user does not respond to an AskUserQuestion or interrupts to move on, note which decisions were left unresolved. At the end of the review, list these as "Unresolved decisions that may bite you later" — never silently default to an option. - diff --git a/plan-eng-review/sections/review-sections.md.tmpl b/plan-eng-review/sections/review-sections.md.tmpl index 85daa8d94..11514b460 100644 --- a/plan-eng-review/sections/review-sections.md.tmpl +++ b/plan-eng-review/sections/review-sections.md.tmpl @@ -6,6 +6,10 @@ {{LEARNINGS_SEARCH}} +**Plan-review evidence:** Apply the calibration gate below before Section 1. For proposed work, quote the motivating plan requirement (plan file:line); verify it against existing interfaces where applicable. Do not require nonexistent future code or describe a proposed regression as an observed one. Code-specific examples apply when critiquing existing code. Put suppressed findings in a `Suppressed findings` appendix to the review report. + +{{CONFIDENCE_CALIBRATION}} + ### 1. Architecture review Evaluate: * Overall system design and component boundaries. @@ -21,8 +25,6 @@ For each issue found in this section, call AskUserQuestion individually. One iss **STOP.** Do NOT proceed to the next review section, edit the plan file with the proposed fix, or call ExitPlanMode until the user responds. An issue with an "obvious fix" is still an issue and still needs explicit user approval before it lands in the plan. Loading the AskUserQuestion schema via ToolSearch and then writing the recommendation as chat prose is the failure mode this gate exists to prevent. -{{CONFIDENCE_CALIBRATION}} - ### 2. Code quality review Evaluate: * Code organization and module structure. @@ -222,4 +224,3 @@ Use AskUserQuestion with only the applicable options: ## Unresolved decisions If the user does not respond to an AskUserQuestion or interrupts to move on, note which decisions were left unresolved. At the end of the review, list these as "Unresolved decisions that may bite you later" — never silently default to an option. - diff --git a/scripts/resolvers/design.ts b/scripts/resolvers/design.ts index 31ebb36d1..4ebec6799 100644 --- a/scripts/resolvers/design.ts +++ b/scripts/resolvers/design.ts @@ -839,7 +839,7 @@ ${litmusItems} - ALWAYS preserve visited vs unvisited link distinction (visited links must have a different color) - NEVER float headings between paragraphs (heading must be visually closer to the section it introduces than to the preceding section) -**AI Slop blacklist** (the 10 patterns that scream "AI-generated"): +**AI Slop blacklist** (the ${AI_SLOP_BLACKLIST.length} patterns that scream "AI-generated"): ${slopItems} 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.`; diff --git a/scripts/resolvers/review.ts b/scripts/resolvers/review.ts index d425ed7df..b4017a8da 100644 --- a/scripts/resolvers/review.ts +++ b/scripts/resolvers/review.ts @@ -639,12 +639,14 @@ review. The user turns this off only by asking explicitly ${codexPreflight({ disabledBehavior: 'skip-all' })} -When the mode is \`ready\`, \`not_installed\`, or \`not_authed\`, print one line so the off-switch +On \`under_codex\`, no in-host substitute is defined here: skip this outside-voice section and continue to the required outputs. Do not invoke Codex again or label a self-review as independent. + +For all other non-disabled modes (\`ready\`, \`not_installed\`, \`not_authed\`, \`broken_install\`, \`model_unusable\`), print one line so the off-switch stays discoverable: "Running the outside voice automatically (standard step). Disable: \`gstack-config set codex_reviews disabled\`." -**Construct the plan review prompt** (for \`ready\`, \`not_installed\`, and \`not_authed\` — skip only on \`disabled\`). +**Construct the plan review prompt** for every remaining mode, including all Claude fallback modes (skip on \`disabled\` or \`under_codex\`). Read the plan file being reviewed (the file the user pointed this review at, or the branch -diff scope). If a CEO plan document was written in Step 0D-POST, read that too — it contains +diff scope). If a CEO plan document from an earlier \`/plan-ceo-review\` Step 0D-POST is available, read that too — it contains the scope decisions and vision. Construct this prompt (substitute the actual plan content — if plan content exceeds 30KB, @@ -690,7 +692,7 @@ CODEX SAYS (plan review — outside voice): - Timeout: "Codex timed out after 5 minutes." Fall back to the Claude subagent below. - Empty response: "Codex returned no response." Fall back to the Claude subagent below. -**If \`CODEX_MODE: not_installed\` or \`not_authed\` (or Codex errored at runtime):** +**If \`CODEX_MODE: not_installed\`, \`not_authed\`, \`broken_install\`, or \`model_unusable\` (or Codex errored at runtime):** Dispatch via the Agent tool with \`run_in_background: false\` (subagents default to background since ${CC_BACKGROUND_DEFAULT_SINCE}; the findings must land before the workflow continues). The subagent has fresh context and no conversation bias — but it is the SAME model family, not an outside model; weigh its agreement accordingly. Bound it the same way as Codex: cap the dispatch at a 5-minute timeout so "never blocking" diff --git a/setup-deploy/SKILL.md b/setup-deploy/SKILL.md index 5c0da6866..5975deae7 100644 --- a/setup-deploy/SKILL.md +++ b/setup-deploy/SKILL.md @@ -482,6 +482,7 @@ find . -maxdepth 1 -name '*.gemspec' 2>/dev/null | grep -q . && echo "PROJECT_TY ### Step 3: Platform-specific setup Based on what was detected, guide the user through platform-specific configuration. +If several platforms are detected, ask which one serves this project's production target before proceeding. Detection is a hint, not a selection. Confirm whether the project is a web app, API, CLI, or library; use detected CLI/library markers as defaults. #### Fly.io @@ -518,6 +519,7 @@ If vercel.json or .vercel detected: 2. If installed: `vercel ls --prod 2>/dev/null | head -3` 3. Vercel deploys automatically on push — preview on PR, production on merge to main 4. Set health check: the production URL from vercel project settings +Ask for the production URL if not available from the CLI, then confirm it before writing. #### Netlify @@ -526,6 +528,11 @@ If netlify.toml detected: 1. Extract site info from netlify.toml 2. Netlify deploys automatically on push 3. Set health check: the production URL +Ask for and confirm the production URL; do not infer it from a repository name. + +#### Heroku / Railway + +These markers do not identify the production app or service reliably. Keep the detected platform as a suggestion and use the Custom / Manual questions below to collect the production URL, trigger, and status check. #### GitHub Actions only @@ -562,6 +569,8 @@ Use AskUserQuestion to gather the information: ### Step 4: Write configuration +Before writing, collect fields not already confirmed: merge method (squash/merge/rebase, constrained to methods allowed by repo settings), pre-merge command or none, deploy trigger, and status/health checks. Ask only for missing values, across every platform path. If the project does not deploy, set platform/URL/workflow/status/health/trigger to `none`, retain its CLI/library project type, and skip deploy verification. Show the complete proposed configuration and obtain confirmation. + Read CLAUDE.md (or create it). Find and replace the `## Deploy Configuration` section if it exists, or append it at the end. diff --git a/setup-deploy/SKILL.md.tmpl b/setup-deploy/SKILL.md.tmpl index 5630f294b..8bf42e65e 100644 --- a/setup-deploy/SKILL.md.tmpl +++ b/setup-deploy/SKILL.md.tmpl @@ -82,6 +82,7 @@ find . -maxdepth 1 -name '*.gemspec' 2>/dev/null | grep -q . && echo "PROJECT_TY ### Step 3: Platform-specific setup Based on what was detected, guide the user through platform-specific configuration. +If several platforms are detected, ask which one serves this project's production target before proceeding. Detection is a hint, not a selection. Confirm whether the project is a web app, API, CLI, or library; use detected CLI/library markers as defaults. #### Fly.io @@ -118,6 +119,7 @@ If vercel.json or .vercel detected: 2. If installed: `vercel ls --prod 2>/dev/null | head -3` 3. Vercel deploys automatically on push — preview on PR, production on merge to main 4. Set health check: the production URL from vercel project settings +Ask for the production URL if not available from the CLI, then confirm it before writing. #### Netlify @@ -126,6 +128,11 @@ If netlify.toml detected: 1. Extract site info from netlify.toml 2. Netlify deploys automatically on push 3. Set health check: the production URL +Ask for and confirm the production URL; do not infer it from a repository name. + +#### Heroku / Railway + +These markers do not identify the production app or service reliably. Keep the detected platform as a suggestion and use the Custom / Manual questions below to collect the production URL, trigger, and status check. #### GitHub Actions only @@ -162,6 +169,8 @@ Use AskUserQuestion to gather the information: ### Step 4: Write configuration +Before writing, collect fields not already confirmed: merge method (squash/merge/rebase, constrained to methods allowed by repo settings), pre-merge command or none, deploy trigger, and status/health checks. Ask only for missing values, across every platform path. If the project does not deploy, set platform/URL/workflow/status/health/trigger to `none`, retain its CLI/library project type, and skip deploy verification. Show the complete proposed configuration and obtain confirmation. + Read CLAUDE.md (or create it). Find and replace the `## Deploy Configuration` section if it exists, or append it at the end. diff --git a/test/helpers/carve-guards.ts b/test/helpers/carve-guards.ts index 697500e53..0907c0757 100644 --- a/test/helpers/carve-guards.ts +++ b/test/helpers/carve-guards.ts @@ -471,7 +471,7 @@ export const CARVE_GUARDS: Record = { mustPrecedeStop: ['land-deploy-confirmed'], mustMoveToSection: [ 'PRE-MERGE READINESS REPORT', - 'gh pr merge --squash --auto --delete-branch', + 'gh pr merge "$MERGE_FLAG" --auto --delete-branch', 'DEPLOY INFRASTRUCTURE VALIDATION', ], gateAfterStop: undefined, // operational skill diff --git a/test/land-and-deploy-postfail.test.ts b/test/land-and-deploy-postfail.test.ts index 451aa7faa..51fe3595a 100644 --- a/test/land-and-deploy-postfail.test.ts +++ b/test/land-and-deploy-postfail.test.ts @@ -85,9 +85,9 @@ describe("PR #1620 §4a-postfail in land-and-deploy template", () => { expect(body).toMatch(/Do NOT remove the user's primary working tree/); }); - test("MERGED branch continues to §4a CI auto-deploy detection", () => { + test("MERGED branch continues to §4b CI auto-deploy detection", () => { const body = readTmpl(); - expect(body).toMatch(/continue to §4a/); + expect(body).toMatch(/continue to §4b \(CI auto-deploy detection\)/); }); // #2656: the failed merge carried --delete-branch; the recovery path must diff --git a/test/upgrade-setup-recovery.test.ts b/test/upgrade-setup-recovery.test.ts new file mode 100644 index 000000000..1d0a1a579 --- /dev/null +++ b/test/upgrade-setup-recovery.test.ts @@ -0,0 +1,64 @@ +import { describe, expect, test } from 'bun:test'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { spawnSync } from 'child_process'; + +const template = readFileSync(join(import.meta.dir, '../gstack-upgrade/SKILL.md.tmpl'), 'utf8'); +const blockAfter = (marker: string) => { + const section = template.slice(template.indexOf(marker)); + return section.match(/```bash\n([\s\S]*?)\n```/)![1].replaceAll('{{SETUP_COMMAND}}', './setup'); +}; + +describe.skipIf(process.platform === 'win32')('upgrade setup recovery (real shell)', () => { + for (const mode of ['vendored', 'local'] as const) { + for (const setupExit of [0, 1]) { + test(`${mode}: setup exit ${setupExit} ${setupExit ? 'restores old install' : 'removes backup only after success'}`, () => { + const root = mkdtempSync(join(tmpdir(), 'upgrade-recovery-')); + const target = join(root, 'target'); + const source = join(root, 'source'); + const bin = join(root, 'bin'); + try { + for (const dir of [target, source, bin]) mkdirSync(dir); + writeFileSync(join(target, 'VERSION'), 'old'); + writeFileSync(join(source, 'VERSION'), 'new'); + writeFileSync(join(source, 'setup'), '#!/bin/sh\nexit "$SETUP_EXIT"\n', { mode: 0o755 }); + writeFileSync(join(bin, 'git'), '#!/bin/sh\nfor last; do :; done\ncp -R "$UPGRADE_FIXTURE" "$last"\n', { mode: 0o755 }); + const script = blockAfter(mode === 'vendored' + ? '**For vendored installs**' + : '**If `LOCAL_GSTACK` is non-empty AND `TEAM_MODE` is NOT `true`:**'); + const result = spawnSync('bash', ['-c', script], { + cwd: root, encoding: 'utf8', timeout: 10_000, + env: { ...process.env, PATH: `${bin}:${process.env.PATH}`, INSTALL_DIR: mode === 'vendored' ? target : source, + LOCAL_GSTACK: target, UPGRADE_FIXTURE: source, SETUP_EXIT: String(setupExit) }, + }); + expect(result.status, result.stderr).toBe(setupExit); + expect(readFileSync(join(target, 'VERSION'), 'utf8')).toBe(setupExit ? 'old' : 'new'); + expect(existsSync(`${target}.bak`)).toBe(false); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + } + } + + test('git setup failure is not routed into the divergence reset fallback', () => { + const root = mkdtempSync(join(tmpdir(), 'upgrade-git-setup-')); + try { + const bin = join(root, 'bin'); + mkdirSync(bin); + writeFileSync(join(bin, 'git'), '#!/bin/sh\nif [ "$1" = rev-parse ]; then echo old-commit; fi\nexit 0\n', { mode: 0o755 }); + writeFileSync(join(root, 'setup'), '#!/bin/sh\nexit 1\n', { mode: 0o755 }); + const result = spawnSync('bash', ['-c', blockAfter('**For git installs**')], { + cwd: root, encoding: 'utf8', timeout: 10_000, + env: { ...process.env, PATH: `${bin}:${process.env.PATH}`, INSTALL_DIR: root }, + }); + expect(result.status).toBe(1); + expect(result.stderr).toContain('SETUP_FAILED'); + expect(result.stdout).not.toContain('FF_REFUSED'); + expect(result.stdout).not.toContain('FF_OK'); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); +}); diff --git a/test/workflow-excerpt.test.ts b/test/workflow-excerpt.test.ts index 40c180252..a4595fa35 100644 --- a/test/workflow-excerpt.test.ts +++ b/test/workflow-excerpt.test.ts @@ -65,11 +65,37 @@ describe('workflow judge excerpts', () => { expect(text).toContain('## Step 9:'); }); + test('plan review evidence and design approval rules precede their use', () => { + const eng = readWorkflowExcerpt('plan-eng-review/SKILL.md', '## Review Sections', '## CRITICAL RULE'); + expect(eng.indexOf('## Confidence Calibration')).toBeLessThan(eng.indexOf('### 1. Architecture review')); + expect(eng).toContain('quote the motivating plan requirement'); + expect(eng).toContain('including all Claude fallback modes'); + expect(eng).toContain('no in-host substitute is defined here'); + const design = readWorkflowExcerpt('plan-design-review/SKILL.md', '## Review Sections', '## CRITICAL RULE'); + expect(design).toContain('wait for approval, then edit the plan and re-rate'); + const pass4 = design.slice(design.indexOf('### Pass 4:'), design.indexOf('### Pass 5:')); + expect(pass4.indexOf('### Design Hard Rules')).toBeLessThan(pass4.indexOf('FIX TO 10:')); + expect(pass4).toContain('caps this pass below 8'); + }); + test('fails closed for missing excerpt markers', () => { expect(() => readWorkflowExcerpt('ship/SKILL.md', '# missing', null)).toThrow('Start marker not found'); expect(() => readWorkflowExcerpt('ship/SKILL.md', '# Ship:', '# missing')).toThrow('End marker not found'); }); + test('deploy gates and navigation timing formulas are executable as documented', () => { + const land = readFileSync(join(import.meta.dir, '../land-and-deploy/SKILL.md.tmpl'), 'utf8'); + expect(land).not.toContain('Skip Step 3, go to Step 4'); + expect(land).toContain('continue to Step 3.4, then Step 3.5 before merging'); + const benchmark = readFileSync(join(import.meta.dir, '../benchmark/SKILL.md.tmpl'), 'utf8'); + const timings = { startTime: 0, domInteractive: 600, domComplete: 1200, loadEventEnd: 1400 }; + for (const [label, expected] of [['DOM Interactive', 600], ['DOM Complete', 1200], ['Full Load', 1400]] as const) { + const formula = benchmark.match(new RegExp(`\\*\\*${label}\\*\\*: \x60([^\x60]+)\x60`))![1]; + const actual = new Function(...Object.keys(timings), `return ${formula}`)(...Object.values(timings)); + expect(actual).toBe(expected); + } + }); + test('WIP squash example consumes the prepared todo and preserves file contents', () => { const cwd = mkdtempSync(join(tmpdir(), 'ship-wip-example-')); const env = {