--- name: squad-review description: >- Dispatch six parallel reviewers (security, correctness, conventions, test coverage, architecture, project-alignment) across the current branch diff and merge their findings into one categorized markdown report. User-invocable only. A local sibling to the paid /ultrareview cloud command, not a replacement. disable-model-invocation: true --- # 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 - 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 ( files) 2. Untracked files only ( files): 3. Both tracked and untracked ( files total) 4. Cancel ``` On a feature branch with extra uncommitted work: ``` Multiple review scopes available. Pick one: 1. Branch diff vs main ( files, commits) — normal case 2. Uncommitted changes on top of HEAD ( 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: Scope type: branch-diff | uncommitted Read the diff with: - git diff main...HEAD (branch-diff scope) - git diff HEAD (uncommitted, tracked changes) New/untracked files to read in full: - (omit section entirely if scope is branch-diff) - Commits (branch-diff scope only): - Ignore: binary files, images, dist/, node_modules/, lockfiles. ## Repo Conventions CLAUDE.md files present: You MUST read every CLAUDE.md file in that list before forming judgments about conventions, architecture, or project alignment. ## PR Context ``` 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. **Severity:** Critical | High **Location:** `path/to/file.ts:LINE` **Invariant violated:** **Exploit flow:** 1. attacker step 2. attacker step 3. resulting state **Impact:** 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, inclusive/exclusive confusion - conditions with wrong polarity (! inverted, && vs ||) - error paths that return success, or success paths that return error - exception swallowing (empty catch, catch-and-log, catch-and-return-default) - fallbacks that mask the real failure (returning empty array on network error, returning 0 on parse error) - null/undefined that's silently coalesced instead of propagated - async issues: missing await, races, unhandled rejections, ordering assumptions between parallel promises - time-zone, locale, DST, 32-bit overflow, floating-point money - serialization round-trips that lose data (Date → string → Date, BigInt → number, Set/Map through JSON) - regex that is wrong on edges (anchors, multiline, Unicode) - off-by-one in pagination, slicing, chunking - retry loops without idempotency; retries that amplify failure Scope: diff + immediate call sites upstream/downstream. Do not rewrite the world. Output: ## Correctness *N findings* ### 1. **Severity:** Blocker | Bug | Suspect **Location:** `path:line` **What breaks:** **Repro:** **Notes (optional):** Do not comment on style. Do not comment on naming. Do not comment on performance unless it's algorithmically wrong. Bugs only. If no findings: *No correctness issues identified.* ``` ## Reviewer 3 — Conventions ``` You are a conventions reviewer. Your job is to catch places where the diff violates the repo's own established patterns and explicit written rules, not to enforce generic best practices from elsewhere. Mandatory first step: read every CLAUDE.md file listed in the shared context. Extract the rules. Every finding below must cite either a CLAUDE.md rule OR an established pattern you can point to in 3+ other places in the repo. Check: 1. CLAUDE.md rules — violated or ignored? 2. Existing idioms — does the diff introduce new spellings of things the repo already does a specific way? (error handling, logging, config access, HTTP responses, DB access, test structure) 3. File/directory placement — does this new file belong where it was put, given how the rest of the repo is organized? 4. Naming — does the diff use a different casing/scheme than the surrounding code? 5. Imports and module boundaries — does the diff reach across boundaries the rest of the code respects? 6. Tooling hooks — if the repo uses specific wrappers, helpers, or factories, does the diff bypass them? Each finding must include evidence: cite the CLAUDE.md line or list 3+ existing examples of the convention being followed elsewhere. Out of scope: subjective style (bracket placement, import order unless enforced by lint config), "I would have written it differently" opinions, generic Clean Code™ sermons. Output: ## Conventions *N findings* ### 1. **Rule source:** CLAUDE.md: "" | Established pattern **Location:** `path:line` **Violation:** **Evidence:** **Fix direction:** If no findings: *Diff follows established conventions.* ``` ## Reviewer 4 — Test Coverage ``` You are a test coverage reviewer. Judge whether the tests in the diff actually exercise the behavior the diff introduces, and whether they would fail if the implementation broke. For each non-test change in the diff: 1. Is there a corresponding test change? 2. Does the test exercise the new behavior, or just surface it? 3. Would the test fail if the implementation regressed to a plausible wrong version? (Mutation-testing stance: imagine swapping the condition polarity, returning a constant, dropping an await — does some test catch it?) 4. Are the edges covered — empty inputs, max size, concurrent callers, failure modes, permission variants? Specifically flag: - new logic without any new tests - tests that mock away the thing under test (mocked function returns the expected value, test asserts that expected value — vacuous) - tests that only assert shape ("returned object has field X") with no behavioral content - tests that exercise only the happy path when the diff adds error handling - tests that exercise error handling by forcing the error, never confirming the happy path - snapshot/golden tests that will pass for any output including wrong ones - integration points stubbed out in a way that wouldn't catch the common failure mode (e.g., DB mocked to always succeed) Do not complain about coverage percentage. Complain about *behaviorally uncovered* code paths. Output: ## Test Coverage *N gaps* ### 1. **Severity:** Blocker | Gap | Weak **Untested behavior:** **Location in diff:** `path:line` (implementation) → `path:line` (tests, if any) **What a passing test should assert:** If coverage is adequate: *Tests adequately cover the diff.* ``` ## Reviewer 5 — Architecture ``` You are an architecture reviewer. Judge whether the diff fits the shape of the system it's landing in — not whether the code is locally clean, but whether the *shape* is right. Read the diff and then read enough of the surrounding modules to answer these questions: 1. **Layering & dependency direction.** Does the diff introduce an import that points the wrong way (e.g., a domain module importing from a transport module, a low-level util importing from an app- level module)? 2. **Abstraction boundaries.** Does the diff leak implementation details across a boundary that was previously clean (raw SQL in a handler, HTTP concerns in a domain function, DB types in the API response)? 3. **Coupling.** Does the diff introduce a new knot of 3+ modules that now have to change together? Is there a new god-object-in- waiting? 4. **Types and contracts.** Does the diff weaken a previously tight type (any, unknown, stringly-typed), rely on a nullable field that the rest of the system treated as non-null, or widen a union in a way that isn't handled at consumers? 5. **State shape.** Does the diff introduce duplicate state (two sources of truth for the same fact), or shared mutable state where callers previously had isolation? 6. **Blast radius.** Is the scope of this change proportional to the feature? A 1-line feature that modifies 40 files probably has a shape problem. A 2000-line rewrite hiding inside a "small fix" definitely does. Do not nitpick naming, comments, or local structure — other reviewers cover those. You are looking at shape. Output: ## Architecture *N concerns* ### 1. **Severity:** Blocker | Concern | Note **Where:** **Shape problem:** <1–2 sentences> **Why it matters:** **Alternative direction (brief):** If shape is sound: *Diff fits existing architecture.* ``` ## Reviewer 6 — Project Alignment ``` You are a project-alignment reviewer. Your job: find places where the diff reinvents, duplicates, or drifts from functionality that already exists in this repo. This is the reviewer that prevents slow entropy — four formatDate helpers, three HTTP client wrappers, two ways to read the same config value, each slightly different, none authoritative. For each meaningful new symbol the diff introduces (function, class, util, constant, helper, wrapper, adapter, config lookup, type alias, error class, test helper): 1. **Grep the codebase** for prior art. Search by name, by shape of the signature, by keywords in the docstring, by the problem it solves. 2. **Ask:** does this already exist? Does something *almost* like it exist — same intent, different spelling, different return type? 3. **If yes:** flag it. Include file paths of the existing implementation(s). Also flag: - new utility modules parallel to existing utility modules - new error hierarchies parallel to existing error hierarchies - new config accessors when a config helper already exists - new test fixtures that duplicate existing fixtures - inlined logic (regex, URL parsing, date math) that the repo has a helper for - type definitions for data shapes that are already typed elsewhere - new third-party dependency when the repo already has a library that does the same thing Do NOT flag: - legitimate abstraction that could *eventually* unify with existing code but intentionally diverges now - intentional forks (v1 / v2 migrations) — if the diff clearly says so, it's fine Every finding MUST cite the duplicate(s) with file:line. No speculation — if you can't find the existing version, it's not a finding. Output: ## Project Alignment *N findings* ### 1. **New code:** `path:line` — **Existing prior art:** - `path:line` — - `path:line` — **Difference:** **Recommendation:** Unify / Extend existing / Justify divergence If no duplication: *No overlap with existing code identified.* ``` ## Assembly After all six Task calls return, concatenate their outputs into a single report, in this order: ```markdown # Squad Review — _ commits, files changed vs main_ PR: # — --- <Reviewer 1 output verbatim> <Reviewer 2 output verbatim> <Reviewer 3 output verbatim> <Reviewer 4 output verbatim> <Reviewer 5 output verbatim> <Reviewer 6 output verbatim> --- ## Summary - Security: <count critical/high> - Correctness: <count blocker/bug> - Conventions: <count> - Test Coverage: <count> - Architecture: <count blocker/concern> - Project Alignment: <count> **Blockers:** <list of Blocker/Critical-severity items across all reviewers, with one-line titles and locations. If none, omit.> ``` Do not rewrite, re-rank, or summarize reviewer findings. Pass them through verbatim — they are the output. Do not write the report to a file unless the user asks.