[GGzili]

Summary

Addresses all four blocking items from PR #5 review:

• restore command safety: no-arg lists backups and exits; --latest restores most recent; --force skips
  confirmation
• prune-backups timestamp parsing: sorts by filename timestamp, not fs metadata; unparseable .bak files skipped
  with warning
• sync --dry-run completeness: previews both SQLite rows AND rollout files
  (would-update/would-prepare/would-skip)
• Documentation: README updated in CN and EN

Test plan

• cargo check passes
• cargo fmt clean
• All 36 existing tests pass
• Manual: codex-threadripper restore lists backups
• Manual: codex-threadripper sync --dry-run shows preview
• Manual: codex-threadripper prune-backups --keep 3 --force

---

[Wangnov]

Request changes.

Thanks for addressing the restore/prune/dry-run surface. The direction looks good, but I would not merge this yet:

1. CI is currently red: cargo clippy --locked --all-targets -- -D warnings fails on src/state_db.rs due to clippy::unnecessary_sort_by. Please switch the backup sort to sort_by_key(... Reverse(...)).

2. restore_sqlite_from_backup is not crash/failure safe. It removes the live SQLite file before copying the backup into place, so a copy/fsync failure can leave the user with no state DB. Please restore via temp-file copy + fsync + atomic rename + parent dir sync, and consider cleaning stale -wal / -shm sidecars.

3. restore <BACKUP_PATH> --latest is currently accepted and silently uses the explicit path. Please make these clap args mutually exclusive or return an explicit error.

4. Please add automated tests for restore/prune/dry-run. The PR currently says the 36 existing tests pass, but these new safety paths are exactly where regressions would be expensive.

[Wangnov]

Thanks for the v2 — big step up from #5. All four blocking items from the #5 review are addressed, and the hygiene checks pass on my end:

• cargo fmt --check, cargo build, cargo clippy --all-targets -- -D warnings all clean
• cargo test → 36 passed
• dry-run reuses the real read_first_line / parse_matching_session_meta / set_session_meta_provider path, so the preview matches the actual sync — nice touch
• prune sorts by the filename timestamp and skips unparseable .bak with a warning; restore with no arg lists + exits, --latest is explicit, and there's a [y/N] confirm

The feature set is worth having. Before merge, though, restore/prune are destructive and have a few data-safety issues that need fixing. Grouped below.

Blocking

B1 — restore doesn't clear the WAL/SHM sidecars, which can corrupt the restored DB.
Codex's state_5.sqlite runs in WAL mode (there's a multi-MB -wal beside it at runtime). restore_sqlite_from_backup does remove_file(main) + fs::copy(backup → main) only, leaving the previous DB's state_5.sqlite-wal / -shm in place. Next time Codex/app-server opens "new main DB + stale WAL", SQLite tries to replay a WAL that doesn't belong to it — best case ignored, worst case it replays stale pages into the restored DB or reports database disk image is malformed.

Minimal repro:

T=$(mktemp -d); mkdir -p "$T/backups"
sqlite3 "$T/backups/seed.db" "CREATE TABLE threads(id TEXT NOT NULL, model_provider TEXT NOT NULL); INSERT INTO threads VALUES('row_from_BACKUP','openai');"
mv "$T/backups/seed.db" "$T/backups/state_5.sqlite.1700000000000.bak"
sqlite3 "$T/state_5.sqlite" "CREATE TABLE threads(id TEXT NOT NULL, model_provider TEXT NOT NULL); INSERT INTO threads VALUES('row_from_CURRENT','cong');"
printf 'STALE' > "$T/state_5.sqlite-wal"; printf 'STALE' > "$T/state_5.sqlite-shm"
codex-threadripper restore --latest --force --codex-home "$T"
ls "$T"   # state_5.sqlite-wal and -shm are still there

Fix: delete state_5.sqlite-wal and state_5.sqlite-shm next to the target as part of restore.

B2 — restore is non-atomic, doesn't snapshot the current DB, and can self-delete the restore source.
remove_file(main) then fs::copy means if the copy itself fails (disk full, I/O error, backup removed concurrently), you're left with the original deleted and the new one not written — the state DB is gone. (A corrupt backup is already guarded by the SELECT COUNT(*) FROM threads check before the delete, which is good.) Two things to fix:

• Snapshot the current DB first (create_sqlite_backup_file(main) if it exists), then copy to a temp file + fsync + atomic rename over the target — matches the temp+rename pattern already used for backups and rewrite_first_line_atomically.
• A concrete reachable case: passing the live DB path itself (or a sym/hardlink to it) as BACKUP_PATH passes the threads check, then remove_file(target) deletes the restore source and fs::copy fails → no usable DB. Guard by canonicalize-ing both paths and rejecting source == target.

B7 — list_backups matches any *.bak, not state_5.sqlite.<unix_ms>.bak.
It filters only on the .bak extension and parse_backup_timestamp just takes the digits after the last .. Three consequences:

• If backups/ ever contains .bak files for the other embedded DBs (goals_1 / memories_1 / logs_2) or anything else, prune-backups sorts them all together and can delete a different database's backups — real data loss.
• restore --latest can pick an unrelated *.bak that happens to have the largest trailing number.
• Conversely, legitimately-but-non-unix_ms-named backups are silently treated as "unparseable" and ignored by restore --latest / prune (e.g. a deliberately-named state_5.sqlite.before-local-thread-repair.<…>.bak).

Fix: scope list_backups / parse_backup_timestamp to the current DB's filename — only match ^<sqlite_file_name>\.(\d+)\.bak$.

Should-fix

B3 — restore/prune don't account for a running Codex. If app-server is running during restore, it can overwrite the restored DB or conflict on the WAL. At minimum, add a line to the confirm prompt asking the user to quit Codex / Codex.app first.

B4 — no tests for the new code. parse_backup_timestamp is a pure function and trivial to cover; the prune keep/boundary + unparseable-skip behavior, a restore round-trip, and the dry-run counts should all be tested. A destructive command shouldn't really land without tests.

B5 — prune's kept count ignores unparseable backups. prune_backups returns (deleted, keep), but unparseable .bak files stay on disk, so "kept N" won't match the actual file count in backups/.

B6 — dry-run output can contradict itself. When only would_skip > 0 (everything else 0), it prints the rollout section and "No changes needed — everything is already aligned", because the no-changes check doesn't include would_skip.

nit

• Open the backup read-only (OpenFlags::SQLITE_OPEN_READ_ONLY) for the threads validation. Opening a WAL-mode .bak read-write leaves *.bak-wal / *.bak-shm sidecars in backups/ that never get cleaned up.

Fix the Blocking items (and ideally add tests for the new paths) and I'm happy to merge. Thanks again for sticking with this.