diff --git a/src/databases/officer_db/src/chat-session-cleanup.ts b/src/databases/officer_db/src/chat-session-cleanup.ts new file mode 100644 index 00000000..0f47daee --- /dev/null +++ b/src/databases/officer_db/src/chat-session-cleanup.ts @@ -0,0 +1,42 @@ +import { inArray } from 'drizzle-orm'; +import { db } from './db'; +import { chatSessionEvents } from './chat-events/schema'; +import { chatNotificationPrefs } from './chat-notification-prefs/schema'; +import { chatSessionModels } from './chat-session-models/schema'; +import { nudges } from './nudges/schema'; + +/* + Everything the platform stores ABOUT a conversation, forgotten together. + + ── Why this exists as one function ── + + Four tables are keyed on a transcript id and none of them has a foreign key to anything, because the thing + they describe is a FILE — a transcript on disk, written by the harness. There is no row for a chat session + to reference, so the database cannot cascade and the delete path has to be explicit. + + It was not. Deleting a conversation removed the transcript and left all four behind, for ever. Found on + 2026-08-22 by counting rows after two commanders were deleted through the UI: `chat_session_models` held 36 + rows against far fewer live conversations. + + ── Why it is more than untidy ── + + Three of the four are inert — a model, a bell preference, a durable copy of turn output that nothing will + ask for again. `nudges` is not: it is a SCHEDULE, and the tick in `nudger.ts` reads it. A nudge left behind + by a deleted conversation is a timer that keeps firing at something that does not exist. + + ── Why it takes a list ── + + A `/clear` chain is several transcripts presenting as one conversation, and deleting it deletes every part. + Forgetting one id and leaving its siblings is the same bug at a smaller scale, so the caller passes the ids + it actually removed. +*/ +export async function forgetChatSessions(sessionIds: string[]): Promise { + if (!sessionIds.length) return; + // Sequential rather than a transaction: these are independent tables with no invariant between them, and + // a partial forget is strictly better than a failed delete that leaves the transcript gone and every row + // in place. Each is idempotent — running it twice removes nothing the second time. + await db.delete(nudges).where(inArray(nudges.sessionId, sessionIds)); + await db.delete(chatSessionModels).where(inArray(chatSessionModels.sessionId, sessionIds)); + await db.delete(chatNotificationPrefs).where(inArray(chatNotificationPrefs.sessionId, sessionIds)); + await db.delete(chatSessionEvents).where(inArray(chatSessionEvents.sessionId, sessionIds)); +} diff --git a/src/databases/officer_db/src/index.ts b/src/databases/officer_db/src/index.ts index 5c0c7d0d..34e9a5e3 100644 --- a/src/databases/officer_db/src/index.ts +++ b/src/databases/officer_db/src/index.ts @@ -39,6 +39,7 @@ export * from './nudges'; export * from './ai-provider-usage'; export * from './anthropic-accounts'; export * from './theme-favorites'; +export { forgetChatSessions } from './chat-session-cleanup'; export * from './command-hq'; export * from './dashboards'; export * from './integrations'; diff --git a/src/servers/api/chat/chat.ts b/src/servers/api/chat/chat.ts index 45c4459f..13eb167c 100644 --- a/src/servers/api/chat/chat.ts +++ b/src/servers/api/chat/chat.ts @@ -12,6 +12,7 @@ import { getChatNotifyPreference, getChatNotifyPreferences, setChatNotifyPreference, + forgetChatSessions, } from 'officerdb'; import type { NotifyPreference } from 'officerdb/types'; import { setNudge, getNudge, cancelNudge } from 'officerdb'; @@ -376,8 +377,21 @@ chatRouter.delete('/sessions/:id', async (ctx) => { const id = ctx.req.param('id'); const cwd = cwdOf(ctx, who.home); if (isOpenCodeSessionId(id) && !who.isOwner) return ctx.text('Not found', 404); - const ok = isOpenCodeSessionId(id) ? await deleteOpenCodeSession(id) : deleteClaudeSession(who, cwd, id); - if (!ok) return ctx.text('Not found', 404); + if (isOpenCodeSessionId(id)) { + if (!(await deleteOpenCodeSession(id))) return ctx.text('Not found', 404); + return ctx.json({ ok: true }); + } + + // `.length`, not truthiness: this returns the IDS it removed and an empty array is truthy in JavaScript, + // so `if (removed)` would report a delete that found nothing as a success. + const removed = deleteClaudeSession(who, cwd, id); + if (!removed.length) return ctx.text('Not found', 404); + + // The transcript is gone; forget what the platform stored ABOUT it. Four tables are keyed on a transcript + // id with no foreign key — there is no row for a conversation to reference — so nothing cascades and this + // has to be explicit. One of them, `nudges`, is a live schedule: left behind it keeps firing at a + // conversation that no longer exists. + await forgetChatSessions(removed); return ctx.json({ ok: true }); }); diff --git a/src/servers/api/chat/claude-sessions.ts b/src/servers/api/chat/claude-sessions.ts index 8ab6975e..dba70740 100644 --- a/src/servers/api/chat/claude-sessions.ts +++ b/src/servers/api/chat/claude-sessions.ts @@ -642,9 +642,12 @@ const findTranscript = (who: ChatIdentity, cwd: string, sessionId: string): stri * routinely (see `findTranscript`), and the wrong group would find no chain and silently delete one * part of several. */ -export function deleteClaudeSession(who: ChatIdentity, cwd: string, sessionId: string): boolean { +export function deleteClaudeSession(who: ChatIdentity, cwd: string, sessionId: string): string[] { const filePath = findTranscript(who, cwd, sessionId); - if (!filePath) return false; + // The IDS it removed, not a boolean — empty means nothing was there. The caller needs them to forget the + // rows keyed on each one (`forgetChatSessions`), and a `/clear` chain is several transcripts presenting as + // one conversation: returning true would have told a caller to forget the head and orphan its parts. + if (!filePath) return []; const ownCwd = firstCwd(who.osUser, filePath); const ids = ownCwd ? chainFileIds(who, ownCwd, sessionId) : [sessionId]; @@ -653,7 +656,7 @@ export function deleteClaudeSession(who: ChatIdentity, cwd: string, sessionId: s // success — the conversation reappeared on the next refresh. const dir = dirname(filePath); for (const id of ids) removeAs(who.osUser, join(dir, `${id}.jsonl`)); - return true; + return ids; } /** diff --git a/src/servers/api/chat/websocket.ts b/src/servers/api/chat/websocket.ts index 2f672ee3..e4e39539 100644 --- a/src/servers/api/chat/websocket.ts +++ b/src/servers/api/chat/websocket.ts @@ -464,19 +464,41 @@ function createMessageHandler( 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 => { + // TEMPORARY instrumentation, 2026-08-22. The chosen name has failed to reach the transcript twice, and + // reasoning about the path has been wrong twice. Remove once the cause is known. + logger.warn('[commander-name] fileAsCommander', { + commandHq, + transcriptId, + hasName: !!commanderName, + name: commanderName ?? null, + alreadyNamed: commanderNamed, + }); if (!commandHq || !transcriptId) return; const userId = sessionManager.getSession(sessionId)?.userId; if (!userId) return; const filedId = transcriptId; void createCommandHqSession(userId, filedId) .then(async () => { - // The chosen name, applied once and only if one was chosen. - // - // `renameClaudeSession` writes a `summary` record, which is what sets `hasExplicitTitle` — and an - // explicit title is never overwritten by the harness's own `ai-title` or by the first-message - // fallback. So this wins permanently rather than racing the titling. - if (!commanderName?.trim()) return; + /* + 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); @@ -487,7 +509,8 @@ function createMessageHandler( // 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. - renameClaudeSession(who, who.home, filedId, commanderName.trim()); + commanderNamed = renameClaudeSession(who, who.home, filedId, commanderName.trim()); + logger.warn('[commander-name] rename attempt', { filedId, home: who.home, ok: commanderNamed }); }) .catch((err: unknown) => { logger.warn('Could not file this session as a commander', { @@ -554,6 +577,9 @@ 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 diff --git a/src/servers/api/command-hq/router.ts b/src/servers/api/command-hq/router.ts index 3e70ee8b..d88c75c0 100644 --- a/src/servers/api/command-hq/router.ts +++ b/src/servers/api/command-hq/router.ts @@ -5,7 +5,7 @@ import { checkBudget } from '../sub-agents/budget'; import * as errors from '@@/custom-errors'; import { resolveHomeDir } from '@@/user-home'; import { getCommandersDir, toShellUsername } from '@@/data-path'; -import { getChatEventsSince, ingestSpawnedChildren } from 'officerdb'; +import { forgetChatSessions, getChatEventsSince, ingestSpawnedChildren } from 'officerdb'; import { clearSpawnedRecord, listSpawnedSessions, @@ -552,14 +552,23 @@ commandHqRouter.delete('/commanders/:sessionId', async (ctx) => { (c) => c.spawnedSessionId, ); let childrenDeleted = 0; + // Every transcript id actually removed — the commander's and its children's, chains included. Collected + // rather than assumed, because a `/clear` chain is several ids behind one conversation. + const forgotten: string[] = []; for (const child of children) { try { // A spawned agent runs in the platform checkout, not the commanders directory, so its transcript // lives in that project's group — the same lookup `/chat/sessions/:id` falls back to. // The cwd is a HINT here, not an authority — `deleteClaudeSession` falls back to locating the - // transcript wherever it actually is, which is what makes this work for a child that ran in the - // platform checkout rather than the commanders directory. - if (deleteClaudeSession(who, who.home, child)) childrenDeleted += 1; + // transcript wherever it actually is, which is what makes this work for a child that ran in a + // different directory from its parent. + // + // `.length`, not truthiness: this returns the ids it removed, and an empty array is truthy. + const removedChild = deleteClaudeSession(who, who.home, child); + if (removedChild.length) { + childrenDeleted += 1; + forgotten.push(...removedChild); + } } catch { // Already gone, or never written. Counted as not-deleted rather than failing the request. } @@ -567,9 +576,21 @@ commandHqRouter.delete('/commanders/:sessionId', async (ctx) => { clearSpawnedRecord(who.email, sessionId); // 3. The commander itself. - deleteClaudeSession(who, who.home, sessionId); + forgotten.push(...deleteClaudeSession(who, who.home, sessionId)); await deleteCommandHqSession(user.id, sessionId); + /* + Forget what the platform stored ABOUT every transcript that just went — the commander's and its + children's, in one call. + + The `command_hq_*` tables cascade from the row, because they reference it. These four do not: they are + keyed on a transcript id, which is a FILE, so there is no row for them to reference and nothing to + cascade from. Left behind they are mostly inert — a model, a bell preference, a durable copy of turn + output — except `nudges`, which is a live schedule that would keep firing at a conversation that no + longer exists. + */ + await forgetChatSessions(forgotten); + // Said back rather than a bare ok: this is the one irreversible action here, and the counts are what // make it auditable after the fact. return ctx.json({ ok: true, processesStopped: stopped, childrenFound: children.length, childrenDeleted });