mirror of
https://github.com/himanshudongre/smriti.git
synced 2026-08-28 05:14:59 +00:00
Stop swallowing chat-title generation failures silently
Some checks failed
Deploy website to GitHub Pages / deploy (push) Has been cancelled
Some checks failed
Deploy website to GitHub Pages / deploy (push) Has been cancelled
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).
This commit is contained in:
parent
713ed9a007
commit
0c63ead534
2 changed files with 285 additions and 12 deletions
|
|
@ -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])
|
||||
|
|
|
|||
208
backend/tests/integration/test_chat_title.py
Normal file
208
backend/tests/integration/test_chat_title.py
Normal file
|
|
@ -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
|
||||
Loading…
Add table
Reference in a new issue