fix(core): surface provider persistence errors in management API

core/engine.go

@@ -12114,7 +12114,9 @@ func (e *Engine) executeCardAction(cmd, args, sessionKey string) {
 			s.ClearHistory()
 			e.sessions.Save()
 			if e.providerSaveFunc != nil {
-				_ = e.providerSaveFunc(provName)
+				if err := e.providerSaveFunc(provName); err != nil {
+					slog.Error("failed to save provider", "error", err)
+				}
 			}
 		}
 

core/management.go

@@ -1154,14 +1154,26 @@ func (m *ManagementServer) handleProjectProviders(w http.ResponseWriter, r *http
 			action = parts[1]
 		}
 		if action == "activate" && r.Method == http.MethodPost {
-			if !ps.SetActiveProvider(provName) {
+			found := false
+			for _, p := range ps.ListProviders() {
+				if p.Name == provName {
+					found = true
+					break
+				}
+			}
+			if !found {
 				mgmtError(w, http.StatusNotFound, fmt.Sprintf("provider not found: %s", provName))
 				return
 			}
-			e.resetAllSessions()
 			if e.providerSaveFunc != nil {
-				_ = e.providerSaveFunc(provName)
+				if err := e.providerSaveFunc(provName); err != nil {
+					slog.Error("failed to save active provider", "provider", provName, "error", err)
+					mgmtError(w, http.StatusInternalServerError, "failed to persist provider: "+err.Error())
+					return
+				}
 			}
+			ps.SetActiveProvider(provName)
+			e.resetAllSessions()
 			mgmtJSON(w, http.StatusOK, map[string]any{
 				"active_provider": provName,
 				"message":         "provider activated",
@@ -1188,10 +1200,14 @@ func (m *ManagementServer) handleProjectProviders(w http.ResponseWriter, r *http
 				mgmtError(w, http.StatusNotFound, fmt.Sprintf("provider not found: %s", provName))
 				return
 			}
-			ps.SetProviders(remaining)
 			if e.providerRemoveSaveFunc != nil {
-				_ = e.providerRemoveSaveFunc(provName)
+				if err := e.providerRemoveSaveFunc(provName); err != nil {
+					slog.Error("failed to persist provider removal", "provider", provName, "error", err)
+					mgmtError(w, http.StatusInternalServerError, "failed to persist provider removal: "+err.Error())
+					return
+				}
 			}
+			ps.SetProviders(remaining)
 			mgmtOK(w, "provider removed")
 			return
 		}
@@ -1246,12 +1262,16 @@ func (m *ManagementServer) handleProjectProviders(w http.ResponseWriter, r *http
 			Thinking: body.Thinking,
 			Env:      body.Env,
 		}
+		if e.providerAddSaveFunc != nil {
+			if err := e.providerAddSaveFunc(prov); err != nil {
+				slog.Error("failed to persist provider", "provider", prov.Name, "error", err)
+				mgmtError(w, http.StatusInternalServerError, "failed to persist provider: "+err.Error())
+				return
+			}
+		}
 		providers := ps.ListProviders()
 		providers = append(providers, prov)
 		ps.SetProviders(providers)
-		if e.providerAddSaveFunc != nil {
-			_ = e.providerAddSaveFunc(prov)
-		}
 		mgmtJSON(w, http.StatusOK, map[string]any{
 			"name":    body.Name,
 			"message": "provider added",

core/management_provider_persist_test.go

@@ -0,0 +1,225 @@
+package core
+
+import (
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"sync"
+	"testing"
+)
+
+// stubPersistProvider is a ProviderSwitcher with a mutex so handler tests can
+// assert on in-memory state without racing the HTTP handler goroutine.
+type stubPersistProvider struct {
+	stubAgent
+	mu        sync.Mutex
+	providers []ProviderConfig
+	active    string
+}
+
+func (s *stubPersistProvider) ListProviders() []ProviderConfig {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	out := make([]ProviderConfig, len(s.providers))
+	copy(out, s.providers)
+	return out
+}
+func (s *stubPersistProvider) SetProviders(p []ProviderConfig) {
+	s.mu.Lock()
+	s.providers = append([]ProviderConfig(nil), p...)
+	s.mu.Unlock()
+}
+func (s *stubPersistProvider) GetActiveProvider() *ProviderConfig {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	for i := range s.providers {
+		if s.providers[i].Name == s.active {
+			p := s.providers[i]
+			return &p
+		}
+	}
+	return nil
+}
+func (s *stubPersistProvider) SetActiveProvider(name string) bool {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	if name == "" {
+		s.active = ""
+		return true
+	}
+	for _, p := range s.providers {
+		if p.Name == name {
+			s.active = name
+			return true
+		}
+	}
+	return false
+}
+func (s *stubPersistProvider) activeName() string {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	return s.active
+}
+func (s *stubPersistProvider) count() int {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	return len(s.providers)
+}
+
+func newMgmtProviderServer(t *testing.T, token string, save, addSave, removeSave func() error) (*ManagementServer, *httptest.Server, *Engine, *stubPersistProvider) {
+	t.Helper()
+	agent := &stubPersistProvider{
+		providers: []ProviderConfig{{Name: "prov-a"}, {Name: "prov-b"}},
+	}
+	e := NewEngine("test-project", agent, nil, "", LangEnglish)
+	e.sessions = NewSessionManager("")
+	e.providerSaveFunc = func(string) error { return save() }
+	e.providerAddSaveFunc = func(ProviderConfig) error { return addSave() }
+	e.providerRemoveSaveFunc = func(string) error { return removeSave() }
+
+	mgmt := NewManagementServer(0, token, nil)
+	mgmt.RegisterEngine("test-project", e)
+
+	mux := http.NewServeMux()
+	mux.HandleFunc("/api/v1/projects/", mgmt.wrap(mgmt.handleProjectRoutes))
+	ts := httptest.NewServer(mux)
+	t.Cleanup(ts.Close)
+	return mgmt, ts, e, agent
+}
+
+// Regression: activate previously returned 200 even when persisting the active
+// provider to disk failed, so the in-memory state diverged from what was on
+// disk without telling the caller. The handler must now return 500 and leave
+// the active provider unchanged when save fails.
+func TestMgmt_ActivateProvider_PersistFailure(t *testing.T) {
+	wantErr := errors.New("disk full")
+	_, ts, _, agent := newMgmtProviderServer(t, "tok",
+		func() error { return wantErr },
+		func() error { return nil },
+		func() error { return nil },
+	)
+
+	req, _ := http.NewRequest(http.MethodPost, ts.URL+"/api/v1/projects/test-project/providers/prov-a/activate", nil)
+	req.Header.Set("Authorization", "Bearer tok")
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		t.Fatalf("post activate: %v", err)
+	}
+	defer func() { _ = resp.Body.Close() }()
+	if resp.StatusCode != http.StatusInternalServerError {
+		t.Fatalf("status = %d, want 500 on persist failure", resp.StatusCode)
+	}
+	if got := agent.activeName(); got != "" {
+		t.Fatalf("active provider mutated to %q despite persist failure", got)
+	}
+}
+
+// Regression: DELETE provider must not mutate in-memory state when disk removal
+// fails; it must surface a 500 so the user knows the operation didn't take.
+func TestMgmt_DeleteProvider_PersistFailure(t *testing.T) {
+	wantErr := errors.New("permission denied")
+	_, ts, _, agent := newMgmtProviderServer(t, "tok",
+		func() error { return nil },
+		func() error { return nil },
+		func() error { return wantErr },
+	)
+
+	req, _ := http.NewRequest(http.MethodDelete, ts.URL+"/api/v1/projects/test-project/providers/prov-a", nil)
+	req.Header.Set("Authorization", "Bearer tok")
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		t.Fatalf("delete: %v", err)
+	}
+	defer func() { _ = resp.Body.Close() }()
+	if resp.StatusCode != http.StatusInternalServerError {
+		t.Fatalf("status = %d, want 500 on persist failure", resp.StatusCode)
+	}
+	if got := agent.count(); got != 2 {
+		t.Fatalf("provider count = %d after failed delete, want 2 (unchanged)", got)
+	}
+}
+
+// Regression: POST provider must surface persistence failures as 500 and must
+// not add the provider to in-memory state when disk write fails.
+func TestMgmt_AddProvider_PersistFailure(t *testing.T) {
+	wantErr := errors.New("config read-only")
+	_, ts, _, agent := newMgmtProviderServer(t, "tok",
+		func() error { return nil },
+		func() error { return wantErr },
+		func() error { return nil },
+	)
+
+	body := strings.NewReader(`{"name":"prov-c","api_key":"x"}`)
+	req, _ := http.NewRequest(http.MethodPost, ts.URL+"/api/v1/projects/test-project/providers", body)
+	req.Header.Set("Authorization", "Bearer tok")
+	req.Header.Set("Content-Type", "application/json")
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		t.Fatalf("post add: %v", err)
+	}
+	defer func() { _ = resp.Body.Close() }()
+	if resp.StatusCode != http.StatusInternalServerError {
+		t.Fatalf("status = %d, want 500 on persist failure", resp.StatusCode)
+	}
+	if got := agent.count(); got != 2 {
+		t.Fatalf("provider count = %d after failed add, want 2 (unchanged)", got)
+	}
+}
+
+// Sanity: when persistence succeeds, the happy paths still do the in-memory
+// mutation and return 200.
+func TestMgmt_ProviderLifecycle_HappyPath(t *testing.T) {
+	_, ts, _, agent := newMgmtProviderServer(t, "tok",
+		func() error { return nil },
+		func() error { return nil },
+		func() error { return nil },
+	)
+
+	// activate
+	req, _ := http.NewRequest(http.MethodPost, ts.URL+"/api/v1/projects/test-project/providers/prov-a/activate", nil)
+	req.Header.Set("Authorization", "Bearer tok")
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		t.Fatalf("activate: %v", err)
+	}
+	_ = resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("activate status = %d, want 200", resp.StatusCode)
+	}
+	if got := agent.activeName(); got != "prov-a" {
+		t.Fatalf("active = %q, want prov-a", got)
+	}
+
+	// add
+	addBody := strings.NewReader(`{"name":"prov-c"}`)
+	req, _ = http.NewRequest(http.MethodPost, ts.URL+"/api/v1/projects/test-project/providers", addBody)
+	req.Header.Set("Authorization", "Bearer tok")
+	req.Header.Set("Content-Type", "application/json")
+	resp, err = http.DefaultClient.Do(req)
+	if err != nil {
+		t.Fatalf("add: %v", err)
+	}
+	_ = resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("add status = %d, want 200", resp.StatusCode)
+	}
+	if got := agent.count(); got != 3 {
+		t.Fatalf("count after add = %d, want 3", got)
+	}
+
+	// delete
+	req, _ = http.NewRequest(http.MethodDelete, ts.URL+"/api/v1/projects/test-project/providers/prov-c", nil)
+	req.Header.Set("Authorization", "Bearer tok")
+	resp, err = http.DefaultClient.Do(req)
+	if err != nil {
+		t.Fatalf("delete: %v", err)
+	}
+	_ = resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("delete status = %d, want 200", resp.StatusCode)
+	}
+	if got := agent.count(); got != 2 {
+		t.Fatalf("count after delete = %d, want 2", got)
+	}
+}