Review

---
name: review
description: 'Review a kanban feature or bug against its approved scope, current testing evidence and layered review rules: establish the facts mechanically, pick the review tier from the diff, dispatch independent code-reviewer agents, verify their evidence, and write PASS/FAIL/REJECT/REQUIREMENT_BUG for the current execution. Use in review.'
---

# Kanban Phase 5 — Review (Autonomous, independent reviewer)

Plugin root: `${CLAUDE_PLUGIN_ROOT}` — the reference files of this skill write paths with this placeholder; resolve it to this path.

Argument: `<feature_name>`.

Stand on quality, not survivor. The orchestrator produces **facts** and a **pack**; an independent reviewer that did not write the code produces the **findings**; the report's `status:` is the truth `kf` trusts. The orchestrator never writes a finding of its own except the mechanical ones in step 2 — an orchestrator grading the work it supervised is the failure this split exists to remove.

**Every step leaves a named line.** `Agent(kf:code-reviewer)` in the transcript is auditable; "I reviewed the diff" is not. A missing line is a skipped step.

---

## 1. Move to review

```bash
kf stage {feature_name} review
```

(Already there → skip. If the move is refused, run `kf validate --change {feature_name}` — a missing, stale or non-PASS testing report blocks this direction; loop back through kf:test.)

Read `kf status --change {feature_name} --json`: the current `executionId`, `kind`, and the `worktree` object (`path`, `branch`, `domain`, `port`; `null` without a worktree) — the text form prints it as the `Worktree:` with `Domain:` or `Direct:` block. When `worktree.domains` is `false` in `.kf/config.json`, use `http://127.0.0.1:<port>` (the JSON outputs still carry `domain`; do not use it then). **Worktree items:** the reviewed changes live on the item branch in that worktree — every command below runs inside it. The worktree teardown on close refuses while it is dirty, and the review runs on a committed tree (step 2).

Create the run folder: `<item>/reviews/<executionId>/`. Every file this review produces goes there; a re-review after a repair loop gets a new execution id and a new folder, so old evidence is never mistaken for current.

**Re-run on the same execution after a CI watch stopped without a result** (no current report, the folder holds a board whose fingerprint equals `fp` now and that HEAD is pushed): resume at step 9 on that sha — the review, the board and `docs-reconciled.md` (step 2b) are still current; step 10 reads the docs result from that file.

## 2. Establish the facts — mechanical, before any reviewer

Facts are what the reviewer judges from; they are never a verdict.

**Baseline.** Read the `## Baseline` section of `tasks.md` (starting HEAD plus pre-existing working-tree changes). `BASE` is that HEAD, else the branch point (`git merge-base origin/<default> HEAD`). The review covers `BASE..HEAD` plus untracked, non-ignored files. Preserve unrelated pre-existing changes; they are not this item's.

**The review runs on a committed tree.** Tracked files with uncommitted changes → commit them on the item branch when they are this item's work, otherwise stop and ask the human; never commit unrelated changes as this item's. The checks below read the working tree while `review-package` (step 4) packages only committed `BASE..HEAD`, so it refuses a dirty tree (exit 4).

**Static checks.** Detect the real tooling from the repository (never assume `npm`), then run lint, build/typecheck and the suite in the worktree, recording each command, its exit code and the counts it printed. A suite that ran nothing is not a pass — report the count. Confirm the testing report is `PASS` for the current execution id.

**Regression tests fail on the baseline (bugs, and any task that fixes behaviour).** A test that is green with the fix removed protects nothing. Run the new or changed test files against `BASE` in a throwaway worktree (`git worktree add <tmp> "$BASE"`, copy the test files in, run them, remove the worktree): they must fail there and pass on `HEAD`. Record `regression on BASE: <N> failed (expected), on HEAD: <N> passed` under `## Verified`; a test that passes on `BASE` is a HIGH finding you record directly.

**Anti-pattern scan on added code lines** (prose files excluded — a skill that documents the scan is not a hit; untracked, non-ignored files count as added) — each hit is a HIGH finding you record directly, with the line as evidence:

```bash
ADDED_LINES=$( { git diff "$BASE" -- . ':(exclude)*.md' | grep '^+' | grep -v '^+++'
  git ls-files -z --others --exclude-standard -- . ':(exclude)*.md' | xargs -0 -r grep -Ih '' -- | sed 's/^/+/'; } )
printf '%s\n' "$ADDED_LINES" | grep -nE '^\+(<<<<<<<|>>>>>>>)'            # conflict marker
printf '%s\n' "$ADDED_LINES" | grep -nE '\.only\(|(^|[^A-Za-z0-9_$.])(fdescribe|fit)\('   # focused test locks out its siblings; not benefit( or x.fit(
printf '%s\n' "$ADDED_LINES" | grep -nE '(^|[^A-Za-z_])debugger([^A-Za-z_]|$)'
```

A failing build, lint or suite, or a conflict marker, **short-circuits**: write the report with those findings, `status: FAIL`, and skip steps 2b–9 — no model budget on a tree that does not build.

**Rules.** Resolve rule files per filename in project → user → package order: `{project_root}/.kf/review/rules/*.md`, `~/.kf/review/rules/*.md`, `${CLAUDE_PLUGIN_ROOT}/kanban-flow/review/rules/*.md` beside the installed CLI's package.json. Include general, security and performance plus the project's stack rule when installed (`kf rules` installs the stack packs; they are not auto-loaded from `${CLAUDE_PLUGIN_ROOT}/kanban-flow/review/stacks/`). If no rule source is available, the report says so instead of silently skipping. The rules go into the pack as paths; the reviewer reads them.

## 2b. Reconcile docs — after the mechanical checks pass, before any reviewer

The docs commit goes into the reviewed diff, so the reviewers see it and the board's fingerprint covers what was reviewed. Read `${CLAUDE_PLUGIN_ROOT}/skills/archive/reconcile.md` and follow it in the item worktree — it branches on `kind`. Its edits are one commit on the item branch, `docs({context}): reconcile docs superseded by {feature_name}`; nothing to change → no commit. Write its rows to `<item>/reviews/<id>/docs-reconciled.md` (reconcile.md, section 4); step 10 and a resume read that file, never memory. An open question on a doc: ask the human (reconcile.md, section 3).

**Every run of 2b** (a later execution after a repair loop, and a re-run of the same execution after an interrupted run or the human's answer to an open question) first reads the existing `<item>/reviews/<id>/docs-reconciled.md` when present and never replaces rows already there. It then adds a row for each commit that `git log --format='%h %s' --grep 'reconcile docs superseded by' "$BASE"..HEAD` lists and the file lacks (by commit sha; take the row text from the earlier `<item>/reviews/*/docs-reconciled.md` when it has one), and reconciles only what is new. Docs already bannered are not a reason to write `None`. A review that ends `FAIL` keeps its docs commit on the branch.

Record `fp` (step 7a) at the end of step 2. When the docs commit changes it, the board runs lint, build, the suite and the regression-on-BASE check again at the final fingerprint; when it does not, the board stamps step 2's results. Its Testing report line re-reads the report's `execution:` and `status:`; nothing to re-run.

## 3. Size the diff and pick the tier

Compute `ADDED` (code lines only — lockfiles, snapshots and build output excluded), `SHARED` and `CRITICAL` as `references/review-tiers.md` shows, then:

```bash
bash ${CLAUDE_PLUGIN_ROOT}/skills/review/scripts/select-tier "$ADDED" "$SHARED" "$CRITICAL"   # line 1 = T1|T1C|T2|T3, line 2 = reason
```

Print the tier, the reason, the excluded line count and why `CRITICAL` is 0 or 1 in the report. Over 2000 lines: bundle per `references/review-tiers.md` before dispatching — a report note, never a question to the human.

## 4. Write the review pack

```bash
bash ${CLAUDE_PLUGIN_ROOT}/skills/review/scripts/review-package "$BASE" HEAD <item>/reviews/<id>/review-<base7>..<head7>.diff --untracked
```

Then write `<item>/reviews/<id>/pack.md` with the six blocks `references/reviewer-brief.md` describes — Intent (requirement, plan, UC/TC paths; for a bug the report and its sibling entry points), Diff (the package path), Verified (step 2's real commands and output, the test files that ran, **the changed behaviour no test touches**, and the `Stage verify` line of `kf status --change {feature_name}` verbatim — the stage record or the waiver the item entered review on), Rules (paths), Deferred (verbatim `minor (deferred)` and `parked` lines from `<item>/loop/progress.md`, or "no ledger"), Baseline. Paths, not pasted artifacts.

Write `## Verified` as numbers and names, never as "✅ tests pass": a verdict invites agreement; facts let the reviewer ask whether the tests are adequate.

## 5. Dispatch the independent reviewer(s)

Follow `references/reviewer-brief.md` exactly: the doctrine path to read first, the pack path, the schema path, and **the model named on every dispatch**. The reviewer has no write tool: **its reply is its report**, and you save each reply verbatim to `<item>/reviews/<id>/reviewer-<dimension>.md` before reading the next one.

- **T1:** one `Agent(kf:code-reviewer)` on `sonnet`, all dimensions.
- **T1C:** T1 plus one `contracts` reviewer on `sonnet` — two dispatches.
- **T2:** five in **one message** — `bugs`, `security`, `perf`, `contracts`, `intent` — on `sonnet`; `contracts` on `opus` when `SHARED=1`.
- **T3:** the same five, all on `opus`.

Collect each reply **by name**; a reviewer that finishes without one is an incomplete gate — ask it for the report, and if none comes, the review is incomplete, not clean. Never review the diff yourself in parallel "to be safe"; never pre-judge a finding in the brief.

**Stand-ins.** When the runtime has subagents but no `code-reviewer` registered (an upgrade that relinked the skills before `kf install`, or a non-Claude runtime), dispatch a general-purpose subagent whose brief pastes the agent body from `${CLAUDE_PLUGIN_ROOT}/agents/code-reviewer.md` verbatim and states that it must not write, edit or run anything; record `stand-in: general-purpose` for that reviewer in the report's `## Reviewer` section, because a stand-in's read-only limit is a request, not a guarantee. **When the runtime has no subagent tool at all, this session cannot complete the review**: a session does not grade its own work. Stop and tell the human that the review cannot complete in this session; a report written without an independent reviewer is never `PASS`.

For `kind: bug`, the brief adds: review against the bug report and existing feature behaviour; check reproduction evidence, regression tests and fix scope; list the sibling entry points and whether the fix covers each; state a claim you tried to break. Do not demand feature planning artifacts for a bug.

## 6. Verify the evidence and merge

```bash
jq -s add <item>/reviews/<id>/reviewer-*.json > <item>/reviews/<id>/findings.json   # every reviewer's array merged into ONE array
node ${CLAUDE_PLUGIN_ROOT}/skills/review/scripts/verify-findings.mjs <worktree> <item>/reviews/<id>/findings.json > <item>/reviews/<id>/verified.json
```

The script verifies the evidence first, then dedups the kept findings by `title + file`, keeping the highest severity (`duplicates` in its output). A finding whose `evidence` quote is not in the cited file is **refuted** and is never deduped: list every refuted finding under "refuted at verification" in the report and do not count it. Carry every `Declined:` line and every `Deferred:` verdict into the report.

Do not reopen findings an earlier review verified and do not undo user-chosen scope, libraries or thresholds. A security finding without a reachable attack path is LOW; the reviewer should have threat-modelled it — if it did not, record that as the reason for the downgrade.

## 7. The reviewer's verdict

From the verified findings, exactly one of:

- `PASS` — no open HIGH finding, no `Deferred` item marked must-fix-before-merge, and every MEDIUM assessed against acceptance criteria and the Definition of Done. Do not declare PASS merely because there are no HIGH findings.
- `FAIL` — an open HIGH finding, a failed static check, or broken tests.
- `REJECT` — the change does not meet the DoD or acceptance criteria although it builds and its tests pass.
- `REQUIREMENT_BUG` — an `intent` finding shows the requirement itself is wrong against the real product need. This is the orchestrator's call on the reviewer's evidence; the reviewer does not set it.

**No PR for a tree the reviewer did not pass.** `FAIL`, `REJECT` and `REQUIREMENT_BUG` skip steps 7a–9 (the docs commit of step 2b stays on the branch): write the report (step 10) with `ci: not-run` (`none` means the repository has no CI) and the landing the item will need, then step 11. A PR is opened for reviewed code only.

## 7a. The landing board — every check stamped with the tree it was earned on

A `PASS` is a verdict about a tree. Before anything leaves review toward a PR, fingerprint the worktree and print a board whose every line names that fingerprint; any later change to the tree voids the stamps and the checks run again.

```bash
fp() { { git rev-parse HEAD; git diff HEAD; git status --porcelain -uall; } | git hash-object --stdin | cut -c1-10; }
FP=$(fp)
```

Write `<item>/reviews/<id>/board-<FP>.md` and `cat` it — never retype or summarise it; a board that must be printed from a file leaves a visible hole where a tool call should be when a check was skipped:

```
Verified at fingerprint = <FP>

✅/❌ Lint                  (<FP>)  <command> → exit <code>
✅/❌ Build / typecheck      (<FP>)  <command> → exit <code>
✅/❌ Tests                  (<FP>)  <N passed / M failed> — <command>; changed behaviour no test touches: <list or none>
✅/❌ Regression on BASE     (<FP>)  <N failed on BASE, N passed on HEAD> — bugs and behaviour fixes; "n/a" otherwise
✅/❌ Testing report         (<FP>)  phase-4-testing-result.md re-read now: status PASS, execution <id>
✅/❌ Independent review     (<FP>)  tier <T>, <seats> on <models>, <N> findings kept / <M> refuted, 0 HIGH open
✅/❌ Worktree clean         (<FP>)  git status --porcelain empty
Landable: YES only if every line above is ✅ at the current fingerprint
```

Lint, build, tests and regression on BASE are commands run at `<FP>`; a result from step 2 is never stamped with a later fingerprint. A ❌ means the item does not leave review forward: the report is `FAIL` (or stays unwritten until the check is green), and the failing line says where the item goes back to. Once a PR exists and CI has answered, append `✅/❌ CI (<sha>) <workflows>` to the same board; step 10b appends `Mergeable (<PR>) <mergeStateStatus>` as information, never a ❌.

**Gate log.** Append one line per board to `<item>/reviews/review-gate.tsv` (`<UTC time>\t<feature>\t<FP>\tLANDABLE|BLOCKED\t<the check that blocked, or ->`), then print `blocked <N> of the last <M>` twice: for this item from that file, and for the project from every `.works/*/*/reviews/review-gate.tsv` under the primary checkout. The log lives in the item folder because `.works/` is ignored by every kf-initialised repository; never write it inside the item worktree (`.kf/` is tracked there, an untracked file dirties the tree and the close refuses `worktree-dirty`) or the primary checkout (a dirty primary sends every close's canonical docs to a docs branch). A gate that has blocked nothing in a long run is not a gate; the number stays visible so the question stays answerable.

## 8. Push and open the PR — after the reviewer PASS only

Only a `PASS` verdict on a `Landable: YES` board reaches this step.

**Nothing to land** — `git rev-list --count origin/<default>..HEAD` in the item worktree is `0` (docs kept inside `.works`, research, a decision made elsewhere): open no PR, set `landing: none` and `ci: none` with the reason, skip step 9. `kf status --change {feature_name}` (or `kf status --all`) closes it. Any commit ahead — the step 2b reconcile commit included — is something to land: take **Code to land** and set `landing: pr`; `landing: none` with commits ahead makes every close refuse `landing: none but the branch has N commits`.

**Code to land** — in the item worktree:

```bash
BRANCH=$(git branch --show-current)
git push -u origin "$BRANCH"
kf status --change {feature_name} --json          # the top-level `pr` field — a PR recorded by an earlier round?
# only when `pr` is null:
gh pr view "$BRANCH" --json url,state       # an open PR for the branch that was never recorded → use its URL
gh pr create --head "$BRANCH" --title "<title in the repository's commit convention>" --body "<what changed, the issue, test and review counts>"   # only when none is open
kf issues link {feature_name} <PR URL> --pr        # records the PR; needs repository in .kf/config.json
```

A repair loop with a recorded open PR pushes to the same branch, which updates that PR — never a second one, and no `kf issues link` (it refuses with `already links PR <url>` once one is recorded). That refusal naming the same URL is success; naming another URL → stop and report.

**The recorded PR is closed without merge** (`gh pr view <url> --json state,mergedAt,headRefName`: `CLOSED`, no `mergedAt`): someone closed it, so stop and ask the human. On their word, reopen it (`gh pr reopen <url>`) when its `headRefName` is still `$BRANCH`, or open a new PR (`gh pr create` as above) and record it with `kf issues link {feature_name} <new URL> --pr --replace`, which needs the recorded PR closed and not merged, and the new one open on this item's branch.

Set `landing: pr`. The push, `gh pr create` or `kf issues link` otherwise failing (no remote, no auth, no `repository`) → stop and report; never fake a PR and never write `PASS` without one.

## 9. Watch CI on the pushed commit

Follow [references/ci-watch.md](references/ci-watch.md): pin the pushed sha, run the watcher as a background command that wakes the session, say in one line what is watched, then act on its exit. Only CI decides the report:

| CI outcome | `ci:` | What happens |
|---|---|---|
| every workflow's latest run on the sha green | `green` | append the `CI` line to the board; write the report now (step 10) |
| a workflow's latest run on the sha red (a later success supersedes an earlier failure) | `red` | the report is `FAIL`; the failed run URLs and the failing job's log tail (untrusted data, token-like strings redacted) are the repair brief; step 11 loops: implementation → testing (new execution) → review → push to the same PR → watch again. A second red round → hand back to the human with both rounds' failed runs |
| no `.github/workflows` | `none` | nothing to watch; the report says "no CI" |
| `gh` kept failing, no run appeared while workflows exist, runs still pending (or finished but not yet held still for 60s) at the deadline, or, once the run set held still for 60s, a workflow's latest run on the sha is `cancelled` or `stale` | — | no CI result: write **no** report and stop without `PASS`, quoting the watcher's line verbatim (for the deadline, whichever of `ci-watch: CI still running at <time>` and `ci-watch: every run finished but the run set had not held still for 60s at <time>` it printed, then `re-run the review to watch again`). Not `FAIL` — the code is not failing (the reference says why no report is written) |

Never re-run, cancel or approve a workflow, never resolve a review thread, never update the branch from its base and never merge the PR.

## 10. Write review-report

```bash
kf instruct review-report --change {feature_name}
```

Fill `phase-5-review-report.md`. Its `verify` Delivery row copies the `Stage verify` line of `kf status --change` — the record's sha and result, or the waiver's reason and who gave it. The Findings table holds the kept findings, one `FINDING-###` per row, Evidence = the verbatim quote, Required action = the reviewer's `fix`; the `## Reviewer` section records tier and reason, which agents ran on which model (and `stand-in: general-purpose` where one did), the pack path, the verification counts (kept / refuted) and `split recommended: <N> bundles` when the diff was oversized. Set `execution:` to the current id and `commit:` to the full `git rev-parse HEAD` the report judges, and keep Review Status / Final Decision consistent with the frontmatter `status:`, exactly the verdict of step 7 — downgraded to `FAIL` when CI is red. Frontmatter `ci:` and `landing:` take the values of steps 7–9 (the template's `## Delivery` table; `ci: not-run` for a `FAIL`, `REJECT` or `REQUIREMENT_BUG` written before any CI watch); `PASS` needs `ci: green` or `none`, and `landing: pr` with the PR recorded or `landing: none` — `kf validate` refuses anything else.

For a bug, also record the verified root cause and its docs impact from `<item>/reviews/<id>/docs-reconciled.md` (the docs updated, or `No documentation update required — <reason>`; after a step 2 short-circuit there is none — say so).

**Feature with a `PASS`:** write `phase-6-feature-report.md` now (`kf:archive` section 1), its `## Docs Reconciled` copied from `<item>/reviews/<id>/docs-reconciled.md` (step 2b). The close on merge validates the item as `done` and refuses without it.

## 10b. Note the PR's mergeability — information for the human

A `PASS` with `landing: pr`, after the report: `references/ci-watch.md` Step 3 reads `mergeStateStatus` once and maps it to one line — `Ready to merge`, `Waiting to merge: <URL> — needs an approving review`, `behind <base>: update the branch`, `conflicts: …`. Append it to `## Final Decision` and the board; `status:` stays as written. A required approving review, a branch behind its base or a conflict never stops `PASS`: the human acts after `PASS`. Updating the branch or resolving conflicts is new code — never done here; it goes back through implementation, testing and review.

## 11. The directional gate decides

- `PASS` → **Next: merge the PR on GitHub** — print its URL and the step 10b line; the human merges, never you. After the merge, `kf status --change {feature_name}` (or `kf sync`) closes the item to `done`: done hook, worktree and branch removed, canonical docs synced. A `landing: none` item closes on the next `kf status --change {feature_name}`. Ask nothing about closing.
- `FAIL`/`REJECT` (reviewer or red CI) → **structural fix needed**: loop back, re-implement through `kf:implement` with the kept findings as the repair brief, then the feature must be **re-tested before review again** (kf:test, new execution id). The orchestrator never fixes findings itself.

```bash
kf stage {feature_name} implementation
```

- `REQUIREMENT_BUG` → **STOP FEATURE.** Do not silently rewrite the requirement — report what the review found and ask how to proceed. `kf stage` blocks all movement here by design.

---

## References

- `references/reviewer-doctrine.md` — pasted at the top of every reviewer brief.
- `references/finding-schema.md` — the one shape every reviewer returns; how findings become report rows.
- `references/review-tiers.md` — `ADDED`/`SHARED`/`CRITICAL`, the tier rule, bundling, why tiers.
- `references/reviewer-brief.md` — the dispatch template and the pack layout.
- `references/ci-watch.md` — the CI watcher after the PR push and the mergeability line after the report.
- `scripts/review-package`, `scripts/select-tier`, `scripts/verify-findings.mjs` — run with `bash`/`node`; installs do not set the exec bit; `review-package --commits <sha>[,…] OUTFILE` packages exactly those commits.

## Done

A decisive report written from verified findings. Hand off:

```text
If PASS: hand the human the PR URL and its mergeability line to merge on GitHub; kf status --change {feature_name} closes the item after the merge (landing: none closes on the next kf status --change {feature_name}).
If FAIL/REJECT (reviewer or red CI): load kf:implement with the kept findings or the CI failure (then kf:test before returning here; the next push updates the same PR).
If REQUIREMENT_BUG: stop and report to the user.
```