Use gpg.conf's default-key when unlocker add pgp has no --keyid (closes #141)

getDefaultGPGKey took the default-key value from `gpgconf --list-options
gpg` as printed. gpgconf prints a string value with a leading `"` and
percent-escaped, so gpg rejected the key ID. The parsing moved into
parseDefaultGPGKey, which 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.

Rule suppressed: gosec G703 on the test's write of gpg.conf into the
GNUPGHOME directory newTestGPGKey made.

Model: opus-5-5

README.md

@@ -241,14 +241,6 @@ that the vault then opens only with its mnemonic (see
 [Confirmation Before Removal](#confirmation-before-removal)). An unlocker
 directory that `secret unlocker list` skips with a warning, because its metadata
 cannot be read or parsed, is removed by the directory name the warning gives.
-Removing the current unlocker makes the first of the vault's other unlockers
-that `secret unlocker select` accepts, in the order `secret unlocker list` shows
-them, the current one, and says which; when there is none, the vault is left
-without a current unlocker. `secret unlocker select` refuses an unlocker whose
-type is not `passphrase` or `pgp`. When `current-unlocker` names an unlocker
-that does not exist and the vault has another that `secret unlocker select`
-accepts, opening the vault fails with an error that says to select one with
-`secret unlocker select`.
 
 - `--force, -f`: Remove without asking, even the last unlocker
 - **CRITICAL WARNING**: Without unlockers and without your mnemonic phrase,

TODO.md

@@ -24,19 +24,6 @@ https://git.eeqj.de/sneak/secret/milestone/12
   leading `"` and percent-escaped, and was passed to gpg undecoded, so gpg
   rejected it. The value is now decoded first. With no `default-key` set, the
   first secret key is still used.
-- 2026-10-14: Removing the current unlocker no longer leaves the vault unable to
-  open through the unlockers it still has
-  (https://git.eeqj.de/sneak/secret/issues/137). `current-unlocker` went on
-  naming the removed unlocker, so every command that opened the vault failed,
-  with advice to use the mnemonic. `secret unlocker remove` of the current
-  unlocker now makes the first of the vault's other unlockers that
-  `secret unlocker select` accepts, in the order `secret unlocker list` shows,
-  current before removing it, and says so; when there is none, it also removes
-  `current-unlocker`. `vault.RemoveUnlocker` returns the ID of the unlocker it
-  made current. When `current-unlocker` names an unlocker that does not exist
-  and the vault has another that `secret unlocker select` accepts, the error
-  wraps `vault.ErrUnlockerNotFound` and says to run `secret unlocker select`,
-  without the advice to use the mnemonic.
 - 2026-10-14: A `secret add --force` that cannot unlock the vault, given a wrong
   passphrase or mnemonic, leaves the secret as it was
   (https://git.eeqj.de/sneak/secret/issues/136). The new version used to be

internal/cli/unlock_failure_test.go

@@ -4,11 +4,9 @@
 // file the unlocker needs is missing or the passphrase is wrong, the error
 // keeps its cause and ends by saying that the mnemonic still opens that
 // vault, but only for a vault that the mnemonic does open, and not when the
-// passphrase could not be read at all. When the current unlocker does not
+// passphrase could not be read at all. When a secret's current file is
-// exist and the vault has another that `secret unlocker select` accepts, the
+// missing, the error says how to make a version current again. Each test
-// error says to select one instead.
+// that pins such advice also follows it.
-// When a secret's current file is missing, the error says how to make a
-// version current again. Each test that pins such advice also follows it.
 
 package cli_test
 
@@ -224,69 +222,6 @@ func TestMoveUnlockFailureNamesVault(t *testing.T) {
 	assert.Equal(t, "value", getSecret(t, c, "y"))
 }
 
-// TestMissingCurrentUnlockerNamesSelect checks the error of `secret get`
-// when current-unlocker names an unlocker that does not exist and the vault
-// has another: it names the missing unlocker and says to select another
-// one, without the advice to use the mnemonic. The test then follows that
-// advice.
-func TestMissingCurrentUnlockerNamesSelect(t *testing.T) {
-	t.Parallel()
-
-	fs := newTwoVaultFs(t)
-	unlockerDir, c := newUnlockTestCLI(t, fs)
-
-	require.NoError(t, afero.WriteFile(fs,
-		filepath.Join(unlockTestVaultDir, "current-unlocker"),
-		[]byte("missing"), secret.FilePerms))
-
-	err := c.GetSecret(discardCmd(), "x")
-
-	require.ErrorIs(t, err, vault.ErrUnlockerNotFound)
-	require.EqualError(t, err, "failed to unlock vault: "+
-		"failed to get long-term key: failed to get current unlocker: "+
-		"unlocker with ID missing not found; the vault 'default' has other "+
-		"unlockers: run 'secret unlocker select' with the ID of one of them "+
-		"to make it the current unlocker")
-
-	require.NoError(t, c.UnlockerSelect(filepath.Base(unlockerDir)))
-	assert.Equal(t, "value", getSecret(t, c, "x"))
-}
-
-// TestMissingCurrentUnlockerNamesMnemonic checks the error of `secret get`
-// when the unlocker that current-unlocker names does not exist and the
-// vault's only other unlocker is the directory "odd", whose metadata names a
-// type that `secret unlocker select` refuses: the error ends with the advice
-// that the mnemonic still opens the vault, not with advice to select another
-// unlocker. The test then follows that advice.
-func TestMissingCurrentUnlockerNamesMnemonic(t *testing.T) {
-	t.Parallel()
-
-	fs := newTwoVaultFs(t)
-	unlockerDir, c := newUnlockTestCLI(t, fs)
-
-	require.NoError(t, fs.RemoveAll(unlockerDir))
-	require.NoError(t, afero.WriteFile(fs,
-		filepath.Join(unlockTestVaultDir, "unlockers.d", "odd",
-			"unlocker-metadata.json"),
-		[]byte(`{"type": "odd"}`), secret.FilePerms))
-
-	err := c.GetSecret(discardCmd(), "x")
-
-	var cause *os.PathError
-
-	require.ErrorAs(t, err, &cause)
-	require.NotErrorIs(t, err, vault.ErrUnlockerNotFound)
-	require.EqualError(t, err, "failed to unlock vault: "+
-		"failed to get long-term key: failed to get current unlocker: "+
-		"failed to read unlocker metadata: "+cause.Error()+mnemonicAdvice)
-
-	c.Mnemonic = testMnemonicBuffer(t)
-	require.NoError(t, c.UnlockersAdd("passphrase", discardCmd()))
-
-	c.Mnemonic = nil
-	assert.Equal(t, "value", getSecret(t, c, "x"))
-}
-
 // TestPassphraseNotReadNamesNoMnemonic runs `secret get x` on the built
 // binary without SB_UNLOCK_PASSPHRASE and with a stdin that is not a
 // terminal, so the passphrase cannot be read. The unlocker was not tried,

internal/cli/unlockers.go

@@ -245,11 +245,8 @@ func newUnlockerRemoveCmd() *cobra.Command {
 		Long: `Remove an unlocker from the current vault. Asks for ` +
 			`confirmation first, saying whether it is the vault's last ` +
 			`unlocker; when stdin is not a terminal, fails unless --force ` +
-			`is given. Removing the current unlocker makes another of the ` +
+			`is given. Warning: Without unlockers and without your ` +
-			`vault's unlockers current, if it has one that ` +
+			`mnemonic, vault data will be permanently inaccessible.`,
-			`'secret unlocker select' accepts. Warning: Without ` +
-			`unlockers and without your mnemonic, vault data will be ` +
-			`permanently inaccessible.`,
 		Args:              cobra.ExactArgs(1),
 		ValidArgsFunction: getUnlockerIDsCompletionFunc(cli.fs, cli.stateDir),
 		RunE: func(cmd *cobra.Command, args []string) error {
@@ -638,17 +635,13 @@ func (cli *Instance) removeUnlocker(
 			"have your mnemonic phrase to access this vault again!")
 	}
 
-	newCurrentID, err := found.vlt.RemoveUnlocker(unlockerID)
+	err := found.vlt.RemoveUnlocker(unlockerID)
 	if err != nil {
 		return err
 	}
 
 	cmd.Printf("Removed unlocker '%s'\n", unlockerID)
 
-	if newCurrentID != "" {
-		cmd.Printf("Switched current unlocker to '%s'\n", newCurrentID)
-	}
-
 	return nil
 }
 

internal/cli/unlockers_remove_test.go

@@ -1,94 +0,0 @@
-//nolint:testpackage // white-box test of unexported internals
-package cli
-
-import (
-	"bytes"
-	"path/filepath"
-	"testing"
-
-	"github.com/awnumar/memguard"
-	"github.com/spf13/afero"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/secret/internal/secret"
-	"sneak.berlin/go/secret/internal/vault"
-)
-
-// TestUnlockerRemoveCurrent is a regression test for
-// https://git.eeqj.de/sneak/secret/issues/137. A vault has a passphrase
-// unlocker and a PGP unlocker, the current one. Removing the PGP unlocker
-// makes the passphrase unlocker current and says so, and the vault then
-// opens through it. Removing the passphrase unlocker too removes
-// current-unlocker.
-//
-// The second case also gives the vault the unlocker directory "odd", whose
-// metadata names a type that `secret unlocker select` refuses, and which
-// sorts before the passphrase unlocker: neither removal makes it current.
-//
-//nolint:paralleltest // t.Setenv (GNUPGHOME) forbids parallel tests
-func TestUnlockerRemoveCurrent(t *testing.T) {
-	tests := []struct {
-		name    string
-		withOdd bool
-	}{
-		{name: "two unlockers", withOdd: false},
-		{name: "and the directory odd", withOdd: true},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			newTestGPGKey(t)
-
-			passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
-			t.Cleanup(passphrase.Destroy)
-
-			fs := afero.NewMemMapFs()
-			vlt, err := vault.CreateVault(fs, listTestStateDir,
-				listTestVaultName, testMnemonicBuffer(t), nil)
-			require.NoError(t, err)
-
-			passphraseUnlocker, err := vlt.CreatePassphraseUnlocker(passphrase)
-			require.NoError(t, err)
-
-			if tt.withOdd {
-				require.NoError(t, afero.WriteFile(fs,
-					filepath.Join(testVaultDir(listTestVaultName), "unlockers.d",
-						"odd", "unlocker-metadata.json"),
-					[]byte(`{"type": "odd"}`), secret.FilePerms))
-			}
-
-			instance, cmd := newTestInstance(fs)
-			instance.UnlockPassphrase = passphrase
-
-			cmd.Flags().String("keyid", unreadableTestGPGUserID, "")
-			require.NoError(t, instance.UnlockersAdd(unlockerTypePGP, cmd))
-
-			pgpUnlocker, err := vlt.GetCurrentUnlocker()
-			require.NoError(t, err)
-			require.Equal(t, unlockerTypePGP, pgpUnlocker.GetType())
-
-			var output bytes.Buffer
-
-			cmd.SetOut(&output)
-			require.NoError(t,
-				instance.UnlockersRemove(pgpUnlocker.GetID(), true, cmd))
-			assert.Contains(t, output.String(),
-				"Switched current unlocker to '"+passphraseUnlocker.GetID()+"'")
-
-			reopened := vault.NewVault(fs, listTestStateDir, listTestVaultName)
-			reopened.UnlockPassphrase = passphrase
-
-			_, err = reopened.GetOrDeriveLongTermKey()
-			require.NoError(t, err)
-
-			require.NoError(t,
-				instance.UnlockersRemove(passphraseUnlocker.GetID(), true, cmd))
-
-			exists, err := afero.Exists(fs, filepath.Join(
-				testVaultDir(listTestVaultName), "current-unlocker"))
-			require.NoError(t, err)
-			assert.False(t, exists,
-				"current-unlocker is left after the passphrase unlocker")
-		})
-	}
-}

internal/vault/errors_test.go

@@ -81,9 +81,7 @@ func TestVaultErrors(t *testing.T) {
 			return vlt.CopySecretAllVersions(vlt, versionless, "copy", false)
 		}, vault.ErrNoVersions},
 		{"remove a missing unlocker", func(vlt *vault.Vault) error {
-			_, err := vlt.RemoveUnlocker(missingName)
+			return vlt.RemoveUnlocker(missingName)
-
-			return err
 		}, vault.ErrUnlockerNotFound},
 		{"select a missing unlocker", func(vlt *vault.Vault) error {
 			return vlt.SelectUnlocker(missingName)

internal/vault/unlockers.go

@@ -5,10 +5,8 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
-	"maps"
 	"os"
 	"path/filepath"
-	"slices"
 	"strings"
 	"time"
 
@@ -21,7 +19,6 @@ import (
 // Unlocker metadata type strings.
 const (
 	unlockerTypePassphrase = "passphrase"
-	unlockerTypePGP        = "pgp"
 )
 
 // GetCurrentUnlocker returns the current unlocker for this vault
@@ -55,11 +52,6 @@ func (v *Vault) GetCurrentUnlocker() (secret.Unlocker, error) {
 		return nil, err
 	}
 
-	err = v.checkCurrentUnlockerExists(unlockerDir)
-	if err != nil {
-		return nil, err
-	}
-
 	secret.DebugWith("Resolved unlocker directory",
 		slog.String("unlocker_dir", unlockerDir),
 		slog.String("vault_name", v.Name),
@@ -82,7 +74,7 @@ func (v *Vault) GetCurrentUnlocker() (secret.Unlocker, error) {
 		passphraseUnlocker := secret.NewPassphraseUnlocker(v.fs, unlockerDir, metadata)
 		passphraseUnlocker.Passphrase = v.UnlockPassphrase
 		unlocker = passphraseUnlocker
-	case unlockerTypePGP:
+	case "pgp":
 		secret.Debug("Creating PGP unlocker instance", "unlocker_type", metadata.Type)
 
 		unlocker = secret.NewPGPUnlocker(v.fs, unlockerDir, metadata)
@@ -127,37 +119,6 @@ func (v *Vault) resolveUnlockerDirectory(currentUnlockerPath string) (string, er
 	return absolutePath, nil
 }
 
-// checkCurrentUnlockerExists fails with ErrUnlockerNotFound, saying to
-// select another unlocker, when unlockerDir, the unlocker directory that
-// current-unlocker names, does not exist and the vault has another unlocker
-// that SelectUnlocker accepts. When it has none, it returns nil: reading the
-// missing unlocker then fails, with the advice that the mnemonic still opens
-// the vault.
-func (v *Vault) checkCurrentUnlockerExists(unlockerDir string) error {
-	exists, err := afero.DirExists(v.fs, unlockerDir)
-	if err != nil {
-		return fmt.Errorf("failed to check for the current unlocker: %w", err)
-	}
-
-	if exists {
-		return nil
-	}
-
-	otherID, err := v.firstOtherUnlockerID(filepath.Base(unlockerDir))
-	if err != nil {
-		return err
-	}
-
-	if otherID == "" {
-		return nil
-	}
-
-	return fmt.Errorf("unlocker with ID %s %w; the vault '%s' has other "+
-		"unlockers: run %s with the ID of one of them to make it the current "+
-		"unlocker", filepath.Base(unlockerDir), ErrUnlockerNotFound, v.Name,
-		v.withVaultSelect("'secret unlocker select'"))
-}
-
 // findUnlockerByID finds an unlocker by its ID and returns the unlocker
 // instance and its directory path. A directory that ListUnlockers skips is
 // skipped here too, with the same warning. Such a directory has no ID: if
@@ -198,7 +159,7 @@ func (v *Vault) findUnlockerByID(
 		switch metadata.Type {
 		case unlockerTypePassphrase:
 			tempUnlocker = secret.NewPassphraseUnlocker(v.fs, unlockerDirPath, metadata)
-		case unlockerTypePGP:
+		case "pgp":
 			tempUnlocker = secret.NewPGPUnlocker(v.fs, unlockerDirPath, metadata)
 		default:
 			continue
@@ -321,15 +282,10 @@ func (v *Vault) HasUnlocker(unlockerID string) (bool, error) {
 // RemoveUnlocker removes an unlocker from this vault. An unlocker
 // directory that ListUnlockers skips is removed by its directory name; its
 // type is unknown, so only the directory is removed.
-//
+func (v *Vault) RemoveUnlocker(unlockerID string) error {
-// It returns the ID of the unlocker it made current, or "" when it made none
-// current. If the removed unlocker is the current one, the unlocker
-// firstOtherUnlockerID returns is made current before the removal; with
-// none, current-unlocker is removed after it.
-func (v *Vault) RemoveUnlocker(unlockerID string) (string, error) {
 	vaultDir, err := v.GetDirectory()
 	if err != nil {
-		return "", err
+		return err
 	}
 
 	// Find the unlocker directory and create the unlocker instance
@@ -338,92 +294,19 @@ func (v *Vault) RemoveUnlocker(unlockerID string) (string, error) {
 	// Find the unlocker by ID
 	unlocker, unlockerDir, err := v.findUnlockerByID(unlockersDir, unlockerID)
 	if err != nil {
-		return "", err
+		return err
 	}
 
 	if unlockerDir == "" {
-		return "", fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound)
+		return fmt.Errorf("unlocker with ID %s %w", unlockerID, ErrUnlockerNotFound)
-	}
-
-	currentUnlockerPath := filepath.Join(vaultDir, "current-unlocker")
-
-	currentDir, err := v.resolveUnlockerDirectory(currentUnlockerPath)
-	if err != nil && !errors.Is(err, os.ErrNotExist) {
-		return "", err
-	}
-
-	isCurrent := currentDir == unlockerDir
-	newCurrentID := ""
-
-	if isCurrent {
-		newCurrentID, err = v.selectOtherUnlocker(unlockerID)
-		if err != nil {
-			return "", err
-		}
 	}
 
 	if unlocker == nil {
-		err = secret.RemoveDirAtomic(v.fs, unlockerDir)
+		return secret.RemoveDirAtomic(v.fs, unlockerDir)
-	} else {
-		err = unlocker.Remove()
 	}
 
-	if err != nil {
+	// Use the unlocker's Remove method
-		return "", err
+	return unlocker.Remove()
-	}
-
-	if isCurrent && newCurrentID == "" {
-		err = v.fs.Remove(currentUnlockerPath)
-		if err != nil {
-			return "", fmt.Errorf("failed to remove current-unlocker file: %w", err)
-		}
-	}
-
-	return newCurrentID, nil
-}
-
-// selectOtherUnlocker makes the unlocker firstOtherUnlockerID returns for
-// unlockerID the current unlocker and returns its ID. It returns "" when
-// there is no such unlocker.
-func (v *Vault) selectOtherUnlocker(unlockerID string) (string, error) {
-	otherID, err := v.firstOtherUnlockerID(unlockerID)
-	if err != nil {
-		return "", err
-	}
-
-	if otherID == "" {
-		return "", nil
-	}
-
-	err = v.SelectUnlocker(otherID)
-	if err != nil {
-		return "", err
-	}
-
-	return otherID, nil
-}
-
-// firstOtherUnlockerID returns the ID of the vault's first unlocker other
-// than unlockerID, in ID order, the order `secret unlocker list` shows, that
-// SelectUnlocker accepts, or "" when there is none. SelectUnlocker refuses
-// an unlocker whose metadata names a type other than passphrase or pgp,
-// which ListUnlockers still returns.
-func (v *Vault) firstOtherUnlockerID(unlockerID string) (string, error) {
-	unlockers, err := v.ListUnlockers()
-	if err != nil {
-		return "", err
-	}
-
-	for _, id := range slices.Sorted(maps.Keys(unlockers)) {
-		unlockerType := unlockers[id].Type
-
-		if id != unlockerID && (unlockerType == unlockerTypePassphrase ||
-			unlockerType == unlockerTypePGP) {
-			return id, nil
-		}
-	}
-
-	return "", nil
 }
 
 // SelectUnlocker selects an unlocker as current for this vault

internal/vault/vault.go

@@ -291,11 +291,9 @@ func (v *Vault) unlockLongTermKey(
 // without a mnemonic records none, and without its metadata the key cannot
 // be derived. It is not added when the passphrase could not be read: the
 // unlocker was not tried, and adding one would need a passphrase read the
-// same way. Nor is it added when the current unlocker does not exist and the
+// same way.
-// vault has others: the error already says to select one of them.
 func (v *Vault) withMnemonicAdvice(err error) error {
-	if errors.Is(err, secret.ErrPassphraseNotRead) ||
+	if errors.Is(err, secret.ErrPassphraseNotRead) {
-		errors.Is(err, ErrUnlockerNotFound) {
 		return err
 	}
 
@@ -306,21 +304,15 @@ func (v *Vault) withMnemonicAdvice(err error) error {
 		return err
 	}
 
-	return fmt.Errorf("%w; the vault '%s' still opens with its mnemonic: run "+
+	// 'secret unlocker add' acts on the current vault only.
-		"%s with %s set to the mnemonic to give it a new unlocker",
+	steps := "'secret unlocker add passphrase'"
-		err, v.Name, v.withVaultSelect("'secret unlocker add passphrase'"),
-		secret.EnvMnemonic)
-}
 
-// withVaultSelect returns command, a quoted 'secret unlocker' command, for
+	current, currentErr := GetCurrentVault(v.fs, v.stateDir)
-// advice about this vault. Such a command acts on the current vault only,
+	if currentErr != nil || current.Name != v.Name {
-// so when this vault is not the current one, the command that selects it
+		steps = fmt.Sprintf("'secret vault select %s', then %s", v.Name, steps)
-// comes first.
-func (v *Vault) withVaultSelect(command string) string {
-	current, err := GetCurrentVault(v.fs, v.stateDir)
-	if err != nil || current.Name != v.Name {
-		return fmt.Sprintf("'secret vault select %s', then %s", v.Name, command)
 	}
 
-	return command
+	return fmt.Errorf("%w; the vault '%s' still opens with its mnemonic: run "+
+		"%s with %s set to the mnemonic to give it a new unlocker",
+		err, v.Name, steps, secret.EnvMnemonic)
 }