--- name: cr description: "Full code review: triangulate every change statically, model state/timing slices dynamically. Use when reviewing a PR / branch / commit / working-tree diff; pass `hard` for per-slice subagent fan-out." --- # cr — Full Code Review > Form: **text diagrams for the skeleton, pseudocode for the deterministic logic > (orchestration, gating, report format), short prose checklists for judgement.** Most review is static — read the snapshot, judge right/wrong; easy, never skip it. The hard part is **deriving dynamic behaviour from static code** (timing / state / concurrency). cr does both tracks. ## Principles (reason from these when no rule fits) - **P1 Independent judgement** — reviewers are clean subagents; they never see each other's work; the main session never judges, filters, or steers between them. - **P2 Lossless relay** — the main session owns only the frame; everything inside a finding is the reviewer's own words. - **P3 Real behaviour over proxies** — judge what code and dependencies actually do (source, comparison, caller), not what a name, comment, mock, green test or intuition suggests. - **P4 Solution diagram first, then the breaks** — the reader understands a problem only against the whole solution; draw once, say only what the drawing can't; rate by the harm the broken promise existed to prevent. --- ## 0. Skeleton ```text ┌──────────────────────────────┐ │ main session │ │ run_cr(target, user_args) │ dispatches + assembles, └──────────────┬───────────────┘ never reviews (P1) ▼ ┌──────────────────────────────┐ │ choose_tier (by size only) │ └───┬──────────┬───────────┬───┘ tiny │ default │ │ user said "hard" ▼ ▼ ▼ ┌─────────┐ ┌─────────┐ ┌───────────────────┐ │ 1 × all │ │ static │ │ triage → static + │ │ │ │ dynamic │ │ 1 per slice │ └────┬────┘ └────┬────┘ └─────────┬─────────┘ └───────────┼────────────────┘ ▼ each reviewer, in parallel, isolated ┌──────────────────────────────┐ │ review(scope) §2 │ │ triage → Part A → Part B │ │ → emit ReviewerReport │ └──────────────┬───────────────┘ ▼ ┌──────────────────────────────┐ │ format_gate §4 │── fail ──► back to the SAME └──────────────┬───────────────┘ reviewer, re-emit pass ▼ ┌──────────────────────────────┐ │ assemble §1 │ │ merge accomplices, place ✗#n │ └──────────────┬───────────────┘ ▼ heading+verdict → diagram → findings → coverage ``` --- ## 1. Main session > Dispatched as a review subagent? You are the clean reviewer — skip to §2. Don't spawn > further subagents or disclaim their absence. ```python def run_cr(target, user_args): intent = requirements_only(target) # PR text / requirement / commit message; # never the dev session's rationale tier = choose_tier(target, user_args) scopes = { "single": [Scope("all")], "default": [Scope("static"), Scope("dynamic")], "hard": [Scope("static")] + [Scope("slice", s) for s in spawn_triage(target)], }[tier] # tier == "hard" and no slices → note "no dynamic slice, fan-out degenerates to 1 static" reports = parallel(spawn_clean(PROMPT.format(target, intent, s)) for s in scopes) for r in reports: while (errors := format_gate(r)): # §4 — countable checks only r = ask_same_reviewer(r, "fit your own findings to the format", errors) return assemble(reports) def choose_tier(target, user_args): if "hard" in user_args: return "hard" # only when the user asks; never escalate, # never suggest it if clearly_tiny(target): return "single" # size only — dynamic-surface is the return "default" # reviewer's call; unsure → default PROMPT = """Read this skill (); review {diff range}; the change intends: {intent}; running tests is . Apply §2 to {scope}, triage it yourself, emit a ReviewerReport (§3).""" ``` ```python def assemble(reports): # MAY: ask a reviewer for ITS OWN missing detail; format-gate bounce # MUST NOT: relay one reviewer's findings / hints to another; point a reviewer at a path; # downgrade, drop, re-title, rewrite, summarise, reclassify Origin # dispute a finding → spawn a FRESH clean subagent to re-check findings = merge_accomplices(f for r in reports for f in r.findings) # same root cause → one finding: # severity = max(ratings) origin = earliest(introduced > activated > pre-existing) # body = the block tracing more of the effect, verbatim # + one line: "Independently confirmed by the reviewer (, rated )" findings.sort(key=(severity desc, origin order)) diagram = static_report(reports).diagram # static reviewer covers the whole diff for f in findings: diagram.mark_once(f.break_location, f"✗#{f.n} {f.severity} {f.break_note}") # mechanical placement only — no other edit to the diagram return render(heading_and_verdict(findings), # the ONLY text the main session writes # (incl. the > 5 findings index, §4), diagram, # together with the coverage block [f.block for f in findings], coverage(reports)) ``` --- ## 2. Reviewer ```python def review(scope): static_surface = all_changes(diff) dynamic_surface = slices_touching(diff, "state | timing | concurrency | retries | cross-process hop") # A judges as-written; B judges over-time. Same spot can be in both. findings = [] if scope in ("all", "static"): findings += part_A(static_surface) # checklist §2.A diagram = draw_solution_diagram(diff) # §3 rules; working copy for the report if scope in ("all", "dynamic", "slice") and dynamic_surface: slices = enumerate_all_slices(dynamic_surface, ARCHETYPES) # list ALL first for s in slices: # a missed slice = all its risks missed model = build_model(s) # state diagram · timeline · trajectory for risk in RISKS: # §2.B, all six, every slice findings += evaluate(risk, model) coverage.add(s, "✗#n" if s has finding else "✓") # slice not modelled → coverage.add(s, "— not modelled: ") for f in findings: f.consequence = "When , does not happen." f.origin = revert_counterfactual(f) # §3 f.severity = rate(f) # §3 if not traced_to_birth(f) and f.kind == "race": f.probability += " needs dynamic verification: " # keep the tier return ReviewerReport(findings, diagram, coverage) # working notes stay with you ``` ### 2.A Static checklist — triangulate against intent / input domain / surroundings Only semantic problems tools can't judge (style / format / unused / type errors → linter). - **A1 vs intent** — do name / signature / PR text / comments **promise** what the code does? Do error / boundary branches return the right thing, not the happy-path value? Comments and docs in sync? - **A2 vs input domain** (#1 bug source) — null / empty / single / duplicate / boundary / overflow / negative / unicode / over-length? Error path as correct as happy path, including the error **shapes** that actually arrive (P3)? Resources closed, state consistent on early return? External input treated as untrusted? - **A3 vs surroundings** — contract drift (all consumers updated?); **dependency contracts** checked in the dependency's source or docs (returns / throws / internal behaviour / defaults / units, P3); convention / layering; types bypassed by `any` / casts; security checks the siblings all have. - **Wrap-up** — duplicated decisions; dead code; local perf (N+1, IO in loop, unbounded); tests: boundary / error assertions, red-before-fix, **test doubles behave like the real boundary** (P3). ### 2.B Dynamic checklist `ARCHETYPES` — sweep all when enumerating slices: - shared-state init + multi-write · state reused across processes / re-entry · check-then-act across an async gap (TOCTOU) · fire-and-forget write + later read · implicit context across a hop · **state machines inside a dependency** (auto-retry, backoff, cache, pool, buffer — what does the caller observe while it runs and once it ends?) `build_model` — read who reads / writes (across features, callers, into dependencies), then draw for yourself: **state diagram** (who changes it under what condition), **timeline** (all actors on one axis, traced to where the state is truly born), **change trajectory** (created / changed / read / overwritten / lost). `RISKS` — evaluate every one on every slice: | Risk | Look for | |---|---| | Order race | a writer before the initializer; "awaited here" ≠ first touch | | Overlap / undercount | same event recorded twice, or by nobody | | Lost update | stale read overwrites the correct value | | Fail-open wrong value | degradation lets a wrong / missing value through; read the guard's actual comparison (`NaN` is false both ways) | | Cross-process / re-entry | reused state cross-talks; implicit context lost over the hop | | Provenance break | key value no-op'd / overwritten / wrapped / type-erased between source and use | Uncertain about ordering or arithmetic → frame as satisfiability: free variables, only real happens-before constraints, invariant; SAT = bug with its trigger, UNSAT = safe. Disciplines: a census isn't done until order + overlap are verified · no downgrading on local observation · green tests ≠ verification (was the bug's layer mocked away?). --- ## 3. Finding semantics ```python def revert_counterfactual(f): if gone_if_reverted(f): return "introduced" # incl. half-landed changes: one side # of a contract/enum/mirror updated, # even if the line is outside the diff if harmless_if_reverted(f): return "activated" # this change made it reachable return "pre-existing" # never blank; unsure → likeliest + what would settle it. Orthogonal to severity. def rate(f): if f.impact == "big" and (f.probability != "negligible" or f.irreversible): return "🔴" # irreversible = data loss / wrong money / security if f.should_fix: return "🟡" return "⚪" # a safeguard failing in the very scenario it exists for is rated by the harm it was # meant to prevent — "no worse than before the change" is not a downgrade ``` `Consequence` — the first promised behaviour that fails, under its trigger, one clause: **"When , does not happen."** Mechanism → `Break`; downstream → `Effect`. ("X instead of Y" smuggles in a downstream effect — state Y's failure.) **Solution diagram rules** — one ` ```text ` block, top to bottom, ≤ ~80 columns: one box per component (name + `file:line`), decisions as branches labelled with their condition, happy path straight down, dependencies collapsed to one box unless a break lives inside. It is a claim about behaviour (P3): every arrow is real control flow, arms that don't join in code don't join here, every path drawn to where it ends. Lines never cross a box or each other — repeat a terminal box (e.g. the same fail-closed exit) at the end of each path rather than route a long line across the diagram. Annotations in a right-hand column. Each finding marked **once**: `✗#n 🔴 `. ```text Webhook dedup (this change) ┌───────────────────────────┐ │ provider webhook delivery │ └─────────────┬─────────────┘ ▼ ┌───────────────────────────┐ │ receive handler.ts:40 │ └─────────────┬─────────────┘ ▼ ┌───────────────────────────┐ │ dedup lookup dedup.ts:12 │ ✗#1 🔴 key = id + timestamp: │ key = id + timestamp │ a redelivery never matches └──────┬─────────────┬──────┘ hit │ │ miss ▼ ▼ ┌──────────────┐ ┌──────────────────────┐ │ ignore │ │ process handler.ts:58│ │ (promised │ └──────────┬───────────┘ │ for repeats) │ ▼ └──────────────┘ ┌──────────────────────┐ │ charge billing.ts:90 │ └──────────────────────┘ ``` --- ## 4. Report format and gate ```python REPORT = [ # exactly these four parts, nothing else "heading+verdict", # "## Review: " + " findings (🔴a 🟡b ⚪c). Worst: " "diagram", # §3; unmarked if no findings "findings", # one block each, sorted "coverage", # ```text block ] # > 5 findings → index before the diagram, a Markdown list, one item per finding: # "- #1 🔴 introduced — " (bare lines collapse into one paragraph) # language = the user's; identifiers and file:line unchanged # blank line before the "## Review" heading and before and after every code block # test runs / scratch scripts / side notes → a finding's Why, or the last coverage line def format_gate(report) -> list[str]: # countable checks only; never judges content errors = [] for f in report.findings: body = f.block.body if not body.is_fenced("text"): errors.append(f"#{f.n}: body not in ```text") if body.line_count() > 8: errors.append(f"#{f.n}: {body.line_count()} lines > 8") if body.max_line_width() > 100: errors.append(f"#{f.n}: line > 100 chars") if body.has_sub_bullets(): errors.append(f"#{f.n}: sub-bullets") if f.block.heading != f"### #{f.n} {f.severity} {f.consequence}": errors.append(f"#{f.n}: heading ≠ Consequence") for line in report.coverage.slice_lines(): if words(line.name) > 6: errors.append(f"coverage: '{line.name}' > 6 words") if report.diagram.marks_per_finding() > 1: errors.append("a ✗#n marked more than once") if report.has_parts_outside(REPORT): errors.append("content outside the four parts") return errors ``` Finding block — Markdown heading, body in a ` ```text ` block (plain text, no backticks, continuation lines indented under the field): ```` ### #1 🔴 When the provider redelivers the same webhook, the duplicate is not ignored ```text introduced · every redelivery; depends on provider retry policy Break: ✗#1 dedup.ts:12 — the key includes the delivery timestamp, so a redelivery never matches Effect: redelivery → processed again (handler.ts:58) → charged twice (billing.ts:90) Why: dedup must key on what stays stable across redeliveries; verified against the provider's retry docs Fix: key on the delivery id alone; add a test that redelivers the same payload ``` ```` `Break` may carry a flat zoom-in of at most 3 lines when the mechanism needs it. Coverage block — always present, one slice per line, marks `✗#n` / `✓` / `— not modelled:`: ```text introduced 1 (🔴1) / activated 0 / pre-existing 0 S1 dedup lookup on redelivery ✗#1 S2 concurrent first deliveries ✓ S3 retry after handler crash — not modelled: no retry path in diff ``` Static-only reviewer → `slices: none (static only)`. --- ## Self-check ```python assert every reviewer triaged and judged alone; nothing relayed between tracks # P1 assert every report passed format_gate, or was bounced to its own author # P2 assert main session wrote only heading+verdict (+ index) and coverage; findings verbatim # P2 assert blank line before "## Review"; index (> 5 findings) is a "- " list, one per line # §4 — # main-session text never passes format_gate assert every behavioural claim rests on real code / source, not a proxy # P3 assert diagram arrows == real control flow; each ✗#n once, right-hand column # P4 assert every slice ✓ / ✗#n / not-modelled; every finding has an Origin # coverage ```