{"id":"code-review-excellence","name":"code-review-excellence","summary":"効果的なコードレビューの実践を習得し、建設的なフィードバックを提供し、早期にバグを発見し、知識共有を促進しつつチームの士気を維持しましょう。","body":"# Code Review Excellence\n\nTransform code reviews from gatekeeping to knowledge sharing through constructive feedback, systematic analysis, and collaborative improvement.\n\n## When to Use This Skill\n\n- Reviewing pull requests and code changes\n- Establishing code review standards for teams\n- Mentoring junior developers through reviews\n- Conducting architecture reviews\n- Creating review checklists and guidelines\n- Improving team collaboration\n- Reducing code review cycle time\n- Maintaining code quality standards\n\n## Core Principles\n\n### 1. The Review Mindset\n\n**Goals of Code Review:**\n\n- Catch bugs and edge cases\n- Ensure code maintainability\n- Share knowledge across team\n- Enforce coding standards\n- Improve design and architecture\n- Build team culture\n\n**Not the Goals:**\n\n- Show off knowledge\n- Nitpick formatting (use linters)\n- Block progress unnecessarily\n- Rewrite to your preference\n\n### 2. Effective Feedback\n\n**Good Feedback is:**\n\n- Specific and actionable\n- Educational, not judgmental\n- Focused on the code, not the person\n- Balanced (praise good work too)\n- Prioritized (critical vs nice-to-have)\n\n```markdown\n❌ Bad: \"This is wrong.\"\n✅ Good: \"This could cause a race condition when multiple users\naccess simultaneously. Consider using a mutex here.\"\n\n❌ Bad: \"Why didn't you use X pattern?\"\n✅ Good: \"Have you considered the Repository pattern? It would\nmake this easier to test. Here's an example: [link]\"\n\n❌ Bad: \"Rename this variable.\"\n✅ Good: \"[nit] Consider `userCount` instead of `uc` for\nclarity. Not blocking if you prefer to keep it.\"\n```\n\n### 3. Review Scope\n\n**What to Review:**\n\n- Logic correctness and edge cases\n- Security vulnerabilities\n- Performance implications\n- Test coverage and quality\n- Error handling\n- Documentation and comments\n- API design and naming\n- Architectural fit\n\n**What Not to Review Manually:**\n\n- Code formatting (use Prettier, Black, etc.)\n- Import organization\n- Linting violations\n- Simple typos\n\n## Review Process\n\n### Phase 1: Context Gathering (2-3 minutes)\n\n```markdown\nBefore diving into code, understand:\n\n1. Read PR description and linked issue\n2. Check PR size (>400 lines? Ask to split)\n3. Review CI/CD status (tests passing?)\n4. Understand the business requirement\n5. Note any relevant architectural decisions\n```\n\n### Phase 2: High-Level Review (5-10 minutes)\n\n```markdown\n1. **Architecture & Design**\n   - Does the solution fit the problem?\n   - Are there simpler approaches?\n   - Is it consistent with existing patterns?\n   - Will it scale?\n\n2. **File Organization**\n   - Are new files in the right places?\n   - Is code grouped logically?\n   - Are there duplicate files?\n\n3. **Testing Strategy**\n   - Are there tests?\n   - Do tests cover edge cases?\n   - Are tests readable?\n```\n\n### Phase 3: Line-by-Line Review (10-20 minutes)\n\n```markdown\nFor each file:\n\n1. **Logic & Correctness**\n   - Edge cases handled?\n   - Off-by-one errors?\n   - Null/undefined checks?\n   - Race conditions?\n\n2. **Security**\n   - Input validation?\n   - SQL injection risks?\n   - XSS vulnerabilities?\n   - Sensitive data exposure?\n\n3. **Performance**\n   - N+1 queries?\n   - Unnecessary loops?\n   - Memory leaks?\n   - Blocking operations?\n\n4. **Maintainability**\n   - Clear variable names?\n   - Functions doing one thing?\n   - Complex code commented?\n   - Magic numbers extracted?\n```\n\n### Phase 4: Summary & Decision (2-3 minutes)\n\n```markdown\n1. Summarize key concerns\n2. Highlight what you liked\n3. Make clear decision:\n   - ✅ Approve\n   - 💬 Comment (minor suggestions)\n   - 🔄 Request Changes (must address)\n4. Offer to pair if complex\n```\n\n## Review Techniques\n\n### Technique 1: The Checklist Method\n\n```markdown\n## Security Checklist\n\n- [ ] User input validated and sanitized\n- [ ] SQL queries use parameterization\n- [ ] Authentication/authorization checked\n- [ ] Secrets not hardcoded\n- [ ] Error messages don't leak info\n\n## Performance Checklist\n\n- [ ] No N+1 queries\n- [ ] Database queries indexed\n- [ ] Large lists paginated\n- [ ] Expensive operations cached\n- [ ] No blocking I/O in hot paths\n\n## Testing Checklist\n\n- [ ] Happy path tested\n- [ ] Edge cases covered\n- [ ] Error cases tested\n- [ ] Test names are descriptive\n- [ ] Tests are deterministic\n```\n\n### Technique 2: The Question Approach\n\nInstead of stating problems, ask questions to encourage thinking:\n\n```markdown\n❌ \"This will fail if the list is empty.\"\n✅ \"What happens if `items` is an empty array?\"\n\n❌ \"You need error handling here.\"\n✅ \"How should this behave if the API call fails?\"\n\n❌ \"This is inefficient.\"\n✅ \"I see this loops through all users. Have we considered\nthe performance impact with 100k users?\"\n```\n\n### Technique 3: Suggest, Don't Command\n\n````markdown\n## Use Collaborative Language\n\n❌ \"You must change this to use async/await\"\n✅ \"Suggestion: async/await might make this more readable:\n`typescript\n    async function fetchUser(id: string) {\n        const user = await db.query('SELECT * FROM users WHERE id = ?', id);\n        return user;\n    }\n    `\nWhat do you think?\"\n\n❌ \"Extract this into a function\"\n✅ \"This logic appears in 3 places. Would it make sense to\nextract it into a shared utility function?\"\n````\n\n### Technique 4: Differentiate Severity\n\n```markdown\nUse labels to indicate priority:\n\n🔴 [blocking] - Must fix before merge\n🟡 [important] - Should fix, discuss if disagree\n🟢 [nit] - Nice to have, not blocking\n💡 [suggestion] - Alternative approach to consider\n📚 [learning] - Educational comment, no action needed\n🎉 [praise] - Good work, keep it up!\n\nExample:\n\"🔴 [blocking] This SQL query is vulnerable to injection.\nPlease use parameterized queries.\"\n\n\"🟢 [nit] Consider renaming `data` to `userData` for clarity.\"\n\n\"🎉 [praise] Excellent test coverage! This will catch edge cases.\"\n```\n\n## Language-Specific Patterns\n\n### Python Code Review\n\n```python\n# Check for Python-specific issues\n\n# ❌ Mutable default arguments\ndef add_item(item, items=[]):  # Bug! Shared across calls\n    items.append(item)\n    return items\n\n# ✅ Use None as default\ndef add_item(item, items=None):\n    if items is None:\n        items = []\n    items.append(item)\n    return items\n\n# ❌ Catching too broad\ntry:\n    result = risky_operation()\nexcept:  # Catches everything, even KeyboardInterrupt!\n    pass\n\n# ✅ Catch specific exceptions\ntry:\n    result = risky_operation()\nexcept ValueError as e:\n    logger.error(f\"Invalid value: {e}\")\n    raise\n\n# ❌ Using mutable class attributes\nclass User:\n    permissions = []  # Shared across all instances!\n\n# ✅ Initialize in __init__\nclass User:\n    def __init__(self):\n        self.permissions = []\n```\n\n### TypeScript/JavaScript Code Review\n\n```typescript\n// Check for TypeScript-specific issues\n\n// ❌ Using any defeats type safety\nfunction processData(data: any) {  // Avoid any\n    return data.value;\n}\n\n// ✅ Use proper types\ninterface DataPayload {\n    value: string;\n}\nfunction processData(data: DataPayload) {\n    return data.value;\n}\n\n// ❌ Not handling async errors\nasync function fetchUser(id: string) {\n    const response = await fetch(`/api/users/${id}`);\n    return response.json();  // What if network fails?\n}\n\n// ✅ Handle errors properly\nasync function fetchUser(id: string): Promise<User> {\n    try {\n        const response = await fetch(`/api/users/${id}`);\n        if (!response.ok) {\n            throw new Error(`HTTP ${response.status}`);\n        }\n        return await response.json();\n    } catch (error) {\n        console.error('Failed to fetch user:', error);\n        throw error;\n    }\n}\n\n// ❌ Mutation of props\nfunction UserProfile({ user }: Props) {\n    user.lastViewed = new Date();  // Mutating prop!\n    return <div>{user.name}</div>;\n}\n\n// ✅ Don't mutate props\nfunction UserProfile({ user, onView }: Props) {\n    useEffect(() => {\n        onView(user.id);  // Notify parent to update\n    }, [user.id]);\n    return <div>{user.name}</div>;\n}\n```\n\n## Advanced Review Patterns\n\n### Pattern 1: Architectural Review\n\n```markdown\nWhen reviewing significant changes:\n\n1. **Design Document First**\n   - For large features, request design doc before code\n   - Review design with team before implementation\n   - Agree on approach to avoid rework\n\n2. **Review in Stages**\n   - First PR: Core abstractions and interfaces\n   - Second PR: Implementation\n   - Third PR: Integration and tests\n   - Easier to review, faster to iterate\n\n3. **Consider Alternatives**\n   - \"Have we considered using [pattern/library]?\"\n   - \"What's the tradeoff vs. the simpler approach?\"\n   - \"How will this evolve as requirements change?\"\n```\n\n### Pattern 2: Test Quality Review\n\n```typescript\n// ❌ Poor test: Implementation detail testing\ntest('increments counter variable', () => {\n    const component = render(<Counter />);\n    const button = component.getByRole('button');\n    fireEvent.click(button);\n    expect(component.state.counter).toBe(1);  // Testing internal state\n});\n\n// ✅ Good test: Behavior testing\ntest('displays incremented count when clicked', () => {\n    render(<Counter />);\n    const button = screen.getByRole('button', { name: /increment/i });\n    fireEvent.click(button);\n    expect(screen.getByText('Count: 1')).toBeInTheDocument();\n});\n\n// Review questions for tests:\n// - Do tests describe behavior, not implementation?\n// - Are test names clear and descriptive?\n// - Do tests cover edge cases?\n// - Are tests independent (no shared state)?\n// - Can tests run in any order?\n```\n\n### Pattern 3: Security Review\n\n```markdown\n## Security Review Checklist\n\n### Authentication & Authorization\n\n- [ ] Is authentication required where needed?\n- [ ] Are authorization checks before every action?\n- [ ] Is JWT validation proper (signature, expiry)?\n- [ ] Are API keys/secrets properly secured?\n\n### Input Validation\n\n- [ ] All user inputs validated?\n- [ ] File uploads restricted (size, type)?\n- [ ] SQL queries parameterized?\n- [ ] XSS protection (escape output)?\n\n### Data Protection\n\n- [ ] Passwords hashed (bcrypt/argon2)?\n- [ ] Sensitive data encrypted at rest?\n- [ ] HTTPS enforced for sensitive data?\n- [ ] PII handled according to regulations?\n\n### Common Vulnerabilities\n\n- [ ] No eval() or similar dynamic execution?\n- [ ] No hardcoded secrets?\n- [ ] CSRF protection for state-changing operations?\n- [ ] Rate limiting on public endpoints?\n```\n\n## Giving Difficult Feedback\n\n### Pattern: The Sandwich Method (Modified)\n\n```markdown\nTraditional: Praise + Criticism + Praise (feels fake)\n\nBetter: Context + Specific Issue + Helpful Solution\n\nExample:\n\"I noticed the payment processing logic is inline in the\ncontroller. This makes it harder to test and reuse.\n\n[Specific Issue]\nThe calculateTotal() function mixes tax calculation,\ndiscount logic, and database queries, making it difficult\nto unit test and reason about.\n\n[Helpful Solution]\nCould we extract this into a PaymentService class? That\nwould make it testable and reusable. I can pair with you\non this if helpful.\"\n```\n\n### Handling Disagreements\n\n```markdown\nWhen author disagrees with your feedback:\n\n1. **Seek to Understand**\n   \"Help me understand your approach. What led you to\n   choose this pattern?\"\n\n2. **Acknowledge Valid Points**\n   \"That's a good point about X. I hadn't considered that.\"\n\n3. **Provide Data**\n   \"I'm concerned about performance. Can we add a benchmark\n   to validate the approach?\"\n\n4. **Escalate if Needed**\n   \"Let's get [architect/senior dev] to weigh in on this.\"\n\n5. **Know When to Let Go**\n   If it's working and not a critical issue, approve it.\n   Perfection is the enemy of progress.\n```\n\n## Best Practices\n\n1. **Review Promptly**: Within 24 hours, ideally same day\n2. **Limit PR Size**: 200-400 lines max for effective review\n3. **Review in Time Blocks**: 60 minutes max, take breaks\n4. **Use Review Tools**: GitHub, GitLab, or dedicated tools\n5. **Automate What You Can**: Linters, formatters, security scans\n6. **Build Rapport**: Emoji, praise, and empathy matter\n7. **Be Available**: Offer to pair on complex issues\n8. **Learn from Others**: Review others' review comments\n\n## Common Pitfalls\n\n- **Perfectionism**: Blocking PRs for minor style preferences\n- **Scope Creep**: \"While you're at it, can you also...\"\n- **Inconsistency**: Different standards for different people\n- **Delayed Reviews**: Letting PRs sit for days\n- **Ghosting**: Requesting changes then disappearing\n- **Rubber Stamping**: Approving without actually reviewing\n- **Bike Shedding**: Debating trivial details extensively\n\n## Templates\n\n### PR Review Comment Template\n\n```markdown\n## Summary\n\n[Brief overview of what was reviewed]\n\n## Strengths\n\n- [What was done well]\n- [Good patterns or approaches]\n\n## Required Changes\n\n🔴 [Blocking issue 1]\n🔴 [Blocking issue 2]\n\n## Suggestions\n\n💡 [Improvement 1]\n💡 [Improvement 2]\n\n## Questions\n\n❓ [Clarification needed on X]\n❓ [Alternative approach consideration]\n\n## Verdict\n\n✅ Approve after addressing required changes\n```","author":"@wshobson","ownerProfile":null,"authorContacts":null,"sourceUrl":"https://github.com/wshobson/agents/tree/main/plugins/developer-essentials/skills/code-review-excellence","license":"MIT","category":"review","lang":"en","tokens":3062,"stars":0,"calls30d":2,"claimed":false,"visibility":"public","origin":"crawler","version":"0.1.0","createdAt":"2026-08-22","updatedAt":"2026-08-22","files":[],"requires":{"mcp":[],"tools":[]},"safety":{"flags":[{"code":"code.eval","kind":"dangerous-code","where":"SKILL.md:417","excerpt":"eval(","message":"evaluates code at runtime","severity":"warn"}],"scannedAt":"2026-08-22","hasScripts":false,"networkEndpoints":[]}}