From 0a816f957852755b0d0d2f896883e5dce498eee3 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:41:56 +0000 Subject: [PATCH 1/5] feat(sleep): stage one proposal and manifest row per skill Add SkillProposal, skill_proposal_rows, and write_skill_proposals: validate skill names, live target paths, and collisions before writing, then write each skill's proposal atomically. write_staging gains an optional skill_proposals fan-out and keeps the legacy single-proposal layout when it is unused. Refs #120 --- skillopt_sleep/staging.py | 131 +++++++++++++++++++++- tests/test_sleep_staging_fanout.py | 169 +++++++++++++++++++++++++++++ 2 files changed, 299 insertions(+), 1 deletion(-) create mode 100644 tests/test_sleep_staging_fanout.py diff --git a/skillopt_sleep/staging.py b/skillopt_sleep/staging.py index 12201ec9..bca36778 100644 --- a/skillopt_sleep/staging.py +++ b/skillopt_sleep/staging.py @@ -11,8 +11,10 @@ import os import re import shutil +import tempfile import time -from typing import Any, List, Optional +from dataclasses import dataclass +from typing import Any, Dict, List, Optional, Sequence from skillopt_sleep.types import SleepReport @@ -234,6 +236,124 @@ def redact_secrets(value: Any) -> Any: return value +class StagingError(ValueError): + """A proposal could not be staged safely (bad name, bad target, collision).""" + + +@dataclass +class SkillProposal: + """One skill's proposed document plus the live file it would replace.""" + + skill_name: str + proposed_skill: str + live_skill_path: str + + +def _safe_skill_name(name: object) -> str: + """Return a skill name usable as a single path segment, else "".""" + if not isinstance(name, str): + return "" + candidate = name.strip() + if not candidate or candidate in {os.curdir, os.pardir}: + return "" + if candidate.startswith("~") or os.path.isabs(candidate): + return "" + if os.path.splitdrive(candidate)[0]: + return "" + separators = {"/", "\\", os.sep, os.altsep or os.sep} + if any(sep in candidate for sep in separators): + return "" + if any(ord(ch) < 32 or ord(ch) == 127 for ch in candidate): + return "" + return candidate + + +def _safe_live_path(path: object) -> str: + """Return an absolute, traversal-free ``*.md`` target path, else "".""" + if not isinstance(path, str) or not path.strip(): + return "" + candidate = path.strip() + if candidate.startswith("~") or not os.path.isabs(candidate): + return "" + if os.path.normpath(candidate) != candidate: + return "" + if not candidate.endswith(".md"): + return "" + return candidate + + +def proposal_filename(skill_name: str) -> str: + """Staged filename for one skill's proposal (unique per skill name).""" + return f"proposed_SKILL.{skill_name}.md" + + +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) + 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()) + os.replace(tmp, path) + except BaseException: + if os.path.exists(tmp): + os.unlink(tmp) + raise + + +def skill_proposal_rows(proposals: Sequence[SkillProposal]) -> List[Dict[str, Any]]: + """Validate proposals and return their manifest rows, in input order. + + Raises :class:`StagingError` on an unusable skill name, an unsafe live target + path, or a collision on either the skill name or the live path: a night must + never stage two skills into one file or point a proposal at the wrong one. + """ + rows: List[Dict[str, Any]] = [] + seen_paths: Dict[str, str] = {} + for proposal in proposals: + name = _safe_skill_name(proposal.skill_name) + if not name: + raise StagingError(f"unsafe skill name for staging: {proposal.skill_name!r}") + live = _safe_live_path(proposal.live_skill_path) + if not live: + raise StagingError( + f"unsafe live skill path for {name!r}: {proposal.live_skill_path!r}" + ) + if any(row["skill_name"] == name for row in rows): + raise StagingError(f"duplicate skill name in staging fan-out: {name!r}") + if live in seen_paths: + raise StagingError( + f"skills {seen_paths[live]!r} and {name!r} target the same file: {live}" + ) + seen_paths[live] = name + rows.append({ + "skill_name": name, + "proposed_file": proposal_filename(name), + "live_skill_path": live, + }) + return rows + + +def write_skill_proposals( + out_dir: str, proposals: Sequence[SkillProposal] +) -> List[Dict[str, Any]]: + """Stage one uniquely named proposal file per skill; return manifest rows. + + Every proposal is validated before anything is written, so a rejected + fan-out leaves no partial files behind. + """ + rows = skill_proposal_rows(proposals) + if not rows: + return rows + 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) + return rows + + def _ts_dir() -> str: return time.strftime("%Y%m%d-%H%M%S", time.localtime()) @@ -279,16 +399,23 @@ def write_staging( live_memory_path: str, report_md: str, out_dir: str = "", + skill_proposals: Sequence[SkillProposal] = (), ) -> str: """Write proposals + report into staging// and return that path. ``out_dir`` lets the cycle pre-create the night's staging folder at cycle START, so incremental artifacts (evidence.jsonl) accumulate in the same place the report lands. + + ``skill_proposals`` stages one extra uniquely named file and manifest row per + skill for a multi-skill night. Left empty, the staging layout and manifest + are exactly the legacy single-proposal ones. """ out = out_dir or os.path.join(staging_root(project), _ts_dir()) os.makedirs(out, exist_ok=True) + skill_rows = write_skill_proposals(out, skill_proposals) + manifest = { "live_skill_path": live_skill_path, "live_memory_path": live_memory_path, @@ -296,6 +423,8 @@ def write_staging( "has_memory": proposed_memory is not None, "accepted": report.accepted, } + if skill_rows: + manifest["skills"] = skill_rows if proposed_skill is not None: with open(os.path.join(out, "proposed_SKILL.md"), "w", encoding="utf-8") as f: f.write(proposed_skill) diff --git a/tests/test_sleep_staging_fanout.py b/tests/test_sleep_staging_fanout.py new file mode 100644 index 00000000..88b9527e --- /dev/null +++ b/tests/test_sleep_staging_fanout.py @@ -0,0 +1,169 @@ +"""Tests for per-skill staging fan-out (issue #120). + +Pure-stdlib (unittest), hermetic (tmpdir only), no API key, no network. +Run: python -m pytest tests/test_sleep_staging_fanout.py +""" +from __future__ import annotations + +import json +import os +import tempfile +import unittest + +from skillopt_sleep.staging import ( + SkillProposal, + StagingError, + proposal_filename, + skill_proposal_rows, + write_skill_proposals, + write_staging, +) +from skillopt_sleep.types import SleepReport + + +def _proposal(name="example-skill", body="# example\n", live=None, root="/tmp/live"): + if live is None: + live = os.path.join(root, name, "SKILL.md") + return SkillProposal(name, body, live) + + +def _report(): + return SleepReport(night=1, project="/repo/example", accepted=True, + gate_action="accept_new_best") + + +class TestSkillProposalRows(unittest.TestCase): + def test_one_row_per_skill_in_order(self): + rows = skill_proposal_rows([_proposal("alpha"), _proposal("beta")]) + self.assertEqual([r["skill_name"] for r in rows], ["alpha", "beta"]) + self.assertEqual([r["proposed_file"] for r in rows], + ["proposed_SKILL.alpha.md", "proposed_SKILL.beta.md"]) + self.assertEqual(rows[0]["live_skill_path"], "/tmp/live/alpha/SKILL.md") + + def test_filenames_are_unique_per_skill(self): + self.assertNotEqual(proposal_filename("alpha"), proposal_filename("beta")) + + def test_duplicate_skill_name_is_refused(self): + with self.assertRaises(StagingError): + skill_proposal_rows([_proposal("alpha"), + _proposal("alpha", live="/tmp/other/SKILL.md")]) + + def test_two_skills_targeting_one_file_are_refused(self): + shared = "/tmp/live/shared/SKILL.md" + with self.assertRaises(StagingError): + skill_proposal_rows([_proposal("alpha", live=shared), + _proposal("beta", live=shared)]) + + def test_unsafe_skill_names_are_refused(self): + for bad in ["", " ", ".", "..", "../escape", "a/b", "a\\b", "/abs", + "~home", "bad\nname"]: + with self.assertRaises(StagingError, msg=bad): + skill_proposal_rows([_proposal(bad)]) + + def test_unsafe_live_paths_are_refused(self): + for bad in ["", "relative/SKILL.md", "~/skills/a/SKILL.md", + "/tmp/live/../../etc/SKILL.md", "/tmp/live/a/SKILL.txt"]: + with self.assertRaises(StagingError, msg=bad): + skill_proposal_rows([_proposal("alpha", live=bad)]) + + +class TestWriteSkillProposals(unittest.TestCase): + def test_writes_one_file_per_skill(self): + with tempfile.TemporaryDirectory() as tmp: + rows = write_skill_proposals(tmp, [ + _proposal("alpha", "# alpha\n"), + _proposal("beta", "# beta\n"), + ]) + self.assertEqual(sorted(os.listdir(tmp)), + ["proposed_SKILL.alpha.md", "proposed_SKILL.beta.md"]) + with open(os.path.join(tmp, rows[0]["proposed_file"]), encoding="utf-8") as f: + self.assertEqual(f.read(), "# alpha\n") + + def test_no_proposals_writes_nothing(self): + with tempfile.TemporaryDirectory() as tmp: + self.assertEqual(write_skill_proposals(tmp, []), []) + self.assertEqual(os.listdir(tmp), []) + + def test_rejected_fan_out_leaves_no_partial_files(self): + with tempfile.TemporaryDirectory() as tmp: + with self.assertRaises(StagingError): + write_skill_proposals(tmp, [_proposal("alpha"), _proposal("../escape")]) + self.assertEqual(os.listdir(tmp), []) + + def test_writes_leave_no_temporary_files_behind(self): + with tempfile.TemporaryDirectory() as tmp: + write_skill_proposals(tmp, [_proposal("alpha")]) + self.assertEqual([n for n in os.listdir(tmp) if n.startswith(".tmp-")], []) + + def test_rewrite_replaces_content_atomically(self): + with tempfile.TemporaryDirectory() as tmp: + write_skill_proposals(tmp, [_proposal("alpha", "# first\n")]) + write_skill_proposals(tmp, [_proposal("alpha", "# second\n")]) + path = os.path.join(tmp, proposal_filename("alpha")) + with open(path, encoding="utf-8") as f: + self.assertEqual(f.read(), "# second\n") + self.assertEqual(sorted(os.listdir(tmp)), [proposal_filename("alpha")]) + + +class TestWriteStagingCompatibility(unittest.TestCase): + def _manifest(self, out): + with open(os.path.join(out, "manifest.json"), encoding="utf-8") as f: + return json.load(f) + + def test_legacy_layout_when_multi_skill_is_unused(self): + with tempfile.TemporaryDirectory() as tmp: + out = write_staging( + tmp, report=_report(), proposed_skill="# skill\n", + proposed_memory="# memory\n", + 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( + sorted(os.listdir(out)), + ["manifest.json", "proposed_CLAUDE.md", "proposed_SKILL.md", + "report.json", "report.md"], + ) + manifest = self._manifest(out) + self.assertNotIn("skills", manifest) + self.assertTrue(manifest["has_skill"]) + + def test_fan_out_adds_files_and_manifest_rows(self): + with tempfile.TemporaryDirectory() as tmp: + live_root = os.path.join(tmp, "live") + out = write_staging( + tmp, report=_report(), proposed_skill=None, proposed_memory=None, + live_skill_path=os.path.join(live_root, "SKILL.md"), + live_memory_path=os.path.join(live_root, "CLAUDE.md"), + report_md="# report\n", + skill_proposals=[ + _proposal("alpha", "# alpha\n", root=live_root), + _proposal("beta", "# beta\n", root=live_root), + ], + ) + self.assertEqual( + sorted(os.listdir(out)), + ["manifest.json", "proposed_SKILL.alpha.md", "proposed_SKILL.beta.md", + "report.json", "report.md"], + ) + rows = self._manifest(out)["skills"] + 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")) + + def test_unsafe_fan_out_writes_no_manifest(self): + with tempfile.TemporaryDirectory() as tmp: + with self.assertRaises(StagingError): + write_staging( + tmp, report=_report(), proposed_skill=None, 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", + skill_proposals=[_proposal("alpha", live="relative/SKILL.md")], + ) + for root, _dirs, files in os.walk(tmp): + self.assertNotIn("manifest.json", files, root) + + +if __name__ == "__main__": + unittest.main() From ec112c368da9bb4c83db6e5a80958fd866061d10 Mon Sep 17 00:00:00 2001 From: Dan Baciu Date: Thu, 6 Aug 2026 07:28:41 +0400 Subject: [PATCH 2/5] fix(sleep): refuse proposals that collide on a case-insensitive filesystem Staging two skills whose names differ only by case silently destroyed one of them. write_skill_proposals returned two manifest rows while leaving one file on disk: proposed_SKILL.Research.md, named for the first skill and containing the second skill's document. Reproduced on macOS; Windows behaves the same. That is precisely what skill_proposal_rows promises never to happen -- "a night must never stage two skills into one file or point a proposal at the wrong one" -- and it did both at once. The duplicate check compared skill names exactly, so Research and research passed it, and only the filesystem merged them afterwards. Staged filenames are now compared case-insensitively and a collision raises, matching how every other collision in this function is handled. Skill names themselves stay case-sensitive: the pair is legal on Linux, but the proposals share one staging directory, so refusing is the conservative reading of the promise rather than inventing a disambiguating filename. Two tests: the pair is refused, and a second that asserts the filesystem outcome directly -- staged file count must equal manifest row count -- so if the refusal is ever relaxed the loss is caught rather than the intent. --- skillopt_sleep/staging.py | 24 +++++++++++++++++++++--- tests/test_sleep_staging_fanout.py | 26 ++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/skillopt_sleep/staging.py b/skillopt_sleep/staging.py index bca36778..99d5fb8c 100644 --- a/skillopt_sleep/staging.py +++ b/skillopt_sleep/staging.py @@ -308,11 +308,21 @@ def skill_proposal_rows(proposals: Sequence[SkillProposal]) -> List[Dict[str, An """Validate proposals and return their manifest rows, in input order. Raises :class:`StagingError` on an unusable skill name, an unsafe live target - path, or a collision on either the skill name or the live path: a night must - never stage two skills into one file or point a proposal at the wrong one. + path, or a collision on the skill name, the staged filename, or the live + path: a night must never stage two skills into one file or point a proposal + at the wrong one. + + Staged filenames are compared case-insensitively. Skill names are + case-sensitive, so ``Research`` and ``research`` are two different skills on + a case-sensitive filesystem — but their proposal files land in one staging + directory, and on macOS and Windows that directory is case-insensitive, so + the second write silently replaces the first and the manifest then points a + surviving filename at another skill's content. Refusing the pair is the + conservative reading of the promise above. """ rows: List[Dict[str, Any]] = [] seen_paths: Dict[str, str] = {} + seen_files: Dict[str, str] = {} for proposal in proposals: name = _safe_skill_name(proposal.skill_name) if not name: @@ -324,14 +334,22 @@ def skill_proposal_rows(proposals: Sequence[SkillProposal]) -> List[Dict[str, An ) if any(row["skill_name"] == name for row in rows): raise StagingError(f"duplicate skill name in staging fan-out: {name!r}") + proposed_file = proposal_filename(name) + file_key = proposed_file.lower() + if file_key in seen_files: + raise StagingError( + f"skills {seen_files[file_key]!r} and {name!r} stage to the same " + f"file on a case-insensitive filesystem: {proposed_file}" + ) if live in seen_paths: raise StagingError( f"skills {seen_paths[live]!r} and {name!r} target the same file: {live}" ) seen_paths[live] = name + seen_files[file_key] = name rows.append({ "skill_name": name, - "proposed_file": proposal_filename(name), + "proposed_file": proposed_file, "live_skill_path": live, }) return rows diff --git a/tests/test_sleep_staging_fanout.py b/tests/test_sleep_staging_fanout.py index 88b9527e..a2e752a9 100644 --- a/tests/test_sleep_staging_fanout.py +++ b/tests/test_sleep_staging_fanout.py @@ -48,6 +48,32 @@ def test_duplicate_skill_name_is_refused(self): skill_proposal_rows([_proposal("alpha"), _proposal("alpha", live="/tmp/other/SKILL.md")]) + def test_names_differing_only_by_case_are_refused(self): + # Skill names are case-sensitive, but every proposal lands in one + # staging directory — and on macOS and Windows that directory is + # case-insensitive. Before this guard, staging "Research" then + # "research" produced two manifest rows but one file on disk, named + # after the first skill and containing the second one's document. + with self.assertRaises(StagingError): + skill_proposal_rows([_proposal("Research"), + _proposal("research", live="/tmp/other/SKILL.md")]) + + def test_case_differing_proposals_never_lose_a_staged_file(self): + # Guards the guard: if the refusal above is ever relaxed, this asserts + # the actual filesystem outcome rather than the intent. + with tempfile.TemporaryDirectory() as out: + try: + rows = write_skill_proposals( + out, + [_proposal("Research"), + _proposal("research", live="/tmp/other/SKILL.md")], + ) + except StagingError: + return # refused up front, which is the desired behaviour + staged = [f for f in os.listdir(out) if f.startswith("proposed_")] + self.assertEqual(len(staged), len(rows), + "a manifest row exists whose staged file was overwritten") + def test_two_skills_targeting_one_file_are_refused(self): shared = "/tmp/live/shared/SKILL.md" with self.assertRaises(StagingError): From 7084a8ff5d09af6622439634ca1908a3140fdaeb Mon Sep 17 00:00:00 2001 From: Dan Baciu Date: Thu, 6 Aug 2026 08:19:01 +0400 Subject: [PATCH 3/5] =?UTF-8?q?fix(sleep):=20address=20staging=20review=20?= =?UTF-8?q?=E2=80=94=20four=20verified=20defects?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each was reproduced before changing anything. - The live-path collision check was case-sensitive, so /x/A.md and /x/a.md passed it and two skills could overwrite each other's live document. Note os.path.normcase is NOT the fix: it only folds case on Windows, so it is a no-op on the macOS box where the collision is equally real. Keyed on casefold() instead, matching the staged-filename check. - write_skill_proposals iterated `proposals` twice. The annotation says Sequence but nothing enforces it, and a generator was drained by validation, leaving the write loop empty: measured rows=2, files=0 — a complete manifest for files that never existed. Materialised once at the top. - _safe_skill_name accepted characters Windows cannot store (: * ? " < > |) and trailing dots. Those became filenames and failed with an OSError from inside the write rather than a StagingError naming the skill. A trailing SPACE needed no guard — the name is stripped before validation. - _safe_live_path required input == normpath(input), which rejected duplicate separators and every forward-slash absolute path on Windows. It now rejects traversal on the raw input first, then normalises. That ordering matters and the existing suite proved it: normalising first resolves /live/../../etc/SKILL.md to /etc/SKILL.md with no ".." left to catch, turning the traversal guard into a traversal helper. Also switched the filename key from lower() to casefold() for the Unicode pairs lower() leaves distinct. --- skillopt_sleep/staging.py | 46 +++++++++++++++++++++++++----- tests/test_sleep_staging_fanout.py | 34 ++++++++++++++++++++++ 2 files changed, 73 insertions(+), 7 deletions(-) diff --git a/skillopt_sleep/staging.py b/skillopt_sleep/staging.py index 99d5fb8c..49d36179 100644 --- a/skillopt_sleep/staging.py +++ b/skillopt_sleep/staging.py @@ -265,6 +265,13 @@ def _safe_skill_name(name: object) -> str: return "" if any(ord(ch) < 32 or ord(ch) == 127 for ch in candidate): return "" + # The name becomes a filename, so reject what Windows cannot store. Without + # this the write fails with an OSError from deep inside staging instead of + # a StagingError naming the offending skill. + if any(ch in candidate for ch in ':*?"<>|'): + return "" + if candidate[-1] in {".", " "}: + return "" return candidate @@ -272,10 +279,22 @@ def _safe_live_path(path: object) -> str: """Return an absolute, traversal-free ``*.md`` target path, else "".""" if not isinstance(path, str) or not path.strip(): return "" - candidate = path.strip() - if candidate.startswith("~") or not os.path.isabs(candidate): + raw = path.strip() + if raw.startswith("~"): + return "" + # Reject traversal on the RAW input, before normalising. Normalising first + # would silently resolve "/live/../../etc/SKILL.md" into "/etc/SKILL.md" + # and then accept it, because no ".." survives the collapse -- turning a + # traversal guard into a traversal helper. + if any(part in {os.curdir, os.pardir} for part in raw.replace("\\", "/").split("/")): return "" - if os.path.normpath(candidate) != candidate: + # Only then normalise, so a caller is not forced to hand over an already + # canonical string. The old form demanded input == normpath(input), which + # rejected benign duplicate separators and every forward-slash absolute + # path on Windows (normpath rewrites those to backslashes, so a safe path + # never matched itself). + candidate = os.path.normpath(raw) + if not os.path.isabs(candidate): return "" if not candidate.endswith(".md"): return "" @@ -335,17 +354,25 @@ def skill_proposal_rows(proposals: Sequence[SkillProposal]) -> List[Dict[str, An if any(row["skill_name"] == name for row in rows): raise StagingError(f"duplicate skill name in staging fan-out: {name!r}") proposed_file = proposal_filename(name) - file_key = proposed_file.lower() + # casefold, not lower: it folds Unicode pairs lower() leaves distinct, + # which is the comparison a case-insensitive filesystem actually makes. + file_key = proposed_file.casefold() if file_key in seen_files: raise StagingError( f"skills {seen_files[file_key]!r} and {name!r} stage to the same " f"file on a case-insensitive filesystem: {proposed_file}" ) - if live in seen_paths: + # Same reasoning for the live target: /x/A.md and /x/a.md are one file + # on macOS and Windows, so an exact-string check lets two skills + # overwrite each other's live document. casefold rather than + # os.path.normcase: normcase only folds case on Windows, so it is a + # no-op on the macOS box where the collision is just as real. + live_key = live.casefold() + if live_key in seen_paths: raise StagingError( - f"skills {seen_paths[live]!r} and {name!r} target the same file: {live}" + f"skills {seen_paths[live_key]!r} and {name!r} target the same file: {live}" ) - seen_paths[live] = name + seen_paths[live_key] = name seen_files[file_key] = name rows.append({ "skill_name": name, @@ -363,6 +390,11 @@ def write_skill_proposals( Every proposal is validated before anything is written, so a rejected fan-out leaves no partial files behind. """ + # Materialise once. The annotation says Sequence but nothing enforces it, + # and a generator would be drained by the validation pass — leaving the + # write loop with nothing to iterate and returning a full set of manifest + # rows for files that were never created. + proposals = list(proposals) rows = skill_proposal_rows(proposals) if not rows: return rows diff --git a/tests/test_sleep_staging_fanout.py b/tests/test_sleep_staging_fanout.py index a2e752a9..dcc6dbb3 100644 --- a/tests/test_sleep_staging_fanout.py +++ b/tests/test_sleep_staging_fanout.py @@ -74,6 +74,40 @@ def test_case_differing_proposals_never_lose_a_staged_file(self): self.assertEqual(len(staged), len(rows), "a manifest row exists whose staged file was overwritten") + def test_live_paths_differing_only_by_case_are_refused(self): + # os.path.normcase would not catch this: it only folds case on Windows, + # so it is a no-op on the macOS filesystem where /x/A.md and /x/a.md + # are nevertheless the same file. + with self.assertRaises(StagingError): + skill_proposal_rows([_proposal("alpha", live="/tmp/live/A.md"), + _proposal("beta", live="/tmp/live/a.md")]) + + def test_a_generator_of_proposals_still_writes_every_file(self): + # The annotation says Sequence but nothing enforces it. Validation used + # to drain a generator, leaving the write loop empty and returning a + # full set of manifest rows for files that were never created. + with tempfile.TemporaryDirectory() as out: + gen = (_proposal(n, live=f"/tmp/live/{n}/SKILL.md") for n in ("alpha", "beta")) + rows = write_skill_proposals(out, gen) + staged = [f for f in os.listdir(out) if f.startswith("proposed_")] + self.assertEqual(len(staged), len(rows)) + self.assertEqual(len(rows), 2) + + def test_names_windows_cannot_store_are_refused_cleanly(self): + # These reach the filesystem as a filename. Without an explicit guard + # they raise OSError from inside the write instead of a StagingError + # naming the offending skill. + for bad in ["a:b", "a*b", "a?b", 'a"b', "ab", "a|b", "trailing."]: + with self.assertRaises(StagingError, msg=bad): + skill_proposal_rows([_proposal(bad)]) + + def test_absolute_paths_needing_normalisation_are_accepted(self): + # Requiring the input to already equal normpath() rejected safe paths: + # duplicate separators everywhere, and every forward-slash absolute + # path on Windows. Normalising first keeps the traversal guard. + rows = skill_proposal_rows([_proposal("alpha", live="/tmp/live//alpha/SKILL.md")]) + self.assertEqual(rows[0]["live_skill_path"], os.path.normpath("/tmp/live/alpha/SKILL.md")) + def test_two_skills_targeting_one_file_are_refused(self): shared = "/tmp/live/shared/SKILL.md" with self.assertRaises(StagingError): From 74e904efac8ba3b18d2e4a3da251a00a207e74a4 Mon Sep 17 00:00:00 2001 From: Dan Baciu Date: Thu, 6 Aug 2026 08:42:58 +0400 Subject: [PATCH 4/5] refactor(sleep): type the proposal parameters as Iterable, not Sequence The previous commit made write_skill_proposals materialise its input so a generator survives validation, and the suite now passes one deliberately. That left the Sequence annotation describing a narrower contract than the code actually honours, which misleads type checkers and IDEs. Widened the three proposal parameters to Iterable[SkillProposal] and fixed the comment that still explained the old Sequence-vs-reality mismatch. --- skillopt_sleep/staging.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/skillopt_sleep/staging.py b/skillopt_sleep/staging.py index 49d36179..a3e4a314 100644 --- a/skillopt_sleep/staging.py +++ b/skillopt_sleep/staging.py @@ -14,7 +14,7 @@ import tempfile import time from dataclasses import dataclass -from typing import Any, Dict, List, Optional, Sequence +from typing import Any, Dict, Iterable, List, Optional, Sequence from skillopt_sleep.types import SleepReport @@ -323,7 +323,7 @@ def _write_atomic(path: str, text: str) -> None: raise -def skill_proposal_rows(proposals: Sequence[SkillProposal]) -> List[Dict[str, Any]]: +def skill_proposal_rows(proposals: Iterable[SkillProposal]) -> List[Dict[str, Any]]: """Validate proposals and return their manifest rows, in input order. Raises :class:`StagingError` on an unusable skill name, an unsafe live target @@ -383,17 +383,17 @@ def skill_proposal_rows(proposals: Sequence[SkillProposal]) -> List[Dict[str, An def write_skill_proposals( - out_dir: str, proposals: Sequence[SkillProposal] + out_dir: str, proposals: Iterable[SkillProposal] ) -> List[Dict[str, Any]]: """Stage one uniquely named proposal file per skill; return manifest rows. Every proposal is validated before anything is written, so a rejected fan-out leaves no partial files behind. """ - # Materialise once. The annotation says Sequence but nothing enforces it, - # and a generator would be drained by the validation pass — leaving the - # write loop with nothing to iterate and returning a full set of manifest - # rows for files that were never created. + # Materialise once. The signature accepts any Iterable, so a generator is + # legal input — and it would otherwise be drained by the validation pass, + # leaving the write loop with nothing to iterate and returning a full set + # of manifest rows for files that were never created. proposals = list(proposals) rows = skill_proposal_rows(proposals) if not rows: @@ -449,7 +449,7 @@ def write_staging( live_memory_path: str, report_md: str, out_dir: str = "", - skill_proposals: Sequence[SkillProposal] = (), + skill_proposals: Iterable[SkillProposal] = (), ) -> str: """Write proposals + report into staging// and return that path. From 60991b1c2017ba3d72ccc59cd8b5562806d43704 Mon Sep 17 00:00:00 2001 From: Dan Baciu Date: Fri, 7 Aug 2026 05:09:30 +0400 Subject: [PATCH 5/5] style(sleep): drop the now-unused Sequence import Left behind when the proposal parameters were widened to Iterable. It appears only in the import line and trips unused-import linters. --- skillopt_sleep/staging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skillopt_sleep/staging.py b/skillopt_sleep/staging.py index a3e4a314..41799a9b 100644 --- a/skillopt_sleep/staging.py +++ b/skillopt_sleep/staging.py @@ -14,7 +14,7 @@ import tempfile import time from dataclasses import dataclass -from typing import Any, Dict, Iterable, List, Optional, Sequence +from typing import Any, Dict, Iterable, List, Optional from skillopt_sleep.types import SleepReport