Skip to content

Commit ec8316a

Browse files
Yerazeclaude
andauthored
test(db): schema-drift tripwire — CI fails on createTables/migration divergence (#3962 task 1.6) (#3992)
Adds two files that permanently encode the current gap between the two schema-bootstrap paths so Phase 3.3 can delete createTables()/createIndexes() safely: - src/db/schemaDrift.allowlist.ts — 15-entry typed allowlist of known divergences (10 onlyInBootstrap, 3 onlyInReplay, 2 sqlMismatch), each with a Phase-3.3 resolution note. Sorted by key; can only shrink. - src/db/schemaDrift.test.ts — Vitest suite that builds Variant A (DatabaseService singleton → createTables + createIndexes + migration loop against a temp file) and Variant B (createTestDb() migration-replay only), normalizes sqlite_master DDL, diffs, and fails on any unexpected divergence OR any stale allowlist entry. Runtime ~1.7 s (well under the 5 s suite-file budget). Phase-3.3 action items encoded in the allowlist: - 7 createIndexes()-only indexes silently lost on replay-only install → add a migration creating each before deleting createIndexes(). - 3 index name-case pairs (camelCase bootstrap vs lowercase migration) → rename or drop the camelCase duplicates. - 2 column-order mismatches (createTables vs ALTER ADD COLUMN history) → accept or add a table-rebuild migration. Claude-Session: https://claude.ai/code/session_01AphLfgUJWaFNAgU5UXdJ4n Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent fc462d2 commit ec8316a

3 files changed

Lines changed: 276 additions & 2 deletions

File tree

docs/internal/dev-notes/REMEDIATION_EPIC.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,8 @@ Each unchecked phase = one worktree → architect spec → implementation → re
2424
- [x] **1.2** — Type-check the tests: `tsconfig.tests.json` (full strict — see phase log deviation), wired into CI non-blocking first; flip to blocking once clean.
2525
- [x] **1.3** — Integration-grade route-test harness: in-memory SQLite + real Express app with real `requirePermission`/session/auth wiring; 3–5 representative route test files converted (source-scoped permission coverage); no mass conversion.
2626
- [x] **1.4** — Lint ratchets: `no-explicit-any``error` with checked-in baseline; forbid raw `fetch(` in `src/components/**`/`src/pages/**` (baselined); `react-hooks/exhaustive-deps` + `prefer-const``error`.
27-
- [ ] **1.5** — Response-envelope convention: `ok(res, data)` / `fail(res, status, code, msg)` helper for the `{ success, error, code }` envelope; documented in CLAUDE.md; new/modified handlers must use it.
28-
- [ ] **1.6** — Schema-drift tripwire: CI test diffing `createTables()` schema vs full migration replay (001→latest), normalized `sqlite_master`, fail on divergence.
27+
- [x] **1.5** — Response-envelope convention: `ok(res, data)` / `fail(res, status, code, msg)` helper for the `{ success, error, code }` envelope; documented in CLAUDE.md; new/modified handlers must use it.
28+
- [x] **1.6** — Schema-drift tripwire: CI test diffing `createTables()` schema vs full migration replay (001→latest), normalized `sqlite_master`, fail on divergence.
2929

3030
## Ordering notes
3131

@@ -48,3 +48,5 @@ Record per-phase: PR link, deviations from plan, follow-ups.
4848
- **1.2** — PR TBD (tsconfig.tests.json + `typecheck:tests` script + non-blocking CI steps in ci.yml/pr-tests.yml). Deviations: (a) kept FULL strict incl. noImplicitAny — plan's "noImplicitAny off" injects ~50 false-positive errors into prod src via evolving-any inference; (b) top-level `tests/` dir NOT included — its files pull @types/node into the program and poison frontend inference (~60 spurious errors). Baseline: 283 errors, all in src test files, 0 prod. Flip-to-blocking when count reaches 0 (burndown is a follow-up; top ~8 files hold ~150 errors).
4949
- **1.3** — PR #3986 merged (createRouteTestApp() real-middleware harness + 4 conversions). Ride-alongs: real prod bug fixed (v1/messages GET 500'd without sourceId under fail-closed — missing ?? ALL_SOURCES) + harness anonymous-user race fix. 3 CodeQL alerts on the test fixture left open for human triage (recommend dismiss as used-in-tests).
5050
- **1.4** — PR #3989 merged (count-based lint ratchet, eslint-baseline.json 410 files/2,515 violations; no-explicit-any/exhaustive-deps/prefer-const → error; raw-fetch ban in components/pages; CI lint now BLOCKING — closes the 0.5 deviation). Census correction: 727 pre-existing errors (468 config false-positives fixed at config level, not baselined).
51+
- **1.5** — PR #3990 merged (ok()/fail() helpers + 2 exemplar conversions + CLAUDE.md convention; ApiService does-not-unwrap constraint documented). No deviations.
52+
- **1.6** — PR TBD (schemaDrift.test.ts + schemaDrift.allowlist.ts; 15-entry allowlist; census confirmed 15 divergences — 10 onlyInBootstrap, 3 onlyInReplay, 2 sqlMismatch — matching the architect spec exactly). Finding flagged for Phase 3.3: 7 createIndexes()-only indexes (idx_messages_createdAt/fromNodeId/toNodeId, idx_nodes_updatedAt, idx_route_segments_distance/recordholder/timestamp) are silently lost on a replay-only fresh install; Phase 3.3 must add migrations creating each before deleting createIndexes(). No deviations.

src/db/schemaDrift.allowlist.ts

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
/**
2+
* KNOWN divergences between the two schema-bootstrap paths (see schemaDrift.test.ts).
3+
* Each entry is drift that Phase 3.3 (single source of truth for schema bootstrap)
4+
* will remove. When Phase 3.3 reconciles an item, DELETE its entry here — the test
5+
* fails on BOTH unexpected new drift AND stale (no-longer-present) allowlist entries,
6+
* so the list can only shrink. Keep sorted by `key`.
7+
*/
8+
9+
export type DriftKind = 'onlyInBootstrap' | 'onlyInReplay' | 'sqlMismatch';
10+
export interface AllowedDrift { key: string; kind: DriftKind; reason: string; }
11+
12+
export const SCHEMA_DRIFT_ALLOWLIST: readonly AllowedDrift[] = [
13+
// --- (1) Index name-case drift: createIndexes() uses camelCase; migrations create lowercase ---
14+
// SQLite treats both as distinct objects, so variant A has both and variant B has only lowercase.
15+
// Phase 3.3: rename createIndexes() entries to match the lowercase migration names (or drop dupes).
16+
{ key: 'index:idx_nodes_lastHeard', kind: 'onlyInBootstrap',
17+
reason: 'Name-case drift vs idx_nodes_lastheard: createIndexes() uses camelCase; migration 001 creates lowercase idx_nodes_lastheard on nodes(lastHeard).' },
18+
{ key: 'index:idx_nodes_nodeId', kind: 'onlyInBootstrap',
19+
reason: 'Name-case drift vs idx_nodes_nodeid: createIndexes() uses camelCase; migration 001 creates lowercase idx_nodes_nodeid on nodes(nodeId).' },
20+
{ key: 'index:idx_telemetry_nodeId', kind: 'onlyInBootstrap',
21+
reason: 'Name-case drift vs idx_telemetry_nodeid: createIndexes() uses camelCase; migration 036 creates lowercase idx_telemetry_nodeid on telemetry(nodeId).' },
22+
23+
// --- (2) Indexes only createIndexes() creates — no migration creates these ---
24+
// A replay-only fresh install silently loses these indexes (perf regression risk).
25+
// Phase 3.3: add a migration creating each, then delete from this allowlist.
26+
{ key: 'index:idx_messages_createdAt', kind: 'onlyInBootstrap',
27+
reason: 'createIndexes()-only; no migration creates this index on messages(createdAt). Phase 3.3: add as a migration.' },
28+
{ key: 'index:idx_messages_fromNodeId', kind: 'onlyInBootstrap',
29+
reason: 'createIndexes()-only; no migration creates this index on messages(fromNodeId). Phase 3.3: add as a migration.' },
30+
{ key: 'index:idx_messages_toNodeId', kind: 'onlyInBootstrap',
31+
reason: 'createIndexes()-only; no migration creates this index on messages(toNodeId). Phase 3.3: add as a migration.' },
32+
{ key: 'index:idx_nodes_updatedAt', kind: 'onlyInBootstrap',
33+
reason: 'createIndexes()-only; no migration creates this index on nodes(updatedAt). Phase 3.3: add as a migration.' },
34+
{ key: 'index:idx_route_segments_distance', kind: 'onlyInBootstrap',
35+
reason: 'createIndexes()-only; no migration creates this index ON route_segments(distanceKm DESC). Phase 3.3: add as a migration.' },
36+
{ key: 'index:idx_route_segments_recordholder', kind: 'onlyInBootstrap',
37+
reason: 'createIndexes()-only; no migration creates this index ON route_segments(isRecordHolder). Phase 3.3: add as a migration.' },
38+
{ key: 'index:idx_route_segments_timestamp', kind: 'onlyInBootstrap',
39+
reason: 'createIndexes()-only; no migration creates this index ON route_segments(timestamp). Phase 3.3: add as a migration.' },
40+
41+
// --- (1 cont.) Lowercase counterparts only in migration replay ---
42+
// Mirror entries for the name-case pairs above (these exist in B but not A).
43+
{ key: 'index:idx_nodes_lastheard', kind: 'onlyInReplay',
44+
reason: 'Name-case drift vs idx_nodes_lastHeard: migration 001 creates lowercase; createIndexes() creates the camelCase variant as a separate object.' },
45+
{ key: 'index:idx_nodes_nodeid', kind: 'onlyInReplay',
46+
reason: 'Name-case drift vs idx_nodes_nodeId: migration 001 creates lowercase; createIndexes() creates the camelCase variant as a separate object.' },
47+
{ key: 'index:idx_telemetry_nodeid', kind: 'onlyInReplay',
48+
reason: 'Name-case drift vs idx_telemetry_nodeId: migration 036 creates lowercase; createIndexes() creates the camelCase variant as a separate object.' },
49+
50+
// --- (3) Column-order drift: same column set, different physical order ---
51+
// createTables() defines the full DDL in one order; migrations reach the same
52+
// column set chronologically via ALTER TABLE ADD COLUMN (columns appended at end).
53+
// Functionally equivalent but normalized DDL strings differ. Phase 3.3: accept or
54+
// add a table-rebuild migration to align the order.
55+
{ key: 'table:auto_key_repair_log', kind: 'sqlMismatch',
56+
reason: 'Column order differs: bootstrap DDL has sourceid before oldkeyfragment/newkeyfragment; migration replay (ALTER TABLE ADD COLUMN order) places sourceid after oldkeyfragment/newkeyfragment.' },
57+
{ key: 'table:user_map_preferences', kind: 'sqlMismatch',
58+
reason: 'Column order differs: bootstrap DDL places legacy columns (showaccuracyregions, showestimatedpositions, showmeshcorenodes, sortby, sortdirection) in mid-table position; migration replay appends them at the end via ALTER TABLE ADD COLUMN.' },
59+
] as const;

src/db/schemaDrift.test.ts

Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,213 @@
1+
/**
2+
* Schema-drift tripwire (Task 1.6 / Phase 3.3 prerequisite).
3+
*
4+
* Compares the schema produced by two bootstrap paths:
5+
*
6+
* Variant A ("bootstrap path") — the real production fresh-install path:
7+
* DatabaseService constructor → createTables() + createIndexes() + migration loop.
8+
* This is what every real deployment runs today.
9+
*
10+
* Variant B ("replay path") — migration chain only:
11+
* createTestDb() replays migration 001 baseline → 112 against :memory:.
12+
* This is Phase 3.3's target (delete createTables/createIndexes).
13+
*
14+
* Phase 3.3 deletes createTables()/createIndexes() and makes the replay path the only
15+
* path. Until then, this test proves A ≡ B modulo the documented allowlist and
16+
* enforces allowlist burn-down: new entries are rejected; stale entries are rejected.
17+
*
18+
* Raw sqlite_master reads live in a *.test.ts file and are therefore exempt from the
19+
* project's no-restricted-syntax (raw SQL) ESLint ban.
20+
*/
21+
22+
import { afterAll, describe, it } from 'vitest';
23+
import Database from 'better-sqlite3';
24+
import * as fs from 'fs';
25+
import * as os from 'os';
26+
import * as path from 'path';
27+
28+
// ─── Variant A: must set DATABASE_PATH BEFORE importing the singleton ──────────
29+
//
30+
// vitest.config.ts sets env.DATABASE_PATH = ':memory:' which would be picked up
31+
// by the singleton, but we need a temp file so we can open a second read-only
32+
// connection to read sqlite_master after construction (a :memory: DB can't be
33+
// re-opened by a second connection). Override here at module top, before any
34+
// dynamic import of the singleton.
35+
const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'meshmonitor-schema-drift-'));
36+
const tempDbFile = path.join(tempDir, 'bootstrap.db');
37+
process.env.DATABASE_PATH = tempDbFile;
38+
39+
// ─── Variant B ────────────────────────────────────────────────────────────────
40+
import { createTestDb, type TestDb } from '../server/test-helpers/testDb.js';
41+
import { SCHEMA_DRIFT_ALLOWLIST, type DriftKind } from './schemaDrift.allowlist.js';
42+
43+
// ─── Types ────────────────────────────────────────────────────────────────────
44+
45+
interface SchemaRow {
46+
type: string;
47+
name: string;
48+
sql: string | null;
49+
}
50+
51+
interface OnlyInOne { key: string; sql: string; }
52+
interface Mismatch { key: string; sqlA: string; sqlB: string; }
53+
54+
// ─── Normalization ────────────────────────────────────────────────────────────
55+
56+
/**
57+
* Normalize a sqlite_master DDL string for comparison.
58+
*
59+
* Rules (per spec):
60+
* 1. Strip `IF NOT EXISTS` (case-insensitive).
61+
* 2. Collapse all whitespace runs to a single space.
62+
* 3. Remove spaces around `,`, `(`, `)`.
63+
* 4. Strip `"` and `` ` `` (SQLite quotes identifiers inconsistently).
64+
* 5. Lowercase.
65+
* 6. Trim.
66+
*
67+
* Column order is intentionally preserved — it is significant for catching
68+
* category-(3) drift (createTables column order vs ALTER TABLE ADD COLUMN order).
69+
*/
70+
function normalizeDdl(sql: string | null): string {
71+
if (!sql) return '';
72+
return sql
73+
.replace(/IF NOT EXISTS/gi, '')
74+
.replace(/\s+/g, ' ')
75+
.replace(/\s*,\s*/g, ',')
76+
.replace(/\s*\(\s*/g, '(')
77+
.replace(/\s*\)\s*/g, ')')
78+
.replace(/["`]/g, '')
79+
.toLowerCase()
80+
.trim();
81+
}
82+
83+
/** Collect all non-sqlite_% schema objects from a connection, keyed by `type:name`. */
84+
function collectSchema(conn: Database.Database): Map<string, string> {
85+
const rows = conn.prepare(
86+
`SELECT type, name, sql FROM sqlite_master
87+
WHERE name NOT LIKE 'sqlite_%'
88+
ORDER BY type, name`
89+
).all() as SchemaRow[];
90+
return new Map(rows.map(r => [`${r.type}:${r.name}`, normalizeDdl(r.sql)]));
91+
}
92+
93+
// ─── Test ─────────────────────────────────────────────────────────────────────
94+
95+
describe('schema bootstrap drift', () => {
96+
let testDbB: TestDb;
97+
let connA: Database.Database;
98+
99+
afterAll(() => {
100+
try { connA?.close(); } catch { /* ignore */ }
101+
try { testDbB?.close(); } catch { /* ignore */ }
102+
try { fs.rmSync(tempDir, { recursive: true }); } catch { /* ignore */ }
103+
// Restore env to avoid polluting subsequent test files in the same fork
104+
process.env.DATABASE_PATH = ':memory:';
105+
});
106+
107+
it('createTables()/createIndexes() and migration replay agree modulo the allowlist', { timeout: 30000 }, async () => {
108+
// ── Variant A: import the DatabaseService singleton ────────────────────
109+
// The dynamic import must happen INSIDE the test (not at module top) so that
110+
// process.env.DATABASE_PATH is already overridden when the module is first
111+
// evaluated. Vitest per-file module isolation makes this reliable.
112+
const { default: _dbService } = await import('../services/database.js');
113+
114+
// Open a second read-only connection to the same file to read sqlite_master.
115+
// (We can't reuse the singleton's internal connection because it is private.)
116+
connA = new Database(tempDbFile, { readonly: true });
117+
const mapA = collectSchema(connA);
118+
119+
// ── Variant B: migration-replay-only path ──────────────────────────────
120+
testDbB = createTestDb();
121+
const mapB = collectSchema(testDbB.sqlite);
122+
123+
// ── Diff ───────────────────────────────────────────────────────────────
124+
const onlyInBootstrap: OnlyInOne[] = [];
125+
const onlyInReplay: OnlyInOne[] = [];
126+
const sqlMismatch: Mismatch[] = [];
127+
128+
for (const [key, sqlA] of mapA) {
129+
if (!mapB.has(key)) {
130+
onlyInBootstrap.push({ key, sql: sqlA });
131+
} else if (mapB.get(key) !== sqlA) {
132+
sqlMismatch.push({ key, sqlA, sqlB: mapB.get(key)! });
133+
}
134+
}
135+
for (const [key, sqlB] of mapB) {
136+
if (!mapA.has(key)) {
137+
onlyInReplay.push({ key, sql: sqlB });
138+
}
139+
}
140+
141+
// ── Allowlist check ────────────────────────────────────────────────────
142+
// Build a set of allowlisted (key, kind) pairs for fast lookup.
143+
type AllowKey = `${string}::${DriftKind}`;
144+
const allowSet = new Set<AllowKey>(
145+
SCHEMA_DRIFT_ALLOWLIST.map(e => `${e.key}::${e.kind}` as AllowKey)
146+
);
147+
148+
// Track which allowlist entries were "consumed" (i.e. actually found in the diff).
149+
const consumed = new Set<AllowKey>();
150+
151+
// Unexpected divergences (not in allowlist) → failure.
152+
const unexpected: string[] = [];
153+
154+
// Helper to check one divergence against the allowlist.
155+
function check(key: string, kind: DriftKind, label: string): void {
156+
const ak: AllowKey = `${key}::${kind}`;
157+
if (allowSet.has(ak)) {
158+
consumed.add(ak);
159+
} else {
160+
unexpected.push(label);
161+
}
162+
}
163+
164+
for (const { key, sql } of onlyInBootstrap) {
165+
check(key, 'onlyInBootstrap',
166+
` + ${key} (in fresh-install/createTables path, MISSING from migration replay)\n` +
167+
` DDL: ${sql}\n` +
168+
` hint: add a migration creating this, or add to allowlist if intentional pre-Phase-3.3`
169+
);
170+
}
171+
for (const { key, sql } of onlyInReplay) {
172+
check(key, 'onlyInReplay',
173+
` - ${key} (in migration replay, MISSING from createTables path)\n` +
174+
` DDL: ${sql}`
175+
);
176+
}
177+
for (const { key, sqlA, sqlB } of sqlMismatch) {
178+
check(key, 'sqlMismatch',
179+
` ~ ${key} (DDL differs)\n` +
180+
` bootstrap: ${sqlA}\n` +
181+
` replay: ${sqlB}`
182+
);
183+
}
184+
185+
// Stale allowlist entries (allowlisted but no longer divergent) → failure.
186+
const stale: string[] = [];
187+
for (const e of SCHEMA_DRIFT_ALLOWLIST) {
188+
const ak: AllowKey = `${e.key}::${e.kind}`;
189+
if (!consumed.has(ak)) {
190+
stale.push(
191+
` ! ${e.key} was allowlisted (kind: ${e.kind}) but is no longer divergent` +
192+
` — remove it from schemaDrift.allowlist.ts`
193+
);
194+
}
195+
}
196+
197+
if (unexpected.length > 0 || stale.length > 0) {
198+
const lines: string[] = [
199+
'Schema bootstrap drift changed. createTables()/createIndexes() (src/services/database.ts)' +
200+
' and the migration chain (src/server/migrations/) disagree.' +
201+
' Reconcile, or update schemaDrift.allowlist.ts. See Task 1.6 / Phase 3.3.',
202+
'',
203+
];
204+
if (unexpected.length > 0) {
205+
lines.push(`UNEXPECTED DIVERGENCES (${unexpected.length}):`, ...unexpected, '');
206+
}
207+
if (stale.length > 0) {
208+
lines.push(`STALE ALLOWLIST ENTRIES (${stale.length}):`, ...stale, '');
209+
}
210+
throw new Error(lines.join('\n'));
211+
}
212+
});
213+
});

0 commit comments

Comments
 (0)