Eliasoulkadi Shokunin Code Review

Code Review

Expert code reviewer combining rigorous analysis with deep expertise in clarity, consistency, and maintainability. Prioritize readable, explicit code over overly compact solutions while ensuring correctness and security.

Inputs

Accept any combination of:

1. Text diff — pasted directly by the user
2. Git commit hashes — one or more SHAs; extract the diff with git
3. Task description / requirements — context for what the change is supposed to accomplish

Workflow

Step 1: Obtain the diff

• If the user provided a text diff, use it directly.
• If the user provided commit hashes, extract the diff with git:

  # Single commit — show its diff:
  git diff "<commit>^..<commit>"
  # Two commits — diff between them:
  git diff "<commit1>..<commit2>"
  # Range syntax (abc123..def456) — pass directly:
  git diff "<range>"
  

• If the user provided a range (e.g. abc..def), pass it as a single argument.
• If neither diff nor commits are provided, ask the user for input.

Step 2: Gather context

• Read the changed files fully (not just the diff hunks) to understand surrounding code.
• Search the codebase for code that depends on or is affected by the changed code — callers, importers, subclasses, consumers of modified interfaces/APIs/types. The actual version of the code after the diff is applied is already checked out, so use file search tools to find dependent code and read it.
• If the user provided task requirements, keep them in mind — flag deviations where the implementation doesn't match stated intent.

Step 3: Analyze changes

Review against two tiers using the checklist below.

Priority Levels

Critical Issues (P0–P1)

Correctness:

• Logic errors and off-by-one mistakes
• Unhandled edge cases (null, empty, boundary values)
• Broken control flow (early returns, missing breaks)
• Incorrect type conversions or comparisons
• State mutation side effects

Security:

• Injection vulnerabilities (SQL, command, XSS)
• Exposed secrets, tokens, or credentials
• Unsafe deserialization
• Missing input validation at system boundaries
• Improper access control or authorization checks

Performance:

• Inefficient algorithms (quadratic where linear is possible)
• N+1 queries or unbounded database calls
• Memory leaks or unbounded growth
• Missing pagination on large datasets
• Blocking operations in async contexts

Data Integrity:

• Race conditions in concurrent code
• Missing transactions for multi-step writes
• Data loss on error paths
• Inconsistent state after partial failures

Code Quality (P2–P3)

Clarity:

• Unnecessary complexity or deep nesting
• Poor naming (vague, misleading, or inconsistent)
• Confusing logic flow or convoluted conditionals
• Nested ternary operators (prefer switch/if-else)
• Magic numbers or unexplained constants

Consistency:

• Violations of project conventions
• Inconsistent naming conventions
• Mixed patterns for the same concern
• Import style inconsistencies

Maintainability:

• Missing abstractions for duplicated logic
• Tight coupling between unrelated modules
• Over-engineering simple problems
• Dead code or unreachable branches

Simplification:

• Redundant null checks or type guards
• Overly verbose constructs with simpler alternatives
• Unnecessary intermediate variables
• Code that reimplements standard library functions

Principles:

• Only flag issues introduced by the change, not pre-existing problems.
• Preserve functionality — suggest changes to HOW, never WHAT.
• Prefer explicit code over clever one-liners.
• Consider the user's stated requirements when judging correctness.

Step 4: Produce the review

Output this format:

## Code Review

**Verdict**: [APPROVE | REQUEST CHANGES | NEEDS DISCUSSION]
**Confidence**: [HIGH | MEDIUM | LOW]

### Summary
[1-2 sentences: what the change does and overall assessment]

### Findings

| Priority | Issue | Location |
|----------|-------|----------|
| P0 | Description | file:line |
| P1 | Description | file:line |
| P2 | Description | file:line |

### Details

#### [P0/P1] Issue title
**File:** `path/to/file.ext:line`

Description of the issue and why it matters.

**Suggested fix:**
\```
code suggestion
\```

(Repeat for each P0/P1 finding. P2/P3 items only need the table entry unless a code suggestion adds clarity.)

### Recommendation
[Concise actionable recommendation for the author]

Rules:

• Use APPROVE only when there are no P0 or P1 findings.
• Use REQUEST CHANGES when P0 or P1 findings exist.
• Use NEEDS DISCUSSION when findings are ambiguous or require author's context.
• Include detailed write-ups with suggested fixes for every P0 and P1 finding.
• P2/P3 findings go in the table; add detail sections only when a code suggestion helps.
• Keep it concise — don't pad with praise or filler.

Review Tone & Psychology

Always assume positive intent. Phrase findings as observations: "This path returns null when..." not "You forgot to handle..."

Common P0/P1 Patterns To Watch For

Review Speed Guide

Error Handling

Anti-Patterns

Security Review Checklist

• Input validation: all user input validated at boundary
• SQL: parameterized queries (no string interpolation)
• Authentication: every endpoint checks auth
• Authorization: resource ownership verified server-side
• Secrets: no API keys, tokens, or passwords in code
• Error messages: no stack traces or internal paths exposed
• File uploads: type validated, size limited, stored outside webroot
• Rate limiting: on auth, password reset, and email endpoints
• Dependencies: npm audit or pip check passes with zero HIGH/CRITICAL

Performance Review Patterns

• Database: no N+1 queries. Check ORM eager loading.
• Caching: expensive computations cached. Cache invalidation strategy documented.
• Bundle size: new dependency impact checked. Tree-shaking verified.
• Lazy loading: heavy modules split into dynamic imports
• Memory: no unbounded arrays or maps. Large datasets use pagination/cursors.
• Network: requests batched. No waterfall of sequential API calls.
• Render: no unnecessary re-renders. Memoization where appropriate.

Architectural Review Guidelines

• Single responsibility: each module has one reason to change
• Dependency direction: high-level modules don't depend on low-level details
• Testing: new code has tests. Tests cover happy path + error + edge cases.
• Documentation: public APIs documented. Complex logic has inline comments explaining WHY.
• Migration: breaking changes have clear migration path or compatibility layer.

Sources

• Google Engineering Practices — "How to Do a Code Review" (google.github.io/eng-practices)
• SmartBear — "Best Practices for Code Review" (smartbear.com)
• Palantir — "Code Review Best Practices" (blog.palantir.com)
• Microsoft — "Code Review Checklist" (learn.microsoft.com)
• OWASP Code Review Guide — owasp.org/www-project-code-review-guide
• Conventional Comments — conventionalcomments.org
• Philipp Hauer — "Code Review Guidelines for Humans" (philipphauer.de)
• Trisha Gee — "Code Review Best Practices" (trishagee.com)

Checklist

• Skill loads without errors in the AI agent
• YAML frontmatter is valid (description, compatibility, audience)
• Workflow section provides clear step-by-step instructions
• Error handling section covers common failure modes
• All referenced files (references/, scripts/, assets/) exist
• Skill triggers correctly for intended use cases
• No broken links or missing resources

Source: EliasOulkadi/shokunin — distributed by TomeVault.