feat(shell): route built-in commands with cobra - #90
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR replaces the shell's hand-written command dispatcher, help table, and per-command flag parsing with Cobra routing for the shell's own built-in commands (help, login, module, version). Common flags (--context, --output) are declared once on the root, help is rendered from the real command tree, and flag/usage failures are converted into the shell's typed problem model with the documented usage exit class. Crucially, product namespaces are deliberately not registered as Cobra commands: an unrecognized first argument still falls through to module-store resolution with its arguments unparsed, preserving the passthrough contract that #85 exists to protect. It settles the four open decisions from the shell-command-framework research in the new ADR 0008.
Changes:
- Introduces
internal/app/command.gobuilding the Cobra root (non-interspersed parsing, silenced errors/usage, disabled completion, templated help) plus a flag-forwarding shim so built-in bodies keep parsing their own arguments. - Rewrites
dispatch/helpinapp.goand unifies the two--outputparsers viaoutputFlagValueininvoke.go, with a table-driven test pinning them to agree. - Adds
cobra/pflagdependencies and a boundaries test asserting the shell links neither Cobra's doc generator nor its Markdown/YAML transitive deps; adds ADR 0008 and points the research doc at it.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/app/command.go |
New Cobra command tree, flag forwarding, usage-problem wrapping, suggestions |
internal/app/app.go |
Dispatch routes shell commands/flags to Cobra; namespace passthrough preserved; help via tree |
internal/app/invoke.go |
outputFlagValue unifies attached-value output-flag spellings on the namespace path |
internal/app/commandui_test.go |
End-to-end tests for help, suggestions, flag positions, refusals, passthrough |
internal/app/outputflag_internal_test.go |
Pins pflag and hand parser of --output to agree |
internal/boundaries/boundaries_test.go |
Asserts linked set includes cobra/pflag, excludes doc-gen deps |
go.mod / go.sum |
Adds cobra, pflag, mousetrap dependencies |
docs/adr/0008-...md |
Records the Cobra adoption decision and its constraints |
docs/research/shell-command-framework.md |
Status pointer to ADR 0008 |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Declared here so its description reads in the shell's voice rather than | ||
| // as the framework's default. | ||
| // Flag parsing stops at the first argument that is not a flag. Everything | ||
| // after it may be a product namespace and the module's own flags, and those | ||
| // must reach the module verbatim rather than being parsed here. Without | ||
| // this, "wso2 --context prod api list --env stage" fails on --env, which is | ||
| // the module's flag and none of the shell's business. | ||
| root.Flags().SetInterspersed(false) | ||
|
|
||
| root.PersistentFlags().BoolP("help", "h", false, "Show help for a command.") | ||
| root.PersistentFlags().String(contextFlag, "", "Use the named context instead of the selected one.") | ||
| root.PersistentFlags().StringP(outputFlag, "o", string(output.ModeTable), "Render results as table or json.") |
There was a problem hiding this comment.
Fixed in 8e47211. The comment now sits directly above the help flag it explains.
| if err := root.Execute(); err != nil { | ||
| return usageProblem(err) |
There was a problem hiding this comment.
Fixed in 8e47211. Flag failures are now wrapped by Cobra's flag-error hook, so usageProblem only ever sees a parse failure and Execute's other errors reach Run for its own classification. TestAFailureInsideACommandIsNotReportedAsAUsageProblem fails with exit 64 if the old wrapping comes back.
b3fee03 to
f3ddb74
Compare
Stacked on #84. Closes #87, the first ticket under #85.
The shell's own commands are now routed by Cobra. The flags common to them are declared once on the root instead of re-parsed per command, help is rendered from the real command tree rather than a hand-maintained table, and a flag failure arrives as a typed usage problem with recovery guidance and the documented exit class rather than in Cobra's voice.
docs/research/shell-command-framework.mdrecommended deferring this until one of three triggers fired; two had, and its four open decisions are settled in ADR 0008, which the research document now points at.Flag semantics did not change. The built-in bodies still parse their own arguments behind the new routing, reached through a shim that re-attaches the flags Cobra parsed, so the existing tests are the regression suite for the change. Converting the remaining hand-parsed flags is #89.
A product namespace is not a Cobra command. An argument the shell does not recognize as a built-in never enters the tree; it resolves against the managed module store as before. That keeps built-in precedence a property of dispatch order rather than an interaction with a command set discovered at runtime, and keeps
wso2 helpfree of any module store read. Cobra's allowlist for unknown flags is deliberately not used anywhere: it does not forward an unknown flag, it discards it with its value, sowso2 api gateway list --env prodwould reach the module as[gateway list]and run against the wrong environment.Flag parsing stops at the first argument that is not a flag. This is the one non-obvious setting. Without it Cobra parses the whole remaining argument list, and
wso2 --context prod api list --env stagefails on--env— a module's flag rejected by the shell, which is the constraint #85 exists to protect. The first version of this branch had that defect, and the test matrix that was supposed to catch it passed because no case put a module flag after a leading shell flag.A flag a command cannot act on is refused, not ignored. No built-in honors
--outputtoday:versionrenders fixed fields and the module lifecycle commands render their own tables, so only the namespace path acts on it. Accepting it and dropping it would leave the user believing it took effect, so each command names the shell flags it honors and the rest are refused asshell.unsupported_flag. Three of #87's acceptance criteria were amended for this and related reasons, with the reasoning recorded in a comment on that issue.Two further defects found while reviewing and fixed here: a shell flag before a namespace printed help instead of dispatching, and the two parsers of
--outputhad already drifted, since pflag accepts-o=jsonand-ojsonwhile the passthrough parser matched only a bare-o. A test now feeds every spelling through both parsers and asserts they agree. The combined shorthand is claimed only when its value is an actual mode, so a module flag such as-optimizestays the module's.The shell links
cobraandpflagand nothing else from that family. Cobra's documentation generator pulls a Markdown renderer and a YAML parser into the module graph, and a boundaries test asserts they are absent from the linked set. Completion is deliberately not added: until a module declares its command tree (#86) it would know every built-in and no product command, which reads as absence of the command rather than absence of information.Verified with
golangci-lint run(0 issues),go test ./internal/..., and the acceptance runs covering version, unknown commands, and brokered reference access. Fourtest/acceptancetests fail locally on GPG commit signing; they fail identically on the base branch and are unrelated to this change.