fix(bulk): honor ?adv= advanced-search ruleset in by-filter bulk ops (bookshelf-t7cmm) #1463

Merged

zombor merged 2 commits from bd-bookshelf-t7cmm into main 2026-09-02 15:04:08 +00:00

Conversation 2 Commits 2 Files changed 14 +1050 -202

zombor commented 2026-09-02 14:06:29 +00:00

Owner

Copy link

Summary

By-filter bulk operations (Select-all-matching → Move & Organize / enrich / covers / DELETE / attach / lock / shelf-assign) rebuilt their match set via books.ParseViewQueryFilter, which had no handling for the ?adv= advanced-search param that ListHandler (the regular list view) already decodes. So an active advanced search was silently dropped and the by-filter bulk op operated on the user's entire accessible library set instead of the search results actually shown — a high-severity data-safety bug, since by-filter DELETE would hard-delete the whole library rather than the search results.

Root cause

• internal/books/handler.go:365 (list view) already decodes adv via decodeAdvSearch (wired to magic.DecodeAdvSearch) and injects the predicate as Filter.MagicWhere/MagicArgs/MagicJoins.
• The by-filter bulk paths never called that decoder:
  • Preview: internal/books/bulk_move_filter_preview_handler.go buildPreviewIDsParams → ParseViewQueryFilter (no adv handling).
  • Execute: internal/wfengine's BulkByFilterWorkflow activity re-parses ViewQuery per epoch via internal/app/build_enrich_deps.go's listFilteredIDsPageFromViewQuery (same gap).
  • Lock/Attach/Shelf-assign by-filter handlers (bulk_lock_by_filter_handler.go, bulk_attach_by_filter_handler.go, bulk_shelf_assign_by_filter_handler.go) share the same ParseViewQueryFilter call with no adv handling.
• Even where MagicWhere support existed on the target params struct, two silent field-copy gaps dropped it anyway: FilteredIDsPageParams had no MagicWhere/MagicArgs/MagicJoins fields at all, and the shared applyFilterFacetsToBulkLockParams copy helper didn't copy them either.

Fix

• New books.ApplyAdvSearch(filter, q, decodeAdvSearch, userID, userLibraryIDs) — decodes q's adv param (when present) via the injected AdvSearchDecoder and overlays MagicWhere/MagicArgs/MagicJoins onto the filter. Mirrors ListHandler's existing ?adv= handling exactly.
• FilteredIDsPageParams gained MagicWhere/MagicArgs/MagicJoins fields, wired into buildFilteredBookIDsPageQuery (mirroring the pattern already used by resolveLockFilterIDs).
• New shared books.FilteredIDsPageParamsFromFilter replaces two near-duplicate private mapping functions (one in the preview handler, one in internal/app) that had silently diverged — now both the preview path and the BulkByFilterWorkflow activity path build FilteredIDsPageParams from the same function, so a future field addition can't silently regress one path only.
• ApplyAdvSearch threaded into: the move-filter preview handler, internal/app's listFilteredIDsPageFromViewQuery (the workflow activity's per-epoch ID page builder, using magic.DecodeAdvSearch directly), and the lock/attach/shelf-assign by-filter handlers (d.DecodeAdvSearch from appwire.Deps, already wired for the list view).
• applyFilterFacetsToBulkLockParams (shared by lock/attach/shelf-assign) now also copies MagicWhere/MagicArgs/MagicJoins.

No workflow command-sequence change — BulkByFilterWorkflow's activity/sub-workflow/ContinueAsNew shape is untouched; only the SQL predicate the existing ListFilteredIDsPage activity builds is affected. No gowf.Version gate needed.

Test plan

• New black-box tests (package books_test) for ApplyAdvSearch, FilteredIDsPageParams.MagicWhere SQL injection, and regression coverage on the preview handler + lock/attach/shelf-assign handlers proving an active ?adv= ruleset is decoded and injected as MagicWhere, and that a decodeAdvSearch error surfaces as a 5xx.
• internal/app and **/wire.go stay excluded from the coverage gate per project convention (pure wiring, verified by e2e); the actual decode/injection logic lives in internal/books and is unit-tested there.
• make test lint and make coverage all green locally (100% coverage gate).

Closes bead bookshelf-t7cmm on merge.

## Summary By-filter bulk operations (Select-all-matching → Move & Organize / enrich / covers / DELETE / attach / lock / shelf-assign) rebuilt their match set via `books.ParseViewQueryFilter`, which had no handling for the `?adv=` advanced-search param that `ListHandler` (the regular list view) already decodes. So an active advanced search was silently dropped and the by-filter bulk op operated on the user's **entire accessible library set** instead of the search results actually shown — a high-severity data-safety bug, since by-filter **DELETE** would hard-delete the whole library rather than the search results. ## Root cause - `internal/books/handler.go:365` (list view) already decodes `adv` via `decodeAdvSearch` (wired to `magic.DecodeAdvSearch`) and injects the predicate as `Filter.MagicWhere/MagicArgs/MagicJoins`. - The by-filter bulk paths never called that decoder: - Preview: `internal/books/bulk_move_filter_preview_handler.go` `buildPreviewIDsParams` → `ParseViewQueryFilter` (no adv handling). - Execute: `internal/wfengine`'s `BulkByFilterWorkflow` activity re-parses `ViewQuery` per epoch via `internal/app/build_enrich_deps.go`'s `listFilteredIDsPageFromViewQuery` (same gap). - Lock/Attach/Shelf-assign by-filter handlers (`bulk_lock_by_filter_handler.go`, `bulk_attach_by_filter_handler.go`, `bulk_shelf_assign_by_filter_handler.go`) share the same `ParseViewQueryFilter` call with no adv handling. - Even where `MagicWhere` support existed on the target params struct, two silent field-copy gaps dropped it anyway: `FilteredIDsPageParams` had no `MagicWhere/MagicArgs/MagicJoins` fields at all, and the shared `applyFilterFacetsToBulkLockParams` copy helper didn't copy them either. ## Fix - New `books.ApplyAdvSearch(filter, q, decodeAdvSearch, userID, userLibraryIDs)` — decodes `q`'s `adv` param (when present) via the injected `AdvSearchDecoder` and overlays `MagicWhere/MagicArgs/MagicJoins` onto the filter. Mirrors `ListHandler`'s existing `?adv=` handling exactly. - `FilteredIDsPageParams` gained `MagicWhere/MagicArgs/MagicJoins` fields, wired into `buildFilteredBookIDsPageQuery` (mirroring the pattern already used by `resolveLockFilterIDs`). - New shared `books.FilteredIDsPageParamsFromFilter` replaces two near-duplicate private mapping functions (one in the preview handler, one in `internal/app`) that had silently diverged — now both the preview path and the `BulkByFilterWorkflow` activity path build `FilteredIDsPageParams` from the same function, so a future field addition can't silently regress one path only. - `ApplyAdvSearch` threaded into: the move-filter preview handler, `internal/app`'s `listFilteredIDsPageFromViewQuery` (the workflow activity's per-epoch ID page builder, using `magic.DecodeAdvSearch` directly), and the lock/attach/shelf-assign by-filter handlers (`d.DecodeAdvSearch` from `appwire.Deps`, already wired for the list view). - `applyFilterFacetsToBulkLockParams` (shared by lock/attach/shelf-assign) now also copies `MagicWhere/MagicArgs/MagicJoins`. No workflow command-sequence change — `BulkByFilterWorkflow`'s activity/sub-workflow/ContinueAsNew shape is untouched; only the SQL predicate the existing `ListFilteredIDsPage` activity builds is affected. No `gowf.Version` gate needed. ## Test plan - New black-box tests (`package books_test`) for `ApplyAdvSearch`, `FilteredIDsPageParams.MagicWhere` SQL injection, and regression coverage on the preview handler + lock/attach/shelf-assign handlers proving an active `?adv=` ruleset is decoded and injected as `MagicWhere`, and that a `decodeAdvSearch` error surfaces as a 5xx. - `internal/app` and `**/wire.go` stay excluded from the coverage gate per project convention (pure wiring, verified by e2e); the actual decode/injection logic lives in `internal/books` and is unit-tested there. - `make test lint` and `make coverage` all green locally (100% coverage gate). Closes bead bookshelf-t7cmm on merge.

zombor added 1 commit 2026-09-02 14:06:29 +00:00

fix(bulk): honor ?adv= advanced-search ruleset in by-filter bulk ops (bookshelf-t7cmm) ... ebd6fb6fb4

By-filter bulk operations (move/enrich/covers/delete/attach/lock/shelf-assign)
rebuilt their match set from ParseViewQueryFilter, which had no handling for the
?adv= advanced-search param that ListHandler already decodes for the list view.
An active advanced search was silently dropped, so Select-all-matching bulk ops
(including DELETE) operated on the user's entire accessible library set instead
of the search results shown on screen.

Adds books.ApplyAdvSearch (decodes adv via the existing magic.DecodeAdvSearch and
overlays MagicWhere/MagicArgs/MagicJoins onto the parsed Filter), threads it
through every by-filter seam that previously dropped it: the move-filter preview
handler, the BulkByFilterWorkflow activity's per-epoch ID page builder, and the
lock/attach/shelf-assign by-filter handlers. Also fixes two silent field-copy
gaps (FilteredIDsPageParams and applyFilterFacetsToBulkLockParams) that dropped
Magic* even when set, and extracts a single shared FilteredIDsPageParamsFromFilter
mapping used by both the preview and workflow-execute paths so they cannot
silently diverge again.

zombor commented 2026-09-02 14:21:02 +00:00

Author

Owner

Copy link

Security Review — PR #1463 (bd-bookshelf-t7cmm)

Scope: ?adv= advanced-search ruleset now threaded through the shared filtered-IDs
query used by by-filter bulk preview + BulkByFilterWorkflow (delete/move/attach/
lock/shelf-assign/covers/enrich), closing the "adv= ignored → operated on whole
library" data-safety gap.

Checked: SQL injection surface, multi-user/authz scoping, prod wiring for silent
adv-drop, cross-op consistency, secrets/PII in logs.

Findings:

No BLOCKER, MAJOR, or MINOR findings.

Verification detail (for the record):

1. SQL injection — magic.DecodeAdvSearch/TranslateWithOptions (unmodified by
   this diff) build predicates via a fixed switch-statement field catalog with
   ? placeholders; values only ever go into the returned args []any. The new
   filtered_ids_store.go injection point (buildFilteredBookIDsPageQuery)
   appends p.MagicWhere as an opaque string into the wheres slice (joined
   with " AND ") and appends p.MagicArgs positionally to whereArgs —
   no string concatenation of user data into SQL text, no new interpolation
   hole introduced.

2. Multi-user/authz (core of the fix) — MagicWhere is always appended to the
   wheres slice already containing the mandatory b.library_id IN (...)
   (fail-closed to 1=0 when UserLibraryIDs is empty/nil,
   filter_predicates.go:105-116), and all entries are joined with AND
   (filtered_ids_store.go:118) — the adv predicate can only narrow, never
   widen, the accessible set. userID/userLibraryIDs are sourced from
   userIDFromRequest(r) + getUserLibraryIDs(ctx, userID) (session-derived)
   in every handler touched (attach/lock/shelf-assign/move-preview), never from
   request body/query. ApplyAdvSearch passes them straight through to
   decodeAdvSearch, matching the pre-existing ListHandler pattern.

3. No prod path silently drops adv — grepped every nil, // decodeAdvSearch-
   shaped nil argument; all occurrences are in *_test.go files. Prod wiring
   (internal/books/wire.go) passes d.DecodeAdvSearch (set in
   internal/app/app.go:617 to magic.DecodeAdvSearch, a real function) to
   every by-filter handler that gained the parameter. The workflow-driven paths
   (delete/move/covers/enrich) go through listFilteredIDsPageFromViewQuery
   in internal/app/build_enrich_deps.go, which now unconditionally calls
   books.ApplyAdvSearch(filter, parsed, magic.DecodeAdvSearch, ...) on every
   epoch — not parameterized/nilable at that call site.

4. Consistency — attach, lock, shelf-assign, move-preview handlers, and the
   shared workflow ID-sweep activity (used by delete/move/covers/enrich) all
   route through the same ApplyAdvSearch → FilteredIDsPageParamsFromFilter
   → buildFilteredBookIDsPageQuery path. No mutating by-filter op was found
   still bypassing adv.

5. No secrets/PII logged in the touched handlers (only counts, IDs, trace_id).
   Ownership checks (checkMagicShelfAccess, resolveMagicShelfScope) are
   unchanged by this diff.

Also noted: DecodeAdvSearch returning nil is a documented no-op
(ApplyAdvSearch returns the filter unchanged when decodeAdvSearch == nil),
which is correct test-double behavior but relies on prod wiring always
supplying a real decoder — confirmed true today at every call site (see #3).
No follow-up needed since there's no code path in wire.go/build_enrich_deps.go
that could invoke a handler with a nil decoder in production.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

**Security Review — PR #1463 (bd-bookshelf-t7cmm)** Scope: `?adv=` advanced-search ruleset now threaded through the shared filtered-IDs query used by by-filter bulk preview + `BulkByFilterWorkflow` (delete/move/attach/ lock/shelf-assign/covers/enrich), closing the "adv= ignored → operated on whole library" data-safety gap. Checked: SQL injection surface, multi-user/authz scoping, prod wiring for silent adv-drop, cross-op consistency, secrets/PII in logs. Findings: No BLOCKER, MAJOR, or MINOR findings. Verification detail (for the record): 1. SQL injection — `magic.DecodeAdvSearch`/`TranslateWithOptions` (unmodified by this diff) build predicates via a fixed switch-statement field catalog with `?` placeholders; values only ever go into the returned `args []any`. The new `filtered_ids_store.go` injection point (`buildFilteredBookIDsPageQuery`) appends `p.MagicWhere` as an opaque string into the `wheres` slice (joined with `" AND "`) and appends `p.MagicArgs` positionally to `whereArgs` — no string concatenation of user data into SQL text, no new interpolation hole introduced. 2. Multi-user/authz (core of the fix) — `MagicWhere` is always appended to the `wheres` slice already containing the mandatory `b.library_id IN (...)` (fail-closed to `1=0` when `UserLibraryIDs` is empty/nil, `filter_predicates.go:105-116`), and all entries are joined with `AND` (`filtered_ids_store.go:118`) — the adv predicate can only narrow, never widen, the accessible set. `userID`/`userLibraryIDs` are sourced from `userIDFromRequest(r)` + `getUserLibraryIDs(ctx, userID)` (session-derived) in every handler touched (attach/lock/shelf-assign/move-preview), never from request body/query. `ApplyAdvSearch` passes them straight through to `decodeAdvSearch`, matching the pre-existing `ListHandler` pattern. 3. No prod path silently drops adv — grepped every `nil, // decodeAdvSearch`- shaped nil argument; all occurrences are in `*_test.go` files. Prod wiring (`internal/books/wire.go`) passes `d.DecodeAdvSearch` (set in `internal/app/app.go:617` to `magic.DecodeAdvSearch`, a real function) to every by-filter handler that gained the parameter. The workflow-driven paths (delete/move/covers/enrich) go through `listFilteredIDsPageFromViewQuery` in `internal/app/build_enrich_deps.go`, which now unconditionally calls `books.ApplyAdvSearch(filter, parsed, magic.DecodeAdvSearch, ...)` on every epoch — not parameterized/nilable at that call site. 4. Consistency — attach, lock, shelf-assign, move-preview handlers, and the shared workflow ID-sweep activity (used by delete/move/covers/enrich) all route through the same `ApplyAdvSearch` → `FilteredIDsPageParamsFromFilter` → `buildFilteredBookIDsPageQuery` path. No mutating by-filter op was found still bypassing adv. 5. No secrets/PII logged in the touched handlers (only counts, IDs, trace_id). Ownership checks (`checkMagicShelfAccess`, `resolveMagicShelfScope`) are unchanged by this diff. Also noted: `DecodeAdvSearch` returning `nil` is a documented no-op (`ApplyAdvSearch` returns the filter unchanged when `decodeAdvSearch == nil`), which is correct test-double behavior but relies on prod wiring always supplying a real decoder — confirmed true today at every call site (see #3). No follow-up needed since there's no code path in `wire.go`/`build_enrich_deps.go` that could invoke a handler with a nil decoder in production. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-09-02 14:25:02 +00:00

Author

Owner

Copy link

Code Review: PR #1463 (bd-bookshelf-t7cmm)

Reviewed the diff only (per policy, did not re-run tests; CI green + mergeable=true confirmed independently).

Summary of what I verified (traced, not assumed):

• The synchronous by-filter handlers (lock, attach, shelf-assign, move-preview) are correctly wired with d.DecodeAdvSearch in internal/books/wire.go and call the new books.ApplyAdvSearch before building FilteredIDsPageParams/ListBooksFilteredParams.
• Critically, the workflow EXECUTE path (enrich, covers, DELETE, custom-fetch, llm-vision, move, metadata, scan-file — all routed through BulkByFilterWorkflow via StartBulkByFilter) is fixed at the single shared composition point listFilteredIDsPageFromViewQuery in internal/app/build_enrich_deps.go:237-268, which now calls books.ApplyAdvSearch(filter, parsed, magic.DecodeAdvSearch, f.UserID, userLibIDs) before building params via the new books.FilteredIDsPageParamsFromFilter. Since every by-filter workflow op shares this one function, DELETE (the bead's specific worst-case concern) is fixed, not just preview.
• SQL safety: MagicArgs are bound as placeholders (whereArgs = append(whereArgs, p.MagicArgs...)), never string-concatenated; wheres are joined with " AND " uniformly (filtered_ids_store.go:196), so the adv predicate is properly ANDed with the rest, not OR'd in a way that could widen the set.
• Multi-user scoping: userID/userLibraryIDs passed to ApplyAdvSearch/DecodeAdvSearch come from userIDFromRequest/getUserLibraryIDs (session-derived) at every call site checked (lock/attach/shelf-assign/move-preview handlers, and the per-epoch getUserLibraryIDs re-resolution in buildListFilteredIDsPageFn) — never from the request body.
• No workflow command-sequence change: internal/wfengine/bulk_by_filter_workflow.go and module_bulk_by_filter.go have zero diff. The fix is entirely activity-internal (the ID set an existing activity produces), so no gowf.Version gate is required — confirmed correct.
• Architecture boundary intact: internal/books/filter_parse.go and filtered_ids_store.go import only net/url/middleware, no magic or workflow-engine import. AdvSearchDecoder is a plain func type; only internal/app wires the concrete magic.DecodeAdvSearch.
• All nil, // decodeAdvSearch occurrences are confined to *_test.go files — no production wiring passes nil for a real by-filter handler.
• Tests are non-vacuous, black-box (package books_test), and drive real behavior: filter_parse_test.go's ApplyAdvSearch Describe asserts pass-through of encoded/userID/userLibraryIDs to a stub decoder and error propagation; filtered_ids_store_test.go asserts MagicWhere/MagicJoins/MagicArgs actually appear in the built SQL string and args slice.

---

[MAJOR] internal/app/build_enrich_deps.go:237-268 — the riskiest composition point (workflow EXECUTE for DELETE/enrich/covers/move/etc.) has zero test coverage proving it honors adv
internal/app is explicitly exempted from the 100%-coverage gate because "correctness is verified by e2e tests" (see scripts/check-coverage.sh comment, and CLAUDE.md's "internal/app is pure wiring: NO TESTS in that package"). But this PR adds no e2e test exercising ?adv= through the real BulkByFilterWorkflow execute path (no diff under e2e/). The lower-layer primitives (ApplyAdvSearch, FilteredIDsPageParamsFromFilter, SQL-shape building) are well unit-tested in isolation, but the exact composition in listFilteredIDsPageFromViewQuery — parse ViewQuery → ApplyAdvSearch → FilteredIDsPageParamsFromFilter → listPage — is untested end-to-end. This matters because the original bug was precisely a case of individually-correct-looking code with one call site silently dropping adv; the fix's most safety-critical call site (the one that actually deletes/moves/enriches books) is verified only by manual reasoning, not a test. Given the bead's own severity framing ("by-filter DELETE under an active adv search would hard-delete the whole library"), this deserves at least a targeted addition to the existing e2e/browser/journey_bulk_toolbar_test.go (which already drives /bulk/filter/ — see main:e2e/browser/journey_bulk_toolbar_test.go:189-253) or a wfengine integration test asserting the workflow's produced ID set excludes books that fail an active adv ruleset. Suggested fix: add one It step to the existing bulk-toolbar journey (or a BulkByFilterWorkflow tester-based test) that triggers a by-filter delete/enrich with ?adv= active and asserts a book failing the ruleset survives / is not touched.

[MINOR] internal/books/bulk_lock_by_filter_handler.go:47-51 (and the identical pattern in bulk_attach_by_filter_handler.go, bulk_shelf_assign_by_filter_handler.go) — MagicShelfID silently clobbers an adv-derived predicate instead of combining them
buildBulkLockFilterParams (called first) sets p.MagicWhere/Args/Joins from ApplyAdvSearch's result when req.ViewQuery carries adv=. Immediately after, if req.MagicShelfID != 0 { applyMagicShelfScopeToParams(...) } unconditionally overwrites p.MagicWhere/Args/Joins with the magic shelf's predicate, silently discarding the adv ruleset. The equivalent workflow path (buildListFilteredIDsPageFn in internal/app/build_enrich_deps.go:389-394) has the same precedence: it branches to listMagicShelfIDsPage before ever parsing ViewQuery/adv. Static JS evidence (filter_drawer_controller.js:394, ALLOWED_CONTEXT_KEYS includes both magic_shelf_id and general filter context) suggests the UI can plausibly combine a magic-shelf view with an active advanced search. This isn't a regression introduced by this PR (pre-PR, adv was unconditionally dropped in every case), so it doesn't block this fix, but it's an incomplete edge case worth a follow-up bead — file one so it isn't lost (bd create "Fix: adv= silently dropped when MagicShelfID is also present in by-filter ops" -d "Follow-up to bookshelf-t7cmm: ...").

REVIEW VERDICT: 0 blocker, 1 major, 1 minor

## Code Review: PR #1463 (bd-bookshelf-t7cmm) Reviewed the diff only (per policy, did not re-run tests; CI green + mergeable=true confirmed independently). **Summary of what I verified (traced, not assumed):** - The synchronous by-filter handlers (lock, attach, shelf-assign, move-preview) are correctly wired with `d.DecodeAdvSearch` in `internal/books/wire.go` and call the new `books.ApplyAdvSearch` before building `FilteredIDsPageParams`/`ListBooksFilteredParams`. - Critically, the **workflow EXECUTE path** (enrich, covers, DELETE, custom-fetch, llm-vision, move, metadata, scan-file — all routed through `BulkByFilterWorkflow` via `StartBulkByFilter`) is fixed at the single shared composition point `listFilteredIDsPageFromViewQuery` in `internal/app/build_enrich_deps.go:237-268`, which now calls `books.ApplyAdvSearch(filter, parsed, magic.DecodeAdvSearch, f.UserID, userLibIDs)` before building params via the new `books.FilteredIDsPageParamsFromFilter`. Since every by-filter workflow op shares this one function, DELETE (the bead's specific worst-case concern) is fixed, not just preview. - SQL safety: `MagicArgs` are bound as placeholders (`whereArgs = append(whereArgs, p.MagicArgs...)`), never string-concatenated; `wheres` are joined with `" AND "` uniformly (`filtered_ids_store.go:196`), so the adv predicate is properly ANDed with the rest, not OR'd in a way that could widen the set. - Multi-user scoping: `userID`/`userLibraryIDs` passed to `ApplyAdvSearch`/`DecodeAdvSearch` come from `userIDFromRequest`/`getUserLibraryIDs` (session-derived) at every call site checked (lock/attach/shelf-assign/move-preview handlers, and the per-epoch `getUserLibraryIDs` re-resolution in `buildListFilteredIDsPageFn`) — never from the request body. - No workflow command-sequence change: `internal/wfengine/bulk_by_filter_workflow.go` and `module_bulk_by_filter.go` have zero diff. The fix is entirely activity-internal (the ID set an existing activity produces), so no `gowf.Version` gate is required — confirmed correct. - Architecture boundary intact: `internal/books/filter_parse.go` and `filtered_ids_store.go` import only `net/url`/`middleware`, no `magic` or workflow-engine import. `AdvSearchDecoder` is a plain func type; only `internal/app` wires the concrete `magic.DecodeAdvSearch`. - All `nil, // decodeAdvSearch` occurrences are confined to `*_test.go` files — no production wiring passes nil for a real by-filter handler. - Tests are non-vacuous, black-box (`package books_test`), and drive real behavior: `filter_parse_test.go`'s `ApplyAdvSearch` Describe asserts pass-through of encoded/userID/userLibraryIDs to a stub decoder and error propagation; `filtered_ids_store_test.go` asserts `MagicWhere`/`MagicJoins`/`MagicArgs` actually appear in the built SQL string and args slice. --- [MAJOR] internal/app/build_enrich_deps.go:237-268 — the riskiest composition point (workflow EXECUTE for DELETE/enrich/covers/move/etc.) has zero test coverage proving it honors adv `internal/app` is explicitly exempted from the 100%-coverage gate because "correctness is verified by e2e tests" (see `scripts/check-coverage.sh` comment, and CLAUDE.md's "`internal/app` is pure wiring: NO TESTS in that package"). But this PR adds **no e2e test** exercising `?adv=` through the real `BulkByFilterWorkflow` execute path (no diff under `e2e/`). The lower-layer primitives (`ApplyAdvSearch`, `FilteredIDsPageParamsFromFilter`, SQL-shape building) are well unit-tested in isolation, but the exact composition in `listFilteredIDsPageFromViewQuery` — parse ViewQuery → ApplyAdvSearch → FilteredIDsPageParamsFromFilter → listPage — is untested end-to-end. This matters because the *original bug* was precisely a case of individually-correct-looking code with one call site silently dropping adv; the fix's most safety-critical call site (the one that actually deletes/moves/enriches books) is verified only by manual reasoning, not a test. Given the bead's own severity framing ("by-filter DELETE under an active adv search would hard-delete the whole library"), this deserves at least a targeted addition to the existing `e2e/browser/journey_bulk_toolbar_test.go` (which already drives `/bulk/filter/` — see main:e2e/browser/journey_bulk_toolbar_test.go:189-253) or a `wfengine` integration test asserting the workflow's produced ID set excludes books that fail an active `adv` ruleset. Suggested fix: add one `It` step to the existing bulk-toolbar journey (or a `BulkByFilterWorkflow` tester-based test) that triggers a by-filter delete/enrich with `?adv=` active and asserts a book failing the ruleset survives / is not touched. [MINOR] internal/books/bulk_lock_by_filter_handler.go:47-51 (and the identical pattern in bulk_attach_by_filter_handler.go, bulk_shelf_assign_by_filter_handler.go) — MagicShelfID silently clobbers an adv-derived predicate instead of combining them `buildBulkLockFilterParams` (called first) sets `p.MagicWhere/Args/Joins` from `ApplyAdvSearch`'s result when `req.ViewQuery` carries `adv=`. Immediately after, `if req.MagicShelfID != 0 { applyMagicShelfScopeToParams(...) }` unconditionally **overwrites** `p.MagicWhere/Args/Joins` with the magic shelf's predicate, silently discarding the adv ruleset. The equivalent workflow path (`buildListFilteredIDsPageFn` in `internal/app/build_enrich_deps.go:389-394`) has the same precedence: it branches to `listMagicShelfIDsPage` before ever parsing `ViewQuery`/adv. Static JS evidence (`filter_drawer_controller.js:394`, `ALLOWED_CONTEXT_KEYS` includes both `magic_shelf_id` and general filter context) suggests the UI can plausibly combine a magic-shelf view with an active advanced search. This isn't a regression introduced by this PR (pre-PR, adv was unconditionally dropped in every case), so it doesn't block this fix, but it's an incomplete edge case worth a follow-up bead — file one so it isn't lost (`bd create "Fix: adv= silently dropped when MagicShelfID is also present in by-filter ops" -d "Follow-up to bookshelf-t7cmm: ..."`). REVIEW VERDICT: 0 blocker, 1 major, 1 minor

zombor added 1 commit 2026-09-02 14:41:57 +00:00

test(bulk): cover adv-honoring EXECUTE path for by-filter bulk ops (bookshelf-t7cmm) ... c3ba5b13e3

internal/app is coverage-exempt (verified only by e2e), so the previous fix
left the riskiest seam untested: whether ?adv= actually reaches the shared
composition point that drives EVERY by-filter WORKFLOW execute op (enrich,
covers, DELETE, custom-fetch, llm-vision, move, metadata, scan-file).

Extract the composition logic out of internal/app's unexported
listFilteredIDsPageFromViewQuery into a new exported
books.ListFilteredIDsPageFromViewQuery, matching the existing
buildPreviewIDsParams pattern already used by the move-filter-preview
handler. internal/app now just wires magic.DecodeAdvSearch into the public
books function instead of duplicating the ParseViewQueryFilter +
MergeScalarFilterFields + ApplyAdvSearch + FilteredIDsPageParamsFromFilter
chain locally.

Add a black-box test in internal/books/filter_parse_test.go that drives the
new function with a stubbed listPage simulating DB filtering on MagicWhere,
asserting an adv-excluded book id is NOT returned when ?adv= is present
(and IS returned when absent) — proving the adv predicate is wired on the
real execute path, not just exercised via the already-tested ApplyAdvSearch
helper in isolation. Confirmed the assertions fail (RED) when the
ApplyAdvSearch call is temporarily removed from the new function.

zombor merged commit b30e208821 into main 2026-09-02 15:04:08 +00:00

zombor referenced this pull request from a commit 2026-09-02 15:04:12 +00:00

Merge pull request 'fix(bulk): honor ?adv= advanced-search ruleset in by-filter bulk ops (bookshelf-t7cmm)' (#1463) from bd-bookshelf-t7cmm 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!1463

No description provided.