## Step 9: Pre-Landing Review Review the diff for structural issues that tests don't catch. 1. Read `.claude/skills/review/checklist.md`. If the file cannot be read, **STOP** and report the error. 2. Run `git diff origin/` to get the full diff (scoped to feature changes against the freshly-fetched base branch). 3. Apply the review checklist in two passes: - **Pass 1 (CRITICAL):** SQL & Data Safety, LLM Output Trust Boundary - **Pass 2 (INFORMATIONAL):** All remaining categories ## Confidence Calibration Every finding MUST include a confidence score (1-10): | Score | Meaning | Display rule | |-------|---------|-------------| | 9-10 | Verified by reading specific code. Concrete bug or exploit demonstrated. | Show normally | | 7-8 | High confidence pattern match. Very likely correct. | Show normally | | 5-6 | Moderate. Could be a false positive. | Show with caveat: "Medium confidence, verify this is actually an issue" | | 3-4 | Low confidence. Pattern is suspicious but may be fine. | Suppress from main report. Include in appendix only. | | 1-2 | Speculation. | Only report if severity would be P0. | **Finding format:** \`[SEVERITY] (confidence: N/10) file:line — description\` Example: \`[P1] (confidence: 9/10) app/models/user.rb:42 — SQL injection via string interpolation in where clause\` \`[P2] (confidence: 5/10) app/controllers/api/v1/users_controller.rb:18 — Possible N+1 query, verify with production logs\` ### Pre-emit verification gate (#1539 — kills the "field doesn't exist" FP class) Before any finding is promoted to the report, the gate requires: 1. **Quote the specific code line that motivates the finding** — file:line plus the verbatim text of the line(s) that triggered it. If the finding is "field X doesn't exist on model Y", quote the lines of class Y where the field would live. If "dict.get() might return None", quote the dict initialization. If "race condition between A and B", quote both A and B. 2. **If you cannot quote the motivating line(s), the finding is unverified.** Force its confidence to 4-5 (suppressed from the main report). It still goes into the appendix so reviewers can audit calibration, but the user does NOT see it in the critical-pass output. Do not work around this by inventing speculative confidence 7+ — that defeats the gate. **Framework-meta nudge:** When the symbol is generated by a framework metaclass, descriptor, ORM Meta inner-class, or migration history (Django `Meta`, Rails `has_many`/`scope`, SQLAlchemy `relationship`/`Column`, TypeORM decorators, Sequelize `init`/`belongsTo`, Prisma generated client), quote the meta-construct (the `Meta` block, the migration, the decorator, the schema file) instead of expecting the literal name in the class body. The verification is "I read the source that creates this symbol", not "I grep'd for the name and didn't find it." Deeper framework-aware verification (model introspection, migration-history-aware checks, ORM dialect detection) is deliberately out of scope for the lighter gate — see the deferred `~/.gstack-dev/plans/1539-framework-aware-review.md` design doc. The FP classes the gate kills (measured against Django Sprint 2.5 #1539): | FP class | Why the gate catches it | |---|---| | "field doesn't exist on model" | Requires quoting the model class body or Meta; the field's absence becomes obvious | | "dict.get() might be None" | Requires quoting the dict initialization (e.g. Django form's `cleaned_data` is `{}`-initialized) | | "save() might lose fields" | Requires quoting the ORM signature or model definition | | "update_fields might miss X" | Requires quoting the field set; if X doesn't exist, the FP is self-evident | **Calibration learning:** If you report a finding with confidence < 7 and the user confirms it IS a real issue, that is a calibration event. Your initial confidence was too low. Log the corrected pattern as a learning so future reviews catch it with higher confidence. ## Design Review (conditional, diff-scoped) Check if the diff touches frontend files using `gstack-diff-scope`: ```bash source <(~/.claude/skills/gstack/bin/gstack-diff-scope 2>/dev/null) ``` **If `SCOPE_FRONTEND=false`:** Skip design review silently. No output. **If `SCOPE_FRONTEND=true`:** 1. **Check for DESIGN.md.** If `DESIGN.md` or `design-system.md` exists in the repo root, read it. All design findings are calibrated against it — patterns blessed in DESIGN.md are not flagged. If not found, use universal design principles. 2. **Read `.claude/skills/review/design-checklist.md`.** If the file cannot be read, skip design review with a note: "Design checklist not found — skipping design review." 3. **Read each changed frontend file** (full file, not just diff hunks). Frontend files are identified by the patterns listed in the checklist. 4. **Apply the design checklist** against the changed files. For each item: - **[HIGH] mechanical CSS fix** (`outline: none`, `!important`, `font-size < 16px`): classify as AUTO-FIX - **[HIGH/MEDIUM] design judgment needed**: classify as ASK - **[LOW] intent-based detection**: present as "Possible — verify visually or run /design-review" 5. **Include findings** in the review output under a "Design Review" header, following the output format in the checklist. Design findings merge with code review findings into the same Fix-First flow. 6. **Log the result** for the Review Readiness Dashboard: ```bash ~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"design-review-lite","timestamp":"TIMESTAMP","status":"STATUS","findings":N,"auto_fixed":M,"commit":"COMMIT"}' ``` Substitute: TIMESTAMP = ISO 8601 datetime, STATUS = "clean" if 0 findings or "issues_found", N = total findings, M = auto-fixed count, COMMIT = output of `git rev-parse --short HEAD`. 7. **Codex design voice** (optional, automatic if available): ```bash command -v codex >/dev/null 2>&1 && echo "CODEX_AVAILABLE" || echo "CODEX_NOT_AVAILABLE" ``` If Codex is available, run a lightweight design check on the diff: ```bash TMPERR_DRL=$(mktemp /tmp/codex-drl-XXXXXXXX) _REPO_ROOT=$(git rev-parse --show-toplevel) || { echo "ERROR: not in a git repo" >&2; exit 1; } codex exec "Review the git diff on this branch. Run 7 litmus checks (YES/NO each): 1. Brand/product unmistakable in first screen? 2. One strong visual anchor present? 3. Page understandable by scanning headlines only? 4. Each section has one job? 5. Are cards actually necessary? 6. Does motion improve hierarchy or atmosphere? 7. Would design feel premium with all decorative shadows removed? Flag any hard rejections: 1. Generic SaaS card grid as first impression 2. Beautiful image with weak brand 3. Strong headline with no clear action 4. Busy imagery behind text 5. Sections repeating same mood statement 6. Carousel with no narrative purpose 7. App UI made of stacked cards instead of layout 5 most important design findings only. Reference file:line." -C "$_REPO_ROOT" -s read-only -c 'model_reasoning_effort="high"' --enable web_search_cached < /dev/null 2>"$TMPERR_DRL" ``` Use a 5-minute timeout (`timeout: 300000`). After the command completes, read stderr: ```bash cat "$TMPERR_DRL" && rm -f "$TMPERR_DRL" ``` **Error handling:** All errors are non-blocking. On auth failure, timeout, or empty response — skip with a brief note and continue. Present Codex output under a `CODEX (design):` header, merged with the checklist findings above. Include any design findings alongside the code review findings. They follow the same Fix-First flow below. ## Step 9.1: Review Army — Specialist Dispatch ### Detect stack and scope ```bash source <(~/.claude/skills/gstack/bin/gstack-diff-scope 2>/dev/null) || true # Detect stack for specialist context STACK="" [ -f Gemfile ] && STACK="${STACK}ruby " [ -f package.json ] && STACK="${STACK}node " [ -f requirements.txt ] || [ -f pyproject.toml ] && STACK="${STACK}python " [ -f go.mod ] && STACK="${STACK}go " [ -f Cargo.toml ] && STACK="${STACK}rust " echo "STACK: ${STACK:-unknown}" DIFF_BASE=$(git merge-base origin/ HEAD) DIFF_INS=$(git diff "$DIFF_BASE" --stat | tail -1 | grep -oE '[0-9]+ insertion' | grep -oE '[0-9]+' || echo "0") DIFF_DEL=$(git diff "$DIFF_BASE" --stat | tail -1 | grep -oE '[0-9]+ deletion' | grep -oE '[0-9]+' || echo "0") DIFF_LINES=$((DIFF_INS + DIFF_DEL)) echo "DIFF_LINES: $DIFF_LINES" # Detect test framework for specialist test stub generation TEST_FW="" { [ -f jest.config.ts ] || [ -f jest.config.js ]; } && TEST_FW="jest" [ -f vitest.config.ts ] && TEST_FW="vitest" { [ -f spec/spec_helper.rb ] || [ -f .rspec ]; } && TEST_FW="rspec" { [ -f pytest.ini ] || [ -f conftest.py ]; } && TEST_FW="pytest" [ -f go.mod ] && TEST_FW="go-test" echo "TEST_FW: ${TEST_FW:-unknown}" ``` ### Read specialist hit rates (adaptive gating) ```bash ~/.claude/skills/gstack/bin/gstack-specialist-stats 2>/dev/null || true ``` ### Select specialists Based on the scope signals above, select which specialists to dispatch. **Always-on (dispatch on every review with 50+ changed lines):** 1. **Testing** — read `~/.claude/skills/gstack/review/specialists/testing.md` 2. **Maintainability** — read `~/.claude/skills/gstack/review/specialists/maintainability.md` **If DIFF_LINES < 50:** Skip all specialists. Print: "Small diff ($DIFF_LINES lines) — specialists skipped." Continue to the Fix-First flow (item 4). **Conditional (dispatch if the matching scope signal is true):** 3. **Security** — if SCOPE_AUTH=true, OR if SCOPE_BACKEND=true AND DIFF_LINES > 100. Read `~/.claude/skills/gstack/review/specialists/security.md` 4. **Performance** — if SCOPE_BACKEND=true OR SCOPE_FRONTEND=true. Read `~/.claude/skills/gstack/review/specialists/performance.md` 5. **Data Migration** — if SCOPE_MIGRATIONS=true. Read `~/.claude/skills/gstack/review/specialists/data-migration.md` 6. **API Contract** — if SCOPE_API=true. Read `~/.claude/skills/gstack/review/specialists/api-contract.md` 7. **Design** — if SCOPE_FRONTEND=true. Use the existing design review checklist at `~/.claude/skills/gstack/review/design-checklist.md` ### Adaptive gating After scope-based selection, apply adaptive gating based on specialist hit rates: For each conditional specialist that passed scope gating, check the `gstack-specialist-stats` output above: - If tagged `[GATE_CANDIDATE]` (0 findings in 10+ dispatches): skip it. Print: "[specialist] auto-gated (0 findings in N reviews)." - If tagged `[NEVER_GATE]`: always dispatch regardless of hit rate. Security and data-migration are insurance policy specialists — they should run even when silent. **Force flags:** If the user's prompt includes `--security`, `--performance`, `--testing`, `--maintainability`, `--data-migration`, `--api-contract`, `--design`, or `--all-specialists`, force-include that specialist regardless of gating. Note which specialists were selected, gated, and skipped. Print the selection: "Dispatching N specialists: [names]. Skipped: [names] (scope not detected). Gated: [names] (0 findings in N+ reviews)." --- ### Dispatch specialists in parallel For each selected specialist, launch an independent subagent via the Agent tool. **Launch ALL selected specialists in a single message** (multiple Agent tool calls) so they run in parallel. Each subagent has fresh context — no prior review bias. **Each specialist subagent prompt:** Construct the prompt for each specialist. The prompt includes: 1. The specialist's checklist content (you already read the file above) 2. Stack context: "This is a {STACK} project." 3. Past learnings for this domain (if any exist): ```bash ~/.claude/skills/gstack/bin/gstack-learnings-search --type pitfall --query "{specialist domain}" --limit 5 2>/dev/null || true ``` If learnings are found, include them: "Past learnings for this domain: {learnings}" 4. Instructions: "You are a specialist code reviewer. Read the checklist below, then run `DIFF_BASE=$(git merge-base origin/ HEAD) && git diff "$DIFF_BASE"` to get the full diff. Apply the checklist against the diff. For each finding, output a JSON object on its own line: {\"severity\":\"CRITICAL|INFORMATIONAL\",\"confidence\":N,\"path\":\"file\",\"line\":N,\"category\":\"category\",\"summary\":\"description\",\"fix\":\"recommended fix\",\"fingerprint\":\"path:line:category\",\"specialist\":\"name\"} Required fields: severity, confidence, path, category, summary, specialist. Optional: line, fix, fingerprint, evidence, test_stub. If you can write a test that would catch this issue, include it in the `test_stub` field. Use the detected test framework ({TEST_FW}). Write a minimal skeleton — describe/it/test blocks with clear intent. Skip test_stub for architectural or design-only findings. If no findings: output `NO FINDINGS` and nothing else. Do not output anything else — no preamble, no summary, no commentary. Stack context: {STACK} Past learnings: {learnings or 'none'} CHECKLIST: {checklist content}" **Subagent configuration:** - Use `subagent_type: "general-purpose"` - Do NOT use `run_in_background` — all specialists must complete before merge - If any specialist subagent fails or times out, log the failure and continue with results from successful specialists. Specialists are additive — partial results are better than no results. --- ### Step 9.2: Collect and merge findings After all specialist subagents complete, collect their outputs. **Parse findings:** For each specialist's output: 1. If output is "NO FINDINGS" — skip, this specialist found nothing 2. Otherwise, parse each line as a JSON object. Skip lines that are not valid JSON. 3. Collect all parsed findings into a single list, tagged with their specialist name. **Fingerprint and deduplicate:** For each finding, compute its fingerprint: - If `fingerprint` field is present, use it - Otherwise: `{path}:{line}:{category}` (if line is present) or `{path}:{category}` Group findings by fingerprint. For findings sharing the same fingerprint: - Keep the finding with the highest confidence score - Tag it: "MULTI-SPECIALIST CONFIRMED ({specialist1} + {specialist2})" - Boost confidence by +1 (cap at 10) - Note the confirming specialists in the output **Apply confidence gates:** - Confidence 7+: show normally in the findings output - Confidence 5-6: show with caveat "Medium confidence — verify this is actually an issue" - Confidence 3-4: move to appendix (suppress from main findings) - Confidence 1-2: suppress entirely **Compute PR Quality Score:** After merging, compute the quality score: `quality_score = max(0, 10 - (critical_count * 2 + informational_count * 0.5))` Cap at 10. Log this in the review result at the end. **Output merged findings:** Present the merged findings in the same format as the current review: ``` SPECIALIST REVIEW: N findings (X critical, Y informational) from Z specialists [For each finding, in order: CRITICAL first, then INFORMATIONAL, sorted by confidence descending] [SEVERITY] (confidence: N/10, specialist: name) path:line — summary Fix: recommended fix [If MULTI-SPECIALIST CONFIRMED: show confirmation note] PR Quality Score: X/10 ``` These findings flow into the Fix-First flow (item 4) alongside the checklist pass (Step 9). The Fix-First heuristic applies identically — specialist findings follow the same AUTO-FIX vs ASK classification. **Compile per-specialist stats:** After merging findings, compile a `specialists` object for the review-log persist. For each specialist (testing, maintainability, security, performance, data-migration, api-contract, design, red-team): - If dispatched: `{"dispatched": true, "findings": N, "critical": N, "informational": N}` - If skipped by scope: `{"dispatched": false, "reason": "scope"}` - If skipped by gating: `{"dispatched": false, "reason": "gated"}` - If not applicable (e.g., red-team not activated): omit from the object Include the Design specialist even though it uses `design-checklist.md` instead of the specialist schema files. Remember these stats — you will need them for the review-log entry in Step 5.8. --- ### Red Team dispatch (conditional) **Activation:** Only if DIFF_LINES > 200 OR any specialist produced a CRITICAL finding. If activated, dispatch one more subagent via the Agent tool (foreground, not background). The Red Team subagent receives: 1. The red-team checklist from `~/.claude/skills/gstack/review/specialists/red-team.md` 2. The merged specialist findings from Step 9.2 (so it knows what was already caught) 3. The git diff command Prompt: "You are a red team reviewer. The code has already been reviewed by N specialists who found the following issues: {merged findings summary}. Your job is to find what they MISSED. Read the checklist, run `DIFF_BASE=$(git merge-base origin/ HEAD) && git diff "$DIFF_BASE"`, and look for gaps. Output findings as JSON objects (same schema as the specialists). Focus on cross-cutting concerns, integration boundary issues, and failure modes that specialist checklists don't cover." If the Red Team finds additional issues, merge them into the findings list before the Fix-First flow (item 4). Red Team findings are tagged with `"specialist":"red-team"`. If the Red Team returns NO FINDINGS, note: "Red Team review: no additional issues found." If the Red Team subagent fails or times out, skip silently and continue. ### Step 9.3: Cross-review finding dedup Before classifying findings, check if any were previously skipped by the user in a prior review on this branch. ```bash ~/.claude/skills/gstack/bin/gstack-review-read ``` Parse the output: only lines BEFORE `---CONFIG---` are JSONL entries (the output also contains `---CONFIG---` and `---HEAD---` footer sections that are not JSONL — ignore those). For each JSONL entry that has a `findings` array: 1. Collect all fingerprints where `action: "skipped"` 2. Note the `commit` field from that entry If skipped fingerprints exist, get the list of files changed since that review: ```bash git diff --name-only HEAD ``` For each current finding (from both the checklist pass (Step 9) and specialist review (Step 9.1-9.2)), check: - Does its fingerprint match a previously skipped finding? - Is the finding's file path NOT in the changed-files set? If both conditions are true: suppress the finding. It was intentionally skipped and the relevant code hasn't changed. Print: "Suppressed N findings from prior reviews (previously skipped by user)" **Only suppress `skipped` findings — never `fixed` or `auto-fixed`** (those might regress and should be re-checked). If no prior reviews exist or none have a `findings` array, skip this step silently. Output a summary header: `Pre-Landing Review: N issues (X critical, Y informational)` 4. **Classify each finding from both the checklist pass and specialist review (Step 9.1-Step 9.2) as AUTO-FIX or ASK** per the Fix-First Heuristic in checklist.md. Critical findings lean toward ASK; informational lean toward AUTO-FIX. 5. **Auto-fix all AUTO-FIX items.** Apply each fix. Output one line per fix: `[AUTO-FIXED] [file:line] Problem → what you did` 6. **If ASK items remain,** present them in ONE AskUserQuestion: - List each with number, severity, problem, recommended fix - Per-item options: A) Fix B) Skip - Overall RECOMMENDATION - If 3 or fewer ASK items, you may use individual AskUserQuestion calls instead 7. **After all fixes (auto + user-approved):** - If ANY fixes were applied: commit fixed files by name (`git add && git commit -m "fix: pre-landing review fixes"`), then **STOP** and tell the user to run `/ship` again to re-test. - If no fixes applied (all ASK items skipped, or no issues found): continue to Step 12. 8. Output summary: `Pre-Landing Review: N issues — M auto-fixed, K asked (J fixed, L skipped)` If no issues found: `Pre-Landing Review: No issues found.` 9. Persist the review result to the review log: ```bash ~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"review","timestamp":"TIMESTAMP","status":"STATUS","issues_found":N,"critical":N,"informational":N,"quality_score":SCORE,"specialists":SPECIALISTS_JSON,"findings":FINDINGS_JSON,"commit":"'"$(git rev-parse --short HEAD)"'","via":"ship"}' ``` Substitute TIMESTAMP (ISO 8601), STATUS ("clean" if no issues, "issues_found" otherwise), and N values from the summary counts above. The `via:"ship"` distinguishes from standalone `/review` runs. - `quality_score` = the PR Quality Score computed in Step 9.2 (e.g., 7.5). If specialists were skipped (small diff), use `10.0` - `specialists` = the per-specialist stats object compiled in Step 9.2. Each specialist that was considered gets an entry: `{"dispatched":true/false,"findings":N,"critical":N,"informational":N}` if dispatched, or `{"dispatched":false,"reason":"scope|gated"}` if skipped. Example: `{"testing":{"dispatched":true,"findings":2,"critical":0,"informational":2},"security":{"dispatched":false,"reason":"scope"}}` - `findings` = array of per-finding records. For each finding (from checklist pass and specialists), include: `{"fingerprint":"path:line:category","severity":"CRITICAL|INFORMATIONAL","action":"ACTION"}`. ACTION is `"auto-fixed"`, `"fixed"` (user approved), or `"skipped"` (user chose Skip). Save the review output — it goes into the PR body in Step 19. ---