feat: cap conversation history with token budget to prevent unbounded prompt growth - #49
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
… prompt growth - Add MAX_HISTORY_TOKENS (default 16000) and MAX_HISTORY_TURNS (default 50) env config - Trim oldest turn-pairs at pair granularity before each Gemini call - Surface truncated boolean in ChatResponse - Add include_history flag to ChatRequest (default true) to omit payload bloat - Use cheap local estimate (chars/4) avoiding extra network calls - Document new env vars in README - Add 13 unit tests covering truncation boundary, pair-granularity invariant, and env configurability Closes Deen-Bridge#13
WalkthroughThis PR adds configurable chat-history trimming, confidence and scholar-review workflows, hadith grading, fiqh classification, semantic caching, study generation, and layered safety enforcement, with expanded documentation, dependencies, CI coverage, and offline tests. ChangesChat intelligence and response controls
Study content generation
Layered content safety
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
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: 6
🤖 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 `@history.py`:
- Around line 13-14: Validate MAX_HISTORY_TOKENS and MAX_HISTORY_TURNS while
parsing their environment values, requiring non-negative integers and raising a
clear configuration error for invalid or negative values. Keep the existing
defaults and history-processing behavior unchanged for valid settings.
- Around line 40-44: Update the history token calculation around estimate_tokens
to iterate over every part in each message and sum the tokens from all parts
that expose text, rather than only parts[0]. Preserve the zero-token behavior
for messages or parts without text, and add a regression test covering multipart
Gemini messages exceeding MAX_HISTORY_TOKENS.
In `@main.py`:
- Around line 145-147: Update the async handler around trim_history and the
synchronous chat.send_message call to use the SDK’s asynchronous API or
asyncio.to_thread, preventing model responses from blocking the event loop.
Preserve the existing per-chat serialization behavior while offloading only the
blocking Gemini request.
In `@tests/test_history_truncation.py`:
- Around line 169-171: Update test_budget_configurable_via_env to set the
relevant environment variables with monkeypatch and reload the history module
before invoking trim_history. Remove the direct patches to
history.MAX_HISTORY_TOKENS and history.MAX_HISTORY_TURNS, and call trim_history
from the reloaded module so the test validates environment parsing.
- Around line 115-148: Strengthen test_no_orphaned_turns_after_truncation by
asserting every retained history pair is ordered user then model, not only that
the length is even. In test_preserves_recent_context_after_truncation, replace
the nonexistent "Old question 1" marker with content actually present in the
fixture, such as "very old" or the repeated C marker, and assert that old
content was removed while recent context remains.
- Line 8: Add pytest as an explicitly pinned dependency in requirements.txt so
tests/test_history_truncation.py can import it in clean CI environments.
🪄 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: f1cb4c63-2164-4479-8a8e-ca1b43a9ccb0
📒 Files selected for processing (4)
README.mdhistory.pymain.pytests/test_history_truncation.py
| MAX_HISTORY_TOKENS = int(os.getenv("MAX_HISTORY_TOKENS", "16000")) | ||
| MAX_HISTORY_TURNS = int(os.getenv("MAX_HISTORY_TURNS", "50")) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject invalid history limits at startup.
A negative MAX_HISTORY_TURNS makes the loop at Line 34 remain true after history is emptied, so the next pair-pop raises IndexError. Negative token limits also silently discard all history. Parse these settings as non-negative integers and raise a clear configuration error otherwise.
Proposed fix
+def _read_non_negative_int(name: str, default: int) -> int:
+ value = int(os.getenv(name, str(default)))
+ if value < 0:
+ raise ValueError(f"{name} must be non-negative")
+ return value
+
-MAX_HISTORY_TOKENS = int(os.getenv("MAX_HISTORY_TOKENS", "16000"))
-MAX_HISTORY_TURNS = int(os.getenv("MAX_HISTORY_TURNS", "50"))
+MAX_HISTORY_TOKENS = _read_non_negative_int("MAX_HISTORY_TOKENS", 16000)
+MAX_HISTORY_TURNS = _read_non_negative_int("MAX_HISTORY_TURNS", 50)📝 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.
| MAX_HISTORY_TOKENS = int(os.getenv("MAX_HISTORY_TOKENS", "16000")) | |
| MAX_HISTORY_TURNS = int(os.getenv("MAX_HISTORY_TURNS", "50")) | |
| def _read_non_negative_int(name: str, default: int) -> int: | |
| value = int(os.getenv(name, str(default))) | |
| if value < 0: | |
| raise ValueError(f"{name} must be non-negative") | |
| return value | |
| MAX_HISTORY_TOKENS = _read_non_negative_int("MAX_HISTORY_TOKENS", 16000) | |
| MAX_HISTORY_TURNS = _read_non_negative_int("MAX_HISTORY_TURNS", 50) |
🤖 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 `@history.py` around lines 13 - 14, Validate MAX_HISTORY_TOKENS and
MAX_HISTORY_TURNS while parsing their environment values, requiring non-negative
integers and raising a clear configuration error for invalid or negative values.
Keep the existing defaults and history-processing behavior unchanged for valid
settings.
| total = sum( | ||
| estimate_tokens(m.parts[0].text) | ||
| if hasattr(m, "parts") and m.parts and hasattr(m.parts[0], "text") | ||
| else 0 | ||
| for m in chat_session.history |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and inspect the relevant section with line numbers.
fd -a '^history\.py$' .Repository: Deen-Bridge/dnb-ai
Length of output: 186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the target file and relevant tests, then inspect the surrounding code.
python3 - <<'PY'
import os
from pathlib import Path
matches = []
for p in Path('.').rglob('history.py'):
matches.append(str(p))
print("HISTORY_FILES:")
for m in matches:
print(m)
print("\nTEST_FILES:")
for p in Path('.').rglob('*test*'):
if p.is_file() and p.suffix in {'.py', '.ts', '.js'}:
if 'history' in p.name.lower() or 'token' in p.name.lower() or 'gemini' in p.name.lower():
print(p)
PYRepository: Deen-Bridge/dnb-ai
Length of output: 228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg '(^|/)history\.py$|test|gemini|token'Repository: Deen-Bridge/dnb-ai
Length of output: 518
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and inspect the relevant section with line numbers.
file=$(git ls-files | rg '(^|/)history\.py$' | head -n 1)
echo "FILE=$file"
if [ -n "$file" ]; then
nl -ba "$file" | sed -n '1,140p'
fi
echo
echo "TESTS:"
git ls-files | rg '(^|/).*(test|spec).*\.(py|js|ts)$|(^|/)tests?/' | rg 'history|token|gemini|chat'Repository: Deen-Bridge/dnb-ai
Length of output: 212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(git ls-files | rg '(^|/)history\.py$' | head -n 1)
echo "FILE=$file"
nl -ba "$file" | sed -n '1,160p'Repository: Deen-Bridge/dnb-ai
Length of output: 212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "hello"Repository: Deen-Bridge/dnb-ai
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [Path('history.py'), Path('tests/test_history_truncation.py')]:
print(f"\n### {path} ###")
text = path.read_text()
lines = text.splitlines()
# Print all lines with numbers if reasonably small, otherwise first 220 lines.
start = 1
end = min(len(lines), 260)
for i in range(start, end + 1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: Deen-Bridge/dnb-ai
Length of output: 9875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the specific logic around history truncation and token counting.
python3 - <<'PY'
from pathlib import Path
for file in ['history.py', 'tests/test_history_truncation.py']:
p = Path(file)
print(f"\nFILE: {file}")
for i, line in enumerate(p.read_text().splitlines(), 1):
if 'estimate_tokens' in line or 'parts' in line or 'history' in line or 'truncate' in line or 'token' in line:
print(f"{i:4d}: {line}")
PYRepository: Deen-Bridge/dnb-ai
Length of output: 4907
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [Path('history.py'), Path('tests/test_history_truncation.py')]:
print(f"\n### {path} ###")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if i <= 220:
print(f"{i:4d}: {line}")
PYRepository: Deen-Bridge/dnb-ai
Length of output: 9875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for file in ('history.py', 'tests/test_history_truncation.py'):
p = Path(file)
print(f"\n=== {file} ===")
for i, line in enumerate(p.read_text().splitlines(), 1):
if i <= 220:
print(f"{i:4d}: {line}")
PYRepository: Deen-Bridge/dnb-ai
Length of output: 9875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for file in ('history.py', 'tests/test_history_truncation.py'):
p = Path(file)
print(f"\n=== {file} ===")
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if i <= 220:
print(f"{i:4d}: {line}")
PYRepository: Deen-Bridge/dnb-ai
Length of output: 9875
Count all text parts in history trimming. Lines 40-44 only budget parts[0], so multipart Gemini messages can slip past MAX_HISTORY_TOKENS. Sum every text part and add a multipart regression test.
🤖 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 `@history.py` around lines 40 - 44, Update the history token calculation around
estimate_tokens to iterate over every part in each message and sum the tokens
from all parts that expose text, rather than only parts[0]. Preserve the
zero-token behavior for messages or parts without text, and add a regression
test covering multipart Gemini messages exceeding MAX_HISTORY_TOKENS.
| # Trim history to stay within budget (oldest turn-pairs dropped) | ||
| truncated = trim_history(chat) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and the relevant symbols.
git ls-files | rg '^main\.py$|(^|/)main\.py$|(^|/)requirements\.txt$|(^|/)pyproject\.toml$|(^|/)poetry\.lock$|(^|/)Pipfile(\.lock)?$'
echo '--- outline main.py ---'
ast-grep outline main.py --view expanded || true
echo '--- search for send_message / Gemini / async endpoint ---'
rg -n "send_message|Gemini|async def|trim_history|ChatRequest|ChatResponse" main.pyRepository: Deen-Bridge/dnb-ai
Length of output: 1724
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant slice of main.py with line numbers.
sed -n '120,190p' main.py | cat -nRepository: Deen-Bridge/dnb-ai
Length of output: 3270
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any local dependency hints for the Gemini client.
rg -n "google\.generativeai|genai|GenerativeModel|asyncio\.to_thread|to_thread|send_message_async" .Repository: Deen-Bridge/dnb-ai
Length of output: 311
Offload the Gemini call from the async handler. chat.send_message(...) is synchronous, so this endpoint blocks the event loop while the model responds. Use the SDK’s async API or wrap it with asyncio.to_thread, and keep per-chat serialization intact.
🤖 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 145 - 147, Update the async handler around trim_history
and the synchronous chat.send_message call to use the SDK’s asynchronous API or
asyncio.to_thread, preventing model responses from blocking the event loop.
Preserve the existing per-chat serialization behavior while offloading only the
blocking Gemini request.
Source: Path instructions
|
|
||
| from unittest.mock import MagicMock, PropertyMock | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- tracked files of interest ---'
git ls-files | rg '(^|/)(requirements(\.txt)?|pyproject\.toml|setup\.py|setup.cfg|tox.ini|Pipfile|poetry\.lock|tests/test_history_truncation\.py)$'
printf '\n%s\n' '--- tests/test_history_truncation.py ---'
cat -n tests/test_history_truncation.py
printf '\n%s\n' '--- manifest search for pytest ---'
rg -n --hidden --glob '!**/.git/**' '\bpytest\b' requirements.txt requirements*.txt pyproject.toml setup.py setup.cfg tox.ini Pipfile poetry.lock 2>/dev/null || trueRepository: Deen-Bridge/dnb-ai
Length of output: 8153
Pin pytest in requirements.txt. This test module imports pytest directly, but the tracked manifest doesn’t declare it, so a clean CI environment can fail before the test suite starts.
🤖 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_history_truncation.py` at line 8, Add pytest as an explicitly
pinned dependency in requirements.txt so tests/test_history_truncation.py can
import it in clean CI environments.
Source: Path instructions
| def test_no_orphaned_turns_after_truncation(self, monkeypatch): | ||
| """History must always have pairs — never a lone user or model turn.""" | ||
| monkeypatch.setattr("history.MAX_HISTORY_TOKENS", 10) | ||
| monkeypatch.setattr("history.MAX_HISTORY_TURNS", 50) | ||
| pairs = [ | ||
| ("AAAAA AAAAA AA", "BBBBB BBBBB BB"), | ||
| ("CCCCC CCCCC CC", "DDDDD DDDDD DD"), | ||
| ("EEEEE EEEEE EE", "FFFFF FFFFF FF"), | ||
| ] | ||
| chat = _make_chat_session(pairs) | ||
| trim_history(chat) | ||
| assert len(chat.history) % 2 == 0 | ||
|
|
||
| def test_returns_true_when_truncation_happens(self, monkeypatch): | ||
| monkeypatch.setattr("history.MAX_HISTORY_TOKENS", 3) | ||
| monkeypatch.setattr("history.MAX_HISTORY_TURNS", 50) | ||
| chat = _make_chat_session([("AAAA AAAA", "BBBB BBBB")]) | ||
| assert trim_history(chat) is True | ||
|
|
||
| def test_preserves_recent_context_after_truncation(self, monkeypatch): | ||
| monkeypatch.setattr("history.MAX_HISTORY_TOKENS", 20) | ||
| monkeypatch.setattr("history.MAX_HISTORY_TURNS", 50) | ||
| pairs = [ | ||
| ("A" * 40 + " very old", "B" * 40 + " very old response"), | ||
| ("C" * 40 + " old", "D" * 40 + " old response"), | ||
| ("Recent question", "Recent answer"), | ||
| ] | ||
| chat = _make_chat_session(pairs) | ||
| trim_history(chat) | ||
| remaining = " ".join( | ||
| m.parts[0].text for m in chat.history | ||
| ) | ||
| assert "Recent" in remaining | ||
| assert "Old question 1" not in remaining |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert actual pair ordering and removed content.
len(history) % 2 == 0 allows invalid model,user sequences, and "Old question 1" is never added to the fixture, so Line 148 passes even if old turns remain. Assert each retained pair is user then model, and check for real old markers such as "very old" or "C" * 40.
🤖 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_history_truncation.py` around lines 115 - 148, Strengthen
test_no_orphaned_turns_after_truncation by asserting every retained history pair
is ordered user then model, not only that the length is even. In
test_preserves_recent_context_after_truncation, replace the nonexistent "Old
question 1" marker with content actually present in the fixture, such as "very
old" or the repeated C marker, and assert that old content was removed while
recent context remains.
| def test_budget_configurable_via_env(self, monkeypatch): | ||
| monkeypatch.setattr("history.MAX_HISTORY_TOKENS", 1000) | ||
| monkeypatch.setattr("history.MAX_HISTORY_TURNS", 3) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test environment loading rather than patched globals.
This test patches already-imported module constants, so it never validates os.getenv() parsing or defaults. Set the environment variables and reload history before exercising the reloaded trim_history.
🤖 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_history_truncation.py` around lines 169 - 171, Update
test_budget_configurable_via_env to set the relevant environment variables with
monkeypatch and reload the history module before invoking trim_history. Remove
the direct patches to history.MAX_HISTORY_TOKENS and history.MAX_HISTORY_TURNS,
and call trim_history from the reloaded module so the test validates environment
parsing.
|
@Abdul-dev-creator please fix conflict and point pr to dev branch |
Resolve conflicts and integrate token-budget history trimming into the refactored chat flow (safety pipeline, fiqh, semantic cache, confidence). Closes Deen-Bridge#13
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (6)
study.py (2)
523-529: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead code: redundant length check.
LessonTextSource.lesson_text(Line 99-104) already enforcesmax_length=MAX_LESSON_TEXT_LENGTHvia Pydantic, so FastAPI rejects an oversized payload with a 422 before this handler ever runs (confirmed bytest_oversized_lesson_text_is_rejected_by_pydantic, tests/test_study.py Lines 184-187). This block can never execute.🧹 Proposed cleanup
- if isinstance(request.source, LessonTextSource): - text_len = len(request.source.lesson_text) - if text_len > MAX_LESSON_TEXT_LENGTH: - raise HTTPException( - status_code=422, - detail=f"lesson_text must not exceed {MAX_LESSON_TEXT_LENGTH} characters (got {text_len})", - ) -🤖 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 `@study.py` around lines 523 - 529, Remove the redundant text-length validation block from the request handler, including the unused text_len calculation and HTTPException, since LessonTextSource already enforces MAX_LESSON_TEXT_LENGTH through Pydantic before the handler runs. Preserve the surrounding LessonTextSource handling unchanged.
401-404: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing exception chaining (
raise ... from).Static analysis (Ruff B904) flags this: re-raising as
ValueErrorwithoutfrom ediscards the original traceback context, making debugging harder.🔧 Proposed fix
try: data = json.loads(response_text) except json.JSONDecodeError as e: - raise ValueError(f"Invalid JSON: {e}") + raise ValueError(f"Invalid JSON: {e}") from e🤖 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 `@study.py` around lines 401 - 404, Update the JSON parsing exception handler around json.loads in the response-processing flow to chain the new ValueError explicitly from the caught JSONDecodeError using raise ... from e. Preserve the existing error message and exception type.Source: Linters/SAST tools
tests/test_fiqh.py (1)
155-167: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd negative fixture assertions for non-fiqh prompts.
The fixture tests currently verify positive fiqh matches, but non-fiqh rows only check that
expect_schoolsis empty. Assert thatclassify_fiqh(case["prompt"]) is Falsefor those rows, and assert the public classifier for positive rows as well.This strengthens the fixture-driven classification contract and guards against the broad matching issue above.
🤖 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_fiqh.py` around lines 155 - 167, Update the fixture assertions in test_non_fiqh_cases_have_empty_schools to also require classify_fiqh(case["prompt"]) is False for every non-fiqh row. In the positive fiqh fixture test, or an equivalent positive-row assertion, require classify_fiqh(case["prompt"]) is True while preserving the existing keyword_match check.requirements.txt (1)
7-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider pinning
redisexactly like its new neighbors.
PyYAML==6.0.2andnumpy==2.2.4are exact-pinned, butredis>=5.0.0is open-ended — a future majorredisrelease could get pulled in on a fresh install without a corresponding diff, which works against reproducible builds.♻️ Suggested fix
-redis>=5.0.0 +redis==5.2.1(swap in whatever version you've actually tested against)
🤖 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` around lines 7 - 9, Pin the redis dependency exactly in requirements.txt instead of leaving it open-ended at redis>=5.0.0. Use the specific redis version tested with the project, matching the exact-pinning style of PyYAML and numpy..github/workflows/ci.yml (1)
36-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a catch-all
pyteststep as a safety net.The per-area pytest steps are great for readable CI logs, but they only run the specific files listed — any future test file that isn't explicitly wired in here will simply never execute in CI, silently. A trailing "Run full test suite" step (e.g.
pytest -q tests/) would guard against that without needing to remove the granular steps.🤖 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 @.github/workflows/ci.yml around lines 36 - 55, Add a trailing full-suite pytest step after the existing per-area test steps, using the tests directory as the target (for example, “Run full test suite” with pytest -q tests/). Keep all granular steps unchanged so their readable CI output is preserved.hadith.py (1)
322-342: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
@lru_cacheon an instance method pinsselfforever.
_indexis decorated with@lru_cache(maxsize=None)directly on the method (line 328), which caches(self, collection)tuples — everyBundledGradingSourceinstance ever constructed stays alive for the process lifetime since it's a cache key. Today there's a single module-level_default_source, so it's harmless in practice, but it's the exact anti-pattern Ruff's B019 flags, and it'll bite silently if this class is ever instantiated per-request or per-test with a customdata_dir.♻️ Suggested fix — cache per-instance without leaking `self` via `lru_cache`
class BundledGradingSource(GradingSource): def __init__(self, data_dir: Path = DATA_DIR): self._data_dir = data_dir + self._index_cache: Dict[str, Optional[Dict]] = {} - `@lru_cache`(maxsize=None) def _index(self, collection: str) -> Optional[Dict[str, Dict]]: + if collection in self._index_cache: + return self._index_cache[collection] path = self._data_dir / f"{collection}.json" if not path.exists(): + self._index_cache[collection] = None return None with open(path, encoding="utf-8") as f: payload = json.load(f) by_number: Dict[int, Dict] = {} by_book_number: Dict[Tuple[int, int], Dict] = {} for record in payload.get("hadiths", []): by_number[record["n"]] = record if record.get("book") is not None and record.get("bn") is not None: by_book_number[(record["book"], record["bn"])] = record - return {"by_number": by_number, "by_book_number": by_book_number} + result = {"by_number": by_number, "by_book_number": by_book_number} + self._index_cache[collection] = result + return result🤖 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 `@hadith.py` around lines 322 - 342, Remove the direct `@lru_cache` decoration from BundledGradingSource._index so cached keys do not retain self indefinitely. Replace it with per-instance caching initialized by BundledGradingSource.__init__, while preserving collection-based lookup results and the existing missing-file behavior.Source: Linters/SAST tools
🤖 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 @.github/workflows/ci.yml:
- Line 31: Update the CI lint and compile target lists in the workflow to
include semantic_cache.py and the test modules tests/test_semantic_cache.py,
tests/test_fiqh.py, tests/test_study.py, and tests/test_hadith.py. Ensure both
flake8 and compileall cover these files while preserving the existing checks and
options.
In `@fiqh.py`:
- Around line 95-96: Resolve the flake8 violations at fiqh.py lines 95-96 by
splitting the long MADHHAB_LEAD_INSTRUCTION string without changing its content;
at tests/test_fiqh.py line 12, remove the unused VALID_MADHHABS import or use
it; and at lines 165-167, wrap the long parametrization decorator and assertion
while preserving test behavior.
- Around line 42-67: Update keyword_match() to use token- and phrase-boundary
matching instead of raw substring checks, preventing generic entries such as
“can i” and “interest” from matching unrelated text. Replace overly broad
FIQH_KEYWORDS entries with domain-scoped patterns while preserving legitimate
fiqh classification, and add negative regression cases for prompts such as
password resets and “This is interesting.”
In `@hadith.py`:
- Around line 322-363: Update BundledGradingSource.get() so that when book is
provided, it only returns the by_book_number match and returns None if that pair
is absent; reserve the by_number fallback for calls without a book argument,
preventing unmatched book citations from receiving an unrelated grade.
- Around line 460-491: Update build_caution_note so the unverified-reference
trigger applies when all cited references are unverified, including answers with
multiple citations, rather than only when references has length one. Preserve
the existing flagged-reference handling and caution wording, and add coverage in
tests/test_hadith.py for two unverified citations.
In `@main.py`:
- Around line 215-218: Update the async cache-read path around normalize_text,
embed_text, and semantic_cache.get to avoid calling synchronous embed_text on
the event loop; use the available asynchronous embedding API with await, or
offload the synchronous call via asyncio.to_thread. Apply the same non-blocking
treatment to the cache-write embedding path around the referenced embedding
call, preserving existing cache behavior.
In `@README.md`:
- Around line 72-75: Update the README setup instructions so the copied
.env.example contents are preserved: remove the redundant GEMINI_API_KEY echo
step, or change it to append with >> and explicitly tell users to edit the
placeholder key. Keep the cp .env.example .env command as the source of the
complete environment configuration.
In `@safety/pipeline.py`:
- Around line 32-38: Update SafetyPipeline.run_async to offload the synchronous
_complete call, including generator execution, to the project’s existing
worker-thread or executor mechanism so Gemini network calls do not block the
event loop. Preserve the current arguments and returned SafetyResult, and add a
test verifying the generator executes outside the event-loop thread.
In `@safety/policy.yaml`:
- Around line 75-91: Update the DB-SAFE-004 policy entry’s output_patterns to
mirror its existing harmful religious-content patterns, enabling OutputCheck to
replace generated religious justifications for violence. Add a replacement test
covering this category and verify the generated harmful output is replaced.
In `@semantic_cache.py`:
- Around line 139-157: Add a shared lock to SemanticCache and use it to
synchronize all reads and mutations of the paired _entries and _access_times
lists across get(), put(), and _find_best_match(). Ensure each operation holds
the lock for its complete iteration or update, including eviction, so concurrent
calls from the event loop and asyncio.to_thread cannot leave the lists
inconsistent.
In `@study.py`:
- Around line 412-436: Update the quiz and flashcard validation branches to
reject empty lists as well as non-list values, preserving the existing “must be
a non-empty array” errors and avoiding insertion of empty results. Apply the
same emptiness check in both branches; leave the broader exception handling
unchanged unless needed for this validation fix.
- Around line 258-277: Update the async generate_study_content flow to execute
the synchronous _generate_content → GeminiGenerator.generate chain via
run_in_threadpool, ensuring model.generate_content does not block the event loop
while preserving the existing response and error behavior.
- Around line 307-313: Update _build_prompt so lesson_text is not hard-truncated
to 500 characters; preserve the full LessonTextSource.lesson_text up to its
20,000-character limit, or replace the fixed cap with the existing
MAX_LESSON_TEXT_LENGTH configuration and document the token-budget behavior if
truncation is required.
In `@tests/redteam/test_live.py`:
- Around line 16-19: Update the live audit setup in test_live.py to use the
google-genai client instead of the deprecated google.generativeai import,
configure it with the existing GEMINI_API_KEY, and select a currently supported
Gemini model. Update requirements.txt to replace the google-generativeai==0.8.3
dependency with the corresponding google-genai package, preserving the test’s
existing assertions and opt-in behavior.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 36-55: Add a trailing full-suite pytest step after the existing
per-area test steps, using the tests directory as the target (for example, “Run
full test suite” with pytest -q tests/). Keep all granular steps unchanged so
their readable CI output is preserved.
In `@hadith.py`:
- Around line 322-342: Remove the direct `@lru_cache` decoration from
BundledGradingSource._index so cached keys do not retain self indefinitely.
Replace it with per-instance caching initialized by
BundledGradingSource.__init__, while preserving collection-based lookup results
and the existing missing-file behavior.
In `@requirements.txt`:
- Around line 7-9: Pin the redis dependency exactly in requirements.txt instead
of leaving it open-ended at redis>=5.0.0. Use the specific redis version tested
with the project, matching the exact-pinning style of PyYAML and numpy.
In `@study.py`:
- Around line 523-529: Remove the redundant text-length validation block from
the request handler, including the unused text_len calculation and
HTTPException, since LessonTextSource already enforces MAX_LESSON_TEXT_LENGTH
through Pydantic before the handler runs. Preserve the surrounding
LessonTextSource handling unchanged.
- Around line 401-404: Update the JSON parsing exception handler around
json.loads in the response-processing flow to chain the new ValueError
explicitly from the caught JSONDecodeError using raise ... from e. Preserve the
existing error message and exception type.
In `@tests/test_fiqh.py`:
- Around line 155-167: Update the fixture assertions in
test_non_fiqh_cases_have_empty_schools to also require
classify_fiqh(case["prompt"]) is False for every non-fiqh row. In the positive
fiqh fixture test, or an equivalent positive-row assertion, require
classify_fiqh(case["prompt"]) is True while preserving the existing
keyword_match check.
🪄 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: 9380f9a8-e54f-4abd-8f1a-5d84dd634f2c
📒 Files selected for processing (47)
.env.example.github/workflows/ci.yml.gitignoreCONTRIBUTING.mdREADME.mdconfidence.pydata/hadith/PROVENANCE.mddata/hadith/abudawud.jsondata/hadith/bukhari.jsondata/hadith/ibnmajah.jsondata/hadith/malik.jsondata/hadith/muslim.jsondata/hadith/nasai.jsondata/hadith/tirmidhi.jsonfiqh.pyhadith.pymain.pyrequirements.txtreview.pyreview_store.pysafety/POLICY.mdsafety/__init__.pysafety/input_gate.pysafety/output_check.pysafety/pipeline.pysafety/policy.pysafety/policy.yamlscripts/build_hadith_data.pysemantic_cache.pystudy.pytests/__init__.pytests/conftest.pytests/fixtures/fiqh_cases.jsonltests/fixtures/flashcards_valid.jsontests/fixtures/invalid_json.jsontests/fixtures/quiz_valid.jsontests/fixtures/schema_invalid.jsontests/fixtures/semantic_invalid.jsontests/redteam/prompts.yamltests/redteam/test_live.pytests/redteam/test_pipeline.pytests/test_confidence.pytests/test_fiqh.pytests/test_hadith.pytests/test_review_queue.pytests/test_semantic_cache.pytests/test_study.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 14
🧹 Nitpick comments (6)
study.py (2)
523-529: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead code: redundant length check.
LessonTextSource.lesson_text(Line 99-104) already enforcesmax_length=MAX_LESSON_TEXT_LENGTHvia Pydantic, so FastAPI rejects an oversized payload with a 422 before this handler ever runs (confirmed bytest_oversized_lesson_text_is_rejected_by_pydantic, tests/test_study.py Lines 184-187). This block can never execute.🧹 Proposed cleanup
- if isinstance(request.source, LessonTextSource): - text_len = len(request.source.lesson_text) - if text_len > MAX_LESSON_TEXT_LENGTH: - raise HTTPException( - status_code=422, - detail=f"lesson_text must not exceed {MAX_LESSON_TEXT_LENGTH} characters (got {text_len})", - ) -🤖 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 `@study.py` around lines 523 - 529, Remove the redundant text-length validation block from the request handler, including the unused text_len calculation and HTTPException, since LessonTextSource already enforces MAX_LESSON_TEXT_LENGTH through Pydantic before the handler runs. Preserve the surrounding LessonTextSource handling unchanged.
401-404: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing exception chaining (
raise ... from).Static analysis (Ruff B904) flags this: re-raising as
ValueErrorwithoutfrom ediscards the original traceback context, making debugging harder.🔧 Proposed fix
try: data = json.loads(response_text) except json.JSONDecodeError as e: - raise ValueError(f"Invalid JSON: {e}") + raise ValueError(f"Invalid JSON: {e}") from e🤖 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 `@study.py` around lines 401 - 404, Update the JSON parsing exception handler around json.loads in the response-processing flow to chain the new ValueError explicitly from the caught JSONDecodeError using raise ... from e. Preserve the existing error message and exception type.Source: Linters/SAST tools
tests/test_fiqh.py (1)
155-167: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd negative fixture assertions for non-fiqh prompts.
The fixture tests currently verify positive fiqh matches, but non-fiqh rows only check that
expect_schoolsis empty. Assert thatclassify_fiqh(case["prompt"]) is Falsefor those rows, and assert the public classifier for positive rows as well.This strengthens the fixture-driven classification contract and guards against the broad matching issue above.
🤖 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_fiqh.py` around lines 155 - 167, Update the fixture assertions in test_non_fiqh_cases_have_empty_schools to also require classify_fiqh(case["prompt"]) is False for every non-fiqh row. In the positive fiqh fixture test, or an equivalent positive-row assertion, require classify_fiqh(case["prompt"]) is True while preserving the existing keyword_match check.requirements.txt (1)
7-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider pinning
redisexactly like its new neighbors.
PyYAML==6.0.2andnumpy==2.2.4are exact-pinned, butredis>=5.0.0is open-ended — a future majorredisrelease could get pulled in on a fresh install without a corresponding diff, which works against reproducible builds.♻️ Suggested fix
-redis>=5.0.0 +redis==5.2.1(swap in whatever version you've actually tested against)
🤖 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` around lines 7 - 9, Pin the redis dependency exactly in requirements.txt instead of leaving it open-ended at redis>=5.0.0. Use the specific redis version tested with the project, matching the exact-pinning style of PyYAML and numpy..github/workflows/ci.yml (1)
36-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a catch-all
pyteststep as a safety net.The per-area pytest steps are great for readable CI logs, but they only run the specific files listed — any future test file that isn't explicitly wired in here will simply never execute in CI, silently. A trailing "Run full test suite" step (e.g.
pytest -q tests/) would guard against that without needing to remove the granular steps.🤖 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 @.github/workflows/ci.yml around lines 36 - 55, Add a trailing full-suite pytest step after the existing per-area test steps, using the tests directory as the target (for example, “Run full test suite” with pytest -q tests/). Keep all granular steps unchanged so their readable CI output is preserved.hadith.py (1)
322-342: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
@lru_cacheon an instance method pinsselfforever.
_indexis decorated with@lru_cache(maxsize=None)directly on the method (line 328), which caches(self, collection)tuples — everyBundledGradingSourceinstance ever constructed stays alive for the process lifetime since it's a cache key. Today there's a single module-level_default_source, so it's harmless in practice, but it's the exact anti-pattern Ruff's B019 flags, and it'll bite silently if this class is ever instantiated per-request or per-test with a customdata_dir.♻️ Suggested fix — cache per-instance without leaking `self` via `lru_cache`
class BundledGradingSource(GradingSource): def __init__(self, data_dir: Path = DATA_DIR): self._data_dir = data_dir + self._index_cache: Dict[str, Optional[Dict]] = {} - `@lru_cache`(maxsize=None) def _index(self, collection: str) -> Optional[Dict[str, Dict]]: + if collection in self._index_cache: + return self._index_cache[collection] path = self._data_dir / f"{collection}.json" if not path.exists(): + self._index_cache[collection] = None return None with open(path, encoding="utf-8") as f: payload = json.load(f) by_number: Dict[int, Dict] = {} by_book_number: Dict[Tuple[int, int], Dict] = {} for record in payload.get("hadiths", []): by_number[record["n"]] = record if record.get("book") is not None and record.get("bn") is not None: by_book_number[(record["book"], record["bn"])] = record - return {"by_number": by_number, "by_book_number": by_book_number} + result = {"by_number": by_number, "by_book_number": by_book_number} + self._index_cache[collection] = result + return result🤖 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 `@hadith.py` around lines 322 - 342, Remove the direct `@lru_cache` decoration from BundledGradingSource._index so cached keys do not retain self indefinitely. Replace it with per-instance caching initialized by BundledGradingSource.__init__, while preserving collection-based lookup results and the existing missing-file behavior.Source: Linters/SAST tools
🤖 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 @.github/workflows/ci.yml:
- Line 31: Update the CI lint and compile target lists in the workflow to
include semantic_cache.py and the test modules tests/test_semantic_cache.py,
tests/test_fiqh.py, tests/test_study.py, and tests/test_hadith.py. Ensure both
flake8 and compileall cover these files while preserving the existing checks and
options.
In `@fiqh.py`:
- Around line 95-96: Resolve the flake8 violations at fiqh.py lines 95-96 by
splitting the long MADHHAB_LEAD_INSTRUCTION string without changing its content;
at tests/test_fiqh.py line 12, remove the unused VALID_MADHHABS import or use
it; and at lines 165-167, wrap the long parametrization decorator and assertion
while preserving test behavior.
- Around line 42-67: Update keyword_match() to use token- and phrase-boundary
matching instead of raw substring checks, preventing generic entries such as
“can i” and “interest” from matching unrelated text. Replace overly broad
FIQH_KEYWORDS entries with domain-scoped patterns while preserving legitimate
fiqh classification, and add negative regression cases for prompts such as
password resets and “This is interesting.”
In `@hadith.py`:
- Around line 322-363: Update BundledGradingSource.get() so that when book is
provided, it only returns the by_book_number match and returns None if that pair
is absent; reserve the by_number fallback for calls without a book argument,
preventing unmatched book citations from receiving an unrelated grade.
- Around line 460-491: Update build_caution_note so the unverified-reference
trigger applies when all cited references are unverified, including answers with
multiple citations, rather than only when references has length one. Preserve
the existing flagged-reference handling and caution wording, and add coverage in
tests/test_hadith.py for two unverified citations.
In `@main.py`:
- Around line 215-218: Update the async cache-read path around normalize_text,
embed_text, and semantic_cache.get to avoid calling synchronous embed_text on
the event loop; use the available asynchronous embedding API with await, or
offload the synchronous call via asyncio.to_thread. Apply the same non-blocking
treatment to the cache-write embedding path around the referenced embedding
call, preserving existing cache behavior.
In `@README.md`:
- Around line 72-75: Update the README setup instructions so the copied
.env.example contents are preserved: remove the redundant GEMINI_API_KEY echo
step, or change it to append with >> and explicitly tell users to edit the
placeholder key. Keep the cp .env.example .env command as the source of the
complete environment configuration.
In `@safety/pipeline.py`:
- Around line 32-38: Update SafetyPipeline.run_async to offload the synchronous
_complete call, including generator execution, to the project’s existing
worker-thread or executor mechanism so Gemini network calls do not block the
event loop. Preserve the current arguments and returned SafetyResult, and add a
test verifying the generator executes outside the event-loop thread.
In `@safety/policy.yaml`:
- Around line 75-91: Update the DB-SAFE-004 policy entry’s output_patterns to
mirror its existing harmful religious-content patterns, enabling OutputCheck to
replace generated religious justifications for violence. Add a replacement test
covering this category and verify the generated harmful output is replaced.
In `@semantic_cache.py`:
- Around line 139-157: Add a shared lock to SemanticCache and use it to
synchronize all reads and mutations of the paired _entries and _access_times
lists across get(), put(), and _find_best_match(). Ensure each operation holds
the lock for its complete iteration or update, including eviction, so concurrent
calls from the event loop and asyncio.to_thread cannot leave the lists
inconsistent.
In `@study.py`:
- Around line 412-436: Update the quiz and flashcard validation branches to
reject empty lists as well as non-list values, preserving the existing “must be
a non-empty array” errors and avoiding insertion of empty results. Apply the
same emptiness check in both branches; leave the broader exception handling
unchanged unless needed for this validation fix.
- Around line 258-277: Update the async generate_study_content flow to execute
the synchronous _generate_content → GeminiGenerator.generate chain via
run_in_threadpool, ensuring model.generate_content does not block the event loop
while preserving the existing response and error behavior.
- Around line 307-313: Update _build_prompt so lesson_text is not hard-truncated
to 500 characters; preserve the full LessonTextSource.lesson_text up to its
20,000-character limit, or replace the fixed cap with the existing
MAX_LESSON_TEXT_LENGTH configuration and document the token-budget behavior if
truncation is required.
In `@tests/redteam/test_live.py`:
- Around line 16-19: Update the live audit setup in test_live.py to use the
google-genai client instead of the deprecated google.generativeai import,
configure it with the existing GEMINI_API_KEY, and select a currently supported
Gemini model. Update requirements.txt to replace the google-generativeai==0.8.3
dependency with the corresponding google-genai package, preserving the test’s
existing assertions and opt-in behavior.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 36-55: Add a trailing full-suite pytest step after the existing
per-area test steps, using the tests directory as the target (for example, “Run
full test suite” with pytest -q tests/). Keep all granular steps unchanged so
their readable CI output is preserved.
In `@hadith.py`:
- Around line 322-342: Remove the direct `@lru_cache` decoration from
BundledGradingSource._index so cached keys do not retain self indefinitely.
Replace it with per-instance caching initialized by
BundledGradingSource.__init__, while preserving collection-based lookup results
and the existing missing-file behavior.
In `@requirements.txt`:
- Around line 7-9: Pin the redis dependency exactly in requirements.txt instead
of leaving it open-ended at redis>=5.0.0. Use the specific redis version tested
with the project, matching the exact-pinning style of PyYAML and numpy.
In `@study.py`:
- Around line 523-529: Remove the redundant text-length validation block from
the request handler, including the unused text_len calculation and
HTTPException, since LessonTextSource already enforces MAX_LESSON_TEXT_LENGTH
through Pydantic before the handler runs. Preserve the surrounding
LessonTextSource handling unchanged.
- Around line 401-404: Update the JSON parsing exception handler around
json.loads in the response-processing flow to chain the new ValueError
explicitly from the caught JSONDecodeError using raise ... from e. Preserve the
existing error message and exception type.
In `@tests/test_fiqh.py`:
- Around line 155-167: Update the fixture assertions in
test_non_fiqh_cases_have_empty_schools to also require
classify_fiqh(case["prompt"]) is False for every non-fiqh row. In the positive
fiqh fixture test, or an equivalent positive-row assertion, require
classify_fiqh(case["prompt"]) is True while preserving the existing
keyword_match check.
🪄 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: 9380f9a8-e54f-4abd-8f1a-5d84dd634f2c
📒 Files selected for processing (47)
.env.example.github/workflows/ci.yml.gitignoreCONTRIBUTING.mdREADME.mdconfidence.pydata/hadith/PROVENANCE.mddata/hadith/abudawud.jsondata/hadith/bukhari.jsondata/hadith/ibnmajah.jsondata/hadith/malik.jsondata/hadith/muslim.jsondata/hadith/nasai.jsondata/hadith/tirmidhi.jsonfiqh.pyhadith.pymain.pyrequirements.txtreview.pyreview_store.pysafety/POLICY.mdsafety/__init__.pysafety/input_gate.pysafety/output_check.pysafety/pipeline.pysafety/policy.pysafety/policy.yamlscripts/build_hadith_data.pysemantic_cache.pystudy.pytests/__init__.pytests/conftest.pytests/fixtures/fiqh_cases.jsonltests/fixtures/flashcards_valid.jsontests/fixtures/invalid_json.jsontests/fixtures/quiz_valid.jsontests/fixtures/schema_invalid.jsontests/fixtures/semantic_invalid.jsontests/redteam/prompts.yamltests/redteam/test_live.pytests/redteam/test_pipeline.pytests/test_confidence.pytests/test_fiqh.pytests/test_hadith.pytests/test_review_queue.pytests/test_semantic_cache.pytests/test_study.py
🛑 Comments failed to post (14)
.github/workflows/ci.yml (1)
31-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Lint/compile checks don't cover all the new files this PR adds tests for.
tests/test_semantic_cache.py,tests/test_fiqh.py,tests/test_study.py, andtests/test_hadith.pyall get their ownpyteststep below (lines 39-49), but none of them — nor thesemantic_cache.pymodule itself — appear in theflake8orcompilealltarget lists. That means style violations or outright syntax errors in these new files won't fail the build the way they should.🔧 Suggested fix
- run: flake8 main.py stellar.py safety tests/redteam study.py fiqh.py hadith.py confidence.py review.py review_store.py tests/test_confidence.py tests/test_review_queue.py scripts/build_hadith_data.py --max-line-length=120 --ignore=E501,W503 + run: flake8 main.py stellar.py safety tests/redteam study.py fiqh.py hadith.py confidence.py review.py review_store.py semantic_cache.py tests/test_confidence.py tests/test_review_queue.py tests/test_hadith.py tests/test_fiqh.py tests/test_study.py tests/test_semantic_cache.py scripts/build_hadith_data.py --max-line-length=120 --ignore=E501,W503 - name: Check syntax - run: python -m compileall -q main.py stellar.py safety tests/redteam study.py fiqh.py hadith.py confidence.py review.py review_store.py scripts/build_hadith_data.py + run: python -m compileall -q main.py stellar.py safety tests/redteam study.py fiqh.py hadith.py confidence.py review.py review_store.py semantic_cache.py scripts/build_hadith_data.pyAs per path instructions, "CI enforces flake8, so style violations fail the build" — worth keeping that guarantee true for every new file landing in this PR.
Also applies to: 33-34
🤖 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 @.github/workflows/ci.yml at line 31, Update the CI lint and compile target lists in the workflow to include semantic_cache.py and the test modules tests/test_semantic_cache.py, tests/test_fiqh.py, tests/test_study.py, and tests/test_hadith.py. Ensure both flake8 and compileall cover these files while preserving the existing checks and options.Source: Path instructions
fiqh.py (2)
42-67: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid raw substring matching for fiqh classification.
keyword_match()classifies unrelated prompts such as “Can I reset my password?” ("can i") and “This is interesting” ("interest") as fiqh. Use token/phrase boundaries and replace generic phrases with domain-scoped patterns, then add negative regression cases.Cross-file impact:
main.pyuses this result to add fiqh context and mark the request high-stakes, so these false positives alter live chat behavior.🤖 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 `@fiqh.py` around lines 42 - 67, Update keyword_match() to use token- and phrase-boundary matching instead of raw substring checks, preventing generic entries such as “can i” and “interest” from matching unrelated text. Replace overly broad FIQH_KEYWORDS entries with domain-scoped patterns while preserving legitimate fiqh classification, and add negative regression cases for prompts such as password resets and “This is interesting.”
95-96: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Resolve the flake8 violations before merging.
These changed Python files currently contain lint violations that can fail CI:
fiqh.py#L95-L96: split the longMADHHAB_LEAD_INSTRUCTIONstring.tests/test_fiqh.py#L12-L12: remove or use the unusedVALID_MADHHABSimport.tests/test_fiqh.py#L165-L167: wrap the long parametrization decorator and assertion.📍 Affects 2 files
fiqh.py#L95-L96(this comment)tests/test_fiqh.py#L12-L12tests/test_fiqh.py#L165-L167🤖 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 `@fiqh.py` around lines 95 - 96, Resolve the flake8 violations at fiqh.py lines 95-96 by splitting the long MADHHAB_LEAD_INSTRUCTION string without changing its content; at tests/test_fiqh.py line 12, remove the unused VALID_MADHHABS import or use it; and at lines 165-167, wrap the long parametrization decorator and assertion while preserving test behavior.Source: Path instructions
hadith.py (2)
322-363: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
get()can silently return the wrong hadith's grade when a book number is given but doesn't match.Nice, clean lookup structure overall! One correctness gap though: when
bookis provided but(book, number)isn't found inby_book_number,get()falls through toby_number.get(number)(line 352-353) and treatsnumberas a global sequential number. If a citation names a book that doesn't contain that number (typo, wrong book index, etc.), this can return a completely unrelated hadith'sGradeRecord— and the caller (annotate()) will present that grade as if it belonged to the cited hadith. Given the whole point of this module is to avoid misattributing authenticity grades, this fallback works against that goal.🐛 Proposed fix — don't fall back to global-number when a book was specified but not matched
def get(self, collection: str, number: int, book: Optional[int] = None) -> Optional[GradeRecord]: index = self._index(collection) if index is None: return None - record = None if book is not None: record = index["by_book_number"].get((book, number)) - if record is None: + else: record = index["by_number"].get(number) if record is None: return NoneWorth adding a regression test in
tests/test_hadith.pyfor "book specified but wrong/unmatched →None, not a wrong record" — happy to draft one if useful.🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 332-332: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(path, encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').(open-filename-from-request)
🪛 Ruff (0.15.21)
[warning] 328-328: Use of
functools.lru_cacheorfunctools.cacheon methods can lead to memory leaks(B019)
🤖 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 `@hadith.py` around lines 322 - 363, Update BundledGradingSource.get() so that when book is provided, it only returns the by_book_number match and returns None if that pair is absent; reserve the by_number fallback for calls without a book argument, preventing unmatched book citations from receiving an unrelated grade.
460-491: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Caution note is skipped when several citations are all unverified.
sole_unverifiedonly fires when there's exactly one reference and it's unverified (line 470-472). If an answer cites two or more hadith and none of them resolve against the bundled dataset,flaggedis empty (unverified refs aren't individually flagged) andsole_unverifiedisFalse(sincelen(references) != 1) — sobuild_caution_notereturnsNoneeven though the entire evidentiary basis is unverified. That seems to run against the module's stated "never overstate authenticity" policy.🛡️ Suggested fix — trigger on "all unverified", not just "sole unverified"
flagged = [r for r in references if r.flagged] - sole_unverified = ( - len(references) == 1 and not references[0].verified - ) + all_unverified = bool(references) and all(not r.verified for r in references) - if not flagged and not sole_unverified: + if not flagged and not all_unverified: return None lines = ["Note on hadith authenticity in this answer:"] for r in flagged: lines.append(f"- \"{r.raw}\" — {r.note}.") - if sole_unverified: - lines.append( - f"- \"{references[0].raw}\" is the only hadith cited here and its grading " - "could not be verified against the bundled dataset — treat it as unverified, " - "not as an authentic narration." - ) + if all_unverified: + if len(references) == 1: + lines.append( + f"- \"{references[0].raw}\" is the only hadith cited here and its grading " + "could not be verified against the bundled dataset — treat it as unverified, " + "not as an authentic narration." + ) + else: + lines.append( + "- None of the hadith cited here could be verified against the bundled " + "dataset — treat all of them as unverified, not as authentic narrations." + )Would also be worth a test in
tests/test_hadith.pycovering "two citations, both unverified" to lock this in.📝 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 build_caution_note(text: str, references: List[HadithReference]) -> Optional[str]: """Compose a single caution suffix for the response, or None if not needed. Two triggers, per the grading policy: - a da'if/mawdu' or non-marfu hadith cited without an existing caveat nearby (i.e. presented as unqualified evidence), and - an unverified/ungraded reference that is the *sole* hadith support in the whole answer. """ flagged = [r for r in references if r.flagged] all_unverified = bool(references) and all(not r.verified for r in references) if not flagged and not all_unverified: return None lines = ["Note on hadith authenticity in this answer:"] for r in flagged: lines.append(f"- \"{r.raw}\" — {r.note}.") if all_unverified: if len(references) == 1: lines.append( f"- \"{references[0].raw}\" is the only hadith cited here and its grading " "could not be verified against the bundled dataset — treat it as unverified, " "not as an authentic narration." ) else: lines.append( "- None of the hadith cited here could be verified against the bundled " "dataset — treat all of them as unverified, not as authentic narrations." ) lines.append( "Please verify with a qualified scholar or a hadith reference (e.g. Sunnah.com) " "before relying on this as evidence." ) return "\n".join(lines)🤖 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 `@hadith.py` around lines 460 - 491, Update build_caution_note so the unverified-reference trigger applies when all cited references are unverified, including answers with multiple citations, rather than only when references has length one. Preserve the existing flagged-reference handling and caution wording, and add coverage in tests/test_hadith.py for two unverified citations.main.py (1)
215-218: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
does google-generativeai 0.8.3 provide an async embed_content coroutine💡 Result:
Yes, the google-generativeai library (version 0.8.3) does provide an asynchronous method for generating embeddings, named embed_content_async [1][2]. In this library, you can call embed_content_async as a coroutine to perform asynchronous embedding requests [2]. However, users should be aware of the following context regarding this library: 1. Deprecation Status: The google-generativeai package is now deprecated [3]. It has been succeeded by the new, unified Google Gen AI SDK (google-genai) [4][3]. 2. Known Issues: Asynchronous methods in the legacy google-generativeai library have been reported to experience instability and errors in certain environments (such as Google Colab or when using REST transport) [5][6]. 3. Recommended Migration: Google strongly recommends migrating to the new Google Gen AI SDK (google-genai), which features a modern, more robust asynchronous API accessed via client.aio (e.g., client.aio.models.embed_content) [7][8][4][9][3].
Citations:
- 1: https://github.com/google-gemini/generative-ai-python/blob/main/docs/api/google/generativeai.md
- 2: https://github.com/google-gemini/deprecated-generative-ai-python/blob/master/google/generativeai/embedding.py
- 3: https://pypi.org/project/google-generativeai/
- 4: https://github.com/googleapis/python-genai
- 5: google-gemini/deprecated-generative-ai-python#499
- 6: google-gemini/deprecated-generative-ai-python#732
- 7: https://googleapis.github.io/python-genai/genai.html
- 8: https://googleapis-python-genai-70.mintlify.app/api/models/embed-content
- 9: https://googleapis.github.io/python-genai/
Avoid the synchronous embedding call in the async handler.
embed_text()is a network round-trip, so calling it directly inmain.py:215-218blocks the event loop; the same applies to the cache-write path atmain.py:372-375. If this helper still usesgoogle-generativeai, switch toembed_content_async/awaitwhere available, or offload the sync call withasyncio.to_threadso other requests can keep flowing.🤖 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 215 - 218, Update the async cache-read path around normalize_text, embed_text, and semantic_cache.get to avoid calling synchronous embed_text on the event loop; use the available asynchronous embedding API with await, or offload the synchronous call via asyncio.to_thread. Apply the same non-blocking treatment to the cache-write embedding path around the referenced embedding call, preserving existing cache behavior.Source: Path instructions
README.md (1)
72-75: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The new
cpstep gets clobbered two lines later. Nice addition documenting the template copy — but Line 75'secho "GEMINI_API_KEY=..." > .envuses a truncating redirect (>), which overwrites the.envyou just copied on Line 73 and throws away every optional variable in the template. A reader following the steps literally ends up with a one-line.env. Either drop theecholine or switch it to append (>>) and remind the user to edit the key.📝 Suggested fix
# Copy environment template and add your API key cp .env.example .env - -echo "GEMINI_API_KEY=your_api_key_here" > .env +# then edit .env and set GEMINI_API_KEY📝 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.# Copy environment template and add your API key cp .env.example .env # then edit .env and set GEMINI_API_KEY🤖 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 `@README.md` around lines 72 - 75, Update the README setup instructions so the copied .env.example contents are preserved: remove the redundant GEMINI_API_KEY echo step, or change it to append with >> and explicitly tell users to edit the placeholder key. Keep the cp .env.example .env command as the source of the complete environment configuration.safety/pipeline.py (1)
32-38: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Offload generation from the event loop.
run_asyncoffloads classification but then calls_complete()directly;_complete()synchronously invokesgenerator. A Gemini network call therefore blocks concurrent async requests. Offload_complete(and add a test that the generator runs outside the loop).Proposed fix
+import asyncio from dataclasses import dataclass @@ started = perf_counter() decision = await self.input_gate.evaluate_async(prompt) - return self._complete(prompt, generator, decision, started) + return await asyncio.to_thread( + self._complete, prompt, generator, decision, started + )As per path instructions, “network calls should be awaited or offloaded”.
📝 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.async def run_async( self, prompt: str, generator: Callable[[str], str] ) -> SafetyResult: """Run the pipeline while keeping classification off the event loop.""" started = perf_counter() decision = await self.input_gate.evaluate_async(prompt) return await asyncio.to_thread( self._complete, prompt, generator, decision, started )🤖 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 32 - 38, Update SafetyPipeline.run_async to offload the synchronous _complete call, including generator execution, to the project’s existing worker-thread or executor mechanism so Gemini network calls do not block the event loop. Preserve the current arguments and returned SafetyResult, and add a test verifying the generator executes outside the event-loop thread.Source: Path instructions
safety/policy.yaml (1)
75-91: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add output enforcement for harmful religious content.
DB-SAFE-004has nooutput_patterns, soOutputCheckcannot replace generated religious justifications for violence. Mirror the harm patterns here and add a generated-output replacement test for this category.🤖 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/policy.yaml` around lines 75 - 91, Update the DB-SAFE-004 policy entry’s output_patterns to mirror its existing harmful religious-content patterns, enabling OutputCheck to replace generated religious justifications for violence. Add a replacement test covering this category and verify the generated harmful output is replaced.semantic_cache.py (1)
139-157: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm cache is entered from both an async handler and a worker thread. rg -nP '(get_cache\(\)|semantic_cache)\.(get|put)\s*\(' --type=py rg -nP 'asyncio\.to_thread\([^)]*cache' --type=pyRepository: Deen-Bridge/dnb-ai
Length of output: 156
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== relevant files ==" git ls-files | rg '(^|/)(semantic_cache|main|review)\.py$|semantic_cache\.py$' echo echo "== semantic_cache.py outline ==" ast-grep outline semantic_cache.py --view expanded || true echo echo "== call sites for cache methods ==" rg -n --hidden --glob '*.py' '\b(get|put)\(' semantic_cache.py main.py review.py || true rg -n --hidden --glob '*.py' 'get_cache\(\)|SemanticCache|semantic_cache' . || true echo echo "== async/thread usage around cache ==" rg -n --hidden --glob '*.py' 'asyncio\.to_thread|ThreadPoolExecutor|run_in_executor|to_thread\(' . || trueRepository: Deen-Bridge/dnb-ai
Length of output: 5292
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== review.py around cache write ==" sed -n '120,175p' review.py | cat -n echo echo "== review.py around to_thread usage ==" sed -n '260,290p' review.py | cat -n echo echo "== main.py around cache read/write ==" sed -n '200,390p' main.py | cat -n echo echo "== semantic_cache.py methods ==" sed -n '115,230p' semantic_cache.py | cat -nRepository: Deen-Bridge/dnb-ai
Length of output: 17707
Guard
SemanticCachewith a lock.main.pyuses this singleton on the event loop, andreview.pycallsget_cache().put(...)fromasyncio.to_thread(...). The unsynchronized_entries/_access_timesmutations inget(),put(), and_find_best_match()can race and leave the paired lists out of sync or fail mid-iteration.🤖 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 `@semantic_cache.py` around lines 139 - 157, Add a shared lock to SemanticCache and use it to synchronize all reads and mutations of the paired _entries and _access_times lists across get(), put(), and _find_best_match(). Ensure each operation holds the lock for its complete iteration or update, including eviction, so concurrent calls from the event loop and asyncio.to_thread cannot leave the lists inconsistent.study.py (3)
258-277: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the referenced file and inspect the relevant sections with line numbers. git ls-files | rg '^study\.py$|/study\.py$|study\.py$' || true wc -l study.py sed -n '220,340p' study.py | cat -nRepository: Deen-Bridge/dnb-ai
Length of output: 4997
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the file around the cited endpoint and generator implementation. sed -n '220,340p' study.py | cat -nRepository: Deen-Bridge/dnb-ai
Length of output: 4955
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the endpoint and call chain around the second cited range. sed -n '480,545p' study.py | cat -n # Also inspect tests for any concurrency/offload expectations. sed -n '1,260p' tests/test_study.py | cat -nRepository: Deen-Bridge/dnb-ai
Length of output: 12767
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '480,545p' study.py | cat -n sed -n '1,260p' tests/test_study.py | cat -nRepository: Deen-Bridge/dnb-ai
Length of output: 12767
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the helper that the async route calls. sed -n '340,490p' study.py | cat -nRepository: Deen-Bridge/dnb-ai
Length of output: 6612
Offload Gemini generation from the async route.
generate_study_contentisasync, but it still runs the sync_generate_content()→GeminiGenerator.generate()→model.generate_content(...)chain on the event loop. That blocks other requests while the network call is in flight. Userun_in_threadpoolfor the current sync path, or switch togenerate_content_asyncand make the helper chain async.🤖 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 `@study.py` around lines 258 - 277, Update the async generate_study_content flow to execute the synchronous _generate_content → GeminiGenerator.generate chain via run_in_threadpool, ensuring model.generate_content does not block the event loop while preserving the existing response and error behavior.Source: Path instructions
307-313: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find the relevant file and symbols. git ls-files | rg '(^|/)study\.py$|MAX_LESSON_TEXT_LENGTH|LessonTextSource|_build_prompt|lesson_text' # Inspect outline if available, otherwise fall back to targeted reads. if [ -f study.py ]; then echo "=== study.py lines around 280-340 ===" sed -n '280,340p' study.py fi # Search for the constant and prompt-building references. rg -n "MAX_LESSON_TEXT_LENGTH|LessonTextSource|_build_prompt|lesson_text[:=]" .Repository: Deen-Bridge/dnb-ai
Length of output: 3719
🏁 Script executed:
#!/bin/bash set -euo pipefail git ls-files | rg '(^|/)study\.py$|MAX_LESSON_TEXT_LENGTH|LessonTextSource|_build_prompt|lesson_text' if [ -f study.py ]; then echo "=== study.py lines around 280-340 ===" sed -n '280,340p' study.py fi rg -n "MAX_LESSON_TEXT_LENGTH|LessonTextSource|_build_prompt|lesson_text[:=]" .Repository: Deen-Bridge/dnb-ai
Length of output: 3719
🏁 Script executed:
#!/bin/bash set -euo pipefail pwd ls -1Repository: Deen-Bridge/dnb-ai
Length of output: 396
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show the prompt-building tests. sed -n '200,280p' tests/test_study.py # Search for any docs or comments explaining the 500-char prompt limit. rg -n "500|token budget|prompt budget|truncate|truncat|lesson_text" README.md CONTRIBUTING.md study.py tests/test_study.py main.py review.py fiqh.py hadith.py history.py stellar.py safety scripts dataRepository: Deen-Bridge/dnb-ai
Length of output: 50375
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== tests/test_study.py around prompt-building tests ===" sed -n '212,265p' tests/test_study.py echo "=== focused docs/search for prompt size / truncation notes ===" rg -n -m 20 "prompt budget|token budget|500 chars|500-character|500 char|truncate|truncat|MAX_LESSON_TEXT_LENGTH|lesson_text" README.md CONTRIBUTING.md study.py tests/test_study.py main.py review.py review_store.py semantic_cache.pyRepository: Deen-Bridge/dnb-ai
Length of output: 3993
Forward more of
lesson_textinto the prompt._build_promptcuts lessons to 500 chars even thoughLessonTextSource.lesson_textallows 20,000. That silently drops most of larger submissions, so generated quizzes/flashcards can miss the actual source. If the cap is intentional for token budget, make it configurable and document it; otherwise remove it or tie it toMAX_LESSON_TEXT_LENGTH.🤖 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 `@study.py` around lines 307 - 313, Update _build_prompt so lesson_text is not hard-truncated to 500 characters; preserve the full LessonTextSource.lesson_text up to its 20,000-character limit, or replace the fixed cap with the existing MAX_LESSON_TEXT_LENGTH configuration and document the token-budget behavior if truncation is required.
412-436: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Empty
quizzes/flashcardsarray silently passes validation.The error message says
"'quizzes' must be a non-empty array"(Line 415) but the code only checksisinstance(raw, list)— an empty list[]passes this check and producesresult["quizzes"] = [], contradicting the message and silently returning fewer items than the requestedcount. Consider adding an explicit emptiness check (and no test currently covers this case).🔧 Proposed fix
raw = data.get("quizzes") - if not isinstance(raw, list): + if not isinstance(raw, list) or not raw: errors.append("'quizzes' must be a non-empty array")(Apply the analogous change to the
flashcardsbranch below.)Separately, static analysis flags blind
except Exceptionat Lines 421 and 434 (Ruff BLE001) — narrowing topydantic.ValidationError(plus perhapsTypeError) would make failure modes more explicit, though the current broad catch is defensible here since it feeds the retry loop.🧰 Tools
🪛 Ruff (0.15.21)
[warning] 421-421: Do not catch blind exception:
Exception(BLE001)
[warning] 434-434: 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 `@study.py` around lines 412 - 436, Update the quiz and flashcard validation branches to reject empty lists as well as non-list values, preserving the existing “must be a non-empty array” errors and avoiding insertion of empty results. Apply the same emptiness check in both branches; leave the broader exception handling unchanged unless needed for this validation fix.Source: Linters/SAST tools
tests/redteam/test_live.py (1)
16-19: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
According to the official Gemini API documentation, which supported model and Pythongoogle-genaiSDK API should replacegemini-2.5-flash-preview-05-20andgoogle-generativeaifor a JSON classification audit?💡 Result:
To migrate from the legacy
google-generativeaiSDK and thegemini-2.5-flash-preview-05-20model for your JSON classification audit, you should adopt the following: 1. Recommended SDK: You must migrate to thegoogle-genaiPython SDK [1][2]. Thegoogle-generativeailibrary is considered a legacy library, is no longer actively maintained, and is deprecated [2]. You can install the new SDK usingpip install google-genai[3]. 2. Recommended Model: Thegemini-2.5-flash-preview-05-20model has been deprecated [4]. For a current production replacement, you should usegemini-3.6-flash[4]. Thegoogle-genaiSDK introduces a new, centralizedClientobject architecture, which simplifies credential management and provides a more consistent interface for interacting with models, including features like structured JSON responses using Pydantic [1][5]. Migration guides provided by Google offer direct before-and-after code examples to facilitate this transition [1].Citations:
- 1: https://ai.google.dev/gemini-api/docs/migrate
- 2: https://ai.google.dev/gemini-api/docs/libraries
- 3: https://cloud.google.com/vertex-ai/generative-ai/docs/start/libraries
- 4: https://ai.google.dev/gemini-api/docs/deprecations
- 5: https://ai.google.dev/gemini-api/docs/migrate.md.txt
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="tests/redteam/test_live.py" printf '\n== file outline ==\n' ast-grep outline "$FILE" --view expanded || true printf '\n== relevant lines ==\n' nl -ba "$FILE" | sed -n '1,120p' printf '\n== SDK/model references in repo ==\n' rg -n "google-generativeai|google-genai|gemini-2\.5-flash-preview-05-20|gemini-3\.6-flash" .Repository: Deen-Bridge/dnb-ai
Length of output: 341
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== test_live.py ==\n' sed -n '1,120p' tests/redteam/test_live.py printf '\n== dependency references ==\n' rg -n "google-generativeai|google-genai" requirements* pyproject.toml poetry.lock uv.lock Pipfile* . -g '!**/.git/**' || trueRepository: Deen-Bridge/dnb-ai
Length of output: 1537
Replace the deprecated Gemini client and model in the live audit.
tests/redteam/test_live.pystill importsgoogle.generativeaiand instantiatesgemini-2.5-flash-preview-05-20, andrequirements.txtpinsgoogle-generativeai==0.8.3. Move this test togoogle-genaiand a supported model so the opt-in audit can reach its assertions instead of failing up front.🤖 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/redteam/test_live.py` around lines 16 - 19, Update the live audit setup in test_live.py to use the google-genai client instead of the deprecated google.generativeai import, configure it with the existing GEMINI_API_KEY, and select a currently supported Gemini model. Update requirements.txt to replace the google-generativeai==0.8.3 dependency with the corresponding google-genai package, preserving the test’s existing assertions and opt-in behavior.
2 participants
Overview
This PR adds a token budget system to cap conversation history and prevent unbounded prompt growth in chat sessions. Before each Gemini call, the oldest turn-pairs are dropped until both the token budget and turn cap are satisfied.
Related Issue
Closes #13
Changes
History Budget and Truncation
API Surface
Documentation
Tests
Summary by CodeRabbit
truncatedresponse flag.