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

[alexmmiller]

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

[hal-eisen-adfa]

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.

[hal-eisen-adfa]

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.

[alexmmiller]

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.

[hal-eisen-adfa]

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.

[alexmmiller]

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.

[hal-eisen-adfa]

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.

[alexmmiller]

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

[hal-eisen-adfa]

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.

[alexmmiller]

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.

[hal-eisen-adfa]

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.

[alexmmiller]

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.

[alexmmiller]

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)

3. 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.
4. 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.
5. 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.

Sent prematurely, apologies

[alexmmiller]

Review: database-insertion path

Focused pass over the files that write to documentation.db. Four things on this branch do:

File	What it writes
ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/populate_db.py	wipes and rebuilds k/html/* + assets/*
ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/insert_optimized_media.py	replaces k/html/images/*, rewrites pages, deletes unreferenced media
scripts/sync_kotlin_stdlib_docs/sync_kdoc_json_to_db.py	overwrites/deletes k/kotlin-{stdlib,reflect,test}/* + orphaned tooltips
.github/workflows/build-kotlin-docs.yaml:244	one inline INSERT OR IGNORE INTO ContentTypes

optimize_media.py, build_nav.py, and find_missing_assets.py never open a connection.

Two things that are right, up front

Content types are handled correctly throughout. Every type is resolved by value through get_content_type/get_id, which raise if the row is missing; extension mapping is an explicit allowlist (EXTENSION_TO_CONTENT_TYPE) that skips-and-warns on an unknown extension rather than falling back to text/plain; compression is read from ContentTypes.compression rather than assumed. There is not one hardcoded contentTypeID in the PR. This matters because ~34% of the .html rows in the current production DB are typed text/plain, caused by exactly the two anti-patterns this PR avoids (a text/plain fallback for unrecognized extensions, and hardcoded positional type IDs). The text/html-for-JSON-storage choice at populate_db.py:149 is deliberate and correct — templateId points the server at page.peb, so the served type is HTML.

The big DELETE is correctly scoped. populate_db.py:530's DELETE FROM Content WHERE path LIKE 'k/html/%' OR path LIKE 'assets/%' hits 489 rows against the current production DB (267 pages + nav, 222 images) plus the 3 assets — exactly what the script reinserts. The 3,264 k/ rows outside k/html/ (stdlib, disambiguation) are untouched.

Findings

1. HIGH — a conversion failure silently drops a live page and leaves a normal-looking nav link to it. populate_db.py:483

convert_file failures are caught, logged to stderr, and skipped, so the page never reaches pages and no Content row is inserted — but line 530 already deleted the old, working row in the same transaction, which then commits. Worse, topic_index_db still contains the stem (only blacklisted stems get popped at line 460), so build_node resolves it, sets no_link = False, and emits a normally-styled <a href="/k/html/<stem>.html"> with a humanize()-derived title. Net effect of one bad .md: a page vanishes from production and the sidebar links to a 404 in the same styling as every working page.

This is a regression against the standalone path — build_nav.py's load_page_index builds stem_to_id from generated JSON, so a failed page drops out of nav automatically. The in-process variant loses that property.

Minimum fix: topic_index_db.pop(md_path.stem, None) in the except, matching the blacklist path so links render styled-broken. Better: exit non-zero on any conversion failure, since CI uploads this DB to Drive and nothing currently checks that Converted {len(pages)}/{len(md_files)} is a clean ratio.

2. MEDIUM-HIGH — duplicate .md stems abort the whole run. populate_db.py:475-479

md_files iterates every *.md and derives db_id = f"k/html/{md_path.stem}". Content.path is UNIQUE (UNIQUE('path') in the live schema does enforce — SQLite resolves the quoted string as an identifier; verified), so two same-stem files in different topics/ subdirectories mean two inserts at one path → IntegrityError → rollback → raw traceback, after the full conversion pass has already run. md_to_json.build_topic_index handles exactly this case with keep-first-plus-warning, and topic_index_db inherits that dedupe — but the conversion loop doesn't. It fails safe (nothing uploaded), but it should dedupe by stem the same way rather than crash on an upstream rename.

3. MEDIUM — delete_unreferenced_media is an unguarded mass delete with no dry-run. insert_optimized_media.py:290

It deletes every k/html/images/* row not referenced by a k/html/%.html page with templateId != 0. In CI that's safe — steps 2 and 3 share if: !inputs.skip_website_docs, so pages always exist. Run standalone against a DB without those page rows and collect_referenced_media returns an empty set, so it deletes every image, including the ones inserted seconds earlier in the same transaction. There's no floor check (if not referenced: abort) and, unlike sync_kdoc_json_to_db.py, this script has no --dry-run — the most destructive of the three is the one without a preview mode.

Related blind spot: only page/nav rows are scanned, so an image referenced from assets/docs.css or a .peb template would be collected as garbage. Latent today (no url() in docs.css, no image refs in either template — checked), but it's a silent-data-loss trap for whoever adds a CSS background later.

4. MEDIUM — sync_kdoc_json_to_db.py ignores the chunking contract the rest of the PR treats as mandatory. sync_kdoc_json_to_db.py:164

It does UPDATE Content SET content = ? with the entire new blob and never chunks. Meanwhile any existing <path>-N fragment matches the prefix LIKE, fails relative_target_path's file lookup, and gets deleted. So a stdlib page crossing 1 MiB produces one oversized row that no longer matches WebServer.kt's reassembly protocol — the same protocol populate_db.py:145 argues has to be byte-exact on both sides. Latent right now (largest stdlib blob is 172 KB, and none of the 44 chunked rows in the DB are under these prefixes), but the two scripts disagree on an invariant the PR itself documents as non-negotiable.

5. MEDIUM — unknown contentTypeID silently writes uncompressed bytes and commits. sync_kdoc_json_to_db.py:154-157

compression_by_type.get(...) missing → compression = "none" → raw bytes stored in a row whose declared type says brotli. The warning goes to stderr, the transaction commits, and the server gets undecompressable content. Given that a mistyped content column already shipped to production undetected for months, this should be fatal, not a warning.

6. LOW-MEDIUM — delete-after-insert in the rename loop can remove a just-inserted row. insert_optimized_media.py:415

Inserts run first, then for old_name in rename_map: delete_content(...). If one rename's new name equals another's old name (a.png → b.webp alongside b.webp → c.webp), the delete pass removes the row the insert pass just wrote from a.png. This is precisely the chain-rename hazard rewrite_pages' docstring reasons about carefully for text substitution, and the same reasoning wasn't applied here. Unlikely with Writerside sources; the fix is just moving the delete loop above the insert loop.

7. LOW — unescaped LIKE wildcards. insert_optimized_media.py:103

delete_content does path LIKE ? with f"{path}-%" built from a filename. _ and % are wildcards. No current image name contains either, but NAV_CONTENT_PATH is k/html/_nav.html, so the pattern is already k/html/_nav.html-% with a live wildcard in it. Add ESCAPE '\' and escape the parameter. Every other LIKE in the PR uses literal prefixes and is fine.

8. LOW — image_index_db assumes a flat zip and would break silently otherwise. populate_db.py:467

{name: name for name in image_names} keys on the full zip entry, but Converter.resolve_image_src looks up src.rsplit("/", 1)[-1], and md_to_json.build_image_index maps bare-name → rel-path. A nested zip entry gives a key nothing can match, so images resolve as missing while the row lands at k/html/images/sub/foo.png. {Path(name).name: name} makes it match the documented contract at no cost.

9. LOW — the three scripts back up two different ways. populate_db.py:212 and insert_optimized_media.py use VACUUM INTO (documented as WAL-safe); sync_kdoc_json_to_db.py:39 uses shutil.copy2. The live DB is journal_mode=delete, so this isn't currently biting, but the PR's own docstring makes the argument for VACUUM INTO and then one script doesn't follow it. Also worth noting all three run in CI against the same ~300 MB file, so a run keeps three full backup copies on the runner.

Nits

• build-kotlin-docs.yaml:151 says scour/cairosvg are "not in the root requirements.txt" — this same PR adds them to requirements.txt, so they're installed twice and the comment is stale.
• .gitignore's *.db doesn't match documentation.db.backup-<ts> or documentation.db.bak.<ts>, so every local run leaves untracked noise in git status.

Merge order

The PR body's caveat holds — populate_db.py, build_nav.py, and find_missing_assets.py all import md_to_json, which isn't in this diff, so nothing here is runnable until #23 lands. This review was done against #23's copy of md_to_json.py.

[alexmmiller]

Build Kotlin Docs now runs end-to-end — 06a43f5

Pushed 06a43f5. Context for reviewers: this workflow had never executed anywhere. gh api .../workflows/341694427/runs reports total_count: 0, and both boxes in this PR's own test plan are still unchecked. I ran it locally under act against a copy of the real 248 MB documentation.db, which turned up two blockers that stopped step 4/5 dead before it generated anything.

Step 4/5 could not build — two independent causes

1. It needs a JDK 8 toolchain that nothing installs. kotlin-stdlib-docs's own plugins:dokka-samples-transformer-plugin and plugins:dokka-version-filter-plugin each hardcode kotlin { jvmToolchain(8) }, and dokkaGenerateModuleJson depends on dokkaGeneratePublicationHtml — so their dependencies have to resolve even though we only ever want JSON out. With just the JDK 17 the workflow installs, Gradle dies at task-graph resolution:

> Failed to calculate the value of task ':plugins:dokka-samples-transformer-plugin:compileJava'
  property 'javaCompiler'.
   > Cannot find a Java installation on your machine (Linux ... aarch64) matching:
     {languageVersion=8, ...}. Toolchain download repositories have not been configured.

This only survives on a runner that happens to have an EOL JDK 8 lying around for Gradle's auto-detection — which is an undeclared dependency on the runner image, and is exactly what a self-hosted runner or an act container won't have. The injected build.gradle.kts now retargets those two subprojects at the running JVM (not a hardcoded 17, so it can never request a toolchain that isn't installed). The pipeline needs exactly one JDK.

2. It resolved stdlib artifacts that are published nowhere. kotlin_big pulls the real kotlin-stdlib/-reflect/-test binaries from <kotlin-root>/build/repo at the checkout's own defaultSnapshotVersion (2.5.255-SNAPSHOT) — artifacts that exist only after a full local build of the kotlin repo. Against git clone --depth 1 that resolves to nothing:

> Could not find org.jetbrains.kotlin:kotlin-stdlib:2.5.255-SNAPSHOT

Upstream honours -PkotlinLibsRepo/-PdeployVersion only under TeamCity. They're now ordinary Gradle properties, plumbed through build-stdlib-json-docs.sh, both workflows, run_e2e_pipeline_test.sh and the act runner as kotlin_libs_version (default 2.4.10) / kotlin_libs_repo. kotlin_big already declares mavenCentral(), so a released version needs no repo override at all — kotlin_libs_repo is there for a private/snapshot repo. Documenting the stdlib now takes ~3 minutes instead of a Kotlin rebuild.

Also in this commit

• run-build-kotlin-docs-with-act.sh retargeted. It drove build-kotlin-docs.yaml, whose WIF auth can never succeed under act, and hard-required all five Drive/Slack secrets. It now drives build-kotlin-docs-local.yaml, needs no secrets, takes host paths and bind-mounts them, and passes --container-daemon-socket - (colima can't bind-mount its socket).
• New skip_stdlib_docs input, the mirror of skip_website_docs; setting both is rejected. Skips the slowest half when iterating on website content.
• Blacklist de-duplicated. The ADFA-4737 trio was spelled out twice per workflow — once applied in step 2/5, once restated in the verification step — so the verification could drift onto a different list and still report PASS. Now one BLACKLISTED_ELEMENT_TITLES job env var read with mapfile -t at both sites, matching what run_e2e_pipeline_test.sh already did correctly.
• populate_db.py backup moved. Its ~250 MB VACUUM INTO backup ran before conversion — i.e. before the last step that can still refuse to write — so every run that bailed on a conversion failure left a full copy behind.
• populate_db.py no longer runs pngquant. It quantized every PNG on insert, all of which insert_optimized_media.py replaces moments later. Measured on the live corpus: 0 PNG rows survive step 3 (161 become .webp; the remaining 51 are .svg/.gif, which pngquant can't touch anyway). Step 2/5 went 35.4s → 14.5s.
• Documented that act does not apply workflow_dispatch input defaults — which inverts dry_run and would write the rebuilt database back over db_path. The runner script always passes every input explicitly.

Verification

Both halves run green under act against a copy of the real production database:

Website half	267 pages, 217 image rows (161 .webp), 3 assets, page.peb/nav.peb upserted, PASS: all 38 blacklisted topic page(s) confirmed absent
Stdlib half	Step 4/5 3m15s, Step 5/5 37s, 3132 k/kotlin-stdlib/* rows
Compression	sampled page, nav and stdlib rows all decode with brotli -D against the stored 256 KiB dictionary and fail under plain brotli — i.e. genuinely dictionary-compressed, and the existing dictionary was reused, never retrained
Tests	158 (ProcessKotlinWebsiteJSON + sync_kotlin_stdlib_docs) + 189 (docdb-studio) pass

Workflow parity is unchanged: 14 byte-identical shared steps, 2 differing only in Drive-specific packages (zip, google-api-python-client & friends), and 14 one-side-only steps confined to the Drive boundary. kotlin_libs_version/kotlin_libs_repo are shared with identical defaults, and Step 4/5's run block is identical in both files.

Worth a reviewer's eye

• kotlin_libs_version defaults to 2.4.10 while kotlin_ref defaults to master, so the out-of-the-box pairing documents 2.4.10 binaries against master's build scripts. It resolves and builds — that's what the run above did — but the coherent production setting pins both together. Deliberate default, flagging it so it's a decision rather than a surprise.
• The Java-8 fix compiles those two Dokka plugins for the running JVM instead of 8. They're only ever loaded in-process by Dokka under that same JVM, so this should be inert, but it is a change to how upstream's subprojects get built.
• All five findings from @hal-eisen-adfa's earlier review remain fixed; I re-verified each against the current head while working through this.

[hal-eisen-adfa]

Code review: 15 findings — 2 critical, 3 high, 10 medium

Reviewed the full main...HEAD diff (38 files, ~10,150 insertions). Inline comments follow, tagged F01–F15. Where a comment says "Reproduced" or "Verified", the failure was actually run against a scratch database or temp directory and the numbers are from that run — not inferred from reading.

The two criticals are the ones I'd want fixed before this merges. Both destroy content and then exit 0 with truthful-looking statistics:

• F01 — two images that collide on a rewritten extension (logo.png + logo.jpg → logo.webp) silently become one; the loser is gone and its pages render the survivor. On the live path via --webp.
• F02 — migrating page X DELETEs an unrelated page that merely happens to be named X-1, then reports errors 0. 122 rows in, 121 out, no diagnostic.

---

Test coverage for these 15 findings: zero

All 158 tests on this branch pass — 131 in ProcessKotlinWebsiteJSON, 27 in sync_kotlin_stdlib_docs — and not one of the 15 findings above is caught by any of them. That's a deduction, not a guess: each bug reproduces on demand, so if any test constructed the triggering input, that test would be red.

What the gaps have in common is that nothing in the suite feeds these functions data they didn't expect:

Finding	Input no test constructs
F01	Two source images in one directory that collide on a rewritten extension
F02	An unrelated X-1 page sitting alongside X in the migration input
F03	A second run of build_nav.py over a directory containing its own nav.json
F04	Any assertion on final file size or freelist pages, so a missing VACUUM is invisible
F06 / F08	A fragment chain starting at -2, or one with an interior gap (p-1, p-2, p-4)
F07	An invocation where the input and output directories are the same path
F09	Dokka output containing a page with no pre-existing Content row
F10	A tooltip URI carrying ?query rather than #fragment

F06 and F08 stand out: renumber_misnumbered_fragments.py was added in this PR specifically to repair chains that start at -2 or have gaps, yet no test builds either shape. The repair tool and the bug it should have caught arrived in the same changeset.

For the two criticals especially, the regression tests are cheap — both reproduce in a handful of lines against a tmp SQLite DB or tmp dir — and both failure modes are permanent, silent data loss, which is exactly where a test earns the most.

Separately: no workflow in this repo runs pytest at all

This PR adds roughly 1,300 lines of regression tests that cover the data-loss paths, and none of them execute in CI. Whatever coverage exists today holds only as long as each person remembers to run it locally. Worth adding a pytest job before the suite grows further — otherwise the tests this PR contributes will quietly rot.

---

Lower-severity observations (not commented inline)

• build_nav.py:64 resolves duplicate stems last-wins over an unsorted rglob, while md_to_json.build_topic_index does first-sorted-wins with a warning — nav output isn't reproducible across machines.
• build_nav.py:140 render_node does no HTML escaping despite claiming to be "byte-identical to nav.peb's renderNavNode" (Pebble autoescapes); a toc-title containing " breaks out of the aria-label attribute.
• populate_db.py:694 pops topic_index_db mid-loop while Converter holds the same dict by reference, so under --allow-conversion-failures only pages converted after the failure get broken-link styling.
• sync_kdoc_json_to_db.py:276 takes the VACUUM INTO backup before the MAX_DELETE_FRACTION precheck can abort, so runs that refuse to proceed still write a full-size copy.
• optimize_media.py:267 defaults loop to 0, turning a play-once GIF into an infinite loop.
• docdb_studio.py:1317's dictionary cache is never invalidated, which remint_dictionary.py (added in this same PR) can stale out.
• CLAUDE.md ships stale: says "three workflows" while adding two, points at a scripts/kotlin/build-stdlib-json-docs.sh path that doesn't exist, and claims nothing here writes to Templates while populate_db.py:482 does.

Checked and cleared

Recording these so nobody re-treads them — all were plausible suspects that held up:

• renumber_chain's two-pass parking rename is genuinely collision-free against UNIQUE(path) in both shift directions.
• fragment_chain's LIKE over-match plus regex re-check is exact (the _/% tests pass).
• upsert_template's ON CONFLICT(name) works against the real UNIQUE('name') quirk — confirmed in sqlite3; SQLite parses it as a column constraint.
• insert_chunked_content and sync_kdoc's write_content chunk identically and are idempotent across repeated runs.
• The two workflow files haven't diverged in any way that weakens the local one (full diff -u).
• The blacklist survives YAML → mapfile → parse_blacklist_path intact.
• The build.gradle.kts DSL changes compile and were exercised on Gradle 8.14.4.
• PAGE_PEB_STATIC_ASIDE still matches page.peb byte-for-byte, and the DB template rewrite produces the right data-nav-src.
• Nav ids, data-nav-id, and sidebar.js's pathname matching line up correctly on the database-served path.

[hal-eisen-adfa]

F01 · critical · silent data loss

encode_raster rewrites the destination to dst.with_suffix(".webp") (and optimize_svg does the same to .png at L350) with no detection that two different sources now map to one output. logo.png and logo.jpg in the same directory both become logo.webp; whichever one the rglob loop at L438 reaches second silently clobbers the first.

Reproduced end-to-end: exit 0, no warning, both DB rows marked [REMOVED], a single logo.webp inserted, and both pages rewritten to point at it. The JPEG is permanently gone and its pages render the PNG. The same collapse happens when an oversized diagram.svg rasterizes onto an existing diagram.png.

This is on the live path — --webp is passed at .github/workflows/build-kotlin-docs.yaml:301 and run_e2e_pipeline_test.sh:145. The seen_names guard in insert_optimized_media.py:469 cannot help, because the collision already happened in the work dir before that code runs.

Suggested fix: track claimed output paths inside the loop and either disambiguate (logo-jpg.webp) or hard-fail on collision. Silently dropping a source image shouldn't be reachable.

[alexmmiller]

Fixed in d9df0f0. Reproduced first (errors 0, both DB rows [REMOVED], one logo.webp, the JPEG gone) — thank you for the precise repro.

Went with disambiguation rather than hard-fail: a future upstream image pair shouldn't be able to break the whole pipeline, and the rename flows out through optimize_directory's existing renamed map, so rewrite_pages repoints the stored URLs automatically. Since optimize_raster/optimize_svg own the final extension, I de-conflict the stem up front instead of predicting the suffix: first in sorted order keeps it, later ones fold their original extension in (logo.jpg -> logo-jpg.webp), with a numeric suffix if that still collides. A warning is logged either way.

Checked the real webHelpImages.zip: 299 entries, 0 colliding stems — so this was latent, not live. Covered by test_sources_differing_only_by_extension_both_survive, a 3-way variant, and test_non_colliding_names_keep_their_own_stems so the de-confliction can't start renaming things that didn't collide.

[hal-eisen-adfa]

F02 · critical · silent data loss

write_item deletes everything in existing - wanted, and wanted comes from load_base_rows (L301), which classifies any path matching X-<digits> as a continuation chunk of X whenever X also exists — with no chunk-size test.

So two independent pages that merely happen to be named X and X-1 (neither one chunked) get misread as a single chunked item. Migrating X produces wanted = {}, and this line DELETEs X-1 outright. Worse, the victim was already excluded from base_rows, so it is never scanned, never counted, and never reported.

Reproduced: 122 rows in, 121 rows out, and the run printed scanned 121, migrated 121, errors 0.

Two things widen the blast radius: remint_dictionary.py imports this same write_item, and verify_remint_dictionary.py can't catch it because verify() enumerates from load_base_rows too — same blind spot.

The correct test already exists in this PR: renumber_misnumbered_fragments.py:19 keys on "content length is exactly CHUNK_SIZE". And load_base_rows already SELECTs LENGTH(C.content) without using it. Gating the continuation classification on that length should be a small change.

[alexmmiller]

Fixed in d9df0f0. Reproduced exactly as you described — 122 rows in, 121 out, scanned 121, migrated 121, errors 0, and the victim never even appeared in load_base_rows.

Both sites needed it, not just the classifier: write_item's existing came from fragment_chain, which matches every X-<digits>, so the DELETE would still have hit the row even with load_base_rows fixed. Both now gate on is_chunked_base — the base row actually being CHUNK_SIZE bytes.

One correction worth recording: my first attempt also required a contiguous walk from -1, which broke test_chain_numbered_from_minus_two_is_migrated_not_miscounted. An ADFA-5171 chain is still a real chain, so the gate is the base length only, exactly as you suggested. Chain membership stays with fragment_chain so -2 still resolves. write_item now resolves existing before the UPDATE, since it's the base row's current length that marks it chunked.

Covered by test_independent_page_named_like_a_fragment_is_not_a_continuation, test_write_item_does_not_delete_an_unrelated_lookalike_page, and test_genuinely_chunked_base_still_owns_its_continuations (the -2 case).

[hal-eisen-adfa]

F03 · high

load_page_index runs json.loads(...) then page.get("id") over every *.json under docs_json_dir. But main() writes nav.json — a top-level array — into output_dir, and the README documents the invocation (L82) with the output dir as the scan dir:

build_nav.py <docs-root> <output-dir> <output-dir>

So the second run reads its own previous output. Reproduced: AttributeError: 'list' object has no attribute 'get'. Either skip the generated nav.json by name, or guard on isinstance(page, dict).

[alexmmiller]

Fixed in d9df0f0 — isinstance(page, dict) guard, so anything that isn't a page object is skipped rather than crashing. Covered by test_load_page_index_skips_the_generated_nav_json, which writes both a page and a nav.json array into one directory.

Also took the build_nav.py:64 point from your lower-severity list while I was here: the rglob is now sorted() and duplicate stems resolve first-wins with a warning, matching md_to_json.build_topic_index — which is what the pages themselves were converted against, so the two now agree and the output is reproducible.

[hal-eisen-adfa]

F04 · high

This script rewrites every k/kotlin-stdlib* blob, deletes Content rows plus their fragment chains, and deletes Tooltips/TooltipButtons rows — then just commits. There's no final VACUUM, which every other writer in this repo does: populate_db.py:851, insert_optimized_media.py:507, renumber_misnumbered_fragments.py:166, docdb_studio.py:694.

Verified: a run that shrank content down to 27 bytes left the file at 3,178,496 bytes with 768 of 776 pages sitting on the freelist.

This is step 5/5 of the pipeline, so nothing downstream reclaims it — the bloat ships in the on-device database.

[alexmmiller]

Fixed in d9df0f0 — trailing VACUUM on its own connection after the transaction closes, same shape as populate_db.py and insert_optimized_media.py, skipped on --dry-run (nothing was committed, so there's nothing to reclaim). Module docstring updated to say so.

[hal-eisen-adfa]

F05 · high · regression

_find_brotli_cli() is called here while building the argv — i.e. before the plain-Brotli fallback at L1457 can run — and the BrotliCliMissing it raises isn't caught by the surrounding except OSError.

Result: on any database that has a CompressionDictionary, plain-Brotli rows become unreadable without the CLI installed. Those are exactly the rows the docstring calls the "load-bearing, not defensive" fallback: plugin-contributed rows and partially-migrated DBs. brotli.decompress reads them fine, and did read them before this PR. The user gets told to install brotli, which won't fix that row.

Resolve the CLI lazily (or catch BrotliCliMissing alongside OSError) so the fallback stays reachable.

[alexmmiller]

Fixed in d9df0f0. Confirmed the mechanism: BrotliCliMissing subclasses brotli.error, not OSError, so it went straight past the except OSError and the fallback below was unreachable.

Narrower fix than "catch it alongside OSError", though — that would have made a genuinely dictionary-compressed row fall back to plain decoding and fail with a confusing error, and it breaks test_missing_brotli_cli_is_reported_not_swallowed, which deliberately asserts the missing CLI is reported. So: resolve the CLI first; on BrotliCliMissing, try plain decoding and return it if it works (your case — the plain row), and re-raise the original BrotliCliMissing if it doesn't (a real dictionary row, where "install brotli" is the right advice). Verified both directions by hand.

[hal-eisen-adfa]

F11 · medium

Under set -euo pipefail (L49), this command substitution kills the script when grep matches nothing — so the friendly if [ -z "$DOKKA_VERSION" ] message at L107-110 is dead code and can never print.

A kotlin ref that renames the dokka catalog key fails "Step 4/5" with exit 1 and zero diagnostic output. Appending || true, or splitting the grep and sed into separate steps, restores the intended message.

[alexmmiller]

Fixed in d9df0f0 — || true on the substitution, so the if [ -z "$DOKKA_VERSION" ] message below is now reachable instead of the script dying with exit 1 and no output.

[hal-eisen-adfa]

F12 · medium

trap ... EXIT doesn't fire on an untrapped fatal signal, which contradicts the header's promise at L15-18 that "the kotlin checkout is left exactly as it was found."

Ctrl-C during the multi-hour Gradle build leaves the swapped-in build.gradle.kts sitting in the developer's kotlin clone and orphans the mktemp original. trap restore_build_gradle EXIT INT TERM HUP covers it.

[alexmmiller]

Fixed in d9df0f0 — trap restore_build_gradle EXIT INT TERM HUP, so the header's promise that the checkout is left exactly as found now holds for Ctrl-C during the Gradle build.

[hal-eisen-adfa]

F13 · medium

flat_nav / id_to_index include nav nodes whose id was synthesized by build_node for an unconverted *.topic (build_nav.py:122), and which therefore have no Content row.

This is already visible in the committed output — templates/nav.html:1285 carries data-nav-id="api-references". Nav rendering colours that node as a non-link, but the pager doesn't: its two neighbours in document order get prev/next rendered by page.peb:41-42 as ordinary pager links to a hard 404.

Filtering synthesized ids out of flat_nav before building the pager would make the two agree.

[alexmmiller]

Fixed in d9df0f0 — flat_nav is filtered to ids that actually have a page before the pager is built, so prev/next and the sidebar agree on what's reachable. Confirmed your example: api-references is in the committed nav.html as data-nav-id, and its neighbours were getting pager links to a 404.

[hal-eisen-adfa]

F14 · medium

The lock signalling is asymmetric. This "Grabbing baton" notification is ungated and fires on every run, while 'Notify Slack: build complete' at L427 carries if: ${{ !inputs.dry_run }} plus the implicit success().

Since dry_run defaults to true (L119), an ordinary run grabs the baton and never drops it — and so does any run that fails somewhere in the 3-hour build. The channel ends up showing a baton held forever by a dead run, which is the exact failure the convention exists to prevent.

Gate the two notifications identically, and consider if: always() on the release so a failed build still drops the baton.

[alexmmiller]

Fixed in d9df0f0 in both workflows. You're right that the asymmetry was the bug and that dry_run defaulting to true made it the normal case.

Went with if: always() on the release rather than matching the grab's gate: a failed build has to drop the baton too, which a plain !dry_run wouldn't do. Since it now always fires, the message reports the actual outcome — failed / dry run / updated — instead of always claiming an update.

[hal-eisen-adfa]

F15 · medium · silent corruption

get_compression_dictionary catches sqlite3.OperationalError — which covers "database is locked" — and returns None. compress_for_storage then reads that None as "this database has no dictionary" and writes a plain Brotli row into a dictionary database. The only signal is a stderr line no GUI user will see. (Not caching the failure limits later calls, but does nothing for the call that's writing right now.)

This is reachable from import_content_files itself: phase 1's orphan DELETE holds a write transaction on connection #1 while the dictionary read opens connection #2 against the same file.

Either propagate the error instead of returning None, or warm the dictionary before opening the write transaction.

[alexmmiller]

Fixed in d9df0f0. get_compression_dictionary takes strict=, and compress_for_storage passes it: an indeterminate answer now raises instead of being read as "no dictionary" and writing a plain row into a dictionary database.

Kept the lenient default for the read paths deliberately — the existing docstring's reasoning still holds there, and decoding a dictionary row without the dictionary raises loudly rather than returning wrong bytes, so a retry is safe. It's only the write path that can't tell the two apart.

[alexmmiller]

All 15 findings fixed, with regression tests and a CI job — d9df0f0

Thanks for this — the two criticals were both real and both reproduced before I touched anything. Replies are on each thread; this covers the two structural points and what I did not do.

On "test coverage for these 15 findings: zero"

Fair, and the deduction was right. Added tests/test_review_findings.py — 11 tests over the inputs you listed as unconstructed.

I checked the claim rather than assuming it. Stashing the source fixes and running the new tests against the pre-fix code:

8 failed, 3 passed
FAILED test_sources_differing_only_by_extension_both_survive        (F01)
FAILED test_three_way_extension_collision_all_survive               (F01)
FAILED test_optimizing_into_the_input_directory_is_refused          (F07)
FAILED test_independent_page_named_like_a_fragment_is_not_a_continuation  (F02)
FAILED test_write_item_does_not_delete_an_unrelated_lookalike_page  (F02)
FAILED test_reassemble_content_handles_a_chain_numbered_from_two    (F06)
FAILED test_chain_with_interior_gap_is_reported_as_gapped           (F08)
FAILED test_load_page_index_skips_the_generated_nav_json            (F03)

The 3 that pass both ways are deliberate must-not-regress guards: non-colliding names keep their stems, a contiguous chain from -1 is still left alone, and a genuinely chunked base still owns its continuations.

That last one earned its place immediately. My first cut at F02 gated the continuation test on a contiguous walk from -1, which is stricter than what you suggested and broke test_chain_numbered_from_minus_two_is_migrated_not_miscounted — an ADFA-5171 chain is still a real chain. The gate is now the base row's length alone, exactly as you proposed.

On "no workflow in this repo runs pytest at all"

Added .github/workflows/python-tests.yaml — push, PR and manual dispatch. Three suites, because their dependency setups differ: ProcessKotlinWebsiteJSON + sync_kotlin_stdlib_docs against the root requirements.txt, and docdb-studio through its own uv.lock. Installs pngquant, brotli and zstd, since the tests exercise the real binaries rather than mocking them.

Verified under act rather than assuming it works:

✅ Install system dependencies · Install Python dependencies
✅ ProcessKotlinWebsiteJSON + sync_kotlin_stdlib_docs   169 passed
✅ docdb-studio                                          189 passed
🏁 Job succeeded

From the lower-severity list

Fixed: build_nav.py:64 (sorted rglob, first-wins with a warning, agreeing with build_topic_index); build_nav.py:140 (render_node now escapes — which is what actually makes it byte-identical to autoescaping nav.peb; I checked the committed nav.html and no current toc-title contains &, " or <, so it isn't stale); sync_kdoc_json_to_db.py:276 (backup moved below the MAX_DELETE_FRACTION precheck); and the three CLAUDE.md inaccuracies.

Not fixed, deliberately, so they don't look handled:

• populate_db.py:694 — the mid-loop topic_index_db.pop() under --allow-conversion-failures. Fixing it properly means knowing which pages fail before converting any, which needs a second pass over the corpus; I didn't want to make that trade quietly. Only reachable with that flag.
• optimize_media.py:267 — GIF loop defaulting to 0. Small, but it needs the source's own loop count read back through Pillow, and I'd rather do it with a test than by inspection.
• docdb_studio.py:1317 — the never-invalidated dictionary cache that remint_dictionary.py can stale out. Needs a deliberate invalidation key (mtime? explicit clear on re-mint?), which is a design call rather than a patch.

Happy to take any of the three in a follow-up if you'd rather they land here.

State

358 tests pass (169 + 189, up from 347). Both Kotlin-docs workflows still parse and plan under act, and the parity between them is unchanged: 14 byte-identical shared steps, 2 differing only in Drive-specific packages, 14 one-side-only steps at the Drive boundary.

Still outstanding and not from this review: the PR is CONFLICTING against main and needs a rebase — build-kotlin-docs.yaml landed separately via e83bb9dc (#30) and #23 has since merged.

[hal-eisen-adfa]

Automated review pass (/code-review xhigh) over the full PR diff vs c627c1b.

14 inline findings below: 1 critical, 5 high, 8 medium. Two follow-up comments on the thread group them by shared root cause — F01/F02/F08/F13 (one chunking protocol, four implementations) and F04/F05/F10 (optimize_media name-planning pre-pass).

A 15th, low-severity finding is not worth an inline comment: renumber_misnumbered_fragments.is_contiguous_from_one is dead code after find_chains was rewritten to check contiguity inline — one grep hit repo-wide (its own definition), no test.

[hal-eisen-adfa]

F01 · High — unmatched_source_pages() is handed row IDs, not paths

all_rows is SELECT id, path, contentTypeID, languageID, templateId, so {row[0] for row in all_rows} is a set of integer ids. Inside unmatched_source_pages, candidate is a string like k/kotlin-stdlib/kotlin.text/index.html, which can never be in a set of ints — the membership test always fails and every source page is reported unmatched.

Run against normal Dokka output, this step prints warning: 38412 page(s) ... have no Content row and will NOT be inserted, listing pages that are in fact present and being updated. A genuinely new page — the exact condition this check was added to surface — is indistinguishable in that noise.

all_paths ({row[1] for row in all_rows}) is already computed 38 lines above and is what should be passed here.

[alexmmiller]

Fixed in 46235fb. Straightforwardly my bug — all_rows is SELECT id, path, ..., so that set was integer ids and the membership test could never match. all_paths was right there.

Covered by TestUnmatchedSourcePages, including test_row_ids_would_have_matched_nothing, which pins the actual regression (given ids, every page comes back unmatched) rather than only the fixed behaviour.

[hal-eisen-adfa]

F02 · High — unindexed full-table LIKE scan for every row updated

write_content() is called once per matched Content row (tens of thousands for kotlin-stdlib), and stale = fragment_paths(cur, path) issues SELECT path FROM Content WHERE path LIKE '<path>-%' every single time. SQLite's default LIKE is case-insensitive, so the UNIQUE(path) index cannot serve it — each call scans the whole Content table. Roughly 30k updated rows over a ~40k-row table is >1e9 row comparisons added by this PR, where the previous code was a single indexed UPDATE ... WHERE id = ?.

The scan is also unconditional: stale can only be non-empty when the existing base row was exactly CHUNK_SIZE bytes, which is essentially never for these pages. Gate it the way migrate_content_to_dictionary_brotli.is_chunked_base does, or reuse the all_paths set main() has already built.

See the standalone comment on F01/F02/F08/F13 — this is one of four divergent copies of the chunking rule.

[alexmmiller]

Fixed in 46235fb. fragment_paths now delegates to content_chunking.owned_fragment_paths, which resolves the base row's length first — an indexed lookup on UNIQUE(path) — and only reaches the LIKE when that length is exactly CHUNK_SIZE. For these pages it essentially never is, so the scan is gone from the common path entirely.

Your framing of F02 and F08 as the same misunderstanding pointing opposite directions is what made the shared module obviously right rather than just tidier.

[hal-eisen-adfa]

F03 · Medium — the justifying comment is wrong about nav.peb, so the 404 link is only half removed

build_node synthesizes page_id = 'k/html/api-references' for api-references.topic and sets noLinkColor. But nav.peb's renderNavNode branches on {% if node.id %} and emits <a class="nav-link" style="color: ..." href="/k/html/api-references.html"> — a coloured link, not a non-link. build_nav.render_node does the identical thing.

So "nav.peb colours those as non-links" is not accurate, and this fix only removes such nodes from prev/next. A reader who clicks "API reference" in the sidebar still gets a 404 from WebServer.kt.

Either drop the synthesized id for nodes with no Content row (the pager filter right below already computes exactly that set), or make nav.peb render a .nav-group-title when noLinkColor is set.

[alexmmiller]

Fixed in 46235fb, and thank you for reading the template rather than my comment — the comment was wrong. nav.peb branches on {% if node.id %} and emits a coloured <a href>; only an id-less node reaches <span class="nav-group-title">. So the previous fix removed the 404 from the pager and left it in the sidebar, which is the half a reader actually clicks.

Took your first option: clear the synthesized id for any node with no Content row. That makes nav.peb render it as a group title, and since flatten_nav_ids skips id-less nodes the pager filter falls out for free. noLinkColor is untouched, so the styling is unchanged.

Verified against the live corpus: one such node (k/html/api-references), and afterwards zero nav ids in the stored _nav.html have no Content row.

[hal-eisen-adfa]

F04 · Critical — de-confliction is scoped per source subdirectory, but the output namespace is flat

claimed is keyed (rel.parent, candidate), so this guard only de-conflicts within one source subdirectory. insert_optimized_media.py addresses every image by bare basename.

Concrete failure: media_dir contains sub-a/logo.png and sub-b/logo.jpg. Both keep the stem logo and, with --webp (what both workflows pass), become sub-a/logo.webp and sub-b/logo.webp. insert_optimized_media's seen_names guard then skips the second with a warning and writes a single row at k/html/images/logo.webp — while build_rename_map maps both logo.png and logo.jpg to logo.webp, and rewrite_pages repoints both pages at the survivor. One image permanently gone, one page silently rendering the wrong picture, exit 0.

populate_db.py was updated in this same PR to flatten and warn on exactly this nested-zip case; optimize_media was not. Key claimed on the predicted output basename, not (parent, stem).

See the standalone comment on F04/F05/F10 for the shared root cause.

[alexmmiller]

Fixed in 46235fb. Reproduced first — with --webp, sub-a/logo.png and sub-b/logo.jpg both produced logo.webp, and insert_optimized_media's seen_names skipped the second while build_rename_map had already pointed both pages at the survivor.

De-confliction is now keyed on the predicted output basename, matching the flat namespace insert_optimized_media actually addresses — the same namespace populate_db was already modelling. Covered by test_same_basename_in_different_directories_deconflicts.

[hal-eisen-adfa]

F05 · Medium — de-confliction fires for collisions that cannot occur

The rename is unconditional, including in runs and for file types where no extension rewrite is possible.

Without --webp (the default), over a directory holding logo.png and logo.jpg: neither encoder changes the extension, so the outputs would be logo.png and logo.jpg with no collision at all — but the stem de-confliction fires anyway and writes logo-jpg.jpg. That lands in renamed, so build_rename_map records logo.jpg -> logo-jpg.jpg, rewrite_pages rewrites every stored /k/html/images/logo.jpg reference, the old row is deleted and a new one inserted. All for a non-collision. Same for two passthrough non-images sharing a stem (notes.txt + notes.md), which are only ever copy2'd.

Condition the guard on whether the extension can actually change (cfg["webp"], or the svg-rasterize path), or predict the output name per file rather than blanket-applying the rename.

[alexmmiller]

Fixed in 46235fb by the same change as F04 — which is the useful part of your observation that the two are mirror images: keying on the predicted output name fixes both at once, where any source-path key gets one of them wrong.

New possible_output_names() returns the set of basenames process_file could write, given cfg["webp"] and the svg-rasterize path. Without --webp a .png and a .jpg have disjoint possible outputs, so nothing is renamed and renamed stays empty — no stored-URL rewrites, no row churn. Covered by test_no_rename_when_the_extension_cannot_change, which also includes the notes.txt/notes.md passthrough pair.

One deliberate imprecision: the SVG branch genuinely can't be predicted (rasterization depends on the optimized size vs the threshold), so both possible names are treated as claimed. That can de-conflict a pair that wouldn't have collided — which costs a rename, where the reverse costs an image.

[hal-eisen-adfa]

F10 · Medium — src.stat() sits outside the per-file try, so one bad entry kills the whole run

optimize_directory collects sources from rglob("*") filtered only by not p.is_dir(), which keeps broken symlinks (is_dir() is False for a dangling link). The first thing process_file does is original_size = src.stat().st_size, outside the try/except that exists precisely to increment stats["errors"] and keep going.

FileNotFoundError propagates out of optimize_directory, and insert_optimized_media's caller only catches ValueError — so the run dies with an unhandled traceback instead of the intended "N file(s) failed to optimize; aborting before touching the database".

Move the stat() inside the try. See the standalone comment on F04/F05/F10.

[alexmmiller]

Fixed in 46235fb. Both parts: collect_sources() now validates the inventory at collection time with is_file() (which is False for a dangling symlink, unlike not is_dir()), reporting and counting each one; and src.stat() moved inside process_file's try, so a source that disappears mid-run is one file's error rather than an unhandled traceback past insert_optimized_media's ValueError-only catch.

Covered by test_broken_symlink_does_not_abort_the_run.

[hal-eisen-adfa]

F11 · Medium — this script's docs contradict the workflow change in the same PR

build-kotlin-docs-local.yaml:401 is now if: always(), carrying an explicit comment that it is deliberately not gated on dry_run. This script still tells the user — here in the header, and again in the runtime dry-run notice — that "build complete" "is gated on dry_run=false. Pass --live to write back and see it."

A user who exports SLACK_WEBHOOK_URL and does a dry run will fire a real Slack message they were told would not fire, on a channel where the baton messages are a shared lock convention. Update both strings, or re-gate the workflow step.

[alexmmiller]

Fixed in 46235fb — updated the header and the runtime dry-run notice to match if: always(). Both now say the notification fires regardless and reports the outcome, rather than telling the user to pass --live to see it.

Chose to correct the docs rather than re-gate the step: the baton has to be dropped on a failed build too, which is what always() is buying.

[hal-eisen-adfa]

F12 · Medium — pip in CI against the repo's uv convention, plus a dead install

Two things:

1. pip usage. The convention for this project is uv for Python dependency management — never pip, pip3, or the uv pip shim. This PR touches three such steps and leaves them all on pip: build-kotlin-docs.yaml:203/208, build-kotlin-docs-local.yaml:218/221, and python-tests.yaml:56. uv is already wired up in this repo — python-tests.yaml installs it for the docdb-studio job — and the ProcessKotlinWebsiteJSON README documents uv run --with-requirements ... as the supported invocation.

2. Dead install + wrong comment. The edited comment says "markdown-it-py: ProcessKotlinWebsiteJSON's own requirement (see its README); scour/cairosvg are in requirements.txt already" — but markdown-it-py>=2.0 is in requirements.txt on main and was installed by the preceding line all along. The extra pip install markdown-it-py is dead work and the comment's premise is wrong.

[alexmmiller]

Both fixed in 46235fb.

The dead install is gone — you're right that markdown-it-py>=2.0 is in requirements.txt and the preceding line already installed it, so the comment's premise was wrong as well as the work being wasted.

On pip: converted all three workflows. Each pipeline workflow now has a job-level $UV_RUN (uv run --with-requirements requirements.txt --, with the Drive workflow adding the google-api deps via --with), mirroring run_e2e_pipeline_test.sh's own UV_RUN array, and every python3 ... step goes through it. python-tests.yaml uses uv run directly. The separate install steps are gone.

Verified by running the local workflow end-to-end under act on the converted form: job succeeded, blacklist verification PASS, 473 k/html/* rows. Incidentally this improved workflow parity — the two now share 15 identical steps with only the apt line differing (zip, Drive-only).

[hal-eisen-adfa]

F13 · Medium — reassemble_content no longer mirrors WebServer.kt's reassembly protocol

The old loop stopped as soon as a fragment came back shorter than CHUNK_SIZE, exactly as WebServer.kt does — and the docstring still claims that mirroring. The new loop concatenates every member of fragment_chain unconditionally, and the if row is None: break is dead code, since fragment_chain only returns paths that exist.

For the "gapped" chain shape that renumber_misnumbered_fragments explicitly refuses to repair — e.g. p-1 (CHUNK_SIZE), p-2 (short), p-4 — this tool now reassembles p + p-1 + p-2 + p-4 while the server serves p + p-1 + p-2. rewrite_pages would then re-compress and store a blob the server never had, or fail to decompress with a message that points nowhere.

Keep the short-fragment terminator and use fragment_chain only for suffix discovery. See the standalone comment on F01/F02/F08/F13.

[alexmmiller]

Fixed in 46235fb — and this one was mine from the previous round: I replaced the probe-from--1 loop with fragment_chain to handle ADFA-5171 numbering and dropped the terminator in doing it, which trades a truncation bug for a fabrication bug.

reassemble_content now delegates to content_chunking.reassemble, which does suffix-agnostic discovery and then stops at the first short fragment, exactly as you describe. The dead if row is None is gone. Covered by test_reassembly_stops_at_the_short_fragment using your p-1 full / p-2 short / p-4 shape, plus test_ownership_still_includes_the_orphaned_tail to pin that deletion keeps taking p-4 — the two questions the module now separates.

[hal-eisen-adfa]

F14 · Medium — this header explains a three-way split, but only two suites actually run

The comment reads "docdb-studio and check-tools each own a pyproject.toml + uv.lock, while ProcessKotlinWebsiteJSON runs against the root requirements.txt", and the workflow's stated purpose is "Runs the repo's pytest suites on every push and PR". There are three suites' worth of tests in the repo and only two are executed.

check-tools/tests/test_db_health_checker.py — covering the health checker wired into the daily docdb-regression-test.yaml cron — continues to run nowhere. Add a uv run --frozen -- python -m pytest -q step with working-directory: check-tools, matching the docdb-studio step.

[alexmmiller]

Half fixed in 46235fb, and I want to be explicit about the half that isn't.

The header is corrected — it no longer describes a three-way split while running two.

I did not add the check-tools step, because it would make every PR red immediately: 6 of its 13 tests already fail on main, in code this PR doesn't touch. test_db_health_checker.py expects one issue from each of the empty-content, orphaned-tooltip and broken-URI checks and gets zero:

tests/test_db_health_checker.py:23: assert 0 == 1  (empty_content_issues)
tests/test_db_health_checker.py:33: assert 0 == 1  (orphaned_tooltip_issues)
tests/test_db_health_checker.py:43: assert 0 == 1  (broken_uri_issues)
6 failed, 7 passed

So your premise is right — that suite runs nowhere, and it covers the checker behind the daily cron — but wiring it up needs those fixed first, which is its own change rather than something to smuggle in here. The workflow header now records that, with the exact step to add once they pass. Happy to take it as a follow-up if you'd rather it land before this merges.

[hal-eisen-adfa]

F01 / F02 / F08 / F13 — one chunking protocol, four implementations

These four inline findings look unrelated (a wrong variable, a slow query, a missed repair, a dropped terminator) but they are the same defect: the Content chunking protocol is re-derived from scratch at every call site, and each derivation is wrong in a different way.

The protocol

WebServer.kt reassembles a page as: read path; if that blob is exactly CHUNK_SIZE, keep appending path-1, path-2, … in order; stop at the first fragment shorter than CHUNK_SIZE. Two rules follow from that, and both matter:

1. <base>-<N> is a continuation only if the base row is exactly CHUNK_SIZE long. The base merely existing proves nothing — guide.html and guide.html-1 are perfectly legal as two unrelated pages.
2. A short fragment terminates the chain. Anything after it is not part of the page the server serves, whatever is sitting in the table.

The correct implementation

migrate_content_to_dictionary_brotli.py already has it right, and this PR is where it got right:

is_chunked_base(lengths, prefix)   # gates on length == CHUNK_SIZE
load_base_rows(...)                # uses that gate

That is the reference. It encodes rule 1 explicitly and is the only copy in the repo that does.

How each site deviates

ID	Site	Deviation	Consequence
F01	sync_kdoc_json_to_db.py:390	Doesn't reach the protocol at all — passes row ids where paths are compared	Every page reported unmatched; the real signal is buried under ~38k false warnings
F02	sync_kdoc_json_to_db.py:136	Discovers fragments with an unindexed LIKE '<path>-%' scan, run unconditionally	~1e9 row comparisons; the scan can only ever return rows when rule 1 holds, which it almost never does
F08	renumber_misnumbered_fragments.py:59	Applies rule 1 without the length gate — "base exists" is treated as sufficient	Genuinely misnumbered chains are silently skipped; reports "Renumbered 0 chain(s)" on a DB that still serves the page truncated
F13	insert_optimized_media.py:194	Drops rule 2 — concatenates the whole fragment_chain	Reassembles a blob the server never serves on gapped chains, then re-compresses and stores it

F02 and F08 are the same misunderstanding pointing opposite directions: F02 does the expensive lookup when rule 1 says it cannot match, F08 skips the repair when rule 1 says it should have matched.

Recommended approach

Extract the protocol into one module — is_chunked_base(lengths, prefix) and a fragment_chain(...) that honours the short-fragment terminator — and have all four sites call it:

• F01 — pass all_paths (already computed 38 lines above), not {row[0] ...}.
• F02 — gate on is_chunked_base before any fragment lookup, or reuse the all_paths set main() already holds in memory. Either removes the LIKE scan entirely in the common case.
• F08 — replace the "base exists" test with is_chunked_base. Same for sync_kdoc_json_to_db.py:85 (is_fragment_path), which has the identical bug.
• F13 — restore the short-fragment terminator; use fragment_chain for suffix discovery only, not as the definition of the chain.

The shared module should be the one place that knows CHUNK_SIZE and the terminator rule, so the next tool that touches Content inherits the server's behaviour instead of guessing at it. A single round-trip test — chunk a blob, reassemble it, assert it matches what WebServer.kt's algorithm produces, including a gapped-chain case — would have caught F08 and F13 together.

[hal-eisen-adfa]

F04 / F05 / F10 — one root cause in optimize_media.py's name-planning pre-pass

All three findings live in the new pre-pass that walks sources and builds dst_rel_for. They share a single root cause:

The pre-pass plans entirely from source-side attributes — rel.parent, rel.stem, the extension as written on disk — and never reconciles that plan against the two things that actually determine the outcome: the predicted output name, and whether the source is a real readable file.

Each finding is one facet of that.

ID	Facet	What is assumed	What is true
F04	Namespace	Collisions are per-directory, so (rel.parent, stem) is a sufficient key	The consumer namespace is flat — insert_optimized_media addresses every image by bare basename, so sub-a/logo.png and sub-b/logo.jpg both land on logo.webp
F05	Extension	Two files sharing a stem will collide	They collide only if the encoder rewrites both extensions to the same thing. Without --webp, logo.png/logo.jpg never collide, yet logo.jpg is still renamed and every stored URL rewritten
F10	Inventory	Every entry from rglob("*") that is not p.is_dir() is a statable file	Broken symlinks pass that filter (is_dir() is False for a dangling link), and process_file stats outside the try, so one of them takes down the whole run

F04 and F05 are exact mirror images, which is the tell that the keying is wrong rather than merely too narrow: F04 misses collisions that will happen (different directories, same output name), F05 invents collisions that cannot happen (same stem, different output names). A key computed from the source path cannot get both right, because it is not the thing that collides.

Recommended approach

Compute the predicted output name once, per source, using the same logic process_file will use to decide the destination extension (cfg["webp"], the svg-rasterize path, passthrough copy), then:

1. De-conflict on that predicted name, keyed by basename alone — matching the flat namespace insert_optimized_media actually consumes. This fixes F04 and F05 in one change: names that will genuinely collide get de-conflicted regardless of directory, and names that cannot collide are left alone, so no gratuitous renamed entries and no needless stored-URL rewrites.
2. Validate the inventory at collection time — drop and report entries that are not readable regular files — and move the src.stat() inside process_file's try so a mid-run disappearance is still counted in stats["errors"] rather than escaping as an unhandled traceback. That fixes F10.

Worth noting: populate_db.py was changed in this same PR to flatten nested media and warn on basename collisions — i.e. the flat-namespace reality was already recognised on one side of the pipeline. optimize_media needs to model the same namespace, or the two halves will keep disagreeing.

[alexmmiller]

All 14 findings addressed (13 fixed, 1 deliberately not) — 46235fb

Both root-cause groupings were right, and both are fixed at that level rather than site by site. Several of these were regressions from my previous round; noted individually below.

F01/F02/F08/F13 — the chunking protocol

New content_chunking.py owns it. The part of your write-up that changed my approach was separating the two questions the rules answer, which the old call sites conflated:

• owned_fragment_paths() — ownership. What a delete or replace must take, including an orphaned tail past a gap, or it leaves rows behind.
• served_fragment_paths() / reassemble() — reassembly. What the server actually concatenates, stopping at the first short fragment.

populate_db, insert_optimized_media, migrate_content_to_dictionary_brotli, renumber_misnumbered_fragments and sync_kdoc_json_to_db all call it now. Both duplicate CHUNK_SIZE definitions and eight ad-hoc fragment helpers are gone.

One consequence worth flagging, because it bit me: once chain discovery depends on the base row's length, any site that deletes the base before resolving its chain silently stops finding fragments. Three did. Existing tests caught all three — but only after I updated fixtures that chained fragments off 1-byte base rows, a shape that cannot occur and that passed either way.

F04/F05/F10 — the name-planning pre-pass

Keying on the predicted output basename fixes F04 and F05 in one change, which is exactly the payoff of your observation that they're mirror images. New possible_output_names() derives what process_file will write from cfg["webp"] and the svg-rasterize path; collect_sources() validates the inventory and stat() moved inside the try.

One deliberate imprecision: the SVG branch genuinely isn't predictable (rasterization depends on the optimized size vs the threshold), so both possible names are claimed. That can de-conflict a pair that wouldn't have collided — a rename, where the reverse costs an image.

The rest

F03 — you were right to read the template instead of my comment. nav.peb branches on {% if node.id %} and emits a coloured <a href>; the previous fix removed the 404 from the pager and left it in the sidebar, which is the half a reader clicks. Clearing the id fixes both.

F06 was mine (the strict= lock), F07 was mine (the trap), F13 was mine (dropping the terminator while fixing the -1 probe — trading a truncation bug for a fabrication bug). F12: all three workflows are off pip onto a job-level $UV_RUN mirroring run_e2e_pipeline_test.sh, and the dead markdown-it-py install is gone.

F14 — not done, deliberately

Your premise is right: that suite runs nowhere and it covers the checker behind the daily cron. But 6 of its 13 tests already fail on main, in code this PR doesn't touch:

tests/test_db_health_checker.py:23: assert 0 == 1  (empty_content_issues)
tests/test_db_health_checker.py:33: assert 0 == 1  (orphaned_tooltip_issues)
tests/test_db_health_checker.py:43: assert 0 == 1  (broken_uri_issues)
6 failed, 7 passed

Adding the step would make every PR red for an unrelated reason. The header now records that, with the exact step to add once they pass. Happy to take it as a follow-up if you'd rather it land first.

Verification

11 new regression tests over the inputs you identified; the 5 behavioural ones fail against the pre-fix code:

FAILED test_reassembly_stops_at_the_short_fragment                 (F13)
FAILED test_misnumbered_chain_behind_a_lookalike_base_is_found     (F08)
FAILED test_same_basename_in_different_directories_deconflicts     (F04)
FAILED test_no_rename_when_the_extension_cannot_change             (F05)
FAILED test_broken_symlink_does_not_abort_the_run                  (F10)

Plus test_row_ids_would_have_matched_nothing (F01) and test_ownership_still_includes_the_orphaned_tail, which pins the ownership/serving distinction so a future "simplification" can't collapse them again.

181 + 189 tests pass. The local workflow runs green end-to-end under act on the converted uv form — blacklist verification PASS, 473 k/html/* rows, 160 .webp — and the F03 fix fired on the real corpus (Rendering 1 nav entry/entries with no page as non-links: k/html/api-references), after which zero nav ids in the stored _nav.html lack a Content row.

Workflow parity improved as a side effect of the uv conversion: 15 identical shared steps now, with only the apt line differing (zip, Drive-only).

F15 (is_contiguous_from_one) removed.

[alexmmiller]

Self-review round: 5 findings, all fixed — a59d777

No new reviewer feedback here. After the last round I ran /code-review high over the full main...HEAD diff as a check on my own work, and it turned up five issues — two of them defects in the fixes I'd just made. Recording them here rather than quietly patching, since one is a partial fix to something @hal-eisen-adfa raised and the other is a case of me verifying one half of a two-path change.

1. content_chunking.served_fragment_paths implemented half its own rule

The module docstring promises reassembly stops at the first fragment shorter than CHUNK_SIZE "or the first one missing". Only the short-fragment half was coded, so a chain with a hole jumped it:

rows:    p (1 MiB), p-1 (1 MiB), p-2 (1 MiB), p-3 MISSING, p-4 (short)
before:  ['p-1','p-2','p-4']  ->  reassembles 3,145,744 bytes
after:   ['p-1','p-2']        ->  reassembles 3,145,728 bytes
server:  ['p-1','p-2']            probes p-3, misses, stops

This is the same fabrication F13 was about, reached from the gap side instead of the short-fragment side — so my fix for F13 closed one door and left the other open. Gapped chains aren't hypothetical: renumber_misnumbered_fragments has a gapped category it reports and deliberately leaves alone, so a database can sit in this state indefinitely. rewrite_pages would then hand the over-long blob to compressor.decompress, which raises on the trailing bytes and aborts the whole media update — on rows the server reads fine.

Contiguity is now enforced from wherever the chain starts, not from 1, so an ADFA-5171 chain numbered from -2 is still readable whole by the migration and repair tooling. owned_fragment_paths is deliberately unchanged — a delete still has to take the orphaned p-4 rather than leaving it behind. That ownership/serving split (which came out of your F01/F02/F08/F13 write-up) is what made this a two-line fix instead of a redesign.

2. The dangling-nav-link fix was applied to one of its two paths

For F03 I cleared the synthesized ids in populate_db.py, verified the database output had zero nav ids without a Content row, and stopped. But build_nav.py renders the same nodes through the same if node["id"] branch, and its standalone output still emitted live links to pages nothing generates. The committed templates/nav.html shipped two of them:

- <a class="nav-link" style="color: #999999;" href="/home.html" data-nav-id="home">Home</a>
- <a class="nav-link" style="color: #999999;" href="/api-references.html" data-nav-id="api-references">Overview</a>

The pass is now a module-level build_nav.drop_unreachable_ids(nodes, real_page_ids) that populate_db imports rather than reimplementing — the static site and the database have to agree on what's reachable, or one ships links the other can't satisfy.

On the artifact: I corrected those two entries by hand rather than regenerating nav.html. kotlin-web-site has moved since it was built (304 topics vs the 303 it came from), so a full regen would fold unrelated upstream drift into a committed file and make this diff unreviewable. The two lines now match what the fixed generator emits byte for byte; a regen against the original ref would be a no-op.

3–5. Smaller

• optimize_media de-confliction was case-sensitive — logo.PNG and logo.png never de-conflicted despite being the same file on a case-insensitive filesystem. F04's namespace mismatch reached through case. Keys are casefolded now.
• content_chunking.split_into_chunks had no callers. The three sites that split blobs still do it inline. Removed rather than wired in: the write half already agrees across all three (checked at the exact-multiple boundaries), and an untested fourth copy is precisely the divergence this module exists to prevent.
• if: always() on "build complete" could drop a baton that was never grabbed. A run failing before "build started" — a bad db_path, a failed Drive download — posted a release for a lock it never acquired, which is F14's asymmetry pointing the other way. Now gated on steps.slack_started.outcome == 'success'.

A correction to my own review

I reported the case-sensitivity issue as reproduced. It wasn't — macOS can't hold logo.png and LOGO.PNG in one source directory, so my repro created a single file and proved nothing. The fix is still right (verified at the level the comparison actually happens), but it's latent and needs the source and output filesystems to differ in case sensitivity. The pipeline runs on Linux end to end, so it isn't live. Flagging because I'd rather the severity be right than look thorough.

Verification

6 new tests. The 2 pinning the gap terminator fail against the pre-fix code:

FAILED test_reassembly_stops_at_a_gap_in_the_numbering
FAILED test_gap_inside_a_misnumbered_chain_still_terminates

The build_nav one can't fail pre-fix — the function didn't exist — so the evidence there is the artifact diff above. The rest are must-not-regress guards, including test_adfa_5171_chain_from_two_is_still_read_whole, which pins the one place this deliberately diverges from the server.

187 + 189 tests pass. Local workflow green end to end under act: PASS: all 38 blacklisted topic page(s) confirmed absent, 473 k/html/* rows, 160 .webp, the F03 message still firing on the real corpus, and the newly-gated Slack step correctly running when the acquire succeeded. Parity between the two workflows holds at 15 identical shared steps, the apt line the only difference.

State

MERGEABLE at a59d777; BLOCKED is REVIEW_REQUIRED. 34 threads are answered but open — resolving them is the reviewer's call. The one item still deliberately not done is F14's check-tools CI step, for the reason in that thread: 6 of its 13 tests already fail on main in code this PR doesn't touch.

[hal-eisen-adfa]

Automated review pass (/code-review xhigh) over the full PR diff vs c627c1b, after the two rounds of review fixes landed.

11 inline findings below, F25-F35: 1 critical, 3 high, 7 medium. The numbering continues from the previous round rather than restarting, so no id in this PR ever means two different things.

Three of them - F29, F30, F31 - are items already raised in the 2026-09-01 pass, in the "lower-severity observations" section that carried no inline comment. They were not addressed in either fix commit and no reply explains why, so they are re-raised here inline where they can be tracked.

What I verified, and how: every line number and code shape below was read on a59d777. The suites are genuinely green - ProcessKotlinWebsiteJSON + sync_kotlin_stdlib_docs 187 passed, docdb-studio 189 passed - matching the counts the commit messages claim. I also checked the F14 deferral from last round and it is accurate: check-tools is 6 failed / 7 passed on this branch, all in test_db_health_checker.py, in code this PR does not touch.

What I did not verify: F25 is reasoned from the workflow source and act's documented input handling, not reproduced by running act. It is the one finding I would want confirmed by a run before acting on it - see the note in that comment.

Two lower-severity observations not worth an inline comment:

• build_nav.render_node says it is "kept byte-identical to templates/nav.peb's renderNavNode macro", but it joins on "" while nav.peb puts each element on its own line, so the two outputs cannot actually be diffed to check the claim. Its indent parameter is accepted at line 182, never read, and not passed down to the recursive call at line 210.
• find_missing_assets.FENCE_RE strips only triple-backtick fences, so an <include from="..."> example inside a ~~~ fence or an indented code block is reported as a broken reference. The module docstring specifically advertises that fenced examples are ignored; links and images get that for free through markdown-it, only this standalone regex scan does not.

[hal-eisen-adfa]

F25 · Critical - act passes these inputs as strings, so every if: gate in the local workflow inverts.

On real GitHub, a workflow_dispatch input declared type: boolean arrives as a boolean. Under act it arrives as the string you typed. In a GitHub expression a non-empty string casts to true, so !"false" is false.

build-kotlin-docs-local.yaml gates on the negation in all thirteen places:

254,261,265,271,275,288,348:  if: ${{ !inputs.skip_website_docs }}
302,309,321:                  if: ${{ !inputs.skip_stdlib_docs }}
401:                          if: ${{ !inputs.dry_run }}

So a default run through this script - which sends --input skip_website_docs=false at line 231 - skips Steps 1/5 through 3/5 and 4/5's page rewrite: find_missing_assets, populate_db, insert_optimized_media. It does the stdlib half only. And --live, which sends dry_run=false, makes !inputs.dry_run false, so "Write updated database back to db_path" at :401 never runs and the flag documented as "write the rebuilt database back" writes nothing.

Skipped steps report green, so this does not fail - it silently does less. That matters for the "local workflow runs green end-to-end under act" claim in both fix commits: a green act run is consistent with this bug rather than evidence against it.

The workflow header at build-kotlin-docs-local.yaml:21-27 already reasons about these inputs as strings, but only for the unset case ("an input you don't pass arrives empty"). Passing them explicitly, which this script always does and which that comment recommends, is what triggers the inversion.

I have not run act to confirm this - it is read off the workflow source and act's documented behaviour, so it is the one finding here I would reproduce before acting on it. If it does reproduce, if: ${{ inputs.x != 'true' }} (or fromJSON) is the shape that works under both.

[alexmmiller]

Reproduced as you asked, and it doesn't hold up — the gates evaluate correctly under act in both directions. Two runs, run-build-kotlin-docs-with-act.sh driving the local workflow:

--live --skip-stdlib-docs (i.e. dry_run=false skip_website_docs=false skip_stdlib_docs=true):

✅ Success - Main Step 1/5: find missing assets
✅ Success - Main Step 2/5: convert and populate
✅ Success - Main Step 3/5: optimize and insert media
✅ Success - Main Write updated database back to db_path
   | Wrote updated documentation.db back to /mnt/act-inputs/documentation.db

and the input file's md5 changed, 66b76b42… → 3dfc39b6…, so !inputs.dry_run gated on with dry_run=false passed as a string.

--skip-website-docs: zero occurrences of steps 1–3 in the log. So the same expression gates off when the input really is true.

The reason is that ! in a GitHub expression isn't string-truthiness — it's the documented cast to number, and 'false' casts to 0, so !'false' is true. 'true' casts to NaN, !NaN is false. The string case lands the same way the boolean case does, which is why act and GitHub agree here.

Worth flagging the other direction too: the suggested if: ${{ inputs.x != 'true' }} would break on real GitHub, where the input is a boolean. true != 'true' casts both to numbers — 1 != NaN — which is true, so a step gated that way would run when the flag is set. fromJSON works under both; !inputs.x already does.

Nothing changed for this one. I've added the two log excerpts to the run notes so the next person doesn't have to re-derive it.

[hal-eisen-adfa]

F26 · High - possible_output_names mispredicts the one raster case that keeps its extension, so F04's failure mode is still reachable.

This line claims {stem}.webp for every raster source when --webp is set. But optimize_raster does not convert an animated GIF - resize_animated_gif writes it back as a GIF, and the docstring at line 264 says so explicitly ("--webp is intentionally not honored here").

Concrete failure, with --webp (what both workflows pass): media/A/logo.gif (animated) and media/B/logo.gif (static).

1. A/logo.gif claims logo.webp, but actually writes A/logo.gif.
2. B/logo.gif sees logo.webp already claimed, is de-conflicted to the stem logo-gif, and writes B/logo-gif.webp.
3. renamed gets A/logo.gif -> ... unchanged, and B/logo.gif -> B/logo-gif.webp.
4. build_rename_map collapses to basenames: logo.gif -> logo-gif.webp.
5. rewrite_pages repoints every stored /k/html/images/logo.gif reference at logo-gif.webp - including the pages that wanted the animated one.
6. The row at k/html/images/logo.gif is now referenced by nothing, and delete_unreferenced_media removes it.

Animated image permanently gone, its pages rendering the static one, exit 0. That is the same shape as F04, reached through the single output name this pre-pass cannot predict from the suffix alone.

The prediction needs the animated/static distinction, which is knowable up front (Image.open(src).n_frames, or getattr(img, "is_animated", False)) - the same test optimize_raster makes later. Alternatively, return {f"{stem}.webp", f"{stem}{suffix}"} for .gif under --webp, which is the conservative both-possibilities treatment this function already applies to SVG two lines above, and for the same reason.

[alexmmiller]

Fixed, both halves — you were right that the first one alone isn't enough.

possible_output_names now resolves the animated case rather than guessing: _is_animated_gif(src) opens the file and returns True/False/None, and an animated GIF claims {stem}.gif only. Unreadable (None) claims both names, so a file we can't classify can't have its name taken by something else.

That alone still lost the animation, though. Two sources sharing a basename (a/logo.gif, b/logo.gif) hit the de-confliction loop, which renamed the second and put an entry in rename_map repointing the shared stored name. Since the output namespace is flat, only one of them can be addressed at all — so there's now a duplicate-basename pre-pass that keeps the first and skips the rest with a warning, matching what insert_optimized_media's insert loop already does downstream. No rename, no repoint.

On the real corpus, --webp on: the animated GIF survives with its 3 frames, renamed is empty for it, and F04's cross-directory de-confliction still fires for the cases that genuinely collide.

Three tests: test_animated_gif_survives_a_webp_run, test_still_gif_is_still_converted_to_webp (the exemption is narrow — a single-frame GIF still converts), and test_duplicate_basenames_keep_the_first_without_repointing_it. The first and third fail pre-fix.

[hal-eisen-adfa]

F27 · High - the insert loop reads the work dir, not this run's output, so a stale file is inserted as if this run had produced it.

optimize_directory has just returned the set of files it wrote, but the loop below walks sorted(work_dir.rglob("*")) instead. Anything already sitting in that directory is inserted.

work_dir is the documented third positional, so keeping a fixed one between runs to avoid re-optimizing is a supported usage. Run 1 optimizes a media dir containing old-logo.png; before run 2 that source is deleted. optimize_directory never touches it, but rglob still finds work_dir/old-logo.png and inserts a row at k/html/images/old-logo.png.

Whether that row then survives has nothing to do with what this run was asked to insert - it comes down to whether some page still happens to reference the old name, which delete_unreferenced_media decides several steps later. A resurrected image and a deleted one are both reachable from the same stale file.

Driving the loop from optimize_directory's own result closes it. Requiring an empty work_dir (or clearing it first) also works, but changes the documented behaviour of the positional.

[alexmmiller]

Fixed. optimize_directory now returns an OptimizeResult(renamed, written) NamedTuple, and the insert loop iterates sorted(result.written) instead of rglob-ing the work directory.

That matters exactly as you describe: the work dir is a documented positional, so reusing one between runs to skip re-optimizing is supported — and an rglob then inserts files an earlier run left behind, whose sources are gone. test_written_lists_only_this_run_s_outputs puts a stale file in the output directory and asserts it is absent from written (and still on disk — this doesn't delete it, it just stops treating it as this run's output). Fails pre-fix.

[hal-eisen-adfa]

F28 · High (latent) - this branch renders an image reference in a form insert_optimized_media's rewriting cannot see.

This renders a standalone b.type == "image" block from a bare b.src field. md_to_json.py never emits one - its own docstring at line 58 states there is no standalone image block type - so today the branch is dead.

The problem is what happens the moment it isn't. Both of insert_optimized_media's reference scanners are anchored on the escaped quote that only ever follows an HTML src="..." attribute in the JSON:

• rewrite_pages (line 209): f'{IMAGES_URL_PREFIX}{old_name}\\"'
• IMAGE_REF_RE (line 268): re.escape(IMAGES_URL_PREFIX) + r'([^\\"]+)\\"'

A block serialized as {"type":"image","src":"/k/html/images/foo.png"} ends in a plain ", and matches neither. So a page using this form would (a) keep pointing at foo.png after a --webp rename, and (b) be invisible to collect_referenced_media, which means delete_unreferenced_media deletes foo.png as unused. Silent data loss, armed by a template branch and disarmed only by the producer's current behaviour.

Either drop the dead branch, or widen both patterns to cover the bare-field form. Dropping it is the smaller change and keeps one way of expressing an image; the anchoring comment above IMAGE_REF_RE should then say that it depends on that.

[alexmmiller]

Dropped the branch, and said why in both places.

md_to_json.py is explicit that images are inline-only ("There is no standalone "image" block type — CommonMark only ever produces "image" as an inline token"), and its block-level fallback says the same, so nothing this pipeline produces ever reached this branch. page.peb now carries a comment where it was, warning that re-adding one means widening IMAGE_REF_RE and rewrite_pages first — and the comment above IMAGE_REF_RE now states that dependency from its own side.

Verified against a real Pebble 3.2.2 engine rather than by inspection: the edited template compiles, an {"type": "image", "src": ...} block falls through to the {% else %} html fallback, <table>/<hr>/paragraphs still render, and the comment emits nothing.

test_markdown_images_stay_inline_never_a_block pins the invariant the three now share.

[hal-eisen-adfa]

F29 · Medium - topic_index_db.pop runs mid-loop, so the broken-link styling this comment promises applies only to pages converted after the failure.

Raised in the 2026-09-01 pass as a lower-severity observation (populate_db.py:694 as it was numbered then); not addressed in either fix commit, and no reply explains why, so re-raising it inline.

Converter was handed topic_index_db by reference at line 653 and consults it during convert_file. Popping the stem here only affects conversions that have not happened yet.

With --allow-conversion-failures: topics/a-page.md sorts first, converts fine, and links to z-page. topics/z-page.md then fails. a-page was already rendered with an ordinary <a href="/k/html/z-page.html">, and no Content row is written for z-page, so that link 404s in the app - which is exactly what the comment above says this pop prevents. Alphabetical position decides whether a given link renders as a styled broken link or as a live 404.

The pattern that works is the one drop_unreachable_ids uses for nav a few lines down: resolve against the final pages set after the loop, rather than mutating shared state during it. That needs a second pass over the converted pages' links, which is more than a one-line fix - which may well be why it was skipped. If that is the call, it is worth saying so in the comment, because as written the comment claims a guarantee the code only delivers for part of the corpus.

[alexmmiller]

Fixed, and the comment corrected.

One correction first: this was answered — the 2026-09-01 summary comment lists it under "Not fixed, deliberately, so they don't look handled", as populate_db.py:694, with the reason ("fixing it properly means knowing which pages fail before converting any, which needs a second pass over the corpus; I didn't want to make that trade quietly"). Easy to miss in a long comment, and it's on me that it wasn't also in the thread.

The second pass turned out to be cheap once scoped correctly, so it's in rather than deferred. After the refusal check — i.e. only on the --allow-conversion-failures path that can still write — pages_linking_to(pages, failed_stems) finds the pages carrying an already-resolved href="/k/html/<stem>.html", and just those are re-converted against the now-correct index. A page converted after the drop never contains that form (resolve_href returns None and leaves the raw <stem>.md, styled broken), so the scan selects exactly the stale ones and nothing else — usually zero, and never the whole corpus.

The comment at the pop now says what the drop actually governs (nav, and conversions that haven't happened yet) and points at the pass that repairs the rest.

test_pages_linking_to_finds_the_pre_failure_pages covers the three shapes: the pre-failure page, a nested block, and the post-failure page that must not be re-converted.

[hal-eisen-adfa]

F31 · Medium - loop defaults to 0, which turns a play-once GIF into one that loops forever.

Raised in the 2026-09-01 pass as a lower-severity observation; not addressed, and no reply explains why, so re-raising it inline.

A GIF authored with no NETSCAPE application-extension block plays once, and Pillow represents that as the "loop" key being absent from img.info - not as loop=0. 0 is the value that means "loop forever". So img.info.get("loop", 0) reads "play once" and writes "loop forever", and save(..., loop=0) emits an explicit NETSCAPE block that was not in the source.

That contradicts this function's own docstring two lines up, which says it preserves "frame count, each frame's own duration, and the loop count".

The fix is to omit the kwarg entirely when the key is absent, rather than to pick a different default:

save_kwargs = {"duration": durations, "disposal": 2, "optimize": True}
if "loop" in img.info:
    save_kwargs["loop"] = img.info["loop"]

[alexmmiller]

Fixed, with the test.

Same correction as on F29: this was answered — the 2026-09-01 summary lists it under "Not fixed, deliberately", as optimize_media.py:267, with the reason being that it needs the source's own loop count read back through Pillow and I'd rather do it with a test than by inspection. That's what's now happened, so the deferral has expired rather than been ignored.

loop is passed to save only when "loop" in img.info. Pillow signals "plays once" by omitting the key, and 0 means "loop forever", so info.get("loop", 0) read one and wrote the other — adding a NETSCAPE application-extension block the source never had.

test_resize_preserves_a_play_once_gif (fails pre-fix: 'loop': 0 appears in the output's info) and test_resize_preserves_an_explicit_loop_count for the other direction. One fixture note that cost me a few minutes: GIF frames that are byte-identical get collapsed on save, so the fixture draws a moving rectangle — otherwise you get a single-frame "animation" that proves nothing.

[hal-eisen-adfa]

F32 · Medium - backup taken before the scan, so every no-op run leaves a full-size copy behind.

This is the pattern the same PR removed from the other two tools. sync_kdoc_json_to_db.py now backs up only after the MAX_DELETE_FRACTION precheck, with a comment saying that taking it earlier "meant a run that correctly aborted on a layout mismatch still left a full-size copy of the database behind, for nothing". populate_db.py was changed the same way.

Here the equivalent "nothing to do" outcome is the common one, not the exceptional one: this script is documented as idempotent ("a second run finds nothing left to fix"), so re-running it against the production database is the expected case, and each such run writes another VACUUM INTO copy of a ~250MB file before discovering there are zero chains to repair.

Moving the backup below find_chains, gated on misnumbered being non-empty, matches what the other two tools now do.

[alexmmiller]

Fixed — third tool, same pattern as the other two.

main now scans first (find_fragment_paths → find_chains) and backs up only if misnumbered. The VACUUM is gated too, on stats["chains_renumbered"]: a run that moved no rows has no freed space to reclaim, and vacuuming rewrites the whole ~250MB file for nothing.

repair(conn, misnumbered=None, gapped=None) takes the scan results so main can decide about the backup before anything is written; both stay optional, so repair(conn) still does the whole job for callers that just want it done.

test_a_clean_database_is_not_backed_up and test_a_database_needing_repair_is_backed_up.

[hal-eisen-adfa]

F33 · Medium - sys.path.insert(0, ...) puts ProcessKotlinWebsiteJSON ahead of the standard library.

Position 0 means that directory is searched before everything, stdlib included. Any module added to ProcessKotlinWebsiteJSON/ whose name collides with a stdlib one - types.py, json.py, select.py, io.py are all plausible names for something in there - would be imported in its place, for this script and for everything it imports transitively. The failure surfaces far from the file that caused it, and the directory is one nobody would think to suspect.

This script needs that directory reachable, not preferred: sys.path.append does the same job with none of that. Worth doing now while the import list is short, since the cost of the collision is paid by whoever adds the colliding file rather than by whoever chose the insert.

[alexmmiller]

Fixed — sys.path.append(...), so the directory is searched last rather than ahead of the standard library.

[hal-eisen-adfa]

F34 · Medium - collect_referenced_media (line 271) repeats every reassembly and decompression rewrite_pages just performed.

Commenting here on the shared helper; the duplicated pass is at line 271.

rewrite_pages selects the k/html page rows, calls reassemble_content on each, and decompresses each one. collect_referenced_media then runs the same query, the same reassemble_content, and the same decompress over the same rows - its docstring says as much ("the same row selection/reassembly rewrite_pages uses") - to build a reference set from text rewrite_pages was already holding.

The cost is not incidental. DictionaryCompressor has no in-process path: decompress shells out to the brotli CLI once per row. Over ~1,000 k/html/*.html pages that is ~1,000 extra subprocess spawns plus a second full reassembly pass, in the step that already dominates the run.

rewrite_pages already has each page's decompressed text in hand and already applies IMAGE_REF_RE-shaped substitutions to it. Having it collect the IMAGE_REF_RE matches it sees and return them alongside changed removes the second pass entirely. The ordering constraint in delete_unreferenced_media's docstring - that it must run after rename-rewriting so it sees post-rename names - is satisfied by construction if the set is built from the rewritten text.

[alexmmiller]

Fixed — one pass, as you describe.

rewrite_pages now returns RewriteResult(changed, referenced), collecting IMAGE_REF_RE matches from each row's final text (post-substitution where it substituted), and main hands that to delete_unreferenced_media as referenced=. Your point about the ordering constraint is what makes this safe: building the set from the rewritten text satisfies it by construction.

One thing that had to change to make it work in both directions: the if not rename_map: return 0 shortcut is gone, because a run with nothing to rename still needs the reference scan — otherwise the second pass just moves rather than disappears. That meant guarding the pattern, since "|".join(()) is "" and re.compile("") matches at every position; old_ref_pattern is now None when there's nothing to substitute. test_an_empty_rename_map_rewrites_nothing pins that.

collect_referenced_media stays as the reference implementation and as the fallback when delete_unreferenced_media is called without a set — that's how its existing tests call it.

test_pages_are_decompressed_once_per_run counts compressor.decompress calls across both functions with a real rename in play: 4 pages, 4 decompressions. The old shape was 8.

One correction on the size of it, since I measured rather than estimated: the corpus has 268 k/html/*.html page rows, not ~1,000, and a full reference pass over them takes 2.3s. So the saving is 2.3s out of a ~63s step — real, and worth having for a change that also removes the duplicated row-selection logic, but an order of magnitude smaller than the thread suggests. The end-to-end act run is otherwise identical: 298 images inserted, 224 renamed, 47 pages rewritten, 98 unreferenced deleted, 160 .webp rows — same numbers as before the change.

[hal-eisen-adfa]

F35 · Medium - $2 is read without an arity check, so a trailing flag dies under set -u instead of printing usage.

usage() is defined a few lines above for exactly this class of mistake, but a user who types

build-stdlib-json-docs.sh kotlin-repo --kotlin-libs-version

gets $0: line 60: $2: unbound variable and exit 1, with no hint that the flag needs a value or that a usage message exists.

Same shape for every --flag VALUE option in run-build-kotlin-docs-with-act.sh:115-124.

[ $# -ge 2 ] || { log "error: $1 needs a value"; usage; exit 1; }

before each $2 read covers it.

[alexmmiller]

Fixed in both scripts. A need_value helper checks [ $# -ge 2 ] before each $2 read — the seven flags in run-build-kotlin-docs-with-act.sh and the two in build-stdlib-json-docs.sh:

$ ./run-build-kotlin-docs-with-act.sh --db-path
error: --db-path needs a value

$ build-stdlib-json-docs.sh kotlin-repo --kotlin-libs-version
error: --kotlin-libs-version needs a value
Usage: ... [--kotlin-libs-version V] [--kotlin-libs-repo URL] <path-to-kotlin-repo-root> [output-dir]

test_a_flag_without_a_value_is_reported runs both through bash and asserts exit 1, the message, and no unbound variable.

[alexmmiller]

Third review round: 10 of 11 fixed, 1 disproved

Pushed as a single commit, 0204dbc4.

F25 (critical) did not reproduce

You flagged this as the one to verify first, so I did. Two act runs through run-build-kotlin-docs-with-act.sh:

--live --skip-stdlib-docs — dry_run=false skip_website_docs=false skip_stdlib_docs=true:

✅ Step 1/5: find_missing_assets.py           [1.03s]
✅ Step 2/5: populate_db.py                   [12.6s]
✅ Step 3/5: insert_optimized_media.py        [1m2.6s]
✅ Write updated database back to db_path
   | Wrote updated documentation.db back to /mnt/act-inputs/documentation.db

The input file's md5 changed, 66b76b42… → f90054e8…. A second run with --skip-website-docs shows zero occurrences of steps 1–3. So !inputs.x gates correctly in both directions with the value arriving as a string.

! isn't string-truthiness — it's the documented cast to number. 'false' → 0, so !'false' is true; 'true' → NaN, so !'true' is false. Both land where the boolean case does. The suggested inputs.x != 'true' would break on real GitHub, where the input is a boolean: 1 != NaN is true, so the step would run when the flag is set.

The other ten

F26 high	Animated GIFs can't be re-encoded to WEBP, so possible_output_names now opens the file (_is_animated_gif) instead of assuming; unreadable claims both names rather than guessing. That alone wasn't enough — two sources sharing a basename still produced a rename_map entry repointing the shared stored name, so duplicates now keep-first-skip-rest, matching the insert loop downstream.
F27 high	OptimizeResult(renamed, written); the insert loop follows written, so a reused work dir can't resurrect a file whose source is gone.
F28 high	Dead b.type == "image" branch removed. Both page.peb and the comment above IMAGE_REF_RE now record that the anchoring depends on images only ever being src="...".
F29 med	Fixed rather than documented: after the refusal check, pages_linking_to finds pages carrying an already-resolved href="/k/html/<stem>.html" and re-converts just those. Pages converted after the drop never contain that form, so the scan selects exactly the stale ones.
F30 med	list_stored_media asks is_continuation_path; its docstring was describing the old presence-only rule and now matches the code.
F31 med	loop passed through only when the source carried one.
F32 med	Scan first, back up only if misnumbered, VACUUM only if rows moved.
F33 med	sys.path.append.
F34 med	rewrite_pages returns RewriteResult(changed, referenced), collected from post-substitution text; delete_unreferenced_media takes it.
F35 med	need_value before each $2 read — 7 flags in one script, 2 in the other.

Also from the older lower-severity list: render_node's unused indent parameter is gone, and find_missing_assets now uses md_to_json.fenced_spans rather than a second, backtick-only fence pattern — a <include> shown inside a ~~~ sample was being reported as a broken reference.

Two corrections to my own earlier claims

"Byte-identical to nav.peb." I wrote that in the 2026-09-01 summary while explaining the escaping fix. It isn't true and I should have checked rather than asserted it: nav.peb's tags sit on their own source lines, so its output carries newlines where render_node joins on "". Rendering both over the same tree with a real Pebble 3.2.2 engine shows that whitespace and nothing else — the escaping is identical, " and & on both sides, which is the part the fix was about. The comment now says so precisely.

F34's cost. I measured instead of repeating the estimate: the corpus has 268 k/html/*.html page rows, not ~1,000, and a full reference pass over them takes 2.3s. The saving is 2.3s of a ~63s step — real, and it removes the duplicated row-selection logic, but an order of magnitude smaller than the thread implies. The code comment carries the measured figure.

One thing worth flagging back

F29 and F31 both say "no reply explains why". They were answered — the 2026-09-01 summary comment has a "Not fixed, deliberately, so they don't look handled" section listing populate_db.py:694 (the mid-loop pop, deferred because a proper fix needs a second pass) and optimize_media.py:267 (the GIF loop default, deferred because it wanted a test rather than inspection). Buried in a long comment, and not repeated in the threads, which is on me — but the reasoning existed. Both are now done, so the deferrals have expired rather than been ignored.

Verification

20 new tests, in tests/test_review_findings.py. The ones that can fail pre-fix do:

FAILED test_animated_gif_survives_a_webp_run
FAILED test_duplicate_basenames_keep_the_first_without_repointing_it
FAILED test_written_lists_only_this_run_s_outputs
FAILED test_resize_preserves_a_play_once_gif

test_markdown_images_stay_inline_never_a_block passes both ways by design — it pins the invariant page.peb, IMAGE_REF_RE and rewrite_pages now all rest on.

174 + 33 + 189 pass. The 5 failures and 1 collection error under scripts/ are pre-existing — identical list on the base commit, and the collection error is a missing faker_file dependency.

Local workflow green end to end under act, with output identical to the previous round: 266/266 pages converted, 298 images inserted, 224 renamed, 47 page/nav rows rewritten, 98 unreferenced deleted, 160 .webp rows, PASS: all 38 blacklisted topic page(s) confirmed absent. The F26 fix is latent on this corpus — it has no animated GIFs and no duplicate basenames — which is why it has its own tests rather than a diff in the run output.

Workflow parity re-checked and unchanged: the only differences between build-kotlin-docs.yaml and build-kotlin-docs-local.yaml remain the Drive I/O boundary (auth, download, upload → cp / read / write) and the workflow name. build-stdlib-json-docs.sh changed this round, so it was exercised by the run above.

[alexmmiller]

Six self-review rounds since the last update — 44 findings, and a caveat about them

0204dbc4..99c9b2f8, six commits, +1461/−135. Every one of your review threads has a reply, and nothing of yours is outstanding — the architectural writeups from 2026-09-02 landed as content_chunking.py (six call sites now import it), and the F04/F05/F10 recommendation landed as possible_output_names + a flat, basename-keyed claimed map.

What follows is what my own /code-review passes found afterwards. I'm reporting it in full because most of it was defects in my own fixes, not in the original PR — and two were worse than anything in your reviews.

The two that mattered

An animated WEBP was flattened to a single frame. Fixing your F26 (animated GIFs mispredicted as .webp) introduced an ANIMATABLE_EXTENSIONS set to decide which sources to probe. .webp was excluded on the reasoning that an animated webp is copied through under the same .webp name a converted one gets — so the probe couldn't change the name. True. But a later commit made optimize_raster consume that same value to decide whether to re-encode, and the set had never been chosen for that question. Verified both directions: 4 frames in, 1 frame out at 6b5345c2; 4 frames at its parent. Exit 0, no warning, and the written-vs-claimed guard couldn't see it because the name was exactly as predicted.

The set is gone. One question — "is this file animated" — one answer, taken from the file, for every raster; the naming rule follows from it (under --webp a raster keeps its extension exactly when it is animated). That also caught a case neither list had: an MPO stored as .jpg, which Pillow reports as animated.

An SVG with an uppercase extension aborted the whole run. possible_output_names returned the lowercase SVG_EXTENSION literal while optimize_svg writes the source's own spelling, so Diagram.SVG was claimed as Diagram.svg. Harmless for as long as it only fed a casefolded map — fatal once I added a check comparing what was written against what was claimed. One ordinary file, run dead after all 299 images were optimized.

The rest, by round

	Found	Notable
fa1848f3	8	Duplicate-basename skip keyed casefolded while populate_db indexes exactly → a case-differing pair lost one image and left a live dangling reference. The F35 test broke this PR's own CI (it shells out to a script whose command -v act check ran before argument parsing).
63a2dea0	8	The uppercase-SVG abort, plus the guard being case-sensitive where the pass it validates is casefolded, and raising RuntimeError past insert_optimized_media's except ValueError.
966fb905	7	The guard's exemption keyed on "the probe failed" rather than "the planner claimed nothing" — which come apart under --webp false — so it waved real drift through as a warning.
6b5345c2	7	The animated-WEBP flattening.
38caac43	7	An animated raster skips --max-width and was logged as "Optimized", counted as an optimization: a 900×900 source ships at 900×900 with the log saying otherwise.
99c9b2f8	7	A replaced comment that was appended instead of substituted, leaving the superseded rationale — the exact argument that caused the WEBP bug — sitting one line above its replacement.

Full detail is in each commit message.

What I'd take from this

Three of these were introduced by guards or refactors I added while fixing your findings, and one of those guards — the written-vs-claimed check — has now been the subject of four consecutive rounds. It is worth its keep (it is what would catch the next planner/writer divergence, and it is silent across the whole real corpus), but the pattern is clear enough to name: each round's fix added a parallel structure beside an existing one, and the two then disagreed. claimed_for vs claimed, then unpredicted vs claimed. Both are gone; the per-source plan is one record.

The other thing worth naming is that the corpus is a weak oracle for this code. webHelpImages.zip has no animated non-GIF, no uppercase extensions, no duplicate basenames, and 299 exactly-distinct filenames. Every bug above passed a byte-identical corpus run and a green act build. The tests carry that weight now — 219 in ProcessKotlinWebsiteJSON, of which the ones added in these six rounds are all constructed fixtures for shapes the corpus does not contain — but "the pipeline runs green" should be read as necessary, not sufficient.

State

MERGEABLE at 99c9b2f8; BLOCKED is REVIEW_REQUIRED. 219 + 33 + 189 tests pass; the 5 failures and 1 collection error under scripts/ are pre-existing on the base commit (a missing faker_file dependency and test_android_tooltips.py).

The local workflow is green end to end under act, with the same counts as every prior round: 266/266 pages, 298 images inserted, 224 renames, 47 page/nav rows rewritten, 98 unreferenced deleted, 160 .webp rows, PASS: all 38 blacklisted topic page(s) confirmed absent.

Two notes on that run. The built database's md5 changed for the first time in five rounds — that is upstream drift, not this branch: exactly two of 30,620 rows differ (k/html/functions.html, k/html/releases.html), zero image rows, and both source files were committed upstream in kotlin-web-site earlier that day. And act needed --action-offline-mode to run at all, because the gh token on my machine had expired and act could not fetch the action repos; unrelated to the branch, but it is why that run's invocation differs from the ones quoted earlier.

I've stopped self-reviewing here. Six rounds in, the yield has gone from silent data loss to comment accuracy, and the remaining risk is better addressed by your eyes than by another pass of mine.