pi-review

Files (2)

• SKILL.md
• findings-ledger.schema.json · assets/

SKILL.md (first lines)

# Pi Review

Review the task-relevant diff without editing it. The parent defines scope, launches independent reviewers, verifies evidence, owns the findings ledger, and decides the gate. Do not shell out to Codex, Claude Code, or another agent CLI.

## Usage

```text
/skill:pi-review                    # current task-relevant diff
/skill:pi-review <baseline>         # baseline to HEAD
/skill:pi-review <files-or-scope>   # explicit review scope
```

A standalone review leaves project files unchanged. When review is part of an authorized implementation workflow, a later single writer may apply parent-approved fixes.

## Preflight and scope

1. Require `pi-subagents >=0.46.0`; if the installed version cannot be verified, report the unverified preflight rather than assuming retained-resume behavior.
2. Run `subagent({ action: "list" })`; use only executable, non-disabled reviewers.
3. Derive tracked and untracked scoped files from user input and Git state. Never review the whole repository when the task owns a narrower diff.
4. Establish the target baseline and compare every candidate finding against it.
5. Read repository instructions and matching path-scoped rules. If no applicable standards resolve, record that fact rather than fabricating a path.
6. Inspect callers of changed public symbols with the repository's structural index when available; otherwise use bounded read-only search.
7. Record the validation contract and acceptance criteria the change claims to satisfy.
8. Materialize a read-only review packet outside the repository containing the scoped patch, baseline excerpts needed for comparison, changed/untracked file list, validation contract, matching standards, and the full finding contract below. Verify the packet contains no secrets or unrelated controlled data.

Preserve unrelated changes. Reviewer children are read-only: no project/source edits, commits, pushes, comments, merges, releases, or publication.

## Review angles

Use one reviewer for a small, low-risk diff. For broad, risky, security-sensitive, or user-visible changes, launch fresh-context reviewers in parallel with distinct prompts. Typical angles:

- correctness, regressions, concurrency, and failure behavior;
- tests, mutation strength, and validation gaps;
- security, privacy, data handling, and least privilege;
- performance, resource bounds, and availability;
- API, migration, documentation, or user-flow contracts;
- simplicity and maintainability when complexity itself creates risk.

Every reviewer reads the same verified review packet and then inspects scoped project files directly. Pi's builtin reviewer has read/search tools but no shell, so a bare Git ref is not sufficient evidence. The parent runs Git commands and safe probes, materializes their output, and remains responsible for validating baseline and runtime claims. Do not provide another reviewer's verdict or repair advice during discovery.

Prefer a reviewer from a different model family or provider than the implementer when an allowed capable model is available. Cross-family review is a quality option, not a reason to bypass model scope, data policy, availability, or cost constraints. Inspect the live agent/model mapping before overriding defaults; never hardcode a model that may not exist. Record the implementer and reviewer families. If policy requires cross-family independence and none is available, block or obtain an explicit waiver; otherwise record the same-family limitation as residual risk.

## Launch

Before launch, write the shared packet to a readable absolute path such as `<scratch>/pi-review-packet.md`. The packet carries the complete finding contract, so every fresh reviewer receives the same schema without inheriting this skill.

```javascript
subagent({
  workflowScript: `return runs.all([
    {
      key: "correctness",
      agent: "reviewer",
      task: "Read <packet-path>, then review its scoped change for correctness and regressions. Inspect named project files directly. Do not edit files. Return only findings matching the packet contract."
    },
    {
      key: "tests",
      agent: "reviewer",
      task: "Read <packet-path>, then review its scoped tests for vacuity, mutation strength, and validation gaps. Inspect named project files directly. Do not edit files. Return only findings matching the packet contract."
    },
    {
      key: "security",
      agent: "reviewer",
      task: "Read <packet-path>, then review its scoped change for security, privacy, data handling, least privilege, and resource bounds. Inspect named project files directly. Do not edit files. Return only findings matching the packet contract."
    }
  ])`,
  context: "fresh",
  async: true
})
```

Resolve `<packet-path>` before launch. Use fewer lanes when angles would duplicate one another. Keep run IDs from the returned results so the same reviewers can verify their findings later. Retained resume is for reviewers reading a stable integration checkout; do not promise resume into a managed writer worktree that Pi may already have cleaned up. Repairs use a new worker run or managed worktree. Use durable artifacts for large reports; otherwise inline output is sufficient.

## Finding contract

Require findings in severity order. Each finding includes:

- stable fingerprint;
- severity: Critical, High, Medium, or Low;
- `file:line` or the narrowest available location;
- evidence class: `EMPIRICALLY_VERIFIED`, `PROBED`, or `HYPOTHETICAL`;
- scope status: `REGRESSION`, `NEWLY_REACHABLE`, `PRE_EXISTING`, or `HYPOTHETICAL`;
- baseline comparison;
- violated invariant;
- failing execution path or trigger;
- concise evidence;
- repair direction, not an executor brief;
- named proof test or check.

Reject style-only preferences without a repository rule or concrete maintenance risk. Group test gaps beneath their root defect instead of multiplying findings.

### Evidence and scope

- `EMPIRICALLY_VERIFIED`: the parent reproduced the failure or observed the changed execution path fail with direct runtime evidence.
- `PROBED`: a bounded parent-run experiment or decisive source/baseline comparison demonstrates reachability, but not a complete production reproduction.
- `HYPOTHETICAL`: plausible from inspection, but reachability or impact remains unproven.
- `REGRESSION`: the diff introduces the defect.
- `NEWLY_REACHABLE`: the diff materially expands reachability or impact.
- `PRE_EXISTING`: equivalent behavior and impact exist at baseline.
- `HYPOTHETICAL`: the path is not demonstrated.

Keep pre-existing findings visible as residual risk, but do not block on them unless the change worsens them, the task owns them, or project policy requires closure. Never label a changed authorization boundary, capability, data flow, or reachability as pre-existing merely because adjacent code already existed.

Probe a hypothetical Critical/High once when a bounded safe probe can determine reachability. If it remains hypothetical, record nonblocking residual risk rather than inflating certainty.

## Test-vacuity audit

Review every new or changed test for whether it can fail when the named behavior breaks. Check especially for:

- async work cancelled before its first execution point;
- events or futures set but never awaited;
- assertions on state assigned directly by the test;
- defaults that mask missing or renamed symbols;
- mocks that bypass the changed seam;
- mutation patches that match no unique source location.

Name the exact test and broken behavior. When practical, the parent independently applies the smallest reversible mutation and confirms the test fails.

## Parent synthesis

From tcbuilds/nautilus, MIT.

More from tcbuilds/nautilus