diff --git a/.claude/commands/review-plan.md b/.claude/commands/review-plan.md index e57b40bc..2f7b4c65 100644 --- a/.claude/commands/review-plan.md +++ b/.claude/commands/review-plan.md @@ -41,11 +41,11 @@ Options: ## Constraints -- **Read-only**: Do NOT create, edit, or delete any files. This skill only reads and reports. +- **Read-only for project files**: Do NOT create, edit, or delete any project files (source code, tests, documentation, configuration). The only files this skill writes are the review output (`~/.claude/plans/.review.md`) and the sentinel (`~/.claude/plans/.last-reviewed`), both in `~/.claude/plans/`. - **Advisory-only**: Provide feedback and recommendations. Do not implement fixes. - **No code changes**: Do not modify any source code, test files, or documentation. -- Use the Read tool for files and the Glob/Grep tools for searching. Do not use Write, Edit, NotebookEdit, or any file-modifying Bash commands. -- The `gh api` calls used with `--pr` are read-only API requests, consistent with the read-only constraint. +- Use the Read tool for files and the Glob/Grep tools for searching. Do not use Edit, NotebookEdit, or file-modifying Bash commands on project files. The Write tool and `mkdir -p` may only target `~/.claude/plans/` (for the review output file, sentinel, and output directory). +- The `gh api` calls used with `--pr` are read-only API requests, consistent with the project-files read-only constraint. ## Instructions @@ -69,7 +69,9 @@ After completing the standard 8-dimension review in Step 4, add a **Delta Assess - Which previously-raised issues remain unresolved? - Are there any new issues introduced by the revisions? -If no prior review is available in conversation context (e.g., the user passed `--updated` on the first invocation, or the context was compressed), still include the Delta Assessment section but fill each subsection with: "Delta assessment unavailable — no prior review found in conversation context. Full fresh review performed." +Additionally, check if a prior review file exists at `~/.claude/plans/.review.md` (derived from the plan's basename, always in `~/.claude/plans/`). If it exists, read it as a supplementary source of prior review context. When conversation context has been compressed between rounds, use the review file's content for delta assessment instead. If both conversation context and the review file are available, prefer whichever source is more detailed. + +If no prior review is available from either source (conversation context or review file), still include the Delta Assessment section but fill each subsection with: "Delta assessment unavailable — no prior review found in conversation context or review file. Full fresh review performed." ### Step 2: Read CLAUDE.md for Project Context @@ -292,9 +294,25 @@ Use judgment, not just substring matching — the plan may use different words t - Unaddressed P2/Medium items count as Medium issues - Unaddressed P3/Low items count as Low issues +### Step 4b: Display Plan Content + +Before presenting the review, display the full plan content so the user can cross-reference the review findings against what was actually written: + +``` +## Plan Content: + + + +--- +``` + +This ensures the user can read the plan immediately before reading the review findings. Display the full plan content as-is from the file. + +Note: The plan content is displayed in the terminal only — it is NOT included in the `.review.md` file (Step 6), which contains only the review output. The plan is already persisted as its own file. + ### Step 5: Present Structured Feedback -Present the review in the following format. Do NOT skip any section — if a section has no findings, write "None." for that section. The Delta Assessment section is only included when the `--updated` flag was provided (see Step 1b). The PR Context and PR Feedback Coverage sections are only included when `--pr` was provided with a non-empty comment. +Present the review in the following format. Number each issue sequentially within its severity section (e.g., CRITICAL #1, CRITICAL #2, MEDIUM #1) to enable cross-referencing with `/revise-plan`. Do NOT skip any section — if a section has no findings, write "None." for that section. The Delta Assessment section is only included when the `--updated` flag was provided (see Step 1b). The PR Context and PR Feedback Coverage sections are only included when `--pr` was provided with a non-empty comment. ``` ## Overall Assessment @@ -402,11 +420,72 @@ The `--pr` URL must be the same across the initial review and the `--updated` re - **Needs revision**: Has critical issues or many medium issues that require rethinking the approach ``` +### Step 6: Save Review to File + +After displaying the review in the conversation (Step 5), persist it to a file alongside the plan. + +0. **Ensure the plans directory exists**: + ```bash + mkdir -p ~/.claude/plans + ``` + +1. **Derive the review file path**: Extract the plan file's basename, replace the trailing `.md` with `.review.md`, and place it in `~/.claude/plans/`. For example, `~/.claude/plans/foo.md` → `~/.claude/plans/foo.review.md`. If the plan is at `/repo/.claude/plans/bar.md`, the review still goes to `~/.claude/plans/bar.review.md`. + +2. **Get the current timestamp**: + ```bash + date -u +%Y-%m-%dT%H:%M:%SZ + ``` + +3. **Construct the review file** with YAML frontmatter followed by the review body: + + ```yaml + --- + plan: ~/.claude/plans/foo.md + reviewed_at: "2026-02-15T14:30:00Z" + verdict: "Needs revision" + critical_count: 2 + medium_count: 3 + low_count: 1 + flags: ["--updated", "--pr"] + --- + ``` + + The `plan:` value must be the plan file path as resolved in Step 1 — the same path used throughout this skill invocation. The hook expands `~` to `$HOME` before comparison, so `~/...` paths work correctly. The key requirement is that this value, after `~` expansion, exactly matches the plan file path the hook resolves from the sentinel or fallback. + + The `flags` field is a list of CLI flags that were active during this review. Possible values: `"--updated"`, `"--pr"`. Empty list `[]` if no flags were used. + + Followed by the full review content (everything from "## Overall Assessment" through "## Summary", exactly as displayed in the conversation). + +4. **Write the review file** using the Write tool. Overwrite any existing file at this path (expected on `--updated` re-reviews). + +5. **Write the sentinel file** `~/.claude/plans/.last-reviewed` containing the plan file path (just the path, no YAML): + ``` + ~/.claude/plans/foo.md + ``` + This sentinel is read by the ExitPlanMode hook to identify which plan was most recently reviewed. + +6. **Abort on write failure**: If the review file write fails, report a hard error and stop. Do NOT proceed with the "Tip: In the planning window..." footer. The review file is required by the ExitPlanMode hook — a missing file will permanently block plan approval. + ``` + Error: Failed to write review file to . + Ensure ~/.claude/plans/ exists and is writable, then retry. + The review content was displayed above — copy it if needed. + ``` + If the sentinel write fails, emit a warning (the sentinel is a convenience, not a hard requirement — the hook falls back to `ls -t`). + +7. **Append a footer** to the conversation output: + ``` + --- + Review saved to: + Tip: In the planning window, the review will be read automatically before plan approval. + ``` + ## Notes -- This skill is strictly read-only — it does not create, edit, or delete any files +- This skill is read-only for project files — it writes two files in `~/.claude/plans/`: the review output (`.review.md`) and the sentinel (`.last-reviewed`) - Plan files are typically located in `~/.claude/plans/` -- The review is displayed directly in the conversation, not saved to a file +- The review is displayed in the conversation (primary reading surface) and saved to a `.review.md` file alongside the plan (for persistence and cross-session exchange) +- On `--updated` re-reviews, the prior `.review.md` file is read for delta context and then overwritten with the new review +- Pairs with the in-plan-mode review workflow (CLAUDE.md) for in-session review - For best results, run this before implementing a plan to catch issues early - The 8 dimensions are tuned for plan-specific failure modes, not generic code review - Use `--updated` when re-reviewing a revised plan to get a delta assessment of what changed since the prior review @@ -420,5 +499,3 @@ The `--pr` URL must be the same across the initial review and the `--updated` re timestamp on any PR comment and selecting "Copy link". - For `pullrequestreview-` URLs, both the review body and its inline comments are fetched (matching `/read-feedback-revise` behavior). -- Verification of these changes should be done manually before merging the PR - that adds this feature. See the Verification section of this plan. diff --git a/.claude/commands/revise-plan.md b/.claude/commands/revise-plan.md new file mode 100644 index 00000000..14da23c8 --- /dev/null +++ b/.claude/commands/revise-plan.md @@ -0,0 +1,224 @@ +--- +description: Read plan review feedback and revise the plan with user overrides +argument-hint: "[] [-- ]" +--- + +# Revise Plan + +Read structured review feedback for a Claude Code plan, display it in the terminal for user consideration, and revise the plan to address the issues — incorporating user overrides and notes. + +## Arguments + +`$ARGUMENTS` may contain: +- **Plan file path** (optional): Path to the plan file, e.g., `~/.claude/plans/foo.md`. If omitted, auto-detected from the most recent `.md` file in `~/.claude/plans/` (excluding `*.review.md` files). +- `--` separator followed by **user notes** (optional): Free-form text with directives about which review items to accept, reject, or modify. + +Parse `$ARGUMENTS` by splitting on ` -- ` (space-dash-dash-space). Everything before the separator is the plan path (if non-empty after trimming). Everything after is user notes. If `$ARGUMENTS` does not contain ` -- `, the entire argument is treated as the plan path (or empty if `$ARGUMENTS` is empty). + +Examples: +- `/revise-plan` — auto-detect plan, accept all review feedback +- `/revise-plan ~/.claude/plans/foo.md` — specific plan, accept all feedback +- `/revise-plan -- Disagree with CRITICAL #2, the API handles this` — auto-detect, with overrides +- `/revise-plan ~/.claude/plans/foo.md -- Skip all LOW items, for MEDIUM #1 use a simpler approach` — specific plan with overrides + +## Constraints + +- **Plan file only**: Only modifies the plan file. Does NOT create, edit, or delete any project source code, tests, or documentation. +- **Works outside plan mode**: This skill is invoked from a normal (non-plan-mode) conversation. It enters plan mode via `EnterPlanMode` for the revision step. +- **Terminal-first**: The review content is always displayed in the terminal for the user to read before any revision begins. + +## Instructions + +### Step 1: Locate Plan File + +Determine the plan file path: + +1. **From arguments**: If a path was provided before ` -- `, use it. +2. **From most recent plan**: If no path provided, find the most recent plan: + ```bash + ls -t ~/.claude/plans/*.md 2>/dev/null | grep -v '\.review\.md$' | head -1 + ``` +3. **Ask user**: If no plan file found, use AskUserQuestion: + ``` + Which plan file would you like to revise? + Enter the path (e.g., ~/.claude/plans/plan-name.md) + ``` + +Verify the plan file exists by reading it. If it doesn't exist, report the error and stop. + +### Step 2: Locate and Read Review File + +Derive the review file path: extract the plan file's basename, replace the trailing `.md` with `.review.md`, and look in `~/.claude/plans/`. For example, `~/.claude/plans/foo.md` → `~/.claude/plans/foo.review.md`. + +**If the review file exists**: Read it, proceed to Step 3. + +**If the review file does not exist**: Use AskUserQuestion: +- "Run a review now (spawns a review agent)" (Recommended) +- "Skip review and enter plan mode directly" + +If "Run a review now" is chosen: +- Use the Task tool with `subagent_type: "general-purpose"`. Prompt the agent: + ``` + You are reviewing a Claude Code plan file as an independent reviewer. + + 1. Read the review criteria from `.claude/commands/review-plan.md` (Steps 2 through 5) + 2. Read the plan file at: + 3. Follow the review instructions: read CLAUDE.md for project context, read referenced files, evaluate across 8 dimensions, present structured feedback + 4. Number each issue sequentially within its severity section (CRITICAL #1, MEDIUM #1, etc.) + 5. Return ONLY the structured review output (from "## Overall Assessment" through "## Summary"). Do NOT include the "## Plan Content" display (Step 4b) — it is for terminal display only and must not be persisted to the review file. + ``` +- Save the agent's review output (from "## Overall Assessment" through "## Summary") to the review file path with YAML frontmatter (see `.claude/commands/review-plan.md` Step 6 for format). Do not include plan content in the review file. +- Write the plan path to `~/.claude/plans/.last-reviewed` +- Proceed to Step 3 with the review content + +If "Skip review" is chosen: +- Skip Steps 3-5 (no review to display, check, or parse) +- In Step 6, since there are no review issues, present only: + - "Enter plan mode with general guidance" (Recommended) + - "Cancel" + If "Cancel" is chosen, stop and report "Revision cancelled." +- In Step 7, since there are no review issues to address: + - Skip rule-based revision (no CRITICAL/MEDIUM/LOW to process) + - Apply user notes as general guidance for the revision + - Ensure the plans directory exists: `mkdir -p ~/.claude/plans` + - Write a minimal "Skipped" review marker to `~/.claude/plans/.review.md` (the centralized review path from Change 3) before calling `ExitPlanMode` to satisfy the hook: + ```yaml + --- + plan: + reviewed_at: + verdict: "Skipped" + critical_count: 0 + medium_count: 0 + low_count: 0 + flags: [] + --- + Review skipped by user. + ``` + - Write the plan path to `~/.claude/plans/.last-reviewed` (same as the review-present path in Step 2) + - In `## Revision Notes`, record: "Review skipped — revision based on user notes only" + - All issue counts are zero in the Addressed/Dismissed/Open sections + - If the review marker write fails, report an error and stop — the hook requires this file on disk. + +### Step 3: Display Plan and Review in Terminal + +Display both the plan content and the review in the conversation. The plan was already read in Step 1, and the review in Step 2. This is the primary reading surface — the user reads both here. + +``` +## Plan: + + + +--- + +## Review for + + + +--- +Source: +``` + +### Step 4: Staleness Check + +Compare file modification times. The review file is always in `~/.claude/plans/`: +```bash +PLAN_PATH="" +REVIEW_PATH="$HOME/.claude/plans/$(basename "$PLAN_PATH" .md).review.md" +[ "$PLAN_PATH" -nt "$REVIEW_PATH" ] && echo "STALE" || echo "FRESH" +``` + +If the plan is newer than the review, warn: +``` +Warning: The plan file was modified after this review was generated. +The review may be commenting on an older version of the plan. +Consider running `/review-plan --updated` for a fresh review. +``` + +### Step 5: Parse and Summarize Review + +Extract from the review content: +- Issues by severity: CRITICAL #N, MEDIUM #N, LOW #N +- Checklist gaps +- Questions for Author +- Verdict + +Display a summary: +``` +Found: N CRITICAL, N MEDIUM, N LOW issues, N checklist gaps, N questions +Verdict: +``` + +### Step 6: Collect User Input + +If user notes were provided in `$ARGUMENTS` (after ` -- `), parse them for directives: +- "disagree with #N" or "dismiss #N" → mark that issue as dismissed +- "skip #N" → mark as dismissed +- "skip all LOW" → dismiss all LOW severity issues +- "for #N, do X instead" → override the suggested fix +- "address all" or no specific directives → accept all feedback +- Free-form text applies as general guidance + +If no user notes were provided, use AskUserQuestion: +- "Address all issues" (Recommended) +- "Let me specify which items to address or dismiss" + +If "Let me specify" is chosen, the user provides free-form text. Parse as above. + +### Step 7: Enter Plan Mode and Revise + +Call `EnterPlanMode` to transition into plan mode. In plan mode: + +1. **Read the current plan file** in full +2. **Read source files** referenced in CRITICAL/MEDIUM issues and files the plan proposes to modify +3. **Revise the plan** following these rules: + - **CRITICAL issues**: Address unless user explicitly dismissed with justification + - **MEDIUM issues**: Address unless user dismissed + - **LOW issues**: Skip unless user explicitly requested them + - **Questions for Author**: Incorporate user's answers if provided; otherwise note as "Open — to be resolved during implementation" + - **Checklist gaps**: Add missing items as plan steps where appropriate +4. **Append a `## Revision Notes` section** at the end of the plan: + ```markdown + ## Revision Notes + + Revised based on review at . + + ### Addressed + - CRITICAL #1: + - MEDIUM #2: + + ### Dismissed + - MEDIUM #3: + + ### Open + - Question #1: + ``` +5. **Write the revised plan** using the Edit or Write tool +6. **Touch the review file** to update its mtime so the hook's staleness check passes after an intentional revision: + ```bash + touch ~/.claude/plans/.review.md + ``` +7. **Call `ExitPlanMode`** for user approval + +### Step 8: Report + +After exiting plan mode, summarize: +``` +Plan revised: +- Addressed: N issues +- Dismissed: N issues +- Open: N items + +To re-review the revised plan: + /review-plan --updated +``` + +## Notes + +- This skill works outside plan mode and transitions into plan mode for the revision +- The review is always displayed in the terminal for the user to read before any revision begins +- If no review file exists, a review agent can be spawned automatically — no second window needed +- User notes/overrides take priority over review recommendations +- Only the plan file is modified; no project source code is touched +- The `## Revision Notes` section provides an audit trail of what was addressed and why +- Pairs with `/review-plan` for iterative revision: `/review-plan` generates feedback, `/revise-plan` addresses it +- For subsequent rounds, run `/review-plan --updated` to get a delta assessment of improvements diff --git a/.claude/hooks/check-plan-review.sh b/.claude/hooks/check-plan-review.sh new file mode 100755 index 00000000..cf75edb3 --- /dev/null +++ b/.claude/hooks/check-plan-review.sh @@ -0,0 +1,117 @@ +#!/bin/bash +# PreToolUse hook for ExitPlanMode: ensure a plan review exists before approval. +# +# Output protocol: PreToolUse hooks must output JSON to stdout and exit 0. +# Allow: exit 0 (no output, or JSON with permissionDecision: "allow") +# Deny: exit 0 with JSON { hookSpecificOutput: { permissionDecision: "deny", ... } } +# Error: exit 2 means hook error (not a deliberate block) — avoid this. +# +# Strategy: +# 1. Read ~/.claude/plans/.last-reviewed sentinel (written by review step) +# 2. If sentinel exists, use its contents as the plan path +# 3. If no sentinel, fall back to most recent .md in ~/.claude/plans/ +# 4. Check for .review.md in ~/.claude/plans/ (by plan basename) — deny if missing +# 5. Check staleness — deny if plan is newer than review +# +# Known limitations: +# - The ls -t fallback (step 3) can pick the wrong plan if multiple files exist. +# - A stale sentinel can approve an unreviewed newer plan. This occurs only when: +# (a) the sentinel points to an older plan with a valid review, AND +# (b) a newer plan was created without updating the sentinel. +# CLAUDE.md instructs sentinel updates on new plan creation (line 647), making (b) +# unlikely in practice. A runtime guard was tested and removed (cb8d5e3) because it +# caused false denials in multi-plan workflows. Automated tests in +# test-check-plan-review.sh verify existing behavior; full mitigation would require +# tool-level integration (not hook-level). +# - The -nt comparison has 1-second granularity on macOS. A plan edited and +# reviewed within the same second could produce a false "fresh" result. In +# practice, reviews always take longer. +# - Review files are derived from plan basename only. Two plans with the same +# filename in different directories would map to the same review file. Claude +# Code always creates plans in ~/.claude/plans/, so this is unlikely. +# +# Dependencies: None (uses printf for JSON output, no jq required). + +deny() { + # Output JSON deny decision to stdout, then exit 0 (not exit 2) + # Sanitize message: escape double quotes and backslashes for valid JSON + local msg + msg=$(printf '%s' "$1" | sed 's/\\/\\\\/g; s/"/\\"/g') + printf '{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"deny","permissionDecisionReason":"%s"}}' "$msg" + exit 0 +} + +# Validate that a review file's YAML plan: field matches the expected plan path. +# Usage: validate_review_plan_field +# Returns 0 if match, 1 if mismatch. Returns 1 if plan: field is missing (safe default). +validate_review_plan_field() { + local review_file="$1" expected="$2" + local yaml_plan + # Extract YAML frontmatter (lines 2 through next ---), then grep for plan: field. + # Uses a pipeline instead of nested sed braces for BSD sed (macOS) compatibility. + yaml_plan=$(sed -n '2,/^---$/p' "$review_file" | grep '^plan:' | head -1 | sed 's/^plan:[[:space:]]*//;s/[[:space:]]*$//;s/^"//;s/"$//;s/^'"'"'//;s/'"'"'$//') + # Expand ~ in the YAML value + yaml_plan="${yaml_plan/#\~/$HOME}" + [ "$yaml_plan" = "$expected" ] +} + +PLANS_DIR="$HOME/.claude/plans" +SENTINEL="$PLANS_DIR/.last-reviewed" + +# Step 1-2: Try sentinel first +if [ -f "$SENTINEL" ]; then + PLAN_FILE=$(head -1 "$SENTINEL" 2>/dev/null) + # Expand ~ if present + PLAN_FILE="${PLAN_FILE/#\~/$HOME}" + if [ -n "$PLAN_FILE" ] && [ -f "$PLAN_FILE" ]; then + PLAN_BASENAME=$(basename "$PLAN_FILE") + REVIEW_FILE="$PLANS_DIR/${PLAN_BASENAME%.md}.review.md" + if [ -f "$REVIEW_FILE" ]; then + if ! validate_review_plan_field "$REVIEW_FILE" "$PLAN_FILE"; then + deny "Review file $REVIEW_FILE is for a different plan (expected: $PLAN_FILE). Re-run the review." + fi + # Deny if plan was modified after its review (stale review) + if [ "$PLAN_FILE" -nt "$REVIEW_FILE" ]; then + deny "Plan review is stale: $PLAN_FILE was modified after $REVIEW_FILE. Re-run the review before approval." + fi + exit 0 # Review exists and is fresh, allow + else + deny "No plan review found for: $PLAN_FILE. Expected: $REVIEW_FILE. Run a plan review before presenting for approval." + fi + fi +fi + +# Step 3: Fall back to most recent plan file +PLAN_FILE=$(ls -t "$PLANS_DIR"/*.md 2>/dev/null | grep -v '\.review\.md$' | head -1) + +if [ -z "$PLAN_FILE" ]; then + # No plan files at all — allow ExitPlanMode (not a plan-mode session) + exit 0 +fi + +# Step 4-5: Check for review and staleness +PLAN_BASENAME=$(basename "$PLAN_FILE") +REVIEW_FILE="$PLANS_DIR/${PLAN_BASENAME%.md}.review.md" + +if [ -f "$REVIEW_FILE" ]; then + if ! validate_review_plan_field "$REVIEW_FILE" "$PLAN_FILE"; then + deny "Review file $REVIEW_FILE is for a different plan (expected: $PLAN_FILE). Re-run the review." + fi + if [ "$PLAN_FILE" -nt "$REVIEW_FILE" ]; then + deny "Plan review is stale: $PLAN_FILE was modified after $REVIEW_FILE. Re-run the review before approval." + fi + exit 0 # Review exists and is fresh, allow +else + deny "No plan review found. Expected: $REVIEW_FILE. Follow the Plan Review Before Approval instructions in CLAUDE.md." +fi + +# Manual verification checklist: +# 1. No sentinel, no plans: ExitPlanMode should ALLOW (not a plan session) +# 2. Sentinel points to plan with fresh review: ALLOW +# 3. Sentinel points to plan with stale review: DENY +# 4. Sentinel points to plan with no review: DENY +# 5. Sentinel stale (wrong plan), review exists for fallback plan: validate plan: field +# 6. No sentinel, fallback to most recent plan with review: ALLOW +# 7. No sentinel, fallback to most recent plan without review: DENY +# 8. Review file plan: field doesn't match plan path: DENY +# 9. Review file has no plan: field (e.g., old format): DENY (empty string != plan path) diff --git a/.claude/hooks/test-check-plan-review.sh b/.claude/hooks/test-check-plan-review.sh new file mode 100644 index 00000000..1f20f3cb --- /dev/null +++ b/.claude/hooks/test-check-plan-review.sh @@ -0,0 +1,206 @@ +#!/bin/bash +# Automated tests for check-plan-review.sh +# Run: bash .claude/hooks/test-check-plan-review.sh + +set -u + +HOOK_SCRIPT="$(cd "$(dirname "$0")" && pwd)/check-plan-review.sh" +TMPBASE=$(mktemp -d) +trap 'rm -rf "$TMPBASE"' EXIT + +PASS=0 +FAIL=0 + +run_test() { + local name="$1" case_dir="$2" expect="$3" + local output + output=$(HOME="$case_dir" bash "$HOOK_SCRIPT" 2>/dev/null) + if [ "$expect" = "allow" ]; then + if echo "$output" | grep -q '"permissionDecision":"deny"'; then + echo "FAIL: $name (expected ALLOW, got DENY)" + echo " output: $output" + FAIL=$((FAIL + 1)) + else + echo "PASS: $name" + PASS=$((PASS + 1)) + fi + else + if echo "$output" | grep -q '"permissionDecision":"deny"'; then + echo "PASS: $name" + PASS=$((PASS + 1)) + else + echo "FAIL: $name (expected DENY, got ALLOW)" + echo " output: $output" + FAIL=$((FAIL + 1)) + fi + fi +} + +# Helper: create a minimal review file with YAML frontmatter +# Usage: create_review +create_review() { + local review_path="$1" plan_path="$2" + cat > "$review_path" < "$PLAN" +touch -t 202601010001 "$PLAN" +create_review "$REVIEW" "$PLAN" +touch -t 202601010002 "$REVIEW" +echo "$PLAN" > "$CASE_DIR/.claude/plans/.last-reviewed" +run_test "Case 2: Sentinel → fresh review → ALLOW" "$CASE_DIR" "allow" + +# ============================================================ +# Case 3: Sentinel → plan with stale review → DENY +# ============================================================ +CASE_DIR="$TMPBASE/case3" +mkdir -p "$CASE_DIR/.claude/plans" +PLAN="$CASE_DIR/.claude/plans/test-plan.md" +REVIEW="$CASE_DIR/.claude/plans/test-plan.review.md" +echo "# Plan" > "$PLAN" +touch -t 202601010002 "$PLAN" +create_review "$REVIEW" "$PLAN" +touch -t 202601010001 "$REVIEW" +echo "$PLAN" > "$CASE_DIR/.claude/plans/.last-reviewed" +run_test "Case 3: Sentinel → stale review → DENY" "$CASE_DIR" "deny" + +# ============================================================ +# Case 4: Sentinel → plan with no review → DENY +# ============================================================ +CASE_DIR="$TMPBASE/case4" +mkdir -p "$CASE_DIR/.claude/plans" +PLAN="$CASE_DIR/.claude/plans/test-plan.md" +echo "# Plan" > "$PLAN" +echo "$PLAN" > "$CASE_DIR/.claude/plans/.last-reviewed" +run_test "Case 4: Sentinel → no review → DENY" "$CASE_DIR" "deny" + +# ============================================================ +# Case 5: Sentinel → non-existent file, fallback plan has review +# with matching plan: field → ALLOW +# ============================================================ +CASE_DIR="$TMPBASE/case5" +mkdir -p "$CASE_DIR/.claude/plans" +PLAN="$CASE_DIR/.claude/plans/real-plan.md" +REVIEW="$CASE_DIR/.claude/plans/real-plan.review.md" +echo "# Plan" > "$PLAN" +touch -t 202601010001 "$PLAN" +create_review "$REVIEW" "$PLAN" +touch -t 202601010002 "$REVIEW" +# Sentinel points to a non-existent file +echo "$CASE_DIR/.claude/plans/deleted-plan.md" > "$CASE_DIR/.claude/plans/.last-reviewed" +run_test "Case 5: Sentinel → non-existent file, fallback with review → ALLOW" "$CASE_DIR" "allow" + +# ============================================================ +# Case 6: No sentinel, fallback newest plan with review → ALLOW +# ============================================================ +CASE_DIR="$TMPBASE/case6" +mkdir -p "$CASE_DIR/.claude/plans" +PLAN="$CASE_DIR/.claude/plans/test-plan.md" +REVIEW="$CASE_DIR/.claude/plans/test-plan.review.md" +echo "# Plan" > "$PLAN" +touch -t 202601010001 "$PLAN" +create_review "$REVIEW" "$PLAN" +touch -t 202601010002 "$REVIEW" +# No sentinel file +run_test "Case 6: No sentinel, fallback with review → ALLOW" "$CASE_DIR" "allow" + +# ============================================================ +# Case 7: No sentinel, fallback newest plan without review → DENY +# ============================================================ +CASE_DIR="$TMPBASE/case7" +mkdir -p "$CASE_DIR/.claude/plans" +PLAN="$CASE_DIR/.claude/plans/test-plan.md" +echo "# Plan" > "$PLAN" +# No sentinel, no review +run_test "Case 7: No sentinel, fallback without review → DENY" "$CASE_DIR" "deny" + +# ============================================================ +# Case 8: Review file plan: field doesn't match plan path → DENY +# ============================================================ +CASE_DIR="$TMPBASE/case8" +mkdir -p "$CASE_DIR/.claude/plans" +PLAN="$CASE_DIR/.claude/plans/test-plan.md" +REVIEW="$CASE_DIR/.claude/plans/test-plan.review.md" +echo "# Plan" > "$PLAN" +touch -t 202601010001 "$PLAN" +# Review points to a different plan +create_review "$REVIEW" "/some/other/plan.md" +touch -t 202601010002 "$REVIEW" +echo "$PLAN" > "$CASE_DIR/.claude/plans/.last-reviewed" +run_test "Case 8: Review plan: field mismatch → DENY" "$CASE_DIR" "deny" + +# ============================================================ +# Case 9: Review file has no plan: field → DENY +# ============================================================ +CASE_DIR="$TMPBASE/case9" +mkdir -p "$CASE_DIR/.claude/plans" +PLAN="$CASE_DIR/.claude/plans/test-plan.md" +REVIEW="$CASE_DIR/.claude/plans/test-plan.review.md" +echo "# Plan" > "$PLAN" +touch -t 202601010001 "$PLAN" +# Review without plan: field in frontmatter +cat > "$REVIEW" < "$CASE_DIR/.claude/plans/.last-reviewed" +run_test "Case 9: Review has no plan: field → DENY" "$CASE_DIR" "deny" + +# ============================================================ +# Case 10: Sentinel → plan-A with review, newer plan-B also has +# review → ALLOW (sentinel is trusted, not overridden) +# ============================================================ +CASE_DIR="$TMPBASE/case10" +mkdir -p "$CASE_DIR/.claude/plans" +PLAN_A="$CASE_DIR/.claude/plans/plan-a.md" +REVIEW_A="$CASE_DIR/.claude/plans/plan-a.review.md" +PLAN_B="$CASE_DIR/.claude/plans/plan-b.md" +REVIEW_B="$CASE_DIR/.claude/plans/plan-b.review.md" +echo "# Plan A" > "$PLAN_A" +touch -t 202601010001 "$PLAN_A" +create_review "$REVIEW_A" "$PLAN_A" +touch -t 202601010002 "$REVIEW_A" +# Plan B is newer +echo "# Plan B" > "$PLAN_B" +touch -t 202601010003 "$PLAN_B" +create_review "$REVIEW_B" "$PLAN_B" +touch -t 202601010004 "$REVIEW_B" +# Sentinel points to older plan-A (intentional multi-plan workflow) +echo "$PLAN_A" > "$CASE_DIR/.claude/plans/.last-reviewed" +run_test "Case 10: Sentinel trusts plan-A, newer plan-B exists → ALLOW" "$CASE_DIR" "allow" + +# ============================================================ +# Summary +# ============================================================ +echo "---" +echo "$PASS passed, $FAIL failed" +[ "$FAIL" -eq 0 ] diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 00000000..f079c80a --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,15 @@ +{ + "hooks": { + "PreToolUse": [ + { + "matcher": "ExitPlanMode", + "hooks": [ + { + "type": "command", + "command": "$CLAUDE_PROJECT_DIR/.claude/hooks/check-plan-review.sh" + } + ] + } + ] + } +} diff --git a/CLAUDE.md b/CLAUDE.md index e1d76280..333a9e83 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -642,6 +642,65 @@ When implementing features or fixes, follow this workflow to ensure quality and Address any warnings before proceeding. +### Plan Review Before Approval + +When writing a new plan file (via EnterPlanMode), update the sentinel so the hook tracks the correct plan: +```bash +echo "" > ~/.claude/plans/.last-reviewed +``` + +Before calling `ExitPlanMode`, offer the user an independent plan review. Use `AskUserQuestion`: + +- "Run review agent for independent feedback" (Recommended) +- "Present plan for approval as-is" + +If "present as-is" is chosen, ensure `~/.claude/plans` exists (`mkdir -p`), then write a minimal review marker to `~/.claude/plans/.review.md`: +```yaml +--- +plan: +reviewed_at: +verdict: "Skipped" +critical_count: 0 +medium_count: 0 +low_count: 0 +flags: [] +--- +Review skipped by user. +``` +Also write the plan path to `~/.claude/plans/.last-reviewed`. This satisfies the ExitPlanMode hook. + +If review is requested: + +1. **Spawn review agent**: Use the Task tool with `subagent_type: "general-purpose"`. Prompt the agent to: + - Read `.claude/commands/review-plan.md` and follow its Steps 2 through 5 + - Review the plan file at the current plan path + - Return ONLY the structured review output (from "## Overall Assessment" through "## Summary") — do not include the "## Plan Content" display + Do NOT specify a model — let the agent inherit the current model for review quality. + +2. **Display the review agent's output** in the conversation, preceded by the plan content (display the plan yourself for terminal reading). The user reads both here in the terminal — this is the primary reading surface. + +3. **Save to file**: Ensure `~/.claude/plans` exists (`mkdir -p`) before writing. Write the review output (from "## Overall Assessment" through "## Summary") to `~/.claude/plans/.review.md`, with YAML frontmatter (plan path, timestamp, verdict, issue counts). Also write the plan path to `~/.claude/plans/.last-reviewed`. If the review file write fails, report an error and stop — the hook requires this file. If the sentinel write fails, warn but continue (the hook falls back to `ls -t`). + +4. **Collect feedback**: Use `AskUserQuestion`: + - "Address all issues and re-review" (Recommended) + - "Address all issues and present for approval" + - "Let me specify which items to address or dismiss" + If the user selects the third option, they will provide free-form text with directives (e.g., "disagree with CRITICAL #2", "skip all LOW", "for MEDIUM #1, do X instead"). + +5. **Revise the plan** based on review feedback and user directives: + - CRITICAL: address unless user explicitly dismissed with justification + - MEDIUM: address unless user dismissed + - LOW: skip unless user explicitly requested + - Checklist gaps: add missing items as plan steps + - Append a `## Revision Notes` section documenting what was addressed, dismissed, and why + - After writing the revised plan, touch the review file to update its mtime (`touch "$REVIEW_FILE"`) so the hook's staleness check passes after the intentional revision + +6. **Loop or exit**: If user chose "re-review", go back to step 1 (spawn a fresh review agent for the revised plan). Otherwise, call `ExitPlanMode`. + +If `ExitPlanMode` is blocked by the plan review hook (no `.review.md` found), follow the steps above to run a review before retrying. + +**Rollback**: To remove the plan review workflow, delete this subsection from CLAUDE.md, remove the `PreToolUse` entry from `.claude/settings.json`, and delete `.claude/hooks/check-plan-review.sh`. + ### Phase 4: Submit - Use `/submit-pr` to create the PR - Automated AI review will run additional methodology and edge case checks