kreeader/kreeader-server
Watch
3
0
Fork
You've already forked kreeader-server
 1
Code Issues 23 Pull requests Projects Releases Packages Wiki Activity Actions

Review findings 2026-08-28: batch pages, recompress, attribution, ratchet flags #147

New issue
Closed
opened 2026-08-28 04:03:51 -05:00 by kreeader-user · 4 comments
kreeader-user commented 2026-08-28 04:03:51 -05:00
Owner

Interim adversarial review of tonight's four merges (#141 recompress, #142 batch pages, #143 attribution, ratchet publisher flags), standing in for the Sol review blocked on codex reauth. Full findings verified by running code. Status ledger:

Fixed on master (cf25c5c): attribution-correction pane_editor gate bypass (was deployed); bubble-backfill geometry wipe on undecodable pages (was deployed).

Fix wave dispatched (HIGH):

  • recompress: duplicate zip entry names collapse the plan map (archive/recompress.go:390) — second entry's bytes written for both, original destroyed; also X.png->X.webp colliding with existing X.webp. Must land before recompress.enabled is ever flipped. Fix: key plan by file index, refuse books with duplicate wantNames.
  • agent batch fetch: Client.Timeout spans body reads, so detection slower than 5min fails the batch, discards completed work, re-detects per-page (agent/agent.go:92, batchpages.go:110). Fix: per-request context without total deadline + small read-ahead buffer.

Fix wave dispatched (MEDIUM): orderPreserved wrong invariant (over-refuses); ScannerBusy checked once per hours-long run; survey estimator (mean-of-ratios, and pngBytes includes never-convertible 16-bit pages); unmeasurable-pages caveat threshold never fires; RecentRefusals unfiltered; bubble Depth = deepest-not-tip defect; tail tip nondeterministic via map iteration; rung confidences not on comparable scale (+ pane vs page diagonal); ReasonNoHit dead code (miss rate tautological); human corrections stored rung=tail (needs RungHuman); same-pane figures can share a cluster.

LOW (same wave where cheap): marker header pre-auth documented trade-off (write it down); AppendAttributionsWithDB returns zero id/created_at; batch partial-stream degradation claim unreachable; historicalComicIDTables keyed by raw strings; clamp01 NaN passthrough + zero-direction tail/zero-area figure scoring; cmd/migrate-identity childTables missing 4 tables (derive from db.ComicIDTables).

Ratchet (separate wave, kreeader-ratchet): ship the revert script the commit claimed; flags-worked orphan tags (positivehooks deletes flag, not tag); --per-publisher ignored under --stratify-publisher; NULL detect_score crash; non-Latin publisher_key collapse to empty; README/selftest coverage. NEEDS ANDREW: keep or revert the 264 production flags — every model adoption now re-detects 103 newly-flagged books (panetrain/auto.go:593), a recurring GPU cost nobody signed off on; also handleListFlags has no Limit and the queue went 68->332.

Verified CLEAN (evidence, not silence): batch API traversal/injection/allocation surface; identity migration covers all 30 comic_id tables (schema-parser enumeration); DB portability (no boolean=integer recurrence); recompress journal/replay/swap machinery aside from the collisions; bubble reshape fix correct (28/28 defects vs 1/28); geometry conventions; TranscriptVersion 2 migration; ratchet SQL replayed clean on a throwaway PG 18.6.

Sol re-review still queued behind codex reauth.

Interim adversarial review of tonight's four merges (#141 recompress, #142 batch pages, #143 attribution, ratchet publisher flags), standing in for the Sol review blocked on codex reauth. Full findings verified by running code. Status ledger: **Fixed on master (cf25c5c):** attribution-correction pane_editor gate bypass (was deployed); bubble-backfill geometry wipe on undecodable pages (was deployed). **Fix wave dispatched (HIGH):** - recompress: duplicate zip entry names collapse the plan map (archive/recompress.go:390) — second entry's bytes written for both, original destroyed; also X.png->X.webp colliding with existing X.webp. Must land before recompress.enabled is ever flipped. Fix: key plan by file index, refuse books with duplicate wantNames. - agent batch fetch: Client.Timeout spans body reads, so detection slower than 5min fails the batch, discards completed work, re-detects per-page (agent/agent.go:92, batchpages.go:110). Fix: per-request context without total deadline + small read-ahead buffer. **Fix wave dispatched (MEDIUM):** orderPreserved wrong invariant (over-refuses); ScannerBusy checked once per hours-long run; survey estimator (mean-of-ratios, and pngBytes includes never-convertible 16-bit pages); unmeasurable-pages caveat threshold never fires; RecentRefusals unfiltered; bubble Depth = deepest-not-tip defect; tail tip nondeterministic via map iteration; rung confidences not on comparable scale (+ pane vs page diagonal); ReasonNoHit dead code (miss rate tautological); human corrections stored rung=tail (needs RungHuman); same-pane figures can share a cluster. **LOW (same wave where cheap):** marker header pre-auth documented trade-off (write it down); AppendAttributionsWithDB returns zero id/created_at; batch partial-stream degradation claim unreachable; historicalComicIDTables keyed by raw strings; clamp01 NaN passthrough + zero-direction tail/zero-area figure scoring; cmd/migrate-identity childTables missing 4 tables (derive from db.ComicIDTables). **Ratchet (separate wave, kreeader-ratchet):** ship the revert script the commit claimed; flags-worked orphan tags (positivehooks deletes flag, not tag); --per-publisher ignored under --stratify-publisher; NULL detect_score crash; non-Latin publisher_key collapse to empty; README/selftest coverage. NEEDS ANDREW: keep or revert the 264 production flags — every model adoption now re-detects 103 newly-flagged books (panetrain/auto.go:593), a recurring GPU cost nobody signed off on; also handleListFlags has no Limit and the queue went 68->332. **Verified CLEAN (evidence, not silence):** batch API traversal/injection/allocation surface; identity migration covers all 30 comic_id tables (schema-parser enumeration); DB portability (no boolean=integer recurrence); recompress journal/replay/swap machinery aside from the collisions; bubble reshape fix correct (28/28 defects vs 1/28); geometry conventions; TranscriptVersion 2 migration; ratchet SQL replayed clean on a throwaway PG 18.6. Sol re-review still queued behind codex reauth.
kreeader-user commented 2026-08-28 04:05:32 -05:00
Author
Owner

Fix wave started on branch fix/review-20260828 (off master cf25c5c). Working the HIGH items first (recompress duplicate-name plan collapse + wantNames refusal; batch-fetch total-deadline + read-ahead buffer), then the MEDIUM/LOW list in issue order. Each fix gets its own commit with a test that fails before and passes after. Will not close this issue or merge.

Fix wave started on branch `fix/review-20260828` (off master cf25c5c). Working the HIGH items first (recompress duplicate-name plan collapse + wantNames refusal; batch-fetch total-deadline + read-ahead buffer), then the MEDIUM/LOW list in issue order. Each fix gets its own commit with a test that fails before and passes after. Will not close this issue or merge.
kreeader-user commented 2026-08-28 04:42:57 -05:00
Author
Owner

Mid-progress on fix/review-20260828 (HEAD bb81555). HIGH 1-3 and MEDIUM 4-13 landed, one commit each, every one with a test that fails on master and passes on the branch. Notable reproductions captured in the commits: the duplicate-name recompress plan built a verified part serving page B for page A; the survey estimator projected 784,685 bytes saved where the measured truth on the same sample was 2,970; the tail-tip map walk flipped the anchor on run 3 of 400. Findings 6 and 7 share one commit (same function, same overclaim); everything else is one-to-one. Now working LOW 14-20.

Mid-progress on `fix/review-20260828` (HEAD bb81555). HIGH 1-3 and MEDIUM 4-13 landed, one commit each, every one with a test that fails on master and passes on the branch. Notable reproductions captured in the commits: the duplicate-name recompress plan built a *verified* part serving page B for page A; the survey estimator projected 784,685 bytes saved where the measured truth on the same sample was 2,970; the tail-tip map walk flipped the anchor on run 3 of 400. Findings 6 and 7 share one commit (same function, same overclaim); everything else is one-to-one. Now working LOW 14-20.
kreeader-user commented 2026-08-28 05:02:25 -05:00
Author
Owner

Fix wave complete — branch fix/review-20260828, head 5f6e517

All 20 findings addressed. 18 commits, one per logical fix (6+7 share one — same function, same overclaim). Every fix carries a test that fails on master and passes on the branch; where the fix introduced a new symbol the pre-fix failure is a compile error, so I additionally reverted the behaviour in place and captured the real numbers, quoted below.

# Finding Status Commit
1 recompress plan keyed on f.Name, duplicate entries collapse fixed 7017a1f
2 webpName collides with an existing X.webp fixed 7017a1f
3 batch fetch on a total-deadline client + unbuffered jobs fixed e3fd516
4 orderPreserved compares sorted vs physical order fixed 0d16fdc
5 ScannerBusy checked once per multi-hour run fixed 8dcb81f
6 survey estimator: mean-of-ratios, 16-bit bytes in the base fixed 2550e49
7 unmeasurable-pages caveat never fires fixed 2550e49
8 RecentRefusals buries pixel mismatches fixed 160abcf
9 bubble tail Depth = global max; map-order tip choice fixed b12cd99
10 rung confidence scales overlap; pane vs page diagonal fixed 1c690f9
11 ReasonNoHit dead code fixed 08a9fbb
12 corrections stored Rung="tail" fixed f278130
13 two figures in one pane can share a cluster fixed bb81555
14 pre-auth marker fingerprinting trade-off undocumented fixed 47ca3a3
15 AppendAttributionsWithDB slice-copy: id/created_at 0 fixed e1f740a
16 partial-stream degradation claim was false fixed (made real) 61881a0
17 historicalComicIDTables raw strings fixed 78435f1
18 clamp01 NaN, zero-direction tails, zero-area figures fixed 9249ba1
19 migrate-identity childTables hand list fixed 5d1995c
20 docs/speaker-attribution.md gate claim + new semantics fixed 5f6e517

Nothing deferred.

Reproductions worth recording

  • #1/#2 — on master a CBZ with two members named 001.png built a .part that passed verifyPart, the ListPages count check AND the order check, and served image B for entry A. {001.png, 001.webp} built a part with two members literally named 001.webp. Both would have destroyed the original on swap. Now refused whole with ErrDuplicateEntryName before a byte is written; the plan is keyed by archive index.
  • #6 — on the mixed-page-size fixture, with the sample covering the entire population (so nothing is extrapolated), the old estimator projected 784,685 bytes saved where the measured truth was 2,970. Ratio-of-sums reproduces the measurement.
  • #9b — the tied symmetric shape flipped its tail anchor on run 3 of 400.
  • #10 — weakest tail hit 0.298, strongest proximity guess 0.447. Bands are now disjoint: tail 0.50–1.00, proximity 0.05–0.45.
  • #18 — a tail with direction (0,0) produced a tail attribution at confidence 0.925; a zero-area figure box won a proximity attribution at 0.435.
  • #3 — with the read-ahead removed the reader stalls at 2 entries (10,240 bytes) where 5 (24,064) are required.
  • #19 — cmd/migrate-identity's own TestIntrospectionFindsSchemaAndRejectsUnknownReference was already failing on master, unnoticed because ./cmd/... is outside the usual test lane: dialogue_attributions, series_suspect_members, recompress_journals, recompress_page_events were reported as unknown references from a newer schema. Now derived from db.KnownComicReferenceTables.

Design decisions recorded in commit messages

  • batchReadAheadPages = 4 — conservative, not configurable; memory bounded at (parallel + 4) page bodies.
  • Batch client: no Client.Timeout, but 30s dial / 30s TLS / 2m response-header.
  • Never-convertible exclusion covers 16-bit-source and too-large (both permanent codec limits); anomalies (pixel-mismatch, encode-failed) deliberately stay in the projectable base and are surfaced by the 5% caveat instead.
  • proximityMaxRange = 0.75 of the region diagonal.
  • Mid-run scanner detection stops the run cleanly (noted, resumable) rather than failing the job.
  • A figure whose only cluster match is taken by a same-pane sibling opens a new cluster rather than taking a worse match.

API shape changes mirrored in api/openapi.yaml

  • /api/librarian/recompress: new verify_failures[] + verify_failure_total; recent_refusals is now everything except pixel mismatches; row shape factored into a shared RecompressPageEvent schema (enum gains 16-bit-source).
  • DialogueAttribution.rung enum gains human, with the exclusion rule and the confidence bands documented.

Gates (all clean on 5f6e517)

$ GOWORK=off CGO_LDFLAGS=-lprotobuf go build ./...      # exit 0, no output
$ GOWORK=off CGO_LDFLAGS=-lprotobuf go vet ./...        # exit 0, no output
$ GOWORK=off CGO_LDFLAGS=-lprotobuf go test ./api/ ./db/ ./agent/ ./archive/ ./librarian/ ./components/... -count=1
ok  kreeader-server/api                                       41.9s
ok  kreeader-server/db                                         8.5s
ok  kreeader-server/agent                                      1.3s
ok  kreeader-server/archive                                    3.6s
ok  kreeader-server/librarian                                  6.1s
ok  kreeader-server/components/...                    (all ok, 0 fail)

Also run, since #19 lives there: go test ./cmd/... ./scanner/... . — all ok.

Not merged, not closed, no deploy, no production access, per the brief.

## Fix wave complete — branch `fix/review-20260828`, head `5f6e517` All 20 findings addressed. 18 commits, one per logical fix (6+7 share one — same function, same overclaim). Every fix carries a test that fails on master and passes on the branch; where the fix introduced a new symbol the pre-fix failure is a compile error, so I additionally reverted the *behaviour* in place and captured the real numbers, quoted below. | # | Finding | Status | Commit | |---|---|---|---| | 1 | recompress plan keyed on `f.Name`, duplicate entries collapse | fixed | `7017a1f` | | 2 | `webpName` collides with an existing `X.webp` | fixed | `7017a1f` | | 3 | batch fetch on a total-deadline client + unbuffered jobs | fixed | `e3fd516` | | 4 | `orderPreserved` compares sorted vs physical order | fixed | `0d16fdc` | | 5 | `ScannerBusy` checked once per multi-hour run | fixed | `8dcb81f` | | 6 | survey estimator: mean-of-ratios, 16-bit bytes in the base | fixed | `2550e49` | | 7 | unmeasurable-pages caveat never fires | fixed | `2550e49` | | 8 | `RecentRefusals` buries pixel mismatches | fixed | `160abcf` | | 9 | bubble tail `Depth` = global max; map-order tip choice | fixed | `b12cd99` | | 10 | rung confidence scales overlap; pane vs page diagonal | fixed | `1c690f9` | | 11 | `ReasonNoHit` dead code | fixed | `08a9fbb` | | 12 | corrections stored `Rung="tail"` | fixed | `f278130` | | 13 | two figures in one pane can share a cluster | fixed | `bb81555` | | 14 | pre-auth marker fingerprinting trade-off undocumented | fixed | `47ca3a3` | | 15 | `AppendAttributionsWithDB` slice-copy: id/created_at 0 | fixed | `e1f740a` | | 16 | partial-stream degradation claim was false | fixed (made real) | `61881a0` | | 17 | `historicalComicIDTables` raw strings | fixed | `78435f1` | | 18 | `clamp01` NaN, zero-direction tails, zero-area figures | fixed | `9249ba1` | | 19 | `migrate-identity` `childTables` hand list | fixed | `5d1995c` | | 20 | `docs/speaker-attribution.md` gate claim + new semantics | fixed | `5f6e517` | Nothing deferred. ### Reproductions worth recording - **#1/#2** — on master a CBZ with two members named `001.png` built a `.part` that passed `verifyPart`, the `ListPages` count check AND the order check, and served image B for entry A. `{001.png, 001.webp}` built a part with two members literally named `001.webp`. Both would have destroyed the original on swap. Now refused whole with `ErrDuplicateEntryName` before a byte is written; the plan is keyed by archive index. - **#6** — on the mixed-page-size fixture, with the sample covering the *entire* population (so nothing is extrapolated), the old estimator projected **784,685 bytes saved** where the measured truth was **2,970**. Ratio-of-sums reproduces the measurement. - **#9b** — the tied symmetric shape flipped its tail anchor on **run 3 of 400**. - **#10** — weakest tail hit `0.298`, strongest proximity guess `0.447`. Bands are now disjoint: tail 0.50–1.00, proximity 0.05–0.45. - **#18** — a tail with direction `(0,0)` produced a `tail` attribution at **confidence 0.925**; a zero-area figure box won a proximity attribution at `0.435`. - **#3** — with the read-ahead removed the reader stalls at 2 entries (10,240 bytes) where 5 (24,064) are required. - **#19** — `cmd/migrate-identity`'s own `TestIntrospectionFindsSchemaAndRejectsUnknownReference` was **already failing on master**, unnoticed because `./cmd/...` is outside the usual test lane: `dialogue_attributions`, `series_suspect_members`, `recompress_journals`, `recompress_page_events` were reported as unknown references from a newer schema. Now derived from `db.KnownComicReferenceTables`. ### Design decisions recorded in commit messages - `batchReadAheadPages = 4` — conservative, not configurable; memory bounded at `(parallel + 4)` page bodies. - Batch client: no `Client.Timeout`, but 30s dial / 30s TLS / 2m response-header. - Never-convertible exclusion covers `16-bit-source` **and** `too-large` (both permanent codec limits); anomalies (`pixel-mismatch`, `encode-failed`) deliberately stay in the projectable base and are surfaced by the 5% caveat instead. - `proximityMaxRange = 0.75` of the region diagonal. - Mid-run scanner detection stops the run cleanly (noted, resumable) rather than failing the job. - A figure whose only cluster match is taken by a same-pane sibling opens a **new** cluster rather than taking a worse match. ### API shape changes mirrored in `api/openapi.yaml` - `/api/librarian/recompress`: new `verify_failures[]` + `verify_failure_total`; `recent_refusals` is now everything *except* pixel mismatches; row shape factored into a shared `RecompressPageEvent` schema (enum gains `16-bit-source`). - `DialogueAttribution.rung` enum gains `human`, with the exclusion rule and the confidence bands documented. ### Gates (all clean on `5f6e517`) ``` $ GOWORK=off CGO_LDFLAGS=-lprotobuf go build ./... # exit 0, no output $ GOWORK=off CGO_LDFLAGS=-lprotobuf go vet ./... # exit 0, no output $ GOWORK=off CGO_LDFLAGS=-lprotobuf go test ./api/ ./db/ ./agent/ ./archive/ ./librarian/ ./components/... -count=1 ok kreeader-server/api 41.9s ok kreeader-server/db 8.5s ok kreeader-server/agent 1.3s ok kreeader-server/archive 3.6s ok kreeader-server/librarian 6.1s ok kreeader-server/components/... (all ok, 0 fail) ``` Also run, since #19 lives there: `go test ./cmd/... ./scanner/... .` — all ok. Not merged, not closed, no deploy, no production access, per the brief.
kreeader-user commented 2026-08-28 05:38:46 -05:00
Author
Owner

All findings resolved and verified live:

  • cf25c5c: gate bypass + backfill geometry wipe (deployed in 30.70).
  • fix/review-20260828 merged (18 commits, all 20 remaining findings, each with a fails-before/passes-after test) — deployed as 30.70.
  • Ratchet side landed on kreeader-ratchet master (revert script keyed on the run record, orphan-tag cleanup mode, 4 script bugs, CORRECTIONS.md honesty record, selftest +24 checks).
  • Bonus find during cutover verification: staleComics treated undecodable-marked pages as pending forever, so one corrupt archive entry re-leased its whole book to the agent in a hot loop (13-page book detected 3x in 4 minutes, 722 MiB books in rotation). Fixed + tested, deployed as 30.70.2; the loop books have not reappeared since (0 occurrences in the settle window), while 401-page/560 MiB and 214-page/722 MiB books batch-fetch in ~2.5 min through the no-total-deadline client — which the old 5-minute deadline would have killed mid-body.

Still open elsewhere: keep-or-revert decision on the 264 production flags (questions/, revert is one command now); Sol re-review queued behind codex reauth.

All findings resolved and verified live: - cf25c5c: gate bypass + backfill geometry wipe (deployed in 30.70). - fix/review-20260828 merged (18 commits, all 20 remaining findings, each with a fails-before/passes-after test) — deployed as 30.70. - Ratchet side landed on kreeader-ratchet master (revert script keyed on the run record, orphan-tag cleanup mode, 4 script bugs, CORRECTIONS.md honesty record, selftest +24 checks). - Bonus find during cutover verification: staleComics treated undecodable-marked pages as pending forever, so one corrupt archive entry re-leased its whole book to the agent in a hot loop (13-page book detected 3x in 4 minutes, 722 MiB books in rotation). Fixed + tested, deployed as 30.70.2; the loop books have not reappeared since (0 occurrences in the settle window), while 401-page/560 MiB and 214-page/722 MiB books batch-fetch in ~2.5 min through the no-total-deadline client — which the old 5-minute deadline would have killed mid-body. Still open elsewhere: keep-or-revert decision on the 264 production flags (questions/, revert is one command now); Sol re-review queued behind codex reauth.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
master
fix/agreement-and-skew-validation
fix/apply-gcd-candidate-binding
fix/detector-fingerprint
fix/token-creator-liveness
oss
v30.93-0
v30.92-3
v30.91-2
v30.90-0
v30.89-4
v30.88-0
v30.87-1
v30.86-7
v30.85-0
v30.84-7
v30.83-0
v30.82-0
v30.81-6
v30.80-1
v30.79-5
v30.78-3
v30.77-4
v30.76-1
v30.75-3
v30.73-0
v30.72-0
v30.71-0
v30.70-1
v30.68-1
v30.67-5
v30.66-1
v30.65-1
v30.64-3
v30.63-7
v30.62-1
v30.61-0
v30.60-1
v30.59-1
v30.58-0
v30.57-0
v30.56-0
v30.55-1
v30.54-1
v30.53-0
v30.52-0
v30.51-0
v30.50-0
v30.49-0
v30.48-0
v30.47-1
v30.46-1
v30.45-0
v30.44-0
v30.43-6
v30.42-0
v30.41-0
v30.40-0
v30.39-1
v30.37-0
v30.36-0
v30.35-1
v30.34-0
v30.33-0
v30.32-0
v30.31-1
v30.30-0
v30.29-0
v30.28-0
v30.2-0
v30.1-0
major-30
major-29
major-28
major-27
major-26
major-25
major-24
major-23
major-22
major-21
major-20
major-19
major-18
major-17
major-16
major-15
major-14
major-13
major-12
major-11
major-10
major-9
major-8
major-7
major-6
major-5
major-4
major-3
major-2
major-1
major-0
Labels
Clear labels   
agent

   
blocked

   
foreground

   
needs-andrew
  
Compat/Breaking
Breaking change that won't be backward compatible

  
Kind/Bug
Something is not working

  
Kind/Documentation
Documentation changes

  
Kind/Enhancement
Improve existing functionality

  
Kind/Feature
New functionality

  
Kind/Security
This is security issue

  
Kind/Testing
Issue or pull request related to testing

  
Priority
Critical
 The priority is critical

  
Priority
High
 The priority is high

  
Priority
Low
 The priority is low

  
Priority
Medium
 The priority is medium

  
Reviewed
Confirmed
 Issue has been confirmed

  
Reviewed
Duplicate
 This issue or pull request already exists

  
Reviewed
Invalid
 Invalid issue

  
Reviewed
Won't Fix
 This issue won't be fixed

  
Status
Abandoned
 Somebody has started to work on this but abandoned work

  
Status
Blocked
 Something is blocking this issue or pull request

  
Status
Need More Info
 Feedback is required to reproduce issue or to continue work

  
Tabled
Not to be implemented yet. This is for much later.

  
needs-andrew
Blocked on Andrew: decision, credential, hardware, or eyes

No labels
agent
blocked
foreground
needs-andrew
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Tabled
needs-andrew
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
kreeader/kreeader-server#147
No description provided.