Skip to content

feat(sleep): add revert command to rollback adopted proposals - #280

Open
RohithPariki wants to merge 5 commits into
microsoft:mainfrom
RohithPariki:feat/247-revert-command
Open

RohithPariki wants to merge 5 commits into
microsoft:mainfrom
RohithPariki:feat/247-revert-command

Conversation

@RohithPariki

Copy link
Copy Markdown
Contributor

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.

@Yif-Yang

Copy link
Copy Markdown
Contributor

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.

… stack model

- Enforce adoption locking and recovery before reverting legacy or per-skill adoptions
- Validate and bind receipts against trusted staging manifest targets and safe root paths
- Enforce LIFO stack history ordering to prevent reverting an older night when a newer night is adopted
- Clean up immutable backups upon successful reversion to enable re-adoption (adopt -> revert -> adopt)
- Resolve default revert target using latest_adopted_staging to skip newer unadopted staging nights
- Expand test_sleep_revert.py with 20 unit tests covering adverse, boundary, and regression cases
@RohithPariki

Copy link
Copy Markdown
Contributor Author

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.

@Yif-Yang

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

skillopt-sleep: adopt() writes a backup that nothing can restore — add a revert command

2 participants