Skip to content

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

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

RohithPariki wants to merge 3 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.

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