mirror of
https://github.com/duthaho/claudekit.git
synced 2026-07-27 07:01:02 +03:00
Add new methodology skills for enhanced development workflows
This commit is contained in:
@@ -0,0 +1,258 @@
|
||||
# Receiving Code Review
|
||||
|
||||
## Description
|
||||
|
||||
Workflow for processing code review feedback effectively. Prioritize issues, apply fixes, and iterate until approval.
|
||||
|
||||
## When to Use
|
||||
|
||||
- After receiving review feedback
|
||||
- Processing automated review results
|
||||
- Handling reviewer comments on PRs
|
||||
- Iterating after code review rejection
|
||||
|
||||
---
|
||||
|
||||
## Feedback Categories
|
||||
|
||||
### Critical Issues
|
||||
|
||||
**Definition**: Must fix before proceeding. Security vulnerabilities, data loss risks, broken functionality.
|
||||
|
||||
```markdown
|
||||
Examples:
|
||||
- SQL injection vulnerability
|
||||
- Unhandled null pointer
|
||||
- Data corruption possibility
|
||||
- Authentication bypass
|
||||
```
|
||||
|
||||
**Response**: Fix immediately. Do not proceed until resolved.
|
||||
|
||||
### Important Issues
|
||||
|
||||
**Definition**: Should fix before proceeding. Code quality, maintainability, potential bugs.
|
||||
|
||||
```markdown
|
||||
Examples:
|
||||
- Missing error handling
|
||||
- Inefficient algorithm
|
||||
- Poor naming
|
||||
- Missing tests for edge cases
|
||||
```
|
||||
|
||||
**Response**: Fix before merging. May defer to follow-up if blocking.
|
||||
|
||||
### Minor Issues
|
||||
|
||||
**Definition**: Can fix later. Style preferences, optional improvements.
|
||||
|
||||
```markdown
|
||||
Examples:
|
||||
- Variable naming suggestions
|
||||
- Comment improvements
|
||||
- Minor refactoring opportunities
|
||||
- Documentation polish
|
||||
```
|
||||
|
||||
**Response**: Note for later. Can merge without addressing.
|
||||
|
||||
---
|
||||
|
||||
## Processing Workflow
|
||||
|
||||
### Step 1: Categorize All Feedback
|
||||
|
||||
```markdown
|
||||
## Review Feedback
|
||||
|
||||
### Critical (Must Fix)
|
||||
1. Line 45: SQL query vulnerable to injection
|
||||
2. Line 89: User data exposed in logs
|
||||
|
||||
### Important (Should Fix)
|
||||
1. Line 23: Missing null check
|
||||
2. Line 67: Test doesn't cover error path
|
||||
|
||||
### Minor (Can Defer)
|
||||
1. Line 12: Consider renaming 'x' to 'count'
|
||||
2. Line 34: Could extract to helper function
|
||||
```
|
||||
|
||||
### Step 2: Fix Critical Issues First
|
||||
|
||||
```markdown
|
||||
Addressing critical issue 1:
|
||||
- File: src/db/queries.ts:45
|
||||
- Issue: SQL injection vulnerability
|
||||
- Fix: Use parameterized query
|
||||
- Verification: Tested with malicious input
|
||||
```
|
||||
|
||||
### Step 3: Fix Important Issues
|
||||
|
||||
```markdown
|
||||
Addressing important issue 1:
|
||||
- File: src/services/user.ts:23
|
||||
- Issue: Missing null check
|
||||
- Fix: Added guard clause
|
||||
- Verification: Test added for null case
|
||||
```
|
||||
|
||||
### Step 4: Note Minor Issues
|
||||
|
||||
```markdown
|
||||
Deferred for follow-up:
|
||||
- Line 12: Variable rename (tracked in TODO)
|
||||
- Line 34: Extract helper (low priority)
|
||||
```
|
||||
|
||||
### Step 5: Request Re-Review
|
||||
|
||||
After fixes applied, request re-review with:
|
||||
|
||||
```markdown
|
||||
## Re-Review Request
|
||||
|
||||
### Fixed Issues
|
||||
- [x] SQL injection (line 45) - Now uses parameterized query
|
||||
- [x] Data exposure (line 89) - Removed user data from logs
|
||||
- [x] Null check (line 23) - Added guard clause
|
||||
- [x] Test coverage (line 67) - Added error path test
|
||||
|
||||
### Deferred (Minor)
|
||||
- Variable rename (line 12) - Will address in cleanup PR
|
||||
|
||||
### Changes Since Last Review
|
||||
- 4 files modified
|
||||
- 2 tests added
|
||||
- All previous feedback addressed
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Handling Disagreements
|
||||
|
||||
### When You Disagree with Feedback
|
||||
|
||||
```markdown
|
||||
1. Don't dismiss immediately
|
||||
2. Consider the reviewer's perspective
|
||||
3. Explain your reasoning
|
||||
4. Provide evidence (code, tests, docs)
|
||||
5. Be open to being wrong
|
||||
6. Escalate if needed (tech lead, team discussion)
|
||||
```
|
||||
|
||||
### Disagreement Response Template
|
||||
|
||||
```markdown
|
||||
## Re: [Feedback item]
|
||||
|
||||
I considered this feedback carefully. Here's my perspective:
|
||||
|
||||
**Reviewer's concern**: [Their point]
|
||||
|
||||
**My reasoning**: [Why I did it this way]
|
||||
|
||||
**Evidence**: [Tests, benchmarks, docs supporting approach]
|
||||
|
||||
**Proposed resolution**: [Accept, discuss, or defer]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Common Feedback Types
|
||||
|
||||
### Security Issues
|
||||
|
||||
Always fix immediately:
|
||||
|
||||
```typescript
|
||||
// Before (vulnerable)
|
||||
const query = `SELECT * FROM users WHERE id = '${userId}'`;
|
||||
|
||||
// After (secure)
|
||||
const query = 'SELECT * FROM users WHERE id = $1';
|
||||
const result = await db.query(query, [userId]);
|
||||
```
|
||||
|
||||
### Error Handling
|
||||
|
||||
Add comprehensive handling:
|
||||
|
||||
```typescript
|
||||
// Before
|
||||
const user = await getUser(id);
|
||||
return user.name;
|
||||
|
||||
// After
|
||||
const user = await getUser(id);
|
||||
if (!user) {
|
||||
throw new NotFoundError(`User ${id} not found`);
|
||||
}
|
||||
return user.name;
|
||||
```
|
||||
|
||||
### Test Coverage
|
||||
|
||||
Add missing tests:
|
||||
|
||||
```typescript
|
||||
// Before: Only happy path tested
|
||||
it('should return user', async () => {
|
||||
const user = await getUser('valid-id');
|
||||
expect(user).toBeDefined();
|
||||
});
|
||||
|
||||
// After: Edge cases covered
|
||||
it('should return user', async () => { /* ... */ });
|
||||
it('should throw NotFoundError for missing user', async () => { /* ... */ });
|
||||
it('should throw ValidationError for invalid id', async () => { /* ... */ });
|
||||
```
|
||||
|
||||
### Performance
|
||||
|
||||
Address efficiency concerns:
|
||||
|
||||
```typescript
|
||||
// Before (N+1 query)
|
||||
const users = await getUsers();
|
||||
for (const user of users) {
|
||||
user.orders = await getOrders(user.id);
|
||||
}
|
||||
|
||||
// After (batch query)
|
||||
const users = await getUsers();
|
||||
const userIds = users.map(u => u.id);
|
||||
const ordersByUser = await getOrdersForUsers(userIds);
|
||||
users.forEach(u => u.orders = ordersByUser[u.id]);
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Re-Review Checklist
|
||||
|
||||
Before requesting re-review:
|
||||
|
||||
- [ ] All Critical issues fixed
|
||||
- [ ] All Important issues fixed (or explicitly deferred with reason)
|
||||
- [ ] Minor issues noted for follow-up
|
||||
- [ ] Tests added/updated for fixes
|
||||
- [ ] Full test suite passes
|
||||
- [ ] Changes summarized for reviewer
|
||||
|
||||
---
|
||||
|
||||
## Iteration Limits
|
||||
|
||||
```markdown
|
||||
If review requires 3+ cycles:
|
||||
1. STOP
|
||||
2. Schedule discussion with reviewer
|
||||
3. Identify root cause of misalignment
|
||||
4. May need design discussion
|
||||
5. Don't keep iterating endlessly
|
||||
```
|
||||
|
||||
---
|
||||
Reference in New Issue
Block a user