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 <noreply@anthropic.com>

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

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 {

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, ", ")))
+	}
+}

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))
 		},

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 {
+				Handler:     agentsHandler(deps),
-					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, ", ")))
-				},
 			},
 		},
 	}

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 {
+				Handler:     agentsHandler(deps),
-					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, ", ")))
-				},
 			},
 		},
 	}

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 🦞")
 		},
 	}

pkg/commands/cmd_switch.go

@@ -15,11 +15,8 @@ func switchCommand(deps *Deps) Definition {
 				Description: "Switch to a different model",
 				ArgsUsage:   "to <name>",
 				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>
 					value := nthToken(req.Text, 3) // tokens: [/switch, model, to, <value>]
@@ -38,11 +35,8 @@ func switchCommand(deps *Deps) Definition {
 				Description: "Switch to a different channel",
 				ArgsUsage:   "to <name>",
 				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 == "" {

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

pkg/commands/dispatcher.go

@@ -8,28 +8,21 @@ import (
 type Handler func(ctx context.Context, req Request) error
 
 type Request struct {
-	Channel   string
+	Channel  string
-	ChatID    string
+	ChatID   string
-	SenderID  string
+	SenderID string
-	Text      string
+	Text     string
-	MessageID string
+	Reply    func(text string) error
-	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
 	}

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}
 }

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) {