fix(repair): honor --dry-run for repair --mode from-sqlite (#2133) - #2138
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1ff13856a
ℹ️ 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".
| # helper repair uses elsewhere — so the preview matches the per-collection | ||
| # counts a real rebuild upserts. Reads the original ``source_palace`` | ||
| # (not yet archived). Must never take the mine-lock or rename anything. | ||
| if dry_run: |
There was a problem hiding this comment.
Handle in-place previews before requiring the archive flag
When a user runs the advertised mempalace repair --mode from-sqlite --dry-run with the normal defaults, --source resolves to --palace while --archive-existing remains false. The earlier in-place validation therefore returns {} before this branch is reached, and the CLI exits with status 1 instead of showing a preview. Allow dry runs to bypass the archive requirement while still validating that the source database exists.
Useful? React with 👍 / 👎.
| source_palace=source_path, | ||
| dest_palace=palace_path, | ||
| archive_existing_dest=archive_existing, | ||
| dry_run=dry_run, |
There was a problem hiding this comment.
Update the alias test for the new keyword argument
This now passes dry_run=False even when the parsed namespace has no dry_run attribute, but the existing test_cmd_repair_rebuild_index_alias_uses_sqlite_archive uses assert_called_once_with and expects only the previous three arguments. Consequently the test suite deterministically fails on the repair rebuild-index alias test; update that assertion to include the new keyword.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🟢 Ready to approve
The dry-run behavior is correctly short-circuited before locking/archiving/writing, and the new tests cover the reported destructive regression scenarios.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Fixes a safety regression in the repair workflow by ensuring mempalace repair --mode from-sqlite --dry-run is truly read-only, providing a preview of per-collection would-be rebuild counts without performing any archival, locking, or re-embedding work.
Changes:
- Add
dry_runsupport torepair.rebuild_from_sqlite()with an early, read-only preview path that reports SQLite-ground-truth row counts and exits. - Update CLI
repair --mode from-sqlitedispatch to forward--dry-runand skip the destructive confirmation prompt when previewing. - Add focused test coverage for cross-palace and in-place dry-run behavior and for “fail closed” handling when SQLite counts are unreadable; document the behavior in the changelog.
File summaries
| File | Description |
|---|---|
mempalace/repair.py |
Introduces dry_run preview logic for from-SQLite rebuilds and ensures no irreversible steps occur during preview. |
mempalace/cli.py |
Forwards --dry-run to the from-SQLite rebuild path and avoids destructive confirmation prompts for previews. |
tests/test_repair.py |
Adds regression tests ensuring dry-run performs no writes/archives and fails closed on unreadable counts. |
tests/test_cli.py |
Ensures CLI forwards dry_run=True and does not call confirm_destructive_action during preview. |
CHANGELOG.md |
Documents the corrected --dry-run behavior for repair --mode from-sqlite. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Summary
Fixes #2133 / #2095:
mempalace repair --mode from-sqlite --dry-runwas accepting the flag but running the real archive + rebuild.Supersedes / re-implements the approach from #1654 on current
develop(that PR is CONFLICTING after single-writer + mine-lock work).Behavior
rebuild_from_sqlite(..., dry_run=True)validates source, prints per-collection would-be upsert counts viasqlite_drawer_count, returns those counts, and does not:mine_palace_lockconfirm_destructive_actionunder--dry-run(preview is not gated on--yes){}(fail closed) rather thanor 0(addresses fatkobra review note on fix(repair): honor --dry-run in repair --mode from-sqlite #1654)Tests
.pre-rebuild-*dry_run=Trueand does not call confirmChangelog
Notes added under the 3.7.0 entry so this ships with the release.
Test plan