Stop vault safety checks from reading unreadable state as empty (closes #51)

Adding a PGP unlocker checked unlockers.d for a duplicate and, when the
directory or an unlocker's metadata file could not be read, reported no
duplicate and went on. The check now reads unlockers.d itself and stops
with an error naming the path and cause; `unlocker list` keeps skipping
unlockers it cannot read.

The same flaw guarded removing the last unlocker and removing a vault
(an unreadable secrets directory counted as no secrets) and vault
import (an unreadable pub.age counted as no long-term key). Those now
stop with an error too.

`vault rm` and `unlocker rm` now take the state directory lock and call
an unexported function that does the work, as `vault import` does.

Model: opus-5-5

TODO.md

@@ -25,6 +25,15 @@ Bring the repo into policy compliance in one commit:
 
 # Completed Steps
 
+- 2026-10-04: `secret init` refuses when the default vault exists, and
+  `secret vault create NAME` when `NAME` does, with "vault NAME already
+  exists", before writing anything. The check is in `vault.CreateVault`,
+  which both commands call while holding the state directory lock, so two
+  creates of one vault at once cannot both pass the check. Before, either
+  command replaced the vault's metadata, passphrase unlocker and
+  `longterm.age`, so none of its secrets could be decrypted any more. Both
+  commands now ask for the unlocker passphrase before creating the vault,
+  so one stopped at that prompt leaves no vault behind.
 - 2026-10-04: The `internal/cli` tests are back to about their time
   before the state directory lock
   (https://git.eeqj.de/sneak/secret/issues/80). The test that each
@@ -78,9 +87,9 @@ Bring the repo into policy compliance in one commit:
       has one, and to a PGP, keychain or Secure Enclave unlocker added
       on the same host and day as another of its type
       (https://git.eeqj.de/sneak/secret/issues/71);
-    - from `vault create` stopped at the passphrase prompt, a new vault
-      with no unlocker that is already the current vault; from `init`
-      stopped there, the default vault with no unlocker;
+    - from `init` or `vault create` killed after the passphrase prompt
+      but before the unlocker is written, a vault with no unlocker,
+      which `vault create` has already made the current vault;
     - from an unlocker add stopped before its metadata is written, a
       directory that `unlocker list` warns about and `unlocker rm`
       cannot remove;
@@ -89,6 +98,13 @@ Bring the repo into policy compliance in one commit:
       being removed, encrypted keys included. Nothing deletes it; it
       must be deleted by hand
       (https://git.eeqj.de/sneak/secret/issues/75).
+- 2026-10-03: The checks run before changing a vault now stop with an
+  error naming the path and cause when they cannot read what they
+  inspect, instead of reading the failure as "nothing there": the
+  duplicate check before `unlocker add pgp` (an unreadable
+  `unlockers.d` or unlocker metadata file), the secret count that
+  guards removing the last unlocker and removing a vault, and the
+  existing long-term key check before `vault import`.
 - 2026-10-03: `version rm`, `version promote` and `get --version`
   accept a version only if it is one of the versions `version list`
   lists for that secret, compared as typed before any path is built

internal/cli/create_vault_test.go

@@ -0,0 +1,148 @@
+package cli_test
+
+import (
+	"testing"
+
+	"git.eeqj.de/sneak/secret/internal/cli"
+	"git.eeqj.de/sneak/secret/internal/secret"
+	"git.eeqj.de/sneak/secret/internal/vault"
+	"github.com/awnumar/memguard"
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/require"
+)
+
+// TestCreateExistingVaultChangesNothing is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/74, where running `secret init`
+// a second time, or `secret vault create` with the name of an existing
+// vault, replaced that vault's keys, so that none of its secrets could be
+// decrypted any more. Each must refuse, change nothing, and leave every
+// vault's secret readable through its passphrase unlocker.
+//
+//nolint:paralleltest // t.Setenv forbids parallel subtests
+func TestCreateExistingVaultChangesNothing(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+	t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
+
+	// `secret init`, `secret vault create work`, `secret vault select
+	// default`, and the secret "x" in each vault. "work" is then not the
+	// current vault, which creating it again must not change.
+	fs := afero.NewMemMapFs()
+	c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
+	cmd := &cobra.Command{}
+
+	require.NoError(t, c.Init(cmd))
+	require.NoError(t, c.CreateVault(cmd, "work"))
+	require.NoError(t, c.SelectVault(cmd, "default"))
+
+	vaults, err := vault.ListVaults(fs, testStateDir)
+	require.NoError(t, err)
+	require.Len(t, vaults, 2)
+
+	for _, name := range vaults {
+		value := memguard.NewBufferFromBytes([]byte("value"))
+		err := vault.NewVault(fs, testStateDir, name).AddSecret("x", value, false)
+		require.NoError(t, err)
+	}
+
+	before := snapshotStateDir(t, fs)
+
+	tests := []struct {
+		command string
+		want    string
+		run     func(c *cli.Instance) error
+	}{
+		{
+			"init",
+			"failed to create default vault: vault default already exists",
+			func(c *cli.Instance) error { return c.Init(cmd) },
+		},
+		{
+			"vault create default",
+			"vault default already exists",
+			func(c *cli.Instance) error { return c.CreateVault(cmd, "default") },
+		},
+		{
+			"vault create work",
+			"vault work already exists",
+			func(c *cli.Instance) error { return c.CreateVault(cmd, "work") },
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.command, func(t *testing.T) {
+			fs := newFsFromSnapshot(t, before)
+
+			err := tt.run(cli.NewCLIInstanceWithStateDir(fs, testStateDir))
+
+			require.EqualError(t, err, tt.want)
+			require.Equal(t, before, snapshotStateDir(t, fs))
+		})
+	}
+
+	// Every case left the state directory exactly as recorded in before, so
+	// reading each vault's secret once from it shows that it still decrypts
+	// after each case. Without the mnemonic, reading a secret goes through
+	// the vault's passphrase unlocker, which is slow.
+	t.Setenv(secret.EnvMnemonic, "")
+
+	for _, name := range vaults {
+		value, err := vault.NewVault(fs, testStateDir, name).GetSecret("x")
+		require.NoError(t, err)
+		require.Equal(t, "value", string(value))
+	}
+}
+
+// TestStopAtPassphrasePromptLeavesNothing is a regression test for the
+// review of https://git.eeqj.de/sneak/secret/pulls/82: `secret init` or
+// `secret vault create` stopped at the passphrase prompt left a vault with
+// no unlocker, which neither command would then create again. Each must ask
+// for the passphrase before writing anything.
+//
+//nolint:paralleltest // t.Setenv forbids parallel subtests
+func TestStopAtPassphrasePromptLeavesNothing(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	// Without the passphrase in the environment, both commands prompt for
+	// it, which fails because the tests do not run in a terminal.
+	t.Setenv(secret.EnvUnlockPassphrase, "")
+
+	// An empty state directory for `secret init`, and one holding the vault
+	// "default" for `secret vault create work`.
+	empty := afero.NewMemMapFs()
+	require.NoError(t, empty.MkdirAll(testStateDir, secret.DirPerms))
+
+	withDefault := afero.NewMemMapFs()
+	_, err := vault.CreateVault(withDefault, testStateDir, "default")
+	require.NoError(t, err)
+
+	cmd := &cobra.Command{}
+
+	tests := []struct {
+		command string
+		fs      afero.Fs
+		run     func(c *cli.Instance) error
+	}{
+		{
+			"init",
+			empty,
+			func(c *cli.Instance) error { return c.Init(cmd) },
+		},
+		{
+			"vault create work",
+			withDefault,
+			func(c *cli.Instance) error { return c.CreateVault(cmd, "work") },
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.command, func(t *testing.T) {
+			before := snapshotStateDir(t, tt.fs)
+
+			err := tt.run(cli.NewCLIInstanceWithStateDir(tt.fs, testStateDir))
+
+			require.ErrorContains(t, err, "failed to read passphrase")
+			require.Equal(t, before, snapshotStateDir(t, tt.fs))
+		})
+	}
+}

internal/cli/init.go

@@ -160,6 +160,14 @@ func (cli *Instance) initialize(cmd *cobra.Command) error {
 			errInvalidMnemonicPhrase)
 	}
 
+	// Ask for the unlocker passphrase before creating the vault, so that
+	// stopping at the prompt leaves no vault without an unlocker behind
+	passphraseBuffer, err := resolvePassphrase()
+	if err != nil {
+		return err
+	}
+	defer passphraseBuffer.Destroy()
+
 	// Set mnemonic in environment for CreateVault to use
 	restoreMnemonicEnv := setMnemonicEnv(mnemonicStr)
 	defer restoreMnemonicEnv()
@@ -175,13 +183,6 @@ func (cli *Instance) initialize(cmd *cobra.Command) error {
 	// Unlock the vault with the derived long-term key
 	vlt.Unlock(ltIdentity)
 
-	// Prompt for passphrase for unlocker
-	passphraseBuffer, err := resolvePassphrase()
-	if err != nil {
-		return err
-	}
-	defer passphraseBuffer.Destroy()
-
 	// Create passphrase-protected unlocker
 	secret.Debug("Creating passphrase-protected unlocker")
 

internal/cli/lock_test.go

@@ -271,11 +271,12 @@ func stateDirModTimes(t *testing.T, fs afero.Fs) map[string]int64 {
 }
 
 // setupEveryCommand makes what each command in
-// TestChangingCommandsWaitForLock needs: the current vault "default" with
-// two versions of "test/secret", the vault "other" without a long-term key,
-// for vault import, and the file testInput. If withUnlocker is set, it also
-// gives "default" a passphrase unlocker, which is slow. It returns the older
-// version and the unlocker's ID.
+// TestChangingCommandsWaitForLock needs: the current vault "work" with two
+// versions of "test/secret", the vault "other" without a long-term key, for
+// vault import, and the file testInput. There is no vault "default", which
+// init creates. If withUnlocker is set, it also gives "work" a passphrase
+// unlocker, which is slow. It returns the older version and the unlocker's
+// ID.
 func setupEveryCommand(
 	t *testing.T, fs afero.Fs, withUnlocker bool,
 ) (string, string) {
@@ -288,7 +289,7 @@ func setupEveryCommand(
 	require.NoError(t, err)
 	require.NoError(t, fs.Remove(filepath.Join(otherDir, "pub.age")))
 
-	vlt, err := vault.CreateVault(fs, testStateDir, "default")
+	vlt, err := vault.CreateVault(fs, testStateDir, "work")
 	require.NoError(t, err)
 
 	addTestSecret(t, vlt, []byte("older"), false)

internal/cli/unlockers.go

@@ -49,7 +49,6 @@ var (
 		"is already added as an unlocker")
 	errUnsupportedUnlockerType = errors.New("unsupported unlocker type")
 	errLastUnlocker            = errors.New("refusing to remove last unlocker")
-	errUnlockerExists          = errors.New("unlocker already exists")
 )
 
 // UnlockerInfo represents unlocker information for display
@@ -695,8 +694,15 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error {
 	// Check if this GPG key is already added
 	expectedID := "pgp-" + fingerprint
 
-	err = cli.checkUnlockerExists(vlt, expectedID)
+	exists, err := cli.checkUnlockerExists(vlt, expectedID)
 	if err != nil {
+		return fmt.Errorf(
+			"could not check whether GPG key %s is already an unlocker: %w",
+			gpgKeyID, err,
+		)
+	}
+
+	if exists {
 		return fmt.Errorf("GPG key %s %w", gpgKeyID, errGPGKeyAlreadyUnlocker)
 	}
 
@@ -714,7 +720,8 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error {
 	return nil
 }
 
-// UnlockersRemove removes an unlocker with safety checks
+// UnlockersRemove removes an unlocker, holding the state directory lock
+// while removeUnlocker runs
 func (cli *Instance) UnlockersRemove(
 	unlockerID string, force bool, cmd *cobra.Command,
 ) error {
@@ -724,6 +731,13 @@ func (cli *Instance) UnlockersRemove(
 	}
 	defer release()
 
+	return cli.removeUnlocker(unlockerID, force, cmd)
+}
+
+// removeUnlocker removes an unlocker with safety checks
+func (cli *Instance) removeUnlocker(
+	unlockerID string, force bool, cmd *cobra.Command,
+) error {
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -788,44 +802,65 @@ func (cli *Instance) UnlockerSelect(unlockerID string) error {
 	return vlt.SelectUnlocker(unlockerID)
 }
 
-// checkUnlockerExists checks if an unlocker with the given ID exists
-func (cli *Instance) checkUnlockerExists(vlt *vault.Vault, unlockerID string) error {
-	// Get the list of unlockers and check if any match the ID
-	unlockers, err := vlt.ListUnlockers()
-	if err != nil {
-		secret.Warn("Could not list unlockers during duplicate check", "error", err)
-
-		return nil // If we can't list unlockers, assume it doesn't exist
-	}
-
-	// Get vault directory to construct unlocker instances
+// checkUnlockerExists reports whether the vault already has an unlocker
+// with the given ID. It returns an error, and no answer, when unlockers.d
+// or an unlocker's metadata file cannot be read; the caller must then not
+// create the unlocker. It reads unlockers.d itself because
+// vault.ListUnlockers skips an unlocker it cannot read, which suits
+// `unlocker list` but not this check: the skipped unlocker may be the
+// duplicate. A directory whose metadata file is missing or corrupt is not
+// a working unlocker and is passed over.
+func (cli *Instance) checkUnlockerExists(
+	vlt *vault.Vault, unlockerID string,
+) (bool, error) {
 	vaultDir, err := vlt.GetDirectory()
 	if err != nil {
-		secret.Warn("Could not get vault directory during duplicate check",
-			"error", err)
-
-		return nil
+		return false, fmt.Errorf("failed to get vault directory: %w", err)
 	}
 
-	// Check each unlocker's ID
 	unlockersDir := filepath.Join(vaultDir, "unlockers.d")
 
-	for _, metadata := range unlockers {
-		// Construct the unlocker matching this metadata to get its ID
-		id, err := findUnlockerIDByMetadata(cli.fs, unlockersDir, metadata, true)
-		if err != nil {
-			secret.Warn(
-				"Could not read unlockers directory during duplicate check, "+
-					"skipping unlocker",
-				"unlockers_dir", unlockersDir, "error", err)
+	entries, err := afero.ReadDir(cli.fs, unlockersDir)
+	if errors.Is(err, os.ErrNotExist) {
+		return false, nil
+	}
 
+	if err != nil {
+		return false, fmt.Errorf(
+			"failed to read unlockers directory %s: %w", unlockersDir, err,
+		)
+	}
+
+	for _, entry := range entries {
+		if !entry.IsDir() {
 			continue
 		}
 
-		if id != "" && id == unlockerID {
-			return errUnlockerExists
+		unlockerDir := filepath.Join(unlockersDir, entry.Name())
+
+		metadataBytes, err := afero.ReadFile(
+			cli.fs, filepath.Join(unlockerDir, "unlocker-metadata.json"))
+		if errors.Is(err, os.ErrNotExist) {
+			continue
+		}
+
+		if err != nil {
+			return false, fmt.Errorf(
+				"failed to read metadata of unlocker %s: %w", unlockerDir, err,
+			)
+		}
+
+		var metadata secret.UnlockerMetadata
+
+		err = json.Unmarshal(metadataBytes, &metadata)
+		if err != nil {
+			continue
+		}
+
+		if unlockerIDFromDir(cli.fs, unlockerDir, metadata, true) == unlockerID {
+			return true, nil
 		}
 	}
 
-	return nil
+	return false, nil
 }

internal/cli/unreadable_dir_test.go

@@ -0,0 +1,353 @@
+// Unreadable Directory Tests
+//
+// The checks that guard adding a PGP unlocker (is this key already an
+// unlocker?), removing the last unlocker and removing a vault (does the
+// vault hold secrets?), and importing a mnemonic (does the vault already
+// have a long-term key?) each look at the vault on disk before acting.
+// When that look fails they must refuse to act, not read the failure as
+// "nothing there" and go ahead.
+//
+// The tests make the look fail with a wrapper around the in-memory
+// filesystem, which the state directory lock refuses. So they call the
+// function each command runs once it holds the lock, such as removeVault
+// for RemoveVault.
+
+//nolint:testpackage // white-box test of unexported internals
+package cli
+
+import (
+	"context"
+	"errors"
+	"io"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"git.eeqj.de/sneak/secret/internal/secret"
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+const (
+	// unreadableTestGPGUserID is the user ID of the throwaway GPG key the
+	// PGP unlocker tests generate, and the --keyid they pass.
+	unreadableTestGPGUserID = "unlocker-test@example.com"
+
+	// unreadableTestSecretName is the secret stored in the vaults the
+	// removal tests remove from.
+	unreadableTestSecretName = "api-key"
+
+	// unreadableTestOtherVault is a second vault for the vault removal
+	// test, since the last vault can never be removed.
+	unreadableTestOtherVault = "work"
+
+	// unreadableTestSecretsDirName is the directory holding a vault's
+	// secrets, and unreadableTestCurrentFileName the per-secret file
+	// naming its current version.
+	unreadableTestSecretsDirName  = "secrets.d"
+	unreadableTestCurrentFileName = "current"
+)
+
+// errStatFailed is returned by statFailFs in place of a successful stat.
+var errStatFailed = errors.New("input/output error")
+
+// statFailFs fails every Stat of one path, as an I/O or permission error
+// on that path would.
+type statFailFs struct {
+	afero.Fs
+
+	path string
+}
+
+func (f *statFailFs) Stat(name string) (os.FileInfo, error) {
+	if name == f.path {
+		return nil, errStatFailed
+	}
+
+	return f.Fs.Stat(name)
+}
+
+// errOpenFailed is returned by openFailFs in place of a successful open.
+var errOpenFailed = errors.New("permission denied")
+
+// openFailFs fails every Open of one path, as a directory without read
+// permission does: checking that it exists succeeds, listing it fails.
+type openFailFs struct {
+	afero.Fs
+
+	path string
+}
+
+//nolint:ireturn // afero.File is the interface required by afero.Fs
+func (f *openFailFs) Open(name string) (afero.File, error) {
+	if name == f.path {
+		return nil, errOpenFailed
+	}
+
+	return f.Fs.Open(name)
+}
+
+// testVaultDir returns the directory of the named vault in the synthetic
+// state directory built by newListTestVault.
+func testVaultDir(vaultName string) string {
+	return filepath.Join(listTestStateDir, "vaults.d", vaultName)
+}
+
+// newTestInstance returns a CLI instance on fs whose output is discarded.
+func newTestInstance(fs afero.Fs) (*Instance, *cobra.Command) {
+	cmd := &cobra.Command{}
+	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
+
+	return &Instance{fs: fs, stateDir: listTestStateDir, cmd: cmd}, cmd
+}
+
+// assertDirEntries asserts that dir holds exactly the named entries.
+func assertDirEntries(t *testing.T, fs afero.Fs, dir string, want ...string) {
+	t.Helper()
+
+	entries, err := afero.ReadDir(fs, dir)
+	require.NoError(t, err)
+
+	names := make([]string, 0, len(entries))
+	for _, entry := range entries {
+		names = append(names, entry.Name())
+	}
+
+	assert.ElementsMatch(t, want, names)
+}
+
+// newTestGPGKey points GNUPGHOME at a fresh directory, generates a GPG key
+// without a passphrase there, and returns the key's fingerprint.
+func newTestGPGKey(t *testing.T) string {
+	t.Helper()
+
+	// Not t.TempDir(): on macOS its path is too long for the gpg-agent
+	// socket, which is created inside GNUPGHOME there.
+	gnupgHome, err := os.MkdirTemp("", "gpg") //nolint:usetesting // short path
+	require.NoError(t, err)
+
+	t.Cleanup(func() { _ = os.RemoveAll(gnupgHome) })
+	t.Setenv("GNUPGHOME", gnupgHome)
+
+	t.Cleanup(func() {
+		// Stop the gpg-agent that key generation starts; cleanups run in
+		// reverse order, so this happens before its directory is removed.
+		// t.Context is already canceled when cleanup runs.
+		ctx := context.WithoutCancel(t.Context())
+		_ = exec.CommandContext(ctx, "gpgconf", "--kill", "gpg-agent").Run()
+	})
+
+	output, err := exec.CommandContext(t.Context(), "gpg", "--batch",
+		"--pinentry-mode", "loopback", "--passphrase", "",
+		"--quick-gen-key", unreadableTestGPGUserID, "ed25519", "sign", "never",
+	).CombinedOutput()
+	require.NoError(t, err, "generating the test GPG key: %s", output)
+
+	fingerprint, err := secret.ResolveGPGKeyFingerprint(unreadableTestGPGUserID)
+	require.NoError(t, err)
+
+	return fingerprint
+}
+
+// addTestPGPUnlocker runs `secret unlocker add pgp` for the test key
+// against fs.
+func addTestPGPUnlocker(fs afero.Fs) error {
+	instance, cmd := newTestInstance(fs)
+	cmd.Flags().String("keyid", unreadableTestGPGUserID, "")
+
+	return instance.addPGPUnlocker(cmd)
+}
+
+// TestAddPGPUnlockerDuplicateCheck asserts that adding a PGP unlocker for
+// a key that already has one fails, and creates no unlocker directory,
+// when unlockers.d or the existing unlocker's metadata file cannot be
+// read; and, as the control case, that the existing unlocker is refused
+// as a duplicate when everything can be read.
+//
+//nolint:paralleltest // t.Setenv (GNUPGHOME) forbids parallel tests
+func TestAddPGPUnlockerDuplicateCheck(t *testing.T) {
+	fingerprint := newTestGPGKey(t)
+	unlockersDir := filepath.Join(
+		testVaultDir(listTestVaultName), listTestUnlockersDirName)
+	duplicateDir := filepath.Join(unlockersDir, listTestUnlockerDirTwo)
+
+	// newVaultWithDuplicate returns a vault holding an unlocker for the
+	// test key, beside the one newListTestVault writes.
+	newVaultWithDuplicate := func(t *testing.T) afero.Fs {
+		t.Helper()
+
+		base := newListTestVault(t, 1)
+		writePGPUnlocker(t, base, unlockersDir, listTestUnlockerDirTwo,
+			time.Date(2026, time.August, 10, 12, 30, 0, 0, time.UTC),
+			fingerprint)
+
+		return base
+	}
+
+	tests := []struct {
+		name    string
+		failFs  func(base afero.Fs) afero.Fs
+		wantErr error
+		// wantPath is the path the error must name.
+		wantPath string
+	}{
+		{
+			name: "unlockers.d unreadable",
+			failFs: func(base afero.Fs) afero.Fs {
+				return &unlockersDirFailFs{Fs: base}
+			},
+			wantErr:  errUnlockersDirUnreadable,
+			wantPath: unlockersDir,
+		},
+		{
+			name: "existing unlocker's metadata unreadable",
+			failFs: func(base afero.Fs) afero.Fs {
+				return &metadataReadFailFs{
+					Fs: base,
+					unreadablePath: filepath.Join(
+						duplicateDir, listTestMetadataFileName),
+				}
+			},
+			wantErr:  errMetadataUnreadable,
+			wantPath: duplicateDir,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			base := newVaultWithDuplicate(t)
+
+			err := addTestPGPUnlocker(tt.failFs(base))
+
+			require.ErrorIs(t, err, tt.wantErr)
+			require.NotErrorIs(t, err, errGPGKeyAlreadyUnlocker)
+			assert.Contains(t, err.Error(), tt.wantPath,
+				"the error must name what it could not read")
+			assertDirEntries(t, base, unlockersDir,
+				listTestUnlockerDirOne, listTestUnlockerDirTwo)
+		})
+	}
+
+	t.Run("duplicate refused", func(t *testing.T) {
+		base := newVaultWithDuplicate(t)
+
+		err := addTestPGPUnlocker(base)
+
+		require.ErrorIs(t, err, errGPGKeyAlreadyUnlocker)
+		assertDirEntries(t, base, unlockersDir,
+			listTestUnlockerDirOne, listTestUnlockerDirTwo)
+	})
+}
+
+// writeTestSecret stores a secret with a current-version pointer, which is
+// what makes it count as a secret, in the given vault directory.
+func writeTestSecret(t *testing.T, fs afero.Fs, vaultDir string) {
+	t.Helper()
+
+	secretDir := filepath.Join(
+		vaultDir, unreadableTestSecretsDirName, unreadableTestSecretName)
+	require.NoError(t, fs.MkdirAll(secretDir, listTestDirPerm))
+	require.NoError(t, afero.WriteFile(fs,
+		filepath.Join(secretDir, unreadableTestCurrentFileName),
+		[]byte("20260809.001"), listTestFilePerm))
+}
+
+// TestRemoveLastUnlockerAbortsWhenSecretsUnreadable asserts that the last
+// unlocker is kept when the secrets it protects cannot be counted.
+func TestRemoveLastUnlockerAbortsWhenSecretsUnreadable(t *testing.T) {
+	t.Parallel()
+
+	vaultDir := testVaultDir(listTestVaultName)
+	unlockersDir := filepath.Join(vaultDir, listTestUnlockersDirName)
+	secretsDir := filepath.Join(vaultDir, unreadableTestSecretsDirName)
+
+	for _, path := range []string{
+		secretsDir,
+		filepath.Join(secretsDir, unreadableTestSecretName,
+			unreadableTestCurrentFileName),
+	} {
+		t.Run(filepath.Base(path), func(t *testing.T) {
+			t.Parallel()
+
+			base := newListTestVault(t, 1)
+			writeTestSecret(t, base, vaultDir)
+			instance, cmd := newTestInstance(&statFailFs{Fs: base, path: path})
+
+			err := instance.removeUnlocker(
+				"pgp-"+listTestGPGKeyID+"A", false, cmd)
+
+			require.ErrorIs(t, err, errStatFailed)
+			assertDirEntries(t, base, unlockersDir, listTestUnlockerDirOne)
+		})
+	}
+}
+
+// TestRemoveVaultAbortsWhenSecretsDirUnreadable asserts that a vault is
+// kept when whether it holds secrets cannot be determined: when checking
+// that secrets.d exists fails, and when it exists but cannot be listed.
+func TestRemoveVaultAbortsWhenSecretsDirUnreadable(t *testing.T) {
+	t.Parallel()
+
+	vaultDir := testVaultDir(unreadableTestOtherVault)
+	secretsDir := filepath.Join(vaultDir, unreadableTestSecretsDirName)
+
+	tests := []struct {
+		name    string
+		failFs  func(base afero.Fs) afero.Fs
+		wantErr error
+	}{
+		{
+			name: "check fails",
+			failFs: func(base afero.Fs) afero.Fs {
+				return &statFailFs{Fs: base, path: secretsDir}
+			},
+			wantErr: errStatFailed,
+		},
+		{
+			name: "listing fails",
+			failFs: func(base afero.Fs) afero.Fs {
+				return &openFailFs{Fs: base, path: secretsDir}
+			},
+			wantErr: errOpenFailed,
+		},
+	}
+
+	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.failFs(base))
+
+			err := instance.removeVault(cmd, unreadableTestOtherVault, false)
+
+			require.ErrorIs(t, err, tt.wantErr)
+
+			exists, err := afero.DirExists(base, vaultDir)
+			require.NoError(t, err)
+			assert.True(t, exists, "the vault must not be removed")
+		})
+	}
+}
+
+// TestVaultImportAbortsWhenPubKeyUnreadable asserts that a mnemonic import
+// stops when whether the vault already has a long-term key cannot be
+// determined.
+func TestVaultImportAbortsWhenPubKeyUnreadable(t *testing.T) {
+	t.Parallel()
+
+	base := newListTestVault(t, 1)
+	instance, cmd := newTestInstance(&statFailFs{
+		Fs: base, path: filepath.Join(testVaultDir(listTestVaultName), "pub.age"),
+	})
+
+	err := instance.importMnemonic(cmd, listTestVaultName)
+
+	require.ErrorIs(t, err, errStatFailed)
+}

internal/cli/vault.go

@@ -309,6 +309,14 @@ func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
 		return errInvalidMnemonicPhrase
 	}
 
+	// Ask for the unlocker passphrase before creating the vault, so that
+	// stopping at the prompt leaves no vault without an unlocker behind
+	passphraseBuffer, err := resolvePassphrase()
+	if err != nil {
+		return err
+	}
+	defer passphraseBuffer.Destroy()
+
 	// Set mnemonic in environment for CreateVault to use
 	restoreMnemonicEnv := setMnemonicEnv(mnemonicStr)
 	defer restoreMnemonicEnv()
@@ -336,13 +344,6 @@ func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
 	// Unlock the vault with the derived long-term key
 	vlt.Unlock(ltIdentity)
 
-	// Get or prompt for passphrase
-	passphraseBuffer, err := resolvePassphrase()
-	if err != nil {
-		return err
-	}
-	defer passphraseBuffer.Destroy()
-
 	// Create passphrase-protected unlocker
 	secret.Debug("Creating passphrase-protected unlocker")
 
@@ -400,8 +401,12 @@ func (cli *Instance) vaultImportPreflight(
 	// Check if vault already has a public key
 	pubKeyPath := vaultDir + "/pub.age"
 
-	_, err = cli.fs.Stat(pubKeyPath)
-	if err == nil {
+	exists, err = afero.Exists(cli.fs, pubKeyPath)
+	if err != nil {
+		return "", "", "", fmt.Errorf("failed to check %s: %w", pubKeyPath, err)
+	}
+
+	if exists {
 		return "", "", "", fmt.Errorf("vault '%s' %w",
 			vaultName, errVaultHasLongTermKey)
 	}
@@ -561,17 +566,26 @@ func (cli *Instance) importMnemonic(cmd *cobra.Command, vaultName string) error
 }
 
 // vaultHasSecrets reports whether the vault directory contains any secrets
-func (cli *Instance) vaultHasSecrets(vaultDir string) bool {
+func (cli *Instance) vaultHasSecrets(vaultDir string) (bool, error) {
 	secretsDir := filepath.Join(vaultDir, "secrets.d")
 
-	exists, _ := afero.DirExists(cli.fs, secretsDir)
+	exists, err := afero.DirExists(cli.fs, secretsDir)
+	if err != nil {
+		return false, fmt.Errorf("failed to check secrets directory %s: %w",
+			secretsDir, err)
+	}
+
 	if !exists {
-		return false
+		return false, nil
 	}
 
 	entries, err := afero.ReadDir(cli.fs, secretsDir)
+	if err != nil {
+		return false, fmt.Errorf("failed to read secrets directory %s: %w",
+			secretsDir, err)
+	}
 
-	return err == nil && len(entries) > 0
+	return len(entries) > 0, nil
 }
 
 // switchAwayFromVault selects another vault as current before removal
@@ -600,7 +614,8 @@ func (cli *Instance) switchAwayFromVault(
 	return nil
 }
 
-// RemoveVault removes a vault with safety checks
+// RemoveVault removes a vault, holding the state directory lock while
+// removeVault runs
 func (cli *Instance) RemoveVault(cmd *cobra.Command, name string, force bool) error {
 	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
 	if err != nil {
@@ -608,6 +623,11 @@ func (cli *Instance) RemoveVault(cmd *cobra.Command, name string, force bool) er
 	}
 	defer release()
 
+	return cli.removeVault(cmd, name, force)
+}
+
+// removeVault removes a vault with safety checks
+func (cli *Instance) removeVault(cmd *cobra.Command, name string, force bool) error {
 	// Get list of all vaults
 	vaults, err := vault.ListVaults(cli.fs, cli.stateDir)
 	if err != nil {
@@ -641,7 +661,10 @@ func (cli *Instance) RemoveVault(cmd *cobra.Command, name string, force bool) er
 	}
 
 	// Check if vault has secrets
-	hasSecrets := cli.vaultHasSecrets(vaultDir)
+	hasSecrets, err := cli.vaultHasSecrets(vaultDir)
+	if err != nil {
+		return err
+	}
 
 	// Require --force if vault has secrets
 	if hasSecrets && !force {

internal/vault/errors.go

@@ -26,6 +26,10 @@ var (
 	// as "vault <name> does not exist".
 	ErrVaultNotFound = errors.New("does not exist")
 
+	// ErrVaultExists indicates that a vault to be created already exists.
+	// Composed as "vault <name> already exists".
+	ErrVaultExists = errors.New("already exists")
+
 	// ErrNilValueBuffer indicates a nil value buffer was supplied.
 	ErrNilValueBuffer = errors.New("value buffer is nil")
 

internal/vault/management.go

@@ -191,7 +191,11 @@ func processMnemonicForVault(
 	return derivationIndex, publicKeyHash, familyHash, nil
 }
 
-// CreateVault creates a new vault
+// CreateVault creates a new vault and selects it as the current vault. It
+// refuses a vault that already exists before writing anything: creating it
+// again would replace its keys, and its secrets could no longer be
+// decrypted. The commands that call it hold the state directory lock, so no
+// other command can create the vault between the check and the writes.
 func CreateVault(fs afero.Fs, stateDir string, name string) (*Vault, error) {
 	secret.Debug("Creating new vault", "name", name, "state_dir", stateDir)
 
@@ -207,12 +211,22 @@ func CreateVault(fs afero.Fs, stateDir string, name string) (*Vault, error) {
 
 	secret.Debug("Vault name validation passed", "vault_name", name)
 
-	// Create vault directory structure
 	vaultDir := filepath.Join(stateDir, "vaults.d", name)
+
+	exists, err := afero.DirExists(fs, vaultDir)
+	if err != nil {
+		return nil, fmt.Errorf("failed to check if vault exists: %w", err)
+	}
+
+	if exists {
+		return nil, fmt.Errorf("vault %s %w", name, ErrVaultExists)
+	}
+
+	// Create vault directory structure
 	secret.Debug("Creating vault directory structure", "vault_dir", vaultDir)
 
 	// Create main vault directory
-	err := fs.MkdirAll(vaultDir, secret.DirPerms)
+	err = fs.MkdirAll(vaultDir, secret.DirPerms)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create vault directory: %w", err)
 	}

internal/vault/vault.go

@@ -138,7 +138,12 @@ func (v *Vault) NumSecrets() (int, error) {
 
 	secretsDir := filepath.Join(vaultDir, "secrets.d")
 
-	exists, _ := afero.DirExists(v.fs, secretsDir)
+	exists, err := afero.DirExists(v.fs, secretsDir)
+	if err != nil {
+		return 0, fmt.Errorf("failed to check secrets directory %s: %w",
+			secretsDir, err)
+	}
+
 	if !exists {
 		return 0, nil
 	}
@@ -162,7 +167,7 @@ func (v *Vault) NumSecrets() (int, error) {
 
 		exists, err := afero.Exists(v.fs, currentFile)
 		if err != nil {
-			continue // Skip directories we can't read
+			return 0, fmt.Errorf("failed to check %s: %w", currentFile, err)
 		}
 
 		if exists {