fix(cli): address Volva code-vs-contract drift (issue #3)

Volva code-review surfaced 5 findings against the TDD-passing
implementation; all 5 addressed.

Drift fixes (code):
- Add `assert argv is None or all(isinstance(a, str) for a in argv)`
  at both `main` and `_parse_args` entry points (PRE-001 was unenforced).
- `main` now catches `SystemExit` and returns `exc.code` verbatim —
  argparse's --help (SystemExit(0)) was escaping through main as an
  unhandled exception. Contract amended in-place to spell out the
  SystemExit-from-argparse-clean-exits passthrough in both
  `main` and `_parse_args` ERROR_ROUTING. New `help_exits_cleanly`
  test added per the contract amendment.
- Add the PRE-001 union-type assert at `_render_event` entry —
  unmatched Event variants would have silently no-op'd.
- `_run_turn` now awaits `cancel_task` in the `finally` block before
  returning. Under fast-stream + slow-cancel scenarios the
  `[cancel_failed]` line could miss being written before _run_turn
  returns, AND _amain could close the AsyncClient while the cancel
  POST was still in flight. `_cancel_and_log` swallows all errors
  per INV-009 so the await is safe.

Test gap fix:
- New `_FlushCountingIO` subclass counts flush() calls;
  `test_text_to_stdout_only` and `test_done_writes_newline_and_label`
  now assert `flush_count == 1` to verify INV-010 (per-chunk flush).
  Previously the tests would have passed even with flush removed.

Meta-note carried in persistent-memory: TDD caught central behavior
(stdout/stderr routing, exit-code mapping, create-session ordering,
SIGINT idempotence); the cross-model code review consistently catches
assert-boundary + observability-shape gaps across all three issues
(#1: 4 findings, #2: 3 findings, #3: 5 findings).

118/118 tests GREEN; ruff clean; drift check clean.
This commit is contained in:
2026-05-20 22:59:26 -07:00
parent db27774c51
commit 9717fb80e2
4 changed files with 64 additions and 9 deletions
+2 -1
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.** 117/117 tests GREEN (42 sse_client + 19 sessions + 54 cli + 1 boundary + 1 metadata); ruff clean.
**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.
What's in the repo:
- `docs/design-brief.md` — the locked design (copy from `brokkr-smithy/docs/ratatoskr-design-brief.md`).
@@ -93,6 +93,7 @@ decision. Captures rationale that won't be obvious from code alone.
- `[2026-05-21]` **Issue #3 + contract: `ratatoskr.cli --send` non-interactive stdout presenter.** Composes `create_session` (when `--new`) with `stream_turn` + `cancel_turn` into a one-shot CLI. Hard invariants: no `textual` / `rich` imports (raw stdout — `--raw` is TUI-only per design-brief §6); stdout for `Text` deltas + the post-`Done` newline ONLY; everything else labeled to stderr. Six FN blocks (`main`, `_parse_args`, `_amain`, `_render_event`, `_run_turn`, `_cancel_and_log`). The `_run_turn` race-loop is the load-bearing piece: races `__anext__` against `sigint_event.wait()` so a mid-stream SIGINT lands within one event boundary; once cancel is in flight, the race-loop stops creating new `wait()` tasks (the no-busy-loop fix Volva flagged). 9-bucket exit-code table (0/2/3 terminal; 10/11/12 usage/auth/agent; 20/21/22 server/network/protocol). `prd:` pinned to issue #3 body SHA `206ef51709d43b2c` at `2026-05-21T05:13:29+00:00`; first contract in the repo with a code-level `dependencies:` block (issues #1 and #2).
- `[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.
## Tried and abandoned