skill-review

Instruction file imported from jketreno/clare (.cursor/rules/skill-review.mdc). Copyright stays with the author.

name: review description: "Code review for correctness, KISS/DRY/YAGNI/SOLID, security, language BKMs, tests, and docs" mode: agent

Review Diff vs. Base Ref

Scope and Baseline

Review all changes on the current branch against an appropriate base ref — as if performing a thorough pull-request code review. Do not guess at correctness; read the actual diff and the relevant file context.

Step 1 — Determine the Base Ref

Resolve the base ref in priority order. Use the first one that succeeds:

1. User-specified — if the user says "review vs main", "against origin/staging", etc., use that ref.
2. Upstream tracking branch — git rev-parse --verify "upstream/$(git symbolic-ref --short HEAD 2>/dev/null)" 2>/dev/null
3. Origin tracking branch (same name) — git rev-parse --verify "origin/$(git symbolic-ref --short HEAD 2>/dev/null)" 2>/dev/null
4. origin/main — git rev-parse --verify origin/main 2>/dev/null
5. origin/master — git rev-parse --verify origin/master 2>/dev/null
6. Local main — git rev-parse --verify main 2>/dev/null
7. Ask — if none resolve, say: "I couldn't determine a base branch. What should I diff against?"

Step 2 — Gather the Diff

# Unified diff from merge base (PR semantics — use triple-dot)
git diff <BASE_REF>...HEAD

# File summary (what changed at a glance)
git diff --stat <BASE_REF>...HEAD

# Untracked new files not yet committed
git status --short

Read the complete diff before beginning the review. Do not review hunks in isolation.

Step 3 — Load File Context

For every file that appears in the diff, read the full file — not just the changed hunks. This is required to:

• Detect misuse of a new or changed API by its callers
• Verify that error handling is consistent with the rest of the file
• Identify invariants that the changed code may break
• Confirm that imports, exports, and type signatures are consistent

Review Checklist

Apply every category below to every changed file. Do not skip a category because the change looks small — subtle bugs hide in small diffs.

1. Correctness & Logic

• Off-by-one — loop bounds, slice/array indices, fence-post counting
• Null / undefined / nil dereference — accessing a field or calling a method before verifying the value is non-null
• Integer overflow / underflow — unguarded arithmetic on values that could exceed type bounds
• Incorrect boolean logic — misplaced negation, wrong operator precedence, short-circuit evaluation that skips side effects unintentionally
• Swallowed errors — errors caught and dropped, converted to empty/zero returns, or logged but not propagated when propagation is required
• Wrong error type returned — creating a different error subtype or error code than the caller expects
• API contract violation — calling a function with arguments in the wrong order, the wrong units (ms vs s, bytes vs kB), or the wrong type
• Algorithm correctness — does the implementation match the stated intent? Trace through at least one boundary case mentally.
• Missing return / fallthrough — missing return in a branch; switch/match case that falls through when it should not
• Reference vs. value equality — == vs .equals() (Java), is vs == (Python), === vs == (JS), pointer vs. struct comparison (Go/C)
• Unintended mutation — function modifies the caller's data structure without that being declared in its contract
• Incorrect assumptions about external state — assuming a file always exists, a DB always responds within a timeout, a network resource is always reachable

2. Security (OWASP Top 10 + Common Patterns)

• Injection — SQL, shell command, LDAP, XPath, template injection, or path traversal via unsanitized user input; ensure parameterized queries or allow-list validation
• Broken access control / IDOR — operating on a resource by user-supplied ID without verifying the caller owns or is authorized to access that ID
• Hardcoded credentials — API keys, passwords, tokens, private keys, or symmetric secrets committed in source or in default config
• Sensitive data exposure — PII or secrets logged, returned in error messages, stored in plaintext, or included in URLs (which end up in logs)
• Broken cryptography — MD5/SHA-1 for integrity, ECB mode, hardcoded IVs, key reuse, non-constant-time comparison of secrets or MACs
• XSS — user-supplied content rendered into HTML or JavaScript without escaping; React dangerouslySetInnerHTML, server-side templates with unescaped interpolation
• CSRF — state-mutating endpoints (POST, PUT, PATCH, DELETE) without CSRF token validation for browser-facing APIs
• SSRF — server fetching a URL supplied by the user without an allow-list of permitted hosts/schemes
• XXE — XML parser with external entity processing enabled when parsing untrusted input
• Security misconfiguration — debug modes left on, verbose stack traces in production responses, permissive CORS (*), missing security headers (Content-Security-Policy, X-Frame-Options)
• Timing attacks — secrets, tokens, or HMACs compared with == (not constant-time); use crypto.timingSafeEqual (Node), hmac.compare_digest (Python), subtle.ConstantTimeCompare (Go)
• New dependencies — for each newly added package: check for known CVEs, review license compatibility, and consider whether a lighter or in-tree alternative exists
• Dependency confusion / typosquatting — verify that newly added packages use the correct registry scope and exact expected package name; check for private package names that could be shadowed by public registries
• Overly broad permissions / scopes — IAM roles, OAuth scopes, file permissions, or container capabilities granted wider than the code actually requires (principle of least privilege)
• Secrets in structured logs / telemetry — fields like request_body, headers, or user_data serialized into logs or tracing spans without redaction of sensitive fields
• Unsafe deserialization — deserializing untrusted input (pickle, Java ObjectInputStream, YAML load, eval-based JSON parsing) without safe loader or schema validation

3. Concurrency & Race Conditions

• Unsynchronized shared mutable state — multiple goroutines, threads, or async handlers reading and writing the same variable without a mutex, atomic, or channel
• TOCTOU (time-of-check / time-of-use) — checking a condition (file exists, record exists, balance is sufficient) and then acting on it in a separate step, without holding a lock or using a transaction
• Deadlock potential — locks acquired in inconsistent order across code paths; locks held across I/O or external calls; nested locks that are never released on error
• Missing atomic operations — compound read-modify-write (counter++, map[key] = map[key] + 1) without atomics or a mutex
• Missing await / unhandled promise rejections — async functions called without await; .catch() missing on a Promise chain; top-level await without try/catch
• Event-loop blocking — synchronous heavy computation, fs.readFileSync, or blocking network calls on the main thread in Node.js or a browser context
• Goroutine leaks — goroutine started that has no guaranteed exit path when the parent context is cancelled or the channel it reads is never closed (Go)
• Missing database transaction — multi-step operations (read-then-write, debit-then-credit) that must be atomic executed without a transaction, leaving the DB in an inconsistent state on partial failure
• Stale closure — a goroutine or callback closes over a loop variable by reference; the variable has changed by the time the closure executes (Go loop variable capture, JS var in for)

4. Design Principles (KISS / DRY / YAGNI / SOLID)

KISS — Keep It Simple

• Over-engineered solution — the implementation is more complex than the problem warrants; a simpler approach (fewer layers, fewer abstractions, less indirection) would achieve the same result
• Unnecessary abstraction layers — wrapper classes, factory methods, strategy patterns, or middleware introduced when the code has only one concrete implementation and no stated need for extensibility
• Clever code over clear code — one-liners, ternary chains, or bitwise tricks that sacrifice readability for brevity without a measurable performance justification
• Excessive cyclomatic complexity — deeply nested conditionals or loops; consider early returns, guard clauses, or decomposition into smaller functions

DRY — Don't Repeat Yourself

• Duplicated logic — identical or near-identical blocks in two or more locations that should be extracted into a shared function or module
• Duplicated knowledge — the same business rule, constant, or configuration value defined in multiple places where a single source of truth should be referenced
• Copy-paste with minor variations — blocks that differ only in variable names or literals, indicating a missing parameterized abstraction

YAGNI — You Ain't Gonna Need It

• Speculative generality — interfaces, parameters, extension points, or configuration options added "in case we need them later" with no current consumer
• Unused parameters or options — function signatures that accept arguments no caller currently provides, or configuration knobs that are always set to their default
• Premature optimization — caching, pooling, lazy initialization, or custom data structures introduced without evidence of a performance problem in the current workload
• Gold-plating — features, error handling branches, or edge-case coverage beyond what the requirements or ticket specify

SOLID Principles

• Single Responsibility (SRP) — a function, module, or class doing multiple unrelated things; if you need two different reasons to change it, split it
• Open/Closed (OCP) — extending behavior requires modifying existing code instead of adding new code; look for growing switch/match/if-else chains that should be polymorphism, a registry, or a strategy map
• Liskov Substitution (LSP) — a subtype, interface implementation, or duck-typed replacement that violates the contract of the type it substitutes (throws unexpected errors, ignores required parameters, changes return semantics)
• Interface Segregation (ISP) — a consumer forced to depend on methods or fields it does not use; an interface or struct with members relevant to only some of its implementors should be split
• Dependency Inversion (DIP) — high-level logic directly instantiating or importing low-level infrastructure (database clients, HTTP clients, file system) instead of depending on an abstraction passed via constructor, parameter, or module boundary

Code Hygiene

• Magic numbers / strings — hardcoded literals with no named constant or comment explaining their meaning or origin
• Dead code — unreachable branches, variables assigned but never read, functions defined but never called, imports never used
• Abstraction leakage — internal implementation details (concrete types, storage mechanisms, internal error codes) exposed through a public interface
• Naming inconsistency — mixed conventions (camelCase vs snake_case), misleading names (a function named get* that has side effects), inconsistent verb tense
• Inappropriate global state — module-level mutable singletons that make parallelism or isolated testing difficult
• Tell, Don't Ask (TDA) violation — code that queries an object's internal state, makes a decision, then calls back into the object; the decision logic should live inside the object

5. Language-Specific Best Practices

Apply the checks for the language(s) present in the diff.

JavaScript / TypeScript

• == instead of === (type coercion bugs)
• any on new interfaces; unchecked type assertions (as Foo without a preceding instanceof or type guard)
• Floating-point equality === instead of an epsilon / Number.EPSILON check
• var declarations (prefer let/const for block scoping)
• Multi-level callback nesting (prefer async/await)
• Prototype pollution risk — merging untrusted objects via Object.assign({}, userInput) or spread
• Promise chains missing .catch() or a try/catch around top-level await
• Non-null assertions (!) on values that could genuinely be null or undefined at runtime
• Barrel file re-exports that defeat tree-shaking or create circular dependency chains
• Missing AbortController / signal propagation on fetch or long-lived async operations that should be cancellable

Python

• Mutable default arguments — def f(items=[]) or def f(cfg={}):
• Bare except: — catches SystemExit and KeyboardInterrupt; use except Exception: or a specific type
• is for value comparison — correct only for singletons (None, True, False); use == for all other values
• Float equality with == — use math.isclose()
• Missing if __name__ == "__main__": guard on executable scripts
• Unrestricted eval() or exec() on user input
• Missing type hints on public function signatures (for projects using mypy/pyright)
• os.path string manipulation instead of pathlib.Path for filesystem operations (Python 3.4+)

Go

• Ignored error returns — _ , err discarded or _ used where the error must be handled
• Goroutine started without a clear exit path tied to a context.Context cancellation
• Functions performing I/O that accept no context.Context argument
• Inconsistent value vs. pointer receivers on the same type's method set
• init() functions with side effects that make packages hard to test in isolation
• Exported types/functions that should be unexported — overly broad public API surface within internal packages
• sync.Mutex embedded in a struct that is copied by value, silently breaking the mutex

Rust

• unwrap() or expect() in non-test, non-prototype code without a // Safety: or // Invariant: comment justifying why the panic cannot occur
• unsafe blocks without a // SAFETY: comment explaining which invariants hold
• Unchecked integer arithmetic in non-performance-critical paths (prefer checked_*, saturating_*, or wrapping_*)
• clone() where a borrow would suffice, especially in hot paths
• .to_string() / .to_owned() in function signatures where &str / AsRef<str> would avoid allocation

General / Any Language

• Allocations inside tight inner loops that could be hoisted or pooled
• N+1 query patterns — fetching related records one-by-one inside a loop instead of a batch fetch or JOIN
• Blocking I/O called from within an async or reactive context
• Logging that could produce unbounded output under load (e.g. logging every request body without size limits)
• Unbounded collections — maps, lists, or caches that grow without eviction, leading to memory exhaustion under sustained load
• Missing timeouts — HTTP clients, database connections, or external calls with no timeout or deadline, risking thread/goroutine starvation

6. Tests & Coverage

• Untested code paths — new branches, functions, or error cases with no corresponding test
• Stale tests — behavior changed but existing tests still pass because they test the old behavior; tests are now confirmation tests rather than constraint tests
• Weak assertions — toBeTruthy(), assert(result), or checking implementation details instead of observable, user-facing behavior
• Missing edge cases — no tests for: null/undefined/nil inputs, empty collections, zero/negative numbers, maximum-size inputs, concurrent callers, or injected errors
• Test isolation violations — tests that share mutable module-level state or depend on execution order within the suite
• Flaky patterns — sleep() for timing synchronization, real network calls in unit tests without mocking, non-deterministic data ordering, reliance on wall-clock time
• Confirmation tests (CLARE [A]) — tests that describe "what the code currently does" rather than "what must always be true regardless of implementation"; these should be constraint tests
• Uninformative test names — "works correctly", "handles the case" vs. "never creates duplicate users for the same email" or "returns 403 when caller does not own the resource"
• Missing contract / schema-lock tests — new public API surface with no test that will fail if the shape changes unintentionally

7. Documentation & Contracts

• What-comments — comments that describe what the code does when it is readable from the code itself; replace with why-comments explaining intent, constraints, or non-obvious tradeoffs
• Stale comments — comments that contradict or no longer match the current implementation
• Public API changes without doc updates — new or removed parameters, changed return types, changed error conditions, or changed side effects without updating JSDoc / docstrings / OpenAPI / README
• Missing rationale for non-obvious decisions — performance tradeoffs, known third-party bugs worked around, intentional relaxation of a constraint
• Breaking changes without changelog — removal or rename of a public symbol, change of wire format, or change to a persisted data schema without a CHANGELOG entry or migration guide
• Broken examples — code examples in doc comments or README that no longer compile, run, or reflect the current API
• README drift — setup or usage steps that diverge from the actual process after this change

Output Format

Structure your response exactly as follows. Omit a section only if it truly has no content; always include the header.

## Code Review — <branch-name> vs <base-ref>
(<N> files changed, +<additions> / -<deletions>)

### Issues

**[SEVERITY] `path/to/file.ext:42`** — Short descriptive title
> Explanation of the problem and why it matters.
> Suggested fix or the approach to take.

... (one block per issue) ...

_No issues found._   ← use only when Issues is genuinely empty

### Observations

- `path/to/file.ext:17` — Context-only note (no action required).

_None._   ← use only when there are no observations

### Execution Plan

> Only include this section when Issues is non-empty.

Ordered list of fixes. Mark items that can be done in parallel.

1. Fix [issue] in `path/file.ext:42`
2. Fix [issue] in `other.ext:88` — can parallelize with item 4
3. Fix [issue] in `third.ext:12`
4. Fix [issue] in `fourth.ext:55` — can parallelize with item 2

Severity scale:

Constraints

• Never fabricate line numbers. Every file:line reference must come from the actual diff or file contents you read.
• Read the full file for every changed file before reporting issues — do not rely on diff context alone.
• Large diffs (>500 lines changed): Summarize by directory or logical concern first, then drill into individual issues.
• Prioritize by severity. Report CRITICAL and HIGH issues before MEDIUM and LOW.
• Be specific. A vague note ("this could be null") is not actionable. Include the exact location, the condition under which it fails, and a concrete fix.
• Separate issues from observations. Never mix "you must fix this" with "I noticed this interesting thing."
• Do not report style preferences (indentation, naming) as issues unless they violate an explicitly configured linter rule or the project's stated conventions.