From 24794d31e4a7231df0d023131df8063cb6fe33cb Mon Sep 17 00:00:00 2001 From: lxowalle Date: Mon, 13 Apr 2026 01:12:20 +0800 Subject: [PATCH] fix install_skill force reinstall rollback --- pkg/tools/skills_install.go | 51 ++++++++++++++++++- pkg/tools/skills_install_test.go | 85 ++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 2 deletions(-) diff --git a/pkg/tools/skills_install.go b/pkg/tools/skills_install.go index 1bab82bf2..79d0672b9 100644 --- a/pkg/tools/skills_install.go +++ b/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. - os.RemoveAll(targetDir) + 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 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 diff --git a/pkg/tools/skills_install_test.go b/pkg/tools/skills_install_test.go index ec8902b36..125348883 100644 --- a/pkg/tools/skills_install_test.go +++ b/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) +}