pr-review

# PR Review Skill Review Pull Requests or local changes with structured, thorough analysis. <IMPORTANT> **BE THOROUGH AND PICKY:** - Review ENTIRE changed files, not just changed lines - Report ALL issues found - do not skip or filter anything - Apply programming skill rules strictly - Check every function, class, and code block in changed files **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` </IMPORTANT> > **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](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 | Purpose | |-------|---------| | 0. Determine target | PR# / URL -> GitHub PR; nothing -> local diff vs main; "re-review" -> changes since last review | | 1. Gather + read | Changed files, full diff, commit messages, CI status; read each file ONCE; build Data Package | | 1.5. Spawn 8 agents in parallel | Static, Dead Code, Code Smells + Quality, Language Rules, Architecture (conditional), Simplify, Performance, UB Detection (C/C++/unsafe-Rust only) | | 2. Aggregate | Merge findings, map to severity, deduplicate, sort, classify change type | | 3. Review tests | Coverage, edge cases, naming, independence, assertions; suggest missing tests | | 4. Final report | Severity-sorted, agent-sourced, with code fixes; chat-only by default, save .md only if asked | ## Phase 0: Determine Review Target **Do NOT ask the user what to review. Determine automatically:** | User Input | Action | |------------|--------| | PR number (e.g., `123`, `#123`) | Review GitHub PR #123 | | PR URL (e.g., `github.com/.../pull/123`) | Review that GitHub PR | | "re-review" or "review again" | Re-review mode (show only new changes) | | Nothing / just "review" | Review local changes vs main branch | ### 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) ```bash 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 | Extension | Language | Used by | |-----------|----------|---------| | `.cpp` / `.hpp` / `.h` / `.cc` | C++ | language-rules-agent, code-smells-agent | | `.py` | Python | language-rules-agent, code-smells-agent | | `CMakeLists.txt` / `.cmake` | CMake | language-rules-agent | ### 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. <IMPORTANT> **Use general-purpose agents** (not Explore agents) since they receive pre-loaded context. All agents run in parallel - invoke all of them in a single tool call block. Each agent has a unique identity, loads its skill, and maintains memory. </IMPORTANT> ### 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 | # | Agent ID | Skill to Load | Memory File | |---|----------|---------------|-------------| | 1 | `static-analysis-agent` | `static-analysis` | `agents/static-analysis.md` | | 2 | `dead-code-agent` | *(none)* | `agents/dead-code.md` | | 3 | `code-smells-agent` | `code-smells` + load `QUALITY.md` from this skill | `agents/code-smells.md` | | 4 | `language-rules-agent` | `programming-cpp` or `programming-python` | `agents/language-rules.md` | | 5 | `architecture-agent` | `architecture-analyze` | `agents/architecture.md` | | 6 | `simplify-agent` | `simplify` | `agents/simplify.md` | | 7 | `performance-agent` | *(none, loads `PERFORMANCE.md` from this skill)* | `agents/performance.md` | | 8 | `ub-detection-agent` | `programming-cpp` (for C++ Core Guidelines lifetime / type-safety rules) | `agents/ub-detection.md` | **Memory location:** `~/.claude/projects/<project>/memory/agents/` ### The 8 Analysis Agents (purpose + memory topic) | # | Agent ID | Purpose | Memory topic | |---|----------|---------|--------------| | 1 | `static-analysis-agent` | Run linters/tools (clang-tidy, ruff, etc.) on full file contents | Tool configs, false positives, suppressions | | 2 | `dead-code-agent` | Unused code, comments, unreachable code, comment hygiene | Intentionally-unused code, reserved APIs | | 3 | `code-smells-agent` | 22 smells + 4 quality dimensions per `QUALITY.md` | Project thresholds, quality-dim overrides | | 4 | `language-rules-agent` | C++ / Python / CMake best practices per `programming-*` skills | Project conventions, intentional deviations | | 5 | `architecture-agent` | Module boundaries, dependencies, testability (conditional - see signal table) | Module map, interfaces, dependency patterns | | 6 | `simplify-agent` | Reuse, unnecessary complexity, verbose patterns per `simplify` skill | Project utilities, intentional verbosity | | 7 | `performance-agent` | Hot-path classification + alloc/complexity/locks/IO per `PERFORMANCE.md` | Hot files, accepted alloc patterns, benchmarks | | 8 | `ub-detection-agent` | Hunts undefined behaviour in C/C++/unsafe-Rust (signed overflow, lifetime, strict aliasing, data races, alignment, null deref, sanitizer-coverage check) | Sanitizer wiring in CI, vetted `reinterpret_cast` / `union` / `bit_cast` sites, confirmed-safe `unsafe` blocks | ### 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:** | Signal | Indicates Architecture | |--------|------------------------| | New directories created | New module/component | | New/modified interfaces or abstract classes | API boundaries changing | | Changes to factories, DI, object creation | Dependency structure changing | | New CMake targets (`add_library`, `add_executable`) | New build units | | Changes across 5+ files in different modules | Cross-cutting change | | New external dependencies | Integration points | | Changes to base/core classes | Foundation shifting | 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. | Agent ID | Prompt file | |----------|-------------| | `static-analysis-agent` | [agents/prompts/agent-1-static-analysis.md](agents/prompts/agent-1-static-analysis.md) | | `dead-code-agent` | [agents/prompts/agent-2-dead-code.md](agents/prompts/agent-2-dead-code.md) | | `code-smells-agent` | [agents/prompts/agent-3-code-smells-quality.md](agents/prompts/agent-3-code-smells-quality.md) | | `language-rules-agent` | [agents/prompts/agent-4-language-rules.md](agents/prompts/agent-4-language-rules.md) | | `architecture-agent` | [agents/prompts/agent-5-architecture.md](agents/prompts/agent-5-architecture.md) |