Repository navigation
ADFA-4739: Kotlin docs DB pipeline + Build Kotlin Docs GitHub Action - #24
Conversation
…, generate and add templates to display it to database, optimize and insert Kotlin website media. Script to sync current kotlin-stdlib documentation against a newly-generated documentation set (for now, used to do pruning for ADFA-4737 https://appdevforall.atlassian.net/browse/ADFA-4737)
- README: templates/assets are already included in the repo, not left for the user to place separately. - sync_kdoc_json_to_db.py: default --db to documentation.db in cwd instead of a personal /home/alex path; add explicit BEGIN to match sibling scripts' transaction style. - insert_optimized_media.py: rewrite_pages now substitutes renamed image references in a single regex pass over each row's original text, instead of sequential str.replace calls on a mutating buffer, which could double-rewrite a reference if one rename's new name chained into another rename's old name. - run_e2e_pipeline_test.sh: add a verification step that reuses populate_db.py's own pruning logic to confirm blacklisted topic pages are actually absent from the database, replacing a weaker proxy check that only tested for k/html/wasm% paths. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Brings in the updated KDoc-to-JSON Dokka plugin (sourceSet whitelisting, package-index/structure verification scripts) so this branch has the current plugin used to generate the kotlin-stdlib docs consumed by sync_kdoc_json_to_db.py.
- run_e2e_pipeline_test.sh: replace the manually-supplied STDLIB_ALL_LIBS
path with STDLIB_DOCS_DIR (a kotlin repo's libraries/tools/kotlin-stdlib-docs
checkout). New Step 4/5 derives the repo root and calls
build-stdlib-json-docs.sh to freshly build/publish the kdoc-to-json plugin
and generate kotlin-stdlib/-reflect/-test JSON docs (common+jvm source
sets only, per the existing plugin config) before Step 5/5 syncs them into
the database.
- Add build-stdlib-json-docs.sh: builds a fresh copy of the plugin, swaps in
the JSON-plugin-enabled build.gradle.kts for the duration of the build
(restored on exit via trap), and prints only the resulting all-libs path
to stdout so it composes as STDLIB_ALL_LIBS="$(build-stdlib-json-docs.sh
<kotlin-repo-root>)" - every gradlew invocation inside it is redirected to
stderr so that capture isn't polluted by build console output.
- build.gradle.kts: re-add the Dokka dev-snapshot Maven repo (same
dokka_repository property/default kotlin-stdlib-docs' own
settings.gradle.kts uses) - kotlin-stdlib-docs' own
dokka-samples-transformer-plugin subproject needs a Dokka dev build that
isn't on Maven Central, and the allprojects{} repositories block here had
removed access to it for every subproject.
Verified end-to-end against a real kotlin checkout: fresh plugin build,
JSON generation (common+jvm only), and sync into a scratch documentation.db
all succeed, with blacklist pruning verification passing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Documents what OfflineDocumentationTools is (the offline tooling that builds/edits the documentation database consumed by Code on the Go), how its pieces relate to each other, and — most importantly — the gap between the schema this repo's tools expect and the schema the current production documentation.db actually has (added templateId/Templates/ Bookshelf/BookCategories/PUCC_* tables, dropped the legacy ide_tooltip_table). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI counterpart to run_e2e_pipeline_test.sh: builds kotlin-stdlib/-reflect/-test JSON docs and the Kotlin website docs, then loads both into a Drive-hosted copy of documentation.db and uploads the result back to the same file. Google Drive file IDs (database and Writerside's webHelpImages.zip) resolve from secrets/inputs by default, with hard-codeable TEST_*_FILE_ID overrides for one-off manual testing. Posts "Grabbing baton"/"...Dropping baton" to Slack (SLACK_WEBHOOK_URL) around the run, since it mutates a single shared Drive file. Defaults to dry_run so a first run never touches production. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…739) Adds the code that loads converted Kotlin website content (build_nav.py, populate_db.py, media insertion) and kotlin-stdlib/-reflect/-test JSON content (sync_kdoc_json_to_db.py) into documentation.db, the local e2e test script for that pipeline, and the CI workflow that runs it end-to-end against a Drive-hosted copy of the database. Split out of the larger Kotlin-docs pipeline PR (#21) so the DB-manipulation side (this ticket) can be reviewed separately from producing the raw JSON data for the Kotlin website (ADFA-5039, PR #23). This PR depends on ADFA-5039 merging first - populate_db.py, build_nav.py, and find_missing_assets.py all import md_to_json.py, which isn't included here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
hal-eisen-adfa
left a comment
There was a problem hiding this comment.
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):
CLAUDE.mdmakes a factual claim abouttemplateIdthat the same commit falsifies — and reasons from it to a scope conclusion.nav.pebemits anav-hiddenclass 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.
Blocking: CLAUDE.md's templateId "out of scope" claim was falsified by this same PR (populate_db.py/insert_optimized_media.py both read/write it) - narrowed the claim to the names that are actually absent. The nav-hidden class nav.peb emits was inert (no consuming CSS rule), rendering Writerside-hidden nav entries (e.g. individual tour steps) visible - added the missing docs.css rule. Non-blocking: rewrote the Dokka-plugin-kdoc2json bullet (and decisions log) to reflect that fix/ADFA-4514 is merged, rather than describing it as an outstanding rebase. find_missing_assets.py swallowed per-file scan failures and always exited 0, so a totally broken corpus still looked clean - added a failure counter, --allow-failures flag, and a report line, mirroring md_to_json.py's pattern from #23. Converted README.md, optimize_media.py, and run_e2e_pipeline_test.sh from bare pip/python3 to uv run --with-requirements, and added scour/cairosvg to requirements.txt, matching the repo's established uv convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Pushed 66da59d addressing your review. Replied inline to each of the 5 line comments individually; summary here. Blocking (2)
Non-blocking (3)
TestsAdded Verified the |
Sent prematurely, apologies
# Conflicts: # ProcessDocs/ProcessKotlinDocs/ProcessKotlinWebsiteJSON/README.md # requirements.txt
Its own read_text(encoding="utf-8") sat outside any try/except, so a malformed file raised uncaught and killed the whole process - bypassing --allow-failures entirely, unlike every other file read in this script. Now catches, counts, and reports it the same way the main conversion loop already does; find_include_warnings returns (warnings, failed) and main() folds that count into the existing failure total. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
populate_db.py trains a zstd fast-cover dictionary (256 KiB) from this run's own pages/nav on first use and stores it in a new CompressionDictionary table, then compresses every page/nav/image/asset row against it via the brotli CLI's -D flag (the installed Python brotli package has no dictionary API). Never retrains an existing dictionary: a dictionary-compressed row is only decodable against the exact dictionary it was compressed with, verified empirically to fail silently-wrong rather than loudly on a mismatch, so retraining would orphan every already-migrated row. insert_optimized_media.py rewrites the same rows populate_db.py writes (image optimization, in-place URL rewrites), so it now loads and reuses the same dictionary instead of the old plain-Brotli calls it would otherwise silently corrupt those rows with. ADFA-5153.
populate_db.py and insert_optimized_media.py only ever touch their own subset of Content (k/html/%, assets/%). Every other Content row -- reference docs, tooltip-linked pages, whatever else -- was still plain Brotli, no dictionary. migrate_content_to_dictionary_brotli.py recompresses every remaining 'brotli' row against the shared CompressionDictionary (training one from a representative whole-corpus sample if none exists yet), so the "every brotli row uses the dictionary" assumption WebServer.kt's reader depends on actually holds. Idempotent by construction: a plain decode reliably fails once a row is already dictionary-compressed (verified over 200 trials), so re-running is always a safe no-op. Backs up first (VACUUM INTO), runs in one transaction. Run against the real documentation.db: 29,748/29,751 brotli rows migrated, 131.1MB -> 85.6MB compressed, 299.0MB -> 255.3MB overall. ADFA-5153.
Every 'brotli' Content row in the real database is now compressed against the shared CompressionDictionary (see the prior two commits), but docdb_studio.py still read and wrote plain Brotli in three places: get_html_anchors_for_path, fetch_content_for_path (both decode), and compress_for_storage via import_content_files (encode). Against the migrated database this wasn't a latent risk -- it was already broken: a plain decode of dictionary-compressed content reliably fails, so anchor validation and content preview were silently erroring on every real page, and any new import would have written dictionary-incompatible plain Brotli back into a database that assumes there is none left. get_compression_dictionary(db_path) reads and caches a database's CompressionDictionary (or None, for a database that predates ADFA-5153) -- docdb-studio never creates or retrains one itself, only ever reads whatever another tool already produced. compress_for_storage/decompress_brotli shell out to the brotli CLI's -D flag when a dictionary is present, matching populate_db.py's approach, and fall back to the plain brotli package otherwise. decompress_brotli deliberately raises brotli.error on failure so the two existing call sites' `except brotli.error:` handling didn't need to change. Verified against the real (migrated) documentation.db: anchor lookup and content fetch both now work on real pages that previously would have errored. ADFA-5153.
Each row's recompress spawns its own `brotli` subprocess, so the ~30,000-row real migration was dominated by process-spawn overhead running strictly sequentially. Retrospective feedback: this should have been parallelized from the start rather than accepting a slow serial run. migrate() now runs reassemble+plain-decompress+dictionary-recompress on a ThreadPoolExecutor (defaults to ThreadPoolExecutor's own min(32, cpu_count+4), tuned for exactly this I/O/subprocess-bound shape); each worker opens its own read-only connection (a single sqlite3.Connection isn't safe across threads) and reuses one DictionaryCompressor per thread rather than one per row. The actual delete+insert writes stay serialized on the caller's connection, which SQLite requires anyway. Measured 3-6x faster than sequential on synthetic benchmarks. DictionaryCompressor gets an atexit safety-net close(), since a per-thread instance has no single call site that can cleanly scope a `with` block around it the way populate_db.py's/insert_optimized_media.py's own single-threaded usage already does. Test fixture switched from :memory: to a real temp file, since worker threads need an actual db_path to open their own connections against - an in-memory database has none and can't be shared across connections at all. ADFA-5153.
This is the pipeline that actually produces the live documentation.db (scripts/DocumentationDatabase.py, fixed earlier on this ticket, turned out to be dead code -- its tag-triggered workflow hasn't fired since db-2025-07-16b). populate_db.py has always run its own bare VACUUM with no page_size pin, so the real fix belongs here. Extracted vacuum_and_pin_page_size(), mirroring docdb_studio.py's vacuum_database(): pins page_size via PRAGMA before VACUUM, and works around WAL journal mode silently preventing PRAGMA page_size from taking effect (this file's own backup_database docstring already anticipates a live/WAL-mode database). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
vacuum_and_pin_page_size (commit b203500) mirrored docdb-studio.py's original vacuum_database(): in-place VACUUM + a journal_mode round-trip, which requires exclusive access to db_path. SQLite refuses to switch a WAL-mode database away from WAL while ANY other connection has it open -- even one from a function that has already returned, since Python's `with sqlite3.connect(...) as conn:` does not close conn on exit. Empirically reproduced and fixed the identical bug in docdb-studio.py's vacuum_database (PR #25); this mirrors that fix here since this pipeline's own VACUUM is the one actually run against the live documentation.db. Rewritten on VACUUM INTO: rebuild into a temp file next to db_path (read-only snapshot of the source, no exclusive access needed), then atomically swap it into place with os.replace. journal_mode=WAL is reapplied to the new file's final path (VACUUM INTO always produces a plain rollback-journal file), and stale sidecars from the replaced file are cleaned up. Two new tests: the fix succeeds with both an unrelated open connection and an unclosed caller-style connection present at once (the actual scenario the old design was fragile against), and the original file is left untouched if VACUUM INTO fails partway (temp file cleaned up, no partial swap). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
tempfile.mkstemp() always creates its file mode 0600 regardless of the original's mode or the process umask. The VACUUM INTO rewrite swaps that temp file into db_path's place via os.replace, which never restored the original permissions -- alexmmiller's QA of the mirrored docdb-studio.py fix caught this silently dropping documentation.db from 644 to 600 on every vacuum; same bug here since this pipeline's vacuum_and_pin_page_size uses the identical mkstemp+replace pattern. Capture db_path's mode before the rewrite and os.chmod it back after the swap. New test confirms a 644 file stays 644 across vacuum_and_pin_page_size (and fails against the pre-fix code, dropping to 600). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nal_mode-read connection Same fix as the mirrored docdb-studio.py version: VACUUM INTO's target accepts a bound parameter (already used by this file's own backup_database for the same reason), sidestepping SQL string-literal escaping for a path containing a single quote (e.g. "David's Docs") rather than hand-rolling it. Also explicitly closes the journal_mode -read connection instead of relying on it being reassigned by the next `with` block. New test: a quote in db_path's parent directory no longer breaks the statement. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same findings as the mirrored docdb-studio.py fix's third self-review: - chmod the temp file to the original permissions before os.replace, not after -- fixing it up afterward left a real window where db_path was visible at mkstemp's 0600, and left permissions permanently wrong if the chmod itself failed. - Explicitly close the VACUUM INTO and WAL-reapply connections, and give the WAL-reapply connection the same 30s timeout as its siblings in the same function. 19/19 local tests pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…to ADFA-5153 Benchmarking showed page_size=1024 vs 2048 has essentially the same performance and a negligible size difference before compression (and likely less after this PR's dictionary compression) -- adding complexity without benefit. ADFA-5141 is declined; this PR is only about the Brotli dictionary compression (ADFA-5153) and the page_size work rode along on this branch by coincidence of timing, not by scope. Restores populate_db.py's original plain VACUUM call and removes vacuum_and_pin_page_size, SQLITE_PAGE_SIZE_BYTES, the now-unused os/stat imports, and their dedicated test file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
WebServer.kt's reassembly loop always probes "<path>-1" first, but 14 of 19 chunked Content rows in the real documentation.db number their continuations starting at "-2" instead, with no "-1" row at all. The first lookup misses, the loop stops after the base 1 MB chunk, and the row is served short: a corrupt image (compression='none', silent 200) or a decode failure (compression='brotli', 500) - confirmed against a local copy of the shipped database (md5 34c879595bd6fb87e5b68989369680a8). No writer in this tool ever produced that numbering - populate_db.py, insert_optimized_media.py, and migrate_content_to_dictionary_brotli.py all go through insert_chunked_content, which has always started fragments at -1. This is inherited data older than this pipeline, not something it can regenerate correctly by re-running existing tools. renumber_misnumbered_fragments.py finds base rows whose fragment chain (via LIKE, sorted on the parsed numeric suffix rather than assumed paths) doesn't start at 1, and renumbers it to a contiguous run starting at -1, lowest-suffix first so each rename's target is the path just vacated by the previous one. A chain with an actual gap (a genuinely missing chunk, a different failure) is reported and left alone rather than guessed at. Content bytes are never touched, only paths, so it's safe regardless of a row's compression. Verified against a scratch copy of the real database: renumbers exactly the 14 chains the ticket found, and the two example rows (the devsite gif, the Javadoc index) reassemble and decode correctly afterward.
…bering ADFA-5171: Repair chunked Content rows misnumbered from -2
Every finding from the three reviews on PR #26 that lands in files this PR touches, plus the one dictionary-consistency problem outside it that this PR itself creates. Data loss, both silent: * The migration deleted and re-inserted each base row. Content carries AddBook/DeleteBook triggers on '%.pdf' paths, so that cycle replaced every curated Bookshelf entry with 'CURRENT_TIMESTAMP || id' under a fresh Content.id -- verified on the real database: (53507, category 5, "Android Notes for Professionals") became (53508, category NULL, "2026-08-21 22:37:5553508"). 15 brotli-typed .pdf rows and all 7 Bookshelf rows are in scope. Writes are now UPDATE in place, with continuation rows reconciled by exact path. * delete_content interpolated a path straight into LIKE, where `_` is a wildcard and the `-%` suffix was not restricted to digits, so unrelated rows could be deleted permanently (hal-eisen-adfa). No write path goes through LIKE any more. Rows silently skipped while the run reported success: * reassemble_content probed "<path>-1", so an ADFA-5171 chain numbered from -2 reassembled truncated, failed to decode, and was counted as "already dictionary-compressed". The corpus has 29,751 base rows and exactly 3 with continuations; the run reported 29,748 migrated and 3 already-migrated in a first-ever migration, which is precisely those 3. Chain discovery is now shared with the repair script (populate_db.fragment_chain), so the two cannot drift apart again. * Any decode failure counted as "already migrated" (hal-eisen-adfa). Rows are now classified by decoding both ways: identical either way means the encoder never referenced the dictionary and there is nothing to gain (~0.5% of the real corpus, and the reason a second run used to re-migrate them -- alexmmiller); plain-only means migrate; dictionary-only means done; neither is an error, never a success. * Recompressed bytes are verified to round-trip before being written. Concurrency and memory: * Each worker opened its own read connection while the caller held one write transaction over the whole run, which deadlocks under journal_mode=delete -- documentation.db's actual mode (alexmmiller). All database access is now on the calling thread; workers receive bytes. Commits are batched, so an interrupted run keeps finished batches and resumes. * Blobs are no longer selected for every row up front (~130 MB held at once). Dictionary training, measured on the real corpus with only the sampling varied: first 300 rows by path (all under "a/") 36.2% smaller than plain 300 rows stratified across doc sets 33.2% <- worse stratified, 32 MiB plaintext budget 48.3% <- best first-by-path, same 32 MiB budget 36.4% <- volume alone: nil The docstring promised "a random sample drawn across the WHOLE Content table" and delivered the first 300 paths alphabetically -- 299 of them under "a/", while j/ (10,326 rows) and k/ (3,757) trained nothing (hal-eisen-adfa). Fixing it by stratifying alone makes things worse: quotas drawn from smaller doc sets starve the trainer, which then cannot even fill a 256 KiB dictionary. Both halves are needed, so sampling is now stratified by stored bytes and bounded by a plaintext budget, seeded for reproducibility since a stored dictionary is never retrained. renumber_misnumbered_fragments: * A chain numbered from -0 passed the "starts at 1?" guard and renamed onto an occupied slot, tripping UNIQUE(path) and rolling back every other repair in the pass (hal-eisen-adfa). Such a chain is repaired rather than skipped -- the app probes "-1", finds it, and serves the chain with "-0" dropped -- via a parking pass that is correct in either shift direction. docdb-studio: * sqlite3.OperationalError covers "database is locked", and caching that as "no dictionary" downgraded the whole session to plain Brotli (hal-eisen-adfa). Only definitive answers are cached now. * The new `brotli` CLI dependency raised RuntimeError/OSError out of paths that guard only `brotli.error` (hal-eisen-adfa). Missing-binary now raises a BrotliCliMissing subclass of brotli.error, with an actionable message. * decompress_brotli decoded dictionary-only, so it could not read plain rows -- which a dictionary database always contains: anything a plugin contributes on-device, anything written outside populate_db.py, and everything mid- migration. It now falls back to a plain decode, as WebServer.kt does. sync_kdoc_json_to_db (outside this PR's diff, but this PR is what makes documentation.db a dictionary database): * compress_for used plain brotli.compress, leaving every k/kotlin-stdlib row plain inside a dictionary database (hal-eisen-adfa). It now compresses against the database's dictionary when there is one. * "Source file missing => delete the row" had no floor: a Dokka layout change makes every lookup miss, and the script would delete every stdlib row plus its parent Tooltips and exit 0 (hal-eisen-adfa). Sources are resolved up front and a wholesale miss aborts. Corrected in populate_db's DictionaryCompressor docstring, because two reviews reasoned from it: the two mismatch directions are not alike. Decoding a dictionary row with NO dictionary is loud (398 of 400 real rows raised, 2 returned identical bytes, none wrong), which is what makes both this script's idempotency check and WebServer.kt's fallback sound. Decoding with the WRONG dictionary is the silent case (50% raised, 38% returned different bytes with no error, 12% identical). The test asserting a wrong-dictionary decode does not raise was asserting that coin flip; it now asserts the invariant that holds. Tests: 25 in ProcessKotlinWebsiteJSON (up from 21) and 173 in docdb-studio (up from 170) pass. New coverage for the -2 chain, an undecodable row, Bookshelf survival through the triggers, a never-referenced-dictionary row across two runs, stratified sample determinism and spread, zero-based renumbering, one bad chain not blocking other repairs, a locked database not being cached, a plain row in a dictionary database, and a missing brotli CLI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All 15 findings fixed, with regression tests and a CI job — d9df0f0Thanks 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 I checked the claim rather than assuming it. Stashing the source fixes and running the new tests against the pre-fix code: The 3 that pass both ways are deliberate must-not-regress guards: non-colliding names keep their stems, a contiguous chain from That last one earned its place immediately. My first cut at F02 gated the continuation test on a contiguous walk from On "no workflow in this repo runs pytest at all"Added Verified under act rather than assuming it works: From the lower-severity listFixed: Not fixed, deliberately, so they don't look handled:
Happy to take any of the three in a follow-up if you'd rather they land here. State358 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 |
Resolves the six conflicts by keeping this branch's versions, which are the newer side in every case: main last touched all six in e83bb9d (ADFA-5153, #30, 2026-08-24), this branch in d9df0f0 (2026-09-01). Checked before discarding main's side that nothing was actually lost - every main-only line is an earlier form of something this branch has since changed: build-kotlin-docs.yaml the twice-stated blacklist (now one job env var), the pre-kotlin_libs step 4/5 call, and the "Dropping baton" step gated on !dry_run populate_db.py the pngquant pass (removed as redundant - 0 of its PNG rows survive step 3) and the backup taken before the conversion-failure refusal insert_optimized_media.py reassemble_content probing from "-1"; the rest of its diff is indentation from wrapping optimize_directory in try/except ValueError migrate_content_...brotli.py the unguarded "X-<digits>" continuation test sync_kdoc_json_to_db.py the pre-chunking version: copy2 backup, no fragment handling, "#fragment"-only URI split docdb_studio.py get_compression_dictionary without strict=, and _find_brotli_cli resolved inside the argv The merged tree is byte-identical to d9df0f0, and every file main changed without conflicting (docdb-regression-test.yaml, docdb-studio/README.md, remint_dictionary.py, the four dictionary test modules) is already identical here, so this merge records the shared history without changing content. 358 tests pass (169 + 189). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
hal-eisen-adfa
left a comment
There was a problem hiding this comment.
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.
F01 / F02 / F08 / F13 — one chunking protocol, four implementationsThese 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
The correct implementation
is_chunked_base(lengths, prefix) # gates on length == CHUNK_SIZE
load_base_rows(...) # uses that gateThat is the reference. It encodes rule 1 explicitly and is the only copy in the repo that does. How each site deviates
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 approachExtract the protocol into one module —
The shared module should be the one place that knows |
F04 / F05 / F10 — one root cause in
|
| 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:
- De-conflict on that predicted name, keyed by basename alone — matching the flat namespace
insert_optimized_mediaactually 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 gratuitousrenamedentries and no needless stored-URL rewrites. - Validate the inventory at collection time — drop and report entries that are not readable regular files — and move the
src.stat()insideprocess_file'stryso a mid-run disappearance is still counted instats["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.
Several of these are regressions from the previous round's fixes; the review
grouped them by root cause, and both groups are addressed at that level rather
than site by site.
Group 1 - one chunking protocol, four implementations (F01/F02/F08/F13)
WebServer.kt reassembles a page by reading `path`, and if that blob is exactly
CHUNK_SIZE, appending `path-1`, `path-2`, ... until a fragment comes back
shorter. Two rules follow: a "<base>-<N>" row is a continuation only if the
base is exactly CHUNK_SIZE (a base merely existing proves nothing - "X" and
"X-1" are legal as unrelated pages), and a short fragment terminates the
chain. Both were being re-derived per call site, each wrong differently.
New content_chunking.py owns both rules and distinguishes the two questions
they answer: owned_fragment_paths (ownership - what a delete or replace must
take, including an orphaned tail past a gap) and served_fragment_paths /
reassemble (what the server actually concatenates). populate_db,
insert_optimized_media, migrate_content_to_dictionary_brotli,
renumber_misnumbered_fragments and sync_kdoc_json_to_db all now call it; the
two duplicate CHUNK_SIZE definitions and eight ad-hoc fragment helpers are
gone.
* F01 unmatched_source_pages was handed {row[0] ...} - integer Content ids -
so the membership test never matched and every source page was reported
unmatched. all_paths, already computed 38 lines above, is what it wanted.
* F02 write_content ran an unindexed "LIKE '<path>-%'" scan per updated row
(~1e9 comparisons across kotlin-stdlib) for a lookup that can only return
rows when the base is CHUNK_SIZE. Now gated on that first.
* F08 find_fragment_paths treated "base exists" as sufficient, so an ordinary
page at guide.html hid a genuinely misnumbered chain at guide.html-1 from
the tool written to repair it. Same defect fixed in sync's is_fragment_path.
* F13 reassemble_content had dropped the short-fragment terminator, so on a
gapped chain it rebuilt a blob the server never serves - which rewrite_pages
would then re-compress and store.
Discovering a chain now depends on the base row's length, so three call sites
that deleted the base before resolving its chain had to be reordered.
Group 2 - optimize_media's name-planning pre-pass (F04/F05/F10)
The pre-pass planned from source-side attributes and never reconciled that
against the predicted output name or whether the source was readable.
* F04 keyed collisions on (parent, stem), but insert_optimized_media
addresses images by bare basename - so sub-a/logo.png and sub-b/logo.jpg
both became logo.webp, one image lost, one page rendering the wrong picture.
* F05 the mirror image: it renamed pairs that could not collide (no --webp
means no extension rewrite), churning stored URLs for nothing.
* F10 "not is_dir()" keeps broken symlinks, and process_file stat()'d outside
its try, so one dangling link killed the run with an unhandled traceback.
Now de-conflicted on possible_output_names() keyed by basename alone, and
collect_sources() drops unreadable entries while stat() moved inside the try.
Remaining findings
* F03 the previous round's comment was wrong: nav.peb branches on
`{% if node.id %}` and renders a *coloured link*, not a non-link, so the
sidebar entry for an unconverted *.topic still 404'd. Clearing the id is
what actually fixes it, and drops it from the pager for free. Verified on
the live corpus: one such node (api-references), zero nav ids now without a
Content row.
* F06 the strict= dictionary lookup ran inside the import's own write
transaction, so a self-inflicted lock aborted the whole import. Resolved
and cached before the write connection opens.
* F07 the INT/TERM/HUP handler restored state but never exited, so Ctrl-C
carried on into the rest of the script and the EXIT trap then re-ran a cp
of an already-removed file. Handlers exit 130; restore is idempotent.
* F09 sidebar.js never checked response.ok, so a 404/500 body was injected
and rendered as the navigation.
* F11 the act script still documented "build complete" as gated on dry_run
after the workflow moved to if: always().
* F12 all three workflows moved off pip to the repo's uv convention (a
job-level $UV_RUN mirroring run_e2e_pipeline_test.sh); the dead
"pip install markdown-it-py" is gone, it having been in requirements.txt.
* F15 is_contiguous_from_one was dead after find_chains was rewritten.
F14 (add check-tools to CI) is NOT done, deliberately: 6 of its 13 tests
already fail on main, in code this PR doesn't touch, so the step would make
every PR red for an unrelated reason. The header now says so and carries the
one-liner to add once they pass.
11 new regression tests over the inputs the review identified; the 5
behavioural ones fail against the pre-fix code. 181 + 189 pass. The local
workflow runs green end-to-end under act on the uv conversion.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All 14 findings addressed (13 fixed, 1 deliberately not) — 46235fbBoth 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 protocolNew
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-passKeying 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 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 restF03 — you were right to read the template instead of my comment. F06 was mine (the F14 — not done, deliberatelyYour 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 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. Verification11 new regression tests over the inputs you identified; the 5 behavioural ones fail against the pre-fix code: Plus 181 + 189 tests pass. The local workflow runs green end-to-end under act on the converted uv form — blacklist verification PASS, 473 Workflow parity improved as a side effect of the uv conversion: 15 identical shared steps now, with only the apt line differing ( F15 ( |
Two real defects, both in last round's fixes, plus three smaller ones. content_chunking.served_fragment_paths implemented half its own terminator rule. The module docstring says reassembly stops at the first fragment shorter than CHUNK_SIZE "or the first one missing", but only the short- fragment half was coded, so a chain with a hole jumped it and appended a tail the server never reaches. Reproduced with p-1 (full), p-2 (full), p-3 missing, p-4 (short): it returned p-1,p-2,p-4 and reassembled 3,145,744 bytes where WebServer.kt probes p-3, misses, and serves 3,145,728. That is the same fabrication F13 was raised about, reached from the gap side instead of the short-fragment side, and gapped chains are not hypothetical - renumber_misnumbered_fragments has a category for them it deliberately leaves alone. Contiguity is now enforced, from whatever suffix the chain begins at rather than from 1, so an ADFA-5171 chain numbered from -2 is still readable whole by the migration and repair tooling. owned_fragment_paths is unchanged: a delete still has to take the orphaned tail. The dangling-nav-link fix was applied to the database path only. build_nav.py renders the same nodes through the same `if node["id"]` branch, so its standalone output still emitted live links to pages nothing generates - and the committed templates/nav.html shipped two of them, /home.html and /api-references.html. The pass is now a module-level build_nav.drop_unreachable_ids that populate_db imports rather than reimplementing, since the static site and the database have to agree on what is reachable. nav.html is corrected by hand rather than regenerated: kotlin-web-site has moved on (304 topics vs the 303 it was built from), and a full regen would fold unrelated upstream drift into the artifact. The two entries now match what the fixed generator emits, byte for byte. Also: * optimize_media's output de-confliction keyed claimed names as written, so logo.PNG and logo.png never de-conflicted despite being the same file on a case-insensitive filesystem - the F04 namespace mismatch reached through case. Keys are casefolded now. Latent: it needs the source and output filesystems to differ in case sensitivity, and the pipeline runs on Linux end to end. * 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, and an untested fourth copy is the divergence this module exists to prevent. * "Notify Slack: build complete" ran on a bare always(), so a run failing before "build started" (a bad db_path, a failed download) posted a baton release for a lock it never acquired - F14's asymmetry pointing the other way. Now gated on the acquire step's own outcome. 6 new tests; the 2 that pin the gap terminator fail against the pre-fix code. 187 + 189 pass, and the local workflow is green end to end under act. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Self-review round: 5 findings, all fixed — a59d777No new reviewer feedback here. After the last round I ran 1.
|
hal-eisen-adfa
left a comment
There was a problem hiding this comment.
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_nodesays 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. Itsindentparameter is accepted at line 182, never read, and not passed down to the recursive call at line 210.find_missing_assets.FENCE_REstrips 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.
F25 (critical, act input inversion) did not reproduce - two act runs show
the negation gates evaluating correctly in both directions, and the
suggested `inputs.x != 'true'` would invert on real GitHub instead. The
other ten are fixed:
- F26 an animated GIF is no longer predicted as .webp (it cannot be
re-encoded), and duplicate basenames keep-first-skip-rest rather
than de-conflicting onto a name nothing writes
- F27 optimize_directory returns OptimizeResult(renamed, written); the
insert loop follows `written` instead of rglob-ing the work dir
- F28 drop the unreachable `b.type == "image"` branch from page.peb, and
record in both places that IMAGE_REF_RE's anchoring depends on it
- F29 re-convert the pages that linked to a failed page before its stem
was dropped, so broken-link styling covers the whole corpus, not
just what converted after the failure
- F30 list_stored_media asks is_continuation_path, so an image genuinely
named "<other>-1" is listed rather than swallowed
- F31 a GIF's loop count is passed through only when the source had one;
info.get("loop", 0) turned "plays once" into "loops forever"
- F32 renumber_misnumbered_fragments scans before backing up, and
vacuums only when it moved rows
- F33 sys.path.append, not insert(0), so the stdlib still wins
- F34 rewrite_pages returns the references it saw; delete_unreferenced_
media reuses them instead of decompressing every page a second time
- F35 arity checks before each `$2` read in both shell scripts
Also from the lower-severity list: render_node's "byte-identical to
nav.peb" claim is corrected (whitespace differs; a Pebble 3.2.2 render
confirms nothing else does) and its unused `indent` parameter dropped;
find_missing_assets uses md_to_json.fenced_spans instead of a second,
backtick-only fence pattern.
20 new tests. 174 + 33 + 189 pass; the 5 failures and 1 collection error
in scripts/ are pre-existing on the base commit. Local workflow green
end-to-end under act with identical output to the previous round.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third review round: 10 of 11 fixed, 1 disprovedPushed as a single commit, F25 (critical) did not reproduceYou flagged this as the one to verify first, so I did. Two act runs through
The input file's md5 changed,
The other ten
Also from the older lower-severity list: Two corrections to my own earlier claims"Byte-identical to F34's cost. I measured instead of repeating the estimate: the corpus has 268 One thing worth flagging backF29 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 Verification20 new tests, in
174 + 33 + 189 pass. The 5 failures and 1 collection error under 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 Workflow parity re-checked and unchanged: the only differences between |
Self-review of the previous commit, which introduced two of these. - The duplicate-basename skip added for F26 keyed on src.name.lower(), but populate_db indexes image basenames exactly - so "a/Logo.png" and "b/logo.png" are two addressable images there, and dropping one left the page referencing it resolving to a row nothing inserts. They are a collision, which `claimed` already de-conflicts, not a duplicate. - The F35 test shelled out to run-build-kotlin-docs-with-act.sh, whose `command -v act` check ran before argument parsing - so it failed on any machine without act, including this PR's own python-tests.yaml. The check now sits with the other validation, just before the first thing that needs act; a mistyped flag is reported as a mistyped flag. - F26's animated exemption stopped at .gif, but optimize_raster copies *any* animated raster through unchanged, so an APNG or multi-frame TIFF under --webp still claimed a .webp nothing writes. Replaced with ANIMATABLE_EXTENSIONS; .webp is excluded (same name either way) and JPEG/BMP cannot be animated, so nothing pays for a probe it can't use. - possible_output_names now takes the answer rather than the path, so it does no I/O and the probe happens once per source instead of once per de-confliction attempt. - optimize_directory and rewrite_pages kept their old -> dict / -> int annotations after returning NamedTuples. - fenced_spans runs an unterminated fence to EOF, so one stray ``` line silently stopped find_missing_assets checking the rest of the file - a false negative in the report whose job is catching what's missing. It now says so, and only for a genuinely unclosed fence. - Dropped dead `if False` scaffolding and a vacuous assert from the F28 test. - repair() answered a half-supplied scan with a fresh one, silently discarding the caller's snapshot; it refuses now. 14 new tests; every one that can fail against the previous commit does. On the real 299-image corpus the optimize pass is byte-for-byte unchanged - same 299 written, same 219 renames, same output bytes, and the added probe costs nothing measurable (38.6s vs 39.0s). Running insert_optimized_media against the real database gives the same 160 .webp rows as the last act run. 188 + 33 + 189 pass; scripts/ has the same 5 pre-existing failures as the base commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two of these are misses in the previous commit's own fixes.
- The act check moved above the argument *validation* rather than below
it, so five semantic errors (both --skip flags, --db-path missing or
nonexistent, --images-zip-path missing or nonexistent) were still
answered with "act is required" on a machine without act. It now sits
immediately before the act invocation; all five report themselves, and
act is still required once the arguments are good.
- possible_output_names' undetermined case claimed both names, which was
widened from GIFs to every PNG and TIFF by the last commit. It now
claims nothing: optimize_raster repeats the same Image.open and
is_animated access, so whatever makes the probe fail makes process_file
fail too, and the source writes no file at all. Holding two names for
it de-conflicted a real image against an output that never appears.
And the deeper one behind both of the last two rounds' F26-shaped bugs:
optimize_directory now checks that the name process_file actually wrote
was one the planning pass claimed for that source, and fails loudly
otherwise. The prediction lives in a second place from the code it
predicts and has drifted twice, silently both times; this is the missing
connection between the halves.
Also:
- _is_animated_raster reports a probe failure through the logger instead
of swallowing it - it runs for every PNG now, and a systematic failure
would otherwise shift the whole de-confliction pass unexplained.
- outside_fences' `source` is required, not defaulted: the warning is the
entire point of that change, and an optional argument is how a later
call site quietly gets the old silence back.
- The unterminated-fence check runs once on spans[-1]. Only the last span
can be unterminated (fenced_spans appends the run-to-EOF one after its
loop), so checking every span re-split every code block for nothing.
- `animated: Optional[bool]`, and a non-bool argument is refused - the
parameter took a Path one revision ago, and a Path is truthy, so a
stale call reported "animated" for every file with no error.
- The case-handling test asserts the property rather than the
"{stem}-{ext}" naming literal.
6 new tests, and every changed behaviour fails against the previous
commit. 192 + 33 + 189 pass; scripts/ unchanged from base. The real
299-image corpus is byte-for-byte identical again, and the local
workflow under act produces the same database md5 as the last two
rounds - with the new written-vs-claimed check silent across every path
the corpus exercises, including SVG rasterization.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The drift check added in the previous commit crashed on ordinary input. possible_output_names' SVG branch returned the lowercase SVG_EXTENSION literal instead of echoing the source's own suffix - every other branch already echoes it - so "Diagram.SVG" was claimed as "Diagram.svg" while optimize_svg writes `dst`, which keeps the source's spelling. Harmless while it only fed the casefolded `claimed` map; once the check compared what was written against what was claimed it aborted the entire run over one ordinary file, after all 299 images had been optimized. The corpus has no uppercase extensions, so neither the full-corpus run nor act could have caught it. Three things had to change for that to be safe, not just quiet: - The check asks `claimed`, casefolded like every other lookup in the pass, instead of a second exact-case dict. De-confliction guarantees no two sources hold the same lowercased name, so "did this source claim this name" is exactly `claimed[name] is src` - and `claimed_for` is gone rather than kept in step by hand. - A write from a source that claimed nothing (its animation could not be determined, so the planner expected optimize_raster to fail too) is one file's bad luck, not drift. It is now reported and the file kept - this module's rule is that a single file's problem does not end the run, and a dangling symlink doing exactly that was fixed as a bug in this same PR. Kept rather than dropped, deviating from the review's suggestion: discarding a successfully optimized image is the worse outcome, and insert_optimized_media's seen_names guard still covers the only residual risk. - Genuine drift raises ValueError, not RuntimeError, so insert_optimized_media's `except ValueError` turns it into a clean "error: ..." line instead of letting a traceback escape main(). Also `logger: Optional[Logger]`, the annotation this commit's parent fixed one function above and repeated here. 9 new tests, including the uppercase case across .SVG/.Svg/.PNG/.JPG/.TXT with and without --webp; the six raster/passthrough parametrizations pass against the parent commit too, which is what pins the crash to the SVG branch. 204 + 33 + 189 pass; scripts/ unchanged from base. The real 299-image corpus is byte-for-byte identical, and act produces the same database md5 as the last three rounds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The drift check's exemption keyed off the wrong thing, and let real drift through. `if src in unpredicted` read as "the planner claimed nothing for this source", but it recorded "the probe failed" - and those come apart under --webp false, where possible_output_names returns the source's own name before it ever consults `animated`. So for any file whose probe failed, a writer that produced a name nobody claimed got a warning and an exit 0 instead of the error the check exists to raise. The warning even said "the de-confliction pass held no name for it" while `claimed` held one. Rather than correct the condition, remove the case: optimize_raster now takes the planner's `animated` answer instead of re-deriving it with its own `getattr(img, "is_animated", False)` a few lines later. Two answers to one question was the whole reason a source could be unpredictable here and processable there. A source whose animation is undetermined is now refused in optimize_raster, counted by process_file as one file's error like any other, and never reaches the check - so the planner's "claims nothing" is always accurate and the exemption has nothing left to describe. Both answers already came from the same access on the same file, so this only changes behaviour on a transient. The per-source plan is one record (Plan: dst_rel, names, animated) instead of a dst dict beside a parallel set - every parallel structure this pass has grown ended up disagreeing with the one next to it, which is what both of the last two rounds' bugs were. Also: - The drift message reports `plan.names`, the claim as the planner wrote it, not `claimed`'s casefolded keys - reporting "diagram.svg" for a source that claimed "Diagram.SVG" points the reader at a case mismatch that isn't the problem. - The check compares with == rather than `is`: Path defines __eq__, and identity held only because two loops happened to share list objects. - The uppercase test asserts the written name per case, not just that one file came out. Dropped test_drift_is_refused_even_when_the_probe_failed as written: it stubbed the probe to False - a probe that succeeded - so it demonstrated nothing about the exemption and passed against the unfixed tree. The regression for that hole is the --webp False half of test_an_undetermined_probe_skips_one_file_and_no_more, which fails against it. 206 + 33 + 189 pass; scripts/ unchanged from base. Real 299-image corpus byte-for-byte identical, and act gives the same database md5 for the fourth round running. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit flattened every animated WEBP to a single frame. optimize_raster stopped deriving is_animated itself and took the planner's answer instead - but the planner probed only ANIMATABLE_EXTENSIONS, and .webp was deliberately excluded from that set on the grounds that "an animated WEBP is copied through as a .webp, which is what the conversion branch predicts anyway, so there is nothing to resolve and no reason to pay for opening the file". True for predicting a *name*; wrong once the same value decided whether to re-encode. A 4-frame webp came out with 1 frame, exit 0, no warning - and the written-vs-claimed guard could not see it, because the name was exactly what was predicted. Only the frames were gone. The set is gone with it. One question - "is this file animated" - now gets one answer, taken from the file, for every raster; and the naming rule follows from it uniformly: under --webp a raster keeps its own extension exactly when it is animated, because that is when optimize_raster copies it through instead of re-encoding. No curated membership to get wrong, which it had been twice (first .gif alone, then a four-entry set missing .webp and .jpg). That also restores, and this time documents, the handling of an MPO stored as .jpg - a phone stereo or burst capture, which Pillow reports as animated. It is copied through rather than flattened to its primary frame; the previous commit changed that silently as a side effect. Also: - optimize_raster's copy-through comment named webp as its example while that branch was unreachable for webp. It says what the branch really covers now, MPO included. - process_file's docstring said `animated` was "irrelevant for SVG and passthrough files", implying it carried a real answer for every raster; it was a hard-coded False for four of the eight raster extensions. - test_an_undetermined_probe_skips_one_file_and_no_more used other.jpg as its control, which survived only because .jpg was unprobed. Now a .txt passthrough, which no widening of the probe can touch. New: test_every_animated_raster_keeps_its_frames_under_webp over animated WEBP, APNG and MPO-as-.jpg, plus a static counterpart so the exemption stays animation and not extension. The webp and MPO cases fail against the previous commit; the APNG case passes, which is the control showing what the old set did and did not cover. 214 + 33 + 189 pass; scripts/ unchanged from base. Real 299-image corpus byte-for-byte identical, 39.5s -> 40.0s for probing every raster rather than four extensions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
None of these is data loss; two are worth the change. - The comment replacing ANIMATABLE_EXTENSIONS' rationale was appended rather than substituted, so the sentence it replaced survived directly above it: "opening every JPEG to learn that would cost an extra pass over the whole corpus", immediately followed by "Probed for every raster". The stale half is the exact argument that narrowed the probe set and flattened every animated WEBP, sitting there ready to persuade the next reader. The block is now one accurate paragraph, including what the widening really costs (39.5s -> 40.0s, which the previous comment called "measurable at neither end") and that .bmp is probed despite not being animatable - the price of having no list to get wrong. - An animated raster is copied byte-for-byte: no resize, no re-encode. It was logged as "Optimized src -> dst" and counted in stats["raster"], so nothing anywhere said that the file ships at its original dimensions, past --max-width, into a database bundled into an offline app. optimize_raster now returns (path, copied_through) - the shape optimize_svg already uses for was_rasterized - and process_file counts it in stats["copied"] and logs "Copied unresized (animated)". - possible_output_names checked `animated is None` only after the non-webp early return, so an unprobeable raster claimed a name there while optimize_raster refuses it and writes nothing. That held the name against other sources and broke the "claims nothing exactly when it writes nothing" invariant the written-vs-claimed check reads as a guarantee. The check is now above the --webp branch. - test_every_animated_raster_keeps_its_frames now runs both --webp settings; every animation test ran only the webp path, which is not the one where the None case was mishandled. New: the two reporting tests. The copy-through one fails against the previous commit, printing the "Optimized" line it should not have. 219 + 33 + 189 pass; scripts/ unchanged from base. Real 299-image corpus byte-for-byte identical, and the raster/copied tallies are unchanged at 223/1 - it holds no animated non-GIF, which is why none of this was visible there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six self-review rounds since the last update — 44 findings, and a caveat about them
What follows is what my own The two that matteredAn animated WEBP was flattened to a single frame. Fixing your F26 (animated GIFs mispredicted as 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 ( An SVG with an uppercase extension aborted the whole run. The rest, by round
Full detail is in each commit message. What I'd take from thisThree 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. 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 State
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 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 ( 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. |
Summary
An end-to-end pipeline for loading Kotlin website content and
kotlin-stdlib/-reflect/-test JSON docs into
documentation.db, plus aGitHub Action (
Build Kotlin Docs) that runs it against a Drive-hosted copyof the database.
Scope
Split out of #21
into two ticket-scoped PRs:
md_to_json.py).This PR depends on ADFA-5039 merging first.
populate_db.py,build_nav.py, andfind_missing_assets.pyallimportmd_to_json.pydirectly, and that file isn't included here. Please review this PR's
content, but hold off merging until #23 lands -
README.mdin particularwill very likely conflict with #23's own new
README.mdat 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 intodocumentation.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- syncsDokka-generated kotlin-stdlib/-reflect/-test JSON into the database.
ProcessDocs/ProcessKotlinDocs/run_e2e_pipeline_test.sh- local e2e testof the whole pipeline against a scratch copy of the database.
.github/workflows/build-kotlin-docs.yaml- CI counterpart: pullsdocumentation.db(and Writerside'swebHelpImages.zip) from GoogleDrive, runs the same pipeline, uploads the result back.
Dokka-plugin-kdoc2json/scripts/kotlin/build-stdlib-json-docs.sh+build.gradle.ktstweak - builds the kdoc-to-json plugin and generatesthe stdlib JSON docs the sync script consumes.
CLAUDE.md- repo-level orientation doc (schema drift between this repo'stooling and the live production database, repo tour, decisions log).
Test plan
run_e2e_pipeline_test.shlocally against a scratch database.Build Kotlin Docswithdry_run: trueand confirm thesummary/blacklist-verification steps pass.
made during the split, aside from
README.mdbeing trimmed to drop themd_to_json.py-only section now covered by ADFA-5039).