nnb9: black-box internal/stats tests (bookshelf-nnb9.29) #1442
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-nnb9.29"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Removes the
internal/stats/export_test.gore-export shim and convertshandler_internal_test.gofrom white-box (package stats) to black-box (package stats_test) — everyinternal/stats/*_test.gonow drives only public entry points (GetHandler,GetDashboard,GetReadingDebt,GetSurvivalCurve,ListSeriesProgress).render_test.goreplaceshandler_internal_test.go, covering every unexported template-data-building helper inhandler.go(bar-pct math,hourLabel/bucketLabel, heatmap grid, donut-slice adapters) by renderingGetHandler's real HTML path against a delimited plain-text template and asserting on the rendered body.completion_race_test.go,era_vs_rating_test.go,length_sweet_spot_test.godrive their row-mapping helpers throughGetDashboard's stubbed query deps.reading_debt_test.go/survival_curve_test.gocovercomputeReadingDebt/computeSurvivalCurveand the SQL/arg builders by capturing whatGetReadingDebt/GetSurvivalCurveactually send to a spy query func.series_progress_test.gocoversbuildLibraryFilter/buildSeriesProgressQueryviaListSeriesProgress, andfetchSeriesProgressWithQuery's pagination viaGetDashboard'slistSeriesProgress+seriesCursor/seriesQueryparams.fetchSeriesProgressWithQuery'slimitparam was dead code (GetDashboard's only call site always passed the fixeddefaultSeriesProgressLimit) — removed the unreachable default/clamp branches along with the param, since an unreachable branch is dead code per policy, not a reason to add a white-box test.Deletes both grandfathered
internal/statsentries fromscripts/test_policy_check/allowlist.txt, shrinking the burn-down list.Test plan
go build ./...go test ./internal/stats/...— 372 specs greengo test ./internal/stats/... -coverprofile=... -covermode=set— 99.0% (onlywire.go, excluded by the coverage gate, is uncovered)make test-policy-check— OK, no net-new white-box testsgolangci-lint run ./internal/stats/...— 0 issuesmake test— full unit suite greenCloses bead bookshelf-nnb9.29 on merge.
Security review (adversarial, diff-only) — PR #1442 / bookshelf-nnb9.29
Reviewed the full diff
origin/main...origin/bd-bookshelf-nnb9.29forinternal/stats, focused on the two production-code changes (series_progress.go,service.go) plus the white-box→black-box test conversion.DoS/Scale surface (limit clamp removal): Traced
fetchSeriesProgressWithQuery's only caller (GetDashboardinservice.go:324) and confirmed there is no HTTP-exposedseries_limit/tunable query param anywhere ininternal/handlerorinternal/stats— grepped forseries_limit,SeriesProgressLimit,maxSeriesProgressLimit,fetchSeriesProgressWithQueryrepo-wide.Limitis now hardcoded to thedefaultSeriesProgressLimitconstant (50) at the single call site (series_progress.go:241), so the removedif limit <= 0 { … }/if limit > max { … }clamp was genuinely dead code — no caller could ever have supplied a limit ≤0 or >200 to begin with. Confirmed bounded: the query is still capped at 50 rows per page with cursor pagination (series_progress.go:253). No regression.Per-user/library scoping: No scoping code was touched.
service.go's one-line diff only drops the trailinglimitargument from thefetchSeriesProgressWithQuerycall —userIDandcore.libraryIDsare passed through unchanged.series_progress.go's fail-closed guard (len(libIDs) == 0→ empty page, no unscoped DB query) is untouched. The former white-boxbuildLibraryFilter/ExportBuildLibraryFilterunit tests (asserting1=0fail-closed clause whenidsis nil/empty, andlibrary_id IN (?, ?)when populated) were preserved and re-expressed as black-box coverage via the publicstats.ListSeriesProgressinseries_progress_test.go— scoping behavior is still under test, just exercised through the public API per the black-box test policy.Exported symbols: Grepped the diff for
+func/+type/+var/+const— every addition is confined to_test.gofiles, all declaringpackage stats_test(verified acrosscompletion_race_test.go,era_vs_rating_test.go,reading_debt_test.go,series_progress_test.go,survival_curve_test.go,render_test.go,length_sweet_spot_test.go,audio_v2_test.go). No new exported production symbol was added toseries_progress.go/service.goto make this conversion pass.export_test.goandhandler_internal_test.gowere correctly deleted along with their correspondingscripts/test_policy_check/allowlist.txtentries (no orphaned allowlist grandfather).Secrets/PII: grepped the full diff for password/secret/token/apikey/PEM-block patterns — none found. No secrets in new test fixtures.
No BLOCKER, MAJOR, or MINOR findings. This is a clean, mechanical black-box conversion; the production-code delta is a legitimate dead-code removal, not a behavioral or security regression.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Adversarial review of PR #1442 (bookshelf-nnb9.29) — black-box conversion of internal/stats tests.
Phase 1 (spec compliance): matches the bead — export_test.go + handler_internal_test.go removed, render_test.go added, allowlist.txt trimmed to exactly the two removed files, all internal/stats test files declare
package stats_test.Phase 2 findings:
No BLOCKER/MAJOR/MINOR findings.
Verification notes (adversarial checks performed):
Scale/limit-clamp removal (the flagged risk) —
fetchSeriesProgressWithQuerydropped itslimit intparam and the<=0/>maxclamp logic (internal/stats/series_progress.go:219-260). Traced the only caller:GetDashboard(internal/stats/service.go:324) previously always passed the constantdefaultSeriesProgressLimit(50) — never a user/request-derived value (handler.go has no?series_limitquery param). So the clamp was genuinely dead code; the query is still bounded (fixedLimit: defaultSeriesProgressLimitat series_progress.go:241,LIMIT ?still present in buildSeriesProgressQuery).maxSeriesProgressLimit(200) was removed since nothing reads it. Not a Scale violation — confirmed viaseries_progress_test.go"when entries returned equal the fixed page size (50), sets next cursor" which exercises the fixed-50 path end-to-end through the publicGetDashboard.Anti-gaming (no unexported symbol newly exported) — diffed exported identifiers; production diff is limited to series_progress.go/service.go (comment + signature simplification only, no new exports). export_test.go (11
Export*shims) is fully deleted, not relocated/renamed.Real vs. vacuous coverage — spot-checked render_test.go (all 24
buildXxxBars/donut/mini-hist helpers from the old handler_internal_test.go have a correspondingDescribeblock, e.g.CATBAR:Sci-Fi:5:50|asserts a real relative-percentage computation, not an echoed stub value), series_progress_test.go, reading_debt_test.go, survival_curve_test.go — all drive production code via the public API (stats.ListSeriesProgress,stats.GetDashboard,stats.GetReadingDebt,stats.GetSurvivalCurve,stats.GetHandler) with spy/stub query funcs and assert on production-computed values (percentages, SQL text/arg positions, mapped rows). No guarded/tautological/self-validating patterns observed.allowlist.txt — diff is exactly the two removed-file lines (
internal/stats/export_test.go,internal/stats/handler_internal_test.go); no other lines touched.REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Adversarial security review of PR #1435 (bookshelf-v3bi0).
Scope:
static/js/controllers/bookdrop_file_editor_controller.js+ its Vitest spec only — thebookdrop:metadatacopiedCustomEvent now dispatches on save success instead of optimistically.Findings:
[MINOR] static/js/controllers/bookdrop_file_editor_controller.js:433 —
_pendingDispatchCopyis a per-controller instance flag, not per-fieldWhen a copy-triggered save collides with an in-flight save and a second
_doSavecall queues behind it (_savePendingAfterInflight),_pendingDispatchCopyis 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, sinceproposalIdValueon 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:
innerHTMLtouched; the eventdetail.proposalIdis echoed from an already-renderedproposalIdValueback into a same-page CustomEvent, never re-inserted into markup.REVIEW VERDICT: 0 blocker, 0 major, 1 minor
Adversarial review of PR #1442 (bookshelf-nnb9.29) — black-box conversion of internal/stats tests.
Phase 1 (spec compliance): matches the bead — export_test.go + handler_internal_test.go removed, render_test.go added, allowlist.txt trimmed to exactly the two removed files, all internal/stats test files declare
package stats_test.Phase 2 findings:
No BLOCKER/MAJOR/MINOR findings.
Verification notes (adversarial checks performed):
Scale/limit-clamp removal (the flagged risk) - fetchSeriesProgressWithQuery dropped its limit int param and the <=0 / >max clamp logic (internal/stats/series_progress.go:219-260). Traced the only caller: GetDashboard (internal/stats/service.go:324) previously always passed the constant defaultSeriesProgressLimit (50) - never a user/request-derived value (handler.go has no ?series_limit query param). So the clamp was genuinely dead code; the query is still bounded (fixed Limit: defaultSeriesProgressLimit at series_progress.go:241, LIMIT ? still present in buildSeriesProgressQuery). maxSeriesProgressLimit (200) was removed since nothing reads it. Not a Scale violation - confirmed via series_progress_test.go "when entries returned equal the fixed page size (50), sets next cursor" which exercises the fixed-50 path end-to-end through the public GetDashboard.
Anti-gaming (no unexported symbol newly exported) - diffed exported identifiers; production diff is limited to series_progress.go/service.go (comment + signature simplification only, no new exports). export_test.go (11 Export* shims) is fully deleted, not relocated/renamed.
Real vs. vacuous coverage - spot-checked render_test.go (all 24 buildXxxBars/donut/mini-hist helpers from the old handler_internal_test.go have a corresponding Describe block, e.g. CATBAR:Sci-Fi:5:50| asserts a real relative-percentage computation, not an echoed stub value), series_progress_test.go, reading_debt_test.go, survival_curve_test.go - all drive production code via the public API (stats.ListSeriesProgress, stats.GetDashboard, stats.GetReadingDebt, stats.GetSurvivalCurve, stats.GetHandler) with spy/stub query funcs and assert on production-computed values (percentages, SQL text/arg positions, mapped rows). No guarded/tautological/self-validating patterns observed.
allowlist.txt - diff is exactly the two removed-file lines (internal/stats/export_test.go, internal/stats/handler_internal_test.go); no other lines touched.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Adversarial review of PR #1442 (bookshelf-nnb9.29) — black-box conversion of internal/stats tests.
Phase 1 (spec compliance): matches the bead — export_test.go + handler_internal_test.go removed, render_test.go added, allowlist.txt trimmed to exactly the two removed files, all internal/stats test files declare
package stats_test.Phase 2 findings:
No BLOCKER/MAJOR/MINOR findings.
Verification notes (adversarial checks performed):
Scale/limit-clamp removal (the flagged risk) - fetchSeriesProgressWithQuery dropped its limit int param and the <=0 / >max clamp logic (internal/stats/series_progress.go:219-260). Traced the only caller: GetDashboard (internal/stats/service.go:324) previously always passed the constant defaultSeriesProgressLimit (50) - never a user/request-derived value (handler.go has no ?series_limit query param). So the clamp was genuinely dead code; the query is still bounded (fixed Limit: defaultSeriesProgressLimit at series_progress.go:241, LIMIT ? still present in buildSeriesProgressQuery). maxSeriesProgressLimit (200) was removed since nothing reads it. Not a Scale violation - confirmed via series_progress_test.go "when entries returned equal the fixed page size (50), sets next cursor" which exercises the fixed-50 path end-to-end through the public GetDashboard.
Anti-gaming (no unexported symbol newly exported) - diffed exported identifiers; production diff is limited to series_progress.go/service.go (comment + signature simplification only, no new exports). export_test.go (11 Export* shims) is fully deleted, not relocated/renamed.
Real vs. vacuous coverage - spot-checked render_test.go (all 24 buildXxxBars/donut/mini-hist helpers from the old handler_internal_test.go have a corresponding Describe block, e.g. CATBAR:Sci-Fi:5:50| asserts a real relative-percentage computation, not an echoed stub value), series_progress_test.go, reading_debt_test.go, survival_curve_test.go - all drive production code via the public API (stats.ListSeriesProgress, stats.GetDashboard, stats.GetReadingDebt, stats.GetSurvivalCurve, stats.GetHandler) with spy/stub query funcs and assert on production-computed values (percentages, SQL text/arg positions, mapped rows). No guarded/tautological/self-validating patterns observed.
allowlist.txt - diff is exactly the two removed-file lines (internal/stats/export_test.go, internal/stats/handler_internal_test.go); no other lines touched.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
654b4068d94f21e0d919