diff --git a/pkg/tools/skills_install.go b/pkg/tools/skills_install.go index c7db02eb3..19a34d1cc 100644 --- a/pkg/tools/skills_install.go +++ b/pkg/tools/skills_install.go @@ -156,6 +156,19 @@ func (t *InstallSkillTool) Execute(ctx context.Context, args map[string]any) *To return ErrorResult(fmt.Sprintf("skill %q is flagged as malicious and cannot be installed", slug)) } + if !workspaceHasValidInstalledSkill(t.workspace, dirName) { + rmErr := os.RemoveAll(targetDir) + if rmErr != nil { + logger.ErrorCF("tool", "Failed to remove invalid installed skill", + map[string]any{ + "tool": "install_skill", + "target_dir": targetDir, + "error": rmErr.Error(), + }) + } + return ErrorResult(fmt.Sprintf("failed to install %q: registry archive is not a valid skill", slug)) + } + // Write origin metadata. if err := writeOriginMeta(targetDir, registry, slug, result.Version); err != nil { logger.ErrorCF("tool", "Failed to write origin metadata", @@ -224,3 +237,16 @@ func writeOriginMeta(targetDir string, registry skills.SkillRegistry, slug, vers // Use unified atomic write utility with explicit sync for flash storage reliability. return fileutil.WriteFileAtomic(filepath.Join(targetDir, ".skill-origin.json"), data, 0o600) } + +func workspaceHasValidInstalledSkill(workspace, directory string) bool { + loader := skills.NewSkillsLoader(workspace, "", "") + for _, skill := range loader.ListSkills() { + if skill.Source != "workspace" { + continue + } + if filepath.Base(filepath.Dir(skill.Path)) == directory { + return true + } + } + return false +} diff --git a/pkg/tools/skills_install_test.go b/pkg/tools/skills_install_test.go index 9ecf34ea3..7e051368d 100644 --- a/pkg/tools/skills_install_test.go +++ b/pkg/tools/skills_install_test.go @@ -15,6 +15,8 @@ import ( type mockInstallRegistry struct{} +const validSkillMarkdown = "---\nname: pr-review\ndescription: Review pull requests\n---\n# PR Review\n" + func (m *mockInstallRegistry) Name() string { return "clawhub" } func (m *mockInstallRegistry) ResolveInstallDirName(target string) (string, error) { @@ -32,11 +34,17 @@ func (m *mockInstallRegistry) GetSkillMeta(context.Context, string) (*skills.Ski } func (m *mockInstallRegistry) DownloadAndInstall( - context.Context, - string, - string, - string, + _ context.Context, + _ string, + _ string, + targetDir string, ) (*skills.InstallResult, error) { + if err := os.MkdirAll(targetDir, 0o755); err != nil { + return nil, err + } + if err := os.WriteFile(filepath.Join(targetDir, "SKILL.md"), []byte(validSkillMarkdown), 0o600); err != nil { + return nil, err + } return &skills.InstallResult{Version: "test"}, nil } @@ -59,11 +67,17 @@ func (m *mockGitHubInstallRegistry) GetSkillMeta(context.Context, string) (*skil } func (m *mockGitHubInstallRegistry) DownloadAndInstall( - context.Context, - string, - string, - string, + _ context.Context, + _ string, + _ string, + targetDir string, ) (*skills.InstallResult, error) { + if err := os.MkdirAll(targetDir, 0o755); err != nil { + return nil, err + } + if err := os.WriteFile(filepath.Join(targetDir, "SKILL.md"), []byte(validSkillMarkdown), 0o600); err != nil { + return nil, err + } return &skills.InstallResult{Version: "main"}, nil } @@ -72,14 +86,57 @@ type stubGitHubInstallRegistry struct { } func (m *stubGitHubInstallRegistry) DownloadAndInstall( - context.Context, - string, - string, - string, + _ context.Context, + _ string, + _ string, + targetDir string, ) (*skills.InstallResult, error) { + if err := os.MkdirAll(targetDir, 0o755); err != nil { + return nil, err + } + if err := os.WriteFile(filepath.Join(targetDir, "SKILL.md"), []byte(validSkillMarkdown), 0o600); err != nil { + return nil, err + } return &skills.InstallResult{Version: "main"}, nil } +type mockInvalidInstallRegistry struct{} + +func (m *mockInvalidInstallRegistry) Name() string { return "clawhub" } + +func (m *mockInvalidInstallRegistry) ResolveInstallDirName(target string) (string, error) { + return target, nil +} + +func (m *mockInvalidInstallRegistry) SkillURL(slug, _ string) string { return slug } + +func (m *mockInvalidInstallRegistry) Search(context.Context, string, int) ([]skills.SearchResult, error) { + return nil, nil +} + +func (m *mockInvalidInstallRegistry) GetSkillMeta(context.Context, string) (*skills.SkillMeta, error) { + return nil, nil +} + +func (m *mockInvalidInstallRegistry) DownloadAndInstall( + _ context.Context, + _ string, + _ string, + targetDir string, +) (*skills.InstallResult, error) { + if err := os.MkdirAll(targetDir, 0o755); err != nil { + return nil, err + } + if err := os.WriteFile( + filepath.Join(targetDir, "SKILL.md"), + []byte("---\nname: bad_skill\ndescription: invalid name\n---\n# Invalid\n"), + 0o600, + ); err != nil { + return nil, err + } + return &skills.InstallResult{Version: "test"}, nil +} + func TestInstallSkillToolName(t *testing.T) { tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir()) assert.Equal(t, "install_skill", tool.Name()) @@ -209,3 +266,20 @@ func TestInstallSkillToolAllowsGitHubURLSlug(t *testing.T) { assert.Equal(t, "main", meta.InstalledVersion) assert.NotZero(t, meta.InstalledAt) } + +func TestInstallSkillToolRejectsInvalidInstalledSkill(t *testing.T) { + workspace := t.TempDir() + registryMgr := skills.NewRegistryManager() + registryMgr.AddRegistry(&mockInvalidInstallRegistry{}) + tool := NewInstallSkillTool(registryMgr, workspace) + + result := tool.Execute(context.Background(), map[string]any{ + "slug": "broken-skill", + "registry": "clawhub", + }) + + assert.True(t, result.IsError) + assert.Contains(t, result.ForLLM, "not a valid skill") + _, err := os.Stat(filepath.Join(workspace, "skills", "broken-skill")) + assert.True(t, os.IsNotExist(err)) +}