fix(tui): address Volva code-vs-contract drift (issue #4)

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.
This commit is contained in:
vh
2026-05-21 00:46:06 -07:00
parent dd89239c34
commit 942e33898c
4 changed files with 60 additions and 27 deletions
+1
View File
@@ -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 `<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).
- `[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