From c45fef913fdb977b8bbe1dfc2e647b73e4fddced Mon Sep 17 00:00:00 2001 From: ParthSareen Date: Thu, 7 May 2026 15:22:14 -0700 Subject: [PATCH] fix(cmd): harden codex app config writes --- cmd/launch/codex.go | 115 +++++++++++-- cmd/launch/codex_app.go | 186 +++++++++++++++----- cmd/launch/codex_app_test.go | 322 +++++++++++++++++++++++++++++++++-- cmd/launch/codex_test.go | 67 ++++++++ 4 files changed, 622 insertions(+), 68 deletions(-) diff --git a/cmd/launch/codex.go b/cmd/launch/codex.go index 114159177..28b9b5ac2 100644 --- a/cmd/launch/codex.go +++ b/cmd/launch/codex.go @@ -81,14 +81,19 @@ func writeCodexProfile(configPath string) error { type codexLaunchProfileOptions struct { activate bool + profileName string forceAPIAuth bool setRootModelConfig bool model string modelCatalogPath string + backupIntegration string } func writeCodexLaunchProfile(configPath string, opts codexLaunchProfileOptions) error { baseURL := codexBaseURL() + profileName := codexLaunchProfileName(opts) + profileHeader := codexProfileHeaderFor(profileName) + providerHeader := codexProviderHeaderFor(profileName) content, readErr := os.ReadFile(configPath) text := "" @@ -103,11 +108,11 @@ func writeCodexLaunchProfile(configPath string, opts codexLaunchProfileOptions) model := strings.TrimSpace(opts.model) if model == "" { - model = codexSectionStringValue(text, codexProfileHeader(), "model") + model = codexSectionStringValue(text, profileHeader, "model") } modelCatalogPath := strings.TrimSpace(opts.modelCatalogPath) if modelCatalogPath == "" { - modelCatalogPath = codexSectionStringValue(text, codexProfileHeader(), "model_catalog_json") + modelCatalogPath = codexSectionStringValue(text, profileHeader, "model_catalog_json") } profileLines := []string{} @@ -116,7 +121,7 @@ func writeCodexLaunchProfile(configPath string, opts codexLaunchProfileOptions) } profileLines = append(profileLines, fmt.Sprintf("openai_base_url = %q", baseURL), - fmt.Sprintf("model_provider = %q", codexProfileName), + fmt.Sprintf("model_provider = %q", profileName), ) if opts.forceAPIAuth { profileLines = append(profileLines, `forced_login_method = "api"`) @@ -130,11 +135,11 @@ func writeCodexLaunchProfile(configPath string, opts codexLaunchProfileOptions) lines []string }{ { - header: codexProfileHeader(), + header: profileHeader, lines: profileLines, }, { - header: codexProviderHeader(), + header: providerHeader, lines: []string{ fmt.Sprintf("name = %q", codexProviderName), fmt.Sprintf("base_url = %q", baseURL), @@ -144,13 +149,13 @@ func writeCodexLaunchProfile(configPath string, opts codexLaunchProfileOptions) } if opts.activate { - text = codexSetRootStringValue(text, "profile", codexProfileName) + text = codexSetRootStringValue(text, "profile", profileName) } if opts.setRootModelConfig { if model != "" { text = codexSetRootStringValue(text, "model", model) } - text = codexSetRootStringValue(text, "model_provider", codexProfileName) + text = codexSetRootStringValue(text, "model_provider", profileName) if modelCatalogPath != "" { text = codexSetRootStringValue(text, "model_catalog_json", modelCatalogPath) } @@ -162,11 +167,21 @@ func writeCodexLaunchProfile(configPath string, opts codexLaunchProfileOptions) if err := codexValidateConfigText(text); err != nil { return err } + if err := codexValidateLaunchProfileText(text, profileName, opts, model, modelCatalogPath, baseURL); err != nil { + return err + } if err := os.MkdirAll(filepath.Dir(configPath), 0o755); err != nil { return err } - return fileutil.WriteWithBackup(configPath, []byte(text)) + return codexWriteWithBackup(configPath, []byte(text), opts.backupIntegration) +} + +func codexLaunchProfileName(opts codexLaunchProfileOptions) string { + if name := strings.TrimSpace(opts.profileName); name != "" { + return name + } + return codexProfileName } func codexBaseURL() string { @@ -174,11 +189,79 @@ func codexBaseURL() string { } func codexProfileHeader() string { - return fmt.Sprintf("[profiles.%s]", codexProfileName) + return codexProfileHeaderFor(codexProfileName) } func codexProviderHeader() string { - return fmt.Sprintf("[model_providers.%s]", codexProfileName) + return codexProviderHeaderFor(codexProfileName) +} + +func codexProfileHeaderFor(profileName string) string { + return fmt.Sprintf("[profiles.%s]", profileName) +} + +func codexProviderHeaderFor(profileName string) string { + return fmt.Sprintf("[model_providers.%s]", profileName) +} + +func codexValidateLaunchProfileText(text, profileName string, opts codexLaunchProfileOptions, model, modelCatalogPath, baseURL string) error { + for _, check := range []struct { + path []string + want string + }{ + {[]string{"profiles", profileName, "openai_base_url"}, baseURL}, + {[]string{"profiles", profileName, "model_provider"}, profileName}, + {[]string{"model_providers", profileName, "name"}, codexProviderName}, + {[]string{"model_providers", profileName, "base_url"}, baseURL}, + {[]string{"model_providers", profileName, "wire_api"}, "responses"}, + } { + if got, ok := codexStringValue(text, check.path...); !ok || got != check.want { + return fmt.Errorf("generated Codex config missing %s = %q", strings.Join(check.path, "."), check.want) + } + } + if opts.forceAPIAuth { + if got, ok := codexStringValue(text, "profiles", profileName, "forced_login_method"); !ok || got != "api" { + return fmt.Errorf("generated Codex config missing profiles.%s.forced_login_method = %q", profileName, "api") + } + } + if model != "" { + if got, ok := codexStringValue(text, "profiles", profileName, "model"); !ok || got != model { + return fmt.Errorf("generated Codex config missing profiles.%s.model = %q", profileName, model) + } + } + if modelCatalogPath != "" { + if got, ok := codexStringValue(text, "profiles", profileName, "model_catalog_json"); !ok || got != modelCatalogPath { + return fmt.Errorf("generated Codex config missing profiles.%s.model_catalog_json = %q", profileName, modelCatalogPath) + } + } + if opts.activate { + if got := codexRootStringValue(text, "profile"); got != profileName { + return fmt.Errorf("generated Codex config missing profile = %q", profileName) + } + } + if opts.setRootModelConfig { + if model != "" { + if got := codexRootStringValue(text, "model"); got != model { + return fmt.Errorf("generated Codex config missing model = %q", model) + } + } + if got := codexRootStringValue(text, "model_provider"); got != profileName { + return fmt.Errorf("generated Codex config missing model_provider = %q", profileName) + } + if modelCatalogPath != "" { + if got := codexRootStringValue(text, "model_catalog_json"); got != modelCatalogPath { + return fmt.Errorf("generated Codex config missing model_catalog_json = %q", modelCatalogPath) + } + } + } + return nil +} + +func codexWriteWithBackup(path string, data []byte, integration string) error { + if strings.TrimSpace(integration) != "" { + return fileutil.WriteWithBackup(path, data, integration) + } + return fileutil.WriteWithBackup(path, data) } func codexUpsertSection(text, header string, lines []string) string { @@ -199,6 +282,18 @@ func codexUpsertSection(text, header string, lines []string) string { return text + block } +func codexRemoveSection(text, header string) string { + targetPath, ok := codexTableHeaderPath(header) + if !ok { + return text + } + start, end, found := codexSectionRange(text, targetPath) + if !found { + return text + } + return text[:start] + text[end:] +} + func codexRootStringValue(text, key string) string { value, _ := codexStringValue(text, key) return value diff --git a/cmd/launch/codex_app.go b/cmd/launch/codex_app.go index c4f377fc0..d08ca7e3c 100644 --- a/cmd/launch/codex_app.go +++ b/cmd/launch/codex_app.go @@ -16,12 +16,12 @@ import ( "github.com/ollama/ollama/api" "github.com/ollama/ollama/cmd/config" - "github.com/ollama/ollama/cmd/internal/fileutil" "github.com/ollama/ollama/envconfig" ) const ( codexAppIntegrationName = "codex-app" + codexAppProfileName = "ollama-launch-codex-app" codexAppBundleID = "com.openai.codex" codexAppModelCatalogFilename = "ollama-launch-models.json" codexAppRestoreHint = "To restore your usual Codex profile, run: ollama launch codex-app --restore" @@ -85,9 +85,11 @@ func (c *CodexApp) ConfigureWithModels(primary string, models []string) error { } return writeCodexLaunchProfile(configPath, codexLaunchProfileOptions{ activate: true, + profileName: codexAppProfileName, setRootModelConfig: true, model: primary, modelCatalogPath: catalogPath, + backupIntegration: codexAppIntegrationName, }) } @@ -101,23 +103,44 @@ func (c *CodexApp) CurrentModel() string { return "" } text := string(data) - if codexRootStringValue(text, "model_provider") == codexProfileName { - baseURL := codexSectionStringValue(text, codexProviderHeader(), "base_url") - if codexNormalizeURL(baseURL) == codexNormalizeURL(codexBaseURL()) { - return strings.TrimSpace(codexRootStringValue(text, "model")) + for _, profileName := range codexAppManagedProfileNames() { + if codexRootStringValue(text, "model_provider") == profileName { + baseURL := codexSectionStringValue(text, codexProviderHeaderFor(profileName), "base_url") + if codexNormalizeURL(baseURL) == codexNormalizeURL(codexBaseURL()) { + return strings.TrimSpace(codexRootStringValue(text, "model")) + } } } - if codexRootStringValue(text, "profile") != codexProfileName { + + profileName := codexRootStringValue(text, "profile") + if !codexAppIsManagedProfileName(profileName) { return "" } - if codexSectionStringValue(text, codexProfileHeader(), "model_provider") != codexProfileName { + if codexSectionStringValue(text, codexProfileHeaderFor(profileName), "model_provider") != profileName { return "" } - baseURL := codexSectionStringValue(text, codexProviderHeader(), "base_url") + baseURL := codexSectionStringValue(text, codexProviderHeaderFor(profileName), "base_url") if codexNormalizeURL(baseURL) != codexNormalizeURL(codexBaseURL()) { return "" } - return strings.TrimSpace(codexSectionStringValue(text, codexProfileHeader(), "model")) + return strings.TrimSpace(codexSectionStringValue(text, codexProfileHeaderFor(profileName), "model")) +} + +func codexAppManagedProfileNames() []string { + return []string{codexAppProfileName, codexProfileName} +} + +func codexAppIsManagedProfileName(profileName string) bool { + for _, candidate := range codexAppManagedProfileNames() { + if profileName == candidate { + return true + } + } + return false +} + +func codexAppIsOwnedProfileName(profileName string) bool { + return profileName == codexAppProfileName } func (c *CodexApp) Onboard() error { @@ -173,26 +196,23 @@ func (c *CodexApp) Restore() error { state, stateErr := loadCodexAppRestoreState() if stateErr == nil { - text = codexRestoreRootStringValue(text, "profile", state.HadProfile, state.Profile) - text = codexRestoreRootStringValue(text, "model", state.HadModel, state.Model) - text = codexRestoreRootStringValue(text, "model_provider", state.HadModelProvider, state.ModelProvider) - text = codexRestoreRootStringValue(text, "model_catalog_json", state.HadModelCatalogJSON, state.ModelCatalogJSON) - } else if codexRootStringValue(text, "profile") == codexProfileName { - text = codexRemoveRootValue(text, "profile") - if codexRootStringValue(text, "model_provider") == codexProfileName { - text = codexRemoveRootValue(text, "model_provider") - } - if catalogPath, err := codexAppModelCatalogPath(); err == nil && codexRootStringValue(text, "model_catalog_json") == catalogPath { - text = codexRemoveRootValue(text, "model_catalog_json") - } + text = codexAppRestoreRootValues(text, state) + } else if os.IsNotExist(stateErr) { + text = codexAppRemoveOwnedRootValues(text) + } else { + return stateErr + } + if !codexAppRootReferencesOwnedConfig(text) { + text = codexAppRemoveOwnedSections(text) } if err := codexValidateConfigText(text); err != nil { return err } - if err := fileutil.WriteWithBackup(configPath, []byte(text)); err != nil { + if err := codexWriteWithBackup(configPath, []byte(text), codexAppIntegrationName); err != nil { return err } + codexAppRemoveOwnedCatalogIfUnused(text) _ = os.Remove(codexAppRestoreStatePath()) return codexAppLaunchOrRestart("Restart Codex to use your usual profile?") } @@ -246,7 +266,7 @@ func writeCodexAppModelCatalog(path string, models []string) error { if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { return err } - return fileutil.WriteWithBackup(path, append(data, '\n')) + return codexWriteWithBackup(path, append(data, '\n'), codexAppIntegrationName) } func codexAppCatalogModelNames(primary string, fallback []string) []string { @@ -711,6 +731,69 @@ func codexNormalizeURL(raw string) string { return strings.TrimRight(strings.TrimSpace(raw), "/") } +func codexAppRootStillManaged(text string) bool { + if codexAppIsOwnedProfileName(codexRootStringValue(text, "profile")) { + return true + } + if codexAppIsOwnedProfileName(codexRootStringValue(text, "model_provider")) { + return true + } + return false +} + +func codexAppRootReferencesOwnedConfig(text string) bool { + return codexRootStringValue(text, "profile") == codexAppProfileName || + codexRootStringValue(text, "model_provider") == codexAppProfileName +} + +func codexAppRootReferencesCatalog(text string) bool { + catalogPath, err := codexAppModelCatalogPath() + if err != nil { + return false + } + return codexRootStringValue(text, "model_catalog_json") == catalogPath +} + +func codexAppRemoveOwnedSections(text string) string { + text = codexRemoveSection(text, codexProfileHeaderFor(codexAppProfileName)) + text = codexRemoveSection(text, codexProviderHeaderFor(codexAppProfileName)) + return text +} + +func codexAppRemoveOwnedCatalogIfUnused(text string) { + if codexAppRootReferencesCatalog(text) { + return + } + if catalogPath, err := codexAppModelCatalogPath(); err == nil { + _ = os.Remove(catalogPath) + } +} + +func codexAppRemoveOwnedRootValues(text string) string { + if !codexAppRootStillManaged(text) { + return text + } + text = codexRemoveRootValue(text, "profile") + if codexAppIsOwnedProfileName(codexRootStringValue(text, "model_provider")) { + text = codexRemoveRootValue(text, "model_provider") + } + if catalogPath, err := codexAppModelCatalogPath(); err == nil && codexRootStringValue(text, "model_catalog_json") == catalogPath { + text = codexRemoveRootValue(text, "model_catalog_json") + } + return text +} + +func codexAppRestoreRootValues(text string, state codexAppRestoreState) string { + if !codexAppRootStillManaged(text) { + return text + } + text = codexRestoreRootStringValue(text, "profile", state.HadProfile, state.Profile) + text = codexRestoreRootStringValue(text, "model", state.HadModel, state.Model) + text = codexRestoreRootStringValue(text, "model_provider", state.HadModelProvider, state.ModelProvider) + text = codexRestoreRootStringValue(text, "model_catalog_json", state.HadModelCatalogJSON, state.ModelCatalogJSON) + return text +} + type codexAppRestoreState struct { HadProfile bool `json:"had_profile"` Profile string `json:"profile,omitempty"` @@ -723,21 +806,35 @@ type codexAppRestoreState struct { } func saveCodexAppRestoreState(configPath string) error { - statePath := codexAppRestoreStatePath() - if stateData, err := os.ReadFile(statePath); err == nil { - if codexAppRestoreStateHasRootConfig(stateData) { - return nil - } - configData, err := os.ReadFile(configPath) - if err != nil { - if os.IsNotExist(err) { - return nil - } + configText := "" + configExists := false + if configData, err := os.ReadFile(configPath); err == nil { + configText = string(configData) + if err := codexValidateConfigText(configText); err != nil { return err } + configExists = true + } else if !os.IsNotExist(err) { + return err + } + + statePath := codexAppRestoreStatePath() + if stateData, err := os.ReadFile(statePath); err == nil { + hasRootConfig, err := codexAppRestoreStateHasRootConfig(stateData) + if err != nil { + return err + } + if hasRootConfig { + return nil + } + if !configExists { + return nil + } var existing codexAppRestoreState - _ = json.Unmarshal(stateData, &existing) - upgraded := codexAppRestoreStateFromText(string(configData)) + if err := json.Unmarshal(stateData, &existing); err != nil { + return err + } + upgraded := codexAppRestoreStateFromText(configText) upgraded.HadProfile = existing.HadProfile upgraded.Profile = existing.Profile return writeCodexAppRestoreState(upgraded) @@ -745,26 +842,21 @@ func saveCodexAppRestoreState(configPath string) error { return err } - data, err := os.ReadFile(configPath) - if err != nil { - if os.IsNotExist(err) { - return writeCodexAppRestoreState(codexAppRestoreState{}) - } - return err + if !configExists { + return writeCodexAppRestoreState(codexAppRestoreState{}) } - - return writeCodexAppRestoreState(codexAppRestoreStateFromText(string(data))) + return writeCodexAppRestoreState(codexAppRestoreStateFromText(configText)) } -func codexAppRestoreStateHasRootConfig(data []byte) bool { +func codexAppRestoreStateHasRootConfig(data []byte) (bool, error) { var raw map[string]json.RawMessage if err := json.Unmarshal(data, &raw); err != nil { - return true + return false, err } _, hasModel := raw["had_model"] _, hasModelProvider := raw["had_model_provider"] _, hasModelCatalogJSON := raw["had_model_catalog_json"] - return hasModel && hasModelProvider && hasModelCatalogJSON + return hasModel && hasModelProvider && hasModelCatalogJSON, nil } func codexAppRestoreStateFromText(text string) codexAppRestoreState { @@ -800,7 +892,7 @@ func writeCodexAppRestoreState(state codexAppRestoreState) error { if err != nil { return err } - return fileutil.WriteWithBackup(path, data) + return codexWriteWithBackup(path, data, codexAppIntegrationName) } func loadCodexAppRestoreState() (codexAppRestoreState, error) { diff --git a/cmd/launch/codex_app_test.go b/cmd/launch/codex_app_test.go index 5d5aed2f8..302e02c6f 100644 --- a/cmd/launch/codex_app_test.go +++ b/cmd/launch/codex_app_test.go @@ -9,6 +9,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/ollama/ollama/cmd/internal/fileutil" ) func withCodexAppPlatform(t *testing.T, goos string) { @@ -161,16 +163,16 @@ func TestCodexAppConfigureActivatesOllamaProfile(t *testing.T) { } for _, want := range []string{ - `profile = "ollama-launch"`, + fmt.Sprintf(`profile = %q`, codexAppProfileName), `model = "llama3.2"`, - `model_provider = "ollama-launch"`, + fmt.Sprintf(`model_provider = %q`, codexAppProfileName), fmt.Sprintf(`model_catalog_json = %q`, catalogPath), - `[profiles.ollama-launch]`, + codexProfileHeaderFor(codexAppProfileName), `model = "llama3.2"`, `openai_base_url = "http://127.0.0.1:9999/v1/"`, - `model_provider = "ollama-launch"`, + fmt.Sprintf(`model_provider = %q`, codexAppProfileName), `model_catalog_json = "`, - `[model_providers.ollama-launch]`, + codexProviderHeaderFor(codexAppProfileName), `name = "Ollama"`, `base_url = "http://127.0.0.1:9999/v1/"`, `wire_api = "responses"`, @@ -206,6 +208,134 @@ func TestCodexAppConfigureActivatesOllamaProfile(t *testing.T) { } } +func TestCodexAppConfigureUsesAppSpecificProfileWithoutTouchingCLIProfile(t *testing.T) { + tmpDir := t.TempDir() + setTestHome(t, tmpDir) + t.Setenv("OLLAMA_HOST", "http://127.0.0.1:9999") + + configPath := filepath.Join(tmpDir, ".codex", "config.toml") + if err := os.MkdirAll(filepath.Dir(configPath), 0o755); err != nil { + t.Fatal(err) + } + existing := "" + + `profile = "default"` + "\n\n" + + "[profiles.ollama-launch]\n" + + `model = "cli-model"` + "\n" + + `openai_base_url = "http://cli.invalid/v1/"` + "\n" + + `model_provider = "ollama-launch"` + "\n\n" + + "[model_providers.ollama-launch]\n" + + `name = "CLI Ollama"` + "\n" + + `base_url = "http://cli.invalid/v1/"` + "\n" + + `wire_api = "responses"` + "\n\n" + + "[profiles.default]\n" + + `model = "gpt-5.5"` + "\n" + if err := os.WriteFile(configPath, []byte(existing), 0o644); err != nil { + t.Fatal(err) + } + + if err := (&CodexApp{}).ConfigureWithModels("llama3.2", []string{"llama3.2"}); err != nil { + t.Fatalf("ConfigureWithModels returned error: %v", err) + } + + data, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + content := string(data) + if got := codexRootStringValue(content, "profile"); got != codexAppProfileName { + t.Fatalf("root profile = %q, want %q", got, codexAppProfileName) + } + if got := codexSectionStringValue(content, codexProfileHeader(), "openai_base_url"); got != "http://cli.invalid/v1/" { + t.Fatalf("CLI profile base URL = %q, want preserved CLI URL in:\n%s", got, content) + } + if got := codexSectionStringValue(content, codexProviderHeader(), "name"); got != "CLI Ollama" { + t.Fatalf("CLI provider name = %q, want preserved CLI provider in:\n%s", got, content) + } + if got := codexSectionStringValue(content, codexProfileHeaderFor(codexAppProfileName), "model"); got != "llama3.2" { + t.Fatalf("app profile model = %q, want llama3.2", got) + } + if got := codexSectionStringValue(content, codexProviderHeaderFor(codexAppProfileName), "base_url"); got != "http://127.0.0.1:9999/v1/" { + t.Fatalf("app provider base URL = %q", got) + } + assertBackupContains(t, filepath.Join(fileutil.BackupDir(), codexAppIntegrationName, "config.toml.*"), `profile = "default"`) +} + +func TestCodexAppConfigureRejectsMalformedTomlBeforeSideEffects(t *testing.T) { + tmpDir := t.TempDir() + setTestHome(t, tmpDir) + + configPath := filepath.Join(tmpDir, ".codex", "config.toml") + if err := os.MkdirAll(filepath.Dir(configPath), 0o755); err != nil { + t.Fatal(err) + } + existing := "profile = \n" + if err := os.WriteFile(configPath, []byte(existing), 0o644); err != nil { + t.Fatal(err) + } + + err := (&CodexApp{}).ConfigureWithModels("llama3.2", []string{"llama3.2"}) + if err == nil || !strings.Contains(err.Error(), "invalid Codex config TOML") { + t.Fatalf("ConfigureWithModels error = %v, want invalid TOML", err) + } + data, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + if string(data) != existing { + t.Fatalf("malformed config should be left untouched, got:\n%s", data) + } + if _, err := os.Stat(codexAppRestoreStatePath()); !os.IsNotExist(err) { + t.Fatalf("restore state should not be written before config validation, err=%v", err) + } + catalogPath, err := codexAppModelCatalogPath() + if err != nil { + t.Fatal(err) + } + if _, err := os.Stat(catalogPath); !os.IsNotExist(err) { + t.Fatalf("model catalog should not be written before config validation, err=%v", err) + } +} + +func TestCodexAppConfigureRejectsMalformedTomlEvenWithExistingRestoreState(t *testing.T) { + tmpDir := t.TempDir() + setTestHome(t, tmpDir) + + configPath := filepath.Join(tmpDir, ".codex", "config.toml") + if err := os.MkdirAll(filepath.Dir(configPath), 0o755); err != nil { + t.Fatal(err) + } + existing := "[profiles.ollama-launch\nmodel = \"llama3.2\"\n" + if err := os.WriteFile(configPath, []byte(existing), 0o644); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(codexAppRestoreStatePath()), 0o755); err != nil { + t.Fatal(err) + } + restoreState := `{"had_profile":true,"profile":"default","had_model":true,"model":"gpt-5.5","had_model_provider":true,"model_provider":"openai","had_model_catalog_json":false}` + if err := os.WriteFile(codexAppRestoreStatePath(), []byte(restoreState), 0o644); err != nil { + t.Fatal(err) + } + + err := (&CodexApp{}).ConfigureWithModels("llama3.2", []string{"llama3.2"}) + if err == nil || !strings.Contains(err.Error(), "invalid Codex config TOML") { + t.Fatalf("ConfigureWithModels error = %v, want invalid TOML", err) + } + data, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + if string(data) != existing { + t.Fatalf("malformed config should be left untouched, got:\n%s", data) + } + stateData, err := os.ReadFile(codexAppRestoreStatePath()) + if err != nil { + t.Fatal(err) + } + if string(stateData) != restoreState { + t.Fatalf("restore state should be left untouched, got:\n%s", stateData) + } +} + func TestCodexAppCurrentModelRequiresManagedActiveProfile(t *testing.T) { tmpDir := t.TempDir() setTestHome(t, tmpDir) @@ -217,10 +347,10 @@ func TestCodexAppCurrentModelRequiresManagedActiveProfile(t *testing.T) { } content := "" + "profile = \"default\"\n\n" + - "[profiles.ollama-launch]\n" + + codexProfileHeaderFor(codexAppProfileName) + "\n" + "model = \"llama3.2\"\n" + - "model_provider = \"ollama-launch\"\n\n" + - "[model_providers.ollama-launch]\n" + + fmt.Sprintf("model_provider = %q\n\n", codexAppProfileName) + + codexProviderHeaderFor(codexAppProfileName) + "\n" + "base_url = \"http://127.0.0.1:11434/v1/\"\n" if err := os.WriteFile(configPath, []byte(content), 0o644); err != nil { t.Fatal(err) @@ -242,8 +372,8 @@ func TestCodexAppCurrentModelReadsManagedRootConfig(t *testing.T) { } content := "" + `model = "qwen3:8b"` + "\n" + - `model_provider = "ollama-launch"` + "\n\n" + - "[model_providers.ollama-launch]\n" + + fmt.Sprintf(`model_provider = %q`, codexAppProfileName) + "\n\n" + + codexProviderHeaderFor(codexAppProfileName) + "\n" + `base_url = "http://127.0.0.1:11434/v1/"` + "\n" if err := os.WriteFile(configPath, []byte(content), 0o644); err != nil { t.Fatal(err) @@ -410,10 +540,13 @@ func TestCodexAppRestoreRestoresPreviousProfile(t *testing.T) { if err != nil { t.Fatal(err) } - if !strings.Contains(string(data), `profile = "default"`) || strings.Contains(string(data), `profile = "ollama-launch"`) { + if !strings.Contains(string(data), `profile = "default"`) || strings.Contains(string(data), fmt.Sprintf(`profile = %q`, codexAppProfileName)) { t.Fatalf("restore should restore previous active profile, got:\n%s", data) } restored := string(data) + if strings.Contains(restored, codexProfileHeaderFor(codexAppProfileName)) || strings.Contains(restored, codexProviderHeaderFor(codexAppProfileName)) { + t.Fatalf("restore should remove owned app sections, got:\n%s", restored) + } for key, want := range map[string]string{ "profile": "default", "model": "gpt-5.5", @@ -432,6 +565,173 @@ func TestCodexAppRestoreRestoresPreviousProfile(t *testing.T) { } } +func TestCodexAppRestoreRejectsMalformedTomlWithoutWriting(t *testing.T) { + tmpDir := t.TempDir() + setTestHome(t, tmpDir) + withCodexAppPlatform(t, "darwin") + + configPath := filepath.Join(tmpDir, ".codex", "config.toml") + if err := os.MkdirAll(filepath.Dir(configPath), 0o755); err != nil { + t.Fatal(err) + } + existing := "model = \"unterminated\n" + if err := os.WriteFile(configPath, []byte(existing), 0o644); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(codexAppRestoreStatePath()), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(codexAppRestoreStatePath(), []byte(`{"had_profile":false,"had_model":false,"had_model_provider":false,"had_model_catalog_json":false}`), 0o644); err != nil { + t.Fatal(err) + } + + err := (&CodexApp{}).Restore() + if err == nil || !strings.Contains(err.Error(), "invalid Codex config TOML") { + t.Fatalf("Restore error = %v, want invalid TOML", err) + } + data, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + if string(data) != existing { + t.Fatalf("malformed config should be left untouched, got:\n%s", data) + } + if _, err := os.Stat(codexAppRestoreStatePath()); err != nil { + t.Fatalf("restore state should remain after failed restore: %v", err) + } +} + +func TestCodexAppRestoreDoesNotStompUserChangedRootConfig(t *testing.T) { + tmpDir := t.TempDir() + setTestHome(t, tmpDir) + withCodexAppPlatform(t, "darwin") + + var openCalls int + withCodexAppProcessHooks(t, + func() bool { return false }, + func() error { return nil }, + func() error { + openCalls++ + return nil + }, + ) + + configPath := filepath.Join(tmpDir, ".codex", "config.toml") + if err := os.MkdirAll(filepath.Dir(configPath), 0o755); err != nil { + t.Fatal(err) + } + catalogPath, err := codexAppModelCatalogPath() + if err != nil { + t.Fatal(err) + } + existing := "" + + `profile = "manual"` + "\n" + + `model = "gpt-5.5"` + "\n" + + `model_provider = "openai"` + "\n\n" + + codexProfileHeaderFor(codexAppProfileName) + "\n" + + `model = "llama3.2"` + "\n" + + fmt.Sprintf(`model_catalog_json = %q`, catalogPath) + "\n\n" + + codexProviderHeaderFor(codexAppProfileName) + "\n" + + `base_url = "http://127.0.0.1:11434/v1/"` + "\n\n" + + "[profiles.manual]\n" + + `model = "gpt-5.5"` + "\n" + if err := os.WriteFile(configPath, []byte(existing), 0o644); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(catalogPath), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(catalogPath, []byte(`{"models":[]}`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(codexAppRestoreStatePath()), 0o755); err != nil { + t.Fatal(err) + } + restoreState := `{"had_profile":true,"profile":"default","had_model":true,"model":"old","had_model_provider":true,"model_provider":"old-provider","had_model_catalog_json":false}` + if err := os.WriteFile(codexAppRestoreStatePath(), []byte(restoreState), 0o644); err != nil { + t.Fatal(err) + } + + if err := (&CodexApp{}).Restore(); err != nil { + t.Fatalf("Restore returned error: %v", err) + } + + data, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + content := string(data) + for key, want := range map[string]string{ + "profile": "manual", + "model": "gpt-5.5", + "model_provider": "openai", + } { + if got := codexRootStringValue(content, key); got != want { + t.Fatalf("root %s = %q, want %q in:\n%s", key, got, want, content) + } + } + if strings.Contains(content, codexProfileHeaderFor(codexAppProfileName)) || strings.Contains(content, codexProviderHeaderFor(codexAppProfileName)) { + t.Fatalf("owned app sections should be removed when no longer active, got:\n%s", content) + } + if _, err := os.Stat(catalogPath); !os.IsNotExist(err) { + t.Fatalf("owned catalog should be removed when unused, err=%v", err) + } + if openCalls != 1 { + t.Fatalf("open calls = %d, want 1", openCalls) + } +} + +func TestCodexAppRestoreDoesNotTreatCLIProfileAsOwned(t *testing.T) { + tmpDir := t.TempDir() + setTestHome(t, tmpDir) + withCodexAppPlatform(t, "darwin") + + withCodexAppProcessHooks(t, + func() bool { return false }, + func() error { return nil }, + func() error { return nil }, + ) + + configPath := filepath.Join(tmpDir, ".codex", "config.toml") + if err := os.MkdirAll(filepath.Dir(configPath), 0o755); err != nil { + t.Fatal(err) + } + existing := "" + + `profile = "ollama-launch"` + "\n" + + `model = "cli-model"` + "\n" + + `model_provider = "ollama-launch"` + "\n\n" + + "[profiles.ollama-launch]\n" + + `model = "cli-model"` + "\n" + + `openai_base_url = "http://cli.invalid/v1/"` + "\n" + + `model_provider = "ollama-launch"` + "\n\n" + + "[model_providers.ollama-launch]\n" + + `name = "CLI Ollama"` + "\n" + + `base_url = "http://cli.invalid/v1/"` + "\n" + + `wire_api = "responses"` + "\n" + if err := os.WriteFile(configPath, []byte(existing), 0o644); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(codexAppRestoreStatePath()), 0o755); err != nil { + t.Fatal(err) + } + restoreState := `{"had_profile":true,"profile":"default","had_model":true,"model":"gpt-5.5","had_model_provider":true,"model_provider":"openai","had_model_catalog_json":false}` + if err := os.WriteFile(codexAppRestoreStatePath(), []byte(restoreState), 0o644); err != nil { + t.Fatal(err) + } + + if err := (&CodexApp{}).Restore(); err != nil { + t.Fatalf("Restore returned error: %v", err) + } + + data, err := os.ReadFile(configPath) + if err != nil { + t.Fatal(err) + } + if string(data) != existing { + t.Fatalf("CLI Codex profile should be left untouched, got:\n%s", data) + } +} + func TestCodexAppRunRestartsRunningAppWhenConfirmed(t *testing.T) { withCodexAppPlatform(t, "darwin") restoreConfirm := withLaunchConfirmPolicy(launchConfirmPolicy{yes: true}) diff --git a/cmd/launch/codex_test.go b/cmd/launch/codex_test.go index 824e15bbd..6b8173568 100644 --- a/cmd/launch/codex_test.go +++ b/cmd/launch/codex_test.go @@ -6,6 +6,8 @@ import ( "slices" "strings" "testing" + + "github.com/ollama/ollama/cmd/internal/fileutil" ) func TestCodexArgs(t *testing.T) { @@ -177,6 +179,53 @@ func TestWriteCodexProfile(t *testing.T) { } }) + t.Run("rejects malformed existing toml variants without writing", func(t *testing.T) { + tests := map[string]string{ + "duplicate root key": "profile = \"default\"\nprofile = \"other\"\n", + "unterminated string": "model = \"gpt-5.5\n", + "bad table": "[profiles.ollama-launch\nmodel = \"llama3.2\"\n", + "duplicate table key": "[profiles.ollama-launch]\nmodel = \"a\"\nmodel = \"b\"\n", + } + for name, existing := range tests { + t.Run(name, func(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.toml") + if err := os.WriteFile(configPath, []byte(existing), 0o644); err != nil { + t.Fatal(err) + } + + err := writeCodexProfile(configPath) + if err == nil || !strings.Contains(err.Error(), "invalid Codex config TOML") { + t.Fatalf("writeCodexProfile error = %v, want invalid TOML", err) + } + + data, _ := os.ReadFile(configPath) + if string(data) != existing { + t.Fatalf("invalid config should be left untouched, got:\n%s", data) + } + }) + } + }) + + t.Run("backs up previous config before overwrite", func(t *testing.T) { + tmpDir := t.TempDir() + setTestHome(t, tmpDir) + configPath := filepath.Join(tmpDir, ".codex", "config.toml") + if err := os.MkdirAll(filepath.Dir(configPath), 0o755); err != nil { + t.Fatal(err) + } + existing := "# original-codex-backup-marker\n[profiles.default]\nmodel = \"gpt-5.5\"\n" + if err := os.WriteFile(configPath, []byte(existing), 0o644); err != nil { + t.Fatal(err) + } + + if err := writeCodexProfile(configPath); err != nil { + t.Fatal(err) + } + + assertBackupContains(t, filepath.Join(fileutil.BackupDir(), "config.toml.*"), "original-codex-backup-marker") + }) + t.Run("updates equivalent quoted root keys", func(t *testing.T) { tmpDir := t.TempDir() configPath := filepath.Join(tmpDir, "config.toml") @@ -327,3 +376,21 @@ func TestEnsureCodexConfig(t *testing.T) { } }) } + +func assertBackupContains(t *testing.T, pattern, marker string) { + t.Helper() + backups, err := filepath.Glob(pattern) + if err != nil { + t.Fatal(err) + } + for _, backupPath := range backups { + data, err := os.ReadFile(backupPath) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(data), marker) { + return + } + } + t.Fatalf("backup matching %q with marker %q not found", pattern, marker) +}