|
| 1 | +--- |
| 2 | +description: Comprehensively review pull requests following Pattern Stack standards with TDD validation |
| 3 | +argument-hint: [pr-number] or leave blank to review current branch's PR |
| 4 | +allowed-tools: Task, Bash, Read, MultiEdit, Glob, Grep, WebFetch |
| 5 | +--- |
| 6 | + |
| 7 | +# Pattern Stack PR Review Process |
| 8 | + |
| 9 | +## Reference Context |
| 10 | +First, load these key files to understand the project context: |
| 11 | +- @CLAUDE.md - Project overview and development commands |
| 12 | +- @.claude/commands/plan/refine-issue.md - How we refine and plan issues |
| 13 | +- @Makefile - Available test and validation commands |
| 14 | +- @src/cli_patterns/core/ - Core type definitions and protocols |
| 15 | + |
| 16 | +For backend-patterns reviews, also load: |
| 17 | +- @../backend-patterns/CLAUDE.md - Backend patterns architecture |
| 18 | +- @../backend-patterns/.claude/commands/atomic-commit-workflow.md - Commit standards |
| 19 | + |
| 20 | +You are conducting a comprehensive pull request review for the Pattern Stack ecosystem. This review process ensures code quality, architectural compliance, and alignment with our development methodology. |
| 21 | + |
| 22 | +## Phase 1: Context Discovery & Analysis |
| 23 | + |
| 24 | +### Step 1.1: Identify the PR |
| 25 | +$ARGUMENTS |
| 26 | + |
| 27 | +If a PR number was provided, fetch details: |
| 28 | +```bash |
| 29 | +gh pr view $1 |
| 30 | +``` |
| 31 | + |
| 32 | +If no PR specified, find current branch's PR: |
| 33 | +```bash |
| 34 | +gh pr status |
| 35 | +``` |
| 36 | + |
| 37 | +### Step 1.2: Understand the Task Context |
| 38 | +Use task patterns to understand what was implemented: |
| 39 | +```bash |
| 40 | +# Get the Linear issue details from PR description or branch name |
| 41 | +tp show [issue-id] |
| 42 | +``` |
| 43 | + |
| 44 | +Also review: |
| 45 | +- @cli-patterns-docs/adrs/ - Any relevant ADRs for architectural decisions |
| 46 | +- @tests/ - Test structure to understand testing patterns |
| 47 | + |
| 48 | +Look for: |
| 49 | +- Original requirements and acceptance criteria |
| 50 | +- Related issues in the hierarchy (parent epics, dependencies) |
| 51 | +- Comments showing refinement decisions |
| 52 | +- Any ADRs referenced |
| 53 | + |
| 54 | +### Step 1.3: Fetch and Analyze Changes |
| 55 | +Launch a subagent to comprehensively analyze the changes: |
| 56 | + |
| 57 | +**Task Tool Usage:** |
| 58 | +``` |
| 59 | +Task: "Analyze PR changes comprehensively" |
| 60 | +Prompt: " |
| 61 | +1. Use 'gh pr diff [pr-number]' to get the full diff |
| 62 | +2. Identify all files changed and categorize them by: |
| 63 | + - Layer (for backend-patterns: atoms/features/molecules/organisms) |
| 64 | + - Component (for cli-patterns: core/execution/ui/parser) |
| 65 | + - Type (implementation/tests/docs) |
| 66 | +3. Create a change summary including: |
| 67 | + - Total lines added/removed |
| 68 | + - Test file ratio (test files vs implementation files) |
| 69 | + - Component boundaries crossed |
| 70 | +4. Check commit history with 'git log --oneline [base]..HEAD' |
| 71 | +5. Verify atomic commit practices |
| 72 | +" |
| 73 | +Subagent: general-purpose |
| 74 | +``` |
| 75 | + |
| 76 | +## Phase 2: Multi-Dimensional Review |
| 77 | + |
| 78 | +### Step 2.1: Architecture Compliance Review |
| 79 | +Launch architecture review agent: |
| 80 | + |
| 81 | +**Task Tool Usage:** |
| 82 | +``` |
| 83 | +Task: "Review architectural compliance" |
| 84 | +Prompt: " |
| 85 | +Review the PR for Pattern Stack architectural compliance: |
| 86 | +
|
| 87 | +FOR BACKEND-PATTERNS: |
| 88 | +1. Verify Atomic Architecture v2.1 compliance: |
| 89 | + - Unidirectional dependencies (Organisms → Molecules → Features → Atoms) |
| 90 | + - Features handle ONE model only |
| 91 | + - No cross-feature imports |
| 92 | + - Clean boundaries (no business logic in models, no permissions in services) |
| 93 | +2. Check import directions using 'make validate' if available |
| 94 | +3. Verify infrastructure subsystem usage (cache, rate_limit patterns) |
| 95 | +
|
| 96 | +FOR CLI-PATTERNS: |
| 97 | +1. Verify protocol-based architecture: |
| 98 | + - Review @src/cli_patterns/core/types.py for semantic types (BranchId, ActionId, OptionKey) |
| 99 | + - Check @src/cli_patterns/core/protocols.py for protocol contracts |
| 100 | + - Stateless execution maintained |
| 101 | +2. Check MyPy strict mode compliance with 'make type-check' |
| 102 | +3. Verify design token usage - review @src/cli_patterns/ui/design/tokens.py |
| 103 | +
|
| 104 | +Report any architectural violations found. |
| 105 | +" |
| 106 | +Subagent: general-purpose |
| 107 | +``` |
| 108 | + |
| 109 | +### Step 2.2: Test Coverage & TDD Validation |
| 110 | +Launch test validation agent: |
| 111 | + |
| 112 | +**Task Tool Usage:** |
| 113 | +``` |
| 114 | +Task: "Validate test coverage and TDD practices" |
| 115 | +Prompt: " |
| 116 | +1. Run full test suite: 'make test' |
| 117 | +2. Check coverage: 'make test-coverage' |
| 118 | +3. Verify 80% minimum coverage for backend-patterns |
| 119 | +4. For each implementation file, verify corresponding test file exists |
| 120 | +5. Run tests in isolation to verify no interdependencies: |
| 121 | + - Unit tests: 'make test-unit' |
| 122 | + - Integration tests: 'make test-integration' |
| 123 | +6. Check for: |
| 124 | + - Proper use of test markers (@pytest.mark.unit, @pytest.mark.integration) |
| 125 | + - Test fixtures and factories usage |
| 126 | + - Edge case coverage |
| 127 | + - Performance benchmarks where applicable |
| 128 | +7. Verify tests were written BEFORE implementation (check commit order) |
| 129 | +" |
| 130 | +Subagent: general-purpose |
| 131 | +``` |
| 132 | + |
| 133 | +### Step 2.3: Code Quality & Standards Review |
| 134 | +Launch code quality agent: |
| 135 | + |
| 136 | +**Task Tool Usage:** |
| 137 | +``` |
| 138 | +Task: "Review code quality and standards" |
| 139 | +Prompt: " |
| 140 | +Perform comprehensive code quality checks: |
| 141 | +
|
| 142 | +1. Linting: 'make lint' or 'make ci' |
| 143 | +2. Formatting: Verify 'make format' produces no changes |
| 144 | +3. Type checking: 'make typecheck' passes without errors |
| 145 | +4. For Python code verify: |
| 146 | + - Type hints on all functions |
| 147 | + - Async-first design where appropriate |
| 148 | + - Proper error handling with structured exceptions |
| 149 | + - No hardcoded secrets or credentials |
| 150 | +5. For TypeScript/JavaScript: |
| 151 | + - Proper typing (no 'any' without justification) |
| 152 | + - Consistent import style |
| 153 | +6. Check documentation: |
| 154 | + - Docstrings for public APIs |
| 155 | + - Updated README if new features added |
| 156 | + - CLAUDE.md updates if development patterns changed |
| 157 | +" |
| 158 | +Subagent: general-purpose |
| 159 | +``` |
| 160 | + |
| 161 | +### Step 2.4: Pattern Stack Integration Opportunities |
| 162 | +Launch pattern enhancement agent: |
| 163 | + |
| 164 | +**Task Tool Usage:** |
| 165 | +``` |
| 166 | +Task: "Identify Pattern Stack enhancement opportunities" |
| 167 | +Prompt: " |
| 168 | +Review the code for opportunities to enhance with Pattern Stack packages: |
| 169 | +
|
| 170 | +1. Check if any implemented patterns could use: |
| 171 | + - pattern_stack.atoms.cache for caching needs |
| 172 | + - pattern_stack.atoms.security for auth/JWT |
| 173 | + - pattern_stack.atoms.validators for field validation |
| 174 | + - pattern_stack.core.loader for auto-discovery |
| 175 | + - pattern_stack.core.registry for component tracking |
| 176 | +
|
| 177 | +2. For CLI implementations, check if they could benefit from: |
| 178 | + - Composable parser system for command handling |
| 179 | + - Design token system for consistent theming |
| 180 | + - Protocol-based implementations for extensibility |
| 181 | +
|
| 182 | +3. Identify any code that reimplements existing Pattern Stack functionality |
| 183 | +
|
| 184 | +4. Look for opportunities to extract reusable patterns into: |
| 185 | + - Atoms (domain-agnostic utilities) |
| 186 | + - Features (data services) |
| 187 | + - Molecules (domain entities) |
| 188 | +
|
| 189 | +Report enhancement opportunities without being prescriptive. |
| 190 | +" |
| 191 | +Subagent: general-purpose |
| 192 | +``` |
| 193 | + |
| 194 | +### Step 2.5: Security & Performance Review |
| 195 | +Launch security and performance agent: |
| 196 | + |
| 197 | +**Task Tool Usage:** |
| 198 | +``` |
| 199 | +Task: "Review security and performance implications" |
| 200 | +Prompt: " |
| 201 | +Conduct security and performance review: |
| 202 | +
|
| 203 | +SECURITY: |
| 204 | +1. Check for exposed credentials or API keys |
| 205 | +2. Verify proper input validation and sanitization |
| 206 | +3. Review authentication/authorization logic |
| 207 | +4. Check for SQL injection vulnerabilities |
| 208 | +5. Verify secure password handling (if applicable) |
| 209 | +6. Check for proper error messages (no stack traces to users) |
| 210 | +
|
| 211 | +PERFORMANCE: |
| 212 | +1. Look for N+1 query problems |
| 213 | +2. Check for unnecessary database calls |
| 214 | +3. Verify proper use of caching where appropriate |
| 215 | +4. Check for memory leaks or unbounded growth |
| 216 | +5. Review async/await usage for I/O operations |
| 217 | +6. Run benchmarks if available: 'make test-benchmark' |
| 218 | +
|
| 219 | +Report any concerns found. |
| 220 | +" |
| 221 | +Subagent: general-purpose |
| 222 | +``` |
| 223 | + |
| 224 | +## Phase 3: Atomic Commit Validation |
| 225 | + |
| 226 | +### Step 3.1: Review Commit Granularity |
| 227 | +Analyze the commit history for atomic commit practices: |
| 228 | + |
| 229 | +```bash |
| 230 | +# Get detailed commit history |
| 231 | +git log --stat [base-branch]..HEAD |
| 232 | + |
| 233 | +# Check each commit builds independently |
| 234 | +for commit in $(git rev-list [base-branch]..HEAD); do |
| 235 | + git checkout $commit |
| 236 | + make test # or appropriate test command |
| 237 | +done |
| 238 | +``` |
| 239 | + |
| 240 | +Verify: |
| 241 | +- Each commit represents ONE logical change |
| 242 | +- Commits maintain working state (build and tests pass) |
| 243 | +- Proper commit message format: `type(scope): description (ticket)` |
| 244 | +- No "WIP" or "fix typo" commits (should be squashed) |
| 245 | + |
| 246 | +## Phase 4: Collaborative Feedback |
| 247 | + |
| 248 | +### Step 4.1: Synthesize Findings |
| 249 | +Compile all agent findings into structured feedback: |
| 250 | + |
| 251 | +1. **Critical Issues** (must fix before merge) |
| 252 | + - Architectural violations |
| 253 | + - Failing tests or insufficient coverage |
| 254 | + - Security vulnerabilities |
| 255 | + - Breaking changes without migration path |
| 256 | + |
| 257 | +2. **Important Suggestions** (should address) |
| 258 | + - Code quality improvements |
| 259 | + - Performance optimizations |
| 260 | + - Missing documentation |
| 261 | + - Pattern Stack enhancement opportunities |
| 262 | + |
| 263 | +3. **Minor Notes** (nice to have) |
| 264 | + - Style improvements |
| 265 | + - Refactoring opportunities |
| 266 | + - Additional test cases |
| 267 | + |
| 268 | +### Step 4.2: Create Review Comments |
| 269 | +Based on the findings, create constructive feedback: |
| 270 | + |
| 271 | +```markdown |
| 272 | +## PR Review for #[PR-NUMBER]: [PR Title] |
| 273 | + |
| 274 | +### ✅ Strengths |
| 275 | +- [What was done well] |
| 276 | +- [Good patterns observed] |
| 277 | + |
| 278 | +### 🔴 Critical Issues |
| 279 | +[List critical issues that block merge] |
| 280 | + |
| 281 | +### 🟡 Suggestions for Improvement |
| 282 | +[List important but non-blocking items] |
| 283 | + |
| 284 | +### 💡 Pattern Stack Opportunities |
| 285 | +[Opportunities to leverage Pattern Stack packages] |
| 286 | + |
| 287 | +### 📊 Metrics |
| 288 | +- Test Coverage: X% |
| 289 | +- Files Changed: X |
| 290 | +- Tests Added: X |
| 291 | +- Atomic Commits: X/Y follow standards |
| 292 | + |
| 293 | +### 📝 Detailed Feedback |
| 294 | +[Specific file-level comments] |
| 295 | +``` |
| 296 | + |
| 297 | +### Step 4.3: Provide Actionable Next Steps |
| 298 | +Create a clear action plan: |
| 299 | + |
| 300 | +```markdown |
| 301 | +## Recommended Actions |
| 302 | + |
| 303 | +1. **Immediate fixes needed:** |
| 304 | + - [ ] Fix failing test in test_parser.py |
| 305 | + - [ ] Add missing type hints in executor.py |
| 306 | + - [ ] Update coverage to meet 80% minimum |
| 307 | + |
| 308 | +2. **Before final approval:** |
| 309 | + - [ ] Add integration tests for new parser pipeline |
| 310 | + - [ ] Update CLAUDE.md with new development pattern |
| 311 | + - [ ] Consider extracting validation logic to atoms layer |
| 312 | + |
| 313 | +3. **Future improvements (create tickets):** |
| 314 | + - [ ] Implement caching for expensive operations |
| 315 | + - [ ] Add performance benchmarks |
| 316 | +``` |
| 317 | + |
| 318 | +## Phase 5: Documentation & Learning |
| 319 | + |
| 320 | +### Step 5.1: Update Project Knowledge |
| 321 | +If the review revealed important patterns or decisions: |
| 322 | + |
| 323 | +1. Create or update ADRs for architectural decisions |
| 324 | +2. Update CLAUDE.md with new patterns or anti-patterns discovered |
| 325 | +3. Add examples to documentation if novel solutions were implemented |
| 326 | + |
| 327 | +### Step 5.2: Track Review Patterns |
| 328 | +Document recurring issues for team improvement: |
| 329 | + |
| 330 | +```bash |
| 331 | +# Add comment to Linear ticket about review findings |
| 332 | +tp comment [issue-id] "PR Review completed. Key findings: ..." |
| 333 | +``` |
| 334 | + |
| 335 | +## Important Review Principles |
| 336 | + |
| 337 | +### Be Constructive |
| 338 | +- Acknowledge good work before criticizing |
| 339 | +- Provide specific examples and solutions |
| 340 | +- Explain the "why" behind suggestions |
| 341 | + |
| 342 | +### Consider Context |
| 343 | +- Understand the task requirements from Linear |
| 344 | +- Review previous refinement decisions |
| 345 | +- Consider timeline and scope constraints |
| 346 | + |
| 347 | +### Think System-Wide |
| 348 | +- How does this change affect other components? |
| 349 | +- Does it maintain backward compatibility? |
| 350 | +- Will it scale with future requirements? |
| 351 | + |
| 352 | +### Foster Learning |
| 353 | +- Explain Pattern Stack patterns when suggesting changes |
| 354 | +- Share relevant documentation links |
| 355 | +- Provide code examples for complex suggestions |
| 356 | + |
| 357 | +## Review Checklist |
| 358 | + |
| 359 | +Before completing the review, ensure: |
| 360 | +- [ ] All tests pass locally |
| 361 | +- [ ] Architecture compliance verified |
| 362 | +- [ ] Test coverage meets requirements |
| 363 | +- [ ] Code quality checks pass |
| 364 | +- [ ] Security implications considered |
| 365 | +- [ ] Performance impact assessed |
| 366 | +- [ ] Documentation updated as needed |
| 367 | +- [ ] Commit history follows atomic practices |
| 368 | +- [ ] Pattern Stack opportunities identified |
| 369 | +- [ ] Linear ticket will be updated with findings |
| 370 | + |
| 371 | +## Completion |
| 372 | + |
| 373 | +After review: |
| 374 | +1. Post review comments on GitHub PR |
| 375 | +2. Update Linear ticket with review summary |
| 376 | +3. Set appropriate PR labels (needs-work, approved, etc.) |
| 377 | +4. Notify PR author of review completion |
| 378 | + |
| 379 | +Remember: The goal is not just to find problems, but to ensure the code advances Pattern Stack's goals of composability, type safety, and maintainability while helping the team grow and improve. |
0 commit comments