cr · diff
git:20260829.57c4561 to git:20260924.f61f662
290 added, 114 removed. Audit A to A.
---
name: cr
- description: "Full code review: triangulate every change statically, model state/timing slices dynamically."
+ 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
- Do a PR's complete review in one pass: **broad static sweep + deep dynamic dig**. This skill is **self-contained** in both methods.
+ > Form: **text diagrams for the skeleton, pseudocode for the deterministic logic
+ > (orchestration, gating, report format), short prose checklists for judgement.**
- 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.
+ 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.
- ## Execution mode (choose before you start)
+ ## Principles (reason from these when no rule fits)
- **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.
+ - **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.
- 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".
+ ## 0. Skeleton
- 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)`.
+ ```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
+ ```
- **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).
+ ## 1. Main session
- ## 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**.
+ > Dispatched as a review subagent? You are the clean reviewer — skip to §2. Don't spawn
+ > further subagents or disclaim their absence.
- 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.
+ ```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)
- ## Part A — Static triangulation: cross-locate the change against three references
+ 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)
- 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.
+ return assemble(reports)
- 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**:
+ 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
- ### 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?
+ 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)."""
+ ```
- ### 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.
+ ```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
- ### 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?
+ 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))
- ### 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?
+ 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,
+ diagram, # together with the coverage block
+ [f.block for f in findings],
+ coverage(reports))
+ ```
+
---
- ## Part B — Dynamic: derive the static code into a model, then review (dynamic-surface slices only)
+ ## 2. Reviewer
- 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.
+ ```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.
- **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?)
+ 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>")
- ⚠ **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.
+ 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
- ### 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**.
+ return ReviewerReport(findings, diagram, coverage) # working notes stay with you
+ ```
- ### 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.
+ ### 2.A Static checklist — triangulate against intent / input domain / surroundings
- These two diagrams are the evidence — many bugs (a 6-second gap, a key created too early) only reveal themselves once drawn.
+ Only semantic problems tools can't judge (style / format / unused / type errors → linter).
- ### B3. Evaluate 6 classes of dynamic risk on the model
+ - **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).
- | 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.) |
+ ### 2.B Dynamic checklist
- **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).
+ `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?)
- **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.
+ `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:
- ## Produce findings (summary table + details)
+ | 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 |
- 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.
+ 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?).
- **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.
+ ## 3. Finding semantics
- `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.
+ ```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.
- | # | 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 |
+ 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
+ ```
- Then expand by table number:
+ `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>`.
+
+ ```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 │
+ └──────────────────────┘
```
- ### #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)
+
+ ---
+
+ ## 4. Report format and gate
+
+ ```python
+ 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 → plain-text index before the diagram: "#1 🔴 introduced — <Consequence>"
+ # language = the user's; identifiers and file:line unchanged
+ # blank line 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
```
- - 🔴 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:
+ 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
```
- Impact ↑
- big │ 🔴 #1
- mid │ 🟡 #2
- small│ 🟡 #3 ⚪ #4
- └─────────────────────────────→ Probability
- low mid high
+ ````
+
+ `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
```
- ## 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.
+ 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 and coverage; findings verbatim # P2
+ 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
+ ```