feat(commands): centralized command registry with sub-command routing by mingmxren · Pull Request #959 · sipeed/picoclaw

mingmxren · 2026-03-01T14:51:44Z

Description

Introduces a centralized pkg/commands package that serves as the single source of truth for all slash command metadata, parsing, and dispatch. Replaces scattered inline command handling across loop.go and channel adapters with a unified architecture.

Architecture

+=====================================================================+
|                           pkg/commands                              |
|                                                                     |
|  +--------------+    +---------------+    +-----------------------+ |
|  | Definition   |    | Registry      |    | Executor              | |
|  |              |    |               |    |                       | |
|  | Name         |--->| index: map    |--->| Execute(req)          | |
|  | SubCommands  |    | [name]->def   |    |   |- parseCommandName | |
|  | Handler      |    |               |    |   |- Lookup O(1)      | |
|  |              |    | Lookup()      |    |   '- sub-cmd routing  | |
|  | Effective    |    | Definitions() |    |                       | |
|  | Usage()      |    +---------------+    | Reply nil guard       | |
|  +--------------+                         | (centralized)         | |
|                                           +-----------------------+ |
|  +----------------------------------------------------------------+ |
|  | Command Groups (stateless definitions)                         | |
|  |                                                                | |
|  | cmd_start.go       /start                                      | |
|  | cmd_help.go        /help            (auto-generated usage)     | |
|  | cmd_show.go        /show [model|channel|agents]                | |
|  | cmd_list.go        /list [models|channels|agents]              | |
|  | cmd_switch.go      /switch [model to <name>|channel to <name>] | |
|  | handler_agents.go  shared agentsHandler (dedup)                | |
|  +----------------------------------------------------------------+ |
|                                                                     |
|  +-------------+  +------------+  +-------------------------------+ |
|  | Runtime     |  | request.go |  | Request / ExecuteResult       | |
|  | (per-req)   |  |            |  |                               | |
|  |             |  | HasCommand |  | Channel, ChatID, SenderID     | |
|  | GetModel    |  |  Prefix()  |  | Text, Reply callback          | |
|  | ListAgent   |  | nthToken() |  |                               | |
|  | ListDefs    |  | parseCmd() |  | Outcome: Handled/Passthrough  | |
|  | GetChan     |  +------------+  +-------------------------------+ |
|  | SwitchMod   |                                                     |
|  | SwitchCh    |                                                     |
|  +-------------+                                                     |
+=====================================================================+
        |                                       |
        | func fields                           | []Definition
        | built per-request                     | (stateless)
        v                                       v
+----------------------+             +--------------------------+
| pkg/agent            |             | pkg/channels/telegram    |
|                      |             |                          |
| AgentLoop            |             | startCommandRegistration |
| |- cmdRegistry       |             | (async + backoff)        |
| |  (cached, once)    |             |                          |
| |- handleCommand()   |             | Derives Telegram         |
| |  HasCommandPrefix  |             | BotCommand menu from     |
| |  (supports / !)    |             | Definition metadata      |
| |- buildCommandsRuntime()          +--------------------------+
| |  (per-request)     |
| '- NewExecutor       |
|    (registry,runtime)|
+----------------------+


Data Flow (processMessage)
==========================

  User message ("/show model" or "!switch model to gpt-4")
       |
       v
  resolveMessageRoute()  -->  (route, agent, routeErr)
       |
       v
  handleCommand()  <-- runs BEFORE routeErr check
       |                so /help, /show, /switch work even when routing fails
       |
       |- commands.HasCommandPrefix()  -->  supports "/" and "!"
       |- buildCommandsRuntime()       -->  creates Runtime with current state
       |
       v
  NewExecutor(registry, runtime).Execute()
       |
       |- parseCommandName()   -->  normalize, strip @bot suffix
       |- Registry.Lookup()    -->  O(1) map lookup
       '- executeDefinition()
            |
            |- nil Reply guard (centralized)
            |- simple cmd  ->  Handler(ctx, req, rt)
            '- sub-cmd     ->  match SubCommand.Name -> Handler(ctx, req, rt)
                                    |
                                    v
                          Runtime callbacks
                          e.g. rt.SwitchModel()
       |
       v
  routeErr != nil?  -->  return error (non-command messages need routing)
       |
       v
  runAgentLoop(agent, ...)

Key Design Decisions

Registry / Runtime separation: Registry holds stateless command metadata (cached once on AgentLoop). Runtime carries per-request dependencies (built fresh each handleCommand call). This cleanly separates "what commands exist" from "what resources they need".
Per-request Executor: NewExecutor(registry, runtime) is created per-request, binding the stateless Registry with the current Runtime. This enables future session management to inject per-request context (e.g. session scope) without modifying the registry.
Handler signature func(ctx, req, rt) error: Handlers receive Runtime as an explicit parameter instead of closing over deps. This makes dependencies visible, testable, and per-request.
SubCommand pattern: Sub-commands are declared structurally within Definition. EffectiveUsage() auto-generates help text from sub-command names, preventing metadata/handler drift.
Centralized nil guard: req.Reply nil check is done once in executeDefinition, not in every handler.
Commands before route gate: handleCommand() runs before checking routeErr, so global commands (/help, /show, /switch) remain available even when routing fails. Context-dependent commands that need a routed agent report "unavailable" via their nil-Runtime guards.

Changes

New package: pkg/commands

definition.go — Definition, SubCommand, EffectiveUsage() auto-generation
registry.go — O(1) map-based command lookup with alias support
executor.go — Two-outcome dispatch (Handled/Passthrough) with sub-command routing and Reply error propagation
request.go — Request/Handler types, shared parsing: HasCommandPrefix(), nthToken(), parseCommandName()
runtime.go — Per-request runtime dependency struct (ListDefinitions, SwitchModel, SwitchChannel, etc.)
cmd_*.go — Per-group command definitions (start, help, show, list, switch)
handler_agents.go — Shared agentsHandler used by both /show agents and /list agents
builtin.go — Aggregates all built-in definitions (stateless, no parameters)

Modified: pkg/agent/loop.go

Cache cmdRegistry as AgentLoop field (created once in struct init)
buildCommandsRuntime() constructs per-request Runtime; wires ListDefinitions from registry
handleCommand() runs before route error check — global commands work even when routing fails
modelMu mutex protects AgentInstance.Model writes in SwitchModel callback

Modified: pkg/channels/telegram

Async startCommandRegistration with exponential backoff replaces blocking initBotCommands
Derives Telegram BotCommand menu from BuiltinDefinitions() metadata

New: pkg/channels/interfaces.go

CommandRegistrarCapable interface for platform menu registration

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Code refactoring (no functional changes, no api changes)

AI Code Generation

Mostly AI-generated (AI draft, Human verified/modified)

Related Issue

[Feature] Unify slash commands across all channels #570 [Feature] Unify slash commands across all channels
[BUG] Telegram /help command references may be incomplete #579 [BUG] Telegram /help command references may be incomplete

Technical Context

Reference: pkg/commands/definition.go, pkg/commands/registry.go, pkg/commands/executor.go, pkg/commands/runtime.go
Reasoning: Keep command metadata, dispatch behavior, and platform registration synchronized from one canonical source. SubCommand pattern prevents help text / handler drift. Registry/Runtime separation enables future session management without architectural changes.

Test Environment

Hardware: Mac mini
OS: macOS 26.3
Channels: Telegram, WhatsApp

Checklist

My code/docs follow the style of this project.
I have performed a self-review of my own changes.
I have updated the documentation accordingly.

pkg/commands/builtin_test.go

pkg/commands/registry.go

pkg/channels/telegram/command_registration.go

pkg/commands/dispatcher.go

mingmxren

Addressed remaining review comments.

pkg/agent/loop.go

cmd/picoclaw/internal/onboard/helpers.go

pkg/commands/executor.go

… and registration

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>

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>

…ontains() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

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>

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>

…allbacks 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>

…andRegistration only Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

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

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

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

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>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…mplates 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>

- Move cmdRegistry init into struct literal (review comment sipeed#11) - Rename buildRuntime → buildCommandsRuntime for clarity (review comment sipeed#12) - Add comment to default switch case explaining passthrough (review comment sipeed#13) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…tness - 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>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

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>

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>

…mand 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>

CLAassistant · 2026-03-05T14:11:44Z

All committers have signed the CLA.

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>

yinwm · 2026-03-06T08:31:40Z

`/switch channel` Semantic Issue

Noticed that `/switch channel` only validates if a channel exists, but doesn't actually switch anything. This is a historical issue (not introduced by this PR).
PR has already made the message more honest (from "Switched target channel to X" to "Channel 'X' is available and enabled"), which is a good improvement.
However, the command name `/switch` is still misleading: users see "switch" and expect a state change, but it only validates existence.

Suggestion

This is a historical issue, not introduced by this PR. Consider creating a follow-up issue to discuss:

Is actual "switch" functionality needed?
If not, consider renaming to `/check channel` or `/verify channel` (possibly keeping `/switch` as an alias)
If needed, implement actual switch logic
Or at least clarify in help docs that this only validates availability, not actual switching
This is a minor issue that doesn't block merging this valuable architectural refactoring PR.

/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 sipeed#959. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

mingmxren · 2026-03-06T08:59:15Z

/switch channel Semantic Issue

Noticed that /switch channel only validates if a channel exists, but doesn't actually switch anything. This is a historical issue (not introduced by this PR). PR has already made the message more honest (from "Switched target channel to X" to "Channel 'X' is available and enabled"), which is a good improvement. However, the command name /switch is still misleading: users see "switch" and expect a state change, but it only validates existence.

Suggestion

This is a historical issue, not introduced by this PR. Consider creating a follow-up issue to discuss:
* Is actual "switch" functionality needed?

* If not, consider renaming to `/check channel` or `/verify channel` (possibly keeping `/switch` as an alias)

* If needed, implement actual switch logic

* Or at least clarify in help docs that this only validates availability, not actual switching
  This is a minor issue that doesn't block merging this valuable architectural refactoring PR.

@yinwm
Solved by a963826

yinwm

LGTM

This was referenced Mar 1, 2026

feat(session): Session management [Phase 2/2] session lifecycle and scope management #960

Draft

feat(commands): Session management [Phase 3/3] command execution centralization #961

Closed

mingmxren changed the title ~~feat(commands): cross-channel command registry~~ [Phase 1/3] feat(commands): cross-channel command registry Mar 1, 2026

mingmxren changed the title ~~[Phase 1/3] feat(commands): cross-channel command registry~~ feat(commands): Session management [Phase 1/3] cross-channel command registry Mar 1, 2026

github-actions bot mentioned this pull request Mar 1, 2026

🦞 OpenClaw 生态日报 2026-03-01 rollysys/agents-radar#18

Open

sipeed-bot bot added type: enhancement New feature or request domain: channel labels Mar 3, 2026

mingmxren force-pushed the codex/pr-split-1-command-registry branch 3 times, most recently from 824d7a1 to 236ed47 Compare March 3, 2026 08:11

Zhaoyikaiii reviewed Mar 3, 2026

View reviewed changes

mingmxren force-pushed the codex/pr-split-1-command-registry branch from 236ed47 to 554aa40 Compare March 3, 2026 08:39

mingmxren changed the title ~~feat(commands): Session management [Phase 1/3] cross-channel command registry~~ feat(commands): Session management [Phase 1/2] command execution + session core Mar 3, 2026

mingmxren changed the title ~~feat(commands): Session management [Phase 1/2] command execution + session core~~ feat(commands): Session management [Phase 1/2] command centralization and registration Mar 3, 2026

mingmxren force-pushed the codex/pr-split-1-command-registry branch 2 times, most recently from 351b3d1 to ffc063e Compare March 3, 2026 13:19

mingmxren changed the title ~~feat(commands): Session management [Phase 1/2] command centralization and registration~~ feat(commands): centralized command registry with sub-command routing Mar 3, 2026

mingmxren commented Mar 3, 2026

View reviewed changes

pkg/agent/loop.go Outdated Show resolved Hide resolved

pkg/agent/loop.go Outdated Show resolved Hide resolved

pkg/agent/loop.go Outdated Show resolved Hide resolved

github-actions bot mentioned this pull request Mar 4, 2026

🦞 OpenClaw 生态日报 2026-03-04 duanyytop/agents-radar#71

Open

mingmxren commented Mar 4, 2026

View reviewed changes

cmd/picoclaw/internal/onboard/helpers.go Outdated Show resolved Hide resolved

mingmxren requested a review from Zhaoyikaiii March 4, 2026 10:17

Zhaoyikaiii reviewed Mar 4, 2026

View reviewed changes

pkg/commands/executor.go Outdated Show resolved Hide resolved

mingmxren and others added 8 commits March 5, 2026 14:45

feat(commands): Session management [Phase 1/2] command centralization…

0d5d1a8

… and registration

feat(commands): add Deps struct and secondToken helper, remove dead c…

c940133

…ontains() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

fix(telegram): remove duplicate initBotCommands, keep async startComm…

77a0957

…andRegistration only Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

mingmxren and others added 12 commits March 5, 2026 14:45

chore(commands): restore Outcome comments and annotate Deps.Config

3f388a0

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

chore: add docs/plans to .gitignore and untrack existing files

ef86148

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

style: fix gci import grouping and godoc formatting

aead637

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

refactor(onboard): extract legacy filename to constant

9e6c2b2

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

mingmxren force-pushed the codex/pr-split-1-command-registry branch from 1d47d23 to 92ed0c5 Compare March 5, 2026 06:47

fix: executeDefinition Unknown option

c87533e

mingmxren force-pushed the codex/pr-split-1-command-registry branch from 92ed0c5 to c87533e Compare March 5, 2026 07:10

mingmxren requested a review from Zhaoyikaiii March 5, 2026 14:23

yinwm approved these changes Mar 6, 2026

View reviewed changes

yinwm merged commit b716b8a into sipeed:main Mar 6, 2026
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

feat(commands): centralized command registry with sub-command routing#959
yinwm merged 24 commits intosipeed:mainfrom
mingmxren:codex/pr-split-1-command-registry

mingmxren commented Mar 1, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mingmxren left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CLAassistant commented Mar 5, 2026 •

edited

Loading

Uh oh!

yinwm commented Mar 6, 2026

Uh oh!

mingmxren commented Mar 6, 2026

`/switch channel` Semantic Issue

Suggestion

Uh oh!

yinwm left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

mingmxren commented Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Architecture

Key Design Decisions

Changes

Type of Change

AI Code Generation

Related Issue

Technical Context

Test Environment

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mingmxren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CLAassistant commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yinwm commented Mar 6, 2026

`/switch channel` Semantic Issue

Suggestion

Uh oh!

mingmxren commented Mar 6, 2026

/switch channel Semantic Issue

Suggestion

Uh oh!

yinwm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mingmxren commented Mar 1, 2026 •

edited

Loading

CLAassistant commented Mar 5, 2026 •

edited

Loading

`/switch channel` Semantic Issue