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 @ 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.

| Tool | Purpose | | --- | --- | | git | Diff and blame inspection | | gh (GitHub CLI) | Pull request metadata, review comments | | rg (ripgrep) | Pattern sweep across the diff | | fd | Fast file finder | | jscpd | Multi-language clone detection | | semgrep | Polyglot SAST with rules per language | | golangci-lint | Go meta-linter (50+ linters) | | staticcheck | Go advanced static analysis | | govulncheck | Go CVE scan | | cargo clippy | Rust idiomatic linter | | cargo audit | Rust CVE scan | | cargo deny | Rust dependency policy | | tsc | TypeScript compiler (--noEmit) | | eslint / biome | TypeScript linter | | knip | Find unused TS exports/files/deps | | npm audit / bun audit | Node/Bun CVE scan | | ruff | Python lint + format (replaces flake8/black/isort/most of pylint) | | mypy / pyright | Python strict type check | | bandit | Python SAST (CWE-mapped) | | pip-audit / safety | Python CVE scan | | pytest | Python test runner (optional verification only) | | Test runners (go test, cargo test, bun test, vitest, pytest) | Optional verification only |

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.

| $LANG | Reference to load | Primary linter | | --- | --- | --- | | go | [references/GOLANG.md](references/GOLANG.md) | golangci-lint | | rust | [references/RUST.md](references/RUST.md) | cargo clippy | | typescript | [references/TYPESCRIPT.md](references/TYPESCRIPT.md) (+ [references/NEXTJS.md](references/NEXTJS.md) if Next.js 16+ App Router detected via package.json and app/) | tsc --noEmit + eslint/biome | | python | [references/PYTHON.md](references/PYTHON.md) | ruff + mypy/pyright + bandit | | polyglot | Run Phase 1–5 per language detected | per-language | | other | Skip Phase 1.2 sweeps; run Phase 2 + generic Phase 3–5 | semgrep generic ruleset |

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

| Root type | Ownership rule | Static command root | | --- | --- | --- | | Go module | nearest ancestor go.mod | run Go tools from that module | | Cargo workspace | workspace root if workspace exists; otherwise crate root | run Cargo tools with package filters when available | | npm/pnpm/yarn/bun workspace | nearest package or workspace root from lockfile config | run package scripts in touched package first | | Python project | nearest ancestor pyproject.toml (else requirements.txt) | run ruff / mypy / bandit from that root |

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 ( --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.

| # | Category | Detection | | --- | --- | --- | | 1 | Hardcoded secrets | rg -i '(password\|secret\|api[_-]?key\|token\|bearer)\s*[:=]\s*["\x27][A-Za-z0-9/+=_-]{8,}["\x27]' | | 2 | TODO/FIXME/XXX | rg -n '\b(TODO\|FIXME\|XXX\|HACK)\b' (finding only when newly introduced, shipping to main, and not linked to an issue) | | 3 | Stack trace exposure | rg -n '(stack\|stacktrace\|traceback\|panic)' --type-add 'web:*.{go,ts,tsx,rs,py}' --type web | | 4 | Unbounded loops / collections | rg -n 'for\s*\(\s*;;\s*\)\|while\s*\(true\)\|loop\s*\{' | | 5 | Disabled error handling | rg -n '_ =\|catch\s*\(\s*_\s*\)\|\.unwrap\(\)\|\.expect\(\|\.ok\(\)\.unwrap\(\|except\s*:\|except\s+Exception\s*:' | | 6 | Insecure crypto | rg -n '(md5\|sha1\|des\|InsecureSkipVerify\|crypto/rand vs math/rand)' | | 7 | Logging of sensitive data | rg -n 'log\.(Info\|Debug\|Print).*\b(password\|token\|secret\|cookie\|authorization)\b' | | 8 | Wide-open CORS | rg -n 'Access-Control-Allow-Origin.*\*\|AllowAllOrigins\|origin: ["\x27]\*' | | 9 | Disabled lints / suppressions | rg -n '(// nolint\|//nolint\|#\[allow\(\|@ts-ignore\|@ts-nocheck\|eslint-disable\|# noqa\|# ruff: noqa\|# type: ignore\|# pyright: ignore)' | | 10 | Test code in production paths | rg -n '(println\!\|console\.log\|fmt\.Println\|^\s*print\()' --glob '!**/*test*' |

Per-language sweeps live in [references/GOLANG.md](references/GOLANG.md), [references/RUST.md](references/RUST.md), [references/TYPESCRIPT.md](references/TYPESCRIPT.md), and [references/PYTHON.md](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

| Smell | Detection | Action | | --- | --- | --- | | Unrelated changes mixed in | git diff --name-only against the PR description | Ask the author to split | | Whitespace-only churn | git diff --ignore-all-space shows a smaller diff | Note as Low severity, not blocking | | Large generated files committed | *.lock, *.min.js, dist/ in the diff | Verify intentional; check .gitignore | | Re-formatted file alongside a tiny logic change | Many lines changed, few semantic | Ask for a separate format-only commit |

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)

| OWASP | Category | What to read | | --- | --- | --- | | API1 | Broken Object Level Authorization (BOLA / IDOR) | Every handler that takes an ID — does it check ownership? | | API2 | Broken Authentication | Token issue/refresh/revoke paths; password handling | | API3 | Broken Object Property Level Authorization | Mass assignment; allow-listed fields on update | | API4 | Unrestricted Resource Consumption | Rate limits, body size limits, pagination caps | | API5 | Broken Function Level Authorization | Admin / non-admin route separation, RBAC checks | | API6 | Unrestricted Access to Sensitive Business Flows | Captcha / quotas on signup, order, transfer | | API7 | Server-Side Request Forgery | Outbound HTTP calls with user-supplied URLs | | API8 | Security Misconfiguration | Headers (CSP, HSTS), TLS, debug flags, default creds | | API9 | Improper Inventory Management | Public endpoints not in OpenAPI / spec | | API10 | Unsafe Consumption of APIs | Deserialization of upstream JSON without schema | | Web1–10 | Web equivalents

…

Source & license

This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code.

• Author: Bruno-Cunha-Souza
• Source: Bruno-Cunha-Souza/ValarMindSkills
• License: MIT

Install and usage instructions live in the source repository linked above.