feat(sleep): emit per-skill group rows from the nightly cycle - #208
Open
Bogdan (Dan) Baciu (bogdanbaciu21) wants to merge 6 commits into
Open
feat(sleep): emit per-skill group rows from the nightly cycle#208Bogdan (Dan) Baciu (bogdanbaciu21) wants to merge 6 commits into
Bogdan (Dan) Baciu (bogdanbaciu21) wants to merge 6 commits 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
…ystem 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.
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.
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.
Left behind when the proposal parameters were widened to Iterable. It appears only in the import line and trips unused-import linters.
Follow-up requested on microsoft#187: wire skill_group_reports() into the production cycle so the schema added there becomes user-visible runtime reporting. cycle.py did not import multi_skill at all, so the whole per-skill path was unreachable from a real night. It now groups the mined tasks by skill hint, consolidates each group independently, and persists the rows on SleepReport. Opt-in via multi_skill_report, default off. Each hinted group costs one extra consolidation, so this is a cost decision rather than a free improvement, and it follows the same opt-in shape as slow_update_gate_with_selection. A night whose evidence yields only the catch-all group adds no rows and no calls. The rows also render in report.md, not just report.json. That file is what a human reads before /sleep adopt, so per-skill verdicts belong there; otherwise the reviewer sees one aggregate verdict that no individual skill necessarily earned. One rendering decision worth calling out. A reject_unverified score was measured on the same tasks the edits were derived from -- the comparison consolidate.py declines to certify, noting it is how a reward hack reaches 1.000. Printed bare it reads as an improvement rejected for no reason, so that cell is marked "(unvalidated)". Accepted rows are unmarked. Each group currently starts from the managed document; resolving a hinted group to its own live SKILL.md is the resolver's job and is not wired here. Six tests: off by default, a mixed night with one accepted and one rejected group, independent per-row verdicts and task counts, rows reaching report.json, report.md rendering with the unvalidated marker, and no section when off.
Copilot started reviewing on behalf of
Bogdan (Dan) Baciu (bogdanbaciu21)
August 7, 2026 10:54
View session
Contributor
There was a problem hiding this comment.
Pull request overview
This PR wires per-skill-group reporting into the nightly sleep cycle so that multi-skill nights emit one independent gate-decision row per discovered skill group (opt-in via multi_skill_report), and renders those rows in report.md for reviewer-facing visibility.
Changes:
- Integrates
group_tasks_by_skill_hint()+consolidate_groups()intorun_sleep_cycle()and persists rows ontoSleepReport.skill_groupswhen enabled. - Renders a “Per-skill groups” markdown table in
report.md, including an “(unvalidated)” marker forreject_unverified. - Adds/extends tests covering the opt-in wiring and (separately) staging fan-out manifest/file behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
skillopt_sleep/cycle.py |
Adds opt-in multi-skill consolidation for reporting and renders per-skill group rows into report.md. |
skillopt_sleep/staging.py |
Adds multi-skill proposal fan-out support (validation, atomic writes) and includes optional per-skill rows in the staging manifest. |
tests/test_sleep_engine.py |
Adds TestMultiSkillReportWiring coverage for opt-in behavior, JSON persistence, and report.md rendering. |
tests/test_sleep_staging_fanout.py |
New hermetic unittest suite validating per-skill staging fan-out behaviors and safety checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+232
to
+234
| for g in report.skill_groups: | ||
| name = g.skill_name or "_(no skill name)_" | ||
| if g.status == "consolidated": |
Comment on lines
+285
to
+290
| # 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 "" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #187, as requested there: wire
skill_group_reports()into the production cycle so the schema that PR added becomes user-visible runtime reporting.What was missing
cycle.pydid not importmulti_skillat all, so the per-skill path was unreachable from a real night. It now groups the mined tasks by skill hint, consolidates each group independently, and persists the rows onSleepReport.Opt-in, and why
Enabled with
multi_skill_report, default off. Each hinted group costs one extra consolidation, so this is a cost decision rather than a free improvement — the same opt-in shape asslow_update_gate_with_selection. A night whose evidence yields only the catch-all group adds no rows and makes no extra calls.The mixed night you asked for
One night, three groups, independent verdicts:
research-skillprogramming-skillthin-skillThe rejection is a real gate outcome rather than a contrived fixture: a single-task group has no held-out split, so the gate declines to certify it. The rejected row does not pull down its neighbours.
Rows render in
report.md, not onlyreport.jsonreport.mdis the page a reviewer reads before/sleep adopt, so the per-skill verdicts belong there. Without it the reviewer sees one aggregate verdict that no individual skill necessarily earned.One rendering decision worth flagging for review. A
reject_unverifiedscore was measured on the same tasks the edits were derived from — the comparisonconsolidate.pydeclines to certify, noting in its own comment that this is how a reward hack scores 1.0. Printed bare it reads as an improvement rejected for no reason, so that cell is marked(unvalidated). Accepted rows are unmarked. Happy to change the wording if you would prefer different phrasing.Known limitation
Each group currently starts from the managed document. Resolving a hinted group to its own live
SKILL.mdis the resolver's job (#185) and is deliberately not wired here, to keep this slice small. A row therefore describes what that group's evidence did to the managed skill, not to a separate file.Tests
Six, in
TestMultiSkillReportWiring: off by default; a mixed night with one accepted and one rejected group; independent per-row verdicts and task counts; rows reachingreport.json;report.mdrendering including the unvalidated marker; and no section at all when the feature is off.220 passed / 3 skipped across the sleep suite.