[Spec] Enforce Mandatory Code Review Before Merge/PR

[michael-newsrx]

Problem

During implementation, I created a Pull Request immediately after completing work, bypassing code review entirely.

This violates the requesting-code-review skill which says "Review early, review often" and "Mandatory: After completing major feature, Before merge to main".

Root Cause

The finishing-a-development-branch skill presents these options AFTER verification:

Implementation complete. What would you like to do?

1. Merge back to <base-branch> locally
2. Push and create a Pull Request
3. Keep the branch as-is (I'll handle it later)
4. Discard this work

These options allow bypassing code review. The skill should ENFORCE review before any merge/PR.

Proposed Fix

Add Code Review Step to finishing-a-development-branch

Replace the flow:

Current:

Step 1: Verify Tests
Step 2: Determine Base Branch
Step 3: Present Options
Step 4: Execute Choice

New:

Step 1: Verify Tests
Step 2: Determine Base Branch
Step 3: Request Code Review (REQUIRED)
Step 4: Address Review Feedback
Step 5: Present Options (after review passes)
Step 6: Execute Choice

Add Step 3: Request Code Review

NEW section in finishing-a-development-branch:

### Step 3: Request Code Review (REQUIRED)

**Before presenting options, request code review:**

1. Get base and head SHAs:
```bash
BASE_SHA=$(git merge-base HEAD <integration-branch>)
HEAD_SHA=$(git rev-parse HEAD)

2. Dispatch code-reviewer subagent:

Use Task tool with subagent_type: "general"
description: "Review code quality"

prompt: |
  You are reviewing code quality for completed work.
  
  **What was implemented:**
  [Brief description from spec]
  
  **Base commit:** {BASE_SHA}
  **Head commit:** {HEAD_SHA}
  
  Review for:
  - Code quality issues
  - Missing requirements
  - Test coverage
  - Documentation
  
  Report format:
  - Strengths: What's done well
  - Issues (Critical): Bugs, incorrect logic, will break
  - Issues (Important): Should fix - confusing names, missing error handling
  - Issues (Minor): Could fix - small improvements
  
  Use these categories. If none, say "None".

3. Wait for review results.

If Critical issues found:

Critical issues found. Must fix before proceeding:

[Show issues]

Fix these issues, then re-request review.

If Important issues found:

Important issues found. Fix before merge/PR:

[Show issues]

Fix these issues, then proceed to Step 4.

If only Minor issues (or no issues):

Code review passed. Ready to proceed.

[Show minor issues - these can be addressed later]

MANDATORY: Do not proceed to Step 4 (Present Options) until code review passes.

### Update Step 4 (formerly Step 3): Present Options

```markdown
### Step 4: Present Options (After Review Passes)

Present options ONLY after code review passes (no Critical/Important issues):

Code review passed. Implementation complete. What would you like to do?

1. Merge back to locally
2. Push and create a Pull Request against
3. Keep the branch as-is (I'll handle it later)
4. Discard this work

Which option?

Add Enforcement

Add to Red Flags section:

**Never:**
- Proceed with failing tests
- Skip code review before merge/PR
- Merge without verifying tests on result
- Create PR without code review passing
- Delete work without confirmation
- Force-push without explicit request

Files to Modify

• skills/finishing-a-development-branch/SKILL.md - Add required code review step

Success Criteria

• Code review is MANDATORY before presenting merge/PR options
• Cannot proceed to merge/PR without passing review (no Critical/Important issues)
• Clear error messages when review fails
• Reviewer subagent provides structured feedback
• Minor issues don't block but are noted

Priority

HIGH - This defect allowed creating a PR without review, violating core quality gate.