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. ---