squad-review

What this skill does

# Squad Review

Fans out six independent reviewers in parallel against the pending
branch changes, then assembles one markdown report with one section
per reviewer.

Inspired by the "just do what ultrareview does" pattern: dispatch →
collect → categorize. Cheaper, local, and fully under your control.

## Pre-gathered Context

<!--
  `!`-prefix executes before dispatch; output is injected into each
  reviewer's prompt as shared context. Keep small — reviewers pull
  file contents themselves.
-->

- Branch: !`git rev-parse --abbrev-ref HEAD`
- Branch diff stat vs main: !`git diff main...HEAD --stat 2>/dev/null || echo "no diff vs main"`
- Commits on branch vs main: !`git log main..HEAD --oneline 2>/dev/null`
- Uncommitted tracked changes stat: !`git diff HEAD --stat 2>/dev/null`
- Untracked files: !`git ls-files --others --exclude-standard | head -50`
- CLAUDE.md files in repo: !`find . -maxdepth 4 -type f -name 'CLAUDE.md' -not -path '*/node_modules/*' -not -path '*/.git/*' 2>/dev/null | head -10 || true`
- Open PR metadata: !`gh pr view --json title,body,comments 2>/dev/null || echo "no open PR"`

## Scope Selection

From the context above, three possible scopes may exist:

- **branch-diff** — "Branch diff stat vs main" lists files
- **uncommitted-tracked** — "Uncommitted tracked changes stat" lists files
- **untracked** — "Untracked files" has entries

### Decision rules

1. **All empty** → do NOT dispatch. Respond: _No changes to review._ Stop.
2. **Exactly one scope available, and it is `branch-diff`** → proceed
   automatically. State the scope in one sentence, then dispatch.
3. **Any other case (on `main` with uncommitted work, or a feature
   branch that also has pending scratch, or untracked-only, etc.)**
   → ASK THE USER TO PICK. Do not dispatch until they choose.

### Menu format (case 3)

Present a numbered list showing only the options that apply, with file
counts and (for small sets) file names. Include `Cancel` as the last
option. Example templates:

On `main` with tracked + untracked:

```
Multiple review scopes available. Pick one:

1. Uncommitted tracked changes (<N> files)
2. Untracked files only (<N> files): <comma-separated names>
3. Both tracked and untracked (<N> files total)
4. Cancel
```

On a feature branch with extra uncommitted work:

```
Multiple review scopes available. Pick one:

1. Branch diff vs main (<N> files, <N> commits) — normal case
2. Uncommitted changes on top of HEAD (<N> files)
3. Cancel
```

Wait for the user's choice. Once selected, state the chosen scope in
one sentence and dispatch.

### Binary/generated exclusions

Regardless of scope, all reviewers ignore:

- images (`*.png`, `*.jpg`, `*.jpeg`, `*.gif`, `*.webp`, `*.svg` as image content)
- compiled artifacts (`dist/`, `build/`, `node_modules/`)
- lockfiles (`pnpm-lock.yaml`, `package-lock.json`, `yarn.lock`)
- auto-generated manifests if they are pure sync output (e.g. if a
  file is regenerated by a build script and shows up in the diff with
  no hand-edits, skip it — reviewers should use their judgment)

## Dispatch Protocol

Issue **six `Task` calls in a single message** so they run in parallel.
For each, use:

- `subagent_type`: `general-purpose` (full tool access — reviewers must
  be able to read arbitrary files and grep the repo)
- `description`: the reviewer name (e.g., "Security review")
- `prompt`: the inlined reviewer prompt below, with the shared context
  block prepended

Shared context block to prepend to every reviewer prompt (fill in
according to the scope selected above):

```
## Scope
Branch: <branch name>
Scope type: branch-diff | uncommitted
Read the diff with: <one of:>
  - git diff main...HEAD         (branch-diff scope)
  - git diff HEAD                (uncommitted, tracked changes)
New/untracked files to read in full:
  - <path> (omit section entirely if scope is branch-diff)
  - <path>
Commits (branch-diff scope only):
  - <one-liners>
Ignore: binary files, images, dist/, node_modules/, lockfiles.

## Repo Conventions
CLAUDE.md files present: <list from context above>
You MUST read every CLAUDE.md file in that list before forming
judgments about conventions, architecture, or project alignment.

## PR Context
<PR title + body if present, else "no open PR">
```

Tell each reviewer to read the diff itself via git — do not paste the
diff into the prompt.

## Reviewer 1 — Security

```
You are a critical-severity-only security reviewer. Find show-stopper
vulnerabilities in the diff on this branch — bugs that would make an
engineer drop everything and fix on a Friday.

Method: invariant-binding analysis across trust boundaries.

For each sensitive operation touched by the diff (data access, state
change, money movement, auth decisions, secret handling, ID lookups,
file I/O, outbound URLs, deserialization), list the invariants it
relies on as bindings:
  credential ↔ tenant/scope ↔ actor/session ↔ target/resource ↔
  action/intent ↔ time/state

Trace where each binding comes from — DB record, signed token,
server-generated session, config. Then hunt for cases where a binding
is missing, bypassable, weakened, or inconsistently sourced across
parallel code paths (legacy routes, feature flags, admin shortcuts,
internal endpoints exposed externally).

Confirm end-to-end exploitability before reporting. If you cannot
construct the smallest counterexample — attacker starting state,
specific requests, attacker-obtainable data — it is not a finding.

Hard rules:
- Scope to the branch diff. Read surrounding code only to confirm
  exploitability.
- Zero low-severity. Open redirects, CSS injection, missing headers,
  verbose errors — out. Bar is: reflected XSS, substantial IDOR where
  the attacker can obtain the identifier (not guess a UUID), auth
  bypass, injection with a user-reachable sink, SSRF to internals,
  client-only trust-boundary violations, race conditions with real
  impact.
- Do not propose fixes. Do not write files. Findings inline only.
- Do not read memories, prior findings, or git commits beyond HEAD —
  reason fresh from the code.

Detection patterns (priming, not a checklist):
- auth check in middleware but handler callable directly
- role/permission checked at UI but not API
- user-supplied ID without ownership verification vs session
- bulk op that doesn't validate each item's ownership
- JWT/session claims trusted without signature verification
- session/token accepted across tenant boundaries
- signed data mixed with unsigned in the same flow
- client-side validation only (price, quantity, permissions)
- SSRF via user-controlled URLs, path traversal in file ops
- deserialization of untrusted data
- check-then-act without atomicity (balance, inventory, status)
- parallel requests bypassing rate limits
- debug/test endpoints reachable in production
- new env vars, secrets, or tokens introduced without corresponding
  scope/rotation/expiry binding

Output (markdown, no preamble):

## Security

*N findings, M critical*

### 1. <short title>
**Severity:** Critical | High
**Location:** `path/to/file.ts:LINE`
**Invariant violated:** <which binding breaks>
**Exploit flow:**
1. attacker step
2. attacker step
3. resulting state
**Impact:** <new capability the attacker gains that they did not
have before, tied to the threat model>

If no critical/high findings, write exactly:
*No critical or high findings.*

Do not pad with advisories. Do not suggest hardening.
```

## Reviewer 2 — Correctness

```
You are a correctness reviewer. Find real bugs the diff introduces or
exposes — logic that will misbehave at runtime for plausible inputs
or states. Silent failure is the worst kind of bug; hunt for it.

For each changed function/handler:
1. Enumerate the inputs (types, ranges, null/undefined, empty, edge).
2. Enumerate the outputs and side effects.
3. Trace each input class through to output. Where does it diverge
   from what a caller would expect?

Specifically look for:
- swapped or misordered arguments
- off-by-one, fencepost, i