Files
picoclaw/pkg/commands/builtin_test.go
T
Ming b716b8a053 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-03-06 17:31:40 +08:00

146 lines
3.7 KiB
Go

package commands
import (
"context"
"strings"
"testing"
)
func findDefinitionByName(t *testing.T, defs []Definition, name string) Definition {
t.Helper()
for _, def := range defs {
if def.Name == name {
return def
}
}
t.Fatalf("missing /%s definition", name)
return Definition{}
}
func TestBuiltinHelpHandler_ReturnsFormattedMessage(t *testing.T) {
defs := BuiltinDefinitions()
helpDef := findDefinitionByName(t, defs, "help")
if helpDef.Handler == nil {
t.Fatalf("/help handler should not be nil")
}
var reply string
err := helpDef.Handler(context.Background(), Request{
Text: "/help",
Reply: func(text string) error {
reply = text
return nil
},
}, nil)
if err != nil {
t.Fatalf("/help handler error: %v", err)
}
// Now uses auto-generated EffectiveUsage which includes agents
if !strings.Contains(reply, "/show [model|channel|agents]") {
t.Fatalf("/help reply missing /show usage, got %q", reply)
}
if !strings.Contains(reply, "/list [models|channels|agents]") {
t.Fatalf("/help reply missing /list usage, got %q", reply)
}
}
func TestBuiltinShowChannel_PreservesUserVisibleBehavior(t *testing.T) {
defs := BuiltinDefinitions()
ex := NewExecutor(NewRegistry(defs), nil)
cases := []string{"telegram", "whatsapp"}
for _, channel := range cases {
var reply string
res := ex.Execute(context.Background(), Request{
Channel: channel,
Text: "/show channel",
Reply: func(text string) error {
reply = text
return nil
},
})
if res.Outcome != OutcomeHandled {
t.Fatalf("/show channel on %s: outcome=%v, want=%v", channel, res.Outcome, OutcomeHandled)
}
want := "Current Channel: " + channel
if reply != want {
t.Fatalf("/show channel reply=%q, want=%q", reply, want)
}
}
}
func TestBuiltinListChannels_UsesGetEnabledChannels(t *testing.T) {
rt := &Runtime{
GetEnabledChannels: func() []string {
return []string{"telegram", "slack"}
},
}
defs := BuiltinDefinitions()
ex := NewExecutor(NewRegistry(defs), rt)
var reply string
res := ex.Execute(context.Background(), Request{
Text: "/list channels",
Reply: func(text string) error {
reply = text
return nil
},
})
if res.Outcome != OutcomeHandled {
t.Fatalf("/list channels: outcome=%v, want=%v", res.Outcome, OutcomeHandled)
}
if !strings.Contains(reply, "telegram") || !strings.Contains(reply, "slack") {
t.Fatalf("/list channels reply=%q, want telegram and slack", reply)
}
}
func TestBuiltinShowAgents_RestoresOldBehavior(t *testing.T) {
rt := &Runtime{
ListAgentIDs: func() []string {
return []string{"default", "coder"}
},
}
defs := BuiltinDefinitions()
ex := NewExecutor(NewRegistry(defs), rt)
var reply string
res := ex.Execute(context.Background(), Request{
Text: "/show agents",
Reply: func(text string) error {
reply = text
return nil
},
})
if res.Outcome != OutcomeHandled {
t.Fatalf("/show agents: outcome=%v, want=%v", res.Outcome, OutcomeHandled)
}
if !strings.Contains(reply, "default") || !strings.Contains(reply, "coder") {
t.Fatalf("/show agents reply=%q, want agent IDs", reply)
}
}
func TestBuiltinListAgents_RestoresOldBehavior(t *testing.T) {
rt := &Runtime{
ListAgentIDs: func() []string {
return []string{"default", "coder"}
},
}
defs := BuiltinDefinitions()
ex := NewExecutor(NewRegistry(defs), rt)
var reply string
res := ex.Execute(context.Background(), Request{
Text: "/list agents",
Reply: func(text string) error {
reply = text
return nil
},
})
if res.Outcome != OutcomeHandled {
t.Fatalf("/list agents: outcome=%v, want=%v", res.Outcome, OutcomeHandled)
}
if !strings.Contains(reply, "default") || !strings.Contains(reply, "coder") {
t.Fatalf("/list agents reply=%q, want agent IDs", reply)
}
}