From c2fc9f3e2a2cbba99f824674e19169c3beda7865 Mon Sep 17 00:00:00 2001 From: mingmxren Date: Tue, 3 Mar 2026 23:38:42 +0800 Subject: [PATCH] refactor(commands): address code review findings - Remove dead ExecuteResult.Reply field and unused branch in loop.go - Extract shared agentsHandler for /show agents and /list agents - Remove redundant firstToken/secondToken (use nthToken instead) - Simplify Telegram startup: pass BuiltinDefinitions directly - Centralize req.Reply nil guard in executeDefinition - Extract unavailableMsg constant (was duplicated 5 times) - Remove unused MessageID from Request - Remove stale "reserved for Phase 2" comment on Deps.Config Co-Authored-By: Claude Opus 4.6 --- pkg/agent/loop.go | 3 --- pkg/channels/telegram/telegram.go | 2 +- pkg/commands/cmd_agents.go | 21 ++++++++++++++++ pkg/commands/cmd_help.go | 3 --- pkg/commands/cmd_list.go | 24 +++--------------- pkg/commands/cmd_show.go | 23 ++--------------- pkg/commands/cmd_start.go | 3 --- pkg/commands/cmd_switch.go | 10 ++------ pkg/commands/deps.go | 2 +- pkg/commands/dispatcher.go | 33 +++++++------------------ pkg/commands/executor.go | 16 ++++++------ pkg/commands/show_list_handlers_test.go | 3 --- 12 files changed, 47 insertions(+), 96 deletions(-) create mode 100644 pkg/commands/cmd_agents.go diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 7555fc882..4f986870e 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -1533,9 +1533,6 @@ func (al *AgentLoop) handleCommand( if commandReply != "" { return commandReply, true } - if result.Reply != "" { - return result.Reply, true - } return "", true default: return "", false diff --git a/pkg/channels/telegram/telegram.go b/pkg/channels/telegram/telegram.go index f5b8278c6..afd0a8510 100644 --- a/pkg/channels/telegram/telegram.go +++ b/pkg/channels/telegram/telegram.go @@ -130,7 +130,7 @@ func (c *TelegramChannel) Start(ctx context.Context) error { "username": c.bot.Username(), }) - c.startCommandRegistration(c.ctx, commands.NewRegistry(commands.BuiltinDefinitions(&commands.Deps{Config: c.config})).Definitions()) + c.startCommandRegistration(c.ctx, commands.BuiltinDefinitions(&commands.Deps{Config: c.config})) go func() { if err = bh.Start(); err != nil { diff --git a/pkg/commands/cmd_agents.go b/pkg/commands/cmd_agents.go new file mode 100644 index 000000000..55ea2190d --- /dev/null +++ b/pkg/commands/cmd_agents.go @@ -0,0 +1,21 @@ +package commands + +import ( + "context" + "fmt" + "strings" +) + +// agentsHandler returns a shared handler for both /show agents and /list agents. +func agentsHandler(deps *Deps) Handler { + return func(_ context.Context, req Request) error { + if deps.ListAgentIDs == nil { + return req.Reply(unavailableMsg) + } + 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_help.go b/pkg/commands/cmd_help.go index 6bce6431c..0bbcfc0af 100644 --- a/pkg/commands/cmd_help.go +++ b/pkg/commands/cmd_help.go @@ -12,9 +12,6 @@ func helpCommand(deps *Deps) Definition { 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)) }, diff --git a/pkg/commands/cmd_list.go b/pkg/commands/cmd_list.go index c7ad13b34..cd85e57b8 100644 --- a/pkg/commands/cmd_list.go +++ b/pkg/commands/cmd_list.go @@ -15,11 +15,8 @@ func listCommand(deps *Deps) Definition { 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.") + return req.Reply(unavailableMsg) } name, provider := deps.GetModelInfo() if provider == "" { @@ -35,11 +32,8 @@ func listCommand(deps *Deps) Definition { 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.") + return req.Reply(unavailableMsg) } enabled := deps.GetEnabledChannels() if len(enabled) == 0 { @@ -51,19 +45,7 @@ func listCommand(deps *Deps) Definition { { 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, ", "))) - }, + Handler: agentsHandler(deps), }, }, } diff --git a/pkg/commands/cmd_show.go b/pkg/commands/cmd_show.go index 16931977e..1ec3ca5a9 100644 --- a/pkg/commands/cmd_show.go +++ b/pkg/commands/cmd_show.go @@ -3,7 +3,6 @@ package commands import ( "context" "fmt" - "strings" ) func showCommand(deps *Deps) Definition { @@ -15,11 +14,8 @@ func showCommand(deps *Deps) Definition { 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.") + return req.Reply(unavailableMsg) } name, provider := deps.GetModelInfo() return req.Reply(fmt.Sprintf("Current Model: %s (Provider: %s)", name, provider)) @@ -29,28 +25,13 @@ func showCommand(deps *Deps) Definition { 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, ", "))) - }, + Handler: agentsHandler(deps), }, }, } diff --git a/pkg/commands/cmd_start.go b/pkg/commands/cmd_start.go index 48c4cc4e2..e1efd6613 100644 --- a/pkg/commands/cmd_start.go +++ b/pkg/commands/cmd_start.go @@ -8,9 +8,6 @@ func startCommand() Definition { 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/cmd_switch.go b/pkg/commands/cmd_switch.go index 619197fec..19a6b94d8 100644 --- a/pkg/commands/cmd_switch.go +++ b/pkg/commands/cmd_switch.go @@ -15,11 +15,8 @@ func switchCommand(deps *Deps) Definition { Description: "Switch to a different model", ArgsUsage: "to ", Handler: func(_ context.Context, req Request) error { - if req.Reply == nil { - return nil - } if deps.SwitchModel == nil { - return req.Reply("Command unavailable in current context.") + return req.Reply(unavailableMsg) } // Parse: /switch model to value := nthToken(req.Text, 3) // tokens: [/switch, model, to, ] @@ -38,11 +35,8 @@ func switchCommand(deps *Deps) Definition { Description: "Switch to a different channel", ArgsUsage: "to ", Handler: func(_ context.Context, req Request) error { - if req.Reply == nil { - return nil - } if deps.SwitchChannel == nil { - return req.Reply("Command unavailable in current context.") + return req.Reply(unavailableMsg) } value := nthToken(req.Text, 3) if nthToken(req.Text, 2) != "to" || value == "" { diff --git a/pkg/commands/deps.go b/pkg/commands/deps.go index 9db6711f9..af1a63fe3 100644 --- a/pkg/commands/deps.go +++ b/pkg/commands/deps.go @@ -7,7 +7,7 @@ import "github.com/sipeed/picoclaw/pkg/config" // invocation time, not at construction time, so late-bound values // (e.g. channelManager set after NewAgentLoop) are visible. type Deps struct { - Config *config.Config // reserved for Phase 2 session commands + Config *config.Config GetModelInfo func() (name, provider string) ListAgentIDs func() []string GetEnabledChannels func() []string diff --git a/pkg/commands/dispatcher.go b/pkg/commands/dispatcher.go index 369d5cc2d..2b1bbc806 100644 --- a/pkg/commands/dispatcher.go +++ b/pkg/commands/dispatcher.go @@ -8,28 +8,21 @@ import ( type Handler func(ctx context.Context, req Request) error type Request struct { - Channel string - ChatID string - SenderID string - Text string - MessageID string - Reply func(text string) error + Channel string + ChatID string + SenderID string + Text string + Reply func(text string) error } +const unavailableMsg = "Command unavailable in current context." + var commandPrefixes = []string{"/", "!"} -func firstToken(input string) string { - parts := strings.Fields(strings.TrimSpace(input)) - if len(parts) == 0 { - return "" - } - return parts[0] -} - // parseCommandName accepts "/name", "!name", and Telegram's "/name@bot", then // normalizes to lowercase command names. func parseCommandName(input string) (string, bool) { - token := firstToken(input) + token := nthToken(input, 0) if token == "" { return "", false } @@ -57,18 +50,10 @@ func trimCommandPrefix(token string) (string, bool) { return "", false } -func secondToken(input string) string { - parts := strings.Fields(strings.TrimSpace(input)) - if len(parts) < 2 { - return "" - } - return parts[1] -} - // HasCommandPrefix returns true if the input starts with a recognized // command prefix (e.g. "/" or "!"). func HasCommandPrefix(input string) bool { - token := firstToken(input) + token := nthToken(input, 0) if token == "" { return false } diff --git a/pkg/commands/executor.go b/pkg/commands/executor.go index 9dcaef672..72b9c9ccb 100644 --- a/pkg/commands/executor.go +++ b/pkg/commands/executor.go @@ -17,7 +17,6 @@ const ( type ExecuteResult struct { Outcome Outcome Command string - Reply string Err error } @@ -51,6 +50,11 @@ func (e *Executor) Execute(ctx context.Context, req Request) ExecuteResult { } func (e *Executor) executeDefinition(ctx context.Context, req Request, def Definition) ExecuteResult { + // Ensure Reply is always non-nil so handlers don't need to check. + if req.Reply == nil { + req.Reply = func(string) error { return nil } + } + // Simple command — no sub-commands if len(def.SubCommands) == 0 { if def.Handler == nil { @@ -61,11 +65,9 @@ func (e *Executor) executeDefinition(ctx context.Context, req Request, def Defin } // Sub-command routing - subName := secondToken(req.Text) + subName := nthToken(req.Text, 1) if subName == "" { - if req.Reply != nil { - _ = req.Reply("Usage: " + def.EffectiveUsage()) - } + _ = req.Reply("Usage: " + def.EffectiveUsage()) return ExecuteResult{Outcome: OutcomeHandled, Command: def.Name} } @@ -81,8 +83,6 @@ func (e *Executor) executeDefinition(ctx context.Context, req Request, def Defin } // Unknown sub-command - if req.Reply != nil { - _ = req.Reply(fmt.Sprintf("Unknown parameter: %s. Usage: %s", subName, def.EffectiveUsage())) - } + _ = req.Reply(fmt.Sprintf("Unknown parameter: %s. Usage: %s", subName, def.EffectiveUsage())) return ExecuteResult{Outcome: OutcomeHandled, Command: def.Name} } diff --git a/pkg/commands/show_list_handlers_test.go b/pkg/commands/show_list_handlers_test.go index bd3c44763..42ce4fa10 100644 --- a/pkg/commands/show_list_handlers_test.go +++ b/pkg/commands/show_list_handlers_test.go @@ -55,9 +55,6 @@ func TestShowListHandlers_ChannelPolicy(t *testing.T) { if passthrough.Command != "foo" { t.Fatalf("whatsapp /foo command=%q, want=%q", passthrough.Command, "foo") } - if passthrough.Reply != "" { - t.Fatalf("whatsapp /foo reply=%q, want empty", passthrough.Reply) - } } func TestShowListHandlers_ListHandledOnAllChannels(t *testing.T) {