diff --git a/changes/unreleased/70-skill-symlink-for-goose-discovery.yaml b/changes/unreleased/70-skill-symlink-for-goose-discovery.yaml new file mode 100644 index 00000000..b16f90e1 --- /dev/null +++ b/changes/unreleased/70-skill-symlink-for-goose-discovery.yaml @@ -0,0 +1,8 @@ +kind: feature +description: > + Symlink $HOME/.agents/skills to the skills directory (default /opt/skills, + configurable via HARNESS_SKILLS_DIR) so goose discovers mounted SkillCard + content via its native skill platform. The link is placed in the home + directory rather than the clone to avoid touching the repo tree. Skills + are no longer injected into the prompt; the agent runtime handles + progressive disclosure. diff --git a/harness/cmd/migration-harness/main.go b/harness/cmd/migration-harness/main.go index ef3d89ed..d8c0196c 100644 --- a/harness/cmd/migration-harness/main.go +++ b/harness/cmd/migration-harness/main.go @@ -8,7 +8,6 @@ import ( "os/signal" "path/filepath" "strconv" - "strings" "sync/atomic" "syscall" "time" @@ -126,7 +125,7 @@ func runStage(cmd *cobra.Command, args []string) error { logging.Ok("cloned to %s, branch %s", cloneDir, creds.Branch) // 4. Discover skills early — controls which setup steps run - skillContent, skillPaths, err := discoverSkills() + skillPaths, err := discoverSkills() if err != nil { return fmt.Errorf("discover skills: %w", err) } @@ -145,6 +144,15 @@ func runStage(cmd *cobra.Command, args []string) error { }); err != nil { logging.Warn("gitignore: %v", err) } + + home, err := os.UserHomeDir() + if err != nil { + return fmt.Errorf("resolve home dir: %w", err) + } + if err := symlinkSkillsDir(home, skillsDir()); err != nil { + return fmt.Errorf("skill symlink: %w", err) + } + logging.Ok("symlinked %s/.agents/skills → %s", home, skillsDir()) } if hasSkills { @@ -281,7 +289,6 @@ func runStage(cmd *cobra.Command, args []string) error { stagePrompt := prompt.Build(prompt.Layers{ AgentPrompt: cfg.AgentPrompt, WorkflowGuide: cfg.WorkflowGuide, - Skill: skillContent, StageTask: cfg.StageInstructions, }) @@ -373,6 +380,34 @@ func runStage(cmd *cobra.Command, args []string) error { return nil } +func symlinkSkillsDir(homeDir, skillsSrc string) error { + skillsSrc, err := filepath.Abs(skillsSrc) + if err != nil { + return fmt.Errorf("resolve skills source: %w", err) + } + + agentsDir := filepath.Join(homeDir, ".agents") + if err := os.MkdirAll(agentsDir, 0o755); err != nil { + return err + } + + link := filepath.Join(agentsDir, "skills") + if info, err := os.Lstat(link); err == nil { + if info.Mode()&os.ModeSymlink != 0 { + if target, err := os.Readlink(link); err == nil && target == skillsSrc { + return nil + } + if err := os.Remove(link); err != nil { + return fmt.Errorf("remove stale symlink %s: %w", link, err) + } + } else { + return fmt.Errorf("%s already exists and is not a symlink", link) + } + } + + return os.Symlink(skillsSrc, link) +} + const defaultSkillsDir = "/opt/skills" func skillsDir() string { @@ -382,30 +417,21 @@ func skillsDir() string { return defaultSkillsDir } -func discoverSkills() (string, []string, error) { +func discoverSkills() ([]string, error) { pattern := filepath.Join(skillsDir(), "*/SKILL.md") matches, err := filepath.Glob(pattern) if err != nil { - return "", nil, err + return nil, err } if len(matches) == 0 { logging.Info("no skills found at %s — proceeding without skills", pattern) - return "", nil, nil + return nil, nil } - var combined strings.Builder - for i, m := range matches { - content, err := os.ReadFile(m) - if err != nil { - return "", nil, fmt.Errorf("read skill %s: %w", m, err) - } + for _, m := range matches { logging.Info("discovered skill: %s", m) - if i > 0 { - combined.WriteString("\n\n---\n\n") - } - combined.Write(content) } - return combined.String(), matches, nil + return matches, nil } func resolveFromHub(cfg *config.Config, hubClient *hub.Client) (*git.Credentials, error) { diff --git a/harness/cmd/migration-harness/main_test.go b/harness/cmd/migration-harness/main_test.go index 6380ade8..0a5e3c0f 100644 --- a/harness/cmd/migration-harness/main_test.go +++ b/harness/cmd/migration-harness/main_test.go @@ -3,6 +3,7 @@ package main import ( "os" "path/filepath" + "strings" "testing" "github.com/konveyor/migration-harness/internal/config" @@ -12,13 +13,10 @@ func TestDiscoverSkills_NoSkills(t *testing.T) { dir := t.TempDir() t.Setenv("HARNESS_SKILLS_DIR", dir) - content, paths, err := discoverSkills() + paths, err := discoverSkills() if err != nil { t.Fatalf("expected no error, got: %v", err) } - if content != "" { - t.Errorf("expected empty content, got: %q", content) - } if len(paths) != 0 { t.Errorf("expected no paths, got: %v", paths) } @@ -36,13 +34,10 @@ func TestDiscoverSkills_WithSkills(t *testing.T) { t.Fatal(err) } - content, paths, err := discoverSkills() + paths, err := discoverSkills() if err != nil { t.Fatalf("unexpected error: %v", err) } - if content != "do the thing" { - t.Errorf("expected skill content, got: %q", content) - } if len(paths) != 1 { t.Errorf("expected 1 path, got: %v", paths) } @@ -60,18 +55,129 @@ func TestDiscoverSkills_EmptySkillFile(t *testing.T) { t.Fatal(err) } - content, paths, err := discoverSkills() + paths, err := discoverSkills() if err != nil { t.Fatalf("unexpected error: %v", err) } - if content != "" { - t.Errorf("expected empty content, got: %q", content) - } if len(paths) != 1 { t.Errorf("expected 1 path (skill is mounted), got: %v", paths) } } +func TestSymlinkSkillsDir(t *testing.T) { + homeDir := t.TempDir() + skillsSrc := t.TempDir() + + if err := symlinkSkillsDir(homeDir, skillsSrc); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + link := filepath.Join(homeDir, ".agents", "skills") + target, err := os.Readlink(link) + if err != nil { + t.Fatalf("expected symlink at %s: %v", link, err) + } + if target != skillsSrc { + t.Errorf("symlink target = %q, want %q", target, skillsSrc) + } +} + +func TestSymlinkSkillsDir_AlreadyExistsDir(t *testing.T) { + homeDir := t.TempDir() + skillsSrc := t.TempDir() + + if err := os.MkdirAll(filepath.Join(homeDir, ".agents", "skills"), 0o755); err != nil { + t.Fatal(err) + } + + err := symlinkSkillsDir(homeDir, skillsSrc) + if err == nil { + t.Fatal("expected error when .agents/skills already exists as a directory") + } + if !strings.Contains(err.Error(), "not a symlink") { + t.Errorf("error should mention 'not a symlink', got: %v", err) + } +} + +func TestSymlinkSkillsDir_Idempotent(t *testing.T) { + homeDir := t.TempDir() + skillsSrc := t.TempDir() + + if err := symlinkSkillsDir(homeDir, skillsSrc); err != nil { + t.Fatalf("first call: %v", err) + } + if err := symlinkSkillsDir(homeDir, skillsSrc); err != nil { + t.Fatalf("second call (same target) should be idempotent: %v", err) + } + + link := filepath.Join(homeDir, ".agents", "skills") + target, err := os.Readlink(link) + if err != nil { + t.Fatalf("expected symlink at %s: %v", link, err) + } + if target != skillsSrc { + t.Errorf("symlink target = %q, want %q", target, skillsSrc) + } +} + +func TestSymlinkSkillsDir_RelinksOnDifferentTarget(t *testing.T) { + homeDir := t.TempDir() + oldSrc := t.TempDir() + newSrc := t.TempDir() + + if err := symlinkSkillsDir(homeDir, oldSrc); err != nil { + t.Fatalf("first call: %v", err) + } + if err := symlinkSkillsDir(homeDir, newSrc); err != nil { + t.Fatalf("second call (different target): %v", err) + } + + link := filepath.Join(homeDir, ".agents", "skills") + target, err := os.Readlink(link) + if err != nil { + t.Fatalf("expected symlink at %s: %v", link, err) + } + if target != newSrc { + t.Errorf("symlink target = %q, want %q", target, newSrc) + } +} + +func TestSymlinkSkillsDir_ResolvesRelativePath(t *testing.T) { + parent := t.TempDir() + homeDir := filepath.Join(parent, "home") + skillsSrc := filepath.Join(parent, "skills") + if err := os.MkdirAll(homeDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(skillsSrc, 0o755); err != nil { + t.Fatal(err) + } + + relPath, err := filepath.Rel(parent, skillsSrc) + if err != nil { + t.Fatal(err) + } + + oldWd, _ := os.Getwd() + if err := os.Chdir(parent); err != nil { + t.Fatal(err) + } + defer os.Chdir(oldWd) + + if err := symlinkSkillsDir(homeDir, relPath); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + link := filepath.Join(homeDir, ".agents", "skills") + target, err := os.Readlink(link) + if err != nil { + t.Fatalf("expected symlink at %s: %v", link, err) + } + if !filepath.IsAbs(target) { + t.Errorf("symlink target should be absolute, got %q", target) + } +} + func TestParseHubTokenID(t *testing.T) { tests := []struct { name string diff --git a/harness/internal/prompt/prompt.go b/harness/internal/prompt/prompt.go index 3ffd34e9..70c4dce5 100644 --- a/harness/internal/prompt/prompt.go +++ b/harness/internal/prompt/prompt.go @@ -28,14 +28,12 @@ not say where to put something. ` // Layers are the context layers composed into a stage prompt, ordered from -// least to most specific. Any of them may be empty except Skill. +// least to most specific. type Layers struct { // AgentPrompt is the Agent's standing prompt. AgentPrompt string // WorkflowGuide is the workflow's ambient guide. WorkflowGuide string - // Skill is the content discovered from the mounted SkillCards. - Skill string // StageTask is the task for this stage. StageTask string } @@ -59,16 +57,8 @@ func Build(l Layers) string { b.WriteString("\n\n") } - // Skills are optional (#82): with none mounted the agent still needs to be - // told to commit, since nothing else in the prompt says so. - if l.Skill != "" { - b.WriteString("## Skill Instructions\n\n") - b.WriteString(l.Skill) - b.WriteString("\n\n") - } else { - b.WriteString("## Working Guidelines\n\n") - b.WriteString("Commit your changes to git with a descriptive message when your work is complete.\n\n") - } + b.WriteString("## Working Guidelines\n\n") + b.WriteString("Commit your changes to git with a descriptive message when your work is complete.\n\n") if l.StageTask != "" { b.WriteString("## Stage Task\n\n") diff --git a/harness/internal/prompt/prompt_test.go b/harness/internal/prompt/prompt_test.go index beb0a78d..82678466 100644 --- a/harness/internal/prompt/prompt_test.go +++ b/harness/internal/prompt/prompt_test.go @@ -9,7 +9,6 @@ func fullLayers() Layers { return Layers{ AgentPrompt: "AGENT PROMPT", WorkflowGuide: "WORKFLOW GUIDE", - Skill: "SKILL BODY", StageTask: "STAGE TASK", } } @@ -21,10 +20,8 @@ func TestBuildPutsStagingRulesFirst(t *testing.T) { t.Fatalf("staging rules missing from prompt:\n%s", got) } - // Order matters as much as presence: the rules have to precede the skill, - // so a skill that says where to write cannot read as overriding them. rules := strings.Index(got, "Working Environment") - for _, later := range []string{"AGENT PROMPT", "WORKFLOW GUIDE", "SKILL BODY", "STAGE TASK"} { + for _, later := range []string{"AGENT PROMPT", "WORKFLOW GUIDE", "STAGE TASK"} { if strings.Index(got, later) < rules { t.Errorf("%q appears before the staging rules; rules must come first", later) } @@ -34,7 +31,7 @@ func TestBuildPutsStagingRulesFirst(t *testing.T) { func TestBuildOrdersLayersLeastToMostSpecific(t *testing.T) { got := Build(fullLayers()) - order := []string{"AGENT PROMPT", "WORKFLOW GUIDE", "SKILL BODY", "STAGE TASK"} + order := []string{"AGENT PROMPT", "WORKFLOW GUIDE", "STAGE TASK"} for i := 1; i < len(order); i++ { if strings.Index(got, order[i]) < strings.Index(got, order[i-1]) { t.Errorf("%q should come after %q", order[i], order[i-1]) @@ -43,40 +40,33 @@ func TestBuildOrdersLayersLeastToMostSpecific(t *testing.T) { } func TestBuildOmitsEmptyLayers(t *testing.T) { - got := Build(Layers{Skill: "SKILL BODY"}) + got := Build(Layers{}) for _, header := range []string{"## Workflow Guide", "## Stage Task"} { if strings.Contains(got, header) { t.Errorf("empty layer produced %q header", header) } } - if !strings.Contains(got, "## Skill Instructions") { - t.Error("skill content should always be included") + if !strings.Contains(got, "## Working Guidelines") { + t.Error("working guidelines should always be included") } } -// Skills are optional (#82). With none mounted the agent still needs to be told -// to commit, since nothing else in the prompt says so. -func TestBuildFallsBackWhenNoSkills(t *testing.T) { +func TestBuildIncludesCommitGuideline(t *testing.T) { got := Build(Layers{AgentPrompt: "AGENT PROMPT"}) - if strings.Contains(got, "## Skill Instructions") { - t.Error("empty skill should not produce a Skill Instructions header") - } if !strings.Contains(got, "## Working Guidelines") { - t.Fatalf("no skills should fall back to Working Guidelines:\n%s", got) + t.Fatalf("working guidelines missing:\n%s", got) } if !strings.Contains(got, "Commit your changes to git") { - t.Error("fallback should tell the agent to commit") + t.Error("working guidelines should tell the agent to commit") } } -// The ending differed depending on whether StageTask was set: sections append -// "\n\n" but StageTask appended nothing. func TestBuildEndsWithExactlyOneNewline(t *testing.T) { cases := map[string]Layers{ "with stage task": fullLayers(), - "without stage task": {Skill: "SKILL BODY"}, + "without stage task": {}, } for name, layers := range cases { t.Run(name, func(t *testing.T) {