feat(sleep): report per-skill-group gate decisions - #187
Yifan Yang (Yif-Yang) merged 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds per-skill-group gate reporting for multi-skill Sleep nights, while preserving the existing single-skill summary fields for backward compatibility with older report readers.
Changes:
- Introduces
types.SkillGroupReportand addsSleepReport.skill_groups(default empty) so multi-skill nights can report one gate-evidence row per skill. - Adds
multi_skill.skill_group_reports(outcomes)to build per-group report rows from each group’s own consolidation/gate evidence. - Adds/extends deterministic stdlib-only tests covering row defaults, ordering, isolation of evidence, and legacy/single-skill compatibility.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
skillopt_sleep/types.py |
Adds SkillGroupReport and extends SleepReport with skill_groups for backward-compatible serialization. |
skillopt_sleep/multi_skill.py |
Implements group consolidation outcomes and converts outcomes into per-skill report rows. |
tests/test_sleep_multi_skill.py |
Tests independent per-skill-group consolidation behavior and failure/skip isolation. |
tests/test_sleep_skill_group_report.py |
Tests per-skill-group gate row reporting and SleepReport compatibility/serialization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Returns one entry per input group, keyed by skill name and ordered | ||
| first-seen. A group is ``skipped`` when it is unusable (blank name, no | ||
| tasks, repeated name) and ``failed`` when its own consolidation raised; both | ||
| leave every other group's decision untouched. |
| name = (group.skill_name or "").strip() | ||
| if not name: | ||
| out.setdefault("", GroupConsolidation("", SKIPPED, reason="group has no skill name")) | ||
| continue |
|
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. |
Add SkillGroupReport plus SleepReport.skill_groups (default empty) and build the rows from each group's own baseline, candidate score, decision, and reason, so a weak group can neither block nor borrow a strong group's evidence and older single-group report consumers keep working. Refs microsoft#120
… count Two defects in the per-group report, both found by testing the docstring against the code rather than reading it: - skill_group_reports said "one report row per group". It builds one row per entry in `outcomes`, which is keyed by skill name — so groups that collapsed onto a shared key upstream are already one entry and cannot each get a row. Measured: four groups in, two rows out. This is the same claim that was corrected on consolidate_groups in microsoft#186; it had been copied down here. - A group skipped for a blank name reported n_tasks=0 even when it carried tasks. The row is meant to be that group's own evidence, so a zero count misstates why it was dropped — it reads as "empty group" when the real reason was the missing name. The count is now carried onto the skipped row. Both are pinned by tests: one asserting rows track outcomes and stay fewer than the input groups, one asserting a blank-named group still reports its real task count.
184895e to
827cc45
Compare
There was a problem hiding this comment.
🟢 Ready to approve
The changes appear backward-compatible (default-empty skill_groups) and are well-covered by targeted tests validating ordering, skipped/failed behavior, and serialization.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Thank you again — merged! This lands the per-skill reporting contract cleanly while preserving compatibility with existing single-skill reports. To make the feature usable in normal SkillOpt-Sleep runs, could you please continue with a small follow-up that wires That follow-up will turn the schema/helper added here into user-visible runtime reporting. Please feel free to send it whenever you have bandwidth — we would be happy to review it, and we appreciate the careful incremental work. |
ABSOLUTELY! We will get this going nicely. I have a ton of PRs lined up, happy to "unleash" them :) |
Follow-up requested on #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.
Summary
Sixth review-sized slice of #120: make a multi-skill night's gate decisions
legible per skill, without breaking readers that only know single-skill reports.
types.SkillGroupReport: one row per skill with its ownstatus,accepted,gate_action,baseline_score,candidate_score,n_tasks,applied/rejected edit counts, and
reason;SleepReport.skill_groups: List[SkillGroupReport], default empty — asingle-managed-skill night serializes exactly as before plus
"skill_groups": [],and the existing flat summary fields stay authoritative for older consumers;
multi_skill.skill_group_reports(outcomes)builds those rows from each group'sown evidence, in first-seen order;
GroupConsolidationnow recordsn_tasks, so a skipped or failed group canreport how much input it had without a side table.
A skipped or failed group reports its reason with zeroed scores rather than
inheriting a neighbour's numbers, and an accepted group's row is unaffected by a
weak group's rejection — no group can block or borrow another's evidence.
Tests
python -m pytest -q tests/test_sleep_skill_group_report.py→ 8 passedpython -m pytest -q→ 574 passed, 7 skipped (basemainat8304e6c:556 passed, 7 skipped)
python -m ruff check skillopt_sleep/types.py skillopt_sleep/multi_skill.py tests/test_sleep_skill_group_report.py→ All checks passed
Covered: row defaults, strong-vs-weak groups keeping their own scores, first-seen
row order, skipped/failed rows carrying reasons and no borrowed scores, rows from
a real mixed
MockBackendnight, emptyskill_groupson a single-skill report,legacy keyword construction, and serialization alongside the flat summary.
Refs #120