Check in the CommonMark spec suite and pin its exception list #58

Open

lilleman wants to merge 6 commits from commonmark-spec-suite into main

pull from: commonmark-spec-suite

merge into: larvit:main

larvit:main

Conversation 2 Commits 6 Files Changed 9

+6371 -12

lilleman commented 2026-09-05 18:56:38 +02:00

Owner

Copy Link

Copy Source

Check in the CommonMark 0.31.2 spec suite as corpus data and run it against markdownToAdf with three checks (canonical fixpoint, reference-HTML text, element counts), gated by a maintainer-curated exception list.

64 exceptions: 6 fixpoint (the §2 holes the flavour has no escape for), 5 text, 53 count (the mark model nests marks per text node where CommonMark nests HTML elements). The suite is vendored, not a dependency (AGENTS.md §5); the spec examples are CC-BY-SA-4.0.

Check in the CommonMark 0.31.2 spec suite as corpus data and run it against `markdownToAdf` with three checks (canonical fixpoint, reference-HTML text, element counts), gated by a maintainer-curated exception list. 64 exceptions: 6 fixpoint (the §2 holes the flavour has no escape for), 5 text, 53 count (the mark model nests marks per text node where CommonMark nests HTML elements). The suite is vendored, not a dependency (AGENTS.md §5); the spec examples are CC-BY-SA-4.0.

lilleman added 1 commit 2026-09-05 18:56:39 +02:00

Check in the CommonMark spec suite and pin its exception list e6610d7057

lilleman added 1 commit 2026-09-06 21:59:00 +02:00

Pin the suite run, gate the exception list, and trim the corpus README 12b57a06a9

lilleman commented 2026-09-06 22:24:50 +02:00

Author

Owner

Copy Link

Copy Source

Stability review

The three-check design is sound, the vendoring rationale holds up (spec.json sha256 d431b29d…c1b2a20 matches spec.commonmark.org/0.31.2/spec.json byte for byte; commonmark-spec@0.31.2 from npm is indeed CommonJS-only, require/exports, no type: "module"), and the exception list is a real pin rather than a mute in the directions it covers — adding an entry for a passing example fails twice, an entry naming no example fails, a duplicate (example, check) fails. No index drift: everything is keyed on the spec's own example field. No CI cost — 1277 tests in 878 ms on Node, 432 ms on Bun, ~1 s on Deno; green on all three plus the Firefox leg.

Positive control confirms the suite adds real signal: corrupting code-span text in the parser produces 2 CommonMark failures the entire hand corpus misses.

Findings below are from fault injection against the branch, each reverted.

🔴 1. The refusal path asserts nothing — 86/652 examples are checked by nothing

src/commonmark-spec.test.ts:257-261

const parse = markdownToAdf(example.markdown)
if (!parse.ok) {
  assert.equal(exceptionIndex.get(`${example.example}:fixpoint`), undefined, …)
  return
}

The early return makes no assertion about the conversion. It checks only that no fixpoint exception is filed — and none is filed for any refusing example, so it is vacuously true for all of them, and stays vacuously true for any example that starts refusing. 86 of 652 examples take this path today (72 unmappable-html, 14 unmappable-image). The suite advertises 652 examples under three checks; 566 get them.

• Injecting a refusal for every non-empty input leaves 646 of 652 examples passing — only the 6 carrying a fixpoint exception fail. This is exactly the vacuity todo.md 3k says the three checks exist to prevent ("The fixpoint alone is self-consistency a parser returning the empty document passes…"), reintroduced one level up: a parser returning an error skips all three.
• A realistic-shaped regression — refusing any input containing foo, 391 examples, 334 previously converting — produces no failing test for 339 of them.

It also contradicts the settled definition at todo.md:59-60: "§2's canonical fixpoint: a named error, or markdown that parses and emits to itself byte for byte." The implementation names none of the 86.

Smallest fix: treat a refusal as a fourth outcome that must be named — an errors map of example → code compared as one deepEqual, so both an example that starts refusing and one that stops are failures, and the code is pinned too. Cheaper than 86 more exception entries.

🟠 2. An exception pins that an example diverges, never how

src/commonmark-spec.test.ts:275 and :286 both assert only typeof … === 'string'. All three checks compute the exact divergence — countMismatch returns em 2/1, fixpointRefused returns unspellable-link, textMismatch returns both strings — and every caller discards it. reason is free text compared to nothing.

Two demonstrations:

• Patching markdownToAdf so [](./target.md) returns a paragraph reading literally TOTAL GARBAGE — arbitrarily wrong output for example 484 — leaves all 718 tests passing. The same mutation on a non-excepted example ([link](/uri "title")) correctly fails 2 tests. So the suite is a genuine gate for the 588 unexcepted examples and blind for the 64 excepted example/check pairs.
• Forcing markType to always return 'strong', so every em becomes strong — a catastrophic regression to precisely the mark-collapse logic these exceptions record — leaves all 53 count-excepted examples green and 0 of the 64 pin tests failing. Those 53 are the suite's only coverage of the *(*a*)* carve-out, so it gives zero regression protection for the rule most likely to regress.

Fix: put the divergence in the entry ("divergence": "em 2/1", the error code for fixpoint) and compare it. 53 short strings, and each entry becomes self-checking.

🟠 3. Two text exceptions record a defect in the test's own oracle, not in the library

referenceText and concatenatedText disagree structurally on whitespace, and in both cases the parser output is correct:

• 39 (foo

bar): reference HTML is <p>foo\n\nbar</p> — CommonMark keeps the newlines literal — and the parser yields exactly "foo\n\nbar". referenceText collapses \n\n to one space, producing the mismatch. The filed reason ("CommonMark reads it a soft break") is not what the reference HTML shows.
• 300: the parser yields heading + paragraph in the correct list item, matching <h2>Bar</h2>\nbaz. referenceText inserts a space at that boundary; concatenatedText joins every text node with '' and inserts no block separator anywhere. The whole divergence is that one space.

Both entries disappear if the two flatteners agree on block separation. As filed they read as library defects and will be chased as such.

🟠 4. The text oracle shares its entity decoder with the code under test

src/commonmark-spec.test.ts:8 imports readEntityReference from ./markdown/entity-references.ts — the same function the parse path uses at src/markdown/commonmark-grammar.ts:111. A bug in it corrupts both sides of the comparison identically and the check still passes. The reference HTML is meant to be the independent oracle; decoding it with the library's own decoder makes the text check blind to exactly the entity handling it appears to cover.

🟡 5. The list conflates categories that deserve different fates

Nothing in the schema separates "accepted by design, will never change" from "known bug, not yet fixed". Grouping the 64:

• 48 count — the mark-model difference todo.md 3k anticipates. Permanent.
• 2 count (301, 302) — 3h's list-marker-change behaviour. Anticipated.
• 2 count + 2 text (484, 487) — [](./target.md) and []() stay literal text; CommonMark renders an empty link. Unimplemented behaviour.
• 1 count + 1 text (540) — [ẞ] against [SS]: /url. Narrow: ASCII case folding works (539, 541, 555, 561 all pass unexcepted), only Unicode full folding (ẞ→ss) fails. The reason reads as though case folding fails generally.
• 2 text (39, 300) — finding 3.

484/487 are parser gaps carrying the same weight as 48 entries that will never move. A kind field keeps the second group visible.

🟡 6. Heading level is invisible to the suite

src/commonmark-spec.test.ts:74, 111-112 collapse h1–h6 and every heading node into one bucket. Injecting level 6 → 5 in the parser produces zero CommonMark failures — caught only by the pre-existing commonmark-subset/heading fixture and one unit test. Keying on h${level} on both sides costs one template literal.

🟡 7. AGENTS.md:205 is now false

browser-tests/run.js:75 excludes the new kind, while §10 still says the browser leg "converts the whole corpus". A reader concludes the spec suite is proved on SpiderMonkey; it is not — the only non-V8, non-JSC engine never sees it. Per §12 (a false claim in a doc is a bug, fixed where found) this belongs in the diff: name the three kinds it does convert, or drop "whole".

🟡 8. Two decision records disagree, and the PR resolves it silently

todo.md:69-72, marked Settled (the maintainer, 2026-08-27): "One outcome is no exception and must not be filed as one: valid CommonMark parsing to a document adfToMarkdown refuses is a §2 hole." All six fixpoint entries (196, 202, 330, 331, 340, 502) are exactly that — they parse (:229 asserts parsed.ok) and the emit refuses (:231).

AGENTS.md:57, newer, says the opposite for one sub-case: "CommonMark spells link destinations the flavour has no escape for … todo.md 3k's exception list names those." That blesses 196/202/502 but not 330/331/340 (unspellable-line-start). AGENTS.md §2 also still points at todo.md for the list, which now lives at corpus/commonmark-spec/exceptions.json.

Per §15 the resolution belongs in the diff — strike the stale todo.md sentence or drop the six entries. As it stands the next reader of 3k treats all six as bugs to fix. todo.md 3k is also still unticked, though this PR implements it.

🟢 Minor

• src/commonmark-spec.test.ts:59-61 — the test is named "the pinned 0.31.2 run" but asserts only spec.length === 652, and 0.30 also has 652 examples. The real pin holds (swapping 0.30's spec.json in gives 79 failures), so this is naming, not a hole — but hashing the file would pin what corpus/README.md actually claims ("vendored byte-exact").
• :234 is the repo's only assert.deepStrictEqual; every other test file uses assert.deepEqual (8 in corpus.test.ts, 17 in directive-syntax.test.ts). Under node:assert/strict they are the same function — two spellings, one goal.
• :240, :248 — bare assert.ok(parsed.ok) with no message, while :229 has one. These fire from the pin loop when an excepted example stops parsing and report "The expression evaluated to a falsy value" with no example number.
• reason is never checked non-empty; "" satisfies every assertion.
• :67 cites bare 3e; AGENTS.md §14 already states the rule and is the stabler reference — 3e is a ticked item whose text moves to todo-history.md.
• corpus/README.md:15, 19-21 repeat "vendored" and "CommonMark 0.31.2"; only the licence and the gate carve-out are new information.

Questions

• todo.md:67 says "The exception list stays the maintainer's." Were these 64 curated by hand, or derived and labelled so? That decides whether finding 8 is the author's call to make.
• Branch coverage rises 98.15 → 98.25 with the floor left at 98. AGENTS.md §10 says the branch floor "only ever moves upward" — a mandate to ratchet, or only a ban on lowering? No regression either way; main already carried the slack.

## Stability review The three-check design is sound, the vendoring rationale holds up (`spec.json` sha256 `d431b29d…c1b2a20` matches `spec.commonmark.org/0.31.2/spec.json` byte for byte; `commonmark-spec@0.31.2` from npm is indeed CommonJS-only, `require`/`exports`, no `type: "module"`), and the exception list is a real pin rather than a mute in the directions it covers — adding an entry for a passing example fails twice, an entry naming no example fails, a duplicate `(example, check)` fails. No index drift: everything is keyed on the spec's own `example` field. No CI cost — 1277 tests in 878 ms on Node, 432 ms on Bun, ~1 s on Deno; green on all three plus the Firefox leg. Positive control confirms the suite adds real signal: corrupting code-span text in the parser produces 2 CommonMark failures the entire hand corpus misses. Findings below are from fault injection against the branch, each reverted. ### 🔴 1. The refusal path asserts nothing — 86/652 examples are checked by nothing `src/commonmark-spec.test.ts:257-261` ```ts const parse = markdownToAdf(example.markdown) if (!parse.ok) { assert.equal(exceptionIndex.get(`${example.example}:fixpoint`), undefined, …) return } ``` The early return makes no assertion about the conversion. It checks only that no `fixpoint` exception is filed — and none is filed for any refusing example, so it is vacuously true for all of them, and stays vacuously true for any example that *starts* refusing. **86 of 652 examples take this path today** (72 `unmappable-html`, 14 `unmappable-image`). The suite advertises 652 examples under three checks; 566 get them. - Injecting a refusal for every non-empty input leaves **646 of 652 examples passing** — only the 6 carrying a fixpoint exception fail. This is exactly the vacuity `todo.md` 3k says the three checks exist to prevent ("The fixpoint alone is self-consistency a parser returning the empty document passes…"), reintroduced one level up: a parser returning an *error* skips all three. - A realistic-shaped regression — refusing any input containing `foo`, 391 examples, 334 previously converting — produces **no failing test for 339 of them**. It also contradicts the settled definition at `todo.md:59-60`: "§2's canonical fixpoint: **a named error**, or markdown that parses and emits to itself byte for byte." The implementation names none of the 86. Smallest fix: treat a refusal as a fourth outcome that must be named — an `errors` map of `example → code` compared as one `deepEqual`, so both an example that starts refusing and one that stops are failures, and the code is pinned too. Cheaper than 86 more exception entries. ### 🟠 2. An exception pins *that* an example diverges, never *how* `src/commonmark-spec.test.ts:275` and `:286` both assert only `typeof … === 'string'`. All three checks compute the exact divergence — `countMismatch` returns `em 2/1`, `fixpointRefused` returns `unspellable-link`, `textMismatch` returns both strings — and every caller discards it. `reason` is free text compared to nothing. Two demonstrations: - Patching `markdownToAdf` so `[](./target.md)` returns a paragraph reading literally `TOTAL GARBAGE` — arbitrarily wrong output for example 484 — leaves **all 718 tests passing**. The same mutation on a non-excepted example (`[link](/uri "title")`) correctly fails 2 tests. So the suite is a genuine gate for the 588 unexcepted examples and blind for the 64 excepted example/check pairs. - Forcing `markType` to always return `'strong'`, so every `em` becomes `strong` — a catastrophic regression to precisely the mark-collapse logic these exceptions record — leaves **all 53 count-excepted examples green and 0 of the 64 pin tests failing**. Those 53 are the suite's only coverage of the `*(*a*)*` carve-out, so it gives zero regression protection for the rule most likely to regress. Fix: put the divergence in the entry (`"divergence": "em 2/1"`, the error code for fixpoint) and compare it. 53 short strings, and each entry becomes self-checking. ### 🟠 3. Two `text` exceptions record a defect in the test's own oracle, not in the library `referenceText` and `concatenatedText` disagree structurally on whitespace, and in both cases the parser output is correct: - **39** (`foo&#10;&#10;bar`): reference HTML is `<p>foo\n\nbar</p>` — CommonMark keeps the newlines literal — and the parser yields exactly `"foo\n\nbar"`. `referenceText` collapses `\n\n` to one space, producing the mismatch. The filed reason ("CommonMark reads it a soft break") is not what the reference HTML shows. - **300**: the parser yields heading + paragraph in the correct list item, matching `<h2>Bar</h2>\nbaz`. `referenceText` inserts a space at that boundary; `concatenatedText` joins every text node with `''` and inserts no block separator anywhere. The whole divergence is that one space. Both entries disappear if the two flatteners agree on block separation. As filed they read as library defects and will be chased as such. ### 🟠 4. The text oracle shares its entity decoder with the code under test `src/commonmark-spec.test.ts:8` imports `readEntityReference` from `./markdown/entity-references.ts` — the same function the parse path uses at `src/markdown/commonmark-grammar.ts:111`. A bug in it corrupts both sides of the comparison identically and the check still passes. The reference HTML is meant to be the independent oracle; decoding it with the library's own decoder makes the text check blind to exactly the entity handling it appears to cover. ### 🟡 5. The list conflates categories that deserve different fates Nothing in the schema separates "accepted by design, will never change" from "known bug, not yet fixed". Grouping the 64: - **48 count** — the mark-model difference `todo.md` 3k anticipates. Permanent. - **2 count (301, 302)** — 3h's list-marker-change behaviour. Anticipated. - **2 count + 2 text (484, 487)** — `[](./target.md)` and `[]()` stay literal text; CommonMark renders an empty link. Unimplemented behaviour. - **1 count + 1 text (540)** — `[ẞ]` against `[SS]: /url`. Narrow: ASCII case folding works (539, 541, 555, 561 all pass unexcepted), only Unicode *full* folding (ẞ→ss) fails. The reason reads as though case folding fails generally. - **2 text (39, 300)** — finding 3. 484/487 are parser gaps carrying the same weight as 48 entries that will never move. A `kind` field keeps the second group visible. ### 🟡 6. Heading level is invisible to the suite `src/commonmark-spec.test.ts:74, 111-112` collapse `h1`–`h6` and every `heading` node into one bucket. Injecting `level 6 → 5` in the parser produces **zero** CommonMark failures — caught only by the pre-existing `commonmark-subset/heading` fixture and one unit test. Keying on `h${level}` on both sides costs one template literal. ### 🟡 7. `AGENTS.md:205` is now false `browser-tests/run.js:75` excludes the new kind, while §10 still says the browser leg "converts the **whole corpus**". A reader concludes the spec suite is proved on SpiderMonkey; it is not — the only non-V8, non-JSC engine never sees it. Per §12 (a false claim in a doc is a bug, fixed where found) this belongs in the diff: name the three kinds it does convert, or drop "whole". ### 🟡 8. Two decision records disagree, and the PR resolves it silently `todo.md:69-72`, marked *Settled (the maintainer, 2026-08-27)*: "One outcome is no exception and must not be filed as one: valid CommonMark parsing to a document `adfToMarkdown` refuses is a §2 hole." All six `fixpoint` entries (196, 202, 330, 331, 340, 502) are exactly that — they parse (`:229` asserts `parsed.ok`) and the *emit* refuses (`:231`). `AGENTS.md:57`, newer, says the opposite for one sub-case: "CommonMark spells link destinations the flavour has no escape for … `todo.md` 3k's exception list names those." That blesses 196/202/502 but not 330/331/340 (`unspellable-line-start`). AGENTS.md §2 also still points at `todo.md` for the list, which now lives at `corpus/commonmark-spec/exceptions.json`. Per §15 the resolution belongs in the diff — strike the stale `todo.md` sentence or drop the six entries. As it stands the next reader of 3k treats all six as bugs to fix. `todo.md` 3k is also still unticked, though this PR implements it. ### 🟢 Minor - `src/commonmark-spec.test.ts:59-61` — the test is named "the pinned 0.31.2 run" but asserts only `spec.length === 652`, and **0.30 also has 652 examples**. The real pin holds (swapping 0.30's `spec.json` in gives 79 failures), so this is naming, not a hole — but hashing the file would pin what `corpus/README.md` actually claims ("vendored byte-exact"). - `:234` is the repo's only `assert.deepStrictEqual`; every other test file uses `assert.deepEqual` (8 in `corpus.test.ts`, 17 in `directive-syntax.test.ts`). Under `node:assert/strict` they are the same function — two spellings, one goal. - `:240, :248` — bare `assert.ok(parsed.ok)` with no message, while `:229` has one. These fire from the pin loop when an excepted example stops parsing and report "The expression evaluated to a falsy value" with no example number. - `reason` is never checked non-empty; `""` satisfies every assertion. - `:67` cites bare `3e`; AGENTS.md §14 already states the rule and is the stabler reference — `3e` is a ticked item whose text moves to `todo-history.md`. - `corpus/README.md:15, 19-21` repeat "vendored" and "CommonMark 0.31.2"; only the licence and the gate carve-out are new information. ### Questions - `todo.md:67` says "The exception list stays the maintainer's." Were these 64 curated by hand, or derived and labelled so? That decides whether finding 8 is the author's call to make. - Branch coverage rises 98.15 → 98.25 with the floor left at 98. AGENTS.md §10 says the branch floor "only ever moves upward" — a mandate to ratchet, or only a ban on lowering? No regression either way; `main` already carried the slack.

lilleman added 1 commit 2026-09-06 23:09:04 +02:00

Pin refusal codes and divergences; fix the text oracle and count by heading level dfbb92b7ab

lilleman added 1 commit 2026-09-06 23:15:18 +02:00

Cite §14 instead of the ticked 3e item in the count comment 6e3b7e692c

lilleman commented 2026-09-07 08:05:44 +02:00

Author

Owner

Copy Link

Copy Source

Stability review — second pass

Every finding from the first pass was re-verified by re-running the original fault injections, not by reading the diff. Seven of eight are genuinely closed, one was renamed rather than fixed and is downgraded to non-blocking. The suite is 717 tests green in ~0.7 s; Node, Deno 2.9.6 and Bun 1.4.0 all pass 1277/1277; coverage holds at 100 lines / 100 functions / 98.25 branches.

The fixes are real, not restructuring:

Previous finding	Then	Now
Refusal path asserted nothing	total parser death left 646/652 passing	642/717 fail; the 75 survivors are exactly the 3 meta tests plus the 72 unmappable-html pins matching the injected code — so the code is compared, not just the fact of refusal
Narrow refusal regression	339/391 silent	swapping unmappable-image→unmappable-html at all four sites fails exactly 14 tests, precisely the image pins
Exception pinned that, not how	markType→'strong' left 0 failures	113 failures (74 per-example, 39 pins); the garbage-output injection on example 484 that previously left all 718 green now fails 2
Heading level invisible	level 6→5 gave 0 failures	1 failure (example 62 — the suite holds exactly one <h6>)
Examples 39/300	oracle defect filed as library divergence	oracle repaired, exceptions deleted — 64→62 entries, removing exactly (39,'text') and (300,'text') and adding nothing
Self-confirming oracle	shared readEntityReference	independent namedEntity map; the import is gone
Category conflation	one flat list	kind: 48 mark-model, 6 unspellable, 8 pending
AGENTS.md §10, todo.md 3k	false / contradictory	both corrected in the diff

refusals.json pinning each of the 86 refusing examples to its code is stronger than the single deepEqual suggested, and closes both directions — an example that starts refusing and one that stops.

Three things the fix commits introduced, all fixed by deletion.

🔴 1. The numeric entity decoder cannot match — it is dead code that would also not work if reached

src/commonmark-spec.test.ts:171-188. reference is text.slice(index, end + 1), so it includes the trailing ; — while both numeric patterns anchor with $ immediately after the digits:

const decimal = /^&#(\d+)$/.exec(reference)          // reference is "
" — never matches
const hexadecimal = /^&#[xX]([0-9A-Fa-f]+)$/.exec(reference)

Exercised directly:

"&"   -> { length: 5, text: "&" }
"
"   -> undefined
"A"   -> undefined
"A"  -> undefined
"ö"  -> undefined

So characterOf is unreachable, and the comment's claim that "named and numeric cover a future re-pin" is the opposite of true: at that re-pin decodeHtmlEntity returns undefined and referenceText falls through to pushing &, #, 1, 0, ; as literal characters — a silently wrong oracle rather than an error. Keeping this is strictly worse than deleting it, because it reads as coverage that does not exist.

Nothing will ever catch it: --test-coverage-exclude="src/**/*.test.ts" puts the test file's own dead branches outside the 100% floor. AGENTS.md §11 — "no speculative generality — a second consumer, or it goes."

Fix: delete characterOf, both numeric branches and the unused apos entry. decodeHtmlEntity collapses to the lookup plus an early return.

🔴 2. corpus/README.md:20-21 instructs a regeneration that does not exist, and the same PR says the opposite

"The two lists are derived and labelled, not hand-picked — regenerate them rather than hand-edit when re-pinning."

There is no generator — the repo's only scripts are ci.sh, docker-runner.sh, publish.sh, and package.json has just build, test, typecheck. Meanwhile this same PR adds AGENTS.md:66: "A spec bump is a deliberate re-pin, exceptions re-derived by hand beside it", and todo.md:67 still reads "The exception list stays the maintainer's."

The lists are also visibly hand-written: example 540's two entries word the same reason differently — "The case-folding shortcut reference is unresolved…" on count, "A case-folding shortcut reference is unresolved…" on text. A generator does not produce that drift.

At the 0.32 re-pin a reader hunts for the regenerator, finds none, and either writes one unasked or hand-edits in defiance of the instruction they just read. AGENTS.md §12: a false claim in a doc is a bug, fixed where found. Fix: delete the sentence; the preceding one already gives each list's shape.

🔴 3. src/commonmark-spec.test.ts:166-168 — three comment lines that are false about the code below them

// The reference HTML's own entity set is the four cmark emits; this decoder is independent of the library's
// (finding: a bug in the shared decoder would corrupt both sides of the comparison). Named and numeric cover a
// future re-pin; the current suite holds only the named four.

• (finding: …) narrates a review finding — self-justification by contrast with what it replaced. That is commit-message material, and the commit already carries it.
• It is wrong twice. namedEntity on the next line holds five keys, not four (apos never occurs). And the suite does not hold "only the named four": ö appears 3 times, in examples 31 and 630. That is harmless only because both are on the refusal list, so the oracle never reads their text — an invariant the comment does not state and nothing enforces. If either stops refusing, ö decodes to the literal text ö, the same silent-wrong-oracle failure as finding 1.

Fix: delete all three lines. With finding 1 applied, a four-entry map named namedEntity needs no comment.

🟡 4. The 62 exception-pin tests detect nothing the per-example loop misses

:347-354 duplicates :334-343. Under the markType→'strong' injection, 39 pin tests failed and every one of their examples was already failing its own per-example test — zero unique detections. They cost 8.6% of the suite, each re-running markdownToAdf up to four more times.

Two assertions there are unique, both list hygiene: :350 (an exception naming an example absent from spec.json) and :351 (an exception whose example is also on the refusal list — a genuinely good catch). Fix: fold both into the meta test at :77 as loops over exceptions, and drop the per-entry loop.

🟡 5. Nothing enforces the rule todo.md 3k was just corrected to state

kind is shape-validated at :46 and never read again. The rule "a fixable §2 hole must not be filed as an exception" now lives only in prose, so a later milestone can file {check:'fixpoint', kind:'pending'} for valid CommonMark the emitter refuses and the suite goes green — the exact outcome 3k forbids. The invariant already holds in the data (all 6 fixpoint entries are unspellable), so pinning it costs one line in the meta test.

🟢 Minor

• :73 — the test is named "holds the full 652-example 0.31.2 run" but still asserts only the length, and 0.30 also has 652 examples. Downgraded from the last round: swapping 0.30's suite in wholesale (276 of 652 examples differ) leaves that test passing but fails 131 others, so the version is de-facto pinned by the refusal and exception data. Naming defect, not a coverage hole — drop "0.31.2" or pin something version-bearing.
• :86-87 and :254-255 are two-line comments; :86-87 restates AGENTS.md §14, which it then cites.
• Five markdownToAdf calls per non-refusing example (:320, plus one each in countMismatch/textMismatch and two in fixpointRefused) — the same result computed five times. Not a problem at 2.9 s; parsing once and passing the document to the three checks would remove it.

❓ Are 301/302 really pending?

corpus/README.md:20 defines pending as "a parser gap a later milestone may close", but todo.md:67-69 presents the marker-change list split as the settled consequence of 3h ("one entry is owed already"). If the library will never split a list on marker change these are permanent and mislabelled. Since nothing asserts kind, a mislabel is silent either way — finding 5 would catch this class.

## Stability review — second pass Every finding from the first pass was re-verified by re-running the original fault injections, not by reading the diff. **Seven of eight are genuinely closed**, one was renamed rather than fixed and is downgraded to non-blocking. The suite is 717 tests green in ~0.7 s; Node, Deno 2.9.6 and Bun 1.4.0 all pass 1277/1277; coverage holds at 100 lines / 100 functions / 98.25 branches. The fixes are real, not restructuring: | Previous finding | Then | Now | |---|---|---| | Refusal path asserted nothing | total parser death left **646/652 passing** | **642/717 fail**; the 75 survivors are exactly the 3 meta tests plus the 72 `unmappable-html` pins matching the injected code — so the *code* is compared, not just the fact of refusal | | Narrow refusal regression | 339/391 silent | swapping `unmappable-image`→`unmappable-html` at all four sites fails **exactly 14** tests, precisely the image pins | | Exception pinned *that*, not *how* | `markType`→`'strong'` left **0** failures | **113** failures (74 per-example, 39 pins); the garbage-output injection on example 484 that previously left all 718 green now fails 2 | | Heading level invisible | level 6→5 gave **0** failures | **1** failure (example 62 — the suite holds exactly one `<h6>`) | | Examples 39/300 | oracle defect filed as library divergence | **oracle repaired, exceptions deleted** — 64→62 entries, removing exactly `(39,'text')` and `(300,'text')` and adding nothing | | Self-confirming oracle | shared `readEntityReference` | independent `namedEntity` map; the import is gone | | Category conflation | one flat list | `kind`: 48 `mark-model`, 6 `unspellable`, 8 `pending` | | AGENTS.md §10, todo.md 3k | false / contradictory | both corrected in the diff | `refusals.json` pinning each of the 86 refusing examples to its code is stronger than the single `deepEqual` suggested, and closes both directions — an example that starts refusing and one that stops. Three things the fix commits introduced, all fixed by deletion. ### 🔴 1. The numeric entity decoder cannot match — it is dead code that would also not work if reached `src/commonmark-spec.test.ts:171-188`. `reference` is `text.slice(index, end + 1)`, so it **includes the trailing `;`** — while both numeric patterns anchor with `$` immediately after the digits: ```ts const decimal = /^&#(\d+)$/.exec(reference) // reference is "&#10;" — never matches const hexadecimal = /^&#[xX]([0-9A-Fa-f]+)$/.exec(reference) ``` Exercised directly: ``` "&amp;" -> { length: 5, text: "&" } "&#10;" -> undefined "&#65;" -> undefined "&#x41;" -> undefined "&ouml;" -> undefined ``` So `characterOf` is unreachable, and the comment's claim that "named and numeric cover a future re-pin" is the opposite of true: at that re-pin `decodeHtmlEntity` returns `undefined` and `referenceText` falls through to pushing `&`, `#`, `1`, `0`, `;` as literal characters — a silently wrong oracle rather than an error. Keeping this is strictly worse than deleting it, because it reads as coverage that does not exist. Nothing will ever catch it: `--test-coverage-exclude="src/**/*.test.ts"` puts the test file's own dead branches outside the 100% floor. AGENTS.md §11 — "no speculative generality — a second consumer, or it goes." **Fix**: delete `characterOf`, both numeric branches and the unused `apos` entry. `decodeHtmlEntity` collapses to the lookup plus an early return. ### 🔴 2. `corpus/README.md:20-21` instructs a regeneration that does not exist, and the same PR says the opposite > "The two lists are derived and labelled, not hand-picked — regenerate them rather than hand-edit when re-pinning." There is no generator — the repo's only scripts are `ci.sh`, `docker-runner.sh`, `publish.sh`, and `package.json` has just `build`, `test`, `typecheck`. Meanwhile this same PR adds `AGENTS.md:66`: "A spec bump is a deliberate re-pin, exceptions **re-derived by hand** beside it", and `todo.md:67` still reads "The exception list stays the maintainer's." The lists are also visibly hand-written: example 540's two entries word the same reason differently — `"The case-folding shortcut reference is unresolved…"` on `count`, `"A case-folding shortcut reference is unresolved…"` on `text`. A generator does not produce that drift. At the 0.32 re-pin a reader hunts for the regenerator, finds none, and either writes one unasked or hand-edits in defiance of the instruction they just read. AGENTS.md §12: a false claim in a doc is a bug, fixed where found. **Fix**: delete the sentence; the preceding one already gives each list's shape. ### 🔴 3. `src/commonmark-spec.test.ts:166-168` — three comment lines that are false about the code below them ``` // The reference HTML's own entity set is the four cmark emits; this decoder is independent of the library's // (finding: a bug in the shared decoder would corrupt both sides of the comparison). Named and numeric cover a // future re-pin; the current suite holds only the named four. ``` - `(finding: …)` narrates a review finding — self-justification by contrast with what it replaced. That is commit-message material, and the commit already carries it. - It is wrong twice. `namedEntity` on the next line holds **five** keys, not four (`apos` never occurs). And the suite does **not** hold "only the named four": `&ouml;` appears 3 times, in examples 31 and 630. That is harmless only because both are on the refusal list, so the oracle never reads their text — an invariant the comment does not state and nothing enforces. If either stops refusing, `&ouml;` decodes to the literal text `&ouml;`, the same silent-wrong-oracle failure as finding 1. **Fix**: delete all three lines. With finding 1 applied, a four-entry map named `namedEntity` needs no comment. ### 🟡 4. The 62 exception-pin tests detect nothing the per-example loop misses `:347-354` duplicates `:334-343`. Under the `markType`→`'strong'` injection, **39 pin tests failed and every one of their examples was already failing its own per-example test — zero unique detections.** They cost 8.6% of the suite, each re-running `markdownToAdf` up to four more times. Two assertions there *are* unique, both list hygiene: `:350` (an exception naming an example absent from `spec.json`) and `:351` (an exception whose example is also on the refusal list — a genuinely good catch). **Fix**: fold both into the meta test at `:77` as loops over `exceptions`, and drop the per-entry loop. ### 🟡 5. Nothing enforces the rule todo.md 3k was just corrected to state `kind` is shape-validated at `:46` and never read again. The rule "a fixable §2 hole must not be filed as an exception" now lives only in prose, so a later milestone can file `{check:'fixpoint', kind:'pending'}` for valid CommonMark the emitter refuses and the suite goes green — the exact outcome 3k forbids. The invariant already holds in the data (all 6 fixpoint entries are `unspellable`), so pinning it costs one line in the meta test. ### 🟢 Minor - `:73` — the test is named "holds the full 652-example 0.31.2 run" but still asserts only the length, and **0.30 also has 652 examples**. Downgraded from the last round: swapping 0.30's suite in wholesale (276 of 652 examples differ) leaves that test passing but fails 131 others, so the version is de-facto pinned by the refusal and exception data. Naming defect, not a coverage hole — drop "0.31.2" or pin something version-bearing. - `:86-87` and `:254-255` are two-line comments; `:86-87` restates AGENTS.md §14, which it then cites. - Five `markdownToAdf` calls per non-refusing example (`:320`, plus one each in `countMismatch`/`textMismatch` and two in `fixpointRefused`) — the same result computed five times. Not a problem at 2.9 s; parsing once and passing the document to the three checks would remove it. ### ❓ Are 301/302 really `pending`? `corpus/README.md:20` defines `pending` as "a parser gap a later milestone may close", but `todo.md:67-69` presents the marker-change list split as the settled consequence of 3h ("one entry is owed already"). If the library will never split a list on marker change these are permanent and mislabelled. Since nothing asserts `kind`, a mislabel is silent either way — finding 5 would catch this class.

lilleman added 1 commit 2026-09-09 14:08:32 +02:00

Delete the unreachable entity decoder, hash-pin spec.json, and fold the exception pin loop d1331a6612

lilleman added 1 commit 2026-09-09 15:40:31 +02:00

Spell the check set once; fix the doubled spec version and a stale filename f81bebe8d0

Some checks are pending

Hide all checks

CI / gate (push) Successful in 18s

Details

CI / publish (push) Has been skipped

Details

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin commonmark-spec-suite:commonmark-spec-suite

git checkout commonmark-spec-suite

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

No items

No Label

Milestone

No items

No Milestone

Assignees

Clear assignees

claes.eismar.se lilleman (Lilleman auf Larv) martin.siden

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: larvit/adf-codec#58