diff --git a/internal/skills/coverage_test.go b/internal/skills/coverage_test.go new file mode 100644 index 00000000..0e3cff30 --- /dev/null +++ b/internal/skills/coverage_test.go @@ -0,0 +1,995 @@ +package skills + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +func TestSourceKindValidateCases(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + kind SourceKind + wantErr bool + }{ + {name: "local", kind: SourceKindLocal}, + {name: "builtin", kind: SourceKindBuiltin}, + {name: "invalid", kind: SourceKind("x"), wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := tt.kind.Validate() + if tt.wantErr && err == nil { + t.Fatalf("expected error for kind=%q", tt.kind) + } + if !tt.wantErr && err != nil { + t.Fatalf("unexpected error for kind=%q: %v", tt.kind, err) + } + }) + } +} + +func TestActivationScopeValidateCases(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + scope ActivationScope + wantErr bool + }{ + {name: "global", scope: ScopeGlobal}, + {name: "workspace", scope: ScopeWorkspace}, + {name: "session", scope: ScopeSession}, + {name: "explicit", scope: ScopeExplicit}, + {name: "invalid", scope: ActivationScope("x"), wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := tt.scope.Validate() + if tt.wantErr && err == nil { + t.Fatalf("expected error for scope=%q", tt.scope) + } + if !tt.wantErr && err != nil { + t.Fatalf("unexpected error for scope=%q: %v", tt.scope, err) + } + }) + } +} + +func TestDescriptorValidateCases(t *testing.T) { + t.Parallel() + + base := Descriptor{ + ID: "go-review", + Name: "Go Review", + Source: Source{ + Kind: SourceKindLocal, + }, + Scope: ScopeExplicit, + } + + tests := []struct { + name string + mutate func(d Descriptor) Descriptor + wantError string + }{ + { + name: "valid", + mutate: func(d Descriptor) Descriptor { + return d + }, + }, + { + name: "empty id", + mutate: func(d Descriptor) Descriptor { + d.ID = " " + return d + }, + wantError: "id is empty", + }, + { + name: "empty name", + mutate: func(d Descriptor) Descriptor { + d.Name = "" + return d + }, + wantError: "name is empty", + }, + { + name: "invalid source kind", + mutate: func(d Descriptor) Descriptor { + d.Source.Kind = SourceKind("unknown") + return d + }, + wantError: "invalid source kind", + }, + { + name: "invalid scope", + mutate: func(d Descriptor) Descriptor { + d.Scope = ActivationScope("unknown") + return d + }, + wantError: "invalid activation scope", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + d := tt.mutate(base) + err := d.Validate() + if tt.wantError == "" && err != nil { + t.Fatalf("unexpected error: %v", err) + } + if tt.wantError != "" { + if err == nil || !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(tt.wantError)) { + t.Fatalf("expected error containing %q, got %v", tt.wantError, err) + } + } + }) + } +} + +func TestContentValidateCases(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + content Content + wantErr bool + }{ + { + name: "ok", + content: Content{ + Instruction: "review code", + }, + }, + { + name: "empty instruction", + content: Content{ + Instruction: " ", + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := tt.content.Validate() + if tt.wantErr && !errors.Is(err, ErrEmptyContent) { + t.Fatalf("expected ErrEmptyContent, got %v", err) + } + if !tt.wantErr && err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + } +} + +func TestLoadIssueErrorCases(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + issue LoadIssue + want string + }{ + { + name: "message and path", + issue: LoadIssue{ + Code: IssueInvalidMetadata, + Path: "/tmp/skill/SKILL.md", + Message: "bad metadata", + }, + want: "skills: bad metadata (/tmp/skill/SKILL.md)", + }, + { + name: "fallback to err", + issue: LoadIssue{ + Code: IssueReadFailed, + Err: errors.New("permission denied"), + }, + want: "skills: permission denied", + }, + { + name: "fallback to code", + issue: LoadIssue{ + Code: IssueDuplicateID, + }, + want: "skills: duplicate_id", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := tt.issue.Error(); got != tt.want { + t.Fatalf("Error() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestNormalizeWorkspaceCases(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + workspace string + want string + }{ + {name: "empty", workspace: " ", want: ""}, + { + name: "cleaned", + workspace: filepath.Join("a", "b", "..", "c"), + want: filepath.Clean(filepath.Join("a", "b", "..", "c")), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := normalizeWorkspace(tt.workspace); got != tt.want { + t.Fatalf("normalizeWorkspace(%q) = %q, want %q", tt.workspace, got, tt.want) + } + }) + } +} + +func TestFilterCases(t *testing.T) { + t.Parallel() + + descriptor := Descriptor{ + ID: "go-review", + Name: "Go Review", + Source: Source{ + Kind: SourceKindLocal, + }, + Scope: ScopeWorkspace, + } + + tests := []struct { + name string + input ListInput + wantSource bool + wantScope bool + }{ + { + name: "default all source but workspace requires workspace path", + input: ListInput{}, + wantSource: true, + wantScope: false, + }, + { + name: "source miss and scope hit", + input: ListInput{ + SourceKinds: []SourceKind{SourceKindBuiltin}, + Workspace: "repo", + }, + wantSource: false, + wantScope: true, + }, + { + name: "source and scope explicit filters", + input: ListInput{ + SourceKinds: []SourceKind{SourceKindLocal}, + Scopes: []ActivationScope{ScopeWorkspace}, + }, + wantSource: true, + wantScope: true, + }, + { + name: "scope explicit mismatch", + input: ListInput{ + Scopes: []ActivationScope{ScopeGlobal}, + }, + wantSource: true, + wantScope: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := allowBySource(tt.input, descriptor); got != tt.wantSource { + t.Fatalf("allowBySource() = %v, want %v", got, tt.wantSource) + } + if got := allowByScope(tt.input, descriptor); got != tt.wantScope { + t.Fatalf("allowByScope() = %v, want %v", got, tt.wantScope) + } + }) + } +} + +func TestLocalLoaderAdditionalBranches(t *testing.T) { + t.Parallel() + + t.Run("context canceled before load", func(t *testing.T) { + t.Parallel() + root := t.TempDir() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + _, err := NewLocalLoader(root).Load(ctx) + if !errors.Is(err, context.Canceled) { + t.Fatalf("expected context canceled, got %v", err) + } + }) + + t.Run("root skill loaded and unreadable skill file reported", func(t *testing.T) { + t.Parallel() + + root := t.TempDir() + // root-level skill should be discoverable. + if err := os.WriteFile(filepath.Join(root, skillFileName), []byte("## Instruction\nroot skill"), 0o644); err != nil { + t.Fatalf("write root skill: %v", err) + } + + // Make one candidate unreadable by turning SKILL.md into a directory. + badDir := filepath.Join(root, "bad") + if err := os.MkdirAll(filepath.Join(badDir, skillFileName), 0o755); err != nil { + t.Fatalf("mkdir bad skill dir: %v", err) + } + + snapshot, err := NewLocalLoader(root).Load(context.Background()) + if err != nil { + t.Fatalf("Load() error = %v", err) + } + if len(snapshot.Skills) != 1 { + t.Fatalf("expected 1 loaded skill, got %d", len(snapshot.Skills)) + } + foundReadFail := false + for _, issue := range snapshot.Issues { + if issue.Code == IssueReadFailed { + foundReadFail = true + break + } + } + if !foundReadFail { + t.Fatalf("expected IssueReadFailed in %+v", snapshot.Issues) + } + }) + + t.Run("load reports abs error via hook", func(t *testing.T) { + t.Parallel() + loader := NewLocalLoader("x") + loader.absPath = func(string) (string, error) { + return "", errors.New("abs failed") + } + _, err := loader.Load(context.Background()) + if err == nil || !strings.Contains(strings.ToLower(err.Error()), "resolve root") { + t.Fatalf("expected resolve root error, got %v", err) + } + }) + + t.Run("load reports stat error via hook", func(t *testing.T) { + t.Parallel() + loader := NewLocalLoader("x") + loader.absPath = func(string) (string, error) { + return "x", nil + } + loader.statPath = func(string) (os.FileInfo, error) { + return nil, errors.New("access denied") + } + _, err := loader.Load(context.Background()) + if err == nil || !strings.Contains(strings.ToLower(err.Error()), "stat root") { + t.Fatalf("expected stat root error, got %v", err) + } + }) + + t.Run("load reports read root error via hook", func(t *testing.T) { + t.Parallel() + loader := NewLocalLoader("x") + loader.absPath = func(string) (string, error) { + return "x", nil + } + loader.statPath = func(string) (os.FileInfo, error) { + return fakeDirInfo{name: "x"}, nil + } + loader.readDir = func(string) ([]os.DirEntry, error) { + return nil, errors.New("readdir denied") + } + _, err := loader.Load(context.Background()) + if err == nil || !strings.Contains(strings.ToLower(err.Error()), "read root") { + t.Fatalf("expected read root error, got %v", err) + } + }) + + t.Run("load exits when context canceled inside candidate loop", func(t *testing.T) { + t.Parallel() + + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "a"), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + loader := NewLocalLoader(root) + ctx := &errAfterNCallsContext{cancelAt: 2} + _, err := loader.Load(ctx) + if !errors.Is(err, context.Canceled) { + t.Fatalf("expected context canceled, got %v", err) + } + }) + + t.Run("load handles hidden skill directory and nil hooks fallback", func(t *testing.T) { + t.Parallel() + + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, ".hidden"), 0o755); err != nil { + t.Fatalf("mkdir hidden: %v", err) + } + if err := os.MkdirAll(filepath.Join(root, "visible"), 0o755); err != nil { + t.Fatalf("mkdir visible: %v", err) + } + if err := os.WriteFile(filepath.Join(root, "visible", skillFileName), []byte("## Instruction\nvisible"), 0o644); err != nil { + t.Fatalf("write visible skill: %v", err) + } + + loader := &LocalLoader{root: root} + snapshot, err := loader.Load(context.Background()) + if err != nil { + t.Fatalf("Load() error = %v", err) + } + if len(snapshot.Skills) != 1 { + t.Fatalf("expected only visible skill loaded, got %d", len(snapshot.Skills)) + } + }) +} + +func TestParseLocalSkillCases(t *testing.T) { + t.Parallel() + + root := t.TempDir() + skillDir := filepath.Join(root, "Go_Review") + skillPath := filepath.Join(skillDir, skillFileName) + + tests := []struct { + name string + raw string + wantErr string + wantIssueCode LoadIssueCode + assertSkill func(t *testing.T, skill Skill) + }{ + { + name: "invalid frontmatter", + raw: `--- +id: a +`, + wantErr: "frontmatter end marker", + wantIssueCode: IssueInvalidMetadata, + }, + { + name: "invalid yaml frontmatter", + raw: `--- +id: [a +--- +## Instruction +do it`, + wantErr: "did not find expected", + wantIssueCode: IssueInvalidMetadata, + }, + { + name: "invalid scope", + raw: `--- +id: a +scope: bad +--- +## Instruction +do it`, + wantErr: "invalid activation scope", + wantIssueCode: IssueInvalidMetadata, + }, + { + name: "invalid source kind", + raw: `--- +id: a +source: cloud +--- +## Instruction +do it`, + wantErr: "invalid source kind", + wantIssueCode: IssueInvalidMetadata, + }, + { + name: "unsupported source", + raw: `--- +id: a +source: builtin +--- +## Instruction +do it`, + wantErr: "only supports source=local", + wantIssueCode: IssueUnsupportedSource, + }, + { + name: "defaults and helpers", + raw: `This is plain body instruction. + +## references +- [Spec](https://go.dev/ref/spec) +- ./README.md + +## examples +- first +- second + +## tool_hints +- filesystem_read_file +- bash +`, + assertSkill: func(t *testing.T, skill Skill) { + t.Helper() + if skill.Descriptor.ID != "go-review" { + t.Fatalf("unexpected id: %q", skill.Descriptor.ID) + } + if skill.Descriptor.Name != "go-review" { + t.Fatalf("unexpected name from first heading: %q", skill.Descriptor.Name) + } + if skill.Descriptor.Version != "v1" { + t.Fatalf("unexpected default version: %q", skill.Descriptor.Version) + } + if skill.Descriptor.Scope != ScopeExplicit { + t.Fatalf("unexpected default scope: %q", skill.Descriptor.Scope) + } + if skill.Descriptor.Source.Kind != SourceKindLocal { + t.Fatalf("unexpected source kind: %q", skill.Descriptor.Source.Kind) + } + if len(skill.Content.References) != 2 { + t.Fatalf("unexpected references: %+v", skill.Content.References) + } + if len(skill.Content.Examples) != 2 { + t.Fatalf("unexpected examples: %+v", skill.Content.Examples) + } + if len(skill.Content.ToolHints) != 2 { + t.Fatalf("unexpected tool hints: %+v", skill.Content.ToolHints) + } + }, + }, + { + name: "name falls back to id without heading", + raw: `--- +id: no-heading-name +--- +just plain instruction without heading`, + assertSkill: func(t *testing.T, skill Skill) { + t.Helper() + if skill.Descriptor.Name != "no-heading-name" { + t.Fatalf("expected name fallback to id, got %q", skill.Descriptor.Name) + } + }, + }, + { + name: "heading used as name", + raw: `--- +id: heading-name +--- +# Heading Name +## Instruction +Do review`, + assertSkill: func(t *testing.T, skill Skill) { + t.Helper() + if skill.Descriptor.Name != "Heading Name" { + t.Fatalf("expected heading as name, got %q", skill.Descriptor.Name) + } + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + skill, issues, err := parseLocalSkill(root, skillDir, skillPath, tt.raw) + if tt.wantErr != "" { + if err == nil || !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(tt.wantErr)) { + t.Fatalf("expected parse error containing %q, got %v", tt.wantErr, err) + } + if len(issues) == 0 || issues[0].Code != tt.wantIssueCode { + t.Fatalf("expected issue code %q, got %+v", tt.wantIssueCode, issues) + } + return + } + + if err != nil { + t.Fatalf("unexpected parse error: %v", err) + } + if len(issues) != 0 { + t.Fatalf("unexpected issues: %+v", issues) + } + if tt.assertSkill != nil { + tt.assertSkill(t, skill) + } + }) + } +} + +func TestParseLocalSkillDescriptorValidationFailure(t *testing.T) { + t.Parallel() + + root := t.TempDir() + skillDir := filepath.Join(root, "good") + skillPath := filepath.Join(skillDir, skillFileName) + raw := `--- +id: good +name: Good +--- +## Instruction +do it` + + _, issues, err := parseLocalSkillWithValidator( + root, + skillDir, + skillPath, + raw, + func(Descriptor) error { return errors.New("descriptor broken") }, + ) + if err == nil || !strings.Contains(strings.ToLower(err.Error()), "descriptor broken") { + t.Fatalf("expected descriptor validation error, got %v", err) + } + if len(issues) == 0 || issues[0].Code != IssueInvalidMetadata { + t.Fatalf("expected invalid metadata issue, got %+v", issues) + } +} + +func TestSplitFrontMatterCases(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + raw string + wantMeta string + wantBody string + wantHas bool + wantError string + }{ + { + name: "no frontmatter", + raw: "plain text", + wantHas: false, + wantBody: "plain text", + }, + { + name: "valid frontmatter", + raw: `--- +id: a +--- +hello`, + wantMeta: "id: a", + wantBody: "hello", + wantHas: true, + }, + { + name: "missing end marker", + raw: `--- +id: a`, + wantError: "end marker not found", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + meta, body, has, err := splitFrontMatter(tt.raw) + if tt.wantError != "" { + if err == nil || !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(tt.wantError)) { + t.Fatalf("expected error containing %q, got %v", tt.wantError, err) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if has != tt.wantHas { + t.Fatalf("has = %v, want %v", has, tt.wantHas) + } + if meta != tt.wantMeta { + t.Fatalf("meta = %q, want %q", meta, tt.wantMeta) + } + if body != tt.wantBody { + t.Fatalf("body = %q, want %q", body, tt.wantBody) + } + }) + } +} + +func TestLoaderHelperFunctions(t *testing.T) { + t.Parallel() + + if got := deriveSkillID("___"); got != "skill" { + t.Fatalf("deriveSkillID fallback = %q, want skill", got) + } + if got := deriveSkillID(filepath.Join("tmp", "Go Review")); got != "go-review" { + t.Fatalf("deriveSkillID normalize = %q, want go-review", got) + } + + if got := firstHeading("no heading"); got != "" { + t.Fatalf("firstHeading = %q, want empty", got) + } + if got := firstHeading("# My Heading\ncontent"); got != "My Heading" { + t.Fatalf("firstHeading = %q, want My Heading", got) + } + if got := firstHeading("## Section Heading\ncontent"); got != "" { + t.Fatalf("firstHeading(level2) = %q, want empty", got) + } + + if got := normalizedHeading("## Tool-Hints "); got != "tool-hints" { + t.Fatalf("normalizedHeading = %q", got) + } + if got := normalizedHeading("plain text"); got != "" { + t.Fatalf("normalizedHeading for plain text = %q, want empty", got) + } + + refs := parseReferences("- [Doc](./doc.md)\n- ./raw.txt") + if len(refs) != 2 || refs[0].Title != "Doc" || refs[1].Path != "./raw.txt" { + t.Fatalf("parseReferences unexpected result: %+v", refs) + } + + items := parseListItems("- one\n* two\n\n") + if len(items) != 2 || items[0] != "one" || items[1] != "two" { + t.Fatalf("parseListItems unexpected result: %+v", items) + } + + merged := mergeToolHints( + []string{"bash", "BASH", "filesystem_read_file", " "}, + []string{"filesystem_read_file", "webfetch"}, + ) + if len(merged) != 3 { + t.Fatalf("mergeToolHints unexpected result: %+v", merged) + } +} + +func TestParseSectionsAliasesAndDefaultBranch(t *testing.T) { + t.Parallel() + + sections := parseSections(`## Reference +- [Doc](./doc.md) +## Example +- e1 +## Tool-Hints +- bash +## Unknown +go back to instruction`) + + if !strings.Contains(sections["references"], "[Doc](./doc.md)") { + t.Fatalf("references section unexpected: %q", sections["references"]) + } + if !strings.Contains(sections["examples"], "e1") { + t.Fatalf("examples section unexpected: %q", sections["examples"]) + } + if !strings.Contains(sections["toolhints"], "bash") { + t.Fatalf("toolhints section unexpected: %q", sections["toolhints"]) + } + if !strings.Contains(sections["instruction"], "go back to instruction") { + t.Fatalf("instruction section unexpected: %q", sections["instruction"]) + } +} + +type staticLoader struct { + snapshot Snapshot + err error +} + +func (l staticLoader) Load(ctx context.Context) (Snapshot, error) { + return l.snapshot, l.err +} + +func TestMemoryRegistryBoundaryCases(t *testing.T) { + t.Parallel() + + t.Run("nil registry methods", func(t *testing.T) { + t.Parallel() + var r *MemoryRegistry + if issues := r.Issues(); issues != nil { + t.Fatalf("expected nil issues, got %+v", issues) + } + err := r.Refresh(context.Background()) + if err == nil || !strings.Contains(strings.ToLower(err.Error()), "registry is nil") { + t.Fatalf("expected nil registry error, got %v", err) + } + }) + + t.Run("nil loader", func(t *testing.T) { + t.Parallel() + r := NewRegistry(nil) + err := r.Refresh(context.Background()) + if err == nil || !strings.Contains(strings.ToLower(err.Error()), "loader is nil") { + t.Fatalf("expected nil loader error, got %v", err) + } + }) + + t.Run("refresh canceled by context", func(t *testing.T) { + t.Parallel() + r := NewRegistry(staticLoader{}) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + err := r.Refresh(ctx) + if !errors.Is(err, context.Canceled) { + t.Fatalf("expected context canceled, got %v", err) + } + }) + + t.Run("list/get with canceled context", func(t *testing.T) { + t.Parallel() + r := NewRegistry(staticLoader{ + snapshot: Snapshot{ + Skills: []Skill{{ + Descriptor: Descriptor{ + ID: "go-review", + Name: "Go Review", + Source: Source{ + Kind: SourceKindLocal, + }, + Scope: ScopeExplicit, + }, + Content: Content{Instruction: "test"}, + }}, + }, + }) + if err := r.Refresh(context.Background()); err != nil { + t.Fatalf("refresh: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + if _, err := r.List(ctx, ListInput{}); !errors.Is(err, context.Canceled) { + t.Fatalf("expected context canceled from List, got %v", err) + } + if _, _, err := r.Get(ctx, "go-review"); !errors.Is(err, context.Canceled) { + t.Fatalf("expected context canceled from Get, got %v", err) + } + }) + + t.Run("get empty id", func(t *testing.T) { + t.Parallel() + r := NewRegistry(staticLoader{ + snapshot: Snapshot{ + Skills: []Skill{{ + Descriptor: Descriptor{ + ID: "go-review", + Name: "Go Review", + Source: Source{ + Kind: SourceKindLocal, + }, + Scope: ScopeExplicit, + }, + Content: Content{Instruction: "test"}, + }}, + }, + }) + if err := r.Refresh(context.Background()); err != nil { + t.Fatalf("refresh: %v", err) + } + _, _, err := r.Get(context.Background(), " ") + if err == nil || !errors.Is(err, ErrSkillNotFound) { + t.Fatalf("expected ErrSkillNotFound for empty id, got %v", err) + } + }) + + t.Run("refresh records invalid normalized id issue", func(t *testing.T) { + t.Parallel() + r := NewRegistry(staticLoader{ + snapshot: Snapshot{ + Skills: []Skill{ + { + Descriptor: Descriptor{ + ID: "---", + Name: "Bad", + Source: Source{ + Kind: SourceKindLocal, + }, + Scope: ScopeExplicit, + }, + Content: Content{Instruction: "bad"}, + }, + }, + }, + }) + if err := r.Refresh(context.Background()); err != nil { + t.Fatalf("refresh: %v", err) + } + issues := r.Issues() + if len(issues) == 0 || issues[0].Code != IssueInvalidMetadata { + t.Fatalf("expected invalid metadata issue, got %+v", issues) + } + }) + + t.Run("list and get return ensureLoaded error", func(t *testing.T) { + t.Parallel() + r := NewRegistry(failingLoader{}) + if _, err := r.List(context.Background(), ListInput{}); err == nil { + t.Fatalf("expected list error") + } + r2 := NewRegistry(failingLoader{}) + if _, _, err := r2.Get(context.Background(), "x"); err == nil { + t.Fatalf("expected get error") + } + }) + + t.Run("ensureLoaded returns nil when already loaded", func(t *testing.T) { + t.Parallel() + r := NewRegistry(staticLoader{ + snapshot: Snapshot{ + Skills: []Skill{{ + Descriptor: Descriptor{ + ID: "x", + Name: "X", + Source: Source{ + Kind: SourceKindLocal, + }, + Scope: ScopeExplicit, + }, + Content: Content{Instruction: "x"}, + }}, + }, + }) + if err := r.Refresh(context.Background()); err != nil { + t.Fatalf("refresh: %v", err) + } + if err := r.ensureLoaded(context.Background()); err != nil { + t.Fatalf("ensureLoaded: %v", err) + } + }) + + t.Run("list skips by source filter", func(t *testing.T) { + t.Parallel() + r := NewRegistry(staticLoader{ + snapshot: Snapshot{ + Skills: []Skill{{ + Descriptor: Descriptor{ + ID: "x", + Name: "X", + Source: Source{ + Kind: SourceKindLocal, + }, + Scope: ScopeExplicit, + }, + Content: Content{Instruction: "x"}, + }}, + }, + }) + list, err := r.List(context.Background(), ListInput{ + SourceKinds: []SourceKind{SourceKindBuiltin}, + }) + if err != nil { + t.Fatalf("list: %v", err) + } + if len(list) != 0 { + t.Fatalf("expected source filtered empty list, got %d", len(list)) + } + }) +} + +type fakeDirInfo struct { + name string +} + +func (f fakeDirInfo) Name() string { return f.name } +func (f fakeDirInfo) Size() int64 { return 0 } +func (f fakeDirInfo) Mode() os.FileMode { return os.ModeDir | 0o755 } +func (f fakeDirInfo) ModTime() time.Time { return time.Time{} } +func (f fakeDirInfo) IsDir() bool { return true } +func (f fakeDirInfo) Sys() any { return nil } + +type errAfterNCallsContext struct { + calls int + cancelAt int +} + +func (c *errAfterNCallsContext) Deadline() (time.Time, bool) { return time.Time{}, false } +func (c *errAfterNCallsContext) Done() <-chan struct{} { return nil } +func (c *errAfterNCallsContext) Value(any) any { return nil } +func (c *errAfterNCallsContext) Err() error { + c.calls++ + if c.calls >= c.cancelAt { + return context.Canceled + } + return nil +} diff --git a/internal/skills/errors.go b/internal/skills/errors.go new file mode 100644 index 00000000..2d57dd95 --- /dev/null +++ b/internal/skills/errors.go @@ -0,0 +1,12 @@ +package skills + +import "errors" + +var ( + // ErrSkillNotFound indicates registry cannot find a skill by id. + ErrSkillNotFound = errors.New("skills: skill not found") + // ErrEmptyContent indicates parsed skill content has no usable instruction. + ErrEmptyContent = errors.New("skills: content is empty") + // ErrSkillRootNotFound indicates configured local root does not exist. + ErrSkillRootNotFound = errors.New("skills: root directory not found") +) diff --git a/internal/skills/filter.go b/internal/skills/filter.go new file mode 100644 index 00000000..9a05dee2 --- /dev/null +++ b/internal/skills/filter.go @@ -0,0 +1,33 @@ +package skills + +import "strings" + +func allowBySource(input ListInput, descriptor Descriptor) bool { + if len(input.SourceKinds) == 0 { + return true + } + for _, kind := range input.SourceKinds { + if descriptor.Source.Kind == kind { + return true + } + } + return false +} + +func allowByScope(input ListInput, descriptor Descriptor) bool { + if len(input.Scopes) > 0 { + for _, scope := range input.Scopes { + if descriptor.Scope == scope { + return true + } + } + return false + } + + switch descriptor.Scope { + case ScopeWorkspace: + return strings.TrimSpace(input.Workspace) != "" + default: + return true + } +} diff --git a/internal/skills/loader.go b/internal/skills/loader.go new file mode 100644 index 00000000..9290d811 --- /dev/null +++ b/internal/skills/loader.go @@ -0,0 +1,562 @@ +package skills + +import ( + "context" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "regexp" + "sort" + "strings" + + "gopkg.in/yaml.v3" +) + +const skillFileName = "SKILL.md" +const defaultMaxSkillFileBytes int64 = 1 << 20 + +var headingPattern = regexp.MustCompile(`^\s{0,3}#{1,6}\s+(.+?)\s*$`) +var firstLevelHeadingPattern = regexp.MustCompile(`^\s{0,3}#\s+(.+?)\s*$`) +var errSkillFileTooLarge = errors.New("skill file exceeds size limit") + +// LocalLoader scans one root directory and loads local skills. +type LocalLoader struct { + root string + + absPath func(string) (string, error) + statPath func(string) (os.FileInfo, error) + lstatPath func(string) (os.FileInfo, error) + readDir func(string) ([]os.DirEntry, error) + readSkillFile func(string, int64) ([]byte, error) + maxFileBytes int64 + validateDescriptor func(Descriptor) error +} + +// NewLocalLoader creates a loader for one local skills root. +func NewLocalLoader(root string) *LocalLoader { + return &LocalLoader{ + root: strings.TrimSpace(root), + absPath: filepath.Abs, + statPath: os.Stat, + lstatPath: os.Lstat, + readDir: os.ReadDir, + readSkillFile: readFileWithLimit, + maxFileBytes: defaultMaxSkillFileBytes, + validateDescriptor: Descriptor.Validate, + } +} + +// Load scans local skill directories and returns parsed skills + non-fatal issues. +func (l *LocalLoader) Load(ctx context.Context) (Snapshot, error) { + if err := ctx.Err(); err != nil { + return Snapshot{}, err + } + root := strings.TrimSpace(l.root) + if root == "" { + return Snapshot{}, fmt.Errorf("%w: empty root", ErrSkillRootNotFound) + } + + absPath := l.absPath + if absPath == nil { + absPath = filepath.Abs + } + statPath := l.statPath + if statPath == nil { + statPath = os.Stat + } + lstatPath := l.lstatPath + if lstatPath == nil { + lstatPath = os.Lstat + } + readDir := l.readDir + if readDir == nil { + readDir = os.ReadDir + } + readSkillFile := l.readSkillFile + if readSkillFile == nil { + readSkillFile = readFileWithLimit + } + validateDescriptor := l.validateDescriptor + if validateDescriptor == nil { + validateDescriptor = Descriptor.Validate + } + maxFileBytes := l.maxFileBytes + if maxFileBytes <= 0 { + maxFileBytes = defaultMaxSkillFileBytes + } + + absRoot, err := absPath(root) + if err != nil { + return Snapshot{}, fmt.Errorf("skills: resolve root %q: %w", root, err) + } + info, err := statPath(absRoot) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return Snapshot{}, fmt.Errorf("%w: %s", ErrSkillRootNotFound, absRoot) + } + return Snapshot{}, fmt.Errorf("skills: stat root %q: %w", absRoot, err) + } + if !info.IsDir() { + return Snapshot{}, fmt.Errorf("skills: root %q is not directory", absRoot) + } + + entries, err := readDir(absRoot) + if err != nil { + return Snapshot{}, fmt.Errorf("skills: read root %q: %w", absRoot, err) + } + + snapshot := Snapshot{ + Skills: make([]Skill, 0, len(entries)+1), + Issues: make([]LoadIssue, 0, len(entries)+1), + } + + candidates := make([]string, 0, len(entries)+1) + rootSkillFile := filepath.Join(absRoot, skillFileName) + _, rootSkillErr := statPath(rootSkillFile) + if rootSkillErr == nil { + candidates = append(candidates, absRoot) + } else if !errors.Is(rootSkillErr, os.ErrNotExist) { + snapshot.Issues = append(snapshot.Issues, LoadIssue{ + Code: IssueReadFailed, + Path: rootSkillFile, + Message: "stat skill file failed", + Err: rootSkillErr, + }) + } + + for _, entry := range entries { + if !entry.IsDir() { + continue + } + name := strings.TrimSpace(entry.Name()) + if name == "" || strings.HasPrefix(name, ".") { + continue + } + candidates = append(candidates, filepath.Join(absRoot, name)) + } + sort.Strings(candidates) + for _, skillDir := range candidates { + if err := ctx.Err(); err != nil { + return Snapshot{}, err + } + + skillPath := filepath.Join(skillDir, skillFileName) + fileInfo, statErr := lstatPath(skillPath) + if statErr != nil { + if errors.Is(statErr, os.ErrNotExist) { + snapshot.Issues = append(snapshot.Issues, LoadIssue{ + Code: IssueSkillFileMissing, + Path: skillPath, + Message: "missing SKILL.md", + Err: statErr, + }) + continue + } + snapshot.Issues = append(snapshot.Issues, LoadIssue{ + Code: IssueReadFailed, + Path: skillPath, + Message: "stat skill file failed", + Err: statErr, + }) + continue + } + if !fileInfo.Mode().IsRegular() { + snapshot.Issues = append(snapshot.Issues, LoadIssue{ + Code: IssueReadFailed, + Path: skillPath, + Message: "skill file is not regular file", + }) + continue + } + if fileInfo.Size() > maxFileBytes { + snapshot.Issues = append(snapshot.Issues, LoadIssue{ + Code: IssueReadFailed, + Path: skillPath, + Message: fmt.Sprintf("skill file exceeds size limit (%d bytes)", maxFileBytes), + }) + continue + } + + data, readErr := readSkillFile(skillPath, maxFileBytes) + if readErr != nil { + if errors.Is(readErr, os.ErrNotExist) { + snapshot.Issues = append(snapshot.Issues, LoadIssue{ + Code: IssueSkillFileMissing, + Path: skillPath, + Message: "missing SKILL.md", + Err: readErr, + }) + continue + } + if errors.Is(readErr, errSkillFileTooLarge) { + snapshot.Issues = append(snapshot.Issues, LoadIssue{ + Code: IssueReadFailed, + Path: skillPath, + Message: fmt.Sprintf("skill file exceeds size limit (%d bytes)", maxFileBytes), + Err: readErr, + }) + continue + } + snapshot.Issues = append(snapshot.Issues, LoadIssue{ + Code: IssueReadFailed, + Path: skillPath, + Message: "read skill file failed", + Err: readErr, + }) + continue + } + + skill, parseIssues, parseErr := parseLocalSkillWithValidator( + absRoot, + skillDir, + skillPath, + string(data), + validateDescriptor, + ) + if len(parseIssues) > 0 { + snapshot.Issues = append(snapshot.Issues, parseIssues...) + } + if parseErr != nil { + continue + } + snapshot.Skills = append(snapshot.Skills, skill) + } + + return snapshot, nil +} + +type skillFrontMatter struct { + ID string `yaml:"id"` + Name string `yaml:"name"` + Description string `yaml:"description"` + Version string `yaml:"version"` + Source string `yaml:"source"` + Scope string `yaml:"scope"` + ToolHints []string `yaml:"tool_hints"` +} + +// parseLocalSkill 解析单个本地技能文件并返回结构化结果与非致命问题。 +func parseLocalSkill(root, skillDir, skillPath, raw string) (Skill, []LoadIssue, error) { + return parseLocalSkillWithValidator(root, skillDir, skillPath, raw, Descriptor.Validate) +} + +// parseLocalSkillWithValidator 与 parseLocalSkill 逻辑一致,但允许注入校验器以覆盖异常分支测试。 +func parseLocalSkillWithValidator( + root, skillDir, skillPath, raw string, + validateDescriptor func(Descriptor) error, +) (Skill, []LoadIssue, error) { + metaText, body, hasMeta, splitErr := splitFrontMatter(raw) + if splitErr != nil { + issue := LoadIssue{ + Code: IssueInvalidMetadata, + Path: skillPath, + Message: "invalid frontmatter", + Err: splitErr, + } + return Skill{}, []LoadIssue{issue}, splitErr + } + + meta := skillFrontMatter{} + if hasMeta { + if err := yaml.Unmarshal([]byte(metaText), &meta); err != nil { + issue := LoadIssue{ + Code: IssueInvalidMetadata, + Path: skillPath, + Message: "frontmatter parse failed", + Err: err, + } + return Skill{}, []LoadIssue{issue}, err + } + } + + scope := ScopeExplicit + if strings.TrimSpace(meta.Scope) != "" { + scope = ActivationScope(strings.TrimSpace(meta.Scope)) + if err := scope.Validate(); err != nil { + issue := LoadIssue{ + Code: IssueInvalidMetadata, + Path: skillPath, + Message: "invalid scope", + Err: err, + } + return Skill{}, []LoadIssue{issue}, err + } + } + + if src := strings.TrimSpace(meta.Source); src != "" { + sourceKind := SourceKind(src) + if err := sourceKind.Validate(); err != nil { + issue := LoadIssue{ + Code: IssueInvalidMetadata, + Path: skillPath, + Message: "invalid source kind", + Err: err, + } + return Skill{}, []LoadIssue{issue}, err + } + if sourceKind != SourceKindLocal { + issue := LoadIssue{ + Code: IssueUnsupportedSource, + Path: skillPath, + Message: "local loader only supports source=local", + } + return Skill{}, []LoadIssue{issue}, errors.New(issue.Message) + } + } + + sections := parseSections(body) + instruction := strings.TrimSpace(sections["instruction"]) + if instruction == "" { + instruction = strings.TrimSpace(body) + } + content := Content{ + Instruction: instruction, + References: parseReferences(sections["references"]), + Examples: parseListItems(sections["examples"]), + ToolHints: mergeToolHints(parseListItems(sections["toolhints"]), meta.ToolHints), + } + if err := content.Validate(); err != nil { + issue := LoadIssue{ + Code: IssueEmptyContent, + Path: skillPath, + Message: "skill content is empty", + Err: err, + } + return Skill{}, []LoadIssue{issue}, err + } + + id := strings.TrimSpace(meta.ID) + if id == "" { + id = deriveSkillID(skillDir) + } + name := strings.TrimSpace(meta.Name) + if name == "" { + name = firstHeading(body) + } + if strings.TrimSpace(name) == "" { + name = id + } + version := strings.TrimSpace(meta.Version) + if version == "" { + version = "v1" + } + + descriptor := Descriptor{ + ID: id, + Name: name, + Description: strings.TrimSpace(meta.Description), + Version: version, + Source: Source{ + Kind: SourceKindLocal, + RootDir: root, + SkillDir: skillDir, + FilePath: skillPath, + }, + Scope: scope, + } + if err := validateDescriptor(descriptor); err != nil { + issue := LoadIssue{ + Code: IssueInvalidMetadata, + Path: skillPath, + SkillID: id, + Message: "descriptor validation failed", + Err: err, + } + return Skill{}, []LoadIssue{issue}, err + } + + return Skill{ + Descriptor: descriptor, + Content: content, + }, nil, nil +} + +func splitFrontMatter(raw string) (meta string, body string, has bool, err error) { + trimmed := strings.ReplaceAll(raw, "\r\n", "\n") + if !strings.HasPrefix(trimmed, "---\n") { + return "", trimmed, false, nil + } + + lines := strings.Split(trimmed, "\n") + endLine := -1 + for i := 1; i < len(lines); i++ { + if strings.TrimSpace(lines[i]) == "---" { + endLine = i + break + } + } + if endLine < 0 { + return "", "", false, errors.New("frontmatter end marker not found") + } + + meta = strings.TrimSpace(strings.Join(lines[1:endLine], "\n")) + body = strings.TrimSpace(strings.Join(lines[endLine+1:], "\n")) + return meta, body, true, nil +} + +func deriveSkillID(skillDir string) string { + base := strings.ToLower(strings.TrimSpace(filepath.Base(skillDir))) + base = strings.ReplaceAll(base, "_", "-") + base = strings.ReplaceAll(base, " ", "-") + base = strings.Trim(base, "-") + if base == "" { + return "skill" + } + return base +} + +func firstHeading(body string) string { + lines := strings.Split(strings.ReplaceAll(body, "\r\n", "\n"), "\n") + for _, line := range lines { + m := firstLevelHeadingPattern.FindStringSubmatch(line) + if len(m) != 2 { + continue + } + return strings.TrimSpace(strings.Trim(m[1], "#")) + } + return "" +} + +// readFileWithLimit 以受限方式读取技能文件,避免读取阶段超过内存阈值。 +func readFileWithLimit(path string, maxBytes int64) ([]byte, error) { + if maxBytes <= 0 { + return nil, fmt.Errorf("%w: %d", errSkillFileTooLarge, maxBytes) + } + file, err := os.Open(path) + if err != nil { + return nil, err + } + defer file.Close() + + limited := io.LimitReader(file, maxBytes+1) + data, err := io.ReadAll(limited) + if err != nil { + return nil, err + } + if int64(len(data)) > maxBytes { + return nil, fmt.Errorf("%w: %d", errSkillFileTooLarge, maxBytes) + } + return data, nil +} + +func parseSections(body string) map[string]string { + sections := map[string]string{ + "instruction": "", + "references": "", + "examples": "", + "toolhints": "", + } + current := "instruction" + lines := strings.Split(strings.ReplaceAll(body, "\r\n", "\n"), "\n") + + appendLine := func(key string, line string) { + if sections[key] == "" { + sections[key] = line + return + } + sections[key] += "\n" + line + } + + for _, line := range lines { + heading := normalizedHeading(line) + if heading != "" { + switch heading { + case "instruction": + current = "instruction" + continue + case "references", "reference": + current = "references" + continue + case "examples", "example": + current = "examples" + continue + case "toolhints", "tool-hints", "tool_hints": + current = "toolhints" + continue + default: + current = "instruction" + } + } + appendLine(current, line) + } + + for key := range sections { + sections[key] = strings.TrimSpace(sections[key]) + } + return sections +} + +func normalizedHeading(line string) string { + m := headingPattern.FindStringSubmatch(line) + if len(m) != 2 { + return "" + } + heading := strings.ToLower(strings.TrimSpace(m[1])) + heading = strings.ReplaceAll(heading, " ", "") + return heading +} + +var markdownLinkPattern = regexp.MustCompile(`\[(?P[^\]]+)\]\((?P<path>[^)]+)\)`) + +func parseReferences(text string) []Reference { + lines := strings.Split(strings.ReplaceAll(text, "\r\n", "\n"), "\n") + refs := make([]Reference, 0, len(lines)) + for _, line := range lines { + trimmed := strings.TrimSpace(strings.TrimLeft(line, "-*")) + if trimmed == "" { + continue + } + + m := markdownLinkPattern.FindStringSubmatch(trimmed) + if len(m) == 3 { + refs = append(refs, Reference{ + Title: strings.TrimSpace(m[1]), + Path: strings.TrimSpace(m[2]), + }) + continue + } + refs = append(refs, Reference{ + Path: trimmed, + }) + } + return refs +} + +func parseListItems(text string) []string { + lines := strings.Split(strings.ReplaceAll(text, "\r\n", "\n"), "\n") + items := make([]string, 0, len(lines)) + for _, line := range lines { + trimmed := strings.TrimSpace(strings.TrimLeft(line, "-*")) + if trimmed == "" { + continue + } + items = append(items, trimmed) + } + return items +} + +func mergeToolHints(sectionHints []string, metadataHints []string) []string { + out := make([]string, 0, len(sectionHints)+len(metadataHints)) + seen := make(map[string]struct{}, len(sectionHints)+len(metadataHints)) + appendHint := func(raw string) { + h := strings.TrimSpace(raw) + if h == "" { + return + } + key := strings.ToLower(h) + if _, ok := seen[key]; ok { + return + } + seen[key] = struct{}{} + out = append(out, h) + } + for _, hint := range sectionHints { + appendHint(hint) + } + for _, hint := range metadataHints { + appendHint(hint) + } + return out +} diff --git a/internal/skills/loader_test.go b/internal/skills/loader_test.go new file mode 100644 index 00000000..1d386100 --- /dev/null +++ b/internal/skills/loader_test.go @@ -0,0 +1,417 @@ +package skills + +import ( + "bytes" + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +func TestLocalLoaderLoad(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + setup func(t *testing.T, root string) + wantSkills int + wantIssueCode LoadIssueCode + }{ + { + name: "load success with frontmatter and sections", + setup: func(t *testing.T, root string) { + t.Helper() + writeSkillFile(t, root, "go-review", `--- +id: go-review +name: Go Review +description: review go code +version: v1.2.0 +scope: explicit +source: local +tool_hints: + - filesystem_read_file +--- +# Go Review +## Instruction +Review Go code with concrete findings. + +## References +- [Go Spec](https://go.dev/ref/spec) + +## Examples +- report critical bug with file path and line + +## ToolHints +- filesystem_grep +`) + }, + wantSkills: 1, + }, + { + name: "missing skill file creates issue but no fatal error", + setup: func(t *testing.T, root string) { + t.Helper() + if err := os.MkdirAll(filepath.Join(root, "missing-skill"), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + }, + wantSkills: 0, + wantIssueCode: IssueSkillFileMissing, + }, + { + name: "invalid metadata creates issue and skips bad skill", + setup: func(t *testing.T, root string) { + t.Helper() + writeSkillFile(t, root, "bad-metadata", `--- +id: bad-meta +scope: invalid_scope +--- +# bad +## Instruction +test`) + }, + wantSkills: 0, + wantIssueCode: IssueInvalidMetadata, + }, + { + name: "empty content creates issue and skips bad skill", + setup: func(t *testing.T, root string) { + t.Helper() + writeSkillFile(t, root, "empty-content", `--- +id: empty-content +scope: explicit +--- +`) + }, + wantSkills: 0, + wantIssueCode: IssueEmptyContent, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + root := t.TempDir() + tt.setup(t, root) + + loader := NewLocalLoader(root) + snapshot, err := loader.Load(context.Background()) + if err != nil { + t.Fatalf("Load() error = %v", err) + } + if len(snapshot.Skills) != tt.wantSkills { + t.Fatalf("skills count = %d, want %d", len(snapshot.Skills), tt.wantSkills) + } + if tt.wantIssueCode != "" { + found := false + for _, issue := range snapshot.Issues { + if issue.Code == tt.wantIssueCode { + found = true + break + } + } + if !found { + t.Fatalf("expected issue code %q, got %+v", tt.wantIssueCode, snapshot.Issues) + } + } + + if len(snapshot.Skills) > 0 { + skill := snapshot.Skills[0] + if strings.TrimSpace(skill.Descriptor.ID) == "" { + t.Fatalf("expected non-empty skill id") + } + if strings.TrimSpace(skill.Content.Instruction) == "" { + t.Fatalf("expected non-empty instruction") + } + if skill.Descriptor.Source.Kind != SourceKindLocal { + t.Fatalf("expected source kind local, got %q", skill.Descriptor.Source.Kind) + } + } + }) + } +} + +func TestLocalLoaderLoadBoundaries(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + root string + prepare func(t *testing.T, root string) + expectErr string + }{ + { + name: "empty root", + root: "", + expectErr: "root directory not found", + }, + { + name: "root not exists", + root: filepath.Join(t.TempDir(), "missing-root"), + expectErr: "root directory not found", + }, + { + name: "root is file", + root: filepath.Join(t.TempDir(), "root.txt"), + prepare: func(t *testing.T, root string) { + t.Helper() + if err := os.WriteFile(root, []byte("x"), 0o644); err != nil { + t.Fatalf("write file: %v", err) + } + }, + expectErr: "not directory", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if tt.prepare != nil { + tt.prepare(t, tt.root) + } + loader := NewLocalLoader(tt.root) + _, err := loader.Load(context.Background()) + if err == nil || !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(tt.expectErr)) { + t.Fatalf("expected error containing %q, got %v", tt.expectErr, err) + } + }) + } +} + +func TestLocalLoaderLoadFrontmatterEOF(t *testing.T) { + t.Parallel() + + root := t.TempDir() + writeSkillFile(t, root, "eof-frontmatter", `--- +id: eof-frontmatter +name: EOF Frontmatter +---`) + + loader := NewLocalLoader(root) + snapshot, err := loader.Load(context.Background()) + if err != nil { + t.Fatalf("Load() error = %v", err) + } + if len(snapshot.Skills) != 0 { + t.Fatalf("expected no loaded skill, got %d", len(snapshot.Skills)) + } + + var invalidMetaFound bool + var emptyContentFound bool + for _, issue := range snapshot.Issues { + if issue.Code == IssueInvalidMetadata { + invalidMetaFound = true + } + if issue.Code == IssueEmptyContent { + emptyContentFound = true + } + } + if invalidMetaFound { + t.Fatalf("unexpected invalid metadata issue, got %+v", snapshot.Issues) + } + if !emptyContentFound { + t.Fatalf("expected empty content issue, got %+v", snapshot.Issues) + } +} + +func TestLocalLoaderLoadFileConstraints(t *testing.T) { + t.Parallel() + + t.Run("non regular file reports read issue", func(t *testing.T) { + t.Parallel() + + root := t.TempDir() + writeSkillFile(t, root, "good", "## Instruction\ngood") + + badDir := filepath.Join(root, "bad") + if err := os.MkdirAll(filepath.Join(badDir, skillFileName), 0o755); err != nil { + t.Fatalf("mkdir bad skill dir: %v", err) + } + + loader := NewLocalLoader(root) + snapshot, err := loader.Load(context.Background()) + if err != nil { + t.Fatalf("Load() error = %v", err) + } + if len(snapshot.Skills) != 1 { + t.Fatalf("expected 1 loaded skill, got %d", len(snapshot.Skills)) + } + + found := false + for _, issue := range snapshot.Issues { + if issue.Code == IssueReadFailed && strings.Contains(issue.Message, "not regular") { + found = true + break + } + } + if !found { + t.Fatalf("expected non-regular read issue, got %+v", snapshot.Issues) + } + }) + + t.Run("oversized file reports read issue", func(t *testing.T) { + t.Parallel() + + root := t.TempDir() + writeSkillFile(t, root, "good", "## Instruction\ngood") + + largeDir := filepath.Join(root, "large") + if err := os.MkdirAll(largeDir, 0o755); err != nil { + t.Fatalf("mkdir large dir: %v", err) + } + large := bytes.Repeat([]byte("a"), int(defaultMaxSkillFileBytes)+1) + if err := os.WriteFile(filepath.Join(largeDir, skillFileName), large, 0o644); err != nil { + t.Fatalf("write large SKILL.md: %v", err) + } + + loader := NewLocalLoader(root) + snapshot, err := loader.Load(context.Background()) + if err != nil { + t.Fatalf("Load() error = %v", err) + } + if len(snapshot.Skills) != 1 { + t.Fatalf("expected 1 loaded skill, got %d", len(snapshot.Skills)) + } + + found := false + for _, issue := range snapshot.Issues { + if issue.Code == IssueReadFailed && strings.Contains(issue.Message, "size limit") { + found = true + break + } + } + if !found { + t.Fatalf("expected oversize read issue, got %+v", snapshot.Issues) + } + }) +} + +func TestLocalLoaderLoadReportsRootSkillStatError(t *testing.T) { + t.Parallel() + + root := t.TempDir() + writeSkillFile(t, root, "good", "## Instruction\ngood") + + loader := NewLocalLoader(root) + originStat := loader.statPath + rootSkillPath := filepath.Join(root, skillFileName) + loader.statPath = func(path string) (os.FileInfo, error) { + if path == rootSkillPath { + return nil, os.ErrPermission + } + return originStat(path) + } + + snapshot, err := loader.Load(context.Background()) + if err != nil { + t.Fatalf("Load() error = %v", err) + } + if len(snapshot.Skills) != 1 { + t.Fatalf("expected 1 loaded skill, got %d", len(snapshot.Skills)) + } + + requireLoadIssue(t, snapshot, "root skill stat failure", func(issue LoadIssue) bool { + return issue.Code == IssueReadFailed && issue.Path == rootSkillPath && errors.Is(issue.Err, os.ErrPermission) + }) +} + +func TestLocalLoaderLoadEnforcesSizeLimitAtReadTime(t *testing.T) { + t.Parallel() + + root := t.TempDir() + writeSkillFile(t, root, "good", "## Instruction\ngood") + + largeDir := filepath.Join(root, "large-race") + if err := os.MkdirAll(largeDir, 0o755); err != nil { + t.Fatalf("mkdir large dir: %v", err) + } + largePath := filepath.Join(largeDir, skillFileName) + large := bytes.Repeat([]byte("a"), int(defaultMaxSkillFileBytes)+1) + if err := os.WriteFile(largePath, large, 0o644); err != nil { + t.Fatalf("write large SKILL.md: %v", err) + } + + loader := NewLocalLoader(root) + originLstat := loader.lstatPath + loader.lstatPath = func(path string) (os.FileInfo, error) { + if path == largePath { + return fixedRegularFileInfo{name: skillFileName, size: 1}, nil + } + return originLstat(path) + } + + snapshot, err := loader.Load(context.Background()) + if err != nil { + t.Fatalf("Load() error = %v", err) + } + if len(snapshot.Skills) != 1 { + t.Fatalf("expected 1 loaded skill, got %d", len(snapshot.Skills)) + } + + requireLoadIssue(t, snapshot, "enforced read limit", func(issue LoadIssue) bool { + return issue.Code == IssueReadFailed && issue.Path == largePath && strings.Contains(issue.Message, "size limit") + }) +} + +func TestParseLocalSkillNameFallbackUsesOnlyLevelOneHeading(t *testing.T) { + t.Parallel() + + root := t.TempDir() + skillDir := filepath.Join(root, "heading-fallback") + skillPath := filepath.Join(skillDir, skillFileName) + raw := `--- +id: heading-fallback +--- +## References +- [Doc](./doc.md) +` + + skill, issues, err := parseLocalSkill(root, skillDir, skillPath, raw) + if err != nil { + t.Fatalf("parseLocalSkill() error = %v", err) + } + if len(issues) != 0 { + t.Fatalf("expected no issues, got %+v", issues) + } + if skill.Descriptor.Name != skill.Descriptor.ID { + t.Fatalf("expected name fallback to id, got name=%q id=%q", skill.Descriptor.Name, skill.Descriptor.ID) + } +} + +func requireLoadIssue(t *testing.T, snapshot Snapshot, desc string, match func(LoadIssue) bool) { + t.Helper() + for _, issue := range snapshot.Issues { + if match(issue) { + return + } + } + t.Fatalf("%s: expected matching issue, got %+v", desc, snapshot.Issues) +} + +func writeSkillFile(t *testing.T, root string, dir string, content string) { + t.Helper() + skillDir := filepath.Join(root, dir) + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatalf("mkdir skill dir: %v", err) + } + if err := os.WriteFile(filepath.Join(skillDir, skillFileName), []byte(content), 0o644); err != nil { + t.Fatalf("write SKILL.md: %v", err) + } +} + +type fixedRegularFileInfo struct { + name string + size int64 +} + +func (f fixedRegularFileInfo) Name() string { return f.name } +func (f fixedRegularFileInfo) Size() int64 { return f.size } +func (f fixedRegularFileInfo) Mode() os.FileMode { return 0o644 } +func (f fixedRegularFileInfo) ModTime() time.Time { return time.Time{} } +func (f fixedRegularFileInfo) IsDir() bool { return false } +func (f fixedRegularFileInfo) Sys() any { return nil } diff --git a/internal/skills/registry.go b/internal/skills/registry.go new file mode 100644 index 00000000..385aef0c --- /dev/null +++ b/internal/skills/registry.go @@ -0,0 +1,167 @@ +package skills + +import ( + "context" + "fmt" + "sort" + "strings" + "sync" +) + +// MemoryRegistry is an in-memory skills registry backed by one loader. +type MemoryRegistry struct { + loader Loader + + mu sync.RWMutex + loaded bool + byID map[string]Skill + issues []LoadIssue +} + +// NewRegistry creates a registry from one loader. +func NewRegistry(loader Loader) *MemoryRegistry { + return &MemoryRegistry{ + loader: loader, + byID: map[string]Skill{}, + issues: []LoadIssue{}, + } +} + +// Refresh reloads all skills from the loader and rebuilds the in-memory index. +func (r *MemoryRegistry) Refresh(ctx context.Context) error { + if r == nil { + return fmt.Errorf("skills: registry is nil") + } + if r.loader == nil { + return fmt.Errorf("skills: loader is nil") + } + if err := ctx.Err(); err != nil { + return err + } + + snapshot, err := r.loader.Load(ctx) + if err != nil { + r.mu.Lock() + r.issues = []LoadIssue{{ + Code: IssueRefreshFailed, + Message: "skills refresh failed", + Err: err, + }} + r.mu.Unlock() + return err + } + + index := make(map[string]Skill, len(snapshot.Skills)) + issues := append([]LoadIssue(nil), snapshot.Issues...) + for _, skill := range snapshot.Skills { + key := normalizeSkillID(skill.Descriptor.ID) + if key == "" { + issues = append(issues, LoadIssue{ + Code: IssueInvalidMetadata, + Path: skill.Descriptor.Source.FilePath, + Message: "empty skill id after normalization", + }) + continue + } + if existing, ok := index[key]; ok { + issues = append(issues, LoadIssue{ + Code: IssueDuplicateID, + Path: skill.Descriptor.Source.FilePath, + SkillID: skill.Descriptor.ID, + Message: fmt.Sprintf( + "duplicate skill id %q (already loaded from %s)", + skill.Descriptor.ID, + existing.Descriptor.Source.FilePath, + ), + }) + continue + } + index[key] = skill + } + + r.mu.Lock() + r.loaded = true + r.byID = index + r.issues = issues + r.mu.Unlock() + return nil +} + +// List returns descriptors that match input filters. +func (r *MemoryRegistry) List(ctx context.Context, input ListInput) ([]Descriptor, error) { + if err := r.ensureLoaded(ctx); err != nil { + return nil, err + } + if err := ctx.Err(); err != nil { + return nil, err + } + + r.mu.RLock() + descriptors := make([]Descriptor, 0, len(r.byID)) + for _, skill := range r.byID { + descriptor := skill.Descriptor + if !allowBySource(input, descriptor) { + continue + } + if !allowByScope(input, descriptor) { + continue + } + descriptors = append(descriptors, descriptor) + } + r.mu.RUnlock() + + sort.Slice(descriptors, func(i, j int) bool { + return normalizeSkillID(descriptors[i].ID) < normalizeSkillID(descriptors[j].ID) + }) + return descriptors, nil +} + +// Get returns one skill descriptor and content by id. +func (r *MemoryRegistry) Get(ctx context.Context, id string) (Descriptor, Content, error) { + if err := r.ensureLoaded(ctx); err != nil { + return Descriptor{}, Content{}, err + } + if err := ctx.Err(); err != nil { + return Descriptor{}, Content{}, err + } + + key := normalizeSkillID(id) + if key == "" { + return Descriptor{}, Content{}, fmt.Errorf("%w: id is empty", ErrSkillNotFound) + } + + r.mu.RLock() + skill, ok := r.byID[key] + r.mu.RUnlock() + if !ok { + return Descriptor{}, Content{}, fmt.Errorf("%w: %s", ErrSkillNotFound, id) + } + return skill.Descriptor, skill.Content, nil +} + +// Issues returns the latest non-fatal loader/registry issues. +func (r *MemoryRegistry) Issues() []LoadIssue { + if r == nil { + return nil + } + r.mu.RLock() + defer r.mu.RUnlock() + return append([]LoadIssue(nil), r.issues...) +} + +func (r *MemoryRegistry) ensureLoaded(ctx context.Context) error { + r.mu.RLock() + loaded := r.loaded + r.mu.RUnlock() + if loaded { + return nil + } + return r.Refresh(ctx) +} + +func normalizeSkillID(id string) string { + normalized := strings.ToLower(strings.TrimSpace(id)) + normalized = strings.ReplaceAll(normalized, "_", "-") + normalized = strings.ReplaceAll(normalized, " ", "-") + return strings.Trim(normalized, "-") +} diff --git a/internal/skills/registry_test.go b/internal/skills/registry_test.go new file mode 100644 index 00000000..4e9a3ecf --- /dev/null +++ b/internal/skills/registry_test.go @@ -0,0 +1,341 @@ +package skills + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestMemoryRegistryRefreshAndQuery(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + setup func(t *testing.T, root string) + wantList int + wantIssueCode LoadIssueCode + getID string + expectGetErr string + }{ + { + name: "refresh success and get by id", + setup: func(t *testing.T, root string) { + t.Helper() + writeSkillFile(t, root, "go-review", `--- +id: go-review +name: Go Review +scope: explicit +--- +## Instruction +Review code strictly.`) + }, + wantList: 1, + getID: "go-review", + }, + { + name: "duplicate id produces issue but keeps one skill", + setup: func(t *testing.T, root string) { + t.Helper() + writeSkillFile(t, root, "skill-a", `--- +id: duplicate-id +name: A +scope: explicit +--- +## Instruction +A`) + writeSkillFile(t, root, "skill-b", `--- +id: duplicate-id +name: B +scope: explicit +--- +## Instruction +B`) + }, + wantList: 1, + wantIssueCode: IssueDuplicateID, + getID: "duplicate-id", + }, + { + name: "get missing skill returns not found", + setup: func(t *testing.T, root string) { + t.Helper() + writeSkillFile(t, root, "present", `--- +id: present +name: Present +scope: explicit +--- +## Instruction +present`) + }, + wantList: 1, + getID: "missing-id", + expectGetErr: "skill not found", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + root := t.TempDir() + tt.setup(t, root) + + registry := NewRegistry(NewLocalLoader(root)) + if err := registry.Refresh(context.Background()); err != nil { + t.Fatalf("Refresh() error = %v", err) + } + + gotList, err := registry.List(context.Background(), ListInput{}) + if err != nil { + t.Fatalf("List() error = %v", err) + } + if len(gotList) != tt.wantList { + t.Fatalf("List() count = %d, want %d", len(gotList), tt.wantList) + } + + if tt.wantIssueCode != "" { + found := false + for _, issue := range registry.Issues() { + if issue.Code == tt.wantIssueCode { + found = true + break + } + } + if !found { + t.Fatalf("expected issue code %q, got %+v", tt.wantIssueCode, registry.Issues()) + } + } + + _, _, getErr := registry.Get(context.Background(), tt.getID) + if tt.expectGetErr != "" { + if getErr == nil || !strings.Contains(strings.ToLower(getErr.Error()), strings.ToLower(tt.expectGetErr)) { + t.Fatalf("expected get error containing %q, got %v", tt.expectGetErr, getErr) + } + return + } + if getErr != nil { + t.Fatalf("unexpected Get() error: %v", getErr) + } + }) + } +} + +func TestMemoryRegistryRefreshUpdatesResult(t *testing.T) { + t.Parallel() + + root := t.TempDir() + writeSkillFile(t, root, "skill-a", `--- +id: skill-a +name: Skill A +scope: explicit +--- +## Instruction +skill a`) + + registry := NewRegistry(NewLocalLoader(root)) + if err := registry.Refresh(context.Background()); err != nil { + t.Fatalf("initial Refresh() error = %v", err) + } + + listBefore, err := registry.List(context.Background(), ListInput{}) + if err != nil { + t.Fatalf("List() before refresh error = %v", err) + } + if len(listBefore) != 1 { + t.Fatalf("expected 1 skill before refresh, got %d", len(listBefore)) + } + + writeSkillFile(t, root, "skill-b", `--- +id: skill-b +name: Skill B +scope: explicit +--- +## Instruction +skill b`) + + if err := registry.Refresh(context.Background()); err != nil { + t.Fatalf("second Refresh() error = %v", err) + } + listAfter, err := registry.List(context.Background(), ListInput{}) + if err != nil { + t.Fatalf("List() after refresh error = %v", err) + } + if len(listAfter) != 2 { + t.Fatalf("expected 2 skills after refresh, got %d", len(listAfter)) + } +} + +func TestMemoryRegistryListFilters(t *testing.T) { + t.Parallel() + + root := t.TempDir() + writeSkillFile(t, root, "global", `--- +id: global-skill +name: Global +scope: global +--- +## Instruction +global`) + writeSkillFile(t, root, "workspace", `--- +id: workspace-skill +name: Workspace +scope: workspace +--- +## Instruction +workspace`) + + registry := NewRegistry(NewLocalLoader(root)) + if err := registry.Refresh(context.Background()); err != nil { + t.Fatalf("Refresh() error = %v", err) + } + + tests := []struct { + name string + input ListInput + wantCount int + }{ + { + name: "default list hides workspace scope without workspace context", + input: ListInput{}, + wantCount: 1, + }, + { + name: "workspace context includes workspace scope", + input: ListInput{ + Workspace: filepath.Join(root, "repo"), + }, + wantCount: 2, + }, + { + name: "scope filter returns explicit scope subset", + input: ListInput{ + Scopes: []ActivationScope{ScopeWorkspace}, + }, + wantCount: 1, + }, + { + name: "source filter local", + input: ListInput{ + SourceKinds: []SourceKind{SourceKindLocal}, + Workspace: filepath.Join(root, "repo"), + }, + wantCount: 2, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + list, err := registry.List(context.Background(), tt.input) + if err != nil { + t.Fatalf("List() error = %v", err) + } + if len(list) != tt.wantCount { + t.Fatalf("List() count = %d, want %d", len(list), tt.wantCount) + } + }) + } +} + +type failingLoader struct{} + +func (failingLoader) Load(ctx context.Context) (Snapshot, error) { + return Snapshot{}, os.ErrPermission +} + +type flakyLoader struct { + calls int +} + +func (l *flakyLoader) Load(ctx context.Context) (Snapshot, error) { + l.calls++ + if l.calls == 1 { + return Snapshot{}, os.ErrPermission + } + return Snapshot{ + Skills: []Skill{{ + Descriptor: Descriptor{ + ID: "recovered", + Name: "Recovered", + Source: Source{ + Kind: SourceKindLocal, + }, + Scope: ScopeExplicit, + }, + Content: Content{ + Instruction: "ok", + }, + }}, + }, nil +} + +func TestMemoryRegistryRefreshFailure(t *testing.T) { + t.Parallel() + + registry := NewRegistry(failingLoader{}) + err := registry.Refresh(context.Background()) + if err == nil { + t.Fatalf("expected refresh error") + } + if !strings.Contains(strings.ToLower(err.Error()), "permission") { + t.Fatalf("expected permission error, got %v", err) + } + + issues := registry.Issues() + if len(issues) == 0 || issues[0].Code != IssueRefreshFailed { + t.Fatalf("expected refresh failed issue, got %+v", issues) + } +} + +func TestMemoryRegistryEnsureLoadedRetriesAfterFailure(t *testing.T) { + t.Parallel() + + loader := &flakyLoader{} + registry := NewRegistry(loader) + + if _, err := registry.List(context.Background(), ListInput{}); err == nil { + t.Fatalf("expected first list error") + } + if loader.calls != 1 { + t.Fatalf("expected 1 load call after first list, got %d", loader.calls) + } + + list, err := registry.List(context.Background(), ListInput{}) + if err != nil { + t.Fatalf("expected second list success, got %v", err) + } + if loader.calls != 2 { + t.Fatalf("expected 2 load calls after retry, got %d", loader.calls) + } + if len(list) != 1 || list[0].ID != "recovered" { + t.Fatalf("unexpected list after retry: %+v", list) + } +} + +func TestNormalizeSkillID(t *testing.T) { + t.Parallel() + + tests := []struct { + input string + want string + }{ + {input: " Go_Review ", want: "go-review"}, + {input: "skill id", want: "skill-id"}, + {input: "---", want: ""}, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.input, func(t *testing.T) { + t.Parallel() + got := normalizeSkillID(tt.input) + if got != tt.want { + t.Fatalf("normalizeSkillID(%q) = %q, want %q", tt.input, got, tt.want) + } + }) + } +} diff --git a/internal/skills/types.go b/internal/skills/types.go new file mode 100644 index 00000000..9f34c3fb --- /dev/null +++ b/internal/skills/types.go @@ -0,0 +1,181 @@ +package skills + +import ( + "context" + "fmt" + "path/filepath" + "strings" +) + +// SourceKind describes where a skill comes from. +type SourceKind string + +const ( + SourceKindLocal SourceKind = "local" + SourceKindBuiltin SourceKind = "builtin" +) + +// Validate reports whether the source kind is supported. +func (k SourceKind) Validate() error { + switch k { + case SourceKindLocal, SourceKindBuiltin: + return nil + default: + return fmt.Errorf("skills: invalid source kind %q", k) + } +} + +// ActivationScope controls where the skill should be visible/active. +type ActivationScope string + +const ( + ScopeGlobal ActivationScope = "global" + ScopeWorkspace ActivationScope = "workspace" + ScopeSession ActivationScope = "session" + ScopeExplicit ActivationScope = "explicit" +) + +// Validate reports whether the activation scope is supported. +func (s ActivationScope) Validate() error { + switch s { + case ScopeGlobal, ScopeWorkspace, ScopeSession, ScopeExplicit: + return nil + default: + return fmt.Errorf("skills: invalid activation scope %q", s) + } +} + +// Source describes the origin of one skill. +type Source struct { + Kind SourceKind + RootDir string + SkillDir string + FilePath string +} + +// Descriptor is the stable, indexable skill metadata. +type Descriptor struct { + ID string + Name string + Description string + Version string + Source Source + Scope ActivationScope +} + +// Validate reports whether the descriptor is well-formed. +func (d Descriptor) Validate() error { + if strings.TrimSpace(d.ID) == "" { + return fmt.Errorf("skills: descriptor id is empty") + } + if strings.TrimSpace(d.Name) == "" { + return fmt.Errorf("skills: descriptor name is empty") + } + if err := d.Source.Kind.Validate(); err != nil { + return err + } + if err := d.Scope.Validate(); err != nil { + return err + } + return nil +} + +// Reference is one optional doc/reference entry declared by a skill. +type Reference struct { + Path string + Title string + Summary string +} + +// Content is the prompt-facing payload of a skill. +type Content struct { + Instruction string + References []Reference + Examples []string + ToolHints []string +} + +// Validate reports whether content is usable. +func (c Content) Validate() error { + if strings.TrimSpace(c.Instruction) == "" { + return ErrEmptyContent + } + return nil +} + +// Skill bundles descriptor and content. +type Skill struct { + Descriptor Descriptor + Content Content +} + +// LoadIssueCode identifies one non-fatal loader/registry problem. +type LoadIssueCode string + +const ( + IssueSkillFileMissing LoadIssueCode = "skill_file_missing" + IssueInvalidMetadata LoadIssueCode = "invalid_metadata" + IssueEmptyContent LoadIssueCode = "empty_content" + IssueDuplicateID LoadIssueCode = "duplicate_id" + IssueRefreshFailed LoadIssueCode = "refresh_failed" + IssueReadFailed LoadIssueCode = "read_failed" + IssueParseFailed LoadIssueCode = "parse_failed" + IssueUnsupportedSource LoadIssueCode = "unsupported_source" +) + +// LoadIssue captures one non-fatal error so one bad skill does not break others. +type LoadIssue struct { + Code LoadIssueCode + Path string + SkillID string + Message string + Err error +} + +// Error returns a readable issue message. +func (i LoadIssue) Error() string { + base := strings.TrimSpace(i.Message) + if base == "" && i.Err != nil { + base = strings.TrimSpace(i.Err.Error()) + } + if base == "" { + base = string(i.Code) + } + if strings.TrimSpace(i.Path) == "" { + return "skills: " + base + } + return fmt.Sprintf("skills: %s (%s)", base, i.Path) +} + +// Snapshot is one loader output batch. +type Snapshot struct { + Skills []Skill + Issues []LoadIssue +} + +// Loader loads skills from one or many sources. +type Loader interface { + Load(ctx context.Context) (Snapshot, error) +} + +// ListInput controls skill listing filters. +type ListInput struct { + SourceKinds []SourceKind + Scopes []ActivationScope + Workspace string +} + +// Registry is the read/refresh entrypoint used by upper layers. +type Registry interface { + List(ctx context.Context, input ListInput) ([]Descriptor, error) + Get(ctx context.Context, id string) (Descriptor, Content, error) + Refresh(ctx context.Context) error +} + +func normalizeWorkspace(workspace string) string { + trimmed := strings.TrimSpace(workspace) + if trimmed == "" { + return "" + } + return filepath.Clean(trimmed) +}