feat(sleep): report per-skill-group gate decisions - #187
Closed
bogdanbaciu21 wants to merge 2 commits into
Closed
Conversation
…ation Add consolidate_groups: each skill group runs through the existing consolidate() with its own tasks, document, and gate decision. Task-less, name-less, repeated, and raising groups are reported instead of aborting the night, shared memory is read-only, and adoption stays explicit. Refs microsoft#120
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
Contributor
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.
Comment on lines
+59
to
+62
| 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. |
Comment on lines
+69
to
+72
| name = (group.skill_name or "").strip() | ||
| if not name: | ||
| out.setdefault("", GroupConsolidation("", SKIPPED, reason="group has no skill name")) | ||
| continue |
Contributor
Author
|
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. |
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.
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