From a10037426017f8b8ba6a634144069ac93885bb11 Mon Sep 17 00:00:00 2001 From: mingmxren Date: Tue, 3 Mar 2026 22:54:41 +0800 Subject: [PATCH] refactor(commands): split into command-group files with Deps injection Extract show/list/start/help into individual cmd_*.go files. Replace config.Config parameter with Deps struct for runtime data. Restore /show agents and /list agents sub-commands. Use EffectiveUsage for auto-generated help text. Bridge external callers (agent/loop.go, telegram.go) with Deps wrapper until Task 5 fully wires the Deps fields. Co-Authored-By: Claude Opus 4.6 --- pkg/agent/loop.go | 2 +- pkg/channels/telegram/telegram.go | 2 +- pkg/commands/builtin.go | 165 +----------------------- pkg/commands/builtin_test.go | 98 ++++++++++---- pkg/commands/cmd_help.go | 42 ++++++ pkg/commands/cmd_list.go | 70 ++++++++++ pkg/commands/cmd_show.go | 57 ++++++++ pkg/commands/cmd_start.go | 17 +++ pkg/commands/show_list_handlers_test.go | 15 ++- 9 files changed, 276 insertions(+), 192 deletions(-) create mode 100644 pkg/commands/cmd_help.go create mode 100644 pkg/commands/cmd_list.go create mode 100644 pkg/commands/cmd_show.go create mode 100644 pkg/commands/cmd_start.go diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 8822d75ff..818478fa3 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -1467,7 +1467,7 @@ func (al *AgentLoop) handleCommand( return "", false } - executor := commands.NewExecutor(commands.NewRegistry(commands.BuiltinDefinitions(al.cfg))) + executor := commands.NewExecutor(commands.NewRegistry(commands.BuiltinDefinitions(&commands.Deps{Config: al.cfg}))) var commandReply string result := executor.Execute(ctx, commands.Request{ diff --git a/pkg/channels/telegram/telegram.go b/pkg/channels/telegram/telegram.go index 136dff1ac..ff22ab821 100644 --- a/pkg/channels/telegram/telegram.go +++ b/pkg/channels/telegram/telegram.go @@ -137,7 +137,7 @@ func (c *TelegramChannel) Start(ctx context.Context) error { "username": c.bot.Username(), }) - c.startCommandRegistration(c.ctx, commands.NewRegistry(commands.BuiltinDefinitions(c.config)).Definitions()) + c.startCommandRegistration(c.ctx, commands.NewRegistry(commands.BuiltinDefinitions(&commands.Deps{Config: c.config})).Definitions()) go func() { if err = bh.Start(); err != nil { diff --git a/pkg/commands/builtin.go b/pkg/commands/builtin.go index e80e63821..e746aaf9d 100644 --- a/pkg/commands/builtin.go +++ b/pkg/commands/builtin.go @@ -1,163 +1,12 @@ package commands -import ( - "context" - "fmt" - "strings" - - "github.com/sipeed/picoclaw/pkg/config" -) - -func BuiltinDefinitions(cfg *config.Config) []Definition { +// BuiltinDefinitions returns all built-in command definitions. +// Each command group is defined in its own cmd_*.go file. +func BuiltinDefinitions(deps *Deps) []Definition { return []Definition{ - { - Name: "start", - Description: "Start the bot", - Usage: "/start", - Handler: replyText("Hello! I am PicoClaw 🦞"), - }, - { - Name: "help", - Description: "Show this help message", - Usage: "/help", - Handler: func(_ context.Context, req Request) error { - if req.Reply == nil { - return nil - } - defs := NewRegistry(BuiltinDefinitions(cfg)).Definitions() - return req.Reply(FormatHelpMessage(defs)) - }, - }, - { - Name: "show", - Description: "Show current configuration", - Usage: "/show [model|channel]", - Handler: func(_ context.Context, req Request) error { - if req.Reply == nil { - return nil - } - if cfg == nil { - return req.Reply("Command unavailable in current context.") - } - args := commandArgs(req.Text) - if args == "" { - return req.Reply("Usage: /show [model|channel]") - } - - switch args { - case "model": - return req.Reply(fmt.Sprintf( - "Current Model: %s (Provider: %s)", - cfg.Agents.Defaults.GetModelName(), - cfg.Agents.Defaults.Provider, - )) - case "channel": - return req.Reply(fmt.Sprintf("Current Channel: %s", req.Channel)) - default: - return req.Reply(fmt.Sprintf("Unknown parameter: %s. Try 'model' or 'channel'.", args)) - } - }, - }, - { - Name: "list", - Description: "List available options", - Usage: "/list [models|channels]", - Handler: func(_ context.Context, req Request) error { - if req.Reply == nil { - return nil - } - if cfg == nil { - return req.Reply("Command unavailable in current context.") - } - args := commandArgs(req.Text) - if args == "" { - return req.Reply("Usage: /list [models|channels]") - } - - switch args { - case "models": - provider := cfg.Agents.Defaults.Provider - if provider == "" { - provider = "configured default" - } - return req.Reply(fmt.Sprintf( - "Configured Model: %s\nProvider: %s\n\nTo change models, update config.json", - cfg.Agents.Defaults.GetModelName(), - provider, - )) - case "channels": - enabled := enabledChannels(cfg) - return req.Reply(fmt.Sprintf("Enabled Channels:\n- %s", strings.Join(enabled, "\n- "))) - default: - return req.Reply(fmt.Sprintf("Unknown parameter: %s. Try 'models' or 'channels'.", args)) - } - }, - }, + startCommand(), + helpCommand(deps), + showCommand(deps), + listCommand(deps), } } - -func FormatHelpMessage(defs []Definition) string { - if len(defs) == 0 { - return "No commands available." - } - - lines := make([]string, 0, len(defs)) - for _, def := range defs { - usage := def.Usage - if usage == "" { - usage = "/" + def.Name - } - desc := def.Description - if desc == "" { - desc = "No description" - } - lines = append(lines, fmt.Sprintf("%s - %s", usage, desc)) - } - return strings.Join(lines, "\n") -} - -func commandArgs(text string) string { - parts := strings.SplitN(text, " ", 2) - if len(parts) < 2 { - return "" - } - return strings.TrimSpace(parts[1]) -} - -func replyText(text string) Handler { - return func(_ context.Context, req Request) error { - if req.Reply == nil { - return nil - } - return req.Reply(text) - } -} - -func enabledChannels(cfg *config.Config) []string { - enabled := make([]string, 0, 8) - if cfg.Channels.Telegram.Enabled { - enabled = append(enabled, "telegram") - } - if cfg.Channels.WhatsApp.Enabled { - enabled = append(enabled, "whatsapp") - } - if cfg.Channels.Feishu.Enabled { - enabled = append(enabled, "feishu") - } - if cfg.Channels.Discord.Enabled { - enabled = append(enabled, "discord") - } - if cfg.Channels.Slack.Enabled { - enabled = append(enabled, "slack") - } - if cfg.Channels.DingTalk.Enabled { - enabled = append(enabled, "dingtalk") - } - if cfg.Channels.LINE.Enabled { - enabled = append(enabled, "line") - } - if cfg.Channels.OneBot.Enabled { - enabled = append(enabled, "onebot") - } - return enabled -} diff --git a/pkg/commands/builtin_test.go b/pkg/commands/builtin_test.go index adbbd835b..dbff65e5e 100644 --- a/pkg/commands/builtin_test.go +++ b/pkg/commands/builtin_test.go @@ -4,8 +4,6 @@ import ( "context" "strings" "testing" - - "github.com/sipeed/picoclaw/pkg/config" ) func findDefinitionByName(t *testing.T, defs []Definition, name string) Definition { @@ -20,7 +18,8 @@ func findDefinitionByName(t *testing.T, defs []Definition, name string) Definiti } func TestBuiltinHelpHandler_ReturnsFormattedMessage(t *testing.T) { - defs := BuiltinDefinitions(nil) + deps := &Deps{} + defs := BuiltinDefinitions(deps) helpDef := findDefinitionByName(t, defs, "help") if helpDef.Handler == nil { t.Fatalf("/help handler should not be nil") @@ -37,25 +36,26 @@ func TestBuiltinHelpHandler_ReturnsFormattedMessage(t *testing.T) { if err != nil { t.Fatalf("/help handler error: %v", err) } - if !strings.Contains(reply, "/show [model|channel] - Show current configuration") { + // Now uses auto-generated EffectiveUsage which includes agents + if !strings.Contains(reply, "/show [model|channel|agents]") { t.Fatalf("/help reply missing /show usage, got %q", reply) } - if !strings.Contains(reply, "/list [models|channels] - List available options") { + if !strings.Contains(reply, "/list [models|channels|agents]") { t.Fatalf("/help reply missing /list usage, got %q", reply) } } func TestBuiltinShowChannel_PreservesUserVisibleBehavior(t *testing.T) { - defs := BuiltinDefinitions(&config.Config{}) - showDef := findDefinitionByName(t, defs, "show") - if showDef.Handler == nil { - t.Fatalf("/show handler should not be nil") - } + deps := &Deps{} + defs := BuiltinDefinitions(deps) + + // show now uses sub-commands, so we need the executor to route + ex := NewExecutor(NewRegistry(defs)) cases := []string{"telegram", "whatsapp"} for _, channel := range cases { var reply string - err := showDef.Handler(context.Background(), Request{ + res := ex.Execute(context.Background(), Request{ Channel: channel, Text: "/show channel", Reply: func(text string) error { @@ -63,8 +63,8 @@ func TestBuiltinShowChannel_PreservesUserVisibleBehavior(t *testing.T) { return nil }, }) - if err != nil { - t.Fatalf("/show channel handler error on %s: %v", channel, err) + if res.Outcome != OutcomeHandled { + t.Fatalf("/show channel on %s: outcome=%v, want=%v", channel, res.Outcome, OutcomeHandled) } want := "Current Channel: " + channel if reply != want { @@ -73,29 +73,77 @@ func TestBuiltinShowChannel_PreservesUserVisibleBehavior(t *testing.T) { } } -func TestBuiltinListChannels_UsesConfigEnabledChannels(t *testing.T) { - cfg := &config.Config{} - cfg.Channels.Telegram.Enabled = true - cfg.Channels.Slack.Enabled = true - - defs := BuiltinDefinitions(cfg) - listDef := findDefinitionByName(t, defs, "list") - if listDef.Handler == nil { - t.Fatalf("/list handler should not be nil") +func TestBuiltinListChannels_UsesGetEnabledChannels(t *testing.T) { + deps := &Deps{ + GetEnabledChannels: func() []string { + return []string{"telegram", "slack"} + }, } + defs := BuiltinDefinitions(deps) + ex := NewExecutor(NewRegistry(defs)) var reply string - err := listDef.Handler(context.Background(), Request{ + res := ex.Execute(context.Background(), Request{ Text: "/list channels", Reply: func(text string) error { reply = text return nil }, }) - if err != nil { - t.Fatalf("/list channels handler error: %v", err) + if res.Outcome != OutcomeHandled { + t.Fatalf("/list channels: outcome=%v, want=%v", res.Outcome, OutcomeHandled) } if !strings.Contains(reply, "telegram") || !strings.Contains(reply, "slack") { t.Fatalf("/list channels reply=%q, want telegram and slack", reply) } } + +func TestBuiltinShowAgents_RestoresOldBehavior(t *testing.T) { + deps := &Deps{ + ListAgentIDs: func() []string { + return []string{"default", "coder"} + }, + } + defs := BuiltinDefinitions(deps) + ex := NewExecutor(NewRegistry(defs)) + + var reply string + res := ex.Execute(context.Background(), Request{ + Text: "/show agents", + Reply: func(text string) error { + reply = text + return nil + }, + }) + if res.Outcome != OutcomeHandled { + t.Fatalf("/show agents: outcome=%v, want=%v", res.Outcome, OutcomeHandled) + } + if !strings.Contains(reply, "default") || !strings.Contains(reply, "coder") { + t.Fatalf("/show agents reply=%q, want agent IDs", reply) + } +} + +func TestBuiltinListAgents_RestoresOldBehavior(t *testing.T) { + deps := &Deps{ + ListAgentIDs: func() []string { + return []string{"default", "coder"} + }, + } + defs := BuiltinDefinitions(deps) + ex := NewExecutor(NewRegistry(defs)) + + var reply string + res := ex.Execute(context.Background(), Request{ + Text: "/list agents", + Reply: func(text string) error { + reply = text + return nil + }, + }) + if res.Outcome != OutcomeHandled { + t.Fatalf("/list agents: outcome=%v, want=%v", res.Outcome, OutcomeHandled) + } + if !strings.Contains(reply, "default") || !strings.Contains(reply, "coder") { + t.Fatalf("/list agents reply=%q, want agent IDs", reply) + } +} diff --git a/pkg/commands/cmd_help.go b/pkg/commands/cmd_help.go new file mode 100644 index 000000000..6bce6431c --- /dev/null +++ b/pkg/commands/cmd_help.go @@ -0,0 +1,42 @@ +package commands + +import ( + "context" + "fmt" + "strings" +) + +func helpCommand(deps *Deps) Definition { + return Definition{ + Name: "help", + Description: "Show this help message", + Usage: "/help", + Handler: func(_ context.Context, req Request) error { + if req.Reply == nil { + return nil + } + defs := BuiltinDefinitions(deps) + return req.Reply(formatHelpMessage(defs)) + }, + } +} + +func formatHelpMessage(defs []Definition) string { + if len(defs) == 0 { + return "No commands available." + } + + lines := make([]string, 0, len(defs)) + for _, def := range defs { + usage := def.EffectiveUsage() + if usage == "" { + usage = "/" + def.Name + } + desc := def.Description + if desc == "" { + desc = "No description" + } + lines = append(lines, fmt.Sprintf("%s - %s", usage, desc)) + } + return strings.Join(lines, "\n") +} diff --git a/pkg/commands/cmd_list.go b/pkg/commands/cmd_list.go new file mode 100644 index 000000000..c7ad13b34 --- /dev/null +++ b/pkg/commands/cmd_list.go @@ -0,0 +1,70 @@ +package commands + +import ( + "context" + "fmt" + "strings" +) + +func listCommand(deps *Deps) Definition { + return Definition{ + Name: "list", + Description: "List available options", + SubCommands: []SubCommand{ + { + Name: "models", + Description: "Configured models", + Handler: func(_ context.Context, req Request) error { + if req.Reply == nil { + return nil + } + if deps.GetModelInfo == nil { + return req.Reply("Command unavailable in current context.") + } + name, provider := deps.GetModelInfo() + if provider == "" { + provider = "configured default" + } + return req.Reply(fmt.Sprintf( + "Configured Model: %s\nProvider: %s\n\nTo change models, update config.json", + name, provider, + )) + }, + }, + { + Name: "channels", + Description: "Enabled channels", + Handler: func(_ context.Context, req Request) error { + if req.Reply == nil { + return nil + } + if deps.GetEnabledChannels == nil { + return req.Reply("Command unavailable in current context.") + } + enabled := deps.GetEnabledChannels() + if len(enabled) == 0 { + return req.Reply("No channels enabled") + } + return req.Reply(fmt.Sprintf("Enabled Channels:\n- %s", strings.Join(enabled, "\n- "))) + }, + }, + { + Name: "agents", + Description: "Registered agents", + Handler: func(_ context.Context, req Request) error { + if req.Reply == nil { + return nil + } + if deps.ListAgentIDs == nil { + return req.Reply("Command unavailable in current context.") + } + ids := deps.ListAgentIDs() + if len(ids) == 0 { + return req.Reply("No agents registered") + } + return req.Reply(fmt.Sprintf("Registered agents: %s", strings.Join(ids, ", "))) + }, + }, + }, + } +} diff --git a/pkg/commands/cmd_show.go b/pkg/commands/cmd_show.go new file mode 100644 index 000000000..16931977e --- /dev/null +++ b/pkg/commands/cmd_show.go @@ -0,0 +1,57 @@ +package commands + +import ( + "context" + "fmt" + "strings" +) + +func showCommand(deps *Deps) Definition { + return Definition{ + Name: "show", + Description: "Show current configuration", + SubCommands: []SubCommand{ + { + Name: "model", + Description: "Current model and provider", + Handler: func(_ context.Context, req Request) error { + if req.Reply == nil { + return nil + } + if deps.GetModelInfo == nil { + return req.Reply("Command unavailable in current context.") + } + name, provider := deps.GetModelInfo() + return req.Reply(fmt.Sprintf("Current Model: %s (Provider: %s)", name, provider)) + }, + }, + { + Name: "channel", + Description: "Current channel", + Handler: func(_ context.Context, req Request) error { + if req.Reply == nil { + return nil + } + return req.Reply(fmt.Sprintf("Current Channel: %s", req.Channel)) + }, + }, + { + Name: "agents", + Description: "Registered agents", + Handler: func(_ context.Context, req Request) error { + if req.Reply == nil { + return nil + } + if deps.ListAgentIDs == nil { + return req.Reply("Command unavailable in current context.") + } + ids := deps.ListAgentIDs() + if len(ids) == 0 { + return req.Reply("No agents registered") + } + return req.Reply(fmt.Sprintf("Registered agents: %s", strings.Join(ids, ", "))) + }, + }, + }, + } +} diff --git a/pkg/commands/cmd_start.go b/pkg/commands/cmd_start.go new file mode 100644 index 000000000..48c4cc4e2 --- /dev/null +++ b/pkg/commands/cmd_start.go @@ -0,0 +1,17 @@ +package commands + +import "context" + +func startCommand() Definition { + return Definition{ + Name: "start", + Description: "Start the bot", + Usage: "/start", + Handler: func(_ context.Context, req Request) error { + if req.Reply == nil { + return nil + } + return req.Reply("Hello! I am PicoClaw 🦞") + }, + } +} diff --git a/pkg/commands/show_list_handlers_test.go b/pkg/commands/show_list_handlers_test.go index bfea94b99..bd3c44763 100644 --- a/pkg/commands/show_list_handlers_test.go +++ b/pkg/commands/show_list_handlers_test.go @@ -4,13 +4,11 @@ import ( "context" "strings" "testing" - - "github.com/sipeed/picoclaw/pkg/config" ) func TestShowListHandlers_ChannelPolicy(t *testing.T) { - cfg := &config.Config{} - ex := NewExecutor(NewRegistry(BuiltinDefinitions(cfg))) + deps := &Deps{} + ex := NewExecutor(NewRegistry(BuiltinDefinitions(deps))) var telegramReply string handled := ex.Execute(context.Background(), Request{ @@ -63,9 +61,12 @@ func TestShowListHandlers_ChannelPolicy(t *testing.T) { } func TestShowListHandlers_ListHandledOnAllChannels(t *testing.T) { - cfg := &config.Config{} - cfg.Channels.Telegram.Enabled = true - ex := NewExecutor(NewRegistry(BuiltinDefinitions(cfg))) + deps := &Deps{ + GetEnabledChannels: func() []string { + return []string{"telegram"} + }, + } + ex := NewExecutor(NewRegistry(BuiltinDefinitions(deps))) var reply string res := ex.Execute(context.Background(), Request{