From b2b7c2a8984f74ae8b008830203fb500186ea9c4 Mon Sep 17 00:00:00 2001 From: igerber Date: Sun, 15 Feb 2026 12:06:58 -0500 Subject: [PATCH 1/8] Add plan review workflow with hook enforcement and revise-plan skill Introduce an automated plan review gate before ExitPlanMode: - review-plan.md: Add Step 6 to persist reviews as .review.md files with YAML frontmatter; support reading prior review files for --updated delta assessment; add issue numbering for cross-referencing with /revise-plan - revise-plan.md: New skill to read review feedback, display it, collect user overrides, and revise the plan in plan mode - check-plan-review.sh: PreToolUse hook that blocks ExitPlanMode unless a .review.md sibling exists for the current plan (sentinel + fallback) - settings.json: Register the hook - CLAUDE.md: Document the in-plan-mode review workflow with review/revise loop, skip-with-marker option, and rollback instructions Co-Authored-By: Claude Opus 4.6 --- .claude/commands/review-plan.md | 64 ++++++++-- .claude/commands/revise-plan.md | 186 +++++++++++++++++++++++++++++ .claude/hooks/check-plan-review.sh | 67 +++++++++++ .claude/settings.json | 15 +++ CLAUDE.md | 58 +++++++++ 5 files changed, 383 insertions(+), 7 deletions(-) create mode 100644 .claude/commands/revise-plan.md create mode 100755 .claude/hooks/check-plan-review.sh create mode 100644 .claude/settings.json diff --git a/.claude/commands/review-plan.md b/.claude/commands/review-plan.md index e57b40bc..78d3499f 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 (`.review.md`) and the sentinel (`~/.claude/plans/.last-reviewed`), both alongside the plan file. - **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. The Write tool may only be used for the review output file and sentinel. +- 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 `.review.md` (sibling to the plan file — replace the trailing `.md` with `.review.md`). 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 @@ -294,7 +296,7 @@ Use judgment, not just substring matching — the plan may use different words t ### 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 +404,59 @@ 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. + +1. **Derive the review file path**: Replace the trailing `.md` in the plan file path with `.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 `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. **Handle write failure gracefully**: If either write fails (permissions, directory doesn't exist, etc.), emit a warning in the conversation but do NOT fail the review. The conversation output is the primary artifact. + +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 outside the repo: the review output (`.review.md`) and a sentinel (`~/.claude/plans/.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 diff --git a/.claude/commands/revise-plan.md b/.claude/commands/revise-plan.md new file mode 100644 index 00000000..11139d82 --- /dev/null +++ b/.claude/commands/revise-plan.md @@ -0,0 +1,186 @@ +--- +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 by replacing the trailing `.md` in the plan file path with `.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 the COMPLETE structured review output (from "## Overall Assessment" through "## Summary") + ``` +- Save the agent's output to the review file path with YAML frontmatter (see Change 1, Step 6 for format) +- Write the plan path to `~/.claude/plans/.last-reviewed` +- Proceed to Step 3 with the review content + +If "Skip review" is chosen: Skip to Step 6 with no review context. + +### Step 3: Display Review in Terminal + +Display the full review content in the conversation (excluding YAML frontmatter). This is the primary reading surface — the user reads the review here, not by opening the file. + +``` +## Plan Review for + + + +--- +Source: +``` + +### Step 4: Staleness Check + +Compare file modification times: +```bash +[ -nt ] && 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. **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..cd873df2 --- /dev/null +++ b/.claude/hooks/check-plan-review.sh @@ -0,0 +1,67 @@ +#!/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 sibling .review.md — deny if missing +# +# Known limitations: +# - The ls -t fallback (step 3) can pick the wrong plan if multiple files exist. +# - A stale sentinel from a prior session can allow a new plan through unreviewed. +# The CLAUDE.md guidance mitigates both by updating the sentinel on new plan creation. +# - printf %s in deny() does not escape quotes/backslashes in $1. Plan file paths +# almost never contain these characters. If needed later, add sanitization: +# MSG=$(echo "$1" | sed 's/"/\\"/g') +# +# 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 +} + +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 + REVIEW_FILE="${PLAN_FILE%.md}.review.md" + if [ -f "$REVIEW_FILE" ]; then + exit 0 # Review exists, 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: Check for review +REVIEW_FILE="${PLAN_FILE%.md}.review.md" + +if [ -f "$REVIEW_FILE" ]; then + exit 0 # Review exists, allow +else + deny "No plan review found. Expected: $REVIEW_FILE. Follow the Plan Review Before Approval instructions in CLAUDE.md." +fi 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..b2ec05c7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -642,6 +642,64 @@ 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, write a minimal review marker to `.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 the complete structured review output + Do NOT specify a model — let the agent inherit the current model for review quality. + +2. **Display the review** in the conversation. The user reads the review here in the terminal — this is the primary reading surface. + +3. **Save to file**: Write the review to `.review.md` alongside the plan file, with YAML frontmatter (plan path, timestamp, verdict, issue counts). Also write the plan path to `~/.claude/plans/.last-reviewed`. If write fails, warn but continue — the terminal display is what matters. + +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 + +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 From 45a95dcae645ed5f03716e4a872524bcbfa69a75 Mon Sep 17 00:00:00 2001 From: igerber Date: Sun, 15 Feb 2026 13:00:43 -0500 Subject: [PATCH 2/8] Display plan content alongside review output in all review surfaces Add Step 4b to review-plan.md to display the full plan before the structured review. Update revise-plan.md Step 3 to show both the plan and review together. Clarify in CLAUDE.md that the review agent's output includes the plan content via Step 4b. Co-Authored-By: Claude Opus 4.6 --- .claude/commands/review-plan.md | 24 ++++++++++++--- .claude/commands/revise-plan.md | 48 ++++++++++++++++++++++++------ .claude/hooks/check-plan-review.sh | 28 +++++++++++++---- CLAUDE.md | 6 ++-- 4 files changed, 84 insertions(+), 22 deletions(-) diff --git a/.claude/commands/review-plan.md b/.claude/commands/review-plan.md index 78d3499f..3059c7f4 100644 --- a/.claude/commands/review-plan.md +++ b/.claude/commands/review-plan.md @@ -41,7 +41,7 @@ Options: ## Constraints -- **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 (`.review.md`) and the sentinel (`~/.claude/plans/.last-reviewed`), both alongside the plan file. +- **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 Edit, NotebookEdit, or file-modifying Bash commands. The Write tool may only be used for the review output file and sentinel. @@ -69,7 +69,7 @@ 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? -Additionally, check if a prior review file exists at `.review.md` (sibling to the plan file — replace the trailing `.md` with `.review.md`). 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. +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." @@ -294,6 +294,22 @@ 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. 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. @@ -408,7 +424,7 @@ The `--pr` URL must be the same across the initial review and the `--updated` re After displaying the review in the conversation (Step 5), persist it to a file alongside the plan. -1. **Derive the review file path**: Replace the trailing `.md` in the plan file path with `.review.md`. +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 @@ -452,7 +468,7 @@ After displaying the review in the conversation (Step 5), persist it to a file a ## Notes -- This skill is read-only for project files — it writes two files outside the repo: the review output (`.review.md`) and a sentinel (`~/.claude/plans/.last-reviewed`) +- 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 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 diff --git a/.claude/commands/revise-plan.md b/.claude/commands/revise-plan.md index 11139d82..223ec4fe 100644 --- a/.claude/commands/revise-plan.md +++ b/.claude/commands/revise-plan.md @@ -48,7 +48,7 @@ Verify the plan file exists by reading it. If it doesn't exist, report the error ### Step 2: Locate and Read Review File -Derive the review file path by replacing the trailing `.md` in the plan file path with `.review.md`. +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. @@ -71,16 +71,45 @@ If "Run a review now" is chosen: - Write the plan path to `~/.claude/plans/.last-reviewed` - Proceed to Step 3 with the review content -If "Skip review" is chosen: Skip to Step 6 with no review context. +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 + - 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. + ``` + - In `## Revision Notes`, record: "Review skipped — revision based on user notes only" + - All issue counts are zero in the Addressed/Dismissed/Open sections + +### 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. -### Step 3: Display Review in Terminal +``` +## Plan: -Display the full review content in the conversation (excluding YAML frontmatter). This is the primary reading surface — the user reads the review here, not by opening the file. + -``` -## Plan Review for +--- + +## Review for - + --- Source: @@ -88,9 +117,10 @@ Source: ### Step 4: Staleness Check -Compare file modification times: +Compare file modification times. The review file is always in `~/.claude/plans/`: ```bash -[ -nt ] && echo "STALE" || echo "FRESH" +REVIEW_PATH="$HOME/.claude/plans/$(basename .md).review.md" +[ -nt "$REVIEW_PATH" ] && echo "STALE" || echo "FRESH" ``` If the plan is newer than the review, warn: diff --git a/.claude/hooks/check-plan-review.sh b/.claude/hooks/check-plan-review.sh index cd873df2..ad463928 100755 --- a/.claude/hooks/check-plan-review.sh +++ b/.claude/hooks/check-plan-review.sh @@ -10,7 +10,8 @@ # 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 sibling .review.md — deny if missing +# 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. @@ -19,6 +20,12 @@ # - printf %s in deny() does not escape quotes/backslashes in $1. Plan file paths # almost never contain these characters. If needed later, add sanitization: # MSG=$(echo "$1" | sed 's/"/\\"/g') +# - 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). @@ -40,9 +47,14 @@ if [ -f "$SENTINEL" ]; then # Expand ~ if present PLAN_FILE="${PLAN_FILE/#\~/$HOME}" if [ -n "$PLAN_FILE" ] && [ -f "$PLAN_FILE" ]; then - REVIEW_FILE="${PLAN_FILE%.md}.review.md" + PLAN_BASENAME=$(basename "$PLAN_FILE") + REVIEW_FILE="$PLANS_DIR/${PLAN_BASENAME%.md}.review.md" if [ -f "$REVIEW_FILE" ]; then - exit 0 # Review exists, allow + # 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 @@ -57,11 +69,15 @@ if [ -z "$PLAN_FILE" ]; then exit 0 fi -# Step 4: Check for review -REVIEW_FILE="${PLAN_FILE%.md}.review.md" +# 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 - exit 0 # Review exists, allow + 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 diff --git a/CLAUDE.md b/CLAUDE.md index b2ec05c7..087c1463 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -654,7 +654,7 @@ Before calling `ExitPlanMode`, offer the user an independent plan review. Use `A - "Run review agent for independent feedback" (Recommended) - "Present plan for approval as-is" -If "present as-is" is chosen, write a minimal review marker to `.review.md`: +If "present as-is" is chosen, write a minimal review marker to `~/.claude/plans/.review.md`: ```yaml --- plan: @@ -677,9 +677,9 @@ If review is requested: - Return the complete structured review output Do NOT specify a model — let the agent inherit the current model for review quality. -2. **Display the review** in the conversation. The user reads the review here in the terminal — this is the primary reading surface. +2. **Display the review agent's output** in the conversation. The output includes the plan content (via Step 4b of review-plan.md) followed by the structured review. The user reads both here in the terminal — this is the primary reading surface. -3. **Save to file**: Write the review to `.review.md` alongside the plan file, with YAML frontmatter (plan path, timestamp, verdict, issue counts). Also write the plan path to `~/.claude/plans/.last-reviewed`. If write fails, warn but continue — the terminal display is what matters. +3. **Save to file**: Write the review 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 write fails, warn but continue — the terminal display is what matters. 4. **Collect feedback**: Use `AskUserQuestion`: - "Address all issues and re-review" (Recommended) From ff5ad9afa71d07197e7aa0131b49cf23333cafbc Mon Sep 17 00:00:00 2001 From: igerber Date: Sun, 15 Feb 2026 17:11:48 -0500 Subject: [PATCH 3/8] Address AI review feedback: fix sentinel write, add plan-field validation Fix P1 bug where skip-review path in revise-plan.md didn't write the .last-reviewed sentinel, allowing stale sentinels to pass the hook. Add validate_review_plan_field() to check-plan-review.sh to verify the review file's YAML plan: field matches the expected plan path. Also fix dangling cross-reference, add verification checklist, and remove leftover plan text from review-plan.md. Co-Authored-By: Claude Opus 4.6 --- .claude/commands/review-plan.md | 2 -- .claude/commands/revise-plan.md | 3 ++- .claude/hooks/check-plan-review.sh | 29 +++++++++++++++++++++++++++++ 3 files changed, 31 insertions(+), 3 deletions(-) diff --git a/.claude/commands/review-plan.md b/.claude/commands/review-plan.md index 3059c7f4..e1417c88 100644 --- a/.claude/commands/review-plan.md +++ b/.claude/commands/review-plan.md @@ -486,5 +486,3 @@ After displaying the review in the conversation (Step 5), persist it to a file a 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 index 223ec4fe..00c48254 100644 --- a/.claude/commands/revise-plan.md +++ b/.claude/commands/revise-plan.md @@ -67,7 +67,7 @@ If "Run a review now" is chosen: 4. Number each issue sequentially within its severity section (CRITICAL #1, MEDIUM #1, etc.) 5. Return the COMPLETE structured review output (from "## Overall Assessment" through "## Summary") ``` -- Save the agent's output to the review file path with YAML frontmatter (see Change 1, Step 6 for format) +- Save the agent's output to the review file path with YAML frontmatter (see `.claude/commands/review-plan.md` Step 6 for format) - Write the plan path to `~/.claude/plans/.last-reviewed` - Proceed to Step 3 with the review content @@ -93,6 +93,7 @@ If "Skip review" is chosen: --- 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 diff --git a/.claude/hooks/check-plan-review.sh b/.claude/hooks/check-plan-review.sh index ad463928..93e6cdf1 100755 --- a/.claude/hooks/check-plan-review.sh +++ b/.claude/hooks/check-plan-review.sh @@ -38,6 +38,18 @@ deny() { 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 + yaml_plan=$(sed -n '/^---$/,/^---$/{ /^plan:/{ s/^plan:[[:space:]]*//; s/[[:space:]]*$//; s/^"//; s/"$//; s/^'"'"'//; s/'"'"'$//; p; q; } }' "$review_file") + # Expand ~ in the YAML value + yaml_plan="${yaml_plan/#\~/$HOME}" + [ "$yaml_plan" = "$expected" ] +} + PLANS_DIR="$HOME/.claude/plans" SENTINEL="$PLANS_DIR/.last-reviewed" @@ -50,6 +62,9 @@ if [ -f "$SENTINEL" ]; 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." @@ -74,6 +89,9 @@ 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 @@ -81,3 +99,14 @@ if [ -f "$REVIEW_FILE" ]; then 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) From a2f1a85c3e67ee289610e8f25b2673360c091571 Mon Sep 17 00:00:00 2001 From: igerber Date: Sun, 15 Feb 2026 17:41:51 -0500 Subject: [PATCH 4/8] Address second AI review: fix staleness gate, BSD sed, stale comment - Add review file touch after plan revision to prevent staleness gate from blocking the documented revise-plan flow (P1) - Clarify plan: field path-matching requirements in review-plan.md (P2) - Fix BSD sed incompatibility in validate_review_plan_field (already applied) - Remove stale escaping limitation comment now that deny() sanitizes (P3) Co-Authored-By: Claude Opus 4.6 --- .claude/commands/review-plan.md | 2 ++ .claude/commands/revise-plan.md | 6 +++++- .claude/hooks/check-plan-review.sh | 7 +++---- CLAUDE.md | 1 + 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/.claude/commands/review-plan.md b/.claude/commands/review-plan.md index e1417c88..7cbb0ad3 100644 --- a/.claude/commands/review-plan.md +++ b/.claude/commands/review-plan.md @@ -445,6 +445,8 @@ After displaying the review in the conversation (Step 5), persist it to a file a --- ``` + 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). diff --git a/.claude/commands/revise-plan.md b/.claude/commands/revise-plan.md index 00c48254..b0bee0ae 100644 --- a/.claude/commands/revise-plan.md +++ b/.claude/commands/revise-plan.md @@ -190,7 +190,11 @@ Call `EnterPlanMode` to transition into plan mode. In plan mode: - Question #1: ``` 5. **Write the revised plan** using the Edit or Write tool -6. **Call `ExitPlanMode`** for user approval +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 diff --git a/.claude/hooks/check-plan-review.sh b/.claude/hooks/check-plan-review.sh index 93e6cdf1..b8285d8b 100755 --- a/.claude/hooks/check-plan-review.sh +++ b/.claude/hooks/check-plan-review.sh @@ -17,9 +17,6 @@ # - The ls -t fallback (step 3) can pick the wrong plan if multiple files exist. # - A stale sentinel from a prior session can allow a new plan through unreviewed. # The CLAUDE.md guidance mitigates both by updating the sentinel on new plan creation. -# - printf %s in deny() does not escape quotes/backslashes in $1. Plan file paths -# almost never contain these characters. If needed later, add sanitization: -# MSG=$(echo "$1" | sed 's/"/\\"/g') # - 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. @@ -44,7 +41,9 @@ deny() { validate_review_plan_field() { local review_file="$1" expected="$2" local yaml_plan - yaml_plan=$(sed -n '/^---$/,/^---$/{ /^plan:/{ s/^plan:[[:space:]]*//; s/[[:space:]]*$//; s/^"//; s/"$//; s/^'"'"'//; s/'"'"'$//; p; q; } }' "$review_file") + # 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" ] diff --git a/CLAUDE.md b/CLAUDE.md index 087c1463..648cacc7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -693,6 +693,7 @@ If review is requested: - 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`. From 9fda24fd7aeaf6cb72ba022d828e8c2f373bd5c0 Mon Sep 17 00:00:00 2001 From: igerber Date: Sun, 15 Feb 2026 18:43:06 -0500 Subject: [PATCH 5/8] Address AI review: fix stale-sentinel bypass, quote paths in revise-plan - Add guard in check-plan-review.sh to detect when the sentinel points to an older plan but a newer unreviewed plan exists in the directory - Update Known limitations comment to reflect the new guard - Add verification checklist items 10-12 for stale-sentinel scenarios - Quote variable in revise-plan.md staleness-check example Co-Authored-By: Claude Opus 4.6 --- .claude/commands/revise-plan.md | 5 +++-- .claude/hooks/check-plan-review.sh | 21 +++++++++++++++++++-- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/.claude/commands/revise-plan.md b/.claude/commands/revise-plan.md index b0bee0ae..320da8ee 100644 --- a/.claude/commands/revise-plan.md +++ b/.claude/commands/revise-plan.md @@ -120,8 +120,9 @@ Source: Compare file modification times. The review file is always in `~/.claude/plans/`: ```bash -REVIEW_PATH="$HOME/.claude/plans/$(basename .md).review.md" -[ -nt "$REVIEW_PATH" ] && echo "STALE" || echo "FRESH" +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: diff --git a/.claude/hooks/check-plan-review.sh b/.claude/hooks/check-plan-review.sh index b8285d8b..41c37e39 100755 --- a/.claude/hooks/check-plan-review.sh +++ b/.claude/hooks/check-plan-review.sh @@ -15,8 +15,8 @@ # # Known limitations: # - The ls -t fallback (step 3) can pick the wrong plan if multiple files exist. -# - A stale sentinel from a prior session can allow a new plan through unreviewed. -# The CLAUDE.md guidance mitigates both by updating the sentinel on new plan creation. +# - A stale sentinel from a prior session is caught by comparing against the newest +# plan in the directory, but relies on filesystem mtime for the comparison. # - 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. @@ -58,6 +58,20 @@ if [ -f "$SENTINEL" ]; then # Expand ~ if present PLAN_FILE="${PLAN_FILE/#\~/$HOME}" if [ -n "$PLAN_FILE" ] && [ -f "$PLAN_FILE" ]; then + # Guard: deny if a newer unreviewed plan exists than the sentinel references + NEWEST_PLAN=$(ls -t "$PLANS_DIR"/*.md 2>/dev/null | grep -v '\.review\.md$' | head -1) + if [ -n "$NEWEST_PLAN" ] && [ "$NEWEST_PLAN" != "$PLAN_FILE" ]; then + # A different, newer plan exists. Check if it has a valid review. + NEWEST_BASENAME=$(basename "$NEWEST_PLAN") + NEWEST_REVIEW="$PLANS_DIR/${NEWEST_BASENAME%.md}.review.md" + if [ ! -f "$NEWEST_REVIEW" ] || [ "$NEWEST_PLAN" -nt "$NEWEST_REVIEW" ]; then + deny "Sentinel (.last-reviewed) points to $(basename "$PLAN_FILE"), but a newer unreviewed plan exists: $NEWEST_BASENAME. Review it or update the sentinel." + fi + if ! validate_review_plan_field "$NEWEST_REVIEW" "$NEWEST_PLAN"; then + deny "Sentinel (.last-reviewed) points to $(basename "$PLAN_FILE"), and $NEWEST_BASENAME has a review file but it belongs to a different plan. Re-run the review or update the sentinel." + fi + fi + PLAN_BASENAME=$(basename "$PLAN_FILE") REVIEW_FILE="$PLANS_DIR/${PLAN_BASENAME%.md}.review.md" if [ -f "$REVIEW_FILE" ]; then @@ -109,3 +123,6 @@ fi # 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) +# 10. Sentinel points to older plan, newer plan exists with valid review: ALLOW +# 11. Sentinel points to older plan, newer plan exists without review: DENY +# 12. Sentinel points to older plan, newer plan has review for wrong plan: DENY From cb8d5e3e792fc73c8fa156bd86184b04baaa4452 Mon Sep 17 00:00:00 2001 From: igerber Date: Sun, 15 Feb 2026 19:08:05 -0500 Subject: [PATCH 6/8] Address third AI review: hard-fail on review write, remove newer-plan guard - Make review file write a hard requirement (review-plan.md, revise-plan.md, CLAUDE.md): if the write fails, report an error and stop instead of warning. Add mkdir -p before writes to prevent missing-directory failures. - Remove overly aggressive "newer plan exists" guard from check-plan-review.sh that caused false denials in multi-plan workflows. Revert Known Limitations comment to original form and remove related checklist items. Co-Authored-By: Claude Opus 4.6 --- .claude/commands/review-plan.md | 13 ++++++++++++- .claude/commands/revise-plan.md | 2 ++ .claude/hooks/check-plan-review.sh | 21 ++------------------- CLAUDE.md | 4 ++-- 4 files changed, 18 insertions(+), 22 deletions(-) diff --git a/.claude/commands/review-plan.md b/.claude/commands/review-plan.md index 7cbb0ad3..6fbfe081 100644 --- a/.claude/commands/review-plan.md +++ b/.claude/commands/review-plan.md @@ -424,6 +424,11 @@ The `--pr` URL must be the same across the initial review and the `--updated` re 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**: @@ -459,7 +464,13 @@ After displaying the review in the conversation (Step 5), persist it to a file a ``` This sentinel is read by the ExitPlanMode hook to identify which plan was most recently reviewed. -6. **Handle write failure gracefully**: If either write fails (permissions, directory doesn't exist, etc.), emit a warning in the conversation but do NOT fail the review. The conversation output is the primary artifact. +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: ``` diff --git a/.claude/commands/revise-plan.md b/.claude/commands/revise-plan.md index 320da8ee..7dc39348 100644 --- a/.claude/commands/revise-plan.md +++ b/.claude/commands/revise-plan.md @@ -80,6 +80,7 @@ If "Skip review" is chosen: - 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 --- @@ -96,6 +97,7 @@ If "Skip review" is chosen: - 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 diff --git a/.claude/hooks/check-plan-review.sh b/.claude/hooks/check-plan-review.sh index 41c37e39..ac090181 100755 --- a/.claude/hooks/check-plan-review.sh +++ b/.claude/hooks/check-plan-review.sh @@ -15,8 +15,8 @@ # # Known limitations: # - The ls -t fallback (step 3) can pick the wrong plan if multiple files exist. -# - A stale sentinel from a prior session is caught by comparing against the newest -# plan in the directory, but relies on filesystem mtime for the comparison. +# - A stale sentinel from a prior session can allow a new plan through unreviewed. +# The CLAUDE.md guidance mitigates both by updating the sentinel on new plan creation. # - 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. @@ -58,20 +58,6 @@ if [ -f "$SENTINEL" ]; then # Expand ~ if present PLAN_FILE="${PLAN_FILE/#\~/$HOME}" if [ -n "$PLAN_FILE" ] && [ -f "$PLAN_FILE" ]; then - # Guard: deny if a newer unreviewed plan exists than the sentinel references - NEWEST_PLAN=$(ls -t "$PLANS_DIR"/*.md 2>/dev/null | grep -v '\.review\.md$' | head -1) - if [ -n "$NEWEST_PLAN" ] && [ "$NEWEST_PLAN" != "$PLAN_FILE" ]; then - # A different, newer plan exists. Check if it has a valid review. - NEWEST_BASENAME=$(basename "$NEWEST_PLAN") - NEWEST_REVIEW="$PLANS_DIR/${NEWEST_BASENAME%.md}.review.md" - if [ ! -f "$NEWEST_REVIEW" ] || [ "$NEWEST_PLAN" -nt "$NEWEST_REVIEW" ]; then - deny "Sentinel (.last-reviewed) points to $(basename "$PLAN_FILE"), but a newer unreviewed plan exists: $NEWEST_BASENAME. Review it or update the sentinel." - fi - if ! validate_review_plan_field "$NEWEST_REVIEW" "$NEWEST_PLAN"; then - deny "Sentinel (.last-reviewed) points to $(basename "$PLAN_FILE"), and $NEWEST_BASENAME has a review file but it belongs to a different plan. Re-run the review or update the sentinel." - fi - fi - PLAN_BASENAME=$(basename "$PLAN_FILE") REVIEW_FILE="$PLANS_DIR/${PLAN_BASENAME%.md}.review.md" if [ -f "$REVIEW_FILE" ]; then @@ -123,6 +109,3 @@ fi # 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) -# 10. Sentinel points to older plan, newer plan exists with valid review: ALLOW -# 11. Sentinel points to older plan, newer plan exists without review: DENY -# 12. Sentinel points to older plan, newer plan has review for wrong plan: DENY diff --git a/CLAUDE.md b/CLAUDE.md index 648cacc7..521f5ebb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -654,7 +654,7 @@ Before calling `ExitPlanMode`, offer the user an independent plan review. Use `A - "Run review agent for independent feedback" (Recommended) - "Present plan for approval as-is" -If "present as-is" is chosen, write a minimal review marker to `~/.claude/plans/.review.md`: +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: @@ -679,7 +679,7 @@ If review is requested: 2. **Display the review agent's output** in the conversation. The output includes the plan content (via Step 4b of review-plan.md) followed by the structured review. The user reads both here in the terminal — this is the primary reading surface. -3. **Save to file**: Write the review 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 write fails, warn but continue — the terminal display is what matters. +3. **Save to file**: Ensure `~/.claude/plans` exists (`mkdir -p`) before writing. Write the review 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) From 1376aa7f4a0a935c8097cadf5da92d58cefde1fa Mon Sep 17 00:00:00 2001 From: igerber Date: Sun, 15 Feb 2026 19:38:34 -0500 Subject: [PATCH 7/8] Address fourth AI review: prevent plan content leaking into review files, scope constraint to project files Co-Authored-By: Claude Opus 4.6 --- .claude/commands/review-plan.md | 2 +- .claude/commands/revise-plan.md | 4 ++-- CLAUDE.md | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.claude/commands/review-plan.md b/.claude/commands/review-plan.md index 6fbfe081..2f7b4c65 100644 --- a/.claude/commands/review-plan.md +++ b/.claude/commands/review-plan.md @@ -44,7 +44,7 @@ Options: - **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 Edit, NotebookEdit, or file-modifying Bash commands. The Write tool may only be used for the review output file and sentinel. +- 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 diff --git a/.claude/commands/revise-plan.md b/.claude/commands/revise-plan.md index 7dc39348..14da23c8 100644 --- a/.claude/commands/revise-plan.md +++ b/.claude/commands/revise-plan.md @@ -65,9 +65,9 @@ If "Run a review now" is chosen: 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 the COMPLETE structured review output (from "## Overall Assessment" through "## Summary") + 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 output to the review file path with YAML frontmatter (see `.claude/commands/review-plan.md` Step 6 for format) +- 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 diff --git a/CLAUDE.md b/CLAUDE.md index 521f5ebb..333a9e83 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -674,12 +674,12 @@ 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 the complete structured review output + - 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. The output includes the plan content (via Step 4b of review-plan.md) followed by the structured review. The user reads both here in the terminal — this is the primary reading surface. +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 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`). +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) From 4947a3196c10019670360320100463a7db98b07a Mon Sep 17 00:00:00 2001 From: igerber Date: Sun, 15 Feb 2026 20:10:02 -0500 Subject: [PATCH 8/8] Address fifth AI review: document stale-sentinel limitation, add automated hook tests Co-Authored-By: Claude Opus 4.6 --- .claude/hooks/check-plan-review.sh | 10 +- .claude/hooks/test-check-plan-review.sh | 206 ++++++++++++++++++++++++ 2 files changed, 214 insertions(+), 2 deletions(-) create mode 100644 .claude/hooks/test-check-plan-review.sh diff --git a/.claude/hooks/check-plan-review.sh b/.claude/hooks/check-plan-review.sh index ac090181..cf75edb3 100755 --- a/.claude/hooks/check-plan-review.sh +++ b/.claude/hooks/check-plan-review.sh @@ -15,8 +15,14 @@ # # Known limitations: # - The ls -t fallback (step 3) can pick the wrong plan if multiple files exist. -# - A stale sentinel from a prior session can allow a new plan through unreviewed. -# The CLAUDE.md guidance mitigates both by updating the sentinel on new plan creation. +# - 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. 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 ]