From f7ff41c2e0a5ee13c98e895b4375e24a177f5a20 Mon Sep 17 00:00:00 2001 From: Jberma23 Date: Sun, 16 Aug 2026 19:44:28 +0000 Subject: [PATCH 1/3] Handle a journal no key can open, and close it when locking Two gaps between what 0015 decided and what the code does. The first is the case 0015 designs for. It keeps the key WHEN_UNLOCKED_THIS_DEVICE_ONLY precisely so it does not travel in an iCloud backup - but journal.db lives in Documents and does. Restoring a backup onto a new phone, which is what people do when they replace a handset, brings the file back without the key. Nothing handled that. SecureStore returns null both for "nothing stored yet" and for "the entry was invalidated", key.ts read null as a first run and minted a fresh key, and database.ts asked for the key before opening the file so it could not know better. The result was a generic "Could not open the journal database" on that launch and every launch after, with no explanation and nowhere to go. getOrCreateDatabaseKey now reports whether it minted the key, and database.ts uses that: a key we just created cannot fail to read a file we just created, so if the decrypt probe fails the file was already there and belonged to a key that is gone. That is now UnrecoverableJournalError rather than a generic failure, and the useless key is deleted so the next launch reaches the same branch instead of degrading to something undiagnosable. The journal really is unrecoverable - 0007 commits to saying so during onboarding - but "made on a different phone" is a true thing to say and leaves the user somewhere to go. The UI for starting over is not built; this is the mechanism it needs. The second gap is that the unlock gate locked the view and nothing else. database.ts held a decrypted handle and the key stayed in memory for the life of the process, which made WHEN_UNLOCKED_THIS_DEVICE_ONLY a property of the first launch and nothing after it, because the keychain was never asked again. The gate now closes the database on the same background event that re-locks. Also corrects two comments in key.ts that were wrong rather than merely thin. keychainAccessible is iOS-only, so THIS_DEVICE_ONLY keeps the key out of iCloud but does nothing about Google backup - that comes from the expo-secure-store config plugin's backup rules, which makes the bare plugin entry in app.json load-bearing. And WHEN_PASSCODE_SET_THIS_DEVICE_ONLY is recorded as considered and rejected: it looks like a free upgrade, but it cannot store a key at all on the devices 0018 is about, and it turns removing a passcode into a data-loss event. Co-Authored-By: Claude Opus 5 (1M context) --- mobile/src/components/unlock-gate.test.tsx | 26 +++++++++ mobile/src/components/unlock-gate.tsx | 13 +++++ mobile/src/lib/db/database.test.ts | 60 +++++++++++++++++++- mobile/src/lib/db/database.ts | 66 ++++++++++++++++++++-- mobile/src/lib/db/key.test.ts | 15 ++++- mobile/src/lib/db/key.ts | 46 +++++++++++++-- 6 files changed, 212 insertions(+), 14 deletions(-) diff --git a/mobile/src/components/unlock-gate.test.tsx b/mobile/src/components/unlock-gate.test.tsx index 62b5bb2..c536d57 100644 --- a/mobile/src/components/unlock-gate.test.tsx +++ b/mobile/src/components/unlock-gate.test.tsx @@ -3,14 +3,23 @@ import { AppState, Platform, Text as RNText } from 'react-native'; import { UnlockGate } from './unlock-gate'; import { checkUnlockAvailability, requestUnlock } from '@/lib/auth/unlock'; +import { closeJournalDatabase } from '@/lib/db/database'; jest.mock('@/lib/auth/unlock', () => ({ checkUnlockAvailability: jest.fn(), requestUnlock: jest.fn(), })); +// The gate closes the database when it re-locks. Mocked rather than exercised: +// the real one needs expo-sqlite and a device, and what matters here is only +// that the gate asks. +jest.mock('@/lib/db/database', () => ({ + closeJournalDatabase: jest.fn(async () => undefined), +})); + const availability = checkUnlockAvailability as jest.Mock; const unlock = requestUnlock as jest.Mock; +const closeDatabase = closeJournalDatabase as jest.Mock; const originalOS = Platform.OS; function setPlatform(os: typeof Platform.OS) { @@ -148,9 +157,26 @@ describe('UnlockGate', () => { await waitFor(() => expect(screen.queryByText('Journal contents')).toBeNull()); expect(screen.getByText('Your journal is locked')).toBeVisible(); + // Locking the view is only half of it. While the handle stays open the key + // is still in memory and any caller can still read, which makes 0015's + // WHEN_UNLOCKED_THIS_DEVICE_ONLY a first-launch property and nothing more. + expect(closeDatabase).toHaveBeenCalled(); + jest.restoreAllMocks(); }); + it('does not close a database the web build never opened', async () => { + setPlatform('web'); + + await render( + + + + ); + + expect(closeDatabase).not.toHaveBeenCalled(); + }); + it('does not gate the web build, which holds no journal data', async () => { setPlatform('web'); diff --git a/mobile/src/components/unlock-gate.tsx b/mobile/src/components/unlock-gate.tsx index ba53a1f..7ed26ae 100644 --- a/mobile/src/components/unlock-gate.tsx +++ b/mobile/src/components/unlock-gate.tsx @@ -16,6 +16,7 @@ import { ActivityIndicator, AppState, Platform, View } from 'react-native'; import { Button } from '@/components/ui/button'; import { Text } from '@/components/ui/text'; import { checkUnlockAvailability, requestUnlock } from '@/lib/auth/unlock'; +import { closeJournalDatabase } from '@/lib/db/database'; type GateState = | { status: 'checking' } @@ -105,6 +106,18 @@ export function UnlockGate({ children, skip = false }: UnlockGateProps) { const subscription = AppState.addEventListener('change', (next) => { if (next === 'background') { setState({ status: 'locked', message: null }); + + // Close the database too, not just the view. The gate is the only thing + // that knows the app is meant to be locked, and locking only the UI + // leaves db/database.ts holding a decrypted handle and the key in memory + // for the life of the process - at which point 0015's + // WHEN_UNLOCKED_THIS_DEVICE_ONLY is a property of the first launch and + // nothing after it, because the keychain is never asked again. Dropping + // the handle here is what makes that option mean something. + // + // Failures are swallowed on purpose: this runs on the way out of the + // foreground, there is nobody to tell, and the next unlock re-opens. + void closeJournalDatabase().catch(() => undefined); } }); diff --git a/mobile/src/lib/db/database.test.ts b/mobile/src/lib/db/database.test.ts index 9807630..5f24c26 100644 --- a/mobile/src/lib/db/database.test.ts +++ b/mobile/src/lib/db/database.test.ts @@ -6,6 +6,7 @@ import { DatabaseUnavailableError, destroyJournalDatabase, getJournalDatabase, + UnrecoverableJournalError, } from './database'; import { deleteDatabaseKey, getOrCreateDatabaseKey } from './key'; import { migrate } from './migrations'; @@ -57,9 +58,13 @@ beforeEach(async () => { setPlatform('ios'); db = fakeDatabase(); openDatabaseAsync.mockResolvedValue(db); - getKey.mockResolvedValue(KEY); + getKey.mockResolvedValue({ key: KEY, created: false }); migrateMock.mockResolvedValue(0); deleteDatabaseAsync.mockResolvedValue(undefined); + // Has to resolve, not return undefined: the code under test chains .catch() + // onto it, and a bare jest.fn() would throw a TypeError that swallows the + // error the caller was actually meant to see. + deleteKey.mockResolvedValue(undefined); }); afterEach(async () => { @@ -124,6 +129,59 @@ describe('getJournalDatabase', () => { }); }); +describe('a journal that no key can open', () => { + /** A key we just minted cannot fail against a file we just created. */ + function restoredFromAnotherPhone() { + getKey.mockResolvedValue({ key: KEY, created: true }); + db.getFirstAsync.mockRejectedValue(new Error('file is not a database')); + } + + it('is reported as its own error rather than a generic open failure', async () => { + // Restoring a backup onto a new phone brings journal.db back but not the + // key, which 0015 keeps THIS_DEVICE_ONLY on purpose. Saying "could not open + // the database" there is true and useless; this is the case that has to be + // nameable so something can eventually offer to start over. + restoredFromAnotherPhone(); + + await expect(getJournalDatabase()).rejects.toThrow(UnrecoverableJournalError); + }); + + it('takes the useless key back out, so the next launch reaches the same branch', async () => { + // Leaving it stored would make created=false next time, and the diagnosis + // would degrade to a generic failure that explains nothing. + restoredFromAnotherPhone(); + + await expect(getJournalDatabase()).rejects.toThrow(UnrecoverableJournalError); + expect(deleteKey).toHaveBeenCalled(); + }); + + it('closes the handle it could not read', async () => { + restoredFromAnotherPhone(); + + await expect(getJournalDatabase()).rejects.toThrow(UnrecoverableJournalError); + expect(db.closeAsync).toHaveBeenCalled(); + }); + + it('does not blame a stored key for a decrypt failure, or delete it', async () => { + // Same symptom, different cause: the key was already there, so this is + // corruption or something else - not a journal from another phone. Deleting + // the key here would destroy a database that might still be readable. + getKey.mockResolvedValue({ key: KEY, created: false }); + db.getFirstAsync.mockRejectedValue(new Error('file is not a database')); + + await expect(getJournalDatabase()).rejects.toThrow(DatabaseUnavailableError); + expect(deleteKey).not.toHaveBeenCalled(); + }); + + it('leaves a genuine first run alone', async () => { + // Fresh install: key minted, empty file, decrypt fine. Nothing to report. + getKey.mockResolvedValue({ key: KEY, created: true }); + + await expect(getJournalDatabase()).resolves.toBeDefined(); + expect(deleteKey).not.toHaveBeenCalled(); + }); +}); + describe('closeJournalDatabase', () => { it('closes an open handle', async () => { await getJournalDatabase(); diff --git a/mobile/src/lib/db/database.ts b/mobile/src/lib/db/database.ts index 402a9a5..f246310 100644 --- a/mobile/src/lib/db/database.ts +++ b/mobile/src/lib/db/database.ts @@ -25,6 +25,32 @@ export class DatabaseUnavailableError extends Error { } } +/** + * There is a journal on this device and no key that opens it. The data is gone. + * + * The path here is not exotic - it is the ordinary one. 0015 stores the key + * WHEN_UNLOCKED_THIS_DEVICE_ONLY precisely so it does *not* travel in an iCloud + * backup, but `journal.db` lives in Documents and does. So restoring a backup + * onto a new phone - which is what people do when they replace a handset - + * brings back the file without the key. + * + * Treating that as a first run and minting a fresh key is the worst available + * response: every read then fails, and the app is bricked on every launch after + * with a message that explains nothing. So it is called out as its own error. + * The journal genuinely cannot be recovered - 0007 commits to telling people + * that during onboarding - but "your journal was made on a different phone and + * cannot be opened here" is a true thing to say, and it leaves the user + * somewhere to go. `destroyJournalDatabase` is the way to start over. + * + * The UI for that is not built; this is the mechanism it needs. + */ +export class UnrecoverableJournalError extends Error { + constructor(message: string, options?: { cause?: unknown }) { + super(message, options); + this.name = 'UnrecoverableJournalError'; + } +} + /** * Cached as the in-flight promise, not the resolved handle, so that two screens * mounting at once share one open rather than racing to create two. Cleared on @@ -45,17 +71,14 @@ async function open(): Promise { ); } - const key = await getOrCreateDatabaseKey(); + const { key, created } = await getOrCreateDatabaseKey(); const db = await SQLite.openDatabaseAsync(DATABASE_NAME); try { // Has to be the first statement executed against the connection. await db.execAsync(rawKeyPragma(key)); - // Touch the schema to force SQLCipher to actually decrypt a page. Without - // this, a wrong key is not discovered until the first real query, which - // could be several screens away from the thing that caused it. - await db.getFirstAsync('SELECT count(*) FROM sqlite_master'); + await assertReadable(db, created); await db.execAsync('PRAGMA journal_mode = WAL'); await db.execAsync('PRAGMA foreign_keys = ON'); @@ -64,12 +87,45 @@ async function open(): Promise { } catch (cause) { // Do not leave a half-configured handle behind for the next caller. await db.closeAsync().catch(() => undefined); + if (cause instanceof UnrecoverableJournalError) throw cause; throw new DatabaseUnavailableError('Could not open the journal database.', { cause }); } return db; } +/** + * Reads a page, so a key that does not fit this file is discovered here rather + * than several screens away at the first real query. + * + * What this does not catch: on a build without SQLCipher, `PRAGMA key` is + * silently ignored - SQLite ignores unknown pragmas - and this read succeeds + * against a plaintext file. Detecting that needs `PRAGMA cipher_version`; see + * issue #130. + * + * What it does catch, given `keyWasCreated`, is the restored-backup case. A key + * we just minted cannot fail to read a database we just created, so if it fails + * the file was already there and belonged to a key that is gone. + */ +async function assertReadable(db: SQLiteDatabase, keyWasCreated: boolean): Promise { + try { + await db.getFirstAsync('SELECT count(*) FROM sqlite_master'); + } catch (cause) { + if (!keyWasCreated) throw cause; + + // Take the useless key back out. Leaving it would turn a diagnosable state + // into an undiagnosable one: the next launch would read a stored key, and + // this branch - the only thing that knows what actually happened - would + // never run again. + await deleteDatabaseKey().catch(() => undefined); + + throw new UnrecoverableJournalError( + 'There is a journal on this device that no key can open. It was almost certainly restored from a backup of another phone, which does not carry the key. The journal cannot be recovered; starting a new one means erasing it.', + { cause } + ); + } +} + /** The shared database handle, opening and migrating it on first call. */ export function getJournalDatabase(): Promise { openPromise ??= open().catch((error: unknown) => { diff --git a/mobile/src/lib/db/key.test.ts b/mobile/src/lib/db/key.test.ts index 21351a6..476e5d7 100644 --- a/mobile/src/lib/db/key.test.ts +++ b/mobile/src/lib/db/key.test.ts @@ -37,7 +37,7 @@ describe('getOrCreateDatabaseKey', () => { it('generates, stores and returns a key the first time', async () => { getItemAsync.mockResolvedValue(null); - await expect(getOrCreateDatabaseKey()).resolves.toBe(HEX); + await expect(getOrCreateDatabaseKey()).resolves.toEqual({ key: HEX, created: true }); expect(getRandomBytesAsync).toHaveBeenCalledWith(32); expect(setItemAsync).toHaveBeenCalledWith('journal.database.key', HEX, expect.any(Object)); @@ -46,12 +46,23 @@ describe('getOrCreateDatabaseKey', () => { it('returns the stored key without generating a new one', async () => { getItemAsync.mockResolvedValue(HEX); - await expect(getOrCreateDatabaseKey()).resolves.toBe(HEX); + await expect(getOrCreateDatabaseKey()).resolves.toEqual({ key: HEX, created: false }); expect(getRandomBytesAsync).not.toHaveBeenCalled(); expect(setItemAsync).not.toHaveBeenCalled(); }); + it('reports whether it minted the key, because null is ambiguous', async () => { + // SecureStore returns null both for "nothing stored yet" and for "the entry + // was invalidated". Only database.ts can tell those apart - by finding out + // whether the key opens the file - so this flag has to reach it. + getItemAsync.mockResolvedValue(null); + await expect(getOrCreateDatabaseKey()).resolves.toMatchObject({ created: true }); + + getItemAsync.mockResolvedValue(HEX); + await expect(getOrCreateDatabaseKey()).resolves.toMatchObject({ created: false }); + }); + it('stores the key without requireAuthentication, and device-only', async () => { // This is the decision documented at length in key.ts. If someone turns // requireAuthentication on, adding a fingerprint silently destroys every diff --git a/mobile/src/lib/db/key.ts b/mobile/src/lib/db/key.ts index 7d01ca9..8ea9b5f 100644 --- a/mobile/src/lib/db/key.ts +++ b/mobile/src/lib/db/key.ts @@ -47,9 +47,26 @@ const KEYCHAIN_ENTRY = 'journal.database.key'; * loss, this is the better side of the trade - but it is a product decision as * much as a technical one and it deserves a signed-off ADR of its own. * - * THIS_DEVICE_ONLY additionally keeps the key out of iCloud/Google backups, so - * a restored backup on a new phone cannot decrypt a copied database file. That - * is the same boundary 0003 draws and what issue #115 asks for. + * THIS_DEVICE_ONLY keeps the key out of iCloud backups, so a restored backup on + * a new phone cannot decrypt a copied database file. That is the same boundary + * 0003 draws and what issue #115 asks for. Note that database.ts has to handle + * the other side of that: the file does come back from a backup, so a restored + * phone finds a journal it cannot read and has to say so rather than mint a new + * key over it. + * + * This option is iOS-only - `keychainAccessible` is @platform ios in + * expo-secure-store, so it does nothing on Android. The Android equivalent + * comes from the expo-secure-store config plugin, whose backup rules exclude + * the SecureStore shared preferences from Auto Backup. That makes the bare + * "expo-secure-store" entry in app.json load-bearing rather than registration. + * + * WHEN_PASSCODE_SET_THIS_DEVICE_ONLY is the upgrade that looks free and is not. + * It is stronger, and unlike requireAuthentication it does not bind to + * biometric enrollment - but expo documents it as "the user must have set a + * passcode in order to store an entry. If the user removes their passcode, the + * entry will be deleted." It therefore cannot store a key at all on the devices + * 0018 is about, and it turns removing a passcode into a data-loss event: the + * same catastrophe as the fingerprint case, with a rarer trigger. */ const KEYCHAIN_OPTIONS: SecureStore.SecureStoreOptions = { keychainAccessible: SecureStore.WHEN_UNLOCKED_THIS_DEVICE_ONLY, @@ -67,13 +84,30 @@ function toHex(bytes: Uint8Array): string { return Array.from(bytes, (b) => b.toString(16).padStart(2, '0')).join(''); } +export type DatabaseKey = { + /** Raw key as hex, not a passphrase - see `rawKeyPragma`. */ + readonly key: string; + /** + * True when this call minted the key rather than reading a stored one. + * + * The caller needs this, and the reason is worth stating. `getItemAsync` + * resolves to null "if there is no entry for the given key **or if the key + * has been invalidated**" - expo's own wording. One value, two very different + * situations, and only one of them is a first run. Nothing at this level can + * tell them apart, because that takes knowing whether a database file already + * exists; database.ts can, by trying to decrypt it. So this flag hands the + * question up rather than guessing here. + */ + readonly created: boolean; +}; + /** * Returns the database key, generating and storing one the first time. * * The hex string this returns is a raw key, not a passphrase - see * `rawKeyPragma` for why that distinction matters at the PRAGMA. */ -export async function getOrCreateDatabaseKey(): Promise { +export async function getOrCreateDatabaseKey(): Promise { let existing: string | null; try { existing = await SecureStore.getItemAsync(KEYCHAIN_ENTRY, KEYCHAIN_OPTIONS); @@ -88,7 +122,7 @@ export async function getOrCreateDatabaseKey(): Promise { }); } - if (existing) return existing; + if (existing) return { key: existing, created: false }; const key = toHex(await Crypto.getRandomBytesAsync(KEY_BYTES)); @@ -100,7 +134,7 @@ export async function getOrCreateDatabaseKey(): Promise { }); } - return key; + return { key, created: true }; } /** From f04459ec163d7375c3e9927872270d2dfda51bcd Mon Sep 17 00:00:00 2001 From: Jberma23 Date: Sun, 16 Aug 2026 19:44:37 +0000 Subject: [PATCH 2/3] Fix repository update() binding and create() return value Two defects in the repeatable-entry layer that #118 and #49 are both built on. Neither was caught by the tests, and neither is caught by TypeScript. update() reached its sibling method through `this`, which is only bound when it is called as repo.update(...). `const { update } = repo` and `onPress={repo.update}` are both ordinary React and both threw "TypeError: find is not a function" at runtime. find is now a plain function above the returned object, so nothing in the repository depends on how it was called. create() built its return value from the caller's input rather than from what it wrote. A field the caller omitted was stored as NULL and returned as absent, so create() and find() disagreed about the same row; and keys that were never columns were dropped on insert but echoed back, making them look saved. It now returns the declared fields and only those, with omissions turned into the NULL that actually goes to the database. Nothing consumes createRepository yet, so this is not a regression - but the cost of fixing it rises with every screen written against the old behaviour, and #118 is next. Fixes #134 Co-Authored-By: Claude Opus 5 (1M context) --- mobile/src/lib/db/repository.test.ts | 44 +++++++++++++++++++++++++ mobile/src/lib/db/repository.ts | 48 ++++++++++++++++++++-------- 2 files changed, 79 insertions(+), 13 deletions(-) diff --git a/mobile/src/lib/db/repository.test.ts b/mobile/src/lib/db/repository.test.ts index 08ff407..b9deec8 100644 --- a/mobile/src/lib/db/repository.test.ts +++ b/mobile/src/lib/db/repository.test.ts @@ -101,6 +101,31 @@ describeSql('createRepository', () => { expect(stored?.phone).toBeNull(); }); + it('returns exactly what find() reads back', async () => { + // The two used to disagree: an omitted field was stored as NULL and + // returned as absent, so a screen rendering `created` saw something the + // database did not contain. + const created = await contacts.create({ + name: 'Sam Okafor', + relationship: 'Neighbour', + } as Contact); + + await expect(contacts.find(created.id)).resolves.toEqual(created); + }); + + it('does not hand back keys that were never columns', async () => { + // Undeclared keys are dropped on insert, so echoing them makes the return + // value look like a saved record when it is not. + const created = await contacts.create({ + name: 'Sam', + relationship: 'Friend', + phone: null, + nickname: 'Sammy', + } as never); + + expect(created).not.toHaveProperty('nickname'); + }); + it('treats SQL in a value as text, not as SQL', async () => { const created = await contacts.create({ name: "Robert'); DROP TABLE contacts;--", @@ -189,6 +214,25 @@ describeSql('createRepository', () => { it('throws for an id that is not there', async () => { await expect(contacts.update('nope', { name: 'X' })).rejects.toThrow(RepositoryError); }); + + it('still works when pulled off the repository', async () => { + // `const { update } = repo` and `onPress={repo.update}` are both ordinary + // React. While update reached its sibling through `this`, both threw + // TypeError at runtime, and TypeScript had nothing to say about it. + const created = await contacts.create({ name: 'Alex', relationship: 'Sister', phone: null }); + const { update } = contacts; + + await expect(update(created.id, { name: 'Alexis' })).resolves.toMatchObject({ + name: 'Alexis', + }); + }); + + it('still works detached on the no-op path, which reads a row back too', async () => { + const created = await contacts.create({ name: 'Alex', relationship: 'Sister', phone: null }); + const { update } = contacts; + + await expect(update(created.id, {})).resolves.toMatchObject({ name: 'Alex' }); + }); }); describe('remove', () => { diff --git a/mobile/src/lib/db/repository.ts b/mobile/src/lib/db/repository.ts index abd1d4e..c0b5328 100644 --- a/mobile/src/lib/db/repository.ts +++ b/mobile/src/lib/db/repository.ts @@ -104,36 +104,58 @@ export function createRepository( // millisecond keep a stable order between renders instead of swapping around. const ordering = 'ORDER BY created_at ASC, id ASC'; + // A plain function rather than a method, deliberately. `update` needs to read + // a row back, and reaching a sibling through `this` breaks the moment anyone + // writes `const { update } = repo` or passes `repo.update` as a callback - + // both ordinary in React, neither caught by TypeScript, and the failure is a + // TypeError at runtime. + async function find(id: string): Promise | null> { + const db = await getDatabase(); + const row = await db.getFirstAsync>( + `SELECT ${selection} FROM ${table} WHERE id = ?`, + id + ); + return row ?? null; + } + + /** + * The declared fields, and only those, with anything the caller left out + * turned into the NULL that will actually be stored. + * + * This is what `create` returns rather than the caller's own object, so that + * the entry it hands back matches the row `find` reads. Spreading the input + * instead lets an omitted field come back as `undefined` while the database + * holds `null`, and lets keys that were never columns travel onwards as + * though they had been saved. + */ + function normalise(values: Partial): TFields { + return Object.fromEntries(fields.map((field) => [field, values[field] ?? null])) as TFields; + } + return { async list() { const db = await getDatabase(); return db.getAllAsync>(`SELECT ${selection} FROM ${table} ${ordering}`); }, - async find(id) { - const db = await getDatabase(); - const row = await db.getFirstAsync>( - `SELECT ${selection} FROM ${table} WHERE id = ?`, - id - ); - return row ?? null; - }, + find, async create(values) { const db = await getDatabase(); const id = Crypto.randomUUID(); const now = new Date().toISOString(); + const stored = normalise(values); const columns = ['id', 'created_at', 'updated_at', ...fields]; const placeholders = columns.map(() => '?').join(', '); - const bound = [id, now, now, ...fields.map((field) => values[field] ?? null)]; + const bound = [id, now, now, ...fields.map((field) => stored[field])]; await db.runAsync( `INSERT INTO ${table} (${columns.join(', ')}) VALUES (${placeholders})`, - bound + bound as FieldValue[] ); - return { ...values, id, createdAt: now, updatedAt: now }; + return { ...stored, id, createdAt: now, updatedAt: now }; }, async update(id, changes) { @@ -143,7 +165,7 @@ export function createRepository( if (changed.length === 0) { // Nothing to write. Returning the row as-is beats touching updatedAt // for an edit that changed nothing. - const current = await this.find(id); + const current = await find(id); if (!current) throw new RepositoryError(`No ${table} entry with id ${id}.`); return current; } @@ -161,7 +183,7 @@ export function createRepository( throw new RepositoryError(`No ${table} entry with id ${id}.`); } - const updated = await this.find(id); + const updated = await find(id); if (!updated) throw new RepositoryError(`No ${table} entry with id ${id}.`); return updated; }, From d515cf6df8795f5820be69c38cf9742c3dc4ffab Mon Sep 17 00:00:00 2001 From: Jberma23 Date: Sun, 16 Aug 2026 19:44:46 +0000 Subject: [PATCH 3/3] Make the node:sqlite guard actually run HAS_NODE_SQLITE was meant to let the SQLite-backed suites skip on a Node without node:sqlite. It could never do that: the static import is resolved before any module code runs, so on Node below 22.5 the module threw at import time and the try/catch around the guard was never reached. The two suites errored out instead of skipping, on a version engines still permits (>=20.19.4). CI runs 22.x, which is exactly why it could sit unnoticed. The require is now lazy, so the guard evaluates, and createInMemoryDatabase fails with a message naming the Node version it needs rather than a resolution error from inside node_modules. Adds a test for the helper itself. Making the require throw is the only way to exercise the old-Node path from a new Node, and without it nothing in CI would notice this regressing back - which is how it got here. Fixes #133 Co-Authored-By: Claude Opus 5 (1M context) --- mobile/jest/in-memory-sqlite.test.ts | 57 ++++++++++++++++++++++++++++ mobile/jest/in-memory-sqlite.ts | 29 +++++++++++--- 2 files changed, 80 insertions(+), 6 deletions(-) create mode 100644 mobile/jest/in-memory-sqlite.test.ts diff --git a/mobile/jest/in-memory-sqlite.test.ts b/mobile/jest/in-memory-sqlite.test.ts new file mode 100644 index 0000000..3c6f149 --- /dev/null +++ b/mobile/jest/in-memory-sqlite.test.ts @@ -0,0 +1,57 @@ +// The guard in in-memory-sqlite.ts only earns its place if it actually runs on +// a Node without node:sqlite. It did not: a static `import ... from +// 'node:sqlite'` is resolved before any module code, so the two SQLite-backed +// suites errored out on Node below 22.5 instead of skipping - issue #133, on a +// version `engines` still permits. +// +// CI runs 22.x, so nothing here would notice a regression. Making the require +// throw is the only way to test the old-Node path from a new Node. + +// doMock registrations live for the whole file, so each test states which world +// it wants rather than inheriting the previous one's. +beforeEach(() => { + jest.resetModules(); + jest.dontMock('node:sqlite'); +}); + +function withoutNodeSqlite() { + jest.doMock('node:sqlite', () => { + throw new Error('No such built-in module: node:sqlite'); + }); +} + +describe('on a Node without node:sqlite', () => { + it('reports the feature as missing rather than throwing at import', () => { + withoutNodeSqlite(); + + // The import itself is the assertion: before the fix it threw here. + const { HAS_NODE_SQLITE } = require('./in-memory-sqlite'); + + expect(HAS_NODE_SQLITE).toBe(false); + }); + + it('refuses to build a database, and says which Node it needs', () => { + withoutNodeSqlite(); + + const { createInMemoryDatabase } = require('./in-memory-sqlite'); + + expect(() => createInMemoryDatabase()).toThrow(/Node 22.5 or newer/); + }); +}); + +describe('on a Node that has it', () => { + it('reports the feature as present and builds a working database', async () => { + const { HAS_NODE_SQLITE, createInMemoryDatabase } = require('./in-memory-sqlite'); + + expect(HAS_NODE_SQLITE).toBe(true); + + const db = createInMemoryDatabase(); + try { + await db.execAsync('CREATE TABLE t (id TEXT)'); + await db.runAsync('INSERT INTO t (id) VALUES (?)', 'a'); + await expect(db.getFirstAsync('SELECT id FROM t')).resolves.toEqual({ id: 'a' }); + } finally { + await db.closeAsync(); + } + }); +}); diff --git a/mobile/jest/in-memory-sqlite.ts b/mobile/jest/in-memory-sqlite.ts index 8498874..aa976e3 100644 --- a/mobile/jest/in-memory-sqlite.ts +++ b/mobile/jest/in-memory-sqlite.ts @@ -10,18 +10,29 @@ // cannot tell you is anything about SQLCipher, which has no Node build and // needs a device. Encryption is verified on hardware (issue #101), not here. -import { DatabaseSync } from 'node:sqlite'; +import type { DatabaseSync } from 'node:sqlite'; import type { SQLiteDatabase } from 'expo-sqlite'; -/** True when the running Node has node:sqlite (22.5+). */ -export const HAS_NODE_SQLITE = (() => { +/** + * node:sqlite's `DatabaseSync`, or null on a Node that does not have it. + * + * The require is lazy on purpose. A static `import { DatabaseSync } from + * 'node:sqlite'` is resolved before any code in this module runs, so on Node + * below 22.5 the module throws at import time and no guard written here can + * catch it - the dependent suites then error out instead of skipping. That is + * issue #133, and `engines` currently permits 20.19.4. + */ +const DatabaseSyncClass: typeof DatabaseSync | null = (() => { try { - return typeof DatabaseSync === 'function'; + return (require('node:sqlite') as typeof import('node:sqlite')).DatabaseSync; } catch { - return false; + return null; } })(); +/** True when the running Node has node:sqlite (22.5+). */ +export const HAS_NODE_SQLITE = DatabaseSyncClass !== null; + /** * expo-sqlite accepts either `run(sql, a, b)` or `run(sql, [a, b])`, and this * codebase uses both. Collapse them into one array. @@ -34,7 +45,13 @@ function normaliseParams(params: unknown[]): unknown[] { export type InMemoryDatabase = SQLiteDatabase & { readonly raw: DatabaseSync }; export function createInMemoryDatabase(): InMemoryDatabase { - const raw = new DatabaseSync(':memory:'); + if (!DatabaseSyncClass) { + throw new Error( + 'node:sqlite is unavailable - this helper needs Node 22.5 or newer. Guard the suite with HAS_NODE_SQLITE.' + ); + } + + const raw = new DatabaseSyncClass(':memory:'); const db = { raw,