Hide Chainlit Sources when no evidence is available - #807
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe PR hardens source validation and rendering, protects authoritative document-link fields, adds conversational retrieval classification, and filters unattributed sources while allowing structured responses to retain uncited sources. Tests cover source safety, retrieval decisions, and citation behavior. Source handling
Retrieval and citation behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant QueryContextualizer
participant QueryService
participant Retriever
participant SourceFilter
User->>QueryContextualizer: submit conversational or factual request
QueryContextualizer->>QueryService: return requires_retrieval and query_list
QueryService->>Retriever: retrieve when required or forced
Retriever-->>QueryService: return answer context and sources
QueryService->>SourceFilter: filter sources by terminal citations
SourceFilter-->>User: return answer with attributed sources
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/unit/test_app_front_secret.py (2)
434-453: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a case with markdown-special characters in
title/snippet.Once escaping is added for
source_label/snippet(see the companion comment onopenrag/app_front.pylines 478-497), a regression test with characters like],(,)in the title would guard against link-spoofing regressions.🤖 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/unit/test_app_front_secret.py` around lines 434 - 453, Extend test_chainlit_keeps_valid_web_sources to use a web source title and snippet containing Markdown-special characters such as ], (, and ). Update the expected source name and rendered element content to verify _format_sources escapes these values while preserving the intended link URL and preventing link-spoofing.
456-480: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding a case where the chunk endpoint returns a non-dict JSON body.
This would cover the
AttributeErrorgap raised onopenrag/app_front.pylines 528-562 (data.get(...)failing whenresponse.json()isn't a dict), ensuring that path is also gracefully skipped rather than propagating.🤖 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/unit/test_app_front_secret.py` around lines 456 - 480, Extend test_chainlit_skips_unavailable_text_sources with a case where the mocked chunk endpoint returns valid JSON that is not a dictionary, such as a list or scalar. Assert that _format_sources gracefully skips the source and returns empty elements and source_names, covering the data.get handling in the source-fetching path.
🤖 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 `@openrag/app_front.py`:
- Around line 528-562: Update the source-processing try/except around
__fetch_page_content to catch AttributeError as well as the existing
httpx.HTTPError, TypeError, and ValueError exceptions, so malformed non-dict
JSON responses skip only the affected source via the existing warning and
continue behavior.
- Around line 478-497: Update the web-source rendering block around
source_label, content, and cl.Text to escape title-derived link-label text and
snippet text for Markdown before interpolating them. Preserve the URL as the
validated link target and keep the displayed source and snippet content
otherwise unchanged.
---
Nitpick comments:
In `@tests/unit/test_app_front_secret.py`:
- Around line 434-453: Extend test_chainlit_keeps_valid_web_sources to use a web
source title and snippet containing Markdown-special characters such as ], (,
and ). Update the expected source name and rendered element content to verify
_format_sources escapes these values while preserving the intended link URL and
preventing link-spoofing.
- Around line 456-480: Extend test_chainlit_skips_unavailable_text_sources with
a case where the mocked chunk endpoint returns valid JSON that is not a
dictionary, such as a list or scalar. Assert that _format_sources gracefully
skips the source and returns empty elements and source_names, covering the
data.get handling in the source-fetching path.
🪄 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: c993b759-9a76-466c-8cdb-11b7c3069ac7
📒 Files selected for processing (2)
openrag/app_front.pytests/unit/test_app_front_secret.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec3e036978
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review — verified against
|
andyne13
left a comment
andyne13
left a comment
There was a problem hiding this comment.
Follow-up on 43060c69 — the clickability fix is correct, the strip set is too wide
Re-verified the fix against the new head. The blocker is genuinely resolved: _safe_source_name() makes the name Markdown-inert at creation and it's used for both element.name and the rendered list, so Chainlit's matcher finds it again. I re-ran the same before/after harness — every case is clickable now, including the ordinary .pdf and .txt sources that were broken:
pdf, page=3 was link_ok=False -> link_ok=True
notes.txt was link_ok=False -> link_ok=True
pdf, page=None was dropped -> renders
Nice extras I didn't ask for: page=int(page) if page_label else None (valid — cl.Pdf.page is Optional[int] = None), httpx.InvalidURL in the except tuple, and duplicate document sources no longer collapsing.
One refinement worth doing in this PR, since it's the same constant.
The problem
_MARKDOWN_UNSAFE_SOURCE_NAME_CHARS = str.maketrans(dict.fromkeys("[]()*_`~#>|\\", " "))Twelve characters become spaces, but only some of them can actually alter rendering where a source name ends up. Chainlit substitutes a matched name into `[name](href)`, so the name lands inside a link label — which rules out every block-level construct. I checked each character against a CommonMark renderer in that exact position:
| Char | In a link label | Verdict |
|---|---|---|
[ ] |
closes the label early → breakout | keep stripping |
* |
foo*bar*baz → foo<em>bar</em>baz |
keep stripping |
` |
opens a code span | keep stripping |
\ |
a trailing one escapes the closing ] |
keep stripping |
> |
harmless in a label, but opens a blockquote on the degraded path where no element matched | keep, cheap insurance |
_ |
rapport_annuel_2026.pdf renders literally (intraword _ is never emphasis) |
over-aggressive |
( ) |
[report.pdf (page: 3)](/el/x) → valid link, label intact |
over-aggressive |
~ | # |
render literally | over-aggressive |
The cost lands on ordinary filenames — underscores are in most of them:
rapport_annuel_2026.pdf -> rapport annuel 2026.pdf
ISO_9001_procedure_v2.docx -> ISO 9001 procedure v2.docx
C++_style_guide.pdf -> C++ style guide.pdf
report.pdf (page: 3) -> report.pdf page: 3
There's also a knock-on: distinct files collapse onto the same base and get counter-suffixed, so a user sees a b.pdf and a b.pdf 2 for two unrelated documents (a_b.pdf and a b.pdf).
Patch
I applied and tested this locally against 43060c69 — 46 tests pass, ruff check and ruff format --check clean, and the before/after harness still shows link_ok=True everywhere.
--- a/openrag/app_front.py
+++ b/openrag/app_front.py
@@
-_MARKDOWN_UNSAFE_SOURCE_NAME_CHARS = str.maketrans(dict.fromkeys("[]()*_`~#>|\\", " "))
+# Only the characters that can actually alter rendering where a source name
+# ends up: Chainlit substitutes the name into `[name](url)`, so brackets and a
+# trailing backslash can break out of the link label, `*` and a backtick pair
+# still emphasise/codify inside it, and a leading `>` opens a blockquote on the
+# degraded path where no element matched. Everything else — `_ ( ) ~ # |` —
+# renders literally there, so stripping it only mangles ordinary filenames.
+_MARKDOWN_UNSAFE_SOURCE_NAME_CHARS = str.maketrans(dict.fromkeys("[]*`\\>", " "))Three existing tests pin the stripped output and need updating with it:
--- a/tests/unit/test_app_front_secret.py
+++ b/tests/unit/test_app_front_secret.py
@@ test_chainlit_keeps_page_less_pdf_sources
- assert source_names == ["report draft .pdf"]
+ assert source_names == ["report_ draft .pdf"]
@@ test_chainlit_escapes_untrusted_web_source_markdown
- assert source_names == ["Reference https://spoof.test trusted"]
+ assert source_names == ["Reference (https://spoof.test) trusted"]
@@ test_chainlit_keeps_source_names_unique_after_sanitizing
{"source_type": "web", "title": "Reference [draft]", "url": "https://example.test/one"},
- {"source_type": "web", "title": "Reference (draft)", "url": "https://example.test/two"},
+ {"source_type": "web", "title": "Reference *draft*", "url": "https://example.test/two"},That last one matters: under the narrower set those two titles no longer collide, so the test would stop exercising the de-duplication path — swapping the second to Reference *draft* keeps it meaningful.
And one new test, so the constant can't quietly drift again. It asserts the security invariant directly and needs no Markdown renderer:
@pytest.mark.parametrize(
"hostile_name",
[
"x](https://evil.test)",
"a]bc",
"](javascript:alert(1))",
"nested[a](b)c",
"trailing\\",
"**bold**",
"back`tick`",
">quote",
],
)
def test_source_name_cannot_break_out_of_the_markdown_link(monkeypatch, hostile_name):
"""Chainlit rewrites a matched name into ``[name](url)``.
Whatever the metadata contains, the sanitised name must not carry a
character that can close that label early or start an inline construct
inside it — otherwise a crafted filename or web-result title escapes into
the surrounding Markdown.
"""
module = _load_app_front(monkeypatch, auth_mode="token", module_name="app_front_source_name_safety_test")
safe_name = module._safe_source_name(hostile_name, {})
assert not set(safe_name) & set("[]*`\\>")
assert safe_name.strip() == safe_nameEvery hostile input above still yields exactly one anchor with the correct href under the narrowed set — the security properties are unchanged, only the mangling of legitimate names goes away.
Deliberately not chased
_draft_.pdf still renders as draft.pdf, because boundary underscores do emphasise inside a label. Neutralising only those needs a positional rule (_ is emphasis only when not between alphanumerics), and that immediately mis-handles C++_style_guide.pdf since + isn't alphanumeric. It's cosmetic rather than a breakout, so it isn't worth the complexity — a comment is the right amount of attention.
Minor, non-blocking
Six reachable paths still drop a source with no log line — :496 (entry not a dict), :508/:511 (bad web URL), :530 (no usable filename/file_url), :578 (no chunk_url), :581 (empty chunk content). Only the exception path at :584 logs. Since the whole point of this PR is to hide the block when there's no evidence, a support request of "it cited documents but showed nothing" currently can't distinguish nothing was cited from six sources were discarded. :581 is the one most likely to fire quietly, on image-only or whitespace chunks. One logger.debug per drop with the index and reason would close it.
(:519 isn't a drop — it's the success-path loop advance — and :555/:558 sit in the only_txt=True branch, which on_message never reaches.)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bae5a27f0f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d89102f57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37e4b46099
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@openrag/services/orchestrators/query_service.py`:
- Around line 92-93: Update the JSON example in the query contextualizer
instruction so requires_retrieval uses a neutral boolean placeholder instead of
being hard-coded to true. Preserve the existing query_list and temporal_filters
structure while ensuring the final hint does not force retrieval for greetings
or casual requests.
🪄 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: c127fa77-4844-4136-8cb3-a2f1b6dfec8a
📒 Files selected for processing (10)
openrag/app_front.pyopenrag/core/models/query.pyopenrag/core/utils/source_filtering.pyopenrag/prompts/templates/query_contextualizer_tmpl.txtopenrag/prompts/templates/spoken_style_answer_tmpl.txtopenrag/prompts/templates/sys_prompt_tmpl.txtopenrag/services/orchestrators/query_service.pytests/unit/core/utils/test_source_filtering.pytests/unit/services/orchestrators/test_query_service.pytests/unit/test_app_front_secret.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 726a38761b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4ee108183
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1616658861
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@hedhoud heads-up on a merge interaction — nothing wrong on your side, just so it isn't a surprise. I reviewed today's four commits. The The interaction is with #835 (DB-backed prompt library). That PR replaces the # your new no-retrieval path (copied from the existing line in _prepare_chat)
tmpl = self._spoken_style_answer_prompt if spoken_style else self._sys_prompt_tmpltWhichever of us merges second has to reconcile that. My suggestion: land this PR first and I'll absorb it in #835 — it's a three-line substitution in a file I'm already working in, and it means you don't have to learn an architecture you haven't touched just to ship a Chainlit fix. If #835 happens to land first instead, the replacement is: prompt_type = "spoken_style_answer" if spoken_style else "sys_prompt"
tmpl = await self._prompt_service.resolve_prompt(
prompt_type, names=[self._generation_prompt_name(prompt_type, partition)]
)which is what Worth flagging because git may not force the issue: the visible conflict is elsewhere in One substantive review point, separate from the above — The markdown-link case looks like a straight bug. The other two are a trade-off you may well accept — previously those markers stayed visible — but a quoted reference in a document now gets mangled and mis-attributed. Worth a guard, or a note that it's deliberate. |
andyne13
left a comment
andyne13
left a comment
There was a problem hiding this comment.
Approving — the Chainlit source handling is in good shape and the strip-set refinement from my earlier review landed exactly as suggested.
What I verified on this branch (merged with develop): full unit suite 2268 passed, layer guard OK, and the requires_retrieval design fails safe — SearchQueries.requires_retrieval defaults to True, so a model that omits the field still retrieves, and force_retrieval covers websearch/map-reduce. b141f46f also closes a real gap its own predecessor opened, where the no-retrieval path sent no system prompt at all. I also checked the streaming path with a citation marker split across SSE chunks ('[Source' + ' 1]') — stripped correctly.
Two things left as follow-ups rather than blockers:
-
Inline citation stripping still mangles text at this head —
include_inline_markersdefaults toTrue, so the default path is unchanged:'See [Sources 1](http://x) for details.' -> 'See(http://x) for details.' 'The contract says: "see [Sources 1, 2] of annex A"' -> 'The contract says: "see of annex A"' (+ records citations 1,2)The markdown-link case looks like a straight bug; the quoted-document case mangles text and mis-attributes sources. Worth a guard in a follow-up.
-
The #835 interaction described in my earlier comment —
self._spoken_style_answer_prompt/self._sys_prompt_tmpltare replaced there by per-request resolution. Nothing for you to do: land this first and I'll absorb it on my side.
The no-retrieval reply came from #807 and read an __init__-time snapshot this branch removes. Git auto-merged that reference without raising a conflict, so the resolution I wrote during the merge had nothing asserting it — the same shape of gap that let two stale attribute references through in the first place.
2 participants
Closes #778
Context
A source should appear only when it actually supports the answer. Conversational replies were showing unrelated documents and presenting OpenRAG as a generic assistant, which made the product behavior unclear.
Fix
Chainlit now ignores sources it cannot safely render, and RAG responses keep only sources the model actually cited. Conversational requests receive an OpenRAG-specific answer without document sources, while factual and explicit search requests keep normal retrieval. Web results are validated before citation numbering so valid evidence remains aligned.
Validation
The full unit suite passes (2,263 tests), along with repository lint, formatting, and package build checks. Live Chainlit checks confirmed that identity and capability questions describe OpenRAG as LINAGORA’s document-grounded RAG system without sources, while document-backed answers keep valid citations.