Skip to content

fix: confine legacy skill adoption to staged roots - #290

Open
Jason (1315577677) wants to merge 1 commit into
microsoft:mainfrom
1315577677:fix/legacy-skill-root-containment
Open

Jason (1315577677) wants to merge 1 commit into
microsoft:mainfrom
1315577677:fix/legacy-skill-root-containment

Conversation

@1315577677

Copy link
Copy Markdown

Summary

  • record the trusted skill roots for legacy staging manifests
  • reject legacy SKILL.md targets outside those roots during adoption
  • add regression coverage for a retargeted legacy skill path

This addresses the legacy SKILL.md containment gap described in #288. The separate staging provenance-marker concern in that issue is out of scope.

Validation

  • python3 -m pytest -q
  • python3 -m ruff check skillopt_sleep/staging.py tests/test_sleep_adopt_skill_subset.py
  • python3 -m compileall -q skillopt_sleep tests
  • git diff --check

Full test suite: 1498 passed, 11 skipped.

@1315577677

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@Yif-Yang

Copy link
Copy Markdown
Contributor

Thank you for the narrowly scoped legacy containment change.

Reviewed 8191b3b, including a clean local merge onto 79124b3. Both exact-head and merge suites pass 1,496 tests, and the narrower scope is clearer than treating this as all of #288.

The remaining acceptance gap is the source of authority for the allowed roots: staged_skill_roots() reads the same editable manifest as the live destination. This check is useful containment against a destination-only change, but it does not establish the caller/config-authoritative boundary described in the issue. Please carry trusted destinations or roots into adoption, or protect the staging metadata before relying on it. Security-sensitive reproduction details should stay private under SECURITY.md.

Please coordinate with #289 so one coherent change covers the intended legacy boundary. Keep #288 open for the remaining memory/provenance/release work; exact-head official CI also still awaits approval.

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.

2 participants