From 36db9a2ae0fa3afca92c0faace95b072ce76d6d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Mon, 14 Sep 2026 19:48:11 +0100 Subject: [PATCH] command-hq: self-reported arm/fire/disarm for loops that outlive their turn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing arm path only fires off the harness's own task:started/ task:notification tracking for the live session that submitted the dialog, so a loop started detached (setsid/nohup) or as a tracked job that dies with its turn — which is every real commander on this machine — never arms in Postgres and never records a fire, even after running correctly for weeks. Adds POST /commanders/dir/:dirName/{arm,fire,disarm}, a second door to the same armCommandHqWatcher/recordCommandHqWatcherFired/ disarmCommandHqWatcher functions, authenticated with the commander's own long-lived OFFICER_API_KEY and keyed on dirName since that's what a loop already knows about itself. --- docs/command-hq.md | 42 ++++++++++++- src/servers/api/command-hq/router.ts | 91 +++++++++++++++++++++++++++- 2 files changed, 127 insertions(+), 6 deletions(-) diff --git a/docs/command-hq.md b/docs/command-hq.md index ee306847..1290eed8 100644 --- a/docs/command-hq.md +++ b/docs/command-hq.md @@ -62,11 +62,14 @@ for exact request/response shapes; the table below is the map, not the full cont | GET | `/commanders/:sessionId/turns` | — | `{turns: [...]}` — the raw per-turn cost ledger, oldest first. What "cost over time" plots from | | GET | `/commanders/:sessionId/fires` | — | `{fires: [...]}` — every trigger, oldest first. The full history behind `watcherLastFiredAt` | | POST | `/commanders` | `{sessionId, categoryId?}` | files an EXISTING session; idempotent; 404 if the session isn't in the commanders directory. Not the creation path — see `/commanders/new` | -| PATCH | `/commanders/:sessionId` | `{categoryId?, name?, watcherArmed?}` | `name` proxies to `renameClaudeSession`; `watcherArmed: false` disarms (arming has no route — needs a live task id) | +| PATCH | `/commanders/:sessionId` | `{categoryId?, name?, watcherArmed?}` | `name` proxies to `renameClaudeSession`; `watcherArmed: false` disarms. This route still can't arm — see the `dir/:dirName` routes below for the one path that can from outside a running turn | | DELETE | `/commanders/:sessionId` | — | deletes the TRANSCRIPT (`/clear` chain and all) via `deleteClaudeSession`, then the row. Not reversible, and not the same thing `/purge` does — see "The `/chat` ↔ `/command-hq` relationship" below | | POST | `/purge` | — | sweeps rows whose transcript no longer resolves; returns `{removed}` | | POST | `/commanders/:sessionId/tags` | `{tagId}` | | | DELETE | `/commanders/:sessionId/tags/:tagId` | — | | +| POST | `/commanders/dir/:dirName/arm` | `{taskId}` | self-reported arm, for a loop the harness never sees fire — see "Self-reported arm" below. `:dirName` is the commander's own numeric id as a string, not the transcript uuid | +| POST | `/commanders/dir/:dirName/fire` | `{taskId}` | self-reported trigger; no-ops (`{ok:false}`) unless `taskId` matches the row's current `watcherTaskId` | +| POST | `/commanders/dir/:dirName/disarm` | — | self-reported clean exit; same effect as `PATCH {watcherArmed:false}`, resolved by dirName instead | **A commander's conversation is `GET /chat/sessions/:id`, unchanged.** This router deliberately does not duplicate it — `GET /commanders/:sessionId` returns Command HQ's own metadata: `{sessionId, categoryId, @@ -463,8 +466,9 @@ occur. `recordCommandHqTurn` in `queries.ts`; exposed read-only on `GET /commanders` and `GET /commanders/:sessionId` (`watcherLastFiredAt` survives as a name in the JSON response — it is just computed there, `MAX(fired_at)`, not read off a column), and `PATCH /commanders/:sessionId` accepts - `{watcherArmed: false}` to disarm (arming itself has no route — it needs a live task id, which only a - running turn has). + `{watcherArmed: false}` to disarm. Arming still has no route THERE — it needs a live task id, which only + a running turn has — but see "Self-reported arm" below for the route that exists precisely because a + loop that outlives its turn doesn't have one. - **`armWatcher` threaded end-to-end, client to server.** `BackgroundJobDialog` computes it (`watching && onFire === 'handoff'`) → `sendWithHiddenPrefix` → `QueuedPrompt.armWatcher` → `sendPrompt`'s new last parameter → the chat websocket message → `ClientMessage`'s `'chat'` variant → `startClaudeTurn`'s @@ -537,6 +541,38 @@ OWN user id, not the row it belonged to). Found by re-reading every call site th after the fact, not by any tool — worth remembering the next time a signature changes shape rather than just its types: a compiler that stays quiet is not the same as a diff that is correct. +### Self-reported arm — 2026-09-14 + +The mechanism above has a gap it never surfaced until four real commanders on the owner's own machine were +checked against it: `pendingArmWatcher` is consumed on the harness's `task:started` for the live session +that submitted the arm dialog, and stays valid only as long as that session's own tracked-background-task +plumbing does. A loop that needs to outlive its turn — `setsid`/`nohup`/`disown`, or any tracked background +job that the harness itself drops when the turn ends, which multiple real commanders here do on purpose, +one of them (`docs`-adjacent `WATCHERS.md` for the OCR watcher) as an explicit standing decision favouring +job-panel visibility over survival — never produces that event. `watcherArmed` stayed `false` and +`command_hq_watcher_fires` stayed empty forever for every one of them, even while the loop genuinely polled +and genuinely fired, repeatedly, for weeks. Not cosmetic: `GET /commanders`'s card already papers over the +`watcherArmed` boolean with a live pid check (`row.watcherArmed || liveLoopPid(...) !== null`, `router.ts`), +so the badge was always right — but nothing papers over an empty `command_hq_watcher_fires`, so +`watcherLastFiredAt` read `null` forever on every card for a commander that had fired dozens of times. + +Fixed by giving the loop a second, independent way to say the same thing the harness's own tracking says +for a session-scoped commander: three routes under `/commanders/dir/:dirName/{arm,fire,disarm}`, +authenticated with the commander's own long-lived `OFFICER_API_KEY` (the same credential its loop already +holds and already uses for `/api/notify` and `/api/tasks/*/run` — nothing new minted), keyed on `dirName` +because that is what a loop already knows about itself without needing its own transcript uuid. `arm` and +`disarm` call `armCommandHqWatcher`/`disarmCommandHqWatcher` directly — the same functions `websocket.ts` +calls, just reached from a second door. `fire` keeps the identical staleness guard `task:notification` +already has (`watcherTaskId` must match), so a superseded instance of a loop — an old pid a crash left +running, now replaced by a freshly-armed one — can't keep writing fires for a watch it no longer is. The +task id itself is no longer harness-issued for this path: the loop mints its own (pid + start time is +enough) at the top of the script and reuses it for every `fire` call until it restarts. + +This does not change anything about the original harness-driven path — a commander whose loop genuinely is +a tracked background job on a live session still arms exactly as before, and most of what's written above +about `pendingArmWatcher` and the noticing in `createMessageHandler` is unchanged. It adds a second, parallel +way to reach the same two DB functions, for the class of commander the first path structurally can't reach. + ## Later — explicitly deferred, not forgotten Raised during spec and set aside for a second version, in the owner's words: _"that's not... that's out of diff --git a/src/servers/api/command-hq/router.ts b/src/servers/api/command-hq/router.ts index 3bdcd9c7..336dc862 100644 --- a/src/servers/api/command-hq/router.ts +++ b/src/servers/api/command-hq/router.ts @@ -41,9 +41,12 @@ import { setCommanderSessionId, listCommandHqSessions, getCommandHqSession, + getCommandHqSessionById, setCommandHqSessionCategory, purgeCommandHqSessions, deleteCommandHqSession, + armCommandHqWatcher, + recordCommandHqWatcherFired, disarmCommandHqWatcher, getCommandHqCostSummary, listCommandHqTurns, @@ -721,9 +724,9 @@ commandHqRouter.post('/commanders', async (ctx) => { * `name: string` — rename. Proxies straight to `renameClaudeSession`, the SAME function `/chat`'s own * rename action calls — there is no separate name field on this table to disagree with it. * `watcherArmed: false` — disarm a watcher, so the harness stops re-arming it on its next fire. Only - * `false` is accepted here: arming requires a live task id (`armCommandHqWatcher`), which only the turn - * that starts the watcher has — there is no product path to arm one from outside a running turn, and - * this route is not it. + * `false` is accepted here: arming through THIS route would need a live task id, which only the turn + * that starts the watcher has. A loop reporting its own task id arms through `POST .../dir/:dirName/arm` + * below instead — that route exists precisely because this one can't take it. */ commandHqRouter.patch('/commanders/:sessionId', async (ctx) => { const user = ctx.get('user'); @@ -751,6 +754,88 @@ commandHqRouter.patch('/commanders/:sessionId', async (ctx) => { return ctx.json({ ok: true }); }); +/** + * The self-report counterpart to `armCommandHqWatcher`/`recordCommandHqWatcherFired`'s harness-driven + * calls in `websocket.ts`. Those fire off a `task:started`/`task:notification` event from the harness's + * OWN background-task tracking on the live session that submitted the arm dialog — which never happens + * for a loop that outlives its turn (`setsid`/`nohup`, or a tracked background job that dies with the + * turn anyway, which is most of them). `watcher_armed` stayed false and `command_hq_watcher_fires` stayed + * empty forever for exactly that class of commander — see `docs/command-hq.md` → "Self-reported arm". + * + * Authenticated with the commander's own `OFFICER_API_KEY` (minted per-commander in `credential.ts`), the + * same door its loop already uses for `/api/notify` and `/api/tasks/*​/run`. Keyed on `:dirName`, not + * `:sessionId` — a loop knows its own directory name (it's the folder it's running from; `dirName` is + * literally `String(id)`, see `createCommander`) and has no reason to also know its transcript's uuid. + */ +commandHqRouter.post('/commanders/dir/:dirName/arm', async (ctx) => { + const user = ctx.get('user'); + const id = Number(ctx.req.param('dirName')); + if (!Number.isInteger(id)) throw errors.NOT_FOUND('not a commander'); + const row = await getCommandHqSessionById(user.id, id); + if (!row?.sessionId) throw errors.NOT_FOUND('not a commander'); + + const body = await ctx.req.json<{ taskId?: string }>().catch(() => ({}) as { taskId?: string }); + const taskId = body.taskId?.trim(); + if (!taskId) throw errors.BAD_REQUEST('taskId required'); + + await armCommandHqWatcher(user.id, row.sessionId, taskId); + return ctx.json({ ok: true }); +}); + +/** + * `fire`'s own guard mirrors the harness-driven path exactly: only records a trigger when this row is + * CURRENTLY armed with a matching task id, so an old instance of a loop that a fresh restart superseded + * (new pid, new self-generated task id) can't keep writing fires for a watch it no longer is — the same + * protection `websocket.ts`'s `task:notification` handler gets from checking `watcherTaskId === taskId`. + */ +commandHqRouter.post('/commanders/dir/:dirName/fire', async (ctx) => { + const user = ctx.get('user'); + const id = Number(ctx.req.param('dirName')); + if (!Number.isInteger(id)) throw errors.NOT_FOUND('not a commander'); + const row = await getCommandHqSessionById(user.id, id); + if (!row) throw errors.NOT_FOUND('not a commander'); + + const body = await ctx.req.json<{ taskId?: string }>().catch(() => ({}) as { taskId?: string }); + const taskId = body.taskId?.trim(); + if (!taskId) throw errors.BAD_REQUEST('taskId required'); + + if (!row.watcherArmed || row.watcherTaskId !== taskId) { + return ctx.json({ ok: false, reason: 'not the current armed task' }); + } + await recordCommandHqWatcherFired(row.id, taskId); + return ctx.json({ ok: true }); +}); + +/** + * The loop's own clean-exit counterpart to `arm` — best-effort, same as every other disarm path: a loop + * killed without running its trap still leaves `watcher_armed: true` until the next arm or an explicit + * disarm, exactly as it already could before this route existed. + * + * Takes the SAME `taskId` guard as `fire`, and for the same reason: a restart replaces the old instance + * with a fresh arm before the old one's process necessarily notices it's been superseded (an inotify + * loop's kill can outrace the new one's `arm` call by seconds). An unguarded disarm from the OLD + * instance's exit trap would then clear the NEW instance's `watcher_armed` a moment after it was set — + * caught in exactly this shape while restarting a real duplicate on this machine, 2026-09-14. `taskId` + * is optional so an out-of-band disarm (nothing self-reported ever armed this row) still works — the + * guard only applies once there is a task id on the row to disagree with. + */ +commandHqRouter.post('/commanders/dir/:dirName/disarm', async (ctx) => { + const user = ctx.get('user'); + const id = Number(ctx.req.param('dirName')); + if (!Number.isInteger(id)) throw errors.NOT_FOUND('not a commander'); + const row = await getCommandHqSessionById(user.id, id); + if (!row?.sessionId) throw errors.NOT_FOUND('not a commander'); + + const body = await ctx.req.json<{ taskId?: string }>().catch(() => ({}) as { taskId?: string }); + const taskId = body.taskId?.trim(); + if (taskId && row.watcherTaskId && row.watcherTaskId !== taskId) { + return ctx.json({ ok: false, reason: 'not the current armed task' }); + } + + await disarmCommandHqWatcher(user.id, row.sessionId); + return ctx.json({ ok: true }); +}); + /** * DELETE /command-hq/commanders/:sessionId *