/reviewing-code-changes

name: reviewing-code-changes
description: Perform a security review of a diff, branch, or pull request — assessing what the change introduces, weakens, or exposes, with a triage-first workflow and false-positive discipline. Use when asked to security review a PR or branch, check a diff before merge or commit, or assess whether a change is safe to ship.
verified: 2026-07-27

Reviewing Code Changes

Reviewing a diff is not auditing a codebase in miniature. The diff shows you what changed but hides what the change *means* — the caller that now receives untrusted data, the check that used to run, the assumption that no longer holds. Reviewing only the added lines is the standard way to miss the bug.

When to Use

• Security review of a pull request, branch, or commit range
• Pre-commit or pre-merge check on your own changes
• Assessing risk of a dependency bump or config change
• Reviewing an AI-generated change before it ships

When NOT to Use

• **Whole-codebase audit** — use `auditing-code-for-vulnerabilities`
• **Runtime testing of the deployed change** — use the relevant testing skill
• **Dependency/CI changes as the main subject** — use `auditing-supply-chain`

Get the Real Diff First

# Against the actual merge base, not against whatever HEAD happens to be
BASE=$(git merge-base HEAD origin/main)
git diff --stat $BASE...HEAD
git diff $BASE...HEAD

# Rename and whitespace noise hides real changes
git diff -M -C -w $BASE...HEAD

# Files changed most, and by whom — where to spend attention
git diff --numstat $BASE...HEAD | sort -rn | head -20

Three things to establish before reading code:

1. **What is this change trying to do?** Read the PR description and the tests. A security review without the intent is pattern matching. 2. **Does the diff match the description?** Unexplained files — a config change, a new dependency, a workflow edit riding along with a feature — are the highest-signal thing in a review. 3. **What is the change's exposure?** A change to an unauthenticated request path deserves an order of magnitude more attention than an internal refactor.

Triage: Where to Spend the Review

Not all diffs deserve equal time. Rank hunks by what they touch:

| Priority | Signals | | --- | --- | | **Critical** | Auth/authz logic, session handling, crypto, input parsing on an untrusted boundary, SQL/command/template construction, file path handling, deserialization, CI/CD workflows, IAM/security-group config, dependency additions | | **High** | New endpoints or routes, new tool/RPC surface, error handling on security paths, logging of user data, tenant scoping, cache keys, rate limits | | **Medium** | Business logic that consumes validated input, refactors that move security-relevant code | | **Low** | Tests, docs, formatting, comments — but read test *deletions* |

# Fast triage of a large diff
git diff $BASE...HEAD --name-only | rg 'auth|login|session|crypto|password|token|admin|permission|\.github/workflows|Dockerfile|terraform|policy'
git diff $BASE...HEAD -U10 | rg -n '^\+.*(eval\(|exec\(|system\(|innerHTML|pickle|yaml\.load|Sprintf.*SELECT|shell=True|verify=False|InsecureSkipVerify)'

What to Look For

Removals and weakenings — read the minus lines first

The most dangerous diffs delete things. Search the removed lines specifically:

git diff $BASE...HEAD | rg '^-' | rg -i 'auth|verify|valid|check|sanitiz|escape|permission|assert|csrf|limit'

• A validation call deleted "because it was redundant"
• An authorization decorator removed during a refactor
• A test deleted rather than fixed
• A `TODO: add authz` that was never done and is now shipping
• A timeout, size cap, or rate limit removed
• `strict` mode, a security header, or a CSP directive relaxed

Additions — the ordinary classes, in context

Apply the bug-class hunting from `auditing-code-for-vulnerabilities`, but scoped to the changed paths: injection sinks, authorization on new object lookups, SSRF in new outbound calls, path handling on new file operations, deserialization, secrets, weak randomness, unsafe defaults.

The two questions that catch most real issues in a diff:

> **Does new untrusted data reach an old sink?** > **Does old untrusted data reach a new sink?**

Both require reading outside the diff.

Context beyond the diff

Read the whole function for every non-trivial hunk. Then check callers:

# Who calls the changed function, and do they still satisfy its assumptions?
rg -n 'changedFunctionName\s*\(' --type <lang>
# What else in the codebase uses the pattern this change introduced?
git log -1 --format=%H $BASE  # anchor, then compare behaviour before/after

Specifically check whether the change alters an invariant that other code relies on: a function that used to sanitize and now does not, a return value whose meaning flipped, a nullable that became non-null, an ordering guarantee that was dropped.

Config, infra, and pipeline changes riding along

git diff $BASE...HEAD -- '*.yml' '*.yaml' '*.tf' 'Dockerfile*' '.github/**' '*.json'

• New or bumped dependencies: who maintains them, and does the lockfile change

match the manifest change? (See `auditing-supply-chain`.)

• Workflow permissions widened, `pull_request_target` added, unpinned actions
• Security group / firewall rules opened, buckets made public, IAM `"*"`
• Debug flags, verbose logging of request bodies, disabled TLS verification
• Feature flags that default to on

AI-generated and large mechanical changes

Machine-written diffs fail in characteristic ways: plausible-looking validation that does not actually constrain, error handling that swallows security-relevant failures, invented API usage that silently no-ops, and copied patterns applied where the surrounding assumptions differ. For a large generated diff, sample the security-relevant hunks and verify against the real library behaviour rather than reading for plausibility.