Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 43 additions & 12 deletions pkg/skills/skills.go
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ func loadSkillsFlat(dir string) []Skill {

var skills []Skill
for _, entry := range entries {
if !entry.IsDir() || isHiddenOrSymlink(entry) {
if !entry.IsDir() || (isHidden(entry) || isSymlink(entry)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not following symlinks? The real change with symlink are security issues to ensure that they don't allow to go outside of the project folder....

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to work on loadSkillsFlat in that PR. Happy to work on it in another PR

continue
}

Expand All @@ -210,14 +210,48 @@ func loadSkillsFlat(dir string) []Skill {
}

// loadSkillsRecursive loads skills from all subdirectories (Codex format).
// It tracks visited real directory paths to avoid infinite loops caused by
// symlinks that form cycles.
func loadSkillsRecursive(dir string) []Skill {
visited := make(map[string]bool)

// Resolve the root so cycles back to it are detected.
if realDir, err := filepath.EvalSymlinks(dir); err == nil {
visited[realDir] = true
}

return walkSkillsRecursive(dir, visited)
}

// walkSkillsRecursive walks dir for SKILL.md files, using visited to skip
// directories whose real path has already been traversed.
func walkSkillsRecursive(dir string, visited map[string]bool) []Skill {
var skills []Skill

_ = filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error {
if err != nil || d.IsDir() {
if err != nil {
return nil
}

if d.IsDir() {
if path != dir && isHidden(d) {
return fs.SkipDir
}

// Resolve and de-duplicate real directory paths to catch
// cycles introduced through symlinks higher up.
if path != dir {
if realPath, err := filepath.EvalSymlinks(path); err == nil {
if visited[realPath] {
return fs.SkipDir
}
visited[realPath] = true
}
}
return nil
}
if isHiddenOrSymlink(d) || d.Name() != skillFile {

if d.Name() != skillFile {
return nil
}

Expand Down Expand Up @@ -277,17 +311,14 @@ func parseFrontmatter(content string) (Skill, bool) {
return skill, true
}

// isValidSkill validates skill constraints.
func isValidSkill(skill Skill) bool {
// Description and name is required
if skill.Description == "" || skill.Name == "" {
return false
}
return skill.Description != "" && skill.Name != ""
}

return true
func isHidden(entry fs.DirEntry) bool {
return strings.HasPrefix(entry.Name(), ".")
}

// isHiddenOrSymlink returns true for hidden files/dirs or symlinks.
func isHiddenOrSymlink(entry fs.DirEntry) bool {
return strings.HasPrefix(entry.Name(), ".") || entry.Type()&os.ModeSymlink != 0
func isSymlink(entry fs.DirEntry) bool {
return entry.Type()&os.ModeSymlink != 0
}
38 changes: 38 additions & 0 deletions pkg/skills/skills_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,44 @@ description: A flat global agents skill
assert.True(t, foundFlat, "Expected to find flat-skill from ~/.agents/skills/flat-skill")
}

func TestLoadSkillsFromDir_RecursiveSymlinkCycle(t *testing.T) {
tmpDir := t.TempDir()

// Create a skill in a subdirectory.
skillDir := filepath.Join(tmpDir, "real-skill")
require.NoError(t, os.MkdirAll(skillDir, 0o755))

skillContent := `---
name: real-skill
description: A real skill
---

# Real Skill
`
require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(skillContent), 0o644))

// Create a symlink cycle: tmpDir/real-skill/link -> tmpDir
require.NoError(t, os.Symlink(tmpDir, filepath.Join(skillDir, "link")))

// loadSkillsRecursive must return without looping forever.
skills := loadSkillsFromDir(tmpDir, true)

require.Len(t, skills, 1)
assert.Equal(t, "real-skill", skills[0].Name)
assert.Equal(t, "A real skill", skills[0].Description)
}

func TestLoadSkillsFromDir_RecursiveSymlinkSelfReference(t *testing.T) {
tmpDir := t.TempDir()

// Create a directory that symlinks to itself.
require.NoError(t, os.Symlink(tmpDir, filepath.Join(tmpDir, "self")))

// Must not loop forever.
skills := loadSkillsFromDir(tmpDir, true)
assert.Empty(t, skills)
}

func TestLoad_AgentsSkillsProjectFromNestedDir(t *testing.T) {
// Create a fake git repo with .agents/skills at the root
tmpRepo := t.TempDir()
Expand Down