From a1e69bf392aae69f8dadd940d38fee069487dbf0 Mon Sep 17 00:00:00 2001 From: Sinity Date: Tue, 14 Jul 2026 23:07:57 +0200 Subject: [PATCH 1/3] fix(storage): skip backup manifest requirement for additive-only migrations Problem: `ops maintenance migrate-tier` required a verified backup manifest for every durable-tier migration, even ones that only add a new empty table (010_sinex_publication_obligations.sql, 011_excised_content.sql) and touch no existing column or row. That's unnecessary operator friction for migrations that carry zero data-loss risk. Solution: migration files can opt out via a leading `-- migration-safety: additive-no-backup` marker comment. migration_runner.py tracks `requires_backup` per loaded MigrationStep and only demands (and validates) a backup manifest when at least one step in the applied chain lacks the marker. `--backup-manifest` on `ops maintenance migrate-tier` is now optional, required only when the runner determines it's needed. Verification: devtools test tests/unit/storage/test_durable_migrations.py tests/unit/cli/test_archive_maintenance_cli.py -- 89 passed, 5 failed (all 5 pre-existing and unrelated, tracked in polylogue-p5li: assertion-export ordering, embedding-orphan reconciliation, and one trigger-already-exists failure, independently confirmed present on unmodified origin/master). mypy --strict clean, ruff clean. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GjyDHiwc5QfXGgDMGuFEB8 --- .../cli/commands/maintenance/_migrate_tier.py | 10 +-- polylogue/storage/sqlite/migration_runner.py | 71 +++++++++++-------- .../010_sinex_publication_obligations.sql | 1 + .../migrations/source/011_excised_content.sql | 1 + .../unit/cli/test_archive_maintenance_cli.py | 25 +++++++ tests/unit/storage/test_durable_migrations.py | 19 +++++ 6 files changed, 92 insertions(+), 35 deletions(-) diff --git a/polylogue/cli/commands/maintenance/_migrate_tier.py b/polylogue/cli/commands/maintenance/_migrate_tier.py index b7a27d08f1..fc1814fc9e 100644 --- a/polylogue/cli/commands/maintenance/_migrate_tier.py +++ b/polylogue/cli/commands/maintenance/_migrate_tier.py @@ -30,12 +30,12 @@ @click.argument("tier", type=click.Choice(tuple(sorted(tier.value for tier in DURABLE_MIGRATION_TIERS)))) @click.option( "--backup-manifest", - required=True, + required=False, type=click.Path(path_type=Path, exists=True), - help="Verified polylogue ops backup manifest or backup directory containing manifest.json.", + help="Verified backup manifest. Required only when a selected migration changes existing durable data.", ) @click.option("--output-format", type=click.Choice(["plain", "json"]), default="plain", show_default=True) -def migrate_tier_command(tier: str, backup_manifest: Path, output_format: str) -> None: +def migrate_tier_command(tier: str, backup_manifest: Path | None, output_format: str) -> None: """Apply additive migrations for one durable archive tier. Derived tiers are intentionally excluded from this command; rebuild or @@ -58,7 +58,7 @@ def migrate_tier_command(tier: str, backup_manifest: Path, output_format: str) - "ok": False, "tier": tier, "path": str(path), - "backup_manifest": str(backup_manifest), + "backup_manifest": str(backup_manifest) if backup_manifest is not None else None, "error": str(exc), }, indent=2, @@ -73,7 +73,7 @@ def migrate_tier_command(tier: str, backup_manifest: Path, output_format: str) - "ok": True, "tier": tier, "path": str(path), - "backup_manifest": str(backup_manifest), + "backup_manifest": str(backup_manifest) if backup_manifest is not None else None, "backup_receipt": str(result.backup_receipt) if result.backup_receipt is not None else None, "from_version": result.from_version, "to_version": result.to_version, diff --git a/polylogue/storage/sqlite/migration_runner.py b/polylogue/storage/sqlite/migration_runner.py index 6bc08c2c2f..9c434a8d23 100644 --- a/polylogue/storage/sqlite/migration_runner.py +++ b/polylogue/storage/sqlite/migration_runner.py @@ -24,6 +24,7 @@ _MIGRATION_NAME_RE = re.compile(r"^(?P\d{3,})_[a-z0-9_]+\.sql$") _VERIFICATION_RECEIPT_FILE = "verification-receipt.json" _SQLITE_SIDECAR_SUFFIXES = ("-wal", "-shm", "-journal") +_ADDITIVE_NO_BACKUP_MARKER = "-- migration-safety: additive-no-backup" class MigrationError(RuntimeError): @@ -36,6 +37,7 @@ class MigrationStep: version: int name: str sql: str + requires_backup: bool @dataclass(frozen=True, slots=True) @@ -63,12 +65,14 @@ def _load_migrations(tier: ArchiveTier) -> tuple[MigrationStep, ...]: match = _MIGRATION_NAME_RE.match(item.name) if match is None: continue + sql = item.read_text(encoding="utf-8") steps.append( MigrationStep( tier=tier, version=int(match.group("version")), name=item.name, - sql=item.read_text(encoding="utf-8"), + sql=sql, + requires_backup=_ADDITIVE_NO_BACKUP_MARKER not in sql, ) ) versions = [step.version for step in steps] @@ -556,42 +560,49 @@ def migrate_archive_tier( conn: sqlite3.Connection, tier: ArchiveTier, *, - backup_manifest: Path, + backup_manifest: Path | None, ) -> MigrationResult: """Apply additive migrations for one durable tier.""" if tier not in DURABLE_MIGRATION_TIERS: raise MigrationError(f"{tier.value} tier does not support in-place migrations") _checkpoint_live_tier(conn) - validate_migration_backup_manifest(backup_manifest, tier, connection=conn) - try: - conn.execute("BEGIN IMMEDIATE") - backup_receipt = validate_migration_backup_manifest(backup_manifest, tier, connection=conn) - current_version = int(conn.execute("PRAGMA user_version").fetchone()[0] or 0) - target_version = ARCHIVE_VERSION_BY_TIER[tier] - if current_version == target_version: - conn.rollback() - return MigrationResult( - tier=tier, - from_version=current_version, - to_version=target_version, - applied_versions=(), - backup_receipt=backup_receipt, - ) - if current_version == 0: - raise MigrationError(f"{tier.value} tier is empty; initialize it fresh instead of migrating") - if current_version > target_version: - raise MigrationError( - f"{tier.value} tier version {current_version} is newer than this runtime expects ({target_version})" - ) + current_version = int(conn.execute("PRAGMA user_version").fetchone()[0] or 0) + target_version = ARCHIVE_VERSION_BY_TIER[tier] + if current_version == target_version: + return MigrationResult( + tier=tier, + from_version=current_version, + to_version=target_version, + applied_versions=(), + ) + if current_version == 0: + raise MigrationError(f"{tier.value} tier is empty; initialize it fresh instead of migrating") + if current_version > target_version: + raise MigrationError( + f"{tier.value} tier version {current_version} is newer than this runtime expects ({target_version})" + ) - steps = tuple(step for step in _load_migrations(tier) if current_version < step.version <= target_version) - expected_versions = tuple(range(current_version + 1, target_version + 1)) - actual_versions = tuple(step.version for step in steps) - if actual_versions != expected_versions: - raise MigrationError( - f"{tier.value} migration chain is incomplete: expected {expected_versions}, found {actual_versions}" - ) + steps = tuple(step for step in _load_migrations(tier) if current_version < step.version <= target_version) + expected_versions = tuple(range(current_version + 1, target_version + 1)) + actual_versions = tuple(step.version for step in steps) + if actual_versions != expected_versions: + raise MigrationError( + f"{tier.value} migration chain is incomplete: expected {expected_versions}, found {actual_versions}" + ) + requires_backup = any(step.requires_backup for step in steps) + if requires_backup and backup_manifest is None: + raise MigrationError(f"{tier.value} migration requires a verified backup manifest") + if requires_backup: + assert backup_manifest is not None + validate_migration_backup_manifest(backup_manifest, tier, connection=conn) + try: + conn.execute("BEGIN IMMEDIATE") + backup_receipt = ( + validate_migration_backup_manifest(backup_manifest, tier, connection=conn) + if requires_backup and backup_manifest is not None + else None + ) start_version = current_version applied: list[int] = [] for step in steps: diff --git a/polylogue/storage/sqlite/migrations/source/010_sinex_publication_obligations.sql b/polylogue/storage/sqlite/migrations/source/010_sinex_publication_obligations.sql index ff4d3ec04e..a414f740cd 100644 --- a/polylogue/storage/sqlite/migrations/source/010_sinex_publication_obligations.sql +++ b/polylogue/storage/sqlite/migrations/source/010_sinex_publication_obligations.sql @@ -1,3 +1,4 @@ +-- migration-safety: additive-no-backup -- Durable Sinex-backed-mode publication obligation ledger (polylogue-303r.2). -- -- One row per (object_id, protocol_version, revision_id, manifest_digest) diff --git a/polylogue/storage/sqlite/migrations/source/011_excised_content.sql b/polylogue/storage/sqlite/migrations/source/011_excised_content.sql index c1fa295583..575b778dda 100644 --- a/polylogue/storage/sqlite/migrations/source/011_excised_content.sql +++ b/polylogue/storage/sqlite/migrations/source/011_excised_content.sql @@ -1,3 +1,4 @@ +-- migration-safety: additive-no-backup -- Durable removed-content ledger for standalone/off-mode excision -- (polylogue-27m). Purely additive: one new table, no existing column or -- constraint changes. diff --git a/tests/unit/cli/test_archive_maintenance_cli.py b/tests/unit/cli/test_archive_maintenance_cli.py index 901ba84c25..62727d56b5 100644 --- a/tests/unit/cli/test_archive_maintenance_cli.py +++ b/tests/unit/cli/test_archive_maintenance_cli.py @@ -24,6 +24,7 @@ ) from polylogue.storage.sqlite.archive_tiers.archive_plan import ArchiveInitAction, ArchiveInitPlan from polylogue.storage.sqlite.archive_tiers.bootstrap import initialize_archive_tier +from polylogue.storage.sqlite.archive_tiers.source import SOURCE_DDL from polylogue.storage.sqlite.archive_tiers.source_write import write_source_raw_session_blob_ref from polylogue.storage.sqlite.archive_tiers.types import ArchiveTier from polylogue.storage.sqlite.archive_tiers.user import USER_SCHEMA_VERSION @@ -1107,6 +1108,30 @@ def fake_init(plan: ArchiveInitPlan) -> ArchiveInitResult: ] +def test_migrate_tier_cli_allows_additive_source_ledgers_without_backup( + cli_workspace: dict[str, Path], cli_runner: CliRunner +) -> None: + source_db = cli_workspace["archive_root"] / "source.db" + with sqlite3.connect(source_db) as conn: + conn.executescript(SOURCE_DDL) + conn.execute("DROP TABLE excised_content") + conn.execute("DROP TABLE sinex_publication_obligations") + conn.execute("PRAGMA user_version = 9") + conn.commit() + + result = cli_runner.invoke( + cli, + ["--plain", "ops", "maintenance", "migrate-tier", "source", "--output-format", "json"], + catch_exceptions=False, + ) + + assert result.exit_code == 0 + payload = json.loads(result.output) + assert payload["backup_manifest"] is None + assert payload["backup_receipt"] is None + assert payload["applied_versions"] == [10, 11] + + def test_backup_verify_then_migrate_tier_cli_applies_user_migration_with_receipt( cli_workspace: dict[str, Path], cli_runner: CliRunner, diff --git a/tests/unit/storage/test_durable_migrations.py b/tests/unit/storage/test_durable_migrations.py index 1702cbbafa..49e21a93ad 100644 --- a/tests/unit/storage/test_durable_migrations.py +++ b/tests/unit/storage/test_durable_migrations.py @@ -547,6 +547,25 @@ def test_source_tier_v1_migrates_to_current_without_native_uniqueness( conn.close() +def test_source_additive_ledger_migrations_do_not_require_a_backup(tmp_path: Path) -> None: + db_path = tmp_path / "source.db" + with sqlite3.connect(db_path) as conn: + conn.executescript(SOURCE_DDL) + conn.execute("DROP TABLE excised_content") + conn.execute("DROP TABLE sinex_publication_obligations") + conn.execute("PRAGMA user_version = 9") + conn.commit() + + result = migrate_archive_tier(conn, ArchiveTier.SOURCE, backup_manifest=None) + + assert result.from_version == 9 + assert result.to_version == SOURCE_SCHEMA_VERSION == 11 + assert result.applied_versions == (10, 11) + assert result.backup_receipt is None + tables = {row[0] for row in conn.execute("SELECT name FROM sqlite_master WHERE type = 'table'")} + assert {"sinex_publication_obligations", "excised_content"} <= tables + + def test_source_tier_v7_expands_origin_checks_with_verified_backup( workspace_env: dict[str, Path], tmp_path: Path, From a8a4d4d78b26f0ab637b156c391fb264179f7f4f Mon Sep 17 00:00:00 2001 From: Sinity Date: Tue, 14 Jul 2026 23:11:58 +0200 Subject: [PATCH 2/3] fix(archive): correct broken polylogue.types import in display mixin DisplayTitleTagsMixin imported SessionId from polylogue.types, a module that doesn't exist (the real path is polylogue.core.types). Because the module uses `from __future__ import annotations`, the broken import never raised at runtime -- annotations are lazily stored as strings -- but mypy couldn't resolve the name, silently typed `self.id` as Any, and flagged `display_title`'s `self.id[:8]` return as "Returning Any from function declared to return str". This was blocking the pre-push devtools verify --quick gate for unrelated pushes. Verification: mypy --strict polylogue/archive/session/display_mixin.py clean; devtools test tests/unit/core/test_session_semantics.py tests/unit/sources/test_null_guard_properties.py -- 56 passed. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GjyDHiwc5QfXGgDMGuFEB8 --- polylogue/archive/session/display_mixin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polylogue/archive/session/display_mixin.py b/polylogue/archive/session/display_mixin.py index 4f7a944a48..d928e4989d 100644 --- a/polylogue/archive/session/display_mixin.py +++ b/polylogue/archive/session/display_mixin.py @@ -9,7 +9,7 @@ from typing import TYPE_CHECKING from polylogue.archive.session.branch_type import BranchType -from polylogue.types import SessionId +from polylogue.core.types import SessionId if TYPE_CHECKING: pass From a5f778aa7554832d290651c7bf73a208fa2f55a6 Mon Sep 17 00:00:00 2001 From: Sinity Date: Wed, 15 Jul 2026 00:00:26 +0200 Subject: [PATCH 3/3] fix(storage): address CodeRabbit findings on migration_runner refactor Two real issues from PR #2905's automated review: 1. Version/step selection was read once, before BEGIN IMMEDIATE, so a concurrent migrate_archive_tier call that advanced the tier between the read and the lock produced a confusing "expected version N, found M" instead of the correct no-op result. Extracted _pending_migration_steps() and call it both in the lock-free precheck (fast-fail on a missing backup manifest without wasting a write-lock acquisition) and again, authoritatively, right after BEGIN IMMEDIATE acquires the lock. 2. The additive-no-backup marker used substring matching (`_ADDITIVE_NO_BACKUP_MARKER not in sql`), which would waive the backup requirement if the marker text ever appeared in a comment, string literal, or later in the file. Added _requires_migration_backup(), requiring the marker to be the file's first non-blank line. Fixing (1) surfaced a real regression in my own earlier refactor: the original code called validate_migration_backup_manifest() twice -- once before BEGIN IMMEDIATE (establishing a WAL-empty baseline) and once after (re-validating with the same connection, so a nonempty WAL from an intervening write raises "changed before the migration lock" instead of migrating over data the verified backup never covered). My precheck/lock split had collapsed this to a single post-lock call, silently dropping that race protection -- test_migration_rejects_live_writes_committed_after_receipt_validation caught it immediately (turned into "database is locked" instead of the expected MigrationError, since the test's simulated concurrent writer now raced the migration's own transaction instead of the pre-lock read). Restored the paired pre-lock validation call. Verification: devtools test tests/unit/storage/test_durable_migrations.py tests/unit/cli/test_archive_maintenance_cli.py -- 90 passed, 5 failed (all 5 pre-existing and unrelated, tracked in polylogue-p5li, independently confirmed present on unmodified origin/master). mypy --strict clean across polylogue/ (992 files). ruff clean. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GjyDHiwc5QfXGgDMGuFEB8 --- polylogue/storage/sqlite/migration_runner.py | 93 +++++++++++++++---- tests/unit/storage/test_durable_migrations.py | 20 ++++ 2 files changed, 95 insertions(+), 18 deletions(-) diff --git a/polylogue/storage/sqlite/migration_runner.py b/polylogue/storage/sqlite/migration_runner.py index 9c434a8d23..083bdacaeb 100644 --- a/polylogue/storage/sqlite/migration_runner.py +++ b/polylogue/storage/sqlite/migration_runner.py @@ -53,6 +53,17 @@ def _migration_package(tier: ArchiveTier) -> str: return f"polylogue.storage.sqlite.migrations.{tier.value}" +def _requires_migration_backup(sql: str) -> bool: + """A migration opts out of the backup requirement only via a header directive. + + Substring matching would waive the backup requirement if the marker text + ever appeared in a comment, SQL string literal, or later in the file -- + require it to be the file's first non-blank line instead. + """ + first_nonblank = next((line.strip() for line in sql.splitlines() if line.strip()), "") + return first_nonblank != _ADDITIVE_NO_BACKUP_MARKER + + def _load_migrations(tier: ArchiveTier) -> tuple[MigrationStep, ...]: if tier not in DURABLE_MIGRATION_TIERS: return () @@ -72,7 +83,7 @@ def _load_migrations(tier: ArchiveTier) -> tuple[MigrationStep, ...]: version=int(match.group("version")), name=item.name, sql=sql, - requires_backup=_ADDITIVE_NO_BACKUP_MARKER not in sql, + requires_backup=_requires_migration_backup(sql), ) ) versions = [step.version for step in steps] @@ -556,6 +567,23 @@ def _execute_migration_sql(conn: sqlite3.Connection, sql: str) -> None: raise MigrationError("migration SQL ended with an incomplete statement") +def _pending_migration_steps( + conn: sqlite3.Connection, + tier: ArchiveTier, + *, + current_version: int, + target_version: int, +) -> tuple[MigrationStep, ...]: + steps = tuple(step for step in _load_migrations(tier) if current_version < step.version <= target_version) + expected_versions = tuple(range(current_version + 1, target_version + 1)) + actual_versions = tuple(step.version for step in steps) + if actual_versions != expected_versions: + raise MigrationError( + f"{tier.value} migration chain is incomplete: expected {expected_versions}, found {actual_versions}" + ) + return steps + + def migrate_archive_tier( conn: sqlite3.Connection, tier: ArchiveTier, @@ -566,38 +594,67 @@ def migrate_archive_tier( if tier not in DURABLE_MIGRATION_TIERS: raise MigrationError(f"{tier.value} tier does not support in-place migrations") _checkpoint_live_tier(conn) - current_version = int(conn.execute("PRAGMA user_version").fetchone()[0] or 0) target_version = ARCHIVE_VERSION_BY_TIER[tier] - if current_version == target_version: + + # Lock-free precheck: fail fast (no wasted write-lock acquisition) when a + # backup manifest is required but missing. This read is intentionally not + # authoritative -- a concurrent migrate_archive_tier call could change the + # version before this one acquires BEGIN IMMEDIATE below, so every value + # computed here is re-derived from a fresh read once the lock is held. + precheck_version = int(conn.execute("PRAGMA user_version").fetchone()[0] or 0) + if precheck_version == target_version: return MigrationResult( tier=tier, - from_version=current_version, + from_version=precheck_version, to_version=target_version, applied_versions=(), ) - if current_version == 0: + if precheck_version == 0: raise MigrationError(f"{tier.value} tier is empty; initialize it fresh instead of migrating") - if current_version > target_version: + if precheck_version > target_version: raise MigrationError( - f"{tier.value} tier version {current_version} is newer than this runtime expects ({target_version})" + f"{tier.value} tier version {precheck_version} is newer than this runtime expects ({target_version})" ) - - steps = tuple(step for step in _load_migrations(tier) if current_version < step.version <= target_version) - expected_versions = tuple(range(current_version + 1, target_version + 1)) - actual_versions = tuple(step.version for step in steps) - if actual_versions != expected_versions: - raise MigrationError( - f"{tier.value} migration chain is incomplete: expected {expected_versions}, found {actual_versions}" - ) - requires_backup = any(step.requires_backup for step in steps) - if requires_backup and backup_manifest is None: + precheck_steps = _pending_migration_steps( + conn, tier, current_version=precheck_version, target_version=target_version + ) + precheck_requires_backup = any(step.requires_backup for step in precheck_steps) + if precheck_requires_backup and backup_manifest is None: raise MigrationError(f"{tier.value} migration requires a verified backup manifest") - if requires_backup: + if precheck_requires_backup: + # Baseline validation before acquiring the write lock. The paired + # post-lock call below re-validates with the same connection; + # _validate_live_source_fingerprint rejects a nonempty WAL, so a + # write that lands on the live tier between this call and BEGIN + # IMMEDIATE is caught as "changed before the migration lock" instead + # of migrating over data the verified backup never covered. assert backup_manifest is not None validate_migration_backup_manifest(backup_manifest, tier, connection=conn) try: conn.execute("BEGIN IMMEDIATE") + # Authoritative re-read: a concurrent migration may have advanced (or + # completed) the tier between the precheck above and this lock + # acquisition. Recomputing here instead of trusting the precheck + # avoids failing the per-step version check below with a confusing + # "expected version N, found M" instead of the correct no-op result. + current_version = int(conn.execute("PRAGMA user_version").fetchone()[0] or 0) + if current_version == target_version: + conn.rollback() + return MigrationResult( + tier=tier, + from_version=current_version, + to_version=target_version, + applied_versions=(), + ) + if current_version > target_version: + raise MigrationError( + f"{tier.value} tier version {current_version} is newer than this runtime expects ({target_version})" + ) + steps = _pending_migration_steps(conn, tier, current_version=current_version, target_version=target_version) + requires_backup = any(step.requires_backup for step in steps) + if requires_backup and backup_manifest is None: + raise MigrationError(f"{tier.value} migration requires a verified backup manifest") backup_receipt = ( validate_migration_backup_manifest(backup_manifest, tier, connection=conn) if requires_backup and backup_manifest is not None diff --git a/tests/unit/storage/test_durable_migrations.py b/tests/unit/storage/test_durable_migrations.py index 49e21a93ad..70a93abf90 100644 --- a/tests/unit/storage/test_durable_migrations.py +++ b/tests/unit/storage/test_durable_migrations.py @@ -566,6 +566,26 @@ def test_source_additive_ledger_migrations_do_not_require_a_backup(tmp_path: Pat assert {"sinex_publication_obligations", "excised_content"} <= tables +def test_additive_no_backup_marker_must_be_the_header_not_a_substring() -> None: + """CodeRabbit #2905: substring matching would waive the backup requirement + if the marker text ever appeared in a comment, string literal, or later in + the file. It must be the file's first non-blank line.""" + header = migration_runner._ADDITIVE_NO_BACKUP_MARKER + + assert ( + migration_runner._requires_migration_backup(f"{header}\n-- a real migration\nCREATE TABLE t (x INTEGER);") + is False + ) + assert migration_runner._requires_migration_backup(f"{header} trailing text\nCREATE TABLE t (x INTEGER);") is True + assert ( + migration_runner._requires_migration_backup(f"-- unrelated comment\n{header}\nCREATE TABLE t (x INTEGER);") + is True + ) + assert migration_runner._requires_migration_backup(f"CREATE TABLE t (x TEXT DEFAULT '{header}');") is True + assert migration_runner._requires_migration_backup("CREATE TABLE t (x INTEGER);") is True + assert migration_runner._requires_migration_backup(f"\n\n {header} \nCREATE TABLE t (x INTEGER);") is False + + def test_source_tier_v7_expands_origin_checks_with_verified_backup( workspace_env: dict[str, Path], tmp_path: Path,