From 7c2c36bda271075b95513b7009d4df007bbfa497 Mon Sep 17 00:00:00 2001 From: Eva H <63033505+hoyyeva@users.noreply.github.com> Date: Wed, 6 May 2026 08:32:54 -0700 Subject: [PATCH] cmd/launch: improve integration backup UX (#15907) --- cmd/internal/fileutil/files.go | 95 +++++++++++++++++++++--- cmd/internal/fileutil/files_test.go | 111 +++++++++++++++++++++++++++- cmd/launch/cline.go | 2 +- cmd/launch/command_test.go | 17 ++--- cmd/launch/droid.go | 2 +- cmd/launch/droid_test.go | 4 +- cmd/launch/hermes.go | 2 +- cmd/launch/integrations_test.go | 2 +- cmd/launch/launch.go | 6 +- cmd/launch/launch_test.go | 26 ------- cmd/launch/models.go | 36 +-------- cmd/launch/openclaw.go | 2 +- cmd/launch/openclaw_test.go | 5 +- cmd/launch/opencode.go | 2 +- cmd/launch/pi.go | 4 +- cmd/launch/pi_test.go | 57 ++++++++++++++ cmd/launch/vscode.go | 4 +- cmd/launch/vscode_test.go | 47 ++++++++++++ 18 files changed, 324 insertions(+), 100 deletions(-) diff --git a/cmd/internal/fileutil/files.go b/cmd/internal/fileutil/files.go index bc57ba05..9e03c820 100644 --- a/cmd/internal/fileutil/files.go +++ b/cmd/internal/fileutil/files.go @@ -8,9 +8,16 @@ import ( "fmt" "os" "path/filepath" + "sort" + "strconv" + "strings" "time" ) +// Keep a bounded number of backups per file so config backups do not grow +// without limit. We keep the 5 most recent backups and do not pin the oldest. +const maxBackupsPerFile = 5 + // ReadJSON reads a JSON object file into a generic map. func ReadJSON(path string) (map[string]any, error) { data, err := os.ReadFile(path) @@ -36,34 +43,51 @@ func copyFile(src, dst string) error { return os.WriteFile(dst, data, info.Mode().Perm()) } -// BackupDir returns the shared backup directory used before overwriting files. +// BackupDir returns the shared backup root used before overwriting files. func BackupDir() string { - return filepath.Join(os.TempDir(), "ollama-backups") + if home, err := os.UserHomeDir(); err == nil && home != "" { + return filepath.Join(home, ".ollama", "backup") + } + return filepath.Join(os.TempDir(), "ollama-backup") } -func backupToTmp(srcPath string) (string, error) { +func writeBackupCopy(srcPath string, integration string) (string, error) { dir := BackupDir() + name := filepath.Base(srcPath) + if integration != "" { + dir = filepath.Join(dir, integration) + } + if err := os.MkdirAll(dir, 0o755); err != nil { return "", err } - backupPath := filepath.Join(dir, fmt.Sprintf("%s.%d", filepath.Base(srcPath), time.Now().Unix())) + backupPath := filepath.Join(dir, fmt.Sprintf("%s.%d", name, time.Now().Unix())) if err := copyFile(srcPath, backupPath); err != nil { return "", err } + pruneOldBackups(dir, name, maxBackupsPerFile) return backupPath, nil } -// WriteWithBackup writes data to path via temp file + rename, backing up any existing file first. -func WriteWithBackup(path string, data []byte) error { +// WriteWithBackup writes data to path via temp file + rename, backing up any +// existing file first. Callers may optionally pass one integration name to +// store backups under BackupDir()/...//. +func WriteWithBackup(path string, data []byte, integration ...string) error { + backupIntegration := "" + if len(integration) > 0 { + backupIntegration = integration[0] + } + var backupPath string // backup must be created before any writes to the target file if existingContent, err := os.ReadFile(path); err == nil { - if !bytes.Equal(existingContent, data) { - backupPath, err = backupToTmp(path) - if err != nil { - return fmt.Errorf("backup failed: %w", err) - } + if bytes.Equal(existingContent, data) { + return nil + } + backupPath, err = writeBackupCopy(path, backupIntegration) + if err != nil { + return fmt.Errorf("backup failed: %w", err) } } else if !os.IsNotExist(err) { return fmt.Errorf("read existing file: %w", err) @@ -101,3 +125,52 @@ func WriteWithBackup(path string, data []byte) error { return nil } + +func pruneOldBackups(dir, name string, keep int) { + if keep < 1 { + return + } + + entries, err := os.ReadDir(dir) + if err != nil { + return + } + + type backupEntry struct { + name string + timestamp int64 + } + + prefix := name + "." + backups := make([]backupEntry, 0, len(entries)) + for _, entry := range entries { + if entry.IsDir() || !strings.HasPrefix(entry.Name(), prefix) { + continue + } + + timestamp, err := strconv.ParseInt(strings.TrimPrefix(entry.Name(), prefix), 10, 64) + if err != nil { + continue + } + + backups = append(backups, backupEntry{ + name: entry.Name(), + timestamp: timestamp, + }) + } + + if len(backups) <= keep { + return + } + + sort.Slice(backups, func(i, j int) bool { + if backups[i].timestamp != backups[j].timestamp { + return backups[i].timestamp > backups[j].timestamp + } + return backups[i].name > backups[j].name + }) + + for _, backup := range backups[keep:] { + _ = os.Remove(filepath.Join(dir, backup.name)) + } +} diff --git a/cmd/internal/fileutil/files_test.go b/cmd/internal/fileutil/files_test.go index f00a63b1..0c1108a8 100644 --- a/cmd/internal/fileutil/files_test.go +++ b/cmd/internal/fileutil/files_test.go @@ -18,6 +18,12 @@ func TestMain(m *testing.M) { if err := os.Setenv("TMPDIR", tmpRoot); err != nil { panic(err) } + if err := os.Setenv("HOME", tmpRoot); err != nil { + panic(err) + } + if err := os.Setenv("USERPROFILE", tmpRoot); err != nil { + panic(err) + } code := m.Run() _ = os.RemoveAll(tmpRoot) @@ -41,6 +47,17 @@ func isolatedTempDir(t *testing.T) string { func TestWriteWithBackup(t *testing.T) { tmpDir := isolatedTempDir(t) + t.Run("uses ollama directory under home", func(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + + want := filepath.Join(home, ".ollama", "backup") + if got := BackupDir(); got != want { + t.Fatalf("BackupDir() = %q, want %q", got, want) + } + }) + t.Run("creates file", func(t *testing.T) { path := filepath.Join(tmpDir, "new.json") data := mustMarshal(t, map[string]string{"key": "value"}) @@ -63,7 +80,7 @@ func TestWriteWithBackup(t *testing.T) { } }) - t.Run("creates backup in the temp backup directory", func(t *testing.T) { + t.Run("creates backup in the shared backup directory", func(t *testing.T) { path := filepath.Join(tmpDir, "backup.json") os.WriteFile(path, []byte(`{"original": true}`), 0o644) @@ -110,6 +127,35 @@ func TestWriteWithBackup(t *testing.T) { } }) + t.Run("stores hinted backups under a subdirectory", func(t *testing.T) { + path := filepath.Join(tmpDir, "hinted.json") + os.WriteFile(path, []byte(`{"original": true}`), 0o644) + + data := mustMarshal(t, map[string]bool{"updated": true}) + if err := WriteWithBackup(path, data, "openclaw"); err != nil { + t.Fatal(err) + } + + entries, err := os.ReadDir(filepath.Join(BackupDir(), "openclaw")) + if err != nil { + t.Fatal(err) + } + + var found bool + for _, entry := range entries { + name := entry.Name() + if len(name) > len("hinted.json.") && name[:len("hinted.json.")] == "hinted.json." { + found = true + _ = os.Remove(filepath.Join(BackupDir(), "openclaw", name)) + break + } + } + + if !found { + t.Error("backup file was not created under hint directory") + } + }) + t.Run("no backup for new file", func(t *testing.T) { path := filepath.Join(tmpDir, "nobak.json") @@ -189,6 +235,35 @@ func TestWriteWithBackup(t *testing.T) { t.Error("backup file with timestamp not found") } }) + + t.Run("retains only the five newest backups per file", func(t *testing.T) { + path := filepath.Join(tmpDir, "pruned.json") + if err := os.WriteFile(path, []byte(`{"v": 0}`), 0o644); err != nil { + t.Fatal(err) + } + + for i := 1; i <= maxBackupsPerFile; i++ { + backupPath := filepath.Join(BackupDir(), fmt.Sprintf("pruned.json.%d", i)) + if err := os.WriteFile(backupPath, []byte(fmt.Sprintf(`{"v": %d}`, i)), 0o644); err != nil { + t.Fatal(err) + } + } + + if err := WriteWithBackup(path, []byte(`{"v": 1}`)); err != nil { + t.Fatal(err) + } + + backups, err := filepath.Glob(filepath.Join(BackupDir(), "pruned.json.*")) + if err != nil { + t.Fatal(err) + } + if len(backups) != maxBackupsPerFile { + t.Fatalf("expected %d backups after pruning, got %d", maxBackupsPerFile, len(backups)) + } + if _, err := os.Stat(filepath.Join(BackupDir(), "pruned.json.1")); !os.IsNotExist(err) { + t.Fatalf("expected oldest backup to be pruned, stat err = %v", err) + } + }) } // Edge case tests for files.go @@ -251,6 +326,36 @@ func TestWriteWithBackup_PermissionDenied(t *testing.T) { } } +func TestWriteWithBackup_UnchangedContentIsNoOp(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("permission tests unreliable on Windows") + } + + tmpDir := isolatedTempDir(t) + path := filepath.Join(tmpDir, "unchanged-noop.json") + data := []byte(`{"same":true}`) + if err := os.WriteFile(path, data, 0o644); err != nil { + t.Fatal(err) + } + + if err := os.Chmod(tmpDir, 0o555); err != nil { + t.Fatal(err) + } + defer os.Chmod(tmpDir, 0o755) + + if err := WriteWithBackup(path, data); err != nil { + t.Fatalf("expected unchanged write to be a no-op, got %v", err) + } + + backups, err := filepath.Glob(filepath.Join(BackupDir(), "unchanged-noop.json.*")) + if err != nil { + t.Fatal(err) + } + if len(backups) != 0 { + t.Fatalf("expected no backups for unchanged content, got %d", len(backups)) + } +} + // TestWriteWithBackup_DirectoryDoesNotExist verifies behavior when target directory doesn't exist. // writeWithBackup doesn't create directories - caller is responsible. func TestWriteWithBackup_DirectoryDoesNotExist(t *testing.T) { @@ -302,9 +407,9 @@ func TestBackupToTmp_SpecialCharsInFilename(t *testing.T) { path := filepath.Join(tmpDir, "my config (backup).json") os.WriteFile(path, []byte(`{"test": true}`), 0o644) - backupPath, err := backupToTmp(path) + backupPath, err := writeBackupCopy(path, "") if err != nil { - t.Fatalf("backupToTmp with special chars failed: %v", err) + t.Fatalf("writeBackupCopy with special chars failed: %v", err) } // Verify backup exists and has correct content diff --git a/cmd/launch/cline.go b/cmd/launch/cline.go index 9ad88680..68957b50 100644 --- a/cmd/launch/cline.go +++ b/cmd/launch/cline.go @@ -78,7 +78,7 @@ func (c *Cline) Edit(models []string) error { if err != nil { return err } - return fileutil.WriteWithBackup(configPath, data) + return fileutil.WriteWithBackup(configPath, data, "cline") } func (c *Cline) Models() []string { diff --git a/cmd/launch/command_test.go b/cmd/launch/command_test.go index 386b55cd..4bae22d8 100644 --- a/cmd/launch/command_test.go +++ b/cmd/launch/command_test.go @@ -479,7 +479,7 @@ func TestLaunchCmdHeadlessWithYes_AutoPullsMissingLocalModel(t *testing.T) { } } -func TestLaunchCmdHeadlessWithoutYes_ReturnsActionableConfirmError(t *testing.T) { +func TestLaunchCmdHeadlessWithoutYes_AllowsConfiguredLaunch(t *testing.T) { tmpDir := t.TempDir() setLaunchTestHome(t, tmpDir) withLauncherHooks(t) @@ -511,17 +511,14 @@ func TestLaunchCmdHeadlessWithoutYes_ReturnsActionableConfirmError(t *testing.T) cmd := LaunchCmd(func(cmd *cobra.Command, args []string) error { return nil }, func(cmd *cobra.Command) {}) cmd.SetArgs([]string{"stubeditor", "--model", "llama3.2"}) err := cmd.Execute() - if err == nil { - t.Fatal("expected launch command to fail without --yes in headless mode") + if err != nil { + t.Fatalf("expected launch command to succeed without --yes when an explicit model is provided, got %v", err) } - if !strings.Contains(err.Error(), "re-run with --yes") { - t.Fatalf("expected actionable --yes guidance, got %v", err) + if diff := compareStringSlices(stub.edited, [][]string{{"llama3.2"}}); diff != "" { + t.Fatalf("unexpected editor writes (-want +got):\n%s", diff) } - if len(stub.edited) != 0 { - t.Fatalf("expected no editor writes when confirmation is blocked, got %v", stub.edited) - } - if stub.ranModel != "" { - t.Fatalf("expected launch to abort before run, got %q", stub.ranModel) + if stub.ranModel != "llama3.2" { + t.Fatalf("expected launch to run configured model, got %q", stub.ranModel) } } diff --git a/cmd/launch/droid.go b/cmd/launch/droid.go index 1612352a..45ba215b 100644 --- a/cmd/launch/droid.go +++ b/cmd/launch/droid.go @@ -96,7 +96,7 @@ func (d *Droid) Edit(models []string) error { if err != nil { return err } - return fileutil.WriteWithBackup(settingsPath, data) + return fileutil.WriteWithBackup(settingsPath, data, "droid") } func updateDroidSettings(settingsMap map[string]any, settings droidSettings, models []string) map[string]any { diff --git a/cmd/launch/droid_test.go b/cmd/launch/droid_test.go index f3714306..e0804fee 100644 --- a/cmd/launch/droid_test.go +++ b/cmd/launch/droid_test.go @@ -1172,7 +1172,7 @@ func TestDroidEdit_BackupCreated(t *testing.T) { settingsDir := filepath.Join(tmpDir, ".factory") settingsPath := filepath.Join(settingsDir, "settings.json") - backupDir := filepath.Join(os.TempDir(), "ollama-backups") + backupDir := fileutil.BackupDir() os.MkdirAll(settingsDir, 0o755) @@ -1186,7 +1186,7 @@ func TestDroidEdit_BackupCreated(t *testing.T) { } // Find backup containing our unique marker - backups, _ := filepath.Glob(filepath.Join(backupDir, "settings.json.*")) + backups, _ := filepath.Glob(filepath.Join(backupDir, "droid", "settings.json.*")) foundBackup := false for _, backup := range backups { data, err := os.ReadFile(backup) diff --git a/cmd/launch/hermes.go b/cmd/launch/hermes.go index fd09fe61..5ec72386 100644 --- a/cmd/launch/hermes.go +++ b/cmd/launch/hermes.go @@ -132,7 +132,7 @@ func (h *Hermes) Configure(model string) error { if err := os.MkdirAll(filepath.Dir(configPath), 0o755); err != nil { return err } - return fileutil.WriteWithBackup(configPath, data) + return fileutil.WriteWithBackup(configPath, data, "hermes") } func (h *Hermes) CurrentModel() string { diff --git a/cmd/launch/integrations_test.go b/cmd/launch/integrations_test.go index 310aaf95..fdd3ce18 100644 --- a/cmd/launch/integrations_test.go +++ b/cmd/launch/integrations_test.go @@ -822,7 +822,7 @@ func TestPrepareEditorIntegration_SavesOnlyAfterSuccessfulEdit(t *testing.T) { } editor := &stubEditorRunner{editErr: errors.New("boom")} - err := prepareEditorIntegration("droid", editor, editor, []string{"new-model"}) + err := prepareEditorIntegration("droid", editor, []string{"new-model"}) if err == nil || !strings.Contains(err.Error(), "setup failed") { t.Fatalf("expected setup failure, got %v", err) } diff --git a/cmd/launch/launch.go b/cmd/launch/launch.go index 34948bdf..ee03d778 100644 --- a/cmd/launch/launch.go +++ b/cmd/launch/launch.go @@ -710,7 +710,7 @@ func (c *launcherClient) launchEditorIntegration(ctx context.Context, name strin } if (needsConfigure || req.ModelOverride != "") && !savedMatchesModels(saved, models) { - if err := prepareEditorIntegration(name, runner, editor, models); err != nil { + if err := prepareEditorIntegration(name, editor, models); err != nil { return err } } @@ -738,7 +738,7 @@ func (c *launcherClient) launchManagedSingleIntegration(ctx context.Context, nam if err != nil { return err } - if err := prepareManagedSingleIntegration(name, runner, managed, target, configureModels); err != nil { + if err := prepareManagedSingleIntegration(name, managed, target, configureModels); err != nil { return err } if refresher, ok := managed.(ManagedRuntimeRefresher); ok { @@ -777,7 +777,7 @@ func (c *launcherClient) launchManagedAutodiscoveryIntegration(ctx context.Conte needsConfigure := req.ForceConfigure || req.ConfigureOnly || !autodiscovery.AutodiscoveryConfigured() || !savedMatchesModels(saved, []string{target}) if needsConfigure { - if err := prepareManagedAutodiscoveryIntegration(name, runner, autodiscovery, target); err != nil { + if err := prepareManagedAutodiscoveryIntegration(name, autodiscovery, target); err != nil { return err } if refresher, ok := autodiscovery.(ManagedRuntimeRefresher); ok { diff --git a/cmd/launch/launch_test.go b/cmd/launch/launch_test.go index 14b21331..9b63dcdf 100644 --- a/cmd/launch/launch_test.go +++ b/cmd/launch/launch_test.go @@ -1628,14 +1628,6 @@ func TestLaunchIntegration_EditorForceConfigure(t *testing.T) { return []string{"llama3.2", "qwen3:8b"}, nil } - var proceedPrompt bool - DefaultConfirmPrompt = func(prompt string, options ConfirmOptions) (bool, error) { - if prompt == "Proceed?" { - proceedPrompt = true - } - return true, nil - } - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.URL.Path { case "/api/experimental/model-recommendations": @@ -1663,9 +1655,6 @@ func TestLaunchIntegration_EditorForceConfigure(t *testing.T) { if !multiCalled { t.Fatal("expected multi selector to be used for forced editor configure") } - if !proceedPrompt { - t.Fatal("expected backup warning confirmation before edit") - } if diff := compareStringSlices(editor.edited, [][]string{{"llama3.2", "qwen3:8b"}}); diff != "" { t.Fatalf("unexpected edited models (-want +got):\n%s", diff) } @@ -1866,9 +1855,6 @@ func TestLaunchIntegration_EditorConfigureMultiSkipsMissingLocalAndPersistsAccep return []string{"glm-5:cloud", "missing-local"}, nil } DefaultConfirmPrompt = func(prompt string, options ConfirmOptions) (bool, error) { - if prompt == "Proceed?" { - return true, nil - } if prompt == "Download missing-local?" { return false, nil } @@ -1950,9 +1936,6 @@ func TestLaunchIntegration_EditorConfigureMultiSkipsUnauthedCloudAndPersistsAcce return []string{"llama3.2", "glm-5:cloud"}, nil } DefaultConfirmPrompt = func(prompt string, options ConfirmOptions) (bool, error) { - if prompt == "Proceed?" { - return true, nil - } t.Fatalf("unexpected prompt: %q", prompt) return false, nil } @@ -2037,9 +2020,6 @@ func TestLaunchIntegration_EditorConfigureMultiRemovesReselectedFailingModel(t * return append([]string(nil), preChecked...), nil } DefaultConfirmPrompt = func(prompt string, options ConfirmOptions) (bool, error) { - if prompt == "Proceed?" { - return true, nil - } t.Fatalf("unexpected prompt: %q", prompt) return false, nil } @@ -2129,9 +2109,6 @@ func TestLaunchIntegration_EditorConfigureMultiAllFailuresKeepsExistingAndSkipsL if prompt == "Download missing-local-a?" || prompt == "Download missing-local-b?" { return false, nil } - if prompt == "Proceed?" { - t.Fatal("did not expect proceed prompt when no models are accepted") - } t.Fatalf("unexpected prompt: %q", prompt) return false, nil } @@ -2472,9 +2449,6 @@ func TestLaunchIntegration_ConfigureOnlyDoesNotRequireInstalledBinary(t *testing if editor.ranModel != "" { t.Fatalf("expected configure-only flow to skip launch, got %q", editor.ranModel) } - if !slices.Contains(prompts, "Proceed?") { - t.Fatalf("expected editor warning prompt, got %v", prompts) - } if !slices.Contains(prompts, "Launch LauncherEditor now?") { t.Fatalf("expected configure-only launch prompt, got %v", prompts) } diff --git a/cmd/launch/models.go b/cmd/launch/models.go index a3911bf5..619de47f 100644 --- a/cmd/launch/models.go +++ b/cmd/launch/models.go @@ -16,7 +16,6 @@ import ( "github.com/ollama/ollama/api" "github.com/ollama/ollama/cmd/config" - "github.com/ollama/ollama/cmd/internal/fileutil" "github.com/ollama/ollama/format" internalcloud "github.com/ollama/ollama/internal/cloud" "github.com/ollama/ollama/internal/modelref" @@ -300,12 +299,7 @@ func pullMissingModel(ctx context.Context, client *api.Client, model string) err } // prepareEditorIntegration persists models and applies editor-managed config files. -func prepareEditorIntegration(name string, runner Runner, editor Editor, models []string) error { - if ok, err := confirmConfigEdit(runner, editor.Paths()); err != nil { - return err - } else if !ok { - return errCancelled - } +func prepareEditorIntegration(name string, editor Editor, models []string) error { if err := editor.Edit(models); err != nil { return fmt.Errorf("setup failed: %w", err) } @@ -315,12 +309,7 @@ func prepareEditorIntegration(name string, runner Runner, editor Editor, models return nil } -func prepareManagedSingleIntegration(name string, runner Runner, managed ManagedSingleModel, model string, models []string) error { - if ok, err := confirmConfigEdit(runner, managed.Paths()); err != nil { - return err - } else if !ok { - return errCancelled - } +func prepareManagedSingleIntegration(name string, managed ManagedSingleModel, model string, models []string) error { models = dedupeModelList(append([]string{model}, models...)) var err error if withModels, ok := managed.(ManagedModelListConfigurer); ok { @@ -337,12 +326,7 @@ func prepareManagedSingleIntegration(name string, runner Runner, managed Managed return nil } -func prepareManagedAutodiscoveryIntegration(name string, runner Runner, autodiscovery ManagedAutodiscoveryIntegration, model string) error { - if ok, err := confirmConfigEdit(runner, autodiscovery.Paths()); err != nil { - return err - } else if !ok { - return errCancelled - } +func prepareManagedAutodiscoveryIntegration(name string, autodiscovery ManagedAutodiscoveryIntegration, model string) error { if err := autodiscovery.ConfigureAutodiscovery(); err != nil { return fmt.Errorf("setup failed: %w", err) } @@ -352,20 +336,6 @@ func prepareManagedAutodiscoveryIntegration(name string, runner Runner, autodisc return nil } -func confirmConfigEdit(runner Runner, paths []string) (bool, error) { - if len(paths) == 0 { - return true, nil - } - - fmt.Fprintf(os.Stderr, "This will modify your %s configuration:\n", runner) - for _, path := range paths { - fmt.Fprintf(os.Stderr, " %s\n", path) - } - fmt.Fprintf(os.Stderr, "Backups will be saved to %s/\n\n", fileutil.BackupDir()) - - return ConfirmPrompt("Proceed?") -} - // buildModelList merges existing models with recommendations for selection UIs. func buildModelList(existing []modelInfo, preChecked []string, current string) (items []ModelItem, orderedChecked []string, existingModels, cloudModels map[string]bool) { return buildModelListWithRecommendations(existing, recommendedModels, preChecked, current) diff --git a/cmd/launch/openclaw.go b/cmd/launch/openclaw.go index 5eb37072..b7ffcc77 100644 --- a/cmd/launch/openclaw.go +++ b/cmd/launch/openclaw.go @@ -753,7 +753,7 @@ func (c *Openclaw) Edit(models []string) error { if err != nil { return err } - if err := fileutil.WriteWithBackup(configPath, data); err != nil { + if err := fileutil.WriteWithBackup(configPath, data, "openclaw"); err != nil { return err } diff --git a/cmd/launch/openclaw_test.go b/cmd/launch/openclaw_test.go index cf49b15d..ce98bc4c 100644 --- a/cmd/launch/openclaw_test.go +++ b/cmd/launch/openclaw_test.go @@ -17,6 +17,7 @@ import ( "time" "github.com/ollama/ollama/api" + "github.com/ollama/ollama/cmd/internal/fileutil" ) func TestOpenclawIntegration(t *testing.T) { @@ -1154,7 +1155,7 @@ func TestOpenclawEdit_BackupCreated(t *testing.T) { setTestHome(t, tmpDir) configDir := filepath.Join(tmpDir, ".openclaw") configPath := filepath.Join(configDir, "openclaw.json") - backupDir := filepath.Join(os.TempDir(), "ollama-backups") + backupDir := fileutil.BackupDir() os.MkdirAll(configDir, 0o755) uniqueMarker := fmt.Sprintf("test-marker-%d", os.Getpid()) @@ -1165,7 +1166,7 @@ func TestOpenclawEdit_BackupCreated(t *testing.T) { t.Fatal(err) } - backups, _ := filepath.Glob(filepath.Join(backupDir, "openclaw.json.*")) + backups, _ := filepath.Glob(filepath.Join(backupDir, "openclaw", "openclaw.json.*")) foundBackup := false for _, backup := range backups { data, _ := os.ReadFile(backup) diff --git a/cmd/launch/opencode.go b/cmd/launch/opencode.go index 34a1bb25..18b04eb9 100644 --- a/cmd/launch/opencode.go +++ b/cmd/launch/opencode.go @@ -163,7 +163,7 @@ func (o *OpenCode) Edit(modelList []string) error { if err != nil { return err } - return fileutil.WriteWithBackup(statePath, stateData) + return fileutil.WriteWithBackup(statePath, stateData, "opencode") } func (o *OpenCode) Models() []string { diff --git a/cmd/launch/pi.go b/cmd/launch/pi.go index 89c5327d..05065cc2 100644 --- a/cmd/launch/pi.go +++ b/cmd/launch/pi.go @@ -272,7 +272,7 @@ func (p *Pi) Edit(models []string) error { if err != nil { return err } - if err := fileutil.WriteWithBackup(configPath, configData); err != nil { + if err := fileutil.WriteWithBackup(configPath, configData, "pi"); err != nil { return err } @@ -290,7 +290,7 @@ func (p *Pi) Edit(models []string) error { if err != nil { return err } - return fileutil.WriteWithBackup(settingsPath, settingsData) + return fileutil.WriteWithBackup(settingsPath, settingsData, "pi") } func (p *Pi) Models() []string { diff --git a/cmd/launch/pi_test.go b/cmd/launch/pi_test.go index 46dc069e..f289a047 100644 --- a/cmd/launch/pi_test.go +++ b/cmd/launch/pi_test.go @@ -14,6 +14,7 @@ import ( "testing" "github.com/ollama/ollama/api" + "github.com/ollama/ollama/cmd/internal/fileutil" "github.com/ollama/ollama/types/model" ) @@ -887,6 +888,62 @@ func TestPiEdit(t *testing.T) { }) } +func TestPiEdit_CreatesDistinctBackupsForEachManagedFile(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/show" { + fmt.Fprint(w, `{"capabilities":[],"model_info":{}}`) + return + } + w.WriteHeader(http.StatusNotFound) + })) + defer srv.Close() + t.Setenv("OLLAMA_HOST", srv.URL) + + pi := &Pi{} + tmpDir := t.TempDir() + setTestHome(t, tmpDir) + + configDir := filepath.Join(tmpDir, ".pi", "agent") + modelsPath := filepath.Join(configDir, "models.json") + settingsPath := filepath.Join(configDir, "settings.json") + backupDir := fileutil.BackupDir() + + if err := os.MkdirAll(configDir, 0o755); err != nil { + t.Fatal(err) + } + + modelsOriginal := fmt.Sprintf(`{"marker":"models-%d","providers":{"ollama":{"models":[]}}}`, os.Getpid()) + settingsOriginal := fmt.Sprintf(`{"marker":"settings-%d","defaultProvider":"other","defaultModel":"old"}`, os.Getpid()) + if err := os.WriteFile(modelsPath, []byte(modelsOriginal), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(settingsPath, []byte(settingsOriginal), 0o644); err != nil { + t.Fatal(err) + } + + if err := pi.Edit([]string{"llama3.2"}); err != nil { + t.Fatalf("Edit() error = %v", err) + } + + assertBackupMatches := func(pattern, want string) { + t.Helper() + backups, err := filepath.Glob(filepath.Join(backupDir, pattern)) + if err != nil { + t.Fatalf("glob %q failed: %v", pattern, err) + } + for _, backup := range backups { + data, err := os.ReadFile(backup) + if err == nil && string(data) == want { + return + } + } + t.Fatalf("backup matching %q with expected content not found", pattern) + } + + assertBackupMatches(filepath.Join("pi", "models.json.*"), modelsOriginal) + assertBackupMatches(filepath.Join("pi", "settings.json.*"), settingsOriginal) +} + func TestPiModels(t *testing.T) { pi := &Pi{} diff --git a/cmd/launch/vscode.go b/cmd/launch/vscode.go index 5f81d53f..495f00f5 100644 --- a/cmd/launch/vscode.go +++ b/cmd/launch/vscode.go @@ -273,7 +273,7 @@ func (v *VSCode) Edit(models []string) error { if err != nil { return err } - if err := fileutil.WriteWithBackup(clmPath, data); err != nil { + if err := fileutil.WriteWithBackup(clmPath, data, "vscode"); err != nil { return err } @@ -350,7 +350,7 @@ func (v *VSCode) updateSettings() { if err != nil { return } - _ = fileutil.WriteWithBackup(settingsPath, updated) + _ = fileutil.WriteWithBackup(settingsPath, updated, "vscode") } func (v *VSCode) statePath() string { diff --git a/cmd/launch/vscode_test.go b/cmd/launch/vscode_test.go index 2cec3b3d..4a3dc2cb 100644 --- a/cmd/launch/vscode_test.go +++ b/cmd/launch/vscode_test.go @@ -9,6 +9,7 @@ import ( "testing" _ "github.com/mattn/go-sqlite3" + "github.com/ollama/ollama/cmd/internal/fileutil" ) func TestVSCodeIntegration(t *testing.T) { @@ -156,6 +157,52 @@ func TestVSCodeEditCleansUpOldSettings(t *testing.T) { } } +func TestVSCodeEdit_CreatesDistinctBackupsForManagedFiles(t *testing.T) { + v := &VSCode{} + tmpDir := t.TempDir() + setTestHome(t, tmpDir) + t.Setenv("XDG_CONFIG_HOME", "") + + clmPath := testVSCodePath(t, tmpDir, "chatLanguageModels.json") + settingsPath := testVSCodePath(t, tmpDir, "settings.json") + backupDir := fileutil.BackupDir() + + if err := os.MkdirAll(filepath.Dir(clmPath), 0o755); err != nil { + t.Fatal(err) + } + + clmOriginal := `[{"vendor":"ollama","name":"Ollama","url":"http://old:11434"}]` + settingsOriginal := `{"github.copilot.chat.byok.ollamaEndpoint":"http://old:11434","ollama.launch.configured":true,"editor.fontSize":14}` + if err := os.WriteFile(clmPath, []byte(clmOriginal), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(settingsPath, []byte(settingsOriginal), 0o644); err != nil { + t.Fatal(err) + } + + if err := v.Edit([]string{"llama3.2"}); err != nil { + t.Fatal(err) + } + + assertBackupMatches := func(pattern, want string) { + t.Helper() + backups, err := filepath.Glob(filepath.Join(backupDir, pattern)) + if err != nil { + t.Fatalf("glob %q failed: %v", pattern, err) + } + for _, backup := range backups { + data, err := os.ReadFile(backup) + if err == nil && string(data) == want { + return + } + } + t.Fatalf("backup matching %q with expected content not found", pattern) + } + + assertBackupMatches(filepath.Join("vscode", "chatLanguageModels.json.*"), clmOriginal) + assertBackupMatches(filepath.Join("vscode", "settings.json.*"), settingsOriginal) +} + func TestVSCodePaths(t *testing.T) { v := &VSCode{} tmpDir := t.TempDir()