Skip to content

ADFA-5039: Convert kotlin-web-site docs to JSON - #23

Merged
alexmmiller merged 7 commits into
mainfrom
fix/ADFA-5039
Aug 26, 2026
Merged

ADFA-5039: Convert kotlin-web-site docs to JSON#23
alexmmiller merged 7 commits into
mainfrom
fix/ADFA-5039

Conversation

@alexmmiller

@alexmmiller alexmmiller commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds md_to_json.py, which converts a kotlin-web-site/docs checkout
(JetBrains Writerside-flavored Markdown) into the JSON block schema this
project's templating engine renders.

Scope

This is split out of the larger end-to-end Kotlin-docs pipeline in
#21,
which is being separated into two ticket-scoped PRs:

  • ADFA-5039 (this PR): producing the JSON data for the Kotlin website.
  • ADFA-4739 (follow-up PR): build_nav.py, find_missing_assets.py,
    populate_db.py, media insertion, and the GitHub Action that loads
    everything into documentation.db. That PR depends on this one merging
    first, since populate_db.py/build_nav.py import md_to_json.py.

Changes

  • ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/md_to_json.py - the converter.
  • config.json - theming config it takes as input.
  • markdown-it-py added to root requirements.txt.
  • README.md scoped to this script's usage and output schema.
  • review_build_json.sh - a throwaway reviewer helper (installs
    requirements, clones kotlin-web-site, runs the converter) so you can see
    real JSON output with no other setup. Not part of the actual pipeline.

Test plan

Converts kotlin-web-site/docs (Writerside-flavored Markdown) into the JSON
block schema this project's templating engine renders - one JSON file per
topic, plus theme.json and a copy of images/. Split out of the larger
Kotlin-docs pipeline PR so this ticket's scope (producing the JSON) can be
reviewed independently of the database-insertion side (ADFA-4739).

Includes review_build_json.sh, a throwaway helper that clones
kotlin-web-site and runs the converter against it, for reviewers to see
real output without any other setup - not part of the actual pipeline.

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

@luisguzman-adfa luisguzman-adfa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

PR #23 — review comments

Ran md_to_json.py against a small fixture with intentionally broken references (and cross-checked with review_build_json.sh): it converts as expected and the output matches the documented block schema:
title + %var% substitution, cross-page link resolution (b.md#first-heading/b.html#first-heading), external/broken-link coloring, image rewrite with {width=...} folding, and %var% inside code all come through, with the correct "unknown topic" warning for the deliberately-broken link.

Docstring and README are accurate and the "known limitations" are well-scoped.

Test block (Ubuntu)

# 1) get the repo on the PR branch
git clone https://github.com/appdevforall/OfflineDocumentationTools.git
cd OfflineDocumentationTools
git switch fix/ADFA-5039
cd ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON

# 2) run md_to_json.py against a tiny fixture with intentionally broken refs
python3 -m venv /tmp/mdvenv && source /tmp/mdvenv/bin/activate
pip install markdown-it-py --quiet
D=$(mktemp -d); mkdir -p "$D/docs/topics" "$D/docs/images"
printf '<vars><var name="v" value="2.0.0"/></vars>' > "$D/docs/v.list"
printf 'PNG' > "$D/docs/images/m.png"
cat > "$D/docs/topics/a.md" <<'MD'
[//]: # (title: Sample %v%)
## First heading
Para with [x-link](b.md#first-heading), [external](https://x.com), [broken](missing.md).
![alt](m.png){width="200"}
```kotlin
fun main() { println("%v%") }
```
MD
printf '[//]: # (title: B)\n## First heading\nhi\n' > "$D/docs/topics/b.md"
python3 md_to_json.py "$D/docs" "$D/out" config.json && python3 -m json.tool "$D/out/topics/a.json"

Decision points

  1. Heading ids vs. the anchors links point at. slugify() derives ids from heading text while resolve_href keeps the source #anchor verbatim. Simple headings line up (First headingfirst-heading matched its link), but if Writerside's anchor algorithm diverges from slugify on headings with inline code/punctuation, or on duplicate headings, a link resolves to the page but lands on no anchor, silently. Worth spot-checking those cases against a real cross-reference.

  2. Tests. No automated test for md_to_json.py (only review_build_json.sh). The block schema is the contract the templating step depends on, so a small fixture + a check on the emitted blocks would guard it, or, if that's landing with ADFA-4739, a one-line note saying so.

Suggestion

  1. parse_attrs blanks a bare attribute value when the tag also has a quoted one: parse_attrs('group-key=gradle title="Gradle"'){'group-key': '', 'title': 'Gradle'}. The '="' in attr_str check is whole-string, so any quoted attribute empties every bare one. Only fires for a block-level <tab>/<tabs> with a bare attr (Writerside usually quotes everything), so low-trigger, but real, deciding per-pair (use the bare group when the quoted one didn't match) fixes it.

Minor suggestions

  1. slugify doesn't de-dup ids — two headings with the same text share an id, so an anchor to the second lands on the first. The usual -2/-3 suffixing handles it.

  2. Broken/external coloring appends style="color: …"; on an <a> that already has a style that's a duplicate attribute (first wins in HTML5), so those links won't be colored — and the color is injected unescaped, so a hex check in load_config would fail a typo loudly.

  3. page_id isn't posix-normalized (str(rel.with_suffix(""))), while build_topic_index normalizes backslashes, so the id (and the resolved /….html path) come out with backslashes on Windows. .as_posix() keeps them consistent; no-op on Linux/CI.

  4. README usage shows --topics-subdir but not --images-subdir (both exist).

@hal-eisen-adfa hal-eisen-adfa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Deep review of md_to_json.py and friends. 15 findings, left inline.

These aren't read-the-code guesses — I sparse-cloned JetBrains/kotlin-web-site (304 topics + v.list) and ran this converter over the whole corpus, so most findings below come with real file:line citations and occurrence counts from actual Kotlin docs.

Three separate content-loss bugs, all confirmed against the live corpus:

  1. TAG_RE eats <table> (line 84) — <table> appears 38 times in the source and 0 times in the generated JSON. 76 table lines across 18 files are silently consumed.
  2. merge_attr_lines deletes any {...} paragraph (line 427) even when it recovers zero attributes — the content is dropped and nothing is gained.
  3. <tabs> discards all non-tab children (line 469) — intro and trailing prose inside a tabs block vanish with no warning.

Findings 2 and 5 compound badly: { style = "note" } (spaces around =) parses to no attributes and gets swallowed, so tour/kotlin-tour-welcome.md — the first page of the Kotlin tour — renders its note as a plain blockquote. 7 files hit this.

Also flagging line 278 as a markup-injection hole: broken-ext-link-color is interpolated into an HTML attribute unvalidated. Config is repo-controlled so it isn't remotely exploitable, but it's a one-line fix.

Please note this file is byte-identical to the md_to_json.py in #21git diff between the two branches is empty for it. Findings on lines 84, 142 and 278 duplicate comments I left there. Fixing once fixes both, but whichever PR merges second will carry them if only one gets patched. Worth deciding how #21 and #23 relate before either lands.

In fairness, a number of things I went looking for turned out to be fine, and I want to be explicit so this doesn't read as a wall of doom: no _raw leakage into the output JSON (0 corpus-wide), no tag_marker leakage (0), no empty-string heading ids (0), no IndexError on empty table cells, no blank-line stripping in raw <pre> blocks, and the unpinned markdown-it-py is genuinely safe (1.1.0 already exposes Token.attrs as a dict). The overall shape of the converter is sound — the damage is concentrated in the Writerside tag/attribute parsing.

Comment thread ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/md_to_json.py Outdated
Comment thread ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/md_to_json.py Outdated
Comment thread ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/md_to_json.py Outdated
Comment thread ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/md_to_json.py Outdated
Comment thread ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/review_build_json.sh Outdated
Comment thread ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/md_to_json.py Outdated
Fixes 12 correctness bugs found by review (verified against a live
kotlin-web-site corpus, not just read-the-code):

- TAG_RE matched "tab" as a prefix of "table", silently eating every raw
  HTML table in the corpus (38 occurrences / 18 files).
- merge_attr_lines deleted any {...}-shaped paragraph even when it parsed
  to zero attrs, dropping real content.
- <tabs> blocks discarded any non-<tab> sibling (intro/trailing prose).
- A mismatched closing tag_marker was silently ignored, leaving the wrong
  frame open and relocating later content into it.
- ATTR_PAIR_RE didn't allow whitespace around "=", and parse_attrs' whole-
  string quoted-vs-bare check blanked bare values whenever any sibling in
  the same group was quoted.
- fold_image_attrs only handled exactly one trailing {...} group and
  required the whole text token to be nothing else, so a second attribute
  group or trailing prose leaked as literal visible text.
- broken-ext-link-color was interpolated into a style="..." attribute
  unvalidated (markup-injection hole) and unconditionally appended even
  when the <a> already had a style=, producing a silently-ignored
  duplicate attribute.
- main() swallowed per-file conversion failures and still exited 0.
- The image-src regex rewrote src= on any element, not just <img>,
  producing false "image not found" warnings for <script>/<iframe>.
- page_id/sourceFile used str(Path(...)) instead of .as_posix(), which
  would disagree with build_topic_index's forward-slashed ids on Windows.
- build_topic_index resolved duplicate topic stems first-wins with no
  warning, unlike the equivalent image-filename collision handling.
- Heading lines with a trailing Writerside attribute suffix (most commonly
  {id="..."}, also seen as {completion-point=...}) rendered that suffix as
  literal visible garbage text, with no attribute handling and no
  anchor-override support - found via corpus verification, not flagged by
  either reviewer, but the same bug family and comparably common (~39
  affected pages).

Also: de-dupes heading ids that collide on identical text (anchor to the
second heading no longer lands on the first); skips re-copying unchanged
images and prunes stale topic JSON left over from a previous run; switches
review_build_json.sh from raw pip/python3 to `uv run` (PEP 668
externally-managed-environment installs were crashing it outright on
Homebrew/Debian/Ubuntu Python).

Adds tests/test_md_to_json.py (30 cases, one per fix above, several reusing
reviewers' own repro snippets) and tests/conftest.py so pytest can find the
module under test.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@alexmmiller

Copy link
Copy Markdown
Collaborator Author

Pushed 6b25e26 addressing both reviews. Replied inline to each of the 15 line comments individually; summary + the two review-level threads here.

Fixed (all 12 correctness findings)

Every "blocking" item from @hal-eisen-adfa's review is fixed, plus @luisguzman-adfa's suggestion (#3, same bug as the line-142 finding) and minor points (#4 slugify de-dup, #5 duplicate style attr, #6 path separator, #7 README --images-subdir). Each is verified with a direct unit test in the new tests/test_md_to_json.py (30 cases), and re-confirmed against a real kotlin-web-site corpus run via review_build_json.sh where the original finding cited corpus numbers.

One more bug found during verification (not flagged by either review)

While re-running the corpus to confirm the fold_image_attrs fix, I noticed heading lines with a trailing Writerside attribute suffix - most commonly ## Title {id="custom-anchor"}, also seen as {completion-point=...} - were rendering that suffix as literal visible garbage text, with zero attribute handling. Same bug family as the image-attrs findings, just a different injection point the corpus checks happened not to exercise. Affected ~39 pages. Fixed the same way: Converter.extract_trailing_attrs strips it and, when an explicit id is given, uses it to override the auto-slug (which is the actual point of that syntax - a stable custom anchor) rather than just discarding it.

Decision point #1 (heading id vs. anchor mismatch)

Didn't fix - it's not a demonstrated bug, no concrete repro was given, and reproducing Writerside's own anchor-slugging algorithm to compare against is a bigger investigation than this PR's scope. Documented as a known limitation in both the module docstring and README instead, so it's tracked rather than silently dropped.

Decision point #2 (tests)

Added - tests/test_md_to_json.py, one case per fix above plus a few backward-compat checks, several reusing your own repro snippets directly (the getting-started.md:89 two-brace-group case, the missing-</tab> case, the {a.length}/{it.length} case). Run with:

uv run --with pytest --with-requirements ../../../requirements.txt python3 -m pytest tests/

On #21/#23/#24

@hal-eisen-adfa flagged that md_to_json.py is byte-identical between this PR and #21, so the same findings apply to both. That's expected - #21 is the original, unsplit branch (fix/ADFA-4737) that #23 and #24 (fix/ADFA-4739) were carved out of; #21 stays open for now purely as a reference for that split and will be closed as superseded once both land, never merged itself. So fixing it here is the right (and only) place - #21 inherits nothing further from this point on.

Worth flagging for @hal-eisen-adfa and @luisguzman-adfa specifically: build_nav.py, find_missing_assets.py, and populate_db.py (which import this file directly) live in #24, not here - if either of you is planning a similar pass over those, they depend on this PR's version of md_to_json.py/build_topic_index/build_image_index, which is why I kept both functions' return signatures unchanged even while fixing their internals (see the CONTAINER_TAGS/dead-code reply for specifics).

@hal-eisen-adfa hal-eisen-adfa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Round 2 — all 15 previous findings verified fixed; 5 new ones introduced by the fixes

I re-ran the converter over a fresh 304-topic kotlin-web-site corpus and ran your test suite (30/30 pass, 304/304 converted, exit 0). Every one of the 15 findings from my last pass is genuinely fixed — I verified against real output, not just the diff:

Finding Evidence
TAG_RE ate <table> 30 in source → 30 in output (was 0); orphan "type":"tab" blocks: 0
merge_attr_lines deleted {...} paragraphs guarded on non-empty parse_attrs
<tabs> dropped non-tab children whatsnew24.md intro prose survives
{ style = "note" } kotlin-tour-welcome.json now emits attrs: {"style": "note"}
bare attrs blanked 411 populated width="N", 0 blanked; group-key populates
image attr braces leaked 15 → 0 genuine
color injection / duplicate style= validated at load_config, merged not appended
main() exited 0 on failure exit 1 confirmed, --allow-failures works
src= on every element the 8 remaining warnings are all genuine missing .png — no false positives
Windows separators as_posix() throughout
image re-copy / stale JSON ctime unchanged on 2nd run; stale JSON pruned

Two things I want to be explicit about, because you pushed back and you were right:

  • I checked your "the 2 residual {width= hits are inside <!-- comments" claim. It holds — both are in commented-out markdown.
  • I checked converter.warnings / build_image_index's tuple against #24. find_missing_assets.py does read both. That's not dead code and I was wrong to call it that — keeping the signatures stable was the correct call.

Also: finding the heading {id=...} suffix bug yourself, during verification, on a corpus path neither reviewer's checks exercised, is exactly the right instinct. ~39 pages is not a small catch.


The 5 new findings

All five are introduced by this round's fixes — none existed in 7f67b2fb. Two are serious. The pattern worth noting: three of the five are cases where the fix is correct in exactly the case the new test exercises and wrong just outside it.

  1. group_containers now crashes on a top-level unmatched closer (line 588) — dropped len(stack) > 1 guard, KeyError.
  2. rmtree can delete the source docs (line 812) — no output_dir != docs_root guard. Reproduced.
  3. The style= merge emits invalid CSS (line 352) — no ; separator; both rules get dropped.
  4. fold_image_attrs eats the separating space (line 382) — minor.
  5. COLOR_RE.match raises TypeError on a non-string (line 741) — minor.

Details inline. 1 and 2 are each a couple of lines; I'd want both before this lands, especially since #24 wires this into CI. Everything else here is in good shape — this was a thorough pass and the corpus numbers back it up.

Resolving all 15 previous threads.

Comment thread ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/md_to_json.py Outdated
Comment thread ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/md_to_json.py Outdated
Comment thread ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/md_to_json.py Outdated
Comment thread ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/md_to_json.py Outdated
group_containers crashed (KeyError) on a top-level unmatched closing tag
since the len(stack) > 1 guard was dropped during the round-1 fix. The
topics-dir pruning rmtree could delete the source docs when output_dir
resolves to the same tree as docs_root. The duplicate-style merge could
produce invalid (dropped) CSS when the existing style had no trailing
";". fold_image_attrs re-stripped the remainder after an image, eating
the leading space before trailing prose. load_config raised a bare
TypeError instead of its own clean error on a non-string color value.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
alexmmiller pushed a commit that referenced this pull request Aug 10, 2026
Blocking: CLAUDE.md's templateId "out of scope" claim was falsified by
this same PR (populate_db.py/insert_optimized_media.py both read/write
it) - narrowed the claim to the names that are actually absent. The
nav-hidden class nav.peb emits was inert (no consuming CSS rule),
rendering Writerside-hidden nav entries (e.g. individual tour steps)
visible - added the missing docs.css rule.

Non-blocking: rewrote the Dokka-plugin-kdoc2json bullet (and decisions
log) to reflect that fix/ADFA-4514 is merged, rather than describing it
as an outstanding rebase. find_missing_assets.py swallowed per-file
scan failures and always exited 0, so a totally broken corpus still
looked clean - added a failure counter, --allow-failures flag, and a
report line, mirroring md_to_json.py's pattern from #23. Converted
README.md, optimize_media.py, and run_e2e_pipeline_test.sh from bare
pip/python3 to uv run --with-requirements, and added scour/cairosvg to
requirements.txt, matching the repo's established uv convention.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@alexmmiller

Copy link
Copy Markdown
Collaborator Author

Pushed 5ec1f60 addressing round 2. Replied inline to each of the 5 new line comments individually; summary here.

Fixed (all 5 round-2 findings)

  1. group_containers crash on a top-level unmatched closer (line 588) - restored the len(stack) > 1 guard dropped during round 1, so stack[-1]["type"] is never evaluated against the bare root frame (which has no "type" key). Verified '</note>\n\nAfter.\n' no longer raises and still emits the kind: "tag" warning.
  2. rmtree could delete the source docs (line 812) - added an explicit topics_out_dir.resolve() == topics_dir.resolve() guard before the rmtree, exiting 1 with a clear error instead of deleting docs_root's topics/.
  3. Style-merge produced invalid CSS (line 352) - used your suggested sep logic verbatim: only inserts "; " when the existing style value is non-empty and doesn't already end in ;.
  4. fold_image_attrs ate the word-boundary space (line 382) - the remainder is no longer re-.strip()'d after slicing; only the truthiness check strips, the content itself keeps its leading space.
  5. load_config raised a bare TypeError on a non-string color value (line 741) - added an isinstance(config[key], str) check ahead of the regex match so it hits the intended clean error message instead.

Tests

One regression test per fix, plus tightened test_fold_image_attrs_preserves_trailing_prose to assert the exact string (" Slack:", not the .strip()'d "Slack:") - per your own note that the old assertion wouldn't have caught #4 either way. 34/34 tests pass (29 existing + 5 new/tightened).

@hal-eisen-adfa

Copy link
Copy Markdown
Collaborator

Code review

Found 3 issues:

  1. Self-closing container tags open a container that never closes, silently nesting the rest of the page inside it. In TAG_RE, the greedy ([^>]*) absorbs the trailing / before the optional /? can match, so group 1 (closing) stays empty and <tab title="A"/> is recorded as an opening marker. Reproduced against HEAD: <tab title="A"/> followed by a paragraph yields [{"type": "tab", "blocks": [{"type": "paragraph", ...}]}] with no warning emitted. No test exercises a self-closing tag. This was flagged on PR ADFA-4739: Pipeline for producing template-based Kotlin documentation #21 against the byte-identical file (discussion_r3732882620, "A self-closing tag opens a container that never closes, swallowing the rest of the page") but was not carried over into this PR.

# "table", consuming "<table>" as tag "tab" with attrs "le".
CONTAINER_TAGS = {"tabs", "tab", "note", "tip", "warning"}
TAG_RE = re.compile(r"^<(/?)(" + "|".join(CONTAINER_TAGS) + r")(?![\w-])([^>]*)/?>$", re.I)

  1. Indented (4-space) code blocks are emitted as raw unescaped HTML instead of code. convert_node handles markdown-it's fence token but has no case for code_block, which CommonMark produces for indented code. Those fall through to the generic fallback below and become {"type": "html", "html": <raw content>} — so code containing < or & (e.g. List<String>) is passed through as markup rather than displayed as code, and the block loses its code typing. The string code_block does not appear anywhere in md_to_json.py or in tests/test_md_to_json.py.

# Fallback: anything not explicitly handled (images are inline-only,
# so plain "image" blocks don't occur at block level; captured via
# paragraph HTML instead).
return {"type": "html", "html": self.rewrite_urls(str(t.content or ""))}

  1. The documented image block type is never emitted, so the published schema is wrong. No code path in convert_node/convert_nodes constructs {"type": "image", ...} — markdown-it only ever produces image as an inline token nested in a paragraph, which render_inline turns into an <img> inside that block's html string. The fallback's own comment states this ("images are inline-only, so plain "image" blocks don't occur at block level; captured via paragraph HTML instead"), contradicting the docstring schema below and the matching image entry in README.md. Since producing this JSON schema is the stated deliverable of this PR, a template author on the ADFA-4739 follow-up would write dead handling for a block that never appears.

{"type": "list", "ordered": false, "items": [{"blocks": [...]}]}
{"type": "table", "headers": ["a", "b"], "rows": [["1", "2"]]}
{"type": "image", "src": "...", "alt": "..."}
{"type": "hr"}
{"type": "tabs", "attrs": {"group": "build-system"},

Frequency note, so these can be triaged rather than taken at face value: against a kotlin-web-site clone at 5aff3dc (298 files under docs/topics), issue 1 has 0 occurrences and issue 2 has 1 — in async-programming.md, whose content contains no < or & and so is not currently mangled. Both are latent rather than actively breaking output today. Issue 3 affects the contract the follow-up PR builds against.

Also confirmed: all round-1 and round-2 review findings are genuinely fixed at 5ec1f60a, re-derived from the current file rather than from the "fixed in X" replies.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

Self-closing container tags (<tab .../>) had their trailing "/" eaten
by TAG_RE's greedy attrs group, so they were recorded as openers that
never close - silently nesting the rest of the page inside them. TAG_RE
now captures the self-close marker in its own group, and the html_block
dispatch emits an immediate open/close pair for one.

Indented (4-space) code blocks produced markdown-it's "code_block"
token, which convert_node had no case for - they fell through to the
generic fallback and rendered as unescaped raw HTML instead of a typed
code block. Added a code_block case alongside the existing fence one.

The documented "image" block type was never actually emitted - images
are inline-only in markdown-it, always folded into their containing
block's own html. Fixed the docstring/README to match reality instead
of documenting a block shape that can't occur.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@alexmmiller

Copy link
Copy Markdown
Collaborator Author

Pushed 5336ee5 addressing all 3.

  1. Self-closing tags (TAG_RE) - fixed by splitting the self-close marker into its own capture group (([^>]*?)\s*(/?)>$ instead of a single greedy ([^>]*)/?>$), so it can no longer be swallowed by the attrs group. convert_node's html_block dispatch now emits an immediate open/close pair for a self-closing tag instead of a bare opener. Verified <tab title="A"/> followed by a paragraph no longer nests that paragraph inside the tab, and confirmed <table>/</table> still correctly don't match. Covered by test_tag_re_captures_self_closing_marker_separately, test_html_block_self_closing_tag_emits_open_and_close_markers, and test_self_closing_tag_does_not_swallow_trailing_content.
  2. Indented code blocks - added a code_block case to convert_node alongside the existing fence one, emitting the same {"type": "code", ...} shape. Verified List<String> x = ... (4-space indented) now comes out typed as code with the raw text intact, not as an unescaped html block. Covered by test_indented_code_block_renders_as_code_not_html.
  3. Documented image block type - agreed this was a docs-vs-reality mismatch rather than a code bug, so fixed the docstring and README instead of adding new "promote an image-only paragraph to a block" behavior that could change what ADFA-4739's templates receive. Both now state explicitly that images are always inline content inside their containing block's html, never a standalone block.

38/38 tests pass (34 previous + 4 new).

@alexmmiller
alexmmiller requested review from Daniel-ADFA, dara-abijo-adfa, jatezzz and jimturner-adfa and removed request for jomen-adfa August 20, 2026 20:06

@hal-eisen-adfa hal-eisen-adfa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Automated review at high effort, run against the live corpus rather than the diff alone: I sparse-cloned JetBrains/kotlin-web-site (303 topics, 7,863 links), ran the converter over it, and audited the output. Every finding below is reproduced.

Baseline health is good — 303/303 topics convert with only upstream-broken-link warnings, no schema leaks (_raw, tag_marker, and tabs-without-tabs are all zero), and the 38-test suite passes.

1 HIGH, 3 MEDIUM, 7 LOW inline. The HIGH is the one that matters: slugify() strips punctuation instead of hyphenating it, which breaks 209 real cross-references. The rest are mostly latent or documentation-contract issues.

Things I specifically tried to break and couldn't: all four bug classes fixed across the earlier review rounds hold up on the live corpus — TAG_RE's (?![\w-]) correctly rejects <table>, self-closing <tab/> yields a balanced marker pair, merge_attr_lines preserves { it.length }-style paragraphs (1 real occurrence, in functions.md), and the style-attribute merge produces valid CSS. A 32-vs-31 heading discrepancy I chased in compatibility-guide-17.md turned out to be a heading inside an HTML comment — correct behavior, not a loss.

Comment thread ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/md_to_json.py Outdated
Comment thread requirements.txt Outdated
HIGH: slugify() deleted punctuation instead of hyphenating it, diverging
from Writerside's own anchor algorithm on any heading with "." "/" ":"
etc. - 209 dead #anchor links measured on the live kotlin-web-site
corpus, down to 15 with this fix.

MEDIUM: fold_image_attrs set folded_any on a regex match rather than a
successful parse, so a non-attribute "{...}" group (a CSS class
shorthand, or prose) was swallowed instead of left as visible text -
same guard merge_attr_lines/extract_trailing_attrs already apply.
Also extended fold_image_attrs to handle a trailing "{...}" group
after a *link*, not just an image/heading (real hit in
js/js-ir-compiler.md). group_containers now warns when a container is
still open at EOF, symmetric with the existing unmatched-closer warning.

LOW: MD_LINK_RE and resolve_image_src now resolve a ".md"/image
reference that carries a path prefix by bare filename (Writerside's own
convention) instead of silently failing to match/resolve with no
warning; also drops resolve_image_src's dead startswith("/") check.
Blank lines inside a raw HTML run (e.g. <pre>, or between <table> rows)
are preserved instead of dropped. main()'s output-aliases-source guard
now runs before theme.json/images are written, not just before the
topics rmtree. images/ now prunes stale files the same way topics/
JSON already did, via a new _prune_stale_files. Documented that "html"
blocks are passthrough fragments that may be structurally unbalanced
across block boundaries. markdown-it-py pinned with a >=2.0 floor
(Token.attrs was a list before that version).

13 new regression tests; 55/55 pass.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@alexmmiller

Copy link
Copy Markdown
Collaborator Author

Pushed 4fddb34 addressing all 11 findings from the latest automated review. Replied inline to each thread individually; summary here.

HIGH (1)

  1. slugify() deleted punctuation instead of hyphenating it (line 211) - measured at 209 dead #anchor links on the live corpus. Fixed using your suggested regex; verified all four of your repro examples now match their linked anchors exactly.

MEDIUM (3)

  1. fold_image_attrs swallowed a {...} group that parsed to nothing (line 393) - folded_any now only sets on a successful parse_attrs, mirroring the guard merge_attr_lines/extract_trailing_attrs already use.
  2. {...} after a link rendered as literal text (line 428) - extended fold_image_attrs to fold a trailing group onto the matching link_open's attrs (via a new _find_link_open helper), not just after images/headings.
  3. A container left open at EOF warned nothing (line 608) - group_containers now checks len(stack) > 1 after the loop and warns, symmetric with the existing unmatched-closer branch.

LOW (7)

  1. .md links with a path prefix neither resolved nor warned (line 281) - MD_LINK_RE now absorbs a path prefix and resolves by bare filename, same as build_topic_index.
  2. Image srcs with a path component silently dropped (line 295) - resolve_image_src now strips to bare filename before lookup; also dropped the now-genuinely-dead startswith("/") duplicate check.
  3. Unbalanced raw-HTML fragments (line 536) - documented as an explicit contract in the module docstring rather than changed, per your own framing.
  4. Blank lines inside a raw HTML run dropped (line 572) - elif line.strip():else: so they're preserved in the joined output.
  5. main() wrote into the source before validating it wasn't the source (line 828) - the aliasing guard now runs before theme.json/images/ are touched, not just before the topics rmtree.
  6. Stale images never pruned (line 833) - added _prune_stale_files, run after the copytree, keeping the _copy_if_changed mtime-skip optimization intact (a pruning pass instead of switching to rmtree-then-recopy).
  7. markdown-it-py had no version floor (requirements.txt:8) - pinned >=2.0. Flagged the stale ADFA-4739 comment you mentioned for whoever's on that branch, since it's a different file I can't fix from here.

Tests

13 new regression tests, several reusing your exact repro snippets/corpus examples. 55/55 pass (42 previous + 13 new).

All six are latent: the live kotlin-web-site corpus triggers none of them, and
converting all 303 pages before and after produces byte-identical JSON with an
identical warning set. Each is a case the corpus happens not to contain today.

 - An explicit {id="..."} only beat a *later* auto-slug. In the other order
   (`## Custom anchor` then `## Something {id="custom-anchor"}`) the auto-slug
   claimed the id first and both headings shipped id="custom-anchor", so any
   link to the explicit one landed on the auto one. reserve_explicit_heading_ids
   now registers every explicit id up front, before any heading converts, so
   explicit wins in both directions. Scoped to inlines directly inside a
   heading - a paragraph merely ending in "{id=...}" is not an anchor. Two
   genuinely identical explicit ids are a source bug this can't fix, so both
   are kept as authored and it warns.
 - A heading whose text is all punctuation ("## ...") slugified to "", i.e.
   id="" - invalid HTML and unlinkable. Falls back to "section", numbered by
   the existing de-dup loop.
 - extract_title matched a `[//]: # (title: ...)` comment inside a fenced code
   block, which both set a bogus title and deleted that line out of the code
   sample being displayed. It now skips matches inside ``` / ~~~ fences,
   handling tilde fences, longer fences nesting shorter ones, and an
   unterminated fence running to EOF.
 - A raw html_block run of nothing but blank lines emitted
   {"type": "html", "html": ""} - a truthy list of empty strings. Blank lines
   inside a run that has content are still preserved, which is what keeps
   <pre>/<script> bodies intact.
 - COLOR_RE's flat {3,8} accepted #12345 and #1234567, which no browser does.
   Narrowed to the lengths CSS defines. This value is interpolated into a
   style="" attribute, so the validator should mean what it says.
 - build_tree popped an already-empty stack on a stray closing token, turning
   a malformed stream into "IndexError: pop from empty list" with nothing
   naming the cause. Defensive - markdown-it always balances - but it now
   ignores the stray closer and says so.

Docstring: the "Block shapes" list omitted note/tip/warning, which
group_containers genuinely emits (README already documented them). Also records
that a <tab> written outside any <tabs> comes through as a bare "tab" block a
template won't render - verified, and it does not warn.

Tests: 55 -> 91, coverage 66% -> 81% (main() is subprocess-tested, so
under-reported either way). Beyond one regression test per fix, this closes the
coverage gaps the review named: %variables% substitution and load_variables had
no test at all; block-level <note> was covered by nothing, since the corpus only
uses the single-line form; and both <tabs> fallback paths - synthesizing tabs
from adjacent code fences, and dropping a wrapper with nothing tab-like - were
untested despite each having fixed a content-dropping bug. Adds direct tests for
the blockquote/list/table/hr converters and a guard that "_raw" never reaches
the JSON.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alexmmiller pushed a commit that referenced this pull request Aug 25, 2026
…line)

Brings in the ADFA-5153/ADFA-5171 work merged to fix/ADFA-4737 via PRs #26
and #27, which this branch forked from #21 too early to receive. Without it
the pipeline cannot run against the current production database at all:
~/documentation.db is schema 2.0.0, every "brotli" Content row is compressed
against the shared 256 KiB raw LZ77 dictionary in CompressionDictionary, and
plain Brotli cannot decode any of it (measured: 0 of 24 sampled rows).

Conflict resolution - all twelve were add/add, so each was decided per file
rather than 3-way merged:

Took theirs (the dictionary lineage is strictly ahead on these three), then
re-applied this branch's review fixes on top:
 - populate_db.py: DictionaryCompressor, train/load_or_create_dictionary,
   fragment_chain, page_size pinning. Re-applied the conversion-failure
   abort, the same-stem dedupe, and the basename-keyed image index.
 - insert_optimized_media.py: dictionary-aware reads/writes. Re-applied the
   delete_unreferenced_media floor check, the delete-before-insert ordering,
   and --dry-run.
 - sync_kdoc_json_to_db.py: DictionaryBrotli, load_compression_dictionary,
   MAX_DELETE_FRACTION. Re-applied CHUNK_SIZE fragmentation, the fatal
   unknown-contentTypeID, and the VACUUM INTO backup.

Took ours (PR #23/#24 refined these after the split): md_to_json.py,
find_missing_assets.py, optimize_media.py, assets/docs.css, README.md,
run_e2e_pipeline_test.sh, .gitignore.

Hand-merged: build-kotlin-docs.yaml (our corrected requirements/webp comments
plus their brotli-CLI rationale); CLAUDE.md (ours, with the 2.0.0 blocker note
rewritten as a description of how the three writers now handle the dictionary,
since the merge resolves it).

Two of this branch's own fixes were dropped as superseded:
 - like_escape/ESCAPE '\' is replaced by fragment_chain, which does the
   over-matching LIKE once and re-checks each candidate's digit suffix. That
   also handles ADFA-5171 chains numbered from -2, which escaping does not.
   sync_kdoc_json_to_db.fragment_paths was rewritten to match rather than
   probing "-1" and stopping at the first gap.
 - The hand-rolled DictionaryCompressor added to the sync script last commit
   is replaced by theirs.

Tests updated for the merged APIs (collect_referenced_media and
delete_unreferenced_media now take a compressor; DictionaryBrotli is
compress-only, so its tests decode through the brotli CLI). 105 pass: 78 in
ProcessKotlinWebsiteJSON, 27 in scripts/sync_kotlin_stdlib_docs.

Verified against a copy of ~/documentation.db: 3,238 stdlib rows rewritten,
12/12 sampled decode against the dictionary, untouched trees unaffected, row
count unchanged at 30,649.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alexmmiller pushed a commit that referenced this pull request Aug 25, 2026
Brings this branch's copy of md_to_json.py up to PR #23's tip. It had been
stale since the 1cf41d2 merge: 4fddb34 ("Fix 10 issues from Hal's automated
corpus review") and 1d7f6a7 ("Fix 6 latent defects from the md_to_json.py code
review") both landed on fix/ADFA-5039 afterwards, and populate_db.py,
build_nav.py and find_missing_assets.py all import this module directly.

md_to_json.py and tests/test_md_to_json.py are now identical on both branches.

One conflict, in requirements.txt, resolved as the union: this branch added
scour/cairosvg for optimize_media.py, #23 pinned markdown-it-py>=2.0.

158 tests pass (131 in ProcessKotlinWebsiteJSON, 27 in sync_kotlin_stdlib_docs).

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

Copy link
Copy Markdown
Collaborator Author

Full code review of ProcessKotlinWebsiteJSON/, and six fixes

Reviewed the whole directory at 4fddb348. The code is solid — 55/55 tests passed, ruff check --select=E9,F,B,SIM,C4 clean, and against a fresh kotlin-web-site clone it converted 303/303 pages in 1.1s, byte-for-byte deterministic across runs, with stale-output pruning working for both topics/ and images/ (including now-empty directories).

Six defects found and fixed in 1d7f6a7e. All six are latent — I checked each against the live corpus and none of them fire there.

The fixes

# Defect Fix
1 An explicit {id="..."} only beat a later auto-slug. In the other order the auto-slug claimed the id first and both headings shipped the same id, so any link to the explicit one landed on the auto one. New reserve_explicit_heading_ids pass registers every explicit id before any heading converts, so explicit wins in both directions. Scoped to inlines directly inside a heading — a paragraph merely ending in {id=...} is not an anchor. Two genuinely identical explicit ids are a source bug this can't fix, so both are kept as authored and it warns.
2 A punctuation-only heading (## ...) slugified to ""id="" is invalid HTML and unlinkable. Falls back to section, numbered by the existing de-dup loop.
3 extract_title matched a [//]: # (title: ...) comment inside a fenced code block, both setting a bogus title and deleting that line out of the code sample being displayed. New fenced_spans skips matches inside ``` / ~~~ fences — handles tilde fences, longer fences nesting shorter ones, and an unterminated fence running to EOF.
4 A raw html_block run of nothing but blank lines emitted {"type": "html", "html": ""} (a truthy list of empty strings). flush_raw requires actual content. Blank lines within a run that has content are still preserved, which is what keeps <pre>/<script> bodies intact.
5 COLOR_RE's flat {3,8} accepted #12345 and #1234567, which no browser does. Narrowed to the lengths CSS defines. This value is interpolated into a style="" attribute, so the validator should mean what it says.
6 build_tree popped an already-empty stack on a stray closing token — IndexError: pop from empty list with nothing naming the cause. Ignores the stray closer and warns. Defensive; markdown-it always balances.

How I verified these are safe

Since every finding is latent, the meaningful check wasn't the unit tests — it was a differential corpus run. I converted all 303 pages with the converter at 4fddb348 and again with the fixed one, and diffed:

  • 0 differing output files
  • identical 57-warning set

So these change nothing about today's output; they only close cases the corpus doesn't currently contain.

On those 57 link warnings, for the record: they're 27 distinct references, and I checked every one against the checkout. All are genuinely absent from topics/ — cross-repo links into kotlinx.coroutines and dokka. No false positives from the resolver.

Docstring

The "Block shapes" list omitted note/tip/warning, which group_containers genuinely emits (the README already documented them, and points readers at the module docstring "for full shapes"). Now added.

Also recorded, after testing it: a <tab> written outside any <tabs> comes through as a bare {"type": "tab"} block that a template written against the documented shapes won't render — and it does not warn. I documented the actual behaviour rather than changing it, since that's beyond the scope of this review. Worth filing separately if you want it to warn.

Tests: 55 → 91, coverage 66% → 81%

Beyond one regression test per fix, this closes the coverage gaps the review turned up — features that had no test at all:

  • %variables% substitution (substitute_vars / load_variables) — a documented feature that reaches every rendered string.
  • Block-level <note> — covered by nothing. Note the corpus can't cover it either: all 5 <note> uses in kotlin-web-site are the single-line form inside releases.md's hand-written HTML tables, which is the documented passthrough case and behaves correctly.
  • Both <tabs> fallback paths — synthesising tabs from adjacent code fences, and dropping a wrapper with nothing tab-like inside. Each had previously fixed a content-dropping bug, and neither was tested.
  • Direct tests for the blockquote/list/table/hr converters, and a guard that the internal _raw key never reaches the JSON.

(Coverage is under-reported either way — main() is exercised by 10 subprocess tests that coverage can't see.)

Note for #24

fix/ADFA-4739 had a stale copy of md_to_json.py — it merged this branch before 4fddb348 landed. I've synced it (80e234a9); md_to_json.py and its tests are now identical on both branches.

@alexmmiller
alexmmiller merged commit c627c1b into main Aug 26, 2026
@alexmmiller
alexmmiller deleted the fix/ADFA-5039 branch August 26, 2026 21:18
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.

3 participants