amadan
Fix what the branch review found: the Markdown fix was incomplete
The whole-branch review by Fable, per AGENTS.md's highest-capability
rule, found one blocking defect and two instances of this repo's own
recurring shape. All are fixed; gate green, drive 44/44.
## BLOCKING — the injection fix let indented markers through
74320cb's title claims "a newline in a run can no longer inject
Markdown into an export". It could. The fix taught escapeMarkdown where
a LINE begins, and then asked `i == lineStart` — which is the first
CHARACTER on a line, not the first non-space. CommonMark allows a block
marker up to three spaces of indentation and all of them interrupt a
paragraph, so every one of these still injected:
"intro\n # one space then hash" -> a real heading
"intro\n # three spaces then hash" -> a real heading
"intro\n - one space then bullet" -> a real bullet
"intro\n 1. one space then list" -> a real ordered list
And two characters were never escaped in any position at all:
"intro\n===" -> a setext H1, which promotes the
paragraph ABOVE it
"intro\n~~~\nswallowed" -> a code fence that interrupts the
paragraph and then eats every following
line of the document
The fence one is the worst of them: it does not restyle a line, it
swallows the rest of the file.
So the state is now "have we seen anything but spaces on this line
yet". Any run of spaces counts as still-at-line-start rather than
counting to three, because over-escaping renders correctly and this
function's own header already made that trade. '=' and '~' are escaped
at the start of a line only, so "a = b" is untouched, and only the
first character of a run needs it — an escaped '=' means the line is
not a setext underline, so "\===" is the whole fix rather than three
backslashes buying what one bought.
I had written a fix that was true of the case I tested and not of the
case the same sentence describes, which is the failure this repo names
in AGENTS.md and which I described in the very commit that had it.
## Recorded rather than fixed: four spaces is an indented code block
No escaping can prevent it — the block is made by the whitespace, not
by any character a backslash goes in front of. The text survives
verbatim, which is the module's actual promise, but renders as code.
Fixing it means rewriting the author's own leading whitespace, which is
a different decision. TestMarkdownIndentedRunLinesBecomeCodeBlocks
records the behaviour so it stays a decision rather than becoming
somebody's discovery.
## Two checks that could not see their own failure
Both are the shape the review was asked to hunt for, and both are one
line.
"a query matching nothing says so" tested that SOME text appeared in
the status element. The palette writes two different strings there —
data-empty when nothing matched, data-failed when the fetch threw — so
the check passed just as well when the search was BROKEN and the person
was being told so. It compares against the element's own data-empty
now. Confirmed by making the empty case report the failure copy: it
goes red naming both strings.
The 320px checks measured against documentElement.clientWidth and never
asserted that it was 320. It reads 320 only because rastrillo's
vendored topbar emits a viewport meta — nothing in this app does. Lose
that on a module upgrade and Chrome lays out at 980px, clientWidth
reads 980, and both checks pass while measuring a desktop-width page.
The viewport is asserted now, and a mismatch says "this check measured
nothing" rather than reporting a comfortable pass. Confirmed by driving
at 360px.
## Also from the review
- list.html's locale caveat had the threshold and the symptom wrong.
Because the field gives way first and the button never shrinks, a
wide label eats the FIELD long before anything overflows: ~200px
leaves ~80px of field, ~270px a sliver, and only past ~280px does
anything cross the edge. So the first failure is an unreadable search
box, not a 1.4.10 breach. Stated as the sequence it is.
- caretChecks is wrapped in group() too. The toolbar section is still
inline and still aborts the run; named as a follow-up in the comment
rather than moved, because relocating a hundred lines of working
checks while fixing them is how a harness stops being trustworthy.
- 55a4ac0 said "twenty checks"; the palette group is 19. 19 + 2 + 23 =
44, which is what the drive prints.
Verified: seven new escaping cases red first and green after; the two
weakened checks confirmed red against deliberate breaks; full gate
green; drive 44/44.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WxucgadpWLE9XNWvfHBsnb
fixtures/browser-drive.mjs+42 −7internal/docs/views/list.html+14 −4internal/export/markdown.go+55 −14internal/export/markdown_test.go+74 −3
CI passed — run details
Intent
<task-notification> <task-id>a4189668f6d97492c</task-id> <tool-use-id>toolu_01PfxFc8WDvECkfV34U7ZVXn</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-docs/4831640d-80a1-45ea-828c-336ad36a8cca/tasks/a4189668f6d97492c.output</output-file> <status>completed</status> <summary>Agent "Fable whole-branch review" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>Worktree is clean. Here is the review. # Whole-branch review: `worktree-discussions-triage` (4 commits over `origin/main` 7507a33) Read as one diff. Go suite green (`./internal/...`, 157s on the docs package). Browser drive run independently on this branch: 44/44, exit 0. I did not run the full gate: nothing on the branch touches the Rust crate, the wasm artifact or the Yjs wire tests it exists for, and both halves it shares with this branch (Go build and tests) I ran directly. ## BLOCKING ### 1. The `escapeMarkdown` fix does not hold — the injection is still reachable through one to three leading spaces, and through `===` and `~~~` Executed against the branch's own `export.Markdown` (throwaway test, since deleted, in `internal/export/`). Input on the left is a single paragraph text run; right is the exported bytes: ``` "intro\n # one space then hash" → "intro\n # one space then hash\n" "intro\n # three spaces then hash"→ "intro\n # three spaces then hash\n" "intro\n - one space then bullet" → "intro\n - one space then bullet\n" "intro\n 1. one space then list" → "intro\n 1. one space then list\n" "intro\n===" → "intro\n===\n" "intro\n~~~\nswallowed" → "intro\n~~~\nswallowed\n" "~~~\nrun-start fence" → "~~~\nrun-start fence\n" ``` Every one of those is a real block in CommonMark: ATX headings, bullets and `1.` lists all allow up to three spaces of indentation, and all three interrupt a paragraph (ATX explicitly; a bullet when the item is non-empty; an ordered list when it starts at 1 — exactly the case the branch's test uses). `intro\n===` is a setext H1. `~~~` is a code fence that interrupts a paragraph and then swallows every following line of the document until a closing fence. (No CommonMark renderer is installed on this box and none is in `fixtures/node_modules`, so the "becomes a block" half rests on the spec — §4.2, §4.3, §4.5, §5.2/5.3 — rather than a rendered proof. The exported bytes above are measured.) Why the fix misses them: `writeInline` runs `splitSpace` on the whole run, which trims spaces only at the run's two ends. Spaces after an embedded newline stay inside `core`, so the escaper sees the space at `lineStart`, writes it, and then `#` at `lineStart+1` is "mid-line" and goes through unescaped. `=` and `~` are not in the escape set at all, in any position (`~~~` at the very start of a run is a pre-existing hole, but the commit title claims "a newline in a run can no longer inject Markdown", and it can). Test inputs to add: the seven above, plus `"intro\n \t# x"` if you decide tabs count (a tab is four columns so it becomes indented code, which is a different fidelity loss). Suggested shape of the fix, for whoever takes it: track "at line start or only whitespace since line start" (a `lineOnlySpace` flag reset at `\n`, cleared on the first non-space rune) instead of `i == lineStart`, and either add `=`/`~` to the always-escape set or escape them when they are the first non-space on a line. Not fixed here, per your instruction. Everything else I probed in the escaper is correct: `i` from `range` is always a rune start, `\n` is one byte so `lineStart = i+1` is a rune boundary, and `s[lineStart:i]` therefore never starts mid-rune — verified with `"é\n# after multibyte"` and `"日本\n1) after cjk"` (both escaped correctly). `\r\n` works (`\r` is trailing text, the line starts after `\n`). A run beginning with a newline works. `!atLineStart && startsWithDigits(...)` is redundant-but-safe since the slice is empty at line start anyway. ## Non-blocking, worth fixing ### 2. Third check that asserts something adjacent: "a query matching nothing says so" `fixtures/browser-drive.mjs`, the check reads `[docs-jumpto-status]` and passes on `text.length > 0`. The palette writes two different strings into that element: `data-empty` ("No documents match") and `data-failed` (the fetch-error copy, `jump-to.mjs` `search()` catch branch). So a regression where `/documents/search` returns 500 only when nothing matches leaves this check green while the palette is telling the person the search *broke*. In practice the run would still go red at the last check, because that catch branch also calls `console.error`, which `pageProblems` collects — so the suite catches it, but under the name "the page reported no JavaScript errors" rather than the one that claims to be testing this. Fix: compare against the element's own `data-empty` attribute. (The "stale message" check has the same shape but is fine as written: it asserts emptiness, which is the right question.) ### 3. The 320px reflow check never asserts the viewport is 320 `reflowChecks` compares everything against `document.documentElement.clientWidth` and puts `found.viewport` only in the failure detail. The value is 320 today only because rastrillo's vendored `ui/layouts/topbar.html` emits `<meta name="viewport" content="width=device-width">` — nothing in this app does (the app's views mention "viewport" only in comments). If that meta is ever lost on a module upgrade, Chrome's mobile emulation lays out at 980px, `clientWidth` reads 980, and both "does not scroll sideways at 320px" checks pass while measuring a desktop-width page. That is precisely the "cannot tell no from I can't see it" shape. One-line fix: add `found.viewport === 320` to the condition. ### 4. The button-label caveat in `list.html` describes the second failure mode, not the first With `flex: 1 1 6rem; min-inline-size: 0` on the field and `flex: 0 0 auto` on the button, the field collapses toward zero *before* the button overflows. The comment says a label "wider than the 320px column would overflow"; the actual sequence is: a ~200px label leaves an ~80px field, a ~270px label leaves a sliver, and only past ~280px (288 column minus the 8px gap) does anything cross the edge. Not a 1.4.10 failure, but the comment claims to have thought about the locale case and has the threshold and the symptom wrong. Same note applies to `[docs-create-form]`'s existing comment ("about 270px"). ### 5. Commit message claims that the code does not support (minor) - 55a4ac0 says "Twenty checks, not seventeen". The palette group is 19 (count the drive output between "Control-K opens the palette inside a document" and "Enter opens the chosen document"). 19 + 2 reflow + 23 existing = 44, which matches. - `group()`'s comment says an abort "becomes its own named failure instead" of a smaller total. It does become a named failure (and exit 1), but the total still shrinks; the run cannot be silently green, which is what matters. Only the two new groups are wrapped — a throw anywhere in the pre-existing body of `run()` (toolbar, caret sections) still ends the run before either group, reported by the outer catch as "the drive ran to completion" but with the palette and reflow checks absent. Worth wrapping those sections too at some point. ## Checked and found correct - **CSS fix is real, not moved.** Measured: both pages have zero elements crossing the 320px edge and `scrollWidth == clientWidth`. Framework side verified against the pinned `rastrillo@v0.22.0` `ui/tokens.css`: `[rst-input]` sets `width: 100%` (flex-basis wins on the main axis; our two-attribute selector outranks its one; no `!important` anywhere in the file); `[rst-btn]` is `inline-flex` with no `white-space` or `flex-shrink` of its own; `[rst-page-header]` is `flex-wrap: wrap` with `space-between`; `[rst-page]` pads `--rst-sp-4` per side, so the 288px column arithmetic is right. The reasoning that the create form was a casualty of the search form's min-content floor (a wrapping flex container is at least as wide as its widest item's min-content) is correct. - **NFC is orthogonal to accent folding.** NFC never drops a combining mark, so `cafe` cannot match `café` — the test asserts this directly. Its singleton decompositions (U+2126→Ω, U+212B→Å, U+2000/2001→U+2002/2003, U+1FEF→backtick, U+037E→;) never produce or consume anything the stripping loop acts on except spaces, which are spaces on both sides. NFC≠NFKC, so ligatures and compatibility forms are untouched. Applying it inside `normaliseForSearch` guarantees both sides agree. Go's `ToLower` is a simple mapping, so no case mapping re-decomposes. `go.mod` promotion to direct is right; `go.sum` already had it. - **`group()`** does what it says: a throw in one group is caught, reported as a FAIL line, and the next group runs. Confirmed by reading `ws.onopen`: the summary counts `checks` and exits 1 on any failure. - **Derived-query logic (`multi`)**: the list page renders exactly one `/d/` anchor per row (`list.html` `[rst-row-main] a`), so a letter counted in two titles is two documents. A decomposed-accent title could make the JS count a title the server's NFC form would not match, but that errs toward a red "Down moves" check, not a false green. - **"the exact title ranks first"**: querying a whole title reaches `rankExact` in `rankTitle` (equality after normalisation, em dash collapsed on both sides), so first-row identity is guaranteed, not tie-broken. The debounce (120ms) is longer than `typeText`'s 20ms gap, so only the final query fires. - **"a search URL renders the matching document" / "hides the ones that do not match"**: genuine. The query is echoed only into the input's `value` attribute (excluded from `innerText`), and the no-results copy (`docs.list.search.none.*`) does not interpolate the query, so an empty result page cannot contain the title. `hidden.length > 0` is the correct relaxation for titles that legitimately share substrings. - **"reopening does not repeat the last empty-result message" / "reopens with an empty field"**: `show()` calls `setStatus('')` and `input.value = ''` — the checks read exactly those. - **Focus-return and Up/Down rewrites** are sound: a named origin element and a guard on Down having moved. - **`jsonString` via `encoding/json`**: escapes `<>&` as `\u003c` etc., which parses back to the same string; expected values compare export output, not JSON. - **The block-prefix test** (`TestMarkdownKeepsALiteralNewlineInsideItsBlock`) covers a real path: `writeList` re-indents continuation lines and the blockquote case re-prefixes them; the newline in a run reaches both through `inlineString`. ## Bottom line One blocking defect: the branch's headline fix in commit 74320cb is incomplete in a way that is reachable with the same inputs the commit itself names, and its title makes a claim the code does not meet. Items 2 and 3 are the repo's recurring pattern and are each a one-line change; I would fold them into the same fix round. Nothing else on the branch is wrong.</result> <usage><subagent_tokens>133326</subagent_tokens><tool_uses>38</tool_uses><duration_ms>663592</duration_ms></usage> </task-notification>
Attributed to this commit: 83.4 g CO₂e (10233410 tokens). This is an attribution, not a measurement — usage counted from this commit's own marker forward to the next one on worktree-discussions-triage, never backward from it. A turn that produced more than one commit books all of its tokens to the last one, and a concurrent session on this branch can add to the same figure. Some of it was priced using the shared default rate, because no model-specific factor row exists yet for at least one model involved. The Carbon view has the repo total, which is never derived from this attribution.
diff --git a/fixtures/browser-drive.mjs b/fixtures/browser-drive.mjs| index d59cf73..0c107e7 100644 |
| --- a/fixtures/browser-drive.mjs |
| +++ b/fixtures/browser-drive.mjs |
| @@ -403,8 +403,6 @@ async function run() { |
| check('undo reverses the last change', false, err.message) |
| } |
| - await caretChecks() |
| - |
| // Each of these is wrapped so that a failure inside one cannot take |
| // the checks after it down with it. Proven necessary rather than |
| // assumed: breaking the palette's Control-K binding threw on the |
| @@ -414,6 +412,16 @@ async function run() { |
| // silently removes other checks when it fails is the same "cannot |
| // tell no from I can't see it" shape this file exists to avoid, so |
| // an aborted group is now itself a failed check that names the group. |
| + // |
| + // NOT yet true of the toolbar section above, which is inline in this |
| + // function and throws deliberately in two places ("could not select |
| + // a word to format"). Those aborts are correct in themselves — there |
| + // is no point continuing without a selection — but they still cost |
| + // every group below, and the fix is to extract that section the way |
| + // these three are extracted. Left as a follow-up rather than done |
| + // here, because moving a hundred lines of working checks at the same |
| + // time as fixing them is how a harness stops being trustworthy. |
| + await group('the caret-placement checks', caretChecks) |
| await group('the jump-to palette checks', paletteChecks) |
| await group('the 320px reflow checks', reflowChecks) |
| @@ -644,16 +652,31 @@ async function paletteChecks() { |
| } |
| // A query matching nothing says so rather than showing an empty box. |
| + // |
| + // Compared against the status element's own data-empty, NOT merely |
| + // "some text appeared". The palette writes two different strings |
| + // into that element — data-empty when nothing matched, data-failed |
| + // when the fetch threw — so "text is non-empty" passes just as well |
| + // when the search is BROKEN and the person is being told so. That is |
| + // the wrong answer under a check whose name claims to be about the |
| + // empty case. |
| await pressKey('k', { code: 'KeyK', vk: 75, modifiers: CONTROL }) |
| await sleep(300) |
| await typeText('zzzznothingmatchesthis') |
| try { |
| - await settleOn( |
| + const said = await settleOn( |
| 'the empty-result message', |
| () => evaluate(`(()=>{const s=document.querySelector('[docs-jumpto-status]');return s.hidden?'':s.textContent})()`), |
| (t) => t.length > 0, |
| ) |
| - check('a query matching nothing says so', true) |
| + const expected = await evaluate( |
| + `document.querySelector('[docs-jumpto-status]').getAttribute('data-empty')`, |
| + ) |
| + check( |
| + 'a query matching nothing says so', |
| + said.trim() === (expected || '').trim(), |
| + `it says ${JSON.stringify(said)}; the empty-result copy is ${JSON.stringify(expected)}`, |
| + ) |
| } catch (err) { |
| check('a query matching nothing says so', false, err.message) |
| } |
| @@ -757,11 +780,23 @@ async function reflowChecks() { |
| over.sort((a, b) => b.d - a.d) |
| return { scrollWidth: document.documentElement.scrollWidth, viewport: vw, worst: over.slice(0, 3) } |
| })()`) |
| + // The viewport is asserted to BE 320, not merely used as the |
| + // yardstick. Everything here measures against |
| + // documentElement.clientWidth, and that reads 320 only because |
| + // rastrillo's vendored topbar layout emits |
| + // <meta name="viewport" content="width=device-width"> — nothing |
| + // in this app does. Lose that meta on a module upgrade and |
| + // Chrome's mobile emulation lays out at 980px, clientWidth reads |
| + // 980, and both of these checks pass while measuring a |
| + // desktop-width page: a green result that has stopped looking at |
| + // the thing it is named for. |
| check( |
| `${name} does not scroll sideways at 320px`, |
| - found.scrollWidth <= found.viewport && found.worst.length === 0, |
| - `scrollWidth ${found.scrollWidth} vs viewport ${found.viewport}; ` + |
| - found.worst.map((o) => `${o.what} ends at ${o.right}`).join(', '), |
| + found.viewport === 320 && found.scrollWidth <= found.viewport && found.worst.length === 0, |
| + found.viewport !== 320 |
| + ? `the page laid out at ${found.viewport}px, not 320 — check the viewport meta tag; this check measured nothing` |
| + : `scrollWidth ${found.scrollWidth} vs viewport ${found.viewport}; ` + |
| + found.worst.map((o) => `${o.what} ends at ${o.right}`).join(', '), |
| ) |
| } |
| } finally { |
diff --git a/internal/docs/views/list.html b/internal/docs/views/list.html| index 40b60ed..929e38a 100644 |
| --- a/internal/docs/views/list.html |
| +++ b/internal/docs/views/list.html |
| @@ -234,10 +234,20 @@ |
| flex: 0 0 auto for the same reason: the whole shortfall comes out |
| of the field, so the button keeps its whole label. It is the |
| words that are load-bearing here, not the box. |
| - The same locale caveat [docs-create-form] carries applies here — |
| - a catalog whose docs.list.search.label is wider than the 320px |
| - column would overflow rather than wrap, and the fix then is a |
| - wrap allowance keyed to the label, not a smaller field. */ |
| + The locale caveat, stated as the sequence it actually is rather |
| + than as a single threshold. Because the field gives way first and |
| + the button never shrinks, a wider label eats the FIELD long |
| + before anything overflows: a ~200px label leaves about 80px of |
| + field, ~270px leaves a sliver, and only past ~280px — the 288px |
| + column less the gap — does anything cross the edge. So the first |
| + failure a wide catalog produces is a search box too narrow to |
| + read what you typed, which is a usability problem and not a |
| + 1.4.10 one; the overflow comes later. English is 131px and is the |
| + only catalog today. The fix when it arrives is a wrap allowance |
| + keyed to the label, not a smaller field. |
| + ([docs-create-form]'s own comment above states the same caveat as |
| + a bare "wider than about 270px overflows", which is the second |
| + half of this sequence and not the part that bites first.) */ |
| [docs-search-form] button { |
| white-space: nowrap; |
| flex: 0 0 auto; |
diff --git a/internal/export/markdown.go b/internal/export/markdown.go| index 83417fa..d89e7f8 100644 |
| --- a/internal/export/markdown.go |
| +++ b/internal/export/markdown.go |
| @@ -302,34 +302,74 @@ func codeSpan(s string) string { |
| // over-escaped file renders correctly and reads slightly noisily in |
| // source, while an under-escaped one renders as something the author |
| // did not write. |
| -// "The start of a line" is tracked rather than inferred from index 0. |
| +// "The start of a line" is tracked rather than inferred from index 0, |
| +// and it means the first NON-SPACE on a line rather than the first |
| +// character. |
| // |
| -// It used to ask whether i == 0, which is the start of the RUN and not |
| +// Two rounds of this were wrong, in the same direction, and both are |
| +// worth naming because the second looked like a fix. |
| +// |
| +// It first asked whether i == 0, which is the start of the RUN and not |
| // the start of a line. A run can contain a literal newline — nothing |
| // stops one: POST /d/{sqid}/updates accepts Yjs bytes that never met |
| -// ProseMirror's schema, and a paste can carry one — and everything |
| -// after that newline was then treated as mid-sentence. So "#" became a |
| -// heading, "- " became a bullet and "1." became an ordered list, in the |
| -// middle of somebody's paragraph. An exported document said something |
| -// its author did not write, which is the one failure this module is |
| -// not allowed to have. |
| +// ProseMirror's schema, and a paste can carry one — so everything |
| +// after a newline was treated as mid-sentence, and "#" became a |
| +// heading, "- " a bullet, "1." an ordered list, mid-paragraph. |
| +// |
| +// Tracking lineStart fixed exactly the unindented case and no more. |
| +// CommonMark lets a block marker carry up to THREE spaces of |
| +// indentation (§4.2 for ATX headings, §5.2 for lists) and all of them |
| +// interrupt a paragraph, so " # x" one space in is still a heading |
| +// while `i == lineStart` says it is mid-line. Caught in review, with |
| +// the seven inputs now in the test table. |
| +// |
| +// So the state is "have we seen anything but spaces on this line yet", |
| +// which is what atLineStart means below. Any run of spaces is treated |
| +// as still-at-line-start rather than counting to three: over-escaping |
| +// renders correctly and reads slightly noisily in source, which is the |
| +// trade this function's own header already chose. |
| +// |
| +// Two characters are escaped that a first reading would not expect: |
| // |
| -// lineStart is what makes the two questions the same question again. |
| +// - '=' because a line of them is a setext heading, which underlines |
| +// the paragraph ABOVE it — so it silently promotes text that is |
| +// nowhere near it. ('-' is already escaped for the bullet case, |
| +// which covers setext H2 as a side effect.) |
| +// - '~' because "~~~" opens a code fence that interrupts a paragraph |
| +// and then swallows every following line until a closing fence. It |
| +// does not restyle a line, it eats the rest of the document. |
| +// |
| +// Both only at the start of a line, so "a = b" is left alone. |
| +// |
| +// KNOWN AND NOT FIXED HERE: four or more leading spaces after a |
| +// newline make an indented code block, and no amount of escaping |
| +// changes that — the block is made by the whitespace, not by any |
| +// character this function could put a backslash in front of. The text |
| +// survives verbatim and is rendered as code. Fixing it means altering |
| +// the author's own leading whitespace, which is a different decision |
| +// from escaping and not one to take in passing. |
| +// TestMarkdownIndentedRunLinesBecomeCodeBlocks records the current |
| +// behaviour so it stays a decision rather than becoming a discovery. |
| func escapeMarkdown(s string) string { |
| var b strings.Builder |
| b.Grow(len(s)) |
| - lineStart := 0 // byte index in s where the current line begins |
| + lineStart := 0 // byte index where the current line begins |
| + atLineStart := true // nothing but spaces seen on this line yet |
| for i, r := range s { |
| - atLineStart := i == lineStart |
| switch r { |
| case '\n': |
| b.WriteRune(r) |
| lineStart = i + 1 |
| + atLineStart = true |
| + continue |
| + case ' ', '\t': |
| + // Indentation does not end the line's start. |
| + b.WriteRune(r) |
| continue |
| case '\\', '`', '*', '_', '[', ']', '<', '>': |
| b.WriteByte('\\') |
| b.WriteRune(r) |
| - case '#', '+', '-': |
| + case '#', '+', '-', '=', '~': |
| // Only special at the start of a line. Inside a sentence |
| // they are ordinary punctuation, and escaping every hyphen |
| // in a document would be unreadable. |
| @@ -340,14 +380,15 @@ func escapeMarkdown(s string) string { |
| case '.', ')': |
| // "1." and "1)" start an ordered list, but only after |
| // digits at the start of a line — so the digits are counted |
| - // from the line, not from the run. |
| - if !atLineStart && startsWithDigits(s[lineStart:i]) { |
| + // from the line's first non-space, not from the run. |
| + if !atLineStart && startsWithDigits(strings.TrimLeft(s[lineStart:i], " \t")) { |
| b.WriteByte('\\') |
| } |
| b.WriteRune(r) |
| default: |
| b.WriteRune(r) |
| } |
| + atLineStart = false |
| } |
| return b.String() |
| } |
diff --git a/internal/export/markdown_test.go b/internal/export/markdown_test.go| index 6c66934..3e185d0 100644 |
| --- a/internal/export/markdown_test.go |
| +++ b/internal/export/markdown_test.go |
| @@ -140,11 +140,51 @@ func TestMarkdownEscapesTextThatWouldBecomeMarkup(t *testing.T) { |
| {"intro\n+ not a bullet", "intro\n\\+ not a bullet"}, |
| {"intro\n1. not a list", "intro\n1\\. not a list"}, |
| {"intro\n1) not a list", "intro\n1\\) not a list"}, |
| - // And the flip side: the same characters mid-line after a |
| - // newline are still ordinary punctuation, so the fix must not |
| - // escape its way out of the problem. |
| + |
| + // CommonMark allows a block marker up to THREE spaces of |
| + // indentation (§4.2 ATX headings, §5.2 lists), and all of them |
| + // interrupt a paragraph. So "at the start of a line" has to |
| + // mean "first non-space on a line", not "first character on a |
| + // line" — the first version of this fix escaped only the |
| + // latter, and every one of these went through untouched. |
| + {"intro\n # one space", "intro\n \\# one space"}, |
| + {"intro\n # three spaces", "intro\n \\# three spaces"}, |
| + {"intro\n - one space", "intro\n \\- one space"}, |
| + {"intro\n 1. one space", "intro\n 1\\. one space"}, |
| + |
| + // A setext heading underlines the line ABOVE it, so a run of |
| + // "=" on its own line silently promotes the preceding |
| + // paragraph to an H1. "-" was already escaped for the bullet |
| + // case, which covers setext H2 as a side effect. |
| + // |
| + // Only the FIRST character needs the backslash, and that is |
| + // the whole escape rather than a partial one: a setext |
| + // underline must be a line of nothing but "=", and a line |
| + // beginning with an escape is not that. Escaping all three |
| + // would be three backslashes to buy what one already bought. |
| + {"intro\n===", "intro\n\\==="}, |
| + |
| + // "~~~" opens a code fence that interrupts a paragraph and |
| + // then swallows every following line until a closing fence — |
| + // so this one does not merely restyle a line, it eats the rest |
| + // of the document. Same first-character-only reasoning: an |
| + // escaped tilde is not a fence opener. |
| + // |
| + // Escaped at the start of a line in any position, including |
| + // the start of the run, which was a hole even before newlines |
| + // were considered. |
| + {"intro\n~~~\nswallowed", "intro\n\\~~~\nswallowed"}, |
| + {"~~~ at the run start", "\\~~~ at the run start"}, |
| + |
| + // And the flip side: the same characters mid-line, after a |
| + // newline or not, are ordinary punctuation. A fix that escapes |
| + // its way out of the problem would make every hyphen and every |
| + // equals sign in every export unreadable, and would have |
| + // passed a test that only checked the bad cases. |
| {"intro\nwell-known", "intro\nwell-known"}, |
| {"intro\nversion 1.2", "intro\nversion 1.2"}, |
| + {"a = b and c ~ d", "a = b and c ~ d"}, |
| + {"intro\na = b", "intro\na = b"}, |
| } |
| for _, c := range cases { |
| js := `{"type":"doc","content":[{"type":"paragraph","content":[{"type":"text","text":` + jsonString(c.text) + `}]}]}` |
| @@ -390,3 +430,34 @@ func TestMarkdownKeepsALiteralNewlineInsideItsBlock(t *testing.T) { |
| t.Errorf("got:\n%q\nwant:\n%q", got, want) |
| } |
| } |
| + |
| +// Four or more leading spaces on a line make an indented code block, |
| +// and escaping cannot prevent it: the block is made by the whitespace, |
| +// not by any character a backslash could go in front of. |
| +// |
| +// So this records what currently happens rather than asserting what |
| +// should. The text survives verbatim — which is the module's actual |
| +// promise, never lose the words — but a reader sees it as code. The |
| +// fix, if it is ever wanted, means rewriting the author's own leading |
| +// whitespace, and that is a different decision from escaping, not one |
| +// to take in passing. |
| +// |
| +// Here so the next person to find it finds a decision rather than a |
| +// surprise, and so anyone who changes the behaviour has to change a |
| +// test that says why. |
| +func TestMarkdownIndentedRunLinesBecomeCodeBlocks(t *testing.T) { |
| + js := `{"type":"doc","content":[{"type":"paragraph","content":[ |
| + {"type":"text","text":"intro\n four spaces in"} |
| + ]}]}` |
| + got := strings.TrimSpace(export.Markdown(mustParse(t, "", js))) |
| + want := "intro\n four spaces in" |
| + if got != want { |
| + t.Errorf("got %q, want %q", got, want) |
| + } |
| + // The words are all still there, which is the guarantee that does |
| + // hold. Asserted separately so a change to the spacing still has |
| + // to keep this true. |
| + if !strings.Contains(got, "four spaces in") { |
| + t.Errorf("the text was lost: %q", got) |
| + } |
| +} |