refactor(consolidation): delete dead packages/handlers + unscoped magic footguns (bookshelf-9snmz.1.3) #1255

Merged

zombor merged 2 commits from bd-bookshelf-9snmz.1.3 into main 2026-07-27 12:40:08 +00:00

Conversation 3 Commits 2 Files changed 20 +110 -1007

zombor commented 2026-07-27 02:17:33 +00:00

Owner

Copy link

Summary

Deletes four clusters of dead code (scoped to avoid overlap with sibling PR #1254 which already removed hardcoversync.SyncUserBooks/pushBooks and shelves.ListBooks):

1. internal/version package (~37 LOC deleted) — self-admitted placeholder; main.version via -ldflags is the real version source. Removed from UNIT_PKGS and check-coverage.sh.

2. Dead admin-users list handler (~66 LOC deleted) — GET /admin/users is a 301 redirect in production; adminUsersListHandler/parseListParams/buildUserListRows/adminUsersPageData and related constants are dead. AdminUserListRow preserved (used by settings/shell_handler.go). Updated export_test.go to omit the dead route.

3. Dead startupHooks machinery in internal/app (~15 LOC deleted) — field declared and slice allocated, but no hook was ever appended; the loop iterated an empty slice on every startup. Pure dead weight.

4. Unscoped shelf/magic query paths (~108 LOC deleted) — security footguns — production wires only the *Scoped variants; the unscoped paths bypass per-user library scoping:

   • magic.CountBooks + buildMagicShelfCountQuery (unscoped; CountBooksScoped is sole prod caller)
   • magic.ListBooksForMagicShelf + buildMagicShelfQuery (unscoped; ListBooksForMagicShelfScoped is sole prod caller)
   • magic.EncodeAdvSearch (no production callers; DecodeAdvSearch is live)
   • magic.ValidOperator (no production callers; ValidFieldOperator is live)

All companion tests deleted with their symbols. Added tests for branches previously covered by deleted books_test.go (clampBooksLimit bounds, tr.Joins, afterID > 0, rows.Err()).

Test plan

• make coverage passes (100% gate, zero uncovered statement blocks)
• make lint clean for changed packages
• go build ./... succeeds
• Sibling PR #1254 scope excluded (hardcoversync + shelves.ListBooks reverted to origin/main)

Closes bead bookshelf-9snmz.1.3 on merge.

## Summary Deletes four clusters of dead code (scoped to avoid overlap with sibling PR #1254 which already removed hardcoversync.SyncUserBooks/pushBooks and shelves.ListBooks): 1. **`internal/version` package (~37 LOC deleted)** — self-admitted placeholder; `main.version` via `-ldflags` is the real version source. Removed from `UNIT_PKGS` and `check-coverage.sh`. 2. **Dead admin-users list handler (~66 LOC deleted)** — `GET /admin/users` is a 301 redirect in production; `adminUsersListHandler`/`parseListParams`/`buildUserListRows`/`adminUsersPageData` and related constants are dead. `AdminUserListRow` preserved (used by `settings/shell_handler.go`). Updated `export_test.go` to omit the dead route. 3. **Dead `startupHooks` machinery in `internal/app` (~15 LOC deleted)** — field declared and slice allocated, but no hook was ever appended; the loop iterated an empty slice on every startup. Pure dead weight. 4. **Unscoped shelf/magic query paths (~108 LOC deleted) — security footguns** — production wires only the `*Scoped` variants; the unscoped paths bypass per-user library scoping: - `magic.CountBooks` + `buildMagicShelfCountQuery` (unscoped; `CountBooksScoped` is sole prod caller) - `magic.ListBooksForMagicShelf` + `buildMagicShelfQuery` (unscoped; `ListBooksForMagicShelfScoped` is sole prod caller) - `magic.EncodeAdvSearch` (no production callers; `DecodeAdvSearch` is live) - `magic.ValidOperator` (no production callers; `ValidFieldOperator` is live) All companion tests deleted with their symbols. Added tests for branches previously covered by deleted `books_test.go` (`clampBooksLimit` bounds, `tr.Joins`, `afterID > 0`, `rows.Err()`). ## Test plan - [x] `make coverage` passes (100% gate, zero uncovered statement blocks) - [x] `make lint` clean for changed packages - [x] `go build ./...` succeeds - [x] Sibling PR #1254 scope excluded (hardcoversync + shelves.ListBooks reverted to origin/main) Closes bead bookshelf-9snmz.1.3 on merge.

zombor added 1 commit 2026-07-27 02:17:33 +00:00

refactor(consolidation): delete dead packages/handlers + unscoped magic-shelf footguns (bookshelf-9snmz.1.3) ... 6512da7def

Scope (after sibling PR #1254 carved out hardcoversync/shelves.ListBooks):

1. Delete entire internal/version package (~37 LOC): self-admitted placeholder;
   main.version via -ldflags is the real version source. Removed from UNIT_PKGS
   and check-coverage.sh package list.

2. Delete dead admin-users list handler (~66 LOC): GET /admin/users is a 301
   redirect in production; adminUsersListHandler/parseListParams/buildUserListRows/
   adminUsersPageData/related constants are all dead. AdminUserListRow is preserved
   (still used by settings/shell_handler.go for the Settings Users tab).
   Updated export_test.go to omit the dead route from test mux registration.

3. Delete dead startupHooks machinery in internal/app (~15 LOC): startupHooks field
   and startupHooksSlice declared and allocated, but no hook was ever appended; the
   run loop iterated an empty slice on every startup. Pure dead weight.

4. Delete unscoped shelf/magic query paths (~108 LOC) — per-user-scoping SECURITY
   FOOTGUNS whose removal is a correctness win:
   - magic.CountBooks + buildMagicShelfCountQuery (unscoped; CountBooksScoped is
     the sole production caller)
   - magic.ListBooksForMagicShelf + buildMagicShelfQuery (unscoped; ListBooksForMagicShelfScoped
     is the sole production caller)
   - magic.EncodeAdvSearch (dead; no production callers; DecodeAdvSearch is live)
   - magic.ValidOperator (dead; ValidFieldOperator is live and wired)

   Deleted books_test.go entirely (all 6 Describe blocks tested dead functions).
   Added coverage for previously-covered branches now only reachable via the scoped
   path: clampBooksLimit bounds, tr.Joins join clause, afterID > 0 predicate, and
   scanMagicShelfBooks rows.Err() error path.

All companion tests deleted with their symbols. make coverage passes at 100%.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-07-27 02:30:29 +00:00

Author

Owner

Copy link

Code Review: PR #1255 (Deletion PR Safety)

Summary

This PR safely removes 4 unscoped magic-shelf query functions (CountBooks, ListBooksForMagicShelf, EncodeAdvSearch, ValidOperator), a dead admin users list handler, the internal/version package, and the startupHooks machinery. It adds 6 new coverage tests covering branches in the surviving Scoped variants.

Verification Results

Deleted Symbol Caller Analysis — ALL ZERO PRODUCTION CALLERS ✓

• CountBooks (unscoped): Handler already uses CountBooksScoped(ContentRestrictions). No prod callers. ✓
• ListBooksForMagicShelf (unscoped): No production callers found. ✓
• EncodeAdvSearch: No production callers found (only comment reference in docstring of DecodeAdvSearch). ✓
• ValidOperator: No production callers found (only documentation reference in ValidFieldOperator comment). ✓
• adminUsersListHandler + parseListParams + buildUserListRows: No production callers. ✓
• startupHooks field + loop: No production references found in app.go after deletion. ✓
• internal/version package: No production imports found. ✓

Preserved Symbols — CORRECTLY RETAINED ✓

• AdminUserListRow: Actively used in internal/settings/shell_handler.go for admin users enriched with permission counts. ✓

Coverage Exclusions — CORRECT ✓

• Makefile: ./internal/version/... removed from UNIT_PKGS line. ✓
• scripts/check-coverage.sh: ./internal/version/... removed from coverage unit-test list. ✓

New Coverage Tests — LEGIT (NOT PADDING) ✓

All 6 new tests in list_books_for_magic_shelf_scoped_test.go cover real branches previously untested:

1. clampBooksLimit: limit ≤ 0 — Asserts LIMIT 50 in captured query args. Covers defaultBooksLimit branch.
2. clampBooksLimit: limit > 200 — Asserts LIMIT 200 in captured query args. Covers maxBooksLimit cap.
3. buildMagicShelfQueryScoped: tr.Joins included — Asserts JOIN clause appears in SQL. Covers join concatenation.
4. buildMagicShelfQueryScoped: afterID > 0 — Asserts "id < ?" predicate + afterID arg. Covers cursor pagination.
5. scanMagicShelfBooks: rows.Err() propagates — Asserts error on row iteration failure. Covers error path.

All tests use black-box testing (invoke ListBooksForMagicShelfScoped via public interface, inspect captured SQL). Each asserts real behavior, not just line coverage.

Test File Organization — CLEAN ✓

• Old 203-line books_test.go fully deleted (tested unscoped functions). ✓
• Old 41-line operators_test.go fully deleted (tested ValidOperator). ✓
• 44 lines of EncodeAdvSearch tests deleted from advsearch_test.go. ✓
• 222-line admin_handler_test.go fully deleted (tested adminUsersListHandler). ✓
• 13-line version_suite_test.go + 22-line version_test.go fully deleted. ✓
• 6 new tests added to surviving list_books_for_magic_shelf_scoped_test.go. ✓

Test Exports — ACCURATE ✓

• internal/users/export_test.go: Removed mux.Handle("GET /admin/users", ...) route from ExportRegisterAdminHandlersForTest. Docstring updated to reflect that the list is now a 301 redirect in the Settings shell. ✓

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

The PR is safe for merge. All deletions have zero production callers, the coverage tests are legitimate and cover real branches, and the exclusion updates are correct and complete.

# Code Review: PR #1255 (Deletion PR Safety) ## Summary This PR safely removes 4 unscoped magic-shelf query functions (CountBooks, ListBooksForMagicShelf, EncodeAdvSearch, ValidOperator), a dead admin users list handler, the internal/version package, and the startupHooks machinery. It adds 6 new coverage tests covering branches in the surviving Scoped variants. ## Verification Results ### Deleted Symbol Caller Analysis — ALL ZERO PRODUCTION CALLERS ✓ - **CountBooks (unscoped)**: Handler already uses `CountBooksScoped(ContentRestrictions)`. No prod callers. ✓ - **ListBooksForMagicShelf (unscoped)**: No production callers found. ✓ - **EncodeAdvSearch**: No production callers found (only comment reference in docstring of DecodeAdvSearch). ✓ - **ValidOperator**: No production callers found (only documentation reference in ValidFieldOperator comment). ✓ - **adminUsersListHandler + parseListParams + buildUserListRows**: No production callers. ✓ - **startupHooks field + loop**: No production references found in app.go after deletion. ✓ - **internal/version package**: No production imports found. ✓ ### Preserved Symbols — CORRECTLY RETAINED ✓ - **AdminUserListRow**: Actively used in `internal/settings/shell_handler.go` for admin users enriched with permission counts. ✓ ### Coverage Exclusions — CORRECT ✓ - **Makefile**: `./internal/version/...` removed from UNIT_PKGS line. ✓ - **scripts/check-coverage.sh**: `./internal/version/...` removed from coverage unit-test list. ✓ ### New Coverage Tests — LEGIT (NOT PADDING) ✓ All 6 new tests in `list_books_for_magic_shelf_scoped_test.go` cover real branches previously untested: 1. **clampBooksLimit: limit ≤ 0** — Asserts LIMIT 50 in captured query args. Covers `defaultBooksLimit` branch. 2. **clampBooksLimit: limit > 200** — Asserts LIMIT 200 in captured query args. Covers `maxBooksLimit` cap. 3. **buildMagicShelfQueryScoped: tr.Joins included** — Asserts JOIN clause appears in SQL. Covers join concatenation. 4. **buildMagicShelfQueryScoped: afterID > 0** — Asserts "id < ?" predicate + afterID arg. Covers cursor pagination. 5. **scanMagicShelfBooks: rows.Err() propagates** — Asserts error on row iteration failure. Covers error path. All tests use black-box testing (invoke `ListBooksForMagicShelfScoped` via public interface, inspect captured SQL). Each asserts real behavior, not just line coverage. ### Test File Organization — CLEAN ✓ - Old 203-line `books_test.go` **fully deleted** (tested unscoped functions). ✓ - Old 41-line `operators_test.go` **fully deleted** (tested ValidOperator). ✓ - 44 lines of `EncodeAdvSearch` tests deleted from `advsearch_test.go`. ✓ - 222-line `admin_handler_test.go` **fully deleted** (tested adminUsersListHandler). ✓ - 13-line `version_suite_test.go` + 22-line `version_test.go` **fully deleted**. ✓ - 6 new tests added to surviving `list_books_for_magic_shelf_scoped_test.go`. ✓ ### Test Exports — ACCURATE ✓ - **internal/users/export_test.go**: Removed `mux.Handle("GET /admin/users", ...)` route from `ExportRegisterAdminHandlersForTest`. Docstring updated to reflect that the list is now a 301 redirect in the Settings shell. ✓ --- REVIEW VERDICT: **0 blocker, 0 major, 0 minor** The PR is safe for merge. All deletions have zero production callers, the coverage tests are legitimate and cover real branches, and the exclusion updates are correct and complete.

zombor commented 2026-07-27 02:31:00 +00:00

Author

Owner

Copy link

Security re-review — PR #1251 (concurrent-accept TOCTOU MAJOR)

Verified git diff origin/main...origin/bd-bookshelf-t582g.7.

The concurrent-accept clobbering MAJOR is CLOSED. Finalize now uses a non-clobbering os.Link (fails EEXIST when the dest name already exists — os.Link does not follow a symlink at the target and never write-through-clobbers) with a bounded re-uniquify retry (finalizeWithRetry, review_service.go:695-740). Two concurrent accepts of same-base-named proposals can no longer both land on the same path: the link is the atomic exclusive claim; the race loser gets EEXIST and advances to the next (N) suffix. Confirmed via internal/app/build_extended_deps.go:1806 wiring LinkFile: os.Link and the EEXIST-retry test (review_service_test.go:2051-2110).

Checklist against the four points:

1. Atomic/exclusive placement — YES. os.Link+EEXIST is the mutual-exclusion primitive; the winner's file is never overwritten.
2. No new symlink-follow / traversal — CLEAN. Temp is os.CreateTemp(destDir,...) inside the confined destDir; every link candidate is filepath.Join(destDir, base+"(N)"+ext) off a filepath.Base'd name with a numeric suffix. No separators, no .., no follow-through.
3. Temp perms — ACCEPTABLE. 0644 is applied only after content is copied+fsync+closed (stage.go:107-111); during the write window the file is 0600 in the library dir (not a world-writable tmp). 0644 read-only is the intended final library-file mode.
4. Residual TOCTOU — see MAJOR below (correctness, not a security escalation).

[MAJOR] internal/bookdrop/review_service.go:998 — DB records the pre-finalize filename; EEXIST-retry path leaves a dangling book_file reference
The TX records FileName: safeFileName / FileSubPath: fileSubPath (review_service.go:955-957), computed by uniquifyDestPath BEFORE the TX (line 876). finalizeWithRetry runs AFTER the commit (line 998) and, on EEXIST, advances the on-disk path to a different suffix (e.g. DB says novel.epub but the file links to novel (2).epub — exactly the path the EEXIST-retry test asserts, review_service_test.go:2106-2107). Result in the concurrent-race path: the committed book_file row points at a path that does not exist, and the actual file is orphaned on disk (a rescan may re-import it as a duplicate). This is the security fix trading a clobber for a dangling reference — the winner's data is safe (so no security escalation), but it is a real data-integrity regression on the very race this PR fixes. Fix: capture the final linked path/name returned from finalizeWithRetry and reconcile the book_file row (update file_name/file_sub_path) after finalize, or move the link before the TX and record the actually-claimed name. The existing test masks this by stubbing upsertBookFile as a no-op — add an assertion that the stored FileName equals the finally-linked name in the EEXIST-retry case.

[MINOR] internal/bookdrop/review_service.go:997 — LinkFile has no nil-default despite the doc claiming one
The doc comment (review_service.go:272) says "Defaults to os.Link when nil," but linkFn := p.LinkFile is passed straight into finalizeWithRetry with no if linkFn == nil { linkFn = os.Link } guard (contrast StatFile/MkdirAll, which are defaulted). Production wiring passes os.Link so this is not exploitable, but a nil would panic and the code/doc disagree. Add the nil-default to match the contract.

REVIEW VERDICT: 0 blocker, 1 major, 1 minor

## Security re-review — PR #1251 (concurrent-accept TOCTOU MAJOR) Verified `git diff origin/main...origin/bd-bookshelf-t582g.7`. **The concurrent-accept clobbering MAJOR is CLOSED.** Finalize now uses a non-clobbering `os.Link` (fails `EEXIST` when the dest name already exists — `os.Link` does not follow a symlink at the target and never write-through-clobbers) with a bounded re-uniquify retry (`finalizeWithRetry`, review_service.go:695-740). Two concurrent accepts of same-base-named proposals can no longer both land on the same path: the link is the atomic exclusive claim; the race loser gets `EEXIST` and advances to the next `(N)` suffix. Confirmed via `internal/app/build_extended_deps.go:1806` wiring `LinkFile: os.Link` and the EEXIST-retry test (review_service_test.go:2051-2110). Checklist against the four points: 1. Atomic/exclusive placement — YES. `os.Link`+EEXIST is the mutual-exclusion primitive; the winner's file is never overwritten. 2. No new symlink-follow / traversal — CLEAN. Temp is `os.CreateTemp(destDir,...)` inside the confined `destDir`; every link candidate is `filepath.Join(destDir, base+"(N)"+ext)` off a `filepath.Base`'d name with a numeric suffix. No separators, no `..`, no follow-through. 3. Temp perms — ACCEPTABLE. 0644 is applied only *after* content is copied+fsync+closed (stage.go:107-111); during the write window the file is 0600 in the library dir (not a world-writable tmp). 0644 read-only is the intended final library-file mode. 4. Residual TOCTOU — see MAJOR below (correctness, not a security escalation). [MAJOR] internal/bookdrop/review_service.go:998 — DB records the pre-finalize filename; EEXIST-retry path leaves a dangling book_file reference The TX records `FileName: safeFileName` / `FileSubPath: fileSubPath` (review_service.go:955-957), computed by `uniquifyDestPath` BEFORE the TX (line 876). `finalizeWithRetry` runs AFTER the commit (line 998) and, on `EEXIST`, advances the on-disk path to a different suffix (e.g. DB says `novel.epub` but the file links to `novel (2).epub` — exactly the path the EEXIST-retry test asserts, review_service_test.go:2106-2107). Result in the concurrent-race path: the committed `book_file` row points at a path that does not exist, and the actual file is orphaned on disk (a rescan may re-import it as a duplicate). This is the security fix trading a clobber for a dangling reference — the winner's data is safe (so no security escalation), but it is a real data-integrity regression on the very race this PR fixes. Fix: capture the final linked path/name returned from `finalizeWithRetry` and reconcile the `book_file` row (update `file_name`/`file_sub_path`) after finalize, or move the link before the TX and record the actually-claimed name. The existing test masks this by stubbing `upsertBookFile` as a no-op — add an assertion that the stored `FileName` equals the finally-linked name in the EEXIST-retry case. [MINOR] internal/bookdrop/review_service.go:997 — `LinkFile` has no nil-default despite the doc claiming one The doc comment (review_service.go:272) says "Defaults to os.Link when nil," but `linkFn := p.LinkFile` is passed straight into `finalizeWithRetry` with no `if linkFn == nil { linkFn = os.Link }` guard (contrast `StatFile`/`MkdirAll`, which are defaulted). Production wiring passes `os.Link` so this is not exploitable, but a nil would panic and the code/doc disagree. Add the nil-default to match the contract. REVIEW VERDICT: 0 blocker, 1 major, 1 minor

zombor commented 2026-07-27 02:31:26 +00:00

Author

Owner

Copy link

Security Review — PR #1255 (bd-bookshelf-9snmz.1.3)

Dead-code deletion: internal/version, dead admin-users list handler, startupHooks, and 4 unscoped magic-shelf query paths. Reviewed git diff origin/main...origin/bd-bookshelf-9snmz.1.3. This deletion is security-positive — it removes per-user-scoping footguns. All three focus questions check out.

Verification summary:

• (1) Unscoped magic-shelf paths — fully removed, no remaining caller. CountBooks, ListBooksForMagicShelf, buildMagicShelfCountQuery, buildMagicShelfQuery, EncodeAdvSearch, ValidOperator are all deleted. No qualified (magic.CountBooks() or in-package call remains. Production wiring in internal/shelves/magic/wire.go uses only the scoped variants: ListBooksForMagicShelfScoped (line 54) and CountBooksScoped (line 55). The handler Deps.CountBooks field is a different symbol (a struct field bound to CountBooksScoped), not the deleted package func — no collision. No unscoped footgun path survives. Surviving *Scoped funcs remain tested; go build ./internal/shelves/magic/... passes.
• (2) Admin-users list handler removal preserves the auth gate. GET /admin/users is not dropped — routes.go:70 now serves an adminRequired-wrapped 301 redirect to /settings/users. The write routes remain adminRequired-wrapped. AdminUserListRow + ListUsersPermissions are preserved and consumed by the live Settings Users tab (internal/settings/shell_handler.go:616). No permission gate lost.
• (3) No dangling references / build breaks. go build of internal/{shelves/magic,users,settings,app} all pass. internal/version fully removed from Makefile UNIT_PKGS and scripts/check-coverage.sh. startupHooks field + Run() loop removed cleanly with no remaining appender.

Findings

[MINOR] internal/users/admin_handler.go:30 (+ internal/users/wire.go:187) — dead AdminDeps.ListUsers field
The ListUsers field's only consumer (adminUsersListHandler) was deleted, but the field remains in the struct and is still populated in wire.go:187. It compiles (field assignment, not a call), so no build break and no footgun (the live Settings list path uses settings' own ListAdminUsers). Remove the now-unused AdminDeps.ListUsers field and its wire.go assignment to finish the cleanup.

[MINOR] templates/pages/admin_users.html — orphaned template
Its only production reference was the deleted adminUsersListHandler (remaining grep hit is a test file). The live users list now renders via the Settings shell template. Delete the orphaned template.

[MINOR] internal/shelves/magic/operators.go:142 — stale doc-comment reference to deleted ValidOperator
ValidFieldOperator's comment says "Unlike ValidOperator it consults…" but ValidOperator no longer exists. Reword to drop the dangling reference.

[MINOR] internal/app/app.go:779 — stale comment referencing deleted CountBooks
Comment reads "…guard produces 1=0 and CountBooks returns 0" but the unscoped CountBooks is deleted; the code path uses CountBooksScoped. Update the comment.

REVIEW VERDICT: 0 blocker, 0 major, 4 minor

## Security Review — PR #1255 (bd-bookshelf-9snmz.1.3) Dead-code deletion: `internal/version`, dead admin-users list handler, `startupHooks`, and 4 unscoped magic-shelf query paths. Reviewed `git diff origin/main...origin/bd-bookshelf-9snmz.1.3`. This deletion is **security-positive** — it removes per-user-scoping footguns. All three focus questions check out. **Verification summary:** - **(1) Unscoped magic-shelf paths — fully removed, no remaining caller.** `CountBooks`, `ListBooksForMagicShelf`, `buildMagicShelfCountQuery`, `buildMagicShelfQuery`, `EncodeAdvSearch`, `ValidOperator` are all deleted. No qualified (`magic.CountBooks(`) or in-package call remains. Production wiring in `internal/shelves/magic/wire.go` uses only the scoped variants: `ListBooksForMagicShelfScoped` (line 54) and `CountBooksScoped` (line 55). The handler `Deps.CountBooks` field is a different symbol (a struct field bound to `CountBooksScoped`), not the deleted package func — no collision. No unscoped footgun path survives. Surviving `*Scoped` funcs remain tested; `go build ./internal/shelves/magic/...` passes. - **(2) Admin-users list handler removal preserves the auth gate.** `GET /admin/users` is not dropped — `routes.go:70` now serves an `adminRequired`-wrapped 301 redirect to `/settings/users`. The write routes remain `adminRequired`-wrapped. `AdminUserListRow` + `ListUsersPermissions` are preserved and consumed by the live Settings Users tab (`internal/settings/shell_handler.go:616`). No permission gate lost. - **(3) No dangling references / build breaks.** `go build` of `internal/{shelves/magic,users,settings,app}` all pass. `internal/version` fully removed from `Makefile` UNIT_PKGS and `scripts/check-coverage.sh`. `startupHooks` field + Run() loop removed cleanly with no remaining appender. ### Findings [MINOR] internal/users/admin_handler.go:30 (+ internal/users/wire.go:187) — dead `AdminDeps.ListUsers` field The `ListUsers` field's only consumer (`adminUsersListHandler`) was deleted, but the field remains in the struct and is still populated in `wire.go:187`. It compiles (field assignment, not a call), so no build break and no footgun (the live Settings list path uses settings' own `ListAdminUsers`). Remove the now-unused `AdminDeps.ListUsers` field and its wire.go assignment to finish the cleanup. [MINOR] templates/pages/admin_users.html — orphaned template Its only production reference was the deleted `adminUsersListHandler` (remaining grep hit is a test file). The live users list now renders via the Settings shell template. Delete the orphaned template. [MINOR] internal/shelves/magic/operators.go:142 — stale doc-comment reference to deleted `ValidOperator` `ValidFieldOperator`'s comment says "Unlike ValidOperator it consults…" but `ValidOperator` no longer exists. Reword to drop the dangling reference. [MINOR] internal/app/app.go:779 — stale comment referencing deleted `CountBooks` Comment reads "…guard produces 1=0 and CountBooks returns 0" but the unscoped `CountBooks` is deleted; the code path uses `CountBooksScoped`. Update the comment. REVIEW VERDICT: 0 blocker, 0 major, 4 minor

zombor added 1 commit 2026-07-27 02:35:01 +00:00

refactor(consolidation): fold review minors — remove orphaned ListUsers field + template + stale comments ... 6a1f14e786

- users.AdminDeps.ListUsers field: deleted (no handler consumed it after
  adminUsersListHandler was removed; ListAdminUsers is still wired via
  settings.ShellDeps.ListAdminUsers for the live Settings Users tab)
- users/wire.go:187: stop populating AdminDeps.ListUsers
- admin_handler_test.go: remove ListUsers stub from test dep struct
- templates/pages/admin_users.html: deleted (orphaned; route is now a 301
  redirect and the handler that rendered it is gone)
- operators.go:142: remove "Unlike ValidOperator" from ValidFieldOperator
  doc comment (ValidOperator was deleted)
- app.go:779: fix stale comment "CountBooks" → "CountBooksScoped"

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor force-pushed bd-bookshelf-9snmz.1.3 from 6a1f14e786 to 6be16b0d9a 2026-07-27 12:27:48 +00:00 Compare

zombor merged commit 0eb8fb809b into main 2026-07-27 12:40:08 +00:00

zombor referenced this pull request from a commit 2026-07-27 12:40:11 +00:00

Merge pull request 'refactor(consolidation): delete dead packages/handlers + unscoped magic footguns (bookshelf-9snmz.1.3)' (#1255) from bd-bookshelf-9snmz.1.3 into main

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!1255

No description provided.