apache / apache/ossie

cli: argument validation gaps remain after #334 (plugin, plugin install, convert)

Open
#345 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.