feat(sleep): resolve discovered skill names through bounded local roots - #185
Closed
bogdanbaciu21 wants to merge 1 commit into
Closed
feat(sleep): resolve discovered skill names through bounded local roots#185bogdanbaciu21 wants to merge 1 commit into
bogdanbaciu21 wants to merge 1 commit into
Conversation
Add a read-only resolver that normalizes an untrusted skill name, matches it only inside documented skill roots, refuses traversal and symlink escapes, and distinguishes found, missing, ambiguous, and rejected outcomes. Refs microsoft#120
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new, security-focused resolver for mapping untrusted “skill name” strings observed in transcripts to a bounded local SKILL.md path under documented Claude skill roots, without reading or modifying any skill content. This supports the broader Issue #120 direction by providing a safe building block for later per-skill fan-out.
Changes:
- Added
skillopt_sleep/skill_resolver.pywith skill-name normalization, documented root discovery, and a resolution API that distinguishesfound/missing/ambiguous/rejected. - Added
tests/test_sleep_skill_resolver.pycovering normalization rules, root precedence, ambiguity vs missing, traversal rejection, and symlink escape containment.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| skillopt_sleep/skill_resolver.py | Implements bounded skill resolution (normalization, root discovery, containment checks, and statusful results). |
| tests/test_sleep_skill_resolver.py | Adds hermetic unit tests validating resolver behavior and safety properties (including symlink escape cases). |
Comments suppressed due to low confidence (2)
skillopt_sleep/skill_resolver.py:137
- If
SkillResolution.candidatesis meant to be immutable,resolve_skill()should return a tuple rather than reusing the mutablematcheslist (otherwise callers can still mutate the list via the returned object).
if len(matches) > 1:
return SkillResolution(
name=normalized,
status=AMBIGUOUS,
candidates=matches,
reason="several skill roots define this skill",
)
return SkillResolution(name=normalized, status=FOUND, path=matches[0], candidates=matches)
tests/test_sleep_skill_resolver.py:75
- If
SkillResolution.candidatesis changed to a tuple for immutability, update this assertion accordingly (expect a tuple of real paths).
self.assertEqual(res.status, AMBIGUOUS)
self.assertEqual(res.path, "")
self.assertEqual(res.candidates,
[os.path.realpath(first), os.path.realpath(second)])
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+74
to
+76
| claude_home = os.path.abspath(os.path.expanduser(str(getattr(cfg, "claude_home", "")))) | ||
| if not claude_home: | ||
| return [] |
Comment on lines
+103
to
+104
| if os.path.commonpath([real_root, skill_file]) != real_root: | ||
| return "" |
| name: str | ||
| status: str | ||
| path: str = "" | ||
| candidates: List[str] = field(default_factory=list) |
Comment on lines
+120
to
+124
| path = _write_skill(tmp, "example-skill", "# original\n") | ||
| before = (os.stat(path).st_size, open(path, encoding="utf-8").read()) | ||
| resolve_skill("example-skill", [tmp]) | ||
| self.assertEqual((os.stat(path).st_size, | ||
| open(path, encoding="utf-8").read()), before) |
| res = resolve_skill("example-skill", [tmp]) | ||
| self.assertEqual(res.status, MISSING) | ||
| self.assertEqual(res.path, "") | ||
| self.assertEqual(res.candidates, []) |
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
Fourth review-sized slice of #120. Skill names observed in transcripts are
untrusted strings, so before anything can act per skill there needs to be a
narrow, read-only way to turn a name into a local
SKILL.md.Adds
skillopt_sleep/skill_resolver.py:normalize_skill_name(name)accepts a single safe path segment only —whitespace-trimmed, case and punctuation preserved — and rejects empty names,
./.., separators, absolute/drive/~paths, and control characters;skill_search_roots(cfg)returns the documented roots that exist, in fixedprecedence:
<claude_home>/skills, then<claude_home>/plugins/cache/*/*/skills;resolve_skill(name, roots)returns a frozenSkillResolutionwith statusfound/missing/ambiguous/rejected, so a name defined in no root isa different signal from one defined in several (candidates listed, no winner
guessed).
Containment is checked after symlink resolution: a skill directory or
SKILL.mdthat points outside its root is refused, while a symlinked root itself still
resolves. The resolver only stats and reports — it never reads skill bodies,
writes, creates, or adopts anything — and no existing module or config behavior
changes, so legacy
target_skill_pathhandling is untouched.Tests
python -m pytest -q tests/test_sleep_skill_resolver.py→ 17 passedpython -m pytest -q→ 573 passed, 7 skipped (basemainat8304e6c:556 passed, 7 skipped)
python -m ruff check skillopt_sleep/skill_resolver.py tests/test_sleep_skill_resolver.py→ All checks passed
Covered: normalization accept/reject table, single local skill, missing vs
ambiguous, directory without
SKILL.md, repeated root, traversal, symlinkedskill dir and symlinked
SKILL.mdescaping the root, symlinked root, unchangedskill file after resolution, no roots, config root precedence, and legacy
target_skill_path. All fixtures are hermetic temp dirs with synthetic names.Refs #120