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

                          ┌──────────────────────────────┐
                          │ 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.

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 (<absolute path to this file>); review {diff range};
the change intends: {intent}; running tests is <allowed / not allowed>.
Apply §2 to {scope}, triage it yourself, emit a ReviewerReport (§3)."""

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 <other> reviewer (<file:line>, rated <sev>)"
    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

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: <reason>")

    for f in findings:
        f.consequence = "When <trigger>, <the promised behaviour> 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: <what's missing>"   # 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:

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

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 <trigger>, <the promised behaviour> 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 🔴 <what>.

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

REPORT = [                                  # exactly these four parts, nothing else
    "heading+verdict",   # "## Review: <target>" + "<N> findings (🔴a 🟡b ⚪c). Worst: <Consequence verbatim>"
    "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 — <Consequence>"   (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::

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

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