LCORE-515: Split rag chunks #849
Conversation
WalkthroughAdds multi-format RAG chunk parsing to TurnSummary: JSON-array/list parsing, formatted "Result N" block parsing, per-block metadata extraction (source/score), new regex patterns and logger, plus unit tests covering JSON, formatted, edge cases, and fallback behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 3
🧹 Nitpick comments (1)
src/utils/types.py (1)
184-194: Inconsistent content fallback for list items without"content"key.When a list item lacks a
"content"key, line 189 usesstr(chunk)as fallback, which produces a stringified dict like"{'source': 'x'}". This may not be the intended behavior.Consider either skipping chunks without content or explicitly handling this case:
if isinstance(data, list): for chunk in data: if isinstance(chunk, dict): + content = chunk.get("content") + if not content: + continue # Skip chunks without content self.rag_chunks.append( RAGChunk( - content=chunk.get("content", str(chunk)), + content=content, source=chunk.get("source"), score=chunk.get("score"), ) ) - return bool(data) + return bool(self.rag_chunks)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/utils/types.py(2 hunks)tests/unit/utils/test_types.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/utils/types.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests; do not use unittest framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/utils/test_types.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/utils/test_types.py
🧬 Code graph analysis (2)
src/utils/types.py (1)
src/models/responses.py (2)
ToolCall(33-38)RAGChunk(25-30)
tests/unit/utils/test_types.py (1)
src/utils/types.py (3)
GraniteToolParser(50-87)TurnSummary(106-260)_extract_rag_chunks_from_response(136-165)
🪛 GitHub Actions: Black
tests/unit/utils/test_types.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted (tests/unit/utils/test_types.py). Run 'black tests/unit/utils/test_types.py' to fix.
🪛 GitHub Actions: Python linter
tests/unit/utils/test_types.py
[error] 73-73: Pylint: W0212 - Access to a protected member _extract_rag_chunks_from_response of a client class (protected-access)
[error] 79-79: Pylint: W0212 - Access to a protected member _extract_rag_chunks_from_response of a client class (protected-access)
[error] 93-93: Pylint: W0212 - Access to a protected member _extract_rag_chunks_from_response of a client class (protected-access)
[error] 112-112: Pylint: W0212 - Access to a protected member _extract_rag_chunks_from_response of a client class (protected-access)
[error] 128-128: Pylint: W0212 - Access to a protected member _extract_rag_chunks_from_response of a client class (protected-access)
[error] 150-150: Pylint: W0212 - Access to a protected member _extract_rag_chunks_from_response of a client class (protected-access)
[error] 178-178: Pylint: W0212 - Access to a protected member _extract_rag_chunks_from_response of a client class (protected-access)
[error] 195-195: Pylint: W0212 - Access to a protected member _extract_rag_chunks_from_response of a client class (protected-access)
[error] 205-205: Pylint: W0212 - Access to a protected member _extract_rag_chunks_from_response of a client class (protected-access)
[error] 233-233: Pylint: W0212 - Access to a protected member _extract_rag_chunks_from_response of a client class (protected-access)
[error] 254-254: Pylint: W0212 - Access to a protected member _extract_rag_chunks_from_response of a client class (protected-access)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (5)
tests/unit/utils/test_types.py (2)
201-210: Verify fallback behavior aligns with implementation.The test correctly validates that unrecognized formats fall back to a single chunk with
source=DEFAULT_RAG_TOOLandscore=None. This matches the implementation's fallback logic in lines 158-165 ofsrc/utils/types.py.
212-246: Real-world test case provides valuable coverage.This test validates the actual response format from
knowledge_searchtool, including multiline content with markdown, IMPORTANT admonitions, and proper metadata extraction. Good coverage of production scenarios.src/utils/types.py (3)
229-260: Implementation of_parse_single_chunkis well-structured.Good use of
ast.literal_eval(safer thaneval) for metadata parsing, with proper exception handling and debug logging. The priority ofdocs_urloversourcematches the PR objectives for extracting the actual document URL.
136-165: Clear orchestration of parsing strategies with appropriate fallback.The method correctly tries JSON first (likely more structured), then formatted text, then falls back to single-chunk behavior. The early return on empty/whitespace is efficient.
21-29: Unable to fully verify due to repository access constraints; however, the review comment's assessment is technically sound and appropriately conditional.The regex pattern concern identified in the review is valid:
\{[^}]+\}will indeed fail on nested braces within metadata values. However, the review comment itself appropriately acknowledges that this pattern works for flat metadata structures and suggests the concern is only relevant if nested metadata objects occur in practice.Without direct codebase access, I cannot definitively verify whether:
- Metadata in practice contains nested objects
- Test cases demonstrate the flat structure assumption
- The
ast.literal_evalparsing of matched metadata would handle edge casesThe original review comment's conclusion stands on merit: The pattern works with the expected flat metadata structure, and the limitation is appropriately documented as a potential issue if that assumption changes.
| try: | ||
| # Parse the response to get chunks | ||
| # Try JSON first | ||
| try: | ||
| data = json.loads(response_content) | ||
| if isinstance(data, dict) and "chunks" in data: | ||
| for chunk in data["chunks"]: | ||
| data = json.loads(response_content) | ||
| if isinstance(data, dict) and "chunks" in data: | ||
| for chunk in data["chunks"]: | ||
| self.rag_chunks.append( | ||
| RAGChunk( | ||
| content=chunk.get("content", ""), | ||
| source=chunk.get("source"), | ||
| score=chunk.get("score"), | ||
| ) | ||
| ) | ||
| return True |
There was a problem hiding this comment.
Empty chunks array returns True, may trigger unexpected behavior.
If the JSON contains {"chunks": []} (empty array), the loop executes zero times but returns True at line 183. This prevents the fallback from creating a single chunk, resulting in no RAG chunks at all.
Consider checking if chunks were actually added:
if isinstance(data, dict) and "chunks" in data:
+ if not data["chunks"]:
+ return False
for chunk in data["chunks"]:
self.rag_chunks.append(
RAGChunk(
content=chunk.get("content", ""),
source=chunk.get("source"),
score=chunk.get("score"),
)
)
return True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| # Parse the response to get chunks | |
| # Try JSON first | |
| try: | |
| data = json.loads(response_content) | |
| if isinstance(data, dict) and "chunks" in data: | |
| for chunk in data["chunks"]: | |
| data = json.loads(response_content) | |
| if isinstance(data, dict) and "chunks" in data: | |
| for chunk in data["chunks"]: | |
| self.rag_chunks.append( | |
| RAGChunk( | |
| content=chunk.get("content", ""), | |
| source=chunk.get("source"), | |
| score=chunk.get("score"), | |
| ) | |
| ) | |
| return True | |
| try: | |
| data = json.loads(response_content) | |
| if isinstance(data, dict) and "chunks" in data: | |
| if not data["chunks"]: | |
| return False | |
| for chunk in data["chunks"]: | |
| self.rag_chunks.append( | |
| RAGChunk( | |
| content=chunk.get("content", ""), | |
| source=chunk.get("source"), | |
| score=chunk.get("score"), | |
| ) | |
| ) | |
| return True |
🤖 Prompt for AI Agents
In src/utils/types.py around lines 172 to 183, the code returns True whenever
"chunks" exists in the parsed JSON even if it's an empty list, causing no
RAGChunk to be created; modify the logic to only return True if at least one
chunk was actually appended to self.rag_chunks (e.g., check a counter or length
before returning), and if no chunks were added fall through to the existing
fallback that creates a single chunk; also guard against non-list "chunks"
values by treating them as absent so the fallback runs.
| for _result_num, content_block in matches: | ||
| chunk = self._parse_single_chunk(content_block) | ||
| if chunk: | ||
| self.rag_chunks.append(chunk) | ||
|
|
||
| return bool(self.rag_chunks) |
There was a problem hiding this comment.
Return value check may be incorrect after partial JSON parsing failure.
Line 227 checks bool(self.rag_chunks), which includes any chunks added by a failed JSON parsing attempt. If _try_parse_json_chunks appends chunks then throws an exception, it returns False, but self.rag_chunks is non-empty. Then _try_parse_formatted_chunks returns True even if it parsed nothing from formatted text.
Track chunks added within this method instead:
def _try_parse_formatted_chunks(self, response_content: str) -> bool:
# ... docstring ...
if "Result" not in response_content or "Content:" not in response_content:
return False
matches = RAG_RESULT_PATTERN.findall(response_content)
if not matches:
return False
+ chunks_added = 0
for _result_num, content_block in matches:
chunk = self._parse_single_chunk(content_block)
if chunk:
self.rag_chunks.append(chunk)
+ chunks_added += 1
- return bool(self.rag_chunks)
+ return chunks_added > 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _result_num, content_block in matches: | |
| chunk = self._parse_single_chunk(content_block) | |
| if chunk: | |
| self.rag_chunks.append(chunk) | |
| return bool(self.rag_chunks) | |
| chunks_added = 0 | |
| for _result_num, content_block in matches: | |
| chunk = self._parse_single_chunk(content_block) | |
| if chunk: | |
| self.rag_chunks.append(chunk) | |
| chunks_added += 1 | |
| return chunks_added > 0 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/utils/types.py (2)
175-205: Fix bugs where no chunks are added but method returnsTrue.There are two scenarios where this method returns
Truedespite adding no chunks:
Line 191: When
data["chunks"]is an empty list[], the loop executes zero times but still returnsTrue, preventing the fallback from creating a single chunk.Line 202: When
datais a non-empty list containing only non-dict elements (e.g.,[1, 2, 3]or["string"]), no chunks are added butbool(data)returnsTrue.Additionally, the return type annotation is missing.
Apply this diff to track chunks added and fix the return logic:
- def _try_parse_json_chunks(self, response_content: str) -> bool: + def _try_parse_json_chunks(self, response_content: str) -> bool: """Try to parse response as JSON chunks. Returns True if successfully parsed, False otherwise. """ + chunks_added = 0 try: data = json.loads(response_content) if isinstance(data, dict) and "chunks" in data: for chunk in data["chunks"]: self.rag_chunks.append( RAGChunk( content=chunk.get("content", ""), source=chunk.get("source"), score=chunk.get("score"), ) ) - return True + chunks_added += 1 + return chunks_added > 0 if isinstance(data, list): for chunk in data: if isinstance(chunk, dict): self.rag_chunks.append( RAGChunk( content=chunk.get("content", str(chunk)), source=chunk.get("source"), score=chunk.get("score"), ) ) - return bool(data) + chunks_added += 1 + return chunks_added > 0 except (json.JSONDecodeError, KeyError, AttributeError, TypeError, ValueError): pass return False
207-235: Track chunks added within this method to avoid incorrect return value.Line 235 returns
bool(self.rag_chunks), which includes any chunks added by previous parsing attempts. If_try_parse_json_chunkspartially adds chunks before failing and returningFalse, this method will returnTrueeven if it parsed nothing from the formatted text.Additionally, the return type annotation is missing.
Apply this diff to track only chunks added by this method:
- def _try_parse_formatted_chunks(self, response_content: str) -> bool: + def _try_parse_formatted_chunks(self, response_content: str) -> bool: """Try to parse formatted text response with 'Result N' blocks. Parses responses in format: knowledge_search tool found N chunks: BEGIN of knowledge_search tool results. Result 1 Content: <text> Metadata: {'chunk_id': '...', 'docs_url': '...', 'title': '...', ...} Result 2 ... END of knowledge_search tool results. Returns True if at least one chunk was parsed, False otherwise. """ # Check if this looks like a formatted RAG response if "Result" not in response_content or "Content:" not in response_content: return False matches = RAG_RESULT_PATTERN.findall(response_content) if not matches: return False + chunks_added = 0 for _result_num, content_block in matches: chunk = self._parse_single_chunk(content_block) if chunk: self.rag_chunks.append(chunk) + chunks_added += 1 - return bool(self.rag_chunks) + return chunks_added > 0
🧹 Nitpick comments (2)
src/utils/types.py (1)
136-136: Add return type annotation.The method is missing an explicit return type annotation. Per coding guidelines, all functions must include complete type annotations.
Apply this diff:
- def _extract_rag_chunks_from_response(self, response_content: str) -> None: + def _extract_rag_chunks_from_response(self, response_content: str) -> None:Wait, looking more carefully at line 136, it actually has no return type. Apply this diff:
- def _extract_rag_chunks_from_response(self, response_content: str) -> None: + def _extract_rag_chunks_from_response(self, response_content: str) -> None:Actually, I see the issue - the current code doesn't have
-> None. Let me correct:- def _extract_rag_chunks_from_response(self, response_content: str): + def _extract_rag_chunks_from_response(self, response_content: str) -> None:tests/unit/utils/test_types.py (1)
261-272: Add edge case tests for empty and invalid chunk data.The test suite is comprehensive but missing edge cases that would catch the bugs identified in
_try_parse_json_chunks:
- Empty chunks array:
{"chunks": []}- List with non-dict elements:
[1, 2, 3]or["string"]- Mixed list with dict and non-dict:
[{"content": "valid"}, "invalid", {"content": "valid2"}]These cases should fall back to single-chunk extraction, ensuring the bugs at lines 191 and 202 in
src/utils/types.pyare properly handled.Add these test methods after line 272:
def test_json_empty_chunks_array(self, caplog: pytest.LogCaptureFixture) -> None: """Test that empty chunks array falls back to single chunk.""" summary = self._create_turn_summary() response = json.dumps({"chunks": []}) with caplog.at_level(logging.WARNING): summary._extract_rag_chunks_from_response(response) # Should fall back and create single chunk assert len(summary.rag_chunks) == 1 assert summary.rag_chunks[0].source == DEFAULT_RAG_TOOL assert "Unable to parse individual RAG chunks" in caplog.records[0].message def test_json_list_with_non_dict_elements(self, caplog: pytest.LogCaptureFixture) -> None: """Test that list with only non-dict elements falls back to single chunk.""" summary = self._create_turn_summary() response = json.dumps([1, 2, 3, "string"]) with caplog.at_level(logging.WARNING): summary._extract_rag_chunks_from_response(response) # Should fall back and create single chunk assert len(summary.rag_chunks) == 1 assert summary.rag_chunks[0].source == DEFAULT_RAG_TOOL def test_json_list_mixed_elements(self) -> None: """Test that list with mixed dict and non-dict elements extracts only dicts.""" summary = self._create_turn_summary() response = json.dumps([ {"content": "First valid"}, "invalid string", {"content": "Second valid"}, 123, ]) summary._extract_rag_chunks_from_response(response) # Should extract only the dict elements assert len(summary.rag_chunks) == 2 assert summary.rag_chunks[0].content == "First valid" assert summary.rag_chunks[1].content == "Second valid"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/utils/types.py(2 hunks)tests/unit/utils/test_types.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests; do not use unittest framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/utils/test_types.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/utils/test_types.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/utils/types.py
🧬 Code graph analysis (2)
tests/unit/utils/test_types.py (1)
src/utils/types.py (3)
GraniteToolParser(50-87)TurnSummary(106-268)_extract_rag_chunks_from_response(136-173)
src/utils/types.py (1)
src/models/responses.py (2)
ToolCall(33-38)RAGChunk(25-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
- GitHub Check: build-pr
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/utils/types.py (2)
188-219: Clarify behavior when JSON contains no usable chunks; consider basing success on chunks added.As written,
_try_parse_json_chunksreturnsTruefor any dict with a"chunks"key (even if it’s an empty list or non-list) and for any non-empty JSON list, even if noRAGChunkis actually appended (e.g., list of non-dict items). That means the fallback single‑chunk path is skipped whilerag_chunksremains unchanged for this response. If you want “success” to mean “at least one chunk was parsed and appended”, you can gate the return values on a local counter instead ofbool(data).def _try_parse_json_chunks(self, response_content: str) -> bool: """Try to parse response as JSON chunks. Returns True if successfully parsed, False otherwise. """ try: - data = json.loads(response_content) - if isinstance(data, dict) and "chunks" in data: - for chunk in data["chunks"]: - self.rag_chunks.append( - RAGChunk( - content=chunk.get("content", ""), - source=chunk.get("source"), - score=chunk.get("score"), - ) - ) - return True - if isinstance(data, list): - for chunk in data: - if isinstance(chunk, dict): - self.rag_chunks.append( - RAGChunk( - content=chunk.get("content", str(chunk)), - source=chunk.get("source"), - score=chunk.get("score"), - ) - ) - return bool(data) + data = json.loads(response_content) + if isinstance(data, dict) and "chunks" in data: + chunks = data["chunks"] + # Treat missing or empty/non-list "chunks" as a parse miss so fallback can run. + if not isinstance(chunks, list) or not chunks: + return False + for chunk in chunks: + if not isinstance(chunk, dict): + continue + self.rag_chunks.append( + RAGChunk( + content=chunk.get("content", ""), + source=chunk.get("source"), + score=chunk.get("score"), + ) + ) + return True + if isinstance(data, list): + chunks_added = 0 + for chunk in data: + if isinstance(chunk, dict): + self.rag_chunks.append( + RAGChunk( + content=chunk.get("content", str(chunk)), + source=chunk.get("source"), + score=chunk.get("score"), + ) + ) + chunks_added += 1 + return chunks_added > 0 except (json.JSONDecodeError, KeyError, AttributeError, TypeError, ValueError): pass return FalseIf the intended semantics are that an empty
"chunks": []should result in no RAG chunks (and no fallback text chunk), you may instead keep thereturn Truefor that case and only adjust the list-branchreturnto be based onchunks_added. Please confirm desired behavior given howrag_chunksis consumed downstream.
220-249: Return value of_try_parse_formatted_chunksshould reflect chunks parsed in this call, not globalrag_chunks.
return bool(self.rag_chunks)can incorrectly report success when this method adds no new chunks butrag_chunksalready contains items from a previous JSON parse or an earlier tool call. That will skip the single‑chunk fallback and can hide parse failures for later responses.matches = RAG_RESULT_PATTERN.findall(response_content) if not matches: return False - for _result_num, content_block in matches: - chunk = self._parse_single_chunk(content_block) - if chunk: - self.rag_chunks.append(chunk) - - return bool(self.rag_chunks) + chunks_added = 0 + for _result_num, content_block in matches: + chunk = self._parse_single_chunk(content_block) + if chunk: + self.rag_chunks.append(chunk) + chunks_added += 1 + + # Only report success if this call actually appended at least one chunk. + return chunks_added > 0
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/types.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/utils/types.py
🧬 Code graph analysis (1)
src/utils/types.py (1)
src/models/responses.py (2)
ToolCall(33-38)RAGChunk(25-30)
🪛 GitHub Actions: Python linter
src/utils/types.py
[error] 27-27: C0301: Line too long (106/100) (line-too-long). Pylint reported while running: uv run pylint src tests. Exit code 16.
[error] 28-28: C0301: Line too long (111/100) (line-too-long). Pylint reported while running: uv run pylint src tests. Exit code 16.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (2)
src/utils/types.py (2)
149-186: RAG extraction flow and fallback logging look good.The multi-stage parsing (JSON → formatted → single-chunk fallback) with early returns and a bounded warning log message is clear and aligns with the stated behavior of the PR.
250-281: Single-chunk parsing and metadata handling look robust.The use of
ast.literal_evalwith a guarded regex and debug-only logging on failures, plus falling back to content-only chunks when metadata is missing or invalid, is a good balance between resilience and preserving useful data.
| # RAG Response Format Patterns | ||
| # ============================ | ||
| # These patterns match the format produced by llama-stack's knowledge_search tool. | ||
| # Source: llama_stack/providers/inline/tool_runtime/rag/memory.py | ||
| # | ||
| # The format consists of: | ||
| # - Header (hardcoded): "knowledge_search tool found N chunks:\nBEGIN of knowledge_search tool results.\n" | ||
| # - Chunks (configurable template, default): "Result {index}\nContent: {chunk.content}\nMetadata: {metadata}\n" | ||
| # - Footer (hardcoded): "END of knowledge_search tool results.\n" | ||
| # | ||
| # Note: The chunk template is configurable via RAGQueryConfig.chunk_template in llama-stack. | ||
| # If customized, these patterns may not match. A warning is logged when fallback occurs. | ||
|
|
||
| # Pattern to match individual RAG result blocks: " Result N\nContent: ..." | ||
| # Captures result number and everything until the next result or end marker | ||
| RAG_RESULT_PATTERN = re.compile( | ||
| r"\s*Result\s+(\d+)\s*\nContent:\s*(.*?)(?=\s*Result\s+\d+\s*\n|END of knowledge_search)", | ||
| re.DOTALL, | ||
| ) |
There was a problem hiding this comment.
Fix pylint line-too-long in RAG format documentation comments.
Pylint is currently failing on Lines 27–28 due to overly long comment lines. You can keep the documentation but wrap it over multiple shorter lines.
-# The format consists of:
-# - Header (hardcoded): "knowledge_search tool found N chunks:\nBEGIN of knowledge_search tool results.\n"
-# - Chunks (configurable template, default): "Result {index}\nContent: {chunk.content}\nMetadata: {metadata}\n"
-# - Footer (hardcoded): "END of knowledge_search tool results.\n"
+# The format consists of:
+# - Header (hardcoded):
+# "knowledge_search tool found N chunks:\n"
+# "BEGIN of knowledge_search tool results.\n"
+# - Chunks (configurable template, default):
+# "Result {index}\n"
+# "Content: {chunk.content}\n"
+# "Metadata: {metadata}\n"
+# - Footer (hardcoded):
+# "END of knowledge_search tool results.\n"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # RAG Response Format Patterns | |
| # ============================ | |
| # These patterns match the format produced by llama-stack's knowledge_search tool. | |
| # Source: llama_stack/providers/inline/tool_runtime/rag/memory.py | |
| # | |
| # The format consists of: | |
| # - Header (hardcoded): "knowledge_search tool found N chunks:\nBEGIN of knowledge_search tool results.\n" | |
| # - Chunks (configurable template, default): "Result {index}\nContent: {chunk.content}\nMetadata: {metadata}\n" | |
| # - Footer (hardcoded): "END of knowledge_search tool results.\n" | |
| # | |
| # Note: The chunk template is configurable via RAGQueryConfig.chunk_template in llama-stack. | |
| # If customized, these patterns may not match. A warning is logged when fallback occurs. | |
| # Pattern to match individual RAG result blocks: " Result N\nContent: ..." | |
| # Captures result number and everything until the next result or end marker | |
| RAG_RESULT_PATTERN = re.compile( | |
| r"\s*Result\s+(\d+)\s*\nContent:\s*(.*?)(?=\s*Result\s+\d+\s*\n|END of knowledge_search)", | |
| re.DOTALL, | |
| ) | |
| # RAG Response Format Patterns | |
| # ============================ | |
| # These patterns match the format produced by llama-stack's knowledge_search tool. | |
| # Source: llama_stack/providers/inline/tool_runtime/rag/memory.py | |
| # | |
| # The format consists of: | |
| # - Header (hardcoded): | |
| # "knowledge_search tool found N chunks:\n" | |
| # "BEGIN of knowledge_search tool results.\n" | |
| # - Chunks (configurable template, default): | |
| # "Result {index}\n" | |
| # "Content: {chunk.content}\n" | |
| # "Metadata: {metadata}\n" | |
| # - Footer (hardcoded): | |
| # "END of knowledge_search tool results.\n" | |
| # | |
| # Note: The chunk template is configurable via RAGQueryConfig.chunk_template in llama-stack. | |
| # If customized, these patterns may not match. A warning is logged when fallback occurs. | |
| # Pattern to match individual RAG result blocks: " Result N\nContent: ..." | |
| # Captures result number and everything until the next result or end marker | |
| RAG_RESULT_PATTERN = re.compile( | |
| r"\s*Result\s+(\d+)\s*\nContent:\s*(.*?)(?=\s*Result\s+\d+\s*\n|END of knowledge_search)", | |
| re.DOTALL, | |
| ) |
🧰 Tools
🪛 GitHub Actions: Python linter
[error] 27-27: C0301: Line too long (106/100) (line-too-long). Pylint reported while running: uv run pylint src tests. Exit code 16.
[error] 28-28: C0301: Line too long (111/100) (line-too-long). Pylint reported while running: uv run pylint src tests. Exit code 16.
🤖 Prompt for AI Agents
In src/utils/types.py around lines 21 to 39, several documentation comment lines
(notably the RAG format description on lines ~27–28) exceed pylint's
line-too-long limit; break those long comment lines into multiple shorter
comment lines (each starting with '#') so no line exceeds the project's max
length (e.g., 80 chars), preserving the original wording and punctuation but
wrapping sentences across lines to satisfy pylint.
1 participant
Description
Fix RAG chunk extraction to parse individual chunks
Problem
The
_extract_rag_chunks_from_responsefunction merged all RAG chunks into a single blob instead of parsing them individually. This caused:sourcealways set to"knowledge_search"instead of actual document URLSolution
Parse "Result N" blocks from formatted
knowledge_searchresponses, extracting:docs_url)Falls back to previous single-chunk behavior for unknown formats.
Before
After
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.