cm-code-review

Code Review — Request + Receive + Complete

TL;DR

• Use when task complete, before/during PR review
• Pre-review: lint, tests, diff scan, blast radius
• Reads: handoff/exec.json — Writes: handoff/review.json
• Severity: info | warn | error | critical
• Next: cm-quality-gate

Full review lifecycle in one skill: Request → Receive → Integrate.

Part A: Requesting Code Review

When to Request

Mandatory:

• After each task in cm-execution
• After completing major features
• Before merge to main

Optional but valuable:

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

How to Request

1. Get git SHAs:

   BASE_SHA=$(git rev-parse HEAD~1)
   HEAD_SHA=$(git rev-parse HEAD)
   

2. Dispatch reviewer subagent with:

   • What was implemented
   • Plan/requirements reference
   • Base and head SHAs
   • Brief description

3. Act on feedback:

   • Fix Critical issues immediately
   • Fix Important issues before proceeding
   • Note Minor issues for later
   • Push back if reviewer is wrong (with reasoning)

---

Part B: Receiving Code Review

When to Use

When receiving feedback — whether from human reviewers, AI reviewers, or code review subagents.

The Protocol

1. READ feedback completely before responding
2. UNDERSTAND the technical reasoning
3. VERIFY if the feedback is technically correct
4. RESPOND with evidence, not agreement

Response Framework

Feedback Type	Response
Technically correct	Fix it. Thank reviewer.
Unclear intent	Ask for clarification with specific questions
Technically questionable	Challenge with evidence (code, tests, docs)
Stylistic preference	Discuss trade-offs, defer to team convention

Red Flags — STOP

• Blindly implementing all suggestions without verification
• "Performative agreement" — saying yes without understanding
• Implementing a suggestion that breaks existing tests
• Making changes you can't justify technically

Anti-Pattern: Performative Agreement

❌ "Good catch! Fixed."  (without verifying it's actually a problem)
✅ "I verified this: [evidence]. The suggestion is correct because [reason]. Fixed."
✅ "I investigated this: [evidence]. The current code is correct because [reason]."

---

Part C: Finishing a Development Branch

When to Use

When implementation is complete and all tests pass.

The Process

1. Verify current state:

   npm run test:gate  # All tests must pass
   git status          # Working tree should be clean
   

2. Present options to user:

   Option	When	Command
   Merge to main	Feature ready	git checkout main && git merge feature-branch
   Create PR	Needs team review	git push origin feature-branch
   Keep working	More tasks remain	Continue on branch
   Cleanup only	Abandoned/merged	git worktree remove path

3. Execute chosen option

4. Cleanup:

   • Remove the isolated worktree/workspace if your execution flow created one
   • Delete feature branch if merged
   • Update task tracking

Rules

• Never merge with failing tests
• Never force push main/production
• Always use cm-identity-guard before git push

---

Step FINAL: Record Review Learnings

After processing review feedback, ALWAYS update .cm/CONTINUITY.md:

• Key Decisions: If reviewer changed architecture approach, record with scope: [Decision]: [Rationale] — scope: [global|module:{name}]
• Mistakes & Learnings: If reviewer caught a pattern mistake, record with scope:
  • What Failed: [the pattern that was wrong]
  • How to Prevent: [correct pattern going forward]
  • Scope: [global | module:{name} | file:{path}]

Anti-duplicate: If similar learning exists, reinforce it instead of creating new.

Token savings: Future code reviews in same project avoid repeating the same feedback. Reviewer patterns become accumulated knowledge.

---

Integration

Skill	Relationship
cm-execution	Reviews after each task in execution
cm-quality-gate	Tests must pass before finishing branch
cm-identity-guard	Before git push
cm-execution	Cleanup isolated workspace after completion

Karpathy Discipline — Review Checklist

Block any PR/diff that violates these regardless of green tests:

• Scope creep: changed line that doesn't trace to the task → request removal.
• Bloat: new abstraction with one caller, premature config, error paths for impossible inputs → request simplification.
• Side-effect edits: unrelated formatting, renamed identifiers, "improved" comments not in scope → revert.
• Hidden assumptions: logic that depends on an unstated invariant → require it surfaced (test or doc) or rejected.
• Weak goals: PR description says "make it work" with no verifiable criterion → send back to planning.

Review heuristic: if you can't answer "what task forced this line?" for every changed line, the diff is too wide.

The Bottom Line

Review early. Verify feedback. Ship with evidence, not hope.