feat(cli): implement issue #3 contract via TDD
54 contract-listed tests authored + GREEN per the vertical-slice ordering (_parse_args → _render_event → _cancel_and_log → _run_turn → _amain → main). 117/117 tests GREEN suite-wide; ruff clean. The _run_turn race-loop is the load-bearing piece. Per iteration, the await on the next event is raced against sigint_event.wait() when NOT cancelling. Once SIGINT fires (with last_turn_id known), _cancel_and_log is spawned, cancelling=True flips, and subsequent iterations skip wait()-task creation entirely — the bug Volva flagged in contract review would otherwise busy-wake on the already-set event each iteration. Implementation notes: - _UsageErrorParser subclasses argparse.ArgumentParser and overrides error() to raise _ArgparseError instead of calling sys.exit; _parse_args catches and re-raises as UsageError per the contract's ERROR_ROUTING. - _GatedStream test helper (custom httpx.AsyncByteStream that pauses on asyncio.Event entries) makes SIGINT-mid-stream tests deterministic without sleep-based timing — gates release via side-channels (the cancel-mock sets an event when its endpoint is observed). - _sse_resp test helper wraps respx Response with the text/event-stream content-type, dedupes the boilerplate across the 13 _run_turn tests. - Strong-ref cancel_task local in _run_turn holds the fire-and-forget cancel task to suppress RUF006 / asyncio GC warning. One in-flight contract amendment during TDD: no_busy_loop_after_cancel test description originally said "exactly ONE wait()-shaped task" but the natural race-loop shape produces 2 (iter 1 raced w/ text, iter 2 raced w/ sigint → flipped cancelling; iter 3+ skipped). Amended to "TWO total wait() coroutines" with rationale; the busy-loop check is preserved (iter 3+ MUST skip). Persistent-memory updated per the commit-along rule: new module landed, recent-decisions log entries for #3 (contract + Volva paraphrase + TDD), next natural moves rotated to /volva-code-review on the implementation.
This commit is contained in:
+11
-5
@@ -30,7 +30,7 @@ separate dev team rather than an in-tree Worldtree tool.
|
||||
|
||||
## Current state / in-flight
|
||||
|
||||
**Status: `ratatoskr.sse_client` + `ratatoskr.sessions` both implemented via TDD against their issue-scoped contracts.** 62/62 tests GREEN (42 sse_client + 19 sessions + 1 boundary); ruff clean.
|
||||
**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.
|
||||
|
||||
What's in the repo:
|
||||
- `docs/design-brief.md` — the locked design (copy from `brokkr-smithy/docs/ratatoskr-design-brief.md`).
|
||||
@@ -44,6 +44,9 @@ What's in the repo:
|
||||
- `tests/test_sse_client.py` — 37 tests covering all four FN blocks' TESTS: entries verbatim (13 + 10 + 8 + 6). Real HTTP wire via respx mocks; SSE wire format constructed by helper `_sse_chunk`. Connection-drop test uses custom `httpx.AsyncByteStream` subclass that yields chunks then raises `RemoteProtocolError`.
|
||||
- `src/ratatoskr/sessions.py` — **implemented 2026-05-21** per `docs/contracts/issues/2.contract.md`. Two functions (`create_session`, `list_sessions`) + two frozen dataclasses (`SessionInfo`, `SessionPage`) + three exception types (`AgentNotFound`, `InvalidCursor`, `SessionApiFailed`). `SessionInfo` uses origin-conditional defaults per INV-001/INV-002 (create-origin: `name=None`, `archived=False`, `tags=[]`, `message_count=<from body>`; list-origin: same defaults for absent/null fields, `message_count=None`). `SessionApiFailed` truncates `.body` to ≤1024 at construction. No code shared with `sse_client.py` (convention-dependency only per issue #2 `dependencies:`).
|
||||
- `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_no_worldtree_imports.py` — boundary smoke test (passes; verified 2026-05-20).
|
||||
- `tests/snapshots/README.md` — recording/replay convention for SSE snapshot tests.
|
||||
|
||||
@@ -56,10 +59,10 @@ 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. Optional: `/volva-code-review docs/contracts/issues/2.contract.md` against the freshly-landed implementation (precedent: issue #1's code-review caught 4 negative-space drifts the TDD round missed).
|
||||
2. Build the `--send` stdout presenter under `ratatoskr.cli` — composes `create_session` + `stream_turn` into the non-interactive mode (design-brief §8b). First chance to exercise both modules against a real Worldtree.
|
||||
3. Record real SSE snapshot fixtures from a running Worldtree. `--send --new` is itself a recording probe — capture its outputs to `tests/snapshots/` for replay-based regression coverage.
|
||||
4. Textual TUI app shell — second presenter; multi-pane observability dashboard per design-brief §5.
|
||||
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.
|
||||
|
||||
## Recent decisions
|
||||
|
||||
@@ -87,6 +90,9 @@ decision. Captures rationale that won't be obvious from code alone.
|
||||
- `[2026-05-21]` **Volva paraphrase round on `docs/contracts/issues/2.contract.md`.** Volva flagged 5 ambiguities; operator approved amendments to 3 of them. (1) `tags`/`archived`/`name` defaulting semantics now explicit: `tags: list[str]` (default `[]`), `archived: bool` (default `False`), `name: str | None` (default `None`); INV-001/INV-002 + STEPS aligned. (2) `include_archived_query` test tightened: default → URL has NO `include_archived` param at all (was "no param OR explicit false" — softened the assertion against STEP 2's prescriptive behavior). (5) `metadata` populated-vs-defaulted slippage resolved: INV-001 + INV-002 now spell out the defensive `body.get("metadata", {})` default for spec drift tolerance. Volva flags #3 (exception `.body` sensitivity) and #4 (`assert` for runtime validation) reviewed and kept as-is — both intentional and consistent with issue #1's precedent. Drift check still clean (amendments don't touch the pinned issue body).
|
||||
- `[2026-05-21]` **Volva code-vs-contract review round on `ratatoskr.sse_client`.** Volva flagged 4 findings (3 drifts + 1 test-gap), all code-side "fix it" recommendations: (1) `_iter_events` fell off cleanly on EOF before terminal, violating INV-001 ("MUST NOT raise StopAsyncIteration before a terminal event arrives unless connection drops"); fix tracks `terminal_seen` flag and raises `SseConnectionDropped` on clean-EOF-without-terminal. (2) Both `SseConnectFailed.body` and `CancelFailed.body` stored full response bytes; ERROR_ROUTING specified truncation to `[:1024]`; fix truncates in `__init__` before storing. (3) `_parse_sse_id` PRE-001 specified `assert isinstance(raw, str)`, but code called `.split(":")` directly (incidental `AttributeError` on non-str); fix adds the assert. (4) Test-gap on cancel_turn's "other status → CancelFailed" branch; fix adds a 503 test with >1024-byte body that double-covers finding #2. Meta-note: Volva said TDD caught the main happy/adversarial shape; the misses were "negative space" cases (clean EOF, exception payload truncation, untested generic cancel branch) — calibration evidence that cross-model review pulls weight on the same-model author's blind spots. 43 tests GREEN post-fix (42 sse_client + 1 boundary), ruff clean.
|
||||
- `[2026-05-21]` **Volva paraphrase round on `docs/contracts/issues/1.contract.md`.** Volva flagged 5 ambiguities; operator approved amendments to 3 of them. (1) `reconnect_turn` STEP 2 punt resolved: signature now carries `content: str`; STEP 2 body is `json={"content": content}` matching spec §Reconnect flow example verbatim. Spec line 732 makes the agent's tools+LLM run "exactly once regardless of disconnects/reconnects" — the `content` is a wire-schema requirement, not re-processed server-side. (2) `_parse_sse_id` tightened: `turn_id ≥ 1` AND `seq ≥ 1` (was `≥ 0`); spec §SSE id format line 705 explicitly states `seq` starts at 1, and `turn_id` is SQLite autoincrement (≥1). Test `happy_zero_seq` flipped to `zero_seq [adversarial]`; new `zero_turn_id` + `negative_seq` adversarial tests added. (3) INV-003 clarified to spell out the two-entry-point semantics: `stream_turn` establishes `turn_id` from the first event (first event always yields); `reconnect_turn` parses the expected `turn_id` FROM `last_event_id` BEFORE the connection opens, so the first server event is already a flip-candidate and is NOT yielded on mismatch. Volva flags #3 (MalformedSseId-vs-ValueError split) and #5 (exactly-one-terminal as server-assumed) noted but kept as-is — deliberate distinctions. Drift check still clean against issue #1 (amending the contract doesn't touch the pinned issue body).
|
||||
- `[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.
|
||||
|
||||
## Tried and abandoned
|
||||
|
||||
|
||||
Reference in New Issue
Block a user