From 5fc454a9c3238b7f9f2b73c257ed1d0a9a3737b8 Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Wed, 12 Aug 2026 11:19:54 -0700 Subject: [PATCH] fix(recap): render the suggestion ghost in the composer BEFORE Tab MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The recap suggestion (#828) armed correctly — Tab inserted it — but the ghost never painted: appLayout's placeholder gate keyed on `composer.empty`, which is CONVERSATION-emptiness (useMainApp: `!historyItems.some(m => m.kind !== 'intro')`), not input-emptiness. So the slot was blanked in the only state where a suggestion exists (mid-conversation), and every state-level test still passed. The slot selection now lives in `composerPlaceholder({busy, conversationEmpty, pendingSuggestion})`: busy blanks it, an armed suggestion wins whenever idle, and the static `Try "..."` hint stays fresh-conversation-only (historical behavior byte-preserved). Input-emptiness was never this gate's concern — TextInput itself hides any placeholder once the input has text. Regression coverage at the level that would have caught this: a new screen-level e2e (tests/test_tui_recap_ghost_e2e.py) runs the REAL built TUI in a POSIX pty against a deterministic fake agent-server and reads the terminal with pyte — asserting the suggestion is VISIBLE before any Tab, and that Tab + typing APPENDS (ghost became editable input). Mutation-verified in both directions: with the fix stashed and dist rebuilt it fails exactly on the pre-Tab ghost assertion. Skip-guarded via importorskip (pyte, pty) + node/dist checks — a missing `pty` on Windows is a clean skip, not a collection error (simulated). pyte added to requirements.dev.txt + pyproject dev extras in lockstep. Plus 4 unit tests pinning the composerPlaceholder truth table. Co-Authored-By: Claude Fable 5 --- pyproject.toml | 4 + requirements.dev.txt | 3 + tests/test_tui_recap_ghost_e2e.py | 224 +++++++++++++++++++++++++ ui-tui/src/__tests__/turnRecap.test.ts | 23 +++ ui-tui/src/components/appLayout.tsx | 10 +- ui-tui/src/content/placeholders.ts | 27 +++ 6 files changed, 289 insertions(+), 2 deletions(-) create mode 100644 tests/test_tui_recap_ghost_e2e.py diff --git a/pyproject.toml b/pyproject.toml index b8cfdc137..0172fcd6c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -107,6 +107,10 @@ dev = [ # Ch16 Phase 0: coverage reporting for the ≥90% line-coverage DoD on # the new ``src/bridge/`` modules. "pytest-cov>=5.0", + # Terminal emulator for screen-level TUI e2e tests + # (tests/test_tui_recap_ghost_e2e.py — skips where node/dist are absent). + # Kept in lockstep with requirements.dev.txt. + "pyte>=0.8.2", ] [project.scripts] diff --git a/requirements.dev.txt b/requirements.dev.txt index 497442f2b..09282b3d0 100644 --- a/requirements.dev.txt +++ b/requirements.dev.txt @@ -8,3 +8,6 @@ twine>=5.0.0 pytest>=8.0.0 pytest-asyncio>=1.3.0 pytest-cov>=5.0 +# Terminal emulator for screen-level TUI e2e tests +# (tests/test_tui_recap_ghost_e2e.py — skips where node/dist are absent). +pyte>=0.8.2 diff --git a/tests/test_tui_recap_ghost_e2e.py b/tests/test_tui_recap_ghost_e2e.py new file mode 100644 index 000000000..9d194a52f --- /dev/null +++ b/tests/test_tui_recap_ghost_e2e.py @@ -0,0 +1,224 @@ +"""Screen-level regression test: the recap suggestion renders as composer +ghost text BEFORE Tab, and Tab turns it into real input. + +The first ship of the recap feature (#828) armed the state correctly — Tab +inserted the suggestion — but the ghost never PAINTED: the appLayout +placeholder gate keyed on ``composer.empty``, which is conversation-emptiness, +so the slot was blanked in the only state where a suggestion exists +(mid-conversation). Every state-level test passed. Only looking at the actual +terminal catches that class of bug, so this test runs the REAL TUI binary +(``ui-tui/dist/entry.js``) in a PTY against a deterministic fake agent-server +speaking the NDJSON protocol, and reads the screen with pyte: + +1. wait for the composer, type a prompt, Enter; +2. fake server replies (stream → result → recap frame with a FIXED suggestion); +3. assert the suggestion text is on the composer row before any Tab; +4. Tab, then type a marker — the marker must APPEND (ghost became input; + pre-Tab typing would replace, since a placeholder hides on input). + +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 for +real 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 (same guard as +# test_tool_system_tools.py's pty import). +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"), +] + +SUGGESTION = "fix all four issues and re-run" +RECAP_TEXT = "Goal was testing the ghost. Turn done. Next: accept the suggestion." + +# A minimal agent-server: init frame, echo turn (stream → assistant → result), +# then the recap frame with a FIXED suggestion; every control_request gets an +# empty-object reply so startup RPCs (settings, workflow list, …) resolve. +FAKE_SERVER = textwrap.dedent( + """ + import json, sys, threading, time + + def emit(obj): + sys.stdout.write(json.dumps(obj) + "\\n") + sys.stdout.flush() + + emit({ + "type": "system", "subtype": "init", "session_id": "s1", + "model": "fake-model", "tools": [], "permission_mode": "default", + "protocol_version": "0.1.0", "cwd": ".", + }) + + def turn(): + emit({"type": "stream_event", "session_id": "s1", "event": { + "type": "content_block_delta", + "delta": {"type": "text_delta", "text": "smoke ok"}}}) + emit({"type": "assistant", "session_id": "s1", "uuid": "a1", + "message": {"role": "assistant", + "content": [{"type": "text", "text": "smoke ok"}]}}) + emit({"type": "result", "subtype": "success", "session_id": "s1", + "num_turns": 1, "result": "smoke ok", "duration_ms": 5, + "is_error": False, "usage": None, "total_cost_usd": 0.0, + "cost": {}, "session_turns": 1}) + time.sleep(0.4) # recap generation happens after the result + emit({"type": "system", "subtype": "recap", "session_id": "s1", + "recap": %(recap)r, "suggestion": %(suggestion)r}) + + 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") == "user": + threading.Thread(target=turn, daemon=True).start() + elif 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": {}}}) + """ +) % {"recap": RECAP_TEXT, "suggestion": SUGGESTION} + + +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_suggestion_ghost_renders_before_tab_and_tab_accepts(tui): + # Composer up (the ❯ prompt row paints once the client is interactive). + assert tui.wait_for("❯", 30), f"composer never appeared:\n{tui.dump()}" + + tui.send("hello") + assert tui.wait_for("hello", 5), f"typed text not echoed:\n{tui.dump()}" + tui.send("\r") + + # Fake turn completes and the recap frame lands. + assert tui.wait_for("recap:", 20), f"recap line missing:\n{tui.dump()}" + assert tui.row_with(RECAP_TEXT.split(".")[0]) is not None + + # THE regression: the suggestion must be visible BEFORE any Tab press, + # as ghost text in the (empty) composer. + assert tui.wait_for(SUGGESTION, 5), ( + "suggestion ghost did not render before Tab:\n" + tui.dump() + ) + + # Tab accepts; typing then APPENDS — proving the ghost became real input + # (pre-Tab typing would hide the placeholder and show only the marker). + tui.send("\t") + tui.pump(0.6) + tui.send("XY") + assert tui.wait_for(SUGGESTION + "XY", 5), ( + "Tab did not turn the ghost into editable input:\n" + tui.dump() + ) diff --git a/ui-tui/src/__tests__/turnRecap.test.ts b/ui-tui/src/__tests__/turnRecap.test.ts index 89b5af982..609ff2541 100644 --- a/ui-tui/src/__tests__/turnRecap.test.ts +++ b/ui-tui/src/__tests__/turnRecap.test.ts @@ -9,6 +9,7 @@ import { turnController } from '../app/turnController.js' import { resetTurnState } from '../app/turnStore.js' import { getUiState, patchUiState, resetUiState } from '../app/uiStore.js' import { shouldAcceptPendingSuggestion } from '../app/useInputHandlers.js' +import { composerPlaceholder, PLACEHOLDER } from '../content/placeholders.js' import type { Msg } from '../types.js' // Fake child process for the GatewayClient NDJSON translation test — same @@ -131,6 +132,28 @@ describe('turn.recap event handling', () => { }) }) +describe('composerPlaceholder — what the ghost slot shows', () => { + it('shows the armed suggestion MID-conversation (the shipped regression: conversationEmpty gated it off exactly where suggestions exist)', () => { + expect( + composerPlaceholder({ busy: false, conversationEmpty: false, pendingSuggestion: 'fix all four issues and re-run' }) + ).toBe('fix all four issues and re-run') + }) + + it('suggestion wins over the fresh-conversation hint too', () => { + expect(composerPlaceholder({ busy: false, conversationEmpty: true, pendingSuggestion: 'do it' })).toBe('do it') + }) + + it('keeps the static hint fresh-conversation-only', () => { + expect(composerPlaceholder({ busy: false, conversationEmpty: true, pendingSuggestion: null })).toBe(PLACEHOLDER) + expect(composerPlaceholder({ busy: false, conversationEmpty: false, pendingSuggestion: null })).toBe('') + }) + + it('busy blanks the slot regardless', () => { + expect(composerPlaceholder({ busy: true, conversationEmpty: false, pendingSuggestion: 'x' })).toBe('') + expect(composerPlaceholder({ busy: true, conversationEmpty: true, pendingSuggestion: null })).toBe('') + }) +}) + describe('shouldAcceptPendingSuggestion — Tab accepts only what is visibly suggested', () => { const visible = { busy: false, completionsLen: 0, input: '', suggestion: 'fix the tests' } diff --git a/ui-tui/src/components/appLayout.tsx b/ui-tui/src/components/appLayout.tsx index 053f7f852..06b084b3e 100644 --- a/ui-tui/src/components/appLayout.tsx +++ b/ui-tui/src/components/appLayout.tsx @@ -8,7 +8,7 @@ import { $isBlocked, $overlayState, patchOverlayState } from '../app/overlayStor import { $uiState } from '../app/uiStore.js' import { usePet } from '../app/usePet.js' import { INLINE_MODE, SHOW_FPS, TERMUX_TUI_MODE, TRANSCRIPT_COLOR } from '../config/env.js' -import { PLACEHOLDER } from '../content/placeholders.js' +import { composerPlaceholder } from '../content/placeholders.js' import { prevRenderedMsg, showsInterTurnSeparator } from '../domain/blockLayout.js' import { COMPOSER_PROMPT_GAP_WIDTH, @@ -348,7 +348,13 @@ const ComposerPane = memo(function ComposerPane({ onChange={composer.updateInput} onPaste={composer.handleTextPaste} onSubmit={composer.submit} - placeholder={composer.empty && !ui.busy ? (ui.pendingSuggestion ?? PLACEHOLDER) : ''} + placeholder={composerPlaceholder({ + busy: ui.busy, + // NB: composer.empty is CONVERSATION-emptiness (fresh + // transcript), not input-emptiness — see composerPlaceholder. + conversationEmpty: composer.empty, + pendingSuggestion: ui.pendingSuggestion + })} value={composer.input} voiceRecordKey={composer.voiceRecordKey} /> diff --git a/ui-tui/src/content/placeholders.ts b/ui-tui/src/content/placeholders.ts index 06b5a4fb3..df27bbc40 100644 --- a/ui-tui/src/content/placeholders.ts +++ b/ui-tui/src/content/placeholders.ts @@ -12,6 +12,33 @@ export const PLACEHOLDERS = [ export const PLACEHOLDER = pick(PLACEHOLDERS) +/** + * What the composer's ghost slot shows while idle. The recap suggestion wins + * whenever one is armed — that is the whole feature: it appears MID- + * conversation, right after a turn ends. The static `Try "…"` hint stays + * fresh-conversation-only (its historical behavior). Busy always blanks the + * slot, and TextInput itself hides any placeholder once the input has text, + * so input-emptiness is NOT this function's concern. + * + * Regression note: the first ship gated BOTH on `composer.empty`, which is + * conversation-emptiness (useMainApp: `!historyItems.some(m => m.kind !== + * 'intro')`), not input-emptiness — so the suggestion ghost could never + * render in the only state where a suggestion exists. Tab-accept still + * worked (it checks the real input), which is exactly why the miss was + * invisible to the state-level tests. + */ +export const composerPlaceholder = (state: { + busy: boolean + conversationEmpty: boolean + pendingSuggestion: null | string +}): string => { + if (state.busy) { + return '' + } + + return state.pendingSuggestion ?? (state.conversationEmpty ? PLACEHOLDER : '') +} + /** * The tab-acceptable query inside a composer placeholder: `Try "explain this * codebase"` suggests the query `explain this codebase`; a placeholder with no