Code Review

Code Review

Overview

Structured, actionable code review focused on correctness, bugs, style, and maintainability. Act as a senior engineer: thorough, pragmatic, impact-first.

When to Use

• Reviewing staged changes before committing.
• Reviewing branch changes before merging or opening a PR.
• Checking a diff for correctness, bugs, style, or structure.
• Final review of all changes on a feature branch.
• Quick sanity-check of a small changeset before pushing.

When NOT to use:

• Implementing new features from scratch.
• Running test suites (see writing-tests or equivalent).
• Refactoring without a prior review (review first, then refactor on request).
• Linting or formatting only — use a linter or formatter directly.

Invocation Notice

• Tell the user when this skill is running: code-review. Skip the notice when the user asked for the skill by name or slash command; spelling and spacing need not match. A phrase from this skill's own trigger list is not a name — naming the work is not naming the skill.

Constraints

• Do NOT modify code unless explicitly asked.
• Do NOT guess missing code or behavior; ask for missing context.
• Do NOT suggest adding suppressions (e.g., #pragma warning disable).
• Stay focused on review quality; avoid unrelated commentary.
• Work strictly from the provided diff or code context.

Quick Reference

Common Mistakes

• Reviewing without full context — jumping to conclusions without reading the complete diff or understanding intent.
• Mixing review and edits — modifying code during the review phase instead of separating feedback from implementation.
• Nitpick overload — burying critical issues under dozens of style nitpicks.
• Ignoring intent — critiquing design choices without understanding the constraints or goals behind them.
• Incomplete scope — reviewing only one file when the change spans multiple files or modules.

Workflow

Step 0 — Gate: Scope & Tools

Determine review scope and available tooling before doing any analysis.

Scope detection:

Identify whether this is a pre-commit or pre-merge review, then set the base ref accordingly.

Build the review packet first. Run scripts/build_review_packet.py (shipped in this skill's scripts/) to resolve scope deterministically and write one packet — diff, changed-file list, and source-of-truth — that every later step and every subagent reads by path instead of re-deriving the base ref and re-running git:

# Auto-detects staged vs. branch vs. worktree. Pass the spec so the packet
# carries what the change is judged against (see source-of-truth note below).
scripts/build_review_packet.py --intent "<what this change is for>" --include <spec-or-plan-if-any>

It prints JSON with packet_path and has_source_of_truth. Use packet_path as the deterministic scope baseline for the rest of the review and for every subagent dispatch — it fixes what changed so no one re-derives it, but it is a floor, not a ceiling: keep reading surrounding code, callers, history, and tests beyond the diff as the review demands. The raw git commands below are what the script automates; reach for them directly only when scoping a review the script's scope detection does not cover.

Source of truth. If has_source_of_truth is false, the packet is a bare diff. A reviewer grading spec compliance from a bare diff will silently invent a spec and grade against it. When a spec exists, supply --intent/--include so the packet carries it; when it does not, judge only stated intent and say so — never infer a spec from the diff and grade against it.

Pre-commit (staged changes):

# List staged files
git diff --cached --name-status

# View staged diff
git --no-pager diff --cached

Pre-merge (branch vs. target):

# Resolve target base branch from origin/HEAD when available
BASE_BRANCH=$(git rev-parse --abbrev-ref origin/HEAD 2>/dev/null)
BASE_BRANCH=${BASE_BRANCH#origin/}
BASE_BRANCH=${BASE_BRANCH:-main}
MERGE_BASE=$(git merge-base "$BASE_BRANCH" HEAD)

# List changed files on this branch relative to the target base
git diff --name-status "$MERGE_BASE"...HEAD

# View full branch diff
git --no-pager diff "$MERGE_BASE"...HEAD

If the user does not specify scope, infer from context:

• On a feature branch with commits ahead of the target base branch → pre-merge review.
• Staged changes present → pre-commit review.
• Ask if ambiguous, especially when default branch detection fails.

Graph-enhanced tooling gate:

If the code-review-graph MCP plugin is available, probe and ensure freshness:

1. Call build_or_update_graph_tool() to run an incremental update.
2. Call list_graph_stats_tool() to verify the graph has nodes and check last_updated.
3. If either call fails or the graph is empty → proceed with the git-only path for the remainder of the review. Do not retry.

When the graph is available, follow references/code-review-graph-integration.md for enhanced analysis at each subsequent step. The reference doc specifies required and optional tool calls per phase, with decision criteria for when to use each.

Parallel subagent gate:

If the Agent tool is available and the change is non-trivial, plan to use the parallel-subagent path in Step 2. See references/parallel-subagent-review.md for the thresholds, dispatch pattern, and prompt templates. Otherwise continue with the single-agent path. These dispatch rules are purpose-built for code review and take precedence over any general subagent skill (e.g. subagent-patterns) when the work is a code review.

Step 1 — Triage & Context

Build a prioritized review plan before reading any code in detail.

Always (git-based):

• Review the diff to understand what changed and why.
• For medium/large or cross-module changes, write a short review plan focused on risk hotspots.

Git diagnostics (useful for unfamiliar codebases or large diffs — see references/git-diagnostics-before-review.md):

1. Check file churn and bug clusters to identify which changed files historically attract the most defects.
2. Review team composition and recent contributor activity for bus-factor context.
3. Look for firefighting patterns (reverts, hotfixes) that signal stability risks in the areas under review.

When graph is available (see references/code-review-graph-integration.md Phase 1):

• Call detect_changes_tool(base=<base>) for risk-scored, priority-ordered triage. Use its risk scores to order files highest-risk first.
• Call get_review_context_tool(base=<base>) for token-efficient structural context instead of reading entire changed files.
• Optionally call get_affected_flows_tool (change touches 2+ files), get_architecture_overview_tool (change spans 3+ directories), or get_impact_radius_tool (need raw blast-radius data) as the scope warrants.

Triage output: Produce a prioritized file list and a brief review plan identifying the highest-risk areas to focus on.

Step 2 — Structured Review (No Edits)

Pick the path set at the Step 0 parallel subagent gate.

Parallel-subagent path (when gate is active):

Follow references/parallel-subagent-review.md — fan out quality, spec-compliance, and security subagents in parallel (plus architecture when warranted), synthesize, dispatch a devil's-advocate subagent for a challenge pass, then do final synthesis and tiebreak yourself. The security subagent (Agent 4) is always dispatched; it uses the OWASP Top 10:2025 checklist from securing-code skill references. Include the architecture subagent when: pre-merge / pre-PR review, the user mentions "merge", the user explicitly requests architectural review, or you judge it necessary (confirm with the user before dispatching). Clarify author intent with the user before dispatching; subagents have no conversation history. Skip the single-agent steps below — the reference doc drives this path end-to-end through Step 3.

Single-agent path (default):

For each changed file or module, summarize what changed and why. Review tests before implementation: do tests exist for the changed behavior, do they assert behavior (not internals), do they cover edge cases and regressions? Test gaps shape what to scrutinize in the implementation pass.

Review the data structures before the logic. A structure that can hold invalid states leaves each consumer to guard against them or assume them away, and both choices generate findings elsewhere in this review; correcting the shape retires them at the source. Tests may pass either way, so make this a deliberate pass — references/data-representation.md has where to look, the three tests, and the evidence each finding must carry.

Then evaluate each area in the Quick Reference table above.

Rules:

• If intent is unclear, ask clarifying questions before judging.
• If an area has no issues, state that explicitly.
• Reference specific code locations (line numbers or quoted snippets).
• Keep feedback actionable and concise.
• Review all changed lines before final judgment; open surrounding context where needed.
• Address runtime errors, validation, error handling, concurrency risks, resource usage, and obvious performance pitfalls.
• Behavior changes without tests → raise an issue (at least Medium).
• Style comments are non-blocking unless they map to project conventions.
• Dependency manifest or lockfile changes → assess supply-chain risk and compatibility.
• Call out TODO comments and their implications.
• For security & risk: use securing-code skill's references/tier2-review.md checklist (OWASP-mapped) as the evaluation framework.

When graph is available (see references/code-review-graph-integration.md Phase 2):

• For every changed function: call query_graph_tool(pattern="tests_for", target=<function>) to flag test coverage gaps.
• For interface/signature changes: call query_graph_tool(pattern="callers_of", target=<function>) to verify consumers are updated.
• Optionally use query_graph_tool("importers_of") for module-level API changes, query_graph_tool("inheritors_of") for base class changes, find_large_functions_tool for complexity flags, semantic_search_nodes_tool to check for duplication, and refactor_tool("dead_code") to catch orphaned code.

Compile findings into issues using the template in assets/issue-template.md. Sort issues by priority: critical > high > medium > low. For each issue, include blocking status, confidence, and evidence.

Step 3 — Report

Present the review as a prioritized list of issues from Step 2.

When graph was used, append:

• Blast Radius Summary — directly changed functions/files, impacted downstream count, affected execution flows.
• Test Coverage Gaps — table of changed functions with no test coverage or insufficient coverage for new behavior.

Step 4 — Apply Changes (Only on Explicit Request)

Proceed only when the user asks to "apply changes", "fix issues", "implement suggestions", or similar.

1. Apply only changes discussed in Step 2.
2. Make minimal, targeted edits.
3. Do not introduce additional refactors without user approval.
4. After editing, summarize what changed and why, mapping back to the original review items.

Scope Note

Treat these recommendations as preferred defaults. When a default conflicts with project constraints, suggest a better-fit alternative, call out tradeoffs, and note compensating controls.

References

• assets/issue-template.md — issue type/priority legends and suggestion format.
• references/data-representation.md — the data-shape lens: where to look, three tests, and the evidence each finding must carry.
• references/review-best-practices-links.md — external review best-practice links used by this skill.
• references/git-diagnostics-before-review.md — git commands for assessing codebase health before reviewing.
• references/code-review-graph-integration.md — tool dispatch playbook for code-review-graph MCP plugin (required + optional tools per phase, decision guide).
• references/parallel-subagent-review.md — parallel-subagent dispatch path: thresholds, agent prompt templates, synthesis and red-team steps.
• securing-code skill (references/tier2-review.md, references/tier3-*.md) — OWASP-mapped security checklist used by the security subagent and single-agent security lens.