Code Review

Overview

Code review requires technical evaluation, not emotional performance. Request reviews early, receive feedback with rigor.

Core principles:

•Requesting: Review early, review often. Catch issues before they cascade.
•Receiving: Verify before implementing. Technical correctness over social comfort.

Announce at start: "I'm using gambit:code-review to [request/receive] review."

Rigidity Level

MEDIUM FREEDOM - Follow the self-review checklist strictly before requesting. Verify all feedback before implementing. Adapt review scope to context. No performative agreement.

Quick Reference

Requesting Review

Step	Action	STOP If
1	Run self-review checklist	Critical issues found
2	Dispatch code-reviewer agent	-
3	Act on feedback by severity	Critical unfixed

Receiving Review

Step	Action	STOP If
1	Read complete feedback	-
2	Restate requirement in own words	Any item unclear
3	Verify against codebase	Feedback breaks things
4	Implement one item at a time	Tests fail

Forbidden responses: "You're absolutely right!", "Great point!", "Thanks for catching that!"

Part 1: Requesting Review

When to Request

Mandatory:

•After completing Task from epic
•After completing major feature
•Before merge to main

Optional but valuable:

•When stuck (fresh perspective)
•Before refactoring (baseline check)
•After fixing complex bug

Step 1: Self-Review Checklist

BEFORE dispatching reviewer, verify:

bash

# Run all automated checks
Task
  subagent_type: "hyperpowers:test-runner"
  prompt: "Run: go test ./... && golangci-lint run"

Automated checks:

• All tests pass
• No linter warnings
• No TODOs without issue numbers: grep -r "TODO" src/ | grep -v "#"
• No stub implementations: grep -r "unimplemented\|todo!\|panic" src/
• No unsafe patterns in production: grep -r "\.unwrap()\|\.expect(" src/

Code quality self-check:

• Error handling proper (Result/Option, not panic)
• Edge cases handled (empty, nil, boundaries)
• Names clear (would junior understand in 6 months?)
• No "while I'm here" changes mixed in
• Tests meaningful (not tautological)

If any critical issues found: Fix before requesting review.

Step 2: Dispatch Code-Reviewer Agent

First, get git SHAs:

bash

BASE_SHA=$(git rev-parse HEAD~N)  # or origin/main for full branch diff
HEAD_SHA=$(git rev-parse HEAD)

Then dispatch with this template:

code

Task
  subagent_type: "feature-dev:code-reviewer"
  description: "Review [brief description]"
  prompt: |
    Review the changes in this commit range:

    ## What Was Implemented
    {WHAT_WAS_IMPLEMENTED}
    [Brief description of what you built]

    ## Requirements
    {PLAN_OR_REQUIREMENTS}
    [What it should do - from Task or epic]

    ## Commit Range
    Base: {BASE_SHA}
    Head: {HEAD_SHA}

    ## Specific Concerns
    {CONCERNS}
    [Areas you're uncertain about]

    ## Review Focus
    - Does implementation match requirements?
    - Are there bugs, edge cases missed?
    - Is error handling sufficient?
    - Are tests meaningful (not tautological)?
    - Is code clear and maintainable?

    Return findings as:
    - Critical: [Must fix before merge]
    - Important: [Should fix before proceeding]
    - Minor: [Note for future]
    - Strengths: [What's done well]

Template placeholders:

Placeholder	Description
`{WHAT_WAS_IMPLEMENTED}`	What you just built
`{PLAN_OR_REQUIREMENTS}`	What it should do (from Task)
`{BASE_SHA}`	Starting commit
`{HEAD_SHA}`	Ending commit
`{CONCERNS}`	Areas you're uncertain about

Step 3: Act on Feedback

By severity:

Severity	Action
Critical	Fix immediately. Do not proceed until fixed.
Important	Fix before proceeding to next Task.
Minor	Note for later. OK to defer.

If reviewer wrong:

•Push back with technical reasoning
•Show code/tests that prove it works
•Request clarification

Part 2: Receiving Review

The Response Pattern

code

WHEN receiving feedback:

1. READ: Complete feedback without reacting
2. UNDERSTAND: Restate requirement in own words (or ask)
3. VERIFY: Check against codebase reality
4. EVALUATE: Technically sound for THIS codebase?
5. RESPOND: Technical acknowledgment or reasoned pushback
6. IMPLEMENT: One item at a time, test each

Forbidden Responses

NEVER:

•"You're absolutely right!" (performative)
•"Great point!" / "Excellent feedback!" (performative)
•"Thanks for catching that!" (performative)
•"Let me implement that now" (before verification)

INSTEAD:

•Restate the technical requirement
•Ask clarifying questions
•Push back with technical reasoning if wrong
•Just start working (actions > words)

Handling Unclear Feedback

IF any item is unclear:

code

STOP - do not implement anything yet
ASK for clarification on unclear items

Why: Items may be related. Partial understanding = wrong implementation.

Example:

code

Reviewer: "Fix items 1-6"
You understand 1,2,3,6. Unclear on 4,5.

❌ WRONG: Implement 1,2,3,6 now, ask about 4,5 later
✅ RIGHT: "I understand items 1,2,3,6. Need clarification on 4 and 5 before proceeding."

Verifying Feedback Before Implementing

BEFORE implementing external feedback:

•Check: Technically correct for THIS codebase?
•Check: Breaks existing functionality?
•Check: Reason for current implementation?
•Check: Works on all platforms/versions?
•Check: Does reviewer understand full context?

IF suggestion seems wrong:

•Push back with technical reasoning
•Show code/tests that prove current approach works

IF can't easily verify:

•Say so: "I can't verify this without [X]. Should I investigate?"

IF conflicts with prior architectural decisions:

•Stop and discuss before implementing

YAGNI Check for "Professional" Features

IF reviewer suggests "implementing properly":

bash

# Check if feature is actually used
grep -r "functionName" src/

•If unused: "This function isn't called. Remove it (YAGNI)?"
•If used: Then implement properly

Implementation Order

FOR multi-item feedback:

•Clarify anything unclear FIRST
•
Then implement in this order:
- •Blocking issues (breaks, security)
- •Simple fixes (typos, imports)
- •Complex fixes (refactoring, logic)
•Test each fix individually
•Verify no regressions

When to Push Back

Push back when:

•Suggestion breaks existing functionality
•Reviewer lacks full context
•Violates YAGNI (unused feature)
•Technically incorrect for this stack
•Legacy/compatibility reasons exist
•Conflicts with architectural decisions

How to push back:

•Use technical reasoning, not defensiveness
•Ask specific questions
•Reference working tests/code
•Escalate if architectural

Acknowledging Correct Feedback

When feedback IS correct:

code

✅ "Fixed. [Brief description of what changed]"
✅ "Good catch - [specific issue]. Fixed in [location]."
✅ [Just fix it and show in the code]

❌ "You're absolutely right!"
❌ "Great point!"
❌ "Thanks for catching that!"

Why no performative agreement: Actions speak. Just fix it. The code itself shows you heard.

Correcting Your Pushback

If you pushed back and were wrong:

code

✅ "You were right - I checked [X] and it does [Y]. Implementing now."
✅ "Verified and you're correct. My understanding was wrong because [reason]. Fixing."

❌ Long apology
❌ Defending why you pushed back
❌ Over-explaining

State the correction factually and move on.

Critical Rules

Rules That Have No Exceptions

•Run self-review checklist before requesting → Don't waste reviewer time on obvious issues
•No performative agreement → Technical acknowledgment or pushback only
•Verify feedback before implementing → Check if it breaks things
•Clarify unclear items FIRST → Don't implement partial understanding
•Test each fix individually → One at a time, verify no regressions
•Push back with technical reasoning → Not defensiveness, not blind acceptance

Common Mistakes

Mistake	Fix
Performative agreement	State requirement or just act
Blind implementation	Verify against codebase first
Batch without testing	One at a time, test each
Assuming reviewer is right	Check if breaks things
Avoiding pushback	Technical correctness > comfort
Partial implementation	Clarify all items first
Skip self-review	Always run checklist before requesting

Examples

Bad: Performative Agreement

code

Reviewer: "Remove legacy code"

❌ "You're absolutely right! Let me remove that..."
[Removes code without checking if it's actually unused]
[Build breaks because legacy code was still called]

Good: Technical Verification

code

Reviewer: "Remove legacy code"

✅ "Checking..."
$ grep -r "legacyFunction" src/
src/handler.go:45: result := legacyFunction(data)

"legacyFunction is still called at handler.go:45. Should I:
A) Update handler to use new function, then remove legacy?
B) Keep legacy for now until handler is migrated?
C) Something else?"

Bad: Blind Implementation

code

Reviewer: "Use async/await pattern here"

[Implements async/await without checking]
[Breaks because caller doesn't handle promises]
[Introduces race condition in tests]

Good: Verify Then Implement

code

Reviewer: "Use async/await pattern here"

✅ "Checking callers..."
$ grep -r "functionName" src/
3 callers found, all synchronous

"Converting to async would require updating 3 callers:
- handler.go:45
- service.go:78
- batch.go:112

Want me to update all callers, or keep synchronous for now?"

YAGNI Example

code

Reviewer: "Implement proper metrics tracking with database, date filters, CSV export"

✅ "Checking usage..."
$ grep -r "metricsEndpoint" src/
[no results]

"Nothing calls metricsEndpoint. Remove it (YAGNI)?
Or is there usage I'm missing?"

Unclear Feedback Example

code

Reviewer: "Fix items 1-6"

Items 1,2,3,6: Clear
Items 4,5: Unclear what specifically to change

❌ WRONG:
"Fixing 1,2,3,6 now..."
[Implements partial, asks about 4,5 after]

✅ RIGHT:
"I understand items 1,2,3,6. Need clarification on 4 and 5:
- Item 4 mentions 'proper error handling' - which function?
- Item 5 says 'update tests' - add new tests or modify existing?

Will implement all 6 once clarified."

Verification Checklist

Before Requesting Review

After Receiving Review

• Read complete feedback without reacting
• Restated unclear items (or asked for clarification)
• Verified feedback against codebase
• Evaluated if technically sound
• Pushed back on incorrect suggestions (with reasoning)
• Implemented one item at a time
• Tested after each fix
• Verified no regressions

Can't check all boxes? Return to process.

Integration

This skill calls:

•gambit:verify (for self-review verification)
•feature-dev:code-reviewer agent (for requesting review)
•test-runner agent (for running tests)

This skill is called by:

•gambit:execute-plan (after each Task)
•Before merging to main
•When stuck and need fresh perspective

Workflow:

code

Complete Task
    ↓
gambit:code-review (requesting)
    ↓
Self-review checklist
    ↓
Dispatch code-reviewer agent
    ↓
Receive feedback
    ↓
gambit:code-review (receiving)
    ↓
Verify → Implement → Test
    ↓
Ready for next Task

Resources

Self-review patterns:

•Run automated checks FIRST
•Check edge cases: empty, nil, max, unicode
•Ask "would junior understand this in 6 months?"
•Check test meaningfulness (not tautological)

When to push back:

•Breaks existing functionality → Show failing test
•YAGNI → Show grep proving unused
•Wrong for this stack → Explain constraint
•Lacks context → Provide missing context

When stuck:

•Feedback unclear → Ask before implementing
•Feedback seems wrong → Verify, then push back with evidence
•Feedback breaks things → Report what breaks, ask how to proceed
•Multiple items → Clarify all, then implement in order