Set the time zone in main and run Entry in its own test process (closes #192)

cli.Entry() set time.Local to UTC and ended with memguard.Purge(), and
TestEntryWipesBuffersAndReturnsExitCode ran it in the same process as
the other tests. A timer left by an earlier test could read time.Local
as it changed, which the race detector reported on some runs, and the
purge could destroy a buffer still in use, the likeliest cause of a
crash seen on another run.

main() in cmd/secret now sets time.Local before it calls cli.Entry();
the test that runs the built binary with TZ=America/New_York still
covers it. The Entry test runs cli.Entry() in a copy of the test binary
started with SECRET_TEST_RUN_ENTRY set, which init picks up so that
TestMain does not run there.

Model: opus-5-5

README.md

@@ -67,6 +67,10 @@ and unlocker set. This allows for complete isolation between different contexts
 
 ## Commands Reference
 
+Every time `secret` prints, in a table or in `--json` output, is in UTC,
+whatever `TZ` or the system's time zone is set to. The date in a version's name
+is the UTC date too.
+
 ### Confirmation Before Removal
 
 `secret rm`, `secret version rm`, `secret vault remove` and
@@ -114,7 +118,11 @@ are allowed, and a name must not be empty, `.` or `..`.
 
 #### `secret vault select <name>`
 
-Switches to the specified vault for subsequent operations.
+Switches to the specified vault for subsequent operations. When `currentvault`
+is empty or names a vault that does not exist, every command that uses the
+current vault fails with an error that names the file and says to run
+`secret vault select <name>`. `secret vault list` still lists the vaults, and
+`secret info` still runs; both show no vault as current.
 
 #### `secret vault remove <name> [--force]` / `secret vault rm` ⚠️ 🛑
 
@@ -142,7 +150,11 @@ value may be over 100 MiB, whatever the limit.
 
 #### `secret add <secret-name> [--force]`
 
-Adds a secret to the current vault. Reads the secret value from stdin.
+Adds a secret to the current vault. Reads the secret value from stdin. When the
+value ends in a newline, that one newline is dropped, so
+`echo value | secret add name` stores `value`; from `printf 'a\n\n'` it stores
+`a` and one newline. To keep a file's final newline, as in a PEM key, use
+`secret import`, which stores the file's bytes unchanged.
 
 - `--force, -f`: Overwrite existing secret
 
@@ -155,13 +167,13 @@ contain `//`, or have `..` as a path segment.
 
 #### `secret get <secret-name> [--version <version>]`
 
-Retrieves and outputs a secret value to stdout. A failed write of the value,
-such as to a file on a full disk, prints the error and exits 1, with two
-exceptions. If the program reading from a pipe has already exited, as in
-`secret get s | true`, the broken-pipe signal (SIGPIPE) ends `secret` with no
-message and the shell reports exit status 141. If the shell closed stdout before
-`secret` started (`secret get s >&-`), the value is written to `/dev/null` and
-`secret get` exits 0.
+Writes the secret's value to stdout exactly as stored, with no newline added
+after it. A failed write of the value, such as to a file on a full disk, prints
+the error and exits 1, with two exceptions. If the program reading from a pipe
+has already exited, as in `secret get s | true`, the broken-pipe signal
+(SIGPIPE) ends `secret` with no message and the shell reports exit status 141.
+If the shell closed stdout before `secret` started (`secret get s >&-`), the
+value is written to `/dev/null` and `secret get` exits 0.
 
 - `--version, -v`: Get a specific version (default: current)
 
@@ -204,6 +216,12 @@ Moves or renames a secret within the current vault.
 Lists all versions of a secret showing creation time, status, and validity
 period.
 
+`STATUS` is `current` for the secret's current version, `expired` for another
+version whose `NOT_AFTER` has passed, and `inactive` for any other version.
+Adding a version sets `NOT_AFTER` on the version that was current;
+`secret version promote` sets no times. A version that is not current therefore
+shows `inactive` when no version was added while it was current.
+
 #### `secret version promote <secret-name> <version>`
 
 Promotes a specific version to current by rewriting the secret's `current` file
@@ -261,6 +279,12 @@ Creates a new unlocker of the specified type:
 A vault has one passphrase unlocker: adding one replaces the one the vault has,
 which is removed only once the new one is the current unlocker.
 
+`secret unlocker add passphrase` asks on the terminal for the new passphrase
+even when `SB_UNLOCK_PASSPHRASE` is set, and uses the variable to unlock the
+vault. With no controlling terminal, as under cron or in a CI job, the new
+unlocker takes its passphrase from `SB_UNLOCK_PASSPHRASE`, and the command says
+so.
+
 #### `secret unlocker remove <unlocker-id> [--force]` / `secret unlocker rm` ⚠️ 🛑
 
 **DANGER**: Permanently removes an unlocker. It first asks for confirmation,
@@ -294,10 +318,16 @@ Selects an unlocker as the current default for operations.
 #### `secret import <secret-name> --source <filename>`
 
 Imports a secret from a file and stores it in the current vault under the given
-name.
+name. The file's bytes are stored unchanged, a final newline included.
 
 ### Encryption Operations
 
+With `--output`, `secret encrypt` and `secret decrypt` hold the result in memory
+and write it to `--output` only once encryption or decryption has succeeded, so
+a failure to encrypt or decrypt leaves `--output` untouched. A failure while
+writing `--output`, such as a full disk, can leave it empty or partly written.
+The command can need up to about four times as much memory as the result.
+
 #### `secret encrypt <secret-name> [--input=file] [--output=file]`
 
 Encrypts data using an Age key stored as a secret. If the secret doesn't exist,
@@ -408,6 +438,14 @@ asks when stdin is a pipe, as in `echo value | secret add name --force` or
 `cat file | secret encrypt key`. With no controlling terminal, as under cron or
 in a CI job, the command fails and names the variable to set.
 
+A command that changes anything locks the state directory, through its file
+`lock`, and another such command waits until the lock is released.
+`secret init`, `secret vault create`, `secret unlocker add`, `secret mv` from
+one vault to another, and `secret add`, `secret import` and
+`secret generate secret` with `--force`, ask for the mnemonic or passphrase
+before they take the lock, so a prompt of theirs left open in one terminal holds
+up no command in another.
+
 ## Security Features
 
 ### Encryption
@@ -544,11 +582,11 @@ secret decrypt encryption/mykey --input document.txt.age --output document.txt
 ### Vault Management
 
 - **Derivation Index**: Each vault uses a unique derivation index from the
-  mnemonic, and thus a unique key pair; creating a vault or importing a mnemonic
-  fails, with an error naming the file, when any vault's `vault-metadata.json`
-  exists but cannot be read or parsed; a vault directory with no
-  `vault-metadata.json` is skipped, so the index it uses can be given to a new
-  vault
+  mnemonic, and thus a unique key pair; creating a vault (`secret init` and
+  `secret vault create`) fails, with an error naming the file, when any vault's
+  `vault-metadata.json` exists but cannot be read or parsed; a vault directory
+  with no `vault-metadata.json` is skipped, so the index it uses can be given to
+  a new vault
 - **Public Key Hash**: Double SHA-256 hash of the vault's public key; the same
   hash of the index-0 public key identifies vaults from the same mnemonic
 - **Automatic Key Derivation**: When creating vaults with a mnemonic, keys are

TODO.md

@@ -8,8 +8,7 @@
 
 # Status
 
-pre-1.0. No git tags. Open work is tracked on the issue tracker, which is
-authoritative.
+pre-1.0. Open work is tracked on the issue tracker, which is authoritative.
 
 # Next Step
 
@@ -18,6 +17,183 @@ https://git.eeqj.de/sneak/secret/milestone/12
 
 # Completed Steps
 
+- 2026-10-15: `make test` no longer fails on some runs in `internal/cli`
+  (https://git.eeqj.de/sneak/secret/issues/192). `cli.Entry()` set `time.Local`
+  to UTC and ended by wiping every memguard buffer, and a test ran it in the
+  same process as the other tests. A timer left by an earlier test could read
+  `time.Local` as it changed, which the race detector reported, and the wipe
+  could destroy a buffer still in use, the likeliest cause of a crash of the
+  test process on another run. `main()` in `cmd/secret` now sets `time.Local`
+  before it calls `cli.Entry()`, and that test runs `cli.Entry()` in a process
+  of its own.
+- 2026-10-15: `secret mv` from one vault to another unlocks both vaults before
+  it takes the state directory lock
+  (https://git.eeqj.de/sneak/secret/issues/189). It unlocked them while holding
+  the lock, and unlocking can ask for a passphrase, so every other command that
+  changes anything waited at that prompt. `moveSecretCrossVault` now checks the
+  move, refusing an existing destination secret without `--force`, unlocks both
+  vaults, and only then takes the lock. Under the lock, it checks that the
+  destination vault still has the long-term key it unlocked, and
+  `copySecretAndRemoveSource` checks the move again before it copies. A move
+  within a vault takes the lock in `moveSecretWithinVault`, so `MoveSecret`
+  takes none itself. The error for a vault created again with another key, which
+  `secret unlocker add` shares, now ends "nothing was changed".
+- 2026-10-15: `secret init`, `secret vault create`, `secret unlocker add`, and
+  `secret add`, `secret import` and `secret generate secret` with `--force` ask
+  for the mnemonic or passphrase, or unlock the vault, before they take the
+  state directory lock (https://git.eeqj.de/sneak/secret/issues/152). They did
+  so while holding the lock, so every other command that changes anything waited
+  at the prompt. Under the lock, `secret unlocker add` fails if the vault was
+  removed, or created again with another key, and refuses a GPG key added
+  meanwhile; a forced add of a secret created meanwhile lets go of the lock,
+  gets the key and takes the lock again. `secret.CreatePGPUnlocker` takes the
+  unlocked vault; `SetMnemonic` and `SetUnlockPassphrase` are gone.
+- 2026-10-15: A `secret mv` to another vault that copies the secret but cannot
+  remove the source fails (https://git.eeqj.de/sneak/secret/issues/159). It
+  printed a warning and the usual "Moved" line and exited 0. If the source
+  secret's directory is still in place, the error says the copy holds all
+  versions and the source remains. If the source left its vault and only the
+  temporary directory it was moved into could not be deleted, the error says the
+  secret is moved and names that directory, which the error of `RemoveDirAtomic`
+  now does.
+- 2026-10-15: `secret encrypt` and `secret decrypt` with `--output` hold the
+  result in memory and open the output file only once encryption or decryption
+  has succeeded (https://git.eeqj.de/sneak/secret/issues/143). They emptied that
+  file before reading any input, so a failed command left it empty, and
+  encrypting a file onto itself gave the encryption of nothing. A failure to
+  encrypt or decrypt now leaves `--output` untouched, and `--input` can name the
+  same file. A failure while writing `--output`, such as a full disk, can still
+  leave it empty or partly written; an error from closing it is reported.
+  Without `--output` the result still goes to stdout as it is produced.
+- 2026-10-15: A command that uses the current vault fails when `currentvault` is
+  empty or names no vault (https://git.eeqj.de/sneak/secret/issues/156).
+  `secret list` reported an empty vault with exit status 0, and `secret add`
+  failed on a missing directory. `ResolveVaultSymlink` now refuses an empty or
+  invalid name, or one with no directory under `vaults.d`, with an error that
+  wraps `ErrVaultNotFound`, names the file and says to run
+  `secret vault select <name>`. `secret vault list` and `secret info` still run.
+- 2026-10-15: `secret` prints every time in UTC, whatever the machine's time
+  zone (https://git.eeqj.de/sneak/secret/issues/169). `secret list`,
+  `secret version list`, `secret unlocker list` and `secret info` printed times
+  in the local time zone without naming it, so one vault showed different times
+  on two machines. `Entry()` now sets `time.Local` to UTC before anything else,
+  as policy 6 in `AGENTS.md` asks, so every time the program prints or writes is
+  in UTC. `--json` times end in `Z`, and a new version's name takes the UTC
+  date. The README says so.
+- 2026-10-15: `secret vault`, `secret unlocker`, `secret version` and
+  `secret generate` fail when given a subcommand they do not have
+  (https://git.eeqj.de/sneak/secret/issues/183). They printed their help and
+  exited 0, so a mistyped command looked like one that worked. Each now exits 1
+  with `unknown command "<name>" for "secret <parent>"`, and with no arguments
+  still prints its help. Cobra checks a command's `Args` only when it has a
+  `RunE`, so each gets `Args: cobra.NoArgs` and a `RunE` that prints its help;
+  their help now shows a `secret <parent> [flags]` usage line.
+  `secret vault help` fails as an unknown subcommand, while `secret help vault`
+  and `secret vault --help` still work.
+- 2026-10-15: With `SB_UNLOCK_PASSPHRASE` set, `secret unlocker add passphrase`
+  asks on the terminal for the new unlocker's passphrase and uses the variable
+  to unlock the vault (https://git.eeqj.de/sneak/secret/issues/154). It gave the
+  new unlocker the passphrase in the variable without asking. With no
+  controlling terminal, the new unlocker takes the variable's passphrase and the
+  output says so. `secret.ErrNoTerminal` is exported so that the command can
+  tell a missing terminal from other failures to read, such as nothing typed or
+  a confirmation that does not match, which still fail the command.
+- 2026-10-15: `secret init` and `secret vault create` stop with an error naming
+  the file when a vault's `vault-metadata.json` cannot be read or parsed, and
+  leave the state directory as it was
+  (https://git.eeqj.de/sneak/secret/issues/140). `GetNextDerivationIndex` passed
+  over such a vault, so the next vault made from the same mnemonic could get its
+  derivation index and so the same long-term key. A vault directory with no
+  metadata file is still skipped.
+- 2026-10-15: The README says that `secret add` drops one trailing newline from
+  stdin, that `secret import` stores a file's bytes unchanged, and that
+  `secret get` writes the value with no newline added
+  (https://git.eeqj.de/sneak/secret/issues/158). It said none of this, so a PEM
+  key piped into `secret add` came back one byte short with no hint why. A new
+  test stores `a\n\n` through `add` and through `import` and checks that `add`
+  kept two bytes and `import` three. No program code changes.
+- 2026-10-15: When gpg fails to decrypt, to encrypt or to look up a key's
+  fingerprint, the error ends with what gpg wrote to stderr, trimmed
+  (https://git.eeqj.de/sneak/secret/issues/146). It gave only gpg's exit status,
+  so the user could not tell why. `cmd.Output` keeps gpg's stderr in the
+  `*exec.ExitError` it returns, and the three errors now use it. When gpg wrote
+  nothing to stderr the error is as before, and when gpg wrote several lines the
+  error has them all.
+- 2026-10-15: `secret version list` no longer shows a version without
+  `NOT_AFTER` as expired (https://git.eeqj.de/sneak/secret/issues/150). STATUS
+  was `expired` for every version other than the current one, so after a promote
+  the newest version, which has no `NOT_AFTER`, read `expired`. STATUS is now
+  `current` for the current version, `expired` for another version whose
+  `NOT_AFTER` has passed, and `inactive` for any other version. The README
+  describes the three.
+- 2026-10-15: `secret unlocker list --json` for a vault with no unlockers, and
+  `secret vault list --json` when `vaults.d` holds no vaults, print the empty
+  list as `[]` instead of `null` (https://git.eeqj.de/sneak/secret/issues/148).
+  Both lists were nil slices, which `encoding/json` writes as `null`; each is
+  now made as an empty slice before it is filled. `vault.ListVaults` therefore
+  returns an empty slice instead of nil for an empty `vaults.d`.
+- 2026-10-15: `secret add`, `secret import` and `secret generate secret` fail
+  with an error, not memguard's panic, when a secret is too large for the
+  locked-memory limit (https://git.eeqj.de/sneak/secret/issues/145). The value
+  was held twice, so half the usual 8 MiB limit was enough to crash them.
+  `secret` now raises the limit to the hard limit at startup, and each of the
+  three commands refuses, before its first locked allocation, a limit that
+  leaves fewer than 8 pages for its keys and buffers. The value is read a page
+  at a time into a `memguard.Stream`, which keeps it encrypted, and copied once
+  into locked memory after its size is checked against 100 MiB and the limit. It
+  leaves the stream through `Next`, not `Read`, because `Read` re-encrypts a
+  leftover piece and memguard hangs if that runs out of locked memory.
+- 2026-10-15: Total Size in `secret info` counts every file of each version: the
+  value, the metadata and the version's two keys
+  (https://git.eeqj.de/sneak/secret/issues/147). It added up each version's
+  `data.age` and `metadata.age`, but a version keeps its value in `value.age`,
+  so no secret's value was counted. A version directory that cannot be read adds
+  nothing and gets a warning naming it.
+- 2026-10-15: `secret vault import` is gone, with its tests and its README entry
+  (https://git.eeqj.de/sneak/secret/issues/139). It took only a vault without a
+  long-term key, and `secret init` and `secret vault create` give every vault
+  one, so it always failed; a mnemonic enters through those two commands. The
+  help and the note of `secret generate mnemonic` named `secret import`, which
+  imports a file as a secret, and now name `secret init` and
+  `secret vault create`.
+- 2026-10-15: `secret version promote --help` says promote rewrites the
+  `current` file, not a "current symlink"
+  (https://git.eeqj.de/sneak/secret/issues/153). `current` is a plain file
+  holding the version name, as the README already said, and the comment in
+  `PromoteVersion` now says so too.
+- 2026-10-14: `secret init` and `secret vault create` refuse a vault that
+  already exists before they ask for the mnemonic or the unlocker passphrase
+  (https://git.eeqj.de/sneak/secret/issues/151). They asked first, and
+  `secret init` printed "Initialized secrets manager at" before refusing. The
+  check `vault.CreateVault` made before writing is now
+  `vault.CheckNewVaultName`, which both commands call before the first prompt
+  and `CreateVault` still calls. It validates the name too, so
+  `secret vault create` with an invalid name also fails before the prompts.
+  `secret init` prints the "Initialized" line only once the vault is created.
+- 2026-10-14: `secret get` fails when it cannot write the value
+  (https://git.eeqj.de/sneak/secret/issues/155). `GetSecret` and
+  `GetSecretWithVersion` discarded the result of writing it to stdout, so a full
+  disk or a failing stdout left an empty or cut-short file and exit status 0.
+  The write error is now returned, so the command prints it and exits 1. A
+  stdout the shell closed before `secret` started still exits 0, because the Go
+  runtime opens `/dev/null` in its place and the write there succeeds.
+- 2026-10-14: `secret list` and `secret list --json` give each secret's own
+  creation and last-update times (https://git.eeqj.de/sneak/secret/issues/142).
+  They gave the time of the listing for every secret, because
+  `Secret.LoadMetadata` set both to the current time. It now reads file
+  modification times, so listing still needs no passphrase. The last update is
+  when the secret's `current` file was written. The creation is when its oldest
+  version's `value.age` was written, not that version's directory, whose time
+  changes when the next add rewrites its `metadata.age`. A move to another vault
+  gives the copied files the times of the originals. `GetSecretObject` no longer
+  loads the times, so `secret get` keeps its error for a missing `current` file.
+- 2026-10-14: `secret unlocker add pgp` without `--keyid` uses the `default-key`
+  in `gpg.conf` (https://git.eeqj.de/sneak/secret/issues/141).
+  `getDefaultGPGKey` took that value from `gpgconf --list-options gpg` as
+  printed, with a leading `"` and percent-escaped, so gpg rejected the key ID.
+  The new `parseDefaultGPGKey` drops the `"` and percent-decodes the rest with
+  `url.PathUnescape`, which leaves `+` alone. A value that does not decode is an
+  error, not a fallback to the first secret key.
 - 2026-10-14: A wrong passphrase, from `SB_UNLOCK_PASSPHRASE` or typed at the
   prompt, gives the error `wrong passphrase for unlocker <id>`
   (https://git.eeqj.de/sneak/secret/issues/157). It used to be age's "no

cmd/secret/main.go

@@ -3,10 +3,16 @@ package main
 
 import (
 	"os"
+	"time"
 
 	"sneak.berlin/go/secret/internal/cli"
 )
 
 func main() {
+	// Every time secret prints or writes is in UTC, whatever TZ or the
+	// system's time zone is set to. It is set here, before cli.Entry starts
+	// any goroutine that could read time.Local while it changes.
+	time.Local = time.UTC //nolint:gosmopolitan // this makes local time UTC
+
 	os.Exit(cli.Entry())
 }

internal/cli/create_vault_test.go

@@ -144,25 +144,20 @@ func TestVaultCreationLeavesNoSecretInEnvironment(t *testing.T) {
 // TestNoVaultImportCommand is a regression test for
 // https://git.eeqj.de/sneak/secret/issues/139: `secret vault import`
 // refused every vault that `secret vault create` makes, because each has a
-// long-term key. The command is gone, so cobra answers it with the help of
-// `secret vault`, as it does any subcommand of `secret vault` it does not
-// know.
+// long-term key. The command is gone, so it fails as any subcommand of
+// `secret vault` that does not exist does.
 func TestNoVaultImportCommand(t *testing.T) {
 	t.Setenv(secret.EnvStateDir, t.TempDir())
+	t.Setenv(secret.EnvMnemonic, testMnemonic)
+	t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
 
-	run := func(args ...string) string {
-		t.Setenv(secret.EnvMnemonic, testMnemonic)
-		t.Setenv(secret.EnvUnlockPassphrase, testPassphrase)
+	_, err := cli.ExecuteCommandInProcess(
+		strings.Fields("vault create work"), "", nil)
+	require.NoError(t, err)
 
-		output, err := cli.ExecuteCommandInProcess(args, "", nil)
-		require.NoError(t, err, "%v", args)
-
-		return output
-	}
-
-	run("vault", "create", "work")
-
-	require.Equal(t, run("vault"), run("vault", "import", "work"))
+	_, err = cli.ExecuteCommandInProcess(
+		strings.Fields("vault import work"), "", nil)
+	require.EqualError(t, err, `unknown command "import" for "secret vault"`)
 }
 
 // TestStopAtPassphrasePromptLeavesNothing is a regression test for the

internal/cli/crypto.go

@@ -1,6 +1,7 @@
 package cli
 
 import (
+	"bytes"
 	"errors"
 	"fmt"
 	"io"
@@ -107,8 +108,8 @@ func (cli *Instance) storeNewEncryptionKey(
 // 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.
+// at the passphrase prompt, and Encrypt reads its input and writes its
+// output without the lock.
 func (cli *Instance) resolveEncryptionKey(
 	vlt *vault.Vault, secretName string,
 ) (*memguard.LockedBuffer, error) {
@@ -188,17 +189,17 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
 		input = file
 	}
 
-	// Set up output writer
+	// Set up output writer. With --output the result is held in memory
+	// and written only once encryption has succeeded, so a failure to
+	// encrypt leaves --output untouched and --input can be the same file.
+	// A failure while writing --output, such as a full disk, can leave it
+	// empty or partly written.
 	output := cli.cmd.OutOrStdout()
 
-	if outputFile != "" {
-		file, err := cli.fs.Create(outputFile)
-		if err != nil {
-			return fmt.Errorf("failed to create output file: %w", err)
-		}
-		defer func() { _ = file.Close() }()
+	var result bytes.Buffer
 
-		output = file
+	if outputFile != "" {
+		output = &result
 	}
 
 	// Encrypt the data
@@ -217,7 +218,11 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
 		return fmt.Errorf("failed to finalize encryption: %w", err)
 	}
 
-	return nil
+	if outputFile == "" {
+		return nil
+	}
+
+	return cli.writeOutputFile(outputFile, result.Bytes())
 }
 
 // Decrypt decrypts data using an age secret key stored in a secret
@@ -278,17 +283,17 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
 		input = file
 	}
 
-	// Set up output writer
+	// Set up output writer. With --output the result is held in memory
+	// and written only once decryption has succeeded, so a failure to
+	// decrypt leaves --output untouched and --input can be the same file.
+	// A failure while writing --output, such as a full disk, can leave it
+	// empty or partly written.
 	output := cli.cmd.OutOrStdout()
 
-	if outputFile != "" {
-		file, err := cli.fs.Create(outputFile)
-		if err != nil {
-			return fmt.Errorf("failed to create output file: %w", err)
-		}
-		defer func() { _ = file.Close() }()
+	var result bytes.Buffer
 
-		output = file
+	if outputFile != "" {
+		output = &result
 	}
 
 	// Decrypt the data
@@ -302,6 +307,32 @@ func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
 		return fmt.Errorf("failed to decrypt data: %w", err)
 	}
 
+	if outputFile == "" {
+		return nil
+	}
+
+	return cli.writeOutputFile(outputFile, result.Bytes())
+}
+
+// writeOutputFile creates outputFile, or empties it if it exists, and
+// writes result into it.
+func (cli *Instance) writeOutputFile(outputFile string, result []byte) error {
+	file, err := cli.fs.Create(outputFile)
+	if err != nil {
+		return fmt.Errorf("failed to create output file: %w", err)
+	}
+
+	_, err = file.Write(result)
+
+	closeErr := file.Close()
+	if err == nil {
+		err = closeErr
+	}
+
+	if err != nil {
+		return fmt.Errorf("failed to write output file: %w", err)
+	}
+
 	return nil
 }
 

internal/cli/crypto_test.go

@@ -0,0 +1,185 @@
+//nolint:testpackage // sets the unexported fields of Instance
+package cli
+
+import (
+	"errors"
+	"fmt"
+	"io"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/vault"
+)
+
+// plainText is the content of plain.txt in the directory newCryptoTest makes.
+const plainText = "document"
+
+// newCryptoTest returns an Instance whose current vault, in a new state
+// directory on the real filesystem, holds the age key secret "key", and a
+// new directory holding only plain.txt and plain.txt.age, its encryption
+// with that key.
+func newCryptoTest(t *testing.T) (*Instance, string) {
+	t.Helper()
+
+	fs := afero.NewOsFs()
+	stateDir := t.TempDir()
+
+	_, err := vault.CreateVault(fs, stateDir, "default", testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	cli := NewCLIInstanceWithStateDir(fs, stateDir)
+	cli.Mnemonic = testMnemonicBuffer(t)
+	cli.cmd = &cobra.Command{}
+
+	dir := t.TempDir()
+	plain := filepath.Join(dir, "plain.txt")
+	require.NoError(t, afero.WriteFile(fs, plain, []byte(plainText), 0o600))
+	require.NoError(t, cli.Encrypt("key", plain, plain+".age"))
+
+	return cli, dir
+}
+
+// TestFailedCryptoLeavesOutputUnchanged is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/143, where secret encrypt and
+// secret decrypt emptied the file named by --output before they failed.
+// A failure to encrypt or decrypt must leave an existing file as it was and
+// create none, and give the same error as without --output.
+func TestFailedCryptoLeavesOutputUnchanged(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name string
+		run  func(cli *Instance, dir, output string) error
+	}{
+		// Reading a directory fails once encryption has begun
+		{"encrypt", func(cli *Instance, dir, output string) error {
+			return cli.Encrypt("key", dir, output)
+		}},
+		// plain.txt is not an age file
+		{"decrypt", func(cli *Instance, dir, output string) error {
+			return cli.Decrypt("key", filepath.Join(dir, "plain.txt"), output)
+		}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			cli, dir := newCryptoTest(t)
+			existing := filepath.Join(dir, "existing.txt")
+			require.NoError(t, afero.WriteFile(cli.fs, existing, []byte("precious"), 0o600))
+
+			cli.cmd.SetOut(io.Discard)
+			toStdout := tt.run(cli, dir, "")
+			require.Error(t, toStdout)
+
+			require.EqualError(t, tt.run(cli, dir, existing), toStdout.Error())
+			require.EqualError(t, tt.run(cli, dir, filepath.Join(dir, "new.txt")),
+				toStdout.Error())
+
+			content, err := afero.ReadFile(cli.fs, existing)
+			require.NoError(t, err)
+			require.Equal(t, "precious", string(content))
+
+			exists, err := afero.Exists(cli.fs, filepath.Join(dir, "new.txt"))
+			require.NoError(t, err)
+			require.False(t, exists, "a failed %s created --output", tt.name)
+		})
+	}
+}
+
+// TestCryptoInPlace checks that secret encrypt and secret decrypt given
+// one file as both --input and --output replace it with the right result.
+// Before https://git.eeqj.de/sneak/secret/issues/143 they emptied the file
+// before reading it.
+func TestCryptoInPlace(t *testing.T) {
+	t.Parallel()
+
+	cli, dir := newCryptoTest(t)
+	file := filepath.Join(dir, "plain.txt")
+
+	require.NoError(t, cli.Encrypt("key", file, file))
+	require.NoError(t, cli.Decrypt("key", file, file))
+
+	content, err := afero.ReadFile(cli.fs, file)
+	require.NoError(t, err)
+	require.Equal(t, plainText, string(content))
+}
+
+// errCloseFailed is returned by the files closeFailFs creates in place of
+// a successful Close, as NFS reports a full disk only when the file is
+// closed.
+var errCloseFailed = errors.New("no space left on device")
+
+// closeFailFs creates files whose Close fails with errCloseFailed.
+type closeFailFs struct {
+	afero.Fs
+}
+
+// closeFailFile is a file created through closeFailFs.
+type closeFailFile struct {
+	afero.File
+}
+
+func (f closeFailFile) Close() error {
+	_ = f.File.Close()
+
+	return errCloseFailed
+}
+
+//nolint:ireturn // afero.File is the interface required by afero.Fs
+func (f closeFailFs) Create(name string) (afero.File, error) {
+	file, err := f.Fs.Create(name)
+	if err != nil {
+		return nil, err
+	}
+
+	return closeFailFile{File: file}, nil
+}
+
+// TestDecryptFailsWhenClosingOutputFails checks that secret decrypt fails
+// when closing --output fails, since some filesystems report a failed
+// write only then.
+func TestDecryptFailsWhenClosingOutputFails(t *testing.T) {
+	t.Parallel()
+
+	cli, dir := newCryptoTest(t)
+	cli.fs = closeFailFs{Fs: cli.fs}
+
+	err := cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"),
+		filepath.Join(dir, "out.txt"))
+	require.ErrorIs(t, err, errCloseFailed)
+}
+
+// TestDecryptToDescriptorOfDeletedFile checks that secret decrypt given
+// /dev/fd/N as --output writes through that descriptor. A program may give
+// the command a temporary file it has already deleted as stdout, pass
+// /dev/stdout as --output and read the result back from that file.
+func TestDecryptToDescriptorOfDeletedFile(t *testing.T) {
+	t.Parallel()
+
+	cli, dir := newCryptoTest(t)
+
+	file, err := os.CreateTemp(t.TempDir(), "out")
+	require.NoError(t, err)
+
+	defer func() { _ = file.Close() }()
+
+	require.NoError(t, os.Remove(file.Name()))
+
+	output := fmt.Sprintf("/dev/fd/%d", file.Fd())
+	require.NoError(t, cli.Decrypt("key", filepath.Join(dir, "plain.txt.age"), output))
+
+	// On macOS the command's open of /dev/fd/N shares this descriptor's
+	// position, which its write leaves at the end.
+	_, err = file.Seek(0, io.SeekStart)
+	require.NoError(t, err)
+
+	content, err := io.ReadAll(file)
+	require.NoError(t, err)
+	require.Equal(t, plainText, string(content))
+}

internal/cli/current_vault_test.go

@@ -0,0 +1,60 @@
+package cli_test
+
+import (
+	"bytes"
+	"path/filepath"
+	"strconv"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/cli"
+	"sneak.berlin/go/secret/internal/secret"
+	"sneak.berlin/go/secret/internal/vault"
+)
+
+// TestListFailsWhenCurrentVaultDoesNotExist is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/156, where a currentvault file
+// that was empty, held an invalid vault name, or named a vault that does
+// not exist, made `secret list` report an empty vault. Listing must fail
+// with an error that names the file and says to run `secret vault select`,
+// `secret vault list` must still list the vaults, and selecting a vault
+// must make listing work again. The name "." would make vaults.d itself the
+// vault, and "../vaults.d/work" would reach the vault work by a path.
+func TestListFailsWhenCurrentVaultDoesNotExist(t *testing.T) {
+	t.Parallel()
+
+	for _, name := range []string{"", ".", "../vaults.d/work", "gone"} {
+		t.Run(strconv.Quote(name), func(t *testing.T) {
+			t.Parallel()
+
+			fs := newTwoVaultFs(t)
+			currentVaultPath := filepath.Join(testStateDir, "currentvault")
+			require.NoError(t, afero.WriteFile(fs, currentVaultPath,
+				[]byte(name), secret.FilePerms))
+
+			c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
+
+			var out bytes.Buffer
+
+			cmd := &cobra.Command{}
+			cmd.SetOut(&out)
+
+			err := c.ListSecrets(cmd, false, true, "")
+			require.ErrorIs(t, err, vault.ErrVaultNotFound)
+			require.ErrorContains(t, err, currentVaultPath)
+			require.ErrorContains(t, err, "secret vault select")
+			assert.Empty(t, out.String())
+
+			require.NoError(t, c.ListVaults(cmd, false))
+			assert.Equal(t, "Available vaults:\n  default\n  work\n", out.String())
+
+			require.NoError(t, c.SelectVault(cmd, "default"))
+			out.Reset()
+			require.NoError(t, c.ListSecrets(cmd, false, true, ""))
+			assert.Equal(t, "x\n", out.String())
+		})
+	}
+}

internal/cli/entry_test.go

@@ -3,6 +3,7 @@ package cli_test
 import (
 	"bufio"
 	"context"
+	"fmt"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -16,29 +17,67 @@ import (
 	"sneak.berlin/go/secret/internal/secret"
 )
 
-// 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
+const (
+	// envRunEntry, when set, makes this test binary run cli.Entry with its
+	// own arguments and exit, instead of running the tests.
+	envRunEntry = "SECRET_TEST_RUN_ENTRY"
 
-	t.Cleanup(func() { os.Args = savedArgs })
+	// bufferWiped is what the test binary prints, run with envRunEntry set,
+	// when cli.Entry wiped the buffer made before it ran.
+	bufferWiped = "Entry wiped the buffer made before it ran"
+)
+
+// With envRunEntry set, the test binary makes a buffer, runs cli.Entry,
+// says whether the buffer is wiped, and exits with Entry's exit code. This
+// is done in init, not in a test, so that TestMain does not run in that
+// process: it would build the secret binary again, and remove it at the end,
+// while the other tests run it.
+//
+//nolint:gochecknoinits // must run before TestMain, as above
+func init() {
+	if os.Getenv(envRunEntry) == "" {
+		return
+	}
+
+	buf := memguard.NewBufferFromBytes([]byte("key material"))
+	code := cli.Entry()
+
+	if !buf.IsAlive() {
+		fmt.Fprintln(os.Stderr, bufferWiped)
+	}
+
+	os.Exit(code)
+}
+
+// Entry must return its exit code rather than exit, so that its deferred
+// memguard purge runs on the success and the error path alike. Entry wipes
+// every memguard buffer in the process, including those other tests are
+// using, so it runs in a process of its own: this test binary, run again
+// with envRunEntry set.
+func TestEntryWipesBuffersAndReturnsExitCode(t *testing.T) {
+	t.Parallel()
 
 	tests := []struct {
 		args     []string
 		exitCode int
 	}{
-		{args: []string{"secret", "--help"}, exitCode: 0},
-		{args: []string{"secret", "no-such-command"}, exitCode: 1},
+		{args: []string{"--help"}, exitCode: 0},
+		{args: []string{"no-such-command"}, exitCode: 1},
 	}
 
 	for _, tt := range tests {
-		buf := memguard.NewBufferFromBytes([]byte("key material"))
-		os.Args = tt.args
+		//nolint:gosec // G204: test runs its own test binary
+		cmd := exec.CommandContext(t.Context(), os.Args[0], 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)
+		cmd.Env = append(os.Environ(), envRunEntry+"=1")
+
+		output, err := cmd.CombinedOutput()
+		require.NotNil(t, cmd.ProcessState, "%v did not run: %v", tt.args, err)
+
+		assert.Equal(t, tt.exitCode, cmd.ProcessState.ExitCode(),
+			"exit code for %v: %s", tt.args, output)
+		assert.Contains(t, string(output), bufferWiped,
+			"Entry left a buffer unwiped for %v", tt.args)
 	}
 }
 

internal/cli/generate.go

@@ -29,6 +29,12 @@ func newGenerateCmd() *cobra.Command {
 		Use:   "generate",
 		Short: "Generate random data",
 		Long:  `Generate various types of random data including mnemonics and secrets.`,
+		// Cobra checks Args only on a command that has a RunE; without
+		// one, an unknown subcommand prints this help and exits 0.
+		Args: cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return cmd.Help()
+		},
 	}
 
 	cmd.AddCommand(newGenerateMnemonicCmd())
@@ -169,12 +175,6 @@ 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 {
@@ -187,7 +187,7 @@ func (cli *Instance) GenerateSecret(
 	secretBuffer := memguard.NewBufferFromBytes([]byte(secretValue))
 	defer secretBuffer.Destroy()
 
-	err = vlt.AddSecret(secretName, secretBuffer, force)
+	err = cli.storeSecret(vlt, secretName, secretBuffer, force)
 	if err != nil {
 		return err
 	}

internal/cli/init.go

@@ -67,36 +67,15 @@ func (cli *Instance) promptMnemonic() (*memguard.LockedBuffer, func(), error) {
 	return mnemonicBuffer, mnemonicBuffer.Destroy, nil
 }
 
-// Init initializes the secret manager, holding the state directory lock
-// while initialize runs
+// Init initializes the secret manager. It asks for the mnemonic and the
+// passphrase before it takes the state directory lock, so that no other
+// command waits while the user answers, and holds the 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
-	stateDir := cli.GetStateDir()
-	secret.DebugWith("Creating state directory", slog.String("path", stateDir))
-
-	err := cli.fs.MkdirAll(stateDir, secret.DirPerms)
-	if err != nil {
-		secret.Debug("Failed to create state directory", "error", err)
-
-		return fmt.Errorf("failed to create state directory: %w", err)
-	}
-
 	// Refuse an existing default vault before asking for anything
-	err = vault.CheckNewVaultName(cli.fs, cli.stateDir, defaultVaultName)
+	err := vault.CheckNewVaultName(cli.fs, cli.stateDir, defaultVaultName)
 	if err != nil {
 		return fmt.Errorf("failed to create default vault: %w", err)
 	}
@@ -135,11 +114,38 @@ func (cli *Instance) initialize(cmd *cobra.Command) error {
 	}
 	defer cleanupPassphrase()
 
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	return cli.initialize(cmd, mnemonic, passphraseBuffer)
+}
+
+// initialize creates the state directory, and the default vault from
+// mnemonic with a passphrase unlocker protected by passphrase. Another
+// command may have created the vault since Init checked: vault.CreateVault
+// checks again.
+func (cli *Instance) initialize(
+	cmd *cobra.Command, mnemonic, passphrase *memguard.LockedBuffer,
+) error {
+	// Create state directory
+	stateDir := cli.GetStateDir()
+	secret.DebugWith("Creating state directory", slog.String("path", stateDir))
+
+	err := cli.fs.MkdirAll(stateDir, secret.DirPerms)
+	if err != nil {
+		secret.Debug("Failed to create state directory", "error", err)
+
+		return fmt.Errorf("failed to create state directory: %w", err)
+	}
+
 	// Create the default vault with its passphrase unlocker
 	secret.Debug("Creating default vault")
 
 	vlt, err := vault.CreateVault(cli.fs, cli.stateDir, defaultVaultName,
-		mnemonic, passphraseBuffer)
+		mnemonic, passphrase)
 	if err != nil {
 		secret.Debug("Failed to create default vault", "error", err)
 

internal/cli/list_times_test.go

@@ -3,6 +3,9 @@ package cli_test
 import (
 	"bytes"
 	"encoding/json"
+	"os"
+	"os/exec"
+	"path/filepath"
 	"testing"
 	"time"
 
@@ -125,6 +128,57 @@ func TestMovedSecretKeepsItsTimes(t *testing.T) {
 		"updated %s, then %s", beforeMove.UpdatedAt, afterMove.UpdatedAt)
 }
 
+// TestListedTimesAreUTC is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/169, where times were printed
+// in the local time zone. It runs `secret list` on the built binary with
+// TZ set to New York, where the secret's last update, 03:04:05 UTC on 2
+// January, is 22:04:05 the day before.
+func TestListedTimesAreUTC(t *testing.T) {
+	t.Parallel()
+
+	stateDir := t.TempDir()
+
+	vlt, err := vault.CreateVault(
+		afero.NewOsFs(), stateDir, "default", testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	vlt.Mnemonic = testMnemonicBuffer(t)
+	err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("one")), false)
+	require.NoError(t, err)
+
+	updated := time.Date(2026, time.January, 2, 3, 4, 5, 0, time.UTC)
+	current := filepath.Join(stateDir, "vaults.d", "default", "secrets.d", "x",
+		"current")
+	require.NoError(t, os.Chtimes(current, updated, updated))
+
+	tests := []struct {
+		flags []string
+		want  string
+	}{
+		{flags: nil, want: "2026-01-02 03:04"},
+		{
+			flags: []string{"--json"},
+			want:  `"updated_at": "2026-01-02T03:04:05Z"`,
+		},
+	}
+
+	for _, tt := range tests {
+		args := append([]string{"list"}, tt.flags...)
+
+		//nolint:gosec // G204: test executes the freshly built secret binary
+		cmd := exec.CommandContext(t.Context(), secretBinaryPath(t), args...)
+		cmd.Env = []string{
+			secret.EnvStateDir + "=" + stateDir,
+			"TZ=America/New_York",
+			"PATH=" + os.Getenv("PATH"),
+		}
+
+		output, err := cmd.CombinedOutput()
+		require.NoError(t, err, "%v: %s", args, output)
+		assert.Contains(t, string(output), tt.want, "%v", args)
+	}
+}
+
 // listSecrets returns the output of `secret list`, or of
 // `secret list --json` when jsonOutput is set.
 func listSecrets(t *testing.T, c *cli.Instance, jsonOutput bool) string {

internal/cli/lock_test.go

@@ -280,9 +280,9 @@ func stateDirModTimes(t *testing.T, fs afero.Fs) map[string]int64 {
 	return modTimes
 }
 
-// setupEveryCommand makes what each command in
-// TestChangingCommandsWaitForLock needs: the current vault "work" with two
-// versions of "test/secret", the vault "other", and the file testInput.
+// setupEveryCommand makes what the commands that the lock tests below run
+// need: the current vault "work" with two versions of "test/secret", the
+// vault "other", and the file testInput.
 // There is no vault "default", which init creates. If withUnlocker is set,
 // it also gives "work" a passphrase unlocker, which is slow. It returns the
 // older version and the unlocker's ID.
@@ -346,6 +346,25 @@ func waitingForLock() bool {
 	return false
 }
 
+// requireWaitingForLock returns once a command waits for the state
+// directory lock. It fails the test if the command finishes first, sending
+// its error on done, or never waits.
+func requireWaitingForLock(t *testing.T, done <-chan error) {
+	t.Helper()
+
+	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):
+		}
+	}
+}
+
 // 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
@@ -385,17 +404,7 @@ func requireWaitsForLock(
 
 	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):
-		}
-	}
+	requireWaitingForLock(t, done)
 
 	assert.Equal(t, before, stateDirModTimes(t, fs),
 		"changed the state directory before waiting for the lock")
@@ -472,6 +481,311 @@ func TestChangingCommandsWaitForLock(t *testing.T) {
 	}
 }
 
+// TestCommandsAskBeforeLocking is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/152: these commands took the
+// state directory lock before asking for the mnemonic or a passphrase, so
+// while one waited at its prompt, every other command that changes anything
+// waited for it. Each runs here while the test holds the lock, given what
+// takes it to the last step at which it can ask: the passphrase prompt of
+// init and vault create, and the unlock of the vault, which asks for the
+// passphrase of its passphrase unlocker, for the others. The tests have no
+// terminal, so a prompt fails at once. Each command must get there, and
+// fail, without waiting for the lock.
+//
+//nolint:paralleltest // waitingForLock sees any test's command waiting for the lock
+func TestCommandsAskBeforeLocking(t *testing.T) {
+	newTestGPGKey(t)
+
+	mnemonic := testMnemonicBuffer(t)
+
+	wrongPassphrase := memguard.NewBufferFromBytes([]byte("wrong-passphrase"))
+	t.Cleanup(wrongPassphrase.Destroy)
+
+	for _, tc := range []struct {
+		name string
+		run  func(cli *Instance) error
+		// want is wrapped in the error the command fails with
+		want error
+	}{
+		{"init", func(cli *Instance) error {
+			cli.Mnemonic = mnemonic
+
+			return cli.Init(cli.cmd)
+		}, secret.ErrNoTerminal},
+		{"vault create", func(cli *Instance) error {
+			cli.Mnemonic = mnemonic
+
+			return cli.CreateVault(cli.cmd, "created")
+		}, secret.ErrNoTerminal},
+		// With no terminal to ask for the new passphrase on, the new
+		// unlocker takes the one in SB_UNLOCK_PASSPHRASE, which then fails
+		// to unlock the vault
+		{"unlocker add passphrase", func(cli *Instance) error {
+			cli.UnlockPassphrase = wrongPassphrase
+
+			return cli.UnlockersAdd("passphrase", cli.cmd)
+		}, secret.ErrWrongPassphrase},
+		{"unlocker add pgp", func(cli *Instance) error {
+			cli.cmd.Flags().String("keyid", unreadableTestGPGUserID, "")
+
+			return cli.UnlockersAdd("pgp", cli.cmd)
+		}, secret.ErrNoTerminal},
+		{"add --force", func(cli *Instance) error {
+			return cli.AddSecret("test/secret", true)
+		}, secret.ErrNoTerminal},
+		{"import --force", func(cli *Instance) error {
+			return cli.ImportSecret(cli.cmd, "test/secret", testInput, true)
+		}, secret.ErrNoTerminal},
+		{"generate secret --force", func(cli *Instance) error {
+			return cli.GenerateSecret(cli.cmd, "test/secret", 16, "base58", true)
+		}, secret.ErrNoTerminal},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			requireAsksBeforeLocking(t, tc.run, tc.want)
+		})
+	}
+}
+
+// startWhileLocked makes what setupEveryCommand makes, with the passphrase
+// unlocker, on fs, an empty in-memory filesystem, takes the state directory
+// lock there, and starts run, given a CLI instance on fs with "value" on
+// stdin. It returns the function that releases the lock, which does so only
+// the first time it is called, and the channel that run's error comes on.
+// The lock is also released when the test ends, so that later tests can
+// take it.
+func startWhileLocked(
+	t *testing.T, fs afero.Fs, run func(cli *Instance) error,
+) (func(), <-chan error) {
+	t.Helper()
+
+	setupEveryCommand(t, fs, true)
+
+	release, err := vault.LockStateDir(fs, testStateDir)
+	require.NoError(t, err)
+
+	release = sync.OnceFunc(release)
+	t.Cleanup(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) }()
+
+	return release, done
+}
+
+// requireAsksBeforeLocking starts run while holding the state directory
+// lock, as startWhileLocked does. run must fail, with an error wrapping
+// want, without waiting for the lock.
+func requireAsksBeforeLocking(
+	t *testing.T, run func(cli *Instance) error, want error,
+) {
+	t.Helper()
+
+	release, done := startWhileLocked(t, afero.NewMemMapFs(), run)
+
+	timeout := time.After(lockWait)
+
+	for !waitingForLock() {
+		select {
+		case err := <-done:
+			require.ErrorIs(t, err, want)
+
+			return
+		case <-timeout:
+			t.Fatal("neither failed nor waited for the lock")
+		case <-time.After(time.Millisecond):
+		}
+	}
+
+	// Let the command go on, so that it releases the lock
+	release()
+
+	select {
+	case <-done:
+	case <-time.After(lockWait):
+	}
+
+	t.Fatal("waited for the lock before asking")
+}
+
+// TestCommandsCheckAgainOnceLocked checks that commands which check what
+// they change before they ask for anything, and so before they take the
+// state directory lock, check again once they hold it: another command may
+// have changed it in the meantime. Each runs while the test holds the lock,
+// given what it needs to ask for nothing. Once it waits for the lock, the
+// test makes the change another command could have made, then releases the
+// lock. The command must fail, and change nothing.
+//
+//nolint:paralleltest // waitingForLock sees any test's command waiting for the lock
+func TestCommandsCheckAgainOnceLocked(t *testing.T) {
+	fingerprint := newTestGPGKey(t)
+
+	mnemonic := testMnemonicBuffer(t)
+
+	otherMnemonic := memguard.NewBufferFromBytes([]byte("legal winner thank " +
+		"year wave sausage worth useful legal winner thank yellow"))
+	t.Cleanup(otherMnemonic.Destroy)
+
+	passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
+	t.Cleanup(passphrase.Destroy)
+
+	// work returns the vault "work" on fs, opened with the mnemonic
+	work := func(fs afero.Fs) *vault.Vault {
+		vlt := vault.NewVault(fs, testStateDir, "work")
+		vlt.Mnemonic = mnemonic
+
+		return vlt
+	}
+
+	for _, tc := range []struct {
+		name string
+		run  func(cli *Instance) error
+		// change is what another command changes while run waits
+		change func(fs afero.Fs) error
+		// want is wrapped in the error the command fails with
+		want error
+	}{
+		{
+			"unlocker add, vault removed",
+			func(cli *Instance) error {
+				cli.Mnemonic, cli.UnlockPassphrase = mnemonic, passphrase
+
+				return cli.UnlockersAdd("passphrase", cli.cmd)
+			},
+			func(fs afero.Fs) error {
+				return fs.RemoveAll(testStateDir + "/vaults.d/work")
+			},
+			vault.ErrVaultNotFound,
+		},
+		{
+			"unlocker add, vault created again from another mnemonic",
+			func(cli *Instance) error {
+				cli.Mnemonic, cli.UnlockPassphrase = mnemonic, passphrase
+
+				return cli.UnlockersAdd("passphrase", cli.cmd)
+			},
+			func(fs afero.Fs) error {
+				err := fs.RemoveAll(testStateDir + "/vaults.d/work")
+				if err != nil {
+					return err
+				}
+
+				_, err = vault.CreateVault(fs, testStateDir, "work",
+					otherMnemonic, passphrase)
+
+				return err
+			},
+			errVaultCreatedAgain,
+		},
+		{
+			"unlocker add pgp, key added",
+			func(cli *Instance) error {
+				cli.Mnemonic = mnemonic
+				cli.cmd.Flags().String("keyid", unreadableTestGPGUserID, "")
+
+				return cli.UnlockersAdd("pgp", cli.cmd)
+			},
+			func(fs afero.Fs) error {
+				_, err := secret.CreatePGPUnlocker(work(fs),
+					unreadableTestGPGUserID, fingerprint)
+
+				return err
+			},
+			errGPGKeyAlreadyUnlocker,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			requireChecksAgainOnceLocked(t, tc.run, tc.change, tc.want)
+		})
+	}
+}
+
+// requireChecksAgainOnceLocked starts run while holding the state directory
+// lock, as startWhileLocked does. Once run waits for the lock, change makes
+// the change that another command could have made in the meantime, and the
+// lock is released. run must then fail, with an error wrapping want, and
+// change nothing.
+func requireChecksAgainOnceLocked(
+	t *testing.T, run func(cli *Instance) error,
+	change func(fs afero.Fs) error, want error,
+) {
+	t.Helper()
+
+	fs := afero.NewMemMapFs()
+	release, done := startWhileLocked(t, fs, run)
+
+	requireWaitingForLock(t, done)
+	require.NoError(t, change(fs))
+
+	// Left out: taking and releasing the lock writes the lock file
+	lockFile := testStateDir + "/lock"
+
+	before := stateDirModTimes(t, fs)
+	delete(before, lockFile)
+
+	release()
+
+	select {
+	case err := <-done:
+		require.ErrorIs(t, err, want)
+	case <-time.After(lockWait):
+		t.Fatal("did not finish once the lock was released")
+	}
+
+	after := stateDirModTimes(t, fs)
+	delete(after, lockFile)
+
+	assert.Equal(t, before, after, "changed the state directory")
+}
+
+// TestForcedAddReplacesSecretAddedMeanwhile runs a forced add of a secret
+// that does not exist yet while the test holds the state directory lock.
+// Once it waits for the lock, the test adds the secret, as another command
+// could have, and releases the lock. The forced add must replace that
+// secret, as if it had run after the other command.
+//
+//nolint:paralleltest // waitingForLock sees any test's command waiting for the lock
+func TestForcedAddReplacesSecretAddedMeanwhile(t *testing.T) {
+	mnemonic := testMnemonicBuffer(t)
+
+	fs := afero.NewMemMapFs()
+	release, done := startWhileLocked(t, fs, func(cli *Instance) error {
+		cli.Mnemonic = mnemonic
+
+		return cli.AddSecret("added", true)
+	})
+
+	requireWaitingForLock(t, done)
+
+	work := vault.NewVault(fs, testStateDir, "work")
+	work.Mnemonic = mnemonic
+
+	other := memguard.NewBufferFromBytes([]byte("other"))
+	t.Cleanup(other.Destroy)
+
+	require.NoError(t, work.AddSecret("added", other, false))
+
+	release()
+
+	select {
+	case err := <-done:
+		require.NoError(t, err)
+	case <-time.After(lockWait):
+		t.Fatal("did not finish once the lock was released")
+	}
+
+	value, err := work.GetSecret("added")
+	require.NoError(t, err)
+	t.Cleanup(value.Destroy)
+
+	assert.Equal(t, "value", string(value.Bytes()))
+}
+
 // 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.

internal/cli/move_cross_vault_test.go

@@ -0,0 +1,315 @@
+//nolint:testpackage // white-box test of unexported internals
+package cli
+
+import (
+	"errors"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/awnumar/memguard"
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/secret"
+	"sneak.berlin/go/secret/internal/vault"
+)
+
+// errPermissionDenied is returned by renameFailFs and removeAllFailFs in
+// place of a rename or removal that succeeds.
+var errPermissionDenied = errors.New("permission denied")
+
+// renameFailFs fails every Rename of one path, as a read-only parent
+// directory does.
+type renameFailFs struct {
+	afero.Fs
+
+	path string
+}
+
+func (f *renameFailFs) Rename(oldname, newname string) error {
+	if oldname == f.path {
+		return errPermissionDenied
+	}
+
+	return f.Fs.Rename(oldname, newname)
+}
+
+// removeAllFailFs fails every RemoveAll of an entry of the directory dir,
+// as a file in it that cannot be deleted does, and records that entry's
+// path in failed.
+type removeAllFailFs struct {
+	afero.Fs
+
+	dir    string
+	failed string
+}
+
+func (f *removeAllFailFs) RemoveAll(path string) error {
+	if filepath.Dir(path) == f.dir {
+		f.failed = path
+
+		return errPermissionDenied
+	}
+
+	return f.Fs.RemoveAll(path)
+}
+
+// createCrossVaults creates in fs the vaults "work" and "default", where
+// "default" holds the secret "x" with the value "value".
+func createCrossVaults(t *testing.T, fs afero.Fs) {
+	t.Helper()
+
+	_, err := vault.CreateVault(fs, testStateDir, "work",
+		testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	source, err := vault.CreateVault(fs, testStateDir, "default",
+		testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	err = source.AddSecret("x",
+		memguard.NewBufferFromBytes([]byte("value")), false)
+	require.NoError(t, err)
+}
+
+// requireValue requires that the vault vaultName in fs holds secretName
+// with the value createCrossVaults stored.
+func requireValue(t *testing.T, fs afero.Fs, vaultName, secretName string) {
+	t.Helper()
+
+	vlt := vault.NewVault(fs, testStateDir, vaultName)
+	vlt.Mnemonic = testMnemonicBuffer(t)
+
+	value, err := vlt.GetSecret(secretName)
+	require.NoError(t, err, vaultName+":"+secretName)
+
+	defer value.Destroy()
+
+	require.Equal(t, []byte("value"), value.Bytes())
+}
+
+// TestCrossVaultMoveFailsWhenSourceRemains is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/159, where a move between two
+// vaults whose source could not be removed reported the secret moved and
+// succeeded. It must fail, and the copy and the source must both be there.
+// The state directory lock refuses the wrapped filesystem, so the test
+// calls copySecretAndRemoveSource, which a move between vaults runs once it
+// holds the lock.
+func TestCrossVaultMoveFailsWhenSourceRemains(t *testing.T) {
+	t.Parallel()
+
+	base := afero.NewMemMapFs()
+	createCrossVaults(t, base)
+
+	instance := NewCLIInstanceWithStateDir(&renameFailFs{
+		Fs:   base,
+		path: testStateDir + "/vaults.d/default/secrets.d/x",
+	}, testStateDir)
+
+	source := vault.NewVault(instance.fs, testStateDir, "default")
+	source.Mnemonic = testMnemonicBuffer(t)
+
+	err := instance.copySecretAndRemoveSource(&cobra.Command{}, source, "x",
+		vault.NewVault(instance.fs, testStateDir, "work"), "y", false)
+	require.ErrorIs(t, err, errPermissionDenied)
+	require.ErrorContains(t, err, "secret 'default:x' is copied with all "+
+		"1 version(s) to 'work:y', but the source remains")
+
+	requireValue(t, base, "default", "x")
+	requireValue(t, base, "work", "y")
+}
+
+// TestCrossVaultMoveFailsWhenOldFilesRemain makes the removal of the source
+// fail after the source has been moved out of its vault, when only the
+// temporary directory it was moved into cannot be deleted. The move must
+// fail, say that the secret is moved, and name that directory.
+func TestCrossVaultMoveFailsWhenOldFilesRemain(t *testing.T) {
+	t.Parallel()
+
+	base := afero.NewMemMapFs()
+	createCrossVaults(t, base)
+
+	sourceVaultDir := testStateDir + "/vaults.d/default"
+	fs := &removeAllFailFs{Fs: base, dir: sourceVaultDir}
+
+	instance := NewCLIInstanceWithStateDir(fs, testStateDir)
+
+	source := vault.NewVault(fs, testStateDir, "default")
+	source.Mnemonic = testMnemonicBuffer(t)
+
+	err := instance.copySecretAndRemoveSource(&cobra.Command{}, source, "x",
+		vault.NewVault(fs, testStateDir, "work"), "y", false)
+	require.ErrorIs(t, err, errPermissionDenied)
+	require.ErrorContains(t, err, "secret 'default:x' is moved with all "+
+		"1 version(s) to 'work:y', but its old files could not all be deleted")
+
+	// The error names the directory that holds the old files
+	require.NotEmpty(t, fs.failed)
+	require.ErrorContains(t, err, fs.failed)
+
+	oldFiles, err := afero.DirExists(base, filepath.Join(fs.failed, "x"))
+	require.NoError(t, err)
+	require.True(t, oldFiles)
+
+	// The source vault no longer holds the secret
+	remains, err := afero.DirExists(base, sourceVaultDir+"/secrets.d/x")
+	require.NoError(t, err)
+	require.False(t, remains)
+
+	requireValue(t, base, "work", "y")
+}
+
+// TestCrossVaultMoveUnlocksBeforeLocking is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/189: a move between two vaults
+// unlocked them while holding the state directory lock, and unlocking a
+// vault can ask for its passphrase, so every other command that changes
+// anything waited while the user answered. Here each vault has a passphrase
+// unlocker whose passphrase is the vault's name, and the move is given the
+// passphrase of only one of them. Run while the test holds the lock, it
+// must fail to unlock the other vault without waiting for the lock.
+//
+//nolint:paralleltest // times commands against the in-memory lock all tests share
+func TestCrossVaultMoveUnlocksBeforeLocking(t *testing.T) {
+	fs := afero.NewMemMapFs()
+	createCrossVaults(t, fs)
+
+	vaults, err := vault.ListVaults(fs, testStateDir)
+	require.NoError(t, err)
+
+	for _, name := range vaults {
+		vlt := vault.NewVault(fs, testStateDir, name)
+		vlt.Mnemonic = testMnemonicBuffer(t)
+
+		passphrase := memguard.NewBufferFromBytes([]byte(name))
+		_, err = vlt.CreatePassphraseUnlocker(passphrase)
+		passphrase.Destroy()
+		require.NoError(t, err)
+	}
+
+	// The move is from "default" to "work"
+	for _, tc := range []struct {
+		// failing is the vault that the move cannot unlock, given the
+		// passphrase of the vault named given
+		failing, given string
+	}{
+		{"source", "work"},
+		{"destination", "default"},
+	} {
+		t.Run(tc.failing, func(t *testing.T) {
+			passphrase := memguard.NewBufferFromBytes([]byte(tc.given))
+			defer passphrase.Destroy()
+
+			instance := NewCLIInstanceWithStateDir(fs, testStateDir)
+			instance.UnlockPassphrase = passphrase
+
+			release, err := vault.LockStateDir(fs, testStateDir)
+			require.NoError(t, err)
+			// Also frees a waiting move if the test fails, so that it
+			// releases the lock the other tests use
+			defer release()
+
+			done := make(chan error, 1)
+
+			go func() {
+				done <- instance.MoveSecret(&cobra.Command{},
+					"default:x", "work:y", false)
+			}()
+
+			select {
+			case err := <-done:
+				require.ErrorIs(t, err, secret.ErrWrongPassphrase)
+				require.ErrorContains(t, err,
+					"failed to unlock "+tc.failing+" vault")
+			case <-time.After(lockWait):
+				t.Fatal("waited for the lock before unlocking the " +
+					tc.failing + " vault")
+			}
+		})
+	}
+}
+
+// TestCrossVaultMoveChecksAgainOnceLocked runs a move between vaults, given
+// the mnemonic, as TestCommandsCheckAgainOnceLocked runs the commands in its
+// table: once the move waits for the state directory lock, the test removes
+// the source secret or the destination vault, or creates the destination
+// vault again from another mnemonic, and the move must then fail and change
+// nothing.
+//
+//nolint:paralleltest // waitingForLock sees any test's command waiting for the lock
+func TestCrossVaultMoveChecksAgainOnceLocked(t *testing.T) {
+	mnemonic := testMnemonicBuffer(t)
+
+	otherMnemonic := memguard.NewBufferFromBytes([]byte("legal winner thank " +
+		"year wave sausage worth useful legal winner thank yellow"))
+	t.Cleanup(otherMnemonic.Destroy)
+
+	move := func(cli *Instance) error {
+		cli.Mnemonic = mnemonic
+
+		return cli.MoveSecret(cli.cmd, "work:test/secret", "other:moved", false)
+	}
+
+	t.Run("source removed", func(t *testing.T) {
+		requireChecksAgainOnceLocked(t, move, func(fs afero.Fs) error {
+			return fs.RemoveAll(testStateDir + "/vaults.d/work/secrets.d/test%secret")
+		}, vault.ErrSecretNotFound)
+	})
+
+	t.Run("destination vault removed", func(t *testing.T) {
+		requireChecksAgainOnceLocked(t, move, func(fs afero.Fs) error {
+			return fs.RemoveAll(testStateDir + "/vaults.d/other")
+		}, vault.ErrVaultNotFound)
+	})
+
+	t.Run("destination vault created again from another mnemonic", func(t *testing.T) {
+		// Set by the move, which has finished once
+		// requireChecksAgainOnceLocked returns
+		var moveErr error
+
+		requireChecksAgainOnceLocked(t, func(cli *Instance) error {
+			moveErr = move(cli)
+
+			return moveErr
+		}, func(fs afero.Fs) error {
+			err := fs.RemoveAll(testStateDir + "/vaults.d/other")
+			if err != nil {
+				return err
+			}
+
+			_, err = vault.CreateVault(fs, testStateDir, "other", otherMnemonic, nil)
+
+			return err
+		}, errVaultCreatedAgain)
+
+		// secret unlocker add fails with the same error, so its message
+		// must fit a move too
+		require.EqualError(t, moveErr, "vault 'other' was removed and "+
+			"created again with another key while it was being unlocked; "+
+			"nothing was changed")
+	})
+}
+
+// TestCrossVaultMoveRefusesExistingSecretBeforeUnlocking checks that a move
+// between vaults onto a secret that exists, without --force, is refused
+// before either vault is unlocked, since unlocking can ask for a
+// passphrase. The vaults have no unlockers and the move is given no
+// mnemonic, so unlocking either vault fails.
+func TestCrossVaultMoveRefusesExistingSecretBeforeUnlocking(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	createCrossVaults(t, fs)
+
+	work := vault.NewVault(fs, testStateDir, "work")
+	work.Mnemonic = testMnemonicBuffer(t)
+
+	err := work.AddSecret("y",
+		memguard.NewBufferFromBytes([]byte("other")), false)
+	require.NoError(t, err)
+
+	instance := NewCLIInstanceWithStateDir(fs, testStateDir)
+
+	err = instance.MoveSecret(&cobra.Command{}, "default:x", "work:y", false)
+	require.ErrorIs(t, err, vault.ErrSecretExists)
+}

internal/cli/passphrase_prompt_test.go

@@ -138,6 +138,148 @@ func TestInitWritesNoBlankLineAfterMnemonic(t *testing.T) {
 		"stderr: %q", stderr.String())
 }
 
+// TestUnlockerAddAsksForNewPassphrase is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/154: with SB_UNLOCK_PASSPHRASE set,
+// `secret unlocker add passphrase` gave the new unlocker the passphrase in
+// the variable without asking. At a terminal it must ask for the new
+// passphrase there, twice, and unlock the vault with the variable, so that
+// `secret get` then opens the vault with the passphrase typed. pty.Open is
+// described above TestRemoveIgnoresTerminalOnStdout.
+func TestUnlockerAddAsksForNewPassphrase(t *testing.T) {
+	t.Parallel()
+
+	const newPassphrase = "new-passphrase"
+
+	stateDir := t.TempDir()
+
+	passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
+	defer passphrase.Destroy()
+
+	vlt, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default",
+		testMnemonicBuffer(t), passphrase)
+	require.NoError(t, err)
+
+	value := memguard.NewBufferFromBytes([]byte("v1"))
+	defer value.Destroy()
+
+	require.NoError(t, vlt.AddSecret("s1", value, false))
+
+	ctx, cancel := context.WithTimeout(t.Context(), commandWait)
+	defer cancel()
+
+	//nolint:gosec // G204: test executes the freshly built secret binary
+	cmd := exec.CommandContext(ctx, secretBinaryPath(t),
+		"unlocker", "add", "passphrase")
+	cmd.Env = []string{
+		secret.EnvStateDir + "=" + stateDir,
+		secret.EnvUnlockPassphrase + "=" + testPassphrase,
+		"PATH=" + os.Getenv("PATH"),
+		"HOME=" + os.Getenv("HOME"),
+	}
+
+	ptmx, tty, err := pty.Open()
+	require.NoError(t, err)
+
+	defer func() { _ = ptmx.Close() }()
+
+	deadline, _ := ctx.Deadline()
+	require.NoError(t, ptmx.SetReadDeadline(deadline))
+
+	cmd.Stderr = tty
+	// Ctty is a descriptor in the new process: 2, its stderr.
+	cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true, Setctty: true, Ctty: 2}
+
+	require.NoError(t, cmd.Start())
+
+	_ = tty.Close()
+
+	var (
+		shown []byte
+		char  byte
+	)
+
+	terminal := bufio.NewReader(ptmx)
+
+	for _, prompt := range []string{
+		"Enter passphrase for unlocker: ", "Confirm passphrase: ",
+	} {
+		for !bytes.HasSuffix(shown, []byte(prompt)) {
+			char, err = terminal.ReadByte()
+			require.NoError(t, err,
+				"secret unlocker add passphrase did not ask on the terminal: %s",
+				shown)
+
+			shown = append(shown, char)
+		}
+
+		_, err = ptmx.WriteString(newPassphrase + "\n")
+		require.NoError(t, err)
+	}
+
+	// The read ends once the command has exited and so closed the terminal.
+	rest, _ := io.ReadAll(terminal)
+
+	require.NoError(t, cmd.Wait(),
+		"secret unlocker add passphrase: %s%s", shown, rest)
+
+	// The binary, not this test process, opens the vault: the new unlocker
+	// has age's full scrypt work factor, which is slow under the race
+	// detector.
+	assert.Equal(t, "v1",
+		runWithoutTerminal(t, stateDir, newPassphrase, "get", "s1"))
+}
+
+// TestUnlockerAddWithoutTerminalSaysPassphraseIsVariable is a regression
+// test for https://git.eeqj.de/sneak/secret/issues/154: with no terminal to
+// ask on, `secret unlocker add passphrase` gives the new unlocker the
+// passphrase in SB_UNLOCK_PASSPHRASE, and its output must say so.
+func TestUnlockerAddWithoutTerminalSaysPassphraseIsVariable(t *testing.T) {
+	t.Parallel()
+
+	stateDir := t.TempDir()
+
+	passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
+	defer passphrase.Destroy()
+
+	_, err := vault.CreateVault(afero.NewOsFs(), stateDir, "default",
+		testMnemonicBuffer(t), passphrase)
+	require.NoError(t, err)
+
+	output := runWithoutTerminal(t, stateDir, testPassphrase,
+		"unlocker", "add", "passphrase")
+
+	assert.Contains(t, output, "The new unlocker takes its passphrase "+
+		"from SB_UNLOCK_PASSPHRASE, since there is no terminal to ask for a "+
+		"new one on\n")
+}
+
+// runWithoutTerminal runs the secret binary with args and SB_UNLOCK_PASSPHRASE
+// set to passphrase, in a new session, which has no terminal, and returns
+// what it printed once it has succeeded.
+func runWithoutTerminal(
+	t *testing.T, stateDir, passphrase string, args ...string,
+) string {
+	t.Helper()
+
+	ctx, cancel := context.WithTimeout(t.Context(), commandWait)
+	defer cancel()
+
+	//nolint:gosec // G204: test executes the freshly built secret binary
+	cmd := exec.CommandContext(ctx, secretBinaryPath(t), args...)
+	cmd.Env = []string{
+		secret.EnvStateDir + "=" + stateDir,
+		secret.EnvUnlockPassphrase + "=" + passphrase,
+		"PATH=" + os.Getenv("PATH"),
+		"HOME=" + os.Getenv("HOME"),
+	}
+	cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true}
+
+	output, err := cmd.CombinedOutput()
+	require.NoError(t, err, "secret %v: %s", args, output)
+
+	return string(output)
+}
+
 // runAtTerminal runs the secret binary with args and input on stdin, in a
 // new session whose controlling terminal is a new terminal, which is also
 // its stderr: a command typed at a terminal with its input piped. pty.Open

internal/cli/secrets.go

@@ -422,26 +422,92 @@ func (cli *Instance) AddSecret(secretName string, force bool) error {
 	// 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)
+	return cli.storeSecret(vlt, secretName, valueBuffer, force)
+}
+
+// storeSecret adds the secret name with value to vlt, as vault.AddSecret
+// does, holding the state directory lock while it does. With force, adding
+// an existing secret replaces its current version, which needs the vault's
+// long-term key. Getting the key can ask for the passphrase, so storeSecret
+// gets it before it takes the lock, and no other command waits while the
+// user answers. If another command adds the secret while this one waits for
+// the lock, storeSecret lets go of the lock, gets the key, and takes the
+// lock again.
+func (cli *Instance) storeSecret(
+	vlt *vault.Vault, name string, value *memguard.LockedBuffer, force bool,
+) error {
+	err := vault.ValidateSecretName(name)
 	if err != nil {
 		return err
 	}
+
+	replacing, err := replacesVersion(vlt, name, force)
+	if err != nil {
+		return err
+	}
+
+	if replacing {
+		_, err = vlt.GetOrDeriveLongTermKey()
+		if err != nil {
+			return fmt.Errorf("failed to get long-term key: %w", err)
+		}
+	}
+
+	needsKey, err := cli.addWithoutAsking(vlt, name, value, force)
+	if !needsKey {
+		return err
+	}
+
+	_, err = vlt.GetOrDeriveLongTermKey()
+	if err != nil {
+		return fmt.Errorf("failed to get long-term key: %w", err)
+	}
+
+	// With the key in memory, this adds the secret
+	_, err = cli.addWithoutAsking(vlt, name, value, force)
+
+	return err
+}
+
+// addWithoutAsking takes the state directory lock and adds the secret name
+// with value to vlt, as vault.AddSecret does. If that would replace the
+// secret's current version while the vault's long-term key is not in
+// memory, getting the key could ask for the passphrase with the lock held:
+// addWithoutAsking then adds nothing and returns true.
+func (cli *Instance) addWithoutAsking(
+	vlt *vault.Vault, name string, value *memguard.LockedBuffer, force bool,
+) (bool, error) {
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return false, err
+	}
 	defer release()
 
-	// Add the secret to the vault
-	secret.Debug("Calling vault.AddSecret", "secret_name", secretName,
-		"value_length", valueBuffer.Size(), "force", force)
-
-	err = vlt.AddSecret(secretName, valueBuffer, force)
+	replacing, err := replacesVersion(vlt, name, force)
 	if err != nil {
-		secret.Debug("vault.AddSecret failed", "error", err)
-
-		return err
+		return false, err
 	}
 
-	secret.Debug("vault.AddSecret completed successfully")
+	if replacing && vlt.Locked() {
+		return true, nil
+	}
 
-	return nil
+	return false, vlt.AddSecret(name, value, force)
+}
+
+// replacesVersion reports whether adding the secret name to vlt replaces
+// the secret's current version: whether force is set and the secret has one.
+func replacesVersion(vlt *vault.Vault, name string, force bool) (bool, error) {
+	if !force {
+		return false, nil
+	}
+
+	exists, err := secret.NewSecret(vlt, name).Exists()
+	if err != nil {
+		return false, fmt.Errorf("failed to check if secret exists: %w", err)
+	}
+
+	return exists, nil
 }
 
 // GetSecret retrieves and prints the current version of a secret
@@ -707,14 +773,7 @@ func (cli *Instance) ImportSecret(
 
 	// 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)
+	err = cli.storeSecret(vlt, secretName, valueBuffer, force)
 	if err != nil {
 		return err
 	}
@@ -816,24 +875,12 @@ func (cli *Instance) findSecretToRemove(
 	}, nil
 }
 
-// MoveSecret moves or renames a secret (within or across vaults), holding
-// the state directory lock.
+// MoveSecret moves or renames a secret (within or across vaults). It
+// leaves taking the state directory lock to moveSecretWithinVault and
+// moveSecretCrossVault, since a move between vaults unlocks both vaults
+// before it takes the 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)
@@ -933,8 +980,9 @@ func (cli *Instance) existingVault(name string) (*vault.Vault, error) {
 	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.
+// moveSecretWithinVault renames a secret within the vault vlt, holding the
+// state directory lock. 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 {
@@ -944,6 +992,12 @@ func (cli *Instance) moveSecretWithinVault(
 		return fmt.Errorf("secret '%s' %w", source, errMoveOntoItself)
 	}
 
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	vaultDir, err := vlt.GetDirectory()
 	if err != nil {
 		return err
@@ -1030,59 +1084,75 @@ func (cli *Instance) sameDirectory(dir, other string) (bool, error) {
 
 // moveSecretCrossVault handles moving between two different vaults. Its
 // caller, MoveSecret, has already checked both secret names and that both
-// vaults exist.
+// vaults exist. The copy needs the source vault's long-term key and is
+// encrypted to the destination vault's long-term public key. The
+// destination vault is unlocked to check that the copy will open with what
+// the user gave, and, once the state directory lock is held, that it still
+// has the key unlocked. Either unlock can ask for a passphrase, so both
+// happen before the lock is taken, and no other command waits while the
+// user answers.
 func (cli *Instance) moveSecretCrossVault(
 	cmd *cobra.Command,
 	srcVault *vault.Vault, srcSecretName string,
 	destVault *vault.Vault, destSecretName string,
 	force bool,
 ) error {
-	srcVaultDir, err := srcVault.GetDirectory()
-	if err != nil {
-		return fmt.Errorf("failed to get source vault directory: %w", err)
-	}
-
-	// Verify source secret exists
-	srcStorageName := strings.ReplaceAll(srcSecretName, "/", "%")
-	srcSecretDir := filepath.Join(srcVaultDir, "secrets.d", srcStorageName)
-
-	exists, err := afero.DirExists(cli.fs, srcSecretDir)
-	if err != nil || !exists {
-		return fmt.Errorf("secret '%s' %w in vault '%s'",
-			srcSecretName, vault.ErrSecretNotFound, srcVault.Name)
-	}
-
-	// The source is removed after the copy, so a destination that is the
-	// 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)
-	}
-
-	destStorageName := strings.ReplaceAll(destSecretName, "/", "%")
-	destSecretDir := filepath.Join(destVaultDir, "secrets.d", destStorageName)
-
-	same, err := cli.sameDirectory(srcSecretDir, destSecretDir)
+	// Before unlocking, check that the source secret exists, that the
+	// destination is not the source under another name, and, without
+	// --force, that the destination secret does not exist
+	_, err := cli.checkCrossVaultMove(
+		srcVault, srcSecretName, destVault, destSecretName, force)
 	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)
-	}
-
-	// Copying needs the long-term keys of both vaults
 	srcVault.Mnemonic, srcVault.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
 	destVault.Mnemonic, destVault.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
 
 	// Unlock destination vault (will fail if neither mnemonic nor unlocker available)
-	_, err = destVault.GetOrDeriveLongTermKey()
+	destIdentity, err := destVault.GetOrDeriveLongTermKey()
 	if err != nil {
 		return fmt.Errorf("failed to unlock destination vault '%s': %w", destVault.Name, err)
 	}
 
+	_, err = srcVault.GetOrDeriveLongTermKey()
+	if err != nil {
+		return fmt.Errorf("failed to unlock source vault '%s': %w",
+			srcVault.Name, err)
+	}
+
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	err = cli.checkLongTermKey(destVault, destIdentity)
+	if err != nil {
+		return err
+	}
+
+	return cli.copySecretAndRemoveSource(
+		cmd, srcVault, srcSecretName, destVault, destSecretName, force)
+}
+
+// copySecretAndRemoveSource copies the secret srcSecretName of srcVault,
+// with all its versions, to destSecretName in destVault, and removes the
+// source. moveSecretCrossVault calls it holding the state directory lock.
+// Another command may have changed either vault since moveSecretCrossVault
+// checked them, so it checks them again first.
+func (cli *Instance) copySecretAndRemoveSource(
+	cmd *cobra.Command,
+	srcVault *vault.Vault, srcSecretName string,
+	destVault *vault.Vault, destSecretName string,
+	force bool,
+) error {
+	srcSecretDir, err := cli.checkCrossVaultMove(
+		srcVault, srcSecretName, destVault, destSecretName, force)
+	if err != nil {
+		return err
+	}
+
 	// Count versions for user feedback
 	versions, _ := secret.ListVersions(cli.fs, srcSecretDir)
 	versionCount := len(versions)
@@ -1097,12 +1167,9 @@ func (cli *Instance) moveSecretCrossVault(
 	// Delete source secret
 	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",
-			srcVault.Name, srcSecretName, destVault.Name, destSecretName, versionCount)
-
-		return nil
+		return cli.sourceRemovalError(err, srcSecretDir,
+			srcVault.Name+":"+srcSecretName, destVault.Name+":"+destSecretName,
+			versionCount)
 	}
 
 	cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",
@@ -1110,3 +1177,90 @@ func (cli *Instance) moveSecretCrossVault(
 
 	return nil
 }
+
+// checkCrossVaultMove checks that the secret srcSecretName exists in
+// srcVault, that destSecretName in destVault is not the source under
+// another name, and, unless force is set, that destSecretName does not
+// exist in destVault. It returns the source's directory.
+func (cli *Instance) checkCrossVaultMove(
+	srcVault *vault.Vault, srcSecretName string,
+	destVault *vault.Vault, destSecretName string,
+	force bool,
+) (string, error) {
+	srcVaultDir, err := srcVault.GetDirectory()
+	if err != nil {
+		return "", fmt.Errorf("failed to get source vault directory: %w", err)
+	}
+
+	// Verify source secret exists
+	srcStorageName := strings.ReplaceAll(srcSecretName, "/", "%")
+	srcSecretDir := filepath.Join(srcVaultDir, "secrets.d", srcStorageName)
+
+	exists, err := afero.DirExists(cli.fs, srcSecretDir)
+	if err != nil || !exists {
+		return "", fmt.Errorf("secret '%s' %w in vault '%s'",
+			srcSecretName, vault.ErrSecretNotFound, srcVault.Name)
+	}
+
+	// The source is removed after the copy, so a destination that is the
+	// 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)
+	}
+
+	destStorageName := strings.ReplaceAll(destSecretName, "/", "%")
+	destSecretDir := filepath.Join(destVaultDir, "secrets.d", destStorageName)
+
+	same, err := cli.sameDirectory(srcSecretDir, destSecretDir)
+	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)
+	}
+
+	exists, err = afero.DirExists(cli.fs, destSecretDir)
+	if err != nil {
+		return "", fmt.Errorf("failed to check if destination secret exists: %w", err)
+	}
+
+	if exists && !force {
+		return "", fmt.Errorf(
+			"secret '%s' %w in vault '%s' (use --force to overwrite)",
+			destSecretName, vault.ErrSecretExists, destVault.Name)
+	}
+
+	return srcSecretDir, nil
+}
+
+// sourceRemovalError returns the error of a move between vaults that
+// copied all versionCount versions of the secret srcName to destName, and
+// then failed with removeErr to remove srcSecretDir, the directory of
+// srcName. The removal can fail after the source has left its vault, when
+// only the temporary directory it was moved into cannot be deleted, so the
+// error says the source remains only if srcSecretDir is still there.
+func (cli *Instance) sourceRemovalError(
+	removeErr error, srcSecretDir, srcName, destName string, versionCount int,
+) error {
+	remains, err := afero.DirExists(cli.fs, srcSecretDir)
+
+	switch {
+	case err != nil:
+		return fmt.Errorf("secret '%s' is copied with all %d version(s) "+
+			"to '%s', but removing the source failed and whether it "+
+			"remains is unknown: %w",
+			srcName, versionCount, destName, errors.Join(removeErr, err))
+	case remains:
+		return fmt.Errorf("secret '%s' is copied with all %d version(s) "+
+			"to '%s', but the source remains: %w",
+			srcName, versionCount, destName, removeErr)
+	default:
+		return fmt.Errorf("secret '%s' is moved with all %d version(s) "+
+			"to '%s', but its old files could not all be deleted: %w",
+			srcName, versionCount, destName, removeErr)
+	}
+}

internal/cli/secrets_size_test.go

@@ -36,17 +36,11 @@ func newSizeTestVault(t *testing.T) (afero.Fs, *vault.Vault) {
 
 	fs := afero.NewMemMapFs()
 
-	// Create vault
+	// Create vault, which selects it as the current vault
 	_, err := vault.CreateVault(fs, testStateDir, testVaultName,
 		testMnemonicBuffer(t), nil)
 	require.NoError(t, err)
 
-	// Set current vault
-	currentVaultPath := filepath.Join(testStateDir, "currentvault")
-	vaultPath := filepath.Join(testStateDir, "vaults.d", testVaultName)
-	err = afero.WriteFile(fs, currentVaultPath, []byte(vaultPath), 0o600)
-	require.NoError(t, err)
-
 	// Get vault and set up long-term key
 	vlt, err := vault.GetCurrentVault(fs, testStateDir)
 	require.NoError(t, err)

internal/cli/trailing_newline_test.go

@@ -0,0 +1,47 @@
+//nolint:testpackage // white-box test of unexported internals
+package cli
+
+import (
+	"bytes"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestAddDropsOneTrailingNewlineImportKeepsIt pins the rule the README
+// gives: `secret add` drops one trailing newline from stdin, and
+// `secret import` stores the file's bytes unchanged.
+func TestAddDropsOneTrailingNewlineImportKeepsIt(t *testing.T) {
+	t.Parallel()
+
+	fs, vlt := newSizeTestVault(t)
+	value := []byte("a\n\n")
+
+	cmd := &cobra.Command{}
+	cmd.SetIn(bytes.NewReader(value))
+
+	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
+	cli.cmd = cmd
+
+	require.NoError(t, cli.AddSecret("added", false))
+
+	sourceFile := "/test/value"
+	require.NoError(t, afero.WriteFile(fs, sourceFile, value, 0o600))
+	require.NoError(t, cli.ImportSecret(cmd, "imported", sourceFile, false))
+
+	added, err := vlt.GetSecret("added")
+	require.NoError(t, err)
+
+	defer added.Destroy()
+
+	imported, err := vlt.GetSecret("imported")
+	require.NoError(t, err)
+
+	defer imported.Destroy()
+
+	assert.Equal(t, []byte("a\n"), added.Bytes())
+	assert.Equal(t, []byte("a\n\n"), imported.Bytes())
+}

internal/cli/unknown_subcommand_test.go

@@ -0,0 +1,58 @@
+package cli_test
+
+import (
+	"fmt"
+	"os/exec"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/secret"
+)
+
+// TestUnknownSubcommandFails is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/183: given a subcommand it does not
+// have, `secret vault`, `secret unlocker`, `secret version` and `secret
+// generate` printed their help and exited 0, so a script could not tell a
+// mistyped command from a successful one. Each runs here as the built binary
+// and must exit 1 naming the subcommand, as an unknown command does at the
+// top level, and must still print its help and exit 0 when given nothing.
+func TestUnknownSubcommandFails(t *testing.T) {
+	t.Parallel()
+
+	stateDir := t.TempDir()
+
+	// run runs the built binary with args and returns what it printed and
+	// its exit code
+	run := func(args ...string) (string, int) {
+		//nolint:gosec // G204: test executes the freshly built secret binary
+		cmd := exec.CommandContext(t.Context(), secretBinaryPath(t), args...)
+		cmd.Env = []string{secret.EnvStateDir + "=" + stateDir}
+
+		output, err := cmd.CombinedOutput()
+		if err != nil {
+			var exitErr *exec.ExitError
+			require.ErrorAs(t, err, &exitErr, "secret %v did not run", args)
+		}
+
+		return string(output), cmd.ProcessState.ExitCode()
+	}
+
+	for _, call := range []string{
+		"vault improt", "unlocker foo", "version foo", "generate foo",
+	} {
+		parent, unknown, _ := strings.Cut(call, " ")
+
+		output, exitCode := run(parent, unknown)
+		assert.Equal(t, 1, exitCode, "secret %s should fail:\n%s", call, output)
+		assert.Contains(t, output, fmt.Sprintf(
+			"unknown command %q for \"secret %s\"", unknown, parent))
+
+		output, exitCode = run(parent)
+		assert.Equal(t, 0, exitCode,
+			"secret %s should succeed:\n%s", parent, output)
+		assert.Contains(t, output, usageHeading,
+			"secret %s should print its help", parent)
+	}
+}

internal/cli/unlockers.go

@@ -15,6 +15,7 @@ import (
 	"strings"
 	"time"
 
+	"filippo.io/age"
 	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
 	"sneak.berlin/go/secret/internal/secret"
@@ -39,6 +40,10 @@ var (
 	// composes "GPG key <id> is already added as an unlocker".
 	errGPGKeyAlreadyUnlocker = errors.New(
 		"is already added as an unlocker")
+	// errVaultCreatedAgain carries only the message tail; the caller
+	// composes "vault '<name>' was removed and created again ...".
+	errVaultCreatedAgain = errors.New("was removed and created again " +
+		"with another key while it was being unlocked; nothing was changed")
 )
 
 // UnlockerInfo represents unlocker information for display
@@ -126,6 +131,12 @@ func newUnlockerCmd() *cobra.Command {
 		Use:   "unlocker",
 		Short: "Manage unlockers",
 		Long:  `Create, list, and remove unlockers for the current vault.`,
+		// Cobra checks Args only on a command that has a RunE; without
+		// one, an unknown subcommand prints this help and exits 0.
+		Args: cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return cmd.Help()
+		},
 	}
 
 	cmd.AddCommand(newUnlockerListCmd())
@@ -314,7 +325,8 @@ func (cli *Instance) UnlockersList(jsonOutput bool) error {
 		return err
 	}
 
-	var unlockers []UnlockerInfo
+	// Not nil, so that a vault with no unlockers prints [] in JSON, not null
+	unlockers := make([]UnlockerInfo, 0, len(unlockerMetadata))
 
 	for _, unlockerID := range slices.Sorted(maps.Keys(unlockerMetadata)) {
 		metadata := unlockerMetadata[unlockerID]
@@ -397,12 +409,6 @@ 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)
@@ -416,6 +422,64 @@ func (cli *Instance) UnlockersAdd(unlockerType string, cmd *cobra.Command) error
 	}
 }
 
+// unlockThenLock gets the long-term key of vlt, the vault an unlocker is
+// added to, then takes the state directory lock and returns the function
+// that releases it. Getting the key can ask for a passphrase, so it comes
+// before the lock, as the question of askThenLock does, and no other
+// command waits while the user answers. Once the lock is held, it fails if
+// another command has removed the vault in the meantime, since writing the
+// unlocker would create the vault's directory again, holding nothing else.
+// It also fails if the vault was removed and created again with another
+// long-term key, which the unlocker would then hold instead of the new
+// vault's own.
+func (cli *Instance) unlockThenLock(vlt *vault.Vault) (func(), error) {
+	ltIdentity, err := vlt.GetOrDeriveLongTermKey()
+	if err != nil {
+		return nil, fmt.Errorf("failed to get long-term key: %w", err)
+	}
+
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return nil, err
+	}
+
+	err = cli.checkLongTermKey(vlt, ltIdentity)
+	if err != nil {
+		release()
+
+		return nil, err
+	}
+
+	return release, nil
+}
+
+// checkLongTermKey checks, for the vault vlt unlocked as ltIdentity before
+// the state directory lock was taken, that vlt still exists and that its
+// long-term public key is still that of ltIdentity. Another command may have
+// removed the vault in the meantime, or removed it and created it again with
+// another long-term key. The caller holds the lock.
+func (cli *Instance) checkLongTermKey(
+	vlt *vault.Vault, ltIdentity *age.X25519Identity,
+) error {
+	_, err := cli.existingVault(vlt.GetName())
+	if err != nil {
+		return err
+	}
+
+	vaultDir, _ := vlt.GetDirectory()
+
+	ltPubKey, err := afero.ReadFile(cli.fs, filepath.Join(vaultDir, "pub.age"))
+	if err != nil {
+		return fmt.Errorf("failed to read long-term public key: %w", err)
+	}
+
+	if string(ltPubKey) != ltIdentity.Recipient().String() {
+		return fmt.Errorf("vault '%s' %w", vlt.GetName(), errVaultCreatedAgain)
+	}
+
+	return nil
+}
+
 // autoSelectUnlocker selects the newly created unlocker as current,
 // printing a warning if selection fails
 func autoSelectUnlocker(cmd *cobra.Command, vlt *vault.Vault, unlockerID string) {
@@ -435,23 +499,31 @@ func (cli *Instance) addPassphraseUnlocker(cmd *cobra.Command) error {
 		return fmt.Errorf("failed to get current vault: %w", err)
 	}
 
-	// For passphrase unlockers, we don't need the vault to be unlocked
-	// The CreatePassphraseUnlocker method will handle getting the
-	// long-term key
 	vlt.Mnemonic, vlt.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
 
-	// The new unlocker gets the passphrase from the environment, which also
-	// unlocks the current passphrase unlocker, else the one entered here
-	passphraseBuffer := cli.UnlockPassphrase
-	if passphraseBuffer == nil {
-		// Use secure passphrase input with confirmation
-		passphraseBuffer, err = readSecurePassphrase("Enter passphrase for unlocker: ")
-		if err != nil {
-			return err
-		}
+	// The new passphrase is asked for even when SB_UNLOCK_PASSPHRASE is
+	// set, and the variable unlocks the vault. With no terminal to ask on,
+	// the new unlocker takes the passphrase in the variable.
+	fromVariable := false
+
+	passphraseBuffer, err := readSecurePassphrase("Enter passphrase for unlocker: ")
+
+	switch {
+	case errors.Is(err, secret.ErrNoTerminal) && cli.UnlockPassphrase != nil:
+		passphraseBuffer = cli.UnlockPassphrase
+		fromVariable = true
+	case err != nil:
+		return err
+	default:
 		defer passphraseBuffer.Destroy()
 	}
 
+	release, err := cli.unlockThenLock(vlt)
+	if err != nil {
+		return err
+	}
+	defer release()
+
 	passphraseUnlocker, err := vlt.CreatePassphraseUnlocker(passphraseBuffer)
 	if err != nil {
 		return err
@@ -459,6 +531,12 @@ func (cli *Instance) addPassphraseUnlocker(cmd *cobra.Command) error {
 
 	cmd.Printf("Created passphrase unlocker: %s\n", passphraseUnlocker.GetID())
 
+	if fromVariable {
+		cmd.Printf("The new unlocker takes its passphrase from %s, since "+
+			"there is no terminal to ask for a new one on\n",
+			secret.EnvUnlockPassphrase)
+	}
+
 	// CreatePassphraseUnlocker has already made it the current unlocker
 	cmd.Printf("Automatically selected as current unlocker\n")
 
@@ -484,12 +562,13 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error {
 		cmd.Printf("Using default GPG key: %s\n", gpgKeyID)
 	}
 
-	// Check if this key is already added as an unlocker
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
 		return fmt.Errorf("failed to get current vault: %w", err)
 	}
 
+	vlt.Mnemonic, vlt.UnlockPassphrase = cli.Mnemonic, cli.UnlockPassphrase
+
 	// Resolve the GPG key ID to its fingerprint, once: the duplicate check
 	// and the new unlocker's metadata both use this result
 	fingerprint, err := secret.ResolveGPGKeyFingerprint(gpgKeyID)
@@ -497,21 +576,26 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error {
 		return fmt.Errorf("failed to resolve GPG key fingerprint: %w", err)
 	}
 
-	// Check if this GPG key is already added
-	exists, err := cli.pgpUnlockerExists(vlt, fingerprint)
+	// Refuse a key that is already an unlocker before asking for anything,
+	// and again once the lock is held, since another command may have added
+	// it in the meantime
+	err = cli.checkPGPKeyNotAdded(vlt, gpgKeyID, fingerprint)
 	if err != nil {
-		return fmt.Errorf(
-			"could not check whether GPG key %s is already an unlocker: %w",
-			gpgKeyID, err,
-		)
+		return err
 	}
 
-	if exists {
-		return fmt.Errorf("GPG key %s %w", gpgKeyID, errGPGKeyAlreadyUnlocker)
+	release, err := cli.unlockThenLock(vlt)
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	err = cli.checkPGPKeyNotAdded(vlt, gpgKeyID, fingerprint)
+	if err != nil {
+		return err
 	}
 
-	pgpUnlocker, err := secret.CreatePGPUnlocker(cli.fs, cli.stateDir,
-		gpgKeyID, fingerprint, cli.Mnemonic, cli.UnlockPassphrase)
+	pgpUnlocker, err := secret.CreatePGPUnlocker(vlt, gpgKeyID, fingerprint)
 	if err != nil {
 		return err
 	}
@@ -686,6 +770,27 @@ func (cli *Instance) UnlockerSelect(unlockerID string) error {
 	return vlt.SelectUnlocker(unlockerID)
 }
 
+// checkPGPKeyNotAdded fails when vlt already has a PGP unlocker for the GPG
+// key gpgKeyID, whose fingerprint is fingerprint, or when that cannot be
+// checked.
+func (cli *Instance) checkPGPKeyNotAdded(
+	vlt *vault.Vault, gpgKeyID, fingerprint string,
+) error {
+	exists, err := cli.pgpUnlockerExists(vlt, fingerprint)
+	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)
+	}
+
+	return nil
+}
+
 // pgpUnlockerExists reports whether the vault already has a PGP unlocker
 // for the GPG key with the given fingerprint. It returns an error, and no
 // answer, when unlockers.d or an unlocker's metadata file cannot be read;

internal/cli/unlockers_list_test.go

@@ -1,13 +1,14 @@
 // Unlocker List Tests
 //
-// Tests for `secret unlocker list` behavior when an unlocker's metadata
-// cannot be read or used:
+// Tests for `secret unlocker list`:
 //
 //   - TestUnlockersListToleratesCorruptMetadata: one unlocker's corrupt
 //     metadata does not stop the others from being listed.
 //   - TestUnlockersListSkipsUnreadableMetadata: an unlocker whose metadata
 //     file cannot be checked for or read is left out, and the other is
 //     still listed.
+//   - TestUnlockersListEmptyJSON: a vault with no unlockers lists them as
+//     an empty JSON list.
 
 //nolint:testpackage // white-box test of unexported internals
 package cli
@@ -321,3 +322,36 @@ func TestUnlockersListSkipsUnreadableMetadata(t *testing.T) {
 		})
 	}
 }
+
+// TestUnlockersListEmptyJSON is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/148: listing the unlockers of a
+// vault that has none, as after its last unlocker is removed, printed
+// "unlockers": null in JSON form instead of an empty list.
+func TestUnlockersListEmptyJSON(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	vaultDir := filepath.Join(listTestStateDir, "vaults.d", listTestVaultName)
+
+	require.NoError(t, afero.WriteFile(
+		fs, filepath.Join(listTestStateDir, "currentvault"),
+		[]byte(listTestVaultName), listTestFilePerm,
+	))
+	require.NoError(t, fs.MkdirAll(
+		filepath.Join(vaultDir, listTestUnlockersDirName), listTestDirPerm,
+	))
+
+	var buf bytes.Buffer
+
+	cmd := &cobra.Command{}
+	cmd.SetOut(&buf)
+
+	instance := &Instance{fs: fs, stateDir: listTestStateDir, cmd: cmd}
+	require.NoError(t, instance.UnlockersList(true))
+
+	var decoded map[string]any
+
+	require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded))
+	assert.Equal(t, []any{}, decoded["unlockers"],
+		"no unlockers must be printed as [], not %s", buf.String())
+}

internal/cli/unreadable_dir_test.go

@@ -9,11 +9,12 @@
 // "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 addPGPUnlocker
-// for UnlockersAdd, or, for a removal, the function that makes its checks,
-// such as findVaultToRemove for RemoveVault, which runs again under the
-// lock before anything is removed, with --force or without.
+// filesystem, which the state directory lock refuses. So they call a
+// function that makes the look before any lock is taken: addPGPUnlocker for
+// UnlockersAdd, which looks again once it holds the lock, or, for a removal,
+// the function that makes its checks, such as findVaultToRemove for
+// RemoveVault, which runs again under the lock before anything is removed,
+// with --force or without.
 
 //nolint:testpackage // white-box test of unexported internals
 package cli

internal/cli/vault.go

@@ -29,6 +29,12 @@ func newVaultCmd() *cobra.Command {
 		Use:   "vault",
 		Short: "Manage vaults",
 		Long:  `Create, list, and select vaults for organizing secrets.`,
+		// Cobra checks Args only on a command that has a RunE; without
+		// one, an unknown subcommand prints this help and exits 0.
+		Args: cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return cmd.Help()
+		},
 	}
 
 	cmd.AddCommand(newVaultListCmd())
@@ -216,18 +222,14 @@ func (cli *Instance) resolvePassphrase() (*memguard.LockedBuffer, func(), error)
 	return passphraseBuffer, passphraseBuffer.Destroy, nil
 }
 
-// CreateVault creates a new vault
+// CreateVault creates a new vault. It asks for the mnemonic and the
+// passphrase before it takes the state directory lock, so that no other
+// command waits while the user answers.
 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()
-
 	// Refuse an invalid name or an existing vault before asking for anything
-	err = vault.CheckNewVaultName(cli.fs, cli.stateDir, name)
+	err := vault.CheckNewVaultName(cli.fs, cli.stateDir, name)
 	if err != nil {
 		return err
 	}
@@ -259,7 +261,14 @@ func (cli *Instance) CreateVault(cmd *cobra.Command, name string) error {
 	}
 	defer cleanupPassphrase()
 
-	// Create the vault with its passphrase unlocker
+	release, err := vault.LockStateDir(cli.fs, cli.stateDir)
+	if err != nil {
+		return err
+	}
+	defer release()
+
+	// Create the vault with its passphrase unlocker. Another command may
+	// have created it since the check above: vault.CreateVault checks again.
 	vlt, err := vault.CreateVault(cli.fs, cli.stateDir, name,
 		mnemonic, passphraseBuffer)
 	if err != nil {

internal/cli/vault_list_test.go

@@ -0,0 +1,42 @@
+package cli_test
+
+import (
+	"bytes"
+	"encoding/json"
+	"path/filepath"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/cli"
+	"sneak.berlin/go/secret/internal/secret"
+)
+
+// TestVaultListEmptyJSON is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/148: listing the vaults when
+// vaults.d holds none printed "vaults": null in JSON form instead of an
+// empty list.
+func TestVaultListEmptyJSON(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	require.NoError(t, fs.MkdirAll(
+		filepath.Join(testStateDir, "vaults.d"), secret.DirPerms,
+	))
+
+	var buf bytes.Buffer
+
+	cmd := &cobra.Command{}
+	cmd.SetOut(&buf)
+
+	c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
+	require.NoError(t, c.ListVaults(cmd, true))
+
+	var decoded map[string]any
+
+	require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded))
+	assert.Equal(t, []any{}, decoded["vaults"],
+		"no vaults must be printed as [], not %s", buf.String())
+}

internal/cli/version.go

@@ -43,6 +43,12 @@ func VersionCommands(cli *Instance) *cobra.Command {
 		Short: "Manage secret versions",
 		Long: "Commands for managing secret versions including listing, " +
 			"promoting, and retrieving specific versions",
+		// Cobra checks Args only on a command that has a RunE; without
+		// one, an unknown subcommand prints this help and exits 0.
+		Args: cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return cmd.Help()
+		},
 	}
 
 	// List versions command
@@ -229,9 +235,13 @@ func printVersionRow(
 	}
 
 	// Determine status
-	status := "expired"
-	if version == currentVersion {
+	status := "inactive"
+
+	switch {
+	case version == currentVersion:
 		status = "current"
+	case sv.Metadata.NotAfter != nil && sv.Metadata.NotAfter.Before(time.Now()):
+		status = "expired"
 	}
 
 	// Format timestamps

internal/cli/version_test.go

@@ -5,6 +5,8 @@
 // - TestListVersionsCommand: Tests `secret version list` command output
 // - TestListVersionsNonExistentSecret: Tests error handling for missing secrets
 // - TestPromoteVersionCommand: Tests `secret version promote` command
+// - TestListVersionsStatusAfterPromote: Tests each version's status after
+//   promoting an older version
 // - TestPromoteNonExistentVersion: Tests error handling for invalid promotion
 // - TestPromoteHelpSaysCurrentFileIsRewritten: Tests `secret version promote
 //   --help` text
@@ -244,6 +246,66 @@ func TestPromoteVersionCommand(t *testing.T) {
 	assert.Equal(t, []byte("version-1"), promoted.Bytes())
 }
 
+func TestListVersionsStatusAfterPromote(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+	cli := NewCLIInstanceWithStateDir(fs, testStateDir)
+	cli.Mnemonic = testMnemonicBuffer(t)
+
+	setupTestVault(t, fs)
+
+	vlt, err := vault.GetCurrentVault(fs, testStateDir)
+	require.NoError(t, err)
+
+	vlt.Mnemonic = cli.Mnemonic
+
+	addTestSecret(t, vlt, []byte("version-1"), false)
+	addTestSecret(t, vlt, []byte("version-2"), true)
+	addTestSecret(t, vlt, []byte("version-3"), true)
+
+	vaultDir, err := vlt.GetDirectory()
+	require.NoError(t, err)
+
+	// Newest first
+	versions, err := secret.ListVersions(fs,
+		filepath.Join(vaultDir, "secrets.d", "test%secret"))
+	require.NoError(t, err)
+	require.Len(t, versions, 3)
+
+	newest, middle, oldest := versions[0], versions[1], versions[2]
+
+	cmd := newRootCmd()
+
+	var buf bytes.Buffer
+	cmd.SetOut(&buf)
+	cmd.SetErr(&buf)
+
+	err = cli.PromoteVersion(cmd, "test/secret", oldest)
+	require.NoError(t, err)
+
+	buf.Reset()
+
+	err = cli.ListVersions(cmd, "test/secret")
+	require.NoError(t, err)
+
+	// STATUS is the fourth field of a row, since CREATED is a date and a time
+	statuses := map[string]string{}
+
+	for line := range strings.SplitSeq(buf.String(), "\n") {
+		fields := strings.Fields(line)
+		if len(fields) > 3 {
+			statuses[fields[0]] = fields[3]
+		}
+	}
+
+	assert.Equal(t, "current", statuses[oldest])
+	// Its NOT_AFTER was set when the newest version was added
+	assert.Equal(t, "expired", statuses[middle])
+	// It has no NOT_AFTER
+	assert.Equal(t, "inactive", statuses[newest])
+}
+
 func TestPromoteNonExistentVersion(t *testing.T) {
 	t.Parallel()
 

internal/secret/atomic.go

@@ -149,7 +149,9 @@ func WriteDir(fs afero.Fs, dir string, write func(dir string) error) error {
 
 // 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.
+// deleted. A crash part-way leaves only that temporary directory behind. If
+// only deleting the temporary directory fails, dir is already gone, and the
+// error names the temporary directory, which holds what is left of it.
 func RemoveDirAtomic(fs afero.Fs, dir string) error {
 	tmp, err := TempDirFor(fs, dir)
 	if err != nil {
@@ -165,7 +167,8 @@ func RemoveDirAtomic(fs afero.Fs, dir string) error {
 
 	err = fs.RemoveAll(tmp)
 	if err != nil {
-		return fmt.Errorf("failed to remove %s: %w", dir, err)
+		return fmt.Errorf("failed to remove %s after moving %s into it: %w",
+			tmp, dir, err)
 	}
 
 	return nil

internal/secret/crypto.go

@@ -15,10 +15,13 @@ import (
 
 var (
 	errNilPassphraseBuffer = errors.New("passphrase buffer is nil")
-	errNoTerminal          = errors.New("there is no terminal to ask on")
 	errNothingEntered      = errors.New("nothing was entered")
 )
 
+// ErrNoTerminal is wrapped in the error of ReadPassphrase and ReadMnemonic
+// when the process has no controlling terminal to ask on.
+var ErrNoTerminal = errors.New("there is no terminal to ask on")
+
 // ErrMnemonicNotRead is wrapped in every error of ReadMnemonic: there is no
 // terminal to read the mnemonic from, reading it failed, or it was empty.
 var ErrMnemonicNotRead = errors.New("failed to read mnemonic")
@@ -203,7 +206,7 @@ func readFromTerminal(
 
 		return nil, fmt.Errorf(
 			"%w: %w. Please set the %s environment variable or run interactively",
-			notRead, errNoTerminal, envVar)
+			notRead, ErrNoTerminal, envVar)
 	}
 	defer func() { _ = terminal.Close() }()
 

internal/secret/pgpunlock_test.go

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

internal/secret/pgpunlocker.go

@@ -192,58 +192,45 @@ func generatePGPUnlockerName() (string, error) {
 	return fmt.Sprintf("%s-pgp-%s", hostname, enrollmentTime), nil
 }
 
-// pgpUnlockerDir returns the current vault and the directory in it for a
-// new PGP unlocker, named after the host and the time.
-//
-//nolint:ireturn // the vault is only available behind VaultInterface
-func pgpUnlockerDir(
-	fs afero.Fs, stateDir string,
-) (VaultInterface, string, error) {
-	// Get current vault
-	vault, err := GetCurrentVault(fs, stateDir)
-	if err != nil {
-		return nil, "", fmt.Errorf("failed to get current vault: %w", err)
-	}
-
+// pgpUnlockerDir returns the directory in vault for a new PGP unlocker,
+// named after the host and the time.
+func pgpUnlockerDir(vault VaultInterface) (string, error) {
 	// Generate the unlocker name based on hostname and time
 	unlockerName, err := generatePGPUnlockerName()
 	if err != nil {
-		return nil, "", fmt.Errorf("failed to generate unlocker name: %w", err)
+		return "", fmt.Errorf("failed to generate unlocker name: %w", err)
 	}
 
 	vaultDir, err := vault.GetDirectory()
 	if err != nil {
-		return nil, "", fmt.Errorf("failed to get vault directory: %w", err)
+		return "", fmt.Errorf("failed to get vault directory: %w", err)
 	}
 
-	return vault, filepath.Join(vaultDir, "unlockers.d", unlockerName), nil
+	return filepath.Join(vaultDir, "unlockers.d", unlockerName), nil
 }
 
-// CreatePGPUnlocker creates a new PGP unlocker and stores it in the vault.
+// CreatePGPUnlocker creates a new PGP unlocker and stores it in vault.
 // It encrypts to the GPG key gpgKeyID and records fingerprint, that key's
 // fingerprint as ResolveGPGKeyFingerprint returns it, in the metadata.
 // Everything that can fail short of writing a file is done before anything
 // is written, and the files are written through WriteDir, so a failure
-// leaves no partial unlocker. The long-term key comes from mnemonic when it
-// is not nil, else from the current unlocker, which is given passphrase when
-// it is a passphrase unlocker.
+// leaves no partial unlocker. The long-term key comes from vault's
+// GetOrDeriveLongTermKey: the key it holds when it is unlocked, else the
+// key from its mnemonic or its current unlocker.
 func CreatePGPUnlocker(
-	fs afero.Fs, stateDir, gpgKeyID, fingerprint string,
-	mnemonic, passphrase *memguard.LockedBuffer,
+	vault VaultInterface, gpgKeyID, fingerprint string,
 ) (*PGPUnlocker, error) {
 	err := checkGPGAvailable()
 	if err != nil {
 		return nil, err
 	}
 
-	vault, unlockerDir, err := pgpUnlockerDir(fs, stateDir)
+	unlockerDir, err := pgpUnlockerDir(vault)
 	if err != nil {
 		return nil, err
 	}
 
-	// The vault's GetOrDeriveLongTermKey, in step 2, uses both
-	vault.SetMnemonic(mnemonic)
-	vault.SetUnlockPassphrase(passphrase)
+	fs := vault.GetFilesystem()
 
 	// Step 1: Generate a new age keypair for the PGP unlocker
 	ageIdentity, err := age.GenerateX25519Identity()
@@ -295,7 +282,7 @@ func CreatePGPUnlocker(
 func encryptPGPUnlockerKeys(
 	vault VaultInterface, ageIdentity *age.X25519Identity, gpgKeyID string,
 ) ([]byte, []byte, error) {
-	// From the mnemonic or the current unlocker, as for a passphrase unlocker
+	// The key vault holds, or else from its mnemonic or current unlocker
 	ltIdentity, err := vault.GetOrDeriveLongTermKey()
 	if err != nil {
 		return nil, nil, fmt.Errorf("failed to get long-term key: %w", err)
@@ -388,7 +375,8 @@ func ResolveGPGKeyFingerprint(keyID string) (string, error) {
 			return "", fmt.Errorf("%w: %s", ErrGPGKeyNotFound, keyID)
 		}
 
-		return "", fmt.Errorf("failed to resolve GPG key fingerprint: %w", err)
+		return "", fmt.Errorf(
+			"failed to resolve GPG key fingerprint: %w", withGPGStderr(err))
 	}
 
 	// Parse the output to extract the fingerprint
@@ -418,6 +406,24 @@ func checkGPGAvailable() error {
 	return nil
 }
 
+// withGPGStderr appends gpg's stderr, which says why gpg failed, to err
+// from cmd.Output. cmd.Output keeps that stderr in the *exec.ExitError.
+// err is returned unchanged when gpg wrote nothing to stderr.
+func withGPGStderr(err error) error {
+	var exitErr *exec.ExitError
+
+	if !errors.As(err, &exitErr) {
+		return err
+	}
+
+	stderr := strings.TrimSpace(string(exitErr.Stderr))
+	if stderr == "" {
+		return err
+	}
+
+	return fmt.Errorf("%w: %s", err, stderr)
+}
+
 // gpgEncryptDefault is the default implementation of GPG encryption
 func gpgEncryptDefault(data *memguard.LockedBuffer, keyID string) ([]byte, error) {
 	if data == nil {
@@ -437,7 +443,7 @@ func gpgEncryptDefault(data *memguard.LockedBuffer, keyID string) ([]byte, error
 
 	output, err := cmd.Output()
 	if err != nil {
-		return nil, fmt.Errorf("GPG encryption failed: %w", err)
+		return nil, fmt.Errorf("GPG encryption failed: %w", withGPGStderr(err))
 	}
 
 	return output, nil
@@ -450,7 +456,7 @@ func gpgDecryptDefault(encryptedData []byte) (*memguard.LockedBuffer, error) {
 
 	output, err := cmd.Output()
 	if err != nil {
-		return nil, fmt.Errorf("GPG decryption failed: %w", err)
+		return nil, fmt.Errorf("GPG decryption failed: %w", withGPGStderr(err))
 	}
 
 	// Create a secure buffer for the decrypted data

internal/secret/pgpunlocker_test.go

@@ -58,7 +58,8 @@ func TestCreatePGPUnlockerFailureWritesNothing(t *testing.T) {
 	}}
 
 	_, err = secret.CreatePGPUnlocker(
-		fs, testVaultStateDir, testGPGKeyID, testGPGFingerprint, nil, nil)
+		vault.NewVault(fs, testVaultStateDir, testVaultName),
+		testGPGKeyID, testGPGFingerprint)
 	require.Error(t, err)
 
 	vaultDir, err := vlt.GetDirectory()
@@ -87,18 +88,40 @@ func TestPGPUnlockerAddedTwiceKeepsFirst(t *testing.T) {
 
 	fs := afero.NewMemMapFs()
 	mnemonic := testMnemonicBuffer(t)
-	_, err := vault.CreateVault(fs, testVaultStateDir, testVaultName, mnemonic, nil)
+	vlt, err := vault.CreateVault(fs, testVaultStateDir, testVaultName, mnemonic, nil)
 	require.NoError(t, err)
 
-	first, err := secret.CreatePGPUnlocker(
-		fs, testVaultStateDir, testGPGKeyID, testGPGFingerprint, mnemonic, nil)
+	first, err := secret.CreatePGPUnlocker(vlt, testGPGKeyID, testGPGFingerprint)
 	require.NoError(t, err)
 
 	firstFiles := dirFiles(t, fs, first.GetDirectory())
 
-	second, err := secret.CreatePGPUnlocker(
-		fs, testVaultStateDir, testGPGKeyID, testGPGFingerprint, mnemonic, nil)
+	second, err := secret.CreatePGPUnlocker(vlt, testGPGKeyID, testGPGFingerprint)
 	require.NoError(t, err)
 	assert.NotEqual(t, first.GetDirectory(), second.GetDirectory())
 	assert.Equal(t, firstFiles, dirFiles(t, fs, first.GetDirectory()))
 }
+
+// TestGPGDecryptErrorHasGPGReason runs the real gpg on data that is not an
+// OpenPGP message and checks that the error carries the reason gpg writes
+// to stderr.
+func TestGPGDecryptErrorHasGPGReason(t *testing.T) {
+	// The empty GNUPGHOME keeps gpg away from the user's own keyring;
+	// LC_ALL=C keeps gpg's message in English.
+	t.Setenv("GNUPGHOME", t.TempDir())
+	t.Setenv("LC_ALL", "C")
+
+	_, err := secret.GPGDecryptFunc([]byte("not an OpenPGP message"))
+	require.ErrorContains(t, err, "gpg: no valid OpenPGP data found")
+}
+
+// TestGPGDecryptErrorWithoutGPGStderr checks that when gpg fails without
+// writing to stderr, the error ends with the exit status and nothing after it.
+//
+//nolint:paralleltest // installFakeGPG uses t.Setenv
+func TestGPGDecryptErrorWithoutGPGStderr(t *testing.T) {
+	installFakeGPG(t)
+
+	_, err := secret.GPGDecryptFunc([]byte("not an OpenPGP message"))
+	require.EqualError(t, err, "GPG decryption failed: exit status 1")
+}

internal/secret/secret.go

@@ -28,11 +28,6 @@ type VaultInterface interface {
 	GetFilesystem() afero.Fs
 	GetCurrentUnlocker() (Unlocker, error)
 	GetOrDeriveLongTermKey() (*age.X25519Identity, error)
-	// SetMnemonic and SetUnlockPassphrase give GetOrDeriveLongTermKey the
-	// mnemonic to derive the long-term key from, and the passphrase for a
-	// current passphrase unlocker; nil for none.
-	SetMnemonic(mnemonic *memguard.LockedBuffer)
-	SetUnlockPassphrase(passphrase *memguard.LockedBuffer)
 	CreatePassphraseUnlocker(
 		passphrase *memguard.LockedBuffer) (*PassphraseUnlocker, error)
 }

internal/secret/secret_test.go

@@ -118,12 +118,6 @@ func (m *MockVault) GetOrDeriveLongTermKey() (*age.X25519Identity, error) {
 	return nil, errNotImplementedInMock
 }
 
-func (m *MockVault) SetMnemonic(mnemonic *memguard.LockedBuffer) {
-	m.mnemonic = mnemonic
-}
-
-func (m *MockVault) SetUnlockPassphrase(_ *memguard.LockedBuffer) {}
-
 func (m *MockVault) CreatePassphraseUnlocker(
 	_ *memguard.LockedBuffer,
 ) (*PassphraseUnlocker, error) {

internal/secret/version.go

@@ -32,7 +32,8 @@ type VersionMetadata struct {
 	ID        string     `json:"id"`                  // ULID
 	CreatedAt *time.Time `json:"createdAt,omitempty"` // When version was created
 	NotBefore *time.Time `json:"notBefore,omitempty"` // When this version becomes active
-	NotAfter  *time.Time `json:"notAfter,omitempty"`  // Expiry (nil = current)
+	// NotAfter is set when another version is added while this one is current
+	NotAfter *time.Time `json:"notAfter,omitempty"`
 }
 
 // Version represents a version of a secret

internal/secret/version_test.go

@@ -90,10 +90,6 @@ func (m *MockVersionVault) GetOrDeriveLongTermKey() (*age.X25519Identity, error)
 	return nil, errNotImplementedInMock
 }
 
-func (m *MockVersionVault) SetMnemonic(_ *memguard.LockedBuffer) {}
-
-func (m *MockVersionVault) SetUnlockPassphrase(_ *memguard.LockedBuffer) {}
-
 func (m *MockVersionVault) CreatePassphraseUnlocker(
 	_ *memguard.LockedBuffer,
 ) (*secret.PassphraseUnlocker, error) {

internal/vault/errors_test.go

@@ -132,7 +132,7 @@ func TestGetSecretWithWrongMnemonic(t *testing.T) {
 	t.Cleanup(mnemonic.Destroy)
 
 	vlt := vault.NewVault(created.GetFilesystem(), testStateDir, testVaultName)
-	vlt.SetMnemonic(mnemonic)
+	vlt.Mnemonic = mnemonic
 
 	_, err := vlt.GetSecret(testSecretName)
 	require.ErrorIs(t, err, vault.ErrMnemonicMismatch)

internal/vault/management.go

@@ -55,7 +55,10 @@ func ValidateVaultName(name string) error {
 }
 
 // ResolveVaultSymlink reads the currentvault file to get the path to the current vault
-// The file contains just the vault name (e.g., "default")
+// The file contains just the vault name (e.g., "default"). An empty or
+// invalid name, or the name of a vault that does not exist, is an error
+// wrapping ErrVaultNotFound that names the file and says to run
+// 'secret vault select <name>'.
 func ResolveVaultSymlink(fs afero.Fs, currentVaultPath string) (string, error) {
 	secret.Debug("resolveVaultSymlink starting", "path", currentVaultPath)
 
@@ -74,6 +77,21 @@ func ResolveVaultSymlink(fs afero.Fs, currentVaultPath string) (string, error) {
 	stateDir := filepath.Dir(currentVaultPath)
 	absolutePath := filepath.Join(stateDir, "vaults.d", vaultName)
 
+	// An empty name would make vaults.d itself the vault's directory
+	exists := false
+	if isValidVaultName(vaultName) {
+		exists, err = afero.DirExists(fs, absolutePath)
+		if err != nil {
+			return "", fmt.Errorf("failed to check if vault exists: %w", err)
+		}
+	}
+
+	if !exists {
+		return "", fmt.Errorf("vault '%s' named in %s %w; run "+
+			"'secret vault select <name>' to select a vault",
+			vaultName, currentVaultPath, ErrVaultNotFound)
+	}
+
 	secret.Debug("Resolved to absolute path", "absolute_path", absolutePath)
 
 	return absolutePath, nil
@@ -140,8 +158,9 @@ func ListVaults(fs afero.Fs, stateDir string) ([]string, error) {
 		return nil, fmt.Errorf("failed to read vaults directory: %w", err)
 	}
 
-	// Extract vault names
-	var vaults []string
+	// Extract vault names. Not nil, so that no vaults prints [] in JSON,
+	// not null.
+	vaults := make([]string, 0, len(entries))
 
 	for _, entry := range entries {
 		if entry.IsDir() {
@@ -216,9 +235,9 @@ func processMnemonicForVault(
 // vault: one wrapping ErrInvalidVaultName when it is not a valid vault name,
 // and one wrapping ErrVaultExists when that vault already exists. Creating an
 // existing vault again would replace its keys, and its secrets could no
-// longer be decrypted. CreateVault checks this before writing anything; the
-// commands that create a vault also check it before they ask for the
-// mnemonic or passphrase, holding the state directory lock.
+// longer be decrypted. The commands that create a vault check it before they
+// ask for the mnemonic or passphrase, and CreateVault checks it again, under
+// the state directory lock, before writing anything.
 func CheckNewVaultName(fs afero.Fs, stateDir string, name string) error {
 	err := ValidateVaultName(name)
 	if err != nil {

internal/vault/secrets_version_test.go

@@ -336,7 +336,7 @@ func TestVaultVersionTimestampsAfterUnlock(t *testing.T) {
 
 	// A new instance is locked; AddSecret unlocks it with the passphrase
 	vault := NewVault(fs, testStateDir, "test")
-	vault.SetUnlockPassphrase(passphrase)
+	vault.UnlockPassphrase = passphrase
 
 	addTestSecretToVault(t, vault, testSecretPath, []byte("version-2"), true)
 	require.False(t, fs.readAt.IsZero(), "AddSecret did not unlock the vault")

internal/vault/vault.go

@@ -64,18 +64,6 @@ func (v *Vault) ClearLongTermKey() {
 	v.longTermKey = nil
 }
 
-// SetMnemonic sets v.Mnemonic, for code that has v only as a
-// secret.VaultInterface.
-func (v *Vault) SetMnemonic(mnemonic *memguard.LockedBuffer) {
-	v.Mnemonic = mnemonic
-}
-
-// SetUnlockPassphrase sets v.UnlockPassphrase, for code that has v only as
-// a secret.VaultInterface.
-func (v *Vault) SetUnlockPassphrase(passphrase *memguard.LockedBuffer) {
-	v.UnlockPassphrase = passphrase
-}
-
 // GetOrDeriveLongTermKey gets the long-term key from memory or derives it
 // from available sources
 func (v *Vault) GetOrDeriveLongTermKey() (*age.X25519Identity, error) {

internal/vault/vault_error_test.go

@@ -128,7 +128,7 @@ func TestAddSecretForceWithWrongPassphraseChangesNothing(t *testing.T) {
 	t.Cleanup(wrong.Destroy)
 
 	vlt := vault.NewVault(fs, testStateDir, testVaultName)
-	vlt.SetUnlockPassphrase(wrong)
+	vlt.UnlockPassphrase = wrong
 
 	secretDir := filepath.Join(testStateDir, "vaults.d", testVaultName,
 		"secrets.d", testSecretName)