From a0db5ea03e78b5ced4d598c0692aee4914079012 Mon Sep 17 00:00:00 2001 From: mingmxren Date: Tue, 3 Mar 2026 16:08:10 +0800 Subject: [PATCH] feat(commands): Session management [Phase 3/3] command execution centralization --- README.md | 8 + README.zh.md | 8 + pkg/agent/loop.go | 264 +++++--------- pkg/agent/loop_test.go | 217 ++++++++++++ pkg/channels/interfaces.go | 7 - pkg/channels/interfaces_command_test.go | 10 - pkg/channels/telegram/telegram.go | 101 ++++-- pkg/channels/telegram/telegram_commands.go | 154 --------- pkg/channels/telegram/telegram_dispatch.go | 63 ---- .../telegram/telegram_dispatch_test.go | 50 ++- .../telegram_group_command_filter_test.go | 147 ++++++++ pkg/channels/whatsapp/whatsapp.go | 56 +-- .../whatsapp/whatsapp_command_test.go | 51 +-- .../whatsapp_native/whatsapp_command_test.go | 66 ++-- .../whatsapp_native/whatsapp_native.go | 41 --- pkg/commands/builtin.go | 254 +++++++++++--- pkg/commands/builtin_test.go | 38 ++ pkg/commands/dispatcher.go | 90 +---- pkg/commands/dispatcher_test.go | 117 ------- pkg/commands/executor.go | 71 ++++ pkg/commands/executor_test.go | 155 +++++++++ pkg/commands/runtime.go | 31 ++ pkg/commands/runtime_test.go | 50 +++ pkg/commands/session_handlers_test.go | 324 ++++++++++++++++++ pkg/commands/show_list_handlers_test.go | 88 +++++ 25 files changed, 1615 insertions(+), 846 deletions(-) delete mode 100644 pkg/channels/telegram/telegram_commands.go delete mode 100644 pkg/channels/telegram/telegram_dispatch.go create mode 100644 pkg/channels/telegram/telegram_group_command_filter_test.go delete mode 100644 pkg/commands/dispatcher_test.go create mode 100644 pkg/commands/executor.go create mode 100644 pkg/commands/executor_test.go create mode 100644 pkg/commands/runtime.go create mode 100644 pkg/commands/runtime_test.go create mode 100644 pkg/commands/session_handlers_test.go create mode 100644 pkg/commands/show_list_handlers_test.go diff --git a/README.md b/README.md index 48d0b12dc..c90063045 100644 --- a/README.md +++ b/README.md @@ -341,6 +341,7 @@ picoclaw gateway **4. Telegram command menu (auto-registered at startup)** PicoClaw now keeps command definitions in one shared registry. On startup, Telegram will automatically register supported bot commands (for example `/start`, `/help`, `/new`, `/session`, `/show`, `/list`) so command menu and runtime behavior stay in sync. +Telegram command menu registration remains channel-local discovery UX; generic command execution is handled centrally in the agent loop via the commands executor. If command registration fails (network/API transient errors), the channel still starts and PicoClaw retries registration in the background. @@ -742,6 +743,13 @@ Use `session.dm_scope` to control DM session isolation and `session.backlog_limi `/new` (or `/reset`) starts a fresh active session for the current scope. `/session list` and `/session resume ` operate within that same scope. +### Unified Command Execution Policy + +- Generic slash commands are executed through a single path in `pkg/agent/loop.go` via `commands.Executor`. +- Channel adapters no longer consume generic commands locally; they forward inbound text to the bus/agent path. Telegram still auto-registers supported commands at startup. +- Unknown slash command (for example `/foo`) passes through to normal LLM processing. +- Registered but unsupported command on the current channel (for example `/show` on WhatsApp) returns an explicit user-facing error and stops further processing. + ### 🔒 Security Sandbox PicoClaw runs in a sandboxed environment by default. The agent can only access files and execute commands within the configured workspace. diff --git a/README.zh.md b/README.zh.md index bf5161cf0..d6205508d 100644 --- a/README.zh.md +++ b/README.zh.md @@ -310,6 +310,7 @@ PicoClaw 支持多种聊天平台，使您的 Agent 能够连接到任何地方 ### Telegram 命令注册（启动时自动同步） PicoClaw 现在使用统一的命令定义来源。启动时会自动将 Telegram 支持的命令（例如 `/start`、`/help`、`/new`、`/session`、`/show`、`/list`）注册到 Bot 命令菜单，确保菜单展示与实际行为一致。 +Telegram 侧保留的是命令菜单注册能力；通用命令的实际执行统一走 Agent Loop 中的 commands executor。 如果注册因网络或 API 短暂异常失败，不会阻塞 channel 启动；系统会在后台自动重试。 @@ -383,6 +384,13 @@ PicoClaw 将数据存储在您配置的工作区中（默认：`~/.picoclaw/work `/new`（或 `/reset`）会在当前作用域创建新会话；`/session list` 和 `/session resume ` 只在当前作用域内生效。 +### 统一命令执行策略 + +- 通用斜杠命令通过 `pkg/agent/loop.go` 中的 `commands.Executor` 统一执行。 +- Channel 适配器不再在本地消费通用命令；它们只负责把入站文本转发到 bus/agent 路径。Telegram 仍会在启动时自动注册其支持的命令菜单。 +- 未注册的斜杠命令（例如 `/foo`）会透传给 LLM 按普通输入处理。 +- 已注册但当前 channel 不支持的命令（例如 WhatsApp 上的 `/show`）会返回明确的用户可见错误，并停止后续处理。 + ### 心跳 / 周期性任务 (Heartbeat) PicoClaw 可以自动执行周期性任务。在工作区创建 `HEARTBEAT.md` 文件： diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 314a72a59..d6f5d0b6b 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -12,7 +12,6 @@ import ( "errors" "fmt" "path/filepath" - "strconv" "strings" "sync" "sync/atomic" @@ -21,6 +20,7 @@ import ( "github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/channels" + "github.com/sipeed/picoclaw/pkg/commands" "github.com/sipeed/picoclaw/pkg/config" "github.com/sipeed/picoclaw/pkg/constants" "github.com/sipeed/picoclaw/pkg/logger" @@ -58,23 +58,7 @@ type processOptions struct { NoHistory bool // If true, don't load session history (for heartbeat) } -const ( - defaultResponse = "I've completed processing but have no response to give. Increase `max_tool_iterations` in config.json." - commandPrefixSlash = "/" - commandMentionSeparator = "@" - commandNameNew = "/new" - commandNameReset = "/reset" - commandNameSession = "/session" - commandNameShow = "/show" - commandNameList = "/list" - commandNameSwitch = "/switch" - sessionKeyAgentPrefix = "agent:" - metadataKeyAccountID = "account_id" - metadataKeyGuildID = "guild_id" - metadataKeyTeamID = "team_id" - metadataKeyParentPeerKind = "parent_peer_kind" - metadataKeyParentPeerID = "parent_peer_id" -) +const defaultResponse = "I've completed processing but have no response to give. Increase `max_tool_iterations` in config.json." func NewAgentLoop( cfg *config.Config, @@ -514,11 +498,11 @@ func (al *AgentLoop) processMessage(ctx context.Context, msg bus.InboundMessage) func (al *AgentLoop) resolveMessageRoute(msg bus.InboundMessage) (routing.ResolvedRoute, *AgentInstance, error) { route := al.registry.ResolveRoute(routing.RouteInput{ Channel: msg.Channel, - AccountID: inboundMetadata(msg, metadataKeyAccountID), + AccountID: msg.Metadata["account_id"], Peer: extractPeer(msg), ParentPeer: extractParentPeer(msg), - GuildID: inboundMetadata(msg, metadataKeyGuildID), - TeamID: inboundMetadata(msg, metadataKeyTeamID), + GuildID: msg.Metadata["guild_id"], + TeamID: msg.Metadata["team_id"], }) agent, ok := al.registry.GetAgent(route.AgentID) @@ -533,7 +517,7 @@ func (al *AgentLoop) resolveMessageRoute(msg bus.InboundMessage) (routing.Resolv } func resolveScopeKey(route routing.ResolvedRoute, msgSessionKey string) string { - if msgSessionKey != "" && strings.HasPrefix(msgSessionKey, sessionKeyAgentPrefix) { + if msgSessionKey != "" && strings.HasPrefix(msgSessionKey, "agent:") { return msgSessionKey } return route.SessionKey @@ -1404,153 +1388,56 @@ func (al *AgentLoop) handleCommand( route routing.ResolvedRoute, agent *AgentInstance, ) (string, bool) { - // Scope-aware command routing: session-affecting commands are resolved - // against route-derived scope keys so each chat/group keeps isolated history. + // Scope-aware command routing is delegated to the runtime-backed executor. + // Session commands (/new, /session) operate on route-derived scope keys via + // agentCommandRuntime.ScopeKey(), while channel-agnostic commands share one + // execution path. content := strings.TrimSpace(msg.Content) - if !strings.HasPrefix(content, commandPrefixSlash) { + if !strings.HasPrefix(content, "/") { return "", false } - parts := strings.Fields(content) - if len(parts) == 0 { - return "", false - } + runtime := newAgentCommandRuntime(msg, route, agent, al.cfg) + executor := commands.NewExecutor(commands.NewRegistry(commands.BuiltinDefinitionsWithRuntime(al.cfg, runtime))) - cmd := parts[0] - if at := strings.Index(cmd, commandMentionSeparator); at > 0 { - cmd = cmd[:at] - } - args := parts[1:] + var commandReply string + result := executor.Execute(ctx, commands.Request{ + Channel: msg.Channel, + ChatID: msg.ChatID, + SenderID: msg.SenderID, + Text: msg.Content, + Reply: func(text string) error { + commandReply = text + return nil + }, + }) - switch cmd { - case commandNameNew, commandNameReset: - // Create and rotate session state only inside this scope. - scopeKey := resolveScopeKey(route, msg.SessionKey) - newSessionKey, err := agent.Sessions.StartNew(scopeKey) - if err != nil { - return fmt.Sprintf("Failed to start new session: %v", err), true + switch result.Outcome { + case commands.OutcomeHandled: + if result.Err != nil { + return mapCommandError(result), true + } + if commandReply != "" { + return commandReply, true + } + if result.Reply != "" { + return result.Reply, true + } + return "", true + case commands.OutcomePassthrough: + parts := strings.Fields(content) + if len(parts) == 0 { + return "", false + } + cmd := parts[0] + if at := strings.Index(cmd, "@"); at > 0 { + cmd = cmd[:at] + } + if cmd != "/switch" { + return "", false } - backlogLimit := config.DefaultSessionBacklogLimit - if al.cfg != nil { - backlogLimit = al.cfg.Session.EffectiveBacklogLimit() - } - - pruned, err := agent.Sessions.Prune(scopeKey, backlogLimit) - if err != nil { - return fmt.Sprintf( - "Started new session (%s), but pruning old sessions failed: %v", - newSessionKey, - err, - ), true - } - - if len(pruned) == 0 { - return fmt.Sprintf("Started new session: %s", newSessionKey), true - } - return fmt.Sprintf("Started new session: %s (pruned %d old session(s))", newSessionKey, len(pruned)), true - - case commandNameSession: - if len(args) < 1 { - return "Usage: /session [list|resume ]", true - } - - // List/resume operate on the same scope-local ordering used by /new. - scopeKey := resolveScopeKey(route, msg.SessionKey) - switch args[0] { - case "list": - list, err := agent.Sessions.List(scopeKey) - if err != nil { - return fmt.Sprintf("Failed to list sessions: %v", err), true - } - if len(list) == 0 { - return "No sessions found for current chat.", true - } - - lines := make([]string, 0, len(list)+1) - lines = append(lines, "Sessions for current chat:") - for _, item := range list { - activeMarker := " " - if item.Active { - activeMarker = "*" - } - updated := "-" - if !item.UpdatedAt.IsZero() { - updated = item.UpdatedAt.Format("2006-01-02 15:04") - } - lines = append(lines, fmt.Sprintf( - "%d. [%s] %s (%d msgs, updated %s)", - item.Ordinal, - activeMarker, - item.SessionKey, - item.MessageCnt, - updated, - )) - } - return strings.Join(lines, "\n"), true - - case "resume": - if len(args) != 2 { - return "Usage: /session resume ", true - } - index, err := strconv.Atoi(args[1]) - if err != nil || index < 1 { - return "Usage: /session resume ", true - } - sessionKey, err := agent.Sessions.Resume(scopeKey, index) - if err != nil { - return fmt.Sprintf("Failed to resume session %d: %v", index, err), true - } - return fmt.Sprintf("Resumed session %d: %s", index, sessionKey), true - - default: - return "Usage: /session [list|resume ]", true - } - - case commandNameShow: - if len(args) < 1 { - return "Usage: /show [model|channel|agents]", true - } - switch args[0] { - case "model": - defaultAgent := al.registry.GetDefaultAgent() - if defaultAgent == nil { - return "No default agent configured", true - } - return fmt.Sprintf("Current model: %s", defaultAgent.Model), true - case "channel": - return fmt.Sprintf("Current channel: %s", msg.Channel), true - case "agents": - agentIDs := al.registry.ListAgentIDs() - return fmt.Sprintf("Registered agents: %s", strings.Join(agentIDs, ", ")), true - default: - return fmt.Sprintf("Unknown show target: %s", args[0]), true - } - - case commandNameList: - if len(args) < 1 { - return "Usage: /list [models|channels|agents]", true - } - switch args[0] { - case "models": - return "Available models: configured in config.json per agent", true - case "channels": - if al.channelManager == nil { - return "Channel manager not initialized", true - } - channels := al.channelManager.GetEnabledChannels() - if len(channels) == 0 { - return "No channels enabled", true - } - return fmt.Sprintf("Enabled channels: %s", strings.Join(channels, ", ")), true - case "agents": - agentIDs := al.registry.ListAgentIDs() - return fmt.Sprintf("Registered agents: %s", strings.Join(agentIDs, ", ")), true - default: - return fmt.Sprintf("Unknown list target: %s", args[0]), true - } - - case commandNameSwitch: + args := parts[1:] if len(args) < 3 || args[1] != "to" { return "Usage: /switch [model|channel] to ", true } @@ -1565,6 +1452,10 @@ func (al *AgentLoop) handleCommand( } oldModel := defaultAgent.Model defaultAgent.Model = value + if al.cfg != nil { + al.cfg.Agents.Defaults.ModelName = value + al.cfg.Agents.Defaults.Model = value + } return fmt.Sprintf("Switched model from %s to %s", oldModel, value), true case "channel": if al.channelManager == nil { @@ -1582,6 +1473,46 @@ func (al *AgentLoop) handleCommand( return "", false } +type agentCommandRuntime struct { + scope string + sess commands.SessionOps + cfg *config.Config +} + +func newAgentCommandRuntime( + msg bus.InboundMessage, + route routing.ResolvedRoute, + agent *AgentInstance, + cfg *config.Config, +) commands.Runtime { + // Build a narrow runtime adapter so command handlers can read only what they + // need (scope, session ops, config) without importing AgentLoop internals. + return agentCommandRuntime{ + scope: resolveScopeKey(route, msg.SessionKey), + sess: agent.Sessions, + cfg: cfg, + } +} + +func (r agentCommandRuntime) ScopeKey() string { + return r.scope +} + +func (r agentCommandRuntime) SessionOps() commands.SessionOps { + return r.sess +} + +func (r agentCommandRuntime) Config() *config.Config { + return r.cfg +} + +func mapCommandError(result commands.ExecuteResult) string { + if result.Command == "" { + return fmt.Sprintf("Failed to execute command: %v", result.Err) + } + return fmt.Sprintf("Failed to execute /%s: %v", result.Command, result.Err) +} + // extractPeer extracts the routing peer from the inbound message's structured Peer field. func extractPeer(msg bus.InboundMessage) *routing.RoutePeer { if msg.Peer.Kind == "" { @@ -1598,17 +1529,10 @@ func extractPeer(msg bus.InboundMessage) *routing.RoutePeer { return &routing.RoutePeer{Kind: msg.Peer.Kind, ID: peerID} } -func inboundMetadata(msg bus.InboundMessage, key string) string { - if msg.Metadata == nil { - return "" - } - return msg.Metadata[key] -} - // extractParentPeer extracts the parent peer (reply-to) from inbound message metadata. func extractParentPeer(msg bus.InboundMessage) *routing.RoutePeer { - parentKind := inboundMetadata(msg, metadataKeyParentPeerKind) - parentID := inboundMetadata(msg, metadataKeyParentPeerID) + parentKind := msg.Metadata["parent_peer_kind"] + parentID := msg.Metadata["parent_peer_id"] if parentKind == "" || parentID == "" { return nil } diff --git a/pkg/agent/loop_test.go b/pkg/agent/loop_test.go index 503471667..146c95f8d 100644 --- a/pkg/agent/loop_test.go +++ b/pkg/agent/loop_test.go @@ -337,6 +337,29 @@ func (m *simpleMockProvider) GetDefaultModel() string { return "mock-model" } +type countingMockProvider struct { + response string + calls int +} + +func (m *countingMockProvider) Chat( + ctx context.Context, + messages []providers.Message, + tools []providers.ToolDefinition, + model string, + opts map[string]any, +) (*providers.LLMResponse, error) { + m.calls++ + return &providers.LLMResponse{ + Content: m.response, + ToolCalls: []providers.ToolCall{}, + }, nil +} + +func (m *countingMockProvider) GetDefaultModel() string { + return "counting-mock-model" +} + // mockCustomTool is a simple mock tool for registration testing type mockCustomTool struct{} @@ -576,6 +599,200 @@ func TestHandleCommand_NewAndSessionCommands(t *testing.T) { } } +func TestProcessMessage_CommandOutcomes(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "agent-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + Model: "test-model", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + Session: config.SessionConfig{ + DMScope: "per-channel-peer", + BacklogLimit: 20, + }, + } + + msgBus := bus.NewMessageBus() + provider := &countingMockProvider{response: "LLM reply"} + al := NewAgentLoop(cfg, msgBus, provider) + helper := testHelper{al: al} + + baseMsg := bus.InboundMessage{ + Channel: "whatsapp", + SenderID: "user1", + ChatID: "chat1", + Peer: bus.Peer{ + Kind: "direct", + ID: "user1", + }, + } + + showResp := helper.executeAndGetResponse(t, context.Background(), bus.InboundMessage{ + Channel: baseMsg.Channel, + SenderID: baseMsg.SenderID, + ChatID: baseMsg.ChatID, + Content: "/show channel", + Peer: baseMsg.Peer, + }) + if showResp != "Current Channel: whatsapp" { + t.Fatalf("unexpected /show reply: %q", showResp) + } + if provider.calls != 0 { + t.Fatalf("LLM should not be called for handled command, calls=%d", provider.calls) + } + + fooResp := helper.executeAndGetResponse(t, context.Background(), bus.InboundMessage{ + Channel: baseMsg.Channel, + SenderID: baseMsg.SenderID, + ChatID: baseMsg.ChatID, + Content: "/foo", + Peer: baseMsg.Peer, + }) + if fooResp != "LLM reply" { + t.Fatalf("unexpected /foo reply: %q", fooResp) + } + if provider.calls != 1 { + t.Fatalf("LLM should be called exactly once after /foo passthrough, calls=%d", provider.calls) + } + + route := al.registry.ResolveRoute(routing.RouteInput{ + Channel: baseMsg.Channel, + Peer: extractPeer(baseMsg), + }) + scopeKey := route.SessionKey + + newResp := helper.executeAndGetResponse(t, context.Background(), bus.InboundMessage{ + Channel: baseMsg.Channel, + SenderID: baseMsg.SenderID, + ChatID: baseMsg.ChatID, + Content: "/new", + Peer: baseMsg.Peer, + }) + if !strings.Contains(newResp, "Started new session: "+scopeKey+"#2") { + t.Fatalf("unexpected /new reply: %q", newResp) + } + if provider.calls != 1 { + t.Fatalf("LLM should not be called for handled /new command, calls=%d", provider.calls) + } +} + +func TestProcessMessage_CLI_SessionCommandsStillWork(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "agent-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + Model: "test-model", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + Session: config.SessionConfig{ + BacklogLimit: 20, + }, + } + + msgBus := bus.NewMessageBus() + provider := &countingMockProvider{response: "LLM reply"} + al := NewAgentLoop(cfg, msgBus, provider) + helper := testHelper{al: al} + + newResp := helper.executeAndGetResponse(t, context.Background(), bus.InboundMessage{ + Channel: "cli", + SenderID: "user1", + ChatID: "cli", + Content: "/new", + }) + if !strings.Contains(newResp, "Started new session:") { + t.Fatalf("unexpected /new reply on cli: %q", newResp) + } + + listResp := helper.executeAndGetResponse(t, context.Background(), bus.InboundMessage{ + Channel: "cli", + SenderID: "user1", + ChatID: "cli", + Content: "/session list", + }) + if !strings.Contains(listResp, "Sessions for current chat:") { + t.Fatalf("unexpected /session list reply on cli: %q", listResp) + } + + if provider.calls != 0 { + t.Fatalf("LLM should not be called for handled cli session commands, calls=%d", provider.calls) + } +} + +func TestProcessMessage_SwitchModelShowModelConsistency(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "agent-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + Provider: "openai", + Model: "before-switch", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + } + + msgBus := bus.NewMessageBus() + provider := &countingMockProvider{response: "LLM reply"} + al := NewAgentLoop(cfg, msgBus, provider) + helper := testHelper{al: al} + + switchResp := helper.executeAndGetResponse(t, context.Background(), bus.InboundMessage{ + Channel: "telegram", + SenderID: "user1", + ChatID: "chat1", + Content: "/switch model to after-switch", + Peer: bus.Peer{ + Kind: "direct", + ID: "user1", + }, + }) + if !strings.Contains(switchResp, "Switched model from before-switch to after-switch") { + t.Fatalf("unexpected /switch reply: %q", switchResp) + } + + showResp := helper.executeAndGetResponse(t, context.Background(), bus.InboundMessage{ + Channel: "telegram", + SenderID: "user1", + ChatID: "chat1", + Content: "/show model", + Peer: bus.Peer{ + Kind: "direct", + ID: "user1", + }, + }) + if !strings.Contains(showResp, "Current Model: after-switch (Provider: openai)") { + t.Fatalf("unexpected /show model reply after switch: %q", showResp) + } + + if provider.calls != 0 { + t.Fatalf("LLM should not be called for /switch and /show, calls=%d", provider.calls) + } +} + // TestToolResult_SilentToolDoesNotSendUserMessage verifies silent tools don't trigger outbound func TestToolResult_SilentToolDoesNotSendUserMessage(t *testing.T) { tmpDir, err := os.MkdirTemp("", "agent-test-*") diff --git a/pkg/channels/interfaces.go b/pkg/channels/interfaces.go index 2bb4da3e0..42f18405c 100644 --- a/pkg/channels/interfaces.go +++ b/pkg/channels/interfaces.go @@ -48,10 +48,3 @@ type PlaceholderRecorder interface { type CommandRegistrarCapable interface { RegisterCommands(ctx context.Context, defs []commands.Definition) error } - -// CommandParserCapable is implemented by channels that expose a command -// dispatch entrypoint backed by shared command definitions/dispatcher. -// It is optional and intended for cross-channel command handling features. -type CommandParserCapable interface { - DispatchCommand(ctx context.Context, req commands.Request) commands.Result -} diff --git a/pkg/channels/interfaces_command_test.go b/pkg/channels/interfaces_command_test.go index 947f6b5c1..de5502644 100644 --- a/pkg/channels/interfaces_command_test.go +++ b/pkg/channels/interfaces_command_test.go @@ -11,16 +11,6 @@ type mockRegistrar struct{} func (mockRegistrar) RegisterCommands(context.Context, []commands.Definition) error { return nil } -type mockParser struct{} - -func (mockParser) DispatchCommand(context.Context, commands.Request) commands.Result { - return commands.Result{Matched: false} -} - func TestCommandRegistrarCapable_Compiles(t *testing.T) { var _ CommandRegistrarCapable = mockRegistrar{} } - -func TestCommandParserCapable_Compiles(t *testing.T) { - var _ CommandParserCapable = mockParser{} -} diff --git a/pkg/channels/telegram/telegram.go b/pkg/channels/telegram/telegram.go index 1924586b3..2b8a8d4c1 100644 --- a/pkg/channels/telegram/telegram.go +++ b/pkg/channels/telegram/telegram.go @@ -41,14 +41,12 @@ var ( type TelegramChannel struct { *channels.BaseChannel - bot *telego.Bot - bh *th.BotHandler - commands TelegramCommander - dispatcher commands.Dispatching - config *config.Config - chatIDs map[string]int64 - ctx context.Context - cancel context.CancelFunc + bot *telego.Bot + bh *th.BotHandler + config *config.Config + chatIDs map[string]int64 + ctx context.Context + cancel context.CancelFunc registerFunc func(context.Context, []commands.Definition) error commandRegCancel context.CancelFunc @@ -94,8 +92,6 @@ func NewTelegramChannel(cfg *config.Config, bus *bus.MessageBus) (*TelegramChann return &TelegramChannel{ BaseChannel: base, - commands: NewTelegramCommands(bot, cfg), - dispatcher: commands.NewDispatcher(commands.NewRegistry(commands.BuiltinDefinitions(cfg))), bot: bot, config: cfg, chatIDs: make(map[string]int64), @@ -129,9 +125,6 @@ func (c *TelegramChannel) Start(ctx context.Context) error { c.bh = bh bh.HandleMessage(func(ctx *th.Context, message telego.Message) error { - if c.dispatchCommand(ctx, message) { - return nil - } return c.handleMessage(ctx, &message) }, th.AnyMessage()) @@ -716,34 +709,34 @@ func escapeHTML(text string) string { // isBotMentioned checks if the bot is mentioned in the message via entities. func (c *TelegramChannel) isBotMentioned(message *telego.Message) bool { - botUsername := c.bot.Username() - if botUsername == "" { + text, entities := telegramEntityTextAndList(message) + if text == "" || len(entities) == 0 { return false } - entities := message.Entities - if entities == nil { - entities = message.CaptionEntities + botUsername := "" + if c.bot != nil { + botUsername = c.bot.Username() } + runes := []rune(text) for _, entity := range entities { - if entity.Type == "mention" { - // Extract the mention text from the message - text := message.Text - if text == "" { - text = message.Caption - } - runes := []rune(text) - end := entity.Offset + entity.Length - if end <= len(runes) { - mention := string(runes[entity.Offset:end]) - if strings.EqualFold(mention, "@"+botUsername) { - return true - } - } + entityText, ok := telegramEntityText(runes, entity) + if !ok { + continue } - if entity.Type == "text_mention" && entity.User != nil { - if entity.User.Username == botUsername { + + switch entity.Type { + case telego.EntityTypeMention: + if botUsername != "" && strings.EqualFold(entityText, "@"+botUsername) { + return true + } + case telego.EntityTypeTextMention: + if botUsername != "" && entity.User != nil && strings.EqualFold(entity.User.Username, botUsername) { + return true + } + case telego.EntityTypeBotCommand: + if isBotCommandEntityForThisBot(entityText, botUsername) { return true } } @@ -751,6 +744,46 @@ func (c *TelegramChannel) isBotMentioned(message *telego.Message) bool { return false } +func telegramEntityTextAndList(message *telego.Message) (string, []telego.MessageEntity) { + if message.Text != "" { + return message.Text, message.Entities + } + return message.Caption, message.CaptionEntities +} + +func telegramEntityText(runes []rune, entity telego.MessageEntity) (string, bool) { + if entity.Offset < 0 || entity.Length <= 0 { + return "", false + } + end := entity.Offset + entity.Length + if entity.Offset >= len(runes) || end > len(runes) { + return "", false + } + return string(runes[entity.Offset:end]), true +} + +func isBotCommandEntityForThisBot(entityText, botUsername string) bool { + if !strings.HasPrefix(entityText, "/") { + return false + } + command := strings.TrimPrefix(entityText, "/") + if command == "" { + return false + } + + at := strings.IndexRune(command, '@') + if at == -1 { + // A bare /command delivered to this bot is intended for this bot. + return true + } + + mentionUsername := command[at+1:] + if mentionUsername == "" || botUsername == "" { + return false + } + return strings.EqualFold(mentionUsername, botUsername) +} + // stripBotMention removes the @bot mention from the content. func (c *TelegramChannel) stripBotMention(content string) string { botUsername := c.bot.Username() diff --git a/pkg/channels/telegram/telegram_commands.go b/pkg/channels/telegram/telegram_commands.go deleted file mode 100644 index 79f4e2a53..000000000 --- a/pkg/channels/telegram/telegram_commands.go +++ /dev/null @@ -1,154 +0,0 @@ -package telegram - -import ( - "context" - "fmt" - "strings" - - "github.com/mymmrac/telego" - - "github.com/sipeed/picoclaw/pkg/commands" - "github.com/sipeed/picoclaw/pkg/config" -) - -type TelegramCommander interface { - Help(ctx context.Context, message telego.Message) error - Start(ctx context.Context, message telego.Message) error - Show(ctx context.Context, message telego.Message) error - List(ctx context.Context, message telego.Message) error -} - -type cmd struct { - bot *telego.Bot - config *config.Config -} - -func NewTelegramCommands(bot *telego.Bot, cfg *config.Config) TelegramCommander { - return &cmd{ - bot: bot, - config: cfg, - } -} - -func commandArgs(text string) string { - parts := strings.SplitN(text, " ", 2) - if len(parts) < 2 { - return "" - } - return strings.TrimSpace(parts[1]) -} - -func (c *cmd) Help(ctx context.Context, message telego.Message) error { - defs := commands.NewRegistry(commands.BuiltinDefinitions(c.config)).Definitions() - msg := commands.FormatHelpMessage(defs) - _, err := c.bot.SendMessage(ctx, &telego.SendMessageParams{ - ChatID: telego.ChatID{ID: message.Chat.ID}, - Text: msg, - ReplyParameters: &telego.ReplyParameters{ - MessageID: message.MessageID, - }, - }) - return err -} - -func (c *cmd) Start(ctx context.Context, message telego.Message) error { - _, err := c.bot.SendMessage(ctx, &telego.SendMessageParams{ - ChatID: telego.ChatID{ID: message.Chat.ID}, - Text: "Hello! I am PicoClaw 🦞", - ReplyParameters: &telego.ReplyParameters{ - MessageID: message.MessageID, - }, - }) - return err -} - -func (c *cmd) Show(ctx context.Context, message telego.Message) error { - args := commandArgs(message.Text) - if args == "" { - _, err := c.bot.SendMessage(ctx, &telego.SendMessageParams{ - ChatID: telego.ChatID{ID: message.Chat.ID}, - Text: "Usage: /show [model|channel]", - ReplyParameters: &telego.ReplyParameters{ - MessageID: message.MessageID, - }, - }) - return err - } - - var response string - switch args { - case "model": - response = fmt.Sprintf("Current Model: %s (Provider: %s)", - c.config.Agents.Defaults.GetModelName(), - c.config.Agents.Defaults.Provider) - case "channel": - response = "Current Channel: telegram" - default: - response = fmt.Sprintf("Unknown parameter: %s. Try 'model' or 'channel'.", args) - } - - _, err := c.bot.SendMessage(ctx, &telego.SendMessageParams{ - ChatID: telego.ChatID{ID: message.Chat.ID}, - Text: response, - ReplyParameters: &telego.ReplyParameters{ - MessageID: message.MessageID, - }, - }) - return err -} - -func (c *cmd) List(ctx context.Context, message telego.Message) error { - args := commandArgs(message.Text) - if args == "" { - _, err := c.bot.SendMessage(ctx, &telego.SendMessageParams{ - ChatID: telego.ChatID{ID: message.Chat.ID}, - Text: "Usage: /list [models|channels]", - ReplyParameters: &telego.ReplyParameters{ - MessageID: message.MessageID, - }, - }) - return err - } - - var response string - switch args { - case "models": - provider := c.config.Agents.Defaults.Provider - if provider == "" { - provider = "configured default" - } - response = fmt.Sprintf("Configured Model: %s\nProvider: %s\n\nTo change models, update config.json", - c.config.Agents.Defaults.GetModelName(), provider) - - case "channels": - var enabled []string - if c.config.Channels.Telegram.Enabled { - enabled = append(enabled, "telegram") - } - if c.config.Channels.WhatsApp.Enabled { - enabled = append(enabled, "whatsapp") - } - if c.config.Channels.Feishu.Enabled { - enabled = append(enabled, "feishu") - } - if c.config.Channels.Discord.Enabled { - enabled = append(enabled, "discord") - } - if c.config.Channels.Slack.Enabled { - enabled = append(enabled, "slack") - } - response = fmt.Sprintf("Enabled Channels:\n- %s", strings.Join(enabled, "\n- ")) - - default: - response = fmt.Sprintf("Unknown parameter: %s. Try 'models' or 'channels'.", args) - } - - _, err := c.bot.SendMessage(ctx, &telego.SendMessageParams{ - ChatID: telego.ChatID{ID: message.Chat.ID}, - Text: response, - ReplyParameters: &telego.ReplyParameters{ - MessageID: message.MessageID, - }, - }) - return err -} diff --git a/pkg/channels/telegram/telegram_dispatch.go b/pkg/channels/telegram/telegram_dispatch.go deleted file mode 100644 index d1221acff..000000000 --- a/pkg/channels/telegram/telegram_dispatch.go +++ /dev/null @@ -1,63 +0,0 @@ -package telegram - -import ( - "context" - "strconv" - - "github.com/mymmrac/telego" - - "github.com/sipeed/picoclaw/pkg/commands" - "github.com/sipeed/picoclaw/pkg/logger" -) - -func (c *TelegramChannel) DispatchCommand(ctx context.Context, req commands.Request) commands.Result { - if c.dispatcher == nil { - return commands.Result{Matched: false} - } - return c.dispatcher.Dispatch(ctx, req) -} - -// dispatchCommand adapts Telegram updates to the shared dispatcher contract. -// This keeps command semantics identical across channels while preserving -// Telegram-specific reply mechanics (reply_to message id). -func (c *TelegramChannel) dispatchCommand(ctx context.Context, message telego.Message) bool { - senderID := "" - if message.From != nil { - senderID = strconv.FormatInt(message.From.ID, 10) - } - - res := c.DispatchCommand(ctx, commands.Request{ - Channel: "telegram", - ChatID: strconv.FormatInt(message.Chat.ID, 10), - SenderID: senderID, - Text: message.Text, - MessageID: strconv.Itoa(message.MessageID), - Reply: func(text string) error { - _, err := c.bot.SendMessage(ctx, &telego.SendMessageParams{ - ChatID: telego.ChatID{ID: message.Chat.ID}, - Text: text, - ReplyParameters: &telego.ReplyParameters{ - MessageID: message.MessageID, - }, - }) - return err - }, - }) - if !res.Matched { - return false - } - - if res.Err != nil { - logger.ErrorCF("telegram", "Command execution failed", map[string]any{ - "command": res.Command, - "error": res.Err.Error(), - }) - } - if res.Matched && !res.Handled { - logger.DebugCF("telegram", "Command matched without handler; passing to normal flow", map[string]any{ - "command": res.Command, - }) - } - - return true -} diff --git a/pkg/channels/telegram/telegram_dispatch_test.go b/pkg/channels/telegram/telegram_dispatch_test.go index 1e187f5ac..1ea4a4824 100644 --- a/pkg/channels/telegram/telegram_dispatch_test.go +++ b/pkg/channels/telegram/telegram_dispatch_test.go @@ -3,30 +3,50 @@ package telegram import ( "context" "testing" + "time" "github.com/mymmrac/telego" - "github.com/sipeed/picoclaw/pkg/commands" + "github.com/sipeed/picoclaw/pkg/bus" + "github.com/sipeed/picoclaw/pkg/channels" ) -func TestDispatchCommand_UsesDispatcher(t *testing.T) { - ch := &TelegramChannel{} - called := false - ch.dispatcher = commands.DispatchFunc(func(context.Context, commands.Request) commands.Result { - called = true - return commands.Result{Matched: true, Command: "noop"} - }) +func TestHandleMessage_DoesNotConsumeGenericCommandsLocally(t *testing.T) { + messageBus := bus.NewMessageBus() + ch := &TelegramChannel{ + BaseChannel: channels.NewBaseChannel("telegram", nil, messageBus, nil), + chatIDs: make(map[string]int64), + ctx: context.Background(), + } - msg := telego.Message{ - Text: "/help", - MessageID: 7, + msg := &telego.Message{ + Text: "/new", + MessageID: 9, Chat: telego.Chat{ - ID: 123, + ID: 123, + Type: "private", + }, + From: &telego.User{ + ID: 42, + FirstName: "Alice", }, } - handled := ch.dispatchCommand(context.Background(), msg) - if !handled || !called { - t.Fatalf("handled=%v called=%v", handled, called) + if err := ch.handleMessage(context.Background(), msg); err != nil { + t.Fatalf("handleMessage error: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + + inbound, ok := messageBus.ConsumeInbound(ctx) + if !ok { + t.Fatal("expected inbound message to be forwarded") + } + if inbound.Channel != "telegram" { + t.Fatalf("channel=%q", inbound.Channel) + } + if inbound.Content != "/new" { + t.Fatalf("content=%q", inbound.Content) } } diff --git a/pkg/channels/telegram/telegram_group_command_filter_test.go b/pkg/channels/telegram/telegram_group_command_filter_test.go new file mode 100644 index 000000000..0d5b985fe --- /dev/null +++ b/pkg/channels/telegram/telegram_group_command_filter_test.go @@ -0,0 +1,147 @@ +package telegram + +import ( + "context" + "fmt" + "strings" + "testing" + "time" + + "github.com/mymmrac/telego" + ta "github.com/mymmrac/telego/telegoapi" + + "github.com/sipeed/picoclaw/pkg/bus" + "github.com/sipeed/picoclaw/pkg/channels" + "github.com/sipeed/picoclaw/pkg/config" +) + +type getMeCaller struct { + username string +} + +func (c getMeCaller) Call(_ context.Context, url string, _ *ta.RequestData) (*ta.Response, error) { + if strings.HasSuffix(url, "/getMe") { + result := fmt.Sprintf(`{"id":1,"is_bot":true,"first_name":"bot","username":%q}`, c.username) + return &ta.Response{Ok: true, Result: []byte(result)}, nil + } + return &ta.Response{Ok: true, Result: []byte("true")}, nil +} + +func newTestTelegramBot(t *testing.T, username string) *telego.Bot { + t.Helper() + + token := "123456:" + strings.Repeat("a", 35) + bot, err := telego.NewBot(token, + telego.WithAPICaller(getMeCaller{username: username}), + telego.WithDiscardLogger(), + ) + if err != nil { + t.Fatalf("NewBot error: %v", err) + } + return bot +} + +func newGroupMentionOnlyChannel(t *testing.T, botUsername string) (*TelegramChannel, *bus.MessageBus) { + t.Helper() + + messageBus := bus.NewMessageBus() + ch := &TelegramChannel{ + BaseChannel: channels.NewBaseChannel("telegram", nil, messageBus, nil, + channels.WithGroupTrigger(config.GroupTriggerConfig{MentionOnly: true}), + ), + bot: newTestTelegramBot(t, botUsername), + chatIDs: make(map[string]int64), + ctx: context.Background(), + } + return ch, messageBus +} + +func TestHandleMessage_GroupMentionOnly_BotCommandEntity(t *testing.T) { + tests := []struct { + name string + text string + wantForwarded bool + wantContent string + }{ + { + name: "command with bot username", + text: "/new@testbot", + wantForwarded: true, + wantContent: "/new", + }, + { + name: "bare command", + text: "/new", + wantForwarded: true, + wantContent: "/new", + }, + { + name: "command for another bot", + text: "/new@otherbot", + wantForwarded: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ch, messageBus := newGroupMentionOnlyChannel(t, "testbot") + + msg := &telego.Message{ + Text: tc.text, + Entities: []telego.MessageEntity{{ + Type: telego.EntityTypeBotCommand, + Offset: 0, + Length: len([]rune(tc.text)), + }}, + MessageID: 42, + Chat: telego.Chat{ + ID: 123, + Type: "group", + }, + From: &telego.User{ + ID: 7, + FirstName: "Alice", + }, + } + + if err := ch.handleMessage(context.Background(), msg); err != nil { + t.Fatalf("handleMessage error: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 150*time.Millisecond) + defer cancel() + + inbound, ok := messageBus.ConsumeInbound(ctx) + if tc.wantForwarded { + if !ok { + t.Fatal("expected inbound message to be forwarded") + } + if inbound.Content != tc.wantContent { + t.Fatalf("content=%q want=%q", inbound.Content, tc.wantContent) + } + return + } + + if ok { + t.Fatalf("expected message to be filtered, got content=%q", inbound.Content) + } + }) + } +} + +func TestIsBotMentioned_MentionEntityUnaffected(t *testing.T) { + ch, _ := newGroupMentionOnlyChannel(t, "testbot") + + msg := &telego.Message{ + Text: "@testbot hello", + Entities: []telego.MessageEntity{{ + Type: telego.EntityTypeMention, + Offset: 0, + Length: len("@testbot"), + }}, + } + + if !ch.isBotMentioned(msg) { + t.Fatal("expected mention entity to be treated as bot mention") + } +} diff --git a/pkg/channels/whatsapp/whatsapp.go b/pkg/channels/whatsapp/whatsapp.go index c22d69cd3..70b3e02bf 100644 --- a/pkg/channels/whatsapp/whatsapp.go +++ b/pkg/channels/whatsapp/whatsapp.go @@ -11,7 +11,6 @@ import ( "github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/channels" - "github.com/sipeed/picoclaw/pkg/commands" "github.com/sipeed/picoclaw/pkg/config" "github.com/sipeed/picoclaw/pkg/identity" "github.com/sipeed/picoclaw/pkg/logger" @@ -20,14 +19,13 @@ import ( type WhatsAppChannel struct { *channels.BaseChannel - conn *websocket.Conn - config config.WhatsAppConfig - url string - dispatcher commands.Dispatching - ctx context.Context - cancel context.CancelFunc - mu sync.Mutex - connected bool + conn *websocket.Conn + config config.WhatsAppConfig + url string + ctx context.Context + cancel context.CancelFunc + mu sync.Mutex + connected bool } func NewWhatsAppChannel(cfg config.WhatsAppConfig, bus *bus.MessageBus) (*WhatsAppChannel, error) { @@ -44,7 +42,6 @@ func NewWhatsAppChannel(cfg config.WhatsAppConfig, bus *bus.MessageBus) (*WhatsA BaseChannel: base, config: cfg, url: cfg.BridgeURL, - dispatcher: commands.NewDispatcher(commands.NewRegistry(commands.BuiltinDefinitions(nil))), connected: false, }, nil } @@ -251,44 +248,5 @@ func (c *WhatsAppChannel) handleIncomingMessage(msg map[string]any) { return } - if c.tryHandleCommand(c.ctx, content, chatID, senderID, messageID) { - return - } - c.HandleMessage(c.ctx, peer, messageID, senderID, chatID, content, mediaPaths, metadata, sender) } - -func (c *WhatsAppChannel) tryHandleCommand( - ctx context.Context, - text, chatID, senderID, messageID string, -) bool { - res := c.DispatchCommand(ctx, commands.Request{ - Channel: "whatsapp", - ChatID: chatID, - SenderID: senderID, - Text: text, - MessageID: messageID, - Reply: func(text string) error { - return c.Send(ctx, bus.OutboundMessage{ChatID: chatID, Content: text}) - }, - }) - if res.Err != nil { - logger.WarnCF("whatsapp", "Command execution failed", map[string]any{ - "command": res.Command, - "error": res.Err.Error(), - }) - } - if res.Matched && !res.Handled { - logger.DebugCF("whatsapp", "Command matched without handler; passing to normal flow", map[string]any{ - "command": res.Command, - }) - } - return res.Matched -} - -func (c *WhatsAppChannel) DispatchCommand(ctx context.Context, req commands.Request) commands.Result { - if c.dispatcher == nil { - return commands.Result{Matched: false} - } - return c.dispatcher.Dispatch(ctx, req) -} diff --git a/pkg/channels/whatsapp/whatsapp_command_test.go b/pkg/channels/whatsapp/whatsapp_command_test.go index 1de2f2712..ee8aa4a52 100644 --- a/pkg/channels/whatsapp/whatsapp_command_test.go +++ b/pkg/channels/whatsapp/whatsapp_command_test.go @@ -3,32 +3,39 @@ package whatsapp import ( "context" "testing" + "time" - "github.com/sipeed/picoclaw/pkg/commands" + "github.com/sipeed/picoclaw/pkg/bus" + "github.com/sipeed/picoclaw/pkg/channels" + "github.com/sipeed/picoclaw/pkg/config" ) -func TestTryHandleCommand_UsesDispatcher(t *testing.T) { - ch := &WhatsAppChannel{} - called := false - ch.dispatcher = commands.DispatchFunc(func(context.Context, commands.Request) commands.Result { - called = true - return commands.Result{Matched: true, Handled: true} +func TestHandleIncomingMessage_DoesNotConsumeGenericCommandsLocally(t *testing.T) { + messageBus := bus.NewMessageBus() + ch := &WhatsAppChannel{ + BaseChannel: channels.NewBaseChannel("whatsapp", config.WhatsAppConfig{}, messageBus, nil), + ctx: context.Background(), + } + + ch.handleIncomingMessage(map[string]any{ + "type": "message", + "id": "mid1", + "from": "user1", + "chat": "chat1", + "content": "/help", }) - handled := ch.tryHandleCommand(context.Background(), "/help", "chat1", "user1", "mid1") - if !handled || !called { - t.Fatalf("handled=%v called=%v", handled, called) - } -} - -func TestTryHandleCommand_MatchedWithoutHandler_DoesNotFallThrough(t *testing.T) { - ch := &WhatsAppChannel{} - ch.dispatcher = commands.DispatchFunc(func(context.Context, commands.Request) commands.Result { - return commands.Result{Matched: true, Handled: false, Command: "unknown"} - }) - - handled := ch.tryHandleCommand(context.Background(), "/unknown", "chat1", "user1", "mid1") - if !handled { - t.Fatal("expected matched command to be treated as handled") + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + + inbound, ok := messageBus.ConsumeInbound(ctx) + if !ok { + t.Fatal("expected inbound message to be forwarded") + } + if inbound.Channel != "whatsapp" { + t.Fatalf("channel=%q", inbound.Channel) + } + if inbound.Content != "/help" { + t.Fatalf("content=%q", inbound.Content) } } diff --git a/pkg/channels/whatsapp_native/whatsapp_command_test.go b/pkg/channels/whatsapp_native/whatsapp_command_test.go index 3fe7e31bd..cc2dcb619 100644 --- a/pkg/channels/whatsapp_native/whatsapp_command_test.go +++ b/pkg/channels/whatsapp_native/whatsapp_command_test.go @@ -5,32 +5,52 @@ package whatsapp import ( "context" "testing" + "time" - "github.com/sipeed/picoclaw/pkg/commands" + "go.mau.fi/whatsmeow/proto/waE2E" + "go.mau.fi/whatsmeow/types" + "go.mau.fi/whatsmeow/types/events" + "google.golang.org/protobuf/proto" + + "github.com/sipeed/picoclaw/pkg/bus" + "github.com/sipeed/picoclaw/pkg/channels" + "github.com/sipeed/picoclaw/pkg/config" ) -func TestTryHandleCommand_UsesDispatcher(t *testing.T) { - ch := &WhatsAppNativeChannel{} - called := false - ch.dispatcher = commands.DispatchFunc(func(context.Context, commands.Request) commands.Result { - called = true - return commands.Result{Matched: true, Handled: true} - }) +func TestHandleIncoming_DoesNotConsumeGenericCommandsLocally(t *testing.T) { + messageBus := bus.NewMessageBus() + ch := &WhatsAppNativeChannel{ + BaseChannel: channels.NewBaseChannel("whatsapp_native", config.WhatsAppConfig{}, messageBus, nil), + runCtx: context.Background(), + } - handled := ch.tryHandleCommand(context.Background(), "/help", "chat1", "user1", "mid1") - if !handled || !called { - t.Fatalf("handled=%v called=%v", handled, called) - } -} - -func TestTryHandleCommand_MatchedWithoutHandler_DoesNotFallThrough(t *testing.T) { - ch := &WhatsAppNativeChannel{} - ch.dispatcher = commands.DispatchFunc(func(context.Context, commands.Request) commands.Result { - return commands.Result{Matched: true, Handled: false, Command: "unknown"} - }) - - handled := ch.tryHandleCommand(context.Background(), "/unknown", "chat1", "user1", "mid1") - if !handled { - t.Fatal("expected matched command to be treated as handled") + evt := &events.Message{ + Info: types.MessageInfo{ + MessageSource: types.MessageSource{ + Sender: types.NewJID("1001", types.DefaultUserServer), + Chat: types.NewJID("1001", types.DefaultUserServer), + }, + ID: "mid1", + PushName: "Alice", + }, + Message: &waE2E.Message{ + Conversation: proto.String("/new"), + }, + } + + ch.handleIncoming(evt) + + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + + inbound, ok := messageBus.ConsumeInbound(ctx) + if !ok { + t.Fatal("expected inbound message to be forwarded") + } + if inbound.Channel != "whatsapp_native" { + t.Fatalf("channel=%q", inbound.Channel) + } + if inbound.Content != "/new" { + t.Fatalf("content=%q", inbound.Content) } } diff --git a/pkg/channels/whatsapp_native/whatsapp_native.go b/pkg/channels/whatsapp_native/whatsapp_native.go index 3e579dea5..188a7c8fa 100644 --- a/pkg/channels/whatsapp_native/whatsapp_native.go +++ b/pkg/channels/whatsapp_native/whatsapp_native.go @@ -30,7 +30,6 @@ import ( "github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/channels" - "github.com/sipeed/picoclaw/pkg/commands" "github.com/sipeed/picoclaw/pkg/config" "github.com/sipeed/picoclaw/pkg/identity" "github.com/sipeed/picoclaw/pkg/logger" @@ -56,7 +55,6 @@ type WhatsAppNativeChannel struct { mu sync.Mutex runCtx context.Context runCancel context.CancelFunc - dispatcher commands.Dispatching reconnectMu sync.Mutex reconnecting bool stopping atomic.Bool // set once Stop begins; prevents new wg.Add calls @@ -78,7 +76,6 @@ func NewWhatsAppNativeChannel( BaseChannel: base, config: cfg, storePath: storePath, - dispatcher: commands.NewDispatcher(commands.NewRegistry(commands.BuiltinDefinitions(nil))), } return c, nil } @@ -390,9 +387,6 @@ func (c *WhatsAppNativeChannel) handleIncoming(evt *events.Message) { if !c.IsAllowedSender(sender) { return } - if c.tryHandleCommand(c.runCtx, content, chatID, senderID, messageID) { - return - } logger.DebugCF( "whatsapp", @@ -402,41 +396,6 @@ func (c *WhatsAppNativeChannel) handleIncoming(evt *events.Message) { c.HandleMessage(c.runCtx, peer, messageID, senderID, chatID, content, mediaPaths, metadata, sender) } -func (c *WhatsAppNativeChannel) tryHandleCommand( - ctx context.Context, - text, chatID, senderID, messageID string, -) bool { - res := c.DispatchCommand(ctx, commands.Request{ - Channel: "whatsapp_native", - ChatID: chatID, - SenderID: senderID, - Text: text, - MessageID: messageID, - Reply: func(text string) error { - return c.Send(ctx, bus.OutboundMessage{ChatID: chatID, Content: text}) - }, - }) - if res.Err != nil { - logger.WarnCF("whatsapp", "Command execution failed", map[string]any{ - "command": res.Command, - "error": res.Err.Error(), - }) - } - if res.Matched && !res.Handled { - logger.DebugCF("whatsapp", "Command matched without handler; passing to normal flow", map[string]any{ - "command": res.Command, - }) - } - return res.Matched -} - -func (c *WhatsAppNativeChannel) DispatchCommand(ctx context.Context, req commands.Request) commands.Result { - if c.dispatcher == nil { - return commands.Result{Matched: false} - } - return c.dispatcher.Dispatch(ctx, req) -} - func (c *WhatsAppNativeChannel) Send(ctx context.Context, msg bus.OutboundMessage) error { if !c.IsRunning() { return channels.ErrNotRunning diff --git a/pkg/commands/builtin.go b/pkg/commands/builtin.go index c1db0fd55..431cee441 100644 --- a/pkg/commands/builtin.go +++ b/pkg/commands/builtin.go @@ -3,12 +3,38 @@ package commands import ( "context" "fmt" + "strconv" "strings" "github.com/sipeed/picoclaw/pkg/config" ) func BuiltinDefinitions(cfg *config.Config) []Definition { + return builtinDefinitions(cfg, nil) +} + +// BuiltinDefinitionsWithRuntime returns builtin command definitions with runtime-backed +// session command handlers enabled only when runtime is usable. +func BuiltinDefinitionsWithRuntime(cfg *config.Config, runtime Runtime) []Definition { + return builtinDefinitions(cfg, runtime) +} + +func builtinDefinitions(cfg *config.Config, runtime Runtime) []Definition { + // Runtime-backed handlers keep session-aware commands decoupled from concrete + // agent/channel implementations. Commands are enabled only when runtime is valid. + sessionRuntime := runtimeIfUsable(runtime) + + var newHandler Handler + var sessionHandler Handler + if sessionRuntime != nil { + newHandler = func(_ context.Context, req Request) error { + return handleNewCommand(req, sessionRuntime, cfg) + } + sessionHandler = func(_ context.Context, req Request) error { + return handleSessionCommand(req, sessionRuntime) + } + } + return []Definition{ { Name: "start", @@ -33,40 +59,20 @@ func BuiltinDefinitions(cfg *config.Config) []Definition { Aliases: []string{"reset"}, Description: "Start a new chat session", Usage: "/new", + Handler: newHandler, }, { Name: "session", Description: "Manage chat sessions", Usage: "/session [list|resume ]", + Handler: sessionHandler, }, { 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)) - } + return handleShowCommand(req, cfg) }, }, { @@ -74,34 +80,7 @@ func BuiltinDefinitions(cfg *config.Config) []Definition { 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)) - } + return handleListCommand(req, cfg) }, }, } @@ -137,13 +116,178 @@ func commandArgs(text string) string { func replyText(text string) Handler { return func(_ context.Context, req Request) error { - if req.Reply == nil { - return nil - } - return req.Reply(text) + return reply(req, text) } } +func handleNewCommand(req Request, runtime Runtime, fallbackCfg *config.Config) error { + // /new rotates the active session within one runtime scope, then prunes + // older sessions according to configured backlog policy. + scopeKey := runtime.ScopeKey() + newSessionKey, err := runtime.SessionOps().StartNew(scopeKey) + if err != nil { + return reply(req, fmt.Sprintf("Failed to start new session: %v", err)) + } + + backlogLimit := config.DefaultSessionBacklogLimit + cfg := fallbackCfg + if runtime.Config() != nil { + cfg = runtime.Config() + } + if cfg != nil { + backlogLimit = cfg.Session.EffectiveBacklogLimit() + } + + pruned, err := runtime.SessionOps().Prune(scopeKey, backlogLimit) + if err != nil { + return reply(req, fmt.Sprintf( + "Started new session (%s), but pruning old sessions failed: %v", + newSessionKey, + err, + )) + } + + if len(pruned) == 0 { + return reply(req, fmt.Sprintf("Started new session: %s", newSessionKey)) + } + return reply(req, fmt.Sprintf("Started new session: %s (pruned %d old session(s))", newSessionKey, len(pruned))) +} + +func handleSessionCommand(req Request, runtime Runtime) error { + // /session subcommands always operate on runtime scope to prevent cross-chat + // session pointer leakage. + args := strings.Fields(commandArgs(req.Text)) + if len(args) < 1 { + return reply(req, "Usage: /session [list|resume ]") + } + + scopeKey := runtime.ScopeKey() + switch args[0] { + case "list": + list, err := runtime.SessionOps().List(scopeKey) + if err != nil { + return reply(req, fmt.Sprintf("Failed to list sessions: %v", err)) + } + if len(list) == 0 { + return reply(req, "No sessions found for current chat.") + } + + lines := make([]string, 0, len(list)+1) + lines = append(lines, "Sessions for current chat:") + for _, item := range list { + activeMarker := " " + if item.Active { + activeMarker = "*" + } + updated := "-" + if !item.UpdatedAt.IsZero() { + updated = item.UpdatedAt.Format("2006-01-02 15:04") + } + lines = append(lines, fmt.Sprintf( + "%d. [%s] %s (%d msgs, updated %s)", + item.Ordinal, + activeMarker, + item.SessionKey, + item.MessageCnt, + updated, + )) + } + return reply(req, strings.Join(lines, "\n")) + + case "resume": + if len(args) != 2 { + return reply(req, "Usage: /session resume ") + } + index, err := strconv.Atoi(args[1]) + if err != nil || index < 1 { + return reply(req, "Usage: /session resume ") + } + sessionKey, err := runtime.SessionOps().Resume(scopeKey, index) + if err != nil { + return reply(req, fmt.Sprintf("Failed to resume session %d: %v", index, err)) + } + return reply(req, fmt.Sprintf("Resumed session %d: %s", index, sessionKey)) + + default: + return reply(req, "Usage: /session [list|resume ]") + } +} + +func handleShowCommand(req Request, cfg *config.Config) error { + if cfg == nil { + return reply(req, "Command unavailable in current context.") + } + + args := commandArgs(req.Text) + if args == "" { + return reply(req, "Usage: /show [model|channel]") + } + + switch args { + case "model": + return reply(req, fmt.Sprintf( + "Current Model: %s (Provider: %s)", + cfg.Agents.Defaults.GetModelName(), + cfg.Agents.Defaults.Provider, + )) + case "channel": + return reply(req, fmt.Sprintf("Current Channel: %s", req.Channel)) + default: + return reply(req, fmt.Sprintf("Unknown parameter: %s. Try 'model' or 'channel'.", args)) + } +} + +func handleListCommand(req Request, cfg *config.Config) error { + if cfg == nil { + return reply(req, "Command unavailable in current context.") + } + + args := commandArgs(req.Text) + if args == "" { + return reply(req, "Usage: /list [models|channels]") + } + + switch args { + case "models": + provider := cfg.Agents.Defaults.Provider + if provider == "" { + provider = "configured default" + } + return reply(req, 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 reply(req, fmt.Sprintf("Enabled Channels:\n- %s", strings.Join(enabled, "\n- "))) + default: + return reply(req, fmt.Sprintf("Unknown parameter: %s. Try 'models' or 'channels'.", args)) + } +} + +func reply(req Request, text string) error { + if req.Reply == nil { + return nil + } + return req.Reply(text) +} + +func runtimeIfUsable(runtime Runtime) Runtime { + // Guardrails: runtime-backed handlers are disabled unless scope and session ops + // are both present, so command registration can still expose metadata safely. + if runtime == nil { + return nil + } + if runtime.SessionOps() == nil { + return nil + } + if strings.TrimSpace(runtime.ScopeKey()) == "" { + return nil + } + return runtime +} + func enabledChannels(cfg *config.Config) []string { enabled := make([]string, 0, 8) if cfg.Channels.Telegram.Enabled { diff --git a/pkg/commands/builtin_test.go b/pkg/commands/builtin_test.go index 5e04008db..4d77ab185 100644 --- a/pkg/commands/builtin_test.go +++ b/pkg/commands/builtin_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/session" ) func findDefinitionByName(t *testing.T, defs []Definition, name string) Definition { @@ -116,3 +117,40 @@ func TestBuiltinListChannels_UsesConfigEnabledChannels(t *testing.T) { t.Fatalf("/list channels reply=%q, want telegram and slack", reply) } } + +type builtinTestSessionOps struct{} + +func (f *builtinTestSessionOps) ResolveActive(scopeKey string) (string, error) { return "", nil } +func (f *builtinTestSessionOps) StartNew(scopeKey string) (string, error) { return "", nil } +func (f *builtinTestSessionOps) List(scopeKey string) ([]session.SessionMeta, error) { + return nil, nil +} +func (f *builtinTestSessionOps) Resume(scopeKey string, index int) (string, error) { + return "", nil +} +func (f *builtinTestSessionOps) Prune(scopeKey string, limit int) ([]string, error) { + return nil, nil +} + +type builtinTestRuntime struct { + scope string + ops SessionOps +} + +func (f *builtinTestRuntime) ScopeKey() string { return f.scope } +func (f *builtinTestRuntime) SessionOps() SessionOps { return f.ops } +func (f *builtinTestRuntime) Config() *config.Config { return nil } + +func TestBuiltinDefinitionsWithRuntime_EnablesSessionHandlers(t *testing.T) { + runtime := &builtinTestRuntime{scope: "scope", ops: &builtinTestSessionOps{}} + defs := BuiltinDefinitionsWithRuntime(nil, runtime) + + newDef := findDefinitionByName(t, defs, "new") + sessionDef := findDefinitionByName(t, defs, "session") + if newDef.Handler == nil { + t.Fatalf("/new should provide runtime-backed handler when runtime is available") + } + if sessionDef.Handler == nil { + t.Fatalf("/session should provide runtime-backed handler when runtime is available") + } +} diff --git a/pkg/commands/dispatcher.go b/pkg/commands/dispatcher.go index ca617ded6..205b10f99 100644 --- a/pkg/commands/dispatcher.go +++ b/pkg/commands/dispatcher.go @@ -16,59 +16,6 @@ type Request struct { Reply func(text string) error } -type Result struct { - Matched bool - Handled bool - Command string - Err error -} - -type Dispatcher struct { - reg *Registry -} - -type Dispatching interface { - Dispatch(ctx context.Context, req Request) Result -} - -type DispatchFunc func(ctx context.Context, req Request) Result - -func (f DispatchFunc) Dispatch(ctx context.Context, req Request) Result { - return f(ctx, req) -} - -var commandPrefixes = []string{"/", "!"} - -// NewDispatcher binds the unified parser/executor flow to one command registry. -func NewDispatcher(reg *Registry) *Dispatcher { - return &Dispatcher{reg: reg} -} - -// Dispatch parses slash commands and executes handlers from the shared registry. -// Unmatched messages intentionally return Matched=false so callers can fall back -// to normal agent message handling. -func (d *Dispatcher) Dispatch(ctx context.Context, req Request) Result { - cmdName, ok := parseCommandName(req.Text) - if !ok { - return Result{Matched: false} - } - - for _, def := range d.reg.Definitions() { - if !matchesCommand(def, cmdName) { - continue - } - if def.Handler == nil { - // Definition-only command (for menu registration / discovery). - // Let the inbound message continue to the agent loop. - return Result{Matched: false, Handled: false, Command: def.Name} - } - err := def.Handler(ctx, req) - return Result{Matched: true, Handled: true, Command: def.Name, Err: err} - } - - return Result{Matched: false} -} - func firstToken(input string) string { parts := strings.Fields(strings.TrimSpace(input)) if len(parts) == 0 { @@ -77,53 +24,24 @@ func firstToken(input string) string { return parts[0] } -// parseCommandName accepts "/name", "!name", and Telegram's "/name@bot", then -// normalizes to lowercase command names. +// parseCommandName accepts both "/name" and "/name@bot", then normalizes to "name". func parseCommandName(input string) (string, bool) { token := firstToken(input) - if token == "" { + if token == "" || !strings.HasPrefix(token, "/") { return "", false } - name, ok := trimCommandPrefix(token) - if !ok { - return "", false - } + name := strings.TrimPrefix(token, "/") if i := strings.Index(name, "@"); i >= 0 { name = name[:i] } - name = normalizeCommandName(name) + name = strings.TrimSpace(name) if name == "" { return "", false } return name, true } -func trimCommandPrefix(token string) (string, bool) { - for _, prefix := range commandPrefixes { - if strings.HasPrefix(token, prefix) { - return strings.TrimPrefix(token, prefix), true - } - } - return "", false -} - -func normalizeCommandName(name string) string { - return strings.ToLower(strings.TrimSpace(name)) -} - -func matchesCommand(def Definition, cmdName string) bool { - if normalizeCommandName(def.Name) == cmdName { - return true - } - for _, alias := range def.Aliases { - if normalizeCommandName(alias) == cmdName { - return true - } - } - return false -} - func contains(items []string, target string) bool { for _, item := range items { if item == target { diff --git a/pkg/commands/dispatcher_test.go b/pkg/commands/dispatcher_test.go deleted file mode 100644 index 3c1c0d7dd..000000000 --- a/pkg/commands/dispatcher_test.go +++ /dev/null @@ -1,117 +0,0 @@ -package commands - -import ( - "context" - "testing" -) - -func TestDispatcher_MatchSlashCommand(t *testing.T) { - called := false - defs := []Definition{ - { - Name: "help", - Handler: func(context.Context, Request) error { - called = true - return nil - }, - }, - } - d := NewDispatcher(NewRegistry(defs)) - - res := d.Dispatch(context.Background(), Request{ - Channel: "telegram", - Text: "/help", - }) - if !res.Matched || !called || res.Err != nil { - t.Fatalf("dispatch result = %+v, called=%v", res, called) - } -} - -func TestDispatcher_DoesNotMatchWithoutSlash(t *testing.T) { - d := NewDispatcher(NewRegistry([]Definition{{Name: "help"}})) - - res := d.Dispatch(context.Background(), Request{ - Channel: "telegram", - Text: "help", - }) - if res.Matched { - t.Fatalf("expected unmatched for plain text, got %+v", res) - } -} - -func TestDispatcher_MatchTelegramMentionSyntax(t *testing.T) { - called := false - d := NewDispatcher(NewRegistry([]Definition{ - { - Name: "help", - Handler: func(context.Context, Request) error { - called = true - return nil - }, - }, - })) - - res := d.Dispatch(context.Background(), Request{ - Channel: "telegram", - Text: "/help@my_bot", - }) - if !res.Matched || !res.Handled || !called || res.Err != nil { - t.Fatalf("dispatch result = %+v, called=%v", res, called) - } -} - -func TestDispatcher_MatchBangPrefix(t *testing.T) { - called := false - d := NewDispatcher(NewRegistry([]Definition{ - { - Name: "help", - Handler: func(context.Context, Request) error { - called = true - return nil - }, - }, - })) - - res := d.Dispatch(context.Background(), Request{ - Channel: "telegram", - Text: "!help", - }) - if !res.Matched || !res.Handled || !called || res.Err != nil { - t.Fatalf("dispatch result = %+v, called=%v", res, called) - } -} - -func TestDispatcher_CommandMatchingIsCaseInsensitive(t *testing.T) { - called := false - d := NewDispatcher(NewRegistry([]Definition{ - { - Name: "show", - Handler: func(context.Context, Request) error { - called = true - return nil - }, - }, - })) - - res := d.Dispatch(context.Background(), Request{ - Channel: "telegram", - Text: "/SHOW", - }) - if !res.Matched || !res.Handled || !called || res.Err != nil { - t.Fatalf("dispatch result = %+v, called=%v", res, called) - } -} - -func TestDispatcher_PassThroughDefinitionWithoutHandler(t *testing.T) { - d := NewDispatcher(NewRegistry([]Definition{ - {Name: "session"}, // menu-only / pass-through definition - })) - - res := d.Dispatch(context.Background(), Request{ - Channel: "telegram", - Text: "/session list", - }) - if res.Matched { - t.Fatalf("expected pass-through unmatched result, got %+v", res) - } -} diff --git a/pkg/commands/executor.go b/pkg/commands/executor.go new file mode 100644 index 000000000..69d4a83b0 --- /dev/null +++ b/pkg/commands/executor.go @@ -0,0 +1,71 @@ +package commands + +import ( + "context" +) + +type Outcome int + +const ( + // OutcomePassthrough means this input should continue through normal agent flow. + OutcomePassthrough Outcome = iota + // OutcomeHandled means a command handler executed (with or without handler error). + OutcomeHandled +) + +type ExecuteResult struct { + Outcome Outcome + Command string + Reply string + Err error +} + +type Executor struct { + reg *Registry +} + +func NewExecutor(reg *Registry) *Executor { + return &Executor{reg: reg} +} + +// Execute implements a two-state command decision: +// 1) handled: execute command immediately; +// 2) passthrough: not a command or intentionally deferred to agent logic. +func (e *Executor) Execute(ctx context.Context, req Request) ExecuteResult { + cmdName, ok := parseCommandName(req.Text) + if !ok { + return ExecuteResult{Outcome: OutcomePassthrough} + } + + if e == nil || e.reg == nil { + return ExecuteResult{Outcome: OutcomePassthrough, Command: cmdName} + } + + passthroughCommand := "" + + for _, def := range e.reg.Definitions() { + if !matchesCommand(def, cmdName) { + continue + } + if passthroughCommand == "" { + passthroughCommand = def.Name + } + if def.Handler == nil { + continue + } + err := def.Handler(ctx, req) + return ExecuteResult{Outcome: OutcomeHandled, Command: def.Name, Err: err} + } + if passthroughCommand != "" { + return ExecuteResult{Outcome: OutcomePassthrough, Command: passthroughCommand} + } + + return ExecuteResult{Outcome: OutcomePassthrough, Command: cmdName} +} + +func matchesCommand(def Definition, cmdName string) bool { + if def.Name == cmdName { + return true + } + return contains(def.Aliases, cmdName) +} diff --git a/pkg/commands/executor_test.go b/pkg/commands/executor_test.go new file mode 100644 index 000000000..ebf091413 --- /dev/null +++ b/pkg/commands/executor_test.go @@ -0,0 +1,155 @@ +package commands + +import ( + "context" + "errors" + "testing" +) + +func TestExecutor_RegisteredWithoutHandler_ReturnsPassthrough(t *testing.T) { + defs := []Definition{{Name: "show"}} + ex := NewExecutor(NewRegistry(defs)) + + res := ex.Execute(context.Background(), Request{Channel: "whatsapp", Text: "/show"}) + if res.Outcome != OutcomePassthrough { + t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomePassthrough) + } +} + +func TestExecutor_UnknownSlashCommand_ReturnsPassthrough(t *testing.T) { + defs := []Definition{{Name: "show"}} + ex := NewExecutor(NewRegistry(defs)) + + res := ex.Execute(context.Background(), Request{Channel: "telegram", Text: "/unknown"}) + if res.Outcome != OutcomePassthrough { + t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomePassthrough) + } +} + +func TestExecutor_SupportedCommandWithHandler_ReturnsHandled(t *testing.T) { + called := false + defs := []Definition{ + { + Name: "help", + Handler: func(context.Context, Request) error { + called = true + return nil + }, + }, + } + ex := NewExecutor(NewRegistry(defs)) + + res := ex.Execute(context.Background(), Request{Channel: "telegram", Text: "/help@my_bot"}) + if res.Outcome != OutcomeHandled { + t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled) + } + if !called { + t.Fatalf("expected handler to be called") + } +} + +func TestExecutor_AliasWithoutHandler_ReturnsPassthrough(t *testing.T) { + defs := []Definition{ + { + Name: "show", + Aliases: []string{"display"}, + }, + } + ex := NewExecutor(NewRegistry(defs)) + + res := ex.Execute(context.Background(), Request{Channel: "whatsapp", Text: "/display"}) + if res.Outcome != OutcomePassthrough { + t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomePassthrough) + } + if res.Command != "show" { + t.Fatalf("command=%q, want=%q", res.Command, "show") + } +} + +func TestExecutor_AliasWithHandler_ReturnsHandled(t *testing.T) { + called := false + defs := []Definition{ + { + Name: "new", + Aliases: []string{"reset"}, + Handler: func(context.Context, Request) error { + called = true + return nil + }, + }, + } + ex := NewExecutor(NewRegistry(defs)) + + res := ex.Execute(context.Background(), Request{Channel: "telegram", Text: "/reset"}) + if res.Outcome != OutcomeHandled { + t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled) + } + if res.Command != "new" { + t.Fatalf("command=%q, want=%q", res.Command, "new") + } + if !called { + t.Fatalf("expected handler to be called") + } +} + +func TestExecutor_SupportedCommandWithNilHandler_ReturnsPassthrough(t *testing.T) { + defs := []Definition{ + {Name: "session"}, + } + ex := NewExecutor(NewRegistry(defs)) + + res := ex.Execute(context.Background(), Request{Channel: "telegram", Text: "/session list"}) + if res.Outcome != OutcomePassthrough { + t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomePassthrough) + } + if res.Command != "session" { + t.Fatalf("command=%q, want=%q", res.Command, "session") + } +} + +func TestExecutor_NilHandlerDoesNotMaskLaterHandler(t *testing.T) { + called := false + defs := []Definition{ + {Name: "session"}, + { + Name: "session", + Handler: func(context.Context, Request) error { + called = true + return nil + }, + }, + } + ex := NewExecutor(NewRegistry(defs)) + + res := ex.Execute(context.Background(), Request{Channel: "telegram", Text: "/session"}) + if res.Outcome != OutcomeHandled { + t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled) + } + if res.Command != "session" { + t.Fatalf("command=%q, want=%q", res.Command, "session") + } + if !called { + t.Fatalf("expected later handler to be called") + } +} + +func TestExecutor_HandlerErrorIsPropagated(t *testing.T) { + wantErr := errors.New("handler failed") + defs := []Definition{ + { + Name: "help", + Handler: func(context.Context, Request) error { + return wantErr + }, + }, + } + ex := NewExecutor(NewRegistry(defs)) + + res := ex.Execute(context.Background(), Request{Channel: "telegram", Text: "/help"}) + if res.Outcome != OutcomeHandled { + t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled) + } + if !errors.Is(res.Err, wantErr) { + t.Fatalf("err=%v, want=%v", res.Err, wantErr) + } +} diff --git a/pkg/commands/runtime.go b/pkg/commands/runtime.go new file mode 100644 index 000000000..8731494de --- /dev/null +++ b/pkg/commands/runtime.go @@ -0,0 +1,31 @@ +package commands + +import ( + "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/session" +) + +// SessionOps defines the session lifecycle operations command handlers rely on. +// Implementations are expected to be scope-aware and deterministic for a given scopeKey. +type SessionOps interface { + // ResolveActive returns the active session key for scopeKey, creating default scope state if needed. + ResolveActive(scopeKey string) (string, error) + // StartNew creates and activates a new session for scopeKey and returns its key. + StartNew(scopeKey string) (string, error) + // List returns ordered session metadata for scopeKey. + List(scopeKey string) ([]session.SessionMeta, error) + // Resume activates the session at a 1-based index within scopeKey and returns its key. + Resume(scopeKey string, index int) (string, error) + // Prune deletes older sessions for scopeKey according to limit and returns deleted session keys. + Prune(scopeKey string, limit int) ([]string, error) +} + +// Runtime exposes the minimal agent runtime state needed by command handlers. +type Runtime interface { + // ScopeKey returns the resolved scope identifier used for session operations. + ScopeKey() string + // SessionOps returns scoped session lifecycle operations. + SessionOps() SessionOps + // Config returns process config for read-only access by handlers; callers must not mutate it. + Config() *config.Config +} diff --git a/pkg/commands/runtime_test.go b/pkg/commands/runtime_test.go new file mode 100644 index 000000000..aef3d9b19 --- /dev/null +++ b/pkg/commands/runtime_test.go @@ -0,0 +1,50 @@ +package commands + +import ( + "testing" + + "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/session" +) + +type fakeSessionOps struct{} + +func (f *fakeSessionOps) ResolveActive(scopeKey string) (string, error) { + return "", nil +} + +func (f *fakeSessionOps) StartNew(scopeKey string) (string, error) { + return "", nil +} + +func (f *fakeSessionOps) List(scopeKey string) ([]session.SessionMeta, error) { + return nil, nil +} + +func (f *fakeSessionOps) Resume(scopeKey string, index int) (string, error) { + return "", nil +} + +func (f *fakeSessionOps) Prune(scopeKey string, limit int) ([]string, error) { + return nil, nil +} + +type fakeRuntime struct{} + +func (f *fakeRuntime) ScopeKey() string { + return "" +} + +func (f *fakeRuntime) SessionOps() SessionOps { + return &fakeSessionOps{} +} + +func (f *fakeRuntime) Config() *config.Config { + return &config.Config{} +} + +func TestRuntimeContracts_MinimalSessionOps(t *testing.T) { + var _ SessionOps = (*fakeSessionOps)(nil) + var _ SessionOps = (*session.SessionManager)(nil) + var _ Runtime = (*fakeRuntime)(nil) +} diff --git a/pkg/commands/session_handlers_test.go b/pkg/commands/session_handlers_test.go new file mode 100644 index 000000000..4a82e90ef --- /dev/null +++ b/pkg/commands/session_handlers_test.go @@ -0,0 +1,324 @@ +package commands + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/session" +) + +type sessionHandlerFakeSessionOps struct { + startNewScopeKeys []string + startNewValue string + startNewErr error + + pruneScopeKeys []string + pruneLimits []int + pruneValue []string + pruneErr error + + listScopeKeys []string + listValue []session.SessionMeta + listErr error + + resumeScopeKeys []string + resumeIndices []int + resumeValue string + resumeErr error +} + +func (f *sessionHandlerFakeSessionOps) ResolveActive(scopeKey string) (string, error) { + return "", nil +} + +func (f *sessionHandlerFakeSessionOps) StartNew(scopeKey string) (string, error) { + f.startNewScopeKeys = append(f.startNewScopeKeys, scopeKey) + return f.startNewValue, f.startNewErr +} + +func (f *sessionHandlerFakeSessionOps) List(scopeKey string) ([]session.SessionMeta, error) { + f.listScopeKeys = append(f.listScopeKeys, scopeKey) + return f.listValue, f.listErr +} + +func (f *sessionHandlerFakeSessionOps) Resume(scopeKey string, index int) (string, error) { + f.resumeScopeKeys = append(f.resumeScopeKeys, scopeKey) + f.resumeIndices = append(f.resumeIndices, index) + return f.resumeValue, f.resumeErr +} + +func (f *sessionHandlerFakeSessionOps) Prune(scopeKey string, limit int) ([]string, error) { + f.pruneScopeKeys = append(f.pruneScopeKeys, scopeKey) + f.pruneLimits = append(f.pruneLimits, limit) + return f.pruneValue, f.pruneErr +} + +type sessionHandlerFakeRuntime struct { + scope string + ops SessionOps + cfg *config.Config +} + +func (f *sessionHandlerFakeRuntime) ScopeKey() string { + return f.scope +} + +func (f *sessionHandlerFakeRuntime) SessionOps() SessionOps { + return f.ops +} + +func (f *sessionHandlerFakeRuntime) Config() *config.Config { + return f.cfg +} + +func TestSessionHandlers_New_UsesRuntimeSessionOps(t *testing.T) { + ops := &sessionHandlerFakeSessionOps{ + startNewValue: "scope#2", + pruneValue: []string{"scope#1"}, + } + runtime := &sessionHandlerFakeRuntime{ + scope: "scope", + ops: ops, + cfg: &config.Config{ + Session: config.SessionConfig{BacklogLimit: 7}, + }, + } + + var reply string + ex := NewExecutor(NewRegistry(BuiltinDefinitionsWithRuntime(nil, runtime))) + res := ex.Execute(context.Background(), Request{ + Channel: "whatsapp", + Text: "/new", + Reply: func(text string) error { + reply = text + return nil + }, + }) + + if res.Outcome != OutcomeHandled { + t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled) + } + if len(ops.startNewScopeKeys) != 1 || ops.startNewScopeKeys[0] != "scope" { + t.Fatalf("startNew calls=%v, want [scope]", ops.startNewScopeKeys) + } + if len(ops.pruneScopeKeys) != 1 || ops.pruneScopeKeys[0] != "scope" { + t.Fatalf("prune scope calls=%v, want [scope]", ops.pruneScopeKeys) + } + if len(ops.pruneLimits) != 1 || ops.pruneLimits[0] != 7 { + t.Fatalf("prune limits=%v, want [7]", ops.pruneLimits) + } + if reply != "Started new session: scope#2 (pruned 1 old session(s))" { + t.Fatalf("reply=%q", reply) + } +} + +func TestSessionHandlers_SessionResume_UsesRuntimeSessionOps(t *testing.T) { + ops := &sessionHandlerFakeSessionOps{resumeValue: "scope#3"} + runtime := &sessionHandlerFakeRuntime{ + scope: "scope", + ops: ops, + cfg: &config.Config{}, + } + + var reply string + ex := NewExecutor(NewRegistry(BuiltinDefinitionsWithRuntime(nil, runtime))) + res := ex.Execute(context.Background(), Request{ + Channel: "whatsapp", + Text: "/session resume 3", + Reply: func(text string) error { + reply = text + return nil + }, + }) + + if res.Outcome != OutcomeHandled { + t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled) + } + if len(ops.resumeScopeKeys) != 1 || ops.resumeScopeKeys[0] != "scope" { + t.Fatalf("resume scope calls=%v, want [scope]", ops.resumeScopeKeys) + } + if len(ops.resumeIndices) != 1 || ops.resumeIndices[0] != 3 { + t.Fatalf("resume indices=%v, want [3]", ops.resumeIndices) + } + if reply != "Resumed session 3: scope#3" { + t.Fatalf("reply=%q", reply) + } +} + +func TestSessionHandlers_SessionList_UsesRuntimeSessionOps(t *testing.T) { + ops := &sessionHandlerFakeSessionOps{ + listValue: []session.SessionMeta{ + { + Ordinal: 1, + SessionKey: "scope#3", + UpdatedAt: time.Date(2026, 3, 1, 9, 7, 0, 0, time.UTC), + MessageCnt: 4, + Active: true, + }, + }, + } + runtime := &sessionHandlerFakeRuntime{ + scope: "scope", + ops: ops, + cfg: &config.Config{}, + } + + var reply string + ex := NewExecutor(NewRegistry(BuiltinDefinitionsWithRuntime(nil, runtime))) + res := ex.Execute(context.Background(), Request{ + Channel: "whatsapp", + Text: "/session list", + Reply: func(text string) error { + reply = text + return nil + }, + }) + + if res.Outcome != OutcomeHandled { + t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled) + } + if len(ops.listScopeKeys) != 1 || ops.listScopeKeys[0] != "scope" { + t.Fatalf("list scope calls=%v, want [scope]", ops.listScopeKeys) + } + if reply != "Sessions for current chat:\n1. [*] scope#3 (4 msgs, updated 2026-03-01 09:07)" { + t.Fatalf("reply=%q", reply) + } +} + +func TestSessionHandlers_MissingRuntime_Passthrough(t *testing.T) { + ex := NewExecutor(NewRegistry(BuiltinDefinitionsWithRuntime(nil, nil))) + + for _, input := range []string{"/new", "/session list"} { + res := ex.Execute(context.Background(), Request{ + Channel: "whatsapp", + Text: input, + }) + if res.Outcome != OutcomePassthrough { + t.Fatalf("text=%q outcome=%v, want=%v", input, res.Outcome, OutcomePassthrough) + } + } +} + +func TestSessionHandlers_NilSessionOps_Passthrough(t *testing.T) { + runtime := &sessionHandlerFakeRuntime{ + scope: "scope", + ops: nil, + cfg: &config.Config{}, + } + ex := NewExecutor(NewRegistry(BuiltinDefinitionsWithRuntime(nil, runtime))) + + for _, input := range []string{"/new", "/session list"} { + res := ex.Execute(context.Background(), Request{ + Channel: "whatsapp", + Text: input, + }) + if res.Outcome != OutcomePassthrough { + t.Fatalf("text=%q outcome=%v, want=%v", input, res.Outcome, OutcomePassthrough) + } + } +} + +func TestSessionHandlers_EmptyScope_Passthrough(t *testing.T) { + runtime := &sessionHandlerFakeRuntime{ + scope: " ", + ops: &sessionHandlerFakeSessionOps{}, + cfg: &config.Config{}, + } + ex := NewExecutor(NewRegistry(BuiltinDefinitionsWithRuntime(nil, runtime))) + + for _, input := range []string{"/new", "/session list"} { + res := ex.Execute(context.Background(), Request{ + Channel: "whatsapp", + Text: input, + }) + if res.Outcome != OutcomePassthrough { + t.Fatalf("text=%q outcome=%v, want=%v", input, res.Outcome, OutcomePassthrough) + } + } +} + +func TestSessionHandlers_ErrorAndValidationReplies(t *testing.T) { + tests := []struct { + name string + text string + ops *sessionHandlerFakeSessionOps + wantReply string + }{ + { + name: "start new error", + text: "/new", + ops: &sessionHandlerFakeSessionOps{startNewErr: errors.New("boom")}, + wantReply: "Failed to start new session: boom", + }, + { + name: "prune error", + text: "/new", + ops: &sessionHandlerFakeSessionOps{ + startNewValue: "scope#2", + pruneErr: errors.New("prune failed"), + }, + wantReply: "Started new session (scope#2), but pruning old sessions failed: prune failed", + }, + { + name: "list error", + text: "/session list", + ops: &sessionHandlerFakeSessionOps{listErr: errors.New("list failed")}, + wantReply: "Failed to list sessions: list failed", + }, + { + name: "resume error", + text: "/session resume 2", + ops: &sessionHandlerFakeSessionOps{resumeErr: errors.New("resume failed")}, + wantReply: "Failed to resume session 2: resume failed", + }, + { + name: "resume missing index", + text: "/session resume", + ops: &sessionHandlerFakeSessionOps{}, + wantReply: "Usage: /session resume ", + }, + { + name: "resume non numeric index", + text: "/session resume abc", + ops: &sessionHandlerFakeSessionOps{}, + wantReply: "Usage: /session resume ", + }, + { + name: "resume zero index", + text: "/session resume 0", + ops: &sessionHandlerFakeSessionOps{}, + wantReply: "Usage: /session resume ", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + runtime := &sessionHandlerFakeRuntime{ + scope: "scope", + ops: tc.ops, + cfg: &config.Config{}, + } + + var reply string + ex := NewExecutor(NewRegistry(BuiltinDefinitionsWithRuntime(nil, runtime))) + res := ex.Execute(context.Background(), Request{ + Channel: "whatsapp", + Text: tc.text, + Reply: func(text string) error { + reply = text + return nil + }, + }) + + if res.Outcome != OutcomeHandled { + t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled) + } + if reply != tc.wantReply { + t.Fatalf("reply=%q, want=%q", reply, tc.wantReply) + } + }) + } +} diff --git a/pkg/commands/show_list_handlers_test.go b/pkg/commands/show_list_handlers_test.go new file mode 100644 index 000000000..bfea94b99 --- /dev/null +++ b/pkg/commands/show_list_handlers_test.go @@ -0,0 +1,88 @@ +package commands + +import ( + "context" + "strings" + "testing" + + "github.com/sipeed/picoclaw/pkg/config" +) + +func TestShowListHandlers_ChannelPolicy(t *testing.T) { + cfg := &config.Config{} + ex := NewExecutor(NewRegistry(BuiltinDefinitions(cfg))) + + var telegramReply string + handled := ex.Execute(context.Background(), Request{ + Channel: "telegram", + Text: "/show channel", + Reply: func(text string) error { + telegramReply = text + return nil + }, + }) + if handled.Outcome != OutcomeHandled { + t.Fatalf("telegram /show outcome=%v, want=%v", handled.Outcome, OutcomeHandled) + } + if telegramReply != "Current Channel: telegram" { + t.Fatalf("telegram /show reply=%q, want=%q", telegramReply, "Current Channel: telegram") + } + + var whatsappReply string + handledWhatsApp := ex.Execute(context.Background(), Request{ + Channel: "whatsapp", + Text: "/show channel", + Reply: func(text string) error { + whatsappReply = text + return nil + }, + }) + if handledWhatsApp.Outcome != OutcomeHandled { + t.Fatalf("whatsapp /show outcome=%v, want=%v", handledWhatsApp.Outcome, OutcomeHandled) + } + if handledWhatsApp.Command != "show" { + t.Fatalf("whatsapp /show command=%q, want=%q", handledWhatsApp.Command, "show") + } + if whatsappReply != "Current Channel: whatsapp" { + t.Fatalf("whatsapp /show reply=%q, want=%q", whatsappReply, "Current Channel: whatsapp") + } + + passthrough := ex.Execute(context.Background(), Request{ + Channel: "whatsapp", + Text: "/foo", + }) + if passthrough.Outcome != OutcomePassthrough { + t.Fatalf("whatsapp /foo outcome=%v, want=%v", passthrough.Outcome, OutcomePassthrough) + } + 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) { + cfg := &config.Config{} + cfg.Channels.Telegram.Enabled = true + ex := NewExecutor(NewRegistry(BuiltinDefinitions(cfg))) + + var reply string + res := ex.Execute(context.Background(), Request{ + Channel: "whatsapp", + Text: "/list channels", + Reply: func(text string) error { + reply = text + return nil + }, + }) + if res.Outcome != OutcomeHandled { + t.Fatalf("whatsapp /list outcome=%v, want=%v", res.Outcome, OutcomeHandled) + } + if res.Command != "list" { + t.Fatalf("whatsapp /list command=%q, want=%q", res.Command, "list") + } + if !strings.Contains(reply, "telegram") { + t.Fatalf("whatsapp /list reply=%q, expected enabled channels content", reply) + } +}