contract(issue#7): author + amend #1/#3/#4 for empty-skip + MalformedSseData

Issue #7: mid-stream robustness fix discovered via 2026-05-22 crash. Long
mimir TUI conversation (turn 93, 1077 events consumed) crashed on event
1078 with JSONDecodeError("Expecting value: line 1 column 1 (char 0)")
from json.loads('') on an empty-data SSE frame. _iter_events
unconditionally called json.loads on every dispatched event; when
httpx_sse surfaces a frame with id: present but data: empty/missing
(a known library-vs-spec divergence), parsing fails and propagates.

Two-rule fix in _iter_events:
- Empty sse.data (exact `== ''`): SKIP silently per SSE spec (keepalive
  semantics). Don't yield, don't advance last_sse_id, don't set
  terminal_seen. ORDERING: skip fires BEFORE _parse_sse_id, so a
  keepalive with a malformed id is still a keepalive (intentional).
- Non-empty sse.data that fails json.loads: raise new MalformedSseData
  (sibling to MalformedSseId, mirrors raw[:200] truncation pattern).
  Wire-level protocol error; presenters route to [malformed_sse_data]
  + exit 22 in cli, transcript label + state→idle in tui (INV-008).

Volva paraphrase round: 4 ambiguities, all amended:
1. INV-001 prose tightened — exact `sse.data == ''` rule made
   prominent; "keepalive" framing demoted to intent-not-rule;
   whitespace-only data explicitly listed as malformed (not skipped);
   specific state names (last_sse_id, terminal_seen) instead of vague
   "any counter".
2. STEPS pseudocode spells out the ordering — empty-skip happens
   BEFORE _parse_sse_id; empty-data with bad id is silently swallowed.
3. empty_data_skipped test description fixed (had off-by-one count +
   wrong wording around last_sse_id intermediate state).
4. (paired with #1 above).

Volva code-review post-implementation: 3 findings, all addressed:
F1 (test-gap): empty_data_skipped proves yielded events but not
   internal last_sse_id non-advancement. New
   empty_data_skip_preserves_last_seen_sse_id test probes via
   SseConnectionDropped.last_seen_sse_id after a drop following the
   skipped frame — if the skip had transiently advanced last_sse_id,
   the exception payload would carry the wrong value.
F2 (precision, contract amend): MalformedSseData ERROR_ROUTING said
   "log truncated raw" but stream_turn doesn't log — sse_client is a
   library, presenters own observability. Amended to "propagate to
   caller (no logging at sse_client layer); presenters log exc.raw."
F3 (test-gap): cli malformed_sse_data test asserted label but not
   `raw='X'` shape and not truncation. Tightened existing test +
   added malformed_sse_data_truncation with 5000-char payload —
   verifies MalformedSseData.raw truncation carries through the
   presenter's repr() rendering.

**Calibration milestone**: issue #7 is the first issue with ZERO drift
findings from Volva code-review. TDD caught all runtime behavior
cleanly. The 3 findings were assertion-precision and
architectural-correctness-of-wording, not behavioral. Hypothesis:
tighter contract spec + smaller code surface shifts Volva's role
from "catch behavioral drift" to "tighten observability + wording".

Cumulative calibration table: #1 (4 findings, 3 drift + 1 test-gap),
#2 (3, 1+1+1 precision), #3 (5, 3+1+1), #4 (8, 5+2+1), #7 (3, 0 drift
+ 2 test-gap + 1 precision).

Contracts touched (all drift-check clean):
- docs/contracts/issues/7.contract.md (new): the coordinating record.
- docs/contracts/issues/1.contract.md: _iter_events STEP 3.0
  empty-skip + ordering note; STEP 3.c JSONDecodeError → MalformedSseData;
  new MalformedSseData ERROR_ROUTING (propagate-to-caller wording per
  F2); 4 new TESTS entries including F1's last-seen probe.
- docs/contracts/issues/3.contract.md: _run_turn ERROR_ROUTING +
  malformed_sse_data tests (incl. F3 truncation).
- docs/contracts/issues/4.contract.md: INV-008 mentions MalformedSseData;
  _stream_turn_worker ERROR_ROUTING + new TEST.
This commit is contained in:
vh
2026-05-22 16:41:16 -07:00
parent 6f7192f8db
commit 7028c5bc11
4 changed files with 337 additions and 2 deletions
+3 -2
View File
@@ -90,7 +90,7 @@ The shell is the load-bearing primary surface. Together with `--send`, it makes
- **INV-005 [hard]**: Markdown rendering on agent output is default-on; `--raw` is the opt-out. With markdown enabled, `Text` event deltas stream as raw text appended to the RichLog as they arrive (no mid-stream markdown attempt — partial markdown like `**hel` would render ugly), and on `Done` a separator + the full markdown-rendered assistant message is appended below the streamed deltas. **This means the assistant's response visibly appears TWICE in the transcript by design — once as the streamed raw deltas, once as the post-Done markdown render — separated by a horizontal-rule separator.** This is the v1 accepted trade-off for streaming-visibility-without-mid-stream-markdown-ugliness; the cleaner Static-then-commit pattern (streaming into a replaceable widget, then committing the markdown version in place) is documented in `open_questions:` as the follow-up if the double-display proves empirically noisy. Implementers MUST NOT attempt the Static-then-commit pattern in this shell — it's deferred. With `--raw`, only the streamed deltas appear; no post-Done re-render; no double-display.
- **INV-006 [hard]**: User-prompt echo in the transcript MUST visibly distinguish user input from assistant output. Format: `❯ <content>` for user lines (with a literal `❯` prefix); assistant lines have no prefix. The prefix is also a screen-reader-friendly affordance.
- **INV-007 [hard]**: One `httpx.AsyncClient` per app lifetime — opened in `on_mount`, closed in `on_unmount` via the async-with context manager pattern. The client is NOT recreated per turn (would burn the TCP connection pool).
- **INV-008 [hard]**: Mid-session network/protocol errors (`SseConnectionDropped`, `SseConnectFailed`, `MalformedSseId`, `TurnIdFlip`) during a streaming turn render as error lines in the transcript and return the app to **idle** state — they do NOT exit the app. Only initial session-create errors exit (per Data flow exit codes).
- **INV-008 [hard]**: Mid-session network/protocol errors (`SseConnectionDropped`, `SseConnectFailed`, `MalformedSseId`, `MalformedSseData` (issue #7), `TurnIdFlip`) during a streaming turn render as error lines in the transcript and return the app to **idle** state — they do NOT exit the app. Only initial session-create errors exit (per Data flow exit codes).
- **INV-009 [hard]**: No `core.*` / `worldtree.*` imports. The boundary smoke (`tests/test_no_worldtree_imports.py`) covers `src/ratatoskr/` as a whole including the new tui.py.
## Out of scope
@@ -272,7 +272,7 @@ POST: [POST-002 side_effect] each event passed through _render_event_to_log exac
POST: [POST-003 side_effect] for Done events with NOT args.raw: a separator line + the markdown-rendered Done.response appended to RichLog (INV-005)
POST: [POST-004 state_change] active_turn_id is set to event.sse_id.turn_id on the FIRST yielded event (for cancel_turn use by action_interrupt)
ERROR_ROUTING:
SseConnectFailed | SseConnectionDropped | MalformedSseId | TurnIdFlip:
SseConnectFailed | SseConnectionDropped | MalformedSseId | MalformedSseData | TurnIdFlip:
local_handling: append `[<label>] <details>` to RichLog (mirror cli.py's error labels)
flow_control: abort (the iteration aborts; finally-block restores state)
state_recovery: state → idle; footer hint reset; active_turn_id cleared. (INV-008: mid-session errors do NOT exit the app.)
@@ -304,6 +304,7 @@ TESTS:
active_turn_id_set_on_first_event [trace]: mock yields text(42:1) then waits; after first render, self.active_turn_id == 42 (verifies the cancel path can pick it up)
sse_connect_failed_returns_to_idle [error]: mock returns 404 → "[sse_connect_failed]" label in RichLog; state → idle; app does NOT exit (INV-008)
connection_dropped_returns_to_idle [error]: mock raises RemoteProtocolError mid-stream → "[connection_dropped]" label; state → idle
malformed_sse_data_returns_to_idle [error,issue#7]: mock yields text + event with `data: not-json` → "[malformed_sse_data]" label; state → idle; app does NOT exit (INV-008)
rendered_event_per_event [trace]: spy on _render_event_to_log; mock yields N events; call_count == N (terminal events included, since Done/Error/Cancelled also render through it)
```