mirror of
https://github.com/garrytan/gstack.git
synced 2026-08-21 13:37:14 +02:00
* fix(plan-tune): reject never-ask on one-way ids at --write --check already ignored those prefs; --write still stored them and --stats counted them as a working NEVER_ASK. Refuse the write and count leftover on-disk prefs as INERT_ONE_WAY. Co-authored-by: Cursor <cursoragent@cursor.com> * Fix: gstack-config get returns "" with exit 0 for keys that have no default Skill preambles read configuration with VAR=$(gstack-config get <key> 2>/dev/null || echo "<default>") and that fallback only fires on a non-zero exit. lookup_default ended in a catch-all that echoed "" and returned 0, so for any key missing from the table VAR came back empty and the default written right there in the preamble was unreachable. The skill then branched on a value it never specified: "skip entirely if QUESTION_TUNING is false", reached with QUESTION_TUNING="". Four keys that skills actually read had no entry and took that path: question_tuning -> callers assume "false" repo_mode -> callers assume "unknown" team_mode -> callers assume "false" transcript_ingest_mode -> callers assume "off" Each default above is the value the call sites already substitute in their own `|| echo` fallback, so this only makes reachable what was already intended. The catch-all now returns non-zero. That is deliberately scoped to the unknown-key arm alone: keys whose default is intentionally empty still exit 0, because "" is their real answer and their callers depend on it -- cross_project_learnings ("unset triggers the first-time prompt"), redact_repo_visibility ("empty falls through to gh/glab detection"), salience_allowlist, user_slug_at_*. Making every empty answer an error would have broken those. test/gstack-config-defaults.test.ts pins the class rather than the four instances: it parses the case arms and asserts every `gstack-config get <key>` site in the tree is covered, so adding a read without a default fails CI. It also pins the exit-code contract in both directions. Verified failing against the pre-fix script, where it names exactly those four keys. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(redact): a typo'd subcommand no longer exits 0 having done nothing main() recognised exactly two subcommands and let everything else fall through to the stdin scan. On empty stdin that prints "(no findings)" and exits 0, so: $ gstack-redact install-prepush-hooks # plural typo gstack-redact scan — repo UNKNOWN (no findings) $ echo $? 0 No hook was installed, and the operator has every reason to believe the credential guard is armed. A guard that silently no-ops must never exit 0. Two smaller faults in the same dispatch, both of which lead people here: - There was no --help handler, so `gstack-redact --help` fell through to the scanner. Piping a credential to it scanned the secret and exited 3. - With no piped input and no --from-file, readInput() blocks on readSync(fd 0) until an EOF that an interactive terminal never sends. That prints nothing at all, so it reads as a hang rather than as "this is a filter, feed it". Now: --help/-h/help prints usage and exits 0; an unrecognised positional prints the offender and exits 1; a TTY with nothing piped in prints usage instead of blocking. "scan" stays accepted, because the human output header reads "gstack-redact scan — repo …" and that is what people type. Usage errors exit 1, deliberately not 2 or 3. Those mean MEDIUM and HIGH findings and callers gate dispatch on them, so a usage error exiting 2 would be read as "medium findings — prompt the user". A test pins that. Tests: 4 written failing first, then fixed. Full suite 7,722 pass / 0 fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(browse): one ambiguous ref no longer kills the whole annotated screenshot `snapshot -a` exits 1 with "Selector matched multiple elements" on most real pages, so /qa, /canary and /land-and-deploy silently produce reports whose screenshots do not exist. Plain `screenshot <path>` is unaffected. Refs are built as getByRole(role, {name}) and disambiguated with .nth() when role+name repeats. That disambiguation cannot fire for a node with NO accessible name: the locator degrades to getByRole(role) with no name filter, and the count driving .nth() is taken from the FILTERED aria snapshot while getByRole matches the unfiltered DOM. Measured on a live page: the tree surfaced 2 unnamed paragraphs, the DOM had 9. Landmarks (banner/main/contentinfo) and paragraphs are correctly unnamed per ARIA, so this is the common case rather than an edge case. boundingBox() then hits Playwright strict mode, and the catch allowlisted only timeout/closed/Target/Execution-context messages — so the strict-mode error was re-thrown and aborted every remaining annotation. Two changes: - `.first()` before boundingBox(), so an ambiguous ref draws a box on its first match instead of aborting. The heatmap path below has always tolerated this via a bare `catch {}`; annotate was the only path that could be killed outright. - the catch no longer re-throws on unrecognised messages. A box we cannot measure is a box we do not draw, never a reason to lose the rest of the page. Set BROWSE_DEBUG to see what was skipped. Also: `-o` passed without `-a`/`-H` was silently ignored (exit 0, no file), which reads as "screenshots are broken" rather than "you forgot a flag". It now warns and points at `browse screenshot <path>`. Verified by rebuilding both ways against the same page with 51 refs present: before — "Selector matched multiple elements", no file written after — exit 0, 229KB PNG Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(version-bump): missing or empty VERSION no longer repairs a fabricated 0.0.0.0 into package.json repair now fails with exit 2 when the VERSION file is absent or empty instead of folding to DEFAULT ("0.0.0.0") — which passed VERSION_RE and regressed package.json below where it started. classify gains an additive versionFileExists field so /ship can tell a real 0.0.0.0 from a fabricated one. Re-derived from PR #2612 under the generated-file screening rule. Fixes #2600 (repair half; the path-configurability half landed in v1.67 via #2531). Contributed by @Lockyer228 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(memory-ingest): --probe counts post-attribution, through the same gate --bulk uses probeMode previously stat'd every walked file, so setup-gbrain gated its silent bulk ingest on pre-filter counts that the write path would never ingest (#2394). The attribution decision now lives in ONE shared gate (sessionIsAttributable — cheap-parse: cwd extraction + memoized resolveGitRemote, never a full page build) used by BOTH probeMode and preparePages, so the two stages' post-attribution counts are structurally identical. ProbeReport gains skipped_unattributed; the probe prints what it excluded and --include-unattributed restores raw counts. The parity is pinned at the prepare stage (probe post-attribution == transcripts reaching import), deliberately NOT == final written. Re-derived from PR #2612 under the generated-file screening rule; the shared-gate design and the remote memo are additions from the plan review. Fixes #2394. Contributed by @Lockyer228 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(browse): allow CPU and network throttling for performance measurement Adds Emulation.setCPUThrottlingRate and Network.emulateNetworkConditions to CDP_ALLOWLIST. Motivation: diagnosing a real "uploads take 1-2 minutes" report, the only machine available was a fast developer workstation. Client-side processing measured 1.4s where the user experienced minutes, so the conclusion had to be reached arithmetically rather than observed. Throttling would have let the measurement reproduce the reporter's conditions directly. Both fit the existing posture rather than widening it: - Emulation already allows setDeviceMetricsOverride, clearDeviceMetricsOverride and setUserAgentOverride, which are equally mutating and scoped to the tab. - Neither method reads page content. setCPUThrottlingRate affects only timing; emulateNetworkConditions constrains traffic rather than inspecting it, so no request bodies, headers or cookies are exposed. Both are output: 'trusted' because they return no page-derived data. scope 'tab' for both, matching the surrounding Emulation entries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(session-update): lock pidfile records the live holder; hard TTL bounds every wedge (#2613) echo $$ inside the backgrounded subshell recorded the PARENT hook's PID — which exits immediately — so every subsequent session judged the lock stale and rm -rf'd a LIVE holder's lock, letting concurrent updaters run over each other. The pidfile now records ${BASHPID:-$(sh -c 'echo $PPID')} (macOS bash 3.2 has no BASHPID; the sh child's PPID is exactly this subshell). Staleness is now two independent detectors: PID liveness (as before, but against the real holder), and a 30-minute hard TTL on the heartbeat mtime — reclaimed regardless of kill -0, so a recycled PID or hung holder can't wedge the lock forever. The holder touches the pidfile after the pull and after setup, so a legitimately-slow run keeps itself alive. Empty and missing pidfiles are respected inside the TTL window (the mkdir→echo race) and reclaimed past it. Fixes #2613. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(browse): explicit windowsHide on every Bun.spawn site + census tripwire (#2575 residual) Bun.spawn sites were structurally outside the windowsHide census (it swept child_process bindings only). The runtime was already safe — native Bun hides consoles by default and bun-polyfill.cjs defaults windowsHide !== false since #2523/#2539 — but implicit defaults are exactly what regress silently. Every Bun.spawn/spawnSync in browse/src now carries the explicit flag (harmless on unix-only sites like Xvfb/xattr/open), and a second SWEEP in windows-spawn-hide.test.ts fails CI on any new flagless Bun.spawn site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(gbrain): brain worktree advances on the daily sync — no more silently stale brains (#2516) The daily pull refreshed only ~/.gstack itself, never the detached worktree at ~/.gstack-brain-worktree that gbrain actually indexes — so after setup the brain served stale pages forever unless setup-gbrain/sync-gbrain happened to run. brain-sync --once now advances the worktree once per 24h behind an ATTEMPT stamp (.brain-worktree-last-advance — a persistently-failing advance warns once a day, not at every skill boundary), inside the existing run lock and before any ingest step touches the worktree. The new gstack-gbrain-source-wireup --advance-only is built for the unattended cadence: git-only (no gbrain prereqs), pins every operation to the managed worktree (refuses paths that are not worktrees of the artifacts repo), refuses dirty worktrees, and never runs the force-remove recovery — a cron path must not be able to delete local changes. A static pin keeps the force-remove out. docs/gbrain-sync.md stops overclaiming the old cadence. Fixes #2516. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(memory-ingest): honor the per-remote deny/read-only trust policy (#2392) Transcript ingest now respects the same trust store as code import — the gate existed only in gstack-gbrain-sync's runCodeImport, so memory-ingest happily ingested transcripts from deny-listed repos. preparePages filters prepared transcript pages through ONE batch policy lookup (new 'get --batch' verb on bin/gstack-gbrain-repo-policy — the script owns URL normalization; the client adds repoPolicyTierBatch, one spawn for all distinct remotes, so large corpora never pay a 10s-timeout subprocess per remote). Outcomes match code-import semantics: read-only → clean skip (skipped_policy_readonly), deny → counted refusal (skipped_policy_deny), corrupted/unreadable store → HARD ERROR before any write (state, staging, egress receipt, and import all untouched) with the recovery command named — policy corruption must never read as successful ingestion. Artifacts are never policy-filtered (their git_remote is a project slug, not a remote). Fixes #2392. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(config): repo_mode keeps its empty no-default semantics (#2611 follow-up) The ported defaults table synthesized repo_mode → "unknown", but EMPTY is load-bearing for that key: gstack-repo-mode treats any non-empty answer as a user override and skips its own repo classification — the synthesized default turned the classifier into dead code (REPO_MODE=unknown everywhere; caught by test/gstack-repo-mode.test.ts via the wave's cross-agent blame protocol). repo_mode joins the empty-is-real carve-outs (empty output, exit 0). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(pair-agent): consent before killing a healthy headless daemon The pair-agent headed switch spawned 'connect --force-restart' unconditionally — auto-killing a live headless daemon (open tabs, cookies, logins) in direct contradiction of the iron rule it sits beside ('only an explicit --force-restart may kill a live daemon'). The CLI now captures daemon liveness BEFORE ensureServer (which can itself boot a fresh daemon) and relaunches only when the user passed --force-restart to pair-agent; otherwise it prints the tab count and continues against the existing daemon. The /pair-agent skill gains a matching one-way-door consent question (template half rides the wave's template block). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(gbrain-status): MCP scoping is per-project, and project-local beats user scope hasRemoteOnlyGbrainMcp scanned EVERY project's mcpServers in ~/.claude.json, so one project's remote gbrain registration reclassified broken local engines as thin-client machine-wide. It now reads user scope plus only the cwd's nearest-ancestor project key. The precedence itself was verified empirically and hermetically (fake HOME + CLAUDE_CONFIG_DIR fixtures, claude 2.1.233): with both scopes defining gbrain, 'claude mcp get gbrain' reports Scope: Local config — PROJECT-LOCAL WINS. Both in-repo consumers assumed the opposite; brain-cache's endpoint resolution flips to nearest-ancestor-project-first, and the stale user-first pin in brain-cache-roundtrip now pins the verified precedence. (The user-first jq in the brain-sync preamble resolver gets the same swap in the template block.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(slug): gstack-slug matches remote-slug's owner-repo canonical form (live misfile bug) Found live during this wave's CEO review: bin/gstack-slug emitted SLUG=garrytan for this garrytan/gstack worktree while remote-slug correctly gave garrytan-gstack — decisions, timeline, ceo-plans, and learnings were filing into the wrong project store (observed polluting Context Recovery with another repo's decisions). Root cause: a stray empty ~/.git directory made the walk-up crown $HOME as the outermost project root; the remote lookup ran only against that root, failed silently, and the basename fallback cached 'garrytan' sticky. NOT worktree-specific — any strong marker on a non-repo ancestor triggered it. Fix: the walk now finds the outermost ancestor whose .git actually resolves an origin remote and derives owner-repo with remote-slug's byte-identical parse; marker-only ancestors keep anchoring the basename fallback but can no longer shadow a real remote. A new cache self-heal recomputes the poisoned shape (cached == basename of a marker root while a remote-bearing repo exists below), preserving legit #2212 stickiness. Nested-repo walk-up, no-remote and non-git fallbacks, and the SLUG=/BRANCH= eval contract are unchanged, pinned by a 10-case parity suite. Store migration for pre-fix data is tracked in TODOS.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(brain-sync): per-record spool dir — the enqueue/drain race dies structurally Producers appended lines to .brain-queue.jsonl while the drain re-read and os.replace'd it; the in-code comment admitted a lockless append between the re-read and the replace was lost. Locks and rename-rotation designs were both reviewed and rejected (each retained a tail race); the shipped design is a maildir-style spool: one FILE per record in .brain-queue.d/ (tmp + atomic rename), the drain snapshots filenames, processes, and deletes exactly what it snapshotted. Writer and drainer never share an inode — nothing to race. Semantics: at-least-once (a crash between process and unlink re-drains; downstream content-hash dedup absorbs duplicates); retained (privacy-held) records keep their files; unparseable records are kept + warned, never destroyed. Legacy .brain-queue.jsonl migrates atomically on the next drain (crash-leftover .migrating files recovered too); status/drop-queue count both surfaces; discover-new writes spool records and advances its cursor per-record-written. The preamble's queue-depth line switches to spool count in this wave's template block. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(bin-context): native slug fallback walks up like bash gstack-slug slugFromEnvironment derived the slug from the INNERMOST repo's origin while bash gstack-slug walks to the outermost project root — nested/vendored repos split their stores across the bash/native boundary (win32 hits the native path constantly). The native fallback now ports _outermost_project_root faithfully (strong/weak markers, outermost-strong-wins, 64-depth cap, fixed-point termination) plus the full resolution order: env override → walk-up → sticky cache with the #1125 self-heal → remote get-url → basename. Twelve mirrored scenarios drive BOTH implementations against the same fixtures and pin identical slugs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(next-version): git fallback queries the live remote, never mutates, and keeps 3-digit width The degraded path counted every remote-tracking ref on every remote — stale experiment branches and second remotes inflated version allocation, and a failed base read flipped 3-digit repos to 4-digit slots. Now: ls-remote --heads origin first (GIT_TERMINAL_PROMPT=0, 5s timeout, zero local ref mutation); on failure, local refs/remotes/origin ONLY with an explicit stale-refs warning; a failed base read zeroes at the LOCAL version file's width so a 3-digit repo allocates 0.0.1, not 0.0.1.0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(setup): hooks register the global-install path and re-point stale ones Registering hooks from a dev worktree baked that worktree's absolute path into settings.json — deleting the worktree left a dead hook erroring on every session stop, and the presence-only dedup (list-sources | grep) could never re-point it. setup's hook paths now route through _hook_install_path (global install preferred, source dir fallback), and the new ensure-event verb on gstack-settings-hook compares the registered command payload against canonical: identical → no write, different → single atomic replacement (never zero or two registrations). The plan-tune hooks had the same stale pattern and get the same fix without re-triggering their consent prompt. Also hardened: bun 1.3.13 turns an uncaught sync fs error in bun -e into a SILENT exit 0 — the registrar's write path now catches, prints, and exits 1, so a failed update can never report fake-green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(preamble): learnings capture is unconditional at completion (#2402) 43 of 44 learnings entries came from explicit /learn — the completion-status prose read 'if you discovered a durable project quirk... log it', which models treated as optional. The step now ALWAYS runs: review the session for durable learnings, log each one, and state 'No durable learnings this session' explicitly when the review comes up empty — an empty result, never a skipped step. Re-derived from PR #2612 under the generated-file screening rule. Fixes #2402. Contributed by @Lockyer228 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(scrape): untrusted-content warning on the page-fetching skills (#2441) /scrape and /skillify consumed page content with zero injection guidance — the CHANGELOG claimed coverage the skills didn't have. The warning now lives in ONE exported const (UNTRUSTED_CONTENT_WARNING in resolvers/browse.ts), embedded in the browse COMMAND_REFERENCE as before AND injected standalone into both skills via the new {{UNTRUSTED_CONTENT_WARNING}} token — single source, wording can never drift between surfaces. Re-derived from PR #2612 under the generated-file screening rule. (Structural isolation for skillify-generated code is tracked as its own TODO.) Fixes #2441. Contributed by @Lockyer228 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(review): checklist paths resolve from the installed skill root (#2518) /review Step 2 read .claude/skills/review/checklist.md — a path relative to the TARGET repo, which only resolves in gstack's own checkout. Every checklist/greptile-triage/TODOS-format reference (six across five templates — two more than the issue named, same class) now uses the installed-root form ~/.claude/skills/gstack/review/... that the templates' other references already use. The install-root class itself (non-default install dirs) is #1882, deliberately its own PR. Fixes #2518. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(pair-agent): one-way-door consent question before a daemon relaunch (template half) The skill flow now checks daemon liveness before Step 4 and asks an explicit one-way-door question (tabs/cookies/logins are lost) before passing --force-restart — never proceeding on a vague reply. Pairs with the CLI-half commit that stopped pair-agent auto-killing live daemons. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(codex): resume does not amortize the ~21K session prelude (#2387) Measured (#2387): every codex exec call pays Codex's session prelude, and a resumed call came in slightly ABOVE a fresh one — resume buys continuity, never token savings. The skill now says so where the resume flow lives: prefer one codex call per skill, batch questions into it. Fixes #2387. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(upgrade): fast-forward first; reset --hard only behind a proved-safe gate (#2517) /gstack-upgrade went straight to stash + reset --hard origin/main. Now it tries git pull --ff-only --autostash first (the same policy session-update's auto-upgrade uses). The destructive fallback runs unprompted ONLY when both git status --porcelain AND git rev-list origin/main..HEAD are empty — a clean tree with unpushed local commits is NOT safe, reset destroys them. Anything else requires an explicit one-way-door confirmation that lists every dirty file and unpushed commit being discarded. Fixes #2517. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(preamble): brain-sync block counts the spool queue and resolves MCP project-first Two resolver halves deferred from earlier wave commits: the queue-depth line counts .brain-queue.d/*.json spool records (plus legacy lines until the drain migrates them), and GBRAIN_MCP_ENTRY_JQ swaps its operands to nearest-ancestor-project-first — matching the empirically verified Claude Code precedence (project-local beats user scope) instead of the backwards user-first assumption. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: regenerate SKILL.md docs + golden fixtures (single regen for the template block) Pure generator output for the six template/resolver commits above (learnings capture, untrusted-content warning, review paths, pair-agent consent, codex resume note, upgrade ff-only, brain-sync block) — bun run gen:skill-docs + --host codex + --host factory, with the three ship golden fixtures refreshed per the documented procedure. The three sidecar-path pins in gen-skill-docs.test.ts move to the new installed-root/$GSTACK_ROOT contract (#2518). Restores template freshness; full suite green from here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: TODOS.md — strike the six wave-fixed residuals, add two follow-ups The v1.67 adversarial-review residuals section shrinks to the one item the wave couldn't reach (iOS tap routing — needs real-device verification). New entries: skillify structural isolation (a prose warning is not a boundary for page-derived generated code) and the slug store migration (pre-fix sessions on stray-marker machines filed data under the degraded slug; post-fix reads go to the correct store, so history needs a merge/alias). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: align cross-cutting pins with the wave's contracts Three suites pinned pre-wave behavior: browse's gstack-config test asserted the old unknown-key ''/exit-0 shape (#2611 made it exit 1); the Windows-paths suite pinned O_APPEND enqueue atomicity (the spool design satisfies the same invariant via tmp + os.replace, one file per record — pinned in its new form); and nine carve-guard skeleton ceilings absorbed the #2402 unconditional-learnings prose (~450B per skill), bumped with measured values per the guard's own protocol. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: re-anchor the referenced-path scanner self-check to the gstack-rooted review refs The self-check pinned the review checklist as a class-1 alias-relative ref; #2518 moved those refs to the installed gstack root (class 2). The guard now proves the scanner sees them in their new class, so the class-2 assertion can't go vacuous. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: pin the wave's prose-tier behaviors (ship coverage-audit gap closure) The coverage audit found one regression-shaped gap: nothing pinned that the upgrade template's ff-only pull precedes the gated reset --hard (#2517) — a future template edit reverting to reset-first would fail nothing. Pinned: the ordering, the FF_OK gate, and the unpushed-commits check. Also pinned the two minor gaps: the {{UNTRUSTED_CONTENT_WARNING}} injection points in scrape/skillify (#2441) and brain-uninstall's spool-dir cleanup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: pre-landing review round — 8 auto-fixes + 8 accepted findings hardened The ship review army (4 specialists + red-team + checklist, 29 findings) produced 8 mechanical auto-fixes and 11 decisions; the accepted set: - win32 slug parity completed: lib/bin-context.ts gains the remote-first outermost walk + degraded-cache self-heal the bash side got this wave — the two implementations now agree on the stray-marker live-bug shape, pinned by shared fixtures (multi-specialist 9/10 finding). - probe honors the plan's bounded-read decision: 256KB prefix, extraction semantics mirrored from parseTranscriptJsonl so probe/prepare can never diverge on the same file (>1MB transcript test). - policy normalize parity: bash normalize() now matches canonicalizeRemote on .git/-trailing and uppercase-.GIT shapes (7-shape corpus pinned two ways) — a deny for those shapes could previously slip the transcript gate. - session-update reclaim is TOCTOU-safe (atomic mv-aside on both branches). - settings-hook: unparseable settings.json errors instead of being replaced with {}; ensure-event keys on (event, source) so matcher changes update in place — never zero or two registrations. - dot-only slug guard at both parse sites (hostile 'url = ..' can't escape projects/); enqueue tmp-file janitor (1h TTL, inside the drain lock); brain-sync .migrating never clobbered; drop-queue/status count .migrating; snapshot -o warning correct + surfaced in diff mode; version-bump test order-dependence removed; uninstall clears the advance stamp. Deferred with record: slug heal-probe cost sentinel (P3 TODO), FF_OK conflation (noted, misdiagnosis-only). 270 pass / 0 fail across the 10 touched suites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: adversarial round — the P0 finalize fail-safe and 12 hardened findings Three adversarial passes (Claude fresh-context, Codex chaos, Codex structured with P1 gate) on the full wave diff. Multi-source findings, all fixed: - P0: finalize_queue is now explicit-delete-only — a record is unlinked ONLY when classification proves it staged or dropped; a classifier crash, a missing class file, or a malformed pulled .brain-privacy-map.json (which previously nuked the whole snapshotted queue, remotely triggerable) now retains everything, warns, and re-drains next run. load_privacy_map treats corrupt maps as retain-all, never as empty. - next-version cannot silently drop a live claim: unreadable advertised refs get a targeted --depth=1 fetch + retry; still-unreadable claims surface as UNKNOWN warnings instead of duplicate-version silence. - session-update lock: ownership-checked EXIT trap (a TTL-reclaimed holder can no longer delete the new holder's lock) + a 5-min background heartbeat so a legitimately-slow pull/setup is never reclaimed while alive. - ensure-event collapses ALL same-(event,source) duplicates to one canonical entry; unique per-process tmp path; setup call sites surface (not swallow) the hardened refusals. - memory-ingest: --limit counts only policy-permitted pages (denied records no longer starve permitted ones); --probe applies the same policy filter as --bulk (skipped_policy_* fields on the report). - version-bump repair accepts a genuine literal 0.0.0.0 VERSION file. - slug heal restricted to the stray-.git shape — package.json-anchored wrapper roots keep their legit sticky identity (#2212 preserved). - brain-sync: idle fast path sees leftover .migrating records; unparseable spool records quarantine instead of warning forever; migration comment stops overclaiming the transition-window race. - CDP throttling justifications document override persistence (callers own restoration), pinned in the allowlist test. Deferred with record: deny retroactivity for already-ingested pages (P2 TODO, same semantics as the code-import gate); legacy-migration tail race (transition-window, requires pre-spool writers). 288 pass / 0 fail across the 10 touched suites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: regenerate SKILL.md docs + goldens (Windows-separator jq fix) Pure generator output for the brain-sync block's jq ancestor match now accepting backslash-formed Windows project keys — previously project-scoped brains were invisible on Windows while the TS scope resolvers saw them. Golden ship fixtures refreshed per the documented procedure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: codex verify-pass residuals — chunked cwd read, post-filter partial count, migrating depth The verify re-review passed the P1 gate (0 P1s) and left three residuals, all applied: transcriptCwdFromPrefix reads in chunks until one complete record (4MB cap) so a giant first prompt can't truncate mid-JSON and break probe/bulk parity; partial_pages derives from the FINAL prepared set instead of the whole scanned corpus; the preamble queue-depth line counts leftover .brain-queue.jsonl.migrating records like the status path does (regen + goldens included). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: bump version and changelog (v1.68.0.0) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: update project documentation for v1.68.0.0 BROWSER.md: fix the $B cdp example (positional JSON params, not --json; depth is the real CDP param) and add the new perf-throttling examples (Emulation.setCPUThrottlingRate, Network.emulateNetworkConditions) with their clear-override counterparts. USING_GBRAIN_WITH_GSTACK.md: the state-files table row for the sync queue now names the maildir-style spool dir .brain-queue.d/ that replaced .brain-queue.jsonl this release. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: align memory-pipeline probe pins with the #2394 stage-count contract The paid-tier E2E pinned the pre-fix contract (probe headline = raw discovered). Probe now counts post-attribution — the same gate --bulk uses — with an explicit unattributed-skip line. Adds the --include-unattributed companion pin so all 9 fixtures stay accounted for. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(next-version): batch missing-tip fetches — one bounded round trip, never a per-branch crawl The targeted-fetch retry for branches whose advertised tip has no local object ran ONE git fetch per branch (10s cap each). On a shallow clone against a busy remote that crawls the network for minutes — CI's shard deadline killed the free suite mid-file. Missing tips now collect into a single batched shallow fetch (15s cap); refs still missing after the batch (one unservable ref fails the whole transfer) get a capped per-branch retry, and anything past the cap warns as an UNKNOWN claim instead of fetching. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(next-version): pin the batched fetch + make the offline-contract tests hermetic Two new G2 pins: N unfetched claim branches resolve with exactly ONE fetch spawn (PATH-shimmed git counts invocations), and one unservable ref no longer poisons the batch — live claims resolve via the bounded retry while only the ghost warns UNKNOWN. The #2545 offline-contract tests now run the CLI in a local fixture repo instead of the repo's own checkout: the checkout path did a live ls-remote against the real origin (operator-network-dependent, and the CI shard-deadline hang). The online-contract test gains a succeeding gh stub, so fallback:null is asserted deterministically instead of only when the operator happens to be authed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(redact-cli): derive the synthetic AWS-key fixture — no contiguous credential literal in source The CI quality gate scans every ADDED diff line with the redact engine, so the #2610 port's raw fixture literals failed the very gate they exist to test. The fixture is now assembled at runtime; the scanner still receives the identical bytes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(next-version): pin the fixture's host via origin-URL sniff — kills the last environment dependence The hermetic offline-contract fixture had no origin remote, so detectHost() fell through to auth probes: a machine with glab authed passed via the gitlab path while a bare CI runner read host:unknown (offline stays false there) and failed. The fixture now pushes to a local bare origin at a path containing github.com — the URL sniff pins host:github identically everywhere, asserted explicitly in both tests, with every git call still local. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: y$un_ <forrest.sun527@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: benjamin beres <benjamin.beres@bienpreter.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Ricky <ricky@kinokostudio.com.hk> Co-authored-by: Connex Client Access <paul@paulkortman.com> Co-authored-by: henbima <henbima@gmail.com>
493 lines
35 KiB
Cheetah
493 lines
35 KiB
Cheetah
## Review Sections (11 sections, after scope and mode are agreed)
|
|
|
|
**Anti-skip rule:** Never condense, abbreviate, or skip any review section (1-11) regardless of plan type (strategy, spec, code, infra). Every section in this skill exists for a reason. "This is a strategy doc so implementation sections don't apply" is always wrong — implementation details are where strategy breaks down. If a section genuinely has zero findings, say "No issues found" and move on — but you must evaluate it.
|
|
|
|
{{ANTI_SHORTCUT_CLAUSE}}
|
|
|
|
### Section 1: Architecture Review
|
|
Evaluate and diagram:
|
|
* Overall system design and component boundaries. Draw the dependency graph.
|
|
* Data flow — all four paths. For every new data flow, ASCII diagram the:
|
|
* Happy path (data flows correctly)
|
|
* Nil path (input is nil/missing — what happens?)
|
|
* Empty path (input is present but empty/zero-length — what happens?)
|
|
* Error path (upstream call fails — what happens?)
|
|
* State machines. ASCII diagram for every new stateful object. Include impossible/invalid transitions and what prevents them.
|
|
* Coupling concerns. Which components are now coupled that weren't before? Is that coupling justified? Draw the before/after dependency graph.
|
|
* Scaling characteristics. What breaks first under 10x load? Under 100x?
|
|
* Single points of failure. Map them.
|
|
* Security architecture. Auth boundaries, data access patterns, API surfaces. For each new endpoint or data mutation: who can call it, what do they get, what can they change?
|
|
* Production failure scenarios. For each new integration point, describe one realistic production failure (timeout, cascade, data corruption, auth failure) and whether the plan accounts for it.
|
|
* Rollback posture. If this ships and immediately breaks, what's the rollback procedure? Git revert? Feature flag? DB migration rollback? How long?
|
|
|
|
**EXPANSION and SELECTIVE EXPANSION additions:**
|
|
* What would make this architecture beautiful? Not just correct — elegant. Is there a design that would make a new engineer joining in 6 months say "oh, that's clever and obvious at the same time"?
|
|
* What infrastructure would make this feature a platform that other features can build on?
|
|
|
|
**SELECTIVE EXPANSION:** If any accepted cherry-picks from Step 0D affect the architecture, evaluate their architectural fit here. Flag any that create coupling concerns or don't integrate cleanly — this is a chance to revisit the decision with new information.
|
|
|
|
Required ASCII diagram: full system architecture showing new components and their relationships to existing ones.
|
|
**STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. If this section turned up zero findings, state "No issues, moving on" and proceed. If the section has findings, you MUST call AskUserQuestion as a tool_use — a finding with an "obvious fix" is still a finding and still needs user approval before any change lands in the plan. Do NOT proceed until the user responds.
|
|
**Reminder: Do NOT make any code changes. Review only.**
|
|
|
|
### Section 2: Error & Rescue Map
|
|
This is the section that catches silent failures. It is not optional.
|
|
For every new method, service, or codepath that can fail, fill in this table:
|
|
```
|
|
METHOD/CODEPATH | WHAT CAN GO WRONG | EXCEPTION CLASS
|
|
-------------------------|-----------------------------|-----------------
|
|
ExampleService#call | API timeout | TimeoutError
|
|
| API returns 429 | RateLimitError
|
|
| API returns malformed JSON | JSONParseError
|
|
| DB connection pool exhausted| ConnectionPoolExhausted
|
|
| Record not found | RecordNotFound
|
|
-------------------------|-----------------------------|-----------------
|
|
|
|
EXCEPTION CLASS | RESCUED? | RESCUE ACTION | USER SEES
|
|
-----------------------------|-----------|------------------------|------------------
|
|
TimeoutError | Y | Retry 2x, then raise | "Service temporarily unavailable"
|
|
RateLimitError | Y | Backoff + retry | Nothing (transparent)
|
|
JSONParseError | N ← GAP | — | 500 error ← BAD
|
|
ConnectionPoolExhausted | N ← GAP | — | 500 error ← BAD
|
|
RecordNotFound | Y | Return nil, log warning | "Not found" message
|
|
```
|
|
Rules for this section:
|
|
* Catch-all error handling (`rescue StandardError`, `catch (Exception e)`, `except Exception`) is ALWAYS a smell. Name the specific exceptions.
|
|
* Catching an error with only a generic log message is insufficient. Log the full context: what was being attempted, with what arguments, for what user/request.
|
|
* Every rescued error must either: retry with backoff, degrade gracefully with a user-visible message, or re-raise with added context. "Swallow and continue" is almost never acceptable.
|
|
* For each GAP (unrescued error that should be rescued): specify the rescue action and what the user should see.
|
|
* For LLM/AI service calls specifically: what happens when the response is malformed? When it's empty? When it hallucinates invalid JSON? When the model returns a refusal? Each of these is a distinct failure mode.
|
|
**STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. If this section turned up zero findings, state "No issues, moving on" and proceed. If the section has findings, you MUST call AskUserQuestion as a tool_use — a finding with an "obvious fix" is still a finding and still needs user approval before any change lands in the plan. Do NOT proceed until the user responds.
|
|
**Reminder: Do NOT make any code changes. Review only.**
|
|
|
|
### Section 3: Security & Threat Model
|
|
Security is not a sub-bullet of architecture. It gets its own section.
|
|
Evaluate:
|
|
* Attack surface expansion. What new attack vectors does this plan introduce? New endpoints, new params, new file paths, new background jobs?
|
|
* Input validation. For every new user input: is it validated, sanitized, and rejected loudly on failure? What happens with: nil, empty string, string when integer expected, string exceeding max length, unicode edge cases, HTML/script injection attempts?
|
|
* Authorization. For every new data access: is it scoped to the right user/role? Is there a direct object reference vulnerability? Can user A access user B's data by manipulating IDs?
|
|
* Secrets and credentials. New secrets? In env vars, not hardcoded? Rotatable?
|
|
* Dependency risk. New gems/npm packages? Security track record?
|
|
* Data classification. PII, payment data, credentials? Handling consistent with existing patterns?
|
|
* Injection vectors. SQL, command, template, LLM prompt injection — check all.
|
|
* Audit logging. For sensitive operations: is there an audit trail?
|
|
|
|
For each finding: threat, likelihood (High/Med/Low), impact (High/Med/Low), and whether the plan mitigates it.
|
|
**STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. If this section turned up zero findings, state "No issues, moving on" and proceed. If the section has findings, you MUST call AskUserQuestion as a tool_use — a finding with an "obvious fix" is still a finding and still needs user approval before any change lands in the plan. Do NOT proceed until the user responds.
|
|
**Reminder: Do NOT make any code changes. Review only.**
|
|
|
|
### Section 4: Data Flow & Interaction Edge Cases
|
|
This section traces data through the system and interactions through the UI with adversarial thoroughness.
|
|
|
|
**Data Flow Tracing:** For every new data flow, produce an ASCII diagram showing:
|
|
```
|
|
INPUT ──▶ VALIDATION ──▶ TRANSFORM ──▶ PERSIST ──▶ OUTPUT
|
|
│ │ │ │ │
|
|
▼ ▼ ▼ ▼ ▼
|
|
[nil?] [invalid?] [exception?] [conflict?] [stale?]
|
|
[empty?] [too long?] [timeout?] [dup key?] [partial?]
|
|
[wrong [wrong type?] [OOM?] [locked?] [encoding?]
|
|
type?]
|
|
```
|
|
For each node: what happens on each shadow path? Is it tested?
|
|
|
|
**Interaction Edge Cases:** For every new user-visible interaction, evaluate:
|
|
```
|
|
INTERACTION | EDGE CASE | HANDLED? | HOW?
|
|
---------------------|------------------------|----------|--------
|
|
Form submission | Double-click submit | ? |
|
|
| Submit with stale CSRF | ? |
|
|
| Submit during deploy | ? |
|
|
Async operation | User navigates away | ? |
|
|
| Operation times out | ? |
|
|
| Retry while in-flight | ? |
|
|
List/table view | Zero results | ? |
|
|
| 10,000 results | ? |
|
|
| Results change mid-page| ? |
|
|
Background job | Job fails after 3 of | ? |
|
|
| 10 items processed | |
|
|
| Job runs twice (dup) | ? |
|
|
| Queue backs up 2 hours | ? |
|
|
```
|
|
Flag any unhandled edge case as a gap. For each gap, specify the fix.
|
|
**STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. If this section turned up zero findings, state "No issues, moving on" and proceed. If the section has findings, you MUST call AskUserQuestion as a tool_use — a finding with an "obvious fix" is still a finding and still needs user approval before any change lands in the plan. Do NOT proceed until the user responds.
|
|
**Reminder: Do NOT make any code changes. Review only.**
|
|
|
|
### Section 5: Code Quality Review
|
|
Evaluate:
|
|
* Code organization and module structure. Does new code fit existing patterns? If it deviates, is there a reason?
|
|
* DRY violations. Be aggressive. If the same logic exists elsewhere, flag it and reference the file and line.
|
|
* Naming quality. Are new classes, methods, and variables named for what they do, not how they do it?
|
|
* Error handling patterns. (Cross-reference with Section 2 — this section reviews the patterns; Section 2 maps the specifics.)
|
|
* Missing edge cases. List explicitly: "What happens when X is nil?" "When the API returns 429?" etc.
|
|
* Over-engineering check. Any new abstraction solving a problem that doesn't exist yet?
|
|
* Under-engineering check. Anything fragile, assuming happy path only, or missing obvious defensive checks?
|
|
* Cyclomatic complexity. Flag any new method that branches more than 5 times. Propose a refactor.
|
|
**STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. If this section turned up zero findings, state "No issues, moving on" and proceed. If the section has findings, you MUST call AskUserQuestion as a tool_use — a finding with an "obvious fix" is still a finding and still needs user approval before any change lands in the plan. Do NOT proceed until the user responds.
|
|
**Reminder: Do NOT make any code changes. Review only.**
|
|
|
|
### Section 6: Test Review
|
|
Make a complete diagram of every new thing this plan introduces:
|
|
```
|
|
NEW UX FLOWS:
|
|
[list each new user-visible interaction]
|
|
|
|
NEW DATA FLOWS:
|
|
[list each new path data takes through the system]
|
|
|
|
NEW CODEPATHS:
|
|
[list each new branch, condition, or execution path]
|
|
|
|
NEW BACKGROUND JOBS / ASYNC WORK:
|
|
[list each]
|
|
|
|
NEW INTEGRATIONS / EXTERNAL CALLS:
|
|
[list each]
|
|
|
|
NEW ERROR/RESCUE PATHS:
|
|
[list each — cross-reference Section 2]
|
|
```
|
|
For each item in the diagram:
|
|
* What type of test covers it? (Unit / Integration / System / E2E)
|
|
* Does a test for it exist in the plan? If not, write the test spec header.
|
|
* What is the happy path test?
|
|
* What is the failure path test? (Be specific — which failure?)
|
|
* What is the edge case test? (nil, empty, boundary values, concurrent access)
|
|
|
|
Test ambition check (all modes): For each new feature, answer:
|
|
* What's the test that would make you confident shipping at 2am on a Friday?
|
|
* What's the test a hostile QA engineer would write to break this?
|
|
* What's the chaos test?
|
|
|
|
Test pyramid check: Many unit, fewer integration, few E2E? Or inverted?
|
|
Flakiness risk: Flag any test depending on time, randomness, external services, or ordering.
|
|
Load/stress test requirements: For any new codepath called frequently or processing significant data.
|
|
|
|
For LLM/prompt changes: Check CLAUDE.md for the "Prompt/LLM changes" file patterns. If this plan touches ANY of those patterns, state which eval suites must be run, which cases should be added, and what baselines to compare against.
|
|
**STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. If this section turned up zero findings, state "No issues, moving on" and proceed. If the section has findings, you MUST call AskUserQuestion as a tool_use — a finding with an "obvious fix" is still a finding and still needs user approval before any change lands in the plan. Do NOT proceed until the user responds.
|
|
**Reminder: Do NOT make any code changes. Review only.**
|
|
|
|
### Section 7: Performance Review
|
|
Evaluate:
|
|
* N+1 queries. For every new ActiveRecord association traversal: is there an includes/preload?
|
|
* Memory usage. For every new data structure: what's the maximum size in production?
|
|
* Database indexes. For every new query: is there an index?
|
|
* Caching opportunities. For every expensive computation or external call: should it be cached?
|
|
* Background job sizing. For every new job: worst-case payload, runtime, retry behavior?
|
|
* Slow paths. Top 3 slowest new codepaths and estimated p99 latency.
|
|
* Connection pool pressure. New DB connections, Redis connections, HTTP connections?
|
|
**STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. If this section turned up zero findings, state "No issues, moving on" and proceed. If the section has findings, you MUST call AskUserQuestion as a tool_use — a finding with an "obvious fix" is still a finding and still needs user approval before any change lands in the plan. Do NOT proceed until the user responds.
|
|
**Reminder: Do NOT make any code changes. Review only.**
|
|
|
|
### Section 8: Observability & Debuggability Review
|
|
New systems break. This section ensures you can see why.
|
|
Evaluate:
|
|
* Logging. For every new codepath: structured log lines at entry, exit, and each significant branch?
|
|
* Metrics. For every new feature: what metric tells you it's working? What tells you it's broken?
|
|
* Tracing. For new cross-service or cross-job flows: trace IDs propagated?
|
|
* Alerting. What new alerts should exist?
|
|
* Dashboards. What new dashboard panels do you want on day 1?
|
|
* Debuggability. If a bug is reported 3 weeks post-ship, can you reconstruct what happened from logs alone?
|
|
* Admin tooling. New operational tasks that need admin UI or rake tasks?
|
|
* Runbooks. For each new failure mode: what's the operational response?
|
|
|
|
**EXPANSION and SELECTIVE EXPANSION addition:**
|
|
* What observability would make this feature a joy to operate? (For SELECTIVE EXPANSION, include observability for any accepted cherry-picks.)
|
|
**STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. If this section turned up zero findings, state "No issues, moving on" and proceed. If the section has findings, you MUST call AskUserQuestion as a tool_use — a finding with an "obvious fix" is still a finding and still needs user approval before any change lands in the plan. Do NOT proceed until the user responds.
|
|
**Reminder: Do NOT make any code changes. Review only.**
|
|
|
|
### Section 9: Deployment & Rollout Review
|
|
Evaluate:
|
|
* Migration safety. For every new DB migration: backward-compatible? Zero-downtime? Table locks?
|
|
* Feature flags. Should any part be behind a feature flag?
|
|
* Rollout order. Correct sequence: migrate first, deploy second?
|
|
* Rollback plan. Explicit step-by-step.
|
|
* Deploy-time risk window. Old code and new code running simultaneously — what breaks?
|
|
* Environment parity. Tested in staging?
|
|
* Post-deploy verification checklist. First 5 minutes? First hour?
|
|
* Smoke tests. What automated checks should run immediately post-deploy?
|
|
|
|
**EXPANSION and SELECTIVE EXPANSION addition:**
|
|
* What deploy infrastructure would make shipping this feature routine? (For SELECTIVE EXPANSION, assess whether accepted cherry-picks change the deployment risk profile.)
|
|
**STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. If this section turned up zero findings, state "No issues, moving on" and proceed. If the section has findings, you MUST call AskUserQuestion as a tool_use — a finding with an "obvious fix" is still a finding and still needs user approval before any change lands in the plan. Do NOT proceed until the user responds.
|
|
**Reminder: Do NOT make any code changes. Review only.**
|
|
|
|
### Section 10: Long-Term Trajectory Review
|
|
Evaluate:
|
|
* Technical debt introduced. Code debt, operational debt, testing debt, documentation debt.
|
|
* Path dependency. Does this make future changes harder?
|
|
* Knowledge concentration. Documentation sufficient for a new engineer?
|
|
* Reversibility. Rate 1-5: 1 = one-way door, 5 = easily reversible.
|
|
* Ecosystem fit. Aligns with Rails/JS ecosystem direction?
|
|
* The 1-year question. Read this plan as a new engineer in 12 months — obvious?
|
|
|
|
**EXPANSION and SELECTIVE EXPANSION additions:**
|
|
* What comes after this ships? Phase 2? Phase 3? Does the architecture support that trajectory?
|
|
* Platform potential. Does this create capabilities other features can leverage?
|
|
* (SELECTIVE EXPANSION only) Retrospective: Were the right cherry-picks accepted? Did any rejected expansions turn out to be load-bearing for the accepted ones?
|
|
**STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. If this section turned up zero findings, state "No issues, moving on" and proceed. If the section has findings, you MUST call AskUserQuestion as a tool_use — a finding with an "obvious fix" is still a finding and still needs user approval before any change lands in the plan. Do NOT proceed until the user responds.
|
|
**Reminder: Do NOT make any code changes. Review only.**
|
|
|
|
### Section 11: Design & UX Review (skip if no UI scope detected)
|
|
The CEO calling in the designer. Not a pixel-level audit — that's /plan-design-review and /design-review. This is ensuring the plan has design intentionality.
|
|
|
|
Evaluate:
|
|
* Information architecture — what does the user see first, second, third?
|
|
* Interaction state coverage map:
|
|
FEATURE | LOADING | EMPTY | ERROR | SUCCESS | PARTIAL
|
|
* User journey coherence — storyboard the emotional arc
|
|
* AI slop risk — does the plan describe generic UI patterns?
|
|
* DESIGN.md alignment — does the plan match the stated design system?
|
|
* Responsive intention — is mobile mentioned or afterthought?
|
|
* Accessibility basics — keyboard nav, screen readers, contrast, touch targets
|
|
|
|
**EXPANSION and SELECTIVE EXPANSION additions:**
|
|
* What would make this UI feel *inevitable*?
|
|
* What 30-minute UI touches would make users think "oh nice, they thought of that"?
|
|
|
|
Required ASCII diagram: user flow showing screens/states and transitions.
|
|
|
|
If this plan has significant UI scope, recommend: "Consider running /plan-design-review for a deep design review of this plan before implementation."
|
|
**STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. If this section turned up zero findings, state "No issues, moving on" and proceed. If the section has findings, you MUST call AskUserQuestion as a tool_use — a finding with an "obvious fix" is still a finding and still needs user approval before any change lands in the plan. Do NOT proceed until the user responds.
|
|
**Reminder: Do NOT make any code changes. Review only.**
|
|
|
|
{{CODEX_PLAN_REVIEW}}
|
|
|
|
### Outside Voice Integration Rule
|
|
|
|
Outside voice findings are INFORMATIONAL until the user explicitly approves each one.
|
|
Do NOT incorporate outside voice recommendations into the plan without presenting each
|
|
finding via AskUserQuestion and getting explicit approval. This applies even when you
|
|
agree with the outside voice. Cross-model consensus is a strong signal — present it as
|
|
such — but the user makes the decision.
|
|
|
|
## Post-Implementation Design Audit (if UI scope detected)
|
|
After implementation, run `/design-review` on the live site to catch visual issues that can only be evaluated with rendered output.
|
|
|
|
## CRITICAL RULE — How to ask questions
|
|
Follow the AskUserQuestion format from the Preamble above. Additional rules for plan reviews:
|
|
* **One issue = one AskUserQuestion call.** Never combine multiple issues into one question.
|
|
* Describe the problem concretely, with file and line references.
|
|
* Present 2-3 options, including "do nothing" where reasonable.
|
|
* For each option: effort, risk, and maintenance burden in one line.
|
|
* **Map the reasoning to my engineering preferences above.** One sentence connecting your recommendation to a specific preference.
|
|
* Label with issue NUMBER + option LETTER (e.g., "3A", "3B").
|
|
* **Zero findings:** if a section has zero findings, state "No issues, moving on" and proceed. Otherwise, use AskUserQuestion for each finding — a finding with an "obvious fix" is still a finding and still needs user approval before any change lands in the plan.
|
|
|
|
## Required Outputs
|
|
|
|
### "NOT in scope" section
|
|
List work considered and explicitly deferred, with one-line rationale each.
|
|
|
|
### "What already exists" section
|
|
List existing code/flows that partially solve sub-problems and whether the plan reuses them.
|
|
|
|
### "Dream state delta" section
|
|
Where this plan leaves us relative to the 12-month ideal.
|
|
|
|
### Error & Rescue Registry (from Section 2)
|
|
Complete table of every method that can fail, every exception class, rescued status, rescue action, user impact.
|
|
|
|
### Failure Modes Registry
|
|
```
|
|
CODEPATH | FAILURE MODE | RESCUED? | TEST? | USER SEES? | LOGGED?
|
|
---------|----------------|----------|-------|----------------|--------
|
|
```
|
|
Any row with RESCUED=N, TEST=N, USER SEES=Silent → **CRITICAL GAP**.
|
|
|
|
### TODOS.md updates
|
|
Present each potential TODO as its own individual AskUserQuestion. Never batch TODOs — one per question. Never silently skip this step. Follow the format in `~/.claude/skills/gstack/review/TODOS-format.md`.
|
|
|
|
For each TODO, describe:
|
|
* **What:** One-line description of the work.
|
|
* **Why:** The concrete problem it solves or value it unlocks.
|
|
* **Pros:** What you gain by doing this work.
|
|
* **Cons:** Cost, complexity, or risks of doing it.
|
|
* **Context:** Enough detail that someone picking this up in 3 months understands the motivation, the current state, and where to start.
|
|
* **Effort estimate:** S/M/L/XL (human team) → with CC+gstack: S→S, M→S, L→M, XL→L
|
|
* **Priority:** P1/P2/P3
|
|
* **Depends on / blocked by:** Any prerequisites or ordering constraints.
|
|
|
|
Then present options: **A)** Add to TODOS.md **B)** Skip — not valuable enough **C)** Build it now in this PR instead of deferring.
|
|
|
|
### Scope Expansion Decisions (EXPANSION and SELECTIVE EXPANSION only)
|
|
For EXPANSION and SELECTIVE EXPANSION modes: expansion opportunities and delight items were surfaced and decided in Step 0D (opt-in/cherry-pick ceremony). The decisions are persisted in the CEO plan document. Reference the CEO plan for the full record. Do not re-surface them here — list the accepted expansions for completeness:
|
|
* Accepted: {list items added to scope}
|
|
* Deferred: {list items sent to TODOS.md}
|
|
* Skipped: {list items rejected}
|
|
|
|
### Diagrams (mandatory, produce all that apply)
|
|
1. System architecture
|
|
2. Data flow (including shadow paths)
|
|
3. State machine
|
|
4. Error flow
|
|
5. Deployment sequence
|
|
6. Rollback flowchart
|
|
|
|
### Stale Diagram Audit
|
|
List every ASCII diagram in files this plan touches. Still accurate?
|
|
|
|
{{TASKS_SECTION_EMIT:ceo-review}}
|
|
|
|
### Completion Summary
|
|
```
|
|
+====================================================================+
|
|
| MEGA PLAN REVIEW — COMPLETION SUMMARY |
|
|
+====================================================================+
|
|
| Mode selected | EXPANSION / SELECTIVE / HOLD / REDUCTION |
|
|
| System Audit | [key findings] |
|
|
| Step 0 | [mode + key decisions] |
|
|
| Section 1 (Arch) | ___ issues found |
|
|
| Section 2 (Errors) | ___ error paths mapped, ___ GAPS |
|
|
| Section 3 (Security)| ___ issues found, ___ High severity |
|
|
| Section 4 (Data/UX) | ___ edge cases mapped, ___ unhandled |
|
|
| Section 5 (Quality) | ___ issues found |
|
|
| Section 6 (Tests) | Diagram produced, ___ gaps |
|
|
| Section 7 (Perf) | ___ issues found |
|
|
| Section 8 (Observ) | ___ gaps found |
|
|
| Section 9 (Deploy) | ___ risks flagged |
|
|
| Section 10 (Future) | Reversibility: _/5, debt items: ___ |
|
|
| Section 11 (Design) | ___ issues / SKIPPED (no UI scope) |
|
|
+--------------------------------------------------------------------+
|
|
| NOT in scope | written (___ items) |
|
|
| What already exists | written |
|
|
| Dream state delta | written |
|
|
| Error/rescue registry| ___ methods, ___ CRITICAL GAPS |
|
|
| Failure modes | ___ total, ___ CRITICAL GAPS |
|
|
| TODOS.md updates | ___ items proposed |
|
|
| Scope proposals | ___ proposed, ___ accepted (EXP + SEL) |
|
|
| CEO plan | written / skipped (HOLD/REDUCTION) |
|
|
| Outside voice | ran (codex/claude) / skipped |
|
|
| Lake Score | X/Y recommendations chose complete option |
|
|
| Diagrams produced | ___ (list types) |
|
|
| Stale diagrams found | ___ |
|
|
| Unresolved decisions | ___ (listed below) |
|
|
+====================================================================+
|
|
```
|
|
|
|
### Unresolved Decisions
|
|
If any AskUserQuestion goes unanswered, note it here. Never silently default.
|
|
|
|
## Handoff Note Cleanup
|
|
|
|
After producing the Completion Summary, clean up any handoff notes for this branch —
|
|
the review is complete and the context is no longer needed.
|
|
|
|
```bash
|
|
setopt +o nomatch 2>/dev/null || true # zsh compat
|
|
{{SLUG_EVAL}}
|
|
rm -f ~/.gstack/projects/$SLUG/*-$BRANCH-ceo-handoff-*.md 2>/dev/null || true
|
|
```
|
|
|
|
## Review Log
|
|
|
|
After producing the Completion Summary above, persist the review result.
|
|
|
|
**PLAN MODE EXCEPTION — ALWAYS RUN:** This command writes review metadata to
|
|
`~/.gstack/` (user config directory, not project files). The skill preamble
|
|
already writes to `~/.gstack/sessions/` and `~/.gstack/analytics/` — this is
|
|
the same pattern. The review dashboard depends on this data. Skipping this
|
|
command breaks the review readiness dashboard in /ship.
|
|
|
|
```bash
|
|
~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"plan-ceo-review","timestamp":"TIMESTAMP","status":"STATUS","unresolved":N,"critical_gaps":N,"mode":"MODE","scope_proposed":N,"scope_accepted":N,"scope_deferred":N,"commit":"COMMIT"}'
|
|
~/.claude/skills/gstack/bin/gstack-decision-log '{"decision":"CEO review (MODE): SCOPE_SUMMARY","rationale":"VERDICT","scope":"branch","source":"skill","confidence":8}' 2>/dev/null || true
|
|
```
|
|
|
|
The second command records the accepted scope as a durable cross-session decision so the next session sees what was settled (and why) without re-litigating it. It writes to `~/.gstack/` (same pattern as review-log), is non-interactive, and is best-effort (`|| true` — never blocks the review). Substitute `SCOPE_SUMMARY` (e.g. "accepted 4 of 6 proposals" for expansion, or "held scope" / "cut 3 items" for HOLD/REDUCTION) and `VERDICT` (the one-line verdict from the summary).
|
|
|
|
Before running this command, substitute the placeholder values from the Completion Summary you just produced:
|
|
- **TIMESTAMP**: current ISO 8601 datetime (e.g., 2026-03-16T14:30:00)
|
|
- **STATUS**: "clean" if 0 unresolved decisions AND 0 critical gaps; otherwise "issues_open"
|
|
- **unresolved**: number from "Unresolved decisions" in the summary
|
|
- **critical_gaps**: number from "Failure modes: ___ CRITICAL GAPS" in the summary
|
|
- **MODE**: the mode the user selected (SCOPE_EXPANSION / SELECTIVE_EXPANSION / HOLD_SCOPE / SCOPE_REDUCTION)
|
|
- **scope_proposed**: number from "Scope proposals: ___ proposed" in the summary (0 for HOLD/REDUCTION)
|
|
- **scope_accepted**: number from "Scope proposals: ___ accepted" in the summary (0 for HOLD/REDUCTION)
|
|
- **scope_deferred**: number of items deferred to TODOS.md from scope decisions (0 for HOLD/REDUCTION)
|
|
- **COMMIT**: output of `git rev-parse --short HEAD`
|
|
|
|
{{REVIEW_DASHBOARD}}
|
|
|
|
{{PLAN_FILE_REVIEW_REPORT}}
|
|
|
|
## Next Steps — Review Chaining
|
|
|
|
After displaying the Review Readiness Dashboard, recommend the next review(s) based on what this CEO review discovered. Read the dashboard output to see which reviews have already been run and whether they are stale.
|
|
|
|
**Recommend /plan-eng-review if eng review is not skipped globally** — check the dashboard output for `skip_eng_review`. If it is `true`, eng review is opted out — do not recommend it. Otherwise, eng review is the required shipping gate. If this CEO review expanded scope, changed architectural direction, or accepted scope expansions, emphasize that a fresh eng review is needed. If an eng review already exists in the dashboard but the commit hash shows it predates this CEO review, note that it may be stale and should be re-run.
|
|
|
|
**Recommend /plan-design-review if UI scope was detected** — specifically if Section 11 (Design & UX Review) was NOT skipped, or if accepted scope expansions included UI-facing features. If an existing design review is stale (commit hash drift), note that. In SCOPE REDUCTION mode, skip this recommendation — design review is unlikely relevant for scope cuts.
|
|
|
|
**If both are needed, recommend eng review first** (required gate), then design review.
|
|
|
|
Use AskUserQuestion to present the next step. Include only applicable options:
|
|
- **A)** Run /plan-eng-review next (required gate)
|
|
- **B)** Run /plan-design-review next (only if UI scope detected)
|
|
- **C)** Skip — I'll handle reviews manually
|
|
|
|
## docs/designs Promotion (EXPANSION and SELECTIVE EXPANSION only)
|
|
|
|
At the end of the review, if the vision produced a compelling feature direction, offer to promote the CEO plan to the project repo. AskUserQuestion:
|
|
|
|
"The vision from this review produced {N} accepted scope expansions. Want to promote it to a design doc in the repo?"
|
|
- **A)** Promote to `docs/designs/{FEATURE}.md` (committed to repo, visible to the team)
|
|
- **B)** Keep in `~/.gstack/projects/` only (local, personal reference)
|
|
- **C)** Skip
|
|
|
|
If promoted, copy the CEO plan content to `docs/designs/{FEATURE}.md` (create the directory if needed) and update the `status` field in the original CEO plan from `ACTIVE` to `PROMOTED`.
|
|
|
|
## Formatting Rules
|
|
* NUMBER issues (1, 2, 3...) and LETTERS for options (A, B, C...).
|
|
* Label with NUMBER + LETTER (e.g., "3A", "3B").
|
|
* One sentence max per option.
|
|
* After each section, pause and wait for feedback.
|
|
* Use **CRITICAL GAP** / **WARNING** / **OK** for scannability.
|
|
|
|
{{LEARNINGS_LOG}}
|
|
|
|
{{GBRAIN_SAVE_RESULTS}}
|
|
|
|
{{BRAIN_WRITE_BACK}}
|
|
|
|
{{BRAIN_CACHE_REFRESH}}
|
|
|
|
## Mode Quick Reference
|
|
```
|
|
┌────────────────────────────────────────────────────────────────────────────────┐
|
|
│ MODE COMPARISON │
|
|
├─────────────┬──────────────┬──────────────┬──────────────┬────────────────────┤
|
|
│ │ EXPANSION │ SELECTIVE │ HOLD SCOPE │ REDUCTION │
|
|
├─────────────┼──────────────┼──────────────┼──────────────┼────────────────────┤
|
|
│ Scope │ Push UP │ Hold + offer │ Maintain │ Push DOWN │
|
|
│ │ (opt-in) │ │ │ │
|
|
│ Recommend │ Enthusiastic │ Neutral │ N/A │ N/A │
|
|
│ posture │ │ │ │ │
|
|
│ 10x check │ Mandatory │ Surface as │ Optional │ Skip │
|
|
│ │ │ cherry-pick │ │ │
|
|
│ Platonic │ Yes │ No │ No │ No │
|
|
│ ideal │ │ │ │ │
|
|
│ Delight │ Opt-in │ Cherry-pick │ Note if seen │ Skip │
|
|
│ opps │ ceremony │ ceremony │ │ │
|
|
│ Complexity │ "Is it big │ "Is it right │ "Is it too │ "Is it the bare │
|
|
│ question │ enough?" │ + what else │ complex?" │ minimum?" │
|
|
│ │ │ is tempting"│ │ │
|
|
│ Taste │ Yes │ Yes │ No │ No │
|
|
│ calibration │ │ │ │ │
|
|
│ Temporal │ Full (hr 1-6)│ Full (hr 1-6)│ Key decisions│ Skip │
|
|
│ interrogate │ │ │ only │ │
|
|
│ Observ. │ "Joy to │ "Joy to │ "Can we │ "Can we see if │
|
|
│ standard │ operate" │ operate" │ debug it?" │ it's broken?" │
|
|
│ Deploy │ Infra as │ Safe deploy │ Safe deploy │ Simplest possible │
|
|
│ standard │ feature scope│ + cherry-pick│ + rollback │ deploy │
|
|
│ │ │ risk check │ │ │
|
|
│ Error map │ Full + chaos │ Full + chaos │ Full │ Critical paths │
|
|
│ │ scenarios │ for accepted │ │ only │
|
|
│ CEO plan │ Written │ Written │ Skipped │ Skipped │
|
|
│ Phase 2/3 │ Map accepted │ Map accepted │ Note it │ Skip │
|
|
│ planning │ │ cherry-picks │ │ │
|
|
│ Design │ "Inevitable" │ If UI scope │ If UI scope │ Skip │
|
|
│ (Sec 11) │ UI review │ detected │ detected │ │
|
|
└─────────────┴──────────────┴──────────────┴──────────────┴────────────────────┘
|
|
```
|