Unknown subcommands of vault, unlocker, version and generate fail (closes #183)

Given a subcommand they do not have, `secret vault`, `secret unlocker`,
`secret version` and `secret generate` printed their help and exited 0,
so a mistyped command looked like a successful one. Each now exits 1
with `unknown command "<name>" for "secret <parent>"`, and with no
arguments still prints its help.

Cobra checks a command's Args only when it has a RunE, so each parent
gets `Args: cobra.NoArgs` and a RunE that prints its help. Args alone
changes nothing. TestNoVaultImportCommand required the old answer to
`secret vault import` and now requires the error.

Judgement call: their help now shows a `secret <parent> [flags]` usage
line, which cobra adds for any command that runs.
Judgement call: `secret vault help` now fails as an unknown subcommand;
`secret help vault` and `secret vault --help` still work.

Model: opus-5-5

README.md

@@ -204,6 +204,12 @@ Moves or renames a secret within the current vault.
 Lists all versions of a secret showing creation time, status, and validity
 period.
 
+`STATUS` is `current` for the secret's current version, `expired` for another
+version whose `NOT_AFTER` has passed, and `inactive` for any other version.
+Adding a version sets `NOT_AFTER` on the version that was current;
+`secret version promote` sets no times. A version that is not current therefore
+shows `inactive` when no version was added while it was current.
+
 #### `secret version promote <secret-name> <version>`
 
 Promotes a specific version to current by rewriting the secret's `current` file

internal/cli/create_vault_test.go

@@ -144,25 +144,20 @@ func TestVaultCreationLeavesNoSecretInEnvironment(t *testing.T) {
 // TestNoVaultImportCommand is a regression test for
 // https://git.eeqj.de/sneak/secret/issues/139: `secret vault import`
 // refused every vault that `secret vault create` makes, because each has a
-// long-term key. The command is gone, so cobra answers it with the help of
-// `secret vault`, as it does any subcommand of `secret vault` it does not
-// know.
+// long-term key. The command is gone, so it fails as any subcommand of
+// `secret vault` that does not exist does.
 func TestNoVaultImportCommand(t *testing.T) {
 	t.Setenv(secret.EnvStateDir, t.TempDir())
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+	t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
 
-	run := func(args ...string) string {
-		t.Setenv(secret.EnvMnemonic, testMnemonic)
-		t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
+	_, err := cli.ExecuteCommandInProcess(
+		strings.Fields("vault create work"), "", nil)
+	require.NoError(t, err)
 
-		output, err := cli.ExecuteCommandInProcess(args, "", nil)
-		require.NoError(t, err, "%v", args)
-
-		return output
-	}
-
-	run("vault", "create", "work")
-
-	require.Equal(t, run("vault"), run("vault", "import", "work"))
+	_, err = cli.ExecuteCommandInProcess(
+		strings.Fields("vault import work"), "", nil)
+	require.EqualError(t, err, `unknown command "import" for "secret vault"`)
 }
 
 // TestStopAtPassphrasePromptLeavesNothing is a regression test for the

internal/cli/generate.go

@@ -29,6 +29,12 @@ func newGenerateCmd() *cobra.Command {
 		Use:   "generate",
 		Short: "Generate random data",
 		Long:  `Generate various types of random data including mnemonics and secrets.`,
+		// Cobra checks Args only on a command that has a RunE; without
+		// one, an unknown subcommand prints this help and exits 0.
+		Args: cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return cmd.Help()
+		},
 	}
 
 	cmd.AddCommand(newGenerateMnemonicCmd())

internal/cli/unknown_subcommand_test.go

@@ -0,0 +1,58 @@
+package cli_test
+
+import (
+	"fmt"
+	"os/exec"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/secret"
+)
+
+// TestUnknownSubcommandFails is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/183: given a subcommand it does not
+// have, `secret vault`, `secret unlocker`, `secret version` and `secret
+// generate` printed their help and exited 0, so a script could not tell a
+// mistyped command from a successful one. Each runs here as the built binary
+// and must exit 1 naming the subcommand, as an unknown command does at the
+// top level, and must still print its help and exit 0 when given nothing.
+func TestUnknownSubcommandFails(t *testing.T) {
+	t.Parallel()
+
+	stateDir := t.TempDir()
+
+	// run runs the built binary with args and returns what it printed and
+	// its exit code
+	run := func(args ...string) (string, int) {
+		//nolint:gosec // G204: test executes the freshly built secret binary
+		cmd := exec.CommandContext(t.Context(), secretBinaryPath(t), args...)
+		cmd.Env = []string{secret.EnvStateDir + "=" + stateDir}
+
+		output, err := cmd.CombinedOutput()
+		if err != nil {
+			var exitErr *exec.ExitError
+			require.ErrorAs(t, err, &exitErr, "secret %v did not run", args)
+		}
+
+		return string(output), cmd.ProcessState.ExitCode()
+	}
+
+	for _, call := range []string{
+		"vault improt", "unlocker foo", "version foo", "generate foo",
+	} {
+		parent, unknown, _ := strings.Cut(call, " ")
+
+		output, exitCode := run(parent, unknown)
+		assert.Equal(t, 1, exitCode, "secret %s should fail:\n%s", call, output)
+		assert.Contains(t, output, fmt.Sprintf(
+			"unknown command %q for \"secret %s\"", unknown, parent))
+
+		output, exitCode = run(parent)
+		assert.Equal(t, 0, exitCode,
+			"secret %s should succeed:\n%s", parent, output)
+		assert.Contains(t, output, usageHeading,
+			"secret %s should print its help", parent)
+	}
+}

internal/cli/unlockers.go

@@ -126,6 +126,12 @@ func newUnlockerCmd() *cobra.Command {
 		Use:   "unlocker",
 		Short: "Manage unlockers",
 		Long:  `Create, list, and remove unlockers for the current vault.`,
+		// Cobra checks Args only on a command that has a RunE; without
+		// one, an unknown subcommand prints this help and exits 0.
+		Args: cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return cmd.Help()
+		},
 	}
 
 	cmd.AddCommand(newUnlockerListCmd())
@@ -314,7 +320,8 @@ func (cli *Instance) UnlockersList(jsonOutput bool) error {
 		return err
 	}
 
-	var unlockers []UnlockerInfo
+	// Not nil, so that a vault with no unlockers prints [] in JSON, not null
+	unlockers := make([]UnlockerInfo, 0, len(unlockerMetadata))
 
 	for _, unlockerID := range slices.Sorted(maps.Keys(unlockerMetadata)) {
 		metadata := unlockerMetadata[unlockerID]

internal/cli/unlockers_list_test.go

@@ -1,13 +1,14 @@
 // Unlocker List Tests
 //
-// Tests for `secret unlocker list` behavior when an unlocker's metadata
-// cannot be read or used:
+// Tests for `secret unlocker list`:
 //
 //   - TestUnlockersListToleratesCorruptMetadata: one unlocker's corrupt
 //     metadata does not stop the others from being listed.
 //   - TestUnlockersListSkipsUnreadableMetadata: an unlocker whose metadata
 //     file cannot be checked for or read is left out, and the other is
 //     still listed.
+//   - TestUnlockersListEmptyJSON: a vault with no unlockers lists them as
+//     an empty JSON list.
 
 //nolint:testpackage // white-box test of unexported internals
 package cli
@@ -321,3 +322,36 @@ func TestUnlockersListSkipsUnreadableMetadata(t *testing.T) {
 		})
 	}
 }
+
+// TestUnlockersListEmptyJSON is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/148: listing the unlockers of a
+// vault that has none, as after its last unlocker is removed, printed
+// "unlockers": null in JSON form instead of an empty list.
+func TestUnlockersListEmptyJSON(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	vaultDir := filepath.Join(listTestStateDir, "vaults.d", listTestVaultName)
+
+	require.NoError(t, afero.WriteFile(
+		fs, filepath.Join(listTestStateDir, "currentvault"),
+		[]byte(listTestVaultName), listTestFilePerm,
+	))
+	require.NoError(t, fs.MkdirAll(
+		filepath.Join(vaultDir, listTestUnlockersDirName), listTestDirPerm,
+	))
+
+	var buf bytes.Buffer
+
+	cmd := &cobra.Command{}
+	cmd.SetOut(&buf)
+
+	instance := &Instance{fs: fs, stateDir: listTestStateDir, cmd: cmd}
+	require.NoError(t, instance.UnlockersList(true))
+
+	var decoded map[string]any
+
+	require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded))
+	assert.Equal(t, []any{}, decoded["unlockers"],
+		"no unlockers must be printed as [], not %s", buf.String())
+}

internal/cli/vault.go

@@ -29,6 +29,12 @@ func newVaultCmd() *cobra.Command {
 		Use:   "vault",
 		Short: "Manage vaults",
 		Long:  `Create, list, and select vaults for organizing secrets.`,
+		// Cobra checks Args only on a command that has a RunE; without
+		// one, an unknown subcommand prints this help and exits 0.
+		Args: cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return cmd.Help()
+		},
 	}
 
 	cmd.AddCommand(newVaultListCmd())

internal/cli/vault_list_test.go

@@ -0,0 +1,42 @@
+package cli_test
+
+import (
+	"bytes"
+	"encoding/json"
+	"path/filepath"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/cli"
+	"sneak.berlin/go/secret/internal/secret"
+)
+
+// TestVaultListEmptyJSON is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/148: listing the vaults when
+// vaults.d holds none printed "vaults": null in JSON form instead of an
+// empty list.
+func TestVaultListEmptyJSON(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	require.NoError(t, fs.MkdirAll(
+		filepath.Join(testStateDir, "vaults.d"), secret.DirPerms,
+	))
+
+	var buf bytes.Buffer
+
+	cmd := &cobra.Command{}
+	cmd.SetOut(&buf)
+
+	c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
+	require.NoError(t, c.ListVaults(cmd, true))
+
+	var decoded map[string]any
+
+	require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded))
+	assert.Equal(t, []any{}, decoded["vaults"],
+		"no vaults must be printed as [], not %s", buf.String())
+}

internal/cli/version.go

@@ -43,6 +43,12 @@ func VersionCommands(cli *Instance) *cobra.Command {
 		Short: "Manage secret versions",
 		Long: "Commands for managing secret versions including listing, " +
 			"promoting, and retrieving specific versions",
+		// Cobra checks Args only on a command that has a RunE; without
+		// one, an unknown subcommand prints this help and exits 0.
+		Args: cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return cmd.Help()
+		},
 	}
 
 	// List versions command
@@ -229,9 +235,13 @@ func printVersionRow(
 	}
 
 	// Determine status
-	status := "expired"
-	if version == currentVersion {
+	status := "inactive"
+
+	switch {
+	case version == currentVersion:
 		status = "current"
+	case sv.Metadata.NotAfter != nil && sv.Metadata.NotAfter.Before(time.Now()):
+		status = "expired"
 	}
 
 	// Format timestamps

internal/cli/version_test.go

@@ -5,6 +5,8 @@
 // - TestListVersionsCommand: Tests `secret version list` command output
 // - TestListVersionsNonExistentSecret: Tests error handling for missing secrets
 // - TestPromoteVersionCommand: Tests `secret version promote` command
+// - TestListVersionsStatusAfterPromote: Tests each version's status after
+//   promoting an older version
 // - TestPromoteNonExistentVersion: Tests error handling for invalid promotion
 // - TestPromoteHelpSaysCurrentFileIsRewritten: Tests `secret version promote
 //   --help` text
@@ -244,6 +246,66 @@ func TestPromoteVersionCommand(t *testing.T) {
 	assert.Equal(t, []byte("version-1"), promoted.Bytes())
 }
 
+func TestListVersionsStatusAfterPromote(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
+	cli.Mnemonic = testMnemonicBuffer(t)
+
+	setupTestVault(t, fs)
+
+	vlt, err := vault.GetCurrentVault(fs, testStateDir)
+	require.NoError(t, err)
+
+	vlt.Mnemonic = cli.Mnemonic
+
+	addTestSecret(t, vlt, []byte("version-1"), false)
+	addTestSecret(t, vlt, []byte("version-2"), true)
+	addTestSecret(t, vlt, []byte("version-3"), true)
+
+	vaultDir, err := vlt.GetDirectory()
+	require.NoError(t, err)
+
+	// Newest first
+	versions, err := secret.ListVersions(fs,
+		filepath.Join(vaultDir, "secrets.d", "test%secret"))
+	require.NoError(t, err)
+	require.Len(t, versions, 3)
+
+	newest, middle, oldest := versions[0], versions[1], versions[2]
+
+	cmd := newRootCmd()
+
+	var buf bytes.Buffer
+	cmd.SetOut(&buf)
+	cmd.SetErr(&buf)
+
+	err = cli.PromoteVersion(cmd, "test/secret", oldest)
+	require.NoError(t, err)
+
+	buf.Reset()
+
+	err = cli.ListVersions(cmd, "test/secret")
+	require.NoError(t, err)
+
+	// STATUS is the fourth field of a row, since CREATED is a date and a time
+	statuses := map[string]string{}
+
+	for line := range strings.SplitSeq(buf.String(), "\n") {
+		fields := strings.Fields(line)
+		if len(fields) > 3 {
+			statuses[fields[0]] = fields[3]
+		}
+	}
+
+	assert.Equal(t, "current", statuses[oldest])
+	// Its NOT_AFTER was set when the newest version was added
+	assert.Equal(t, "expired", statuses[middle])
+	// It has no NOT_AFTER
+	assert.Equal(t, "inactive", statuses[newest])
+}
+
 func TestPromoteNonExistentVersion(t *testing.T) {
 	t.Parallel()
 

internal/secret/version.go

@@ -32,7 +32,8 @@ type VersionMetadata struct {
 	ID        string     `json:"id"`                  // ULID
 	CreatedAt *time.Time `json:"createdAt,omitempty"` // When version was created
 	NotBefore *time.Time `json:"notBefore,omitempty"` // When this version becomes active
-	NotAfter  *time.Time `json:"notAfter,omitempty"`  // Expiry (nil = current)
+	// NotAfter is set when another version is added while this one is current
+	NotAfter *time.Time `json:"notAfter,omitempty"`
 }
 
 // Version represents a version of a secret

internal/vault/management.go

@@ -140,8 +140,9 @@ func ListVaults(fs afero.Fs, stateDir string) ([]string, error) {
 		return nil, fmt.Errorf("failed to read vaults directory: %w", err)
 	}
 
-	// Extract vault names
-	var vaults []string
+	// Extract vault names. Not nil, so that no vaults prints [] in JSON,
+	// not null.
+	vaults := make([]string, 0, len(entries))
 
 	for _, entry := range entries {
 		if entry.IsDir() {