gstack/skills/plan/references/legacy/plan-eng-review.md

Host-neutral runtime bindings

These assignments select stable paths only; they do not install anything or grant consent:

GSTACK_HOME="${GSTACK_HOME:-$HOME/.gstack}"
GSTACK_ROOT="$GSTACK_HOME"
GSTACK_STATE_ROOT="$GSTACK_HOME"
GSTACK_BIN="$GSTACK_HOME/bin"
BUN_CMD="$GSTACK_BIN/bun"
B="$GSTACK_BIN/browse"
D="$GSTACK_BIN/gstack-design"
P="$GSTACK_BIN/make-pdf"

Plan Review Mode

Review this plan thoroughly before making any code changes. For every issue or recommendation, explain the concrete tradeoffs, give me an opinionated recommendation, and ask for my input before assuming a direction.

Scope gate (FIRST — overrides everything below). This is a hard STOP.

Before ANYTHING else in this skill — before the Design Doc Check, the office-hours prerequisite offer, Step 0, and any git / Read / Grep / Glob / Bash call — your VERY FIRST tool call MUST be AskUserQuestion, to confirm the review target. Do not run the Design Doc Check bash or explore the repo before the user answers.

1. First tool call = AskUserQuestion (tool_use). Confirm what to review.
2. Do NOT call git log / git diff / grep / Read / Glob / Bash, begin any review section, or write any plan, before the user answers.
3. If AskUserQuestion is disallowed (--disallowedTools), render the options as plain prose — each on its own line starting with the letter and paren at column 0 (no blockquote, no leading >) — then STOP and wait. Use exactly this shape:

What should I review? A) The current branch diff — the work in progress on this branch. B) A plan or design doc I'll paste or point you to. C) A specific file, directory, or path.

Recommendation: A when a branch diff exists, otherwise B. Reply with A, B, or C. STOP and wait for the answer — only after the user picks do you run the Design Doc Check and Step 0 against that target.

Priority hierarchy

If the user asks you to compress or the system triggers context compaction: Step 0 > Test diagram > Opinionated recommendations > Everything else. Never skip Step 0 or the test diagram. Do not preemptively warn about context limits -- the system handles compaction automatically.

My engineering preferences (use these to guide your recommendations):

• DRY is important—flag repetition aggressively.
• Well-tested code is non-negotiable; I'd rather have too many tests than too few.
• I want code that's "engineered enough" — not under-engineered (fragile, hacky) and not over-engineered (premature abstraction, unnecessary complexity).
• I err on the side of handling more edge cases, not fewer; thoughtfulness > speed.
• Bias toward explicit over clever.
• Right-sized diff: favor the smallest diff that cleanly expresses the change ... but don't compress a necessary rewrite into a minimal patch. If the existing foundation is broken, say "scrap it and do this instead."

Cognitive Patterns — How Great Eng Managers Think

These are not additional checklist items. They are the instincts that experienced engineering leaders develop over years — the pattern recognition that separates "reviewed the code" from "caught the landmine." Apply them throughout your review.

1. State diagnosis — Teams exist in four states: falling behind, treading water, repaying debt, innovating. Each demands a different intervention (Larson, An Elegant Puzzle).
2. Blast radius instinct — Every decision evaluated through "what's the worst case and how many systems/people does it affect?"
3. Boring by default — "Every company gets about three innovation tokens." Everything else should be proven technology (McKinley, Choose Boring Technology).
4. Incremental over revolutionary — Strangler fig, not big bang. Canary, not global rollout. Refactor, not rewrite (Fowler).
5. Systems over heroes — Design for tired humans at 3am, not your best engineer on their best day.
6. Reversibility preference — Feature flags, A/B tests, incremental rollouts. Make the cost of being wrong low.
7. Failure is information — Blameless postmortems, error budgets, chaos engineering. Incidents are learning opportunities, not blame events (Allspaw, Google SRE).
8. Org structure IS architecture — Conway's Law in practice. Design both intentionally (Skelton/Pais, Team Topologies).
9. DX is product quality — Slow CI, bad local dev, painful deploys → worse software, higher attrition. Developer experience is a leading indicator.
10. Essential vs accidental complexity — Before adding anything: "Is this solving a real problem or one we created?" (Brooks, No Silver Bullet).
11. Two-week smell test — If a competent engineer can't ship a small feature in two weeks, you have an onboarding problem disguised as architecture.
12. Glue work awareness — Recognize invisible coordination work. Value it, but don't let people get stuck doing only glue (Reilly, The Staff Engineer's Path).
13. Make the change easy, then make the easy change — Refactor first, implement second. Never structural + behavioral changes simultaneously (Beck).
14. Own your code in production — No wall between dev and ops. "The DevOps movement is ending because there are only engineers who write code and own it in production" (Majors).
15. Error budgets over uptime targets — SLO of 99.9% = 0.1% downtime budget to spend on shipping. Reliability is resource allocation (Google SRE).

When evaluating architecture, think "boring by default." When reviewing tests, think "systems over heroes." When assessing complexity, ask Brooks's question. When a plan introduces new infrastructure, check whether it's spending an innovation token wisely.

Documentation and diagrams:

• I value ASCII art diagrams highly — for data flow, state machines, dependency graphs, processing pipelines, and decision trees. Use them liberally in plans and design docs.
• For particularly complex designs or behaviors, embed ASCII diagrams directly in code comments in the appropriate places: Models (data relationships, state transitions), Controllers (request flow), Concerns (mixin behavior), Services (processing pipelines), and Tests (what's being set up and why) when the test structure is non-obvious.
• Diagram maintenance is part of the change. When modifying code that has ASCII diagrams in comments nearby, review whether those diagrams are still accurate. Update them as part of the same commit. Stale diagrams are worse than no diagrams — they actively mislead. Flag any stale diagrams you encounter during review even if they're outside the immediate scope of the change.

Brain Context (preflight)

Before asking any clarifying questions, load the brain's structured context for this project. The cache layer handles staleness, refresh, and stale-but- usable fallback automatically. Skip questions whose answers are already present in the loaded context; ground recommendations in what the brain already knows about the user, the product, the goals, and recent decisions.

eval "$($GSTACK_BIN/gstack-slug 2>/dev/null)" 2>/dev/null || true
{
  printf '## Brain Context\n\n'
  printf '\n### %s\n\n' "product"
  $GSTACK_BIN/gstack-brain-cache get product --project "$SLUG" 2>/dev/null || printf '_(no product digest available yet)_\n'
  printf '\n### %s\n\n' "recent-decisions"
  $GSTACK_BIN/gstack-brain-cache get recent-decisions --project "$SLUG" 2>/dev/null || printf '_(no recent-decisions digest available yet)_\n'
} > /tmp/.gstack-brain-context-$$.md 2>/dev/null
[ -s /tmp/.gstack-brain-context-$$.md ] && cat /tmp/.gstack-brain-context-$$.md
rm -f /tmp/.gstack-brain-context-$$.md 2>/dev/null || true

How to use this context:

• If product digest names the value prop, target user, or stage — don't re-ask.
• If goals digest lists active goals — frame recommendations against them.
• If recent-decisions digest names a prior scope/architecture choice — flag if this plan contradicts.
• If user-profile digest carries calibration pattern statements ("tends to over-engineer security") — surface them when relevant.
• If a digest is (no X digest available yet), treat that section as cold; ask the user.

Privacy: Salience digest is filtered by allowlist (D9 default: projects/, gstack/, concepts/ only). Personal/family/therapy content never leaks here.

---

---

BEFORE YOU START:

Design Doc Check

setopt +o nomatch 2>/dev/null || true  # zsh compat
SLUG=$($GSTACK_BIN/remote-slug 2>/dev/null || basename "$(git rev-parse --show-toplevel 2>/dev/null || pwd)")
BRANCH=$(git rev-parse --abbrev-ref HEAD 2>/dev/null | tr '/' '-' || echo 'no-branch')
DESIGN=$(ls -t "${GSTACK_HOME:-$HOME/.gstack}"/projects/${PROJECT_ID:-unknown}/*-$BRANCH-design-*.md 2>/dev/null | head -1)
[ -z "$DESIGN" ] && DESIGN=$(ls -t "${GSTACK_HOME:-$HOME/.gstack}"/projects/${PROJECT_ID:-unknown}/*-design-*.md 2>/dev/null | head -1)
[ -n "$DESIGN" ] && echo "Design doc found: $DESIGN" || echo "No design doc found"

If a design doc exists, read it. Use it as the source of truth for the problem statement, constraints, and chosen approach. If it has a Supersedes: field, note that this is a revised design — check the prior version for context on what changed and why.

Prerequisite Skill Offer

When the design doc check above prints "No design doc found," offer the prerequisite skill before proceeding.

Say to the user via AskUserQuestion:

"No design doc found for this branch. $plan --mode Discovery --module office-hours produces a structured problem statement, premise challenge, and explored alternatives — it gives this review much sharper input to work with. Takes about 10 minutes. The design doc is per-feature, not per-product — it captures the thinking behind this specific change."

Options:

• A) Run $plan --mode Discovery --module office-hours now (we'll pick up the review right after)
• B) Skip — proceed with standard review

If they skip: "No worries — standard review. If you ever want sharper input, try $plan --mode Discovery --module office-hours first next time." Then proceed normally. Do not re-offer later in the session.

If they choose A:

Say: "Running $plan --mode Discovery --module office-hours inline. Once the design doc is ready, I'll pick up the review right where we left off."

Read the $plan --mode Discovery --module office-hours skill file at references/legacy/office-hours.md using the Read tool.

If unreadable: Skip with "Could not load $plan --mode Discovery --module office-hours — skipping." and continue.

Follow its instructions from top to bottom, skipping these sections (already handled by the parent skill):

• Preamble (run first)
• AskUserQuestion Format
• Completeness Principle — Boil the Ocean
• Search Before Building
• Contributor Mode
• Completion Status Protocol
• Telemetry (run last)
• Step 0: Detect platform and base branch
• Review Readiness Dashboard
• Plan File Review Report
• Prerequisite Skill Offer
• Plan Status Footer

Execute every other section at full depth. When the loaded skill's instructions are complete, continue with the next step below.

After $plan --mode Discovery --module office-hours completes, re-run the design doc check:

setopt +o nomatch 2>/dev/null || true  # zsh compat
SLUG=$($GSTACK_BIN/remote-slug 2>/dev/null || basename "$(git rev-parse --show-toplevel 2>/dev/null || pwd)")
BRANCH=$(git rev-parse --abbrev-ref HEAD 2>/dev/null | tr '/' '-' || echo 'no-branch')
DESIGN=$(ls -t "${GSTACK_HOME:-$HOME/.gstack}"/projects/${PROJECT_ID:-unknown}/*-$BRANCH-design-*.md 2>/dev/null | head -1)
[ -z "$DESIGN" ] && DESIGN=$(ls -t "${GSTACK_HOME:-$HOME/.gstack}"/projects/${PROJECT_ID:-unknown}/*-design-*.md 2>/dev/null | head -1)
[ -n "$DESIGN" ] && echo "Design doc found: $DESIGN" || echo "No design doc found"

If a design doc is now found, read it and continue the review. If none was produced (user may have cancelled), proceed with standard review.

Step 0: Scope Challenge

Reminder: the Scope gate at the top of this skill is a hard STOP. Do not run Step 0 until the user has answered it, and run it against the target they chose.

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.

4. Search check: For each architectural pattern, infrastructure component, or concurrency approach the plan introduces:

   • 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}"
   • Are there known footguns? Search: "{framework} {pattern} pitfalls"

   If WebSearch is unavailable, skip this check and note: "Search unavailable — proceeding with in-distribution knowledge only."

   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?

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.

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)? If the plan defers distribution, flag it explicitly in the "NOT in scope" section — don't let it silently drop.

If the complexity check triggers (8+ files or 2+ new classes/services), STOP before any review-section work. Call AskUserQuestion: name what's overbuilt, propose a minimal version that achieves the core goal, ask whether to reduce or proceed as-is. The AskUserQuestion call is a tool_use, not prose — call the tool directly.

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.

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.

Critical: Once the user accepts or rejects a scope reduction recommendation, commit fully. Do not re-argue for smaller scope during later review sections. Do not silently reduce scope or skip planned components.

Lazy specialist phase: review-sections

When the workflow reaches this phase, read references/sections/plan-eng-review/review-sections.md completely and execute it before continuing. Do not summarize or skip its questions, pressure, gates, evidence, artifacts, mutation boundary, or exit behavior.

Section self-check (before you finish)

Confirm you Read the review section the Section index named, and executed every review section (Architecture, Code Quality, Tests, Performance), the outside voice, and the required outputs in full. If you produced findings or the review report from memory without Reading references/sections/plan-eng-review/review-sections.md, stop and Read it now.

EXIT PLAN MODE GATE (BLOCKING)

Before calling ExitPlanMode, run this self-check. If any item fails, do the missing work — do NOT call ExitPlanMode:

1. Read the plan file with the Read tool (after your most recent write to it).
2. Confirm the LAST ## heading in the file is ## GSTACK REVIEW REPORT. In-body prose that mentions "outside voice", "codex findings", or similar does NOT count — only the structured ## GSTACK REVIEW REPORT section satisfies this check.
3. Confirm the report has a Runs / Status / Findings table and a VERDICT line (CODEX / CROSS-MODEL absorbed if applicable).
4. Confirm the report's FINAL non-whitespace line is the unresolved-decisions status: the exact unbolded NO UNRESOLVED DECISIONS, or a bullet of a final **UNRESOLVED DECISIONS:** block. BLOCKING, no "if applicable" escape — a bolded sentinel, any trailing CODEX/CROSS-MODEL/VERDICT/prose, or a missing status each FAILS the gate.
5. If a plan file is in context for this skill invocation: confirm gstack-review-log was called and gstack-review-read was run at least once. If no plan file is in context (e.g. $review --mode Deep --module codex consult against a diff with no plan), this check short-circuits — checks 1-4 already short-circuit when no plan file exists.

Failing this gate and calling ExitPlanMode anyway is a contract violation — the user will see a plan whose review report is missing or stale, and will (correctly) reject it. Self-deception failure mode to watch for: feeling "done" after writing review prose into the plan body. The body prose is not the report. The report is a separate, structured, table-bearing section that must be the file's terminal heading.

Upstream judgment port: PR #679

Match the user language

User-language rule

Write questions, progress updates, reports, and artifacts in the language used by the user. Source material, code identifiers, commands, and quotations may remain in their original language when translating them would reduce accuracy.

Upstream judgment port: PR #1071

Make normalized data models the default

Data-model judgment

Default to a normalized relational model. Denormalization needs a measured performance reason plus a consistency plan. A JSON field is appropriate for genuinely opaque or externally owned payloads, but not as an escape hatch for known, stable variants that deserve typed columns or tables. The engineering review must state entities, ownership, cardinality, constraints, indexes, migration/backfill, rollback, and how invalid combinations are prevented.

Upstream judgment port: PR #2030

Record only signal-bearing learnings

Signal-gated learning

Persist a learning only when the interaction contains a useful, reusable signal such as an explicit preference, correction, accepted recommendation, or rejected direction. Track helpful and harmful outcomes separately. Do not manufacture a learning merely because a workflow completed.