feat(commands): centralized command registry with sub-command routing (#959)

* feat(commands): Session management [Phase 1/2] command centralization and registration * docs: add design for command registry post-review fixes Documents the architecture decisions for fixing 5 Important issues from code review: SubCommand pattern, Deps struct, command-group files, Executor caching, and Telegram registration dedup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(commands): add SubCommand type and EffectiveUsage method Introduce SubCommand struct for declaring sub-commands structurally within a parent command Definition. The EffectiveUsage() method auto-generates usage strings from sub-command names and args, preventing drift between help text and actual handler behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(commands): add Deps struct and secondToken helper, remove dead contains() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(commands): add sub-command routing to Executor Uses Registry.Lookup for O(1) command dispatch instead of iterating all definitions. Definitions with SubCommands are routed to matching sub-command handlers. Missing or unknown sub-commands reply with auto-generated usage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(commands): split into command-group files with Deps injection Extract show/list/start/help into individual cmd_*.go files. Replace config.Config parameter with Deps struct for runtime data. Restore /show agents and /list agents sub-commands. Use EffectiveUsage for auto-generated help text. Bridge external callers (agent/loop.go, telegram.go) with Deps wrapper until Task 5 fully wires the Deps fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * perf(commands): cache Executor in AgentLoop, wire Deps with runtime callbacks Create Executor once in NewAgentLoop instead of per-message. Deps closures capture AgentLoop pointer for late-bound access to channelManager and runtime agent model. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(telegram): remove duplicate initBotCommands, keep async startCommandRegistration only Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore(commands): restore Outcome comments and annotate Deps.Config Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(commands): consolidate /switch into commands package, fix ! prefix Move /switch model and /switch channel handling from inline loop.go logic into cmd_switch.go using the SubCommand + Deps pattern. This removes the OutcomePassthrough branch in handleCommand entirely. Also replace the hardcoded "/" prefix check with commands.HasCommandPrefix so that "!" prefixed commands are correctly routed to the Executor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: add docs/plans to .gitignore and untrack existing files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(commands): address code review findings - Remove dead ExecuteResult.Reply field and unused branch in loop.go - Extract shared agentsHandler for /show agents and /list agents - Remove redundant firstToken/secondToken (use nthToken instead) - Simplify Telegram startup: pass BuiltinDefinitions directly - Centralize req.Reply nil guard in executeDefinition - Extract unavailableMsg constant (was duplicated 5 times) - Remove unused MessageID from Request - Remove stale "reserved for Phase 2" comment on Deps.Config Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(commands): replace Deps with per-request Runtime Separate stateless Registry (cached on AgentLoop) from per-request Runtime (passed to handlers at execution time). This enables future session management features to inject per-request context without modifying the command registry. - Rename Deps → Runtime, move to runtime.go - Change Handler signature: func(ctx, req) error → func(ctx, req, rt *Runtime) error - NewExecutor now takes (registry, runtime) — executor is created per-request - BuiltinDefinitions() no longer takes parameters (stateless) - AgentLoop caches cmdRegistry, builds Runtime via buildRuntime() - Update all cmd_*.go handlers and tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: fix gci import grouping and godoc formatting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(onboard): skip legacy AGENT.md when copying embedded workspace templates The workspace/ directory contains both AGENT.md (legacy) and AGENTS.md (current). copyEmbeddedToTarget was copying both, causing the test TestCopyEmbeddedToTargetUsesAgentsMarkdown to fail. Skip AGENT.md during the walk to match the expected behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(agent): address self-review comments on loop.go - Move cmdRegistry init into struct literal (review comment #11) - Rename buildRuntime → buildCommandsRuntime for clarity (review comment #12) - Add comment to default switch case explaining passthrough (review comment #13) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * 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> * refactor(onboard): extract legacy filename to constant Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(agent): handle commands before route error check Move handleCommand() before the routeErr gate so global commands (/help, /show, /switch) remain available even when routing fails. Context-dependent commands that need a routed agent will report "unavailable" through their nil-Runtime guards. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * revert: remove unnecessary AGENT.md skip in onboard Reverts 02d0c04 and 74deae1. The test failure was caused by a local leftover workspace/AGENT.md file (gitignored but embedded by go:embed). Deleting the local file fixes the root cause; the code-level skip was never needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: executeDefinition Unknown option * fix(agent): use routed agent for model commands, restore Telegram command diff - Remove modelMu: message processing is serial, no concurrent writes - Pass routed agent to handleCommand/buildCommandsRuntime instead of always using default agent - GetModelInfo/SwitchModel are nil when agent is nil (route failed), handlers reply "unavailable" - Restore GetMyCommands + slices.Equal check before SetMyCommands to avoid unnecessary Telegram API calls on restart Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(commands): remove unintended config mutation in SwitchModel SwitchModel should only update the routed agent's runtime Model field. Writing to cfg.Agents.Defaults.ModelName was a behavioral change that corrupts the default agent config when switching a non-default agent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(commands): move /switch channel to /check channel /switch channel only validates availability, not actually switching. Rename to /check channel to match actual behavior. /switch channel now shows a redirect message pointing users to the new command. Addresses review feedback from yinwm on PR #959. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
2026-06-12 18:08:54 +00:00 · 2026-03-06 17:31:40 +08:00
parent c3c293297d
commit b716b8a053
36 changed files with 2428 additions and 353 deletions
@@ -21,6 +21,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"
@@ -46,6 +47,7 @@ type AgentLoop struct {
 	channelManager *channels.Manager
 	mediaStore     media.MediaStore
 	transcriber    voice.Transcriber
+	cmdRegistry    *commands.Registry
 }

 // processOptions configures how a message is processed
@@ -61,7 +63,15 @@ 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."
+const (
+	defaultResponse           = "I've completed processing but have no response to give. Increase `max_tool_iterations` in config.json."
+	sessionKeyAgentPrefix     = "agent:"
+	metadataKeyAccountID      = "account_id"
+	metadataKeyGuildID        = "guild_id"
+	metadataKeyTeamID         = "team_id"
+	metadataKeyParentPeerKind = "parent_peer_kind"
+	metadataKeyParentPeerID   = "parent_peer_id"
+)

 func NewAgentLoop(
 	cfg *config.Config,
@@ -84,14 +94,17 @@ func NewAgentLoop(
 		stateManager = state.NewManager(defaultAgent.Workspace)
 	}

-	return &AgentLoop{
+	al := &AgentLoop{
 		bus:         msgBus,
 		cfg:         cfg,
 		registry:    registry,
 		state:       stateManager,
 		summarizing: sync.Map{},
 		fallback:    fallbackChain,
+		cmdRegistry: commands.NewRegistry(commands.BuiltinDefinitions()),
 	}
+
+	return al
 }

 // registerSharedTools registers tools that are shared across all agents (web, message, spawn).
@@ -549,27 +562,18 @@ func (al *AgentLoop) processMessage(ctx context.Context, msg bus.InboundMessage)
 		return al.processSystemMessage(ctx, msg)
 	}

-	// Check for commands
-	if response, handled := al.handleCommand(ctx, msg); handled {
+	route, agent, routeErr := al.resolveMessageRoute(msg)
+
+	// Commands are checked before requiring a successful route.
+	// Global commands (/help, /show, /switch) work even when routing fails;
+	// context-dependent commands check their own Runtime fields and report
+	// "unavailable" when the required capability is nil.
+	if response, handled := al.handleCommand(ctx, msg, agent); handled {
 		return response, nil
 	}

-	// Route to determine agent and session key
-	route := al.registry.ResolveRoute(routing.RouteInput{
-		Channel:    msg.Channel,
-		AccountID:  msg.Metadata["account_id"],
-		Peer:       extractPeer(msg),
-		ParentPeer: extractParentPeer(msg),
-		GuildID:    msg.Metadata["guild_id"],
-		TeamID:     msg.Metadata["team_id"],
-	})
-
-	agent, ok := al.registry.GetAgent(route.AgentID)
-	if !ok {
-		agent = al.registry.GetDefaultAgent()
-	}
-	if agent == nil {
-		return "", fmt.Errorf("no agent available for route (agent_id=%s)", route.AgentID)
+	if routeErr != nil {
+		return "", routeErr
 	}

 	// Reset message-tool state for this round so we don't skip publishing due to a previous round.
@@ -579,17 +583,18 @@ func (al *AgentLoop) processMessage(ctx context.Context, msg bus.InboundMessage)
 		}
 	}

-	// Use routed session key, but honor pre-set agent-scoped keys (for ProcessDirect/cron)
-	sessionKey := route.SessionKey
-	if msg.SessionKey != "" && strings.HasPrefix(msg.SessionKey, "agent:") {
-		sessionKey = msg.SessionKey
-	}
+	// Resolve session key from route, while preserving explicit agent-scoped keys.
+	scopeKey := resolveScopeKey(route, msg.SessionKey)
+	sessionKey := scopeKey

 	logger.InfoCF("agent", "Routed message",
 		map[string]any{
-			"agent_id":    agent.ID,
-			"session_key": sessionKey,
-			"matched_by":  route.MatchedBy,
+			"agent_id":      agent.ID,
+			"scope_key":     scopeKey,
+			"session_key":   sessionKey,
+			"matched_by":    route.MatchedBy,
+			"route_agent":   route.AgentID,
+			"route_channel": route.Channel,
 		})

 	return al.runAgentLoop(ctx, agent, processOptions{
@@ -604,6 +609,34 @@ 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),
+		Peer:       extractPeer(msg),
+		ParentPeer: extractParentPeer(msg),
+		GuildID:    inboundMetadata(msg, metadataKeyGuildID),
+		TeamID:     inboundMetadata(msg, metadataKeyTeamID),
+	})
+
+	agent, ok := al.registry.GetAgent(route.AgentID)
+	if !ok {
+		agent = al.registry.GetDefaultAgent()
+	}
+	if agent == nil {
+		return routing.ResolvedRoute{}, nil, fmt.Errorf("no agent available for route (agent_id=%s)", route.AgentID)
+	}
+
+	return route, agent, nil
+}
+
+func resolveScopeKey(route routing.ResolvedRoute, msgSessionKey string) string {
+	if msgSessionKey != "" && strings.HasPrefix(msgSessionKey, sessionKeyAgentPrefix) {
+		return msgSessionKey
+	}
+	return route.SessionKey
+}
+
 func (al *AgentLoop) processSystemMessage(
 	ctx context.Context,
 	msg bus.InboundMessage,
@@ -1504,94 +1537,87 @@ func (al *AgentLoop) estimateTokens(messages []providers.Message) int {
 	return totalChars * 2 / 5
 }

-func (al *AgentLoop) handleCommand(ctx context.Context, msg bus.InboundMessage) (string, bool) {
-	content := strings.TrimSpace(msg.Content)
-	if !strings.HasPrefix(content, "/") {
+func (al *AgentLoop) handleCommand(
+	ctx context.Context,
+	msg bus.InboundMessage,
+	agent *AgentInstance,
+) (string, bool) {
+	if !commands.HasCommandPrefix(msg.Content) {
 		return "", false
 	}

-	parts := strings.Fields(content)
-	if len(parts) == 0 {
+	if al.cmdRegistry == nil {
 		return "", false
 	}

-	cmd := parts[0]
-	args := parts[1:]
+	rt := al.buildCommandsRuntime(agent)
+	executor := commands.NewExecutor(al.cmdRegistry, rt)

-	switch cmd {
-	case "/show":
-		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
-		}
+	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
+		},
+	})

-	case "/list":
-		if len(args) < 1 {
-			return "Usage: /list [models|channels|agents]", true
+	switch result.Outcome {
+	case commands.OutcomeHandled:
+		if result.Err != nil {
+			return mapCommandError(result), true
 		}
-		switch args[0] {
-		case "models":
-			return "Available models: configured in config.json per agent", true
-		case "channels":
+		if commandReply != "" {
+			return commandReply, true
+		}
+		return "", true
+	default: // OutcomePassthrough — let the message fall through to LLM
+		return "", false
+	}
+}
+
+func (al *AgentLoop) buildCommandsRuntime(agent *AgentInstance) *commands.Runtime {
+	rt := &commands.Runtime{
+		Config:          al.cfg,
+		ListAgentIDs:    al.registry.ListAgentIDs,
+		ListDefinitions: al.cmdRegistry.Definitions,
+		GetEnabledChannels: func() []string {
 			if al.channelManager == nil {
-				return "Channel manager not initialized", true
+				return nil
 			}
-			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 "/switch":
-		if len(args) < 3 || args[1] != "to" {
-			return "Usage: /switch [model|channel] to <name>", true
-		}
-		target := args[0]
-		value := args[2]
-
-		switch target {
-		case "model":
-			defaultAgent := al.registry.GetDefaultAgent()
-			if defaultAgent == nil {
-				return "No default agent configured", true
-			}
-			oldModel := defaultAgent.Model
-			defaultAgent.Model = value
-			return fmt.Sprintf("Switched model from %s to %s", oldModel, value), true
-		case "channel":
+			return al.channelManager.GetEnabledChannels()
+		},
+		SwitchChannel: func(value string) error {
 			if al.channelManager == nil {
-				return "Channel manager not initialized", true
+				return fmt.Errorf("channel manager not initialized")
 			}
 			if _, exists := al.channelManager.GetChannel(value); !exists && value != "cli" {
-				return fmt.Sprintf("Channel '%s' not found or not enabled", value), true
+				return fmt.Errorf("channel '%s' not found or not enabled", value)
 			}
-			return fmt.Sprintf("Switched target channel to %s", value), true
-		default:
-			return fmt.Sprintf("Unknown switch target: %s", target), true
+			return nil
+		},
+	}
+	if agent != nil {
+		rt.GetModelInfo = func() (string, string) {
+			return agent.Model, al.cfg.Agents.Defaults.Provider
+		}
+		rt.SwitchModel = func(value string) (string, error) {
+			oldModel := agent.Model
+			agent.Model = value
+			return oldModel, nil
 		}
 	}
+	return rt
+}

-	return "", false
+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.
@@ -1610,10 +1636,17 @@ 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 := msg.Metadata["parent_peer_kind"]
-	parentID := msg.Metadata["parent_peer_id"]
+	parentKind := inboundMetadata(msg, metadataKeyParentPeerKind)
+	parentID := inboundMetadata(msg, metadataKeyParentPeerID)
 	if parentKind == "" || parentID == "" {
 		return nil
 	}