Code Review Correctness

Review every changed file, and report a finding only when confident it is a real defect. A false alarm costs more trust than a missed nit.

Coverage

Build a checklist of every changed file before reading any of them. Each file ends as reviewed, or skipped with a concrete reason. A small or secondary file (a header, an interface, a config counterpart, a test) is not covered by reviewing the file it belongs to. Report the totals: files changed, reviewed, skipped. Do not stop after the first serious finding.

Focus

• Read the pull request title and description, or the commit messages, first. Judge the change against the stated intent, and report where the diff does not do what it says: a file, function, or test the description says this change adds or modifies that the diff does not touch (check the target tree before calling it missing), or a deferral the description states that the diff does anyway.
• Comment on added and modified code only. Deleted and unchanged code is context, except where the deletion itself introduces the defect (a removed check, a dropped branch) or the change makes unchanged code fail; report that at the nearest changed line.
• When the context is unclear, read the surrounding code or search for the callers before judging. Do not flag on assumption.
• Look across the changed files for what one change requires of another: a renamed field, a new parameter, a changed return shape, a config key, a doc or type that should have moved with it.
• Skip comments, generated markers, and formatting unless asked.
• Confirm each finding in the code before reporting it: the symbol really is undefined, the branch really is unreachable, the caller really can pass that value.
• Before dropping a finding as a false positive, point to the line that disproves it. Unverifiable is not wrong.

Do not report

• A defect in unchanged lines that the change did not cause. Note it in one line at the end, unmarked as a finding, if it is serious.
• Code that looks wrong but is correct once the surrounding code is read.
• A nit a senior engineer would not raise in review.
• Anything a linter, formatter, or type checker in the repo will catch.
• Anything the code explicitly silences at that site with an ignore comment.

What to check

• Correctness: logic, boundary conditions (empty input, first and last element, null or missing key, zero divisor, exact float comparison), error paths, and behavior under concurrent calls.
• Performance, only on a hot path or at real data scale: N+1 queries, work repeated inside a loop, resources not released.
• Maintainability: a name that misstates what the code does, or a departure from the project's existing pattern.

Security

Follow the untrusted value: where it enters, every place the change carries it, and what it reaches. A value is untrusted until something in the diff or the code around it validates it, and a value from one service is untrusted to the next.

• Injection: a query, shell command, template, HTML fragment, log format, path, or redirect built by concatenating or interpolating a value instead of parameterizing or escaping it.
• Untrusted input reaching eval, a deserializer, an archive extractor, a URL a server fetches (server-side request forgery), or a file path (traversal).
• Authentication and authorization on every entry point the change adds or changes: who may call it, whether the check is on the object the caller named rather than only on the route, and whether a changed default opens something that was closed.
• Secrets and personal data: a credential, token, key, or connection string in source, a default, a fixture, or a log line, and personal data in a log, an error message, or an analytics call.
• Crypto and randomness: a weak or non-standard algorithm, a hard-coded or reused key, salt, or nonce, a comparison of secrets that is not constant-time, and a general-purpose random generator for a token, a password reset, or a session id.
• Trust boundaries the change moves: a check dropped, a validation weakened, a permissive cross-origin or cookie setting, a new dependency or endpoint that now sees data it did not see before.

Severity

Use the scale the caller passed when it passed one. Otherwise: high is security, data loss, a crash, or a critical function failing; medium is an edge case, performance, or a maintainability problem that can go wrong; low is style, readability, or a minor best practice. A security defect is high. A high finding blocks the merge; medium and low do not.

Number findings and sort by severity. Report each as path:line, severity, what is wrong, why it matters with the fact that shows it, and what the code must do.

Language and file notes

When the coverage checklist holds a file of a type below, open that reference before reviewing that file. Each is a short file in this skill's directory. A file type without a reference gets the general checks above.

• references/python.md — Python
• references/dotnet.md — .NET
• references/typescript-react.md — TypeScript and React
• references/github-workflows.md — GitHub workflows
• references/sql-database.md — SQL and database functions