Skip to content

miner(doctor): checkAmsBacktestProposals creates and retention-prunes the event ledger from a read-only command #10002

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

status and doctor declare themselves read-only at packages/loopover-miner/lib/status.ts:37-39:

// Slim laptop-mode CLI commands (#2288): `status` (what's installed + where local state lives) and `doctor` (is
// this laptop set up correctly). Both are read-only and 100% local — no repo-scanning, no coding-agent invocation,
// no GitHub writes, and no network calls of any kind.

The only write doctor documents is the state-dir probe (checkStateDirWritable,
packages/loopover-miner/lib/status.ts:369-386), which creates and immediately removes one file.

But runDoctorChecks also runs checkAmsBacktestProposals (packages/loopover-miner/lib/status.ts:586), which
opens the event ledger unconditionally at packages/loopover-miner/lib/status.ts:537-541:

export function checkAmsBacktestProposals(env: Record<string, string | undefined> = process.env, nowMs: number = Date.now()): DoctorCheck {
  try {
    const eventLedger = initEventLedger(resolveEventLedgerDbPath(env));
    try {
      const proposals = buildAmsBacktestProposals(readAmsThresholdBacktestRuns(eventLedger), nowMs);

initEventLedger (packages/loopover-miner/lib/event-ledger.ts:160-178) is not a read: it goes through
openLocalStoreDb (packages/loopover-miner/lib/local-store.ts:66-88, which mkdirSyncs the parent, constructs
new DatabaseSync(resolvedPath) — creating the file — and chmodSyncs it), then runs
CREATE TABLE IF NOT EXISTS miner_event_ledger …, then applySchemaMigrations(db, [addTenantIdColumn]) (which
writes PRAGMA user_version and can ALTER TABLE), and then:

  // Opt-in retention (#4834): prune aged/excess rows when an operator has enabled it; a no-op by default.
  pruneLedgerByRetention(db, EVENT_LEDGER_RETENTION_SPEC, resolveLedgerRetentionPolicy(), Date.now());

So two concrete things happen that doctor says do not:

  1. On a laptop that has never run discover/attempt, loopover-miner doctor creates
    event-ledger.sqlite3. That also corrupts doctor's own report: storeIntegrityChecks runs after it
    (packages/loopover-miner/lib/status.ts:587) and checkStoreIntegrity's "not created yet" branch
    (packages/loopover-miner/lib/store-maintenance.ts:117-119) can never be reached for event-ledger — the
    store is reported as an existing, healthy file that doctor itself just made.
  2. When an operator has set LOOPOVER_MINER_LEDGER_RETENTION_DAYS or
    LOOPOVER_MINER_LEDGER_RETENTION_MAX_ROWS (packages/loopover-miner/lib/store-maintenance.ts:19-20), running
    doctor deletes audit rows from the append-only ledger via pruneLedgerByRetention
    (packages/loopover-miner/lib/store-maintenance.ts:164-199). A diagnostic command must not mutate the audit
    trail it is diagnosing.

Note also that resolveLedgerRetentionPolicy() is called with no argument inside initEventLedger, so it reads
process.env — it does not even honour the env object checkAmsBacktestProposals was given.

The repo has already fixed exactly this class of bug once, in the same helper, for discover --dry-run
packages/loopover-miner/lib/discover-cli.ts:530-540:

      // #9679: --dry-run must make ZERO filesystem writes, but initEventLedger creates + migrates + prunes the
      // ledger file. On the dry-run path only read the override when the ledger file ALREADY exists (opening a
      // not-yet-existing SQLite file is itself a write, and retention pruning can delete rows) -- a missing file
      // falls back to the shipped default ... Same "skip a file that doesn't exist yet" discipline as migrate-cli.ts /
      // store-maintenance.ts.
      if (!parsed.dryRun || existsSync(ledgerDbPath)) {

checkAmsBacktestProposals has no such guard.

Requirements

  • checkAmsBacktestProposals must not create the event-ledger file. When
    resolveEventLedgerDbPath(env) does not exist, it must return { name: "ams-backtest-proposals", ok: true, detail: … } describing that there is nothing to propose, without opening any handle.
  • checkAmsBacktestProposals must not run schema migrations or retention pruning against an existing ledger. Read
    the persisted backtest runs through a driver-enforced read-only connection, the same
    new DatabaseSync(dbPath, { readOnly: true }) form checkStoreIntegrity
    (packages/loopover-miner/lib/store-maintenance.ts:122) and openClaimLedgerReadOnly
    (packages/loopover-miner/lib/claim-ledger.ts:400-416) already use — note the camelCase readOnly; the
    lowercase readonly key is silently ignored by node:sqlite and opens read-write.
  • The check must stay fail-open: any open/read failure still returns ok: true with an informational detail, as
    today at packages/loopover-miner/lib/status.ts:553-555. doctor must keep working on a box whose ledger is
    broken.
  • When the ledger exists and holds backtest-cleared proposals, the rendered detail string must be byte-identical
    to today's (packages/loopover-miner/lib/status.ts:545-549), including the trailing
    "-- nothing applies automatically (apply-min-rank needs the config flag AND --approve)" clause.
  • Do NOT change initEventLedger, pruneLedgerByRetention, or the retention env contract — the ledger's own
    open path must keep creating/migrating/pruning for the writers that legitimately call it.
  • Do NOT change checkStateDirWritable, which is a documented, intentional probe write.

⚠️ Required pattern: mirror packages/loopover-miner/lib/store-maintenance.ts:116-130
(checkStoreIntegrity) — existsSync short-circuit, new DatabaseSync(dbPath, { readOnly: true }), everything
wrapped so a failure degrades to a report rather than a throw. What does NOT satisfy this issue: (a) leaving
initEventLedger in place and merely adding an existsSync guard, which still migrates and retention-prunes an
existing ledger every time doctor runs; (b) adding a new readOnly flag parameter to initEventLedger and
threading it through every caller, a repo-wide change to a store every module opens; (c) deleting the
ams-backtest-proposals check from runDoctorChecks.

Deliverables

  • checkAmsBacktestProposals({ LOOPOVER_MINER_CONFIG_DIR: tmp }) against a temp dir with no
    event-ledger.sqlite3 returns ok: true and leaves existsSync(resolveEventLedgerDbPath(env)) false
    asserted in the existing checkAmsBacktestProposals (#8186) describe block at
    test/unit/miner-status.test.ts:583.
  • runDoctorChecks({ LOOPOVER_MINER_CONFIG_DIR: tmp }) against a temp dir with no stores leaves
    event-ledger.sqlite3 absent, and its store-integrity:event-ledger check reports the "not created yet"
    detail — asserted in test/unit/miner-status.test.ts.
  • With LOOPOVER_MINER_LEDGER_RETENTION_MAX_ROWS=1 set and an event ledger seeded with three events,
    checkAmsBacktestProposals(env) leaves all three rows in place (readEvents().length === 3 afterwards) —
    asserted in test/unit/miner-status.test.ts.
  • With an event ledger holding one backtest-cleared ams_threshold_backtest_run inside the lookback,
    checkAmsBacktestProposals(env, nowMs) still returns the exact same detail string it returns today —
    the assertion already at test/unit/miner-status.test.ts:608 must keep passing unchanged.
  • A regression test at test/unit/miner-status.test.ts named for this bug (e.g. REGRESSION: doctor does not create or prune the event ledger) that fails against the current code.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example one that
adds the missing-file guard but still opens an existing ledger read-write and prunes it — does not resolve this
issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's coverage.include lists
packages/loopover-miner/lib/**/*.ts, so status.ts is measured and gated. Both arms of every branch the change
introduces need a test: existsSync(dbPath) true and false; the proposals.length === 0 vs non-empty rendering
arms (already present at packages/loopover-miner/lib/status.ts:542-549); and the outer catch fail-open arm,
which must be reachable — exercise it with a path that exists but is not a SQLite database.

Expected Outcome

loopover-miner doctor stops being a writer: it no longer creates event-ledger.sqlite3 on a fresh laptop, no
longer runs schema migrations or retention deletions against an operator's audit trail, and its
store-integrity:event-ledger line reports the store's real pre-existing state instead of one doctor just
created.

Links & Resources

  • packages/loopover-miner/lib/status.ts:37-39 — the read-only contract doctor declares
  • packages/loopover-miner/lib/status.ts:537-556checkAmsBacktestProposals
  • packages/loopover-miner/lib/status.ts:586-587 — its position ahead of the store-integrity sweep
  • packages/loopover-miner/lib/event-ledger.ts:160-178initEventLedger's create + migrate + prune
  • packages/loopover-miner/lib/store-maintenance.ts:116-130checkStoreIntegrity, the read-only pattern to mirror
  • packages/loopover-miner/lib/discover-cli.ts:530-540 — the #9679 precedent for this exact helper

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions