fix(markitdown-ocr): sort placeholders by length before substitution to prevent prefix collision - #2387
Open
Azhar (bunnysayzz) wants to merge 1 commit into
Conversation
Author
|
@microsoft-github-policy-service agree |
…to prevent prefix collision When a DOCX has 11 or more images, the substitution loop in DocxConverterWithOCR._convert_local() iterates in ascending index order and calls str.replace(placeholder, ocr_block) for each. Because "MARKITDOWNOCRBLOCK1" is a prefix of "MARKITDOWNOCRBLOCK10" through "MARKITDOWNOCRBLOCK19", replacing index 1 first corrupts every two-digit placeholder: MARKITDOWNOCRBLOCK10 becomes the content of block 1 followed by a literal "0", dropping the OCR content for image 10 and duplicating the content for image 1. Fix: build the full list of (placeholder, ocr_block) pairs first, then sort by descending placeholder length before iterating. Longer placeholders (MARKITDOWNOCRBLOCK10) are replaced before shorter ones (MARKITDOWNOCRBLOCK1), so no prefix collision can occur. Five regression tests added to tests/test_ocr_placeholder_collision.py: - test_buggy_code_leaves_corrupted_placeholder_for_11_images: documents the bug - test_fixed_code_no_remaining_placeholders_for_11_images: verifies fix (11 images) - test_fixed_code_all_ocr_blocks_present_for_11_images: all blocks present - test_fixed_code_no_remaining_placeholders_for_100_images: verifies fix (100 images) - test_no_regression_under_10_images: no regression for small documents Fixes microsoft#2383
Azhar (bunnysayzz)
force-pushed
the
fix/ocr-placeholder-prefix-collision-2383
branch
from
September 8, 2026 03:28
da02680 to
1808375
Compare
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.
Root cause
_PLACEHOLDER = "MARKITDOWNOCRBLOCK{}"inpackages/markitdown-ocr/src/markitdown_ocr/_docx_converter_with_ocr.py.The substitution loop in
DocxConverterWithOCR._convert_local()iterates in ascending index order and callsstr.replace(placeholder, ocr_block)for each image. Because"MARKITDOWNOCRBLOCK1"is a prefix of"MARKITDOWNOCRBLOCK10"through"MARKITDOWNOCRBLOCK19", replacing index 1 first corrupts every two-digit placeholder:MARKITDOWNOCRBLOCK10becomes the OCR content of block 1 followed by a literal"0", dropping the OCR content for image 10 and duplicating the content for image 1.Reproduction (no OCR service needed):
Fix
Build the full list of
(placeholder, ocr_block)pairs first, then sort by descending placeholder length before iterating. Longer placeholders (MARKITDOWNOCRBLOCK10) are replaced before shorter ones (MARKITDOWNOCRBLOCK1), so no prefix collision can occur.Test evidence
Five regression tests in
packages/markitdown-ocr/tests/test_ocr_placeholder_collision.py:The first test (
test_buggy_code_leaves_corrupted_placeholder_for_11_images) documents the bug by asserting the old behavior. The remaining four verify the fix.Fixes #2383