nnb9: black-box internal/library tests (bookshelf-nnb9.13) #1439

Merged

zombor merged 2 commits from bd-bookshelf-nnb9.13 into main 2026-08-13 01:22:00 +00:00

Conversation 3 Commits 2 Files changed 7 +341 -531

zombor commented 2026-08-12 20:02:27 +00:00

Owner

Copy link

Removes internal/library/export_test.go (the package library re-export
shim) and re-covers every unexported branch it exposed by driving the
public HTTP surface (buildMux + RegisterRoutes) instead of exported
wrapper funcs.

• healthHandler, getLibraryNamingPatternHandler,
  saveLibraryNamingPatternHandler, previewLibraryNamingPatternHandler,
  and libraryMatchWeightsDeps' CheckLibraryExists are now exercised via
  real requests through buildMux(deps).
• buildLibraryFieldPriorityMatrix and applyLibraryMatrixUpdate are now
  exercised via GET/PUT /libraries/{id}/metadata-field-priority.
• Simplified fpSlotStr/getSettingsFieldProvider/getSettingsEnabledField
  to fold genuinely-unreachable switch-fallback branches into the last
  case as default (n is always 1-4; key is always one of the fixed
  libraryFieldDefs keys), removing dead code instead of white-box
  testing it.
• Deletes the internal/library/export_test.go line from
  scripts/test_policy_check/allowlist.txt.

Scope: internal/library only. internal/library/scan
(batch_seed_internal_test.go) is a separate bead (bookshelf-nnb9.14),
not touched here.

Test plan:

• go build ./... clean
• go test ./internal/library/... — all green, 97.4% (100% excluding
  wire.go, which is already excluded from the coverage gate)
• make lint clean (incl. test-policy-check, e2e-policy-check)
• make test (full repo) green

Closes bead bookshelf-nnb9.13 on merge.

Removes internal/library/export_test.go (the package library re-export shim) and re-covers every unexported branch it exposed by driving the public HTTP surface (buildMux + RegisterRoutes) instead of exported wrapper funcs. - healthHandler, getLibraryNamingPatternHandler, saveLibraryNamingPatternHandler, previewLibraryNamingPatternHandler, and libraryMatchWeightsDeps' CheckLibraryExists are now exercised via real requests through buildMux(deps). - buildLibraryFieldPriorityMatrix and applyLibraryMatrixUpdate are now exercised via GET/PUT /libraries/{id}/metadata-field-priority. - Simplified fpSlotStr/getSettingsFieldProvider/getSettingsEnabledField to fold genuinely-unreachable switch-fallback branches into the last case as `default` (n is always 1-4; key is always one of the fixed libraryFieldDefs keys), removing dead code instead of white-box testing it. - Deletes the internal/library/export_test.go line from scripts/test_policy_check/allowlist.txt. Scope: internal/library only. internal/library/scan (batch_seed_internal_test.go) is a separate bead (bookshelf-nnb9.14), not touched here. Test plan: - go build ./... clean - go test ./internal/library/... — all green, 97.4% (100% excluding wire.go, which is already excluded from the coverage gate) - make lint clean (incl. test-policy-check, e2e-policy-check) - make test (full repo) green Closes bead bookshelf-nnb9.13 on merge.

zombor added 1 commit 2026-08-12 20:02:28 +00:00

Convert internal/library test debt from white-box to black-box (bookshelf-nnb9.13) ... ad7c3d4357

Removes internal/library/export_test.go (the package library re-export
shim) and re-covers every unexported branch it exposed by driving the
public HTTP surface (buildMux + RegisterRoutes) instead:

- healthHandler, getLibraryNamingPatternHandler,
  saveLibraryNamingPatternHandler, previewLibraryNamingPatternHandler,
  and libraryMatchWeightsDeps' CheckLibraryExists are now exercised via
  real requests through buildMux(deps) rather than exported wrapper funcs.
- buildLibraryFieldPriorityMatrix and applyLibraryMatrixUpdate are now
  exercised via GET/PUT /libraries/{id}/metadata-field-priority.

Also simplifies fpSlotStr/getSettingsFieldProvider/getSettingsEnabledField
to fold their genuinely-unreachable switch-fallback branches (n is always
1-4; key is always one of the fixed libraryFieldDefs keys) into the last
case as `default`, removing dead code instead of white-box-testing it.

Deletes the internal/library/export_test.go line from
scripts/test_policy_check/allowlist.txt.

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

zombor commented 2026-08-12 20:17:02 +00:00

Author

Owner

Copy link

Adversarial security review — PR #1439 (bookshelf-nnb9.13)

Reviewed the full diff (origin/main...origin/bd-bookshelf-nnb9.13). Confirmed this is genuinely a test-only refactor:

• internal/library/export_test.go deleted; all remaining test files in internal/library declare package library_test (black-box only) — no white-box test files remain, consistent with the allowlist line removal in scripts/test_policy_check/allowlist.txt.
• The only production file touched is internal/library/field_priority.go. The change is a behavior-preserving cleanup: three switch statements (getSettingsFieldProvider, getSettingsEnabledField, fpSlotStr) had their final explicit case replaced with default: plus a comment explaining the switch is exhaustive over the call sites (libraryFieldDefs fixed keys / literal slot ints 1-4, never user-supplied). Verified callers (buildLibraryFieldRows) only ever pass keys from the fixed libraryFieldDefs table and literal 1..4 — key/n are never request-supplied, so removing the unreachable return nil/return "" fallthrough does not change behavior or introduce a reachable different code path. No scoping, ownership, DISK_TYPE, or path-validation logic was touched.
• No unexported symbol was newly exported; the package's public API surface is unchanged (if anything, an internal-test-only export surface shrank).
• No secrets/tokens/PII in the modified test fixtures (grepped for password/secret/token/apikey — only unrelated matches: a JSON field named Key and an unknown token validation error string).

No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

Adversarial security review — PR #1439 (bookshelf-nnb9.13) Reviewed the full diff (`origin/main...origin/bd-bookshelf-nnb9.13`). Confirmed this is genuinely a test-only refactor: - `internal/library/export_test.go` deleted; all remaining test files in `internal/library` declare `package library_test` (black-box only) — no white-box test files remain, consistent with the allowlist line removal in `scripts/test_policy_check/allowlist.txt`. - The only production file touched is `internal/library/field_priority.go`. The change is a behavior-preserving cleanup: three `switch` statements (`getSettingsFieldProvider`, `getSettingsEnabledField`, `fpSlotStr`) had their final explicit `case` replaced with `default:` plus a comment explaining the switch is exhaustive over the call sites (`libraryFieldDefs` fixed keys / literal slot ints 1-4, never user-supplied). Verified callers (`buildLibraryFieldRows`) only ever pass keys from the fixed `libraryFieldDefs` table and literal `1..4` — key/n are never request-supplied, so removing the unreachable `return nil`/`return ""` fallthrough does not change behavior or introduce a reachable different code path. No scoping, ownership, DISK_TYPE, or path-validation logic was touched. - No unexported symbol was newly exported; the package's public API surface is unchanged (if anything, an internal-test-only export surface shrank). - No secrets/tokens/PII in the modified test fixtures (grepped for password/secret/token/apikey — only unrelated matches: a JSON field named `Key` and an `unknown token` validation error string). No findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-12 20:18:47 +00:00

Author

Owner

Copy link

Adversarial review of PR #1439 (bookshelf-nnb9.13) — internal/library white-box→black-box conversion.

Verified (anti-gaming checks per dispatch instructions):

1. No new exported symbols: export_test.go (90 lines) deleted outright; no Export* shim functions or type aliases were added anywhere else in the diff. library.Deps fields used by the new tests (CheckHealth, GetUserLibraryIDs, GetLibraryNamingPattern, UpdateLibraryNamingPattern, ValidateNamingTemplate, PreviewNamingPattern, LoadGlobalFieldPriority, LoadLibraryFieldPriority) were already public pre-PR (confirmed no new exported identifiers in internal/library/*.go production files).

2. Re-covering tests are real, not self-validating:

   • health_handler_test.go, naming_pattern_handler_test.go, field_priority_test.go, handler_test.go (match-weights block) all now route through buildMux(deps) + real httptest.Server, exercising the actual registered HTTP routes end-to-end instead of calling an exported handler-constructor directly.
   • field_priority_test.go's new PUT tests (savedOv/savedID) are captured inside the stubbed UpdateLibraryFieldPriority dep, which is only invoked by the real production handler — not set by the test itself. Genuine production-path exercise.
   • The removed FpSlotStr/GetSettingsFieldProvider/GetSettingsEnabledField "unknown key" tests correspond to unexported branches that were legitimately deleted as dead code (see #5 below), so removing their now-unreachable-branch tests is correct, not a coverage loss.

3. All internal/library/*_test.go (non-scan) files are package library_test. Confirmed via git show on every test file in the branch — no leftover package library. internal/library/scan/batch_seed_internal_test.go (package scan) is untouched, correctly out of scope for this bead.

4. scripts/test_policy_check/allowlist.txt diff removes exactly one line, internal/library/export_test.go; the internal/library/scan/batch_seed_internal_test.go line is untouched/intact.

5. Production-code changes are limited to internal/library/field_priority.go: getSettingsFieldProvider, getSettingsEnabledField, and fpSlotStr each had their explicit case "rating" / case 4 converted to default, removing the trailing unreachable return nil/return "". Verified this is safe: getSettingsFieldProvider/getSettingsEnabledField have exactly two call sites, both in buildLibraryFieldRows, driven only by def.key from the fixed libraryFieldDefs slice (never external/request input) — confirmed via grep, only 2 call sites total. fpSlotStr call sites are the 4 literal fpSlotStr(fp, 1..4) calls. The separate, PUT-facing setSettingsFieldProvider/setSettingsEnabledField (which DO take external key values from the request body and must no-op on unknown keys) were NOT touched by this PR — their existing implicit no-op default is unchanged, and the new "body with all field keys ... plus an unrecognized key" test in field_priority_test.go correctly exercises that unchanged no-op path. No behavior change for any reachable caller.

Minor observations (do not block):

[MINOR] internal/library/handler_test.go:2041-2043 — the converted "GET /libraries/{id}/match-weights" It("calls the Get dep and returns 200 when library exists", ...) has two separate Expect calls (Expect(getCalled).To(BeTrue()) and Expect(resp.StatusCode).To(Equal(http.StatusOK))) instead of one. Fold into a single Expect(getCalled, resp.StatusCode).To(...)-style compound assertion or split into two Its per project convention (project-conventions.md → Test structure: "exactly ONE Expect per It").

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

Adversarial review of PR #1439 (bookshelf-nnb9.13) — internal/library white-box→black-box conversion. Verified (anti-gaming checks per dispatch instructions): 1. No new exported symbols: `export_test.go` (90 lines) deleted outright; no `Export*` shim functions or type aliases were added anywhere else in the diff. `library.Deps` fields used by the new tests (`CheckHealth`, `GetUserLibraryIDs`, `GetLibraryNamingPattern`, `UpdateLibraryNamingPattern`, `ValidateNamingTemplate`, `PreviewNamingPattern`, `LoadGlobalFieldPriority`, `LoadLibraryFieldPriority`) were already public pre-PR (confirmed no new exported identifiers in `internal/library/*.go` production files). 2. Re-covering tests are real, not self-validating: - `health_handler_test.go`, `naming_pattern_handler_test.go`, `field_priority_test.go`, `handler_test.go` (match-weights block) all now route through `buildMux(deps)` + real `httptest.Server`, exercising the actual registered HTTP routes end-to-end instead of calling an exported handler-constructor directly. - `field_priority_test.go`'s new PUT tests (`savedOv`/`savedID`) are captured inside the stubbed `UpdateLibraryFieldPriority` dep, which is only invoked by the real production handler — not set by the test itself. Genuine production-path exercise. - The removed `FpSlotStr`/`GetSettingsFieldProvider`/`GetSettingsEnabledField` "unknown key" tests correspond to unexported branches that were legitimately deleted as dead code (see #5 below), so removing their now-unreachable-branch tests is correct, not a coverage loss. 3. All `internal/library/*_test.go` (non-scan) files are `package library_test`. Confirmed via `git show` on every test file in the branch — no leftover `package library`. `internal/library/scan/batch_seed_internal_test.go` (`package scan`) is untouched, correctly out of scope for this bead. 4. `scripts/test_policy_check/allowlist.txt` diff removes exactly one line, `internal/library/export_test.go`; the `internal/library/scan/batch_seed_internal_test.go` line is untouched/intact. 5. Production-code changes are limited to `internal/library/field_priority.go`: `getSettingsFieldProvider`, `getSettingsEnabledField`, and `fpSlotStr` each had their explicit `case "rating"` / `case 4` converted to `default`, removing the trailing unreachable `return nil`/`return ""`. Verified this is safe: `getSettingsFieldProvider`/`getSettingsEnabledField` have exactly two call sites, both in `buildLibraryFieldRows`, driven only by `def.key` from the fixed `libraryFieldDefs` slice (never external/request input) — confirmed via grep, only 2 call sites total. `fpSlotStr` call sites are the 4 literal `fpSlotStr(fp, 1..4)` calls. The separate, PUT-facing `setSettingsFieldProvider`/`setSettingsEnabledField` (which DO take external `key` values from the request body and must no-op on unknown keys) were NOT touched by this PR — their existing implicit no-op default is unchanged, and the new "body with all field keys ... plus an unrecognized key" test in `field_priority_test.go` correctly exercises that unchanged no-op path. No behavior change for any reachable caller. Minor observations (do not block): [MINOR] internal/library/handler_test.go:2041-2043 — the converted "GET /libraries/{id}/match-weights" `It("calls the Get dep and returns 200 when library exists", ...)` has two separate `Expect` calls (`Expect(getCalled).To(BeTrue())` and `Expect(resp.StatusCode).To(Equal(http.StatusOK))`) instead of one. Fold into a single `Expect(getCalled, resp.StatusCode).To(...)`-style compound assertion or split into two `It`s per project convention (project-conventions.md → Test structure: "exactly ONE Expect per It"). REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor commented 2026-08-12 20:19:06 +00:00

Author

Owner

Copy link

Adversarial security review of PR #1435 (bookshelf-v3bi0).

Scope: static/js/controllers/bookdrop_file_editor_controller.js + its Vitest spec only — the bookdrop:metadatacopied CustomEvent now dispatches on save success instead of optimistically.

Findings:

[MINOR] static/js/controllers/bookdrop_file_editor_controller.js:433 — _pendingDispatchCopy is a per-controller instance flag, not per-field
When a copy-triggered save collides with an in-flight save and a second _doSave call queues behind it (_savePendingAfterInflight), _pendingDispatchCopy is OR'd across whichever calls queued in that window (ctrl._pendingDispatchCopy || !!dispatchCopyEvent). This is a UI-correctness nit (the review row could auto-check slightly earlier/later than the exact field that succeeded), not a security issue — no cross-user or cross-proposal leak, since proposalIdValue on the dispatched event is scoped to the mounted controller's own row. No fix required before merge; flag only if the review-row auto-check behavior is later found flaky under rapid-copy usage.

Security assessment:

• No XSS / unsafe DOM injection: no innerHTML touched; the event detail.proposalId is echoed from an already-rendered proposalIdValue back into a same-page CustomEvent, never re-inserted into markup.
• No per-user/cross-user leak or ownership bypass: this changes only timing of a UI notification for an already-authorized, already-persisted per-request save; it does not change what data is fetched/written or which proposal it targets.
• No secret/token handling in this diff.
• Change is additive/gating only (fires strictly less often — after success, not optimistically) so it cannot newly expose state that wasn't already server-confirmed.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

Adversarial security review of PR #1435 (bookshelf-v3bi0). Scope: `static/js/controllers/bookdrop_file_editor_controller.js` + its Vitest spec only — the `bookdrop:metadatacopied` CustomEvent now dispatches on save success instead of optimistically. Findings: [MINOR] static/js/controllers/bookdrop_file_editor_controller.js:433 — `_pendingDispatchCopy` is a per-controller instance flag, not per-field When a copy-triggered save collides with an in-flight save and a second `_doSave` call queues behind it (`_savePendingAfterInflight`), `_pendingDispatchCopy` is OR'd across whichever calls queued in that window (`ctrl._pendingDispatchCopy || !!dispatchCopyEvent`). This is a UI-correctness nit (the review row could auto-check slightly earlier/later than the exact field that succeeded), not a security issue — no cross-user or cross-proposal leak, since `proposalIdValue` on the dispatched event is scoped to the mounted controller's own row. No fix required before merge; flag only if the review-row auto-check behavior is later found flaky under rapid-copy usage. Security assessment: - No XSS / unsafe DOM injection: no `innerHTML` touched; the event `detail.proposalId` is echoed from an already-rendered `proposalIdValue` back into a same-page CustomEvent, never re-inserted into markup. - No per-user/cross-user leak or ownership bypass: this changes only *timing* of a UI notification for an already-authorized, already-persisted per-request save; it does not change what data is fetched/written or which proposal it targets. - No secret/token handling in this diff. - Change is additive/gating only (fires strictly less often — after success, not optimistically) so it cannot newly expose state that wasn't already server-confirmed. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor added 1 commit 2026-08-13 01:13:04 +00:00

Merge branch 'main' into bd-bookshelf-nnb9.13 6d5b5aa1eb

zombor merged commit be70722abb into main 2026-08-13 01:22:00 +00:00

zombor referenced this pull request from a commit 2026-08-13 01:22:04 +00:00

Merge pull request 'nnb9: black-box internal/library tests (bookshelf-nnb9.13)' (#1439) from bd-bookshelf-nnb9.13 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!1439

No description provided.