Skip to content

Add plan-review eval harness + content-hash plan-approval gate (workflow-eval step 1b) #716

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
igerber merged 7 commits into from
Jul 21, 2026
+7,198 −489
Merged

Add plan-review eval harness + content-hash plan-approval gate (workflow-eval step 1b) #716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
89cc78a
Plan-review eval harness + content-hash ExitPlanMode gate (workflow-e…
igerber Jul 20, 2026
d327533
Address CI review round 8: write-once campaigns, model-shaped redacti…
igerber Jul 20, 2026
bc2fe30
Address CI review round 9: bracketed wrapper execution, kind discrimi…
igerber Jul 20, 2026
080c937
Address CI review round 10: fingerprint whole case definitions, requi…
igerber Jul 20, 2026
b194463
Fix Pure Python Fallback CI leg: unique-name loader for reviewer-eval…
igerber Jul 20, 2026
1294333
Address CI review round 12: extract substantive question-phrased find…
igerber Jul 20, 2026
341e4f4
Guard the real-pin control-prompt test for shallow clones
igerber Jul 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 68 additions & 57 deletions .claude/commands/review-plan.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,22 +41,50 @@ 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 (`~/.claude/plans/<plan-stem>.review.md`) and the sentinel (`~/.claude/plans/.last-reviewed`), both in `~/.claude/plans/`.
- **Read-only for project files**: Do NOT create, edit, or delete any project files (source code, tests, documentation, configuration). The only PERSISTENT output is the review file (the helper-derived `review_path` in `~/.claude/plans/` — its filename carries a canonical-path digest). The workflow also uses invocation-scoped TEMPORARY files, which are allowed: the ingress/scratch files under `$(git rev-parse --git-path plan-review)` (inside `.git/`, never project content) and the helper-managed snapshot/state/meta/body files under `~/.claude/plans/.snapshots/` (the helper deletes them on persist and on abort).
- **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 on project files. The Write tool and `mkdir -p` may only target `~/.claude/plans/` (for the review output file, sentinel, and output directory).
- 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 target only `~/.claude/plans/` and the two temporary locations above.
- The `gh api` calls used with `--pr` are read-only API requests, consistent with the project-files read-only constraint.

## Instructions

### Step 1: Read the Plan File
### Step 1: Take an Immutable Snapshot of the Plan

Read the plan file at the path provided in `$ARGUMENTS`.
**The review must certify exactly the bytes it examined.** The whole
snapshot/verify/persist protocol lives in the tested helper
`.claude/scripts/plan_snapshot.py` (see `tests/test_plan_snapshot.py`) — the
raw plan path is UNTRUSTED and never touches a shell; it reaches the helper
via a file written with the Write tool:

If the file does not exist, report the error and stop:
```
Error: Plan file not found at <path>
```
1. Derive the scratch dir (one Bash call; deterministic, worktree-correct):
```bash
SCRATCH="$(git rev-parse --git-path plan-review)"
mkdir -p "$SCRATCH" && echo "$SCRATCH"
```
2. **Write the raw plan path** (exactly as supplied, `~` and all) to
`<scratch>/plan-path.txt` with the Write tool — never `echo`/heredoc.
3. **Run the helper** (re-derive `SCRATCH` in this call):
```bash
SCRATCH="$(git rev-parse --git-path plan-review)"
python3 .claude/scripts/plan_snapshot.py snapshot --plan-path-file "$SCRATCH/plan-path.txt"
```
It normalizes the path as data (`~` expansion, canonical realpath — any
absolute path is accepted; the path never touches a shell), reads the plan
bytes ONCE,
writes an invocation-unique immutable snapshot + state token, and prints
JSON: `state_path`, `snapshot_path`, `meta_path`, `body_path`, `plan_path`,
`plan_sha256`, `review_path`. Non-zero exit = invalid/unreadable path —
report its message and stop. **Confirm the printed `plan_path` is the plan
you supplied** (a concurrent session overwriting the ingress file is
thereby detected — if it differs, re-run from step 2).
4. **Read the SNAPSHOT file** (Read tool, the printed `snapshot_path`) — it is
the ONLY text this review examines. The state token keys the rest of the
protocol; Step 6's persist certifies the RECORDED snapshot digest only
after re-verifying the live plan against it. **If the review aborts for any
reason before persisting** (error, user cancellation), clean up the
invocation: `python3 .claude/scripts/plan_snapshot.py abort --state-file
"<state-path>"` — temporary snapshot files must not accumulate.

### Step 1b: Handle Re-Review (if `--updated`)

Expand All @@ -69,7 +97,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 `~/.claude/plans/<plan-basename>.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.
Additionally, check for a prior review via the Step 1 snapshot output: its `review_path` is the canonical location (review filenames carry a canonical-path digest — never derive them from the basename). If a file exists there, 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."

Expand Down Expand Up @@ -441,57 +469,40 @@ The `--pr` URL must be the same across the initial review and the `--updated` re

### 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
After displaying the review in the conversation (Step 5), persist it via the
helper — it re-verifies the live plan against the reviewed snapshot, builds the
frontmatter (setting `plan:` and `plan_sha256:` itself from the snapshot — the
caller cannot mis-stamp them), writes atomically, and cleans the snapshot up:

1. **Write the review body** (everything from "## Overall Assessment" through
"## Summary", exactly as displayed) to the exact `body_path` printed in
Step 1, with the Write tool (invocation-unique — concurrent reviews cannot
cross-wire inputs).

2. **Write the meta JSON** to the exact `meta_path` printed in Step 1, with
the Write tool:
```json
{"reviewed_at": "2026-02-15T14:30:00Z", "assessment": "Significant issues found",
"critical_count": 2, "medium_count": 3, "low_count": 1, "flags": ["--updated", "--pr"]}
```
(`reviewed_at` from `date -u +%Y-%m-%dT%H:%M:%SZ`; `flags` lists the CLI
flags active during this review — `"--updated"`, `"--pr"`, or `[]`.)

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**:
3. **Persist via the state token** — substitute the literal `state_path`
printed in Step 1 (helper-generated, safe charset):
```bash
date -u +%Y-%m-%dT%H:%M:%SZ
python3 .claude/scripts/plan_snapshot.py persist --state-file "<state-path>"
```

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"
assessment: "Significant issues found"
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 <review-file-path>.
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:
- **Exit 3** means the plan was modified during the review: the review was
NOT persisted (it examined the snapshot, and the live content is now
something else). Relay the helper's message and stop — re-run
/review-plan against the current plan. Do NOT proceed to the footer.
- Any other non-zero exit: report the message and stop (the review file is
required by the ExitPlanMode hook; a missing one blocks approval).
- On success it prints the review path (canonical — derived from the
realpath the hook also uses, so symlink aliases cannot split the key).

4. **Append a footer** to the conversation output:
```
---
Review saved to: <review-file-path>
Expand All @@ -500,7 +511,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 in `~/.claude/plans/`: the review output (`<plan-basename>.review.md`) and the sentinel (`.last-reviewed`)
- This skill is read-only for project files — its one persistent output is the review file at the helper-derived `review_path` (canonical basename + canonical-path digest, in `~/.claude/plans/`), whose `plan_sha256` frontmatter is what the ExitPlanMode hook validates against the plan's current content
- 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
Expand Down
102 changes: 71 additions & 31 deletions .claude/commands/revise-plan.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,27 +48,61 @@ 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: 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`.
Locate the review via the helper (review filenames carry a canonical-path
digest — NEVER derive them from the basename): first initialize the scratch
dir (it need not exist on a fresh worktree):
```bash
SCRATCH="$(git rev-parse --git-path plan-review)"
mkdir -p "$SCRATCH" && echo "$SCRATCH"
```
Write the raw plan path to `<scratch>/plan-path.txt` (Write tool), then `python3 .claude/scripts/plan_snapshot.py check --plan-path-file
"$SCRATCH/plan-path.txt"` — its JSON reports `plan_path`, `review_path`,
`review_exists`, and `fresh`. Confirm the printed `plan_path` is the plan you
supplied — the ingress file is shared per-worktree, so a concurrent session
may have overwritten it between your Write and the check; if it differs,
re-Write the path and re-run the check before using any of its output.

**If the review file exists**: Read it, proceed to Step 3.
**If `review_exists` is true**: Read the file at the printed `review_path`,
proceed to Step 3.

**If the review file does not exist**: Use AskUserQuestion:
**If `review_exists` is false**: 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:
- **First take an immutable snapshot via the tested helper** (the raw plan
path is untrusted — it reaches the helper via a file written with the Write
tool, never a shell): derive `SCRATCH="$(git rev-parse --git-path
plan-review)"` (mkdir -p it), Write the raw plan path to
`<scratch>/plan-path.txt`, then run
`python3 .claude/scripts/plan_snapshot.py snapshot --plan-path-file
"$SCRATCH/plan-path.txt"` — it prints JSON with `state_path`,
`snapshot_path`, `meta_path`, `body_path`, `plan_path`, `plan_sha256`,
`review_path`. Confirm the printed `plan_path` is the plan you supplied
(detects a concurrent ingress overwrite; re-run if not). Non-zero exit:
report and stop.
- Use the Task tool with `subagent_type: "general-purpose"`, pointing the agent
at the SNAPSHOT path (never the live plan). 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: <plan-path>
2. Read the plan file at: <snapshot-path> (an immutable snapshot of the plan)
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`
- After the agent returns: Write its review output (from "## Overall
Assessment" through "## Summary" — no plan content) to the exact `body_path`
printed by snapshot, Write the meta JSON (reviewed_at, assessment, counts,
`"flags": []`) to the exact `meta_path`, then run
`python3 .claude/scripts/plan_snapshot.py persist --state-file
"<state-path>"` (the literal printed path). Exit 3 = the plan changed while
being reviewed: NOT persisted — relay the message and re-run. The helper
certifies the recorded snapshot digest and sets `plan:`/`plan_sha256:`
itself. On any pre-persist failure or cancellation, run
`plan_snapshot.py abort --state-file "<state-path>"` so temporary files
never accumulate.
- Proceed to Step 3 with the review content

If "Skip review" is chosen:
Expand All @@ -80,21 +114,16 @@ 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/<plan-basename>.review.md` (the centralized review path from Change 3) before calling `ExitPlanMode` to satisfy the hook:
```yaml
---
plan: <plan-file-path>
reviewed_at: <ISO 8601 timestamp>
assessment: "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)
- Write a "Skipped" marker via the helper (it snapshots, hashes, and
stamps `plan:`/`plan_sha256:` itself): Write the raw plan path to
`<scratch>/plan-path.txt`, run `plan_snapshot.py snapshot
--plan-path-file ...` (as in Step 2 — confirm the printed `plan_path`
is the plan you supplied before using its output), Write
`{"reviewed_at": "<ISO 8601>", "assessment": "Skipped",
"critical_count": 0, "medium_count": 0, "low_count": 0, "flags": []}` to
the printed `meta_path`, Write `Review skipped by user.` to the printed
`body_path`, then run `plan_snapshot.py persist --state-file
"<state-path>"` before calling `ExitPlanMode` to satisfy the hook.
- 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.
Expand All @@ -120,14 +149,20 @@ Source: <review-file-path>

### Step 4: Staleness Check

Compare file modification times. The review file is always in `~/.claude/plans/`:
Compare the plan's content hash against the one recorded in the review file
via the helper's read-only probe (the raw path flows through file ingress —
never a shell): Write the raw plan path to `<scratch>/plan-path.txt`
(`SCRATCH="$(git rev-parse --git-path plan-review)"`), then:
```bash
PLAN_PATH="<plan-path>"
REVIEW_PATH="$HOME/.claude/plans/$(basename "$PLAN_PATH" .md).review.md"
[ "$PLAN_PATH" -nt "$REVIEW_PATH" ] && echo "STALE" || echo "FRESH"
SCRATCH="$(git rev-parse --git-path plan-review)"
python3 .claude/scripts/plan_snapshot.py check --plan-path-file "$SCRATCH/plan-path.txt"
```
Confirm the printed `plan_path` is the plan you supplied (shared per-worktree
ingress — a concurrent session may have overwritten it); if it differs,
re-Write the path and re-run the check.

If the plan is newer than the review, warn:
If the printed `fresh` is false (hash mismatch, or no `plan_sha256` recorded),
the review is STALE — it reviewed different plan content. Warn:
```
Warning: The plan file was modified after this review was generated.
The review may be commenting on an older version of the plan.
Expand Down Expand Up @@ -193,10 +228,15 @@ Call `EnterPlanMode` to transition into plan mode. In plan mode:
- Question #1: <question text — to be resolved during implementation>
```
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/<plan-basename>.review.md
```
6. **Re-review the revised plan** — never merely re-stamp the old review's hash
onto content it did not examine (that would recreate the old `touch` bypass
with better cosmetics). Spawn the review agent again (Step 2's "Run a review
now" flow) over the REVISED plan; that fresh review writes the new
`plan_sha256`. If the user explicitly declines a re-review, write a fresh
Skipped marker (Step 2's template, with the new hash) — its
`assessment: "Skipped"` accurately records that the revised content was not
independently reviewed. The ExitPlanMode hook denies on hash mismatch; there
is no mtime/touch bypass.
7. **Call `ExitPlanMode`** for user approval

### Step 8: Report
Expand Down
Loading
Loading