Skip to content

fix: keep whitespace inside fenced code blocks when normalizing results - #2529

Open
Manohar Paturi (ManoharPaturi) wants to merge 5 commits into
microsoft:mainfrom
ManoharPaturi:fix/preserve-code-block-whitespace
Open

Manohar Paturi (ManoharPaturi) wants to merge 5 commits into
microsoft:mainfrom
ManoharPaturi:fix/preserve-code-block-whitespace

Conversation

@ManoharPaturi

Copy link
Copy Markdown

Fixes #2528.

the normalizer now tracks fenced regions (backtick and tilde fences) and skips line-level rstrip and newline-run collapsing inside them, so code blocks come through byte-exact while prose outside fences keeps the existing normalization.

tests cover blank-line runs inside fences, trailing spaces, tilde fences, and that outside-fence normalization still applies. all fail on main, pass here.

Copilot AI lite review requested due to automatic review settings September 18, 2026 11:57

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There's still a boundary case here: a blank-line run immediately before a fence can escape the collapse. With para\n\n\n```..., flush_segment() leaves para\n\n, then the final "\n".join(out) adds another newline before the fence, so it stays at three newlines instead of two. Could you add an adjacent-fence case and avoid adding that extra separator?

MarkItDown._convert applied its whitespace cleanup (rstrip every line,
collapse 3+ newlines to 2) to the entire converted document, which
corrupted fenced code blocks: blank-line runs inside a fence collapsed
and trailing spaces were stripped, changing code content. Track code
fences while normalizing and leave their contents untouched.

Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
@ManoharPaturi

Copy link
Copy Markdown
Author

good catch, reproduced it exactly as you described (three newlines surviving before the fence). fixed by stripping the trailing newlines when a segment flushes because a fence follows, and keeping exactly one blank line between preceding prose and the fence. the EOF flush keeps the old behavior so trailing newlines at end of document are untouched.

added the adjacent-fence case as a test, plus 2-and-4-newline variants. the whitespace suite is green and the only failure in the full run (test_speech_transcription) is the same on main, it needs a model download in this environment.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rechecked 7e6ed5c. The fence boundary case is fixed: a prose segment is stripped of trailing newlines before the separator is added, so runs immediately before a fence collapse to exactly one blank line. The adjacent-fence regression covers the case I raised. No blocker from me.

@ManoharPaturi

Copy link
Copy Markdown
Author

thanks for the quick recheck and the thorough review, the boundary catch made the fix properly correct. the workflows still need a maintainer approval to run, so whenever someone with write access can green-light those, this should be ready to go.

@kokokoXUY XU (kokokoXUY) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new fence detector only recognizes fences at the start of a line after at most three spaces. It misses valid fenced code inside a block quote (> ```text) and inside a list item with four absolute spaces before the fence. I ran MarkItDown().convert() from head 7e6ed5c on both cases. A plain fence preserved its contents, but the block-quote case changed '> first \n' to '> first\n'; the list case stripped trailing spaces and collapsed a two-blank-line run inside the code block. These openings stay in the prose branch, so the whitespace loss reported in #2528 remains for nested fences.

Could you add regression coverage for a fence inside a block quote and a list, then make the normalizer recognize container prefixes or otherwise preserve those code lines? CommonMark permits these container cases.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rechecked current 7e6ed5c5 after the new nested-fence report. The issue is real and supersedes my earlier approval.

_CODE_FENCE_RE = ^ {0,3}(... ) only recognizes a fence at the absolute start of the line after at most three spaces. Valid fenced blocks inside Markdown containers are therefore missed. In particular, a block-quoted fence such as > ```text never enters fence mode, and a fence inside a list item can have more than three absolute leading spaces even though its indentation is valid relative to the list marker. Those lines stay in the prose segment, so rstrip() and newline-run collapsing can still alter code contents.

Please add regressions for at least a block-quoted fenced block and a list-contained fenced block and make fence detection container-aware (or otherwise avoid normalizing the nested code body). My earlier boundary concern remains fixed; this is a separate container-prefix gap.

@ManoharPaturi

Copy link
Copy Markdown
Author

XU (@kokokoXUY) Sylvester Kaczmarek (@sylvesterkaczmarek) the container gap is real, thank you for the precise report with runnable cases. XU's fix is exactly right so i folded it into this branch with his authorship preserved (1652976, cherry-picked from the fork PR): fence detection is now container-aware with quote-depth matching on open and close, and list-item indentation is handled by measuring the fence position inside the container rather than from the absolute line start.

verified the two reported cases on the new head: the block-quoted fence keeps its trailing spaces and blank-line runs, and the list-contained fence keeps its code whitespace too. the full whitespace suite is 14/14 and the markitdown suite is green except the pre-existing speech-model download failure that also fails on main.

kokokoXUY's fork PR can be closed once this lands, the commit is identical.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rechecked current 1652976b against the nested-container gap from my last review. The fence parser is now container-aware: it tracks block-quote depth, measures list-contained fence indentation relative to the active list context, requires matching quote depth on close, and exits fence mode when the containing quote/list ends.

The new regressions cover the two cases I asked for directly (block-quoted and list-contained fences), plus container termination, nested-list state, quote-inside-list, adjacent container text, and the indented-literal-fence control. My earlier top-level boundary concern also remains covered. No remaining blocker from my review.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rechecked current 1917d7ed against the nested-fence fix I previously approved. The three commits since 1652976b are an upstream main merge covering Python 3.14/dependency/version updates; they do not touch the fence-normalization implementation or its regressions. The container-aware fence fix remains unchanged. No blocker from my review.

@workstonedai-collab

Copy link
Copy Markdown

I found one remaining opening-fence case on head 1917d7e: when the fence starts immediately after the list marker (- ```text or 1. ```text), code trailing spaces and blank-line contents are still normalized. A list inside a block quote reproduces it as well.

I prepared a small follow-up PR against your branch: ManoharPaturi#2

The patch allows list-marker stripping only for opening detection, so a literal - ``` line inside an existing code fence does not close it. Six new backtick/tilde cases plus that closing control pass alongside your 14 existing whitespace regressions (21 passed). The tests use MarkItDown.convert with explicit UTF-8 and synthetic content, with no external services.

The follow-up is available for merge or cherry-pick into this PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reproduced this on current head 1917d7e. All six opening-fence cases from the follow-up fail on the current branch (unordered, ordered, and quoted lists with both backtick and tilde fences), while the literal list-marker-inside-a-fence control passes.

I also ran the follow-up patch at 6130675: the existing 14 whitespace regressions plus the 7 new cases pass 21/21, and git diff --check is clean. The implementation is appropriately narrow: list-marker stripping is allowed only for opening-fence detection, not closing-fence detection.

Please merge/cherry-pick ManoharPaturi#2 before merging this PR. I'm marking this as changes requested until that correction is incorporated.

@kokokoXUY XU (kokokoXUY) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verified independently on both trees — the six cases do fail on 1917d7ede4eb, and the follow-up at 6130675 fixes them without touching the closing-fence path.

Method. Two worktrees of one clone: this PR's head (1917d7ede4eb) and ManoharPaturi/markitdown#2 (6130675, fix: preserve code fences following list markers). Each run imported markitdown from that tree's packages/markitdown/src with the resolved markitdown.__file__ asserted, so the copy installed in site-packages cannot shadow it.

Results

check 1917d7ede4eb 6130675
tests/test_code_fence_whitespace.py 14 passed 14 passed
tests/test_list_marker_code_fences.py (6 parametrisations + control) 6 failed, 1 passed 7 passed
my matrix: those 6 + 17 variants + 3 literal-marker controls 20 failed, 4 passed 24 passed
rest of tests/ (¹) 99 failed, 810 passed, 6 skipped 99 failed, 817 passed, 6 skipped

¹ tests/test_outlook_msg_ansi.py, tests/test_pptx_{empty_title,images,none_text,notes,svg}.py and tests/test_xlsx_images.py were excluded because this environment has no PIL/olefile. The other 99 failures are pre-existing here (optional extras such as pdfplumber/azure are missing, plus the vector suites), so I diffed the failed-test sets instead of the totals: they are identical between the two trees, the only difference being the six test_fence_after_list_marker_preserves_whitespace parametrisations. git diff --check 1917d7ede4eb 6130675 is clean.

The 17 extra cases I ran — all fail on the PR head, all pass at 6130675:

  • other bullet markers and ordered forms: * ```text, + ~~~text, 1) ```text, 10. ```text
  • wider spacing after the marker: - ```text (three spaces, content indent 4)
  • inside a block quote: > 1. ```text, > * ```text
  • nesting: - outer / - ```text (content indent 4), two levels deep (indent 6), and - item with a > - ```text continuation
  • no info string: - ```
  • closing-fence shapes: ``` (trailing spaces) and an unindented closing line, where the fence must end so that the following outside is normalised
  • a four-backtick opening fence (- ````text) not closed by a three-backtick line
  • controls that pass on both trees: * ``` and 1. ``` inside a top-level fence, and - ``` inside a list fence

Two notes, neither of them blocking

  1. list_item.end() == list_indent cannot reject anything as written. active_list_indent is content_indent, computed as len(indent) + len(marker) + len(space), which is exactly _LIST_ITEM_RE.match(rest).end() — and a line that introduces a list item pushes that context before the fence check runs, so the two values are always equal when the branch is reached. The narrowing that actually protects the control case is allow_list_marker=True on the opening-fence path and its absence from _closes_fence. Fine as defensive code; just not the selectivity check.

  2. The candidate is "everything after the marker", so a list item whose text merely starts with a fence marker is now treated as opening a fence. - ```text``` inline code is a paragraph in CommonMark (a backtick fence's info string may not contain a backtick), but the patch finds a marker there:

>>> _is_code_fence('- ```text``` inline code', 2, allow_list_marker=True)
_FenceMarker(marker='```', end=5, quote_depth=0)

With the following lines indented to the item's content width that changes the output:

in  = '- ```text``` inline code\n  next   \n  \n  \n  end\noutside  \n'
1917d7ede4eb -> '- ```text``` inline code\n  next\n\n  end\noutside\n'
6130675      -> '- ```text``` inline code\n  next   \n  \n  \n  end\noutside\n'

That is over-preservation inside a list item rather than corruption, it needs the following lines to be indented by the item's content width to matter, and I would not hold the PR for it. If you want to close it, rejecting a candidate whose info string contains a backtick (for a backtick marker) would do it.

Disclosure: this verification was prepared with an AI coding assistant (DeepSeek) under my direction; I reviewed the diffs and the commands above before posting.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rechecked current head b3dedf5 after the latest main merge. That merge does not include the list-marker opening fix from ManoharPaturi#2: the current PR diff still has no allow_list_marker path or the seven list-marker regressions. The six reproduced opening cases therefore remain unresolved on this head.

My changes-requested review still stands: please merge/cherry-pick ManoharPaturi#2 (or an equivalent fix) before this PR lands.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rechecked current head cdf761f. This now includes the exact opening-only list-marker fix I previously validated in ManoharPaturi#2: list markers are stripped only while detecting an opening fence, the closing path stays unchanged, and the six backtick/tilde list-opening cases plus the literal-marker control are included as regressions. My changes-requested blocker is resolved.

@ManoharPaturi

Copy link
Copy Markdown
Author

Workstone (@workstonedai-collab) great catch, and thanks for doing the work as a ready patch. folded your commit into this branch with your authorship kept (cdf761f). the allow_list_marker split between opening and closing detection is the right shape, and the closing control case (a literal `- ``` inside a fence not closing it) is exactly the regression i would have missed.

all 21 fence cases pass on this head alongside the existing 14 whitespace regressions. the only local failures in the full suite are the azure cu/docintel/speech tests, which fail the same way on a clean checkout here because those optional sdks are not installed in my env, nothing to do with this change.

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.

fenced code blocks lose blank-line runs and trailing spaces during conversion

5 participants