Cr

---
name: cr
description: "Full code review: triangulate every change statically, model state/timing slices dynamically."
---

# cr — Full Code Review

Do a PR's complete review in one pass: **broad static sweep + deep dynamic dig**. This skill is **self-contained** in both methods.

Most PR review is static — read the snapshot and you can judge right/wrong, **easy but must not be skipped**. The hard part, the part you can't catch line-by-line, is **deriving dynamic behaviour from static code**: timing / state evolution / concurrency. cr does both tracks.

## Execution mode (choose before you start)

**Iron rule: the actual review always runs in a 【clean subagent】; the main session only dispatches + mechanically merges, NEVER reviews in person.** Having just written the code in the main session, carrying the defensive "I know why I did it this way" context, is the worst possible reviewer. When a subagent is spawned via the Agent tool it **does not inherit the main session's history** — it sees only the diff + this skill → that is exactly the source of fresh eyes. The dev session's residual history **must not pollute** triage / modelling / downgrade judgements.

Two side-effect red lines: ① the main session **does not make substantive judgements on the subagent's behalf**; ② splitting static out also removes the "static breadth dilutes dynamic modelling" dilution of the main thread.

Three tiers by scale (**all enter at least one clean subagent**). **You pick between the first two by scale yourself; hard is enabled ONLY when the user explicitly asks for it — never escalate to it on your own** — however large the diff or however high the risk, without the user saying hard you cap at the default tier, and don't suggest switching to hard in the report either:
- **Single subagent**: no dynamic surface / tiny → 1 clean subagent runs Part A. Main session just relays the diff + collects the report.
- **Default (2 subagents)**: has a dynamic surface → 1 static + 1 dynamic, **both clean, in parallel**, each triaging on the clean diff itself (static scans all changes / dynamic lists every slice via the slice-archetype checklist).
- **hard (1 + N subagents)**: **only when the user explicitly asks for hard (e.g. `/cr hard`)** → first 1 clean triage subagent produces the slice list, then fan out 1 static + 1 subagent per slice. Deepest, slowest. Once the user asks for it, run it as asked — don't downgrade on your own because "this change is small"; if no dynamic slice can be carved out, just note "no dynamic slice, per-slice fan-out degenerates to 1 static".

Subagent prompt template: `Read this skill (cr/SKILL.md); apply only the matching Part to <your chunk (all of Part A / one dynamic slice)>, triage it yourself, and output findings in the unified format (including the origin tag)`.

**Synthesis (main session, organize only — never re-judge)**: gather each subagent's findings → dedup (mark same-root-cause as "accomplices") → sort by impact × probability → one report + gradient chart. **It is forbidden to use the main session's dev context to whitewash / downgrade any finding** — take what the subagent judged as-is. To dispute one, **spawn a fresh clean subagent to re-check**, not the main session deciding on its own. The main session may organize the result into a short summary table, but the **expanded details must preserve the subagent's key evidence density**: for dynamic findings, carry over the state diagram / timeline / change trajectory / counterexample path; for static findings, carry over the triangulation evidence. Deduplication and wording cleanup are allowed; compressing a detailed subagent model into a one-line summary is not. Origin tags are taken as-is too — reclassifying an `introduced` finding as `pre-existing` is a form of whitewashing, and the main session is the most likely to do it (it knows "I didn't touch that part").

> Trade-off: the default 2-subagent both isolates dev pollution and removes static-dilutes-dynamic; hard's per-slice fan-out is extra depth (in practice it dug image/video up to 🔴, and multiple independently-converging subagents raised confidence), expensive and slow, selected explicitly by the user (never auto-escalated).

## Step 1 — Triage (do this first in both modes)
Read the diff, cut two surfaces (may overlap):
- **Static surface** = all changes → go to **Part A**.
- **Dynamic surface** = slices touching **state / timing / concurrency / cross-process timing** → additionally go to **Part B**.

Splitting principle: **A judges as-written** (is it written correctly), **B judges over-time** (can timing break it through); check both surfaces on the same spot. No dynamic surface → skip Part B.

---

## Part A — Static triangulation: cross-locate the change against three references

correctness, that "unknown", is triangulated against three independent references — **intent / input domain / surroundings**. Any one alone misses something (intent-only misses boundaries, input-only misses intent, surroundings-only misses both); only the three together nail it down.

Pick only the **semantic-layer problems tools can't judge** (style / format / unused vars / type mismatch go to linter & type-checker). Static bugs are almost always **the snapshot failing to match some reference**:

### A1. vs intent (did it do what it claims)
- Do the name / signature / type / PR description / comment **promises** match what the code **actually does**? (`isValid` returns true on error; PR says it changes X but actually touched Y)
- Do error / boundary branches return the **right** thing, or did they carelessly return the happy-path value?
- Are comments / docs in sync with the new code, or still describing the old behaviour?

### A2. vs input domain (did it cover the full input set) — the #1 bug source
- Is the whole set handled: null / undefined / empty / single element / duplicate / boundary value / overflow / negative / unicode / over-length?
- Is the **error path** as correct as the happy path? On early returns / exception paths, are resources closed and state consistent?
- Is external input (user text, params, deserialization, webhook) treated as **untrusted**? (injection, privilege escalation, unvalidated, secrets into logs)
- Classic disease: happy path correct, input domain not fully covered.

### A3. vs surroundings (consistent with existing contracts / conventions)
- **Contract drift**: changed a signature / type / enum / return shape — did **all consumers** change with it? (grep-visible, lint won't flag)
- **Convention deviation**: should have used the project's established pattern / helper but used a raw primitive / reinvented it?
- **Layering / boundary**: bypassed a layer it should go through, introduced a forbidden dependency direction?
- **Type as source of truth**: or bypassed by `any` / assertions / casts?
- **Security alignment**: a check the siblings all have (authn, tenant isolation) — missing here?

### Wrap-up (light; don't redo what tools cover)
- Duplicated logic — especially duplicated **decisions / policies** — should converge to a single source? Dead code, over-complexity?
- **Local** performance: N+1, IO in a loop, an obviously degraded algorithm, unbounded growth? (state-evolving cases go to Part B)
- Tests: do the new logic's **boundary / error paths** have assertions? Does the red test fail before the fix and pass after?

---

## Part B — Dynamic: derive the static code into a model, then review (dynamic-surface slices only)

You can't see timing bugs in a snapshot — they live in "how things change over time once running". Derive it into a dynamic model and review that, instead of scanning line-by-line.

**Model per slice (hard rule)**: before modelling, list **all** independent dynamic slices and tick each through B1–B3 — **don't model just the most obvious one and call it done; missing one slice = all of that slice's dynamic risks are missed**. To avoid omissions, sweep these **slice archetypes**:
- **Shared-state init + multi-write** (accumulator / key / cache: who creates first, who writes many)
- **State reused across processes / re-entry** (retry / recovery / multi-worker / queue)
- **check-then-act across an async gap** (precheck → slow operation → side-effect lands late; concurrent starts collectively cross the precheck — TOCTOU)
- **fire-and-forget write + later read** (read overwrites before the write lands)
- **Implicit context across a hop** (ALS / log context: still there after crossing process / queue?)

⚠ **One piece of code can be both a static finding point and a dynamic slice** — don't stop modelling it as a dynamic slice just because Part A already scanned it statically (wording, duplication, optional params). Static "claiming" ≠ dynamic coverage.

### B1. Static read-in (input)
Read out: **what states exist** · **who reads/who writes** (across features, across upstream callers — don't stop at this file) · **contract / type** · **control flow and entry points**.

### B2. Build the dynamic model (the main artifact, written into the report as evidence)
- **State diagram** — what states + transitions exist; annotate each transition with **who changes it under what condition**.
- **Timeline** — all writers/readers on a time axis, marking who's first; **pull it through across components, across processes**, tracing back to the moment the state is **truly born** (not where it's re-referenced locally).
- **Change trajectory** — how the key state value flows along the timeline: created / changed / read by whom, ever overwritten / no-op'd / lost midway.

These two diagrams are the evidence — many bugs (a 6-second gap, a key created too early) only reveal themselves once drawn.

### B3. Evaluate 6 classes of dynamic risk on the model

| Risk | What to look for on the model |
|---|---|
| **Order race** | Is there a writer running before the initializer? "init is awaited in this function" ≠ it's the first touch of that state. Trace to the earliest cross-feature writer. |
| **Overlap / undercount** | Multiple writers to one accumulator: is the same event recorded by >1 path (double count)? Is some event class recorded by no one (undercount)? Don't trust the comment's "this only records X" division. |
| **Lost update** | fire-and-forget write + a following read: does it read a stale value and **overwrite** the locally-correct value? |
| **fail-open wrong value** | Does the degradation path let through a *wrong value* or a *missing value*? Returning a confident wrong value for poisoned input = only half safe. **To judge fail-open/fail-closed, read the guard's actual comparison (`x < threshold` vs `x >= threshold`); don't go by comment or intuition — `NaN`/`Infinity` sentinels make every `x < threshold → safe return` guard evaluate false, so both let-through and block paths break at once.** |
| **Cross-process / re-entry** | Does retry / recovery / multi-worker reusing the same state cross-talk? Does implicit context (ALS / log context) survive a cross-process hop? |
| **Provenance break** | From source to landing point, is the key value no-op'd / overwritten / type-erased midway? (Seeing dedup / skip / idempotency guards → ask what failure they prevent → go to the path **without** them, or an older version, to find that failure.) |

**Escalation: frame hard ordering / arithmetic as satisfiability** (optional — only for **order race** and **extreme value / cancellation**, and only when informal reasoning is uncertain):
1. **Free variables** — what can vary? Order (writer interleaving), values (key numeric variables / inputs / counts).
2. **Write only real constraints** — which happens-before relations truly hold (await / lock / queue order)? Which are just your **assumption**? + the invariant to hold.
3. **∃ counterexample?** — **SAT** (found an order/value that breaks the invariant) = bug, the counterexample is the trigger scenario; **UNSAT** = proven safe under this model (only if the model is faithful to the code).

**Three disciplines (they decide findings quality)**
1. **A census isn't done until both order + overlap relations are verified.** Listing writers ≠ census complete.
2. **No downgrading on local observation.** For ordering / race findings, before downgrading you must trace the timeline to the state's birth. SMT phrasing: did you treat a non-existent happens-before as a constraint? ("some step is awaited in this function" ≠ it globally precedes other components' writes to the same state.) Not traced → mark `needs dynamic verification`, keep the tier, don't drop to ⚪.
3. **Green tests ≠ verification.** Ask: is there an **adversarial-order** test (shuffled / retry / write-before-init)? Was the layer the bug lives in mocked away? With a real trace, reconcile against the timeline.

---

## Produce findings (summary table + details)

Severity = **impact × probability**; give the product conclusion directly, don't make the reader do the math. Merge and dedup findings from both tracks. First output a **short summary table** for scanning, then expand each finding by number. Do not put long evidence into table cells; chat windows squeeze wide Markdown tables into unreadable narrow columns.

**Every finding carries an `Origin` tag.** The first question a reader has is "which of these did this change cause, and which were already there" — the report answers that itself instead of waiting to be asked. The test is a **revert counterfactual** (revert this change: is the problem still there?), NOT "is the offending code inside the diff":

- **Introduced** — reverting makes it disappear. **This includes half-landed changes**: a contract / enum / mirror table / bilingual copy / generated artifact updated on one side but not the other — the offending line **counts as introduced even when it is not in the diff**, because this change is what put the two sides out of sync. This is the class most often misjudged as pre-existing.
- **Activated** — reverting leaves it there, but unreachable / inconsequential. This change is what made it reachable or costly for the first time (a new entry point, caller, provider, config shape).
- **Pre-existing** — it happens either way; no causal link to this change, just spotted in passing.

`Origin` is **orthogonal** to severity: severity is decided by impact × probability alone. `Pre-existing` is not downgraded for "not being from this change", and `introduced` is not upgraded for "being freshly written" — the two answer different questions: whether to fix, versus who fixes it and when.

| # | Severity | Origin | Location | Consequence | Direction |
|---|---|---|---|---|---|
| 1 | 🔴/🟡/⚪ | introduced/activated/pre-existing | `file:line` | What happens if unfixed (one plain-language consequence, no jargon) | One-sentence repair direction |

Then expand by table number:

```
### #1 <one-line consequence>
Impact: how bad + to whom (specific: which users / tenants / callers, not a vague "users")
Probability: how likely + what it depends on; if uncertain, say how to raise/lower the tier
Origin: introduced / activated / pre-existing + a one-line counterfactual reason (what a revert would do); for "one side updated, the other not", name both sides
Evidence: static → which triangulation reference fails (intent / input domain / surroundings); dynamic → preserve the subagent's state diagram / timeline / change trajectory / counterexample path, not a one-line summary
Fix: what it should be / the broken invariant (jargon goes here)
```

- 🔴 high probability × big impact (blocks release) | 🟡 should fix but not bleeding | ⚪ nice-to-have. Sort summary table rows in descending severity, and **within the same severity order introduced → activated → pre-existing**, so "this change's bill" stays together.
- `Consequence` states the **outcome**, not the technical cause (cause goes in the expanded `Evidence` / `Fix`).
- `Location` must be a precise `file:line`; for a merged accomplice finding, use the core location in the table and mention other locations in the expanded `Evidence`.
- Keep `Direction` in the summary table to one sentence; put the complete `Fix` in the expanded detail.
- Expanded details are the complete information surface: if a subagent provided a dynamic model, timeline, path labels, or key code locations, the final report must carry those details forward; the summary table may be short, the detail must not erase evidence.
- `Origin` is one of the three, never blank. If you can't tell, put the most likely one and say in the expanded `Origin` line what is missing to settle it (usually "haven't confirmed whether this code was touched by this change" — go check `git diff`, don't guess).
- If a merged accomplice finding spans two origins (typically a newly introduced caller plus a pre-existing underlying defect), the table takes the **earlier** one (introduced > activated > pre-existing) and the detail explains both sides.
- Multiple same-root-cause → mark as **"accomplices"** in `Consequence` or expanded `Evidence` and merge into one finding; don't report the same problem twice (especially when static/dynamic each see one side of the same line).
- Expand each finding only once after the table; don't also emit a 7-column long table.
- Close with a one-line origin tally first: `introduced N (🔴a 🟡b ⚪c) / activated M / pre-existing K`, so the reader doesn't have to count back.
- Then the **impact × probability** gradient chart:

```
Impact ↑
 big  │ 🔴 #1
 mid  │            🟡 #2
 small│ 🟡 #3                  ⚪ #4
      └─────────────────────────────→ Probability
        low        mid        high
```

## Don't go through the motions
- **Don't skip escalation**: if Step 1 flagged a dynamic surface, you MUST actually run Part B on it (modelling + 6 risk classes); scanning statically once and stopping = a miss.
- **Don't compete with tools** (static): style / format go to linter & type-checker, cr only picks the semantic layer.
- **Don't hand-wave the dynamic**: seeing state / timing / concurrency, **build the model** (Part B), don't conclude from reading code.
- **Trivial changes**: no dynamic surface → skip Part B, deliver only a static report.