From b43b49504487de1351bfdef60db78b50518c6afe Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Fri, 14 Aug 2026 00:36:40 -0700 Subject: [PATCH] fix(tui): a sibling slash command no longer cancels /model's reply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Picking a model in /model switched the session but left the stats line under the composer reading the OLD provider AND model — `anthropic · claude-opus-5` next to answers coming from DeepSeek. #836 is what armed it. createSlashHandler kept ONE flight counter shared by every dispatch, and guarded/stale() dropped any reply that was no longer the newest — a guard meant for "the user moved on from this command" that could not tell that apart from "the user ran a different command". #836 gave the picker a third step, so onModelSelect now dispatches `/model …` and `/effort …` back-to-back in a single tick: the second bumped the counter before the first's RPC returned, and the `/model` reply — the one that folds provider+model into ui.info via infoAfterModelSwitch and prints `model → …` — was discarded. The backend had already committed the switch, so only the label was wrong, and both halves went stale rather than just the provider #836 threaded. Count dispatches per resolved command name instead, so aliases of one command still supersede each other while an unrelated command leaves it alone. Unregistered commands keep one shared lane on purpose: there the output IS the product, so a newer exec slash should still suppress an older one's late output (the existing slash.exec test pins that). Only fires when step 3 lands on a real effort level — the picker preselects the session's live level and emits no `/effort` for `auto`, so an auto session dispatches one command and never saw this. Both unit tests are mutation-checked: reinstating the shared counter fails the same-tick test, and keying on the full command string fails the supersede test. The e2e drives the real TUI binary in a PTY through all three picker steps, and reproduces the exact reported row before the fix. Co-Authored-By: Claude Opus 5 --- tests/test_tui_model_picker_stats_e2e.py | 267 ++++++++++++++++++ .../src/__tests__/createSlashHandler.test.ts | 58 +++- ui-tui/src/app/createSlashHandler.ts | 29 +- ui-tui/src/app/interfaces.ts | 3 +- ui-tui/src/app/slash/types.ts | 2 +- ui-tui/src/app/useMainApp.ts | 2 +- 6 files changed, 352 insertions(+), 9 deletions(-) create mode 100644 tests/test_tui_model_picker_stats_e2e.py diff --git a/tests/test_tui_model_picker_stats_e2e.py b/tests/test_tui_model_picker_stats_e2e.py new file mode 100644 index 000000000..8c6fd287b --- /dev/null +++ b/tests/test_tui_model_picker_stats_e2e.py @@ -0,0 +1,267 @@ +"""Screen-level regression test: the stats line under the composer repoints to +the newly selected provider AND model after a /model picker switch. + +#836 threaded `provider` end to end so a cross-provider selection could not +leave `anthropic · gpt-5.6-luna` on the row — and every state-level test for it +passed. It also gave the picker a third step (effort), which made +``onModelSelect`` dispatch ``/model …`` and ``/effort …`` back-to-back in ONE +tick. ``createSlashHandler`` kept a single "flight" counter shared by every +slash, so the second dispatch marked the first superseded and +``guarded`` discarded the ``/model`` reply — the one that folds provider+model +into ``ui.info``. The backend still switched, so the session answered from +DeepSeek while the row kept reading ``anthropic · claude-opus-5``. + +The bug only exists when both commands land in the same tick, which no typed +input can produce — it needs the real picker driving the real handler. So this +runs the REAL TUI binary (``ui-tui/dist/entry.js``) in a PTY against a +deterministic fake agent-server speaking NDJSON, walks the three picker steps +with the keyboard, and reads the row back with pyte. + +Skips (never fails) when the local environment can't run it: no node, no built +``ui-tui/dist``, no pyte, or non-POSIX (pty module). The python CI job installs +no node toolchain, so there this is a documented skip; it runs on dev machines. +""" + +from __future__ import annotations + +import json +import os +import select +import shutil +import subprocess +import sys +import textwrap +import time +from pathlib import Path + +import pytest + +REPO = Path(__file__).resolve().parent.parent +ENTRY = REPO / "ui-tui" / "dist" / "entry.js" + +pyte = pytest.importorskip("pyte", reason="pyte not installed (dev-only e2e)") +# importorskip, NOT a top-level import: `pty` does not exist on Windows, and a +# module-level ImportError is a pytest COLLECTION ERROR there — the win32 +# skipif marker below never gets a chance to apply. +pty = pytest.importorskip("pty", reason="POSIX pty required") + +pytestmark = [ + pytest.mark.skipif(sys.platform == "win32", reason="POSIX pty required"), + pytest.mark.skipif(shutil.which("node") is None, reason="node not on PATH"), + pytest.mark.skipif(not ENTRY.exists(), reason="ui-tui/dist not built"), +] + +OLD_MODEL = "claude-opus-5" +OLD_PROVIDER = "anthropic" +NEW_MODEL = "deepseek-v4-flash" +NEW_PROVIDER = "deepseek" + +# A minimal agent-server: an init frame that seeds the stats line with the OLD +# pairing, a one-provider/one-model picker so the arrow keys have nothing to +# disambiguate, a full effort ladder so step 3 renders (that third step is what +# fires the second dispatch), and a set_model that echoes the new provider the +# way agent_server._do_set_model does. Every other control_request gets an +# empty-object reply so startup RPCs resolve. +FAKE_SERVER = textwrap.dedent( + """ + import json, sys + + def emit(obj): + sys.stdout.write(json.dumps(obj) + "\\n") + sys.stdout.flush() + + emit({ + "type": "system", "subtype": "init", "session_id": "s1", + "model": %(old_model)r, "provider": %(old_provider)r, + "tools": [], "permission_mode": "default", + "protocol_version": "0.1.0", "cwd": ".", + }) + + def answer(subtype): + if subtype == "list_model_providers": + return { + "ok": True, + "model": %(old_model)r, + "provider": %(old_provider)r, + "providers": [{ + "slug": %(new_provider)r, + "name": %(new_provider)r, + "authenticated": True, + "models": [%(new_model)r], + "total_models": 1, + }], + } + if subtype == "effort_options": + # `current` must name a REAL level, not "". Step 3 lists + # [auto, *levels] and lands on the session's live level, and the + # picker only emits `/effort` when the chosen row is not `auto` — + # so a session sitting on `auto` dispatches ONE command and cannot + # arm this regression at all. The reported session was on `max`, + # which is why plain Enter-through hit it. + return { + "ok": True, + "supported": True, + "levels": ["low", "medium", "high"], + "current": "high", + } + if subtype == "set_model": + # The shape #836 settled on: echo the provider beside the model. + return {"ok": True, "model": %(new_model)r, "provider": %(new_provider)r} + return {} + + for line in sys.stdin: + line = line.strip() + if not line: + continue + try: + msg = json.loads(line) + except json.JSONDecodeError: + continue + if msg.get("type") == "control_request": + req = msg.get("request") or {} + emit({"type": "control_response", "response": { + "subtype": req.get("subtype", ""), + "request_id": msg.get("request_id"), + "response": answer(req.get("subtype", "")), + }}) + """ +) % { + "old_model": OLD_MODEL, + "old_provider": OLD_PROVIDER, + "new_model": NEW_MODEL, + "new_provider": NEW_PROVIDER, +} + + +class _TuiSession: + """The real TUI in a PTY, screen mirrored into a pyte emulator.""" + + COLS, ROWS = 140, 40 + + def __init__(self, tmp_path: Path): + server_path = tmp_path / "fake_agent_server.py" + server_path.write_text(FAKE_SERVER, encoding="utf-8") + env = { + **os.environ, + "TERM": "xterm-256color", + "CLAWCODEX_WORKSPACE": str(tmp_path), + "CLAWCODEX_CONFIG_DIR": str(tmp_path / "cfg"), + "CLAWCODEX_AGENT_SERVER_CMD": json.dumps( + [sys.executable, str(server_path)] + ), + } + self.master, slave = pty.openpty() + # Emulator and PTY must agree on geometry or wraps differ. + import fcntl + import struct + import termios + + fcntl.ioctl( + slave, termios.TIOCSWINSZ, + struct.pack("HHHH", self.ROWS, self.COLS, 0, 0), + ) + self.proc = subprocess.Popen( + ["node", str(ENTRY)], + stdin=slave, stdout=slave, stderr=slave, + cwd=str(tmp_path), env=env, close_fds=True, + ) + os.close(slave) + self.screen = pyte.Screen(self.COLS, self.ROWS) + self.stream = pyte.ByteStream(self.screen) + + def pump(self, seconds: float) -> None: + end = time.time() + seconds + while time.time() < end: + ready, _, _ = select.select([self.master], [], [], 0.05) + if not ready: + continue + try: + data = os.read(self.master, 65536) + except OSError: + return + if not data: + return + self.stream.feed(data) + + def wait_for(self, needle: str, timeout: float) -> bool: + end = time.time() + timeout + while time.time() < end: + self.pump(0.2) + if any(needle in row for row in self.screen.display): + return True + return False + + def send(self, text: str) -> None: + os.write(self.master, text.encode()) + + def row_with(self, needle: str) -> str | None: + for row in self.screen.display: + if needle in row: + return row + return None + + def dump(self) -> str: + return "\n".join( + row.rstrip() for row in self.screen.display if row.strip() + ) + + def close(self) -> None: + self.proc.terminate() + try: + self.proc.wait(timeout=5) + except subprocess.TimeoutExpired: + self.proc.kill() + self.proc.wait() # reap — no zombie for the rest of the run + os.close(self.master) + + +@pytest.fixture() +def tui(tmp_path): + session = _TuiSession(tmp_path) + yield session + session.close() + + +def test_stats_line_repoints_to_the_picked_provider_and_model(tui): + # Composer up (the ❯ prompt row paints once the client is interactive). + assert tui.wait_for("❯", 30), f"composer never appeared:\n{tui.dump()}" + + # The stats line starts on the session's init pairing — this is the + # "before" the assertion at the bottom has to move off, so a picker that + # silently no-ops cannot pass by accident. + assert tui.wait_for(OLD_PROVIDER, 10), ( + f"stats line never showed the initial provider:\n{tui.dump()}" + ) + before = tui.row_with(f"{OLD_PROVIDER} · {OLD_MODEL}") + assert before is not None, ( + f"stats line did not start at '{OLD_PROVIDER} · {OLD_MODEL}':\n{tui.dump()}" + ) + + # Open the picker and walk its three steps. One provider and one model, so + # Enter alone selects each; step 3 lands preselected on `high` (the fake's + # `current`), so Enter there emits `/effort high` alongside `/model` — the + # two-dispatches-in-one-tick that arms the regression. + tui.send("/model") + tui.pump(0.5) + tui.send("\r") + assert tui.wait_for(NEW_PROVIDER, 10), f"picker never listed the provider:\n{tui.dump()}" + + tui.send("\r") # step 1 → provider + assert tui.wait_for(NEW_MODEL, 10), f"picker never listed the model:\n{tui.dump()}" + + tui.send("\r") # step 2 → model + assert tui.wait_for("low", 10), f"effort step never rendered:\n{tui.dump()}" + + tui.send("\r") # step 3 → effort, which applies the switch + + # THE regression: the row must name the provider AND model just selected. + assert tui.wait_for(f"{NEW_PROVIDER} · {NEW_MODEL}", 15), ( + "stats line did not repoint after the picker switch " + f"(expected '{NEW_PROVIDER} · {NEW_MODEL}'):\n{tui.dump()}" + ) + + # And neither stale half may survive anywhere on the row. + row = tui.row_with(f"{NEW_PROVIDER} · {NEW_MODEL}") + assert row is not None + assert OLD_PROVIDER not in row, f"stale provider still on the stats row: {row!r}" + assert OLD_MODEL not in row, f"stale model still on the stats row: {row!r}" diff --git a/ui-tui/src/__tests__/createSlashHandler.test.ts b/ui-tui/src/__tests__/createSlashHandler.test.ts index 63a61c33b..28d74299c 100644 --- a/ui-tui/src/__tests__/createSlashHandler.test.ts +++ b/ui-tui/src/__tests__/createSlashHandler.test.ts @@ -981,11 +981,65 @@ describe('createSlashHandler', () => { expect(ctx.transcript.sys).toHaveBeenCalledWith('title: demo title') }) }) + + // The /model picker dispatches `/model …` then `/effort …` synchronously in + // one tick. A single shared flight counter made the second dispatch mark the + // first as superseded, so the `/model` reply — which folds provider+model + // into ui.info and prints `model → …` — was dropped and the stats line kept + // describing the previous session while the backend had already switched. + it('still applies a /model switch when another command is dispatched in the same tick', async () => { + patchUiState({ + info: { model: 'claude-opus-5', profile_name: 'anthropic', skills: {}, tools: {} }, + sid: 'sid-abc' + }) + + const rpc = vi.fn((method: string) => + Promise.resolve( + method === 'config.set' ? { ok: true, provider: 'deepseek', value: 'deepseek-v4-flash' } : { ok: true } + ) + ) + + const ctx = buildCtx({ gateway: { ...buildGateway(), rpc } }) + const handler = createSlashHandler(ctx) + + for (const cmd of [`/model deepseek-v4-flash --provider deepseek ${TUI_SESSION_MODEL_FLAG}`, '/effort max']) { + handler(cmd) + } + + await vi.waitFor(() => { + expect(getUiState().info).toMatchObject({ model: 'deepseek-v4-flash', profile_name: 'deepseek' }) + }) + + expect(ctx.transcript.sys).toHaveBeenCalledWith('model → deepseek-v4-flash') + }) + + it('still supersedes an older dispatch of the SAME command', async () => { + patchUiState({ + info: { model: 'claude-opus-5', profile_name: 'anthropic', skills: {}, tools: {} }, + sid: 'sid-abc' + }) + + const rpc = vi.fn((_method: string, params: any) => + Promise.resolve({ ok: true, provider: 'deepseek', value: String(params.value).split(' ')[0] }) + ) + + const ctx = buildCtx({ gateway: { ...buildGateway(), rpc } }) + const handler = createSlashHandler(ctx) + + handler('/model deepseek-v4-flash --provider deepseek') + handler('/model deepseek-v4-plus --provider deepseek') + + await vi.waitFor(() => { + expect(getUiState().info).toMatchObject({ model: 'deepseek-v4-plus' }) + }) + + expect(ctx.transcript.sys).not.toHaveBeenCalledWith('model → deepseek-v4-flash') + }) }) const buildCtx = (overrides: Partial = {}): Ctx => ({ ...overrides, - slashFlightRef: overrides.slashFlightRef ?? { current: 0 }, + slashFlightRef: overrides.slashFlightRef ?? { current: {} }, composer: { ...buildComposer(), ...overrides.composer }, gateway: { ...buildGateway(), ...overrides.gateway }, local: { ...buildLocal(), ...overrides.local }, @@ -1050,7 +1104,7 @@ const buildVoice = () => ({ }) interface Ctx { - slashFlightRef: { current: number } + slashFlightRef: { current: Record } composer: ReturnType gateway: ReturnType local: ReturnType diff --git a/ui-tui/src/app/createSlashHandler.ts b/ui-tui/src/app/createSlashHandler.ts index fc4612d7d..aff7b8993 100644 --- a/ui-tui/src/app/createSlashHandler.ts +++ b/ui-tui/src/app/createSlashHandler.ts @@ -13,13 +13,36 @@ export function createSlashHandler(ctx: SlashHandlerContext): (cmd: string) => b const { page, send, sys } = ctx.transcript const handler = (cmd: string): boolean => { - const flight = ++ctx.slashFlightRef.current const ui = getUiState() const sid = ui.sid const parsed = parseSlashCommand(cmd) const argTail = parsed.arg ? ` ${parsed.arg}` : '' - const stale = () => flight !== ctx.slashFlightRef.current || getUiState().sid !== sid + const found = findSlashCommand(parsed.name) + + // A registered command counts its own dispatches; everything that falls + // through to the backend pipeline below shares one lane. + // + // The guard exists so a late reply cannot clobber state the user has moved + // on from, but "the user ran some other command" is not moving on from this + // one — and a single shared counter could not tell those apart. The /model + // picker dispatches `/model …` and `/effort …` back-to-back in one tick + // (useMainApp onModelSelect), so the second marked the first superseded and + // the pending `/model` reply — the one that folds provider+model into + // ui.info and prints `model → …` — was dropped, leaving the stats line + // describing the previous session while the backend had already switched. + // + // The shared exec lane is deliberate rather than incidental: there the + // OUTPUT is the whole product, so printing the results of a command the + // user has already moved past is noise, and any newer exec slash should + // still suppress it. Keyed on the canonical name so aliases of one command + // supersede each other; a session change is caught by the sid check. + const flights = ctx.slashFlightRef + const key = found ? `cmd:${found.name}` : 'exec' + const flight = (flights.current[key] ?? 0) + 1 + flights.current[key] = flight + + const stale = () => flight !== flights.current[key] || getUiState().sid !== sid const guarded = (fn: (r: T) => void) => @@ -37,8 +60,6 @@ export function createSlashHandler(ctx: SlashHandlerContext): (cmd: string) => b const runCtx: SlashRunCtx = { ...ctx, flight, guarded, guardedErr, sid, stale, ui } - const found = findSlashCommand(parsed.name) - if (found) { found.run(parsed.arg, runCtx, cmd) diff --git a/ui-tui/src/app/interfaces.ts b/ui-tui/src/app/interfaces.ts index e452c1f9b..4e35b08f7 100644 --- a/ui-tui/src/app/interfaces.ts +++ b/ui-tui/src/app/interfaces.ts @@ -397,7 +397,8 @@ export interface SlashHandlerContext { resumeById: (id: string) => void setSessionStartedAt: StateSetter } - slashFlightRef: MutableRefObject + /** Newest dispatch number per command name — see createSlashHandler. */ + slashFlightRef: MutableRefObject> transcript: { page: (text: string, title?: string) => void panel: (title: string, sections: PanelSection[]) => void diff --git a/ui-tui/src/app/slash/types.ts b/ui-tui/src/app/slash/types.ts index 14781aa12..551b0246a 100644 --- a/ui-tui/src/app/slash/types.ts +++ b/ui-tui/src/app/slash/types.ts @@ -7,7 +7,7 @@ export interface SlashRunCtx extends SlashHandlerContext { guarded: (fn: (r: T) => void) => (r: null | T) => void guardedErr: (e: unknown) => void sid: null | string - slashFlightRef: MutableRefObject + slashFlightRef: MutableRefObject> stale: () => boolean ui: UiState } diff --git a/ui-tui/src/app/useMainApp.ts b/ui-tui/src/app/useMainApp.ts index 0a00200d2..f2e8cdd42 100644 --- a/ui-tui/src/app/useMainApp.ts +++ b/ui-tui/src/app/useMainApp.ts @@ -217,7 +217,7 @@ export function useMainApp(gw: GatewayClient) { ) ) - const slashFlightRef = useRef(0) + const slashFlightRef = useRef>({}) const slashRef = useRef<(cmd: string) => boolean>(() => false) const colsRef = useRef(cols) const scrollRef = useRef(null)