fix(review-minors): batch of 8 review-minor follow-ups (bookshelf-ifwpa) #1397

Merged

zombor merged 1 commit from bd-bookshelf-ifwpa into main 2026-08-08 15:36:32 +00:00

Conversation 2 Commits 1 Files changed 22 +403 -156

zombor commented 2026-08-08 02:34:32 +00:00

Owner

Copy link

Summary

Umbrella PR folding 8 small review-minor follow-up beads into one reviewable diff, per bookshelf-ifwpa. Each sub-bead was investigated against current origin/main first; some findings were already moot (code refactored since the original review) and are noted below.

Addressed

• cco6 (0zpr.3/#322 On Deck): ListOnDeck's next-unread NOT EXISTS anti-join gets a b2.id < b.id tiebreaker so two books sharing a series_number can't both surface as "next" (data-quality footgun). Anon-user test sentinel switched from an int64 zero-value (BeZero passes even if stub is called with uid=0) to a falsifiable bool flag.
• dhf4 (#355 eapl): buildEnrichDeps (194 lines) split into buildEnrichBulkWaitFor / buildEnrichProviders / buildEnrichRatingSetters / buildEnrichLoadRefreshOpts / buildEnrichRefetchRaw helpers, bringing it under the funlen gate — the .golangci.yml grandfather exclusion for it is removed. wfengine/engine.go's New() now uses e.logger consistently (was mixing the local logger param and e.logger after e is constructed).
• drcg (1ho9/#324 comic dedup): added a UNIQUE KEY (book_id, creator_id, role) on comic_metadata_creator_mapping via migration 0051 (with a dedup pre-step for any pre-existing duplicate rows) so INSERT IGNORE in persist.go is a real DB-side guard instead of a silent no-op (the table only had a PK on auto-increment id). persist_test.go's 5 duplicate-name regression Describe blocks now call the SUT in JustBeforeEach instead of BeforeEach.
• ebti (#1015 wdow): pdf_page_images_streaming_test.go's decode-loop collapsed to a single HaveEach(WithTransform(...)) Expect. llm_scan_workflow_test.go's 3 vacuous if errors.As(err, &wfErr) { Expect(wfErr.Permanent).To(BeFalse()) } guards (never execute the inner Expect on the happy/expected path) replaced with unconditional Expect(errors.As(err, &wfErr)).To(BeFalse()) — verified against ScanBook's actual error-wrapping logic that these paths never produce a *gowf.Error.
• k5pd (0zpr.2/#320 Discover rail): ContainElements(int64(4), int64(4)) (only proved id=4 wasn't duplicated — missed id=5) replaced with ConsistOf(1,2,3,4,5) asserting the exact expected id set.
• q1lq (#354 sp5u): bookdrop.IngestFile now takes a cheap indexed exists(ctx, path) (bool, error) dep and skips extractMeta (a full EPUB ZIP parse) whenever the file is already known to bookdrop_file — InsertBookdropFileIfNew's ON DUPLICATE KEY UPDATE clause never touches original_metadata for existing rows, so that extraction work was pure waste on every rescan pass over an unchanged library. Added a filename-fallback (non-EPUB) coverage test that was previously missing.
• iunx (obdm follow-ups): scan_run.go's Phase-3 doc comment now explicitly states the authors/series post-TX gap is NOT auto-healed on a later scan (by design — see bookshelf-obdm). batch_seed_extra_test.go's mismatch-skip test now asserts execCallCount==0 to actually prove the defensive !ok skip fired. persist_test.go's TX-args assertion uses a panic-safe ContainElement(ContainElement(...)) match instead of indexing txExecArgs[0] directly. scan_extra.go doc comments no longer reference the no-longer-existing singular InsertBookAndFileIfAbsent function name (the singular path was already removed in a prior refactor — batching is now the only insert path).
• 8uhoy: comicvine/title_test.go's tautological Expect(result, err).NotTo(BeNil()) (trivially false on a non-nilable value struct) replaced with Expect(result.Title, err).NotTo(BeEmpty()).

Left out (reported, not folded in — stay their own follow-up items)

• dhf4 item 3 (e2e/api journey_6 manual poll → Gomega Eventually): the file's multi-Expect-per-It pattern is already permitted under the project's e2e multi-Expect relaxation policy (CLAUDE.md), and the manual time.Now()+Sleep poll idiom is used consistently across that entire journey file. Converting only this one spec would be an inconsistent partial refactor.
• drcg item 2 (char/team/location INSERT IGNORE → ON DUPLICATE KEY UPDATE): those mapping tables already have real PK-based uniqueness (book_id, entity_id), so INSERT IGNORE is meaningful there today. The suggestion is speculative future-hardening against a delete path for reference tables that doesn't exist yet.
• q1lq item 3 (256MB buffered EPUB fallback): already documented in internal/library/scan/meta.go as a non-io.ReaderAt (effectively test-only) fallback path; no code change needed.
• q1lq item 4 (slog.Default() in cover_extract.go): fixing requires threading a logger through ExtractCoverFromFile and its whole call chain (findOPFPath/findCoverPath/parseOPF/decodeXMLEntry) across every caller — disproportionate diff growth for a cosmetic pre-existing nit the original review explicitly flagged as "not introduced by #354".
• k5pd item 1 (ListDiscover MIN/MAX multi-Expect It): no longer exists — the Describe block was refactored since PR #320 and no test matching that description remains on origin/main.

Docs: N/A — internal test/code hygiene + one query-level SQL fix (adds an index/constraint, no schema column change), no user-facing surface changes.

Closes bookshelf-ifwpa. Also closes: bookshelf-cco6, bookshelf-dhf4, bookshelf-drcg, bookshelf-ebti, bookshelf-k5pd, bookshelf-q1lq, bookshelf-iunx, bookshelf-8uhoy.

Test plan

• make test — all packages green
• make lint — 0 issues, all policy checks (e2e/test/screenshot/controller/csrf/cursor) pass
• make integration — internal/db (incl. schema-compat allowlist guard), internal/dbtest, internal/wfengine, cmd/pergamum all green, confirming migration 0051 applies cleanly
• CI green (see below)

🤖 Generated with Claude Code

## Summary Umbrella PR folding 8 small review-minor follow-up beads into one reviewable diff, per bookshelf-ifwpa. Each sub-bead was investigated against current origin/main first; some findings were already moot (code refactored since the original review) and are noted below. ### Addressed - **cco6** (0zpr.3/#322 On Deck): `ListOnDeck`'s next-unread `NOT EXISTS` anti-join gets a `b2.id < b.id` tiebreaker so two books sharing a `series_number` can't both surface as "next" (data-quality footgun). Anon-user test sentinel switched from an `int64` zero-value (`BeZero` passes even if stub is called with uid=0) to a falsifiable `bool` flag. - **dhf4** (#355 eapl): `buildEnrichDeps` (194 lines) split into `buildEnrichBulkWaitFor` / `buildEnrichProviders` / `buildEnrichRatingSetters` / `buildEnrichLoadRefreshOpts` / `buildEnrichRefetchRaw` helpers, bringing it under the funlen gate — the `.golangci.yml` grandfather exclusion for it is removed. `wfengine/engine.go`'s `New()` now uses `e.logger` consistently (was mixing the local `logger` param and `e.logger` after `e` is constructed). - **drcg** (1ho9/#324 comic dedup): added a `UNIQUE KEY (book_id, creator_id, role)` on `comic_metadata_creator_mapping` via migration 0051 (with a dedup pre-step for any pre-existing duplicate rows) so `INSERT IGNORE` in `persist.go` is a real DB-side guard instead of a silent no-op (the table only had a PK on auto-increment `id`). `persist_test.go`'s 5 duplicate-name regression `Describe` blocks now call the SUT in `JustBeforeEach` instead of `BeforeEach`. - **ebti** (#1015 wdow): `pdf_page_images_streaming_test.go`'s decode-loop collapsed to a single `HaveEach(WithTransform(...))` `Expect`. `llm_scan_workflow_test.go`'s 3 vacuous `if errors.As(err, &wfErr) { Expect(wfErr.Permanent).To(BeFalse()) }` guards (never execute the inner Expect on the happy/expected path) replaced with unconditional `Expect(errors.As(err, &wfErr)).To(BeFalse())` — verified against `ScanBook`'s actual error-wrapping logic that these paths never produce a `*gowf.Error`. - **k5pd** (0zpr.2/#320 Discover rail): `ContainElements(int64(4), int64(4))` (only proved id=4 wasn't duplicated — missed id=5) replaced with `ConsistOf(1,2,3,4,5)` asserting the exact expected id set. - **q1lq** (#354 sp5u): `bookdrop.IngestFile` now takes a cheap indexed `exists(ctx, path) (bool, error)` dep and skips `extractMeta` (a full EPUB ZIP parse) whenever the file is already known to `bookdrop_file` — `InsertBookdropFileIfNew`'s `ON DUPLICATE KEY UPDATE` clause never touches `original_metadata` for existing rows, so that extraction work was pure waste on every rescan pass over an unchanged library. Added a filename-fallback (non-EPUB) coverage test that was previously missing. - **iunx** (obdm follow-ups): `scan_run.go`'s Phase-3 doc comment now explicitly states the authors/series post-TX gap is NOT auto-healed on a later scan (by design — see bookshelf-obdm). `batch_seed_extra_test.go`'s mismatch-skip test now asserts `execCallCount==0` to actually prove the defensive `!ok` skip fired. `persist_test.go`'s TX-args assertion uses a panic-safe `ContainElement(ContainElement(...))` match instead of indexing `txExecArgs[0]` directly. `scan_extra.go` doc comments no longer reference the no-longer-existing singular `InsertBookAndFileIfAbsent` function name (the singular path was already removed in a prior refactor — batching is now the only insert path). - **8uhoy**: `comicvine/title_test.go`'s tautological `Expect(result, err).NotTo(BeNil())` (trivially false on a non-nilable value struct) replaced with `Expect(result.Title, err).NotTo(BeEmpty())`. ### Left out (reported, not folded in — stay their own follow-up items) - **dhf4 item 3** (e2e/api journey_6 manual poll → `Gomega Eventually`): the file's multi-`Expect`-per-`It` pattern is already permitted under the project's e2e multi-Expect relaxation policy (CLAUDE.md), and the manual `time.Now()+Sleep` poll idiom is used consistently across that entire journey file. Converting only this one spec would be an inconsistent partial refactor. - **drcg item 2** (char/team/location `INSERT IGNORE` → `ON DUPLICATE KEY UPDATE`): those mapping tables already have real PK-based uniqueness `(book_id, entity_id)`, so `INSERT IGNORE` is meaningful there today. The suggestion is speculative future-hardening against a delete path for reference tables that doesn't exist yet. - **q1lq item 3** (256MB buffered EPUB fallback): already documented in `internal/library/scan/meta.go` as a non-`io.ReaderAt` (effectively test-only) fallback path; no code change needed. - **q1lq item 4** (`slog.Default()` in `cover_extract.go`): fixing requires threading a logger through `ExtractCoverFromFile` and its whole call chain (`findOPFPath`/`findCoverPath`/`parseOPF`/`decodeXMLEntry`) across every caller — disproportionate diff growth for a cosmetic pre-existing nit the original review explicitly flagged as "not introduced by #354". - **k5pd item 1** (ListDiscover MIN/MAX multi-`Expect` `It`): no longer exists — the `Describe` block was refactored since PR #320 and no test matching that description remains on `origin/main`. Docs: N/A — internal test/code hygiene + one query-level SQL fix (adds an index/constraint, no schema column change), no user-facing surface changes. Closes bookshelf-ifwpa. Also closes: bookshelf-cco6, bookshelf-dhf4, bookshelf-drcg, bookshelf-ebti, bookshelf-k5pd, bookshelf-q1lq, bookshelf-iunx, bookshelf-8uhoy. ## Test plan - [x] `make test` — all packages green - [x] `make lint` — 0 issues, all policy checks (e2e/test/screenshot/controller/csrf/cursor) pass - [x] `make integration` — `internal/db` (incl. schema-compat allowlist guard), `internal/dbtest`, `internal/wfengine`, `cmd/pergamum` all green, confirming migration 0051 applies cleanly - [x] CI green (see below) 🤖 Generated with [Claude Code](https://claude.com/claude-code)

zombor added 1 commit 2026-08-08 02:34:32 +00:00

fix(review-minors): batch of 8 review-minor follow-ups (bookshelf-ifwpa) ... fb7e7e5f58

Addresses cco6/dhf4/drcg/ebti/k5pd/q1lq/iunx/8uhoy in one reviewable PR:

- cco6: ListOnDeck's next-unread NOT EXISTS anti-join gets a b2.id tiebreaker
  so two books sharing a series_number can't both surface as "next"; test
  sentinel switched from an int64 zero-value to a falsifiable bool flag.
- dhf4: buildEnrichDeps split into buildEnrichBulkWaitFor/buildEnrichProviders/
  buildEnrichRatingSetters/buildEnrichLoadRefreshOpts/buildEnrichRefetchRaw
  helpers, dropping it under the funlen gate (grandfather exclusion removed);
  wfengine/engine.go's New() now uses e.logger consistently instead of the
  local logger param after e is constructed.
- drcg: added a UNIQUE KEY on comic_metadata_creator_mapping(book_id,
  creator_id, role) (migration 0051, with a dedup pre-step) so INSERT IGNORE
  is a real DB-side guard instead of a no-op; persist_test.go's 5 duplicate-*
  Describe blocks now call the SUT in JustBeforeEach instead of BeforeEach.
- ebti: pdf_page_images_streaming_test.go's decode-loop collapsed to a single
  HaveEach/WithTransform Expect; llm_scan_workflow_test.go's 3 vacuous
  `if errors.As(...) { Expect(...) }` guards replaced with unconditional
  Expect(errors.As(...)).To(BeFalse()) assertions that actually run.
- k5pd: ContainElements dup-check now asserts the full expected id set via
  ConsistOf (was only checking id=4 wasn't duplicated, missing id=5).
- q1lq: IngestFile now takes a cheap indexed `exists` dep and skips extractMeta
  (EPUB ZIP parse) whenever the file is already known to bookdrop_file — the
  ON DUPLICATE KEY branch never consults original_metadata for existing rows,
  so extraction for those was pure waste on every rescan pass. Added a
  filename-fallback (non-EPUB) coverage test.
- iunx: scan_run.go's Phase-3 doc comment now states the authors/series
  post-TX gap is NOT auto-healed; batch_seed_extra_test.go's mismatch-skip
  test now asserts execCallCount==0; persist_test.go's TX-args It uses a
  panic-safe ContainElement(ContainElement(...)) match instead of indexing
  txExecArgs[0] directly; scan_extra.go doc comments no longer reference the
  no-longer-existing singular InsertBookAndFileIfAbsent function name.
- 8uhoy: comicvine/title_test.go's tautological BeNil() on a value struct
  replaced with a meaningful Expect(result.Title, err).NotTo(BeEmpty()).

Left out (reported, not folded in):
- dhf4 item 3 (e2e/api journey_6 manual poll -> Gomega Eventually): the file's
  multi-Expect-per-It pattern is already permitted under the project's e2e
  multi-Expect relaxation policy, and the manual time.Now()+Sleep poll idiom
  is used consistently throughout that journey file — converting only this
  one spec would be an inconsistent partial refactor. Stays its own item.
- drcg item 2 (char/team/location INSERT IGNORE -> ON DUPLICATE KEY UPDATE):
  those tables already have real PK-based uniqueness (book_id, entity_id), so
  INSERT IGNORE is meaningful there today; the suggestion is speculative
  future-hardening for a delete path that doesn't exist yet.
- q1lq item 3 (256MB buffered EPUB fallback): already documented in
  internal/library/scan/meta.go as a non-ReaderAt-only (effectively test-only)
  fallback path; no code change needed.
- q1lq item 4 (slog.Default() in cover_extract.go): fixing this requires
  threading a logger through ExtractCoverFromFile and its whole call chain
  (findOPFPath/findCoverPath/parseOPF/decodeXMLEntry) across every caller —
  disproportionate diff growth for a cosmetic pre-existing nit explicitly
  flagged as "not introduced by #354".
- k5pd item 1 (ListDiscover MIN/MAX multi-Expect It): no longer exists — the
  Describe was refactored since PR #320 and no test matching that description
  remains.

Docs: N/A — internal test/code hygiene + one query-level SQL fix, no
user-facing surface changes.

Closes bookshelf-ifwpa; addresses cco6/dhf4/drcg/ebti/k5pd/q1lq/iunx/8uhoy

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

zombor commented 2026-08-08 02:43:13 +00:00

Author

Owner

Copy link

Security Review — PR #1397 (bookshelf-ifwpa)

(Correcting the previous comment on this PR, which was posted with stale content from an unrelated PR due to a local file-write error on my end — apologies for the noise.)

Adversarial security pass over the diff (8 review-minor cleanups folded into one PR: migration 0051, bookdrop exists-check optimization, app.go/build_enrich_deps.go refactors, test hardening).

No findings. Summary of what was checked:

• Migration 0051 (internal/db/migrations/0051_comic_creator_mapping_unique.{up,down}.sql): the DELETE ... INNER JOIN dedup step matches on the exact composite key (book_id, creator_id, role) before adding the UNIQUE KEY, keeping the lowest id — no cross-entity data loss, no authz-relevant mapping affected (comic creator/character/team/location mappings are global metadata, not per-user). No user input is interpolated into this migration.
• SQL parameterization: the new BookdropFileExistsByPath query (internal/db/queries/bookdrop.sql / generated internal/db/sqlc/bookdrop.sql.go) uses a ? placeholder bound via QueryRowContext(ctx, query, filePath) — properly parameterized, no string concatenation. INSERT IGNORE in internal/comic/persist.go (unchanged by this PR) already uses the same three columns as the new UNIQUE constraint, so the dedup key is consistent between the Go-side guard and the new DB-side constraint.
• bookdrop.IngestFile exists-check (internal/bookdrop/service.go): the new exists dependency only skips extraction, never skips the insert call — the ON DUPLICATE KEY UPDATE path is still hit unconditionally, so proposal-reactivation/status semantics are unchanged. On an exists check error it fails open (extracts anyway) and logs a Warn with path + err only — no secrets/PII, matches logging-standard. Confirmed via internal/bookdrop/service_test.go new cases (exists=true skips extract but still inserts; exists error still extracts).
• Multi-user/scoping: none of the touched files (bookdrop, home/service.go, library/scan/scan_run.go, wfengine/engine.go, app.go, build_enrich_deps.go) remove a user_id scope, ownership check, or CSRF token. build_enrich_deps.go's 192-line diff is a pure decomposition into enrichBulkWaitFor/buildEnrichRatingSetters/buildEnrichProviders/buildEnrichLoadRefreshOpts/buildEnrichRefetchRaw helpers (funlen gate) — same params threaded through unchanged, confirmed against origin/main. home/service.go's SQL change only adds a b2.id < b.id tiebreaker to the existing next-in-series anti-join (flake-prevention determinism fix per review-standard), not a scoping change.
• q1lq "dead buffer"/duplicate-ingest test changes: internal/bookdrop/meta_test.go adds coverage for an existing filename-fallback path; no validation logic was removed, only test assertions tightened (e.g. internal/metadata/comicvine/title_test.go, internal/wfengine/llm_scan_workflow_test.go, internal/library/scan/persist_test.go all moved to stricter matchers, not looser ones).
• Logging: internal/wfengine/engine.go fixes two spots to use e.logger instead of the pre-construction logger param (cosmetic/gofmt-adjacent, no new log fields, nothing sensitive).
• CSP: no template/CSS changes in this diff — no inline style= surface touched.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #1397 (bookshelf-ifwpa) (Correcting the previous comment on this PR, which was posted with stale content from an unrelated PR due to a local file-write error on my end — apologies for the noise.) Adversarial security pass over the diff (8 review-minor cleanups folded into one PR: migration 0051, bookdrop exists-check optimization, app.go/build_enrich_deps.go refactors, test hardening). **No findings.** Summary of what was checked: - **Migration 0051** (`internal/db/migrations/0051_comic_creator_mapping_unique.{up,down}.sql`): the `DELETE ... INNER JOIN` dedup step matches on the exact composite key `(book_id, creator_id, role)` before adding the `UNIQUE KEY`, keeping the lowest `id` — no cross-entity data loss, no authz-relevant mapping affected (comic creator/character/team/location mappings are global metadata, not per-user). No user input is interpolated into this migration. - **SQL parameterization**: the new `BookdropFileExistsByPath` query (`internal/db/queries/bookdrop.sql` / generated `internal/db/sqlc/bookdrop.sql.go`) uses a `?` placeholder bound via `QueryRowContext(ctx, query, filePath)` — properly parameterized, no string concatenation. `INSERT IGNORE` in `internal/comic/persist.go` (unchanged by this PR) already uses the same three columns as the new UNIQUE constraint, so the dedup key is consistent between the Go-side guard and the new DB-side constraint. - **`bookdrop.IngestFile` exists-check** (`internal/bookdrop/service.go`): the new `exists` dependency only skips *extraction*, never skips the `insert` call — the ON DUPLICATE KEY UPDATE path is still hit unconditionally, so proposal-reactivation/status semantics are unchanged. On an `exists` check error it fails open (extracts anyway) and logs a `Warn` with path + err only — no secrets/PII, matches logging-standard. Confirmed via `internal/bookdrop/service_test.go` new cases (exists=true skips extract but still inserts; exists error still extracts). - **Multi-user/scoping**: none of the touched files (`bookdrop`, `home/service.go`, `library/scan/scan_run.go`, `wfengine/engine.go`, `app.go`, `build_enrich_deps.go`) remove a `user_id` scope, ownership check, or CSRF token. `build_enrich_deps.go`'s 192-line diff is a pure decomposition into `enrichBulkWaitFor`/`buildEnrichRatingSetters`/`buildEnrichProviders`/`buildEnrichLoadRefreshOpts`/`buildEnrichRefetchRaw` helpers (funlen gate) — same params threaded through unchanged, confirmed against `origin/main`. `home/service.go`'s SQL change only adds a `b2.id < b.id` tiebreaker to the existing next-in-series anti-join (flake-prevention determinism fix per review-standard), not a scoping change. - **`q1lq` "dead buffer"/duplicate-ingest test changes**: `internal/bookdrop/meta_test.go` adds coverage for an existing filename-fallback path; no validation logic was removed, only test assertions tightened (e.g. `internal/metadata/comicvine/title_test.go`, `internal/wfengine/llm_scan_workflow_test.go`, `internal/library/scan/persist_test.go` all moved to *stricter* matchers, not looser ones). - **Logging**: `internal/wfengine/engine.go` fixes two spots to use `e.logger` instead of the pre-construction `logger` param (cosmetic/gofmt-adjacent, no new log fields, nothing sensitive). - **CSP**: no template/CSS changes in this diff — no inline `style=` surface touched. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-08 02:44:17 +00:00

Author

Owner

Copy link

Code Review — PR #1397 (bookshelf-ifwpa)

Reviewed the diff against origin/main and cross-checked each of the 8 folded sub-beads plus the PR body's "Left out" justifications. CI is green; did not re-run tests.

Linter-baseline integrity (.golangci.yml)

Verified: exactly one exclude-rules entry removed (build_enrich_deps.go / buildEnrichDeps funlen), zero added. Traced buildEnrichDeps end-to-end — it's a genuine burn-down: the 194-line function is split into enrichBulkWaitFor, enrichRatingSetters/buildEnrichRatingSetters, enrichProviders/buildEnrichProviders, buildEnrichLoadRefreshOpts, buildEnrichRefetchRaw, leaving buildEnrichDeps itself at ~48 lines / well under the 60-line/40-statement funlen gate. Legit.

Migration 0051 (comic_creator_mapping UNIQUE)

up.sql adds a dedup DELETE ... INNER JOIN ... ON ... AND t1.id > t2.id pre-step (keeps lowest id) before ALTER TABLE ... ADD UNIQUE KEY (book_id, creator_id, role) — correctly handles pre-existing duplicate rows on a real DB, won't fail the ALTER. down.sql is the correct inverse (DROP KEY). Index-only, no new column — compliant with the Grimmory-compat column-freeze rule. Confirmed internal/comic/persist.go's INSERT IGNORE (unchanged) is now a real DB-side guard instead of a no-op, per the PR's stated intent — the PR does not convert it to ON DUPLICATE KEY UPDATE; that's drcg item 2, explicitly and correctly left out in the PR body (char/team/location tables already have real PK-based uniqueness, so the speculative-hardening rationale is sound).

q1lq (slog.Default / bookdrop skip-reparse)

bookdrop.IngestFile now takes an injected exists(ctx, path) (bool, error) and skips extractMeta when the file is already known; on an exists error it fails open (extracts anyway) — matches the doc comment. New BookdropFileExistsByPath query is an indexed EXISTS(SELECT 1 ... WHERE file_path = ?) (unique index) — cheap, no scale concern. Both callers (app.go's startBookdropGoroutines and build_bookdrop_deps.go's buildBookdropScanDeps) updated consistently. slog.Default() in cover_extract.go (q1lq item 4) is explicitly left out with a reasonable disproportionate-diff justification.

Test hygiene / conventions

• All touched/added *_test.go files remain package <pkg>_test (black-box) — spot-checked all 9.
• One-Expect-per-It preserved throughout (including the new bookdrop exists-check tests, which correctly fold err into the value Expect).
• No coverage-exclusion script changes.
• home/service.go's On-Deck tiebreaker (b2.id < b.id) and the anon-user sentinel fix (bool flag replacing a BeZero-passes-trivially int64) both check out against the described footgun.
• scan_run.go Phase-3 doc comment, batch_seed_extra_test.go's execCallCount==0 assertion, and persist_test.go's panic-safe ContainElement(ContainElement(...)) all verified against iunx.
• llm_scan_workflow_test.go's vacuous if errors.As(...) { Expect(...) } guards correctly replaced with unconditional Expect(errors.As(err, &wfErr)).To(BeFalse()) (ebti).
• title_test.go's tautological NotTo(BeNil()) on a non-nilable struct replaced with result.Title ... NotTo(BeEmpty()) (8uhoy).

Findings

[MINOR] internal/bookdrop/service.go:153-159 — alreadyKnown/existsErr declared via := mid-function instead of hoisted into the function's top var (...) block
Both vars are referenced across the next two statements (cross-block meaning per project-conventions.md "Variable declarations: prefer top-of-block"), unlike the genuinely scope-local vars the convention exempts. Minor stylistic inconsistency with the rest of the same function, which otherwise strictly hoists (absPath, info, result, ext, meta, err are all in the var block). Fix: add alreadyKnown bool and existsErr error to the existing var (...) block.

[MINOR] PR body — dhf4 item 2 (shared rate-limiter across HTTP + worker enrich paths) is neither listed under "Addressed" nor "Left out"
Investigated independently: it appears already moot — both books.Wire (HTTP path, via build_extended_deps.go) and buildEnrichDeps (worker path) key their token buckets identically ("token_bucket.<id>.rate" / "token_bucket.<id>") against the same DB-backed ratelimiter.NewDBTokenBucket with atomic ClaimTokenBucketSlot/ClaimTokenBucketSlotWithReserve claims — so the two paths already share real cross-process rate-limiting state, not separate in-memory limiter instances, and the dhf4 concern doesn't apply to the current code. Recommend a one-line addition to the PR body's "Left out" section (or an "Already moot" note) so future readers don't have to re-derive this the way this review did. No code change needed.

No blockers, no majors. Every sub-bead's claimed "addressed" change was verified against the actual diff; every "left out" claim was independently checked against origin/main and found accurate.

REVIEW VERDICT: 0 blocker, 0 major, 2 minor

## Code Review — PR #1397 (bookshelf-ifwpa) Reviewed the diff against `origin/main` and cross-checked each of the 8 folded sub-beads plus the PR body's "Left out" justifications. CI is green; did not re-run tests. ### Linter-baseline integrity (.golangci.yml) Verified: exactly one `exclude-rules` entry removed (`build_enrich_deps.go` / `buildEnrichDeps` funlen), zero added. Traced `buildEnrichDeps` end-to-end — it's a genuine burn-down: the 194-line function is split into `enrichBulkWaitFor`, `enrichRatingSetters`/`buildEnrichRatingSetters`, `enrichProviders`/`buildEnrichProviders`, `buildEnrichLoadRefreshOpts`, `buildEnrichRefetchRaw`, leaving `buildEnrichDeps` itself at ~48 lines / well under the 60-line/40-statement funlen gate. Legit. ### Migration 0051 (comic_creator_mapping UNIQUE) `up.sql` adds a dedup `DELETE ... INNER JOIN ... ON ... AND t1.id > t2.id` pre-step (keeps lowest id) before `ALTER TABLE ... ADD UNIQUE KEY (book_id, creator_id, role)` — correctly handles pre-existing duplicate rows on a real DB, won't fail the ALTER. `down.sql` is the correct inverse (`DROP KEY`). Index-only, no new column — compliant with the Grimmory-compat column-freeze rule. Confirmed `internal/comic/persist.go`'s `INSERT IGNORE` (unchanged) is now a real DB-side guard instead of a no-op, per the PR's stated intent — the PR does **not** convert it to `ON DUPLICATE KEY UPDATE`; that's `drcg` item 2, explicitly and correctly left out in the PR body (char/team/location tables already have real PK-based uniqueness, so the speculative-hardening rationale is sound). ### q1lq (slog.Default / bookdrop skip-reparse) `bookdrop.IngestFile` now takes an injected `exists(ctx, path) (bool, error)` and skips `extractMeta` when the file is already known; on an `exists` error it fails open (extracts anyway) — matches the doc comment. New `BookdropFileExistsByPath` query is an indexed `EXISTS(SELECT 1 ... WHERE file_path = ?)` (unique index) — cheap, no scale concern. Both callers (`app.go`'s `startBookdropGoroutines` and `build_bookdrop_deps.go`'s `buildBookdropScanDeps`) updated consistently. `slog.Default()` in `cover_extract.go` (q1lq item 4) is explicitly left out with a reasonable disproportionate-diff justification. ### Test hygiene / conventions - All touched/added `*_test.go` files remain `package <pkg>_test` (black-box) — spot-checked all 9. - One-Expect-per-It preserved throughout (including the new bookdrop `exists`-check tests, which correctly fold err into the value `Expect`). - No coverage-exclusion script changes. - `home/service.go`'s On-Deck tiebreaker (`b2.id < b.id`) and the anon-user sentinel fix (`bool` flag replacing a `BeZero`-passes-trivially `int64`) both check out against the described footgun. - `scan_run.go` Phase-3 doc comment, `batch_seed_extra_test.go`'s `execCallCount==0` assertion, and `persist_test.go`'s panic-safe `ContainElement(ContainElement(...))` all verified against `iunx`. - `llm_scan_workflow_test.go`'s vacuous `if errors.As(...) { Expect(...) }` guards correctly replaced with unconditional `Expect(errors.As(err, &wfErr)).To(BeFalse())` (ebti). - `title_test.go`'s tautological `NotTo(BeNil())` on a non-nilable struct replaced with `result.Title ... NotTo(BeEmpty())` (8uhoy). ### Findings [MINOR] internal/bookdrop/service.go:153-159 — `alreadyKnown`/`existsErr` declared via `:=` mid-function instead of hoisted into the function's top `var (...)` block Both vars are referenced across the next two statements (cross-block meaning per project-conventions.md "Variable declarations: prefer top-of-block"), unlike the genuinely scope-local vars the convention exempts. Minor stylistic inconsistency with the rest of the same function, which otherwise strictly hoists (`absPath`, `info`, `result`, `ext`, `meta`, `err` are all in the var block). Fix: add `alreadyKnown bool` and `existsErr error` to the existing `var (...)` block. [MINOR] PR body — dhf4 item 2 (shared rate-limiter across HTTP + worker enrich paths) is neither listed under "Addressed" nor "Left out" Investigated independently: it appears already moot — both `books.Wire` (HTTP path, via `build_extended_deps.go`) and `buildEnrichDeps` (worker path) key their token buckets identically (`"token_bucket.<id>.rate"` / `"token_bucket.<id>"`) against the same DB-backed `ratelimiter.NewDBTokenBucket` with atomic `ClaimTokenBucketSlot`/`ClaimTokenBucketSlotWithReserve` claims — so the two paths already share real cross-process rate-limiting state, not separate in-memory limiter instances, and the dhf4 concern doesn't apply to the current code. Recommend a one-line addition to the PR body's "Left out" section (or an "Already moot" note) so future readers don't have to re-derive this the way this review did. No code change needed. No blockers, no majors. Every sub-bead's claimed "addressed" change was verified against the actual diff; every "left out" claim was independently checked against origin/main and found accurate. REVIEW VERDICT: 0 blocker, 0 major, 2 minor

zombor force-pushed bd-bookshelf-ifwpa from fb7e7e5f58 to b3f1ed8a41 2026-08-08 15:28:52 +00:00 Compare

zombor merged commit d097d2f0e3 into main 2026-08-08 15:36:32 +00:00

zombor referenced this pull request from a commit 2026-08-08 15:36:35 +00:00

Merge pull request 'fix(review-minors): batch of 8 review-minor follow-ups (bookshelf-ifwpa)' (#1397) from bd-bookshelf-ifwpa into main

zombor referenced this pull request 2026-08-12 20:00:32 +00:00

fix(review): address batch MINOR/def-in-depth follow-ups (bookshelf-syyzy) #1437

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

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

zombor/pergamum!1397

No description provided.