diff --git a/polylogue/maintenance/agent_meta_sidecar_purge_apply.py b/polylogue/maintenance/agent_meta_sidecar_purge_apply.py index 4d784edbd2..adc5dfb193 100644 --- a/polylogue/maintenance/agent_meta_sidecar_purge_apply.py +++ b/polylogue/maintenance/agent_meta_sidecar_purge_apply.py @@ -54,7 +54,7 @@ ) from polylogue.storage.sqlite.archive_tiers.archive import ArchiveStore from polylogue.storage.sqlite.archive_tiers.types import ArchiveTier -from polylogue.storage.sqlite.migration_runner import validate_migration_backup_manifest +from polylogue.storage.sqlite.migration_runner import validate_backup_manifest_covers_derived_tier TOOL_VERSION = "agent-meta-sidecar-purge-apply-v1" @@ -198,7 +198,7 @@ def apply_agent_meta_sidecar_purge( validate_conn = sqlite3.connect(str(index_db), uri=True) try: _checkpoint_live_tier(validate_conn) - validate_migration_backup_manifest(backup_manifest, ArchiveTier.INDEX, connection=validate_conn) + validate_backup_manifest_covers_derived_tier(backup_manifest, ArchiveTier.INDEX, connection=validate_conn) plan = scan_agent_meta_sidecar_sessions(validate_conn, source_db, limit=limit) finally: validate_conn.close() @@ -216,6 +216,21 @@ def apply_agent_meta_sidecar_purge( if session_ids: store = ArchiveStore(archive_root, read_only=False) try: + # Authoritative re-validation now that the writer lease is held + # (ArchiveStore.__init__ acquires it synchronously above) -- + # matches attachment_reacquisition.py's / migrate_archive_tier's + # pattern: a concurrent write between the first, lock-free + # precheck and this lease acquisition would make the backup + # stale, and the lease guarantees nothing else can write between + # this check and delete_sessions below. + revalidate_conn = sqlite3.connect(str(index_db), uri=True) + try: + _checkpoint_live_tier(revalidate_conn) + validate_backup_manifest_covers_derived_tier( + backup_manifest, ArchiveTier.INDEX, connection=revalidate_conn + ) + finally: + revalidate_conn.close() store.delete_sessions(session_ids) finally: store.close() diff --git a/polylogue/storage/attachment_reacquisition.py b/polylogue/storage/attachment_reacquisition.py index 3f5eed1072..e33e36dc48 100644 --- a/polylogue/storage/attachment_reacquisition.py +++ b/polylogue/storage/attachment_reacquisition.py @@ -79,7 +79,7 @@ from polylogue.storage.runtime.raw.records import RawSessionRecord from polylogue.storage.sqlite.archive_tiers.types import ArchiveTier from polylogue.storage.sqlite.archive_tiers.write import _attachment_id -from polylogue.storage.sqlite.migration_runner import validate_migration_backup_manifest +from polylogue.storage.sqlite.migration_runner import validate_backup_manifest_covers_derived_tier from polylogue.storage.sqlite.queries.mappers_archive import _row_to_raw_session logger = get_logger(__name__) @@ -424,12 +424,13 @@ def apply_attachment_reacquisition( ``dry_run=False`` requires both ``manifest_path`` (an immutable, append-only JSONL receipt of every row acted on) and ``backup_manifest`` - (a verified backup manifest for the ``index`` tier, the same gate durable- - tier migrations use via - :func:`polylogue.storage.sqlite.migration_runner.validate_migration_backup_manifest` - -- index.db is rebuildable, but this mutation is still gated behind a - verified backup so an operator-authorized ``--apply`` can never be the - first time backup coverage is checked). + (a verified backup manifest for the ``index`` tier, checked via + :func:`polylogue.storage.sqlite.migration_runner.validate_backup_manifest_covers_derived_tier` + -- index.db is rebuildable and was never wired for the cryptographic + attestation durable-tier migrations require, so this checks manifest + coverage and a byte-exact live fingerprint instead; this mutation is + still gated behind a verified backup so an operator-authorized + ``--apply`` can never be the first time backup coverage is checked). Only two actions are ever taken: promoting a ``reacquirable`` attachment's ``blob_hash``/``byte_count``/``acquisition_status`` to ``'acquired'`` @@ -474,7 +475,7 @@ def apply_attachment_reacquisition( _checkpoint_index_tier(index_conn) # Lock-free precheck: reject a missing/stale/wrong-tier manifest # before paying for classification + a write-lock acquisition. - validate_migration_backup_manifest(backup_manifest, ArchiveTier.INDEX, connection=index_conn) + validate_backup_manifest_covers_derived_tier(backup_manifest, ArchiveTier.INDEX, connection=index_conn) with closing(sqlite3.connect(f"file:{source_db}?mode=ro", uri=True)) as source_conn_ro: plan = plan_attachment_reacquisition( @@ -553,7 +554,7 @@ def apply_attachment_reacquisition( # migrate_archive_tier's / raw_live_source_reconciliation_apply's # pattern: a concurrent write between the precheck and this lock # acquisition would make the backup stale. - validate_migration_backup_manifest(backup_manifest, ArchiveTier.INDEX, connection=index_conn) + validate_backup_manifest_covers_derived_tier(backup_manifest, ArchiveTier.INDEX, connection=index_conn) reacquired_count = 0 reacquired_bytes = 0 diff --git a/polylogue/storage/sqlite/migration_runner.py b/polylogue/storage/sqlite/migration_runner.py index 083bdacaeb..035d8a9773 100644 --- a/polylogue/storage/sqlite/migration_runner.py +++ b/polylogue/storage/sqlite/migration_runner.py @@ -498,10 +498,23 @@ def _validate_blob_inventory( raise MigrationError(f"migration backup blob hash mismatch: {blob['path']}") -def validate_migration_backup_manifest( - path: Path, tier: ArchiveTier, *, connection: sqlite3.Connection | None = None +def _validate_backup_manifest_covers_tier( + path: Path, tier: ArchiveTier, *, connection: sqlite3.Connection, require_attestation: bool ) -> Path: - """Validate that ``path`` has a successful backup verification receipt.""" + """Validate that ``path`` has a successful backup verification receipt. + + ``require_attestation`` gates the cryptographic HMAC attestation check. + Attestations are only ever minted for durable tiers (source, user) by + ``daemon/backup.py``'s ``_write_successful_verification_receipt`` -- a + derived tier (index, embeddings) can never carry one, by design, so + requiring it for those tiers would make backup-manifest validation + permanently unsatisfiable rather than merely strict. Callers protecting a + mutation against a derived tier (e.g. the agent-meta-sidecar purge, which + deletes rows from index.db) still get every other guarantee here -- + manifest/receipt shape, tier inclusion, and a byte-exact live fingerprint + recomputed from the current on-disk file -- just not the attestation, + which durable-tier migrations (``migrate_archive_tier``) still require. + """ manifest_path = _backup_manifest_path(path) if not manifest_path.exists() and not manifest_path.is_symlink(): raise MigrationError(f"migration requires an existing backup manifest; missing {manifest_path}") @@ -521,13 +534,12 @@ def validate_migration_backup_manifest( receipt = _load_json(receipt_path, label="verification receipt") if receipt.get("format") != VERIFICATION_RECEIPT_FORMAT: raise MigrationError(f"migration backup receipt has unsupported format: {receipt_path}") - if connection is None: - raise MigrationError("migration backup receipt authentication requires the live tier connection") live_tier_path = _connection_main_path(connection).resolve(strict=False) - try: - verify_verification_receipt(receipt, tier=tier.value, live_tier_path=live_tier_path) - except BackupAttestationError as exc: - raise MigrationError(f"migration backup receipt authentication failed: {exc}") from exc + if require_attestation: + try: + verify_verification_receipt(receipt, tier=tier.value, live_tier_path=live_tier_path) + except BackupAttestationError as exc: + raise MigrationError(f"migration backup receipt authentication failed: {exc}") from exc if receipt.get("verdict") != "success": raise MigrationError(f"migration backup receipt is not a successful verification: {receipt_path}") artifact_inventory = _cached_backup_artifact_inventory(backup_root) @@ -555,6 +567,30 @@ def validate_migration_backup_manifest( return receipt_path +def validate_migration_backup_manifest( + path: Path, tier: ArchiveTier, *, connection: sqlite3.Connection | None = None +) -> Path: + """Validate a backup manifest for a durable-tier migration (requires attestation).""" + if connection is None: + raise MigrationError("migration backup receipt authentication requires the live tier connection") + return _validate_backup_manifest_covers_tier(path, tier, connection=connection, require_attestation=True) + + +def validate_backup_manifest_covers_derived_tier( + path: Path, tier: ArchiveTier, *, connection: sqlite3.Connection +) -> Path: + """Validate a backup manifest covers a derived tier (index, embeddings) at its live fingerprint. + + For use by actuators that mutate a derived tier and want backup coverage + as a safety net before doing so, without requiring the cryptographic + attestation that only durable tiers (source, user) ever carry -- see + ``_validate_backup_manifest_covers_tier``'s docstring. + """ + if tier in DURABLE_MIGRATION_TIERS: + raise MigrationError(f"{tier.value} is a durable tier; use validate_migration_backup_manifest instead") + return _validate_backup_manifest_covers_tier(path, tier, connection=connection, require_attestation=False) + + def _execute_migration_sql(conn: sqlite3.Connection, sql: str) -> None: statement = "" for line in sql.splitlines(keepends=True): @@ -701,5 +737,6 @@ def migrate_archive_tier( "MigrationResult", "MigrationStep", "migrate_archive_tier", + "validate_backup_manifest_covers_derived_tier", "validate_migration_backup_manifest", ] diff --git a/tests/unit/maintenance/test_agent_meta_sidecar_purge_apply.py b/tests/unit/maintenance/test_agent_meta_sidecar_purge_apply.py index b2c2a07109..d38fb9ed4f 100644 --- a/tests/unit/maintenance/test_agent_meta_sidecar_purge_apply.py +++ b/tests/unit/maintenance/test_agent_meta_sidecar_purge_apply.py @@ -220,7 +220,7 @@ def _fake_validate(manifest: Path, tier: object, *, connection: sqlite3.Connecti return manifest.with_name("verification-receipt.json") monkeypatch.setattr( - "polylogue.maintenance.agent_meta_sidecar_purge_apply.validate_migration_backup_manifest", + "polylogue.maintenance.agent_meta_sidecar_purge_apply.validate_backup_manifest_covers_derived_tier", _fake_validate, ) @@ -236,7 +236,11 @@ def _fake_validate(manifest: Path, tier: object, *, connection: sqlite3.Connecti "claude-code-session:agent-bbb222.meta", } assert report.backup_manifest == manifest - assert validated == [(manifest, ArchiveTier.INDEX)] + # Validated twice: a lock-free precheck before classification, then an + # authoritative revalidation after the write lease is held (polylogue- + # 5kmn7 CodeRabbit follow-up) -- closes the TOCTOU window between the + # precheck and the actual delete. + assert validated == [(manifest, ArchiveTier.INDEX), (manifest, ArchiveTier.INDEX)] remaining = _session_ids(archive_root) assert remaining == { @@ -275,7 +279,7 @@ def _reject(manifest: Path, tier: object, *, connection: sqlite3.Connection) -> raise ValueError("backup manifest does not match live index.db") monkeypatch.setattr( - "polylogue.maintenance.agent_meta_sidecar_purge_apply.validate_migration_backup_manifest", + "polylogue.maintenance.agent_meta_sidecar_purge_apply.validate_backup_manifest_covers_derived_tier", _reject, ) @@ -304,7 +308,7 @@ def _fake_validate(manifest: Path, tier: object, *, connection: sqlite3.Connecti return manifest.with_name("verification-receipt.json") monkeypatch.setattr( - "polylogue.maintenance.agent_meta_sidecar_purge_apply.validate_migration_backup_manifest", + "polylogue.maintenance.agent_meta_sidecar_purge_apply.validate_backup_manifest_covers_derived_tier", _fake_validate, ) @@ -316,3 +320,89 @@ def _fake_validate(manifest: Path, tier: object, *, connection: sqlite3.Connecti assert _session_ids(archive_root) == before_sessions assert _receipt_rows(archive_root) == {} + + +def test_apply_accepts_a_real_backup_manifest_from_ops_backup(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """polylogue-5kmn7 anti-vacuity regression. + + Every other apply test in this file monkeypatches the backup-manifest + validator entirely, so none of them ever exercised a manifest actually + produced by ``polylogue ops backup --profile full_evidence --verify``. + That gap hid a real bug: the old ``validate_migration_backup_manifest`` + call unconditionally required a cryptographic HMAC attestation for the + index tier, but attestations are only ever minted for durable tiers + (source, user) by ``daemon/backup.py`` -- no real backup, however fresh + or complete, could ever satisfy it. Discovered running the live purge + against a same-day, all-tiers-present, verdict=success manifest. + + This test builds the fixture archive, runs the real ``backup_archive()`` + entry point (the same code ``polylogue ops backup`` invokes) against it, + and lets ``apply_agent_meta_sidecar_purge`` validate that real manifest + with no monkeypatching -- proving + ``validate_backup_manifest_covers_derived_tier`` actually accepts what + the backup system actually produces. + """ + archive_root = _build_fixture_archive(tmp_path) + monkeypatch.setenv("POLYLOGUE_ARCHIVE_ROOT", str(archive_root)) + + from polylogue.daemon.backup import backup_archive + + backup_result = backup_archive(output_dir=tmp_path / "backup", profile="full_evidence", verify=True) + assert backup_result.ok, backup_result.error + assert backup_result.verified, backup_result.verification + assert backup_result.output_path is not None + manifest = Path(backup_result.output_path) / "manifest.json" + assert manifest.exists() + + report = apply_agent_meta_sidecar_purge(archive_root, backup_manifest=manifest, dry_run=False) + + assert report.applied + assert report.purged_count == 2 + assert report.shape_mismatch_count == 0 + assert _session_ids(archive_root) == { + f"{Origin.CLAUDE_CODE_SESSION.value}:agent-aaa111", + f"{Origin.CLAUDE_CODE_SESSION.value}:agent-bbb222", + f"{Origin.CLAUDE_CODE_SESSION.value}:s2-uuid", + } + assert set(_receipt_rows(archive_root)) == set(report.purged_session_ids) + + +def test_apply_refuses_when_backup_goes_stale_between_precheck_and_write_lease( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Concurrency regression (polylogue-5kmn7 CodeRabbit follow-up). + + A concurrent write to index.db between the lock-free precheck and the + write-lease acquisition must be caught by the authoritative + revalidation, not silently ignored -- proving the second call is + actually wired in and enforced, not merely present as dead code. The + fake validator simulates what a real live-fingerprint mismatch would + raise (validate_backup_manifest_covers_derived_tier itself already has + focused coverage for the real mismatch path via + _validate_live_source_fingerprint). + """ + archive_root = _build_fixture_archive(tmp_path) + before_sessions = _session_ids(archive_root) + + calls = 0 + + def _fake_validate(manifest: Path, tier: object, *, connection: sqlite3.Connection) -> Path: + nonlocal calls + calls += 1 + if calls == 2: + raise ValueError("migration backup receipt live tier hash mismatch") + return manifest.with_name("verification-receipt.json") + + monkeypatch.setattr( + "polylogue.maintenance.agent_meta_sidecar_purge_apply.validate_backup_manifest_covers_derived_tier", + _fake_validate, + ) + + manifest = tmp_path / "verified-backup" / "manifest.json" + + with pytest.raises(ValueError, match="hash mismatch"): + apply_agent_meta_sidecar_purge(archive_root, backup_manifest=manifest, dry_run=False) + + assert calls == 2 + assert _session_ids(archive_root) == before_sessions + assert _receipt_rows(archive_root) == {} diff --git a/tests/unit/storage/test_attachment_reacquisition.py b/tests/unit/storage/test_attachment_reacquisition.py index 644832bbf0..09384f962c 100644 --- a/tests/unit/storage/test_attachment_reacquisition.py +++ b/tests/unit/storage/test_attachment_reacquisition.py @@ -163,7 +163,7 @@ def _fake_validate(manifest: Path, tier: object, *, connection: sqlite3.Connecti return manifest.with_name("verification-receipt.json") monkeypatch.setattr( - "polylogue.storage.attachment_reacquisition.validate_migration_backup_manifest", + "polylogue.storage.attachment_reacquisition.validate_backup_manifest_covers_derived_tier", _fake_validate, ) return validated @@ -350,7 +350,9 @@ def test_apply_refuses_when_backup_manifest_invalid(tmp_path: Path, monkeypatch: def _reject(manifest: Path, tier: object, *, connection: sqlite3.Connection) -> Path: raise ValueError("backup manifest does not match live index.db") - monkeypatch.setattr("polylogue.storage.attachment_reacquisition.validate_migration_backup_manifest", _reject) + monkeypatch.setattr( + "polylogue.storage.attachment_reacquisition.validate_backup_manifest_covers_derived_tier", _reject + ) with pytest.raises(ValueError, match="does not match"): apply_attachment_reacquisition(