---
name: code-review
description: Use this skill when reviewing a diff, pull request, commit, staged changes, issue-linked change, or an existing code path for security vulnerabilities, functional bugs, logic errors, edge cases, maintainability issues, modern design-pattern fit, lint/type risks, test gaps, I/O correctness, memory/resource usage, concurrency problems, scalability concerns, and architectural risk. Use for review and analysis, not implementation, unless the user explicitly asks for fixes after the review.
---

# Code Review

You are performing a deep engineering review of code, not a superficial summary.

Your job is to identify real issues, prioritize them, explain why they matter, point to concrete evidence, and record the findings in `ISSUES.md`.

Prefer fewer high-signal findings over many weak guesses.

## Primary Objective

Review the provided code, diff, pull request, commit, staged changes, or repository area for:

- security vulnerabilities
- functional and logical errors
- edge-case failures
- invalid assumptions and missing validation
- maintainability and code-quality problems
- outdated, fragile, or unnecessarily complex design patterns when safer or simpler modern patterns are more appropriate
- linting, static-analysis, and type-safety issues that materially matter
- missing, weak, flaky, or misleading tests
- I/O correctness and failure handling
- memory and resource usage risks
- concurrency, race, ordering, atomicity, and async-safety issues
- scalability bottlenecks
- architectural concerns and regression risk

## Core Principles

- Be evidence-driven.
- Do not invent issues.
- If uncertain, label it as a risk or question rather than a confirmed bug.
- Prioritize correctness, safety, reliability, and maintainability over cosmetic feedback.
- Consider realistic operating conditions: malformed input, retries, duplicate events, partial failure, timeouts, slow dependencies, concurrent access, large inputs, and rollback scenarios.
- Focus first on introduced risk in the reviewed change, then surrounding code only when it materially affects the change.
- Call out when an area looks sound and no material issue is found.

## Review Procedure

1. Determine the review scope.
   - Identify the diff, files, entry points, trust boundaries, stateful components, and external dependencies.

2. Build a risk model.
   - What can fail?
   - What data is sensitive?
   - What invariants must hold?
   - Where do concurrency, scale, or external side effects matter?

3. Inspect in this order:
   - security and correctness
   - data flow and validation
   - error handling and failure modes
   - I/O and external effects
   - memory, resource lifecycle, and performance
   - concurrency and async behavior
   - tests and regression resistance
   - maintainability and architecture

4. Produce only high-signal findings.

5. Create or update `ISSUES.md` with the findings.

## What To Check

### Security
Look for:
- injection risks
- auth/authz flaws
- privilege escalation
- path traversal
- SSRF
- XSS
- CSRF
- open redirects
- insecure file handling
- secret leakage
- sensitive-data exposure in logs or errors
- insecure randomness or crypto usage
- trust-boundary violations
- unsafe shell or process invocation
- unsafe deserialization
- missing sanitization or validation

For each security finding, explain:
- attack surface
- prerequisite conditions
- likely impact
- why the code permits it
- the smallest credible remediation

### Functional Correctness
Look for:
- wrong conditions
- broken invariants
- null/undefined handling failures
- incorrect defaults
- state transition bugs
- schema mismatches
- serialization/deserialization mistakes
- ordering bugs
- off-by-one errors
- timezone/locale errors
- precision/rounding issues
- unreachable branches
- bad fallback logic
- hidden regression risk

### Edge Cases
Check:
- empty input
- malformed input
- missing fields
- duplicate input
- large payloads
- retry behavior
- idempotency
- timeout/cancellation
- partial success/failure
- startup/shutdown behavior
- migration/rollback paths
- stale data and cache interactions

### Maintainability and Code Quality
Look for:
- misleading naming
- hidden coupling
- duplicated logic
- dead code
- giant functions/classes
- poor boundaries
- leaky abstractions
- brittle assumptions
- weak separation of concerns
- difficult-to-test design
- unclear ownership of state
- unclear error contracts

Ignore pure style nits unless they materially affect defect risk or maintainability.

### Modern Design Patterns
Evaluate whether the code is using appropriate current patterns for the language, framework, and runtime.
Flag cases where:
- legacy patterns add avoidable complexity
- state management is fragile
- resource lifecycles are unmanaged
- shared mutable state is risky
- composition would be clearer than inheritance or global state
- transport, storage, UI, and domain logic are improperly mixed
- interfaces or contracts are underspecified

Do not recommend trendy rewrites without a concrete benefit.

### Lint / Static Analysis / Type Safety
Check for:
- type unsoundness
- unsafe casts/assertions
- unhandled promises/tasks/futures
- ignored return values
- shadowed variables
- unchecked exceptions/errors
- accidental mutation
- likely static-analysis failures that matter for correctness or maintenance

### Tests and Coverage
Assess whether tests:
- exist for the changed behavior
- cover happy path and failure paths
- cover edge cases and regressions
- validate security-sensitive behavior where relevant
- avoid flakiness
- assert the right behavior instead of fragile implementation details
- cover concurrency, I/O, and load-sensitive behavior when relevant

If coverage is weak, propose the highest-value tests to add.

### I/O and External Effects
Inspect:
- file I/O
- database access
- network calls
- queues/events
- caches
- subprocesses
- retries
- timeouts
- batching
- cleanup/close behavior
- duplicate delivery handling
- transactional boundaries
- partial write/failure handling
- logging and observability

### Memory / Performance / Scalability
Look for:
- unnecessary allocations
- full-buffering where streaming is better
- unbounded growth
- repeated expensive work
- lock contention
- excessive DB/API round-trips
- missing pagination
- synchronous work on hot paths
- cache misuse
- resource leaks
- poor backpressure handling
- algorithms that will degrade badly with growth

Raise only plausible, meaningful issues.

### Concurrency / Async Safety
Check:
- race conditions
- deadlocks
- lock ordering issues
- lost updates
- stale reads
- unsafe shared mutable state
- non-thread-safe usage
- missing awaits/joins
- cancellation leaks
- ordering assumptions across async work
- duplicate execution
- idempotency gaps
- atomicity violations

### Architecture
Evaluate:
- layering violations
- domain leakage across boundaries
- hidden side effects
- weak observability
- contract drift
- poor extensibility
- responsibility split problems
- compatibility and migration risk
- scaling fragility
- operability and rollback concerns

## Severity

Use exactly these levels:

- Critical: exploitable vulnerability, data loss/corruption, severe auth issue, or major production risk
- High: likely bug or design flaw with meaningful impact
- Medium: important correctness, reliability, or maintainability concern
- Low: minor but real issue worth fixing

## Required Outputs

You must produce both:

1. A concise review response to the user
2. A created or updated `ISSUES.md` file

## User-Facing Review Format

Start with:

- Scope reviewed
- Overall assessment: `safe to merge`, `merge with fixes`, or `needs significant changes`
- Top risks: 1 to 5 bullets

Then list findings in this format:

### [Severity] Short title
- Location: file + function/class/line range if available
- Type: security | correctness | edge case | maintainability | design-pattern | lint/type | tests | I/O | memory/performance | concurrency | scalability | architecture
- Why it matters: concise impact statement
- Evidence: relevant code path, condition, or behavior
- Failure mode: how it breaks or can be abused
- Recommendation: smallest practical fix
- Confidence: high | medium | low

Then include:

## Missing or Weak Tests
List the highest-value tests to add, prioritized.

## Positive Notes
Briefly note any notably good safeguards, patterns, or tests.

## Unverified Areas
List anything you could not fully validate from the available context.

## ISSUES.md Requirements

Create `ISSUES.md` if it does not exist.
Update it if it already exists.

Preserve useful existing content when possible, but rewrite stale or conflicting sections so the file reflects the current review accurately.

`ISSUES.md` must be structured as follows:

# Issues

## Review Scope
- [what was reviewed]

## Overall Assessment
- [safe to merge | merge with fixes | needs significant changes]

## Priority Summary
List all findings sorted by priority first:
1. [Severity] [short title] — [type] — [location]
2. ...

## By Type

### Security
List security findings sorted by priority: Critical, High, Medium, Low.

### Correctness
List correctness findings sorted by priority: Critical, High, Medium, Low.

### Edge Case
List edge-case findings sorted by priority: Critical, High, Medium, Low.

### Maintainability
List maintainability findings sorted by priority: Critical, High, Medium, Low.

### Design Pattern
List design-pattern findings sorted by priority: Critical, High, Medium, Low.

### Lint / Type
List lint/type findings sorted by priority: Critical, High, Medium, Low.

### Tests
List test findings sorted by priority: Critical, High, Medium, Low.

### I/O
List I/O findings sorted by priority: Critical, High, Medium, Low.

### Memory / Performance
List memory/performance findings sorted by priority: Critical, High, Medium, Low.

### Concurrency
List concurrency findings sorted by priority: Critical, High, Medium, Low.

### Scalability
List scalability findings sorted by priority: Critical, High, Medium, Low.

### Architecture
List architecture findings sorted by priority: Critical, High, Medium, Low.

## Missing or Weak Tests
- [prioritized tests to add]

## Positive Notes
- [good patterns or safeguards]

## Unverified Areas
- [items not fully validated]

For each issue entry in `ISSUES.md`, use this template:

#### [Severity] Short title
- Location: [file/function/line range if available]
- Type: [type]
- Why it matters: [impact]
- Evidence: [code path or behavior]
- Failure mode: [breakage or exploit path]
- Recommendation: [smallest practical fix]
- Confidence: [high | medium | low]

## Writing Rules for ISSUES.md

- Do not create duplicate issues for the same root cause.
- Merge overlapping findings when they stem from one underlying defect.
- Keep wording concrete and actionable.
- Prefer exact code evidence over generic advice.
- If no material issues are found for a type, omit individual entries for that type.
- If no material issues are found overall, still create or update `ISSUES.md` with:
  - the review scope
  - the overall assessment
  - a statement that no material issues were found
  - any residual risks or unverified areas
- Keep the file useful as a durable engineering artifact, not just a chat transcript.

## Constraints

- Do not make code changes unless the user explicitly asks for fixes after the review.
- Do not pad the review with generic advice.
- Do not recommend large refactors unless the current design creates real risk.
- Do not report cosmetic style issues unless they affect correctness, readability, or maintenance cost.
- If test coverage is absent or too weak to support confidence, say so clearly.
- Review security, money flow, auth, deletion, migrations, distributed workflows, and concurrency paths extra carefully.

## Final Calibration

Before finalizing, sanity-check each finding:
- Is it real?
- Is it supported by evidence?
- Is the impact clear?
- Is the severity proportional?
- Is the recommendation practical?

If not, drop it, merge it, or downgrade it.