From 6df5031119e5a8e6fcedd7a854626ce8a6ff43c0 Mon Sep 17 00:00:00 2001 From: JasonZQH Date: Tue, 28 Jul 2026 13:24:01 -0700 Subject: [PATCH 1/3] feat(state): back up the ledger before migrating it, and migrate atomically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every command that opens .curator/ runs pending migrations, and until now it did so on the live ledger with no copy and no transaction. v0.1.4 adds the Execution tables and dual-writes; that is the first migration with real scope to damage user data, and it should not land before there is a way back. Three gaps, all in initialize_database: No backup. The ledger is now copied to .curator/archive/ before anything writes, but only when there is actually something to migrate — with 114 call sites, copying on every open would be absurd. It uses sqlite3's online backup API, not shutil.copy: the ledger runs in WAL mode, and a copy of the main file alone can miss committed pages still in the -wal. Verified — a file copy of a freshly written ledger cannot even see the table. No transaction. Migrations committed one at a time, so a failure on the third left the first two applied and the ledger in a state no version is written to read. They now share one transaction and roll back together, DDL included. No visibility. Nothing could answer "what will opening this ledger do". curator doctor now reports pending migrations without applying them — it only ever used connect_database, never initialize_database, so it is a dry run by construction rather than by a flag. Backups are timestamped rather than named for the schema version they precede: a migration number is an internal counter, and "v3" reads like a SQLite version to the person who has to find the file. Since a timestamp is no easier to recall, doctor prints the newest backup with the full restore command, and finds it by reading the directory so it still answers when the ledger will not open. Naming is shared with reset's archive so the two cannot drift. Rollback stays documented rather than a command: restoring is one cp, and a command that overwrites a live ledger needs a confirmation gate and a test surface the roadmap did not ask for. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 20 +++ README.md | 12 +- src/curator/cli.py | 14 +- src/curator/diagnostics/doctor.py | 49 ++++++- src/curator/init/reset.py | 8 +- src/curator/rendering/terminal.py | 23 +++- src/curator/state/backup.py | 79 +++++++++++ src/curator/state/db.py | 114 +++++++++++++--- tests/test_migration_safety.py | 209 ++++++++++++++++++++++++++++++ 9 files changed, 500 insertions(+), 28 deletions(-) create mode 100644 src/curator/state/backup.py create mode 100644 tests/test_migration_safety.py diff --git a/CHANGELOG.md b/CHANGELOG.md index d6d0493..a87a304 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,26 @@ All notable changes to Curator are documented here. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Added +- **The ledger is backed up before a schema migration.** Opening `.curator/` with a newer + Curator applies pending migrations automatically; it now copies the ledger to + `.curator/archive/…-pre-migration.sqlite` first. The copy uses SQLite's online backup + API rather than a file copy, so data still sitting in the write-ahead log is preserved. + A ledger with nothing to migrate — the usual case, since every command opens it — is not + copied. +- **`curator doctor` reports pending migrations and the newest backup.** It lists what the + next open will apply without applying it, and prints a ready-to-paste `cp` command to + restore the most recent backup. The backup lookup reads the directory, not the ledger, so + it still answers when the ledger will not open. + +### Fixed +- **A failed migration no longer leaves a half-migrated ledger.** Pending migrations and + their `schema_version` rows now share one transaction, so a failure rolls the schema + changes back with it instead of stopping in an intermediate state and raises + `CuratorStateError`. Previously each migration committed on its own. + ## [0.1.1] — 2026-07-28 ### Fixed diff --git a/README.md b/README.md index 5790ec3..ed25009 100644 --- a/README.md +++ b/README.md @@ -138,7 +138,7 @@ Full history in [CHANGELOG.md](CHANGELOG.md). | `curator provider add ` | Detect and register a provider CLI (`claude-code` / `codex`) | | `curator provider list` | List configured provider profiles | | `curator status` | Show current project state | -| `curator doctor` | Run environment and readiness checks | +| `curator doctor` | Run environment and readiness checks, including pending schema migrations | | `curator reset` | Archive the ledger and clear runtime state (`--hard` removes `.curator/`) | | `curator contract validate` | Validate editable role contracts | | `curator --version` | Print the version | @@ -157,6 +157,16 @@ Full history in [CHANGELOG.md](CHANGELOG.md). TUI adds Up/Down history, Tab completion, Shift+Enter/Ctrl+J continuation lines, Esc interruption, and two-stage Ctrl+C shutdown. +**Upgrades and your ledger.** Opening `.curator/` with a newer Curator applies any pending +schema migrations automatically. Before it touches anything, the ledger is copied to +`.curator/archive/…-pre-migration.sqlite`, and the migration runs as a single transaction — +if it fails, the ledger is left exactly as it was. `curator doctor` shows what is pending +before it runs and, afterwards, prints the newest backup with the command to restore it: + +```bash +cp .curator/archive/curator--pre-migration.sqlite .curator/curator.sqlite +``` + ## Known limitations Phase 0 is deliberately narrow. Where a claim above could be read more broadly than the code diff --git a/src/curator/cli.py b/src/curator/cli.py index d7e15c2..39b965e 100644 --- a/src/curator/cli.py +++ b/src/curator/cli.py @@ -21,6 +21,7 @@ from curator.rendering.terminal import ( render_contract_validation_report, render_doctor_report, + render_migration_outcome, render_status_report, ) from curator.providers.setup import add_provider_profile, resolve_provider_name @@ -66,6 +67,13 @@ def _echo_init_summary(created_files_count: int, skipped_files_count: int) -> No typer.echo(f"Skipped existing files: {skipped_files_count}") +def _echo_migration(outcome) -> None: + """Print the migration notice when opening the ledger actually migrated it.""" + message = render_migration_outcome(outcome) + if message: + typer.echo(message) + + def _ensure_curator_state(root: Path, yes: bool) -> None: """Create Curator state when requested or raise for interactive approval.""" if not yes: @@ -232,10 +240,11 @@ def provider_add_command( raise typer.Exit(1) connection = connect_database(paths.database) try: - initialize_database(connection) + migration = initialize_database(connection) result = add_provider_profile(connection, name) finally: connection.close() + _echo_migration(migration) typer.echo(result.message) if not result.created: raise typer.Exit(1) @@ -255,10 +264,11 @@ def provider_list_command() -> None: return connection = connect_database(paths.database) try: - initialize_database(connection) + migration = initialize_database(connection) profiles = load_provider_profiles(connection) finally: connection.close() + _echo_migration(migration) if not profiles: typer.echo("Providers:\n- none (run curator provider add )") return diff --git a/src/curator/diagnostics/doctor.py b/src/curator/diagnostics/doctor.py index cf46e40..fcecc4b 100644 --- a/src/curator/diagnostics/doctor.py +++ b/src/curator/diagnostics/doctor.py @@ -8,7 +8,8 @@ from curator.core.paths import build_curator_paths from curator.diagnostics.models import DoctorCheck, DoctorReport from curator.providers.profiles import resolve_runtime_mode -from curator.state.db import connect_database +from curator.state.backup import latest_pre_migration_backup +from curator.state.db import connect_database, pending_migrations def _check_existing_path(path: Path, ok_detail: str, missing_detail: str) -> DoctorCheck: @@ -56,10 +57,56 @@ def inspect_project_health(project_root: Path | str) -> DoctorReport: "package": DoctorCheck(status="ok", detail=f"curator {__version__}"), "state": state_check, "database": database_check, + "migrations": _migrations_check(paths.database) if initialized else _NO_LEDGER_CHECK, + "backup": _backup_check(paths.curator_dir), }, ) +_NO_LEDGER_CHECK = DoctorCheck(status="missing", detail="No ledger yet.") + + +def _migrations_check(database: Path) -> DoctorCheck: + """Report which migrations the next open will apply, without applying them.""" + try: + connection = connect_database(database) + try: + pending = pending_migrations(connection) + finally: + connection.close() + except sqlite3.DatabaseError as error: + return DoctorCheck(status="fail", detail=f"Cannot read the ledger: {error}") + + if not pending: + return DoctorCheck(status="ok", detail="Schema is current.") + + versions = ", ".join(str(version) for version in pending) + return DoctorCheck( + status="pending", + detail=( + f"{len(pending)} migration(s) pending ({versions}); the next command that opens " + "the ledger applies them, backing it up to .curator/archive/ first." + ), + ) + + +def _backup_check(curator_dir: Path) -> DoctorCheck: + """Point at the newest pre-migration backup and how to restore it. + + Reads the directory rather than the ledger, so this still answers when the ledger will + not open — which is when someone actually needs it. + """ + backup = latest_pre_migration_backup(curator_dir) + if backup is None: + return DoctorCheck(status="none", detail="No pre-migration backup taken yet.") + + database = curator_dir / "curator.sqlite" + return DoctorCheck( + status="ok", + detail=f"Newest pre-migration backup: {backup}\n restore: cp {backup} {database}", + ) + + def _resolve_mode(database: Path) -> str: """Return the runtime mode label for an existing database.""" try: diff --git a/src/curator/init/reset.py b/src/curator/init/reset.py index d138d2b..ba45506 100644 --- a/src/curator/init/reset.py +++ b/src/curator/init/reset.py @@ -2,11 +2,12 @@ import shutil from dataclasses import dataclass, field -from datetime import UTC, datetime +from datetime import datetime from pathlib import Path from curator.core.paths import build_curator_paths from curator.core.schema import CuratorPaths +from curator.state.backup import archive_stamp, ledger_archive_dir @dataclass(frozen=True) @@ -67,10 +68,9 @@ def reset_curator_state( archived_database = None if paths.database.exists(): - stamp = (now or datetime.now(UTC)).strftime("%Y%m%dT%H%M%SZ") - archive_dir = paths.curator_dir / "archive" + archive_dir = ledger_archive_dir(paths.curator_dir) archive_dir.mkdir(parents=True, exist_ok=True) - archived_database = archive_dir / f"curator-{stamp}.sqlite" + archived_database = archive_dir / f"curator-{archive_stamp(now)}.sqlite" shutil.move(str(paths.database), str(archived_database)) removed_paths = [] diff --git a/src/curator/rendering/terminal.py b/src/curator/rendering/terminal.py index 6de6708..a2a9dfe 100644 --- a/src/curator/rendering/terminal.py +++ b/src/curator/rendering/terminal.py @@ -20,8 +20,29 @@ def render_doctor_report(report: DoctorReport) -> str: f"State: {report.checks['state'].status}", f"Database: {report.checks['database'].status}", f"Mode: {report.mode}", - f"Recommended next step: {report.recommended_next_step}", ] + migrations = report.checks.get("migrations") + if migrations is not None and migrations.status != "missing": + lines.append(f"Migrations: {migrations.status} — {migrations.detail}") + backup = report.checks.get("backup") + if backup is not None and backup.status == "ok": + lines.append(f"Backup: {backup.detail}") + lines.append(f"Recommended next step: {report.recommended_next_step}") + return "\n".join(lines) + + +def render_migration_outcome(outcome) -> str: + """Render what a just-applied migration did, or "" when nothing was migrated. + + Migrating a ledger is not something to do silently: the user should learn that their + schema moved and where the copy of the old one is, at the moment it happens. + """ + if outcome is None: + return "" + versions = ", ".join(str(version) for version in outcome.applied) + lines = [f"Migrated ledger: schema v{outcome.from_version} → v{outcome.to_version} ({versions})."] + if outcome.backup is not None: + lines.append(f"Previous ledger backed up to {outcome.backup}") return "\n".join(lines) diff --git a/src/curator/state/backup.py b/src/curator/state/backup.py new file mode 100644 index 0000000..cdd11e6 --- /dev/null +++ b/src/curator/state/backup.py @@ -0,0 +1,79 @@ +"""Copy the Curator ledger aside before a schema migration rewrites it.""" + +import sqlite3 +from datetime import UTC, datetime +from pathlib import Path + +# Migration backups and reset archives are both "a ledger as it was", so they share one +# directory and one timestamp format. The suffix is what tells them apart on disk. +_ARCHIVE_DIR_NAME = "archive" +_STAMP_FORMAT = "%Y%m%dT%H%M%SZ" +PRE_MIGRATION_SUFFIX = "-pre-migration" + + +def ledger_archive_dir(curator_dir: Path) -> Path: + """Return the directory holding superseded copies of the ledger.""" + return Path(curator_dir) / _ARCHIVE_DIR_NAME + + +def archive_stamp(now: datetime | None = None) -> str: + """Return the UTC timestamp used to name an archived ledger.""" + return (now or datetime.now(UTC)).strftime(_STAMP_FORMAT) + + +def backup_ledger( + connection: sqlite3.Connection, + curator_dir: Path, + now: datetime | None = None, +) -> Path: + """Copy the open ledger into the archive directory and return the backup path. + + Uses SQLite's online backup API rather than copying the file. The ledger runs in WAL + mode, so a file copy can miss pages still sitting in the write-ahead log and produce a + backup that is silently short of the data it is supposed to preserve. + """ + directory = ledger_archive_dir(curator_dir) + directory.mkdir(parents=True, exist_ok=True) + + destination_path = _free_backup_path(directory, archive_stamp(now)) + destination = sqlite3.connect(destination_path) + try: + connection.backup(destination) + finally: + destination.close() + return destination_path + + +def latest_pre_migration_backup(curator_dir: Path) -> Path | None: + """Return the newest pre-migration backup, or None when none has been taken. + + Reads the directory rather than the ledger, so it still answers when the ledger itself + will not open — which is exactly when someone is looking for a backup. + """ + directory = ledger_archive_dir(curator_dir) + if not directory.is_dir(): + return None + + backups = list(directory.glob(f"curator-*{PRE_MIGRATION_SUFFIX}.sqlite")) + if not backups: + return None + # By mtime, not by name: a same-second backup is disambiguated with an index that + # sorts before the plain name, so lexicographic order would report the oldest. + return max(backups, key=lambda path: path.stat().st_mtime) + + +def _free_backup_path(directory: Path, stamp: str) -> Path: + """Return a backup path that does not already exist. + + Two migrations inside the same second would otherwise resolve to one filename and the + second would overwrite the first — discarding the older ledger state. + """ + candidate = directory / f"curator-{stamp}{PRE_MIGRATION_SUFFIX}.sqlite" + if not candidate.exists(): + return candidate + + for index in range(2, 100): + candidate = directory / f"curator-{stamp}-{index}{PRE_MIGRATION_SUFFIX}.sqlite" + if not candidate.exists(): + return candidate + raise FileExistsError(f"cannot find a free backup name in {directory}") diff --git a/src/curator/state/db.py b/src/curator/state/db.py index 6a64b84..47666ed 100644 --- a/src/curator/state/db.py +++ b/src/curator/state/db.py @@ -1,13 +1,26 @@ """Manage SQLite connections and database initialization.""" import sqlite3 +from dataclasses import dataclass from datetime import UTC, datetime from pathlib import Path from typing import Callable +from curator.core.errors import CuratorStateError +from curator.state.backup import backup_ledger from curator.state.migrations import phase0_schema_sql +@dataclass(frozen=True) +class MigrationOutcome: + """Describe the schema migrations one initialization actually applied.""" + + applied: tuple[int, ...] + from_version: int + to_version: int + backup: Path | None = None + + class CuratorConnection(sqlite3.Connection): """Track Curator-owned transaction nesting without mutating sqlite3.Connection.""" @@ -96,25 +109,88 @@ def _ensure_memory_entry_learning_columns(connection: sqlite3.Connection) -> Non ) -def _apply_versioned_migrations(connection: sqlite3.Connection) -> None: - """Run numbered migrations that are not yet recorded in schema_version.""" - applied = { - row["version"] - for row in connection.execute("select version from schema_version").fetchall() - } - for version, migration in VERSIONED_MIGRATIONS: - if version in applied: - continue +def applied_migrations(connection: sqlite3.Connection) -> set[int]: + """Return the migration versions this ledger has already recorded. + + A ledger old enough to predate the schema_version table has recorded nothing, which is + a valid state to migrate from rather than an error — and this is called from read-only + callers like doctor that never create the table. + """ + try: + rows = connection.execute("select version from schema_version").fetchall() + except sqlite3.OperationalError: + return set() + return {row["version"] for row in rows} + + +def pending_migrations(connection: sqlite3.Connection) -> list[int]: + """Return the migration versions this ledger has not applied yet, in order.""" + already = applied_migrations(connection) + return [version for version, _ in VERSIONED_MIGRATIONS if version not in already] + + +def _apply_versioned_migrations(connection: sqlite3.Connection, pending: list[int]) -> None: + """Apply pending migrations as one all-or-nothing unit. + + Every migration and its schema_version row share a single transaction: SQLite rolls + DDL back with everything else, so a failure half way through leaves the ledger exactly + as it was rather than in a state no version of Curator is written to read. + """ + migrations = dict(VERSIONED_MIGRATIONS) + connection.begin_transaction() + try: + for version in pending: + migrations[version](connection) + connection.execute( + "insert into schema_version (version, applied_at) values (?, ?)", + (version, datetime.now(UTC).isoformat()), + ) + connection.commit_transaction() + except Exception as error: + connection.rollback_transaction() + raise CuratorStateError(f"schema migration failed and was rolled back: {error}") from error + + +def _ledger_database_path(connection: sqlite3.Connection) -> Path | None: + """Return the file backing this connection, or None for an in-memory ledger.""" + for row in connection.execute("pragma database_list").fetchall(): + if row["name"] == "main" and row["file"]: + return Path(row["file"]) + return None + + +def _ledger_has_content(connection: sqlite3.Connection) -> bool: + """Report whether this ledger already holds objects worth preserving.""" + row = connection.execute("select count(*) as count from sqlite_master").fetchone() + return bool(row["count"]) + + +def initialize_database(connection: sqlite3.Connection) -> MigrationOutcome | None: + """Create the Phase 0 SQLite tables and apply pending migrations. + + Returns what was migrated, or None when the ledger was already current — which is the + common case, since every command that opens the ledger lands here. An existing ledger + with work to do is copied to .curator/archive/ first, before anything writes to it. + """ + pending = pending_migrations(connection) + previous = max(applied_migrations(connection), default=0) + + backup: Path | None = None + if pending and _ledger_has_content(connection): + database_path = _ledger_database_path(connection) + if database_path is not None: + backup = backup_ledger(connection, database_path.parent) - migration(connection) - connection.execute( - "insert into schema_version (version, applied_at) values (?, ?)", - (version, datetime.now(UTC).isoformat()), - ) - - -def initialize_database(connection: sqlite3.Connection) -> None: - """Create the Phase 0 SQLite tables and apply pending migrations.""" connection.executescript(phase0_schema_sql()) - _apply_versioned_migrations(connection) + if pending: + _apply_versioned_migrations(connection, pending) connection.commit() + + if not pending: + return None + return MigrationOutcome( + applied=tuple(pending), + from_version=previous, + to_version=max(pending), + backup=backup, + ) diff --git a/tests/test_migration_safety.py b/tests/test_migration_safety.py new file mode 100644 index 0000000..e24d973 --- /dev/null +++ b/tests/test_migration_safety.py @@ -0,0 +1,209 @@ +"""Verify a schema migration backs the ledger up, is atomic, and stays visible. + +Every command that opens the ledger runs migrations, so the guarantees under test are: +the old ledger is preserved before anything writes, a failure leaves the ledger exactly as +it was, and nothing migrates without a way to see it coming. +""" + +import sqlite3 + +import pytest + +from curator.core.errors import CuratorStateError +from curator.core.paths import build_curator_paths +from curator.diagnostics.doctor import inspect_project_health +from curator.state.backup import ( + backup_ledger, + latest_pre_migration_backup, + ledger_archive_dir, +) +from curator.state.db import ( + connect_database, + initialize_database, + pending_migrations, +) + + +def _open(tmp_path): + """Return a connection to this project's ledger.""" + return connect_database(build_curator_paths(tmp_path).database) + + +def _backups(tmp_path): + """Return every pre-migration backup on disk for this project.""" + directory = ledger_archive_dir(build_curator_paths(tmp_path).curator_dir) + return sorted(directory.glob("*-pre-migration.sqlite")) if directory.is_dir() else [] + + +def _schema_rows(connection): + """Return the schema_version table as comparable tuples.""" + return [ + (row["version"], row["applied_at"]) + for row in connection.execute("select version, applied_at from schema_version") + ] + + +def _add_migration(monkeypatch, version, migration): + """Append one migration to the registry for the duration of a test.""" + from curator.state import db + + monkeypatch.setattr( + db, "VERSIONED_MIGRATIONS", (*db.VERSIONED_MIGRATIONS, (version, migration)) + ) + + +def test_a_fresh_ledger_is_not_backed_up(tmp_path): + """Verify creating a ledger writes no backup — there is nothing to preserve.""" + connection = _open(tmp_path) + + outcome = initialize_database(connection) + + assert outcome is not None and outcome.applied == (1, 2) + assert outcome.backup is None + assert _backups(tmp_path) == [] + connection.close() + + +def test_an_up_to_date_ledger_reports_nothing_and_writes_no_backup(tmp_path): + """Verify the common case — every command opens the ledger — stays free of copies.""" + connection = _open(tmp_path) + initialize_database(connection) + + for _ in range(10): + assert initialize_database(connection) is None + + assert _backups(tmp_path) == [] + connection.close() + + +def test_a_pending_migration_backs_the_ledger_up_first(tmp_path, monkeypatch): + """Verify an existing ledger is copied aside before a new migration touches it.""" + connection = _open(tmp_path) + initialize_database(connection) + _add_migration( + monkeypatch, + 3, + lambda conn: conn.execute("alter table memory_entries add column probe text"), + ) + + outcome = initialize_database(connection) + + assert outcome is not None and outcome.applied == (3,) + assert outcome.from_version == 2 and outcome.to_version == 3 + assert outcome.backup is not None and outcome.backup.exists() + assert _backups(tmp_path) == [outcome.backup] + + # The backup is a real ledger, and it predates the migration. + restored = sqlite3.connect(outcome.backup) + columns = {row[1] for row in restored.execute("pragma table_info(memory_entries)")} + assert "probe" not in columns + restored.close() + connection.close() + + +def test_a_failed_migration_leaves_the_ledger_untouched(tmp_path, monkeypatch): + """Verify a migration that raises rolls back its DDL and its schema_version row.""" + connection = _open(tmp_path) + initialize_database(connection) + before_schema = _schema_rows(connection) + before_columns = {row["name"] for row in connection.execute("pragma table_info(memory_entries)")} + + def _doomed(conn): + """Change the schema, then fail — the change must not survive.""" + conn.execute("alter table memory_entries add column half_applied text") + raise RuntimeError("migration exploded") + + _add_migration(monkeypatch, 3, _doomed) + + with pytest.raises(CuratorStateError): + initialize_database(connection) + + assert _schema_rows(connection) == before_schema # version 3 was never recorded + after_columns = {row["name"] for row in connection.execute("pragma table_info(memory_entries)")} + assert after_columns == before_columns # the DDL rolled back with it + + backups = _backups(tmp_path) + assert len(backups) == 1 # and the pre-migration copy is still there to fall back on + sqlite3.connect(backups[0]).close() + connection.close() + + +def test_the_backup_captures_data_still_sitting_in_the_wal(tmp_path): + """Verify the online backup API is used, not a file copy. + + The ledger runs in WAL mode, so a committed row can live in the -wal file until a + checkpoint. Copying curator.sqlite alone would silently lose it. + """ + connection = _open(tmp_path) + initialize_database(connection) + connection.execute("create table wal_probe (value text)") + connection.execute("insert into wal_probe values ('kept')") + connection.commit() + + wal = build_curator_paths(tmp_path).database.with_name("curator.sqlite-wal") + assert wal.exists() and wal.stat().st_size > 0 # the row really is in the WAL + + backup = backup_ledger(connection, build_curator_paths(tmp_path).curator_dir) + + restored = sqlite3.connect(backup) + assert [row[0] for row in restored.execute("select value from wal_probe")] == ["kept"] + restored.close() + connection.close() + + +def test_pending_migrations_treats_a_ledger_without_schema_version_as_unmigrated(tmp_path): + """Verify a ledger predating the schema_version table is migratable, not an error.""" + connection = _open(tmp_path) + + assert pending_migrations(connection) == [1, 2] # no schema_version table exists yet + connection.close() + + +def test_doctor_reports_pending_migrations_without_applying_them(tmp_path, monkeypatch): + """Verify the dry-run really is dry — doctor must never migrate the ledger.""" + connection = _open(tmp_path) + initialize_database(connection) + before = _schema_rows(connection) + connection.close() + + _add_migration(monkeypatch, 3, lambda conn: conn.execute("select 1")) + + report = inspect_project_health(tmp_path) + inspect_project_health(tmp_path) + + assert report.checks["migrations"].status == "pending" + assert "3" in report.checks["migrations"].detail + + connection = _open(tmp_path) + assert _schema_rows(connection) == before + assert pending_migrations(connection) == [3] + connection.close() + + +def test_doctor_points_at_the_newest_backup_with_a_restore_command(tmp_path, monkeypatch): + """Verify recovery never requires remembering a timestamp.""" + connection = _open(tmp_path) + initialize_database(connection) + _add_migration(monkeypatch, 3, lambda conn: conn.execute("select 1")) + outcome = initialize_database(connection) + connection.close() + + check = inspect_project_health(tmp_path).checks["backup"] + + assert check.status == "ok" + assert str(outcome.backup) in check.detail + assert "cp " in check.detail and "curator.sqlite" in check.detail + + +def test_latest_backup_is_the_newest_one_not_the_last_alphabetically(tmp_path): + """Verify same-second backups do not make an older copy look like the newest.""" + curator_dir = build_curator_paths(tmp_path).curator_dir + connection = _open(tmp_path) + initialize_database(connection) + + first = backup_ledger(connection, curator_dir) + second = backup_ledger(connection, curator_dir) # same second -> indexed name + + assert first != second + assert latest_pre_migration_backup(curator_dir) == second + connection.close() From 4f37c2ce9039bb4d87e030a2f897e37992aaf691 Mon Sep 17 00:00:00 2001 From: JasonZQH Date: Tue, 28 Jul 2026 13:51:21 -0700 Subject: [PATCH 2/3] fix(state): never offer a half-written backup as a restore point MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found reviewing #42. sqlite3.connect creates the destination file before connection.backup() runs, and backup_ledger had no cleanup on the failure path — so a backup interrupted by a full disk, an I/O error, or a killed process left a 0-byte file carrying the real ...-pre-migration.sqlite name. That file is the newest, so latest_pre_migration_backup selected it and curator doctor printed it as the ledger to restore. Following that advice copies an empty database over a live ledger and destroys it — on exactly the disaster path this code exists to protect. Reproduced before fixing: with a good 290KB backup already on disk, one failed attempt left a 0-byte sibling and latest_pre_migration_backup returned the 0-byte one. Backups are now written to a .partial name the restore glob does not match and renamed into place only after the copy completes, so an incomplete backup can never carry the real name — this covers the killed-process case too, which cleanup in an except block would not. The partial is removed on failure, and an unusable file is skipped when choosing what to advertise. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 6 +++++ src/curator/state/backup.py | 27 +++++++++++++++++---- tests/test_migration_safety.py | 43 ++++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a87a304..5427845 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,12 @@ All notable changes to Curator are documented here. The format follows it still answers when the ledger will not open. ### Fixed +- **A failed backup is never offered as a restore point.** `sqlite3.connect` creates the + destination up front, so a backup interrupted by a full disk, an I/O error, or a killed + process left a truncated file carrying the real name. Being the newest, it became what + `curator doctor` advertised as the ledger to restore — and copying it over a live ledger + would have destroyed it. Backups are now written under a name the restore path ignores and + renamed into place only once complete, and an unusable file is never advertised. - **A failed migration no longer leaves a half-migrated ledger.** Pending migrations and their `schema_version` rows now share one transaction, so a failure rolls the schema changes back with it instead of stopping in an intermediate state and raises diff --git a/src/curator/state/backup.py b/src/curator/state/backup.py index cdd11e6..5ca4a60 100644 --- a/src/curator/state/backup.py +++ b/src/curator/state/backup.py @@ -36,11 +36,22 @@ def backup_ledger( directory.mkdir(parents=True, exist_ok=True) destination_path = _free_backup_path(directory, archive_stamp(now)) - destination = sqlite3.connect(destination_path) + # Write under a name the restore path ignores, then publish by renaming. sqlite3.connect + # creates the file up front, so a backup that dies half way — disk full, I/O error, + # killed process — would otherwise leave a truncated file carrying the real name. That + # file is the newest, so it becomes what doctor offers as the restore point, and copying + # it over a live ledger destroys the ledger. Only a completed backup gets the real name. + partial_path = destination_path.with_suffix(".partial") try: - connection.backup(destination) - finally: - destination.close() + destination = sqlite3.connect(partial_path) + try: + connection.backup(destination) + finally: + destination.close() + partial_path.replace(destination_path) + except BaseException: + partial_path.unlink(missing_ok=True) + raise return destination_path @@ -54,7 +65,13 @@ def latest_pre_migration_backup(curator_dir: Path) -> Path | None: if not directory.is_dir(): return None - backups = list(directory.glob(f"curator-*{PRE_MIGRATION_SUFFIX}.sqlite")) + # An empty file is never a usable ledger, and this path tells someone what to copy over + # a live one, so anything that cannot be a backup is skipped rather than offered. + backups = [ + path + for path in directory.glob(f"curator-*{PRE_MIGRATION_SUFFIX}.sqlite") + if path.stat().st_size > 0 + ] if not backups: return None # By mtime, not by name: a same-second backup is disambiguated with an index that diff --git a/tests/test_migration_safety.py b/tests/test_migration_safety.py index e24d973..61d440e 100644 --- a/tests/test_migration_safety.py +++ b/tests/test_migration_safety.py @@ -195,6 +195,49 @@ def test_doctor_points_at_the_newest_backup_with_a_restore_command(tmp_path, mon assert "cp " in check.detail and "curator.sqlite" in check.detail +def test_a_failed_backup_is_never_offered_as_a_restore_point(tmp_path, monkeypatch): + """Verify a backup that dies half way cannot become what doctor tells you to restore. + + sqlite3.connect creates the destination up front, so a failure mid-copy used to leave a + truncated file carrying the real name. Being newest, it shadowed every good backup — and + the restore command copies it over the live ledger, destroying it. + """ + from curator.state.db import CuratorConnection + + curator_dir = build_curator_paths(tmp_path).curator_dir + connection = _open(tmp_path) + initialize_database(connection) + good = backup_ledger(connection, curator_dir) + + def _explode(self, target, **kwargs): + """Fail the way a full disk does — after the destination file already exists.""" + raise sqlite3.OperationalError("disk I/O error") + + monkeypatch.setattr(CuratorConnection, "backup", _explode, raising=False) + with pytest.raises(sqlite3.OperationalError): + backup_ledger(connection, curator_dir) + monkeypatch.undo() + + assert _backups(tmp_path) == [good] # the failed attempt left nothing behind + assert latest_pre_migration_backup(curator_dir) == good + assert not list(ledger_archive_dir(curator_dir).glob("*.partial")) + connection.close() + + +def test_an_empty_backup_file_is_not_offered_as_a_restore_point(tmp_path): + """Verify a zero-byte file in the archive never becomes the advertised restore point.""" + curator_dir = build_curator_paths(tmp_path).curator_dir + connection = _open(tmp_path) + initialize_database(connection) + good = backup_ledger(connection, curator_dir) + + stray = ledger_archive_dir(curator_dir) / "curator-29990101T000000Z-pre-migration.sqlite" + stray.touch() # newer by name and mtime, but empty + + assert latest_pre_migration_backup(curator_dir) == good + connection.close() + + def test_latest_backup_is_the_newest_one_not_the_last_alphabetically(tmp_path): """Verify same-second backups do not make an older copy look like the newest.""" curator_dir = build_curator_paths(tmp_path).curator_dir From 788ff17458cba8e998529ebcd01a15be94aa1807 Mon Sep 17 00:00:00 2001 From: JasonZQH Date: Tue, 28 Jul 2026 13:58:42 -0700 Subject: [PATCH 3/3] =?UTF-8?q?chore(release):=20v0.1.2=20=E2=80=94=20migr?= =?UTF-8?q?ation=20safety?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moves both version declarations to 0.1.2, dates the changelog entry, and points the README install pins and version table at the new tag. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 3 ++- README.md | 11 ++++++----- pyproject.toml | 2 +- src/curator/__init__.py | 2 +- 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5427845..d02c3f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ All notable changes to Curator are documented here. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [Unreleased] +## [0.1.2] — 2026-07-28 ### Added - **The ledger is backed up before a schema migration.** Opening `.curator/` with a newer @@ -80,5 +80,6 @@ Serial single-writer (local `flock`, no cross-host coordination); the decisions/ — not the provider transcript — are the system of record; macOS primary, Linux in CI, Windows via WSL2 only. +[0.1.2]: https://github.com/JasonZQH/CURATOR/releases/tag/v0.1.2 [0.1.1]: https://github.com/JasonZQH/CURATOR/releases/tag/v0.1.1 [0.1.0]: https://github.com/JasonZQH/CURATOR/releases/tag/v0.1.0 diff --git a/README.md b/README.md index ed25009..3961516 100644 --- a/README.md +++ b/README.md @@ -70,11 +70,11 @@ curl -fsSL https://raw.githubusercontent.com/JasonZQH/CURATOR/main/install.sh | Or with pipx / uv: ```bash -pipx install "git+https://github.com/JasonZQH/CURATOR.git@v0.1.1" -uvx --from "git+https://github.com/JasonZQH/CURATOR.git@v0.1.1" curator +pipx install "git+https://github.com/JasonZQH/CURATOR.git@v0.1.2" +uvx --from "git+https://github.com/JasonZQH/CURATOR.git@v0.1.2" curator ``` -> Pin a released tag (`@v0.1.1`) for a reproducible install, or drop it to track `main`. +> Pin a released tag (`@v0.1.2`) for a reproducible install, or drop it to track `main`. > On Windows, run inside WSL2. **Open it** — from any project directory: @@ -106,8 +106,9 @@ Small requests start immediately; `/gate on` reviews the goal proposal first. | Version | Status | What it is | |---|---|---| -| **v0.1.1** | Current | Patch on Phase 0. A Codex tool call is counted once rather than once per lifecycle event, so the activity block no longer reads ~2× high. Docs numbering folded onto a single `v0.1.x` roadmap. | -| v0.1.0 | Previous | Phase 0 — local · single-writer · sequential. `Goal → writer → deterministic verifier → fresh-context reviewer → human confirm`, on a durable SQLite ledger. Opt-in `/resume stash` for the clean-tree guard. | +| **v0.1.2** | Current | Migration safety. Upgrading backs the ledger up before any schema migration touches it, migrations apply as one all-or-nothing transaction, and `curator doctor` shows what is pending plus how to restore. | +| v0.1.1 | Previous | Patch on Phase 0. A Codex tool call is counted once rather than once per lifecycle event, so the activity block no longer reads ~2× high. Docs numbering folded onto a single `v0.1.x` roadmap. | +| v0.1.0 | | Phase 0 — local · single-writer · sequential. `Goal → writer → deterministic verifier → fresh-context reviewer → human confirm`, on a durable SQLite ledger. Opt-in `/resume stash` for the clean-tree guard. | v0.1.0 highlights: full-screen first-run trust & setup, keyboard-selectable slash commands and proposal actions, PM/Engineer/Reviewer seat labels, persistent history with Tab completion and diff --git a/pyproject.toml b/pyproject.toml index cfc29f4..e0f28f1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -5,7 +5,7 @@ build-backend = "hatchling.build" [project] name = "curator" -version = "0.1.1" +version = "0.1.2" description = "Local-first coding-agent workbench with real provider orchestration." readme = { file = "README.md", content-type = "text/markdown" } requires-python = ">=3.11" diff --git a/src/curator/__init__.py b/src/curator/__init__.py index 4882ed1..025766d 100644 --- a/src/curator/__init__.py +++ b/src/curator/__init__.py @@ -1,3 +1,3 @@ """Expose package-level metadata for Curator Phase 0.""" -__version__ = "0.1.1" +__version__ = "0.1.2"