From 0c63ead53404b6c15f027968634126c3a36d420b Mon Sep 17 00:00:00 2001 From: Himanshu Dongre Date: Sat, 23 May 2026 18:09:27 +0530 Subject: [PATCH] Stop swallowing chat-title generation failures silently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The /api/v4/chat/sessions/{id}/title endpoint wrapped its entire body in a bare `except Exception: pass`. When the background LLM provider was missing or the call failed, the endpoint silently returned the unchanged session — clients could not tell "title not requested yet" apart from "tried and failed because no provider." Same anti-pattern as the extract bug (713ed9a), lower severity (titles are cosmetic). This is the smallest correct fix: - Title generation is still best-effort. The endpoint always returns 200 with the session. The chat surface is never blocked. - Failures are no longer silent. The bare except is replaced with three typed branches: * ProviderNotConfiguredError → WARNING log + status "skipped:provider_not_configured" * generic provider call error → WARNING log + status "skipped:provider_error" * DB write error after successful generation → ERROR log + status "skipped:db_error" (also rolls back the session) - SessionResponse gains an optional `title_generation_status: str` field (default None). The title endpoint sets it to one of the four enum values above ("ok" on success). Every other endpoint that returns SessionResponse continues to return null for this field — additive, no client-breaking change. - Logs carry session_id, provider, model, exception type, and message — enough to diagnose, no secrets (provider SDK errors do not place API keys in str(e); the test asserts no "sk-" / "bearer " patterns). Chat-send is not touched. Title generation is only invoked by the dedicated /title endpoint (verified by grep), so chat-send was already independent of this code path and remains independent. Tests (new file backend/tests/integration/test_chat_title.py): - test_title_generation_succeeds_with_provider: pins the happy path (status "ok", no WARNING/ERROR noise). - test_title_generation_fails_loud_without_provider: regression test for the bare-except bug — asserts 200, session title unchanged, status "skipped:provider_not_configured", exactly one WARNING log with diagnostic context, no exception bubbles. - test_title_generation_handles_provider_call_error: pins the generic provider-error branch. - test_title_endpoint_400_when_no_turns: pins the existing precondition so the typed-except rewrite doesn't accidentally swallow it. - test_other_session_endpoints_omit_title_generation_status: confirms the additive field is null on other SessionResponse endpoints. Full backend integration suite: 170 passed locally (was 165 + 5 new). --- backend/app/api/routes/chat.py | 89 ++++++-- backend/tests/integration/test_chat_title.py | 208 +++++++++++++++++++ 2 files changed, 285 insertions(+), 12 deletions(-) create mode 100644 backend/tests/integration/test_chat_title.py diff --git a/backend/app/api/routes/chat.py b/backend/app/api/routes/chat.py index 1ab0dfa..771f423 100644 --- a/backend/app/api/routes/chat.py +++ b/backend/app/api/routes/chat.py @@ -21,10 +21,13 @@ from __future__ import annotations import hashlib import json +import logging import uuid from datetime import datetime, timezone from typing import Optional +logger = logging.getLogger(__name__) + from fastapi import APIRouter, Depends, HTTPException, Query, Response from pydantic import BaseModel, Field from sqlalchemy import select @@ -211,6 +214,14 @@ class SessionResponse(BaseModel): branch_name: str created_at: datetime updated_at: datetime + # Set only by the /sessions/{id}/title endpoint. Stable enum-like values: + # "ok" — title was generated and saved + # "skipped:provider_not_configured" — no background LLM provider configured + # "skipped:provider_error" — provider call failed (network/4xx/etc.) + # "skipped:db_error" — DB write failed after generation + # All other endpoints return SessionResponse with this field unset (null), + # so clients that don't know about it are unaffected. + title_generation_status: Optional[str] = None model_config = {"from_attributes": True} @@ -499,7 +510,16 @@ def get_session_generic(session_id: uuid.UUID, db: Session = Depends(get_db)): @router.post("/sessions/{session_id}/title", response_model=SessionResponse) def generate_session_title(session_id: uuid.UUID, db: Session = Depends(get_db)): - """Generate a meaningful title for a session using the background intelligence model.""" + """Generate a meaningful title for a session using the background intelligence model. + + Always returns 200 with the session — title generation is best-effort and + must never block the chat surface. Failure modes are surfaced via + `title_generation_status` (stable enum) and a single WARNING/ERROR log + line with diagnostic context (session id, provider, model, exception + type/message). Pre this fix, failures were swallowed silently by a bare + `except Exception: pass`, so a misconfigured provider was indistinguishable + from "title not requested yet." + """ session = db.get(ChatSession, session_id) if not session: raise HTTPException(status_code=404, detail="Session not found") @@ -520,23 +540,68 @@ def generate_session_title(session_id: uuid.UUID, db: Session = Depends(get_db)) f"{transcript}\n\nTitle:" ) + cfg = get_config() + bg_provider = cfg.background.provider + bg_model = cfg.background.model + + # Resolve adapter. Provider-not-configured is the most common failure path + # — make it visible (WARNING + status), never silent. Returns the session + # unchanged so the chat surface is not blocked. try: - cfg = get_config() - bg_provider = cfg.background.provider - bg_model = cfg.background.model adapter = get_adapter(bg_provider, allow_mock=False) + except ProviderNotConfiguredError as e: + logger.warning( + "session_title: skipped — provider not configured " + "(session_id=%s provider=%s model=%s exc=%s: %s)", + session_id, bg_provider, bg_model, type(e).__name__, e, + ) + response = SessionResponse.model_validate(session) + response.title_generation_status = "skipped:provider_not_configured" + return response + + # Provider call. Network errors, 4xx/5xx from the LLM, JSON parse, etc. + # Logged at WARNING — the chat surface is not blocked. We log the + # exception type and str(e); provider SDK errors do not place the API + # key in str(e), so no secret leakage. session_id, provider, model give + # enough context to diagnose without dumping the raw request. + try: raw_title = adapter.send([{"role": "user", "content": prompt}], model=bg_model).strip() - title = raw_title.strip("\"'").strip() - if len(title) > 60: - title = title[:60] - session.title = title - session.updated_at = _utcnow() + except Exception as e: + logger.warning( + "session_title: skipped — provider call failed " + "(session_id=%s provider=%s model=%s exc=%s: %s)", + session_id, bg_provider, bg_model, type(e).__name__, e, + ) + response = SessionResponse.model_validate(session) + response.title_generation_status = "skipped:provider_error" + return response + + title = raw_title.strip("\"'").strip() + if len(title) > 60: + title = title[:60] + session.title = title + session.updated_at = _utcnow() + + # DB write. A failure here is a real bug (LLM call already succeeded), + # so log at ERROR. Still return 200 with the unchanged in-memory session + # so the chat surface is not blocked. The next call will retry. + try: db.commit() db.refresh(session) - except Exception: - pass # Return unchanged session if title generation fails + except Exception as e: + db.rollback() + logger.error( + "session_title: skipped — db write failed after successful generation " + "(session_id=%s provider=%s model=%s exc=%s: %s)", + session_id, bg_provider, bg_model, type(e).__name__, e, + ) + response = SessionResponse.model_validate(session) + response.title_generation_status = "skipped:db_error" + return response - return session + response = SessionResponse.model_validate(session) + response.title_generation_status = "ok" + return response @router.get("/sessions/{session_id}/turns", response_model=list[TurnResponse]) diff --git a/backend/tests/integration/test_chat_title.py b/backend/tests/integration/test_chat_title.py new file mode 100644 index 0000000..d54ab32 --- /dev/null +++ b/backend/tests/integration/test_chat_title.py @@ -0,0 +1,208 @@ +"""Integration tests for POST /api/v4/chat/sessions/{id}/title. + +Pre-fix behavior: a bare `except Exception: pass` swallowed every failure +(provider not configured, provider call errors, DB errors). The endpoint +always returned 200, and clients could not distinguish "never asked for +a title yet" from "tried and failed because no provider." + +Post-fix contract (this file pins it): + - Endpoint still always returns 200 with the session (chat surface is + never blocked by title generation). + - A new response field `title_generation_status` carries a stable + enum-like value: "ok" | "skipped:provider_not_configured" | + "skipped:provider_error" | "skipped:db_error". + - On failure, a single WARNING/ERROR log line is emitted with + session_id, provider, model, exception type, and message — enough + to diagnose without leaking secrets. + - No exception bubbles out of the handler. +""" + +import logging +import pytest + +from app.config_loader import ProviderNotConfiguredError +from app.providers.base import ProviderAdapter +from app.api.routes import chat as chat_route + + +# ── helpers (kept inline so this file stands alone) ────────────────────────── + + +def _create_repo(client, name="Chat Title Test Repo"): + r = client.post("/api/v2/repos", json={"name": name}) + assert r.status_code == 201, r.text + return r.json()["id"] + + +def _create_session(client, repo_id, title="default"): + r = client.post(f"/api/v4/chat/spaces/{repo_id}/sessions", json={ + "title": title, "provider": "openrouter", "model": "mock", + }) + assert r.status_code == 201, r.text + return r.json()["id"] + + +def _send_one_turn(client, session_id, message="Hello world"): + """Send a single turn via the mock adapter so /title has something to + summarize. Uses use_mock=True so this fixture requires no provider key.""" + r = client.post("/api/v4/chat/send", json={ + "session_id": session_id, + "provider": "openrouter", + "model": "mock", + "message": message, + "use_mock": True, + }) + assert r.status_code == 200, r.text + + +def _setup_session_with_turns(client, original_title="Original Title"): + """Create a repo + session + one turn so the title endpoint has input.""" + repo_id = _create_repo(client) + session_id = _create_session(client, repo_id, title=original_title) + _send_one_turn(client, session_id, "Let's discuss the refactor plan.") + return session_id + + +# ── Fake adapters for monkeypatching get_adapter ───────────────────────────── + + +class _GoodAdapter(ProviderAdapter): + """Returns a fixed title string.""" + def __init__(self, title: str = "My Refactor Plan"): + self._title = title + + def send(self, messages, model, **kwargs): + return self._title + + def healthcheck(self) -> bool: # pragma: no cover + return True + + +class _ErroringAdapter(ProviderAdapter): + """Raises on send() — simulates network/API failure.""" + def send(self, messages, model, **kwargs): + raise RuntimeError("simulated provider failure (no API key in this message)") + + def healthcheck(self) -> bool: # pragma: no cover + return True + + +# ── Success path ───────────────────────────────────────────────────────────── + + +def test_title_generation_succeeds_with_provider(client, monkeypatch, caplog): + """When a real provider answers, the title is saved and status == 'ok'. + No WARNING/ERROR log noise on the happy path.""" + monkeypatch.setattr(chat_route, "get_adapter", lambda p, allow_mock=False: _GoodAdapter("My Refactor Plan")) + + session_id = _setup_session_with_turns(client, original_title="default") + + with caplog.at_level(logging.WARNING, logger="app.api.routes.chat"): + r = client.post(f"/api/v4/chat/sessions/{session_id}/title") + + assert r.status_code == 200, r.text + data = r.json() + assert data["title"] == "My Refactor Plan" + assert data["title_generation_status"] == "ok" + # No WARNING/ERROR on happy path + assert not [rec for rec in caplog.records if rec.levelno >= logging.WARNING] + + +# ── Failure path — provider not configured (the regression test) ───────────── + + +def test_title_generation_fails_loud_without_provider(client, monkeypatch, caplog): + """No provider configured: returns 200 with the session UNCHANGED, + status == 'skipped:provider_not_configured', and a WARNING log line + carrying session_id, provider, model, and exception type/message. + + This is the regression test for the bare-except bug: pre-fix, the + endpoint silently returned the unchanged session with no signal. + """ + def _raise_not_configured(provider, allow_mock=False): + raise ProviderNotConfiguredError( + f"Provider '{provider}' has no API key configured." + ) + monkeypatch.setattr(chat_route, "get_adapter", _raise_not_configured) + + session_id = _setup_session_with_turns(client, original_title="My Sticky Title") + + with caplog.at_level(logging.WARNING, logger="app.api.routes.chat"): + r = client.post(f"/api/v4/chat/sessions/{session_id}/title") + + # No exception bubbled — endpoint still returns 200. + assert r.status_code == 200, r.text + data = r.json() + # Title is UNCHANGED (the chat surface is never blocked / corrupted). + assert data["title"] == "My Sticky Title" + # Status is the stable enum so the UI can switch on it. + assert data["title_generation_status"] == "skipped:provider_not_configured" + + # WARNING was logged with enough context to diagnose. + warnings = [r for r in caplog.records if r.levelno == logging.WARNING] + assert len(warnings) == 1, f"expected exactly one WARNING, got {warnings}" + msg = warnings[0].getMessage() + assert "session_title" in msg + assert "provider not configured" in msg + assert str(session_id) in msg + assert "ProviderNotConfiguredError" in msg + # Sanity: no obvious secret leakage patterns (api keys, bearer tokens). + assert "sk-" not in msg.lower() + assert "bearer " not in msg.lower() + + +# ── Failure path — provider call errors ────────────────────────────────────── + + +def test_title_generation_handles_provider_call_error(client, monkeypatch, caplog): + """Provider call raises a generic exception (network, 4xx, parse error, etc.): + returns 200 with the session unchanged, status == 'skipped:provider_error', + WARNING log line. No exception bubbles.""" + monkeypatch.setattr(chat_route, "get_adapter", lambda p, allow_mock=False: _ErroringAdapter()) + + session_id = _setup_session_with_turns(client, original_title="My Sticky Title") + + with caplog.at_level(logging.WARNING, logger="app.api.routes.chat"): + r = client.post(f"/api/v4/chat/sessions/{session_id}/title") + + assert r.status_code == 200, r.text + data = r.json() + assert data["title"] == "My Sticky Title" + assert data["title_generation_status"] == "skipped:provider_error" + + warnings = [r for r in caplog.records if r.levelno == logging.WARNING] + assert len(warnings) == 1 + msg = warnings[0].getMessage() + assert "provider call failed" in msg + assert "RuntimeError" in msg + assert str(session_id) in msg + + +# ── Pre-condition preserved: no turns is still a 400 ───────────────────────── + + +def test_title_endpoint_400_when_no_turns(client): + """Session with zero turns still returns 400 (genuine error, not silent). + The new typed-except code path must not accidentally turn this into a 200.""" + repo_id = _create_repo(client) + session_id = _create_session(client, repo_id) + # Intentionally no turns + + r = client.post(f"/api/v4/chat/sessions/{session_id}/title") + assert r.status_code == 400, r.text + + +# ── Other endpoints that return SessionResponse are unaffected ─────────────── + + +def test_other_session_endpoints_omit_title_generation_status(client): + """The new field defaults to None on every endpoint that returns + SessionResponse other than /title. Pinned so we never accidentally leak + a stale status from a previous call.""" + repo_id = _create_repo(client) + session_id = _create_session(client, repo_id) + r = client.get(f"/api/v4/chat/sessions/{session_id}") + assert r.status_code == 200, r.text + data = r.json() + # Field is present (additive) but null for non-title endpoints. + assert data.get("title_generation_status") is None