Skip to content

ADFA-4739: Kotlin docs DB pipeline + Build Kotlin Docs GitHub Action - #24

Open
alexmmiller wants to merge 2 commits into
mainfrom
fix/ADFA-4739
Open

ADFA-4739: Kotlin docs DB pipeline + Build Kotlin Docs GitHub Action#24
alexmmiller wants to merge 2 commits into
mainfrom
fix/ADFA-4739

Conversation

@alexmmiller

@alexmmiller alexmmiller commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

An end-to-end pipeline for loading Kotlin website content and
kotlin-stdlib/-reflect/-test JSON docs into documentation.db, plus a
GitHub Action (Build Kotlin Docs) that runs it against a Drive-hosted copy
of the database.

Scope

Split out of #21
into two ticket-scoped PRs:

  • ADFA-5039 - producing the raw JSON data for the Kotlin website (md_to_json.py).
  • ADFA-4739 (this PR) - the database-manipulation side and the GitHub Action.

This PR depends on ADFA-5039 merging first. populate_db.py,
build_nav.py, and find_missing_assets.py all import md_to_json.py
directly, and that file isn't included here. Please review this PR's
content, but hold off merging until #23 lands - README.md in particular
will very likely conflict with #23's own new README.md at the same path
(both add a new file there) and need a quick manual merge to combine the two
once both are in.

Changes

  • ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/: build_nav.py
    (sidebar nav from kr.tree), find_missing_assets.py (source QA),
    populate_db.py (inserts pages/nav/media into documentation.db,
    supports pruning via --blacklisted-element-titles), optimize_media.py
    / insert_optimized_media.py (media optimization + DB update),
    templates/, assets/.
  • scripts/sync_kotlin_stdlib_docs/sync_kdoc_json_to_db.py - syncs
    Dokka-generated kotlin-stdlib/-reflect/-test JSON into the database.
  • ProcessDocs/ProcessKotlinDocs/run_e2e_pipeline_test.sh - local e2e test
    of the whole pipeline against a scratch copy of the database.
  • .github/workflows/build-kotlin-docs.yaml - CI counterpart: pulls
    documentation.db (and Writerside's webHelpImages.zip) from Google
    Drive, runs the same pipeline, uploads the result back.
  • Dokka-plugin-kdoc2json/scripts/kotlin/build-stdlib-json-docs.sh +
    build.gradle.kts tweak - builds the kdoc-to-json plugin and generates
    the stdlib JSON docs the sync script consumes.
  • CLAUDE.md - repo-level orientation doc (schema drift between this repo's
    tooling and the live production database, repo tour, decisions log).

Test plan

  • Once ADFA-5039 is merged and this branch is rebased, re-run
    run_e2e_pipeline_test.sh locally against a scratch database.
  • Trigger Build Kotlin Docs with dry_run: true and confirm the
    summary/blacklist-verification steps pass.
  • Reviewer confirms the scripts match their originals in ADFA-4739: Pipeline for producing template-based Kotlin documentation #21 (no edits
    made during the split, aside from README.md being trimmed to drop the
    md_to_json.py-only section now covered by ADFA-5039).

…739)

Adds the code that loads converted Kotlin website content (build_nav.py,
populate_db.py, media insertion) and kotlin-stdlib/-reflect/-test JSON
content (sync_kdoc_json_to_db.py) into documentation.db, the local e2e test
script for that pipeline, and the CI workflow that runs it end-to-end
against a Drive-hosted copy of the database.

Split out of the larger Kotlin-docs pipeline PR (#21) so the DB-manipulation
side (this ticket) can be reviewed separately from producing the raw JSON
data for the Kotlin website (ADFA-5039, PR #23). This PR depends on ADFA-5039
merging first - populate_db.py, build_nav.py, and find_missing_assets.py all
import md_to_json.py, which isn't included here.

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

@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.

Review scoped to the files that are NEW in this PR. I deliberately skipped populate_db.py, build_nav.py, insert_optimized_media.py, sync_kdoc_json_to_db.py and build-kotlin-docs.yaml — I verified by blob hash that those are byte-identical to #21, where I've already left 11 findings. Everything below is specific to this PR.

Blocking (2):

  1. CLAUDE.md makes a factual claim about templateId that the same commit falsifies — and reasons from it to a scope conclusion.
  2. nav.peb emits a nav-hidden class that nothing consumes, so entries meant to be hidden render visible.

Non-blocking (3) — real, but lower severity or arguably intentional; flagged for your judgment rather than as merge blockers.

Context worth stating plainly: this PR is a clean split from #21, and the split itself is well executed — README.md correctly documents the ADFA-5039 dependency, and I checked that this PR is not shipping a stale pre-fix version of anything. A number of things I went looking for came back clean: build-stdlib-json-docs.sh's restore trap genuinely fires on both failure and SIGINT (verified with timeout -s INT) and is registered before the file swap, so there's no race window; optimize_media.py handles pngquant failures and animated images with explicit fallback-and-log rather than silent data loss, and returns a non-zero exit on errors; docs.css/sidebar.js/tabs.js/nav.peb agree on breakpoints and class names; and the README's documented CLI invocations match the argparse definitions exactly.

I also considered and did not file the page.peb "lone <tab>" workaround here — the root cause is md_to_json.py's tag regex, which isn't in this PR, and I've already filed it as blocking on #23.

Comment thread CLAUDE.md Outdated
Comment on lines +86 to +88
scope for this repo. (A repo-wide search for `templateId`, `Templates`, `Bookshelf`,
`BookCategories`, or `PUCC` turns up zero matches outside `WebServer.kt` itself, which is
consistent with that division of responsibility.) Concretely, relative to the schema above:

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.

Blocking — this claim is false as of the commit that adds it, and it's load-bearing.

The parenthetical asserts a repo-wide search for templateId "turns up zero matches outside WebServer.kt", and the surrounding sentence uses that to conclude the concern "is not a gap to fill; it's out of scope for this repo."

But this same commit adds populate_db.py, which references templateId at lines 87, 98, 145, 261, 266, 276 and 286 — including the actual insert:

INSERT INTO Content (path, languageID, content, contentTypeID, templateId) VALUES (...)

and insert_optimized_media.py, which filters on it at lines 214-215 and 256 (... WHERE ... AND templateId != 0). sync_kdoc_json_to_db.py:13 mentions it too.

So the grep this sentence invites the reader to trust returns numerous in-repo hits the moment this PR lands.

What makes this worth blocking rather than a doc nit: CLAUDE.md exists to orient future readers and agents, and this isn't a stale aside — the false premise is used to justify a scope boundary. An agent reading this will conclude the repo doesn't touch templateId and may "helpfully" strip it from exactly the INSERT that needs it.

Suggest either dropping the parenthetical or narrowing it to the Templates/Bookshelf/BookCategories/PUCC names, which I did not find outside WebServer.kt.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 66da59d - dropped templateId from the "zero matches outside WebServer.kt" list, keeping only Templates, Bookshelf, BookCategories, PUCC (the names that actually are absent), and added a clarifying note that populate_db.py/insert_optimized_media.py do read/write templateId directly since it's a plain column on Content they populate.

</nav>

{% macro renderNavNode(node) %}
<li class="nav-item{% if node.hidden %} nav-hidden{% endif %}">

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.

Blocking — the nav-hidden class is inert, so hidden nav entries render fully visible.

This line emits nav-hidden for nodes the tree marks hidden, and the header comment at line 10 documents hidden as "true for entries Writerside hides from the primary tree." But nothing anywhere consumes the class. Verified:

grep -c 'nav-hidden' assets/docs.css            -> 0
grep -n  'hidden'     assets/sidebar.js          -> (no matches)
grep -c 'nav-hidden' templates/nav.html          -> 29

I read docs.css in full (227 lines): the only display: none rules are for .nav-item > .nav-subtree, .nav-toggle and .nav-backdrop — none for .nav-hidden. And page.peb links only /assets/docs.css, so there's no other stylesheet that could supply the rule, and nothing injects CSS elsewhere.

Concrete effect: the committed nav.html already carries 29 nav-hidden entries — the Kotlin tour steps, e.g. "Hello world" — and every one of them shows in the sidebar identically to a normal entry. Writerside hides those individual tour steps from the primary nav by design, so the shipped sidebar gets cluttered with dozens of entries that were never meant to be listed.

One .nav-hidden { display: none; } rule in docs.css closes it. Worth deciding deliberately though — if these should actually be visible, the class and its comment are the thing to remove.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 66da59d - added .nav-hidden { display: none; } to assets/docs.css, matching the documented intent ("entries Writerside hides from the primary tree"). Went with hiding them rather than removing the class.

Comment thread CLAUDE.md Outdated
Comment on lines +150 to +155
design plus a flowchart image; there is no code here yet. The actual implementation (the Dokka
`JsonRenderer`/`ModelMapper`/`LinkPostProcessor` plugin, its test suite, and the
`kotlin-stdlib-docs` build scripts) exists only on the unmerged branch **`fix/ADFA-4514`**. That
branch's diff against `main` also shows it removing recent `docdb-studio` work and all of
`scripts/pdfjs/` — almost certainly because the branch was cut before those were added and hasn't
been rebased, not because it intends to delete them. **Flagged: rebase `fix/ADFA-4514` onto

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.

Non-blocking — this section describes a branch state that is already out of date.

The text says Dokka-plugin-kdoc2json/ on main is "just a README.md describing the intended design plus a flowchart image; there is no code here yet", that the implementation "exists only on the unmerged branch fix/ADFA-4514", and flags "rebase fix/ADFA-4514 onto current main before merging". Line 183 repeats the rebase item in the decisions log.

fix/ADFA-4514 is already merged — 4c6b8aef ("Merge pull request #18 from appdevforall/fix/ADFA-4514") is on main, and git ls-tree -r main shows JsonOutputPlugin.kt, JsonRenderer.kt, ModelMapper.kt, LinkPostProcessor.kt, the test suite and the kotlin-stdlib-docs build scripts all present on main today.

So a reader is told a whole plugin implementation is missing from main and that a rebase is still outstanding, when both are resolved.

I've marked this non-blocking because it's a point-in-time note that was presumably true when drafted, and unlike the templateId claim above nothing reasons from it. But it's the kind of staleness that a repo-orientation doc is specifically supposed to avoid, and it'll mislead the next agent that reads it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 66da59d - rewrote the Dokka-plugin-kdoc2json/ bullet to describe it as merged (fix/ADFA-4514, 4c6b8aef) with the actual files present on main, and removed the now-resolved rebase item from the decisions log (was line 183).

Comment on lines +25 to +26
- `pip install markdown-it-py Pillow scour brotli`
- `cairosvg` (only needed if an optimized SVG exceeds `--svg-rasterize-threshold`): `pip install cairosvg`

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.

Non-blocking — bare pip install / python3, against the repo's own established pattern.

This is the same objection I left as blocking on #23's review_build_json.sh, so flagging it here for consistency rather than re-litigating it.

Our convention (~/.claude/CLAUDE.md) is explicit: "Always use uv for Python project management. Never use pip, pip3, or the uv pip compatibility shim", and "never invoke python/python3 directly outside a uv run context."

In this PR that shows up in several places:

  • these two lines (pip install markdown-it-py Pillow scour brotli, pip install cairosvg)
  • README lines 53, 56, 59, 72, 90, 109, 115 — every usage example is python3 <script>.py ...
  • optimize_media.py:47-48 (docstring) and :302 (a runtime RuntimeError telling the user to pip install cairosvg)
  • run_e2e_pipeline_test.sh lines 100, 105, 125, 141, 145, 175 — six bare python3 invocations

This isn't only a global-preference thing: docdb-studio/ and check-tools/ in this same repo each ship a pyproject.toml + uv.lock and document uv sync / uv run, and docdb-studio/CLAUDE.md says "Dependencies are managed with uv. Always use uv run."

Related and independently worth fixing: scour and cairosvg are not in requirements.txt (unchanged by this PR), so they exist only in these prose instructions.

I've left this non-blocking because these are developer-facing helper scripts and you may have deliberately kept them dependency-light — but if so, that's worth a line in the README saying so.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Non-blocking, addressed in 66da59d rather than just documented as intentional - converted every pip install/bare python3 reference (README usage examples, optimize_media.py's docstring + RuntimeError message, and all 6 invocations in run_e2e_pipeline_test.sh) to uv run --with-requirements <repo-root>/requirements.txt, matching the review_build_json.sh fix on #23. Also added scour/cairosvg to requirements.txt since they were only ever in prose.

Comment on lines +97 to +98
except Exception as exc: # noqa: BLE001 - surface which file broke, keep auditing the rest
print(f"error scanning {md_path}: {exc}", file=sys.stderr)

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.

Non-blocking — per-file scan failures are swallowed and the script still exits 0.

The except catches everything from converter.convert_file(...), prints to stderr, and continues — but nothing counts the failures, and main() has no sys.exit(1) path except the directory-validation check at line 82. So if every file in the corpus raised, this still exits 0 and prints a report claiming nothing is broken.

That matters because of how the script is positioned: it's step 1/5 in run_e2e_pipeline_test.sh:100, and the README describes it as the pre-flight gate where you "fix anything broken in the source before converting it." set -euo pipefail in the shell script can't help — exit 0 is exit 0.

Same shape I flagged as blocking on populate_db.py:483 (#21) and md_to_json.py:607 (#23). I've scored it lower here because this tool only writes an advisory markdown report and doesn't mutate the database, so a bad run misleads rather than corrupts.

Counting failures and exiting non-zero — or at minimum printing a N files failed to scan line into the report itself — would make a broken run distinguishable from a clean one.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 66da59d - added a failure counter and --allow-failures flag mirroring md_to_json.py's pattern from #23: sys.exit(1) if any file failed to scan (unless the flag is passed), and a "N file(s) failed to scan" line (with an explicit "this report is incomplete" marker when non-zero) in the emitted report itself. Covered by new tests in tests/test_find_missing_assets.py, which stub the not-yet-merged md_to_json import via PYTHONPATH since that module isn't on this branch until #23 lands.

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 66da59d addressing your review. Replied inline to each of the 5 line comments individually; summary here.

Blocking (2)

  1. CLAUDE.md's templateId "out of scope" claim was false as of this same PR (line 88) - dropped templateId from the "zero matches outside WebServer.kt" parenthetical, keeping only Templates/Bookshelf/BookCategories/PUCC (the names that actually are absent), and added a note that populate_db.py/insert_optimized_media.py do read/write templateId directly.
  2. nav-hidden was inert (templates/nav.peb:38) - added .nav-hidden { display: none; } to assets/docs.css, so Writerside-hidden nav entries (the 29 tour-step cases you found in the committed nav.html) are actually hidden.

Non-blocking (3)

  1. Stale Dokka-plugin-kdoc2json branch state (CLAUDE.md:150-156, decisions log line 183) - rewrote the bullet to describe the plugin as merged (fix/ADFA-4514, 4c6b8aef) with the actual files present on main, and removed the resolved rebase item from the decisions log.
  2. pip/bare python3 vs. the repo's uv convention - converted every usage example in README.md, optimize_media.py's docstring + RuntimeError message, and all 6 invocations in run_e2e_pipeline_test.sh to uv run --with-requirements <repo-root>/requirements.txt. Also added scour/cairosvg to requirements.txt, since they'd only ever existed in prose.
  3. find_missing_assets.py swallowed per-file scan failures and exited 0 regardless (line 98) - added a failure counter, a --allow-failures opt-out flag, sys.exit(1) on any scan failure, and a "N file(s) failed to scan" line in the report itself (with an explicit "this report is incomplete" marker when non-zero) - mirroring the pattern md_to_json.py uses on ADFA-5039: Convert kotlin-web-site docs to JSON #23.

Tests

Added tests/test_find_missing_assets.py covering item 5's exit-code/report behavior. Since md_to_json.py isn't on this branch yet (lands with #23), the tests stub that import via PYTHONPATH and run the script as a subprocess rather than importing it directly. 3/3 pass.

Verified the uv run --with-requirements invocations actually work (installed uv locally and ran optimize_media.py --help plus the heredoc pattern used in the e2e script through it) and that run_e2e_pipeline_test.sh is still syntactically valid (bash -n). Full end-to-end execution of find_missing_assets.py/populate_db.py still isn't possible here until #23 merges, per the PR description.

@alexmmiller
alexmmiller dismissed hal-eisen-adfa’s stale review August 10, 2026 18:57

Sent prematurely, apologies

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.

2 participants