zombor/pergamum
1
0
Fork 0
Code Issues Pull requests 3 Projects Releases Packages 2 Wiki Activity Actions

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

Closed
zombor wants to merge 2 commits from bd-bookshelf-72g7 into main
pull from: bd-bookshelf-72g7
merge into: zombor:main
Conversation 3 Commits 2 Files changed 24 +160 -77
zombor commented 2026-06-19 15:20:51 +00:00
Owner
Copy link

Summary

  • type Module func(*Deps) error — modules now receive a pointer so any func-field they assign (e.g. AfterScanSeed, LibraryCheckHealthBatch) is visible to app.New after the loop.
  • Removed the *func pointer hacks (AfterScanSeed *func(...) / LibraryCheckHealthBatch *func(...)) introduced in #633 as a workaround for the by-value footgun — they are no longer needed.
  • All 20 domain Wire funcs updated: func Wire(d appwire.Deps) error → func Wire(d *appwire.Deps) error. Field access d.X is unchanged (Go auto-derefs pointers).
  • internal/app/app.go loop calls m(&d); scan deps receive &d.AfterScanSeed for lazy deref at scan time (wfengine is constructed before the modules loop, hook is set during).
  • Guard test in internal/appwire/appwire_test.go proves module-assigned AfterScanSeed and LibraryCheckHealthBatch survive the loop — if Module is ever reverted to by-value, the test fails.
  • Makefile: added internal/appwire/... to UNIT_PKGS.

Test plan

  • go build ./... passes
  • make test — all 41 packages pass
  • make coverage — zero uncovered statement blocks
  • Guard test in internal/appwire/appwire_test.go green

Closes bead bookshelf-72g7 on merge.

## Summary - **`type Module func(*Deps) error`** — modules now receive a pointer so any func-field they assign (e.g. `AfterScanSeed`, `LibraryCheckHealthBatch`) is visible to `app.New` after the loop. - **Removed the `*func` pointer hacks** (`AfterScanSeed *func(...)` / `LibraryCheckHealthBatch *func(...)`) introduced in #633 as a workaround for the by-value footgun — they are no longer needed. - **All 20 domain `Wire` funcs** updated: `func Wire(d appwire.Deps) error` → `func Wire(d *appwire.Deps) error`. Field access `d.X` is unchanged (Go auto-derefs pointers). - **`internal/app/app.go`** loop calls `m(&d)`; scan deps receive `&d.AfterScanSeed` for lazy deref at scan time (wfengine is constructed before the modules loop, hook is set during). - **Guard test** in `internal/appwire/appwire_test.go` proves module-assigned `AfterScanSeed` and `LibraryCheckHealthBatch` survive the loop — if `Module` is ever reverted to by-value, the test fails. - **`Makefile`**: added `internal/appwire/...` to `UNIT_PKGS`. ## Test plan - [x] `go build ./...` passes - [x] `make test` — all 41 packages pass - [x] `make coverage` — zero uncovered statement blocks - [x] Guard test in `internal/appwire/appwire_test.go` green Closes bead bookshelf-72g7 on merge.
refactor(appwire): pass *Deps to modules, remove *func nil-trap hacks (bookshelf-72g7)
Some checks failed
/ JS Unit Tests (pull_request) Waiting to run
Details
/ E2E Browser (pull_request) Has been cancelled
Details
/ Lint (pull_request) Has been cancelled
Details
/ E2E API (pull_request) Has been cancelled
Details
/ Test (pull_request) Has been cancelled
Details
/ Integration (pull_request) Has been cancelled
Details
784b4337e0
 
Change Module type from func(Deps) to func(*Deps) so modules can assign
func-fields (AfterScanSeed, LibraryCheckHealthBatch) and have those writes
visible to app.New after the modules loop — eliminating the by-value
silent-nil footgun structurally.

- appwire.go: Module type → func(*Deps) error; AfterScanSeed and
  LibraryCheckHealthBatch reverted from *func pointer hack to plain func
  fields (now safe because *Deps passes reference through the loop)
- app.go: loop calls m(&d); removed var afterScanSeedFn/*func plumbing;
  BuildScanDeps receives &d.AfterScanSeed so the lazy-deref closure still
  works (scan deps are wired before modules run, hook assigned during);
  navCountDeps.CheckLibraryHealth reads d.LibraryCheckHealthBatch directly
- All 20 domain Wire funcs: signature func(d appwire.Deps) → func(d *appwire.Deps);
  field access d.X unchanged (Go auto-derefs); books/library assignment sites
  simplified to direct assign (no nil guard + pointer dereference)
- books/wire.go: wireLLMIdentifyResult also updated to *appwire.Deps
- Makefile: add internal/appwire/... to UNIT_PKGS
- internal/appwire/appwire_test.go: guard test proving module-assigned
  AfterScanSeed and LibraryCheckHealthBatch are visible after loop

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
refactor(appwire): fix guard test to use BeforeEach/JustBeforeEach one-Expect-per-It pattern
All checks were successful
/ JS Unit Tests (pull_request) Successful in 45s
Details
/ Lint (pull_request) Successful in 1m33s
Details
/ E2E API (pull_request) Successful in 1m40s
Details
/ Test (pull_request) Successful in 2m41s
Details
/ E2E Browser (pull_request) Successful in 2m36s
Details
/ Integration (pull_request) Successful in 2m42s
Details
dfe252a971
 
Split multi-Expect It blocks into proper Ginkgo BDD structure per project conventions.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
zombor commented 2026-06-19 15:28:55 +00:00
Author
Owner
Copy link

SECURITY REVIEW — bookshelf-72g7 / PR #648

Findings

Security-relevant Deps fields: value→pointer change

Verification: all security-gated func-fields are set in app.New in the struct literal (lines ~225–320) or in the explicit block before the modules loop (lines ~394–399), BEFORE for _, m := range modules { m(&d) } is called.

Fields confirmed set before module loop:

  • AdminRequired — set in struct literal
  • BookdropRequired, BookEditMetadataRequired, BookDeleteRequired, BookDownloadRequired, all BookBulk*Required, BookMoveOrganizeRequired, BookUploadRequired, BookDetachRequired — set in struct literal
  • LibraryManipulateRequired, LibraryBulkAutoFetchRequired, LibraryBulkRegenerateCoverRequired, LibraryManageMetadataConfigRequired — set in struct literal
  • ExtractUser — set in struct literal
  • GetOIDCConfig — set in struct literal
  • CheckBookAccess, UserIDFromRequest, GetUserLibraryIDs — set explicitly in the block at ~line 394–399, still before the modules loop

None of these are written by any module; no module can nil them out or replace them. The pointer change does not expose any of these to module mutation.

AfterScanSeed / LibraryCheckHealthBatch revert

AfterScanSeed is set by books.Wire and used only as a non-fatal post-ingest hook (recomputes match score). LibraryCheckHealthBatch is set by library.Wire and used only for nav health dots. Neither gates authentication, authorization, or ownership checks. Confirmed: no security gate runs through either field. The revert from *func to plain func is safe and simplifying.

CSRF / AuthMiddleware ordering

The middleware chain is built entirely after the modules loop, using values captured directly from local variables (not from d). The order is unchanged: TraceID → RequestLogger → MetricsRecorder → Recoverer → AuthMiddleware → MaxBytes → CSRF → MethodOverride → NavMiddleware → mux. No module can influence middleware ordering. No change here.

Module execution order

modules is a package-level var slice (not modified by any module). No module writes to d.Mux in a way that could register a route before its capability middleware — all route registrations go through RegisterRoutes within the module which adds the required middleware inline. Order unchanged from main.

Guard test

internal/appwire/appwire_test.go adds a regression test that proves *Deps wiring works: if Module is ever reverted to func(Deps), the assignment is silently discarded and the test fails. This is a meaningful safety net.

No security-relevant field becomes nil, unassigned, or mis-wired as a result of this change. No auth/ownership/CSRF behavior is altered.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

**SECURITY REVIEW — bookshelf-72g7 / PR #648** ## Findings ### Security-relevant Deps fields: value→pointer change **Verification: all security-gated func-fields are set in `app.New` in the struct literal (lines ~225–320) or in the explicit block before the modules loop (lines ~394–399), BEFORE `for _, m := range modules { m(&d) }` is called.** Fields confirmed set before module loop: - `AdminRequired` — set in struct literal - `BookdropRequired`, `BookEditMetadataRequired`, `BookDeleteRequired`, `BookDownloadRequired`, all `BookBulk*Required`, `BookMoveOrganizeRequired`, `BookUploadRequired`, `BookDetachRequired` — set in struct literal - `LibraryManipulateRequired`, `LibraryBulkAutoFetchRequired`, `LibraryBulkRegenerateCoverRequired`, `LibraryManageMetadataConfigRequired` — set in struct literal - `ExtractUser` — set in struct literal - `GetOIDCConfig` — set in struct literal - `CheckBookAccess`, `UserIDFromRequest`, `GetUserLibraryIDs` — set explicitly in the block at ~line 394–399, still before the modules loop None of these are written by any module; no module can nil them out or replace them. The pointer change does not expose any of these to module mutation. ### AfterScanSeed / LibraryCheckHealthBatch revert `AfterScanSeed` is set by `books.Wire` and used only as a non-fatal post-ingest hook (recomputes match score). `LibraryCheckHealthBatch` is set by `library.Wire` and used only for nav health dots. Neither gates authentication, authorization, or ownership checks. Confirmed: no security gate runs through either field. The revert from `*func` to plain `func` is safe and simplifying. ### CSRF / AuthMiddleware ordering The middleware chain is built entirely after the modules loop, using values captured directly from local variables (not from `d`). The order is unchanged: `TraceID → RequestLogger → MetricsRecorder → Recoverer → AuthMiddleware → MaxBytes → CSRF → MethodOverride → NavMiddleware → mux`. No module can influence middleware ordering. No change here. ### Module execution order `modules` is a package-level `var` slice (not modified by any module). No module writes to `d.Mux` in a way that could register a route before its capability middleware — all route registrations go through `RegisterRoutes` within the module which adds the required middleware inline. Order unchanged from main. ### Guard test `internal/appwire/appwire_test.go` adds a regression test that proves `*Deps` wiring works: if `Module` is ever reverted to `func(Deps)`, the assignment is silently discarded and the test fails. This is a meaningful safety net. --- **No security-relevant field becomes nil, unassigned, or mis-wired as a result of this change. No auth/ownership/CSRF behavior is altered.** REVIEW VERDICT: 0 blocker, 0 major, 0 minor
zombor commented 2026-06-19 15:31:29 +00:00
Author
Owner
Copy link

Code Review — bookshelf-72g7 (PR #648)

Phase 0: DEMO Verification

No DEMO block required — pure structural refactor with no user-facing behavior change. CI is the behavioral gate (confirmed green per dispatch).

Phase 1 & 2: Spec Compliance + Code Quality

All 20 Wire funcs converted: confirmed 20 removals of func Wire(d appwire.Deps) error and 20 additions of func Wire(d *appwire.Deps) error in the diff. No Wire func left at the old signature.

*No local value copy of d: scanned every Wire func — none takes := *d or otherwise copies the pointed-to struct. All field assignments are visible to app.New after the loop.

AfterScanSeed reversion is correct: books.Wire (first in the modules slice) assigns d.AfterScanSeed = func(...) directly. BuildScanDeps is called with &d.AfterScanSeed — a pointer to the struct field — before the modules loop. The AfterSeed closure in BuildScanDeps dereferences lazily (if afterScanSeed != nil && *afterScanSeed != nil), so it sees the value books.Wire wrote at scan-execution time. Correct and sound.

LibraryCheckHealthBatch reversion is correct: library.Wire assigns d.LibraryCheckHealthBatch = checkHealthBatch directly. app.New reads it after the modules loop (app.go:537) and passes it to middleware.WrapHealthCheck. WrapHealthCheck(nil, ...) returns nil, so a missed wiring disables health dots without panicking. Correct.

Guard test is sound: the test constructs a Module that assigns d.AfterScanSeed / d.LibraryCheckHealthBatch via *appwire.Deps and asserts the assignments are visible in the caller's deps var after the call. Under the old func(Deps) error type, the literal func(d *appwire.Deps) error would not satisfy appwire.Module — the test would fail to compile, which is stronger than a runtime assertion.

*StartupHooks append through Deps is safe: d.StartupHooks = append(d.StartupHooks, ...) inside books.Wire updates the slice header on the Deps struct in-place because the module receives &d. app.New reads d.StartupHooks after the loop (app.go:614). Correct.

wireLLMIdentifyResult helper correctly updated: signature changed from appwire.Deps to *appwire.Deps, called as wireLLMIdentifyResult(d) from inside Wire(d *appwire.Deps). Types match.

[MINOR] internal/app/app.go:628 — Stale docstring in BuildScanDeps
The comment says "books.Wire sets the function via d.StartupHooks" but books.Wire now assigns d.AfterScanSeed directly — no StartupHooks involvement. Should say "books.Wire sets d.AfterScanSeed directly (Module receives *Deps)". No correctness impact.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Code Review — bookshelf-72g7 (PR #648) ### Phase 0: DEMO Verification No DEMO block required — pure structural refactor with no user-facing behavior change. CI is the behavioral gate (confirmed green per dispatch). --- ### Phase 1 & 2: Spec Compliance + Code Quality **All 20 Wire funcs converted:** confirmed 20 removals of `func Wire(d appwire.Deps) error` and 20 additions of `func Wire(d *appwire.Deps) error` in the diff. No Wire func left at the old signature. **No local value copy of *d:** scanned every Wire func — none takes `:= *d` or otherwise copies the pointed-to struct. All field assignments are visible to `app.New` after the loop. **AfterScanSeed reversion is correct:** `books.Wire` (first in the modules slice) assigns `d.AfterScanSeed = func(...)` directly. `BuildScanDeps` is called with `&d.AfterScanSeed` — a pointer to the struct field — before the modules loop. The `AfterSeed` closure in `BuildScanDeps` dereferences lazily (`if afterScanSeed != nil && *afterScanSeed != nil`), so it sees the value `books.Wire` wrote at scan-execution time. Correct and sound. **LibraryCheckHealthBatch reversion is correct:** `library.Wire` assigns `d.LibraryCheckHealthBatch = checkHealthBatch` directly. `app.New` reads it after the modules loop (app.go:537) and passes it to `middleware.WrapHealthCheck`. `WrapHealthCheck(nil, ...)` returns nil, so a missed wiring disables health dots without panicking. Correct. **Guard test is sound:** the test constructs a Module that assigns `d.AfterScanSeed` / `d.LibraryCheckHealthBatch` via `*appwire.Deps` and asserts the assignments are visible in the caller's `deps` var after the call. Under the old `func(Deps) error` type, the literal `func(d *appwire.Deps) error` would not satisfy `appwire.Module` — the test would fail to compile, which is stronger than a runtime assertion. **StartupHooks append through *Deps is safe:** `d.StartupHooks = append(d.StartupHooks, ...)` inside `books.Wire` updates the slice header on the Deps struct in-place because the module receives `&d`. `app.New` reads `d.StartupHooks` after the loop (app.go:614). Correct. **wireLLMIdentifyResult helper correctly updated:** signature changed from `appwire.Deps` to `*appwire.Deps`, called as `wireLLMIdentifyResult(d)` from inside `Wire(d *appwire.Deps)`. Types match. --- [MINOR] internal/app/app.go:628 — Stale docstring in BuildScanDeps The comment says "books.Wire sets the function via d.StartupHooks" but `books.Wire` now assigns `d.AfterScanSeed` directly — no StartupHooks involvement. Should say "books.Wire sets d.AfterScanSeed directly (Module receives *Deps)". No correctness impact. --- REVIEW VERDICT: 0 blocker, 0 major, 1 minor
zombor commented 2026-06-19 15:31:54 +00:00
Author
Owner
Copy link

Code Review — bookshelf-72g7 (PR #648)

Phase 0: DEMO Verification

No DEMO block required — pure structural refactor, no user-facing behavior change. CI is the behavioral gate (confirmed green per dispatch).

Phase 1 & 2: Spec Compliance + Code Quality

All 20 Wire funcs converted: 20 removals of func Wire(d appwire.Deps) error and 20 additions of func Wire(d *appwire.Deps) error in the diff. No Wire func left at the old signature.

*No local value copy of d: scanned every Wire func — none takes := *d or otherwise copies the pointed-to struct. All field assignments are visible to app.New after the loop.

AfterScanSeed reversion is correct: books.Wire (first in modules slice) assigns d.AfterScanSeed = func(...) directly. BuildScanDeps is called with &d.AfterScanSeed — a pointer to the struct field — before the modules loop. The AfterSeed closure dereferences lazily, so it sees the value books.Wire wrote at scan-execution time. Correct and sound.

LibraryCheckHealthBatch reversion is correct: library.Wire assigns d.LibraryCheckHealthBatch = checkHealthBatch directly. app.New reads it after the modules loop (app.go:537) and passes it to middleware.WrapHealthCheck. WrapHealthCheck(nil, ...) returns nil — no panic on missed wiring. Correct.

Guard test is sound: constructs a Module that assigns d.AfterScanSeed/d.LibraryCheckHealthBatch via *appwire.Deps and asserts assignments are visible in the caller's deps var. Under the old func(Deps) error type, func(d *appwire.Deps) error would not satisfy appwire.Module — the test would fail to compile, stronger than a runtime assertion.

*StartupHooks append through Deps is safe: d.StartupHooks = append(...) inside books.Wire updates the slice header in-place (module receives &d). app.New reads d.StartupHooks after the loop (app.go:614). Correct.

wireLLMIdentifyResult helper correctly updated: signature changed from appwire.Deps to *appwire.Deps, called as wireLLMIdentifyResult(d) from inside Wire(d *appwire.Deps). Types match.

[MINOR] internal/app/app.go:628 — Stale docstring in BuildScanDeps
The comment says "books.Wire sets the function via d.StartupHooks" but books.Wire now assigns d.AfterScanSeed directly — no StartupHooks involvement. Should say "books.Wire sets d.AfterScanSeed directly (Module receives *Deps)". No correctness impact.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Code Review — bookshelf-72g7 (PR #648) ### Phase 0: DEMO Verification No DEMO block required — pure structural refactor, no user-facing behavior change. CI is the behavioral gate (confirmed green per dispatch). --- ### Phase 1 & 2: Spec Compliance + Code Quality **All 20 Wire funcs converted:** 20 removals of `func Wire(d appwire.Deps) error` and 20 additions of `func Wire(d *appwire.Deps) error` in the diff. No Wire func left at the old signature. **No local value copy of *d:** scanned every Wire func — none takes `:= *d` or otherwise copies the pointed-to struct. All field assignments are visible to `app.New` after the loop. **AfterScanSeed reversion is correct:** `books.Wire` (first in modules slice) assigns `d.AfterScanSeed = func(...)` directly. `BuildScanDeps` is called with `&d.AfterScanSeed` — a pointer to the struct field — before the modules loop. The `AfterSeed` closure dereferences lazily, so it sees the value `books.Wire` wrote at scan-execution time. Correct and sound. **LibraryCheckHealthBatch reversion is correct:** `library.Wire` assigns `d.LibraryCheckHealthBatch = checkHealthBatch` directly. `app.New` reads it after the modules loop (app.go:537) and passes it to `middleware.WrapHealthCheck`. `WrapHealthCheck(nil, ...)` returns nil — no panic on missed wiring. Correct. **Guard test is sound:** constructs a Module that assigns `d.AfterScanSeed`/`d.LibraryCheckHealthBatch` via `*appwire.Deps` and asserts assignments are visible in the caller's `deps` var. Under the old `func(Deps) error` type, `func(d *appwire.Deps) error` would not satisfy `appwire.Module` — the test would fail to compile, stronger than a runtime assertion. **StartupHooks append through *Deps is safe:** `d.StartupHooks = append(...)` inside `books.Wire` updates the slice header in-place (module receives `&d`). `app.New` reads `d.StartupHooks` after the loop (app.go:614). Correct. **wireLLMIdentifyResult helper correctly updated:** signature changed from `appwire.Deps` to `*appwire.Deps`, called as `wireLLMIdentifyResult(d)` from inside `Wire(d *appwire.Deps)`. Types match. --- [MINOR] internal/app/app.go:628 — Stale docstring in `BuildScanDeps` The comment says "books.Wire sets the function via d.StartupHooks" but `books.Wire` now assigns `d.AfterScanSeed` directly — no StartupHooks involvement. Should say "books.Wire sets d.AfterScanSeed directly (Module receives *Deps)". No correctness impact. --- REVIEW VERDICT: 0 blocker, 0 major, 1 minor
zombor closed this pull request 2026-06-19 16:02:12 +00:00
All checks were successful
/ JS Unit Tests (pull_request) Successful in 45s
Details
/ Lint (pull_request) Successful in 1m33s
Required
Details
/ E2E API (pull_request) Successful in 1m40s
Required
Details
/ Test (pull_request) Successful in 2m41s
Required
Details
/ E2E Browser (pull_request) Successful in 2m36s
Required
Details
/ Integration (pull_request) Successful in 2m42s
Required
Details

Pull request closed

Please reopen this pull request to perform a merge.
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
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!648
No description provided.