Add /plan-review command and plan-reviewer subagent - #9
Draft
kubiknyc wants to merge 2 commits into
Draft
Conversation
Reviewing a plan in the context that produced it is rationalization, not review — the model is anchored on its own reasoning and rationalizes. This adds the machinery to review a plan from fresh context instead, before any of it gets built. `plan-reviewer` is a read-only subagent that takes a lens (factual grounding, adversarial, or invariants) and returns a JSON verdict. It is told to default to refuted and to verify claims about existing code by opening the file rather than trusting the plan's description of it — a false premise about what already exists is the most common way a plan fails. It complements worklog-reviewer, which reviews diffs; this one runs while throwing the plan away is still cheap. `/plan-review` orchestrates: writes the plan and the original request to `.plan-review/`, fans out three reviewers in parallel with only those two files, then merges — dropping findings with no file and no failure scenario, and settling reviewer disagreements by reading the code rather than averaging. `.plan-review/` is gitignored; it also gives a later PreToolUse hook on ExitPlanMode a findings file to gate on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M7xXZbGTUPQGST1F3UUUkB
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Step 1 wrote the plan to `.plan-review/plan.md`, but plan mode is read-only — the command failed on its first action in the exact situation it exists for. The files were only ever a convenience; the reviewers just need the plan text. Fall back to passing plan and request inline when writes are blocked, and say explicitly not to leave plan mode to get around it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M7xXZbGTUPQGST1F3UUUkB
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds tooling to review an implementation plan from fresh context before it gets built. A plan reviewed inside the context that produced it is rationalization — the model is anchored on its own reasoning. Two new files: a
plan-reviewersubagent and a/plan-reviewslash command that fans it out.plan-reviewer(.claude/agents/plan-reviewer.md) is read-only (Read, Grep, Glob, Bash). It takes a lens —factual-grounding,adversarial, orinvariants— and returns a JSON verdict. It is told to default to refuted, to treat "I could not verify this" as a valid finding, and to check claims about existing code by opening the file rather than trusting the plan's description of it. It complementsworklog-reviewer, which reviews diffs; this one runs while throwing the plan away is still cheap. Theinvariantslens carries the plan-stage version of this repo's hard rules — OTA safety, SELECT-only report tables, schema parity, platform split, sync purity, themutationQueue.tscoverage pin, tests never underapp/, testID coverage, and open decisions indocs/architecture/00-README.md./plan-review(.claude/commands/plan-review.md) writes the plan and the original request to.plan-review/, spawns the three lenses in parallel with only those two file paths — deliberately withholding the author's reasoning — then merges: drops findings with neither a file nor a concrete failure scenario, settles reviewer disagreements by reading the code rather than averaging, and reports surviving assumptions before the user approves..plan-review/is gitignored. It also gives a futurePreToolUsehook onExitPlanModea findings file to gate on, if the advisory version turns out not to be enough.Milestone / plan task
None — tooling, not a milestone task.
Gates
npm run verifygreen locally — not run:node_modulesis absent in this container andnpm run typecheckfails atexpo/tsconfig.base, which is environmental, not caused by this diff. The change is outside everythingverifycovers: no TS, no tests, and.prettierignoreexcludes both.claude/and*.md, soformat:checkdoes not read either new file. CI will confirm.docs/architecture/00-README.md? → noOpen decisions touched
None.
Adversarial review
Not run —
worklog-reviewerchecksapp/,src/,scripts/, and.maestro/, and this diff touches none of them. Two agent-configuration markdown files and one.gitignoreline.One judgement call worth flagging for review: the three lenses are fixed in the command rather than being chosen per plan. That is deliberate — letting the author pick the lenses reintroduces the bias the fan-out exists to remove — but it means a plan whose real risk sits outside all three gets a clean verdict. The
unverifiablefield is the partial mitigation, and the merge step is instructed to treat it as amber rather than green.Generated by Claude Code