review

Files (20)

• LIMITATIONS.md
• SKILL.md
• cleanup.md · references/
• fresh.md · references/
• peer.md · references/
• prove-it.md · references/
• report-template.md · references/
• research-verify.md · references/
• single-lane.md · references/
• consume-peer-verdict.sh · scripts/
• emit-attestation.sh · scripts/
• post-peer-review.sh · scripts/
• resolve-pr-worktree.sh · scripts/
• sibling-roots.py · scripts/
• validate-prove-it.sh · scripts/
• test-blast-radius.sh · tests/
• test-carry-forward.sh · tests/
• test-consume-peer-verdict.sh · tests/
• test-lane-verdicts.sh · tests/
• test-review-router.sh · tests/

SKILL.md (first lines)

<!-- style-exception: this file's producer bullets under "The attestation surface" are established dense single-line paragraphs joining several clauses with semicolons and dashes, a convention used throughout the file. Rewriting that file-wide convention is out of scope for, which only touches one such bullet's body to name the new PostToolUse hook. -->

# Review

**One command on a diff.** `/review` routes to the right reviewer set for the diff in front of you.

| Mode | What runs | Shared object |
|------|-----------|---------------|
| default (bare, or a level token) | the fno review lane: inline angles, dedup, three-state verify, one attestation; `--fresh` runs the same lane once in a fresh context ([fresh.md](references/fresh.md)) | the diff |
| `prove-it` | runtime evidence at the changed code's real surface, with a validator that refuses an unprobed PASS | the running change |
| `cleanup` | the apply-or-skip pass: four cleanup angles once, no gate weight, no attestation | the diff |
| `peer` | a cross-model second opinion, optionally producing a verdict-gated local attestation | the diff |
| `research` | advisory research-verify panel (fact-checker / citation-auditor / contradiction-finder / completeness-critic) | a `doc` deliverable (brief + sources sidecar) |
| `sigma` | RETIRED. The token refuses and names the default lane as the replacement. | - |

This is a **router**, not a monolith. It parses the first argument as a mode, announces the resolved mode, then loads that mode's reference and follows it in this same context. It never calls another skill at runtime (it dispatches review subagents via the Task/Agent tool and loads modes via Read).

## Review-cap gate

The round budget is `review.max_rounds` (default 2), counted across the whole life of the PR. One reviewed head is one round, pass or fail. Read it with `fno do pr status <n>`. When it reads `rounds_exhausted: true`, the review phase is complete. Request no review and emit no attestation. Decline what you do not fix with a recorded reason, and merge on green CI. Open findings stay in the PR conversation. When it reads null, the gate has no row at this head yet: run `fno-agents review-coverage --cwd . --pr <n>` and read again.

A moved head does not by itself owe a round. A rebase whose code delta is unchanged carries the attestation. A fix commit, or a rebase that resolved conflicts, can leave coverage uncovered. Then the stop gate or `fno do pr status` names the owed round, inside the budget. Never copy an earlier verdict onto a new head. An attestation certifies a review that ran, and each new head it lands on spends a round.

One path checks the budget for you. On Claude, the Skill-tool hook (`hooks/review-hold.sh`) denies a review whose rounds are spent. `fno do target request-self-review` sends its request whatever the budget reads. A review that arrives with no Skill-tool call meets no hook. So read the budget before you request or run a review. Two shapes pass the hook at the cap, both the attestation law's own. `--verify-fixes` in the invocation flags declares a scoped fix-verification of named findings, which is not a round. The emitter stamps the verified round onto the pass as `review_round`, so the counter reads that round. A rebase delta at or over `review.carry_interdiff_lines` (default 100) takes one optional review.

## Active skill freshness preflight

Before resolving any argument, run the diagnostic against the active review instructions:

```bash
if FRESHNESS_OUTPUT=$(fno doctor plugin-file "$SKILL_DIR/SKILL.md"); then
  FRESHNESS_EXIT=0
else
  FRESHNESS_EXIT=$?
fi
printf '%s\n' "$FRESHNESS_OUTPUT"
case "$FRESHNESS_OUTPUT" in
  *PLUGIN_FILE_STALE*)
    exit "$FRESHNESS_EXIT"
    ;;
  *PLUGIN_FILE_UNKNOWN*)
    echo "review warning: active skill freshness is unknown; continuing"
    ;;
  *PLUGIN_FILE_FRESH*)
    ;;
  *)
    if [ "$FRESHNESS_EXIT" -ne 0 ]; then
      echo "review warning: freshness diagnostic unavailable (exit $FRESHNESS_EXIT); continuing"
    else
      echo "review refused: active skill freshness diagnostic returned no recognized marker"
      exit 4
    fi
    ;;
esac
```

## Step 1: Resolve the mode (ALWAYS announce it)

The grammar is `[level] [--comment] [--fix] [--fresh] [<pr#>|<branch>|<path>]`, with `prove-it`, `cleanup`, `peer`, `research`, and `declare` as leading mode tokens. A flag is a token in any accepted spelling of a known flag name. Accept `--comment`, bare `comment`, or the em-dash `—comment` (what a phone autocorrects the double hyphen into). The same three spellings hold for `fix` and for `fresh`. The accepted set is the `KNOWN_REVIEW_FLAGS` table in `crates/fno/src/review_invocation_core.rs`, the one owner of the review-invocation vocabulary (the vendored twin `crates/fno-agents/src/review_invocation_owner.rs` pins byte equality). That one table feeds both this router and the invocation telemetry. A spelling accepted here is also recorded there, and no fourth spelling is invented. Strip the flags BEFORE reading the first remaining token. The order is load-bearing. Stripping after the target test hands the flag tokens to the target slot. There they resolve to nothing, and the run dies having posted nothing. `--comment` never drops findings without a PR. They hold on the branch and HEAD (the `review_attestation` row), and the create flow's publish-review step posts them once the PR opens. `--fresh` is a default-lane modifier only. A `--fresh` token that survives the strip while the resolved mode is `prove-it`, `cleanup`, `peer`, `research`, or `declare` refuses loudly. Print `refused: --fresh modifies only the default review lane` and stop with a non-zero result. Then read the first remaining token:

- **no argument** -> mode is the default lane at a level sized from the diff. Print exactly: `running fno review lane (default, level from diff)` and continue to Step 2. With `--fresh`, print `running fno review lane (default, level from diff, fresh)` instead and continue to Step 2f.
- **a level token** (`low` `medium` `high` `xhigh` `max`) -> mode is the default lane at that explicit level. Print `running fno review lane (level <token>)` (`running fno review lane (level <token>, fresh)` with `--fresh`). Any remaining tokens are the target. Continue to Step 2, or Step 2f with `--fresh`.
- **`ultra`** -> REFUSE. Print exactly `refused: ultra is billed separately and no fno surface issues it; use max` and stop with a non-zero result. Run nothing, emit nothing.
- **`sigma`** -> REFUSE, naming the replacement. Print exactly `refused: sigma is retired; the default review lane replaced it - run /fno:review [level] [<target>] (or /fno:review peer for a cross-model read)` and stop with a non-zero result. Run nothing, dispatch nothing.
- **`prove-it`** -> mode is `prove-it`, runtime evidence. Print `running prove-it (runtime evidence)`. The remaining tokens, if any, are the target. Continue to Step 2p.
- **`cleanup`** -> mode is `cleanup`, the apply-or-skip pass. Print `running cleanup (apply-or-skip)`. The remaining tokens, if any, are the target. Continue to Step 2c.
- **`peer`** -> mode is `peer`. Print `running peer review (cross-model)`. The remaining tokens are peer's own arguments (`[PR#|branch] [codex|gemini]`). Continue to Step 3.
- **`research`** -> mode is `research`. Print `running research-verify (advisory)`. The remaining tokens, if any, are the brief path. Continue to Step 4.
- **`declare`** -> mode is `declare`. Print `emitting self-cert attestation (declare)`. Continue to Step 5.
- **any other non-empty token** -> one test decides between target and mode. If the token is all digits, it is a PR number - a target, not a mode. Else if `git rev-parse --verify --quiet <token>` succeeds, it is a branch target. Else if it contains a `/`, `.`, or ends in a recognized file suffix, treat it as a path target. Anything else is an unknown mode. Do NOT default, do NOT guess. Print:

  ```
  unknown review mode: '<token>'
  valid modes: prove-it, cleanup, peer, research, declare (bare = the fno review lane; or lead with a level: low medium high xhigh max)
  ```

  and stop with a non-zero result (emit no review, dispatch no agents). This is the locked router contract: an unknown non-empty mode never silently falls through to a default. A number, a resolvable ref, or a path-like token is a TARGET, never a mode.

- **a target-slot token that resolves to nothing but matches a known flag name** -> REFUSE loudly. If the token survives the flag strip and then resolves to no PR, no branch, and no path, print exactly `refused: '<token>' looks like a misspelled flag; accepted spellings are --comment, bare comment, or em-dash —comment (same three for fix)` and stop with a non-zero result. A flag that reached the target slot unstripped must never be silently absorbed as a target; the silent absorption is what made the misspelling invisible.

> A level is never inherited from a previous invocation: an explicit token records `explicit`, a bare invocation sizes from the diff, and no run reuses a typed level (that upstream behavior is a hazard, not a feature).

> When any target token survives this step, export it as `REVIEW_TARGET` for the empty-diff guard below. A PR number resolves to its head ref here (`gh pr view <n> --json headRefName -q .headRefName`), so a PR named from a worktree that is not the PR's checkout is still inspected at its own head. A bare invocation leaves `REVIEW_TARGET` empty.

> For a numeric PR target, resolve the local checkout with `bash "${SKILL_DIR}/scripts/resolve-pr-worktree.sh" "$REVIEW_TARGET"` and enter the returned path before Phase 0. If the PR branch has no listed worktree, stop with that refusal; reading source files from the caller's checkout would review a different tree. This also makes the later attestation measure the same branch and HEAD.

## Step 2: the default mode (the fno review lane)

### 2a. Empty-diff guard (before anything runs)

If there is nothing to review, report it and exit cleanly - never review an empty diff. The guard tests the TARGET Step 1 resolved, never the caller's tree by default: a named PR's diff is `$BASE..<pr head>`, a named branch's diff is `$BASE..<branch>`, a named path's diff is that path, and only a bare invocation (no target token) reads the caller's HEAD. The guard can never certify the caller's diff as a substitute for a target that resolved to something else:

```bash
BASE="${BASE:-origin/main}"
git fetch -q origin 2>/dev/null || true
# Only fire the guard when we are CONFIDENT the diff is empty. For a bare
# invocation: no staged or unstaged changes AND a resolvable base shows zero
# commits ahead of HEAD. If BASE does not resolve (no origin remote, non-main
# default branch), do NOT short-circuit to "empty" - fall through to the
# lane's Phase 0, which resolves the diff itself and reports emptiness from
# there. This avoids silently skipping a review of committed work when the
# base ref is just unknown here.
if [ -z "$REVIEW_TARGET" ]; then
  if git diff --quiet 2>/dev/null && git diff --cached --quiet 2>/dev/null \
     && git rev-parse --verify --quiet "$BASE" >/dev/null 2>&1 \
     && [ -z "$(git log "$BASE"..HEAD --oneline 2>/dev/null)" ]; then
    echo "no changes to review"
    exit 0
  fi
else
  # A target was named (Step 1 exported it as REVIEW_TARGET: a PR number,
  # a branch, or a path). Empty means THAT target's diff is empty - e.g. a
  # PR whose branch carries no commits ahead of its base - never the
  # caller's tree. An UNRESOLVABLE base or target never reads as empty:
  # the rev-parse conjunct fails, the guard stays silent, and the lane's
  # Phase 0 resolves the diff and reports the real failure there.
  if git rev-parse --verify --quiet "$BASE" >/dev/null 2>&1 \
     && git rev-parse --verify --quiet "$REVIEW_TARGET" >/dev/null 2>&1 \
     && [ -z "$(git log "$BASE".."$REVIEW_TARGET" --oneline 2>/dev/null)" ]; then

From bllshttng/fno, Apache-2.0.

More from bllshttng/fno