refactor(cli): migrate to Cobra-based command structure#429
refactor(cli): migrate to Cobra-based command structure#429xiaket merged 17 commits intosipeed:mainfrom
Conversation
Refactor CLI to use Cobra instead of manual os.Args parsing. - Introduce root command and structured subcommands under cmd/picoclaw/internal - Convert agent, auth, cron, gateway, migrate, onboard, skills, status and version to Cobra commands - Replace manual flag parsing with Cobra flags - Remove direct os.Args usage from command handlers - Keep existing command behavior and output semantics This change focuses on CLI structure and maintainability. No business logic changes intended.
bd3e195 to
0d2202e
Compare
|
I will keep this branch synced with |
|
Happy to be duplicated: #214 |
nikolasdehor
left a comment
There was a problem hiding this comment.
Significant refactor moving from manual os.Args parsing to Cobra. The structure looks well-organized with each command getting its own package under cmd/picoclaw/internal/. A few observations:
-
No tests for CLI commands — This is a large refactor touching 36 files with 1725 additions. Even basic tests verifying command registration and flag parsing would help catch regressions. Cobra makes this easy with cmd.Execute() in test contexts.
-
LDFLAGS change — The Makefile now injects version info into github.com/sipeed/picoclaw/cmd/picoclaw/internal instead of main. Make sure goreleaser or any release automation also uses this updated path, otherwise release builds will have empty version strings.
-
Error handling style — The helper functions (e.g., agentCmd, authLoginCmd) properly return errors via RunE, which is good. However, some functions still use fmt.Printf for warnings (e.g., 'Warning: could not update config') instead of returning them. This is inconsistent — either log all warnings or propagate them.
-
Unused authHelp function — The auth/helpers.go defines an authHelp() function that manually prints help text, but with Cobra this should be auto-generated. If this function is called somewhere, it duplicates what Cobra provides. If not, it is dead code.
-
interactiveMode error comparisons — In agent/helpers.go, err == readline.ErrInterrupt and err == io.EOF should use errors.Is for consistency with PR #460 which is migrating the codebase to errors.Is.
-
Dependency addition — Cobra + pflag + mousetrap are reasonable dependencies for a CLI tool. The indirect dependency count is acceptable.
Overall this is a good direction. The main concern is the lack of tests for such a large refactor. Consider adding at least integration tests that verify the command tree is wired correctly.
|
@nikolasdehor Regarding the error comparisons: I intentionally left them unchanged in this PR to preserve original behavior and avoid introducing additional noise in an already large structural refactor. The goal here was strictly CLI restructuring. I agree that migrating remaining direct error comparisons to I’ll also take care of the other noted items (CLI tests, LDFLAGS verification, help cleanup, and consistency improvements) to keep this refactor solid and maintainable. Appreciate the thorough feedback. |
- Add tests for CLI command tree and flag parsing - Align LDFLAGS injection path for version info - Remove unused manual help function
|
Thanks again for the detailed review. I’ve pushed updates addressing the comments (CLI tests, LDFLAGS alignment, help cleanup, etc.) and synced the branch with the latest Would appreciate another look when you have time. |
Replace standard library testing error checks (t.Error*, t.Fatalf) with assert/require from stretchr/testify across all cobra command tests for improved readability and consistency.
|
Note: #513 also migrates the CLI to Cobra. Both PRs address the same refactoring goal. Suggesting the maintainer pick one to avoid conflicting implementations. |
- Move subcommand registration out of PersistentPreRunE - Ensure `picoclaw skills <subcommand>` resolves correctly - Minor install command and test cleanups
xiaket
left a comment
There was a problem hiding this comment.
cmd/picoclaw/internal/auth/status.go need to be updated, but others are nits and for your consideration only.
Thanks for bearing with me! I have not been very swift in the review of this PR.
Thanks for the thorough review - really appreciate the detailed feedback. I’ll update No worries at all about the timing - I appreciate you taking the time to review such a large PR. |
* refactor(cli): migrate to Cobra-based command structure Refactor CLI to use Cobra instead of manual os.Args parsing. - Introduce root command and structured subcommands under cmd/picoclaw/internal - Convert agent, auth, cron, gateway, migrate, onboard, skills, status and version to Cobra commands - Replace manual flag parsing with Cobra flags - Remove direct os.Args usage from command handlers - Keep existing command behavior and output semantics This change focuses on CLI structure and maintainability. No business logic changes intended. * chore(cli): remove version2 alias and make cobra a direct dependency * test(cli): add basic command tests - Add tests for CLI command tree and flag parsing - Align LDFLAGS injection path for version info - Remove unused manual help function * test: migrate command tests to testify assertions Replace standard library testing error checks (t.Error*, t.Fatalf) with assert/require from stretchr/testify across all cobra command tests for improved readability and consistency. * fix(cli): make linter happy * test: avoid duplication in windows config path test * test: simplify allowed command checks using slices.Contains * fix(skills): register subcommands during command construction - Move subcommand registration out of PersistentPreRunE - Ensure `picoclaw skills <subcommand>` resolves correctly - Minor install command and test cleanups * refactor(cli): address review feedback and improve command clarity * fix(authLogoutCmd): rm os.Exit
* refactor(cli): migrate to Cobra-based command structure Refactor CLI to use Cobra instead of manual os.Args parsing. - Introduce root command and structured subcommands under cmd/picoclaw/internal - Convert agent, auth, cron, gateway, migrate, onboard, skills, status and version to Cobra commands - Replace manual flag parsing with Cobra flags - Remove direct os.Args usage from command handlers - Keep existing command behavior and output semantics This change focuses on CLI structure and maintainability. No business logic changes intended. * chore(cli): remove version2 alias and make cobra a direct dependency * test(cli): add basic command tests - Add tests for CLI command tree and flag parsing - Align LDFLAGS injection path for version info - Remove unused manual help function * test: migrate command tests to testify assertions Replace standard library testing error checks (t.Error*, t.Fatalf) with assert/require from stretchr/testify across all cobra command tests for improved readability and consistency. * fix(cli): make linter happy * test: avoid duplication in windows config path test * test: simplify allowed command checks using slices.Contains * fix(skills): register subcommands during command construction - Move subcommand registration out of PersistentPreRunE - Ensure `picoclaw skills <subcommand>` resolves correctly - Minor install command and test cleanups * refactor(cli): address review feedback and improve command clarity * fix(authLogoutCmd): rm os.Exit
📝 Description
This PR refactors the CLI implementation by replacing manual
os.Args-based argument parsing with a structured command hierarchy built on Cobra.The change introduces a root command and dedicated subcommand packages under
cmd/picoclaw/internal. Existing commands (agent,auth,cron,gateway,migrate,onboard,skills,status,version) are migrated to Cobra-based command definitions.Manual flag parsing and direct
os.Argsaccess have been removed from command handlers. All flags and argument validation are now handled through Cobra.No business logic or runtime behavior is intended to change. This refactor focuses on improving CLI structure, maintainability, and extensibility.
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
N/A
📚 Technical Context (Skip for Docs)
🧪 Test Environment
📸 Evidence (Optional)
Verified that all existing commands behave identically:
Help output and flag handling function correctly under Cobra.
☑️ Checklist