---
name: receiving-code-review
description: Use when receiving code review feedback for sf. Verify before implementing, push back with evidence when wrong, no performative agreement. Actions over words.
---

# Receiving Code Review

## Core Principle

Verify before implementing. Technical correctness over social comfort. Actions over words.

A reviewer's job is to find real issues. Your job is to verify each point and either fix it or push back with evidence — not to agree quickly so the conversation ends.

## Skill Chain

Side-chain skill — activates when feedback arrives on already-delivered work.

```
← prev:  (invoked when review feedback arrives)
→ next:  return to spec-first-tdd / wave-implementation if the fix needs new work,
         or finish-and-verify if it's a small in-scope fix
```

If the fix is trivial (lint, style, naming): apply it inline and return to `finish-and-verify` without stopping.

If the fix requires new contract work: invoke `spec-first-tdd` (write a failing regression test first; the reviewer found a real behaviour gap → that's a missing test).

Do not pause for approval mid-fix unless the review widens scope or changes the approved plan materially.

## The Response Pattern

```
1. READ:        Complete feedback without reacting.
2. UNDERSTAND:  Restate the requirement — or ask for clarification.
3. VERIFY:      Check against the codebase. Tests, callers, history.
4. EVALUATE:    Is this technically sound for sf's contract?
5. RESPOND:     Technical acknowledgment, or reasoned pushback.
6. IMPLEMENT:   One item at a time. Test each. Verify each.
```

## Forbidden Responses

Never:

- "You're absolutely right!"
- "Great point!"
- "Let me implement that now" (before verification).
- Any thanks/gratitude expression.

Instead:

- Restate the requirement.
- Ask for clarification.
- Push back with evidence.
- Or just fix it and let the diff speak.

## Verify Before Implementing

Before changing any code based on review feedback:

```bash
# Does the suggested pattern exist elsewhere?
rg -n "<suggested pattern>" src/ packages/

# Who depends on the symbol the reviewer wants changed?
rg -nF "<symbol>" src/ packages/

# Why is this written this way? (look at history before disagreeing)
git log --follow -p -- <file>
git blame -L <range> <file>

# Do existing tests cover the contract the reviewer wants?
rg -n "<test-name-pattern>" src/ packages/
```

Search memory for prior context the reviewer might be unaware of:

```
sf_search_memories(query="<topic of feedback>", limit=5)
```

If you can't verify, say so: *"I can't verify this without checking <X>. Investigating first."*

Label triage claims:

- `Observed:` directly from code, tests, logs, traces, or tool output.
- `Inferred:` conclusion supported by observed evidence.
- `Proposed:` rebuttal or implementation not yet validated.

For non-trivial runtime/provider review items, verification must include:

- An explicit repro/debug pass for the failing boundary.
- The same repro/debug pass after the fix.
- Not just trace reading or code inspection.

## sf-Specific Checks Before Implementing a Suggestion

Before changing code based on a suggestion, verify it doesn't violate sf's invariants:

- **Iron Law**: Does the suggestion ask you to skip the failing test → fix → green cycle? Push back. The bug is a missing test; write it first.
- **Purpose Gate**: Does the suggestion add an exported symbol without a complete PDD packet (Purpose, Consumer, Contract, Failure boundary, Evidence, Non-goals, Invariants, Assumptions — see [`purpose-driven-development`](../purpose-driven-development/SKILL.md))? Push back. A symbol that doesn't earn all 8 fields is unspecified, and "I'll fill them in later" is a scope-creep vector. Evidence must be machine-executable or `[MANUAL: reviewer + scenario]` — prose-only is rejected.
- **YAGNI**: Does the suggestion add abstraction with zero current callers? Run `rg` for callers — if zero, push back: *"Nothing calls this in production. Adds dead code. Remove per YAGNI?"*
- **Recent decisions**: If the suggestion contradicts a `.sf/DECISIONS.md` entry or a recent ADR (`docs/dev/ADR-*.md`), check the history before implementing or pushing back.
- **Self-modification boundary**: If the suggestion edits a protected file (`SPEC.md`, `BUILD_PLAN.md`, `AGENTS.md`, etc.) without explicit human approval, push back — those need human sign-off.

## When to Push Back

Push back when:

- The suggestion violates the Iron Law or Purpose Gate.
- The suggestion introduces dead code (YAGNI).
- The suggestion breaks existing behaviour (`npm test` fails after applying).
- The reviewer lacks full context (callers count, blast radius, prior decision).
- The suggestion conflicts with a deliberate architectural decision in `DECISIONS.md` or an ADR.

How: with evidence, not opinion.

> "I checked — `claim()` has 47 callers across `auto-dispatch.ts` and `worktree-manager.ts`. Renaming would break all of them; I'd suggest deprecating with a re-export instead."

> "ADR-009 chose the orchestration kernel split deliberately to keep `auto-loop` and `auto-dispatch` separable. Merging them as suggested would re-couple the boundary."

## Review Debate and Synthesis

For architectural, boundary, or behaviour-preservation review comments, run an adversarial pass via [`advisory-partner`](../advisory-partner/SKILL.md): advocate strengthens the reviewer's position; challenger strengthens the original design's position. Record a **Review Synthesis** when the review changes a contract or boundary.

If the unit's `proposal.md` exists, validate review comments against the declared requirement deltas before implementing them.

## Implementing Feedback

Review fixes follow the same TDD contract as any other change. **A reviewer finding a bug = a missing test.** For non-trivial review fixes, write the failing test first via `spec-first-tdd` before changing code. The test proves the reviewer's concern was real and prevents regression.

```
For multi-item feedback:
  1. Clarify anything unclear FIRST — ask before touching code.
  2. Order: blocking issues → simple fixes → complex changes.
  3. For non-trivial fixes: failing test FIRST, then fix.
  4. Run tests after each item — not all at the end.
  5. typecheck + lint on changed files before claiming done.
```

Don't batch everything and run the gate once at the end — test each change independently. A single gate at the end can hide which fix introduced a regression.

Keep scope tight: fix the validated review issue, not every adjacent redesign the comment makes tempting.

After resolving significant review comments:

```
log_reasoning(
  category="review-learning",
  content="review caught: <what> in <component> — prevent by <design principle or test gap>",
  confidence=0.9
)
```

Skip for trivial lint/style fixes — only record when the reviewer found a real behaviour gap.

## Acknowledging Correct Feedback

✅ "Fixed — <one sentence what changed>."
✅ Just fix it; the diff speaks.

❌ "You're absolutely right!"
❌ "Thanks for catching that!"
❌ Any gratitude expression.

## If You Pushed Back and Were Wrong

✅ "Verified — you're correct. <What changed>. Fixed."

❌ Long apology.
❌ Defending the pushback.

Factual. Move on.
