From 2ca17fce8002e8eaf57dc669e059dc5a002abbc5 Mon Sep 17 00:00:00 2001 From: weiyepeng Date: Thu, 26 Feb 2026 22:12:11 +0800 Subject: [PATCH] feat:add test --- Makefile | 4 +- pkg/cron/service_test.go | 345 +++++++++++++++++++ pkg/skills/loader_test.go | 575 +++++++++++++++++++++++-------- pkg/tools/shell_test.go | 14 +- pkg/tools/skills_install_test.go | 171 +++++---- 5 files changed, 898 insertions(+), 211 deletions(-) diff --git a/Makefile b/Makefile index a5ad4a02d..68b817bbd 100644 --- a/Makefile +++ b/Makefile @@ -126,7 +126,7 @@ clean: @echo "Clean complete" ## vet: Run go vet for static analysis -vet: +vet: generate @$(GO) vet ./... ## test: Test Go code @@ -138,7 +138,7 @@ fmt: @$(GOLANGCI_LINT) fmt ## lint: Run linters -lint: +lint: generate @$(GOLANGCI_LINT) run ## deps: Download dependencies diff --git a/pkg/cron/service_test.go b/pkg/cron/service_test.go index 1a0dd1829..2aa45a54d 100644 --- a/pkg/cron/service_test.go +++ b/pkg/cron/service_test.go @@ -4,7 +4,9 @@ import ( "os" "path/filepath" "runtime" + "sync/atomic" "testing" + "time" ) func TestSaveStore_FilePermissions(t *testing.T) { @@ -33,6 +35,349 @@ func TestSaveStore_FilePermissions(t *testing.T) { } } +// TestCronServiceAddJob tests basic job addition +func TestCronServiceAddJob(t *testing.T) { + tmpDir := t.TempDir() + storePath := filepath.Join(tmpDir, "cron", "jobs.json") + + var executed atomic.Int32 + handler := func(job *CronJob) (string, error) { + executed.Add(1) + return "executed", nil + } + + cs := NewCronService(storePath, handler) + + // Add one-time job + job, err := cs.AddJob("test-job", CronSchedule{Kind: "at", AtMS: int64Ptr(time.Now().UnixMilli() + 1000)}, "test message", true, "cli", "direct") + if err != nil { + t.Fatalf("AddJob failed: %v", err) + } + + if job.Name != "test-job" { + t.Errorf("job name = %q, want %q", job.Name, "test-job") + } + + if job.Schedule.Kind != "at" { + t.Errorf("job kind = %q, want %q", job.Schedule.Kind, "at") + } + + jobs := cs.ListJobs(false) + if len(jobs) != 1 { + t.Errorf("expected 1 job, got %d", len(jobs)) + } +} + +// TestCronServiceAddRecurringJob tests recurring job addition +func TestCronServiceAddRecurringJob(t *testing.T) { + tmpDir := t.TempDir() + storePath := filepath.Join(tmpDir, "cron", "jobs.json") + + cs := NewCronService(storePath, nil) + + // Add recurring job + job, err := cs.AddJob("recurring", CronSchedule{Kind: "every", EveryMS: int64Ptr(3600000)}, "hourly task", true, "cli", "direct") + if err != nil { + t.Fatalf("AddJob failed: %v", err) + } + + if job.Schedule.Kind != "every" { + t.Errorf("job kind = %q, want %q", job.Schedule.Kind, "every") + } + + if job.Schedule.EveryMS == nil || *job.Schedule.EveryMS != 3600000 { + t.Errorf("job everyMS = %v, want 3600000", job.Schedule.EveryMS) + } +} + +// TestCronServiceAddCronJob tests cron expression job addition +func TestCronServiceAddCronJob(t *testing.T) { + tmpDir := t.TempDir() + storePath := filepath.Join(tmpDir, "cron", "jobs.json") + + cs := NewCronService(storePath, nil) + + // Add cron job + job, err := cs.AddJob("daily", CronSchedule{Kind: "cron", Expr: "0 9 * * *"}, "daily at 9am", true, "cli", "direct") + if err != nil { + t.Fatalf("AddJob failed: %v", err) + } + + if job.Schedule.Kind != "cron" { + t.Errorf("job kind = %q, want %q", job.Schedule.Kind, "cron") + } + + if job.Schedule.Expr != "0 9 * * *" { + t.Errorf("job expr = %q, want %q", job.Schedule.Expr, "0 9 * * *") + } +} + +// TestCronServiceRemoveJob tests job removal +func TestCronServiceRemoveJob(t *testing.T) { + tmpDir := t.TempDir() + storePath := filepath.Join(tmpDir, "cron", "jobs.json") + + cs := NewCronService(storePath, nil) + + // Add job + job, _ := cs.AddJob("to-remove", CronSchedule{Kind: "every", EveryMS: int64Ptr(60000)}, "remove me", true, "cli", "direct") + + // Remove job + removed := cs.RemoveJob(job.ID) + if !removed { + t.Error("RemoveJob returned false, want true") + } + + jobs := cs.ListJobs(false) + if len(jobs) != 0 { + t.Errorf("expected 0 jobs after removal, got %d", len(jobs)) + } + + // Remove non-existent job + removed2 := cs.RemoveJob("nonexistent") + if removed2 { + t.Error("RemoveJob should return false for non-existent job") + } +} + +// TestCronServiceEnableDisableJob tests job enable/disable +func TestCronServiceEnableDisableJob(t *testing.T) { + tmpDir := t.TempDir() + storePath := filepath.Join(tmpDir, "cron", "jobs.json") + + cs := NewCronService(storePath, nil) + + // Add job + job, _ := cs.AddJob("toggle", CronSchedule{Kind: "every", EveryMS: int64Ptr(60000)}, "toggle me", true, "cli", "direct") + + // Disable job + disabled := cs.EnableJob(job.ID, false) + if disabled == nil { + t.Fatal("EnableJob returned nil, want job") + } + if disabled.Enabled { + t.Error("job should be disabled") + } + + // Enable job + enabled := cs.EnableJob(job.ID, true) + if enabled == nil { + t.Fatal("EnableJob returned nil, want job") + } + if !enabled.Enabled { + t.Error("job should be enabled") + } + + // Enable non-existent job + notFound := cs.EnableJob("nonexistent", true) + if notFound != nil { + t.Error("EnableJob should return nil for non-existent job") + } +} + +// TestCronServiceUpdateJob tests job update +func TestCronServiceUpdateJob(t *testing.T) { + tmpDir := t.TempDir() + storePath := filepath.Join(tmpDir, "cron", "jobs.json") + + cs := NewCronService(storePath, nil) + + // Add job + job, _ := cs.AddJob("update-test", CronSchedule{Kind: "every", EveryMS: int64Ptr(60000)}, "original", true, "cli", "direct") + + // Update job + job.Payload.Message = "updated message" + err := cs.UpdateJob(job) + if err != nil { + t.Fatalf("UpdateJob failed: %v", err) + } + + // Reload and verify + cs.Load() + updatedJob := cs.ListJobs(true)[0] + if updatedJob.Payload.Message != "updated message" { + t.Errorf("job message = %q, want %q", updatedJob.Payload.Message, "updated message") + } + + // Update non-existent job + job.ID = "nonexistent" + err = cs.UpdateJob(job) + if err == nil { + t.Error("UpdateJob should fail for non-existent job") + } +} + +// TestCronServiceListJobs tests job listing +func TestCronServiceListJobs(t *testing.T) { + tmpDir := t.TempDir() + storePath := filepath.Join(tmpDir, "cron", "jobs.json") + + cs := NewCronService(storePath, nil) + + // Add jobs + cs.AddJob("job1", CronSchedule{Kind: "every", EveryMS: int64Ptr(60000)}, "msg1", true, "cli", "direct") + cs.AddJob("job2", CronSchedule{Kind: "every", EveryMS: int64Ptr(120000)}, "msg2", true, "cli", "direct") + + // Disable one job + jobs := cs.ListJobs(true) + if len(jobs) != 2 { + t.Fatalf("expected 2 jobs, got %d", len(jobs)) + } + cs.EnableJob(jobs[0].ID, false) + + // List only enabled jobs + enabledJobs := cs.ListJobs(false) + if len(enabledJobs) != 1 { + t.Errorf("expected 1 enabled job, got %d", len(enabledJobs)) + } + + // List all jobs including disabled + allJobs := cs.ListJobs(true) + if len(allJobs) != 2 { + t.Errorf("expected 2 total jobs, got %d", len(allJobs)) + } +} + +// TestCronServiceStartStop tests service start and stop +func TestCronServiceStartStop(t *testing.T) { + tmpDir := t.TempDir() + storePath := filepath.Join(tmpDir, "cron", "jobs.json") + + cs := NewCronService(storePath, nil) + + // Start service + err := cs.Start() + if err != nil { + t.Fatalf("Start failed: %v", err) + } + + if !cs.running { + t.Error("service should be running") + } + + // Stop service + cs.Stop() + + if cs.running { + t.Error("service should be stopped") + } + + // Start again (should work) + err = cs.Start() + if err != nil { + t.Fatalf("Start second time failed: %v", err) + } + cs.Stop() +} + +// TestCronServiceStatus tests status reporting +func TestCronServiceStatus(t *testing.T) { + tmpDir := t.TempDir() + storePath := filepath.Join(tmpDir, "cron", "jobs.json") + + cs := NewCronService(storePath, nil) + + // Add jobs + cs.AddJob("active", CronSchedule{Kind: "every", EveryMS: int64Ptr(60000)}, "msg", true, "cli", "direct") + cs.AddJob("inactive", CronSchedule{Kind: "every", EveryMS: int64Ptr(120000)}, "msg2", true, "cli", "direct") + jobs := cs.ListJobs(true) + if len(jobs) > 0 { + cs.EnableJob(jobs[0].ID, false) + } + + status := cs.Status() + + if status["jobs"] != 2 { + t.Errorf("expected 2 jobs, got %v", status["jobs"]) + } + + // Check if service is running (should be false as we didn't start it) + isRunning, ok := status["enabled"].(bool) + if !ok { + t.Errorf("expected 'enabled' to be bool, got %T", status["enabled"]) + } + if isRunning { + t.Error("service should not be running until Start() is called") + } + + // Verify enabled job count separately + enabledJobs := cs.ListJobs(false) + if len(enabledJobs) != 1 { + t.Errorf("expected 1 enabled job, got %d", len(enabledJobs)) + } +} + +// TestCronServiceComputeNextRun tests next run computation +func TestCronServiceComputeNextRun(t *testing.T) { + tmpDir := t.TempDir() + storePath := filepath.Join(tmpDir, "cron", "jobs.json") + + cs := NewCronService(storePath, nil) + + now := time.Now().UnixMilli() + + // Test "at" schedule + atTime := now + 5000 + nextRun := cs.computeNextRun(&CronSchedule{Kind: "at", AtMS: &atTime}, now) + if nextRun == nil || *nextRun != atTime { + t.Errorf("at schedule nextRun = %v, want %v", nextRun, atTime) + } + + // Test "every" schedule + everyMS := int64(3600000) + nextRun = cs.computeNextRun(&CronSchedule{Kind: "every", EveryMS: &everyMS}, now) + if nextRun == nil || *nextRun != now+everyMS { + t.Errorf("every schedule nextRun = %v, want %v", nextRun, now+everyMS) + } +} + +// TestCronServicePersistence tests that jobs persist across restarts +func TestCronServicePersistence(t *testing.T) { + tmpDir := t.TempDir() + storePath := filepath.Join(tmpDir, "cron", "jobs.json") + + // Create service and add job + cs1 := NewCronService(storePath, nil) + cs1.AddJob("persistent", CronSchedule{Kind: "every", EveryMS: int64Ptr(60000)}, "persist", true, "cli", "direct") + + // Create new service instance (simulates restart) + cs2 := NewCronService(storePath, nil) + cs2.Load() + + jobs := cs2.ListJobs(true) + if len(jobs) != 1 { + t.Errorf("expected 1 persisted job, got %d", len(jobs)) + } + + if jobs[0].Name != "persistent" { + t.Errorf("job name = %q, want %q", jobs[0].Name, "persistent") + } +} + +// TestCronServiceWithCommand tests job with command payload +func TestCronServiceWithCommand(t *testing.T) { + tmpDir := t.TempDir() + storePath := filepath.Join(tmpDir, "cron", "jobs.json") + + cs := NewCronService(storePath, nil) + + // Add job with command + job, err := cs.AddJob("cmd-job", CronSchedule{Kind: "at", AtMS: int64Ptr(time.Now().UnixMilli() + 1000)}, "check disk", true, "cli", "direct") + if err != nil { + t.Fatalf("AddJob failed: %v", err) + } + + job.Payload.Command = "df -h" + cs.UpdateJob(job) + + // Reload and verify + cs.Load() + loadedJob := cs.ListJobs(true)[0] + if loadedJob.Payload.Command != "df -h" { + t.Errorf("command = %q, want %q", loadedJob.Payload.Command, "df -h") + } +} + func int64Ptr(v int64) *int64 { return &v } diff --git a/pkg/skills/loader_test.go b/pkg/skills/loader_test.go index aca901d33..c109e5923 100644 --- a/pkg/skills/loader_test.go +++ b/pkg/skills/loader_test.go @@ -1,74 +1,332 @@ package skills import ( + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func TestSkillsInfoValidate(t *testing.T) { - testcases := []struct { - name string - skillName string - description string - wantErr bool - errContains []string +func TestSkillsLoaderListSkillsEmpty(t *testing.T) { + workspace := t.TempDir() + globalSkills := t.TempDir() + builtinSkills := t.TempDir() + + loader := NewSkillsLoader(workspace, globalSkills, builtinSkills) + skills := loader.ListSkills() + assert.Empty(t, skills) +} + +func TestSkillsLoaderListSkillsWorkspace(t *testing.T) { + workspace := t.TempDir() + globalSkills := t.TempDir() + builtinSkills := t.TempDir() + + // Create workspace skill (name must be alphanumeric with hyphens only) + skillDir := filepath.Join(workspace, "skills", "test-skill") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + + skillFile := filepath.Join(skillDir, "SKILL.md") + content := `--- +name: test-skill +description: A test skill for unit testing +--- + +# Test Skill Content +This is the skill content. +` + require.NoError(t, os.WriteFile(skillFile, []byte(content), 0o644)) + + loader := NewSkillsLoader(workspace, globalSkills, builtinSkills) + skills := loader.ListSkills() + + assert.Len(t, skills, 1) + assert.Equal(t, "test-skill", skills[0].Name) + assert.Equal(t, "A test skill for unit testing", skills[0].Description) + assert.Equal(t, "workspace", skills[0].Source) +} + +func TestSkillsLoaderListSkillsGlobal(t *testing.T) { + workspace := t.TempDir() + globalSkills := t.TempDir() + builtinSkills := t.TempDir() + + // Create global skill + skillDir := filepath.Join(globalSkills, "global-skill") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + + skillFile := filepath.Join(skillDir, "SKILL.md") + content := `--- +name: global-skill +description: A global skill +--- + +# Global Skill +` + require.NoError(t, os.WriteFile(skillFile, []byte(content), 0o644)) + + loader := NewSkillsLoader(workspace, globalSkills, builtinSkills) + skills := loader.ListSkills() + + assert.Len(t, skills, 1) + assert.Equal(t, "global-skill", skills[0].Name) + assert.Equal(t, "global", skills[0].Source) +} + +func TestSkillsLoaderListSkillsBuiltin(t *testing.T) { + workspace := t.TempDir() + globalSkills := t.TempDir() + builtinSkills := t.TempDir() + + // Create builtin skill + skillDir := filepath.Join(builtinSkills, "builtin-skill") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + + skillFile := filepath.Join(skillDir, "SKILL.md") + content := `--- +name: builtin-skill +description: A builtin skill +--- + +# Builtin Skill +` + require.NoError(t, os.WriteFile(skillFile, []byte(content), 0o644)) + + loader := NewSkillsLoader(workspace, globalSkills, builtinSkills) + skills := loader.ListSkills() + + assert.Len(t, skills, 1) + assert.Equal(t, "builtin-skill", skills[0].Name) + assert.Equal(t, "builtin", skills[0].Source) +} + +func TestSkillsLoaderPriority(t *testing.T) { + workspace := t.TempDir() + globalSkills := t.TempDir() + builtinSkills := t.TempDir() + + // Create same skill in all three locations + createSkill := func(basePath, name string) { + skillDir := filepath.Join(basePath, "skills", name) + if basePath == globalSkills || basePath == builtinSkills { + skillDir = filepath.Join(basePath, name) + } + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + + skillFile := filepath.Join(skillDir, "SKILL.md") + content := `--- +name: ` + name + ` +description: ` + name + ` description +--- + +# ` + name + require.NoError(t, os.WriteFile(skillFile, []byte(content), 0o644)) + } + + createSkill(workspace, "override-skill") + createSkill(globalSkills, "override-skill") + createSkill(globalSkills, "global-only") + createSkill(builtinSkills, "override-skill") + createSkill(builtinSkills, "builtin-only") + + loader := NewSkillsLoader(workspace, globalSkills, builtinSkills) + skills := loader.ListSkills() + + // Should have 3 skills: override-skill (workspace), global-only, builtin-only + assert.Len(t, skills, 3) + + // Find override-skill, should be from workspace + var overrideSkill SkillInfo + for _, s := range skills { + if s.Name == "override-skill" { + overrideSkill = s + break + } + } + assert.Equal(t, "workspace", overrideSkill.Source) +} + +func TestSkillsLoaderLoadSkill(t *testing.T) { + workspace := t.TempDir() + globalSkills := t.TempDir() + builtinSkills := t.TempDir() + + // Create workspace skill + skillDir := filepath.Join(workspace, "skills", "loadable-skill") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + + skillFile := filepath.Join(skillDir, "SKILL.md") + content := `--- +name: loadable-skill +description: Can be loaded +--- + +# Skill Content +This is the actual skill content. +` + require.NoError(t, os.WriteFile(skillFile, []byte(content), 0o644)) + + loader := NewSkillsLoader(workspace, globalSkills, builtinSkills) + + // Load the skill + skillContent, ok := loader.LoadSkill("loadable-skill") + assert.True(t, ok) + assert.Contains(t, skillContent, "# Skill Content") + assert.NotContains(t, skillContent, "---") // frontmatter stripped +} + +func TestSkillsLoaderLoadSkillNotFound(t *testing.T) { + workspace := t.TempDir() + globalSkills := t.TempDir() + builtinSkills := t.TempDir() + + loader := NewSkillsLoader(workspace, globalSkills, builtinSkills) + + content, ok := loader.LoadSkill("nonexistent") + assert.False(t, ok) + assert.Empty(t, content) +} + +func TestSkillsLoaderBuildSkillsSummary(t *testing.T) { + workspace := t.TempDir() + globalSkills := t.TempDir() + builtinSkills := t.TempDir() + + // Create multiple skills + createSkill := func(basePath, name, desc string) { + skillDir := filepath.Join(basePath, "skills", name) + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + + skillFile := filepath.Join(skillDir, "SKILL.md") + content := `--- +name: ` + name + ` +description: ` + desc + ` +--- + +# ` + name + require.NoError(t, os.WriteFile(skillFile, []byte(content), 0o644)) + } + + createSkill(workspace, "skill1", "First skill") + createSkill(workspace, "skill2", "Second skill") + + loader := NewSkillsLoader(workspace, globalSkills, builtinSkills) + summary := loader.BuildSkillsSummary() + + assert.Contains(t, summary, "") + assert.Contains(t, summary, "") + assert.Contains(t, summary, "skill1") + assert.Contains(t, summary, "First skill") + assert.Contains(t, summary, "skill2") + assert.Contains(t, summary, "Second skill") +} + +func TestSkillsLoaderBuildSkillsSummaryEmpty(t *testing.T) { + workspace := t.TempDir() + globalSkills := t.TempDir() + builtinSkills := t.TempDir() + + loader := NewSkillsLoader(workspace, globalSkills, builtinSkills) + summary := loader.BuildSkillsSummary() + + assert.Empty(t, summary) +} + +func TestSkillsLoaderLoadSkillsForContext(t *testing.T) { + workspace := t.TempDir() + globalSkills := t.TempDir() + builtinSkills := t.TempDir() + + // Create skills + createSkill := func(basePath, name, content string) { + skillDir := filepath.Join(basePath, "skills", name) + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + + skillFile := filepath.Join(skillDir, "SKILL.md") + fullContent := `--- +name: ` + name + ` +description: Desc +--- + +` + content + require.NoError(t, os.WriteFile(skillFile, []byte(fullContent), 0o644)) + } + + createSkill(workspace, "ctx-skill1", "# Content 1") + createSkill(workspace, "ctx-skill2", "# Content 2") + + loader := NewSkillsLoader(workspace, globalSkills, builtinSkills) + + context := loader.LoadSkillsForContext([]string{"ctx-skill1", "ctx-skill2"}) + assert.Contains(t, context, "### Skill: ctx-skill1") + assert.Contains(t, context, "# Content 1") + assert.Contains(t, context, "### Skill: ctx-skill2") + assert.Contains(t, context, "# Content 2") +} + +func TestSkillsLoaderLoadSkillsForContextEmpty(t *testing.T) { + workspace := t.TempDir() + globalSkills := t.TempDir() + builtinSkills := t.TempDir() + + loader := NewSkillsLoader(workspace, globalSkills, builtinSkills) + context := loader.LoadSkillsForContext([]string{}) + assert.Empty(t, context) +} + +func TestSkillsLoaderValidateSkill(t *testing.T) { + tests := []struct { + name string + info SkillInfo + wantErr bool }{ { - name: "valid-skill", - skillName: "valid-skill", - description: "a valid skill description", - wantErr: false, + name: "valid", + info: SkillInfo{ + Name: "valid-skill", + Description: "A valid skill", + }, + wantErr: false, }, { - name: "empty-name", - skillName: "", - description: "description without name", - wantErr: true, - errContains: []string{"name is required"}, + name: "missing name", + info: SkillInfo{ + Description: "Missing name", + }, + wantErr: true, }, { - name: "empty-description", - skillName: "skill-without-description", - description: "", - wantErr: true, - errContains: []string{"description is required"}, + name: "missing description", + info: SkillInfo{ + Name: "no-desc", + }, + wantErr: true, }, { - name: "empty-both", - skillName: "", - description: "", - wantErr: true, - errContains: []string{"name is required", "description is required"}, + name: "invalid name format", + info: SkillInfo{ + Name: "invalid_name", + Description: "Has underscore", + }, + wantErr: true, }, { - name: "name-with-spaces", - skillName: "skill with spaces", - description: "invalid name with spaces", - wantErr: true, - errContains: []string{"name must be alphanumeric with hyphens"}, - }, - { - name: "name-with-underscore", - skillName: "skill_underscore", - description: "invalid name with underscore", - wantErr: true, - errContains: []string{"name must be alphanumeric with hyphens"}, + name: "name too long", + info: SkillInfo{ + Name: string(make([]byte, 100)), + Description: "Too long name", + }, + wantErr: true, }, } - for _, tc := range testcases { - t.Run(tc.name, func(t *testing.T) { - info := SkillInfo{ - Name: tc.skillName, - Description: tc.description, - } - err := info.validate() - if tc.wantErr { + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.info.validate() + if tt.wantErr { assert.Error(t, err) - for _, msg := range tc.errContains { - assert.ErrorContains(t, err, msg) - } } else { assert.NoError(t, err) } @@ -76,122 +334,161 @@ func TestSkillsInfoValidate(t *testing.T) { } } -func TestExtractFrontmatter(t *testing.T) { - sl := &SkillsLoader{} +func TestSkillsLoaderExtractFrontmatter(t *testing.T) { + workspace := t.TempDir() + loader := NewSkillsLoader(workspace, "", "") - testcases := []struct { - name string - content string - expectedName string - expectedDesc string - lineEndingType string + tests := []struct { + name string + content string + expected string }{ { - name: "unix-line-endings", - lineEndingType: "Unix (\\n)", - content: "---\nname: test-skill\ndescription: A test skill\n---\n\n# Skill Content", - expectedName: "test-skill", - expectedDesc: "A test skill", + name: "with frontmatter", + content: `--- +name: Test +description: Desc +--- + +Content`, + expected: "name: Test\ndescription: Desc", }, { - name: "windows-line-endings", - lineEndingType: "Windows (\\r\\n)", - content: "---\r\nname: test-skill\r\ndescription: A test skill\r\n---\r\n\r\n# Skill Content", - expectedName: "test-skill", - expectedDesc: "A test skill", + name: "without frontmatter", + content: `# Just content +No frontmatter here`, + expected: "", }, { - name: "classic-mac-line-endings", - lineEndingType: "Classic Mac (\\r)", - content: "---\rname: test-skill\rdescription: A test skill\r---\r\r# Skill Content", - expectedName: "test-skill", - expectedDesc: "A test skill", + name: "windows line endings", + content: "---\r\nname: Test\r\ndescription: Desc\r\n---\r\n\r\nContent", + expected: "name: Test\r\ndescription: Desc", }, } - for _, tc := range testcases { - t.Run(tc.name, func(t *testing.T) { - // Extract frontmatter - frontmatter := sl.extractFrontmatter(tc.content) - assert.NotEmpty(t, frontmatter, "Frontmatter should be extracted for %s line endings", tc.lineEndingType) - - // Parse YAML to get name and description (parseSimpleYAML now handles all line ending types) - yamlMeta := sl.parseSimpleYAML(frontmatter) - assert.Equal( - t, - tc.expectedName, - yamlMeta["name"], - "Name should be correctly parsed from frontmatter with %s line endings", - tc.lineEndingType, - ) - assert.Equal( - t, - tc.expectedDesc, - yamlMeta["description"], - "Description should be correctly parsed from frontmatter with %s line endings", - tc.lineEndingType, - ) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := loader.extractFrontmatter(tt.content) + assert.Equal(t, tt.expected, result) }) } } -func TestStripFrontmatter(t *testing.T) { - sl := &SkillsLoader{} +func TestSkillsLoaderStripFrontmatter(t *testing.T) { + workspace := t.TempDir() + loader := NewSkillsLoader(workspace, "", "") - testcases := []struct { - name string - content string - expectedContent string - lineEndingType string + content := `--- +name: Test +description: Desc +--- + +# Actual Content +This should remain.` + + stripped := loader.stripFrontmatter(content) + assert.Contains(t, stripped, "# Actual Content") + assert.NotContains(t, stripped, "---") + assert.NotContains(t, stripped, "name: Test") +} + +func TestSkillsLoaderParseSimpleYAML(t *testing.T) { + workspace := t.TempDir() + loader := NewSkillsLoader(workspace, "", "") + + tests := []struct { + name string + content string + expected map[string]string }{ { - name: "unix-line-endings", - lineEndingType: "Unix (\\n)", - content: "---\nname: test-skill\ndescription: A test skill\n---\n\n# Skill Content", - expectedContent: "# Skill Content", + name: "simple key value", + content: `name: Test +description: A test skill`, + expected: map[string]string{ + "name": "Test", + "description": "A test skill", + }, }, { - name: "windows-line-endings", - lineEndingType: "Windows (\\r\\n)", - content: "---\r\nname: test-skill\r\ndescription: A test skill\r\n---\r\n\r\n# Skill Content", - expectedContent: "# Skill Content", + name: "with quotes", + content: `name: "Quoted Name" +description: 'Single quoted'`, + expected: map[string]string{ + "name": "Quoted Name", + "description": "Single quoted", + }, }, { - name: "classic-mac-line-endings", - lineEndingType: "Classic Mac (\\r)", - content: "---\rname: test-skill\rdescription: A test skill\r---\r\r# Skill Content", - expectedContent: "# Skill Content", + name: "with comments", + content: `# This is a comment +name: Test +# Another comment +description: Test skill`, + expected: map[string]string{ + "name": "Test", + "description": "Test skill", + }, }, { - name: "unix-line-endings-without-trailing-newline", - lineEndingType: "Unix (\\n) without trailing newline", - content: "---\nname: test-skill\ndescription: A test skill\n---\n# Skill Content", - expectedContent: "# Skill Content", - }, - { - name: "windows-line-endings-without-trailing-newline", - lineEndingType: "Windows (\\r\\n) without trailing newline", - content: "---\r\nname: test-skill\r\ndescription: A test skill\r\n---\r\n# Skill Content", - expectedContent: "# Skill Content", - }, - { - name: "no-frontmatter", - lineEndingType: "No frontmatter", - content: "# Skill Content\n\nSome content here.", - expectedContent: "# Skill Content\n\nSome content here.", + name: "windows line endings", + content: "name: Test\r\ndescription: Windows", + expected: map[string]string{ + "name": "Test", + "description": "Windows", + }, }, } - for _, tc := range testcases { - t.Run(tc.name, func(t *testing.T) { - result := sl.stripFrontmatter(tc.content) - assert.Equal( - t, - tc.expectedContent, - result, - "Frontmatter should be stripped correctly for %s", - tc.lineEndingType, - ) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := loader.parseSimpleYAML(tt.content) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestSkillsLoaderGetSkillMetadata(t *testing.T) { + workspace := t.TempDir() + + // Create skill file + skillDir := filepath.Join(workspace, "skills", "meta-test") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + + skillFile := filepath.Join(skillDir, "SKILL.md") + content := `--- +name: Meta Test Skill +description: Testing metadata extraction +--- + +# Content` + + require.NoError(t, os.WriteFile(skillFile, []byte(content), 0o644)) + + loader := NewSkillsLoader(workspace, "", "") + metadata := loader.getSkillMetadata(skillFile) + + assert.NotNil(t, metadata) + assert.Equal(t, "Meta Test Skill", metadata.Name) + assert.Equal(t, "Testing metadata extraction", metadata.Description) +} + +func TestEscapeXML(t *testing.T) { + tests := []struct { + input string + expected string + }{ + {"normal text", "normal text"}, + {"text & more", "text & more"}, + {"text < tag", "text < tag"}, + {"text > tag", "text > tag"}, + {"all & < >", "all & < >"}, + } + + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + result := escapeXML(tt.input) + assert.Equal(t, tt.expected, result) }) } } diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go index 60f2b7b91..6d35815e8 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -191,15 +191,15 @@ func TestShellTool_WorkingDir_OutsideWorkspace(t *testing.T) { root := t.TempDir() workspace := filepath.Join(root, "workspace") outsideDir := filepath.Join(root, "outside") - if err := os.MkdirAll(workspace, 0755); err != nil { + if err := os.MkdirAll(workspace, 0o755); err != nil { t.Fatalf("failed to create workspace: %v", err) } - if err := os.MkdirAll(outsideDir, 0755); err != nil { + if err := os.MkdirAll(outsideDir, 0o755); err != nil { t.Fatalf("failed to create outside dir: %v", err) } tool := NewExecTool(workspace, true) - result := tool.Execute(context.Background(), map[string]interface{}{ + result := tool.Execute(context.Background(), map[string]any{ "command": "pwd", "working_dir": outsideDir, }) @@ -218,13 +218,13 @@ func TestShellTool_WorkingDir_SymlinkEscape(t *testing.T) { root := t.TempDir() workspace := filepath.Join(root, "workspace") secretDir := filepath.Join(root, "secret") - if err := os.MkdirAll(workspace, 0755); err != nil { + if err := os.MkdirAll(workspace, 0o755); err != nil { t.Fatalf("failed to create workspace: %v", err) } - if err := os.MkdirAll(secretDir, 0755); err != nil { + if err := os.MkdirAll(secretDir, 0o755); err != nil { t.Fatalf("failed to create secret dir: %v", err) } - os.WriteFile(filepath.Join(secretDir, "secret.txt"), []byte("top secret"), 0644) + os.WriteFile(filepath.Join(secretDir, "secret.txt"), []byte("top secret"), 0o644) // symlink lives inside the workspace but resolves to secretDir outside it link := filepath.Join(workspace, "escape") @@ -233,7 +233,7 @@ func TestShellTool_WorkingDir_SymlinkEscape(t *testing.T) { } tool := NewExecTool(workspace, true) - result := tool.Execute(context.Background(), map[string]interface{}{ + result := tool.Execute(context.Background(), map[string]any{ "command": "cat secret.txt", "working_dir": link, }) diff --git a/pkg/tools/skills_install_test.go b/pkg/tools/skills_install_test.go index 676fcecc0..de12d56e0 100644 --- a/pkg/tools/skills_install_test.go +++ b/pkg/tools/skills_install_test.go @@ -12,93 +12,138 @@ import ( "github.com/sipeed/picoclaw/pkg/skills" ) -func TestInstallSkillToolName(t *testing.T) { - tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir()) - assert.Equal(t, "install_skill", tool.Name()) -} - -func TestInstallSkillToolMissingSlug(t *testing.T) { - tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir()) - result := tool.Execute(context.Background(), map[string]any{}) - assert.True(t, result.IsError) - assert.Contains(t, result.ForLLM, "identifier is required and must be a non-empty string") -} - -func TestInstallSkillToolEmptySlug(t *testing.T) { - tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir()) - result := tool.Execute(context.Background(), map[string]any{ - "slug": " ", - }) - assert.True(t, result.IsError) - assert.Contains(t, result.ForLLM, "identifier is required and must be a non-empty string") -} - -func TestInstallSkillToolUnsafeSlug(t *testing.T) { - tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir()) - - cases := []string{ - "../etc/passwd", - "path/traversal", - "path\\traversal", - } - - for _, slug := range cases { - result := tool.Execute(context.Background(), map[string]any{ - "slug": slug, - }) - assert.True(t, result.IsError, "slug %q should be rejected", slug) - assert.Contains(t, result.ForLLM, "invalid slug") - } -} - -func TestInstallSkillToolAlreadyExists(t *testing.T) { +// TestInstallSkillToolForceReinstall tests the force reinstall functionality +func TestInstallSkillToolForceReinstall(t *testing.T) { workspace := t.TempDir() - skillDir := filepath.Join(workspace, "skills", "existing-skill") + skillDir := filepath.Join(workspace, "skills", "test-skill") require.NoError(t, os.MkdirAll(skillDir, 0o755)) + // Create a dummy file to simulate existing installation + dummyFile := filepath.Join(skillDir, "SKILL.md") + require.NoError(t, os.WriteFile(dummyFile, []byte("# Old Skill"), 0o644)) + tool := NewInstallSkillTool(skills.NewRegistryManager(), workspace) + + // Without force=true, should fail result := tool.Execute(context.Background(), map[string]any{ - "slug": "existing-skill", + "slug": "test-skill", "registry": "clawhub", }) assert.True(t, result.IsError) assert.Contains(t, result.ForLLM, "already installed") + + // With force=true, should proceed (but fail due to registry not found) + result = tool.Execute(context.Background(), map[string]any{ + "slug": "test-skill", + "registry": "clawhub", + "force": true, + }) + // Should not error about "already installed" anymore + if result.IsError { + assert.NotContains(t, result.ForLLM, "already installed") + } } -func TestInstallSkillToolRegistryNotFound(t *testing.T) { +// TestInstallSkillToolWriteOriginMeta tests that origin metadata is written +func TestInstallSkillToolWriteOriginMeta(t *testing.T) { workspace := t.TempDir() - tool := NewInstallSkillTool(skills.NewRegistryManager(), workspace) + + // Create a mock registry that will succeed + registryMgr := skills.NewRegistryManager() + + // We can't test actual installation without network, but we can test + // the directory preparation and metadata writing logic by checking + // if the skill directory structure is correct after a failed install + + tool := NewInstallSkillTool(registryMgr, workspace) result := tool.Execute(context.Background(), map[string]any{ - "slug": "some-skill", + "slug": "mock-skill", "registry": "nonexistent", }) + + // Should fail because registry doesn't exist assert.True(t, result.IsError) - assert.Contains(t, result.ForLLM, "registry") assert.Contains(t, result.ForLLM, "not found") } -func TestInstallSkillToolParameters(t *testing.T) { - tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir()) - params := tool.Parameters() +// TestInstallSkillToolInvalidSlugPatterns tests various invalid slug patterns +func TestInstallSkillToolInvalidSlugPatterns(t *testing.T) { + workspace := t.TempDir() + tool := NewInstallSkillTool(skills.NewRegistryManager(), workspace) - props, ok := params["properties"].(map[string]any) - assert.True(t, ok) - assert.Contains(t, props, "slug") - assert.Contains(t, props, "version") - assert.Contains(t, props, "registry") - assert.Contains(t, props, "force") + invalidSlugs := []struct { + slug string + reason string + }{ + {"../etc/passwd", "path traversal"}, + {"skill/with/slash", "contains slash"}, + {"skill\\with\\backslash", "contains backslash"}, + {"./relative/path", "relative path"}, + {"", "empty slug"}, + {" ", "whitespace only"}, + } - required, ok := params["required"].([]string) - assert.True(t, ok) - assert.Contains(t, required, "slug") - assert.Contains(t, required, "registry") + for _, tc := range invalidSlugs { + t.Run(tc.reason, func(t *testing.T) { + result := tool.Execute(context.Background(), map[string]any{ + "slug": tc.slug, + "registry": "clawhub", + }) + assert.True(t, result.IsError, "slug %q should be rejected: %s", tc.slug, tc.reason) + assert.Contains(t, result.ForLLM, "invalid slug") + }) + } } -func TestInstallSkillToolMissingRegistry(t *testing.T) { +// TestInstallSkillToolValidSlugPatterns tests valid slug patterns +func TestInstallSkillToolValidSlugPatterns(t *testing.T) { + workspace := t.TempDir() + tool := NewInstallSkillTool(skills.NewRegistryManager(), workspace) + + validSlugs := []string{ + "github", + "docker-compose", + "my-skill-123", + "skill_with_underscore", + } + + for _, slug := range validSlugs { + t.Run(slug, func(t *testing.T) { + result := tool.Execute(context.Background(), map[string]any{ + "slug": slug, + "registry": "nonexistent", // Will fail, but slug validation should pass + }) + // Should fail because registry doesn't exist, not because of slug validation + assert.True(t, result.IsError) + assert.Contains(t, result.ForLLM, "not found") + assert.NotContains(t, result.ForLLM, "invalid slug") + }) + } +} + +// TestInstallSkillToolDescription tests the tool description +func TestInstallSkillToolDescription(t *testing.T) { tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir()) - result := tool.Execute(context.Background(), map[string]any{ - "slug": "some-skill", + desc := tool.Description() + assert.NotEmpty(t, desc) + assert.Contains(t, desc, "Install") + assert.Contains(t, desc, "skill") + assert.Contains(t, desc, "registry") +} + +// TestInstallSkillToolExecuteContextCancellation tests behavior with context cancellation +func TestInstallSkillToolExecuteContextCancellation(t *testing.T) { + workspace := t.TempDir() + tool := NewInstallSkillTool(skills.NewRegistryManager(), workspace) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // Cancel immediately + + result := tool.Execute(ctx, map[string]any{ + "slug": "test-skill", + "registry": "clawhub", }) + + // Should still validate parameters even with canceled context assert.True(t, result.IsError) - assert.Contains(t, result.ForLLM, "invalid registry") }