Repository navigation
Follow Windows directory junctions during discovery and file reads - #104
Merged
Merged
Conversation
Since Go 1.23, a Windows directory junction is reported as an irregular file: neither a directory nor a symlink. Discovery matched only those two kinds, so a skills directory made of junctions, a common Windows layout, was silently reduced to the few entries that were real directories. Fixes #101. Matching the irregular kind is not enough on its own. EvalSymlinks no longer crosses a junction either, and fails with "not a directory" when one sits above the last element of a path, so a junctioned skill would be found and then every file in it refused by the containment check. util.RealPath expands junctions to their targets, outermost first, before evaluating symlinks. Discovery and ResolvesWithin use it, so a junctioned skill reads normally, a junctioned alias or cycle is visited once, and a junction inside a skill that leads outside it is refused, as a symlinked directory already was. Symlink tests used to skip on Windows unconditionally, which is why none of this reached CI. A shared testutil.Symlink helper skips only when the host denies the privilege; the hosted Windows runners are elevated, so the suite now runs there. Junction tests, which need no privilege, cover discovery, containment, internal links, and a check run against a junctioned catalog.
…test The end-to-end junction test copied the valid-skill fixture under new directory names, so every copy failed the check that a skill's name matches its directory. The junctioned skills were all discovered and read, which is what the test is for; only the fixture needed renaming.
There was a problem hiding this comment.
🟡 Changes recommended
Junctions inside symlink targets remain unresolved, causing valid skills to be skipped or rejected.
1 open issue
What changed in this PR
Adds Windows junction support to skill discovery and path containment checks, addressing skipped junctioned skills.
Changes:
- Introduces junction-aware path resolution.
- Adds Windows regression and integration tests.
- Enables symlink tests when privileges permit and documents junction support.
| File | Description |
|---|---|
| util/util.go | Uses junction-aware containment checks. |
| util/util_windows_test.go | Tests junction resolution and safe reads. |
| util/util_test.go | Enables privilege-aware symlink tests. |
| util/realpath.go | Adds shared path resolution. |
| util/junction_windows.go | Expands Windows junction targets. |
| util/junction_other.go | Preserves non-Windows behavior. |
| structure/links_windows_test.go | Tests junction link boundaries. |
| structure/links_test.go | Enables Windows symlink coverage. |
| skillcheck/validator.go | Follows junctions during discovery. |
| skillcheck/validator_test.go | Adopts shared symlink helper. |
| skillcheck/discovery_windows_test.go | Tests junction discovery scenarios. |
| skill/skill_test.go | Enables Windows symlink rejection test. |
| README.md | Documents junction discovery support. |
| internal/testutil/symlink.go | Adds privilege-aware symlink helper. |
| internal/testutil/privilege_windows.go | Detects missing symlink privileges. |
| internal/testutil/privilege_other.go | Keeps other creation failures fatal. |
| internal/testutil/junction_windows.go | Adds junction creation helper. |
| cmd/junction_windows_integration_test.go | Tests junctioned catalogs end to end. |
Review effort: Balanced
RealPath expanded junctions first and then called EvalSymlinks, so a junction met inside a symlink's target was still left to EvalSymlinks, which fails on it with "not a directory". A catalog entry that is a symlink to a skill behind a junction was skipped by discovery, and reads and link checks through such an alias failed. Flagged in review. Replace the two-phase expansion with one walker on Windows that follows each symlink or junction as it is met, resolving relative symlink targets against the link's directory or volume, restarting from the volume after each substitution, and giving up after 255 links as EvalSymlinks does. EvalSymlinks then only normalizes the link-free result. Other platforms keep calling EvalSymlinks directly. Cover a symlink into a junction, a relative one, a junction to a symlink, a read through a symlink into a junction, and discovery of a symlinked skill behind a junction.
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.
Fixes #101.
What was wrong
Since Go 1.23, a Windows directory junction is reported as an irregular file:
IsDir()is false and the symlink bit is not set. The discovery switch indiscoverSkillsmatched only directories and symlinks, so a skills directory made of junctions (the reported layout: a real source tree plus a junctioned view of it) was silently reduced to the few entries that were real directories. The reporter saw 2 of 104 skills.The one-line fix from the issue is not enough on its own. Go's
filepath.EvalSymlinksno longer crosses a junction either, and its walker fails with "not a directory" when a junction sits above the last element of a path. A junctioned skill would therefore be found and then have every file in it refused byutil.ResolvesWithin.What changed
util.RealPathisEvalSymlinksextended to junctions. On Windows it walks the path and follows each symlink or junction as it is met, in whatever order they chain, restarting after each substitution and bounded at 255 links likeEvalSymlinks; the link-free result is then normalized byEvalSymlinks. Elsewhere it isEvalSymlinks.RealPath, so a junctioned skill is found, a junctioned alias is reported once, and a junction cycle neither loops nor spends the directory budget.ResolvesWithinusesRealPathfor both root and path, so a skill reached through a junction reads normally, and a junction inside a skill that leads outside it is refused, as a symlinked directory already was (SafeReadFile does not guard against symlinked directories #88). Internal links get the same treatment (Should internal-link validation reject symlinks that resolve outside the skill directory? #86).Windows CI
The symlink tests skipped on Windows unconditionally ("symlinks require admin"), which is why this never reached CI. A shared
internal/testutil.Symlinkhelper now skips only when the host actually denies the privilege. The hosted Windows runners are elevated, so the whole symlink suite runs there.testutil.Junctioncreates junctions, which need no privilege.New Windows-only tests: junctioned catalog, junction into a grouping directory, junction alias, junction cycle, dangling junction,
RealPaththrough a junction and through mixed symlink/junction chains in both orders, a symlinked skill behind a junction,SafeReadFileon a junctioned root and through an escaping junction, internal links in and out of junctions, and an end-to-endcheck -o jsonon a junctioned catalog that asserts every skill is validated with no errors.Verification
go test -race ./...,go vetfor darwin and windows, andgolangci-lint runare clean locally. The junction tests only run on Windows, so the Windows CI leg is the verification for them.No CHANGELOG entry, following the release-commit pattern.