From ba7ec072cd7e44eb8e9a2e112a56a48c6dcd4f1c Mon Sep 17 00:00:00 2001 From: mingmxren Date: Wed, 4 Mar 2026 12:50:57 +0800 Subject: [PATCH] refactor(commands): address code review findings on naming and correctness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename dispatcher.go → request.go (no Dispatcher type remains) - Rename cmd_agents.go → handler_agents.go (shared handler, not a top-level command) - Add modelMu to protect AgentInstance.Model writes in SwitchModel - Add ListDefinitions to Runtime so /help uses registry instead of BuiltinDefinitions() - Fix SwitchChannel message: validation-only callback should not say "Switched" - Propagate Reply errors in executor instead of discarding with _ = - Add HasCommandPrefix unit test Co-Authored-By: Claude Opus 4.6 --- pkg/agent/loop.go | 6 +++- pkg/commands/cmd_help.go | 9 ++++-- pkg/commands/cmd_switch.go | 2 +- pkg/commands/cmd_switch_test.go | 2 +- pkg/commands/executor.go | 8 +++--- .../{cmd_agents.go => handler_agents.go} | 0 pkg/commands/{dispatcher.go => request.go} | 0 pkg/commands/request_test.go | 28 +++++++++++++++++++ pkg/commands/runtime.go | 1 + 9 files changed, 47 insertions(+), 9 deletions(-) rename pkg/commands/{cmd_agents.go => handler_agents.go} (100%) rename pkg/commands/{dispatcher.go => request.go} (100%) create mode 100644 pkg/commands/request_test.go diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index cdca8bb56..f51f38f6a 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -48,6 +48,7 @@ type AgentLoop struct { mediaStore media.MediaStore transcriber voice.Transcriber cmdRegistry *commands.Registry + modelMu sync.Mutex // protects AgentInstance.Model writes in SwitchModel } // processOptions configures how a message is processed @@ -1509,7 +1510,8 @@ func (al *AgentLoop) buildCommandsRuntime() *commands.Runtime { } return agent.Model, al.cfg.Agents.Defaults.Provider }, - ListAgentIDs: al.registry.ListAgentIDs, + ListAgentIDs: al.registry.ListAgentIDs, + ListDefinitions: al.cmdRegistry.Definitions, GetEnabledChannels: func() []string { if al.channelManager == nil { return nil @@ -1517,6 +1519,8 @@ func (al *AgentLoop) buildCommandsRuntime() *commands.Runtime { return al.channelManager.GetEnabledChannels() }, SwitchModel: func(value string) (string, error) { + al.modelMu.Lock() + defer al.modelMu.Unlock() defaultAgent := al.registry.GetDefaultAgent() if defaultAgent == nil { return "", fmt.Errorf("no default agent configured") diff --git a/pkg/commands/cmd_help.go b/pkg/commands/cmd_help.go index deca55bab..94f7f0101 100644 --- a/pkg/commands/cmd_help.go +++ b/pkg/commands/cmd_help.go @@ -11,8 +11,13 @@ func helpCommand() Definition { Name: "help", Description: "Show this help message", Usage: "/help", - Handler: func(_ context.Context, req Request, _ *Runtime) error { - defs := BuiltinDefinitions() + Handler: func(_ context.Context, req Request, rt *Runtime) error { + var defs []Definition + if rt != nil && rt.ListDefinitions != nil { + defs = rt.ListDefinitions() + } else { + defs = BuiltinDefinitions() + } return req.Reply(formatHelpMessage(defs)) }, } diff --git a/pkg/commands/cmd_switch.go b/pkg/commands/cmd_switch.go index c0a3d7970..ca4057260 100644 --- a/pkg/commands/cmd_switch.go +++ b/pkg/commands/cmd_switch.go @@ -45,7 +45,7 @@ func switchCommand() Definition { if err := rt.SwitchChannel(value); err != nil { return req.Reply(err.Error()) } - return req.Reply(fmt.Sprintf("Switched target channel to %s", value)) + return req.Reply(fmt.Sprintf("Channel '%s' is available and enabled", value)) }, }, }, diff --git a/pkg/commands/cmd_switch_test.go b/pkg/commands/cmd_switch_test.go index 088beaed4..419d1d82d 100644 --- a/pkg/commands/cmd_switch_test.go +++ b/pkg/commands/cmd_switch_test.go @@ -141,7 +141,7 @@ func TestSwitchChannel_Success(t *testing.T) { if res.Outcome != OutcomeHandled { t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled) } - want := "Switched target channel to telegram" + want := "Channel 'telegram' is available and enabled" if reply != want { t.Fatalf("reply=%q, want=%q", reply, want) } diff --git a/pkg/commands/executor.go b/pkg/commands/executor.go index f2910fcab..03634cd19 100644 --- a/pkg/commands/executor.go +++ b/pkg/commands/executor.go @@ -68,8 +68,8 @@ func (e *Executor) executeDefinition(ctx context.Context, req Request, def Defin // Sub-command routing subName := nthToken(req.Text, 1) if subName == "" { - _ = req.Reply("Usage: " + def.EffectiveUsage()) - return ExecuteResult{Outcome: OutcomeHandled, Command: def.Name} + err := req.Reply("Usage: " + def.EffectiveUsage()) + return ExecuteResult{Outcome: OutcomeHandled, Command: def.Name, Err: err} } normalized := normalizeCommandName(subName) @@ -84,6 +84,6 @@ func (e *Executor) executeDefinition(ctx context.Context, req Request, def Defin } // Unknown sub-command - _ = req.Reply(fmt.Sprintf("Unknown parameter: %s. Usage: %s", subName, def.EffectiveUsage())) - return ExecuteResult{Outcome: OutcomeHandled, Command: def.Name} + err := req.Reply(fmt.Sprintf("Unknown parameter: %s. Usage: %s", subName, def.EffectiveUsage())) + return ExecuteResult{Outcome: OutcomeHandled, Command: def.Name, Err: err} } diff --git a/pkg/commands/cmd_agents.go b/pkg/commands/handler_agents.go similarity index 100% rename from pkg/commands/cmd_agents.go rename to pkg/commands/handler_agents.go diff --git a/pkg/commands/dispatcher.go b/pkg/commands/request.go similarity index 100% rename from pkg/commands/dispatcher.go rename to pkg/commands/request.go diff --git a/pkg/commands/request_test.go b/pkg/commands/request_test.go new file mode 100644 index 000000000..4389e453b --- /dev/null +++ b/pkg/commands/request_test.go @@ -0,0 +1,28 @@ +package commands + +import "testing" + +func TestHasCommandPrefix(t *testing.T) { + tests := []struct { + input string + want bool + }{ + {"/help", true}, + {"!help", true}, + {"/switch model to gpt-4", true}, + {"!switch model to gpt-4", true}, + {"hello", false}, + {"", false}, + {" ", false}, + {"hello /world", false}, + {"/", true}, + {"!", true}, + {" /help", true}, + } + for _, tt := range tests { + got := HasCommandPrefix(tt.input) + if got != tt.want { + t.Errorf("HasCommandPrefix(%q) = %v, want %v", tt.input, got, tt.want) + } + } +} diff --git a/pkg/commands/runtime.go b/pkg/commands/runtime.go index a1da81e06..227d495f4 100644 --- a/pkg/commands/runtime.go +++ b/pkg/commands/runtime.go @@ -9,6 +9,7 @@ type Runtime struct { Config *config.Config GetModelInfo func() (name, provider string) ListAgentIDs func() []string + ListDefinitions func() []Definition GetEnabledChannels func() []string SwitchModel func(value string) (oldModel string, err error) SwitchChannel func(value string) error