feat(citations): return structured Quran and Hadith citations on chat answers - #83
Merged
Merged
Conversation
… answers Adds a citations module that parses a delimited JSON block appended after the model's prose, validates Quran references against the 114-surah index and Hadith collections against the existing alias and grading tables, and returns typed citation objects on /chat and /chat/stream. Feeds the citation_verification confidence signal that confidence.py already reserved but nothing produced. Parsing is total: malformed, truncated, or absent blocks yield an empty list and never fail a turn. Adds 59 tests, an offline eval script with extraction/validity/rejection floors, and CI wiring. Closes Deen-Bridge#15
WalkthroughAdds structured Quran, Hadith, and scholarly citations with defensive validation, non-streaming and streaming chat integration, confidence scoring, offline evaluation, tests, CI coverage, and README documentation. ChangesStructured citations
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ChatEndpoint
participant Model
participant CitationStreamFilter
Client->>ChatEndpoint: Send chat request
ChatEndpoint->>Model: Generate response with citation instructions
Model-->>ChatEndpoint: Return prose and citation block
ChatEndpoint->>CitationStreamFilter: Filter streamed output
CitationStreamFilter-->>Client: Emit prose-only deltas
ChatEndpoint-->>Client: Send terminal done event with citations
Possibly related PRs
Suggested reviewers: 🚥 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: 5
🧹 Nitpick comments (4)
tests/test_citations_integration.py (1)
159-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test name promises more than it asserts.
test_validated_citations_raise_confidence_above_the_unverified_ceilingonly checksscored["score"] > plain["score"], which would still pass if both stayed pinned under the 0.65 ceiling. Since liftingCONFIDENCE_UNVERIFIED_CEILINGis the headline claim in the README, it's worth asserting directly — otherwise a regression that reinstates the cap goes unnoticed.💚 Suggested extra assertion
assert "citation_verification" not in plain["signals"] assert scored["score"] > plain["score"] + # The point of an external signal: the unverified cap no longer applies. + from confidence import UNVERIFIED_CEILING + + assert plain["score"] <= UNVERIFIED_CEILING + assert scored["score"] > UNVERIFIED_CEILINGAdjust the import to whatever
confidence.pyactually exports.🤖 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_citations_integration.py` around lines 159 - 178, Strengthen test_validated_citations_raise_confidence_above_the_unverified_ceiling by importing the exported CONFIDENCE_UNVERIFIED_CEILING from confidence.py and asserting scored["score"] exceeds that ceiling, while retaining the existing signal and comparison assertions.citations.py (1)
241-250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider bounding the free-text scholarly fields.
Nice touch accepting both
work/titleanddetail/note— models are inconsistent about exactly that. One asymmetry though: the Quran and hadith paths are validated against bundled data, whilework,author, anddetailare model-authored strings echoed to the client with no length bound. WithMAX_CITATIONS = 24a badly behaving model could inflate a response with 24 multi-KB blobs. A cheap truncation keeps the response shape predictable.♻️ Optional: truncate scholarly free text
+_MAX_FIELD_LEN = 200 + + +def _bounded(value: Optional[str]) -> Optional[str]: + return value[:_MAX_FIELD_LEN] if value else None + + def _build_scholarly(raw: Dict[str, Any]) -> Tuple[Optional[ScholarlyReference], Optional[str]]: work = _clean_str(raw.get("work")) or _clean_str(raw.get("title")) if not work: return None, "scholarly reference without a work" citation = ScholarlyReference( - work=work, - author=_clean_str(raw.get("author")), - detail=_clean_str(raw.get("detail")) or _clean_str(raw.get("note")), + work=_bounded(work), + author=_bounded(_clean_str(raw.get("author"))), + detail=_bounded(_clean_str(raw.get("detail")) or _clean_str(raw.get("note"))), ) return citation, None🤖 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 `@citations.py` around lines 241 - 250, Bound the model-authored scholarly fields in _build_scholarly before constructing ScholarlyReference: truncate work, author, and detail to the project’s established maximum free-text length, while preserving the existing work/title and detail/note fallback behavior and missing-work validation.tests/test_citations.py (1)
218-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten the cap assertion.
attempted <= 24passes even if the cap regressed to 1 (or if the loop stopped parsing entirely). Since all 59 fixtures are valid, distinct ayat in Al-Baqarah, you can pin both ends of the behaviour exactly.💚 Suggested assertion
extraction = parse_citations(json.dumps({"citations": many})) - assert extraction.attempted <= 24 + assert extraction.attempted == MAX_CITATIONS + assert len(extraction.citations) == MAX_CITATIONSwith
MAX_CITATIONSadded to the import block at the top.🤖 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_citations.py` around lines 218 - 222, Strengthen TestCaps.test_citation_count_is_capped by importing MAX_CITATIONS and asserting extraction.attempted equals MAX_CITATIONS, while also asserting it is less than the 59 valid input citations to verify both the cap and continued parsing..github/workflows/ci.yml (1)
31-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider replacing the hand-maintained file lists with a flake8 config.
Every new module now has to be appended to two ~400-character lines, and they've already drifted:
tests/test_citations.pyandtests/test_citations_integration.pyare in theflake8list but missing fromcompileall. Asetup.cfg/.flake8withexcludeplusflake8 .makes new files linted by default and removes the class of "forgot to add the file" gaps. As per path instructions, "CI enforces flake8, so style violations fail the build" — which is exactly why the enumeration shouldn't be able to silently skip a file.♻️ Sketch
.flake8:[flake8] max-line-length = 120 extend-ignore = E501,W503 exclude = .git,__pycache__,venv,datathen:
- name: Run linting - run: flake8 main.py citations.py memory ... --max-line-length=120 --ignore=E501,W503 + run: flake8 . - name: Check syntax - run: python -m compileall -q main.py citations.py memory ... + run: python -m compileall -q .🤖 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 31 - 34, Replace the hand-maintained file lists in the CI lint and syntax-check steps with repository-wide discovery: configure Flake8 via a root .flake8 or setup.cfg using the existing max-line-length, ignored rules, and generated-directory exclusions, then run flake8 . so new modules are included automatically. Update the compileall step to use an equivalent recursive source selection or otherwise include the same repository Python files, preserving the existing exclusions and avoiding drift between the two checks.Source: Path instructions
🤖 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 `@citations.py`:
- Around line 292-319: Update the duplicate-citation branch in the citation
extraction loop to avoid incrementing attempted for entries that collapse into
an existing citation, while preserving the single citation in
extraction.citations and the existing deduplication behavior. Adjust the flow
around extraction.attempted and the fingerprint check, and extend
TestDeduplication.test_identical_citations_collapse to assert the corrected
score.
- Around line 387-397: Update finish() in the _in_block branch to retain and
return the prose portion produced by extract_citations(self._tail), rather than
discarding it and returning only _pending. Preserve citation extraction and
pending-buffer cleanup while ensuring trailing text after the citation block is
emitted consistently with the non-streaming path.
- Around line 333-342: Update the citation extraction flow around
_BLOCK_PATTERN.search to remove every complete citation block from visible
prose, while keeping the first matched block’s payload authoritative for
parse_citations. After removing complete blocks, also strip any unterminated
citation tail using _UNTERMINATED_PATTERN before returning the prose, preserving
the existing behavior when no complete block exists.
In `@README.md`:
- Around line 280-289: Update the README JSON example’s hadith citation so the
number field is represented as the API returns it: a string value rather than an
integer. Keep the example’s other citation fields unchanged.
In `@scripts/eval_citations.py`:
- Around line 106-115: Update the live evaluation branch around
fetch_live_answer so it does not derive attempted, valid, or rejection-rate
metrics from the server’s surviving citations. In live mode, validate that no
planted-invalid reference appears in the returned citations, and guard the
rejection-rate output and MIN_REJECTION_RATE assertion so they apply only to
offline evaluation; preserve normal citation leak detection and offline scoring.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 31-34: Replace the hand-maintained file lists in the CI lint and
syntax-check steps with repository-wide discovery: configure Flake8 via a root
.flake8 or setup.cfg using the existing max-line-length, ignored rules, and
generated-directory exclusions, then run flake8 . so new modules are included
automatically. Update the compileall step to use an equivalent recursive source
selection or otherwise include the same repository Python files, preserving the
existing exclusions and avoiding drift between the two checks.
In `@citations.py`:
- Around line 241-250: Bound the model-authored scholarly fields in
_build_scholarly before constructing ScholarlyReference: truncate work, author,
and detail to the project’s established maximum free-text length, while
preserving the existing work/title and detail/note fallback behavior and
missing-work validation.
In `@tests/test_citations_integration.py`:
- Around line 159-178: Strengthen
test_validated_citations_raise_confidence_above_the_unverified_ceiling by
importing the exported CONFIDENCE_UNVERIFIED_CEILING from confidence.py and
asserting scored["score"] exceeds that ceiling, while retaining the existing
signal and comparison assertions.
In `@tests/test_citations.py`:
- Around line 218-222: Strengthen TestCaps.test_citation_count_is_capped by
importing MAX_CITATIONS and asserting extraction.attempted equals MAX_CITATIONS,
while also asserting it is less than the 59 valid input citations to verify both
the cap and continued parsing.
🪄 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: 04a57875-20ae-40fe-ba95-69d77cad6651
📒 Files selected for processing (8)
.github/workflows/ci.ymlREADME.mdcitations.pydata/eval/citations_eval.jsonlmain.pyscripts/eval_citations.pytests/test_citations.pytests/test_citations_integration.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Chat answers now carry a
citationslist of validated, typed references.2:153arrives as a surah number, an ayah number and the surah's name from the index — not as a substring the client has to parse back out of prose.{ "response": "Allah counsels the believers to seek help in patience and prayer...", "citations": [ {"type": "quran", "surah": 2, "ayah_start": 153, "ayah_end": null, "surah_name": "Al-Baqarah"}, {"type": "hadith", "collection": "Sahih al-Bukhari", "number": 1, "grading": "sahih"}, {"type": "scholarly", "work": "Riyad as-Salihin", "author": "Al-Nawawi", "detail": "Book of Patience"} ] }The field is additive on
ChatResponseand on the/chat/streamterminaldoneevent, so existing clients are unaffected.A delimited block, not whole-response JSON
Forcing the whole answer into JSON degrades prose quality, and one malformed brace loses the entire answer. The model instead appends a single
<<<CITATIONS>>> … <<<END_CITATIONS>>>block after its prose, which is parsed off and never shown. On the streaming endpoint a small hold-back filter keeps a half-emitted marker out of the SSE deltas.Parsing is total. Malformed, truncated or absent blocks yield an empty list and the prose is still returned; a block cut off by
max_output_tokensis stripped from the prose anyway. Nothing in this path can fail a chat turn, and a test class is dedicated to exactly that.Validation reuses what the repo already trusts
Quran bounds come from
data/quran/surah_index.jsonthroughtafsir.surah_by_number/surah_by_name— the same 114-surah index the tafsir layer validates against, so the two cannot drift apart, and2:300is refused against Al-Baqarah's real 286 ayat.surah_nameis always taken from that index and never from the model, which is what makes the field trustworthy.Hadith collections go through
hadith.normalize_collection's existing alias table rather than a second one, and gradings are read from the bundled grading dataset rather than from the model's claim. An unrecognised collection is rejected, not echoed back. Citations are deduplicated and capped at 24 per answer.corpus.pyis deliberately not used: its backingdata/quran_uthmani.jsonis currently a 642-byte stub holding only surahs 1, 2 and 114, so bounds-checking against it would report valid citations as fabricated.This completes #40 rather than duplicating it
confidence.pyhas always reserved acitation_verificationsignal at weight 0.30, andmain.pyhas always carried aCitationVerificationResulttype — but nothing produced either, andverifier.pyhad exactly one importer, its own test.Closing that gap is the headline effect. The share of a turn's attempted citations that validated is now fed into
build_signals, so a well-cited answer is no longer capped byCONFIDENCE_UNVERIFIED_CEILING(0.65) and stops being hedged as unverified. An answer that cites nothing produces no signal at all and is not penalised for it.Testing
59 new tests.
tests/test_citations.py(42) covers block splitting, malformed input, Quran and Hadith validation, scholarly references, type inference, scoring, deduplication, caps and the stream filter.tests/test_citations_integration.py(17) drives the real/chathandler with a mocked model and asserts no marker survives into any visible field.Offline eval.
scripts/eval_citations.pyscores 15 cases indata/eval/citations_eval.jsonlwith no API key and no network, and runs in CI:Four of the fifteen cases plant fabrications — an out-of-range surah, an ayah past a surah's real bound, an unknown collection, and a block mixing one real citation with one invented one. They are excluded from the validity denominator and scored only by the rejection rate, so a correct parser is never punished for refusing them, and a parser that blindly accepts everything cannot pass.
No regressions. The scoped suite (
pytest -q tests test --ignore=venv) goes from 732 to 791 passing. The failures that remain were already failing ondevbefore this branch: threetest_adhkar.py404s (the #59 router is merged but never registered inmain.py), three TTL tests expecting attl=0item to be expired, and a timing-sensitive concurrency check. None are touched here.flake8is clean across all new and changed files. CI now lints and compile-checks the new module and script, runs the citation tests, and runs the offline eval.Known limitation
A semantic-cache hit replays stored prose with an empty citation list, because the cache stores text only. No markers leak, and re-asking with
X-Cache-Bypassreturns citations. Widening the cache record shape is out of scope for this change.Closes #15