From fd243642209e3f2773c3d6cef125ee3711cd1e5c Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sun, 12 Apr 2026 18:15:55 +0200 Subject: [PATCH 1/2] =?UTF-8?q?refactor(channels):=20data-driven=20init=20?= =?UTF-8?q?=E2=80=94=20factories=20self-manage=20enabled=20check?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit initChannels was a manual dispatch table; adding a new channel required a human to remember to add an if-enabled block there, separate from writing the factory. Email was wired to the registry but never to the dispatch table, so it was silently skipped at runtime. - getAllFactories() added to registry so initChannels can iterate all registered factories in a single loop - initChannel now skips silently when factory returns (nil, nil) instead of storing nil in m.channels - Every factory closure gains an Enabled guard (return nil, nil when disabled); WhatsApp routing logic (UseNative) moved from initChannels into the two WhatsApp factories - hiddenValues/updateKeys in manager_channel.go now handle email SMTPPassword/IMAPPassword so hot-reload doesn't lose credentials - manager_sushi30_test.go: integration tests verify email channel is present after NewManager with Email.Enabled=true, and absent when disabled Adding a channel in future requires only init.go + blank import in gateway.go — the dispatch is automatic. Co-Authored-By: Claude Sonnet 4.6 --- pkg/channels/dingtalk/init.go | 3 + pkg/channels/discord/init.go | 3 + pkg/channels/feishu/init.go | 3 + pkg/channels/line/init.go | 3 + pkg/channels/maixcam/init.go | 3 + pkg/channels/manager.go | 93 ++-------------------------- pkg/channels/manager_channel.go | 7 +++ pkg/channels/manager_sushi30_test.go | 53 ++++++++++++++++ pkg/channels/matrix/init.go | 3 + pkg/channels/onebot/init.go | 3 + pkg/channels/pico/init.go | 6 ++ pkg/channels/qq/init.go | 3 + pkg/channels/registry.go | 11 ++++ pkg/channels/slack/init.go | 3 + pkg/channels/teams_webhook/init.go | 3 + pkg/channels/telegram/init.go | 3 + pkg/channels/vk/init.go | 3 + pkg/channels/wecom/init.go | 3 + pkg/channels/weixin/weixin.go | 3 + pkg/channels/whatsapp/init.go | 6 +- pkg/channels/whatsapp_native/init.go | 3 + 21 files changed, 132 insertions(+), 89 deletions(-) create mode 100644 pkg/channels/manager_sushi30_test.go diff --git a/pkg/channels/dingtalk/init.go b/pkg/channels/dingtalk/init.go index 5f49bce8c..2e77e8e17 100644 --- a/pkg/channels/dingtalk/init.go +++ b/pkg/channels/dingtalk/init.go @@ -8,6 +8,9 @@ import ( func init() { channels.RegisterFactory("dingtalk", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) { + if !cfg.Channels.DingTalk.Enabled { + return nil, nil + } return NewDingTalkChannel(cfg.Channels.DingTalk, b) }) } diff --git a/pkg/channels/discord/init.go b/pkg/channels/discord/init.go index 8381dc9e9..2c0d3d488 100644 --- a/pkg/channels/discord/init.go +++ b/pkg/channels/discord/init.go @@ -9,6 +9,9 @@ import ( func init() { channels.RegisterFactory("discord", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) { + if !cfg.Channels.Discord.Enabled { + return nil, nil + } ch, err := NewDiscordChannel(cfg.Channels.Discord, b) if err == nil { ch.tts = tts.DetectTTS(cfg) diff --git a/pkg/channels/feishu/init.go b/pkg/channels/feishu/init.go index 7e5a62dae..57d114842 100644 --- a/pkg/channels/feishu/init.go +++ b/pkg/channels/feishu/init.go @@ -8,6 +8,9 @@ import ( func init() { channels.RegisterFactory("feishu", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) { + if !cfg.Channels.Feishu.Enabled { + return nil, nil + } return NewFeishuChannel(cfg.Channels.Feishu, b) }) } diff --git a/pkg/channels/line/init.go b/pkg/channels/line/init.go index 9265575cc..06f0016f2 100644 --- a/pkg/channels/line/init.go +++ b/pkg/channels/line/init.go @@ -8,6 +8,9 @@ import ( func init() { channels.RegisterFactory("line", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) { + if !cfg.Channels.LINE.Enabled { + return nil, nil + } return NewLINEChannel(cfg.Channels.LINE, b) }) } diff --git a/pkg/channels/maixcam/init.go b/pkg/channels/maixcam/init.go index 5a269b22b..666f399a0 100644 --- a/pkg/channels/maixcam/init.go +++ b/pkg/channels/maixcam/init.go @@ -8,6 +8,9 @@ import ( func init() { channels.RegisterFactory("maixcam", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) { + if !cfg.Channels.MaixCam.Enabled { + return nil, nil + } return NewMaixCamChannel(cfg.Channels.MaixCam, b) }) } diff --git a/pkg/channels/manager.go b/pkg/channels/manager.go index c4326fda0..468392178 100644 --- a/pkg/channels/manager.go +++ b/pkg/channels/manager.go @@ -329,6 +329,8 @@ func (m *Manager) initChannel(name, displayName string) { "channel": displayName, "error": err.Error(), }) + } else if ch == nil { + // factory returned nil — channel disabled, skip silently } else { // Inject MediaStore if channel supports it if m.mediaStore != nil { @@ -351,96 +353,11 @@ func (m *Manager) initChannel(name, displayName string) { } } -func (m *Manager) initChannels(channels *config.ChannelsConfig) error { +func (m *Manager) initChannels(_ *config.ChannelsConfig) error { logger.InfoC("channels", "Initializing channel manager") - if channels.Telegram.Enabled && channels.Telegram.Token.String() != "" { - m.initChannel("telegram", "Telegram") - } - - if channels.WhatsApp.Enabled { - waCfg := channels.WhatsApp - if waCfg.UseNative { - m.initChannel("whatsapp_native", "WhatsApp Native") - } else if waCfg.BridgeURL != "" { - m.initChannel("whatsapp", "WhatsApp") - } - } - - if channels.Feishu.Enabled { - m.initChannel("feishu", "Feishu") - } - - if channels.Discord.Enabled && channels.Discord.Token.String() != "" { - m.initChannel("discord", "Discord") - } - - if channels.MaixCam.Enabled { - m.initChannel("maixcam", "MaixCam") - } - - if channels.QQ.Enabled { - m.initChannel("qq", "QQ") - } - - if channels.DingTalk.Enabled && channels.DingTalk.ClientID != "" { - m.initChannel("dingtalk", "DingTalk") - } - - if channels.Slack.Enabled && channels.Slack.BotToken.String() != "" { - m.initChannel("slack", "Slack") - } - - if channels.Matrix.Enabled && - m.config.Channels.Matrix.Homeserver != "" && - m.config.Channels.Matrix.UserID != "" && - m.config.Channels.Matrix.AccessToken.String() != "" { - m.initChannel("matrix", "Matrix") - } - - if channels.LINE.Enabled && channels.LINE.ChannelAccessToken.String() != "" { - m.initChannel("line", "LINE") - } - - if channels.OneBot.Enabled && channels.OneBot.WSUrl != "" { - m.initChannel("onebot", "OneBot") - } - - if channels.WeCom.Enabled && channels.WeCom.BotID != "" && channels.WeCom.Secret.String() != "" { - m.initChannel("wecom", "WeCom") - } - - if channels.Weixin.Enabled && channels.Weixin.Token.String() != "" { - m.initChannel("weixin", "Weixin") - } - - if channels.Pico.Enabled && channels.Pico.Token.String() != "" { - m.initChannel("pico", "Pico") - } - - if channels.PicoClient.Enabled && channels.PicoClient.URL != "" { - m.initChannel("pico_client", "Pico Client") - } - - if channels.IRC.Enabled && channels.IRC.Server != "" { - m.initChannel("irc", "IRC") - } - - if channels.VK.Enabled && channels.VK.Token.String() != "" && channels.VK.GroupID != 0 { - m.initChannel("vk", "VK") - } - - if channels.TeamsWebhook.Enabled && len(channels.TeamsWebhook.Webhooks) > 0 { - hasValidTarget := false - for _, target := range channels.TeamsWebhook.Webhooks { - if target.WebhookURL.String() != "" { - hasValidTarget = true - break - } - } - if hasValidTarget { - m.initChannel("teams_webhook", "Teams Webhook") - } + for name := range getAllFactories() { + m.initChannel(name, name) } logger.InfoCF("channels", "Channel initialization completed", map[string]any{ diff --git a/pkg/channels/manager_channel.go b/pkg/channels/manager_channel.go index b54facda4..6afcb3435 100644 --- a/pkg/channels/manager_channel.go +++ b/pkg/channels/manager_channel.go @@ -62,6 +62,9 @@ func hiddenValues(key string, value map[string]any, ch config.ChannelsConfig) { value["app_secret"] = ch.Feishu.AppSecret.String() value["encrypt_key"] = ch.Feishu.EncryptKey.String() value["verification_token"] = ch.Feishu.VerificationToken.String() + case "email": + value["smtp_password"] = ch.Email.SMTPPassword.String() + value["imap_password"] = ch.Email.IMAPPassword.String() case "teams_webhook": // Expose webhook URLs for hash computation (they contain secrets) webhooks := make(map[string]string) @@ -173,6 +176,10 @@ func updateKeys(newcfg, old *config.ChannelsConfig) { newcfg.Feishu.EncryptKey = old.Feishu.EncryptKey newcfg.Feishu.VerificationToken = old.Feishu.VerificationToken } + if newcfg.Email.Enabled { + newcfg.Email.SMTPPassword = old.Email.SMTPPassword + newcfg.Email.IMAPPassword = old.Email.IMAPPassword + } if newcfg.TeamsWebhook.Enabled { // Copy SecureString webhook URLs from old config for name, oldTarget := range old.TeamsWebhook.Webhooks { diff --git a/pkg/channels/manager_sushi30_test.go b/pkg/channels/manager_sushi30_test.go new file mode 100644 index 000000000..16fa4cc95 --- /dev/null +++ b/pkg/channels/manager_sushi30_test.go @@ -0,0 +1,53 @@ +package channels_test + +import ( + "testing" + + "github.com/sipeed/picoclaw/pkg/bus" + "github.com/sipeed/picoclaw/pkg/channels" + "github.com/sipeed/picoclaw/pkg/config" + + _ "github.com/sipeed/picoclaw/pkg/channels/email" +) + +// TestInitChannels_EmailPickedUp verifies that a channel with Enabled=true is +// actually registered in the manager after initialization. This guards against +// the class of bug where a factory is registered in init() but the manager's +// dispatch logic is never wired up. +func TestInitChannels_EmailPickedUp(t *testing.T) { + cfg := config.DefaultConfig() + cfg.Channels.Email = config.EmailConfig{ + Enabled: true, + SMTPHost: "smtp.example.com", + SMTPFrom: *config.NewSecureString("bot@example.com"), + IMAPHost: "imap.example.com", + IMAPUser: *config.NewSecureString("bot@example.com"), + } + + m, err := channels.NewManager(cfg, bus.NewMessageBus(), nil) + if err != nil { + t.Fatalf("NewManager returned error: %v", err) + } + + ch, ok := m.GetChannel("email") + if !ok || ch == nil { + t.Error("email channel should be present in manager after NewManager with Email.Enabled=true") + } +} + +// TestInitChannels_DisabledChannelAbsent verifies that a factory returning +// (nil, nil) — i.e. the channel is disabled — does not populate the manager. +func TestInitChannels_DisabledChannelAbsent(t *testing.T) { + cfg := config.DefaultConfig() + // Email disabled (default) + + m, err := channels.NewManager(cfg, bus.NewMessageBus(), nil) + if err != nil { + t.Fatalf("NewManager returned error: %v", err) + } + + ch, ok := m.GetChannel("email") + if ok && ch != nil { + t.Error("email channel should not be present in manager when Email.Enabled=false") + } +} diff --git a/pkg/channels/matrix/init.go b/pkg/channels/matrix/init.go index 4d6ad45a7..a577cf226 100644 --- a/pkg/channels/matrix/init.go +++ b/pkg/channels/matrix/init.go @@ -10,6 +10,9 @@ import ( func init() { channels.RegisterFactory("matrix", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) { + if !cfg.Channels.Matrix.Enabled { + return nil, nil + } matrixCfg := cfg.Channels.Matrix cryptoDatabasePath := matrixCfg.CryptoDatabasePath if cryptoDatabasePath == "" { diff --git a/pkg/channels/onebot/init.go b/pkg/channels/onebot/init.go index 84c06dfd6..df0d25273 100644 --- a/pkg/channels/onebot/init.go +++ b/pkg/channels/onebot/init.go @@ -8,6 +8,9 @@ import ( func init() { channels.RegisterFactory("onebot", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) { + if !cfg.Channels.OneBot.Enabled { + return nil, nil + } return NewOneBotChannel(cfg.Channels.OneBot, b) }) } diff --git a/pkg/channels/pico/init.go b/pkg/channels/pico/init.go index 0319279d8..c975ecdb6 100644 --- a/pkg/channels/pico/init.go +++ b/pkg/channels/pico/init.go @@ -8,9 +8,15 @@ import ( func init() { channels.RegisterFactory("pico", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) { + if !cfg.Channels.Pico.Enabled { + return nil, nil + } return NewPicoChannel(cfg.Channels.Pico, b) }) channels.RegisterFactory("pico_client", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) { + if !cfg.Channels.PicoClient.Enabled { + return nil, nil + } return NewPicoClientChannel(cfg.Channels.PicoClient, b) }) } diff --git a/pkg/channels/qq/init.go b/pkg/channels/qq/init.go index 15b955089..75dbc876c 100644 --- a/pkg/channels/qq/init.go +++ b/pkg/channels/qq/init.go @@ -8,6 +8,9 @@ import ( func init() { channels.RegisterFactory("qq", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) { + if !cfg.Channels.QQ.Enabled { + return nil, nil + } return NewQQChannel(cfg.Channels.QQ, b) }) } diff --git a/pkg/channels/registry.go b/pkg/channels/registry.go index 36a05bf3e..47c26ff6d 100644 --- a/pkg/channels/registry.go +++ b/pkg/channels/registry.go @@ -30,3 +30,14 @@ func getFactory(name string) (ChannelFactory, bool) { f, ok := factories[name] return f, ok } + +// getAllFactories returns a shallow copy of all registered channel factories. +func getAllFactories() map[string]ChannelFactory { + factoriesMu.RLock() + defer factoriesMu.RUnlock() + result := make(map[string]ChannelFactory, len(factories)) + for k, v := range factories { + result[k] = v + } + return result +} diff --git a/pkg/channels/slack/init.go b/pkg/channels/slack/init.go index c131bb291..ff3c06379 100644 --- a/pkg/channels/slack/init.go +++ b/pkg/channels/slack/init.go @@ -8,6 +8,9 @@ import ( func init() { channels.RegisterFactory("slack", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) { + if !cfg.Channels.Slack.Enabled { + return nil, nil + } return NewSlackChannel(cfg.Channels.Slack, b) }) } diff --git a/pkg/channels/teams_webhook/init.go b/pkg/channels/teams_webhook/init.go index fca960039..cab9438d3 100644 --- a/pkg/channels/teams_webhook/init.go +++ b/pkg/channels/teams_webhook/init.go @@ -8,6 +8,9 @@ import ( func init() { channels.RegisterFactory("teams_webhook", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) { + if !cfg.Channels.TeamsWebhook.Enabled { + return nil, nil + } return NewTeamsWebhookChannel(cfg.Channels.TeamsWebhook, b) }) } diff --git a/pkg/channels/telegram/init.go b/pkg/channels/telegram/init.go index ac87bb805..f40e012cf 100644 --- a/pkg/channels/telegram/init.go +++ b/pkg/channels/telegram/init.go @@ -8,6 +8,9 @@ import ( func init() { channels.RegisterFactory("telegram", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) { + if !cfg.Channels.Telegram.Enabled { + return nil, nil + } return NewTelegramChannel(cfg, b) }) } diff --git a/pkg/channels/vk/init.go b/pkg/channels/vk/init.go index 6a5927a32..d5ef31ab1 100644 --- a/pkg/channels/vk/init.go +++ b/pkg/channels/vk/init.go @@ -8,6 +8,9 @@ import ( func init() { channels.RegisterFactory("vk", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) { + if !cfg.Channels.VK.Enabled { + return nil, nil + } return NewVKChannel(cfg, b) }) } diff --git a/pkg/channels/wecom/init.go b/pkg/channels/wecom/init.go index 3aad84d42..ba5dfacaf 100644 --- a/pkg/channels/wecom/init.go +++ b/pkg/channels/wecom/init.go @@ -8,6 +8,9 @@ import ( func init() { channels.RegisterFactory("wecom", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) { + if !cfg.Channels.WeCom.Enabled { + return nil, nil + } return NewChannel(cfg.Channels.WeCom, b) }) } diff --git a/pkg/channels/weixin/weixin.go b/pkg/channels/weixin/weixin.go index a0d0c96b5..4c1daa144 100644 --- a/pkg/channels/weixin/weixin.go +++ b/pkg/channels/weixin/weixin.go @@ -37,6 +37,9 @@ type WeixinChannel struct { func init() { channels.RegisterFactory("weixin", func(cfg *config.Config, bus *bus.MessageBus) (channels.Channel, error) { + if !cfg.Channels.Weixin.Enabled { + return nil, nil + } return NewWeixinChannel(cfg.Channels.Weixin, bus) }) } diff --git a/pkg/channels/whatsapp/init.go b/pkg/channels/whatsapp/init.go index d9c2669c3..9a8647022 100644 --- a/pkg/channels/whatsapp/init.go +++ b/pkg/channels/whatsapp/init.go @@ -8,6 +8,10 @@ import ( func init() { channels.RegisterFactory("whatsapp", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) { - return NewWhatsAppChannel(cfg.Channels.WhatsApp, b) + waCfg := cfg.Channels.WhatsApp + if !waCfg.Enabled || waCfg.UseNative || waCfg.BridgeURL == "" { + return nil, nil + } + return NewWhatsAppChannel(waCfg, b) }) } diff --git a/pkg/channels/whatsapp_native/init.go b/pkg/channels/whatsapp_native/init.go index 2deee2297..eef8174aa 100644 --- a/pkg/channels/whatsapp_native/init.go +++ b/pkg/channels/whatsapp_native/init.go @@ -11,6 +11,9 @@ import ( func init() { channels.RegisterFactory("whatsapp_native", func(cfg *config.Config, b *bus.MessageBus) (channels.Channel, error) { waCfg := cfg.Channels.WhatsApp + if !waCfg.Enabled || !waCfg.UseNative { + return nil, nil + } storePath := waCfg.SessionStorePath if storePath == "" { storePath = filepath.Join(cfg.WorkspacePath(), "whatsapp") From 43951f24b76d26391df3b9b2afdde7062edeb7f0 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Mon, 13 Apr 2026 15:00:28 +0200 Subject: [PATCH 2/2] fix(lint): fix gci import order in manager_sushi30_test.go Co-Authored-By: Claude Sonnet 4.6 --- pkg/channels/manager_sushi30_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/channels/manager_sushi30_test.go b/pkg/channels/manager_sushi30_test.go index 16fa4cc95..a0be189d1 100644 --- a/pkg/channels/manager_sushi30_test.go +++ b/pkg/channels/manager_sushi30_test.go @@ -5,9 +5,8 @@ import ( "github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/channels" - "github.com/sipeed/picoclaw/pkg/config" - _ "github.com/sipeed/picoclaw/pkg/channels/email" + "github.com/sipeed/picoclaw/pkg/config" ) // TestInitChannels_EmailPickedUp verifies that a channel with Enabled=true is