Prepare Pull Request Skill

Use this skill when planning and preparing Pull Requests. For reviewing PRs, use pr-review instead.

This skill includes a mandatory verification phase that:

• Checks code against programming standards
• Finds unused code, TODOs, and cleanup opportunities
• Identifies code duplication and improvement opportunities
• Validates architecture and design patterns

Invoke relevant programming skills for verification:

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

ALSO invoke git-gh-client to verify GitHub CLI is available for PR creation. </IMPORTANT>

PR Preparation Process

┌─────────────────────────────────────────────────────────────────┐
│                   User asks to prepare PR                        │
└─────────────────────────────────────────────────────────────────┘
                                │
                                ▼
                    ┌───────────────────────┐
                    │ Phase 0: Verify gh    │
                    │ Invoke: git-gh-client │
                    └───────────────────────┘
                                │
                                ▼
                    ┌───────────────────────┐
                    │ Phase 1: Gather       │
                    │ - Get changed files   │
                    │ - Identify languages  │
                    │ - Load relevant skills│
                    └───────────────────────┘
                                │
                                ▼
                    ┌───────────────────────┐
                    │ Phase 2: Code Review  │
                    │ - Standards check     │
                    │ - Unused code/TODOs   │
                    │ - Duplication check   │
                    │ - Architecture review │
                    └───────────────────────┘
                                │
                                ▼
                    ┌───────────────────────┐
                    │ Phase 3: Report       │
                    │ - Present findings    │
                    │ - Ask user decisions  │
                    │ - Apply fixes         │
                    └───────────────────────┘
                                │
                                ▼
                    ┌───────────────────────┐
                    │ Phase 4: Size Check   │
                    │ - Count lines changed │
                    │ - Split if needed     │
                    └───────────────────────┘
                                │
                                ▼
                    ┌───────────────────────┐
                    │ Phase 5: Create PR    │
                    │ - Write description   │
                    │ - Push and submit     │
                    └───────────────────────┘

Phase 0: Verify GitHub CLI

FIRST ACTION: Invoke git-gh-client skill

This ensures gh CLI is installed and authenticated for PR creation. If not available, git-gh-client provides installation instructions.

Phase 1: Gather Information

Get Changed Files

# Get list of changed files
git diff --name-only HEAD~1..HEAD  # or vs main branch
git diff --stat

# Identify file types
git diff --name-only | xargs -I{} basename {} | sed 's/.*\.//' | sort | uniq -c

Load Programming Skills

Based on file types, invoke relevant skills:

Phase 2: Code Verification

2.1 Standards Compliance Check

For each changed file, verify against loaded programming skills:

C++ Checks:

• Follows C++ Core Guidelines
• Uses modern C++17 features appropriately
• Const correctness (const, constexpr where applicable)
• Proper use of noexcept, [[nodiscard]]
• RAII for resource management
• No raw new/delete (use smart pointers)
• Proper error handling

Python Checks:

• PEP 8 compliance
• Type hints on all functions
• Docstrings where needed
• No bare except: clauses
• Proper use of context managers

CMake Checks:

• Target-based approach
• Proper visibility keywords
• No deprecated commands

2.2 Unused Code and TODOs

Search for and report:

# Find TODOs in changed files
git diff --name-only | xargs grep -n "TODO\|FIXME\|XXX\|HACK" 2>/dev/null

# Find commented-out code (heuristic)
git diff --name-only | xargs grep -n "^[[:space:]]*//.*[;{}]" 2>/dev/null

For each TODO/FIXME found, ask user:

Use AskUserQuestion tool:

{
  "questions": [{
    "question": "Found TODO in file.cpp:42: 'TODO: implement error handling'. What should we do?",
    "header": "TODO found",
    "options": [
      {"label": "Fix now", "description": "Implement the TODO before creating PR"},
      {"label": "Remove", "description": "Delete the TODO comment (if no longer needed)"},
      {"label": "Keep", "description": "Leave as-is, will address in future PR"},
      {"label": "Convert to issue", "description": "Create GitHub issue to track this"}
    ],
    "multiSelect": false
  }]
}

For unused code (commented out, dead code):

{
  "questions": [{
    "question": "Found commented-out code in file.cpp:78-85. What should we do?",
    "header": "Dead code",
    "options": [
      {"label": "Remove", "description": "Delete the commented code (recommended)"},
      {"label": "Keep", "description": "Leave as-is (explain why in PR)"},
      {"label": "Restore", "description": "Uncomment and use the code"}
    ],
    "multiSelect": false
  }]
}

2.3 Code Duplication Check

Analyze changed files for:

Within-file duplication:

• Similar code blocks (>10 lines)
• Repeated logic patterns
• Copy-paste signatures

Cross-file duplication:

• Same function in multiple files
• Repeated utility code
• Patterns that should be abstracted

Report format:

### Duplication Found

**Location 1:** `src/module_a/handler.cpp:45-60`
**Location 2:** `src/module_b/processor.cpp:120-135`
**Similarity:** ~90%

**Suggested action:** Extract to common utility function in `src/common/utils.cpp`

Ask user:

{
  "questions": [{
    "question": "Found duplicate code in handler.cpp and processor.cpp. Extract to shared utility?",
    "header": "Duplication",
    "options": [
      {"label": "Extract now", "description": "Create shared function and refactor both usages"},
      {"label": "Keep separate", "description": "Leave as-is (intentional duplication)"},
      {"label": "Track for later", "description": "Create issue to address in future refactor"}
    ],
    "multiSelect": false
  }]
}

2.4 Architecture and Design Review

Check for:

Structural issues:

• Functions too long (>50 lines)
• Classes with too many responsibilities
• Deep nesting (>3 levels)
• Long parameter lists (>5 params)
• Circular dependencies

Design pattern opportunities:

• Could Factory pattern simplify object creation?
• Could Strategy pattern replace conditionals?
• Could Observer pattern decouple components?

C++ specific:

• Could templates reduce duplication?
• Could constexpr move computation to compile-time?
• Could STL algorithms replace manual loops?

Report improvements:

### Improvement Opportunities

| Location | Issue | Suggestion | Priority |
|----------|-------|------------|----------|
| `parser.cpp:validate()` | Function is 80 lines | Split into smaller functions | Medium |
| `handler.cpp:process()` | 6 parameters | Use parameter object or builder | Low |
| `utils.cpp:findItem()` | Manual loop | Use `std::find_if` | Low |
| `factory.cpp` | Switch on type | Consider Factory pattern | Medium |

Ask user for each significant improvement:

{
  "questions": [{
    "question": "validate() in parser.cpp is 80 lines. Should we refactor before PR?",
    "header": "Long function",
    "options": [
      {"label": "Refactor now", "description": "Split into smaller functions in this PR"},
      {"label": "Separate PR", "description": "Create refactoring PR first, then this PR"},
      {"label": "Keep as-is", "description": "Leave for now, note in PR description"}
    ],
    "multiSelect": false
  }]
}

2.5 Potential Bug Detection

Check for common issues:

Memory/Resource:

• Uninitialized variables
• Missing null checks
• Resource leaks (files, connections)
• Use-after-move

Logic:

• Off-by-one errors
• Missing break in switch
• Incorrect operator precedence
• Floating-point comparison with ==

Concurrency:

• Race conditions
• Missing locks
• Deadlock potential

Security:

• SQL injection risks
• Command injection
• Buffer overflows
• Hardcoded credentials

2.6 Test Coverage Check

# Check if new code has tests
git diff --name-only | grep -v "_test\|test_\|_spec" | while read f; do
    testfile=$(echo "$f" | sed 's/\.cpp/_test.cpp/' | sed 's/\.py/test_&/')
    if [ ! -f "$testfile" ]; then
        echo "Missing tests for: $f"
    fi
done

Ask if tests are missing:

{
  "questions": [{
    "question": "No tests found for new_feature.cpp. Add tests before PR?",
    "header": "Missing tests",
    "options": [
      {"label": "Add tests now", "description": "Write unit tests before creating PR"},
      {"label": "PR without tests", "description": "Create PR, add tests as follow-up"},
      {"label": "Not needed", "description": "Code doesn't require tests (explain why)"}
    ],
    "multiSelect": false
  }]
}

Phase 3: Verification Report

Present comprehensive findings to user:

## PR Preparation Report

### Standards Compliance
| Check | Status | Notes |
|-------|--------|-------|
| C++ Core Guidelines | ✅ Pass | |
| Const correctness | ⚠️ Warning | 2 functions missing const |
| Error handling | ✅ Pass | |

### Code Quality Issues

**Must Fix (blocking):**
1. ❌ Uninitialized variable in `parser.cpp:45`
2. ❌ Missing null check in `handler.cpp:78`

**Should Fix (recommended):**
1. ⚠️ TODO in `utils.cpp:23` - decide: fix/remove/keep
2. ⚠️ Commented code in `old_impl.cpp:100-120`
3. ⚠️ Duplicate code in `module_a/` and `module_b/`

**Suggestions (optional):**
1. 💡 Long function `validate()` could be split
2. 💡 Manual loop could use `std::find_if`
3. 💡 Consider Factory pattern for object creation

### Test Coverage
- New files: 3
- Files with tests: 2
- Missing tests: `new_feature.cpp`

### Summary
- **Blocking issues:** 2
- **Warnings:** 3
- **Suggestions:** 3

Then ask:

{
  "questions": [{
    "question": "How should we proceed with the 2 blocking issues?",
    "header": "Blocking issues",
    "options": [
      {"label": "Fix all", "description": "Fix all blocking issues before PR"},
      {"label": "Show details", "description": "Show me each issue to decide individually"},
      {"label": "Override", "description": "Create PR anyway (not recommended)"}
    ],
    "multiSelect": false
  }]
}

Phase 4: PR Size Check

Ideal PR Size

• < 400 lines changed - Easy to review, quick turnaround
• 400-800 lines - Acceptable for complex features
• > 800 lines - Should be split into multiple PRs

When to Split

Split into multiple PRs when:

• Changes touch multiple unrelated systems
• Refactoring can be separated from feature work
• Infrastructure changes can land independently
• Tests can be added before implementation

Split Strategy

Large Feature
     │
     ├── PR 1: Refactoring (prepare codebase)
     │         - Extract interfaces
     │         - Move code to better locations
     │         - No behavior change
     │
     ├── PR 2: Infrastructure
     │         - Add new dependencies
     │         - Create base classes/interfaces
     │         - Add configuration
     │
     ├── PR 3: Core Implementation
     │         - Main feature logic
     │         - Unit tests
     │
     └── PR 4: Integration
              - Wire everything together
              - Integration tests
              - Documentation

Phase 5: Create the PR

PR Structure

Every PR MUST have these sections:

1. Motivation

Why is this change needed?

## Motivation

[Explain the problem or need this PR addresses]

- What issue does this solve?
- What feature does this enable?
- What improvement does this bring?

Related issue: #123 (if applicable)

2. Technical Details

What does this PR change and how?

## Technical Details

[Explain the implementation approach]

### Changes
- [List key changes]
- [Explain architectural decisions]
- [Note any trade-offs made]

### Files Changed
- `src/module/file.cpp` - [Brief description]
- `include/module/file.hpp` - [Brief description]

3. Test Plan

How was this tested?

## Test Plan

### Unit Tests
- [ ] `test_feature_basic` - Tests basic functionality
- [ ] `test_feature_edge_cases` - Tests boundary conditions
- [ ] `test_feature_error_handling` - Tests error paths

### Manual Testing
- [ ] [Step-by-step manual test if applicable]

### Verification
- [ ] All existing tests pass
- [ ] New tests added for new functionality
- [ ] No new warnings introduced

PR Template

## Motivation

[Why is this change needed? What problem does it solve?]

## Technical Details

### Summary
[One paragraph summary of the changes]

### Key Changes
- [Change 1]
- [Change 2]
- [Change 3]

### Design Decisions
[Explain any significant design choices and their rationale]

## Test Plan

### Automated Tests
- [ ] Unit tests added/updated
- [ ] All tests passing

### Manual Verification
- [ ] [Describe manual testing performed]

## Pre-PR Verification

- [x] Code verified against programming standards
- [x] No blocking issues found
- [x] TODOs addressed or tracked
- [x] No unnecessary code duplication

## Checklist

- [ ] Code follows project style guidelines
- [ ] Self-review completed
- [ ] Comments added only for non-obvious logic
- [ ] Documentation updated (if applicable)
- [ ] No unrelated changes included

Creating the PR

After verification is complete and user approves:

# Create branch (if not already on feature branch)
git checkout -b feature/descriptive-name

# Stage and commit (use conventional commits if project uses them)
git add .
git commit -m "feat: add user avatar upload

- Add avatar upload endpoint
- Create AvatarUpload component
- Add image validation"

# Push and create PR
git push -u origin HEAD

# Create PR with gh CLI
gh pr create --title "feat: Add user avatar upload" --body "$(cat <<'EOF'
## Motivation

Users need the ability to upload profile avatars to personalize their accounts.
This feature was requested in #123.

## Technical Details

### Summary
Adds avatar upload functionality with client-side preview and server-side validation.

### Key Changes
- Added `POST /api/users/avatar` endpoint
- Created `AvatarUpload` React component with drag-and-drop
- Implemented image validation (size, format)
- Added image resizing to reduce storage

### Design Decisions
- Chose to resize on server to ensure consistent dimensions
- Using WebP format for storage to reduce size

## Test Plan

### Automated Tests
- [x] `test_avatar_upload_valid_image` - Upload succeeds with valid JPG
- [x] `test_avatar_upload_invalid_format` - Rejects non-image files
- [x] `test_avatar_upload_too_large` - Rejects files > 5MB
- [x] `test_avatar_resize` - Verifies resizing to 256x256

### Manual Verification
- [x] Upload works in Chrome, Firefox, Safari
- [x] Drag-and-drop works correctly
- [x] Preview displays before upload

## Pre-PR Verification

- [x] Code verified against C++ Core Guidelines
- [x] No blocking issues found
- [x] All TODOs addressed
- [x] No code duplication

EOF
)"

Verification Checklist Summary

Before creating PR, ensure:

Blocking (must fix)

• No uninitialized variables
• No null pointer risks
• No resource leaks
• No security vulnerabilities
• All tests pass

Important (should fix)

• TODOs addressed (fixed, removed, or tracked)
• No commented-out code
• No significant duplication
• Functions not excessively long
• Test coverage adequate

Optional (nice to have)

• STL algorithms used where applicable
• Design patterns applied where beneficial
• Compile-time computation where possible
• Documentation complete

Integration with Planning

During Task Planning

When creating a plan in planning/feature-*.md or planning/refactor-*.md:

1. Assess total scope - How many lines will change?
2. Identify logical boundaries - What can be separated?
3. Plan PR sequence - Which PRs depend on others?
4. Document in plan file:

## PR Strategy

### PR 1: [Title]
**Scope:** [What's included]
**Dependencies:** None
**Estimated size:** ~200 lines

### PR 2: [Title]
**Scope:** [What's included]
**Dependencies:** PR 1
**Estimated size:** ~350 lines

Examples

Good PR: Focused and Clean

## Motivation

Session timeout is incorrectly set to 5 minutes instead of 30 minutes,
causing users to be logged out unexpectedly. Fixes #456.

## Technical Details

### Summary
Fixed token refresh timing comparison that caused premature session expiry.

### Key Changes
- Fixed comparison operator in `tokenService.ts:45` (`>` → `>=`)
- Added constant `SESSION_TIMEOUT_MS` to centralize configuration

### Design Decisions
- Moved timeout value to config for easier adjustment in future

## Test Plan

### Automated Tests
- [x] `test_token_refresh_at_boundary` - Tests exact timeout boundary
- [x] `test_session_persists_29_minutes` - Verifies no early logout

### Manual Verification
- [x] Logged in and waited 25 minutes - session persisted
- [x] Verified refresh token request at expected time

## Pre-PR Verification

- [x] Code verified against standards
- [x] No TODOs or commented code
- [x] No duplication introduced

Bad PR: Unverified, Messy

## Changes

- Fixed session timeout bug
- TODO: add more tests later
- Left old implementation commented out just in case
- Also refactored auth module while I was there

Problems:

• No verification performed
• Contains TODO
• Contains commented-out code
• Multiple unrelated changes

References

• How to Write a Git Commit Message
• GitHub Pull Request Documentation
• C++ Core Guidelines