fix(#20): heid-bug-hunt fixups — admin-stream + bifrost hardening (slice-6)
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.
This commit is contained in:
+1
-1
@@ -4,7 +4,7 @@ build-backend = "hatchling.build"
|
|||||||
|
|
||||||
[project]
|
[project]
|
||||||
name = "ratatoskr"
|
name = "ratatoskr"
|
||||||
version = "0.21.19"
|
version = "0.21.20"
|
||||||
description = "Worldtree Conversation API debug console (web + headless CLI) — multi-pane observability"
|
description = "Worldtree Conversation API debug console (web + headless CLI) — multi-pane observability"
|
||||||
readme = "README.md"
|
readme = "README.md"
|
||||||
requires-python = ">=3.12"
|
requires-python = ">=3.12"
|
||||||
|
|||||||
@@ -599,7 +599,9 @@ async def _session_bifrost_endpoint(request: Request) -> JSONResponse:
|
|||||||
{"error_code": "network_error", "message": str(exc)},
|
{"error_code": "network_error", "message": str(exc)},
|
||||||
status_code=502,
|
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:
|
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]:
|
async def gen() -> AsyncIterator[bytes]:
|
||||||
transport = client_factory()
|
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:
|
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):
|
async for ev in wt.stream_admin_events(client):
|
||||||
if not _admin_event_matches_web(ev, session_id):
|
if not _admin_event_matches_web(ev, session_id):
|
||||||
continue
|
continue
|
||||||
|
|||||||
+14
-6
@@ -727,8 +727,9 @@ async def stream_admin_events(
|
|||||||
and `type`/`data` may be None (→ `""` / `{}`) — normalized HERE so the web filter +
|
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
|
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
|
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`,
|
rows): SDK `ConnectFailed` (a connect-time transport / auth-resolution failure — the
|
||||||
NOT `ConnectFailed`) → `SseConnectFailed`; SDK `ConnectionDropped` (a connect-time
|
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
|
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
|
resumable EOF → cursor) → `SseConnectionDropped`. The SDK stream is best-effort (skips
|
||||||
malformed frames — no `Malformed*`).
|
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):
|
async for ev in client.admin.stream_events(last_event_id=last_event_id):
|
||||||
yield AdminEvent(
|
yield AdminEvent(
|
||||||
id=ev.admin_id if isinstance(ev.admin_id, int) else 0,
|
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,
|
timestamp=ev.timestamp,
|
||||||
data=dict(ev.data) if isinstance(ev.data, Mapping) else {},
|
data=dict(ev.data) if isinstance(ev.data, Mapping) else {},
|
||||||
)
|
)
|
||||||
except wtsdk.ConnectionDropped as exc:
|
except wtsdk.ConnectionDropped as exc:
|
||||||
raise SseConnectionDropped(last_seen_sse_id=exc.last_seen_sse_id) from 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:
|
except ApiError as exc:
|
||||||
# The admin stream raises ApiError("admin_stream_failed", status=…) on a non-200
|
# A non-200 open raises ApiError("admin_stream_failed", status=…) → SseConnectFailed.
|
||||||
# open (a connect-time transport failure instead surfaces as ConnectionDropped);
|
|
||||||
# map the non-200 → SseConnectFailed so the web's stream-error handler catches it.
|
|
||||||
raise SseConnectFailed(status=exc.status, body=(exc.body or "").encode()) from exc
|
raise SseConnectFailed(status=exc.status, body=(exc.body or "").encode()) from exc
|
||||||
|
|||||||
@@ -1276,6 +1276,19 @@ class TestSessionBifrostEndpoint:
|
|||||||
assert resp.status_code == 404
|
assert resp.status_code == 404
|
||||||
assert resp.json()["error_code"] == "bifrost_state_unavailable"
|
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:
|
class TestAdminEventsEndpoint:
|
||||||
"""admin_events_endpoint — SSE proxy of GET /admin/events, session-filtered (#11)."""
|
"""admin_events_endpoint — SSE proxy of GET /admin/events, session-filtered (#11)."""
|
||||||
|
|||||||
+18
-2
@@ -1205,13 +1205,29 @@ class TestStreamAdminEventsWt:
|
|||||||
assert fake.calls[-1] == ("stream_events", (), {"last_event_id": 42})
|
assert fake.calls[-1] == ("stream_events", (), {"last_event_id": 42})
|
||||||
|
|
||||||
async def test_non_200_apierror_maps_to_sse_connect_failed(self) -> None:
|
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
|
# A non-200 open raises ApiError("admin_stream_failed", status=…) → SseConnectFailed.
|
||||||
# non-200 open (NOT ConnectFailed) — mapped → SseConnectFailed for the web.
|
|
||||||
fake = _FakeAdmin(stream_error=ApiError("admin_stream_failed", "no", status=502))
|
fake = _FakeAdmin(stream_error=ApiError("admin_stream_failed", "no", status=502))
|
||||||
with pytest.raises(SseConnectFailed) as ei:
|
with pytest.raises(SseConnectFailed) as ei:
|
||||||
await _drain(stream_admin_events(_wtad(fake)))
|
await _drain(stream_admin_events(_wtad(fake)))
|
||||||
assert ei.value.status == 502
|
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:
|
async def test_connection_dropped_carries_cursor(self) -> None:
|
||||||
# A mid-stream drop / resumable EOF carries the resume cursor.
|
# A mid-stream drop / resumable EOF carries the resume cursor.
|
||||||
fake = _FakeAdmin(stream_error=wtsdk.ConnectionDropped("42"))
|
fake = _FakeAdmin(stream_error=wtsdk.ConnectionDropped("42"))
|
||||||
|
|||||||
Reference in New Issue
Block a user