fix(cli): show error/hide help for unsupported subcommands (coder#10760) (coder#12624)

elasticspoon · web-flow · commit 5011edc292b2 · 2024-03-17T22:17:43.000-05:00
Fixes coder#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] <subcommand> ``` ```sh ❯ coder idontexist | head -n5 coder v2.3.3+e491217 USAGE: coder [global-flags] <subcommand> ``` 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.
diff --git 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
@@ -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
@@ -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 <workspace>")
 }
 
 func TestPortForward(t *testing.T) {
diff --git 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
@@ -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
@@ -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=

Original file line number	Diff line number	Diff line change
`@@ -69,10 +69,6 @@ func (r RootCmd) portForward() serpent.Command {`
`69`	`69`	`return xerrors.Errorf("parse port-forward specs: %w", err)`
`70`	`70`	`}`
`71`	`71`	`if len(specs) == 0 {`
`72`		`- err = inv.Command.HelpHandler(inv)`
`73`		`- if err != nil {`
`74`		`- return xerrors.Errorf("generate help output: %w", err)`
`75`		`- }`
`76`	`72`	`return xerrors.New("no port-forwards requested")`
`77`	`73`	`}`
`78`	`74`