You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Commit 46235fb
Browse filesBrowse the repository at this point in the historyBrowse files
Fix the 14 findings from Hal's second PR #24 review
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>
0 commit comments