Skip to content

feat(miner): roll the SqliteDriver seam onto four non-transactional local stores#7552

Closed
shin-core wants to merge 1 commit into
JSONbored:mainfrom
shin-core:feat/sqlite-driver-seam-7282-v3
Closed

feat(miner): roll the SqliteDriver seam onto four non-transactional local stores#7552
shin-core wants to merge 1 commit into
JSONbored:mainfrom
shin-core:feat/sqlite-driver-seam-7282-v3

Conversation

@shin-core

Copy link
Copy Markdown
Contributor

What

Continues the #7175 SqliteDriver seam rollout (#7282): migrates the genuinely-non-transactional AMS local stores onto the same additive, still-synchronous openLocalStoreAdapter / driver.query pattern already proven by run-state.js (#7194) and policy-doc-cache.js.

Migrated (4):

  • lib/contribution-profile-cache.js
  • lib/policy-verdict-cache.js
  • lib/prediction-ledger.js
  • lib/replay-snapshot.js

For each: the store is opened via openLocalStoreAdapter(resolvedPath) (returning { db, driver }), and all CRUD (SELECT/INSERT/UPDATE/DELETE) now routes through driver.query(sql, params). Schema creation, migrations, retention, and purge stay on the underlying db handle exactly as before. The public store APIs remain synchronous and unchanged — this is purely additive, mirroring the reference stores.

Verified out of scope (left unmigrated, on purpose)

Per the deliverable's own "verify each individually for hidden transaction/RETURNING usage" caveat, two of the six listed stores are not actually non-transactional and belong to the separate runOn(client) interactive-transaction design the issue tracks in its next checkbox:

  • ranked-candidates.jsreplaceAll is a genuine BEGIN IMMEDIATE → DELETE + multi-INSERT → COMMIT/ROLLBACK atomic block, which the current driver.query seam can't express.
  • worktree-allocator.jsacquire is an interactive BEGIN IMMEDIATE read-then-conditional-write transaction, and releaseByAttempt uses UPDATE … RETURNING …, which store-db-adapter.js's own doc explicitly flags as not-yet-safe to route through the seam.

Migrating either would silently break an atomic path, so both are correctly deferred.

No behavior change

The SQL, parameters, return shapes, and synchronous API of every migrated store are byte-identical — only the handle CRUD goes through the seam instead of a raw DatabaseSync prepared statement. Proof: all six stores' existing test suites pass completely unmodified (90/90), including the two unmigrated ones.

Validation

  • npm run typecheck — clean (exit 0, zero errors) after a fresh @loopover/engine build.
  • All six store test files pass unchanged: miner-contribution-profile-cache, miner-policy-verdict-cache, miner-prediction-ledger, miner-replay-snapshot, miner-ranked-candidates, miner-worktree-allocator-collisions — 90/90.
  • Coverage: every changed CRUD-routing line in the 4 migrated files is covered by those existing tests (the only uncovered lines in the report are pre-existing untested paths I did not touch — e.g. prediction-ledger's corrupted-row throw and default-singleton wrappers).
  • npm run test:miner-pack passes; git diff --check clean.

Closes #7282

@shin-core
shin-core requested a review from JSONbored as a code owner July 20, 2026 21:47
@superagent-security superagent-security Bot added the contributor:flagged Contributor flagged for review by trust analysis. label Jul 20, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

🚨 Contributor flagged. Click here for more info: Superagent Dashboard

@superagent-security

Copy link
Copy Markdown
Contributor

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

@loopover-orb loopover-orb Bot added gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier. labels Jul 20, 2026
@loopover-orb

loopover-orb Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - fixes required

Review updated: 2026-07-20 21:58:56 UTC

12 files · 1 AI reviewer · no blockers · CI failing · blocked

🛑 Suggested Action - Fix Blockers

Review summary
This PR mechanically migrates four non-transactional local stores (contribution-profile-cache, policy-verdict-cache, prediction-ledger, replay-snapshot) from raw `db.prepare().run()/.get()/.all()` calls to `driver.query(sql, params)` returning `{ rows }`, following the exact pattern already established in run-state.js. The transformation is consistent and mechanically correct in every hunk I can see: params arrays match placeholder order, `rows[0]` correctly replaces `.get()`, and `.map()` over `rows` correctly replaces `.all()`. The PR correctly and explicitly excludes ranked-candidates.js and worktree-allocator.js for real transactional/RETURNING reasons, which is the right call. However, it ships with zero new or updated tests despite touching the core read/write path of four stores.

Nits — 6 non-blocking
  • No test changes accompany this refactor (0 test-path lines across all 12 changed files) — even though this claims to be a pure mechanical seam swap, a regression in `driver.query`'s param-binding or `{ rows }` shape (e.g. in `prediction-ledger.js` `readPredictions`'s conditional query) would not be caught without re-running each store's existing suite; confirm the PR description's 'all six stores' existing test suites pass completely' claim actually re-exercises these four stores' CRUD paths, not just the schema/migration code paths.
  • packages/loopover-miner/lib/replay-snapshot.js:132 and .ts:210 introduce a bare `feat(miner): freeze/snapshot mechanism for historical replay targets #3010` issue reference in a new doc comment with no named constant — consistent with existing conventions in the file so low value to change, but flagging per convention scan.
  • The `asPredictionDbRow`/row-casting helpers (`prediction-ledger.ts:179`) now take `Record<string, unknown>` instead of `Record<string, SQLOutputValue>`, widening the type — verify `driver.query`'s row type doesn't lose useful compile-time guarantees other callers relied on.
  • Add or confirm a test run specifically exercising `appendPrediction`/`readPredictions`, `getSnapshot`/`saveSnapshot`, `get`/`put` on the two caches through the new `driver.query` seam post-migration, not just pre-migration prepared-statement tests.
  • Consider one changelog/PR-description line clarifying which existing test files were re-run against these four stores to substantiate the 'byte-identical, tests pass' claim, since the diff itself shows no test file changes.
  • Code changes lack test evidence — Add focused regression tests or explain why existing coverage is sufficient.

CI checks failing

  • validate
  • validate-tests (5)
  • validate-tests (4)
  • validate-tests (2)
  • validate-tests (1)
  • validate-tests (3)
  • validate-code
Flagged checks (non-blocking)
  • Contributor trust — Contributor flagged for review

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #7282
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 (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 75 registered-repo PR(s), 47 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor shin-core; Gittensor profile; 75 PR(s), 0 issue(s).
Improvement ℹ️ None detected risk: low · value: none · LLM: moderate
Linked issue satisfaction

Partially addressed
The PR migrates four of the six explicitly-listed non-transactional stores (contribution-profile-cache, policy-verdict-cache, prediction-ledger, replay-snapshot) onto the SqliteDriver seam following the proven pattern, and gives well-reasoned justification for deferring ranked-candidates.js and worktree-allocator.js due to real transaction/RETURNING usage per the issue's own caveat, but it does no

Review context
  • Author: shin-core
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: TypeScript, JavaScript, Solidity, Dart, Python, CSS, PHP, Rust
  • Official Gittensor activity: 75 PR(s), 0 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 20, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (CI is failing (validate, validate-tests (5), validate-tests (4), validate-tests (2), validate-tests (1), validate-tests (3), validate-code)). 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 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:flagged Contributor flagged for review by trust analysis. gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(miner): finish rolling the SqliteDriver seam onto AMS's remaining local stores

1 participant