Skip to content

fix(markdown): keep every preprocessing step on the same source line - #389

Merged
PathGao merged 1 commit into
sftwrdotdev:masterfrom
PathGao:fix/line-count-preserving-transforms
Aug 2, 2026
Merged

fix(markdown): keep every preprocessing step on the same source line#389
PathGao merged 1 commit into
sftwrdotdev:masterfrom
PathGao:fix/line-count-preserving-transforms

Conversation

@PathGao

@PathGao PathGao commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

The contract nobody wrote down

raw buffer
   │  process_parenthesized_autolinks
   │  process_internal_embeds
   │  process_wikilinks  (block ids, highlights, inline footnotes)
   │  protect_display_math_underscores
   ▼
markdown_to_html(…, sourcepos = true)   → line numbers refer to the PROCESSED text
   ▼
frontend reads data-sourcepos
   ▼
documentSession.toggleTaskCheckbox(line) writes into the RAW buffer at that line

So every step has to map input line N to output line N. Nothing said so, and nothing checked. When a step breaks it, a reading-mode checkbox rewrites a different line of the user's document — the shape of #352, where a - [x] marker ended up inside a code block.

Three steps broke it

Each row was probed on the unmodified code before fixing:

Step Verdict Measured
process_parenthesized_autolinks already safe URL scan breaks on whitespace, so a match cannot span a newline
process_internal_embeds breaks it moved line 5 to line 9. A stray ![[ paired with the ]] of a real embed lines below: prose rewritten into an <img> attribute, four lines eaten
process_wikilinks — wikilinks already safe upstream's if full.as_str().contains('\n') { return literal }, now pinned by the corpus
process_wikilinks — block ids breaks it (?m)\s+\^…$\s+ eats the newline. A block id on its own line folded onto the paragraph above; the HTML then claimed line 3 while raw line 3 was blank
process_wikilinks — highlights already safe [^=\n]+ excludes newline
process_wikilinks — inline footnotes breaks it [^\]]+ matches newlines; a wrapped ^[…] collapsed two lines into one and the task below lost its data-task-checkbox
process_wikilinks — appended [^ifn-N]: definitions grows, but safe appended strictly after the last input line, so no existing line is renumbered. This is why the contract is "line N stays line N", not "line counts are equal"
protect_display_math_underscores already safe per-character substitution inside $$…$$

The fixes, and why each shape

Embeds — drop (?s), do not bail out after matching. The wikilink step uses "match, then return literal if it contains a newline", but that shape is wrong here: with (?s), the runaway match still consumes the well-formed ![[real.png]] inside it, so the real image silently stops rendering either way. Not matching at all leaves the later embed free. The test asserts all three outcomes — prose intact, task clickable, image renders.

Obsidian's embed syntax is single-line in every documented form (![[Note]], ![[Note#^b15695]], ![[img.jpg|100x145]], ![[Doc.pdf#page=3]]), so a ![[ with no ]] on its line is not an embed and leaving it literal is the correct reading, not a degradation.

Block ids — capture the whitespace and re-emit it verbatim. Obsidian does accept a block id alone on the line after the block it names, so refusing to match would remove a feature. (?m)(\s+)\^([a-zA-Z0-9_-]+)$ re-emits the captured run: for the common trailing " ^id" this is the same single space that used to be hardcoded, and for the own-line form the newline goes back.

Inline footnotes — [^\]\n]+, mirroring HIGHLIGHT_RE. A wrapped ^[…] now stays literal.

This is a deliberate divergence from Pandoc, which allows an inline note to wrap within a paragraph. A multi-line inline note cannot be rewritten line-count-preservingly — the reference is one token, the text is many lines — so honouring it and honouring the contract are mutually exclusive. The contract wins; the multi-line spelling that does work is the standard [^ref] + [^ref]: … pair, which comrak already supports. Obsidian documents only the single-line form, and its multi-line behaviour is an open request rather than defined syntax.

annotate_task_checkboxes is unchanged, and now says why

It compares against the raw buffer. That looks like a mismatch — the HTML came from the processed text — but it is the fail-safe for this contract: when a step shifts lines, it fails closed and the checkbox stays inert. The block-id case is a live demonstration; passing the processed text instead would have made a checkbox there writable straight into the wrong line, upgrading every future regression from inert checkbox to corrupted document.

A doc comment now states that, and task_checkboxes_stay_inert_when_the_html_and_the_buffer_disagree pins it: HTML rendered from one document, fed the raw buffer of another whose line 3 is a code fence — the #352 shape — plus a control asserting a matching buffer is annotated.

Making the contract explicit

  • every_preprocessing_step_preserves_source_line_numbers walks a registry × a 14-document corpus (every syntax plus the malformed spelling of each: lone ![[, unterminated ^[, split wikilink, tilde/long/unclosed fences, CRLF, no trailing newline, multibyte, tables, nested tasks). The probe appends a sentinel to every line-prefix and checks its index — that tolerates the footnote step's legitimate trailing append, and means a step that deletes one line and inserts another cannot cancel out.
  • the_whole_preprocessing_pipeline_preserves_source_line_numbers runs the same corpus through the composition, since one step's output is the next one's input.
  • every_convert_markdown_preprocessing_step_is_registered extracts the calls in convert_markdown's body and asserts set equality with the registry, minus a two-name allowlist each carrying a written reason. Proven by adding a fifth unregistered transform: 101 passed / 1 failed. So "add a step, forget the list" is caught by CI, not by reviewer memory.

Counter-proof

Baseline master: 95 passed. Tests added first, transforms unmodified: 96 passed / 6 failed. Final: 105 passed / 0 failed.

Reverted Result
(?s) back on the embed regex 3 failed
block-id regex back to \s+ + hardcoded space 3 failed
footnote regex back to [^\]]+ 3 failed
the TASK_SOURCE_RE fail-safe removed 1 failed
all three transforms reverted 5 failed
a fifth unregistered transform added 1 failed
npm run check   432 files, 0 errors, 0 warnings
npm test        375 / 375
cargo test      105 / 105
cargo clippy    3 warnings — the exact baseline three

Known limits

  • The fail-safe is a backstop, not a proof. It only asks whether that raw line looks like a task item; a shift that maps one task line onto a different task line passes it. The contract tests are the real defence.
  • The sentinel probe detects displacement, not mangling. A step that keeps every line number while corrupting text within a line is invisible to it; the stray-embed test covers that case specifically, but only that case.
  • The corpus is finite — not a fuzz test. Adding an input alongside a new step is cheap and expected.
  • Behaviour changes worth a reviewer's sign-off: a wrapped ^[…] no longer becomes a footnote, and a stray ![[ no longer produces an <img>. Both stay literal, and both are documented at the regex.
  • \r is still matched by . in the embed regex — a pathological lone \r mid-line could land inside src/alt. Pre-existing and unrelated to line numbers; left alone.

🤖 Generated with Claude Code

@PathGao

PathGao commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator Author

Windows cargo test failure fixed — it was the new test reading its own source, not the transforms.

test tests::every_convert_markdown_preprocessing_step_is_registered ... FAILED
panicked at src\lib.rs:1419:47: convert_markdown must be terminated

There is no .gitattributes, so the Windows runner checks out with core.autocrlf=true and include_str!("lib.rs") hands the test CRLF source. The two \n-anchored needles then behave differently, which is what made it look odd:

  • the opening needle still matches — the \n half of the preceding \r\n supplies the leading newline, and the needle ends at { before the line ending;
  • the terminator "\n}\n" does not — under CRLF the closing line is \r\n}\r\n, so there is no \n immediately after }.

The test panicked before reaching its assertion. Fixed by normalising at the read: include_str!("lib.rs").replace("\r\n", "\n"). The assertion is about the shape of the source, not about how the working tree stores newlines.

Not adding .gitattributes: it would change checkout behaviour for every contributor and can trigger a whole-repo re-normalisation — a repo policy call, not a CI fix inside a PR under review. It would also only mask the fragility, leaving the needles one config flip away from red.

Reproduced on macOS by converting the file in place exactly as a Windows checkout would, which matched CI byte for byte (same test, same line, same message).

before after
LF 105 pass / 0 fail 105 / 0
CRLF 104 / 1 fail 105 / 0

Checked it isn't now passing vacuously: with the source in CRLF, renaming a registry entry made it fail with the drift assertion and the extracted list correctly named all four real steps — so body extraction genuinely works under CRLF.

Worth stating since it was the other hypothesis: the CRLF-document contract was never in question. The corpus entry with \r\n input is a compiled-in string literal, unaffected by file line endings, and it passed on Windows too. No corpus entry was touched.

@PathGao
PathGao force-pushed the fix/line-count-preserving-transforms branch from 334cd13 to bd73489 Compare August 2, 2026 21:41
`convert_markdown` preprocesses the raw buffer, renders the result with
`sourcepos = true`, and hands those line numbers to the frontend, which
writes task-checkbox toggles back into the RAW buffer at that number.
Every step therefore has to map input line N to output line N. Nothing
stated that, and three steps broke it:

- `INTERNAL_EMBED_RE` carried `(?s)`, so a stray `![[` paired with the
  `]]` of a real embed lines below and swallowed everything between.
  Measured: four lines eaten and the prose rewritten into an `<img>`
  attribute.
- `BLOCK_ID_RE` matched `\s+` before `^id`, swallowing the newline when
  the id sits on its own line and folding the anchor onto the previous
  one.
- `INLINE_FOOTNOTE_RE` used `[^\]]+`, which matches newlines, so a
  wrapped `^[...]` collapsed into a single line.

The embed regex drops `(?s)` rather than bailing out to literal after
matching: a runaway match consumes the well-formed embed inside it, so
the real image silently stops rendering either way. Not matching leaves
it free. The block-id regex captures its leading whitespace and re-emits
it verbatim, so the own-line form keeps working. The footnote regex
mirrors `HIGHLIGHT_RE`.

`annotate_task_checkboxes` is unchanged. It checks the RAW line on
purpose - it is the fail-safe for this contract, and the block-id case
is a live demonstration: the HTML claimed line 3 while raw line 3 was
blank. "Unifying" it would delete the guard and upgrade every future
regression from an inert checkbox to a corrupted document. That is now
written at the function.

The contract is no longer implicit: a registry test walks every step
against a 14-document corpus, and a second test extracts the calls in
`convert_markdown`'s body and asserts the registry lists all of them,
so a new step that forgets to register fails CI instead of review.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@PathGao

PathGao commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto 78e3158. The three red checks above were measured against a master that was itself red — the singleImplementationConvention meta-test went stale when #388 and #397 landed in that order, fixed in #398. Note Windows had already flipped to pass with the CRLF fix; the other three were collateral.

On the current base:

npm run check   432 files, 0 errors, 0 warnings
npm test        401 / 401
cargo test      105 / 105

@PathGao
PathGao force-pushed the fix/line-count-preserving-transforms branch from bd73489 to 609534f Compare August 2, 2026 22:10
@PathGao
PathGao merged commit d236fa8 into sftwrdotdev:master Aug 2, 2026
4 checks passed
@PathGao
PathGao deleted the fix/line-count-preserving-transforms branch August 2, 2026 22:26
PathGao pushed a commit that referenced this pull request Aug 3, 2026
`process_parenthesized_autolinks` ran one scan per `(<scheme>://`
candidate, walking forward for the balancing `)` and stopping at the
first whitespace. That walk has no early exit when the run holds more
`(` than `)`, so in a whitespace-free run with no closing paren every
candidate walked to the end of the run: cost quadratic in the length of
the longest run, and this pass runs on every keystroke via
`convert_markdown`.

At a fixed 1 MB of input, 100-byte runs cost 12 ms and one 1 MB run cost
63 s. Ordinary prose is unaffected — a 2.5 MB document of normal
parenthesized links was already linear at 3.9 ms — so this is reachable
by a generated or hostile `.md`, not by writing one.

The pairing now comes from a single left-to-right pass. Each candidate
is pushed with the nesting depth its `)` has to bring the run back to,
so the `)` that pops it is by construction the first one at which the
depth relative to that `(` returns to zero — the same `)` the old scan
found. Whitespace clears the pending candidates, which is the same
boundary the old scan's `break` enforced.

Output is unchanged, and `mod tests` now says so mechanically: the old
scan is kept verbatim as the reference and asserted byte-identical
(`Cow` variant included) over 49 adversarial inputs, the line-contract
corpus in both LF and CRLF, and every arrangement of parens, schemes,
whitespace and backticks up to five pieces long. The same comparison
over 2.2M inputs offline — every `.md` in the repo, the render fixtures,
the math corpus, exhaustive enumeration and 2M random documents — found
no difference either.

#290 stays fixed and the line-number contract of #389 is untouched: every
rewrite is still confined to the bytes between a `(` and its `)`, neither
of which can be a newline.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.

1 participant