From 11ae2f056efe5e9674d098379aa532274ac1d923 Mon Sep 17 00:00:00 2001 From: Vuong Hoang Date: Sun, 19 Jul 2026 13:13:44 -0700 Subject: [PATCH] =?UTF-8?q?fix(#20):=20heid-bug-hunt=20fixups=20=E2=80=94?= =?UTF-8?q?=20admin-stream=20+=20bifrost=20hardening=20(slice-6)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cold spec-free panel (Gróa + Hulda + Regin, source-verified by Heid): the adapter's core re-wrap is sound, but 4 real hardening gaps the conformance CR couldn't see — all in failure-path normalization + open-world degrade, judged against the general ConnectFailed floor + the degrade-never-crash promise. All fixed: - [bug, 3/3] `stream_admin_events` never mapped `ConnectFailed` — the SDK admin-stream open raises it on a connect-time / auth-resolution failure (the general transport floor; confirmed in the SDK source), and `stream_turn` + the bifrost GET both catch it, and this endpoint's OWN comment claimed it did. An unmapped ConnectFailed escaped the web gen's `except (Sse*)` and aborted the SSE with no `stream_error`. Now mapped → `SseConnectFailed`, mirroring stream_turn. - [bug, 2/3] non-str `type` crashed the web filter — the re-wrap used `ev.type or ""` (falsy-only), so a truthy non-str `type` (123, a list) reached `.startswith` → AttributeError. Now `ev.type if isinstance(ev.type, str) else ""` (matches the admin_id/data isinstance guards — same container-type class as slice-5). - [robustness] `_session_bifrost_endpoint` did `dict(bstate)` on the open-world 200 body — a non-mapping (list/scalar) → TypeError/500. Now degrades to `{}` (I introduced this in slice-6 by changing `JSONResponse(bstate)` → `dict(bstate)`). - [robustness] `_admin_events_endpoint.gen` allocated the transport + built `_wt_client` BEFORE the try/finally — a construction failure would leak the httpx transport. Moved `_wt_client` inside the try so the finally always closes it. Voided (Heid): Regin's `dict(ev.data)` TypeError — the `isinstance(_, Mapping)` guard already routes non-mappings to `{}` before `dict()`. Added adapter tests (ConnectFailed→SseConnectFailed; non-str type→"") + a web test (non-mapping bifrost body → 200 {}). Suite 494 green; my code ruff-clean (13 E501/F841 in test_web_server.py are PRE-EXISTING, HEAD-identical, untouched); mypy clean on wt.py. Live smoke re-run clean (real session.created event re-wrapped; bifrost 404 envelope). Patch bump 0.21.19 → 0.21.20. --- pyproject.toml | 2 +- src/ratatoskr/web/server.py | 14 +++++++++----- src/ratatoskr/wt.py | 20 ++++++++++++++------ tests/test_web_server.py | 13 +++++++++++++ tests/test_wt.py | 20 ++++++++++++++++++-- uv.lock | 2 +- 6 files changed, 56 insertions(+), 15 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 7addd42..08b9f4d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "ratatoskr" -version = "0.21.19" +version = "0.21.20" description = "Worldtree Conversation API debug console (web + headless CLI) — multi-pane observability" readme = "README.md" requires-python = ">=3.12" diff --git a/src/ratatoskr/web/server.py b/src/ratatoskr/web/server.py index 70269d2..3819a8e 100644 --- a/src/ratatoskr/web/server.py +++ b/src/ratatoskr/web/server.py @@ -599,7 +599,9 @@ async def _session_bifrost_endpoint(request: Request) -> JSONResponse: {"error_code": "network_error", "message": str(exc)}, status_code=502, ) - return JSONResponse(dict(bstate), status_code=200) + # Open-world read: degrade a non-mapping 200 body to {} rather than 500ing on + # `dict(non-mapping)` (heid bug-hunt slice-6). + return JSONResponse(dict(bstate) if isinstance(bstate, Mapping) else {}, status_code=200) def _admin_event_matches_web(ev: AdminEvent, session_id: str | None) -> bool: @@ -628,11 +630,13 @@ async def _admin_events_endpoint(request: Request) -> Response: async def gen() -> AsyncIterator[bytes]: transport = client_factory() - # slice-6: admin_auth rides on the wt client (built with admin_key); the adapter - # re-wraps the SDK's AdminEvent → ratatoskr's (id/type/data degraded) and the - # stream's terminal SDK errors (incl. ConnectFailed) → the Sse* types below. - client = _wt_client(transport, admin_key=admin_key) try: + # slice-6: admin_auth rides on the wt client (built with admin_key); the + # adapter re-wraps the SDK's AdminEvent → ratatoskr's (id/type/data degraded) + # and the stream's terminal SDK errors (incl. ConnectFailed) → the Sse* types + # below. Built INSIDE the try so a construction failure still hits the finally + # that closes the transport — no leak (heid bug-hunt slice-6). + client = _wt_client(transport, admin_key=admin_key) async for ev in wt.stream_admin_events(client): if not _admin_event_matches_web(ev, session_id): continue diff --git a/src/ratatoskr/wt.py b/src/ratatoskr/wt.py index dd29abc..3a0e802 100644 --- a/src/ratatoskr/wt.py +++ b/src/ratatoskr/wt.py @@ -727,8 +727,9 @@ async def stream_admin_events( and `type`/`data` may be None (→ `""` / `{}`) — normalized HERE so the web filter + formatter (`ev.id` / `ev.type` / `ev.data`) never crash on a partial wire (chosen over yielding SDK events through + rewiring the web filter). Error map (INV-CUT-2, stream - rows): SDK `ApiError` (a non-200 open — the admin stream raises `admin_stream_failed`, - NOT `ConnectFailed`) → `SseConnectFailed`; SDK `ConnectionDropped` (a connect-time + rows): SDK `ConnectFailed` (a connect-time transport / auth-resolution failure — the + SDK's general transport floor) → `SseConnectFailed`; SDK `ApiError` (a non-200 open, + `admin_stream_failed`) → `SseConnectFailed`; SDK `ConnectionDropped` (a connect-time transport failure → cursor None, OR a mid-stream drop / the long-lived stream's resumable EOF → cursor) → `SseConnectionDropped`. The SDK stream is best-effort (skips malformed frames — no `Malformed*`). @@ -737,14 +738,21 @@ async def stream_admin_events( async for ev in client.admin.stream_events(last_event_id=last_event_id): yield AdminEvent( id=ev.admin_id if isinstance(ev.admin_id, int) else 0, - type=ev.type or "", + # isinstance-guard `type` (not `or ""`): a truthy NON-str type (123, a + # list from a partial wire) would otherwise reach `.startswith` in the + # web filter → AttributeError (heid bug-hunt slice-6; match admin_id/data). + type=ev.type if isinstance(ev.type, str) else "", timestamp=ev.timestamp, data=dict(ev.data) if isinstance(ev.data, Mapping) else {}, ) except wtsdk.ConnectionDropped as exc: raise SseConnectionDropped(last_seen_sse_id=exc.last_seen_sse_id) from exc + except wtsdk.ConnectFailed as exc: + # A connect-time transport / auth-resolution failure surfaces as ConnectFailed + # (the SDK's general transport floor) — map it → SseConnectFailed, mirroring + # stream_turn. The web gen catches the Sse* types, so an unmapped ConnectFailed + # would escape and abort the SSE with no labeled stream_error (heid bug-hunt). + raise SseConnectFailed(status=exc.status, body=(exc.message or "").encode()) from exc except ApiError as exc: - # The admin stream raises ApiError("admin_stream_failed", status=…) on a non-200 - # open (a connect-time transport failure instead surfaces as ConnectionDropped); - # map the non-200 → SseConnectFailed so the web's stream-error handler catches it. + # A non-200 open raises ApiError("admin_stream_failed", status=…) → SseConnectFailed. raise SseConnectFailed(status=exc.status, body=(exc.body or "").encode()) from exc diff --git a/tests/test_web_server.py b/tests/test_web_server.py index 7c9737a..59fefdd 100644 --- a/tests/test_web_server.py +++ b/tests/test_web_server.py @@ -1276,6 +1276,19 @@ class TestSessionBifrostEndpoint: assert resp.status_code == 404 assert resp.json()["error_code"] == "bifrost_state_unavailable" + @respx.mock + def test_non_mapping_body_degrades_to_empty(self) -> None: + """robustness: a non-mapping open-world 200 body (list/scalar) → 200 {} envelope, + never a `dict(non-mapping)` TypeError/500 (heid bug-hunt slice-6).""" + respx.get("https://w.example/admin/sessions/s-1/bifrost").mock( + return_value=httpx.Response(200, json=["not", "a", "mapping"]) + ) + from ratatoskr.web.server import create_app + app = create_app(_mock_client_factory(), admin_key="adm-key") + resp = TestClient(app).get("/api/sessions/s-1/bifrost") + assert resp.status_code == 200 + assert resp.json() == {} + class TestAdminEventsEndpoint: """admin_events_endpoint — SSE proxy of GET /admin/events, session-filtered (#11).""" diff --git a/tests/test_wt.py b/tests/test_wt.py index cabf3c4..c016927 100644 --- a/tests/test_wt.py +++ b/tests/test_wt.py @@ -1205,13 +1205,29 @@ class TestStreamAdminEventsWt: assert fake.calls[-1] == ("stream_events", (), {"last_event_id": 42}) async def test_non_200_apierror_maps_to_sse_connect_failed(self) -> None: - # The SDK admin stream raises ApiError("admin_stream_failed", status=…) on a - # non-200 open (NOT ConnectFailed) — mapped → SseConnectFailed for the web. + # A non-200 open raises ApiError("admin_stream_failed", status=…) → SseConnectFailed. fake = _FakeAdmin(stream_error=ApiError("admin_stream_failed", "no", status=502)) with pytest.raises(SseConnectFailed) as ei: await _drain(stream_admin_events(_wtad(fake))) assert ei.value.status == 502 + async def test_connect_failed_maps_to_sse_connect_failed(self) -> None: + # A connect-time transport / auth-resolution failure surfaces as ConnectFailed + # (the SDK's general floor) → SseConnectFailed, mirroring stream_turn — else it + # escapes the web gen's Sse* handler and aborts the SSE (heid bug-hunt slice-6). + fake = _FakeAdmin(stream_error=wtsdk.ConnectFailed("connect_failed", "refused", status=0)) + with pytest.raises(SseConnectFailed) as ei: + await _drain(stream_admin_events(_wtad(fake))) + assert ei.value.status == 0 + + async def test_nonstr_type_degrades_to_empty(self) -> None: + # A truthy NON-str `type` (a partial/wrong open-world wire) must degrade to "" + # so the web filter's `.startswith` never AttributeErrors — `or ""` (falsy-only) + # would let it through; the isinstance guard catches it (heid bug-hunt slice-6). + fake = _FakeAdmin(events=[_SdkAdminEvent(1, 123, "t", {"session_id": "s"})]) + out = await _drain(stream_admin_events(_wtad(fake))) + assert out[0].type == "" + async def test_connection_dropped_carries_cursor(self) -> None: # A mid-stream drop / resumable EOF carries the resume cursor. fake = _FakeAdmin(stream_error=wtsdk.ConnectionDropped("42")) diff --git a/uv.lock b/uv.lock index 4a2f070..0266d8b 100644 --- a/uv.lock +++ b/uv.lock @@ -472,7 +472,7 @@ wheels = [ [[package]] name = "ratatoskr" -version = "0.21.19" +version = "0.21.20" source = { editable = "." } dependencies = [ { name = "httpx" },