cli: argument validation gaps remain after #334 (plugin, plugin install, convert)
- Dominant language
- Python
- Stars
- 2.1k
- Forks
- 267
- Avg merge
- 4d 20h
- Merged PRs (30d)
- 24
Description
### Description
#334 / #335 fixed `ossie plugin list`, but the same "silently ignore positional arguments and exit 0" defect remains in three other places, including one level up on the `plugin` parent command itself.
The parent case is the most likely to be hit, since a typo'd subcommand is more common than a stray argument to a valid one, and it is easy to assume it was covered by #334.
**Important:** the one-line fix from #335 does *not* work on the parent command. See "Cause" below before attempting it.
### Steps to reproduce
Against `main` with #335 merged:
```bash
ossie plugin bogus; echo $? # unknown subcommand
ossie plugin help list; echo $? # wrong help, still succeeds
ossie plugin install a b c; echo $? # extra positional args ignored
ossie plugin install; echo $? # no name and no --all
ossie convert --from x --input y extra; echo $? # trailing arg ignored
```
### Actual behavior
All five exit `0`. `ossie plugin bogus` prints the `plugin` help to STDOUT and reports success; the rest print `not yet implemented` and report success, discarding the arguments.
### Expected behavior
Each should reject the invocation with a non-zero exit status and show usage, consistent with `plugin list` after #335 and with `plugin remove`.
### Cause
Two different causes, which is why one fix does not cover all of them:
**`plugin install` (`cli/cmd/plugin/install.go:25`) and `convert` (`cli/cmd/convert.go:25`)** simply declare no `Args` validator. Cobra treats a nil validator as accepting arbitrary arguments — the same root cause as #334.
**`plugin` (`cli/cmd/plugin/plugin.go:23`) is different.** Adding `Args: cobra.NoArgs` to `Cmd` has no effect, verified by building it: because the parent has no `Run`/`RunE`, cobra's `execute()` returns `flag.ErrHelp` at the `if !c.Runnable()` check (`command.go:954`) and never reaches `c.ValidateArgs` (`command.go:968`). The `Args` field is dead code on a non-runnable command.
### Suggested fix
- `plugin install`: `Args: cobra.MaximumNArgs(1)`, plus a check that exactly one of the plugin name or `--all` is supplied.
- `convert`: `Args: cobra.NoArgs` (it is flag-only).
- `plugin`: give it a `RunE` that prints help and returns an error, so an unknown or missing subcommand exits non-zero. `Args: cobra.NoArgs` alone will not do it.
To keep this from recurring as commands are added, consider one table-driven test that walks `rootCmd.Commands()` recursively and asserts every command either declares a non-nil `Args` or is a parent with an explicit help-and-error `RunE`. That would have caught all three of these in a single pass.
Note that a test asserting this by calling `cmd.ValidateArgs(...)` directly will pass on the `plugin` parent while the real CLI still exits 0. These need to be exercised through `rootCmd.SetArgs(...)` / `Execute()` with captured output.
### Related, likely a separate issue
The unimplemented stubs print `not yet implemented` and `return nil`, so they exit 0 while doing nothing: `convert` (`convert.go:46`), `validate` (`validate.go:38`), `plugin install` (`install.go:36`), `plugin remove` (`remove.go:33`). A pipeline running `ossie validate model.yaml && deploy` would deploy an unvalidated model. These should write to `cmd.ErrOrStderr()` and return an error until implemented. Happy to split this out if preferred.
### Environment
- Apache Ossie: PR #335 branch `fix/plugin-list-reject-args` @ `5947c69`
- Go: `go1.27.0 darwin/arm64`
- OS: macOS arm64
Contributor guide
Research direction
Read cli/cmd/plugin/plugin.go:23, cli/cmd/plugin/install.go:25, and cli/cmd/convert.go:25, then inspect Cobra's execution path at command.go:954 and command.go:968. Exercise rootCmd.SetArgs(...) and Execute() with captured output rather than calling ValidateArgs directly. Done means the listed invalid invocations return non-zero and show usage, with coverage for the command tree.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- cli, testing-qa
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100