git-prepare-pull-request

# Prepare Pull Request Skill Use this skill when planning and preparing Pull Requests. For reviewing PRs, use `pr-review` instead. <IMPORTANT> **Before creating a PR, run comprehensive code verification.** 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 ```bash # 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: | File Extension | Skills to Load | |----------------|----------------| | `.cpp`, `.hpp`, `.h` | `programming-cpp`, `programming-cpp-design-patterns`, `programming-cpp-stl-algorithms`, `programming-cpp-naming-rules` | | `.py` | `programming-python` | | `CMakeLists.txt`, `.cmake` | `programming-cmake-best-practices` | ## Phase 2: Code Verification <IMPORTANT> This phase is **MANDATORY** before creating a PR. Catch problems before reviewers do. </IMPORTANT> ### 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: ```bash # 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: ```json { "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):** ```json { "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:** ```markdown ### 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:** ```json { "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:** ```markdown ### 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:** ```json { "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 ```bash # 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:** ```json { "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: ```markdown ## 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:** ```json { "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