fix(store): add a schema migration runner - #46
Conversation
SCHEMA_VERSION moved from 1 to 2 when run_counter was added (#12), but _init_schema only wrote the version row when it was absent -- a database created under version 1 kept reporting version 1 forever, regardless of what its tables actually looked like. _init_schema now applies an ordered, idempotent list of migrations for any version between the database's recorded version and SCHEMA_VERSION, and writes the corrected version afterward. A recorded version newer than this build supports raises SchemaVersionError rather than being silently opened. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H2jDXUwrbM4eupm7CyLivS
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e2c3181a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| CREATE TABLE IF NOT EXISTS run_counter ( | ||
| day TEXT PRIMARY KEY, | ||
| next_n INTEGER NOT NULL | ||
| ); |
There was a problem hiding this comment.
When a genuine v1 database already contains runs for the current day, this migration creates an empty run_counter, so the first subsequent next_run_id(day) returns RUN-<day>-001; because save_run upserts on run_id, that can silently overwrite the existing run with the same ID. The migration should initialize each day's counter from the maximum matching suffix already present in runs before the upgraded store is used.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and confirmed real: a v1 database can already hold today's runs under the old MAX(run_id)-in-Python scheme, and an empty counter would collide with them since save_run upserts on run_id. Fixed in 6ff293a — the migration now seeds each day's counter from the highest suffix already in use, with a test that pins the exact scenario (existing RUN-20260811-001/002, migration lands the counter at 2 so the next claim is 003).
Generated by Claude Code
| if current > SCHEMA_VERSION: | ||
| raise SchemaVersionError( | ||
| f"{self.path} reports schema version {current}, newer than the " | ||
| f"{SCHEMA_VERSION} this build supports; refusing to open it", | ||
| details={"found_version": current, "supported_version": SCHEMA_VERSION}, | ||
| ) |
There was a problem hiding this comment.
When a newer schema has renamed or removed any v1 table or indexed column, _SCHEMA has already executed before this version guard, so opening it can mutate the database with missing legacy objects or raise sqlite3.OperationalError from an incompatible index statement instead of the promised SchemaVersionError. Read or minimally bootstrap the version table and reject unsupported future versions before executing the v1 schema.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — fixed in 6ff293a. _init_schema now bootstraps and checks only the schema_version table first; _SCHEMA and the migrations only run once that guard has passed, so a future-version database gets the clean SchemaVersionError refusal instead of _SCHEMA mutating it or raising a raw sqlite3.OperationalError.
Generated by Claude Code
… applying legacy schema Addresses two review findings from Codex on #46: - P1: a v1 database can already hold runs for "today" under the old MAX(run_id)-in-Python scheme. Migrating to run_counter with an empty counter would hand out RUN-<day>-001 again, and because save_run upserts on run_id, that would silently overwrite the pre-existing run. The migration now seeds each day's counter from the highest suffix already in use. - P2: _SCHEMA was applied before the version guard ran, so a database from a future build that had renamed or dropped a v1 table could be mutated, or raise a raw sqlite3.OperationalError, instead of getting the clean SchemaVersionError refusal. The version table is now bootstrapped and checked first, before _SCHEMA or any migration runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H2jDXUwrbM4eupm7CyLivS
2 participants
What changed
SCHEMA_VERSIONmoved from 1 to 2 whenrun_counterwas added (#12), but_init_schemaonly wrote theschema_versionrow when it was absent. A database created back under version 1 kept reporting version 1 forever, regardless of what its actual tables looked like — a correctness gap, since any code branching on the stored version would silently misbehave against a pre-existing database._init_schemanow applies an ordered, idempotent list of migrations for any version between what's stored andSCHEMA_VERSION, then corrects the stored row; a version newer than this build supports raisesSchemaVersionErrorrather than being silently opened.Closes #44.
Type
Purple coverage
Checks
make checkpasses (ruff, mypy, pytest — 412 tests, 1 pre-existing skip)agentsec validate --strictpassesagentsec run --target demo-agent-fixture --profile nightlystill exits 1 with exactlyAGT-TENANT-001andAGT-MEMPOIS-001blockingNotes for the reviewer
_SCHEMAnow holds only the version-1 shape (schema_version,runs,findings,audit_log); therun_countertable that shipped in Close all twelve findings from the local deployment review #12 moved into_MIGRATIONSas the entry for version 2, so it's applied once, explicitly, rather than folded permanently into the base bootstrap script. Both the base schema and every migration must stay idempotent (IF NOT EXISTS/ equivalent), since_init_schemaruns the base script unconditionally on every open and a fresh database walks every migration in sequence rather than starting pre-upgraded.tests/test_store_migrations.pybuilds a byte-for-byte version-1 database (norun_counter, version row = 1) to exercise the real upgrade path, plus: fresh-database bootstrap lands directly onSCHEMA_VERSION; reopening an already-migrated database is a no-op (would raise if a migration re-ran withoutIF NOT EXISTS); a database claiming a version newer than this build supports raisesSchemaVersionErrorand — importantly — the stored version is not rewritten by the failed attempt, so the problem doesn't silently disappear on retry.docs/roadmap.mdandCHANGELOG.mdupdated; the "Migration runner — overdue" line moves from Core — open to the Core table as ✅.Generated by Claude Code