Created
June 4, 2026 20:07
-
-
Save pdevito3/0d48f662f5c75174ff7e1af57267189a to your computer and use it in GitHub Desktop.
pr review skill
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| --- | |
| name: pr-review | |
| description: Review code changes since a fixed point using parallel specialist sub-agents for bugs, spec fit, standards, API/domain design, performance, security, tests, and AI-generated-code risks. Use when the user wants to review a branch, PR, WIP changes, or asks to "review since X", "review this PR", or "check this diff". | |
| --- | |
| # Review | |
| Reviews the diff between `HEAD` and a fixed point using focused sub-agents, then validates findings before reporting. | |
| ## Severity Levels | |
| Each issue must use one severity: | |
| - **Blocker** — likely broken, unsafe, data-loss, security issue, or spec-critical miss. | |
| - **High** — serious correctness, maintainability, performance, or test gap likely to matter. | |
| - **Medium** — meaningful concern worth fixing before merge. | |
| - **Low** — minor cleanup or follow-up suggestion. | |
| Each issue must include: | |
| - Severity | |
| - Category | |
| - File/hunk | |
| - Evidence from diff/spec/standard | |
| - Why it matters | |
| - Suggested fix | |
| ## Process | |
| ### 1. Pin the fixed point | |
| Use the fixed point exactly as supplied: commit SHA, branch, tag, `main`, `HEAD~5`, etc. | |
| If missing, ask: | |
| > Review against what — a branch, commit, tag, or main? | |
| Capture: | |
| ```sh | |
| git diff <fixed-point>...HEAD | |
| git log <fixed-point>..HEAD --oneline | |
| git diff --name-only <fixed-point>...HEAD | |
| ``` | |
| Use three-dot diff so comparison is against the merge-base. | |
| ### 2. Gather context | |
| Find and summarize relevant context before spawning agents: | |
| #### Spec Sources | |
| In order: | |
| 1. Issue references in commits | |
| 2. User-provided spec path | |
| 3. Matching docs under `docs/`, `specs/`, `.scratch/` | |
| 4. Ask the user | |
| If no spec exists, the Spec review reports `No spec available`. | |
| #### Standards Sources | |
| Look for: | |
| - `CLAUDE.md` | |
| - `AGENTS.md` | |
| - `CONTRIBUTING.md` | |
| - `CONTEXT.md` | |
| - `STYLE*` | |
| - `STANDARDS*` | |
| - ADRs | |
| - Context-specific guidance | |
| Also note machine-enforced standards: | |
| - `.editorconfig` | |
| - lint configs | |
| - formatter configs | |
| - tsconfig/project configs | |
| Do not report issues already guaranteed by tooling. | |
| ### 3. Spawn specialist sub-agents in parallel | |
| Use separate sub-agents so findings remain independent. | |
| Each reviewer should: | |
| - Read the relevant context | |
| - Review the diff | |
| - Return only actionable findings | |
| - Include severity recommendations | |
| - Stay under 500 words | |
| #### Bugs / Correctness | |
| Look for: | |
| - Logic bugs | |
| - Edge cases | |
| - Null handling | |
| - Async issues | |
| - Concurrency issues | |
| - Race conditions | |
| - State inconsistencies | |
| - Error-handling defects | |
| - Behavior regressions | |
| - Broken invariants | |
| Focus on correctness over style. | |
| #### Spec | |
| Compare implementation to the issue, PRD, or specification. | |
| Report: | |
| - Missing requirements | |
| - Partial implementations | |
| - Scope creep | |
| - Incorrect implementations | |
| - Ambiguous interpretations | |
| Every finding must cite supporting spec evidence. | |
| #### Standards | |
| Check against documented repository standards only. | |
| For every finding: | |
| - Cite the source file | |
| - Cite the rule | |
| - Distinguish objective violations from judgment calls | |
| Skip anything already enforced automatically. | |
| #### API / Domain Design | |
| Favor: | |
| - Rich domain modeling | |
| - Clear boundaries | |
| - Explicit behavior | |
| - Functional design where appropriate | |
| - Simplicity | |
| - Maintainability | |
| Look for: | |
| - Overengineering | |
| - Anemic domain models | |
| - Leaky abstractions | |
| - Generic abstractions without clear value | |
| - Excessive indirection | |
| - Poor naming | |
| - Hidden coupling | |
| - Ownership ambiguity | |
| Prefer simpler designs unless complexity is clearly justified. | |
| #### Performance | |
| Look for: | |
| - Missing indexes | |
| - N+1 queries | |
| - Excessive allocations | |
| - Expensive loops | |
| - Repeated work | |
| - Unnecessary materialization | |
| - Repeated serialization/deserialization | |
| - Chatty network usage | |
| - Cache opportunities | |
| - Hot-path inefficiencies | |
| Consider language and platform-specific improvements such as: | |
| - Frozen collections | |
| - Immutable structures | |
| - Better lookup strategies | |
| - Streaming instead of buffering | |
| - More efficient data structures | |
| Only report issues likely to matter in practice. | |
| #### Security / Privacy | |
| Look for: | |
| - Auth/authz issues | |
| - Injection risks | |
| - Secret exposure | |
| - Sensitive logging | |
| - Tenant-boundary violations | |
| - Unsafe deserialization | |
| - SSRF | |
| - Path traversal | |
| - CSRF/CORS issues | |
| - Session weaknesses | |
| - Dangerous defaults | |
| Focus on realistic attack surfaces. | |
| #### Tests | |
| Evaluate whether behavior is adequately verified. | |
| Look for: | |
| - Missing tests | |
| - Missing regression coverage | |
| - Missing edge-case coverage | |
| - Weak assertions | |
| - Brittle tests | |
| - False confidence | |
| - Untested failure paths | |
| Treat tests as proof of behavior, not coverage metrics. | |
| #### AI Slop / Generated-Code Risk | |
| Look for signs that code was generated but insufficiently reviewed: | |
| - Boilerplate abstractions | |
| - Dead code | |
| - Unused indirection | |
| - Hallucinated APIs | |
| - Inconsistent patterns | |
| - Generic comments | |
| - Broad catch-all error handling | |
| - Fake-looking tests | |
| - Repository-pattern mismatches | |
| Do not report "looks AI-generated" as a finding by itself. | |
| Only report actual risks introduced by generated code. | |
| ### 4. Evaluation / Hallucination Check | |
| After all reviewers finish, run a final evaluator. | |
| The evaluator must: | |
| - Re-read the diff | |
| - Re-read all findings | |
| - Verify evidence | |
| - Remove unsupported claims | |
| - Merge duplicates | |
| - Normalize severity | |
| - Downgrade speculative findings | |
| Classify each finding as: | |
| - **Valid** — directly supported by evidence | |
| - **Likely Valid** — strong inference but not fully provable | |
| - **Unclear** — requires human confirmation | |
| - **Invalid** — unsupported or contradicted | |
| Remove Invalid findings entirely. | |
| Prefer fewer high-confidence findings over many speculative ones. | |
| The evaluator may not invent entirely new findings unless required to correct an obvious omission. | |
| ### 5. Final Report | |
| Group findings by severity. | |
| Use: | |
| ```md | |
| ## Summary | |
| - Blockers: X | |
| - High: Y | |
| - Medium: Z | |
| - Low: A | |
| - Worst issue: ... | |
| ## Blocker | |
| ### [Category] Short title | |
| - File: | |
| - Evidence: | |
| - Why it matters: | |
| - Suggested fix: | |
| - Confidence: | |
| ## High | |
| ... | |
| ## Medium | |
| ... | |
| ## Low | |
| ... | |
| ## Skipped | |
| - Spec unavailable | |
| - Standards unavailable | |
| ``` | |
| If there are no findings: | |
| ```md | |
| ## Summary | |
| No actionable findings identified. | |
| All reviewer findings were either resolved by implementation details, unsupported by evidence, or below the reporting threshold. | |
| ``` | |
| ## Review Principles | |
| - Correctness over cleverness. | |
| - Simplicity over abstraction. | |
| - Maintainability over flexibility. | |
| - Evidence over intuition. | |
| - High-signal findings over exhaustive findings. | |
| - Do not duplicate compiler, formatter, or linter output. | |
| - Do not block on speculative concerns. | |
| - Prefer domain-driven designs over generic frameworks. | |
| - Prefer explicit behavior over hidden magic. | |
| - Generated-looking code is not a defect unless it introduces risk. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment