Code Review

Code Review Lifecycle

"Code is read far more often than it is written. A review is the first reading after the first write." — adapted from the Go proverbs.

This skill conducts a structured, evidence-first review of a code change set. It is read-only by default — it never edits code, it produces a report. It is language-aware for Go, Rust, TypeScript (Node and Bun), and Python (CPython 3.13 / 3.14 on FastAPI / Django / Flask); other languages are best-effort using the generic principles. It is lifecycle-driven: detect → sweep → read → assess → report → cross-link.

The skill exists because LLM reviewers tend to hallucinate findings: invented function names, wrong file paths, fabricated CVEs, and severity inflation. Every guardrail in the Constraints section is there to push back on those failure modes.

When to Use

• A pull request is open and the user wants a thorough review before approving or merging.
• A specific commit, branch, or directory needs an audit (security, performance, maintainability, or all three).
• Legacy code is about to be refactored and the user wants a baseline assessment.
• A pre-release gate confirms the diff matches the quality level the team thinks they are running at.
• An incident post-mortem revealed a class of bug and the user wants the surrounding code swept for it.
• The user explicitly asks: 'review code', 'code review', 'revisar código', 'PR review', 'auditar código', or invokes /valarmindskills:code-review.

Do not use when

• The user wants to fix code — this skill never edits. Hand off to the user or to @code-debugger for runtime issues.
• The user wants a commit message or release notes — use @github-commit or @github-release-note.
• The change is a single typo, comment edit, or trivial rename — review overhead exceeds the value; tell the user and stop.
• The change is in a language the skill cannot detect (not Go, Rust, TypeScript, Python, or covered by an explicit @<lang> skill). Surface the gap and ask whether the user wants a generic pass.
• The user's primary ask is to run tests, reproduce a failure, or debug runtime behavior — use @code-debugger. This skill may run optional verification commands only when they are explicitly requested or needed to validate a review finding.
• The diff is on infrastructure (Terraform, Kubernetes manifests) — use @code-security-review (Next branch — references/nextjs/; Go branch — references/golang/; Python branch — references/python/) or @ci-cd-generator for those domains.

Prerequisites

Install before starting a review. Each tool's absence is logged and the related Phase is degraded but never silently skipped. The default mode is static review: read diffs, run linters, type checks, SAST, and dependency audit. Tests and runtime commands are optional verification, not part of the default review.

Required access:

• Read access to the repository and the diff (locally or via gh pr diff)
• Permission to invoke linters, type checks, and dependency scanners on the host
• Explicit permission or user request before running tests or other runtime verification
• If the review targets a private dependency: read access to that module

The skill does not require write access. It never commits, never pushes, never edits source files.

Phase 0 — Project & Scope Detection

Detect language, package manager, review scope, and diff range before sweeping anything. Run the steps in order; stop at the first conclusive match per axis.

# Step 1 — language at the repo root
test -f go.mod        && echo "language: go"
test -f Cargo.toml    && echo "language: rust"
test -f package.json  && echo "language: typescript"
test -f tsconfig.json && echo "  ts-config: present"
test -f pyproject.toml && echo "language: python"
test -f requirements.txt && echo "language: python (legacy manifest)"

# Step 2 — TypeScript runtime (only if language=typescript)
test -f bun.lockb         && echo "runtime: bun, pm: bun"
test -f pnpm-lock.yaml    && echo "runtime: node, pm: pnpm"
test -f yarn.lock         && echo "runtime: node, pm: yarn"
test -f package-lock.json && echo "runtime: node, pm: npm"

# Step 2b — Python package manager (only if language=python)
test -f uv.lock       && echo "  pm: uv"
test -f poetry.lock   && echo "  pm: poetry"
test -f Pipfile.lock  && echo "  pm: pipenv"

# Step 3 — review scope and base branch
git rev-parse --abbrev-ref HEAD
gh pr view --json number,title,baseRefName,headRefName 2>/dev/null
BASE_REF="$(gh pr view --json baseRefName --jq .baseRefName 2>/dev/null || git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's@^origin/@@' || echo main)"
BASE_REMOTE="origin/$BASE_REF"
BASE_SHA="$(git merge-base "$BASE_REMOTE" HEAD 2>/dev/null || git merge-base "$BASE_REF" HEAD)"
DIFF_RANGE="$BASE_SHA...HEAD"
git diff --name-only "$DIFF_RANGE" | wc -l
git diff --shortstat "$DIFF_RANGE"

# Step 4 — polyglot or monorepo
fd -t f -d 5 '^(go.mod|Cargo.toml|package.json|pyproject.toml)$' .      # multiple roots → monorepo

Persist as $LANG ∈ {go, rust, typescript, python, polyglot, other}, $BASE_REF, $BASE_SHA, and $DIFF_RANGE.

If the diff exceeds 50 files or 1500 lines, ask the user to split the review or to scope it to a subset before proceeding. Large reviews dilute attention and amplify hallucination risk.

0.1 Diff Scope Contract

Default to the diff. A finding is in scope only when the changed line is in $DIFF_RANGE or the changed code makes an existing line newly reachable, newly exposed, or newly unsafe. Issues outside the diff are reported as Out-of-scope observation unless the user explicitly requested a baseline audit.

Use null-delimited file lists when passing changed files between tools. The loop is portable across empty diffs and file names with spaces:

git diff --name-only -z "$DIFF_RANGE" -- '*.go' | while IFS= read -r -d '' file; do rg -n '<pattern>' "$file"; done
git diff --name-only -z "$DIFF_RANGE" -- '*.rs' | while IFS= read -r -d '' file; do rg -n '<pattern>' "$file"; done
git diff --name-only -z "$DIFF_RANGE" -- '*.ts' '*.tsx' | while IFS= read -r -d '' file; do rg -n '<pattern>' "$file"; done
git diff --name-only -z "$DIFF_RANGE" -- '*.py' | while IFS= read -r -d '' file; do rg -n '<pattern>' "$file"; done

0.2 Monorepo / Workspace Handling

When multiple go.mod, Cargo.toml, or package.json files are present, map each changed file to the nearest owning root before running toolchains. Run Phase 1–5 per touched root, not from the repository root unless the project convention requires it.

Phase 1 — Static Analysis Sweep

Run the static toolchain first; treat results as leads, never as conclusions. Calibration: every linter has known false-positive classes — start each automated finding at Medium severity and only promote to High with manual confirmation in Phase 2.

1.1 Automated Toolchain

# Polyglot SAST (always run if available)
semgrep --config=auto --error --severity=ERROR --severity=WARNING

# Go
golangci-lint run ./...
staticcheck ./...
go vet -all ./...
govulncheck ./...

# Rust
cargo clippy --all-targets --all-features -- -D warnings
cargo audit
cargo deny check

# TypeScript / Node / Bun
bunx tsc --noEmit              # or: npx tsc --noEmit
bunx biome check .             # or: bunx eslint .
bunx knip
bun audit                      # or: npm audit --omit=dev

# Python
ruff check .
ruff format --check .
mypy --strict .                # or: pyright
bandit -r . -q            # run from package root (pyproject.toml dir); use src/ if project uses src-layout
pip-audit
safety check

# Duplication (any language)
npx jscpd --min-lines 5 --min-tokens 50 ./

For each tool, capture the raw output and keep the version (<tool> --version) in the findings report. A finding without a tool version is not reproducible.

1.1.1 Optional Verification Commands

Run tests only when the user asks, CI output is unavailable, or a finding needs confirmation. Label them separately from static tools in the report.

go test ./...                 # add -race only for concurrency findings or explicit request
cargo test --all-features
bun test                      # or: npx vitest run
pytest -q                     # Python

Never claim pass/fail unless the command and relevant output are shown in the report.

1.2 Pattern Sweep — language-agnostic

For each category below, run the grep across changed files only using $DIFF_RANGE and null-delimited file lists, then read the matching files for context.

Per-language sweeps live in references/GOLANG.md, references/RUST.md, references/TYPESCRIPT.md, and references/PYTHON.md.

Phase 2 — Manual Read-Through

Read the diff with the same posture a teammate would: from base branch to head, file by file, top to bottom. The automated tools cannot judge intent — this phase is where intent meets implementation.

2.1 Read order

1. Tests first if any new tests exist — they encode the author's intent.
2. Public API surface — exported functions, types, routes, schemas, CLI flags.
3. Internal logic — handlers, services, business rules.
4. Plumbing — DI wiring, config, build.
5. Infrastructure — Dockerfile, workflow, IaC (only if it touches the diff).

2.2 Read-through questions

For every changed function or block, answer in your head:

• Is the name intention-revealing? Could a reader infer purpose without reading the body?
• Does the function do one thing? If not, why is the merge OK?
• What happens with nil / empty / negative / huge / concurrent inputs?
• What invariants must hold before and after this code runs? Are they checked or assumed?
• Where does untrusted input enter? Where does it leave the boundary trusted?
• What resource is acquired? Where is it released? Under failure?
• What time does this code take in the worst case? Memory? Allocations?
• Could this race with another goroutine / task / Promise?
• Are errors propagated with context or swallowed?
• If this panics / crashes / throws, what is the blast radius?

A finding is born only when the answer is unsatisfactory and the evidence is in the diff. Hallucinations are findings born without one of those two preconditions.

2.3 Diff hygiene

Phase 3 — Security Review

Run after Phase 2 because security findings depend on understanding intent. The OWASP API Top 10 (2023) and OWASP Web Top 10 (2025) are the reference frames.

3.1 Security categories (sweep + read)

For deeper stack-specific OWASP audits, hand off to @code-security-review (Go branch — references/golang/; Next branch — references/nextjs/; Python branch — references/python/). This skill stops at "this PR likely introduces a class-X issue at file.ext:LINE — recommend running the dedicated skill".

3.2 Cross-language security smells

Every language's reference file (GOLANG, RUST, TYPESCRIPT, PYTHON) lists detection commands and code examples for: SQL injection, XSS / SSTI, SSRF, path traversal, command injection, insecure deserialization (pickle / yaml in Python), hardcoded secrets, insecure crypto, log injection, open redirect, race conditions (including free-threaded races on python3.14t), resource exhaustion.

Phase 4 — Performance & Scalability Review

Performance findings have the lowest hallucination tolerance — the LLM cannot benchmark. Constrain claims to patterns known to scale poorly, not to predicted latencies.

For Next.js 16.2.x App Router projects, also load references/NEXTJS.md — covers the <img> rule, Server Components / RSC payload minimization, "use cache", Turbopack notes, and experimental.prefetchInlining trade-offs.

Every finding must cite a file:line and quote the exact code; "this might be slow" without code is not a finding.

Phase 5 — Test, Maintainability & Style Review

5.1 Tests

• New behavior path → new or existing test? If not, severity is usually Medium unless the change is type-only, generated, config-only, unreachable without a future feature flag, or covered by existing tests that the diff exercises.
• Test asserts behavior (output, side effect) or implementation (mock call count)? Behavior tests are durable; implementation tests are smells unless justified.
• Negative paths covered: error propagation, edge inputs, timeouts, cancellation.
• Race detector / property-based tests for concurrent code (Go -race, Rust loom, TypeScript fast-check).
• Test names readable as sentences (TestServer_RejectsUnauthenticatedRequest).

Do not file "missing test" as a finding until you have searched for existing coverage in the touched package. If coverage exists but does not cover the changed branch, cite the missing branch precisely.

5.2 Maintainability (overlap with @clean-code)

• Function size: > 30 lines is a smell. > 60 lines is a finding.
• Cyclomatic complexity: > 10 is a smell. > 20 is a finding.
• Duplication: same block in 2 places is a smell on the third occurrence; before that, leave it. Use the Rule of Three.
• Naming: if a name needs a comment to explain it, the name is wrong.
• Comments: each comment should explain why, not what. Drop "what" comments.
• Public surface: every new exported symbol must have a use site or a doc-comment. If neither, ask why it is exported.

For deep refactor recommendations, hand off to @clean-code.

5.3 Style

Rely on the project's auto-formatter (gofmt, rustfmt, prettier/biome). Style differences that the formatter would catch are not findings — they are bugs in the CI configuration.

Phase 6 — Findings Synthesis & Output

Aggregate, deduplicate, and rank. Each finding is a row in the report; each row needs every column filled or it is dropped.

6.1 Severity rubric

Calibration aids — see references/SEVERITY_RUBRIC.md for the full Impact × Likelihood matrix and per-tool false-positive notes.

6.2 Risk tag

Tag every suggested change so the author can triage:

• SAFE — isolated, no behavior change, no public API impact.
• REVIEW — touches middleware, auth, shared utils, or a module boundary.
• BREAKING — changes a signature, response shape, schema, or observable behavior.

6.3 Confidence tag

• High — the evidence is exhaustive and the reasoning is mechanical.
• Medium — the pattern matches but intent could plausibly justify it.
• Low — the reviewer would benefit from a second opinion.

If Confidence=Low and Severity ≥ High, escalate explicitly: state "needs human review before action".

Constraints

• Never edit code. This skill is a reviewer, not an editor. Suggestions are diffs in the report, never Edit/Write calls.
• Never invent a file path, function name, or symbol. Every reference must be copied from the diff or the repo. If you do not know, say "not in diff — please confirm".
• Never claim a test passes / fails without showing the command and its output. Tests are optional verification, not default review. If results are needed and you cannot run them, ask the user to run them and paste output.
• Never invent a CVE. Cite only CVEs that appear in govulncheck, cargo audit, npm audit, pip-audit, safety, or semgrep output of this run.
• Never quote a line you did not read. Open the file at the cited line; the quote in the report must match byte-for-byte.
• Never inflate severity to look thorough. Inflated severity erodes trust. Use the rubric.
• Never list a finding without a file:line, a code quote, an impact, a fix, and a risk tag. Drop incomplete findings.
• Always run language detection (Phase 0) before sweeping (Phase 1). Skipping detection produces wrong-language patterns and false positives.
• Always cap a single review at 50 files / 1500 lines. If exceeded, ask to split before continuing.
• Always emit the report verbatim in the Output format below — even if there are zero findings.
• Always cross-link to the dedicated skill when the finding belongs to its domain (@code-security-review Go branch — references/golang/; Next branch — references/nextjs/; Python branch — references/python/; @clean-code, @code-debugger; Next.js performance — references/NEXTJS.md here).
• Must include the tool versions used in the report. A review without tool versions is not reproducible.

Output format

Print verbatim after every successful run. The report is the deliverable.

code-review: <branch / PR# / commit range>
  language(s):     <go | rust | typescript | python | polyglot>
  mode:            <static review | static + optional verification>
  base:            <base ref> @ <merge-base sha>
  scope:           <files changed> files / <lines added>+ / <lines deleted>-
  tools:           semgrep <ver>, golangci-lint <ver>, ...
  verification:    <not run | go test ./...: pass | cargo test: fail | ...>
  duration:        Phase 0–6 walked

Findings (ranked by severity, then by file):

| ID  | Sev      | Conf   | Risk     | File:Line                | Title                              |
| --- | -------- | ------ | -------- | ------------------------ | ---------------------------------- |
| R001 | High     | High   | REVIEW   | api/handlers/order.go:42 | BOLA — missing user_id check       |
| R002 | High     | Medium | SAFE     | api/store/db.go:118      | Unbounded SELECT without LIMIT     |
| R003 | Medium   | High   | SAFE     | api/util/log.go:7        | Logs Authorization header verbatim |
| R004 | Low      | High   | SAFE     | api/handlers/order.go:8  | Stuttering name OrderOrder         |

Detailed findings:

  R001 — BOLA — missing user_id check
    File:        api/handlers/order.go:42
    Code:
      | order, err := h.store.GetOrder(ctx, c.Param("id"))
      | if err != nil { ... }
      | c.JSON(200, order)
    Impact:      Any authenticated user can read any order by guessing IDs.
    Suggested fix (REVIEW):
      | order, err := h.store.GetOrder(ctx, c.Param("id"))
      | if err != nil { ... }
    + if order.UserID != claims.UserID { c.AbortWithStatus(403); return }
      | c.JSON(200, order)
    Verification: add an integration test that asserts a 403 when user A
                  requests user B's order id.
    Cross-link:  See @code-security-review (Go branch — references/golang/API.md Phase 2) for full BOLA audit.

  R002 — Unbounded SELECT without LIMIT
    File:        api/store/db.go:118
    Code:
      | rows, err := db.QueryContext(ctx, "SELECT * FROM orders WHERE user_id=$1", uid)
    Impact:      Memory exhaustion when a user has many orders.
    Suggested fix (SAFE): add LIMIT/OFFSET pagination + max-page-size guard.
    Verification: load test with 10k-row user; verify p99 latency stays bounded.

  ... (one block per finding) ...

Summary:
  Critical: 0   High: 2   Medium: 1   Low: 1   Info: 0
  Blocking-merge findings: 2 (R001, R002)
  Suggested next step:
    1. Author addresses R001 and R002 before re-request.
    2. Run @code-security-review (Go branch — references/golang/API.md Phase 2) to confirm no further BOLA cases.
    3. Re-review with `/valarmindskills:code-review` after fixes.

Skill version: code-review @ <git rev of SKILL.md>

When there are zero findings, print this shape instead of inventing minor findings:

Findings (ranked by severity, then by file):

| ID | Sev | Conf | Risk | File:Line | Title |
| -- | --- | ---- | ---- | --------- | ----- |
| (no findings) | | | | | |

Detailed findings:
  (none)

Summary:
  Critical: 0   High: 0   Medium: 0   Low: 0   Info: 0
  Blocking-merge findings: 0
  LGTM — no blocking issues found in scope.

Info-level observations are allowed after the LGTM only when grounded in exact files read during the review.

Related Skills

• @code-debugger — when a finding is a runtime failure rather than a code-review concern, hand off here.
• @clean-code — for refactor patterns and Rule-of-Three deduplication recommendations.
• @code-security-review — multi-language + Go (Gin/Fiber) + Python (FastAPI/Django/Flask, references/python/) + Next.js 16 App Router security lifecycle: design patterns + active runtime testing + stack-specific vulns + 100-vuln catalog (references/WEB_VULNERABILITIES.md) for cross-linking findings.
• @github-pr-review — GitHub-flavored PR review (posts comments via gh).
• @github-commit — when the author wants help drafting the fix commit.
• @superpowers — engineering posture (TDD, evidence-first) for the author addressing findings.

References

• CHECKLIST — copy-paste cheat sheet ordered by review phase
• SEVERITY_RUBRIC — Impact × Likelihood matrix, OWASP/CWE map, per-tool false-positive notes
• GOLANG — Go-specific patterns, sweeps, and example findings
• RUST — Rust-specific patterns, sweeps, and example findings
• TYPESCRIPT — TypeScript / Node / Bun patterns, sweeps, and example findings
• PYTHON — Python 3.13/3.14 (FastAPI / Django / Flask) patterns, sweeps, and example findings (ruff, mypy, bandit, free-threaded build notes)
• NEXTJS — Next.js 16.2.x performance rules (RSC, <img>, "use cache", Turbopack)
• EXAMPLE — end-to-end worked review of a Go pull request