Run the checks again on every script/cibuild (closes #54)

On an unchanged tree docker served every check step of the Dockerfile
from its build cache, so a second script/cibuild ran no lint, tests or
build and still succeeded.

script/cibuild now passes the current time as the CHECK_EPOCH build
argument. The lint and build stages each declare it after their module
download and before `COPY . .`. A build argument whose value changes
makes every RUN step after its declaration miss the cache, so the
checks run on each build while the base images, the apk install and the
module downloads stay cached.

Model: opus-5-5

Dockerfile

@@ -6,9 +6,9 @@ WORKDIR /src
 COPY go.mod go.sum ./
 RUN go mod download
 
-# script/cibuild sets CHECK_EPOCH to the current time, so every step below
+# script/cibuild sets CHECK_EPOCH to the current time, so the RUN steps
-# runs again on each build, an unchanged tree included, while the steps
+# below run again on each build, an unchanged tree included, while the
-# above stay cached. ARG is per stage: the build stage declares it too.
+# steps above stay cached. ARG is per stage: the build stage declares it too.
 ARG CHECK_EPOCH
 
 COPY . .
@@ -30,7 +30,7 @@ WORKDIR /build
 COPY go.mod go.sum ./
 RUN go mod download
 
-# As in the lint stage: the steps below run again on each script/cibuild.
+# As in the lint stage: the RUN steps below run again on each script/cibuild.
 ARG CHECK_EPOCH
 
 COPY . .

README.md

@@ -198,7 +198,9 @@ 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.
+has secrets unless --force is used. An unlocker directory that
+`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

@@ -29,10 +29,40 @@ Bring the repo into policy compliance in one commit:
   tree (https://git.eeqj.de/sneak/secret/issues/54). It passes the
   current time as the `CHECK_EPOCH` build argument, which both the lint
   and the build stage of the `Dockerfile` declare after their module
-  download, so every step from `COPY . .` on runs on each build while
+  download, so the `RUN` steps below the argument run again on each
-  the base images and module downloads stay cached. Before, a second run
+  build while the base images and module downloads stay cached. Before,
-  on the same tree took every check from the build cache and reported
+  a second run on the same tree took every check from the build cache
-  success having run nothing.
+  and reported success having run nothing.
+- 2026-10-04: A failed unlocker add no longer leaves a partial unlocker
+  directory (https://git.eeqj.de/sneak/secret/issues/48).
+  `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
+  types write their files through `secret.WriteDir`: a new unlocker is
+  built in a temporary directory, renamed into place when complete and
+  removed on a failure. One added under the directory name of an
+  existing unlocker is still 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
@@ -118,13 +148,10 @@ Bring the repo into policy compliance in one commit:
     - 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
+    - data under a `.tmp-` name in the state directory: a secret,
-      directory that `unlocker list` warns about and `unlocker rm`
+      version or unlocker being added, or the secret, version, unlocker
-      cannot remove;
+      or vault being removed, encrypted keys included. Nothing deletes
-    - data under a `.tmp-` name in the state directory: a secret or
+      it; it must be deleted by hand
-      version being added, or the secret, version, unlocker or vault
-      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
@@ -232,8 +259,6 @@ 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,9 +46,30 @@ func newRootCmd() *cobra.Command {
 		Short: "A simple secrets manager",
 		Long: `A simple secrets manager to store and retrieve sensitive ` +
 			`information securely.`,
-		// Ensure usage is shown after errors
+		// Cobra prints the error a command returns; Entry does not.
-		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

@@ -685,7 +685,8 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error {
 		return fmt.Errorf("failed to get current vault: %w", err)
 	}
 
-	// Resolve the GPG key ID to its fingerprint
+	// Resolve the GPG key ID to its fingerprint, once: the duplicate check
+	// and the new unlocker's metadata both use this result
 	fingerprint, err := secret.ResolveGPGKeyFingerprint(gpgKeyID)
 	if err != nil {
 		return fmt.Errorf("failed to resolve GPG key fingerprint: %w", err)
@@ -706,7 +707,8 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error {
 		return fmt.Errorf("GPG key %s %w", gpgKeyID, errGPGKeyAlreadyUnlocker)
 	}
 
-	pgpUnlocker, err := secret.CreatePGPUnlocker(cli.fs, cli.stateDir, gpgKeyID)
+	pgpUnlocker, err := secret.CreatePGPUnlocker(
+		cli.fs, cli.stateDir, gpgKeyID, fingerprint)
 	if err != nil {
 		return err
 	}
@@ -744,14 +746,43 @@ func (cli *Instance) removeUnlocker(
 		return err
 	}
 
-	// Get list of unlockers
+	// Get list of unlockers. It leaves out a directory whose metadata file
+	// 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 {
@@ -785,6 +816,20 @@ 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_add_test.go

@@ -0,0 +1,35 @@
+//nolint:testpackage // white-box test of unexported internals
+package cli
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// unknownTestGPGUserID is a GPG user ID that no key in the test keyring has.
+const unknownTestGPGUserID = "not-in-keyring@example.com"
+
+// TestAddPGPUnlockerUnknownKey asserts that adding a PGP unlocker for a key
+// the keyring does not hold fails at looking up the key's fingerprint and
+// leaves no new unlocker directory. The error must come from the lookup: a
+// lookup moved after anything is written would also come after getting the
+// vault's long-term key, which fails first on every platform but macOS
+// (https://git.eeqj.de/sneak/secret/issues/88).
+//
+//nolint:paralleltest // t.Setenv (GNUPGHOME) forbids parallel tests
+func TestAddPGPUnlockerUnknownKey(t *testing.T) {
+	newTestGPGKey(t)
+
+	base := newListTestVault(t, 1)
+	instance, cmd := newTestInstance(base)
+	cmd.Flags().String("keyid", unknownTestGPGUserID, "")
+
+	err := instance.addPGPUnlocker(cmd)
+
+	require.ErrorContains(t, err, "failed to resolve GPG key fingerprint")
+	assertDirEntries(t, base,
+		filepath.Join(testVaultDir(listTestVaultName), listTestUnlockersDirName),
+		listTestUnlockerDirOne)
+}

internal/cli/unlockers_corrupt_test.go

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

@@ -0,0 +1,46 @@
+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/secret/atomic.go

@@ -1,6 +1,7 @@
 package secret
 
 import (
+	"errors"
 	"fmt"
 	"path/filepath"
 
@@ -61,6 +62,52 @@ func TempDirFor(fs afero.Fs, target string) (string, error) {
 	return dir, nil
 }
 
+// WriteDir calls write to write the files of the directory dir. When dir does
+// not exist yet, write writes them into a temporary directory from TempDirFor,
+// which is then renamed to dir, so that neither a failure nor a crash leaves
+// dir half-written; on a failure the temporary directory is removed, and a
+// failure to remove it is returned along with the first. A directory cannot be
+// renamed over one that has files in it, so when dir already exists, write
+// writes into it in place; dir is then never removed.
+func WriteDir(fs afero.Fs, dir string, write func(dir string) error) error {
+	exists, err := afero.Exists(fs, dir)
+	if err != nil {
+		return fmt.Errorf("failed to check for %s: %w", dir, err)
+	}
+
+	if exists {
+		return write(dir)
+	}
+
+	// Create the directory the finished one is renamed into
+	err = fs.MkdirAll(filepath.Dir(dir), DirPerms)
+	if err != nil {
+		return fmt.Errorf("failed to create %s: %w", filepath.Dir(dir), err)
+	}
+
+	tmp, err := TempDirFor(fs, dir)
+	if err != nil {
+		return err
+	}
+
+	err = write(tmp)
+	if err == nil {
+		err = fs.Rename(tmp, dir)
+	}
+
+	if err != nil {
+		removeErr := fs.RemoveAll(tmp)
+		if removeErr != nil {
+			err = errors.Join(err,
+				fmt.Errorf("failed to remove %s: %w", tmp, removeErr))
+		}
+
+		return err
+	}
+
+	return nil
+}
+
 // RemoveDirAtomic deletes the directory dir so that it disappears in one
 // rename: dir is moved into a new directory from TempDirFor, which is then
 // deleted. A crash part-way leaves only that temporary directory behind.

internal/secret/atomic_test.go

@@ -35,6 +35,10 @@ const currentFile = "current"
 // unlockerMetadataFile is the file a new unlocker writes last.
 const unlockerMetadataFile = "unlocker-metadata.json"
 
+// privKeyFile is the file that holds the encrypted private key of a version
+// or of a passphrase unlocker.
+const privKeyFile = "priv.age"
+
 // unlockerPassphrase protects the passphrase unlockers the tests create.
 //
 //nolint:gosec // G101: test data, not a real credential
@@ -453,7 +457,7 @@ func TestVersionSaveIsWholeOrAbsent(t *testing.T) {
 
 				if exists {
 					assert.ElementsMatch(t,
-						[]string{"pub.age", "value.age", "priv.age", "metadata.age"},
+						[]string{"pub.age", "value.age", privKeyFile, "metadata.age"},
 						dirNames(t, base, versionDir),
 						"version directory visible before it was complete")
 				}
@@ -496,7 +500,7 @@ func TestVersionSaveFailureLeavesNothing(t *testing.T) {
 			writeLongTermKey(t, base, stateDir)
 
 			fs := hookFs{Fs: base, before: func(op, path string) error {
-				if op == opRename && filepath.Base(path) == "priv.age" {
+				if op == opRename && filepath.Base(path) == privKeyFile {
 					return errInjected
 				}
 
@@ -642,37 +646,119 @@ func TestPassphraseUnlockerGetsKeyFirst(t *testing.T) {
 	require.Error(t, err)
 }
 
-// TestPassphraseUnlockerWritesMetadataLast checks that the last file a new
+// TestPassphraseUnlockerIsWholeOrAbsent checks, before every change that
-// passphrase unlocker writes in its directory is its metadata: an unlocker
+// creating a passphrase unlocker makes, that the unlocker's directory either
-// directory without metadata is never used, so one interrupted earlier
+// does not exist or holds all of its files: a crash or a failure at any point
-// cannot be.
+// leaves no partial unlocker.
-func TestPassphraseUnlockerWritesMetadataLast(t *testing.T) {
+//
+//nolint:paralleltest // t.Setenv forbids t.Parallel
+func TestPassphraseUnlockerIsWholeOrAbsent(t *testing.T) {
 	t.Setenv(secret.EnvMnemonic, testMnemonic)
 
-	base := afero.NewMemMapFs()
+	files := []string{"pub.age", privKeyFile, "longterm.age", unlockerMetadataFile}
-	vlt, err := vault.CreateVault(base, testVaultStateDir, testVaultName)
-	require.NoError(t, err)
 
-	vaultDir, err := vlt.GetDirectory()
+	for _, tfs := range testFilesystems {
-	require.NoError(t, err)
+		t.Run(tfs.name, func(t *testing.T) {
+			base, stateDir := tfs.open(t)
+			vlt, err := vault.CreateVault(base, stateDir, testVaultName)
+			require.NoError(t, err)
 
-	unlockerDir := filepath.Join(vaultDir, "unlockers.d", "passphrase")
+			vaultDir, err := vlt.GetDirectory()
+			require.NoError(t, err)
 
-	var last string
+			unlockerDir := filepath.Join(vaultDir, "unlockers.d", "passphrase")
 
-	fs := hookFs{Fs: base, before: func(_, path string) error {
+			fs := hookFs{Fs: base, before: func(string, string) error {
-		if filepath.Dir(path) == unlockerDir {
+				exists, err := afero.DirExists(base, unlockerDir)
-			last = filepath.Base(path)
+				require.NoError(t, err)
-		}
 
-		return nil
+				if exists {
-	}}
+					assert.ElementsMatch(t, files, dirNames(t, base, unlockerDir),
+						"unlocker directory visible before it was complete")
+				}
 
-	passphrase := memguard.NewBufferFromBytes([]byte(unlockerPassphrase))
+				return nil
-	defer passphrase.Destroy()
+			}}
 
-	_, err = vault.NewVault(fs, testVaultStateDir, testVaultName).
+			passphrase := memguard.NewBufferFromBytes([]byte(unlockerPassphrase))
-		CreatePassphraseUnlocker(passphrase)
+			defer passphrase.Destroy()
-	require.NoError(t, err)
+
-	assert.Equal(t, unlockerMetadataFile, last)
+			_, err = vault.NewVault(fs, stateDir, testVaultName).
+				CreatePassphraseUnlocker(passphrase)
+			require.NoError(t, err)
+			assert.ElementsMatch(t, files, dirNames(t, base, unlockerDir))
+		})
+	}
+}
+
+// TestWriteDirFailureLeavesNothing makes writing a new directory fail after
+// a file has been written in it, and checks that neither the directory nor
+// its temporary directory is left behind; and, when the temporary directory
+// cannot be removed either, that both failures are reported.
+func TestWriteDirFailureLeavesNothing(t *testing.T) {
+	t.Parallel()
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			base, dir := tfs.open(t)
+			listed := filepath.Join(dir, "unlockers.d")
+			target := filepath.Join(listed, "new")
+
+			writeThenFail := func(tmp string) error {
+				require.NoError(t, secret.WriteFileAtomic(base,
+					filepath.Join(tmp, unlockerMetadataFile), []byte("{}")))
+
+				return errInjected
+			}
+
+			err := secret.WriteDir(base, target, writeThenFail)
+			require.ErrorIs(t, err, errInjected)
+
+			// Nothing in the directory that is listed, nor beside it
+			assert.Empty(t, dirNames(t, base, listed))
+			assert.Equal(t, []string{"unlockers.d"}, dirNames(t, base, dir))
+
+			fs := hookFs{Fs: base, before: func(op, _ string) error {
+				if op == opRemove {
+					return os.ErrPermission
+				}
+
+				return nil
+			}}
+
+			err = secret.WriteDir(fs, target, writeThenFail)
+			require.ErrorIs(t, err, errInjected)
+			require.ErrorIs(t, err, os.ErrPermission)
+			assert.Empty(t, dirNames(t, base, listed))
+		})
+	}
+}
+
+// TestWriteDirKeepsExistingDir makes writing into a directory that already
+// exists fail, and checks that the directory, with what was in it, is still
+// there: WriteDir writes into it in place and never removes it.
+func TestWriteDirKeepsExistingDir(t *testing.T) {
+	t.Parallel()
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			fs, dir := tfs.open(t)
+			target := filepath.Join(dir, "unlockers.d", "passphrase")
+			require.NoError(t, fs.MkdirAll(target, secret.DirPerms))
+			require.NoError(t, secret.WriteFileAtomic(fs,
+				filepath.Join(target, unlockerMetadataFile), []byte("{}")))
+
+			err := secret.WriteDir(fs, target, func(got string) error {
+				assert.Equal(t, target, got)
+
+				return errInjected
+			})
+			require.ErrorIs(t, err, errInjected)
+			assert.Equal(t, []string{unlockerMetadataFile}, dirNames(t, fs, target))
+		})
+	}
 }

internal/secret/keychainunlocker.go

@@ -341,16 +341,13 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 		return nil, fmt.Errorf("failed to generate keychain item name: %w", err)
 	}
 
-	// Create unlocker directory using the keychain item name as the directory name
+	// The unlocker directory is named after the keychain item
 	vaultDir, err := vault.GetDirectory()
 	if err != nil {
 		return nil, fmt.Errorf("failed to get vault directory: %w", err)
 	}
 
 	unlockerDir := filepath.Join(vaultDir, "unlockers.d", keychainItemName)
-	if err := fs.MkdirAll(unlockerDir, DirPerms); err != nil {
-		return nil, fmt.Errorf("failed to create unlocker directory: %w", err)
-	}
 
 	// Step 1: Generate a new age keypair for the keychain unlocker
 	ageIdentity, err := age.GenerateX25519Identity()
@@ -358,6 +355,8 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 		return nil, fmt.Errorf("failed to generate age keypair: %w", err)
 	}
 
+	ageRecipient := ageIdentity.Recipient().String()
+
 	// Step 2: Generate a random passphrase for encrypting the age private key
 	agePrivKeyPassphrase, err := generateRandomPassphrase(agePrivKeyPassphraseLength)
 	if err != nil {
@@ -365,14 +364,7 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 	}
 	defer agePrivKeyPassphrase.Destroy()
 
-	// Step 3: Store age recipient as plaintext
+	// Step 3: Encrypt age private key with the generated passphrase
-	ageRecipient := ageIdentity.Recipient().String()
-	recipientPath := filepath.Join(unlockerDir, "pub.txt")
-	if err := WriteFileAtomic(fs, recipientPath, []byte(ageRecipient)); err != nil {
-		return nil, fmt.Errorf("failed to write age recipient: %w", err)
-	}
-
-	// Step 4: Encrypt age private key with the generated passphrase and store on disk
 	// Create a secure buffer for the private key
 	agePrivKeyStr := ageIdentity.String()
 	agePrivKeyBuffer := memguard.NewBufferFromBytes([]byte(agePrivKeyStr))
@@ -383,31 +375,20 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 		return nil, fmt.Errorf("failed to encrypt age private key with passphrase: %w", err)
 	}
 
-	agePrivKeyPath := filepath.Join(unlockerDir, "priv.age")
+	// Step 4: Get or derive the long-term private key
-	if err := WriteFileAtomic(fs, agePrivKeyPath, encryptedAgePrivKey); err != nil {
-		return nil, fmt.Errorf("failed to write encrypted age private key: %w", err)
-	}
-
-	// Step 5: Get or derive the long-term private key
 	ltPrivKeyData, err := getLongTermPrivateKey(fs, vault)
 	if err != nil {
 		return nil, err
 	}
 	defer ltPrivKeyData.Destroy()
 
-	// Step 6: Encrypt long-term private key to the new age unlocker
+	// Step 5: Encrypt long-term private key to the new age unlocker
 	encryptedLtPrivKeyToAge, err := EncryptToRecipient(ltPrivKeyData, ageIdentity.Recipient())
 	if err != nil {
 		return nil, fmt.Errorf("failed to encrypt long-term private key to age unlocker: %w", err)
 	}
 
-	// Write encrypted long-term private key
+	// Step 6: Prepare keychain data
-	ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age")
-	if err := WriteFileAtomic(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge); err != nil {
-		return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err)
-	}
-
-	// Step 7: Prepare keychain data
 	keychainData := KeychainData{
 		AgePublicKey:         ageRecipient,
 		AgePrivKeyPassphrase: agePrivKeyPassphrase,
@@ -420,12 +401,7 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 	}
 	defer keychainDataBuffer.Destroy()
 
-	// Step 8: Store data in keychain
+	// Step 7: Prepare enhanced metadata
-	if err := storeInKeychain(keychainItemName, keychainDataBuffer); err != nil {
-		return nil, fmt.Errorf("failed to store data in keychain: %w", err)
-	}
-
-	// Step 9: Create and write enhanced metadata
 	keychainMetadata := KeychainUnlockerMetadata{
 		UnlockerMetadata: UnlockerMetadata{
 			Type:      "keychain",
@@ -440,10 +416,37 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 		return nil, fmt.Errorf("failed to marshal unlocker metadata: %w", err)
 	}
 
-	if err := WriteFileAtomic(fs,
+	// Step 8: Write the unlocker's files and store the data in the keychain,
-		filepath.Join(unlockerDir, "unlocker-metadata.json"),
+	// the metadata last
-		metadataBytes); err != nil {
+	err = WriteDir(fs, unlockerDir, func(dir string) error {
-		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
+		pubPath := filepath.Join(dir, "pub.txt")
+		if err := WriteFileAtomic(fs, pubPath, []byte(ageRecipient)); err != nil {
+			return fmt.Errorf("failed to write age recipient: %w", err)
+		}
+
+		privPath := filepath.Join(dir, "priv.age")
+		if err := WriteFileAtomic(fs, privPath, encryptedAgePrivKey); err != nil {
+			return fmt.Errorf("failed to write encrypted age private key: %w", err)
+		}
+
+		ltKeyPath := filepath.Join(dir, "longterm.age")
+		if err := WriteFileAtomic(fs, ltKeyPath, encryptedLtPrivKeyToAge); err != nil {
+			return fmt.Errorf("failed to write encrypted long-term private key: %w", err)
+		}
+
+		if err := storeInKeychain(keychainItemName, keychainDataBuffer); err != nil {
+			return fmt.Errorf("failed to store data in keychain: %w", err)
+		}
+
+		metadataPath := filepath.Join(dir, "unlocker-metadata.json")
+		if err := WriteFileAtomic(fs, metadataPath, metadataBytes); err != nil {
+			return fmt.Errorf("failed to write unlocker metadata: %w", err)
+		}
+
+		return nil
+	})
+	if err != nil {
+		return nil, err
 	}
 
 	return &KeychainUnlocker{

internal/secret/pgpunlock_test.go

@@ -290,7 +290,7 @@ Passphrase: ` + testPassphrase + `
 		}
 
 		// Now create a PGP unlock key (this will use our custom GPGEncryptFunc)
-		pgpUnlocker, err := secret.CreatePGPUnlocker(fs, stateDir, keyID)
+		pgpUnlocker, err := secret.CreatePGPUnlocker(fs, stateDir, keyID, fingerprint)
 		if err != nil {
 			t.Fatalf("Failed to create PGP unlock key: %v", err)
 		}

internal/secret/pgpunlocker.go

@@ -222,20 +222,13 @@ func generatePGPUnlockerName() (string, error) {
 	return fmt.Sprintf("%s-pgp-%s", hostname, enrollmentDate), nil
 }
 
-// preparePGPUnlockerDir checks GPG availability and creates the
+// pgpUnlockerDir returns the current vault and the directory in it for a
-// unlocker directory in the current vault, returning the vault and the
+// new PGP unlocker, named after the host and the day.
-// directory path.
 //
 //nolint:ireturn // the vault is only available behind VaultInterface
-func preparePGPUnlockerDir(
+func pgpUnlockerDir(
 	fs afero.Fs, stateDir string,
 ) (VaultInterface, string, error) {
-	// Check if GPG is available
-	err := checkGPGAvailable()
-	if err != nil {
-		return nil, "", err
-	}
-
 	// Get current vault
 	vault, err := GetCurrentVault(fs, stateDir)
 	if err != nil {
@@ -248,27 +241,29 @@ func preparePGPUnlockerDir(
 		return nil, "", fmt.Errorf("failed to generate unlocker name: %w", err)
 	}
 
-	// Create unlocker directory using the generated name
 	vaultDir, err := vault.GetDirectory()
 	if err != nil {
 		return nil, "", fmt.Errorf("failed to get vault directory: %w", err)
 	}
 
-	unlockerDir := filepath.Join(vaultDir, "unlockers.d", unlockerName)
+	return vault, filepath.Join(vaultDir, "unlockers.d", unlockerName), nil
-
-	err = fs.MkdirAll(unlockerDir, DirPerms)
-	if err != nil {
-		return nil, "", fmt.Errorf("failed to create unlocker directory: %w", err)
-	}
-
-	return vault, unlockerDir, nil
 }
 
-// CreatePGPUnlocker creates a new PGP unlocker and stores it in the vault
+// CreatePGPUnlocker creates a new PGP unlocker and stores it in the vault.
+// It encrypts to the GPG key gpgKeyID and records fingerprint, that key's
+// fingerprint as ResolveGPGKeyFingerprint returns it, in the metadata.
+// Everything that can fail short of writing a file is done before anything
+// is written, and the files are written through WriteDir, so a failure
+// leaves no partial unlocker.
 func CreatePGPUnlocker(
-	fs afero.Fs, stateDir string, gpgKeyID string,
+	fs afero.Fs, stateDir, gpgKeyID, fingerprint string,
 ) (*PGPUnlocker, error) {
-	vault, unlockerDir, err := preparePGPUnlockerDir(fs, stateDir)
+	err := checkGPGAvailable()
+	if err != nil {
+		return nil, err
+	}
+
+	vault, unlockerDir, err := pgpUnlockerDir(fs, stateDir)
 	if err != nil {
 		return nil, err
 	}
@@ -279,77 +274,13 @@ func CreatePGPUnlocker(
 		return nil, fmt.Errorf("failed to generate age keypair: %w", err)
 	}
 
-	// Step 2: Store age recipient as plaintext
+	// Step 2: Encrypt the long-term private key to the new keypair, and the
-	ageRecipient := ageIdentity.Recipient().String()
+	// keypair's private key to the GPG key
-	recipientPath := filepath.Join(unlockerDir, "pub.txt")
+	encryptedLtPrivKey, encryptedAgePrivKey, err := encryptPGPUnlockerKeys(
-
+		fs, vault, ageIdentity, gpgKeyID)
-	err = WriteFileAtomic(fs, recipientPath, []byte(ageRecipient))
-	if err != nil {
-		return nil, fmt.Errorf("failed to write age recipient: %w", err)
-	}
-
-	// Step 3: Get or derive the long-term private key
-	ltPrivKeyData, err := getLongTermPrivateKey(fs, vault)
 	if err != nil {
 		return nil, err
 	}
-	defer ltPrivKeyData.Destroy()
-
-	// Step 7: Encrypt long-term private key to the new age unlocker
-	encryptedLtPrivKeyToAge, err := EncryptToRecipient(
-		ltPrivKeyData, ageIdentity.Recipient())
-	if err != nil {
-		return nil, fmt.Errorf(
-			"failed to encrypt long-term private key to age unlocker: %w", err)
-	}
-
-	// Write encrypted long-term private key
-	ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age")
-
-	err = WriteFileAtomic(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge)
-	if err != nil {
-		return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err)
-	}
-
-	// Step 8: Encrypt age private key to the GPG key ID
-	// Use memguard to protect the private key in memory
-	agePrivateKeyBuffer := memguard.NewBufferFromBytes([]byte(ageIdentity.String()))
-	defer agePrivateKeyBuffer.Destroy()
-
-	encryptedAgePrivKey, err := GPGEncryptFunc(agePrivateKeyBuffer, gpgKeyID)
-	if err != nil {
-		return nil, fmt.Errorf("failed to encrypt age private key with GPG: %w", err)
-	}
-
-	agePrivKeyPath := filepath.Join(unlockerDir, "priv.age.gpg")
-
-	err = WriteFileAtomic(fs, agePrivKeyPath, encryptedAgePrivKey)
-	if err != nil {
-		return nil, fmt.Errorf("failed to write encrypted age private key: %w", err)
-	}
-
-	// Steps 9-10: Resolve the fingerprint and write enhanced metadata
-	pgpMetadata, err := writePGPUnlockerMetadata(fs, unlockerDir, gpgKeyID)
-	if err != nil {
-		return nil, err
-	}
-
-	return &PGPUnlocker{
-		Directory: unlockerDir,
-		Metadata:  pgpMetadata.UnlockerMetadata,
-		fs:        fs,
-	}, nil
-}
-
-// writePGPUnlockerMetadata resolves the GPG key fingerprint and writes
-// the unlocker metadata file, returning the metadata written.
-func writePGPUnlockerMetadata(
-	fs afero.Fs, unlockerDir string, gpgKeyID string,
-) (*PGPUnlockerMetadata, error) {
-	fingerprint, err := ResolveGPGKeyFingerprint(gpgKeyID)
-	if err != nil {
-		return nil, fmt.Errorf("failed to resolve GPG key fingerprint: %w", err)
-	}
 
 	pgpMetadata := PGPUnlockerMetadata{
 		UnlockerMetadata: UnlockerMetadata{
@@ -365,13 +296,85 @@ func writePGPUnlockerMetadata(
 		return nil, fmt.Errorf("failed to marshal unlocker metadata: %w", err)
 	}
 
-	err = WriteFileAtomic(fs,
+	// Step 3: Write the unlocker's files, the metadata last
-		filepath.Join(unlockerDir, "unlocker-metadata.json"), metadataBytes)
+	err = WriteDir(fs, unlockerDir, func(dir string) error {
+		return writePGPUnlockerFiles(fs, dir, ageIdentity.Recipient(),
+			encryptedLtPrivKey, encryptedAgePrivKey, metadataBytes)
+	})
 	if err != nil {
-		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
+		return nil, err
 	}
 
-	return &pgpMetadata, nil
+	return &PGPUnlocker{
+		Directory: unlockerDir,
+		Metadata:  pgpMetadata.UnlockerMetadata,
+		fs:        fs,
+	}, nil
+}
+
+// encryptPGPUnlockerKeys returns the vault's long-term private key encrypted
+// to the new PGP unlocker's age keypair, and that keypair's private key
+// encrypted to the GPG key gpgKeyID.
+func encryptPGPUnlockerKeys(
+	fs afero.Fs, vault VaultInterface,
+	ageIdentity *age.X25519Identity, gpgKeyID string,
+) ([]byte, []byte, error) {
+	// Get or derive the long-term private key
+	ltPrivKeyData, err := getLongTermPrivateKey(fs, vault)
+	if err != nil {
+		return nil, nil, err
+	}
+	defer ltPrivKeyData.Destroy()
+
+	encryptedLtPrivKey, err := EncryptToRecipient(
+		ltPrivKeyData, ageIdentity.Recipient())
+	if err != nil {
+		return nil, nil, fmt.Errorf(
+			"failed to encrypt long-term private key to age unlocker: %w", err)
+	}
+
+	// Use memguard to protect the private key in memory
+	agePrivateKeyBuffer := memguard.NewBufferFromBytes([]byte(ageIdentity.String()))
+	defer agePrivateKeyBuffer.Destroy()
+
+	encryptedAgePrivKey, err := GPGEncryptFunc(agePrivateKeyBuffer, gpgKeyID)
+	if err != nil {
+		return nil, nil, fmt.Errorf(
+			"failed to encrypt age private key with GPG: %w", err)
+	}
+
+	return encryptedLtPrivKey, encryptedAgePrivKey, nil
+}
+
+// writePGPUnlockerFiles writes the files of a PGP unlocker into dir, the
+// metadata last.
+func writePGPUnlockerFiles(
+	fs afero.Fs, dir string, ageRecipient *age.X25519Recipient,
+	encryptedLtPrivKey, encryptedAgePrivKey, metadataBytes []byte,
+) error {
+	err := WriteFileAtomic(fs, filepath.Join(dir, "pub.txt"),
+		[]byte(ageRecipient.String()))
+	if err != nil {
+		return fmt.Errorf("failed to write age recipient: %w", err)
+	}
+
+	err = WriteFileAtomic(fs, filepath.Join(dir, "longterm.age"), encryptedLtPrivKey)
+	if err != nil {
+		return fmt.Errorf("failed to write encrypted long-term private key: %w", err)
+	}
+
+	err = WriteFileAtomic(fs, filepath.Join(dir, "priv.age.gpg"), encryptedAgePrivKey)
+	if err != nil {
+		return fmt.Errorf("failed to write encrypted age private key: %w", err)
+	}
+
+	err = WriteFileAtomic(fs,
+		filepath.Join(dir, "unlocker-metadata.json"), metadataBytes)
+	if err != nil {
+		return fmt.Errorf("failed to write unlocker metadata: %w", err)
+	}
+
+	return nil
 }
 
 // validateGPGKeyID validates that a GPG key ID is safe for command execution

internal/secret/pgpunlocker_test.go

@@ -0,0 +1,67 @@
+package secret_test
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"git.eeqj.de/sneak/secret/internal/secret"
+	"git.eeqj.de/sneak/secret/internal/vault"
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// The GPG key ID and fingerprint passed to CreatePGPUnlocker.
+const (
+	testGPGKeyID       = "0123456789ABCDEF"
+	testGPGFingerprint = "0123456789ABCDEF0123456789ABCDEF01234567"
+)
+
+// fakeGPGScript is a gpg for which `gpg --version` succeeds and anything
+// else fails.
+const fakeGPGScript = `#!/bin/sh
+[ "$*" = --version ]
+`
+
+// installFakeGPG makes fakeGPGScript the only gpg on PATH for the test.
+func installFakeGPG(t *testing.T) {
+	t.Helper()
+
+	dir := t.TempDir()
+
+	//nolint:gosec // G306: the script must be executable
+	err := os.WriteFile(filepath.Join(dir, "gpg"), []byte(fakeGPGScript), 0o700)
+	require.NoError(t, err)
+
+	t.Setenv("PATH", dir)
+}
+
+// TestCreatePGPUnlockerFailureWritesNothing makes CreatePGPUnlocker fail at
+// getting the vault's long-term key, which used to come after part of the
+// unlocker was written, and asserts that nothing is written. Getting the key
+// fails because on macOS there is no mnemonic and no current unlocker, and
+// on every other platform it always fails
+// (https://git.eeqj.de/sneak/secret/issues/88).
+func TestCreatePGPUnlockerFailureWritesNothing(t *testing.T) {
+	installFakeGPG(t)
+	t.Setenv(secret.EnvMnemonic, "")
+
+	base := afero.NewMemMapFs()
+	vlt, err := vault.CreateVault(base, testVaultStateDir, testVaultName)
+	require.NoError(t, err)
+
+	fs := hookFs{Fs: base, before: func(_, path string) error {
+		t.Errorf("changed %s", path)
+
+		return nil
+	}}
+
+	_, err = secret.CreatePGPUnlocker(
+		fs, testVaultStateDir, testGPGKeyID, testGPGFingerprint)
+	require.Error(t, err)
+
+	vaultDir, err := vlt.GetDirectory()
+	require.NoError(t, err)
+	assert.Empty(t, dirNames(t, base, filepath.Join(vaultDir, "unlockers.d")))
+}

internal/secret/seunlocker_darwin.go

@@ -254,7 +254,7 @@ func CreateSecureEnclaveUnlocker(
 		)
 	}
 
-	// Step 4: Create unlocker directory and write files
+	// Step 4: Prepare the unlocker directory's path and metadata
 	vaultDir, err := vault.GetDirectory()
 	if err != nil {
 		return nil, fmt.Errorf("failed to get vault directory: %w", err)
@@ -262,23 +262,7 @@ func CreateSecureEnclaveUnlocker(
 
 	unlockerDirName := fmt.Sprintf("se-%s", filepath.Base(seKeyLabel))
 	unlockerDir := filepath.Join(vaultDir, "unlockers.d", unlockerDirName)
-	if err := fs.MkdirAll(unlockerDir, DirPerms); err != nil {
-		return nil, fmt.Errorf(
-			"failed to create unlocker directory: %w",
-			err,
-		)
-	}
 
-	// Write SE-encrypted long-term key
-	ltKeyPath := filepath.Join(unlockerDir, seLongtermFilename)
-	if err := WriteFileAtomic(fs, ltKeyPath, encryptedLtKey); err != nil {
-		return nil, fmt.Errorf(
-			"failed to write SE-encrypted long-term key: %w",
-			err,
-		)
-	}
-
-	// Write metadata
 	seMetadata := SecureEnclaveUnlockerMetadata{
 		UnlockerMetadata: UnlockerMetadata{
 			Type:      seUnlockerType,
@@ -294,9 +278,25 @@ func CreateSecureEnclaveUnlocker(
 		return nil, fmt.Errorf("failed to marshal metadata: %w", err)
 	}
 
-	metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json")
+	// Step 5: Write the SE-encrypted long-term key, then the metadata
-	if err := WriteFileAtomic(fs, metadataPath, metadataBytes); err != nil {
+	err = WriteDir(fs, unlockerDir, func(dir string) error {
-		return nil, fmt.Errorf("failed to write metadata: %w", err)
+		ltKeyPath := filepath.Join(dir, seLongtermFilename)
+		if err := WriteFileAtomic(fs, ltKeyPath, encryptedLtKey); err != nil {
+			return fmt.Errorf(
+				"failed to write SE-encrypted long-term key: %w",
+				err,
+			)
+		}
+
+		metadataPath := filepath.Join(dir, "unlocker-metadata.json")
+		if err := WriteFileAtomic(fs, metadataPath, metadataBytes); err != nil {
+			return fmt.Errorf("failed to write metadata: %w", err)
+		}
+
+		return nil
+	})
+	if err != nil {
+		return nil, err
 	}
 
 	return &SecureEnclaveUnlocker{

internal/vault/unlockers.go

@@ -126,7 +126,11 @@ func (v *Vault) resolveUnlockerDirectory(currentUnlockerPath string) (string, er
 }
 
 // findUnlockerByID finds an unlocker by its ID and returns the unlocker
-// instance and its directory path
+// instance and its directory path. A directory that ListUnlockers skips is
+// 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(
@@ -137,42 +141,24 @@ func (v *Vault) findUnlockerByID(
 		return nil, "", fmt.Errorf("failed to read unlockers directory: %w", err)
 	}
 
+	skippedDirPath := ""
+
 	for _, file := range files {
 		if !file.IsDir() {
 			continue
 		}
 
-		// Read metadata file
+		unlockerDirPath := filepath.Join(unlockersDir, file.Name())
-		metadataPath := filepath.Join(unlockersDir, file.Name(), "unlocker-metadata.json")
 
-		exists, err := afero.Exists(v.fs, metadataPath)
+		metadata, ok := v.readUnlockerMetadataOrWarn(unlockersDir, file.Name())
-		if err != nil {
+		if !ok {
-			return nil, "", fmt.Errorf(
+			if file.Name() == unlockerID {
-				"failed to check if metadata exists for unlocker %s: %w",
+				skippedDirPath = unlockerDirPath
-				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
 
@@ -195,7 +181,7 @@ func (v *Vault) findUnlockerByID(
 		}
 	}
 
-	return nil, "", nil
+	return nil, skippedDirPath, nil
 }
 
 // ListUnlockers returns a list of available unlockers for this vault
@@ -226,44 +212,12 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) {
 	var unlockers []UnlockerMetadata
 
 	for _, file := range files {
-		if file.IsDir() {
+		if !file.IsDir() {
-			// Read metadata file
+			continue
-			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)
 		}
 	}
@@ -271,7 +225,54 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) {
 	return unlockers, nil
 }
 
-// RemoveUnlocker removes an unlocker from this vault
+// readUnlockerMetadataOrWarn reads the metadata of the unlocker directory
+// 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 {
@@ -282,15 +283,19 @@ func (v *Vault) RemoveUnlocker(unlockerID string) error {
 	unlockersDir := filepath.Join(vaultDir, "unlockers.d")
 
 	// Find the unlocker by ID
-	unlocker, _, err := v.findUnlockerByID(unlockersDir, unlockerID)
+	unlocker, unlockerDir, err := v.findUnlockerByID(unlockersDir, unlockerID)
 	if err != nil {
 		return err
 	}
 
-	if unlocker == nil {
+	if unlockerDir == "" {
 		return fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound)
 	}
 
+	if unlocker == nil {
+		return secret.RemoveDirAtomic(v.fs, unlockerDir)
+	}
+
 	// Use the unlocker's Remove method
 	return unlocker.Remove()
 }
@@ -306,12 +311,14 @@ func (v *Vault) SelectUnlocker(unlockerID string) error {
 	unlockersDir := filepath.Join(vaultDir, "unlockers.d")
 
 	// Find the unlocker by ID
-	_, targetUnlockerDir, err := v.findUnlockerByID(unlockersDir, unlockerID)
+	unlocker, targetUnlockerDir, err := v.findUnlockerByID(unlockersDir, unlockerID)
 	if err != nil {
 		return err
 	}
 
-	if targetUnlockerDir == "" {
+	// A directory found without an unlocker is one ListUnlockers skips; it
+	// cannot be selected.
+	if unlocker == nil {
 		return fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound)
 	}
 
@@ -350,26 +357,14 @@ func (v *Vault) CreatePassphraseUnlocker(
 		return nil, fmt.Errorf("failed to get long-term key: %w", err)
 	}
 
-	// Create unlocker directory
 	unlockerDir := filepath.Join(vaultDir, "unlockers.d", unlockerTypePassphrase)
 
-	err = v.fs.MkdirAll(unlockerDir, secret.DirPerms)
-	if err != nil {
-		return nil, fmt.Errorf("failed to create unlocker directory: %w", err)
-	}
-
 	// Generate new age keypair for unlocker
 	unlockerIdentity, err := age.GenerateX25519Identity()
 	if err != nil {
 		return nil, fmt.Errorf("failed to generate unlocker: %w", err)
 	}
 
-	// Write the unlocker keypair (public and passphrase-encrypted private)
-	err = v.writeUnlockerKeypair(unlockerDir, unlockerIdentity, passphrase)
-	if err != nil {
-		return nil, err
-	}
-
 	// Encrypt long-term private key to this unlocker
 	ltPrivKeyBuffer := memguard.NewBufferFromBytes([]byte(ltIdentity.String()))
 	defer ltPrivKeyBuffer.Destroy()
@@ -380,15 +375,6 @@ func (v *Vault) CreatePassphraseUnlocker(
 		return nil, fmt.Errorf("failed to encrypt long-term private key: %w", err)
 	}
 
-	ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age")
-
-	err = secret.WriteFileAtomic(v.fs, ltPrivKeyPath, encryptedLtPrivKey)
-	if err != nil {
-		return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err)
-	}
-
-	// Write the metadata last: readers skip an unlocker directory without
-	// it, so an unlocker interrupted before this point is never used.
 	metadata := UnlockerMetadata{
 		Type:      unlockerTypePassphrase,
 		CreatedAt: time.Now(),
@@ -400,11 +386,13 @@ func (v *Vault) CreatePassphraseUnlocker(
 		return nil, fmt.Errorf("failed to marshal metadata: %w", err)
 	}
 
-	metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json")
+	// Write the unlocker's files, the metadata last
-
+	err = secret.WriteDir(v.fs, unlockerDir, func(dir string) error {
-	err = secret.WriteFileAtomic(v.fs, metadataPath, metadataBytes)
+		return v.writeUnlockerFiles(dir, unlockerIdentity, passphrase,
+			encryptedLtPrivKey, metadataBytes)
+	})
 	if err != nil {
-		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
+		return nil, err
 	}
 
 	// Create the unlocker instance
@@ -450,12 +438,14 @@ func (v *Vault) readUnlockerMetadata(unlockerDir string) (UnlockerMetadata, erro
 	return metadata, nil
 }
 
-// writeUnlockerKeypair writes the unlocker's public key and its
+// writeUnlockerFiles writes the files of a passphrase unlocker into
-// passphrase-encrypted private key into the unlocker directory.
+// unlockerDir: its public key, its passphrase-encrypted private key, the
-func (v *Vault) writeUnlockerKeypair(
+// long-term private key encrypted to it, and its metadata, last.
+func (v *Vault) writeUnlockerFiles(
 	unlockerDir string,
 	unlockerIdentity *age.X25519Identity,
 	passphrase *memguard.LockedBuffer,
+	encryptedLtPrivKey, metadataBytes []byte,
 ) error {
 	// Write public key
 	pubKeyPath := filepath.Join(unlockerDir, "pub.age")
@@ -485,5 +475,17 @@ func (v *Vault) writeUnlockerKeypair(
 		return fmt.Errorf("failed to write encrypted unlocker private key: %w", err)
 	}
 
+	err = secret.WriteFileAtomic(v.fs,
+		filepath.Join(unlockerDir, "longterm.age"), encryptedLtPrivKey)
+	if err != nil {
+		return fmt.Errorf("failed to write encrypted long-term private key: %w", err)
+	}
+
+	err = secret.WriteFileAtomic(v.fs,
+		filepath.Join(unlockerDir, "unlocker-metadata.json"), metadataBytes)
+	if err != nil {
+		return fmt.Errorf("failed to write unlocker metadata: %w", err)
+	}
+
 	return nil
 }