From 755439f95d8211d7295c99519fc597c0fafba0ff Mon Sep 17 00:00:00 2001
From: OwenYWT <ywt000818@gmail.com>
Date: Tue, 7 Apr 2026 17:08:26 +0800
Subject: [PATCH] fix(config): save empty config before clearing keychain
 entries

---
 cmd/config/config_test.go | 73 +++++++++++++++++++++++++++++++++++++++
 cmd/config/remove.go      | 14 ++++----
 2 files changed, 81 insertions(+), 6 deletions(-)

diff --git a/cmd/config/config_test.go b/cmd/config/config_test.go
index beb58c6c..2644467d 100644
--- a/cmd/config/config_test.go
+++ b/cmd/config/config_test.go
@@ -6,6 +6,8 @@ package config
 import (
 	"context"
 	"errors"
+	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 
@@ -21,6 +23,17 @@ func (n *noopConfigKeychain) Get(service, account string) (string, error) { retu
 func (n *noopConfigKeychain) Set(service, account, value string) error    { return nil }
 func (n *noopConfigKeychain) Remove(service, account string) error        { return nil }
 
+type recordingConfigKeychain struct {
+	removed []string
+}
+
+func (r *recordingConfigKeychain) Get(service, account string) (string, error) { return "", nil }
+func (r *recordingConfigKeychain) Set(service, account, value string) error    { return nil }
+func (r *recordingConfigKeychain) Remove(service, account string) error {
+	r.removed = append(r.removed, service+":"+account)
+	return nil
+}
+
 func TestConfigInitCmd_FlagParsing(t *testing.T) {
 	f, _, _, _ := cmdutil.TestFactory(t, nil)
 	f.IOStreams.In = strings.NewReader("secret123\n")
@@ -221,6 +234,66 @@ func TestConfigRemoveCmd_FlagParsing(t *testing.T) {
 	}
 }
 
+func TestConfigRemoveRun_SaveFailurePreservesExistingConfigAndSecrets(t *testing.T) {
+	configDir := t.TempDir()
+	t.Setenv("LARKSUITE_CLI_CONFIG_DIR", configDir)
+
+	multi := &core.MultiAppConfig{
+		Apps: []core.AppConfig{{
+			AppId: "app-test",
+			AppSecret: core.SecretInput{
+				Ref: &core.SecretRef{Source: "keychain", ID: "appsecret:app-test"},
+			},
+			Brand: core.BrandFeishu,
+			Users: []core.AppUser{{UserOpenId: "ou_1", UserName: "Tester"}},
+		}},
+	}
+	if err := core.SaveMultiAppConfig(multi); err != nil {
+		t.Fatalf("SaveMultiAppConfig() error = %v", err)
+	}
+
+	kc := &recordingConfigKeychain{}
+	f, _, _, _ := cmdutil.TestFactory(t, nil)
+	f.Keychain = kc
+
+	// Make subsequent config saves fail while keeping the existing config readable.
+	if err := os.Chmod(configDir, 0500); err != nil {
+		t.Fatalf("Chmod(%s) error = %v", configDir, err)
+	}
+	defer os.Chmod(configDir, 0700)
+
+	err := configRemoveRun(&ConfigRemoveOptions{Factory: f})
+	if err == nil {
+		t.Fatal("expected save failure")
+	}
+	if !strings.Contains(err.Error(), "failed to save config") {
+		t.Fatalf("error = %v, want failed to save config", err)
+	}
+	if len(kc.removed) != 0 {
+		t.Fatalf("expected no keychain cleanup before successful save, got removals: %v", kc.removed)
+	}
+
+	// Restore permissions and confirm the original config is still intact.
+	if err := os.Chmod(configDir, 0700); err != nil {
+		t.Fatalf("restore Chmod(%s) error = %v", configDir, err)
+	}
+	saved, err := core.LoadMultiAppConfig()
+	if err != nil {
+		t.Fatalf("LoadMultiAppConfig() error = %v", err)
+	}
+	if saved == nil || len(saved.Apps) != 1 || saved.Apps[0].AppId != "app-test" {
+		t.Fatalf("saved config = %#v, want original single app preserved", saved)
+	}
+	if got := saved.Apps[0].AppSecret.Ref; got == nil || got.ID != "appsecret:app-test" {
+		t.Fatalf("saved app secret ref = %#v, want preserved keychain ref", got)
+	}
+
+	configPath := filepath.Join(configDir, "config.json")
+	if _, err := os.Stat(configPath); err != nil {
+		t.Fatalf("expected existing config file to remain, stat error = %v", err)
+	}
+}
+
 func TestSaveAsProfile_RejectsProfileNameCollisionWithExistingAppID(t *testing.T) {
 	t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
 
diff --git a/cmd/config/remove.go b/cmd/config/remove.go
index 0d7835e8..ced7f254 100644
--- a/cmd/config/remove.go
+++ b/cmd/config/remove.go
@@ -44,7 +44,14 @@ func configRemoveRun(opts *ConfigRemoveOptions) error {
 		return output.ErrValidation("not configured yet")
 	}
 
-	// Clean up keychain entries for all apps
+	// Save empty config first. If this fails, keep secrets and tokens intact so the
+	// existing config can still be retried instead of ending up half-removed.
+	empty := &core.MultiAppConfig{Apps: []core.AppConfig{}}
+	if err := core.SaveMultiAppConfig(empty); err != nil {
+		return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err)
+	}
+
+	// Clean up keychain entries for all apps after config is cleared.
 	for _, app := range config.Apps {
 		core.RemoveSecretStore(app.AppSecret, f.Keychain)
 		for _, user := range app.Users {
@@ -52,11 +59,6 @@ func configRemoveRun(opts *ConfigRemoveOptions) error {
 		}
 	}
 
-	// Save empty config
-	empty := &core.MultiAppConfig{Apps: []core.AppConfig{}}
-	if err := core.SaveMultiAppConfig(empty); err != nil {
-		return output.Errorf(output.ExitInternal, "internal", "failed to save config: %v", err)
-	}
 	output.PrintSuccess(f.IOStreams.ErrOut, "Configuration removed")
 	userCount := 0
 	for _, app := range config.Apps {