fix(rag): keep sources when the LLM omits the citation tag - #847
Conversation
No [Sources: ...] tag means the model didn't report which sources it used, not that it used none. filter_sources_by_citations now keeps all retrieved sources in that case instead of hiding them, since answers were frequently coming back with no cited sources at all.
|
Warning Review limit reachedNext included review available in 23 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesCitation source handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The PR preserves sources when citation tags are omitted and adds citation metadata for consumers; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Adds extra.retrieved_sources, carrying every source retrieval produced regardless of citation filtering, so clients can see what was searched even when extra.sources ends up narrower than the full retrieval set.
Clearer than retrieved_sources for a field meant for debugging and RAG evaluation.
andyne13
left a comment
andyne13
left a comment
There was a problem hiding this comment.
Verified the branch locally at f4bd28a6: full unit suite passes (2431), ruff check, ruff format --check and check_layer_imports.py are all clean.
Two notes before the findings, since this PR partially reverts #807:
- It does not reopen #778. #807's conversational fix works by not retrieving at all — the
requires_retrieval=Falsebranch in_prepare_chatreturnspayload, [], [], True, sosourcesis already[]and the citation filter never runs. Greetings and identity questions still show no sources. - The
allow_uncitedremoval is correct dead-weight cleanup: structured output previously reached "keep everything" viaallow_uncited=Trueand now reaches it viacitations is None. Same result, and_allows_uncited_sources→_is_structured_outputis the honest name.
Confirming @hedhoud's point, with a wider scope
The post-truncation issue is real, and the context budget isn't the only thing narrowing the set before it reaches all_retrieved_sources:
docs = [docs[i] for i in included]—query_service.py:510(context budget), and again at:574in_prepare_completions, so thecomplete()path has the same gap.web_results = [web_results[number - web_start_index] for number in web_source_numbers]—:524, same for web results.docs = await self._map_reduce(...)—:494replaces the retrieved docs with generated summary docs whenuse_map_reduceis set, so on that pathall_retrieved_sourcescontains no retrieved document at all.
If the field is meant for debugging and evaluation, it needs to be captured right after retrieval, before all three of these.
Missing: nothing distinguishes "cited" from "unreported"
This is the one I'd most like to see addressed. After this change extra.sources conflates two very different states, and all_retrieved_sources doesn't disambiguate them either — when the tag is missing, sources == all_retrieved_sources, but that is also true when the model legitimately cited every source. Neither the Chainlit front nor the admin UI can tell the two apart, so the #778 failure mode becomes unobservable rather than fixed.
One extra key solves it:
extra_payload = {
"sources": filtered,
"all_retrieved_sources": sources,
"citations_reported": citations is not None,
}That lets a client render "Sources cited" vs "Sources consulted", and makes the tag-omission rate measurable instead of invisible.
The public API contract wasn't updated
CLAUDE.md documents the new field, but the OpenAPI descriptions that clients actually read still say extra contains only sources:
openrag/api/routers/user/chat.py:448(chat completions)openrag/api/routers/user/chat.py:554(completions)
Nothing under docs/content/ describes extra, so those two docstrings are the whole public contract for this field.
PR description is stale
The description says extra.retrieved_sources; commit f4bd28a6 renamed it to all_retrieved_sources. The auto-generated walkthrough inherited the old name too. Worth fixing before merge so the changelog is right. The summary bullet "Structured-output responses no longer process citation markers incorrectly" also isn't part of this diff — that gate already exists on develop.
Nits, non-blocking
- On-wire duplication.
filtered_jsonis serialized once and attached to both the tail chunk and the finish chunk, so a streamed answer now carries the source array four times instead of twice — and in exactly the case this PR targets (no tag), the two arrays are byte-identical. It's bounded, since_dict_to_chunkstripstextfrom metadata, so I would keep it if unconditional presence is what makes the field useful for eval. Just flagging the cost. - Truncated streams. Upstream dying before the tag now yields
citations is None→ every source marked cited, alongsidetruncated: true. Defensible, but the warning atsource_filtering.py:275logssources=len(filtered), which will read as "cited N sources" for answers that cited none. - Test coverage asymmetry.
chatgottest_chat_without_citation_keeps_retrieved_sources;complete()has no equivalent, and no test asserts thatcomplete()emitsall_retrieved_sourcesat all. Also worth noting theprepare_sources=lambda d, w: [...] if d or w else []stub change is load-bearing — it is more faithful than the old stub (real__prepare_sourcesatchat.py:214does return[]for empty docs), but it makes the surroundingassert sources == []assertions trivially true.
all_retrieved_sources was built from the same docs/web_results already truncated by format_context()/format_web_context() to fit the prompt's token budget, so it silently dropped anything retrieval returned but couldn't fit — defeating its purpose as the complete set for debugging and RAG evaluation. Snapshot the full retrieval set before that truncation and thread it through chat, chat_stream, and complete.
With no [Sources: ...] tag, `sources` falls back to keeping every retrieved source — identical, from the client's view, to the model explicitly citing all of them. Add `citations_reported` (true only when a tag, even an empty/none one, was found) so clients can tell "cited everything" apart from "didn't report citations at all".
|
Design note before this gets implemented — shape only, no implementation detail. Three sets nest here: retrieved ⊇ presented ⊇ cited. Retrieved measures recall, presented is what the model could possibly have used, cited is what it claims it used. They answer different questions, so each wants its own stage-accurate name, and no field should mean two different stages depending on circumstance. That's also the cleanest argument against the current fallback: One caution on the fix @hedhoud asked for, worth knowing up front: citation numbers are positions into the presented list. The citation-bearing field therefore can't simply be swapped for the fuller set. Web sources are numbered starting from the document count after truncation, so widening that list silently misaligns them. The retrieved set needs to arrive as its own field that nothing indexes into. Also worth flagging that the gap is large rather than marginal: the context budget is sized for ten documents while the retrieval path is uncapped and can return up to Last thought: a full retrieval dump on every response is debug telemetry on the hot path. Behind a request flag, or in traces, would serve evaluation better without growing every answer. |
Map-reduce reassigns `docs` to LLM-generated summaries before the prompt is built, and the retrieval snapshot for all_retrieved_sources was taken after that reassignment — so on the map-reduce path it held summaries instead of anything retrieval actually returned. Move the snapshot above the map-reduce call, alongside the existing pre-truncation capture. Follow-up to f3c5b2f, from PR #847 review.
The OpenAPI-facing description of the extra response field (the public contract clients actually read) still only mentioned sources. CLAUDE.md was updated for these fields but the router docstrings weren't.
…dump Addresses andyne13's design note on #847: `sources` conflated "what was cited" with "everything, because there was no tag" — no distinct field existed for what was actually shown to the LLM. Add `presented_sources` (everything shown, pre-citation-filter) and `cited_sources` (strictly what was cited, never falling back like `sources` does), so a client can render "cited" vs "consulted" and Chainlit can adopt `cited_sources` with a `presented_sources` fallback when it's empty. `sources` is left untouched for backward compatibility with existing clients (e.g. Twake). Also gate `all_retrieved_sources` behind a new `metadata.include_all_retrieved_sources` request flag (default off): dumping the full, uncapped retrieval set on every response is debug/eval telemetry that most callers don't need on the hot path. Added the `metadata` field to OpenAICompletionRequest, which was missing it entirely — completions couldn't reach any metadata flag before this, including the pre-existing spoken_style_answer.
Chainlit was still reading the legacy extra.sources field. Switch to cited_sources (strictly what the model cited) with a presented_sources fallback when nothing was cited, per the plan discussed on PR #847. Also add the complete()-path no-citation test flagged as a coverage gap in that review's nits.
|
Thanks for both passes — replying to the review and the design note together since they overlap. From the review:
From the design note — went with option 1's non-breaking version: Kept On the correctness caution: confirmed already safe — On the budget/cap mismatch: agreed it's the more fundamental issue, but leaving that out of scope here — happy to open a follow-up issue if you'd like to track it separately. On gating the full dump: done — |
|
Filed the context-budget/reranker.top_k mismatch as a separate issue: #851 — keeping it out of this PR's scope. |
Point future readers at #851 from the two spots where the gap actually lives: the token-budget sizing in query_service.py, and the pipeline code where reranker_top_k is read but never applied as a final cutoff.
hedhoud
left a comment
hedhoud
left a comment
There was a problem hiding this comment.
The follow-up changes address the earlier source-truncation and citation-state concerns across chat, streaming, completion, and map-reduce. The focused tests and CI are green, and the backward-compatible fallback remains intact.
3 participants
Why
Sources are not cited in a lot of messages: PR #807 changed the fallback so a missing
[Sources: ...]tag hides all sources instead of keeping them. In practice the model often skips the tag on answers that are genuinely grounded in retrieved documents, so those sources were being dropped from the response.Fix
filter_sources_by_citationsnow keeps all presented sources when no tag is found, instead of returning none.extra.citations_reported(bool) tells a client whether the model actually emitted a[Sources: ...]tag (even an empty one) —falseis the only case wheresourcesfalls back to keeping everything, so a client can distinguish "cited every source" from "didn't report citations at all".extra.presented_sources: everything actually shown to the LLM (after context-budget truncation), regardless of citation.extra.cited_sources: strictly what the model cited via the tag — unlikesources, never falls back to "everything".sourcesitself is left untouched for backward compatibility with existing clients (e.g. Twake); new consumers (e.g. Chainlit) should move tocited_sources, falling back topresented_sourcesin the UI when it's empty.extra.all_retrieved_sources: the complete retrieval set, captured before context-budget truncation (and, on the map-reduce path, before map-reduce replaces retrieved docs with LLM-generated summaries) — so it now genuinely reflects what retrieval returned inchat,chat_stream, andcomplete. Gated behindmetadata.include_all_retrieved_sources: true(default off) since it's debug/eval telemetry — retrieval is uncapped up toretriever.top_kwhile the context budget only fits a handful of documents, so this can be a large payload.Summary by CodeRabbit
New Features
Bug Fixes