Skip corrupt unlocker metadata in unlocker select and remove (closes #72)

findUnlockerByID failed on the first unlocker directory whose metadata
could not be checked, read or parsed, so `secret unlocker select` and
`remove` failed when one sorted before the unlocker asked for. It now
skips such a directory with the warning ListUnlockers gives, through
the code both now share. A skipped directory is removed by its
directory name, with RemoveDirAtomic, and cannot be selected.
`unlocker remove` applies the last-unlocker check only when the ID is
that of the single listed unlocker, so removing a skipped directory is
never refused as removing the last one.

Model: opus-5-5

internal/cli/unlockers.go

@@ -744,14 +744,33 @@ func (cli *Instance) removeUnlocker(
 		return err
 	}
 
-	// Get list of unlockers
+	// Get list of unlockers. It leaves out a directory whose metadata
+	// cannot be read or parsed: that unlocker does not work, so removing it
+	// by its directory name never removes the last unlocker.
 	unlockers, err := vlt.ListUnlockers()
 	if err != nil {
 		return fmt.Errorf("failed to list unlockers: %w", err)
 	}
 
 	// Check if we're removing the last unlocker
+	removingLast := false
+
 	if len(unlockers) == 1 {
+		vaultDir, err := vlt.GetDirectory()
+		if err != nil {
+			return fmt.Errorf("failed to get vault directory: %w", err)
+		}
+
+		lastID, err := findUnlockerIDByMetadata(cli.fs,
+			filepath.Join(vaultDir, "unlockers.d"), unlockers[0], true)
+		if err != nil {
+			return err
+		}
+
+		removingLast = lastID == unlockerID
+	}
+
+	if removingLast {
 		// Check if vault has secrets
 		numSecrets, err := vlt.NumSecrets()
 		if err != nil {

internal/cli/unlockers_corrupt_test.go

@@ -0,0 +1,112 @@
+// Corrupt Unlocker Tests
+//
+// `secret unlocker select` and `secret unlocker remove` find an unlocker
+// by its ID. These tests give the first unlocker, which sorts before the
+// one the commands act on, metadata that is not JSON, and check that the
+// commands step past it, and that it can itself be removed by its
+// directory name, which `secret unlocker list` names in its warning.
+
+//nolint:testpackage // white-box test of unexported internals
+package cli
+
+import (
+	"path/filepath"
+	"testing"
+
+	"git.eeqj.de/sneak/secret/internal/vault"
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// newCorruptUnlockerVault returns the two-unlocker test vault with the
+// metadata of the first unlocker replaced by text that is not JSON.
+func newCorruptUnlockerVault(t *testing.T) *afero.MemMapFs {
+	t.Helper()
+
+	fs := newListTestVault(t, 2)
+	require.NoError(t, afero.WriteFile(fs,
+		filepath.Join(testVaultDir(listTestVaultName), listTestUnlockersDirName,
+			listTestUnlockerDirOne, listTestMetadataFileName),
+		[]byte("not json"), listTestFilePerm))
+
+	return fs
+}
+
+// TestUnlockerSelectSkipsCorruptUnlocker asserts that the second unlocker
+// can be selected, and that the corrupt one, having no type to be used as,
+// cannot be selected by its directory name.
+func TestUnlockerSelectSkipsCorruptUnlocker(t *testing.T) {
+	t.Parallel()
+
+	fs := newCorruptUnlockerVault(t)
+	instance, _ := newTestInstance(fs)
+
+	require.NoError(t, instance.UnlockerSelect("pgp-"+listTestGPGKeyID+"B"))
+
+	current, err := afero.ReadFile(fs,
+		filepath.Join(testVaultDir(listTestVaultName), "current-unlocker"))
+	require.NoError(t, err)
+	assert.Equal(t, listTestUnlockerDirTwo, string(current))
+
+	err = instance.UnlockerSelect(listTestUnlockerDirOne)
+	require.ErrorIs(t, err, vault.ErrUnlockerNotFound)
+}
+
+// TestUnlockerRemoveWithCorruptUnlocker asserts that the second unlocker
+// can be removed, unless the vault holds secrets: the corrupt unlocker
+// cannot unlock the vault, so the second is its last. The corrupt one can
+// be removed by its directory name without --force even then.
+func TestUnlockerRemoveWithCorruptUnlocker(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name        string
+		unlockerID  string
+		withSecret  bool
+		wantErr     error
+		wantEntries []string
+	}{
+		{
+			name:        "the other unlocker",
+			unlockerID:  "pgp-" + listTestGPGKeyID + "B",
+			wantEntries: []string{listTestUnlockerDirOne},
+		},
+		{
+			name:       "the other unlocker, the last one, with secrets",
+			unlockerID: "pgp-" + listTestGPGKeyID + "B",
+			withSecret: true,
+			wantErr:    errLastUnlocker,
+			wantEntries: []string{
+				listTestUnlockerDirOne, listTestUnlockerDirTwo,
+			},
+		},
+		{
+			name:        "the corrupt unlocker by its directory name",
+			unlockerID:  listTestUnlockerDirOne,
+			withSecret:  true,
+			wantEntries: []string{listTestUnlockerDirTwo},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			fs := newCorruptUnlockerVault(t)
+			if tt.withSecret {
+				writeTestSecret(t, fs, testVaultDir(listTestVaultName))
+			}
+
+			instance, cmd := newTestInstance(fs)
+
+			err := instance.UnlockersRemove(tt.unlockerID, false, cmd)
+			require.ErrorIs(t, err, tt.wantErr)
+
+			assertDirEntries(t, fs,
+				filepath.Join(testVaultDir(listTestVaultName),
+					listTestUnlockersDirName),
+				tt.wantEntries...)
+		})
+	}
+}