feat(sleep): resolve discovered skill names through bounded local roots - #185
feat(sleep): resolve discovered skill names through bounded local roots#185bogdanbaciu21 wants to merge 5 commits into
Conversation
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.
| claude_home = os.path.abspath(os.path.expanduser(str(getattr(cfg, "claude_home", "")))) | ||
| if not claude_home: | ||
| return [] |
| if os.path.commonpath([real_root, skill_file]) != real_root: | ||
| return "" |
| name: str | ||
| status: str | ||
| path: str = "" | ||
| candidates: List[str] = field(default_factory=list) |
| 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, []) |
|
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. |
9e66675 to
b5adb82
Compare
Yif-Yang
left a comment
Yif-Yang
left a comment
There was a problem hiding this comment.
Thanks for keeping this as a small, read-only slice. I rechecked the current head, including an independent Claude Code + Opus 5.0 pass. The direction is useful, but three filesystem-contract issues should be fixed before we expose this resolver:
- An absent or empty claude_home is passed through abspath(""), which turns it into the current working directory. That can accidentally trust /skills. Please validate the raw value before expanduser/abspath and return no roots when it is empty.
- os.path.commonpath() can raise ValueError for unrelated roots/drives on Windows. _contained_skill_file() should treat that as not contained rather than letting resolution fail.
- Cache discovery currently stops at ///skills. Claude marketplace installations can have a version directory, ////skills>, so the resolver can miss the installed skill it is intended to find. Please derive the supported layout from a real install (or support both bounded layouts) rather than assuming one depth.
All current tests use synthetic temporary directories. Please add one path-layout smoke based on an actual marketplace-installed Claude plugin; a sanitized fixture matching the observed tree is fine. This is not a request for a model benchmark—the important experiment here is validating the real filesystem contract.
Five points from review on microsoft#185, plus one adjacent hardening: - skill_search_roots: guard the blank claude_home BEFORE abspath. os.path.abspath("") returns the CWD, so a config override of claude_home="" turned the working directory into a skill root. The existing `if not claude_home` check could never fire because it ran on the already-absolutised value. - _contained_skill_file: os.path.commonpath raises ValueError when the resolved skill file lands on a different drive (Windows, reachable via symlink). Treat it as "not contained" rather than letting it crash resolution. - SkillResolution.candidates is now Tuple[str, ...] with a () default, so the frozen dataclass is actually immutable; callers can no longer mutate the result in place. - Tests use context managers instead of open(...).read(), so no descriptor is left open (matters on Windows, where an open handle blocks delete). - Test assertions updated for the tuple. Adjacent: plugin-cache discovery now goes through a _listdir helper that swallows OSError, so an unreadable cache directory degrades to "no plugin roots" instead of raising. Previously os.path.isdir() passing did not guarantee os.listdir() would succeed. New tests cover each fix: blank/whitespace/None claude_home, an unreadable plugin cache, tuple immutability, and a commonpath that raises ValueError.
b5adb82 to
719c74c
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 (4)
tests/test_sleep_skill_resolver.py:107
- This test depends on creating a symlinked
SKILL.md, which can raise on systems where symlinks aren’t supported or allowed. Skipping in that case keeps the suite portable while still validating the security behavior where symlinks exist.
os.symlink(elsewhere, os.path.join(root, "example-skill", "SKILL.md"))
tests/test_sleep_skill_resolver.py:115
- Creating a symlinked root may fail on some Windows environments due to symlink privilege restrictions. Consider skipping this test when
os.symlinkisn’t available/allowed so Windows runs don’t fail spuriously.
os.symlink(real, link)
tests/test_sleep_skill_resolver.py:196
- Using
chmod 0o000to simulate an unreadable directory is not reliable cross-platform (especially on Windows, where permissions behave differently). Mockingos.listdirfor the cache path makes this test deterministic while still exercising the error-handling path in_listdir().
os.chmod(cache, 0o000)
try:
cfg = load_config(claude_home=claude_home)
self.assertEqual(skill_search_roots(cfg), [skills])
finally:
os.chmod(cache, 0o700)
tests/test_sleep_skill_resolver.py:97
- These symlink-based assertions will fail on platforms/environments where
os.symlinkis unavailable or requires elevated privileges (notably Windows CI without Developer Mode). It’d be more robust to skip this test when symlink creation isn’t permitted, rather than hard-failing the suite.
This issue also appears in the following locations of the same file:
- line 107
- line 115
os.symlink(os.path.join(outside, "example-skill"),
os.path.join(root, "example-skill"))
|
Thanks for the quick revision. The empty One requested filesystem-contract item is still outstanding: Could you support the bounded versioned layout (while retaining the legacy layout if needed) and add a sanitized fixture or smoke test that mirrors an actual installed plugin tree? Once that is covered, we can re-review this small slice quickly. |
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
Five points from review on microsoft#185, plus one adjacent hardening: - skill_search_roots: guard the blank claude_home BEFORE abspath. os.path.abspath("") returns the CWD, so a config override of claude_home="" turned the working directory into a skill root. The existing `if not claude_home` check could never fire because it ran on the already-absolutised value. - _contained_skill_file: os.path.commonpath raises ValueError when the resolved skill file lands on a different drive (Windows, reachable via symlink). Treat it as "not contained" rather than letting it crash resolution. - SkillResolution.candidates is now Tuple[str, ...] with a () default, so the frozen dataclass is actually immutable; callers can no longer mutate the result in place. - Tests use context managers instead of open(...).read(), so no descriptor is left open (matters on Windows, where an open handle blocks delete). - Test assertions updated for the tuple. Adjacent: plugin-cache discovery now goes through a _listdir helper that swallows OSError, so an unreadable cache directory degrades to "no plugin roots" instead of raising. Previously os.path.isdir() passing did not guarantee os.listdir() would succeed. New tests cover each fix: blank/whitespace/None claude_home, an unreadable plugin cache, tuple immutability, and a commonpath that raises ValueError.
Cache discovery stopped at <cache>/<marketplace>/<plugin>/skills. Checked against a real Claude install: that layout does not occur there at all. Every installed plugin exposes skills at <cache>/<marketplace>/<plugin>/<version>/skills — 9 of 9 — so plugin-cache discovery returned nothing on a working machine, not merely "missed some". Supports both layouts, but deliberately contributes AT MOST ONE root per installed plugin: several versions of the same plugin can be present at once (the real install carries three of one plugin). Adding each version directory as a peer root would make an ordinary upgraded plugin resolve AMBIGUOUS, which would fire constantly for the most common case. The newest version wins, with the legacy unversioned directory as the fallback when no version directory carries skills. Version ordering is numeric per segment, so 1.10.0 outranks 1.9.0 rather than losing a string comparison; non-numeric segments (2.0.0-beta) sort below a bare number at the same position, and the directory name is the final tie-break so the order is total and deterministic. Tests add the sanitized real-install fixture that was requested: the versioned layout, a three-version plugin asserting newest-wins rather than AMBIGUOUS, numeric-vs-lexicographic ordering, two marketplaces each contributing a root, and the legacy layout still resolving. Verified against the actual cache: roots discovered went 1 -> 8, and chrome-devtools-mcp resolves to 1.6.0 only, never 1.1.1 or 1.5.0.
719c74c to
109bafb
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 (2)
skillopt_sleep/skill_resolver.py:90
_version_sort_key()currently sorts prerelease-like names (e.g.2.0.0-beta) after the corresponding release (2.0.0) because list comparison treats a shorter equal-prefix list as smaller. That contradicts the docstring (“keeping prereleases under releases”) and can cause_plugin_skills_root()to prefer a prerelease over a stable version when both are installed.
segments = []
for part in re.split(r"[._\-+]", name):
if part.isdigit():
segments.append((1, int(part), ""))
else:
tests/test_sleep_skill_resolver.py:259
- The version-selection tests cover numeric ordering (
1.9.0vs1.10.0) but don’t cover prerelease ordering (2.0.0-betashould not win over2.0.0). Adding a regression test here would prevent_plugin_skills_root()from selecting a prerelease when a stable release is present.
def test_version_ordering_is_numeric_not_lexicographic(self):
with tempfile.TemporaryDirectory() as tmp:
claude_home = os.path.join(tmp, ".claude")
plugin = os.path.join(claude_home, "plugins", "cache", "market", "plugin")
for version in ["1.9.0", "1.10.0"]:
_write_skill(os.path.join(plugin, version, "skills"), "example-skill")
cfg = load_config(claude_home=claude_home)
roots = [r for r in skill_search_roots(cfg) if r.startswith(plugin)]
# "1.10.0" < "1.9.0" as strings; it must still win as a version.
self.assertEqual(roots, [os.path.join(plugin, "1.10.0", "skills")])
|
Good catch, and after investigating it was a bigger issue than expected. I did review against a real Claude install and the unversioned layout does not occur there at all. Every installed plugin exposes its skills at One thing that fell out of looking at the real tree: several versions of the same plugin can be installed side by side — one plugin here has three (1.1.1, 1.5.0, 1.6.0). In my view, contributing each version directory as a peer root would make an ordinary upgraded plugin resolve So discovery now contributes at most one root per installed plugin: the newest version wins, with the legacy unversioned directory as the fallback when no version directory carries skills. Version ordering compares numerically per segment, so Correction to an earlier version of this comment: I originally wrote that prereleases sorted below releases. They did not. Copilot caught that the first key ranked Added the sanitized fixture you asked for, mirroring the observed install tree: the versioned layout, a three-version plugin asserting newest-wins rather than Copilot's remaining notes are handled as well: the prerelease ordering fix above, plus portability guards so the symlink and permission tests skip cleanly where the platform will not support them rather than failing for unrelated reasons. Ready for your re-read whenever suits. Thanks and appreciate the help here. |
_version_sort_key built one flat list of segments, so "2.0.0" and "2.0.0-beta" shared a numeric prefix and the shorter list sorted lower — Python ranks a list that prefixes another as smaller. Newest-wins therefore selected the prerelease, the opposite of what the docstring claimed, and _plugin_skills_root would have preferred 2.0.0-beta over 2.0.0 when both were installed. The key now separates the leading numeric segments from any prerelease suffix and ranks a bare release above a suffixed one at the same numeric prefix. Ordering verified across releases (1.10.0 > 1.9.0), prereleases (2.0.0 > 2.0.0-beta, 2.0.0-beta > 2.0.0-alpha), differing depths (1.0.0 > 1.0), embedded suffixes (1.0.0 > 1.0.0rc1) and non-numeric names (1.0.0 > main). Adds the requested regression: a plugin with 2.0.0 and 2.0.0-beta installed side by side must resolve to 2.0.0, plus direct key assertions for each ordering form.
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 (4)
tests/test_sleep_skill_resolver.py:107
- This test calls os.symlink() directly; on Windows (and some locked-down CI environments) symlink creation can fail without admin/Developer Mode. Skipping when symlinks are unavailable keeps the suite portable.
os.symlink(elsewhere, os.path.join(root, "example-skill", "SKILL.md"))
tests/test_sleep_skill_resolver.py:115
- This test calls os.symlink() directly; on Windows (and some CI environments) symlink creation can fail due to permissions. Consider skipping when symlinks are unavailable to avoid platform-specific failures.
os.symlink(real, link)
tests/test_sleep_skill_resolver.py:97
- This test calls os.symlink() directly; on Windows (and some locked-down CI environments) symlink creation can fail without admin/Developer Mode, causing unrelated test failures. Consider skipping the test when symlinks are unavailable by catching OSError and calling self.skipTest(...).
This issue also appears in the following locations of the same file:
- line 107
- line 115
os.symlink(os.path.join(outside, "example-skill"),
os.path.join(root, "example-skill"))
tests/test_sleep_skill_resolver.py:196
- This test relies on chmod(0o000) making the plugin cache unreadable so that os.listdir raises. On Windows (and some filesystems), chmod may be unsupported or may not remove read access, and it can also raise errors that would fail the test. Consider skipping when permissions cannot be modified, and make the restoration chmod best-effort.
os.chmod(cache, 0o000)
try:
cfg = load_config(claude_home=claude_home)
self.assertEqual(skill_search_roots(cfg), [skills])
finally:
Four tests asserted platform behaviour that does not exist everywhere: - Three symlink tests called os.symlink() directly. Windows needs admin or Developer Mode, and some CI sandboxes refuse symlinks outright, so the suite failed there for reasons unrelated to the code under test. They now go through a helper that skips when the platform will not create one. The security property being asserted only exists where symlinks do, so skipping is the honest outcome rather than a failure. - The unreadable-plugin-cache test relied on chmod(0o000) actually removing read access. Windows and several filesystems ignore mode bits, and root bypasses them, so the precondition silently did not hold and the assertion proved nothing. It now verifies the directory really became unreadable and skips when it did not, and the restoring chmod is best-effort so a failure there cannot mask the real result. No production code touched; 28 resolver tests still pass on POSIX.
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.Rebased onto current
main(e7014cd). This branch adds two new files andedits none, so it can be reviewed and merged on its own; #184 is the logical
predecessor but not a technical dependency.
Tests
Base is unmodified
mainate7014cd, same machine, same interpreter(CPython 3.12.13, macOS):
pytest tests/test_sleep_skill_resolver.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: normalization accept/reject table, single local skill,
missing vs ambiguous, directory without
SKILL.md, repeated root, traversal,symlinked skill dir and symlinked
SKILL.mdescaping the root, symlinked root,unchanged skill file after resolution, no roots, config root precedence, and
legacy
target_skill_path. All fixtures are hermetic temp dirs with syntheticnames.
Refs #120