fix(markdown): keep every preprocessing step on the same source line - #389
Conversation
|
Windows There is no
The test panicked before reaching its assertion. Fixed by normalising at the read: Not adding 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).
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 |
334cd13 to
bd73489
Compare
`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>
|
Rebased onto On the current base: |
bd73489 to
609534f
Compare
`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>
The contract nobody wrote down
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:
process_parenthesized_autolinksprocess_internal_embedsmoved line 5 to line 9. A stray![[paired with the]]of a real embed lines below: prose rewritten into an<img>attribute, four lines eatenprocess_wikilinks— wikilinksif full.as_str().contains('\n') { return literal }, now pinned by the corpusprocess_wikilinks— block ids(?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 blankprocess_wikilinks— highlights[^=\n]+excludes newlineprocess_wikilinks— inline footnotes[^\]]+matches newlines; a wrapped^[…]collapsed two lines into one and the task below lost itsdata-task-checkboxprocess_wikilinks— appended[^ifn-N]:definitionsprotect_display_math_underscores$$…$$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]+, mirroringHIGHLIGHT_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_checkboxesis unchanged, and now says whyIt 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_disagreepins 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_numberswalks 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_numbersruns the same corpus through the composition, since one step's output is the next one's input.every_convert_markdown_preprocessing_step_is_registeredextracts the calls inconvert_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.(?s)back on the embed regex\s++ hardcoded space[^\]]+TASK_SOURCE_REfail-safe removedKnown limits
^[…]no longer becomes a footnote, and a stray![[no longer produces an<img>. Both stay literal, and both are documented at the regex.\ris still matched by.in the embed regex — a pathological lone\rmid-line could land insidesrc/alt. Pre-existing and unrelated to line numbers; left alone.🤖 Generated with Claude Code