11---
22name : pr-review
3- description : Use when a feature branch is ready for code review, needs rebasing onto main, or before creating/updating a pull request. Also use when asked to review changes, check code quality, or verify a branch is merge-ready.
3+ description : >-
4+ Review code changes for quality, security, correctness, and design. Use when
5+ a feature branch is ready for review, before creating or updating a pull
6+ request, when asked to check code quality, review changes, look at a diff,
7+ or verify a branch is merge-ready. Also triggers on phrases like "review my
8+ code", "what do you think of these changes", or "is this ready to merge".
49---
510
611# PR Review
712
8- Review all changes between ` main ` and the current branch HEAD, plus any staged
9- and unstaged working-tree changes.
13+ A review-only skill. ** Do not modify any files ** — produce a report the author
14+ uses to make their own changes.
1015
11- ## Setup
16+ ## Gather context
1217
13- 1 . ** Rebase** — Unless the user says otherwise, fetch ` origin ` and rebase onto
14- ` origin/main ` , resolving conflicts. If the branch has Alembic migrations,
15- run ` alembic heads ` after rebase — if multiple heads exist, update
16- ` down_revision ` to restore a single linear history.
18+ 1 . ** Diff** — Collect all changes between ` origin/main ` and HEAD, plus any
19+ staged/unstaged working-tree changes. This is the review scope. Treat
20+ all of these changes as a single unit — assume everything will be
21+ committed before merge. Do not report on git staging status (uncommitted,
22+ unstaged, etc.) as a finding.
1723
18- 2 . ** Gather context** — If a PR exists (` gh pr view ` ):
19- - PR description, title, review comments (` gh pr view --comments ` )
20- - Linked issues (` gh issue view N ` ) to understand requirements
21- If no PR exists, review from the diff alone.
24+ 2 . ** PR metadata** — If a PR exists (` gh pr view ` ), read the description,
25+ review comments (` gh pr view --comments ` ), and linked issues
26+ (` gh issue view N ` ) to understand requirements and prior feedback.
2227
23- 3 . ** Read project docs ** — Check for ` AGENTS.md ` , ` CONTRIBUTING.md ` ,
24- ` CLAUDE.md ` . These are authoritative for test commands, linter config, and
25- conventions — use their commands exactly, not generic substitutes .
28+ 3 . ** Project conventions ** — Read ` AGENTS.md ` , ` CONTRIBUTING.md ` , or
29+ ` CLAUDE.md ` if present . These are authoritative for linter commands, test
30+ commands, and coding conventions — use their commands exactly.
2631
2732## Review checklist
2833
29- Review the diff in priority order. Fix blocking issues directly when
30- straightforward; flag issues that need human judgment.
34+ Review the diff in priority order. Report all findings for human review.
3135
3236| # | Category | Severity | Focus |
3337| ---| ----------| ----------| -------|
3438| 1 | Security | Blocking | Injection, leaked secrets, auth gaps, OWASP top 10 |
3539| 2 | Correctness | Blocking | Logic errors, edge cases, mismatch with linked issues |
36- | 3 | Test coverage | Blocking | 100% differential coverage — verify changed code has tests |
37- | 4 | Linter compliance | Blocking | Run project linters on touched files; resolve all findings |
40+ | 3 | Test coverage | Blocking | Differential coverage — verify changed code has tests |
41+ | 4 | Linter compliance | Blocking | Run project linters on touched files; report findings with exact commands |
3842| 5 | Performance | High | N+1 queries, unnecessary allocations, bottlenecks |
39- | 6 | Code quality | Medium | Redundancy, over-complexity, code smells |
40- | 7 | Consistency | Medium | Follow documented conventions; suggest undocumented ones |
41- | 8 | Alembic migrations | Conditional | Idempotence, reversibility, cross-DB compat, data safety, ` batch_alter_table ` for SQLite |
43+ | 6 | Redundancy | High | Duplicated logic, copy-paste patterns, shared-utility opportunities |
44+ | 7 | Design | High | Structural quality — see guidance below |
45+ | 8 | Consistency | Medium | Adherence to documented conventions |
46+ | 9 | Alembic migrations | Conditional | Idempotence, reversibility, cross-DB compat, ` batch_alter_table ` for SQLite |
47+
48+ ## Design review guidance
49+
50+ ### Structure and modularity
51+
52+ - ** Single-responsibility violations** — functions or classes doing more than
53+ one thing. Name what each responsibility is and suggest how to split.
54+ - ** God functions** — functions with >50 lines of logic or >3 levels of
55+ nesting. Identify extraction points.
56+ - ** Long parameter lists** — >5 parameters often indicate a missing config
57+ object or dataclass.
58+ - ** Tight coupling** — modules reaching into each other's internals. Suggest
59+ interface boundaries.
60+ - ** Deep nesting** — suggest early returns, guard clauses, or extracted helpers.
61+
62+ ### Object-oriented design and polymorphism
63+
64+ This codebase tends toward long if/elif/else chains where polymorphic dispatch
65+ would be cleaner. ** Actively look for these opportunities** in changed code and
66+ in code adjacent to changes:
67+
68+ - ** Type-switching conditionals** — e.g., `if transport == "sse": ... elif
69+ transport == "websocket": ...`. Suggest an ABC or Protocol with concrete
70+ implementations per variant.
71+ - ** Conditional behavior by enum/string** — functions branching on a type field.
72+ Suggest the Strategy or Template Method pattern.
73+ - ** Scattered object creation** — conditionals that construct different objects
74+ by type. Suggest a factory method or registry pattern.
75+ - ** Dict-dispatch** — for simpler cases where class hierarchies are overkill,
76+ ` dict[key, callable] ` dispatch tables are a good stepping stone.
77+ - ** Copy-paste behavior across classes** — suggest a ` Protocol ` (structural
78+ subtyping) or mixin.
79+ - ** Missing abstract parents** — when classes share an interface but lack a
80+ common base, suggest an ` ABC ` with ` @abstractmethod ` .
81+
82+ ### Missing abstractions
83+
84+ - ** Repeated patterns** across 3+ call sites → shared utility or base class.
85+ - ** Data bags with scattered behavior** — pure data classes whose related logic
86+ lives in other modules. The behavior should live with the data.
4287
4388## Second opinions
4489
45- After your own review, run available second-opinion tools in parallel as
46- background tasks. If a tool is missing from ` $PATH ` or fails, skip it and note
47- reduced coverage.
90+ After your own review, attempt to run these tools as background tasks. If a
91+ tool is not installed or fails, skip it and note the reduced coverage.
4892
49- - ** Codex** : ` codex exec review --base origin/main `
50- - ** Bob** : Pipe the diff inline — `git diff origin/main..HEAD | bob "Review this
51- diff for correctness, security, and code quality. Be specific about
52- line-level issues."` Tailor the prompt to the PR content.
93+ - ` codex exec review --base origin/main `
94+ - ` git diff origin/main..HEAD | bob "Review this diff for correctness, security, and design quality. Be specific about line-level issues." `
5395
54- Attribute findings to their source (Claude/Codex/Bob) and resolve contradictions.
96+ Attribute findings to their source and resolve contradictions.
5597
5698## Output format
5799
@@ -62,23 +104,28 @@ Attribute findings to their source (Claude/Codex/Bob) and resolve contradictions
62104[ 1-2 sentence overview: what changed, whether it meets PR/issue goals]
63105
64106## Findings
65- | # | Severity | Category | File: Line | Issue | Source |
66- | ---| ----------| ----------| -----------| -------| --------|
67107
68- ## Fixes Applied
69- [ Issues fixed directly, with commit refs]
108+ ### Blocking
109+ | File: Line | Category | Issue | Suggestion |
110+ | -----------| ----------| -------| ------------|
70111
71- ## Remaining Issues
72- [ Issues needing human decision or outside scope]
112+ ### High
113+ | File: Line | Category | Issue | Suggestion |
114+ | -----------| ----------| -------| ------------|
115+
116+ ### Medium
117+ | File: Line | Category | Issue | Suggestion |
118+ | -----------| ----------| -------| ------------|
73119
74120## Recommendation
75- Pick exactly ONE: Ready to merge | Ready after fixing remaining issues | Needs significant rework
121+ [ Pick exactly ONE: "Ready to merge" | "Ready after addressing findings" | "Needs significant rework"]
122+ [ 1 sentence justification]
76123```
77124
78125## Rules
79126
80- - Never mention Claude, Claude Code, or AI in commits or PR text .
81- - Never push unless explicitly asked .
82- - Sign commits with ` git commit -s ` . Verify Git author matches ` gh auth status ` .
83- - Create new commits rather than amending existing ones when rebasing or fixing.
84- - After fix-up commits, re-run linters and tests to confirm no regressions .
127+ - ** Do not modify any files. ** Report findings for the author to address .
128+ - Never mention Claude, Claude Code, or AI in any output .
129+ - Include exact linter commands and output so the author can reproduce .
130+ - Make suggestions concrete — name the method to extract, the class to create,
131+ the interface to define. "Consider refactoring" is not actionable .
0 commit comments