Code Review

A disciplined, methodology-driven code review. The goal is not to leave many comments — it is to find the issues that matter, rank them honestly by impact, and propose a concrete fix for each. A review that flags one real SQL-injection beats a review that flags twenty style nits and misses the bug.

This skill is grounded in established practice:

• Correctness first. Reviewers and research (Google, Microsoft, the SmartBear

studies) converge on the same point: the highest-value defects found in review are logic and correctness defects, not formatting. Read for what the code does before what it looks like.

• Security via OWASP / CWE. The security pass is structured around the

OWASP Top 10 categories and the corresponding CWE weakness classes, so findings map to a recognized taxonomy instead of vibes.

• Performance via Big-O reasoning. Performance findings are justified by

complexity and the cost model (allocations, round-trips, I/O), not by micro-optimization folklore.

• "Comment on what matters, not nits." In the spirit of Google's engineering

code-review guidelines: distinguish things that must change from preferences, label preferences as such, and never block a merge on style when a linter or formatter could settle it.

When to Activate

Activate this skill when the user:

• Shares a diff, patch, or pull request and asks for a review.
• Pastes a file or function and asks "is this correct / safe / any bugs?".
• Asks for a pre-merge check, audit, or second pair of eyes.
• Asks specifically to focus on security, performance, or correctness

of a change (run the relevant pass with extra depth, but still do a quick sweep of the others).

If the request is "write code" or "explain code," this is not the right skill. This skill evaluates existing code.

Step 1: Scope & Context

Before reading line by line, establish what you are reviewing. State these back to the user briefly so assumptions are visible:

• What changed? A diff/PR (review only the changed lines plus their blast

radius) or a whole file (review all of it). For a diff, the surrounding unchanged code is context, not subject — but flag it if the change makes it wrong (e.g. a new caller breaks an old invariant).

• Language & framework. Determines the idioms, the footguns, and the

security surface (e.g. Django ORM vs raw SQL, React vs server-rendered HTML).

• Runtime & security surface. Is this server-side, client-side, a CLI, a

library, infra-as-code? Does it touch the network, the filesystem, a database, user input, authentication, money, or PII? The surface dictates which passes matter most.

• Tests present? Did the change add or modify tests? Is there existing

coverage for the touched code? "No tests for a behavior change" is itself a finding.

• Intent. What is the change supposed to do? Review against intent — a

correct implementation of the wrong thing is still a finding.

If critical context is missing (e.g. you cannot tell whether input is trusted), say so explicitly and review for the worst plausible case.

Step 2: Severity Model

Every finding gets exactly one severity. Be honest — inflating severity trains people to ignore you; deflating it lets real bugs ship.

Rules:

• Label nits as `Nit:` and never block on them. If a formatter/linter can

fix it, say so in one line and move on. Do not let nits dilute the signal.

• One severity per finding. If something is both a perf and a correctness

problem, file it as the higher one and mention the other dimension.

• Justify Critical/High. State the input or condition that triggers the bad

behavior. "Could be a problem" is not a finding; "with n=0 this divides by zero" is.

Step 3: Review Passes in Priority Order

Run the passes in this order. Earlier passes outrank later ones: a correctness bug is more important than a naming nit on the same line. Each pass is a checklist of concrete things to look for.

(a) Correctness — does it actually work?

The highest-value pass. Read the changed logic and ask "for which input does this do the wrong thing?"

• Off-by-one / boundaries: loop bounds, slice indices, < vs <=,

inclusive/exclusive ranges, empty collections, single-element collections.

• Null / undefined / None: dereferencing a value that can be absent;

Optionals unwrapped without a check; missing map keys; default-vs-missing.

• Error handling: swallowed exceptions; errors logged but not handled;

ignored return/error values; partial failures that leave inconsistent state; cleanup that doesn't run on the error path (no finally/defer/with).

• Edge cases: zero, negative, empty string, very large input, Unicode,

duplicate keys, the "happy path only" smell.

• Concurrency / races: shared mutable state without synchronization;

check-then-act (TOCTOU); non-atomic read-modify-write; assuming ordering between async tasks; deadlock / lock-ordering.

• Async / await: missing await (fire-and-forget promise); awaiting in a

loop that should be parallel; unhandled promise rejection; mixing callback and promise styles; async function whose error path is silently dropped.

• Resource leaks: files/sockets/connections/locks opened but not reliably

closed; growing caches/listeners never released; goroutines/threads that never exit.

• Numeric / floating-point: == on floats; money in floats instead of

integer minor units / decimal; integer overflow/truncation; rounding direction.

• Time & timezones: naive vs aware datetimes; assuming UTC; DST gaps;

off-by-one on dates; comparing timestamps in different units (s vs ms).

• Logic: inverted conditions, wrong boolean operator, copy-paste of the

wrong variable, fall-through, default branch missing.

(b) Security — can it be abused? (OWASP / CWE)

Tag each finding with the relevant OWASP category and/or CWE id where it applies.

• Injection — SQL, NoSQL, OS command, LDAP, template. String-concatenated

queries or shell commands from user input. (OWASP A03; CWE-89 SQLi, CWE-78 command injection, CWE-94 code injection.)

• Broken access control — missing/incorrect authz checks; IDOR (acting on an

object id without verifying ownership); trusting a client-supplied role/flag. (OWASP A01; CWE-285, CWE-639.)

• Authentication gaps — missing auth on a sensitive route; weak session

handling; credentials compared non-constant-time. (OWASP A07; CWE-287.)

• Secrets in code — hardcoded API keys, passwords, tokens, private keys;

secrets logged. (CWE-798 hardcoded credentials, CWE-532 secrets in logs.)

• Unsafe deserialization — pickle/yaml.load/Java native deserialization

of untrusted data; prototype pollution. (OWASP A08; CWE-502.)

• SSRF — fetching a user-controlled URL without allow-listing the host.

(OWASP A10; CWE-918.)

• Path traversal — joining user input into a filesystem path without

normalization/containment. (CWE-22.)

• XSS — rendering untrusted data into HTML without escaping; innerHTML /

dangerouslySetInnerHTML; unsanitized template output. (OWASP A03; CWE-79.)

• Missing input validation — trusting length/type/range/format of external

input; mass assignment binding unexpected fields. (CWE-20, CWE-915.)

• Insecure crypto / transport — MD5/SHA1 for passwords (use bcrypt/argon2);

ECB mode; static IV; disabled TLS verification; predictable randomness for tokens. (OWASP A02; CWE-327, CWE-295, CWE-330.)

(c) Performance — will it scale? (Big-O + cost model)

Justify each finding with complexity or a concrete cost (round-trips, bytes, allocations). Do not micro-optimize cold paths.

• N+1 queries — a query inside a loop over rows; missing eager-load/join.
• Accidental O(n²) — nested loops over the same collection; in/indexOf on

a list inside a loop (use a set/map); repeated string concatenation in a loop.

• Unnecessary allocations / copies — copying large structures per iteration;

building a full list when a generator/stream suffices; boxing in hot paths.

• Blocking I/O on a hot path — synchronous network/disk in a request handler

or event loop; missing batching; chatty APIs.

• Missing indexes — querying/filtering on an unindexed column at scale.
• Unbounded growth — caches/queues/lists with no eviction or limit; loading

an entire dataset into memory; recursion without depth bound.

(d) API & Design — is it well-shaped?

• Naming — does the name say what it does? Misleading or vague names; verbs

for queries with side effects.

• Cohesion / responsibility — function doing too much; mixed levels of

abstraction; god object.

• Leaky abstractions — internal types/SQL/HTTP details bleeding through a

public interface.

• Backward compatibility — removed/renamed public function, changed

signature, changed default, changed serialization format, narrowed accepted input. Flag breaking changes explicitly and ask about versioning.

• Error contracts — what does this throw/return on failure? Is it consistent

with siblings? Are errors typed/actionable or stringly-typed?

(e) Tests — is the change covered?

• Coverage for the change — is the new/changed behavior actually exercised?

A behavior change with no test is a finding.

• Edge & negative cases — only the happy path is tested; no test for the

error path, empty input, or the boundary the code handles.

• Flaky patterns — reliance on real time/sleep, network, ordering of maps,

shared global state between tests, randomness without a fixed seed.

• Asserting implementation, not behavior — tests that assert internal calls /

private state and will break on refactor without catching real regressions; over-mocking that tests the mock.

(f) Maintainability — can the next person change it?

• Dead code — unreachable branches, unused vars/imports/params, commented-out

blocks.

• Duplicated logic — copy-paste that should be extracted; the same constant

redefined.

• Unclear names / magic numbers — unexplained literals; abbreviations;

single-letter names outside tight loops.

• Comment quality — comments that restate the code vs explain why; stale

comments contradicting the code; missing doc on a non-obvious public API.

Step 4: Red-Flags Quick Scan

Before (or alongside) the passes, do a fast scan for instant blockers. Any hit is Critical until proven safe:

• Hardcoded secret / API key / password / private key.
• eval / exec / Function() / system() on anything derived from input.
• SQL/command built by string concatenation or f-string with a variable.
• TLS/cert verification disabled (verify=False, rejectUnauthorized: false,

InsecureSkipVerify: true).

• Catch-all that swallows errors silently (except: pass, empty catch {}).
• A committed .env, credentials file, or key material in the diff.
• Infinite-loop / unbounded-recursion risk (loop with no progress toward exit).
• dangerouslySetInnerHTML / innerHTML with non-constant data.
• Disabled auth/permission check, or a # TODO: add auth on a live route.

Step 5: Output Format

Produce the review in this structure. Be specific, cite the line, propose the fix, and separate must-fix from nice-to-have.

1. Verdict

One line, one of:

• Approve — no blocking issues; any comments are Low/Nit.
• Approve with comments — safe to merge after addressing minor points; no

Critical/High.

• Request changes — one or more Critical/High findings must be fixed first.

Follow with a one-sentence rationale.

2. Findings table

Order by severity (Critical → Nit). Use stable IDs (C1, H1, M2, L1, N1) so the user can reference them.

3. Per-finding detail

For each finding, in severity order:

[C1] SQL injection — `path/file.py:42` — Critical (CWE-89, OWASP A03) Why it matters: the user_id value comes straight from the request and is interpolated into the query string, so an attacker can read or drop tables. Fix: use a parameterized query. ``python # before cur.execute(f"SELECT * FROM orders WHERE user_id = {user_id}") # after cur.execute("SELECT * FROM orders WHERE user_id = %s", (user_id,)) ``

Every Critical/High must include the triggering condition and a concrete fix (code where feasible). Mediums should include a fix or clear direction. Lows/Nits can be one line each.

4. What's good

A short, genuine section acknowledging strengths — solid test coverage, a clean abstraction, a good edge-case already handled. This is not filler: it calibrates trust and signals you read the whole change, not just hunted for faults.

Reviewer principles (carry through every review)

• Be specific. Cite file:line. "Somewhere there's a bug" helps no one.
• Propose the fix. A finding without a suggested direction is half a review.
• Separate must-fix from nice-to-have. The severity model is the contract.
• Don't block on nits. Label them, batch them, move on.
• Review the code, not the author. "This function…", not "you always…".
• Stay within scope. Note adjacent problems briefly, but don't rewrite the

PR; the diff under review is the subject.