ReviewOS
Reload

also looking at this

stacks/uplink

fix(app): an installed database gains the columns a new build needs

#43
Merged glennmichael123 wants to merge fix/sqlite-store-migrations into main
4 files +440 -37

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

app/Uplink/desktop-agent.tsmodified+17-0
@@ -167,6 +167,10 @@ export async function startDesktopAgent(options: { version: string }): Promise<D
167167
168168 const sender = new AppleScriptSender()
169169 const store = new SqliteStore(DATABASE_PATH)
170 // A database this app cannot open no longer takes the app with it, so the
171 // one thing that must not happen is it going unmentioned.
172 if (store.schemaError)
173 console.error(`[uplink] ${store.schemaError}`)
170174 const startedAt = Date.now()
171175 let messages: MessagesDb | null = null
172176 let messagesError: string | null = null
@@ -389,6 +393,19 @@ export async function startDesktopAgent(options: { version: string }): Promise<D
389393 : automation?.detail ?? 'Uplink asks for this once the steps above are done.'),
390394 informational: false,
391395 },
396 // Only when there is something to say. An intact database is not a step
397 // anyone performs, and a broken one is not a reason to call the app
398 // unready: it is still reading texts and still answering them, it just
399 // will not remember any of it. See `SqliteStore.schemaError`.
400 ...(store.schemaError
401 ? [{
402 id: 'database',
403 name: 'History',
404 ok: false,
405 detail: store.schemaError,
406 informational: true,
407 }]
408 : []),
392409 ]
393410 // An engine the person has not selected must not hold the app in setup.
394411 const ready = checks.every(check => check.ok || check.informational)
app/Uplink/sqlite-store.tsmodified+150-33
@@ -5,46 +5,91 @@ import { dirname } from 'node:path'
55
66/**
77 * The store the downloadable app uses: one SQLite file in
8 * ~/Library/Application Support/Uplink, no framework, no migrations to run.
8 * ~/Library/Application Support/Uplink, no framework and no migration runner.
99 *
1010 * The Stacks app keeps its model-backed store (`model-store.ts`), whose tables
1111 * the dashboard reads. This one exists because a downloaded Uplink.app has no
1212 * Stacks project around it. Column names and units match the models - cost is
1313 * integer cents - so the two stay describable in one sentence.
14 *
15 * `CREATE TABLE IF NOT EXISTS` does nothing on a Mac that already has the
16 * file, so a column added to a schema string would never reach an installed
17 * copy: the first text after the update would fail on the INSERT, for every
18 * existing user, on every message. That is why the tables are described as
19 * data below rather than as one block of SQL. The same description creates a
20 * new database and tells an old one which columns it is missing, so adding a
21 * column means editing `TABLES`, and there is no second place to forget.
1422 */
1523
1624export interface RunRow extends RunRecord {
1725 id: number
1826}
1927
20const SCHEMA = `
21 CREATE TABLE IF NOT EXISTS state (key TEXT PRIMARY KEY, value TEXT NOT NULL);
22 CREATE TABLE IF NOT EXISTS conversations (
23 chat_guid TEXT PRIMARY KEY,
24 handle TEXT NOT NULL,
25 service TEXT NOT NULL DEFAULT 'iMessage',
26 session_id TEXT,
27 cwd TEXT,
28 last_active_at INTEGER,
29 more_text TEXT
30 );
31 CREATE TABLE IF NOT EXISTS runs (
32 id INTEGER PRIMARY KEY AUTOINCREMENT,
33 chat_guid TEXT NOT NULL,
34 message_guid TEXT NOT NULL,
35 prompt TEXT NOT NULL,
36 cwd TEXT,
37 status TEXT NOT NULL DEFAULT 'queued',
38 reply TEXT,
39 error TEXT,
40 session_id TEXT,
41 cost_cents INTEGER,
42 duration_ms INTEGER,
43 last_activity TEXT,
44 started_at INTEGER,
45 finished_at INTEGER
46 );
47`
28/**
29 * Stamped into `PRAGMA user_version`. Nothing branches on it today: the
30 * columns themselves are reconciled. It is recorded so a database can say how
31 * old it is, and so the day a change needs more than a new column - a backfill,
32 * a rename - has a version to key off.
33 */
34const SCHEMA_VERSION = 1
35
36export interface TableSpec {
37 name: string
38 /**
39 * Definitions in creation order. A column added here later is also added to
40 * databases that already exist, so it has to be one SQLite can ALTER in:
41 * nullable or carrying a constant default, and never PRIMARY KEY or UNIQUE.
42 *
43 * Worth knowing before trusting a green test: SQLite only enforces the
44 * NOT NULL and constant-default halves once a table has rows. An empty one
45 * accepts both. So a new column can pass on a fresh database and fail on
46 * every Mac that has been running, which is this file's own bug wearing a
47 * different hat. `tests/unit/uplink/sqlite-store.test.ts` migrates a
48 * populated database for that reason.
49 */
50 columns: Record<string, string>
51}
52
53export const TABLES: TableSpec[] = [
54 {
55 name: 'state',
56 columns: {
57 key: 'TEXT PRIMARY KEY',
58 value: 'TEXT NOT NULL',
59 },
60 },
61 {
62 name: 'conversations',
63 columns: {
64 chat_guid: 'TEXT PRIMARY KEY',
65 handle: 'TEXT NOT NULL',
66 service: 'TEXT NOT NULL DEFAULT \'iMessage\'',
67 session_id: 'TEXT',
68 cwd: 'TEXT',
69 last_active_at: 'INTEGER',
70 more_text: 'TEXT',
71 },
72 },
73 {
74 name: 'runs',
75 columns: {
76 id: 'INTEGER PRIMARY KEY AUTOINCREMENT',
77 chat_guid: 'TEXT NOT NULL',
78 message_guid: 'TEXT NOT NULL',
79 prompt: 'TEXT NOT NULL',
80 cwd: 'TEXT',
81 status: 'TEXT NOT NULL DEFAULT \'queued\'',
82 reply: 'TEXT',
83 error: 'TEXT',
84 session_id: 'TEXT',
85 cost_cents: 'INTEGER',
86 duration_ms: 'INTEGER',
87 last_activity: 'TEXT',
88 started_at: 'INTEGER',
89 finished_at: 'INTEGER',
90 },
91 },
92]
4893
4994const RUN_COLUMNS: Record<keyof RunRecord, string> = {
5095 chatGuid: 'chat_guid',
@@ -62,14 +107,67 @@ const RUN_COLUMNS: Record<keyof RunRecord, string> = {
62107 finishedAt: 'finished_at',
63108}
64109
110/**
111 * Bring a database up to the shape `TABLES` describes: create the tables it
112 * does not have, and add to the ones it does any column it is missing.
113 *
114 * Forward only, and idempotent, so it runs on every start and costs one
115 * `PRAGMA table_info` per table once a database is current. The whole thing is
116 * one transaction, which in SQLite covers the `ALTER TABLE`s and the version
117 * stamp together: a database is either fully updated or untouched, never left
118 * halfway by a Mac that went to sleep.
119 */
120export function migrate(db: Database, tables: TableSpec[] = TABLES): void {
121 db.transaction(() => {
122 for (const table of tables) {
123 // Empty means the table does not exist, which is also how a brand new
124 // file answers, so one query covers both cases.
125 const present = new Set((db.query(`PRAGMA table_info(${table.name})`).all() as Array<{ name: string }>)
126 .map(column => column.name))
127 const columns = Object.entries(table.columns)
128
129 if (present.size === 0) {
130 db.run(`CREATE TABLE ${table.name} (${columns.map(([name, definition]) => `${name} ${definition}`).join(', ')})`)
131 continue
132 }
133 for (const [name, definition] of columns) {
134 if (!present.has(name))
135 db.run(`ALTER TABLE ${table.name} ADD COLUMN ${name} ${definition}`)
136 }
137 }
138 // Interpolated, not bound: PRAGMA takes no parameter.
139 db.run(`PRAGMA user_version = ${SCHEMA_VERSION}`)
140 })()
141}
142
143export function schemaVersion(db: Database): number {
144 return (db.query('PRAGMA user_version').get() as { user_version: number }).user_version
145}
146
65147export class SqliteStore implements Store {
66148 private db: Database
67149
150 /**
151 * Why this Uplink is running without its database, if it is.
152 *
153 * The store is built at the top of the desktop agent, under launchd, where
154 * an uncaught throw is a restart loop and the menubar never appears to say
155 * why. A corrupt file, a full disk or a migration SQLite refuses would all
156 * end there. Answering texts without remembering them is worth more than an
157 * app that is not running, so the file is abandoned for an in-memory
158 * database and the popover explains it.
159 */
160 readonly schemaError: string | null = null
161
68162 constructor(path: string) {
69 mkdirSync(dirname(path), { recursive: true })
70 this.db = new Database(path, { create: true })
71 this.db.exec('PRAGMA journal_mode = WAL;')
72 this.db.exec(SCHEMA)
163 try {
164 mkdirSync(dirname(path), { recursive: true })
165 this.db = open(path)
166 }
167 catch (error) {
168 this.schemaError = `${message(error)}. Uplink is answering texts but not saving them.`
169 this.db = open(':memory:')
170 }
73171 }
74172
75173 close(): void {
@@ -147,3 +245,22 @@ export class SqliteStore implements Store {
147245 }))
148246 }
149247}
248
249function open(path: string): Database {
250 const db = new Database(path, { create: true })
251 try {
252 // A second Uplink reading the same file is the normal case during an
253 // update, and WAL is what lets it read while this one writes.
254 db.exec('PRAGMA journal_mode = WAL;')
255 migrate(db)
256 return db
257 }
258 catch (error) {
259 db.close()
260 throw error
261 }
262}
263
264function message(error: unknown): string {
265 return error instanceof Error ? error.message : String(error)
266}
tests/unit/uplink/sqlite-store.test.tsmodified+151-2
@@ -1,12 +1,13 @@
11import type { UplinkConfig } from '../../../app/Uplink/config'
22import { afterEach, describe, expect, it } from 'bun:test'
3import { mkdtempSync, rmSync } from 'node:fs'
3import { Database } from 'bun:sqlite'
4import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'
45import { tmpdir } from 'node:os'
56import { join } from 'node:path'
67import { loadConfig } from '../../../app/Uplink/config'
78import { MessagesDb } from '../../../app/Uplink/messages-db'
89import { cleanToken, readSettings, settingsEnv, tokenLooksValid, writeSettings } from '../../../app/Uplink/settings'
9import { SqliteStore } from '../../../app/Uplink/sqlite-store'
10import { migrate, schemaVersion, SqliteStore, TABLES } from '../../../app/Uplink/sqlite-store'
1011import { Uplink } from '../../../app/Uplink/uplink'
1112import { FakeChatDb, FakeEngine, FakeSender, ME, settle } from './fixtures'
1213
@@ -64,6 +65,154 @@ describe('SqliteStore', () => {
6465 })
6566})
6667
68/**
69 * The shape Uplink 0.1.3 wrote, which is what is on every Mac that already
70 * runs it. Frozen on purpose: it is the oldest database a new build has to
71 * open, so it is the thing an upgrade has to be proved against.
72 */
73const SHIPPED = {
74 state: 'key TEXT PRIMARY KEY, value TEXT NOT NULL',
75 conversations: 'chat_guid TEXT PRIMARY KEY, handle TEXT NOT NULL, service TEXT NOT NULL DEFAULT \'iMessage\', session_id TEXT, cwd TEXT, last_active_at INTEGER, more_text TEXT',
76 runs: 'id INTEGER PRIMARY KEY AUTOINCREMENT, chat_guid TEXT NOT NULL, message_guid TEXT NOT NULL, prompt TEXT NOT NULL, cwd TEXT, status TEXT NOT NULL DEFAULT \'queued\', reply TEXT, error TEXT, session_id TEXT, cost_cents INTEGER, duration_ms INTEGER, last_activity TEXT, started_at INTEGER, finished_at INTEGER',
77}
78
79function write(path: string, tables: Record<string, string>): void {
80 const db = new Database(path, { create: true })
81 for (const [name, columns] of Object.entries(tables))
82 db.run(`CREATE TABLE ${name} (${columns})`)
83 db.close()
84}
85
86function columnsOf(path: string, table: string): string[] {
87 const db = new Database(path)
88 const names = (db.query(`PRAGMA table_info(${table})`).all() as Array<{ name: string }>).map(column => column.name)
89 db.close()
90 return names
91}
92
93describe('SqliteStore migrations', () => {
94 it('creates a database that is already current', () => {
95 const path = join(tempDir(), 'uplink.sqlite')
96 const store = new SqliteStore(path)
97 expect(store.schemaError).toBeNull()
98 store.close()
99
100 for (const table of TABLES)
101 expect(columnsOf(path, table.name)).toEqual(Object.keys(table.columns))
102
103 const db = new Database(path)
104 expect(schemaVersion(db)).toBeGreaterThan(0)
105 db.close()
106 })
107
108 /**
109 * The bug this exists for. `CREATE TABLE IF NOT EXISTS` is a no-op against a
110 * database that is already there, so before this every column added to the
111 * schema was a column an installed copy would never get, and its first INSERT
112 * after the update would fail. Here `runs` is missing the four columns a
113 * newer build names and `conversations` is missing entirely.
114 */
115 it('adds what an installed database is missing, without losing its rows', async () => {
116 const path = join(tempDir(), 'uplink.sqlite')
117 write(path, {
118 state: SHIPPED.state,
119 runs: 'id INTEGER PRIMARY KEY AUTOINCREMENT, chat_guid TEXT NOT NULL, message_guid TEXT NOT NULL, prompt TEXT NOT NULL, cwd TEXT, status TEXT NOT NULL DEFAULT \'queued\', reply TEXT, error TEXT, session_id TEXT, cost_cents INTEGER',
120 })
121 const older = new Database(path)
122 older.run('INSERT INTO runs (chat_guid, message_guid, prompt, cwd, status, reply) VALUES (?, ?, ?, ?, ?, ?)', ['g', 'm', 'answered before the update', '/tmp', 'done', 'hi'])
123 older.close()
124
125 const store = new SqliteStore(path)
126 expect(store.schemaError).toBeNull()
127
128 // The history it already had is still there.
129 expect(store.recentRuns()[0]).toMatchObject({ prompt: 'answered before the update', status: 'done', reply: 'hi' })
130
131 // And the columns it did not have are usable, which is the INSERT that
132 // used to fail on every text.
133 const id = await store.createRun({ chatGuid: 'g', messageGuid: 'm2', prompt: 'after', cwd: '/tmp', status: 'queued' })
134 await store.updateRun(id, { status: 'done', durationMs: 7, lastActivity: 'Thinking', startedAt: 1, finishedAt: 8 })
135 expect(store.recentRuns()[0]).toMatchObject({ id, durationMs: 7, lastActivity: 'Thinking', startedAt: 1, finishedAt: 8 })
136
137 // A table that did not exist at all is created rather than altered.
138 await store.saveConversation({ chatGuid: 'g', handle: ME, service: 'iMessage', sessionId: 's', cwd: '/tmp', lastActiveAt: 5, moreText: null })
139 expect(await store.conversation('g')).toMatchObject({ sessionId: 's' })
140 store.close()
141 })
142
143 /**
144 * The guard that makes the mechanism worth having. SQLite will not ALTER in
145 * a PRIMARY KEY or UNIQUE column at all, and refuses a NOT NULL column with
146 * no default, or a non-constant default, **once the table has rows**. A
147 * column written any of those ways reaches every new install and no existing
148 * one, which is the failure this whole file is about, one level up.
149 *
150 * The rows are the point. On an empty table SQLite 3.51 accepts both
151 * `NOT NULL` with no default and `DEFAULT (datetime())`, so a version of
152 * this test that migrated an empty 0.1.3 database would pass while every
153 * real Mac, which has a history, failed.
154 *
155 * It passes trivially while the schema is unchanged and fails the day
156 * someone adds a column that could not travel.
157 */
158 it('can add every column to a shipped database that is already in use', () => {
159 const path = join(tempDir(), 'uplink.sqlite')
160 write(path, SHIPPED)
161 const used = new Database(path)
162 used.run('INSERT INTO state (key, value) VALUES (?, ?)', ['cursor', '1'])
163 used.run('INSERT INTO conversations (chat_guid, handle) VALUES (?, ?)', ['g', ME])
164 used.run('INSERT INTO runs (chat_guid, message_guid, prompt) VALUES (?, ?, ?)', ['g', 'm', 'p'])
165 used.close()
166
167 const store = new SqliteStore(path)
168 expect(store.schemaError).toBeNull()
169 store.close()
170
171 for (const table of TABLES)
172 expect(columnsOf(path, table.name)).toEqual(Object.keys(table.columns))
173 })
174
175 it('runs twice over the same database without changing it', () => {
176 const path = join(tempDir(), 'uplink.sqlite')
177 new SqliteStore(path).close()
178 const after = TABLES.map(table => columnsOf(path, table.name))
179 new SqliteStore(path).close()
180 expect(TABLES.map(table => columnsOf(path, table.name))).toEqual(after)
181 })
182
183 /**
184 * The store is built at the top of the desktop agent, under launchd, so a
185 * throw here is a restart loop with no menubar to explain it. A ruined file
186 * costs the history, not the app.
187 */
188 it('keeps answering texts when the file cannot be opened', async () => {
189 const path = join(tempDir(), 'uplink.sqlite')
190 writeFileSync(path, 'this is not a database')
191
192 const store = new SqliteStore(path)
193 expect(store.schemaError).toContain('not a database')
194 expect(store.schemaError).toContain('not saving them')
195
196 const id = await store.createRun({ chatGuid: 'g', messageGuid: 'm', prompt: 'still works', cwd: '/tmp', status: 'queued' })
197 expect(store.recentRuns()[0]).toMatchObject({ id, prompt: 'still works' })
198 store.close()
199 })
200
201 it('leaves a half-applied change out of the database entirely', () => {
202 const db = new Database(':memory:')
203 db.run('CREATE TABLE runs (id INTEGER PRIMARY KEY)')
204 // UNIQUE is one SQLite never allows in an ALTER, so this stands in for a
205 // migration that fails partway: the table before it was created first.
206 expect(() => migrate(db, [
207 { name: 'created_first', columns: { a: 'TEXT' } },
208 { name: 'runs', columns: { id: 'INTEGER PRIMARY KEY', nope: 'TEXT UNIQUE' } },
209 ])).toThrow()
210 expect(db.query('SELECT name FROM sqlite_master WHERE type = ?').all('table')).toEqual([{ name: 'runs' }])
211 expect(schemaVersion(db)).toBe(0)
212 db.close()
213 })
214})
215
67216describe('settings', () => {
68217 it('fills in defaults and survives a broken file', () => {
69218 const path = join(tempDir(), 'settings.json')