Make a mistyped subcommand under a group command an error (closes #319)

snapshot, config, remote, database and completion had no Run, and
cobra checks a command's arguments only when it has one, so `vaultik
snapshot craete` printed the group's help and exited 0. Each group now
prints its help only when given no argument; any other word fails as
an unknown command with exit 1, as at the top level.

cobra adds the completion command only when Execute runs, so the root
command now adds it itself to give it a Run. Its scripts then go to
os.Stdout as it was at that moment; a later SetOut does not redirect
them.

Judgement call: the error has no "Did you mean" suggestion, which the
top level gives.

Model: opus-5-5

internal/cli/config.go

@@ -251,6 +251,12 @@ func NewConfigCommand() *cobra.Command {
 		Use:   "config",
 		Short: "Manage the configuration file",
 		Long:  "Commands for creating, editing, and querying the vaultik config file.",
+		// cobra checks Args only on a command that has a Run: without
+		// one, a mistyped subcommand printed help and exited 0.
+		Args: cobra.NoArgs,
+		Run: func(cmd *cobra.Command, _ []string) {
+			_ = cmd.Help()
+		},
 	}
 
 	cmd.AddCommand(newConfigInitCommand())

internal/cli/database.go

@@ -15,6 +15,12 @@ func NewDatabaseCommand() *cobra.Command {
 		Use:   "database",
 		Short: "Manage the local state database",
 		Long:  `Commands for managing the local SQLite state database.`,
+		// cobra checks Args only on a command that has a Run: without
+		// one, a mistyped subcommand printed help and exited 0.
+		Args: cobra.NoArgs,
+		Run: func(cmd *cobra.Command, _ []string) {
+			_ = cmd.Help()
+		},
 	}
 
 	cmd.AddCommand(

internal/cli/entry_unknown_subcommand_test.go

@@ -0,0 +1,43 @@
+package cli //nolint:testpackage // shares runEntry and the command-word constants
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// TestEntryGroupRejectsUnknownSubcommand runs each group command with a
+// mistyped subcommand, as a crontab line might carry one. It must fail
+// with exit 1 and name the word, as `vaultik bogus` does at the top
+// level, instead of printing the group's help and exiting 0 having done
+// nothing. The group name alone still prints its help.
+//
+//nolint:paralleltest // replaces os.Args, os.Stdout and os.Stderr
+func TestEntryGroupRejectsUnknownSubcommand(t *testing.T) {
+	for _, testCase := range []struct {
+		group    string
+		mistyped string
+	}{
+		{group: cmdSnapshot, mistyped: "craete"},
+		{group: "config", mistyped: "inti"},
+		{group: cmdRemote, mistyped: "ifno"},
+		{group: "database", mistyped: "delte"},
+		{group: "completion", mistyped: "nosuchshell"},
+	} {
+		t.Run(testCase.group, func(t *testing.T) {
+			code, stdout, stderr := runEntry(t,
+				testCase.group, testCase.mistyped)
+
+			assert.Equal(t, 1, code)
+			assert.Empty(t, stdout)
+			assert.Contains(t, stderr,
+				`unknown command "`+testCase.mistyped+`"`)
+
+			code, stdout, _ = runEntry(t, testCase.group)
+
+			assert.Equal(t, 0, code)
+			assert.Contains(t, stdout,
+				programName+" "+testCase.group+" [command]")
+		})
+	}
+}

internal/cli/remote.go

@@ -18,6 +18,12 @@ func NewRemoteCommand() *cobra.Command {
 		Use:   "remote",
 		Short: "Remote storage management commands",
 		Long:  "Commands for inspecting and managing remote storage",
+		// cobra checks Args only on a command that has a Run: without
+		// one, a mistyped subcommand printed help and exited 0.
+		Args: cobra.NoArgs,
+		Run: func(cmd *cobra.Command, _ []string) {
+			_ = cmd.Help()
+		},
 	}
 
 	// Add subcommands

internal/cli/root.go

@@ -74,6 +74,21 @@ on the source system.`,
 		NewDatabaseCommand(),
 	)
 
+	// cobra adds the completion command only when Execute runs, and with
+	// no Run, so it skipped its own cobra.NoArgs and a mistyped shell name
+	// printed help and exited 0. Adding it here lets it have a Run. Its
+	// scripts go to os.Stdout as it is now: a later SetOut does not
+	// redirect them.
+	cmd.InitDefaultCompletionCmd()
+
+	for _, sub := range cmd.Commands() {
+		if sub.Name() == "completion" {
+			sub.Run = func(completion *cobra.Command, _ []string) {
+				_ = completion.Help()
+			}
+		}
+	}
+
 	return cmd
 }
 

internal/cli/snapshot.go

@@ -41,6 +41,12 @@ func NewSnapshotCommand() *cobra.Command {
 		Use:   "snapshot",
 		Short: "Snapshot management commands",
 		Long:  "Commands for creating, listing, and managing snapshots",
+		// cobra checks Args only on a command that has a Run: without
+		// one, a mistyped subcommand printed help and exited 0.
+		Args: cobra.NoArgs,
+		Run: func(cmd *cobra.Command, _ []string) {
+			_ = cmd.Help()
+		},
 	}
 
 	// Add subcommands

internal/storage/url.go

@@ -29,10 +29,7 @@ var (
 	ErrURLCredentials     = errors.New(
 		"storage URL must not carry credentials; " +
 			"set s3.access_key_id and s3.secret_access_key in the config instead")
-	ErrURLUnknownParam    = errors.New("unknown query parameter in storage URL")
-	ErrRcloneRemoteParams = errors.New(
-		"rclone URL must not carry parameters in the remote name; " +
-			"set them on the remote with rclone config instead")
+	ErrURLUnknownParam = errors.New("unknown query parameter in storage URL")
 )
 
 // URL represents a parsed storage URL.
@@ -93,9 +90,8 @@ func parseFileURL(rawPath string) (*URL, error) {
 }
 
 // parseS3URL parses an s3://bucket/prefix URL. It rejects credentials in
-// the userinfo or in the endpoint parameter's userinfo, and any query
-// parameter other than endpoint, region and ssl, so a credential-bearing
-// URL is never stored or echoed.
+// the userinfo and any query parameter other than endpoint, region and
+// ssl, so a credential-bearing URL is never stored or echoed.
 func parseS3URL(rawURL string) (*URL, error) {
 	u, err := url.Parse(rawURL)
 	if err != nil {
@@ -118,14 +114,6 @@ func parseS3URL(rawURL string) (*URL, error) {
 		return nil, err
 	}
 
-	// A host name never contains "@", so one in an endpoint, with or
-	// without a scheme, can only introduce userinfo.
-	for _, endpoint := range query["endpoint"] {
-		if strings.Contains(endpoint, "@") {
-			return nil, ErrURLCredentials
-		}
-	}
-
 	return &URL{
 		Scheme:   schemeS3,
 		Bucket:   bucket,
@@ -137,9 +125,8 @@ func parseS3URL(rawURL string) (*URL, error) {
 }
 
 // parseRcloneURL parses an rclone://remote/path URL. rclone:// takes no
-// query parameters, so credentials in the userinfo, parameters in the
-// remote name and any query parameter at all are rejected rather than
-// silently ignored.
+// query parameters, so credentials in the userinfo and any parameter at
+// all are rejected rather than silently ignored.
 func parseRcloneURL(rawURL string) (*URL, error) {
 	u, err := url.Parse(rawURL)
 	if err != nil {
@@ -155,12 +142,6 @@ func parseRcloneURL(rawURL string) (*URL, error) {
 		return nil, ErrMissingRemote
 	}
 
-	// An rclone remote name never contains a comma; one starts rclone's
-	// remote,param=value form, whose values can be credentials.
-	if strings.Contains(remote, ",") {
-		return nil, ErrRcloneRemoteParams
-	}
-
 	err = rejectUnknownParams(u.Query())
 	if err != nil {
 		return nil, err

internal/storage/url_parse_test.go

@@ -150,19 +150,6 @@ func TestParseStorageURLRejectsCredentials(t *testing.T) {
 			wantErr: storage.ErrURLCredentials,
 			secrets: []string{key, secret, "mybucket"},
 		},
-		{
-			name: "s3 userinfo in endpoint",
-			raw: "s3://mybucket?endpoint=https://" +
-				key + ":" + secret + "@minio.example.com",
-			wantErr: storage.ErrURLCredentials,
-			secrets: []string{key, secret},
-		},
-		{
-			name:    "s3 userinfo in endpoint without scheme",
-			raw:     "s3://mybucket?endpoint=" + key + ":" + secret + "@minio.example.com",
-			wantErr: storage.ErrURLCredentials,
-			secrets: []string{key, secret},
-		},
 		{
 			name:    "s3 unknown query param",
 			raw:     "s3://mybucket?access_key=" + key + "&secret=" + secret,
@@ -216,27 +203,6 @@ func TestParseStorageURLRejectsCredentials(t *testing.T) {
 	}
 }
 
-// TestParseStorageURLRejectsRcloneRemoteParams checks that an rclone
-// remote name carrying parameters (rclone's "remote,param=value" form,
-// which can hold a credential) is rejected without echoing the value.
-func TestParseStorageURLRejectsRcloneRemoteParams(t *testing.T) {
-	t.Parallel()
-
-	const secret = "topsecret"
-
-	raw := "rclone://gdrive,client_secret=" + secret + "/backups"
-
-	_, err := storage.ParseStorageURL(raw)
-	if !errors.Is(err, storage.ErrRcloneRemoteParams) {
-		t.Fatalf("ParseStorageURL(%q) error = %v, want %v",
-			raw, err, storage.ErrRcloneRemoteParams)
-	}
-
-	if strings.Contains(err.Error(), secret) {
-		t.Errorf("error message leaked %q: %v", secret, err.Error())
-	}
-}
-
 // TestParseStorageURLParseFailureHidesURL checks that when url.Parse
 // itself fails, the wrapped error carries only the inner cause, not the
 // *url.Error whose text embeds the raw (possibly credential-bearing) URL.