Skip to content

Do not convert an undecodable file to the word "None" - #2418

Merged
afourney merged 3 commits into
microsoft:mainfrom
kevin9327:fix/undetectable-charset-not-none
Sep 9, 2026
Merged

Do not convert an undecodable file to the word "None"#2418
afourney merged 3 commits into
microsoft:mainfrom
kevin9327:fix/undetectable-charset-not-none

Conversation

@kevin9327

Copy link
Copy Markdown
Contributor

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() returns None when nothing decodes the bytes, and
str(None) is "None". Two converters write that expression inline:

# _plain_text_converter.py
text_content = str(from_bytes(file_stream.read()).best())

# _csv_converter.py
content = str(from_bytes(file_stream.read()).best())

Measured, on 4096 bytes no charset claims:

.txt  ->  'None'
.md   ->  'None'
.csv  ->  '| None |\n| --- |'

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:

# _outlook_msg_converter.py
detected = from_bytes(data).best()
if detected is not None:
    return str(detected).strip()
return data.decode("utf-8", errors="ignore").strip()

# _markitdown.py, _get_stream_info_guesses
charset_result = charset_normalizer.from_bytes(stream_page).best()
if charset_result is not None:
    charset = self._normalize_charset(charset_result.encoding)

The two that inlined str(...) are the two that skipped the check.

Fix

Both now check, and fall back the way _outlook_msg_converter already does: decode as UTF-8 and
drop what does not fit. The CSV side gets a small _decode_detected helper so the reason is written
down once.

I chose the existing fallback over raising FileConversionException because it is the convention
already 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.charset still short-circuits detection, and any file
detection does claim decodes exactly as before.

Tests

packages/markitdown/tests/test_undetectable_charset.py:

  • the fixture really is undecodable, so the rest cannot pass for the wrong reason
  • binary with .txt and .md is not the word "None"
  • binary with .csv is not a one-cell table of "None"
  • ordinary text and an ordinary CSV are byte-identical to before
  • a declared charset still wins
  • the same holds for os.urandom bytes, not only the fixed fixture

Against main:

E  AssertionError: assert 'None' != 'None'
E  AssertionError: assert '| None |\n| --- |' != '| None |\n| --- |'

How I tested

Windows 11, Python 3.12, editable install of packages/markitdown.

pytest tests/test_undetectable_charset.py   ->  6 passed
pytest tests/test_module_misc.py            ->  15 failed, 48 passed, 3 skipped

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 skipped both times. They are the tests needing credentials and media
tooling this machine does not have (test_speech_transcription, test_markitdown_llm_parameters
and friends).

black clean on all three files.

@kevin9327

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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() return None, 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.

Comment thread packages/markitdown/tests/test_undetectable_charset.py

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation is correct; only the PR description’s unsupported os.urandom coverage claim needs minor correction.

Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Balanced


# 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))
@afourney
afourney merged commit 24b9e12 into microsoft:main Sep 9, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants