diff --git a/docs/contracts/issues/3.contract.md b/docs/contracts/issues/3.contract.md index 36e1590..8b2b6fd 100644 --- a/docs/contracts/issues/3.contract.md +++ b/docs/contracts/issues/3.contract.md @@ -187,6 +187,10 @@ ERROR_ROUTING: local_handling: write `[auth_error] no API key (set --api-key or WORLDTREE_API_KEY)` to stderr flow_control: abort state_recovery: none + SystemExit (from argparse clean exits — `--help`, `--version`): + local_handling: catch and return `exc.code` verbatim (typically 0); argparse already printed help/version to stdout + flow_control: abort + state_recovery: none STEPS: 1. [setup, flexibility=prescriptive] TRY: args = _parse_args(argv) ON UsageError as exc: @@ -195,6 +199,8 @@ STEPS: ON _AuthError as exc: WRITE f"[auth_error] {exc}\n" to stderr RETURN 11 + ON SystemExit as exc: + RETURN int(exc.code) if exc.code is not None else 0 2. [sequential, flexibility=prescriptive] RETURN asyncio.run(_amain(args)) TESTS: happy_returns_amain_exit_code [happy,tracer]: argv specifies a complete --send invocation; monkeypatch _amain to return 0 → main returns 0 @@ -202,6 +208,7 @@ TESTS: usage_error_both_session_and_new [error]: argv has both --session and --new → returns 10; stderr "[usage_error]" auth_error_missing_key [error]: argv specifies --send/--new/--agent but neither --api-key nor WORLDTREE_API_KEY is set → returns 11; stderr "[auth_error]"; _amain never called no_argv_uses_sys_argv [trace]: argv=None → _parse_args is called with sys.argv[1:] (monkeypatched argparse capture confirms) + help_exits_cleanly [happy]: argv=["--help"] → main returns 0 (or whatever code argparse exits with); _amain never called; help text was printed to stdout by argparse ``` ```contract @@ -232,6 +239,10 @@ ERROR_ROUTING: local_handling: raise _AuthError("no API key (set --api-key or WORLDTREE_API_KEY)") flow_control: abort state_recovery: none + argparse SystemExit (clean exits — `--help`, `--version` etc., code=0): + local_handling: allow to propagate from `_parse_args` to `main`; `main` catches and returns the code verbatim + flow_control: passthrough — argparse already printed help/version to stdout; no further work needed + state_recovery: none (no resources acquired before _parse_args) STEPS: 1. [setup, flexibility=prescriptive] Construct argparse.ArgumentParser: --send (required, str, non-empty) diff --git a/persistent-memory.md b/persistent-memory.md index 74e6018..6dfdfa0 100644 --- a/persistent-memory.md +++ b/persistent-memory.md @@ -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 diff --git a/src/ratatoskr/cli.py b/src/ratatoskr/cli.py index d7f5567..589786e 100644 --- a/src/ratatoskr/cli.py +++ b/src/ratatoskr/cli.py @@ -72,6 +72,7 @@ class _UsageErrorParser(argparse.ArgumentParser): def _parse_args(argv: list[str] | None) -> ParsedArgs: """argparse + env-fallback + xor-validation per the contract.""" + assert argv is None or all(isinstance(a, str) for a in argv) parser = _UsageErrorParser(prog="ratatoskr", description="Worldtree CLI presenter.") parser.add_argument("--send", required=True) parser.add_argument("--session") @@ -113,6 +114,10 @@ def _parse_args(argv: list[str] | None) -> ParsedArgs: def _render_event(event: Event, *, stdout: TextIO, stderr: TextIO) -> None: """Pure event-to-output renderer per the contract STEPS table.""" + assert isinstance( + event, + (WorkerPhase, Thinking, Text, TextBoundary, ToolStart, ToolResult, Done, Error, Cancelled), + ) if isinstance(event, Text): stdout.write(event.content) stdout.flush() @@ -241,11 +246,11 @@ async def _run_turn( finally: if sigint_task is not None and not sigint_task.done(): sigint_task.cancel() - # cancel_task is the strong-ref holder for the fire-and-forget cancel POST (RUF006). - # By the time we reach `finally`, the stream has drained to a terminal event, so the - # cancel POST has either completed or returned an error (swallowed by _cancel_and_log). - # Awaiting it here would block on a guaranteed-done task; the reference alone suffices. - _ = cancel_task + # Await the fire-and-forget cancel so INV-009 + _cancel_and_log POST-002 are + # guaranteed: the [cancel_failed] line (if any) lands before _run_turn returns + # AND before _amain closes the AsyncClient context. _cancel_and_log never raises. + if cancel_task is not None: + await cancel_task async def _amain(args: ParsedArgs) -> int: @@ -294,6 +299,7 @@ async def _amain(args: ParsedArgs) -> int: def main(argv: list[str] | None = None) -> int: """Sync entry point. Maps UsageError/_AuthError to exit codes BEFORE the event loop.""" + assert argv is None or all(isinstance(a, str) for a in argv) try: args = _parse_args(argv) except UsageError as exc: @@ -302,4 +308,8 @@ def main(argv: list[str] | None = None) -> int: except _AuthError as exc: sys.stderr.write(f"[auth_error] {exc}\n") return 11 + except SystemExit as exc: + # argparse's --help / --version short-circuit via SystemExit(0). Pass the code + # through verbatim — argparse already printed help to stdout. + return int(exc.code) if exc.code is not None else 0 return asyncio.run(_amain(args)) diff --git a/tests/test_cli.py b/tests/test_cli.py index a5c7f82..59e076c 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -34,6 +34,18 @@ from ratatoskr.sse_client import ( ) +class _FlushCountingIO(io.StringIO): + """StringIO subclass that counts flush() calls — used to verify INV-010.""" + + def __init__(self) -> None: + super().__init__() + self.flush_count = 0 + + def flush(self) -> None: + self.flush_count += 1 + super().flush() + + def _sse_chunk(sse_id: str, body: dict[str, object]) -> bytes: return f"id: {sse_id}\ndata: {json.dumps(body)}\n\n".encode() @@ -176,15 +188,16 @@ SID = SseId(42, 5) class TestRenderEvent: def test_text_to_stdout_only(self) -> None: """text_to_stdout_only [happy,tracer]: Text → stdout=="hello"; stderr empty; flushed.""" - stdout = io.StringIO() + stdout = _FlushCountingIO() stderr = io.StringIO() _render_event(Text(sse_id=SID, content="hello"), stdout=stdout, stderr=stderr) assert stdout.getvalue() == "hello" assert stderr.getvalue() == "" + assert stdout.flush_count == 1 # INV-010: per-chunk flush def test_done_writes_newline_and_label(self) -> None: - """done_writes_newline_and_label: stdout=="\\n"; stderr "[done]" + turn_id + model.""" - stdout = io.StringIO() + """done_writes_newline_and_label: stdout=="\\n" (flushed); stderr "[done]" labels.""" + stdout = _FlushCountingIO() stderr = io.StringIO() evt = Done( sse_id=SID, @@ -196,6 +209,7 @@ class TestRenderEvent: ) _render_event(evt, stdout=stdout, stderr=stderr) assert stdout.getvalue() == "\n" + assert stdout.flush_count == 1 # INV-010: post-Done newline flushed out_err = stderr.getvalue() assert out_err.startswith("[done]") assert "turn_id=42" in out_err @@ -957,3 +971,22 @@ class TestMain: monkeypatch.setattr(cli_mod, "_amain", fake_amain) rc = main(None) assert rc == 0 + + def test_help_exits_cleanly( + self, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + ) -> None: + """help_exits_cleanly [happy]: --help → main returns 0; _amain never called.""" + amain_calls: list[int] = [] + + async def fake_amain(args: ParsedArgs) -> int: + amain_calls.append(1) + return 0 + + monkeypatch.setattr(cli_mod, "_amain", fake_amain) + rc = main(["--help"]) + assert rc == 0 + assert amain_calls == [] + # argparse prints help text to stdout + assert "ratatoskr" in capsys.readouterr().out