mirror of
https://github.com/sipeed/picoclaw.git
synced 2026-06-12 18:08:54 +00:00
b716b8a053
* 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>
280 lines
6.7 KiB
Go
280 lines
6.7 KiB
Go
package commands
|
|
|
|
import (
|
|
"context"
|
|
"fmt"
|
|
"testing"
|
|
)
|
|
|
|
func TestSwitchModel_Success(t *testing.T) {
|
|
rt := &Runtime{
|
|
SwitchModel: func(value string) (string, error) {
|
|
return "old-model", nil
|
|
},
|
|
}
|
|
ex := NewExecutor(NewRegistry(BuiltinDefinitions()), rt)
|
|
|
|
var reply string
|
|
res := ex.Execute(context.Background(), Request{
|
|
Text: "/switch model to gpt-4",
|
|
Reply: func(text string) error {
|
|
reply = text
|
|
return nil
|
|
},
|
|
})
|
|
if res.Outcome != OutcomeHandled {
|
|
t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled)
|
|
}
|
|
want := "Switched model from old-model to gpt-4"
|
|
if reply != want {
|
|
t.Fatalf("reply=%q, want=%q", reply, want)
|
|
}
|
|
}
|
|
|
|
func TestSwitchModel_MissingToKeyword(t *testing.T) {
|
|
rt := &Runtime{
|
|
SwitchModel: func(value string) (string, error) {
|
|
return "old", nil
|
|
},
|
|
}
|
|
ex := NewExecutor(NewRegistry(BuiltinDefinitions()), rt)
|
|
|
|
var reply string
|
|
res := ex.Execute(context.Background(), Request{
|
|
Text: "/switch model gpt-4",
|
|
Reply: func(text string) error {
|
|
reply = text
|
|
return nil
|
|
},
|
|
})
|
|
if res.Outcome != OutcomeHandled {
|
|
t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled)
|
|
}
|
|
if reply != "Usage: /switch model to <name>" {
|
|
t.Fatalf("reply=%q, want usage message", reply)
|
|
}
|
|
}
|
|
|
|
func TestSwitchModel_MissingValue(t *testing.T) {
|
|
rt := &Runtime{
|
|
SwitchModel: func(value string) (string, error) {
|
|
return "old", nil
|
|
},
|
|
}
|
|
ex := NewExecutor(NewRegistry(BuiltinDefinitions()), rt)
|
|
|
|
var reply string
|
|
res := ex.Execute(context.Background(), Request{
|
|
Text: "/switch model to",
|
|
Reply: func(text string) error {
|
|
reply = text
|
|
return nil
|
|
},
|
|
})
|
|
if res.Outcome != OutcomeHandled {
|
|
t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled)
|
|
}
|
|
if reply != "Usage: /switch model to <name>" {
|
|
t.Fatalf("reply=%q, want usage message", reply)
|
|
}
|
|
}
|
|
|
|
func TestSwitchModel_Error(t *testing.T) {
|
|
rt := &Runtime{
|
|
SwitchModel: func(value string) (string, error) {
|
|
return "", fmt.Errorf("model not found")
|
|
},
|
|
}
|
|
ex := NewExecutor(NewRegistry(BuiltinDefinitions()), rt)
|
|
|
|
var reply string
|
|
res := ex.Execute(context.Background(), Request{
|
|
Text: "/switch model to bad-model",
|
|
Reply: func(text string) error {
|
|
reply = text
|
|
return nil
|
|
},
|
|
})
|
|
if res.Outcome != OutcomeHandled {
|
|
t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled)
|
|
}
|
|
if reply != "model not found" {
|
|
t.Fatalf("reply=%q, want error message", reply)
|
|
}
|
|
}
|
|
|
|
func TestSwitchModel_NilDep(t *testing.T) {
|
|
ex := NewExecutor(NewRegistry(BuiltinDefinitions()), &Runtime{})
|
|
|
|
var reply string
|
|
res := ex.Execute(context.Background(), Request{
|
|
Text: "/switch model to gpt-4",
|
|
Reply: func(text string) error {
|
|
reply = text
|
|
return nil
|
|
},
|
|
})
|
|
if res.Outcome != OutcomeHandled {
|
|
t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled)
|
|
}
|
|
if reply != "Command unavailable in current context." {
|
|
t.Fatalf("reply=%q, want unavailable message", reply)
|
|
}
|
|
}
|
|
|
|
func TestSwitchChannel_Redirect(t *testing.T) {
|
|
ex := NewExecutor(NewRegistry(BuiltinDefinitions()), &Runtime{})
|
|
|
|
var reply string
|
|
res := ex.Execute(context.Background(), Request{
|
|
Text: "/switch channel to telegram",
|
|
Reply: func(text string) error {
|
|
reply = text
|
|
return nil
|
|
},
|
|
})
|
|
if res.Outcome != OutcomeHandled {
|
|
t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled)
|
|
}
|
|
want := "This command has moved. Please use: /check channel <name>"
|
|
if reply != want {
|
|
t.Fatalf("reply=%q, want=%q", reply, want)
|
|
}
|
|
}
|
|
|
|
func TestCheckChannel_Success(t *testing.T) {
|
|
rt := &Runtime{
|
|
SwitchChannel: func(value string) error {
|
|
return nil
|
|
},
|
|
}
|
|
ex := NewExecutor(NewRegistry(BuiltinDefinitions()), rt)
|
|
|
|
var reply string
|
|
res := ex.Execute(context.Background(), Request{
|
|
Text: "/check channel telegram",
|
|
Reply: func(text string) error {
|
|
reply = text
|
|
return nil
|
|
},
|
|
})
|
|
if res.Outcome != OutcomeHandled {
|
|
t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled)
|
|
}
|
|
want := "Channel 'telegram' is available and enabled"
|
|
if reply != want {
|
|
t.Fatalf("reply=%q, want=%q", reply, want)
|
|
}
|
|
}
|
|
|
|
func TestCheckChannel_Error(t *testing.T) {
|
|
rt := &Runtime{
|
|
SwitchChannel: func(value string) error {
|
|
return fmt.Errorf("channel '%s' not found", value)
|
|
},
|
|
}
|
|
ex := NewExecutor(NewRegistry(BuiltinDefinitions()), rt)
|
|
|
|
var reply string
|
|
res := ex.Execute(context.Background(), Request{
|
|
Text: "/check channel unknown",
|
|
Reply: func(text string) error {
|
|
reply = text
|
|
return nil
|
|
},
|
|
})
|
|
if res.Outcome != OutcomeHandled {
|
|
t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled)
|
|
}
|
|
if reply != "channel 'unknown' not found" {
|
|
t.Fatalf("reply=%q, want error message", reply)
|
|
}
|
|
}
|
|
|
|
func TestCheckChannel_NilDep(t *testing.T) {
|
|
ex := NewExecutor(NewRegistry(BuiltinDefinitions()), &Runtime{})
|
|
|
|
var reply string
|
|
res := ex.Execute(context.Background(), Request{
|
|
Text: "/check channel telegram",
|
|
Reply: func(text string) error {
|
|
reply = text
|
|
return nil
|
|
},
|
|
})
|
|
if res.Outcome != OutcomeHandled {
|
|
t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled)
|
|
}
|
|
if reply != "Command unavailable in current context." {
|
|
t.Fatalf("reply=%q, want unavailable message", reply)
|
|
}
|
|
}
|
|
|
|
func TestCheckChannel_MissingValue(t *testing.T) {
|
|
rt := &Runtime{
|
|
SwitchChannel: func(value string) error {
|
|
return nil
|
|
},
|
|
}
|
|
ex := NewExecutor(NewRegistry(BuiltinDefinitions()), rt)
|
|
|
|
var reply string
|
|
res := ex.Execute(context.Background(), Request{
|
|
Text: "/check channel",
|
|
Reply: func(text string) error {
|
|
reply = text
|
|
return nil
|
|
},
|
|
})
|
|
if res.Outcome != OutcomeHandled {
|
|
t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled)
|
|
}
|
|
if reply != "Usage: /check channel <name>" {
|
|
t.Fatalf("reply=%q, want usage message", reply)
|
|
}
|
|
}
|
|
|
|
func TestSwitch_BangPrefix(t *testing.T) {
|
|
rt := &Runtime{
|
|
SwitchModel: func(value string) (string, error) {
|
|
return "old", nil
|
|
},
|
|
}
|
|
ex := NewExecutor(NewRegistry(BuiltinDefinitions()), rt)
|
|
|
|
var reply string
|
|
res := ex.Execute(context.Background(), Request{
|
|
Text: "!switch model to gpt-4",
|
|
Reply: func(text string) error {
|
|
reply = text
|
|
return nil
|
|
},
|
|
})
|
|
if res.Outcome != OutcomeHandled {
|
|
t.Fatalf("! prefix: outcome=%v, want=%v", res.Outcome, OutcomeHandled)
|
|
}
|
|
if reply != "Switched model from old to gpt-4" {
|
|
t.Fatalf("! prefix: reply=%q, want success message", reply)
|
|
}
|
|
}
|
|
|
|
func TestSwitch_NoSubCommand(t *testing.T) {
|
|
ex := NewExecutor(NewRegistry(BuiltinDefinitions()), &Runtime{})
|
|
|
|
var reply string
|
|
res := ex.Execute(context.Background(), Request{
|
|
Text: "/switch",
|
|
Reply: func(text string) error {
|
|
reply = text
|
|
return nil
|
|
},
|
|
})
|
|
if res.Outcome != OutcomeHandled {
|
|
t.Fatalf("outcome=%v, want=%v", res.Outcome, OutcomeHandled)
|
|
}
|
|
// Should get usage message from executor's sub-command routing
|
|
if reply == "" {
|
|
t.Fatal("expected usage reply for bare /switch")
|
|
}
|
|
}
|