From 75dec016eb7c19477320ff153c6c3a70ddee490e Mon Sep 17 00:00:00 2001 From: Vuong Hoang Date: Wed, 1 Jul 2026 12:52:44 -0700 Subject: [PATCH] =?UTF-8?q?fix(web):=20heid-review=20findings=20=E2=80=94?= =?UTF-8?q?=20SSE=20lifecycle=20teardown=20+=20test-shape=20gaps=20(v0.19.?= =?UTF-8?q?3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-frontier panel (Gróa/Hulda/Regin) on the v0.19.2 web surface, triaged: - FIX (Gróa #1, drift): the turn EventSource `onerror` (raw transport drop) now calls hideThinkingNote() — a drop mid-reasoning no longer leaves the " is pondering…" line + its setInterval running (INV-LIFECYCLE). - FIX (Gróa #4 + Hulda #1, convergent drift): openAdminEvents now closes the EventSource + clears state.adminES on `stream_error` (server signalled end) and on a PERMANENT onerror (readyState CLOSED) — native EventSource no longer auto-reconnects into a retry loop; transient CONNECTING drops still reconnect. - TEST (Gróa #2 + Hulda #3): test_routes_registered asserts the 3 new routes; test_state_attached asserts app.state.admin_key (create_app POST-001/002). - TEST (Gróa #3 + Regin #3): AdminEvents stream_error-on-connect-failure test — upstream non-200 -> exactly one `stream_error` frame, then ends (POST-003). - CONTRACT (Hulda #2 + Regin #2, accepted): clarified the Tools inventory renders NAMES only by design (descriptions live in the BifrostState pane); code unchanged. Also lands the web_debug_surface contract as the trail. Accepted-no-op: 403-bifrost / non-404-tools tests (identical code path to the tested 404). Panel found ZERO functional server-side drift; INV-004 escaping confirmed clean across the new panes. 60 web tests pass; JS + ruff clean. --- docs/contracts/web_debug_surface.contract.md | 140 +++++++++++++++++++ pyproject.toml | 2 +- src/ratatoskr/web/static/index.html | 8 ++ tests/test_web_server.py | 23 ++- uv.lock | 2 +- 5 files changed, 172 insertions(+), 3 deletions(-) create mode 100644 docs/contracts/web_debug_surface.contract.md diff --git a/docs/contracts/web_debug_surface.contract.md b/docs/contracts/web_debug_surface.contract.md new file mode 100644 index 0000000..fab8d87 --- /dev/null +++ b/docs/contracts/web_debug_surface.contract.md @@ -0,0 +1,140 @@ +--- +contract_version: "2.1" +module: "ratatoskr.web" +purpose: "v0.19.2 web debug-surface parity: 3 admin/debug panes (Tools inventory, BifrostState, AdminEvents SSE) proxied server-side with the admin key server-held, plus a reliable PAD-refresh poll and a non-engine reasoning indicator in the transcript." +target_module: "ratatoskr.web (server.py routes + entrypoint.py + static/index.html)" +scope: "v0.19.2 web debug-surface parity — bring the browser surface (now the PRIMARY debug surface) to TUI parity. THREE new admin/debug panes proxied server-side + TWO transcript affordances. (1) Tools inventory: GET /api/sessions/{id}/tools proxies owner-scoped get_session_tools into the tools pane (what the LLM HAS at turn-fire), above the live tool events. (2) BifrostState pane: GET /api/sessions/{id}/bifrost proxies admin-scoped get_session_bifrost; the admin key is SERVER-HELD (app.state.admin_key from RATATOSKR_ADMIN_API_KEY), never sent to the browser. (3) AdminEvents pane: GET /api/admin/events is an SSE proxy of stream_admin_events, session-filtered SERVER-side (heartbeats + other-session events dropped), re-emitted under a fixed 'admin_event' name so every dotted type renders with one browser listener. (4) PAD refresh: the persona/affect pane polls a bounded window instead of a single 2s shot that raced the post-turn-async affect.emit. (5) Reasoning indicator: an ephemeral, clearly-non-engine transcript line on `thinking` deltas, cleared when text begins. Direct in-session TDD (the #17/#18 pattern); this contract is authored post-implementation to anchor the heid code review (the client wrappers get_session_tools/get_session_bifrost/stream_admin_events are already contracted in the sessions/sse_client specs — this contract governs the WEB proxy + presenter surface only)." +depends_on: + - "httpx" + - "starlette" + - "ratatoskr.sessions" # get_session_tools, get_session_bifrost, SessionApiFailed + - "ratatoskr.sse_client" # stream_admin_events, AdminEvent, SseConnectFailed/Dropped +used_by: + - "ratatoskr.web.entrypoint" # passes admin_key=RATATOSKR_ADMIN_API_KEY into create_app +language: "python + vanilla JS (single-file SPA, no build)" +complexity: "medium" +estimated_loc: 290 +confidence: 0.8 +assumptions: + - "The three client wrappers exist and are already contracted: get_session_tools(client, session_id)->dict (owner-scoped, consumer bearer; non-200 -> SessionApiFailed), get_session_bifrost(client, session_id, *, admin_key)->dict (OVERRIDES Authorization with admin_key; non-200 -> SessionApiFailed), stream_admin_events(client, *, admin_key)->AsyncIterator[AdminEvent] (non-200 -> SseConnectFailed; mid-drop -> SseConnectionDropped). The web routes are thin proxies over them; they add NO new upstream semantics." + - "AdminEvent = {id:int, type:str, timestamp:str|None, data:dict}. data MOST carry session_id (INV-049). type is a dotted namespace (session.*/turn.*/key.*/system.*)." + - "The web SPA is a single static/index.html served per-request via FileResponse (edits land on browser refresh; server code changes need a restart). Model/tool/admin content is UNTRUSTED text (INV-004) — every render path escapes first (esc() via textContent, or JSON.stringify wrapped in esc())." + - "The internal-LAN trust model (0.0.0.0, no auth/TLS/CORS) is deliberate operator direction. Admin-scoped DATA becoming LAN-visible is accepted under that model; the admin KEY must nonetheless never cross to the browser." + - "Tests: respx mocks the upstream endpoints (absolute w.example URLs) driven through the TestClient; the AdminEvents SSE proxy is tested with a finite mocked SSE byte-stream asserting the filter + fixed event name. Live-proven against ratatoskr:sindra on personal :8081." + +# ───────────────────────────────────────────────────────────────────────────── +functions: + - name: "_session_tools_endpoint" + signature: "async _session_tools_endpoint(request: Request) -> JSONResponse" + description: "GET /api/sessions/{session_id}/tools — proxy owner-scoped tool inventory." + preconditions: + - "session_id in path_params." + postconditions: + - "POST-001: 200 with the upstream inventory dict verbatim on success." + - "POST-002: on SessionApiFailed(status) -> JSONResponse({error_code:'session_tools_unavailable', status}, status_code=status) — status-preserving." + steps: "Open client_factory() client; await get_session_tools(client, session_id); return 200. Except SessionApiFailed -> status-preserving envelope." + flexibility: "prescriptive" + + - name: "_session_bifrost_endpoint" + signature: "async _session_bifrost_endpoint(request: Request) -> JSONResponse" + description: "GET /api/sessions/{session_id}/bifrost — proxy admin-scoped Bifrost dispatch state." + preconditions: + - "session_id in path_params." + - "PRE-001 (fail-visible): app.state.admin_key must be truthy; else 400 admin_key_not_configured with NO upstream call." + postconditions: + - "POST-001: the admin key is read from app.state.admin_key ONLY; it is passed to get_session_bifrost(admin_key=...) and NEVER placed in a response body or surfaced to the browser." + - "POST-002: 200 with the upstream state dict verbatim on success." + - "POST-003: on SessionApiFailed(status) -> {error_code:'bifrost_state_unavailable', status} at status_code=status (notably 404 not-bound, 403 scope-denied)." + steps: "If not admin_key -> 400. Open client; await get_session_bifrost(client, session_id, admin_key=admin_key); 200. Except SessionApiFailed -> status-preserving envelope." + flexibility: "prescriptive" + + - name: "_admin_event_matches_web" + signature: "_admin_event_matches_web(ev: AdminEvent, session_id: str | None) -> bool" + description: "AdminEvents session-filter (mirrors the TUI _admin_event_matches, design-brief §6)." + postconditions: + - "POST-001: ev.type == 'system.heartbeat' -> False (keepalive noise dropped)." + - "POST-002: ev.type.startswith('system.') (non-heartbeat) -> True (stream-integrity signals always pass)." + - "POST-003: otherwise -> True IFF session_id is not None AND ev.data.get('session_id') == session_id (per-session scoping; a None session_id forwards NO non-system event)." + flexibility: "prescriptive" + + - name: "_admin_events_endpoint" + signature: "async _admin_events_endpoint(request: Request) -> Response" + description: "GET /api/admin/events?session_id=... — SSE proxy of stream_admin_events, session-filtered server-side." + preconditions: + - "PRE-001 (fail-visible): app.state.admin_key truthy; else 400 admin_key_not_configured with NO stream opened." + postconditions: + - "POST-001: returns StreamingResponse(media_type='text/event-stream'); the admin key never crosses to the browser." + - "POST-002: ONLY events passing _admin_event_matches_web(ev, session_id) are forwarded; each is re-emitted under the FIXED SSE event name 'admin_event' with {id,type,timestamp,data} in the payload (the real dotted type rides in the payload, so one browser listener renders every type — nothing silently dropped by name)." + - "POST-003: SseConnectFailed/SseConnectionDropped/MalformedSseId/MalformedSseData -> a single 'stream_error' SSE frame, then the stream ends (best-effort; never raises to the browser)." + - "POST-004: asyncio.CancelledError (browser disconnect) re-raises to unwind the generator; the upstream client is aclose()'d in finally on every exit path." + steps: "If not admin_key -> 400. gen(): open client; async-for ev in stream_admin_events(admin_key); skip unless _admin_event_matches_web; yield _format_sse('admin_event', {...}). Except SSE errors -> yield stream_error. Except CancelledError -> raise. Finally aclose(). Return StreamingResponse(gen())." + flexibility: "prescriptive" + + - name: "create_app (amendment)" + signature: "create_app(client_factory, *, end_user_id=None, bifrost_consumer_key=None, bifrost_visible_host=None, affect_read_url=None, admin_key=None) -> Starlette" + description: "New optional admin_key param stored at app.state.admin_key; entrypoint passes RATATOSKR_ADMIN_API_KEY. Three new routes registered." + postconditions: + - "POST-001: app.state.admin_key = admin_key (default None -> the two admin routes fail-visible per their PRE-001)." + - "POST-002: routes /api/sessions/{session_id}/tools, /api/sessions/{session_id}/bifrost, /api/admin/events added; existing routes unchanged." + flexibility: "closed" + + - name: "reasoning indicator (index.html: showThinkingNote / hideThinkingNote)" + signature: "showThinkingNote() ; hideThinkingNote() // called from the turn SSE loop" + description: "Ephemeral transcript affordance signalling reasoning inference — clearly NOT engine output." + postconditions: + - "POST-001: on the first `thinking` delta, an italic ' ' line (✦ glyph, rotating phrase) is shown; it supersedes any live 'awaiting first token' heartbeat." + - "POST-002: the agent display name is derived from state.agentId and rendered via textContent (NEVER innerHTML) — INV-004 holds even for an adversarial agent_id." + - "POST-003: it is removed the instant the first `text` delta arrives, and on any terminal (done/error/cancelled); the rotation interval is cleared on removal (no leaked setInterval)." + flexibility: "prescriptive" + + - name: "PAD refresh poll (index.html: terminal() done-branch)" + signature: "on Done: poll loadPersona over [1500,3500,6500,10500]ms" + description: "Catch the post-turn-async affect.emit without racing it (replaces the single 2s shot)." + postconditions: + - "POST-001: loadAffect sets state.lastAffectAt = snap.emitted_at; the poll captures beforeAt and stops (settled) once state.lastAffectAt !== beforeAt." + - "POST-002: a scheduled poll no-ops if a NEW turn has started (state.turnId truthy) or already settled — no refresh of a stale agent, no unbounded polling." + flexibility: "open" + + - name: "web pane renderers (index.html: renderToolsInventory / renderBifrostState / openAdminEvents)" + signature: "renderToolsInventory(inv) ; renderBifrostState(b) ; openAdminEvents(sessionId)" + description: "Render the three new surfaces; all content escaped (INV-004)." + postconditions: + - "POST-001: every dynamic value (agent_id, tool names/descriptions, endpoint, caps, admin event type + data) is passed through esc() or esc(JSON.stringify(...)); no upstream string reaches innerHTML unescaped." + - "POST-002: openAdminEvents closes a prior EventSource before opening a new one (state.adminES); the admin data blob renders via esc(JSON.stringify(d.data))." + - "POST-003: renderToolsInventory prepends the static inventory ABOVE live tool events without clobbering them (a re-render replaces only the .tools-inventory block)." + - "POST-004: the Tools inventory renders tool NAMES only — a compact comma-joined summary ('what does the LLM have', the debug glance); per-tool DESCRIPTIONS are surfaced in the BifrostState pane's tools list, deliberately NOT duplicated here. (Heid panel Hulda/Regin precision finding — accepted: contract wording clarified, code unchanged; the earlier 'names/descriptions' phrasing in INV-004 refers to the SET of value types that MAY appear across the new panes and must be escaped, not a mandate that every pane render descriptions.)" + flexibility: "open" + +invariants: + - "INV-004 (untrusted-render): ALL model / tool / admin / agent-supplied text is escaped before entering the DOM (esc via textContent, or esc(JSON.stringify)). No new render path introduces an innerHTML sink for upstream content. This is the highest-value review target — the new JS render paths are NOT unit-tested." + - "INV-ADMIN-KEY: the admin key exists ONLY at app.state.admin_key (from RATATOSKR_ADMIN_API_KEY). It is never serialized into any response, never sent to the browser, never logged. The browser receives only the session-filtered RESULT of admin-scoped reads." + - "INV-FILTER: AdminEvents filtering happens SERVER-side (_admin_event_matches_web) — the browser never receives the cross-session admin firehose; only active-session events + non-heartbeat system.* cross the wire." + - "INV-FAIL-VISIBLE: both admin routes return 400 admin_key_not_configured when the key is absent — never a silent empty pane, never an upstream call with an empty bearer." + - "INV-LIFECYCLE: SSE generators and EventSources are cleaned up on every exit path (upstream client aclose() in finally; setInterval cleared in hideThinkingNote; prior EventSource closed before re-open) — no leaked connections, tasks, or timers." + - "INV-ADDITIVE: existing routes, panes, and the turn-stream path are unchanged; the 3 new routes + 2 new tabs are purely additive (59 web tests incl. all prior ones stay green)." +--- + +# v0.19.2 — web debug-surface parity (BifrostState · AdminEvents · Tools · PAD-poll · reasoning) + +## Context + +The browser surface is now the operator's PRIMARY debug surface, and it lagged the +TUI: the TUI gained Tools/BifrostState/AdminEvents panes (v0.18.9–.11) that were never +ported to the web. This change closes that gap and adds two transcript affordances (a +reliable PAD refresh + a reasoning indicator). The client wrappers already existed and +are contracted elsewhere; this contract governs the WEB proxy routes + the SPA presenter +paths, whose JS render code is not unit-tested — hence the cross-frontier code review. + +## Review focus (for the heid panel) + +1. **INV-004 escaping** in every new render path — the un-unit-tested surface; the exact + class of bug (`renderPersonaPane` fabricating a Tier-1 field) that only a cross-model + review caught on #18 D2. +2. **INV-ADMIN-KEY** — confirm the admin key never reaches a response body or the browser. +3. **AdminEvents SSE proxy** (`_admin_events_endpoint`) — generator/filter/lifecycle: fixed + event name, server-side filter, `stream_error` on failure, `aclose()` on every path, + `CancelledError` re-raise on disconnect. +4. **PAD-poll** stop-condition — does `emitted_at` advancement + the `state.turnId` guard + correctly stop the poll without racing or leaking timers? +5. **Reasoning indicator** lifecycle — shown on first `thinking`, removed on first `text` + or terminal, interval cleared (no leaked `setInterval`), name via `textContent`. diff --git a/pyproject.toml b/pyproject.toml index 8311d93..e4c8a43 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "ratatoskr" -version = "0.19.2" +version = "0.19.3" description = "Worldtree Conversation API debug TUI — multi-pane observability dashboard" readme = "README.md" requires-python = ">=3.12" diff --git a/src/ratatoskr/web/static/index.html b/src/ratatoskr/web/static/index.html index ae7dd30..ab27bf7 100644 --- a/src/ratatoskr/web/static/index.html +++ b/src/ratatoskr/web/static/index.html @@ -935,8 +935,15 @@ function openAdminEvents(sessionId) { es.addEventListener("stream_error", (e) => { let d = {}; try { d = JSON.parse(e.data); } catch (_) {} appendTo("pane-admin", `
— admin stream ended: ${esc(d.exception || "error")} —
`); + // Server signalled the stream is over — close so native EventSource does NOT auto-reconnect + // into a retry loop (INV-LIFECYCLE). + es.close(); state.adminES = null; }); es.onerror = () => { + // EventSource auto-reconnects on a transient drop (readyState CONNECTING) — leave that be, + // the admin stream is long-lived. Only tear down on a PERMANENT failure (CLOSED — e.g. a + // 400/403 where no reconnect is coming) so we don't leak a dead handle. + if (es.readyState === EventSource.CLOSED) { state.adminES = null; } const pane = $("pane-admin"); if (pane.querySelector(".empty")) { pane.innerHTML = `
admin stream unavailable — needs the readonly-admin key + admin.events.read scope.
`; @@ -1184,6 +1191,7 @@ async function submitPrompt() { es.addEventListener("cancelled", (e) => terminal("cancelled", "cancelled", e)); es.onerror = () => { audit("sse_connection_dropped turn_id=" + turn_id); + hideThinkingNote(); // a raw drop mid-reasoning must not leave the note + rotator running es.close(); state.eventSource = null; state.turnId = null; $("composer").classList.remove("streaming"); diff --git a/tests/test_web_server.py b/tests/test_web_server.py index 35cc8a1..edde11d 100644 --- a/tests/test_web_server.py +++ b/tests/test_web_server.py @@ -625,6 +625,10 @@ class TestCreateAppShape: "/", "/version", "/api/agents", "/api/sessions", "/api/agents/{agent_id}/persona_state", "/api/affect/{agent_id}", + # v0.19.2 debug-surface parity (create_app POST-002) + "/api/sessions/{session_id}/tools", + "/api/sessions/{session_id}/bifrost", + "/api/admin/events", "/api/turns/{session_id}", "/api/turns/{session_id}/stream", "/api/turns/{session_id}/cancel", ): @@ -633,10 +637,14 @@ class TestCreateAppShape: assert "/static" in paths def test_state_attached(self) -> None: - """state_attached [trace]: app.state.turn_registry is empty dict.""" + """state_attached [trace]: app.state.turn_registry is empty dict; admin_key stored.""" from ratatoskr.web.server import create_app app = create_app(_mock_client_factory()) assert app.state.turn_registry == {} + # create_app POST-001: admin_key defaults None (admin routes fail-visible) + assert app.state.admin_key is None + app2 = create_app(_mock_client_factory(), admin_key="adm-key") + assert app2.state.admin_key == "adm-key" def test_factory_stored(self) -> None: """factory_stored [trace]: app.state.client_factory is the same callable.""" @@ -1182,3 +1190,16 @@ class TestAdminEventsEndpoint: assert "system.degraded" in body # system.* → forwarded assert "system.heartbeat" not in body # heartbeat → dropped assert "turn.started" not in body # other session → dropped + + @respx.mock + def test_stream_error_on_connect_failure(self) -> None: + """error: upstream admin SSE non-200 -> ONE stream_error frame, stream ends (POST-003).""" + respx.get("https://w.example/admin/events").mock( + return_value=httpx.Response(500, content=b"boom") + ) + from ratatoskr.web.server import create_app + app = create_app(_mock_client_factory(), admin_key="adm-key") + body = TestClient(app).get("/api/admin/events?session_id=s-1").text + assert "event: stream_error" in body + assert "SseConnectFailed" in body + assert body.count("event: stream_error") == 1 # exactly one, then ends diff --git a/uv.lock b/uv.lock index b1d7db6..b57bb4e 100644 --- a/uv.lock +++ b/uv.lock @@ -1052,7 +1052,7 @@ wheels = [ [[package]] name = "ratatoskr" -version = "0.19.2" +version = "0.19.3" source = { editable = "." } dependencies = [ { name = "httpx" },