From 5011edc292b2f159c42d0cd70d868971d227efeb Mon Sep 17 00:00:00 2001 From: elasticspoon Date: Sun, 17 Mar 2024 23:17:43 -0400 Subject: [PATCH] fix(cli): show error/hide help for unsupported subcommands (#10760) (#12624) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #10760 The coder CLI quietly accepts any subcommand arguments and silently swallows them. Currently: ```sh ❯ coder | head -n5 coder v2.3.3+e491217 USAGE: coder [global-flags] ``` ```sh ❯ coder idontexist | head -n5 coder v2.3.3+e491217 USAGE: coder [global-flags] ``` Now help output will not be show when there is an unknown subcommand error. Instead users will be given the command for the help output. ```sh ❯ coder idontexist Encountered an error running "coder", see "coder --help" for more information error: unrecognized subcommand "idontexist" ``` ```sh ❯ coder iexistbut idontexist Encountered an error running "coder iexistbut", see "coder iexistbut --help" for more information error: unrecognized subcommand "idontexist" ``` Also this stuff: `Encountered an error running "coder iexistbut"... ` gets written to `os.Stdout` in `prettyErrorFormatter{w: os.Stderr, verbose: r.verbose}`, not sure how to test that output. --- cli/help.go | 24 ++++++++++++++++++++--- cli/portforward.go | 4 ---- cli/portforward_test.go | 5 ----- cli/root.go | 7 +++++-- cli/root_test.go | 43 +++++++++++++++++++++++++++++++++++++++++ go.sum | 2 -- 6 files changed, 69 insertions(+), 16 deletions(-) diff --git a/cli/help.go b/cli/help.go index 0654eb7bd9..4ece61b437 100644 --- a/cli/help.go +++ b/cli/help.go @@ -4,7 +4,9 @@ import ( "bufio" _ "embed" "fmt" + "os" "regexp" + "slices" "sort" "strings" "text/tabwriter" @@ -320,6 +322,25 @@ var usageWantsArgRe = regexp.MustCompile(`<.*>`) // output for a given command. func helpFn() serpent.HandlerFunc { return func(inv *serpent.Invocation) error { + // Check for invalid subcommands before printing help. + if len(inv.Args) > 0 && !usageWantsArgRe.MatchString(inv.Command.Use) { + _, _ = fmt.Fprintf(inv.Stderr, "---\nerror: unrecognized subcommand %q\n", inv.Args[0]) + } + if len(inv.Args) > 0 { + // Return an error so that exit status is non-zero when + // a subcommand is not found. + err := xerrors.Errorf("unrecognized subcommand %q", strings.Join(inv.Args, " ")) + if slices.Contains(os.Args, "--help") { + // Subcommand error is not wrapped in RunCommandErr if command + // is invoked with --help with no HelpHandler + return &serpent.RunCommandError{ + Cmd: inv.Command, + Err: err, + } + } + return err + } + // We use stdout for help and not stderr since there's no straightforward // way to distinguish between a user error and a help request. // @@ -340,9 +361,6 @@ func helpFn() serpent.HandlerFunc { if err != nil { return err } - if len(inv.Args) > 0 && !usageWantsArgRe.MatchString(inv.Command.Use) { - _, _ = fmt.Fprintf(inv.Stderr, "---\nerror: unknown subcommand %q\n", inv.Args[0]) - } return nil } } diff --git a/cli/portforward.go b/cli/portforward.go index e4b44609d5..68a076d590 100644 --- a/cli/portforward.go +++ b/cli/portforward.go @@ -69,10 +69,6 @@ func (r *RootCmd) portForward() *serpent.Command { return xerrors.Errorf("parse port-forward specs: %w", err) } if len(specs) == 0 { - err = inv.Command.HelpHandler(inv) - if err != nil { - return xerrors.Errorf("generate help output: %w", err) - } return xerrors.New("no port-forwards requested") } diff --git a/cli/portforward_test.go b/cli/portforward_test.go index b211a840dd..9ea5335c43 100644 --- a/cli/portforward_test.go +++ b/cli/portforward_test.go @@ -35,15 +35,10 @@ func TestPortForward_None(t *testing.T) { inv, root := clitest.New(t, "port-forward", "blah") clitest.SetupConfig(t, member, root) - pty := ptytest.New(t).Attach(inv) - inv.Stderr = pty.Output() err := inv.Run() require.Error(t, err) require.ErrorContains(t, err, "no port-forwards") - - // Check that the help was printed. - pty.ExpectMatch("port-forward ") } func TestPortForward(t *testing.T) { diff --git a/cli/root.go b/cli/root.go index d75c319c01..27330c856f 100644 --- a/cli/root.go +++ b/cli/root.go @@ -1196,10 +1196,13 @@ func formatMultiError(from string, multi []error, opts *formatOpts) string { // formatter and add it to cliHumanFormatError function. func formatRunCommandError(err *serpent.RunCommandError, opts *formatOpts) string { var str strings.Builder - _, _ = str.WriteString(pretty.Sprint(headLineStyle(), fmt.Sprintf("Encountered an error running %q", err.Cmd.FullName()))) + _, _ = str.WriteString(pretty.Sprint(headLineStyle(), + fmt.Sprintf( + `Encountered an error running %q, see "%s --help" for more information`, + err.Cmd.FullName(), err.Cmd.FullName()))) + _, _ = str.WriteString(pretty.Sprint(headLineStyle(), "\nerror: ")) msgString, special := cliHumanFormatError("", err.Err, opts) - _, _ = str.WriteString("\n") if special { _, _ = str.WriteString(msgString) } else { diff --git a/cli/root_test.go b/cli/root_test.go index 07439322eb..bc10cfceec 100644 --- a/cli/root_test.go +++ b/cli/root_test.go @@ -60,6 +60,49 @@ func TestCommandHelp(t *testing.T) { func TestRoot(t *testing.T) { t.Parallel() + t.Run("MissingRootCommand", func(t *testing.T) { + t.Parallel() + + out := new(bytes.Buffer) + + inv, _ := clitest.New(t, "idontexist") + inv.Stdout = out + + err := inv.Run() + assert.ErrorContains(t, err, + `unrecognized subcommand "idontexist"`) + require.Empty(t, out.String()) + }) + + t.Run("MissingSubcommand", func(t *testing.T) { + t.Parallel() + + out := new(bytes.Buffer) + + inv, _ := clitest.New(t, "server", "idontexist") + inv.Stdout = out + + err := inv.Run() + // subcommand error only when command has subcommands + assert.ErrorContains(t, err, + `unrecognized subcommand "idontexist"`) + require.Empty(t, out.String()) + }) + + t.Run("BadSubcommandArgs", func(t *testing.T) { + t.Parallel() + + out := new(bytes.Buffer) + + inv, _ := clitest.New(t, "list", "idontexist") + inv.Stdout = out + + err := inv.Run() + assert.ErrorContains(t, err, + `wanted no args but got 1 [idontexist]`) + require.Empty(t, out.String()) + }) + t.Run("Version", func(t *testing.T) { t.Parallel() diff --git a/go.sum b/go.sum index 75d2cbcbef..718c742125 100644 --- a/go.sum +++ b/go.sum @@ -218,8 +218,6 @@ github.com/coder/pretty v0.0.0-20230908205945-e89ba86370e0 h1:3A0ES21Ke+FxEM8CXx github.com/coder/pretty v0.0.0-20230908205945-e89ba86370e0/go.mod h1:5UuS2Ts+nTToAMeOjNlnHFkPahrtDkmpydBen/3wgZc= github.com/coder/retry v1.5.1 h1:iWu8YnD8YqHs3XwqrqsjoBTAVqT9ml6z9ViJ2wlMiqc= github.com/coder/retry v1.5.1/go.mod h1:blHMk9vs6LkoRT9ZHyuZo360cufXEhrxqvEzeMtRGoY= -github.com/coder/serpent v0.4.0 h1:L/itwnCxfhLutxQ2mScP3tH1ro8z8+Kc/iKKyZZxEMk= -github.com/coder/serpent v0.4.0/go.mod h1:Wud83ikZF/ulbdMcEMAwqvkEIQx7+l47+ef69M/arAA= github.com/coder/serpent v0.4.1-0.20240315163851-a0148c87ea3f h1:nqJ/Mvm+nLI22n5BIYhvSmTZ6CD+MRo/aGVZwVQgr1k= github.com/coder/serpent v0.4.1-0.20240315163851-a0148c87ea3f/go.mod h1:REkJ5ZFHQUWFTPLExhXYZ1CaHFjxvGNRlLXLdsI08YA= github.com/coder/ssh v0.0.0-20231128192721-70855dedb788 h1:YoUSJ19E8AtuUFVYBpXuOD6a/zVP3rcxezNsoDseTUw=