From a6080c6d0a989065a603d6ac8ad9fb001aa52efd Mon Sep 17 00:00:00 2001 From: kai392 Date: Thu, 30 Jul 2026 03:59:31 +0800 Subject: [PATCH] fix(miner): drift-guard doctor store-integrity list against migrate STORES (#9689) The `migrate` CLI's STORES list and doctor's store-integrity sweep must cover the same durable local stores, but nothing enforced it -- a new store added to one and not the other would silently drift (as orb-export #8318 and laptop-state #8641 both did before being caught by hand). - status.ts: derive STORE_INTEGRITY_NAMES from a single STORE_INTEGRITY_RESOLVERS table (behavior-preserving; same check names, same order) and export it. - migrate-cli.ts: export STORES; refresh the stale store-count comments. - miner-migrate-cli.test.ts: assert STORES names and STORE_INTEGRITY_NAMES are set-equal (both directions), with a pinned sorted list so any future single-sided addition fails CI. Closes #9689 Co-Authored-By: Claude Opus 4.8 --- packages/loopover-miner/lib/migrate-cli.ts | 9 ++-- packages/loopover-miner/lib/status.ts | 62 +++++++++++++--------- test/unit/miner-migrate-cli.test.ts | 45 +++++++++++++++- 3 files changed, 86 insertions(+), 30 deletions(-) diff --git a/packages/loopover-miner/lib/migrate-cli.ts b/packages/loopover-miner/lib/migrate-cli.ts index e71ce66304..530d9c90b2 100644 --- a/packages/loopover-miner/lib/migrate-cli.ts +++ b/packages/loopover-miner/lib/migrate-cli.ts @@ -3,8 +3,9 @@ // whatever command happens to touch it first -- this command instead lets an operator PROACTIVELY bring every // known store's EXISTING on-disk file up to date in one pass (e.g. right after upgrading, or before starting a // fleet), without needing to guess which command happens to touch which store first. Mirrors status.js's -// storeIntegrityChecks [name, resolve*DbPath(env)] store list exactly (same eleven stores `doctor` already -// covers, #6768), but actually OPENS each store (rather than a read-only integrity probe) so its real open/init +// storeIntegrityChecks [name, resolve*DbPath(env)] store list exactly (the same stores `doctor` already +// covers, #6768 -- drift-guarded against STORE_INTEGRITY_NAMES in miner-migrate-cli.test.ts, #9689), but +// actually OPENS each store (rather than a read-only integrity probe) so its real open/init // function's migration path runs for real. A store file that does not exist yet is skipped, not created -- // "migrate" brings existing files up to date; it is not another way to bootstrap fresh state (that's `init`). import { existsSync } from "node:fs"; @@ -50,7 +51,7 @@ export type MigrateStoreDescriptor = { open: (dbPath: string) => { close: () => void }; }; -const STORES: MigrateStoreDescriptor[] = [ +export const STORES: MigrateStoreDescriptor[] = [ { name: "event-ledger", resolveDbPath: resolveEventLedgerDbPath, open: initEventLedger }, { name: "governor-ledger", resolveDbPath: resolveGovernorLedgerDbPath, open: initGovernorLedger }, { name: "prediction-ledger", resolveDbPath: resolvePredictionLedgerDbPath, open: initPredictionLedger }, @@ -156,7 +157,7 @@ function migrateStore({ name, resolveDbPath, open }: MigrateStoreDescriptor, env } /** `stores` is injectable so tests can exercise a store descriptor's failure paths (e.g. a non-Error throw) - * without depending on real node:sqlite error shapes; defaults to the real seven-store list. */ + * without depending on real node:sqlite error shapes; defaults to the real `STORES` list. */ export function runMigrateChecks( env: Record = process.env, stores: MigrateStoreDescriptor[] = STORES, diff --git a/packages/loopover-miner/lib/status.ts b/packages/loopover-miner/lib/status.ts index d2d9a56955..e0bab4043e 100644 --- a/packages/loopover-miner/lib/status.ts +++ b/packages/loopover-miner/lib/status.ts @@ -385,35 +385,47 @@ function checkStateDirWritable(stateDir: string): DoctorCheck { } } +/** Every durable local SQLite store `doctor` integrity-checks, paired with its resolveLocalStoreDbPath + * resolver. This is the single source `STORE_INTEGRITY_NAMES` below is derived from, and the twin of + * migrate-cli.js's `STORES` list (#6768) — the two must hold the same store names (drift-guarded in + * test/unit/miner-migrate-cli.test.ts, #9689). */ +const STORE_INTEGRITY_RESOLVERS: ReadonlyArray) => string]> = [ + ["event-ledger", resolveEventLedgerDbPath], + ["governor-ledger", resolveGovernorLedgerDbPath], + ["prediction-ledger", resolvePredictionLedgerDbPath], + ["portfolio-queue", resolvePortfolioQueueDbPath], + ["claim-ledger", resolveClaimLedgerDbPath], + ["run-state", resolveRunStateDbPath], + ["plan-store", resolvePlanStoreDbPath], + ["governor-state", resolveGovernorStateDbPath], + ["attempt-log", resolveAttemptLogDbPath], + ["replay-snapshot", resolveReplaySnapshotDbPath], + ["worktree-allocator", resolveWorktreeAllocatorDbPath], + ["contribution-profile", resolveContributionProfileCacheDbPath], + ["policy-verdict-cache", resolvePolicyVerdictCacheDbPath], + ["policy-doc-cache", resolvePolicyDocCacheDbPath], + ["ranked-candidates", resolveRankedCandidatesDbPath], + ["deny-hook-synthesis", resolveDenyHookSynthesisDbPath], + // #8318: orb-export.sqlite3 (the opt-in Orb telemetry export's HMAC secret + cursor, #4277/#5681) is a + // durable local store like every entry above, but was never added when it shipped. + ["orb-export", resolveOrbExportDbPath], + // #8641: laptop-state.sqlite3 (laptop-mode bootstrap meta) uses the same resolveLocalStoreDbPath + + // applySchemaMigrations pattern, but was never added to either twin list when it shipped. + ["laptop-state", resolveLaptopStateDbPath], +]; + +/** The durable local SQLite store NAMES `doctor` integrity-checks (#9689). Derived from + * {@link STORE_INTEGRITY_RESOLVERS} (never a duplicated literal) and exported so migrate-cli's `STORES` + * twin list can be drift-guarded against it in a test. */ +export const STORE_INTEGRITY_NAMES: readonly string[] = STORE_INTEGRITY_RESOLVERS.map(([name]) => name); + /** Per-store `PRAGMA integrity_check` sweep for `doctor` (#4834) — flags a corrupted store instead of probing * only one with `SELECT 1`. A store file that does not exist yet is healthy by absence. Keep in sync with * migrate-cli.js's `STORES` list (#6768): every durable local SQLite store using resolveLocalStoreDbPath. */ function storeIntegrityChecks(env: Record): DoctorCheck[] { - const stores: Array<[string, string]> = [ - ["event-ledger", resolveEventLedgerDbPath(env)], - ["governor-ledger", resolveGovernorLedgerDbPath(env)], - ["prediction-ledger", resolvePredictionLedgerDbPath(env)], - ["portfolio-queue", resolvePortfolioQueueDbPath(env)], - ["claim-ledger", resolveClaimLedgerDbPath(env)], - ["run-state", resolveRunStateDbPath(env)], - ["plan-store", resolvePlanStoreDbPath(env)], - ["governor-state", resolveGovernorStateDbPath(env)], - ["attempt-log", resolveAttemptLogDbPath(env)], - ["replay-snapshot", resolveReplaySnapshotDbPath(env)], - ["worktree-allocator", resolveWorktreeAllocatorDbPath(env)], - ["contribution-profile", resolveContributionProfileCacheDbPath(env)], - ["policy-verdict-cache", resolvePolicyVerdictCacheDbPath(env)], - ["policy-doc-cache", resolvePolicyDocCacheDbPath(env)], - ["ranked-candidates", resolveRankedCandidatesDbPath(env)], - ["deny-hook-synthesis", resolveDenyHookSynthesisDbPath(env)], - // #8318: orb-export.sqlite3 (the opt-in Orb telemetry export's HMAC secret + cursor, #4277/#5681) is a - // durable local store like every entry above, but was never added when it shipped. - ["orb-export", resolveOrbExportDbPath(env)], - // #8641: laptop-state.sqlite3 (laptop-mode bootstrap meta) uses the same resolveLocalStoreDbPath + - // applySchemaMigrations pattern, but was never added to either twin list when it shipped. - ["laptop-state", resolveLaptopStateDbPath(env)], - ]; - return stores.map(([name, dbPath]) => checkStoreIntegrity(`store-integrity:${name}`, dbPath)); + return STORE_INTEGRITY_RESOLVERS.map(([name, resolveDbPath]) => + checkStoreIntegrity(`store-integrity:${name}`, resolveDbPath(env)), + ); } /** Validate the discovered `.loopover-miner` config's CONTENT (#4873), not just its path: parse it with the diff --git a/test/unit/miner-migrate-cli.test.ts b/test/unit/miner-migrate-cli.test.ts index a8041c91d3..6ae78edda1 100644 --- a/test/unit/miner-migrate-cli.test.ts +++ b/test/unit/miner-migrate-cli.test.ts @@ -3,7 +3,8 @@ import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { DatabaseSync } from "node:sqlite"; import { afterEach, describe, expect, it, vi } from "vitest"; -import { runMigrate, runMigrateChecks } from "../../packages/loopover-miner/lib/migrate-cli"; +import { runMigrate, runMigrateChecks, STORES } from "../../packages/loopover-miner/lib/migrate-cli"; +import { STORE_INTEGRITY_NAMES } from "../../packages/loopover-miner/lib/status"; import { initPortfolioQueueStore, resolvePortfolioQueueDbPath } from "../../packages/loopover-miner/lib/portfolio-queue"; import { resolveEventLedgerDbPath } from "../../packages/loopover-miner/lib/event-ledger"; import { applySchemaMigrations, BASELINE_SCHEMA_VERSION } from "../../packages/loopover-miner/lib/schema-version"; @@ -326,3 +327,45 @@ describe("loopover-miner migrate (#4871)", () => { expect(fetchSpy).not.toHaveBeenCalled(); }); }); + +describe("STORES / STORE_INTEGRITY_NAMES drift guard (#9689)", () => { + // The two hand-maintained twin lists -- migrate OPENS each store, doctor integrity-checks each -- must hold + // the same store names. That contract was stated in both files' comments but enforced in neither, and was + // broken twice (#8318 orb-export, #8641 laptop-state). These assertions fail CI if a store is added to one + // list and not the other, or silently dropped from BOTH at once. + const EXPECTED_STORE_NAMES = [ + "attempt-log", + "claim-ledger", + "contribution-profile", + "deny-hook-synthesis", + "event-ledger", + "governor-ledger", + "governor-state", + "laptop-state", + "orb-export", + "plan-store", + "policy-doc-cache", + "policy-verdict-cache", + "portfolio-queue", + "prediction-ledger", + "ranked-candidates", + "replay-snapshot", + "run-state", + "worktree-allocator", + ]; + + it("the migrate STORES and doctor STORE_INTEGRITY_NAMES lists are equal as sets and in length", () => { + const migrateNames = STORES.map((store) => store.name); + const integrityNames = [...STORE_INTEGRITY_NAMES]; + expect(new Set(migrateNames)).toEqual(new Set(integrityNames)); + expect(migrateNames.length).toBe(integrityNames.length); + // No duplicate within either list (a set-equality alone would hide a dup masking a missing entry). + expect(new Set(migrateNames).size).toBe(migrateNames.length); + expect(new Set(integrityNames).size).toBe(integrityNames.length); + }); + + it("both lists match the pinned store-name set exactly, so a store dropped from BOTH at once still fails", () => { + expect([...STORES.map((store) => store.name)].sort()).toEqual(EXPECTED_STORE_NAMES); + expect([...STORE_INTEGRITY_NAMES].sort()).toEqual(EXPECTED_STORE_NAMES); + }); +});