fix(miner): purge portfolio-queue and run-state rows in the right-to-be-forgotten sweep - #6697
Conversation
…be-forgotten sweep (JSONbored#6599) loopover-miner purge --repo swept only four of the six local stores that persist repo_full_name. portfolio-queue.js and run-state.js were left untouched, with no warning -- unlike attempt-log.js, which is deliberately reported as not-purgeable because it genuinely has no repo column. An operator honoring a right-to-be-forgotten request kept that repo's rows in both stores and was told the purge succeeded. Both stores gain purgeByRepo built on the shared purgeStoreByRepo, two purge specs are added, and both are wired into REAL_PURGE_TARGETS so --dry-run and the real purge report them alongside the other four with no special-casing. Keeping repo_full_name in a PRIMARY KEY rather than a plain column never made them unpurgeable; they were simply never wired up. Closes JSONbored#6599
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
loopover-orb
Bot
added
the
gittensor:bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6697 +/- ##
=======================================
Coverage 93.65% 93.65%
=======================================
Files 682 682
Lines 68112 68117 +5
Branches 18708 18708
=======================================
+ Hits 63788 63793 +5
Misses 3347 3347
Partials 977 977
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Caution 🛑 LoopOver review result - reject/close recommendedReview updated: 2026-07-16 20:03:42 UTC
Review summary Nits — 5 non-blocking
Why this is blocked
📋 Copy for AI agents — paste into your coding agentCI checks failing
Decision drivers
Context & advisory signals — never blocks the verdict
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.
|
|
LoopOver is closing this pull request on the maintainer's behalf (CI is failing (validate-tests-merge, validate, validate-code, validate-tests (4), validate-tests (2), validate-tests (6), validate-tests (3), validate-tests (1), validate-tests (5)); conflicts with the base branch — resolve and open a fresh PR; No linked issue detected). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed. |
1 participant
Summary
loopover-miner purge --repo <owner/repo>is the operator-invoked right-to-be-forgotten path. Its own header comment claimed it covers "the four stores that have a realrepoColumn". Two more local stores persist rows keyed byrepo_full_nameand were not covered at all:portfolio-queue.js→miner_portfolio_queuerun-state.js→miner_run_stateNeither had a
purgeByRepo, neither had a purge spec, and neither appeared inREAL_PURGE_TARGETS. An operator honoring an erasure request left that repo's rows in both stores and was told the purge succeeded — no warning, no note.That silence is the sharp edge.
attempt-log.jsis deliberately reported as not-purgeable viaATTEMPT_LOG_NOT_PURGEABLE_NOTE, because it genuinely has no repo column. These two had no such structural excuse — they were simply never wired up, so they were omitted invisibly rather than declared.The change
Follows the existing four end-to-end, with no new machinery:
store-maintenance.js, same shape as their four siblings.purgeByRepoon each store, built on the sharedpurgeStoreByRepo, mirroringclaim-ledger.js's implementation — including throwing on a missing/malformedrepoFullNamerather than silently no-opping.REAL_PURGE_TARGETS, sorunPurgeDryRunandrunPurgepick them up automatically. No special-casing in either path.One thing worth stating explicitly, since it looks like a reason these were skipped: both keep
repo_full_nameinside their PRIMARY KEY rather than as a plain column. That's irrelevant to purging — aDELETEby repo works identically either way — so they're purgeable for exactly the same reason the other four are. The code comment records that, so the next reader doesn't re-derive it and conclude they were excluded on purpose.Type declarations — the part
tsccaught and the tests didn'tThis package hand-maintains
.d.tsfiles alongside the JS. Vitest doesn't typecheck, so the suites went green whilenpm run typecheckfailed onpurgeByReponot existing onPortfolioQueueStore/RunStateStore. Fixed in all four: both store types, the two new spec exports, andPurgeCliOptions(which declares the injectable openers and would otherwise have been an incomplete public contract for the two new stores).Tests
purgeByRepoblocks in both stores' suites, mirroringminer-prediction-ledger.test.ts's reference block: deletes only the targeted repo's rows, leaves other repos untouched, returns0on no match, and throws on a missing/malformedrepoFullName— the last one matters because a typo'd repo silently purging nothing would tell an operator their erasure request was honored when it wasn't.--dry-runnow seeds real rows in both stores and asserts{ store: "portfolio-queue", wouldPurge: 2 }/{ store: "run-state", wouldPurge: 1 }alongside the original four, with a second repo present so the counts prove the sweep is repo-scoped rather than a blanket delete.purgeByRepowas called on each, and that the total moved6 → 11.The updated four-store assertions are the issue's own deliverable ("asserting both
--dry-runand a real purge now reportportfolio-queueandrun-state"), not tests bent to fit the code: they enumerate the covered stores exactly, so extending coverage necessarily extends them. No assertion was weakened or removed.Validation
store-maintenance.js(5 lines),portfolio-queue.js(8),run-state.js(7),purge-cli.js(10): zero uncovered statements, zero partial branches on every one. Clears the 99%codecov/patchwall onpackages/loopover-miner/lib/**.npm run typecheck— 0 errors ·eslint— 0 errors/0 warnings ·npm run build:miner(node --checkacross every lib file) passes ·git diff --checkclean · rebased on latestmain, no base conflict.The 4 remaining local failures are pre-existing and not mine — verified, not assumed. They are
miner-portfolio-queue/miner-run-state's DB-path and file-mode tests, which fail identically on cleanmainwith my work stashed (Windows-only:\vs/separators, andstatSync().mode & 0o077returning54instead of0, since Windows has no POSIX permission bits). Same count, same tests, before and after.Scope
packages/,test/).purgeStoreByRepoitself, andattempt-log's deliberate not-purgeable note are untouched.site/,CNAME, orlovablechanges.Safety
--dry-runstill writes nothing; both new stores are counted through the same read-onlycountStoreByRepothe other four use.normalizeRepoFullNamethrow means a malformed repo argument fails loudly rather than reporting a successful purge of nothing.purgeOneStore's per-store try/catch already isolates failures, so a problem opening either new store cannot prevent the other five from being reported.Closes #6599