Do not convert an undecodable file to the word "None" - #2418
Merged
afourney merged 3 commits intoSep 9, 2026
Conversation
Contributor
Author
|
@microsoft-github-policy-service agree |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The regression tests depend on unstable charset-detection heuristics and include a randomized path that can pass without assertions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prevents undecodable text and CSV files from becoming fabricated "None" content.
Changes:
- Adds UTF-8 fallback decoding when charset detection fails.
- Adds regression coverage for text, Markdown, CSV, and declared charsets.
File summaries
| File | Description |
|---|---|
_plain_text_converter.py |
Handles failed charset detection. |
_csv_converter.py |
Adds shared CSV decoding fallback. |
test_undetectable_charset.py |
Adds regression tests for undecodable input. |
Review details
Suppressed comments (1)
packages/markitdown/tests/test_undetectable_charset.py:85
- This randomized test can pass without exercising or asserting anything whenever charset-normalizer recognizes the generated bytes, so it does not reliably establish the claimed behavior. Please replace it with a deterministic stub that makes
best()returnNone, or remove it because the fixed regression case already covers this path.
blob = os.urandom(4096)
if from_bytes(blob).best() is not None:
return # detection claimed it; nothing to assert here
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| # Random bytes: no charset claims them, which is what makes `best()` answer None. | ||
| # Fixed rather than generated, so the test does not depend on chance. | ||
| UNDECODABLE = bytes((7 * i * i + 251 * i + 193) % 256 for i in range(4096)) |
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
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.
What this changes
A binary file handed to MarkItDown with a text extension is converted to the four-character string
None.charset_normalizer.from_bytes(data).best()returnsNonewhen nothing decodes the bytes, andstr(None)is"None". Two converters write that expression inline:Measured, on 4096 bytes no charset claims:
That is content that was never in the file, produced by a tool whose output is read by a model. A
document that failed to decode reads downstream as a document whose contents are the word "None".
The other two callers already guard it
This is the same call in four places, and the other two handle the
None:The two that inlined
str(...)are the two that skipped the check.Fix
Both now check, and fall back the way
_outlook_msg_converteralready does: decode as UTF-8 anddrop what does not fit. The CSV side gets a small
_decode_detectedhelper so the reason is writtendown once.
I chose the existing fallback over raising
FileConversionExceptionbecause it is the conventionalready in the tree, and because the bytes are at least the file's own. If you would rather an
undecodable stream were refused outright, that is a one-line change and I am happy to make it.
Nothing else moves: a declared
stream_info.charsetstill short-circuits detection, and any filedetection does claim decodes exactly as before.
Tests
packages/markitdown/tests/test_undetectable_charset.py:.txtand.mdis not the word "None".csvis not a one-cell table of "None"charsetstill winsos.urandombytes, not only the fixed fixtureAgainst
main:How I tested
Windows 11, Python 3.12, editable install of
packages/markitdown.Those 15 are unchanged by this PR — I ran that file with and without the two edited converters and
got
15 failed, 48 passed, 3 skippedboth times. They are the tests needing credentials and mediatooling this machine does not have (
test_speech_transcription,test_markitdown_llm_parametersand friends).
blackclean on all three files.