Fix: Prevent security.yml from being overwritten during config migration (#1966)

pkg/config/config.go

@@ -1395,6 +1395,18 @@ func LoadConfig(path string) (*Config, error) {
 		if err != nil {
 			return nil, err
 		}
+		// Load existing security config and merge with migrated one to prevent data loss
+		existingSec, secErr := loadSecurityConfig(securityPath(path))
+		if secErr != nil {
+			logger.WarnF("failed to load existing security config during migration", map[string]any{"error": secErr})
+		}
+		if existingSec != nil && cfg.security != nil {
+			cfg.security = mergeSecurityConfig(existingSec, cfg.security)
+			// Re-apply the merged security config to update all channels and models
+			if err = applySecurityConfig(cfg, cfg.security); err != nil {
+				logger.WarnF("failed to re-apply merged security config during migration", map[string]any{"error": err})
+			}
+		}
 		defer func(cfg *Config) {
 			_ = SaveConfig(path, cfg)
 		}(cfg)

pkg/config/migration_integration_test.go

@@ -566,3 +566,118 @@ func TestMigration_Integration_ModelNameField(t *testing.T) {
 		t.Errorf("ModelFallbacks[0] = %q, want %q", cfg.Agents.Defaults.ModelFallbacks[0], "deepseek-chat")
 	}
 }
+
+// TestMigration_PreservesExistingSecurityConfig tests that when migrating from v0 to v1,
+// existing .security.yml values (e.g., loaded from environment variables) are preserved
+// and not overwritten by empty values from the legacy config.
+func TestMigration_PreservesExistingSecurityConfig(t *testing.T) {
+	tmpDir := t.TempDir()
+	configPath := filepath.Join(tmpDir, "config.json")
+	securityPath := filepath.Join(tmpDir, ".security.yml")
+
+	// Create a legacy config (version 0) with model_list and channel config
+	// The model_list doesn't have api_keys, they should come from existing .security.yml
+	legacyConfig := `{
+		"agents": {
+			"defaults": {
+				"provider": "openai",
+				"model": "gpt-4"
+			}
+		},
+		"model_list": [
+			{
+				"model_name": "openai",
+				"model": "openai/gpt-4"
+			}
+		],
+		"channels": {
+			"telegram": {
+				"enabled": true
+			}
+		},
+		"gateway": {
+			"host": "127.0.0.1",
+			"port": 18790
+		},
+		"tools": {
+			"web": {"enabled": true}
+		},
+		"heartbeat": {
+			"enabled": true,
+			"interval": 30
+		},
+		"devices": {
+			"enabled": false
+		}
+	}`
+
+	// Create an existing .security.yml with values that might come from env vars
+	existingSecurity := `model_list:
+  openai:0:
+    api_keys:
+      - sk-existing-key-from-env
+channels:
+  telegram:
+    token: existing-telegram-token-from-env
+  discord:
+    token: existing-discord-token-from-env
+web:
+  brave:
+    api_keys:
+      - existing-brave-key
+`
+
+	if err := os.WriteFile(configPath, []byte(legacyConfig), 0o600); err != nil {
+		t.Fatalf("Failed to write legacy config: %v", err)
+	}
+
+	if err := os.WriteFile(securityPath, []byte(existingSecurity), 0o600); err != nil {
+		t.Fatalf("Failed to write existing security config: %v", err)
+	}
+
+	// Load the config - this should trigger migration
+	cfg, err := LoadConfig(configPath)
+	if err != nil {
+		t.Fatalf("LoadConfig failed: %v", err)
+	}
+
+	// Verify that the migrated config has the existing security values
+	// Telegram token should be preserved
+	if cfg.Channels.Telegram.Token() != "existing-telegram-token-from-env" {
+		t.Errorf("Telegram token was overwritten: got %q, want %q",
+			cfg.Channels.Telegram.Token(), "existing-telegram-token-from-env")
+	}
+
+	// Discord token should be preserved (even though legacy config didn't have it)
+	if cfg.Channels.Discord.Token() != "existing-discord-token-from-env" {
+		t.Errorf("Discord token was overwritten: got %q, want %q",
+			cfg.Channels.Discord.Token(), "existing-discord-token-from-env")
+	}
+
+	// Model API key should be preserved
+	if cfg.ModelList[0].APIKey() != "sk-existing-key-from-env" {
+		t.Errorf("Model API key was overwritten: got %q, want %q",
+			cfg.ModelList[0].APIKey(), "sk-existing-key-from-env")
+	}
+
+	// Brave API key should be preserved
+	if cfg.Tools.Web.Brave.APIKey() != "existing-brave-key" {
+		t.Errorf("Brave API key was overwritten: got %q, want %q",
+			cfg.Tools.Web.Brave.APIKey(), "existing-brave-key")
+	}
+
+	// Reload the security config from disk to verify it wasn't corrupted
+	reloadedSec, err := loadSecurityConfig(securityPath)
+	if err != nil {
+		t.Fatalf("Failed to reload security config: %v", err)
+	}
+
+	if reloadedSec.Channels.Telegram == nil ||
+		reloadedSec.Channels.Telegram.Token != "existing-telegram-token-from-env" {
+		t.Error("Telegram token not preserved in .security.yml file")
+	}
+
+	if reloadedSec.Channels.Discord == nil || reloadedSec.Channels.Discord.Token != "existing-discord-token-from-env" {
+		t.Error("Discord token not preserved in .security.yml file")
+	}
+}

pkg/config/security.go

@@ -244,6 +244,142 @@ func saveSecurityConfig(securityPath string, sec *SecurityConfig) error {
 	return fileutil.WriteFileAtomic(securityPath, buf.Bytes(), 0o600)
 }
 
+// mergeSecurityConfig merges two SecurityConfig instances, preferring non-empty values from 'newer'.
+// This is used during config migration to preserve existing security data while adding new entries.
+func mergeSecurityConfig(existing, newer *SecurityConfig) *SecurityConfig {
+	if existing == nil {
+		return normalizeSecurityConfig(newer)
+	}
+	if newer == nil {
+		return normalizeSecurityConfig(existing)
+	}
+
+	result := normalizeSecurityConfig(nil)
+
+	// Merge ModelList: prefer newer if it has keys, otherwise use existing
+	for k, v := range existing.ModelList {
+		result.ModelList[k] = v
+	}
+	for k, v := range newer.ModelList {
+		if len(v.APIKeys) > 0 {
+			result.ModelList[k] = v
+		}
+	}
+
+	// Merge Channels
+	if existing.Channels != nil {
+		result.Channels = existing.Channels
+	}
+	if newer.Channels != nil {
+		if result.Channels == nil {
+			result.Channels = &ChannelsSecurity{}
+		}
+		mergeChannelsSecurity(result.Channels, newer.Channels)
+	}
+
+	// Merge Web
+	if existing.Web != nil {
+		result.Web = existing.Web
+	}
+	if newer.Web != nil {
+		if result.Web == nil {
+			result.Web = &WebToolsSecurity{}
+		}
+		mergeWebToolsSecurity(result.Web, newer.Web)
+	}
+
+	// Merge Skills
+	if existing.Skills != nil {
+		result.Skills = existing.Skills
+	}
+	if newer.Skills != nil {
+		if result.Skills == nil {
+			result.Skills = &SkillsSecurity{}
+		}
+		mergeSkillsSecurity(result.Skills, newer.Skills)
+	}
+
+	return result
+}
+
+func mergeChannelsSecurity(dst, src *ChannelsSecurity) {
+	if src.Telegram != nil && src.Telegram.Token != "" {
+		dst.Telegram = src.Telegram
+	}
+	if src.Feishu != nil &&
+		(src.Feishu.AppSecret != "" || src.Feishu.EncryptKey != "" || src.Feishu.VerificationToken != "") {
+		dst.Feishu = src.Feishu
+	}
+	if src.Discord != nil && src.Discord.Token != "" {
+		dst.Discord = src.Discord
+	}
+	if src.Weixin != nil && src.Weixin.Token != "" {
+		dst.Weixin = src.Weixin
+	}
+	if src.QQ != nil && src.QQ.AppSecret != "" {
+		dst.QQ = src.QQ
+	}
+	if src.DingTalk != nil && src.DingTalk.ClientSecret != "" {
+		dst.DingTalk = src.DingTalk
+	}
+	if src.Slack != nil && (src.Slack.BotToken != "" || src.Slack.AppToken != "") {
+		dst.Slack = src.Slack
+	}
+	if src.Matrix != nil && src.Matrix.AccessToken != "" {
+		dst.Matrix = src.Matrix
+	}
+	if src.LINE != nil && (src.LINE.ChannelSecret != "" || src.LINE.ChannelAccessToken != "") {
+		dst.LINE = src.LINE
+	}
+	if src.OneBot != nil && src.OneBot.AccessToken != "" {
+		dst.OneBot = src.OneBot
+	}
+	if src.WeCom != nil && (src.WeCom.Token != "" || src.WeCom.EncodingAESKey != "") {
+		dst.WeCom = src.WeCom
+	}
+	if src.WeComApp != nil &&
+		(src.WeComApp.CorpSecret != "" || src.WeComApp.Token != "" || src.WeComApp.EncodingAESKey != "") {
+		dst.WeComApp = src.WeComApp
+	}
+	if src.WeComAIBot != nil &&
+		(src.WeComAIBot.Secret != "" || src.WeComAIBot.Token != "" || src.WeComAIBot.EncodingAESKey != "") {
+		dst.WeComAIBot = src.WeComAIBot
+	}
+	if src.Pico != nil && src.Pico.Token != "" {
+		dst.Pico = src.Pico
+	}
+	if src.IRC != nil && (src.IRC.Password != "" || src.IRC.NickServPassword != "" || src.IRC.SASLPassword != "") {
+		dst.IRC = src.IRC
+	}
+}
+
+func mergeWebToolsSecurity(dst, src *WebToolsSecurity) {
+	if src.Brave != nil && len(src.Brave.APIKeys) > 0 {
+		dst.Brave = src.Brave
+	}
+	if src.Tavily != nil && len(src.Tavily.APIKeys) > 0 {
+		dst.Tavily = src.Tavily
+	}
+	if src.Perplexity != nil && len(src.Perplexity.APIKeys) > 0 {
+		dst.Perplexity = src.Perplexity
+	}
+	if src.GLMSearch != nil && src.GLMSearch.APIKey != "" {
+		dst.GLMSearch = src.GLMSearch
+	}
+	if src.BaiduSearch != nil && src.BaiduSearch.APIKey != "" {
+		dst.BaiduSearch = src.BaiduSearch
+	}
+}
+
+func mergeSkillsSecurity(dst, src *SkillsSecurity) {
+	if src.Github != nil && src.Github.Token != "" {
+		dst.Github = src.Github
+	}
+	if src.ClawHub != nil && src.ClawHub.AuthToken != "" {
+		dst.ClawHub = src.ClawHub
+	}
+}
+
 // SensitiveDataCache caches the compiled regex for filtering sensitive data.
 // SensitiveDataCache caches the strings.Replacer for filtering sensitive data.
 // Computed once on first access via sync.Once.