refactor: parsing cli arguments use cobra by xiaket · Pull Request #513 · sipeed/picoclaw

xiaket · 2026-02-20T07:07:46Z

📝 Description

Parsing cli argument using os.Args is an anti pattern, let's move that to a better library.

🗣️ Type of Change

🐞 Bug fix (non-breaking change which fixes an issue)
✨ New feature (non-breaking change which adds functionality)
📖 Documentation update
⚡ Code refactoring (no functional changes, no api changes)

🤖 AI Code Generation

🤖 Fully AI-generated (100% AI, 0% Human)
🛠️ Mostly AI-generated (AI draft, Human verified/modified)
👨‍💻 Mostly Human-written (Human lead, AI assisted or none)

🔗 Related Issue

#429
#214

🧪 Test Environment

Irrelevant.

📸 Evidence (Optional)

     =============================================
       picoclaw old vs new binary comparison
     =============================================

     [1] Version output — must be identical
       SAME      version
       SAME      --version
       SAME      -v

     [2] Commands that do real work — must be identical
       SAME      auth status
       SAME      auth models
       SAME      cron list
       SAME      skills list
       SAME      status

     [3] Help text — cobra auto-generated replaces hand-written help
       EXPECTED  no args
                 -> cobra shows help instead of hand-written printHelp()
       EXPECTED  unknown command
                 -> cobra error format vs hand-written 'Unknown command:' + full help
       EXPECTED  auth (no subcommand)
                 -> cobra help with examples vs hand-written authHelp()
       EXPECTED  auth badcmd
                 -> cobra shows parent help silently; old printed 'Unknown auth command:'
       EXPECTED  cron (no subcommand)
                 -> cobra help vs hand-written cronHelp()
       EXPECTED  cron badcmd
                 -> cobra shows parent help silently; old printed 'Unknown cron command:'
       EXPECTED  skills (no subcommand)
                 -> cobra help vs hand-written skillsHelp()
       EXPECTED  skills badcmd
                 -> cobra shows parent help silently; old printed 'Unknown skills command:'
       EXPECTED  migrate --help
                 -> cobra help vs hand-written migrateHelp()
       EXPECTED  migrate --unknown-flag
                 -> cobra error format vs hand-written error + migrateHelp()

     [4] --help now works (old had no help handler, timed out doing real work)
       EXPECTED  agent --help
                 -> old: no --help, entered interactive mode (timeout)
       EXPECTED  agent -h
                 -> old: no -h, entered interactive mode (timeout)
       EXPECTED  gateway --help
                 -> old: no --help, started gateway (timeout)
       EXPECTED  onboard --help
                 -> old: no --help, ran onboard wizard (timeout)
       EXPECTED  auth login --help
                 -> old: printed 'Error: --provider is required'; new: shows flag help
       EXPECTED  auth logout --help
                 -> old: ran logout (logged out all); new: shows flag help

     [5] Stricter validation — old silently accepted bad input, new rejects it
       EXPECTED  agent --badflag
                 -> old: ignored flag, entered interactive (timeout); new: error exit 1
       EXPECTED  cron remove (no arg)
                 -> old: printed usage, exit 0; new: cobra 'accepts 1 arg', exit 1
       EXPECTED  cron enable (no arg)
                 -> old: printed usage, exit 0; new: cobra 'accepts 1 arg', exit 1
       EXPECTED  cron disable (no arg)
                 -> old: printed usage, exit 0; new: cobra 'accepts 1 arg', exit 1
       EXPECTED  skills remove (no arg)
                 -> old: printed usage, exit 0; new: cobra 'accepts 1 arg', exit 1
       EXPECTED  skills show (no arg)
                 -> old: printed usage, exit 0; new: cobra 'accepts 1 arg', exit 1
       EXPECTED  skills install (no arg)
                 -> old: printed usage, exit 1; new: cobra 'accepts 1 arg', exit 1

     [6] agent — flag parsing
       SAME      agent -d
       SAME      agent -m hello
       SAME      agent --model x -m y
       SAME      agent -s mysess -m y
       SAME      agent --session s --message hi
       EXPECTED  agent --debug -m hi
                 -> both call LLM and fail; debug log volume differs by map order

     [7] gateway — flag parsing
       EXPECTED  gateway -d
                 -> both start gateway and timeout; real work not CLI parsing
       EXPECTED  gateway --debug
                 -> both start gateway and timeout; real work not CLI parsing

     [8] migrate — all flags must produce identical output
       SAME      migrate --dry-run
       SAME      migrate --force --dry-run
       SAME      migrate --config-only --dry-run
       SAME      migrate --workspace-only --dry-run
       SAME      migrate --refresh --dry-run
       SAME      migrate --openclaw-home /nonexist --dry-run
       SAME      migrate --picoclaw-home /tmp/test --dry-run

     [9] auth — subcommand flag parsing
       EXPECTED  auth login (no --provider)
                 -> old: manual error + exit 0; new: cobra required flag error + exit 1
       SAME      auth logout
       SAME      auth logout -p openai
       SAME      auth logout --provider openai

     [10] cron — subcommand flag parsing
       EXPECTED  cron add --help
                 -> old: printed cronHelp(); new: cobra shows add-specific flags
       EXPECTED  cron list --help
                 -> old: listed jobs; new: cobra shows help
       EXPECTED  cron remove --help
                 -> old: printed usage; new: cobra shows help
       EXPECTED  cron enable --help
                 -> old: printed usage; new: cobra shows help
       EXPECTED  cron disable --help
                 -> old: printed usage; new: cobra shows help
       SAME      cron remove nonexist-id
       SAME      cron enable nonexist-id
       SAME      cron disable nonexist-id

     [11] skills — subcommand flag parsing and aliases
       EXPECTED  skills install --help
                 -> old: printed usage + exit 1; new: cobra help + exit 0
       EXPECTED  skills remove --help
                 -> old: ran remove('--help') + exit 1; new: cobra help + exit 0
       EXPECTED  skills show --help
                 -> old: showed skill '--help' not found; new: cobra help
       EXPECTED  skills search --help
                 -> old: ran search; new: cobra help
       EXPECTED  skills install-builtin --help
                 -> old: ran install-builtin; new: cobra help
       EXPECTED  skills list-builtin --help
                 -> old: ran list-builtin; new: cobra help
       SAME      skills show nonexist
       SAME      skills remove nonexist
       EXPECTED  skills uninstall (alias)
                 -> old: fell through to skillsHelp(); new: cobra alias needs arg, exit 1

     =============================================
       SAME=28  EXPECTED=39  UNEXPECTED=0
     =============================================
       All differences are expected. Migration is safe.

☑️ 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.

nikolasdehor

Thorough Cobra migration with an excellent verification matrix (28 SAME + 39 EXPECTED + 0 UNEXPECTED). The level of testing documented in the PR body gives strong confidence this is a safe migration.

A few observations:

RunE returning nil on user-facing errors: Throughout the code, when an error occurs (e.g., config load failure), the pattern is fmt.Printf("Error: ..."); return nil. This means cobra treats it as success (exit 0). In the old code, some of these paths called os.Exit(1). You may want to return the error (or return fmt.Errorf(...)) for cases that should exit non-zero, and use cobra's SilenceErrors/SilenceUsage to control whether cobra prints its own message. That said, this preserves the existing behavior for the most part, so it's not a blocker.
--version / -v handling: The rootCmd.RunE checks the version flag manually. Cobra has built-in version support via rootCmd.Version = version that handles --version natively. But your approach gives full control over the output format, so this is fine.
Global rootCmd var: Using a package-level var rootCmd with init() is the standard Cobra pattern, though some projects prefer constructing the root command in a function for testability. Not a concern for a CLI binary.
New dependency: spf13/cobra + spf13/pflag + mousetrap adds ~3 transitive deps. For a CLI app this is reasonable — Cobra is the de facto standard.
Unicode escapes: The emoji replacements ("\u2713" for checkmark, "\U0001f50d" for magnifier) are functionally identical. Just noting this is a side effect of the rewrite, not intentional normalization.

Overall this is a significant improvement for maintainability. LGTM.

Signed-off-by: Kai Xia <kaix+github@fastmail.com>

yofabr · 2026-02-20T19:03:00Z

@yinwm This looks good to me

nikolasdehor · 2026-02-23T00:39:41Z

Note: #429 also migrates the CLI to Cobra. Both PRs address the same refactoring goal. Suggesting the maintainer pick one to avoid conflicting implementations.

xiaket · 2026-02-24T10:53:57Z

Closing in favour of #429

nikolasdehor approved these changes Feb 20, 2026

View reviewed changes

yofabr approved these changes Feb 20, 2026

View reviewed changes

xiaket added 4 commits February 21, 2026 01:35

refactor: parsing cli arguments use cobra

446b4ec

Signed-off-by: Kai Xia <kaix+github@fastmail.com>

add example back

6b95596

Signed-off-by: Kai Xia <kaix+github@fastmail.com>

make golangci happy

b2c03a4

Signed-off-by: Kai Xia <kaix+github@fastmail.com>

add example, fix issue with registry flag

c43ca92

Signed-off-by: Kai Xia <kaix+github@fastmail.com>

xiaket force-pushed the refactor-cobra branch from 21e4236 to c43ca92 Compare February 20, 2026 15:10

lint

37a40c1

Signed-off-by: Kai Xia <kaix+github@fastmail.com>

nikolasdehor mentioned this pull request Feb 23, 2026

refactor(cli): migrate to Cobra-based command structure #429

Merged

10 tasks

xiaket closed this Feb 24, 2026

nikolasdehor mentioned this pull request Feb 24, 2026

[Feature] Add --help flag to every subcommand #574

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: parsing cli arguments use cobra#513

refactor: parsing cli arguments use cobra#513
xiaket wants to merge 5 commits intosipeed:mainfrom
xiaket:refactor-cobra

xiaket commented Feb 20, 2026

Uh oh!

nikolasdehor left a comment

Uh oh!

yofabr commented Feb 20, 2026

Uh oh!

nikolasdehor commented Feb 23, 2026

Uh oh!

xiaket commented Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

xiaket commented Feb 20, 2026

📝 Description

🗣️ Type of Change

🤖 AI Code Generation

🔗 Related Issue

🧪 Test Environment

📸 Evidence (Optional)

☑️ Checklist

Uh oh!

nikolasdehor left a comment

Choose a reason for hiding this comment

Uh oh!

yofabr commented Feb 20, 2026

Uh oh!

nikolasdehor commented Feb 23, 2026

Uh oh!

xiaket commented Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants