try to restructure the agents and skills given the new skills and command merge
This commit is contained in:
206
old/skills/code-review/SKILL.md
Normal file
206
old/skills/code-review/SKILL.md
Normal file
@@ -0,0 +1,206 @@
|
||||
---
|
||||
name: code-review
|
||||
model: haiku
|
||||
description: Review code for quality, bugs, security, and style issues. Use when reviewing pull requests, checking code quality, looking for bugs or security vulnerabilities, or when the user asks for a code review.
|
||||
user-invocable: false
|
||||
---
|
||||
|
||||
# Code Review
|
||||
|
||||
Guidelines for reviewing code changes in pull requests.
|
||||
|
||||
## Review Categories
|
||||
|
||||
### Code Quality
|
||||
|
||||
Look for:
|
||||
- **Readability**: Clear naming, logical structure, appropriate comments
|
||||
- **Maintainability**: Easy to modify, follows DRY, single responsibility
|
||||
- **Complexity**: Avoid deep nesting, overly long functions, complex conditionals
|
||||
- **Dead code**: Unused variables, unreachable code, commented-out blocks
|
||||
|
||||
Questions to ask:
|
||||
- Can someone unfamiliar with this code understand it quickly?
|
||||
- Would I be comfortable maintaining this code?
|
||||
- Does this follow existing patterns in the codebase?
|
||||
|
||||
### Potential Bugs
|
||||
|
||||
Look for:
|
||||
- **Edge cases**: Empty arrays, null values, boundary conditions
|
||||
- **Logic errors**: Off-by-one, incorrect operators, inverted conditions
|
||||
- **Race conditions**: Async operations, shared state
|
||||
- **Resource leaks**: Unclosed connections, missing cleanup
|
||||
- **Error handling**: Unhandled exceptions, silent failures
|
||||
|
||||
Questions to ask:
|
||||
- What happens with unexpected input?
|
||||
- Are all error paths handled appropriately?
|
||||
- Could concurrent execution cause issues?
|
||||
|
||||
### Security Concerns
|
||||
|
||||
Look for:
|
||||
- **Injection**: SQL, command, XSS vulnerabilities
|
||||
- **Authentication**: Bypasses, weak validation
|
||||
- **Authorization**: Missing permission checks
|
||||
- **Data exposure**: Logging secrets, exposing internals
|
||||
- **Dependencies**: Known vulnerabilities in imports
|
||||
|
||||
Questions to ask:
|
||||
- Could an attacker exploit this?
|
||||
- Is user input properly validated and sanitized?
|
||||
- Are secrets properly protected?
|
||||
|
||||
### Style & Consistency
|
||||
|
||||
Look for:
|
||||
- **Naming conventions**: Match existing codebase style
|
||||
- **Formatting**: Consistent indentation, spacing
|
||||
- **File organization**: Logical grouping, appropriate location
|
||||
- **Import order**: Following project conventions
|
||||
|
||||
Note: Style issues are lower priority than functional concerns.
|
||||
|
||||
### Test Coverage
|
||||
|
||||
Look for:
|
||||
- **Missing tests**: New functionality without tests
|
||||
- **Edge cases**: Boundary conditions not tested
|
||||
- **Error paths**: Exception handling not verified
|
||||
- **Integration**: Component interactions not covered
|
||||
|
||||
Questions to ask:
|
||||
- Would these tests catch a regression?
|
||||
- Are the assertions meaningful?
|
||||
- Do tests cover the acceptance criteria?
|
||||
|
||||
## Review Process
|
||||
|
||||
1. **Understand context**: Read PR description and linked issues
|
||||
2. **High-level scan**: Understand overall structure and approach
|
||||
3. **Detailed review**: Go through changes file by file
|
||||
4. **Consider impact**: Think about side effects and dependencies
|
||||
5. **Provide feedback**: Be constructive and specific
|
||||
|
||||
## Writing Review Comments
|
||||
|
||||
### Be Constructive
|
||||
- Explain *why* something is an issue
|
||||
- Suggest alternatives when possible
|
||||
- Distinguish between blocking issues and suggestions
|
||||
|
||||
### Be Specific
|
||||
- Reference exact lines or code blocks
|
||||
- Provide concrete examples
|
||||
- Link to relevant documentation or patterns
|
||||
|
||||
### Be Kind
|
||||
- Phrase feedback as questions when appropriate
|
||||
- Acknowledge good solutions
|
||||
- Remember there's a person receiving this feedback
|
||||
|
||||
## Example Review Comments
|
||||
|
||||
### Code Quality
|
||||
|
||||
**Good:**
|
||||
> `src/utils/parser.ts:45` - This function is doing three things: parsing, validating, and transforming. Consider splitting into `parse()`, `validate()`, and `transform()` to improve testability and make each responsibility clear.
|
||||
|
||||
**Bad:**
|
||||
> This code is messy, please clean it up.
|
||||
|
||||
### Potential Bugs
|
||||
|
||||
**Good:**
|
||||
> `src/api/users.ts:23` - `users.find()` returns `undefined` when no match is found, but line 25 accesses `user.id` without a null check. This will throw when the user doesn't exist. Consider: `const user = users.find(...); if (!user) return null;`
|
||||
|
||||
**Bad:**
|
||||
> This might crash.
|
||||
|
||||
### Security
|
||||
|
||||
**Good:**
|
||||
> `src/routes/search.ts:12` - The query parameter is interpolated directly into the SQL string, which allows SQL injection. Use parameterized queries instead: `db.query('SELECT * FROM items WHERE name = ?', [query])`
|
||||
|
||||
**Bad:**
|
||||
> Security issue here.
|
||||
|
||||
### Style
|
||||
|
||||
**Good:**
|
||||
> `src/components/Button.tsx:8` - Minor: The codebase uses `camelCase` for event handlers (e.g., `handleClick`), but this uses `on_click`. Consider renaming for consistency.
|
||||
|
||||
**Bad:**
|
||||
> Wrong naming convention.
|
||||
|
||||
### Test Coverage
|
||||
|
||||
**Good:**
|
||||
> The happy path is well tested. Consider adding a test for when `fetchUser()` rejects - the error handling in line 34 isn't currently covered.
|
||||
|
||||
**Bad:**
|
||||
> Need more tests.
|
||||
|
||||
### Positive Feedback
|
||||
|
||||
**Good:**
|
||||
> Nice use of the builder pattern here - it makes the configuration much more readable than the previous approach with multiple boolean flags.
|
||||
|
||||
## Verdict Criteria
|
||||
|
||||
### LGTM (Looks Good To Me)
|
||||
- No blocking issues
|
||||
- Code meets quality standards
|
||||
- Tests are adequate
|
||||
- Ready to merge
|
||||
|
||||
### Needs Changes
|
||||
- Minor issues that should be addressed
|
||||
- Style improvements
|
||||
- Missing tests for edge cases
|
||||
- Not blocking, but worth fixing
|
||||
|
||||
### Blocking Issues
|
||||
- Security vulnerabilities
|
||||
- Logic errors that would cause bugs
|
||||
- Missing critical functionality
|
||||
- Breaking changes without migration
|
||||
|
||||
## Review Comment Template
|
||||
|
||||
```markdown
|
||||
## AI Code Review
|
||||
|
||||
> This is an automated review generated by the code-reviewer agent.
|
||||
|
||||
### Summary
|
||||
[1-2 sentence overall assessment of the changes]
|
||||
|
||||
### Findings
|
||||
|
||||
#### Code Quality
|
||||
- [Issue with specific file:line reference and explanation]
|
||||
- [Or "Code is well-structured and readable"]
|
||||
|
||||
#### Potential Bugs
|
||||
- [Bug risk with explanation]
|
||||
- [Or "No obvious issues found"]
|
||||
|
||||
#### Security Concerns
|
||||
- [Security issue with severity]
|
||||
- [Or "No security concerns identified"]
|
||||
|
||||
#### Style Notes
|
||||
- [Style improvement suggestion]
|
||||
- [Or "Consistent with codebase conventions"]
|
||||
|
||||
#### Test Coverage
|
||||
- [Missing test scenario]
|
||||
- [Or "Tests adequately cover changes"]
|
||||
|
||||
### Verdict
|
||||
**[LGTM / Needs Changes / Blocking Issues]**
|
||||
|
||||
[Brief explanation of verdict]
|
||||
```
|
||||
Reference in New Issue
Block a user