feat: handle LLM [Sources: none] to filter out unused sources - #262
Merged
Conversation
paultranvan
force-pushed
the
fix/no-citations
branch
from
February 25, 2026 09:33
aea737b to
c7d27f5
Compare
📝 WalkthroughWalkthroughSource extraction now distinguishes missing source tags (None) from explicit "[Sources: none]" (empty set); filtering logic updated to handle None vs empty set. Tests expanded (including streaming/SSE cases). Prompt templates updated to require a final, mandatory sources line. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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: 1
🧹 Nitpick comments (1)
openrag/components/test_source_filtering.py (1)
85-102: Add one regression case for lowercase numbered tags.Consider adding a test for
"[sources: 1, 3]"so parser behavior stays stable if model casing varies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/test_source_filtering.py` around lines 85 - 102, Add a regression test that covers lowercase "sources" with numbered tags to lock parser behavior: create a new test method (e.g., test_sources_lowercase_numbered) alongside test_sources_none* that sets text = "Answer text\n[sources: 1, 3]" (and another variant without brackets if desired), calls extract_and_strip_sources_block(text), asserts clean == "Answer text" and asserts citations == {"1", "3"} (or expected numeric strings), so the parser handles lowercase "sources" and numeric citations consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openrag/components/utils.py`:
- Around line 123-124: The _SOURCES_NUMS_RE regex is currently case-sensitive so
tags like "[sources: 1, 3]" are missed; update its compilation to include
re.IGNORECASE (same as _SOURCES_NONE_RE) so it matches "sources" in any case,
i.e., change the _SOURCES_NUMS_RE declaration to use re.compile(...,
re.IGNORECASE) while keeping the existing capture group and pattern intact.
---
Nitpick comments:
In `@openrag/components/test_source_filtering.py`:
- Around line 85-102: Add a regression test that covers lowercase "sources" with
numbered tags to lock parser behavior: create a new test method (e.g.,
test_sources_lowercase_numbered) alongside test_sources_none* that sets text =
"Answer text\n[sources: 1, 3]" (and another variant without brackets if
desired), calls extract_and_strip_sources_block(text), asserts clean == "Answer
text" and asserts citations == {"1", "3"} (or expected numeric strings), so the
parser handles lowercase "sources" and numeric citations consistently.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
openrag/components/test_source_filtering.pyopenrag/components/utils.pyprompts/example1/spoken_style_answer_tmpl.txtprompts/example1/sys_prompt_tmpl.txt
Ahmath-Gadji
approved these changes
Mar 2, 2026
Ahmath-Gadji
left a comment
Ahmath-Gadji
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. I've testing and got not issue with it
Collaborator
|
This line generates a logging errors: openrag/openrag/components/utils.py Lines 150 to 152 in c7d27f5 --- Logging error in Loguru Handler #47 ---
Record was: {'elapsed': datetime.timedelta(seconds=1002, microseconds=961138), 'exception': None, 'extra': {'citations': {1, 2, 3, 5, 6}, 'matched': "'[Sources: 1, 2, 3, 5, 6]'"}, 'file': (name='utils.py', path='/app/openrag/components/utils.py'), 'function': 'extract_and_strip_sources_block', 'level': (name='DEBUG', no=10, icon='🐞'), 'line': 151, 'message': 'Extracted source citations from LLM response', 'module': 'utils', 'name': 'components.utils', 'process': (id=76, name='SpawnProcess-1'), 'thread': (id=139782434555712, name='MainThread'), 'time': datetime(2026, 3, 2, 14, 26, 52, 638202, tzinfo=datetime.timezone(datetime.timedelta(0), 'UTC'))}
Traceback (most recent call last):
File "/app/.venv/lib/python3.12/site-packages/loguru/_handler.py", line 161, in emit
formatted = precomputed_format.format_map(formatter_record)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
KeyError: '1, 2, 3, 5, 6'
--- End of logging error --- |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
openrag/components/utils.py (1)
123-124:⚠️ Potential issue | 🟡 MinorMake numeric source-tag parsing case-insensitive.
_SOURCES_NUMS_REis still case-sensitive, so lowercase tags like[sources: 1, 3]are treated as “tag missing” and incorrectly fall back to all sources.Proposed fix
-_SOURCES_NUMS_RE = re.compile(r"\n?\[?Sources?\]?\s*:\s*\[?([\d,\s]+)\]?\s*$") +_SOURCES_NUMS_RE = re.compile(r"\n?\[?Sources?\]?\s*:\s*\[?([\d,\s]+)\]?\s*$", re.IGNORECASE)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/utils.py` around lines 123 - 124, _SOURCES_NUMS_RE is currently case-sensitive, so tags like "[sources: 1, 3]" are missed; update the regex compilation for _SOURCES_NUMS_RE to use the re.IGNORECASE flag (same as _SOURCES_NONE_RE) so the pattern matches regardless of case, leaving the pattern string unchanged and only adding the flag to the re.compile call.
🧹 Nitpick comments (1)
openrag/components/utils.py (1)
151-151: Use Loguru.bind()for contextual citation fields.This log call should attach context via
.bind(...)instead of kwargs to align with repo structured-logging rules.Proposed fix
- logger.debug("Extracted source citations from LLM response", citations=sorted(citations), matched=repr(match.group(0))) + logger.bind(citations=sorted(citations), matched=repr(match.group(0))).debug( + "Extracted source citations from LLM response" + )As per coding guidelines, "Use Loguru with structured logging via
get_logger()fromutils.logger, and use.bind()for contextual fields like file_id and partition".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/utils.py` at line 151, The current call to logger.debug passes contextual fields as kwargs; change it to bind those fields on the logger first (e.g., logger.bind(citations=sorted(citations), matched=repr(match.group(0))).debug(...)) so structured logging follows the repo convention; update the statement that uses logger.debug in utils.extracted citation logic (the line referencing logger.debug and match.group) to use get_logger()/logger.bind(...) and then call .debug with the message only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openrag/components/utils.py`:
- Around line 123-124: The file failed the ruff formatter check; run the
formatter to fix whitespace/formatting issues (e.g., run `ruff format` or your
repo's formatting command) and re-commit the changes; specifically ensure the
regular-expression lines defining _SOURCES_NONE_RE and _SOURCES_NUMS_RE are
formatted according to ruff's rules so `ruff format --check` passes.
- Around line 150-151: The current parsing in the comprehension for "citations"
only splits on commas and misses space-separated numbers (e.g., "[Sources: 1
2]"); update the extraction to find all digit groups in match.group(1) (e.g.,
use re.findall(r'\d+', match.group(1))) or split on non-digit characters so
citations becomes a set of ints from those matches, then keep the logger.debug
call with sorted(citations) and matched=repr(match.group(0)) unchanged; refer to
the existing symbols "citations", "match", and the logger.debug line when making
the change.
---
Duplicate comments:
In `@openrag/components/utils.py`:
- Around line 123-124: _SOURCES_NUMS_RE is currently case-sensitive, so tags
like "[sources: 1, 3]" are missed; update the regex compilation for
_SOURCES_NUMS_RE to use the re.IGNORECASE flag (same as _SOURCES_NONE_RE) so the
pattern matches regardless of case, leaving the pattern string unchanged and
only adding the flag to the re.compile call.
---
Nitpick comments:
In `@openrag/components/utils.py`:
- Line 151: The current call to logger.debug passes contextual fields as kwargs;
change it to bind those fields on the logger first (e.g.,
logger.bind(citations=sorted(citations),
matched=repr(match.group(0))).debug(...)) so structured logging follows the repo
convention; update the statement that uses logger.debug in utils.extracted
citation logic (the line referencing logger.debug and match.group) to use
get_logger()/logger.bind(...) and then call .debug with the message only.
paultranvan
force-pushed
the
fix/no-citations
branch
from
March 2, 2026 15:45
fe1ccf0 to
8e075df
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openrag/components/utils.py (1)
123-124:⚠️ Potential issue | 🟠 MajorFix numeric citation extraction to handle all accepted separators.
Line 124 accepts whitespace-separated digits, but Line 150 only comma-splits. Cases like
[Sources: 1 2]are parsed as empty and incorrectly treated as explicit “none”.Proposed fix
- citations = {int(n.strip()) for n in match.group(1).split(",") if n.strip().isdigit()} + citations = {int(n) for n in re.findall(r"\d+", match.group(1))}Also applies to: 150-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/utils.py` around lines 123 - 124, The numeric citation parsing is too restrictive: _SOURCES_NUMS_RE should accept numbers separated by commas or whitespace and the subsequent split that currently uses comma-only splitting must be changed to split on both commas and whitespace; update the regex _SOURCES_NUMS_RE to capture digits separated by commas or spaces (e.g. use a pattern allowing [\d,\s]+) if not already, and replace any .split(',') usage that processes the captured group with re.split(r'[\s,]+', captured_group) (then strip and filter out empty strings) so inputs like "[Sources: 1 2]" and "[Sources: 1,2]" are both parsed correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openrag/components/utils.py`:
- Around line 123-124: The numeric citation parsing is too restrictive:
_SOURCES_NUMS_RE should accept numbers separated by commas or whitespace and the
subsequent split that currently uses comma-only splitting must be changed to
split on both commas and whitespace; update the regex _SOURCES_NUMS_RE to
capture digits separated by commas or spaces (e.g. use a pattern allowing
[\d,\s]+) if not already, and replace any .split(',') usage that processes the
captured group with re.split(r'[\s,]+', captured_group) (then strip and filter
out empty strings) so inputs like "[Sources: 1 2]" and "[Sources: 1,2]" are both
parsed correctly.
- Distinguish three citation states: specific sources cited, explicit "none", and missing tag (fallback to all sources) - Update prompts to always require a [Sources: ...] line - Add streaming tests for all three source filtering cases
paultranvan
force-pushed
the
fix/no-citations
branch
from
March 2, 2026 15:56
8e075df to
6f553fe
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openrag/components/test_source_filtering.py (1)
73-77: Consider adding a test for space-separated sources.Given the regex allows whitespace separators, consider adding a test case to verify space-separated citations are correctly parsed once the parsing fix is applied:
def test_sources_space_separated(self): text = "Answer text\n[Sources: 1 2 3]" clean, citations = extract_and_strip_sources_block(text) assert clean == "Answer text" assert citations == {1, 2, 3}This will help catch regressions in the space-separated parsing path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/test_source_filtering.py` around lines 73 - 77, Add a new unit test to cover space-separated citations: create a test function (e.g., test_sources_space_separated) that calls extract_and_strip_sources_block with text "Answer text\n[Sources: 1 2 3]" and asserts the returned clean string equals "Answer text" and the citations set equals {1, 2, 3}; place it alongside the existing tests (such as test_no_brackets_at_all) so it validates the regex/path that allows whitespace separators and prevents future regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openrag/components/test_source_filtering.py`:
- Around line 73-77: Add a new unit test to cover space-separated citations:
create a test function (e.g., test_sources_space_separated) that calls
extract_and_strip_sources_block with text "Answer text\n[Sources: 1 2 3]" and
asserts the returned clean string equals "Answer text" and the citations set
equals {1, 2, 3}; place it alongside the existing tests (such as
test_no_brackets_at_all) so it validates the regex/path that allows whitespace
separators and prevents future regressions.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
openrag/components/test_source_filtering.pyopenrag/components/utils.pyprompts/example1/spoken_style_answer_tmpl.txtprompts/example1/sys_prompt_tmpl.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- prompts/example1/spoken_style_answer_tmpl.txt
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.
Following #246
Summary by CodeRabbit
New Features
Bug Fixes
Tests