---
name: requesting-code-review
description: Use when submitting work for review (PR creation or peer review). Prepares a structured summary with evidence — test results, typecheck, blast radius, consumer verification — so the reviewer has everything needed to decide.
---

# Requesting Code Review

## Purpose

Make the reviewer's job easy. Provide evidence, not claims. Test output, not "I think it's clean."

A review request without evidence is a waste of the reviewer's time. Either you didn't run the gates, or you did but didn't trust them enough to share — both signal the change isn't ready.

## When to Run

- A slice is GREEN and `finish-and-verify` has passed.
- Opening a PR for human or peer review.
- Inside the auto-loop when a unit reaches `PhaseReview`.

## Skill Chain

```
← prev:  finish-and-verify  (verification is already green)
→ next:  receiving-code-review  (when reviewer feedback arrives)
```

This skill is the optional terminal step of the delivery chain — invoked after verification confirms the change is honest.

## Recognise Your Rationalisations

You are Claude, and you are bad at requesting review. The shortcuts below are the ones you actually take — name each one as you catch yourself, then do the opposite:

- *"Tests passed earlier, I'll just say so."* → No. Re-run after the **last** edit and paste the `Command run / Output observed / Result` block from `finish-and-verify`'s trace format. "Tests pass" without a fresh trace is a self-report, not evidence.
- *"Risk is low."* → Did you actually `rg` the changed symbols and count callers? "Low" without that check is a guess. If blast radius >10, mark Risk medium minimum.
- *"I'll fill in Failure boundary / Assumptions later."* → No. The PDD packet is the spec; an incomplete packet is no spec. If you can't write Assumptions now, you don't yet understand the world this code runs in — go back to PDD.
- *"The Falsifier is obvious, why write it down?"* → If it's obvious, one sentence costs nothing. If you can't write it, the contract isn't crisp — and the reviewer will spend their time discovering what you should already know.
- *"I verified that part manually."* → Either tag it `[MANUAL: <reviewer> + <scenario>]` so the reviewer knows what was checked, or replace it with a runnable command. Prose-only Evidence is rejected — same rule as PDD v2.
- *"The reviewer will catch anything I missed."* → Reviewer time is expensive. You already know the soft spots. Name them in `Falsifier` or in the closing *"strongest reason this could still be wrong"* line so the reviewer focuses on what you couldn't decide alone.

These are the same shortcuts `spec-first-tdd`'s **Red Flags** warn about and the same ones `finish-and-verify`'s trace format prevents. By the time you reach this skill, you should have caught yourself already — this section exists because sometimes you don't.

**When you can't tell:** if after walking through the bullets you're still genuinely unsure whether a PDD field, an Evidence claim, or a Risk rating is honest, dispatch an [`advisory-partner`](../advisory-partner/SKILL.md) subagent before pulling in a human reviewer. It runs on the `validation` model — a different model from the planning/execution model that wrote this summary, so it is not subject to *your* rationalisations. A fast second opinion costs less than a wrong review request, and any disagreement the advisory surfaces is exactly what should land in the *"strongest reason this could still be wrong"* line of the summary. If the advisory verdict is `RECONSIDER` or `PROCEED WITH CAVEAT`, revise before submitting; if it reads `PROCEED` but only after waving away an objection, the contract itself is probably underspecified — go back to PDD.

## Before Requesting Review

All of these must pass. If any fails, **fix first; don't request review.**

```bash
npm run typecheck:extensions                    # zero errors
npm run test:unit -- <pattern>                  # all relevant tests pass
npm run test:integration                        # if integration boundaries touched
rg -nF "<changed symbol>" src/ packages/        # production caller exists
```

For non-trivial runtime/provider fixes, also capture:

- Explicit repro/debug output **before** the fix.
- Same repro/debug output **after** the fix.
- Proof that the solved boundary matches the original failure (not just "tests pass now").

Search prior memory for related context the reviewer might need:

```
sf_search_memories(query="<area of change>", limit=5)
```

## Review Summary Template

```markdown
## What changed
<1–3 sentences: what behaviour changed and why>

## Files changed
- `src/path/to/file.ts` — <one-line description>
- `src/path/to/file.test.ts` — <one-line description>

## Linked work
- Slice: <unit-id> / Milestone: <unit-id>
- Requirement: <Active R-id from REQUIREMENTS.md, if applicable>
- Decision: <DECISIONS.md entry, if applicable>

## PDD packet (all 8 fields)

Restate verbatim from `.sf/active/{unit-id}/pdd.md` — do not paraphrase. Same 8 fields filled in by [`purpose-driven-development`](../purpose-driven-development/SKILL.md). If any field is missing or empty, **stop and return to PDD** — do not request review with an incomplete packet.

- **Purpose**: <why this exists>
- **Consumer**: <production code path that depends on this — file:symbol>
- **Contract**: <observable behaviour that proves success — what the consumer receives, not what the implementation does internally>
- **Failure boundary**: <what *correct failure* looks like if the purpose can't be fulfilled — degrade, surface, do not swallow>
- **Evidence**: see Evidence section below; every criterion must be machine-executable or `[MANUAL: reviewer + scenario]` — prose-only is rejected
- **Non-goals**: <what this change is *not* solving>
- **Invariants**: <what must remain true. If the change touches async / queues / timers / state machines: split into Safety ("X never happens") + Liveness ("Y eventually happens"). Pure synchronous code: safety-only is fine.>
- **Assumptions**: <conditions about the world that MUST be true for this spec to be valid — locking protocols, API stability, caller invariants, deployment context, data shape>

## Value at risk
<what breaks if this returns the wrong answer — used by reviewer to set scrutiny level>

## Evidence
- Typecheck: ✅ `npm run typecheck:extensions`
- Tests: ✅ <X passed, 0 failed, Y skipped>
- Lint: ✅ <touched files clean>
- Repro before fix (non-trivial only): <command + output snippet>
- Repro after fix (non-trivial only): <command + output snippet>

## Blast radius
<callers count, modules touched, transitive surface>

## Risk
<low | medium | high — based on blast radius and change complexity>

## Falsifier
<the observable condition that would prove this contract wrong, and whether it's been checked — distinct from Failure boundary: Failure boundary defines *correct* failure, Falsifier names the observation that proves the *contract itself* is wrong>

## Scope defence
<one sentence — what tempting adjacent work this slice refused. Distinct from Non-goals: Non-goals are static exclusions stated up front; Scope defence is the discipline statement about what was *almost* done but wasn't.>
```

For architecture-heavy or boundary-heavy changes, append:

```markdown
## Review Synthesis
- Advocate strengthened: <strongest case for the design>
- Challenger argued: <strongest case against>
- Falsifier outcome: <was it checked? how?>
- Why the boundary still stands:
```

## Tools for Building the Summary

```bash
git diff --stat HEAD~1                          # what actually changed
rg -nF "<changed symbol>" src/ packages/        # callers
git log -p -- <changed-file>                    # recent history for context
git blame -L <range> <file>                     # who wrote the touched lines
```

## Local Review Loop (optional, for non-trivial changes)

Before requesting external review on a high-risk change, run an internal adversarial pass via [`advisory-partner`](../advisory-partner/SKILL.md):

1. Assemble evidence (typecheck, tests, blast radius, consumer).
2. Write the review summary above.
3. Spawn the default review bundle, passing `parentTrace` so each reviewer can
   audit what you actually did this turn — not just the summary you wrote:

   ```
   subagent({
     parentTrace: "<bullet summary: files edited, tests run + pass/fail, " +
                  "commands and exit codes, any tool errors observed>",
     tasks: [
       { agent: "reviewer", task: "Advocate: strengthen the design. <prompt>" },
       { agent: "reviewer", task: "Challenger-A: attack the strongest assumption. <prompt>" },
       { agent: "security", task: "Challenger-B: attack from a security lens. <prompt>" }
     ]
   })
   ```

   The advocate is most likely to wave away an objection that the trace
   actually contradicts — passing the trace forces the advocate to engage
   with what you did, not what you claimed to do.
4. Collect each reviewer's structured output, including confidence.
5. Synthesise: fill the gap each reviewer exposed; answer or admit each objection in the summary.
6. Escalate only if disagreement remains material after Step 5.

Hard rules for the local loop:

- Do not let all reviewers share the same lens.
- The advocate must have repo access (frame as: "investigate whether X is safe; argue your side").
- If the advocate contradicts your framing, your framing was wrong — re-verify before synthesis.
- If all reviewers report low confidence, gather better evidence rather than adding more opinions.
- **Pass `parentTrace`** to all three subagents so they audit your actual work, not just your summary. The parent trace is your context's record of what you did this turn — assemble it once, pass it to all reviewers via the batch-level field.

Stop when:

- The reviewer can see purpose, evidence, and risk without guessing.
- The strongest objection is either answered or admitted as residual risk.
- Another review pass would not change the decision materially.

## After Submitting

Persist the delivery context so future units can trace what was built and why:

```
log_reasoning(
  category="delivery",
  content="delivered: <what changed> — slice: <id> — consumer: <caller> — insight: <what was learned>",
  confidence=0.9
)
```

## Rules

- Never request review with failing typecheck or tests.
- Include evidence in the summary — not "I think it's clean."
- List every changed file — don't hide changes.
- If blast radius is high (>10 transitive callers), flag it explicitly.
- For non-trivial runtime/provider changes, include the debug/repro evidence — not just trace summaries.
- For architecture-heavy changes, include disagreement evidence — what advocate strengthened, what challenger attacked.
- All 8 PDD packet fields must be present and restated from `.sf/active/{unit-id}/pdd.md`. If any of the 8 is missing, or Evidence contains prose-only criteria, **stop** and return to `purpose-driven-development` — do not request review with an incomplete packet.
- For changes touching async, queues, timers, or state machines, the Invariants field MUST split safety ("X never happens") and liveness ("Y eventually happens"). A single combined invariants list is a v1 artefact and will be rejected.
- Mark major claims as `Observed`, `Inferred`, or `Proposed`.
- Include the strongest reason the change could still be wrong even if tests pass.
