diff --git a/src/servers/api/chat/session-manager.ts b/src/servers/api/chat/session-manager.ts index fcde0940..66d96ae1 100644 --- a/src/servers/api/chat/session-manager.ts +++ b/src/servers/api/chat/session-manager.ts @@ -65,6 +65,25 @@ class SessionManager { return this.sessions.get(sessionId) || null; } + /** + * Record that a live session moved onto a different model. + * + * `getOrCreate` takes a `model` and, for a session that already exists, silently discards it — which was + * correct while the model could not change, and became a lie the moment it could. Everything that routes + * or attributes by `session.model` reads this: `handleStop`, `endTurnIfAgentIsGone`, the orphan adopter, + * and the cost fold. Leaving it stale meant a stop landing on the wrong harness's kill path. + * + * Called only AFTER the harness confirms the switch. A refused switch must leave this alone, or the + * server's record and the running agent disagree in the direction that is hardest to notice. + */ + setModel(sessionId: string, model: string): void { + const session = this.sessions.get(sessionId); + if (!session || session.model === model) return; + session.model = model; + session.meta.model = model; + session.meta.updatedAt = Date.now(); + } + getUserSessions(email: string): UserSession[] { const sessionIds = this.userSessions.get(email) || []; return sessionIds.map((id) => this.sessions.get(id)).filter((s): s is UserSession => s !== undefined); diff --git a/src/servers/api/chat/types.ts b/src/servers/api/chat/types.ts index d49ddc05..15787cc0 100644 --- a/src/servers/api/chat/types.ts +++ b/src/servers/api/chat/types.ts @@ -234,7 +234,19 @@ export type ServerMessage = // these the stream simply stops, which is indistinguishable from a hung turn, a dead socket or a // crashed agent; the honest response to that is to restart the server, which is what actually happened. | { type: 'compact:start'; trigger: 'manual' | 'auto' } - | { type: 'compact:done'; trigger: 'manual' | 'auto'; preTokens: number; durationMs?: number }; + | { type: 'compact:done'; trigger: 'manual' | 'auto'; preTokens: number; durationMs?: number } + /* + The conversation changed model, mid-conversation. + + Durable, and that is the point rather than a nicety: without a record in the log, history renders every + message of a switched session under whichever tier it currently reports, so a transcript half-answered + by opus reads as entirely sonnet. `claude-sessions.ts` reads the FIRST model it sees in a transcript, so + it cannot see a switch at all. + + `ok: false` is a switch the harness refused. It is not an error message — the turn still ran and still + answered, as `from` — so it says what actually happened rather than what was asked for. + */ + | { type: 'model:switched'; from: string | null; to: string; ok: boolean; error?: string }; // The turn-output subset of ServerMessage — everything the agent sidecar produces on its own. The // remaining members (session:init, sync:messages, disconnected, connection-level errors) are officer's: @@ -256,7 +268,8 @@ export type TurnMessageType = | 'task:started' | 'task:notification' | 'compact:start' - | 'compact:done'; + | 'compact:done' + | 'model:switched'; export type TurnMessage = Extract & { prevSeq?: number }; @@ -290,7 +303,9 @@ export type ChatEvent = // Compaction, start and end. The start half has no message in the harness's output stream at all — it // comes from the `PreCompact` hook, which is why it is a sidecar concern and not the parser's. | { type: 'compact:start'; trigger: 'manual' | 'auto' } - | { type: 'compact:done'; trigger: 'manual' | 'auto'; preTokens: number; durationMs?: number }; + | { type: 'compact:done'; trigger: 'manual' | 'auto'; preTokens: number; durationMs?: number } + // Emitted by `applyModelChange` when a live session moves onto a different tier — see claude-manager. + | { type: 'model:switched'; from: string | null; to: string; ok: boolean; error?: string }; export type UserSession = { sessionId: string; diff --git a/src/servers/api/chat/websocket.ts b/src/servers/api/chat/websocket.ts index 5f63d02e..d169a9e8 100644 --- a/src/servers/api/chat/websocket.ts +++ b/src/servers/api/chat/websocket.ts @@ -312,10 +312,32 @@ function foldIntoSession(session: UserSession, msg: TurnMessage, model: string): // Shared by both harnesses now: whichever sidecar ran the turn has already translated and committed it. function createMessageHandler(sessionId: string, model: string) { + /* + The model messages are ATTRIBUTED to, which is not always the model the turn asked for. + + `foldIntoSession` stamps every assistant message with this, so it has to track what the harness actually + did rather than what the client requested. A tier switch is a control request on a live session and can + be refused; when it is, the turn still runs and still answers — as the OLD model — and stamping it with + the new one would put a lie in the transcript that nothing downstream could detect. + + Mutable for that reason, and updated only from the harness's own `model:switched`. + */ + let effective = model; + return (msg: TurnMessage, seq?: number): void => { const session = sessionManager.getSession(sessionId); if (!session) return; - foldIntoSession(session, msg, model); + + if (msg.type === 'model:switched') { + // `to` on success, `from` on refusal — either way, what is answering from here on. + effective = msg.ok ? msg.to : (msg.from ?? effective); + // The server's record follows the harness, never the request: `handleStop`, `endTurnIfAgentIsGone` + // 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); + } + + foldIntoSession(session, msg, effective); for (const socket of session.sockets) sendToClient(socket as ServerWebSocket, msg, seq); }; } @@ -413,7 +435,33 @@ async function handleChat( : withSummary; // Use provided model, or fall back to user default, or the system default. - const model = msg.model || (await getUserDefaultModel(userId)) || DEFAULT_MODEL; + const requested = msg.model || (await getUserDefaultModel(userId)) || DEFAULT_MODEL; + + /* + A conversation may change TIER mid-flight; it may not change HARNESS. + + Tier is a control request on the live session (`setModel`), so opus → sonnet costs nothing and keeps the + transcript. Harness is a different sidecar with a different session id space, and crossing it is what + actually broke: an OpenCode `ses_…` id handed to `claude --resume`, which reads as a dead agent rather + than as a wrong dispatch. Nothing refused it before — the UI simply locked the whole selector after the + first message, which also blocked the harmless half. + + The server's own record wins here, the same precedence `decideResume` uses: a session in memory already + knows its harness, and a socket does not get to re-declare it. + */ + const live = sessionManager.getSession(sessionId); + let model = requested; + if (live && isClaudeModel(live.model) !== isClaudeModel(requested)) { + model = live.model; + logger.warn('Refused a harness change mid-session', { sessionId, requested, keeping: live.model }); + sendToClient(ws, { + type: 'model:switched', + from: live.model, + to: requested, + ok: false, + error: 'A conversation cannot move between Claude Code and OpenCode. Start a new chat for that.', + }); + } logger.info('Model selected for chat', { sessionId, model, clientModel: msg.model || null }); diff --git a/src/servers/sidecar/claude/claude-manager.ts b/src/servers/sidecar/claude/claude-manager.ts index b3842bc0..0e238fe6 100644 --- a/src/servers/sidecar/claude/claude-manager.ts +++ b/src/servers/sidecar/claude/claude-manager.ts @@ -220,6 +220,16 @@ type PersistentSession = { interrupted: boolean; /** What we passed as `--resume`, so a failure can name it and decide whether it was to blame. */ resumeId?: string; + /** + * The model this session is currently answering as, `harness/tier`. + * + * Recorded because `model` reaches the SDK exactly once, in `createSession`'s `query()` options, while + * every later turn is pushed onto that same call — so without this there is nothing to compare a turn's + * requested model against, and a change was silently dropped. Officer's UI, its cost folding and its + * durable log all took the client's word for it, so a switch produced a session that reported one tier + * and answered as another. + */ + model?: string; /** The tail of the child's stderr, so its exit can carry a reason instead of just a number. */ stderrTail: string; /** Whether the child ever produced a single message. False plus a non-zero exit means it never started. */ @@ -468,6 +478,7 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat const resumeId = getClaudeSession(sessionKey, params.userId) ?? params.resumeSessionId; const subModel = params.model?.split('/')[1]; session.resumeId = resumeId; + session.model = params.model; /** * What we are about to run, before we run it. @@ -662,12 +673,48 @@ export async function spawnClaudeStreaming( } if (session) { session.emit = onEvent; // adopt the latest emitter (equivalent across turns; keeps events flowing) + await applyModelChange(session, params.model); } else { session = createSession(params, onEvent); } session.pushTurn(params.prompt, params.images); } +/** + * Move a LIVE session onto a different model, the way the CLI's `/model` does. + * + * `model` is a `query()` option and options are read once, so before this a changed model was dropped on + * the floor — the session kept answering as whatever it started as while Officer's UI, its cost folding and + * its durable log all reported the new tier. The mismatch was invisible: no log line, no error, and the one + * way to trigger it was to leave a chat idle past the 30-minute GC. + * + * `setModel` is a control request, so it needs streaming input mode — which is the only mode this file uses. + * That is why nothing has to be torn down and re-resumed: the conversation, its cwd, its permission mode and + * its transcript all continue, and only the next response comes from a different model. + * + * A refusal is reported and swallowed. The turn the user just typed is worth more than the tier they picked, + * so a `setModel` the harness rejects must not lose the prompt — it answers as the old model and says so. + */ +async function applyModelChange(session: PersistentSession, model: string | undefined): Promise { + if (!model || model === session.model) return; + + const previous = session.model; + const subModel = model.split('/')[1]; + if (!subModel) return; + + try { + await session.query.setModel(subModel); + session.model = model; + console.log(`[claude:model:${session.sessionKey}] ${previous ?? 'default'} -> ${model}`); + session.emit({ type: 'model:switched', from: previous ?? null, to: model, ok: true }); + } catch (err) { + // Leave `session.model` alone, so the next turn tries again rather than believing a switch that failed. + const reason = err instanceof Error ? err.message : String(err); + console.error(`[claude:model:${session.sessionKey}] refused ${previous ?? 'default'} -> ${model}: ${reason}`); + session.emit({ type: 'model:switched', from: previous ?? null, to: model, ok: false, error: reason }); + } +} + /** Interrupt the current turn but KEEP the session alive (the "stop" button). */ export async function interruptClaudeSession(sessionKey: string, userId: number): Promise { const session = ownedSession(sessionKey, userId); diff --git a/src/servers/sidecar/claude/turn-stream.test.ts b/src/servers/sidecar/claude/turn-stream.test.ts index a255d36c..0efa2188 100644 --- a/src/servers/sidecar/claude/turn-stream.test.ts +++ b/src/servers/sidecar/claude/turn-stream.test.ts @@ -170,6 +170,33 @@ describe('createTurnStream', () => { ]); }); + // A tier switch has to survive a reload, and it has to land in the right PLACE. The marker says "opus + // wrote everything above this line", which is false if it is emitted before the last thing opus said is + // flushed out of the delta buffer. + test('a model switch is durable and lands after the previous model’s last words', () => { + const { durable } = run([ + { type: 'delta', text: 'the old model speaking' }, + { type: 'model:switched', from: 'claude-code/opus', to: 'claude-code/sonnet', ok: true }, + { type: 'text', text: 'the new one' }, + ]); + expect(durable).toEqual([ + { type: 'assistant:text', text: 'the old model speaking' }, + { type: 'model:switched', from: 'claude-code/opus', to: 'claude-code/sonnet', ok: true }, + { type: 'assistant:text', text: 'the new one' }, + ]); + }); + + test('a refused switch is recorded too, with its reason', () => { + // Silence here would be the worst outcome: the turn answers as the OLD model while the picker shows the + // new one, which is the exact mismatch this whole change exists to remove. + const { durable } = run([ + { type: 'model:switched', from: 'claude-code/opus', to: 'claude-code/haiku', ok: false, error: 'nope' }, + ]); + expect(durable).toEqual([ + { type: 'model:switched', from: 'claude-code/opus', to: 'claude-code/haiku', ok: false, error: 'nope' }, + ]); + }); + test('streams are independent', () => { const a = createTurnStream('a'); const b = createTurnStream('b'); diff --git a/src/servers/sidecar/claude/turn-stream.ts b/src/servers/sidecar/claude/turn-stream.ts index 583f53e0..58fc7621 100644 --- a/src/servers/sidecar/claude/turn-stream.ts +++ b/src/servers/sidecar/claude/turn-stream.ts @@ -133,6 +133,23 @@ export function createTurnStream(sessionId: string): TurnStream { // notice belongs after it rather than in the middle of a paragraph it interrupted. return [...flushAll(), { msg: { type: 'compact:start', trigger: event.trigger }, durable: true }]; + case 'model:switched': + // Flushed for the same reason, and it matters more here: the marker's whole job is to say where in + // the conversation the tier changed, so it must land after the last thing the old model said. + return [ + ...flushAll(), + { + msg: { + type: 'model:switched', + from: event.from, + to: event.to, + ok: event.ok, + ...(event.error ? { error: event.error } : {}), + }, + durable: true, + }, + ]; + case 'compact:done': return [ { diff --git a/src/workspaces/officerdev/src/apps/Chat/components/MessageBubble.tsx b/src/workspaces/officerdev/src/apps/Chat/components/MessageBubble.tsx index 2654b437..67ebdac4 100644 --- a/src/workspaces/officerdev/src/apps/Chat/components/MessageBubble.tsx +++ b/src/workspaces/officerdev/src/apps/Chat/components/MessageBubble.tsx @@ -15,6 +15,7 @@ import { PlugZap, RotateCcw, FoldVertical, + ArrowLeftRight, } from 'lucide-react'; import { toast } from '@/components/ui/sonner'; import type { Tone } from '@/components/Data'; @@ -211,6 +212,9 @@ export const MessageBubble = ({ message, onAnswer, onRetry, defaultOpen }: Messa case 'compact': return ; + case 'model': + return ; + case 'interrupted': return (
@@ -340,6 +344,44 @@ const CompactActivity = ({ message }: { message: Extract }) => { + // `claude-code/opus` → `opus`. The harness is pinned for the life of a conversation, so repeating it on + // every marker is noise; the tier is the part that changed. + const tier = (id: string | null) => id?.split('/').pop() ?? 'default'; + + return ( +
+
+ + + {message.ok ? ( + <> + {tier(message.from)} → {tier(message.to)} + + ) : ( + <> + Kept {tier(message.from)} + + · {message.error ?? `could not switch to ${tier(message.to)}`} + + + )} + +
+
+ ); +}; + /** * The wait between pressing send and the first token — tens of seconds with thinking enabled — used to * render nothing at all, because the streaming bubble bailed on empty text. The transcript looked frozen diff --git a/src/workspaces/officerdev/src/apps/Chat/components/ModelSelector.test.tsx b/src/workspaces/officerdev/src/apps/Chat/components/ModelSelector.test.tsx new file mode 100644 index 00000000..a32164d6 --- /dev/null +++ b/src/workspaces/officerdev/src/apps/Chat/components/ModelSelector.test.tsx @@ -0,0 +1,81 @@ +import { describe, expect, test } from 'bun:test'; +import { render, screen } from '@testing-library/react'; +import type { ModelOption } from 'state/useModels'; +import { ModelSelector } from './ModelSelector'; + +/** + * Tier is changeable mid-conversation; harness is not. + * + * The selector used to lock entirely on `hasStarted`, which blocked both. Nothing justified it — the commit + * that added the flag says only "default model" — and the hazard it resembles is narrower than what it + * stopped: crossing harnesses hands an OpenCode `ses_…` id to `claude --resume`, which surfaces as a dead + * agent rather than a wrong dispatch. Changing tier is a control request on the live session and always was + * safe. + * + * These two pull in opposite directions, so a test for either alone passes with the other broken: unlock + * everything and the harness test fails; restore the old blanket lock and the tier test fails. + */ +const MODELS: ModelOption[] = [ + { id: 'claude-code/opus', name: 'opus', provider: 'claude-code' }, + { id: 'claude-code/sonnet', name: 'sonnet', provider: 'claude-code' }, + // Two OpenCode providers, so the "other harness" case has more than one thing to choose between — + // otherwise the strip collapses for want of options and the test would pass for the wrong reason. + { id: 'anthropic/claude-3-5', name: 'claude-3-5', provider: 'anthropic' }, + { id: 'openai/gpt-5', name: 'gpt-5', provider: 'openai' }, +] as ModelOption[]; + +const mount = (over: Partial[0]> = {}) => + render( + {}} + model={null} + isConnected + isGenerating={false} + hasStarted + {...over} + />, + ); + +const providerButton = (name: RegExp) => screen.getByRole('button', { name }); + +describe('ModelSelector after a conversation has started', () => { + test('the tier select is enabled', () => { + mount(); + // The ` !isLocked && onModelChange(v)} - disabled={isLocked} + onValueChange={(v) => !isBusy && onModelChange(v)} + disabled={isBusy} > diff --git a/src/workspaces/officerdev/src/apps/Chat/components/turn-rows.ts b/src/workspaces/officerdev/src/apps/Chat/components/turn-rows.ts index 2e0448ad..35b39e3b 100644 --- a/src/workspaces/officerdev/src/apps/Chat/components/turn-rows.ts +++ b/src/workspaces/officerdev/src/apps/Chat/components/turn-rows.ts @@ -18,7 +18,9 @@ import type { ChatMessage } from '../types'; /** Rows that are landmarks about the conversation rather than work done inside a turn. These never fold. */ function isStructural(message: ChatMessage): boolean { - return message.role === 'divider' || message.role === 'compact'; + // `model` belongs here for the same reason `compact` does: it is the answer to "which half of this + // conversation did opus write", which is worthless folded away inside a turn summary. + return message.role === 'divider' || message.role === 'compact' || message.role === 'model'; } /** The work of a turn: everything that belongs behind the summary row. */ diff --git a/src/workspaces/officerdev/src/apps/Chat/types.ts b/src/workspaces/officerdev/src/apps/Chat/types.ts index a5d690b6..69c554a3 100644 --- a/src/workspaces/officerdev/src/apps/Chat/types.ts +++ b/src/workspaces/officerdev/src/apps/Chat/types.ts @@ -88,6 +88,22 @@ export type ChatMessage = preTokens?: number; durationMs?: number; done?: boolean; + } + | { + // Where in the conversation the model changed. + // + // A marker rather than a property of the messages around it, because the interesting thing is the + // BOUNDARY: which half of this conversation is opus and which is sonnet. Per-message model is already + // stamped server-side and shown nowhere, so without a row in the transcript a switched session reads + // as though one model wrote all of it. + // + // `ok: false` is a switch that was refused — by the harness, or by the server when it would have + // crossed harnesses. The turn still ran, as `from`, so the row says that rather than showing an error. + role: 'model'; + from: string | null; + to: string; + ok: boolean; + error?: string; }; export type RunningTask = { taskId: string; description: string; taskType?: string }; @@ -149,7 +165,9 @@ export type ServerMessage = | { type: 'task:notification'; taskId: string; status: 'completed' | 'failed' | 'stopped'; summary: string } /** Context compaction beginning and ending. See the `compact` message role. */ | { type: 'compact:start'; trigger: 'manual' | 'auto' } - | { type: 'compact:done'; trigger: 'manual' | 'auto'; preTokens: number; durationMs?: number }; + | { type: 'compact:done'; trigger: 'manual' | 'auto'; preTokens: number; durationMs?: number } + /** The conversation moved onto a different model. See the `model` message role. */ + | { type: 'model:switched'; from: string | null; to: string; ok: boolean; error?: string }; export type Message = { id: string; diff --git a/src/workspaces/officerdev/src/hooks/useChat.ts b/src/workspaces/officerdev/src/hooks/useChat.ts index 4d9a6cd2..9599750e 100644 --- a/src/workspaces/officerdev/src/hooks/useChat.ts +++ b/src/workspaces/officerdev/src/hooks/useChat.ts @@ -467,6 +467,22 @@ export function useChat(initialSessionId?: string, initialModel?: string | null, setMessages((prev) => [...prev, { role: 'compact', trigger: msg.trigger }]); break; + case 'model:switched': + // Committed for the same reason and it matters more: the marker's job is to say where the tier + // changed, so anything the previous model had already streamed has to land above it. + commitStreaming(); + setMessages((prev) => [ + ...prev, + { role: 'model', from: msg.from, to: msg.to, ok: msg.ok, ...(msg.error ? { error: msg.error } : {}) }, + ]); + // A refusal leaves the conversation on the old model, so the picker has to go back — otherwise it + // keeps showing a tier that is not answering, and every later turn re-requests it. + if (!msg.ok && msg.from) { + setSelectedModel(msg.from); + modelRef.current = msg.from; + } + break; + case 'compact:done': setMessages((prev) => { const at = prev.findLastIndex((m) => m.role === 'compact' && !m.done);