Skip to content

feat(miner): add schema-version migration runner across local stores - #5347

Closed
real-venus wants to merge 1 commit into
JSONbored:mainfrom
real-venus:feat/gittensory-miner-schema-versioning-v1
Closed

feat(miner): add schema-version migration runner across local stores#5347
real-venus wants to merge 1 commit into
JSONbored:mainfrom
real-venus:feat/gittensory-miner-schema-versioning-v1

Conversation

@real-venus

Copy link
Copy Markdown
Contributor

Summary

The miner's local SQLite stores only ever ran CREATE TABLE IF NOT EXISTS, with no user_version/migration mechanism at all — so an older on-disk database file was silently reused with a stale shape, unlike the main product's migrations/ discipline. This adds a lightweight, shared schema-versioning convention across every store.

New: packages/gittensory-miner/lib/schema-version.js (+ .d.ts) exporting applySchemaMigrations(db, migrations):

  • Each store's bootstrap schema is treated as version 1 (BASELINE_SCHEMA_VERSION); a store's migrations array holds only post-baseline changesmigrations[i] upgrades the schema from version i+1 to i+2.
  • The runner reads PRAGMA user_version, runs exactly the pending migrations in order, then stamps the new version — so opening an older file runs its outstanding migrations instead of continuing on an incompatible shape. Re-opening an up-to-date file runs none.
  • A pre-versioning file (user_version 0) already carries the baseline tables (the idempotent CREATE TABLE IF NOT EXISTS ran), so it advances from the baseline.
  • Pure control flow over an injected DatabaseSync handle — deterministic, no IO of its own beyond the PRAGMA read/write.

Wired into all seven stores: claim-ledger, event-ledger, governor-ledger, plan-store, run-state, laptop-init, portfolio-queue.

Portfolio-queue's existing ad-hoc leased_at ALTER is expressed as its first real migration (1 → 2) — kept defensive (checks table_info) so a version-0 file that already ran the pre-convention ad-hoc ALTER is not re-altered into a duplicate-column error. This both removes the ad-hoc branch and demonstrates the convention on a real migration.

Kept lightweight per the issue's boundary — no heavyweight migration framework.

Scope

  • Narrow, one coherent change — one shared runner + uniform per-store wiring + tests
  • In scope (packages/), no blockedPaths, no secrets/private terms
  • Hand-authored .d.ts companion added, matching the package's existing convention

Validation

  • npm run typecheck
  • npm run test:coverage (full unsharded suite)
  • New unit test test/unit/miner-schema-version.test.ts (in-memory DatabaseSync): baseline stamp with no migrations, full run in order on a pre-versioning file, idempotent re-apply, partial catch-up from an intermediate version, and coercion of an absent/non-integer/negative user_version
  • Every existing store test still passes (verified against a clean-main baseline: identical results — the Windows-only chmod/path test failures are pre-existing and unrelated); portfolio-queue's leased_at/markFailed tests confirm the migrated behavior is preserved

Safety

  • Behaviour-preserving: stores gain a user_version stamp and a migration hook; the only converted logic (portfolio-queue leased_at) keeps its exact defensive semantics
  • No secrets, tokens, wallets, trust scores, or reward values in code, tests, or this description

Closes #4832

@real-venus
real-venus requested a review from JSONbored as a code owner July 12, 2026 14:28
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.41%. Comparing base (870d47f) to head (3db86a8).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5347   +/-   ##
=======================================
  Coverage   94.40%   94.41%           
=======================================
  Files         550      551    +1     
  Lines       44116    44135   +19     
  Branches    14631    14631           
=======================================
+ Hits        41649    41668   +19     
  Misses       1792     1792           
  Partials      675      675           
Flag Coverage Δ
shard-1 43.77% <90.90%> (-0.24%) ⬇️
shard-2 34.05% <86.36%> (-0.59%) ⬇️
shard-3 32.19% <90.90%> (+0.64%) ⬆️
shard-4 31.30% <86.36%> (-0.12%) ⬇️
shard-5 33.07% <95.45%> (-0.09%) ⬇️
shard-6 43.65% <86.36%> (+0.29%) ⬆️

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

Files with missing lines Coverage Δ
packages/gittensory-miner/lib/claim-ledger.js 94.87% <100.00%> (+0.06%) ⬆️
packages/gittensory-miner/lib/event-ledger.js 87.32% <100.00%> (+0.18%) ⬆️
packages/gittensory-miner/lib/governor-ledger.js 98.00% <100.00%> (+0.04%) ⬆️
packages/gittensory-miner/lib/laptop-init.js 97.10% <100.00%> (+0.04%) ⬆️
packages/gittensory-miner/lib/plan-store.js 92.45% <100.00%> (+0.07%) ⬆️
packages/gittensory-miner/lib/portfolio-queue.js 95.60% <100.00%> (+0.04%) ⬆️
packages/gittensory-miner/lib/run-state.js 100.00% <100.00%> (ø)
packages/gittensory-miner/lib/schema-version.js 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot added the gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. label Jul 12, 2026
@loopover-orb

loopover-orb Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Caution

🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥

🛑 Gittensory review result - reject/close recommended

Review updated: 2026-07-12 14:52:54 UTC

10 files · 2 AI reviewers · 1 blocker · readiness 100/100 · CI green · unstable

🛑 Suggested Action - Reject/Close

  • AI reviewers agree on a likely critical defect: packages/gittensory-miner/lib/schema-version.js:42 applies multiple migrations before stamping once at line 51, so if migration N succeeds and migration N+1 throws, the next open reruns migration N against an already-mutated schema and can fail permanently
  • wrap the migration loop and final `PRAGMA user_version` in one transaction, or stamp after each successful migration with a transaction around each migration plus its stamp. — Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.

Review summary
This adds a shared, lightweight PRAGMA user_version-based migration runner (schema-version.js) and wires it into all seven local miner SQLite stores, converting portfolio-queue's ad-hoc leased_at ALTER into a real versioned migration. I traced applySchemaMigrations line-by-line against the exact 'downgrade' scenario the failing Orb review flags (current=3, target=2 for an older-code re-open): effective clamps to current so the loop body never executes, and the stamp only fires under `if (current < target)`, so `db.exec(PRAGMA user_version=...)` is unreachable when current > target — the function correctly returns `Math.max(current, target)` without downgrading, matching the 'never downgrades' test case that already exercises this exact scenario. The PRAGMA interpolation is safe (target is a computed integer, never caller text), and every store's baseline-stamp call site is correctly ordered after its `CREATE TABLE IF NOT EXISTS`.

Blockers

  • packages/gittensory-miner/lib/schema-version.js:42 applies multiple migrations before stamping once at line 51, so if migration N succeeds and migration N+1 throws, the next open reruns migration N against an already-mutated schema and can fail permanently; wrap the migration loop and final `PRAGMA user_version` in one transaction, or stamp after each successful migration with a transaction around each migration plus its stamp.
Nits — 5 non-blocking
  • The Gittensory Orb Review Agent CI check failed claiming a downgrade defect at schema-version.js:45, but tracing the guard (`if (current < target)`) and the existing 'never downgrades' test in test/unit/miner-schema-version.test.ts shows the stamp is correctly unreachable when current > target — worth a quick manual confirmation before treating that check as a real blocker.
  • schema-version.js:38-51 is duplicated as six near-identical `applySchemaMigrations(db, [])` call sites across claim-ledger.js, event-ledger.js, governor-ledger.js, laptop-init.js, plan-store.js, and run-state.js — intentional per-store wiring, but worth confirming that's the desired pattern versus a shared helper as more stores gain real migrations.
  • Consider a follow-up doc note in packages/gittensory-miner/README.md's 'Local storage' table calling out the new schema-version convention, since that table currently documents each store's file/table/env-var but not its migration state.
  • portfolio-queue.js:96-104 keeps the defensive `table_info` check inside the migration function, which is the right call for pre-convention files — worth a one-line comment in schema-version.js's own docstring cross-referencing this as the canonical example for future migrations.
  • packages/gittensory-miner/lib/portfolio-queue.js:82 now creates `leased_at` in the baseline table while also modeling it as migration 1→2, which works because the migration is defensive but makes the baseline-version convention harder to reason about for the next store migration.

Why this is blocked

  • packages/gittensory-miner/lib/schema-version.js:42 applies multiple migrations before stamping once at line 51, so if migration N succeeds and migration N+1 throws, the next open reruns migration N against an already-mutated schema and can fail permanently; wrap the migration loop and final `PRAGMA user_version` in one transaction, or stamp after each successful migration with a transaction around each migration plus its stamp.
Signal Result Evidence
Code review ❌ 1 blocker 2 reviewers, synthesized
Linked issue ✅ Linked #4832
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: 98 registered-repo PR(s), 54 merged, 8 issue(s).
Contributor context ✅ Confirmed Gittensor contributor real-venus; Gittensor profile; 98 PR(s), 8 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Improvement ✅ Minor risk: clean · value: minor — Code changes are accompanied by test evidence. LLM value judgment: moderate — The PR closes a real, previously-unaddressed gap (no user_version/migration mechanism at all across seven local stores) with a narrowly-scoped, well-tested shared runner rather than a heavyweight framework, and demonstrates the convention on one real migration (portfolio-queue's leased_at column).
Linked issue satisfaction

Addressed
The PR introduces a shared PRAGMA user_version-based migration runner and wires it into all seven listed local SQLite stores, converting the ad-hoc portfolio-queue ALTER into a real migration, directly satisfying the issue's ask for a lightweight, consistent schema-versioning convention with tests demonstrating migration-on-open behavior.

Review context
  • Author: real-venus
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 98 PR(s), 8 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.
[BETA] Chat with Gittensory

Ask Gittensory a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

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

Full command reference: https://gittensory.aethereal.dev/docs/gittensory-commands

🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb

loopover-orb Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

An AI reviewer flagged a likely defect, but its confidence was below this repository's configured close-confidence floor, so this is held for a maintainer to confirm instead of closing automatically. Resolve the flagged defect (see the review notes), or ask a maintainer to override.

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 12, 2026
The miner's local SQLite stores only ever ran CREATE TABLE IF NOT EXISTS,
with no user_version/migration mechanism — so an older on-disk file was
silently reused with a stale shape. Add a lightweight shared runner,
applySchemaMigrations(db, migrations), and wire it into every store
(claim-ledger, event-ledger, governor-ledger, plan-store, run-state,
laptop-init, portfolio-queue).

Each store's bootstrap schema is treated as version 1; a store's
migrations array holds only post-baseline changes (migrations[i] upgrades
version i+1 to i+2). The runner reads PRAGMA user_version, runs exactly the
pending migrations in order, and stamps the new version, so opening an
older file runs its outstanding migrations instead of continuing on an
incompatible shape. Pre-versioning files (user_version 0) already carry the
baseline tables, so they advance from the baseline. Kept lightweight per
the issue — no heavyweight migration framework.

Portfolio-queue's existing ad-hoc leased_at ALTER is expressed as its first
real migration (1 to 2), staying defensive so a file that already ran the
pre-convention ALTER is not re-altered into a duplicate-column error.

Closes JSONbored#4832
@real-venus
real-venus force-pushed the feat/gittensory-miner-schema-versioning-v1 branch from d6d561a to 3db86a8 Compare July 12, 2026 14:44

@JSONbored JSONbored left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closing:

packages/gittensory-miner/lib/schema-version.js:42 applies multiple migrations before stamping once at line 51, so if migration N succeeds and migration N+1 throws, the next open reruns migration N against an already-mutated schema and can fail permanently; wrap the migration loop and final `PRAGMA user_version` in one transaction, or stamp after each successful migration with a transaction around each migration plus its stamp.

@JSONbored JSONbored closed this Jul 12, 2026
JSONbored added a commit that referenced this pull request Jul 12, 2026
…5378)

gateVerdictReason() fell back to gate.summary for the top "Suggested
Action" reason line -- but evaluateGateCheckCore's summary for a
"failure" conclusion is literally gate.blockers restated as one joined
string (title + action per finding). The "Why this is blocked" section
a few lines below independently renders those SAME gate.blockers
(folded in via buildDualReviewNotes), so every blocked/reject PR
comment printed the identical blocker text twice.

The existing test suite's gate() mock always hand-set a short generic
summary ("A hard blocker was found."), never the realistic
blockers-restated value evaluateGateCheckCore actually produces, so
this never got caught locally -- it surfaced live on PR #5347.

Only fall back to gate.summary/title when gate.blockers is empty (the
neutral/held case, which has no "Why this is blocked" section to
duplicate against and is the only state that still needs a top-level
reason).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. manual-review Gittensor contributor context

Development

Successfully merging this pull request may close these issues.

Add schema versioning to the local SQLite stores

2 participants