diff --git a/docs/command-hq.md b/docs/command-hq.md index 78078336..ee306847 100644 --- a/docs/command-hq.md +++ b/docs/command-hq.md @@ -3,6 +3,17 @@ Spec conversation 2026-08-18, owner + agent, back and forth. This is the write-up asked for partway through it, kept current as answers landed. +**Superseded in part, 2026-08-24, by `docs/commander-directory.md`.** A commander's identity moved from +Claude's transcript id to its own `command_hq_sessions` row, and it gained its own directory +(`$DATA_PATH//commanders//`, `CLAUDE.md` + `state/`) created BEFORE any turn spawns. That +makes several sections below describe the PREVIOUS mechanism rather than the current one — notably +"How a commander is created" (creation is no longer session-first), "Working directory" (a commander no +longer shares one directory per user, nor chooses an arbitrary one — it has its own), and the `POST +/commanders` / `POST /commanders/new` rows in "API reference" (both now require `name` and `referencePath` +and no longer accept an arbitrary `cwd`). This file was not rewritten to match — read +`docs/commander-directory.md` first for anything those sections describe, and treat the code +(`src/servers/api/command-hq/`) as the source of truth where the two disagree. + **Status:** data model, the full REST API including both creation paths (web and `POST /commanders/new`), built and mounted 2026-08-18 (`bunx tsgo` clean, suite at its 7 pre-existing baseline failures — none of them this feature's). `bun db:push` and a restart of `officer` have both since run diff --git a/src/databases/officer_db/src/command-hq/index.ts b/src/databases/officer_db/src/command-hq/index.ts index d7cff72b..32838350 100644 --- a/src/databases/officer_db/src/command-hq/index.ts +++ b/src/databases/officer_db/src/command-hq/index.ts @@ -3,7 +3,9 @@ export { createCommandHqCategory, renameCommandHqCategory, deleteCommandHqCategory, - createCommandHqSession, + createCommander, + setCommanderSessionId, + getCommandHqSessionById, listCommandHqSessions, listCommandHqSessionIds, getCommandHqSession, @@ -31,6 +33,7 @@ export { export type { CommandHqCategory, CommandHqSessionRow, + NewCommander, CommandHqTag, CommandHqWatcherFire, CommandHqTurnCost, diff --git a/src/databases/officer_db/src/command-hq/queries.ts b/src/databases/officer_db/src/command-hq/queries.ts index 0b503030..d97f23f8 100644 --- a/src/databases/officer_db/src/command-hq/queries.ts +++ b/src/databases/officer_db/src/command-hq/queries.ts @@ -54,7 +54,12 @@ export async function deleteCommandHqCategory(userId: number, id: number): Promi export type CommandHqSessionRow = { id: number; - sessionId: string; + /** Claude's transcript uuid — null until the first turn reports one. See the schema file's docstring. */ + sessionId: string | null; + /** `command_hq_sessions.dir_name` — the directory under `getCommandersDir(email)` that IS this commander. */ + dirName: string; + /** What this commander operates on. Seeded with one path at creation; grown in conversation. */ + referencePaths: string[]; /** What the owner called it. Null for rows filed before the column existed — fall back to the transcript. */ name: string | null; categoryId: number | null; @@ -69,6 +74,8 @@ export type CommandHqSessionRow = { const SESSION_ROW_COLUMNS = { id: commandHqSessions.id, sessionId: commandHqSessions.sessionId, + dirName: commandHqSessions.dirName, + referencePaths: commandHqSessions.referencePaths, name: commandHqSessions.name, categoryId: commandHqSessions.categoryId, watcherArmed: commandHqSessions.watcherArmed, @@ -78,30 +85,67 @@ const SESSION_ROW_COLUMNS = { createdAt: commandHqSessions.createdAt, }; +export type NewCommander = { userId: number; name: string; referencePath: string; categoryId?: number | null }; + /** - * File a session as a commander. Upsert on `(user_id, session_id)` — a second call (a retried request, a - * client that raced itself) is a no-op rather than a duplicate-key error, and does NOT move an - * already-categorised commander back to uncategorised. + * Create a commander row — no transcript, no directory. The directory is the CALLER's job + * (`api/command-hq/directory.ts`): this file is Postgres only, and `dirName` has to come back from HERE + * before anything can be created on disk, since it is derived from the row's own id. + * + * Two statements in one transaction rather than a single `INSERT ... RETURNING`: `dirName` is derived FROM + * the id Postgres assigns on insert, so the row has to exist — with some not-null placeholder — before its + * own directory name is knowable. Nothing else here is a candidate: a name can collide (two commanders + * called "Backups"), a random string is one more thing to keep unique, and the id already IS unique and + * stable the moment it exists. */ -export async function createCommandHqSession( - userId: number, - sessionId: string, - categoryId: number | null = null, - name: string | null = null, -): Promise { +export async function createCommander(input: NewCommander): Promise { + return db.transaction(async (tx) => { + const [inserted] = await tx + .insert(commandHqSessions) + .values({ + userId: input.userId, + sessionId: null, + dirName: '', // fixed up below, once the id it is derived from exists — never visible outside this transaction + referencePaths: [input.referencePath], + name: input.name, + categoryId: input.categoryId ?? null, + }) + .returning({ id: commandHqSessions.id }); + const [row] = await tx + .update(commandHqSessions) + .set({ dirName: String(inserted!.id) }) + .where(eq(commandHqSessions.id, inserted!.id)) + .returning(SESSION_ROW_COLUMNS); + return row!; + }); +} + +/** + * Point a commander at its current conversation — the transcript id, once the harness reports one. Called + * from `fileAsCommander` (`api/chat/websocket.ts`) the moment `session:claude` arrives, correlated by the + * commander's own row id (`msg.commanderId`), not by upserting on the transcript id the way this table used + * to be filed: the row already exists by the time any turn spawns, per `docs/commander-directory.md`, so + * there is nothing left to upsert. + * + * A plain UPDATE, not gated on the column currently being null: a dead session replaced by a fresh one in + * the same directory repoints the same row, which is the whole reason this is a pointer and not an identity. + */ +export async function setCommanderSessionId(userId: number, id: number, sessionId: string): Promise { + const rows = await db + .update(commandHqSessions) + .set({ sessionId }) + .where(and(eq(commandHqSessions.id, id), eq(commandHqSessions.userId, userId))) + .returning({ id: commandHqSessions.id }); + return rows.length > 0; +} + +/** One commander's row, by its OWN id — for a caller (the composer, mid-creation) that has no transcript yet. */ +export async function getCommandHqSessionById(userId: number, id: number): Promise { const [row] = await db - .insert(commandHqSessions) - .values({ userId, sessionId, categoryId, name }) - .onConflictDoNothing({ target: [commandHqSessions.userId, commandHqSessions.sessionId] }) - .returning(SESSION_ROW_COLUMNS); - // `onConflictDoNothing` returns nothing on the conflict path, so the existing row is fetched explicitly — - // the caller still needs an id back (to attach tags, say) even when this call raced an earlier one. - if (row) return row; - const [existing] = await db .select(SESSION_ROW_COLUMNS) .from(commandHqSessions) - .where(and(eq(commandHqSessions.userId, userId), eq(commandHqSessions.sessionId, sessionId))); - return existing!; + .where(and(eq(commandHqSessions.userId, userId), eq(commandHqSessions.id, id))); + return row ?? null; } /** @@ -129,15 +173,22 @@ const isCategoryNull = () => isNull(commandHqSessions.categoryId); * Just the session ids this user has filed — the whole shape `/chat`'s list needs to exclude them, and * `/command-hq`'s grid needs to include only them. One query, no join against transcripts (they are not in * Postgres). + * + * A commander between `POST /commanders` and its first turn has no transcript yet — `sessionId` is null — + * and is filtered out here rather than returned as a literal `null` in the set: there is no transcript id + * to exclude from `/chat`'s list, and a null in a `Set` would only ever fail to match anything. */ export async function listCommandHqSessionIds(userId: number): Promise> { const rows = await db .select({ sessionId: commandHqSessions.sessionId }) .from(commandHqSessions) - .where(eq(commandHqSessions.userId, userId)); - return new Set(rows.map((r) => r.sessionId)); + .where(and(eq(commandHqSessions.userId, userId), isNotNullSessionId())); + return new Set(rows.map((r) => r.sessionId!)); } +// drizzle's `eq` cannot express `IS NOT NULL` either. +const isNotNullSessionId = () => sql`${commandHqSessions.sessionId} is not null`; + export async function getCommandHqSession(userId: number, sessionId: string): Promise { const [row] = await db .select(SESSION_ROW_COLUMNS) @@ -352,7 +403,13 @@ export async function deleteCommandHqSession(userId: number, sessionId: string): */ export async function purgeCommandHqSessions(userId: number, liveSessionIds: Set): Promise { const all = await listCommandHqSessions(userId); - const dead = all.filter((row) => !liveSessionIds.has(row.sessionId)).map((row) => row.sessionId); + // A row with `sessionId: null` has no transcript YET — it is mid-creation, not dead — so it is never a + // purge candidate regardless of what `liveSessionIds` holds. Only a row that HAD a transcript and lost it + // is "dead" in the sense this sweep exists to clean up. + const dead = all + .filter((row): row is CommandHqSessionRow & { sessionId: string } => row.sessionId !== null) + .filter((row) => !liveSessionIds.has(row.sessionId)) + .map((row) => row.sessionId); if (!dead.length) return 0; const rows = await db .delete(commandHqSessions) @@ -508,7 +565,11 @@ export async function parentCommanderOf(spawnedSessionId: string): Promise/commanders//`, holding `CLAUDE.md` and `state/`. See + * `docs/commander-directory.md`. + * + * Derived from this row's own serial `id` (`String(id)`) the moment the row is inserted — nothing else + * is guaranteed unique and stable before a name is even chosen. Not the transcript id: that does not + * exist yet at creation, which is the entire point of this table gaining its own identity. + */ + dirName: text('dir_name').notNull(), + /** + * What the commander operates ON — not where it lives. Seeded with one path at creation (the start + * dialog collects exactly one; asking for a list up front gets a worse answer than asking later) and + * grown as the commander and the owner agree on more in conversation. + * + * A Postgres `text[]` rather than `jsonb`: every element is a plain path string with no structure of + * its own, and an array supports appending in place (`array_append`) without reading the whole column + * back to modify it — `jsonb` would buy nothing here but a heavier read-modify-write for the same list. + */ + referencePaths: text('reference_paths') + .array() + .notNull() + .default([]), /** * What the owner called it, held here rather than only in the transcript's summary record. * @@ -130,6 +164,14 @@ export const commandHqSessions = pgTable( watcherStoppedAt: timestamp('watcher_stopped_at', { withTimezone: true }), createdAt: timestamp('created_at', { withTimezone: true }).notNull().defaultNow(), }, + // `sessionId` is nullable now, and this is left as a PLAIN unique index rather than a partial one + // (`.where(isNotNull(...))`) deliberately, not by default: Postgres already treats every NULL as + // distinct from every other NULL in a unique index, so any number of a user's commanders can sit with + // `session_id: null` at once — which is exactly the window between `POST /commanders` creating the row + // and the first turn reporting a real transcript id, for however many commanders are mid-creation at + // the same time. The moment `session_id` is set, uniqueness still applies: one commander per transcript, + // per user, same guarantee this index always gave. A partial index would enforce the identical rule with + // an extra clause to keep in sync — nothing here needs it. (table) => [uniqueIndex('uq_command_hq_sessions_user_session').on(table.userId, table.sessionId)], ); diff --git a/src/servers/api/chat/claude-sessions.ts b/src/servers/api/chat/claude-sessions.ts index 2288c7e1..45daf7a5 100644 --- a/src/servers/api/chat/claude-sessions.ts +++ b/src/servers/api/chat/claude-sessions.ts @@ -1,5 +1,5 @@ -import { basename, dirname, join, resolve } from 'node:path'; -import { DATA_PATH, getCommandersDir } from '../../data-path'; +import { basename, dirname, join } from 'node:path'; +import { DATA_PATH } from '../../data-path'; import { appendTextAs, findPathAs, @@ -120,10 +120,13 @@ export type ClaudeSessionSummary = { * it has to be able to filter commanders out of that gesture itself. A flag only the web knew about * would fix one surface and leave the other wrong. * - * Derived from the CWD rather than from a database join. Every commander a user has shares one - * directory — `getCommandersDir(email)`, never per-commander and never client-chosen — and this list - * already reads each session's `cwd`. So it costs a string comparison, works for a commander whose - * `command_hq_sessions` row was lost, and adds no query. + * Optional and never set by this file — every commander now has its OWN directory + * (`docs/commander-directory.md`), so there is no single path this list could compare a `cwd` against + * the way it once did when every commander shared one. The real answer is a `command_hq_sessions` row + * lookup, which needs a database call this module does not make; `chat.ts` computes it that way and + * spreads it onto the rows it returns — see its own comment on why `isCommander` is deliberately absent + * from THIS module's output. The field stays on the type because other callers read it optionally off + * whatever `ClaudeSessionSummary` they were handed, undefined being the honest answer when nobody set it. */ isCommander?: boolean; }; @@ -1019,26 +1022,6 @@ export function listClaudeSessions(who: ChatIdentity, cwd: string): ClaudeSessio .sort((a, b) => b.updatedAt.localeCompare(a.updatedAt)); } -/** - * Is this the default commanders directory? - * - * **No longer the test for whether something is a commander** — that is the `command_hq_sessions` row, since - * a commander now chooses its own working directory. This remains only for the places that legitimately care - * about that specific path: creating it for a new OS account, and as the historical location of commanders - * created before the cwd was choosable. - * - * Compared after `resolve`, so a trailing slash or a `..` cannot make the same directory look like a - * different one — the cwd here comes from a transcript rather than from code, and transcripts record - * whatever the process was actually in. - */ -export function isCommandersDir(email: string, cwd: string): boolean { - try { - return resolve(cwd) === resolve(getCommandersDir(email)); - } catch { - return false; - } -} - /** * One session's list-shaped summary, found by id wherever it lives. * diff --git a/src/servers/api/chat/is-commanders-dir.test.ts b/src/servers/api/chat/is-commanders-dir.test.ts deleted file mode 100644 index 3c5b8244..00000000 --- a/src/servers/api/chat/is-commanders-dir.test.ts +++ /dev/null @@ -1,37 +0,0 @@ -import { describe, expect, it } from 'bun:test'; -import { resolve } from 'node:path'; -import { isCommandersDir } from './claude-sessions'; -import { getCommandersDir } from '../../data-path'; - -const EMAIL = 'pastilhas@officer.dev'; - -// The live payload's `isCommander` is derived from a cwd read out of a transcript, so it is whatever the -// process was actually in — not a path this code composed. These are the shapes that arrive. -describe('isCommandersDir', () => { - const dir = getCommandersDir(EMAIL); - - it('recognises the commanders directory', () => { - expect(isCommandersDir(EMAIL, dir)).toBe(true); - }); - - it('survives a trailing slash and a .. hop', () => { - expect(isCommandersDir(EMAIL, `${dir}/`)).toBe(true); - expect(isCommandersDir(EMAIL, `${dir}/sub/..`)).toBe(true); - }); - - it('rejects a sibling and a child', () => { - expect(isCommandersDir(EMAIL, resolve(dir, '..'))).toBe(false); - expect(isCommandersDir(EMAIL, resolve(dir, 'nested'))).toBe(false); - }); - - // A commander's cwd belongs to the account that owns it. Reading one user's directory as another's - // would file their commanders under the wrong person and, in the iPhone swipe order, show them. - it('is per account', () => { - expect(isCommandersDir('someone-else@officer.dev', dir)).toBe(false); - }); - - it('never throws on rubbish', () => { - expect(isCommandersDir(EMAIL, '')).toBe(false); - expect(isCommandersDir(EMAIL, '\0')).toBe(false); - }); -}); diff --git a/src/servers/api/chat/types.ts b/src/servers/api/chat/types.ts index 5de0f058..471117a3 100644 --- a/src/servers/api/chat/types.ts +++ b/src/servers/api/chat/types.ts @@ -77,19 +77,14 @@ export type ClientMessage = */ planMode?: boolean; /** - * The name the owner chose for a COMMANDER before its first message. - * - * Claude Code will not accept a name for a new session — the harness names its own from the first - * prompt — so the platform renames the transcript the moment it exists. Carried on the turn rather - * than sent afterwards because it must not depend on a browser still being open: the name was chosen - * before anything was said, and losing it to a closed tab would leave a commander called after - * whatever it happened to say first. - * - * Applied once, by `fileAsCommander`, via `renameClaudeSession` — which writes a `summary` record and - * therefore an EXPLICIT title, and an explicit title is never overwritten by `ai-title` or by the - * first-message fallback. + * The COMMANDER row this turn belongs to — `command_hq_sessions.id`. Set by `POST /command-hq/ + * commanders`, which creates the row (with its name, directory and `CLAUDE.md`) BEFORE this turn ever + * starts — see `docs/commander-directory.md`. Carried on the turn so the server can point that row at + * the transcript id the moment one exists (`fileAsCommander`, `websocket.ts`) and so `resolveChatCwd` + * can resolve this turn's `cwd` to the commander's own directory without trusting a client-chosen + * `cwd` for it. */ - commanderName?: string; + commanderId?: number; /** * This turn is arming a Command HQ watcher — see `docs/command-hq.md` → "The watcher loop". Set only * by `BackgroundJobDialog` submitting a `handoff` job inside a commander (`isCommander` true); every diff --git a/src/servers/api/chat/websocket.ts b/src/servers/api/chat/websocket.ts index 326367b7..911b8536 100644 --- a/src/servers/api/chat/websocket.ts +++ b/src/servers/api/chat/websocket.ts @@ -15,7 +15,12 @@ import { sendClaudeCodeStreaming } from '@@/channels/send-claude-code'; import { sendOpenCodeStreaming } from '@@/channels/send-opencode'; import * as sidecar from '@@/sidecar-registry'; import { join } from 'path'; -import { getOwnerHomeDir, getEmailAccountsDir, findEmailAccountName } from '../../../servers/data-path'; +import { + getOwnerHomeDir, + getEmailAccountsDir, + findEmailAccountName, + getCommanderDir, +} from '../../../servers/data-path'; import { attachedFilePaths } from './attached-files'; import { getUserSettings, @@ -24,16 +29,15 @@ import { getUserById, setChatSessionModel, setChatSessionPlanMode, - createCommandHqSession, + setCommanderSessionId, getCommandHqSession, + getCommandHqSessionById, armCommandHqWatcher, recordCommandHqWatcherFired, recordCommandHqTurn, } from 'officerdb'; import { resolveHomeDir } from '@@/user-home'; import { renameClaudeSession, type ChatIdentity } from './claude-sessions'; -import { chatIdentity } from './chat'; -import { commanderBlock } from '../command-hq/commander-block'; import { claudeLoginState } from '@@/os-user-claude'; import { mkdirSync } from 'node:fs'; import { logger } from './logger'; @@ -146,30 +150,44 @@ async function resolveEmailCwd(ownerEmail: string, accountEmail?: string): Promi // The working directory a chat turn runs in, by context: email → the account dir; /chat → a chosen // pwd or the caller's own home; everything else (browser/project/dashboard) → the given cwd. async function resolveChatCwd( - msg: { context?: string; contextId?: string; cwd?: string }, + msg: { context?: string; contextId?: string; cwd?: string; resumeSessionId?: string; commanderId?: number }, email: string, userId: number, home: string, ): Promise { /* - A commander runs where the owner chose, and the choice is required. + A commander runs in its OWN directory — resolved here, server-side, never trusted from `msg.cwd`. - This used to force `getCommandersDir(email)` and ignore `msg.cwd` entirely — every commander shared one - empty directory whose only job was to BE a cwd, because the cwd was how a commander was recognised. - Identity is the `command_hq_sessions` row now, so the directory is free to be useful: a commander that - works on the platform can live in the platform checkout, where its relative paths mean something and its - transcripts sit beside the work they are about. + This honoured a client-chosen `msg.cwd` from 2026-08-22 until `docs/commander-directory.md` reversed + that call two days later, and the reversal is not just "a crafted message could misdirect a commander" — + the CONTAINMENT reasoning that made a client-chosen path safe (`resolveCwd(home, …)`, so "client-chosen" + never meant "anywhere") still holds, it just no longer applies, because the client has nothing left to + choose. `POST /command-hq/commanders` creates the row, ITS directory and `CLAUDE.md` before any turn + spawns, so this only ever has to look the directory back up: by `commanderId` on the turn that just + created the row (no transcript id yet to look up by), or by the transcript id (`resumeSessionId`, or the + officer session's own resume id on a later turn of the same conversation) once one exists. Either lookup + lands on the same row, and `getCommanderDir(email, row.dirName)` is the one true answer either way. - The chosen path is resolved through `resolveCwd(home, …)`, the same containment the ordinary chat branch - below uses — so "client-chosen" still does not mean "anywhere". A commander cannot be pointed outside - the account's own tree by a crafted message. - - No fallback to the old shared directory when nothing is chosen: the UI requires a directory before the - first message can be sent, and a silent default would be a commander running somewhere nobody picked. - Falling back to the account's home is the honest last resort for a caller that predates the requirement. + Falls back to the account's home only when NEITHER is given, which should not happen once every path + pre-creates the row — the honest last resort for a caller that predates it, not a real branch anybody + expects to take. */ if (msg.context === 'command-hq') { - const dir = msg.cwd?.trim() ? resolveCwd(home, msg.cwd) : home; + const row = + msg.commanderId !== undefined + ? await getCommandHqSessionById(userId, msg.commanderId) + : msg.resumeSessionId + ? await getCommandHqSession(userId, msg.resumeSessionId) + : null; + if (!row) { + // Loud, because the degradation is otherwise invisible: a commander running in the account's home has + // no CLAUDE.md, so it knows nothing about itself and asks the owner what its job is on every turn — + // which reads as the agent being forgetful rather than as a lookup that missed. + console.warn( + `[command-hq] no commander row for ${msg.commanderId !== undefined ? `id ${msg.commanderId}` : `session ${msg.resumeSessionId ?? '(none given)'}`} — falling back to the account home`, + ); + } + const dir = row ? getCommanderDir(email, row.dirName) : home; mkdirSync(dir, { recursive: true }); return dir; } @@ -358,14 +376,12 @@ function createMessageHandler( planMode?: boolean, commandHq?: boolean, /** - * The name the owner chose before the conversation started, for a commander. - * - * Claude Code will not accept a name for a NEW session — the harness names its own — so the platform - * renames it the moment the transcript exists. Done here rather than by the client because it must not - * depend on a browser still being open: the name was chosen before the first message, and losing it to a - * closed tab would leave a commander called after whatever it happened to say first. + * The commander row THIS turn belongs to — `command_hq_sessions.id`, created by `POST /commanders` + * before this turn ever started (`docs/commander-directory.md`). Used to point that row at the transcript + * id the moment one exists, so a commander's own row and directory (name, `CLAUDE.md`, `dirName`) are + * never something this handler creates — only something it finishes wiring up. */ - commanderName?: string, + commanderId?: number, ) { /* The model messages are ATTRIBUTED to, which is not always the model the turn asked for. @@ -451,68 +467,33 @@ function createMessageHandler( }; /* - File this session as a commander, once the transcript id is known — the same wait `record` above is - built around, and for the same reason: there is nothing to key the row on before the harness announces - it. Gated on `commandHq`, which the caller sets only when `msg.context === 'command-hq'`, so an - ordinary `/chat` turn never touches this table. + Point this commander's row at its transcript id, once the transcript id is known — the same wait + `record` above is built around, and for the same reason: there is nothing to key the write on before the + harness announces it. Gated on `commandHq && commanderId`, so an ordinary `/chat` turn never touches + this table, and a commander turn with no id (should not happen once every caller pre-creates the row — + see `docs/commander-directory.md`) is a no-op rather than a guess at creating one. - Idempotent by construction (`createCommandHqSession` upserts on `(user_id, session_id)`), so calling it - again on every later turn of the same commander — which happens, because the client keeps sending - `context: 'command-hq'` for `resolveChatCwd` to keep resolving the right directory — costs one no-op - upsert rather than a duplicate row. That repetition is also a cheap self-heal: if the row were ever lost, - the next turn puts it back. + `setCommanderSessionId` is a plain UPDATE, not an upsert: the row, its directory and its name all exist + already, written by `POST /commanders` before this turn started. There is nothing left to create here — + only a pointer to set, which is why this is so much smaller than the `fileAsCommander` it replaces used + to be. Idempotent by construction (an UPDATE by id), so calling it again on every later turn of the same + commander costs one no-op write, and is a cheap self-heal if the pointer were ever lost. Fire-and-forget, swallowed failure, same posture as `record`: a database hiccup here must not be a reason a chat turn fails. */ - // Whether the chosen name has actually reached the transcript. False until a rename RETURNS true — see - // the retry note inside `fileAsCommander`. - let commanderNamed = false; - const fileAsCommander = (): void => { - if (!commandHq || !transcriptId) return; + if (!commandHq || !transcriptId || commanderId === undefined) return; const userId = sessionManager.getSession(sessionId)?.userId; if (!userId) return; - const filedId = transcriptId; - // The name goes in with the row, not only to the transcript: the list reads it from here and is - // right immediately, rather than waiting on a summary record the harness cannot write until the - // first turn has finished. - void createCommandHqSession(userId, filedId, null, commanderName?.trim() || null) - .then(async () => { - /* - The chosen name, applied once — and RETRIED, because the first attempt usually fails. - - `fileAsCommander` runs on `session:claude`, which arrives at the START of a turn: the transcript - file may not be on disk yet, and `renameClaudeSession` returns false when it cannot find one. It - returns a boolean that nothing was checking, so the name was silently dropped and the conversation - kept the harness's own `ai-title` — which is exactly what the first live test showed. - - So the attempt is recorded and retried at the end of the turn, by which point the file certainly - exists. Idempotent: once it succeeds, `commanderNamed` stops it repeating. - - It still writes a `summary` record, which sets `hasExplicitTitle` — and an explicit title is never - overwritten by `ai-title` or by the first-message fallback. So it wins whenever it lands. - */ - if (!commanderName?.trim() || commanderNamed) return; - // The same identity resolution every chat route uses, rather than a second copy of it: it is what - // decides the home and the OS user, and a rename writes a file as that user. - const user = await getUserById(userId).catch(() => null); - if (!user?.email) return; - const who = await chatIdentity({ id: userId, email: user.email }).catch(() => null); - if (!who) return; - // `who.home` as the hint, not the old shared commanders directory: a commander chooses its own - // working directory now, so that path is usually wrong. `findTranscript` treats the cwd as a HINT - // and locates the transcript wherever it actually is — so this works either way, and passing a - // path we know is probably wrong would be the kind of thing that reads as intent later. - commanderNamed = renameClaudeSession(who, who.home, filedId, commanderName.trim()); - }) - .catch((err: unknown) => { - logger.warn('Could not file this session as a commander', { - sessionId, - transcriptId: filedId, - error: err instanceof Error ? err.message : String(err), - }); + void setCommanderSessionId(userId, commanderId, transcriptId).catch((err: unknown) => { + logger.warn('Could not point this commander at its transcript', { + sessionId, + transcriptId, + commanderId, + error: err instanceof Error ? err.message : String(err), }); + }); }; return (msg: TurnMessage, seq?: number): void => { @@ -573,9 +554,6 @@ function createMessageHandler( record(); fileAsCommander(); } - // And retried on every result until the name lands. The first attempt runs before the transcript file - // exists, so the end of the first turn is the earliest a rename can succeed. - if (msg.type === 'result' && commandHq && !commanderNamed) fileAsCommander(); /* The watcher loop — see `docs/command-hq.md` → "The watcher loop" and `handoffContract` in @@ -791,11 +769,6 @@ async function handleChat( msg: { prompt: string; displayText?: string; - /** - * The owner pressed Start instead of typing an opener — see `commanderBlock`. Carried on the message - * rather than derived, because the only thing that can know it is the client that rendered the button. - */ - selfStarted?: boolean; sessionId?: string; model?: string; cwd?: string; @@ -931,14 +904,6 @@ export function sessionKeyForTranscript(transcriptId: string): string | undefine return undefined; } -/** - * Sessions already told what they are and where to record it. - * - * In memory, keyed on officer's session id: a restart re-briefs, which costs a few hundred tokens on one - * turn and is the safe direction — a commander that was never told is one that cannot be stopped. - */ -const briefedSessions = new Set(); - export async function startClaudeTurn(params: { email: string; username: string; @@ -949,11 +914,6 @@ export async function startClaudeTurn(params: { msg: { prompt: string; displayText?: string; - /** - * The owner pressed Start instead of typing an opener — see `commanderBlock`. Carried on the message - * rather than derived, because the only thing that can know it is the client that rendered the button. - */ - selfStarted?: boolean; groupSlug?: string; context?: string; contextId?: string; @@ -962,8 +922,8 @@ export async function startClaudeTurn(params: { images?: PromptImage[]; resumeSessionId?: string; planMode?: boolean; - /** See `commanderName` on the wire type — the name chosen before a commander's first message. */ - commanderName?: string; + /** See `commanderId` on the wire type — the row `POST /commanders` already created for this turn. */ + commanderId?: number; armWatcher?: boolean; }; effectivePrompt: string; @@ -972,34 +932,14 @@ export async function startClaudeTurn(params: { const { email, username, userId, ws, sessionId, model, msg, onOutcome } = params; /* - A commander is told its record paths ON A TURN, not in the system preamble. - - The preamble is built inside `createSession`, which runs ONCE — on a conversation's first turn. For a - commander that is precisely the turn where the block cannot exist: it has no `command_hq_sessions` row - and no transcript id yet, so `commanderBlock` returns null, and every later turn reuses the same session - and never rebuilds the preamble. So a commander never received the block at all, and its loop had no - paths to record itself at. Its own test caught it: it armed a perfectly good watcher and wrote no - `loop-pid`. - - Riding `effectivePrompt` instead. That is the string the model sees; `displayText` is what the transcript - shows — the same seam the background-job dialog already uses to send a hidden preamble — so this reaches - the agent without appearing in the conversation. - - Once per session, on the first turn where the block resolves, which is the first turn after the commander - is filed. It stays available afterwards because it is in the conversation's own history from then on. + Unlike before `docs/commander-directory.md`, nothing is injected into the prompt here for a commander. + A commander's own `CLAUDE.md` — written by `POST /commanders`, before this turn ever started — covers + what a per-turn block used to have to compile on the fly: its name, its reference path, and the paths + it must record itself at. Claude Code reads a `CLAUDE.md` from the working directory at session start, + so a commander told about itself in the FILE needs no per-turn injection, no `effectivePrompt` splice, + and no first-turn special case — see the deleted `commander-block.ts` for what this replaced and why. */ - let effectivePrompt = params.effectivePrompt; - if (!briefedSessions.has(sessionId)) { - const block = await commanderBlock({ - userId, - sessionId: transcriptIdBySessionKey.get(sessionId) ?? msg.resumeSessionId ?? null, - selfStarted: msg.selfStarted === true, - }); - if (block) { - briefedSessions.add(sessionId); - effectivePrompt = `${block}\n\n---\n\n${effectivePrompt}`; - } - } + const effectivePrompt = params.effectivePrompt; const fail = (error: string) => { sendToClient(ws, { type: 'error', message: error }); onOutcome?.({ ok: false, error }); @@ -1083,7 +1023,7 @@ export async function startClaudeTurn(params: { msg.resumeSessionId, msg.planMode, msg.context === 'command-hq', - msg.commanderName, + msg.commanderId, ); // Wrapped rather than passed straight to the spawn call, so `onOutcome` sees the transcript id at the // exact moment `fileAsCommander` (inside `createMessageHandler`) does — both react to the same @@ -1161,11 +1101,6 @@ async function handleClaudeCodeChat( msg: { prompt: string; displayText?: string; - /** - * The owner pressed Start instead of typing an opener — see `commanderBlock`. Carried on the message - * rather than derived, because the only thing that can know it is the client that rendered the button. - */ - selfStarted?: boolean; groupSlug?: string; context?: string; contextId?: string; @@ -1189,11 +1124,6 @@ async function handleOpenCodeChat( msg: { prompt: string; displayText?: string; - /** - * The owner pressed Start instead of typing an opener — see `commanderBlock`. Carried on the message - * rather than derived, because the only thing that can know it is the client that rendered the button. - */ - selfStarted?: boolean; groupSlug?: string; context?: string; contextId?: string; diff --git a/src/servers/api/command-hq/commander-block.ts b/src/servers/api/command-hq/commander-block.ts deleted file mode 100644 index 403342cc..00000000 --- a/src/servers/api/command-hq/commander-block.ts +++ /dev/null @@ -1,93 +0,0 @@ -import { getCommandHqSession, getUserById } from 'officerdb'; -import { loopPidPath, spawnedRecordPath } from './spawned'; - -/* - What a commander is told about ITSELF, once per session. - - ── Why this exists ── - - A commander must record its own loop's pid and every agent that loop spawns, or it cannot be stopped and - cannot be fully deleted. The instruction for that used to live in ONE place: the preamble the - background-job dialog composes. Which was fine while the only way to arm a commander was through that - dialog, and wrong the moment a commander arms itself from the conversation — the intended flow. An agent - starting its own background job never sees that preamble, so it would record nothing, and delete would go - back to leaving orphaned loops without anybody noticing it had. - - ── Why the paths come from here and the method comes from the skill ── - - The split is not arbitrary. The METHOD — how to write the record, when, what to append — is reference - material that belongs with everything else about commanders, in the skill, where it can be as long as it - needs to be and is read only when relevant. The PATHS are a fact about THIS session that nothing else can - supply: they are rooted in DATA_PATH and keyed on the commander's own transcript id, and an agent knows - neither. So they are compiled in, always, like an affordance — because a fact the agent cannot derive is - exactly what an always-loaded block is for. - - Short for the same reason every affordance is short: it is charged to every turn of every commander. -*/ - -type CommanderBlockParams = { - /** The Officer account, for the row lookup and for the paths. */ - userId: number; - /** Claude's transcript id — what a commander is keyed on everywhere. */ - sessionId: string | null; - /** - * The owner pressed Start rather than typing an opening message. - * - * They supplied a name and a working directory and nothing else, so there is no request to act on and - * the commander has to run the interview itself. Only ever true on the turn the button sends — a - * commander asked something later is an ordinary conversation and must not restart its intake. - */ - selfStarted?: boolean; -}; - -export async function commanderBlock(params: CommanderBlockParams): Promise { - if (!params.sessionId) return null; - - // The ROW decides, not the working directory. A commander chooses its own cwd, so the directory answers - // nothing — and a session in the old shared directory is not a commander unless it was filed as one. - const row = await getCommandHqSession(params.userId, params.sessionId).catch(() => null); - if (!row) return null; - - // The paths need the account's email and the row carries a user id, so this is a second read. Once per - // session spawn, alongside the affordances and the skills index, which are also read here rather than - // cached — the same trade for the same reason. - const user = await getUserById(params.userId).catch(() => null); - if (!user?.email) return null; - const email = user.email; - - return [ - '# You are a commander', - '', - 'This conversation is a commander: an agent that keeps working when nobody is watching. Open the', - '**Commanders** skill before starting or changing a watcher — it carries how the pieces fit, what to', - 'establish with the owner first, and the mistakes that are expensive.', - '', - 'Two paths only this session can be told. **Write to them or you cannot be stopped:**', - '', - `- your loop's own pid: \`${loopPidPath(email, params.sessionId)}\``, - `- one session id per line, for every agent your loop spawns: \`${spawnedRecordPath(email, params.sessionId)}\``, - '', - 'The platform cannot find your process and cannot recover a spawned agent id after it exits, so this', - 'record is the only one. Without it, deleting this commander leaves your loop running.', - '', - '**Never arm a watcher on your first turn.** The first turn is for settling what to watch and what to do', - 'about it — arming before that is arming a guess, and a wrong condition either never fires or fires', - 'constantly and is found out days later. If you were given no paths above, you ARE on your first turn:', - 'that is what their absence means. Ask, and arm once it is agreed.', - /* - The button case. A pointer, not the interview itself: the questions live in the skill, which is - re-read from disk on every run, so rewording one costs nothing. The same words here would cost a - `pm2 restart officer` every time — and this file is the wrong home for doctrine that will be - edited far more often than the mechanism around it. - */ - ...(params.selfStarted - ? [ - '', - '**Nobody has told you what they want yet.** They gave a name and a working directory and', - 'pressed Start, which is a request to run the interview — not a request to do anything else.', - 'Read the **Commanders** skill now and follow "When you started yourself": orient in this', - 'directory first, then open with what you found and ONE question.', - ] - : []), - ].join('\n'); -} diff --git a/src/servers/api/command-hq/directory.ts b/src/servers/api/command-hq/directory.ts new file mode 100644 index 00000000..4038ed36 --- /dev/null +++ b/src/servers/api/command-hq/directory.ts @@ -0,0 +1,140 @@ +import { mkdirSync, writeFileSync } from 'node:fs'; +import { getUserByEmail } from 'officerdb'; +import { lookupOsUser } from '@@/os-user'; +import { join } from 'node:path'; +import { getCommanderDir, getCommanderStateDir } from '../../data-path'; +import { loopPidPath, spawnedRecordPath } from './spawned'; + +/* + A commander's own directory, created once, up front — `docs/commander-directory.md`. + + $DATA_PATH//commanders// + CLAUDE.md written here, extended by the commander as the job is agreed + state/ INSTANCE data: what it spawned, its loop's pid. Empty at creation. + + Both exist before the first turn spawns, which is the whole point: Claude Code reads a CLAUDE.md from the + working directory at session start, so a commander told about itself here needs no per-turn injection — + see the deleted `commander-block.ts` for what this replaced. +*/ + +export type CommanderClaudeMdParams = { + name: string; + referencePath: string; + loopPidPath: string; + spawnedPath: string; +}; + +/** + * The file itself. Prose, not a config dump — this is a document the commander reads once and then keeps + * extending, and the owner may open it directly. + * + * Deliberately does NOT carry the button-specific framing `commander-block.ts` used to add on a + * `selfStarted` turn ("nobody has told you what they want yet — they pressed Start"). That framing is + * conditional on how THIS conversation happened to begin, which a file written once and read on every + * future session cannot express — and it would be actively wrong the day a fresh conversation replaces a + * dead one in this same directory, or if the opening message was typed rather than a button press. The + * button case is still covered: `CommandHqDetail.tsx`'s `startBrief` composes that framing into the + * opening message itself, which is the right place for something true only of one specific turn. + */ +export function commanderClaudeMd(params: CommanderClaudeMdParams): string { + return [ + `# ${params.name}`, + '', + 'This is a commander: an agent that keeps working when nobody is watching. This file is its own —', + 'extend it as the job gets settled, and again whenever what it watches or what it does about that', + 'changes. Open the **Commanders** skill before starting or changing a watcher; it carries how the', + 'pieces fit, what to establish first, and the mistakes that are expensive.', + '', + '## What this watches', + '', + `Reference: \`${params.referencePath}\``, + '', + 'That path is what this commander operates ON, not where it lives — this directory is its own, kept', + 'separate from the thing it watches. Read it before asking what the job is: it was named before', + 'anything else was said, so treat it as the subject rather than decoration. More references can be', + 'added here as they come up in conversation.', + '', + '## Recording itself', + '', + 'Two paths only this commander can be told, and it must write to them or it cannot be stopped:', + '', + `- its loop's own pid: \`${params.loopPidPath}\``, + `- one session id per line, for every agent its loop spawns: \`${params.spawnedPath}\``, + '', + 'The platform cannot find this process and cannot recover a spawned agent id after it exits, so this', + 'record is the only one. Without it, deleting this commander leaves its loop running.', + '', + '## Before arming anything', + '', + 'Never arm a watcher on the first turn. The first turn is for settling what to watch and what to do', + 'about it — arming before that is arming a guess, and a wrong condition either never fires or fires', + 'constantly and is found out days later. Ask, and arm once it is agreed.', + '', + 'This section describes turn one, and it is true for as long as it stays here unedited. Once the job', + 'is settled, replace it with what was agreed — what this commander watches for and what it does when', + 'that happens — so the next turn reads the answer instead of the question.', + '', + ].join('\n'); +} + +/** + * Create a commander's directory and write its `CLAUDE.md`. Called once, by `POST /command-hq/commanders`, + * before any turn spawns — that ordering is the entire fix `docs/commander-directory.md` describes. + * + * `state/` is created empty rather than left to be created lazily on first write (as `getCommanderStateDir` + * callers already do defensively): creating it here means the directory listing a commander's first turn + * sees is complete from the start, definition and state both present, rather than state/ appearing later + * as an unexplained addition. + */ +/** + * Give the new directory to the account that will work in it. + * + * `confineUserTree` chowns `commanders/` to the member ONCE, at provisioning. Anything created after that + * belongs to the service user — so a member's commander directory would be one their own `claude` cannot + * write into, and the failure surfaces as an agent that dies on its first command in a directory it can + * see but not use. Exactly the `general_chat_sessions` mistake `resolveChatCwd` already tells the story + * about, one level deeper. + * + * A no-op for the owner, whose account has no `osUser` and who already owns everything under `DATA_PATH` + * — which is why this was invisible: it works perfectly for the person most likely to test it. + * + * Best effort. A commander whose directory could not be handed over still exists and still works for the + * owner; refusing to create it would be the worse trade. The failure is returned so the caller can say so. + */ +async function giveToAccount(email: string, dir: string): Promise { + const user = await getUserByEmail(email).catch(() => null); + if (!user?.osUser) return null; + + const ids = await lookupOsUser(user.osUser); + if (!ids) return `no Linux account named ${user.osUser}`; + + const proc = Bun.spawn(['sudo', '-n', 'chown', '-R', `${ids.uid}:${ids.gid}`, dir], { + stdout: 'pipe', + stderr: 'pipe', + }); + const err = await new Response(proc.stderr).text(); + return (await proc.exited) === 0 ? null : err.trim() || 'chown failed'; +} + +export async function createCommanderDirectory( + email: string, + dirName: string, + params: { name: string; referencePath: string }, +): Promise<{ dir: string; ownershipError: string | null }> { + const dir = getCommanderDir(email, dirName); + mkdirSync(dir, { recursive: true }); + mkdirSync(getCommanderStateDir(email, dirName), { recursive: true }); + writeFileSync( + join(dir, 'CLAUDE.md'), + commanderClaudeMd({ + name: params.name, + referencePath: params.referencePath, + loopPidPath: loopPidPath(email, dirName), + spawnedPath: spawnedRecordPath(email, dirName), + }), + ); + + // Last, so it covers CLAUDE.md and state/ as well as the directory itself. + const ownershipError = await giveToAccount(email, dir); + return { dir, ownershipError }; +} diff --git a/src/servers/api/command-hq/router.ts b/src/servers/api/command-hq/router.ts index 3d77735d..76bb2ff4 100644 --- a/src/servers/api/command-hq/router.ts +++ b/src/servers/api/command-hq/router.ts @@ -1,25 +1,19 @@ import type { Context } from 'hono'; import { randomUUID } from 'node:crypto'; +import { rmSync } from 'node:fs'; import { createRouter } from '../../create-router'; import { checkBudget } from '../sub-agents/budget'; import * as errors from '@@/custom-errors'; import { resolveHomeDir } from '@@/user-home'; -import { getCommandersDir, toShellUsername } from '@@/data-path'; +import { toShellUsername, getCommanderDir } from '@@/data-path'; import { forgetChatSessions, getChatEventsSince, ingestSpawnedChildren, sessionKeyForTranscriptId } from 'officerdb'; import * as sidecar from '@@/sidecar-registry'; import { findSessionKeyByClaudeSession } from '@@/sidecar/claude/state'; import { sessionKeyForTranscript } from '../chat/websocket'; -import { - clearSpawnedRecord, - listSpawnedSessions, - loopPidPath, - spawnedRecordPath, - stopCommanderProcesses, - liveLoopPid, -} from './spawned'; +import { listSpawnedSessions, loopPidPath, spawnedRecordPath, stopCommanderProcesses, liveLoopPid } from './spawned'; +import { createCommanderDirectory } from './directory'; import type { ServerMessage } from '../chat/types'; import { - listClaudeSessions, loadClaudeSessionById, loadClaudeSessionSummaryById, renameClaudeSession, @@ -33,7 +27,8 @@ import { createCommandHqCategory, renameCommandHqCategory, deleteCommandHqCategory, - createCommandHqSession, + createCommander, + setCommanderSessionId, listCommandHqSessions, getCommandHqSession, setCommandHqSessionCategory, @@ -67,17 +62,26 @@ import { * * ── Where commanders live ── * - * **Wherever the owner chose.** Each commander names its own working directory when it is created, and that - * directory is where its relative paths resolve and where its transcripts are grouped. + * **Its own directory, created by this router before any turn spawns.** `POST /commanders` creates the row + * AND `$DATA_PATH//commanders//` together, writes `CLAUDE.md`, and only then does the + * caller start a turn with `cwd` set to that directory — see `docs/commander-directory.md` for why the + * ordering is the entire fix. * - * It was one shared directory per user until 2026-08-22, because the directory WAS the identity — the only - * way to tell a commander from an ordinary chat. Identity is the `command_hq_sessions` row now, so the - * directory became free to be useful, and a commander that works on the platform can live in the platform. + * Two earlier designs are worth knowing so this one is not mistaken for a third try at the same idea. One + * shared directory per user, until 2026-08-22, because the directory WAS the identity — the only way to + * tell a commander from an ordinary chat, which is also what made this router's own listing impossible to + * do any other way than a directory scan. Then, briefly, a commander chose an arbitrary directory anywhere + * in its owner's tree, once identity moved to the `command_hq_sessions` row — free to be useful, but free + * of a directory a commander could depend on existing, or write anything durable into before its first + * turn even started. This router owning creation is what fixes both: identity is still the row, and the + * directory is still guaranteed to exist and be this commander's own before anything runs in it. * - * The consequence for this router is that listing is no longer a directory scan. It reads its rows and loads - * each transcript BY ID (`loadClaudeSessionSummaryById`) — a handful of lookups rather than one walk. A row - * whose transcript cannot be found anywhere is treated as absent, exactly as a missing file in the old scan - * was, and `POST /purge` is what tidies those. + * The consequence for this router is that listing is not a directory scan either way. It reads its rows and + * loads each transcript BY ID (`loadClaudeSessionSummaryById`) — a handful of lookups rather than one walk. + * A row whose transcript cannot be found anywhere is treated as absent, exactly as a missing file in a scan + * would have been, and `POST /purge` is what tidies those. A row with no transcript YET (freshly created, + * before its first turn) is treated the same way — absent from the grid until it has one — which is fine: + * the composer that just created it already has everything it needs to keep going without a grid entry. */ async function commandHqIdentity(user: { id: number; email: string }): Promise { @@ -213,29 +217,29 @@ commandHqRouter.get('/commanders', async (ctx) => { const categoryId = categoryParam === undefined ? undefined : categoryParam === 'none' ? null : Number(categoryParam); /* - Each commander's transcript is loaded BY ID, not found by scanning one directory. + Each commander's transcript is loaded BY ID, not found by scanning one directory — a commander's own + directory holds `CLAUDE.md` and `state/`, never a transcript, so there is nothing to scan there any more + either. One `locateTranscript` per row instead. Commanders are few — this is a handful of stats, not a + directory walk per row — and a row whose transcript cannot be found anywhere is treated as absent, + which is what `purgeCommandHqSessions` exists to tidy. - It used to be `listClaudeSessions(who, getCommandersDir(...))` — every commander lived in one place, so - one scan found them all and a row without a match there was a row whose transcript had been deleted. A - commander chooses its own working directory now, so that scan finds only the ones that happen to still - live in the old shared folder and would silently hide every other. - - One `locateTranscript` per row instead of one scan for all of them. Commanders are few — this is a - handful of stats, not a directory walk per row — and a row whose transcript cannot be found anywhere is - still treated exactly as before: absent, which is what `purgeCommandHqSessions` exists to tidy. + A row with `sessionId: null` — created but never yet turned — is absent from this grid for the same + reason: there is no transcript to summarise. Filtered out before the lookup rather than after, so + `loadClaudeSessionSummaryById` is never asked about an id that cannot exist. */ const rows = await listCommandHqSessions(user.id, categoryId); - const summaries = rows - .map((row) => loadClaudeSessionSummaryById(who, row.sessionId)) + const withTranscript = rows.filter((row) => row.sessionId !== null); + const summaries = withTranscript + .map((row) => loadClaudeSessionSummaryById(who, row.sessionId!)) .filter((s): s is NonNullable => !!s); const liveById = new Map(summaries.map((s) => [s.id, s])); - const filed = rows.filter((row) => liveById.has(row.sessionId)); + const filed = withTranscript.filter((row) => liveById.has(row.sessionId!)); const [tagsByRow, lastFiredByRow, runningById, costByRow] = await Promise.all([ listCommandHqSessionTags(filed.map((row) => row.id)), getLastFiredAtBySessionRowIds(filed.map((row) => row.id)), - Promise.all(filed.map(async (row) => [row.sessionId, await isRunning(row.sessionId)] as const)).then( + Promise.all(filed.map(async (row) => [row.sessionId!, await isRunning(row.sessionId!)] as const)).then( (pairs) => new Map(pairs), ), // One aggregate query per commander, the same cost class `isRunning` above already accepts for this @@ -248,18 +252,21 @@ commandHqRouter.get('/commanders', async (ctx) => { const commanders: CommanderSummary[] = filed .map((row) => ({ - ...liveById.get(row.sessionId)!, - // The row's name wins over the transcript's. It is written when the commander is filed, while the - // summary record cannot exist until the first turn ends — so until then the transcript still reports - // the opening brief, and the row is the only source that is right from the start. + ...liveById.get(row.sessionId!)!, + // The row's name wins over the transcript's. It is written when the commander is CREATED, before any + // turn runs, while the transcript's own summary record cannot exist until the first turn ends — so + // until then the transcript still reports the opening brief, and the row is the only source that is + // right from the start. ...(row.name ? { title: row.name } : {}), categoryId: row.categoryId, tagIds: tagsByRow.get(row.id) ?? [], - running: runningById.get(row.sessionId) ?? false, + running: runningById.get(row.sessionId!) ?? false, // The pid file is the FACT; `watcher_armed` is a claim only one path sets. A commander that started // its loop as a plain background job recorded its pid correctly and still reported unarmed, so the - // grid offered nothing to stop — which is the one thing this column exists to make possible. - watcherArmed: row.watcherArmed || liveLoopPid(who.email, row.sessionId) !== null, + // grid offered nothing to stop — which is the one thing this column exists to make possible. Keyed + // on `dirName`, not the transcript id: the pid file lives in this commander's OWN `state/`, not + // beside the transcript. + watcherArmed: row.watcherArmed || liveLoopPid(who.email, row.dirName) !== null, watcherArmedAt: row.watcherArmedAt, watcherStoppedAt: row.watcherStoppedAt, watcherLastFiredAt: lastFiredByRow.get(row.id) ?? null, @@ -271,26 +278,31 @@ commandHqRouter.get('/commanders', async (ctx) => { }); /** - * POST /command-hq/commanders/new { prompt: string, displayText?: string, categoryId?: number | null } + * POST /command-hq/commanders/new { prompt: string, name: string, referencePath: string, displayText?: + * string, categoryId?: number | null } * * Creates a commander for a client that cannot open a websocket with the right parameters — mobile, - * notably. The web composer never calls this: it sends an ordinary `/chat` turn with `context: - * 'command-hq'`, and the row is written the moment the harness reports a transcript id (see - * `websocket.ts` → `fileAsCommander`). This route drives the SAME path — `startClaudeTurn`, the function - * that logic was extracted into — with no live socket, and waits for that same id before answering. + * notably. The web composer never calls this: it starts a chat turn directly, in the directory + * `POST /commanders` (below) already created for it. This route drives the identical two-step order for a + * caller with no live socket — create the row and its directory FIRST, via the same `createCommander` + + * `createCommanderDirectory` this router's bare `POST /commanders` uses, THEN start the turn with `cwd` set + * to that directory — rather than a second copy that could drift from it. See + * `docs/commander-directory.md` for why the ordering is the fix and not an implementation detail. + * + * **This changed shape 2026-08-24.** `name` and `referencePath` are now required, and `cwd` is no longer + * accepted as the actual working directory — a commander runs in its OWN directory now, never one a caller + * names. Neither client had been exercised against a running server yet when this changed (`docs/command- + * hq.md`), so there is no live caller to keep compatible with the old shape. * * `prompt` is required, deliberately. A commander begins as an ordinary conversation — sometimes a real * back-and-forth before a background job is ever armed, sometimes just "you know what to do" once that * has already happened elsewhere — but it always begins with something, the same as any `/chat` * conversation does. There is no "empty commander" to create and later fill in. * - * `displayText`, separately, is what TITLES the commander — found missing here by the mobile side after - * this route first shipped without it. A commander created to become a background job sends the full - * `buildBackgroundJobPrompt` mechanics as `prompt` (a page of preamble) with the owner's actual sentence - * only at the end, and `command_hq_sessions` has no name column — the title comes from - * `session.meta.title = (displayText ?? prompt).slice(0, 100)`, same precedence the websocket path - * already uses. Without this field every commander created this way titles itself with the mechanics - * text, and the grid becomes a column of identical, unreadable cards. + * `displayText`, separately, is what the TRANSCRIPT shows for the opening message — the row's own `name` + * already titles the commander everywhere Command HQ reads it, but the harness still names its own summary + * record from the first prompt, and `prompt` here is often the full `buildBackgroundJobPrompt` mechanics + * rather than something a person would want to read back. * * The response's `sessionId` is Claude's own transcript id — not a placeholder, not Officer's — so the * client has something it can immediately resume, attach to, or list among its commanders. Getting it @@ -301,13 +313,31 @@ commandHqRouter.get('/commanders', async (ctx) => { */ commandHqRouter.post('/commanders/new', async (ctx) => { const user = ctx.get('user'); + const who = await commandHqIdentity(user); const body = await ctx.req - .json<{ prompt?: string; displayText?: string; categoryId?: number | null; cwd?: string; name?: string }>() + .json<{ + prompt?: string; + displayText?: string; + categoryId?: number | null; + name?: string; + referencePath?: string; + }>() .catch( - () => ({}) as { prompt?: string; displayText?: string; categoryId?: number | null; cwd?: string; name?: string }, + () => + ({}) as { + prompt?: string; + displayText?: string; + categoryId?: number | null; + name?: string; + referencePath?: string; + }, ); const prompt = body.prompt?.trim(); + const name = body.name?.trim(); + const referencePath = body.referencePath?.trim(); if (!prompt) throw errors.BAD_REQUEST('prompt is required'); + if (!name) throw errors.BAD_REQUEST('name is required'); + if (!referencePath) throw errors.BAD_REQUEST('referencePath is required'); // ── Room to run, before starting work nobody is watching ── // @@ -320,6 +350,10 @@ commandHqRouter.post('/commanders/new', async (ctx) => { const budget = await checkBudget(); if (!budget.ok) throw errors.TOO_MANY_REQUESTS(budget.reason ?? 'not enough usage headroom to start an agent'); + // Row and directory first — see `POST /commanders` below for the same two calls with the full reasoning. + const row = await createCommander({ userId: user.id, name, referencePath, categoryId: body.categoryId ?? null }); + const dir = createCommanderDirectory(who.email, row.dirName, { name, referencePath }); + const sessionId = randomUUID(); const model = (await getUserDefaultModel(user.id)) || DEFAULT_MODEL; @@ -345,12 +379,10 @@ commandHqRouter.post('/commanders/new', async (ctx) => { prompt, displayText: body.displayText?.trim() || undefined, context: 'command-hq', - // Where this commander runs, and its name. Both are chosen before the first message — the UI makes - // them mandatory — so this programmatic path takes them too rather than inventing a default. An - // absent cwd falls back to the account's home in `resolveChatCwd`, which is the honest last resort - // for a caller that predates the requirement. - ...(body.cwd?.trim() ? { cwd: body.cwd.trim() } : {}), - ...(body.name?.trim() ? { commanderName: body.name.trim() } : {}), + // No `cwd` here: `resolveChatCwd` resolves the commander's directory itself from `commanderId` + // (`docs/commander-directory.md`), the same as the web composer's turn does. `dir`, computed above, + // is used only for the JSON response below. + commanderId: row.id, }, effectivePrompt: prompt, onOutcome: settle, @@ -360,20 +392,15 @@ commandHqRouter.post('/commanders/new', async (ctx) => { if (!outcome.ok) throw errors.INTERNAL_SERVER_ERROR(outcome.error); /* - The row already exists — `fileAsCommander` wrote it, off the same `session:claude` event this promise - resolved on. What might not have landed yet is the CATEGORY: that write is fire-and-forget and always - files with none, so a race between it and this request is possible in either order. `createCommandHqSession` - is an upsert-on-conflict-do-nothing (safe either way), and the `setCommandHqSessionCategory` UPDATE below - is unconditional and runs after both, which is what makes the outcome deterministic regardless of which - of the two insert attempts actually won: whichever category this request asked for is the one that is - left standing. + Belt and braces: `fileAsCommander` (`websocket.ts`) already points this row at the transcript id off the + same `session:claude` event this promise resolved on, but that write is fire-and-forget so it is not + guaranteed to have landed by the time this handler resumes. `setCommanderSessionId` is a plain, + idempotent UPDATE — calling it again here costs nothing and guarantees the row is correct before this + response goes out, rather than trusting a race this handler has no way to observe. */ - await createCommandHqSession(user.id, outcome.transcriptId, body.categoryId ?? null); - await setCommandHqSessionCategory(user.id, outcome.transcriptId, body.categoryId ?? null); + await setCommanderSessionId(user.id, row.id, outcome.transcriptId); - // The cwd it ACTUALLY ran in, from the spawn outcome — not a guess at the old shared directory, which is - // no longer where a commander necessarily lives. - return ctx.json({ sessionId: outcome.transcriptId, cwd: body.cwd?.trim() ?? null }); + return ctx.json({ sessionId: outcome.transcriptId, cwd: dir, id: row.id, dirName: row.dirName }); }); /** @@ -402,10 +429,11 @@ commandHqRouter.get('/commanders/:sessionId', async (ctx) => { return ctx.json({ sessionId: row.sessionId, name: row.name, + referencePaths: row.referencePaths, categoryId: row.categoryId, tagIds: tagsByRow.get(row.id) ?? [], running, - watcherArmed: row.watcherArmed || liveLoopPid(who.email, row.sessionId) !== null, + watcherArmed: row.watcherArmed || liveLoopPid(who.email, row.dirName) !== null, watcherArmedAt: row.watcherArmedAt, watcherStoppedAt: row.watcherStoppedAt, watcherLastFiredAt: lastFiredByRow.get(row.id) ?? null, @@ -446,36 +474,47 @@ commandHqRouter.get('/commanders/:sessionId/fires', async (ctx) => { }); /** - * POST /command-hq/commanders { sessionId: string, categoryId?: number } + * POST /command-hq/commanders { name: string, referencePath: string, categoryId?: number | null } * - * Files an EXISTING session as a commander. Idempotent — filing an already-filed session is a no-op, and - * does not move it to a different category (use PATCH for that). + * Creates a commander. **This is the creation path now, and it changed shape 2026-08-24** — it used to file + * an EXISTING session (`{ sessionId, categoryId? }`), which is the wrong order per + * `docs/commander-directory.md`: a chat session existed first and was filed as a commander second, so the + * working directory was fixed before any commander id existed and a commander could never start in a + * directory named after itself. There is no live caller of the old shape to keep compatible with — see the + * design doc's "Order of work". * - * This is a low-level primitive, not the normal creation path. The normal path is starting a new commander - * through Command HQ's own composer, which writes this row itself the moment the harness reports a real - * transcript id — see `docs/command-hq.md` → "How a commander is created". This route exists for that - * wiring to call, and for anything else (a purge-and-refile flow, a script) that needs to file a session - * without going through a live chat turn. - * - * The session must actually exist in the commanders directory, or this refuses — filing a `/chat` session - * makes no sense given commanders and `/chat` never share a working directory. + * Creates, in order: the row (`createCommander`, no transcript — `sessionId` stays null until a turn + * reports one), THEN the directory and its `CLAUDE.md` (`createCommanderDirectory`) — the row has to exist + * first because `dirName` is derived from its own id. Returns the row, including `cwd`: the caller (the + * web composer, `CommandHqDetail.tsx`) starts its chat turn with `cwd` set to exactly this path, and + * threads `commanderId: row.id` on that turn so `fileAsCommander` (`websocket.ts`) can point this same row + * at the transcript id the moment one exists — the same server-driven, don't-depend-on-a-live-browser + * pattern this router already used for the commander's name, now doing the same job for its `sessionId`. + * No separate `PATCH … { sessionId }` route exists for this: it would only reintroduce the thing this + * design removes — a client that has to remember to follow through, on a value the server already knows + * first. */ commandHqRouter.post('/commanders', async (ctx) => { const user = ctx.get('user'); const who = await commandHqIdentity(user); const body = await ctx.req - .json<{ sessionId?: string; categoryId?: number }>() - .catch(() => ({}) as { sessionId?: string; categoryId?: number }); - const sessionId = body.sessionId?.trim(); - if (!sessionId) throw errors.BAD_REQUEST('sessionId is required'); + .json<{ name?: string; referencePath?: string; categoryId?: number | null }>() + .catch(() => ({}) as { name?: string; referencePath?: string; categoryId?: number | null }); + const name = body.name?.trim(); + const referencePath = body.referencePath?.trim(); + if (!name) throw errors.BAD_REQUEST('name is required'); + if (!referencePath) throw errors.BAD_REQUEST('referencePath is required'); - // Located by id wherever it lives, not required to be in the old shared directory. A commander chooses - // its own working directory, so "is it in the commanders folder" stopped being a meaningful check — and - // as a GUARD it only ever established that the transcript exists, which this still does. - if (!loadClaudeSessionSummaryById(who, sessionId)) throw errors.NOT_FOUND('no such conversation'); + const row = await createCommander({ userId: user.id, name, referencePath, categoryId: body.categoryId ?? null }); + const { dir, ownershipError } = await createCommanderDirectory(who.email, row.dirName, { name, referencePath }); + if (ownershipError) { + console.warn(`[command-hq] ${row.dirName}: directory not handed to the account — ${ownershipError}`); + } - const row = await createCommandHqSession(user.id, sessionId, body.categoryId ?? null); - return ctx.json(row); + // `ownershipError` is returned as well as logged. A member whose commander directory could not be handed + // over gets an agent that dies on its first command, and "the platform knew and only wrote it to a log" + // is the worst version of that. + return ctx.json({ ...row, cwd: dir, ...(ownershipError ? { ownershipError } : {}) }); }); /** @@ -592,13 +631,13 @@ commandHqRouter.delete('/commanders/:sessionId', async (ctx) => { is exactly how a deleted commander came back as an untitled ordinary chat. */ const sessionKilled = sessionKey ? await sidecar.killClaudeAndWait(sessionKey, user.id) : false; - const stopped = stopCommanderProcesses(who.email, sessionId); + const stopped = stopCommanderProcesses(who.email, row.dirName); // 2. Everything it spawned. Best-effort per child: one transcript that has already been cleaned up must // not stop the rest from going, and a child is often gone before its parent. // Ingest the inbox before reading it, so a child appended since the last read is still deleted. The file // is lossy by design and the rows are not — but a row that has never been ingested does not exist yet. - const children = (await ingestSpawnedChildren(row.id, listSpawnedSessions(who.email, sessionId))).map( + const children = (await ingestSpawnedChildren(row.id, listSpawnedSessions(who.email, row.dirName))).map( (c) => c.spawnedSessionId, ); let childrenDeleted = 0; @@ -623,7 +662,17 @@ commandHqRouter.delete('/commanders/:sessionId', async (ctx) => { // Already gone, or never written. Counted as not-deleted rather than failing the request. } } - clearSpawnedRecord(who.email, sessionId); + /* + The commander's OWN directory — `CLAUDE.md`, `state/`, and anything else it wrote for itself. Not just + `state/` (which `clearSpawnedRecord` alone would remove): per `docs/commander-directory.md`, deleting a + commander is deleting its directory, whole, which is also the only way to be sure nothing is left + behind. Best-effort, same posture as the children above — a directory already gone is not a failure. + */ + try { + rmSync(getCommanderDir(who.email, row.dirName), { recursive: true, force: true }); + } catch { + // Already gone, or never fully created. The row is still deleted below regardless. + } // 3. The commander itself. forgotten.push(...deleteClaudeSession(who, who.home, sessionId)); @@ -675,10 +724,16 @@ commandHqRouter.post('/purge', async (ctx) => { Scanning one directory would have deleted every commander that chose a different one — a housekeeping route quietly unfiling live commanders is the worst possible version of this bug, because the rows are - what identity depends on now. + what identity depends on now. A row with no transcript yet (`sessionId: null`, mid-creation) is never a + purge candidate at all — see `purgeCommandHqSessions`'s own guard — so it needs no entry in this set. */ const rows = await listCommandHqSessions(user.id); - const stillThere = new Set(rows.map((row) => row.sessionId).filter((id) => !!loadClaudeSessionSummaryById(who, id))); + const stillThere = new Set( + rows + .map((row) => row.sessionId) + .filter((id): id is string => id !== null) + .filter((id) => !!loadClaudeSessionSummaryById(who, id)), + ); const removed = await purgeCommandHqSessions(user.id, stillThere); return ctx.json({ removed }); }); @@ -716,7 +771,7 @@ commandHqRouter.get('/commanders/:sessionId/children', async (ctx) => { const row = await getCommandHqSession(user.id, sessionId); if (!row) throw errors.NOT_FOUND('not a commander'); - const children = await ingestSpawnedChildren(row.id, listSpawnedSessions(who.email, sessionId)); + const children = await ingestSpawnedChildren(row.id, listSpawnedSessions(who.email, row.dirName)); // `unknownCost` said out loud rather than summed as zero: a commander whose children's costs are unknown // must not display a confident total that happens to be too low. That understatement is the whole reason // this exists. @@ -731,23 +786,25 @@ commandHqRouter.get('/commanders/:sessionId/children', async (ctx) => { /** * GET /command-hq/commanders/:sessionId/record-paths * - * Where this commander's loop must write its pid and the session ids it spawns. + * Where this commander's loop must write its pid and the session ids it spawns — the same two paths + * `CLAUDE.md` already states at creation (`docs/commander-directory.md`). This route still exists for + * `BackgroundJobDialog`, which needs them again at ARM time, well after creation, to build the watch loop's + * own prompt (`background-job-prompt.ts`) — a live turn's `CLAUDE.md` is not something this route re-reads. * - * The composer needs these to build the background-job prompt, and cannot derive them: they are rooted in - * `DATA_PATH`, which is a server fact. Returning paths rather than accepting an upload is deliberate — the - * loop writes them itself with a shell redirect, which needs no token and works from inside a background - * job, where an authenticated POST would need a credential the loop has no business holding. - * - * Not gated on the commander existing as a ROW: a brand-new commander asks for these before its first turn - * has filed it, and refusing then would mean the very first loop is the one that cannot record itself. + * Rooted in `state/` inside this commander's own directory, keyed by `dirName` — which means it now needs + * the ROW (to learn `dirName`), unlike before, when the path was a pure function of the transcript id and + * nothing else. A commander always has a row by the time any turn exists, so this is not a new restriction + * in practice — it only makes explicit what used to be implicit. */ commandHqRouter.get('/commanders/:sessionId/record-paths', async (ctx) => { const user = ctx.get('user'); const who = await commandHqIdentity(user); const sessionId = ctx.req.param('sessionId'); + const row = await getCommandHqSession(user.id, sessionId); + if (!row) throw errors.NOT_FOUND('not a commander'); return ctx.json({ - loopPidPath: loopPidPath(who.email, sessionId), - spawnedPath: spawnedRecordPath(who.email, sessionId), + loopPidPath: loopPidPath(who.email, row.dirName), + spawnedPath: spawnedRecordPath(who.email, row.dirName), }); }); diff --git a/src/servers/api/command-hq/spawned.ts b/src/servers/api/command-hq/spawned.ts index 50f0be97..6555a682 100644 --- a/src/servers/api/command-hq/spawned.ts +++ b/src/servers/api/command-hq/spawned.ts @@ -22,11 +22,12 @@ import { descendantPids } from '../tasks/process-tree'; So the loop records it, and the platform reads it. One newline-delimited file per commander: - //commander-state//spawned + //commanders//state/spawned The loop can only do that if it knows which commander it belongs to, which it does not by default — the - id is injected into the background-job prompt by `background-job-prompt.ts`. That is the whole chain, and - every link is required. + path is written into `CLAUDE.md` at creation (`docs/commander-directory.md`) and also carried into the + background-job prompt by `background-job-prompt.ts` for the dialog-driven case. That is the whole chain, + and every link is required. */ const FILE = 'spawned'; @@ -36,8 +37,8 @@ const PID_FILE = 'loop-pid'; const SESSION_ID = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; /** Where the loop should append. Handed to the agent in its prompt; nothing guesses it. */ -export function spawnedRecordPath(email: string, commanderSessionId: string): string { - return join(getCommanderStateDir(email, commanderSessionId), FILE); +export function spawnedRecordPath(email: string, dirName: string): string { + return join(getCommanderStateDir(email, dirName), FILE); } /** @@ -47,8 +48,8 @@ export function spawnedRecordPath(email: string, commanderSessionId: string): st * truncated write, a blank line or a stray log line must not break a delete — they are dropped and the rest * is used. A record that fails closed here would make a commander undeletable because of a bad line. */ -export function listSpawnedSessions(email: string, commanderSessionId: string): string[] { - const path = spawnedRecordPath(email, commanderSessionId); +export function listSpawnedSessions(email: string, dirName: string): string[] { + const path = spawnedRecordPath(email, dirName); if (!existsSync(path)) return []; try { const seen = new Set(); @@ -68,18 +69,26 @@ export function listSpawnedSessions(email: string, commanderSessionId: string): * Kept because the platform spawning an agent on a commander's behalf is the obvious next shape, and when * that arrives it must write the same file rather than a second record. */ -export function recordSpawnedSession(email: string, commanderSessionId: string, spawnedId: string): void { +export function recordSpawnedSession(email: string, dirName: string, spawnedId: string): void { if (!SESSION_ID.test(spawnedId)) return; - if (listSpawnedSessions(email, commanderSessionId).includes(spawnedId.toLowerCase())) return; - const dir = getCommanderStateDir(email, commanderSessionId); + if (listSpawnedSessions(email, dirName).includes(spawnedId.toLowerCase())) return; + const dir = getCommanderStateDir(email, dirName); mkdirSync(dir, { recursive: true }); appendFileSync(join(dir, FILE), `${spawnedId.toLowerCase()}\n`, { mode: 0o600 }); } -/** Drop the whole record. Called when the commander is deleted, after its children have been dealt with. */ -export function clearSpawnedRecord(email: string, commanderSessionId: string): void { +/** + * Drop `state/` alone, leaving `CLAUDE.md` and anything else the commander wrote in place. + * + * Not called from the delete route any more — deleting a commander now removes its WHOLE directory + * (`docs/commander-directory.md`: `state/` is one part of it, not the only thing that needs to go), which + * subsumes this. Kept as the narrower primitive for the shape of operation it names: clearing what a + * commander has HANDLED without deleting the commander itself, which nothing calls today but is exactly + * the kind of reset a commander asked to "start over" might need. + */ +export function clearSpawnedRecord(email: string, dirName: string): void { try { - rmSync(getCommanderStateDir(email, commanderSessionId), { recursive: true, force: true }); + rmSync(getCommanderStateDir(email, dirName), { recursive: true, force: true }); } catch { // A record we cannot remove is untidy, not incorrect: the commander is gone and nothing reads this // without a commander to read it for. @@ -103,13 +112,13 @@ export function clearSpawnedRecord(email: string, commanderSessionId: string): v */ /** Where the loop should write its own pid. Handed to it in its prompt, like the spawned record. */ -export function loopPidPath(email: string, commanderSessionId: string): string { - return join(getCommanderStateDir(email, commanderSessionId), PID_FILE); +export function loopPidPath(email: string, dirName: string): string { + return join(getCommanderStateDir(email, dirName), PID_FILE); } /** The loop's pid, if it recorded one and the process is still alive. */ -export function liveLoopPid(email: string, commanderSessionId: string): number | null { - const path = loopPidPath(email, commanderSessionId); +export function liveLoopPid(email: string, dirName: string): number | null { + const path = loopPidPath(email, dirName); if (!existsSync(path)) return null; const pid = Number(readFileSync(path, 'utf-8').trim()); if (!Number.isInteger(pid) || pid <= 1) return null; @@ -128,8 +137,8 @@ export function liveLoopPid(email: string, commanderSessionId: string): number | * * Children first, then the loop, so the loop cannot spawn a replacement between the two. */ -export function stopCommanderProcesses(email: string, commanderSessionId: string): number { - const pid = liveLoopPid(email, commanderSessionId); +export function stopCommanderProcesses(email: string, dirName: string): number { + const pid = liveLoopPid(email, dirName); if (!pid) return 0; const tree = [...descendantPids(pid), pid]; let signalled = 0; diff --git a/src/servers/data-path.ts b/src/servers/data-path.ts index bb9da9ef..d4e526ff 100644 --- a/src/servers/data-path.ts +++ b/src/servers/data-path.ts @@ -198,17 +198,27 @@ export const getEmailAttachmentCacheDir = (ownerEmail: string) => join(getEmailAccountsDir(ownerEmail), 'attachment_cache'); /** - * Every commander a user has shares this ONE directory as its cwd — deliberately not one directory per - * commander. The first design was per-commander, and was dropped the same night it was proposed: a fresh - * directory would have to exist before or exactly when a commander's first turn fires, and racing that - * creation against the harness's own spawn is precisely the kind of timing hazard Claude Code's - * directory-per-project convention was never built to be driven by. One shared, pre-existing directory - * removes the race instead of handling it. + * The root every commander's own directory lives under — `$DATA_PATH//commanders//`. + * + * **Not a shared cwd any more.** Two designs were tried and retracted before this one. First, one directory + * per commander, dropped the night it was proposed: a fresh directory had to exist before or exactly when a + * commander's first turn fired, and racing that creation against the harness's own spawn was the kind of + * timing hazard Claude Code's directory-per-project convention was never built to be driven by. So it became + * one shared directory for every commander a user has — which removed the race but made the directory the + * commander's only identity, which is what made the ordering problem `docs/commander-directory.md` describes + * inescapable. Then, briefly (2026-08-22), a commander chose an arbitrary directory anywhere in its owner's + * tree, once identity moved to the `command_hq_sessions` row instead of the cwd. + * + * **One directory per commander again, and the race is gone for a different reason than either previous + * attempt solved it.** The row (and so `dirName`) is created by `POST /command-hq/commanders` BEFORE any + * turn spawns — the ordering fix is exactly what makes a fresh, empty, commander-named directory safe to + * create ahead of time: nothing races its own creation, because creation happens first and spawning waits + * on it. See `docs/commander-directory.md`. * * `DATA_PATH`-rooted even for the OWNER, which is a deliberate departure from ordinary `/chat` — an owner's * normal chat runs in `getOwnerHomeDir` (their real login home). Commanders use the managed directory * instead, on both sides of the account split, which is what keeps them out of the owner's own project - * transcripts as surely as `command_hq_sessions` keeps them out of the `/chat` list. See `docs/command-hq.md`. + * transcripts as surely as `command_hq_sessions` keeps them out of the `/chat` list. * * **A sibling of `home`, but reachable like `home` — not confined like every other sibling.** Every other * directory alongside a member's `home` (`attachments`, `email_accounts`, …) is 700 and owned by the @@ -216,11 +226,51 @@ export const getEmailAttachmentCacheDir = (ownerEmail: string) => * wrong for a directory a member's own `claude` must `cd` into via `setpriv`. `confineUserTree` * (`os-user.ts`) chowns this one to the member the same way it chowns `home`. An account provisioned * BEFORE this existed will not have that chown until it is re-provisioned once — see `docs/command-hq.md`. + * + * That chown is recursive at provisioning time only — a subdirectory created for a NEW commander afterwards + * is written by the platform process (the service user), not by the member, so it will not automatically + * carry the member's ownership the way the root does. Not solved here; flagged in + * `docs/commander-directory.md`'s implementation notes as a gap for a member-owned commander specifically, + * not a regression this file introduces. */ export const getCommandersDir = (email: string) => join(DATA_PATH, email, 'commanders'); /** - * Where a commander records what it spawned — one directory per commander, named by its session id. + * One commander's own directory — `CLAUDE.md`, `state/`, and whatever it writes for itself. `dirName` is + * `command_hq_sessions.dir_name`, derived from the row's own serial id at creation. See + * `docs/commander-directory.md`. + */ +/** + * One commander's own directory. + * + * **Guarded, because two callers `rm -rf` what this returns** — deleting a commander removes its directory, + * and `dropSpawnedRecord` removes its `state/`. An empty `dirName` would resolve to the commanders ROOT and + * take every commander the account has with it. + * + * That is not hypothetical arithmetic. `createCommander` inserts the row with `dirName: ''` and fixes it up + * in the same statement pair, because the name is derived from a serial id that does not exist until the + * insert has happened. The transaction means nothing else can observe that state — and a guard here means + * it does not have to, which is the difference between safe and safe-so-far. A traversal is refused for the + * same reason, cheaply, rather than reasoned about. + */ +export const getCommanderDir = (email: string, dirName: string): string => { + if (!dirName || dirName.includes('/') || dirName === '.' || dirName === '..') { + throw new Error(`refusing to resolve a commander directory for ${JSON.stringify(dirName)}`); + } + return join(getCommandersDir(email), dirName); +}; + +/** + * Where a commander records what it spawned and its loop's own pid — `state/` inside its own directory, + * keyed by `dirName` rather than a transcript id. + * + * Used to be a directory of its own (`commander-state//`, a sibling of the transcripts + * rather than the commander), because there was nothing else to key it on before a commander had an + * identity independent of its transcript. Collapsed in here 2026-08-24: `state/` is INSTANCE data (what a + * commander has handled, what it spawned, its loop's pid) as opposed to `CLAUDE.md` and whatever else the + * commander writes beside it, which is DEFINITION — the split `docs/commander-directory.md` calls + * structural, kept as a subdirectory rather than a convention precisely so it is enforced by a path rather + * than remembered by whoever writes into it. * * Beside the transcripts rather than inside them: a transcript is the harness's file and the platform does * not write into it. `spawned` is a plain newline-delimited list of session ids, appended by the loop the @@ -231,8 +281,7 @@ export const getCommandersDir = (email: string) => join(DATA_PATH, email, 'comma * That makes this record load-bearing rather than convenient. Without it a commander cannot be deleted * completely, its children cannot be followed, and its cost cannot be totalled. */ -export const getCommanderStateDir = (email: string, sessionId: string) => - join(DATA_PATH, email, 'commander-state', sessionId); +export const getCommanderStateDir = (email: string, dirName: string) => join(getCommanderDir(email, dirName), 'state'); /** * Which mail account's store to use, found on DISK rather than by asking the database. diff --git a/src/servers/sidecar/claude/claude-manager.ts b/src/servers/sidecar/claude/claude-manager.ts index 3ca3cef2..f8feeb28 100644 --- a/src/servers/sidecar/claude/claude-manager.ts +++ b/src/servers/sidecar/claude/claude-manager.ts @@ -634,18 +634,18 @@ async function createSession( // written an hour ago should be offered on the next session. const skills = await skillsBlock(); /* - And, for a commander, the two paths only this session can be told. + No commander block here, and none in `startClaudeTurn` either, as of `docs/commander-directory.md`. - Not in the skill, which is read on demand and cannot know them; not only in the background-job dialog's - preamble, which is where this used to live and which an agent arming itself never sees. See - `api/command-hq/commander-block.ts`. - */ - /* - No commander block here, deliberately. + Two things used to live here in turn, both dead ends. First, the background-job dialog's own preamble, + which an agent arming itself from the conversation never saw. Then a per-turn block riding + `effectivePrompt` (`commander-block.ts`, deleted 2026-08-24) — which could not resolve on a commander's + FIRST turn either, since this function runs once per session and that turn has no row and no transcript + id yet, so the block was null and no later turn ever rebuilt the preamble. - It used to be composed in, and could never arrive: this runs once per session, and a commander's first - turn has no row and no transcript id, so the block resolved to null and no later turn rebuilt the - preamble. It rides `effectivePrompt` instead — see `startClaudeTurn` in api/chat/websocket.ts. + `POST /command-hq/commanders` creates the row, its directory and its `CLAUDE.md` BEFORE any turn spawns + now, so a commander is told its name, its reference path and the two paths it must record itself at by a + FILE Claude Code reads from its own working directory at session start — the same mechanism this + function's `affordances`/`skills` blocks still use for everything else, minus the per-turn cost. */ const preamble = [affordances, skills].filter(Boolean).join('\n\n') || null; diff --git a/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/EmbeddableChat.tsx b/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/EmbeddableChat.tsx index c117a912..bdcda6d6 100644 --- a/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/EmbeddableChat.tsx +++ b/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/EmbeddableChat.tsx @@ -11,8 +11,11 @@ type EmbeddableChatProps = { * What the transcript shows before there is anything in it. `null` shows nothing. * * The default, t('chat.composer.startHint'), is true on the chat screen and wrong wherever something else - * already explains what to do — a new commander has a setup card and a Start button above it, so the - * message contradicts them and takes 200px of height while doing it. + * already explains what to do, or where nothing shows it at all: a new commander used to render its setup + * card and Start button inside the same pane as this composer, which is what this prop was added for — + * `docs/commander-directory.md` moved setup into its own component, mounted before any chat exists, so + * there is no longer a caller passing `null` here. Left in place; a future caller with the same shape of + * conflict should use it rather than reinvent it. */ emptyState?: ReactNode; /** Which panel this is — see `queue-store`. Absent on the screen, where there is only one chat. */ @@ -44,9 +47,12 @@ type EmbeddableChatProps = { /** * Refuse input entirely, with a reason the composer shows. * - * For a surface where something must be chosen before a conversation can start — a new commander needs a - * name and a working directory, both unchangeable afterwards. Disabling the composer is the only honest - * place to enforce that: anywhere later and the person has already written what they meant to say. + * For a surface where something must be chosen before a conversation can start at all. Disabling the + * composer is the only honest place to enforce that: anywhere later and the person has already written + * what they meant to say. No current caller uses this — a new commander used to (name and working + * directory, both unchangeable afterwards), until `docs/commander-directory.md` moved that choice into a + * separate setup step BEFORE any chat exists to disable, which needed no `disabled` at all — but the prop + * is generic and worth keeping for the next surface with the same shape of requirement. */ disabled?: false | { reason: string }; onMessageComplete?: () => void; diff --git a/src/workspaces/officerdev/src/apps/CommandHQ/CommandHqDetail.tsx b/src/workspaces/officerdev/src/apps/CommandHQ/CommandHqDetail.tsx index 3c85e5ec..315145b9 100644 --- a/src/workspaces/officerdev/src/apps/CommandHQ/CommandHqDetail.tsx +++ b/src/workspaces/officerdev/src/apps/CommandHQ/CommandHqDetail.tsx @@ -17,7 +17,9 @@ import { useCommandHqChildren, useCommandHqCommander, useCommandHqCommanderActions, + useCreateCommander, } from './useCommandHq'; +import type { CommandHqCommanderCreated } from './types'; import { formatCommanderCost, formatTokens } from './format'; import { NEW_COMMANDER_GUIDE } from './newCommanderGuide'; import { useTranslation } from '../../locales'; @@ -26,9 +28,10 @@ import { useTranslation } from '../../locales'; // read the route directly, per `docs/navigation-audit.md`): // // no :sessionId → empty state, "select or start one" -// :sessionId === 'new' → the composer that CREATES a commander (an ordinary chat turn with -// `context: 'command-hq'`, per `docs/command-hq.md` → "How a commander is -// created" — there is no separate "file this" step) +// :sessionId === 'new' → the composer. `POST /command-hq/commanders` creates the row, ITS directory +// and `CLAUDE.md` first (`NewCommanderSetup`, below); only once that exists does +// a chat turn start, with `cwd` set to that directory (`ActiveNewCommander`) — +// see `docs/commander-directory.md` for why the ordering is the fix // :sessionId is a real id → the commander's own transcript, resumed, plus a metadata bar const CHAT_TAIL = 20; @@ -89,12 +92,13 @@ const NewCommanderGuide = () => { * before asking anything at all, all live in the Commanders skill — which is re-read from disk every run, so * it can be reworded without a deploy. Duplicating any of it here would create a second copy to drift. */ -const startBrief = (name: string): string => +const startBrief = (name: string, referencePath: string): string => [ // i18n-exempt: agent brief — the MODEL reads this; the transcript shows displayText to the person `You have been named "${name}", and that is the only thing I have told you about what I want.`, - 'It is not decoration — I chose it before anything else, so treat it as the subject and read this', - 'directory looking for what it refers to.', + `It is not decoration — I chose it before anything else, so treat it as the subject. Your own`, + `CLAUDE.md already names what to look at: \`${referencePath}\`. Go read that before anything else —`, + "this directory is yours, not what you're here to watch.", '', 'Read the **Commanders** skill before you reply. It is how this works, and it is short.', '', @@ -148,20 +152,20 @@ type StartCommanderButtonProps = { * these choices without leaving the page, and somebody who already knows can fill both in without opening * anything. * - * The directory is a button rather than a text field. It is an absolute path on the server and typing one is - * how you get a commander running in a directory that does not exist — `DirPickerModal` browses the account's - * own tree, which is the same containment the server applies to whatever it is handed. + * The reference path is a button rather than a text field. It is an absolute path on the server and typing + * one is how you get a commander pointed at a path that does not exist — `DirPickerModal` browses the + * account's own tree, which is the same containment the server applies to whatever it is handed. */ const CommanderSetup = ({ name, onName, - cwd, - onPickDirectory, + referencePath, + onPickReferencePath, }: { name: string; onName: (next: string) => void; - cwd: string | null; - onPickDirectory: () => void; + referencePath: string | null; + onPickReferencePath: () => void; }) => { const { t, tUi } = useTranslation(); return ( @@ -186,34 +190,95 @@ const CommanderSetup = ({
- {t('commandHq.workingDirectory')} + {t('commandHq.referencePath')} - {t('commandHq.cwdHint')} + {t('commandHq.referencePathHint')}
); }; +/** + * The setup step — name and reference path, nothing live yet. No `useChat` here at all: there is nothing to + * talk to until `POST /command-hq/commanders` returns a row, its directory and its `CLAUDE.md` + * (`docs/commander-directory.md`). `onCreated` hands the result up to `NewCommander`, which switches to + * `ActiveNewCommander` — a fresh mount, not a state transition inside one component, which is what lets + * that component's own `useChat` be constructed with `commanderId` already known from its very first render + * rather than raced against a `setState` that has not landed yet. + */ +function NewCommanderSetup({ + category, + onCreated, +}: { + category: number | 'none' | undefined; + onCreated: (commander: CommandHqCommanderCreated) => void; +}) { + const [name, setName] = useState(''); + const [referencePath, setReferencePath] = useState(null); + const [pickerOpen, setPickerOpen] = useState(false); + const [creating, setCreating] = useState(false); + const ready = name.trim().length > 0 && !!referencePath; + const createCommander = useCreateCommander(); + + const onStart = async () => { + if (!ready || creating) return; + setCreating(true); + try { + const commander = await createCommander.mutateAsync({ + name: name.trim(), + referencePath: referencePath!, + categoryId: typeof category === 'number' ? category : undefined, + }); + onCreated(commander); + } catch (err) { + toast.error(errorText(err, 'could not create this commander')); + setCreating(false); + } + }; + + return ( +
+ setPickerOpen(true)} + /> + void onStart()} + /> + + setPickerOpen(false)} onSelect={setReferencePath} /> +
+ ); +} + /** * A commander started in THIS pane, and it keeps the same live chat for as long as the pane is open. * - * `useChat` mints the transcript id on the harness's first response (`session:claude`); once that lands the - * URL is replaced onto it so a refresh resumes this conversation instead of starting a second one. - * `replaceUrl: false` because `useChat`'s own permalink always writes `/chat/` (see `writePermalink`), - * which is the wrong screen here. + * Mounted only once `NewCommanderSetup` has a real row — `commander` is a prop, not state raced against a + * `useChat` built before it existed, which is the whole reason this is a SEPARATE component rather than a + * branch inside one. `useChat` mints the transcript id on the harness's first response (`session:claude`); + * once that lands the URL is replaced onto it so a refresh resumes this conversation instead of starting a + * second one. `replaceUrl: false` because `useChat`'s own permalink always writes `/chat/` (see + * `writePermalink`), which is the wrong screen here. * * **The URL change must not remount anything.** It used to: the pane swapped to `ExistingCommander`, which * tore down the live websocket and re-read the transcript from the API. The whole conversation vanished at @@ -222,103 +287,79 @@ const CommanderSetup = ({ * Tearing down mid-turn is worse than it looks with a commander, whose turns are long: the pane would drop * the stream and the reader would watch nothing happen for twenty minutes. * - * So this component holds the session from the composer through to the end, exactly as `/chat` does — a - * permalink is for a refresh, not an instruction to reload. `CommandHqDetail` keeps it mounted by + * So this component holds the session from the first message through to the end, exactly as `/chat` does — + * a permalink is for a refresh, not an instruction to reload. `CommandHqDetail` keeps it mounted by * remembering which id started here. */ -function NewCommander({ +function ActiveNewCommander({ + commander, category, onStarted, }: { + commander: CommandHqCommanderCreated; category: number | 'none' | undefined; onStarted: (id: string) => void; }) { const { t, tUi } = useTranslation(); const navigate = useNavigate(); - /* - A name and a working directory, both required BEFORE the first message. - - Neither can be set afterwards without cost. The directory decides where every relative path in every turn - resolves and which project group holds the transcripts, so changing it later would strand a conversation - away from its own history. The name has to reach the transcript the moment it exists — `commanderName` on - the turn — because Claude Code names a new session from its first prompt, and the platform's rename only - wins by getting there first. - - So the composer is disabled until both are chosen. That is why this lives on the card rather than in a - dialog: you read what a commander is and make the two decisions in the same place. - */ - const [name, setName] = useState(''); - const [cwd, setCwd] = useState(null); - const [pickerOpen, setPickerOpen] = useState(false); - const ready = name.trim().length > 0 && !!cwd; const chat = useChat(undefined, undefined, { replaceUrl: false, context: 'command-hq', - // The name rides the turn; the DIRECTORY is a prop on EmbeddableChat below, because that is what feeds - // `sendPrompt`'s `cwdParam`. Two different paths for two values chosen in the same place, which is worth - // a note rather than a discovery. - ...(name.trim() ? { commanderName: name.trim() } : {}), + // Known from this component's very first render — see the docstring above for why that timing is the + // whole point of the split. Rides every turn, not just the first: `resolveChatCwd` reads it server-side + // on every one, and `fileAsCommander` uses it to point this row at the transcript id the moment it + // exists. + commanderId: commander.id, }); - // Also the transcript id: `command_hq_sessions` is keyed on it, so an action addressed by officer's key - // updates nothing and says nothing. Same root cause as the navigate below. - const { setCategory } = useCommandHqCommanderActions(chat.claudeSessionId ?? ''); - const [filed, setFiled] = useState(false); /* - Has this commander been started — which is NOT the same as "has it any messages". - - `messages.length === 0` was standing in for it, and it lies in exactly the situation that matters. - Lose the socket mid-turn and the messages come back empty, so the setup pane and its Start button - reappear on a commander that is already running; pressing Start then sends the opening brief a - SECOND time, and the agent answers a question nobody asked twice. Seen tonight, after a restart I - ran while a commander was live. - - A latch, not a derived value. Once started, always started — the point is to survive the moment the - live state goes empty, so anything recomputed from that state is the bug rather than the fix. + Sent exactly once, the moment this component mounts — not on a button click captured in a closure, so + there is nothing here racing `setState` the way the old single-component version did. `useRef`, not + `useState`: a ref write is visible to the very next synchronous check, which is what makes "send it + exactly once" enforceable at all without a second render to notice a state flag had already flipped. */ - const [started, setStarted] = useState(false); + const sentRef = useRef(false); useEffect(() => { - if (chat.messages.length > 0 || chat.claudeSessionId || chat.isGenerating) setStarted(true); - }, [chat.messages.length, chat.claudeSessionId, chat.isGenerating]); + if (sentRef.current) return; + sentRef.current = true; + chat.sendPrompt( + startBrief(commander.name ?? '', commander.referencePaths[0] ?? ''), + undefined, + undefined, + { path: commander.cwd }, + undefined, + undefined, + 'Set me up.', + ); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + /* Refresh the grid when the first turn ends. - - The commander's NAME is written server-side, into the transcript's summary record, on the first - `result` — the rename can only land once the transcript exists. Nothing tells the client that - happened, so the list kept whatever title it had fetched before, which for a self-started commander - is the opening brief: the pill read "I have given you a name and a working directory…" while the - header beside it read the real name from local state. - - The falling edge of `isGenerating` is the signal, rather than a poll or a shorter `staleTime`: it is - the one moment the server is known to have written something the list is showing. + + Command HQ's own metadata query (`GET /commanders/:sessionId`) has nothing to show until the row has a + `sessionId` — which lands mid-turn, off `fileAsCommander` — so the grid and any open detail view are + stale until this fires once. */ const wasGenerating = useRef(false); const qc = useQueryClient(); useEffect(() => { if (wasGenerating.current && !chat.isGenerating) { - // Both shapes, not just the grid: the header reads the per-commander query and went stale the same - // way, so the first attempt at this fixed the pill and left the title beside it still wrong. qc.invalidateQueries({ queryKey: ['command-hq', 'commanders'] }); qc.invalidateQueries({ queryKey: ['command-hq', 'commander'] }); } wasGenerating.current = chat.isGenerating; }, [chat.isGenerating, qc]); + const [filed, setFiled] = useState(false); useEffect(() => { - // `claudeSessionId`, NOT `sessionId`. The commander is filed under Claude's transcript id - // (`fileAsCommander` in websocket.ts), and this URL has to match or the detail pane fetches an id - // nothing knows. `sessionId` arrives FIRST — `session:init` sets it to officer's own key — so - // navigating on it sent the page to an id that 404s, and `filed` then blocked the correction when the - // real one arrived. A commander that had run and answered correctly rendered as "Send a message to - // start". Waiting for the transcript id costs one more round trip and is the only id that works. + // `claudeSessionId`, NOT `sessionId`. The commander is addressed by Claude's transcript id everywhere + // else in Command HQ, and this URL has to match or the detail pane fetches an id nothing knows. + // `sessionId` arrives FIRST — `session:init` sets it to officer's own key — so navigating on it sent the + // page to an id that 404s, and `filed` then blocked the correction when the real one arrived. if (!chat.claudeSessionId || filed) return; setFiled(true); - // `fileAsCommander` (websocket.ts) always files with no category — it has no way to know one was - // requested from here. The same race the `/commanders/new` HTTP route resolves server-side by always - // running its UPDATE last; done client-side here for the same reason, since the web composer goes - // through the websocket path instead. - if (typeof category === 'number') void setCategory.mutateAsync(category).catch(() => {}); onStarted(chat.claudeSessionId); navigate(commandHqSessionPath(chat.claudeSessionId, category), { replace: true }); // eslint-disable-next-line react-hooks/exhaustive-deps @@ -329,15 +370,9 @@ function NewCommander({ {chat.claudeSessionId ? (