From f816d44438863ee81340e29271a380ad05b75d9f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 26 Apr 2026 21:55:53 -0400 Subject: [PATCH] fix: typed ChatNotFoundError replaces string-prefix sniff in skip routes (T81) --- chat/web/drawer.py | 19 +++++++----- chat/web/skip.py | 27 +++++++++++++---- chat/web/turns.py | 18 ++++++++---- tests/test_drawer_events_threads_skip.py | 37 ++++++++++++++++++++++++ 4 files changed, 82 insertions(+), 19 deletions(-) diff --git a/chat/web/drawer.py b/chat/web/drawer.py index 1098eb5..bcfdc0d 100644 --- a/chat/web/drawer.py +++ b/chat/web/drawer.py @@ -48,6 +48,7 @@ from chat.state.world import active_scene, get_activity, get_chat, get_container from chat.web.bots import get_conn from chat.web.kickoff import get_llm_client from chat.web.skip import ( + ChatNotFoundError, _now_iso, process_elision_skip, process_jump_skip, @@ -993,13 +994,12 @@ async def skip_elision( new_time=new_time, landing_state_hint=landing_state_hint, ) + except ChatNotFoundError as exc: + # Missing chat row: typed exception (T81) replaces the prior + # ``str(exc).startswith("chat not found")`` prefix sniff. + raise HTTPException(status_code=404, detail=str(exc)) except ValueError as exc: - # ``process_elision_skip`` raises on missing-chat or malformed / - # backwards new_time. The drawer used to 404 / 400 these - # separately — preserve the 404-vs-400 split by sniffing the - # error message so existing tests keep passing without changes. - if str(exc).startswith("chat not found"): - raise HTTPException(status_code=404, detail=str(exc)) + # Input-validation failure (malformed or backwards new_time). raise HTTPException(status_code=400, detail=str(exc)) return await drawer(chat_id, request, conn) @@ -1037,9 +1037,12 @@ async def skip_jump( notable_prose=notable_prose, reset_activity=reset_flag, ) + except ChatNotFoundError as exc: + # Missing chat row: typed exception (T81) replaces the prior + # ``str(exc).startswith("chat not found")`` prefix sniff. + raise HTTPException(status_code=404, detail=str(exc)) except ValueError as exc: - if str(exc).startswith("chat not found"): - raise HTTPException(status_code=404, detail=str(exc)) + # Input-validation failure (malformed or backwards new_time). raise HTTPException(status_code=400, detail=str(exc)) return await drawer(chat_id, request, conn) diff --git a/chat/web/skip.py b/chat/web/skip.py index ccbf470..b6aa179 100644 --- a/chat/web/skip.py +++ b/chat/web/skip.py @@ -36,6 +36,17 @@ from chat.state.entities import get_bot, get_you from chat.state.world import get_activity, get_chat +class ChatNotFoundError(Exception): + """Raised when a ``chat_id`` doesn't resolve to a chat row. + + Distinguishes the missing-chat case from generic input-validation + failures (which still raise :class:`ValueError`). HTTP callers map + this to ``404`` and ``ValueError`` to ``400`` — replacing the + earlier ``str(exc).startswith("chat not found")`` prefix sniff + (T81) with a typed dispatch. + """ + + def _parse_iso_time(value: str) -> datetime | None: """Permissive ISO 8601 parser shared with the drawer routes (T59). @@ -93,13 +104,14 @@ async def process_elision_skip( ..., "assistant_event_id": ...}`` so callers can introspect the generated turn (e.g. for SSE rebroadcast or test assertions). - Raises ``ValueError`` on validation failure or when the chat row - can't be located (the drawer maps it to ``HTTP 400`` / ``404`` - respectively; the natural-language path follows the same shape). + Raises :class:`ChatNotFoundError` when the chat row is missing + (HTTP ``404``) and ``ValueError`` on input-validation failure + (HTTP ``400``). Splitting the two lets the drawer route dispatch + on type instead of sniffing the error string (T81). """ chat = get_chat(conn, chat_id) if chat is None: - raise ValueError(f"chat not found: {chat_id}") + raise ChatNotFoundError(f"chat not found: {chat_id}") _validate_new_time(chat, new_time) @@ -178,11 +190,13 @@ async def process_jump_skip( Returns ``{"assistant_text": ..., "speaker_id": ..., "skip_event_id": ..., "assistant_event_id": ...}``. - Raises ``ValueError`` on validation failure (caller maps to ``400``). + Raises :class:`ChatNotFoundError` on missing chat (caller maps to + ``404``) and ``ValueError`` on input-validation failure (caller maps + to ``400``). """ chat = get_chat(conn, chat_id) if chat is None: - raise ValueError(f"chat not found: {chat_id}") + raise ChatNotFoundError(f"chat not found: {chat_id}") _validate_new_time(chat, new_time) @@ -280,6 +294,7 @@ def _now_iso() -> str: __all__ = [ + "ChatNotFoundError", "process_elision_skip", "process_jump_skip", "_now_iso", diff --git a/chat/web/turns.py b/chat/web/turns.py index 3fc51d4..0dd17b5 100644 --- a/chat/web/turns.py +++ b/chat/web/turns.py @@ -79,7 +79,11 @@ from chat.web.kickoff import get_llm_client from chat.web.meanwhile import process_meanwhile_turn from chat.web.pubsub import publish from chat.web.render import render_turn_html as _render_turn_html -from chat.web.skip import _parse_iso_time, process_elision_skip +from chat.web.skip import ( + ChatNotFoundError, + _parse_iso_time, + process_elision_skip, +) router = APIRouter() @@ -333,11 +337,15 @@ async def post_turn( landing_state_hint=getattr(parsed, "landing_state_hint", "") or "", ) + except ChatNotFoundError as exc: + # Defensive: chat existence is checked above, so this only + # fires on a TOCTOU race where the chat row is deleted + # mid-request. T81 split the typed missing-chat case out of + # the generic ValueError so we keep the 404 mapping here. + raise HTTPException(status_code=404, detail=str(exc)) except ValueError as exc: - # The controller raises on missing chat / bad new_time. - # Missing chat is already handled above (we'd have 404'd); - # a bad new_time here is a stub-derivation bug rather than - # user input — surface as 400 with the controller message. + # Bad new_time is a stub-derivation bug rather than user + # input — surface as 400 with the controller message. raise HTTPException(status_code=400, detail=str(exc)) return Response(status_code=204) diff --git a/tests/test_drawer_events_threads_skip.py b/tests/test_drawer_events_threads_skip.py index ab1dafd..a621c2b 100644 --- a/tests/test_drawer_events_threads_skip.py +++ b/tests/test_drawer_events_threads_skip.py @@ -273,6 +273,43 @@ def test_post_skip_elision_advances_clock_and_emits_narration(client, tmp_path): assert tp["speaker_id"] == "bot_a" +def test_skip_route_404_via_typed_exception_class(client, tmp_path): + """T81: drawer skip routes 404 via :class:`ChatNotFoundError`. + + Pre-T81, the route caught ``ValueError`` and recovered the 404 case + by sniffing ``str(exc).startswith("chat not found")`` — fragile if + the message ever changed wording. The controller now raises a typed + exception so the route dispatches on type. Asserting the 404 from + the unseeded chat exercises the typed branch end-to-end; importing + the class confirms it's a real subclass of ``Exception`` and not a + re-export of ``ValueError`` (which would defeat the type split). + """ + # Don't seed any chat — the controller hits ``get_chat`` returning + # ``None`` and raises ``ChatNotFoundError``. The drawer route then + # maps that to ``404`` via the typed handler (no string sniff). + _override_llm([]) + try: + response = client.post( + "/chats/nonexistent/drawer/skip/elision", + data={ + "landing_state_hint": "x", + "new_time": "2026-04-26T20:30:00+00:00", + }, + ) + assert response.status_code == 404 + finally: + app.dependency_overrides.clear() + + # The exception class itself is importable, distinct from ValueError, + # and a proper Exception subclass — pinning the type-based dispatch + # so future refactors can't quietly collapse it back to a string sniff. + from chat.web.skip import ChatNotFoundError + + assert ChatNotFoundError is not None + assert issubclass(ChatNotFoundError, Exception) + assert not issubclass(ChatNotFoundError, ValueError) + + def test_post_skip_elision_invalid_time_returns_400(client, tmp_path): _seed_chat(tmp_path / "test.db") _override_llm([])