Skip to content

ADFA-3305: Convert animated GIFs to VP9 WebM and pack documentation.db - #38

Open
davidschachterADFA wants to merge 35 commits into
mainfrom
fix/ADFA-3305
Open

davidschachterADFA wants to merge 35 commits into
mainfrom
fix/ADFA-3305

Conversation

@davidschachterADFA

@davidschachterADFA davidschachterADFA commented Oct 17, 2026 •

Copy link
Copy Markdown
Collaborator

ADFA-3305

Shrinks documentation.db in four ways:

  • Converts animated GIFs to VP9 WebM. A GIF that can't become WebM gets the smaller of an optimized GIF and an animated WebP.
  • Stores WebP and WebM uncompressed.
  • Rebuilds the file with 4 KB pages instead of 1 KB, and without reserved bytes.
  • Packs the Templates, Content and Tooltips rows to fill pages.

Measured on the 2026-10-13 database:

Measure Before After
Animated GIFs converted to WebM 36 of 37
Size of those animations 49.4 MB 5.2 MB (mean SSIM 0.973)
documentation.db 176.0 MB 120.1 MB
Release documentation.db.br (brotli -q 11) 135.3 MB 95.7 MB

Ships after appdevforall/CodeOnTheGo#2158 (Range support). Without it, 4 of the converted clips freeze mid-loop in Help.

Commits

One step per commit; reviewing by commit is easiest.

optimize_db_media.py

  • 7127206: --types, to optimize only some content types.
  • 1fe440f: keeps animated GIFs' sub-rectangle frames, and tries gifsicle -O3.
  • 863dbfd: animated WebP under --webp.
  • 2f46daa: VP9 WebM under --webm.
  • 22df310: keeps each frame's exact timing.
  • 57d9ca6: encodes with -fflags +bitexact, so a rerun writes the same bytes.

update_media_references.py

  • 1ebe4d2: rewrites an <img> that shows a converted clip as <video autoplay loop muted playsinline>, with an undo log.
  • c060334: ignores a new file's chunk continuations in the rename map.
  • 11f2d86: a described clip gets role=img (unquoted since 5661879) plus aria-label. Without the role, WebView 155 doesn't expose the label.

Other scripts

  • 4e4f29d: set_content_type_compression.py, to store WebP and WebM uncompressed.
  • 8a4e535: rebuild_database.py, which drops the 12 reserved bytes per page.
  • 9011e9d: rebuild_database.py --pack, which fills Content's leaf pages.
  • c0f1408: packs Tooltips too, with ids continuing after the old sequence.

Review fixes (see the review on this PR)

  • e745ec3: --pack Content relinks Bookshelf.contentID, which only triggers tie to Content.id. It refuses to pack a table whose trigger links its id somewhere uncovered.
  • 389368e, eb43fb7: tests that the fingerprint sees values, not just rows, and that packed ids continue past the largest id.
  • 70e311f: the indexed WebP palette samples frames across the whole GIF.
  • 279f221, f415d6e: refuse a rewrite when one filename was converted two ways, and a second output for a re-inserted source.
  • d8e9767: pins WebM encoding to 8 threads, so a 1-CPU machine writes the same bytes.
  • 2a86583, d07a511: cap a GIF's WebP loop count at 65,535, and test that a GIF that repeats a few times stays out of WebM.

4 KB pages

  • e73ab8f: rebuild_database.py --page-size. The source's 1 KB pages came from SQLite's old default. 4 KB pages make the packed database 1,090,560 bytes smaller (121,234,432 -> 120,143,872) and its .br 313,164 bytes smaller (96,007,701 -> 95,694,537).

Later review fixes

  • 12aa8f0 (F351): renames an unquoted value (src=spring.gif) when an <img> becomes a <video>.
  • R3840: the independent review of ADFA-3305: Convert animated GIFs to VP9 WebM and pack documentation.db #38 at 12aa8f0 and ADFA-6596: Convert the documentation videos to VP9 WebM from their originals #40 at 4d2f7b2, fixed in three commits:
    • 0a6affe: tests that --page-size accepts 512 and 65536. It rewords the help ("use 4096 for documentation.db") and drops the docstring's claim that 4 KB is Android's default.
    • 5661879: writes role=img and aria-hidden=true unquoted, so a tag carried in a JSON string stays valid JSON whatever its quoting.
    • 6a4cb1b: matches a filename after whitespace that follows = or ( (src= demo.gif, url( demo.gif )). Before, the page kept naming the deleted GIF and both scripts exited 0.
  • R38: the review of those three commits, fixed in cc09153:
    • update_media_references.py stops, writing nothing, if a converted filename is left after a quote and whitespace (src=" demo.gif", url(' demo.gif')). A browser loads those, but the rewrite leaves them because the same shape can be prose.
    • The <img> attribute parser reads a JSON string's \n, \t, \r and \f as whitespace, so a JSON-carried tag with a line break before alt still gets role=img and aria-label.
    • Tests for whitespace after a quote or /, and for the \r and \f escapes. The test runner passes monkeypatch, so every test file also passes as a plain script.

G10: pack Templates too (ADFA-3305's size analysis, item G10)

  • eef0eed: Content.templateId 0 means "no template", and 7,156 rows have it. The orphan check counted those as references to a missing template, so --pack Templates was refused on every documentation.db. UNDECLARED_REFERENCES now carries each column's "none" value; neither the orphan check nor the identity check counts it, and the copy keeps it. Any other id with no row is still refused, and so is a template whose own id is 0.
  • f7e8633: a table is now planned for the new ids of a table packed in the same run. Before, Content sized templateId on the old ids (0-7, stored in 0 or 1 bytes) while the copy wrote ids above 1,650 (2 bytes), so packing Templates with Content overflowed Content's pages: +11.5 MB on the 2026-10-19 item-B build. Each referencing column is now sized as its parent's largest new id, an upper bound like the rowid's, in whatever order --pack lists the tables.
  • Review fixes (R38-05..11, see the review on this PR):
    • aee470f: a table that holds a packed table's new ids but isn't packed itself now gets a warning when it grows. --pack Templates alone took the 2026-10-20 build's Content from 6,573 leaf pages to 8,787 (+8,953,856 bytes) and exited 0 with nothing said. The check compares with the table's stored rows filled in rowid order at the copy's page size, so it holds when the page size changes. WITHOUT ROWID tables can't be packed and aren't checked. The docstring and --pack help say to pack Templates together with Content, never alone or after it.
    • 1970ebe: tests for template ids that cross the 2-to-3-byte boundary, for Bookshelf.contentID having no "none" value, and for a negative templateId.
    • f25cda2: comments. CodeOnTheGo reads templateId in DocumentationContentSource.kt, not WebServer.kt.

Against today's --pack Content,TooltipTexts --page-size 4096, with --pack Templates,Content,TooltipTexts --page-size 4096:

Build File .br Templates leaf pages Content leaf pages
2026-10-20 (G02, #56) 84,803,584 -> 84,688,896 (-114,688) 66,297,007 -> 66,271,384 260 -> 227 6,573 -> 6,578
2026-10-19 item-B 85,708,800 -> 85,618,688 (-90,112) 67,288,387 -> 67,165,599 202 -> 175 6,671 -> 6,676
  • Where it lands: a phone-storage saving. Repacking alone moves the .br by about 0.1 MB, so the download change is within noise.
  • One run or two: on G02, one run in any of three --pack orders and two passes (Templates, then Content,TooltipTexts) write byte-identical files.
  • Identity: every run reported 22 tables and every row identical, with schema, sqlite_sequence and pragmas unchanged.
  • Today's command writes the same bytes as before this change, on G02 and item-B.
  • The warning: on G02, from its 4 KB file and from a 1 KB copy of it, neither command gives one. --pack Templates alone gives warning: Content is not in --pack, so its pages were not planned for the new ids in Content.templateId: 8,787 leaf pages, against 6,573 with the old ones; pack Content in the same run.
  • Before schema 3.0.0: --pack Templates,Content,Tooltips --page-size 4096 runs on the item-B build's state before its migration (6 templates, 26,354 rows on 0), with no warning. That file was already packed, so it gives the same 112,283,648 bytes as --pack Content,Tooltips.

Tests: 14 new, 36 in all:

  • templateId 0 is not an orphan, the identity check takes it as none, and packing Templates keeps the 0s and relinks the rest;
  • a non-zero id with no template, a negative one, and a template with id 0 are refused;
  • Bookshelf.contentID has no "none" value: a contentID of 0 is an orphan, and a reference left on Content row 0 fails the run;
  • Templates and Content pack in one run as well as in two, in either order, and with template ids that cross the 2-to-3-byte boundary;
  • the warning: its text at 1 KB and 4 KB pages; none when the table is packed or didn't grow; a WITHOUT ROWID table isn't checked; and the page count it compares with equals SQLite's on four fixtures, including tables of 8- and 9-byte cells that fill a page with no byte over.

Fail-without-fix: each of the 6 fixes reverted fails its test, for the reason it is named for: the orphan error on the 0 rows, the Content digest, 68 Content pages against 65, a run accepted instead of refused, and no warning.

Mutants: 26, all killed:

  • the 6 the first draft of this body counted;
  • both readings of the seventh, taking 0 as none for Bookshelf.contentID too, which that draft called equivalent and the review showed isn't;
  • the review's three wrong bounds (the parent's old largest id, its row count, the child's own largest id) and > 0 in the orphan check;
  • 14 of the warning, among them comparing with the source's own pages and a byte more or less of room on a page.

Other places the new command has to reach (on the stack branches; not changed here; they must land with or after this PR, since the old tool refuses --pack Templates):

Run order:

remove_skin_font_comment.py DB
bundle_remote_images.py DB
remove_remote_loads.py DB
check_remote_loads.py DB
set_content_type_compression.py DB image/webp none
set_content_type_compression.py DB video/webm none --create
cp DB migrated.db
optimize_db_media.py DB --webp --webm --types image/gif --save-originals bundle --manifest-out manifest.tsv
update_media_references.py DB --before migrated.db --save-originals bundle
set_content_type_compression.py DB image/x-icon brotli
set_content_type_compression.py DB application/x-font-type1 brotli
rebuild_database.py DB final.db --pack Templates,Content,Tooltips --page-size 4096

The first four steps come from #41 (ADFA-6589), in that order:

  • they remove Help+Manual's commented-out Google Fonts block;
  • they store the API diagrams pages loaded from the internet;
  • they remove other markup that loads files from the internet;
  • the last fails the run if any off-device load is left.

All four have to run before the migrated.db snapshot.
This needs ffmpeg with libvpx-vp9, gifsicle, and the brotli CLI.

The two image/x-icon and application/x-font-type1 steps are ADFA-6602: both types compress well and are always fetched whole. On the 2026-10-13 database their 12 rows go from 213,885 to 173,255 bytes, and the file is 40,960 bytes smaller. They come before the rebuild so packing happens once. A second packed rebuild would renumber every tooltip again.

Testing

Unit tests, run locally with Python 3.14:

Test file Result
test_animated_media.py 40 pass
test_rebuild_database.py 36 pass (22 before G10; Python 3.11 under pytest, 3.14 as a script)
test_set_content_type_compression.py 5 pass
test_media_bundle.py 24 pass, 3 fail
  • The 3 test_media_bundle.py failures also fail on main: they need scour, which isn't installed here. With a pass-through scour stub on PYTHONPATH, all 27 pass.
  • Mutation checks confirmed that the new tests fail without the change they cover.
  • The changed files compile under Python 3.11.
  • python-tests.yaml doesn't run the scripts/ tests, so none of these ran in CI.

Full run on the real database:

  • Integrity check passes, and every converted row decodes.
  • Frame timing is kept exactly, and every reference points to an existing file.
  • Rerunning the scripts changes nothing.
  • Restoring from the originals bundle gives back all 30,551 rows unchanged.
  • After packing, all 7 Bookshelf rows still join Content, as the same books. Integrity check passes and foreign_key_check is empty.

Phone test (Pixel 6 Pro, Android 17, WebView 155), with the Code On The Go Range fix:

  • All 12 clips on 6 pages looped.
  • Each clip's alt text reaches accessibility as its content description.
  • A tooltip, its button and its linked page work from the repacked Tooltips table.
  • This ran before 5661879. role=img gives the same attribute as role="img" (checked with Python's html.parser), but the phone test was not rerun.

Not yet tested:

  • TalkBack's spoken readout.
  • Playback on a low-end phone.

🤖 Generated with Claude Code

https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3

davidschachterADFA and others added 13 commits October 16, 2026 22:22
Without the manifest from the run that optimized a database, nothing tells
this tool's own lossy output apart from an original, so a full run re-encodes
every already-optimized WebP and PNG from lossy input. No manifest from the
ADFA-5552 run survives, so converting the animated GIFs needs a run limited to
image/gif. Rows of other types are left as they are, still archived when
--save-originals asks, and their manifest entries are carried over so the
manifest keeps describing the whole database.

pngquant is now required only when PNG is among the types being optimized.

test_media_bundle._main takes an optional namespace so the new
test_animated_media.py can reuse it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
…M uncompressed

image/webp is marked brotli in ContentTypes. WebP is already entropy-coded:
over the 2026-10-13 database Brotli saved 0.48% on the 921 WebP rows, left 654
of them no smaller, and every fetch paid for a full decompress.

A row's compression lives in ContentTypes, not in the row, so the flag and
every row of the type have to change together. The script does both in one
transaction, reading each row back through the server's chunking before the
commit to check it decodes to the same content, and updates an optimizer
manifest if given - otherwise the next optimizer run would see every re-stored
row as edited and re-encode it. --create adds a type the database lacks, which
video/webm needs before the first --webm run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
… -O3

Moved here from ADFA-6586. optimize_animated_gif() converted every frame to a
full-canvas RGBA image and saved with disposal=2, so every frame was stored
whole. On the 2026-10-13 database, kotlin-wasm-wasi-http.gif is 1,741,772
bytes that way against 429,792 with disposal=1; and a GIF already stored as
sub-rectangles grew up to 10x, so the never-larger rule discarded the result
and the GIF was never downscaled (new-copy-paste-UI.gif is still 800 px wide).

Now a GIF within --max-width only goes through gifsicle -O3, which is
lossless. One that needs resizing is saved by Pillow with disposal=1, so each
frame holds only what changed, and the smaller of that and gifsicle -O3 of it
is kept. disposal=2 stays for a GIF with real transparency: leave-in-place
cannot turn a pixel transparent again, so a moving shape would smear.

Also fixed in the same function: a GIF with no loop count plays once, but
loop=0 was passed for it, which makes it loop forever.

gifsicle is looked up, not required: a database with no animated GIFs runs
without it, and an animated GIF that finds it missing fails on its own row.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
Animated GIFs stayed GIFs even under --webp; they are 49.4 MB of the
2026-10-13 database. Under --webp an animated GIF now gets two more
candidates and the smallest of the three is kept:

  * lossy animated WebP (--webp-quality, method 4: 6 took several times as
    long for a few percent on these animations);
  * WebP quantized to --animated-colors colours (default 64; 0 skips it) and
    stored lossless. Lossless WebP stores a frame of 256 colours or fewer as
    palette indices, so GIFs' restricted palettes pay off. Over the 37 GIFs,
    64 colours was both smaller than lossy WebP and closer to the GIF
    (24.0 vs 28.0 MB). Skipped for real transparency, which would need an
    alpha-aware palette.

GIF counts loop repeats and WebP counts plays, so a GIF that plays once
becomes WebP loop 1 rather than 0, which would loop forever.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
optimize_db_media.py --webm (next commit) turns animated GIFs into VP9 WebM.
Renaming a reference is not enough for those: an <img> does not play video.
Each reference to a file converted to video is rewritten as a whole tag:

  <img alt="A spring" src="media/spring.gif" width="300"/>
  <video autoplay loop muted playsinline aria-label="A spring"
         src="media/spring.webm" width="300"></video>

autoplay, loop, muted and playsinline make it behave like the GIF; muted is
also what lets it start without a gesture. alt becomes aria-label so a screen
reader still describes it (an empty alt becomes aria-hidden), and every other
attribute is carried over with renamed files in it rewritten. Kotlin's
templated pages hold their markup inside JSON strings with every quote escaped,
which is undone to parse the tag and applied again to the result.

A reference to a video anywhere else - a CSS url(), a link, a JSON string, an
<img> with srcset - stops the run before anything is written.
names_not_convertible_to_video() runs the same check for the optimizer, so it
never makes a video this then has to refuse.

The undo log (references.tsv) goes to v2: a tag site records the old and new
tag in full, base64-encoded so the tags' own commas and colons cannot break
the format, which makes undoing a check-and-swap. A v1 log still reads.

The rename map and the lineage check now count video/webm as media.

On the 2026-10-13 database every one of the 38 references to the 37 animated
GIFs is an <img> except the PDF viewer's CSS loading-icon.gif.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
The 37 animated GIFs in the 2026-10-13 database are 49.4 MB; as VP9 WebM at
CRF 32 they are 5.1 MB, mean SSIM 0.973 against the GIFs (excluding the one
transparent file), against 22.2 MB for the best WebP. Muted autoplay, looping
and inline playback work in HelpActivity with no code change (Pixel 6 Pro,
Android 17, WebView 155).

--webm adds a VP9 candidate (yuv420p, profile 0, --webm-crf, default 32) to an
animated GIF's others, kept when smallest - but only when the swap cannot
change what the page shows:

  * no real transparency: yuv420p has no alpha;
  * looping forever: the <video> update_media_references.py writes loops;
  * referenced only from <img> tags it can rewrite, checked before converting
    with names_not_convertible_to_video() - today that rules out the PDF
    viewer's CSS loading-icon.gif.

Frames go to ffmpeg as PNGs through a concat list carrying each frame's
duration; listing the last frame twice and cutting the output at the total
duration with -t keeps the WebM exactly as long as the GIF (without the cut,
trial encodes ran 30-60 ms long).

A WebM is an output only: it is named .webm (OUTPUT_EXTENSIONS) but never
re-encoded, and video/webm rows join the main loop's query solely so their
manifest entries and provenance carry over to the next manifest. --webm needs
ffmpeg and a video/webm content type, and stops up front if either is
missing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
build_rename_map skipped chunk continuation rows on the removed side but not
on the added side. A converted file over CHUNK_SIZE is stored as "foo.webm"
and "foo.webm-1", which share the stem "foo", so the rename looked ambiguous
and was left out with only a note: the run exited 0 while the page still
named the deleted original.

No earlier conversion produced a file that large, so this never showed. The
first run of --webm over the 2026-10-13 database did: conversations-bubble.gif
became a 1.09 MB WebM and its <img> was left pointing at the GIF.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
encode_webm fed ffmpeg a concat list of PNGs with "duration" lines and cut
the output at the total with -t. Each PNG opened at the image demuxer's
25 fps, so every timestamp was rounded to a 40 ms grid, and -t then re-timed
the whole output to a constant 25 fps: over the 2026-10-13 database,
bounce-demo.gif's 5,260 ms came out 3,120 and its closing 2,190 ms pause was
lost. The tests missed it because every synthetic GIF used one delay
throughout - 40 or 50 ms, which a fixed rate happens to represent - and the
check compared only the total, which -t forced to match.

The frames now reach ffmpeg as one APNG, whose per-frame delays it reads to
the millisecond, last frame included, and the encoder's time base is pinned
to 1 ms: left to itself it follows a frame rate guessed from the delays, and
30/70/130 ms delays landed on a 1/30 s grid. A new test uses those delays and
a closing pause, and compares every frame's start time.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
…ry page

documentation.db reserves 12 bytes at the end of every 1 KB page (file header
byte 20). On the 2026-10-13 database they are zero on all 128,099 pages:
reserved space is for SQLite extensions - a per-page checksum, an encryption
nonce - and nothing here uses one. They arrived in one rebuild between
2026-05-05 and 2026-07-29, from no tool in this repository.

SQLite lets reserved space grow but never shrink: VACUUM, VACUUM INTO and the
reserve_bytes file control all kept the 12 bytes. rebuild_database.py copies
the database into a fresh file instead - same page size, auto_vacuum off
(set explicitly: the sqlite3 shell tested with defaults it on, adding a
pointer-map page per ~200 pages), every table filled in rowid order with its
rowids kept, then indexes, triggers, views, sqlite_sequence, user_version and
application_id - then VACUUMs. It keeps the copy only if schema, every row
with its rowid, sqlite_sequence, both pragmas, integrity_check and
foreign_key_check all match the source; otherwise it deletes it and fails.

On that database after this ticket's conversion: 131,173,376 -> 129,705,984
bytes (1.1%) in 1.6 s. A one-time step: nothing here sets reserved bytes and
later VACUUMs keep the zero. The reserved bytes are zeros, so the saving is in
installed size on the device more than in the release .br.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
…pages

SQLite appends rows in rowid order and starts a new leaf page when the next
row does not fit, so a table's leftover page space follows from which rows
happen to be numbered next to each other. Content's leaf pages held 1.59 rows
each with 253 bytes left over: 4.90 MB unused on the 2026-10-13 database.

--pack TABLE renumbers the named tables' rows in an order that packs their
leaf pages best-fit-decreasing, from each row's exact cell size (record
format plus the local-payload rule for oversized rows, the rowid sized as the
largest new one so no estimate is low). Inserting in that order gives exactly
the predicted page count: Content 19,158 -> 14,524 leaf pages, unused space
on them 4.90 MB -> 0.19 MB, the file 129,705,984 -> 124,896,256 bytes.

Declared foreign keys naming a packed table's rows are remapped, and a table
with orphaned references is refused, since an orphan's old id could match a
new one. The comparison maps every renumbered id back, so a packed copy must
still match the source exactly.

Only for tables nothing outside the database knows the ids of. Content is
looked up by path everywhere. Tooltips is left alone: the tooltip feedback
email and debug dialog show its ids. Packing has to be part of every
release's rebuild, since new rows append in arrival order.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
A table's rows are stored in id order, so the only way to choose which rows
share a page is to choose the order in which they get their ids. --pack now
does what a fresh build would: insert the rows in the packing order, each
taking the next id AUTOINCREMENT would hand it, and link every declared
foreign key to the ids handed out. TooltipButtons.tooltipId follows Tooltips.

The new ids continue after both the old sequence and the largest old id, as
AUTOINCREMENT's would, and sqlite_sequence moves on by the row count. An id
from before the rebuild (one quoted in a tooltip feedback email, say) can
stop naming a row but can never come to name a different one.

On the 2026-10-13 database after this ticket's conversion, with Content and
Tooltips packed: Tooltips 16,069 -> 12,402 leaf pages, Content 19,158 ->
14,524, the file 131,173,376 -> 121,236,480 bytes. Tooltips ids move from
1..47,663 to 47,664..93,791; every one of the 54,799 TooltipButtons rows still
belongs to the same tooltip by category and tag; integrity_check and
foreign_key_check pass.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
A <video> converted from an <img> with alt text carried the text as
aria-label alone. On the phone (Pixel 6 Pro, WebView 155) that reached
the accessibility tree as an empty node; with role="img" beside it, the
text was the node's content description. An empty alt still becomes
aria-hidden, with no role.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
ffmpeg's Matroska muxer writes a random 8-byte TrackUID (twice: the
track and its tags), so every rerun gave all 36 converted clips new
bytes, and a rebuilt documentation.db differed from the last one
everywhere a clip was. With bitexact the muxer leaves the random ID
and its version strings out; the decoded frames are unchanged.

The bytes match only for the same ffmpeg and libvpx builds; the files
no longer record which ffmpeg wrote them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
@davidschachterADFA

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

davidschachterADFA and others added 9 commits October 17, 2026 09:52
Bookshelf.contentID names a Content row with no FOREIGN KEY; only
Content's AddBook and DeleteBook triggers tie the two. --pack followed
declared keys alone, so on the real database all 7 Bookshelf rows kept
the old ids and joined no Content row, and the run still reported
success: the fingerprint mapped back only the columns it followed.

- UNDECLARED_REFERENCES lists such columns, and --pack relinks them
  like a declared key: Bookshelf.contentID -> Content, and
  Content.templateId -> Templates, the same gap for --pack Templates.
- --pack refuses a table whose trigger uses its id in a statement that
  writes to no reference the pack follows, naming the trigger. A regex
  reads the trigger SQL; its limits are in check_triggers' docstring.
- The fingerprint maps a followed column only through the new ids, so a
  reference left on an old id now fails the run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
Only a dropped row was tested, so a digest of the rowid alone, or one
built with str() so that 5 and '5' match, passed every test. Four tests
now spoil the copy after it is written and require the run to fail and
delete it: an altered value, values swapped between two rows, an
integer turned into text, and NULL turned into an empty blob. Each fails
against the mutant it is named for.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
Every fixture had sqlite_sequence at or above the largest id, so taking
the base from the sequence alone passed every test, yet with a lagging
sequence, or none (no AUTOINCREMENT), it hands old ids to different
rows. Two tests now pack such tables and require the ids to start after
the largest one; both fail with the base taken from the sequence alone.
The docstring now says the ids continue after whichever is higher.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
…le GIF

frames[::len // 16][:16] stepped by 1 for any animation of 17 to 31
frames and kept frames 0-15, so the shared palette never saw the second
half. A colour shown only late had no entry: a green banner in frames
16-30 of 31 came out dark grey. Four of the 37 animated GIFs in the
database have 17 to 31 frames.

palette_sample now takes up to 16 frames spread evenly from the first to
the last.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
The rename map is keyed by filename, because references are matched by
filename, but the optimizer picks each GIF's output on its own. Two
same-named GIFs could part ways - a/x/foo.gif to foo.webm, b/y/foo.gif
to foo.webp or left a GIF - and the first rename won: b's page became
<video src="y/foo.webm">, a file only a/x has. The second path was only
"noted", and both tools exited 0.

build_rename_map now records what happened at every path holding a
converted filename and raises AmbiguousRename unless it is the same
everywhere. run() names the paths, writes nothing and exits 1.
Same-named files converted alike still share one rename. The real
database has no such filename: its 36-entry map is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
A re-inserted source reclaimed its earlier output only when it came out
with the same extension, and a GIF's output now depends on its content.
Run 1 made demo.webm and its page a <video>; the GIF re-inserted with
transparency became demo.webp on run 2, leaving the stale demo.webm
still linked and demo.webp linked from nowhere. Both runs exited 0.

When --manifest-in says this path made an output that is still stored,
and this run would put the file anywhere else - another extension, or
its own path - the row is now an error and left as it is. The message
says to point the references back at the source, delete the old output,
and run again; following it ends with the page linking demo.webp.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
…same bytes

-fflags +bitexact made reruns match only on machines with the same CPU
count. Left unset, libvpx takes one thread per CPU ffmpeg may use, and
with one thread -row-mt does nothing: on one CPU bounce-demo.gif came
out 11,970 bytes instead of 12,039, and all eight of the largest
animations in the database differed.

With -threads pinned, each count tried from 2 to 64 gave the same bytes
under one CPU and under all 20, at 500 px and 1920 px, and the same
bytes as the WebMs already in converted.db. 8 was as fast as any. Tile
columns do not follow the thread count, so they stay at libvpx's
default.

The new test skips where sched_setaffinity is missing (macOS) or only
one CPU is available; the plain-script runner now reports such a skip
instead of a failure.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
webp_loop turns a GIF's repeat count into WebP's play count by adding
one, so GIF's largest, 65,535 repeats, became 65,536. WebP's count is
16 bits too, and libwebp refused the file ("ERROR assembling WebP");
the exception failed the whole row, so the GIF candidate was lost with
it. Capping at 65,535 plays loses one play of 65,536.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
…of WebM

The <video> written for a WebM always loops, so only a GIF that loops
forever may become one. Only "plays once" was tested, so replacing
loop == 0 with loop != 1 passed every test while turning a GIF that
repeats once, twice or 65,535 times into a <video> that never stops.

The new test fails against that mutant at the first finite count, and
checks that the same animation looping forever does become WebM, so it
cannot pass merely because the GIF candidate came out smallest.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3

@davidschachterADFA davidschachterADFA left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Independent review

Two Claude Code reviewers split this PR by file. CodeRabbit doesn't appear to be installed on this repo: it has no activity on #31 to #38, and @coderabbitai review got no reply.

ID Sev Finding Status
F158 critical --pack Content left Bookshelf.contentID on old ids: bookshelf empty Fixed in e745ec3
F159 medium Only a dropped row tested; a rowid-only fingerprint passed all tests Fixed in 389368e
F160 medium Indexed WebP palette sampled only frames 0-15 of a 17-31 frame GIF Fixed in 70e311f
F161 medium Same-named GIFs converted two ways: a page could name a missing file Fixed in 279f221 (refusal)
F162 medium A re-inserted GIF with a new output type left the old WebM in the page Fixed in f415d6e (refusal)
F163 low WebM bytes differed between 1 thread and 2 or more Fixed in d8e9767
F164 low GIF loop count 65535 became WebP 65536, which libwebp rejects Fixed in 2a86583
F165 low A mutant turning a GIF that repeats N times into a looping <video> passed Fixed in d07a511
F166 low Continuing packed ids past max(rowid) was untested Fixed in eb43fb7
F167 low Rewrite edge cases on markup the database doesn't contain (list below) Not fixed
F168 low Encoder edge cases none of today's GIFs have (list below) Not fixed
F169 low CI's python-tests.yaml runs none of the scripts/ tests Not fixed: needs its own change
F170 nit role="img" on <video> is likely not allowed by ARIA in HTML (unverified) Not fixed (see below)

F158 in detail.

  • Cause: Bookshelf.contentID has no declared foreign key. It's tied to Content.id only by the AddBook and DeleteBook triggers. The pack followed declared keys only, and the verification fingerprint had the same blind spot, so the run reported success.
  • Effect: on the packed 2026-10-13 database, all 7 Bookshelf rows existed but none joined Content.
  • Fix:
    • An explicit table of undeclared links that the pack relinks and the fingerprint checks. It also includes Content.templateId -> Templates.
    • A guard that refuses --pack T when a trigger on T links T's id into a table that isn't covered.
  • Result: the regenerated database keeps all 7 books.

F161 and F162 now stop the run (exit 1) and leave the rows as they are. Resolving references by full path would avoid F161 entirely. That isn't practical here, because pages refer to images by absolute URL, root-relative path and page-relative path.

F163 is caused by -row-mt, which only takes effect with 2 or more threads; tile columns don't depend on the thread count. Pinning 8 threads changes none of the 36 WebMs.

F167 and F168 aren't fixed because none of these cases appears in today's data (28,998 text rows scanned).

  • F167, rewrite edge cases:
    • a GIF named in an attribute other than src;
    • an unquoted SRC=x.gif;
    • guessing JSON escaping from the presence of \";
    • a GIF's own file name used as alt text, which makes a rerun refuse the page.
  • F168, encoder edge cases:
    • frame delays of 10 ms or less, which browsers show as 100 ms;
    • a partial first frame, which Pillow fills opaque where browsers show it transparent;
    • one GIF needing 3 GB of memory;
    • rows that fail to decode being skipped by the WebM reference check.

F170: with role="img", WebView 155 exposes the description as the node's content description, which it doesn't do without the role. TalkBack's spoken readout is still untested.

Unverified; worth a look before this runs elsewhere:

  • -fps_mode needs ffmpeg 5.1 or later, and the script only checks that ffmpeg exists. Ubuntu 22.04 ships 4.4.
  • The PyPI Pillow wheels may lack libimagequant, which would change the indexed-colour results.
  • The WebMs carry no colour-space tag.

Checked after the fixes

  • test_animated_media.py (34) and test_rebuild_database.py (19) pass.
  • Every new test fails against a mutant of the code it covers.
  • The pipeline rerun on the 2026-10-13 database:
    • The verifier passes, and rerunning the scripts changes nothing.
    • Restoring from the originals bundle gives back all 30,551 rows unchanged.
    • All 7 Bookshelf rows join the same books, the integrity check passes, and foreign_key_check is empty.
    • documentation.db is 121,234,432 bytes, and the WebM rows are identical to the previous run.

🤖 Generated with Claude Code

documentation.db has 1 KB pages, which nothing here chose: SQLite's
default was 1 KB until 3.12.0, and VACUUM keeps a database's page size.
Each page has an 8-byte header and each overflow page a 4-byte pointer,
so the megabyte-sized media rows cost less in fewer, larger pages.
Measured on the 2026-10-13 database after conversion, packed:

  page size  documentation.db  brotli -q 11 (.br)
  1 KB       121,234,432       96,007,701
  4 KB       120,143,872       95,694,537

Content, Tooltips and TooltipButtons read back identically. --pack now
plans pages for the copy's page size; a size SQLite would silently
ignore (not a power of two from 512 to 65536) is refused.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
davidschachterADFA added a commit that referenced this pull request Oct 18, 2026
build_rename_map paired each removed file with the new file of the same
directory and stem. That fails when the optimizer had to pick another
name (R6596-02). Beside a stored wide.webm, wide.mp4 became
wide-mp4.webm, found no pair, and its page kept naming the deleted
file, with only a note. With foo.gif and foo.mp4, both old names
paired with foo.webm, so the video's page would show the GIF.

update_media_references.py --manifest takes the optimizer's
--manifest-out and pairs each removed file with the file its
converted-from column says was made from it. Without --manifest the
stem pairing stays, but a new file claimed by two removed files pairs
with neither, and a removed file with no single replacement is now an
error that names it and writes nothing. Neither real run hit the old
note: replaying the pairing over #38's run and this PR's run found 36
and 94 renames and no unpaired file. The ambiguity checks are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
…video>

img_tag_to_video renamed filenames in each attribute value on its own,
and the anchored pattern needs a separator before a name. A quoted value
has its quote and a path has its "/", but an unquoted value that is only
a filename has nothing, so <img src=spring.gif> became
<video ... src=spring.gif>, naming the deleted GIF, and the run exited 0.

The value is now matched with the "=" it is written after. Tests cover
src and data-src0, an unquoted value at the end of the tag and before
another attribute, an unquoted non-video rename in a converted tag, the
JSON-escaped form, and the whole convert, rewrite and restore pipeline,
which gives back the unquoted original byte for byte. The 2026-10-13
database has no unquoted reference to a converted file.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
Review R3840-03, a sibling of F351. build_pattern's lookbehind needed
one of / " ' ( = right before a filename, but HTML allows whitespace
after an attribute's "=" and CSS allows it inside url( ). So
<img src= demo.gif> never matched: the optimizer replaced demo.gif with
demo.webm, the references step left the page naming demo.gif, and both
exited 0. A link written that way was missed by the optimizer's
pre-check too, so its GIF could become WebM.

A match may now start at whitespace (space, tab, CR, LF, FF, or \t \n
\f \r inside a JSON string) that follows "=" or "(". Both forms share
one lookbehind. Written as two alternatives, the pattern made a scan of
the 2026-10-13 database's 28,998 text rows 60% slower; this way it is
6.5% slower (21.99 s against 20.65 s, best of four, one thread).

Group 1 is still the name, and every caller now takes the name's
position from it: rewrite_text's plain renames, and img_tag_to_video's
rename inside a value, which keeps what the match holds before the
name. The enclosing_tag calls get group 1 too, for consistency;
whitespace contains no "<" or ">", so their result is the same.

Checked and left alone:
- A name after other whitespace is still prose ("save it as copy.png"),
  as test_rewrite_and_undo_are_exact_inverses pins.
- A quoted value with leading whitespace (src=" copy.png") still does
  not match: a closing quote, a space and a filename can be prose.
- drop_ogv_videos.py's build_pattern has the same lookbehind. It is on
  main, not part of this PR, and the 2026-10-13 database has no .ogv
  rows left for it to drop.

Tests:
- test_media_bundle: six references with whitespace after "=" or "("
  are renamed and undone exactly. With the fix reverted, or with only
  "=" accepted, only spaces, or no JSON escapes, it fails.
- test_animated_media: <img src= demo.gif> and <a href =\nlinked.gif>
  through the optimizer, the references step and a restore. With the
  fix reverted it fails because the linked GIF became WebM. With
  img_tag_to_video dropping what precedes the name, it fails on the
  page.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
davidschachterADFA added a commit that referenced this pull request Oct 18, 2026
Brings in #38's answers to review R3840: --page-size's limits and help,
role=img and aria-hidden=true written unquoted, and a filename matched
after whitespace that follows "=" or "(".

Three conflicts in update_media_references.py, each resolved by keeping
this branch's code and applying #38's change to it:
- img_tag_to_video's docstring: both paragraphs kept, and this branch's
  unescape_tag call.
- rewrite_text and names_not_convertible_to_video: this branch's video
  branches, with the name's position read from group 1 as #38 now does.

test_video_webm.py, which only this branch has, expects role=img in six
places where it expected role="img", to match #38's img_tag_to_video.

source_tag_to_webm still reads the match's position rather than group
1's; the next commit changes it, with a test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
davidschachterADFA added a commit that referenced this pull request Oct 18, 2026
Review R3840-03, #40's part. #38 now matches a filename after
whitespace that follows "=", and such a match starts at the
whitespace. source_tag_to_webm looked for the converted name only from
where the src value starts, past that whitespace, so it found none in
<source src= clip.mp4 type="video/mp4">: the src was renamed and the
type went on saying video/mp4. At 4d2f7b2 the name was not matched at
all, so the page kept naming the deleted clip and the run exited 0.

It now scans the whole tag and takes each name's position from group
1, for the src check and for the renames it writes.

The new test fails with this change reverted, the type left as
video/mp4.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
@davidschachterADFA

Copy link
Copy Markdown
Collaborator Author

R3840 answered

All nine findings are fixed. #38's share is in 0a6affe..6a4cb1b. It is merged into #40 in 4c82505, and #40's share is in 73a84d4..359699a.

ID Sev PR Status Commit
R3840-01 low #40 Fixed: a <source> hidden by a < or > in a quoted value is refused, and its video stays as stored c42c3e8
R3840-02 low #38 Fixed: role=img and aria-hidden=true are written unquoted, so a JSON-carried tag stays valid JSON 5661879
R3840-03 low both Fixed: a name after whitespace that follows = or ( is found, and a <source>'s type is set 6a4cb1b, 73a84d4
R3840-04 low #40 Fixed: a right-size, wrong-digest manifest entry, and a paired file over two chunks 0d06e27
R3840-05 nit #40 Fixed: the digest file keeps its line ends; a change made during the run is refused 310e1f7
R3840-06 nit both Fixed: tests for each surviving guard; the dead refusal removed 0a6affe, 359699a
R3840-07 nit #38 Fixed: "use 4096 for documentation.db"; the unverified Android-default claim dropped 0a6affe
R3840-08 nit #38 Fixed: two blank lines before each top-level def 0a6affe
R3840-09 nit both Fixed: both descriptions updated, including #41's four steps in the run order (R41-08)

Decisions to check:

  • R3840-01: the reviewer's "nearest <" check fails on title="<b". Instead, the last <source before the name is read with its quotes. The GIF path needed no change: an <img title="a > b"> was already refused.
  • R3840-02: the phone test was not rerun after this change. Python's html.parser reads role=img as the same attribute as role="img".
  • R3840-03: the whitespace form shares one lookbehind with the existing pattern. The scan is 6.5% slower (21.99 s against 20.65 s); a second alternative had made it 60% slower.

How it was checked:

Not fixed:

  • Docstring: its "1 KB until SQLite 3.12.0" claim couldn't be checked offline.
  • drop_ogv_videos.py has the same lookbehind. It is on main, and the database has no .ogv rows.

🤖 Generated with Claude Code

@davidschachterADFA

Copy link
Copy Markdown
Collaborator Author

Independent review of the fix commits

An independent Claude Code review covered 0a6affe, 5661879 and 6a4cb1b, which answer #38's share of the R3840 review (R3840-02, -03, -06, -07, -08, -09). It has 2 low and 2 nits. Each commit fixes what it names, and the real database is unaffected. One quoted form of the R3840-03 shape still leaves a page naming a deleted file with exit 0, and the attribute parser still misses the JSON-escaped whitespace that 6a4cb1b taught the pattern. All are open.

ID Sev File:line Finding
R38-01 low update_media_references.py:419 src=" demo.gif" and url(' demo.gif') still missed: file deleted, exit 0
R38-02 low update_media_references.py:220 JSON \n before alt: the <video> keeps alt, gets no label
R38-03 nit test_media_bundle.py:86 Two mutants of the new pattern survive, one of them the prose guard
R38-04 nit test_rebuild_database.py:264 As a plain script, the suite fails 1 test (needs pytest's monkeypatch)

R38-01.

  • Reproduced: at 6a4cb1b, a database holding a/media/demo.gif and a page, run through optimize_db_media.run with --webm and then update_media_references.run:
    • <img src=" demo.gif" alt="Demo">: demo.gif becomes video/webm, both runs return 0, and the page still says src=" demo.gif".
    • <div style="background:url(' demo.gif')">: the same. A CSS reference should have kept the GIF out of WebM, but the optimizer's pre-check (names_not_convertible_to_video) uses the same pattern and does not see it.
    • Control: <img src= demo.gif> becomes the <video>, as the commit intends.
  • Cause: the commit deliberately leaves out whitespace after a quote, because "a closing quote, a space and a filename can be prose". Browsers trim leading whitespace from a URL attribute and from a quoted url(), so those references load the file. The outcome is the one R3840-03 was about: a dangling reference and exit 0.
  • Real data: the 2026-10-13 database has none of these shapes (0 quote + whitespace + GIF or video name in 28,998 text rows).
  • Suggested fix: R3840-03's second option. After rewriting, fail the run if a removed filename still follows =, (, " or ' plus optional whitespace. A match in prose then stops the run instead of being rewritten silently, so the prose concern does not apply.

R38-02.

  • Reproduced: rewrite_text on json.dumps({"h": '<img src="spring.gif"\nalt="A spring">'}) gives <video autoplay loop muted playsinline src="spring.webm" \nalt="A spring"></video>: no role=img, no aria-label, and an alt that <video> ignores, so a screen reader gets nothing. The same tag as plain HTML gets role=img aria-label="A spring".
  • Cause: unescape_tag undoes only \", and _ATTRIBUTE_RE reads \nalt as the attribute's name, so the key == "alt" branch never runs (\nsrcset likewise escapes the srcset refusal). 6a4cb1b made build_pattern take \n, \t, \r and \f as the whitespace a JSON string writes; the attribute parser beside it was not changed. Predates these commits. ADFA-6596: Convert the documentation videos to VP9 WebM from their originals #40's <source> path has the same gap (R40-01).
  • Real data: 0 <img> or <source> tags in the 2026-10-13 database hold a JSON-escaped line break or tab.
  • Suggested fix: let _ATTRIBUTE_RE take \\[ntrf] as whitespace, as build_pattern does.

R38-03.

  • Reproduced: each mutant applied alone, all 63 tests in test_animated_media.py and test_media_bundle.py pass:
    • removing the inner (?<=[(=]), so whitespace is accepted after /, " and ' too. Then <p>"x" copy.png</p> becomes <p>"x" copy.webp</p>, which is the case the commit says is left alone on purpose;
    • shrinking \\[tnfr] to \\[tn].
  • Cause: test_whitespace_after_an_equals_sign_or_a_parenthesis_still_marks_a_reference has only positive cases, and only \n among the JSON escapes. test_rewrite_and_undo_are_exact_inverses pins prose after plain whitespace, not after a quote.
  • Suggested fix: add a negative case such as '<p>"x" copy.png</p>', which must stay unchanged, and a \r or \f case to the positive list.

R38-04.

  • Reproduced: python3 scripts/test_rebuild_database.py exits 1 with "1 failure(s)": test_pack_plans_pages_for_the_copys_page_size() missing 1 required positional argument: 'monkeypatch'. Under pytest all 22 pass. The file's docstring gives uv run scripts/test_rebuild_database.py as the way to run it.
  • Cause: test_media_bundle._main passes only tmp_path. The test came in with e73ab8f, before these commits; no review raised it. ADFA-6596: Convert the documentation videos to VP9 WebM from their originals #40's 310e1f7 adds a second such test (R40-03).
  • Suggested fix: patch with unittest.mock.patch.object inside the test, or teach _main to pass a minimal monkeypatch.

Earlier findings

ID Verdict Evidence
R3840-02 Fixed Quoting role again fails 7 tests, including the new JSON test; quoting aria-hidden again fails it and the empty-alt test.
R3840-03 Fixed for = and (; R38-01 left Reverting the pattern fails both new tests. Accepting only =, dropping the JSON escapes, using spaces only, dropping the kept prefix in img_tag_to_video, or editing from m.start() each fails at least one.
R3840-06 Fixed (#38's part) size <= 512 and size >= 65536 each fail test_page_size_refuses_what_sqlite_would_silently_ignore.
R3840-07 Fixed Help reads "use 4096 for documentation.db"; the "on Android too" claim is gone.
R3840-08 Fixed Every top-level def in the five changed files has two blank lines before it.
R3840-09 Fixed (#38's part) 12aa8f0 is listed; the test table matches this run.

Checked and found correct

  • Real database: all 28,998 text rows of the 2026-10-13 database, scanned with every GIF and video basename (152): the old and new pattern each find 2,103 matches in 153 rows, and the new one finds none the old one misses. The new scan took 4.3% longer (29.28 s against 28.06 s, one run, one thread), against the commit's 6.5%.
  • Positions: every caller now takes the name from group 1. The enclosing_tag calls give the same result either way, since the whitespace holds no < or >.
  • Unquoted attributes: nothing else in the repository reads role="img" or aria-hidden="true" from a converted page, and undo stores whole tags, so restore is unaffected. ADFA-6596: Convert the documentation videos to VP9 WebM from their originals #40's full real-data run (see its review) shows the only change on real pages is 29 tags on 18 pages losing those quotes.
  • Tests at 6a4cb1b (Python 3.14, with a pass-through scour stub for the 3 tests that need it):
Test file pytest plain script
test_animated_media.py 38 pass pass
test_rebuild_database.py 22 pass 1 fails (R38-04)
test_set_content_type_compression.py 5 pass pass
test_media_bundle.py 25 pass (22 + 3 fail without the stub) pass
test_drop_ogv_videos.py 15 pass pass

That is the response's 105.

Not verified:

  • Playback and accessibility of the unquoted role=img on a phone (the response says the phone test was not rerun either).
  • The docstring's "SQLite's default was 1 KB until 3.12.0" (no release notes offline).

🤖 Generated with Claude Code

- R38-01: update_media_references.run stops, writing nothing, when a
  converted filename is left after a quote and whitespace (src=" demo.gif",
  url(' demo.gif')). A browser loads those, but the rewrite leaves them
  because the same shape can be prose. New build_leftover_pattern.
- R38-02: the attribute parser takes \n, \t, \r and \f, as a JSON string
  writes them, as whitespace between attributes, around "=", after an
  unquoted value and at the end of the tag. A JSON-carried <img> with a line
  break before alt now gets role=img and aria-label, and one before srcset
  is refused.
- R38-03: tests for whitespace after a quote or "/" (left alone) and for the
  \r and \f escapes after "=".
- R38-04: test_media_bundle._main passes pytest's MonkeyPatch to a test that
  takes monkeypatch, so test_rebuild_database.py passes as a plain script.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
davidschachterADFA added a commit that referenced this pull request Oct 18, 2026
Brings in #38's answers to its second review (cc09153): a converted name
left after a quote and whitespace stops the references step, and the
attribute parser reads a JSON string's \n, \t, \r and \f as whitespace.

One conflict, in update_media_references.py's module docstring: both
paragraphs kept, this branch's on video-to-video renames and #38's on the
leftover check.

source_tag_to_webm still ends with its own trailing-junk check, which the
new parser leaves a JSON-escaped line break for; the next commit changes it,
with a test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
@davidschachterADFA

Copy link
Copy Markdown
Collaborator Author

Response to the review of the fix commits

ID Status What changed
R38-01 Fixed in cc09153 The references step stops, writing nothing, if a converted name follows a quote plus whitespace
R38-02 Fixed in cc09153 The attribute parser reads JSON \n \t \r \f as whitespace, also around = and after an unquoted value
R38-03 Fixed in cc09153 Tests for whitespace after a quote or / (left alone), and for the \r and \f escapes after =
R38-04 Fixed in cc09153 test_media_bundle._main passes pytest's MonkeyPatch; test_rebuild_database.py passes as a script

Decisions to check:

Checks:

  • Python 3.14, with a pass-through scour stub on PYTHONPATH for the 3 test_media_bundle.py tests that need it:
Test file pytest plain script
test_animated_media.py 40 pass 40 pass
test_rebuild_database.py 22 pass 22 pass (was 1 failure)
test_set_content_type_compression.py 5 pass 5 pass
test_media_bundle.py 27 pass (24 + 3 that fail without the stub) 27 pass
test_drop_ogv_videos.py 15 pass 15 pass
  • The three changed files compile under Python 3.11.16.
  • 16 mutants, each applied alone to the two media suites; each fails its test:
    • R38-02 revert, and each part of it (no JSON escapes, \\[nt] only, the name or the unquoted value takes the escape, plain \s around =, the old trailing check) -> test_json_escaped_whitespace_between_attributes_still_separates_them;
    • the review's two R38-03 mutants -> test_whitespace_after_a_quote_or_a_slash_does_not_mark_a_reference and test_whitespace_after_an_equals_sign_or_a_parenthesis_still_marks_a_reference;
    • R38-01 revert (leftovers not refused) -> test_whitespace_after_a_quote_stops_the_run_rather_than_leave_the_gif_named (exit 0, not 1);
    • leftover pattern without ', " or the JSON escapes -> that test and test_the_leftover_pattern_finds_a_name_after_a_quote_and_whitespace_only;
    • leftover whitespace optional, or no name boundary -> test_the_leftover_pattern_...;
    • leftover searched in the text before rewriting -> test_whitespace_after_the_equals_sign_still_names_the_gif (exit 1, not 0).
  • R38-04: with the old _main, python3 scripts/test_rebuild_database.py exits 1 on the monkeypatch TypeError; with the new one it exits 0.
  • Real data, a copy of the 2026-10-13 database:
    • all 2,774 <img> tags in 28,998 text rows convert to the same <video>, or get the same refusal, at 6a4cb1b and at cc09153;
    • the optimizer's pre-check blocks the same 3 GIFs with both versions (hmtoggle_arrow0.gif, hmtoggle_arrow1.gif, loading-icon.gif);
    • the leftover pattern, built from all 152 GIF and video names, matches nothing;
    • set_content_type_compression (2 steps), then optimize_db_media.py --webp --webm --types image/gif (36 of 37 GIFs become WebM), then update_media_references.py from each commit on its own copy. Both exit 0 and rewrite 37 references in 18 rows. All 30,551 rows are identical, and the bundle logs are identical apart from their written-at line. A rerun at cc09153 rewrites 0.
    • The ADFA-6589: Keep documentation content on the device #41 steps are not on this branch, so this run did not include them.

🤖 Generated with Claude Code

davidschachterADFA added a commit that referenced this pull request Oct 19, 2026
… as a range

The docstring placed the step before rebuild_database.py, which is only
in the open PR #38, and left out that it runs after the dictionary
re-mint. It also did not say that no workflow runs it: the
build-kotlin-docs workflows end with sync_kdoc_json_to_db.py, which
rewrites every k/ row. The docstring and CLAUDE.md's workflows
paragraph now say all three.

The .br savings were quoted to the byte although packing the same
database twice moves the .br by 173,277 bytes; they are now a range,
with the review's re-measure beside the original figures.

Review findings R47-03 and R47-05 on PR #47. The final "Next:" line was
changed with R47-02 in e2982e2.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
davidschachterADFA and others added 2 commits October 18, 2026 23:39
After migrate_tooltip_schema.py (#45) Tooltips is a WITHOUT ROWID table,
which --pack refuses; the usage example and --pack help named only
Content,Tooltips. Found in the #45 review (R45-07).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
test_rebuild_database.py imports test_media_bundle, which needs brotli,
Pillow and scour, but its script block listed only pytest, so
`uv run scripts/test_rebuild_database.py` failed with "No module named
'brotli'". It now declares what test_media_bundle declares.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
@davidschachterADFA

Copy link
Copy Markdown
Collaborator Author

Two small fixes (cc09153..c947a46):

🤖 Generated with Claude Code

https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3

davidschachterADFA and others added 2 commits October 20, 2026 13:18
Content.templateId 0 means the row is served as it is stored, and 7,156 rows
of the 2026-10-20 build have it. The orphan check counted those as references
to a missing template, so --pack Templates was refused on every
documentation.db; the identity check would then have mapped each 0 as an old
id the pack left behind.

UNDECLARED_REFERENCES now carries each column's "none" value: 0 for
Content.templateId, none for Bookshelf.contentID. Neither check counts it as
a reference, and the copy keeps it. Any other id with no row is still an
orphan. A template whose own id is 0 is refused, since 0 would then both mean
none and name it.

Packing Templates in the same run as Content still overflows Content's pages;
the next commit plans for that.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
packing_order sized every column by its value in the source. A column that
references a table renumbered in the same run gets a new id in the copy, so
packing Templates and Content together sized Content.templateId on the old
ids (0 to 7, stored in 0 or 1 bytes) while the copy wrote ids above 1,650 (2
bytes). Content's pages overflowed: +11.5 MB on the 2026-10-19 item-B build.

rebuild() now works out every packed table's new-id range first, then plans
each table with its references sized as the largest new id of their parent,
the "none" value and NULL excepted. That is an upper bound, as the rowid's
already was, so no estimate is ever low, whichever order --pack lists the
tables in.

So --pack Templates,Content,TooltipTexts --page-size 4096 is the ship-time
command now. Against today's --pack Content,TooltipTexts:

- 2026-10-20 build (G02): 84,803,584 -> 84,688,896 bytes (-114,688), .br
  66,297,007 -> 66,271,384. Templates 260 -> 227 leaf pages, Content 6,573 ->
  6,578. This is exactly the two-pass result (Templates first, then the rest).
- 2026-10-19 item-B build: 85,708,800 -> 85,618,688 (-90,112), .br
  67,288,387 -> 67,165,599.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
@davidschachterADFA

Copy link
Copy Markdown
Collaborator Author

New commits: pack Templates too (G10, head f7e8633)

From ADFA-3305's size analysis, item G10:

  • eef0eed: Content.templateId 0 now means "no template". The orphan check counted those rows as references to a missing template, so --pack Templates was refused on every documentation.db. A non-zero id with no template is still refused, and so is a template whose own id is 0.
  • f7e8633: a table packed in the same run as its parent is now planned for the parent's new ids, so --pack Templates,Content,TooltipTexts works in one run, in any order.

Measured against today's --pack Content,TooltipTexts --page-size 4096, on the 2026-10-20 build (G02, #56): the file is 114,688 bytes smaller (84,803,584 to 84,688,896). The .br change (-25,623) is within repacking noise, so this is a phone-storage saving. Today's command writes byte-identical output with the old and new tool.

Tests: 6 new; 28 pass under pytest and as a plain script.

An independent review follows. The PR body will be updated after its fixes.

🤖 Generated with Claude Code

https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3

@davidschachterADFA

Copy link
Copy Markdown
Collaborator Author

Independent Claude review of the G10 commits (eef0eed, f7e8633)

The code is correct. I found no defect in the copy, the orphan check, the identity check or the planner. On the G02 and item-B builds the new command gives the draft's file and .br sizes and reports all 22 tables identical. A separate SQL check against the source shows every page keeps its template, by name and bytes, or stays on 0. A one-run pack in any of three --pack orders gives a file byte-identical to the Templates-first two-pass. All 5 reverts fail the test named for them, for that reason. The findings are:

  • a newly reachable way to bloat the file (--pack Templates without Content);
  • test gaps that let three wrong-bound mutants survive, along with the "equivalent" seventh mutant, which is not equivalent;
  • a list of stack-branch edits that misses two lines;
  • doc nits.

None is a correctness bug in the shipped command.

ID Severity Where Issue Failure scenario
R38-05 Low rebuild, run --pack Templates alone runs; Content is not replanned G02 +8.95 MB, exit 0
R38-06 Low _templated test Ids never cross a size boundary; a wrong bound passes Bound bug ships unseen
R38-07 Low PR body, tests "Equivalent" 7th mutant is killable; Bookshelf unpinned 0-as-none spreads unseen
R38-08 Low PR body Edit list misses two CLAUDE.md pack lists, two notes Docs keep the old command
R38-09 Nit templateId comment Cites WebServer.kt, not DocumentationContentSource.kt Wrong file named
R38-10 Nit Module docstring "a byte larger": 3,237 rows grow 2 bytes; ragged reflow Wrong figure in the doc
R38-11 Nit PR body, two passes Says sizes were compared; the bytes are identical Undersells the check

R38-05: --pack Templates alone, or after Content, makes Content larger

Evidence, head tool on G02 (sha256 c7de1d8a), --page-size 4096 throughout:

  • --pack Templates: 84,803,584 -> 93,757,440 bytes (+8,953,856), exit 0. The summary prints only Templates renumbered for packing: 260 -> 227 leaf pages; followed by Content.templateId. Content's leaf pages went from 6,573 to 8,787, which shows only when that file's Content is packed next (8,787 -> 6,578).
  • Content first, then Templates (--pack Content,TooltipTexts, then --pack Templates on the result): 95,526,912 bytes, .br 66,401,445. The one run gives 84,688,896 and 66,271,384.

Before eef0eed the tool refused --pack Templates, so this outcome is new. A child table that is not in --pack is copied in rowid order, and each templated row's value grows by 1 or 2 bytes. Every full page of an already packed Content then spills.

Fix:

  • In run(), for each table that holds a followed column but is not in --pack, print its leaf pages before and after. Warn when they grow, for example: Content.templateId follows Templates' new ids, but Content is not in --pack, so its pages were not replanned (6,573 -> 8,787 leaf pages); pack Templates,Content in one run. Warning only on growth keeps Bookshelf (1 page) quiet.
  • Say in the docstring and the --pack help that Templates is packed together with Content, never alone or after it.

R38-06: no test pins the upper bound

Evidence: three mutants of the bound in rebuild() each pass all 28 tests:

  • bases[parent][0] (the parent's base);
  • bases[parent][1] (its row count);
  • base + count (the child's own largest id).

In _templated, base 150, count 150 and largest 300 are all 2-byte values. The real builds are the same: 1,650 and 3,300 on G02, 1,126 and 2,252 on item-B.

I wrote a fixture with 100 templates on ids 32,601-32,700 (sequence 32,700), with Content rows on template 32,601. The copy writes 32,800, which takes 3 bytes. Base 32,700, count 100 and Content's 1,600 each fit in fewer bytes. One-run Content leaf pages against two-pass: 68, 69 and 68 against 66 for the three mutants, and 66 against 66 for head. The same test also kills R4 (no widening), R5 (planning inline) and the bound-127 mutant.

Fix: add that fixture and test (test_the_bound_is_the_parents_largest_new_id). Templates' sequence grows by its row count with each packed rebuild (1,650 -> 3,300 now), so the ids cross 32,767 after about 18 more.

R38-07: the seventh mutant is not equivalent

The draft says: "The seventh (taking 0 as none for Bookshelf.contentID too) is equivalent: a 0 there is refused as an orphan before the identity check sees it." Both readings of that mutant are killable:

  • In UNDECLARED_REFERENCES (("Bookshelf", "contentID", "Content", 0)): the orphan check then skips 0, so the stated reason does not hold. A Bookshelf row on contentID 0 with no Content row 0 is accepted, copied as 0 and passes the identity check, and run() returns 0. Head refuses it (1 row(s) of Bookshelf.contentID name no Content row). Killed by test_a_bookshelf_contentid_of_zero_is_still_an_orphan.
  • In follow() only (0 as none for every column): Content may hold a row with id 0, since only a Templates row 0 is refused. A Bookshelf reference left on old id 0 by a spoiled copy then maps back to a match, and the run keeps that copy. Head fails it on table Bookshelf. Killed by test_a_bookshelf_row_left_on_old_id_zero_fails_the_run.

One more survivor: an orphan check of "templateId" > 0 in place of != 0. A negative templateId is then caught by the identity check (differs in: table Content) instead of the orphan message. Head behaves correctly here, but no test covers it.

Fix: add both tests (in the same file) and, optionally, an orphan test for a negative templateId. Then change the PR text to say all 7 mutants are killed.

R38-08: the list of places to change is incomplete

I grepped all 20 ODT worktrees for --pack, Content,Tooltip and Templates,Content. Two hits are instructions, and the draft's list does not have them:

  • fix/ADFA-3305-tooltip-schema, CLAUDE.md:198: "and pack Content,TooltipTexts afterwards, not Tooltips".
  • fix/ADFA-3305-compress-templates, CLAUDE.md:216: "re-mint afterwards, then pack Content,TooltipTexts, not Tooltips".

The "stay" list misses two measurement notes:

  • fix/ADFA-3305-trim-kotlin-json, scripts/trim_kotlin_json.py:52 (--pack Content,Tooltips).
  • fix/ADFA-6629-optimize-pdfs, scripts/optimize_pdfs.py:64.

fix/ADFA-6596 carries an old rebuild_database.py through a merge of fix/ADFA-3305 and does not edit it, so its next merge brings G10 along.

Fix: add the two CLAUDE.md lines to the must-change list, and the two scripts to the stay list.

R38-09: the comment names the wrong app file

The comment reads "WebServer.kt renders a row through the template it names". In CodeOnTheGo (local origin/stage and fix/ADFA-6589), the read is at common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt:461: if (templateId > 0) render(...) else decoded. WebServer.kt has no templateId. G10 rewrote this line and kept the old name.

Fix: name DocumentationContentSource.kt. The app treats every value of 0 or below as no template. The tool accepts only 0, which is stricter and fine, since neither build has a negative id.

R38-10: "a byte larger" and ragged lines

The module docstring says "Content 6,573 -> 6,578, its templateId values a byte larger". On G02:

  • 3,237 rows on templateId 1 (stored in 0 bytes) grow by 2 bytes;
  • 13,453 rows on 2, 3, 5 and 7 grow by 1.

packing_order's docstring already says "a byte or two". The edit also left three ragged lines:

  • "Rows are compared through a" (fingerprint);
  • "together in one run. Ids continue after the old sequence or the" (module docstring);
  • "and 90,112 off the 2026-10-19 one. A" (module docstring).

Fix: write "a byte or two larger", and reflow the three paragraphs.

R38-11: the two-pass result is byte-identical

On G02, these four outputs all have sha256 99311133b73b5a9...:

  • one run in the order Templates,Content,TooltipTexts;
  • one run in the order Content,TooltipTexts,Templates;
  • one run in the order Content,Templates,TooltipTexts;
  • the two-pass, Templates first and then Content,TooltipTexts.

The second pass sizes templateId on the same new ids that the one run bounds it at, and both continue from the same sequences.

Fix: replace "The sizes were compared, not the bytes" with "the files are byte-identical (G02)".

Checked and sound

  • Tests: all 28 pass under pytest, and as a plain script (28 ok, exit 0). The 6 new tests test what they are named for.

  • Reverts: each fails its test for the reason the draft gives.

    Revert What fails
    Orphan check without the 0 skip Orphan error on the 0 rows (5 tests)
    follow() without the none value The Content digest differs (3 tests)
    No rowid-0 refusal The run returns 0, not 1
    No widening 68 Content pages against 65
    Planned inline, parents seen so far 68 against 65 in the Content,Templates order
  • Mutants killed by the PR's own tests:

    • bound 127 (68 against 65);
    • widening the 0 too (66 against 65);
    • widening only for parents listed first (68 against 65).

    Widening NULLs too is equivalent: templateId is NOT NULL, and that mutant can only overestimate.

  • G02, --pack Templates,Content,TooltipTexts --page-size 4096:

    • 84,803,584 -> 84,688,896 bytes;
    • 22 tables and 193,780 rows identical;
    • Templates 260 -> 227 leaf pages, Content 6,573 -> 6,578, TooltipTexts 2,596;
    • .br 66,271,384, against 66,297,007 for today's command.

    All three orders give the same result.

  • Item-B:

    • 85,708,800 -> 85,618,688 bytes;
    • Templates 202 -> 175 leaf pages, Content 6,671 -> 6,676;
    • .br 67,165,599, against 67,288,387.

    This matches the draft and the docstring's 114,688 and 90,112.

  • .br baselines: the draft's "before" figures are today's command re-run on each final.db (G02 sha 9d659e76, item-B 919d3190). The shipped final.db files compress to 66,204,977 (G02) and 67,133,725 (item-B). Against them the new .br is 66,407 and 31,874 bytes larger. That fits the draft's "within noise".

  • eef0eed alone on item-B: 97,177,600 bytes (+11,468,800); Content 6,671 -> 9,491 leaf pages. This reproduces "+11.5 MB".

  • Old tool on G02: c947a46 refuses --pack Templates,... (7156 row(s) of Content.templateId name no Templates row).

  • Today's command on item-B: the old and new tools write the same bytes (sha 919d3190). This adds a second database to the coordinator's G02 check.

  • G02 output, by SQL against the source:

    • no orphans, and 7,156 rows still on 0;
    • every Content row joins a template with the same name and bytes;
    • Templates ids run 1,651-3,300, and its sequence 1,650 -> 3,300; Content's sequence 124,601 -> 148,447;
    • the 7 Bookshelf rows join the same paths;
    • foreign_key_check is empty and integrity_check is ok.
  • Planner prediction equals the result on G02: Templates 227, Content 6,578, TooltipTexts 2,596 leaf pages (widen {templateId: (3300, 0)}).

  • The none value agrees in all four places: the orphan query, the rowid-0 refusal, the planner and the identity check.

    • New ids start at base + 1, which is at least 1, so a 0 in the copy can only come from a source 0.
    • The source is digested raw, so the none rule cannot hide a copy that writes 0 for a real reference.
  • Unchanged behaviour: Bookshelf.contentID runs the same SQL and follow as before. sqlite_sequence advances by the row count of each packed table only.

  • Edge cases on both builds: neither build has a template with id 0, a NULL templateId (the column is NOT NULL) or a non-zero templateId with no template.

  • App side: CodeOnTheGo looks up includes by name (DatabaseTemplateLoader) and uses templateId only for the page's own template. Its id-to-name cache is cleared on every database switch (switchToDatabase -> clearTemplateCache), so renumbered ids are safe.

  • Restores:

    • optimize_db_media.py --restore-originals writes back the recorded templateId. On G02 only text/html rows are templated (16,690), and every media row is on 0.
    • update_media_references.py --restore-originals keeps each row's current templateId.

    A bundle saved before the rebuild therefore restores correctly after Templates is packed.

  • Docs and code: the docstring and the --pack help give the right table names for each schema. The error messages are accurate after the change. Both files parse with Python 3.9's grammar and on 3.11.

  • Sources unchanged: G02 is still c7de1d8a... and item-B 4c2005de....

Not checked:

🤖 Generated with Claude Code

https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3

davidschachterADFA and others added 3 commits October 20, 2026 13:44
Answers R38-06 and R38-07 of the review of the G10 commits. Tests only.

- R38-06: in every fixture and in both real builds, the parent's old
  largest id, its row count and the child's own largest new id all took as
  many bytes as the right bound, so planning on any of them passed every
  test. A fixture with templates on ids 32601-32700 packs them across the
  2-to-3-byte boundary; each of the three wrong bounds gives 68, 69 or 68
  Content leaf pages against 66.
- R38-07: the "equivalent" mutant was not. Two tests now pin that
  Bookshelf.contentID has no "none" value: a contentID of 0 is an orphan,
  and a reference left on Content row 0 fails the run. A third pins that a
  negative templateId is reported as an orphan, not left to the identity
  check.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
…s it holds

Answers R38-05 of the review of the G10 commits. Before eef0eed the tool
refused --pack Templates; now --pack Templates alone runs, exits 0 and takes
the 2026-10-20 build's Content from 6,573 leaf pages to 8,787 (+8,953,856
bytes), because Content is copied in rowid order with templateId values a
byte or two wider than before.

- unplanned_growth() checks each table that holds a packed table's new ids
  but is not packed itself, and run() warns when it fills more leaf pages
  than its stored rows would: "Content is not in --pack, so its pages were
  not planned for the new ids in Content.templateId: 8,787 leaf pages,
  against 6,573 with the old ones; pack Content in the same run".
- The comparison is against rowid_order_pages(), the same cell sizes the
  planner uses, filled in rowid order at the copy's page size. The source's
  own leaf pages would not do: the copy's page size can differ.
- WITHOUT ROWID tables (Tooltips and TooltipButtons in schema 3.0.0) are not
  checked: they cannot be packed, so no warning could help them.
- The docstring and --pack help say to pack Templates together with
  Content, never alone or after it. The rewritten paragraph also gives
  "a byte or two larger" (R38-10).

On the G02 build, from its 4 KB file and from a 1 KB copy of it: today's
command and the new one give no warning, and the same bytes as before this
commit (sha256 9d659e76 and 99311133); --pack Templates alone warns with the
figures above.

Tests: rowid_order_pages equals the copy's leaf pages on four fixtures at
1 KB and 4 KB, including tables of 8- and 9-byte cells that fill a page with
no byte over; the warning's text at both page sizes; no warning when the
table is packed or did not grow; a WITHOUT ROWID follower is not checked.
14 mutants, all killed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
…g line

Answers R38-09 and the rest of R38-10 of the review of the G10 commits.
Comments only.

- R38-09: CodeOnTheGo reads Content.templateId in
  DocumentationContentSource.kt (`if (templateId > 0) render(...)`), not
  WebServer.kt. The comment now says so, and that the app takes any value
  below 1 as none while this tool takes only 0.
- R38-10: fingerprint()'s docstring had a sentence broken over a short line.
  The module docstring's two ragged lines and "a byte larger" were fixed in
  the paragraph rewritten for R38-05.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
@davidschachterADFA

Copy link
Copy Markdown
Collaborator Author

Response to the review of the G10 commits (head f7e8633, now f25cda2)

All seven findings are fixed. CI passed at f25cda2.

ID Fixed in What changed
R38-05 aee470f A table that holds new ids but isn't packed is warned about when it grows
R38-06 1970ebe A test with template ids across the 2-to-3-byte boundary
R38-07 1970ebe Tests that Bookshelf.contentID has no "none" value, and for a negative templateId
R38-08 PR body The two CLAUDE.md lines and the two measurement notes are listed
R38-09 f25cda2 The comment names DocumentationContentSource.kt
R38-10 aee470f, f25cda2 "a byte or two larger"; the three ragged lines rejoined
R38-11 PR body One run and two passes are byte-identical (sha256 99311133... on G02)

R38-05: I took the warning, not a refusal: the copy is correct, only larger, and the message says how to do better.

  • The check: unplanned_growth() takes each table that holds a packed table's new ids but isn't in --pack. It warns when the copy's leaf pages exceed rowid_order_pages(): the table's stored rows, sized as the planner sizes them, filled in rowid order at the copy's page size. The source's own pages, as the review suggested, would not do: the copy's page size can differ from the source's (the 2026-10-13 database has 1 KB pages, the copy 4 KB).
  • Not checked: WITHOUT ROWID tables (Tooltips and TooltipButtons in schema 3.0.0). They can't be packed, so no warning could help them, and they have no rowid order to plan by.
  • On G02: --pack Templates alone, from the 4 KB file and from a 1 KB copy, prints warning: Content is not in --pack, so its pages were not planned for the new ids in Content.templateId: 8,787 leaf pages, against 6,573 with the old ones; pack Content in the same run.
  • No false warnings:
    • today's command and the new one give none on G02, from either page size;
    • on the item-B build's state before the 3.0.0 migration, --pack Templates,Content,Tooltips gives none either, though there TooltipButtons is a rowid table that follows Tooltips.
  • Same bytes: today's command writes what it did before G10 (G02 9d659e76, item-B 919d3190), and the new one what it did at f7e8633 (99311133).
  • Tests:
    • the prediction equals SQLite's leaf pages on four fixtures at 1 KB and 4 KB, including tables of 8- and 9-byte cells that fill a page with no byte over;
    • the warning's text at both page sizes;
    • no warning when the table is packed, even where packing again costs it a page, or when it didn't grow;
    • a WITHOUT ROWID follower isn't checked.
  • Mutants: 14, all killed: no warning, >=, checking WITHOUT ROWID or packed tables, comparing with the source's own pages or at the source's page size, a starting count of 0, a rowid sized as 1, and a byte or more of room too many or too few on the first or later pages, or at an exact fit. The three fill-arithmetic mutants survived the first version of the tests; the 8- and 9-byte cell tables kill them.

R38-06 and R38-07: each new test fails, for its reason, against the mutant the review named:

  • the three wrong bounds give 68, 69 and 68 Content leaf pages against 66;
  • 0 as none for Bookshelf accepts the orphan;
  • follow() taking 0 as none everywhere keeps the spoiled copy;
  • > 0 in the orphan check gives the identity check's "differs in" instead of the orphan message.

R38-09: the comment also says that the app takes any value below 1 as none while the tool takes only 0, and that a negative id is refused as an orphan.

The review's "Not checked":

Tests: 36 pass under pytest (Python 3.11) and as a plain script (3.14).

🤖 Generated with Claude Code

https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants