Review

---
# Generated by djinn-gen-adapters from theme=djinn. DO NOT EDIT — regenerate via djinn-gen-adapters.
# djinn-compat: 1
name: review
description: "Single-lens code review: ONE reviewer agent sweeps the whole diff — implementation AND tests together — in one pass and emits the fenced JSON triage sidecar the review-loop gate parses (SHIP | SHIP-WITH-FIXES | REWORK plus severity_counts, invented_rate_pct, promoted_findings). One agent, one pass, one sidecar. It MUST NOT fan out. Run as the review-loop's body child (autonomous formula) or as the /wish orchestrator's single-lens review subagent."
---

<!-- djinn-compat-check: begin -->
**Before anything else:** run `djinn skills check --here` in the repo you are working in. If it exits non-zero, stop: relay its output to the user verbatim — it names the fix — and run no other step of this skill. On exit 0, continue.
<!-- djinn-compat-check: end -->

# Review — single-lens

## Overview

This skill is the **single-lens code review**. One reviewer agent reads the whole
diff a bead produced — implementation **and** tests together — and emits one
triage verdict.

The cost model is "one strong reviewer instead of a panel": the formula pins
this step to the strongest model precisely because there is no panel behind it
to catch what the single lens misses.

The output is a fenced ```json``` triage sidecar, so the loop gate (`djinn.check.review_approved` via
`ParseTriageSidecar`) routes it with no special-casing — including the
BLOCKER-count gate, the citation ground-truth gate, and the invented-rate gate.

## The single-lens rule — no fan-out

**You are the reviewer. You do the review yourself.** Fanning out would
reinstate the exact multi-agent cost this skill exists to cut, which is the
whole point of the step. So, absolutely:

- Do **NOT** invoke any other review skill or command to do the review for you.
- Do **NOT** delegate any part of the review to a `Task` / `Agent` subagent —
  not for evidence-gathering, not for a "second opinion", not to parallelize the
  rubric below.

One agent, one pass, one sidecar. If the diff is large, read more of it — do not
split it across helpers.

Two more hard rules, symmetric with every other review path:

- **Do not modify code.** No edits, no fixes, no "while I was in there". The
  loop's paired fix step applies findings on the next iteration.
- **Do not write files.** The sidecar goes in your visible response text.

## Inputs

The invoker provides:

1. **The bead, in a `<bead>` block** — its id, title, description,
   acceptance_criteria, design and notes, copied verbatim from the tracker by
   the invoker. Judge the bead from that block alone. Run no `bd` command: a
   reviewer may run in a sandbox with no beads database, and the block is the
   bead as the invoker read it. The `acceptance_criteria` lines are what the
   completeness concern below is scored against — one `criteria` row each.
2. **The diff to review** — either an explicit base ref, or the
   pre-implementation baseline. Absent an explicit base, reconstruct it:
   `git diff <base>..HEAD` (plus `git status` / `git diff` for uncommitted work).
   Every promoted fix finding must cite a `file:line` that lands inside this
   diff — the harness ground-truths citations against exactly this range.

3. **On iteration 2 and later, two extra blocks** — "What iteration N-1
   found" (the previous pass's promoted findings) and "What the last fix
   changed" (`git diff <that review's SHA>..HEAD`). They are the incremental
   half of this review; see "Answering the previous iteration" below. On
   iteration 1 neither block is present and there is nothing to answer.

If the prompt carries no `<bead>` block, stop and say so — a review with no
acceptance criteria in hand cannot be scored.

## Answering the previous iteration

The review loop is a conversation, not a series of independent samples. When
the two blocks above are present, your sidecar MUST answer them — the harness
computes the loop's progress from your answers, and a pass that ignores them
reads as a loop that is making none.

**For each finding the previous iteration promoted**, emit one `prior_findings`
entry — `{"file", "line", "status"}` — echoing the `file` and `line` exactly as
the block wrote them, so the harness can match your entry to the finding:

- `"status": "resolved"` — the fix landed; this is no longer true of the code.
- `"status": "unresolved"` — still true, whether the fix missed it, only
  half-applied it, or made it worse.

Check the code, not the fix diff's intent. A fix that edited the cited line
without addressing the finding is `unresolved`.

**Every `unresolved` entry MUST also be re-promoted** as a `promoted_findings`
row at the same `file` and `line`, with the severity it still deserves and an
`action` describing what is left to do. The `prior_findings` entry is your
verdict on the last fix; the promoted row is the work order. The harness
carries an unresolved prior finding forward on your behalf if you omit the row,
for as many passes as it survives — but it can only carry the wording of the
reviewer who first raised it, which is a description of the code before any of
these fixes touched it. Re-promoting is how the
fixer learns what the half-applied fix actually left behind. Do not use the
`prior_findings` entry alone as a way to say "still broken" and move on.

**A re-promotion is not a NEW finding.** `origin` describes findings this pass
raised for the first time; the harness knows every citation the loop has been
carrying — not just the last pass's — and ignores any `origin` you put on one of
those, so you cannot accidentally get a re-promoted row filed away as a
follow-up instead of fixed.
Tag it or don't — the row goes back to the fix step either way.

**For each NEW finding you promote**, add an `origin`:

- `"origin": "regression"` — the fix diff introduced it. Something in "What the
  last fix changed" is the reason this finding is true.
- `"origin": "latent"` — it pre-existed that fix. This pass is simply the first
  to look there.

Judge origin from the fix diff, not from how serious the finding feels. The two
are not the same claim and the harness treats them differently:

- A **regression** is the loop's own damage. It keeps the loop iterating.
- A **latent IMPORTANT** is filed as a P2 follow-up bead and is NOT handed to the
  fix step. The bead is tracked; the loop moves on.
- A **BLOCKER** blocks either way. `latent` buys it nothing.

So mislabeling a latent finding as a regression keeps the loop open on work it
was never meant to carry — the failure that deferred a bead after three passes
each found one new IMPORTANT somewhere else. Mislabeling a real regression as
latent ships it. Neither is a safe default; read the diff.

Both bullets above are about findings that are new at this citation. A latent
IMPORTANT the loop is already carrying is not filed a second time, however many
passes ago it was raised.

Omitting `origin` means `regression` (the conservative reading), and omitting
`prior_findings` entirely means "every prior finding is still unresolved as far
as the harness can tell" — both are honest fallbacks, neither is a shortcut.

## Process

### Step 1. Read the bead, then read the diff

Read the bead first so the acceptance criteria are in hand before the code
frames your judgment. Then read the whole diff — implementation and tests in one
sitting, not as two passes. Reviewing them together is the point: the tests are
evidence about the implementation, and the implementation is evidence about
whether the tests are vacuous.

### Step 2. Sweep the rubric

Sweep **every** concern below over the whole diff. Each one is a named concern
you are accountable for, not a suggestion.

**Implementation concerns**

| # | Concern | The question you answer |
|---|---|---|
| 1 | **Correctness bugs** | Bugs, nil dereferences, missing error paths, off-by-ones, unhandled failure branches. Does the code do what it says on any input, not just the happy one? |
| 2 | **Acceptance-criteria completeness** | Which of the bead's `acceptance_criteria` are **not yet represented in code**? Walk them one at a time, recording each as a `criteria` row (Step 5); a criterion with no corresponding code or test is an IMPORTANT at minimum. |
| 3 | **Simplicity / deletable code** | What could be **deleted** without breaking anything — dead branches, unused fields, a duplicated utility that already exists elsewhere in the repo, a layer of indirection carrying one caller? |
| 4 | **Behavior at scale** | What breaks at 10× the input volume the bead assumes (the bead's Scalability Assumption if it states one)? Where is the hot path, and what did this diff put on it? |
| 5 | **Adversarial-caller boundary assumptions** | Which assumptions about callers are wrong if the caller is adversarial or merely careless — unvalidated inputs, trusted-but-external data, invariants asserted in a comment instead of in code? |

**Test concerns**

| # | Concern | The question you answer |
|---|---|---|
| 6 | **Vacuously-true test assertions** | Would any assertion pass against an **empty** implementation? An assertion on a constant, on a value the test itself just set, or on "no error" alone is vacuous. Also: delete the most important line of implementation — which test catches it? If none, say so. |
| 7 | **Cheapest implementation that passes the tests** | What is the cheapest, dumbest implementation that passes every test in this diff? If shipping that would embarrass the author, the tests are underspecified — report it against the tests, not the implementation. When your prompt carries a `## Mutation check` report, every `survived` test in it passes with the bead's code change reverted: give each one exactly one disposition — a guard (it pins behaviour the diff deliberately keeps) listed under `mutation_survivors` (Step 5), or an IMPORTANT `kind: "fix"` test finding citing the test's `file:line` that says what it must assert to fail without the change. A survivor with neither is a refused review. |
| 8 | **Uncovered scenarios** | What scenario is not covered at all — error paths, empty/boundary inputs, the concurrent case? |
| 9 | **Tests coupled to implementation details** | Do any tests assert on internals (call order, private shape, log text) rather than behavior, such that a legitimate refactor breaks them? |

Concerns 1–7 are the ones this step is contractually accountable for; 8 and 9
round out the test lens.

### Step 3. Grade each finding

Assign every finding a severity:

- **`BLOCKER`** — the change is wrong or unsafe as written: a real bug on a
  reachable path, a missing acceptance criterion that makes the bead false, data
  loss, a broken invariant. A BLOCKER means the diff must not ship as-is.
- **`IMPORTANT`** — a real defect worth fixing before ship: a meaningfully
  underspecified test, a scale or boundary hazard, a missing error path that is
  reachable but not yet reached.
- **`NIT`** — worth doing, not worth blocking on. Naming, comment drift, a
  tidier arrangement. These are never fixed inline; whether one becomes a
  follow-up bead is decided by the `priority` grade below.
- **`INVENTED`** — reserved for the self-audit: a finding you could not anchor
  to a real `file:line` in the diff. Count it honestly rather than deleting it;
  the invented-rate is a diagnostic on the reviewer, and a silently-dropped
  invention teaches the harness nothing.

**Within NIT, grade a filing priority (`priority`).** Every promoted NIT `fix`
finding ALSO carries a `priority` grade that decides whether it becomes a
tracked backlog bead or stays report-only. The default is **P4** — anything
whose fix is more verification or polish rather than a behavior change. P4
explicitly includes pure polish, cosmetic, style, speculative-future and
test-micro-tightening findings (the canonical noise: "extract a constant", "use
`bytes.Cut`", "strengthen this assertion", "rename for clarity") **and** the
absent-verification shapes — untested error path, named-coverage gap, drift trap
between two sources. Those stay valuable as report content, but their fix is
"add a test / comment / sync", not a behavior change; graded at the filing floor
they buried the backlog under ~100 add-a-test beads (closed as a class in the
2026-08-03 triage). Grade **P3** when the NIT names a **deferred behavior defect**
— code in the diff that produces observably wrong runtime behavior (wrong
output, panic, false verdict, resource leak, silent data loss) which the fix
step is deferring rather than fixing this iteration. **P3 is report-only**: the
floor moved to P2 (operator ruling D1, 2026-09-03), so a P3-graded NIT is
dropped exactly like a P4 and stays in this sidecar for human reading. Grade
**P2 ONLY** when that deferred behavior defect is severe enough that recording
it in this sidecar is not enough — it has to be carried on the backlog until
someone fixes it. A `review-follow-up` bead has to *earn* P2; P2 is the only
grade that files. Litmus test: if the
entire fix is "add a test", "add a comment", or "sync two sources", it is P4 no
matter how real the underlying risk reads. When in doubt, **P4** — this sidecar
preserves the finding for human reading either way.

The `file_nits` filer reads this grade per-finding and files only at or above
the floor, so an ungraded NIT falls back to the filer's batch default and is
report-only. Grade explicitly when you want a finding tracked.
BLOCKER/IMPORTANT findings route to the fix step, NOT the follow-up filer, so
their `priority` is unused — omit it or leave it empty there, as for counters.

Every promoted **fix** finding MUST cite a real `file` and `line` from the diff.
A finding you cannot anchor is not a finding — either anchor it or count it as
`INVENTED`.

The one honest exception is a finding **about a caller of the changed code** —
"this changed signature breaks its three call sites", "the invariant this diff
altered is assumed here" — whose truthful citation is a file the diff never
touched. Cite the caller's real `file` and `line` and set
`"caller_of_changed_code": true`. The harness's citation gate then keeps the
finding instead of rejecting it for pointing outside the diff. Do NOT set the
flag to smuggle a citation you could not anchor: an unanchorable finding is
still `INVENTED`.

A finding that is an **anti-recommendation** ("do NOT do X", "resist the urge to
extract this") is promoted with `"kind": "counter"` rather than `"kind": "fix"`.
The fix step skips counters by contract; a counter carries no obligation to act.

### Step 4. Pick the verdict

- **`SHIP`** — no actionable findings at severity ≥ IMPORTANT. Note that a
  zero-findings `SHIP` on a non-trivial diff earns exactly one adversarial
  re-review before it is trusted, so do not reach for it to end the loop
  cheaply — reach for it when the code has actually earned it.
- **`SHIP-WITH-FIXES`** — ships once the listed BLOCKER/IMPORTANT findings are
  applied. This is the common verdict.
- **`REWORK`** — the approach itself is wrong; applying the listed fixes would
  not get there.

Do **not** invent findings to look thorough, and do not suppress a real one to
look agreeable. A truthful clean `SHIP` is a legitimate outcome.

### Step 5. Emit the sidecar

Emit a fenced ```json``` block as the **LAST** thing in your **visible** response
text — not inside a tool-result, not in a file. The harness extracts it from
your assistant text stream; a sidecar that never reaches visible text reads as a
parse failure and burns the loop's budget.

````
```json
{
  "verdict": "SHIP-WITH-FIXES",
  "severity_counts": { "BLOCKER": 0, "IMPORTANT": 2, "NIT": 3, "INVENTED": 0 },
  "invented_rate_pct": 0,
  "criteria": [
    { "index": 1, "status": "pass", "file": "internal/djinn/session/run_formula.go", "line": "1318", "note": "buildLoopFixPrompt reads the prior iteration's review output" },
    { "index": 2, "status": "partial", "file": "internal/djinn/session/implementer_test.go", "line": "88", "note": "the skip path is implemented but only the happy path is tested" },
    { "index": 3, "status": "pass", "file": "docs/verdict-state.md", "line": "35", "note": "the doc names the new writer" }
  ],
  "mutation_survivors": [
    { "test": "internal/djinn/session/run_formula_test.go:TestRunOneFormula_NoMutationGateInPipelineIsNoOp", "disposition": "guard", "note": "pins the no-gate pass-through the diff deliberately keeps" }
  ],
  "prior_findings": [
    { "file": "internal/djinn/session/formula_prompts.go", "line": "204", "status": "resolved" },
    { "file": "internal/djinn/session/run_formula.go", "line": "1332", "status": "unresolved" }
  ],
  "promoted_findings": [
    {
      "severity": "IMPORTANT",
      "kind": "fix",
      "origin": "regression",
      "file": "internal/djinn/session/run_formula.go",
      "line": "1332",
      "action": "Handle the nil-step case before dereferencing step.Prompt — a renamed step id panics here instead of erroring.",
      "effort": "S"
    },
    {
      "severity": "IMPORTANT",
      "kind": "fix",
      "origin": "latent",
      "file": "internal/djinn/session/implementer.go",
      "line": "221",
      "action": "The empty-prompt skip path is untested and predates this diff — a pre-existing gap this pass is the first to reach.",
      "effort": "M"
    },
    {
      "severity": "NIT",
      "kind": "fix",
      "priority": "P2",
      "file": "internal/djinn/session/file_nits.go",
      "line": "204",
      "action": "An empty findings batch writes a zero-length artifact instead of skipping, so every downstream iteration reads a false 'filed' verdict until someone fixes it — deferred this iteration and severe enough to carry on the backlog.",
      "effort": "M"
    },
    {
      "severity": "NIT",
      "kind": "fix",
      "priority": "P3",
      "file": "internal/djinn/session/file_nits.go",
      "line": "241",
      "action": "The skip path logs the batch id twice, so the run log double-counts skipped batches — a deferred behavior defect, but the sidecar record is enough.",
      "effort": "S"
    },
    {
      "severity": "NIT",
      "kind": "fix",
      "priority": "P4",
      "file": "internal/djinn/session/file_nits_test.go",
      "line": "88",
      "action": "The empty-batch error path is untested — add a case.",
      "effort": "S"
    },
    {
      "severity": "NIT",
      "kind": "counter",
      "file": "internal/djinn/session/formula_prompts.go",
      "line": "330",
      "action": "Do NOT extract a shared helper for the two builders — they diverge on the codex fallback and the shared shape would grow a mode flag.",
      "effort": "S"
    }
  ]
}
```
````

Field contract:

- `verdict` — exactly one of `SHIP`, `SHIP-WITH-FIXES`, `REWORK`.
- `severity_counts` — an object counting `BLOCKER`, `IMPORTANT`, `NIT`,
  `INVENTED`. The `BLOCKER` count here MUST equal the number of `BLOCKER`
  entries in `promoted_findings`; a mismatch is a structural fault and halts the
  loop.
- `invented_rate_pct` — `INVENTED` findings as a percentage of all findings
  (`0` when you found none).
- `criteria` — an array with **one entry per `acceptance_criteria` line**, in
  the bead's order, each `{index, status, file, line, note}`. Count lines the
  way the harness does: every non-blank line is one criterion. `index` is the
  1-based line number among those; `status` is exactly `pass`, `partial` or
  `fail` (lowercase); `file` and `line` cite the code or test that satisfies
  the criterion (for a `fail`, where the work belongs); `note` says in one
  sentence what you observed there. The harness copies each row onto the bead
  as its per-criterion evidence record and sets `evidence_failed` to the count
  of rows that are not `pass`. A sidecar missing a row for any line is refused
  and the review re-runs — walk every line, including the ones about docs,
  tests and lint.
- `mutation_survivors` — an array of `{test, disposition, note}`, one entry per
  `survived` test in the prompt's `## Mutation check` report that you judge a
  guard (rubric 7). `test` is the report's `file:Name` (or bare `Name`),
  `disposition` is exactly `guard`, and `note` names the behaviour it pins. A
  survivor you do not accept as a guard is instead an IMPORTANT `kind: "fix"`
  finding citing its file. A sidecar that leaves a survivor with neither is
  refused and the review re-runs. Omit the array when there is no report or it
  names no survivor.
- `prior_findings` — an array of `{file, line, status}`, one entry per finding
  the PREVIOUS iteration promoted, with `status` exactly `resolved` or
  `unresolved` (see "Answering the previous iteration"). Absent on iteration 1,
  where there is no previous review; an absent array on a later iteration reads
  as "no prior finding could be confirmed resolved". Each `unresolved` entry
  MUST have a matching `promoted_findings` row at the same citation.
- `promoted_findings` — an array (possibly empty) of
  `{severity, kind, file, line, action, effort}`, plus `origin` from iteration 2
  on, `priority` on NIT `fix` findings and `caller_of_changed_code` on a caller
  finding (see Step 3). `kind` is `fix` or `counter`. `effort` is a rough size
  (`S` / `M` / `L`).
- `origin` — `"regression"` (the last fix introduced this finding) or
  `"latent"` (it pre-existed that fix). Omitted or unrecognized reads as
  `regression`. It decides whether an IMPORTANT holds the loop open or is filed
  as a P2 follow-up bead; a BLOCKER blocks under either origin. It applies only
  to findings NEW at their citation — a re-promotion of a finding the loop is
  already carrying, from any earlier pass, carries no origin, and any origin on
  one is ignored.
- `priority` — the per-finding filing grade for NIT `fix` findings: `"P4"`
  (report-only — the default, including the polish and absent-verification
  shapes), `"P3"` (a deferred behavior defect — report-only, below the P2
  floor), or `"P2"` (a deferred behavior defect severe enough to be carried on
  the backlog until fixed, worth a tracked `review-follow-up` bead — the ONLY
  grade that files), per the within-NIT rubric in Step 3. Optional and unused for
  BLOCKER/IMPORTANT (they route to the fix step, not the filer) and for
  counters; omit it or leave `""` there. An omitted or empty grade on a NIT
  falls back to the filer's batch default priority, which is below the P2 floor
  — an ungraded NIT is therefore report-only.

After the JSON is in your visible text, stop.

## Scope boundary

This skill judges **the diff**. It does NOT:

- Judge whether the bead was the right work to do (that is `/plan-to-beads`).
- Judge whether the bead's criteria are concrete enough to implement against
  (that is `/ac-quality-review`, which already ran before implementation).
- Render the bead's final acceptance verdict (that is `/verify-acceptance`,
  which runs after this loop on the settled tree).
- Apply its own findings (that is the loop's paired fix step).