# API Usage Review Reference

## Review Instructions

1. Identify external library/framework APIs in the code. Cross-reference with project dependency files to determine library versions in use. Filter out standard library and language built-ins. Focus on third-party dependencies. If no external library usage is found, report that and stop.
2. For each identified library with non-trivial usage: resolve the library using documentation MCP tools or WebSearch, query for the specific APIs being used, and note the documented signatures, parameter types, return types, deprecation status, and version requirements.
3. Check available skills for any relevant to the libraries or frameworks identified. Run matching skills to load domain-specific best practices as additional review context.

## What to Review

- **Wrong signatures** — incorrect parameter count, order, or types
- **Deprecated APIs** — using methods/classes/functions marked as deprecated
- **Superseded APIs** — older API still works but documentation recommends a newer alternative
- **Version mismatches** — using APIs not available in the project's pinned version
- **Missing required parameters** — omitting parameters that have no default value
- **Incorrect return type assumptions** — treating the return value as a different type than documented
- **Configuration errors** — invalid option names, wrong value types, removed configuration keys
- **Breaking change patterns** — usage patterns that match known breaking changes between versions
- **Best-practice violations** — patterns that contradict guidance from loaded skills

## Determination Criteria

Flag an issue only when ALL of these hold:

1. The documentation or loaded skill clearly contradicts the usage (not ambiguous or underdocumented)
2. The issue is discrete and actionable
3. The documented behavior applies to the library version in the project's dependency file
4. The issue would cause incorrect behavior, a runtime error, a deprecation warning, or uses an API the documentation explicitly recommends superseding

## Priority Levels

- **P0** — Will cause a runtime error or crash (wrong signature, removed API)
- **P1** — Will cause incorrect behavior silently (wrong parameter type coerced, deprecated API with changed semantics)
- **P2** — Deprecated API that still works but will be removed in a future version
- **P3** — Suboptimal usage where documentation recommends a better alternative

## What to Ignore

- Standard library and language built-in usage
- APIs where documentation is ambiguous or unavailable
- Internal project APIs (only check third-party dependencies)
- Style preferences not grounded in documentation

**Extra metadata:** `**Library:** <name> <version>` and `**Docs:** <brief quote or paraphrase>`

**Verdict label:** `API Usage: <correct | issues found>`

# Consistency Review Reference

## Review Instructions

Identify related files in the project (shared interfaces, similar modules, files importing the same utilities, structurally similar code without shared imports) and read those to detect cross-file patterns. Cross-file issues are the primary focus.

## What to Review

- **Cross-file duplication** — Nearly identical logic in multiple files, copy-pasted functions with slight variation across components, repeated boilerplate that could be extracted into a shared utility, component, or module
- **Architectural inconsistency** — Mixed error handling patterns across modules, inconsistent naming conventions, different approaches to the same concern in different components
- **Abstraction opportunities** — Repeated structural patterns across files that suggest a missing shared abstraction, multiple components manually implementing the same protocol or interface pattern
- **Convention drift** — Divergent logging patterns across modules, inconsistent use of project-defined constants, types, or enums, different approaches to validation, serialization, or resource management

## Determination Criteria

Flag an issue only when ALL of these hold:

1. It spans multiple files or represents an inconsistency across components
2. The issue is discrete and actionable
3. Fixing the inconsistency or extracting the duplication would provide clear maintenance or readability benefit
4. The pattern has at least 2 concrete instances

## Priority Levels

- **P0** — Harmful inconsistency actively causing bugs or maintenance traps (e.g., error handling inconsistency where some paths swallow errors)
- **P1** — Significant duplication or architectural drift that makes changes error-prone
- **P2** — Moderate duplication or convention drift with clear extraction opportunity
- **P3** — Minor inconsistency or duplication with low maintenance impact

## What to Ignore

- Style-only differences unless they indicate a genuine pattern inconsistency
- Intentional variation documented in project guidelines

**Extra metadata:** `**File:** <path 1>, <path 2> (and others)` and `**Category:** <duplication | inconsistency | abstraction | convention-drift>`

**Verdict label:** `Consistency: <consistent | issues found>`

# Correctness Review Reference

## What to Review

Bugs, logic errors, and correctness problems: incorrect control flow, off-by-one errors, null/undefined access, type mismatches, broken error handling, race conditions, resource leaks, and state lifecycle bugs.

Also audit removed behavior: when the change deletes a line or block, identify the invariant, guard, validation, or cleanup it enforced and confirm the change re-establishes it elsewhere or proves it is no longer needed.

Also trace call sites: for each function, method, or exported symbol the change modifies, search for its callers and check whether the change breaks any of them — a new precondition, a changed return shape or nullability, a newly thrown error, a different ordering or timing dependency. Check the other direction too: a parallel change elsewhere in the same changeset can make an existing call unsafe.

## Determination Criteria

Flag an issue only when ALL of these hold:

1. It meaningfully impacts the accuracy, performance, security, or maintainability of the code
2. The bug is discrete and actionable (not a general codebase issue or combination of multiple issues)
3. Fixing it does not demand rigor beyond what exists in the rest of the codebase
4. The author would likely fix the issue if aware of it
5. The bug does not rely on unstated assumptions about the codebase or author's intent
6. Speculation is insufficient — identify the parts of the code that are provably affected
7. The issue is clearly not an intentional change by the original author

## Priority Levels

- **P0** — Drop everything. Blocking release or operations. Only for universal issues that do not depend on assumptions about inputs
- **P1** — Urgent. Should be addressed in the next cycle
- **P2** — Normal. To be fixed eventually
- **P3** — Low. Nice to have

## What to Ignore

- Trivial style unless it obscures meaning or violates documented standards

**Verdict label:** `Correctness: <correct | incorrect>`

# Coverage Review Reference

## Review Instructions

1. Skip non-testable code (config, documentation, CI files, SKILL.md files, markdown).
2. Search for existing test files covering the target code.
3. Identify the project's test framework and conventions by reading existing test files.

### Verifying Pin Claims

Before asserting that a behavior is unpinned, or that a new test pins one, verify it by mutation: in an isolated `git worktree` created under `$TMPDIR`, invert or remove the line at issue, run the suite in its own process group under a timeout enforced from outside the runner, and check whether a test fails. Refer to the worktree by absolute path in every command and join chained steps with `&&`, so a failed step cannot leave the rest running in the shared checkout. Run teardown and verification as their own commands. Leave HEAD where it is: read other refs with `git show <ref>:<path>` rather than `git checkout` or `git switch`. A test that passes against a behavior-changing mutation does not pin the behavior. Before discarding the worktree, stop the process group of every suite run you started, since stopping a runner can leave the processes it spawned alive. After discarding the worktree, verify that `git worktree list` no longer shows it, that `git status --short` is clean, that HEAD is still on the branch it started on, and that the shared tree's dependency directory still resolves (a destroyed install leaves `git status` clean, since it is gitignored). Also confirm that no process from those groups, and none whose command line names the worktree path, is still running, and report by PID any process you could not stop. When you cannot list processes, report that check as unrun and name those process groups and the worktree path. Report damage you cannot repair, with the exact repair command, in place of findings. Cite the mutation and its result as evidence in the finding's paragraph; the `**Failure scenario:**` line still reads trigger → consequence: the unguarded path and what shipping it lets through.

This covers any claim that a specific behavior is or is not guarded, including "tests exist but miss this path" — target the unguarded path. Settle "this module has no tests at all" by inspection.

A surviving mutation proves a gap only when the mutated code behaves differently from the original for some reachable input. Name that input in the finding. When none can be named, the finding is void.

Before running the suite, confirm the mutation landed: `git diff` in the worktree shows the intended change to each mutated file. An edit whose match pattern missed leaves the file untouched. Count the behavior as pinned only when the suite reports a failing test, not when the mutation command or a step chained before the suite exited nonzero.

A failing test proves the behavior is pinned only when the mutation landed in the branch under review. Name that branch and the original behavior the mutation reproduces before running it, then re-read the mutated lines: a mutation placed a statement away fails for a reason unrelated to the gap, and voiding the finding on that evidence hides it with nothing to show the check went wrong.

Skip the mutation when a fresh checkout cannot run the suite cheaply: it needs an install or build step, there is no runnable test command, or the suite depends on state outside the tree such as fixed ports, shared databases, caches, untracked local configuration, or external services. Needing an install is itself a skip trigger here, whether or not that install would succeed. Skip it as well when the line at issue bounds a loop, a retry, or a wait and the suite cannot run under a timeout enforced from outside the test process — the mutation hangs instead of failing, and a per-test timeout can depend on the test cooperating. Run the suite only against dependencies inside the worktree. Reaching the shared tree's install by any route, whether a link, a copy, a mount, or an environment variable redirecting resolution, is not a substitute, since removing a worktree deletes through symlinks and a redirected suite writes into the shared install. When skipping, say in the finding's paragraph that the claim rests on inspection alone.

### Reviewing a Boundary Served by a Double

When a test double is the only executor of an external boundary, a green tier is evidence about the double and not about production. Enumerate in the finding what the double cannot prove: which behaviors of the real dependency it fails to reproduce, and which passing tests are vacuous as a result. A double more permissive, more informative, or more forgiving than the real dependency is itself the defect, so report it against the double. When the changed code is exercised only through the double, the finding concerns that code's coverage in diff mode, naming the double as the reason the coverage is illusory.

## What to Review

- **No test coverage** — functions or modules with no corresponding tests
- **Missing edge cases** — tests exist but miss critical paths (error handling, boundary conditions, empty inputs, concurrent access)
- **Test efficacy** — tests that cannot fail when the behavior they guard breaks (the assertion reads a surface the code under test does not write, or a mechanism other than the one under test produces the same observable)
- **Double fidelity** — a test double more permissive, more informative, or more forgiving than the dependency it stands in for, so tests passing against it say nothing about production
- **Risk-level mismatch** — high-risk code (auth, data handling, financial logic) with only basic happy-path tests
- **Convention gaps** — tests not following the project's established testing patterns

## Determination Criteria

Flag an issue only when ALL of these hold:

1. The code performs meaningful logic worth testing (not pure configuration, boilerplate, or generated code)
2. The gap is discrete and actionable (a specific function or module, not "needs more tests generally")
3. The missing coverage creates real risk proportional to the code's criticality

## Priority Levels

- **P0** — Critical code with no tests (auth, data mutation, payment processing)
- **P1** — Important code with no tests or high-risk code with only happy-path tests
- **P2** — Code with tests but missing significant edge cases
- **P3** — Minor coverage gaps or convention mismatches

## What to Ignore

- Non-testable code (config, documentation, CI files, SKILL.md files, markdown)
- Generated code or trivial getters/setters with no logic

**Verdict label:** `Test Coverage: <adequate | gaps found>`

# Security Review Reference

## Review Instructions

Check if `.turbo/threat-model.md` exists at the repository root. If it does, read sections 2 (Trust Boundaries and Assumptions) and 3 (Attack Surface, Mitigations and Attacker Stories) to understand assets at risk, identified attack surfaces with existing mitigations, and attacker stories. Treat the entry points, sinks, and hot files listed by surfaces that intersect the reviewed scope as read targets, and use the attacker stories to prioritize findings. Treat the threat model as absent when a surface has no cited path that still resolves, ignoring any `:line` suffix. If no threat model exists, proceed without it.

### Review Mindset

Do not treat the existence of a check, sanitizer, or authorization guard as proof of safety. When a defense exists, reason about whether it actually constrains the value or state as intended across the full transformation and execution chain. A regex that validates a URL before decoding does not constrain the decoded URL. A permission check in one handler does not protect a second handler that skips it. Start from what the code is trying to guarantee, then look for ways that guarantee can fail.

Hold dismissals to the same standard. Refute a finding only with a mitigation you located and read. An expectation that the framework escapes the value, that the caller already validated it, or that the type constrains it stays an assumption until you have read the code that enforces it, and an assumption leaves the finding standing.

## What to Review

- **Injection** — SQL, command, template, LDAP, XPath, header injection via unsanitized input
- **Authentication and authorization** — missing or weakened auth checks, hardcoded credentials, insecure token handling, privilege escalation
- **Cryptographic misuse** — weak algorithms, hardcoded keys/IVs, nonce reuse, missing authentication (e.g., AES-CBC without HMAC)
- **Data exposure** — secrets in logs, error messages leaking internals, sensitive data in URLs or query params
- **Input validation** — missing or insufficient validation at trust boundaries, path traversal, open redirects
- **Insecure defaults** — debug mode, permissive CORS, disabled TLS verification, fail-open behavior
- **Deserialization** — untrusted data deserialization without type constraints
- **Dependency risks** — new dependencies with known CVEs, removed security-related dependencies
- **Race conditions** — TOCTOU bugs, unprotected shared state in security-critical paths
- **Transformation chain bypasses** — validation or sanitization that runs before encoding, decoding, normalization, or type coercion, allowing the constrained value to diverge after transformation
- **State and invariant violations** — workflow bypasses where security-critical operations proceed without required preconditions, missing state guards on multi-step processes
- **Resource management** — unbounded allocations from attacker-controlled input, missing rate limiting on sensitive endpoints

## Determination Criteria

Flag an issue only when ALL of these hold:

1. It is a concrete security weakness, not a theoretical concern or defense-in-depth suggestion
2. The vulnerability is discrete and actionable (not a general architecture issue)
3. The vulnerable code path is reachable with attacker-controlled input or attacker-influenced state
4. The author would likely fix the issue if aware of the security implications
5. The issue is demonstrable through a specific attack scenario, not speculation

## Priority Levels

- **P0** — Exploitable by remote unauthenticated attacker with immediate impact (RCE, auth bypass, credential theft)
- **P1** — Exploitable with preconditions (authenticated attacker, specific configuration, race condition)
- **P2** — Security weakness that increases attack surface or weakens defense-in-depth
- **P3** — Minor security hygiene issue with minimal direct impact

## What to Ignore

- Style and naming unless it creates a security-relevant ambiguity
- Defense-in-depth suggestions when the primary defense demonstrably holds (not just exists)
- Vulnerabilities in test code that cannot be reached in production

**Extra metadata:** `**Category:** <vulnerability class>` and `**Sink:** <sink line, verbatim> in <enclosing function or method>`

The sink is the line where attacker-influenced data reaches the dangerous operation. Quote it inside a code span so source punctuation survives rendering. Omit the `**Sink:**` field for findings with no data flow behind them, such as a weak algorithm choice, an insecure default, a hardcoded credential, or a dependency CVE.

**Verdict label:** `Security: <secure | concerns found>`

# Simplicity Review Reference

## Review Instructions

Focus on single-file simplicity: scope, reuse, quality, efficiency, and clarity issues that surface within an individual file or a small set of changes.

For scope, trace the callers of the code you propose deleting and confirm nothing depends on the behavior being removed. Judge "unrequested" against what the reviewed change set out to do; anything it was explicitly asked for is in scope by definition. Ground the finding in what the code does rather than in how much of it there is.

For reuse, identify analogous utilities, helpers, or shared modules elsewhere in the project before suggesting a rewrite. Common locations are utility directories, shared modules, and files adjacent to the changed ones.

## What to Review

### Scope

- **Unrequested machinery** — an abstraction with one implementation, a configuration point with one caller, a factory for one product, a wrapper that only delegates, scaffolding for an anticipated requirement; delete it rather than simplify it
- **Unreachable defensive code** — a branch, guard, retry, or fallback for a state the surrounding code's own constraints rule out; when the callers cannot produce the input, the handling for it is dead on arrival
- **Reinvented standard library or platform feature** — hand-rolled logic the language's standard library or the target platform already ships, or a new dependency for what an already-installed one covers; name the replacement

### Reuse

- **Duplicate functionality** — new functions that duplicate existing utilities; suggest the existing function to use instead
- **Inline logic that could use an existing utility** — hand-rolled string manipulation, manual path handling, custom environment checks, and similar patterns where a utility already exists

### Single-File Quality

- **Redundant state** — state that duplicates existing state, cached values that could be derived, reactive subscriptions that could be direct calls
- **Parameter sprawl** — new parameters added to a function instead of generalizing or restructuring existing ones
- **Copy-paste with slight variation** — near-duplicate code blocks within the same file that should be unified with a shared abstraction
- **Leaky abstractions** — exposing internal details that should be encapsulated, or breaking existing abstraction boundaries
- **Stringly-typed code** — raw strings used where constants, enums, or dedicated types already exist in the codebase
- **Unnecessary wrapper nesting** — container elements or wrapper layers that add no structural or layout value

### Efficiency

- **Unnecessary work** — redundant computations, repeated file reads, duplicate network/API calls, N+1 patterns
- **Algorithmic complexity** — nested iterations, repeated linear searches replaceable by sets/maps, missing early exits
- **Missed concurrency** — independent operations run sequentially when they could run in parallel
- **Hot-path bloat** — new blocking work added to startup or per-request hot paths
- **Unnecessary existence checks** — pre-checking file/resource existence before operating (TOCTOU anti-pattern); operate directly and handle the error
- **Memory** — unbounded data structures, missing cleanup, resource leaks
- **Overly broad operations** — reading entire files when only a portion is needed, loading all items when filtering for one

### Clarity and Standards

- **Project standards** — coding conventions not followed (import sorting, naming conventions, component patterns, error handling patterns, module style). Beyond the auto-loaded instruction files, walk each directory that is an ancestor of a changed file, from the project root down, and read its `CLAUDE.md` and any file those instructions import — a directory's file governs only the files at or below it. Flag a violation only when you can quote the exact rule and cite what breaks it: the offending line, or the location where a required element is missing. Name the file the rule came from
- **Unnecessary complexity** — deep nesting, unclear variable or function names, nested conditionals 3+ levels deep (ternary chains like `a ? x : b ? y : ...`, nested if/else, or nested switch — flatten with early returns, guard clauses, a lookup table, or an if/else-if cascade), redundant boolean comparisons (e.g., `x == true` instead of `x`)
- **Unclear code** — overly compact one-liners that sacrifice readability; explicit code is better than clever code
- **Over-simplification** — too many concerns combined into a single function or component, helpful abstractions removed that were aiding code organization, "fewer lines" prioritized over readability
- **Dead weight** — code no longer reached by any path, and variables, imports, or parameters the change orphaned

### Altitude and Fix-Depth

- **Special case on shared infrastructure** — a narrow branch, flag, or conditional added to a shared mechanism to handle one case, where generalizing the mechanism removes the need for the special case; name the generalization
- **Shallow fix at the symptom** — a change applied at one call site that the same shape will require again at the next similar site; prefer addressing the shared root
- **Wrong layer** — logic placed in a caller, wrapper, or leaf when it belongs in the shared layer all paths flow through, or pushed into shared infrastructure when it is specific to one caller

### Comments and Documentation

- **Asserts a contract the code does not enforce** — states what is handled, excluded, guaranteed, or left untouched, where the code beneath it does something else; verify each such claim against the code rather than reading it as intent, then propose correcting the comment or flag the missing enforcement when the stated contract is the desired one
- **Positional reference that no longer resolves** — carries an ordering or directional word such as "first", "above", or "below" whose target the code has since moved; verify the word still picks out what it names, then propose correcting it to name what it refers to directly, or deleting it when nothing else in the comment survives
- **Restates code, signature, or name** — paraphrases the immediately-following statement or multi-statement block; doc blocks whose text translates a declaration's name; doc blocks above a declaration whose prose elaborates the name and signature without adding rationale; parameter/return descriptions that only echo names and types. Non-obvious constraints (size, units, ranges, preconditions) stay; drop the wrapping parameter/return/error enumeration when no entries survive trimming. Where an instruction file or the documentation tooling's configuration requires that declaration to carry documentation, keep what the requirement covers and flag only entries that describe the code wrongly
- **History or change narration** — references to PRs, tickets, prior behavior, recent changes, "fixed by"/"previously did X"/"no longer Y" framing, or session-narrative voice ("turns out", "discovered", "we found that"); state the current invariant only — past behavior belongs in git history, and session-derived lessons about tooling belong in auto memory or project instructions. Change narration also appears in invariant form: a sentence that reads as a rule but only carries meaning as a contrast with the code's prior behavior, and that you would not write if the code had been greenfield from day one
- **Cross-references that decay** — names the caller ("used by X", "called from Y"), or task/flow/feature-flag context the code was added for ("added for the Y flow", "for the rollout"); delete — caller relationships belong in the call graph, feature context in the PR description
- **Framework or stdlib explainers** — describes what a well-known language keyword or library construct does
- **Low-value section banners** — banners that don't section anything, or that restate what an access modifier or naming convention already conveys
- **Overgrown rationale** — a comment that captures real WHY but in more lines or concerns than the rationale requires; tighten to one sentence per concern, split bundled concerns to their decision points, or lift shared rationale to a design doc or commit message
- **Compensates for unclear code** — a comment that exists because the code is hard to read; flag the underlying code as a refactor opportunity (rename, extract, restructure) rather than tightening the comment
- **Markdown status-update voice** — for markdown changes in the diff, prose framed as recent updates or transitions; rewrite as timeless current-state prose. It also appears in invariant form: a sentence that reads as a rule but only carries meaning as a contrast with the design it replaced, and that you would not write if the project had always worked this way

## Determination Criteria

Flag an issue only when ALL of these hold:

1. The issue meaningfully impacts readability, maintainability, correctness, or performance
2. The issue is discrete and actionable
3. A specific fix is obvious (reuse existing utility X, remove redundant state Y, collapse nested wrapper Z)
4. The fix does not demand rigor beyond what exists in the rest of the codebase
5. The author would likely accept the fix if aware of it, OR the finding challenges a deliberate choice with evidence of a simpler alternative

## Priority Levels

- **P0** — Severe efficiency problem on a hot path, or a reuse miss that duplicates a load-bearing utility
- **P1** — Clear duplication, leaky abstraction, unrequested machinery, or efficiency issue with meaningful impact
- **P2** — Moderate clarity, efficiency, or reuse opportunity
- **P3** — Minor style or readability nit

## What to Ignore

- Style-only differences that do not obscure meaning or violate documented standards
- Micro-optimizations with no measurable impact
- Input validation at trust boundaries, error handling that prevents data loss, security controls, and accessibility affordances; these outrank the Scope bullets when both apply to the same code
- Comments that capture a load-bearing constraint the code itself cannot express — a hidden constraint or invariant, a workaround for a specific bug (ideally with a reference), a non-obvious performance characteristic, a pointer to a spec or RFC section, or behavior that would surprise a future reader. Greenfield test: would you write this comment if the code had been greenfield from day one?

**Extra metadata:** `**Category:** <scope | reuse | quality | efficiency | clarity | altitude | documentation>`

**Verdict label:** `Simplicity: <clean | issues found>`