test(db): cover pruneExpiredRecords' two defensive ?? 0 fallback arms#8532
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
pruneExpiredRecords guards two D1 driver anomalies with ?? 0 -- the dry-run count row and the delete-loop meta.changes -- and neither had direct coverage, while the identical pattern on the sibling dedupeSignalSnapshots already has dedicated tests. Adds three cases mirroring that precedent: a dry-run count query returning no row, one returning a null n, and a delete run() result lacking meta. Each asserts the fallback yields 0 rather than NaN. No production change. Closes JSONbored#8370
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-24 19:07:03 UTC
Review summary Nits — 4 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Summary
src/db/retention.ts'spruneExpiredRecordsguards two D1 driver anomalies with?? 0— the dry-run count row (line 75) and the delete-loopmeta.changes(line 85) — and neither had direct coverage, even though the identical pattern on the siblingdedupeSignalSnapshotsalready has dedicated tests (lines 236-264). This mirrors that precedent exactly rather than inventing a new mocking style.Three cases added:
row?.n ?? 0deleted === 0, notNaNn: nullrow?.n ?? 0deleted === 0run()result lacksmetaresult.meta?.changes ?? 0deleted === 0, notNaNWorth flagging: the line-85 guard prevents an infinite loop, not just a
NaNWhile revert-proofing I removed each
?? 0to confirm the tests actually detect its absence. Removing the line-75 guard fails the test cleanly, as expected. Removing the line-85 guard does something more serious:changesbecomesNaN, and since everyNaNcomparison is false, the loop's exit conditionnever fires — the batched delete loop spins forever. My local run had to be killed on timeout.
So that
?? 0is load-bearing beyond producing a tidy number, and the new test's failure mode for that arm is a hang rather than an assertion error. I'm noting this rather than quietly leaving it: a hang is a worse CI signal than a failure, and a maintainer may decide the loop deserves its ownNumber.isFiniteguard. I have not changed the loop — the issue is scoped to test coverage only, and adding a production guard here is a behavior decision that needs a maintainer's read.Closes #8370
Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #123) — a linked open issue is required for every contributor PR.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateIf any required check was skipped, explain why:
tsc --noEmitis clean andgit diff --checkpasses.codecov/patchhas no changed production lines to score; the scoped run emits a non-emptycoverage/lcov.infocontainingsrc/db/retention.ts(the suite imports it directly), satisfying the "Verify coverage report exists" step.test/unit/retention.test.ts's"dry-run reports eligible rows per table without deleting anything"and"deletes rows older than the window and keeps recent ones"fail on my local checkout. I verified they fail identically with my changes stashed, so they are environmental and untouched by this PR; my three new cases pass, andsrc/db/retention.tsis byte-unmodified (git diff --statempty).Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.UI Evidence
Not applicable — unit-test additions; no visible UI, frontend, docs, or extension change.