feat:add test

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

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
 }

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) {
+func TestSkillsLoaderListSkillsEmpty(t *testing.T) {
-	testcases := []struct {
+	workspace := t.TempDir()
-		name        string
+	globalSkills := t.TempDir()
-		skillName   string
+	builtinSkills := t.TempDir()
-		description string
+
-		wantErr     bool
+	loader := NewSkillsLoader(workspace, globalSkills, builtinSkills)
-		errContains []string
+	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, "<skills>")
+	assert.Contains(t, summary, "</skills>")
+	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",
+			name: "valid",
-			skillName:   "valid-skill",
+			info: SkillInfo{
-			description: "a valid skill description",
+				Name:        "valid-skill",
-			wantErr:     false,
+				Description: "A valid skill",
+			},
+			wantErr: false,
 		},
 		{
-			name:        "empty-name",
+			name: "missing name",
-			skillName:   "",
+			info: SkillInfo{
-			description: "description without name",
+				Description: "Missing name",
-			wantErr:     true,
+			},
-			errContains: []string{"name is required"},
+			wantErr: true,
 		},
 		{
-			name:        "empty-description",
+			name: "missing description",
-			skillName:   "skill-without-description",
+			info: SkillInfo{
-			description: "",
+				Name: "no-desc",
-			wantErr:     true,
+			},
-			errContains: []string{"description is required"},
+			wantErr: true,
 		},
 		{
-			name:        "empty-both",
+			name: "invalid name format",
-			skillName:   "",
+			info: SkillInfo{
-			description: "",
+				Name:        "invalid_name",
-			wantErr:     true,
+				Description: "Has underscore",
-			errContains: []string{"name is required", "description is required"},
+			},
+			wantErr: true,
 		},
 		{
-			name:        "name-with-spaces",
+			name: "name too long",
-			skillName:   "skill with spaces",
+			info: SkillInfo{
-			description: "invalid name with spaces",
+				Name:        string(make([]byte, 100)),
-			wantErr:     true,
+				Description: "Too long name",
-			errContains: []string{"name must be alphanumeric with hyphens"},
+			},
-		},
+			wantErr: true,
-		{
-			name:        "name-with-underscore",
-			skillName:   "skill_underscore",
-			description: "invalid name with underscore",
-			wantErr:     true,
-			errContains: []string{"name must be alphanumeric with hyphens"},
 		},
 	}
 
-	for _, tc := range testcases {
+	for _, tt := range tests {
-		t.Run(tc.name, func(t *testing.T) {
+		t.Run(tt.name, func(t *testing.T) {
-			info := SkillInfo{
+			err := tt.info.validate()
-				Name:        tc.skillName,
+			if tt.wantErr {
-				Description: tc.description,
-			}
-			err := info.validate()
-			if tc.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) {
+func TestSkillsLoaderExtractFrontmatter(t *testing.T) {
-	sl := &SkillsLoader{}
+	workspace := t.TempDir()
+	loader := NewSkillsLoader(workspace, "", "")
 
-	testcases := []struct {
+	tests := []struct {
-		name           string
+		name     string
-		content        string
+		content  string
-		expectedName   string
+		expected string
-		expectedDesc   string
-		lineEndingType string
 	}{
 		{
-			name:           "unix-line-endings",
+			name: "with frontmatter",
-			lineEndingType: "Unix (\\n)",
+			content: `---
-			content:        "---\nname: test-skill\ndescription: A test skill\n---\n\n# Skill Content",
+name: Test
-			expectedName:   "test-skill",
+description: Desc
-			expectedDesc:   "A test skill",
+---
+
+Content`,
+			expected: "name: Test\ndescription: Desc",
 		},
 		{
-			name:           "windows-line-endings",
+			name: "without frontmatter",
-			lineEndingType: "Windows (\\r\\n)",
+			content: `# Just content
-			content:        "---\r\nname: test-skill\r\ndescription: A test skill\r\n---\r\n\r\n# Skill Content",
+No frontmatter here`,
-			expectedName:   "test-skill",
+			expected: "",
-			expectedDesc:   "A test skill",
 		},
 		{
-			name:           "classic-mac-line-endings",
+			name:     "windows line endings",
-			lineEndingType: "Classic Mac (\\r)",
+			content:  "---\r\nname: Test\r\ndescription: Desc\r\n---\r\n\r\nContent",
-			content:        "---\rname: test-skill\rdescription: A test skill\r---\r\r# Skill Content",
+			expected: "name: Test\r\ndescription: Desc",
-			expectedName:   "test-skill",
-			expectedDesc:   "A test skill",
 		},
 	}
 
-	for _, tc := range testcases {
+	for _, tt := range tests {
-		t.Run(tc.name, func(t *testing.T) {
+		t.Run(tt.name, func(t *testing.T) {
-			// Extract frontmatter
+			result := loader.extractFrontmatter(tt.content)
-			frontmatter := sl.extractFrontmatter(tc.content)
+			assert.Equal(t, tt.expected, result)
-			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,
-			)
 		})
 	}
 }
 
-func TestStripFrontmatter(t *testing.T) {
+func TestSkillsLoaderStripFrontmatter(t *testing.T) {
-	sl := &SkillsLoader{}
+	workspace := t.TempDir()
+	loader := NewSkillsLoader(workspace, "", "")
 
-	testcases := []struct {
+	content := `---
-		name            string
+name: Test
-		content         string
+description: Desc
-		expectedContent string
+---
-		lineEndingType  string
+
+# 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",
+			name: "simple key value",
-			lineEndingType:  "Unix (\\n)",
+			content: `name: Test
-			content:         "---\nname: test-skill\ndescription: A test skill\n---\n\n# Skill Content",
+description: A test skill`,
-			expectedContent: "# Skill Content",
+			expected: map[string]string{
+				"name":        "Test",
+				"description": "A test skill",
+			},
 		},
 		{
-			name:            "windows-line-endings",
+			name: "with quotes",
-			lineEndingType:  "Windows (\\r\\n)",
+			content: `name: "Quoted Name"
-			content:         "---\r\nname: test-skill\r\ndescription: A test skill\r\n---\r\n\r\n# Skill Content",
+description: 'Single quoted'`,
-			expectedContent: "# Skill Content",
+			expected: map[string]string{
+				"name":        "Quoted Name",
+				"description": "Single quoted",
+			},
 		},
 		{
-			name:            "classic-mac-line-endings",
+			name: "with comments",
-			lineEndingType:  "Classic Mac (\\r)",
+			content: `# This is a comment
-			content:         "---\rname: test-skill\rdescription: A test skill\r---\r\r# Skill Content",
+name: Test
-			expectedContent: "# Skill Content",
+# Another comment
+description: Test skill`,
+			expected: map[string]string{
+				"name":        "Test",
+				"description": "Test skill",
+			},
 		},
 		{
-			name:            "unix-line-endings-without-trailing-newline",
+			name:    "windows line endings",
-			lineEndingType:  "Unix (\\n) without trailing newline",
+			content: "name: Test\r\ndescription: Windows",
-			content:         "---\nname: test-skill\ndescription: A test skill\n---\n# Skill Content",
+			expected: map[string]string{
-			expectedContent: "# Skill Content",
+				"name":        "Test",
-		},
+				"description": "Windows",
-		{
+			},
-			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.",
 		},
 	}
 
-	for _, tc := range testcases {
+	for _, tt := range tests {
-		t.Run(tc.name, func(t *testing.T) {
+		t.Run(tt.name, func(t *testing.T) {
-			result := sl.stripFrontmatter(tc.content)
+			result := loader.parseSimpleYAML(tt.content)
-			assert.Equal(
+			assert.Equal(t, tt.expected, result)
-				t,
+		})
-				tc.expectedContent,
+	}
-				result,
+}
-				"Frontmatter should be stripped correctly for %s",
+
-				tc.lineEndingType,
+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 &amp; more"},
+		{"text < tag", "text &lt; tag"},
+		{"text > tag", "text &gt; tag"},
+		{"all & < >", "all &amp; &lt; &gt;"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.input, func(t *testing.T) {
+			result := escapeXML(tt.input)
+			assert.Equal(t, tt.expected, result)
 		})
 	}
 }

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,
 	})

pkg/tools/skills_install_test.go

@@ -12,93 +12,138 @@ import (
 	"github.com/sipeed/picoclaw/pkg/skills"
 )
 
-func TestInstallSkillToolName(t *testing.T) {
+// TestInstallSkillToolForceReinstall tests the force reinstall functionality
-	tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir())
+func TestInstallSkillToolForceReinstall(t *testing.T) {
-	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) {
 	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) {
+// TestInstallSkillToolInvalidSlugPatterns tests various invalid slug patterns
-	tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir())
+func TestInstallSkillToolInvalidSlugPatterns(t *testing.T) {
-	params := tool.Parameters()
+	workspace := t.TempDir()
+	tool := NewInstallSkillTool(skills.NewRegistryManager(), workspace)
 
-	props, ok := params["properties"].(map[string]any)
+	invalidSlugs := []struct {
-	assert.True(t, ok)
+		slug   string
-	assert.Contains(t, props, "slug")
+		reason string
-	assert.Contains(t, props, "version")
+	}{
-	assert.Contains(t, props, "registry")
+		{"../etc/passwd", "path traversal"},
-	assert.Contains(t, props, "force")
+		{"skill/with/slash", "contains slash"},
+		{"skill\\with\\backslash", "contains backslash"},
+		{"./relative/path", "relative path"},
+		{"", "empty slug"},
+		{"   ", "whitespace only"},
+	}
 
-	required, ok := params["required"].([]string)
+	for _, tc := range invalidSlugs {
-	assert.True(t, ok)
+		t.Run(tc.reason, func(t *testing.T) {
-	assert.Contains(t, required, "slug")
+			result := tool.Execute(context.Background(), map[string]any{
-	assert.Contains(t, required, "registry")
+				"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{
+	desc := tool.Description()
-		"slug": "some-skill",
+	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")
 }