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 could not be read, reported no duplicate and went on. It now
stops with an error naming the directory and cause.

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. `unlocker list` keeps skipping entries it
cannot read.

`vault rm` and `unlocker rm` now take the state directory lock and call
an unexported function that does the work, as `vault import` does, so
the tests can reach their checks.

Model: opus-5-5

.dockerignore

@@ -1,3 +1,9 @@
+# .git is sent without its config. Without a VERSION build argument the
+# stage that compiles runs `git describe --tags --always` on .git, which
+# does not need .git/config; that file can hold a credential, such as a
+# password in a remote URL or the token the CI checkout step stores there.
+.git/config
+
 # Build artifacts
 secret
 coverage.out

Dockerfile

@@ -9,7 +9,8 @@ RUN go mod download
 COPY . .
 
 RUN make fmt-check
-RUN make lint
+# Not make lint: script/lint is a docker build, which cannot run in here.
+RUN golangci-lint run --config .golangci.yml ./...
 
 # Build stage — tests and compilation
 # golang 1.24.13-alpine (2026-03-10)
@@ -27,7 +28,20 @@ RUN go mod download
 COPY . .
 
 RUN make test
-RUN make build
+
+# The version stamped into the binary: the VERSION build argument when one
+# is given, otherwise `git describe --tags --always` of the .git the build
+# context carries: the tag on a tagged commit, tag-N-gHASH on a commit after
+# one, the short commit when no tag is reachable. A context that carries .git
+# and still yields no version fails the build.
+ARG VERSION
+RUN version="${VERSION:-$(git describe --tags --always)}"; \
+    if [ -e .git ] && { [ -z "$version" ] || [ "$version" = dev ] || \
+        [ "$version" = unknown ]; }; then \
+        echo "no version could be derived although the build context carries .git" >&2; \
+        exit 1; \
+    fi; \
+    make build VERSION="${version:-dev}"
 
 # Runtime stage
 # alpine 3.23 (2026-03-10)

Dockerfile.lint

@@ -0,0 +1,19 @@
+# Lint image, built by script/lint: golangci-lint runs as a build step, so a
+# successful build is a clean lint. Works where the docker daemon is remote
+# and bind mounts are impossible.
+
+# golangci/golangci-lint:v2.12.2 (Debian-based), 2026-08-07
+FROM golangci/golangci-lint:v2.12.2@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240 AS deps
+
+WORKDIR /src
+
+COPY go.mod go.sum ./
+RUN go mod download
+
+# script/lint rebuilds this stage on every run, by this name; the module
+# download above stays cached.
+FROM deps AS lint
+
+COPY . .
+
+RUN golangci-lint run --config .golangci.yml ./...

Makefile

@@ -2,7 +2,7 @@ export CGO_ENABLED=1
 export DOCKER_HOST := ssh://root@ber1app1.local
 
 # Version information
-VERSION := 0.1.0
+VERSION := $(shell git describe --tags --always --dirty 2>/dev/null || echo "dev")
 GIT_COMMIT := $(shell git rev-parse HEAD 2>/dev/null || echo "unknown")
 LDFLAGS := -X 'git.eeqj.de/sneak/secret/internal/cli.Version=$(VERSION)' \
            -X 'git.eeqj.de/sneak/secret/internal/cli.GitCommit=$(GIT_COMMIT)'

README.md

@@ -113,7 +113,9 @@ automatically switch to another vault if removing the current one.
 Adds a secret to the current vault. Reads the secret value from stdin.
 - `--force, -f`: Overwrite existing secret
 
-**Secret Name Format:** `[a-z0-9\.\-\_\/]+`
+**Secret Name Format:** only ASCII letters, digits, `.`, `-`, `_` and `/`
+are allowed, and a name must not be empty, start with `.` or `/`, end with
+`/`, contain `//`, or have `..` as a path segment.
 - Forward slashes (`/`) are converted to percent signs (`%`) for storage
 - Examples: `database/password`, `api.key`, `ssh_private_key`
 
@@ -137,6 +139,9 @@ matching.
 
 Moves or renames a secret within the current vault.
 - Fails if the destination already exists
+- Fails if the destination is the source under another name, such as `foo`
+  for `Foo` on a case-insensitive filesystem (the macOS default); there, to
+  change only the case of a name, move the secret to a third name first
 - Preserves all versions and metadata
 
 ### Version Management
@@ -494,15 +499,18 @@ standard: normalized scripts in `script/` are the entrypoints for the
 development workflow, and the Makefile targets are thin shims that call
 them. We provide:
 
-- `script/bootstrap` — install all dependencies (Go, golangci-lint, Go
+- `script/bootstrap` — install all dependencies (Go, Go module
-  module download), idempotently
+  download), idempotently; golangci-lint is not installed, it runs in
+  docker
 - `script/setup` — make a fresh clone ready for development: runs
   `script/bootstrap`, then `script/install-precommit`
 - `script/projectname` — output the project name (`secret`); used by
   other scripts such as `script/docker`
 - `script/test` — run `go vet` and the test suite (verbose rerun on
   failure)
-- `script/lint` — run `golangci-lint`
+- `script/lint` — run `golangci-lint` in docker only: builds
+  `Dockerfile.lint`, where the linter is a build step that runs on every
+  call, also on an unchanged tree
 - `script/fmt` — format all Go code (writes)
 - `script/fmt-check` — check formatting without writing
 - `script/check` — run `script/test`, `script/lint`, and

TODO.md

@@ -25,6 +25,111 @@ Bring the repo into policy compliance in one commit:
 
 # Completed Steps
 
+- 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
+  changing command waits for the lock releases it as soon as it sees the
+  command waiting there, instead of after a fixed 100 ms. The two vaults
+  with passphrase unlockers that the path and move tests start from are
+  made once and copied for each test.
+- 2026-10-04: `secret mv` rejects a move whose destination is the source
+  under another name, such as `foo` for `Foo` on a case-insensitive
+  filesystem (the macOS default) or a name reached through a symbolic
+  link, before changing anything, with or without `--force`, within a
+  vault and between vaults; before, `--force` removed the destination and
+  so deleted the secret. A rename that changes only letter case works on a
+  case-sensitive filesystem as before.
+- 2026-10-04: Lint runs only in docker: `script/lint` builds
+  `Dockerfile.lint`, where golangci-lint is a build step rebuilt on
+  every run (`--no-cache-filter`), so an unchanged tree is linted too;
+  the module download stays cached. `script/bootstrap` no longer
+  installs golangci-lint, and the `Dockerfile` lint stage calls it
+  directly instead of `make lint`. `golangci-lint config verify` is not
+  run: it fetches its schema live over unpinned HTTPS.
+- 2026-10-04: A PGP unlocker whose metadata has no usable GPG key ID
+  no longer panics: `GetID()` warns with the unlocker's directory and
+  returns `pgp-unknown`. `ListUnlockers` skips, with a warning, an
+  unlocker whose metadata file cannot be checked for, read or parsed
+  instead of failing, so `secret unlocker list` still lists the others;
+  the listing's ID lookup no longer warns about that directory again.
+- 2026-10-03: `secret mv` rejects a move whose destination is the
+  source (`mv --force x x`, `mv --force work:x work:`, or an empty
+  destination, which defaults to the source name) before changing
+  anything; before, `--force` removed the destination first and so
+  deleted the secret. Every vault name given with `vault:` must be one
+  of the existing vaults by exact name, so `work:x work/:x` is rejected
+  instead of being taken for a move between two vaults. A move within a
+  named vault no longer makes that vault the current one, whether it
+  succeeds or fails.
+- 2026-10-03: Commands that change the state directory hold one lock
+  (`flock` on `lock` in the state directory; a mutex on the in-memory
+  test filesystem), so concurrent commands no longer lose versions or
+  race on the current pointers. Every file is written through
+  `secret.WriteFileAtomic` (temporary file, sync, rename), so no file
+  is ever half-written and `current`, `currentvault` and
+  `current-unlocker` never go missing. New versions, new secrets and
+  cross-vault copies are built in a temporary directory and renamed
+  into place, and removals rename out of the way first, so a version
+  or secret is never half-added and never half-removed. An
+  interrupted command can still leave:
+    - a broken unlocker, when it was replacing one: an unlocker added
+      under the directory name of an existing one is rewritten file by
+      file. That happens to a passphrase unlocker added to a vault that
+      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 an unlocker add stopped before its metadata is written, a
+      directory that `unlocker list` warns about and `unlocker rm`
+      cannot remove;
+    - data under a `.tmp-` name in the state directory: a secret or
+      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
+  inspect, instead of reading the failure as "nothing there": the
+  duplicate check before `unlocker add pgp` (an unreadable
+  `unlockers.d`), 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
+  (`secret.VersionExists`), and touch nothing otherwise. An empty
+  `--version` is rejected instead of meaning the current version.
+  Before, `secret version rm x ../../..` deleted the whole vault,
+  `secret version rm x ..` the secret, and `.` or `""` every version.
+- 2026-10-03: Key material is wiped on every exit: `Entry()` returns
+  the exit code after its deferred `memguard.Purge()` has run, and only
+  `main` calls `os.Exit`. SIGINT and SIGTERM go through memguard's
+  handler, which wipes every buffer before exiting; when the process is
+  in the terminal's foreground process group it first restores the
+  terminal settings from startup, so an interrupted passphrase prompt no
+  longer leaves echo off.
+- 2026-10-03: Every command that builds a path from a secret name
+  checks the name first with `vault.ValidateSecretName` and touches
+  nothing when it is invalid: `rm`, `mv` (both names, within a vault
+  and between vaults, before switching the current vault), `import`,
+  `version list`/`promote`/`rm`, `encrypt` and `decrypt`. The error
+  and `README.md` state the naming rule. Before, `secret rm ..`
+  deleted the whole vault and `secret rm .` every secret in it.
+- 2026-10-03: The keychain unlocker's age key passphrase stays in
+  locked memory: it is generated into a locked buffer, and the
+  keychain JSON is written and read by `KeychainData` code in
+  `internal/secret/keychaindata.go` (tested on Linux) without
+  `encoding/json` holding it; the JSON field names are unchanged.
+- 2026-10-02: A plain `docker build .` builds again: the size tests
+  skip a case that needs more locked memory than the process can
+  lock, and run every case under `script/cibuild`. The image stamps the
+  `VERSION` build argument, else `git describe --tags --always`, into
+  `Version`, and fails if `.git` is present but yields no version;
+  `make build` stamps `git describe` too, not a fixed `0.1.0`.
+  `.dockerignore` keeps `.git/config` out; `script/docker` is the
+  canonical copy.
 - 2026-08-07: Updated golangci-lint to v2.12.2 with the canonical
   `.golangci.yml` (all linters enabled minus the standard disable
   list, `lll` 88, tests linted); bumped the `Dockerfile` lint-stage
@@ -80,21 +185,15 @@ Bring the repo into policy compliance in one commit:
     - Command injection: GPG key IDs passed unescaped to exec.Command
       (pgpunlocker.go:323-327); data.String() passed unescaped to the
       security command (keychainunlocker.go:472-476).
-    - Memory security: KeychainData stores AgePrivKeyPassphrase as a
+    - Memory security: age identity .String() creates unprotected
-      plain string (keychainunlocker.go:342,393-396); age identity
+      copies (keychainunlocker.go:356, pgpunlocker.go:256,
-      .String() creates unprotected copies (keychainunlocker.go:356,
+      version.go:155); age secret key held in a plain string in
-      pgpunlocker.go:256, version.go:155); age secret key held in a
+      cli/crypto.go:86,91,113; private keys exposed via buffer.Bytes()
-      plain string in cli/crypto.go:86,91,113; private keys exposed via
+      to GPGEncryptFunc and EncryptWithPassphrase.
-      buffer.Bytes() to GPGEncryptFunc and EncryptWithPassphrase.
+    - Input validation: no maximum secret size (DoS).
-    - Race conditions: no file locking in vault/secrets.go:142-176;
-      non-atomic writes can leave the vault inconsistent.
-    - Input validation: dots in secret names risk path traversal
-      (vault/secrets.go:75-99); no maximum secret size (DoS).
     - Timing attacks: bytes.Equal passphrase compare (cli/init.go:
       209-216); non-constant-time public key compare (vault.go:95-100).
 - High priority:
-    - Return errors instead of panicking on corrupted metadata
-      (pgpunlocker.go:116, keychainunlocker.go:141).
     - Secure temporary file handling and cleanup.
     - Print cobra usage only for argument errors, not internal
       failures.

cmd/secret/main.go

@@ -1,8 +1,12 @@
 // Package main is the entry point for the secret CLI application.
 package main
 
-import "git.eeqj.de/sneak/secret/internal/cli"
+import (
+	"os"
+
+	"git.eeqj.de/sneak/secret/internal/cli"
+)
 
 func main() {
-	cli.Entry()
+	os.Exit(cli.Entry())
 }

go.mod

@@ -16,6 +16,7 @@ require (
 	github.com/stretchr/testify v1.8.4
 	github.com/tyler-smith/go-bip39 v1.1.0
 	golang.org/x/crypto v0.38.0
+	golang.org/x/sys v0.33.0
 	golang.org/x/term v0.32.0
 )
 
@@ -31,7 +32,6 @@ require (
 	github.com/mattn/go-isatty v0.0.20 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/spf13/pflag v1.0.6 // indirect
-	golang.org/x/sys v0.33.0 // indirect
 	golang.org/x/text v0.25.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )

internal/cli/crypto.go

@@ -70,9 +70,44 @@ func newDecryptCmd() *cobra.Command {
 	)
 }
 
+// storeNewEncryptionKey generates an age secret key and stores it as the
+// named secret, holding the state directory lock while it does. It fails
+// with vault.ErrSecretExists if another command stored the secret first.
+// The caller must destroy the returned buffer.
+func (cli *Instance) storeNewEncryptionKey(
+	vlt *vault.Vault, secretName string,
+) (*memguard.LockedBuffer, error) {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return nil, err
+	}
+	defer release()
+
+	identity, err := age.GenerateX25519Identity()
+	if err != nil {
+		return nil, fmt.Errorf("failed to generate age key: %w", err)
+	}
+
+	// Store the generated key directly in a secure buffer
+	secureBuffer := memguard.NewBufferFromBytes([]byte(identity.String()))
+
+	err = vlt.AddSecret(secretName, secureBuffer, false)
+	if err != nil {
+		secureBuffer.Destroy()
+
+		return nil, fmt.Errorf("failed to store age key: %w", err)
+	}
+
+	return secureBuffer, nil
+}
+
 // resolveEncryptionKey returns a secure buffer holding the age secret key
 // for the named secret, generating and storing a new key if the secret
-// does not exist. The caller must destroy the returned buffer.
+// does not exist. The caller must destroy the returned buffer. Only storing
+// a new key takes the state directory lock, so that reading an existing key
+// works on a read-only state directory and keeps no other command waiting
+// at the passphrase prompt, and Encrypt streams its input and output
+// unlocked.
 func (cli *Instance) resolveEncryptionKey(
 	vlt *vault.Vault, secretName string,
 ) (*memguard.LockedBuffer, error) {
@@ -85,23 +120,11 @@ func (cli *Instance) resolveEncryptionKey(
 	}
 
 	if !exists {
-		// Secret doesn't exist, generate new age key and store it
+		key, err := cli.storeNewEncryptionKey(vlt, secretName)
-		identity, err := age.GenerateX25519Identity()
+		if !errors.Is(err, vault.ErrSecretExists) {
-		if err != nil {
+			return key, err
-			return nil, fmt.Errorf("failed to generate age key: %w", err)
 		}
-
+		// Another command stored the key since the check above: read it
-		// Store the generated key directly in a secure buffer
-		secureBuffer := memguard.NewBufferFromBytes([]byte(identity.String()))
-
-		err = vlt.AddSecret(secretName, secureBuffer, false)
-		if err != nil {
-			secureBuffer.Destroy()
-
-			return nil, fmt.Errorf("failed to store age key: %w", err)
-		}
-
-		return secureBuffer, nil
 	}
 
 	// Secret exists, get the age secret key from it
@@ -122,6 +145,11 @@ func (cli *Instance) resolveEncryptionKey(
 
 // Encrypt encrypts data using an age secret key stored in a secret
 func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
+	err := vault.ValidateSecretName(secretName)
+	if err != nil {
+		return err
+	}
+
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -191,6 +219,11 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
 
 // Decrypt decrypts data using an age secret key stored in a secret
 func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
+	err := vault.ValidateSecretName(secretName)
+	if err != nil {
+		return err
+	}
+
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {

internal/cli/entry_test.go

@@ -0,0 +1,108 @@
+package cli_test
+
+import (
+	"bufio"
+	"context"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+
+	"git.eeqj.de/sneak/secret/internal/cli"
+	"git.eeqj.de/sneak/secret/internal/secret"
+	"github.com/awnumar/memguard"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Entry must return its exit code rather than exit, so that its deferred
+// memguard purge runs on the success and the error path alike.
+//
+//nolint:paralleltest // sets os.Args, and Entry wipes every buffer in the process
+func TestEntryWipesBuffersAndReturnsExitCode(t *testing.T) {
+	savedArgs := os.Args
+
+	t.Cleanup(func() { os.Args = savedArgs })
+
+	tests := []struct {
+		args     []string
+		exitCode int
+	}{
+		{args: []string{"secret", "--help"}, exitCode: 0},
+		{args: []string{"secret", "no-such-command"}, exitCode: 1},
+	}
+
+	for _, tt := range tests {
+		buf := memguard.NewBufferFromBytes([]byte("key material"))
+		os.Args = tt.args
+
+		assert.Equal(t, tt.exitCode, cli.Entry(), "exit code for %v", tt.args)
+		assert.False(t, buf.IsAlive(), "Entry left a buffer unwiped for %v", tt.args)
+	}
+}
+
+// Ctrl-C while `secret add` waits for the value on stdin must end the
+// process through memguard's signal handler, which wipes every buffer and
+// exits with status 1, not through Go's default handling, which kills the
+// process with the buffers intact.
+func TestInterruptExitsThroughMemguard(t *testing.T) {
+	t.Parallel()
+
+	const waitingForValue = "Reading secret value from stdin"
+
+	ctx, cancel := context.WithTimeout(t.Context(), time.Minute)
+	defer cancel()
+
+	wd, err := filepath.Abs("../..")
+	require.NoError(t, err)
+
+	secretPath := filepath.Join(wd, "secret")
+	env := []string{
+		secret.EnvStateDir + "=" + t.TempDir(),
+		secret.EnvMnemonic + "=" + testMnemonic,
+		secret.EnvUnlockPassphrase + "=test-passphrase",
+		"PATH=/usr/bin:/bin",
+		// The debug log on stderr shows when add starts waiting for the value.
+		"GODEBUG=berlin.sneak.pkg.secret",
+	}
+
+	//nolint:gosec // G204: test executes the freshly built secret binary
+	initCmd := exec.CommandContext(ctx, secretPath, "init")
+	initCmd.Env = env
+
+	output, err := initCmd.CombinedOutput()
+	require.NoError(t, err, "init should succeed: %s", output)
+
+	//nolint:gosec // G204: test executes the freshly built secret binary
+	addCmd := exec.CommandContext(ctx, secretPath, "add", "test/secret")
+	addCmd.Env = env
+
+	// Held open and never written, so add keeps waiting for the value.
+	stdin, err := addCmd.StdinPipe()
+	require.NoError(t, err)
+
+	defer func() { _ = stdin.Close() }()
+
+	stderr, err := addCmd.StderrPipe()
+	require.NoError(t, err)
+	require.NoError(t, addCmd.Start())
+
+	waiting := false
+
+	scanner := bufio.NewScanner(stderr)
+	for !waiting && scanner.Scan() {
+		waiting = strings.Contains(scanner.Text(), waitingForValue)
+	}
+
+	require.True(t, waiting, "add never logged %q", waitingForValue)
+	require.NoError(t, addCmd.Process.Signal(os.Interrupt))
+
+	err = addCmd.Wait()
+
+	var exitErr *exec.ExitError
+
+	require.ErrorAs(t, err, &exitErr)
+	assert.Equal(t, 1, exitErr.ExitCode(), "add ended with %v", err)
+}

internal/cli/generate.go

@@ -155,6 +155,12 @@ func (cli *Instance) GenerateSecret(
 		return fmt.Errorf("failed to generate random secret: %w", err)
 	}
 
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Store the secret in the vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {

internal/cli/init.go

@@ -103,8 +103,21 @@ func (cli *Instance) setupDefaultVault(
 	return vlt, ltIdentity, nil
 }
 
-// Init initializes the secret manager
+// Init initializes the secret manager, holding the state directory lock
+// while initialize runs
 func (cli *Instance) Init(cmd *cobra.Command) error {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	return cli.initialize(cmd)
+}
+
+// initialize creates the state directory, the default vault and its first
+// unlocker
+func (cli *Instance) initialize(cmd *cobra.Command) error {
 	secret.Debug("Starting secret manager initialization")
 
 	// Create state directory

internal/cli/integration_test.go

@@ -829,6 +829,14 @@ func test09GetSpecificVersion(t *testing.T, tempDir, testMnemonic string, runSec
 
 	require.NoError(t, err, "get current version should succeed")
 	assert.Equal(t, "newpassword456", strings.TrimSpace(output), "should return new secret value without --version")
+
+	// An empty --version is not a version; it does not mean the current one
+	output, err = runSecretWithEnv(map[string]string{
+		secret.EnvMnemonic: testMnemonic,
+	}, "get", "--version", "", "database/password")
+
+	require.Error(t, err, "get with an empty version should fail")
+	assert.Contains(t, output, "version '' not found", "should reject the empty version")
 }
 
 func test10PromoteVersion(t *testing.T, tempDir, testMnemonic string, runSecret func(...string) (string, error), runSecretWithEnv func(map[string]string, ...string) (string, error)) {

internal/cli/lock_test.go

@@ -0,0 +1,550 @@
+//nolint:testpackage // sets the unexported fields of Instance
+package cli
+
+import (
+	"bytes"
+	"io"
+	"os"
+	"path/filepath"
+	"runtime"
+	"strconv"
+	"strings"
+	"sync"
+	"testing"
+	"time"
+
+	"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/assert"
+	"github.com/stretchr/testify/require"
+)
+
+const (
+	// lockWait is how long a test waits for something that must happen
+	// once the lock is free.
+	lockWait = 10 * time.Second
+
+	// testPassphrase protects the passphrase unlockers the tests create.
+	testPassphrase = "test-passphrase"
+
+	// testInput is a file outside the state directory that commands read.
+	testInput = "/input"
+)
+
+// lockInBackground starts taking the state directory lock and returns a
+// channel that delivers the function releasing it once it has been taken.
+func lockInBackground(t *testing.T, fs afero.Fs) <-chan func() {
+	t.Helper()
+
+	taken := make(chan func(), 1)
+
+	go func() {
+		release, err := vault.LockStateDir(fs, testStateDir)
+		if assert.NoError(t, err) {
+			taken <- release
+		}
+	}()
+
+	return taken
+}
+
+// addAtOnce runs one add of the secret name per value, all at once, and
+// returns their errors.
+func addAtOnce(
+	fs afero.Fs, stateDir, name string, force bool, values []string,
+) []error {
+	errs := make(chan error, len(values))
+
+	for _, value := range values {
+		go func() {
+			cli := NewCLIInstanceWithStateDir(fs, stateDir)
+			cli.cmd = &cobra.Command{}
+			cli.cmd.SetIn(strings.NewReader(value))
+
+			errs <- cli.AddSecret(name, force)
+		}()
+	}
+
+	results := make([]error, 0, len(values))
+	for range values {
+		results = append(results, <-errs)
+	}
+
+	return results
+}
+
+// numbered returns count distinct values starting with prefix.
+func numbered(prefix string, count int) []string {
+	values := make([]string, 0, count)
+	for i := range count {
+		values = append(values, prefix+"-"+strconv.Itoa(i))
+	}
+
+	return values
+}
+
+// TestConcurrentAddsKeepEveryVersion runs adds of one secret at once, on
+// the in-memory and on the real filesystem. Without the state directory
+// lock, adds of a new secret all find it absent and replace each other, and
+// forced adds read the same highest version number and overwrite each
+// other's version. With it they behave as if run one after another.
+//
+//nolint:paralleltest // t.Setenv forbids parallel subtests
+func TestConcurrentAddsKeepEveryVersion(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	const adds = 8
+
+	for _, tc := range []struct {
+		name     string
+		fs       afero.Fs
+		stateDir string
+	}{
+		{"memory", afero.NewMemMapFs(), testStateDir},
+		{"real", afero.NewOsFs(), t.TempDir()},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := vault.CreateVault(tc.fs, tc.stateDir, "default")
+			require.NoError(t, err)
+
+			// One add creates the secret; the others find that it exists
+			created := 0
+
+			for _, err := range addAtOnce(tc.fs, tc.stateDir, "shared", false,
+				numbered("create", adds)) {
+				if err == nil {
+					created++
+				} else {
+					require.ErrorIs(t, err, vault.ErrSecretExists)
+				}
+			}
+
+			require.Equal(t, 1, created, "exactly one add creates the secret")
+
+			// Every forced add stores a version of its own
+			for _, err := range addAtOnce(tc.fs, tc.stateDir, "shared", true,
+				numbered("force", adds)) {
+				require.NoError(t, err)
+			}
+
+			vlt, err := vault.GetCurrentVault(tc.fs, tc.stateDir)
+			require.NoError(t, err)
+
+			vaultDir, err := vlt.GetDirectory()
+			require.NoError(t, err)
+
+			versions, err := secret.ListVersions(tc.fs,
+				filepath.Join(vaultDir, "secrets.d", "shared"))
+			require.NoError(t, err)
+			require.Len(t, versions, adds+1, "one version per successful add")
+
+			values := make(map[string]bool, len(versions))
+
+			for _, version := range versions {
+				value, err := vlt.GetSecretVersion("shared", version)
+				require.NoError(t, err)
+
+				values[string(value)] = true
+			}
+
+			assert.Len(t, values, adds+1, "every add stored its own value")
+		})
+	}
+}
+
+// readNotifier passes reads through to Reader and closes reading at the
+// first one.
+type readNotifier struct {
+	io.Reader
+
+	reading chan struct{}
+	once    sync.Once
+}
+
+func (r *readNotifier) Read(p []byte) (int, error) {
+	r.once.Do(func() { close(r.reading) })
+
+	return r.Reader.Read(p)
+}
+
+// TestEncryptPipedIntoAdd runs `secret encrypt key | secret add name` in
+// one process, starting encrypt once add is reading its input. Had add
+// taken the state directory lock before reading, it would hold the lock
+// while waiting for encrypt's output, and encrypt would wait for the lock
+// to store its key: neither would finish.
+func TestEncryptPipedIntoAdd(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	fs := afero.NewMemMapFs()
+	_, err := vault.CreateVault(fs, testStateDir, "default")
+	require.NoError(t, err)
+	require.NoError(t, afero.WriteFile(fs, testInput, []byte("piped"), 0o600))
+
+	pipeReader, pipeWriter := io.Pipe()
+	// If the test gives up, this makes add's read fail, so that both
+	// commands return and release the lock the other tests use
+	t.Cleanup(func() { _ = pipeReader.Close() })
+
+	const commands = 2
+
+	input := &readNotifier{Reader: pipeReader, reading: make(chan struct{})}
+	results := make(chan error, commands)
+
+	go func() {
+		add := NewCLIInstanceWithStateDir(fs, testStateDir)
+		add.cmd = &cobra.Command{}
+		add.cmd.SetIn(input)
+
+		results <- add.AddSecret("encrypted", false)
+	}()
+
+	go func() {
+		<-input.reading
+
+		encrypt := NewCLIInstanceWithStateDir(fs, testStateDir)
+		encrypt.cmd = &cobra.Command{}
+		encrypt.cmd.SetOut(pipeWriter)
+
+		err := encrypt.Encrypt("key", testInput, "")
+		// Ends add's input, as the end of the pipe does
+		_ = pipeWriter.CloseWithError(err)
+
+		results <- err
+	}()
+
+	timeout := time.After(lockWait)
+
+	for range commands {
+		select {
+		case err := <-results:
+			require.NoError(t, err)
+		case <-timeout:
+			t.Fatal("secret encrypt piped into secret add never finished")
+		}
+	}
+}
+
+// TestFailedCommandReleasesLock checks that a command failing after it
+// took the state directory lock leaves the lock free for the next command.
+func TestFailedCommandReleasesLock(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
+
+	// Fails once it holds the lock: there is no current vault
+	err := cli.RemoveSecret(&cobra.Command{}, "missing", false)
+	require.Error(t, err)
+
+	select {
+	case release := <-lockInBackground(t, fs):
+		release()
+	case <-time.After(lockWait):
+		t.Fatal("the failed command left the state directory locked")
+	}
+}
+
+// stateDirModTimes returns the modification time of every file and
+// directory under the test state directory. Any change a command makes, even
+// rewriting a file with the same content, changes it.
+func stateDirModTimes(t *testing.T, fs afero.Fs) map[string]int64 {
+	t.Helper()
+
+	modTimes := make(map[string]int64)
+
+	err := afero.Walk(fs, testStateDir,
+		func(path string, info os.FileInfo, err error) error {
+			if err != nil {
+				return err
+			}
+
+			modTimes[path] = info.ModTime().UnixNano()
+
+			return nil
+		})
+	require.NoError(t, err)
+
+	return modTimes
+}
+
+// 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.
+func setupEveryCommand(
+	t *testing.T, fs afero.Fs, withUnlocker bool,
+) (string, string) {
+	t.Helper()
+
+	other, err := vault.CreateVault(fs, testStateDir, "other")
+	require.NoError(t, err)
+
+	otherDir, err := other.GetDirectory()
+	require.NoError(t, err)
+	require.NoError(t, fs.Remove(filepath.Join(otherDir, "pub.age")))
+
+	vlt, err := vault.CreateVault(fs, testStateDir, "default")
+	require.NoError(t, err)
+
+	addTestSecret(t, vlt, []byte("older"), false)
+	addTestSecret(t, vlt, []byte("newer"), true)
+
+	vaultDir, err := vlt.GetDirectory()
+	require.NoError(t, err)
+
+	versions, err := secret.ListVersions(fs,
+		filepath.Join(vaultDir, "secrets.d", "test%secret"))
+	require.NoError(t, err)
+	require.Len(t, versions, 2)
+
+	unlockerID := ""
+
+	if withUnlocker {
+		passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
+		defer passphrase.Destroy()
+
+		unlocker, err := vlt.CreatePassphraseUnlocker(passphrase)
+		require.NoError(t, err)
+
+		unlockerID = unlocker.GetID()
+	}
+
+	require.NoError(t, afero.WriteFile(fs, testInput, []byte("input"), 0o600))
+
+	// Newest first
+	return versions[1], unlockerID
+}
+
+// waitingForLock reports whether a goroutine is stopped in
+// vault.LockStateDir, waiting for the in-memory filesystem's lock. The
+// stack trace of such a goroutine starts with the reason it waits,
+// "[sync.Mutex.Lock]", and names LockStateDir.
+func waitingForLock() bool {
+	stacks := make([]byte, 1<<20)
+	stacks = stacks[:runtime.Stack(stacks, true)]
+
+	for goroutine := range bytes.SplitSeq(stacks, []byte("\n\n")) {
+		if bytes.Contains(goroutine, []byte("[sync.Mutex.Lock")) &&
+			bytes.Contains(goroutine, []byte("vault.LockStateDir(")) {
+			return true
+		}
+	}
+
+	return false
+}
+
+// requireWaitsForLock runs a command, given what setupEveryCommand made,
+// while holding the state directory lock. The command must neither finish
+// nor change anything before it waits for the lock, and must succeed once
+// the lock is released.
+func requireWaitsForLock(
+	t *testing.T,
+	withUnlocker bool,
+	run func(cli *Instance, olderVersion, unlockerID string) error,
+) {
+	t.Helper()
+
+	fs := afero.NewMemMapFs()
+	olderVersion, unlockerID := setupEveryCommand(t, fs, withUnlocker)
+	before := stateDirModTimes(t, fs)
+
+	release, err := vault.LockStateDir(fs, testStateDir)
+	require.NoError(t, err)
+
+	// Released at most once, and also if the test fails while holding it,
+	// so that later tests can take it
+	release = sync.OnceFunc(release)
+	defer release()
+
+	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
+	cli.cmd = &cobra.Command{}
+	cli.cmd.SetIn(strings.NewReader("value"))
+	cli.cmd.SetOut(io.Discard)
+
+	done := make(chan error, 1)
+
+	go func() { done <- run(cli, olderVersion, unlockerID) }()
+
+	timeout := time.After(lockWait)
+
+	for !waitingForLock() {
+		select {
+		case err := <-done:
+			t.Fatalf("finished while the lock was held, with error %v", err)
+		case <-timeout:
+			t.Fatal("never waited for the lock")
+		case <-time.After(time.Millisecond):
+		}
+	}
+
+	assert.Equal(t, before, stateDirModTimes(t, fs),
+		"changed the state directory before waiting for the lock")
+
+	release()
+
+	select {
+	case err := <-done:
+		require.NoError(t, err)
+	case <-time.After(lockWait):
+		t.Fatal("did not finish once the lock was released")
+	}
+}
+
+// TestChangingCommandsWaitForLock checks that each command that changes the
+// state directory waits for its lock.
+//
+//nolint:paralleltest // t.Setenv forbids parallel subtests
+func TestChangingCommandsWaitForLock(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+	t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
+
+	for _, tc := range []struct {
+		name         string
+		withUnlocker bool
+		run          func(cli *Instance, olderVersion, unlockerID string) error
+	}{
+		{"add", false, func(cli *Instance, _, _ string) error {
+			return cli.AddSecret("added", false)
+		}},
+		{"import", false, func(cli *Instance, _, _ string) error {
+			return cli.ImportSecret(cli.cmd, "imported", testInput, false)
+		}},
+		{"generate secret", false, func(cli *Instance, _, _ string) error {
+			return cli.GenerateSecret(cli.cmd, "generated", 16, "base58", false)
+		}},
+		{"encrypt", false, func(cli *Instance, _, _ string) error {
+			return cli.Encrypt("key", testInput, "")
+		}},
+		{"rm", false, func(cli *Instance, _, _ string) error {
+			return cli.RemoveSecret(cli.cmd, "test/secret", false)
+		}},
+		{"move", false, func(cli *Instance, _, _ string) error {
+			return cli.MoveSecret(cli.cmd, "test/secret", "moved", false)
+		}},
+		{"version promote", false, func(cli *Instance, olderVersion, _ string) error {
+			return cli.PromoteVersion(cli.cmd, "test/secret", olderVersion)
+		}},
+		{"version rm", false, func(cli *Instance, olderVersion, _ string) error {
+			return cli.RemoveVersion(cli.cmd, "test/secret", olderVersion)
+		}},
+		{"vault create", false, func(cli *Instance, _, _ string) error {
+			return cli.CreateVault(cli.cmd, "created")
+		}},
+		{"vault select", false, func(cli *Instance, _, _ string) error {
+			return cli.SelectVault(cli.cmd, "other")
+		}},
+		{"vault import", false, func(cli *Instance, _, _ string) error {
+			return cli.VaultImport(cli.cmd, "other")
+		}},
+		{"vault rm", false, func(cli *Instance, _, _ string) error {
+			return cli.RemoveVault(cli.cmd, "other", false)
+		}},
+		{"unlocker add", false, func(cli *Instance, _, _ string) error {
+			return cli.UnlockersAdd("passphrase", cli.cmd)
+		}},
+		{"unlocker rm", true, func(cli *Instance, _, unlockerID string) error {
+			return cli.UnlockersRemove(unlockerID, true, cli.cmd)
+		}},
+		{"unlocker select", true, func(cli *Instance, _, unlockerID string) error {
+			return cli.UnlockerSelect(unlockerID)
+		}},
+		{"init", false, func(cli *Instance, _, _ string) error {
+			return cli.Init(cli.cmd)
+		}},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			requireWaitsForLock(t, tc.withUnlocker, tc.run)
+		})
+	}
+}
+
+// TestEncryptWithExistingKeyTakesNoLock checks that secret encrypt with a
+// key that already exists, which only reads the state directory, finishes
+// while another command holds the state directory lock.
+func TestEncryptWithExistingKeyTakesNoLock(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	fs := afero.NewMemMapFs()
+	_, err := vault.CreateVault(fs, testStateDir, "default")
+	require.NoError(t, err)
+	require.NoError(t, afero.WriteFile(fs, testInput, []byte("input"), 0o600))
+
+	encrypt := NewCLIInstanceWithStateDir(fs, testStateDir)
+	encrypt.cmd = &cobra.Command{}
+	encrypt.cmd.SetOut(io.Discard)
+
+	// Stores the key
+	require.NoError(t, encrypt.Encrypt("key", testInput, ""))
+
+	release, err := vault.LockStateDir(fs, testStateDir)
+	require.NoError(t, err)
+	// Also frees a waiting encrypt if the test fails, so that it releases
+	// the lock the other tests use
+	defer release()
+
+	done := make(chan error, 1)
+
+	go func() { done <- encrypt.Encrypt("key", testInput, "") }()
+
+	select {
+	case err := <-done:
+		require.NoError(t, err)
+	case <-time.After(lockWait):
+		t.Fatal("secret encrypt with an existing key waited for the lock")
+	}
+}
+
+// TestEncryptStreamsUnlocked checks that secret encrypt has released the
+// state directory lock by the time it writes its output. Holding it while
+// streaming would stall every other changing command for as long as the
+// stream lasts, and forever when the other end of the pipe is one of them.
+func TestEncryptStreamsUnlocked(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	fs := afero.NewMemMapFs()
+	_, err := vault.CreateVault(fs, testStateDir, "default")
+	require.NoError(t, err)
+	require.NoError(t, afero.WriteFile(fs, testInput, []byte("streamed"), 0o600))
+
+	outputReader, outputWriter := io.Pipe()
+	done := make(chan error, 1)
+
+	go func() {
+		encrypt := NewCLIInstanceWithStateDir(fs, testStateDir)
+		encrypt.cmd = &cobra.Command{}
+		encrypt.cmd.SetOut(outputWriter)
+
+		err := encrypt.Encrypt("key", testInput, "")
+		_ = outputWriter.CloseWithError(err)
+
+		done <- err
+	}()
+
+	// The first byte of output: encrypt is streaming now, and blocked
+	// writing until it is read
+	_, err = io.ReadFull(outputReader, make([]byte, 1))
+	require.NoError(t, err)
+
+	taken := lockInBackground(t, fs)
+
+	select {
+	case release := <-taken:
+		release()
+	case <-time.After(lockWait):
+		// Let encrypt finish, so that it releases the lock, then free it
+		// again for the tests that follow
+		_, _ = io.Copy(io.Discard, outputReader)
+
+		(<-taken)()
+		t.Fatal("secret encrypt held the lock while streaming")
+	}
+
+	_, err = io.Copy(io.Discard, outputReader)
+	require.NoError(t, err)
+	require.NoError(t, <-done)
+}

internal/cli/move_test.go

@@ -0,0 +1,229 @@
+package cli_test
+
+import (
+	"os"
+	"path/filepath"
+	"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"
+)
+
+// TestRejectedMoveWithinVaultLeavesStateUnchanged is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/73, where a forced move of a secret
+// onto itself deleted it, also when "work" was spelled two ways, and a failed
+// move within "work" left "work" the current vault. "default" is the current
+// vault in every case, and each case runs on its own copy of the state
+// directory.
+//
+//nolint:paralleltest // newTwoVaultFs uses t.Setenv
+func TestRejectedMoveWithinVaultLeavesStateUnchanged(t *testing.T) {
+	before := snapshotStateDir(t, newTwoVaultFs(t))
+	require.Equal(t, "default", before[testStateDir+"/currentvault"])
+
+	const (
+		ontoItself = "secret 'x' cannot be moved onto itself"
+		workX      = "work:x"
+	)
+
+	tests := []struct {
+		command      string
+		source, dest string
+		force        bool
+		wantErr      string
+	}{
+		{"mv x x", "x", "x", false, ontoItself},
+		{"mv --force x x", "x", "x", true, ontoItself},
+		{"mv --force work:x work:", workX, "work:", true, ontoItself},
+		// An empty destination name defaults to the source name.
+		{`mv --force work:x ""`, workX, "", true, ontoItself},
+		// "work" is a vault name, so the destination is work:x.
+		{"mv --force work:x work", workX, "work", true, ontoItself},
+		{
+			"mv work:nosuch work:y", "work:nosuch", "work:y", false,
+			"secret 'nosuch' not found",
+		},
+		// Only an existing vault is used, so ".." cannot reach the state
+		// directory itself.
+		{
+			"mv --force ..:x ..:y", "..:x", "..:y", true,
+			"vault '..' does not exist",
+		},
+		// Each of these spells "work" a second way. The spelling is not an
+		// existing vault name, so the move is not taken for a move between
+		// two vaults, which would delete the destination, here the source.
+		{
+			"mv --force work:x work/:x", workX, "work/:x", true,
+			"vault 'work/' does not exist",
+		},
+		{
+			"mv --force work/:x work:", "work/:x", "work:", true,
+			"vault 'work/' does not exist",
+		},
+		{
+			"mv --force work:x ./work:x", workX, "./work:x", true,
+			"vault './work' does not exist",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.command, func(t *testing.T) {
+			fs := newFsFromSnapshot(t, before)
+			c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
+
+			err := c.MoveSecret(&cobra.Command{}, tt.source, tt.dest, tt.force)
+
+			require.Equal(t, before, snapshotStateDir(t, fs))
+			require.EqualError(t, err, tt.wantErr)
+		})
+	}
+}
+
+// TestMoveWithinOtherVaultKeepsCurrentVault checks that `secret mv work:x
+// work:y`, with "default" the current vault, renames "x" to "y" in "work" and
+// leaves "default" the current vault.
+//
+//nolint:paralleltest // newTwoVaultFs uses t.Setenv
+func TestMoveWithinOtherVaultKeepsCurrentVault(t *testing.T) {
+	fs := newTwoVaultFs(t)
+	c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
+
+	err := c.MoveSecret(&cobra.Command{}, "work:x", "work:y", false)
+	require.NoError(t, err)
+
+	after := snapshotStateDir(t, fs)
+	workSecrets := testStateDir + "/vaults.d/work/secrets.d/"
+
+	require.Equal(t, "default", after[testStateDir+"/currentvault"])
+	require.Contains(t, after, workSecrets+"y/")
+	require.NotContains(t, after, workSecrets+"x/")
+}
+
+// TestMoveOntoSameSecretUnderAnotherNameIsRejected is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/78: on a case-insensitive
+// filesystem "Foo" and "foo" are one secret, and `secret mv --force Foo foo`
+// removed the destination, which was the source. Symbolic links on the real
+// filesystem give one secret two names here: in "default", "y" is a link to
+// the secret "x", and the secrets.d of "other" is a link to that of
+// "default", so other:x is default:x. Each move must be rejected and leave
+// the secret and the links as they were.
+//
+//nolint:paralleltest // t.Setenv
+func TestMoveOntoSameSecretUnderAnotherNameIsRejected(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	const isSame = "is the same secret on this filesystem"
+
+	tests := []struct {
+		command      string
+		source, dest string
+		force        bool
+		wantErr      string
+	}{
+		{
+			"mv --force y x", "y", "x", true,
+			"secret 'y' cannot be moved onto itself: 'x' " + isSame,
+		},
+		{
+			"mv --force x y", "x", "y", true,
+			"secret 'x' cannot be moved onto itself: 'y' " + isSame,
+		},
+		{
+			"mv x y", "x", "y", false,
+			"secret 'x' cannot be moved onto itself: 'y' " + isSame,
+		},
+		{
+			"mv --force default:x other:x", "default:x", "other:x", true,
+			"secret 'default:x' cannot be moved onto itself: 'other:x' " +
+				isSame,
+		},
+		{
+			"mv default:x other", "default:x", "other", false,
+			"secret 'default:x' cannot be moved onto itself: 'other:x' " +
+				isSame,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.command, func(t *testing.T) {
+			fs := afero.NewOsFs()
+			stateDir := t.TempDir()
+			vaultsDir := filepath.Join(stateDir, "vaults.d")
+
+			// "default" is created last, so it is the current vault.
+			_, err := vault.CreateVault(fs, stateDir, "other")
+			require.NoError(t, err)
+
+			vlt, err := vault.CreateVault(fs, stateDir, "default")
+			require.NoError(t, err)
+
+			err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("value")), false)
+			require.NoError(t, err)
+
+			defaultSecrets := filepath.Join(vaultsDir, "default", "secrets.d")
+			otherSecrets := filepath.Join(vaultsDir, "other", "secrets.d")
+			link := filepath.Join(defaultSecrets, "y")
+
+			require.NoError(t, os.Symlink("x", link))
+			require.NoError(t, os.Remove(otherSecrets))
+			require.NoError(t, os.Symlink(defaultSecrets, otherSecrets))
+
+			c := cli.NewCLIInstanceWithStateDir(fs, stateDir)
+			moveErr := c.MoveSecret(&cobra.Command{}, tt.source, tt.dest, tt.force)
+
+			value, err := vlt.GetSecret("x")
+			require.NoError(t, err)
+			require.Equal(t, "value", string(value))
+
+			target, err := os.Readlink(link)
+			require.NoError(t, err)
+			require.Equal(t, "x", target)
+
+			target, err = os.Readlink(otherSecrets)
+			require.NoError(t, err)
+			require.Equal(t, defaultSecrets, target)
+
+			require.EqualError(t, moveErr, tt.wantErr)
+		})
+	}
+}
+
+// TestForcedCaseOnlyMoveOnCaseSensitiveFilesystem checks that where "Foo"
+// and "foo" are two secrets, `secret mv --force Foo foo` still replaces "foo"
+// with "Foo".
+func TestForcedCaseOnlyMoveOnCaseSensitiveFilesystem(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	fs := afero.NewOsFs()
+	stateDir := t.TempDir()
+
+	vlt, err := vault.CreateVault(fs, stateDir, "default")
+	require.NoError(t, err)
+
+	err = vlt.AddSecret("Foo", memguard.NewBufferFromBytes([]byte("upper")), false)
+	require.NoError(t, err)
+
+	_, err = os.Stat(filepath.Join(stateDir, "vaults.d", "default", "secrets.d", "foo"))
+	if err == nil {
+		t.Skip("the temporary directory is on a case-insensitive filesystem")
+	}
+
+	err = vlt.AddSecret("foo", memguard.NewBufferFromBytes([]byte("lower")), false)
+	require.NoError(t, err)
+
+	c := cli.NewCLIInstanceWithStateDir(fs, stateDir)
+	err = c.MoveSecret(&cobra.Command{}, "Foo", "foo", true)
+	require.NoError(t, err)
+
+	value, err := vlt.GetSecret("foo")
+	require.NoError(t, err)
+	require.Equal(t, "upper", string(value))
+
+	_, err = vlt.GetSecret("Foo")
+	require.ErrorIs(t, err, vault.ErrSecretNotFound)
+}

internal/cli/path_traversal_test.go

@@ -0,0 +1,366 @@
+package cli_test
+
+import (
+	"fmt"
+	"maps"
+	"os"
+	"slices"
+	"strings"
+	"sync"
+	"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"
+)
+
+const (
+	// testStateDir is the in-memory state directory of the test vaults.
+	testStateDir = "/test/state"
+
+	// testPassphrase protects the passphrase unlocker of each test vault.
+	testPassphrase = "test-passphrase"
+
+	// testVersion is a version name in the format the vault uses.
+	testVersion = "20260101.001"
+
+	// missingFile is an import source that does not exist, so an import
+	// that opened it before checking the name would fail with another error.
+	missingFile = "/no/such/file"
+)
+
+// The state directory newTwoVaultFs copies, recorded by snapshotStateDir.
+// Creating a passphrase unlocker is slow by design, so the vaults are made
+// once, by the first test that needs them.
+//
+//nolint:gochecknoglobals // shared by the tests that use newTwoVaultFs
+var (
+	twoVaultsOnce sync.Once
+	twoVaults     map[string]string
+)
+
+// newTwoVaultFs returns an in-memory filesystem holding the vaults "work"
+// and "default", the current one. Each holds the secret "x" and a
+// passphrase unlocker, so both secrets.d and unlockers.d have contents.
+// Every call returns a new copy of the same vaults.
+//
+//nolint:ireturn // afero.Fs is the filesystem abstraction used throughout
+func newTwoVaultFs(t *testing.T) afero.Fs {
+	t.Helper()
+
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	twoVaultsOnce.Do(func() {
+		fs := afero.NewMemMapFs()
+
+		for _, name := range []string{"work", "default"} {
+			vlt, err := vault.CreateVault(fs, testStateDir, name)
+			require.NoError(t, err)
+
+			err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("value")), false)
+			require.NoError(t, err)
+
+			_, err = vlt.CreatePassphraseUnlocker(
+				memguard.NewBufferFromBytes([]byte(testPassphrase)))
+			require.NoError(t, err)
+		}
+
+		twoVaults = snapshotStateDir(t, fs)
+	})
+
+	require.NotNil(t, twoVaults, "making the vaults failed in an earlier test")
+
+	return newFsFromSnapshot(t, twoVaults)
+}
+
+// snapshotStateDir maps every file under the state directory to its
+// contents, and every directory, written with a trailing "/", to "". Two
+// snapshots are equal only if nothing in it was added, removed or changed.
+func snapshotStateDir(t *testing.T, fs afero.Fs) map[string]string {
+	t.Helper()
+
+	tree := map[string]string{}
+
+	err := afero.Walk(fs, testStateDir, func(
+		path string, info os.FileInfo, err error,
+	) error {
+		if err != nil {
+			return err
+		}
+
+		if info.IsDir() {
+			tree[path+"/"] = ""
+
+			return nil
+		}
+
+		content, err := afero.ReadFile(fs, path)
+		if err != nil {
+			return err
+		}
+
+		tree[path] = string(content)
+
+		return nil
+	})
+	require.NoError(t, err)
+
+	return tree
+}
+
+// newFsFromSnapshot returns a new in-memory filesystem holding exactly the
+// directories and files recorded by snapshotStateDir.
+//
+//nolint:ireturn // afero.Fs is the filesystem abstraction used throughout
+func newFsFromSnapshot(t *testing.T, tree map[string]string) afero.Fs {
+	t.Helper()
+
+	fs := afero.NewMemMapFs()
+
+	// In sorted order every directory comes before its contents.
+	for _, path := range slices.Sorted(maps.Keys(tree)) {
+		dir, isDir := strings.CutSuffix(path, "/")
+		if isDir {
+			require.NoError(t, fs.MkdirAll(dir, secret.DirPerms))
+
+			continue
+		}
+
+		err := afero.WriteFile(fs, path, []byte(tree[path]), secret.FilePerms)
+		require.NoError(t, err)
+	}
+
+	return fs
+}
+
+// requireRejectedAndUnchanged runs a command on a copy of the state
+// directory recorded in before. It requires an error with exactly the
+// message of want, so that a later check rejecting the argument does not
+// count, and everything under the state directory as it was: the error
+// alone proves nothing, since it could come after the vault had already
+// been deleted.
+func requireRejectedAndUnchanged(
+	t *testing.T, before map[string]string, want error,
+	run func(c *cli.Instance) error,
+) {
+	t.Helper()
+
+	fs := newFsFromSnapshot(t, before)
+
+	err := run(cli.NewCLIInstanceWithStateDir(fs, testStateDir))
+
+	require.Equal(t, before, snapshotStateDir(t, fs))
+	require.EqualError(t, err, want.Error())
+}
+
+// TestInvalidSecretNameLeavesVaultsUnchanged is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/33, where `secret rm ..` deleted
+// the whole vault, and `secret rm .` or `secret rm ""` every secret in it.
+// Moves and imports use --force, so that only the name check stands in
+// the way.
+//
+//nolint:paralleltest // newTwoVaultFs uses t.Setenv
+func TestInvalidSecretNameLeavesVaultsUnchanged(t *testing.T) {
+	// Creating a passphrase unlocker is slow by design, so the vaults are
+	// created once and each case runs on its own copy of them.
+	before := snapshotStateDir(t, newTwoVaultFs(t))
+
+	vaultDir := testStateDir + "/vaults.d/default"
+	require.Contains(t, before, vaultDir+"/secrets.d/x/")
+	require.Contains(t, before, vaultDir+"/unlockers.d/passphrase/")
+	require.Equal(t, "default", before[testStateDir+"/currentvault"])
+
+	cmd := &cobra.Command{}
+
+	tests := []struct {
+		command  string
+		rejected string // the secret name the command must reject
+		run      func(c *cli.Instance) error
+	}{
+		{"rm ..", "..", func(c *cli.Instance) error {
+			return c.RemoveSecret(cmd, "..", false)
+		}},
+		{"rm .", ".", func(c *cli.Instance) error {
+			return c.RemoveSecret(cmd, ".", false)
+		}},
+		{`rm ""`, "", func(c *cli.Instance) error {
+			return c.RemoveSecret(cmd, "", false)
+		}},
+		{"rm ../../etc", "../../etc", func(c *cli.Instance) error {
+			return c.RemoveSecret(cmd, "../../etc", false)
+		}},
+		{"mv --force .. x", "..", func(c *cli.Instance) error {
+			return c.MoveSecret(cmd, "..", "x", true)
+		}},
+		{"mv --force x ..", "..", func(c *cli.Instance) error {
+			return c.MoveSecret(cmd, "x", "..", true)
+		}},
+		{`mv --force x ""`, "", func(c *cli.Instance) error {
+			return c.MoveSecret(cmd, "x", "", true)
+		}},
+		// "work" is not the current vault: a move within it must not
+		// select it when a name is rejected.
+		{"mv --force work:.. work:x", "..", func(c *cli.Instance) error {
+			return c.MoveSecret(cmd, "work:..", "work:x", true)
+		}},
+		{"mv --force work:x work:..", "..", func(c *cli.Instance) error {
+			return c.MoveSecret(cmd, "work:x", "work:..", true)
+		}},
+		{"mv --force default:.. work", "..", func(c *cli.Instance) error {
+			return c.MoveSecret(cmd, "default:..", "work", true)
+		}},
+		{"mv --force default:.. work:y", "..", func(c *cli.Instance) error {
+			return c.MoveSecret(cmd, "default:..", "work:y", true)
+		}},
+		{"mv --force default:x work:..", "..", func(c *cli.Instance) error {
+			return c.MoveSecret(cmd, "default:x", "work:..", true)
+		}},
+		{"import --force ..", "..", func(c *cli.Instance) error {
+			return c.ImportSecret(cmd, "..", missingFile, true)
+		}},
+		{"import --force .", ".", func(c *cli.Instance) error {
+			return c.ImportSecret(cmd, ".", missingFile, true)
+		}},
+		{"import --force ../../etc", "../../etc", func(c *cli.Instance) error {
+			return c.ImportSecret(cmd, "../../etc", missingFile, true)
+		}},
+		{"version list ..", "..", func(c *cli.Instance) error {
+			return c.ListVersions(cmd, "..")
+		}},
+		{"version promote ..", "..", func(c *cli.Instance) error {
+			return c.PromoteVersion(cmd, "..", testVersion)
+		}},
+		{"version rm ..", "..", func(c *cli.Instance) error {
+			return c.RemoveVersion(cmd, "..", testVersion)
+		}},
+		{"encrypt ..", "..", func(c *cli.Instance) error {
+			return c.Encrypt("..", "", "")
+		}},
+		{"decrypt ..", "..", func(c *cli.Instance) error {
+			return c.Decrypt("..", "", "")
+		}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.command, func(t *testing.T) {
+			requireRejectedAndUnchanged(t, before, vault.ValidateSecretName(tt.rejected), tt.run)
+		})
+	}
+}
+
+// TestInvalidVersionLeavesVaultsUnchanged is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/67, where
+// `secret version rm x ../../..` deleted the whole vault,
+// `secret version rm x ..` the secret x, and `secret version rm x .` or
+// `secret version rm x ""` every version of x. A version argument is
+// accepted only if it is one of the versions `secret version list` lists.
+//
+//nolint:paralleltest // newTwoVaultFs uses t.Setenv
+func TestInvalidVersionLeavesVaultsUnchanged(t *testing.T) {
+	before := snapshotStateDir(t, newTwoVaultFs(t))
+
+	cmd := &cobra.Command{}
+
+	commands := []struct {
+		command string
+		run     func(c *cli.Instance, version string) error
+	}{
+		{"version rm x", func(c *cli.Instance, version string) error {
+			return c.RemoveVersion(cmd, "x", version)
+		}},
+		{"version promote x", func(c *cli.Instance, version string) error {
+			return c.PromoteVersion(cmd, "x", version)
+		}},
+		{"get x --version", func(c *cli.Instance, version string) error {
+			return c.GetSecretWithVersion(cmd, "x", version)
+		}},
+	}
+
+	for _, tt := range commands {
+		for _, version := range []string{"", ".", "..", "../../..", "a/b"} {
+			t.Run(fmt.Sprintf("%s %q", tt.command, version), func(t *testing.T) {
+				want := fmt.Errorf("version '%s' %w '%s'",
+					version, vault.ErrVersionNotFound, "x")
+				requireRejectedAndUnchanged(t, before, want,
+					func(c *cli.Instance) error { return tt.run(c, version) })
+			})
+		}
+	}
+}
+
+// TestRemoveVersionRemovesOnlyThatVersion checks that `secret version rm`
+// with a version that is not the current one removes that version and
+// changes nothing else.
+//
+//nolint:paralleltest // newTwoVaultFs uses t.Setenv
+func TestRemoveVersionRemovesOnlyThatVersion(t *testing.T) {
+	fs := newTwoVaultFs(t)
+
+	vlt, err := vault.GetCurrentVault(fs, testStateDir)
+	require.NoError(t, err)
+
+	// A second version of "x" becomes the current one.
+	err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("new")), true)
+	require.NoError(t, err)
+
+	secretDir := testStateDir + "/vaults.d/default/secrets.d/x"
+	versions, err := secret.ListVersions(fs, secretDir)
+	require.NoError(t, err)
+	require.Len(t, versions, 2)
+
+	// ListVersions lists the newest version first.
+	oldDir := secretDir + "/versions/" + versions[1] + "/"
+	before := snapshotStateDir(t, fs)
+	require.Contains(t, before, oldDir)
+
+	c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
+	err = c.RemoveVersion(&cobra.Command{}, "x", versions[1])
+	require.NoError(t, err)
+
+	// Expected: the state as before without everything under oldDir.
+	want := map[string]string{}
+
+	for path, content := range before {
+		if !strings.HasPrefix(path, oldDir) {
+			want[path] = content
+		}
+	}
+
+	require.Equal(t, want, snapshotStateDir(t, fs))
+}
+
+// TestMoveToVaultNameRenamesInCurrentVault checks that `secret mv x work`,
+// where "work" is also the name of a vault, renames the secret "x" to "work"
+// in the current vault and changes nothing else.
+//
+//nolint:paralleltest // newTwoVaultFs uses t.Setenv
+func TestMoveToVaultNameRenamesInCurrentVault(t *testing.T) {
+	before := snapshotStateDir(t, newTwoVaultFs(t))
+	fs := newFsFromSnapshot(t, before)
+
+	c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
+	err := c.MoveSecret(&cobra.Command{}, "x", "work", false)
+	require.NoError(t, err)
+
+	// Expected: the state as before, with everything under the current
+	// vault's secrets.d/x/ now under secrets.d/work/.
+	oldDir := testStateDir + "/vaults.d/default/secrets.d/x/"
+	newDir := testStateDir + "/vaults.d/default/secrets.d/work/"
+	want := map[string]string{}
+
+	for path, content := range before {
+		rest, found := strings.CutPrefix(path, oldDir)
+		if found {
+			path = newDir + rest
+		}
+
+		want[path] = content
+	}
+
+	require.Contains(t, want, newDir)
+	require.Equal(t, want, snapshotStateDir(t, fs))
+}

internal/cli/root.go

@@ -4,17 +4,38 @@ import (
 	"os"
 
 	"git.eeqj.de/sneak/secret/internal/secret"
+	"github.com/awnumar/memguard"
 	"github.com/spf13/cobra"
+	"golang.org/x/sys/unix"
+	"golang.org/x/term"
 )
 
-// Entry is the entry point for the secret CLI application
+// Entry runs the secret CLI and returns the process exit code. It wipes
-func Entry() {
+// every memguard buffer before it returns, so the caller must do nothing
-	cmd := newRootCmd()
+// but exit with the code.
+func Entry() int {
+	// On SIGINT or SIGTERM memguard runs this function, wipes every buffer
+	// and exits with status 1. The passphrase prompt turns terminal echo
+	// off until the read finishes, so a signal there would leave echo off.
+	// Only a process in the terminal's foreground process group may reset
+	// it: one in the background that tries is stopped instead of exiting.
+	terminalState, terminalErr := term.GetState(unix.Stdin)
 
-	err := cmd.Execute()
+	memguard.CatchSignal(func(os.Signal) {
+		foreground, err := unix.IoctlGetInt(unix.Stdin, unix.TIOCGPGRP)
+		if terminalErr == nil && err == nil && foreground == unix.Getpgrp() {
+			_ = term.Restore(unix.Stdin, terminalState)
+		}
+	}, os.Interrupt, unix.SIGTERM)
+
+	defer memguard.Purge()
+
+	err := newRootCmd().Execute()
 	if err != nil {
-		os.Exit(1)
+		return 1
 	}
+
+	return 0
 }
 
 func newRootCmd() *cobra.Command {

internal/cli/secrets.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"log"
+	"os"
 	"path/filepath"
 	"slices"
 	"strings"
@@ -40,6 +41,7 @@ var (
 	errVaultDoesNotExist           = errors.New("does not exist")
 	errCrossVaultSourceUnqualified = errors.New(
 		"source must specify vault (e.g., vault:secret) for cross-vault move")
+	errMoveOntoItself = errors.New("cannot be moved onto itself")
 )
 
 // bufferInfo tracks a protected buffer and the number of bytes used in it
@@ -109,6 +111,12 @@ func newGetCmd() *cobra.Command {
 				return fmt.Errorf("failed to initialize CLI: %w", err)
 			}
 
+			// Without --version, get the current version. A given
+			// --version is checked as typed, so an empty one is rejected.
+			if !cmd.Flags().Changed("version") {
+				return cli.GetSecret(cmd, args[0])
+			}
+
 			return cli.GetSecretWithVersion(cmd, args[0], version)
 		},
 	}
@@ -377,6 +385,15 @@ func (cli *Instance) AddSecret(secretName string, force bool) error {
 	valueBuffer := combineBuffers(buffers, totalSize)
 	defer valueBuffer.Destroy()
 
+	// Locked only now that stdin has been read: in `secret encrypt key |
+	// secret add name`, holding the lock while reading would leave each
+	// command waiting for the other.
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Add the secret to the vault
 	secret.Debug("Calling vault.AddSecret", "secret_name", secretName,
 		"value_length", valueBuffer.Size(), "force", force)
@@ -393,12 +410,32 @@ func (cli *Instance) AddSecret(secretName string, force bool) error {
 	return nil
 }
 
-// GetSecret retrieves and prints a secret from the current vault
+// GetSecret retrieves and prints the current version of a secret
 func (cli *Instance) GetSecret(cmd *cobra.Command, secretName string) error {
-	return cli.GetSecretWithVersion(cmd, secretName, "")
+	secret.Debug("GetSecret called", "secretName", secretName)
+
+	// Store the command for output
+	cli.cmd = cmd
+
+	// Get current vault
+	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+
+	value, err := vlt.GetSecret(secretName)
+	if err != nil {
+		return err
+	}
+
+	// Print the secret value to stdout
+	_, _ = cli.Print(string(value))
+
+	return nil
 }
 
-// GetSecretWithVersion retrieves and prints a specific version of a secret
+// GetSecretWithVersion retrieves and prints a specific version of a secret.
+// The version must be one of the secret's versions.
 func (cli *Instance) GetSecretWithVersion(
 	cmd *cobra.Command, secretName string, version string,
 ) error {
@@ -417,13 +454,7 @@ func (cli *Instance) GetSecretWithVersion(
 	}
 
 	// Get the secret value
-	var value []byte
+	value, err := vlt.GetSecretVersion(secretName, version)
-	if version == "" {
-		value, err = vlt.GetSecret(secretName)
-	} else {
-		value, err = vlt.GetSecretVersion(secretName, version)
-	}
-
 	if err != nil {
 		secret.Debug("Failed to get secret", "error", err)
 
@@ -603,6 +634,11 @@ func printSecretsTable(
 func (cli *Instance) ImportSecret(
 	cmd *cobra.Command, secretName, sourceFile string, force bool,
 ) error {
+	err := vault.ValidateSecretName(secretName)
+	if err != nil {
+		return err
+	}
+
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -635,6 +671,14 @@ func (cli *Instance) ImportSecret(
 	valueBuffer := combineBuffers(buffers, totalSize)
 	defer valueBuffer.Destroy()
 
+	// Locked only now that the file has been read, as in AddSecret: the
+	// file may be a pipe written by another secret command.
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Store the secret in the vault
 	err = vlt.AddSecret(secretName, valueBuffer, force)
 	if err != nil {
@@ -649,6 +693,17 @@ func (cli *Instance) ImportSecret(
 
 // RemoveSecret removes a secret from the vault
 func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool) error {
+	err := vault.ValidateSecretName(secretName)
+	if err != nil {
+		return err
+	}
+
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Get current vault
 	currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -683,7 +738,7 @@ func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool)
 	}
 
 	// Remove the secret directory
-	err = cli.fs.RemoveAll(secretDir)
+	err = secret.RemoveDirAtomic(cli.fs, secretDir)
 	if err != nil {
 		return fmt.Errorf("failed to remove secret: %w", err)
 	}
@@ -694,21 +749,31 @@ func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool)
 	return nil
 }
 
-// MoveSecret moves or renames a secret (within or across vaults)
+// MoveSecret moves or renames a secret (within or across vaults), holding
+// the state directory lock.
 func (cli *Instance) MoveSecret(
 	cmd *cobra.Command, source, dest string, force bool,
+) error {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	return cli.moveSecret(cmd, source, dest, force)
+}
+
+// moveSecret does the work of MoveSecret. The caller holds the state
+// directory lock.
+func (cli *Instance) moveSecret(
+	cmd *cobra.Command, source, dest string, force bool,
 ) error {
 	// Parse source and destination
 	srcVaultName, srcSecretName, srcQualified := ParseVaultSecretRef(source)
 	destVaultName, destSecretName, destQualified := ParseVaultSecretRef(dest)
 
-	// If neither is qualified, this is a simple within-vault rename
-	if !srcQualified && !destQualified {
-		return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force)
-	}
-
 	// Cross-vault move requires source to be qualified
-	if !srcQualified {
+	if !srcQualified && destQualified {
 		return errCrossVaultSourceUnqualified
 	}
 
@@ -716,53 +781,97 @@ func (cli *Instance) MoveSecret(
 	// Format: "work:secret default" means move to vault "default"
 	// Format: "work:secret default:newname" means move to vault "default"
 	// with a new name
-	if !destQualified {
+	if srcQualified && !destQualified {
 		// Check if dest is actually a vault name
-		vaults, err := vault.ListVaults(cli.fs, cli.stateDir)
+		_, err := cli.existingVault(dest)
-		if err == nil && slices.Contains(vaults, dest) {
+		if err == nil {
 			// dest is a vault name, use source secret name
 			destVaultName = dest
 			destSecretName = srcSecretName
-		}
+		} else {
-
+			// dest is a secret name in source vault
-		// If destVaultName is still empty, dest is a secret name in source vault
-		if destVaultName == "" {
 			destVaultName = srcVaultName
-			destSecretName = dest
 		}
 	}
 
-	// If destination secret name is empty, use source secret name
+	// If destination secret name is empty, use source secret name. A plain
-	if destSecretName == "" {
+	// rename keeps it empty, so that the check below rejects it.
+	if srcQualified && destSecretName == "" {
 		destSecretName = srcSecretName
 	}
 
-	// Same vault? Use simple rename if possible (optimization)
+	// Check both names, for every form of the move, before building any path
-	if srcVaultName == destVaultName {
+	// from them.
-		// Select the vault and do a simple move
+	err := vault.ValidateSecretName(srcSecretName)
-		err := vault.SelectVault(cli.fs, cli.stateDir, srcVaultName)
-		if err != nil {
-			return fmt.Errorf("failed to select vault '%s': %w", srcVaultName, err)
-		}
-
-		return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force)
-	}
-
-	// Cross-vault move
-	return cli.moveSecretCrossVault(
-		cmd, srcVaultName, srcSecretName, destVaultName, destSecretName, force)
-}
-
-// moveSecretWithinVault handles rename within the current vault
-func (cli *Instance) moveSecretWithinVault(
-	cmd *cobra.Command, source, dest string, force bool,
-) error {
-	currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
 		return err
 	}
 
-	vaultDir, err := currentVlt.GetDirectory()
+	err = vault.ValidateSecretName(destSecretName)
+	if err != nil {
+		return err
+	}
+
+	// Neither name is qualified: a rename within the current vault.
+	if !srcQualified {
+		vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
+		if err != nil {
+			return err
+		}
+
+		return cli.moveSecretWithinVault(
+			cmd, vlt, srcSecretName, destSecretName, force)
+	}
+
+	// Both vaults must be existing vaults by exact name, so that two
+	// spellings of one vault, such as "work" and "work/", are never taken for
+	// two vaults. A named vault does not become the current vault.
+	srcVault, err := cli.existingVault(srcVaultName)
+	if err != nil {
+		return err
+	}
+
+	destVault, err := cli.existingVault(destVaultName)
+	if err != nil {
+		return err
+	}
+
+	if srcVaultName == destVaultName {
+		return cli.moveSecretWithinVault(
+			cmd, srcVault, srcSecretName, destSecretName, force)
+	}
+
+	return cli.moveSecretCrossVault(
+		cmd, srcVault, srcSecretName, destVault, destSecretName, force)
+}
+
+// existingVault returns the vault with the given name, or an error if there
+// is none. Unlike vault.SelectVault, it leaves the current vault as it is.
+func (cli *Instance) existingVault(name string) (*vault.Vault, error) {
+	vaults, err := vault.ListVaults(cli.fs, cli.stateDir)
+	if err != nil {
+		return nil, fmt.Errorf("failed to list vaults: %w", err)
+	}
+
+	if !slices.Contains(vaults, name) {
+		return nil, fmt.Errorf("vault '%s' %w", name, errVaultDoesNotExist)
+	}
+
+	return vault.NewVault(cli.fs, cli.stateDir, name), nil
+}
+
+// moveSecretWithinVault renames a secret within the vault vlt. Its caller,
+// MoveSecret, has already checked both secret names.
+func (cli *Instance) moveSecretWithinVault(
+	cmd *cobra.Command, vlt *vault.Vault, source, dest string, force bool,
+) error {
+	// With --force the destination is removed before the source is renamed
+	// onto it, which would delete the secret.
+	if source == dest {
+		return fmt.Errorf("secret '%s' %w", source, errMoveOntoItself)
+	}
+
+	vaultDir, err := vlt.GetDirectory()
 	if err != nil {
 		return err
 	}
@@ -782,6 +891,18 @@ func (cli *Instance) moveSecretWithinVault(
 	destEncoded := strings.ReplaceAll(dest, "/", "%")
 	destDir := filepath.Join(vaultDir, "secrets.d", destEncoded)
 
+	// Removing a destination that is the source under another name, such as
+	// "foo" for "Foo" on a case-insensitive filesystem, would delete it too.
+	same, err := cli.sameDirectory(sourceDir, destDir)
+	if err != nil {
+		return err
+	}
+
+	if same {
+		return fmt.Errorf("secret '%s' %w: '%s' is the same secret on "+
+			"this filesystem", source, errMoveOntoItself, dest)
+	}
+
 	exists, err = afero.DirExists(cli.fs, destDir)
 	if err != nil {
 		return fmt.Errorf("failed to check if destination secret exists: %w", err)
@@ -792,7 +913,7 @@ func (cli *Instance) moveSecretWithinVault(
 			return fmt.Errorf("secret '%s' %w", dest, errSecretExistsNoForce)
 		}
 
-		err = cli.fs.RemoveAll(destDir)
+		err = secret.RemoveDirAtomic(cli.fs, destDir)
 		if err != nil {
 			return fmt.Errorf("failed to remove existing destination: %w", err)
 		}
@@ -808,56 +929,80 @@ func (cli *Instance) moveSecretWithinVault(
 	return nil
 }
 
-// moveSecretCrossVault handles moving between different vaults
+// sameDirectory reports whether the existing directory dir and the path
+// other are one directory under two names, as secrets.d/Foo and
+// secrets.d/foo are on a case-insensitive filesystem, or a directory and a
+// symbolic link to it. Removing other to make room for dir would then delete
+// dir. It is false if other does not exist, and always false on the
+// in-memory filesystem, which has no such aliasing and whose files
+// os.SameFile does not compare.
+func (cli *Instance) sameDirectory(dir, other string) (bool, error) {
+	dirInfo, err := cli.fs.Stat(dir)
+	if err != nil {
+		return false, fmt.Errorf("failed to check %s: %w", dir, err)
+	}
+
+	otherInfo, err := cli.fs.Stat(other)
+	if errors.Is(err, os.ErrNotExist) {
+		return false, nil
+	}
+
+	if err != nil {
+		return false, fmt.Errorf("failed to check %s: %w", other, err)
+	}
+
+	return os.SameFile(dirInfo, otherInfo), nil
+}
+
+// moveSecretCrossVault handles moving between two different vaults. Its
+// caller, MoveSecret, has already checked both secret names and that both
+// vaults exist.
 func (cli *Instance) moveSecretCrossVault(
 	cmd *cobra.Command,
-	srcVaultName, srcSecretName,
+	srcVault *vault.Vault, srcSecretName string,
-	destVaultName, destSecretName string,
+	destVault *vault.Vault, destSecretName string,
 	force bool,
 ) error {
-	// Get source vault
-	srcVault := vault.NewVault(cli.fs, cli.stateDir, srcVaultName)
-
 	srcVaultDir, err := srcVault.GetDirectory()
 	if err != nil {
 		return fmt.Errorf("failed to get source vault directory: %w", err)
 	}
 
-	// Verify source vault exists
-	exists, err := afero.DirExists(cli.fs, srcVaultDir)
-	if err != nil || !exists {
-		return fmt.Errorf("source vault '%s' %w", srcVaultName, errVaultDoesNotExist)
-	}
-
 	// Verify source secret exists
 	srcStorageName := strings.ReplaceAll(srcSecretName, "/", "%")
 	srcSecretDir := filepath.Join(srcVaultDir, "secrets.d", srcStorageName)
 
-	exists, err = afero.DirExists(cli.fs, srcSecretDir)
+	exists, err := afero.DirExists(cli.fs, srcSecretDir)
 	if err != nil || !exists {
 		return fmt.Errorf("secret '%s' %w in vault '%s'",
-			srcSecretName, errSecretNotFound, srcVaultName)
+			srcSecretName, errSecretNotFound, srcVault.Name)
 	}
 
-	// Get destination vault
+	// The source is removed after the copy, so a destination that is the
-	destVault := vault.NewVault(cli.fs, cli.stateDir, destVaultName)
+	// source under another name would be lost with it.
-
 	destVaultDir, err := destVault.GetDirectory()
 	if err != nil {
 		return fmt.Errorf("failed to get destination vault directory: %w", err)
 	}
 
-	// Verify destination vault exists
+	destStorageName := strings.ReplaceAll(destSecretName, "/", "%")
-	exists, err = afero.DirExists(cli.fs, destVaultDir)
+	destSecretDir := filepath.Join(destVaultDir, "secrets.d", destStorageName)
-	if err != nil || !exists {
+
-		return fmt.Errorf("destination vault '%s' %w",
+	same, err := cli.sameDirectory(srcSecretDir, destSecretDir)
-			destVaultName, errVaultDoesNotExist)
+	if err != nil {
+		return err
+	}
+
+	if same {
+		return fmt.Errorf("secret '%s:%s' %w: '%s:%s' is the same secret on "+
+			"this filesystem", srcVault.Name, srcSecretName, errMoveOntoItself,
+			destVault.Name, destSecretName)
 	}
 
 	// Unlock destination vault (will fail if neither mnemonic nor unlocker available)
 	_, err = destVault.GetOrDeriveLongTermKey()
 	if err != nil {
-		return fmt.Errorf("failed to unlock destination vault '%s': %w", destVaultName, err)
+		return fmt.Errorf("failed to unlock destination vault '%s': %w", destVault.Name, err)
 	}
 
 	// Count versions for user feedback
@@ -872,18 +1017,18 @@ func (cli *Instance) moveSecretCrossVault(
 	}
 
 	// Delete source secret
-	err = cli.fs.RemoveAll(srcSecretDir)
+	err = secret.RemoveDirAtomic(cli.fs, srcSecretDir)
 	if err != nil {
 		// Copy succeeded but delete failed - warn but don't fail
 		cmd.Printf("Warning: copied secret but failed to remove source: %v\n", err)
 		cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",
-			srcVaultName, srcSecretName, destVaultName, destSecretName, versionCount)
+			srcVault.Name, srcSecretName, destVault.Name, destSecretName, versionCount)
 
 		return nil
 	}
 
 	cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",
-		srcVaultName, srcSecretName, destVaultName, destSecretName, versionCount)
+		srcVault.Name, srcSecretName, destVault.Name, destSecretName, versionCount)
 
 	return nil
 }

internal/cli/secrets_size_test.go

@@ -17,11 +17,51 @@ import (
 	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/unix"
 )
 
 // testVaultName is the vault name used by the size tests.
 const testVaultName = "test-vault"
 
+// lockedBytesPerSecretByte bounds the locked memory that storing a secret
+// holds at once: the buffers it is read into reach up to 1.5 times its
+// size, and they are then copied into one more buffer of its size.
+const lockedBytesPerSecretByte = 3
+
+// skipIfLockedMemoryTooLow skips the test when this process cannot lock
+// the memory a secret of size bytes needs, found by locking a buffer of
+// that size and releasing it. memguard panics, ending the whole test run,
+// when it cannot lock a buffer, and a plain `docker build .` runs the
+// tests under an 8 MiB locked-memory limit (RLIMIT_MEMLOCK). A process
+// allowed to lock past that limit runs every case.
+func skipIfLockedMemoryTooLow(t *testing.T, size int) {
+	t.Helper()
+
+	need := lockedBytesPerSecretByte * size
+
+	buf, err := unix.Mmap(-1, 0, need,
+		unix.PROT_READ|unix.PROT_WRITE, unix.MAP_PRIVATE|unix.MAP_ANON)
+	require.NoError(t, err)
+
+	lockErr := unix.Mlock(buf)
+
+	// Unmapping the buffer also unlocks it.
+	err = unix.Munmap(buf)
+	require.NoError(t, err)
+
+	if lockErr != nil {
+		var limit unix.Rlimit
+
+		err = unix.Getrlimit(unix.RLIMIT_MEMLOCK, &limit)
+		require.NoError(t, err)
+
+		t.Skipf("a %d-byte secret needs up to %d bytes of locked memory, "+
+			"which could not be locked under the locked-memory limit "+
+			"(RLIMIT_MEMLOCK) of %d bytes: %v",
+			size, need, limit.Cur, lockErr)
+	}
+}
+
 // newSizeTestVault creates an in-memory vault unlocked with the test
 // mnemonic and returns the filesystem and vault.
 //
@@ -59,6 +99,7 @@ func newSizeTestVault(t *testing.T) (afero.Fs, *vault.Vault) {
 // verifies the outcome.
 func runAddSecretSizeCase(t *testing.T, size int, wantErr bool, errMsg string) {
 	t.Helper()
+	skipIfLockedMemoryTooLow(t, size)
 
 	fs, vlt := newSizeTestVault(t)
 
@@ -110,6 +151,7 @@ func runAddSecretSizeCase(t *testing.T, size int, wantErr bool, errMsg string) {
 // verifies the outcome.
 func runImportSecretSizeCase(t *testing.T, size int, wantErr bool, errMsg string) {
 	t.Helper()
+	skipIfLockedMemoryTooLow(t, size)
 
 	fs, vlt := newSizeTestVault(t)
 
@@ -300,6 +342,8 @@ func TestAddSecretBufferGrowth(t *testing.T) {
 
 	for _, size := range sizes {
 		t.Run(fmt.Sprintf("size_%d", size), func(t *testing.T) {
+			skipIfLockedMemoryTooLow(t, size)
+
 			fs, vlt := newSizeTestVault(t)
 
 			// Create test data of exactly the specified size

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
@@ -346,9 +345,14 @@ func unlockerIDFromDir(
 // stored metadata matches the given type and creation time and returns
 // the matching unlocker's ID. It returns ("", nil) when the directory is
 // readable but holds no match, and a non-nil error when the directory
-// itself cannot be read. Callers must distinguish the two: an unreadable
+// itself cannot be read, which means the unlocker's ID cannot be known.
-// directory means the unlocker's real ID is unknowable, so the entry has
+// `unlocker list` then skips the entry rather than show a made-up ID; the
-// to be skipped rather than reported under a synthesized ID.
+// duplicate check before adding an unlocker must stop instead, because
+// the skipped entry may be the duplicate.
+//
+// A metadata file that cannot be read or parsed is skipped without a
+// warning: every caller gets metadata from vault.ListUnlockers first,
+// which has already warned about that directory.
 func findUnlockerIDByMetadata(
 	fs afero.Fs, unlockersDir string, metadata secret.UnlockerMetadata,
 	includeSecureEnclave bool,
@@ -371,9 +375,6 @@ func findUnlockerIDByMetadata(
 		// Check if this is the right unlocker by comparing metadata
 		metadataBytes, err := afero.ReadFile(fs, metadataPath)
 		if err != nil {
-			secret.Warn("Could not read unlocker metadata file",
-				"path", metadataPath, "error", err)
-
 			continue
 		}
 
@@ -381,9 +382,6 @@ func findUnlockerIDByMetadata(
 
 		err = json.Unmarshal(metadataBytes, &diskMetadata)
 		if err != nil {
-			secret.Warn("Could not parse unlocker metadata file",
-				"path", metadataPath, "error", err)
-
 			continue
 		}
 
@@ -534,6 +532,12 @@ func (cli *Instance) printUnlockersTable(unlockers []UnlockerInfo) error {
 
 // UnlockersAdd adds a new unlocker
 func (cli *Instance) UnlockersAdd(unlockerType string, cmd *cobra.Command) error {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	switch unlockerType {
 	case unlockerTypePassphrase:
 		return cli.addPassphraseUnlocker(cmd)
@@ -691,8 +695,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)
 	}
 
@@ -710,9 +721,23 @@ 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 {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	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)
@@ -763,6 +788,12 @@ func (cli *Instance) UnlockersRemove(
 
 // UnlockerSelect selects an unlocker as current
 func (cli *Instance) UnlockerSelect(unlockerID string) error {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -772,44 +803,38 @@ func (cli *Instance) UnlockerSelect(unlockerID string) error {
 	return vlt.SelectUnlocker(unlockerID)
 }
 
-// checkUnlockerExists checks if an unlocker with the given ID exists
+// checkUnlockerExists reports whether the vault already has an unlocker
-func (cli *Instance) checkUnlockerExists(vlt *vault.Vault, unlockerID string) error {
+// with the given ID. It returns an error, and no answer, when unlockers.d
-	// Get the list of unlockers and check if any match the ID
+// cannot be read; the caller must then not create the unlocker.
-	unlockers, err := vlt.ListUnlockers()
+func (cli *Instance) checkUnlockerExists(
-	if err != nil {
+	vlt *vault.Vault, unlockerID string,
-		secret.Warn("Could not list unlockers during duplicate check", "error", err)
+) (bool, error) {
-
-		return nil // If we can't list unlockers, assume it doesn't exist
-	}
-
-	// Get vault directory to construct unlocker instances
 	vaultDir, err := vlt.GetDirectory()
 	if err != nil {
-		secret.Warn("Could not get vault directory during duplicate check",
+		return false, fmt.Errorf("failed to get vault directory: %w", err)
-			"error", err)
-
-		return nil
 	}
 
-	// Check each unlocker's ID
 	unlockersDir := filepath.Join(vaultDir, "unlockers.d")
 
+	unlockers, err := vlt.ListUnlockers()
+	if err != nil {
+		return false, fmt.Errorf(
+			"failed to list unlockers in %s: %w", unlockersDir, err,
+		)
+	}
+
 	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(
+			// Unlike `unlocker list`, never skip here: a skipped entry may be the duplicate.
-				"Could not read unlockers directory during duplicate check, "+
+			return false, err
-					"skipping unlocker",
-				"unlockers_dir", unlockersDir, "error", err)
-
-			continue
 		}
 
 		if id != "" && id == unlockerID {
-			return errUnlockerExists
+			return true, nil
 		}
 	}
 
-	return nil
+	return false, nil
 }

internal/cli/unlockers_list_test.go

@@ -1,13 +1,19 @@
 // Unlocker List Tests
 //
-// Tests for `secret unlocker list` behavior when the unlockers.d directory
+// Tests for `secret unlocker list` behavior when the unlockers.d directory,
-// cannot be read while the listing is being rendered:
+// or an unlocker's metadata in it, cannot be read while the listing is
+// being rendered:
 //
 //   - TestUnlockersListSkipsUnreadableUnlockersDir: an unreadable
 //     unlockers.d yields no rows rather than rows bearing synthesized IDs.
 //   - TestUnlockersListSkipsOnlyUnreadableEntries: a readable entry is
 //     still listed, with its real ID and its current-unlocker marker,
 //     when a later entry's scan fails.
+//   - TestUnlockersListToleratesCorruptMetadata: one unlocker's corrupt
+//     metadata does not stop the others from being listed.
+//   - TestUnlockersListSkipsUnreadableMetadata: an unlocker whose metadata
+//     file cannot be checked for or read is left out, and the other is
+//     still listed.
 //
 // The listing resolves each unlocker's real ID by rescanning unlockers.d
 // after the vault has already enumerated it. If that rescan fails the ID
@@ -22,6 +28,7 @@ import (
 	"bytes"
 	"encoding/json"
 	"errors"
+	"os"
 	"path/filepath"
 	"testing"
 	"time"
@@ -92,6 +99,49 @@ func (f *unlockersDirFailFs) Open(name string) (afero.File, error) {
 	return f.Fs.Open(name)
 }
 
+// errMetadataUnreadable is returned by the test filesystem in place of a
+// successful open of one unlocker's metadata file.
+var errMetadataUnreadable = errors.New("input/output error")
+
+// metadataReadFailFs fails every open of the file at unreadablePath. The
+// file still exists, so checking for it succeeds and only reading it fails.
+type metadataReadFailFs struct {
+	afero.Fs
+
+	unreadablePath string
+}
+
+//nolint:ireturn // afero.File is the interface required by afero.Fs
+func (f *metadataReadFailFs) Open(name string) (afero.File, error) {
+	if name == f.unreadablePath {
+		return nil, errMetadataUnreadable
+	}
+
+	//nolint:wrapcheck // test double must return the wrapped Fs error as-is
+	return f.Fs.Open(name)
+}
+
+// errMetadataUncheckable is returned by the test filesystem in place of a
+// successful check for one unlocker's metadata file.
+var errMetadataUncheckable = errors.New("permission denied")
+
+// metadataStatFailFs fails every check for whether the file at
+// uncheckablePath exists, as when its unlocker directory cannot be entered.
+type metadataStatFailFs struct {
+	afero.Fs
+
+	uncheckablePath string
+}
+
+func (f *metadataStatFailFs) Stat(name string) (os.FileInfo, error) {
+	if name == f.uncheckablePath {
+		return nil, errMetadataUncheckable
+	}
+
+	//nolint:wrapcheck // test double must return the wrapped Fs error as-is
+	return f.Fs.Stat(name)
+}
+
 // writePGPUnlocker writes a PGP unlocker directory with metadata that
 // yields the real ID "pgp-<keyID>".
 func writePGPUnlocker(
@@ -227,3 +277,102 @@ func TestUnlockersListReadableEntriesAreListed(t *testing.T) {
 	assert.True(t, unlockers[0].IsCurrent)
 	assert.False(t, unlockers[1].IsCurrent)
 }
+
+// TestUnlockersListToleratesCorruptMetadata asserts that one unlocker with
+// corrupt metadata does not stop the listing. Metadata that is not JSON
+// leaves that unlocker out; PGP metadata without a usable GPG key ID lists
+// it as "pgp-unknown". The healthy unlocker is listed with its real ID.
+func TestUnlockersListToleratesCorruptMetadata(t *testing.T) {
+	t.Parallel()
+
+	healthyID := "pgp-" + listTestGPGKeyID + "A"
+
+	tests := []struct {
+		name     string
+		metadata string
+		wantIDs  []string
+	}{
+		{
+			name:     "not JSON",
+			metadata: "not json",
+			wantIDs:  []string{healthyID},
+		},
+		{
+			name:     "GPG key ID of the wrong type",
+			metadata: `{"type": "pgp", "gpgKeyId": 42}`,
+			wantIDs:  []string{healthyID, "pgp-unknown"},
+		},
+		{
+			name:     "GPG key ID missing",
+			metadata: `{"type": "pgp"}`,
+			wantIDs:  []string{healthyID, "pgp-unknown"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			fs := newListTestVault(t, 2)
+			metadataPath := filepath.Join(listTestStateDir, "vaults.d",
+				listTestVaultName, listTestUnlockersDirName,
+				listTestUnlockerDirTwo, listTestMetadataFileName)
+			require.NoError(t, afero.WriteFile(
+				fs, metadataPath, []byte(tt.metadata), listTestFilePerm,
+			))
+
+			unlockers := listUnlockersJSON(t, fs)
+			require.Len(t, unlockers, len(tt.wantIDs))
+
+			for i, wantID := range tt.wantIDs {
+				assert.Equal(t, wantID, unlockers[i].ID)
+			}
+		})
+	}
+}
+
+// TestUnlockersListSkipsUnreadableMetadata asserts that an unlocker whose
+// metadata file cannot be checked for or cannot be read is left out of the
+// listing, and the other unlocker is still listed with its real ID. The
+// failing one sorts first, so finding the other's ID has to step past it
+// as well.
+func TestUnlockersListSkipsUnreadableMetadata(t *testing.T) {
+	t.Parallel()
+
+	failingPath := filepath.Join(listTestStateDir, "vaults.d",
+		listTestVaultName, listTestUnlockersDirName,
+		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()
+
+			fs := tt.wrap(newListTestVault(t, 2))
+
+			unlockers := listUnlockersJSON(t, fs)
+
+			require.Len(t, unlockers, 1,
+				"only the unlocker with usable metadata may be listed")
+			assert.Equal(t, "pgp-"+listTestGPGKeyID+"B", unlockers[0].ID,
+				"the listed row must carry the real unlocker ID")
+		})
+	}
+}

internal/cli/unreadable_dir_test.go

@@ -0,0 +1,322 @@
+// 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
+// fails, and creates no unlocker directory, when unlockers.d cannot be
+// read for the duplicate check; and, as the control case, that a readable
+// unlockers.d holding the same key is still refused as a duplicate.
+//
+//nolint:paralleltest // t.Setenv (GNUPGHOME) forbids parallel tests
+func TestAddPGPUnlockerDuplicateCheck(t *testing.T) {
+	fingerprint := newTestGPGKey(t)
+	unlockersDir := filepath.Join(
+		testVaultDir(listTestVaultName), listTestUnlockersDirName)
+
+	tests := []struct {
+		name       string
+		openBudget int
+	}{
+		// The vault's own enumeration of unlockers.d fails.
+		{name: "listing fails", openBudget: 0},
+		// The enumeration succeeds; the rescan that resolves IDs fails.
+		{name: "rescan fails", openBudget: 1},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			base := newListTestVault(t, 1)
+			fs := &unlockersDirFailFs{Fs: base, openBudget: tt.openBudget}
+
+			err := addTestPGPUnlocker(fs)
+
+			require.ErrorIs(t, err, errUnlockersDirUnreadable)
+			require.NotErrorIs(t, err, errGPGKeyAlreadyUnlocker)
+			assert.Contains(t, err.Error(), unlockersDir,
+				"the error must name the directory it could not read")
+			assertDirEntries(t, base, unlockersDir, listTestUnlockerDirOne)
+		})
+	}
+
+	t.Run("duplicate refused", func(t *testing.T) {
+		base := newListTestVault(t, 1)
+		writePGPUnlocker(t, base, unlockersDir, listTestUnlockerDirTwo,
+			time.Date(2026, time.August, 10, 12, 30, 0, 0, time.UTC),
+			fingerprint)
+
+		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

@@ -267,6 +267,12 @@ func resolvePassphrase() (*memguard.LockedBuffer, error) {
 func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
 	secret.Debug("Creating new vault", "name", name, "state_dir", cli.stateDir)
 
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Get or prompt for mnemonic
 	var mnemonicStr string
 
@@ -354,7 +360,13 @@ func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
 
 // SelectVault selects a vault as the current one
 func (cli *Instance) SelectVault(cmd *cobra.Command, name string) error {
-	err := vault.SelectVault(cli.fs, cli.stateDir, name)
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	err = vault.SelectVault(cli.fs, cli.stateDir, name)
 	if err != nil {
 		return err
 	}
@@ -388,8 +400,12 @@ func (cli *Instance) vaultImportPreflight(
 	// Check if vault already has a public key
 	pubKeyPath := vaultDir + "/pub.age"
 
-	_, err = cli.fs.Stat(pubKeyPath)
+	exists, err = afero.Exists(cli.fs, pubKeyPath)
-	if err == nil {
+	if err != nil {
+		return "", "", "", fmt.Errorf("failed to check %s: %w", pubKeyPath, err)
+	}
+
+	if exists {
 		return "", "", "", fmt.Errorf("vault '%s' %w",
 			vaultName, errVaultHasLongTermKey)
 	}
@@ -442,8 +458,21 @@ func updateVaultImportMetadata(
 	return nil
 }
 
-// VaultImport imports a mnemonic into a specific vault
+// VaultImport imports a mnemonic into a specific vault, holding the state
+// directory lock while importMnemonic runs
 func (cli *Instance) VaultImport(cmd *cobra.Command, vaultName string) error {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	return cli.importMnemonic(cmd, vaultName)
+}
+
+// importMnemonic gives the vault a long-term key derived from the mnemonic
+// and a passphrase unlocker
+func (cli *Instance) importMnemonic(cmd *cobra.Command, vaultName string) error {
 	secret.Debug("Importing mnemonic into vault",
 		"vault_name", vaultName, "state_dir", cli.stateDir)
 
@@ -478,7 +507,7 @@ func (cli *Instance) VaultImport(cmd *cobra.Command, vaultName string) error {
 	secret.Debug("Storing long-term public key",
 		"pubkey", ltPublicKey, "vault_dir", vaultDir)
 
-	err = afero.WriteFile(cli.fs, pubKeyPath, []byte(ltPublicKey), secret.FilePerms)
+	err = secret.WriteFileAtomic(cli.fs, pubKeyPath, []byte(ltPublicKey))
 	if err != nil {
 		return fmt.Errorf("failed to store long-term public key: %w", err)
 	}
@@ -536,17 +565,26 @@ func (cli *Instance) VaultImport(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
@@ -575,8 +613,20 @@ 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 {
+		return err
+	}
+	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 {
@@ -610,7 +660,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 {
@@ -626,7 +679,7 @@ func (cli *Instance) RemoveVault(cmd *cobra.Command, name string, force bool) er
 	}
 
 	// Remove the vault directory
-	err = cli.fs.RemoveAll(vaultDir)
+	err = secret.RemoveDirAtomic(cli.fs, vaultDir)
 	if err != nil {
 		return fmt.Errorf("failed to remove vault directory: %w", err)
 	}

internal/cli/version.go

@@ -112,6 +112,11 @@ func VersionCommands(cli *Instance) *cobra.Command {
 func (cli *Instance) ListVersions(cmd *cobra.Command, secretName string) error {
 	secret.Debug("ListVersions called", "secret_name", secretName)
 
+	err := vault.ValidateSecretName(secretName)
+	if err != nil {
+		return err
+	}
+
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -239,6 +244,17 @@ func formatVersionTime(t *time.Time) string {
 func (cli *Instance) PromoteVersion(
 	cmd *cobra.Command, secretName string, version string,
 ) error {
+	err := vault.ValidateSecretName(secretName)
+	if err != nil {
+		return err
+	}
+
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -255,9 +271,7 @@ func (cli *Instance) PromoteVersion(
 	secretDir := filepath.Join(vaultDir, "secrets.d", encodedName)
 
 	// Check if version exists
-	versionDir := filepath.Join(secretDir, "versions", version)
+	exists, err := secret.VersionExists(cli.fs, secretDir, version)
-
-	exists, err := afero.DirExists(cli.fs, versionDir)
 	if err != nil {
 		return fmt.Errorf("failed to check if version exists: %w", err)
 	}
@@ -282,6 +296,17 @@ func (cli *Instance) PromoteVersion(
 func (cli *Instance) RemoveVersion(
 	cmd *cobra.Command, secretName string, version string,
 ) error {
+	err := vault.ValidateSecretName(secretName)
+	if err != nil {
+		return err
+	}
+
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -308,9 +333,7 @@ func (cli *Instance) RemoveVersion(
 	}
 
 	// Check if version exists
-	versionDir := filepath.Join(secretDir, "versions", version)
+	exists, err = secret.VersionExists(cli.fs, secretDir, version)
-
-	exists, err = afero.DirExists(cli.fs, versionDir)
 	if err != nil {
 		return fmt.Errorf("failed to check if version exists: %w", err)
 	}
@@ -333,7 +356,9 @@ func (cli *Instance) RemoveVersion(
 	}
 
 	// Remove the version directory
-	err = cli.fs.RemoveAll(versionDir)
+	versionDir := filepath.Join(secretDir, "versions", version)
+
+	err = secret.RemoveDirAtomic(cli.fs, versionDir)
 	if err != nil {
 		return fmt.Errorf("failed to remove version: %w", err)
 	}

internal/cli/version_test.go

@@ -276,8 +276,8 @@ func TestGetSecretWithVersion(t *testing.T) {
 	var buf bytes.Buffer
 	cmd.SetOut(&buf)
 
-	// Test getting current version (empty version string)
+	// Test getting the current version
-	err = cli.GetSecretWithVersion(cmd, "test/secret", "")
+	err = cli.GetSecret(cmd, "test/secret")
 	require.NoError(t, err)
 	assert.Equal(t, "version-2", buf.String())
 

internal/secret/atomic.go

@@ -0,0 +1,86 @@
+package secret
+
+import (
+	"fmt"
+	"path/filepath"
+
+	"github.com/spf13/afero"
+)
+
+// WriteFileAtomic replaces the file at path with data so that a reader, or
+// a crash at any moment, finds either the old content or the new, never a
+// partial file. The data goes into a temporary file that afero.TempFile
+// creates with mode 0600 in the same directory (a rename is only atomic
+// within one filesystem), is synced to disk, and is renamed over path. The
+// temporary file is removed if any step fails.
+func WriteFileAtomic(fs afero.Fs, path string, data []byte) error {
+	tmp, err := afero.TempFile(fs, filepath.Dir(path),
+		"."+filepath.Base(path)+".tmp-*")
+	if err != nil {
+		return fmt.Errorf("failed to create temporary file for %s: %w", path, err)
+	}
+
+	_, err = tmp.Write(data)
+	if err == nil {
+		err = tmp.Sync()
+	}
+
+	closeErr := tmp.Close()
+	if err == nil {
+		err = closeErr
+	}
+
+	if err == nil {
+		err = fs.Rename(tmp.Name(), path)
+	}
+
+	if err != nil {
+		_ = fs.Remove(tmp.Name())
+
+		return fmt.Errorf("failed to write %s: %w", path, err)
+	}
+
+	return nil
+}
+
+// TempDirFor creates an empty temporary directory in which to build the
+// directory target before renaming it into place, or into which to move
+// target before deleting it. It is made in target's grandparent: on the
+// same filesystem, so the rename is atomic, and outside target's parent,
+// the directory that is listed to find vaults, secrets, versions and
+// unlockers, so one left behind by a crash is never taken for one of them.
+// Its name leaves out target's, which may already be as long as a file name
+// can be.
+func TempDirFor(fs afero.Fs, target string) (string, error) {
+	dir, err := afero.TempDir(fs, filepath.Dir(filepath.Dir(target)), ".tmp-")
+	if err != nil {
+		return "", fmt.Errorf(
+			"failed to create temporary directory for %s: %w", target, err)
+	}
+
+	return dir, 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.
+func RemoveDirAtomic(fs afero.Fs, dir string) error {
+	tmp, err := TempDirFor(fs, dir)
+	if err != nil {
+		return err
+	}
+
+	err = fs.Rename(dir, filepath.Join(tmp, filepath.Base(dir)))
+	if err != nil {
+		_ = fs.Remove(tmp)
+
+		return fmt.Errorf("failed to remove %s: %w", dir, err)
+	}
+
+	err = fs.RemoveAll(tmp)
+	if err != nil {
+		return fmt.Errorf("failed to remove %s: %w", dir, err)
+	}
+
+	return nil
+}

internal/secret/atomic_test.go

@@ -0,0 +1,672 @@
+package secret_test
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"filippo.io/age"
+	"git.eeqj.de/sneak/secret/internal/secret"
+	"git.eeqj.de/sneak/secret/internal/vault"
+	"github.com/awnumar/memguard"
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+var errInjected = errors.New("injected failure")
+
+// The kinds of change hookFs passes to before.
+const (
+	opCreate = "create"
+	opOpen   = "open"
+	opSync   = "sync"
+	opMkdir  = "mkdir"
+	opRemove = "remove"
+	opRename = "rename"
+)
+
+// currentFile is the file in a secret's directory that names its current
+// version.
+const currentFile = "current"
+
+// unlockerMetadataFile is the file a new unlocker writes last.
+const unlockerMetadataFile = "unlocker-metadata.json"
+
+// unlockerPassphrase protects the passphrase unlockers the tests create.
+//
+//nolint:gosec // G101: test data, not a real credential
+const unlockerPassphrase = "unlocker passphrase"
+
+// hookFs passes every call through to Fs, but first calls before for each
+// call that changes the filesystem, and for each Sync of a file opened
+// through it, with the path it changes (the new path, for Rename). A test
+// uses before to inspect the tree at every point where a crash could stop
+// the code under test, or returns an error from it to make that call fail.
+// If opened is set, OpenFile also tells it the mode it opens each file with.
+type hookFs struct {
+	afero.Fs
+
+	before func(op, path string) error
+	opened func(path string, perm os.FileMode)
+}
+
+// hookFile is a file opened through hookFs.
+type hookFile struct {
+	afero.File
+
+	before func(op, path string) error
+}
+
+func (f hookFile) Sync() error {
+	err := f.before(opSync, f.Name())
+	if err != nil {
+		return err
+	}
+
+	return f.File.Sync()
+}
+
+//nolint:ireturn // implements afero.Fs
+func (h hookFs) Create(name string) (afero.File, error) {
+	err := h.before(opCreate, name)
+	if err != nil {
+		return nil, err
+	}
+
+	file, err := h.Fs.Create(name)
+	if err != nil {
+		return nil, err
+	}
+
+	return hookFile{File: file, before: h.before}, nil
+}
+
+//nolint:ireturn // implements afero.Fs
+func (h hookFs) OpenFile(
+	name string, flag int, perm os.FileMode,
+) (afero.File, error) {
+	err := h.before(opOpen, name)
+	if err != nil {
+		return nil, err
+	}
+
+	if h.opened != nil {
+		h.opened(name, perm)
+	}
+
+	file, err := h.Fs.OpenFile(name, flag, perm)
+	if err != nil {
+		return nil, err
+	}
+
+	return hookFile{File: file, before: h.before}, nil
+}
+
+func (h hookFs) Mkdir(name string, perm os.FileMode) error {
+	err := h.before(opMkdir, name)
+	if err != nil {
+		return err
+	}
+
+	return h.Fs.Mkdir(name, perm)
+}
+
+func (h hookFs) MkdirAll(path string, perm os.FileMode) error {
+	err := h.before(opMkdir, path)
+	if err != nil {
+		return err
+	}
+
+	return h.Fs.MkdirAll(path, perm)
+}
+
+func (h hookFs) Remove(name string) error {
+	err := h.before(opRemove, name)
+	if err != nil {
+		return err
+	}
+
+	return h.Fs.Remove(name)
+}
+
+func (h hookFs) RemoveAll(path string) error {
+	err := h.before(opRemove, path)
+	if err != nil {
+		return err
+	}
+
+	return h.Fs.RemoveAll(path)
+}
+
+func (h hookFs) Rename(oldname, newname string) error {
+	err := h.before(opRename, newname)
+	if err != nil {
+		return err
+	}
+
+	return h.Fs.Rename(oldname, newname)
+}
+
+// testFilesystem is a filesystem to run a test on, with a directory in it
+// to work in.
+type testFilesystem struct {
+	name string
+	open func(t *testing.T) (afero.Fs, string)
+}
+
+// testFilesystems are the in-memory filesystem that most tests use and the
+// real one: every rename-based guarantee is checked on both.
+//
+//nolint:gochecknoglobals // read-only table shared by the tests below
+var testFilesystems = []testFilesystem{
+	{"memory", func(*testing.T) (afero.Fs, string) {
+		return afero.NewMemMapFs(), "/test"
+	}},
+	{"real", func(t *testing.T) (afero.Fs, string) {
+		t.Helper()
+
+		return afero.NewOsFs(), t.TempDir()
+	}},
+}
+
+// dirNames lists the names in dir.
+func dirNames(t *testing.T, fs afero.Fs, dir string) []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())
+	}
+
+	return names
+}
+
+// writeLongTermKey gives the test vault under stateDir a new long-term key
+// and returns it.
+func writeLongTermKey(
+	t *testing.T, fs afero.Fs, stateDir string,
+) *age.X25519Identity {
+	t.Helper()
+
+	vault := &MockVersionVault{Name: testVaultName, fs: fs, stateDir: stateDir}
+
+	vaultDir, err := vault.GetDirectory()
+	require.NoError(t, err)
+	require.NoError(t, fs.MkdirAll(vaultDir, 0o700))
+
+	ltIdentity, err := age.GenerateX25519Identity()
+	require.NoError(t, err)
+	require.NoError(t, afero.WriteFile(fs, filepath.Join(vaultDir, "pub.age"),
+		[]byte(ltIdentity.Recipient().String()), 0o600))
+
+	return ltIdentity
+}
+
+// newVaultWithSecret creates the vault name under stateDir from the test
+// mnemonic, with a secret "shared" in it that holds value.
+func newVaultWithSecret(
+	t *testing.T, fs afero.Fs, stateDir, name, value string,
+) *vault.Vault {
+	t.Helper()
+
+	vlt, err := vault.CreateVault(fs, stateDir, name)
+	require.NoError(t, err)
+
+	buffer := memguard.NewBufferFromBytes([]byte(value))
+	defer buffer.Destroy()
+
+	require.NoError(t, vlt.AddSecret("shared", buffer, false))
+
+	return vlt
+}
+
+func TestWriteFileAtomicReplacesFile(t *testing.T) {
+	t.Parallel()
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			fs, dir := tfs.open(t)
+			path := filepath.Join(dir, currentFile)
+
+			require.NoError(t, secret.WriteFileAtomic(fs, path, []byte("old")))
+			require.NoError(t, secret.WriteFileAtomic(fs, path, []byte("new")))
+
+			data, err := afero.ReadFile(fs, path)
+			require.NoError(t, err)
+			assert.Equal(t, "new", string(data))
+
+			info, err := fs.Stat(path)
+			require.NoError(t, err)
+			assert.Equal(t, secret.FilePerms, info.Mode().Perm())
+
+			// No temporary file is left next to it
+			assert.Equal(t, []string{currentFile}, dirNames(t, fs, dir))
+		})
+	}
+}
+
+func TestWriteFileAtomicFailureKeepsOldFile(t *testing.T) {
+	t.Parallel()
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			base, dir := tfs.open(t)
+			path := filepath.Join(dir, currentFile)
+			require.NoError(t, secret.WriteFileAtomic(base, path, []byte("old")))
+
+			fs := hookFs{Fs: base, before: func(op, _ string) error {
+				if op == opRename {
+					return errInjected
+				}
+
+				return nil
+			}}
+
+			err := secret.WriteFileAtomic(fs, path, []byte("new"))
+			require.ErrorIs(t, err, errInjected)
+
+			data, err := afero.ReadFile(base, path)
+			require.NoError(t, err)
+			assert.Equal(t, "old", string(data))
+
+			// The temporary file is removed again
+			assert.Equal(t, []string{currentFile}, dirNames(t, base, dir))
+		})
+	}
+}
+
+// TestRemoveDirAtomic checks that RemoveDirAtomic deletes nothing where the
+// directory stands, which a crash could stop half-way, and that it leaves
+// nothing behind.
+func TestRemoveDirAtomic(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, "secrets.d")
+			target := filepath.Join(listed, "doomed")
+
+			require.NoError(t, base.MkdirAll(filepath.Join(target, "versions"), 0o700))
+			require.NoError(t, secret.WriteFileAtomic(base,
+				filepath.Join(target, currentFile), []byte("20231216.001")))
+
+			fs := hookFs{Fs: base, before: func(op, path string) error {
+				if op == opRemove && strings.HasPrefix(path, target) {
+					t.Errorf("deleted %s where it stands", path)
+				}
+
+				return nil
+			}}
+
+			require.NoError(t, secret.RemoveDirAtomic(fs, target))
+
+			// Gone, and no temporary directory is left in the directory
+			// that is listed or in the one above it
+			assert.Empty(t, dirNames(t, base, listed))
+			assert.Equal(t, []string{"secrets.d"}, dirNames(t, base, dir))
+		})
+	}
+}
+
+// TestLongestNames adds a secret to a vault and removes the vault, both
+// named with 255 bytes, the most a file name may have, on the real
+// filesystem: the temporary directories they use must fit that limit too.
+func TestLongestNames(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	const longestName = 255
+
+	fs := afero.NewOsFs()
+	name := strings.Repeat("a", longestName)
+
+	vlt, err := vault.CreateVault(fs, t.TempDir(), name)
+	require.NoError(t, err)
+
+	value := memguard.NewBufferFromBytes([]byte("long"))
+	defer value.Destroy()
+
+	require.NoError(t, vlt.AddSecret(name, value, false))
+
+	got, err := vlt.GetSecret(name)
+	require.NoError(t, err)
+	assert.Equal(t, "long", string(got))
+
+	vaultDir, err := vlt.GetDirectory()
+	require.NoError(t, err)
+	require.NoError(t, secret.RemoveDirAtomic(fs, vaultDir))
+	assert.NoDirExists(t, vaultDir)
+}
+
+// TestForcedCopyKeepsDestinationUntilReplaced copies a secret over one in
+// another vault, as a forced move between vaults does, and makes the last
+// step that completes the copy fail. The secret it was to replace must
+// still be there unchanged: it may go only once its replacement is whole.
+//
+//nolint:paralleltest // t.Setenv forbids t.Parallel
+func TestForcedCopyKeepsDestinationUntilReplaced(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			base, stateDir := tfs.open(t)
+			src := newVaultWithSecret(t, base, stateDir, "source", "new")
+			dest := newVaultWithSecret(t, base, stateDir, "dest", "old")
+
+			// The copy is complete once its current file is written
+			fs := hookFs{Fs: base, before: func(op, path string) error {
+				if op == opRename && filepath.Base(path) == currentFile {
+					return errInjected
+				}
+
+				return nil
+			}}
+
+			err := vault.NewVault(fs, stateDir, "dest").
+				CopySecretAllVersions(src, "shared", "shared", true)
+			require.ErrorIs(t, err, errInjected)
+
+			value, err := dest.GetSecret("shared")
+			require.NoError(t, err)
+			assert.Equal(t, "old", string(value))
+		})
+	}
+}
+
+// TestTempDirsStayOutOfListings adds a version, adds a secret, copies a
+// secret over another and removes one, and checks that none of them makes a
+// directory directly in secrets.d or in a versions directory. Those are
+// listed to find secrets and versions, so a temporary directory made there
+// would be listed while half-built, and one left by a crash would stay.
+//
+//nolint:paralleltest // t.Setenv forbids t.Parallel
+func TestTempDirsStayOutOfListings(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			base, stateDir := tfs.open(t)
+			newVaultWithSecret(t, base, stateDir, "default", "first")
+
+			fs := hookFs{Fs: base, before: func(op, path string) error {
+				parent := filepath.Base(filepath.Dir(path))
+				if op == opMkdir && (parent == "secrets.d" || parent == "versions") {
+					t.Errorf("made %s where it is listed", path)
+				}
+
+				return nil
+			}}
+			vlt := vault.NewVault(fs, stateDir, "default")
+
+			value := memguard.NewBufferFromBytes([]byte("second"))
+			defer value.Destroy()
+
+			require.NoError(t, vlt.AddSecret("shared", value, true))
+			require.NoError(t, vlt.AddSecret("other", value, false))
+			require.NoError(t, vlt.CopySecretAllVersions(vlt, "shared", "other", true))
+
+			vaultDir, err := vlt.GetDirectory()
+			require.NoError(t, err)
+			require.NoError(t, secret.RemoveDirAtomic(fs,
+				filepath.Join(vaultDir, "secrets.d", "shared")))
+		})
+	}
+}
+
+// TestVersionSaveIsWholeOrAbsent checks, before every change Save makes and
+// once after it returns, that the version directory either does not exist
+// or holds all of its files: a crash at any point leaves no version that
+// cannot be decrypted.
+func TestVersionSaveIsWholeOrAbsent(t *testing.T) {
+	t.Parallel()
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			base, stateDir := tfs.open(t)
+			ltIdentity := writeLongTermKey(t, base, stateDir)
+
+			var versionDir string
+
+			checkVersionDir := func(string, string) error {
+				exists, err := afero.DirExists(base, versionDir)
+				require.NoError(t, err)
+
+				if exists {
+					assert.ElementsMatch(t,
+						[]string{"pub.age", "value.age", "priv.age", "metadata.age"},
+						dirNames(t, base, versionDir),
+						"version directory visible before it was complete")
+				}
+
+				return nil
+			}
+
+			fs := hookFs{Fs: base, before: checkVersionDir}
+			vault := &MockVersionVault{Name: testVaultName, fs: fs, stateDir: stateDir}
+			sv := secret.NewVersion(vault, "test/secret", "20231215.001")
+			versionDir = sv.Directory
+
+			value := memguard.NewBufferFromBytes([]byte("whole or nothing"))
+			defer value.Destroy()
+
+			require.NoError(t, sv.Save(value))
+			require.NoError(t, checkVersionDir("", ""))
+
+			got, err := sv.GetValue(ltIdentity)
+			require.NoError(t, err)
+
+			defer got.Destroy()
+
+			assert.Equal(t, "whole or nothing", got.String())
+		})
+	}
+}
+
+// TestVersionSaveFailureLeavesNothing makes the write of the encrypted
+// private key fail, after the value has been written, and checks that
+// neither the version nor its temporary directory is left behind.
+func TestVersionSaveFailureLeavesNothing(t *testing.T) {
+	t.Parallel()
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			base, stateDir := tfs.open(t)
+			writeLongTermKey(t, base, stateDir)
+
+			fs := hookFs{Fs: base, before: func(op, path string) error {
+				if op == opRename && filepath.Base(path) == "priv.age" {
+					return errInjected
+				}
+
+				return nil
+			}}
+			vault := &MockVersionVault{Name: testVaultName, fs: fs, stateDir: stateDir}
+			sv := secret.NewVersion(vault, "test/secret", "20231215.001")
+
+			value := memguard.NewBufferFromBytes([]byte("never stored"))
+			defer value.Destroy()
+
+			require.ErrorIs(t, sv.Save(value), errInjected)
+
+			// The secret directory holds only the empty versions directory
+			versionsDir := filepath.Dir(sv.Directory)
+			assert.Equal(t, []string{"versions"},
+				dirNames(t, base, filepath.Dir(versionsDir)))
+			assert.Empty(t, dirNames(t, base, versionsDir))
+		})
+	}
+}
+
+// TestCurrentFilesNeverMissing selects the current version, vault and
+// unlocker again and checks, before each change this makes, that the file
+// naming the current one exists: a reader or a crash never finds it
+// missing.
+//
+//nolint:paralleltest // t.Setenv forbids t.Parallel
+func TestCurrentFilesNeverMissing(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			base, stateDir := tfs.open(t)
+			vlt := newVaultWithSecret(t, base, stateDir, testVaultName, "value")
+
+			passphrase := memguard.NewBufferFromBytes([]byte(unlockerPassphrase))
+			defer passphrase.Destroy()
+
+			// Created as the current unlocker
+			unlocker, err := vlt.CreatePassphraseUnlocker(passphrase)
+			require.NoError(t, err)
+
+			vaultDir, err := vlt.GetDirectory()
+			require.NoError(t, err)
+
+			secretDir := filepath.Join(vaultDir, "secrets.d", "shared")
+			version, err := secret.GetCurrentVersion(base, secretDir)
+			require.NoError(t, err)
+
+			for _, tc := range []struct {
+				path     string
+				reselect func(fs afero.Fs) error
+			}{
+				{filepath.Join(secretDir, currentFile), func(fs afero.Fs) error {
+					return secret.SetCurrentVersion(fs, secretDir, version)
+				}},
+				{filepath.Join(stateDir, "currentvault"), func(fs afero.Fs) error {
+					return vault.SelectVault(fs, stateDir, testVaultName)
+				}},
+				{filepath.Join(vaultDir, "current-unlocker"), func(fs afero.Fs) error {
+					return vault.NewVault(fs, stateDir, testVaultName).
+						SelectUnlocker(unlocker.GetID())
+				}},
+			} {
+				fs := hookFs{Fs: base, before: func(string, string) error {
+					exists, err := afero.Exists(base, tc.path)
+					require.NoError(t, err)
+					assert.True(t, exists, "%s is missing", filepath.Base(tc.path))
+
+					return nil
+				}}
+
+				require.NoError(t, tc.reselect(fs))
+			}
+		})
+	}
+}
+
+// TestWriteFileAtomicTempFile checks that WriteFileAtomic creates its
+// temporary file with mode 0600, rather than wider and narrowed later, so
+// that no other user can ever read it, and syncs it before renaming it into
+// place, so that a crash cannot leave the file named but its data lost.
+func TestWriteFileAtomicTempFile(t *testing.T) {
+	t.Parallel()
+
+	for _, tfs := range testFilesystems {
+		t.Run(tfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			base, dir := tfs.open(t)
+
+			var modes []os.FileMode
+
+			synced := false
+			fs := hookFs{
+				Fs: base,
+				before: func(op, _ string) error {
+					switch op {
+					case opSync:
+						synced = true
+					case opRename:
+						assert.True(t, synced, "renamed before syncing")
+					}
+
+					return nil
+				},
+				opened: func(_ string, perm os.FileMode) {
+					modes = append(modes, perm)
+				},
+			}
+
+			require.NoError(t, secret.WriteFileAtomic(fs,
+				filepath.Join(dir, currentFile), []byte("new")))
+			assert.Equal(t, []os.FileMode{secret.FilePerms}, modes)
+		})
+	}
+}
+
+// TestPassphraseUnlockerGetsKeyFirst creates a passphrase unlocker in a
+// vault whose long-term key cannot be had: it must fail without writing
+// anything, so that it never leaves a partial unlocker, nor breaks the one
+// it would replace.
+func TestPassphraseUnlockerGetsKeyFirst(t *testing.T) {
+	// No mnemonic, and no current unlocker to get the key from
+	t.Setenv(secret.EnvMnemonic, "")
+
+	base := afero.NewMemMapFs()
+	_, err := vault.CreateVault(base, testVaultStateDir, testVaultName)
+	require.NoError(t, err)
+
+	fs := hookFs{Fs: base, before: func(_, path string) error {
+		t.Errorf("changed %s before getting the long-term key", path)
+
+		return nil
+	}}
+
+	passphrase := memguard.NewBufferFromBytes([]byte(unlockerPassphrase))
+	defer passphrase.Destroy()
+
+	_, err = vault.NewVault(fs, testVaultStateDir, testVaultName).
+		CreatePassphraseUnlocker(passphrase)
+	require.Error(t, err)
+}
+
+// TestPassphraseUnlockerWritesMetadataLast checks that the last file a new
+// passphrase unlocker writes in its directory is its metadata: an unlocker
+// directory without metadata is never used, so one interrupted earlier
+// cannot be.
+func TestPassphraseUnlockerWritesMetadataLast(t *testing.T) {
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+
+	base := afero.NewMemMapFs()
+	vlt, err := vault.CreateVault(base, testVaultStateDir, testVaultName)
+	require.NoError(t, err)
+
+	vaultDir, err := vlt.GetDirectory()
+	require.NoError(t, err)
+
+	unlockerDir := filepath.Join(vaultDir, "unlockers.d", "passphrase")
+
+	var last string
+
+	fs := hookFs{Fs: base, before: func(_, path string) error {
+		if filepath.Dir(path) == unlockerDir {
+			last = filepath.Base(path)
+		}
+
+		return nil
+	}}
+
+	passphrase := memguard.NewBufferFromBytes([]byte(unlockerPassphrase))
+	defer passphrase.Destroy()
+
+	_, err = vault.NewVault(fs, testVaultStateDir, testVaultName).
+		CreatePassphraseUnlocker(passphrase)
+	require.NoError(t, err)
+	assert.Equal(t, unlockerMetadataFile, last)
+}

internal/secret/helpers_darwin.go

@@ -1,29 +0,0 @@
-//go:build darwin
-
-package secret
-
-import (
-	"crypto/rand"
-	"fmt"
-	"math/big"
-)
-
-// generateRandomString generates a random string of the specified length using the given character set
-func generateRandomString(length int, charset string) (string, error) {
-	if length <= 0 {
-		return "", fmt.Errorf("length must be positive")
-	}
-
-	result := make([]byte, length)
-	charsetLen := big.NewInt(int64(len(charset)))
-
-	for i := range length {
-		randomIndex, err := rand.Int(rand.Reader, charsetLen)
-		if err != nil {
-			return "", fmt.Errorf("failed to generate random number: %w", err)
-		}
-		result[i] = charset[randomIndex.Int64()]
-	}
-
-	return string(result), nil
-}

internal/secret/keychaindata.go

@@ -0,0 +1,142 @@
+package secret
+
+import (
+	"bytes"
+	"encoding/hex"
+	"encoding/json"
+	"errors"
+	"fmt"
+	"strings"
+
+	"github.com/awnumar/memguard"
+)
+
+var (
+	errPassphraseLength = errors.New(
+		"passphrase length must be a positive even number")
+	errPassphraseNotHex = errors.New(
+		"keychain passphrase must be lowercase hex")
+	errNoKeychainPassphrase = errors.New(
+		"keychain data has no agePrivKeyPassphrase string")
+)
+
+// KeychainData is what a keychain unlocker stores in the macOS keychain.
+// It is stored as JSON, but encode and decodeKeychainData keep the
+// passphrase out of encoding/json, which would leave copies of it in
+// ordinary memory.
+type KeychainData struct {
+	AgePublicKey         string
+	AgePrivKeyPassphrase *memguard.LockedBuffer
+	EncryptedLongtermKey string
+}
+
+// generateRandomPassphrase returns length random lowercase hex characters
+// in a locked buffer. The caller must destroy it.
+func generateRandomPassphrase(length int) (*memguard.LockedBuffer, error) {
+	// Each random byte becomes two hex characters.
+	randomBytes := hex.DecodedLen(length)
+	if length <= 0 || hex.EncodedLen(randomBytes) != length {
+		return nil, errPassphraseLength
+	}
+
+	random := memguard.NewBufferRandom(randomBytes)
+	defer random.Destroy()
+
+	passphrase := memguard.NewBuffer(length)
+	hex.Encode(passphrase.Bytes(), random.Bytes())
+	passphrase.Freeze()
+
+	return passphrase, nil
+}
+
+// encode returns d as JSON in a locked buffer:
+// {"agePublicKey":"...","agePrivKeyPassphrase":"...","encryptedLongtermKey":"..."}.
+// The passphrase is copied straight into the buffer, so it must be hex,
+// which JSON does not escape. The caller must destroy the returned buffer.
+func (d *KeychainData) encode() (*memguard.LockedBuffer, error) {
+	if d.AgePrivKeyPassphrase == nil {
+		return nil, errNilPassphraseBuffer
+	}
+
+	if d.AgePrivKeyPassphrase.Size() == 0 {
+		return nil, errEmptyPassphrase
+	}
+
+	for _, c := range d.AgePrivKeyPassphrase.Bytes() {
+		if strings.IndexByte("0123456789abcdef", c) < 0 {
+			return nil, errPassphraseNotHex
+		}
+	}
+
+	publicKey, err := json.Marshal(d.AgePublicKey)
+	if err != nil {
+		return nil, fmt.Errorf("failed to encode age public key: %w", err)
+	}
+
+	longtermKey, err := json.Marshal(d.EncryptedLongtermKey)
+	if err != nil {
+		return nil, fmt.Errorf("failed to encode long-term key: %w", err)
+	}
+
+	parts := [][]byte{
+		[]byte(`{"agePublicKey":`), publicKey,
+		[]byte(`,"agePrivKeyPassphrase":"`), d.AgePrivKeyPassphrase.Bytes(),
+		[]byte(`","encryptedLongtermKey":`), longtermKey,
+		[]byte(`}`),
+	}
+
+	size := 0
+	for _, part := range parts {
+		size += len(part)
+	}
+
+	encoded := memguard.NewBuffer(size)
+
+	written := 0
+	for _, part := range parts {
+		written += copy(encoded.Bytes()[written:], part)
+	}
+
+	encoded.Freeze()
+
+	return encoded, nil
+}
+
+// decodeKeychainData parses keychain data written by encode. The caller
+// must destroy the returned AgePrivKeyPassphrase.
+func decodeKeychainData(data *memguard.LockedBuffer) (*KeychainData, error) {
+	if data == nil {
+		return nil, errNilDataBuffer
+	}
+
+	// json.Unmarshal gives a json.RawMessage field the field's JSON text
+	// unchanged, in the one copy RawMessage makes; it is wiped on return.
+	var fields struct {
+		AgePublicKey         string          `json:"agePublicKey"`
+		AgePrivKeyPassphrase json.RawMessage `json:"agePrivKeyPassphrase"`
+		EncryptedLongtermKey string          `json:"encryptedLongtermKey"`
+	}
+
+	defer func() { memguard.WipeBytes(fields.AgePrivKeyPassphrase) }()
+
+	err := json.Unmarshal(data.Bytes(), &fields)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse keychain data: %w", err)
+	}
+
+	// json.Unmarshal accepted the JSON, so text that starts with a quote is
+	// a whole string. The passphrase is hex, so it is the text between the
+	// quotes.
+	quoted := fields.AgePrivKeyPassphrase
+	if !bytes.HasPrefix(quoted, []byte(`"`)) {
+		return nil, errNoKeychainPassphrase
+	}
+
+	return &KeychainData{
+		AgePublicKey: fields.AgePublicKey,
+		// NewBufferFromBytes wipes the bytes it copies.
+		AgePrivKeyPassphrase: memguard.NewBufferFromBytes(
+			quoted[1 : len(quoted)-1]),
+		EncryptedLongtermKey: fields.EncryptedLongtermKey,
+	}, nil
+}

internal/secret/keychaindata_test.go

@@ -0,0 +1,118 @@
+//nolint:testpackage // white-box test of unexported internals
+package secret
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/awnumar/memguard"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestGenerateRandomPassphrase(t *testing.T) {
+	t.Parallel()
+
+	first, err := generateRandomPassphrase(64)
+	require.NoError(t, err)
+
+	defer first.Destroy()
+
+	second, err := generateRandomPassphrase(64)
+	require.NoError(t, err)
+
+	defer second.Destroy()
+
+	assert.Regexp(t, `^[0-9a-f]{64}$`, first.String())
+	assert.NotEqual(t, first.String(), second.String())
+	assert.False(t, first.IsMutable())
+
+	for _, length := range []int{0, -2, 63} {
+		_, err := generateRandomPassphrase(length)
+		require.ErrorIs(t, err, errPassphraseLength, "length %d", length)
+	}
+}
+
+func TestKeychainDataEncodeDecode(t *testing.T) {
+	t.Parallel()
+
+	passphrase := memguard.NewBufferFromBytes([]byte("0a1b2c3d"))
+	defer passphrase.Destroy()
+
+	data := KeychainData{
+		AgePublicKey:         "age1example",
+		AgePrivKeyPassphrase: passphrase,
+		EncryptedLongtermKey: "beef",
+	}
+
+	encoded, err := data.encode()
+	require.NoError(t, err)
+
+	defer encoded.Destroy()
+
+	assert.JSONEq(t,
+		`{"agePublicKey":"age1example",`+
+			`"agePrivKeyPassphrase":"0a1b2c3d",`+
+			`"encryptedLongtermKey":"beef"}`,
+		encoded.String())
+	assert.False(t, encoded.IsMutable())
+
+	decoded, err := decodeKeychainData(encoded)
+	require.NoError(t, err)
+
+	defer decoded.AgePrivKeyPassphrase.Destroy()
+
+	assert.Equal(t, "age1example", decoded.AgePublicKey)
+	assert.Equal(t, "0a1b2c3d", decoded.AgePrivKeyPassphrase.String())
+	assert.Equal(t, "beef", decoded.EncryptedLongtermKey)
+}
+
+func TestKeychainDataEncodeRejectsBadPassphrase(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name       string
+		passphrase *memguard.LockedBuffer
+		wantErr    error
+	}{
+		{"nil", nil, errNilPassphraseBuffer},
+		{"empty", memguard.NewBuffer(0), errEmptyPassphrase},
+		{
+			"not hex",
+			memguard.NewBufferFromBytes([]byte(`abc"def`)),
+			errPassphraseNotHex,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			data := KeychainData{AgePrivKeyPassphrase: tt.passphrase}
+			_, err := data.encode()
+			require.ErrorIs(t, err, tt.wantErr)
+		})
+	}
+}
+
+func TestDecodeKeychainDataRejectsBadData(t *testing.T) {
+	t.Parallel()
+
+	for _, text := range []string{
+		`{"agePublicKey":"age1example"}`,
+		`{"agePrivKeyPassphrase":42}`,
+	} {
+		data := memguard.NewBufferFromBytes([]byte(text))
+		_, err := decodeKeychainData(data)
+		data.Destroy()
+		require.ErrorIs(t, err, errNoKeychainPassphrase, text)
+	}
+
+	notJSON := memguard.NewBufferFromBytes([]byte(`{"agePrivKeyPassphrase":`))
+	defer notJSON.Destroy()
+
+	_, err := decodeKeychainData(notJSON)
+
+	var syntaxError *json.SyntaxError
+	require.ErrorAs(t, err, &syntaxError)
+}

internal/secret/keychainunlocker.go

@@ -45,13 +45,6 @@ type KeychainUnlocker struct {
 	fs        afero.Fs
 }
 
-// KeychainData represents the data stored in the macOS keychain
-type KeychainData struct {
-	AgePublicKey         string `json:"agePublicKey"`
-	AgePrivKeyPassphrase string `json:"agePrivKeyPassphrase"`
-	EncryptedLongtermKey string `json:"encryptedLongtermKey"`
-}
-
 // GetIdentity implements Unlocker interface for Keychain-based unlockers
 func (k *KeychainUnlocker) GetIdentity() (*age.X25519Identity, error) {
 	DebugWith("Getting keychain unlocker identity",
@@ -81,13 +74,18 @@ func (k *KeychainUnlocker) GetIdentity() (*age.X25519Identity, error) {
 		slog.Int("data_length", len(keychainDataBytes)),
 	)
 
+	// Move the keychain data into locked memory; this wipes keychainDataBytes
+	keychainDataBuffer := memguard.NewBufferFromBytes(keychainDataBytes)
+	defer keychainDataBuffer.Destroy()
+
 	// Step 3: Parse keychain data
-	var keychainData KeychainData
+	keychainData, err := decodeKeychainData(keychainDataBuffer)
-	if err := json.Unmarshal(keychainDataBytes, &keychainData); err != nil {
+	if err != nil {
 		Debug("Failed to parse keychain data", "error", err, "unlocker_id", k.GetID())
 
 		return nil, fmt.Errorf("failed to parse keychain data: %w", err)
 	}
+	defer keychainData.AgePrivKeyPassphrase.Destroy()
 
 	Debug("Parsed keychain data successfully", "unlocker_id", k.GetID())
 
@@ -109,11 +107,7 @@ func (k *KeychainUnlocker) GetIdentity() (*age.X25519Identity, error) {
 
 	// Step 5: Decrypt the age private key using the passphrase from keychain
 	Debug("Decrypting age private key with keychain passphrase", "unlocker_id", k.GetID())
-	// Create secure buffer for the keychain passphrase
+	agePrivKeyBuffer, err := DecryptWithPassphrase(encryptedAgePrivKeyData, keychainData.AgePrivKeyPassphrase)
-	passphraseBuffer := memguard.NewBufferFromBytes([]byte(keychainData.AgePrivKeyPassphrase))
-	defer passphraseBuffer.Destroy()
-
-	agePrivKeyBuffer, err := DecryptWithPassphrase(encryptedAgePrivKeyData, passphraseBuffer)
 	if err != nil {
 		Debug("Failed to decrypt age private key with keychain passphrase", "error", err, "unlocker_id", k.GetID())
 
@@ -195,7 +189,7 @@ func (k *KeychainUnlocker) Remove() error {
 
 	// Step 3: Remove directory
 	Debug("Removing keychain unlocker directory", "directory", k.Directory)
-	if err := k.fs.RemoveAll(k.Directory); err != nil {
+	if err := RemoveDirAtomic(k.fs, k.Directory); err != nil {
 		Debug("Failed to remove keychain unlocker directory", "error", err, "directory", k.Directory)
 
 		return fmt.Errorf("failed to remove keychain unlocker directory: %w", err)
@@ -369,30 +363,28 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 	if err != nil {
 		return nil, fmt.Errorf("failed to generate age private key passphrase: %w", err)
 	}
+	defer agePrivKeyPassphrase.Destroy()
 
 	// Step 3: Store age recipient as plaintext
 	ageRecipient := ageIdentity.Recipient().String()
 	recipientPath := filepath.Join(unlockerDir, "pub.txt")
-	if err := afero.WriteFile(fs, recipientPath, []byte(ageRecipient), FilePerms); err != nil {
+	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 secure buffers for both the private key and passphrase
+	// Create a secure buffer for the private key
 	agePrivKeyStr := ageIdentity.String()
 	agePrivKeyBuffer := memguard.NewBufferFromBytes([]byte(agePrivKeyStr))
 	defer agePrivKeyBuffer.Destroy()
 
-	passphraseBuffer := memguard.NewBufferFromBytes([]byte(agePrivKeyPassphrase))
+	encryptedAgePrivKey, err := EncryptWithPassphrase(agePrivKeyBuffer, agePrivKeyPassphrase)
-	defer passphraseBuffer.Destroy()
-
-	encryptedAgePrivKey, err := EncryptWithPassphrase(agePrivKeyBuffer, passphraseBuffer)
 	if err != nil {
 		return nil, fmt.Errorf("failed to encrypt age private key with passphrase: %w", err)
 	}
 
 	agePrivKeyPath := filepath.Join(unlockerDir, "priv.age")
-	if err := afero.WriteFile(fs, agePrivKeyPath, encryptedAgePrivKey, FilePerms); err != nil {
+	if err := WriteFileAtomic(fs, agePrivKeyPath, encryptedAgePrivKey); err != nil {
 		return nil, fmt.Errorf("failed to write encrypted age private key: %w", err)
 	}
 
@@ -411,7 +403,7 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 
 	// Write encrypted long-term private key
 	ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age")
-	if err := afero.WriteFile(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge, FilePerms); err != nil {
+	if err := WriteFileAtomic(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge); err != nil {
 		return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err)
 	}
 
@@ -422,13 +414,10 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 		EncryptedLongtermKey: hex.EncodeToString(encryptedLtPrivKeyToAge),
 	}
 
-	keychainDataBytes, err := json.Marshal(keychainData)
+	keychainDataBuffer, err := keychainData.encode()
 	if err != nil {
-		return nil, fmt.Errorf("failed to marshal keychain data: %w", err)
+		return nil, fmt.Errorf("failed to encode keychain data: %w", err)
 	}
-
-	// Create a secure buffer for keychain data
-	keychainDataBuffer := memguard.NewBufferFromBytes(keychainDataBytes)
 	defer keychainDataBuffer.Destroy()
 
 	// Step 8: Store data in keychain
@@ -451,9 +440,9 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 		return nil, fmt.Errorf("failed to marshal unlocker metadata: %w", err)
 	}
 
-	if err := afero.WriteFile(fs,
+	if err := WriteFileAtomic(fs,
 		filepath.Join(unlockerDir, "unlocker-metadata.json"),
-		metadataBytes, FilePerms); err != nil {
+		metadataBytes); err != nil {
 		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
 	}
 
@@ -501,7 +490,7 @@ func storeInKeychain(itemName string, data *memguard.LockedBuffer) error {
 	item.SetAccount(itemName)
 	item.SetLabel(fmt.Sprintf("%s - %s", KEYCHAIN_APP_IDENTIFIER, itemName))
 	item.SetDescription("Secret vault keychain data")
-	item.SetData([]byte(data.String()))
+	item.SetData(data.Bytes())
 	item.SetSynchronizable(keychain.SynchronizableNo)
 	// Use AccessibleWhenUnlockedThisDeviceOnly for better security and to trigger auth
 	item.SetAccessible(keychain.AccessibleWhenUnlockedThisDeviceOnly)
@@ -576,8 +565,3 @@ func deleteFromKeychain(itemName string) error {
 
 	return nil
 }
-
-// generateRandomPassphrase generates a random passphrase for encrypting the age private key
-func generateRandomPassphrase(length int) (string, error) {
-	return generateRandomString(length, "0123456789abcdef")
-}

internal/secret/passphraseunlocker.go

@@ -127,7 +127,7 @@ func (p *PassphraseUnlocker) Remove() error {
 
 	// For passphrase unlockers, we just need to remove the directory
 	// No external resources (like keychain items) to clean up
-	err := p.fs.RemoveAll(p.Directory)
+	err := RemoveDirAtomic(p.fs, p.Directory)
 	if err != nil {
 		return fmt.Errorf("failed to remove passphrase unlocker directory: %w", err)
 	}

internal/secret/pgpunlocker.go

@@ -155,14 +155,18 @@ func (p *PGPUnlocker) GetDirectory() string {
 	return p.Directory
 }
 
-// GetID implements Unlocker interface - generates ID from GPG key ID
+// GetID implements Unlocker interface - generates ID from GPG key ID.
+// If the metadata has no usable GPG key ID, it warns with the unlocker's
+// directory and returns "pgp-unknown", so listing the other unlockers
+// still works.
 func (p *PGPUnlocker) GetID() string {
 	// Generate ID using GPG key ID: pgp-<keyid>
 	gpgKeyID, err := p.GetGPGKeyID()
 	if err != nil {
-		// The vault metadata is corrupt - this is a fatal error
+		Warn("PGP unlocker metadata is corrupt or missing its GPG key ID",
-		// We cannot continue with a fallback ID as that would mask data corruption
+			"directory", p.Directory, "error", err)
-		panic(fmt.Sprintf("PGP unlocker metadata is corrupt or missing GPG key ID: %v", err))
+
+		return "pgp-unknown"
 	}
 
 	return "pgp-" + gpgKeyID
@@ -172,7 +176,7 @@ func (p *PGPUnlocker) GetID() string {
 func (p *PGPUnlocker) Remove() error {
 	// For PGP unlockers, we just need to remove the directory
 	// No external resources (like keychain items) to clean up
-	err := p.fs.RemoveAll(p.Directory)
+	err := RemoveDirAtomic(p.fs, p.Directory)
 	if err != nil {
 		return fmt.Errorf("failed to remove PGP unlocker directory: %w", err)
 	}
@@ -197,6 +201,10 @@ func (p *PGPUnlocker) GetGPGKeyID() (string, error) {
 		return "", fmt.Errorf("failed to parse PGP metadata: %w", err)
 	}
 
+	if pgpMetadata.GPGKeyID == "" {
+		return "", fmt.Errorf("PGP metadata: %w", errGPGKeyIDEmpty)
+	}
+
 	return pgpMetadata.GPGKeyID, nil
 }
 
@@ -275,7 +283,7 @@ func CreatePGPUnlocker(
 	ageRecipient := ageIdentity.Recipient().String()
 	recipientPath := filepath.Join(unlockerDir, "pub.txt")
 
-	err = afero.WriteFile(fs, recipientPath, []byte(ageRecipient), FilePerms)
+	err = WriteFileAtomic(fs, recipientPath, []byte(ageRecipient))
 	if err != nil {
 		return nil, fmt.Errorf("failed to write age recipient: %w", err)
 	}
@@ -298,7 +306,7 @@ func CreatePGPUnlocker(
 	// Write encrypted long-term private key
 	ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age")
 
-	err = afero.WriteFile(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge, FilePerms)
+	err = WriteFileAtomic(fs, ltPrivKeyPath, encryptedLtPrivKeyToAge)
 	if err != nil {
 		return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err)
 	}
@@ -315,7 +323,7 @@ func CreatePGPUnlocker(
 
 	agePrivKeyPath := filepath.Join(unlockerDir, "priv.age.gpg")
 
-	err = afero.WriteFile(fs, agePrivKeyPath, encryptedAgePrivKey, FilePerms)
+	err = WriteFileAtomic(fs, agePrivKeyPath, encryptedAgePrivKey)
 	if err != nil {
 		return nil, fmt.Errorf("failed to write encrypted age private key: %w", err)
 	}
@@ -357,9 +365,8 @@ func writePGPUnlockerMetadata(
 		return nil, fmt.Errorf("failed to marshal unlocker metadata: %w", err)
 	}
 
-	err = afero.WriteFile(fs,
+	err = WriteFileAtomic(fs,
-		filepath.Join(unlockerDir, "unlocker-metadata.json"),
+		filepath.Join(unlockerDir, "unlocker-metadata.json"), metadataBytes)
-		metadataBytes, FilePerms)
 	if err != nil {
 		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
 	}

internal/secret/secret_test.go

@@ -310,64 +310,6 @@ func TestPerSecretKeyFunctionality(t *testing.T) {
 	})
 }
 
-// For testing purposes only
-func isValidSecretName(name string) bool {
-	if name == "" {
-		return false
-	}
-	// Valid characters for secret names: letters, numbers, dash, dot, underscore, slash
-	for _, char := range name {
-		if (char < 'a' || char > 'z') && // lowercase letters
-			(char < 'A' || char > 'Z') && // uppercase letters
-			(char < '0' || char > '9') && // numbers
-			char != '-' && // dash
-			char != '.' && // dot
-			char != '_' && // underscore
-			char != '/' { // slash
-			return false
-		}
-	}
-
-	return true
-}
-
-func TestSecretNameValidation(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		name  string
-		valid bool
-	}{
-		{"valid-name", true},
-		{"valid.name", true},
-		{"valid_name", true},
-		{"valid/path/name", true},
-		{"123valid", true},
-		{"", false},
-		{"Valid-Upper-Name", true}, // uppercase allowed
-		{"2025-11-21-ber1app1-vaultik-test-bucket-AKI", true}, // real-world uppercase key ID
-		{"MixedCase/Path/Name", true},                         // mixed case with path
-		{"invalid name", false},                               // space not allowed
-		{"invalid@name", false},                               // @ not allowed
-	}
-
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			t.Parallel()
-
-			result := isValidSecretName(test.name)
-			if result != test.valid {
-				t.Errorf(
-					"isValidSecretName(%q) = %v, want %v",
-					test.name,
-					result,
-					test.valid,
-				)
-			}
-		})
-	}
-}
-
 func TestSecretGetValueWithEnvMnemonicUsesVaultDerivationIndex(t *testing.T) {
 	// This test demonstrates the bug where GetValue uses hardcoded index 0
 	// instead of the vault's actual derivation index when using environment mnemonic

internal/secret/seunlocker_darwin.go

@@ -148,7 +148,7 @@ func (s *SecureEnclaveUnlocker) Remove() error {
 	}
 
 	Debug("Removing SE unlocker directory", "directory", s.Directory)
-	if err := s.fs.RemoveAll(s.Directory); err != nil {
+	if err := RemoveDirAtomic(s.fs, s.Directory); err != nil {
 		return fmt.Errorf("failed to remove SE unlocker directory: %w", err)
 	}
 
@@ -271,7 +271,7 @@ func CreateSecureEnclaveUnlocker(
 
 	// Write SE-encrypted long-term key
 	ltKeyPath := filepath.Join(unlockerDir, seLongtermFilename)
-	if err := afero.WriteFile(fs, ltKeyPath, encryptedLtKey, FilePerms); err != nil {
+	if err := WriteFileAtomic(fs, ltKeyPath, encryptedLtKey); err != nil {
 		return nil, fmt.Errorf(
 			"failed to write SE-encrypted long-term key: %w",
 			err,
@@ -295,7 +295,7 @@ func CreateSecureEnclaveUnlocker(
 	}
 
 	metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json")
-	if err := afero.WriteFile(fs, metadataPath, metadataBytes, FilePerms); err != nil {
+	if err := WriteFileAtomic(fs, metadataPath, metadataBytes); err != nil {
 		return nil, fmt.Errorf("failed to write metadata: %w", err)
 	}
 

internal/secret/version.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"log/slog"
 	"path/filepath"
+	"slices"
 	"sort"
 	"strings"
 	"time"
@@ -131,7 +132,10 @@ func GenerateVersionName(fs afero.Fs, secretDir string) (string, error) {
 	return fmt.Sprintf("%s.%03d", today, newSerial), nil
 }
 
-// Save saves the version metadata and value
+// Save saves the version metadata and value. The files are written into a
+// temporary directory that is renamed to sv.Directory once all of them are
+// complete, so the version directory is either whole or absent, even if the
+// process dies part-way.
 func (sv *Version) Save(value *memguard.LockedBuffer) error {
 	if value == nil {
 		return errNilValueBuffer
@@ -145,14 +149,22 @@ func (sv *Version) Save(value *memguard.LockedBuffer) error {
 
 	fs := sv.vault.GetFilesystem()
 
-	// Create version directory
+	// Create the versions directory the finished version is renamed into
-	err := fs.MkdirAll(sv.Directory, DirPerms)
+	err := fs.MkdirAll(filepath.Dir(sv.Directory), DirPerms)
 	if err != nil {
-		Debug("Failed to create version directory", "error", err, "dir", sv.Directory)
+		Debug("Failed to create versions directory", "error", err, "dir", sv.Directory)
 
-		return fmt.Errorf("failed to create version directory: %w", err)
+		return fmt.Errorf("failed to create versions directory: %w", err)
 	}
 
+	tmpDir, err := TempDirFor(fs, sv.Directory)
+	if err != nil {
+		return err
+	}
+
+	// Once the rename below has moved it into place, this finds nothing.
+	defer func() { _ = fs.RemoveAll(tmpDir) }()
+
 	// Generate a new keypair for this version
 	Debug("Generating version-specific keypair", "version", sv.Version)
 
@@ -173,21 +185,28 @@ func (sv *Version) Save(value *memguard.LockedBuffer) error {
 		slog.String("public_key", versionIdentity.Recipient().String()),
 	)
 
-	err = sv.writePublicKeyAndValue(fs, versionIdentity, value)
+	err = sv.writePublicKeyAndValue(fs, tmpDir, versionIdentity, value)
 	if err != nil {
 		return err
 	}
 
-	err = sv.writeEncryptedPrivateKey(fs, versionPrivateKeyBuffer)
+	err = sv.writeEncryptedPrivateKey(fs, tmpDir, versionPrivateKeyBuffer)
 	if err != nil {
 		return err
 	}
 
-	err = sv.writeEncryptedMetadata(fs, versionIdentity)
+	err = sv.writeEncryptedMetadata(fs, tmpDir, versionIdentity)
 	if err != nil {
 		return err
 	}
 
+	err = fs.Rename(tmpDir, sv.Directory)
+	if err != nil {
+		Debug("Failed to move version into place", "error", err, "dir", sv.Directory)
+
+		return fmt.Errorf("failed to move version into place: %w", err)
+	}
+
 	Debug("Successfully saved secret version",
 		"version", sv.Version, "secret_name", sv.SecretName)
 
@@ -358,17 +377,18 @@ func (sv *Version) GetValue(
 }
 
 // writePublicKeyAndValue stores the version's public key and the value
-// encrypted to it.
+// encrypted to it in dir.
 func (sv *Version) writePublicKeyAndValue(
 	fs afero.Fs,
+	dir string,
 	versionIdentity *age.X25519Identity,
 	value *memguard.LockedBuffer,
 ) error {
 	versionPublicKey := versionIdentity.Recipient().String()
-	pubKeyPath := filepath.Join(sv.Directory, "pub.age")
+	pubKeyPath := filepath.Join(dir, "pub.age")
 	Debug("Writing version public key", "path", pubKeyPath)
 
-	err := afero.WriteFile(fs, pubKeyPath, []byte(versionPublicKey), FilePerms)
+	err := WriteFileAtomic(fs, pubKeyPath, []byte(versionPublicKey))
 	if err != nil {
 		Debug("Failed to write version public key", "error", err, "path", pubKeyPath)
 
@@ -385,10 +405,10 @@ func (sv *Version) writePublicKeyAndValue(
 		return fmt.Errorf("failed to encrypt version value: %w", err)
 	}
 
-	valuePath := filepath.Join(sv.Directory, "value.age")
+	valuePath := filepath.Join(dir, "value.age")
 	Debug("Writing encrypted version value", "path", valuePath)
 
-	err = afero.WriteFile(fs, valuePath, encryptedValue, FilePerms)
+	err = WriteFileAtomic(fs, valuePath, encryptedValue)
 	if err != nil {
 		Debug("Failed to write encrypted version value", "error", err, "path", valuePath)
 
@@ -399,9 +419,10 @@ func (sv *Version) writePublicKeyAndValue(
 }
 
 // writeEncryptedPrivateKey encrypts the version's private key to the
-// vault's long-term public key and stores it.
+// vault's long-term public key and stores it in dir.
 func (sv *Version) writeEncryptedPrivateKey(
 	fs afero.Fs,
+	dir string,
 	versionPrivateKeyBuffer *memguard.LockedBuffer,
 ) error {
 	vaultDir, _ := sv.vault.GetDirectory()
@@ -435,10 +456,10 @@ func (sv *Version) writeEncryptedPrivateKey(
 		return fmt.Errorf("failed to encrypt version private key: %w", err)
 	}
 
-	privKeyPath := filepath.Join(sv.Directory, "priv.age")
+	privKeyPath := filepath.Join(dir, "priv.age")
 	Debug("Writing encrypted version private key", "path", privKeyPath)
 
-	err = afero.WriteFile(fs, privKeyPath, encryptedPrivKey, FilePerms)
+	err = WriteFileAtomic(fs, privKeyPath, encryptedPrivKey)
 	if err != nil {
 		Debug("Failed to write encrypted version private key",
 			"error", err, "path", privKeyPath)
@@ -450,9 +471,10 @@ func (sv *Version) writeEncryptedPrivateKey(
 }
 
 // writeEncryptedMetadata encrypts the version metadata to the version's
-// public key and stores it.
+// public key and stores it in dir.
 func (sv *Version) writeEncryptedMetadata(
 	fs afero.Fs,
+	dir string,
 	versionIdentity *age.X25519Identity,
 ) error {
 	Debug("Encrypting version metadata", "version", sv.Version)
@@ -476,10 +498,10 @@ func (sv *Version) writeEncryptedMetadata(
 		return fmt.Errorf("failed to encrypt version metadata: %w", err)
 	}
 
-	metadataPath := filepath.Join(sv.Directory, "metadata.age")
+	metadataPath := filepath.Join(dir, "metadata.age")
 	Debug("Writing encrypted version metadata", "path", metadataPath)
 
-	err = afero.WriteFile(fs, metadataPath, encryptedMetadata, FilePerms)
+	err = WriteFileAtomic(fs, metadataPath, encryptedMetadata)
 	if err != nil {
 		Debug("Failed to write encrypted version metadata",
 			"error", err, "path", metadataPath)
@@ -524,6 +546,18 @@ func ListVersions(fs afero.Fs, secretDir string) ([]string, error) {
 	return versions, nil
 }
 
+// VersionExists reports whether version is one of the versions ListVersions
+// lists for the secret in secretDir. It only compares names, so a version
+// the user typed can be checked with it before any path is built from it.
+func VersionExists(fs afero.Fs, secretDir string, version string) (bool, error) {
+	versions, err := ListVersions(fs, secretDir)
+	if err != nil {
+		return false, err
+	}
+
+	return slices.Contains(versions, version), nil
+}
+
 // GetCurrentVersion returns the version that the "current" file points to
 // The file contains just the version name (e.g., "20231215.001")
 func GetCurrentVersion(fs afero.Fs, secretDir string) (string, error) {
@@ -540,15 +574,12 @@ func GetCurrentVersion(fs afero.Fs, secretDir string) (string, error) {
 }
 
 // SetCurrentVersion updates the "current" file to point to a specific version
-// The file contains just the version name (e.g., "20231215.001")
+// The file contains just the version name (e.g., "20231215.001"). It is
+// replaced in one rename, so once written it always exists.
 func SetCurrentVersion(fs afero.Fs, secretDir string, version string) error {
 	currentPath := filepath.Join(secretDir, "current")
 
-	// Remove existing file if it exists
+	err := WriteFileAtomic(fs, currentPath, []byte(version))
-	_ = fs.Remove(currentPath)
-
-	// Write just the version name to the file
-	err := afero.WriteFile(fs, currentPath, []byte(version), FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to create current version file: %w", err)
 	}

internal/vault/errors.go

@@ -29,10 +29,11 @@ var (
 	// ErrNilValueBuffer indicates a nil value buffer was supplied.
 	ErrNilValueBuffer = errors.New("value buffer is nil")
 
-	// ErrInvalidSecretName indicates a secret name that does not match
+	// ErrInvalidSecretName indicates a secret name that breaks the naming
-	// the allowed pattern [a-z0-9.\-_/]+. Composed as
+	// rule: only ASCII letters, digits, '.', '-', '_' and '/'; not empty;
-	// "invalid secret name '<name>': must match pattern [a-z0-9.\-_/]+",
+	// no leading '.' or '/', no trailing '/', no '//', no '..' path segment.
-	// or as "invalid secret name: <name>" by GetSecretObject.
+	// Composed by ValidateSecretName as
+	// "invalid secret name '<name>': <the rule>".
 	ErrInvalidSecretName = errors.New("invalid secret name")
 
 	// ErrSecretExists indicates the secret already exists and --force
@@ -48,7 +49,7 @@ var (
 
 	// ErrVersionNotFound indicates the requested secret version does not
 	// exist. Composed as
-	// "version <version> not found for secret <name>".
+	// "version '<version>' not found for secret '<name>'".
 	ErrVersionNotFound = errors.New("not found for secret")
 
 	// ErrNoVersions indicates the source secret has no versions. Composed
@@ -62,4 +63,10 @@ var (
 	// ErrUnlockerNotFound indicates no unlocker with the given ID exists.
 	// Composed as "unlocker with ID <id> not found".
 	ErrUnlockerNotFound = errors.New("not found")
+
+	// ErrNoLockForFilesystem indicates LockStateDir was given a filesystem
+	// it cannot lock. Composed as "cannot lock the state directory on
+	// filesystem <type>".
+	ErrNoLockForFilesystem = errors.New(
+		"cannot lock the state directory on filesystem")
 )

internal/vault/integration_version_test.go

@@ -235,10 +235,10 @@ func testRetrieveSpecificVersions(
 	require.NoError(t, err)
 	assert.Equal(t, []byte("version-3-data"), value3)
 
-	// Empty version should return current
+	// An empty version is not one of the versions; GetSecret gets the
-	valueCurrent, err := vault.GetSecretVersion(secretName, "")
+	// current one
-	require.NoError(t, err)
+	_, err = vault.GetSecretVersion(secretName, "")
-	assert.Equal(t, []byte("version-3-data"), valueCurrent)
+	require.ErrorIs(t, err, ErrVersionNotFound)
 }
 
 func testPromoteOldVersion(

internal/vault/lock.go

@@ -0,0 +1,73 @@
+package vault
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"sync"
+	"syscall"
+
+	"git.eeqj.de/sneak/secret/internal/secret"
+	"github.com/spf13/afero"
+)
+
+// lockFileName is the file in the state directory that LockStateDir locks.
+const lockFileName = "lock"
+
+// memFsLock stands in for the lock file on the in-memory filesystem, which
+// has no file locks. Every in-memory filesystem in the process shares it.
+//
+//nolint:gochecknoglobals // must outlive the call that takes it
+var memFsLock sync.Mutex
+
+// LockStateDir takes the lock that a command changing anything under
+// stateDir holds until it returns, and returns the function that releases
+// it. While one command holds it, the next one waits here. Reads take no
+// lock: each file or directory a command changes is replaced in a single
+// rename, so a reader finds it as it was before or after, never half-made.
+//
+// On the real filesystem the lock is flock(2) on the file "lock" in
+// stateDir, which the kernel releases when the process dies, so a killed
+// command never leaves the tool locked. The in-memory filesystem the tests
+// use has no file locks, so a process-wide mutex stands in for flock there.
+// Any other filesystem is refused rather than left unlocked.
+func LockStateDir(fs afero.Fs, stateDir string) (func(), error) {
+	switch fs.(type) {
+	case *afero.OsFs:
+		return flockStateDir(stateDir)
+	case *afero.MemMapFs:
+		memFsLock.Lock()
+
+		return memFsLock.Unlock, nil
+	default:
+		return nil, fmt.Errorf("%w %T", ErrNoLockForFilesystem, fs)
+	}
+}
+
+// flockStateDir takes flock(2) on the lock file in stateDir, creating the
+// directory and the file if needed. Go opens files close-on-exec, so
+// programs the command runs, such as gpg, do not inherit the lock.
+func flockStateDir(stateDir string) (func(), error) {
+	err := os.MkdirAll(stateDir, secret.DirPerms)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create state directory: %w", err)
+	}
+
+	lockPath := filepath.Join(stateDir, lockFileName)
+
+	//nolint:gosec // G304: the path is the lock file in the state directory
+	file, err := os.OpenFile(lockPath, os.O_RDWR|os.O_CREATE, secret.FilePerms)
+	if err != nil {
+		return nil, fmt.Errorf("failed to open lock file: %w", err)
+	}
+
+	err = syscall.Flock(int(file.Fd()), syscall.LOCK_EX)
+	if err != nil {
+		_ = file.Close()
+
+		return nil, fmt.Errorf("failed to lock %s: %w", lockPath, err)
+	}
+
+	// Closing the file releases the lock.
+	return func() { _ = file.Close() }, nil
+}

internal/vault/lock_test.go

@@ -0,0 +1,134 @@
+package vault_test
+
+import (
+	"testing"
+	"time"
+
+	"git.eeqj.de/sneak/secret/internal/vault"
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+const (
+	// lockWait is how long a test waits for the lock before deciding it
+	// will never come free.
+	lockWait = 10 * time.Second
+
+	// heldWait is how long a test watches a second holder fail to take a
+	// lock that is held. Broken exclusion lets it in at once.
+	heldWait = 100 * time.Millisecond
+)
+
+// lockFilesystem is a filesystem LockStateDir can lock, with a state
+// directory on it.
+type lockFilesystem struct {
+	name     string
+	fs       afero.Fs
+	stateDir string
+}
+
+// lockFilesystems returns the real filesystem, locked with flock, and the
+// in-memory one, locked with a mutex.
+func lockFilesystems(t *testing.T) []lockFilesystem {
+	t.Helper()
+
+	return []lockFilesystem{
+		{"memory", afero.NewMemMapFs(), testStateDir},
+		{"real", afero.NewOsFs(), t.TempDir()},
+	}
+}
+
+// lockInBackground starts taking the lock and returns a channel that
+// delivers the function releasing it once it has been taken.
+func lockInBackground(
+	t *testing.T, fs afero.Fs, stateDir string,
+) <-chan func() {
+	t.Helper()
+
+	taken := make(chan func(), 1)
+
+	go func() {
+		release, err := vault.LockStateDir(fs, stateDir)
+		if assert.NoError(t, err) {
+			taken <- release
+		}
+	}()
+
+	return taken
+}
+
+// TestLockStateDirExcludes checks that while the lock is held a second
+// holder, with its own open lock file on the real filesystem, waits, and
+// that it gets the lock once the first releases it.
+func TestLockStateDirExcludes(t *testing.T) {
+	t.Parallel()
+
+	for _, lfs := range lockFilesystems(t) {
+		t.Run(lfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			release, err := vault.LockStateDir(lfs.fs, lfs.stateDir)
+			require.NoError(t, err)
+
+			taken := lockInBackground(t, lfs.fs, lfs.stateDir)
+
+			select {
+			case second := <-taken:
+				second()
+				release()
+				t.Fatal("a second holder took the lock while it was held")
+			case <-time.After(heldWait):
+			}
+
+			release()
+
+			select {
+			case second := <-taken:
+				second()
+			case <-time.After(lockWait):
+				t.Fatal("the second holder never got the lock")
+			}
+		})
+	}
+}
+
+// TestLockStateDirFreeAfterPanic checks that a holder that panics, and
+// releases the lock with defer as every command does, leaves it free.
+func TestLockStateDirFreeAfterPanic(t *testing.T) {
+	t.Parallel()
+
+	for _, lfs := range lockFilesystems(t) {
+		t.Run(lfs.name, func(t *testing.T) {
+			t.Parallel()
+
+			assert.Panics(t, func() {
+				release, err := vault.LockStateDir(lfs.fs, lfs.stateDir)
+				require.NoError(t, err)
+
+				defer release()
+
+				panic("the command failed")
+			})
+
+			select {
+			case release := <-lockInBackground(t, lfs.fs, lfs.stateDir):
+				release()
+			case <-time.After(lockWait):
+				t.Fatal("the lock was still held after its holder panicked")
+			}
+		})
+	}
+}
+
+// TestLockStateDirRefusesOtherFilesystems checks that a filesystem with no
+// lock implementation is refused instead of being used unlocked.
+func TestLockStateDirRefusesOtherFilesystems(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewReadOnlyFs(afero.NewMemMapFs())
+
+	release, err := vault.LockStateDir(fs, testStateDir)
+	require.ErrorIs(t, err, vault.ErrNoLockForFilesystem)
+	assert.Nil(t, release)
+}

internal/vault/management.go

@@ -169,7 +169,7 @@ func processMnemonicForVault(
 
 	ltPubKeyPath := filepath.Join(vaultDir, "pub.age")
 
-	err = afero.WriteFile(fs, ltPubKeyPath, []byte(ltPubKey), secret.FilePerms)
+	err = secret.WriteFileAtomic(fs, ltPubKeyPath, []byte(ltPubKey))
 	if err != nil {
 		return 0, "", "", fmt.Errorf("failed to write long-term public key: %w", err)
 	}
@@ -295,21 +295,13 @@ func SelectVault(fs afero.Fs, stateDir string, name string) error {
 		return fmt.Errorf("vault %s %w", name, ErrVaultNotFound)
 	}
 
-	// Create or update the currentvault file with just the vault name
+	// Create or replace the currentvault file with just the vault name. It
+	// is replaced in one rename, so it never goes missing.
 	currentVaultPath := filepath.Join(stateDir, "currentvault")
 
-	// Remove existing file if it exists
-	_, err = fs.Stat(currentVaultPath)
-	if err == nil {
-		secret.Debug("Removing existing currentvault file", "path", currentVaultPath)
-
-		_ = fs.Remove(currentVaultPath)
-	}
-
-	// Write just the vault name to the file
 	secret.Debug("Writing currentvault file", "vault_name", name)
 
-	err = afero.WriteFile(fs, currentVaultPath, []byte(name), secret.FilePerms)
+	err = secret.WriteFileAtomic(fs, currentVaultPath, []byte(name))
 	if err != nil {
 		return fmt.Errorf("failed to select vault: %w", err)
 	}

internal/vault/metadata.go

@@ -113,7 +113,7 @@ func SaveVaultMetadata(fs afero.Fs, vaultDir string, metadata *Metadata) error {
 		return fmt.Errorf("failed to marshal vault metadata: %w", err)
 	}
 
-	err = afero.WriteFile(fs, metadataPath, metadataBytes, secret.FilePerms)
+	err = secret.WriteFileAtomic(fs, metadataPath, metadataBytes)
 	if err != nil {
 		return fmt.Errorf("failed to write vault metadata: %w", err)
 	}

internal/vault/secrets.go

@@ -79,6 +79,7 @@ func (v *Vault) ListSecrets() ([]string, error) {
 // - No leading or trailing slashes
 // - No double slashes
 // - No names starting with dots
+// - No ".." path segments
 func isValidSecretName(name string) bool {
 	if name == "" {
 		return false
@@ -110,6 +111,22 @@ func isValidSecretName(name string) bool {
 	return matched
 }
 
+// ValidateSecretName returns an error wrapping ErrInvalidSecretName when
+// name is not a valid secret name. Call it on the name exactly as the user
+// gave it, before building any path from it.
+func ValidateSecretName(name string) error {
+	if !isValidSecretName(name) {
+		return fmt.Errorf(
+			"%w '%s': only ASCII letters, digits, '.', '-', '_' and '/' are allowed, "+
+				"and a name must not be empty, start with '.' or '/', end with '/', "+
+				"contain '//', or have '..' as a path segment",
+			ErrInvalidSecretName, name,
+		)
+	}
+
+	return nil
+}
+
 // AddSecret adds a secret to this vault
 func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool) error {
 	if value == nil {
@@ -124,13 +141,11 @@ func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool)
 	)
 
 	// Validate secret name
-	if !isValidSecretName(name) {
+	err := ValidateSecretName(name)
+	if err != nil {
 		secret.Debug("Invalid secret name provided", "secret_name", name)
 
-		return fmt.Errorf(
+		return err
-			"%w '%s': must match pattern [a-z0-9.\\-_/]+",
-			ErrInvalidSecretName, name,
-		)
 	}
 
 	secret.Debug("Secret name validation passed", "secret_name", name)
@@ -156,17 +171,59 @@ func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool)
 		slog.String("secret_dir", secretDir),
 	)
 
-	// Check for an existing secret and prepare its directory
+	// Check for an existing secret and the version the new one supersedes
-	exists, previousVersion, err := v.prepareSecretDir(name, secretDir, force)
+	exists, previousVersion, err := v.checkExistingSecret(name, secretDir, force)
 	if err != nil {
 		return err
 	}
 
+	if exists {
+		return v.addVersion(name, secretDir, value, previousVersion)
+	}
+
+	return v.addNewSecret(name, secretDir, value)
+}
+
+// addNewSecret creates a secret by assembling its first version and current
+// pointer in a temporary directory, then renaming that directory to
+// secretDir, so an interrupted add leaves no half-made secret behind.
+func (v *Vault) addNewSecret(
+	name, secretDir string, value *memguard.LockedBuffer,
+) error {
+	buildDir, err := secret.TempDirFor(v.fs, secretDir)
+	if err != nil {
+		return err
+	}
+
+	// Once the rename below has moved it into place, this finds nothing.
+	defer func() { _ = v.fs.RemoveAll(buildDir) }()
+
+	err = v.addVersion(name, buildDir, value, nil)
+	if err != nil {
+		return err
+	}
+
+	err = v.fs.Rename(buildDir, secretDir)
+	if err != nil {
+		return fmt.Errorf("failed to move new secret into place: %w", err)
+	}
+
+	return nil
+}
+
+// addVersion saves value as a new version under secretDir, sets the
+// notAfter timestamp of the version it supersedes, if any, and then points
+// current at the new version. Until that last step, current still names the
+// previous version, which stays readable.
+func (v *Vault) addVersion(
+	name, secretDir string, value *memguard.LockedBuffer,
+	previousVersion *secret.Version,
+) error {
 	now := time.Now()
 
 	// Create the new version and save the encrypted value
 	versionName, err := v.createAndSaveVersion(
-		name, secretDir, value, previousVersion, &now, exists)
+		name, secretDir, value, previousVersion, &now)
 	if err != nil {
 		return err
 	}
@@ -236,7 +293,7 @@ func updateVersionMetadata(
 	// Write encrypted metadata
 	metadataPath := filepath.Join(version.Directory, "metadata.age")
 
-	err = afero.WriteFile(fs, metadataPath, encryptedMetadata, secret.FilePerms)
+	err = secret.WriteFileAtomic(fs, metadataPath, encryptedMetadata)
 	if err != nil {
 		return fmt.Errorf("failed to write encrypted version metadata: %w", err)
 	}
@@ -244,18 +301,31 @@ func updateVersionMetadata(
 	return nil
 }
 
-// GetSecret retrieves a secret from this vault
+// GetSecret retrieves the current version of a secret from this vault
 func (v *Vault) GetSecret(name string) ([]byte, error) {
 	secret.DebugWith("Getting secret from vault",
 		slog.String("vault_name", v.Name),
 		slog.String("secret_name", name),
 	)
 
-	return v.GetSecretVersion(name, "")
+	// GetSecretObject validates the name and checks that the secret exists
+	secretObj, err := v.GetSecretObject(name)
+	if err != nil {
+		return nil, err
+	}
+
+	currentVersion, err := secret.GetCurrentVersion(v.fs, secretObj.Directory)
+	if err != nil {
+		secret.Debug("Failed to get current version", "error", err, "secret_name", name)
+
+		return nil, fmt.Errorf("failed to get current version: %w", err)
+	}
+
+	return v.GetSecretVersion(name, currentVersion)
 }
 
-// GetSecretVersion retrieves a specific version of a secret (empty version
+// GetSecretVersion retrieves a specific version of a secret. The version
-// means current)
+// must be one of the secret's versions; GetSecret gets the current one.
 func (v *Vault) GetSecretVersion(name string, version string) ([]byte, error) {
 	secret.DebugWith("Getting secret version from vault",
 		slog.String("vault_name", v.Name),
@@ -263,8 +333,8 @@ func (v *Vault) GetSecretVersion(name string, version string) ([]byte, error) {
 		slog.String("version", version),
 	)
 
-	// Validate the name and resolve the version to fetch
+	// Validate the name and check that the version exists
-	version, err := v.resolveSecretVersion(name, version)
+	err := v.checkSecretVersion(name, version)
 	if err != nil {
 		return nil, err
 	}
@@ -358,8 +428,9 @@ func (v *Vault) UnlockVault() (*age.X25519Identity, error) {
 
 // GetSecretObject retrieves a Secret object with metadata loaded from this vault
 func (v *Vault) GetSecretObject(name string) (*secret.Secret, error) {
-	if !isValidSecretName(name) {
+	err := ValidateSecretName(name)
-		return nil, fmt.Errorf("%w: %s", ErrInvalidSecretName, name)
+	if err != nil {
+		return nil, err
 	}
 
 	// First check if the secret exists by checking for the metadata file
@@ -394,12 +465,14 @@ func (v *Vault) GetSecretObject(name string) (*secret.Secret, error) {
 	return secretObj, nil
 }
 
-// CopySecretVersion copies a single version from source to this vault
+// CopySecretVersion copies a single version from source into destSecretDir
-// It decrypts the value using srcIdentity and re-encrypts for this vault
+// in this vault. It decrypts the value using srcIdentity and re-encrypts
+// for this vault.
 func (v *Vault) CopySecretVersion(
 	srcVersion *secret.Version,
 	srcIdentity *age.X25519Identity,
 	destSecretName string,
+	destSecretDir string,
 	destVersionName string,
 ) error {
 	secret.DebugWith("Copying secret version to vault",
@@ -425,6 +498,7 @@ func (v *Vault) CopySecretVersion(
 
 	// Create destination version with same name
 	destVersion := secret.NewVersion(v, destSecretName, destVersionName)
+	destVersion.Directory = filepath.Join(destSecretDir, "versions", destVersionName)
 
 	// Copy metadata (preserve original timestamps)
 	destVersion.Metadata = srcVersion.Metadata
@@ -465,11 +539,11 @@ func (v *Vault) CopySecretAllVersions(
 		return fmt.Errorf("failed to get destination vault directory: %w", err)
 	}
 
-	// Check if destination secret already exists and clear it if forced
+	// Refuse to replace an existing destination secret unless forced
 	destStorageName := strings.ReplaceAll(destSecretName, "/", "%")
 	destSecretDir := filepath.Join(destVaultDir, "secrets.d", destStorageName)
 
-	err = v.prepareCopyDestination(destSecretDir, destSecretName, force)
+	err = v.checkCopyDestination(destSecretDir, destSecretName, force)
 	if err != nil {
 		return err
 	}
@@ -505,14 +579,8 @@ func (v *Vault) CopySecretAllVersions(
 		return fmt.Errorf("failed to get current version: %w", err)
 	}
 
-	// Create destination secret directory
+	// Copy each version and the current pointer, then move the copy into place
-	err = v.fs.MkdirAll(destSecretDir, secret.DirPerms)
+	err = v.copyVersions(srcVault, srcIdentity,
-	if err != nil {
-		return fmt.Errorf("failed to create destination secret directory: %w", err)
-	}
-
-	// Copy each version and set the current pointer, rolling back on error
-	err = v.copyVersionsWithRollback(srcVault, srcIdentity,
 		srcSecretName, destSecretName, destSecretDir, versions, currentVersion)
 	if err != nil {
 		return err
@@ -527,10 +595,10 @@ func (v *Vault) CopySecretAllVersions(
 	return nil
 }
 
-// prepareSecretDir checks for an existing secret directory and prepares it
+// checkExistingSecret reports whether the secret already exists, refuses to
-// for a new version. It returns whether the secret already existed and the
+// overwrite it unless force is set, and returns its current version, which
-// current version to be superseded, if any.
+// the new version supersedes, if any.
-func (v *Vault) prepareSecretDir(
+func (v *Vault) checkExistingSecret(
 	name, secretDir string, force bool,
 ) (bool, *secret.Version, error) {
 	// Check if secret already exists
@@ -547,19 +615,6 @@ func (v *Vault) prepareSecretDir(
 	secret.Debug("Secret existence check complete", "exists", exists)
 
 	if !exists {
-		// Create secret directory for new secret
-		secret.Debug("Creating secret directory", "secret_dir", secretDir)
-
-		err = v.fs.MkdirAll(secretDir, secret.DirPerms)
-		if err != nil {
-			secret.Debug("Failed to create secret directory",
-				"error", err, "secret_dir", secretDir)
-
-			return false, nil, fmt.Errorf("failed to create secret directory: %w", err)
-		}
-
-		secret.Debug("Created secret directory successfully")
-
 		return false, nil, nil
 	}
 
@@ -624,17 +679,15 @@ func (v *Vault) updatePreviousVersion(
 	return nil
 }
 
-// resolveSecretVersion validates the secret name, verifies the secret and
+// checkSecretVersion validates the secret name and verifies that the secret
-// version exist, and resolves an empty version to the current one.
+// exists and that version is one of its versions.
-func (v *Vault) resolveSecretVersion(name, version string) (string, error) {
+func (v *Vault) checkSecretVersion(name, version string) error {
 	// Validate secret name to prevent path traversal
-	if !isValidSecretName(name) {
+	err := ValidateSecretName(name)
+	if err != nil {
 		secret.Debug("Invalid secret name provided", "secret_name", name)
 
-		return "", fmt.Errorf(
+		return err
-			"%w '%s': must match pattern [a-z0-9.\\-_/]+",
-			ErrInvalidSecretName, name,
-		)
 	}
 
 	// Get vault directory
@@ -642,7 +695,7 @@ func (v *Vault) resolveSecretVersion(name, version string) (string, error) {
 	if err != nil {
 		secret.Debug("Failed to get vault directory", "error", err, "vault_name", v.Name)
 
-		return "", err
+		return err
 	}
 
 	// Convert slashes to percent signs for storage
@@ -654,58 +707,38 @@ func (v *Vault) resolveSecretVersion(name, version string) (string, error) {
 	if err != nil {
 		secret.Debug("Failed to check if secret exists", "error", err, "secret_name", name)
 
-		return "", fmt.Errorf("failed to check if secret exists: %w", err)
+		return fmt.Errorf("failed to check if secret exists: %w", err)
 	}
 
 	if !exists {
 		secret.Debug("Secret not found in vault", "secret_name", name, "vault_name", v.Name)
 
-		return "", fmt.Errorf("secret %s %w", name, ErrSecretNotFound)
+		return fmt.Errorf("secret %s %w", name, ErrSecretNotFound)
-	}
-
-	// Determine which version to get
-	if version == "" {
-		// Get current version
-		currentVersion, err := secret.GetCurrentVersion(v.fs, secretDir)
-		if err != nil {
-			secret.Debug("Failed to get current version", "error", err, "secret_name", name)
-
-			return "", fmt.Errorf("failed to get current version: %w", err)
-		}
-
-		version = currentVersion
-
-		secret.Debug("Using current version", "version", version, "secret_name", name)
 	}
 
 	// Check if version exists
-	versionPath := filepath.Join(secretDir, "versions", version)
+	exists, err = secret.VersionExists(v.fs, secretDir, version)
-
-	exists, err = afero.DirExists(v.fs, versionPath)
 	if err != nil {
 		secret.Debug("Failed to check if version exists", "error", err, "version", version)
 
-		return "", fmt.Errorf("failed to check if version exists: %w", err)
+		return fmt.Errorf("failed to check if version exists: %w", err)
 	}
 
 	if !exists {
 		secret.Debug("Version not found", "version", version, "secret_name", name)
 
-		return "", fmt.Errorf(
+		return fmt.Errorf("version '%s' %w '%s'", version, ErrVersionNotFound, name)
-			"version %s %w %s",
-			version, ErrVersionNotFound, name,
-		)
 	}
 
-	return version, nil
+	return nil
 }
 
 // createAndSaveVersion generates a new version name, sets the version
-// timestamps, and saves the encrypted value. When saving fails for a newly
+// timestamps, and saves the encrypted value under secretDir, which is a
-// created secret, the secret directory is removed again.
+// temporary directory while a new secret is being assembled.
 func (v *Vault) createAndSaveVersion(
 	name, secretDir string, value *memguard.LockedBuffer,
-	previousVersion *secret.Version, now *time.Time, exists bool,
+	previousVersion *secret.Version, now *time.Time,
 ) (string, error) {
 	// Generate new version name
 	versionName, err := secret.GenerateVersionName(v.fs, secretDir)
@@ -719,6 +752,7 @@ func (v *Vault) createAndSaveVersion(
 
 	// Create new version
 	newVersion := secret.NewVersion(v, name, versionName)
+	newVersion.Directory = filepath.Join(secretDir, "versions", versionName)
 
 	// Set version timestamps
 	if previousVersion == nil {
@@ -738,57 +772,73 @@ func (v *Vault) createAndSaveVersion(
 	if err != nil {
 		secret.Debug("Failed to save new version", "error", err, "version", versionName)
 
-		// Clean up the secret directory if this was a new secret
-		if !exists {
-			secret.Debug("Cleaning up secret directory due to save failure",
-				"secret_dir", secretDir)
-
-			_ = v.fs.RemoveAll(secretDir)
-		}
-
 		return "", fmt.Errorf("failed to save version: %w", err)
 	}
 
 	return versionName, nil
 }
 
-// copyVersionsWithRollback copies each version of the source secret into the
+// copyVersions copies each version of the source secret and its current
-// destination directory and sets the current version pointer, removing the
+// pointer into a temporary directory, then moves that directory to
-// partial copy when any step fails.
+// destSecretDir, replacing a secret already there. Nothing in this vault
-func (v *Vault) copyVersionsWithRollback(
+// changes until the copy is complete, so an interrupted copy leaves only a
+// temporary directory behind.
+func (v *Vault) copyVersions(
 	srcVault *Vault, srcIdentity *age.X25519Identity,
 	srcSecretName, destSecretName, destSecretDir string,
 	versions []string, currentVersion string,
 ) error {
-	// Copy each version
+	buildDir, err := secret.TempDirFor(v.fs, destSecretDir)
+	if err != nil {
+		return err
+	}
+
+	// Once the rename below has moved it into place, this finds nothing.
+	defer func() { _ = v.fs.RemoveAll(buildDir) }()
+
 	for _, versionName := range versions {
 		srcVersion := secret.NewVersion(srcVault, srcSecretName, versionName)
 
-		err := v.CopySecretVersion(srcVersion, srcIdentity, destSecretName, versionName)
+		err = v.CopySecretVersion(
+			srcVersion, srcIdentity, destSecretName, buildDir, versionName)
 		if err != nil {
-			// Rollback: remove partial copy
-			secret.Debug("Rolling back partial copy due to error", "error", err)
-
-			_ = v.fs.RemoveAll(destSecretDir)
-
 			return fmt.Errorf("failed to copy version %s: %w", versionName, err)
 		}
 	}
 
-	// Set current version
+	err = secret.SetCurrentVersion(v.fs, buildDir, currentVersion)
-	err := secret.SetCurrentVersion(v.fs, destSecretDir, currentVersion)
 	if err != nil {
-		_ = v.fs.RemoveAll(destSecretDir)
-
 		return fmt.Errorf("failed to set current version: %w", err)
 	}
 
+	// With --force, the secret being replaced goes only now that its
+	// replacement is complete
+	exists, err := afero.DirExists(v.fs, destSecretDir)
+	if err != nil {
+		return fmt.Errorf("failed to check destination: %w", err)
+	}
+
+	if exists {
+		secret.Debug("Removing existing destination secret", "path", destSecretDir)
+
+		err = secret.RemoveDirAtomic(v.fs, destSecretDir)
+		if err != nil {
+			return fmt.Errorf("failed to remove existing destination secret: %w", err)
+		}
+	}
+
+	err = v.fs.Rename(buildDir, destSecretDir)
+	if err != nil {
+		return fmt.Errorf("failed to move copied secret into place: %w", err)
+	}
+
 	return nil
 }
 
-// prepareCopyDestination ensures the destination secret directory can be
+// checkCopyDestination refuses to copy over an existing secret unless force
-// created, removing an existing secret when force is set.
+// is set. A secret being replaced is removed by copyVersions, once its
-func (v *Vault) prepareCopyDestination(
+// replacement is complete.
+func (v *Vault) checkCopyDestination(
 	destSecretDir, destSecretName string, force bool,
 ) error {
 	exists, err := afero.DirExists(v.fs, destSecretDir)
@@ -803,15 +853,5 @@ func (v *Vault) prepareCopyDestination(
 		)
 	}
 
-	if exists && force {
-		// Remove existing secret
-		secret.Debug("Removing existing destination secret", "path", destSecretDir)
-
-		err = v.fs.RemoveAll(destSecretDir)
-		if err != nil {
-			return fmt.Errorf("failed to remove existing destination secret: %w", err)
-		}
-	}
-
 	return nil
 }

internal/vault/secrets_version_test.go

@@ -202,10 +202,10 @@ func TestVaultGetSecretVersion(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, []byte("version-2"), value)
 
-	// Get current (empty version)
+	// An empty version is not one of the versions; GetSecret gets the
-	value, err = vault.GetSecretVersion(testSecretPath, "")
+	// current one
-	require.NoError(t, err)
+	_, err = vault.GetSecretVersion(testSecretPath, "")
-	assert.Equal(t, []byte("version-2"), value)
+	require.ErrorIs(t, err, ErrVersionNotFound)
 }
 
 //nolint:paralleltest // createTestVaultWithKey uses t.Setenv

internal/vault/unlockers.go

@@ -233,9 +233,10 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) {
 
 			exists, err := afero.Exists(v.fs, metadataPath)
 			if err != nil {
-				return nil, fmt.Errorf(
+				secret.Warn("Skipping unlocker directory whose metadata file cannot be checked",
-					"failed to check if metadata exists for unlocker %s: %w",
+					"directory", file.Name(), "error", err)
-					file.Name(), err)
+
+				continue
 			}
 
 			if !exists {
@@ -247,16 +248,20 @@ func (v *Vault) ListUnlockers() ([]UnlockerMetadata, error) {
 
 			metadataBytes, err := afero.ReadFile(v.fs, metadataPath)
 			if err != nil {
-				return nil, fmt.Errorf(
+				secret.Warn("Skipping unlocker directory with unreadable metadata file",
-					"failed to read metadata for unlocker %s: %w", file.Name(), err)
+					"directory", file.Name(), "error", err)
+
+				continue
 			}
 
 			var metadata UnlockerMetadata
 
 			err = json.Unmarshal(metadataBytes, &metadata)
 			if err != nil {
-				return nil, fmt.Errorf(
+				secret.Warn("Skipping unlocker directory with corrupt metadata file",
-					"failed to parse metadata for unlocker %s: %w", file.Name(), err)
+					"directory", file.Name(), "error", err)
+
+				continue
 			}
 
 			unlockers = append(unlockers, metadata)
@@ -310,30 +315,16 @@ func (v *Vault) SelectUnlocker(unlockerID string) error {
 		return fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound)
 	}
 
-	// Create/update current-unlocker file with just the unlocker name
+	// Create or replace the current-unlocker file with just the unlocker
+	// name. It is replaced in one rename, so it never goes missing.
 	currentUnlockerPath := filepath.Join(vaultDir, "current-unlocker")
 
-	// Remove existing file if it exists
-	exists, err := afero.Exists(v.fs, currentUnlockerPath)
-	if err != nil {
-		return fmt.Errorf("failed to check if current-unlocker file exists: %w", err)
-	}
-
-	if exists {
-		err = v.fs.Remove(currentUnlockerPath)
-		if err != nil {
-			return fmt.Errorf("failed to remove existing current-unlocker file: %w", err)
-		}
-	}
-
 	// Get just the unlocker name (basename of the directory)
 	unlockerName := filepath.Base(targetUnlockerDir)
 
-	// Write just the unlocker name to the file
 	secret.Debug("Writing current-unlocker file", "unlocker_name", unlockerName)
 
-	err = afero.WriteFile(v.fs, currentUnlockerPath, []byte(unlockerName),
+	err = secret.WriteFileAtomic(v.fs, currentUnlockerPath, []byte(unlockerName))
-		secret.FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to create current-unlocker file: %w", err)
 	}
@@ -351,6 +342,14 @@ func (v *Vault) CreatePassphraseUnlocker(
 		return nil, fmt.Errorf("failed to get vault directory: %w", err)
 	}
 
+	// We need to get the long-term key (either from memory if unlocked, or
+	// derive it). Getting it before anything is written means failing to
+	// get it changes nothing, even when replacing the current unlocker.
+	ltIdentity, err := v.GetOrDeriveLongTermKey()
+	if err != nil {
+		return nil, fmt.Errorf("failed to get long-term key: %w", err)
+	}
+
 	// Create unlocker directory
 	unlockerDir := filepath.Join(vaultDir, "unlockers.d", unlockerTypePassphrase)
 
@@ -371,33 +370,7 @@ func (v *Vault) CreatePassphraseUnlocker(
 		return nil, err
 	}
 
-	// Create metadata
-	metadata := UnlockerMetadata{
-		Type:      unlockerTypePassphrase,
-		CreatedAt: time.Now(),
-		Flags:     []string{},
-	}
-
-	// Write metadata
-	metadataBytes, err := json.MarshalIndent(metadata, "", "  ")
-	if err != nil {
-		return nil, fmt.Errorf("failed to marshal metadata: %w", err)
-	}
-
-	metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json")
-
-	err = afero.WriteFile(v.fs, metadataPath, metadataBytes, secret.FilePerms)
-	if err != nil {
-		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
-	}
-
 	// Encrypt long-term private key to this unlocker
-	// We need to get the long-term key (either from memory if unlocked, or derive it)
-	ltIdentity, err := v.GetOrDeriveLongTermKey()
-	if err != nil {
-		return nil, fmt.Errorf("failed to get long-term key: %w", err)
-	}
-
 	ltPrivKeyBuffer := memguard.NewBufferFromBytes([]byte(ltIdentity.String()))
 	defer ltPrivKeyBuffer.Destroy()
 
@@ -409,11 +382,31 @@ func (v *Vault) CreatePassphraseUnlocker(
 
 	ltPrivKeyPath := filepath.Join(unlockerDir, "longterm.age")
 
-	err = afero.WriteFile(v.fs, ltPrivKeyPath, encryptedLtPrivKey, secret.FilePerms)
+	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(),
+		Flags:     []string{},
+	}
+
+	metadataBytes, err := json.MarshalIndent(metadata, "", "  ")
+	if err != nil {
+		return nil, fmt.Errorf("failed to marshal metadata: %w", err)
+	}
+
+	metadataPath := filepath.Join(unlockerDir, "unlocker-metadata.json")
+
+	err = secret.WriteFileAtomic(v.fs, metadataPath, metadataBytes)
+	if err != nil {
+		return nil, fmt.Errorf("failed to write unlocker metadata: %w", err)
+	}
+
 	// Create the unlocker instance
 	unlocker := secret.NewPassphraseUnlocker(v.fs, unlockerDir, metadata)
 
@@ -467,9 +460,8 @@ func (v *Vault) writeUnlockerKeypair(
 	// Write public key
 	pubKeyPath := filepath.Join(unlockerDir, "pub.age")
 
-	err := afero.WriteFile(v.fs, pubKeyPath,
+	err := secret.WriteFileAtomic(v.fs, pubKeyPath,
-		[]byte(unlockerIdentity.Recipient().String()),
+		[]byte(unlockerIdentity.Recipient().String()))
-		secret.FilePerms)
 	if err != nil {
 		return fmt.Errorf("failed to write unlocker public key: %w", err)
 	}
@@ -488,7 +480,7 @@ func (v *Vault) writeUnlockerKeypair(
 	// Write encrypted private key
 	privKeyPath := filepath.Join(unlockerDir, "priv.age")
 
-	err = afero.WriteFile(v.fs, privKeyPath, encryptedPrivKey, secret.FilePerms)
+	err = secret.WriteFileAtomic(v.fs, privKeyPath, encryptedPrivKey)
 	if err != nil {
 		return fmt.Errorf("failed to write encrypted unlocker private key: %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 {

internal/vault/vault_error_test.go

@@ -90,4 +90,10 @@ func TestAddSecretCleansUpOnFailure(t *testing.T) {
 	secretDir := filepath.Join(vaultDir, "secrets.d", testSecretName)
 	exists, _ := afero.DirExists(fs, secretDir)
 	assert.False(t, exists, "Secret directory should not exist after failed AddSecret")
+
+	// Nor is the temporary directory the secret was assembled in left behind
+	entries, err := afero.ReadDir(fs, vaultDir)
+	require.NoError(t, err)
+	require.Len(t, entries, 1)
+	assert.Equal(t, "pub.age", entries[0].Name())
 }

script/bootstrap

@@ -6,6 +6,7 @@
 # make, node, yarn, go, or python). Node is used directly if installed;
 # otherwise a pinned version is installed via nvm (installing nvm
 # itself first, from a hash-verified release archive, never curl | sh).
+# golangci-lint is never installed: script/lint runs it in docker.
 #
 # Uncomment the language sections in main() that apply to this repo.
 set -eu
@@ -136,12 +137,6 @@ main() {
 
     # ---- Go repos ----
     if missing go; then pkg_install go golang go go; fi
-    # golangci-lint: packaged in nix, brew, and apk. On apt there is no
-    # package: download a specific release archive from GitHub and
-    # verify its hash (verify_sha256), never curl | sh.
-    if missing golangci-lint; then
-        pkg_install golangci-lint golangci-lint golangci-lint golangci-lint
-    fi
     go mod download
 
     # ---- Python repos ----

script/cibuild

@@ -1,8 +1,9 @@
 #!/bin/sh
 # script/cibuild: run the CI build. The Dockerfile runs script/check
 # (via make check), so a successful build implies all checks pass.
-# The Gitea workflow runs this on push. The memlock ulimit is required
+# The Gitea workflow runs this on push. The memlock ulimit lets the tests
-# because the test suite uses memguard, which mlocks memory.
+# that lock large secrets in memory (memguard mlocks them) run; under the
+# lower limit of a plain `docker build .` they are skipped.
 set -eu
 
 ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"

script/docker

@@ -1,7 +1,8 @@
 #!/bin/sh
 # script/docker: build the Docker image tagged with the project name.
 # Identical in all repos; the tag comes from script/projectname.
-# Generic: needs no adaptation.
+# --no-cache because the gate phases the final stage depends on are RUN
+# steps, and a cached one is a check that did not run.
 set -eu
 
 SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)"
@@ -9,7 +10,15 @@ ROOT="$(cd "$SCRIPT_DIR/.." && pwd -P)"
 
 main() {
     cd "$ROOT"
-    docker build -t "$("$SCRIPT_DIR/projectname")" .
+    # Own line: a failing command substitution inside an argument does
+    # not trip `set -e`, so the inline form degrades silently to an
+    # empty constant. The VERSION build argument takes precedence over
+    # the version a build stage derives from the .git in the context.
+    version="$(git describe --tags --always --dirty 2>/dev/null || true)"
+    [ -n "$version" ] || version="unknown"
+    docker build --no-cache \
+        --build-arg VERSION="$version" \
+        -t "$("$SCRIPT_DIR/projectname")" .
 }
 
 main "$@"

script/lint

@@ -1,14 +1,24 @@
 #!/bin/sh
-# script/lint: run the linter.
+# script/lint: run the linter, in docker only. Builds Dockerfile.lint,
+# where golangci-lint runs as a build step.
+#
+# A cached build lints nothing, so --no-cache-filter rebuilds the lint
+# stage on every run, an unchanged tree included. It ignores a stage name
+# that does not exist, so --target names the same stage: a rename then
+# fails the build instead of serving the lint from cache. cacheonly keeps
+# no image; only the build's success matters.
 set -eu
 
 ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"
 
 main() {
     cd "$ROOT"
-    # CGO is required (Makefile exports this too)
+    docker build \
-    export CGO_ENABLED=1
+        --progress=plain \
-    golangci-lint run --timeout 5m
+        --target lint \
+        --no-cache-filter=lint \
+        --output=type=cacheonly \
+        -f Dockerfile.lint .
 }
 
 main "$@"