feat: add tafsir layer with retrieved, attributed ayah explanations (#54) - #64
Conversation
Verse interpretation was answered from model memory: no named mufassir, no way to see how Ibn Kathir or al-Sa'di actually explained an ayah, and no distinction between a classical reading and a modern gloss. This adds a tafsir layer that replaces recall with retrieval: - POST /tafsir returns per-tafsir explanations for an ayah or a short range, each attributed to a named work, author, and the language the text is in. - GET /tafsir/sources lists the retrievable works and their languages. - Ayah references are validated offline against a bundled surah index (data/quran/surah_index.json, built by scripts/build_surah_index.py), so an out-of-range reference is a 400 naming the real bound rather than an invented verse. - /chat detects verse-explanation questions offline and answers from the same retrieved passages, instructing the model to attribute each claim to a named mufassir and to surface — not flatten — where they differ. - A tafsir unavailable for an ayah degrades to a labelled "unavailable" entry; the rest of the response is unaffected. - Tafsir text is immutable per ayah, so it is cached by exact ayah key through semantic_cache.KeyedCache — the keyed sibling of the semantic response cache, sharing its TTL and eviction config rather than adding a second cache system. Language labels come from the edition that was fetched, not from the API's translated_name.language_name, which describes the language of the work's *name* and would label al-Tabari's Arabic commentary "english". Tests run fully offline against a fake source; CI now lints and compiles the new modules and runs the tafsir suite. Closes Deen-Bridge#54
WalkthroughAdds a tafsir layer with offline Quran reference validation, multi-work Quran.com retrieval, keyed caching, dedicated endpoints, chat grounding, attribution metadata, documentation, and CI tests. ChangesTafsir grounding feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ChatHandler
participant tafsir_retriever
participant QuranComTafsirSource
participant Model
Client->>ChatHandler: POST /chat with verse question
ChatHandler->>tafsir_retriever: retrieve tafsir context
tafsir_retriever->>QuranComTafsirSource: fetch verse and tafsir passages
QuranComTafsirSource-->>tafsir_retriever: attributed tafsir data
tafsir_retriever-->>ChatHandler: TafsirContext and TafsirInfo
ChatHandler->>Model: generate with tafsir instructions
Model-->>Client: grounded response with tafsir metadata
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
🧹 Nitpick comments (4)
tafsir.py (4)
1099-1102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRe-raise
InvalidReferencewith exception chaining.
raise HTTPException(...)insideexcept InvalidReference as exc:drops the original traceback link.flake8-bugbear's B904 flags exactly this pattern, and per path instructions CI enforces flake8-based style checks — worth confirming this rule is active in CI, since it would otherwise fail the build.As per path instructions, "CI enforces flake8, so style violations fail the build."🔧 Proposed fix
except InvalidReference as exc: - raise HTTPException(status_code=400, detail=str(exc)) + raise HTTPException(status_code=400, detail=str(exc)) from exc🤖 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 `@tafsir.py` around lines 1099 - 1102, Update the InvalidReference handler around build_tafsir_response to explicitly chain the raised HTTPException from the caught exception, using the existing exc binding and preserving the 400 status and message detail.Sources: Path instructions, Linters/SAST tools
582-628: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNew
httpx.AsyncClientper request — no connection reuse.
_getopensasync with httpx.AsyncClient(timeout=self.timeout) as client:on every single call, and it's called once for the verse fetch and once per tafsir per ayah. A single/tafsirrequest for a 3-ayah range with the 4 default tafsirs can spin up over a dozen separate clients (each paying its own TCP/TLS handshake toapi.quran.com) instead of reusing pooled connections.Consider holding a module-level (or app-lifespan-scoped)
httpx.AsyncClientand reusing it across calls;set_source/tests are unaffected since they substituteFakeTafsirSourceentirely.🤖 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 `@tafsir.py` around lines 582 - 628, The _get method creates a new httpx.AsyncClient for every Quran API request, preventing connection reuse. Update the containing client class to own and reuse a single AsyncClient across _get calls, while preserving the existing timeout and response/error handling; ensure the client is properly closed through the appropriate lifecycle cleanup.
813-825: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNo length bound on
reference/tafsirsinTafsirRequest.
reference: strandtafsirs: Optional[List[str]]accept arbitrarily long input;resolve_requested_tafsirstruncatestafsirsonly after the full list is parsed by Pydantic, andreferencehas no cap before it's regex-matched. Neither is catastrophic (both regexes here are linear, not exponential), but aField(max_length=...)on each is cheap, self-documenting request-body validation that rejects oversized payloads before they're processed.As per path instructions for FastAPI services: "missing Pydantic validation on request bodies" should be flagged.🛡️ Proposed fix
reference: str = Field( ..., description="Ayah reference: '103:1', a range '103:1-3', or 'Al-Asr 1-3'", json_schema_extra={"examples": ["103:1-3", "2:255", "Al-Fatihah 1"]}, + max_length=64, ) tafsirs: Optional[List[str]] = Field( None, description=( "Tafsir keys to include (see GET /tafsir/sources). " f"Defaults to {list(DEFAULT_TAFSIR_KEYS)}." ), + max_length=MAX_TAFSIRS_PER_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 `@tafsir.py` around lines 813 - 825, Add explicit Pydantic size constraints to both fields in TafsirRequest: set a reasonable max_length for reference and constrain tafsirs as a list before resolve_requested_tafsirs processes it, while preserving the existing optional behavior and descriptions.Source: Path instructions
714-789: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential
awaitin per-ayah/per-tafsir loops — avoidable latency on a user-facing path.
fetch_tafsirs_for_ayah'sfor key in keys:loop awaits each tafsir fetch one at a time, and bothbuild_tafsir_responseandbuild_chat_tafsir_contextawaitfetch_verse_textthenfetch_tafsirs_for_ayahsequentially for each ayah in the range. For a range request withMAX_AYAT_PER_REQUESTayat andMAX_TAFSIRS_PER_REQUESTworks, that's dozens of sequential round-trips to Quran.com on both the/tafsirendpoint and the/chatgrounding path — directly adding to response latency users feel.Since each fetch is independent (attribution is per-key, order can be preserved by list position), these are good candidates for
asyncio.gather— cache hits stay cheap, only genuine upstream misses pay the round-trip, and they'd now run concurrently instead of serially.Also applies to: 891-922, 996-1031
🤖 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 `@tafsir.py` around lines 714 - 789, Update fetch_tafsirs_for_ayah to schedule independent per-key tafsir fetches with asyncio.gather while preserving keys order and each unavailable result. In build_tafsir_response and build_chat_tafsir_context, run per-ayah verse and tafsir work concurrently and gather ayah tasks across the requested range, preserving response ordering and existing cache, fallback, and error semantics.
🤖 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 `@main.py`:
- Around line 162-174: Update tafsir_retriever to wrap the awaitable
build_chat_tafsir_context call in an aggregate timeout, ensuring slow Quran.com
retrieval returns None within the configured limit while preserving the existing
warning and best-effort fallback behavior for timeout or other exceptions.
In `@README.md`:
- Around line 107-128: Update the README JSON example to include the top-level
disclaimer field alongside reference, language, and ayat, using the same shape
and representative value returned by TafsirResponse. Keep the existing tafsir
and unavailable examples unchanged.
In `@tafsir.py`:
- Around line 279-296: The prefix handling in _normalize_surah_name() must also
remove assimilated article forms so names such as “at-tawbah” and “ash-shams”
produce the same canonical key as their lookup counterparts. Update the
normalization logic to reuse the assimilated prefixes accepted by
NAMED_SURAH_PATTERNS[0], while preserving existing punctuation removal,
whitespace collapsing, and al/ul normalization behavior.
---
Nitpick comments:
In `@tafsir.py`:
- Around line 1099-1102: Update the InvalidReference handler around
build_tafsir_response to explicitly chain the raised HTTPException from the
caught exception, using the existing exc binding and preserving the 400 status
and message detail.
- Around line 582-628: The _get method creates a new httpx.AsyncClient for every
Quran API request, preventing connection reuse. Update the containing client
class to own and reuse a single AsyncClient across _get calls, while preserving
the existing timeout and response/error handling; ensure the client is properly
closed through the appropriate lifecycle cleanup.
- Around line 813-825: Add explicit Pydantic size constraints to both fields in
TafsirRequest: set a reasonable max_length for reference and constrain tafsirs
as a list before resolve_requested_tafsirs processes it, while preserving the
existing optional behavior and descriptions.
- Around line 714-789: Update fetch_tafsirs_for_ayah to schedule independent
per-key tafsir fetches with asyncio.gather while preserving keys order and each
unavailable result. In build_tafsir_response and build_chat_tafsir_context, run
per-ayah verse and tafsir work concurrently and gather ayah tasks across the
requested range, preserving response ordering and existing cache, fallback, and
error semantics.
🪄 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: 656582d8-9ce8-427f-bb20-b1a9e1a53212
📒 Files selected for processing (11)
.env.example.github/workflows/ci.ymlREADME.mddata/quran/PROVENANCE.mddata/quran/surah_index.jsonmain.pyrequirements.txtscripts/build_surah_index.pysemantic_cache.pytafsir.pytests/test_tafsir.py
Addresses review feedback on the tafsir layer.
Surah-name normalization only stripped "al"/"ul", so every surah whose
article assimilates into a sun letter normalized to a different key than
the lookup path produced: "surah at-tawbah 5", "as-sajdah", "ash-shams"
and "As-Saff 4" all failed to resolve, and even surah_by_name("Al-Sajdah")
returned None. The article is now stripped in whichever form it was
written, but only when the consonant it assimilated into actually doubles
— so Al-Anfal keeps its "an" rather than becoming "fal". Tests assert
every one of the 114 canonical names and every alias resolves to its own
surah, and that no two surahs share a normalized key.
Retrieval was fully sequential, so a verse-range prompt could stack one
upstream timeout per ayah per work before falling back to an ungrounded
answer. Ayat and the works within an ayah are now fetched concurrently,
and retrieval inside /chat is bounded by TAFSIR_CHAT_TIMEOUT (default
20s): past the budget the turn proceeds ungrounded rather than stalling.
Both paths share one assemble_ayah helper so /tafsir and /chat degrade
identically.
Also documents the disclaimer field in the README response example.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_tafsir.py (1)
710-736: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWall-clock threshold test carries some CI-flakiness risk.
assert elapsed < 0.4against a documented 0.75s serial baseline gives roughly a 2x margin, which is reasonable but timing-based assertions like this can occasionally flake on busy/shared CI runners (contended CPU, GC pauses, etc.), especially as more tests run in parallel over time. Consider asserting concurrency via recorded call timestamps/overlap (or viasource.tafsir_calls/verse_callsordering) instead of a hard wall-clock bound, or widen the margin further if you want to keep it simple.🤖 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_tafsir.py` around lines 710 - 736, Replace the wall-clock assertion in test_retrieval_runs_concurrently_across_ayat with a deterministic concurrency check using recorded tafsir_calls/verse_calls timestamps or ordering, verifying independent ayat retrieval overlaps rather than relying on elapsed < 0.4. If retaining timing-based validation, widen the threshold to reduce CI flakiness while still distinguishing it from the documented serialized baseline.
🤖 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.
Nitpick comments:
In `@tests/test_tafsir.py`:
- Around line 710-736: Replace the wall-clock assertion in
test_retrieval_runs_concurrently_across_ayat with a deterministic concurrency
check using recorded tafsir_calls/verse_calls timestamps or ordering, verifying
independent ayat retrieval overlaps rather than relying on elapsed < 0.4. If
retaining timing-based validation, widen the threshold to reduce CI flakiness
while still distinguishing it from the documented serialized baseline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 61399c27-22b0-4420-8a90-a426b0e1612e
📒 Files selected for processing (4)
.env.exampleREADME.mdtafsir.pytests/test_tafsir.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .env.example
- README.md
|
@Fury03 please fix conflict |
Resolves conflicts with the confidence and scholar-review layer (Deen-Bridge#56), which landed on dev while this branch was in review. Both features touch the same files; nothing was dropped from either side: - main.py: both routers registered, both import blocks kept, and ChatResponse carries both the tafsir and confidence blocks. In the chat handler the two paths compose in order — tafsir retrieval runs first and marks a grounded turn non-cacheable, then confidence assessment, abstention and scholar queueing run on the generated answer, and the cache write is gated on both conditions. - ci.yml: lint and compile cover both feature's modules; both test steps kept. - requirements.txt, .env.example, README.md: both features' entries and sections merged. Full suite green with both features present: 462 passed, 1 skipped.
|
@zeemscript conflicts resolved in bf04442 — the branch is mergeable again and CI is green. The conflicts came from #56 (confidence + scholar review) landing on
Verified beyond the test suite by driving the merged chat handler end-to-end against a stubbed model with live tafsir retrieval: "What does Surah al-Asr mean?" returns all three ayat grounded in four attributed works with the grounding block reaching the prompt, and a mixed tafsir+fiqh question ("Explain 2:275 and tell me the ruling on riba") correctly triggers both the tafsir path and the fiqh path in one turn. Full suite with both features present: 462 passed, 1 skipped. One composition note, deliberately left out of the conflict fix since it changes behaviour rather than resolving a conflict: a tafsir-grounded answer currently scores in the |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (2)
222-222: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the invalid-reference example wording.
“
2:300is a400naming…” is malformed and does not clearly state that the API returns HTTP 400. This could confuse clients relying on the documented validation contract.📝 Proposed fix
- so `2:300` is a `400` naming Al-Baqarah's 286 ayat — never an invented verse. + so `2:300` returns a `400` naming Al-Baqarah's 286 ayat — never an invented verse.🤖 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` at line 222, Update the invalid-reference example wording in the README so it clearly states that requesting 2:300 returns HTTP 400 because Al-Baqarah has only 286 ayat; remove the malformed “is a 400” phrasing while preserving the validation-contract meaning.
125-125: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language identifier to the fenced code block.
This triggers MD040. Mark the block as
text(or another appropriate language) so Markdown tooling can validate it consistently.🛠️ Proposed fix
-``` +```text🤖 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` at line 125, Update the fenced code block in README.md to include an explicit language identifier, using text or another appropriate language, so Markdown linting satisfies MD040.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.
Outside diff comments:
In `@README.md`:
- Line 222: Update the invalid-reference example wording in the README so it
clearly states that requesting 2:300 returns HTTP 400 because Al-Baqarah has
only 286 ayat; remove the malformed “is a 400” phrasing while preserving the
validation-contract meaning.
- Line 125: Update the fenced code block in README.md to include an explicit
language identifier, using text or another appropriate language, so Markdown
linting satisfies MD040.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 39eba816-ca9a-4363-b9d3-dd3c0466c2b7
📒 Files selected for processing (5)
.env.example.github/workflows/ci.ymlREADME.mdmain.pyrequirements.txt
🚧 Files skipped from review as they are similar to previous changes (4)
- requirements.txt
- .env.example
- .github/workflows/ci.yml
- main.py
2 participants
Closes #54
What
Asked "what does Surah al-'Asr mean?", the service produced a paraphrase from the model's own memory — no named tafsir, no way to see how Ibn Kathir or al-Sa'di actually explained the ayah. This PR replaces recall with retrieval.
POST /tafsirAccepts a
surah:ayahreference, a range, or a surah name, plus an optional list of tafsirs and a language. Returns the ayah text, a translation, and per-tafsir explanations — each one attributed to a named work, its author, and the language the text is actually in.GET /tafsir/sourceslists the retrievable works (Ibn Kathir, al-Tabari, al-Qurtubi, al-Sa'di, al-Baghawi, Ma'arif al-Qur'an, and others) and their available languages.Reference validation
data/quran/surah_index.json(built byscripts/build_surah_index.py, provenance indata/quran/PROVENANCE.md) carries surah numbers, names, and ayah counts — no Quranic text. Bounds are checked offline before any network call, so2:300returns a 400 naming Al-Baqarah's 286 ayat instead of a hallucinated verse. The build script asserts 114 surahs / 6236 ayat, so a typo fails the build.Chat integration
A verse-explanation question in
/chat("explain 2:255", "what does Surah al-'Asr mean?") is detected offline via regex + the surah index, then answered from retrieved passages. The system context requires attributing each claim to a named mufassir, forbids adding interpretation from memory, and requires surfacing — not flattening — points where the mufassirun differ.ChatResponsegains an additive optionaltafsirblock naming the works whose text actually backed the answer.Intent detection is deliberately conservative: without the word "surah", both the definite article and an explicit ayah number are required, so "what does ar-Rahman mean?" or a question about the name Muhammad is never read as a surah reference.
Caching (coordinated with #27)
Tafsir text is immutable per ayah, so it is cached by exact ayah key through a new
semantic_cache.KeyedCache— the keyed sibling of the semantic response cache, living in the same module and sharing its TTL and eviction configuration. It is not a second cache system; approximate embedding similarity is simply the wrong matching rule for ayah keys, where2:255and2:256are near-identical strings that must never match each other. Grounded tafsir answers skip the semantic response cache for the same reason — the expensive part is already cached at the retrieval layer.Correctness notes worth reviewing
translated_name.language_namedescribes the language the work's name was translated into — it reports "english" for al-Tabari on an English-locale request, even though the commentary is Arabic. Labelling it that way would be a misattribution, so the label follows the edition this service asked for. Covered by a test.unavailablewith a reason rather than an empty explanation dressed up as commentary. Retrieval failures in/chatare logged and the turn proceeds without tafsir rather than 500ing.Tests
122 new offline tests in
tests/test_tafsir.pyagainst aFakeTafsirSource— reference validation and bounds, response assembly, attribution preservation, both readings surfaced when two tafsirs differ, unavailable/empty degradation, language fallback and labelling, cache hit and key isolation, intent detection (including the false-positive guards), and prompt-block assembly. No live API calls.CI extends flake8 and
compileallto the new modules and adds a tafsir pytest step. Full suite: 336 passed, 1 skipped.Notes
httpx==0.28.1pinned inrequirements.txt(async client — no blocking calls in the async path).QURAN_API_BASE,QURAN_API_TIMEOUT,TAFSIR_MAX_AYAT, andTAFSIR_CHAT_EXCERPT_CHARSare configurable and documented in.env.exampleand the README.Summary by CodeRabbit
/tafsirand/tafsir/sourcesendpoints (language selection, verse ranges, offline bounds validation, and unavailable-work notices).