Add plan review workflow with hook enforcement

[igerber]

Summary

• Add persistent review output: /review-plan now saves structured reviews as .review.md files with YAML frontmatter (verdict, issue counts, flags) alongside the plan file, and writes a sentinel for the hook
• Add /revise-plan skill: reads review feedback, displays it in the terminal, collects user overrides (accept/dismiss/modify per-issue), enters plan mode for revision, and appends audit trail as ## Revision Notes
• Add ExitPlanMode hook gate: check-plan-review.sh blocks plan approval unless a .review.md sibling exists (sentinel-first, fallback to most recent plan)
• Enhance review cross-referencing: issues are now numbered (CRITICAL Add initial diff-diff library implementation #1, MEDIUM Add initial diff-diff library implementation #1) for use with /revise-plan directives
• Support --updated delta from review files: when conversation context is compressed between rounds, the prior .review.md file is read for delta assessment

Methodology references (required if estimator / math changes)

• N/A - no methodology changes. This PR adds tooling for plan review/revision workflow only.

Validation

• No test changes (workflow tooling, not library code)
• Manual verification: hook script tested with sentinel present/absent, review file present/absent

Security / privacy

• Confirm no secrets/PII in this PR: Yes

---

Generated with Claude Code

[github-actions]

Overall assessment: ⚠️ Needs changes

Executive summary

• No estimator, math, variance/SE, or identification changes; methodology registry is not implicated.
• The ExitPlanMode hook can approve a plan that was edited after review because it only checks for file existence, not freshness.
• /revise-plan has an unclear “skip review” branch that jumps into revision steps that assume review context, risking inconsistent behavior and revision notes.
• Review-plan constraints allow writing .review.md next to a plan file even if it’s inside the repo, which conflicts with the “project files read-only” constraint and can dirty the worktree.

Methodology
None. This PR is tooling/workflow-only with no estimator or methodology changes.

Code Quality

• P2 — Impact: A plan can be modified after review and still pass the ExitPlanMode gate because the hook only checks for the review file’s existence, not whether it’s stale. This undermines “review before approval” for updated plans. Concrete fix: compare mtimes and deny when PLAN_FILE is newer than REVIEW_FILE (e.g., [ "$PLAN_FILE" -nt "$REVIEW_FILE" ] && deny "Review is stale..."). Location: .claude/hooks/check-plan-review.sh:L37-L66.

Performance
None.

Maintainability

• P2 — Impact: The “skip review” path in /revise-plan jumps directly to Step 6 (user input) and then Step 7 (revision rules) without any review context, but those steps assume parsed review issues and checklist gaps. This creates ambiguous behavior and inconsistent revision notes. Concrete fix: define explicit behavior for no-review flows (e.g., skip Steps 3–5, treat all issue counts as zero, and in ## Revision Notes record “Review skipped — no issues to address” while applying user notes as general guidance). Location: .claude/commands/revise-plan.md:L55-L75 and .claude/commands/revise-plan.md:L103-L163.

Tech Debt
None.

Security
None.

Documentation/Tests

• P3 — Impact: The updated “read-only for project files” constraint can be violated if a plan file is inside the repo, since the workflow now writes <plan-stem>.review.md next to the plan. That can dirty the repo and contradicts the constraint. Concrete fix: either enforce plan paths outside the repo (warn/abort if inside) or always write review outputs to ~/.claude/plans/ regardless of plan location, and update the wording accordingly. Location: .claude/commands/review-plan.md:L42-L48 and .claude/commands/review-plan.md:L407-L455.
• P3 — Impact: No automated validation for the hook or review-file format; regressions could silently weaken enforcement. Concrete fix: add a lightweight shell test or a documented manual verification checklist for check-plan-review.sh and .review.md frontmatter. Location: .claude/hooks/check-plan-review.sh:L1-L67, .claude/commands/review-plan.md:L407-L455.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 45a95dcae645ed5f03716e4a872524bcbfa69a75

---

Overall Assessment
Needs changes

Executive Summary

• No estimator/methodology changes in this PR; Methodology Registry not implicated.
• P1: /revise-plan skip-review flow does not update .last-reviewed, so the hook can validate the wrong plan when the sentinel is stale.
• P2: Hook validation is basename/sentinel-based, so duplicate plan filenames or stale sentinel can still yield false allow/deny outcomes.
• P3: revise-plan references “Change 1, Step 6” without a local definition, risking inconsistent review file formatting.

Methodology
None. No estimator, math, weighting, variance/SE, identification assumptions, or default behavior changes in this PR.

Code Quality

• P1: Skip-review path writes a “Skipped” review marker but does not update the sentinel that the hook uses to select the plan. Impact: ExitPlanMode can be allowed (or denied) based on a different plan’s review when .last-reviewed is stale. Fix: in the skip-review flow, always write ~/.claude/plans/.last-reviewed with the current plan path at the same time as the skipped review marker, and consider updating it whenever the plan path is resolved. .claude/commands/revise-plan.md:L74 .claude/hooks/check-plan-review.sh:L44

Performance
None.

Maintainability
None.

Tech Debt

• P2: Review lookup is basename-only and sentinel-first; this can collide if two plans share a filename or if the sentinel is stale. Impact: wrong review can be accepted or a correct review can be ignored. Fix: include a stable full-path identifier (hash or sanitized path) in the review filename or parse the YAML plan: field and require it to match the plan path being validated. .claude/hooks/check-plan-review.sh:L44

Security
None.

Documentation/Tests

• P3: revise-plan references “Change 1, Step 6” without a local definition. Impact: ambiguous guidance for formatting the review file, which can break hook expectations. Fix: explicitly reference .claude/commands/review-plan.md Step 6 or inline the frontmatter template. .claude/commands/revise-plan.md:L70
• P3: No automated verification for the new hook/commands. Impact: regressions will be detected only by manual use. Fix: add a lightweight shell test or a documented manual verification checklist adjacent to the hook. .claude/hooks/check-plan-review.sh:L1

Tests not run (workflow/docs change only).

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: ff5ad9afa71d07197e7aa0131b49cf23333cafbc

---

Overall Assessment
⚠️ Needs changes

Executive Summary

• No estimator/identification/variance changes in this PR; methodology registry checks are not applicable.
• P1: The new staleness gate will block the documented /revise-plan and “present for approval” paths after any plan edits unless a re-review is forced.
• P2: The review frontmatter plan: example/wording is ambiguous for non-~/.claude/plans plan locations and can cause the hook to reject valid reviews.
• P3: Minor comment/code mismatch in the hook’s JSON-escaping note.

Methodology

• None (no estimator/identification/variance changes in diff).

Code Quality

• P1 — Impact: ExitPlanMode will be denied after any plan edits because the hook requires the plan mtime to be ≤ the review mtime, but both /revise-plan and the “Address all issues and present for approval” path instruct revising the plan and then calling ExitPlanMode without re-review. This makes the documented flow impossible and forces an implicit re-review loop. Concrete fix: either require re-review explicitly (remove the “present for approval” option and update /revise-plan to re-run /review-plan --updated before exiting), or update the review file after revision and relax the hook when the user explicitly chooses to proceed without re-review. References: CLAUDE.md:L681-L694, .claude/commands/revise-plan.md:L164-L205, .claude/hooks/check-plan-review.sh:L68-L71, .claude/hooks/check-plan-review.sh:L95-L97.

Performance

• None.

Maintainability

• P3 — Impact: The hook comment claims deny() does not escape quotes/backslashes, but the implementation already does. This is misleading for future changes. Concrete fix: update or remove the stale comment. Reference: .claude/hooks/check-plan-review.sh:L16-L37.

Tech Debt

• None.

Security

• None.

Documentation/Tests

• P2 — Impact: The review frontmatter example and wording imply plan: should be ~/.claude/plans/foo.md even when the actual plan file lives elsewhere. The hook validates plan: against the real plan path, so this ambiguity can cause valid reviews to be rejected for non-standard plan locations. Concrete fix: explicitly state that plan: must be the actual plan file path (expanded), and update the example accordingly. References: .claude/commands/review-plan.md:L427-L456, .claude/hooks/check-plan-review.sh:L41-L71.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: a2f1a85c3e67ee289610e8f25b2673360c091571

---

Overall assessment: ⚠️ Needs changes

Executive Summary

• No estimator/methodology changes in this PR; Methodology Registry not implicated.
• ExitPlanMode hook can be bypassed if .last-reviewed points to an older plan that still has a fresh review; the hook exits early and won’t check for a newer plan file. .claude/hooks/check-plan-review.sh:L55-L79
• The staleness-check snippet in /revise-plan does not quote <plan-path>, which will break for paths with spaces. .claude/commands/revise-plan.md:L119-L125

Methodology
None — no estimator, math, weighting, variance/SE, or identification changes in the diff.

Code Quality

• Severity: P2. Impact: If .last-reviewed points to an older plan that still exists and has a fresh review, the hook exits early and allows ExitPlanMode even when a newer plan hasn’t been reviewed. This undermines the enforcement path in the “forgot to update sentinel” scenario. Fix: when the sentinel is present, compare it to the most recent plan in ~/.claude/plans/; if a newer/different plan exists, treat the sentinel as stale and deny with a message to update the sentinel or re-run review, or switch to the newest plan for checks. .claude/hooks/check-plan-review.sh:L55-L79

Performance
None.

Maintainability
None.

Tech Debt
None.

Security
None.

Documentation/Tests

• Severity: P3. Impact: The staleness-check example uses an unquoted <plan-path>, which breaks on paths with spaces and can produce false results. Fix: quote <plan-path> or assign it to a variable and quote it in the -nt test. .claude/commands/revise-plan.md:L119-L125

Tests not run (not requested).

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 9fda24fd7aeaf6cb72ba022d828e8c2f373bd5c0

---

Overall assessment: ⚠️ Needs changes

Executive Summary

• P1: The new hook hard-requires a .review.md, but /review-plan explicitly treats write failures as non-blocking; if ~/.claude/plans is missing or unwritable, ExitPlanMode becomes impossible despite a completed review.
• P2: The “newer plan exists” guard can deny approval for the active plan if any other plan file is newer, even if the active plan is fully reviewed.
• Methodology registry reviewed; no estimator/method changes in this PR.

Methodology

• None. No estimator/math/SE/identification changes were introduced; registry requirements are not implicated.

Code Quality

• P1 — Impact: Workflow dead‑end when review file cannot be written (missing/wrong perms on ~/.claude/plans), because the hook will still deny ExitPlanMode even though the review was completed. Fix: Make review file creation a hard requirement in the command flows—ensure ~/.claude/plans exists (e.g., allow mkdir -p or stop with a clear error) and abort the review if the file cannot be written. Align /review-plan and /revise-plan guidance so write failure is blocking, not “warn and continue.” Location: .claude/commands/review-plan.md:L423-L468, .claude/hooks/check-plan-review.sh:L75-L114.
• P2 — Impact: ExitPlanMode can be denied for the active plan whenever any newer plan exists in ~/.claude/plans and is unreviewed, even if the current plan is fully reviewed (false negatives for multi‑plan workflows). Fix: Only enforce the “newer plan exists” guard when the sentinel is missing/invalid or when the sentinel plan’s review is missing/stale; otherwise allow and optionally warn. Location: .claude/hooks/check-plan-review.sh:L61-L72.

Performance

• None.

Maintainability

• None.

Tech Debt

• None.

Security

• None.

Documentation/Tests

• P3 — Impact: Hook behavior is only described via a manual checklist; no automated tests cover the sentinel/review/staleness matrix, so regressions are easy. Fix: Add a small shell test harness (even a CI script) that exercises the 12 cases listed in the checklist and asserts allow/deny outputs. Location: .claude/hooks/check-plan-review.sh:L116-L128.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: cb8d5e3e792fc73c8fa156bd86184b04baaa4452

---

Overall Assessment
✅ Looks good

Executive Summary

• No estimator, math, weighting, variance/SE, or identification logic changed; methodology registry not implicated by this diff.
• The new hook enforces presence and freshness of plan reviews before ExitPlanMode.
• Minor spec inconsistency: /revise-plan can persist plan content into .review.md, contradicting /review-plan’s “review-only” file format.
• Constraint mismatch: /review-plan forbids file-modifying Bash commands but later requires mkdir -p for the plans dir.
• No tests added (workflow/docs change only).

Methodology
None.

Code Quality

• Severity: P2 | Impact: /revise-plan instructs saving the review agent’s full output, but /review-plan explicitly says plan content must not be written to .review.md. This can cause .review.md to include the full plan (duplication in Step 3 display, larger files, and a format mismatch with the spec). | Concrete fix: When saving agent output, strip the “Plan Content” portion and persist only the review body (“## Overall Assessment” through “## Summary”), or update /review-plan to explicitly allow plan content in the review file and adjust /revise-plan Step 3 to avoid double display. | Location: .claude/commands/revise-plan.md:L59-L71, .claude/commands/review-plan.md:L297-L312, .claude/commands/review-plan.md:L423-L458

Performance
None.

Maintainability

• Severity: P3 | Impact: /review-plan constraints forbid file-modifying Bash commands, but Step 6 requires mkdir -p ~/.claude/plans. This is an internal policy conflict that can block correct execution when the directory is missing. | Concrete fix: Explicitly allow mkdir -p ~/.claude/plans in Constraints, or revise the constraint language to permit directory creation for review outputs. | Location: .claude/commands/review-plan.md:L42-L48, .claude/commands/review-plan.md:L423-L430

Tech Debt
None.

Security
None.

Documentation/Tests
None.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 1376aa7f4a0a935c8097cadf5da92d58cefde1fa

---

Overall assessment: ⚠️ Needs changes

Executive Summary

• No estimator, math, weighting, variance/SE, identification, or default-behavior changes in this PR; methodology registry cross-check not applicable.
• The ExitPlanMode hook can be bypassed if the sentinel points to an older plan; enforcement relies on manual sentinel updates and can approve an unreviewed newer plan. See .claude/hooks/check-plan-review.sh:L16-L75.
• Hook verification is manual-only; consider a small automated test to protect the gating logic. See .claude/hooks/check-plan-review.sh:L102-L111.
• Changes are otherwise scoped to plan-review workflow docs, commands, and hook wiring.

Methodology
None (no estimator/inference changes in the diff).

Code Quality

• P2 (Impact: The review gate can approve an unreviewed plan when the sentinel is stale, weakening the core enforcement guarantee). Concrete fix: If the sentinel plan is not the most recently modified plan, either deny or fall back to the newest plan; alternatively, persist the current plan path automatically from EnterPlanMode and validate against that before allowing ExitPlanMode. Location: .claude/hooks/check-plan-review.sh:L16-L75.

Performance
None.

Maintainability
None.

Tech Debt
None.

Security
None.

Documentation/Tests

• P3 (Impact: Logic regressions in the hook are likely to slip through without automated checks). Concrete fix: Add a small shell-based test that exercises the nine cases listed in the manual checklist and run it in CI. Location: .claude/hooks/check-plan-review.sh:L102-L111.
  Tests not run (not requested).

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 4947a3196c10019670360320100463a7db98b07a

---

Overall assessment: Looks good

Executive Summary

• No estimator or methodology code paths were changed; updates are confined to .claude/ tooling and CLAUDE.md.
• The new hook script enforces review presence and freshness and is backed by a dedicated test script.
• Low-risk correctness gap: review selection by basename in the review/revise commands can mis-associate prior reviews if basenames collide.

Methodology
None. No estimator, weighting, variance/SE, or identification logic changed in this diff (registry consulted).

Code Quality

• P3 Impact: review-plan and revise-plan read prior reviews by basename only, so two plans with the same filename in different directories can pull the wrong review and produce an incorrect delta or revision guidance. Concrete fix: when reading ~/.claude/plans/<plan-basename>.review.md, parse the YAML plan: field and compare it (after ~ expansion) to the resolved plan path; if mismatched or missing, warn and treat as “no prior review.” Locations: .claude/commands/review-plan.md:L72-L74, .claude/commands/revise-plan.md:L49-L53.

Performance
None.

Maintainability
None.

Tech Debt
None.

Security
None.

Documentation/Tests
None.