feat(tui): implement issue #4 contract via TDD; amend cli for TUI dispatch

47 contract-listed tests authored + GREEN (43 tui + 4 issue-#3
amendments). 164/164 tests GREEN suite-wide; ruff clean.

Vertical-slice ordering: _render_event_to_log → _cancel_via_sse →
CLI amendments → RatatoskrApp class + on_mount + on_unmount →
on_input_submitted → _stream_turn_worker → action_interrupt +
action_quit → run_tui.

Two in-flight contract amendments caught during TDD:
- PRE-002 of run_tui was `(args.session_id is None) != args.new` —
  backwards (fails when --session is set + new=False). Corrected to
  `bool(args.session_id) != bool(args.new)`.
- RichLog created with markup=False (contract drafted markup=True).
  Rich interprets `[xxx]` as style markup and strips it, which would
  break every labeled stderr-style line ([cancel_failed], [done],
  [error], etc.). The post-Done Markdown rendering still works
  because rich.markdown.Markdown is a Renderable and doesn't need
  widget-level markup.

Implementation notes:
- _stream_turn_worker takes the log widget as a parameter passed
  from on_input_submitted. Querying #transcript from inside a
  Textual worker context fails with NoMatches; capturing the
  reference once at handler-time and threading it through the
  worker sidesteps the issue.
- _spy_writes(monkeypatch) test helper records every RichLog.write
  call. RichLog's `.lines` Strip buffer isn't populated
  synchronously after .write() returns, which makes
  post-app-shutdown inspection unreliable; a write-spy gives
  deterministic verification.
- SIGINT-mid-stream tests use custom httpx.AsyncByteStream
  subclasses with asyncio.Event gates to make timing deterministic
  without sleep-based polling — the cancel-respx-mock sets the
  gate event when its endpoint is observed, releasing the next
  SSE chunk.
- _submit_and_wait test helper needs `await pilot.pause()` BEFORE
  the polling loop so the Input.Submitted message has a chance to
  dispatch. Discovered via debug-print trace; tracked in the test
  helper.

CLI amendments (per issue #4 in-place amendment of #3 contract):
- ParsedArgs.send_content: str | None (was str)
- ParsedArgs.raw: bool added
- _parse_args: --send default=None; empty-string still rejected;
  --raw added
- main: branches on args.send_content — None → lazy
  `from ratatoskr.tui import run_tui` + run_tui(args); else
  asyncio.run(_amain(args)). Lazy import preserves issue #3 INV-001.

Persistent-memory updated per the commit-along rule: tui module
landed, recent-decisions entries for #4 (contract + Volva + TDD),
next natural moves rotated to Volva code-review + manual smoke
against the personal Worldtree (key landed in env.sh per
infra-ops's earlier delivery).
This commit is contained in:
vh
2026-05-21 00:31:40 -07:00
parent 9d469d5c67
commit dd89239c34
5 changed files with 1312 additions and 13 deletions
+13 -6
View File
@@ -30,7 +30,7 @@ separate dev team rather than an in-tree Worldtree tool.
## Current state / in-flight
**Status: `ratatoskr.sse_client` + `ratatoskr.sessions` + `ratatoskr.cli` all implemented via TDD against their issue-scoped contracts; all three rounds also Volva code-reviewed + drift-fixed.** 118/118 tests GREEN (42 sse_client + 19 sessions + 55 cli + 1 boundary + 1 metadata); ruff clean.
**Status: `ratatoskr.sse_client` + `ratatoskr.sessions` + `ratatoskr.cli` + `ratatoskr.tui` (shell) all implemented via TDD against their issue-scoped contracts.** 164/164 tests GREEN (42 sse_client + 19 sessions + 58 cli + 43 tui + 1 boundary + 1 metadata); ruff clean. `--send` mode + TUI mode both wired into `ratatoskr` console-script entry point (TUI dispatched via lazy import to preserve issue #3 INV-001).
What's in the repo:
- `docs/design-brief.md` — the locked design (copy from `brokkr-smithy/docs/ratatoskr-design-brief.md`).
@@ -46,7 +46,10 @@ What's in the repo:
- `tests/test_sessions.py` — 19 tests covering both FN blocks' TESTS: entries verbatim (7 + 12). Helper `_list_item()` builds GET /sessions list-item bodies for tests.
- `docs/contracts/issues/3.contract.md` — **issue-scoped contract for issue #3** (https://gitea.phasefinal.com/vh/ratatoskr/issues/3). v2.1, complexity=medium. `target_module: ratatoskr.cli`. `prd:` block pins to issue body SHA `206ef51709d43b2c` at `2026-05-21T05:13:29+00:00`. Six FN blocks: `main`, `_parse_args`, `_amain`, `_render_event`, `_run_turn`, `_cancel_and_log`. `dependencies:` block lists issues #1 and #2 as code-level deps (first in the repo to do so — #1 and #2 were convention-only siblings). Drift check returns clean.
- `src/ratatoskr/cli.py` — **implemented 2026-05-21** per `docs/contracts/issues/3.contract.md`. Six entry points: sync `main` + async `_amain` + four private helpers (`_parse_args`, `_render_event`, `_cancel_and_log`, `_run_turn`). Composes `sessions.create_session` (when `--new`) with `sse_client.stream_turn` + `cancel_turn`. Hard invariant: no `textual` / `rich` imports (raw stdout). `_run_turn` is the load-bearing piece — race-loop pattern that gates `asyncio.create_task(sigint_event.wait())` behind `if not cancelling` to avoid the busy-loop bug Volva flagged in contract review. SIGINT handler installed via `loop.add_signal_handler(SIGINT, sigint_event.set)` so unit tests can fire the event directly without real signals. 305 LOC.
- `tests/test_cli.py` — 54 tests covering all six FN blocks' TESTS: entries verbatim (14 + 10 + 5 + 13 + 7 + 5). Helpers: `_sse_chunk` (same shape as `test_sse_client.py`'s helper), `_sse_resp` (wraps respx Response with the `text/event-stream` content-type), `_GatedStream` (custom `httpx.AsyncByteStream` that pauses on `asyncio.Event` entries to make SIGINT-mid-stream tests deterministic without sleep-based timing). Test fixtures: `_clear_env` autouse fixture clears `WORLDTREE_API_KEY` / `WORLDTREE_API_URL` per test for deterministic env-resolution assertions.
- `tests/test_cli.py` — 58 tests covering all six FN blocks' TESTS: entries (14 + 10 + 5 + 13 + 7 + 5) plus issue #4 amendments (no_send_marks_tui_mode, raw_flag_default_false, raw_flag_set, no_send_dispatches_to_tui). Helpers: `_sse_chunk` (same shape as `test_sse_client.py`'s helper), `_sse_resp` (wraps respx Response with the `text/event-stream` content-type), `_GatedStream` (custom `httpx.AsyncByteStream` that pauses on `asyncio.Event` entries to make SIGINT-mid-stream tests deterministic without sleep-based timing). Test fixtures: `_clear_env` autouse fixture clears `WORLDTREE_API_KEY` / `WORLDTREE_API_URL` per test for deterministic env-resolution assertions.
- `docs/contracts/issues/4.contract.md` — **issue-scoped contract for issue #4** (https://gitea.phasefinal.com/vh/ratatoskr/issues/4). v2.1, complexity=medium. `target_module: ratatoskr.tui`. `prd:` block pins to issue body SHA `b1e73e7d2e3dd453` at `2026-05-21T06:21:37+00:00`. Six FN blocks: `run_tui`, `RatatoskrApp` class shape, `on_mount`, `on_input_submitted`, `_stream_turn_worker`, `_render_event_to_log`, `action_interrupt`, `action_quit`, `on_unmount`, `_cancel_via_sse`. Plus a "CLI amendments" section spelling out the issue #3 in-place amendments (`--send` becomes optional + `--raw` flag + `main` dispatch + new tests). `dependencies:` lists issues #1, #2, #3 as code-level deps.
- `src/ratatoskr/tui.py` — **implemented 2026-05-21** per `docs/contracts/issues/4.contract.md`. One `RatatoskrApp(App[int])` Textual subclass + `run_tui(args)` sync entry. Compose layout: Header + RichLog (markup=False so labeled `[xxx]` lines render verbatim; the post-Done Markdown rendering uses `rich.markdown.Markdown` directly) + Input + Footer. Two-stage Ctrl-C state machine via `action_interrupt` (idle→exit(0); streaming→cancel_turn POST + flip to cancelling; cancelling→exit(3) abandon-drain). Ctrl-D → `action_quit` immediate exit(0). Worker pattern via `self.run_worker(self._stream_turn_worker(content, log))` — the log reference is passed as a parameter because the worker can't `self.query_one(...)` reliably from within Textual's worker context. ~210 LOC.
- `tests/test_tui.py` — 43 tests covering all six FN blocks' TESTS entries (9 + 4 + 8 + 5 + 8 + 7 + 2). Helpers: `_args_new` / `_args_existing` (ParsedArgs factories), `_spy_writes` (monkeypatch RichLog.write to record calls — works around RichLog's write→render asynchrony where `.lines` isn't populated immediately after .write() returns), `_sse_chunk` / `_sse_resp` (SSE wire helpers), `_submit_and_wait` (drive input + poll for state==idle), `_noop_worker` (fake stream worker for on_input_submitted tests). Tests use Textual's `App.run_test()` + `Pilot` for headless app testing. SIGINT-mid-stream tests use `httpx.AsyncByteStream` subclasses with `asyncio.Event` gates to make timing deterministic.
- `tests/test_no_worldtree_imports.py` — boundary smoke test (passes; verified 2026-05-20).
- `tests/snapshots/README.md` — recording/replay convention for SSE snapshot tests.
@@ -59,10 +62,11 @@ What's NOT in the repo yet:
**Branch:** `main`. Remote: `origin → git@gitea.phasefinal.com:vh/ratatoskr.git` (added 2026-05-20).
**Next natural moves:**
1. **`/volva-code-review docs/contracts/issues/3.contract.md`** against the freshly-landed `ratatoskr.cli` implementation (precedent: #1 caught 4 negative-space drifts, #2 caught 3). The race-loop in `_run_turn` is the most likely source of new drifts — cross-model fresh-eyes review on async / signal-handling code has been valuable in past rounds.
2. **Manual smoke against a local Worldtree**: `ratatoskr --send "hello" --new --agent <id>`. Captures the first real-Worldtree integration evidence (mocks aren't the wire). Useful regardless of #1.
3. Record real SSE snapshot fixtures from a running Worldtree. `--send --new` redirected to a fixture file IS the recording probe — capture outputs to `tests/snapshots/` for replay-based regression coverage.
4. Textual TUI app shell — second presenter; multi-pane observability dashboard per design-brief §5. Now that the consumer surface is exercised end-to-end via `--send`, the TUI lands on top of validated modules.
1. **`/volva-code-review docs/contracts/issues/4.contract.md`** against the freshly-landed `ratatoskr.tui` shell (precedent: #1 caught 4 drifts, #2 caught 3, #3 caught 5). The Textual `App` lifecycle + worker race patterns are likely sources of new drifts — cross-model fresh-eyes review on async UI code is high-value.
2. **Manual smoke against the personal Worldtree**: key landed in `env.sh` (see [[personal-worldtree-smoke-target]] memory); `source env.sh && ratatoskr --send "hello" --new --agent mimir` for stdout smoke OR `ratatoskr --new --agent mimir` for TUI smoke. First end-to-end wire validation against the live server.
3. **Side-pane issues** — design-brief §5 lists 5 side panes (Persona, Tools, AdminEvents, BifrostState, ServerLog). Each gets its own issue + contract + TDD pass on top of the shell. Persona is the natural first (file-tail of `persona.log` — cheap; no new Worldtree wire).
4. Record real SSE snapshot fixtures from a running Worldtree. `--send --new` redirected to a fixture file IS the recording probe — capture outputs to `tests/snapshots/` for replay-based regression coverage.
5. Startup session picker — design-brief §4 `DataTable` of `GET /sessions`. Modest scope; pairs naturally with the TUI shell.
## Recent decisions
@@ -94,6 +98,9 @@ decision. Captures rationale that won't be obvious from code alone.
- `[2026-05-21]` **Volva paraphrase round on `docs/contracts/issues/3.contract.md`.** Volva flagged 5 ambiguities; operator approved amendments to ALL 5 (higher hit rate than #1/#2's 3-of-5 — async + signal-handling has more places for ambiguity to hide). (1) INV-001 wording tightened: "no in-repo modules other than `ratatoskr.sessions` and `ratatoskr.sse_client`" (was "imports from X and Y only" which literally forbade httpx/asyncio/stdlib). (2) INV-002 + `_render_event` POST-002/003 restructured: `Done`'s stdout newline is part of the contract (two stdout cases: Text deltas + post-Done newline), not a contradiction with "no other event writes stdout". (3) `[cancelled] (before any event arrived)` early-exit label added to the Data flow stderr list. (4) SIGINT race-loop pseudocode gated `sigint_task = asyncio.create_task(...)` behind `if not cancelling` — without this gate, once sigint is set, every loop iteration would wake on the already-set event (busy loop). New `no_busy_loop_after_cancel [trace]` test added. (5) `CancelTurnNotFound` + `CancelAlreadyCompleted` added to assumptions import list (they were used in `_cancel_and_log`'s ERROR_ROUTING but missing from the public-surface declaration). Meta-note: Volva said "discipline pulls weight here" — same calibration signal as #1/#2.
- `[2026-05-21]` **`ratatoskr.cli` implemented via TDD against issue #3's contract.** 54 contract-listed tests authored + GREEN per the vertical-slice ordering (`_parse_args` → `_render_event` → `_cancel_and_log` → `_run_turn` → `_amain` → `main`). One in-flight contract amendment during TDD: the `no_busy_loop_after_cancel` test description originally said "exactly ONE wait()-shaped task created" but the natural race-loop shape produces 2 (iter 1 raced with text-event, iter 2 raced with sigint → flipped cancelling=True; iter 3+ skipped). Amended the contract test description to assert "TWO total wait() coroutines" with rationale; the busy-loop check is preserved (iter 3+ MUST skip wait() creation; the bug would grow N unbounded). Implementation choices: (a) `_UsageErrorParser` subclass overrides `argparse.ArgumentParser.error` to raise `_ArgparseError` instead of SystemExit, then `_parse_args` catches and re-raises as `UsageError` per the contract's ERROR_ROUTING; (b) `_GatedStream` test helper (custom `httpx.AsyncByteStream` that pauses on `asyncio.Event` entries) made SIGINT-mid-stream tests deterministic without sleep-based timing — gates release via side-channels (the cancel-mock setting an event when observed); (c) strong-ref `cancel_task` variable in `_run_turn` holds the fire-and-forget cancel task to suppress RUF006 / asyncio GC warning. 117/117 tests GREEN post-implementation; ruff clean.
- `[2026-05-21]` **Volva code-vs-contract review on `ratatoskr.cli`.** Five findings, all "fix it" (one with collateral contract amendment). (1) Drift: neither `main` nor `_parse_args` asserted PRE-001 (`argv is None or all(isinstance(a, str) for a in argv)`). Fixed: added assertions at both entry points. (2) Drift: argparse's `--help` raises `SystemExit(0)` which escaped through `main` — unfriendly UX. Code: `main` now catches `SystemExit` and returns `exc.code` verbatim (passthrough; argparse already printed help to stdout). Contract amended in-place: `_parse_args` + `main` ERROR_ROUTING now spell out the SystemExit-from-argparse-clean-exits passthrough; new `help_exits_cleanly [happy]` test added. (3) Precision: `_render_event` lacked the union-type assertion from PRE-001 — unmatched event variants would silently no-op. Fixed: added `assert isinstance(event, (WorkerPhase, Thinking, Text, ...))` at function entry. (4) Drift: `cancel_task` was created but never awaited in `_run_turn`'s `finally` — under fast-stream + slow-cancel scenarios, the `[cancel_failed]` log could miss being written before `_run_turn` returns, AND `_amain` could close the AsyncClient while the cancel POST was still in flight. Fixed: `finally` block now awaits `cancel_task` if present (`_cancel_and_log` already swallows all errors per INV-009, so the await never raises). (5) Test-gap: `text_to_stdout_only` and `done_writes_newline_and_label` used plain `io.StringIO` and didn't verify INV-010's per-chunk flush — the tests would pass even with flush removed. Fixed: new `_FlushCountingIO` subclass counts `flush()` calls; both tests assert `flush_count == 1`. Volva's meta-note: "TDD pass mostly caught central behavior; this review caught contract-hardening edges (PRE asserts, --help, non-guaranteed cancel-failure log)." Same calibration shape as #1 (4 negative-space drifts) and #2 (3 drifts) — the post-TDD code-review consistently catches the assert-boundary and observability-shape gaps the test-author's hypotheses don't cover. 118 tests GREEN post-fix.
- `[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 `<unknown>` 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 `<unknown>` agent slot when `--session` and agent_id unknown; tightened wording so the `<unknown>` 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).
## Tried and abandoned