Pr Review

PR Review Skill

Review Pull Requests or local changes with structured, thorough analysis.

Determine review target automatically:

• If user provides PR number or URL → Review that GitHub PR
• If no PR specified → Review local changes vs main branch (no questions asked)

Persist the review (opt-in): Do NOT write a markdown file by default. The report goes to chat output. Save the full markdown to .claude/pr-review-summaries/ ONLY when the user explicitly asks ("save the review", "write a summary file", "persist this", or equivalent). See Phase 4 for filename rules when saving.

Invoke relevant programming skills during review:

• C++ code → programming-cpp, programming-cpp-design-patterns, programming-cpp-stl-algorithms
• Python code → programming-python
• CMake files → programming-cmake-best-practices

Workspace-level overrides. Workspaces may define additional defaults in <workspace>/workflows/pr-review.md; that file overrides anything in this skill (e.g. clone locations, project routing, report destination paths).

Universal Hygiene Rules

Apply to every invocation of this skill. Full text in HYGIENE.md. Summary:

• Local-only by default: do NOT post to GitHub unless the user explicitly says "post" / "submit" / "comment on the PR".
• Fresh-eyes rule: when this skill runs inside a sub-agent, the brief is the only context - no project memory, no prior reviews, no conversation history.
• Required report sections: Header, Intent vs Implementation, Per-File Walkthrough, Findings by Severity, Static Analysis, Security Audit, Performance, API/ABI Compatibility, Documentation, Verdict (APPROVE / REQUEST CHANGES / NEEDS DISCUSSION), Cleanup Confirmation. Full layout in REPORT_TEMPLATE.md.
• Local clone hygiene: record starting branch, stash if dirty, restore on exit via trap/finally - never leave the clone on a detached HEAD or PR branch.

Review Process

Phase 0: Determine Review Target

Do NOT ask the user what to review. Determine automatically:

GitHub PR

Invoke git-gh-client to verify gh is available, then fetch via gh pr view / gh pr diff / gh pr checks.

Local changes (default)

base=$(git rev-parse --verify main 2>/dev/null && echo main || echo master)
git diff --name-only "$base"...HEAD
git diff "$base"...HEAD
git log "$base"...HEAD --oneline

Re-review mode

Diff from the last reviewed SHA (git diff <last>...HEAD or gh pr view N --json commits,reviews). Focus on NEW changes; note which prior issues were addressed.

Phase 1: Gather Information + Read Files

Collect data ONCE and package for agents - minimize redundant file reads.

GitHub PR

Fetch via git-gh-client commands: gh pr view N --json ... for metadata, gh pr diff N for the diff, gh pr checks N for CI, gh api repos/{o}/{r}/pulls/N/comments + gh pr view N --json reviews for prior comments. Note CI failures in the report; do not block review on them unless the user asks.

Local diff (default)

git diff --name-only <base>...HEAD, git diff <base>...HEAD, git log <base>...HEAD --oneline.

1.1 Read each file ONCE

Read the full body (not just changed lines) with the Read tool. Reading once here vs N times across agents = N-fold token savings.

1.2 Identify languages

1.3 Build the Data Package

One package, passed to every spawned agent. Required sections: Files Changed (path + language), Full Diff, File Contents (full body, tagged with language), Commit Messages, PR Context (title, description, author, CI status - if GitHub).

Phase 1.5: Spawn Parallel Analysis Agents

Goal: Launch up to 8 specialized agents in parallel to analyze the packaged data from Phase 1.

Lite mode gate

Before spawning all agents, check the diff scope. Agent IDs match the table in "Agent Identity & Memory System" below.

• Diff < 50 lines added/removed AND no logic changes (docs / comments / formatting / imports / type aliases only): spawn ONLY Agents 1 (static-analysis-agent) and 2 (dead-code-agent). Skip 3, 4, 5, 6, 7, 8.
• Diff < 200 lines AND affects only one file: spawn Agents 1, 2, 3 (code-smells-agent), 4 (language-rules-agent), 7 (performance-agent), 8 (ub-detection-agent if a C/C++/unsafe-Rust file changed). Skip 5 (architecture) and 6 (simplify).
• Otherwise: full 8-agent fan-out (Agent 5 still gated by the architectural-signal table below; Agent 8 still gated by the UB-trigger rule below).

UB-agent trigger (Agent 8). Spawn Agent 8 ONLY when at least one changed file matches *.c, *.cc, *.cpp, *.cxx, *.h, *.hpp, *.hxx, *.inl, *.ipp, *.tpp, OR contains unsafe { (Rust). Skip otherwise (pure Python / CMake / docs / shell diffs do not exercise UB classes).

Document the chosen mode and the spawned agent IDs in the final report's Header section.

Agent Identity & Memory System

Each agent has:

• Unique ID: Used in Agent tool description field for identification
• Skill to Load: Agent invokes this skill using the Skill tool before analysis
• Memory File: Project-specific learnings persisted across reviews

Memory location: ~/.claude/projects/<project>/memory/agents/

The 8 Analysis Agents (purpose + memory topic)

Agent Execution Pattern

Spawn all selected agents in parallel using the Agent tool (one tool block, multiple Agent calls). For each spawn:

• description: the agent ID from the table in "Agent Identity & Memory System"
• subagent_type: general-purpose
• prompt body: contents of the matching file under agents/prompts/ (see "Agent Prompt Templates" below) + the Data Package from Phase 1

Agent 5 only spawns when the architectural-signal table matches. Agent 8 only spawns when the UB-trigger rule above matches. Lite mode (above) further trims the set.

Output discipline (applies to EVERY agent)

Line-number discipline. NEVER cite a line number you have not actually read in the file body. Before emitting path:line, verify:

1. The file content at that line was part of the Data Package passed to you in Phase 1.
2. The line number is within the file's actual length. A finding citing line 660 in a 222-line file is fabricated; the underlying issue may be real but the citation is unactionable and erodes reviewer trust.
3. If you remember the function/class name but not the exact line, cite path:<function-name> or path:<line-N..line-M> covering the function's known range. Omit the specific line rather than guess.

Fabricated line numbers are WORSE than missing line numbers. The reviewer who follows a citation to line 660 of a 222-line file loses trust in every other finding from the agent.

Class-tag discipline. The Class / Issue Type column must use the agent's own vocabulary (UB:*, Perf:*, Lang:*, Smell:*, Dim N:*, Dead:*, Comment:*, Test:*, CMake:*, Arch:*, Simplify:*, Static:*). Do not tag a missing-virtual-destructor finding as Dead:* or a catch(...) as Comment:*.

Completeness discipline. When a checklist item says MUST flag, the agent emits ONE row per offending site - never collapses multiple violations of the same class into a single representative finding. Five using namespace std; instances in five files = five rows.

Conditional Architecture Analysis

Only run Architecture Agent if ANY of these signals present:

If no architectural signals → Skip Agent 5, run Agents 1-4, 6, 7, and 8 (8 only when its UB-trigger rule matches).

Agent Prompt Templates

Each agent's full prompt lives in its own file under agents/prompts/. The orchestrator passes the file's contents + Data Package to the spawned agent.

Architecture Agent fires only when the architectural-signal table below matches. UB Detection Agent fires only when the UB-trigger rule above matches. All others fire per the lite-mode gate.

Phase 2: Aggregate Agent Findings

Wait for all spawned agents from Phase 1.5. Then:

2.1 Severity scale (used by every agent)

UB never downgrades. Any finding from the UB Detection Agent defaults to Critical (100). Drop to Must Fix (80) only when the code path is provably unreachable on every target platform (documented with citation). Never Should Fix or below.

2.2 Merge

Collect every issue. Group by severity (Critical -> Must Fix -> Should Fix -> Nitpick). Sort within each group by file path then line number. Tag each finding with the source agent ID.

2.3 Deduplicate

• Same file:line, same issue -> keep highest severity, merge descriptions
• Same file:line, different issues -> keep both
• Different agents, same general category, different specifics -> keep both

2.4 Classify change type

Feature / Bugfix / Refactor / Docs - put it in the report header.

Phase 3: Review Tests (Manual Check)

3.1 Test Coverage Check

3.2 Suggest Missing Tests

If new code lacks tests, list per-function the cases to add. Checklist:

• Happy path
• Empty/null input
• Invalid input
• Boundary values
• Error handling

Use the project's test framework (GTest/GMock for C++, pytest for Python) and follow the naming convention found in nearby tests.

3.3 Cross-File Consistency (Optional)

If agents missed cross-cutting concerns, manually check:

Phase 4: Generate Final Report

Compile aggregated findings from all phases into a comprehensive review.

Save final report to disk (opt-in)

Default behaviour: do NOT write a markdown file. The report is delivered as chat output. Saving to disk happens ONLY when the user explicitly asks ("save the review", "write a summary file to disk", "persist this report", "drop a markdown under the project", or equivalent).

When the user has asked, persist the report under the git repository root of the project being reviewed.

Finding <repo-root>: Run git rev-parse --show-toplevel from the project you are reviewing (works when the current working directory is anywhere inside that clone). If the reviewed tree is not a git work tree, fall back to the workspace root you were given for that review.

Paths in the user message: Prefer a path relative to <repo-root> (e.g. .claude/pr-review-summaries/123-fix-bug.md) so it is copy-paste friendly across machines. Use an absolute path only if the user context has no single repo root.

Filename

• GitHub PR review: <pr-number>-<pr-title-slug>.md
  • pr-number: the PR number (digits only, no #).
  • pr-title-slug: slug derived from the PR title — lowercase, replace spaces and punctuation with single hyphens, strip leading/trailing hyphens, ASCII only; collapse repeated hyphens; max 60 characters so paths stay reasonable. If the title slug is empty, use review.
• Local review (no PR): local-<branch-slug>-<short-slug>.md
  • branch-slug: current branch name slugified the same way (max 40 chars), or detached if not on a branch.
  • short-slug: from the first line of git log -1 --pretty=%s (slugified, max 40 chars), or changes if unavailable.

File contents: Write the entire Phase 4 report (the full markdown document), not an excerpt. Use the Write tool or equivalent so the file is created or overwritten idempotently for that review run.

User message: After saving, briefly state the path to the saved file (e.g. .claude/pr-review-summaries/123-fix-parser-null-handling.md).

Report must include the sections marked [REQUIRED] in REPORT_TEMPLATE.md. See Universal Hygiene Rules > Report content rules for the full list and rationale.

Report template

See REPORT_TEMPLATE.md in this skill directory for the full report structure. The template covers Header, Summary, Intent vs Implementation, Per-File Walkthrough, Agent Analysis, Architecture, Issues by Severity, Test Coverage, Static Analysis, Security, Performance, API/ABI, Docs, Cleanup, Previous Comments, Checklist, Questions.

When generating a review, copy the template structure and fill in evidence per section.

Issue Categories

Must Fix (Blocking)

Issues that must be resolved before merge:

• Bugs / incorrect behavior
• Security vulnerabilities
• Missing critical tests
• Breaking changes without migration
• Violations of core guidelines

Should Fix (Non-blocking)

Issues worth addressing but not blocking:

• Minor best practice violations
• Missing edge case handling
• Suboptimal performance
• Missing non-critical tests
• Minor code style issues

Nitpicks (Optional)

Suggestions for improvement:

• Naming preferences
• Code organization
• Documentation improvements
• Alternative approaches

Providing Feedback

Be Constructive

Good:

"Consider using std::find_if here instead of the manual loop - it's more expressive and less error-prone."

Bad:

"This loop is wrong."

Be Specific

Good:

"In parser.cpp:45, the error case when input.empty() isn't handled. Consider returning std::nullopt or throwing."

Bad:

"Error handling is missing."

Explain Why

Good:

"Using auto here hides the type and makes the code harder to understand. Since this is a public API, explicit types improve readability."

Bad:

"Don't use auto."

Suggest Solutions

Good:

"This could cause a race condition if called from multiple threads. Consider using std::mutex or making this thread-local."

Bad:

"Not thread-safe."

Integration with Other Skills

Common Review Mistakes

Quick Review Checklist

For fast reviews, at minimum check:

Per-file:

• Read the full diff (don't skim)
• const used appropriately?
• Naming clear and consistent?
• Error handling complete?
• No obvious bugs or edge case misses?

Overall:

• All files reviewed (not just glanced at)?
• Tests exist for new code?
• No security red flags?
• Follows programming skill rules (not existing bad patterns)?
• If user asked to save the report: persisted under .claude/pr-review-summaries/ (see Phase 4); otherwise chat-only delivery is the default.

Agent Memory File Format

Each agent's memory file follows this structure:

# [Agent Name] Memory

## Project: [auto-detected]
Last updated: [date]

## Learned Patterns
<!-- Patterns confirmed across multiple reviews -->

## False Positives
<!-- Issues flagged that turned out to be intentional -->

## Project-Specific Rules
<!-- Deviations from defaults that are acceptable -->

## Key Knowledge
<!-- For Architecture Agent: module map, interfaces, dependencies -->
<!-- For others: important context about this codebase -->

References

• Google Code Review Guidelines
• How to Do Code Reviews Like a Human