fix(chat): async Gemini API calls with retries, timeout, error mapping, and safety handling - #71
Conversation
…g response
Two bugs fixed in main.py:
1. The /ping endpoint returned a Python set literal instead of a
proper JSON dict, which cannot be serialized. Changed to return
{"status": "ok"} as a proper JSON object.
2. The prompt construction used {ISLAMIC_CONTEXT, request.prompt}
which Python interprets as a tuple, causing the model to receive
a stringified tuple instead of clean text. Fixed by:
- Passing ISLAMIC_CONTEXT as system_instruction to GenerativeModel
- Building the prompt string without the tuple syntax
…d-ping-response fix: resolve tuple artifact in prompt construction and malformed /ping response
- Replace in-memory active_chats dict with Redis-backed SessionStore - Load session history from store on each /chat request, rebuild chat session via Gemini SDK, persist updated history after response - Add in-memory fallback when REDIS_URL is unset (local development) - Session TTL via SESSION_TTL_SECONDS env var (default 86400s / 24h) - Delete endpoint removes session from the store - Config via REDIS_URL env var, documented in README - Add 11 unit tests covering save/load/delete, TTL expiry, history serialization roundtrip, and full lifecycle Closes Deen-Bridge#3
…session-persist feat: persist chat sessions in external store instead of process memory
feat: add post-generation citation verifier with offline Quran corpus~
…ror mapping, and safety handling
WalkthroughThe change adds a Quran corpus and citation verifier, Redis-backed session persistence with an in-memory fallback, asynchronous Gemini generation with retries and mapped errors, async safety-pipeline support, and tests covering citations, sessions, concurrency, failures, and safety blocks. ChangesCitation verification
Session persistence
Chat robustness
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ChatEndpoint
participant Gemini
participant SessionStore
Client->>ChatEndpoint: Submit chat request
ChatEndpoint->>SessionStore: Load chat history
SessionStore-->>ChatEndpoint: Return history
ChatEndpoint->>Gemini: Send message asynchronously
Gemini-->>ChatEndpoint: Return response or transient error
ChatEndpoint->>SessionStore: Save updated history
ChatEndpoint-->>Client: Return response or safe HTTP error
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.py (1)
360-473: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftWire the new helpers into
main.py#L360-L473.SessionStore,extract_and_verify_all, andrun_strict_corrective_loopare still unused, so sessions stay in-memory and generated citations never get checked or corrected before the response is returned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.py` around lines 360 - 473, Wire SessionStore into the chat flow so sessions use persistent storage instead of active_chats in memory; update main.py lines 360-473 and initialization/use sites at lines 218-221 and 331-356, with store.py lines 50-156 providing the existing storage API and requiring no direct change. After generating the response, run extract_and_verify_all and pass citation failures through run_strict_corrective_loop before returning it, using verifier.py lines 61-156 as the existing implementation and requiring no direct change there.
🧹 Nitpick comments (3)
requirements.txt (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider capping the upper bound instead of an open-ended
>=.Loosening
==2.5.3to>=2.5.3with no ceiling means a future pydantic v3 release could be pulled in automatically; given how manyBaseModelschemas this service depends on (ChatResponse,CitationVerificationResult, etc.), an untested major bump landing via a routinepip install -r requirements.txtis a real reproducibility risk.📌 Proposed fix
-pydantic>=2.5.3 +pydantic>=2.5.3,<3.0.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@requirements.txt` at line 5, Update the pydantic requirement from the open-ended lower bound to a bounded range that allows compatible 2.x releases while excluding future major versions such as 3.x. Keep the existing minimum version and preserve compatibility for the service’s BaseModel schemas, including ChatResponse and CitationVerificationResult.safety/pipeline.py (1)
41-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_completeand_complete_asyncduplicate the refuse/guidance-injection/enforcement logic.Only the generator-invocation step (Lines 52-59 vs Line 84) actually differs between the sync and async paths; the refusal short-circuit, guidance-prompt construction, and output-enforcement/result-building are copy-pasted. Worth extracting the shared pre/post steps into a helper both methods call, so future policy changes (e.g. a new decision action) don't need to be kept in sync across two copies by hand.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@safety/pipeline.py` around lines 41 - 95, Extract the shared refusal handling, guidance prompt construction, stage tracking, output enforcement, and result construction from _complete and _complete_async into a common helper. Keep only generator invocation separate so the synchronous and asynchronous methods call the helper with their generated output, preserving existing behavior and stages.main.py (1)
648-681: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift
/chat_streamstill uses blocking sync Gemini calls inside its async generator.Unlike the
/chathandler hardened in this PR,event_generator()here isasync defbut callsmodel.start_chat(syncGenerativeModel) andsend_message(..., stream=True)(sync) with no timeout or retry — the exact "blocking call inside an async endpoint" pattern flagged for the rest of this file. Not part of this diff, but worth tracking so this endpoint gets the same treatment as/chat.As per path instructions, "Flag blocking calls inside async endpoints (network calls should be awaited or offloaded)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.py` around lines 648 - 681, Update the /chat_stream event_generator flow to prevent synchronous Gemini operations from blocking the async generator: offload GenerativeModel.start_chat and the streaming send_message call to an appropriate worker mechanism, and apply the same timeout/retry handling used by the hardened /chat endpoint. Preserve the existing streaming response behavior and context construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@corpus.py`:
- Around line 15-23: Update QuranCorpus._load_corpus to catch JSONDecodeError
and OSError while reading or parsing the corpus file, log a warning, and reset
surahs and ayat to empty mappings so module-level QuranCorpus initialization
cannot crash the application. Preserve the existing successful-load behavior and
missing-file fallback.
In `@main.py`:
- Around line 243-270: Distinguish safety-blocked responses from genuinely empty
responses in extract_text_safely by propagating a dedicated SafetyBlockedError
(or the established equivalent) for safety and prompt-feedback blocks while
retaining None for empty results. Update generate() to catch that condition and
return the required respectful HTTP 200 refusal instead of the empty-response
500, including the expected text and citations_verified fields. Thread the same
outcome through chat() and its ChatResponse construction, while preserving
existing handling for genuinely empty responses and other HTTP exceptions.
- Around line 233-237: Update get_model() to pass
safety_settings=get_safety_settings() when constructing the GenerativeModel,
matching the existing model-construction sites and preserving consistent safety
behavior across chat paths.
- Around line 586-617: Update all five exception handlers in the chat request
error path—ResourceExhausted, InvalidArgument,
DeadlineExceeded/asyncio.TimeoutError, ServiceUnavailable, and the generic
Exception handler—to chain each raised HTTPException from its caught exception
using the appropriate exception cause. Keep the existing status codes and
generic client-facing detail messages unchanged.
In `@store.py`:
- Around line 62-75: Configure finite socket and connection timeouts when
creating the Redis client in SessionStore. Update the _redis_load, _redis_save,
and _redis_delete methods to catch Redis operation failures and preserve the
in-memory fallback behavior, ensuring unreachable or slow Redis calls do not
hang requests or propagate unhandled exceptions.
In `@tests/test_chat_robustness.py`:
- Around line 180-184: Update ChatResponse and its response payload to expose
the citations_verified field so the test can read it and receive True for
verified citations; otherwise remove the assertion from the robustness test if
this field is not part of the intended API contract.
In `@tests/test_session_persistence.py`:
- Around line 63-64: Rename the test methods testhistory_to_dicts_empty and
testdicts_to_contents_returns_protos in TestHistorySerialization to names
beginning with test_, preserving their existing test logic so pytest discovers
and runs both checks.
In `@verifier.py`:
- Around line 19-30: Correct the smart-quote delimiters in both QURAN_REF_REGEX
and HADITH_REF_REGEX so the opening quote class uses “ (U+201C) and the closing
quote class uses ” (U+201D), while preserving the existing ASCII and other quote
support. Ensure typographically quoted citations populate the captured quote
text for corpus verification instead of being treated as NOT_QUOTED.
---
Outside diff comments:
In `@main.py`:
- Around line 360-473: Wire SessionStore into the chat flow so sessions use
persistent storage instead of active_chats in memory; update main.py lines
360-473 and initialization/use sites at lines 218-221 and 331-356, with store.py
lines 50-156 providing the existing storage API and requiring no direct change.
After generating the response, run extract_and_verify_all and pass citation
failures through run_strict_corrective_loop before returning it, using
verifier.py lines 61-156 as the existing implementation and requiring no direct
change there.
---
Nitpick comments:
In `@main.py`:
- Around line 648-681: Update the /chat_stream event_generator flow to prevent
synchronous Gemini operations from blocking the async generator: offload
GenerativeModel.start_chat and the streaming send_message call to an appropriate
worker mechanism, and apply the same timeout/retry handling used by the hardened
/chat endpoint. Preserve the existing streaming response behavior and context
construction.
In `@requirements.txt`:
- Line 5: Update the pydantic requirement from the open-ended lower bound to a
bounded range that allows compatible 2.x releases while excluding future major
versions such as 3.x. Keep the existing minimum version and preserve
compatibility for the service’s BaseModel schemas, including ChatResponse and
CitationVerificationResult.
In `@safety/pipeline.py`:
- Around line 41-95: Extract the shared refusal handling, guidance prompt
construction, stage tracking, output enforcement, and result construction from
_complete and _complete_async into a common helper. Keep only generator
invocation separate so the synchronous and asynchronous methods call the helper
with their generated output, preserving existing behavior and stages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dd0e9cb4-491b-4567-b301-549da0c0682e
📒 Files selected for processing (12)
conftest.pycorpus.pydata/quran_uthmani.jsonmain.pypytest.inirequirements.txtsafety/pipeline.pystore.pytest/test_verifier.pytests/test_chat_robustness.pytests/test_session_persistence.pyverifier.py
| def _load_corpus(self) -> None: | ||
| if self.data_file.exists(): | ||
| with open(self.data_file, "r", encoding="utf-8") as f: | ||
| content = json.load(f) | ||
| self.surahs = content.get("surahs", {}) | ||
| self.ayat = content.get("ayat", {}) | ||
| else: | ||
| self.surahs = {} | ||
| self.ayat = {} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard against a malformed corpus file crashing the whole app at import.
_load_corpus doesn't catch json.JSONDecodeError/OSError. Since corpus = QuranCorpus() (line 42) runs at import time, a single corrupted quran_uthmani.json (bad deploy/manual edit) will crash the entire application on startup, not just the citation feature — much like store.py's Redis connection setup already fails gracefully, this should degrade to an empty corpus with a logged warning.
🛡️ Proposed fix
+import logging
import json
from pathlib import Path
from typing import Dict, Any, Optional
+logger = logging.getLogger(__name__)
DATA_PATH = Path(__file__).parent / "data" / "quran_uthmani.json"
def _load_corpus(self) -> None:
if self.data_file.exists():
- with open(self.data_file, "r", encoding="utf-8") as f:
- content = json.load(f)
- self.surahs = content.get("surahs", {})
- self.ayat = content.get("ayat", {})
+ try:
+ with open(self.data_file, "r", encoding="utf-8") as f:
+ content = json.load(f)
+ self.surahs = content.get("surahs", {})
+ self.ayat = content.get("ayat", {})
+ except (json.JSONDecodeError, OSError) as exc:
+ logger.warning("Failed to load Quran corpus from %s: %s", self.data_file, exc)
+ self.surahs = {}
+ self.ayat = {}
else:
self.surahs = {}
self.ayat = {}Also applies to: 42-42
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 16-16: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(self.data_file, "r", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@corpus.py` around lines 15 - 23, Update QuranCorpus._load_corpus to catch
JSONDecodeError and OSError while reading or parsing the corpus file, log a
warning, and reset surahs and ayat to empty mappings so module-level QuranCorpus
initialization cannot crash the application. Preserve the existing
successful-load behavior and missing-file fallback.
| def get_model(): | ||
| return genai.GenerativeModel( | ||
| model_name="gemini-1.5-flash", | ||
| system_instruction=ISLAMIC_CONTEXT, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
get_model() drops safety_settings, unlike every other model-construction site in this file.
The semantic-cache-hit branch (Line 414-417) and chat_stream (Line 650-653) both pass safety_settings=get_safety_settings(), but get_model() — used for every new session in the main async /chat flow — omits it entirely. This silently changes Gemini's content-safety behavior for the primary chat path versus the cache-hit/streaming paths, and may even be part of why safety-blocked responses are now showing up (different, likely stricter, default thresholds).
🔧 Proposed fix
def get_model():
return genai.GenerativeModel(
model_name="gemini-1.5-flash",
system_instruction=ISLAMIC_CONTEXT,
+ safety_settings=get_safety_settings(),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_model(): | |
| return genai.GenerativeModel( | |
| model_name="gemini-1.5-flash", | |
| system_instruction=ISLAMIC_CONTEXT, | |
| ) | |
| def get_model(): | |
| return genai.GenerativeModel( | |
| model_name="gemini-1.5-flash", | |
| system_instruction=ISLAMIC_CONTEXT, | |
| safety_settings=get_safety_settings(), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main.py` around lines 233 - 237, Update get_model() to pass
safety_settings=get_safety_settings() when constructing the GenerativeModel,
matching the existing model-construction sites and preserving consistent safety
behavior across chat paths.
| def extract_text_safely(response: Any) -> Optional[str]: | ||
| """Safely extract text from Gemini response, handling safety blocks gracefully.""" | ||
| if not response: | ||
| return None | ||
|
|
||
| # Check candidates for finish reason / safety blocks | ||
| if hasattr(response, "candidates") and response.candidates: | ||
| candidate = response.candidates[0] | ||
| finish_reason = getattr(candidate, "finish_reason", None) | ||
| if finish_reason is not None: | ||
| reason_name = getattr(finish_reason, "name", str(finish_reason)).upper() | ||
| if reason_name in ("SAFETY", "BLOCKED", "PROMPT_FEEDBACK", "RECITATION", "SPII"): | ||
| return None | ||
|
|
||
| # Check prompt feedback | ||
| if hasattr(response, "prompt_feedback") and response.prompt_feedback: | ||
| block_reason = getattr(response.prompt_feedback, "block_reason", None) | ||
| if block_reason: | ||
| return None | ||
|
|
||
| # Access text property safely (raises ValueError if response has no text/candidate) | ||
| try: | ||
| text = response.text | ||
| if not text: | ||
| return None | ||
| return text | ||
| except (ValueError, AttributeError): | ||
| return None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Safety-blocked responses raise HTTP 500 instead of the required graceful 200 refusal.
extract_text_safely correctly returns None for both a genuine safety block and a truly empty response, but generate() (Line 463-465) collapses both cases into the same HTTPException(500, "Empty response from AI model"). That directly contradicts this PR's own objective ("Handles safety-blocked responses with a respectful HTTP 200 refusal") and the new test_safety_blocked_response_returns_graceful_200 test, which disables the custom safety pipeline and exercises generate() directly expecting status_code == 200 and a specific refusal text. As written, that test should fail: a finish_reason="SAFETY" mock makes extract_text_safely return None, generate() raises 500, and the outer except HTTPException: raise (Line 610-611) surfaces it unchanged.
You need to distinguish "safety-blocked" from "genuinely empty" so the caller can build the 200 refusal (and populate whatever citations_verified/text fields the response schema expects for that path).
💡 Suggested direction (needs wiring into `ChatResponse`)
+class SafetyBlockedError(Exception):
+ """Raised when Gemini's own safety filter blocks a response."""
+
+
def extract_text_safely(response: Any) -> Optional[str]:
"""Safely extract text from Gemini response, handling safety blocks gracefully."""
if not response:
return None
if hasattr(response, "candidates") and response.candidates:
candidate = response.candidates[0]
finish_reason = getattr(candidate, "finish_reason", None)
if finish_reason is not None:
reason_name = getattr(finish_reason, "name", str(finish_reason)).upper()
if reason_name in ("SAFETY", "BLOCKED", "PROMPT_FEEDBACK", "RECITATION", "SPII"):
- return None
+ raise SafetyBlockedError()
if hasattr(response, "prompt_feedback") and response.prompt_feedback:
block_reason = getattr(response.prompt_feedback, "block_reason", None)
if block_reason:
- return None
+ raise SafetyBlockedError()
try:
text = response.text
if not text:
return None
return text
except (ValueError, AttributeError):
- return None
+ raise SafetyBlockedError()Then in generate(), catch SafetyBlockedError and return the respectful refusal text (200) instead of letting it fall into the "empty response" 500 branch, and thread that outcome through to ChatResponse construction in chat().
Also applies to: 435-466
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main.py` around lines 243 - 270, Distinguish safety-blocked responses from
genuinely empty responses in extract_text_safely by propagating a dedicated
SafetyBlockedError (or the established equivalent) for safety and
prompt-feedback blocks while retaining None for empty results. Update generate()
to catch that condition and return the required respectful HTTP 200 refusal
instead of the empty-response 500, including the expected text and
citations_verified fields. Thread the same outcome through chat() and its
ChatResponse construction, while preserving existing handling for genuinely
empty responses and other HTTP exceptions.
| except ResourceExhausted as exc: | ||
| logger.warning("Gemini rate limit exceeded for chat %s: %s", chat_id, exc) | ||
| raise HTTPException( | ||
| status_code=429, | ||
| detail="Rate limit exceeded. Please try again later.", | ||
| ) | ||
| except InvalidArgument as exc: | ||
| logger.warning("Invalid argument for Gemini call in chat %s: %s", chat_id, exc) | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail="Invalid request parameters.", | ||
| ) | ||
| except (DeadlineExceeded, asyncio.TimeoutError) as exc: | ||
| logger.warning("Gemini API call timed out for chat %s: %s", chat_id, exc) | ||
| raise HTTPException( | ||
| status_code=504, | ||
| detail="AI service timed out.", | ||
| ) | ||
| except ServiceUnavailable as exc: | ||
| logger.warning("Gemini service unavailable for chat %s: %s", chat_id, exc) | ||
| raise HTTPException( | ||
| status_code=503, | ||
| detail="AI service temporarily unavailable.", | ||
| ) | ||
| except HTTPException: | ||
| raise | ||
| except Exception as exc: | ||
| logger.exception("Unexpected error in /chat handler for session %s: %s", chat_id, exc) | ||
| raise HTTPException( | ||
| status_code=500, | ||
| detail="AI service error", | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Chain the original exception with raise ... from exc in all five handlers.
Ruff flags B904 on all five except blocks here. This doesn't leak anything to the client (the detail strings stay generic either way), but per path instructions this repo's CI enforces flake8/lint style checks, and an un-chained raise inside an except is exactly the kind of style violation that can fail the build.
As per path instructions, "CI enforces flake8, so style violations fail the build."
🔧 Proposed fix (repeat pattern for all 5 blocks)
except ResourceExhausted as exc:
logger.warning("Gemini rate limit exceeded for chat %s: %s", chat_id, exc)
raise HTTPException(
status_code=429,
detail="Rate limit exceeded. Please try again later.",
- )
+ ) from exc
except InvalidArgument as exc:
logger.warning("Invalid argument for Gemini call in chat %s: %s", chat_id, exc)
raise HTTPException(
status_code=400,
detail="Invalid request parameters.",
- )
+ ) from exc
except (DeadlineExceeded, asyncio.TimeoutError) as exc:
logger.warning("Gemini API call timed out for chat %s: %s", chat_id, exc)
raise HTTPException(
status_code=504,
detail="AI service timed out.",
- )
+ ) from exc
except ServiceUnavailable as exc:
logger.warning("Gemini service unavailable for chat %s: %s", chat_id, exc)
raise HTTPException(
status_code=503,
detail="AI service temporarily unavailable.",
- )
+ ) from exc
except HTTPException:
raise
except Exception as exc:
logger.exception("Unexpected error in /chat handler for session %s: %s", chat_id, exc)
raise HTTPException(
status_code=500,
detail="AI service error",
- )
+ ) from exc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except ResourceExhausted as exc: | |
| logger.warning("Gemini rate limit exceeded for chat %s: %s", chat_id, exc) | |
| raise HTTPException( | |
| status_code=429, | |
| detail="Rate limit exceeded. Please try again later.", | |
| ) | |
| except InvalidArgument as exc: | |
| logger.warning("Invalid argument for Gemini call in chat %s: %s", chat_id, exc) | |
| raise HTTPException( | |
| status_code=400, | |
| detail="Invalid request parameters.", | |
| ) | |
| except (DeadlineExceeded, asyncio.TimeoutError) as exc: | |
| logger.warning("Gemini API call timed out for chat %s: %s", chat_id, exc) | |
| raise HTTPException( | |
| status_code=504, | |
| detail="AI service timed out.", | |
| ) | |
| except ServiceUnavailable as exc: | |
| logger.warning("Gemini service unavailable for chat %s: %s", chat_id, exc) | |
| raise HTTPException( | |
| status_code=503, | |
| detail="AI service temporarily unavailable.", | |
| ) | |
| except HTTPException: | |
| raise | |
| except Exception as exc: | |
| logger.exception("Unexpected error in /chat handler for session %s: %s", chat_id, exc) | |
| raise HTTPException( | |
| status_code=500, | |
| detail="AI service error", | |
| ) | |
| except ResourceExhausted as exc: | |
| logger.warning("Gemini rate limit exceeded for chat %s: %s", chat_id, exc) | |
| raise HTTPException( | |
| status_code=429, | |
| detail="Rate limit exceeded. Please try again later.", | |
| ) from exc | |
| except InvalidArgument as exc: | |
| logger.warning("Invalid argument for Gemini call in chat %s: %s", chat_id, exc) | |
| raise HTTPException( | |
| status_code=400, | |
| detail="Invalid request parameters.", | |
| ) from exc | |
| except (DeadlineExceeded, asyncio.TimeoutError) as exc: | |
| logger.warning("Gemini API call timed out for chat %s: %s", chat_id, exc) | |
| raise HTTPException( | |
| status_code=504, | |
| detail="AI service timed out.", | |
| ) from exc | |
| except ServiceUnavailable as exc: | |
| logger.warning("Gemini service unavailable for chat %s: %s", chat_id, exc) | |
| raise HTTPException( | |
| status_code=503, | |
| detail="AI service temporarily unavailable.", | |
| ) from exc | |
| except HTTPException: | |
| raise | |
| except Exception as exc: | |
| logger.exception("Unexpected error in /chat handler for session %s: %s", chat_id, exc) | |
| raise HTTPException( | |
| status_code=500, | |
| detail="AI service error", | |
| ) from exc |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 588-591: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 594-597: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 600-603: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 606-609: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 614-617: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main.py` around lines 586 - 617, Update all five exception handlers in the
chat request error path—ResourceExhausted, InvalidArgument,
DeadlineExceeded/asyncio.TimeoutError, ServiceUnavailable, and the generic
Exception handler—to chain each raised HTTPException from its caught exception
using the appropriate exception cause. Keep the existing status codes and
generic client-facing detail messages unchanged.
Sources: Path instructions, Linters/SAST tools
| if REDIS_URL and _redis_available: | ||
| try: | ||
| self._redis = aioredis.from_url( | ||
| REDIS_URL, | ||
| decode_responses=True, | ||
| ) | ||
| self._use_redis = True | ||
| logger.info("SessionStore using Redis at %s", REDIS_URL) | ||
| except Exception as exc: | ||
| logger.warning( | ||
| "Failed to connect to Redis at %s (%s); falling back to in-memory", | ||
| REDIS_URL, | ||
| exc, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Redis calls have no timeout and no per-call error handling — a Redis outage will hang or crash requests.
Only the initial from_url(...) connection attempt at construction is wrapped in try/except; _redis_load/_redis_save/_redis_delete issue bare awaits with no socket_timeout/socket_connect_timeout configured and no exception handling. If Redis becomes unreachable or slow after the app starts, these calls can hang indefinitely or raise unhandled exceptions to whatever calls into SessionStore, defeating the same "external call without a timeout on the request path" hazard this PR is careful to avoid for the Gemini calls.
🔧 Proposed fix
self._redis = aioredis.from_url(
REDIS_URL,
decode_responses=True,
+ socket_timeout=5,
+ socket_connect_timeout=5,
) async def _redis_load(self, chat_id: str) -> list[dict[str, str]]:
- raw = await self._redis.get(f"chat:{chat_id}")
+ try:
+ raw = await self._redis.get(f"chat:{chat_id}")
+ except Exception as exc: # noqa: BLE001
+ logger.warning("Redis load failed for chat %s: %s", chat_id, exc)
+ return []
if raw is None:
return []Also applies to: 109-130
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 70-70: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@store.py` around lines 62 - 75, Configure finite socket and connection
timeouts when creating the Redis client in SessionStore. Update the _redis_load,
_redis_save, and _redis_delete methods to catch Redis operation failures and
preserve the in-memory fallback behavior, ensuring unreachable or slow Redis
calls do not hang requests or propagate unhandled exceptions.
| assert res.status_code == 200 | ||
| data = res.json() | ||
| assert data["text"] == "I cannot fulfill this request due to safety guidelines." | ||
| assert data["citations_verified"] is True | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the field exists on ChatResponse
rg -n 'citations_verified' main.pyRepository: Deen-Bridge/dnb-ai
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the response model and any citations_verified references.
rg -n -C 3 'class ChatResponse|citations_verified' .Repository: Deen-Bridge/dnb-ai
Length of output: 851
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '126,150p' main.pyRepository: Deen-Bridge/dnb-ai
Length of output: 1192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 'ChatResponse\(|citations_verified|model_dump\(|return .*ChatResponse|JSONResponse\(' main.py testsRepository: Deen-Bridge/dnb-ai
Length of output: 1897
citations_verified is missing from ChatResponse
tests/test_chat_robustness.py:183 asserts a field the response model doesn’t define, so this will raise KeyError. Add citations_verified to ChatResponse/the response payload, or drop the assertion if the API shouldn’t expose it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_chat_robustness.py` around lines 180 - 184, Update ChatResponse
and its response payload to expose the citations_verified field so the test can
read it and receive True for verified citations; otherwise remove the assertion
from the robustness test if this field is not part of the intended API contract.
| def testhistory_to_dicts_empty(self): | ||
| assert history_to_dicts([]) == [] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm no custom python_functions override exists that would already collect these
cat pytest.iniRepository: Deen-Bridge/dnb-ai
Length of output: 185
🏁 Script executed:
sed -n '1,140p' tests/test_session_persistence.py | cat -nRepository: Deen-Bridge/dnb-ai
Length of output: 6080
Rename these test methods so pytest collects them. testhistory_to_dicts_empty and testdicts_to_contents_returns_protos don’t match pytest’s test_* discovery pattern, so they won’t run at all. TestHistorySerialization is collected, but these two checks are skipped silently. tests/test_session_persistence.py:63-64, 75-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_session_persistence.py` around lines 63 - 64, Rename the test
methods testhistory_to_dicts_empty and testdicts_to_contents_returns_protos in
TestHistorySerialization to names beginning with test_, preserving their
existing test logic so pytest discovers and runs both checks.
| QURAN_REF_REGEX = re.compile( | ||
| r'(?:Surah|Quran|Qur\'an)?\s*\[?\b([1-9]|[1-9]\d|1[0-0]\d|11[0-4])\s*:\s*([1-9]\d*)\b\]?' | ||
| r'(?:\s*[\"\'«”](.*?)[\"\'»“])?', | ||
| re.IGNORECASE | re.DOTALL | ||
| ) | ||
|
|
||
| HADITH_REF_REGEX = re.compile( | ||
| r'\b(Bukhari|Muslim|Abu Dawud|Tirmidhi|Nasa\'i|Ibn Majah|Muwatta|Ahmad)\b' | ||
| r'\s*(?:hadith|no\.|number|#)?\s*(\d+)?' | ||
| r'(?:\s*[\"\'«”](.*?)[\"\'»“])?', | ||
| re.IGNORECASE | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Smart-quote character classes are reversed, letting fabricated smart-quoted citations slip past verification.
In both QURAN_REF_REGEX and HADITH_REF_REGEX, the opening quote class contains ” (U+201D) and the closing class contains “ (U+201C) — backwards from the standard convention (open with “, close with ”). When Gemini outputs a quote using typographic double quotes, the optional quote group fails to match, quote becomes None, and verify_quran_citation reports NOT_QUOTED instead of comparing against the corpus and catching a mismatch — quietly defeating the anti-hallucination check for the very citation style LLMs tend to produce.
🐛 Proposed fix
QURAN_REF_REGEX = re.compile(
r'(?:Surah|Quran|Qur\'an)?\s*\[?\b([1-9]|[1-9]\d|1[0-0]\d|11[0-4])\s*:\s*([1-9]\d*)\b\]?'
- r'(?:\s*[\"\'«”](.*?)[\"\'»“])?',
+ r'(?:\s*[\"\'«“](.*?)[\"\'»”])?',
re.IGNORECASE | re.DOTALL
)
HADITH_REF_REGEX = re.compile(
r'\b(Bukhari|Muslim|Abu Dawud|Tirmidhi|Nasa\'i|Ibn Majah|Muwatta|Ahmad)\b'
r'\s*(?:hadith|no\.|number|#)?\s*(\d+)?'
- r'(?:\s*[\"\'«”](.*?)[\"\'»“])?',
+ r'(?:\s*[\"\'«“](.*?)[\"\'»”])?',
re.IGNORECASE
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| QURAN_REF_REGEX = re.compile( | |
| r'(?:Surah|Quran|Qur\'an)?\s*\[?\b([1-9]|[1-9]\d|1[0-0]\d|11[0-4])\s*:\s*([1-9]\d*)\b\]?' | |
| r'(?:\s*[\"\'«”](.*?)[\"\'»“])?', | |
| re.IGNORECASE | re.DOTALL | |
| ) | |
| HADITH_REF_REGEX = re.compile( | |
| r'\b(Bukhari|Muslim|Abu Dawud|Tirmidhi|Nasa\'i|Ibn Majah|Muwatta|Ahmad)\b' | |
| r'\s*(?:hadith|no\.|number|#)?\s*(\d+)?' | |
| r'(?:\s*[\"\'«”](.*?)[\"\'»“])?', | |
| re.IGNORECASE | |
| ) | |
| QURAN_REF_REGEX = re.compile( | |
| r'(?:Surah|Quran|Qur\'an)?\s*\[?\b([1-9]|[1-9]\d|1[0-0]\d|11[0-4])\s*:\s*([1-9]\d*)\b\]?' | |
| r'(?:\s*[\"\'«“](.*?)[\"\'»”])?', | |
| re.IGNORECASE | re.DOTALL | |
| ) | |
| HADITH_REF_REGEX = re.compile( | |
| r'\b(Bukhari|Muslim|Abu Dawud|Tirmidhi|Nasa\'i|Ibn Majah|Muwatta|Ahmad)\b' | |
| r'\s*(?:hadith|no\.|number|#)?\s*(\d+)?' | |
| r'(?:\s*[\"\'«“](.*?)[\"\'»”])?', | |
| re.IGNORECASE | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@verifier.py` around lines 19 - 30, Correct the smart-quote delimiters in both
QURAN_REF_REGEX and HADITH_REF_REGEX so the opening quote class uses “ (U+201C)
and the closing quote class uses ” (U+201D), while preserving the existing ASCII
and other quote support. Ensure typographically quoted citations populate the
captured quote text for corpus verification instead of being treated as
NOT_QUOTED.
4 participants
Closes #7
Description
What's Happening
The
/chathandler had three related robustness problems around the Gemini call:async def chat(...)but called the synchronouschat.send_message(...).except Exceptionwrappedstr(e)into the HTTP detail and returned it to the client.What We Changed
send_messagewithawait chat.send_message_async(...)so the event loop is never blocked.request_options={"timeout": 30}) and exponential backoff retries for transient upstream errors (ServiceUnavailable,DeadlineExceeded,TimeoutError).chat.historyon failed calls before retrying to prevent duplicate user turns.ResourceExhausted-> HTTP 429 (Rate limit exceeded. Please try again later.)InvalidArgument-> HTTP 400 (Invalid request parameters.)DeadlineExceeded/TimeoutError-> HTTP 504 (AI service timed out.)ServiceUnavailable-> HTTP 503 (AI service temporarily unavailable.)AI service error), logged server-side without leaking raw error text or emojis.I cannot fulfill this request due to safety guidelines.).tests/test_chat_robustness.py.Summary by CodeRabbit
New Features
Bug Fixes
Tests