[RohithPariki]

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.
3. Reading containment roots solely from manifest.json allows an attacker with manifest write access to rewrite both live paths and manifest roots in the same edit without independent validation.

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. Derive authoritative destinations from caller in adopt():
   • adopt() now derives authoritative destinations from the caller: project_root (invoked_project), skill_search_roots(cfg), and an explicitly configured target_skill_path.
   • Manifest values (project_root and skill_roots) are treated as consistency checks against the caller's authority.
   • For label == "memory": Live target is confined to caller_project_root and verified against manifest staged_project_root.
   • For label == "skill": Live target is confined to caller destinations (invoked_project, skill_search_roots(cfg), and explicit target_skill_path) and verified against manifest staged_skill_roots.
   • cmd_adopt and run_sleep_cycle pass cfg=cfg into adopt().
4. CHANGELOG documentation:
   • Added CHANGELOG entry documenting legacy adoption confinement and the fail-closed restage requirement for nights staged before this change.

Testing

• Executed python -m pytest tests/test_sleep_adopt_skill_subset.py:
  • 111 tests passed (4 skipped due to platform-specific symlink support).
• Regression tests in TestLegacyAdoptionIsConfinedToTheStagedRoots and TestAdoptSkillCli:
  • test_tampered_manifest_roots_cannot_escape_caller_destinations
  • test_tampered_manifest_project_root_cannot_escape_caller_project
  • test_legacy_adoption_accepts_caller_explicit_target_outside_native_roots
  • test_cmd_adopt_refuses_tampered_manifest_roots
  • test_legacy_skill_retargeted_onto_an_outside_file_is_refused
  • test_legacy_skill_retargeted_to_create_a_new_outside_file_is_refused
  • test_legacy_memory_retargeted_onto_an_outside_file_is_refused
  • test_legacy_memory_retargeted_to_create_a_new_outside_file_is_refused
  • test_legacy_manifest_without_recorded_skill_roots_is_refused
  • test_legacy_manifest_without_recorded_project_root_is_refused
  • test_staged_project_root_reads_and_validates
  • test_ordinary_legacy_adoption_still_succeeds

Risk

Low. Legitimate legacy in-root adoption and explicitly configured targets are fully supported. Staged nights written with unconfined or missing root manifests fail closed with an explicit error to restage.

Issue

Refs #288

[Yifan Yang (Yif-Yang)]

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]

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.

[Yifan Yang (Yif-Yang)]

Thanks for the update, and sorry for the slow turnaround. I re-reviewed a2436775d8b2 merged onto current main (52ad5a2cd9ce). It merges cleanly, and the full isolated suite passes (2041 passed, 13 skipped, 4 xfailed). The explicit-target regression from my last review is fixed: test_legacy_adoption_accepts_explicit_target_outside_native_roots covers it, and the new legacy containment tests pass.

One point from that review is still open: the roots that adopt() confines legacy targets to are read from the staging manifest itself (project_root and skill_roots). Anything able to rewrite a manifest's live paths can rewrite those fields in the same edit, so they do not add an independent boundary. I confirmed this locally. Please derive the allowed destinations from the caller instead: the current config's project (invoked_project), skill_search_roots(cfg) and an explicitly configured target_skill_path. Treat the manifest values only as a consistency check against those. cmd_adopt already has cfg, so it can pass them in.

Two smaller items:

• Nights staged before this change now stop with "missing 'project_root' … Discard and restage this night". That is reasonable fail-closed behaviour, but please add a CHANGELOG line, since anyone with a pending night will hit it.
• The description still says "Closes Release cut for the Aug adopt/webui hardening + 2 residual staging gaps #288". Please change it to "Refs Release cut for the Aug adopt/webui hardening + 2 residual staging gaps #288": the release request and the staging-provenance gap in Release cut for the Aug adopt/webui hardening + 2 residual staging gaps #288 remain open.

The CI run on this head never started any jobs. I'll trigger a fresh run once the next revision is up.

[RohithPariki]

Thanks Yifan Yang (@Yif-Yang) for the review and guidance!

I have updated the branch with the requested changes:

1. Caller-Authoritative Destinations in adopt():
   • adopt() now derives allowed destinations from the caller (invoked_project, skill_search_roots(cfg), and explicit target_skill_path).
   • Manifest values (project_root and skill_roots) are verified as consistency checks against the caller-authorized destinations, ensuring that an attacker modifying both manifest live paths and manifest roots cannot bypass the caller's trust boundary.
   • cmd_adopt and run_sleep_cycle pass cfg=cfg into adopt().
2. CHANGELOG Entry:
   • Documented under ### Changed that legacy adoption confines targets to caller-authorized destinations and that older nights staged without a recorded project_root in manifest.json fail closed with StagingError requiring restaging.
3. PR Description Scope:
   • Updated the issue reference in the PR description to Refs #288.
4. Regression Tests:
   • Added test_tampered_manifest_roots_cannot_escape_caller_destinations
   • Added test_tampered_manifest_project_root_cannot_escape_caller_project
   • Added test_legacy_adoption_accepts_caller_explicit_target_outside_native_roots
   • Added test_cmd_adopt_refuses_tampered_manifest_roots

Cleanly synced and merged with latest upstream/main. All 111 tests in tests/test_sleep_adopt_skill_subset.py and ruff check pass cleanly.

[Yifan Yang (Yif-Yang)]

Merged, thank you for the careful follow-through. Through the real caller paths (skillopt-sleep adopt, the cycle's auto-adopt), legacy adoption now confines targets to destinations derived from the current config. Manifest roots are only a consistency check. I re-ran the case from my previous review, where the manifest's live path and its recorded project_root are rewritten together: with cfg it is now refused, and the outside file is untouched.

One non-blocking follow-up: a direct adopt(staging_dir) call that passes no caller authority still falls back to the manifest's recorded roots. No in-repo caller does that, but failing closed there (or requiring cfg/explicit roots) would make the boundary unconditional. Happy to take that as a small separate PR.

The release cut and staging-provenance items in #288 stay open.