feat: Initial agent-skills repo — 4 adapted skills for Mosaic Stack
Skills included: - pr-reviewer: Adapted for Gitea/GitHub via platform-aware scripts (dropped fetch_pr_data.py and add_inline_comment.py, kept generate_review_files.py) - code-review-excellence: Methodology and checklists (React, TS, Python, etc.) - vercel-react-best-practices: 57 rules for React/Next.js performance - tailwind-design-system: Tailwind CSS v4 patterns, CVA, design tokens New shell scripts added to ~/.claude/scripts/git/: - pr-diff.sh: Get PR diff (GitHub gh / Gitea API) - pr-metadata.sh: Get PR metadata as normalized JSON Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Jason Woltje
345
skills/pr-reviewer/references/review_criteria.md
Normal file
345
skills/pr-reviewer/references/review_criteria.md
Normal file
@@ -0,0 +1,345 @@
|
||||
# Code Review Criteria
|
||||
|
||||
This document outlines the comprehensive criteria for conducting pull request code reviews. Use this as a checklist when reviewing PRs to ensure thorough, consistent, and constructive feedback.
|
||||
|
||||
## Review Process Overview
|
||||
|
||||
When reviewing a PR, the goal is to ensure changes are:
|
||||
- **Correct**: Solves the intended problem without bugs
|
||||
- **Maintainable**: Easy to understand and modify
|
||||
- **Aligned**: Follows project standards and conventions
|
||||
- **Secure**: Free from vulnerabilities
|
||||
- **Tested**: Covered by appropriate tests
|
||||
|
||||
## 1. Functionality and Correctness
|
||||
|
||||
### Problem Resolution
|
||||
- [ ] **Does the code solve the intended problem?**
|
||||
- Verify changes address the issue or feature described in the PR
|
||||
- Cross-reference with linked tickets (JIRA, GitHub issues)
|
||||
- Test manually or run the code if possible
|
||||
|
||||
### Bugs and Logic
|
||||
- [ ] **Are there bugs or logical errors?**
|
||||
- Check for off-by-one errors
|
||||
- Verify null/undefined/None handling
|
||||
- Review assumptions about inputs and outputs
|
||||
- Look for race conditions or concurrency issues
|
||||
- Check loop termination conditions
|
||||
|
||||
### Edge Cases and Error Handling
|
||||
- [ ] **Edge cases handled?**
|
||||
- Empty collections (arrays, lists, maps)
|
||||
- Null/None/undefined values
|
||||
- Boundary values (min/max integers, empty strings)
|
||||
- Invalid or malformed inputs
|
||||
|
||||
- [ ] **Error handling implemented?**
|
||||
- Network failures
|
||||
- File system errors
|
||||
- Database connection issues
|
||||
- API errors and timeouts
|
||||
- Graceful degradation
|
||||
|
||||
### Compatibility
|
||||
- [ ] **Works across supported environments?**
|
||||
- Browser compatibility (if web app)
|
||||
- OS versions (if desktop/mobile)
|
||||
- Database versions
|
||||
- Language/runtime versions
|
||||
- Doesn't break existing features (regression check)
|
||||
|
||||
## 2. Readability and Maintainability
|
||||
|
||||
### Code Clarity
|
||||
- [ ] **Easy to read and understand?**
|
||||
- Meaningful variable names (avoid `x`, `temp`, `data`)
|
||||
- Meaningful function names (verb-first, descriptive)
|
||||
- Short methods/functions (ideally < 50 lines)
|
||||
- Logical structure and flow
|
||||
- Minimal nested complexity
|
||||
|
||||
### Modularity
|
||||
- [ ] **Single Responsibility Principle?**
|
||||
- Functions/methods do one thing well
|
||||
- Classes have a clear, focused purpose
|
||||
- No "god objects" or overly complex logic
|
||||
|
||||
- [ ] **Suggest refactoring if needed:**
|
||||
- Extract complex logic into helper functions
|
||||
- Break large functions into smaller ones
|
||||
- Separate concerns (UI, business logic, data access)
|
||||
|
||||
### Code Duplication
|
||||
- [ ] **DRY (Don't Repeat Yourself)?**
|
||||
- Repeated code abstracted into helpers
|
||||
- Shared logic moved to libraries/utilities
|
||||
- Avoid copy-paste programming
|
||||
|
||||
### Future-Proofing
|
||||
- [ ] **Allows for easy extensions?**
|
||||
- Avoid hard-coded values (use constants/configs)
|
||||
- Use dependency injection where appropriate
|
||||
- Follow SOLID principles
|
||||
- Consider extensibility without modification
|
||||
|
||||
## 3. Style and Conventions
|
||||
|
||||
### Style Guide Adherence
|
||||
- [ ] **Follows project linter rules?**
|
||||
- ESLint (JavaScript/TypeScript)
|
||||
- Pylint/Flake8/Black (Python)
|
||||
- RuboCop (Ruby)
|
||||
- Checkstyle/PMD (Java)
|
||||
- golangci-lint (Go)
|
||||
|
||||
- [ ] **Formatting consistent?**
|
||||
- Proper indentation (spaces vs. tabs)
|
||||
- Consistent spacing
|
||||
- Line length limits
|
||||
- Import/require organization
|
||||
|
||||
### Codebase Consistency
|
||||
- [ ] **Matches existing patterns?**
|
||||
- Follows established architectural patterns
|
||||
- Uses existing utilities and helpers
|
||||
- Consistent naming conventions
|
||||
- Matches idioms of the language/framework
|
||||
|
||||
### Comments and Documentation
|
||||
- [ ] **Sufficient comments?**
|
||||
- Complex algorithms explained
|
||||
- Non-obvious decisions documented
|
||||
- API contracts clarified
|
||||
- TODOs tracked with ticket numbers
|
||||
|
||||
- [ ] **Not excessive?**
|
||||
- Code should be self-documenting where possible
|
||||
- Avoid obvious comments ("increment i")
|
||||
|
||||
- [ ] **Documentation updated?**
|
||||
- README reflects new features
|
||||
- API docs updated
|
||||
- Inline docs (JSDoc, docstrings, etc.)
|
||||
- Architecture diagrams current
|
||||
|
||||
## 4. Performance and Efficiency
|
||||
|
||||
### Resource Usage
|
||||
- [ ] **Algorithm efficiency?**
|
||||
- Avoid O(n²) or worse in loops
|
||||
- Use appropriate data structures
|
||||
- Minimize database queries (N+1 problem)
|
||||
- Avoid unnecessary computations
|
||||
|
||||
### Scalability
|
||||
- [ ] **Performs well under load?**
|
||||
- No blocking operations in critical paths
|
||||
- Async/await for I/O operations
|
||||
- Pagination for large datasets
|
||||
- Caching where appropriate
|
||||
|
||||
### Optimization Balance
|
||||
- [ ] **Optimizations necessary?**
|
||||
- Premature optimization avoided
|
||||
- Readability not sacrificed for micro-optimizations
|
||||
- Benchmark before complex optimizations
|
||||
- Profile to identify actual bottlenecks
|
||||
|
||||
## 5. Security and Best Practices
|
||||
|
||||
### Vulnerabilities
|
||||
- [ ] **Common security issues addressed?**
|
||||
- SQL injection (use parameterized queries)
|
||||
- XSS (Cross-Site Scripting) - proper escaping
|
||||
- CSRF (Cross-Site Request Forgery) - tokens
|
||||
- Command injection
|
||||
- Path traversal
|
||||
- Authentication/authorization checks
|
||||
|
||||
### Data Handling
|
||||
- [ ] **Sensitive data protected?**
|
||||
- Encrypted in transit (HTTPS/TLS)
|
||||
- Encrypted at rest
|
||||
- Input validation and sanitization
|
||||
- Output encoding
|
||||
- PII handling compliance (GDPR, etc.)
|
||||
|
||||
- [ ] **Secrets management?**
|
||||
- No hardcoded passwords/API keys
|
||||
- Use environment variables
|
||||
- Use secret management systems
|
||||
- No secrets in logs
|
||||
|
||||
### Dependencies
|
||||
- [ ] **New packages justified?**
|
||||
- Actually necessary
|
||||
- From trusted sources
|
||||
- Up-to-date and maintained
|
||||
- No known vulnerabilities
|
||||
- License compatible
|
||||
|
||||
- [ ] **Dependency management?**
|
||||
- Lock files committed
|
||||
- Minimal dependency footprint
|
||||
- Consider alternatives if bloated
|
||||
|
||||
## 6. Testing and Quality Assurance
|
||||
|
||||
### Test Coverage
|
||||
- [ ] **Tests exist for new code?**
|
||||
- Unit tests for individual functions/methods
|
||||
- Integration tests for workflows
|
||||
- End-to-end tests for critical paths
|
||||
|
||||
- [ ] **Tests cover scenarios?**
|
||||
- Happy paths
|
||||
- Error conditions
|
||||
- Edge cases
|
||||
- Boundary conditions
|
||||
|
||||
### Test Quality
|
||||
- [ ] **Tests are meaningful?**
|
||||
- Not just for coverage metrics
|
||||
- Assert actual behavior
|
||||
- Test intent, not implementation
|
||||
- Avoid brittle tests
|
||||
|
||||
- [ ] **Test maintainability?**
|
||||
- Clear test names
|
||||
- Arrange-Act-Assert pattern
|
||||
- Minimal test duplication
|
||||
- Fast execution
|
||||
|
||||
### CI/CD Integration
|
||||
- [ ] **Automated checks pass?**
|
||||
- Linting
|
||||
- Tests (unit, integration, e2e)
|
||||
- Build process
|
||||
- Security scans
|
||||
- Code coverage thresholds
|
||||
|
||||
## 7. Overall PR Quality
|
||||
|
||||
### Scope
|
||||
- [ ] **PR is focused?**
|
||||
- Single feature/fix per PR
|
||||
- Not too large (< 400 lines ideal)
|
||||
- Suggest splitting if combines unrelated changes
|
||||
|
||||
### Commit History
|
||||
- [ ] **Clean, atomic commits?**
|
||||
- Each commit is logical unit
|
||||
- Descriptive commit messages
|
||||
- Follow conventional commits if applicable
|
||||
- Avoid "fix", "update", "wip" vagueness
|
||||
|
||||
### PR Description
|
||||
- [ ] **Clear description?**
|
||||
- Explains **why** changes were made
|
||||
- Links to tickets/issues
|
||||
- Steps to reproduce/test
|
||||
- Screenshots for UI changes
|
||||
- Breaking changes called out
|
||||
- Migration steps if needed
|
||||
|
||||
### Impact Assessment
|
||||
- [ ] **Considered downstream effects?**
|
||||
- API changes (breaking vs. backward-compatible)
|
||||
- Database schema changes
|
||||
- Impact on other teams/services
|
||||
- Performance implications
|
||||
- Monitoring and alerting needs
|
||||
|
||||
## Review Feedback Guidelines
|
||||
|
||||
### Communication Style
|
||||
- **Be constructive and kind**
|
||||
- Frame as suggestions: "Consider X because Y"
|
||||
- Not criticism: "This is wrong"
|
||||
- Acknowledge good work
|
||||
- Explain the "why" behind feedback
|
||||
|
||||
### Prioritization
|
||||
- **Focus on critical issues first:**
|
||||
1. Bugs and correctness
|
||||
2. Security vulnerabilities
|
||||
3. Performance problems
|
||||
4. Design/architecture issues
|
||||
5. Style and conventions
|
||||
|
||||
### Feedback Markers
|
||||
Use clear markers to indicate severity:
|
||||
- **🔴 Blocker**: Must be fixed before merge
|
||||
- **🟡 Important**: Should be addressed
|
||||
- **🟢 Nit**: Nice to have, optional
|
||||
- **💡 Suggestion**: Consider for future
|
||||
- **❓ Question**: Clarification needed
|
||||
- **✅ Praise**: Good work!
|
||||
|
||||
### Time Efficiency
|
||||
- Review promptly (within 24 hours)
|
||||
- For large PRs, review in chunks
|
||||
- Request smaller PRs if too large
|
||||
- Use automated tools to catch style issues
|
||||
|
||||
### Decision Making
|
||||
- **Approve**: Solid overall, minor nits acceptable
|
||||
- **Request Changes**: Blockers must be addressed
|
||||
- **Comment**: Provide feedback without blocking
|
||||
|
||||
## Language/Framework-Specific Considerations
|
||||
|
||||
### JavaScript/TypeScript
|
||||
- Type safety (TypeScript)
|
||||
- Promise handling (avoid callback hell)
|
||||
- Memory leaks (event listeners)
|
||||
- Bundle size impact
|
||||
|
||||
### Python
|
||||
- PEP 8 compliance
|
||||
- Type hints (Python 3.5+)
|
||||
- Virtual environment dependencies
|
||||
- Generator usage for memory efficiency
|
||||
|
||||
### Java
|
||||
- Memory management
|
||||
- Exception handling (checked vs. unchecked)
|
||||
- Thread safety
|
||||
- Immutability where appropriate
|
||||
|
||||
### Go
|
||||
- Error handling (no exceptions)
|
||||
- Goroutine management
|
||||
- Channel usage
|
||||
- Interface design
|
||||
|
||||
### SQL/Database
|
||||
- Index usage
|
||||
- Query performance
|
||||
- Transaction boundaries
|
||||
- Migration reversibility
|
||||
|
||||
### Frontend (React, Vue, Angular)
|
||||
- Component reusability
|
||||
- State management
|
||||
- Accessibility (a11y)
|
||||
- Performance (re-renders, bundle size)
|
||||
|
||||
## Tools and Automation
|
||||
|
||||
Leverage tools to automate checks:
|
||||
- **Linters**: ESLint, Pylint, RuboCop
|
||||
- **Formatters**: Prettier, Black, gofmt
|
||||
- **Security**: Snyk, CodeQL, Dependabot
|
||||
- **Coverage**: Codecov, Coveralls
|
||||
- **Performance**: Lighthouse, WebPageTest
|
||||
- **Accessibility**: axe, WAVE
|
||||
|
||||
## Resources
|
||||
|
||||
- Google Engineering Practices: https://google.github.io/eng-practices/review/
|
||||
- GitHub Code Review Guide: https://github.com/features/code-review
|
||||
- OWASP Top 10: https://owasp.org/www-project-top-ten/
|
||||
- Clean Code (Robert C. Martin)
|
||||
- Code Complete (Steve McConnell)
|
||||
Reference in New Issue
Block a user