Leave no partial unlocker directory when adding an unlocker fails (closes #48)

CreatePGPUnlocker looked up the GPG key's fingerprint, and the keychain
unlocker got the long-term key, only after writing part of the unlocker,
so a failure there left a directory with no metadata. Both now do every
step that can fail before writing anything. `secret unlocker add pgp`
looks the fingerprint up once, for its duplicate check, and passes it to
CreatePGPUnlocker to record. All four unlocker types write their files
through the new secret.WriteDir, which builds a new directory in a
temporary directory, renames it into place when complete and removes it
on a failure. A directory that already exists, as when an unlocker
replaces one of the same name, is written in place and never removed.

Model: opus-5-5

README.md

@@ -198,9 +198,7 @@ Creates a new unlocker of the specified type:
 
 **DANGER**: Permanently removes an unlocker. Like Unix `rm`, this command
 does not ask for confirmation.  Cannot remove the last unlocker if the vault
-has secrets unless --force is used. An unlocker directory that
+has secrets unless --force is used.
-`secret unlocker list` skips with a warning, because its metadata cannot be
-read or parsed, is removed by the directory name the warning gives.
 - `--force, -f`: Force removal of last unlocker even if vault has secrets
 - **CRITICAL WARNING**: Without unlockers and without your mnemonic phrase,
   vault data will be PERMANENTLY INACCESSIBLE

TODO.md

@@ -30,31 +30,12 @@ Bring the repo into policy compliance in one commit:
   `secret unlocker add pgp` resolves the GPG key's fingerprint once, for
   its duplicate check, and passes it to `CreatePGPUnlocker` to record.
   `CreatePGPUnlocker` and `CreateKeychainUnlocker` get the long-term key
-  and encrypt everything before writing anything. All four unlocker
+  and encrypt everything before writing anything. All four unlocker types write their files
-  types write their files through `secret.WriteDir`: a new unlocker is
+  through `secret.WriteDir`: a new unlocker is built in a temporary
-  built in a temporary directory, renamed into place when complete and
+  directory, renamed into place when complete and removed on a failure.
-  removed on a failure. One added under the directory name of an
+  One added under the directory name of an existing unlocker is still
-  existing unlocker is still written into that directory in place
+  written into that directory in place
   (https://git.eeqj.de/sneak/secret/issues/71).
-- 2026-10-04: `secret unlocker select` and `secret unlocker remove`
-  skip, with the warning `unlocker list` gives, an unlocker directory
-  whose metadata file cannot be checked for, read or parsed, instead of
-  failing when it sorts before the unlocker asked for. Such a directory,
-  or one without a metadata file, is removed by its directory name, the
-  name the warning gives; only the directory is removed, since its type
-  is unknown. Removing one whose metadata file is missing or corrupt
-  never counts as removing the last unlocker. Removing one whose metadata
-  file cannot be checked for or read always does, since it may be the
-  only working unlocker, so in a vault with secrets it needs `--force`.
-- 2026-10-04: A failed command prints its error once, without the usage
-  text after it (https://git.eeqj.de/sneak/secret/issues/41). Usage is
-  still printed for a command called wrongly: wrong number of arguments,
-  unknown flag, bad flag value, missing required flag, or flags that
-  break a flag group (mutually exclusive, required together, one
-  required). The root command's `PersistentPreRunE` turns usage off.
-  Cobra checks arguments and flag values before that hook but required
-  flags and flag groups only after it, so the hook checks those two
-  first. Root `SilenceUsage` would have hidden usage for all of these.
 - 2026-10-04: `secret get` keeps the secret in locked memory until it
   writes it out (https://git.eeqj.de/sneak/secret/issues/37):
   `Vault.GetSecret` and `Vault.GetSecretVersion` return a
@@ -251,6 +232,8 @@ Bring the repo into policy compliance in one commit:
       209-216); non-constant-time public key compare (vault.go:95-100).
 - High priority:
     - Secure temporary file handling and cleanup.
+    - Print cobra usage only for argument errors, not internal
+      failures.
     - Initialize a default unlock key at vault creation.
     - Confirmation prompts for destructive operations (keys rm, vault
       deletion).

internal/cli/root.go

@@ -46,30 +46,9 @@ func newRootCmd() *cobra.Command {
 		Short: "A simple secrets manager",
 		Long: `A simple secrets manager to store and retrieve sensitive ` +
 			`information securely.`,
-		// Cobra prints the error a command returns; Entry does not.
+		// Ensure usage is shown after errors
+		SilenceUsage:  false,
 		SilenceErrors: false,
-		// Usage belongs only to a command called wrongly. Cobra has
-		// checked its arguments and flag values before this runs, but
-		// checks required flags (ValidateRequiredFlags) and flag groups
-		// (ValidateFlagGroups) only after it, so both are checked here
-		// to keep usage for them. An error after that comes from running
-		// the command, and usage would only bury it. A subcommand that
-		// sets its own PersistentPreRun replaces this one.
-		PersistentPreRunE: func(cmd *cobra.Command, _ []string) error {
-			err := cmd.ValidateRequiredFlags()
-			if err != nil {
-				return err
-			}
-
-			err = cmd.ValidateFlagGroups()
-			if err != nil {
-				return err
-			}
-
-			cmd.SilenceUsage = true
-
-			return nil
-		},
 	}
 
 	secret.Debug("Adding subcommands to root command")

internal/cli/unlockers.go

@@ -746,43 +746,14 @@ func (cli *Instance) removeUnlocker(
 		return err
 	}
 
-	// Get list of unlockers. It leaves out a directory whose metadata file
+	// Get list of unlockers
-	// is missing or cannot be checked for, read or parsed.
 	unlockers, err := vlt.ListUnlockers()
 	if err != nil {
 		return fmt.Errorf("failed to list unlockers: %w", err)
 	}
 
-	vaultDir, err := vlt.GetDirectory()
-	if err != nil {
-		return fmt.Errorf("failed to get vault directory: %w", err)
-	}
-
-	unlockersDir := filepath.Join(vaultDir, "unlockers.d")
-
 	// Check if we're removing the last unlocker
-	removingLast := false
-
 	if len(unlockers) == 1 {
-		lastID, err := findUnlockerIDByMetadata(
-			cli.fs, unlockersDir, unlockers[0], true)
-		if err != nil {
-			return err
-		}
-
-		removingLast = lastID == unlockerID
-	}
-
-	// unlockerID may instead name a directory left out of the list. If its
-	// metadata file is missing or corrupt it is not a working unlocker, so
-	// removing it never removes the last one. If the file cannot be checked
-	// for or read, the unlocker may be the only working one, so removing it
-	// counts as removing the last unlocker.
-	if metadataUnreadable(cli.fs, filepath.Join(unlockersDir, unlockerID)) {
-		removingLast = true
-	}
-
-	if removingLast {
 		// Check if vault has secrets
 		numSecrets, err := vlt.NumSecrets()
 		if err != nil {
@@ -816,20 +787,6 @@ func (cli *Instance) removeUnlocker(
 	return nil
 }
 
-// metadataUnreadable reports whether checking for or reading the metadata
-// file in the unlocker directory unlockerDir fails. A missing file is not
-// a failure.
-func metadataUnreadable(fs afero.Fs, unlockerDir string) bool {
-	metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json")
-
-	exists, err := afero.Exists(fs, metadataPath)
-	if err == nil && exists {
-		_, err = afero.ReadFile(fs, metadataPath)
-	}
-
-	return err != nil
-}
-
 // UnlockerSelect selects an unlocker as current
 func (cli *Instance) UnlockerSelect(unlockerID string) error {
 	release, err := vault.LockStateDir(cli.fs, cli.stateDir)

internal/cli/unlockers_corrupt_test.go

@@ -1,167 +0,0 @@
-// Corrupt Unlocker Tests
-//
-// `secret unlocker select` and `secret unlocker remove` find an unlocker
-// by its ID. These tests give the first unlocker, which sorts before the
-// one the commands act on, metadata that is not JSON, and check that the
-// commands step past it, and that it can itself be removed by its
-// directory name, which `secret unlocker list` names in its warning. A
-// last test checks that an unlocker whose metadata file cannot be read is
-// removed by its directory name only as the last unlocker is.
-
-//nolint:testpackage // white-box test of unexported internals
-package cli
-
-import (
-	"path/filepath"
-	"testing"
-
-	"git.eeqj.de/sneak/secret/internal/vault"
-	"github.com/spf13/afero"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-// newCorruptUnlockerVault returns the two-unlocker test vault with the
-// metadata of the first unlocker replaced by text that is not JSON.
-func newCorruptUnlockerVault(t *testing.T) *afero.MemMapFs {
-	t.Helper()
-
-	fs := newListTestVault(t, 2)
-	require.NoError(t, afero.WriteFile(fs,
-		filepath.Join(testVaultDir(listTestVaultName), listTestUnlockersDirName,
-			listTestUnlockerDirOne, listTestMetadataFileName),
-		[]byte("not json"), listTestFilePerm))
-
-	return fs
-}
-
-// TestUnlockerSelectSkipsCorruptUnlocker asserts that the second unlocker
-// can be selected, and that the corrupt one, having no type to be used as,
-// cannot be selected by its directory name.
-func TestUnlockerSelectSkipsCorruptUnlocker(t *testing.T) {
-	t.Parallel()
-
-	fs := newCorruptUnlockerVault(t)
-	instance, _ := newTestInstance(fs)
-
-	require.NoError(t, instance.UnlockerSelect("pgp-"+listTestGPGKeyID+"B"))
-
-	current, err := afero.ReadFile(fs,
-		filepath.Join(testVaultDir(listTestVaultName), "current-unlocker"))
-	require.NoError(t, err)
-	assert.Equal(t, listTestUnlockerDirTwo, string(current))
-
-	err = instance.UnlockerSelect(listTestUnlockerDirOne)
-	require.ErrorIs(t, err, vault.ErrUnlockerNotFound)
-}
-
-// TestUnlockerRemoveWithCorruptUnlocker asserts that the second unlocker
-// can be removed, unless the vault holds secrets: the corrupt unlocker
-// cannot unlock the vault, so the second is its last. The corrupt one can
-// be removed by its directory name without --force even then.
-func TestUnlockerRemoveWithCorruptUnlocker(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		name        string
-		unlockerID  string
-		withSecret  bool
-		wantErr     error
-		wantEntries []string
-	}{
-		{
-			name:        "the other unlocker",
-			unlockerID:  "pgp-" + listTestGPGKeyID + "B",
-			wantEntries: []string{listTestUnlockerDirOne},
-		},
-		{
-			name:       "the other unlocker, the last one, with secrets",
-			unlockerID: "pgp-" + listTestGPGKeyID + "B",
-			withSecret: true,
-			wantErr:    errLastUnlocker,
-			wantEntries: []string{
-				listTestUnlockerDirOne, listTestUnlockerDirTwo,
-			},
-		},
-		{
-			name:        "the corrupt unlocker by its directory name",
-			unlockerID:  listTestUnlockerDirOne,
-			withSecret:  true,
-			wantEntries: []string{listTestUnlockerDirTwo},
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-
-			fs := newCorruptUnlockerVault(t)
-			if tt.withSecret {
-				writeTestSecret(t, fs, testVaultDir(listTestVaultName))
-			}
-
-			instance, cmd := newTestInstance(fs)
-
-			err := instance.UnlockersRemove(tt.unlockerID, false, cmd)
-			require.ErrorIs(t, err, tt.wantErr)
-
-			assertDirEntries(t, fs,
-				filepath.Join(testVaultDir(listTestVaultName),
-					listTestUnlockersDirName),
-				tt.wantEntries...)
-		})
-	}
-}
-
-// TestUnlockerRemoveWithUnreadableMetadata asserts that removing the only
-// unlocker of a vault with secrets by its directory name, when its
-// metadata file cannot be checked for or read, is refused without --force:
-// listing leaves it out, but it may still be the vault's only working
-// unlocker. With --force it is removed. The state directory lock refuses
-// the failing filesystem, so the test calls removeUnlocker, which
-// UnlockersRemove runs once it holds the lock.
-func TestUnlockerRemoveWithUnreadableMetadata(t *testing.T) {
-	t.Parallel()
-
-	vaultDir := testVaultDir(listTestVaultName)
-	unlockersDir := filepath.Join(vaultDir, listTestUnlockersDirName)
-	failingPath := filepath.Join(unlockersDir, listTestUnlockerDirOne,
-		listTestMetadataFileName)
-
-	tests := []struct {
-		name string
-		wrap func(base afero.Fs) afero.Fs
-	}{
-		{
-			name: "checking for the file fails",
-			wrap: func(base afero.Fs) afero.Fs {
-				return &metadataStatFailFs{Fs: base, uncheckablePath: failingPath}
-			},
-		},
-		{
-			name: "reading the file fails",
-			wrap: func(base afero.Fs) afero.Fs {
-				return &metadataReadFailFs{Fs: base, unreadablePath: failingPath}
-			},
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-
-			base := newListTestVault(t, 1)
-			writeTestSecret(t, base, vaultDir)
-
-			instance, cmd := newTestInstance(tt.wrap(base))
-
-			err := instance.removeUnlocker(listTestUnlockerDirOne, false, cmd)
-			require.ErrorIs(t, err, errLastUnlocker)
-			assertDirEntries(t, base, unlockersDir, listTestUnlockerDirOne)
-
-			require.NoError(t,
-				instance.removeUnlocker(listTestUnlockerDirOne, true, cmd))
-			assertDirEntries(t, base, unlockersDir)
-		})
-	}
-}

internal/cli/usage_test.go

@@ -1,46 +0,0 @@
-package cli_test
-
-import (
-	"strings"
-	"testing"
-
-	"git.eeqj.de/sneak/secret/internal/cli"
-	"git.eeqj.de/sneak/secret/internal/secret"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-// usageHeading starts the usage text cobra prints after an error.
-const usageHeading = "Usage:"
-
-// A command called wrongly gets usage after its error; a command that
-// fails while running gets its error alone. Either way the command fails
-// and its error is shown exactly once.
-//
-//nolint:paralleltest // executes the CLI in-process and sets the environment
-func TestUsageOnlyForCallErrors(t *testing.T) {
-	// No vault in the state directory, so `get x` fails while running.
-	env := map[string]string{secret.EnvStateDir: t.TempDir()}
-
-	tests := []struct {
-		call      string
-		wantUsage bool
-	}{
-		{call: "get", wantUsage: true},
-		{call: "get x y", wantUsage: true},
-		{call: "get --no-such-flag x", wantUsage: true},
-		{call: "generate secret x --length abc", wantUsage: true},
-		{call: "import x", wantUsage: true},
-		{call: "get x", wantUsage: false},
-	}
-
-	for _, tt := range tests {
-		output, err := cli.ExecuteCommandInProcess(strings.Fields(tt.call), "", env)
-		require.Error(t, err, "%q should fail", tt.call)
-
-		assert.Equal(t, 1, strings.Count(output, err.Error()),
-			"%q should show its error once:\n%s", tt.call, output)
-		assert.Equal(t, tt.wantUsage, strings.Contains(output, usageHeading),
-			"usage shown for %q:\n%s", tt.call, output)
-	}
-}

internal/vault/unlockers.go

@@ -126,11 +126,7 @@ func (v *Vault) resolveUnlockerDirectory(currentUnlockerPath string) (string, er
 }
 
 // findUnlockerByID finds an unlocker by its ID and returns the unlocker
-// instance and its directory path. A directory that ListUnlockers skips is
+// instance and its directory path
-// skipped here too, with the same warning. Such a directory has no ID: if
-// no unlocker has the ID unlockerID but such a directory is named
-// unlockerID, that directory is returned with a nil unlocker, so that
-// RemoveUnlocker can remove it.
 //
 //nolint:ireturn // returns one of several concrete unlocker implementations
 func (v *Vault) findUnlockerByID(
@@ -141,24 +137,42 @@ func (v *Vault) findUnlockerByID(
 		return nil, "", fmt.Errorf("failed to read unlockers directory: %w", err)
 	}
 
-	skippedDirPath := ""
-
 	for _, file := range files {
 		if !file.IsDir() {
 			continue
 		}
 
-		unlockerDirPath := filepath.Join(unlockersDir, file.Name())
+		// Read metadata file
+		metadataPath := filepath.Join(unlockersDir, file.Name(), "unlocker-metadata.json")
 
-		metadata, ok := v.readUnlockerMetadataOrWarn(unlockersDir, file.Name())
+		exists, err := afero.Exists(v.fs, metadataPath)
-		if !ok {
+		if err != nil {
-			if file.Name() == unlockerID {
+			return nil, "", fmt.Errorf(
-				skippedDirPath = unlockerDirPath
+				"failed to check if metadata exists for unlocker %s: %w",
-			}
+				file.Name(), err)
+		}
 
+		if !exists {
+			// Skip directories without metadata - they might not be unlockers
 			continue
 		}
 
+		metadataBytes, err := afero.ReadFile(v.fs, metadataPath)
+		if err != nil {
+			return nil, "", fmt.Errorf(
+				"failed to read metadata for unlocker %s: %w", file.Name(), err)
+		}
+
+		var metadata UnlockerMetadata
+
+		err = json.Unmarshal(metadataBytes, &metadata)
+		if err != nil {
+			return nil, "", fmt.Errorf(
+				"failed to parse metadata for unlocker %s: %w", file.Name(), err)
+		}
+
+		unlockerDirPath := filepath.Join(unlockersDir, file.Name())
+
 		// Create the appropriate unlocker instance
 		var tempUnlocker secret.Unlocker
 
@@ -181,7 +195,7 @@ func (v *Vault) findUnlockerByID(
 		}
 	}
 
-	return nil, skippedDirPath, nil
+	return nil, "", nil
 }
 
 // ListUnlockers returns a list of available unlockers for this vault
@@ -212,12 +226,44 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) {
 	var unlockers []UnlockerMetadata
 
 	for _, file := range files {
-		if !file.IsDir() {
+		if file.IsDir() {
-			continue
+			// Read metadata file
-		}
+			metadataPath := filepath.Join(unlockersDir, file.Name(),
+				"unlocker-metadata.json")
+
+			exists, err := afero.Exists(v.fs, metadataPath)
+			if err != nil {
+				secret.Warn("Skipping unlocker directory whose metadata file cannot be checked",
+					"directory", file.Name(), "error", err)
+
+				continue
+			}
+
+			if !exists {
+				secret.Warn("Skipping unlocker directory with missing metadata file",
+					"directory", file.Name())
+
+				continue
+			}
+
+			metadataBytes, err := afero.ReadFile(v.fs, metadataPath)
+			if err != nil {
+				secret.Warn("Skipping unlocker directory with unreadable metadata file",
+					"directory", file.Name(), "error", err)
+
+				continue
+			}
+
+			var metadata UnlockerMetadata
+
+			err = json.Unmarshal(metadataBytes, &metadata)
+			if err != nil {
+				secret.Warn("Skipping unlocker directory with corrupt metadata file",
+					"directory", file.Name(), "error", err)
+
+				continue
+			}
 
-		metadata, ok := v.readUnlockerMetadataOrWarn(unlockersDir, file.Name())
-		if ok {
 			unlockers = append(unlockers, metadata)
 		}
 	}
@@ -225,54 +271,7 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) {
 	return unlockers, nil
 }
 
-// readUnlockerMetadataOrWarn reads the metadata of the unlocker directory
+// RemoveUnlocker removes an unlocker from this vault
-// name in unlockersDir. If the metadata file cannot be checked for, is
-// missing, or cannot be read or parsed, it warns, naming the directory,
-// and returns false: the caller skips that directory.
-func (v *Vault) readUnlockerMetadataOrWarn(
-	unlockersDir, name string,
-) (UnlockerMetadata, bool) {
-	metadataPath := filepath.Join(unlockersDir, name, "unlocker-metadata.json")
-
-	var metadata UnlockerMetadata
-
-	exists, err := afero.Exists(v.fs, metadataPath)
-	if err != nil {
-		secret.Warn("Skipping unlocker directory whose metadata file cannot be checked",
-			"directory", name, "error", err)
-
-		return metadata, false
-	}
-
-	if !exists {
-		secret.Warn("Skipping unlocker directory with missing metadata file",
-			"directory", name)
-
-		return metadata, false
-	}
-
-	metadataBytes, err := afero.ReadFile(v.fs, metadataPath)
-	if err != nil {
-		secret.Warn("Skipping unlocker directory with unreadable metadata file",
-			"directory", name, "error", err)
-
-		return metadata, false
-	}
-
-	err = json.Unmarshal(metadataBytes, &metadata)
-	if err != nil {
-		secret.Warn("Skipping unlocker directory with corrupt metadata file",
-			"directory", name, "error", err)
-
-		return metadata, false
-	}
-
-	return metadata, true
-}
-
-// RemoveUnlocker removes an unlocker from this vault. An unlocker
-// directory that ListUnlockers skips is removed by its directory name; its
-// type is unknown, so only the directory is removed.
 func (v *Vault) RemoveUnlocker(unlockerID string) error {
 	vaultDir, err := v.GetDirectory()
 	if err != nil {
@@ -283,17 +282,13 @@ func (v *Vault) RemoveUnlocker(unlockerID string) error {
 	unlockersDir := filepath.Join(vaultDir, "unlockers.d")
 
 	// Find the unlocker by ID
-	unlocker, unlockerDir, err := v.findUnlockerByID(unlockersDir, unlockerID)
+	unlocker, _, err := v.findUnlockerByID(unlockersDir, unlockerID)
 	if err != nil {
 		return err
 	}
 
-	if unlockerDir == "" {
-		return fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound)
-	}
-
 	if unlocker == nil {
-		return secret.RemoveDirAtomic(v.fs, unlockerDir)
+		return fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound)
 	}
 
 	// Use the unlocker's Remove method
@@ -311,14 +306,12 @@ func (v *Vault) SelectUnlocker(unlockerID string) error {
 	unlockersDir := filepath.Join(vaultDir, "unlockers.d")
 
 	// Find the unlocker by ID
-	unlocker, targetUnlockerDir, err := v.findUnlockerByID(unlockersDir, unlockerID)
+	_, targetUnlockerDir, err := v.findUnlockerByID(unlockersDir, unlockerID)
 	if err != nil {
 		return err
 	}
 
-	// A directory found without an unlocker is one ListUnlockers skips; it
+	if targetUnlockerDir == "" {
-	// cannot be selected.
-	if unlocker == nil {
 		return fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound)
 	}