diff --git a/docs/claude-sidecar-isolation.md b/docs/claude-sidecar-isolation.md new file mode 100644 index 00000000..e53b95d4 --- /dev/null +++ b/docs/claude-sidecar-isolation.md @@ -0,0 +1,338 @@ +> **Historical — kept for its reasoning, not as a description of the system.** +> Written 2026-07-30, before any of it was built. Stages 0–2 are **done and running**: the agent is a PM2 +> peer (`officer-agent`), the Anthropic proxy is separately named, and turn output is written to +> `chat_session_events` by the sidecar that produced it, so it survives an officer restart. R1, R2, R4 and +> R5 are all satisfied. **Stages 3–5 are not started** and are the only live part of this file. +> Everything below is in the present tense of the day it was written. For where the architecture is +> going, read `sidecar-topology.md`. + +# Making agent sessions survive a platform restart + +2026-07-30. A design document, not a plan of record — no code has been changed. Written to be argued +with; the open questions at the end are the parts I think you should decide, not me. + +## The goal, in one sentence + +**Restarting `officer` should cost you a reconnect, never a session.** You need to be able to work on +the platform and on other sidecars — editing, typechecking, `pm2 restart officer` — while an agent +keeps running and keeps its context. + +Today it costs you the session. This document is about why, and what the smallest correct fix is. + +## What actually happens today (verified) + +I checked my own process ancestry from inside this session: + +``` +PM2 God Daemon +└─ bun start + └─ bun src/server.tsx ← the MAIN officer process + └─ bun run src/servers/sidecar/claude/user-instance.ts ← spawned by officer, on demand + └─ claude --resume 0aebeea6-… ← the agent process (this session) +``` + +So the agent that is talking to you is a **grandchild of the process you restart**. That is the whole +bug, and everything below follows from it. + +### The naming discovery — this is the part that matters most + +`ecosystem.config.cjs:9-14` declares a PM2 process called `officer-claude`. It is natural to assume +that is the sidecar running your agent. **It is not.** + +`src/servers/sidecar/claude/index.ts` — the thing `officer-claude` actually runs — registers itself as: + +```ts +name: 'proxy', +capabilities: ['proxy'], +``` + +and its entire job is four things (`index.ts:10-23`): take a PID lock, load state, ensure an Anthropic +proxy secret exists, and start the Anthropic proxy (`startAnthropicProxy()`, `:20`). Its command +handler answers exactly three messages — `ping`, `state:sync`, `proxy:secret` (`:30-48`). **It never +spawns an agent.** + +The process that actually runs agents is `src/servers/sidecar/claude/user-instance.ts`, and it has +**no PM2 entry at all**. Its only launcher is the main process: + +```ts +// sidecar-registry.ts:200 +const USER_INSTANCE_SCRIPT = resolve(import.meta.dir, 'sidecar/claude/user-instance.ts'); +// sidecar-registry.ts:238 +const proc = Bun.spawn(['bun', 'run', USER_INSTANCE_SCRIPT], { env, stdout: 'inherit', stderr: 'inherit' }); +``` + +So: **`officer-claude` is the credential proxy, misleadingly named; the agent sidecar is unmanaged and +unnamed.** `platform/CLAUDE.md` currently says restarting `officer` "does NOT disturb this Claude +session (it runs under the separate `officer-claude` sidecar)" — that sentence is false, and it is +false because of this naming. Worth fixing regardless of what else we do. + +One good consequence of the split: because the Anthropic proxy lives in the PM2-managed `proxy` +sidecar, my **API traffic** already survives an officer restart. It is only my **process** that dies. +The credential path is in roughly the right place; the process topology is not. + +### Why the process dies — two independent mechanisms + +1. **Process-tree kill.** PM2 signals the whole tree on restart, so the agent gets SIGINT even though + nothing in Officer's code asks for it. *(Inferred from PM2's default `treekill: true`; + `ecosystem.config.cjs` sets no `treekill` key, so the default applies. I did not test this in + isolation.)* +2. **Inherited stdio.** `sidecar-registry.ts:240-241` passes `stdout: 'inherit', stderr: 'inherit'`, + so the agent writes into *officer's* PM2 log pipes. When officer restarts those pipes close, and + subsequent writes fail. Even if the signal were suppressed, the child's output path dies with the + parent. + +Both must be fixed. Fixing only the signal leaves a process writing to a closed pipe. + +### Also relevant: the transport direction is inverted + +`user-instance.ts:19` dials *out* to officer: + +```ts +const API_URL = process.env.API_URL ?? `ws://127.0.0.1:${process.env.PORT ?? '5000'}`; +``` + +The agent sidecar is a **client** of officer, registering over `/api/sidecar/register`. It exposes no +listener, reports no port. That is the exact inverse of the compliant sidecars (slskd, music, vault), +which listen on a loopback port, report it on connect, and let officer forward to them. + +This matters for survivability, not just tidiness: when officer restarts, a sidecar that *listens* +just sits there with its work intact and waits to be forwarded to again. A sidecar that *dials in* has +to notice the drop, reconnect, and re-establish identity — and anything it wanted to emit in the +meantime has nowhere to go. + +## Why fixing parentage alone is not enough + +Suppose we do the minimum: give the agent sidecar a PM2 entry and stop officer from spawning it. The +process now survives. Does your session? + +Not yet. Five things have to hold, and only some are about process lifetime: + +| # | Requirement | Status today | +|---|---|---| +| R1 | The agent process is outside officer's process tree | **broken** — child of officer | +| R2 | The agent's stdio does not belong to officer | **broken** — `'inherit'` | +| R3 | The sidecar survives its control socket dropping, and reconnects | **probably fine** — `connect.ts` has a reconnect backoff table; not tested across a real restart | +| R4 | Events emitted while officer is down are not lost | **broken** — see below | +| R5 | The browser reconnects and asks for what it missed | **already works** — verified in Pass 2, see below | + +**R5 is done, and that is better news than I expected.** The frontend already auto-reconnects with +backoff (`hooks/src/useChatWebSocket.ts:46-58`, `Math.min(5000, 300 * retry)`), already tracks a +monotonic sequence cursor off a `seq` field on every message (`useChat.ts:145-146`), and already sends +a replay request on every reconnect: + +```ts +// useChat.ts:282-285 +const sid = sessionIdRef.current; +if (sid) sendRef.current({ type: 'resume-cursor', sessionId: sid, cursor: cursorRef.current }); +``` + +The backend half exists too (`chat/websocket.ts:612-629`, `getChatEventsSince`). There is even a +"Disconnected" indicator in the UI (`ChatHistory/ChatDetailPanel.tsx:38-52`). + +So **the sequence-and-replay protocol I was about to propose building already exists end to end.** The +only thing wrong with it is *who writes the events*. That collapses Stage 2 below from "design a +durable outbox" to "move the writer" — the single biggest simplification in this plan. + +One gap to close while moving it: nothing verifies sequence continuity. `resume-cursor` is only sent +when a session id is already set, no full resync is forced on reconnect, and the browser cannot tell +whether the server actually retained events from its cursor onward. Today a short retention window +would render as a seamless conversation with a hole in it. + +**R4 is the subtle one and the real work.** Today the agent's output leaves the sidecar as a +`claude:event` message pushed over the registration WebSocket (`user-instance.ts:121-123`): + +```ts +const onEvent = (event: ChatEvent) => { + connection.send({ type: 'claude:event', sessionKey: cmd.params.sessionKey, event }); +}; +``` + +Officer receives those and persists them to Postgres (`chat/websocket.ts:111` → `appendChatEvent`), +and replays on reconnect (`:622` → `getChatEventsSince`). So durability exists — **but it lives on the +wrong side of the socket that just died.** If officer is down, `connection.send` has nowhere to put +the event. The agent keeps working, produces output, and that output is gone. You reconnect to a +session that is alive but has a hole in it. + +That is arguably worse than losing the session, because it is silent. + +## Target topology + +``` +PM2 God Daemon +├─ officer thin: auth, then relay. Restartable at will. +├─ officer-anthropic-proxy (today's mis-named `officer-claude`) — credential path +└─ officer-agent listens on a loopback port, reports it on connect + └─ claude … the agent process, sibling-of-officer's-tree, not descendant +``` + +With the data flow inverted to match slskd: + +- The agent sidecar **listens** on `127.0.0.1:` and reports the port via an `agent:server` + message, exactly as `slskd:server` / `music:server` / `vault:server` already do + (`protocol.ts:60-64`). +- Officer's `/api/chat/*` becomes a catch-all proxy (the 51-line `api/slskd/router.ts` shape) and + `/api/chat/ws` becomes a **WS relay**. Two working precedents already exist in-repo: + `server.tsx:164-228` (dev-server) and `server.tsx:323-326` → `api/vault/websocket.ts` (vault). +- The sidecar **writes its own events to Postgres** with a monotonic per-session sequence number. It + already imports `officerdb` (`user-instance.ts:10`), so this is established precedent, not a new + capability. Officer stops touching `chat_session_events` entirely. +- On reconnect the browser sends `since=` and the **sidecar** answers the replay. Officer relays + the question and the answer, and interprets neither. + +The principle to hold onto, which the pty sidecar already demonstrates: **the work survives, the +tunnel does not.** A terminal shell today outlives a browser disconnect because `terminal/websocket.ts` +deliberately detaches rather than kills. That is the right instinct, applied in the wrong place — the +pty sidecar is a PM2 peer, so its shells are genuinely independent. The agent sidecar should get the +same property. + +## A staged plan + +Each stage is independently shippable and independently testable. The test for every stage is the +same, which is the nice part: **start a long agent turn, `pm2 restart officer`, see what survives.** + +### Stage 0 — make the current state legible (no behaviour change) + +- Rename the PM2 entry `officer-claude` → `officer-anthropic-proxy`, or rename the sidecar's + registered identity from `proxy` to something matching. Right now the process name and the thing it + does disagree, which is how the false claim got into `CLAUDE.md`. +- Fix that `CLAUDE.md` sentence. +- Decide the name for the agent sidecar and use it consistently. + +Cheap, and it stops the next person (or the next me) reasoning from a wrong model. + +### Stage 1 — get the agent out of officer's process tree + +The minimum fix for R1 + R2. Two routes, and I'd want your view on which: + +**1a. PM2-manage it.** Add an `officer-agent` entry to `ecosystem.config.cjs`, delete +`ensureClaudeSidecar` / `spawnAndWaitForRegistration` (`sidecar-registry.ts:198-274`, ~77 lines +including the 50ms registration poll). Officer no longer spawns anything. + +- *Pro:* correct, matches every other sidecar, PM2 restarts and logs it properly. +- *Con:* the per-email spawn model has to go or change — see the open question below. + +**1b. Detach the spawn.** Keep on-demand spawning but `detached: true`, own stdio to its own log file, +own process group. + +- *Pro:* smallest diff, keeps lazy startup. +- *Con:* leaves an unmanaged process PM2 can't see or restart. I think this is the wrong end state, + but it might be a legitimate first step if you want the survivability today. + +After this stage: the process survives, the socket reconnects, **but output produced during the +restart window is still lost.** Worth knowing so the partial win isn't mistaken for the whole one. + +### Stage 2 — move the writer (R4) + +The important stage, and smaller than it looks, because Pass 2 established that the sequence-and-replay +protocol already exists on both ends. This is a relocation, not a new mechanism. The sidecar becomes the +writer of record: + +- Move assignment of the `seq` number into the sidecar, at emit time (today it is assigned downstream, + in officer). +- Sidecar writes each event to `chat_session_events` itself, before/independently of pushing it to + officer. +- Add a sequence-continuity check so a replay gap surfaces instead of rendering as a seamless + conversation. +- Officer stops calling `appendChatEvent` (`chat/websocket.ts:111`) and stops owning + `chat/retention.ts`. +- Replay becomes a sidecar route: `GET /_officer/sessions//events?since=`. + +After this stage a restart during a long turn loses nothing — you reconnect and the missing output +fills in. + +### Stage 3 — invert the transport + +Now the structural fix, which is also most of the Pass 1 claude findings: + +- Sidecar opens a loopback HTTP+WS listener, reports the port (`agent:server`). +- `/api/chat/*` collapses to a catch-all proxy injecting `X-Officer-User`. +- `/api/chat/ws` becomes a byte relay. +- The typed `claude:*` command vocabulary (`protocol.ts:15-19, 37-43`) and the domain verbs in + `sidecar-registry.ts:306-358` disappear — the registration socket carries a port, nothing more. + +### Stage 4 — move the domain logic in behind it + +The rest of the Pass 1 claude findings, now unblocked: the transcript reader +(`chat/claude-sessions.ts`, 361 lines), the turn-loop state machine (`chat/websocket.ts:168-312`), +session CRUD (`chat/chat.ts`), the model catalogue (`chat/list-models.ts:5-9`), CLI install/auth +(`server-settings/claude-code.ts`), and `generate-container-context.ts`'s settings writer. Also point +the `Stop` hook at the sidecar instead of `http://localhost:5000/api/hooks/claude-done` +(`generate-container-context.ts:144-153`, `hono.ts:79-86`) — as written, that hook silently fails +during exactly the restart window we care about. + +### Stage 5 — the harder question: surviving a *sidecar* restart + +Stages 1-4 make the agent survive an **officer** restart. They do not make it survive a restart of the +agent sidecar itself — the agent process is that sidecar's child by design. + +The good news is the primitive already exists and you are already relying on it. My own argv is +`claude --resume 0aebeea6-…`, and `user-instance.ts:31-33` says so explicitly: + +> Single-user platform: the owner runs Claude with no isolation — real HOME, real `~/.claude` — so +> platform sessions have perfect parity with terminal sessions (same config, credentials and +> transcript store, interchangeable via `claude --resume`). + +So the recovery path is: sidecar restarts → re-spawn with `--resume ` → the transcript +on disk carries the context. What that costs is the in-flight turn, not the conversation. Whether +that is good enough, or whether the agent should be a detached grandchild that the sidecar re-adopts, +is a real design decision and I don't have a confident recommendation. + +## Open questions — the ones I'd rather you answered + +1. **Is the per-email spawn model dead weight?** `CLAUDE.md` states single-user is a hard invariant + ("If a change seems to need 'which user is this', the answer is always the owner"), yet the agent + sidecar is keyed per email — `claude:${email}`, a `claudeProcs` Map, a `claudeSpawnWaiters` Map, a + per-email PID lock. If there is only ever one owner, Stage 1a becomes trivial: one PM2 entry, no + fan-out, no registration polling. If you intend multi-tenant later, the fan-out has to stay and + Stage 1 gets harder. **This single answer changes the shape of the whole plan.** + +2. **Relay or redirect?** Officer proxies the agent WebSocket (one origin, keeps your HTTPS reverse + proxy and JWT model intact, but a restart still drops the socket for a moment), or officer hands + the browser a short-lived token and the browser connects to the sidecar directly (survives an + officer restart *without even a reconnect*, but needs its own TLS/origin story and a second + exposed port). I lean relay — the reconnect is cheap once Stage 2 makes it lossless — but the + direct path is the only one where you genuinely never notice. + +3. **Who owns `chat_session_events`?** I've assumed the sidecar (Stage 2). The alternative is a + sidecar-local outbox that drains into officer when it comes back, keeping Postgres officer-only. + More moving parts, but keeps one writer per table. + +4. **What's the acceptable blast radius of an officer restart?** During one, MCP tools that call back + into officer's API fail (`user-instance.ts:20, 77-78`). I'd call that fine — the turn degrades, + the session lives — but you may want the tool layer to retry rather than error. + +5. **Order.** I'd do 0 → 1 → 2 and stop to reassess, because after Stage 2 you have what you actually + asked for and Stages 3-4 become ordinary refactoring you can do incrementally. But if you'd rather + invert the transport first and get durability as a consequence, that's coherent too — it's just a + bigger first step. + +## Two things I noticed while reading, not part of the plan + +- **A credential is written to disk in plaintext.** `user-instance.ts:29` mints a 30-day JWT for the + owner and `:83` writes it into `mcp-host.json` under `DATA_PATH//.container-context/` as part + of the MCP server env. It's the owner's own machine and the token is for their own API, so this may + be entirely intended — but a 30-day owner token at rest in a config file is worth a deliberate + decision rather than an incidental one. +- **Two default ports disagree.** `user-instance.ts:19` defaults to port `5000`, `:20` defaults to + `9010`, for what should be the same officer instance. Only bites if `PORT` is unset. + +## Verified vs not + +**Verified by reading the code or inspecting the running system:** my process ancestry; that +`officer-claude` runs `sidecar/claude/index.ts` and registers as `proxy` with no spawn capability; +that `user-instance.ts` has no PM2 entry and is spawned only at `sidecar-registry.ts:238` with +inherited stdio; that it dials out rather than listening; that events leave via `connection.send`; +that officer persists and replays them; that `--resume` is in my own argv; the two port defaults; the +JWT-to-disk path. + +**Inferred, not tested:** that PM2's default `treekill: true` is the specific mechanism killing the +agent (consistent with the evidence and with `ecosystem.config.cjs` setting no `treekill`, but I did +not isolate it); that `connect.ts`'s reconnect survives a full officer restart cycle; that events are +dropped rather than queued while the socket is down (I read no buffering in `user-instance.ts`, but I +did not read all of `connect.ts`); whether the two `acquireLock()` calls in `index.ts:10` and +`user-instance.ts:51` can collide, since only the latter calls `initPaths` first. + +**Verified after the first draft (Pass 2 of the sidecar audit):** the frontend's reconnect, `seq` +cursor and `resume-cursor` replay request, the backoff constants, and the disconnected UI state. R5 is +satisfied; Stage 2 shrank accordingly. Two claims in the first draft were wrong and are now corrected in +place — I had assumed the replay protocol needed designing, and that the frontend was unverified. diff --git a/docs/navigation-audit.md b/docs/navigation-audit.md index eab1eece..51e15b47 100644 --- a/docs/navigation-audit.md +++ b/docs/navigation-audit.md @@ -1,6 +1,14 @@ # Frontend Navigation Audit — the "opaque click" anti-pattern -**Date:** 2026-07-30 · **Branch:** `sidecars-claude` · **Status:** exploration + documentation only (no code changed) +> **Status, 2026-07-31.** H1 (jobs), H2 (dashboards) and H4's groundwork are **done** — rows are real +> ``s and selection lives in `?selected=` / `useParams`. **H3 is void: the Projects feature was +> deleted**, so every reference to `Projects/`, `SELECTED_PROJECT` and `/projects/:id` below is historical +> and should be read as an example of the pattern, not as work to do. **H4 (chat detail reading +> `sessionId` from `useParams` rather than the `chat:selected-session` channel) is the one item still +> open.** The rules in this file are current and authoritative; the findings table is a snapshot. + + +**Date:** 2026-07-30 · **Origin:** written as exploration before any of the routing work was done. Prep work for the upcoming **full navigation refactor**. This catalogues every place the frontend navigates to / selects an **addressable entity** using a `