fix(storage): skip backup manifest requirement for additive-only migrations#2905
Conversation
…ations 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GjyDHiwc5QfXGgDMGuFEB8
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GjyDHiwc5QfXGgDMGuFEB8
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMigration steps now identify whether backups are required, allowing marked additive source migrations to run without backup manifests. The CLI accepts optional manifests and emits null when omitted. A session display mixin also updates its ChangesMigration safety handling
Session type import correction
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant MigrateTierCLI
participant MigrationRunner
participant SourceSQLite
Operator->>MigrateTierCLI: migrate-tier source
MigrateTierCLI->>MigrationRunner: migrate_archive_tier(backup_manifest=None)
MigrationRunner->>SourceSQLite: inspect version and migration chain
MigrationRunner->>MigrationRunner: detect additive-no-backup steps
MigrationRunner->>SourceSQLite: apply versions 10 and 11
MigrationRunner-->>MigrateTierCLI: MigrationResult with null backup receipt
MigrateTierCLI-->>Operator: JSON success payload
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: a8a4d4d78b
ℹ️ 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".
| 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) |
There was a problem hiding this comment.
Because current_version is captured before BEGIN IMMEDIATE, two concurrent migrate-tier source invocations starting from v9 can both select migrations 10/11; after the first commits, the second acquires the lock and then fails the per-step check with expected version 9, found 11 instead of returning the no-op result the old in-transaction version read would have produced. Please recompute the version and selected steps after taking the write lock.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: version + pending-step selection is now re-read after BEGIN IMMEDIATE acquires the lock (_pending_migration_steps extracted and called both in the lock-free precheck and again inside the transaction), so a concurrent migration that already advanced the tier is handled as a clean no-op instead of a confusing per-step version mismatch.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@polylogue/storage/sqlite/migration_runner.py`:
- Line 75: Update the requires_backup calculation in the migration runner to
recognize _ADDITIVE_NO_BACKUP_MARKER only when it is an exact header directive,
not when it appears in explanatory text, SQL literals, later SQL content, or
with trailing text. Preserve backup requirements for all non-exact matches, and
add tests covering later occurrences and markers followed by trailing text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8ce30da8-7bb2-4546-9b0e-d6bbff25cf88
📒 Files selected for processing (7)
polylogue/archive/session/display_mixin.pypolylogue/cli/commands/maintenance/_migrate_tier.pypolylogue/storage/sqlite/migration_runner.pypolylogue/storage/sqlite/migrations/source/010_sinex_publication_obligations.sqlpolylogue/storage/sqlite/migrations/source/011_excised_content.sqltests/unit/cli/test_archive_maintenance_cli.pytests/unit/storage/test_durable_migrations.py
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GjyDHiwc5QfXGgDMGuFEB8
1 participant
Summary
ops maintenance migrate-tierno longer forces a verified backup manifest when a durable-tier migration chain is entirely additive (new tables only, no existing data touched)-- migration-safety: additive-no-backupmarker comment;migration_runner.pyrequires (and validates) a backup manifest only when at least one applied step lacks the markerpolylogue.typesdoesn't exist; correct path ispolylogue.core.types) indisplay_mixin.pythat was silently typed asAnyby mypy and blocking the pre-push verify gate for any pushProblem
Every durable-tier migration required an operator to produce and verify a backup manifest before running, even for migrations that only add a new empty table with zero data-loss risk. That's unnecessary friction — evidenced by the two most recent source-tier migrations (010_sinex_publication_obligations.sql, 011_excised_content.sql), which are both purely additive.
Solution
MigrationStepnow carries arequires_backup: bool, computed from whether its SQL file contains the-- migration-safety: additive-no-backupmarker.migrate_archive_tieronly demands/validates a backup manifest when the applied migration chain contains at least one step without that marker.--backup-manifeston the CLI is now optional (required=False), required only when the runner determines it's actually 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, independently confirmed present on unmodifiedorigin/master, tracked inpolylogue-p5li: assertion-export ordering ×2, embedding-orphan reconciliation ×2, one trigger-already-exists collision)mypy --strictclean (including thedisplay_mixin.pyimport fix)ruff check/formatcleandevtools verify --quick(pre-push gate): exit 0, all 16 steps greenCo-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes