ReviewOS

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
9 days ago

Closes #9. This is the blocker under #10, the per-thread engine switch, which needs a new column on runs.

What was broken

SqliteStore created its tables with CREATE TABLE IF NOT EXISTS, which does nothing to a database that already exists. A column added to the schema therefore reached every new install and no existing one, and createRun builds its INSERT from RUN_COLUMNS, so the first text after that update would fail for every customer who already had the app, on every message, with no way out but deleting the file.

Nothing has been added yet. That is the only reason this is a cheap change now and a data migration later.

What it does

The tables are described as data instead of as one block of SQL, so one description both creates a new database and tells an old one which columns it lacks.

migrate(db)  ->  per table: PRAGMA table_info
                 empty     -> CREATE TABLE
                 otherwise -> ALTER TABLE ADD COLUMN for each missing one
                 then      -> PRAGMA user_version = SCHEMA_VERSION

Forward only, idempotent, and when there is work it is one transaction. I checked that SQLite rolls the ALTERs and the version stamp back together, so a Mac that sleeps mid-upgrade leaves the database untouched rather than half-updated.

Opening can also fail outright. A ruined uplink.sqlite used to throw out of startDesktopAgent's first statements, under launchd, which is a restart loop with no menubar left to explain it. The file is now abandoned for an in-memory database, and the popover says so in every state. Answering texts without remembering them beats an app that is not running.

The part that nearly got past me

My first guard test migrated an empty 0.1.3-shaped database and passed. Then I checked what SQLite 3.51 actually enforces:

column being addedempty tabletable with rows
NOT NULL, no defaultacceptedrejected
DEFAULT (datetime())acceptedrejected
UNIQUE / PRIMARY KEYrejectedrejected

A column that cannot reach a single real customer passes cleanly against an empty database. That is this bug again, in the test written to catch it. The guard now seeds a row into every table first, and the reason is recorded in both the test and the source.

What the audit changed

I ran an adversarial pass over the first commit: four lenses, every finding handed to a separate agent whose job was to refute it. 23 raised, 17 survived. Three mattered.

A start that changed nothing still took a write lock. migrate() opened a transaction unconditionally, and during an update two Uplinks hold the file for a moment. Losing that race looked identical to a ruined database, so the app abandoned a perfectly healthy file. Measured, on a current database with another process holding the lock:

result
first commitdatabase is locked, falls back to memory, history gone for that launch
nowopens the real file, no error

The work is decided by reading first, so an already-current database returns before any transaction opens, which is almost every launch. busy_timeout covers the launch that does have work.

The menubar check was dead code. menubar.html renders five hardcoded step ids and ignores the rest, and those steps sit inside a section hidden outside the setup state, which is the one state the check could never be in. A customer with a ruined database saw a green dot and "Answering texts from you". It is status.notice now, rendered in every state, and the text says what happened and what to do instead of quoting SQLite.

The header comment was wrong. It claimed a column is added in one place. RUN_COLUMNS names them too, and TypeScript cannot tie it to TABLES. A test pins both directions now, because that drift is #9 again.

Also: column assertions compare sets rather than lists, since ALTER appends and the old assertion would have failed a correct upgrade; the version stamp is asserted on an upgraded database, not only a fresh one; and a table added after 0.1.3 now fails the guard until someone decides whether it needs covering.

Verification

  • 118 unit tests pass, 14 in this file, 8 of them new
  • ./buddy lint and ./buddy typecheck clean
  • the desktop entry still bundles framework-free, and bun build --compile still produces a binary
  • the menubar's inline script parses after the edit

Deliberately not here

Two things the audit found that belong to other code, filed separately rather than widening this: settings.json can still restart-loop the app under launchd, which is the failure this change removed for the database, and license.json is unversioned with a stale valid: true trusted for 14 days.

🤖 Generated with Claude Code

4 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.