Ia Code Review

Code Review

Two-Stage Review

Stage 1 -- Spec compliance (do this FIRST): verify the changes implement what was intended. Check against the PR description, issue, or task spec. Identify missing requirements, unnecessary additions, and interpretation gaps. If the implementation is wrong, stop here -- reviewing code quality on the wrong feature wastes effort.

Stage 2 -- Code quality: only after Stage 1 passes, review for correctness, maintainability, security, and performance.

Scope Resolution

Pre-flight: verify git rev-parse --git-dir exists before anything else. If not in a git repo, ask for explicit file paths.

When no specific files are given, resolve scope via this fallback chain: 1. User-specified files/directories (explicit request) 2. Session-modified files (git diff --name-only for unstaged + staged) 3. All uncommitted files (git diff --name-only HEAD) 4. Untracked files (git ls-files --others --exclude-standard) -- new files are often most review-worthy 5. Zero files → stop. Ask what to review.

Exclude: lockfiles, minified/bundled output, vendored/generated code.

Base-branch resolution for branch reviews

When the review target is a branch (not a working-tree diff), the comparison range is the merge-base, not the working-tree delta — resolve it before reading any diff. Full fallback chain (PR base → default-branch inference → origin/* fallback list → git merge-base → shallow-clone retry) and the "never fall back to git diff HEAD" rule in scope-resolution.md.

When a branch is stacked on another unmerged branch, git merge-base HEAD <default-branch> over-covers — it sweeps in the sibling branch's commits, fabricating findings on files this change doesn't touch. Prefer the hosting platform's authoritative base SHA (PR/MR base_sha, or gh pr diff) over a locally computed merge-base. After the run, intersect every finding's path with the change's --name-only set and discard off-scope ones.

Review Mode Selection

Run this BEFORE reading the full diff. Use metadata only (git diff --stat, file list from scope resolution) to count signals. Reading the diff first creates analysis momentum that bypasses mode selection.

Test file exclusion: exclude test paths (tests/, test/, __tests__/, *.test.*, *.spec.*, *_test.*) from the lines/files/directories signals — they inflate complexity without adding review risk. Filter with git diff --stat -- ':!tests/' ':!*.test.*' ':!*.spec.*' ':!*_test.*' and report both totals: "450 lines changed (280 excluding tests)."

3+ signals → deep review. Inform the user, then dispatch parallel specialist agents per deep-review.md. Pass the diff to agents -- do NOT read it first. Reading and analyzing the diff yourself before dispatching agents defeats the purpose of deep review. Stop here -- do not proceed to the Review Process section.

2 signals → suggest: "This touches N files across M modules. Deep review? (y/n)"

0-1 signals → standard review. Proceed to Review Process below.

Before auto-switching to deep review, check the exceptions list in deep-review.md -- certain change types (pure docs, mechanical refactors, single-file <50 lines) override signal count.

Override: deep forces multi-agent, quick forces single-pass.

Review Process

Standard reviews only. If mode selection triggered deep review, specialist agents handle the review per deep-review.md -- do not run these steps yourself.

1. Context — do these before reading code:

• Scope Drift Check: compare git diff --stat against the PR's stated intent. Classify as CLEAN / DRIFT DETECTED / REQUIREMENTS MISSING. If DRIFT, note the drifted files and ask the author: ship as-is, split, or remove unrelated changes?
• Read the intent: PR description, linked issue, or task spec. If the code does something the intent doesn't describe, or fails to do something the intent promises, flag as a finding — correct code that solves the wrong problem is still wrong.
• Fetch existing discussions (when present): before raising findings, reconcile prior review comments so you don't re-raise issues other reviewers already resolved. Gate the fetch on a presence check to avoid empty work — the exact gh pr view/gh api commands are in scope-resolution.md.
• Run automated gates: execute the project's test/lint suite if available (check CI config for the canonical commands) to catch failures before manual review.

2. Structural scan -- architecture, file organization, API surface changes. Flag breaking changes. For files marked as added (A) in the diff, use the diff content directly -- don't attempt to read them from the working tree when reviewing a remote branch. 3. Line-by-line -- correctness, edge cases, error handling, naming, readability. Use question-based feedback ("What happens if input is empty here?") instead of declarative statements to encourage author thinking. 4. Security -- input validation, auth checks, secrets exposure, injection vectors (SQL, XSS, CSRF, SSRF, command, path traversal, unsafe deserialization). Flag race conditions (TOCTOU, check-then-act). Use security-patterns.md for grep-able detection patterns across 11 vulnerability classes. 5. Test coverage -- verify new code paths have tests. Flag untested error paths, edge cases, and behavioral changes without corresponding test updates. Flag tests coupled to implementation details (mocking internals, testing private methods) -- test behavior, not wiring. 6. Reliability -- error handling completeness, timeout/retry logic, resource cleanup on error paths, graceful degradation. Use reliability-patterns.md for detection patterns and grep-able signals. 7. Removal candidates -- identify dead code, unused imports, feature-flagged code that can be cleaned up. Distinguish safe-to-delete (no references) from defer-with-plan (needs migration). 8. Verify -- run formatter/lint/tests on touched files. State what was skipped and why. If code changes affect features described in README/ARCHITECTURE/CONTRIBUTING, note doc staleness as informational. 9. Summary -- present findings grouped by severity with verdict: Ready to merge / Ready with fixes / Not ready.

Large diffs & PR sizing: For diffs >500 lines, review by module rather than file-by-file. Flag oversized PRs (ideal ~100-300 meaningful lines, excluding generated code) and suggest a split. Module-review approach, sizing thresholds, and the four split strategies (stack / by-file-group / horizontal / vertical) in pr-sizing.md.

Severity and Confidence

Four severity tiers (Critical / Important / Medium / Minor) and a 5-band confidence rubric (0.0-1.0 → Report / Report-if-actionable / Suppress) govern what lands in the report. Full rules, false-positive suppression categories, and the LLM-specific prompt-injection exception in severity-and-confidence.md.

Tie every finding to concrete code evidence (file path, line number, specific pattern), carried in the CR-XXX entry itself — not only in the surrounding prose. An entry that names a function or describes a bug without its [file:line] and ` quoted code ` can't be verified by the reader. Never fabricate references.

What to Check

For category checklists (Correctness, Maintainability & Readability, Performance, Adversarial red-team pass, AI-generated code lens), load check-categories.md. It's the structured checklist for the line-by-line review step.

Language-specific checks live in language-profiles.md — load the profile matching the file extensions in the diff (TypeScript/React, Python, PHP, Shell/CI, Configuration, Data Formats, Security, LLM Trust Boundaries).

Action Routing

For every finding, classify the fix into one of four tiers: safe_auto / gated_auto / manual / advisory. Full decision rules and conflict-resolution policy in action-routing.md. When in doubt, escalate to gated_auto — never promote toward safe_auto on disagreement.

Comment Labels

Prefix inline review comments so authors know what requires action:

• (no prefix) -- required change (maps to Critical or Important severity), blocks merge
• Nit: -- style preference, optional
• Consider: -- suggestion worth evaluating, not blocking
• FYI: -- informational, no action expected

Anti-Patterns in Reviews

• Nitpicking style when linters exist -- defer to automated tools instead
• "While you're at it..." scope creep -- open a separate issue instead
• Blocking on personal preference -- approve with a Minor comment instead
• Rubber-stamping without reading -- always verify at least Stage 1
• Reviewing code quality before verifying spec compliance -- do Stage 1 first
• Recommending fix patterns without checking currency -- verify the pattern is current for the project's framework version before suggesting it. Prefer built-in alternatives from newer versions
• Fighting documented overrides -- if CLAUDE.md, AGENTS.md, or an inline comment documents a deliberate bypass (e.g., "we allow X because Y"), honor it: don't re-raise the concern or work around it "just to be safe". If the override lacks a rationale, suggest documenting one — don't argue the rule.
• Resting a finding on an unverified absence -- "this symbol/handler/path doesn't exist" must be checked directly (read the region, or grep the exact symbol expecting zero lines) before any finding depends on it. A subagent's confident negative, or a hit from a broad/alternation pattern that matched other lines, is the least reliable output: proving absence needs whole-search-space coverage, which under-searching fakes. Positive findings ("here it is at file:line") are trustworthy; negatives must be re-derived.
• Calling a behavioral change a regression without a baseline read -- read the pre-change file at the base (git show <base>:<file>), not just the diff hunk. A re-spec may have intentionally redefined the contract (its description, not the OLD code, is the oracle); a dropped branch may have been a latent bug; a sibling may have always omitted the field. Conversely, pre-existing code outside the diff is this change's responsibility when the new feature makes a previously-invisible defect user-visible -- frame that as introduced here, not a follow-up. When a regression is confirmed against the baseline, cite the introducing commit (SHA, author -- via git blame or git bisect) as part of the finding's evidence, not just the symptom.
• Widening/narrowing a key or guard without checking the mirror bug -- when a fix widens, narrows, or loosens a match key, dedup key, or guard, re-check the failure along the axis the change now ignores: closing duplicate-on-no-match by widening a key opens false-merge-on-shared-key; loosening a guard to admit a good value admits bad ones too; tightening a matcher to drop a bad value drops legitimate ones. Name one concrete opposite-defect case before accepting the change.
• Checking only one projection on a hide/filter/redact change -- enumerate every field in the response that surfaces the same entity: the structured list AND the raw documents/files, the array AND its *_count/*_ids/total, the summary projection AND the detail projection. A filter applied to one projection leaks the entity via a sibling field on the same response. Require a test asserting the hidden entity is absent from each surfacing field, not just the primary list.
• Pre-classifying your own findings as weak in their wording -- severity tags ([Minor], [FYI]) are fine, but phrases like "INFO only", "no action required", "optional cleanup", "operational tradeoff" read to a downstream validator or second reviewer as a self-dismissal and get the finding dropped regardless of real severity. Anchor severity in concrete constants and numbers from the code ("20-minute floor", "every inactive bar"), not hypotheticals -- a named constant is harder to wave off than "potentially hours".

When to Stop and Ask

• Fixing the issues would require an API redesign beyond the PR's scope
• Intent behind a change is ambiguous -- ask rather than assume
• Missing validation tooling (no linter, no tests) -- flag the gap, don't guess

Output Format

## Review: [brief title]

### Critical
- **CR-001.** [file:line] `quoted code` -- [issue]. Score: [0.0-1.0]. [What happens if not fixed]. Fix: [concrete suggestion].

### Important
- **CR-002.** [file:line] `quoted code` -- [issue]. Score: [0.0-1.0]. [Why it matters]. Consider: [alternative approach].

### Medium
- **CR-003.** [file:line] -- [issue]. Score: [0.0-1.0]. [Why it matters].

### Minor
- **CR-004.** [file:line] -- [observation].

### What's Working Well
- [specific positive observation with why it's good]

### Residual Risks
- [unresolved assumptions, areas not fully covered, open questions]

### Verdict
Ready to merge / Ready with fixes / Not ready -- [one-sentence rationale]

Number findings CR-001, CR-002... sequentially across all severities so they're referenceable by ID. Limit to 10 per severity; if more exist, note the count and show the highest-impact ones.

Markdown safety: in table cells, escape literal | as \| — code excerpts with pipe operators (a | b, string | null) split rows silently otherwise. Bullet output is pipe-safe.

For multi-agent consolidation (deep/parallel review), apply the merge algorithm in deep-review.md — same-line dedupe, conflicting severity, NEEDS DECISION flagging, cross-lens confidence boosting.

Clean review (no findings): if the code is solid, say so explicitly — summarize what was checked and why no issues were found. A clean review is a valid outcome, not a sign of insufficient effort.

References

Integration

• ia-receiving-code-review -- the inbound side (processing review feedback received from others). Action-routing terminology maps across: safe_auto ≈ AUTO-FIX, gated_auto ≈ ESCALATE-for-approval, manual ≈ ESCALATE, advisory ≈ FYI (no-op).
• ia-kieran-reviewer agent -- persona-driven Python/TypeScript deep quality review (type safety, naming, modern patterns)
• /ia-review -- full ceremony review (worktrees, ultra-thinking, multi-agent). Deep review is lighter: no worktrees, no plan verification, just parallel specialist agents on the same diff.
• /resolve-pr-parallel command -- batch-resolve PR comments with parallel agents
• ia-security-sentinel agent -- deep security audit beyond the security step in this skill. Also supports threat-model mode for architectural security analysis when the diff introduces new trust boundaries, auth flows, or external API surfaces.

Action Routing — 4-Tier Fix Classification

Load this reference when classifying how each finding's fix should be applied. The binary AUTO-FIX/ASK split is a special case of the 4-tier taxonomy below — the tiers prevent "mechanical fix across a risky boundary" from sliding into AUTO-FIX.

Conflict-resolution rule: when multiple agents disagree on tier for the same finding, always take the more conservative route (safe_auto → gated_auto → manual → advisory is the escalation direction). Never promote a gated_auto to safe_auto because one agent classified it loosely — that's how security fixes ship unreviewed.

Tier decision rule: if a senior engineer would apply the fix without discussion AND the change doesn't cross a behavior/contract/permission boundary, it's safe_auto. When in doubt, escalate to gated_auto.

What to Check — Review Category Checklists

Load this reference during the line-by-line review step. Use the category lists to structure your reading and ensure nothing slips through. Each category corresponds to a class of defect that surfaces repeatedly in production code.

Correctness

• Edge cases (null, empty, boundary values, concurrent access)
• Error paths (are failures handled or swallowed?)
• Type safety (implicit conversions, any types, unchecked casts)
• New enum/status/type values — trace through ALL consumers (switch/case, filter arrays, allowlists). Read code outside the diff. Missing handler = wrong default at runtime.

Maintainability & Readability

• Naming — variables, functions, and classes convey purpose without needing surrounding context
• Function length — long functions that force scrolling; prefer extractable blocks with clear names. Split by responsibility, not line count
• Nesting depth — more than 3 levels of indentation signals a need for early returns, guard clauses, or extraction
• Comment quality — comments explain WHY (constraints, workarounds, non-obvious decisions), not WHAT. Flag comments that restate code or will rot as the code changes
• God classes / SRP violations — class with unrelated responsibilities. Split into focused classes
• Leaky abstractions — implementation details exposed in interfaces or public APIs

Performance

• N+1 queries (loop with query per item — use batch/join instead)
• Unbounded collections (arrays/maps without size limits)
• Missing indexes on queried columns

Adversarial (red-team pass)

• Silent failures — .catch(() => []) or log-and-forget patterns that swallow errors and return success
• Trust assumption exploits — frontend-validated data not re-validated on the backend; internal service inputs treated as trusted
• Edge cases under pressure — max input size, zero items, first-run-ever, double-click within 100ms, concurrent identical requests
• Partial completion — operations that can crash mid-way leaving state inconsistent (no rollback, no cleanup)

AI-generated code lens

Apply when the code is LLM-authored (most diffs are):

• Over-engineering: gratuitous defensive checks for cases the type system or framework already prevents; unnecessary abstraction for a single call site; premature generalization of one concrete case into a generic utility
• Defensive noise: try/catch around operations that cannot throw; null checks on values the signature guarantees non-null; input validation on internal code boundaries already validated upstream
• Cost bloat: long chains of model-cost-inducing work (recursive agent dispatch, per-item API calls, unbounded loops) where a single batch or deterministic routine would suffice
• Scope drift: "while I'm here" edits to unrelated files; rename refactors piggybacking on a bug fix; formatting churn that dwarfs the real change

Flag these as simplification findings, not bugs. The fix is usually deletion, not addition. For a deeper YAGNI pass on an AI-heavy diff, dispatch ia-code-simplicity-reviewer — its six named traps (while-I'm-here, for-future-flexibility, defensive-coding, modernization, consistency, cleanup) map onto this lens and produce a structured simplification report.

Deep Review Process

Multi-agent review that dispatches parallel specialist agents, each analyzing the same diff through a single lens. Produces a unified, deduplicated report.

Specialist Agents

Dispatch all agents in parallel (read-only, safe to parallelize). Each receives the full diff, the PR description/intent, and the scope resolution results.

Agent Prompt Template

Each specialist receives:

Review this diff as a {lens} specialist. Focus exclusively on {focus area}.

DO:
- Read the actual code line-by-line. Trace logic through the diff, not around it.
- Compare every claim made in the PR description against what the diff actually does.
- Quote the specific code that triggers each finding so the author can locate it.
- Treat the PR description as a claim to verify, not a truth to accept.

DON'T:
- Take the author's summary at face value. "Refactored X" may hide behavioral changes.
- Accept "this is covered by tests" without checking the test files in the diff.
- Rubber-stamp sections you didn't open. If you didn't read it, you didn't review it.
- Extrapolate from the description when the code contradicts it -- the code wins.

DIFF:
{full diff content}

PR INTENT:
{PR description or task spec}

SCOPE:
{files list with change types: Added/Modified/Deleted}

Return findings in this format:
- **[file:line]** `quoted code` -- [issue]. Confidence: [0.0-1.0]. [Impact]. Fix: [suggestion].

Only report findings in your domain. Do not comment on other dimensions.
Apply the confidence rubric: suppress anything below 0.60 confidence.
Limit to 10 findings, highest severity first.

Model Selection

• standards, correctness, security, data-migration: use default model (standards reading is high-precision; the others require deeper reasoning about logic, attack surfaces, or data safety)
• testing, maintainability, performance, reliability, cloud-infra, api-contract: use opus (reasoning about absence -- untested paths, missing error handling, breaking changes -- requires deep code comprehension)

Override: if the diff touches auth, payments, or crypto, upgrade security to opus.

Red-Team Pass (Second Phase)

After the parallel specialists return, dispatch a single red-team agent that receives the diff AND the combined specialist findings. This agent looks for what the specialists missed:

• Happy-path assumptions that break under load or unusual input sequences
• Silent failures where errors are swallowed without logging or alerting
• Trust boundary violations (user input flowing into privileged operations without re-validation)
• Cross-category issues that fall between specialist domains
• Integration boundary gaps where two systems meet

Dispatch the red-team pass when: diff >200 lines, OR any specialist found a Critical finding. Skip for small/simple diffs where the parallel pass is sufficient.

Red-team findings merge into the main report with a [red-team] tag. Use default model.

Merge Algorithm

After all agents return, apply these rules in order. Each consolidated finding carries its original CR-XXX ID from the first agent that reported it so PR threads can reference specific findings unambiguously.

Preamble — fingerprint first. Before applying any numbered rule, group findings across agents by fingerprint path:line:issue_class. The rules below operate on these groups: a group of size 1 is handled by rule 5 (single-agent hit), a group of size 2 by rule 6, a group of size 3+ by rule 7. Confidence boosts apply once per group, not per matching rule — use rule 7 if applicable, otherwise rule 6.

1. Same file:line + same issue class → merge into one finding. Keep the higher-severity rating and the more actionable fix text. 2. Same file:line + different issue class → keep both. Tag as "co-located" in the output so the author sees they share a line. 3. Conflicting severity on the same merged finding → always take the higher severity. Do not average. 4. Conflicting recommendations → present both and mark as NEEDS DECISION. Do not silently pick one. 5. One agent flags, others don't → keep the finding if confidence ≥0.70; suppress otherwise. A single agent's low-confidence hit is usually noise. 6. Two agents agree (2+) → fingerprint findings as path:line:issue_class; when two specialists hit the same fingerprint, tag the merged finding MULTI-SPECIALIST CONFIRMED ({s1} + {s2}) and boost confidence by +0.05 (capped at 1.0). Two-agent overlap is evidence-worthy even when it's below the 3-agent threshold. 7. All agents agree (3+) → boost confidence by 0.10 (capped at 1.0). Convergent findings from independent perspectives are more trustworthy. Tag as MULTI-SPECIALIST CONFIRMED ({s1} + {s2} + {s3}...). 8. Apply confidence rubric → suppress findings below threshold per the main skill's rubric. 9. Apply false-positive suppression → remove entries matching the categories in the main skill. 10. Sort by severity (Critical > Important > Medium > Minor), then by confidence within each level. 11. Cap total findings at 20 across all agents. If more exist, note the overflow count.

Skeptic Pass

After the merge algorithm produces the consolidated list, run one Skeptic dispatch over the findings whose confidence is ≥0.70 (post-boost). The Skeptic's job is the opposite of the specialists': take each surviving finding and try to disprove it. Specialists are rewarded for catching bugs; the Skeptic is rewarded for catching false positives.

When to run: any deep review with at least one finding at ≥0.70. Skip if all findings are below that bar (the confidence rubric already suppresses them).

Single dispatch, not per-finding. One agent call carrying the full diff and the consolidated finding list. Per-finding dispatch is wasteful — most disproof attempts fail in the same way (reading the same dispatch guard, the same null check upstream).

Skeptic Prompt Template

You are a Skeptic. The findings below survived a parallel multi-agent code review. Your job is to find ONE concrete reason each finding is wrong, before it lands in the final report.

For each finding, attempt one of:
- REACHABILITY: trace upstream callers. Does any dispatch guard, null check, or branch condition prevent the buggy path from firing under attacker-reachable input? If yes, name the guard with file:line.
- FRAMEWORK BEHAVIOR: does the framework/library actually behave as the finding assumes at the project's pinned version? Cite the docs or the framework source if the finding is wrong.
- TEST EVIDENCE: does the existing test suite already exercise the alleged bug? If a passing test covers the exact path the finding worries about, the finding is likely speculative.
- DUPLICATE: does the finding describe the same defect as a higher-severity finding already in the list? The test is root-cause, not signature -- two findings are duplicates if fixing one fixes the other, even when their file:line or wording differs. Mark for merge.

Per finding, return one of:
- DISPROVED — concrete counter-evidence (file:line of the upstream guard, doc URL, passing test name). Drop or demote to advisory.
- WEAKENED — partial counter-evidence. Reduce severity by one tier and keep.
- HELD — no counter-evidence found. Keep as-is.

DO NOT invent counter-evidence. If you cannot find a real upstream guard, doc citation, or covering test, return HELD. Inventing a phantom guard is worse than letting a false positive through — the author then ignores a real bug because "the Skeptic disproved it."

DIFF:
{full diff content}

CONSOLIDATED FINDINGS (only those with confidence ≥0.70):
{findings list with CR-IDs}

Applying Skeptic Output

• DISPROVED with concrete citation → drop the finding. Note in output header: Skeptic dropped N finding(s).
• DISPROVED without citation, or vague handwave → ignore the disproof. The Skeptic must produce evidence, not opinion.
• WEAKENED → demote one severity tier. Tag the finding [skeptic-weakened: <reason>] so the author sees the partial counter.
• HELD → keep. Tag [skeptic-held] only on findings the Skeptic explicitly examined; this is positive signal that the finding survived adversarial review.

Why this differs from the red-team pass

Red-team looks for what specialists missed (additive). Skeptic challenges what specialists found (subtractive). Both phases run in deep review when triggered: red-team after parallel specialists, Skeptic after merge. They produce opposite-direction edits to the finding list.

Output Format

Same as the standard review output format, with an additional header:

## Review: [brief title] (deep)
Agents: correctness, security, testing, maintainability, performance, reliability [+ conditional: api-contract, data-migration, cloud-infra] [+ red-team if triggered]
Cross-lens agreements: N findings tagged MULTI-SPECIALIST CONFIRMED (K at 3+, M at 2)
Skeptic: examined K findings, dropped D, weakened W, held H (when Skeptic pass ran)

### Critical
...

Include the count of multi-specialist-confirmed findings in the header so reviewers can scan for convergent signal without reading every finding.

When Deep Review Adds Less Value

• Pure documentation/markdown changes -- single-pass is sufficient
• Mechanical refactors (renames, moves) with no logic changes -- single-pass catches drift
• Single-file changes under 50 lines -- multi-agent overhead isn't justified
• The user explicitly requested a quick review

In these cases, fall back to standard single-pass even if complexity signals triggered.

Driving a long-running external reviewer subprocess

When a review is delegated to an external CLI that runs as a subprocess and can take many minutes (codex review, claude -p, a slow test/--parallel-tests reviewer, a /code-review ultra cloud run), the failure mode is operational, not analytical: the reviewer gets killed or re-run prematurely.

Heartbeat tolerance -- don't kill a quiet-but-alive review

Treat progress lines like review still running: elapsed=… pid=… as healthy, not a hang. A long reviewer goes quiet for minutes between heartbeats while a model call or a test suite runs. Do not SIGKILL it just because:

• it has been quiet for 2-5 minutes, or
• it is still running under its declared time budget (e.g. a 30-minute cap).

Inspect or kill only after: multiple missed expected heartbeats, the budget is exceeded, or the subprocess has obviously failed (nonzero exit, broken pipe). Capture stdout/stderr to a file so a quiet tail isn't mistaken for a dead process.

Closeout loop -- run until clean, then stop

• Keep iterating (fix → re-run the external review) until it returns **no

accepted/actionable findings** -- a structured exit 0, not a prose "looks good".

• Stop as soon as it exits clean. Do not run one extra review just to get a

nicer "all clear" summary -- that burns time/tokens and risks new churn.

• Bind the review to one frozen diff bundle (base SHA … head SHA) so every

iteration reviews the same surface; don't re-derive scope mid-loop (see "Base-branch resolution for branch reviews" in the main skill).

False Positive Suppression

Not every potential issue is worth raising. False positives waste author attention and erode trust in the review process.

Suppression Categories

Before reporting a finding, check whether it falls into one of these categories. If it does, suppress it.

1. Pre-existing issues

The finding exists in code that was NOT changed in this diff. Don't raise issues on surrounding code unless they interact directly with the changes. If surrounding code has a real problem that's exposed by the change, note it as informational with the distinction clear.

2. Linter/formatter covered

Style issues that the project's linter or formatter already enforces (or should enforce). Don't duplicate automated tooling. If the project lacks a linter and should have one, note that once in the summary, not per-finding.

3. Intentional design

Code that looks unusual but is deliberately written that way. Signals: comment explaining why, consistent pattern elsewhere in codebase, matches a documented architectural decision, performance-critical section. When uncertain, use question-based feedback ("Was this intentional?") rather than flagging it as a defect.

4. Already handled elsewhere

The "issue" is actually handled in a different layer (middleware validates input, framework handles escaping, type system prevents the error class). Verify the handling exists before suppressing.

5. Generic suggestions

"Consider using X instead of Y" without evidence that Y causes a problem in this specific context. Suggestions need a concrete reason: performance data, maintainability argument tied to this codebase, security concern with evidence.

6. Framework/library internals

Flagging patterns that are idiomatic for the framework in use. Examples: Laravel facades, React hook dependency arrays with stable references, Go error wrapping patterns. Review the code against its framework's conventions, not abstract ideals.

7. Test-specific patterns

Test code follows different rules than production code. Don't flag: hardcoded test data, assertion-heavy functions, mock setup boilerplate, test helper utilities that duplicate production logic for clarity. Do flag: tests that don't actually assert anything, tests that test the mock instead of real behavior.

8. Readability-aiding redundancy

"X is redundant with Y" when the redundancy aids readability. "Add a comment explaining this threshold" when thresholds change during tuning and comments rot. "This assertion could be tighter" when it already covers the behavior. Consistency-only reformatting to match adjacent code style. "Regex doesn't handle edge case X" when input is constrained and X never occurs. Anything the author already fixed in a later commit within the same diff, flagged in their own PR comments, or resolved by a prior reviewer.

When to Override Suppression

Suppress rules don't apply when the finding is Critical severity (security vulnerability, data loss, crash). Critical findings are always reported regardless of category, though they should still include evidence.

Language-Specific Review Profiles

Load the relevant profile(s) based on file extensions present in the diff.

Verifying framework idioms before flagging

Before filing a finding that claims a framework or library behaves a certain way (e.g. "this Eloquent relation runs N+1", "this Next.js cache invalidation is wrong", "this React effect leaks"), verify against current docs at the project's pinned version. Memory-based recall of framework behavior is unreliable across versions; patterns that were traps in one major are often fixed in the next.

If the Context7 MCP is available in the harness, use it:

• resolve-library-id — resolve the library/framework name (e.g. react, next.js, laravel) to a Context7 library ID.
• query-docs — fetch the relevant documentation for that library ID, scoped to a natural-language query, before quoting behavior.

Pin the lookup to the project's actual version. Read package.json, composer.json, requirements.txt, go.mod, or Cargo.toml to identify the major version, then constrain queries (e.g. "Laravel 11 HasOneOrMany limit eager-load behavior").

If Context7 is unavailable, fall back to the vendor's official docs URL directly via the harness's web fetch tool. Do not skip verification — a finding that asserts framework behavior without a citation is worse than no finding, because authors trust review output.

When the verified behavior contradicts the finding's premise, drop the finding and (if reviewing a real diff) add the version-correct behavior to the relevant entry in review-traps-catalog.md so the next review starts smarter.

TypeScript / React (.ts, .tsx, .jsx)

• Hook dependency bugs (stale closures in useEffect)
• any escape hatches -- flag each with a concrete type suggestion
• Unchecked nullable access (?. chains that silently swallow nulls)
• Missing key props in mapped JSX
• Effects without cleanup (subscriptions, timers, event listeners)

Python (.py)

• Mutable default arguments (def f(items=[]))
• Bare except: -- always catch specific exceptions
• Missing async/await (sync call in async context)
• f-string injection in SQL/shell -- use parameterized queries
• type: ignore without justification

PHP (.php)

• SQL injection via string concatenation -- use prepared statements
• Missing declare(strict_types=1) at file top
• Type coercion traps (loose == vs strict ===)
• Mass assignment without $fillable guard
• Unvalidated request input passed to Eloquent

Shell (.sh, .bash, CI configs)

• Unquoted variables ($var vs "$var")
• Missing set -euo pipefail
• Command injection via unsanitized input in eval or backticks
• cd without error check -- use cd dir || exit 1
• Hardcoded paths that should be variables

Configuration (.env, .yml, .yaml, .json, .toml)

Use numbered IDs (CFG-001 ... CFG-006) so config-specific findings can be referenced unambiguously when a review turns up several related config issues:

• CFG-001 Plaintext secrets: API keys, passwords, tokens, DB URIs committed in config files. Use secret managers or .env excluded from VCS.
• CFG-002 Magnitude-change without baseline: a config value shifts by >2x (rate limits, batch sizes, pool caps, retry counts) without a PR-body justification or pre-change baseline measurement. High-magnitude shifts need explicit reasoning.
• CFG-003 Timeout / retry hierarchy inversion: inner call has a longer timeout than outer, or retries compound across layers (client 3× on top of SDK 3× = 9 attempts). Either cascades into thundering-herd failures.
• CFG-004 Pool / limit mismatch: connection pool, worker count, or queue depth does not match the downstream capacity (DB max_connections, upstream rate limit, available memory). Starves under load or overwhelms the downstream.
• CFG-005 Env drift: development values (localhost, short timeouts, verbose logging, permissive CORS) copied to production config without proportional scaling.
• CFG-006 Rollback / observability gap: risky config change lacks a feature flag, canary rollout, or reversible plan; or lacks the metric/alert needed to detect a regression post-deploy.

Data Formats (.csv, .json ingestion, parsers)

• Missing encoding declaration (UTF-8 BOM handling)
• No size/row limit on ingested files (memory exhaustion)
• Trusting field count/shape without validation

Security (all files)

• Show attacker-controlled input path to vulnerable sink, not just "possible injection"
• Injection vectors: SQL, XSS, CSRF, SSRF, command, path traversal, unsafe deserialization
• Race conditions: TOCTOU, check-then-act

LLM Trust Boundaries

• LLM-generated values (emails, URLs, names) written to DB or mailers without format validation
• Structured tool output accepted without type/shape checks
• 0-indexed lists in prompts (LLMs return 1-indexed)
• Prompt text listing capabilities that don't match what's wired up

PR sizing and large-diff strategy

Large diffs (>500 lines)

Review by module/directory rather than file-by-file. Summarize each module's changes first, then drill into high-risk areas. Flag if the PR should be split.

Change sizing

Ideal PRs are ~100-300 lines of meaningful changes (excluding generated code, lockfiles, snapshots). PRs beyond this range have slower review cycles and higher defect rates. When a PR exceeds this, suggest splitting using one of these strategies:

• Stack -- sequential PRs where each builds on the previous, merged in order.
• By file group -- group related files (e.g., model + migration + tests) into separate PRs.
• Horizontal -- split by layer (frontend, API, database).
• Vertical -- split by feature slice (each PR delivers one user-visible behavior end-to-end).

Reliability Patterns

Review lens for operational resilience: what happens when things go wrong at runtime.

Error Handling Completeness

• Swallowed errors: empty catch blocks, .catch(() => {}), bare except: pass. Every error must be logged, re-thrown, or explicitly documented as intentional.
• Partial error handling: catching at the top but not handling failures from intermediate steps. If step 2 of 5 fails, are steps 1's side effects cleaned up?
• Error type specificity: catching broad exception types (Exception, Error) when only specific failures are expected. Broad catches mask unexpected bugs.
• Error context stripping: re-throwing without the original cause/stack. Wrap, don't replace.

Timeout and Cancellation

• Unbounded external calls: HTTP requests, DB queries, queue operations, file I/O without timeouts. Every external call must have an explicit timeout.
• Timeout propagation: if a request has a 30s timeout but calls three services sequentially, each needs a fraction of the budget, not the full 30s.
• Cancellation handling: long-running operations should respect cancellation signals (AbortController, context cancellation, CancellationToken). Check whether in-flight work is abandoned or cleaned up.

Retry Logic

• Retry without idempotency: retrying a non-idempotent operation (payment charge, email send) causes duplicates. Verify idempotency before adding retry logic.
• Retry without backoff: immediate retries under failure just amplify load. Use exponential backoff with jitter.
• Unbounded retries: max attempts must be finite. Infinite retry loops become resource exhaustion.
• Retry surface: retry at the right layer. Retrying an entire transaction because one HTTP call failed wastes work. Retry the call, not the transaction.
• Double retry (stacked retry layers): application @retry wrapping a client SDK that already auto-retries multiplies attempts (3×3 = 9) and the backoff compounds — a nominal 5s timeout becomes 30s+. Audit the client's default retry policy before wrapping it. Retry at exactly one layer: if the SDK retries, configure its policy; do not add another @retry on top.

Circuit Breakers

When calling a flaky upstream service:

• Missing circuit breaker: repeated calls to a failing service waste resources and slow everything downstream. Open the circuit after N consecutive failures, half-open to probe recovery.
• No fallback: when the circuit is open, what happens? Graceful degradation (cached data, default response, feature flag) beats a 500 error.

Resource Cleanup

• Connection/handle leaks on error paths: DB connections, file handles, locks acquired in try blocks must be released in finally/defer/context manager. Check BOTH success and error paths.
• Pool exhaustion: if connections are acquired but not returned on timeout or error, the pool drains over time. This is a slow-burn production incident.
• Subscription leaks: event listeners, WebSocket connections, pub/sub subscriptions registered without corresponding unsubscribe on teardown.

Queue and Job Resilience

• No dead letter queue: failed jobs that exceed retry limits must go somewhere observable, not disappear silently.
• No job idempotency: workers may receive the same message twice (at-least-once delivery). The handler must be safe to re-execute.
• Missing visibility timeout: if a worker crashes mid-processing, the message must become available again within a bounded time.

Detection Patterns

Grep-able signals that often indicate reliability gaps:

# Empty catch blocks
catch\s*\([^)]*\)\s*\{\s*\}
except:?\s*$\n\s*pass

# HTTP calls without timeout
fetch\(.*\)(?!.*timeout)
requests\.(get|post|put|delete)\((?!.*timeout)
axios\.(get|post|put|delete)\((?!.*timeout)

# Retry without backoff
retry.*max.*(?!.*backoff|delay|sleep|wait)

Review Traps Catalog

Concrete review-reasoning failure modes harvested from real Codex cycle disagreements and review post-mortems. Each entry states the Trap (what reviewers do wrong), the Reality (what's actually true), and the Fix (what to do instead). Load this file when running a code review, especially when about to file findings with words like "should", "might", "could break", "what if", or "pattern suggests."

Reachability before severity

Trap: finding a genuine mechanical defect in an internal function (infinite loop, unbounded pointer advance, missing bound check) and filing it as a security issue without verifying that the function is reached from any public API path. Code trace and second-opinion review can both agree on the mechanics while missing that the enclosing dispatch short-circuits before the buggy function runs.

Reality: for a finding to be a security issue, an attacker must be able to reach the buggy code. Dispatch guards like if (i <= 0) upstream, NOTMIME-mode short-circuits, or unreachable conditional branches turn real mechanical defects into dead code from the user-facing API's perspective. Reachability review survives code-trace agreement — two reviewers reading the same function in isolation will both confirm the mechanic and both miss the same dispatch guard.

Fix: for every security finding that names a specific internal function, list every caller on every compiled path and trace the conditions under which each call actually fires. Build a real reproducer before filing. For UB-style findings without a sanitizer trap, verify the standard sanitizer toolbox actually covers the UB class — -fsanitize=pointer-overflow catches arithmetic wrap past UINTPTR_MAX and NULL-base offsets, not "pointer leaves its referent object." If no sanitizer fires and no crash is reproducible, the finding is spec-level UB, not a security issue.

Docs-idiom smoke test for API-hardening changes

Trap: tightening a public-API method so a previously-swallowed failure now throws. Correctness verified against the call graph and existing tests; canonical documentation example not exercised. Tests pass, the change ships, a user runs the docs example a week later and files a bug.

Reality: official documentation shows the idiom users copy. Public APIs carry an implicit contract with the docs, not just with the test suite. "Every test passed" does not prove "every documented usage still works."

Fix: for any change to a widely-used public method, find the canonical example in the official docs and run it against the patched build before declaring done. If the harness has the Context7 MCP, prefer query-docs (after resolve-library-id) for the library at the project's pinned version over a raw web grep — it returns versioned official sections, not SEO blog pollution. Otherwise grep the official docs directly (php.net/manual/en/<class>.<method>.php, library README, Sphinx docs). Add the docs idiom to the test suite as a standing smoke test.

Key-vs-label: open three files before flagging

Trap: when a string field is passed into a form feeding a <Select> whose options use keys, flagging "if the source is a label, the form will submit a label." The assumption isn't verified.

Reality: a five-second check of (a) the API resource/DTO, (b) the fixture/mock, (c) the schema (Zod/Pydantic/etc.) usually settles it. If all three say key, the bug didn't exist.

Fix: before writing a "key vs label" finding, open the three sources above. Only file if at least one of them passes a label through.

"Convention is X" from a 3-file sample

Trap: reviewing a new file in a populated directory, grepping 2-3 siblings, spotting a pattern, and citing it as the convention. If the sample is small and non-random, the "convention" often isn't one.

Reality: directories with 40+ similarly-shaped files frequently have splits. Different authors established different local patterns over time. The 3 files opened happened to use one pattern; the 11+ not opened use another. Both are equally established.

Fix: before writing a "inconsistent with convention" finding, grep across the whole directory, not just neighbours. If both patterns have >3 examples, there is no convention — drop the finding. Only cite "convention" when the evidence is overwhelming (say, >80% of the population on one side).

Consult convention docs BEFORE reading the diff

Trap: projects with agents/*.md, CLAUDE.md, or similar convention docs contain cheap-to-catch rules. Starting review with "look at the diff, see what looks off" misses rules that are in plain sight.

Reality: memory-based recall of project conventions is unreliable, including from reviewers who have read the docs before. Rules that were obvious in the convention doc slip through review.

Fix: for every diff, identify which area it touches (DB migration, audit trail, routing, auth), open the matching convention doc first, and scan for applicable rules. Treat it like a checklist: rule → check diff → either dismiss or flag. Only after that pre-flight, open the diff.

Speculative future-design findings on greenfield code

Trap: reviewing a brand-new feature with no prior consumers, reaching for "what if later..." findings to look thorough — pagination metadata on fixed-N endpoints, polymorphic ID collision worries on UUID models, hardcoded strings in projects with no i18n. Each dressed up as Medium/Minor but with no concrete failure mode in the diff or its near-term consumers.

Reality: greenfield code has no real bugs adjacent to the diff, so the urge to produce a "complete" review surfaces design-future-facing commentary. Our skill explicitly suppresses "generic suggestions without a concrete failure mode" but the rule loses to thoroughness pressure.

Fix: before writing a Medium/Minor finding on new code, ask "what specifically breaks today, or which committed near-term consumer breaks?" If the answer is "later, if X is added" or "if a different shape is needed", drop it. Treat a speculative classification from a second reviewer as confirmation, not an invitation to debate.

"Misnamed class" without reading what the type represents

Trap: when a class name has a noun like File or Record and the body manipulates a model with a different surface name (e.g., DeleteProviderDocumentFileAction operating on a Document model), flagging it as "misnamed". The reasoning is shape-based.

Reality: in many codebases the model name is the domain entity. Document may be the file entity (with storage_path, final_path, thumbnail_path). Reading the model for two lines settles it.

Fix: before flagging "misnamed" or similar naming critique, open the referenced model and skim the columns/methods. If the model represents the noun in the class name, drop the finding. Naming critiques ungrounded in what the type actually represents are noise.

Pattern-matching validation from sibling fields

Trap: a diff adds a new field that "looks like" an existing one (e.g., fax alongside phone). Flagging "why doesn't fax have the same format rule as phone?" The reasoning is analogical.

Reality: the project often already has a convention for the new field across other endpoints that differs from the sibling. A 10-second grep settles it.

Fix: before flagging "field X should use validation rule Y", grep the codebase for 'X' across request classes, resources, and forms. If multiple files treat the field the same way the diff does, the diff is following convention — drop the finding. Analogy to a different field is not evidence.

Consumer doesn't handle new enum case — but does the default break?

Trap: a diff adds a new enum case; greenping every consumer that matches on the enum and flagging each one that doesn't include the new case.

Reality: the mere absence of a case in a match is not a bug. What matters is whether the default / fallback path produces a wrong runtime outcome. Two patterns to distinguish:

1. default short-circuits correctly (returns empty array for a list endpoint, returns null for an optional lookup, throws, logs and skips). Absence of the new case is fine. 2. default returns an empty result that gets silently forwarded into an update/create/delete, producing 200 OK with no work done. Silent-success bug.

Only pattern 2 is a finding.

Fix: for each consumer missing the new case, follow the code path from default to the caller's response. If the caller's behavior under default is already semantically correct for the new case, drop the finding.

Paired-enum invariant drift

Trap: adding a case to enum A without mirroring it in a semantically-sibling enum B used one ORM layer away. Type system doesn't enforce the pair; CI is green and tests pass; production ships a write-then-read crash.

Reality: frameworks let request validation and model casts pick enums independently. Two enums with overlapping but non-identical cases silently drift. A validator using enum A as a superset accepts the new case, persists it, then the model's cast to enum B throws on every subsequent read. The failing layer is nowhere near the change site.

Fix: when adding an enum case, grep every Rule::enum(ThisEnum::class) and every ThisEnum::class cast reference. Check for sibling enums with overlapping cases — paired invariants nothing in the type system protects. If the sibling isn't updated in the same change, write-then-read will break.

New endpoint that duplicates existing behavior

Trap: reviewing a new controller/action/endpoint that does roughly what an existing one already does (destroy a resource, update a resource). Focus pattern-matches to the diff in front of you; the existing implementation is out of mind. You miss that the new endpoint skips guards, policies, soft-delete logic, or cascade handling the existing one already figured out.

Reality: existing implementations on the same resource encode hard-won rules about completed-state protection, cross-tenant scope, soft-delete vs force-delete, audit trails, side-effect ordering. A new alternative endpoint is a high-probability regression vector unless it explicitly reuses the existing flow.

Fix: before writing findings on a new destroy/update/create endpoint, grep for existing destroy/update/create methods on the same resource. Read them in full. Diff every guard and side effect against the new implementation. What's missing is the finding.

Confirmation-style findings dressed up as nits

Trap: writing "nit: I noticed X is pre-existing behavior and the diff doesn't touch it, just confirming the intent is Y." Two signals that the finding has slipped from actionable into noise: (1) the body explicitly notes the behavior is unchanged by the diff, (2) the ask is for the author to confirm intent rather than propose a change.

Reality: review is an author-facing channel. If the finding has no change for the author to make, it's not a comment — it's a note-to-self. Posting inflates review size, dilutes signal of the real findings, and trains the author to skim.

Fix: before posting a nit, ask "what action does this request?" If the answer is "confirm this is intentional", delete the comment. Keep the observation in internal review notes if it matters.

Hypothetical queue/cache concerns without grounding

Trap: enum renames and schema migrations often raise "queue jobs will break on deserialization" or "cached values will mismatch" concerns. Valid classes of risk, but the review comment needs to point to an actual job/cache site that serializes the affected value — otherwise it's a template concern, not a finding.

Fix: before flagging a queue/cache concern, grep for jobs/cache writes that include the affected type as a serialized field. If no such site exists in the diff or in grep results, drop the concern or explicitly label as hypothetical ("if any jobs serialize X directly, this will break — we didn't find any").

Defensive nit not evidenced by data already flowing the same pattern

Trap: drafting a defensive finding ("what if documents is ''? json_decode returns null and foreach (null) errors") on a code path where the same construct has been used by N prior migrations against the same tables in production without incident.

Reality: the hypothetical edge case isn't in the data, and prior migrations are positive evidence that it isn't. If the convention itself is wrong, fix it as a separate cross-cutting cleanup.

Fix: before flagging a defensive nit, grep for the same pattern in adjacent code. If three prior migrations use the identical pattern over the same tables without incident, drop the finding.

Findings on lines outside the MR diff

Trap: reading a new file in a diff, flagging something in surrounding code that was already on the base branch. The reviewer sees the guard/handler/early-return in context and assumes it's part of the change.

Reality: many code-hosting platforms reject comments anchored to lines outside the diff (GitLab DiffNote, GitHub inline comments on unchanged lines). Even when accepted, the finding is out-of-scope for the current change.

Fix: before drafting a comment, confirm the target line is actually inside the MR's diff. git diff <base>...<head> -- <file> is authoritative. If the line isn't in the hunks, either drop the finding or reframe as follow-up: "this behavior is pre-existing but worth addressing separately" — raise as a separate issue, not an inline comment.

Cross-repo contract claims need current remote state

Trap: when a review cites cross-repo backend contracts (routes, schemas), the reviewer's view of the other repo is whatever's in their local working tree — which may be stale. A confident "this endpoint doesn't exist" can be wrong if the companion change has already merged on origin/develop.

Fix: when making a cross-repo contract claim, verify with git show origin/develop:path/to/file before acting. If local is behind, git fetch and re-read. When handing a diff to a subagent for review, note which SHA the review is supposed to be against; LLM tools that supplement from the filesystem will otherwise read pre-change state.

Language-specific gotchas reviewers re-discover

PHP 8 property-access on null does NOT fatal. null->foo emits a Warning and evaluates to null, which the ?? operator catches. Only method calls (null->foo()) throw. Before flagging ?-> (null-safe operator) as a required fix for "potential 500", confirm the suggestion changes runtime behavior beyond warning-level log noise.

PHP `json_encode` comparison is type-safe. json_encode(1) vs json_encode("1") produces 1 vs "1" — distinguishable. json_encode($a) !== json_encode($b) is a valid deep-equality check for JSON-serializable values.

Laravel 11+ `HasUuids::newUniqueId()` returns `Str::uuid7()` (time-ordered). latest('id') on a UUIDv7 PK sorts chronologically — the "UUIDs sort lexicographically, not chronologically" trap only applies to Laravel ≤10 or models overriding newUniqueId().

Laravel 11+ `HasOneOrMany::limit()` in an eager-load is per-parent, not global. ->with(['relation' => fn ($q) => $q->limit(N)]) uses groupLimit when $this->parent->exists is false (eager-load path), which the older "this limits rows total, not per parent" finding no longer applies to.

When flagging a language/framework idiom as broken, first check the vendor source for the current version's behavior. Patterns that were traps in v10 often aren't in v11. If the harness has the Context7 MCP, run query-docs (after resolve-library-id) against the library at the project's pinned version (composer.json / package.json / requirements.txt / go.mod) before filing — see language-profiles.md "Verifying framework idioms before flagging" for the exact protocol.

Same-name symbols across Enum / Model / DTO / Request

Trap: a codebase can have two classes with the same short name in different namespaces (e.g., App\Enums\Foo\Bar + App\Models\Foo\Bar). Citing a validation/serialization rule tied to "ClassName" without verifying which namespace binds. Result: mechanism wrong even if conclusion right.

Fix: when asserting "X is validated via Rule::enum(Y::class)" or similar, open the actual validator/request/casts and read the imports. Confirm which FQN is in scope. If the symbol is ambiguous, say so in the finding and defer the mechanism claim.

Column-level rename misses JSON-embedded values

Trap: reviewing a migration that renames foo = 'a' to foo = 'b', checking every table with a foo column and declaring the rename complete. In codebases that also store the same semantic value inside JSON columns (requirement payloads, config snapshots), the column-level audit misses the JSON sites.

Fix: before declaring a column-level rename complete, grep the full migration history for past renames of the same semantic. Past rename migrations are the best index of where the value lives — both columns and JSON payloads.

Error-string match against uncaptured subprocess output

Trap: a finding (or a test) that asserts on a captured error string from a spawned subprocess -- expect(err.message).toContain("ENOENT"), assert "syntax error" in str(exc), matching $result->getMessage() against a tool's diagnostic. The reviewer accepts it as a real check on the program's output.

Reality: when a child process is spawned with stdio: 'inherit' (Node), subprocess.run(...) without capture_output=True (Python), passthru/proc_open with inherited descriptors (PHP), or any pipe the parent never reads, the child's diagnostics stream straight to the terminal -- they never land in the exception. error.message then holds only the command line ("Command failed: tsc --noEmit"), not the program's actual output. The matcher matches (or misses) the command string, so the assertion passes or fails for a reason unrelated to what the subprocess printed. A test that "checks the compiler reported an error" actually checks that the word appears in the invocation.

Fix: when a finding or test matches on an error string from a subprocess result, trace how the child's stdout/stderr is captured before trusting the match. Confirm the spawn captures output (stdio: 'pipe' / collecting child.stderr; capture_output=True or stderr=PIPE; 2>&1 into a read buffer; proc_open with pipe descriptors the parent reads) and that the matched string is asserted against that captured stream, not against error.message/the command line. If the output is inherited or uncaptured, flag the assertion as matching the command string rather than the program output -- it passes for the wrong reason. Suggest asserting on the captured stream, or on exit code when only success/failure matters.

Scope & comparison-range resolution

Git/gh plumbing for setting up a review: deriving the comparison range for a branch review, and fetching prior discussion before raising findings. The core file-selection fallback chain stays in the main skill; this covers the two detailed cases.

Working-tree safety: never reorganize the user's checkout to review

A review is read-only on the working tree. Setting up a review must not mutate what the user has in progress. Before any other setup step, run:

git status --short --branch -uall

Treat every modified, staged, and untracked file in that output as the user's work-in-progress, not as clutter to clear. Do not, as review setup, run any of: git switch / git checkout <branch>, git reset --hard, git clean, git stash / git stash -u, or gh pr checkout. Each silently relocates or destroys uncommitted work.

Moving untracked work "out of the way" is the same interference, not a safeguard: do not copy or move the user's WIP to /tmp, a backup dir, or any location outside the checkout to "protect" it. Relocating someone's uncommitted work is the same class of harm as stashing it -- it leaves the tree in a state the user did not create and cannot predict.

If the target diff genuinely requires a different branch or a clean tree, stop and ask before switching, stashing, resetting, or cleaning. Reviewing a branch does not require checking it out -- resolve the comparison range and read the diff range directly (see "Base-branch resolution for branch reviews" below); a remote branch reads via git diff <base>...<branch> without touching the working tree.

HEAD-drift guard (when the review ends in a stage/commit/push): record the commit before staging and re-check before the write:

before=$(git rev-parse HEAD)
# ... review, then stage ...
[ "$(git rev-parse HEAD)" = "$before" ] || echo "HEAD moved since review start -- stop and report"

If HEAD moved, or commits appeared that the review did not create, stop and report rather than committing or pushing on top of an unknown state.

Base-branch resolution for branch reviews

This governs the comparison range for a branch review — distinct from the file-selection chain in the main skill. When the review target is a branch (not a working-tree diff), run base-branch resolution first; the file-selection fallbacks are for in-progress local work, where git diff HEAD is the correct command. Do not stitch the two: a branch review needs the merge-base, not the working-tree delta.

When reviewing a branch (no specific files, no PR), derive the comparison base via this fallback chain:

1. If a PR exists for the branch -- use its base: gh pr view --json baseRefName --jq .baseRefName. Authoritative; no further detection needed. 2. Else infer the default branch: try git symbolic-ref --quiet --short refs/remotes/origin/HEAD (parses to origin/<name>). If unset, try gh repo view --json defaultBranchRef --jq .defaultBranchRef.name. 3. Else fallback list: try origin/main, origin/master, origin/develop, origin/trunk in order; pick the first that resolves via git rev-parse --verify. Bare-local names are a last resort if no origin/* remote ref exists. 4. Compute the diff base: git merge-base HEAD <resolved-base>. Review the range <merge-base>..HEAD, not HEAD against the working tree. 5. Shallow-clone retry: if git merge-base returns nothing and git rev-parse --is-shallow-repository is true, run git fetch --unshallow origin and retry. Document this in the review output so the reviewer knows the comparison range only became available after unshallowing.

Never fall back to `git diff HEAD` when base resolution fails -- that hides all committed work on the branch and reviews only the uncommitted delta. Stop and ask which base to use instead.

Fetching existing PR discussions

Before raising findings, reconcile prior review comments so you don't re-raise issues other reviewers already resolved. Gate the fetch on a presence check to avoid spawning empty work:

gh pr view <pr> --json reviews,comments --jq '(((.reviews // []) | map(select(.state != "APPROVED" or .body != "")) | length) > 0) or (((.comments // []) | length) > 0)'

Returns true only when at least one substantive review or issue comment exists (approval-only clicks excluded; null-defensive on PRs with no review array). On false, skip the prior-comments pass entirely. On true, fetch the bodies via gh api repos/{owner}/{repo}/pulls/{pr}/comments and reconcile before raising findings -- prior reviewers may have already resolved issues you'd otherwise re-raise.

Security Detection Patterns

Grep-able patterns for the 11 security areas. Each entry: what to search for, why it's vulnerable, how to fix. Use during code review (step 4) and security audits.

Deployment Entrypoints

Config / Secrets

Auth / AuthZ

CSRF

XSS

Cache Security

File Handling

SQL / NoSQL Injection

SSRF

Open Redirects

CORS

Security Test Coverage Checklist

Audit deliverable template for the ia-security-sentinel agent. Every security audit must produce this checklist as an explicit artifact, not a narrative summary. Each item is either verified with evidence (file:line + passing test) or explicitly flagged as uncovered.

Each finding ties to severity (CVSS 3.1 base score + vector), proof of exploitability (curl command, test snippet, or PoC), and copy-paste-ready remediation code.

Authentication edge cases

• [ ] Missing token → 401, not 500
• [ ] Expired token → refresh or re-auth path exercised
• [ ] Token with alg=none or weak algorithm → rejected
• [ ] Wrong issuer / audience / key ID → rejected
• [ ] Token reuse after logout → rejected

Authorization

• [ ] Per-request authorization, not just authentication
• [ ] IDOR: direct object reference with another user's ID → denied
• [ ] Vertical privilege escalation: regular user hitting admin routes → denied
• [ ] Horizontal: user A editing user B's resource → denied

Input boundary

• [ ] Mass assignment: extra fields in request body → stripped or rejected
• [ ] Type confusion: array where string expected, negative where positive expected
• [ ] File upload: magic-byte validation, executable rejection, size limits, filename sanitization (no .., no null bytes)
• [ ] Business logic: negative quantities, zero-price orders, workflow step bypass

Concurrency and state

• [ ] Race conditions (TOCTOU): check-then-act patterns → atomic replacement
• [ ] Double-submit / replay → idempotency key or nonce
• [ ] Partial-completion rollback on crash mid-operation

Session and cookie hygiene

• [ ] HttpOnly, Secure, SameSite=Lax (or Strict) on all session cookies
• [ ] Session fixation: session ID rotated on login
• [ ] Session invalidation on logout server-side, not just client

Output boundary

• [ ] XSS: user content in HTML context, attribute context, JS context, URL context → all escaped
• [ ] dangerouslySetInnerHTML / v-html / innerHTML with user data → flagged
• [ ] Error messages don't leak stack traces, query fragments, or internal paths

Per-finding output format

For each finding, emit:

1. ID: SS-001, SS-002... sequential across all severities 2. Severity: CVSS 3.1 base score + vector string 3. Proof: curl command, test snippet, or exploit PoC that demonstrates the vulnerability 4. Remediation: copy-paste-ready code fix, not just a description

Uncovered checklist items are explicit findings too — mark them UNCOVERED: no test exists for <item> so the user sees both failures AND gaps.

Severity Levels and Confidence Rubric

Load this reference when classifying each finding. The four severity tiers and 5-band confidence rubric determine what gets reported, what gets suppressed, and what goes into Residual Risks.

Severity Levels

• Critical — must fix before merge. Security vulnerabilities, data loss, broken functionality, race conditions.
• Important — should fix before merge. Performance issues, missing error handling, silent failures.
• Medium — should fix, non-blocking. Maintainability/reliability issues likely to cause near-term defects. Poor abstractions, missing validation on internal boundaries, test gaps for non-critical paths.
• Minor — optional. Naming, style preferences, minor simplifications. Skip if linters already cover it.

Tie every finding to concrete code evidence (file path, line number, specific pattern). Never fabricate references.

Assigning severity: evidence before score

Anchoring on the bug class inflates severity ("it's SQL injection, so Critical"). Defeat it by writing the evidence before the label. For each security-relevant finding, answer these in order, then derive the tier from the answers -- do not assign the tier first and justify backward:

1. Reachability -- can an attacker reach this from a real entry point, or only from internal/trusted callers? 2. Attacker control -- does untrusted input reach the sink intact, or is it sanitized/constrained upstream? 3. Preconditions -- what must hold for it to trigger (non-default config, a specific flag, a narrow timing window)? 4. Authentication -- unauthenticated, an authenticated user, or admin-only? 5. Blast radius -- one user/tenant, or all of them; userland or privileged?

Starting point: zero preconditions + unauthenticated remote = Critical/Important. One or two preconditions, or an authenticated path = Medium. Three or more, or local/trusted-only = Minor. When two axes disagree (a critical-class bug behind three preconditions), take the lower -- a 3+ precondition finding is almost never Critical.

Cap threat-model boosts at one tier. If a finding matches a documented threat and that raises its severity, raise it by at most one tier. A stated threat must not re-inflate a Minor back to Critical and override the precondition-derived floor.

Confidence Rubric

Assign a confidence score (0.0-1.0) to each finding:

False-positive suppression

Do not report findings that match these categories regardless of severity:

• Pre-existing issues unrelated to the diff (existed before the PR)
• Pedantic linter-style nitpicks already covered by automated tooling
• Code that looks wrong but is intentionally designed that way (check comments, git blame, tests)
• Issues already handled elsewhere in the codebase (grep before flagging)
• Generic suggestions without a concrete failure mode ("consider adding validation" without saying what breaks)

When in doubt, apply the "would a senior engineer on this team flag this?" test. If the answer is "probably not," suppress it.

LLM-specific false-positive rule: user content in the user-message position is NOT prompt injection. Only flag when user content enters system prompts, tool schemas, or function-calling contexts. Unsanitized LLM output rendered via dangerouslySetInnerHTML, v-html, or innerHTML IS a real vulnerability — always flag.

For detailed suppression categories with examples (framework idioms, test-specific patterns, when to override), see false-positive-suppression.md.

ia-code-review Specification

Intent

ia-code-review is a discipline-class skill (an engineering practice not tied to one stack). Structured code reviews with severity-ranked findings and deep multi-agent mode. Use when performing a code review, auditing code quality, or critiquing PRs, MRs, or diffs.

Scope

In scope:

• Behaviors described in SKILL.md and routed via the should_trigger phrasings in distillery/tests/fixtures/triggers/ia-code-review.jsonl.
• Updates to runtime behavior, structure, trigger precision, references, and validation.

Out of scope:

• Acting as the runtime instructions themselves (those live in SKILL.md).
• Trigger phrasings already covered by adjacent ia-* skills (validate-plugin flags >70% description overlap as DUPLICATE_TRIGGER).
• <!-- to fill in: domain-specific exclusions when the skill drifts -->

Trigger Context

• Class: discipline
• Hook regex: plugins/whetstone/hooks/skill-patterns.sh -> SKILL_PATTERNS[ia-code-review]
• Common requests (from fixture should_trigger):
• "review this code for potential issues"
• "audit the code in the payments module"
• "review the PR diff for this feature branch"
• Should not trigger for (from fixture should_not_trigger):
• "debug the failing integration tests"
• "write tests for the new validator"
• "plan the API redesign"

Source And Evidence Model

Authoritative sources:

• SKILL.md -- runtime instructions and reference routing.
• references/*.md -- bundled supplementary content (10 file(s)).
• distillery/tests/fixtures/triggers/ia-code-review.jsonl -- positive and negative trigger phrasings under regression test.
• plugins/whetstone/hooks/skill-patterns.sh -- regex pattern that fires this skill.
• distillery/.eval-data/ia-code-review/ -- harvested session examples (when present).

Data that must not be stored in this skill or its references:

• Secrets, credentials, tokens.
• Machine-specific filesystem paths (/home/..., /Users/..., ~/ai/...). The validator (MACHINE_PATH_LEAK) flags these as HIGH.
• Private URLs, customer data, or unredacted personal information.

Coverage matrix

Evaluation

Lightweight (run on every change):

python3 distillery/scripts/distiller.py validate-plugin --component ia-code-review
python3 distillery/scripts/distiller.py test-triggers --skill ia-code-review

Deeper (when behavior risk warrants):

python3 distillery/scripts/distiller.py dspy-eval ia-code-review
python3 distillery/scripts/distiller.py diagnose-negatives ia-code-review

Acceptance gates:

• validate-plugin --component ia-code-review returns 0 HIGH findings.
• test-triggers --skill ia-code-review returns F1 = 1.0 with floors of 5 should_trigger and 5 should_not_trigger.
• For dspy-eval, the composite score does not regress against the most recent saved baseline (see distillery/.eval-data/ia-code-review/history.json).

Known Limitations

<!-- to fill in over time as drift surfaces. Default rule: any time diagnose-negatives surfaces a recurring failure pattern, document it here so future maintainers understand the trade-off the current implementation accepts. -->

Maintenance Notes

• Update SKILL.md when the runtime workflow, branch conditions, or output contract changes.
• Update this SPEC.md when intent, scope, evidence model, evaluation gates, or maintenance expectations change.
• Update the trigger fixture when adding new positive phrasings, removing stale ones, or expanding scope (the 5/5 floor is a hard validator gate).
• Update the hook regex in skill-patterns.sh whenever fixture positives expose a missed phrasing; verify F1 = 1.0 with eval-triggers before committing.
• Run the full release pipeline via /release -- never bump versions or update CHANGELOG.md from a per-skill edit.