fix(hook): skip re-review of a plan already decided this session

[rNoz]

Summary

Closes #1075.

A plan review re-opened for a plan already decided in the same session, in two ways:

1. ExitPlanMode (Claude): an identical plan submitted twice opened two separate reviews. The plan text was read straight from event.tool_input.plan and handed to startPlannotatorServer with no dedup.
2. Stop (Codex): the Stop hook scrapes the transcript for the latest plan every turn. A turn that proposes no new plan (for example one that only wrote a file) still finds the previous, already-approved plan as the latest one, so it is reviewed again. The plan-candidate collection only guarded within a turn, not across a turn boundary.

Approach

The two triggers are fixed differently, because they need different guarantees.

For Claude ExitPlanMode, a small store records an approval bound to the specific transcript occurrence (the exact ExitPlanMode tool-use entry), not just the plan text, using the repo's existing rewind-aware active-branch resolver, with a 5-minute TTL. A retry of that same still-active occurrence re-emits the prior allow decision together with a visible systemMessage so the reuse is never silent; a plan resubmitted after /rewind is a new occurrence and always gets a fresh review, even if the text is byte-identical. Only approvals are ever recorded or replayed; a denial always opens a fresh review on resubmission.

For Codex Stop, the fix is at the source rather than a replay layer: the transcript scraper that collects plan candidates now applies a turn-identity boundary, so a bookkeeping-only turn can no longer resurface an already-decided plan proposed in an earlier turn. There is no decision store and nothing is replayed on this path. When a candidate's turn identity cannot be determined, it is dropped rather than guessed at (fail closed), with a PLANNOTATOR_DEBUG-gated log line recording why, so that narrow case is diagnosable rather than silent.

Design choices

• Configurable, default on: the planDecisionReuse setting (PLANNOTATOR_PLAN_DECISION_REUSE env or config.json) gates the Claude-side occurrence-bound reuse. It defaults ON so the reported bug is fixed without configuration, while anyone who prefers to always re-review can disable it.
• Occurrence, not content, is the key on the Claude path: the same plan text resubmitted as a genuinely new proposal (for example after /rewind) is a different occurrence and always reviews fresh.
• Approvals only: a denial is never recorded or replayed on the Claude path, so a plan denied by mistake can always be reconsidered by resubmitting it.
• The Codex fix has no configuration knob of its own; it corrects the existing turn-scoped candidate collection so it behaves like the rest of that collection already does.
• Gemini plans are file-based with a different identity model and are excluded.
• Storage is best-effort: any I/O failure is swallowed so plan review is never blocked by the store.

Validation

189 targeted tests pass, covering the occurrence-bound store (record/read, TTL boundary, rewind and compact handling, fail-open on a missing or corrupt record), the Codex turn-identity gate, and the config resolver, along with a full build and the full hook test suite (208 tests) after rebasing onto current main.

Compatibility

Additive. A first-time plan review is unchanged. Only an exact resubmission of the same still-active occurrence within the same session is affected on the Claude path, and a Codex bookkeeping-only turn no longer resurfaces a stale plan. No public API, endpoint, or payload changes; the store lives under the existing data dir.

Related work

Thanks to @jpvarbed for filing #1075 and independently exploring a similar per-session decision-recording approach on a fork branch; that discussion informed the direction taken here.

[backnotprop]

Deep review done, including end-to-end reproduction from a worktree with an isolated data dir. The store itself is careful work: fail-open is real (missing file, corrupt JSON, empty session_id all fall through to a fresh review, verified), the config resolver matches house convention with garbage values disabling the feature, hashing makes path-shaped session ids safe, and 7-day pruning bounds growth. But the risk lives at the two call sites, not in the store, and that is where this needs changes.

Blocking

1. An approval replay survives /rewind and /compact. The store keys on session_id, and this repo's own session-log docs state that /rewind writes nothing (entries re-parent, orphaning the rest) and a /compact boundary is a new root in the SAME transcript. Same transcript means same session_id means the store still hits. Reproduced: seed an approval, pipe a byte-identical ExitPlanMode event, and the hook instantly emits behavior allow with no review UI, no browser, no user-visible output. The nightmare sequence is: approve plan P, regret it, /rewind to change course, agent re-proposes identical P, silent auto-approve. The codebase already has machinery for ignoring rewind-orphaned branches (resolveActiveBranchIndices); the store has no equivalent, and no TTL beyond 7-day file retention, so a resumed session can replay a days-old approval.

2. Content-only keys cannot distinguish a stale re-scrape from a genuine re-proposal. Those are byte-identical inputs with opposite correct answers. And every replay is silent: no systemMessage, nothing on stderr, no plans-archive snapshot. Minimum: emit a systemMessage so the human sees the gate was auto-answered. Better: bind replay to the same submission occurrence, not the same bytes (for Claude, no intervening user turn or a short TTL in minutes; for Codex, record the rollout entry index and skip only candidates at or before it).

3. The Codex half replays denials with no human in the loop, and targets a case that mostly does not exist. The replayed block carries the original feedback verbatim and unlabeled, so the agent believes the human freshly rejected its current turn; if the agent's position is that P is what it wants, it can never reach a human again that session. Probing getLatestCodexPlan with realistic fixtures: turns with task_started markers already exclude the old plan, and Plan items are already turn-filtered. The one shape that re-scrapes is an assistant proposed_plan block, because getAssistantProposedPlanText candidates are NOT turn-filtered while plan items are (codex-session.ts:270-305). Fixing the turn filter there is tighter than a decision store and carries none of the replay risk.

Fix before merge regardless of direction: plan-decision-store.ts contains a literal NUL byte (the separator at line 44 is a raw 0x00, not an escape). Verified consequences: git shows the file as Binary (the PR diff literally says Bin 0 -> 4151 bytes, so nobody can review it as a diff), grep needs -a, and rg does not list the file's own definitions. Use \u0000 in the template literal.

Should-fix, if the store direction survives: label replayed feedback (e.g. "Previously requested changes (not re-reviewed): ..."); Claude-path denials are recorded with user feedback but never read (write-only prose on disk); a malformed record on the Codex path currently becomes a fabricated "Plan changes requested" denial rather than being treated as absent (validate decision === approved|denied); the 10 new tests only cover the store in isolation, with nothing on approve-then-resubmit, deny-then-resubmit, corrupt-store-reopens, or session boundaries (I hand-verified several of those; they should be CI regression tests); the env-variables reference page is missing the new knob and AGENTS.md does not say it is hook-runtimes-only (OpenCode and Pi are unaffected); the normalizer duplicates codex-session's normalizePlan and the comment claiming it mirrors version-history dedup is inaccurate (saveToHistory compares exact strings).

Honest bottom line: the annoyance in #1075 is real, but the Claude half needs an occurrence/freshness bound plus a visible replay signal before auto-answering a human gate, and the Codex half is better fixed at collectPlanCandidates' turn filtering than by replaying denials. Happy to talk through the occurrence-binding design if useful, and to re-review quickly on push.

[rNoz]

Pushed the review fixes. Claude reuse is visible and bound to one active submission occurrence. Codex now filters stale plans by turn. Checks are passing.

[backnotprop]

Re-verified f2c5bab in depth, including 10 independent probes and end-to-end runs through the real hook binary. This redesign is exactly right and all three blockers are resolved:

• Approvals only, occurrence-keyed, 5-minute TTL. The store now records only approvals, bound to the specific live ExitPlanMode transcript entry via the repo's existing rewind-aware branch resolver. Reproduced end to end: a same-occurrence retry replays WITH the visible signal (top-level systemMessage sibling of hookSpecificOutput, correct per the hooks schema), and a rewound byte-identical plan opens a fresh review. Compact boundaries, intervening turns, TTL boundary plus-minus 1ms, backward clock skew, stale tool_use_id, garbage transcripts, legacy v1 store records: every probe resolves to a fresh review. Content alone no longer authorizes anything, and denials are never persisted on either path.
• Codex fixed at the source. The turn-identity gate in collectPlanCandidates kills the stale re-scrape (my original three fixtures now behave correctly) and the Codex block in index.ts is byte-identical to main: no store, no replay. The normalizer is properly shared now.
• NUL byte gone (file is text to git and rg again), plus a regression test asserting no raw NUL in the source, which is a nice touch. Docs added to the env-variables reference with the hook-runtime-only scoping stated. Malformed and legacy records are ignored cleanly. Temp-file leak fixed. Full suite 2621 pass, typecheck clean.

Two small asks before merge:

1. Missing turn identity now silently disables Codex plan review. getAssistantProposedPlanText returns null when the Stop payload lacks turn_id or the rollout has no id-carrying turn marker; on a Stop hook, no output means the turn just ends and the user silently loses the review. It is narrow (turn_id has shipped since Add Codex Stop-hook plan review #577) but it lands exactly on the fallback shapes most likely to have messy markers, and there is no debug breadcrumb at any of the Stop-path early exits. Add a PLANNOTATOR_DEBUG log line when a candidate is dropped for lack of turn identity, and if you can, confirm once against a real Codex Stop payload.
2. One unverified assumption worth a single real-session check: the feature only engages if the ExitPlanMode tool_use entry is already in the transcript when the PermissionRequest hook fires. If it is not, occurrence is null and the feature quietly no-ops (fail-open, correct direction, but the Plan review re-opens for a plan already decided in the same session #1075 annoyance would return). A quick confirmation in a live Claude session that reuse actually triggers would close the loop.

Benign residuals, no action needed: a turn boundary without an id inherits the previous activeTurnId (extra review, safe direction); concurrent hook writes can lose a record (extra review, verified with 8 parallel writers, no temp leaks); future record-shape changes invalidate old records via the strict two-key check (safe, worth a comment).

[rNoz]

Rebased onto current main and resolved the environment reference conflict. Added debug-only breadcrumbs for unsafe Codex Stop skips. Local rollout markers and Claude occurrence timing were also checked.

[backnotprop]

Verified the final push directly: the PLANNOTATOR_DEBUG breadcrumb covers both skip shapes (missing Stop payload turn_id, missing id-carrying rollout marker) with distinct messages on stderr and a clean exit, exactly as asked, and the rebase onto current main resolved cleanly. All 208 hook tests pass on the head. This closes out both asks from the re-review; merge-ready from my side once the post-rebase CI finishes.

[jpvarbed]

Thanks for fixing this! This looks like it addresses both cases from my issue without skipping reviews for genuinely new plans.
Appreciate it!

[backnotprop]

Thanks for confirming both cases, that closes the loop on the report. This is queued to merge.