Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
347 changes: 347 additions & 0 deletions packages/rules/.ai-rules/skills/receiving-code-review/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,347 @@
---
name: receiving-code-review
description: >-
Use when receiving PR review feedback or code review comments.
Enforces verification-first response to review feedback with
anti-sycophancy classification and constructive disagreement protocol.
allowed-tools: Read, Grep, Glob, Bash(gh:*, git:*)
argument-hint: [pr-url-or-number]
---

# Receiving Code Review

## Overview

Blind agreement with review feedback is as harmful as ignoring it. Implementing unverified suggestions introduces bugs, degrades architecture, and teaches reviewers that superficial comments get results.

**Core principle:** Treat every review comment as a hypothesis to be tested, not an instruction to be followed. Evidence determines your response, not authority or politeness.

**Violating the letter of this process is violating the spirit of receiving code review.**

## The Iron Law

```
VERIFY BEFORE IMPLEMENTING — AGREEMENT WITHOUT EVIDENCE IS SYCOPHANCY
```

- Reviewer authority does not replace verification. Senior engineers make mistakes too.
- "Sounds good, fixing now" without reading the referenced code is failure.
- Implementing a wrong fix is worse than the original code — it adds confusion AND a bug.

## When to Use

**Always use for:**
- PR review comments requesting changes
- Code review feedback from teammates or AI agents
- EVAL mode feedback on your implementation
- Post-merge review comments requiring follow-up

**Lighter process acceptable for:**
- Typo fixes and formatting-only feedback (verify the typo exists, then fix)
- Documentation wording suggestions (read the suggestion, apply if clearer)

**Do NOT use for:**
- Automated linter/CI feedback (mechanical — just fix or configure)
- Pair programming feedback (synchronous — different workflow)
- Architectural review requiring re-planning (use `brainstorming` or `writing-plans` skills)
- Giving reviews (use `pr-review` skill instead)

## Quick Reference

### Feedback Classification

| Classification | Criteria | Verification Required | Action |
|---------------|----------|----------------------|--------|
| **Must-Fix** | Bug, crash, security vulnerability, data loss | YES — reproduce the issue | Fix immediately after verification |
| **Evaluate** | Alternative approach, refactoring, performance | YES — compare approaches | Implement if better, discuss if not |
| **Preference** | Style, naming, formatting, subjective taste | NO — check project conventions | Accept if convention-aligned, discuss if not |

### Decision Matrix

| Verification Result | Classification | Action |
|--------------------|---------------|--------|
| Issue reproduced | Must-Fix | Fix and cite the reproduction |
| Issue NOT reproduced | Must-Fix | Reply with evidence, ask for scenario |
| Alternative is better | Evaluate | Adopt with acknowledgment |
| Alternative is equivalent or worse | Evaluate | Decline with evidence and tradeoff analysis |
| Matches project convention | Preference | Accept silently |
| Contradicts project convention | Preference | Cite convention, keep current approach |

---

## Phase 1: Triage & Classify

**Goal:** Categorize every review comment before acting on any of them.

**Process:**
1. Read ALL comments before responding to any
2. Classify each comment as Must-Fix, Evaluate, or Preference
3. Order by priority: Must-Fix first, then Evaluate, then Preference

**Classification decision tree:**

```
Is the reviewer claiming a bug, crash, security issue, or data loss?
├── YES → Must-Fix
└── NO → Is the reviewer suggesting an alternative approach or refactoring?
├── YES → Evaluate
└── NO → Preference
```

**Watch for misclassification:**

| Looks Like | Actually Is | How to Tell |
|-----------|-------------|-------------|
| Must-Fix ("this will crash") | Evaluate (edge case that can't happen in context) | Check call sites and input constraints |
| Preference ("rename this") | Must-Fix (name collision causes shadowing bug) | Check scope for conflicts |
| Evaluate ("use pattern X") | Preference (both patterns are equivalent here) | Compare actual tradeoffs in this context |

**Done when:** Every comment has a classification tag. If unsure, classify UP (Preference → Evaluate, Evaluate → Must-Fix).

---

## Phase 2: Verify

**Goal:** Build evidence for each Must-Fix and Evaluate item before acting.

### Verification Matrix

| Feedback Type | Verification Method | Evidence |
|--------------|-------------------|----------|
| "This will crash/fail on X" | Write a test with input X, run it | Test result (pass/fail) |
| "N+1 query / performance issue" | Add logging, hit the endpoint, measure | Query count, timing data |
| "This violates SRP / SOLID" | Evaluate against project conventions and codebase patterns | Convention references, pattern grep results |
| "Use pattern X instead" | Compare both approaches against existing codebase patterns | `grep` for pattern usage, tradeoff list |
| "Missing error handling" | Trace call path, check if error can actually reach this code | Call chain analysis |
| "Race condition possible" | Analyze concurrent access paths | Sequence diagram or test |
| "Style: rename this variable" | **Skip verification** — this is Preference | Check project conventions only |

### Verification Checklist

For each Must-Fix / Evaluate item:

- [ ] Read the exact code the reviewer references (not just the comment)
- [ ] Check if the claimed issue can actually occur given the call context
- [ ] If a bug claim: write a minimal test to reproduce
- [ ] If an alternative approach: grep codebase for both patterns
- [ ] If a convention claim: check `.ai-rules/` or project style guide
- [ ] Record the evidence (test output, grep results, or analysis)

**Done when:** Every Must-Fix and Evaluate item has a verification result: `reproduced`, `not reproduced`, or `tradeoff analyzed`.

**Guard against over-verification:** Preference items do NOT need verification. If you catch yourself writing a test for a variable rename, stop — you are stalling.

---

## Phase 3: Respond

**Goal:** Act on each item based on classification AND verification evidence.

### Processing Order

1. **Must-Fix items** (process first — they may invalidate other comments)
2. **Evaluate items** (some may become moot after Must-Fix changes)
3. **Preference items** (batch these for efficiency)

### Response Templates

**Must-Fix — Verified:**
> Confirmed. Reproduced with [test/scenario]. Fixed in [commit]. Added regression test.

**Must-Fix — Cannot Reproduce:**
> Attempted to reproduce with [specific steps]. Test passes with [input]. Could you share the exact scenario you're seeing? Here's what I tested: [evidence].

**Evaluate — Adopted:**
> Good point. Adopted [approach] because [evidence it's better in this context]. [Brief tradeoff acknowledged].

**Evaluate — Declined:**
> Considered this. Staying with current approach because [concrete evidence]. The tradeoff: [their approach advantage] vs [current approach advantage]. In this context, [reason current wins].

**Evaluate — Partial Agreement:**
> You're right about [the problem]. However, [proposed solution] would [issue]. Instead, I [alternative fix] which addresses the concern while [preserving X].

**Preference — Accepted:**
> Updated.

**Preference — Convention Conflict:**
> Project convention uses [X pattern] per [rule reference]. Keeping for consistency. Happy to discuss changing the convention project-wide if you'd like.

### Tone Calibration

| Reviewer | Tone | Example |
|---------|------|---------|
| Peer | Direct and evidence-based | "Tested this — passes with null input. What scenario triggers the crash?" |
| Senior / Tech Lead | Respectful but still evidence-based | "Great catch on [valid point]. For [other point], I verified with [test] and it passes — could you clarify the scenario?" |
| External Contributor | Welcoming but factual | "Thanks for the review! I checked [claim] and found [evidence]. Here's my reasoning: [explanation]." |

**Key principle:** Deference is not compliance. You can respect someone's expertise while still requiring evidence for their claims.

**Done when:** Every comment has a response: implemented (with commit reference), declined (with evidence), or discussed (with question).

---

## Phase 4: Re-request Review

**Goal:** Close the feedback loop cleanly.

**Checklist:**
- [ ] All Must-Fix items resolved (implemented or discussed with evidence)
- [ ] All Evaluate items resolved (adopted, declined with reasoning, or discussed)
- [ ] All Preference items addressed (accepted or convention-cited)
- [ ] Tests pass after all changes
- [ ] No unrelated changes mixed into fix commits
- [ ] Summary comment posted listing what was changed and what was discussed

**Summary comment template:**
```
Changes made:
- [Must-Fix] Fixed [issue] — added regression test (commit abc123)
- [Evaluate] Adopted [suggestion] for [reason] (commit def456)
- [Preference] Updated [items] (commit ghi789)

Discussed (no change):
- [item]: [brief reason with evidence link]

Ready for re-review.
```

---

## Disagreement Protocol

When you believe the reviewer is wrong, follow this escalation:

### Step 1: Lead with Evidence

```
Bad: "I disagree with this suggestion."
Good: "I tested with [input] and it passes. The null case can't reach
this code path because [caller] validates at line 42. Here's the test: [link]."
```

### Step 2: Acknowledge the Tradeoff

```
Bad: "My approach is better."
Good: "Your approach has [advantage]. Mine has [advantage]. In this context,
[mine/yours] fits better because [specific reason]."
```

### Step 3: Offer Alternatives

```
Bad: "No, I won't change this."
Good: "Instead of [their suggestion], what about [alternative] which
addresses your concern about [X] while preserving [Y]?"
```

### Step 4: Escalate Appropriately

If you cannot reach agreement after evidence exchange:
- Tag a third reviewer for a tiebreaker
- Reference project conventions or architectural decisions
- Accept and document: "Implementing as requested. Noting that [tradeoff] for future reference."

### When to Accept Despite Disagreement

- Reviewer has project-specific context you lack (institutional knowledge)
- The difference is genuinely marginal and not worth the discussion cost
- Project convention sides with the reviewer (convention trumps preference)
- You've been going back and forth for 3+ rounds (diminishing returns)

---

## Anti-Sycophancy Guide

### Prohibited Responses

| Don't Say | Why It's Harmful |
|-----------|-----------------|
| "Great catch! Fixing now." (without verifying) | You don't know if it's actually a catch |
| "You're absolutely right, I should have caught that." | You're performing contrition, not engineering |
| "Sounds good, will update." (without reading code) | Agreeing to changes you haven't evaluated |
| "Thanks for the thorough review!" (as the only response) | Politeness is not a substitute for engagement |
| "I'll fix all of these." (batch-agreeing) | Each comment needs individual evaluation |

### Anti-Patterns

| Anti-Pattern | What Happens | Instead |
|-------------|-------------|---------|
| **Apologetic capitulation** | "You're right, sorry" → implement unverified change | Verify first, then respond with evidence |
| **Scope creep acceptance** | "While you're at it, also refactor X" → silent addition | Acknowledge, create separate issue/PR |
| **Authority-driven compliance** | Senior said it → must be right → implement blindly | Seniors deserve evidence-based responses too |
| **Fix-then-justify** | Implement first, rationalize after | Evaluate BEFORE implementing |
| **Cherry-picking easy wins** | Fix trivial items, ignore hard critical feedback | Process Must-Fix items FIRST |
| **Silent disagreement** | Disagree privately, implement anyway | State your concern on the review thread |
| **Over-verification stalling** | "I need to verify" as excuse to avoid valid feedback | Preference items don't need verification |
| **Conflating preference with defect** | Treating style feedback as a bug fix | Classify correctly, respond accordingly |
| **Performative agreement** | Agreeing enthusiastically to avoid conflict | Evidence determines response, not comfort |

---

## Red Flags — STOP

These thoughts mean you are about to violate the Iron Law:

| Thought | Reality |
|---------|---------|
| "I'll just fix it, it's faster" | Faster than what? You haven't verified it's broken. |
| "The reviewer is senior, they must be right" | Seniority is not evidence. Verify. |
| "It's just a small change, no need to verify" | Small unverified changes compound into big problems. |
| "I don't want to seem argumentative" | Evidence-based responses are not arguments. |
| "They clearly know more about this" | They might. Verify and find out. |
| "I'll verify later after implementing" | That's fix-then-justify. Evaluate BEFORE implementing. |
| "All their other comments were valid, so this one must be too" | Each comment stands on its own evidence. |
| "Pushing back will delay the PR" | Implementing wrong changes delays it more. |

## Common Rationalizations

| Rationalization | Counter |
|----------------|---------|
| "Verifying takes too long" | Implementing a wrong fix takes longer. Reverting takes even longer. |
| "I trust this reviewer" | Trust is not a verification method. Even trusted reviewers make mistakes. |
| "It's not worth the discussion" | If it's not worth discussing, it's not worth implementing either. |
| "The reviewer will be annoyed if I push back" | Professional reviewers respect evidence-based responses. |
| "I already know they're right" | Then verification will be quick. Do it anyway. |
| "This is just bikeshedding" | If so, classify as Preference and handle accordingly. |

---

## Handling Edge Cases

### Conflicting Reviews

When multiple reviewers give contradictory feedback:
1. Surface the conflict explicitly in a comment tagging both reviewers
2. Present both positions with your analysis
3. Do NOT silently pick one — let the reviewers resolve the conflict
4. If unresolved, defer to project convention or escalate to tech lead

### Vague Feedback

When feedback lacks specifics ("this feels wrong", "not clean enough"):
1. Do NOT guess what the reviewer means
2. Ask a specific clarifying question: "Could you point to the specific part that concerns you? Is it the [data flow / naming / structure]?"
3. Wait for clarification before acting

### Feedback on Framework-Mandated Code

When the reviewer suggests changing code required by the framework:
1. Explain the constraint: "This pattern is required by [framework] — see [docs link]"
2. Offer alternatives within the framework's constraints if they exist

### Review of Test Code

Apply higher scrutiny to suggestions that weaken tests:
- Removing edge case tests → challenge firmly with coverage data
- Simplifying assertions → verify the simplified version still catches regressions
- "This test is unnecessary" → check if it covers a unique code path

---

## Related Skills

- **`pr-review`** — For conducting reviews (the giving side of this skill)
- **`systematic-debugging`** — For investigating issues found during verification
- **`test-driven-development`** — For writing verification tests in Phase 2
- **`writing-plans`** — For re-planning when architectural review feedback requires redesign
Loading