refactor(commands): address code review findings on naming and correctness
- 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 <noreply@anthropic.com>
This commit is contained in:
parent
7a43c3e065
commit
ba7ec072cd
9 changed files with 47 additions and 9 deletions
|
|
@ -48,6 +48,7 @@ type AgentLoop struct {
|
||||||
mediaStore media.MediaStore
|
mediaStore media.MediaStore
|
||||||
transcriber voice.Transcriber
|
transcriber voice.Transcriber
|
||||||
cmdRegistry *commands.Registry
|
cmdRegistry *commands.Registry
|
||||||
|
modelMu sync.Mutex // protects AgentInstance.Model writes in SwitchModel
|
||||||
}
|
}
|
||||||
|
|
||||||
// processOptions configures how a message is processed
|
// processOptions configures how a message is processed
|
||||||
|
|
@ -1510,6 +1511,7 @@ func (al *AgentLoop) buildCommandsRuntime() *commands.Runtime {
|
||||||
return agent.Model, al.cfg.Agents.Defaults.Provider
|
return agent.Model, al.cfg.Agents.Defaults.Provider
|
||||||
},
|
},
|
||||||
ListAgentIDs: al.registry.ListAgentIDs,
|
ListAgentIDs: al.registry.ListAgentIDs,
|
||||||
|
ListDefinitions: al.cmdRegistry.Definitions,
|
||||||
GetEnabledChannels: func() []string {
|
GetEnabledChannels: func() []string {
|
||||||
if al.channelManager == nil {
|
if al.channelManager == nil {
|
||||||
return nil
|
return nil
|
||||||
|
|
@ -1517,6 +1519,8 @@ func (al *AgentLoop) buildCommandsRuntime() *commands.Runtime {
|
||||||
return al.channelManager.GetEnabledChannels()
|
return al.channelManager.GetEnabledChannels()
|
||||||
},
|
},
|
||||||
SwitchModel: func(value string) (string, error) {
|
SwitchModel: func(value string) (string, error) {
|
||||||
|
al.modelMu.Lock()
|
||||||
|
defer al.modelMu.Unlock()
|
||||||
defaultAgent := al.registry.GetDefaultAgent()
|
defaultAgent := al.registry.GetDefaultAgent()
|
||||||
if defaultAgent == nil {
|
if defaultAgent == nil {
|
||||||
return "", fmt.Errorf("no default agent configured")
|
return "", fmt.Errorf("no default agent configured")
|
||||||
|
|
|
||||||
|
|
@ -11,8 +11,13 @@ func helpCommand() Definition {
|
||||||
Name: "help",
|
Name: "help",
|
||||||
Description: "Show this help message",
|
Description: "Show this help message",
|
||||||
Usage: "/help",
|
Usage: "/help",
|
||||||
Handler: func(_ context.Context, req Request, _ *Runtime) error {
|
Handler: func(_ context.Context, req Request, rt *Runtime) error {
|
||||||
defs := BuiltinDefinitions()
|
var defs []Definition
|
||||||
|
if rt != nil && rt.ListDefinitions != nil {
|
||||||
|
defs = rt.ListDefinitions()
|
||||||
|
} else {
|
||||||
|
defs = BuiltinDefinitions()
|
||||||
|
}
|
||||||
return req.Reply(formatHelpMessage(defs))
|
return req.Reply(formatHelpMessage(defs))
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -45,7 +45,7 @@ func switchCommand() Definition {
|
||||||
if err := rt.SwitchChannel(value); err != nil {
|
if err := rt.SwitchChannel(value); err != nil {
|
||||||
return req.Reply(err.Error())
|
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))
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
|
|
|
||||||
|
|
@ -141,7 +141,7 @@ func TestSwitchChannel_Success(t *testing.T) {
|
||||||
if res.Outcome != OutcomeHandled {
|
if res.Outcome != OutcomeHandled {
|
||||||
t.Fatalf("outcome=%v, want=%v", 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 {
|
if reply != want {
|
||||||
t.Fatalf("reply=%q, want=%q", reply, want)
|
t.Fatalf("reply=%q, want=%q", reply, want)
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -68,8 +68,8 @@ func (e *Executor) executeDefinition(ctx context.Context, req Request, def Defin
|
||||||
// Sub-command routing
|
// Sub-command routing
|
||||||
subName := nthToken(req.Text, 1)
|
subName := nthToken(req.Text, 1)
|
||||||
if subName == "" {
|
if subName == "" {
|
||||||
_ = req.Reply("Usage: " + def.EffectiveUsage())
|
err := req.Reply("Usage: " + def.EffectiveUsage())
|
||||||
return ExecuteResult{Outcome: OutcomeHandled, Command: def.Name}
|
return ExecuteResult{Outcome: OutcomeHandled, Command: def.Name, Err: err}
|
||||||
}
|
}
|
||||||
|
|
||||||
normalized := normalizeCommandName(subName)
|
normalized := normalizeCommandName(subName)
|
||||||
|
|
@ -84,6 +84,6 @@ func (e *Executor) executeDefinition(ctx context.Context, req Request, def Defin
|
||||||
}
|
}
|
||||||
|
|
||||||
// Unknown sub-command
|
// Unknown sub-command
|
||||||
_ = req.Reply(fmt.Sprintf("Unknown parameter: %s. Usage: %s", subName, def.EffectiveUsage()))
|
err := req.Reply(fmt.Sprintf("Unknown parameter: %s. Usage: %s", subName, def.EffectiveUsage()))
|
||||||
return ExecuteResult{Outcome: OutcomeHandled, Command: def.Name}
|
return ExecuteResult{Outcome: OutcomeHandled, Command: def.Name, Err: err}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
28
pkg/commands/request_test.go
Normal file
28
pkg/commands/request_test.go
Normal file
|
|
@ -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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -9,6 +9,7 @@ type Runtime struct {
|
||||||
Config *config.Config
|
Config *config.Config
|
||||||
GetModelInfo func() (name, provider string)
|
GetModelInfo func() (name, provider string)
|
||||||
ListAgentIDs func() []string
|
ListAgentIDs func() []string
|
||||||
|
ListDefinitions func() []Definition
|
||||||
GetEnabledChannels func() []string
|
GetEnabledChannels func() []string
|
||||||
SwitchModel func(value string) (oldModel string, err error)
|
SwitchModel func(value string) (oldModel string, err error)
|
||||||
SwitchChannel func(value string) error
|
SwitchChannel func(value string) error
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue