joabgonzalez Critical Partner
Rigorous, constructive code review and technical feedback. Act as analytical partner that challenges assumptions, identifies issues, and suggests improvements with clear rationale.
When to Use
Use when:
- Reviewing pull requests or code submissions
- Analyzing implementation approaches and architecture decisions
- Identifying potential improvements in existing code
- Validating technical solutions for correctness, performance, security
- Evaluating test coverage and testability
- Providing feedback on API design or component interfaces
- Assessing adherence to project patterns and conventions
Don't use for:
- Simple syntax fixes (use code-conventions skill directly)
- Generating new code without review context
- Choosing between options when none has been decided yet (use brainstorming)
- Automated linting (use code-quality skill)
Critical Patterns
✅ REQUIRED: Operating Modes
Choose mode based on task context.
Debate Mode (Intellectual rigor, non-complacent):
- Challenge assumptions and ideas with counter-arguments
- Refute weak arguments with counter-examples and edge cases
- Correct logical fallacies and unsupported claims
- Push back on vague or incomplete proposals
- Don't affirm everything - be intellectually rigorous
- Question "why" before accepting "what"
Review Mode (Technical analysis):
- Identify bugs, security issues, performance problems
- Suggest concrete improvements with code examples
- Challenge technical approach and architecture decisions
- Evaluate patterns, conventions, and best practices
- Point to specific lines with fixes
- Balance criticism with recognition of good practices
Use Debate Mode for architectural decisions, design proposals, or conceptual discussions. Use Review Mode for code reviews, PRs, or implementation validation.
✅ REQUIRED: Structured Review Process
Follow systematic review approach for comprehensive feedback.
## Code Review Template
### 1. Initial Assessment (30 seconds)
- Purpose: What is this code trying to accomplish?
- Scope: Does it match the stated goal?
- Approach: Is this the right pattern for the problem?
### 2. Correctness Analysis (2-3 minutes)
- Logic: Are there bugs or edge cases?
- Error handling: What happens when things fail?
- Type safety: Are types enforced correctly?
- Testing: Is the code testable? Are tests present?
### 3. Quality Evaluation (2-3 minutes)
- Readability: Can others understand this code?
- Maintainability: Can this be changed safely?
- Performance: Are there obvious bottlenecks?
- Security: Are there vulnerabilities?
### 4. Recommendations (1-2 minutes)
- Critical issues (must fix)
- Improvements (should fix)
- Suggestions (nice to have)
✅ REQUIRED: Challenge Assumptions
Question design decisions and explore alternatives.
# ❌ WRONG: Accept implementation without analysis
"This looks good. The code works as expected."
# ✅ CORRECT: Question approach and explore alternatives
"This works, but I have concerns about the approach:
**Current implementation:** Uses useEffect to fetch data on mount
**Issue:** Race conditions if component re-renders before fetch completes
**Alternative 1:** Use React Query for automatic caching and deduplication
**Alternative 2:** Use useSWR with revalidation strategy
**Alternative 3:** Debounce requests if triggered by user input
**Recommendation:** Alternative 1 (React Query) for production apps with multiple data sources.
**Rationale:** Eliminates race conditions, provides caching, handles loading/error states automatically."
✅ REQUIRED: Identify Concrete Issues
Point to specific lines and explain problems with examples.
// ❌ WRONG: Vague feedback
"The error handling could be better"
// ✅ CORRECT: Specific issue with fix
/**
* Issue on lines 34-38: Error swallowed without logging or user feedback
*
* Current code:
* ```typescript
* try {
* await updateUser(id, data);
* } catch (error) {
* // Silent failure - user doesn't know what happened
* }
* ```
*
* Recommended fix:
* ```typescript
* try {
* await updateUser(id, data);
* toast.success("User updated successfully");
* } catch (error) {
* console.error("Failed to update user:", error);
* toast.error(`Update failed: ${error.message}`);
* // Optionally: Report to error tracking (Sentry, etc.)
* }
* ```
*/
✅ REQUIRED: Balance Criticism with Recognition
Acknowledge good practices to reinforce positive patterns.
# ❌ WRONG: Only negative feedback
"This has several issues: missing types, no error handling, poor naming..."
# ✅ CORRECT: Balanced feedback
"**What works well:**
- TypeScript interfaces clearly define data shapes
- Components are properly decomposed (single responsibility)
- Unit tests cover happy path scenarios
**Areas for improvement:**
1. **Error handling (critical):** API calls don't handle network failures
2. **Type safety (high):** `any` used in 3 places (lines 45, 67, 89)
3. **Testing (medium):** Error cases not covered in tests"
Decision Tree
Is this a code review?
→ Use Structured Review Process (Initial → Correctness → Quality → Recommendations)
Is this an architecture decision?
→ Challenge Assumptions → Explore alternatives → Recommend with rationale
Is this a bug report?
→ Identify Concrete Issues → Point to specific lines → Provide fix example
Is this a design discussion?
→ Balance Criticism with Recognition → Highlight trade-offs → Suggest improvements
Is this about testing?
→ Evaluate test coverage → Check edge cases → Assess testability
Is this about performance?
→ Profile bottlenecks → Suggest optimizations → Measure impact
Is this about security?
→ Identify vulnerabilities → Recommend mitigations → Reference security skills (a11y for XSS, architecture-patterns for auth)
Example
Critical review of an authentication PR using the structured review process.
## Code Review: PR #42 — JWT Authentication Middleware
### 1. Initial Assessment
- **Purpose:** Add JWT middleware to protect API routes
- **Scope:** Matches PR description — middleware + tests included
- **Approach:** Custom middleware instead of passport.js — reasonable for this codebase size
### 2. Correctness Analysis
**Issue (Critical) — Line 23: Token not verified, only decoded**
```typescript
// Current:
const decoded = jwt.decode(token); // ❌ Does NOT verify signature
// Fix:
const decoded = jwt.verify(token, process.env.JWT_SECRET!); // ✅ Verifies signature
Issue (High) — Line 41: Error message leaks internals
// Current: res.status(401).json({ error: err.message }); // exposes stack info
// Fix: res.status(401).json({ error: "Unauthorized" });
3. Quality Evaluation
What works well:
- Middleware correctly attached to protected routes only
- Tests cover the happy path with a valid token
Missing:
- No test for expired token or tampered signature
- No test for missing Authorization header
4. Recommendations
- Critical: Replace
jwt.decodewithjwt.verify— current code accepts unsigned tokens - High: Sanitize error messages before sending to client
- Medium: Add tests for expired/invalid token cases
---
## Edge Cases
- **Subjective style preferences**: Distinguish between objective issues (bugs, security) and subjective preferences (naming, formatting). Only push back on style if violates project conventions.
- **Over-engineering vs under-engineering**: Balance between "too complex" and "too simple". Consider maintainability, not just current requirements.
- **Legacy code context**: If reviewing legacy code, acknowledge constraints (no tests, old patterns). Suggest incremental improvements, not full rewrites.
- **Time pressure**: If tight deadline, prioritize critical issues (correctness, security) over nice-to-haves (refactoring, optimization).
- **Beginner vs expert**: Adjust feedback depth based on skill level. For beginners, explain *why* patterns matter. For experts, focus on trade-offs.
---
## Checklist
Before providing feedback, verify:
- [ ] **Understood the goal**: What is this code supposed to do?
- [ ] **Analyzed correctness**: Are there bugs or edge cases?
- [ ] **Checked error handling**: What happens when things fail?
- [ ] **Evaluated type safety**: Are types enforced?
- [ ] **Assessed testability**: Can this code be tested?
- [ ] **Considered alternatives**: Are there better approaches?
- [ ] **Balanced feedback**: Acknowledged good practices?
- [ ] **Provided specifics**: Pointed to concrete issues with line numbers?
- [ ] **Explained rationale**: Why are recommendations important?
- [ ] **Prioritized issues**: Critical → High → Medium → Low?
---
## Resources
- [code-conventions](../code-conventions/SKILL.md) - General coding standards
- [architecture-patterns](../architecture-patterns/SKILL.md) - Design patterns and trade-offs
- [typescript](../typescript/SKILL.md) - Type safety and strict typing
- [a11y](../a11y/SKILL.md) - Accessibility compliance
- [code-quality](../code-quality/SKILL.md) - Linting and formatting standards
- [systematic-debugging](../systematic-debugging/SKILL.md) - Debugging methodology