Handle malformed XML in extract_text_from_xml and add edge case test#67
Open
tharunkumar4562 wants to merge 8 commits intoAOSSIE-Org:mainfrom
Open
Handle malformed XML in extract_text_from_xml and add edge case test#67tharunkumar4562 wants to merge 8 commits intoAOSSIE-Org:mainfrom
tharunkumar4562 wants to merge 8 commits intoAOSSIE-Org:mainfrom
Conversation
Contributor
|
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:
Walkthroughextract_text_from_xml now writes to a temporary file and parses atomically with try/except that cleans temp artifacts and re-raises contextual Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
github-actions
bot
added
backend
python
size/M
first-time-contributor
pending-coderabbit-review
and removed
size/M
labels
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openverifiablellm/utils.py`:
- Around line 192-212: The code currently opens output_path before the XML parse
completes, leaving a truncated wiki_clean.txt if parsing fails; modify the logic
in the block using open_func, ET.iterparse, and clean_wikitext to write to a
temporary file (e.g., via tempfile.NamedTemporaryFile or a temp path) and only
atomically replace output_path with os.replace(temp_path, output_path) after the
parse loop finishes successfully, ensuring any ET.ParseError logs the input_path
(using logger.error) and the temp file is removed on exceptions; keep
ET.ParseError handling but do not write to the final output until the atomic
rename succeeds.
In `@tests/test_util.py`:
- Around line 156-174: The test should assert the specific exception type thrown
by utils.extract_text_from_xml instead of any Exception: change the
pytest.raises to expect xml.etree.ElementTree.ParseError (ET.ParseError) and
verify the exception message includes the input file path (input_file) or the
"Failed to parse XML" prefix to ensure the implementation's contextual error is
preserved; target the test function test_extract_text_from_xml_malformed and the
utils.extract_text_from_xml call when updating the assertion.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 5e43f95c-39b7-404f-b9cb-6c09abc7b12f
📒 Files selected for processing (2)
openverifiablellm/utils.pytests/test_util.py
… output on XML parse failure
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openverifiablellm/utils.py`:
- Around line 217-222: The cleanup block currently swallows all exceptions from
temp_output_path.unlink(); change the broad except to only catch OSError (e.g.
except OSError as e:) and emit a warning or logger.warn including the
temp_output_path and exception details so the leftover .tmp is diagnosable,
while still not suppressing the original parse error (do not swallow other
exception types).
- Around line 223-224: The current re-raise creates a fresh ET.ParseError losing
parser metadata; instead, preserve the original exception's structured fields by
creating a new ET.ParseError with your contextual message and then copy
e.position and any other metadata (e.g., e.code) from the caught exception
before raising it (use the caught variable name e and ET.ParseError and include
msg as the new message). Ensure you copy attributes defensively (use getattr(e,
"position", None) and getattr(e, "code", None)) so downstream handlers still see
the original position/code on the raised exception.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: cc2aa65f-4f7f-4d36-8880-1cbd10195694
📒 Files selected for processing (1)
openverifiablellm/utils.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openverifiablellm/utils.py`:
- Around line 212-215: The except block catching ET.ParseError should log the
full traceback so failures are diagnosable: replace the current
logger.error(msg) call in the ET.ParseError handler (the variables input_path
and msg are built there) with logger.exception(msg) or logger.error(msg,
exc_info=True) so the original stack/exception context is preserved when parsing
fails in that code path.
- Around line 235-236: The current code logs "Preprocessing complete" before
calling generate_manifest, which can produce misleading success logs if manifest
generation fails; move the logger.info call so it occurs after
generate_manifest(input_path, output_path) completes (or split into two logs:
one for extraction completion and another for successful manifest creation).
Specifically, update the sequence around generate_manifest, logger.info, and the
variables input_path/output_path so that logger.info("Preprocessing complete.
Output saved to %s", output_path) executes only after
generate_manifest(input_path, output_path) returns successfully.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 9202e810-90b2-4631-948e-248db2253280
📒 Files selected for processing (1)
openverifiablellm/utils.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_util.py`:
- Around line 206-222: The test's failing_iterparse currently raises before a
page is completed, so change it to raise only after the first page has been
fully processed by detecting the page end event: iterate over original_iter
inside failing_iterparse, yield events normally, track when you see an event
where elem.tag.endswith("page") (or the corresponding end event) and only then
raise a _ET.ParseError to simulate a midstream failure after one page has been
written; this ensures utils.extract_text_from_xml actually writes partial data
to tmp_file (and then the test can assert cleanup/partial-write behavior).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: df6352f4-b8ea-4736-aaa9-cf315616ce0d
📒 Files selected for processing (2)
openverifiablellm/utils.pytests/test_util.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openverifiablellm/utils.py`:
- Around line 236-238: The success path in extract_text_from_xml references an
undefined variable write_manifest causing a NameError; fix by removing the
conditional guard and always calling generate_manifest(input_path, output_path)
at the end of extract_text_from_xml (or alternatively add a parameter
write_manifest with a default and use it), but prefer the safer change:
unguarded call to generate_manifest so extract_text_from_xml calls
generate_manifest(input_path, output_path) unconditionally.
In `@openverifiablellm/verify.py`:
- Around line 288-296: The current code unconditionally adds a PASS CheckResult
for "manifest_chunk_size_bytes" using report.add(CheckResult(...
name="manifest_chunk_size_bytes" ...)), which falsely signals success before any
comparison; remove or change this synthetic PASS and instead defer checking to
the reproduced-manifest comparison code: do not call report.add(...) here, and
in the reproduced-manifest block use the existing helper
_check_field(original_manifest.get("chunk_size_bytes"),
regenerated_manifest.get("chunk_size_bytes"), name="manifest_chunk_size_bytes")
(or call _check_field with the correct symbol names) so the stored
chunk_size_bytes is compared to the regenerated value and only marked PASS/FAIL
based on that comparison. Ensure the check uses the same field name
"manifest_chunk_size_bytes" so the report entry is produced by _check_field
rather than pre-marked.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 0ece45bb-2460-4911-aa81-f1be8ee5f47a
📒 Files selected for processing (2)
openverifiablellm/utils.pyopenverifiablellm/verify.py
Comment on lines
+236
to
+238
| # manifest generation must succeed before we declare the pipeline complete | ||
| if write_manifest: | ||
| generate_manifest(input_path, output_path) |
Contributor
There was a problem hiding this comment.
write_manifest is undefined on the success path.
Line 237 references write_manifest, but extract_text_from_xml does not define or accept it. Any successful parse now ends in a NameError before completion; tests/test_util.py:121 still calls this function with only input_path.
Suggested fix
-def extract_text_from_xml(input_path):
+def extract_text_from_xml(input_path, *, write_manifest: bool = True):If manifest writing is not meant to be optional, the safer fix is to remove the guard and always call generate_manifest(...).
🧰 Tools
🪛 Ruff (0.15.5)
[error] 237-237: Undefined name write_manifest
(F821)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openverifiablellm/utils.py` around lines 236 - 238, The success path in
extract_text_from_xml references an undefined variable write_manifest causing a
NameError; fix by removing the conditional guard and always calling
generate_manifest(input_path, output_path) at the end of extract_text_from_xml
(or alternatively add a parameter write_manifest with a default and use it), but
prefer the safer change: unguarded call to generate_manifest so
extract_text_from_xml calls generate_manifest(input_path, output_path)
unconditionally.
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
1 participant
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.
Addressed Issues:
Fixes #48
Screenshots/Recordings:
Before fix: malformed XML raised a low-level ParseError without context.
After fix: the error now includes a clear message indicating which
XML dump failed to parse.
Additional Notes:
While running the test suite locally, I noticed that
extract_text_from_xmldoes not provide useful context when encountering malformed XML.Changes in this PR:
ET.iterparseto catchxml.etree.ElementTree.ParseError.test_extract_text_from_xml_malformedto verify behaviour when malformed XML is encountered.This improves robustness of the XML extraction step and prevents unclear crashes when corrupted Wikipedia dumps are processed.
All tests pass locally.
Checklist
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit
Bug Fixes
Public API
Tests