sp-review-against-plan

Review Against Plan

Overview

You wrote a plan. A worker executed it. Now you're reviewing their PR against that plan. Your job isn't to re-review the whole codebase — specialized agents (pr-review-lead etc.) do that. Your job is narrower and sharper: did the PR actually deliver what the plan said it would?

This skill exists because lenient "it's mostly done, minor gaps can be follow-up nits" reviews leak scope and waste pipeline cycles. Read the rule, then the anti-pattern, then do the review.

The Rule

Every finding falls into exactly one of three categories:

1. In-scope + done ✓ — plan called for it, PR delivered it. No action. Don't bother listing these except as a sanity check.

2. In-scope + undone 🔴 — plan called for it, PR didn't deliver it (or delivered a partial/watered-down version). ALWAYS BLOCKING. Never "non-blocking." Never "fold into follow-up PR." Never "minor gap." If it was in the plan, it's in this PR — or the PR is not done.

3. Cosmetic + no value ⚪ — not in the plan, no real value if added. Don't mention at all. Not even as a "minor nit." Noise wastes attention.

There is no fourth category. No "minor gap, non-blocking, fold into follow-up" escape hatch. That hatch is how scope leaks.

Non-criteria (don't flag these)

• Commit count / commit message style — we squash-merge. Worker's commit granularity is cosmetic and disappears at merge time. 3 commits vs 1 vs 5 — doesn't matter.
• Formatting / whitespace — covered by linters and pre-commit hooks.
• Code style choices equivalent to what you'd have written — if it works and matches the plan, don't re-grade against the version in your head.

Anti-pattern (real example)

From an actual past review:

Review complete. Build + tests + vet all pass. CI still running. Matches plan exactly: ... Minor gaps (non-blocking):

1. Aliyun 3-function refactor skipped — subtask e1f0a134 annotation asked for extracting three helper functions; worker inlined them instead.
2. One AssemblyAI edge case uncovered — we're still billed. Rare case. Could be folded into 7a8ee4f1 cleanup or a follow-up nit commit.

Both items are Category 2 (in-scope + undone). Both were in the plan's subtask annotations. Both got mislabeled as "non-blocking."

What went wrong:

• The reviewer felt the "spirit" of the plan was met, so forgave the specific asks.
• "Follow-up nit PR" sounds cheap but costs a full pipeline slot (plan → implement → review → merge) for work that was already scoped.
• The edge case that costs real money ("still billed") got called "rare" to defer it.

Correct classification:

• Aliyun refactor → Category 2 → BLOCKING → NEED_WORK.
• AssemblyAI edge case → Category 2 → BLOCKING → NEED_WORK.

How to run the review

1. Load the plan

# Execution steps (what the worker was supposed to do):
task <task-uuid> tree

# Orientation (what/why/anti-goals — if present):
# Look for "orientation: flicknote <hex>" in the task annotations, then:
flicknote detail <hex>

2. Walk the plan, bucket each item

For each subtask in the tree and each explicit ask in annotations:

• Read the subtask body + annotations — understand what was actually asked for, including nuances buried in annotation text.
• Check the PR diff for evidence the ask was delivered (git diff origin/main..HEAD or the PR URL).
• Classify: Category 1 ✓ / Category 2 🔴 / Category 3 ⚪ (silent).

Do NOT grade ambition (would a different plan have been better?). You're grading execution against YOUR plan.

3. Emit the verdict

LGTM — zero Category 2 findings. Every plan item is delivered. Advance the pipeline:

ttal go <hex>
# spawns pr-review-lead for the deeper review pass

NEED_WORK — one or more Category 2 findings. Send blockers directly to the worker with ttal send:

cat <<EOF | ttal send --to <hex>:coder
# NEED_WORK

## Blockers (Category 2 — in-scope, undone)

1. <concrete finding>
   Plan asked for: <quote/paraphrase from subtask or annotation>
   PR delivered:  <what's there, or nothing>
   Fix:           <what needs to happen>

2. <next finding>
   ...

## Questions (optional — ask before fixing if unclear)

- <question>
EOF

The worker fixes, pushes new commits, and you re-review. Use the configured task channel for coordination.

Framing discipline

• Findings are concrete. "Plan said extract three helpers; PR inlined them." Not "code quality could be better."
• Cite the plan. Quote the annotation or reference the subtask UUID. The worker shouldn't have to guess what you're referring to.
• Be terse. Category 2 findings are mechanical: plan said X, PR has Y, fix Z. No essays.
• Don't mix categories. If it's Category 3, drop it. Mixing cosmetic wishes into blocker feedback dilutes the signal.

When to escalate

• If the plan itself turns out to be wrong (worker's interpretation was reasonable, yours was unclear) → that's a plan-quality issue on you, not a NEED_WORK on the worker. Revise the plan, annotate the task, re-review.
• If the worker pushed back in a previous round and you now agree → retract that finding, don't double down.
• If the worker genuinely can't do the Category 2 item for reasons outside their control → escalate to the configured admin. Don't downgrade to non-blocking to paper over it.

cat <<'EOF' | ttal send --to <admin-alias>
blocked: <reason>
EOF