mirror of
https://github.com/duthaho/claudekit.git
synced 2026-06-15 14:34:55 +03:00
feat: improved the Claude Kit as a plugin
This commit is contained in:
duthaho
@@ -0,0 +1,331 @@
|
||||
---
|
||||
name: receiving-code-review
|
||||
description: >
|
||||
Use when code review feedback is received, whether from human reviewers, automated tools, or PR comments. Use when processing review comments, handling review rejections, iterating on feedback cycles, or deciding how to prioritize critical vs minor issues. Activate aggressively any time review feedback arrives -- categorize, prioritize, fix critical issues first, and re-request review with a clear summary of changes made.
|
||||
---
|
||||
|
||||
# Receiving Code Review
|
||||
|
||||
## When to Use
|
||||
|
||||
- After receiving review feedback
|
||||
- Processing automated review results
|
||||
- Handling reviewer comments on PRs
|
||||
- Iterating after code review rejection
|
||||
|
||||
## When NOT to Use
|
||||
|
||||
- Self-review of your own code where an independent perspective is what you actually need
|
||||
- Initial implementation before any review has been requested or received
|
||||
- Design or brainstorming phase where feedback is about ideas, not code
|
||||
|
||||
---
|
||||
|
||||
## 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]);
|
||||
```
|
||||
|
||||
```python
|
||||
# Python equivalent
|
||||
# Before (vulnerable)
|
||||
query = f"SELECT * FROM users WHERE email = '{email}'"
|
||||
result = await db.execute(text(query))
|
||||
|
||||
# After (secure — use ORM)
|
||||
result = await db.execute(select(User).where(User.email == email))
|
||||
```
|
||||
|
||||
### 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;
|
||||
```
|
||||
|
||||
```python
|
||||
# Python equivalent
|
||||
# Before
|
||||
try:
|
||||
user = await get_user(user_id)
|
||||
except:
|
||||
return None
|
||||
|
||||
# After
|
||||
try:
|
||||
user = await get_user(user_id)
|
||||
except UserNotFoundError:
|
||||
raise HTTPException(status_code=404, detail=f"User {user_id} not found")
|
||||
```
|
||||
|
||||
### 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 () => { /* ... */ });
|
||||
```
|
||||
|
||||
```python
|
||||
# Python equivalent
|
||||
# Before: Only happy path
|
||||
async def test_get_user(client):
|
||||
response = await client.get("/api/users/1")
|
||||
assert response.status_code == 200
|
||||
|
||||
# After: Edge cases covered
|
||||
async def test_get_user_returns_user(client):
|
||||
response = await client.get("/api/users/1")
|
||||
assert response.status_code == 200
|
||||
|
||||
async def test_get_user_not_found(client):
|
||||
response = await client.get("/api/users/999")
|
||||
assert response.status_code == 404
|
||||
|
||||
async def test_get_user_invalid_id(client):
|
||||
response = await client.get("/api/users/not-a-number")
|
||||
assert response.status_code == 422
|
||||
```
|
||||
|
||||
### 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]);
|
||||
```
|
||||
|
||||
```python
|
||||
# Python equivalent (SQLAlchemy)
|
||||
# Before (N+1)
|
||||
users = (await db.execute(select(User))).scalars().all()
|
||||
for user in users:
|
||||
orders = (await db.execute(select(Order).where(Order.user_id == user.id))).scalars().all()
|
||||
|
||||
# After (eager loading)
|
||||
users = (await db.execute(
|
||||
select(User).options(selectinload(User.orders))
|
||||
)).scalars().all()
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 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
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Related Skills
|
||||
|
||||
- `requesting-code-review` - Companion skill for initiating reviews with proper context before feedback is received
|
||||
- `systematic-debugging` - Use systematic debugging techniques when review feedback reveals bugs that need investigation
|
||||
- `verification-before-completion` - After addressing review feedback, verify all fixes before claiming completion
|
||||
@@ -0,0 +1,190 @@
|
||||
# Feedback Categories Reference
|
||||
|
||||
How to categorize, prioritize, and respond to code review feedback.
|
||||
|
||||
## Category Definitions
|
||||
|
||||
### Critical -- Must Fix Before Merge
|
||||
|
||||
**Impact**: Security vulnerability, data loss, crash, or correctness failure.
|
||||
|
||||
**Examples**:
|
||||
- SQL injection or XSS vulnerability
|
||||
- Missing authentication/authorization check
|
||||
- Data corruption or silent data loss
|
||||
- Unhandled exception that crashes the service
|
||||
- Race condition that causes incorrect results
|
||||
- Breaking change to public API without migration path
|
||||
|
||||
**Response**: Fix immediately. No merge until resolved. Thank the reviewer.
|
||||
|
||||
**Time**: Address within hours, not days.
|
||||
|
||||
### Important -- Should Fix
|
||||
|
||||
**Impact**: Logic error, missing edge case, performance issue, or maintainability concern.
|
||||
|
||||
**Examples**:
|
||||
- Missing null/undefined check on a code path that can be reached
|
||||
- N+1 query that will degrade with data growth
|
||||
- Missing error handling for a plausible failure mode
|
||||
- Incorrect business logic for an edge case
|
||||
- Missing test for a significant code path
|
||||
- Resource leak (connection, file handle, memory)
|
||||
|
||||
**Response**: Fix before merge unless there is a strong reason to defer (document with a ticket if deferring).
|
||||
|
||||
**Time**: Address before the next review round.
|
||||
|
||||
### Minor -- Fix If Easy
|
||||
|
||||
**Impact**: Code style, naming, comments, minor readability.
|
||||
|
||||
**Examples**:
|
||||
- Variable name could be clearer
|
||||
- Comment is slightly inaccurate
|
||||
- Could extract a helper function for readability
|
||||
- Import ordering
|
||||
- Unnecessary intermediate variable
|
||||
- Slightly verbose code that could be simplified
|
||||
|
||||
**Response**: Fix if the change is quick and low-risk. If fixing would require significant refactoring, note it for a follow-up.
|
||||
|
||||
**Time**: Address in the current PR or create a follow-up ticket.
|
||||
|
||||
### Subjective -- Discuss and Decide
|
||||
|
||||
**Impact**: Architectural preference, design philosophy, style choice where both options are valid.
|
||||
|
||||
**Examples**:
|
||||
- "I would have used a class here instead of functions"
|
||||
- "I prefer early returns over nested if-else"
|
||||
- "Consider using pattern X instead of pattern Y"
|
||||
- "This could also be modeled as an event-driven system"
|
||||
- Disagreement on level of abstraction
|
||||
|
||||
**Response**: Engage in discussion. Consider the merits. Agree on a direction or escalate to team lead. Neither side is necessarily wrong.
|
||||
|
||||
**Time**: Resolve within one discussion round if possible.
|
||||
|
||||
## Prioritization Matrix
|
||||
|
||||
| Category | Merge Blocker? | Default Action | Can Defer? |
|
||||
|---|---|---|---|
|
||||
| Critical | Yes | Fix now | No |
|
||||
| Important | Usually | Fix now or create ticket | With justification |
|
||||
| Minor | No | Fix if quick | Yes, with follow-up |
|
||||
| Subjective | No | Discuss | Yes, team decision |
|
||||
|
||||
## How to Handle Each Category
|
||||
|
||||
### Receiving Critical Feedback
|
||||
|
||||
1. Acknowledge the issue immediately
|
||||
2. Do not be defensive -- this is protecting users
|
||||
3. Fix and push the update
|
||||
4. Add a test that would catch the issue
|
||||
5. Consider if similar issues exist elsewhere
|
||||
|
||||
```
|
||||
> Reviewer: This SQL query uses string interpolation, which is vulnerable to injection.
|
||||
>
|
||||
> You: Good catch -- fixed in abc1234. Added parameterized query and a test
|
||||
> that verifies injection attempts are escaped. Also checked the other
|
||||
> queries in this module; they all use parameterized queries already.
|
||||
```
|
||||
|
||||
### Receiving Important Feedback
|
||||
|
||||
1. Evaluate whether the feedback is correct (verify, don't assume)
|
||||
2. If correct, fix it
|
||||
3. If you disagree, explain your reasoning with evidence
|
||||
4. If deferring, create a ticket and reference it
|
||||
|
||||
```
|
||||
> Reviewer: This will N+1 query when loading orders with items.
|
||||
>
|
||||
> You: You're right. Added eager loading with joinedload() in commit def5678.
|
||||
> Added a test that asserts query count stays constant regardless of item count.
|
||||
```
|
||||
|
||||
### Receiving Minor Feedback
|
||||
|
||||
1. Fix quickly if possible
|
||||
2. If it requires significant refactoring, note it
|
||||
|
||||
```
|
||||
> Reviewer: Consider renaming `data` to `order_summary` for clarity.
|
||||
>
|
||||
> You: Renamed in abc9012. Agreed it's clearer.
|
||||
```
|
||||
|
||||
or
|
||||
|
||||
```
|
||||
> Reviewer: This function could be extracted into a utility.
|
||||
>
|
||||
> You: Agree, but it's only used here for now. Created PROJ-789 to extract
|
||||
> it if we need it elsewhere. Keeping it inline for this PR.
|
||||
```
|
||||
|
||||
### Receiving Subjective Feedback
|
||||
|
||||
1. Consider the suggestion genuinely
|
||||
2. Present your reasoning if you disagree
|
||||
3. Look for objective criteria to decide (performance, testability, consistency with codebase)
|
||||
4. If no clear winner, defer to existing codebase conventions
|
||||
5. If still no consensus, the code author decides (or escalate)
|
||||
|
||||
```
|
||||
> Reviewer: I'd prefer a class-based approach here.
|
||||
>
|
||||
> You: I considered that. Went with functions because: (1) no shared state
|
||||
> between operations, (2) matches the pattern in src/services/auth.py,
|
||||
> (3) easier to test in isolation. Happy to discuss further if you see
|
||||
> benefits I'm missing.
|
||||
```
|
||||
|
||||
## Handling Disagreements
|
||||
|
||||
### Step-by-Step Process
|
||||
|
||||
1. **Verify the claim**: Run the test, check the docs, reproduce the scenario. Do not argue from assumption.
|
||||
2. **Propose an alternative**: If you disagree, suggest what you would do instead and explain why.
|
||||
3. **Look for objective evidence**: Benchmarks, test results, documentation, or existing patterns in the codebase.
|
||||
4. **Find common ground**: Often both approaches have merit. Look for a synthesis.
|
||||
5. **Escalate if stuck**: Bring in a third opinion (tech lead, team discussion). Do not let PRs stall.
|
||||
|
||||
### What NOT to Do
|
||||
|
||||
- Do not dismiss feedback without investigation
|
||||
- Do not agree with everything to avoid conflict (performative agreement hides bugs)
|
||||
- Do not take feedback personally
|
||||
- Do not let disagreements block merges for days -- timebox the discussion
|
||||
- Do not relitigate decisions that were already agreed upon by the team
|
||||
|
||||
## Feedback Response Checklist
|
||||
|
||||
For each piece of feedback received:
|
||||
|
||||
- [ ] Read and understand the feedback fully
|
||||
- [ ] Categorize it (critical / important / minor / subjective)
|
||||
- [ ] If technical claim: verify it independently (run the code, check docs)
|
||||
- [ ] Respond with what you did (fixed, deferred with ticket, or discussed)
|
||||
- [ ] If fixed: reference the commit
|
||||
- [ ] If deferred: reference the ticket
|
||||
- [ ] If disagreeing: provide reasoning with evidence
|
||||
|
||||
## Quick Reference: Response Templates
|
||||
|
||||
**Agreeing and fixing:**
|
||||
> Fixed in [commit]. Added test to prevent regression.
|
||||
|
||||
**Agreeing and deferring:**
|
||||
> Agreed. Created [TICKET] to address this. Out of scope for this PR.
|
||||
|
||||
**Disagreeing with reasoning:**
|
||||
> Considered this. Went with [approach] because [reason 1], [reason 2]. Here's [evidence]. Open to discussion.
|
||||
|
||||
**Asking for clarification:**
|
||||
> Can you clarify what you mean by [X]? I want to make sure I address the right concern.
|
||||
Reference in New Issue
Block a user