fix: Improve error logging for external resource access failures - #186
Conversation
📝 WalkthroughWalkthroughAdds a utility to detect external-resource fetch errors (status codes and URLs) and updates the image loader to treat such errors as external: it logs a contextual warning (http_status, url, error) and returns an XML-wrapped empty/partial description instead of raising; non-external errors retain prior handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Loader as Image Loader
participant VLM as VLM Model
participant Ext as External Resource
participant Util as external_resource_errors
participant Logger as Logger
Loader->>VLM: request image description (image URL)
VLM->>Ext: fetch image URL
Ext-->>VLM: network/HTTP error (e.g., 403 / timeout)
VLM-->>Loader: raises/returns Exception (possibly wrapped)
Loader->>Util: is_external_resource_error(Exception)
Util-->>Loader: (is_external=true, status="403", url="https://...")
alt External resource error (is_external=true)
Loader->>Logger: warn {http_status: "403", url: "...", error: Exception}
note right of Loader: return XML-wrapped empty/partial description (no raise)
else Non-external error (is_external=false)
Loader->>Logger: exception (full traceback)
note right of Loader: preserve previous error handling/propagation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (3)
openrag/components/indexer/loaders/base.py (1)
52-57: Potential false positive when status code appears in URLs.The current logic checks if the status code string (e.g.,
"400") exists anywhere in the error message. This could match URLs containing the number, likehttp://example.com/image400.pngorhttp://cdn.example.com:8400/image.png.Consider checking for more specific patterns that indicate actual HTTP status codes:
🔎 Proposed fix to reduce false positives
- # Look for HTTP status codes in the error message - detected_code = "" - for code, description in EXTERNAL_RESOURCE_ERROR_CODES.items(): - if code in error_str or description.lower() in error_str.lower(): - detected_code = code - break + # Look for HTTP status codes in the error message + # Use patterns that indicate actual HTTP status codes, not just the number + detected_code = "" + for code, description in EXTERNAL_RESOURCE_ERROR_CODES.items(): + # Match patterns like "403", "403 Forbidden", "status 403", "status=403", "status: 403" + code_pattern = rf"(?:status[=:\s]*)?\b{code}\b(?:\s+{re.escape(description)})?" + if re.search(code_pattern, error_str, re.IGNORECASE) or description.lower() in error_str.lower(): + detected_code = code + breakopenrag/components/indexer/chunker/chunker.py (1)
201-213: LGTM! Pre-chunking sanitization improves text quality.Sanitizing combined text before chunking is an effective way to remove control characters and excessive whitespace, which saves tokens and improves chunk quality. The inline comment clearly explains the rationale.
The explicit parameter specification provides good clarity about which sanitization operations are applied.
💡 Optional: Consider making sanitization parameters configurable
If different chunking scenarios require different sanitization settings, consider making these parameters configurable through the chunker config rather than hardcoding them:
# In config sanitization: normalize_whitespace: true remove_control_chars: true remove_zero_width_chars: true max_consecutive_newlines: 2 normalize_unicode: true # In code sanitize_config = config.get('sanitization', {}) sanitized_texts = sanitize_text(combined_texts, **sanitize_config)This would provide flexibility without sacrificing the current defaults.
openrag/components/text_sanitizer.py (1)
13-96: LGTM! Comprehensive text sanitization with configurable options.The
sanitize_textfunction provides thorough text cleaning with well-chosen defaults and flexibility through boolean parameters. The implementation correctly handles:
- Unicode normalization to NFC form
- Zero-width character removal (U+200B-U+200D, U+2060, U+FEFF)
- Control character removal while preserving newlines, tabs, and carriage returns
- Whitespace normalization including spaces, tabs, and line-leading/trailing spaces
- Line break normalization and consecutive newline limiting
💡 Optional: Use f-string for regex pattern construction
Line 89 uses string concatenation to build a regex pattern. Consider using an f-string for better readability:
- pattern = r'\n{' + str(max_consecutive_newlines + 1) + r',}' + pattern = rf'\n{{{max_consecutive_newlines + 1},}}'This makes the pattern construction more explicit and slightly more readable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
openrag/components/indexer/chunker/chunker.pyopenrag/components/indexer/chunker/test_chunking.pyopenrag/components/indexer/chunker/utils.pyopenrag/components/indexer/loaders/base.pyopenrag/components/test_text_sanitizer.pyopenrag/components/text_sanitizer.pyopenrag/routers/tools.py
🧰 Additional context used
🧬 Code graph analysis (5)
openrag/components/indexer/chunker/utils.py (1)
openrag/components/text_sanitizer.py (1)
clean_markdown_table_spacing(99-128)
openrag/components/test_text_sanitizer.py (1)
openrag/components/text_sanitizer.py (3)
sanitize_text(13-96)clean_markdown_table_spacing(99-128)sanitize_extracted_text(131-150)
openrag/components/indexer/chunker/chunker.py (1)
openrag/components/text_sanitizer.py (1)
sanitize_text(13-96)
openrag/components/indexer/chunker/test_chunking.py (1)
openrag/components/text_sanitizer.py (1)
clean_markdown_table_spacing(99-128)
openrag/routers/tools.py (1)
openrag/components/text_sanitizer.py (1)
sanitize_extracted_text(131-150)
⏰ 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). (1)
- GitHub Check: index-backup-restore
🔇 Additional comments (13)
openrag/components/indexer/loaders/base.py (2)
18-29: LGTM! Consider whether gateway errors should be included.The mapping covers common 4xx client errors well. Depending on how the VLM backend proxies external requests, gateway errors (502, 503, 504) might also indicate external resource issues. However, excluding them avoids false positives with genuine internal errors, so the current approach is reasonable.
208-226: Good implementation of differentiated error handling.The error handling correctly:
- Detects and logs external resource errors as warnings with useful context (HTTP status, URL)
- Preserves full exception logging for genuine internal errors
- Falls back to
image_datafor the URL when not extracted from the error message- Allows indexing to continue by returning a valid (empty) image description
This aligns well with the PR objective to improve observability for external fetch failures.
openrag/components/indexer/chunker/test_chunking.py (1)
9-9: LGTM! Import refactoring aligns with centralization.The import path update correctly reflects the relocation of
clean_markdown_table_spacingto the centralizedtext_sanitizermodule. This improves code organization and reusability across the codebase.openrag/routers/tools.py (2)
9-9: LGTM! Sanitization integration improves data quality.Adding text sanitization for extracted content is a sound practice that removes control characters and excessive whitespace before returning results to users.
111-116: LGTM! Sanitization properly applied before returning content.The sanitization step correctly processes extracted text before returning it to the user, improving the quality of the extractText tool output. The inline comment clearly documents the purpose.
openrag/components/indexer/chunker/utils.py (1)
4-5: LGTM! Refactoring delegates to centralized implementation.Removing the local
clean_markdown_table_spacingimplementation and importing it from the centralizedtext_sanitizermodule eliminates duplication and improves maintainability.openrag/components/indexer/chunker/chunker.py (1)
4-4: LGTM! Text sanitization module integrated.The import of
sanitize_textenables pre-processing text before chunking, which improves token efficiency and chunk quality.openrag/components/text_sanitizer.py (3)
1-11: LGTM! Well-documented module for text sanitization.The module documentation clearly explains the purpose and scope of the text sanitization utilities. Good use of docstrings to describe the module's functionality.
99-128: LGTM! Markdown table spacing normalization is correct.The function correctly normalizes spacing in markdown tables by:
- Trimming each cell while preserving the table structure
- Handling non-table lines gracefully
- Preserving leading and trailing pipes
- Rebuilding rows with consistent single-space padding
The slicing
cleaned_cells[1:-1]correctly excludes the outer empty strings from splitting on pipes (e.g.,"|A|B|"splits to['', 'A', 'B', '']), and handles edge cases where the slice is empty by producing an empty join result.
131-150: LGTM! Convenience wrapper with clear documentation.The
sanitize_extracted_textfunction provides a well-documented convenience wrapper that applies sensible defaults for text extraction scenarios. The documentation clearly lists which sanitization operations are applied.openrag/components/test_text_sanitizer.py (3)
1-117: LGTM! Comprehensive test coverage for text sanitization.The
TestSanitizeTextclass provides excellent test coverage including:
- Basic functionality (whitespace, tabs, newlines)
- Control and zero-width character removal
- Unicode normalization
- Edge cases (empty strings, complex mixed issues)
- Configuration toggles (enable/disable individual features)
- Boundary conditions (unlimited or zero max consecutive newlines)
The test names are clear and descriptive, making it easy to understand what each test verifies.
119-157: LGTM! Thorough testing of markdown table spacing normalization.The tests cover key scenarios for table spacing normalization:
- Excessive spaces within cells
- Inconsistent spacing across rows
- Empty cells
- Multiline tables with varying whitespace
All test cases verify that the function produces correctly formatted markdown tables with normalized spacing.
159-208: LGTM! Good coverage of the extraction wrapper function.The tests verify that
sanitize_extracted_textcorrectly applies all default sanitization operations and handles realistic extraction scenarios (e.g., PDF text with artifacts). The tests check for the absence of problematic characters and excessive whitespace while preserving content structure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_external_resource_error.py (2)
125-133: Replace unused tuple values with underscores.Multiple tests unpack the 3-tuple return value but don't use all variables. Replace unused variables with
_to follow Python conventions and satisfy the linter.🔎 Example fixes for unused variables
For tests that only check
is_external:- is_external, status_code, url = _is_external_resource_error(error) + is_external, _, _ = _is_external_resource_error(error)For tests that only check
url:- is_external, status_code, url = _is_external_resource_error(error) + _, _, url = _is_external_resource_error(error)For tests that check
is_externalandurlbut notstatus_code:- is_external, status_code, url = _is_external_resource_error(error) + is_external, _, url = _is_external_resource_error(error)For tests that check
is_externalandstatus_codebut noturl:- is_external, status_code, url = _is_external_resource_error(error) + is_external, status_code, _ = _is_external_resource_error(error)As per static analysis hints from Ruff.
Also applies to: 135-143, 145-150, 152-157, 177-182, 184-189, 204-210, 212-218, 226-231, 233-241, 243-251
220-224: Consider adding detection tests for remaining HTTP codes.While this test validates that all expected codes exist in the mapping, consider adding specific detection tests for codes 400, 405, 408, 410, and 451 to ensure the detection logic works correctly for these cases. Currently, only 401, 403, 404, and 429 have dedicated detection tests.
Example test for 400:
def test_detects_400_bad_request_error(self): """Test detection of 400 Bad Request errors.""" error = Exception( "ClientResponseError: 400 Bad Request - https://example.com/image.png" ) is_external, status_code, url = _is_external_resource_error(error) assert is_external is True assert status_code == "400" assert "example.com" in url
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_external_resource_error.py
🧰 Additional context used
🪛 Ruff (0.14.10)
tests/test_external_resource_error.py
130-130: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
140-140: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
148-148: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
148-148: Unpacked variable url is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
155-155: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
155-155: Unpacked variable url is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
180-180: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
180-180: Unpacked variable url is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
187-187: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
187-187: Unpacked variable url is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
207-207: Unpacked variable url is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
215-215: Unpacked variable url is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
229-229: Unpacked variable is_external is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
229-229: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
238-238: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
248-248: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
284-284: Unpacked variable is_external is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
284-284: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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). (1)
- GitHub Check: index-backup-restore
🔇 Additional comments (2)
tests/test_external_resource_error.py (2)
1-27: LGTM! Clean test module structure.The local redefinition approach effectively avoids loading the full indexer stack during testing, and the coverage of HTTP status codes for external resource errors is comprehensive.
30-63: LGTM! Helper function logic is sound.The detection logic appropriately combines HTTP status code matching, error indicator detection, and URL extraction. The regex pattern should handle typical error message formats encountered during image fetching operations.
e960232 to
d363610
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
openrag/utils/external_resource_errors.py (1)
15-25: Consider including common 5xx gateway errors.The current dictionary focuses on 4xx client errors, which aligns well with the PR's objective. However, consider including common gateway/server errors like
502 Bad Gateway,503 Service Unavailable, and504 Gateway Timeoutsince these also indicate external resource issues when VLM models fetch images from remote servers.🔎 Optional enhancement to include gateway errors
EXTERNAL_RESOURCE_ERROR_CODES = { "400": "Bad Request", "401": "Unauthorized", "403": "Forbidden", "404": "Not Found", "405": "Method Not Allowed", "408": "Request Timeout", "410": "Gone", "429": "Too Many Requests", "451": "Unavailable For Legal Reasons", + "502": "Bad Gateway", + "503": "Service Unavailable", + "504": "Gateway Timeout", }openrag/utils/test_external_resource_errors.py (7)
76-85: Add assertion for status_code.The test unpacks
status_codebut doesn't assert its value. For SSL errors without an HTTP status code, add an assertion to validate complete behavior.🔎 Proposed fix
is_external, status_code, url = is_external_resource_error(error) assert is_external is True + assert status_code == "" # No HTTP status code for SSL errors assert "insecure.example.com" in url
86-95: Add assertion for status_code.The test unpacks
status_codebut doesn't assert its value. For connection errors without an HTTP status code, add an assertion to validate complete behavior.🔎 Proposed fix
is_external, status_code, url = is_external_resource_error(error) assert is_external is True + assert status_code == "" # No HTTP status code for connection errors assert "unreachable.example.com" in url
96-102: Add assertions or use dummy variables for unused return values.Both tests unpack
status_codeandurlbut don't assert their values. Either add assertions to validate complete behavior or use dummy variables (_) to indicate intentionally ignored values.🔎 Option 1: Add assertions
For
test_detects_client_response_error_indicator:is_external, status_code, url = is_external_resource_error(error) assert is_external is True + assert status_code == "" + assert url == ""For
test_detects_http_error_indicator:is_external, status_code, url = is_external_resource_error(error) assert is_external is True + assert status_code == "500" + assert url == ""🔎 Option 2: Use dummy variables
- is_external, status_code, url = is_external_resource_error(error) + is_external, _, _ = is_external_resource_error(error) assert is_external is TrueAlso applies to: 103-109
128-134: Add assertions or use dummy variables for unused return values.Both tests unpack
status_codeandurlbut don't assert their values. For internal errors that should not be flagged as external, add assertions to validate all return values are as expected.🔎 Proposed fix
For
test_does_not_flag_type_errors:is_external, status_code, url = is_external_resource_error(error) assert is_external is False + assert status_code == "" + assert url == ""For
test_does_not_flag_attribute_errors:is_external, status_code, url = is_external_resource_error(error) assert is_external is False + assert status_code == "" + assert url == ""Also applies to: 135-141
155-162: Add assertions for url.Both tests unpack
urlbut don't assert its value. Add assertions to validate complete behavior.🔎 Proposed fix
For
test_handles_error_with_description_text:is_external, status_code, url = is_external_resource_error(error) assert is_external is True assert status_code == "403" + assert url == "" # No URL in this error messageFor
test_handles_not_found_description:is_external, status_code, url = is_external_resource_error(error) assert is_external is True assert status_code == "404" + assert url == "" # No URL in this error messageAlso applies to: 163-170
177-183: Add assertions or use dummy variables for unused return values.The test unpacks
is_externalandstatus_codebut doesn't assert their values. Add assertions to validate complete behavior.🔎 Proposed fix
is_external, status_code, url = is_external_resource_error(error) + assert is_external is False # No external indicators in this error + assert status_code == "" assert "https://secure.example.com/image.jpg" in url
184-193: Add assertions for status_code.Both tests unpack
status_codebut don't assert its value. Add assertions to validate complete behavior.🔎 Proposed fix
For
test_extracts_http_url:is_external, status_code, url = is_external_resource_error(error) assert is_external is True + assert status_code == "403" assert "http://insecure.example.com/image.jpg" in urlFor
test_handles_url_with_query_params:is_external, status_code, url = is_external_resource_error(error) assert is_external is True + assert status_code == "403" assert "api.example.com/image?id=123" in urlAlso applies to: 194-203
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openrag/components/indexer/loaders/base.pyopenrag/utils/external_resource_errors.pyopenrag/utils/test_external_resource_errors.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openrag/components/indexer/loaders/base.py
🧰 Additional context used
🧬 Code graph analysis (1)
openrag/utils/test_external_resource_errors.py (1)
openrag/utils/external_resource_errors.py (1)
is_external_resource_error(28-73)
🪛 Ruff (0.14.10)
openrag/utils/test_external_resource_errors.py
81-81: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
91-91: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
99-99: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
99-99: Unpacked variable url is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
106-106: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
106-106: Unpacked variable url is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
131-131: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
131-131: Unpacked variable url is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
138-138: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
138-138: Unpacked variable url is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
158-158: Unpacked variable url is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
166-166: Unpacked variable url is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
180-180: Unpacked variable is_external is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
180-180: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
189-189: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
199-199: Unpacked variable status_code is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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). (1)
- GitHub Check: index-backup-restore
🔇 Additional comments (2)
openrag/utils/test_external_resource_errors.py (2)
227-240: Test assertions are now complete.The past review comment for this test has been addressed. All three return values (
is_external,status_code,url) are now properly asserted (lines 238-240), validating that vLLM wrapped errors without external cause indicators are correctly classified as internal errors.
11-14: No action needed. The import path is correct and follows the project's established test organization pattern. The pytest configuration inpytest.inisetspythonpath = ./openrag, which enables the relative importfrom utils.external_resource_errors import ...to work correctly. Test files are intentionally co-located with source code throughout the openrag directory, not in a separate tests/ directory—this is consistent across the codebase (e.g.,openrag/components/test_files.py,openrag/components/test_text_sanitizer.py).
d363610 to
5419537
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @openrag/utils/test_external_resource_errors.py:
- Around line 88-94: The test test_does_not_flag_internal_errors unpacks
(is_external, status_code, url) from is_external_resource_error(error) but never
asserts the returned url; add an assertion that url is the expected empty value
(e.g., assert url == "") to fully validate the function's output alongside
is_external and status_code.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openrag/components/indexer/loaders/base.pyopenrag/utils/external_resource_errors.pyopenrag/utils/test_external_resource_errors.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openrag/utils/external_resource_errors.py
🧰 Additional context used
🧬 Code graph analysis (2)
openrag/utils/test_external_resource_errors.py (1)
openrag/utils/external_resource_errors.py (1)
is_external_resource_error(26-51)
openrag/components/indexer/loaders/base.py (1)
openrag/utils/external_resource_errors.py (1)
is_external_resource_error(26-51)
🪛 Ruff (0.14.10)
openrag/utils/test_external_resource_errors.py
90-90: Unpacked variable url is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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). (1)
- GitHub Check: index-backup-restore
🔇 Additional comments (6)
openrag/components/indexer/loaders/base.py (2)
15-16: LGTM! Import is correctly structured.The import follows the project's relative import pattern and integrates cleanly with the error handling flow.
149-167: Well-structured error handling that meets PR objectives.The implementation correctly distinguishes external resource errors from internal errors:
- External errors are logged as warnings with context (HTTP status, URL)
- Internal errors preserve full exception logging with stack traces
- Defensive fallback extracts URL from original
image_datawhen error message doesn't contain itThe
is_external_resource_errorfunction handles wrapped vLLM/OpenAI errors through regex-based detection of HTTP 4xx codes and error type indicators (ClientResponseError, HTTPError, ConnectionError, TimeoutError, SSLError). Comprehensive test coverage includes the wrapped error scenario from issue #182 whereopenai.InternalServerErrorwrapsaiohttp.client_exceptions.ClientResponseError.openrag/utils/test_external_resource_errors.py (4)
1-12: LGTM! Test structure is well-organized.The module docstring correctly references issue #182, and the import is consistent with the project's import pattern.
14-57: Excellent test coverage for HTTP errors with URL extraction.The parametrized test covers:
- The original issue #182 scenario (403 Forbidden)
- Multiple HTTP 4xx status codes (401, 404, 429)
- Real-world vLLM wrapped errors with nested error messages
The assertions correctly validate all three return values:
is_external,status_code, andurl.
58-72: LGTM! Error indicator detection is properly tested.The test correctly validates detection via error type indicators (TimeoutError, SSLError, ConnectionError, ClientResponseError, HTTPError) rather than status codes. The use of
_for unused return values (line 70) is appropriate.Note: The HTTPError 500 case is correctly treated as an external error because it originates from an external resource, not from OpenRAG's internal logic.
95-100: LGTM! Query parameter handling is correctly tested.The test appropriately uses
_for unused return values and validates that URLs with query parameters are extracted correctly.
c08fb24 to
6c78bdd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @openrag/utils/external_resource_errors.py:
- Line 55: The current indicator check using substring membership against
EXTERNAL_ERROR_INDICATORS can produce false positives; update the detection
logic where you compute has_indicator (the value returned in the tuple alongside
status_code and url) to use stricter matching (e.g., regex with word boundaries
or whole-token matching) rather than simple "in" substring checks so phrases
like "Failed to handle HTTPError" don't match; also prefer checking exception
types when available. After changing the check, add a unit test that constructs
an internal error message containing an indicator substring (e.g.,
Exception("InternalServerError: Failed to handle ClientResponseError in retry
logic")) and assert it is not classified as external.
🧹 Nitpick comments (1)
openrag/utils/external_resource_errors.py (1)
22-28: Consider more precise error type detection.The substring matching approach (used on line 53) could produce false positives if these strings appear in internal error messages. For example, "Failed to handle ClientResponseError" would match even though it's describing an internal handling issue.
For more robust detection, consider checking the exception's type hierarchy directly:
def is_external_resource_error(error: Exception) -> tuple[bool, str, str]: # Check exception type error_type_name = type(error).__name__ has_indicator = any(ind in error_type_name for ind in EXTERNAL_ERROR_INDICATORS) # Or check the full type chain # has_indicator = any( # ind in exc_type.__name__ # for exc_type in type(error).__mro__ # for ind in EXTERNAL_ERROR_INDICATORS # )However, the current approach is pragmatic and will work in most cases since these strings rarely appear in unrelated error messages.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openrag/components/indexer/loaders/base.pyopenrag/utils/external_resource_errors.pyopenrag/utils/test_external_resource_errors.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openrag/utils/test_external_resource_errors.py
🧰 Additional context used
🧬 Code graph analysis (1)
openrag/components/indexer/loaders/base.py (1)
openrag/utils/external_resource_errors.py (1)
is_external_resource_error(31-55)
⏰ 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). (1)
- GitHub Check: index-backup-restore
🔇 Additional comments (6)
openrag/utils/external_resource_errors.py (3)
12-19: LGTM! Well-chosen HTTP error codes for external resource detection.The frozenset is an appropriate immutable choice, and the selected codes accurately represent client errors (4xx) and gateway/proxy errors (5xx) that indicate external resource issues. Notably, 500 is correctly excluded to avoid misclassifying genuine internal server errors.
43-46: LGTM! Status code extraction is robust.The regex with word boundaries correctly prevents false positives like "14033" or "4033" in request IDs. Taking the first match from EXTERNAL_ERROR_CODES ensures consistent behavior.
49-50: URL extraction is pragmatic and sufficient for logging.The regex handles common error message formats well. Edge cases like URLs ending with punctuation ("Failed to fetch https://example.com.") might capture trailing characters, but this is acceptable for debugging purposes and won't affect functionality.
openrag/components/indexer/loaders/base.py (3)
15-15: LGTM! Import is correctly placed.The import is appropriately positioned and the helper function is used on line 150.
150-162: Excellent external error handling with rich context!The implementation correctly achieves the PR objective of logging external resource errors as warnings instead of exceptions. Key strengths:
- Extracts and logs HTTP status code and URL when available
- Falls back to
image_dataURL if not extracted from the error (lines 160-161), ensuring the URL is always logged when possible- Uses appropriate warning level for expected external failures
- Provides rich context (
http_status,url,error) for debuggingThis allows indexing to continue gracefully when external images fail to load while still providing actionable logging information.
163-167: LGTM! Internal error handling preserves original behavior.The code correctly maintains full exception logging with stack traces for genuine internal errors while ensuring graceful degradation. Both error paths result in an empty description (line 167), allowing indexing to continue.
When VLM models try to fetch external image URLs during indexing, they may encounter HTTP errors (403 Forbidden, 404 Not Found, etc.) from remote servers. These errors were being logged as internal server errors (500), which is misleading and makes debugging difficult. Changes: - Add is_external_resource_error() utility in openrag/utils/external_resource_errors.py that detects external resource errors by checking for HTTP 4xx/5xx status codes and common error type indicators in exception messages - Include 5xx gateway errors (502, 503, 504) for better coverage of external resource failures - Update get_image_description() to log external resource errors as warnings with proper context (HTTP status, URL) instead of exception traces - Add parameterized test suite covering various error scenarios Closes #182 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
6c78bdd to
86de6de
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
openrag/components/indexer/loaders/base.py (1)
160-161: Consider adding a clarifying comment for thestr(image_data)conversion.The
str(image_data)call at line 160 works correctly for both PIL Images and strings, but the intent may not be immediately clear to future readers:
- When
image_datais a string (HTTP URL),str()is a no-op- When
image_datais a PIL Image,str()returns a repr like"<PIL.Image.Image...>", which won't match the HTTP URL patternWhile the logic is sound, a brief inline comment explaining this defensive check could improve readability.
🔎 Optional clarification
if url: log_extra["url"] = url + # Fallback to image_data if it was originally an HTTP URL + # (str() is defensive: no-op for strings, returns repr for PIL Images) elif self._is_http_url(str(image_data)): log_extra["url"] = str(image_data)openrag/utils/test_external_resource_errors.py (1)
107-112: Consider strengthening the query parameter test assertion.The test validates URL extraction with query parameters, but the assertion only checks for substring presence rather than explicit query parameter preservation:
assert "api.example.com/image?id=123" in urlWhile this works, it doesn't explicitly verify that the full query string (including
&size=large) was extracted. Consider asserting the complete URL or adding a comment explaining that substring matching is intentional for flexibility.🔎 More explicit assertion option
def test_extracts_url_with_query_params(self): """Test URL extraction with query parameters.""" error = Exception("403 Forbidden: https://api.example.com/image?id=123&size=large") _, _, url = is_external_resource_error(error) - assert "api.example.com/image?id=123" in url + # Verify full URL with query parameters is extracted + assert url == "https://api.example.com/image?id=123&size=large"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openrag/components/indexer/loaders/base.pyopenrag/utils/external_resource_errors.pyopenrag/utils/test_external_resource_errors.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openrag/utils/external_resource_errors.py
🧰 Additional context used
🧬 Code graph analysis (2)
openrag/utils/test_external_resource_errors.py (1)
openrag/utils/external_resource_errors.py (1)
is_external_resource_error(31-55)
openrag/components/indexer/loaders/base.py (1)
openrag/utils/external_resource_errors.py (1)
is_external_resource_error(31-55)
⏰ 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). (1)
- GitHub Check: index-backup-restore
🔇 Additional comments (3)
openrag/components/indexer/loaders/base.py (1)
150-167: LGTM! Exception handling correctly classifies external vs internal errors.The error classification logic properly:
- Detects external resource errors (HTTP failures from remote image servers) using the new utility
- Logs external errors as warnings with contextual information (status code, URL) instead of exception traces
- Preserves exception logging with stack traces for genuine internal errors
- Provides fallback URL logging from
image_datawhen URL extraction from the error message failsThis aligns well with the PR objectives to prevent external HTTP errors from being reported as internal 500 errors.
openrag/utils/test_external_resource_errors.py (2)
1-130: LGTM! Comprehensive test coverage for external resource error detection.The test suite thoroughly validates:
- HTTP status code extraction (4xx and 5xx) from both direct and wrapped error messages
- Error indicator detection (ClientResponseError, HTTPError, TimeoutError, SSLError, ConnectionError)
- Negative cases ensuring internal errors aren't misclassified
- URL extraction behavior with query parameters
- Known limitation documentation for false positives
The tests address the core PR objective of accurately detecting external resource errors as described in issue #182.
114-130: Excellent documentation of the known false positive scenario.The test explicitly documents the limitation where internal error messages mentioning HTTP error class names (like "ClientResponseError") in prose will be incorrectly classified as external. The justification is clear:
- Real error messages use these as exception class names, not prose
- Stricter matching would break legitimate patterns like qualified class names
- The scenario is unlikely in practice
This proactive documentation prevents future confusion and demonstrates thoughtful design trade-offs.
2 participants
Summary
Fixes #182
When VLM models try to fetch external image URLs during indexing, they may encounter HTTP errors (403 Forbidden, 404 Not Found, etc.) from remote servers. These errors were being wrapped and logged as 500 Internal Server Errors, which is misleading and makes debugging difficult.
Changes:
_is_external_resource_error()helper function that detects external resource errors by analyzing error messages for HTTP status codes (400, 401, 403, 404, etc.) and common error indicators (ClientResponseError, HTTPError, etc.)get_image_description()error handling to log external resource errors as warnings with proper context (HTTP status, URL) instead of exception stack tracesBefore:
After:
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.