refactor(cli): address review feedback and improve command clarity

This commit is contained in:
Ruslan Semagin 2026-02-24 14:11:10 +03:00
parent e62b4e0ac5
commit 9a65c703f6
13 changed files with 55 additions and 43 deletions

View file

@ -17,11 +17,6 @@ func NewAgentCommand() *cobra.Command {
Short: "Interact with the agent directly", Short: "Interact with the agent directly",
Args: cobra.NoArgs, Args: cobra.NoArgs,
RunE: func(cmd *cobra.Command, _ []string) error { RunE: func(cmd *cobra.Command, _ []string) error {
debug, _ = cmd.Flags().GetBool("debug")
message, _ = cmd.Flags().GetString("message")
sessionKey, _ = cmd.Flags().GetString("session")
model, _ = cmd.Flags().GetString("model")
return agentCmd(message, sessionKey, model, debug) return agentCmd(message, sessionKey, model, debug)
}, },
} }

View file

@ -24,12 +24,10 @@ func authLoginCmd(provider string, useDeviceCode bool) error {
case "anthropic": case "anthropic":
return authLoginPasteToken(provider) return authLoginPasteToken(provider)
case "google-antigravity", "antigravity": case "google-antigravity", "antigravity":
authLoginGoogleAntigravity() return authLoginGoogleAntigravity()
default: default:
return fmt.Errorf("unsupported provider: %s (%s)", provider, supportedProvidersMsg) return fmt.Errorf("unsupported provider: %s (%s)", provider, supportedProvidersMsg)
} }
return nil
} }
func authLoginOpenAI(useDeviceCode bool) error { func authLoginOpenAI(useDeviceCode bool) error {
@ -80,7 +78,7 @@ func authLoginOpenAI(useDeviceCode bool) error {
appCfg.Agents.Defaults.Model = "gpt-5.2" appCfg.Agents.Defaults.Model = "gpt-5.2"
if err = config.SaveConfig(internal.GetConfigPath(), appCfg); err != nil { if err = config.SaveConfig(internal.GetConfigPath(), appCfg); err != nil {
return fmt.Errorf("warning: could not update config: %w", err) return fmt.Errorf("could not update config: %w", err)
} }
} }
@ -93,13 +91,12 @@ func authLoginOpenAI(useDeviceCode bool) error {
return nil return nil
} }
func authLoginGoogleAntigravity() { func authLoginGoogleAntigravity() error {
cfg := auth.GoogleAntigravityOAuthConfig() cfg := auth.GoogleAntigravityOAuthConfig()
cred, err := auth.LoginBrowser(cfg) cred, err := auth.LoginBrowser(cfg)
if err != nil { if err != nil {
fmt.Printf("Login failed: %v\n", err) return fmt.Errorf("login failed: %w", err)
os.Exit(1)
} }
cred.Provider = "google-antigravity" cred.Provider = "google-antigravity"
@ -124,8 +121,7 @@ func authLoginGoogleAntigravity() {
} }
if err = auth.SetCredential("google-antigravity", cred); err != nil { if err = auth.SetCredential("google-antigravity", cred); err != nil {
fmt.Printf("Failed to save credentials: %v\n", err) return fmt.Errorf("failed to save credentials: %w", err)
os.Exit(1)
} }
appCfg, err := internal.LoadConfig() appCfg, err := internal.LoadConfig()
@ -163,6 +159,8 @@ func authLoginGoogleAntigravity() {
fmt.Println("\n✓ Google Antigravity login successful!") fmt.Println("\n✓ Google Antigravity login successful!")
fmt.Println("Default model set to: gemini-flash") fmt.Println("Default model set to: gemini-flash")
fmt.Println("Try it: picoclaw agent -m \"Hello world\"") fmt.Println("Try it: picoclaw agent -m \"Hello world\"")
return nil
} }
func fetchGoogleUserEmail(accessToken string) (string, error) { func fetchGoogleUserEmail(accessToken string) (string, error) {
@ -248,7 +246,7 @@ func authLoginPasteToken(provider string) error {
appCfg.Agents.Defaults.Model = "gpt-5.2" appCfg.Agents.Defaults.Model = "gpt-5.2"
} }
if err := config.SaveConfig(internal.GetConfigPath(), appCfg); err != nil { if err := config.SaveConfig(internal.GetConfigPath(), appCfg); err != nil {
return fmt.Errorf("warning: could not update config: %w", err) return fmt.Errorf("could not update config: %w", err)
} }
} }
@ -330,7 +328,9 @@ func authStatusCmd() error {
} }
if len(store.Credentials) == 0 { if len(store.Credentials) == 0 {
return fmt.Errorf("no authenticated providers. run: picoclaw auth login --provider <name>") fmt.Println("No authenticated providers.")
fmt.Println("Run: picoclaw auth login --provider <name>")
return nil
} }
fmt.Println("\nAuthenticated Providers:") fmt.Println("\nAuthenticated Providers:")

View file

@ -24,22 +24,10 @@ func newAddCommand(storePath func() string) *cobra.Command {
Short: "Add a new scheduled job", Short: "Add a new scheduled job",
Args: cobra.NoArgs, Args: cobra.NoArgs,
RunE: func(cmd *cobra.Command, _ []string) error { RunE: func(cmd *cobra.Command, _ []string) error {
if name == "" {
return fmt.Errorf("--name is required")
}
if message == "" {
return fmt.Errorf("--message is required")
}
if every <= 0 && cronExp == "" { if every <= 0 && cronExp == "" {
return fmt.Errorf("either --every or --cron must be specified") return fmt.Errorf("either --every or --cron must be specified")
} }
if every > 0 && cronExp != "" {
return fmt.Errorf("--every and --cron are mutually exclusive")
}
var schedule cron.CronSchedule var schedule cron.CronSchedule
if every > 0 { if every > 0 {
everyMS := every * 1000 everyMS := every * 1000
@ -70,6 +58,7 @@ func newAddCommand(storePath func() string) *cobra.Command {
_ = cmd.MarkFlagRequired("name") _ = cmd.MarkFlagRequired("name")
_ = cmd.MarkFlagRequired("message") _ = cmd.MarkFlagRequired("message")
cmd.MarkFlagsMutuallyExclusive("every", "cron")
return cmd return cmd
} }

View file

@ -41,3 +41,17 @@ func TestNewAddSubcommand(t *testing.T) {
require.NotEmpty(t, val) require.NotEmpty(t, val)
assert.Equal(t, "true", val[0]) assert.Equal(t, "true", val[0])
} }
func TestNewAddCommandEveryAndCronMutuallyExclusive(t *testing.T) {
cmd := newAddCommand(func() string { return "testing" })
cmd.SetArgs([]string{
"--name", "job",
"--message", "hello",
"--every", "10",
"--cron", "0 9 * * *",
})
err := cmd.Execute()
require.Error(t, err)
}

View file

@ -20,6 +20,8 @@ func NewCronCommand() *cobra.Command {
RunE: func(cmd *cobra.Command, _ []string) error { RunE: func(cmd *cobra.Command, _ []string) error {
return cmd.Help() return cmd.Help()
}, },
// Resolve storePath at execution time so it reflects the current config
// and is shared across all subcommands.
PersistentPreRunE: func(_ *cobra.Command, _ []string) error { PersistentPreRunE: func(_ *cobra.Command, _ []string) error {
cfg, err := internal.LoadConfig() cfg, err := internal.LoadConfig()
if err != nil { if err != nil {

View file

@ -9,7 +9,7 @@ func newDisableCommand(storePath func() string) *cobra.Command {
Args: cobra.ExactArgs(1), Args: cobra.ExactArgs(1),
Example: `picoclaw cron disable 1`, Example: `picoclaw cron disable 1`,
RunE: func(_ *cobra.Command, args []string) error { RunE: func(_ *cobra.Command, args []string) error {
cronEnableCmd(storePath(), true, args[0]) cronSetJobEnabled(storePath(), args[0], false)
return nil return nil
}, },
} }

View file

@ -9,7 +9,7 @@ func newEnableCommand(storePath func() string) *cobra.Command {
Args: cobra.ExactArgs(1), Args: cobra.ExactArgs(1),
Example: `picoclaw cron enable 1`, Example: `picoclaw cron enable 1`,
RunE: func(_ *cobra.Command, args []string) error { RunE: func(_ *cobra.Command, args []string) error {
cronEnableCmd(storePath(), false, args[0]) cronSetJobEnabled(storePath(), args[0], true)
return nil return nil
}, },
} }

View file

@ -55,17 +55,11 @@ func cronRemoveCmd(storePath, jobID string) {
} }
} }
func cronEnableCmd(storePath string, disable bool, jobID string) { func cronSetJobEnabled(storePath, jobID string, enabled bool) {
cs := cron.NewCronService(storePath, nil) cs := cron.NewCronService(storePath, nil)
enabled := !disable
job := cs.EnableJob(jobID, enabled) job := cs.EnableJob(jobID, enabled)
if job != nil { if job != nil {
status := "enabled" fmt.Printf("✓ Job '%s' enabled\n", job.Name)
if disable {
status = "disabled"
}
fmt.Printf("✓ Job '%s' %s\n", job.Name, status)
} else { } else {
fmt.Printf("✗ Job %s not found\n", jobID) fmt.Printf("✗ Job %s not found\n", jobID)
} }

View file

@ -47,3 +47,8 @@ func FormatBuildInfo() (build string, goVer string) {
} }
return return
} }
// GetVersion returns the version string
func GetVersion() string {
return version
}

View file

@ -91,3 +91,7 @@ func TestGetConfigPath_Windows(t *testing.T) {
require.True(t, strings.EqualFold(got, want), "GetConfigPath() = %q, want %q", got, want) require.True(t, strings.EqualFold(got, want), "GetConfigPath() = %q, want %q", got, want)
} }
func TestGetVersion(t *testing.T) {
assert.Equal(t, "dev", GetVersion())
}

View file

@ -6,7 +6,7 @@ import (
"github.com/spf13/cobra" "github.com/spf13/cobra"
internal2 "github.com/sipeed/picoclaw/cmd/picoclaw/internal" "github.com/sipeed/picoclaw/cmd/picoclaw/internal"
"github.com/sipeed/picoclaw/pkg/skills" "github.com/sipeed/picoclaw/pkg/skills"
) )
@ -23,7 +23,7 @@ func NewSkillsCommand() *cobra.Command {
Use: "skills", Use: "skills",
Short: "Manage skills", Short: "Manage skills",
PersistentPreRunE: func(cmd *cobra.Command, _ []string) error { PersistentPreRunE: func(cmd *cobra.Command, _ []string) error {
cfg, err := internal2.LoadConfig() cfg, err := internal.LoadConfig()
if err != nil { if err != nil {
return fmt.Errorf("error loading config: %w", err) return fmt.Errorf("error loading config: %w", err)
} }
@ -32,7 +32,7 @@ func NewSkillsCommand() *cobra.Command {
d.installer = skills.NewSkillInstaller(d.workspace) d.installer = skills.NewSkillInstaller(d.workspace)
// get global config directory and builtin skills directory // get global config directory and builtin skills directory
globalDir := filepath.Dir(internal2.GetConfigPath()) globalDir := filepath.Dir(internal.GetConfigPath())
globalSkillsDir := filepath.Join(globalDir, "skills") globalSkillsDir := filepath.Join(globalDir, "skills")
builtinSkillsDir := filepath.Join(globalDir, "picoclaw", "skills") builtinSkillsDir := filepath.Join(globalDir, "picoclaw", "skills")
d.skillsLoader = skills.NewSkillsLoader(d.workspace, globalSkillsDir, builtinSkillsDir) d.skillsLoader = skills.NewSkillsLoader(d.workspace, globalSkillsDir, builtinSkillsDir)

View file

@ -7,10 +7,12 @@
package main package main
import ( import (
"fmt"
"os" "os"
"github.com/spf13/cobra" "github.com/spf13/cobra"
"github.com/sipeed/picoclaw/cmd/picoclaw/internal"
"github.com/sipeed/picoclaw/cmd/picoclaw/internal/agent" "github.com/sipeed/picoclaw/cmd/picoclaw/internal/agent"
"github.com/sipeed/picoclaw/cmd/picoclaw/internal/auth" "github.com/sipeed/picoclaw/cmd/picoclaw/internal/auth"
"github.com/sipeed/picoclaw/cmd/picoclaw/internal/cron" "github.com/sipeed/picoclaw/cmd/picoclaw/internal/cron"
@ -23,9 +25,11 @@ import (
) )
func NewPicoclawCommand() *cobra.Command { func NewPicoclawCommand() *cobra.Command {
short := fmt.Sprintf("%s picoclaw - Personal AI Assistant v%s\n\n", internal.Logo, internal.GetVersion())
cmd := &cobra.Command{ cmd := &cobra.Command{
Use: "picoclaw", Use: "picoclaw",
Short: "picoclaw — Personal AI Assistant", Short: short,
Example: "picoclaw list", Example: "picoclaw list",
} }

View file

@ -1,11 +1,14 @@
package main package main
import ( import (
"fmt"
"slices" "slices"
"testing" "testing"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
"github.com/sipeed/picoclaw/cmd/picoclaw/internal"
) )
func TestNewPicoclawCommand(t *testing.T) { func TestNewPicoclawCommand(t *testing.T) {
@ -13,8 +16,10 @@ func TestNewPicoclawCommand(t *testing.T) {
require.NotNil(t, cmd) require.NotNil(t, cmd)
short := fmt.Sprintf("%s picoclaw - Personal AI Assistant v%s\n\n", internal.Logo, internal.GetVersion())
assert.Equal(t, "picoclaw", cmd.Use) assert.Equal(t, "picoclaw", cmd.Use)
assert.Equal(t, "picoclaw — Personal AI Assistant", cmd.Short) assert.Equal(t, short, cmd.Short)
assert.True(t, cmd.HasSubCommands()) assert.True(t, cmd.HasSubCommands())
assert.True(t, cmd.HasAvailableSubCommands()) assert.True(t, cmd.HasAvailableSubCommands())