feat(sleep): adopt a reviewed subset of staged skills with rollback - #189
Closed
bogdanbaciu21 wants to merge 2 commits into
Closed
feat(sleep): adopt a reviewed subset of staged skills with rollback#189bogdanbaciu21 wants to merge 2 commits into
bogdanbaciu21 wants to merge 2 commits into
Conversation
Add SkillProposal, skill_proposal_rows, and write_skill_proposals: validate skill names, live target paths, and collisions before writing, then write each skill's proposal atomically. write_staging gains an optional skill_proposals fan-out and keeps the legacy single-proposal layout when it is unused. Refs microsoft#120
Add staged_skills and adopt_skills: validate the selection first, back up and atomically write each live skill, roll the whole selection back on failure, and return/persist sha256 before/after receipts. Legacy single-proposal adoption is unchanged, and multi-skill staging plus migration notes are documented. Refs microsoft#120
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends SkillOpt-Sleep’s staging/adoption pipeline to support multi-skill nights by staging one proposal per skill and enabling explicit adoption of a reviewed subset with backups, hashes, and rollback.
Changes:
- Add per-skill proposal staging (
SkillProposal, manifest"skills"rows, and atomic proposal file writes). - Add subset adoption API (
staged_skills(),adopt_skills()) with receipts persisted toadopted_skills.jsonand all-or-nothing rollback. - Document the multi-skill staging layout and subset adoption flow and link it from the Sleep docs index.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| skillopt_sleep/staging.py | Implements multi-skill proposal staging and subset adoption with backups/receipts/rollback. |
| tests/test_sleep_staging_fanout.py | Adds hermetic unittest coverage for per-skill staging fan-out validation and file layout. |
| tests/test_sleep_adopt_skill_subset.py | Adds end-to-end hermetic tests for subset adoption, receipts, refusal cases, and rollback behavior. |
| docs/sleep/README.md | Links to the new multi-skill staging documentation. |
| docs/sleep/multi-skill-staging.md | Documents multi-skill staging layout, subset adoption API, and migration notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+523
to
+526
| staged = os.path.join(staging_dir, str(row.get("proposed_file") or "")) | ||
| if not os.path.isfile(staged): | ||
| raise StagingError(f"staged proposal missing for {name!r}: {staged}") | ||
| plan.append((name, live, staged)) |
Comment on lines
+471
to
+472
| rows = manifest.get("skills") or [] | ||
| return [row for row in rows if isinstance(row, dict)] |
Comment on lines
+293
to
+305
| directory = os.path.dirname(path) or "." | ||
| os.makedirs(directory, exist_ok=True) | ||
| fd, tmp = tempfile.mkstemp(dir=directory, prefix=".tmp-", suffix=".md") | ||
| try: | ||
| with os.fdopen(fd, "w", encoding="utf-8") as f: | ||
| f.write(text) | ||
| f.flush() | ||
| os.fsync(f.fileno()) | ||
| os.replace(tmp, path) | ||
| except BaseException: | ||
| if os.path.exists(tmp): | ||
| os.unlink(tmp) | ||
| raise |
| os.makedirs(skill_backup, exist_ok=True) | ||
| backup_path = os.path.join(skill_backup, os.path.basename(live)) | ||
| shutil.copy2(live, backup_path) | ||
| before = _sha256_file(live) |
Contributor
Author
|
Thanks for the welcome on #120. To keep review load bounded and follow the incremental shape Yif-Yang suggested, I'm closing this PR for now and will reopen it once #182 (the harvesting slice) lands or the maintainer asks for the next slice. The branch stays on my fork so this can be re-opened as-is. Happy to restructure if a different cadence is preferred. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Final review-sized slice of #120: adopt an explicitly reviewed subset of a
multi-skill night's staged proposals, with receipts and all-or-nothing rollback.
In
skillopt_sleep/staging.py:staged_skills(staging_dir)reads the per-skill manifest rows;adopt_skills(staging_dir, skill_names=None)adopts the selection —None= every staged skill,[]= nothing. Unknown or repeated names, unsafemanifest rows, and missing proposal files raise
StagingErrorbefore anywrite; skills outside the selection are never touched;
backup/skills/<skill>/and writtenatomically, and if any write fails the whole selection is restored (files that
did not exist before are removed), so no partial adoption survives;
AdoptedSkillreceipts carryskill_name,live_skill_path,sha256_before(empty when there was no live file),
sha256_after, andbackup_path, and arealso written to
adopted_skills.jsonbeside the report.adopt()and the legacy single-proposal path are unchanged: a night that stagesno per-skill proposals has no
"skills"rows andadopt_skills()returns[].Adoption still never happens implicitly.
docs/sleep/multi-skill-staging.mddocuments the layout, the subset adoption API, and what consumers of
manifest.json/report.jsonneed to know to migrate; it is linked fromdocs/sleep/README.md.Tests
python -m pytest -q tests/test_sleep_adopt_skill_subset.py→ 14 passedpython -m pytest -q→ 584 passed, 7 skipped (basemainat8304e6c:556 passed, 7 skipped)
python -m ruff check skillopt_sleep/staging.py tests/test_sleep_adopt_skill_subset.py→ All checks passed
Covered by an end-to-end two-skill staged-night fixture: reading manifest rows,
legacy nights adopting nothing, one skill adopted while the other is untouched,
before/after hashes and backup contents, persisted receipts, empty and full
selections, a first-time live file reporting an empty before-hash, refusal of
unknown/repeated selections, missing proposals and unsafe rows, rollback of the
whole selection after a failed write (including removing newly created files), and
no adoption without an explicit call.
Refs #120