feat(sleep): stage one proposal and manifest row per skill

[Bogdan (Dan) Baciu (bogdanbaciu21)]

Summary

Seventh review-sized slice of #120: let a night stage a proposal per skill,
keeping the staging directory reviewable and the legacy layout intact.

In skillopt_sleep/staging.py:

• SkillProposal(skill_name, proposed_skill, live_skill_path) and
  StagingError (a ValueError);
• skill_proposal_rows(proposals) validates and returns one manifest row per
  skill (skill_name, proposed_file, live_skill_path) in input order,
  refusing unusable skill names (blank, ./.., separators, absolute, ~,
  control chars), unsafe live targets (relative, unexpanded ~, un-normalized
  traversal, non-.md), and collisions on either the skill name or the live path;
• write_skill_proposals(out_dir, proposals) validates everything before
  writing, then writes proposed_SKILL.<skill>.md per skill through a
  temp-file + os.replace so a reader never sees a half-written proposal and no
  .tmp- files are left behind;
• write_staging(..., skill_proposals=()) stages those files and adds a
  "skills" manifest list only when the fan-out is used.

With skill_proposals unset the staged files and manifest are byte-for-byte the
previous single-proposal layout. Because validation happens before the manifest is
written, a refused fan-out leaves a directory that latest_staging()/adopt()
will not pick up. Adoption itself is unchanged.

Tests

• python -m pytest -q tests/test_sleep_staging_fanout.py → 14 passed
• python -m pytest -q → 570 passed, 7 skipped (base main at 8304e6c:
  556 passed, 7 skipped)
• python -m ruff check skillopt_sleep/staging.py tests/test_sleep_staging_fanout.py
  → All checks passed

Covered: row ordering and unique filenames, duplicate name, two skills targeting
one file, unsafe name and path tables, one file per skill, empty fan-out, no
partial files after a rejection, no leftover temp files, atomic rewrite, legacy
layout and manifest, fan-out files plus manifest rows, and no manifest written
when a fan-out is refused.

Refs #120

[Copilot]

Pull request overview

Adds first-pass support for staging multiple per-skill SKILL.md proposals in a single nightly run (fan-out), while preserving the legacy single-proposal staging layout and manifest when fan-out is unused. This advances issue #120’s goal of routing edits to the real skills that were used, by making the staging directory reviewable and unambiguous per skill.

Changes:

• Introduces SkillProposal and StagingError, plus validation helpers to safely stage one proposal file per skill and emit one manifest row per skill.
• Adds atomic per-skill proposal writing (tempfile + os.replace) to prevent half-written staged proposals.
• Adds a new hermetic stdlib test suite covering ordering, collisions, unsafe names/paths, atomic rewrite, and legacy compatibility.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
skillopt_sleep/staging.py	Adds per-skill proposal fan-out validation + atomic writes, and optionally includes per-skill rows in the staging manifest.
tests/test_sleep_staging_fanout.py	Adds comprehensive unit tests for the new fan-out staging behavior and legacy layout compatibility.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Bogdan (Dan) Baciu (bogdanbaciu21)]

Thanks for the welcome on #120. To keep review load bounded and follow the incremental shape Yif-Yang suggested, I'm closing this PR for now and will reopen it once #182 (the harvesting slice) lands or the maintainer asks for the next slice. The branch stays on my fork so this can be re-opened as-is. Happy to restructure if a different cadence is preferred.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (6)

skillopt_sleep/staging.py:348

• The live-path collision check is currently case-sensitive (string equality). On case-insensitive filesystems (Windows/macOS default), two proposals could target the same file with different casing and bypass this guard. Use os.path.normcase() (or equivalent) for the seen-path key.

        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

skillopt_sleep/staging.py:339

• The staged-filename collision check uses proposed_file.lower(). For caseless matching across platforms, casefold() is the safer choice (lower() misses some Unicode case equivalences), and it better matches how macOS/Windows fold names.

        proposed_file = proposal_filename(name)
        file_key = proposed_file.lower()
        if file_key in seen_files:

skillopt_sleep/staging.py:279

• _safe_live_path() rejects any path whose string form differs from os.path.normpath(). On Windows this rejects otherwise-safe absolute paths that use forward slashes (e.g. "/tmp/..." or "C:/..."), and it also rejects benign duplicate separators. Consider checking for traversal components ("."/"..") explicitly and returning the normalized path instead of requiring exact string equality.

    candidate = path.strip()
    if candidate.startswith("~") or not os.path.isabs(candidate):
        return ""
    if os.path.normpath(candidate) != candidate:
        return ""

skillopt_sleep/staging.py:370

• write_skill_proposals() iterates proposals twice (once in skill_proposal_rows(), again in zip(rows, proposals)). If a caller passes an iterator/generator (runtime doesn’t enforce Sequence), the second pass can be empty and silently skip writing files while still returning rows. Materialize proposals once at the top.

    rows = skill_proposal_rows(proposals)
    if not rows:
        return rows
    os.makedirs(out_dir, exist_ok=True)
    for row, proposal in zip(rows, proposals):

skillopt_sleep/staging.py:439

• write_staging() can now write a manifest that contains a non-empty "skills" list while has_skill/has_memory are false (e.g. fan-out-only staging). latest_staging()/cmd_adopt will pick this directory, but staging.adopt() ignores "skills" and will report "no accepted changes" even though skill proposal files exist. Either teach adopt() to apply manifest["skills"], or make fan-out-only staging explicitly non-adoptable until adoption support lands.

    skill_rows = write_skill_proposals(out, skill_proposals)

    manifest = {
        "live_skill_path": live_skill_path,
        "live_memory_path": live_memory_path,

skillopt_sleep/staging.py:267

• _safe_skill_name() is used to generate a staged filename. It currently allows characters that are invalid in Windows filenames (e.g. ":", "*", "?", "<", ">", "|", quotes) and names that end with a dot/space; those will raise an OSError during staging instead of a clean StagingError.

This issue also appears in the following locations of the same file:

• line 275
• line 337
• line 344

    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 ""

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

skillopt_sleep/staging.py:387

• write_skill_proposals (and skill_proposal_rows / write_staging) are typed as taking a Sequence[SkillProposal], but the implementation explicitly supports one-pass iterables (it materializes list(proposals)), and the new test suite passes a generator successfully. For consistency (and to avoid misleading type-checking / IDEs), consider widening these parameters to Iterable[SkillProposal] and updating the nearby comment that currently calls out Sequence.

def write_skill_proposals(
    out_dir: str, proposals: Sequence[SkillProposal]
) -> List[Dict[str, Any]]:

[Bogdan (Dan) Baciu (bogdanbaciu21)]

Apologies for this. I have an azure VM with windows and linux box in fleet, not sure why I did not check comprehensively and run the test suite in all environments. Will fix.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

skillopt_sleep/staging.py:477

• When skill_rows is non-empty you add a "skills" list to the manifest, but adopt() only applies has_skill/has_memory and will silently update nothing for fan-out staging (e.g. when proposed_skill is None). This makes fan-out nights appear adoptable while producing a no-op adoption.

    if skill_rows:
        manifest["skills"] = skill_rows

skillopt_sleep/staging.py:17

• Sequence is imported but not used anywhere in this module (only appears in the import statement), which will trip unused-import checks in some linters and adds noise.

from typing import Any, Dict, Iterable, List, Optional, Sequence

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

tests/test_sleep_staging_fanout.py:41

• This assertion hard-codes POSIX path separators, but skill_proposal_rows() normalizes live_skill_path via os.path.normpath(). On Windows, /tmp/live/alpha/SKILL.md becomes \\tmp\\live\\alpha\\SKILL.md, so this test will fail even though behavior is correct. Compare against os.path.normpath(...) instead to keep the test cross-platform.

        self.assertEqual(rows[0]["live_skill_path"], "/tmp/live/alpha/SKILL.md")