Skip to content

fix(sleep): confine legacy SKILL.md and CLAUDE.md adoption to staged roots - #289

Open
RohithPariki wants to merge 2 commits into
microsoft:mainfrom
RohithPariki:fix/legacy-adopt-roots-containment
Open

RohithPariki wants to merge 2 commits into
microsoft:mainfrom
RohithPariki:fix/legacy-adopt-roots-containment

Conversation

@RohithPariki

Copy link
Copy Markdown
Contributor

Problem

Issue #288 identified an inconsistency between per-skill adoption (adopt_skills()) and legacy adoption (adopt()):
While adopt_skills() confines live skill targets to the recorded skills roots via _live_target_within_roots() and fails closed if targets escape, legacy adopt() performed no root containment checks on either live_skill_path or live_memory_path.
Furthermore, _planned_live_directories() would create any missing parent directory tree and write staged proposals to arbitrary paths outside the project or skills roots (such as ~/.bashrc or system files) if a manifest was retargeted or tampered with.
In addition, write_staging() did not record skill_roots in the manifest when staging pure legacy proposals, leaving legacy adoptions without a recorded root boundary.

Root Cause

  1. write_staging() omitted project_root from manifest.json and only recorded manifest["skill_roots"] when skill_rows (per-skill proposals) was non-empty.
  2. adopt() validated basename, canonical realpath, and symlink/junction presence, but never validated target containment against skill_roots or project_root.

Solution

  1. Record roots in staging:
    • In write_staging(): Always record manifest["project_root"] = os.path.abspath(project) and populate manifest["skill_roots"] even for legacy proposals (deriving the root from live_skill_path and skill_search_roots).
    • In run_sleep_cycle(): Pass skill_search_roots(cfg) unconditionally to write_staging().
  2. Read and validate staged project root:
    • Added staged_project_root(staging_dir: str) -> str to mirror staged_skill_roots(), requiring an absolute project_root in manifest.json and failing closed with an informative StagingError if missing or invalid.
  3. Enforce containment in adopt():
    • For label == "skill": Ensure _live_target_within_roots(live, skill_roots) holds; otherwise raise StagingError ("live target for legacy skill is outside the skills roots recorded when this night was staged: ...").
    • For label == "memory": Ensure _live_target_within_roots(live, [project_root]) holds; otherwise raise StagingError ("live target for legacy memory is outside the project root: ...").
    • This containment check executes before any directory creation or adoption transaction targets are appended.

Testing

  • Executed python -m pytest tests/test_sleep_adopt_skill_subset.py:
    • 106 tests passed (4 skipped due to platform-specific symlink support).
  • Added TestLegacyAdoptionIsConfinedToTheStagedRoots with 8 focused regression tests:
    • test_legacy_skill_retargeted_onto_an_outside_file_is_refused (victim untouched)
    • test_legacy_skill_retargeted_to_create_a_new_outside_file_is_refused (no directories or files created)
    • test_legacy_memory_retargeted_onto_an_outside_file_is_refused (victim untouched)
    • test_legacy_memory_retargeted_to_create_a_new_outside_file_is_refused (no directory or file created)
    • test_legacy_manifest_without_recorded_skill_roots_is_refused (fails closed)
    • test_legacy_manifest_without_recorded_project_root_is_refused (fails closed)
    • test_staged_project_root_reads_and_validates
    • test_ordinary_legacy_adoption_still_succeeds
  • Executed full cycle and adopt tests in tests/test_sleep_engine.py: all 5 tests passed.

Risk

Low. Valid legacy in-root adoption behavior is fully preserved. Staged nights written with unconfined or missing root manifests fail closed with an explicit error to restage.

Issue

Fixes #288

…roots

Addresses microsoft#288 by ensuring legacy adopt() verifies that live targets are
confined within allowed roots recorded when the night was staged:
- In write_staging(): record project_root in manifest and ensure skill_roots
  is recorded even for pure legacy proposals.
- Add staged_project_root() to read and validate the staged project root.
- In adopt(): require and validate that live_skill_path is within staged
  skill_roots and live_memory_path is within staged project_root, failing
  closed before any files or directory trees are created outside.
- Add TestLegacyAdoptionIsConfinedToTheStagedRoots with 8 regression tests.
@Yif-Yang

Copy link
Copy Markdown
Contributor

Thank you for extending containment checks to the legacy adoption path.

Reviewed 9e157df against current main. The legacy containment checks are useful, but I found a compatibility regression in the real cycle: with an existing native skills root and an explicit target_skill_path outside that root, the cycle accepts and stages the proposal, then adopt refuses the configured target. The same synthetic cycle succeeds on main. Please include the caller's explicitly configured managed target in the authoritative destination policy and add a cycle-to-adopt regression.

There is also still a distinction between checking manifest consistency and establishing which roots the caller trusts. Please bind this boundary to caller/config-authoritative destinations; the root metadata in the manifest is not itself that authority. Coordinate security-sensitive reproduction details privately under SECURITY.md.

Please change Fixes #288 to partial-scope wording: the release and staging-provenance requests remain open. The focused suite passes 228 tests, but these gaps and exact-head CI remain outstanding.

@RohithPariki

RohithPariki commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks for catching this! I have pushed a fix that unconditionally includes the caller's explicitly configured managed target in the authoritative destination policy. I also added a cycle-to-adopt regression test (test_legacy_adoption_accepts_explicit_target_outside_native_roots) to prevent this from recurring.

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.

Release cut for the Aug adopt/webui hardening + 2 residual staging gaps

2 participants