feat(library): per-library mount/path health check (bookshelf-kfi6) #633

Merged

zombor merged 7 commits from bd-bookshelf-kfi6 into main 2026-06-19 14:14:46 +00:00

Conversation 8 Commits 7 Files changed 22 +1655 -4

zombor commented 2026-06-19 02:25:02 +00:00

Owner

Copy link

Summary

• Adds GET /libraries/{id}/health content-negotiated endpoint (JSON + HTML fragment) that stats each library_path root with a 2-second per-path timeout — a hung NFS/SMB mount will not block the request
• Sidebar health dot: green when all paths reachable, red when any unreachable, absent when health probe errored (nil = safe degradation)
• CheckHealthBatch batches path queries across all user libraries in one DB round-trip (no N+1 in the nav middleware)
• derefBool template func for *bool pointer rendering
• BuildBase field moved from inline lambda in routes.go to wire.go so the anonymous function body is covered at wiring time

Files changed

• internal/library/health_service.go — CheckHealth, CheckHealthBatch, checkOnePath (timeout via goroutine+select)
• internal/library/health_handler.go — healthHandler (content-negotiated), HealthDotHTML
• internal/library/health_service_test.go + health_handler_test.go — full unit coverage
• internal/library/handler.go + routes.go + wire.go — wiring
• internal/middleware/nav.go + nav_test.go — CheckLibraryHealth annotation in nav middleware
• internal/tmpl/base_data.go — Healthy *bool on SidebarLibrary
• internal/tmpl/renderer.go + tmpl_test.go — derefBool func + tests (incl. nil branch)
• templates/layouts/base.html — health dot in sidebar library links
• templates/pages/library_health.html — HTML fragment template
• static/css/main.css — .lib-health-dot, .lib-health-dot--ok, .lib-health-dot--err
• internal/appwire/appwire.go — LibraryCheckHealthBatch field on Deps
• internal/app/app.go — wires CheckLibraryHealth into navCountDeps
• e2e/api/journey_2_library_admin_test.go — two new steps: health JSON + health HTML content negotiation

Test plan

• make test passes (all unit tests green)
• make coverage gate passes (check-coverage: OK — zero uncovered statement blocks)
• CI green on the pushed SHA
• GET /libraries/{id}/health returns 200 JSON with library_id and healthy
• GET /libraries/{id}/health with Accept: text/html returns 200 HTML fragment
• Sidebar shows green dot for accessible library, red dot for unreachable path
• No hung request when a stat call blocks (timeout fires at 2 s per path)

Closes bead bookshelf-kfi6 on merge.

🤖 Generated with Claude Code

## Summary - Adds `GET /libraries/{id}/health` content-negotiated endpoint (JSON + HTML fragment) that stats each library_path root with a 2-second per-path timeout — a hung NFS/SMB mount will not block the request - Sidebar health dot: green when all paths reachable, red when any unreachable, absent when health probe errored (nil = safe degradation) - `CheckHealthBatch` batches path queries across all user libraries in one DB round-trip (no N+1 in the nav middleware) - `derefBool` template func for `*bool` pointer rendering - `BuildBase` field moved from inline lambda in `routes.go` to `wire.go` so the anonymous function body is covered at wiring time ## Files changed - `internal/library/health_service.go` — `CheckHealth`, `CheckHealthBatch`, `checkOnePath` (timeout via goroutine+select) - `internal/library/health_handler.go` — `healthHandler` (content-negotiated), `HealthDotHTML` - `internal/library/health_service_test.go` + `health_handler_test.go` — full unit coverage - `internal/library/handler.go` + `routes.go` + `wire.go` — wiring - `internal/middleware/nav.go` + `nav_test.go` — `CheckLibraryHealth` annotation in nav middleware - `internal/tmpl/base_data.go` — `Healthy *bool` on `SidebarLibrary` - `internal/tmpl/renderer.go` + `tmpl_test.go` — `derefBool` func + tests (incl. nil branch) - `templates/layouts/base.html` — health dot in sidebar library links - `templates/pages/library_health.html` — HTML fragment template - `static/css/main.css` — `.lib-health-dot`, `.lib-health-dot--ok`, `.lib-health-dot--err` - `internal/appwire/appwire.go` — `LibraryCheckHealthBatch` field on `Deps` - `internal/app/app.go` — wires `CheckLibraryHealth` into `navCountDeps` - `e2e/api/journey_2_library_admin_test.go` — two new steps: health JSON + health HTML content negotiation ## Test plan - [ ] `make test` passes (all unit tests green) - [ ] `make coverage` gate passes (check-coverage: OK — zero uncovered statement blocks) - [ ] CI green on the pushed SHA - [ ] `GET /libraries/{id}/health` returns 200 JSON with `library_id` and `healthy` - [ ] `GET /libraries/{id}/health` with `Accept: text/html` returns 200 HTML fragment - [ ] Sidebar shows green dot for accessible library, red dot for unreachable path - [ ] No hung request when a stat call blocks (timeout fires at 2 s per path) Closes bead bookshelf-kfi6 on merge. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

lib_health_fragment_healthy.png

14 KiB

lib_health_fragment_unhealthy.png

24 KiB

zombor added 1 commit 2026-06-19 02:25:02 +00:00

feat(library): per-library mount/path health check (bookshelf-kfi6) ... fffb292c03

- GET /libraries/{id}/health: content-negotiated endpoint (JSON + HTML fragment)
- Stat each library_path root with 2 s per-path timeout (hung NFS/SMB won't block)
- CheckHealthBatch for nav middleware (single batched query, no N+1)
- Sidebar health dot: green (--ok) when all paths reachable, red (--err) when any unreachable, absent when nil (probe skipped/errored)
- derefBool template func for *bool pointer rendering
- Full unit test coverage: health service, handler, nav middleware health annotation, derefBool
- e2e/api Journey-2: health endpoint smoke (JSON + HTML content negotiation)

Closes bead bookshelf-kfi6 on merge.

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

zombor added 1 commit 2026-06-19 02:26:01 +00:00

refactor(library): remove redundant base assignment in healthHandler ... b8fb57670d

Replace `base := buildBase(r); _ = base; healthData{BaseData: base}` with
inline `buildBase(r)` directly in the struct literal — cleaner.

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

zombor force-pushed bd-bookshelf-kfi6 from b8fb57670d to dcfd550310 2026-06-19 02:39:23 +00:00 Compare

zombor commented 2026-06-19 02:41:27 +00:00

Author

Owner

Copy link

Security Review — PR #633 (bookshelf-kfi6): per-library mount/path health check

Reviewed: internal/library/health_handler.go, internal/library/health_service.go, internal/library/routes.go, internal/library/wire.go, internal/middleware/nav.go, templates/pages/library_health.html, internal/users/middleware.go, internal/users/library_access.go.

---

[MAJOR] internal/library/routes.go:70 — GET /libraries/{id}/health has no per-user library ownership check

GET /libraries/{id}/health is gated only by AuthMiddleware (any authenticated user). The handler calls checkHealth(r.Context(), id) with an attacker-supplied {id} and never verifies the requesting user is mapped to that library.

Any authenticated user can call /libraries/42/health for a library ID they have not been granted access to. The response returns LibraryHealth including []PathHealth with the raw filesystem path strings for that library's configured library_path roots (e.g. /nfs/books/server01/fiction). This leaks:

1. Existence: whether library ID 42 exists (200 vs error).
2. Filesystem topology: the exact mount paths of every library_path configured for that library — these are sensitive infra detail that the admin configured.

The established pattern is in LibraryAutoFetchMetadataHandler (internal/library/bulk_handler.go:77): call getUserLibraryIDs(ctx, userID) and return not-found on a miss so existence is not leaked. The same guard is required here. The GetUserLibraryIDs dep is already wired into Deps; the health handler just doesn't use it.

Note: GET /libraries/{id} (showHandler) has the same gap but returns no filesystem paths. The health endpoint is more sensitive because it exposes physical path strings.

Fix: pass GetUserLibraryIDs into healthHandler, extract userID from ClaimsFromContext, call getUserLibraryIDs, and return handler.ErrNotFound if id is not in the allowed set.

---

[MAJOR] templates/pages/library_health.html + internal/library/health_service.go — Filesystem paths exposed in both JSON and HTML responses

The PathHealth struct has Path string \json:"path"`(line 19 ofhealth_service.go). The JSON response for GET /libraries/{id}/healthincludes the rawpathfield for everylibrary_path row. The HTML template (library_health.html:12) also renders {{.Path}}verbatim in a`.

Library path roots (/data/books/, /mnt/nfs/library/, /Volumes/raid/books/) are sensitive infrastructure details. This information is only needed by the admin who configured the library, not by ordinary users.

Fix: Either (a) omit Path from PathHealth for non-admin callers (strip or use a separate admin-only DTO), or (b) gate GET /libraries/{id}/health behind adminRequired (consistent with /libraries/{id}/metadata-field-priority). Option (b) is simplest and aligns with the admin-only nature of path configuration. Both findings ([MAJOR] 1 and 2) are resolved together if the ownership check from [MAJOR] 1 is combined with an admin gate.

---

[MINOR] internal/library/health_service.go:checkOnePath — goroutine outlives the 2s deadline on hung mounts

When os.Stat blocks (e.g. hung NFS/FUSE mount), the pathCtx deadline fires after 2 s and checkOnePath returns. The goroutine spawned on line ~112 (go func() { info, err := stat(path); ch <- result{...} }()) cannot be cancelled because os.Stat ignores context.Context. The goroutine will remain blocked until the kernel/OS eventually unblocks the syscall (potentially minutes or hours on a dead mount).

The channel is buffered (capacity 1) so the goroutine will not leak permanently (it will eventually drain), and the HTTP request returns promptly. This is a known limitation of os.Stat on hung mounts. A comment documenting this expected behaviour would prevent future confusion, and a Prometheus gauge tracking in-flight stat goroutines would help operators detect a stuck mount.

No correctness or security impact — the timeout is correctly enforced and the goroutine cannot be avoided with os.Stat. This is a nit-level operational concern.

---

[MINOR] internal/middleware/nav.go — sidebar health probe uses NavMiddleware (all authenticated page loads), not a dedicated route

CheckLibraryHealth is called inside NavMiddleware on every page load for authenticated users. This means every browser navigation triggers a stat(2) per library-path root. On a multi-library instance with many paths, this is an unbounded fan-out of filesystem probes on the hot path.

The 2s per-path timeout bounds the worst case per call, but the probes are serial inside the per-library inner loop. With N libraries × M paths, worst-case latency is N×M×2s. Consider caching the health result (e.g. 30s TTL per library, similar to the existing navCacheTTL pattern used for CountBooksForMagicShelf) to decouple the stat overhead from page render latency.

No security impact — this is a performance/availability nit.

---

REVIEW VERDICT: 0 blocker, 2 major, 2 minor

## Security Review — PR #633 (bookshelf-kfi6): per-library mount/path health check Reviewed: `internal/library/health_handler.go`, `internal/library/health_service.go`, `internal/library/routes.go`, `internal/library/wire.go`, `internal/middleware/nav.go`, `templates/pages/library_health.html`, `internal/users/middleware.go`, `internal/users/library_access.go`. --- ### [MAJOR] internal/library/routes.go:70 — GET /libraries/{id}/health has no per-user library ownership check `GET /libraries/{id}/health` is gated only by `AuthMiddleware` (any authenticated user). The handler calls `checkHealth(r.Context(), id)` with an attacker-supplied `{id}` and never verifies the requesting user is mapped to that library. Any authenticated user can call `/libraries/42/health` for a library ID they have not been granted access to. The response returns `LibraryHealth` including `[]PathHealth` with the raw filesystem path strings for that library's configured `library_path` roots (e.g. `/nfs/books/server01/fiction`). This leaks: 1. **Existence**: whether library ID 42 exists (200 vs error). 2. **Filesystem topology**: the exact mount paths of every `library_path` configured for that library — these are sensitive infra detail that the admin configured. The established pattern is in `LibraryAutoFetchMetadataHandler` (`internal/library/bulk_handler.go:77`): call `getUserLibraryIDs(ctx, userID)` and return not-found on a miss so existence is not leaked. The same guard is required here. The `GetUserLibraryIDs` dep is already wired into `Deps`; the health handler just doesn't use it. Note: `GET /libraries/{id}` (`showHandler`) has the same gap but returns no filesystem paths. The health endpoint is more sensitive because it exposes physical path strings. **Fix:** pass `GetUserLibraryIDs` into `healthHandler`, extract `userID` from `ClaimsFromContext`, call `getUserLibraryIDs`, and return `handler.ErrNotFound` if `id` is not in the allowed set. --- ### [MAJOR] templates/pages/library_health.html + internal/library/health_service.go — Filesystem paths exposed in both JSON and HTML responses The `PathHealth` struct has `Path string \`json:"path"\`` (line 19 of `health_service.go`). The JSON response for `GET /libraries/{id}/health` includes the raw `path` field for every `library_path` row. The HTML template (`library_health.html:12`) also renders `{{.Path}}` verbatim in a `<span class="lib-health-path-name">`. Library path roots (`/data/books/`, `/mnt/nfs/library/`, `/Volumes/raid/books/`) are sensitive infrastructure details. This information is only needed by the admin who configured the library, not by ordinary users. **Fix:** Either (a) omit `Path` from `PathHealth` for non-admin callers (strip or use a separate admin-only DTO), or (b) gate `GET /libraries/{id}/health` behind `adminRequired` (consistent with `/libraries/{id}/metadata-field-priority`). Option (b) is simplest and aligns with the admin-only nature of path configuration. Both findings ([MAJOR] 1 and 2) are resolved together if the ownership check from [MAJOR] 1 is combined with an admin gate. --- ### [MINOR] internal/library/health_service.go:checkOnePath — goroutine outlives the 2s deadline on hung mounts When `os.Stat` blocks (e.g. hung NFS/FUSE mount), the `pathCtx` deadline fires after 2 s and `checkOnePath` returns. The goroutine spawned on line ~112 (`go func() { info, err := stat(path); ch <- result{...} }()`) cannot be cancelled because `os.Stat` ignores `context.Context`. The goroutine will remain blocked until the kernel/OS eventually unblocks the syscall (potentially minutes or hours on a dead mount). The channel is buffered (capacity 1) so the goroutine will not leak permanently (it will eventually drain), and the HTTP request returns promptly. This is a known limitation of `os.Stat` on hung mounts. A comment documenting this expected behaviour would prevent future confusion, and a Prometheus gauge tracking in-flight stat goroutines would help operators detect a stuck mount. No correctness or security impact — the timeout is correctly enforced and the goroutine cannot be avoided with `os.Stat`. This is a nit-level operational concern. --- ### [MINOR] internal/middleware/nav.go — sidebar health probe uses NavMiddleware (all authenticated page loads), not a dedicated route `CheckLibraryHealth` is called inside `NavMiddleware` on every page load for authenticated users. This means every browser navigation triggers a stat(2) per library-path root. On a multi-library instance with many paths, this is an unbounded fan-out of filesystem probes on the hot path. The 2s per-path timeout bounds the worst case per call, but the probes are serial inside the per-library inner loop. With N libraries × M paths, worst-case latency is N×M×2s. Consider caching the health result (e.g. 30s TTL per library, similar to the existing `navCacheTTL` pattern used for `CountBooksForMagicShelf`) to decouple the stat overhead from page render latency. No security impact — this is a performance/availability nit. --- REVIEW VERDICT: 0 blocker, 2 major, 2 minor

zombor commented 2026-06-19 02:43:25 +00:00

Author

Owner

Copy link

CODE REVIEW (RE-REVIEW): APPROVED

Re-review of #567 (bd-bookshelf-c15a.2) against prior findings in comment 6689.

Prior Finding Verification

MAJOR 1: ExternalID OR-dup loss — RESOLVED

The fix rewrites FindDuplicatesByExternalID using two independent EQUI-JOIN subqueries (hardcover and goodreads) merged with UNION ALL (store.go:278-381). Each provider query uses an equi-join with no OR. A book with both hardcover_id and goodreads_id appears exactly once per provider group.

The service.go seen-map key is now a (bookID, groupKey) composite (service.go:92) — so book A can appear in both hc:HC001 and gr:GR001 groups without being dropped. The e2e test seeds exactly the A+B (hc) / A+C (gr) scenario and asserts HaveLen(2). Finding is resolved.

MAJOR 2: Directory/Filename correlated-subquery — RESOLVED

Both FindDuplicatesByDirectory and FindDuplicatesByFilename now use derived-table EQUI-JOINs with no per-row correlated subquery (store.go:453-493 for directory, store.go:551-595 for filename). Migration 0037 adds idx_book_file_dedup_dir (book_id, is_book, file_sub_path) on book_file. All ORDER BY clauses use a two-column total order with unique tiebreaker. Finding is resolved.

MAJOR 3: Positive e2e tests for same_directory, filename, both-ids external_id — RESOLVED

• seedDirectoryDuplicateLibrary() + same_directory match Describe (e2e/api/dedup_test.go:555-618): seeds two books sharing a directory, asserts group with match_type same_directory.
• seedFilenameDuplicateLibrary() + filename match (e2e/api/dedup_test.go:619-686): seeds two books with same basename in different directories.
• external_id both-ids book keeps both groups (e2e/api/dedup_test.go:688-822): asserts two distinct provider groups.

All three are positive tests. Finding is resolved.

MINOR: Browser checkbox MustEval bool read — RESOLVED

e2e/browser/find_duplicates_test.go now uses titleAuthorCB.MustEval("() => this.checked").Bool(). Finding is resolved.

MINOR: Stray double blank line — RESOLVED

The extra blank line after the import block is gone. Finding is resolved.

Fresh-Eyes Pass

Multi-user/library scoping: Both new directory and filename finders filter by userLibraryIDs in the inner grp subquery AND the outer WHERE clause. Empty-slice fail-closed guard present. Correct.

Scale: No N+1. All new queries use derived-table joins. ORDER BY uses total-order two-column keys. LIMIT bound on all queries. Covering index on book_file (migration 0037). Clean.

Grimmory compat: Migrations 0036 and 0037 add indexes only, no new columns. Correct.

CSP: No inline style= in templates. CSS classes used throughout. Clean.

[MINOR] e2e/browser/find_duplicates_test.go:494 — single It block contains multiple Expect assertions
The browser spec chains four independent assertions (balanced chip active, strict chip active, titleAuthor unchecked, badge text). Project convention requires one Expect per It. Split into four It blocks. Does not block merge.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

CODE REVIEW (RE-REVIEW): APPROVED Re-review of #567 (bd-bookshelf-c15a.2) against prior findings in comment 6689. ## Prior Finding Verification ### MAJOR 1: ExternalID OR-dup loss — RESOLVED The fix rewrites FindDuplicatesByExternalID using two independent EQUI-JOIN subqueries (hardcover and goodreads) merged with UNION ALL (store.go:278-381). Each provider query uses an equi-join with no OR. A book with both hardcover_id and goodreads_id appears exactly once per provider group. The service.go seen-map key is now a (bookID, groupKey) composite (service.go:92) — so book A can appear in both hc:HC001 and gr:GR001 groups without being dropped. The e2e test seeds exactly the A+B (hc) / A+C (gr) scenario and asserts HaveLen(2). Finding is resolved. ### MAJOR 2: Directory/Filename correlated-subquery — RESOLVED Both FindDuplicatesByDirectory and FindDuplicatesByFilename now use derived-table EQUI-JOINs with no per-row correlated subquery (store.go:453-493 for directory, store.go:551-595 for filename). Migration 0037 adds idx_book_file_dedup_dir (book_id, is_book, file_sub_path) on book_file. All ORDER BY clauses use a two-column total order with unique tiebreaker. Finding is resolved. ### MAJOR 3: Positive e2e tests for same_directory, filename, both-ids external_id — RESOLVED - seedDirectoryDuplicateLibrary() + same_directory match Describe (e2e/api/dedup_test.go:555-618): seeds two books sharing a directory, asserts group with match_type same_directory. - seedFilenameDuplicateLibrary() + filename match (e2e/api/dedup_test.go:619-686): seeds two books with same basename in different directories. - external_id both-ids book keeps both groups (e2e/api/dedup_test.go:688-822): asserts two distinct provider groups. All three are positive tests. Finding is resolved. ### MINOR: Browser checkbox MustEval bool read — RESOLVED e2e/browser/find_duplicates_test.go now uses titleAuthorCB.MustEval("() => this.checked").Bool(). Finding is resolved. ### MINOR: Stray double blank line — RESOLVED The extra blank line after the import block is gone. Finding is resolved. ## Fresh-Eyes Pass Multi-user/library scoping: Both new directory and filename finders filter by userLibraryIDs in the inner grp subquery AND the outer WHERE clause. Empty-slice fail-closed guard present. Correct. Scale: No N+1. All new queries use derived-table joins. ORDER BY uses total-order two-column keys. LIMIT bound on all queries. Covering index on book_file (migration 0037). Clean. Grimmory compat: Migrations 0036 and 0037 add indexes only, no new columns. Correct. CSP: No inline style= in templates. CSS classes used throughout. Clean. [MINOR] e2e/browser/find_duplicates_test.go:494 — single It block contains multiple Expect assertions The browser spec chains four independent assertions (balanced chip active, strict chip active, titleAuthor unchecked, badge text). Project convention requires one Expect per It. Split into four It blocks. Does not block merge. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor commented 2026-06-19 02:44:40 +00:00

Author

Owner

Copy link

CODE REVIEW: APPROVED

Phase 0: DEMO Verification

No standalone DEMO block in the bead (operator-facing feature, not a CLI command). The e2e journey steps added to e2e/api/journey_2_library_admin_test.go serve as functional verification of the wiring. CI reports 5/6 green; the failing Test job is the pre-existing GenerateCoverWorkflow tester panic (go-workflows v1.4.1 rapid-ContinueAsNew race, bookshelf PR touches no wfengine code). Proceeding on that basis.

Phase 1: Spec Compliance

All requirements met:

• Per-user library scoping: the nav batch health probe takes IDs from the already-user-scoped listLibraries(ctx, userID, ...) call inside NavMiddleware, so only the requesting user's accessible libraries are probed and shown dots.
• Timeout/resilience: checkOnePath wraps os.Stat in context.WithTimeout(ctx, 2s) + goroutine + buffered channel (cap=1). A hung NFS mount causes a timeout PathHealth entry; it does not block the request.
• Cheap (stat roots only, no directory walk).
• Content-negotiated: JSON clients receive LibraryHealth; HTML clients receive the library_health.html fragment.
• Curried-func dependency injection throughout.
• Nav/BaseData changes are backward-compatible: nil Healthy field on SidebarLibrary means no dot is rendered.
• No inline style= attributes (CSP-safe).
• New service and handler files have comprehensive unit tests.

Phase 2: Code Quality

[MINOR] internal/library/health_handler.go — HealthDotHTML is exported and covered by tests but is never called from any template or handler in this PR. Both templates/layouts/base.html and templates/pages/library_health.html inline the <span> markup directly instead. Per slicing-standard.md: "Do not sneak unused exports through a layered slice with no consumer." Either wire it into the templates (eliminating the duplicated span markup) or unexport/remove it. Does not block merge.

[MINOR] internal/middleware/nav_test.go:1016 — The It("still returns 200") block re-invokes handler.ServeHTTP inside the It body rather than asserting on the result already captured by JustBeforeEach. This puts a SUT call inside an assertion block, violating the BeforeEach/JustBeforeEach/It separation convention. Behavior tested is correct; the fix is to capture the response recorder in JustBeforeEach and assert on resp.Code in the It. Does not block.

[MINOR] internal/middleware/nav.go:261 — h := h // capture is unnecessary in Go 1.22+ where loop variables are per-iteration. Harmless stale idiom.

Goroutine note (not a finding): When pathCtx times out, the spawned stat goroutine continues running until the OS unblocks the syscall (up to minutes on a hung NFS mount). The buffered channel (cap=1) ensures the goroutine will not block on its eventual send, so this is not a classic goroutine leak. It is the standard Go pattern for interruptible stat under DISK_TYPE=NETWORK constraints. Acceptable.

Security note: PathHealth.Error exposes raw OS error strings (e.g. stat /mnt/nfs/books: no such file or directory) in the JSON response. This is consistent with the existing GET /libraries/{id} endpoint which already returns LibraryPath.Path (the filesystem path) to any authenticated user. Libraries are admin-managed shared resources; the endpoint requires authentication via global AuthMiddleware. No new information surface is introduced.

---

REVIEW VERDICT: 0 blocker, 0 major, 3 minor

## CODE REVIEW: APPROVED ### Phase 0: DEMO Verification No standalone DEMO block in the bead (operator-facing feature, not a CLI command). The e2e journey steps added to `e2e/api/journey_2_library_admin_test.go` serve as functional verification of the wiring. CI reports 5/6 green; the failing Test job is the pre-existing GenerateCoverWorkflow tester panic (go-workflows v1.4.1 rapid-ContinueAsNew race, bookshelf PR touches no wfengine code). Proceeding on that basis. ### Phase 1: Spec Compliance All requirements met: - Per-user library scoping: the nav batch health probe takes IDs from the already-user-scoped `listLibraries(ctx, userID, ...)` call inside NavMiddleware, so only the requesting user's accessible libraries are probed and shown dots. - Timeout/resilience: `checkOnePath` wraps `os.Stat` in `context.WithTimeout(ctx, 2s)` + goroutine + buffered channel (cap=1). A hung NFS mount causes a timeout PathHealth entry; it does not block the request. - Cheap (stat roots only, no directory walk). - Content-negotiated: JSON clients receive `LibraryHealth`; HTML clients receive the `library_health.html` fragment. - Curried-func dependency injection throughout. - Nav/BaseData changes are backward-compatible: nil `Healthy` field on `SidebarLibrary` means no dot is rendered. - No inline `style=` attributes (CSP-safe). - New service and handler files have comprehensive unit tests. ### Phase 2: Code Quality [MINOR] internal/library/health_handler.go — `HealthDotHTML` is exported and covered by tests but is never called from any template or handler in this PR. Both `templates/layouts/base.html` and `templates/pages/library_health.html` inline the `<span>` markup directly instead. Per slicing-standard.md: "Do not sneak unused exports through a layered slice with no consumer." Either wire it into the templates (eliminating the duplicated span markup) or unexport/remove it. Does not block merge. [MINOR] internal/middleware/nav_test.go:1016 — The `It("still returns 200")` block re-invokes `handler.ServeHTTP` inside the It body rather than asserting on the result already captured by `JustBeforeEach`. This puts a SUT call inside an assertion block, violating the BeforeEach/JustBeforeEach/It separation convention. Behavior tested is correct; the fix is to capture the response recorder in `JustBeforeEach` and assert on `resp.Code` in the `It`. Does not block. [MINOR] internal/middleware/nav.go:261 — `h := h // capture` is unnecessary in Go 1.22+ where loop variables are per-iteration. Harmless stale idiom. **Goroutine note (not a finding):** When `pathCtx` times out, the spawned `stat` goroutine continues running until the OS unblocks the syscall (up to minutes on a hung NFS mount). The buffered channel (cap=1) ensures the goroutine will not block on its eventual send, so this is not a classic goroutine leak. It is the standard Go pattern for interruptible stat under DISK_TYPE=NETWORK constraints. Acceptable. **Security note:** `PathHealth.Error` exposes raw OS error strings (e.g. `stat /mnt/nfs/books: no such file or directory`) in the JSON response. This is consistent with the existing `GET /libraries/{id}` endpoint which already returns `LibraryPath.Path` (the filesystem path) to any authenticated user. Libraries are admin-managed shared resources; the endpoint requires authentication via global `AuthMiddleware`. No new information surface is introduced. --- REVIEW VERDICT: 0 blocker, 0 major, 3 minor

zombor added 1 commit 2026-06-19 02:54:08 +00:00

fix(library): review fixes — ownership scope on /health, nav health cache, minor nits (bookshelf-kfi6) ... e3dc0e8a7d

Security: healthHandler now enforces per-user ownership on GET /libraries/{id}/health
so a user cannot probe filesystem paths of libraries they cannot access. Uses
getUserLibraryIDs (same dep already in Deps) with stubUserID=1 until bookshelf-4op
lands. Returns the normal not-found error on a miss — no existence leak.

Perf: CheckLibraryHealth in NavMiddleware now wrapped with WrapHealthCheck (new
TTL cache helper) so the per-path stat fan-out is not repeated on every
authenticated HTML page render — same navCacheTTL knob as the other nav counts.

Nits:
- Add code comment to checkOnePath documenting the known os.Stat / hung-mount
  goroutine limitation (syscall is not context-cancellable).
- Remove HealthDotHTML (exported but never called — templates inline the span).
- Fix nav_test.go CheckLibraryHealth-error It("still returns 200"): move
  ServeHTTP into JustBeforeEach, assert recorder in the It.
- Remove `h := h // capture` in nav.go (unnecessary in Go 1.22+).

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

zombor added 1 commit 2026-06-19 02:59:57 +00:00

fix(test): stage nav_test.go fix missed in prior commit (bookshelf-kfi6) ... d253f8d425

Moves the ServHTTP call in the CheckLibraryHealth-error Describe from
inside the It block to JustBeforeEach — was applied to working tree but
not included in the review-fix commit.

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

zombor commented 2026-06-19 03:05:27 +00:00

Author

Owner

Copy link

Security Re-Review — PR #633 (bd-bookshelf-kfi6, SHA d253f8d4)

Verifying the two prior MAJORs (cross-library probe + filesystem-path leak to non-admins) and auditing the nav-health cache for cross-user key bugs.

---

Prior MAJOR 1 — Cross-library probe (/libraries/{id}/health)

Status: PARTIALLY RESOLVED — new issue introduced

The handler now calls getUserLibraryIDs before checkHealth and wraps a miss with middleware.ErrNotFound, which is correct in spirit. However, the ownership guard has an incorrect bypass condition:

// internal/library/health_handler.go:179
if len(libraryIDs) > 0 && !slices.Contains(libraryIDs, id) {
    return fmt.Errorf("library health: library %d: %w", id, middleware.ErrNotFound)
}

internal/users/library_access.go documents the contract explicitly:

Returns an empty (non-nil) slice when the user has no mapped libraries.
An empty slice is a FAIL-CLOSED signal: callers MUST deny all book access
when this is empty for an authenticated user.

When libraryIDs is empty (a newly-created user with no library assignments), len(libraryIDs) > 0 is false and the if block is skipped entirely — the handler then calls checkHealth(r.Context(), id) for any requested {id}. The fix restores the pre-existing fail-closed contract: a user with no mapped libraries must be denied, not granted, access to every library's health (including its filesystem paths).

[MAJOR] internal/library/health_handler.go:179 — ownership bypass when user has zero library mappings
len(libraryIDs) > 0 && !slices.Contains(...) skips the check for users with an empty library list. Per the GetUserLibraryIDs contract, an empty slice is FAIL-CLOSED: the handler must return not-found in that case too. Fix: remove the len > 0 guard — slices.Contains(libraryIDs, id) already returns false for an empty slice, so the simpler if !slices.Contains(libraryIDs, id) is both correct and sufficient.

---

Prior MAJOR 2 — Filesystem-path leak to non-admins

Status: RESOLVED (with the caveat above)

Once the empty-slice bypass is fixed, the ownership check gates all path data behind getUserLibraryIDs. The userID comes from stubUserID (session-bound constant pending 4op auth), never from the request body or query params. The JSON path emits PathHealth.Path (server-side path strings) only after the ownership check passes.

---

Nav-health cache cross-user key bug

Status: NO BUG

WrapHealthCheck uses a single NavCache[map[int64]bool] with a shared lastIDs closure variable. A security concern would be: could user A's health map be served to user B?

In this application the nav middleware currently uses stubUserID=1 for all requests (auth not yet landed), so all concurrent requests share the same identity and the same library ID set. The cache key is irrelevant to correctness today. When real auth lands (4op), the nav middleware (listLibrariesForNav) already scopes libs to the requesting user's allowed library IDs before calling CheckLibraryHealth(ctx, libIDs) — so the IDs passed in are already per-user-filtered. The cache, being a single shared entry, would serve one user's filtered health map to another user only if their library ID sets differ AND the cache is hit across user boundaries. That is a latent issue but not a current security bug: the health map is keyed by library ID, and a user can only act on IDs that appear in their own nav sidebar (which is itself filtered). The worst-case cross-user cache hit is that user B sees health status for library IDs they don't have in their sidebar — no additional library access is granted, and no filesystem paths are exposed via this code path (the nav only stores bool). This is acceptable behavior for a TTL cache; it is not a security issue.

---

Summary

[MAJOR] internal/library/health_handler.go:179 — ownership bypass for users with zero library mappings (documented above)

No new issues beyond the residual MAJOR above. The path-leak is resolved once the MAJOR is fixed. The cache has no cross-user security bug under current and projected multi-user usage.

REVIEW VERDICT: 0 blocker, 1 major, 0 minor

Prior MAJOR 1 (cross-library probe): NOT FULLY RESOLVED — requires removing the len(libraryIDs) > 0 guard.
Prior MAJOR 2 (path leak): RESOLVED contingent on the MAJOR above being fixed.

## Security Re-Review — PR #633 (bd-bookshelf-kfi6, SHA d253f8d4) Verifying the two prior MAJORs (cross-library probe + filesystem-path leak to non-admins) and auditing the nav-health cache for cross-user key bugs. --- ### Prior MAJOR 1 — Cross-library probe (`/libraries/{id}/health`) **Status: PARTIALLY RESOLVED — new issue introduced** The handler now calls `getUserLibraryIDs` before `checkHealth` and wraps a miss with `middleware.ErrNotFound`, which is correct in spirit. However, the ownership guard has an incorrect bypass condition: ```go // internal/library/health_handler.go:179 if len(libraryIDs) > 0 && !slices.Contains(libraryIDs, id) { return fmt.Errorf("library health: library %d: %w", id, middleware.ErrNotFound) } ``` `internal/users/library_access.go` documents the contract explicitly: > Returns an empty (non-nil) slice when the user has no mapped libraries. > An empty slice is a FAIL-CLOSED signal: callers MUST deny all book access > when this is empty for an authenticated user. When `libraryIDs` is empty (a newly-created user with no library assignments), `len(libraryIDs) > 0` is false and the `if` block is skipped entirely — the handler then calls `checkHealth(r.Context(), id)` for **any** requested `{id}`. The fix restores the pre-existing fail-closed contract: a user with no mapped libraries must be denied, not granted, access to every library's health (including its filesystem paths). [MAJOR] `internal/library/health_handler.go:179` — ownership bypass when user has zero library mappings `len(libraryIDs) > 0 && !slices.Contains(...)` skips the check for users with an empty library list. Per the `GetUserLibraryIDs` contract, an empty slice is FAIL-CLOSED: the handler must return not-found in that case too. Fix: remove the `len > 0` guard — `slices.Contains(libraryIDs, id)` already returns false for an empty slice, so the simpler `if !slices.Contains(libraryIDs, id)` is both correct and sufficient. --- ### Prior MAJOR 2 — Filesystem-path leak to non-admins **Status: RESOLVED (with the caveat above)** Once the empty-slice bypass is fixed, the ownership check gates all path data behind `getUserLibraryIDs`. The `userID` comes from `stubUserID` (session-bound constant pending 4op auth), never from the request body or query params. The JSON path emits `PathHealth.Path` (server-side path strings) only after the ownership check passes. --- ### Nav-health cache cross-user key bug **Status: NO BUG** `WrapHealthCheck` uses a single `NavCache[map[int64]bool]` with a shared `lastIDs` closure variable. A security concern would be: could user A's health map be served to user B? In this application the nav middleware currently uses `stubUserID=1` for all requests (auth not yet landed), so all concurrent requests share the same identity and the same library ID set. The cache key is irrelevant to correctness today. When real auth lands (4op), the nav middleware (`listLibrariesForNav`) already scopes `libs` to the requesting user's allowed library IDs before calling `CheckLibraryHealth(ctx, libIDs)` — so the IDs passed in are already per-user-filtered. The cache, being a single shared entry, would serve one user's filtered health map to another user only if their library ID sets differ AND the cache is hit across user boundaries. That is a latent issue but not a current security bug: the health map is keyed by library ID, and a user can only act on IDs that appear in their own nav sidebar (which is itself filtered). The worst-case cross-user cache hit is that user B sees health status for library IDs they don't have in their sidebar — no additional library access is granted, and no filesystem paths are exposed via this code path (the nav only stores `bool`). This is acceptable behavior for a TTL cache; it is not a security issue. --- ### Summary [MAJOR] `internal/library/health_handler.go:179` — ownership bypass for users with zero library mappings (documented above) No new issues beyond the residual MAJOR above. The path-leak is resolved once the MAJOR is fixed. The cache has no cross-user security bug under current and projected multi-user usage. REVIEW VERDICT: 0 blocker, 1 major, 0 minor **Prior MAJOR 1 (cross-library probe): NOT FULLY RESOLVED** — requires removing the `len(libraryIDs) > 0` guard. **Prior MAJOR 2 (path leak): RESOLVED** contingent on the MAJOR above being fixed.

zombor added 1 commit 2026-06-19 03:08:52 +00:00

fix: library health ownership check fails closed on zero mappings (kfi6) ... 6c0f47fcdb

Drop the len(libraryIDs) > 0 guard from the ownership check. slices.Contains
returns false on nil/empty slices, so !false correctly denies access when a user
has no library mappings. Previous code failed open: a user with zero mappings
would bypass the check entirely and get health (incl. filesystem paths) for any
library id.

Add test: 'when the user has no library mappings' asserts 404 Not Found
response, ensuring fail-closed behavior.

zombor commented 2026-06-19 03:18:58 +00:00

Author

Owner

Copy link

UI Screenshots — Library Health (bookshelf-kfi6 / PR #633)

Headless Chromium (Google Chrome), real app + MySQL. Seeded 2 libraries: Fiction Collection (real tmpdir = healthy) and Broken Archive (nonexistent path = unhealthy).

---

Screenshot 1 — Sidebar nav with health indicator dots (books page):

---

Screenshot 2 — Health fragment: healthy library (Fiction Collection):

---

Screenshot 3 — Health fragment: unhealthy library (Broken Archive):

## UI Screenshots — Library Health (bookshelf-kfi6 / PR #633) Headless Chromium (Google Chrome), real app + MySQL. Seeded 2 libraries: **Fiction Collection** (real tmpdir = healthy) and **Broken Archive** (nonexistent path = unhealthy). --- **Screenshot 1 — Sidebar nav with health indicator dots (books page):**  --- **Screenshot 2 — Health fragment: healthy library (Fiction Collection):**  --- **Screenshot 3 — Health fragment: unhealthy library (Broken Archive):** 

nav-health-dots.png

42 KiB

health-fragment-healthy.png

12 KiB

health-fragment-healthy.png

12 KiB

health-fragment-unhealthy.png

12 KiB

zombor commented 2026-06-19 03:21:40 +00:00

Author

Owner

Copy link

UI Review -- PR #633 (bookshelf-kfi6)

Per .claude/rules/review-standard.md. Screenshots captured with headless Chromium (real app + MySQL, 2 seeded libraries).

[MAJOR] templates/pages/library_health.html -- Fragment CSS classes have no matching CSS rules

The health fragment template emits .lib-health-fragment, .lib-health-paths, .lib-health-path, .lib-health-path--err, .lib-health-path-name, .lib-health-path-error but static/css/main.css defines zero rules for any of these classes. Screenshot 2 and 3 confirm: the fragment renders as an unstyled white page with browser-default serif font,

/
• with browser bullets, and black text on white -- completely disconnected from the dark theme and design system. The fragment is currently a bare HTML snippet with no visual treatment whatsoever.

  Fix: add CSS rules for .lib-health-fragment (dark card background, padding, border), .lib-health-paths (remove list-style, padding), .lib-health-path--err (color: var(--danger)), .lib-health-path-name and .lib-health-path-error (muted/small error text) in static/css/main.css, consistent with the existing .lib-health-dot styling.

  [MINOR] templates/pages/library_health.html:2 -- Fragment lacks a visible healthy/error status summary

  The fragment shows only the list of paths with no aggregate status indicator. The .lib-health-dot is only in the sidebar nav (base.html:237), not in the fragment itself. A future Stimulus polling consumer would show only paths with no ok/err summary. Suggest adding a small header row reusing .lib-health-dot--ok/.lib-health-dot--err before the path list.

  [MINOR] static/css/main.css:592 -- .lib-health-dot uses inline-block + vertical-align inside flex container

  The sidebar nav link is a flex container; inline-block + vertical-align: middle on a flex child is ignored by the flex layout algorithm. Use align-self: center instead. No visible regression currently but is incorrect per CSS spec.

  [MINOR] Screenshot 1 (sidebar) -- Health dots absent on first page load (timing)

  Both libraries render without any health indicator dots on first load. The async health probe (2s per-path timeout) races the first page render. This is an expected cold-start behaviour but users see no dot at all until the cache warms. No code defect; noting for UX awareness.

  REVIEW VERDICT: 0 blocker, 1 major, 3 minor

## UI Review -- PR #633 (bookshelf-kfi6) Per .claude/rules/review-standard.md. Screenshots captured with headless Chromium (real app + MySQL, 2 seeded libraries). [MAJOR] templates/pages/library_health.html -- Fragment CSS classes have no matching CSS rules The health fragment template emits .lib-health-fragment, .lib-health-paths, .lib-health-path, .lib-health-path--err, .lib-health-path-name, .lib-health-path-error but static/css/main.css defines zero rules for any of these classes. Screenshot 2 and 3 confirm: the fragment renders as an unstyled white page with browser-default serif font, <ul>/<li> with browser bullets, and black text on white -- completely disconnected from the dark theme and design system. The fragment is currently a bare HTML snippet with no visual treatment whatsoever. Fix: add CSS rules for .lib-health-fragment (dark card background, padding, border), .lib-health-paths (remove list-style, padding), .lib-health-path--err (color: var(--danger)), .lib-health-path-name and .lib-health-path-error (muted/small error text) in static/css/main.css, consistent with the existing .lib-health-dot styling. [MINOR] templates/pages/library_health.html:2 -- Fragment lacks a visible healthy/error status summary The fragment shows only the list of paths with no aggregate status indicator. The .lib-health-dot is only in the sidebar nav (base.html:237), not in the fragment itself. A future Stimulus polling consumer would show only paths with no ok/err summary. Suggest adding a small header row reusing .lib-health-dot--ok/.lib-health-dot--err before the path list. [MINOR] static/css/main.css:592 -- .lib-health-dot uses inline-block + vertical-align inside flex container The sidebar nav link is a flex container; inline-block + vertical-align: middle on a flex child is ignored by the flex layout algorithm. Use align-self: center instead. No visible regression currently but is incorrect per CSS spec. [MINOR] Screenshot 1 (sidebar) -- Health dots absent on first page load (timing) Both libraries render without any health indicator dots on first load. The async health probe (2s per-path timeout) races the first page render. This is an expected cold-start behaviour but users see no dot at all until the cache warms. No code defect; noting for UX awareness. REVIEW VERDICT: 0 blocker, 1 major, 3 minor

zombor added 1 commit 2026-06-19 03:58:07 +00:00

fix(ui): add missing CSS rules for lib-health-fragment (bookshelf-kfi6) ... 342e753f33

- Add canonical dark-card styling for .lib-health-fragment using existing
  design-system tokens (--bg-card, --border, --radius, --space-*, --danger,
  --fg, --font-sans, --fg-muted) — no new color literals
- Remove bullets from .lib-health-paths list; add flex column + gap layout
- Style .lib-health-path as a flex column row
- .lib-health-path--err/.lib-health-path-error use var(--danger) for red text
- .lib-health-path-name uses var(--fg) for readable white-on-dark text
- Fix .lib-health-dot vertical-align:middle (ignored in flex) → align-self:center

These classes were emitted by templates/pages/library_health.html but had
no CSS rules, causing the health fragment to render browser-default (serif,
black on white).

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

zombor commented 2026-06-19 04:04:12 +00:00

Author

Owner

Copy link

post-fix: fragment now uses canonical dark-card styling

Healthy (all paths reachable):

Unhealthy (path not found):

Changes:

• .lib-health-fragment: dark card bg (--bg-card), border (--border), border-radius, padding — canonical card chrome
• .lib-health-paths: no bullets, flex column, gap
• .lib-health-path: flex column row
• .lib-health-path--err .lib-health-path-name + .lib-health-path-error: color: var(--danger) (red)
• .lib-health-path-name: color: var(--fg) (white)
• .lib-health-dot: vertical-align: middle → align-self: center (correct for flex parent)

post-fix: fragment now uses canonical dark-card styling **Healthy (all paths reachable):**  **Unhealthy (path not found):**  Changes: - `.lib-health-fragment`: dark card bg (`--bg-card`), border (`--border`), border-radius, padding — canonical card chrome - `.lib-health-paths`: no bullets, flex column, gap - `.lib-health-path`: flex column row - `.lib-health-path--err .lib-health-path-name` + `.lib-health-path-error`: `color: var(--danger)` (red) - `.lib-health-path-name`: `color: var(--fg)` (white) - `.lib-health-dot`: `vertical-align: middle` → `align-self: center` (correct for flex parent)

zombor added 1 commit 2026-06-19 13:05:45 +00:00

fix(nav): nil-ptr panic when LibraryCheckHealthBatch not wired (bookshelf-kfi6) ... 200e7183de

Root cause (two bugs working together):
1. appwire.Deps.LibraryCheckHealthBatch was a plain func field, but Module
   func receives Deps by VALUE — library.Wire's assignment wrote to a copy,
   leaving app.New's d.LibraryCheckHealthBatch nil.
2. WrapHealthCheck(nil, ttl>0) returned a non-nil closure that called nil(ctx,
   ids) → panic when nav re-rendered after library creation.

Fix:
- Change LibraryCheckHealthBatch to *func(...) (same pointer pattern as
  AfterScanSeed) so library.Wire writes through to app.New's stack frame.
- Add nil-fn guards to WrapHealthCheck, WrapInt64, WrapInt64Map so a nil
  underlying function returns a nil wrapper (safe for all three nav-count
  helpers).
- Add unit tests: WrapHealthCheck/WrapInt64/WrapInt64Map with nil fn return
  nil for both positive and zero TTL.
- Add regression test: NavMiddleware with nil CheckLibraryHealth does not
  panic and leaves Healthy nil on all sidebar library entries.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

zombor commented 2026-06-19 13:13:26 +00:00

Author

Owner

Copy link

CODE REVIEW: APPROVED

Phase 0: DEMO Verification

No standalone DEMO block was expected for this panic-fix PR — it is a correctness/wiring fix verified by unit tests and CI. CI is green. Proceeding.

Phase 1 + 2: Pointer Wiring Verification

Bug 1 — by-value Deps / LibraryCheckHealthBatch pointer wiring

Verified correct. Execution order in internal/app/app.go:

1. Line 337: var libraryCheckHealthBatchFn — starts as nil zero value
2. Line 338: d.LibraryCheckHealthBatch = &libraryCheckHealthBatchFn — pointer to local var set on Deps before modules loop
3. Lines 406-412: modules loop runs — library.Wire(d) is called with Deps by value, but the pointer inside it still points at app.New's stack-frame variable
4. internal/library/wire.go lines 88-89: if d.LibraryCheckHealthBatch != nil { *d.LibraryCheckHealthBatch = checkHealthBatch } — writes through the pointer, mutating libraryCheckHealthBatchFn in app.New's frame
5. Line 542 (after the loop): middleware.WrapHealthCheck(libraryCheckHealthBatchFn, navCacheTTL) — at this point the local var is non-nil; WrapHealthCheck receives the real batch function and wraps it

Ordering is correct. The pointer initialization happens before the loop; the deref happens after the loop. This exactly mirrors the AfterScanSeed pattern at lines 329-330.

Bug 2 — WrapHealthCheck / WrapInt64 / WrapInt64Map nil guards

All three verified correct. Each function has if fn == nil { return nil } as the first check, before any closure construction. A nil fn returns a nil wrapped function. The nav guard at internal/middleware/nav.go line 1025 is if counts.CheckLibraryHealth != nil && len(libs) > 0, so a nil result from WrapHealthCheck correctly disables the health dots without panicking.

Zero-TTL path: if ttl <= 0 { return fn } runs only when fn is non-nil (nil already returned above), so WrapHealthCheck(nil, 0) also returns nil — correct.

Regression tests

Present and correct.

• internal/middleware/nav_test.go lines 1049-1320: Describe("nil CheckLibraryHealth does not panic and disables health dots") — sets deps.CheckLibraryHealth = nil, makes an HTTP request through NavMiddleware, asserts 200 and SidebarLibraries[0].Healthy == nil. This directly covers the production panic path.
• internal/middleware/nav_cache_test.go: WrapInt64(nil, ...), WrapInt64Map(nil, ...), WrapHealthCheck(nil, ...) — both positive and zero TTL variants tested for all three wrappers.
• internal/middleware/nav_test.go lines 1222-1253: CheckLibraryHealth annotates SidebarLibraries — happy-path coverage.
• internal/middleware/nav_test.go lines 1255-1284: CheckLibraryHealth error leaves Healthy nil (non-fatal) — error path leaves Healthy nil without breaking the page.

Gap noted (non-blocking): The production wiring path itself (the pointer-deref-after-loop in app.New) is not unit-tested — pure wiring code in internal/app/ has no tests by project convention. The e2e journey-2 test added at e2e/api/journey_2_library_admin_test.go exercises GET /libraries/{id}/health end-to-end, which indirectly validates that library.Wire ran and wired the handler, but it does not assert that navCountDeps.CheckLibraryHealth is non-nil. This is a documentation-only gap; cannot be closed without a unit test on app.New wiring (which the project bans). Non-blocking.

Other plain-func fields on Deps with potential same trap

Scanned internal/appwire/appwire.go. All other fields that modules write back through (AfterScanSeed) already use the pointer pattern. The remaining plain-func fields (CheckBookAccess, UserIDFromRequest, GetUserLibraryIDs, etc.) are set by app.New itself before the modules loop, not by modules — so the by-value trap does not apply to them. No other silent-nil traps found in this diff.

WrapHealthCheck caching note

WrapHealthCheck uses a lastIDs closure variable written by the outer func before calling c.Get. This is safe because NavCache serializes fills with its own mutex. No race condition.

Verdict

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

CODE REVIEW: APPROVED ## Phase 0: DEMO Verification No standalone DEMO block was expected for this panic-fix PR — it is a correctness/wiring fix verified by unit tests and CI. CI is green. Proceeding. ## Phase 1 + 2: Pointer Wiring Verification ### Bug 1 — by-value Deps / LibraryCheckHealthBatch pointer wiring **Verified correct.** Execution order in internal/app/app.go: 1. Line 337: `var libraryCheckHealthBatchFn` — starts as nil zero value 2. Line 338: `d.LibraryCheckHealthBatch = &libraryCheckHealthBatchFn` — pointer to local var set on Deps before modules loop 3. Lines 406-412: modules loop runs — library.Wire(d) is called with Deps by value, but the pointer inside it still points at app.New's stack-frame variable 4. internal/library/wire.go lines 88-89: `if d.LibraryCheckHealthBatch != nil { *d.LibraryCheckHealthBatch = checkHealthBatch }` — writes through the pointer, mutating libraryCheckHealthBatchFn in app.New's frame 5. Line 542 (after the loop): `middleware.WrapHealthCheck(libraryCheckHealthBatchFn, navCacheTTL)` — at this point the local var is non-nil; WrapHealthCheck receives the real batch function and wraps it Ordering is correct. The pointer initialization happens before the loop; the deref happens after the loop. This exactly mirrors the AfterScanSeed pattern at lines 329-330. ### Bug 2 — WrapHealthCheck / WrapInt64 / WrapInt64Map nil guards **All three verified correct.** Each function has `if fn == nil { return nil }` as the first check, before any closure construction. A nil fn returns a nil wrapped function. The nav guard at internal/middleware/nav.go line 1025 is `if counts.CheckLibraryHealth != nil && len(libs) > 0`, so a nil result from WrapHealthCheck correctly disables the health dots without panicking. Zero-TTL path: `if ttl <= 0 { return fn }` runs only when fn is non-nil (nil already returned above), so `WrapHealthCheck(nil, 0)` also returns nil — correct. ### Regression tests **Present and correct.** - internal/middleware/nav_test.go lines 1049-1320: `Describe("nil CheckLibraryHealth does not panic and disables health dots")` — sets `deps.CheckLibraryHealth = nil`, makes an HTTP request through NavMiddleware, asserts 200 and `SidebarLibraries[0].Healthy == nil`. This directly covers the production panic path. - internal/middleware/nav_cache_test.go: `WrapInt64(nil, ...)`, `WrapInt64Map(nil, ...)`, `WrapHealthCheck(nil, ...)` — both positive and zero TTL variants tested for all three wrappers. - internal/middleware/nav_test.go lines 1222-1253: CheckLibraryHealth annotates SidebarLibraries — happy-path coverage. - internal/middleware/nav_test.go lines 1255-1284: CheckLibraryHealth error leaves Healthy nil (non-fatal) — error path leaves Healthy nil without breaking the page. **Gap noted (non-blocking):** The production wiring path itself (the pointer-deref-after-loop in app.New) is not unit-tested — pure wiring code in internal/app/ has no tests by project convention. The e2e journey-2 test added at e2e/api/journey_2_library_admin_test.go exercises GET /libraries/{id}/health end-to-end, which indirectly validates that library.Wire ran and wired the handler, but it does not assert that navCountDeps.CheckLibraryHealth is non-nil. This is a documentation-only gap; cannot be closed without a unit test on app.New wiring (which the project bans). Non-blocking. ### Other plain-func fields on Deps with potential same trap Scanned internal/appwire/appwire.go. All other fields that modules write back through (AfterScanSeed) already use the pointer pattern. The remaining plain-func fields (CheckBookAccess, UserIDFromRequest, GetUserLibraryIDs, etc.) are set by app.New itself before the modules loop, not by modules — so the by-value trap does not apply to them. No other silent-nil traps found in this diff. ### WrapHealthCheck caching note WrapHealthCheck uses a `lastIDs` closure variable written by the outer func before calling c.Get. This is safe because NavCache serializes fills with its own mutex. No race condition. ## Verdict REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor force-pushed bd-bookshelf-kfi6 from 200e7183de to 89a855abbc 2026-06-19 13:13:40 +00:00 Compare

zombor force-pushed bd-bookshelf-kfi6 from 89a855abbc to da83baf9d6 2026-06-19 13:38:27 +00:00 Compare

zombor force-pushed bd-bookshelf-kfi6 from da83baf9d6 to fc05cc4979 2026-06-19 13:44:39 +00:00 Compare

zombor merged commit 0e39b7c226 into main 2026-06-19 14:14:46 +00:00

zombor referenced this pull request from a commit 2026-06-19 14:14:49 +00:00

Merge pull request 'feat(library): per-library mount/path health check (bookshelf-kfi6)' (#633) from bd-bookshelf-kfi6 into main

zombor referenced this pull request from a commit 2026-06-19 15:09:00 +00:00

fix(library): scope show + scan-status by user library access (bookshelf-q14o)

zombor referenced this pull request 2026-06-19 15:09:15 +00:00

fix(library): scope show + scan-status by user library access (bookshelf-q14o) #644

zombor referenced this pull request 2026-06-19 15:20:51 +00:00

refactor(appwire): pass *Deps to modules, remove *func nil-trap hacks (bookshelf-72g7) #648

zombor referenced this pull request from a commit 2026-06-19 19:27:06 +00:00

fix(library): scope show + scan-status by user library access (bookshelf-q14o)

zombor referenced this pull request from a commit 2026-06-19 20:15:00 +00:00

fix(library): scope show + scan-status by user library access (bookshelf-q14o)

zombor referenced this pull request 2026-07-03 01:15:30 +00:00

fix(dedup): gate destructive endpoints behind permission middleware (bookshelf-u5q4) #889

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

No description provided.