mirror of
https://github.com/samsonjs/vibetunnel.git
synced 2026-04-27 15:17:38 +00:00
314 lines
No EOL
12 KiB
Markdown
314 lines
No EOL
12 KiB
Markdown
# /review-pr
|
|
|
|
Perform a comprehensive code review of the current pull request using parallel sub-agents powered by Claude and Gemini, with results intelligently merged.
|
|
|
|
## Usage
|
|
|
|
```
|
|
/review-pr [options]
|
|
```
|
|
|
|
Options:
|
|
- `--focus <area>` - Focus on specific areas. Valid values: `security`, `performance`, `tests`, `architecture`, `documentation`, `all`
|
|
- `--gemini-only` - Perform the review only with gemini CLI
|
|
|
|
## Description
|
|
|
|
This command performs a thorough code review of the current branch's changes compared to the main branch. It runs two parallel sub-agents - one using Claude and another using Gemini - to perform independent comprehensive reviews. The results from both AI models are then intelligently merged to provide a complete multi-perspective analysis.
|
|
|
|
## Examples
|
|
|
|
### Standard PR review
|
|
```
|
|
/review-pr
|
|
```
|
|
|
|
### Security-focused review
|
|
```
|
|
/review-pr --focus security
|
|
```
|
|
|
|
### Performance and scalability review
|
|
```
|
|
/review-pr --focus performance
|
|
```
|
|
|
|
## Implementation
|
|
|
|
When you use this command, I will:
|
|
|
|
1. **Initial PR Context Gathering**
|
|
- Check if required tools are available: `gh`, `git`, and `gemini`
|
|
- Run `gh pr view` to check PR description and any existing comments (handle errors gracefully)
|
|
- Run `git log "main..HEAD" --oneline` to see all commits in this PR (with proper shell escaping)
|
|
- Run `git diff "main...HEAD" --stat` to get an overview of changed files (with proper shell escaping)
|
|
- Generate comprehensive diff with secure temporary file handling:
|
|
```bash
|
|
touch .pr_review_diff.tmp && chmod 600 .pr_review_diff.tmp
|
|
git diff "main...HEAD" > .pr_review_diff.tmp
|
|
```
|
|
- If any git commands fail, provide helpful error messages and guidance
|
|
|
|
2. **Launch Two Parallel Sub-Agents**
|
|
|
|
**Sub-Agent 1: Claude Review**
|
|
- WARNING: This step must be skipped when `--gemini-only` has been passed
|
|
- Performs complete multi-aspect code review using Claude
|
|
- Analyzes all aspects below independently
|
|
|
|
**Sub-Agent 2: Gemini Review**
|
|
- Runs Gemini CLI following instructions from `docs/gemini.md`
|
|
- Performs complete multi-aspect code review using Gemini
|
|
- Form the prompt to the Gemini CLI in a way that it only returns the final output of its findings, to save tokens
|
|
- Analyzes all aspects below independently
|
|
- Use a timeout of 10 minutes for the gemini CLI command (configurable via environment variable `GEMINI_TIMEOUT_SECONDS`, default: 600)
|
|
- **IMPORTANT**: Gemini CLI can only access files within the project directory
|
|
- Create temporary diff file in project root (e.g., `.pr_review_diff.tmp`)
|
|
- Use `@.pr_review_diff.tmp` syntax to include the diff in Gemini prompt
|
|
- Clean up temporary file after review completes
|
|
|
|
**Both sub-agents analyze:**
|
|
|
|
**Correctness & Logic**
|
|
- Verify the code solves the intended problem
|
|
- Check for edge cases, off-by-one errors, null/undefined handling
|
|
- Validate business logic and algorithm correctness
|
|
|
|
**Code Quality & Architecture**
|
|
- Check adherence to DRY and SOLID principles
|
|
- Assess function/class complexity and cohesion
|
|
- Verify consistent naming conventions and code style
|
|
- Ensure changes align with existing architecture patterns
|
|
|
|
**Performance & Scalability**
|
|
- Identify O(n²) algorithms that could be O(n)
|
|
- Check for N+1 database queries
|
|
- Look for memory leaks or excessive allocations
|
|
- Identify missed caching opportunities
|
|
|
|
**Security**
|
|
- Check for SQL injection vulnerabilities
|
|
- Identify XSS vulnerabilities
|
|
- Verify authentication/authorization checks
|
|
- Ensure no sensitive data in logs or commits
|
|
- Validate all user inputs are sanitized
|
|
|
|
**Testing**
|
|
- Verify test coverage for new/modified code
|
|
- Check if tests actually test the right behavior
|
|
- Ensure edge cases are covered
|
|
- Assess test maintainability and brittleness
|
|
|
|
**Error Handling**
|
|
- Verify errors are properly caught and handled
|
|
- Check error messages are helpful for debugging
|
|
- Ensure appropriate logging levels
|
|
- Validate graceful failure modes
|
|
|
|
**Documentation**
|
|
- Check if complex logic is explained
|
|
- Verify API documentation is updated
|
|
- Look for outdated or misleading comments
|
|
- Ensure README/docs are updated if needed
|
|
|
|
**Automated Checks**
|
|
- Execute `pnpm run check` (for web/) if available
|
|
- Run `./scripts/lint.sh` (for mac/) if the script exists
|
|
- Run `./lint.sh` (for tauri/) if the script exists
|
|
- Check if tests pass with appropriate test commands
|
|
- Verify build succeeds
|
|
- If any checks fail, continue with review but note the failures
|
|
|
|
3. **Merge Results**
|
|
- Intelligently combine findings from both Claude and Gemini
|
|
- Identify common issues found by both models (high confidence)
|
|
- Highlight unique insights from each model
|
|
- Resolve any conflicting assessments
|
|
- Generate unified severity ratings
|
|
|
|
4. **Generate Final Review Report**
|
|
- Provide a structured review with:
|
|
- **Summary**: High-level overview of changes
|
|
- **Strengths**: What's done well
|
|
- **Critical Issues**: Must-fix problems with exact line numbers
|
|
- **Suggestions**: Nice-to-have improvements with exact line numbers
|
|
- **Questions**: Clarifications needed with exact line numbers
|
|
- Use accountability mindset - I'm as responsible as the author
|
|
- Provide constructive, mentoring-oriented feedback
|
|
- **CRITICAL**: Always include exact line numbers for every issue found
|
|
- Use format: `filename:line_number` (e.g., `src/server.ts:142`)
|
|
- For multi-line issues, use ranges: `filename:start_line-end_line`
|
|
|
|
5. **Cleanup**
|
|
- Remove temporary diff file `.pr_review_diff.tmp` from project root
|
|
- Ensure no sensitive data remains in temporary files
|
|
|
|
## Review Checklist
|
|
|
|
### ✅ Shared Accountability
|
|
- [ ] I understand I share responsibility for this code once approved
|
|
- [ ] I've reviewed with the same care as if I wrote it
|
|
|
|
### 🎯 Functionality
|
|
- [ ] Code implements intended functionality
|
|
- [ ] Edge cases and error scenarios handled
|
|
- [ ] No regressions introduced
|
|
|
|
### 🏗️ Architecture & Design
|
|
- [ ] Changes align with system architecture
|
|
- [ ] Scalability and maintainability considered
|
|
- [ ] Design patterns appropriately used
|
|
|
|
### 🔒 Security
|
|
- [ ] Input validation present
|
|
- [ ] Authentication/authorization correct
|
|
- [ ] No sensitive data exposed
|
|
- [ ] Dependencies are secure
|
|
|
|
### ⚡ Performance
|
|
- [ ] No unnecessary database queries
|
|
- [ ] Efficient algorithms used
|
|
- [ ] Resource usage is reasonable
|
|
- [ ] Caching utilized where appropriate
|
|
|
|
### 🧪 Testing
|
|
- [ ] Adequate test coverage
|
|
- [ ] Tests are meaningful, not just coverage
|
|
- [ ] Edge cases tested
|
|
- [ ] Tests are maintainable
|
|
|
|
### 📝 Code Quality
|
|
- [ ] Code is DRY
|
|
- [ ] SOLID principles followed
|
|
- [ ] Clear naming and structure
|
|
- [ ] Appropriate comments/documentation
|
|
|
|
### 🔄 Backwards Compatibility
|
|
- [ ] API changes are backwards compatible
|
|
- [ ] Database migrations handled properly
|
|
- [ ] No breaking changes without discussion
|
|
|
|
## Note
|
|
|
|
This command emphasizes:
|
|
- **Parallel Sub-Agent Architecture**: Two independent sub-agents perform complete reviews - one using Claude, another using Gemini CLI
|
|
- **Intelligent Merging**: Final step combines findings from both AI models by identifying common issues (high confidence), highlighting unique insights, and resolving conflicting assessments
|
|
- **Accountability**: Approving means you own the outcome
|
|
- **Mentorship**: Every comment is a teaching opportunity
|
|
- **Thoroughness**: Multiple passes from different angles by both AI models
|
|
- **Actionability**: Specific, clear feedback with examples
|
|
- **Precision**: Every issue must include exact line numbers
|
|
|
|
**Line Number Format Examples:**
|
|
- Single line issue: `src/server/auth.ts:234`
|
|
- Multi-line issue: `src/client/app.ts:45-52`
|
|
- Context reference: `src/utils/helpers.ts:78 (similar pattern at lines 95, 112)`
|
|
|
|
**Sub-Agent Execution:**
|
|
- Claude sub-agent performs the review using Claude's capabilities
|
|
- Gemini sub-agent runs the Gemini CLI following instructions from `docs/gemini.md`
|
|
- Both sub-agents work in parallel for efficiency
|
|
|
|
For large PRs, consider reviewing incrementally and suggesting the author break it into smaller PRs for more effective review.
|
|
|
|
## Prerequisites
|
|
|
|
- Git and GitHub CLI (`gh`) must be installed
|
|
- For full functionality: Gemini CLI must be installed (see Gemini documentation)
|
|
- For `--gemini-only` mode: Only Gemini CLI is required
|
|
|
|
## Security Considerations
|
|
|
|
**Input Sanitization:**
|
|
- All user-provided parameters (e.g., `--focus` values) are validated against a whitelist
|
|
- Git commands use proper shell escaping with quotes around branch names
|
|
- Temporary files are created with secure permissions (mode 600)
|
|
- File paths are validated to prevent directory traversal attacks
|
|
- The `--focus` parameter only accepts predefined values: security, performance, tests, architecture, documentation, all
|
|
|
|
**Safe Command Execution:**
|
|
```bash
|
|
# Always quote branch names and file paths
|
|
git diff "main...HEAD" --stat # Correct
|
|
git diff main...HEAD --stat # Vulnerable to injection
|
|
|
|
# Set restrictive permissions on temporary files
|
|
touch .pr_review_diff.tmp && chmod 600 .pr_review_diff.tmp
|
|
git diff "main...HEAD" > .pr_review_diff.tmp
|
|
```
|
|
|
|
## Error Handling
|
|
|
|
- If required tools are missing, the command will provide installation instructions
|
|
- Git command failures are handled gracefully with helpful error messages
|
|
- If automated checks fail, the review continues but notes the failures
|
|
- Timeout protection: Gemini sub-agent has a 10-minute timeout (configurable via `GEMINI_TIMEOUT_SECONDS` environment variable)
|
|
|
|
## Implementation Notes
|
|
|
|
**Gemini CLI File Access:**
|
|
- Gemini CLI can only access files within the project directory
|
|
- Always save temporary files in the project root with `.tmp` extension
|
|
- Use `.gitignore` patterns for temporary files (e.g., `.pr_review_*.tmp`)
|
|
- Example workflow:
|
|
```bash
|
|
# Create file with secure permissions
|
|
touch .pr_review_diff.tmp && chmod 600 .pr_review_diff.tmp
|
|
|
|
# Save diff to project-accessible location
|
|
git diff "main...HEAD" > .pr_review_diff.tmp
|
|
|
|
# Use with Gemini
|
|
gemini -p "@.pr_review_diff.tmp Your review prompt here"
|
|
|
|
# Clean up
|
|
rm -f .pr_review_diff.tmp
|
|
```
|
|
|
|
**Handling Large Diffs:**
|
|
- For very large PRs, consider splitting the diff by file type or directory
|
|
- Use Gemini's 2M token context window advantage for comprehensive analysis
|
|
- If diff exceeds reasonable size, suggest breaking PR into smaller chunks
|
|
|
|
**Chunking Strategy for Diffs Exceeding 1.5M Tokens:**
|
|
|
|
1. **Automatic splitting by file type:**
|
|
```bash
|
|
# JavaScript/TypeScript files
|
|
git diff "main...HEAD" -- "*.js" "*.ts" "*.jsx" "*.tsx" > .pr_review_js.tmp
|
|
|
|
# Python files
|
|
git diff "main...HEAD" -- "*.py" > .pr_review_py.tmp
|
|
|
|
# Documentation
|
|
git diff "main...HEAD" -- "*.md" "*.rst" "*.txt" > .pr_review_docs.tmp
|
|
```
|
|
|
|
2. **Split by directory structure:**
|
|
```bash
|
|
# Frontend changes
|
|
git diff "main...HEAD" -- "web/src/client/" > .pr_review_frontend.tmp
|
|
|
|
# Backend changes
|
|
git diff "main...HEAD" -- "web/src/server/" > .pr_review_backend.tmp
|
|
|
|
# Test changes
|
|
git diff "main...HEAD" -- "**/test/" "**/tests/" > .pr_review_tests.tmp
|
|
```
|
|
|
|
3. **Prioritize by change type:**
|
|
```bash
|
|
# New files (highest priority)
|
|
git diff "main...HEAD" --name-status | grep "^A" | cut -f2 | xargs git diff "main...HEAD" -- > .pr_review_new.tmp
|
|
|
|
# Modified files
|
|
git diff "main...HEAD" --name-status | grep "^M" | cut -f2 | xargs git diff "main...HEAD" -- > .pr_review_modified.tmp
|
|
|
|
# Deleted files (lowest priority for review)
|
|
git diff "main...HEAD" --name-status | grep "^D" | cut -f2 > .pr_review_deleted_list.tmp
|
|
```
|
|
|
|
4. **Token estimation:**
|
|
- Rough estimate: 1 token ≈ 4 characters
|
|
- Check file size: `wc -c .pr_review_diff.tmp`
|
|
- If > 6MB (≈1.5M tokens), implement chunking strategy |