refactor skills registries and add GitHub-backed skill discovery (#2442)

* refactor skills registries and add GitHub-backed skill discovery

* fix ci

* fix command error

* fix default skills install registry behavior

* fix github registry URL parsing and versioned skill links

* fix skills registry config compatibility and URL installs

* * fix lint

* fix deprecated github base url compatibility

* fix skills registry yaml and github default branch handling

* fix github skills registry fallback and install metadata

* fix cli skills install origin metadata

* fix clawhub registry env compatibility

* fix skills registry config merge compatibility

* fix skill install metadata consistency and onboard template copy

* fix yaml overrides for default skills registries

* fix install_skill registry metadata normalization

* fix github skill URL parsing for slash branch names

* fix skills registry install/search validation and github URLs

* fix github skill URL host validation

* fix install_skill validation for invalid registry archives

* fix redundant skills registry names in saved config

* fix github blob skill URL installs and metadata links

* fix github registry URL scheme validation

* fix v0 skills migration preserving github registry defaults

* fix github blob skill install directory resolution

* fix install_skill rollback on origin metadata write failure

* fix github skill URL validation and registry JSON merging

* fix github registry target resolution and metadata links

* fix install_skill force reinstall rollback

* fix skills config compatibility and legacy security overlays

* fix ci

pkg/tools/skills_install.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 	"sync"
 	"time"
 
@@ -15,6 +16,10 @@ import (
 	"github.com/sipeed/picoclaw/pkg/utils"
 )
 
+const defaultSkillRegistryName = "github"
+
+var persistInstalledSkillOriginMeta = writeOriginMeta
+
 // InstallSkillTool allows the LLM agent to install skills from registries.
 // It shares the same RegistryManager that FindSkillsTool uses,
 // so all registries configured in config are available for installation.
@@ -40,7 +45,7 @@ func (t *InstallSkillTool) Name() string {
 }
 
 func (t *InstallSkillTool) Description() string {
-	return "Install a skill from a registry by slug. Downloads and extracts the skill into the workspace. Use find_skills first to discover available skills."
+	return "Install a skill from a registry by slug. Defaults to GitHub when registry is omitted. Downloads and extracts the skill into the workspace. Use find_skills first to discover available skills."
 }
 
 func (t *InstallSkillTool) Parameters() map[string]any {
@@ -57,14 +62,14 @@ func (t *InstallSkillTool) Parameters() map[string]any {
 			},
 			"registry": map[string]any{
 				"type":        "string",
-				"description": "Registry to install from (required, e.g., 'clawhub')",
+				"description": "Registry to install from (optional, defaults to 'github')",
 			},
 			"force": map[string]any{
 				"type":        "boolean",
 				"description": "Force reinstall if skill already exists (default false)",
 			},
 		},
-		"required": []string{"slug", "registry"},
+		"required": []string{"slug"},
 	}
 }
 
@@ -74,45 +79,86 @@ func (t *InstallSkillTool) Execute(ctx context.Context, args map[string]any) *To
 	t.mu.Lock()
 	defer t.mu.Unlock()
 
-	// Validate slug
 	slug, _ := args["slug"].(string)
-	if err := utils.ValidateSkillIdentifier(slug); err != nil {
-		return ErrorResult(fmt.Sprintf("invalid slug %q: error: %s", slug, err.Error()))
+	if strings.TrimSpace(slug) == "" {
+		return ErrorResult("identifier is required and must be a non-empty string")
 	}
 
 	// Validate registry
 	registryName, _ := args["registry"].(string)
+	if registryName == "" {
+		registryName = defaultSkillRegistryName
+	}
 	if err := utils.ValidateSkillIdentifier(registryName); err != nil {
 		return ErrorResult(fmt.Sprintf("invalid registry %q: error: %s", registryName, err.Error()))
 	}
 
-	version, _ := args["version"].(string)
-	force, _ := args["force"].(bool)
-
-	// Check if already installed.
-	skillsDir := filepath.Join(t.workspace, "skills")
-	targetDir := filepath.Join(skillsDir, slug)
-
-	if !force {
-		if _, err := os.Stat(targetDir); err == nil {
-			return ErrorResult(
-				fmt.Sprintf("skill %q already installed at %s. Use force=true to reinstall.", slug, targetDir),
-			)
-		}
-	} else {
-		// Force: remove existing if present.
-		os.RemoveAll(targetDir)
-	}
-
 	// Resolve which registry to use.
 	registry := t.registryMgr.GetRegistry(registryName)
 	if registry == nil {
 		return ErrorResult(fmt.Sprintf("registry %q not found", registryName))
 	}
 
+	// Validate target and resolve install directory.
+	dirName, err := registry.ResolveInstallDirName(slug)
+	if err != nil {
+		return ErrorResult(fmt.Sprintf("invalid slug %q: error: %s", slug, err.Error()))
+	}
+
+	version, _ := args["version"].(string)
+	force, _ := args["force"].(bool)
+
+	// Check if already installed.
+	skillsDir := filepath.Join(t.workspace, "skills")
+	targetDir := filepath.Join(skillsDir, dirName)
+	backupDir := ""
+	restorePreviousInstall := func() {
+		if backupDir == "" {
+			return
+		}
+		if rmErr := os.RemoveAll(targetDir); rmErr != nil {
+			logger.ErrorCF("tool", "Failed to remove failed install before restore",
+				map[string]any{
+					"tool":       "install_skill",
+					"target_dir": targetDir,
+					"error":      rmErr.Error(),
+				})
+			return
+		}
+		if restoreErr := os.Rename(backupDir, targetDir); restoreErr != nil {
+			logger.ErrorCF("tool", "Failed to restore previous install after failed reinstall",
+				map[string]any{
+					"tool":       "install_skill",
+					"backup_dir": backupDir,
+					"target_dir": targetDir,
+					"error":      restoreErr.Error(),
+				})
+			return
+		}
+		backupDir = ""
+	}
+
+	if !force {
+		if _, statErr := os.Stat(targetDir); statErr == nil {
+			return ErrorResult(
+				fmt.Sprintf("skill %q already installed at %s. Use force=true to reinstall.", slug, targetDir),
+			)
+		}
+	} else {
+		if _, statErr := os.Stat(targetDir); statErr == nil {
+			backupDir = filepath.Join(skillsDir, fmt.Sprintf(".%s.picoclaw-backup-%d", dirName, time.Now().UnixNano()))
+			if renameErr := os.Rename(targetDir, backupDir); renameErr != nil {
+				return ErrorResult(fmt.Sprintf("failed to prepare reinstall for %q: %v", slug, renameErr))
+			}
+		} else if !os.IsNotExist(statErr) {
+			return ErrorResult(fmt.Sprintf("failed to inspect existing install for %q: %v", slug, statErr))
+		}
+	}
+
 	// Ensure skills directory exists.
-	if err := os.MkdirAll(skillsDir, 0o755); err != nil {
-		return ErrorResult(fmt.Sprintf("failed to create skills directory: %v", err))
+	if mkdirErr := os.MkdirAll(skillsDir, 0o755); mkdirErr != nil {
+		restorePreviousInstall()
+		return ErrorResult(fmt.Sprintf("failed to create skills directory: %v", mkdirErr))
 	}
 
 	// Download and install (handles metadata, version resolution, extraction).
@@ -128,6 +174,7 @@ func (t *InstallSkillTool) Execute(ctx context.Context, args map[string]any) *To
 					"error":      rmErr.Error(),
 				})
 		}
+		restorePreviousInstall()
 		return ErrorResult(fmt.Sprintf("failed to install %q: %v", slug, err))
 	}
 
@@ -142,11 +189,26 @@ func (t *InstallSkillTool) Execute(ctx context.Context, args map[string]any) *To
 					"error":      rmErr.Error(),
 				})
 		}
+		restorePreviousInstall()
 		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(),
+				})
+		}
+		restorePreviousInstall()
+		return ErrorResult(fmt.Sprintf("failed to install %q: registry archive is not a valid skill", slug))
+	}
+
 	// Write origin metadata.
-	if err := writeOriginMeta(targetDir, registry.Name(), slug, result.Version); err != nil {
+	if err := persistInstalledSkillOriginMeta(targetDir, registry, slug, result.Version); err != nil {
 		logger.ErrorCF("tool", "Failed to write origin metadata",
 			map[string]any{
 				"tool":     "install_skill",
@@ -156,7 +218,27 @@ func (t *InstallSkillTool) Execute(ctx context.Context, args map[string]any) *To
 				"slug":     slug,
 				"version":  result.Version,
 			})
-		_ = err
+		rmErr := os.RemoveAll(targetDir)
+		if rmErr != nil {
+			logger.ErrorCF("tool", "Failed to roll back install after metadata write failure",
+				map[string]any{
+					"tool":       "install_skill",
+					"target_dir": targetDir,
+					"error":      rmErr.Error(),
+				})
+		}
+		restorePreviousInstall()
+		return ErrorResult(fmt.Sprintf("failed to persist skill metadata for %q: %v", slug, err))
+	}
+	if backupDir != "" {
+		if rmErr := os.RemoveAll(backupDir); rmErr != nil {
+			logger.ErrorCF("tool", "Failed to remove previous install backup after successful reinstall",
+				map[string]any{
+					"tool":       "install_skill",
+					"backup_dir": backupDir,
+					"error":      rmErr.Error(),
+				})
+		}
 	}
 
 	// Build result with moderation warning if suspicious.
@@ -178,17 +260,27 @@ func (t *InstallSkillTool) Execute(ctx context.Context, args map[string]any) *To
 // originMeta tracks which registry a skill was installed from.
 type originMeta struct {
 	Version          int    `json:"version"`
+	OriginKind       string `json:"origin_kind,omitempty"`
 	Registry         string `json:"registry"`
 	Slug             string `json:"slug"`
+	RegistryURL      string `json:"registry_url,omitempty"`
 	InstalledVersion string `json:"installed_version"`
 	InstalledAt      int64  `json:"installed_at"`
 }
 
-func writeOriginMeta(targetDir, registryName, slug, version string) error {
+func writeOriginMeta(targetDir string, registry skills.SkillRegistry, slug, version string) error {
+	normalizedSlug, registryURL := skills.BuildInstallMetadataForRegistryInstance(registry, slug, version)
+	registryName := ""
+	if registry != nil {
+		registryName = registry.Name()
+	}
+
 	meta := originMeta{
 		Version:          1,
+		OriginKind:       "third_party",
 		Registry:         registryName,
-		Slug:             slug,
+		Slug:             normalizedSlug,
+		RegistryURL:      registryURL,
 		InstalledVersion: version,
 		InstalledAt:      time.Now().UnixMilli(),
 	}
@@ -201,3 +293,16 @@ func writeOriginMeta(targetDir, registryName, slug, version string) error {
 	// 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
+}

pkg/tools/skills_install_test.go

@@ -2,6 +2,7 @@ package tools
 
 import (
 	"context"
+	"encoding/json"
 	"os"
 	"path/filepath"
 	"testing"
@@ -12,6 +13,157 @@ import (
 	"github.com/sipeed/picoclaw/pkg/skills"
 )
 
+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) {
+	return target, nil
+}
+
+func (m *mockInstallRegistry) SkillURL(slug, _ string) string { return slug }
+
+func (m *mockInstallRegistry) Search(context.Context, string, int) ([]skills.SearchResult, error) {
+	return nil, nil
+}
+
+func (m *mockInstallRegistry) GetSkillMeta(context.Context, string) (*skills.SkillMeta, error) {
+	return nil, nil
+}
+
+func (m *mockInstallRegistry) 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(validSkillMarkdown), 0o600); err != nil {
+		return nil, err
+	}
+	return &skills.InstallResult{Version: "test"}, nil
+}
+
+type mockGitHubInstallRegistry struct{}
+
+func (m *mockGitHubInstallRegistry) Name() string { return "github" }
+
+func (m *mockGitHubInstallRegistry) ResolveInstallDirName(target string) (string, error) {
+	return "pr-review", nil
+}
+
+func (m *mockGitHubInstallRegistry) SkillURL(slug, _ string) string { return slug }
+
+func (m *mockGitHubInstallRegistry) Search(context.Context, string, int) ([]skills.SearchResult, error) {
+	return nil, nil
+}
+
+func (m *mockGitHubInstallRegistry) GetSkillMeta(context.Context, string) (*skills.SkillMeta, error) {
+	return nil, nil
+}
+
+func (m *mockGitHubInstallRegistry) 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(validSkillMarkdown), 0o600); err != nil {
+		return nil, err
+	}
+	return &skills.InstallResult{Version: "main"}, nil
+}
+
+type stubGitHubInstallRegistry struct {
+	*skills.GitHubRegistry
+}
+
+func (m *stubGitHubInstallRegistry) 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(validSkillMarkdown), 0o600); err != nil {
+		return nil, err
+	}
+	return &skills.InstallResult{Version: "main"}, nil
+}
+
+type mockInvalidInstallRegistry struct{}
+
+type mockFailingInstallRegistry 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 (m *mockFailingInstallRegistry) Name() string { return "clawhub" }
+
+func (m *mockFailingInstallRegistry) ResolveInstallDirName(target string) (string, error) {
+	return target, nil
+}
+
+func (m *mockFailingInstallRegistry) SkillURL(slug, _ string) string { return slug }
+
+func (m *mockFailingInstallRegistry) Search(context.Context, string, int) ([]skills.SearchResult, error) {
+	return nil, nil
+}
+
+func (m *mockFailingInstallRegistry) GetSkillMeta(context.Context, string) (*skills.SkillMeta, error) {
+	return nil, nil
+}
+
+func (m *mockFailingInstallRegistry) DownloadAndInstall(
+	_ context.Context,
+	_ string,
+	_ string,
+	_ string,
+) (*skills.InstallResult, error) {
+	return nil, assert.AnError
+}
+
 func TestInstallSkillToolName(t *testing.T) {
 	tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir())
 	assert.Equal(t, "install_skill", tool.Name())
@@ -34,7 +186,9 @@ func TestInstallSkillToolEmptySlug(t *testing.T) {
 }
 
 func TestInstallSkillToolUnsafeSlug(t *testing.T) {
-	tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir())
+	registryMgr := skills.NewRegistryManager()
+	registryMgr.AddRegistry(skills.NewClawHubRegistry(skills.ClawHubConfig{Enabled: true}))
+	tool := NewInstallSkillTool(registryMgr, t.TempDir())
 
 	cases := []string{
 		"../etc/passwd",
@@ -44,7 +198,8 @@ func TestInstallSkillToolUnsafeSlug(t *testing.T) {
 
 	for _, slug := range cases {
 		result := tool.Execute(context.Background(), map[string]any{
-			"slug": slug,
+			"slug":     slug,
+			"registry": "clawhub",
 		})
 		assert.True(t, result.IsError, "slug %q should be rejected", slug)
 		assert.Contains(t, result.ForLLM, "invalid slug")
@@ -56,7 +211,9 @@ func TestInstallSkillToolAlreadyExists(t *testing.T) {
 	skillDir := filepath.Join(workspace, "skills", "existing-skill")
 	require.NoError(t, os.MkdirAll(skillDir, 0o755))
 
-	tool := NewInstallSkillTool(skills.NewRegistryManager(), workspace)
+	registryMgr := skills.NewRegistryManager()
+	registryMgr.AddRegistry(&mockInstallRegistry{})
+	tool := NewInstallSkillTool(registryMgr, workspace)
 	result := tool.Execute(context.Background(), map[string]any{
 		"slug":     "existing-skill",
 		"registry": "clawhub",
@@ -91,14 +248,176 @@ func TestInstallSkillToolParameters(t *testing.T) {
 	required, ok := params["required"].([]string)
 	assert.True(t, ok)
 	assert.Contains(t, required, "slug")
-	assert.Contains(t, required, "registry")
+	assert.NotContains(t, required, "registry")
 }
 
 func TestInstallSkillToolMissingRegistry(t *testing.T) {
-	tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir())
+	registryMgr := skills.NewRegistryManager()
+	registryMgr.AddRegistry(&mockGitHubInstallRegistry{})
+	tool := NewInstallSkillTool(registryMgr, t.TempDir())
 	result := tool.Execute(context.Background(), map[string]any{
 		"slug": "some-skill",
 	})
-	assert.True(t, result.IsError)
-	assert.Contains(t, result.ForLLM, "invalid registry")
+	assert.False(t, result.IsError)
+	assert.Contains(t, result.ForLLM, `Successfully installed skill`)
+}
+
+func TestInstallSkillToolAllowsGitHubURLSlug(t *testing.T) {
+	registry := skills.GitHubRegistryConfig{Enabled: true, BaseURL: "https://github.com"}.BuildRegistry()
+	githubRegistry, ok := registry.(*skills.GitHubRegistry)
+	require.True(t, ok)
+
+	registryMgr := skills.NewRegistryManager()
+	registryMgr.AddRegistry(&stubGitHubInstallRegistry{GitHubRegistry: githubRegistry})
+	workspace := t.TempDir()
+	tool := NewInstallSkillTool(registryMgr, workspace)
+
+	slug := "https://github.com/synthetic-lab/octofriend/tree/main/.agents/skills/pr-review"
+	result := tool.Execute(context.Background(), map[string]any{
+		"slug":     slug,
+		"registry": "github",
+	})
+
+	assert.False(t, result.IsError)
+	assert.Contains(t, result.ForLLM, `Successfully installed skill`)
+
+	data, err := os.ReadFile(filepath.Join(workspace, "skills", "pr-review", ".skill-origin.json"))
+	require.NoError(t, err)
+
+	var meta originMeta
+	require.NoError(t, json.Unmarshal(data, &meta))
+	assert.Equal(t, "third_party", meta.OriginKind)
+	assert.Equal(t, "github", meta.Registry)
+	assert.Equal(t, "synthetic-lab/octofriend/.agents/skills/pr-review", meta.Slug)
+	assert.Equal(t, slug, meta.RegistryURL)
+	assert.Equal(t, "main", meta.InstalledVersion)
+	assert.NotZero(t, meta.InstalledAt)
+}
+
+func TestInstallSkillToolPreservesGitHubSourceURLWithEnterpriseRegistry(t *testing.T) {
+	registry := skills.GitHubRegistryConfig{Enabled: true, BaseURL: "https://ghe.example.com/git"}.BuildRegistry()
+	githubRegistry, ok := registry.(*skills.GitHubRegistry)
+	require.True(t, ok)
+
+	registryMgr := skills.NewRegistryManager()
+	registryMgr.AddRegistry(&stubGitHubInstallRegistry{GitHubRegistry: githubRegistry})
+	workspace := t.TempDir()
+	tool := NewInstallSkillTool(registryMgr, workspace)
+
+	slug := "https://github.com/synthetic-lab/octofriend/tree/main/.agents/skills/pr-review"
+	result := tool.Execute(context.Background(), map[string]any{
+		"slug":     slug,
+		"registry": "github",
+	})
+
+	assert.False(t, result.IsError)
+
+	data, err := os.ReadFile(filepath.Join(workspace, "skills", "pr-review", ".skill-origin.json"))
+	require.NoError(t, err)
+
+	var meta originMeta
+	require.NoError(t, json.Unmarshal(data, &meta))
+	assert.Equal(t, "synthetic-lab/octofriend/.agents/skills/pr-review", meta.Slug)
+	assert.Equal(t, slug, meta.RegistryURL)
+	assert.Equal(t, "main", meta.InstalledVersion)
+}
+
+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))
+}
+
+func TestInstallSkillToolRollsBackOnOriginMetadataWriteFailure(t *testing.T) {
+	workspace := t.TempDir()
+	registryMgr := skills.NewRegistryManager()
+	registryMgr.AddRegistry(&mockInstallRegistry{})
+	tool := NewInstallSkillTool(registryMgr, workspace)
+
+	previousPersist := persistInstalledSkillOriginMeta
+	persistInstalledSkillOriginMeta = func(string, skills.SkillRegistry, string, string) error {
+		return assert.AnError
+	}
+	defer func() {
+		persistInstalledSkillOriginMeta = previousPersist
+	}()
+
+	result := tool.Execute(context.Background(), map[string]any{
+		"slug":     "rollback-skill",
+		"registry": "clawhub",
+	})
+
+	assert.True(t, result.IsError)
+	assert.Contains(t, result.ForLLM, "failed to persist skill metadata")
+	_, err := os.Stat(filepath.Join(workspace, "skills", "rollback-skill"))
+	assert.True(t, os.IsNotExist(err))
+}
+
+func TestInstallSkillToolForceReinstallRestoresPreviousSkillAfterDownloadFailure(t *testing.T) {
+	workspace := t.TempDir()
+	skillDir := filepath.Join(workspace, "skills", "existing-skill")
+	require.NoError(t, os.MkdirAll(skillDir, 0o755))
+	oldContent := []byte("---\nname: existing-skill\ndescription: Existing skill\n---\n# Existing\n")
+	require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), oldContent, 0o600))
+
+	registryMgr := skills.NewRegistryManager()
+	registryMgr.AddRegistry(&mockFailingInstallRegistry{})
+	tool := NewInstallSkillTool(registryMgr, workspace)
+
+	result := tool.Execute(context.Background(), map[string]any{
+		"slug":     "existing-skill",
+		"registry": "clawhub",
+		"force":    true,
+	})
+
+	assert.True(t, result.IsError)
+	assert.Contains(t, result.ForLLM, "failed to install")
+
+	gotContent, err := os.ReadFile(filepath.Join(skillDir, "SKILL.md"))
+	require.NoError(t, err)
+	assert.Equal(t, oldContent, gotContent)
+}
+
+func TestInstallSkillToolForceReinstallRestoresPreviousSkillAfterMetadataFailure(t *testing.T) {
+	workspace := t.TempDir()
+	skillDir := filepath.Join(workspace, "skills", "existing-skill")
+	require.NoError(t, os.MkdirAll(skillDir, 0o755))
+	oldContent := []byte("---\nname: existing-skill\ndescription: Existing skill\n---\n# Existing\n")
+	require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), oldContent, 0o600))
+
+	registryMgr := skills.NewRegistryManager()
+	registryMgr.AddRegistry(&mockInstallRegistry{})
+	tool := NewInstallSkillTool(registryMgr, workspace)
+
+	previousPersist := persistInstalledSkillOriginMeta
+	persistInstalledSkillOriginMeta = func(string, skills.SkillRegistry, string, string) error {
+		return assert.AnError
+	}
+	defer func() {
+		persistInstalledSkillOriginMeta = previousPersist
+	}()
+
+	result := tool.Execute(context.Background(), map[string]any{
+		"slug":     "existing-skill",
+		"registry": "clawhub",
+		"force":    true,
+	})
+
+	assert.True(t, result.IsError)
+	assert.Contains(t, result.ForLLM, "failed to persist skill metadata")
+
+	gotContent, err := os.ReadFile(filepath.Join(skillDir, "SKILL.md"))
+	require.NoError(t, err)
+	assert.Equal(t, oldContent, gotContent)
 }