From f1eb652d61699d917d4fc98be9f69a731ff23252 Mon Sep 17 00:00:00 2001 From: Rohit C Prasad Date: Wed, 22 Jul 2026 15:43:54 -0700 Subject: [PATCH] Allow mid-session model switching with a persisted transcript marker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Picker stays live for the session; switches persist a model_switch notice (§17 revised). Rebinds refused mid-turn; images become placeholders for non-vision targets at send time. --- coworker/engine.py | 72 ++++++++++++++++++++++ coworker/server/app.py | 38 +++++++----- surfaces/gui/e2e/chat.spec.ts | 6 +- surfaces/gui/e2e/fixtures.ts | 10 +++ surfaces/gui/e2e/model-switch.spec.ts | 33 ++++++++++ surfaces/gui/e2e/session-shell.spec.ts | 5 +- surfaces/gui/src/App.tsx | 8 ++- surfaces/gui/src/components/Composer.tsx | 29 ++++----- surfaces/gui/src/itemsFromMessages.test.ts | 14 +++++ surfaces/gui/src/itemsFromMessages.ts | 6 +- surfaces/gui/src/types.ts | 1 + tests/test_engine.py | 58 +++++++++++++++++ tests/test_server.py | 37 ++++++----- 13 files changed, 261 insertions(+), 56 deletions(-) create mode 100644 surfaces/gui/e2e/model-switch.spec.ts diff --git a/coworker/engine.py b/coworker/engine.py index 5b002c83..7e0530f9 100644 --- a/coworker/engine.py +++ b/coworker/engine.py @@ -177,6 +177,43 @@ class TurnEngine: async for event in self._loop(): yield event + def switch_model(self, model: str) -> Optional[str]: + """Rebind the session's model mid-conversation (roadmap item 3). History is + canonical OpenAI shape and every provider converts per call, so the switch is just + the field write — plus a persisted notice marking WHERE it happened, with a + degradation warning when history carries images the new model can't see (those are + sent as placeholders — see `_outbound_messages`). Returns the notice text, or None + when nothing changed (same model, or first bind on a fresh session).""" + if not model or model == self.model: + return None + had_history = any(m.get("role") != "system" for m in self.messages) + self.model = model + if not had_history: + return None + from .providers.matrix import model_labels + + text = f"Model switched to {model_labels().get(model, model)}" + try: + caps = self.provider.capabilities(model) + except Exception: + caps = None + if ( + caps is not None + and not getattr(caps, "vision", False) + and self._history_has_images() + ): + text += " — earlier images can't be read by this model" + self._append_notice("model_switch", text) + return text + + def _history_has_images(self) -> bool: + return any( + isinstance(p, dict) and p.get("type") == "image_url" + for msg in self.messages + if isinstance(msg.get("content"), list) + for p in msg["content"] + ) + def _append_notice(self, kind: str, text: Optional[str] = None) -> None: """Persist a turn-ending marker (error/interrupted) as a display-only `notice` message: it survives reload like the transcript does, but `_outbound_messages` @@ -868,6 +905,41 @@ class TurnEngine: for msg in out ] + # Images get the same per-turn treatment: a model without vision receives a visible + # placeholder instead of a payload it would reject. Like the PDF path, this re-decides + # per call, so a mid-session switch to/from a vision model always does the right thing. + if any( + isinstance(p, dict) and p.get("type") == "image_url" + for msg in out + if isinstance(msg.get("content"), list) + for p in msg["content"] + ): + caps = self.provider.capabilities(self.model) + if not getattr(caps, "vision", False): + placeholder = { + "type": "text", + "text": "[image attachment — not viewable by this model]", + } + out = [ + ( + { + **msg, + "content": [ + ( + placeholder + if isinstance(p, dict) + and p.get("type") == "image_url" + else p + ) + for p in msg["content"] + ], + } + if isinstance(msg.get("content"), list) + else msg + ) + for msg in out + ] + context = ( self.context_provider() if self.context_provider is not None else "" ) or "" diff --git a/coworker/server/app.py b/coworker/server/app.py index 3908ccc3..e4322739 100644 --- a/coworker/server/app.py +++ b/coworker/server/app.py @@ -1508,11 +1508,23 @@ def create_app(manager: SessionManager) -> FastAPI: } return {"approved": True, "mode": resp.get("mode") or "interactive"} - def _model_locked() -> bool: - # The model is chosen until the first real turn, then fixed for the session's life - # (system message doesn't count as history). Enforced HERE, not just in the GUI, - # so API callers and message races can't rebind a running conversation. - return any(m.get("role") != "system" for m in engine.messages) + async def _apply_model(model: Optional[str]) -> None: + # Mid-session rebind is allowed (roadmap item 3, supersedes the 2026-07-04 + # lock): history is canonical and providers convert per call. A real switch + # appends a persisted notice; broadcast it so live views render the marker + # and update their header. Never rebind mid-turn — the running loop reads + # `engine.model` per iteration and a mixed turn is exactly the breakage the + # old lock existed to prevent. + if not model or manager.is_running(session_id): + return + notice = engine.switch_model(model) + if notice is None: # same model, or first bind on a fresh session + return + manager.persist_session(session_id) + await manager.broadcast_session( + session_id, + {"type": "model_changed", "data": {"model": model, "text": notice}}, + ) def _resolve_pending(resolution: str) -> None: # Live WS responses resolve THE session's single pending prompt (one at a time, since the @@ -1641,20 +1653,14 @@ def create_app(manager: SessionManager) -> FastAPI: except ValueError: pass elif kind == "set_model": - model = message.get("model") - if model and not _model_locked(): - engine.model = model + await _apply_model(message.get("model")) elif kind == "user_message": text = (message.get("text") or "").strip() attachments = message.get("attachments") or [] - # The composer sends its visible model with every message — the FIRST one - # binds the session's model (race-proof across reconnects; see api.ts - # Session.userMessage). After that the model is FIXED for the session's - # life (owner call, 2026-07-04): mixed-model transcripts invite - # provider-quirk breakage. Start a new session to switch. - model = message.get("model") - if model and not _model_locked(): - engine.model = model + # The composer sends its visible model with every message — the FIRST + # one binds the session (race-proof across reconnects; see api.ts + # Session.userMessage), later ones may switch it (notice persisted). + await _apply_model(message.get("model")) if text or attachments: content = build_user_content(text, attachments) asyncio.create_task(run_turn(content)) diff --git a/surfaces/gui/e2e/chat.spec.ts b/surfaces/gui/e2e/chat.spec.ts index 8658a877..b7b42d61 100644 --- a/surfaces/gui/e2e/chat.spec.ts +++ b/surfaces/gui/e2e/chat.spec.ts @@ -21,9 +21,9 @@ test("send → user bubble → streamed echo reply renders", async ({ page }) => // The message carried the composer's visible model (model-per-message contract): what the // user sees at send time is exactly what serves the turn. await expect(page.getByText("[model=anthropic:claude-opus-4-8]")).toBeVisible(); - // …and having sent, the model is now FIXED for this session (§17/§22): the composer picker is - // gone and the fact reads in the topbar's facts subtitle instead. - await expect(page.locator(".dd").filter({ hasText: "Claude Opus" })).toHaveCount(0); + // …and the picker STAYS actionable after the first turn (§17 rev 2026-07-22 — mid-session + // switching shipped); the fact also reads in the topbar's facts subtitle. + await expect(page.locator(".dd").filter({ hasText: "Claude Opus" })).toBeVisible(); await expect(page.getByTestId("session-subtitle")).toContainText("Claude Opus 4.8"); // Composer cleared and re-armed for the next turn. await expect(box).toHaveValue(""); diff --git a/surfaces/gui/e2e/fixtures.ts b/surfaces/gui/e2e/fixtures.ts index 5d11d97e..82a52b58 100644 --- a/surfaces/gui/e2e/fixtures.ts +++ b/surfaces/gui/e2e/fixtures.ts @@ -568,9 +568,11 @@ export async function mockApi(page: import("@playwright/test").Page) { send("ready"); let pendingTool = "run_shell"; // which proposal the next approval decision resolves let epicTimer: ReturnType | null = null; // the slow stream, stoppable via interrupt + let hadTurn = false; // a user_message landed — set_model is now a mid-session switch ws.onMessage((raw) => { const msg = JSON.parse(String(raw)); if (msg.type === "user_message") { + hadTurn = true; send("turn_start", { input: msg.text }); if (/run a tool/i.test(msg.text)) { pendingTool = "run_shell"; @@ -703,6 +705,14 @@ export async function mockApi(page: import("@playwright/test").Page) { } send("interrupted", {}); send("turn_done"); + } else if (msg.type === "set_model") { + // Mid-session switch: the server applies it and broadcasts the persisted marker. + // Like the real server, the FIRST bind (fresh session) is silent. + if (hadTurn) + send("model_changed", { + model: msg.model, + text: `Model switched to ${msg.model}`, + }); } else if (msg.type === "retry") { // Like the real engine: re-runs with NO new user message (turn_start input is empty). send("turn_start", { input: "" }); diff --git a/surfaces/gui/e2e/model-switch.spec.ts b/surfaces/gui/e2e/model-switch.spec.ts new file mode 100644 index 00000000..949fdc1e --- /dev/null +++ b/surfaces/gui/e2e/model-switch.spec.ts @@ -0,0 +1,33 @@ +// Model-layer roadmap item 3 (2026-07-22): the model picker stays actionable for the +// session's whole life (supersedes the 2026-07-04 lock that hid it after the first turn). +// A mid-session switch drops a persisted info marker into the transcript, and later +// messages ride the new model. +import { expect } from "@playwright/test"; +import { test } from "./fixtures"; + +test("mid-session model switch shows the marker and later turns use the new model", async ({ + page, +}) => { + await page.goto("/"); + await page.getByText("Draft the launch note").first().click(); + const box = page.getByPlaceholder(/Ask the coworker/); + await box.fill("hello there"); + await box.press("Enter"); + await expect(page.getByText("Echo: hello there", { exact: false }).first()).toBeVisible(); + + // The picker is still in the composer after the first turn (the old lock hid it). + const picker = page.locator(".dd").filter({ hasText: "Claude Opus 4.8" }); + await expect(picker).toBeVisible(); + await picker.locator(".pill").click(); + await page.locator(".dd-item").filter({ hasText: "GPT-5.5" }).click(); + + // The switch marker lands in the transcript… + await expect(page.getByText(/Model switched to gpt-5.5/).first()).toBeVisible(); + + // …and the next message carries the new model (the fixture echoes it back). + await box.fill("after the switch"); + await box.press("Enter"); + await expect( + page.getByText("Echo: after the switch [model=gpt-5.5]", { exact: false }).first(), + ).toBeVisible(); +}); diff --git a/surfaces/gui/e2e/session-shell.spec.ts b/surfaces/gui/e2e/session-shell.spec.ts index e4aff754..39389a52 100644 --- a/surfaces/gui/e2e/session-shell.spec.ts +++ b/surfaces/gui/e2e/session-shell.spec.ts @@ -44,7 +44,8 @@ test("facts subtitle: absent on a fresh session, persona · model after the firs await expect(page.getByRole("button", { name: "About this persona" })).toHaveCount(0); await expect(page.locator(".dd").filter({ hasText: "Claude Opus 4.8" })).toBeVisible(); - // First turn → the model chip leaves the composer; the facts move up to the subtitle. + // First turn → the facts move up to the subtitle; the picker STAYS in the composer + // (§17 rev 2026-07-22: mid-session model switching shipped, so it remains actionable). const box = page.getByPlaceholder(/Ask the coworker/); await box.fill("hello"); await page.getByRole("button", { name: "Send" }).click(); @@ -52,7 +53,7 @@ test("facts subtitle: absent on a fresh session, persona · model after the firs const sub = page.getByTestId("session-subtitle"); await expect(sub).toContainText("Coworker · Claude Opus 4.8"); - await expect(page.locator(".dd").filter({ hasText: "Claude Opus 4.8" })).toHaveCount(0); + await expect(page.locator(".dd").filter({ hasText: "Claude Opus 4.8" })).toBeVisible(); // The subtitle is the session's fixed facts — clicking it opens the coworker (persona) page, // replacing the old topbar sliders button. diff --git a/surfaces/gui/src/App.tsx b/surfaces/gui/src/App.tsx index 62ca9fff..2e41e34c 100644 --- a/surfaces/gui/src/App.tsx +++ b/surfaces/gui/src/App.tsx @@ -638,6 +638,12 @@ export function App() { if (d.status === "max_iterations_exceeded") setItems((p) => [...p, { kind: "notice", tone: "warn", text: "Stopped: max iterations reached." }]); break; + case "model_changed": + // Mid-session switch (server-applied): update the header fact and drop the + // persisted marker into the live transcript (replay renders it from history). + if (d.model) setModel(d.model); + setItems((p) => [...p, { kind: "notice", tone: "info", text: d.text || "Model switched" }]); + break; case "interrupted": flushPartialStream(); setItems((p) => [...p, { kind: "notice", tone: "warn", text: "Interrupted." }]); @@ -812,6 +818,7 @@ export function App() { sessionRef.current?.setMode(m); }; const changeModel = (m: string) => { + if (running) return; // the server refuses mid-turn rebinds — don't let the header lie setModel(m); sessionRef.current?.setModel(m); }; @@ -1451,7 +1458,6 @@ export function App() { model={model} models={models} modelLabels={modelLabels} - modelLocked={items.length > 0} running={running} connected={connected} modelReady={modelReady} diff --git a/surfaces/gui/src/components/Composer.tsx b/surfaces/gui/src/components/Composer.tsx index 011a8e19..894c892e 100644 --- a/surfaces/gui/src/components/Composer.tsx +++ b/surfaces/gui/src/components/Composer.tsx @@ -49,7 +49,6 @@ interface Props { // The model is FIXED once the session has history (§17): the picker renders ONLY on a fresh // session; after the first turn the fact lives in the topbar subtitle (§22) — no // interactive-then-disabled control. - modelLocked?: boolean; running: boolean; connected: boolean; // False when the default model's provider has no key — the composer shows a "connect a model" @@ -461,8 +460,9 @@ export function Composer(props: Props) { - {/* model — a quiet chip on a FRESH session only; once the session has history the - fact moves up to the topbar subtitle (§17 expressed spatially). */} + {/* model — a quiet chip, now for the session's whole life (§17 rev 2026-07-22: + mid-session switching shipped, so the picker stays actionable; the topbar + subtitle still states the current model). */} {!dictation?.recording && (needsModel ? ( + ) : modelsLoaded ? ( + ) : ( - !props.modelLocked && - (modelsLoaded ? ( - - ) : ( - - )) + ))} {/* mic — immediately before send (owner call, DMG #28 walkthrough) */} diff --git a/surfaces/gui/src/itemsFromMessages.test.ts b/surfaces/gui/src/itemsFromMessages.test.ts index f8b9cf95..993c5e8b 100644 --- a/surfaces/gui/src/itemsFromMessages.test.ts +++ b/surfaces/gui/src/itemsFromMessages.test.ts @@ -68,3 +68,17 @@ describe("itemsFromMessages notices", () => { ]); }); }); + +describe("itemsFromMessages model switch", () => { + it("replays the persisted model_switch marker as an info notice", () => { + const items = itemsFromMessages([ + { role: "user", content: "hi" }, + { role: "notice", kind: "model_switch", text: "Model switched to Kimi K2.6 · Moonshot" }, + ] as any); + expect(items[1]).toEqual({ + kind: "notice", + tone: "info", + text: "Model switched to Kimi K2.6 · Moonshot", + }); + }); +}); diff --git a/surfaces/gui/src/itemsFromMessages.ts b/surfaces/gui/src/itemsFromMessages.ts index 1ac8b53c..1f8b3c98 100644 --- a/surfaces/gui/src/itemsFromMessages.ts +++ b/surfaces/gui/src/itemsFromMessages.ts @@ -59,13 +59,15 @@ export function itemsFromMessages(messages: ConversationMessage[]): Item[] { }); } } else if (m.role === "notice") { - // Persisted turn-ending marker (engine `_append_notice`): error/interrupted survive + // Persisted markers (engine `_append_notice`): error/interrupted/model-switch survive // reload exactly like the live view rendered them. An error notice is retriable — // the Transcript only offers the button when it's the transcript tail. items.push( m.kind === "interrupted" ? { kind: "notice", tone: "warn", text: "Interrupted." } - : { kind: "notice", tone: "warn", text: "Error: " + (m.text || "unknown"), retriable: true }, + : m.kind === "model_switch" + ? { kind: "notice", tone: "info", text: m.text || "Model switched" } + : { kind: "notice", tone: "warn", text: "Error: " + (m.text || "unknown"), retriable: true }, ); } // system messages are omitted; tool-result messages are folded into the tool row above diff --git a/surfaces/gui/src/types.ts b/surfaces/gui/src/types.ts index b835eeee..27f87763 100644 --- a/surfaces/gui/src/types.ts +++ b/surfaces/gui/src/types.ts @@ -15,6 +15,7 @@ export type EventType = | "turn_end" | "error" | "interrupted" + | "model_changed" | "turn_done"; export interface WsEvent { diff --git a/tests/test_engine.py b/tests/test_engine.py index 98f5537c..90e38fdb 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -381,3 +381,61 @@ def test_provider_extras_persist_on_message_and_survive_outbound(tmp_path): outbound = engine._outbound_messages()[-1] assert outbound["_gemini"] == {"text_sig": "c2ln", "call_sigs": []} assert "ts" not in outbound # display sidecars still stripped + + +def test_switch_model_appends_notice_only_midsession(tmp_path): + engine, _ = _engine(tmp_path, [_text_turn("ok")]) + # Fresh session: first bind is silent. + assert engine.switch_model("zai:glm-5.2") is None + assert engine.model == "zai:glm-5.2" + _collect(engine, "hi") + # Same model: no-op. + assert engine.switch_model("zai:glm-5.2") is None + # Real mid-session switch: persisted marker with the matrix label. + text = engine.switch_model("kimi:kimi-k2.6") + assert "Kimi K2.6" in text and engine.model == "kimi:kimi-k2.6" + notice = engine.messages[-1] + assert notice["role"] == "notice" and notice["kind"] == "model_switch" + assert all(m.get("role") != "notice" for m in engine._outbound_messages()) + + +def test_switch_model_warns_when_images_meet_text_only_model(tmp_path): + class NoVisionProvider(ScriptedProvider): + def capabilities(self, model): + return ModelCapabilities(vision=False) + + engine, _ = _engine(tmp_path, [_text_turn("ok")]) + engine.provider = NoVisionProvider([_text_turn("ok")]) + engine.messages.append( + { + "role": "user", + "content": [ + {"type": "text", "text": "look"}, + {"type": "image_url", "image_url": {"url": "data:image/png;base64,AA=="}}, + ], + } + ) + text = engine.switch_model("zai:glm-5.2") + assert "images" in text # degradation is called out in the marker + + +def test_outbound_replaces_images_for_non_vision_models(tmp_path): + class NoVisionProvider(ScriptedProvider): + def capabilities(self, model): + return ModelCapabilities(vision=False) + + engine, _ = _engine(tmp_path, [_text_turn("ok")]) + engine.provider = NoVisionProvider([_text_turn("ok")]) + engine.messages.append( + { + "role": "user", + "content": [ + {"type": "text", "text": "look"}, + {"type": "image_url", "image_url": {"url": "data:image/png;base64,AA=="}}, + ], + } + ) + parts = engine._outbound_messages()[-1]["content"] + assert all(p["type"] != "image_url" for p in parts) + assert "not viewable" in parts[-1]["text"] + assert engine.messages[-1]["content"][1]["type"] == "image_url" # history untouched diff --git a/tests/test_server.py b/tests/test_server.py index 40614c64..686fdb47 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -614,33 +614,38 @@ def test_ws_session_resume_via_store(tmp_path): assert any(s["session_id"] == "keep" and s["messages"] > 0 for s in sessions) -def test_ws_first_message_binds_the_session_model_then_locks(tmp_path): - """The FIRST user_message's model binds the session (race-proof across reconnects — found - 2026-07-04: a new cowork session reconnects to adopt its scratch dir, which could drop a - queued set_model and leave the engine on a stale/resumed model). After the first turn the - model is FIXED for the session's life: later message models and set_model are ignored - (owner call, 2026-07-04 — mixed-model transcripts invite provider-quirk breakage). - """ - client = _client(tmp_path, [_text("ok"), _text("ok again"), _text("still ok")]) +def test_ws_first_message_binds_then_midsession_switch_persists_notice(tmp_path): + """The FIRST user_message's model binds the session silently (race-proof across + reconnects — found 2026-07-04). Mid-session rebinds are ALLOWED (roadmap item 3, + 2026-07-22, supersedes the 07-04 lock): the switch lands as a persisted model_switch + notice and a model_changed broadcast, and the next turn runs on the new model.""" + # 4 turns: 3 user turns + the autotitle's fire-and-forget complete() after turn 1. + client = _client( + tmp_path, [_text("ok"), _text("Session title"), _text("ok again"), _text("still ok")] + ) with client.websocket_connect("/ws/session/model-per-msg") as ws: ready = ws.receive_json() assert ready["type"] == "ready" - default_model = ready["data"]["model"] ws.send_json({"type": "user_message", "text": "hi", "model": "zai:glm-5.2"}) - _drain(ws) + assert "model_changed" not in _drain(ws) # first bind is silent # message WITHOUT a model keeps the bound one (no silent reset to default) ws.send_json({"type": "user_message", "text": "again"}) _drain(ws) - # locked: neither a different message model nor set_model can rebind mid-session ws.send_json({"type": "set_model", "model": "kimi:kimi-k2.6"}) - ws.send_json( - {"type": "user_message", "text": "switch?", "model": "kimi:kimi-k2.6"} - ) + changed = ws.receive_json() + assert changed["type"] == "model_changed" + assert changed["data"]["model"] == "kimi:kimi-k2.6" + assert "Kimi" in changed["data"]["text"] + ws.send_json({"type": "user_message", "text": "switched now"}) _drain(ws) mgr = client.app.state.manager engine = mgr._engines["model-per-msg"] - assert engine.model == "zai:glm-5.2" - assert engine.model != default_model + assert engine.model == "kimi:kimi-k2.6" + # The marker is persisted between the turns; the provider never sees it. + messages = client.get("/v1/sessions/model-per-msg/messages").json()["messages"] + notices = [m for m in messages if m["role"] == "notice"] + assert [n["kind"] for n in notices] == ["model_switch"] + assert all(m.get("role") != "notice" for m in engine._outbound_messages()) def test_session_messages_prefers_the_live_engine(tmp_path):