diff --git a/package.json b/package.json index 7ed7953f..dfb73baf 100644 --- a/package.json +++ b/package.json @@ -19,6 +19,7 @@ "build:dashboard": "bun run ./scripts/build/dashboard.ts", "db:gen": "cd src/databases/officer_db && bun run generate", "db:push": "bun run scripts/gen-plugin-schemas.ts && cd src/databases/officer_db && bun run push", + "postdb:push": "bun run --env-file=.env scripts/backfill-session-models.ts", "db:migrate": "cd src/databases/officer_db && bun run migrate", "dev:emailer": "cd src/workspaces/emailer && bun run dev", "format": "{ git diff --name-only HEAD -- 'src/**/*.ts' 'src/**/*.tsx'; git ls-files --others --exclude-standard -- 'src/**/*.ts' 'src/**/*.tsx'; } | xargs -r prettier --write", diff --git a/scripts/backfill-session-models.ts b/scripts/backfill-session-models.ts new file mode 100644 index 00000000..3e45df5c --- /dev/null +++ b/scripts/backfill-session-models.ts @@ -0,0 +1,74 @@ +/** + * Seed `chat_session_models` for conversations that existed before the table did. + * + * Runs from `postdb:push`, so every machine gets it on the next schema push rather than needing a manual + * step per server. Bun fires `pre`/`post` hooks for colon-named scripts — verified on 1.3.14 against + * `db:push`, because a `post` hook that silently never runs is the worst outcome for a migration. + * + * ── Why a family guess is good enough here ── + * + * A transcript records the API model (`claude-opus-5`), and Officer's picker offers CLI aliases + * (`claude-code/opus`, `claude-code/opus[1m]`). The mapping is one-way: the 1M variant and `opusplan` are + * not recoverable after the fact. So this seeds the FAMILY, which is right about which model answered and + * silent about the variant — and the first real turn afterwards replaces it with the exact alias. + * + * Compare the alternative, which is what this exists to prevent: with no row at all, resuming a session + * falls back to the first entry in the catalogue. That looked correct for as long as the first entry + * happened to be `opus`, and started showing every old conversation as `fable` the day `fable` was added + * above it. + * + * ── Rules it follows ── + * + * INSERT ONLY. `seedMissingChatSessionModels` never overwrites, so a session that has already recorded its + * exact alias keeps it, and re-running this is a no-op rather than a downgrade. + * + * NEVER FAILS THE PUSH. Every failure path exits 0 with a printed reason. A schema push that has already + * applied must not report failure because a cosmetic backfill could not read a transcript — and `postdb:push` + * failing would make `bun db:push` look like it had not worked. + * + * OWNER ONLY, for now. Members' transcripts are readable (`listTranscriptsAs` goes through `sudo`), but the + * platform's own user is not guaranteed to have that permission from a bare CLI invocation, and a member's + * sessions fill in on their next turn anyway. Stated here rather than silently scoped. + */ +import { listTranscriptModels } from '../src/servers/api/chat/claude-sessions'; +import { officerModelForApiModel } from '../src/servers/api/chat/model-aliases'; +import { chatSessionModelsTableExists, seedMissingChatSessionModels } from 'officerdb'; +import { homedir } from 'node:os'; + +const done = (message: string): never => { + console.log(`[backfill:session-models] ${message}`); + process.exit(0); +}; + +try { + // The push may have been a no-op, or aimed at a database where this table is commented out of + // `src/schema.ts`. Checking beats assuming — and beats a confusing "relation does not exist". + if (!(await chatSessionModelsTableExists())) done('chat_session_models is not there yet — nothing to do'); + + const owner = { email: '', home: homedir(), osUser: null, isOwner: true }; + const transcripts = listTranscriptModels(owner); + if (!transcripts.length) done('no transcripts found — nothing to do'); + + // Drop what cannot be named honestly: `` is filtered upstream, and mythos and OpenCode models + // have no Officer id. A dropped session shows no model rather than a wrong one. + const entries = transcripts.flatMap(({ sessionId, apiModel }) => { + const model = officerModelForApiModel(apiModel); + return model ? [{ sessionId, model }] : []; + }); + + const unmapped = transcripts.length - entries.length; + if (!entries.length) done(`${transcripts.length} transcript(s), none with a model Officer offers`); + + const inserted = await seedMissingChatSessionModels(entries); + const skipped = entries.length - inserted; + + done( + `${inserted} seeded` + + (skipped ? `, ${skipped} already recorded` : '') + + (unmapped ? `, ${unmapped} with no Officer equivalent` : '') + + ` (of ${transcripts.length} transcript(s))`, + ); +} catch (err) { + // Deliberately exit 0. See the header: the push has already applied, and this is bookkeeping. + done(`skipped — ${err instanceof Error ? err.message : String(err)}`); +} diff --git a/src/apps/officer-web/Screens/Dashboard/ChatHistory/index.tsx b/src/apps/officer-web/Screens/Dashboard/ChatHistory/index.tsx index 0d24e5ca..77434c7c 100644 --- a/src/apps/officer-web/Screens/Dashboard/ChatHistory/index.tsx +++ b/src/apps/officer-web/Screens/Dashboard/ChatHistory/index.tsx @@ -88,7 +88,14 @@ export const SessionListPage = ({ isNew }: SessionListPageProps) => { // first turn in the default dir during the window before this resolve landed. setSelected({ id: sessionId, - model: detail.model, + // `officerModel`, not `detail.model`. The latter is what the API served (`claude-opus-5`), which + // is a different id space from what the picker offers (`claude-code/opus[1m]`) — so it matched no + // catalogue entry, the selector fell back to whichever model was listed first, and a resumed + // conversation came back claiming the wrong one. Invisible for as long as the first entry was + // `opus`; obvious the day `fable` went above it. The server resolves this from the session's + // stored row, falling back to a family guess. Null means "unknown", and the picker must then + // leave the selection alone rather than guess — see ModelSelector. + model: detail.officerModel ?? undefined, resumeSessionId: sessionId, initialMessages: detail.messages as unknown as NonNullable['initialMessages'], total: detail.total, diff --git a/src/databases/officer_db/src/chat-session-models/index.ts b/src/databases/officer_db/src/chat-session-models/index.ts new file mode 100644 index 00000000..94f3cc8c --- /dev/null +++ b/src/databases/officer_db/src/chat-session-models/index.ts @@ -0,0 +1,7 @@ +export { + setChatSessionModel, + getChatSessionModel, + getChatSessionModels, + seedMissingChatSessionModels, + chatSessionModelsTableExists, +} from './queries'; diff --git a/src/databases/officer_db/src/chat-session-models/queries.ts b/src/databases/officer_db/src/chat-session-models/queries.ts new file mode 100644 index 00000000..6f40cf46 --- /dev/null +++ b/src/databases/officer_db/src/chat-session-models/queries.ts @@ -0,0 +1,78 @@ +import { inArray, sql } from 'drizzle-orm'; +import { db } from '../db'; +import { chatSessionModels } from './schema'; + +/** + * Record the model a session is running as. Upsert — one row per session, last write wins. + * + * Called with the EFFECTIVE model, never the requested one: a tier switch the harness refuses leaves the + * conversation on the old model, and storing what was asked for would put a value here that disagrees with + * the agent that answered. That is the same distinction the per-message attribution makes in `websocket.ts`, + * and it is the reason this takes a plain string rather than reading the client's message itself. + */ +export async function setChatSessionModel(sessionId: string, model: string): Promise { + await db + .insert(chatSessionModels) + .values({ sessionId, model }) + .onConflictDoUpdate({ + target: chatSessionModels.sessionId, + set: { model, updatedAt: new Date() }, + }); +} + +/** One session's stored model, or null when nothing has recorded one yet. */ +export async function getChatSessionModel(sessionId: string): Promise { + const [row] = await db + .select({ model: chatSessionModels.model }) + .from(chatSessionModels) + .where(inArray(chatSessionModels.sessionId, [sessionId])) + .limit(1); + return row?.model ?? null; +} + +/** + * Stored models for a page of sessions, as a Map. + * + * Batched on purpose: the session list renders a model per row, and the per-row alternative is one query + * per session on a screen that routinely shows a hundred of them. Returns an empty Map for an empty input + * rather than issuing `IN ()`, which Postgres rejects. + */ +export async function getChatSessionModels(sessionIds: string[]): Promise> { + if (!sessionIds.length) return new Map(); + const rows = await db + .select({ sessionId: chatSessionModels.sessionId, model: chatSessionModels.model }) + .from(chatSessionModels) + .where(inArray(chatSessionModels.sessionId, sessionIds)); + return new Map(rows.map((r) => [r.sessionId, r.model])); +} + +/** + * Seed models for sessions that have none, and leave every existing row alone. + * + * For the backfill, which infers a FAMILY (`claude-code/opus`) from the transcript because that is all the + * transcript can support. `onConflictDoNothing` is what makes it safe to re-run and safe to run late: a + * session that has since taken a real turn already holds the exact alias the user picked, possibly + * `opus[1m]`, and a family guess must never overwrite it. + * + * Returns how many rows were actually inserted, so the script can say what it did rather than claim + * success for a no-op. + */ +export async function seedMissingChatSessionModels(entries: { sessionId: string; model: string }[]): Promise { + if (!entries.length) return 0; + const rows = await db + .insert(chatSessionModels) + .values(entries) + .onConflictDoNothing({ target: chatSessionModels.sessionId }) + .returning({ sessionId: chatSessionModels.sessionId }); + return rows.length; +} + +/** Whether the table exists yet — the backfill runs from `postdb:push` and must not assume push succeeded. */ +export async function chatSessionModelsTableExists(): Promise { + const rows = await db.execute<{ present: boolean }>( + sql`select to_regclass('chat_session_models') is not null as present`, + ); + const first = + (rows as unknown as { rows?: { present: boolean }[] }).rows?.[0] ?? (rows as never as { present: boolean }[])[0]; + return first?.present === true; +} diff --git a/src/databases/officer_db/src/chat-session-models/schema.ts b/src/databases/officer_db/src/chat-session-models/schema.ts new file mode 100644 index 00000000..e9096350 --- /dev/null +++ b/src/databases/officer_db/src/chat-session-models/schema.ts @@ -0,0 +1,53 @@ +import { pgTable, text, timestamp } from 'drizzle-orm/pg-core'; + +/** + * Which model each chat session is using — the one fact about a session the transcript cannot answer. + * + * ── Why this exists at all ── + * + * Everything else the session list shows (title, cwd, timestamps, message count) is read back out of + * Claude's own transcript on disk. The model is recorded there too — every assistant entry carries a + * `message.model` — but in the WRONG ID SPACE and lossily: + * + * the transcript says Officer offers + * claude-opus-5 claude-code/opus AND claude-code/opus[1m] + * claude-sonnet-5 claude-code/sonnet claude-code/sonnet[1m] + * (whichever answered) claude-code/opusplan + * + * `opus` and `opus[1m]` are indistinguishable after the fact, and `opusplan` records whichever model + * happened to answer that turn. So no mapping can round-trip what the user actually picked — which is + * exactly the field a resumed session needs in order to restore its picker. + * + * The symptom: resume a session and the picker fell back to the first entry in the catalogue. That looked + * fine for as long as the first entry was `opus`, and became visible the day `fable` was added above it. + * + * ── Why a table of its own ── + * + * There is no `chat_sessions` table and deliberately so: a session IS its transcript, and a row that also + * held title and cwd would be a second source of truth for two fields that already have one. This table + * claims only the field the transcript cannot express. + * + * The alternative was to derive it from `chat_session_events`, which does now record `model:switched`. + * Correct data, wrong index: that table holds every assistant message and is unbounded, so asking it for + * the latest model across a list of sessions scans far more than the question deserves. This is the same + * data keyed for the query the list actually runs — one row per session, fetched for a whole page at once. + */ +export const chatSessionModels = pgTable('chat_session_models', { + /** + * The session's transcript uuid — Claude's own id, globally unique, so no user scoping is needed. + * + * The natural key rather than a `serial`: there is exactly one current model per session, and the write + * path is an upsert on every turn. A surrogate id would add a column and a unique index to express the + * constraint the primary key already is. + */ + sessionId: text('session_id').primaryKey(), + /** + * OFFICER's model id (`claude-code/opus[1m]`), not the API's (`claude-opus-5`). + * + * That is the whole point of the table — this is the string the picker offers and the sidecar resolves, + * so it round-trips exactly. Text rather than an enum because the alias set changes with the CLI, and a + * new model must never be a schema migration. + */ + model: text('model').notNull(), + updatedAt: timestamp('updated_at', { withTimezone: true }).notNull().defaultNow(), +}); diff --git a/src/databases/officer_db/src/index.ts b/src/databases/officer_db/src/index.ts index c49d42c9..37df9977 100644 --- a/src/databases/officer_db/src/index.ts +++ b/src/databases/officer_db/src/index.ts @@ -33,6 +33,7 @@ export * from './plugins'; export * from './auth'; export * from './permissions'; export * from './chat-events'; +export * from './chat-session-models'; export * from './dashboards'; export * from './integrations'; export * from './pipeline-jobs'; diff --git a/src/databases/officer_db/src/schema.ts b/src/databases/officer_db/src/schema.ts index 75c31563..807c2881 100644 --- a/src/databases/officer_db/src/schema.ts +++ b/src/databases/officer_db/src/schema.ts @@ -31,6 +31,7 @@ export * from './dashboards/schema'; // dashboards, screens, dashboard_defaults export * from './server/schema'; // server_config (SMTP lives here), server_integrations export * from './pipeline-jobs/schema'; // pipeline_jobs export * from './chat-events/schema'; // chat_session_events +export * from './chat-session-models/schema'; // chat_session_models export * from './agent-panels/schema'; // agent_panels // Core because the tailnet is the perimeter — a security model resting on it cannot treat administering diff --git a/src/servers/api/chat/chat.ts b/src/servers/api/chat/chat.ts index 78e09a59..917200e7 100644 --- a/src/servers/api/chat/chat.ts +++ b/src/servers/api/chat/chat.ts @@ -4,7 +4,7 @@ import { pauseBackgroundTask, restartBackgroundTask } from './task-restart'; import { createRouter } from '../../create-router'; import * as errors from '@@/custom-errors'; import * as sidecar from '@@/sidecar-registry'; -import { getUserSettings } from 'officerdb'; +import { getUserSettings, getChatSessionModel } from 'officerdb'; import { listClaudePwds, listClaudeSessions, @@ -29,6 +29,7 @@ import { listChatModels } from './list-models'; import { logger } from './logger'; import { resolveHomeDir } from '@@/user-home'; import type { ChatIdentity } from './claude-sessions'; +import { officerModelForApiModel } from './model-aliases'; import { readSttConfig } from '../server-settings/stt'; /** @@ -150,8 +151,27 @@ chatRouter.get('/sessions/:id', async (ctx) => { // own, so it gets neither rather than a fabricated answer. const context = isOpenCodeSessionId(id) ? null : claudeSessionContext(who, detail.cwd, id); + /* + The model the PICKER should show, which is not the model `detail` reports. + + `detail.model` is scraped out of the transcript, so it is an API id (`claude-opus-5`) — a different id + space from what the picker offers (`claude-code/opus[1m]`), and lossy: `opus` and `opus[1m]` both write + `claude-opus-5`, and `opusplan` writes whichever model answered. Handing that to the client resolved to + nothing in the catalogue, the picker fell back to the first entry, and a resumed conversation came back + showing the wrong model. + + So the stored row wins where there is one, and the family guess is the fallback for sessions that have + not taken a turn since `chat_session_models` landed. Null when neither can answer — the client shows no + model rather than guessing, which is the whole defect being fixed. + */ + const officerModel = + (await getChatSessionModel(id).catch(() => null)) ?? officerModelForApiModel(detail.model) ?? null; + return ctx.json({ ...detail, + // `detail.model` stays on the payload untouched — it is the honest record of what the API served, and + // the detail view may want it. `officerModel` is the separate question of what to select in the picker. + officerModel, messages, total, offset: windowed ? start : 0, diff --git a/src/servers/api/chat/claude-sessions.ts b/src/servers/api/chat/claude-sessions.ts index d33404fb..469f5dc5 100644 --- a/src/servers/api/chat/claude-sessions.ts +++ b/src/servers/api/chat/claude-sessions.ts @@ -12,6 +12,7 @@ import { type AsUser, } from '../../read-as-user'; import { runAsArgv } from '../../os-user'; +import { isRealModel } from './model-aliases'; // ── Claude session store (source of truth) ── // The `claude` CLI persists every session as a JSONL transcript at @@ -858,6 +859,49 @@ function firstCwd(osUser: AsUser, filePath: string): string { export type ClaudePwd = { cwd: string; sessionCount: number; updatedAt: string; isDefault: boolean }; /** All working directories that have Claude sessions, plus the caller's home. Newest first. */ +/** + * Every transcript this identity owns, with the first real model it recorded. + * + * For the `chat_session_models` backfill (`scripts/backfill-session-models.ts`) — the one caller that wants + * a model per session across the WHOLE tree rather than for one directory. + * + * ── Why the FIRST model is the right one to read ── + * + * Sounds wrong: the table stores the model a session is *currently* using, so the last should win. But + * changing model mid-conversation only became possible on 2026-08-17, so no transcript written before that + * can contain more than one. First and last are the same value for every session this backfill will ever + * see, and reading the first costs a bounded head read instead of a full one — which matters, because the + * transcripts on this machine run to 59MB. + * + * Any session that takes a real turn afterwards overwrites the guess with the exact alias, `[1m]` and all. + * A future re-run of the backfill cannot regress that: `seedMissingChatSessionModels` only inserts. + * + * `` is skipped — it is Claude Code's marker for its own generated entries, not a model. + */ +export function listTranscriptModels(who: ChatIdentity): { sessionId: string; apiModel: string }[] { + const projectsDir = claudeProjectsDir(who.home); + const out: { sessionId: string; apiModel: string }[] = []; + + for (const file of listTranscriptsAs(who.osUser, projectsDir)) { + const path = join(projectsDir, file.slug, `${file.id}.jsonl`); + let head: string; + try { + // 64KB rather than the 32KB `firstCwd` uses: `cwd` is on the very first entry, while the first + // ASSISTANT entry (the only kind that carries a model) sits after the user's opening message, which + // can be long. A miss here costs one absent row, not a wrong one. + head = readHeadAs(who.osUser, path, 65536); + } catch { + continue; // Unreadable transcript. One missing row, and the next turn fills it in. + } + // Scan for the first `"model":"…"` that is not the synthetic marker. Regex rather than JSON.parse per + // line because the head read almost certainly ends mid-line. + const apiModel = [...head.matchAll(/"model":"([^"]+)"/g)].map((m) => m[1]!).find(isRealModel); + if (apiModel) out.push({ sessionId: file.id, apiModel }); + } + + return out; +} + export function listClaudePwds(who: ChatIdentity): ClaudePwd[] { const projectsDir = claudeProjectsDir(who.home); const defaultCwd = who.home; diff --git a/src/servers/api/chat/model-aliases.test.ts b/src/servers/api/chat/model-aliases.test.ts new file mode 100644 index 00000000..58cd6efe --- /dev/null +++ b/src/servers/api/chat/model-aliases.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, test } from 'bun:test'; +import { isRealModel, officerModelForApiModel, SYNTHETIC_MODEL } from './model-aliases'; + +/** + * The five model strings below are not invented — they are every distinct `message.model` found in a real + * 59MB transcript on this machine, in order of first appearance. Two of them are the reason this map is + * needed at all: `` is not a model, and `claude-haiku-4-5-20251001` carries a date suffix the + * others do not, so an exhaustive id list would have missed it. + */ +describe('officerModelForApiModel', () => { + test('resolves the families Officer offers', () => { + expect(officerModelForApiModel('claude-opus-5')).toBe('claude-code/opus'); + expect(officerModelForApiModel('claude-sonnet-5')).toBe('claude-code/sonnet'); + expect(officerModelForApiModel('claude-fable-5')).toBe('claude-code/fable'); + // Date-suffixed, which is why the match is a family prefix rather than a lookup table. + expect(officerModelForApiModel('claude-haiku-4-5-20251001')).toBe('claude-code/haiku'); + }); + + test('a model released tomorrow still resolves to its family', () => { + // The point of prefix matching: a new version must not silently become "unknown". + expect(officerModelForApiModel('claude-opus-6')).toBe('claude-code/opus'); + expect(officerModelForApiModel('claude-sonnet-9-20301231')).toBe('claude-code/sonnet'); + }); + + test('the synthetic marker is not a model', () => { + // Claude Code writes this for entries it generated itself. Storing it would put a value in the picker + // that the CLI would reject. + expect(officerModelForApiModel(SYNTHETIC_MODEL)).toBeNull(); + expect(isRealModel(SYNTHETIC_MODEL)).toBe(false); + expect(isRealModel('claude-opus-5')).toBe(true); + }); + + test('mythos resolves to null rather than to a neighbouring family', () => { + // Project Glasswing only, so it has no Officer entry. A picker option that 401s is worse than none. + expect(officerModelForApiModel('claude-mythos-5')).toBeNull(); + expect(officerModelForApiModel('claude-mythos-preview')).toBeNull(); + }); + + test('null for anything it cannot honestly name', () => { + for (const input of [null, undefined, '', ' ', 'gpt-5', 'anthropic/claude-3-5', 'opus']) { + expect(officerModelForApiModel(input)).toBeNull(); + } + }); + + test('the lossy direction is documented by the map itself', () => { + // `opus` and `opus[1m]` both serve claude-opus-5, so this can only ever return the bare family — which + // is exactly why a real turn must overwrite the guess. If someone "improves" this to return a [1m] + // variant, they have invented information the transcript does not contain. + expect(officerModelForApiModel('claude-opus-5')).not.toContain('[1m]'); + expect(officerModelForApiModel('claude-sonnet-5')).not.toContain('[1m]'); + }); +}); diff --git a/src/servers/api/chat/model-aliases.ts b/src/servers/api/chat/model-aliases.ts new file mode 100644 index 00000000..26f59cb8 --- /dev/null +++ b/src/servers/api/chat/model-aliases.ts @@ -0,0 +1,49 @@ +/** + * Anthropic API model id → the Officer id that most nearly names it. + * + * Two id spaces meet here. A transcript records what the API served (`claude-opus-5`); Officer's picker + * offers what the CLI accepts (`claude-code/opus`, `claude-code/opus[1m]`). This translates one to the + * other, and it is **deliberately lossy in one direction only** — that is the whole reason + * `chat_session_models` exists: + * + * - `opus` and `opus[1m]` both serve `claude-opus-5`, so the 1M variant cannot be recovered. + * - `opusplan` serves whichever of opus/sonnet answered that turn, so it cannot be recognised at all. + * - `mythos` has no Officer entry (Project Glasswing only), so it resolves to null rather than to a + * neighbouring family — offering a model this machine may not reach is worse than offering none. + * + * So this is the FALLBACK for a session that has not taken a turn since the table landed, and the seed for + * the backfill. A real turn overwrites its guess with the exact alias the user picked. Never use it to + * decide what to run — only to decide what to display. + * + * Matching is by FAMILY PREFIX rather than an exhaustive id list, because the API ids are not uniform: + * `claude-haiku-4-5-20251001` carries a date suffix while `claude-fable-5` does not, and new versions ship + * without warning. A prefix match keeps a model released tomorrow resolving to the right family instead of + * to null. + */ + +/** Claude Code's own marker for an entry it generated itself. Not a model, and must never be stored. */ +export const SYNTHETIC_MODEL = ''; + +const FAMILIES: { prefix: string; officerId: string }[] = [ + { prefix: 'claude-fable', officerId: 'claude-code/fable' }, + { prefix: 'claude-opus', officerId: 'claude-code/opus' }, + { prefix: 'claude-sonnet', officerId: 'claude-code/sonnet' }, + { prefix: 'claude-haiku', officerId: 'claude-code/haiku' }, +]; + +/** + * The Officer model id for an API model id, or null when there is no honest answer. + * + * Null for ``, for mythos, for an OpenCode model, and for anything unrecognised. Callers treat + * null as "we do not know", which renders as no model rather than as a guess. + */ +export function officerModelForApiModel(apiModel: string | null | undefined): string | null { + if (!apiModel || apiModel === SYNTHETIC_MODEL) return null; + const id = apiModel.trim().toLowerCase(); + if (!id) return null; + const family = FAMILIES.find((f) => id.startsWith(f.prefix)); + return family?.officerId ?? null; +} + +/** Whether a transcript entry's model is a real model rather than Claude Code's synthetic marker. */ +export const isRealModel = (apiModel: string | null | undefined): boolean => !!apiModel && apiModel !== SYNTHETIC_MODEL; diff --git a/src/servers/api/chat/websocket.ts b/src/servers/api/chat/websocket.ts index d169a9e8..9ede0f63 100644 --- a/src/servers/api/chat/websocket.ts +++ b/src/servers/api/chat/websocket.ts @@ -17,7 +17,14 @@ import * as sidecar from '@@/sidecar-registry'; import { join } from 'path'; import { getOwnerHomeDir, getEmailAccountsDir } from '../../../servers/data-path'; import { attachedFilePaths } from './attached-files'; -import { getUserSettings, getEmailAccounts, getChatEventsSince, appendChatEvent, getUserById } from 'officerdb'; +import { + getUserSettings, + getEmailAccounts, + getChatEventsSince, + appendChatEvent, + getUserById, + setChatSessionModel, +} from 'officerdb'; import { resolveHomeDir } from '@@/user-home'; import { claudeLoginState } from '@@/os-user-claude'; import { mkdirSync } from 'node:fs'; @@ -324,6 +331,35 @@ function createMessageHandler(sessionId: string, model: string) { */ let effective = model; + /* + Claude's transcript uuid, which is NOT `sessionId`. + + `sessionId` is officer's handle; the session LIST is keyed by the transcript's own uuid, and the two are + different strings (see the `session:claude` docs in types.ts). So the durable model record has to be + keyed by the transcript id or it will never join to a list row — and the transcript id is not known when + the turn starts, only when the harness announces it. That is what this waits for. + */ + let transcriptId: string | null = null; + + /** + * Write the model down, once both halves are known. + * + * Fire-and-forget with a swallowed failure: a chat turn must not die because a bookkeeping row would not + * write. The cost of losing it is that the picker falls back to a family guess on the next resume, which + * is the behaviour every session already had before this table existed. + */ + const record = (): void => { + if (!transcriptId) return; + void setChatSessionModel(transcriptId, effective).catch((err: unknown) => { + logger.warn('Could not record the session model', { + sessionId, + transcriptId, + model: effective, + error: err instanceof Error ? err.message : String(err), + }); + }); + }; + return (msg: TurnMessage, seq?: number): void => { const session = sessionManager.getSession(sessionId); if (!session) return; @@ -335,6 +371,19 @@ function createMessageHandler(sessionId: string, model: string) { // and the orphan adopter all route on `session.model`, and a stale one sends a stop to the wrong // sidecar's kill path. sessionManager.setModel(sessionId, effective); + record(); + } + + // The transcript id arrives at the START of a turn, and again on the closing `result` for a session + // whose id was assigned mid-turn. Recording on both means a brand-new conversation gets its model + // written on its very first turn rather than on its second. + if (msg.type === 'session:claude' && msg.claudeSessionId) { + transcriptId = msg.claudeSessionId; + record(); + } + if (msg.type === 'result' && msg.claudeSessionId && msg.claudeSessionId !== transcriptId) { + transcriptId = msg.claudeSessionId; + record(); } foldIntoSession(session, msg, effective); diff --git a/src/workspaces/officerdev/src/apps/Chat/components/ModelSelector.test.tsx b/src/workspaces/officerdev/src/apps/Chat/components/ModelSelector.test.tsx index a32164d6..05e30a44 100644 --- a/src/workspaces/officerdev/src/apps/Chat/components/ModelSelector.test.tsx +++ b/src/workspaces/officerdev/src/apps/Chat/components/ModelSelector.test.tsx @@ -79,3 +79,22 @@ describe('ModelSelector after a conversation has started', () => { expect(providerButton(/claude code/i).hasAttribute('disabled')).toBe(false); }); }); + +describe('ModelSelector default selection', () => { + test('an unstarted composer gets a default', () => { + const picked: string[] = []; + mount({ hasStarted: false, selectedModel: null, model: null, onModelChange: (id) => picked.push(id) }); + expect(picked).toEqual(['claude-code/opus']); + }); + + test('a started conversation with an unresolved model is left alone', () => { + /* + The regression this pins. A resumed session whose model the server could not name used to be + reassigned to whichever model was listed first — invisible while that was `opus`, and the reason every + old conversation came back as `fable` once fable went above it. An unset picker is the honest state. + */ + const picked: string[] = []; + mount({ hasStarted: true, selectedModel: null, model: null, onModelChange: (id) => picked.push(id) }); + expect(picked).toEqual([]); + }); +}); diff --git a/src/workspaces/officerdev/src/apps/Chat/components/ModelSelector.tsx b/src/workspaces/officerdev/src/apps/Chat/components/ModelSelector.tsx index 46566064..d7fa2fa4 100644 --- a/src/workspaces/officerdev/src/apps/Chat/components/ModelSelector.tsx +++ b/src/workspaces/officerdev/src/apps/Chat/components/ModelSelector.tsx @@ -64,12 +64,25 @@ export function ModelSelector({ const providerModels = availableModels.filter((m) => m.provider === activeProvider); const fallbackModelId = providerModels[0]?.id ?? null; - // Sync actual selection when UI shows a fallback provider but nothing is selected + /* + Pick a default only for a chat that has not started. + + This used to fire whenever nothing was selected, which meant a RESUMED conversation whose model had not + resolved got silently reassigned to `providerModels[0]` — the first entry in the catalogue. It read as + correct for as long as that entry was `opus`, and the day `fable` was added above it every resumed + session came back claiming fable. + + `hasStarted` is the gate because it is exactly the distinction that matters: an unstarted composer has + no model and needs one, while a conversation in progress already has one and this component is the last + thing that should be inventing it. When the server cannot say what a session used, the honest outcome is + an unset picker — not a confident wrong answer. + */ useEffect(() => { + if (hasStarted) return; if (!selectedModel && !model && fallbackModelId) { onModelChange(fallbackModelId); } - }, [selectedModel, model, fallbackModelId]); + }, [hasStarted, selectedModel, model, fallbackModelId]); /* Two different questions, which this used to answer with one flag. diff --git a/src/workspaces/state/src/useClaudeSessions.ts b/src/workspaces/state/src/useClaudeSessions.ts index 55f2daf3..1110eb36 100644 --- a/src/workspaces/state/src/useClaudeSessions.ts +++ b/src/workspaces/state/src/useClaudeSessions.ts @@ -50,7 +50,23 @@ export type ClaudeSessionMessage = export type ClaudeSessionDetail = { id: string; + /** + * What the API actually served, straight out of the transcript — `claude-opus-5`. + * + * NOT what the model picker offers. Do not feed this to the selector; use `officerModel`. + */ model: string; + /** + * The same session in OFFICER's id space — `claude-code/opus[1m]` — or null when unknown. + * + * Resolved server-side from the session's stored model, falling back to a family guess off `model`. The + * two exist separately because the mapping is one-way: `opus` and `opus[1m]` both serve `claude-opus-5`, + * so `model` cannot answer "what should the picker show" and this cannot answer "what ran". + * + * Null means unknown, and the picker must leave its selection alone rather than fall back to the first + * catalogue entry — which is the bug this field exists to fix. + */ + officerModel: string | null; cwd: string; messages: ClaudeSessionMessage[]; total: number; // full transcript length (the loaded messages may be a tail window)