From eafe7f2ffccf32287739f787dbb595eaa02a104b Mon Sep 17 00:00:00 2001 From: rNoz Date: Fri, 31 Jul 2026 18:19:20 +0200 Subject: [PATCH 1/4] fix(hook): skip re-review of a plan already decided this session A plan review re-opened for a plan that was already decided (#1075) in two ways: an identical ExitPlanMode plan submitted twice opened two reviews, and a Codex Stop turn that did no planning still scraped the previous turn's already-decided plan and reviewed it again on every bookkeeping turn. saveToHistory already dedups identical resubmissions for version history, but nothing consulted a prior decision before opening a session. Add a per-session decision store (`plan-decision-store.ts`): plan outcomes are recorded keyed by (project, session, normalized-plan hash), one JSON file per (project, session) under the data dir, written atomically (temp + rename), pruned after seven days, entirely best-effort so a store failure never breaks review. Plans are normalized (CRLF to LF + trim) so a Windows or trailing-whitespace resubmission of the same plan still matches. Before opening a review the store is consulted: - Claude ExitPlanMode re-emits only a prior APPROVAL without re-opening; a prior denial still re-opens, so a plan denied by mistake can be reconsidered by resubmitting it. - Codex Stop re-emits a prior approval or denial, because a bookkeeping turn would otherwise re-review the same decided plan every turn. The decision is recorded after each fresh review. The Claude session id comes from the event, the Codex one from its thread/rollout. Gemini plans (file-based, different identity) are excluded. The whole feature is gated by a new `planDecisionReuse` setting (`PLANNOTATOR_PLAN_DECISION_REUSE` env or config.json), defaulting ON so the bug is fixed out of the box while remaining disable-able for anyone who prefers to always re-review. --- AGENTS.md | 1 + apps/hook/server/index.ts | 200 +++++++++++++------ apps/hook/server/plan-decision-store.test.ts | 100 ++++++++++ apps/hook/server/plan-decision-store.ts | Bin 0 -> 4151 bytes packages/shared/config.test.ts | 7 + packages/shared/config.ts | 22 ++ 6 files changed, 272 insertions(+), 58 deletions(-) create mode 100644 apps/hook/server/plan-decision-store.test.ts create mode 100644 apps/hook/server/plan-decision-store.ts diff --git a/AGENTS.md b/AGENTS.md index a0fa8efaa..83e34035a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -148,6 +148,7 @@ claude --plugin-dir ./apps/hook | `PLANNOTATOR_GUIDE_VIEWER_URL` | Base URL of the portable Guided Review viewer that exported guides pin (default `https://guides.show/v1/`). Must be `https:` (or `http:` on localhost for local viewer builds — `bun run --cwd apps/guides-show serve:local`); anything else is ignored. Read by the export endpoints of both servers and by `plannotator guide export` (which also accepts `--viewer-url`). | | `PLANNOTATOR_GUIDE_SHARE_URL` | Base URL of the guide host that Guided Review share links are created on: the review UI's "Create share link", `plannotator guide share`, and `plannotator guide unshare` upload to and delete from it (default `https://guides.show`; the origin of your own deployment of its Cloudflare Worker otherwise, see the `apps/guides-show` README). Must be `http(s)`; credentials, query and fragment are dropped and a trailing slash is trimmed; an invalid value warns once on stderr and falls back to the default so a share setting can never break a server launch or CLI run. An empty-but-set env var counts as unset. Can also be set via `~/.plannotator/config.json` (`{ "guideShareUrl": "https://guides.example.com" }`); the env var takes precedence; there is no per-invocation flag. Resolved by `resolveGuideShareUrl` in `packages/shared/config.ts`. Whether sharing is allowed at all is `PLANNOTATOR_SHARE` (`disabled` turns guide share links off entirely). Removal always goes to the host a saved guide's record names, never merely the currently configured URL, so changing this after sharing does not strand a link. | | `PLANNOTATOR_GUIDE_HISTORY` | Set to `0` / `false` to disable persisting successful Guided Reviews (no guide copies are written to the data dir; the "Previous guides" list is then never populated, though already-saved guides remain readable and listed). **Note that a persisted guide includes a full copy of the diff it was generated against** — `history/.../guides/{id}.patch` beside the `{id}.json` envelope, uncapped, as large as the diff — because that patch is what a later portable export or share link renders (the diff is captured when the guide job launches, never re-read from the working tree). Deleting a guide removes both files; nothing prunes the directory otherwise. Turning this flag off skips the patch copy too, at the cost of exports and share links for guides from that session once the server exits. Default: enabled. Can also be set via `~/.plannotator/config.json` (`{ "guideHistory": false }`); the env var takes precedence. | +| `PLANNOTATOR_PLAN_DECISION_REUSE` | Set to `0` / `false` to disable reusing a prior plan decision, so an identical plan is always re-reviewed. Default: enabled — a plan already decided in the same agent session is not re-opened (an identical `ExitPlanMode` resubmission re-emits the prior approval; a Codex `Stop` turn that re-scrapes the previous turn's decided plan re-emits its approval or denial). A prior denial re-opens on the Claude path so a mistaken denial can be reconsidered. Decisions are recorded per (project, session, normalized plan) under the data dir. Can also be set via `~/.plannotator/config.json` (`{ "planDecisionReuse": false }`); the env var takes precedence. | | `PLANNOTATOR_CURSOR_SANDBOX` | Set to `0` / `false` / `disabled` to stop passing `--sandbox enabled` when launching Cursor's `agent` CLI for review jobs — the flag pair is omitted entirely, deferring to the user's own Cursor Agent sandbox configuration. For systems where Cursor's sandbox cannot start (NixOS, AppArmor-restricted Linux). Default: enabled (`--sandbox enabled` is passed). Can also be set via `~/.plannotator/config.json` (`{ "cursorSandbox": false }`); the env var takes precedence. Note: opting out means the review job's write protection relies on `--mode ask` plus the user's own Cursor configuration. | | `PLANNOTATOR_TODO_PROVIDER` | Set to `off` / `0` / `false` / `disabled` to stop mirroring the approved plan checklist into an editable todo provider during execution. Default: enabled, which syncs only when a provider is detected (currently pi-todos: detected when its todo directory exists — `/.pi/todos` by default, or wherever `PI_TODO_PATH` redirects it when set). The repo-implied `/.pi/todos` must realpath to a location inside the project or the provider reads as absent and never writes, so a symlink committed into a hostile repo cannot redirect todo writes out of it; an explicitly set `PI_TODO_PATH` is the user's own choice and is honored verbatim, including outside the project. The mirror is additive — the progress widget is unaffected either way — and sync is one-way, so provider-side edits never feed back into plan execution. Can also be set via `~/.plannotator/config.json` (`{ "todoProvider": "off" }`); the env var takes precedence. | | `JINA_API_KEY` | Optional Jina Reader API key for higher rate limits (500 RPM vs 20 RPM unauthenticated). Free keys include 10M tokens. | diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index 52f466861..2c347e91a 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -95,7 +95,7 @@ import { handleGoalSetupServerReady, } from "@plannotator/server/goal-setup"; import { type DiffType, detectManagedVcs, prepareLocalReviewDiff, gitRuntime } from "@plannotator/server/vcs"; -import { loadConfig, resolveDefaultDiffType, resolveSharingEnabled } from "@plannotator/shared/config"; +import { loadConfig, resolveDefaultDiffType, resolveSharingEnabled, resolvePlanDecisionReuse } from "@plannotator/shared/config"; import { parseReviewArgs } from "@plannotator/shared/review-args"; import { normalizeGoalSetupBundle, @@ -163,6 +163,7 @@ import { type RenderedMessage, } from "./session-log"; import { findCodexRolloutByThreadId, getLatestCodexPlan, getRecentCodexMessages } from "./codex-session"; +import { getPlanDecision, recordPlanDecision } from "./plan-decision-store"; import { findCopilotPlanContent, findCopilotSessionByAncestorPids, findCopilotSessionForCwd, getRecentCopilotMessages } from "./copilot-session"; import { formatInteractiveNoArgClarification, @@ -2188,35 +2189,76 @@ if (args[0] === "sessions") { } const planProject = (await detectProjectName()) ?? "_unknown"; - const server = await startPlannotatorServer({ - plan: latestPlan.text, - origin: "codex", - sharingEnabled, - shareBaseUrl, - pasteApiUrl, - htmlContent: planHtmlContent, - onReady: async (url, isRemote, port) => { - handleServerReady(url, isRemote, port); + // The Codex session identity is its thread/rollout, not an event session id. + const codexSessionId = process.env.CODEX_THREAD_ID || rolloutPath || ""; + + // Skip re-review of a plan already decided in this session (#1075): a Codex + // Stop turn that did no planning still scrapes the previous turn's plan, so + // an already-decided plan would otherwise re-open on every bookkeeping turn. + // Both approvals and denials dedup here (a denied plan would re-open every + // bookkeeping turn otherwise). Controlled by the planDecisionReuse setting. + const planDecisionReuseEnabled = resolvePlanDecisionReuse(loadConfig()); + const priorDecision = planDecisionReuseEnabled + ? getPlanDecision(planProject, codexSessionId, latestPlan.text) + : null; - if (isRemote && sharingEnabled) { - await writeRemoteShareLink(latestPlan.text, shareBaseUrl, "review the plan", "plan only").catch(() => {}); - } - }, - }); + let result: { + approved: boolean; + feedback?: string; + savedPath?: string; + agentSwitch?: string; + permissionMode?: string; + }; - registerSession({ - pid: process.pid, - port: server.port, - url: server.url, - mode: "plan", - project: planProject, - startedAt: new Date().toISOString(), - label: `plan-${planProject}`, - }); + if (priorDecision) { + result = + priorDecision.decision === "approved" + ? { approved: true } + : { approved: false, feedback: priorDecision.feedback }; + } else { + const server = await startPlannotatorServer({ + plan: latestPlan.text, + origin: "codex", + sharingEnabled, + shareBaseUrl, + pasteApiUrl, + htmlContent: planHtmlContent, + onReady: async (url, isRemote, port) => { + handleServerReady(url, isRemote, port); + + if (isRemote && sharingEnabled) { + await writeRemoteShareLink(latestPlan.text, shareBaseUrl, "review the plan", "plan only").catch(() => {}); + } + }, + }); - const result = await server.waitForDecision(); - await Bun.sleep(1500); - server.stop(); + registerSession({ + pid: process.pid, + port: server.port, + url: server.url, + mode: "plan", + project: planProject, + startedAt: new Date().toISOString(), + label: `plan-${planProject}`, + }); + + result = await server.waitForDecision(); + await Bun.sleep(1500); + server.stop(); + + // Remember this decision so the same plan is not re-reviewed on a later + // bookkeeping turn in this session. + if (planDecisionReuseEnabled) { + recordPlanDecision( + planProject, + codexSessionId, + latestPlan.text, + result.approved + ? { decision: "approved" } + : { decision: "denied", feedback: result.feedback }, + ); + } + } if (result.approved) { console.log("{}"); @@ -2264,43 +2306,85 @@ if (args[0] === "sessions") { } const planProject = (await detectProjectName()) ?? "_unknown"; + const planSessionId = typeof event.session_id === "string" ? event.session_id : ""; + + // Skip re-review of a plan already APPROVED in this session (#1075): an + // identical ExitPlanMode plan submitted twice would otherwise open a second + // review. A prior DENIAL deliberately re-opens, so a plan denied by mistake + // can be reconsidered by resubmitting it (the Codex Stop path below also + // dedups denials, because a bookkeeping turn there would re-review a denied + // plan every turn). Gemini plans live on disk with a different identity + // model, so they are excluded from the dedup here. Controlled by the + // planDecisionReuse setting. + const planDecisionReuseEnabled = resolvePlanDecisionReuse(loadConfig()); + const priorApproved = + !isGemini && + planDecisionReuseEnabled && + getPlanDecision(planProject, planSessionId, planContent)?.decision === "approved"; + + let result: { + approved: boolean; + feedback?: string; + savedPath?: string; + agentSwitch?: string; + permissionMode?: string; + }; - // Start the plan review server - const server = await startPlannotatorServer({ - plan: planContent, - origin: isGemini ? "gemini-cli" : detectedOrigin, - permissionMode, - sharingEnabled, - shareBaseUrl, - pasteApiUrl, - htmlContent: planHtmlContent, - onReady: async (url, isRemote, port) => { - handleServerReady(url, isRemote, port); + if (priorApproved) { + // The permission mode set on the first approval is session-scoped, so it is + // deliberately not re-asserted here. + result = { approved: true }; + } else { + // Start the plan review server + const server = await startPlannotatorServer({ + plan: planContent, + origin: isGemini ? "gemini-cli" : detectedOrigin, + permissionMode, + sharingEnabled, + shareBaseUrl, + pasteApiUrl, + htmlContent: planHtmlContent, + onReady: async (url, isRemote, port) => { + handleServerReady(url, isRemote, port); - if (isRemote && sharingEnabled) { - await writeRemoteShareLink(planContent, shareBaseUrl, "review the plan", "plan only").catch(() => {}); - } - }, - }); + if (isRemote && sharingEnabled) { + await writeRemoteShareLink(planContent, shareBaseUrl, "review the plan", "plan only").catch(() => {}); + } + }, + }); - registerSession({ - pid: process.pid, - port: server.port, - url: server.url, - mode: "plan", - project: planProject, - startedAt: new Date().toISOString(), - label: `plan-${planProject}`, - }); + registerSession({ + pid: process.pid, + port: server.port, + url: server.url, + mode: "plan", + project: planProject, + startedAt: new Date().toISOString(), + label: `plan-${planProject}`, + }); - // Wait for user decision (blocks until approve/deny) - const result = await server.waitForDecision(); + // Wait for user decision (blocks until approve/deny) + result = await server.waitForDecision(); - // Give browser time to receive response and update UI - await Bun.sleep(1500); + // Give browser time to receive response and update UI + await Bun.sleep(1500); - // Cleanup - server.stop(); + // Cleanup + server.stop(); + + // Remember this decision so an identical resubmission in the same session is + // not reviewed again. + if (!isGemini && planDecisionReuseEnabled) { + recordPlanDecision( + planProject, + planSessionId, + planContent, + result.approved + ? { decision: "approved" } + : { decision: "denied", feedback: result.feedback }, + ); + } + } // Output decision in the appropriate format for the harness if (isGemini) { diff --git a/apps/hook/server/plan-decision-store.test.ts b/apps/hook/server/plan-decision-store.test.ts new file mode 100644 index 000000000..dc59630e7 --- /dev/null +++ b/apps/hook/server/plan-decision-store.test.ts @@ -0,0 +1,100 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { existsSync, mkdtempSync, readdirSync, rmSync, utimesSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { getPlanDecision, recordPlanDecision } from "./plan-decision-store"; + +const dirs: string[] = []; +const tmp = (): string => { + const dir = mkdtempSync(join(tmpdir(), "plan-decisions-")); + dirs.push(dir); + return dir; +}; + +afterEach(() => { + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + +const P = "my-project"; + +describe("plan-decision-store (#1075)", () => { + test("returns null before any decision is recorded", () => { + expect(getPlanDecision(P, "sess-a", "# Plan\n", tmp())).toBeNull(); + }); + + test("records and returns an approval for the same project, session, and plan", () => { + const dir = tmp(); + recordPlanDecision(P, "sess-a", "# Plan\n\n- step", { decision: "approved" }, dir); + expect(getPlanDecision(P, "sess-a", "# Plan\n\n- step", dir)).toEqual({ + decision: "approved", + }); + }); + + test("matches plans after normalizing CRLF and surrounding whitespace", () => { + const dir = tmp(); + recordPlanDecision(P, "sess-a", "# Plan\n\n- a\n- b", { decision: "approved" }, dir); + // A Windows resubmission (CRLF) with a trailing blank line still matches. + expect( + getPlanDecision(P, "sess-a", " # Plan\r\n\r\n- a\r\n- b\r\n", dir), + ).toEqual({ decision: "approved" }); + }); + + test("does not match a plan whose body actually changed", () => { + const dir = tmp(); + recordPlanDecision(P, "sess-a", "# Plan\n- a", { decision: "approved" }, dir); + expect(getPlanDecision(P, "sess-a", "# Plan\n- a\n- b", dir)).toBeNull(); + }); + + test("keeps decisions separate per project, session, and plan", () => { + const dir = tmp(); + recordPlanDecision(P, "sess-a", "plan one", { decision: "approved" }, dir); + expect(getPlanDecision("other-project", "sess-a", "plan one", dir)).toBeNull(); + expect(getPlanDecision(P, "sess-b", "plan one", dir)).toBeNull(); + expect(getPlanDecision(P, "sess-a", "plan two", dir)).toBeNull(); + }); + + test("records a denial together with its feedback", () => { + const dir = tmp(); + recordPlanDecision(P, "sess-a", "plan", { decision: "denied", feedback: "add tests" }, dir); + expect(getPlanDecision(P, "sess-a", "plan", dir)).toEqual({ + decision: "denied", + feedback: "add tests", + }); + }); + + test("a later decision for the same plan overwrites the earlier one", () => { + const dir = tmp(); + recordPlanDecision(P, "sess-a", "plan", { decision: "denied", feedback: "no" }, dir); + recordPlanDecision(P, "sess-a", "plan", { decision: "approved" }, dir); + expect(getPlanDecision(P, "sess-a", "plan", dir)).toEqual({ decision: "approved" }); + }); + + test("an empty session id never records or matches (safe fallback)", () => { + const dir = tmp(); + recordPlanDecision(P, "", "plan", { decision: "approved" }, dir); + expect(getPlanDecision(P, "", "plan", dir)).toBeNull(); + }); + + test("tolerates session ids that are file paths or contain separators", () => { + const dir = tmp(); + const pathLikeId = "/Users/x/.codex/sessions/2026/07/rollout-abc.jsonl"; + recordPlanDecision(P, pathLikeId, "plan", { decision: "approved" }, dir); + expect(getPlanDecision(P, pathLikeId, "plan", dir)).toEqual({ decision: "approved" }); + }); + + test("prunes session files older than the retention window on write", () => { + const dir = tmp(); + recordPlanDecision(P, "old-session", "plan", { decision: "approved" }, dir); + const decisionsDir = join(dir, "plan-decisions"); + const [oldFile] = readdirSync(decisionsDir); + const oldPath = join(decisionsDir, oldFile); + const ancientSeconds = Date.now() / 1000 - 8 * 24 * 60 * 60; + utimesSync(oldPath, ancientSeconds, ancientSeconds); + + recordPlanDecision(P, "new-session", "plan", { decision: "approved" }, dir); + expect(existsSync(oldPath)).toBe(false); + expect(getPlanDecision(P, "new-session", "plan", dir)).toEqual({ decision: "approved" }); + expect(getPlanDecision(P, "old-session", "plan", dir)).toBeNull(); + }); +}); diff --git a/apps/hook/server/plan-decision-store.ts b/apps/hook/server/plan-decision-store.ts new file mode 100644 index 0000000000000000000000000000000000000000..768015a41a7f3da5037bcc2cf7546177195eb239 GIT binary patch literal 4151 zcmb_fVRIWb5an}z#bOw^3-&n_Xqgl@gr|hj`y{P)_w9STH{RdhqkVcR&B#h?RjtX$yfy{ZB{d77sgSwi=XOZ8CJ`Qr zg^{9Ik!TuIUrOvY*r*l0Da{7mT#^-)Br%iPQD<9O(zU}CBO@=BTw;HstZgNI_VvNN z@6wE(FKco}&1TYAnkxrO${w;w_@Z!!Q@L2s#iJ|bp2EMgx{w#aDcepf9grggQ!?pjOw z_58^nRH}t!gN-zO@>*Ts7iYE*PQZJU(W6UgR#an~a10(2JeAgsWLeh69U}Zvs6`8R z=F)hJRcjq-iE}gsi0?!00vtm^!r$?RN(vpr=OU=Y3U+~81Wn9@@i{h1#b$GA4O zbKqW#8J!VaW;5gceL9YmVtiYd*I)&}@mTvRfD4R!tdflO$9wY1C)KSQ8EZ-wPjS;p zBqL3bw!e||j-7`z=(sqb^@odroP+yRN?A-r{{H9?rEa+D*Kjijza5>w`03>I8BJ)w ze)$6T%ts&`$9WV8u1!nzB2*))gDL7uzA>%lC>!>^)xem^xGF75T^YpRSb#CHD%U& zcc(HJt(DATJYsc`#a7Cr1I!FgB1*JN8w`b@HV@+F)1rHHTaJ-yVj*G{6J30My%B+A z0H5!CcXyCxZ39Zk1k3T3^(UWS_x9HpX;!EiR4W`$97}7U)(slOE*7|}k z=wM*Ud`0V$`3>H4Q3!u`gil5uom>NQg@0`{pI*hc9;BMi& z+e@X3dTGH}-%UbCOu$wF8&Fc58)>=3f2go3ONgq@Wy;+W@Oh-TT0F0VcQ&Ilg-f}e z)l1$*&|cqZK^sWQ3WyhNvwJ5_K?VX-*w}A!#gOsWkKpldndf7fk@$zaj$fPCdORD_ z;I&4wIWxDB1aJI zoC_wp5lzLVJ(fXMx#!$pJDKg~oJtfyw7t~HE0Mc^F|^x1iiJ&Ibu1rZB;s<=?P9Sl z>1C^P->G9?If+*#@b4xE@u+7>mvtt>b`|jNn`$`R16Kn!H`>PE-XEU;RZ{Y4Z$9lZ z%l>k%0!Uu}CY={797&fOmf}uhp@7l%UKCFYkC?w@)9pQ|=?9lZD7K0%Y<$I$qV#>; zr*BxwgMaIQRfy4Zs&mWJczaUZVX}uSPNp9~#&AJ4Zw#mz{j@z$1=WTELnUs-)%X$c zDIEVlpzXrC9nX(~e_5l+FxzfNnE}4ZN2oZ+*xk2-E^ptO(ztO9F?%4=7D9FjMQ{cq z{5o>fPXSLBoo-m(AD2E}Rc!JjHPjVj?;HI5pj3j7{g#`RvxzKh9FOTGPl?dTF#-vn7nquhMLFUD$4MRRK+3!k9*kUFshme3 z)_(s|#I&-0%t-O(fWMFz2QMbvMR|Rl`F5E@e6vOs>kO+G!TFu}Ho@~@5YeE+s$6YI z3TAc+8qNm4cv21r`6>_2t~es7WB=*^i4GP0-`@*{+*Tp8aVp^m*Lz@WFMYQaNT+X5 zuD*CfR`S->Wr@UK_LiBhmq|(^+Ki7J*Fs{}(r9JUc7|TP;e}0=$K_Z%C`q4#t~7U} z4|>Gsd6ed1Jqf;eOwn$lu{5?4&oW-iq%Q1B<>tx2&n@U%jIC3;l_CGZ4v%f{#Luao zO?)_VWaDz6Z}(=C_*pNjoT}uRr5`9H24d=8g8VYm67J?VDY+XS5#T+YVP3-fd8k)E etzHU@>^x5)U`(higm~%W`@bwgopL%D%YOi3-DPnA literal 0 HcmV?d00001 diff --git a/packages/shared/config.test.ts b/packages/shared/config.test.ts index fe35d8f39..aab01cb04 100644 --- a/packages/shared/config.test.ts +++ b/packages/shared/config.test.ts @@ -15,6 +15,7 @@ import { resolveCursorSandbox, resolveUseGlimpse, resolveAnnotateHistory, + resolvePlanDecisionReuse, resolveGuideHistory, resolveUseJina, resolveTodoProviderEnabled, @@ -285,6 +286,12 @@ describe("config.json boolean coercion", () => { key: "guideHistory", resolve: resolveGuideHistory, }, + { + name: "resolvePlanDecisionReuse", + envVar: "PLANNOTATOR_PLAN_DECISION_REUSE", + key: "planDecisionReuse", + resolve: resolvePlanDecisionReuse, + }, { name: "resolveUseJina", envVar: "PLANNOTATOR_JINA", diff --git a/packages/shared/config.ts b/packages/shared/config.ts index 225e1429e..08185dfab 100644 --- a/packages/shared/config.ts +++ b/packages/shared/config.ts @@ -190,6 +190,13 @@ export interface PlannotatorConfig { * `resolveMarkdownExtensions` in ./markdown-extensions. Default: none. */ markdownExtensions?: string[]; + /** + * Skip re-opening a plan review for a plan already decided in the same agent + * session (#1075): an identical ExitPlanMode resubmission, or a Codex Stop turn + * that re-scrapes the previous turn's decided plan. Set to false to always + * re-review. Default: true. + */ + planDecisionReuse?: boolean; /** * Persist successful Guided Reviews (guide content + per-section reviewed * state) under ~/.plannotator/guides/ (or PLANNOTATOR_DATA_DIR) so they @@ -647,6 +654,21 @@ export function resolveAnnotateHistory(config: PlannotatorConfig): boolean { return coerceConfigBoolean(config.annotateHistory, true); } +/** + * Resolve whether an already-decided plan is re-used instead of re-reviewed + * within the same session (#1075). + * + * Priority (highest wins): + * PLANNOTATOR_PLAN_DECISION_REUSE env var → config.planDecisionReuse → default true + */ +export function resolvePlanDecisionReuse(config: PlannotatorConfig): boolean { + const envVal = process.env.PLANNOTATOR_PLAN_DECISION_REUSE; + if (envVal !== undefined) { + return envVal === "1" || envVal.toLowerCase() === "true"; + } + return coerceConfigBoolean(config.planDecisionReuse, true); +} + /** * Resolve whether successful Guided Reviews are persisted to disk. * From 6b2fa164f7ebd80004e5f6f81e9af5b3e23e1d80 Mon Sep 17 00:00:00 2001 From: rNoz Date: Fri, 31 Jul 2026 21:20:12 +0200 Subject: [PATCH 2/4] fix(hook): scope plan reuse to occurrences Reuse only fresh approvals for the active Claude ExitPlanMode occurrence; never replay Codex or denial decisions. Filter Codex proposal fallbacks to the requested turn. Refs #1169 --- AGENTS.md | 2 +- apps/hook/server/codex-session.test.ts | 17 ++ apps/hook/server/codex-session.ts | 27 ++- apps/hook/server/index.ts | 150 ++++++--------- apps/hook/server/plan-decision-policy.test.ts | 178 ++++++++++++++++++ apps/hook/server/plan-decision-policy.ts | 59 ++++++ apps/hook/server/plan-decision-store.test.ts | 127 +++++++++---- apps/hook/server/plan-decision-store.ts | Bin 4151 -> 5410 bytes apps/hook/server/plan-normalization.ts | 3 + apps/hook/server/session-log.test.ts | 90 +++++++++ apps/hook/server/session-log.ts | 76 ++++++++ .../docs/reference/environment-variables.md | 1 + 12 files changed, 594 insertions(+), 136 deletions(-) create mode 100644 apps/hook/server/plan-decision-policy.test.ts create mode 100644 apps/hook/server/plan-decision-policy.ts create mode 100644 apps/hook/server/plan-normalization.ts diff --git a/AGENTS.md b/AGENTS.md index 83e34035a..6805237dd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -148,7 +148,7 @@ claude --plugin-dir ./apps/hook | `PLANNOTATOR_GUIDE_VIEWER_URL` | Base URL of the portable Guided Review viewer that exported guides pin (default `https://guides.show/v1/`). Must be `https:` (or `http:` on localhost for local viewer builds — `bun run --cwd apps/guides-show serve:local`); anything else is ignored. Read by the export endpoints of both servers and by `plannotator guide export` (which also accepts `--viewer-url`). | | `PLANNOTATOR_GUIDE_SHARE_URL` | Base URL of the guide host that Guided Review share links are created on: the review UI's "Create share link", `plannotator guide share`, and `plannotator guide unshare` upload to and delete from it (default `https://guides.show`; the origin of your own deployment of its Cloudflare Worker otherwise, see the `apps/guides-show` README). Must be `http(s)`; credentials, query and fragment are dropped and a trailing slash is trimmed; an invalid value warns once on stderr and falls back to the default so a share setting can never break a server launch or CLI run. An empty-but-set env var counts as unset. Can also be set via `~/.plannotator/config.json` (`{ "guideShareUrl": "https://guides.example.com" }`); the env var takes precedence; there is no per-invocation flag. Resolved by `resolveGuideShareUrl` in `packages/shared/config.ts`. Whether sharing is allowed at all is `PLANNOTATOR_SHARE` (`disabled` turns guide share links off entirely). Removal always goes to the host a saved guide's record names, never merely the currently configured URL, so changing this after sharing does not strand a link. | | `PLANNOTATOR_GUIDE_HISTORY` | Set to `0` / `false` to disable persisting successful Guided Reviews (no guide copies are written to the data dir; the "Previous guides" list is then never populated, though already-saved guides remain readable and listed). **Note that a persisted guide includes a full copy of the diff it was generated against** — `history/.../guides/{id}.patch` beside the `{id}.json` envelope, uncapped, as large as the diff — because that patch is what a later portable export or share link renders (the diff is captured when the guide job launches, never re-read from the working tree). Deleting a guide removes both files; nothing prunes the directory otherwise. Turning this flag off skips the patch copy too, at the cost of exports and share links for guides from that session once the server exits. Default: enabled. Can also be set via `~/.plannotator/config.json` (`{ "guideHistory": false }`); the env var takes precedence. | -| `PLANNOTATOR_PLAN_DECISION_REUSE` | Set to `0` / `false` to disable reusing a prior plan decision, so an identical plan is always re-reviewed. Default: enabled — a plan already decided in the same agent session is not re-opened (an identical `ExitPlanMode` resubmission re-emits the prior approval; a Codex `Stop` turn that re-scrapes the previous turn's decided plan re-emits its approval or denial). A prior denial re-opens on the Claude path so a mistaken denial can be reconsidered. Decisions are recorded per (project, session, normalized plan) under the data dir. Can also be set via `~/.plannotator/config.json` (`{ "planDecisionReuse": false }`); the env var takes precedence. | +| `PLANNOTATOR_PLAN_DECISION_REUSE` | **Hook-runtime only.** Set to `0` / `false` to disable reuse, so every plan opens a review. Default: enabled — only a fresh Claude approval for the *same active* `ExitPlanMode` occurrence may be reused (for a short retry window), with a visible hook message. A new identical tool occurrence, a rewound/compacted occurrence, or missing transcript identity opens a fresh review; denials never replay. Codex uses current-turn filtering for proposed plans and never replays decisions. Approval records are scoped to project/session/normalized plan and occurrence under the data dir. Can also be set via `~/.plannotator/config.json` (`{ "planDecisionReuse": false }`); the env var takes precedence. | | `PLANNOTATOR_CURSOR_SANDBOX` | Set to `0` / `false` / `disabled` to stop passing `--sandbox enabled` when launching Cursor's `agent` CLI for review jobs — the flag pair is omitted entirely, deferring to the user's own Cursor Agent sandbox configuration. For systems where Cursor's sandbox cannot start (NixOS, AppArmor-restricted Linux). Default: enabled (`--sandbox enabled` is passed). Can also be set via `~/.plannotator/config.json` (`{ "cursorSandbox": false }`); the env var takes precedence. Note: opting out means the review job's write protection relies on `--mode ask` plus the user's own Cursor configuration. | | `PLANNOTATOR_TODO_PROVIDER` | Set to `off` / `0` / `false` / `disabled` to stop mirroring the approved plan checklist into an editable todo provider during execution. Default: enabled, which syncs only when a provider is detected (currently pi-todos: detected when its todo directory exists — `/.pi/todos` by default, or wherever `PI_TODO_PATH` redirects it when set). The repo-implied `/.pi/todos` must realpath to a location inside the project or the provider reads as absent and never writes, so a symlink committed into a hostile repo cannot redirect todo writes out of it; an explicitly set `PI_TODO_PATH` is the user's own choice and is honored verbatim, including outside the project. The mirror is additive — the progress widget is unaffected either way — and sync is one-way, so provider-side edits never feed back into plan execution. Can also be set via `~/.plannotator/config.json` (`{ "todoProvider": "off" }`); the env var takes precedence. | | `JINA_API_KEY` | Optional Jina Reader API key for higher rate limits (500 RPM vs 20 RPM unauthenticated). Free keys include 10M tokens. | diff --git a/apps/hook/server/codex-session.test.ts b/apps/hook/server/codex-session.test.ts index 450ad27ab..a1d998e57 100644 --- a/apps/hook/server/codex-session.test.ts +++ b/apps/hook/server/codex-session.test.ts @@ -453,6 +453,23 @@ describe("getLatestCodexPlan", () => { expect(result).toBeNull(); }); + test("does not scrape a proposed plan from a later task when the requested turn has none", () => { + const requestedTurnId = "turn-requested"; + const laterTurnId = "turn-later"; + const path = writeTempRollout( + buildRollout( + sessionMeta(), + turnStarted(requestedTurnId), + assistantMessage("I have no plan to submit for this task."), + turnCompleted(requestedTurnId), + turnStarted(laterTurnId), + assistantMessage("\nPlan from the later task\n"), + ) + ); + + expect(getLatestCodexPlan(path, { turnId: requestedTurnId })).toBeNull(); + }); + test("returns null when Stop re-entry has no revised plan after the hook prompt", () => { const turnId = "turn-stop-no-revision"; const path = writeTempRollout( diff --git a/apps/hook/server/codex-session.ts b/apps/hook/server/codex-session.ts index b6a4e8d17..ccb33c1d9 100644 --- a/apps/hook/server/codex-session.ts +++ b/apps/hook/server/codex-session.ts @@ -17,6 +17,8 @@ import { readFileSync, readdirSync, statSync } from "node:fs"; import { join } from "node:path"; import { homedir } from "node:os"; +import { normalizePlanText } from "./plan-normalization"; + // --- Types --- type CodexPlanSource = "plan-item" | "assistant-message"; @@ -170,10 +172,6 @@ function extractLastProposedPlan(text: string): string | null { return latest || null; } -function normalizePlan(text: string): string { - return text.replace(/\r\n/g, "\n").trim(); -} - function findLastIndex( entries: RolloutEntry[], predicate: (entry: RolloutEntry) => boolean @@ -280,22 +278,37 @@ function getAssistantProposedPlanText(entry: RolloutEntry): string | null { return extractLastProposedPlan(messageText); } +function getTurnId(entry: RolloutEntry): string | null { + const turnId = entry.payload?.turn_id; + return typeof turnId === "string" && turnId ? turnId : null; +} + function collectPlanCandidates( entries: RolloutEntry[], startIndex: number, turnId?: string ): CodexPlanCandidate[] { const candidates: CodexPlanCandidate[] = []; + let activeTurnId: string | null = null; for (let i = Math.max(startIndex, 0); i < entries.length; i++) { const entry = entries[i]; + if ( + (entry.type === "event_msg" && TURN_START_TYPES.has(entry.payload?.type || "")) || + entry.type === "turn_context" + ) { + activeTurnId = getTurnId(entry); + } const planItemText = getPlanItemText(entry, turnId); if (planItemText) { candidates.push({ index: i, text: planItemText, source: "plan-item" }); } - const assistantPlanText = getAssistantProposedPlanText(entry); + const assistantPlanText = + !turnId || activeTurnId === turnId + ? getAssistantProposedPlanText(entry) + : null; if (assistantPlanText) { candidates.push({ index: i, @@ -457,8 +470,8 @@ export function getLatestCodexPlan( if ( latestBeforeHookPrompt && - normalizePlan(latestBeforeHookPrompt.text) === - normalizePlan(latestAfterHookPrompt.text) + normalizePlanText(latestBeforeHookPrompt.text) === + normalizePlanText(latestAfterHookPrompt.text) ) { return null; } diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index 2c347e91a..e7b43a3a8 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -160,10 +160,14 @@ import { resolveDroidSessionLogForCwd, resolveSessionLogByAncestorPids, resolveSessionLogByCwdScan, + findActiveExitPlanModeOccurrenceInTranscript, type RenderedMessage, } from "./session-log"; import { findCodexRolloutByThreadId, getLatestCodexPlan, getRecentCodexMessages } from "./codex-session"; -import { getPlanDecision, recordPlanDecision } from "./plan-decision-store"; +import { + recordPlanApprovalForSubmission, + shouldReusePlanApproval, +} from "./plan-decision-policy"; import { findCopilotPlanContent, findCopilotSessionByAncestorPids, findCopilotSessionForCwd, getRecentCopilotMessages } from "./copilot-session"; import { formatInteractiveNoArgClarification, @@ -2189,76 +2193,35 @@ if (args[0] === "sessions") { } const planProject = (await detectProjectName()) ?? "_unknown"; - // The Codex session identity is its thread/rollout, not an event session id. - const codexSessionId = process.env.CODEX_THREAD_ID || rolloutPath || ""; - - // Skip re-review of a plan already decided in this session (#1075): a Codex - // Stop turn that did no planning still scrapes the previous turn's plan, so - // an already-decided plan would otherwise re-open on every bookkeeping turn. - // Both approvals and denials dedup here (a denied plan would re-open every - // bookkeeping turn otherwise). Controlled by the planDecisionReuse setting. - const planDecisionReuseEnabled = resolvePlanDecisionReuse(loadConfig()); - const priorDecision = planDecisionReuseEnabled - ? getPlanDecision(planProject, codexSessionId, latestPlan.text) - : null; - - let result: { - approved: boolean; - feedback?: string; - savedPath?: string; - agentSwitch?: string; - permissionMode?: string; - }; + const server = await startPlannotatorServer({ + plan: latestPlan.text, + origin: "codex", + sharingEnabled, + shareBaseUrl, + pasteApiUrl, + htmlContent: planHtmlContent, + onReady: async (url, isRemote, port) => { + handleServerReady(url, isRemote, port); - if (priorDecision) { - result = - priorDecision.decision === "approved" - ? { approved: true } - : { approved: false, feedback: priorDecision.feedback }; - } else { - const server = await startPlannotatorServer({ - plan: latestPlan.text, - origin: "codex", - sharingEnabled, - shareBaseUrl, - pasteApiUrl, - htmlContent: planHtmlContent, - onReady: async (url, isRemote, port) => { - handleServerReady(url, isRemote, port); - - if (isRemote && sharingEnabled) { - await writeRemoteShareLink(latestPlan.text, shareBaseUrl, "review the plan", "plan only").catch(() => {}); - } - }, - }); + if (isRemote && sharingEnabled) { + await writeRemoteShareLink(latestPlan.text, shareBaseUrl, "review the plan", "plan only").catch(() => {}); + } + }, + }); - registerSession({ - pid: process.pid, - port: server.port, - url: server.url, - mode: "plan", - project: planProject, - startedAt: new Date().toISOString(), - label: `plan-${planProject}`, - }); + registerSession({ + pid: process.pid, + port: server.port, + url: server.url, + mode: "plan", + project: planProject, + startedAt: new Date().toISOString(), + label: `plan-${planProject}`, + }); - result = await server.waitForDecision(); - await Bun.sleep(1500); - server.stop(); - - // Remember this decision so the same plan is not re-reviewed on a later - // bookkeeping turn in this session. - if (planDecisionReuseEnabled) { - recordPlanDecision( - planProject, - codexSessionId, - latestPlan.text, - result.approved - ? { decision: "approved" } - : { decision: "denied", feedback: result.feedback }, - ); - } - } + const result = await server.waitForDecision(); + await Bun.sleep(1500); + server.stop(); if (result.approved) { console.log("{}"); @@ -2308,19 +2271,26 @@ if (args[0] === "sessions") { const planProject = (await detectProjectName()) ?? "_unknown"; const planSessionId = typeof event.session_id === "string" ? event.session_id : ""; - // Skip re-review of a plan already APPROVED in this session (#1075): an - // identical ExitPlanMode plan submitted twice would otherwise open a second - // review. A prior DENIAL deliberately re-opens, so a plan denied by mistake - // can be reconsidered by resubmitting it (the Codex Stop path below also - // dedups denials, because a bookkeeping turn there would re-review a denied - // plan every turn). Gemini plans live on disk with a different identity - // model, so they are excluded from the dedup here. Controlled by the - // planDecisionReuse setting. const planDecisionReuseEnabled = resolvePlanDecisionReuse(loadConfig()); - const priorApproved = + const planOccurrence = !isGemini && - planDecisionReuseEnabled && - getPlanDecision(planProject, planSessionId, planContent)?.decision === "approved"; + typeof event.transcript_path === "string" && + event.transcript_path + ? findActiveExitPlanModeOccurrenceInTranscript(event.transcript_path, { + plan: planContent, + toolUseId: + typeof event.tool_use_id === "string" ? event.tool_use_id : undefined, + }) + : null; + const planApprovalContext = { + enabled: planDecisionReuseEnabled, + isGemini, + project: planProject, + sessionId: planSessionId, + plan: planContent, + occurrence: planOccurrence, + }; + const priorApproved = shouldReusePlanApproval(planApprovalContext); let result: { approved: boolean; @@ -2329,11 +2299,13 @@ if (args[0] === "sessions") { agentSwitch?: string; permissionMode?: string; }; + let reusedApproval = false; if (priorApproved) { // The permission mode set on the first approval is session-scoped, so it is // deliberately not re-asserted here. result = { approved: true }; + reusedApproval = true; } else { // Start the plan review server const server = await startPlannotatorServer({ @@ -2372,18 +2344,10 @@ if (args[0] === "sessions") { // Cleanup server.stop(); - // Remember this decision so an identical resubmission in the same session is - // not reviewed again. - if (!isGemini && planDecisionReuseEnabled) { - recordPlanDecision( - planProject, - planSessionId, - planContent, - result.approved - ? { decision: "approved" } - : { decision: "denied", feedback: result.feedback }, - ); - } + recordPlanApprovalForSubmission({ + ...planApprovalContext, + approved: result.approved, + }); } // Output decision in the appropriate format for the harness @@ -2416,6 +2380,10 @@ if (args[0] === "sessions") { console.log( JSON.stringify({ + ...(reusedApproval && { + systemMessage: + "Reused the approval for this same ExitPlanMode submission.", + }), hookSpecificOutput: { hookEventName: "PermissionRequest", decision: { diff --git a/apps/hook/server/plan-decision-policy.test.ts b/apps/hook/server/plan-decision-policy.test.ts new file mode 100644 index 000000000..f7ca47232 --- /dev/null +++ b/apps/hook/server/plan-decision-policy.test.ts @@ -0,0 +1,178 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { mkdtempSync, readdirSync, rmSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; + +import { + findActiveExitPlanModeOccurrence, + type SessionLogEntry, +} from "./session-log"; +import { + recordPlanApprovalForSubmission, + shouldReusePlanApproval, +} from "./plan-decision-policy"; + +const dirs: string[] = []; +const dataDir = (): string => { + const dir = mkdtempSync(join(process.cwd(), ".plan-decision-policy-test-")); + dirs.push(dir); + return dir; +}; + +afterEach(() => { + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + +const plan = "# Plan\n- step"; + +function exitPlanEntry( + entryUuid: string, + parentUuid: string | null, + toolUseId: string, +): SessionLogEntry { + return { + type: "assistant", + uuid: entryUuid, + parentUuid, + message: { + id: `msg-${toolUseId}`, + role: "assistant", + content: [ + { + type: "tool_use", + id: toolUseId, + name: "ExitPlanMode", + input: { plan }, + }, + ], + }, + }; +} + +function context( + dir: string, + occurrence: ReturnType, + overrides: Partial[0]> = {}, +) { + return { + enabled: true, + isGemini: false, + project: "project", + sessionId: "session", + plan, + occurrence, + baseDir: dir, + now: 1_000, + ...overrides, + }; +} + +describe("plan approval reuse policy", () => { + test("reuses an approval only for the same active ExitPlanMode retry", () => { + const dir = dataDir(); + const occurrence = findActiveExitPlanModeOccurrence( + [exitPlanEntry("entry-a", null, "tool-a")], + { plan, toolUseId: "tool-a" }, + ); + const input = context(dir, occurrence); + + recordPlanApprovalForSubmission({ ...input, approved: true }); + + expect(shouldReusePlanApproval(input)).toBe(true); + }); + + test("opens a fresh review for a genuine identical resubmission", () => { + const dir = dataDir(); + const first = findActiveExitPlanModeOccurrence( + [exitPlanEntry("entry-a", null, "tool-a")], + { plan, toolUseId: "tool-a" }, + ); + const second = findActiveExitPlanModeOccurrence( + [ + exitPlanEntry("entry-a", null, "tool-a"), + exitPlanEntry("entry-b", "entry-a", "tool-b"), + ], + { plan, toolUseId: "tool-b" }, + ); + recordPlanApprovalForSubmission({ ...context(dir, first), approved: true }); + + expect(shouldReusePlanApproval(context(dir, second))).toBe(false); + }); + + test("does not replay an approval from a rewound occurrence", () => { + const dir = dataDir(); + const oldOccurrence = findActiveExitPlanModeOccurrence( + [exitPlanEntry("entry-old", null, "tool-old")], + { plan }, + ); + const liveOccurrence = findActiveExitPlanModeOccurrence( + [ + { type: "user", uuid: "root", parentUuid: null, message: { role: "user", content: "plan" } }, + exitPlanEntry("entry-old", "root", "tool-old"), + exitPlanEntry("entry-live", "root", "tool-live"), + ], + { plan }, + ); + recordPlanApprovalForSubmission({ ...context(dir, oldOccurrence), approved: true }); + + expect(shouldReusePlanApproval(context(dir, liveOccurrence))).toBe(false); + }); + + test("does not replay an approval from before a compact boundary", () => { + const dir = dataDir(); + const oldOccurrence = findActiveExitPlanModeOccurrence( + [exitPlanEntry("entry-old", null, "tool-old")], + { plan }, + ); + const compactedOccurrence = findActiveExitPlanModeOccurrence( + [ + { type: "user", uuid: "root", parentUuid: null, message: { role: "user", content: "plan" } }, + exitPlanEntry("entry-old", "root", "tool-old"), + exitPlanEntry("entry-after-compact", null, "tool-after-compact"), + ], + { plan }, + ); + recordPlanApprovalForSubmission({ ...context(dir, oldOccurrence), approved: true }); + + expect(shouldReusePlanApproval(context(dir, compactedOccurrence))).toBe(false); + }); + + test("fails open for a corrupt approval store", () => { + const dir = dataDir(); + const occurrence = findActiveExitPlanModeOccurrence( + [exitPlanEntry("entry-a", null, "tool-a")], + { plan }, + ); + const input = context(dir, occurrence); + recordPlanApprovalForSubmission({ ...input, approved: true }); + const decisionsDir = join(dir, "plan-decisions"); + writeFileSync(join(decisionsDir, readdirSync(decisionsDir)[0]), "{not-json"); + + expect(shouldReusePlanApproval(input)).toBe(false); + }); + + test("does not reuse when disabled or when occurrence identity is unavailable", () => { + const dir = dataDir(); + const occurrence = findActiveExitPlanModeOccurrence( + [exitPlanEntry("entry-a", null, "tool-a")], + { plan }, + ); + const input = context(dir, occurrence); + recordPlanApprovalForSubmission({ ...input, approved: true }); + + expect(shouldReusePlanApproval({ ...input, enabled: false })).toBe(false); + expect(shouldReusePlanApproval({ ...input, occurrence: null })).toBe(false); + }); + + test("never persists a denial for a later retry", () => { + const dir = dataDir(); + const occurrence = findActiveExitPlanModeOccurrence( + [exitPlanEntry("entry-a", null, "tool-a")], + { plan }, + ); + const input = context(dir, occurrence); + + recordPlanApprovalForSubmission({ ...input, approved: false }); + + expect(shouldReusePlanApproval(input)).toBe(false); + }); +}); diff --git a/apps/hook/server/plan-decision-policy.ts b/apps/hook/server/plan-decision-policy.ts new file mode 100644 index 000000000..d04e7608b --- /dev/null +++ b/apps/hook/server/plan-decision-policy.ts @@ -0,0 +1,59 @@ +import { + getPlanApproval, + recordPlanApproval, +} from "./plan-decision-store"; +import type { ActiveExitPlanModeOccurrence } from "./session-log"; + +interface PlanApprovalReuseContext { + enabled: boolean; + isGemini: boolean; + project: string; + sessionId: string; + plan: string; + occurrence: ActiveExitPlanModeOccurrence | null; + baseDir?: string; + now?: number; +} + +export function shouldReusePlanApproval(context: PlanApprovalReuseContext): boolean { + if ( + !context.enabled || + context.isGemini || + !context.occurrence || + !context.sessionId + ) { + return false; + } + + return !!getPlanApproval( + context.project, + context.sessionId, + context.plan, + context.occurrence.key, + context.baseDir, + context.now, + ); +} + +export function recordPlanApprovalForSubmission( + context: PlanApprovalReuseContext & { approved: boolean }, +): void { + if ( + !context.enabled || + context.isGemini || + !context.approved || + !context.occurrence || + !context.sessionId + ) { + return; + } + + recordPlanApproval( + context.project, + context.sessionId, + context.plan, + context.occurrence.key, + context.baseDir, + context.now, + ); +} diff --git a/apps/hook/server/plan-decision-store.test.ts b/apps/hook/server/plan-decision-store.test.ts index dc59630e7..fb11b12b0 100644 --- a/apps/hook/server/plan-decision-store.test.ts +++ b/apps/hook/server/plan-decision-store.test.ts @@ -1,9 +1,21 @@ import { afterEach, describe, expect, test } from "bun:test"; -import { existsSync, mkdtempSync, readdirSync, rmSync, utimesSync } from "node:fs"; +import { + existsSync, + mkdtempSync, + readFileSync, + readdirSync, + rmSync, + utimesSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { getPlanDecision, recordPlanDecision } from "./plan-decision-store"; +import { + APPROVAL_REUSE_MAX_AGE_MS, + getPlanApproval, + recordPlanApproval, +} from "./plan-decision-store"; const dirs: string[] = []; const tmp = (): string => { @@ -19,82 +31,123 @@ afterEach(() => { const P = "my-project"; describe("plan-decision-store (#1075)", () => { - test("returns null before any decision is recorded", () => { - expect(getPlanDecision(P, "sess-a", "# Plan\n", tmp())).toBeNull(); + test("keeps the source file free of raw NUL bytes", () => { + const source = readFileSync( + join(process.cwd(), "apps", "hook", "server", "plan-decision-store.ts"), + ); + expect(source.includes(0)).toBe(false); }); - test("records and returns an approval for the same project, session, and plan", () => { + test("returns null before any approval is recorded", () => { + expect(getPlanApproval(P, "sess-a", "# Plan\n", "occurrence-a", tmp(), 1_000)).toBeNull(); + }); + + test("records an approval only for the same project, session, plan, and occurrence", () => { const dir = tmp(); - recordPlanDecision(P, "sess-a", "# Plan\n\n- step", { decision: "approved" }, dir); - expect(getPlanDecision(P, "sess-a", "# Plan\n\n- step", dir)).toEqual({ - decision: "approved", - }); + recordPlanApproval(P, "sess-a", "# Plan\n\n- step", "occurrence-a", dir, 1_000); + expect( + getPlanApproval(P, "sess-a", "# Plan\n\n- step", "occurrence-a", dir, 1_001), + ).toEqual({ occurrence: "occurrence-a", approvedAt: 1_000 }); + expect( + getPlanApproval(P, "sess-a", "# Plan\n\n- step", "occurrence-b", dir, 1_001), + ).toBeNull(); }); test("matches plans after normalizing CRLF and surrounding whitespace", () => { const dir = tmp(); - recordPlanDecision(P, "sess-a", "# Plan\n\n- a\n- b", { decision: "approved" }, dir); + recordPlanApproval(P, "sess-a", "# Plan\n\n- a\n- b", "occurrence-a", dir, 1_000); // A Windows resubmission (CRLF) with a trailing blank line still matches. expect( - getPlanDecision(P, "sess-a", " # Plan\r\n\r\n- a\r\n- b\r\n", dir), - ).toEqual({ decision: "approved" }); + getPlanApproval( + P, + "sess-a", + " # Plan\r\n\r\n- a\r\n- b\r\n", + "occurrence-a", + dir, + 1_001, + ), + ).toEqual({ occurrence: "occurrence-a", approvedAt: 1_000 }); }); test("does not match a plan whose body actually changed", () => { const dir = tmp(); - recordPlanDecision(P, "sess-a", "# Plan\n- a", { decision: "approved" }, dir); - expect(getPlanDecision(P, "sess-a", "# Plan\n- a\n- b", dir)).toBeNull(); + recordPlanApproval(P, "sess-a", "# Plan\n- a", "occurrence-a", dir, 1_000); + expect( + getPlanApproval(P, "sess-a", "# Plan\n- a\n- b", "occurrence-a", dir, 1_001), + ).toBeNull(); }); - test("keeps decisions separate per project, session, and plan", () => { + test("keeps approvals separate per project, session, and plan", () => { const dir = tmp(); - recordPlanDecision(P, "sess-a", "plan one", { decision: "approved" }, dir); - expect(getPlanDecision("other-project", "sess-a", "plan one", dir)).toBeNull(); - expect(getPlanDecision(P, "sess-b", "plan one", dir)).toBeNull(); - expect(getPlanDecision(P, "sess-a", "plan two", dir)).toBeNull(); + recordPlanApproval(P, "sess-a", "plan one", "occurrence-a", dir, 1_000); + expect(getPlanApproval("other-project", "sess-a", "plan one", "occurrence-a", dir, 1_001)).toBeNull(); + expect(getPlanApproval(P, "sess-b", "plan one", "occurrence-a", dir, 1_001)).toBeNull(); + expect(getPlanApproval(P, "sess-a", "plan two", "occurrence-a", dir, 1_001)).toBeNull(); }); - test("records a denial together with its feedback", () => { + test("treats legacy denial and malformed records as absent", () => { const dir = tmp(); - recordPlanDecision(P, "sess-a", "plan", { decision: "denied", feedback: "add tests" }, dir); - expect(getPlanDecision(P, "sess-a", "plan", dir)).toEqual({ - decision: "denied", - feedback: "add tests", - }); + recordPlanApproval(P, "sess-a", "plan", "occurrence-a", dir, 1_000); + const file = join(dir, "plan-decisions", readdirSync(join(dir, "plan-decisions"))[0]); + const key = Object.keys(JSON.parse(readFileSync(file, "utf-8")))[0]; + + writeFileSync( + file, + JSON.stringify({ + [key]: { decision: "denied", feedback: "add tests" }, + malformed: { occurrence: 42, approvedAt: "now" }, + }), + ); + + expect(getPlanApproval(P, "sess-a", "plan", "occurrence-a", dir, 1_001)).toBeNull(); }); - test("a later decision for the same plan overwrites the earlier one", () => { + test("does not reuse approvals older than the freshness window", () => { const dir = tmp(); - recordPlanDecision(P, "sess-a", "plan", { decision: "denied", feedback: "no" }, dir); - recordPlanDecision(P, "sess-a", "plan", { decision: "approved" }, dir); - expect(getPlanDecision(P, "sess-a", "plan", dir)).toEqual({ decision: "approved" }); + recordPlanApproval(P, "sess-a", "plan", "occurrence-a", dir, 1_000); + expect( + getPlanApproval( + P, + "sess-a", + "plan", + "occurrence-a", + dir, + 1_000 + APPROVAL_REUSE_MAX_AGE_MS + 1, + ), + ).toBeNull(); }); test("an empty session id never records or matches (safe fallback)", () => { const dir = tmp(); - recordPlanDecision(P, "", "plan", { decision: "approved" }, dir); - expect(getPlanDecision(P, "", "plan", dir)).toBeNull(); + recordPlanApproval(P, "", "plan", "occurrence-a", dir, 1_000); + expect(getPlanApproval(P, "", "plan", "occurrence-a", dir, 1_001)).toBeNull(); }); test("tolerates session ids that are file paths or contain separators", () => { const dir = tmp(); const pathLikeId = "/Users/x/.codex/sessions/2026/07/rollout-abc.jsonl"; - recordPlanDecision(P, pathLikeId, "plan", { decision: "approved" }, dir); - expect(getPlanDecision(P, pathLikeId, "plan", dir)).toEqual({ decision: "approved" }); + recordPlanApproval(P, pathLikeId, "plan", "occurrence-a", dir, 1_000); + expect(getPlanApproval(P, pathLikeId, "plan", "occurrence-a", dir, 1_001)).toEqual({ + occurrence: "occurrence-a", + approvedAt: 1_000, + }); }); test("prunes session files older than the retention window on write", () => { const dir = tmp(); - recordPlanDecision(P, "old-session", "plan", { decision: "approved" }, dir); + recordPlanApproval(P, "old-session", "plan", "occurrence-a", dir, 1_000); const decisionsDir = join(dir, "plan-decisions"); const [oldFile] = readdirSync(decisionsDir); const oldPath = join(decisionsDir, oldFile); const ancientSeconds = Date.now() / 1000 - 8 * 24 * 60 * 60; utimesSync(oldPath, ancientSeconds, ancientSeconds); - recordPlanDecision(P, "new-session", "plan", { decision: "approved" }, dir); + recordPlanApproval(P, "new-session", "plan", "occurrence-a", dir, 1_000); expect(existsSync(oldPath)).toBe(false); - expect(getPlanDecision(P, "new-session", "plan", dir)).toEqual({ decision: "approved" }); - expect(getPlanDecision(P, "old-session", "plan", dir)).toBeNull(); + expect(getPlanApproval(P, "new-session", "plan", "occurrence-a", dir, 1_001)).toEqual({ + occurrence: "occurrence-a", + approvedAt: 1_000, + }); + expect(getPlanApproval(P, "old-session", "plan", "occurrence-a", dir, 1_001)).toBeNull(); }); }); diff --git a/apps/hook/server/plan-decision-store.ts b/apps/hook/server/plan-decision-store.ts index 768015a41a7f3da5037bcc2cf7546177195eb239..66263fddfe19ab9b4c6f8474bf036c8f45566299 100644 GIT binary patch literal 5410 zcmcgwZFAek5&o`UvF3OZP(y-@lemc-$z@_YRU5||sjfTYy7mb;h-cw#a0f{&#sA)C z?*#xEDs8`1A1DB~x3{P$~H%i{d_{iVSt(}@gBk5YMWJ;GSiu^7n zR8~zPa`i8nPHR|AOQDxz5@kx-R1Ku7QqrF<-uz0L$|WnTWkbOUZJFYYjZ1|Ql&U7C zmp2kSgl&y5>?oz_Qff1m8Fp=sNKhtJ-ol)vY^=tj)rLxmd34!GaZT0|w~YxYL!Vio zkfPLfEjwI@74-Ve!cK7|RLPs39jKzlO0=c~+Kl`Ywltvzd#Z}JZ%<#)hO(wAXjCFZ zM@h4)O*MMf%{m~GcS;+5u_}`Z-imANE4)Bs`U;*HzHbeeh-WuggePr;3D38UGP2jP zWlg3BmeA))mD|?V!Ym=t8B15vqOGj3K-IiJ6ke#NoAXC@l^>hwB`}cAI8sxXyrrgt z!5Jp}Ro@I_3z;I~yTY8rft5 zNOvfeDKEe9NkeNmzn^#nL^go?GkkUH$#elcm2Gh;n`Z}`gM*|h0hEikKfX9Ur(+sf z%Tt*s&RF&=8-3vunDMNs${m6MeX|5qIZf)OEgAVW8`+k?0un3F0qT-$@a!JkVy_o} z|G4=1<;T|-Snv->k0;;Y=jlBBd^MlX-MZ{7&VA=eXv?ZRuWmJ|qL8TqQ1jI@p9YPD z=_{lm+vkWQxOnJ4XTsv_?EKB&7QcKvfBE(TmJBWWcH0i`q0S|Ym1Bx7g_d}H9$Y*Q z4|IG2Z!-*o3_X9oA9K-o;ypj^&|zJ?(nc0jEi!2-gg8M+Cm@Ivghvq=^0Q3LB#;X9 z=!(p3g*ey|&8%aV!jPp13uCZI;2@P+o^<@d=}A%~ORB(1&6k?Gy@G{r?RWBz#EEwK4Cj#G67%8f=#d4~xa zg+WI134Q%!yoo>Ss{F(&gg9}|Ip7zebEfHM%fpN1>+)|s$D0<|??(fO0YKdLD_eaNIuj-QXVni5RG>urrO56W# zt?`)oq-{!+(_BktMduVyGB+m2UX=(I{NU(w5^Ox-DPBcn0r$o(7^6$jk<&7cb6H-Q zB^|?{C-lV^R?eI1MB<*It<=aZGYZ1LkqRnLXdYy?753!r6gqeYY{Q3uZ?{9Djdh;;^`P!H%rYAk36Pr)ke6^9+#JA9zMsiJ zRLj28=AGNNK;EH64{dr+WfOux+2;AMDtA=jd$Wb_XE@1r>jFM?#2sMA+&rW+e(#fP zD79O+SrYUE-=GncF&M|9%$?VcCy0=rw~l= z>bN{cT@gl?5jqLfdGC#`^q;0S32X}VJ-E%2hZYM##)5-Pc2nN_`rtVI16#PfIj|=1C|BB#&TYm%On+omT8V<1- z`JWKms~t0<`&asb=AoD}MsjJ$6m>|nkSE7BU07h3=+UtOetR*_;)4?;H;EG*OySzL z(KzxSL0KzlAMGDI#rsP2Tk=p+=u+*UOHsGOM;{| zRhA*feU$^>_UB#>2`91dN`j*IwgU9Y>xX?xAk%F4LdNZD7+5j0@t`5Hz(gbDl?7K@ zdg8HImZ^S+3%h7!PhRNrI1h{}TidgT4CRw2EU`yzOA<*6QzX8ND_h$eA delta 1200 zcmZuxPiqrF6gQ}~Zf$E?kRB?&u28dSnzV|Di6K<0pwcR~2x66Nb|>j*va`(0<_~Sm z2k;^co;)df^CAKL3SOiKK@WZg@hkXdHybPMX?Neud-MA@`!4+jUY+gvb=w1%I8;#P zi3|t^HiSjH&WY`I!1jI4n#2Xhaid4^jd}*8L2eTec7uRjBc237gq{IZfX5_ovdBr3 zk{0eyQ6T~X=a-kRT(+{X**O*Jn z5h@W>8dORgDqGYc8SHQo@eSP3hVq7Fei>xIJsfFEJ<+8u;t88Q>eZkUAXG`~br24> zGd>X^5nqF40e)cf1px#0i&`2NPt%fo>Q5y4-x6bqUM`)66{P4et`}J#9fm?wQ6lD% z2r2c#_kk=~NaqaF_8CXE9iPC1L9^z!I?x}A2EZ=t3=PSH8Ov`%cepqo*@df-A!Y%p zgt!&kd0xsRJFba#3+ltsH~p{2ekKw})Z3BYm)0ofjHBYL6>@G89y*5NOU65RsvGzM zX@w$(ZmO3j)-P)ASx)fbkW_AqZ+W>|2Fxwbw6b_sBV}1xm)3}oDRY~&O{<{3pIjM@ zS*cm$-c?RQSA3fb6l{Jz6oFM?P$(9QV6w_H;z$!paWxM+UG-;rM*SE|u5V)8;-JR% zK*M$;*>!?|v~5QY)elRUT|E>Q;(TT>$CwK>dur|T&4_v3RtD3!vk1*2;iIwg_Kq1;38`i||-`F_FiZiSn}i**3J z4)nqMm4)l5{UWJW)!xL?f2EZgoz^aDs$X`NUFoD4(1#ABVIyQGuSPwNSG#NBEp`TDqn{~oD6CeNu~$;{|* RX>w{o-I;pY- diff --git a/apps/hook/server/plan-normalization.ts b/apps/hook/server/plan-normalization.ts new file mode 100644 index 000000000..26ddccbb2 --- /dev/null +++ b/apps/hook/server/plan-normalization.ts @@ -0,0 +1,3 @@ +export function normalizePlanText(plan: string): string { + return plan.replace(/\r\n/g, "\n").trim(); +} diff --git a/apps/hook/server/session-log.test.ts b/apps/hook/server/session-log.test.ts index 458a1562f..c65936092 100644 --- a/apps/hook/server/session-log.test.ts +++ b/apps/hook/server/session-log.test.ts @@ -16,6 +16,7 @@ import { extractRecentRenderedMessages, getRecentRenderedMessages, resolveActiveBranchIndices, + findActiveExitPlanModeOccurrence, findDroidSessionLogsForCwd, resolveDroidSessionLogForCwd, projectSlugFromCwd, @@ -80,6 +81,27 @@ function assistantToolUse( }); } +function exitPlanModeToolUse(toolUseId: string, plan: string): string { + return JSON.stringify({ + type: "assistant", + message: { + id: `msg_${toolUseId}`, + role: "assistant", + content: [ + { + type: "tool_use", + id: toolUseId, + name: "ExitPlanMode", + input: { plan }, + }, + ], + stop_reason: "tool_use", + }, + uuid: crypto.randomUUID(), + parentUuid: crypto.randomUUID(), + }); +} + /** Assistant entry with both text and tool_use */ function assistantTextAndToolUse( msgId: string, @@ -863,6 +885,74 @@ describe("resolveActiveBranchIndices", () => { }); }); +describe("findActiveExitPlanModeOccurrence", () => { + test("identifies the exact active ExitPlanMode tool occurrence", () => { + const plan = "# Plan\n- step"; + const entries = parseSessionLog( + buildLog(userPrompt("make a plan"), exitPlanModeToolUse("toolu_plan", plan)), + ); + + expect( + findActiveExitPlanModeOccurrence(entries, { plan, toolUseId: "toolu_plan" }), + ).toMatchObject({ + toolUseId: "toolu_plan", + entryUuid: entries[1].uuid, + }); + }); + + test("uses the live occurrence after a rewind instead of the orphaned occurrence", () => { + const plan = "# Same plan"; + const entries = parseSessionLog( + buildRewoundLog({ + kept: [userPrompt("make a plan")], + abandoned: [exitPlanModeToolUse("toolu_orphaned", plan)], + resumed: [exitPlanModeToolUse("toolu_live", plan)], + }), + ); + + expect(findActiveExitPlanModeOccurrence(entries, { plan })).toMatchObject({ + toolUseId: "toolu_live", + }); + }); + + test("does not resolve an occurrence before a compact boundary", () => { + const plan = "# Same plan"; + const preCompact = linkChain([ + userPrompt("make a plan"), + exitPlanModeToolUse("toolu_before_compact", plan), + ]); + const postCompact = linkChain( + [exitPlanModeToolUse("toolu_after_compact", plan)], + null, + ); + const entries = parseSessionLog([...preCompact, ...postCompact].join("\n")); + + expect(findActiveExitPlanModeOccurrence(entries, { plan })).toMatchObject({ + toolUseId: "toolu_after_compact", + }); + }); + + test("fails open when the active transcript cannot identify a tool occurrence", () => { + const entries = parseSessionLog( + JSON.stringify({ + type: "assistant", + message: { + role: "assistant", + content: [ + { + type: "tool_use", + name: "ExitPlanMode", + input: { plan: "# Plan" }, + }, + ], + }, + }), + ); + + expect(findActiveExitPlanModeOccurrence(entries, { plan: "# Plan" })).toBeNull(); + }); +}); + describe("extractRecentRenderedMessages — after a rewind", () => { const rewoundLog = () => buildRewoundLog({ diff --git a/apps/hook/server/session-log.ts b/apps/hook/server/session-log.ts index e209e462f..39001b082 100644 --- a/apps/hook/server/session-log.ts +++ b/apps/hook/server/session-log.ts @@ -20,6 +20,8 @@ import { spawnSync } from "node:child_process"; import { join, dirname, basename } from "node:path"; import { homedir } from "node:os"; +import { normalizePlanText } from "./plan-normalization"; + const claudeConfigDir = process.env.CLAUDE_CONFIG_DIR || join(homedir(), ".claude"); const DEFAULT_SESSIONS_DIR = join(claudeConfigDir, "sessions"); @@ -76,6 +78,12 @@ export interface RenderedMessage { timestamp?: string; } +export interface ActiveExitPlanModeOccurrence { + entryUuid: string; + toolUseId: string; + key: string; +} + // --- Session File Discovery --- /** @@ -777,6 +785,74 @@ export function resolveActiveBranchIndices( } } +/** + * Find the live Claude ExitPlanMode tool occurrence for a hook payload. The + * transcript branch and stable entry/tool ids bind reuse to a submission, not + * merely to equal plan text. + */ +export function findActiveExitPlanModeOccurrence( + entries: SessionLogEntry[], + opts: { plan: string; toolUseId?: string }, +): ActiveExitPlanModeOccurrence | null { + const branchIndices = resolveActiveBranchIndices(entries); + if (!branchIndices) return null; + + const expectedPlan = normalizePlanText(opts.plan); + for (let i = entries.length - 1; i >= 0; i--) { + if (!branchIndices.has(i)) continue; + const entry = entries[i]; + const entryUuid = entry?.uuid; + if (typeof entryUuid !== "string" || !entryUuid) continue; + if (getEntryRole(entry) !== "assistant") continue; + const content = entry.message?.content; + if (!Array.isArray(content)) continue; + + for (let blockIndex = content.length - 1; blockIndex >= 0; blockIndex--) { + const block = content[blockIndex]; + if (!block || block.type !== "tool_use") continue; + const toolUseId = typeof block.id === "string" ? block.id : ""; + const toolName = typeof block.name === "string" ? block.name : ""; + const input = block.input; + const submittedPlan = + input && typeof input === "object" && typeof (input as { plan?: unknown }).plan === "string" + ? (input as { plan: string }).plan + : null; + if ( + !toolUseId || + toolName !== "ExitPlanMode" || + submittedPlan === null || + normalizePlanText(submittedPlan) !== expectedPlan || + (opts.toolUseId && toolUseId !== opts.toolUseId) + ) { + continue; + } + + return { + entryUuid, + toolUseId, + key: JSON.stringify([entryUuid, toolUseId]), + }; + } + } + + return null; +} + +/** Read and resolve a live ExitPlanMode occurrence, failing open on any issue. */ +export function findActiveExitPlanModeOccurrenceInTranscript( + transcriptPath: string, + opts: { plan: string; toolUseId?: string }, +): ActiveExitPlanModeOccurrence | null { + try { + return findActiveExitPlanModeOccurrence( + parseSessionLog(readFileSync(transcriptPath, "utf-8")), + opts, + ); + } catch { + return null; + } +} + /** * Extract up to `limit` of the most recent rendered assistant messages. * diff --git a/apps/marketing/src/content/docs/reference/environment-variables.md b/apps/marketing/src/content/docs/reference/environment-variables.md index ad0c7aac4..6e8d24040 100644 --- a/apps/marketing/src/content/docs/reference/environment-variables.md +++ b/apps/marketing/src/content/docs/reference/environment-variables.md @@ -26,6 +26,7 @@ All Plannotator environment variables and their defaults. | `PLANNOTATOR_GUIDE_VIEWER_URL` | `https://guides.show/v1/` | Base URL of the viewer that downloaded portable guides pin. Must be `https:` (or `http:` on localhost). Also `--viewer-url` on `plannotator guide export`. | | `PLANNOTATOR_GUIDE_SHARE_URL` | `https://guides.show` | Base URL of the guide host that Guided Review share links are created on. Set this to your own deployment of the `apps/guides-show` Cloudflare Worker. Also `{ "guideShareUrl": "..." }` in `~/.plannotator/config.json`; the env var takes precedence. | | `PLANNOTATOR_DATA_DIR` | `~/.plannotator` | Override the base directory for Plannotator-managed files (plans, history, drafts, config, hooks, sessions).* Some UI preferences remain in functional browser cookies. When unset, an existing `~/.plannotator` is always used; if it doesn't exist and `$XDG_DATA_HOME` is set to an absolute path, `$XDG_DATA_HOME/plannotator` is used; otherwise `~/.plannotator`. (The XDG spec's implicit `~/.local/share` default is deliberately not applied — only an explicitly-set `$XDG_DATA_HOME` moves the directory.) | +| `PLANNOTATOR_PLAN_DECISION_REUSE` | enabled | **Hook-runtime only.** Set to `0` or `false` to make every plan open a review. Otherwise, only an approval for the same active Claude `ExitPlanMode` occurrence is reused during a short retry window, with a visible hook message. New identical occurrences, rewound/compacted history, missing transcript identity, and all denials open a fresh review. Codex filters proposed plans to the current turn and never replays a decision. Also configurable with `{ "planDecisionReuse": false }` in `~/.plannotator/config.json`; the environment variable takes precedence. | | `PLANNOTATOR_PLAN_TIMEOUT_SECONDS` | `345600` | OpenCode only. `submit_plan` wait timeout in seconds. Set `0` to disable timeout. | | `PLANNOTATOR_TODO_PROVIDER` | auto | Pi/oh-my-pi only. Set to `off` (or `0` / `false` / `disabled`) to stop mirroring the approved plan checklist into an editable todo provider during execution. When enabled, Plannotator syncs the checklist only if a provider is detected — currently [pi-todos](https://github.com/mitsuhiko/agent-stuff), detected by its todo directory existing (`.pi/todos` by default, or wherever `PI_TODO_PATH` redirects it when set). The mirror is additive: the progress widget behaves the same either way, and sync is one-way, so edits made in `/todos` never feed back into plan execution. Can also be set via `~/.plannotator/config.json` (`{ "todoProvider": "off" }`); the env var takes precedence. | From 66ad8b4ea7d1a4d3464f4e3472f9683c19bd9e87 Mon Sep 17 00:00:00 2001 From: rNoz Date: Fri, 31 Jul 2026 21:37:27 +0200 Subject: [PATCH 3/4] fix(codex): require Stop turn identity Missing Stop turn IDs cannot safely distinguish stale assistant plans. --- apps/hook/server/codex-session.test.ts | 35 ++++++++++++++++++- apps/hook/server/codex-session.ts | 6 +++- apps/hook/server/plan-decision-policy.test.ts | 3 +- apps/hook/server/plan-decision-store.test.ts | 4 +-- 4 files changed, 42 insertions(+), 6 deletions(-) diff --git a/apps/hook/server/codex-session.test.ts b/apps/hook/server/codex-session.test.ts index a1d998e57..d2f5bce91 100644 --- a/apps/hook/server/codex-session.test.ts +++ b/apps/hook/server/codex-session.test.ts @@ -71,10 +71,11 @@ function sessionMeta(): string { }); } -function turnContext(): string { +function turnContext(turnId?: string): string { return rolloutLine("turn_context", { cwd: "/tmp/test", model: "o3", + ...(turnId && { turn_id: turnId }), }); } @@ -470,6 +471,38 @@ describe("getLatestCodexPlan", () => { expect(getLatestCodexPlan(path, { turnId: requestedTurnId })).toBeNull(); }); + test("does not scrape an assistant proposed plan for a Stop event without a turn id", () => { + const completedTurnId = "turn-completed"; + const path = writeTempRollout( + buildRollout( + sessionMeta(), + turnStarted(completedTurnId), + assistantMessage("\nPrevious turn plan\n"), + turnCompleted(completedTurnId), + ), + ); + + expect(getLatestCodexPlan(path, { stopHookActive: true })).toBeNull(); + }); + + test("keeps the active task id when a turn context has no id", () => { + const turnId = "turn-with-context"; + const path = writeTempRollout( + buildRollout( + sessionMeta(), + turnStarted(turnId), + turnContext(), + eventMsg("task_started"), + assistantMessage("\nCurrent turn plan\n"), + ), + ); + + expect(getLatestCodexPlan(path, { turnId })).toEqual({ + text: "Current turn plan", + source: "assistant-message", + }); + }); + test("returns null when Stop re-entry has no revised plan after the hook prompt", () => { const turnId = "turn-stop-no-revision"; const path = writeTempRollout( diff --git a/apps/hook/server/codex-session.ts b/apps/hook/server/codex-session.ts index ccb33c1d9..2fefda2f8 100644 --- a/apps/hook/server/codex-session.ts +++ b/apps/hook/server/codex-session.ts @@ -297,7 +297,8 @@ function collectPlanCandidates( (entry.type === "event_msg" && TURN_START_TYPES.has(entry.payload?.type || "")) || entry.type === "turn_context" ) { - activeTurnId = getTurnId(entry); + const boundaryTurnId = getTurnId(entry); + if (boundaryTurnId) activeTurnId = boundaryTurnId; } const planItemText = getPlanItemText(entry, turnId); @@ -430,6 +431,9 @@ export function getLatestCodexPlan( ): CodexPlanResult | null { const entries = parseRolloutEntries(rolloutPath); if (entries.length === 0) return null; + // Stop payloads without a turn id cannot safely distinguish a new plan from + // a previous assistant ; fail closed instead of resurfacing it. + if (!options.turnId) return null; const turnStartIndex = findTurnStartIndex(entries, options.turnId); const candidates = collectPlanCandidates( diff --git a/apps/hook/server/plan-decision-policy.test.ts b/apps/hook/server/plan-decision-policy.test.ts index f7ca47232..f6ec4a0cb 100644 --- a/apps/hook/server/plan-decision-policy.test.ts +++ b/apps/hook/server/plan-decision-policy.test.ts @@ -1,5 +1,6 @@ import { afterEach, describe, expect, test } from "bun:test"; import { mkdtempSync, readdirSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -13,7 +14,7 @@ import { const dirs: string[] = []; const dataDir = (): string => { - const dir = mkdtempSync(join(process.cwd(), ".plan-decision-policy-test-")); + const dir = mkdtempSync(join(tmpdir(), "plan-decision-policy-test-")); dirs.push(dir); return dir; }; diff --git a/apps/hook/server/plan-decision-store.test.ts b/apps/hook/server/plan-decision-store.test.ts index fb11b12b0..8e804dfee 100644 --- a/apps/hook/server/plan-decision-store.test.ts +++ b/apps/hook/server/plan-decision-store.test.ts @@ -32,9 +32,7 @@ const P = "my-project"; describe("plan-decision-store (#1075)", () => { test("keeps the source file free of raw NUL bytes", () => { - const source = readFileSync( - join(process.cwd(), "apps", "hook", "server", "plan-decision-store.ts"), - ); + const source = readFileSync(join(import.meta.dir, "plan-decision-store.ts")); expect(source.includes(0)).toBe(false); }); From e9e7f3e9d365d9e7384e3063fc45785ae7ccb0ef Mon Sep 17 00:00:00 2001 From: rNoz Date: Fri, 31 Jul 2026 22:15:21 +0200 Subject: [PATCH 4/4] fix(codex): log unsafe Stop skips Missing turn identity skips are visible only with PLANNOTATOR_DEBUG. --- apps/hook/server/codex-session.test.ts | 43 ++++++++++++++++++- apps/hook/server/codex-session.ts | 57 ++++++++++++++++++++------ apps/hook/server/index.ts | 20 ++++++++- apps/hook/server/session-log.test.ts | 26 +++++++++++- 4 files changed, 129 insertions(+), 17 deletions(-) diff --git a/apps/hook/server/codex-session.test.ts b/apps/hook/server/codex-session.test.ts index d2f5bce91..f74380b96 100644 --- a/apps/hook/server/codex-session.test.ts +++ b/apps/hook/server/codex-session.test.ts @@ -10,7 +10,13 @@ import { describe, expect, test, afterEach } from "bun:test"; import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { findCodexRolloutByThreadId, getLastCodexMessage, getLatestCodexPlan } from "./codex-session"; +import { + findCodexRolloutByThreadId, + getCodexStopSkipReason, + getLastCodexMessage, + getLatestCodexPlan, + logCodexStopSkip, +} from "./codex-session"; // --- Fixture Helpers --- @@ -389,6 +395,7 @@ describe("getLatestCodexPlan", () => { text: "Authoritative plan item", source: "plan-item", }); + }); test("falls back to raw proposed_plan blocks for plan-only assistant replies", () => { @@ -408,6 +415,40 @@ describe("getLatestCodexPlan", () => { }); }); + describe("Codex Stop skip diagnostics", () => { + test("classifies a missing Stop turn id without reading stale plan content", () => { + expect(getCodexStopSkipReason("not-read.jsonl")).toBe("missing-turn-id"); + }); + + test("requires an id-carrying rollout turn marker", () => { + const turnId = "turn-without-marker"; + const path = writeTempRollout( + buildRollout( + sessionMeta(), + turnStarted("other-turn"), + completedPlanItem("Plan item without matching start marker", turnId), + ), + ); + + expect(getCodexStopSkipReason(path, turnId)).toBe("missing-turn-marker"); + }); + + test("writes the exact skip breadcrumb only when debug is enabled", () => { + const messages: string[] = []; + const write = (message: string) => messages.push(message); + + logCodexStopSkip("missing-turn-id", { debug: "", write }); + expect(messages).toEqual([]); + + logCodexStopSkip("missing-turn-id", { debug: "1", write }); + logCodexStopSkip("missing-turn-marker", { debug: "1", write }); + expect(messages).toEqual([ + "[DEBUG] Codex Stop plan review skipped: missing Stop payload turn_id.", + "[DEBUG] Codex Stop plan review skipped: missing id-carrying rollout turn marker.", + ]); + }); + }); + test("extracts plan blocks surrounded by assistant prose", () => { const turnId = "turn-prose"; const path = writeTempRollout( diff --git a/apps/hook/server/codex-session.ts b/apps/hook/server/codex-session.ts index 2fefda2f8..d89ed73b2 100644 --- a/apps/hook/server/codex-session.ts +++ b/apps/hook/server/codex-session.ts @@ -60,6 +60,8 @@ export interface GetLatestCodexPlanOptions { stopHookActive?: boolean; } +export type CodexStopSkipReason = "missing-turn-id" | "missing-turn-marker"; + const TURN_START_TYPES = new Set(["task_started", "turn_started"]); const TURN_COMPLETE_TYPES = new Set(["task_complete", "turn_completed"]); const PROPOSED_PLAN_RE = /([\s\S]*?)<\/proposed_plan>/gi; @@ -182,23 +184,26 @@ function findLastIndex( return -1; } -function findTurnStartIndex(entries: RolloutEntry[], turnId?: string): number { - const matchingTurnStart = findLastIndex( +function findMatchingTurnMarkerIndex( + entries: RolloutEntry[], + turnId: string, +): number { + return findLastIndex( entries, (entry) => - entry.type === "event_msg" && - TURN_START_TYPES.has(entry.payload?.type || "") && - (!turnId || entry.payload?.turn_id === turnId) + ( + (entry.type === "event_msg" && TURN_START_TYPES.has(entry.payload?.type || "")) || + entry.type === "turn_context" + ) && + getTurnId(entry) === turnId, ); - if (matchingTurnStart !== -1) return matchingTurnStart; +} - const matchingTurnContext = findLastIndex( - entries, - (entry) => - entry.type === "turn_context" && - (!turnId || entry.payload?.turn_id === turnId) - ); - if (matchingTurnContext !== -1) return matchingTurnContext; +function findTurnStartIndex(entries: RolloutEntry[], turnId?: string): number { + const matchingTurnStart = turnId + ? findMatchingTurnMarkerIndex(entries, turnId) + : -1; + if (matchingTurnStart !== -1) return matchingTurnStart; const lastTurnStart = findLastIndex( entries, @@ -283,6 +288,32 @@ function getTurnId(entry: RolloutEntry): string | null { return typeof turnId === "string" && turnId ? turnId : null; } +export function getCodexStopSkipReason( + rolloutPath: string, + turnId?: string, +): CodexStopSkipReason | null { + if (!turnId) return "missing-turn-id"; + const entries = parseRolloutEntries(rolloutPath); + return findMatchingTurnMarkerIndex(entries, turnId) === -1 + ? "missing-turn-marker" + : null; +} + +export function logCodexStopSkip( + reason: CodexStopSkipReason, + opts: { + debug?: string; + write?: (message: string) => void; + } = {}, +): void { + if (!opts.debug) return; + const detail = + reason === "missing-turn-id" + ? "missing Stop payload turn_id." + : "missing id-carrying rollout turn marker."; + (opts.write ?? console.error)(`[DEBUG] Codex Stop plan review skipped: ${detail}`); +} + function collectPlanCandidates( entries: RolloutEntry[], startIndex: number, diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index e7b43a3a8..8f66bef2c 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -163,7 +163,13 @@ import { findActiveExitPlanModeOccurrenceInTranscript, type RenderedMessage, } from "./session-log"; -import { findCodexRolloutByThreadId, getLatestCodexPlan, getRecentCodexMessages } from "./codex-session"; +import { + findCodexRolloutByThreadId, + getCodexStopSkipReason, + getLatestCodexPlan, + getRecentCodexMessages, + logCodexStopSkip, +} from "./codex-session"; import { recordPlanApprovalForSubmission, shouldReusePlanApproval, @@ -2183,8 +2189,18 @@ if (args[0] === "sessions") { process.exit(0); } + const turnId = + typeof event.turn_id === "string" && event.turn_id + ? event.turn_id + : undefined; + const skipReason = getCodexStopSkipReason(rolloutPath, turnId); + if (skipReason) { + logCodexStopSkip(skipReason, { debug: process.env.PLANNOTATOR_DEBUG }); + process.exit(0); + } + const latestPlan = getLatestCodexPlan(rolloutPath, { - turnId: typeof event.turn_id === "string" ? event.turn_id : undefined, + turnId, stopHookActive: !!event.stop_hook_active, }); diff --git a/apps/hook/server/session-log.test.ts b/apps/hook/server/session-log.test.ts index c65936092..09b0f4501 100644 --- a/apps/hook/server/session-log.test.ts +++ b/apps/hook/server/session-log.test.ts @@ -17,6 +17,7 @@ import { getRecentRenderedMessages, resolveActiveBranchIndices, findActiveExitPlanModeOccurrence, + findActiveExitPlanModeOccurrenceInTranscript, findDroidSessionLogsForCwd, resolveDroidSessionLogForCwd, projectSlugFromCwd, @@ -30,7 +31,7 @@ import { resolveSessionLogByCwdScan, type SessionLogEntry, } from "./session-log"; -import { mkdirSync, writeFileSync, rmSync, utimesSync } from "node:fs"; +import { mkdirSync, mkdtempSync, writeFileSync, rmSync, utimesSync } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; @@ -951,6 +952,29 @@ describe("findActiveExitPlanModeOccurrence", () => { expect(findActiveExitPlanModeOccurrence(entries, { plan: "# Plan" })).toBeNull(); }); + + test("resolves ExitPlanMode from the transcript state before PermissionRequest", () => { + const plan = "# Plan\n- step"; + const dir = mkdtempSync(join(tmpdir(), "plannotator-pre-permission-")); + const transcriptPath = join(dir, "session.jsonl"); + try { + writeFileSync( + transcriptPath, + buildLog( + userPrompt("make a plan"), + exitPlanModeToolUse("toolu_before_permission", plan), + ), + ); + + expect( + findActiveExitPlanModeOccurrenceInTranscript(transcriptPath, { plan }), + ).toMatchObject({ + toolUseId: "toolu_before_permission", + }); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); }); describe("extractRecentRenderedMessages — after a rewind", () => {