feat(sleep): group mined tasks by skill hint with catch-all fallback - #184
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds the remaining “grouping primitive” needed to route mined sleep-engine tasks per skill, using harvested Skill tool invocations as evidence, while keeping ambiguous or hint-less tasks in the existing managed-skill catch-all.
Changes:
- Add
SessionDigest.skills_usedharvesting from ClaudeSkilltool-use blocks and propagate a derivedTaskRecord.skill_hintinto both heuristic and LLM miners. - Introduce
mine.group_tasks_by_skill_hint(tasks, managed_skill_name)to deterministically group (and deduplicate) tasks by unambiguous skill hint, with a managed-skill fallback. - Add focused unit tests covering harvesting, hint propagation, dedup semantics, and grouping behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_sleep_skill_hint.py | Adds unit tests for skill_hint propagation, serialization, and dedup behavior. |
| tests/test_sleep_skill_harvest.py | Adds unit tests for harvesting Claude Skill tool invocations into skills_used. |
| tests/test_sleep_engine.py | Adds coverage for group_tasks_by_skill_hint behavior and ordering guarantees. |
| skillopt_sleep/types.py | Extends core dataclasses with SessionDigest.skills_used and TaskRecord.skill_hint. |
| skillopt_sleep/mine.py | Adds session_skill_hint, propagates hints in heuristic mining, extends dedup_tasks, and introduces group_tasks_by_skill_hint. |
| skillopt_sleep/llm_miner.py | Propagates session-derived skill_hint into LLM-mined tasks. |
| skillopt_sleep/harvest.py | Extracts Skill targets from tool-use blocks and persists them into digests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # ambiguous, so drop back to the catch-all path | ||
| if not ex.skill_hint: | ||
| ex.skill_hint = t.skill_hint | ||
| elif t.skill_hint and t.skill_hint != ex.skill_hint: | ||
| ex.skill_hint = "" |
|
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 group_tasks_by_skill_hint: a deterministic helper that groups mined tasks by their optional skill hint in first-seen order, merges duplicate task ids once, and routes missing, conflicting, or partial hint evidence to the configured managed skill. Refs microsoft#120
8eb753e to
48833ed
Compare
There was a problem hiding this comment.
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/mine.py:262
group_tasks_by_skill_hintcurrently returnsTaskRecords whoseskill_hintcan disagree with the grouping decision (e.g., blank/whitespace hints remain truthy, and mixed hinted+unhinted observations still dedup to a non-emptyskill_hint). It also mutates the original inputTaskRecordobjects viadedup_tasks, which conflicts with the PR’s stated “side-effect-free” intent. Consider normalizing hints (strip, treat blanks as empty), deduping on copies, and then forcingtask.skill_hintto the resolved hint actually used for grouping so consumers can’t observe an inconsistent hint.
observed: dict = {}
for t in tasks:
observed.setdefault(t.id, set()).add((t.skill_hint or "").strip())
groups: Dict[str, List[TaskRecord]] = {}
Yif-Yang
left a comment
There was a problem hiding this comment.
Thanks for keeping this slice focused and rebasing it onto current main. The grouping primitive is useful, deterministic, and well covered by focused tests. We will follow up with a small normalization/copying hardening patch so returned records remain consistent without mutating caller-owned inputs.
Summary
Third review-sized slice of #120: the grouping primitive, so mined tasks can be
routed per skill while everything without clear evidence stays in the existing
managed-skill catch-all.
Adds
mine.group_tasks_by_skill_hint(tasks, managed_skill_name), adeterministic, side-effect-free helper returning ordinary insertion-ordered
dicts/lists:
dedup_tasksmerge semantics(source sessions unioned, best outcome kept);
on the same non-empty hint;
unhinted observations for one ID all route to
managed_skill_name;managed_skill_namelands in that one group, never a duplicate.The grouping rule is deliberately the same order-independent "full set of
non-empty hints" rule you asked for in the #183 review, so grouping and
dedup_taskscannot disagree about what an ambiguous ID means.Nothing else changes:
mine(),filter_tasks_for_target(), cycle, gates,staging, and adoption are untouched, and the caller supplies the configured
managed-skill name rather than the helper resolving paths or reading skill files.
Rebased onto current
main(e7014cd); the earlier stacked parents (#182, #183)have landed, so this is now a single commit against
main.Tests
Base is unmodified
mainate7014cd, same machine, same interpreter(CPython 3.12.13, macOS):
pytest tests/test_sleep_engine.py(this branch)pytest tests(this branch)pytest tests(basemain@e7014cd)ruff check .The 2 failures are
tests/test_superpowers_scenarios.py::TestOverlayIntegration(
test_skill_copied_to_correct_path,test_source_checkout_unchanged). They arethe macOS
/var→/private/varsymlink artifact and reproduce unchanged onuntouched
main, so they are not regressions from this branch.Covered by the new tests: empty input, legacy hint-less tasks, blank hint,
interleaved
alpha/beta/alphaordering, duplicate ID merge, conflictinghints, partial hint evidence, and a hint equal to the managed skill name.
Refs #120