From ca65fdbd53460a8ded560c1fa70ce4294aa96852 Mon Sep 17 00:00:00 2001 From: Philip Zeyliger Date: Sun, 8 Feb 2026 21:03:39 +0000 Subject: [PATCH] shelley: fix skills not loaded from non-home working directories Prompt: Recent boldsoftware/shelley gh issue talks about skills not being read depending on working dir. Fix that. DefaultDirs() had overly complex pre-checking logic that scanned candidate directories for skill subdirectories before returning them. This was unnecessary since Discover() already handles scanning, and the complexity could cause edge cases where valid skill directories were missed. Simplified DefaultDirs() to always return candidate directories that exist (~/.config/shelley, ~/.config/agents/skills, ~/.shelley), letting Discover() do the actual skill scanning. Also wired up ProjectSkillsDirs() in collectSkills() so that .skills directories in the project tree are now discovered (they were defined but never called). Fixes https://github.com/boldsoftware/shelley/issues/83 Co-authored-by: Shelley --- server/system_prompt.go | 8 ++- server/system_prompt_test.go | 34 +++++++++++ skills/skills.go | 42 ++----------- skills/skills_test.go | 115 +++++++++++++++++++++++++++++++++++ 4 files changed, 160 insertions(+), 39 deletions(-) diff --git a/server/system_prompt.go b/server/system_prompt.go index c9f3fffb7c54e983bb60fb3e10b72dcc066b2bdc..3113f91c65a5d07bac90039d616d527c448dafdf 100644 --- a/server/system_prompt.go +++ b/server/system_prompt.go @@ -298,12 +298,16 @@ func isExeDev() bool { return err == nil } -// collectSkills discovers skills from default directories and project tree. +// collectSkills discovers skills from default directories, project .skills dirs, +// and the project tree. func collectSkills(workingDir, gitRoot string) string { // Start with default directories (user-level skills) dirs := skills.DefaultDirs() - // Discover user-level skills from configured directories + // Add .skills directories found in the project tree + dirs = append(dirs, skills.ProjectSkillsDirs(workingDir, gitRoot)...) + + // Discover skills from all directories foundSkills := skills.Discover(dirs) // Also discover skills anywhere in the project tree diff --git a/server/system_prompt_test.go b/server/system_prompt_test.go index 3c60d3065f3b660cd931539c5470018a326e5c4e..62d8a568db795ef43f1e16104f962338fbdc9414 100644 --- a/server/system_prompt_test.go +++ b/server/system_prompt_test.go @@ -112,3 +112,37 @@ func min(a, b int) int { } return b } + +// TestSystemPromptIncludesSkillsFromAnyWorkingDir verifies that user-level +// skills (e.g. from ~/.config/agents/skills) appear in the system prompt +// regardless of the conversation's working directory. +// Regression test for https://github.com/boldsoftware/shelley/issues/83 +func TestSystemPromptIncludesSkillsFromAnyWorkingDir(t *testing.T) { + // Create a fake home with a skill + tmpHome := t.TempDir() + skillDir := filepath.Join(tmpHome, ".config", "agents", "skills", "test-skill") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("---\nname: test-skill\ndescription: A test skill for issue 83.\n---\nInstructions.\n"), 0o644); err != nil { + t.Fatal(err) + } + + oldHome := os.Getenv("HOME") + os.Setenv("HOME", tmpHome) + t.Cleanup(func() { os.Setenv("HOME", oldHome) }) + + // Generate system prompt from a directory completely unrelated to home + unrelatedDir := t.TempDir() + prompt, err := GenerateSystemPrompt(unrelatedDir) + if err != nil { + t.Fatalf("GenerateSystemPrompt failed: %v", err) + } + + if !strings.Contains(prompt, "test-skill") { + t.Error("system prompt should contain skill 'test-skill' even when working dir is unrelated to home") + } + if !strings.Contains(prompt, "A test skill for issue 83.") { + t.Error("system prompt should contain the skill description") + } +} diff --git a/skills/skills.go b/skills/skills.go index 8b4694c4798ed3b761bd35554a4c41cfc1175875..6fb922d31ee85bc857c3f15944aa406f549c88e3 100644 --- a/skills/skills.go +++ b/skills/skills.go @@ -303,8 +303,9 @@ func ToPromptXML(skills []Skill) string { } // DefaultDirs returns the default skill directories to search. +// These are always returned if they exist, regardless of the current working directory. func DefaultDirs() []string { - dirs := []string{} + var dirs []string home, err := os.UserHomeDir() if err != nil { @@ -321,48 +322,15 @@ func DefaultDirs() []string { filepath.Join(home, ".shelley"), } - for _, candidateDir := range candidateDirs { - entries, err := os.ReadDir(candidateDir) - if err != nil { - continue - } - for _, entry := range entries { - if entry.IsDir() { - subdir := filepath.Join(candidateDir, entry.Name()) - // Check if this directory contains skills (has subdirs with SKILL.md) - // or is itself a skill directory - if findSkillMD(subdir) != "" { - // This is a skill directory itself, add parent - dirs = append(dirs, candidateDir) - break - } - // Otherwise check if it's a container of skills - if hasSkillSubdirs(subdir) { - dirs = append(dirs, subdir) - } - } + for _, dir := range candidateDirs { + if info, err := os.Stat(dir); err == nil && info.IsDir() { + dirs = append(dirs, dir) } } return dirs } -// hasSkillSubdirs checks if a directory contains any skill subdirectories. -func hasSkillSubdirs(dir string) bool { - entries, err := os.ReadDir(dir) - if err != nil { - return false - } - for _, entry := range entries { - if entry.IsDir() { - if findSkillMD(filepath.Join(dir, entry.Name())) != "" { - return true - } - } - } - return false -} - // expandPath expands ~ to the user's home directory. func expandPath(path string) string { if strings.HasPrefix(path, "~/") { diff --git a/skills/skills_test.go b/skills/skills_test.go index 306d817b64263d2ff59295721a607a3eb07abdfc..bcc65e7986e21b936e6b0569fbd5b4b60b78f85f 100644 --- a/skills/skills_test.go +++ b/skills/skills_test.go @@ -445,3 +445,118 @@ func TestProjectSkillsDirs(t *testing.T) { t.Errorf("second dir = %q, want %q", dirs[1], expectedSecond) } } + +func TestDefaultDirsReturnsExistingCandidates(t *testing.T) { + // Create a fake home directory with skill directories + tmpHome := t.TempDir() + + // Create all three candidate directories + configShelley := filepath.Join(tmpHome, ".config", "shelley") + configAgents := filepath.Join(tmpHome, ".config", "agents", "skills") + dotShelley := filepath.Join(tmpHome, ".shelley") + + for _, dir := range []string{configShelley, configAgents, dotShelley} { + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + } + + // Override HOME + oldHome := os.Getenv("HOME") + os.Setenv("HOME", tmpHome) + t.Cleanup(func() { os.Setenv("HOME", oldHome) }) + + dirs := DefaultDirs() + + if len(dirs) != 3 { + t.Fatalf("expected 3 dirs, got %d: %v", len(dirs), dirs) + } + + // Verify all three candidates are returned + want := map[string]bool{ + configShelley: true, + configAgents: true, + dotShelley: true, + } + for _, d := range dirs { + if !want[d] { + t.Errorf("unexpected dir in result: %s", d) + } + } +} + +func TestDefaultDirsSkipsMissingDirs(t *testing.T) { + tmpHome := t.TempDir() + + // Only create one of the candidate directories + configAgents := filepath.Join(tmpHome, ".config", "agents", "skills") + if err := os.MkdirAll(configAgents, 0o755); err != nil { + t.Fatal(err) + } + + oldHome := os.Getenv("HOME") + os.Setenv("HOME", tmpHome) + t.Cleanup(func() { os.Setenv("HOME", oldHome) }) + + dirs := DefaultDirs() + + if len(dirs) != 1 { + t.Fatalf("expected 1 dir, got %d: %v", len(dirs), dirs) + } + if dirs[0] != configAgents { + t.Errorf("expected %s, got %s", configAgents, dirs[0]) + } +} + +func TestSkillsFoundRegardlessOfWorkingDir(t *testing.T) { + // This is a regression test for: + // https://github.com/boldsoftware/shelley/issues/83 + // + // Skills from ~/.config/agents/skills should be discovered + // regardless of the current working directory. + + tmpHome := t.TempDir() + + // Create a skill in ~/.config/agents/skills/ + skillDir := filepath.Join(tmpHome, ".config", "agents", "skills", "my-skill") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("---\nname: my-skill\ndescription: A test skill.\n---\nContent\n"), 0o644); err != nil { + t.Fatal(err) + } + + oldHome := os.Getenv("HOME") + os.Setenv("HOME", tmpHome) + t.Cleanup(func() { os.Setenv("HOME", oldHome) }) + + // Create a project directory far from home + projectDir := t.TempDir() + + // Simulate what collectSkills does: + // DefaultDirs + Discover should find the skill regardless of project dir + dirs := DefaultDirs() + found := Discover(dirs) + + if len(found) != 1 { + t.Fatalf("expected 1 skill, got %d (dirs=%v)", len(found), dirs) + } + if found[0].Name != "my-skill" { + t.Errorf("expected my-skill, got %s", found[0].Name) + } + + // DiscoverInTree from the project dir should NOT find user-level skills + // (they're in hidden directories which are skipped) + treeSkills := DiscoverInTree(projectDir, projectDir) + if len(treeSkills) != 0 { + t.Errorf("expected 0 tree skills from unrelated project, got %d", len(treeSkills)) + } + + // But the combined result should still have the skill + all := append(found, treeSkills...) + if len(all) != 1 { + t.Fatalf("expected 1 total skill, got %d", len(all)) + } + + _ = projectDir // used above +}