Skip to content

feat(sleep): adopt reviewed skill subsets safely - #212

Open
Bogdan (Dan) Baciu (bogdanbaciu21) wants to merge 7 commits into
microsoft:mainfrom
bogdanbaciu21:skoc-008-adopt-reviewed-skill-subset
Open

feat(sleep): adopt reviewed skill subsets safely#212
Bogdan (Dan) Baciu (bogdanbaciu21) wants to merge 7 commits into
microsoft:mainfrom
bogdanbaciu21:skoc-008-adopt-reviewed-skill-subset

Conversation

@bogdanbaciu21

@bogdanbaciu21 Bogdan (Dan) Baciu (bogdanbaciu21) commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds an end-to-end, review-gated multi-skill Sleep workflow. Each hinted skill is consolidated from its own pinned live baseline, staged independently, and promoted only through an explicit managed or per-skill selection.

Workflow

  • Enable canonical multi_skill_fanout (multi_skill_report remains an alias).
  • Resolve each hint through native project roots or repeatable --skill-root.
  • Read and pin that skill's live bytes and canonical target before consolidation.
  • Stage only independently accepted group proposals with per-group evidence.
  • Inspect pending/adopted/managed state with status.
  • Promote with repeatable --skill NAME, pending-only --all-skills, or --legacy for a co-staged managed proposal.
  • Bare adopt refuses ambiguous fan-out nights; auto-adopt never promotes fan-out.

Integrity contract

  • Versioned, fail-closed manifest with proposal, live-byte, and canonical-path pins.
  • Whole-manifest uniqueness, hard-link, symlink/junction, mode, identity, and drift validation under a staging lock and shared per-target locks.
  • Immutable no-replace backups and append-only exact-schema receipts.
  • WAL-backed managed and fan-out transactions with caught-failure rollback and restart recovery that preserves later external edits.
  • Atomic staging reservation and durable .latest publication.
  • Safe old-runtime compatibility: PyPI 0.2.0 treats new manifests as a no-op; new runtimes refuse old unpinned nights and require restaging.

Interfaces and safety

  • One-document JSON for run/status/adopt/handoff success and failure.
  • Runtime-validated Copilot and Devin MCP arguments with preserved engine status.
  • No post-adoption adapter copies outside the core transaction.
  • Secret/control/ANSI/bidi neutralization in human and Markdown output.
  • Shell-safe POSIX/PowerShell scheduler generation and bounded schedule times.

Validation

  • Full suite: 1,220 passed, 12 skipped, 265 subtests passed.
  • Focused core: 315 passed, 3 skipped, 6 subtests passed.
  • Plugin/MCP: 73 passed, 127 subtests passed.
  • Ruff, compileall, git diff --check, and MkDocs strict build: clean.

Portability boundary

Cooperative SkillOpt adopters serialize through shared locks and revalidate immediately around publication. Portable Python does not provide a filesystem compare-and-swap against an unrelated editor that ignores those locks. POSIX directory entries are fsynced; Windows file contents are flushed, but Python's standard library does not expose an equivalent portable directory fsync.

Supersedes #189. Thanks Yif-Yang for the detailed review and acceptance criteria.

@Yif-Yang

Copy link
Copy Markdown
Contributor

Thanks for working on a safer reviewed-skill adoption flow. The staging/adoption primitives are a useful foundation, but the user-facing workflow is not connected end to end yet.

Before merging, could you please:

  • Connect run_sleep_cycle → SkillProposal → write_staging, then provide a CLI flow that lists staged skills and lets the user adopt a selected subset.
  • Add a cycle-level integration test where more than one skill is proposed, all proposals are staged, and only the selected subset is promoted.
  • Update the documentation so it clearly distinguishes the low-level adoption API added here from an end-to-end multi-skill nightly workflow; the latter is not yet available in the current implementation.
  • Revalidate manifest uniqueness and live-target nonexistence at adoption time, rather than relying only on staging-time checks.
  • Preserve transactionality if updating adopted_skills.json fails: any live skill files written by that adoption should be rolled back as well.

Once the workflow and these safety edges are covered, we would be happy to re-review it.

Address PR 212 review: run_sleep_cycle stages resolved SkillProposals,
status/adopt list and select a subset, uniqueness is rechecked at adopt,
and a failed adopted_skills.json write rolls live files back.

Refs microsoft#212
@bogdanbaciu21

Copy link
Copy Markdown
Contributor Author

Absolutely. On it.

Adversarial CLI, adopt-time, cycle-staging, and auto-adopt cases for
Yifan's five review items. Also tidy isort on the files this slice
touches.

Refs microsoft#120
Harden PR 212 adopt: sha256 pin each staged skill, revalidate the
whole manifest before any live write, refuse symlink/missing-parent
targets, skip notes on the cycle report, and reject empty --skill.

Refs microsoft#212
@bogdanbaciu21
Bogdan (Dan) Baciu (bogdanbaciu21) force-pushed the skoc-008-adopt-reviewed-skill-subset branch from 5b7f47f to f393a7a Compare August 12, 2026 19:05
@bogdanbaciu21

Copy link
Copy Markdown
Contributor Author

Yifan – five items from your review are in this push.

Cycle now stages accepted skills that resolve uniquely. skillopt-sleep status lists them. adopt --skill NAME (repeatable) or --all-skills promotes the reviewed subset. Bare adopt on a multi-skill night lists the names and exits instead of promoting everything. Unresolved, empty, or colliding names skip the night instead of aborting it.

There is a cycle-level test that stages two live skills and adopts one. Only that live file changes.

Docs now split the low-level adopt API (this PR) from a full multi-skill nightly workflow. The latter is still not this PR: groups still consolidate from the managed document.

Adopt re-checks uniqueness against every staged row, not just the selection. If the receipt write fails, the live files from that adoption and the previous adopted_skills.json roll back.

I also tightened adopt itself: staged files are sha256-pinned, live writes refuse symlink targets and symlink parents and will not create parent directories, and empty bodies are refused.

Proof on f393a7a: full pytest 982 passed, 7 skipped. Ready for re-review.

@Yif-Yang

Copy link
Copy Markdown
Contributor

Thanks for addressing the five earlier review items. I re-reviewed the latest head (0fa78cd). The staging, selective-adoption, hash-pinning, and rollback protections now look strong, but there is one remaining correctness blocker in the cycle integration.

Every hinted SkillGroup is still constructed from the same managed skill, and each resulting new_skill is then staged against a different resolved live skill path. With two live skills that have distinct contents, both staged proposals are therefore derived from the managed scaffold rather than from their corresponding live SKILL.md. Adopting one can overwrite a named skill with content that never included its original contents.

Before merge, could you please either:

  • resolve and read the corresponding live skill before consolidating each group, so every proposal is derived from its own live baseline; or
  • keep this PR limited to the low-level staging/adoption API and disable the cycle-level fan-out until that semantic is supported.

Please add a cycle-level regression test with two live skills containing unique markers. Each staged proposal should preserve/derive from its matching marker and must not use the other skill or the managed scaffold as its baseline.

Two smaller items are also worth fixing in the same update: render report.md after cycle skip notes are finalized so it matches report.json, and safely quote or avoid the generated copy/paste --skill NAME commands because accepted names can contain spaces or shell metacharacters.

Once the baseline semantics are corrected, we will be happy to re-review it.

@bogdanbaciu21

Copy link
Copy Markdown
Contributor Author

Absolutely. On it.

@bogdanbaciu21

Copy link
Copy Markdown
Contributor Author

Yifan — thank you for the precise re-review. The remaining baseline blocker and the two smaller items are fixed in this update.

Each hinted group now resolves and reads its corresponding live SKILL.md before consolidation. The raw-byte baseline hash and canonical target identity are carried through staging, and publication is refused if either changes. Cycle regressions use two live skills with unique markers and prove that each proposal derives only from its matching baseline; adopting one subset leaves the other live skill untouched.

report.md is now rendered after skip notes are finalized, so it stays aligned with report.json. Generated guidance no longer interpolates skill names into shell commands: names are displayed as data and commands use a placeholder; Copilot and Devin forward selections as argument-vector elements.

I also completed the safety contract around the end-to-end path: manifest v2 pins, live-baseline drift checks, cross-night target locks, immutable backups and append-only receipts, WAL-backed rollback/restart recovery, pending-only subset adoption, hardened managed adoption, and explicit mixed-night modes. The documentation includes migration and honest POSIX/Windows durability boundaries.

Validation on a233a081:

  • full suite: 1,220 passed, 12 skipped, 265 subtests passed;
  • focused core convergence: 315 passed, 3 skipped, 6 subtests passed;
  • focused plugin/MCP: 73 passed, 127 subtests passed;
  • final adoption/scheduler security slice: 106 passed, 6 subtests passed;
  • changed-file Ruff, compileall, git diff --check, and MkDocs strict build: clean.

Ready for re-review.

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.

2 participants