fix install_skill force reinstall rollback

pkg/tools/skills_install.go

@@ -111,6 +111,32 @@ func (t *InstallSkillTool) Execute(ctx context.Context, args map[string]any) *To
 	// 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 {
@@ -119,12 +145,19 @@ func (t *InstallSkillTool) Execute(ctx context.Context, args map[string]any) *To
 			)
 		}
 	} else {
-		// Force: remove existing if present.
+		if _, statErr := os.Stat(targetDir); statErr == nil {
-		os.RemoveAll(targetDir)
+			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 mkdirErr := os.MkdirAll(skillsDir, 0o755); mkdirErr != nil {
+		restorePreviousInstall()
 		return ErrorResult(fmt.Sprintf("failed to create skills directory: %v", mkdirErr))
 	}
 
@@ -141,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))
 	}
 
@@ -155,6 +189,7 @@ 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))
 	}
 
@@ -168,6 +203,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: registry archive is not a valid skill", slug))
 	}
 
@@ -191,8 +227,19 @@ func (t *InstallSkillTool) Execute(ctx context.Context, args map[string]any) *To
 					"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.
 	var output string

pkg/tools/skills_install_test.go

@@ -102,6 +102,8 @@ func (m *stubGitHubInstallRegistry) DownloadAndInstall(
 
 type mockInvalidInstallRegistry struct{}
 
+type mockFailingInstallRegistry struct{}
+
 func (m *mockInvalidInstallRegistry) Name() string { return "clawhub" }
 
 func (m *mockInvalidInstallRegistry) ResolveInstallDirName(target string) (string, error) {
@@ -137,6 +139,31 @@ func (m *mockInvalidInstallRegistry) DownloadAndInstall(
 	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())
@@ -336,3 +363,61 @@ func TestInstallSkillToolRollsBackOnOriginMetadataWriteFailure(t *testing.T) {
 	_, 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)
+}