feat(sleep): stage one proposal and manifest row per skill - #188
feat(sleep): stage one proposal and manifest row per skill#188bogdanbaciu21 wants to merge 1 commit into
Conversation
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 microsoft#120
There was a problem hiding this comment.
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
SkillProposalandStagingError, 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.
| 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, | ||
| }) |
| 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") |
|
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. |
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)andStagingError(aValueError);skill_proposal_rows(proposals)validates and returns one manifest row perskill (
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-normalizedtraversal, non-
.md), and collisions on either the skill name or the live path;write_skill_proposals(out_dir, proposals)validates everything beforewriting, then writes
proposed_SKILL.<skill>.mdper skill through atemp-file +
os.replaceso 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_proposalsunset the staged files and manifest are byte-for-byte theprevious 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 passedpython -m pytest -q→ 570 passed, 7 skipped (basemainat8304e6c: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