gstack/review/SKILL.md.tmpl

---
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

You are running the `/review` workflow. Analyze the current branch's diff against the base branch for structural issues that tests don't catch.

---

{{SECTION_INDEX:review}}

---

## Step 1: Check branch

1. Run `git branch --show-current` to get the current branch.
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 && DIFF_BASE=$(git merge-base origin/<base> HEAD) && git diff "$DIFF_BASE" --stat` to check if there's a diff. If no diff, output the same message and stop.

---

{{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.** Do not proceed without the checklist.

---

## 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.

**If no PR exists, `gh` fails, API returns an error, or there are zero Greptile comments:** Skip this step silently. Greptile integration is additive — the review works without it.

**If Greptile comments are found:** Store the classifications (VALID & ACTIONABLE, VALID BUT ALREADY FIXED, FALSE POSITIVE, SUPPRESSED) — you will need them in Step 5.

---

## Step 3: Get the diff

Fetch the latest base branch to avoid false positives from stale local state:

```bash
git fetch origin <base> --quiet
```

Compute the merge base, then diff the working tree against that point:

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

This includes both committed and uncommitted changes while excluding commits that landed on the base branch after this branch was created.
Remember the printed start token as REVIEW_START for this pass. Capture it before reading the diff, never at log time. On each full re-review, capture a new token. Read any non-ignored untracked source files too (`git ls-files --others --exclude-standard`); the fingerprint includes them.

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

Check whether this PR's claimed VERSION still points at a free slot in the queue. Advisory only — never blocks review; just informs the reviewer about landing-order risk.

```bash
BRANCH_VERSION=$(git show HEAD:VERSION 2>/dev/null | tr -d '\r\n[:space:]' || echo "")
BASE_BRANCH=$(gh pr view --json baseRefName -q .baseRefName 2>/dev/null || echo main)
BASE_VERSION=$(git show origin/$BASE_BRANCH:VERSION 2>/dev/null | tr -d '\r\n[:space:]' || echo "")
QUEUE_JSON=$(bun 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')
```

- If `OFFLINE=true`: skip this section (no signal to report).
- 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>`.

---

## Step 3.5: Slop scan (advisory)

Run a slop scan on changed files to catch AI code quality issues (empty catches,
redundant `return await`, overcomplicated abstractions):

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

If findings are reported, include them in the review output as an informational
diagnostic. Slop findings are advisory, never blocking. If slop:diff is not
available (e.g., slop-scan not installed), skip this step silently.

---

{{LEARNINGS_SEARCH}}

{{ASIDE_RESEARCH}}

## Step 4: Critical pass (core review)

Apply the CRITICAL categories from the checklist against the diff:
SQL & Data Safety, Race Conditions & Concurrency, LLM Output Trust Boundary, Shell Injection, Enum & Value Completeness.

Also apply the remaining INFORMATIONAL categories that are still in the checklist (Async/Sync Mixing, Column/Field Name Safety, LLM Prompt Issues, Type Coercion, View/Frontend, Time Window Safety, Completeness Gaps, Distribution & CI/CD).

**Enum & Value Completeness requires reading code OUTSIDE the diff.** When the diff introduces a new enum value, status, tier, or type constant, use Grep to find all files that reference sibling values, then Read those files to check if the new value is handled. Shared-code analysis also requires reading related callers outside the diff; keep findings anchored to changed code.

**Search-before-recommending:** When recommending a fix pattern (especially for concurrency, caching, auth, or framework-specific behavior), research through Aside (Web research runs in Aside, above):
- Verify the pattern is current best practice for the framework version in use
- Check if a built-in solution exists in newer versions before recommending a workaround
- Verify API signatures against current docs (APIs change between versions)

```bash
{{ASIDE_EXEC_PRELUDE}}
_aside_exec "Search the web for {framework} {version} {pattern} current best practice and whether a built-in replaces it. Read-only: do not sign in, submit, or change anything. Reply with up to 5 bullets, each with its source URL, then stop."
```

Takes seconds, prevents recommending outdated patterns. If the Aside check did not print `READY`, use the WebSearch tool when the host provides it; with neither, note it and proceed with in-distribution knowledge.

Follow the output format specified in the checklist. Respect the suppressions — do NOT flag items listed in the "DO NOT flag" section.

### Shared-code opportunities (core pass)

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

{{SHARED_LIBS_RUBRIC}}

The core pass owns optional extraction advice. Present only worthwhile, supported proposals; zero is valid. For each proposal, show the changed anchor and other verified callers, smallest helper/destination, preserved differences, compatibility tests, shared-failure risk, and estimated implementation and total removed/added/saved lines from named blocks. Use `"category":"shared-libs","severity":"INFORMATIONAL","advisory":true`, retain `evidence_paths` (all authored supporting paths) and `helper_target:{"path":"...","symbol":"..."}`. When reusing an existing helper, include its authored path in `evidence_paths` so its contract and raw bytes participate in revalidation; a not-yet-created helper belongs only in `helper_target`. Deduplicate equivalent proposals and overlapping savings. Existing-helper reuse is preferable when compatible.

**Identity before merge or suppression:** Compute the structural fingerprint through the installed `sharedLibsFingerprint` helper, never write model-generated hash text. Feed the finding as literal JSON on stdin (replace the example values; keep the quoted delimiter), not interpolated shell code:

```bash
GSTACK_SHARED_LIB=~/.claude/skills/gstack/lib/review-evidence.ts
bun -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 has no reusable identity and must be revalidated. A real defect in the same code remains a normal defect with its own evidence and Fix-First handling. An optional extraction must never suppress, downgrade, or replace that defect, even if they share a supplied fingerprint or an extraction was previously skipped.

{{CONFIDENCE_CALIBRATION}}

---

{{SECTION:review-army}}

---

## Step 5: Fix-First Review

**Every finding gets action — not just critical ones.**

**Keep decisions through fix cycles.** Maintain an in-memory action list for this invocation, initialized once and retained when Steps 3–5.7 repeat. Keep defects and advisories separate; for shared-code advice retain the helper-computed fingerprint, `advisory`, `evidence_paths`, and `helper_target` from the actual decision. Record completed AUTO-FIX/fix actions and explicit Skip choices as they happen. A later zero-edit pass may no longer find an approved extraction because it succeeded; that must not erase its `fixed` action or original identity metadata.

On each repeat pass, re-read all supporting callers and the helper destination before carrying an advisory decision forward. An unrelated auto-fix does not require asking the same question again when the structural identity, proposed contract, and tradeoffs remain unchanged. Compare actual raw source with the evidence read for the decision, including secondary callers and any transformed or indirect paths; changed evidence requires fresh evaluation. If the proposal, behavior, migration, or risk has materially changed, ask a new question instead of inheriting the choice. This invocation-local decision tracking is not cross-review suppression and must never hide a new or recurring defect.

{{CROSS_REVIEW_DEDUP}}

### 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 the severity validation above, every remaining finding with `advisory:true`, including core shared-code advice, is ASK-only even when mechanical. Never auto-apply an optional extraction. Label it `[ADVISORY]`, show the helper, caller migration, tests, and estimated total savings, and let the user approve or skip it. Advisories are excluded from defect counts, score penalties, unresolved-defect totals, and clean-status blockers. A real defect still follows ordinary Fix-First independently of advice touching the same code.

**Test stub override:** Any finding that has a `test_stub` field (generated by a specialist)
is reclassified as ASK regardless of its original classification. When presenting the ASK
item, show the proposed test file path and the test code. The user approves or skips the
test creation. If approved, write the fix + test file. 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. Output: `[FIXED + TEST] [file:line] Problem -> fix + test at [test_path]`

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

Apply each fix directly. For each one, output a one-line summary:
`[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

If there are ASK items remaining, present them in ONE AskUserQuestion:

- List each item with a number, the severity label (or `[ADVISORY]` for optional advice), the problem, and a recommended fix
- For each item, provide options: A) Fix as recommended, B) Skip
- Include an overall RECOMMENDATION

Example format:
```
I auto-fixed 5 issues. 2 need your input:

1. [CRITICAL] app/models/post.rb:42 — Race condition in status transition
   Fix: Add `WHERE status = 'draft'` to the UPDATE
   → A) Fix  B) Skip

2. [INFORMATIONAL] app/services/generator.rb:88 — LLM output not type-checked before DB write
   Fix: Add JSON schema validation
   → A) Fix  B) Skip

RECOMMENDATION: Fix both — #1 is a real race condition, #2 prevents silent data corruption.
```

If 3 or fewer ASK items, you may use individual AskUserQuestion calls instead of batching.
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.

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

Apply fixes for items where the user chose "Fix." Output what was fixed.
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.

If no ASK items exist (everything was AUTO-FIX), skip the question entirely.

### Verification of claims

Before producing the final review output:
- If you claim "this pattern is safe" → cite the specific line proving safety
- If you claim "this is handled elsewhere" → read and cite the handling code
- If you claim "tests cover this" → name the test file and method
- Never say "likely handled" or "probably tested" — verify or flag as unknown

**Rationalization prevention:** "This looks fine" is not a finding. Either cite evidence it IS fine, or flag it as unverified.

### 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 the **Escalation Detection** algorithm from greptile-triage.md to determine whether to use Tier 1 (friendly) or Tier 2 (firm) reply templates.

1. **VALID & ACTIONABLE comments:** These are included in your findings — they follow the Fix-First flow (auto-fixed if mechanical, batched into ASK if not) (A: Fix it now, B: Acknowledge, C: False positive). If the user chooses A (fix), reply using the **Fix reply template** from greptile-triage.md (include inline diff + explanation). If the user chooses C (false positive), reply using the **False Positive reply template** (include evidence + suggested re-rank), save to both per-project and global greptile-history.

2. **FALSE POSITIVE comments:** Present each one via AskUserQuestion:
   - Show the Greptile comment: file:line (or [top-level]) + body summary + permalink URL
   - Explain concisely why it's a false positive
   - Options:
     - A) Reply to Greptile explaining why this is incorrect (recommended if clearly wrong)
     - B) Fix it anyway (if low-effort and harmless)
     - C) Ignore — don't reply, don't fix

   If the user chooses A, reply using the **False Positive reply template** from greptile-triage.md (include evidence + suggested re-rank), save to both per-project and global greptile-history.

3. **VALID BUT ALREADY FIXED comments:** Reply using the **Already Fixed reply template** from greptile-triage.md — no AskUserQuestion needed:
   - Include what was done and the fixing commit SHA
   - Save to both per-project and global greptile-history

4. **SUPPRESSED comments:** Skip silently — these are known false positives from previous triage.

---

## Step 5.5: TODOS cross-reference

Read `TODOS.md` in the repository root (if it exists). Cross-reference the PR against open TODOs:

- **Does this PR close any open TODOs?** If yes, note which items in your output: "This PR addresses TODO: <title>"
- **Does this PR create work that should become a TODO?** If yes, flag it as an informational finding.
- **Are there related TODOs that provide context for this review?** If yes, reference them when discussing related findings.

If TODOS.md doesn't exist, skip this step silently.

---

## Step 5.6: Documentation staleness check

Cross-reference the diff against documentation files. For each `.md` file in the repo root (README.md, ARCHITECTURE.md, CONTRIBUTING.md, CLAUDE.md, etc.):

1. Check if code changes in the diff affect features, components, or workflows described in that doc file.
2. If the doc file was NOT updated in this branch but the code it describes WAS changed, flag it as an INFORMATIONAL finding:
   "Documentation may be stale: [file] describes [feature/component] but code changed in this branch. Consider running `/document-release`."

This is informational only — never critical. The fix action is `/document-release`.

If no documentation files exist, skip this step silently.

---

{{SECTION:adversarial}}

## Step 5.8: Persist Eng Review result

After all review passes complete, persist the final `/review` outcome so `/ship` can
recognize that Eng Review was run on this branch.

Follow the completion/retry and detailed record-field rules in the adversarial section before persisting.

Run:

```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
```

Substitute:
- `TIMESTAMP` = ISO 8601 datetime
- `STATUS` = `"clean"` if there are no remaining unresolved non-advisory defects after Fix-First handling and adversarial review, otherwise `"issues_found"`. Unapproved or skipped advisories never block clean status; incomplete or nonconverged coverage remains governed by the completion rules.
- `issues_found` = total remaining unresolved non-advisory defects
- `critical` = remaining unresolved non-advisory critical defects
- `informational` = remaining unresolved non-advisory informational defects
- `quality_score` = the PR Quality Score computed in Step 4.6 (e.g., 7.5). If specialists were skipped (small diff), use `10.0`
- `COMMIT` = output of `git rev-parse --short HEAD`

{{LEARNINGS_LOG}}

If the review exits early before a real review completes (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 are only applied 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.