Skip to content

feat(sleep): add skillopt-sleep revert to undo an adopt - #248

Open
pravit-amp wants to merge 1 commit into
microsoft:mainfrom
pravit-amp:feat/sleep-revert
Open

feat(sleep): add skillopt-sleep revert to undo an adopt#248
pravit-amp wants to merge 1 commit into
microsoft:mainfrom
pravit-amp:feat/sleep-revert

Conversation

@pravit-amp

Copy link
Copy Markdown
Contributor

Fixes #247

Summary

adopt() backed up every live file it replaced, and nothing could restore that backup — there was no revert, rollback, or undo anywhere in the CLI. Adoption can also run unattended, since cmd_schedule forwards --auto-adopt straight into the installed cron entry / Scheduled Task, so a night that regressed the live skill left the user hand-copying files out of a timestamped staging directory.

  • adds skillopt-sleep revert, undoing the most recent adopt for a project
  • records in the manifest what each adopt replaced, so revert knows what to put back
  • stops adopt from destroying its own backup when the same directory is adopted twice
  • reports the revert target in status, and points at the undo after adopt

Contract

revert restores the pre-adopt state of every live path the target staging directory touched.

A live document that adopt replaced is restored from backup/. A live file that adopt created — you had no such file — is removed instead, because that is the state being returned to; leaving the proposal in place would undo nothing. adopt now records which case each live path was in, since after adoption the file exists either way and the distinction is no longer observable.

Without --staging, the target is the most recently adopted directory that has not already been reverted — deliberately not latest_staging(), whose newest entry may never have been adopted, in which case restoring its backup would undo a change the live files never received. --staging DIR reverts a specific night, so a user can walk back more than one step.

revert raises rather than reporting a successful no-op when the target was never adopted, or when a backup the manifest claims is missing from disk.

Staging directories adopted before this change carry no record, only a backup/ directory. Those are still revertible: their backup files are matched to the manifest's live paths by basename. A file such an adopt created was never backed up and is indistinguishable from one it never touched, so those are left alone rather than deleted on a guess.

The backup fix is a prerequisite, not scope creep

_backup() overwrote unconditionally. Adopting the same staging directory twice copied the already-applied proposal over backup/, losing the only copy of the user's pre-adopt document:

adopt(st); print(open(f"{st}/backup/SKILL.md").read())   # ORIGINAL hand-written skill
adopt(st); print(open(f"{st}/backup/SKILL.md").read())   # PROPOSED (regressed) skill

skillopt-sleep adopt with no --staging resolves to latest_staging(project), so running it twice in a row is easy to do by accident. Built on the old behavior, revert would restore the very regression it exists to undo. _backup() now leaves an existing backup alone, and adopt reuses the recorded entry for a live path instead of recomputing it against the file adopt itself just wrote.

This is listed separately under Fixed in the changelog and splits cleanly into its own PR if maintainers prefer — the cut is _backup() plus the recorded/prior branch in adopt().

Validation

  • 16 new tests in tests/test_sleep_revert.py — restore, remove-created, skill+memory together, idempotent revert, double-adopt backup safety, never-adopted and missing-backup refusals, legacy staging dirs, latest_adopted_staging selection, and CLI round-trip
  • full suite: 1,110 passed, 10 skipped, 130 subtests passed
  • three failures on this branch (test_parse_jsonl_ignores_excessively_nested_json, two TestOverlayIntegration cases) reproduce unchanged on main at 9c776fc and are untouched by this PR — the overlay pair is a macOS /var/private/var symlink issue in superpowers.py, the parser case is a deep-nesting guard that no longer fires on Python 3.13+
  • end-to-end through run_sleep_cycle with auto_adopt=True: revert removed both the created SKILL.md and CLAUDE.md, and a second revert exited 1 with "nothing to revert"
  • python -m mkdocs build --strict passed
  • Ruff clean on the changed files, except one pre-existing unsorted-import finding at __main__.py:554 in cmd_schedule, left untouched to keep the diff focused

Notes for review

status gained two printed lines and a latest_adopted_staging JSON key. That was edge case 4 in the issue — nothing previously distinguished an adopted proposal from a staged one — and it makes the default revert target visible before you run it. Happy to drop it if you would rather keep this PR to the command alone.

Adoption had no undo. adopt() and adopt_skills() take an immutable backup
and publish a receipt, but nothing read either one back, so a night that
regressed a live skill left the user hand-copying files out of a
timestamped staging directory. That matters most unattended: schedule
forwards --auto-adopt into the installed cron entry.

revert() and revert_skills() reverse the receipts microsoft#212 already writes,
which pin the live path, the sha256 before adoption ("" when no file
existed), the sha256 adoption wrote, and the immutable backup. A document
adoption replaced is restored from its backup; one adoption created is
removed, since that is the state being returned to. Selection mirrors
adopt: --skill, --all-skills, --legacy, --staging.

Reverting consumes the backup and drops the night's receipt rows, so the
night can be adopted again — adoption refuses to run while an immutable
backup is present.

It refuses when the live file no longer matches what adoption wrote: it
was edited since, and restoring the backup would discard that work.
Directories adoption created are left alone, because the receipt records
no created_dirs and removing a path whose ownership was not durably
recorded is what the adoption recovery path already refuses.
@pravit-amp

Copy link
Copy Markdown
Contributor Author

Rebased onto bdfdc30 and rewritten on top of #212. The description above predates that rebase and describes the earlier revision — this comment is the accurate summary of what the branch now does.

What changed. The first revision predated #212 and carried its own adoption bookkeeping in manifest.json. That is gone. adopt and adopt_skills already publish receipts (adopted_legacy.json, adopted_skills.json) pinning live_path, sha256_before ("" when no file existed), sha256_after, and backup_path — everything an undo needs. revert() and revert_skills() simply reverse those.

Consequences:

  • The backup-clobber fix is dropped. The earlier revision fixed _backup overwriting itself on a second adopt. feat(sleep): adopt reviewed skill subsets safely #212's immutable backups and its "already adopted from this night" refusal made that moot.
  • Both ledgers are covered, with adopt's selection surface: --skill NAME (repeatable), --all-skills, --legacy, --staging DIR.
  • New refusal: if the live file no longer matches sha256_after, revert stops. It was edited or replaced after adoption, and restoring the backup would discard that work.
  • Revert consumes the backup and clears the night's receipt rows, returning the staging directory to its pre-adopt shape so it can be adopted again — adoption refuses to run while an immutable backup is present. A test covers the full adopt → revert → adopt → revert cycle.

Two deliberate limits, both documented in docs/reference/cli.md:

Directories adoption created are left in place. The receipt records no created_dirs, and removing a directory whose ownership was not durably recorded is the same fail-closed case _recover_transaction_locked already refuses.

Revert is not WAL-journaled. It takes _adoption_locks, runs _recover_before_manifest first so an interrupted adoption settles before anything is undone, re-reads the ledger under the lock, and checks every step against the receipt's pins — a live file already matching sha256_before is treated as done rather than as a mismatch, so an interrupted revert is completed by running it again. Promoting it into the durable transaction would mean teaching _TransactionTarget to delete a path, which the WAL schema has no representation for today, so the sha256_before == "" case needs a schema bump. Glad to do that as a follow-up if you would rather revert be journaled end to end.

On the test suite. 23 new tests in tests/test_sleep_revert.py, driving real adopt/adopt_skills calls rather than hand-built ledgers. Separately, and unrelated to this PR: main at bdfdc30 is currently red on macOS. Verified by stashing — clean main gives 38 failed, 1351 passed, this branch gives 38 failed, 1374 passed. Same 38, plus the 23 from here.

They are all one cause. /tmp and /var are symlinks to /private/... on macOS, so the canonicalization added in #212 returns a resolved path where the tests assert the unresolved one:

AssertionError: '/private/tmp/live/alpha/SKILL.md' != '/tmp/live/alpha/SKILL.md'

I have not touched it here. Happy to open a separate issue if that would be useful.

Also verified: end-to-end through run_sleep_cycle with auto_adopt=True against a pre-existing hand-written skill — adoption changed it (f69ed225b55edb0f5e360448), revert restored it byte-exact and removed the CLAUDE.md adoption had created. python -m mkdocs build --strict passes; Ruff is clean on the changed files.

One reviewer question: status gained a revertable_staging key and two printed lines. It already reports which skills were adopted from the latest night, but the revertable night can be an older one, and that is what a bare revert acts on. Happy to drop it if you would rather keep this to the command alone.

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

1 participant