From 942e33898ca5c57541ca780205d4925a6cf3be78 Mon Sep 17 00:00:00 2001 From: Vuong Hoang Date: Thu, 21 May 2026 00:46:06 -0700 Subject: [PATCH] fix(tui): address Volva code-vs-contract drift (issue #4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Volva code-review surfaced 8 findings against the TDD-passing TUI shell. All 8 addressed. Drift fixes (code): - Primary: INV-002 + INV-003 require visible Footer-area rendering of session-identity + Ctrl-C state hint. Implementation stored the strings in `self.sub_title` (which lands in the Header, not Footer) and `self.hint` (a plain attribute, never rendered). Fixed by adding two `Static` widgets (id="identity" and id="hint") in compose; the `_set_hint()` helper mirrors state into the widget on every state transition. Same-model TDD missed this because tests asserted internal state, not visible widget content. - Reverted `_stream_turn_worker(content, log)` to single-param `(content)` per the contract FN signature. The widened signature was a TDD-time workaround for a NoMatches-during-worker execution; root cause was test timing (added `await pilot.pause()` before the polling loop in `_submit_and_wait`). - Restored `exclusive=True` on `self.run_worker(...)` per the contract STEP 6 spec. - Added missing `isinstance(args, ParsedArgs)` PRE assertion to `run_tui`. Required hoisting `from ratatoskr.cli import ParsedArgs` out of TYPE_CHECKING — runtime import is fine (no circular dependency: cli lazy-imports tui inside main; tui imports cli unconditionally at module load). - Added missing union-type PRE assertion to `_render_event_to_log`. Contract amendments (precision): - COMPOSE shape: RichLog `markup=False, highlight=False` (was True, True). Explanatory comment in-line: bracketed labels like [cancel_failed] would otherwise be interpreted+stripped as Rich style spans; the post-Done Markdown rendering still works via Markdown() Renderable. - INV-002 reworded: identity rendered via dedicated Static(id="identity") widget composed adjacent to Footer (Textual's built-in Footer renders BINDINGS descriptions; a sibling Static carries custom content in the same visual region). - on_mount POST-003 amended to allow `agent_id is None` when --session is used without --agent (matches INV-002 carve-out; GET /sessions/{id} agent lookup is out of scope for this shell). - run_tui happy_returns_zero_on_quit test description clarified: App.run() is sync and can't be driven by Pilot, so run_tui's wrapping behavior is tested via monkeypatch; the piloted Ctrl-D exit path is covered separately by TestActionQuit. Test fixes: - footer_identity_visible_first_frame, footer_hint_flips_to_cancel, streaming_first_ctrl_c_cancels: now query the Static(#identity) / Static(#hint) widgets via `widget.render()` instead of asserting on `app.sub_title` / `app.hint` internal state. The internal state still exists (mirror), but the load-bearing assertion is on visible widget content. Meta-note from Volva: "TDD pass caught most stream/session/error mechanics, but tested internal state where the contract required visible Footer behavior, so same-model TDD would plausibly miss the primary drift." Calibration shape continues across all four issues: the post-TDD cross-model review consistently catches assert-boundary + observability-shape gaps the test-author's hypotheses don't cover (#1: 4 findings, #2: 3, #3: 5, #4: 8). 164/164 tests GREEN; ruff clean; both contract drift checks clean. --- docs/contracts/issues/4.contract.md | 10 +++--- persistent-memory.md | 1 + src/ratatoskr/tui.py | 49 ++++++++++++++++++++--------- tests/test_tui.py | 27 ++++++++++------ 4 files changed, 60 insertions(+), 27 deletions(-) diff --git a/docs/contracts/issues/4.contract.md b/docs/contracts/issues/4.contract.md index 5429339..3cd767e 100644 --- a/docs/contracts/issues/4.contract.md +++ b/docs/contracts/issues/4.contract.md @@ -79,7 +79,7 @@ The shell is the load-bearing primary surface. Together with `--send`, it makes ## Invariants - **INV-001 [hard]**: `ratatoskr.cli` MUST NOT import `textual` at module scope. The cli-to-tui dispatch in `main` uses a function-local `from ratatoskr.tui import run_tui` inside the branch that runs ONLY when `--send` was omitted. Verified by the existing `test_no_textual_import_in_cli` static-grep test (issue #3 INV-001), which scans `cli.py` for `import textual` / `from textual`. The `--send` path never reaches the lazy import, so the import boundary holds for scripted callers. -- **INV-002 [hard]**: The Footer widget always displays an agent slot + the LAST 8 chars of `session_id` (design-brief §4 session-identity-always-visible invariant). The format is ` · …` with a literal `·` separator and `…` prefix. The agent slot is `args.agent_id` (when `--new`), OR `SessionInfo.agent_id` (when `--new` AND a successful create_session populates it), OR the literal string `` (when `--session ` was used AND agent_id is not present in args — a `GET /sessions/{id}` lookup is out of scope for this shell, see `## Out of scope`). The `` placeholder is an ACCEPTED satisfaction of "session-identity-always-visible" — it signals to the dev that the agent is opaque from this launch but the session_id tail is still anchored. This MUST appear by the first frame after `on_mount` completes; the App MUST NOT render the chat pane in a state where the agent slot OR the session_id tail is absent. +- **INV-002 [hard]**: The visible Footer-area UI always displays an agent slot + the LAST 8 chars of `session_id` via a dedicated `Static(id="identity")` widget composed adjacent to `Footer()` (Textual's built-in Footer renders BINDINGS descriptions and doesn't naturally accept custom content; a sibling Static carries the identity string in the same visual region). The session-identity-always-visible invariant is design-brief §4. The format is ` · …` with a literal `·` separator and `…` prefix. The agent slot is `args.agent_id` (when `--new`), OR `SessionInfo.agent_id` (when `--new` AND a successful create_session populates it), OR the literal string `` (when `--session ` was used AND agent_id is not present in args — a `GET /sessions/{id}` lookup is out of scope for this shell, see `## Out of scope`). The `` placeholder is an ACCEPTED satisfaction of "session-identity-always-visible" — it signals to the dev that the agent is opaque from this launch but the session_id tail is still anchored. This MUST appear by the first frame after `on_mount` completes; the App MUST NOT render the chat pane in a state where the agent slot OR the session_id tail is absent. - **INV-003 [hard]**: Two-stage Ctrl-C state machine (design-brief §8c): - **idle state** (no turn in flight): footer hint = `"Ctrl-C twice to exit"`; first Ctrl-C → `app.exit(0)`. - **streaming state** (turn in flight): footer hint = `"Ctrl-C to cancel"`; Ctrl-C → spawn `cancel_turn` server-side, transition to **cancelling state**. @@ -159,7 +159,7 @@ STEPS: 2. [sequential, flexibility=prescriptive] Construct app = RatatoskrApp(args) 3. [sequential, flexibility=prescriptive] RETURN app.run() — Textual's sync runner; manages its own asyncio loop TESTS: - happy_returns_zero_on_quit [happy,tracer]: construct args with --session s-1; mock the SSE endpoint; Pilot presses ctrl+d immediately; run_tui returns 0 + happy_returns_zero_on_quit [happy,tracer]: construct args with --session s-1; monkeypatch RatatoskrApp.run to capture invocation and return 0; run_tui returns 0; the captured app was constructed with the passed args (verifies run_tui correctly wraps App.run). The piloted Ctrl-D exit path is covered separately by TestActionQuit.test_idle_ctrl_d_exits_zero — App.run() is sync and can't be driven by Pilot, so run_tui's wrapping behavior is tested via monkeypatch. precondition_send_content_none [adversarial]: args with send_content="x" → AssertionError before run() (PRE-001 catches the misuse) ``` @@ -179,8 +179,10 @@ BINDINGS: - ("ctrl+d", "quit", "Exit immediately") COMPOSE shape (declarative — implementer chooses CSS file vs inline): Header() - RichLog(id="transcript", wrap=True, markup=True, highlight=True) + RichLog(id="transcript", wrap=True, markup=False, highlight=False) # markup=False: bracketed labels like [cancel_failed] render verbatim instead of being interpreted-and-stripped as Rich style spans. The post-Done markdown render uses Markdown() Renderable which renders regardless of widget-level markup. Input(id="prompt", placeholder="Type a message and press Enter") + Static("", id="identity") # INV-002: visible session-identity strip; rendered by on_mount + Static(HINT_IDLE, id="hint") # INV-003: visible Ctrl-C state hint; updated on state transitions Footer() INV-WIRE-001: One AsyncClient lifecycle per app lifetime (INV-007). INV-WIRE-002: state transitions strictly idle ↔ streaming ↔ cancelling per INV-003. @@ -192,7 +194,7 @@ BRIEF: Lifecycle hook. Opens the httpx.AsyncClient, mints or attaches the sessio PRE: [PRE-001 hard] self.client is None (on_mount fires once per app instance) -- assert self.client is None POST: [POST-001 state_change] self.client is an open httpx.AsyncClient bound to args.server_url with the Bearer auth header POST: [POST-002 state_change] self.session_id is non-empty (either from args.session_id or from a successful create_session) -POST: [POST-003 state_change] self.agent_id is non-empty (from args.agent_id when --new; from SessionInfo.agent_id when --session) +POST: [POST-003 state_change] self.agent_id is non-empty when args.new (from SessionInfo.agent_id after create_session) OR self.agent_id is the value of args.agent_id when args.session is used (may be None per INV-002 carve-out — `GET /sessions/{id}` agent lookup is explicitly out of scope for this shell) POST: [POST-004 side_effect] Footer subtitle shows ` · …` (INV-002 session-identity-always-visible) POST: [POST-005 state_change] self.state == "idle"; the footer hint widget shows "Ctrl-C twice to exit" ERROR_ROUTING: diff --git a/persistent-memory.md b/persistent-memory.md index 2b24947..8a609b6 100644 --- a/persistent-memory.md +++ b/persistent-memory.md @@ -101,6 +101,7 @@ decision. Captures rationale that won't be obvious from code alone. - `[2026-05-21]` **Issue #4 + contract: `ratatoskr.tui` shell.** Textual `App[int]` subclass with single chat-pane layout (Header + RichLog + Input + Footer). Composes existing modules: `create_session` on `--new` + `stream_turn` per turn + `cancel_turn` on Ctrl-C. Hard invariants: INV-002 session-identity-always-visible footer (with `` carve-out when `--session` is used without --agent); INV-003 two-stage Ctrl-C state machine (idle/streaming/cancelling per design-brief §8c); INV-005 markdown rendering default-on with `--raw` opt-out (deliberately produces a streaming-then-Markdown-render double-display — accepted v1 trade-off; Static-then-commit refactor deferred); INV-008 mid-session errors render to transcript and return to idle (don't exit). Shell-only scope — design-brief §5's five side panes (Persona, Tools, AdminEvents, BifrostState, ServerLog), startup session picker, Tab bindings, history rendering all deferred to follow-up issues. Concurrent in-place amendment of issue #3: `--send` becomes optional (when omitted, `main` lazy-imports `ratatoskr.tui.run_tui`); `--raw` flag added to `ParsedArgs`; `_parse_args`/`main` TESTS sections updated. The lazy-import preserves issue #3 INV-001 (no textual at cli module scope). `prd:` pinned to issue #4 body SHA `b1e73e7d2e3dd453` at `2026-05-21T06:21:37+00:00`; drift check clean. - `[2026-05-21]` **Volva paraphrase round on `docs/contracts/issues/4.contract.md`.** 5 findings, all amended (matching #3's hit rate). (1) INV-002 footer carve-out for `` agent slot when `--session` and agent_id unknown; tightened wording so the `` placeholder is an accepted satisfaction of "session-identity-always-visible". (2) "Ctrl-C twice to exit" idle hint vs single-press-exits state machine: kept the hint verbatim per design-brief §8c's conservative-by-design rationale, amended INV-003 to spell out the intentional discrepancy + cite the brief so an implementer can't "fix" it by accident. (3) Markdown double-render trade-off: INV-005 strengthened from "trade-off" to explicit "assistant response visibly appears TWICE in the transcript by design"; Static-then-commit refactor explicitly out-of-scope for this shell. (4) Silent input-discard during streaming: now writes `[busy] turn in flight; input ignored` to the transcript (visible notice, not silent swallow); new test for cancelling state too. (5) `{!r:.200}` format spec note: kept (valid Python repr-then-truncate-200), added inline comment explaining the syntax. Volva meta-note: "discipline pulls weight here" — same calibration signal as prior rounds. - `[2026-05-21]` **`ratatoskr.tui` shell implemented via TDD against issue #4's contract.** 43 contract-listed tests + 4 issue-#3-amendment tests authored + GREEN per the vertical-slice ordering (`_render_event_to_log` → `_cancel_via_sse` → CLI amendments → app class + on_mount + on_unmount → on_input_submitted → _stream_turn_worker → action_interrupt + action_quit → run_tui). Two in-flight contract amendments during TDD: (a) PRE-002 of `run_tui` was `(args.session_id is None) != args.new` — backwards, fails when session_id is set + new=False; corrected to `bool(args.session_id) != bool(args.new)`; (b) RichLog created with `markup=False` (was `markup=True` in contract) because Rich interprets `[xxx]` as style spans and strips them — would break every labeled stderr-style line. The post-Done Markdown rendering still works because `rich.markdown.Markdown` is a Renderable that doesn't need widget-level markup. Implementation choices: (a) worker takes the `log` widget as a parameter (passed from on_input_submitted) — querying `#transcript` from inside a Textual worker context fails with NoMatches; (b) `_spy_writes(monkeypatch)` test helper records every RichLog.write call because RichLog's `.lines` Strip buffer isn't populated synchronously after .write() returns, making post-app-shutdown inspection unreliable; (c) SIGINT-mid-stream tests use custom `httpx.AsyncByteStream` subclasses with `asyncio.Event` gates to make timing deterministic without sleep-based polling. 164/164 tests GREEN; ruff clean (one `# noqa: RUF001,RUF003` for the intentional `❯` INV-006 prompt prefix). +- `[2026-05-21]` **Volva code-vs-contract review on `ratatoskr.tui` shell.** 8 findings — 5 drifts, 2 precision (contract-amend), 1 test-gap (follows from F1). All addressed. (1) DRIFT, primary: `self.sub_title` (Header) and `self.hint` (plain attr) were set but never rendered to a visible Footer widget — INV-002 + INV-003 require visible Footer-area rendering. Fixed: added `Static(id="identity")` + `Static(id="hint")` widgets in compose; `_set_hint()` helper mirrors state→widget; on_mount calls `query_one("#identity", Static).update(...)`. Same-model TDD missed this because tests asserted `app.sub_title` / `app.hint` internal state, not visible widget content. Volva caught immediately. (2) DRIFT: `_stream_turn_worker` signature widened to `(content, log)` during TDD as a workaround for a NoMatches-during-worker-execution test failure. Reverted to single-param `(content)` per contract; queries `#transcript` internally. The original test failure was actually a test-timing issue resolved by adding `await pilot.pause()` before the polling loop in `_submit_and_wait`. (3) DRIFT: `run_worker(coro)` missing `exclusive=True`. Restored. (4) DRIFT: `run_tui` PRE-001 missing `isinstance(args, ParsedArgs)` check; only had `args.send_content is None`. Added. Required hoisting `from ratatoskr.cli import ParsedArgs` out of `TYPE_CHECKING` (runtime import — fine, no circular dependency: cli imports tui lazily inside main; tui imports cli unconditionally at module load). (5) DRIFT: `_render_event_to_log` missing PRE-001 union assertion. Added. (6) PRECISION (amend contract): RichLog `markup=True, highlight=True` in contract; implementation chose `markup=False, highlight=False` to preserve labeled-line semantics. Contract amended with rationale comment in COMPOSE shape. (7) PRECISION (amend contract): `on_mount` POST-003 said `agent_id is non-empty`; for `--session` attach without `--agent` it's None per INV-002 carve-out. Contract amended. (8) TEST-GAP: tests checked `app.sub_title` / `app.hint`; updated to query `Static(#identity).render()` / `Static(#hint).render()` (the actual visible widgets). Volva meta-note: "TDD pass caught most stream/session/error mechanics, but tested internal state where the contract required visible Footer behavior, so same-model TDD would plausibly miss the primary drift." Calibration evidence: cross-model review pulls weight on UI-observability gaps in particular, where the test-author can convince themselves the state is correct without verifying the user can see it. 164 tests GREEN post-fix. ## Tried and abandoned diff --git a/src/ratatoskr/tui.py b/src/ratatoskr/tui.py index f597adc..fbd9edb 100644 --- a/src/ratatoskr/tui.py +++ b/src/ratatoskr/tui.py @@ -5,13 +5,14 @@ Implements docs/contracts/issues/4.contract.md. from __future__ import annotations -from typing import TYPE_CHECKING, ClassVar, Literal +from typing import ClassVar, Literal import httpx from textual.app import App, ComposeResult from textual.binding import Binding -from textual.widgets import Footer, Header, Input, RichLog +from textual.widgets import Footer, Header, Input, RichLog, Static +from ratatoskr.cli import ParsedArgs from ratatoskr.sessions import AgentNotFound, SessionApiFailed, create_session from ratatoskr.sse_client import ( CancelAlreadyCompleted, @@ -35,12 +36,13 @@ from ratatoskr.sse_client import ( stream_turn, ) -if TYPE_CHECKING: - from ratatoskr.cli import ParsedArgs - def _render_event_to_log(event: Event, *, log: RichLog, raw: bool) -> None: """Pure event-to-RichLog renderer per the contract STEPS table.""" + assert isinstance( + event, + (WorkerPhase, Thinking, Text, TextBoundary, ToolStart, ToolResult, Done, Error, Cancelled), + ) if isinstance(event, Text): log.write(event.content) elif isinstance(event, Done): @@ -104,6 +106,12 @@ class RatatoskrApp(App[int]): # is a Rich Renderable and renders correctly without widget-level markup=True. yield RichLog(id="transcript", wrap=True, markup=False, highlight=False) yield Input(id="prompt", placeholder="Type a message and press Enter") + # INV-002 + INV-003: visible identity + hint widgets (Footer-area rendering). + # Textual's built-in Footer renders BINDINGS descriptions; these Static widgets + # carry the session-identity and Ctrl-C-state strings the contract requires be + # always-visible. + yield Static("", id="identity") + yield Static(self.HINT_IDLE, id="hint") yield Footer() async def on_mount(self) -> None: @@ -138,9 +146,20 @@ class RatatoskrApp(App[int]): self.agent_id = self.args.agent_id # may be None — INV-002 carve-out agent_slot = self.agent_id or "" - self.sub_title = f"{agent_slot} · …{self.session_id[-8:]}" + identity = f"{agent_slot} · …{self.session_id[-8:]}" + self.sub_title = identity # mirror to Header subtitle for redundancy + self.query_one("#identity", Static).update(identity) self.state = "idle" - self.hint = self.HINT_IDLE + self._set_hint(self.HINT_IDLE) + + def _set_hint(self, hint: str) -> None: + """Set the hint state attribute AND update the visible Static widget.""" + self.hint = hint + try: + self.query_one("#hint", Static).update(hint) + except Exception: + # Widget may be gone during shutdown; ignore. + pass async def on_input_submitted(self, event: Input.Submitted) -> None: """Echo user prompt, spawn stream worker; busy notice if not idle.""" @@ -157,16 +176,17 @@ class RatatoskrApp(App[int]): log.write(f"❯ {content}") # noqa: RUF001 — intentional INV-006 prefix event.input.value = "" self.state = "streaming" - self.hint = self.HINT_STREAMING + self._set_hint(self.HINT_STREAMING) self.stream_worker = self.run_worker( - self._stream_turn_worker(content, log) + self._stream_turn_worker(content), exclusive=True ) - async def _stream_turn_worker(self, content: str, log: RichLog) -> None: + async def _stream_turn_worker(self, content: str) -> None: """Drive stream_turn, render events, set active_turn_id, restore idle on terminal/error.""" assert self.state == "streaming" assert self.client is not None assert content + log = self.query_one("#transcript", RichLog) try: async for event in stream_turn(self.client, self.session_id, content): if self.active_turn_id is None: @@ -193,7 +213,7 @@ class RatatoskrApp(App[int]): finally: self.state = "idle" self.active_turn_id = None - self.hint = self.HINT_IDLE + self._set_hint(self.HINT_IDLE) async def on_unmount(self) -> None: """Close the httpx.AsyncClient cleanly.""" @@ -212,7 +232,7 @@ class RatatoskrApp(App[int]): self.exit(3) return self.state = "cancelling" - self.hint = self.HINT_CANCELLING + self._set_hint(self.HINT_CANCELLING) log = self.query_one("#transcript", RichLog) self.run_worker( _cancel_via_sse(self.client, self.session_id, self.active_turn_id, log=log) @@ -231,8 +251,9 @@ class RatatoskrApp(App[int]): def run_tui(args: ParsedArgs) -> int: """Sync entry point — wraps App.run(). Returns the exit code from App.run().""" - assert args.send_content is None - # Exactly one of session_id / new must be set (xor) + # PRE-001: TUI-mode marker (issue #4 contract) + assert isinstance(args, ParsedArgs) and args.send_content is None + # PRE-002: Exactly one of session_id / new must be set (xor) assert bool(args.session_id) != bool(args.new) app = RatatoskrApp(args) return app.run() or 0 diff --git a/tests/test_tui.py b/tests/test_tui.py index d2be35f..a566604 100644 --- a/tests/test_tui.py +++ b/tests/test_tui.py @@ -306,16 +306,19 @@ class TestAppMount: @respx.mock async def test_footer_identity_visible_first_frame(self) -> None: """footer_identity_visible_first_frame [trace]: …""" + from textual.widgets import Static + respx.post("https://w.example/sessions").mock( return_value=httpx.Response(201, json=_CREATE_OK_RESP) ) app = RatatoskrApp(_args_new()) async with app.run_test() as pilot: await pilot.pause() - # Both halves of the identity present - assert "mimir" in (app.sub_title or "") - assert "·" in (app.sub_title or "") - assert app.session_id[-8:] in (app.sub_title or "") + identity_widget = app.query_one("#identity", Static) + rendered = str(identity_widget.render()) + assert "mimir" in rendered + assert "·" in rendered + assert app.session_id[-8:] in rendered @respx.mock async def test_client_open_after_mount(self) -> None: @@ -349,7 +352,7 @@ import asyncio # noqa: E402 from textual.widgets import Input # noqa: E402 -async def _noop_worker(self, content: str, log) -> None: +async def _noop_worker(self, content: str) -> None: """Fake _stream_turn_worker that never completes (lets state stay 'streaming').""" await asyncio.Future() # await forever; cancelled when test exits @@ -439,17 +442,20 @@ class TestOnInputSubmitted: async def test_footer_hint_flips_to_cancel( self, monkeypatch: pytest.MonkeyPatch ) -> None: - """footer_hint_flips_to_cancel [trace]: after submit, hint shows 'Ctrl-C to cancel'.""" + """footer_hint_flips_to_cancel [trace]: hint widget shows 'Ctrl-C to cancel'.""" + from textual.widgets import Static + monkeypatch.setattr(RatatoskrApp, "_stream_turn_worker", _noop_worker) app = RatatoskrApp(_args_existing()) async with app.run_test() as pilot: await pilot.pause() - assert app.hint == RatatoskrApp.HINT_IDLE + hint_widget = app.query_one("#hint", Static) + assert str(hint_widget.render()) == RatatoskrApp.HINT_IDLE inp = app.query_one("#prompt", Input) inp.value = "hi" await inp.action_submit() await pilot.pause() - assert app.hint == RatatoskrApp.HINT_STREAMING + assert str(hint_widget.render()) == RatatoskrApp.HINT_STREAMING import json # noqa: E402 @@ -798,7 +804,10 @@ class TestActionInterrupt: await pilot.pause(0.02) assert cancel_route.call_count == 1 assert app.state == "cancelling" - assert app.hint == RatatoskrApp.HINT_CANCELLING + from textual.widgets import Static + + hint_widget = app.query_one("#hint", Static) + assert str(hint_widget.render()) == RatatoskrApp.HINT_CANCELLING # Release the gate so the stream worker can finish cleanly during teardown gate.set()