Stop vault creation when a vault's metadata cannot be read (closes #140)

GetNextDerivationIndex passed over a vault whose vault-metadata.json could
not be read or parsed, so the next vault made from the same mnemonic could
get that vault's derivation index and so the same long-term key. It now
returns an error naming the file, so vault create, vault import and init
stop with that error and leave the state directory as it was. A vault
directory with no metadata file is still skipped.

The regression test breaks one vault's metadata, once with content that
does not parse and once with a test filesystem that fails to open it, and
requires CreateVault with the same mnemonic to fail, naming the file, and
leave no new vault.

Model: opus-5-5

README.md

@@ -204,12 +204,6 @@ 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
@@ -550,11 +544,11 @@ secret decrypt encryption/mykey --input document.txt.age --output document.txt
 ### Vault Management
 
 - **Derivation Index**: Each vault uses a unique derivation index from the
-  mnemonic, and thus a unique key pair; creating a vault (`secret init` and
+  mnemonic, and thus a unique key pair; creating a vault or importing a mnemonic
-  `secret vault create`) fails, with an error naming the file, when any vault's
+  fails, with an error naming the file, when any vault's `vault-metadata.json`
-  `vault-metadata.json` exists but cannot be read or parsed; a vault directory
+  exists but cannot be read or parsed; a vault directory with no
-  with no `vault-metadata.json` is skipped, so the index it uses can be given to
+  `vault-metadata.json` is skipped, so the index it uses can be given to a new
-  a new vault
+  vault
 - **Public Key Hash**: Double SHA-256 hash of the vault's public key; the same
   hash of the index-0 public key identifies vaults from the same mnemonic
 - **Automatic Key Derivation**: When creating vaults with a mnemonic, keys are

internal/cli/unlockers.go

@@ -314,8 +314,7 @@ func (cli *Instance) UnlockersList(jsonOutput bool) error {
 		return err
 	}
 
-	// Not nil, so that a vault with no unlockers prints [] in JSON, not null
+	var unlockers []UnlockerInfo
-	unlockers := make([]UnlockerInfo, 0, len(unlockerMetadata))
 
 	for _, unlockerID := range slices.Sorted(maps.Keys(unlockerMetadata)) {
 		metadata := unlockerMetadata[unlockerID]

internal/cli/unlockers_list_test.go

@@ -1,14 +1,13 @@
 // Unlocker List Tests
 //
-// Tests for `secret unlocker list`:
+// Tests for `secret unlocker list` behavior when an unlocker's metadata
+// cannot be read or used:
 //
 //   - 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
@@ -322,36 +321,3 @@ 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_list_test.go

@@ -1,42 +0,0 @@
-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

@@ -229,13 +229,9 @@ func printVersionRow(
 	}
 
 	// Determine status
-	status := "inactive"
+	status := "expired"
-
+	if version == currentVersion {
-	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,8 +5,6 @@
 // - 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
@@ -246,66 +244,6 @@ 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,8 +32,7 @@ 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 is set when another version is added while this one is current
+	NotAfter  *time.Time `json:"notAfter,omitempty"`  // Expiry (nil = current)
-	NotAfter *time.Time `json:"notAfter,omitempty"`
 }
 
 // Version represents a version of a secret

internal/vault/management.go

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