From e657e09328b8d48ffe9f63a6833a23e35332f73c Mon Sep 17 00:00:00 2001 From: "Bogdan (Dan) Baciu" Date: Mon, 10 Aug 2026 02:32:34 +0400 Subject: [PATCH 1/4] feat(sleep): adopt reviewed skill subsets safely --- docs/sleep/README.md | 4 + docs/sleep/multi-skill-staging.md | 90 ++++++++++ skillopt_sleep/staging.py | 140 ++++++++++++++- tests/test_sleep_adopt_skill_subset.py | 239 +++++++++++++++++++++++++ 4 files changed, 472 insertions(+), 1 deletion(-) create mode 100644 docs/sleep/multi-skill-staging.md create mode 100644 tests/test_sleep_adopt_skill_subset.py diff --git a/docs/sleep/README.md b/docs/sleep/README.md index 5b29ebcf..7f5be874 100644 --- a/docs/sleep/README.md +++ b/docs/sleep/README.md @@ -299,6 +299,10 @@ gate keeps the worst case bounded; keep it **on** by default. ## Learn more +Staging a proposal for more than one skill, and adopting a reviewed subset of +them with backups and hash receipts, is documented in +[`docs/sleep/multi-skill-staging.md`](multi-skill-staging.md). + See the [SkillOpt documentation index](../index.md), the [CLI reference](../reference/cli.md), and the integration-specific READMEs under [`plugins/`](https://github.com/microsoft/SkillOpt/tree/main/plugins). diff --git a/docs/sleep/multi-skill-staging.md b/docs/sleep/multi-skill-staging.md new file mode 100644 index 00000000..2f29e2fb --- /dev/null +++ b/docs/sleep/multi-skill-staging.md @@ -0,0 +1,90 @@ +# Multi-skill staging and subset adoption + +A night can stage a proposal for more than one skill. Adoption stays explicit: +staging only ever writes into the staging directory, and `adopt_skills()` copies +a **reviewed subset** over the live files, with a backup and a hash receipt per +skill. + +Nothing here changes a single-managed-skill night. If a night stages no per-skill +proposals, the staging directory and `manifest.json` are exactly the legacy ones +and `skillopt-sleep adopt` keeps working unchanged. + +## Staging layout + +Legacy (single managed skill) — unchanged: + +```text +.skillopt-sleep/staging/20260728-013000/ +├── manifest.json # live_skill_path, live_memory_path, has_skill, has_memory, accepted +├── proposed_SKILL.md +├── proposed_CLAUDE.md +├── report.json +└── report.md +``` + +Multi-skill night — one extra file and one manifest row per skill: + +```text +.skillopt-sleep/staging/20260728-013000/ +├── manifest.json # …the legacy keys plus "skills": [ … ] +├── proposed_SKILL.alpha.md +├── proposed_SKILL.beta.md +├── report.json # report.skill_groups carries each skill's gate evidence +└── report.md +``` + +```json +{ + "live_skill_path": "/home/dev/.claude/skills/alpha/SKILL.md", + "has_skill": false, + "accepted": true, + "skills": [ + { + "skill_name": "alpha", + "proposed_file": "proposed_SKILL.alpha.md", + "live_skill_path": "/home/dev/.claude/skills/alpha/SKILL.md" + }, + { + "skill_name": "beta", + "proposed_file": "proposed_SKILL.beta.md", + "live_skill_path": "/home/dev/.claude/skills/beta/SKILL.md" + } + ] +} +``` + +A skill name must be a single safe path segment and a live path must be an +absolute, traversal-free `*.md` file; two skills may not share a name or a target +file. A refused fan-out writes no `manifest.json`, so the folder is not adoptable. + +## Adopting a reviewed subset + +```python +from skillopt_sleep.staging import adopt_skills, latest_staging, staged_skills + +staging = latest_staging("/path/to/project") +[row["skill_name"] for row in staged_skills(staging)] # ['alpha', 'beta'] + +receipts = adopt_skills(staging, ["alpha"]) # beta is left alone +receipts[0].sha256_before, receipts[0].sha256_after +``` + +- `skill_names=None` adopts every staged skill; `[]` adopts nothing. +- An unknown or repeated name, an unsafe manifest row, or a missing proposal file + raises `StagingError` **before** anything is written. +- Each live file is backed up to `backup/skills//` and written atomically. +- If any write fails, every file in the selection is restored (and files that did + not exist before are removed), so a partial adoption never survives. +- Receipts (`skill_name`, `live_skill_path`, `sha256_before`, `sha256_after`, + `backup_path`) are returned and written to `adopted_skills.json` in the staging + directory. An empty `sha256_before` means the skill had no live file yet. + +## Migrating + +- **Consumers of `manifest.json`**: treat `"skills"` as optional; when absent the + night is a legacy single-proposal one. +- **Consumers of `report.json`**: `skill_groups` is `[]` on a single-skill night, + and the flat `accepted` / `gate_action` / score fields keep their meaning. +- **Adoption tooling**: `adopt()` still adopts the legacy single proposal pair. + Use `adopt_skills()` for per-skill nights; the two are independent, and neither + runs implicitly. diff --git a/skillopt_sleep/staging.py b/skillopt_sleep/staging.py index fb6f8863..d4e4d63a 100644 --- a/skillopt_sleep/staging.py +++ b/skillopt_sleep/staging.py @@ -7,14 +7,16 @@ """ from __future__ import annotations +import hashlib import json import os import re import shutil +import stat import tempfile import time from dataclasses import dataclass -from typing import Any, Dict, Iterable, List, Optional +from typing import Any, Dict, Iterable, List, Optional, Sequence from skillopt_sleep.types import SleepReport @@ -310,12 +312,17 @@ def _write_atomic(path: str, text: str) -> None: """Write ``text`` to ``path`` atomically, so review never sees half a file.""" directory = os.path.dirname(path) or "." os.makedirs(directory, exist_ok=True) + existing_mode = ( + stat.S_IMODE(os.stat(path).st_mode) if os.path.exists(path) else None + ) 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()) + if existing_mode is not None: + os.chmod(tmp, existing_mode) os.replace(tmp, path) except BaseException: if os.path.exists(tmp): @@ -495,6 +502,137 @@ def write_staging( return out +@dataclass +class AdoptedSkill: + """Receipt for one adopted skill: where it landed and what changed.""" + + skill_name: str + live_skill_path: str + sha256_before: str # "" when no live file existed yet + sha256_after: str + backup_path: str = "" # "" when there was nothing to back up + + +def _sha256_text(text: str) -> str: + return hashlib.sha256(text.encode("utf-8")).hexdigest() + + +def staged_skills(staging_dir: str) -> List[Dict[str, Any]]: + """Manifest rows for the per-skill proposals staged in ``staging_dir``.""" + with open(os.path.join(staging_dir, "manifest.json"), encoding="utf-8") as f: + manifest = json.load(f) + if not isinstance(manifest, dict): + raise StagingError("staging manifest must be a JSON object") + if "skills" not in manifest: + return [] + rows = manifest["skills"] + if not isinstance(rows, list): + raise StagingError("staging manifest 'skills' must be a list") + if any(not isinstance(row, dict) for row in rows): + raise StagingError("every staging manifest 'skills' row must be an object") + return rows + + +def _selected_rows( + rows: Sequence[Dict[str, Any]], skill_names: Optional[Sequence[str]] +) -> List[Dict[str, Any]]: + """Rows for the reviewed subset, in manifest order, or every row.""" + if skill_names is None: + return list(rows) + wanted = [str(n).strip() for n in skill_names] + if not wanted: + return [] + known = {str(row.get("skill_name", "")) for row in rows} + unknown = [n for n in wanted if n not in known] + if unknown: + raise StagingError(f"no staged proposal for: {', '.join(sorted(unknown))}") + duplicates = {n for n in wanted if wanted.count(n) > 1} + if duplicates: + raise StagingError(f"skill selected twice: {', '.join(sorted(duplicates))}") + chosen = set(wanted) + return [row for row in rows if str(row.get("skill_name", "")) in chosen] + + +def adopt_skills( + staging_dir: str, skill_names: Optional[Sequence[str]] = None +) -> List[AdoptedSkill]: + """Adopt an explicitly reviewed subset of staged per-skill proposals. + + ``skill_names`` selects which staged skills to adopt; ``None`` means every + staged skill. Nothing is adopted implicitly and skills outside the selection + are never touched. + + Every selected proposal is validated first, each live file is backed up, and + the writes are rolled back as a set if any one of them fails, so a partial + adoption never survives. Returns a before/after sha256 receipt per skill and + also writes them to ``adopted_skills.json`` in the staging directory. + """ + rows = _selected_rows(staged_skills(staging_dir), skill_names) + if not rows: + return [] + + plan: List[tuple] = [] + for row in rows: + name = _safe_skill_name(row.get("skill_name")) + if not name: + raise StagingError(f"unsafe staged skill name: {row.get('skill_name')!r}") + live = _safe_live_path(row.get("live_skill_path")) + if not live: + raise StagingError( + f"unsafe live skill path for {name!r}: {row.get('live_skill_path')!r}" + ) + proposed_file = row.get("proposed_file") + expected_file = proposal_filename(name) + if proposed_file != expected_file: + raise StagingError( + f"unsafe staged proposal filename for {name!r}: {proposed_file!r}; " + f"expected {expected_file!r}" + ) + staged = os.path.join(staging_dir, expected_file) + if not os.path.isfile(staged): + raise StagingError(f"staged proposal missing for {name!r}: {staged}") + plan.append((name, live, staged)) + + backup_dir = os.path.join(staging_dir, "backup", "skills") + receipts: List[AdoptedSkill] = [] + done: List[tuple] = [] # (live, original_bytes or None) for rollback + try: + for name, live, staged in plan: + with open(staged, encoding="utf-8") as f: + proposed = f.read() + original = None + backup_path = "" + if os.path.exists(live): + with open(live, "rb") as f: + original = f.read() + skill_backup = os.path.join(backup_dir, name) + os.makedirs(skill_backup, exist_ok=True) + backup_path = os.path.join(skill_backup, os.path.basename(live)) + shutil.copy2(live, backup_path) + before = hashlib.sha256(original).hexdigest() if original is not None else "" + _write_atomic(live, proposed) + done.append((live, original)) + receipts.append(AdoptedSkill( + skill_name=name, live_skill_path=live, sha256_before=before, + sha256_after=_sha256_text(proposed), backup_path=backup_path, + )) + except BaseException: + for live, original in reversed(done): + if original is None: + if os.path.exists(live): + os.unlink(live) + else: + with open(live, "wb") as f: + f.write(original) + raise + + _write_atomic( + os.path.join(staging_dir, "adopted_skills.json"), + json.dumps([r.__dict__ for r in receipts], ensure_ascii=False, indent=2), + ) + return receipts + + def _backup(path: str, backup_dir: str) -> None: if os.path.exists(path): os.makedirs(backup_dir, exist_ok=True) diff --git a/tests/test_sleep_adopt_skill_subset.py b/tests/test_sleep_adopt_skill_subset.py new file mode 100644 index 00000000..86247386 --- /dev/null +++ b/tests/test_sleep_adopt_skill_subset.py @@ -0,0 +1,239 @@ +"""Tests for explicit multi-skill subset adoption (issue #120). + +Pure-stdlib (unittest), hermetic (tmpdir only), no API key, no network. +Run: python -m pytest tests/test_sleep_adopt_skill_subset.py +""" +from __future__ import annotations + +import hashlib +import json +import os +import stat +import tempfile +import unittest + +from skillopt_sleep.staging import ( + SkillProposal, + StagingError, + adopt_skills, + staged_skills, + write_staging, +) +from skillopt_sleep.types import SleepReport + + +def _sha(text): + return hashlib.sha256(text.encode("utf-8")).hexdigest() + + +def _read(path): + with open(path, encoding="utf-8") as f: + return f.read() + + +def _write(path, text): + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "w", encoding="utf-8") as f: + f.write(text) + + +class TwoSkillNight: + """End-to-end fixture: a staged night with two per-skill proposals.""" + + def __init__(self, tmp): + self.tmp = tmp + self.live_root = os.path.join(tmp, "live") + self.alpha_live = os.path.join(self.live_root, "alpha", "SKILL.md") + self.beta_live = os.path.join(self.live_root, "beta", "SKILL.md") + _write(self.alpha_live, "# alpha v1\n") + _write(self.beta_live, "# beta v1\n") + self.staging = write_staging( + tmp, + report=SleepReport(night=1, project=tmp, accepted=True), + proposed_skill=None, proposed_memory=None, + live_skill_path=self.alpha_live, + live_memory_path=os.path.join(self.live_root, "CLAUDE.md"), + report_md="# report\n", + skill_proposals=[ + SkillProposal("alpha", "# alpha v2\n", self.alpha_live), + SkillProposal("beta", "# beta v2\n", self.beta_live), + ], + ) + + +class TestStagedSkills(unittest.TestCase): + def test_rows_are_readable_from_the_manifest(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + rows = staged_skills(night.staging) + self.assertEqual([r["skill_name"] for r in rows], ["alpha", "beta"]) + + def test_legacy_single_proposal_night_has_no_staged_skills(self): + with tempfile.TemporaryDirectory() as tmp: + out = write_staging( + tmp, report=SleepReport(night=1, project=tmp), proposed_skill="# s\n", + proposed_memory=None, + live_skill_path=os.path.join(tmp, "live", "SKILL.md"), + live_memory_path=os.path.join(tmp, "live", "CLAUDE.md"), + report_md="# report\n", + ) + self.assertEqual(staged_skills(out), []) + self.assertEqual(adopt_skills(out), []) + + def test_malformed_skills_manifest_shape_is_refused(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + manifest_path = os.path.join(night.staging, "manifest.json") + for malformed in ({"not": "a list"}, [{"skill_name": "alpha"}, "bad"]): + with open(manifest_path, encoding="utf-8") as f: + manifest = json.load(f) + manifest["skills"] = malformed + with open(manifest_path, "w", encoding="utf-8") as f: + json.dump(manifest, f) + with self.assertRaises(StagingError, msg=repr(malformed)): + staged_skills(night.staging) + + +class TestAdoptSkillSubset(unittest.TestCase): + def test_adopting_one_skill_leaves_the_other_untouched(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + receipts = adopt_skills(night.staging, ["alpha"]) + self.assertEqual([r.skill_name for r in receipts], ["alpha"]) + self.assertEqual(_read(night.alpha_live), "# alpha v2\n") + self.assertEqual(_read(night.beta_live), "# beta v1\n") + + def test_receipts_carry_before_and_after_hashes(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + receipt = adopt_skills(night.staging, ["alpha"])[0] + self.assertEqual(receipt.sha256_before, _sha("# alpha v1\n")) + self.assertEqual(receipt.sha256_after, _sha("# alpha v2\n")) + self.assertEqual(receipt.live_skill_path, night.alpha_live) + self.assertEqual(_read(receipt.backup_path), "# alpha v1\n") + + def test_receipts_are_persisted_beside_the_report(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + adopt_skills(night.staging, ["beta"]) + with open(os.path.join(night.staging, "adopted_skills.json"), + encoding="utf-8") as f: + rows = json.load(f) + self.assertEqual([r["skill_name"] for r in rows], ["beta"]) + self.assertEqual(rows[0]["sha256_after"], _sha("# beta v2\n")) + + def test_selecting_no_skills_adopts_nothing(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + self.assertEqual(adopt_skills(night.staging, []), []) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + self.assertEqual(_read(night.beta_live), "# beta v1\n") + self.assertFalse( + os.path.exists(os.path.join(night.staging, "adopted_skills.json"))) + + def test_selecting_every_skill_adopts_all_of_them(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + receipts = adopt_skills(night.staging) + self.assertEqual([r.skill_name for r in receipts], ["alpha", "beta"]) + self.assertEqual(_read(night.alpha_live), "# alpha v2\n") + self.assertEqual(_read(night.beta_live), "# beta v2\n") + + def test_a_new_live_file_reports_an_empty_before_hash(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + os.unlink(night.beta_live) + receipt = [r for r in adopt_skills(night.staging) if r.skill_name == "beta"][0] + self.assertEqual(receipt.sha256_before, "") + self.assertEqual(receipt.backup_path, "") + self.assertEqual(_read(night.beta_live), "# beta v2\n") + + def test_unknown_or_repeated_selection_is_refused_without_writing(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + for selection in (["gamma"], ["alpha", "gamma"], ["alpha", "alpha"]): + with self.assertRaises(StagingError, msg=str(selection)): + adopt_skills(night.staging, selection) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + self.assertEqual(_read(night.beta_live), "# beta v1\n") + + def test_missing_staged_proposal_is_refused_without_writing(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + os.unlink(os.path.join(night.staging, "proposed_SKILL.beta.md")) + with self.assertRaises(StagingError): + adopt_skills(night.staging) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + + def test_unsafe_manifest_row_is_refused_without_writing(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + manifest_path = os.path.join(night.staging, "manifest.json") + with open(manifest_path, encoding="utf-8") as f: + manifest = json.load(f) + manifest["skills"][1]["live_skill_path"] = "relative/SKILL.md" + with open(manifest_path, "w", encoding="utf-8") as f: + json.dump(manifest, f) + with self.assertRaises(StagingError): + adopt_skills(night.staging) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + + def test_manifest_proposal_filename_cannot_escape_staging(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + outside = os.path.join(tmp, "outside.md") + _write(outside, "# not a staged proposal\n") + manifest_path = os.path.join(night.staging, "manifest.json") + with open(manifest_path, encoding="utf-8") as f: + manifest = json.load(f) + manifest["skills"][0]["proposed_file"] = os.path.relpath( + outside, night.staging + ) + with open(manifest_path, "w", encoding="utf-8") as f: + json.dump(manifest, f) + with self.assertRaises(StagingError): + adopt_skills(night.staging, ["alpha"]) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + + def test_adoption_preserves_existing_live_file_mode(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + os.chmod(night.alpha_live, 0o640) + adopt_skills(night.staging, ["alpha"]) + self.assertEqual(stat.S_IMODE(os.stat(night.alpha_live).st_mode), 0o640) + + def test_a_failed_write_rolls_the_whole_selection_back(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + # beta's live path becomes un-writable: its parent is now a file. + os.unlink(night.beta_live) + os.rmdir(os.path.dirname(night.beta_live)) + _write(os.path.dirname(night.beta_live), "not a directory\n") + with self.assertRaises(OSError): + adopt_skills(night.staging) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + self.assertFalse( + os.path.exists(os.path.join(night.staging, "adopted_skills.json"))) + + def test_rollback_removes_files_that_did_not_exist_before(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + os.unlink(night.alpha_live) + os.unlink(night.beta_live) + os.rmdir(os.path.dirname(night.beta_live)) + _write(os.path.dirname(night.beta_live), "not a directory\n") + with self.assertRaises(OSError): + adopt_skills(night.staging) + self.assertFalse(os.path.exists(night.alpha_live)) + + def test_adoption_never_happens_without_an_explicit_call(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + self.assertEqual(_read(night.beta_live), "# beta v1\n") + self.assertTrue(os.path.exists( + os.path.join(night.staging, "proposed_SKILL.alpha.md"))) + + +if __name__ == "__main__": + unittest.main() From e9d8c9303c89ec8d10799263cf31fbe38225e958 Mon Sep 17 00:00:00 2001 From: "Bogdan (Dan) Baciu" Date: Wed, 12 Aug 2026 20:04:05 +0200 Subject: [PATCH 2/4] fix(sleep): wire cycle staging and adopt-time review checks 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/SkillOpt#212 --- docs/reference/cli.md | 5 + docs/sleep/README.md | 9 +- docs/sleep/multi-skill-staging.md | 60 +++++++++-- skillopt_sleep/__main__.py | 70 +++++++++++- skillopt_sleep/cycle.py | 46 +++++++- skillopt_sleep/staging.py | 89 +++++++++++++--- tests/test_sleep_adopt_skill_subset.py | 141 +++++++++++++++++++++++++ 7 files changed, 385 insertions(+), 35 deletions(-) diff --git a/docs/reference/cli.md b/docs/reference/cli.md index ae234146..1b798bff 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -146,6 +146,11 @@ Actions are `run`, `dry-run`, `status`, `adopt`, `harvest`, `schedule`, and | `--progress` / `--json` | Progress or machine-readable output | | `--auto-adopt` | Apply an accepted staged proposal automatically | +`adopt` also accepts `--skill NAME` (repeatable) and `--all-skills` for a night +that staged per-skill proposals. Bare `adopt` on that night lists the names and +exits instead of promoting every skill. See +[multi-skill staging](../sleep/multi-skill-staging.md). + The `mock` and `handoff` backends make no network calls. A real backend sends mining, replay, judging, and reflection prompts derived from harvested transcripts and tasks to its selected provider. Review that provider's diff --git a/docs/sleep/README.md b/docs/sleep/README.md index 7f5be874..3a8953d4 100644 --- a/docs/sleep/README.md +++ b/docs/sleep/README.md @@ -84,6 +84,7 @@ skillopt-sleep dry-run # harvest + mine + replay, report only; stages nothi skillopt-sleep run # a full nightly cycle; the proposal is staged for review skillopt-sleep status # show state + the latest staged proposal skillopt-sleep adopt # apply the latest staged proposal +skillopt-sleep adopt --skill NAME # adopt one staged skill (repeatable) skillopt-sleep schedule # install a nightly cron entry for this project ``` @@ -299,9 +300,11 @@ gate keeps the worst case bounded; keep it **on** by default. ## Learn more -Staging a proposal for more than one skill, and adopting a reviewed subset of -them with backups and hash receipts, is documented in -[`docs/sleep/multi-skill-staging.md`](multi-skill-staging.md). +The **low-level** API for staging one proposal per skill and adopting a reviewed +subset (`staged_skills` / `adopt_skills`, plus `status` and `adopt --skill`) is +documented in [`docs/sleep/multi-skill-staging.md`](multi-skill-staging.md). +That page also states what this slice does **not** yet do: an end-to-end +nightly workflow where each group edits its own live `SKILL.md`. See the [SkillOpt documentation index](../index.md), the [CLI reference](../reference/cli.md), and the integration-specific READMEs under diff --git a/docs/sleep/multi-skill-staging.md b/docs/sleep/multi-skill-staging.md index 2f29e2fb..2d5bfa35 100644 --- a/docs/sleep/multi-skill-staging.md +++ b/docs/sleep/multi-skill-staging.md @@ -1,14 +1,37 @@ # Multi-skill staging and subset adoption -A night can stage a proposal for more than one skill. Adoption stays explicit: -staging only ever writes into the staging directory, and `adopt_skills()` copies -a **reviewed subset** over the live files, with a backup and a hash receipt per -skill. +There are two layers here. Do not collapse them. + +1. **Low-level adoption API** — `staged_skills()` / `adopt_skills()`, plus + `skillopt-sleep status` and `skillopt-sleep adopt --skill`. This slice is + complete: a night can stage one proposal file per resolved skill, a reviewer + can list those names, and an explicit subset is copied over the live files + with a backup and a hash receipt. +2. **End-to-end multi-skill nightly workflow** — each hinted group loading and + editing *its own* live `SKILL.md`, then promoting that file without a human + picking names. That workflow is **not** this slice. `multi_skill_report` + still consolidates every group from the **managed** skill document; staging + only *targets* the resolved live path when the name is `FOUND` and unique. Nothing here changes a single-managed-skill night. If a night stages no per-skill proposals, the staging directory and `manifest.json` are exactly the legacy ones and `skillopt-sleep adopt` keeps working unchanged. +## Nightly wiring (`run_sleep_cycle`) + +When `multi_skill_report` is on and hinted groups pass the gate: + +- the managed catch-all is **not** staged as a per-skill proposal (it stays on + `proposed_SKILL.md`); +- each accepted group name is resolved with `resolve_skill` against + `skill_search_roots(cfg)`; +- only `FOUND` unique live paths become `SkillProposal` rows; +- missing, ambiguous, rejected, or colliding names are skipped rather than + aborting the night. + +Review remains explicit. `auto_adopt` still only runs the legacy `adopt()` +pair; it never silently promotes every staged skill. + ## Staging layout Legacy (single managed skill) — unchanged: @@ -59,6 +82,8 @@ file. A refused fan-out writes no `manifest.json`, so the folder is not adoptabl ## Adopting a reviewed subset +Low-level API: + ```python from skillopt_sleep.staging import adopt_skills, latest_staging, staged_skills @@ -69,12 +94,31 @@ receipts = adopt_skills(staging, ["alpha"]) # beta is left alone receipts[0].sha256_before, receipts[0].sha256_after ``` +CLI: + +```text +python -m skillopt_sleep status --project PATH +python -m skillopt_sleep adopt --project PATH --skill alpha +python -m skillopt_sleep adopt --project PATH --skill alpha --skill beta +python -m skillopt_sleep adopt --project PATH --all-skills +``` + +On a multi-skill night, bare `adopt` does **not** silently promote every staged +skill. It lists the names and asks for `--skill` or `--all-skills`. Legacy +nights (no `skills` in the manifest) still use `adopt()` unchanged. + - `skill_names=None` adopts every staged skill; `[]` adopts nothing. -- An unknown or repeated name, an unsafe manifest row, or a missing proposal file - raises `StagingError` **before** anything is written. +- An unknown or repeated name, an unsafe manifest row, a missing proposal file, + or a uniqueness / live-target collision raises `StagingError` **before** + anything is written. +- Uniqueness and live-target nonexistence are re-checked **at adoption time**, + not only at staging, so a tampered manifest that points two skills at one + file (including via casefold or realpath/symlink) is refused with no writes. + A live path that exists as something other than a file is also refused. - Each live file is backed up to `backup/skills//` and written atomically. -- If any write fails, every file in the selection is restored (and files that did - not exist before are removed), so a partial adoption never survives. +- If any write fails — including `adopted_skills.json` — every live file in the + selection is restored (and files that did not exist before are removed), so a + partial adoption never survives. - Receipts (`skill_name`, `live_skill_path`, `sha256_before`, `sha256_after`, `backup_path`) are returned and written to `adopted_skills.json` in the staging directory. An empty `sha256_before` means the skill had no live file yet. diff --git a/skillopt_sleep/__main__.py b/skillopt_sleep/__main__.py index 416922c3..370555ab 100644 --- a/skillopt_sleep/__main__.py +++ b/skillopt_sleep/__main__.py @@ -4,6 +4,7 @@ python -m skillopt_sleep dry-run # same but report only, no staging/adopt python -m skillopt_sleep status # show state + latest staged proposal python -m skillopt_sleep adopt # apply the latest staged proposal (with backup) + python -m skillopt_sleep adopt --skill NAME # adopt one staged skill (repeatable) python -m skillopt_sleep harvest # just print what would be mined (debug) Common flags: @@ -35,8 +36,8 @@ from skillopt_sleep.cycle import run_sleep_cycle from skillopt_sleep.harvest_sources import harvest_for_config from skillopt_sleep.mine import mine -from skillopt_sleep.staging import adopt as adopt_staging -from skillopt_sleep.staging import latest_staging +from skillopt_sleep.staging import StagingError, adopt as adopt_staging +from skillopt_sleep.staging import adopt_skills, latest_staging, staged_skills from skillopt_sleep.state import SleepState from skillopt_sleep.tasks_file import load_tasks_file, make_tasks_payload, write_tasks_file @@ -222,7 +223,17 @@ def _print_run_report(outcome, args, task_meta: Dict[str, Any]) -> None: if outcome.staging_dir: print(f"[sleep] staged: {outcome.staging_dir}") if not outcome.adopted: - print("[sleep] review it, then: python -m skillopt_sleep adopt") + names = [] + try: + names = [r["skill_name"] for r in staged_skills(outcome.staging_dir)] + except Exception: + names = [] + if names: + listed = " ".join(f"--skill {n}" for n in names) + print("[sleep] review it, then adopt a subset:") + print(f" python -m skillopt_sleep adopt {listed}") + else: + print("[sleep] review it, then: python -m skillopt_sleep adopt") if outcome.adopted: print(f"[sleep] auto-adopted: {', '.join(outcome.adopted_paths)}") @@ -414,6 +425,12 @@ def cmd_status(args) -> int: state = SleepState.load(cfg.state_path) project = cfg.get("invoked_project") or os.getcwd() latest = latest_staging(project) + skills = [] + if latest: + try: + skills = staged_skills(latest) + except Exception: + skills = [] info = { "night": state.night, "state_path": cfg.state_path, @@ -421,6 +438,7 @@ def cmd_status(args) -> int: "history_tail": state.data.get("history", [])[-5:], "latest_staging": latest, "slow_memory_chars": len(state.slow_memory), + "staged_skills": [r.get("skill_name", "") for r in skills], } if args.json: print(json.dumps(info, ensure_ascii=False, indent=2)) @@ -429,6 +447,10 @@ def cmd_status(args) -> int: print(f"[sleep] project: {project}") if latest: print(f"[sleep] latest staged proposal: {latest}") + if skills: + print("[sleep] staged skills:") + for row in skills: + print(f" {row.get('skill_name', '')} -> {row.get('live_skill_path', '')}") rp = os.path.join(latest, "report.md") if os.path.exists(rp): with open(rp) as f: @@ -445,6 +467,40 @@ def cmd_adopt(args) -> int: if not target or not os.path.isdir(target): print("[sleep] nothing to adopt (no staging dir).") return 1 + selected = list(getattr(args, "skills", None) or []) + adopt_all = bool(getattr(args, "all_skills", False)) + if selected and adopt_all: + print("[sleep] use --skill or --all-skills, not both.") + return 2 + try: + rows = staged_skills(target) + except Exception as exc: + print(f"[sleep] cannot read staged skills: {exc}") + return 1 + if selected or adopt_all: + if not rows: + print("[sleep] this night has no per-skill proposals; omit --skill to adopt the legacy pair.") + return 2 + names = None if adopt_all else selected + try: + receipts = adopt_skills(target, names) + except StagingError as exc: + print(f"[sleep] adopt refused: {exc}") + return 2 + except OSError as exc: + print(f"[sleep] adopt failed: {exc}") + return 1 + print(f"[sleep] adopted from {target}") + for receipt in receipts: + print(f" -> {receipt.skill_name}: {receipt.live_skill_path}") + if not receipts: + print("[sleep] (no skills in the selection)") + return 0 + if rows: + print("[sleep] this night staged per-skill proposals; pass --skill NAME or --all-skills.") + for row in rows: + print(f" {row.get('skill_name', '')} -> {row.get('live_skill_path', '')}") + return 2 updated = adopt_staging(target) print(f"[sleep] adopted from {target}") for p in updated: @@ -535,6 +591,14 @@ def main(argv=None) -> int: p_adopt = sub.add_parser("adopt", help="apply latest staged proposal") _add_common(p_adopt) p_adopt.add_argument("--staging", default="", help="specific staging dir") + p_adopt.add_argument( + "--skill", action="append", default=[], dest="skills", + help="adopt this staged skill (repeatable)", + ) + p_adopt.add_argument( + "--all-skills", action="store_true", dest="all_skills", + help="adopt every staged per-skill proposal", + ) p_harvest = sub.add_parser("harvest", help="debug: show mined tasks") _add_common(p_harvest) p_harvest.add_argument("--output", default="", help="write mined tasks JSON for review") diff --git a/skillopt_sleep/cycle.py b/skillopt_sleep/cycle.py index afa9c101..937d28f3 100644 --- a/skillopt_sleep/cycle.py +++ b/skillopt_sleep/cycle.py @@ -25,9 +25,12 @@ from skillopt_sleep.mine import group_tasks_by_skill_hint, mine from skillopt_sleep.multi_skill import ( SkillGroup, + accepted_group_skills, consolidate_groups, skill_group_reports, ) +from skillopt_sleep.skill_resolver import resolve_skill, skill_search_roots +from skillopt_sleep.staging import SkillProposal, StagingError, skill_proposal_rows from skillopt_sleep.staging import adopt as adopt_staging from skillopt_sleep.staging import redact_secrets from skillopt_sleep.staging import write_staging @@ -274,6 +277,36 @@ def _render_report_md(report: SleepReport, cfg: SleepConfig) -> str: return "\n".join(lines) +def _skill_proposals_from_groups( + cfg: SleepConfig, + group_outcomes: dict, + managed_name: str, +) -> List[SkillProposal]: + """Stage per-skill proposals for accepted groups whose names resolve uniquely. + + Groups still consolidate from the managed document; this only chooses the + live ``SKILL.md`` each accepted name would replace. Unresolved, ambiguous, + rejected, or colliding names are skipped so one bad hint cannot abort the + night. The managed catch-all is never staged here — it stays on the legacy + ``proposed_SKILL.md`` path. + """ + roots = skill_search_roots(cfg) + proposals: List[SkillProposal] = [] + for name, new_skill in accepted_group_skills(group_outcomes).items(): + if name == managed_name: + continue + resolution = resolve_skill(name, roots) + if not resolution.ok: + continue + candidate = SkillProposal(name, new_skill, resolution.path) + try: + skill_proposal_rows(proposals + [candidate]) + except StagingError: + continue + proposals.append(candidate) + return proposals + + def run_sleep_cycle( cfg: Optional[SleepConfig] = None, *, @@ -532,12 +565,13 @@ def run_sleep_cycle( # automatic; a night whose evidence produces only the catch-all group adds # no rows and no calls. # - # Each group currently starts from the same managed document. Resolving a - # hinted group to its own live SKILL.md is the resolver's job and is not - # wired here yet, so a row describes what that group's evidence did to the - # managed skill, not to a separate file. + # Each group currently starts from the same managed document. Staging + # targets the resolved live SKILL.md when the name is FOUND and unique; + # the row still describes what that group's evidence did to the managed + # skill, not a separately loaded live file. + group_outcomes = {} + managed_name = cfg.get("managed_skill_name", "skillopt-sleep-learned") if cfg.get("multi_skill_report", False): - managed_name = cfg.get("managed_skill_name", "skillopt-sleep-learned") grouped = group_tasks_by_skill_hint(tasks, managed_name) only_catch_all = len(grouped) == 1 and managed_name in grouped if grouped and not only_catch_all: @@ -574,6 +608,7 @@ def run_sleep_cycle( report_md = _render_report_md(report, cfg) proposed_skill = result.new_skill if (cfg.get("evolve_skill") and result.accepted) else None proposed_memory = result.new_memory if (cfg.get("evolve_memory") and result.accepted) else None + skill_proposals = _skill_proposals_from_groups(cfg, group_outcomes, managed_name) staging_dir = write_staging( project, report=report, @@ -583,6 +618,7 @@ def run_sleep_cycle( live_memory_path=live_memory_path, report_md=report_md, out_dir=staging_dir_pre, + skill_proposals=skill_proposals, ) if ev is not None: ev.log("stage", "staged", staging_dir=staging_dir, diff --git a/skillopt_sleep/staging.py b/skillopt_sleep/staging.py index d4e4d63a..a8bdcd77 100644 --- a/skillopt_sleep/staging.py +++ b/skillopt_sleep/staging.py @@ -553,6 +553,57 @@ def _selected_rows( return [row for row in rows if str(row.get("skill_name", "")) in chosen] +def _revalidate_selected_skill_rows(rows: Sequence[Dict[str, Any]]) -> None: + """Re-run uniqueness and live-target checks at adoption time. + + Staging already refused collisions, but the manifest can be edited between + staging and adopt. A tampered pair that shares a skill name, a staged + filename, or a live target must fail here with no writes. Live paths are + also compared by realpath so a symlink cannot hide a second claim on one + file, and a live path that exists as something other than a file is + refused rather than overwritten. + """ + skill_proposal_rows([ + SkillProposal( + str(row.get("skill_name") or ""), + "", + str(row.get("live_skill_path") or ""), + ) + for row in rows + ]) + seen_real: Dict[str, str] = {} + for row in rows: + name = _safe_skill_name(row.get("skill_name")) + live = _safe_live_path(row.get("live_skill_path")) + if not name or not live: + continue + try: + real = os.path.realpath(live) + except OSError: + real = live + key = real.casefold() + if key in seen_real: + raise StagingError( + f"skills {seen_real[key]!r} and {name!r} target the same file: {live}" + ) + seen_real[key] = name + if os.path.lexists(live) and not os.path.isfile(live): + raise StagingError( + f"live skill path for {name!r} exists and is not a file: {live}" + ) + + +def _restore_live_writes(done: Sequence[tuple]) -> None: + """Restore live files written by a failed adoption, newest first.""" + for live, original in reversed(done): + if original is None: + if os.path.exists(live): + os.unlink(live) + else: + with open(live, "wb") as f: + f.write(original) + + def adopt_skills( staging_dir: str, skill_names: Optional[Sequence[str]] = None ) -> List[AdoptedSkill]: @@ -562,14 +613,17 @@ def adopt_skills( staged skill. Nothing is adopted implicitly and skills outside the selection are never touched. - Every selected proposal is validated first, each live file is backed up, and - the writes are rolled back as a set if any one of them fails, so a partial - adoption never survives. Returns a before/after sha256 receipt per skill and - also writes them to ``adopted_skills.json`` in the staging directory. + Every selected proposal is validated first, including a second uniqueness + and live-target check against the current manifest and filesystem. Each + live file is backed up, and the writes — including ``adopted_skills.json`` + — are rolled back as a set if any one of them fails, so a partial adoption + never survives. Returns a before/after sha256 receipt per skill and also + writes them to ``adopted_skills.json`` in the staging directory. """ rows = _selected_rows(staged_skills(staging_dir), skill_names) if not rows: return [] + _revalidate_selected_skill_rows(rows) plan: List[tuple] = [] for row in rows: @@ -596,6 +650,11 @@ def adopt_skills( backup_dir = os.path.join(staging_dir, "backup", "skills") receipts: List[AdoptedSkill] = [] done: List[tuple] = [] # (live, original_bytes or None) for rollback + receipt_path = os.path.join(staging_dir, "adopted_skills.json") + receipt_original = None + if os.path.isfile(receipt_path): + with open(receipt_path, "rb") as f: + receipt_original = f.read() try: for name, live, staged in plan: with open(staged, encoding="utf-8") as f: @@ -616,20 +675,18 @@ def adopt_skills( skill_name=name, live_skill_path=live, sha256_before=before, sha256_after=_sha256_text(proposed), backup_path=backup_path, )) + _write_atomic( + receipt_path, + json.dumps([r.__dict__ for r in receipts], ensure_ascii=False, indent=2), + ) except BaseException: - for live, original in reversed(done): - if original is None: - if os.path.exists(live): - os.unlink(live) - else: - with open(live, "wb") as f: - f.write(original) + _restore_live_writes(done) + if receipt_original is not None: + with open(receipt_path, "wb") as f: + f.write(receipt_original) + elif os.path.isfile(receipt_path): + os.unlink(receipt_path) raise - - _write_atomic( - os.path.join(staging_dir, "adopted_skills.json"), - json.dumps([r.__dict__ for r in receipts], ensure_ascii=False, indent=2), - ) return receipts diff --git a/tests/test_sleep_adopt_skill_subset.py b/tests/test_sleep_adopt_skill_subset.py index 86247386..73dc6fb2 100644 --- a/tests/test_sleep_adopt_skill_subset.py +++ b/tests/test_sleep_adopt_skill_subset.py @@ -234,6 +234,147 @@ def test_adoption_never_happens_without_an_explicit_call(self): self.assertTrue(os.path.exists( os.path.join(night.staging, "proposed_SKILL.alpha.md"))) + def test_tampered_duplicate_live_paths_are_refused_at_adopt(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + manifest_path = os.path.join(night.staging, "manifest.json") + with open(manifest_path, encoding="utf-8") as f: + manifest = json.load(f) + manifest["skills"][1]["live_skill_path"] = night.alpha_live + with open(manifest_path, "w", encoding="utf-8") as f: + json.dump(manifest, f) + with self.assertRaises(StagingError): + adopt_skills(night.staging) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + self.assertEqual(_read(night.beta_live), "# beta v1\n") + self.assertFalse( + os.path.exists(os.path.join(night.staging, "adopted_skills.json"))) + + def test_live_target_that_is_not_a_file_is_refused_without_writing(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + os.unlink(night.beta_live) + os.mkdir(night.beta_live) + with self.assertRaises(StagingError): + adopt_skills(night.staging, ["beta"]) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + self.assertTrue(os.path.isdir(night.beta_live)) + + def test_receipt_write_failure_rolls_back_live_files(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + os.makedirs(os.path.join(night.staging, "adopted_skills.json")) + with self.assertRaises(OSError): + adopt_skills(night.staging, ["alpha"]) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + self.assertEqual(_read(night.beta_live), "# beta v1\n") + + +class TestCycleStagesResolvedSkillSubset(unittest.TestCase): + """run_sleep_cycle stages resolved skills; adopt promotes only the subset.""" + + def _hinted_tasks(self): + from dataclasses import replace + + from skillopt_sleep.experiments.personas import programmer_persona, researcher_persona + from skillopt_sleep.mine import assign_splits + + research = assign_splits(researcher_persona(), holdout_fraction=0.34, seed=42) + programming = assign_splits(programmer_persona(), holdout_fraction=0.34, seed=1) + tagged = [replace(t, skill_hint="research-skill") for t in research] + tagged += [replace(t, id=f"prog-{t.id}", skill_hint="programming-skill") + for t in programming] + return tagged + + def test_cycle_stages_both_skills_and_subset_adopt_touches_only_one(self): + from skillopt_sleep.config import load_config + from skillopt_sleep.cycle import run_sleep_cycle + + with tempfile.TemporaryDirectory() as proj, tempfile.TemporaryDirectory() as home: + claude_home = os.path.join(home, ".claude") + research_live = os.path.join( + claude_home, "skills", "research-skill", "SKILL.md") + programming_live = os.path.join( + claude_home, "skills", "programming-skill", "SKILL.md") + _write(research_live, "# research-skill v1\n") + _write(programming_live, "# programming-skill v1\n") + cfg = load_config( + invoked_project=proj, projects="invoked", backend="mock", + claude_home=claude_home, + managed_skill_name="skillopt-sleep-learned", auto_adopt=False, + multi_skill_report=True, + ) + outcome = run_sleep_cycle(cfg, seed_tasks=self._hinted_tasks()) + rows = staged_skills(outcome.staging_dir) + names = [r["skill_name"] for r in rows] + self.assertIn("research-skill", names) + self.assertIn("programming-skill", names) + self.assertTrue(os.path.isfile(os.path.join( + outcome.staging_dir, "proposed_SKILL.research-skill.md"))) + self.assertTrue(os.path.isfile(os.path.join( + outcome.staging_dir, "proposed_SKILL.programming-skill.md"))) + self.assertEqual(_read(research_live), "# research-skill v1\n") + self.assertEqual(_read(programming_live), "# programming-skill v1\n") + + receipts = adopt_skills(outcome.staging_dir, ["research-skill"]) + self.assertEqual([r.skill_name for r in receipts], ["research-skill"]) + self.assertNotEqual(_read(research_live), "# research-skill v1\n") + self.assertEqual(_read(programming_live), "# programming-skill v1\n") + + +class TestAdoptSkillCli(unittest.TestCase): + def _cli(self, argv): + import contextlib + import io + + from skillopt_sleep.__main__ import main + + stdout = io.StringIO() + with contextlib.redirect_stdout(stdout): + rc = main(argv) + return rc, stdout.getvalue() + + def test_status_lists_staged_skill_names(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + claude_home = os.path.join(tmp, ".claude") + os.makedirs(claude_home, exist_ok=True) + rc, out = self._cli([ + "status", "--project", tmp, "--claude-home", claude_home, "--json", + ]) + self.assertEqual(rc, 0) + payload = json.loads(out) + self.assertEqual(payload["staged_skills"], ["alpha", "beta"]) + self.assertEqual(payload["latest_staging"], night.staging) + + def test_bare_adopt_on_a_multi_skill_night_lists_and_refuses(self): + with tempfile.TemporaryDirectory() as tmp: + TwoSkillNight(tmp) + claude_home = os.path.join(tmp, ".claude") + os.makedirs(claude_home, exist_ok=True) + rc, out = self._cli([ + "adopt", "--project", tmp, "--claude-home", claude_home, + ]) + self.assertEqual(rc, 2) + self.assertIn("--skill", out) + self.assertIn("alpha", out) + self.assertIn("beta", out) + self.assertEqual(_read(os.path.join(tmp, "live", "alpha", "SKILL.md")), + "# alpha v1\n") + + def test_adopt_skill_flag_promotes_only_the_named_skill(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + claude_home = os.path.join(tmp, ".claude") + os.makedirs(claude_home, exist_ok=True) + rc, out = self._cli([ + "adopt", "--project", tmp, "--claude-home", claude_home, + "--skill", "alpha", + ]) + self.assertEqual(rc, 0, out) + self.assertEqual(_read(night.alpha_live), "# alpha v2\n") + self.assertEqual(_read(night.beta_live), "# beta v1\n") + if __name__ == "__main__": unittest.main() From de1e3986ea4a3a6cae20571016a9ef48b309af65 Mon Sep 17 00:00:00 2001 From: "Bogdan (Dan) Baciu" Date: Wed, 12 Aug 2026 20:15:06 +0200 Subject: [PATCH 3/4] test(sleep): mega-cover PR 212 review paths 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/SkillOpt#120 --- skillopt_sleep/__main__.py | 6 +- skillopt_sleep/cycle.py | 6 +- tests/test_sleep_adopt_skill_subset.py | 297 +++++++++++++++++++++++++ 3 files changed, 302 insertions(+), 7 deletions(-) diff --git a/skillopt_sleep/__main__.py b/skillopt_sleep/__main__.py index 370555ab..098f2db3 100644 --- a/skillopt_sleep/__main__.py +++ b/skillopt_sleep/__main__.py @@ -36,8 +36,8 @@ from skillopt_sleep.cycle import run_sleep_cycle from skillopt_sleep.harvest_sources import harvest_for_config from skillopt_sleep.mine import mine -from skillopt_sleep.staging import StagingError, adopt as adopt_staging -from skillopt_sleep.staging import adopt_skills, latest_staging, staged_skills +from skillopt_sleep.staging import StagingError, adopt_skills, latest_staging, staged_skills +from skillopt_sleep.staging import adopt as adopt_staging from skillopt_sleep.state import SleepState from skillopt_sleep.tasks_file import load_tasks_file, make_tasks_payload, write_tasks_file @@ -554,7 +554,7 @@ def cmd_harvest(args) -> int: def cmd_schedule(args) -> int: - from skillopt_sleep.scheduler import schedule, list_scheduled + from skillopt_sleep.scheduler import list_scheduled, schedule cfg = _cfg_from_args(args) project = cfg.get("invoked_project") or os.getcwd() ok, msg = schedule(project, backend=cfg.get("backend", "mock"), diff --git a/skillopt_sleep/cycle.py b/skillopt_sleep/cycle.py index 937d28f3..2dd00aef 100644 --- a/skillopt_sleep/cycle.py +++ b/skillopt_sleep/cycle.py @@ -17,9 +17,9 @@ from skillopt_sleep import evidence from skillopt_sleep.backend import Backend, CursorBackendError, build_backend -from skillopt_sleep.evidence import EvidenceLog from skillopt_sleep.config import SleepConfig, load_config from skillopt_sleep.dream import dream_consolidate +from skillopt_sleep.evidence import EvidenceLog from skillopt_sleep.harvest_sources import harvest_for_config from skillopt_sleep.memory import ensure_skill_scaffold from skillopt_sleep.mine import group_tasks_by_skill_hint, mine @@ -30,10 +30,8 @@ skill_group_reports, ) from skillopt_sleep.skill_resolver import resolve_skill, skill_search_roots -from skillopt_sleep.staging import SkillProposal, StagingError, skill_proposal_rows +from skillopt_sleep.staging import SkillProposal, StagingError, redact_secrets, skill_proposal_rows, write_staging from skillopt_sleep.staging import adopt as adopt_staging -from skillopt_sleep.staging import redact_secrets -from skillopt_sleep.staging import write_staging from skillopt_sleep.state import SleepState, _now_iso from skillopt_sleep.types import SessionDigest, SleepReport, TaskRecord diff --git a/tests/test_sleep_adopt_skill_subset.py b/tests/test_sleep_adopt_skill_subset.py index 73dc6fb2..492cdc75 100644 --- a/tests/test_sleep_adopt_skill_subset.py +++ b/tests/test_sleep_adopt_skill_subset.py @@ -11,6 +11,7 @@ import stat import tempfile import unittest +from unittest import mock from skillopt_sleep.staging import ( SkillProposal, @@ -375,6 +376,302 @@ def test_adopt_skill_flag_promotes_only_the_named_skill(self): self.assertEqual(_read(night.alpha_live), "# alpha v2\n") self.assertEqual(_read(night.beta_live), "# beta v1\n") + def test_all_skills_flag_promotes_every_staged_skill(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + claude_home = os.path.join(tmp, ".claude") + os.makedirs(claude_home, exist_ok=True) + rc, out = self._cli([ + "adopt", "--project", tmp, "--claude-home", claude_home, + "--all-skills", + ]) + self.assertEqual(rc, 0, out) + self.assertEqual(_read(night.alpha_live), "# alpha v2\n") + self.assertEqual(_read(night.beta_live), "# beta v2\n") + + def test_repeated_skill_flags_adopt_the_named_pair(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + claude_home = os.path.join(tmp, ".claude") + os.makedirs(claude_home, exist_ok=True) + rc, out = self._cli([ + "adopt", "--project", tmp, "--claude-home", claude_home, + "--skill", "alpha", "--skill", "beta", + ]) + self.assertEqual(rc, 0, out) + self.assertEqual(_read(night.alpha_live), "# alpha v2\n") + self.assertEqual(_read(night.beta_live), "# beta v2\n") + + def test_skill_and_all_skills_together_are_refused(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + claude_home = os.path.join(tmp, ".claude") + os.makedirs(claude_home, exist_ok=True) + rc, out = self._cli([ + "adopt", "--project", tmp, "--claude-home", claude_home, + "--skill", "alpha", "--all-skills", + ]) + self.assertEqual(rc, 2) + self.assertIn("not both", out) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + self.assertEqual(_read(night.beta_live), "# beta v1\n") + + def test_legacy_night_bare_adopt_still_copies_the_managed_pair(self): + with tempfile.TemporaryDirectory() as tmp: + live = os.path.join(tmp, "live", "SKILL.md") + memory = os.path.join(tmp, "live", "CLAUDE.md") + _write(live, "# live v1\n") + _write(memory, "# mem v1\n") + write_staging( + tmp, report=SleepReport(night=1, project=tmp, accepted=True), + proposed_skill="# live v2\n", proposed_memory="# mem v2\n", + live_skill_path=live, live_memory_path=memory, + report_md="# report\n", + ) + claude_home = os.path.join(tmp, ".claude") + os.makedirs(claude_home, exist_ok=True) + rc, out = self._cli([ + "adopt", "--project", tmp, "--claude-home", claude_home, + ]) + self.assertEqual(rc, 0, out) + self.assertEqual(_read(live), "# live v2\n") + self.assertEqual(_read(memory), "# mem v2\n") + + def test_skill_flag_on_a_legacy_night_is_refused(self): + with tempfile.TemporaryDirectory() as tmp: + live = os.path.join(tmp, "live", "SKILL.md") + _write(live, "# live v1\n") + write_staging( + tmp, report=SleepReport(night=1, project=tmp, accepted=True), + proposed_skill="# live v2\n", proposed_memory=None, + live_skill_path=live, + live_memory_path=os.path.join(tmp, "live", "CLAUDE.md"), + report_md="# report\n", + ) + claude_home = os.path.join(tmp, ".claude") + os.makedirs(claude_home, exist_ok=True) + rc, out = self._cli([ + "adopt", "--project", tmp, "--claude-home", claude_home, + "--skill", "alpha", + ]) + self.assertEqual(rc, 2) + self.assertIn("no per-skill", out) + self.assertEqual(_read(live), "# live v1\n") + + +class TestAdoptTimeRevalidationMega(unittest.TestCase): + def test_casefold_live_path_collision_is_refused_at_adopt(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + manifest_path = os.path.join(night.staging, "manifest.json") + with open(manifest_path, encoding="utf-8") as f: + manifest = json.load(f) + live_root = os.path.dirname(os.path.dirname(night.alpha_live)) + manifest["skills"][1]["live_skill_path"] = os.path.join( + live_root, "ALPHA", "SKILL.md") + with open(manifest_path, "w", encoding="utf-8") as f: + json.dump(manifest, f) + with self.assertRaises(StagingError): + adopt_skills(night.staging) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + self.assertEqual(_read(night.beta_live), "# beta v1\n") + + def test_symlink_realpath_collision_is_refused_at_adopt(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + alias = os.path.join(night.live_root, "alias", "SKILL.md") + os.makedirs(os.path.dirname(alias), exist_ok=True) + try: + os.symlink(night.alpha_live, alias) + except OSError: + self.skipTest("symlinks unavailable") + manifest_path = os.path.join(night.staging, "manifest.json") + with open(manifest_path, encoding="utf-8") as f: + manifest = json.load(f) + manifest["skills"][1]["live_skill_path"] = alias + with open(manifest_path, "w", encoding="utf-8") as f: + json.dump(manifest, f) + with self.assertRaises(StagingError): + adopt_skills(night.staging) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + self.assertEqual(_read(night.beta_live), "# beta v1\n") + + def test_receipt_write_failure_restores_a_previous_receipt(self): + from skillopt_sleep import staging as staging_mod + + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + adopt_skills(night.staging, ["alpha"]) + receipt_path = os.path.join(night.staging, "adopted_skills.json") + previous = _read(receipt_path) + real_write = staging_mod._write_atomic + + def boom(path, text): + if os.path.basename(path) == "adopted_skills.json": + raise OSError("disk full") + return real_write(path, text) + + with mock.patch.object(staging_mod, "_write_atomic", side_effect=boom): + with self.assertRaises(OSError): + adopt_skills(night.staging, ["beta"]) + self.assertEqual(_read(night.beta_live), "# beta v1\n") + self.assertEqual(_read(night.alpha_live), "# alpha v2\n") + self.assertEqual(_read(receipt_path), previous) + + +def _accepted_group(name, body): + from skillopt_sleep.consolidate import ConsolidationResult + from skillopt_sleep.multi_skill import CONSOLIDATED, GroupConsolidation + + result = ConsolidationResult( + accepted=True, gate_action="accept_new_best", + baseline_score=0.1, candidate_score=0.2, + new_skill=body, new_memory="", + applied_edits=[], rejected_edits=[], + holdout_baseline=0.1, holdout_candidate=0.2, + ) + return GroupConsolidation( + skill_name=name, status=CONSOLIDATED, result=result, n_tasks=2, + ) + + +class TestSkillProposalsFromGroups(unittest.TestCase): + def test_skips_managed_catch_all_and_unresolved_names(self): + from skillopt_sleep.config import load_config + from skillopt_sleep.cycle import _skill_proposals_from_groups + + with tempfile.TemporaryDirectory() as home: + claude_home = os.path.join(home, ".claude") + live = os.path.join(claude_home, "skills", "research-skill", "SKILL.md") + _write(live, "# research v1\n") + cfg = load_config( + claude_home=claude_home, + managed_skill_name="skillopt-sleep-learned", + ) + proposals = _skill_proposals_from_groups( + cfg, + { + "skillopt-sleep-learned": _accepted_group( + "skillopt-sleep-learned", "# managed v2\n"), + "research-skill": _accepted_group( + "research-skill", "# research v2\n"), + "ghost-skill": _accepted_group( + "ghost-skill", "# ghost v2\n"), + }, + "skillopt-sleep-learned", + ) + names = [p.skill_name for p in proposals] + self.assertEqual(names, ["research-skill"]) + self.assertEqual(proposals[0].live_skill_path, os.path.realpath(live)) + self.assertEqual(proposals[0].proposed_skill, "# research v2\n") + + def test_skips_a_second_skill_that_resolves_to_the_same_live_file(self): + from skillopt_sleep.config import load_config + from skillopt_sleep.cycle import _skill_proposals_from_groups + + with tempfile.TemporaryDirectory() as home: + claude_home = os.path.join(home, ".claude") + skills = os.path.join(claude_home, "skills") + research = os.path.join(skills, "research-skill") + alias = os.path.join(skills, "alias-skill") + _write(os.path.join(research, "SKILL.md"), "# research v1\n") + try: + os.symlink(research, alias) + except OSError: + self.skipTest("symlinks unavailable") + cfg = load_config(claude_home=claude_home) + proposals = _skill_proposals_from_groups( + cfg, + { + "research-skill": _accepted_group( + "research-skill", "# research v2\n"), + "alias-skill": _accepted_group( + "alias-skill", "# alias v2\n"), + }, + "skillopt-sleep-learned", + ) + self.assertEqual([p.skill_name for p in proposals], ["research-skill"]) + + +class TestCycleStagingGaps(unittest.TestCase): + def _hinted_tasks(self): + from dataclasses import replace + + from skillopt_sleep.experiments.personas import programmer_persona, researcher_persona + from skillopt_sleep.mine import assign_splits + + research = assign_splits(researcher_persona(), holdout_fraction=0.34, seed=42) + programming = assign_splits(programmer_persona(), holdout_fraction=0.34, seed=1) + tagged = [replace(t, skill_hint="research-skill") for t in research] + tagged += [replace(t, id=f"prog-{t.id}", skill_hint="programming-skill") + for t in programming] + return tagged + + def test_missing_live_skill_is_skipped_not_aborted(self): + from skillopt_sleep.config import load_config + from skillopt_sleep.cycle import run_sleep_cycle + + with tempfile.TemporaryDirectory() as proj, tempfile.TemporaryDirectory() as home: + claude_home = os.path.join(home, ".claude") + research_live = os.path.join( + claude_home, "skills", "research-skill", "SKILL.md") + _write(research_live, "# research-skill v1\n") + cfg = load_config( + invoked_project=proj, projects="invoked", backend="mock", + claude_home=claude_home, + managed_skill_name="skillopt-sleep-learned", auto_adopt=False, + multi_skill_report=True, + ) + outcome = run_sleep_cycle(cfg, seed_tasks=self._hinted_tasks()) + names = [r["skill_name"] for r in staged_skills(outcome.staging_dir)] + self.assertEqual(names, ["research-skill"]) + self.assertFalse(os.path.isfile(os.path.join( + outcome.staging_dir, "proposed_SKILL.programming-skill.md"))) + + def test_report_off_stages_no_per_skill_proposals(self): + from skillopt_sleep.config import load_config + from skillopt_sleep.cycle import run_sleep_cycle + + with tempfile.TemporaryDirectory() as proj, tempfile.TemporaryDirectory() as home: + claude_home = os.path.join(home, ".claude") + _write(os.path.join(claude_home, "skills", "research-skill", "SKILL.md"), + "# research-skill v1\n") + _write(os.path.join(claude_home, "skills", "programming-skill", "SKILL.md"), + "# programming-skill v1\n") + cfg = load_config( + invoked_project=proj, projects="invoked", backend="mock", + claude_home=claude_home, + managed_skill_name="skillopt-sleep-learned", auto_adopt=False, + multi_skill_report=False, + ) + outcome = run_sleep_cycle(cfg, seed_tasks=self._hinted_tasks()) + self.assertEqual(staged_skills(outcome.staging_dir), []) + + def test_auto_adopt_does_not_promote_per_skill_live_files(self): + from skillopt_sleep.config import load_config + from skillopt_sleep.cycle import run_sleep_cycle + + with tempfile.TemporaryDirectory() as proj, tempfile.TemporaryDirectory() as home: + claude_home = os.path.join(home, ".claude") + research_live = os.path.join( + claude_home, "skills", "research-skill", "SKILL.md") + programming_live = os.path.join( + claude_home, "skills", "programming-skill", "SKILL.md") + _write(research_live, "# research-skill v1\n") + _write(programming_live, "# programming-skill v1\n") + cfg = load_config( + invoked_project=proj, projects="invoked", backend="mock", + claude_home=claude_home, + managed_skill_name="skillopt-sleep-learned", auto_adopt=True, + multi_skill_report=True, + ) + outcome = run_sleep_cycle(cfg, seed_tasks=self._hinted_tasks()) + self.assertEqual(_read(research_live), "# research-skill v1\n") + self.assertEqual(_read(programming_live), "# programming-skill v1\n") + names = [r["skill_name"] for r in staged_skills(outcome.staging_dir)] + self.assertIn("research-skill", names) + self.assertIn("programming-skill", names) + if __name__ == "__main__": unittest.main() From f393a7ad1a286b3cbfc81d9a41edd9bf52f26716 Mon Sep 17 00:00:00 2001 From: "Bogdan (Dan) Baciu" Date: Wed, 12 Aug 2026 20:58:37 +0200 Subject: [PATCH 4/4] fix(sleep): pin staged skill hashes and confine adopt targets 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/SkillOpt#212 --- docs/sleep/multi-skill-staging.md | 32 ++-- skillopt_sleep/__main__.py | 6 +- skillopt_sleep/cycle.py | 31 +++- skillopt_sleep/staging.py | 125 +++++++++++--- tests/test_sleep_adopt_skill_subset.py | 219 +++++++++++++++++++++++-- tests/test_sleep_staging_fanout.py | 15 ++ 6 files changed, 369 insertions(+), 59 deletions(-) diff --git a/docs/sleep/multi-skill-staging.md b/docs/sleep/multi-skill-staging.md index 2d5bfa35..8ef711f2 100644 --- a/docs/sleep/multi-skill-staging.md +++ b/docs/sleep/multi-skill-staging.md @@ -26,8 +26,8 @@ When `multi_skill_report` is on and hinted groups pass the gate: - each accepted group name is resolved with `resolve_skill` against `skill_search_roots(cfg)`; - only `FOUND` unique live paths become `SkillProposal` rows; -- missing, ambiguous, rejected, or colliding names are skipped rather than - aborting the night. +- missing, ambiguous, rejected, empty, or colliding names are skipped rather + than aborting the night, and each skip is recorded on `report.notes`. Review remains explicit. `auto_adopt` still only runs the legacy `adopt()` pair; it never silently promotes every staged skill. @@ -65,12 +65,14 @@ Multi-skill night — one extra file and one manifest row per skill: { "skill_name": "alpha", "proposed_file": "proposed_SKILL.alpha.md", - "live_skill_path": "/home/dev/.claude/skills/alpha/SKILL.md" + "live_skill_path": "/home/dev/.claude/skills/alpha/SKILL.md", + "sha256": "" }, { "skill_name": "beta", "proposed_file": "proposed_SKILL.beta.md", - "live_skill_path": "/home/dev/.claude/skills/beta/SKILL.md" + "live_skill_path": "/home/dev/.claude/skills/beta/SKILL.md", + "sha256": "" } ] } @@ -108,17 +110,23 @@ skill. It lists the names and asks for `--skill` or `--all-skills`. Legacy nights (no `skills` in the manifest) still use `adopt()` unchanged. - `skill_names=None` adopts every staged skill; `[]` adopts nothing. -- An unknown or repeated name, an unsafe manifest row, a missing proposal file, - or a uniqueness / live-target collision raises `StagingError` **before** - anything is written. -- Uniqueness and live-target nonexistence are re-checked **at adoption time**, - not only at staging, so a tampered manifest that points two skills at one - file (including via casefold or realpath/symlink) is refused with no writes. +- An unknown or repeated name, an empty `--skill` token, an unsafe manifest + row, a missing proposal file, a sha256 mismatch, an empty proposal body, or a + uniqueness / live-target collision raises `StagingError` **before** anything + is written. +- Uniqueness and live-target checks run **at adoption time against every staged + row**, not only the selection, so adopting one skill cannot hide a sibling + that now points at the same file (including via casefold or realpath/symlink). A live path that exists as something other than a file is also refused. +- Each selected proposal is pinned by the manifest `sha256`. Tampering with the + staged file, or dropping the pin, is refused with no writes. +- The live target must already be `/SKILL.md`. Adopt will not create + parent directories, follow a symlink file, or write through a symlink parent. - Each live file is backed up to `backup/skills//` and written atomically. - If any write fails — including `adopted_skills.json` — every live file in the - selection is restored (and files that did not exist before are removed), so a - partial adoption never survives. + selection is restored (and files that did not exist before are removed), and + the previous receipt bytes are restored atomically, so a partial adoption + never survives. - Receipts (`skill_name`, `live_skill_path`, `sha256_before`, `sha256_after`, `backup_path`) are returned and written to `adopted_skills.json` in the staging directory. An empty `sha256_before` means the skill had no live file yet. diff --git a/skillopt_sleep/__main__.py b/skillopt_sleep/__main__.py index 098f2db3..2c2d9b6d 100644 --- a/skillopt_sleep/__main__.py +++ b/skillopt_sleep/__main__.py @@ -467,7 +467,11 @@ def cmd_adopt(args) -> int: if not target or not os.path.isdir(target): print("[sleep] nothing to adopt (no staging dir).") return 1 - selected = list(getattr(args, "skills", None) or []) + raw_selected = list(getattr(args, "skills", None) or []) + if any(not str(name).strip() for name in raw_selected): + print("[sleep] --skill names must be non-empty.") + return 2 + selected = [str(name).strip() for name in raw_selected] adopt_all = bool(getattr(args, "all_skills", False)) if selected and adopt_all: print("[sleep] use --skill or --all-skills, not both.") diff --git a/skillopt_sleep/cycle.py b/skillopt_sleep/cycle.py index 2dd00aef..9946ad11 100644 --- a/skillopt_sleep/cycle.py +++ b/skillopt_sleep/cycle.py @@ -275,34 +275,48 @@ def _render_report_md(report: SleepReport, cfg: SleepConfig) -> str: return "\n".join(lines) +def _cycle_skip_note(name: str, reason: str) -> str: + """One-line skip reason for report.notes. Names are untrusted free text.""" + label = str(name or "").strip() or "" + return redact_secrets(f"cycle skipped skill {label}: {reason}") + + def _skill_proposals_from_groups( cfg: SleepConfig, group_outcomes: dict, managed_name: str, -) -> List[SkillProposal]: +) -> tuple[List[SkillProposal], List[str]]: """Stage per-skill proposals for accepted groups whose names resolve uniquely. Groups still consolidate from the managed document; this only chooses the live ``SKILL.md`` each accepted name would replace. Unresolved, ambiguous, - rejected, or colliding names are skipped so one bad hint cannot abort the - night. The managed catch-all is never staged here — it stays on the legacy - ``proposed_SKILL.md`` path. + rejected, empty, or colliding names are skipped so one bad hint cannot abort + the night; each skip is recorded on ``report.notes``. The managed catch-all + is never staged here — it stays on the legacy ``proposed_SKILL.md`` path. """ roots = skill_search_roots(cfg) proposals: List[SkillProposal] = [] + notes: List[str] = [] for name, new_skill in accepted_group_skills(group_outcomes).items(): if name == managed_name: continue + if not str(new_skill or "").strip(): + notes.append(_cycle_skip_note(name, "empty proposed_skill")) + continue resolution = resolve_skill(name, roots) if not resolution.ok: + notes.append( + _cycle_skip_note(name, resolution.reason or resolution.status) + ) continue candidate = SkillProposal(name, new_skill, resolution.path) try: skill_proposal_rows(proposals + [candidate]) - except StagingError: + except StagingError as exc: + notes.append(_cycle_skip_note(name, str(exc))) continue proposals.append(candidate) - return proposals + return proposals, notes def run_sleep_cycle( @@ -606,7 +620,10 @@ def run_sleep_cycle( report_md = _render_report_md(report, cfg) proposed_skill = result.new_skill if (cfg.get("evolve_skill") and result.accepted) else None proposed_memory = result.new_memory if (cfg.get("evolve_memory") and result.accepted) else None - skill_proposals = _skill_proposals_from_groups(cfg, group_outcomes, managed_name) + skill_proposals, skip_notes = _skill_proposals_from_groups( + cfg, group_outcomes, managed_name + ) + report.notes.extend(skip_notes) staging_dir = write_staging( project, report=report, diff --git a/skillopt_sleep/staging.py b/skillopt_sleep/staging.py index a8bdcd77..b5dcf4c1 100644 --- a/skillopt_sleep/staging.py +++ b/skillopt_sleep/staging.py @@ -308,10 +308,15 @@ def proposal_filename(skill_name: str) -> str: return f"proposed_SKILL.{skill_name}.md" -def _write_atomic(path: str, text: str) -> None: +def _sha256_text(text: str) -> str: + return hashlib.sha256(text.encode("utf-8")).hexdigest() + + +def _write_atomic(path: str, text: str, *, create_parents: bool = True) -> None: """Write ``text`` to ``path`` atomically, so review never sees half a file.""" directory = os.path.dirname(path) or "." - os.makedirs(directory, exist_ok=True) + if create_parents: + os.makedirs(directory, exist_ok=True) existing_mode = ( stat.S_IMODE(os.stat(path).st_mode) if os.path.exists(path) else None ) @@ -390,6 +395,7 @@ def skill_proposal_rows(proposals: Iterable[SkillProposal]) -> List[Dict[str, An "skill_name": name, "proposed_file": proposed_file, "live_skill_path": live, + "sha256": _sha256_text(proposal.proposed_skill), }) return rows @@ -410,6 +416,11 @@ def write_skill_proposals( rows = skill_proposal_rows(proposals) if not rows: return rows + for row, proposal in zip(rows, proposals): + if not str(proposal.proposed_skill).strip(): + raise StagingError( + f"proposed skill content for {row['skill_name']!r} is empty" + ) os.makedirs(out_dir, exist_ok=True) for row, proposal in zip(rows, proposals): _write_atomic(os.path.join(out_dir, row["proposed_file"]), proposal.proposed_skill) @@ -513,10 +524,6 @@ class AdoptedSkill: backup_path: str = "" # "" when there was nothing to back up -def _sha256_text(text: str) -> str: - return hashlib.sha256(text.encode("utf-8")).hexdigest() - - def staged_skills(staging_dir: str) -> List[Dict[str, Any]]: """Manifest rows for the per-skill proposals staged in ``staging_dir``.""" with open(os.path.join(staging_dir, "manifest.json"), encoding="utf-8") as f: @@ -553,26 +560,33 @@ def _selected_rows( return [row for row in rows if str(row.get("skill_name", "")) in chosen] -def _revalidate_selected_skill_rows(rows: Sequence[Dict[str, Any]]) -> None: +def _revalidate_selected_skill_rows( + rows: Sequence[Dict[str, Any]], + *, + all_rows: Optional[Sequence[Dict[str, Any]]] = None, +) -> None: """Re-run uniqueness and live-target checks at adoption time. Staging already refused collisions, but the manifest can be edited between staging and adopt. A tampered pair that shares a skill name, a staged - filename, or a live target must fail here with no writes. Live paths are + filename, or a live target must fail here with no writes. The check runs + against every staged row, not only the selection, so adopting one skill + cannot hide a sibling that now points at the same file. Live paths are also compared by realpath so a symlink cannot hide a second claim on one file, and a live path that exists as something other than a file is refused rather than overwritten. """ + universe = list(all_rows) if all_rows is not None else list(rows) skill_proposal_rows([ SkillProposal( str(row.get("skill_name") or ""), "", str(row.get("live_skill_path") or ""), ) - for row in rows + for row in universe ]) seen_real: Dict[str, str] = {} - for row in rows: + for row in universe: name = _safe_skill_name(row.get("skill_name")) live = _safe_live_path(row.get("live_skill_path")) if not name or not live: @@ -593,6 +607,35 @@ def _revalidate_selected_skill_rows(rows: Sequence[Dict[str, Any]]) -> None: ) +def _valid_sha256_pin(value: object) -> bool: + if not isinstance(value, str) or len(value) != 64: + return False + return all(ch in "0123456789abcdef" for ch in value) + + +def _adopt_live_target_ok(name: str, live: str) -> None: + """Refuse live targets that would create dirs, follow links, or leave the skill folder.""" + if os.path.islink(live): + raise StagingError(f"live skill path for {name!r} is a symlink: {live}") + parent = os.path.dirname(live) + if os.path.islink(parent): + raise StagingError( + f"live skill parent directory for {name!r} is a symlink: {parent}" + ) + if not os.path.isdir(parent): + raise StagingError( + f"live skill parent directory for {name!r} does not exist: {parent}" + ) + if os.path.basename(live) != "SKILL.md": + raise StagingError( + f"live skill path for {name!r} must be a SKILL.md file: {live}" + ) + if os.path.basename(parent) != name: + raise StagingError( + f"live skill path for {name!r} is not {name}/SKILL.md: {live}" + ) + + def _restore_live_writes(done: Sequence[tuple]) -> None: """Restore live files written by a failed adoption, newest first.""" for live, original in reversed(done): @@ -604,6 +647,26 @@ def _restore_live_writes(done: Sequence[tuple]) -> None: f.write(original) +def _restore_receipt_bytes(path: str, original: Optional[bytes]) -> None: + """Put ``adopted_skills.json`` back without leaving a half-written file.""" + if original is not None: + directory = os.path.dirname(path) or "." + fd, tmp = tempfile.mkstemp(dir=directory, prefix=".tmp-", suffix=".json") + try: + with os.fdopen(fd, "wb") as f: + f.write(original) + f.flush() + os.fsync(f.fileno()) + os.replace(tmp, path) + except BaseException: + if os.path.exists(tmp): + os.unlink(tmp) + raise + return + if os.path.isfile(path): + os.unlink(path) + + def adopt_skills( staging_dir: str, skill_names: Optional[Sequence[str]] = None ) -> List[AdoptedSkill]: @@ -614,16 +677,18 @@ def adopt_skills( are never touched. Every selected proposal is validated first, including a second uniqueness - and live-target check against the current manifest and filesystem. Each - live file is backed up, and the writes — including ``adopted_skills.json`` - — are rolled back as a set if any one of them fails, so a partial adoption - never survives. Returns a before/after sha256 receipt per skill and also - writes them to ``adopted_skills.json`` in the staging directory. + and live-target check against the **whole** current manifest, a sha256 pin + of the staged file, and a live-path layout check. Each live file is backed + up, and the writes — including ``adopted_skills.json`` — are rolled back as + a set if any one of them fails, so a partial adoption never survives. + Returns a before/after sha256 receipt per skill and also writes them to + ``adopted_skills.json`` in the staging directory. """ - rows = _selected_rows(staged_skills(staging_dir), skill_names) + all_rows = staged_skills(staging_dir) + rows = _selected_rows(all_rows, skill_names) if not rows: return [] - _revalidate_selected_skill_rows(rows) + _revalidate_selected_skill_rows(rows, all_rows=all_rows) plan: List[tuple] = [] for row in rows: @@ -635,6 +700,7 @@ def adopt_skills( raise StagingError( f"unsafe live skill path for {name!r}: {row.get('live_skill_path')!r}" ) + _adopt_live_target_ok(name, live) proposed_file = row.get("proposed_file") expected_file = proposal_filename(name) if proposed_file != expected_file: @@ -645,7 +711,18 @@ def adopt_skills( staged = os.path.join(staging_dir, expected_file) if not os.path.isfile(staged): raise StagingError(f"staged proposal missing for {name!r}: {staged}") - plan.append((name, live, staged)) + with open(staged, encoding="utf-8") as f: + proposed = f.read() + pin = row.get("sha256") + if not _valid_sha256_pin(pin): + raise StagingError(f"staged proposal for {name!r} is missing a sha256 pin") + if _sha256_text(proposed) != pin: + raise StagingError( + f"staged proposal for {name!r} does not match its manifest sha256" + ) + if not proposed.strip(): + raise StagingError(f"staged proposal for {name!r} is empty") + plan.append((name, live, proposed)) backup_dir = os.path.join(staging_dir, "backup", "skills") receipts: List[AdoptedSkill] = [] @@ -656,9 +733,7 @@ def adopt_skills( with open(receipt_path, "rb") as f: receipt_original = f.read() try: - for name, live, staged in plan: - with open(staged, encoding="utf-8") as f: - proposed = f.read() + for name, live, proposed in plan: original = None backup_path = "" if os.path.exists(live): @@ -669,7 +744,7 @@ def adopt_skills( backup_path = os.path.join(skill_backup, os.path.basename(live)) shutil.copy2(live, backup_path) before = hashlib.sha256(original).hexdigest() if original is not None else "" - _write_atomic(live, proposed) + _write_atomic(live, proposed, create_parents=False) done.append((live, original)) receipts.append(AdoptedSkill( skill_name=name, live_skill_path=live, sha256_before=before, @@ -681,11 +756,7 @@ def adopt_skills( ) except BaseException: _restore_live_writes(done) - if receipt_original is not None: - with open(receipt_path, "wb") as f: - f.write(receipt_original) - elif os.path.isfile(receipt_path): - os.unlink(receipt_path) + _restore_receipt_bytes(receipt_path, receipt_original) raise return receipts diff --git a/tests/test_sleep_adopt_skill_subset.py b/tests/test_sleep_adopt_skill_subset.py index 492cdc75..dbf19077 100644 --- a/tests/test_sleep_adopt_skill_subset.py +++ b/tests/test_sleep_adopt_skill_subset.py @@ -204,28 +204,56 @@ def test_adoption_preserves_existing_live_file_mode(self): self.assertEqual(stat.S_IMODE(os.stat(night.alpha_live).st_mode), 0o640) def test_a_failed_write_rolls_the_whole_selection_back(self): + from skillopt_sleep import staging as staging_mod + with tempfile.TemporaryDirectory() as tmp: night = TwoSkillNight(tmp) - # beta's live path becomes un-writable: its parent is now a file. - os.unlink(night.beta_live) - os.rmdir(os.path.dirname(night.beta_live)) - _write(os.path.dirname(night.beta_live), "not a directory\n") - with self.assertRaises(OSError): - adopt_skills(night.staging) + real_write = staging_mod._write_atomic + + def boom(path, text, *, create_parents=True): + if path == night.beta_live: + raise OSError("disk full") + return real_write(path, text, create_parents=create_parents) + + with mock.patch.object(staging_mod, "_write_atomic", side_effect=boom): + with self.assertRaises(OSError): + adopt_skills(night.staging) self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + self.assertEqual(_read(night.beta_live), "# beta v1\n") self.assertFalse( os.path.exists(os.path.join(night.staging, "adopted_skills.json"))) def test_rollback_removes_files_that_did_not_exist_before(self): + from skillopt_sleep import staging as staging_mod + with tempfile.TemporaryDirectory() as tmp: night = TwoSkillNight(tmp) os.unlink(night.alpha_live) os.unlink(night.beta_live) + real_write = staging_mod._write_atomic + + def boom(path, text, *, create_parents=True): + if path == night.beta_live: + raise OSError("disk full") + return real_write(path, text, create_parents=create_parents) + + with mock.patch.object(staging_mod, "_write_atomic", side_effect=boom): + with self.assertRaises(OSError): + adopt_skills(night.staging) + self.assertFalse(os.path.exists(night.alpha_live)) + self.assertFalse(os.path.exists(night.beta_live)) + + def test_missing_live_parent_is_refused_before_any_write(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + os.unlink(night.beta_live) os.rmdir(os.path.dirname(night.beta_live)) _write(os.path.dirname(night.beta_live), "not a directory\n") - with self.assertRaises(OSError): + with self.assertRaises(StagingError): adopt_skills(night.staging) - self.assertFalse(os.path.exists(night.alpha_live)) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + self.assertFalse( + os.path.exists(os.path.join(night.staging, "adopted_skills.json"))) def test_adoption_never_happens_without_an_explicit_call(self): with tempfile.TemporaryDirectory() as tmp: @@ -458,6 +486,33 @@ def test_skill_flag_on_a_legacy_night_is_refused(self): self.assertIn("no per-skill", out) self.assertEqual(_read(live), "# live v1\n") + def test_empty_skill_flag_is_refused(self): + with tempfile.TemporaryDirectory() as tmp: + TwoSkillNight(tmp) + claude_home = os.path.join(tmp, ".claude") + os.makedirs(claude_home, exist_ok=True) + rc, out = self._cli([ + "adopt", "--project", tmp, "--claude-home", claude_home, + "--skill", " ", + ]) + self.assertEqual(rc, 2) + self.assertIn("non-empty", out) + self.assertEqual(_read(os.path.join(tmp, "live", "alpha", "SKILL.md")), + "# alpha v1\n") + + def test_skill_flag_strips_surrounding_whitespace(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + claude_home = os.path.join(tmp, ".claude") + os.makedirs(claude_home, exist_ok=True) + rc, out = self._cli([ + "adopt", "--project", tmp, "--claude-home", claude_home, + "--skill", " alpha ", + ]) + self.assertEqual(rc, 0, out) + self.assertEqual(_read(night.alpha_live), "# alpha v2\n") + self.assertEqual(_read(night.beta_live), "# beta v1\n") + class TestAdoptTimeRevalidationMega(unittest.TestCase): def test_casefold_live_path_collision_is_refused_at_adopt(self): @@ -506,10 +561,10 @@ def test_receipt_write_failure_restores_a_previous_receipt(self): previous = _read(receipt_path) real_write = staging_mod._write_atomic - def boom(path, text): + def boom(path, text, *, create_parents=True): if os.path.basename(path) == "adopted_skills.json": raise OSError("disk full") - return real_write(path, text) + return real_write(path, text, create_parents=create_parents) with mock.patch.object(staging_mod, "_write_atomic", side_effect=boom): with self.assertRaises(OSError): @@ -548,7 +603,7 @@ def test_skips_managed_catch_all_and_unresolved_names(self): claude_home=claude_home, managed_skill_name="skillopt-sleep-learned", ) - proposals = _skill_proposals_from_groups( + proposals, notes = _skill_proposals_from_groups( cfg, { "skillopt-sleep-learned": _accepted_group( @@ -564,6 +619,8 @@ def test_skips_managed_catch_all_and_unresolved_names(self): self.assertEqual(names, ["research-skill"]) self.assertEqual(proposals[0].live_skill_path, os.path.realpath(live)) self.assertEqual(proposals[0].proposed_skill, "# research v2\n") + self.assertTrue(any("ghost-skill" in note for note in notes)) + self.assertFalse(any("skillopt-sleep-learned" in note for note in notes)) def test_skips_a_second_skill_that_resolves_to_the_same_live_file(self): from skillopt_sleep.config import load_config @@ -580,7 +637,7 @@ def test_skips_a_second_skill_that_resolves_to_the_same_live_file(self): except OSError: self.skipTest("symlinks unavailable") cfg = load_config(claude_home=claude_home) - proposals = _skill_proposals_from_groups( + proposals, notes = _skill_proposals_from_groups( cfg, { "research-skill": _accepted_group( @@ -591,6 +648,7 @@ def test_skips_a_second_skill_that_resolves_to_the_same_live_file(self): "skillopt-sleep-learned", ) self.assertEqual([p.skill_name for p in proposals], ["research-skill"]) + self.assertTrue(any("alias-skill" in note for note in notes)) class TestCycleStagingGaps(unittest.TestCase): @@ -627,6 +685,9 @@ def test_missing_live_skill_is_skipped_not_aborted(self): self.assertEqual(names, ["research-skill"]) self.assertFalse(os.path.isfile(os.path.join( outcome.staging_dir, "proposed_SKILL.programming-skill.md"))) + self.assertTrue(any( + "programming-skill" in note for note in outcome.report.notes + )) def test_report_off_stages_no_per_skill_proposals(self): from skillopt_sleep.config import load_config @@ -673,5 +734,139 @@ def test_auto_adopt_does_not_promote_per_skill_live_files(self): self.assertIn("programming-skill", names) +class TestAdoptHardeningPinsAndLayout(unittest.TestCase): + def test_tampered_proposal_file_is_refused_by_sha256_pin(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + staged = os.path.join(night.staging, "proposed_SKILL.alpha.md") + _write(staged, "# alpha tampered\n") + with self.assertRaisesRegex(StagingError, "does not match its manifest sha256"): + adopt_skills(night.staging, ["alpha"]) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + self.assertFalse( + os.path.exists(os.path.join(night.staging, "adopted_skills.json"))) + + def test_missing_sha256_pin_is_refused_without_writing(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + manifest_path = os.path.join(night.staging, "manifest.json") + with open(manifest_path, encoding="utf-8") as f: + manifest = json.load(f) + del manifest["skills"][0]["sha256"] + with open(manifest_path, "w", encoding="utf-8") as f: + json.dump(manifest, f) + with self.assertRaisesRegex(StagingError, "missing a sha256 pin"): + adopt_skills(night.staging, ["alpha"]) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + + def test_empty_staged_proposal_is_refused_even_when_hash_matches(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + staged = os.path.join(night.staging, "proposed_SKILL.alpha.md") + _write(staged, " \n") + manifest_path = os.path.join(night.staging, "manifest.json") + with open(manifest_path, encoding="utf-8") as f: + manifest = json.load(f) + manifest["skills"][0]["sha256"] = _sha(" \n") + with open(manifest_path, "w", encoding="utf-8") as f: + json.dump(manifest, f) + with self.assertRaisesRegex(StagingError, "is empty"): + adopt_skills(night.staging, ["alpha"]) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + + def test_symlink_live_file_is_refused_without_writing(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + os.unlink(night.alpha_live) + elsewhere = os.path.join(tmp, "elsewhere.md") + _write(elsewhere, "# elsewhere\n") + try: + os.symlink(elsewhere, night.alpha_live) + except OSError: + self.skipTest("symlinks unavailable") + with self.assertRaisesRegex(StagingError, "is a symlink"): + adopt_skills(night.staging, ["alpha"]) + self.assertEqual(_read(elsewhere), "# elsewhere\n") + self.assertTrue(os.path.islink(night.alpha_live)) + + def test_symlink_parent_directory_is_refused_without_writing(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + real_parent = os.path.dirname(night.alpha_live) + alias_parent = os.path.join(night.live_root, "alias-alpha") + try: + os.symlink(real_parent, alias_parent) + except OSError: + self.skipTest("symlinks unavailable") + alias_live = os.path.join(alias_parent, "SKILL.md") + manifest_path = os.path.join(night.staging, "manifest.json") + with open(manifest_path, encoding="utf-8") as f: + manifest = json.load(f) + manifest["skills"][0]["skill_name"] = "alias-alpha" + manifest["skills"][0]["proposed_file"] = "proposed_SKILL.alias-alpha.md" + manifest["skills"][0]["live_skill_path"] = alias_live + with open(manifest_path, "w", encoding="utf-8") as f: + json.dump(manifest, f) + os.rename( + os.path.join(night.staging, "proposed_SKILL.alpha.md"), + os.path.join(night.staging, "proposed_SKILL.alias-alpha.md"), + ) + with self.assertRaisesRegex(StagingError, "is a symlink"): + adopt_skills(night.staging, ["alias-alpha"]) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + + def test_subset_adopt_refuses_unselected_sibling_realpath_collision(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + alias = os.path.join(night.live_root, "alias", "SKILL.md") + os.makedirs(os.path.dirname(alias), exist_ok=True) + try: + os.symlink(night.alpha_live, alias) + except OSError: + self.skipTest("symlinks unavailable") + manifest_path = os.path.join(night.staging, "manifest.json") + with open(manifest_path, encoding="utf-8") as f: + manifest = json.load(f) + manifest["skills"][1]["live_skill_path"] = alias + with open(manifest_path, "w", encoding="utf-8") as f: + json.dump(manifest, f) + with self.assertRaises(StagingError): + adopt_skills(night.staging, ["alpha"]) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + self.assertEqual(_read(night.beta_live), "# beta v1\n") + + def test_live_path_not_named_skill_md_is_refused(self): + with tempfile.TemporaryDirectory() as tmp: + night = TwoSkillNight(tmp) + wrong = os.path.join(night.live_root, "alpha", "NOTES.md") + _write(wrong, "# notes\n") + manifest_path = os.path.join(night.staging, "manifest.json") + with open(manifest_path, encoding="utf-8") as f: + manifest = json.load(f) + manifest["skills"][0]["live_skill_path"] = wrong + with open(manifest_path, "w", encoding="utf-8") as f: + json.dump(manifest, f) + with self.assertRaisesRegex(StagingError, "must be a SKILL.md file"): + adopt_skills(night.staging, ["alpha"]) + self.assertEqual(_read(night.alpha_live), "# alpha v1\n") + + def test_cycle_skips_empty_proposed_skill_with_a_note(self): + from skillopt_sleep.config import load_config + from skillopt_sleep.cycle import _skill_proposals_from_groups + + with tempfile.TemporaryDirectory() as home: + claude_home = os.path.join(home, ".claude") + live = os.path.join(claude_home, "skills", "research-skill", "SKILL.md") + _write(live, "# research v1\n") + cfg = load_config(claude_home=claude_home) + proposals, notes = _skill_proposals_from_groups( + cfg, + {"research-skill": _accepted_group("research-skill", " \n")}, + "skillopt-sleep-learned", + ) + self.assertEqual(proposals, []) + self.assertTrue(any("empty proposed_skill" in note for note in notes)) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_sleep_staging_fanout.py b/tests/test_sleep_staging_fanout.py index cd3f29f8..89519d1e 100644 --- a/tests/test_sleep_staging_fanout.py +++ b/tests/test_sleep_staging_fanout.py @@ -5,6 +5,7 @@ """ from __future__ import annotations +import hashlib import json import os import tempfile @@ -40,6 +41,10 @@ def test_one_row_per_skill_in_order(self): ["proposed_SKILL.alpha.md", "proposed_SKILL.beta.md"]) self.assertEqual(rows[0]["live_skill_path"], os.path.normpath("/tmp/live/alpha/SKILL.md")) + self.assertEqual( + rows[0]["sha256"], + hashlib.sha256(b"# example\n").hexdigest(), + ) def test_filenames_are_unique_per_skill(self): self.assertNotEqual(proposal_filename("alpha"), proposal_filename("beta")) @@ -147,6 +152,12 @@ def test_writes_one_file_per_skill(self): with open(os.path.join(tmp, rows[0]["proposed_file"]), encoding="utf-8") as f: self.assertEqual(f.read(), "# alpha\n") + def test_empty_proposal_body_is_refused_without_writing(self): + with tempfile.TemporaryDirectory() as tmp: + with self.assertRaisesRegex(StagingError, "is empty"): + write_skill_proposals(tmp, [_proposal("alpha", " \n")]) + self.assertEqual(os.listdir(tmp), []) + def test_no_proposals_writes_nothing(self): with tempfile.TemporaryDirectory() as tmp: self.assertEqual(write_skill_proposals(tmp, []), []) @@ -227,6 +238,10 @@ def test_fan_out_adds_files_and_manifest_rows(self): self.assertEqual([r["skill_name"] for r in rows], ["alpha", "beta"]) self.assertEqual(rows[1]["live_skill_path"], os.path.join(live_root, "beta", "SKILL.md")) + self.assertEqual( + rows[0]["sha256"], + hashlib.sha256(b"# alpha\n").hexdigest(), + ) def test_unsafe_fan_out_writes_no_manifest(self): with tempfile.TemporaryDirectory() as tmp: