From 43fb2952fa3960f6df50c791775fce7fdf9b849e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Mon, 17 Aug 2026 23:43:27 +0000 Subject: [PATCH] a test plan for plan mode, for a day that is not today the owner has no time to test any of it, so the checklist has to work for someone with none of today's context: eleven ordered tests, what each should show, and what its failure looks like. test 4 is called out as the one most likely to be wrong - approving ExitPlanMode drops the harness into default rather than back into bypassPermissions, which is the finding that changed the design and therefore the thing the implementation is most likely to have got wrong. also separates what was verified from what was not: the probe runs against a real claude, the turn-stream unit tests, and then everything end to end, which is nothing. the inline 'what to watch' list in chat-plan-mode.md is replaced by a pointer so there is one source of truth, and TODO.md now says live-but-unexercised rather than built-not-run - the restarts are done. Co-Authored-By: Claude Opus 5 --- TODO.md | 39 ++---- docs/chat-plan-mode-test-plan.md | 223 +++++++++++++++++++++++++++++++ docs/chat-plan-mode.md | 26 ++-- 3 files changed, 247 insertions(+), 41 deletions(-) create mode 100644 docs/chat-plan-mode-test-plan.md diff --git a/TODO.md b/TODO.md index cadbb3df..235a83c2 100644 --- a/TODO.md +++ b/TODO.md @@ -402,35 +402,22 @@ outside the example's new README: ## Waiting on the owner to run it -- [ ] **Plan mode is built and has never executed.** Landed `1b811dfa` on 2026-08-17, typechecks, unit - tested at the translation layer — and not one line of it has run on a machine. The owner was away - when it was finished and asked to be reminded, which is what this entry is. +- [ ] **Plan mode is live and has never been exercised.** Landed `1b811dfa` on 2026-08-17; all three + processes were restarted the same evening, so the code IS running. Not one part of it has been used: + not the toggle, not the plan card, not the round trip from the sidecar's pending promise to a browser + and back, not the stored column, not `settleAllPlans`. - **Restart BOTH:** `pm2 restart officer` and `pm2 restart officer-claude-code`. The sidecar holds - every interesting part (the gate, the pending-plan map, the mode control request); restarting only - the server leaves the old sidecar running with no `canUseTool` at all, and the toggle would then - appear to work while changing nothing. Note the server is ALSO carrying two earlier unrestarted - changes — the tier-less-model fix (`773f0e1c`) and the sessions-list work. + **`docs/chat-plan-mode-test-plan.md` is the checklist** — eleven ordered tests, what each should show, + and what its failure looks like. Test 4 (approve) is the one most likely to be wrong: it is the case + that changed the design, because approving `ExitPlanMode` drops the harness into `default` rather than + back into `bypassPermissions`. - **Then, in `/chat`, in this order:** + Safe to leave untested. Plan mode is opt-in and off by default, so a conversation that never touches + the toggle behaves exactly as it did before. The one failure worth knowing in advance is a turn that + never ends after a plan is answered — that would be a leaked `canUseTool` promise, and + `pm2 restart officer-claude-code` clears it. - 1. The clipboard icon by the model picker lights up, and `pm2 logs officer-claude-code` says - `[claude:plan:] plan mode on`. - 2. Ask for something that needs a plan. The agent researches and does NOT edit — a `Write` in plan - mode should reach neither the disk nor the gate. - 3. The plan card renders with Approve / Reject, and **the turn is genuinely parked** while it sits - there (the spinner stays; that is correct, an agent is blocked on a promise in the sidecar). - 4. Approve → the agent continues *in the same turn*, and `plan mode off` follows immediately rather - than at turn end. Reject with a reason → it revises and asks again. - 5. Refresh mid-plan → the card replays with its decision, rather than a conversation that stops dead. - 6. Pick `claude-code/opusplan` with plan mode OFF → the toggle's tooltip says it is resting on Sonnet. - - **The one failure worth watching for** is a turn that never ends after a plan is answered. That would - mean a `canUseTool` promise was left unresolved — the agent waits forever and the stall watchdog - eventually tears the session down. `settleAllPlans` runs on every teardown path plus a 20-minute - timeout to make it impossible, and it is the part with no test behind it. - - Design decisions and the verification they came from: `docs/chat-plan-mode.md`. + Design and the probe results behind it: `docs/chat-plan-mode.md`. ## Known bugs diff --git a/docs/chat-plan-mode-test-plan.md b/docs/chat-plan-mode-test-plan.md new file mode 100644 index 00000000..85d9f1af --- /dev/null +++ b/docs/chat-plan-mode-test-plan.md @@ -0,0 +1,223 @@ +# Plan mode — test plan + +Written 2026-08-17, the day plan mode was built (`1b811dfa`), for an owner who did not have time to test it +that day. Design and the verification behind it: `docs/chat-plan-mode.md`. This file is only "how do I find +out whether it works". + +**Work through it in order.** The later tests assume the earlier ones passed, and test 4 is the one that +matters most. + +--- + +## Before you start + +Everything below is already true as of 2026-08-17 and is listed so you can check rather than assume. + +| thing | how to confirm | +| ----------------------------- | --------------------------------------------------------------------------------------------------------- | +| all three processes restarted | `pm2 jlist` — `officer`, `officer-claude-code`, `officer-anthropic-proxy` should share a recent uptime | +| the schema is applied | `plan_mode boolean NOT NULL DEFAULT false` on `chat_session_models` | +| the frontend is rebuilt | `bun start` runs `prestart` and Bun bundles the SPA at import, so a restart is enough — no separate build | + +Keep a log tailing in a second terminal for the whole session. Almost every expectation below has a line in it: + +```bash +pm2 logs officer-claude-code | grep -E "claude:plan|claude:model|claude:spawn" +``` + +**One warning.** If you drive this from a chat inside Officer, restarting `officer-claude-code` kills the +turn you are in. Test from a chat you do not mind cutting off. + +--- + +## 1. The toggle goes on + +A clipboard icon sits beside the model picker in the composer, left of the model name. + +1. Open `/chat`, start or resume a Claude conversation. +2. Click the clipboard icon. + +**Expect:** the icon lights (primary colour, tinted background). A centred rule appears in the transcript +reading **"Plan mode on — research only"**. The log says `[claude:plan:] plan mode on`. + +**If the icon lights but no log line appears:** the session was not live — the sidecar collects a session +after 30 minutes idle. That is fine and expected; the mode rides along on the next turn instead. Send a +message and check the log then. + +**If nothing happens at all:** the toggle is hidden for OpenCode models on purpose. Check the model picker +says a `claude-code/…` model. + +## 2. Plan mode actually stops the agent editing + +This is the safety property. It was verified against a real `claude` in a scratch directory before any of +this was built, but never inside Officer. + +1. With plan mode on, ask for something that requires an edit — _"add a second line to notes.txt"_, in a + directory where that file exists and you do not mind it changing. + +**Expect:** the agent reads, researches, and proposes. **The file on disk does not change.** You may see a +`Write` or `Edit` tool row go by in the transcript — that is normal and is the harness refusing it upstream, +not an edit that happened. + +**Check the file.** `cat` it. This is the one test whose result you should not take from the UI. + +## 3. The plan arrives, and the turn parks + +**Expect:** a card in the transcript headed **"Plan — awaiting your approval"**, with the plan rendered as +markdown and two buttons, **Reject** and **Approve**. + +**The spinner keeps spinning, and that is correct.** An agent really is blocked on a promise inside the +sidecar waiting for one of those buttons. A turn that looks finished while a plan is pending would be the +bug. + +Leave it a minute and confirm nothing times out or errors. + +## 4. Approve — the one that matters + +This is the case that changed the design, so it is the case most likely to be wrong. + +Approving `ExitPlanMode` makes the harness leave plan mode **into `default`, not back into +`bypassPermissions`** — and the agent carries straight on inside the same turn. Officer restores +`bypassPermissions` from inside the permission gate for exactly that reason. + +1. Click **Approve**. + +**Expect, in this order:** + +- the card changes to **"Plan approved"** and the buttons disappear +- a rule appears reading **"Plan mode off"** +- the composer's clipboard icon goes dark +- **the agent continues working in the same turn** — it does not wait for another message +- the edit it proposed actually happens this time (check the file again) + +**The failure to watch for:** the agent continues but every tool call is slow, or the turn stalls partway +through. That would mean the restore did not land and each tool is round-tripping through the permission +callback. `pm2 logs officer-claude-code` would show +`[claude:plan:] could not restore bypassPermissions: …`. + +## 5. Reject — which is a revision request, not an abort + +1. Start another planning turn (test 1 + 2). +2. On the card, click **Reject**. A text box appears. +3. Type something specific — _"don't touch the config file, only the notes"_ — and click **Send feedback**. + +**Expect:** the card becomes **"Plan rejected"** with your text kept beneath it, and **the agent revises and +proposes a new plan** in the same turn. It should not stop, and it should not start executing. + +The harness treats a denial message as feedback: its own string is _"Please revise your plan based on the +feedback and call ExitPlanMode again."_ So a rejection with no text still works but is much less useful. + +## 6. Refresh while a plan is pending + +The card and its decision are both durable, so this should survive a reload. + +1. Get a plan card on screen and **do not answer it**. +2. Refresh the page. + +**Expect:** the conversation replays with the plan card still there, still offering Approve / Reject, and the +turn still shown as running. + +3. Now click **Approve**. + +**Expect:** it works — the same pending promise is still held in the sidecar, and the decision reaches it. + +**If the buttons do nothing after a refresh,** the socket rebound but the decision is not finding its +session. That is worth reporting with the `requestId` from the card. + +## 7. Stickiness — the reason there is a database column + +1. Turn plan mode **on** in a conversation that has taken at least one turn. +2. Refresh, or navigate away and back. + +**Expect:** the toggle comes back **on**. + +3. Check the row directly: + +```sql +select session_id, model, plan_mode from chat_session_models order by updated_at desc limit 5; +``` + +**Known and deliberate:** toggling plan mode on a **brand-new** conversation that has never taken a turn +stores nothing — there is no row yet and no model to invent for one. The client holds it and writes it with +the first turn. So test this on a conversation with history. + +## 8. Stop while a plan is pending + +1. Get a plan card on screen. +2. Press the stop button instead of answering. + +**Expect:** the turn ends cleanly. The card resolves to rejected, carrying _"The turn was stopped before the +plan was answered."_ No spinner left running. + +This exercises `settleAllPlans`, which is the guard against the worst failure mode in the whole feature — a +promise nobody resolves, which is a turn that never ends. It runs on stop, on session teardown and on a +20-minute timeout. + +## 9. opusplan stops lying + +The reason any of this was built. + +1. In the model picker choose **`claude-code/opusplan`**, with plan mode **off**. +2. Hover the clipboard icon. + +**Expect:** the tooltip reads **"opusplan rests on Sonnet — turn on Plan for Opus"**. + +3. Turn plan mode on and run a planning turn. + +**Expect:** planning is done by Opus and execution after approval by Sonnet. Officer cannot show you which +model answered mid-turn, so the honest check is cost and latency, or `~/.claude/projects/…` transcripts, +where each assistant entry carries its own `message.model`. + +**Officer does not auto-enter plan mode for `opusplan`,** deliberately — neither does the CLI, and every +"what does this function do" becoming an approvable plan would be miserable. + +## 10. The badge in the sessions list + +**Expect:** a conversation left in plan mode shows a small **plan** badge on its row in the sessions list, +beside the model badge. + +It is worth having because `opusplan` with plan mode off is just Sonnet, and the model name alone cannot +tell you that. + +## 11. OpenCode is untouched + +1. Start a chat on an OpenCode model. + +**Expect:** no clipboard icon at all. OpenCode has no plan mode and no permission-mode control request, so a +disabled button there would be promising something that does not exist. + +--- + +## If it misbehaves + +**Plan mode is opt-in and off by default.** Nothing here changes a conversation that never touches the +toggle, so the first mitigation is simply not to use it. + +If a session wedges with a spinner that will not clear: `pm2 restart officer-claude-code` ends it. The +conversation is on disk and resumes. + +To back the whole thing out: `git revert 1b811dfa`, then restart `officer` and `officer-claude-code`. The +`plan_mode` column can stay — it is `NOT NULL DEFAULT false` and nothing else reads it. + +--- + +## What was verified before this, and what was not + +Being explicit, because "tested" means different things for different parts of this. + +**Verified against a real `claude`** (SDK 0.3.233, scratch directory, 2026-08-17 — the probe runs recorded in +`docs/chat-plan-mode.md`): + +- `setPermissionMode('plan')` works on a session created with `bypassPermissions` +- `canUseTool` fires for `ExitPlanMode` in plan mode +- plan mode blocks a `Write` before it reaches the gate or the disk +- passing `canUseTool` unconditionally is free under `bypassPermissions` +- approval exits plan mode into `default`, and the agent continues in the same turn + +**Verified by unit test:** the three plan events translate and persist correctly through `turn-stream`, and +land in the right order relative to the agent's own text (`turn-stream.test.ts`, confirmed to fail when the +flush is removed). + +**Not verified by anything:** all of it end to end. Nothing in this feature has run inside Officer — not the +toggle, not the card, not the round trip from the sidecar's pending promise to a browser and back, not the +stored column, not `settleAllPlans`. That is what this document is for. diff --git a/docs/chat-plan-mode.md b/docs/chat-plan-mode.md index 6f62af86..9269df5f 100644 --- a/docs/chat-plan-mode.md +++ b/docs/chat-plan-mode.md @@ -3,8 +3,9 @@ Agreed with the owner on 2026-08-17, immediately before a context compaction. Written so the build can start from here with nothing re-derived. -**Status: built in `1b811dfa`, not yet run.** Needs `pm2 restart officer` and `pm2 restart officer-claude-code` -before it does anything — see "Sequence" at the end for what to watch on the first plan. +**Status: built in `1b811dfa`, live since 2026-08-17, and never once exercised.** The restarts are done, so +the code is running. Nothing has been tested — work through `docs/chat-plan-mode-test-plan.md` before +believing any of it. --- @@ -150,21 +151,16 @@ exactly this class of thing; a `session.permissionMode` beside it keeps `applyMo and sends it as the denial message, which the harness turns into a revision request. 5. ~~Return to `bypassPermissions` on approval~~ — fired from inside `planGate`, not at turn end. -**Not yet run.** Everything above is on disk and typechecks; none of it has executed on this machine. It -needs `pm2 restart officer` AND `pm2 restart officer-claude-code` — the sidecar holds all of the interesting -half, and restarting only the server would leave the old one running with no `canUseTool` at all. +**Not yet run.** Everything above is on disk, typechecks and is unit tested at the translation layer; none +of it has executed inside Officer. The processes were restarted on 2026-08-17 (`officer`, +`officer-claude-code`, `officer-anthropic-proxy`) so the code IS live — it has simply never been exercised. -Worth watching on the first real plan, in this order: +**The test plan is `docs/chat-plan-mode-test-plan.md`**, written the same day for an owner who had no time +to run it. Eleven ordered tests, what each should show, and what its failure looks like. Test 4 (approve) is +the one most likely to be wrong, because it is the case that changed the design. -- the composer's clipboard icon lights, and `[claude:plan:] plan mode on` appears in - `pm2 logs officer-claude-code` -- the agent researches and refuses to edit — a `Write` in plan mode should reach neither disk nor the gate -- the plan card renders with Approve / Reject, and **the turn is genuinely parked** while it sits there -- approving continues the same turn, and `plan mode off` follows immediately rather than at turn end -- refreshing mid-plan replays the card with its decision, rather than a conversation that stops dead - -The one failure mode to watch for is a turn that never ends after a plan is answered: that would mean a -`canUseTool` promise was left unresolved, and `settleAllPlans` is what is meant to make it impossible. +Do not treat this feature as working until that document has been worked through. It is opt-in and off by +default, so nothing is at risk in the meantime. ---