[RohithPariki]

Resolves #247

Implements the skillopt-sleep revert command to allow users to cleanly undo adopted proposals by restoring state from their immutable backups.

Changes

• CLI: Added revert subcommand to skillopt_sleep/__main__.py, matching the adopt interface (--staging, --skill, --all-skills, --legacy).
• Core: Added revert_skills and revert in skillopt_sleep/staging.py to restore original bytes safely.
• Safety:
  • Reversion refuses to run if the live file has been locally modified since adoption (protects user's work).
  • Automatically unlinks files that were originally created during the adoption, leaving no orphaned files.
• Testing: Added test_sleep_revert.py with idempotent behavior validation.

[Yifan Yang (Yif-Yang)]

Thank you for adding an explicit undo command; safe rollback is a useful missing capability.

Reviewed 312aaf7 against the current adoption machinery and the outstanding #248 review. The undo command is useful, but this implementation does not resolve that review's receipt-binding, history-ordering, and recovery requirements, and it omits adoption locking/recovery entirely. Please use validated, trusted receipt/destination bindings and a durable transaction before adding filesystem restoration or deletion. A matching content hash alone does not establish that an old night is the current adoption. Coordinate security-sensitive reproduction details privately under SECURITY.md.

Two additional functional regressions are reproducible with synthetic fixtures: adopt -> revert -> adopt fails because revert clears the receipt but leaves the immutable backup, and bare revert after a newer unadopted staging night reports success without undoing the earlier adoption. The default should select the current adopted history head, and backup/receipt cleanup should be recoverable.

The focused suite passes 104 tests, but these adverse paths are not covered by the two new happy-path tests. Please coordinate one coherent rollback implementation with #248 and keep #247 open until the safety contract is met.

[RohithPariki]

Thank you for the detailed review and guidance! I have updated the branch to address all the safety, transaction, and history-ordering requirements:

1. Adoption Locking & Recovery:

   • Revert operations for both legacy managed files and per-skill proposals now run under _adoption_locks across the staging directory and target live paths, and invoke _recover_before_manifest before transactions.
   • Receipt file ID, inode/dev identity, and file modes are validated against race conditions during lock acquisition.

2. Trusted Receipt & Manifest Binding:

   • Receipt live paths are strictly bound to and validated against the trusted staging manifest (_safe_live_path, _adopt_target_ok, _adopt_live_target_ok).
   • Tampered receipts attempting to target paths outside the project roots or containing unexpected schema fields are rejected.

3. LIFO History Ordering (Stack Model):

   • Added _assert_current_adopted_head: an older adopted night cannot be reverted if a subsequent night has also adopted the same target/skill. Newer adoptions must be rolled back first.

4. Resolved adopt -> revert -> adopt Regression:

   • Upon successful rollback, immutable backups and empty backup subdirectories in the staging directory are cleanly unlinked (_unlink_fsync and _clean_empty_backup_dir), allowing clean re-adoption.

5. Default Selection of Adopted History Head:

   • Bare skillopt-sleep revert now selects the target night via latest_adopted_staging(project), ensuring it skips newer staging nights that were never adopted.

6. Expanded Test Suite:

   • Expanded tests/test_sleep_revert.py to 20 comprehensive unit tests covering all adverse paths (tampered receipts, external target paths, stack-order enforcement, unadopted newer nights, and full adopt-revert-adopt cycles). All tests and ruff lint checks pass cleanly.

[Yifan Yang (Yif-Yang)]

Thank you for addressing locking, skipping unadopted nights, and readoption. The October 6 review of bfd8b28320d81 verified those improvements with positive controls; the older comment should not be read as claiming they are still absent.

The revised PR remains unmergeable on correctness/safety grounds, however:

1. Rollback needs whole-set prevalidation and its own durable transaction. A later-target validation failure or final receipt-write failure can leave an earlier target restored and its immutable backup removed without a recoverable committed rollback. Invoking recovery for an earlier adoption transaction does not make the new rollback transaction durable.
2. Binding destination paths is necessary but does not by itself establish the authority of the before/after state and backup used for restoration. Those bindings must be validated before any modification; security-sensitive reproduction details remain private under SECURITY.md.
3. Staging creation order is not adoption order. A valid multi-night sequence can adopt an earlier-generated proposal later, after which both explicit and default rollback selection can reject the legitimate latest adoption.

The shipped focused tests passed 163 tests, while independent producer-realistic checks found three blocker categories across both legacy and per-skill modes. These are not small issues we can safely defer until after merge. Please coordinate one coherent implementation with #248, covering failure recovery, authoritative state binding and actual adoption lineage. #247 remains open; the existence of a revert command alone does not satisfy that request safely.

[RohithPariki]

Thanks for the thorough architectural review Yifan Yang (@Yif-Yang)! We have updated the PR to address all points:

1. Whole-Set Prevalidation & Durable Rollback Transaction WAL

• Prevalidation: Revert now inspects and prevalidates the entire target set (including live targets, immutable backups, and receipts) prior to performing any disk mutation. If any target fails validation (e.g., modified live file, corrupted or missing backup), the entire operation fails closed immediately without partial mutations.
• Dedicated Rollback WAL (.revert-transaction.json): Added a two-phase rollback journal (_REVERT_WAL_FILENAME = ".revert-transaction.json").
  • Crash before receipt commit: Backward recovery restores any mutated live targets back to their proposal state (sha256_after), keeps all backups intact, and clears the journal.
  • Crash after receipt commit: Forward recovery completes post-commit backup removal, updates the lineage ledger, and unlinks the journal.
• Deferred Backup Deletion: Immutable backup files are only deleted strictly after the updated receipt has been durably published to disk via atomic write and parent fsync.

2. Authoritative State & Backup Binding

• revert_skills and revert cross-validate receipt fields (live_skill_path, sha256_before, sha256_after, backup_path) against the authoritative staging manifest.json and trusted containment roots before any modifications take place.
• Rejects tampered receipts with divergent hashes, paths outside allowed skill roots, or unexpected backup paths on newly created files (sha256_before is null/empty).

3. Adoption Lineage Ledger (.adoption_lineage.json)

• Replaced creation timestamp / directory name sorting with a durable .adoption_lineage.json ledger protected by .adoption_lineage.lock.
• latest_adopted_staging(project) queries this ledger to resolve the true most recently adopted night, correctly handling out-of-order and multi-night adoptions.
• A per-target stack model enforces that if multiple nights have touched the same live target, older adoptions cannot be reverted while newer adoptions remain active on that target, preventing ABA history corruption.
• Includes automatic self-healing bootstrap if initialized against pre-existing staging directories without a lineage file.

4. Test Coverage & Validation

• Added comprehensive unit tests in tests/test_sleep_revert.py:
  • test_out_of_order_multi_night_adoptions: Verifies lineage ordering and stack unwinding when nights are adopted out of chronological sequence.
  • test_whole_set_prevalidation_no_partial_mutation: Verifies that a failure on any target aborts the entire batch without partial live changes or deleted backups.
  • test_authoritative_manifest_hash_mismatch_refused: Verifies receipts with altered proposal/baseline hashes are rejected against manifest.json.
  • test_authoritative_manifest_created_file_unexpected_backup_refused: Verifies that newly created files cannot reference unexpected backup paths.
  • test_rollback_wal_backward_recovery_on_crash_before_receipt_commit: Simulates crash prior to receipt commit and validates backward restoration of live files and preservation of backups.
  • test_rollback_wal_forward_recovery_on_crash_after_receipt_commit: Simulates crash post-receipt commit and validates forward cleanup of backups and WAL removal.
• Synced cleanly with upstream main (343db22). All 26 revert tests and adoption transaction tests pass cleanly, and code style adheres to ruff.

[Yifan Yang (Yif-Yang)]

Re-reviewed fb65eaa019ff121784956bf99f1de3e4be6955c0. The new whole-set prevalidation, receipt-write recovery and state-binding checks resolve the corresponding earlier reproductions; the normal readoption controls also pass. The rollback selection passes 26 shipped tests, and 12 of 14 independent cases pass.

The remaining hold is adoption lineage. After reverting the most recent genuine adoption, the next real ancestor can still be blocked by a stale active lineage entry, for both legacy and per-skill rollback. The failure is now later in the sequence than the old staging-order problem. Please ensure receipt synchronization and explicit adoption recording cannot represent one adoption twice, and demonstrate complete successive rollback, imported pre-existing receipts and crash recovery. Sensitive reproduction details remain private. #247 stays open; this is not suitable for merge-first recovery repair.

[RohithPariki]

Thanks Yifan Yang (@Yif-Yang) for catching the duplicate entry in the lineage journal!

Root Cause

When _execute_transaction_locked commits an adoption, it writes the receipt file (adopted_skills.json or adopted_legacy.json) before calling _record_adoption_in_lineage. Inside _record_adoption_in_lineage, _ensure_lineage_synchronized scanned disk, saw the receipt that had just been written, and synthesized an adopt entry because it was not yet in entries. _record_adoption_in_lineage then appended a second adopt entry. On rollback, only one entry was popped, leaving the newer staging directory active on top of the stack and blocking ancestor revert.

Fix

1. Exclude pending adoptions during sync: _ensure_lineage_synchronized now accepts pending_adoption=(staging_dir, target_keys) and skips synthesizing entries for receipts currently being published by _record_adoption_in_lineage.
2. Idempotent recording: _record_adoption_in_lineage and _record_revert_in_lineage inspect _active_targets_from_lineage so identical consecutive adoptions or redundant reverts are never appended.
3. Self-healing deduplication: _sanitize_lineage_entries runs on load in _ensure_lineage_synchronized to clean up any pre-existing duplicate entries.
4. Regression test suite: Added tests in tests/test_sleep_revert.py covering:
   • test_successive_rollback_per_skill (complete multi-night rollback back to v0 baseline)
   • test_successive_rollback_legacy (successive rollback for legacy pairs)
   • test_pre_existing_receipts_imported_once_and_revertable (receipts without lineage imported once and successively reverted)
   • test_existing_duplicate_lineage_entries_sanitized (sanitizes corrupted/duplicated records on load)
   • test_crash_recovery_preserves_single_lineage_entry (idempotent rollback WAL recovery without duplicate lineage records)

All 31 tests in tests/test_sleep_revert.py and 98 tests in tests/test_sleep_adopt_skill_subset.py pass cleanly.

[Yifan Yang (Yif-Yang)]

Re-reviewed 35e3f7214dc3425f7c5b164df667e8af1969a465. The duplicate-lineage fix resolves the previous ordinary successive-rollback reproduction in both legacy and per-skill modes. Repeated readoption, intact-ledger adoption ordering, whole-set prevalidation, receipt-publication recovery and the prior binding controls also pass; 147 focused tests passed in total. Those earlier resolved findings should not be treated as still absent.

Two recovery/history invariants remain unmet. First, rollback can report success after lineage publication fails, discard its recovery journal, and leave receipt state inconsistent with active lineage; an ordinary retry then cannot unwind the genuine ancestor. Receipt and lineage publication must remain durably recoverable together, including the recovery path itself. Second, importing pre-existing receipts still reconstructs history from staging creation order, which can identify the wrong current adoption when actual adoption order differs. Recover trustworthy order where possible and explicitly refuse an ambiguous migration rather than inventing an order.

Four independent cases reproduce these two failures across both modes. This remains on hold; #247 should stay open until the complete recoverability and history contract is demonstrated. #248 remains an overlapping alternative rather than a second implementation to merge.

Review baseline: 1a44050fc53a. The PR remains open; no merge was performed.