review-pack

review-pack

Overview

A structured self-review and user sign-off. The skill:

1. Re-reads the spec and the plan.
2. Reads the full diff from the isolation base to HEAD on the vibe branch.
3. Critiques its own work against the spec (not against abstract best practice — against this spec).
4. Presents a severity-ranked list of findings + the full diff + a sign-off prompt.
5. Waits for user approval before handing off.

This is the last reasoning gate before the pipeline touches anything outside the isolated workspace.

When to invoke

• After verify-gate returns verdict ready.
• Before finish-branch, before any merge, PR, push, tag, or deploy.
• When the user explicitly asks for a review of completed work.

Do not invoke:

• If verify-gate returned not ready. Review-pack is not a bypass.
• If no commits exist on the vibe branch beyond the base — there is nothing to review.

Prerequisites

• Spec exists at the path recorded by the vibe run, status: approved in frontmatter.
• Plan exists at the path recorded by the vibe run.
• Verify-gate report exists with verdict: ready.
• Vibe branch has at least one commit beyond the isolation base.

If any prerequisite fails, stop and surface what is missing.

Self-review procedure (Reflexion)

Run through these critique passes in order. Each pass produces findings with a severity tag.

Pass 1 — spec coverage

For each requirement in the spec's Goals and Constraints sections:

• Is there a commit, test, or code hunk that implements it?
• Is the implementation consistent with the spec's stated Approach?
• Are the spec's Non-goals respected (no new code touching forbidden areas)?

Finding severity:

• block — a Goal is not satisfied, a Non-goal has been violated, a Constraint has been broken.
• warn — a Goal is satisfied weakly (e.g. one test where more were reasonable).
• nit — style, naming, or documentation detail.

Pass 2 — plan fidelity

For each task in the plan:

• Are its "Files" matched by the diff?
• Is the commit message on its commit the one the plan specified?
• Are the tasks' order reflected in commit order?

Finding severity:

• block — a plan task's code is missing from the diff.
• warn — commit messages diverge from the plan's wording.
• nit — extraneous whitespace or non-functional edits introduced.

Pass 3 — code quality (within the spec's scope)

• DRY: are there obvious duplications of logic within the changed files?
• YAGNI: are there new code paths or exports with no caller, no test, and no spec requirement?
• Test adequacy: does each new behavior have at least one assertion that would fail if the code were broken?
• Naming: do new public identifiers match the spec's wording?

Finding severity:

• block — a public API diverges from the spec (name, signature, semantics).
• warn — clear duplication or speculative feature; ambiguous tests.
• nit — local naming nits, minor comments.

Pass 4 — simplicity (Karpathy principle 2)

Run the senior-engineer test on the diff:

• Total LOC added (count from git diff --shortstat).
• Identify the single largest new construct (function / class / module / file).
• Ask: "Could a senior engineer halve this without losing required behavior?"
• If yes, quote the bloat candidate verbatim (file:line range + the code) and name what could be cut.

Finding severity:

• block — a new abstraction has a single caller and could be inlined; speculative configurability with no spec requirement; new code paths/exports with no caller and no test.
• warn — code that is twice the natural length but functionally correct.
• nit — micro-bloat (extra wrapper, unused parameter).

Tag each over-engineering finding so the cut is unambiguous:

• delete: dead code, unused flexibility, speculative feature — replacement: nothing.
• stdlib: hand-rolled thing the standard library ships — name the function.
• native: dependency or code doing what the platform already does — name the feature.
• yagni: abstraction with one implementation, config nobody sets, layer with one caller.
• shrink: same logic, fewer lines — show the shorter form.

End this pass with the metric that matters: net: -<N> lines possible. If there is nothing to cut, write Lean already. This pass hunts complexity only; correctness, security, and performance belong to the other passes, not here.

A block here halts the handoff just like a spec violation.

Pass 5 — surgical-diff (Karpathy principle 3)

For every changed file in the diff:

• Does it appear in at least one plan task's "Files" section?
• Does every changed hunk trace to a specific plan task or spec requirement?

Finding severity:

• block — a file was modified that no plan task names; or a hunk has no traceable origin.
• warn — pre-existing dead code was deleted without explicit user request; formatting or comment changes unrelated to the task.
• nit — touched lines that did not need to change to satisfy the task.

Quote the orphan file:line ranges verbatim.

Pass 6 — self-critique (Reflexion)

Ask explicitly: "What are three ways this implementation could be wrong that the tests would not catch?" Write down the three answers. For each:

• Is there evidence in the diff that the risk is mitigated?
• If not, what would an additional test or assertion look like?

If any of the three risks has no mitigation evidence, raise a warn with a suggested follow-up.

Output structure

Write a review document at docs/reviews/YYYY-MM-DD-<feature>-review.md containing:

# Review — <feature>

**Date:** YYYY-MM-DD
**Spec:** <path>
**Plan:** <path>
**Verify report:** <path>
**Commits under review:** <base sha>..<head sha> on <branch>

## Diff summary

- Files changed: <count>
- Lines added: <n>, removed: <n>
- Commits: <count>

## Findings

### Block
- <one per finding, with evidence: file:line or commit sha>

### Warn
- <...>

### Nit
- <...>

## Self-critique (three risks)

1. <risk> — mitigation: <...> OR follow-up test: <...>
2. ...
3. ...

## Diff

(Full `git diff <base>..HEAD` inline, or a reference to the command the user can run.)

## Sign-off

- [ ] User reviewed findings.
- [ ] User reviewed diff.
- [ ] User approves proceeding to finish-branch.

Compression policy

• Findings: terse bullets; location + the specific defect; no pleasantries.
• Diff: verbatim. Never paraphrase or trim.
• Spec quotes: verbatim.
• Self-critique rationale: one line per risk.

Sign-off gate

After writing the review document, present to the user:

Review: <path>.
Blocks: <n>. Warns: <n>. Nits: <n>.
Diff: <lines added>/<lines removed> across <N commits>.

Approve proceeding to finish-branch? (yes / fix / abort)

Wait for the user's response.

• yes → hand off to finish-branch.
• fix → user gives the specific fixes they want; the orchestrator plans a small targeted update (not a full re-plan).
• abort → stop. User decides whether to roll back the vibe branch or keep it for later.

Never auto-proceed. The user's explicit sign-off is non-optional.

Hard rule: blocks halt the handoff

If the review finds any block-severity issue, the handoff to finish-branch is not available — only fix or abort. The user cannot approve a yes that overrides an unresolved block. Surface this clearly:

Review has <N> block finding(s). `yes` is not available. Choose `fix` or `abort`.

This preserves the invariant that finish-branch only runs on work that passed both verify-gate and a clean review.

Anti-patterns

• Treating warn as block to inflate caution. If it is blocking, it is block; otherwise let the user decide.
• Running review-pack as a silent automation. It requires explicit user sign-off.
• Presenting findings without file:line evidence. Every finding cites a location.
• Proposing refactors outside the spec's scope. Review-pack critiques this work against this spec, not the whole codebase.
• Accepting yes after any block finding.

Output of this skill

• A committed review document at docs/reviews/YYYY-MM-DD-<feature>-review.md.
• An explicit user response: yes (→ finish-branch), fix (→ targeted update), or abort.

Terminal states: yes unblocks finish-branch. fix returns to exec-dispatch for the specific items the user chose. abort stops the pipeline with the branch retained.