Skip to content

fix(miner): register worktree-allocator in purge-cli's right-to-be-forgotten sweep (#8320) - #8559

Closed
philluiz2323 wants to merge 1 commit into
JSONbored:mainfrom
philluiz2323:fix-worktree-allocator-purge-8320
Closed

fix(miner): register worktree-allocator in purge-cli's right-to-be-forgotten sweep (#8320)#8559
philluiz2323 wants to merge 1 commit into
JSONbored:mainfrom
philluiz2323:fix-worktree-allocator-purge-8320

Conversation

@philluiz2323

@philluiz2323 philluiz2323 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

packages/loopover-miner/lib/worktree-allocator.ts's worktree_slots table has a repo_full_name column, just like every other repo-scoped local store, but it was absent from purge-cli.ts's right-to-be-forgotten sweep (REAL_PURGE_TARGETS) — the same recurring gap class already fixed for other stores (#7091, #6599, #8009).

worktree_slots is a fixed pool of maxConcurrency pre-allocated rows (slot_index is the primary key and every slot index must always exist), not an append-only ledger, so the generic purgeStoreByRepo helper (a hard DELETE) is the wrong shape here — it would shrink the pool below maxConcurrency and break ensureSlots'/selectFreeSlot's invariant. This PR follows governor-state.ts's own precedent for a non-uniform store: a hand-written purgeByRepo method directly on the WorktreeAllocator object.

  • Added WorktreeAllocator.purgeByRepo(repoFullName): number — an UPDATE, never a DELETE, that only clears a row where status = 'free' AND repo_full_name = ?. An active slot's row (a live, currently-running attempt's real worktree checkout) is never touched or counted — force-clearing it would desync the allocator from the live checkout on disk.
  • Registered worktree-allocator in purge-cli.ts's REAL_PURGE_TARGETS with no spec/specs field (its purge logic lives on the store object itself), mirroring how governor-state's entry carries specs instead of a single spec for its own non-uniform case.
  • Added a countDryRun extension point to PurgeTarget + runPurgeDryRun for stores whose dry-run preview can't be expressed as a generic LedgerPurgeSpec; worktree-allocator's own dry-run query matches the real purge's status = 'free' AND repo_full_name = ? condition exactly, so --dry-run never reports an active slot as purgeable.
  • Every existing miner-purge-cli.test.ts fixture referencing the "twelve real stores" / toHaveLength(12) / a full REAL_PURGE_TARGETS-shaped options object is updated to the new thirteenth store.

Scope

  • packages/loopover-miner/** only (plus its test files) — not Codecov-gated per this issue's own text.
  • Narrow, single-purpose change; no unrelated refactors.

Validation

  • npm --workspace @loopover/miner run build:tsc — clean
  • npm run typecheck — clean
  • npx vitest run test/unit/miner-worktree-allocator.test.ts test/unit/miner-purge-cli.test.ts test/unit/miner-worktree-allocator-collisions.test.ts test/unit/miner-worktree-allocator-lease-expiry.test.ts test/unit/miner-attempt-cli.test.ts — all green except 4 pre-existing, unrelated Windows-local-only failures (path-separator string assertions expecting /, a chmod-permission-bit assertion Windows doesn't enforce, and two EBUSY: resource busy or locked temp-dir cleanup races) — reproduced identically on a git stash'd clean baseline before this change, so confirmed environment noise, not a regression.
  • git diff --check upstream/main HEAD — clean, no trailing whitespace.
  • ui:* checks — skipped (backend/CLI-only change, no apps/gittensory-ui files touched).

New coverage

  • purgeByRepo clears a free slot carrying a stale repo_full_name and counts it (seeded directly, bypassing the normal release path since normal operation never leaves one stale).
  • purgeByRepo never touches an active slot for the target repo — 0 purged, row unchanged.
  • purgeByRepo returns 0 when no slot matches the repo.
  • purge-cli's dry-run + real purge both exercise the new store end-to-end (free-stale-row counted, active-row excluded, file-doesn't-exist-yet, corrupted-file error path, real on-disk default-path open/close).

Safety

  • No secrets, tokens, wallets, hotkeys/coldkeys, trust scores, or reward/payout values in code, comments, tests, or this PR text.
  • No auth/session/CORS surface touched.
  • Destructive-by-default risk addressed directly: the whole point of this change is that the new purge path can only update a free slot, never delete a row or touch a live attempt's active checkout — covered by the two invariant tests above.

Notes

Closes #8320

…rgotten sweep (#8320)

worktree_slots carries repo_full_name like every other repo-scoped store, but was absent from purge-cli's REAL_PURGE_TARGETS. Adds WorktreeAllocator.purgeByRepo, a hand-written UPDATE (not the generic purgeStoreByRepo DELETE, which would shrink the fixed slot pool) that only ever clears a free slot's stale repo_full_name -- an active slot's live worktree checkout is never touched. Registers the store in REAL_PURGE_TARGETS with its own dry-run counting query matching the same free-only match condition.
@philluiz2323
philluiz2323 requested a review from JSONbored as a code owner July 24, 2026 20:53
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.77%. Comparing base (12aa3e6) to head (996ac25).

Files with missing lines Patch % Lines
packages/loopover-miner/lib/purge-cli.ts 0.00% 6 Missing ⚠️
packages/loopover-miner/lib/worktree-allocator.ts 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8559      +/-   ##
==========================================
- Coverage   89.58%   88.77%   -0.81%     
==========================================
  Files          97       99       +2     
  Lines       22706    22912     +206     
  Branches     3872     3930      +58     
==========================================
  Hits        20341    20341              
- Misses       2187     2393     +206     
  Partials      178      178              
Flag Coverage Δ
backend 0.00% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/loopover-miner/lib/worktree-allocator.ts 0.00% <0.00%> (ø)
packages/loopover-miner/lib/purge-cli.ts 0.00% <0.00%> (ø)

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 24, 2026
@loopover-orb

loopover-orb Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - reject/close recommended

Review updated: 2026-07-24 20:58:58 UTC

5 files · 1 AI reviewer · 1 blocker · CI failing · unstable

🛑 Suggested Action - Reject/Close

Review summary
This PR adds a hand-rolled `purgeByRepo` UPDATE to worktree-allocator.ts (only touching free slots, never active ones) and registers it in purge-cli.ts's REAL_PURGE_TARGETS via a new `countDryRun` extension point, since the fixed-pool `worktree_slots` table can't use the generic DELETE-based `purgeStoreByRepo` helper. The reasoning is sound and matches the existing governor-state precedent for non-uniform stores, the active-slot exclusion is correctly guarded by `status = 'free'` in both the real purge and dry-run count queries, and tests cover the free-slot-cleared, active-slot-untouched, and no-match cases directly against the real allocator. The codecov/patch failure (0.00% of diff) is real but unexplained by the diff itself since the test files here do appear to add real coverage of the new code paths — likely a coverage-tool/config issue rather than a code defect.

Nits — 4 non-blocking
  • purge-cli.ts:117 `countWorktreeAllocatorFreeByRepo` duplicates the SQL condition already encoded in worktree-allocator.ts's `purgeFreeByRepo` prepared statement; consider deriving the dry-run count from a shared constant or exported query fragment so the two can't drift.
  • worktree-allocator.ts's `purgeByRepo` normalizes `repoFullName` via `normalizeRepoFullName` (which throws on malformed input), but purge-cli.ts's `purgeOneStore` doesn't special-case that thrown error versus other store failures — worth confirming the generic try/catch in `purgeOneStore` reports it clearly enough for an operator.
  • Consider extracting the `status = 'free' AND repo_full_name = ?` predicate into a shared constant between worktree-allocator.ts and purge-cli.ts to guarantee the dry-run count and the real purge can never diverge.
  • The PR description mentions fix(miner): worktree-allocator.ts's worktree_slots (has repo_full_name) is absent from purge-cli.ts's right-to-be-forgotten sweep #8320 is the issue being closed — worth double-checking that issue number is accurate and linked, since I can't verify issue-tracker state from the diff alone.

Why this is blocked

  • No linked issue detected: No closing reference or linked issue number was found in the PR metadata/body. — If this PR is intended to solve an issue, link it explicitly in the PR body.
📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. No linked issue detected: No closing reference or linked issue number was found in the PR metadata/body. — If this PR is intended to solve an issue, link it explicitly in the PR body.

CI checks failing

  • codecov/patch — 0.00% of diff hit (target 99.00%)

Decision drivers

  • ❌ Code review — 1 blocker (1 reviewer)
  • ❌ Gate result — Blocking (Repo-configured hard blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ No-issue rationale PR body explains why no issue is linked.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 1037 registered-repo PR(s), 610 merged, 126 issue(s).
Contributor context ✅ Confirmed Gittensor contributor philluiz2323; Gittensor profile; 1037 PR(s), 126 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Review context
  • Author: philluiz2323
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, JavaScript, MDX, TypeScript, CSS, Cuda, HTML, Kotlin
  • Official Gittensor activity: 1037 PR(s), 126 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask 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.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

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.

  • Re-run LoopOver review

@loopover-orb

loopover-orb Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (CI is failing (codecov/patch); 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.

@loopover-orb loopover-orb Bot closed this Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(miner): worktree-allocator.ts's worktree_slots (has repo_full_name) is absent from purge-cli.ts's right-to-be-forgotten sweep

1 participant