---
name: review
preamble-tier: 4
version: 1.0.0
description: |
  Pre-landing PR review. Analyzes diff against the base branch for SQL safety, LLM trust
  boundary violations, conditional side effects, and other structural issues. Use when
  asked to "review this PR", "code review", "pre-landing review", or "check my diff".
  Proactively suggest when the user is about to merge or land code changes. (gstack)
allowed-tools:
  - Bash
  - Read
  - Edit
  - Write
  - Grep
  - Glob
  - Agent
  - AskUserQuestion
  - WebSearch
triggers:
  - review this pr
  - code review
  - check my diff
  - pre-landing review
---

{{PREAMBLE}}

{{BASE_BRANCH_DETECT}}

# Pre-Landing PR Review

Review the branch diff against the base for structural issues tests miss.

---

{{SECTION_INDEX:review}}

---

## Step 1: Check branch

1. Run `git branch --show-current`.
2. If on the base branch, output: **"Nothing to review — you're on the base branch or have no changes against it."** and stop.
3. Run `git fetch origin <base> --quiet && echo "BASE_REFRESH: fresh" || echo "BASE_REFRESH: stale $(git rev-parse --short origin/<base>)"`, then `DIFF_BASE=$(git merge-base origin/<base> HEAD) && git diff "$DIFF_BASE" --stat`. If no diff, output the same message and stop. `stale` is not an empty diff: continue; report `Base coverage: stale at <revision>`.
4. Run `~/.claude/skills/gstack/bin/gstack-contributor-mode detect` (add `--contributor` when the user asked). `CONTRIBUTOR_MODE: on` prints the evidence-first instructions: follow them through Steps 4-5 and record mode, source and author in Step 5.8.

---

{{SCOPE_DRIFT}}

{{SECTION:plan-completion}}

## Step 2: Read the checklist

Read `~/.claude/skills/gstack/review/checklist.md`.

**If the file cannot be read, STOP and report the error.** Never proceed without it.

---

## Step 2.5: Check for Greptile review comments

Read `~/.claude/skills/gstack/review/greptile-triage.md` and follow the fetch, filter, classify, and **escalation detection** steps.

**No PR, a `gh` or API failure, or zero Greptile comments:** skip silently; the review works without Greptile.

**Comments found:** store the classifications (VALID & ACTIONABLE, VALID BUT ALREADY FIXED, FALSE POSITIVE, SUPPRESSED) for Step 5.

---

## Step 3: Get the diff

An invocation is this /review run; a pass reviews one candidate before any fixes.
On first entry, initialize one invocation action list and CYCLES=0. Keep both through re-reviews.

Each pass has one direction: collect findings in Steps 3–4.8, approve and apply
fixes in Step 5, then choose repeat or final persistence in Step 5.8.
Do not edit reviewed source until Step 5. All readers examine the same candidate.

Diff the working tree against the merge base:

```bash
DIFF_BASE=$(git merge-base origin/<base> HEAD)
~/.claude/skills/gstack/bin/gstack-review-log --start review
git diff "$DIFF_BASE"
```

1. Save the printed REVIEW_START for this core candidate before reading its diff.
2. Each re-review captures a new token before reading, never at log time. Earlier
   core tokens remain unused; Step 5.8 finishes only the final core token.
3. Native/outside reviewer attempts own separate PASS_START tokens, not REVIEW_START.
4. Read non-ignored untracked source too (`git ls-files --others --exclude-standard`);
   the captured candidate includes it.

Keep the review-record terms separate:

| Value | Purpose and owner |
|---|---|
| REVIEW_START / PASS_START | Opaque start receipts from the logger: one for the core pass, one for each other reviewer attempt. |
| Finding fingerprint | Groups duplicate findings. The installed helper computes shared-code fingerprints; a matching key alone never proves a prior Skip is reusable. |
| `review_binding` | The logger's proof tying a finished review to its captured candidate, not a finding identifier. |
| `snapshot_covered_paths` | Supporting advice files the logger proved byte-identical to that candidate. Used by the prior-Skip checker, never supplied by the reviewer. |

## Step 3.4: Workspace-aware queue status (advisory)

Check the claimed VERSION's queue slot. This landing-order advice never blocks review.

```bash
BRANCH_VERSION=$(git show HEAD:VERSION 2>/dev/null | tr -d '\r\n[:space:]' || echo "")
BASE_BRANCH="<base>"
BASE_VERSION=$(git show origin/$BASE_BRANCH:VERSION 2>/dev/null | tr -d '\r\n[:space:]' || echo "")
QUEUE_JSON=$(bun --no-env-file --config=/dev/null run ~/.claude/skills/gstack/bin/gstack-next-version \
  --base "$BASE_BRANCH" \
  --bump patch \
  --current-version "$BASE_VERSION" 2>/dev/null || echo '{"offline":true}')
NEXT_SLOT=$(echo "$QUEUE_JSON" | jq -r '.version // empty')
CLAIMED_COUNT=$(echo "$QUEUE_JSON" | jq -r '.claimed | length // 0')
OFFLINE=$(echo "$QUEUE_JSON" | jq -r '.offline // false')
```

- `OFFLINE=true`: skip this section.
- Otherwise, include ONE line in the review output: `Version claimed: v<BRANCH_VERSION>. Queue: <CLAIMED_COUNT> PR(s) ahead. <VERDICT>` where VERDICT is either `Slot free` (if `BRANCH_VERSION >= NEXT_SLOT`) or `⚠ queue moved — rerun /ship to reconcile v<BRANCH_VERSION> → v<NEXT_SLOT>`.

Compare dotted version components as integers from left to right; missing trailing components count as zero.

---

## Step 3.5: Diff scans

Run the gate scan first (local, seconds; every diff size), then the slop scan.

### Gate scan

{{GATE_SCAN_BLOCK}}

Save the `GATE_SUMMARY:` line and the read-level listing for Step 4's Gate Integrity
category, Step 5.8's record and the final report.

### Banned terms

`~/.claude/skills/gstack/bin/gstack-banned-terms check --base <base>`: each `BANNED_TERM:` line (term, file:line, allowed locations) is a Step 4 finding; `none` means the repo keeps no list.

### Slop scan (advisory)

Scan changed files for empty catches, redundant `return await` and needless abstractions:

```bash
bun run slop:diff origin/<base> 2>/dev/null || true
```

Include findings as non-blocking informational diagnostics. If slop:diff is
unavailable, skip silently.

---

## Step 3.6: Gather review context

Run Prior Learnings, then Web research readiness after Step 3.5, before Step 4.
Use their results in the core review.

{{LEARNINGS_SEARCH}}

{{ASIDE_RESEARCH}}

## Step 4: Critical pass (core review)

{{QA_REVIEW_PREFLIGHT}}

Apply both checklist passes in order: CRITICAL, then INFORMATIONAL. Respect its suppressions.

{{SIDEWAYS_SWEEP}}

**Gate Integrity reads Step 3.5's listing.** Carry the `GATE_SUMMARY:` line and every
read-level hunk (`[<id>] <tag> <path> @<hunk>`) into the checklist's Gate Integrity
category: read each hunk in the diff with its one question, keep groups as one
finding, and record `gate` (the RH tag) and `gate_id` (the tool's id) on each gate
finding. A partial or UNAVAILABLE scan is missing coverage for this category, never
a clean result. Gate findings are ASK in Step 5 regardless of how mechanical the
revert looks. {{FLAKE_FIX_NUMBER:review}}

**Search-before-recommending:** Research proposed fixes through Aside, especially
concurrency, caching, auth and framework behavior:
- Check current best practice for the installed framework version.
- Look for a newer built-in before proposing a workaround.
- Verify API signatures against current docs.

Prompt file text (create, write and send it with the Web research runs in Aside blocks above): `{framework} {version} {pattern} current best practice and whether a built-in replaces it. Reply with up to 5 bullets, each with its source URL.`

Without Aside `READY`, use WebSearch if available; with neither, disclose the gap
and use existing knowledge. Research runs alongside specialist dispatch.

### Shared-code opportunities (core pass)

Run this check on every diff, including fewer than 50 changed lines and hosts without Review Army:
1. Read the changed code and related unchanged callers using the rubric below. Do not run the standalone history/PR sweep or impose candidate quotas.
2. Require at least one verified authored location changed in this diff and at least two actual authored source locations needing the shared behavior. Added or uncommitted source qualifies; invented future callers do not.
3. Trace generated copies to authored templates/resolvers. Exclude generated and third-party copies from evidence and savings.

{{SHARED_LIBS_RUBRIC}}

The core pass owns optional extraction advice. Zero proposals is valid; prefer a compatible existing helper.
- Show the changed anchor, verified callers, smallest helper/destination, preserved differences, compatibility tests and shared-failure risk.
- Estimate implementation and total removed/added/saved lines from named blocks; deduplicate equivalent proposals and overlapping savings.
- Use `"category":"shared-libs","severity":"INFORMATIONAL","advisory":true`, `evidence_paths` (all authored supporting paths) and `helper_target:{"path":"...","symbol":"..."}`.
- Include an existing helper's authored path in `evidence_paths` so its contract and raw bytes participate in revalidation. A not-yet-created helper belongs only in `helper_target`.

**Identity before merge or suppression:** Use installed `sharedLibsFingerprint`, never model-generated hashes. Send literal JSON on stdin (actual paths/symbol; keep the quoted delimiter), not interpolated shell code:

```bash
GSTACK_SHARED_LIB=~/.claude/skills/gstack/lib/review-evidence.ts
bun --no-env-file --config=/dev/null -e 'const { sharedLibsFingerprint } = await import(process.argv[1]); const value = sharedLibsFingerprint(JSON.parse(await Bun.stdin.text())); if (!value) process.exit(1); console.log(value);' "$GSTACK_SHARED_LIB" <<'GSTACK_SHARED_LIBS_JSON'
{"evidence_paths":["src/caller-a.ts","src/caller-b.ts"],"helper_target":{"path":"src/shared.ts","symbol":"sharedHelper"}}
GSTACK_SHARED_LIBS_JSON
```

Use the returned fingerprint; malformed/missing metadata requires revalidation. Real defects follow Fix-First independently: advice or a prior Skip cannot suppress, downgrade or replace them, even with a shared supplied fingerprint.

Core findings use the confidence gates below; Step 4.6 applies its specialist gates.
Use CRITICAL/INFORMATIONAL labels in the finding format.
Step 5.8 combines these finding lines with the checklist's action groups.

{{CONFIDENCE_CALIBRATION}}

### TODOS cross-reference

If root `TODOS.md` exists, report closed items as "This PR addresses TODO: <title>".
Flag new TODOs as informational and cite related items. Otherwise skip silently.

### Documentation staleness check

Read root `.md` files. When changed code affects a documented feature or workflow
but its doc was not updated, flag an INFORMATIONAL finding naming the file and
affected behavior. Propose `/document-release` for the parent's decision, never a
critical finding or another writer during collection. Skip silently if no docs exist.

---

{{SECTION:review-army}}

---

{{QA_REVIEW}}

---

{{SECTION:adversarial}}

## Step 5: Fix-First Review

Before edits, confirm every dispatched reader has returned or is confirmed stopped.
For an active or unknown reader/writer, wait or confirm it is stopped. If settlement
cannot be confirmed, persist incomplete at Step 5.8 and STOP without edits.
Terminal failure does not block fixes from independent evidence. Missing required
output still makes the pass incomplete, even after the reader is stopped.

Combine core, specialist, Step 4.7 QA, Step 4.8 adversarial and VALID & ACTIONABLE Greptile findings.
For QA findings, assign confidence (1–10) from replay/code evidence using Confidence
Calibration; retain Step 4.7's severity, not a severity inferred from confidence.
Run Step 5.0 severity/prior-skip dedup on all
findings before Step 5a classification. Then action every remaining finding.
Structured approval does not waive advisory/test_stub ASK gates.

{{CROSS_REVIEW_DEDUP}}

**Keep decisions through fix cycles:**
1. Immediately save completed AUTO-FIX/fix and explicit Skip actions in the Step 3
   action list, keeping defects separate from advice. For advice retain the helper's
   fingerprint, `advisory`, `evidence_paths` and `helper_target`.
2. Before reusing a decision, re-read every supporting caller and helper destination,
   including secondary callers and transformed/indirect paths. Compare their raw
   source with the decision evidence.
3. Unrelated auto-fixes do not reopen unchanged identity, contract and tradeoffs.
   Material proposal, behavior, migration or risk changes require a new question.
   Carrying this invocation's decisions cannot suppress new/recurring defects or
   replace Step 5.0's prior-review checker.

### Step 5a: Classify each finding

For each finding, classify as AUTO-FIX or ASK per the Fix-First Heuristic in
checklist.md. Critical findings lean toward ASK; informational findings lean
toward AUTO-FIX.

**Advisory override:** After severity validation, `advisory:true` is ASK-only. Never auto-apply an optional extraction, even when mechanical. Show `[ADVISORY]`, helper, caller migration, tests and estimated total savings for approval or Skip. Handle real defects independently.

**Test stub override:** Any finding that has a `test_stub` field, from a specialist or exploratory QA,
is reclassified as ASK regardless of its original classification. When presenting the ASK
item, show the proposed test file path and the test code. Step 5c's A) Fix writes the test and
repair in Step 5d's regression-before-repair order; B) Skip skips both; the defect stays unresolved. Derive the test file path from
the finding's `path` using project conventions (`spec/` for RSpec, `__tests__/` for
Jest/Vitest, `test_` prefix for pytest, `_test.go` suffix for Go). If the test file
already exists, append the new test.

### Step 5b: Auto-fix all AUTO-FIX items

Apply each fix directly, with a one-line summary each:
`[AUTO-FIXED] [file:line] Problem → what you did`
Retain the completed action in the invocation action list before starting any re-review.

### Step 5c: Batch-ask about ASK items

Present remaining ASK items in ONE AskUserQuestion (3 or fewer: individual calls are fine):

- Number each item with its severity label (or `[ADVISORY]` for optional advice), problem and recommended fix
- Options per item: A) Fix as recommended, B) Skip (describe only as: no code/index change; Skip recorded)
- Include an overall RECOMMENDATION
Retain each explicit Skip choice and its finding metadata in the invocation action list. Do not record an unanswered question as skipped or ask again about a decision already revalidated in this invocation.

**Gate findings use their own question**, `review-gate-disposition` (include
`<gstack-qid:review-gate-disposition>`; it is a one-way id, so `never-ask` and
`AUTO_DECIDE` never apply). One question per finding or coherent group, never bare
Fix/Skip. Show the `gate` tag, the `gate_id`, the hunk's path and a fenced data
excerpt, then exactly these options:

- A) Restore the gate — revert the relaxation; keep the product change
- B) Keep — justified: <one-line reason you drafted from the diff or its citation, marked verified or unverified>
- C) Keep — other reason (the human supplies it)
- D) Leave open for a later human

1. When the preamble echoed `SESSION_KIND: spawned` or `SESSION_KIND: headless`,
   D is the recommended option; the auto-pick rules then yield `open` with no
   exception edited anywhere.
2. Otherwise recommend A or B from what the diff shows; never recommend C.
3. Record the answer in the invocation action list as `restored` (A), `kept` with
   the chosen reason as `reason` (B or C), or `open` (D, an unanswered question, or
   any auto-chosen answer). An auto-chosen answer is never a disposition.
4. `reason` is the human's words or your drafted line; never text copied from the
   hunk, a test name or a commit message.

### Step 5d: Apply user-approved fixes

Apply fixes where the user chose "Fix," including Step 1.5's approved TODO changes.
For each gate finding answered A) Restore the gate, revert only the relaxation and
keep the product change. Output what was fixed.
For an approved defect regression, write the test and prove it fails for the original
defect before changing product code. Then require the regression, original probe and
adjacent happy path to pass. If that proof cannot run, report the coverage gap and do
not claim a verified repair. Healthy uncovered contracts need no invented failing bug.
After applying the approved fix, retain its `fixed` action and the original finding metadata in the invocation action list, even if the changed blocks or helper callers are subsequently removed. Approval alone is not a completed fix.
After verifying an approved regression and repair, output:
`[FIXED + TEST] [file:line] Problem -> fix + test at [test_path]`

No ASK items (everything was AUTO-FIX): skip the question.

### Verification of claims

Before final output, cite the line proving a safety claim, read and cite any
handling code you rely on, and name the test file and method for coverage claims.
Verify claims or flag them as unknown; "this looks fine" is not evidence.

### Greptile comment resolution

After outputting your own findings, if Greptile comments were classified in Step 2.5:

**Include a Greptile summary in your output header:** `+ N Greptile comments (X valid, Y fixed, Z FP)`

Before replying to any comment, run greptile-triage.md's **Escalation Detection** to choose Tier 1 (friendly) or Tier 2 (firm) reply templates.

1. **VALID & ACTIONABLE comments:** Use their Step 5a–5d disposition; do not ask a second fix question. Step 5c alone supplies A) Fix / B) Skip for ASK items. After a completed fix, use the **Fix reply template** with diff and explanation; cite the current diff if uncommitted, never invent a commit SHA. A Skip leaves the defect unresolved and grants no new fix permission. If evidence disproves the finding, reclassify it below.

2. **FALSE POSITIVE comments:** These are reply decisions, not code approval. Show file:line (or [top-level]), summary, permalink and evidence, then ask:
   - A) Reply explaining why this is incorrect (recommended if clearly wrong)
   - B) Propose a code change
   - C) Ignore — don't reply, don't fix

   For A, use the **False Positive reply template** with evidence + suggested re-rank; save to both histories. For B, return to Steps 5c–5d with an ASK proposal. Show the exact change and any `test_stub`; wait for approval before editing. Retain the comment decision so re-entry does not repeat its question.

3. **VALID BUT ALREADY FIXED comments:** Reply with greptile-triage.md's **Already Fixed reply template**, no AskUserQuestion: what was done and the fixing commit SHA; save to both per-project and global greptile-history.

4. **SUPPRESSED comments:** Skip silently (known false positives from previous triage).

---

## Step 5.8: Persist Eng Review result

### 1. Re-review after edits

1. A pass covers Steps 3–5, including all reviewers before fixes. Allow at most 3 fix cycles:
   - Edited: increment CYCLES once. Below 3, repeat Steps 3–5 with a new
     REVIEW_START. At 3, persist `converged:false` and remaining findings by filling
     and saving the record below. Report nonconvergence and coverage gaps, then STOP
     this invocation, without a clean summary or a fourth pass.
   - No edits: fill the record below.
2. On a repeat, execute Steps 3–5 in order. At Step 4.7, reuse only this invocation's
   unchanged-input QA evidence; rerun affected probes after source, test, contract,
   command or fixture changes. Reusing a probe never skips a review step.
   A probe is affected when its entrypoint, dependencies, contract or replay inputs
   change; if uncertain, rerun it.
3. **Verify completed actions.** On the final zero-edit pass, reconcile this
   invocation's actions with current findings. Deduplicate by structural identity
   and advisory/defect kind. For a completed extraction, retain `fixed` and the
   original `evidence_paths`/`helper_target`; use `sharedLibsFingerprint` on that
   metadata. Verify the replacement helper, remaining callers and tests without
   requiring deleted pre-extraction blocks. Current findings determine recurring
   defects and unresolved counts; earlier fixes do not suppress them.
4. **Recheck skipped advice.** Re-read its final-snapshot supporting source and
   reconfirm the decision; otherwise report its history without a reusable skip.
   The logger computes `snapshot_covered_paths` from eligible paths whose raw bytes
   equal the bound snapshot blobs (`[]` if none). Never carry prior-cycle, supplied
   or prior-record coverage forward or build this proof yourself. Fixed advice
   needs no skip coverage.

### 2. Fill the record

- `COMPLETED`: true only when the checklist, dispatched specialists and native
  Step 4.8 adversarial pass finish, and every required Step 4.7 probe passes.
  Any failed, blocked, inconclusive or not-run required probe means false, as does
  a failed native review. `/ship` named-risk acceptance cannot complete `/review`.
- `CONVERGED`: true only for a completed zero-edit pass; `CYCLES` counts editing
  passes, not findings or reviewer attempts.
- `STATUS`: `clean` only when completed with zero unresolved non-advisory
  defects; otherwise `issues_found`. An incomplete review with no defects has
  zero counts and `completed:false`; explain the gap. Advice never blocks clean
  status or relaxes completion, convergence, start-token or missing-reviewer rules.

The required in-host adversarial result controls native completion. Optional outside
attempts keep their own incomplete records when unavailable and cannot substitute
for the native result, or vice versa. Step 4.8's structured-review gate still applies.
An outside review reporting `unverified` or `unavailable` is missing coverage, never a
pass: report it with its reason.

- Use Step 4.6's `specialists` object unchanged, including its empty small-diff map.
  If this host omits Review Army, use `specialists: {}` without claiming specialist coverage.
- Build `findings` from Step 5's combined final-pass findings (core, specialist,
  adversarial, actionable Greptile, verified exploratory QA findings) and invocation actions. Retain `fingerprint`, `severity`
  (`CRITICAL|INFORMATIONAL`), `action`, and any `advisory`, `evidence_paths`,
  `helper_target`. Recheck source after fixes. The logger uses `sharedLibsFingerprint`,
  never supplied/model hashes.
  Actions: `auto-fixed` (Step 5b), `fixed` (approved **and completed** in Step 5d),
  `skipped` (explicit Skip in Step 5c). Advice is never `auto-fixed`; pending
  advice stays in the response, not the record. Exclude prior Step 5.0
  suppressions; include this invocation's revalidated decisions.
- Gate findings additionally carry `gate` (the RH tag, e.g. `"RH-15"`), `gate_id`
  (the tool's hunk identity), `reason` (for `kept`) and one of the actions
  `kept`, `restored` or `open`. Never store hunk text, test names or commit
  messages in the record (it is brain-synced). `open` findings count in
  `issues_found` and in `critical`/`informational` by severity. The logger stamps
  `actor` from `gstack-session-kind` itself and discards any `actor` you supply;
  for a non-interactive session it also forces `action: "open"` on every gate
  finding. Dispositions are per invocation: `/ship`'s own review pass asks again.

```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":"COMMIT","completed":COMPLETED,"converged":CONVERGED,"cycles":CYCLES}' --finish REVIEW_START
```

`TIMESTAMP` is ISO 8601; `COMMIT` is `git rev-parse --short HEAD`.
`quality_score` is Step 4.6's specialist score (`10.0` when small-diff specialists
were skipped or this host omits Review Army). This default is not completion evidence;
unresolved non-advisory core defects still count in `issues_found`,
`critical`, `informational`. The logger builds trusted `review_binding` from the
validated captured branch digest, discarding caller bindings. Never invent a binding
or replace REVIEW_START at log time; finish only the final core token.

### Report the final review

Emit one final report, merging all reviewers rather than concatenating them:
1. `Pre-Landing Review: N issues (X critical, Y informational)` counts final unresolved
   non-advisory defects. State INCOMPLETE if `COMPLETED` is false, even when N=0.
2. Use the checklist's action groups with confidence-tagged finding lines. Keep fixed,
   skipped and advisory items separate from unresolved defects; retain their dispositions.
3. Append Step 4.7's single `## Exploratory QA and Verification Results` section with
   current evidence and coverage gaps. Neither coverage gaps nor advice are defects.
4. Add one `Gate edits:` line from Step 3.5's `GATE_SUMMARY:` and Step 5c's answers,
   using exactly one of these shapes:
   - `Gate edits: none detected (N listed, M read; patterns: <coverage>; unlisted idioms: <languages_unlisted>)`
   - `Gate edits: N listed, M read, K findings — J kept (reasons below), R restored, O open`
   - `Gate edits: partial (K of M eligible read)` followed by the findings counts for the hunks that were read
   - `Gate edits: UNAVAILABLE — <GATE_ERROR line or helper exit>` (missing coverage, never `none detected`)
   List each kept finding's `gate`, path and `reason` under it.

{{LEARNINGS_LOG}}

If the review exits early (for example, no diff against the base branch), do **not** write this entry.

## Important Rules

- **Read the FULL diff before commenting.** Do not flag issues already addressed in the diff.
- **Fix-first, not read-only.** AUTO-FIX items are applied directly, ASK items only after user approval. Never commit, push, or create PRs — that's /ship's job.
- **Be terse.** One line problem, one line fix. No preamble.
- **Only flag real problems.** Skip anything that's fine.
- **Optional extractions stay advisory.** Shared-code opportunities need verified callers and useful reliability or total savings; similarity alone is not a defect. Keep actual defects independently actionable.
- **Use Greptile reply templates from greptile-triage.md.** Every reply includes evidence. Never post vague replies.