---
name: code-review
description: Use when reviewing written code — a diff, a branch, a pull request, or work an agent claims is finished. Also use as the self-review gate before pushing your own finished change (the gate is named pre-push; a draft push never waits for it). Fans out a parallel council of specialist review lenses over the change, runs the host's declared deterministic gates, and computes an auditable Approve / Request-changes / Needs-human verdict. Do NOT use for standalone authoring or fixing work unrelated to a review (the pre-push gate fixes only the findings it raised), for authoring tests, or for a standalone vulnerability scan — invoke the security-review skill for that.
license: MIT
---

# Code review

Review has three layers, and keeping them apart is the whole design.

| Layer | What it is | Who decides |
|---|---|---|
| **Gates** | Repeatable commands — build, tests, coverage, analyzers, scanners | Nobody. They measure. |
| **Council** | Specialist lenses reading the diff in parallel | A model, per lens, within its remit |
| **Verdict** | Fixed rules over the findings and gate results | Unit-tested code, no model |

The judgement in a review is the council's. The gate that judgement passes through is not.
A verdict produced by the same model that produced the findings cannot be audited, cannot be
regression-tested, and can legitimately differ between two runs over the same diff — so the
last step is arithmetic instead.

## Procedure

### 1. Scope the change

Establish the diff, the files it touches, the technologies present, and the stated intent —
the request, the commit or pull-request description, and any governing plan or specification
under the paths declared as **<plan-index>** and **<specification-index>** in the root
`AGENTS.md` registry.

Two things fall out of this and govern everything after it: **which lenses can possibly
apply**, and **whether the change is reviewable at all**. A diff past the host's
`review.policy.max-reviewable-lines` ceiling is not made safe by reviewing it harder; it
escalates.

**Load the review guidance.** Read the host's review guidance declared as
**<review-guidance>** in the root `AGENTS.md` registry (`REVIEW.md`) before any lens runs. Its
ground rules and the sections that match the changed technologies are the host's own standard
for this change. Each seat receives its assigned sections and the ground rules in its
invocation, and every seat must apply them. The same guidance binds the pre-push self-review
gate, the refutation pass in step 3, and any in-process council fallback in step 2: none of
them reviews against a generic standard instead. If the file is missing or no path is
declared, the report says so in one line rather than reviewing without notice.

### 2. Start the gates and the council together

They are independent. Neither waits on the other.

**Gates.** Two commands, and they are the only things this skill executes:

```bash
kyber-weave review gates . --out artifacts/gates.json
kyber-weave review duplicates . --out artifacts/duplicates.json
```

Write both under `artifacts/`. The CLI subprocess writes them via `process.execute`, so a
denied write tool on the reviewer does not block the run. If the files cannot be persisted,
keep the JSON from the command's response and pass it to `review verdict`.

The second is separate because it is not host-declared work: it reads the repository's
CodeGraph index and clusters symbols whose bodies are identical, which nothing in a normal
gate suite does. Where the repository has no `.codegraph/` index it says so and finds
nothing — indexing is the owner's decision, and the report must not read as though the check
passed.

It runs what the host declared under `review.gates` in `.kyber-weave/kyber-weave.yml`, in
declaration order, and normalizes every result into one document. After the declared gates
finish, the runner collects Coverlet Cobertura from `artifacts/coverage` or `TestResults`
and attaches it to the gate report — that is the coverage evidence `test-adequacy` consumes.
Never substitute an ad-hoc command for a declared gate, and never skip a blocking gate
because you expect it to pass — the output is cited as evidence, and evidence you produced
by hand is not evidence.

If the host has declared no gates, the command says so. Record that: a review with no
executed evidence is a weaker review, and the report must not read as though everything
passed.

**Council.** Invoke one seat per applicable lens, all in flight at once, each named with its
lens file, the review scope, and its assigned review-guidance sections. Every lens declares an
applicability predicate and returns `SKIPPED` with a reason when the diff holds nothing it
owns — auto-skipping is what makes seventeen lenses affordable. Every seat given guidance sections
ends its output with one attestation line, `REVIEW.md sections checked: [<ids>]`, on both its
findings form and its `NO FINDINGS` form (a `review-triage` seat lists `Ground rules`). A
findings or `NO FINDINGS` seat without that line has not shown it applied the guidance; the
report records that as a coverage gap under Council coverage. `SKIPPED` seats return no
attestation, and a missing attestation is never a verdict input. Where the harness cannot start
separate seats, the reviewer applies each lens in turn with the same guidance sections and
returns the same attestation line.

The **Runner** column is not advisory. `review-lens` reads code and judges it. `review-triage`
takes a machine artifact — analyzer diagnostics, a manifest diff — and attributes it to the
change; that work is bounded and checkable, so it runs on the `fast` model profile. Sending a
judgement lens to triage buys shallow findings; sending a triage lens to judgement pays a
premium for attribution.

| Lens | Runner | Owns |
|---|---|---|
| [intent-alignment](references/lenses/intent-alignment.md) | `review-lens` | Does the diff do what its description says, and is it one change? |
| [correctness](references/lenses/correctness.md) | `review-lens` | Boundaries, absent values, error paths, state, real concurrency |
| [security](references/lenses/security.md) | `review-lens` | Exploitable vulnerabilities, via the `security-review` skill |
| [authz-tenancy](references/lenses/authz-tenancy.md) | `review-lens` | Authorization per access, tenant scoping, privilege boundaries |
| [di-composition](references/lenses/di-composition.md) | `review-lens` | Constructor injection, no locally created collaborators, registration |
| [model-placement](references/lenses/model-placement.md) | `review-lens` | Type classification against the declared coding standard |
| [prior-art](references/lenses/prior-art.md) | `review-lens` | Does this already exist? Duplicate types, divergent idioms, generality nothing asked for |
| [duplicate-implementation](references/lenses/duplicate-implementation.md) | **`review-triage`** | Bodies duplicated across the tree, attributed from the duplicates gate |
| [test-adequacy](references/lenses/test-adequacy.md) | `review-lens` | Would the tests catch this breaking? Behaviour over implementation |
| [static-analysis-triage](references/lenses/static-analysis-triage.md) | **`review-triage`** | Analyzer output, attributed and reported by rule id |
| [test-infrastructure](references/lenses/test-infrastructure.md) | `review-lens` | Is the shared test machinery sound: isolation, caches, fixtures, global state? |
| [documentation-accuracy](references/lenses/documentation-accuracy.md) | `review-lens` | Is every factual claim in the changed docs, plans and doc comments true of the code? |
| [performance](references/lenses/performance.md) | `review-lens` | Cost at scale, with a named growth dimension |
| [blast-radius-revertibility](references/lenses/blast-radius-revertibility.md) | `review-lens` | What else it reaches, and whether it can be undone |
| [supportability](references/lenses/supportability.md) | `review-lens` | Diagnosability, correlation, what leaks to the caller |
| [dependency-supply-chain](references/lenses/dependency-supply-chain.md) | **`review-triage`** | What the change asks the project to newly trust |
| [infra-workflow](references/lenses/infra-workflow.md) | `review-lens` | Migrations, workflows, infrastructure, and their trust boundaries |

Only three lenses are triage, and that is the honest count rather than a hedge. The rest need
a judgement about code that a fast model would make badly. `static-analysis-triage` is
nonetheless the single best candidate in the catalogue: it applies to almost every change that
builds, and it carries the largest input of any lens, so it is both the most mechanical seat
and the most frequently expensive one.

Three lenses consume gate output — `test-adequacy` needs the coverage report,
`static-analysis-triage` needs the analyzer results, and `duplicate-implementation` needs the
duplicates report. Issue each when its own gate completes, not behind a barrier on all gates.
When the host has declared a `review.coverage` floor and the gate report carries no coverage
evidence, `test-adequacy` treats that absence as a finding — not a skip, and not "we did
not look".

`prior-art` and `duplicate-implementation` divide one concern by unit and by evidence.
`duplicate-implementation` reports **bodies**, from a gate artifact, and may not report what
it found by reading. `prior-art` reports **types**, from reading, and may not report without
quoting the counterpart it found. Neither may report the other's unit.

**Technology checklists.** The per-technology references —
[C#](references/csharp.md), [Python](references/python.md), [React](references/react.md),
[SQL](references/sql.md), [Pulumi](references/pulumi.md), [Azure](references/azure.md),
[GitHub Actions](references/github-actions.md), [TypeScript](references/typescript.md),
[Rust](references/rust.md), [Tauri](references/tauri.md), [Go](references/go.md) — are lens
modifiers, not lenses. A lens loads
the checklists for the technologies actually present in the diff. Each checklist points at the
host's declared `<technology>-coding-standard` and states no rule of its own; an undeclared or
draft standard is reported, never replaced with a built-in checklist. A checklist's Review traps
section, where it has one, is a set of non-authoritative detection prompts: they are not rules,
they do not substitute for the declared standard, and a finding raised from one needs quoted
evidence like any other.

### 3. Confirm before believing

For every `major` or `critical` finding, spend one more `review-lens` invocation trying to
**refute** it. Say so explicitly in the invocation: the confirming instance argues the
finding is wrong and defaults to refuted when it cannot establish otherwise. Pass it the same
review-guidance sections, and have it test the finding's `guidance:` rule id against them: a
finding that misapplies the rule it names is refuted.

This is the adversarial pass the `security-review` skill already applies to vulnerability
candidates, generalized to every lens and for the same reason — a confident wrong finding
costs more trust than a missed finding costs safety. A refuted finding is dropped, not
downgraded.

### 4. Adjudicate

The reviewer applies the checks no individual lens can, because no lens sees the others:

- **Drop anything missing `excerpt`, `evidence`, or `failure_scenario`.** No charitable
  reconstruction — a finding you complete on the reporter's behalf is one you are inventing.
- **Drop anything whose `confidence` is below `review.policy.min-confidence`.** Equal to
  the threshold is kept; above it is kept. Below it is noise.
- **Verify the quote.** Open the file and confirm the excerpt is really there. A finding
  quoting code that does not exist invalidates that lens's whole run, and the report says so.
- **Reconcile duplicates** — keep the most concrete, and note the corroboration.
- **Reconcile contradictions** — read the code and resolve, or escalate as unresolved.
- **Check the claims against the gates.** Any assertion that something was built, tested, or
  verified, with no corresponding gate result, is a `critical` finding.

### 5. Compute the verdict

```bash
kyber-weave review verdict . --findings findings.json --gates artifacts/gates.json
```

Rules, in evaluation order. The first that fires decides:

| # | Rule | Verdict |
|---|---|---|
| 1 | A changed path matches `review.policy.always-human` | `NEEDS_HUMAN` |
| 2 | The diff exceeds `max-reviewable-lines` | `NEEDS_HUMAN` |
| 3 | A blocking gate failed | `REQUEST_CHANGES` |
| 4 | Any surviving `critical` finding | `REQUEST_CHANGES` |
| 5 | Surviving `major` findings reach `major-count-blocks` | `REQUEST_CHANGES` |
| 6 | Otherwise | `APPROVE` |

Findings whose `confidence` is **below** `review.policy.min-confidence` are excluded before
the critical and major blocking rules. Equal to the threshold is kept; above it is kept.
Below it is dropped. The engine compares `Confidence < MinConfidence`.

**Strictness.** `review.policy.strictness` is a named preset over those two numbers, not a
third mechanism. `balanced` (the default) is `min-confidence: 7`, `major-count-blocks: 3`;
`lenient` is `8` and `5`; `strict` is `5` and `1`, so one surviving major requests changes. A
host that also declares `min-confidence` or `major-count-blocks` overrides the preset for that
number. Minor findings never block at any level. Pass the strictness and the effective
`min-confidence` to **every seat** in its invocation, so a strict host hears lower-confidence
findings instead of having them dropped at the source and a lenient host is not flooded.

The two escalation rules run **before** any finding is weighed, because a reserved path and
an unreviewable diff both say the same thing: this change is not the engine's to settle,
however clean or filthy the council's report was.

**Remediation pass note.** A `REQUEST_CHANGES` verdict on actionable Critical/Major findings earns one bounded remediation pass through the conductor (same session, same file scope, fresh task-gate re-run) before any park flag; Unactionable or second-failure findings park immediately with their finding ids (stop reworking each and list the ids in the PR; park never holds back delivery). Minor/Info findings never trigger a remediation loop and are reported without blocking: the `major-count-blocks` threshold and the table above still decide the verdict, and the fixed rules are not rewritten by this note.

**Risk** (`LOW` / `MEDIUM` / `HIGH`) is graded from what was found and what was touched,
never from diff size. A twelve-line migration dropping a column outranks a three-thousand-line
regeneration of a generated client.

**Coverage is reported, never decisive.** A verdict driven by a coverage number rewards
padding the number, which is exactly what the `test-adequacy` lens is there to catch.

Report the verdict the engine returned. You may not override it. If you believe it is wrong,
say so alongside it with the finding that should have changed it.

## Pre-push self-review gate

"Pre-push" is the name of this gate, kept for its heading and its references. It does not mean the gate runs before the push: the draft pull request is pushed first, and a draft push never waits for this gate. Required CI decides readiness.

Run this gate on your own finished change. Its verdict is evidence for the pull request description; it does not block the push or the ready step. It works the same in any
harness — Claude, Pi, OpenCode, Antigravity, or others — because it uses only the
Procedure above and the host's declared gates.

1. **Sync.** Refresh the remote head and the base branch before the draft push. If another
   actor advanced either, reconcile their changes into your branch and revalidate the
   affected work. Merge, never rebase, never force-push. Confirm the refreshed remote
   head and base branch are reachable from your local `HEAD` before the draft push.
2. **Council over the whole change.** Push first: the change goes up as a draft pull
   request before the council runs, so the council reviews a pull request that already
   exists. Then run full Procedure steps 1–5 over the whole accumulated diff against its
   base, not just the last files touched. Fix major-or-worse findings where a bounded fix
   exists, re-run steps 1–5 over the updated diff, and use that second verdict in the pull
   request description. A verdict computed before a fix is void. List any major-or-worse
   finding still open by id in the pull request description. Minor/Info findings never
   block. Required CI decides readiness; the verdict does not gate the push or the ready step.
3. **Defect-class checklist.** Walk each class explicitly over the whole change, as part of this pre-push gate (the draft push does not wait for it):
   a. Invented/hidden values: `?? 0`, `|| N`, `?? []`, clamps, default window/price/size/duration, unmeasured shown as zero or "measured", partial data looking complete (honest-unobservability: absent stays absent with reason).
   b. Scope/cost: filters/dedupe too wide or single-caller-only; N+1/per-run loops; unbounded reads/copies; caps without paging; large payloads kept alive.
   c. Parity: guard added in one place not every caller/branch (store vs direct-DB, harness vs run scope, API vs UI, producer vs consumer).
   d. Stale text: comments/docs/test names/version-migration numbers/PR description mismatch code (grep old value).
   e. Tests only pure helpers never real render/serve path; assertions locking bug; new test passing even if fix reverted.
   f. Lint and type gates actually run.
4. **Evidence.** Paste the real gate output into the push report. A claim with no
   gate result is "unverified", never "passing".

## Report format

### Verdict
- **Verdict:** `APPROVE` / `REQUEST_CHANGES` / `NEEDS_HUMAN`
- **Risk:** `LOW` / `MEDIUM` / `HIGH`
- **Rule:** the `KW-REVIEW-*` rule that decided it, and why it fired.

### Gates
One line per gate: id, pass or fail, duration, and for a failure the exact command and the
failing output. If no gates were declared or run, say that here in one line rather than
omitting the section.

### Findings
Most severe first:

- **[Severity] [id] — Title**
  - **Location:** `path/to/file.ext:line`
  - **Claim:** what is wrong.
  - **Evidence:** how it is known — a path and line read, or a gate result id.
  - **Failure:** the concrete scenario.
  - **Guidance:** the review-guidance rule id (or section, for an untagged bullet) applied, when the finding applies one.
  - **Suggestion:** the specific fix.

### Council coverage
Every lens, and its outcome: findings, `NO FINDINGS`, or `SKIPPED` with the reason. This
section is not optional. It is the difference between a dimension that was reviewed and one
that was quietly never looked at. A findings or `NO FINDINGS` outcome also carries its
attestation line, which lists the review-guidance sections it checked. A seat with no attestation
line is listed here as a coverage gap.

### Dropped
What was reported and removed, with the `KW-REVIEW-*` rule that removed it. A review that
hides its own false positives cannot be tuned.

## Configuration

Everything host-specific lives in one place — `review:` in `.kyber-weave/kyber-weave.yml`:

```yaml
review:
  gates:
    - id: test
      run: [dotnet, test, -c, Release]     # argv, never a command line
      blocking: true
    - id: inspectcode                      # .NET static analysis; see the resharper-clt skill
      run: [dotnet, jb, inspectcode, MySolution.sln, --output=.scratch/inspectcode.xml, --format=Xml]
      blocking: true
  coverage:
    file-line-percent: 85
    class-line-percent: 85
  duplicates:
    minimum-lines: 4                       # normalized lines, signature and braces excluded
  policy:
    strictness: balanced                  # lenient | balanced | strict; explicit keys below override it
    always-human: ["**/auth/**", "**/*secret*"]
    max-reviewable-lines: 10000
    # major-count-blocks: 3               # optional; overrides the strictness preset
    # min-confidence: 7                    # optional; overrides the strictness preset
    suppressions:
      - id: correctness/generated-nullable
        reason: Regenerated client, tracked separately.
        expires: 2026-11-18                # required; re-justify or it returns
```

`run` is argv because the gate runner refuses a shell. A pipeline or a redirect cannot be
expressed here, and neither can an injection — that is the trade, and it is the right way
round.