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/vaultik/destination_validation_test.go

@@ -161,14 +161,13 @@ func TestVerifyManifestBlobsRejectsShortHash(t *testing.T) {
 		Blobs: []snapshot.BlobInfo{{Hash: "abc", CompressedSize: 1}},
 	}
 
-	verified, missing, mismatched, missingSize, unchecked, err :=
+	verified, missing, mismatched, missingSize, err :=
 		v.verifyManifestBlobs(manifest, &VerifyOptions{JSON: true})
 	require.ErrorIs(t, err, errInvalidBlobHash)
 	require.Zero(t, verified)
 	require.Zero(t, missing)
 	require.Zero(t, mismatched)
 	require.Zero(t, missingSize)
-	require.Empty(t, unchecked)
 }
 
 // TestVerifyBlobChunksRejectsNegativeLength proves a blob_chunks row with a

internal/vaultik/shallow_verify_test.go

@@ -4,20 +4,15 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
-	"errors"
-	"fmt"
 	"io"
 	"os"
-	"path"
 	"path/filepath"
-	"slices"
 	"testing"
 
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/require"
 	"sneak.berlin/go/vaultik/internal/log"
 	"sneak.berlin/go/vaultik/internal/snapshot"
-	"sneak.berlin/go/vaultik/internal/storage"
 	"sneak.berlin/go/vaultik/internal/ui"
 	"sneak.berlin/go/vaultik/internal/vaultik"
 )
@@ -156,224 +151,6 @@ func TestShallowVerifyDetectsMissingDatabase(t *testing.T) {
 		"shallow verify must fail when db.zst.age is absent")
 }
 
-// errStatDenied stands in for any storage error other than not-found: a
-// permission error, an S3 403, a dropped connection.
-var errStatDenied = errors.New("injected stat failure")
-
-// statFailingStorer fails Stat with errStatDenied for the keys in
-// failKeys and passes every other call to the wrapped Storer.
-type statFailingStorer struct {
-	storage.Storer
-
-	failKeys []string
-}
-
-func (s statFailingStorer) Stat(
-	ctx context.Context, key string,
-) (*storage.ObjectInfo, error) {
-	if slices.Contains(s.failKeys, key) {
-		return nil, fmt.Errorf("stat %q: %w", key, errStatDenied)
-	}
-
-	return s.Storer.Stat(ctx, key)
-}
-
-// TestShallowVerifyReportsStatErrorsApartFromMissing backs up a real
-// snapshot, deletes one blob, and makes storage fail Stat with an error
-// other than not-found for a second blob and for the encrypted database.
-// Only the deleted blob may be counted as missing. The second blob and
-// the database must be reported as could not be checked, with the error
-// storage returned, and verify must fail, in the text output and under
-// --json.
-func TestShallowVerifyReportsStatErrorsApartFromMissing(t *testing.T) {
-	log.Initialize(log.Config{})
-	t.Parallel()
-
-	fs := afero.NewOsFs()
-	tempDir := t.TempDir()
-
-	dataDir := filepath.Join(tempDir, "source")
-	storeDir := filepath.Join(tempDir, "remote")
-	dbPath := filepath.Join(tempDir, "index.sqlite")
-
-	chunkSize := int64(32 * 1024)
-	maxBlobSize := int64(128 * 1024)
-
-	// Random content, so the snapshot spans several blobs.
-	require.NoError(t, fs.MkdirAll(dataDir, 0o755))
-	appendRandomBytes(t, fs, map[string][]byte{},
-		filepath.Join(dataDir, "data.bin"), maxBlobSize*4)
-
-	ctx := context.Background()
-
-	cfg, storer, snapshotID := runFileStorageBackup(
-		ctx, t, fs, dataDir, storeDir, dbPath, chunkSize, maxBlobSize)
-
-	blobKeys := storedBlobKeys(t, storeDir)
-	require.GreaterOrEqual(t, len(blobKeys), 2, "expected at least two blobs")
-
-	missingKey, uncheckedKey := blobKeys[0], blobKeys[1]
-	missingHash, uncheckedHash := path.Base(missingKey), path.Base(uncheckedKey)
-
-	require.NoError(t, os.Remove(filepath.Join(storeDir, missingKey)))
-
-	dbKey := "metadata/" + snapshot.RemoteSnapshotKey(snapshotID) + "/db.zst.age"
-	failing := statFailingStorer{
-		Storer:   storer,
-		failKeys: []string{uncheckedKey, dbKey},
-	}
-
-	newVerifier := func(out io.Writer) *vaultik.Vaultik {
-		v := &vaultik.Vaultik{
-			Config:  cfg,
-			Storage: failing,
-			Fs:      fs,
-			Stdout:  out,
-			Stderr:  io.Discard,
-			UI:      ui.NewWithColor(io.Discard, false),
-		}
-		v.SetContext(ctx)
-
-		return v
-	}
-
-	var out bytes.Buffer
-
-	err := newVerifier(&out).VerifySnapshotWithOptions(
-		snapshotID, &vaultik.VerifyOptions{JSON: true})
-	require.Error(t, err)
-
-	result := decodeVerifyResult(t, out.Bytes())
-	require.Equal(t, "failed", result.Status)
-	require.Equal(t, 1, result.Missing, "only the deleted blob is missing")
-	require.Equal(t, result.BlobCount-2, result.Verified)
-	require.Len(t, result.Unchecked, 1)
-	require.Equal(t, uncheckedHash, result.Unchecked[0].Hash)
-	require.Contains(t, result.Unchecked[0].Error, errStatDenied.Error())
-	require.False(t, result.DatabaseMissing,
-		"a database that could not be checked is not missing")
-	require.Contains(t, result.DatabaseCheckError, errStatDenied.Error())
-
-	out.Reset()
-	err = newVerifier(&out).VerifySnapshotWithOptions(
-		snapshotID, &vaultik.VerifyOptions{})
-	require.Error(t, err)
-
-	text := out.String()
-	require.Contains(t, text, "Missing: "+missingHash)
-	require.NotContains(t, text, "Missing: "+uncheckedHash)
-	require.Contains(t, text, "Could not check: "+uncheckedHash)
-	require.Contains(t, text, "Encrypted database:       could not be checked")
-	require.Contains(t, text, errStatDenied.Error())
-	require.Contains(t, text, "FAILED")
-}
-
-// TestShallowVerifyQuietFailsOnStatErrorAlone backs up a healthy snapshot,
-// then makes storage fail Stat, with an error other than not-found, for
-// one blob alone and then for the encrypted database alone. Each run must
-// fail verify, and under --quiet, which drops the report, the error
-// storage returned must still reach stderr.
-//
-//nolint:paralleltest // captureProcessStderr replaces os.Stderr
-func TestShallowVerifyQuietFailsOnStatErrorAlone(t *testing.T) {
-	log.Initialize(log.Config{})
-
-	fs := afero.NewOsFs()
-	tempDir := t.TempDir()
-
-	dataDir := filepath.Join(tempDir, "source")
-	storeDir := filepath.Join(tempDir, "remote")
-	dbPath := filepath.Join(tempDir, "index.sqlite")
-
-	chunkSize := int64(32 * 1024)
-	maxBlobSize := int64(128 * 1024)
-
-	require.NoError(t, fs.MkdirAll(dataDir, 0o755))
-	require.NoError(t, afero.WriteFile(fs,
-		filepath.Join(dataDir, "data.bin"),
-		bytesPattern("shallow-quiet-", int(maxBlobSize*2)), 0o644))
-
-	ctx := context.Background()
-
-	cfg, storer, snapshotID := runFileStorageBackup(
-		ctx, t, fs, dataDir, storeDir, dbPath, chunkSize, maxBlobSize)
-
-	newQuietVerifier := func(s storage.Storer) *vaultik.Vaultik {
-		v := &vaultik.Vaultik{
-			Config:  cfg,
-			Storage: s,
-			Fs:      fs,
-			Stdout:  io.Discard,
-			Stderr:  io.Discard,
-			UI:      ui.NewWithColor(io.Discard, false),
-		}
-		v.SetContext(ctx)
-		v.UI.SetQuiet(true)
-
-		return v
-	}
-
-	require.NoError(t,
-		newQuietVerifier(storer).VerifySnapshotWithOptions(
-			snapshotID, &vaultik.VerifyOptions{}),
-		"shallow verify should pass on a healthy snapshot")
-
-	blobKeys := storedBlobKeys(t, storeDir)
-	require.NotEmpty(t, blobKeys)
-
-	dbKey := "metadata/" + snapshot.RemoteSnapshotKey(snapshotID) + "/db.zst.age"
-
-	for _, failKey := range []string{blobKeys[0], dbKey} {
-		v := newQuietVerifier(statFailingStorer{
-			Storer:   storer,
-			failKeys: []string{failKey},
-		})
-
-		var err error
-
-		stderr := captureProcessStderr(t, func() {
-			err = v.VerifySnapshotWithOptions(
-				snapshotID, &vaultik.VerifyOptions{})
-		})
-
-		require.Error(t, err,
-			"verify must fail when %s could not be checked", failKey)
-		require.Contains(t, stderr, errStatDenied.Error(),
-			"under --quiet the error for %s must still reach stderr", failKey)
-	}
-}
-
-// storedBlobKeys returns the storage key of every blob file under
-// storeDir, sorted.
-func storedBlobKeys(t *testing.T, storeDir string) []string {
-	t.Helper()
-
-	var keys []string
-
-	err := filepath.Walk(filepath.Join(storeDir, "blobs"),
-		func(p string, info os.FileInfo, err error) error {
-			if err != nil {
-				return err
-			}
-
-			if info.IsDir() {
-				return nil
-			}
-
-			rel, err := filepath.Rel(storeDir, p)
-			if err != nil {
-				return err
-			}
-
-			keys = append(keys, filepath.ToSlash(rel))
-
-			return nil
-		})
-	require.NoError(t, err)
-
-	return keys
-}
-
 // growOneBlob appends bytes to the first blob file found under blobsDir,
 // changing its on-disk size so it no longer matches the manifest.
 func growOneBlob(t *testing.T, fs afero.Fs, blobsDir string) {

internal/vaultik/snapshot.go

@@ -14,7 +14,6 @@ import (
 
 	"sneak.berlin/go/vaultik/internal/log"
 	"sneak.berlin/go/vaultik/internal/snapshot"
-	"sneak.berlin/go/vaultik/internal/storage"
 	"sneak.berlin/go/vaultik/internal/types"
 )
 
@@ -775,10 +774,18 @@ func (v *Vaultik) VerifySnapshotWithOptions(
 		v.stdoutf("Checking blob presence and sizes...\n")
 	}
 
-	v.checkSnapshotDatabase(remoteKey, result)
+	// A snapshot is only restorable if its encrypted database is present
+	// alongside the blobs. Shallow verify checks that the object exists; it
+	// does not decrypt it (that is deep verify's job).
+	dbPath := fmt.Sprintf("metadata/%s/db.zst.age", remoteKey)
 
-	result.Verified, result.Missing, result.Mismatched, result.MissingSize,
-		result.Unchecked, err = v.verifyManifestBlobs(manifest, opts)
+	_, dbErr := v.Storage.Stat(v.ctx, dbPath)
+	if dbErr != nil {
+		result.DatabaseMissing = true
+	}
+
+	result.Verified, result.Missing, result.Mismatched, result.MissingSize, err =
+		v.verifyManifestBlobs(manifest, opts)
 	if err != nil {
 		if opts.JSON {
 			result.Status = verifyStatusFailed
@@ -815,46 +822,25 @@ func (v *Vaultik) printVerifyHeader(snapshotID string, opts *VerifyOptions) {
 	}
 }
 
-// checkSnapshotDatabase records in result whether the snapshot's
-// encrypted database is missing or could not be checked. A snapshot is
-// only restorable if its database is present alongside the blobs.
-// Shallow verify checks that the object exists; it does not decrypt it
-// (that is deep verify's job).
-func (v *Vaultik) checkSnapshotDatabase(remoteKey string, result *VerifyResult) {
-	dbPath := fmt.Sprintf("metadata/%s/db.zst.age", remoteKey)
-
-	_, err := v.Storage.Stat(v.ctx, dbPath)
-
-	switch {
-	case errors.Is(err, storage.ErrNotFound):
-		result.DatabaseMissing = true
-	case err != nil:
-		result.DatabaseCheckError = err.Error()
-	}
-}
-
 // verifyManifestBlobs checks that each blob in the manifest is present in
 // storage with the size the manifest records, returning the counts of
 // blobs that were present with the right size, absent, and present but the
-// wrong size, the total bytes of the absent blobs, and the blobs it could
-// not check because storage returned an error other than not-found. It
-// does not read blob contents; deep verification (RunDeepVerify) does
-// that. The size comparison matches the deep path (see
-// verifyBlobExistenceFromDB).
+// wrong size, plus the total bytes of the absent blobs. It does not read
+// blob contents; deep verification (RunDeepVerify) does that. The size
+// comparison matches the deep path (see verifyBlobExistenceFromDB).
 func (v *Vaultik) verifyManifestBlobs(
 	manifest *snapshot.Manifest, opts *VerifyOptions,
-) (int, int, int, int64, []UncheckedBlob, error) {
+) (int, int, int, int64, error) {
 	var (
 		verified, missing, mismatched int
 		missingSize                   int64
-		unchecked                     []UncheckedBlob
 	)
 
 	for _, blob := range manifest.Blobs {
 		// The manifest is unauthenticated, so its blob hashes are checked
 		// before being spliced into a storage path.
 		if !isBlobHash(blob.Hash) {
-			return 0, 0, 0, 0, nil, fmt.Errorf("%w: %s",
+			return 0, 0, 0, 0, fmt.Errorf("%w: %s",
 				errInvalidBlobHash, shortHash(blob.Hash))
 		}
 
@@ -863,7 +849,7 @@ func (v *Vaultik) verifyManifestBlobs(
 
 		stat, err := v.Storage.Stat(v.ctx, blobPath)
 		switch {
-		case errors.Is(err, storage.ErrNotFound):
+		case err != nil:
 			if !opts.JSON && !v.UI.Quiet() {
 				v.stdoutf("  Missing: %s (%s)\n",
 					blob.Hash, ubytes(blob.CompressedSize))
@@ -871,13 +857,6 @@ func (v *Vaultik) verifyManifestBlobs(
 
 			missing++
 			missingSize += blob.CompressedSize
-		case err != nil:
-			if !opts.JSON && !v.UI.Quiet() {
-				v.stdoutf("  Could not check: %s (%v)\n", blob.Hash, err)
-			}
-
-			unchecked = append(unchecked,
-				UncheckedBlob{Hash: blob.Hash, Error: err.Error()})
 		case stat.Size != blob.CompressedSize:
 			if !opts.JSON && !v.UI.Quiet() {
 				v.stdoutf("  Wrong size: %s (store has %s, manifest lists %s)\n",
@@ -890,7 +869,7 @@ func (v *Vaultik) verifyManifestBlobs(
 		}
 	}
 
-	return verified, missing, mismatched, missingSize, unchecked, nil
+	return verified, missing, mismatched, missingSize, nil
 }
 
 // formatVerifyResult outputs the final verification results as JSON or
@@ -911,19 +890,9 @@ func (v *Vaultik) formatVerifyResult(
 		return v.outputVerifyJSON(result)
 	}
 
-	// --quiet drops the report but not the errors storage returned, which
-	// are logged on stderr. A failure is still returned either way, and
-	// the cli layer prints it on stderr.
-	if v.UI.Quiet() {
-		for _, blob := range result.Unchecked {
-			log.Warn("Could not check blob", "blob", blob.Hash, "error", blob.Error)
-		}
-
-		if result.DatabaseCheckError != "" {
-			log.Warn("Could not check the encrypted database",
-				"snapshot", result.SnapshotID, "error", result.DatabaseCheckError)
-		}
-	} else {
+	// Under --quiet a failure is still returned, and the cli layer
+	// prints it on stderr.
+	if !v.UI.Quiet() {
 		v.printVerifySummary(result, failure)
 	}
 
@@ -949,19 +918,10 @@ func (v *Vaultik) printVerifySummary(result *VerifyResult, failure string) {
 		v.stdoutf("  Wrong size:               %d blobs\n", result.Mismatched)
 	}
 
-	if len(result.Unchecked) > 0 {
-		v.stdoutf("  Could not check:          %d blobs\n", len(result.Unchecked))
-	}
-
 	if result.DatabaseMissing {
 		v.stdoutf("  Encrypted database:       missing\n")
 	}
 
-	if result.DatabaseCheckError != "" {
-		v.stdoutf("  Encrypted database:       could not be checked (%s)\n",
-			result.DatabaseCheckError)
-	}
-
 	v.stdoutf("  Status:       ")
 
 	if failure != "" {
@@ -989,19 +949,10 @@ func shallowVerifyFailure(result *VerifyResult) string {
 			fmt.Sprintf("%d blobs have the wrong size", result.Mismatched))
 	}
 
-	if len(result.Unchecked) > 0 {
-		parts = append(parts, fmt.Sprintf("%d blobs could not be checked",
-			len(result.Unchecked)))
-	}
-
 	if result.DatabaseMissing {
 		parts = append(parts, "the encrypted database is missing")
 	}
 
-	if result.DatabaseCheckError != "" {
-		parts = append(parts, "the encrypted database could not be checked")
-	}
-
 	return strings.Join(parts, "; ")
 }
 

internal/vaultik/verify.go

@@ -57,25 +57,11 @@ type VerifyResult struct {
 	Missing     int    `json:"missing"`
 	MissingSize int64  `json:"missing_size,omitempty"`
 	Mismatched  int    `json:"mismatched,omitempty"`
-	// Unchecked lists the blobs shallow verify could not check, because
-	// storage answered with an error other than not-found, so whether
-	// they are present is unknown.
-	Unchecked []UncheckedBlob `json:"unchecked,omitempty"`
 	// DatabaseMissing is set by shallow verify when the snapshot's
 	// encrypted database (metadata/<key>/db.zst.age) is absent, which
 	// makes the snapshot unrestorable regardless of the blobs.
-	DatabaseMissing bool `json:"database_missing,omitempty"`
-	// DatabaseCheckError is set by shallow verify to the error storage
-	// returned when the encrypted database could not be checked.
-	DatabaseCheckError string `json:"database_check_error,omitempty"`
-	ErrorMessage       string `json:"error,omitempty"`
-}
-
-// UncheckedBlob is a blob shallow verify could not check, with the error
-// storage returned for it.
-type UncheckedBlob struct {
-	Hash  string `json:"hash"`
-	Error string `json:"error"`
+	DatabaseMissing bool   `json:"database_missing,omitempty"`
+	ErrorMessage    string `json:"error,omitempty"`
 }
 
 // deepVerifyFailure records a failure in the result and returns it appropriately