Stop vault safety checks from reading unreadable state as empty (closes #51)

Adding a PGP unlocker checked unlockers.d for a duplicate and, when the
directory could not be read, reported no duplicate and went on. It now
stops with an error naming the directory and cause.

The same flaw guarded removing the last unlocker and removing a vault
(an unreadable secrets directory counted as no secrets) and vault
import (an unreadable pub.age counted as no long-term key). Those now
stop with an error too. `unlocker list` keeps skipping entries it
cannot read.

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

Model: opus-5-5

internal/cli/unlockers.go

@@ -49,7 +49,6 @@ var (
 		"is already added as an unlocker")
 	errUnsupportedUnlockerType = errors.New("unsupported unlocker type")
 	errLastUnlocker            = errors.New("refusing to remove last unlocker")
-	errUnlockerExists          = errors.New("unlocker already exists")
 )
 
 // UnlockerInfo represents unlocker information for display
@@ -346,9 +345,10 @@ func unlockerIDFromDir(
 // stored metadata matches the given type and creation time and returns
 // the matching unlocker's ID. It returns ("", nil) when the directory is
 // readable but holds no match, and a non-nil error when the directory
-// itself cannot be read. Callers must distinguish the two: an unreadable
-// directory means the unlocker's real ID is unknowable, so the entry has
-// to be skipped rather than reported under a synthesized ID.
+// itself cannot be read, which means the unlocker's ID cannot be known.
+// `unlocker list` then skips the entry rather than show a made-up ID; the
+// duplicate check before adding an unlocker must stop instead, because
+// the skipped entry may be the duplicate.
 //
 // A metadata file that cannot be read or parsed is skipped without a
 // warning: every caller gets metadata from vault.ListUnlockers first,
@@ -695,8 +695,15 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error {
 	// Check if this GPG key is already added
 	expectedID := "pgp-" + fingerprint
 
-	err = cli.checkUnlockerExists(vlt, expectedID)
+	exists, err := cli.checkUnlockerExists(vlt, expectedID)
 	if err != nil {
+		return fmt.Errorf(
+			"could not check whether GPG key %s is already an unlocker: %w",
+			gpgKeyID, err,
+		)
+	}
+
+	if exists {
 		return fmt.Errorf("GPG key %s %w", gpgKeyID, errGPGKeyAlreadyUnlocker)
 	}
 
@@ -714,7 +721,8 @@ func (cli *Instance) addPGPUnlocker(cmd *cobra.Command) error {
 	return nil
 }
 
-// UnlockersRemove removes an unlocker with safety checks
+// UnlockersRemove removes an unlocker, holding the state directory lock
+// while removeUnlocker runs
 func (cli *Instance) UnlockersRemove(
 	unlockerID string, force bool, cmd *cobra.Command,
 ) error {
@@ -724,6 +732,13 @@ func (cli *Instance) UnlockersRemove(
 	}
 	defer release()
 
+	return cli.removeUnlocker(unlockerID, force, cmd)
+}
+
+// removeUnlocker removes an unlocker with safety checks
+func (cli *Instance) removeUnlocker(
+	unlockerID string, force bool, cmd *cobra.Command,
+) error {
 	// Get current vault
 	vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 	if err != nil {
@@ -788,44 +803,38 @@ func (cli *Instance) UnlockerSelect(unlockerID string) error {
 	return vlt.SelectUnlocker(unlockerID)
 }
 
-// checkUnlockerExists checks if an unlocker with the given ID exists
-func (cli *Instance) checkUnlockerExists(vlt *vault.Vault, unlockerID string) error {
-	// Get the list of unlockers and check if any match the ID
-	unlockers, err := vlt.ListUnlockers()
-	if err != nil {
-		secret.Warn("Could not list unlockers during duplicate check", "error", err)
-
-		return nil // If we can't list unlockers, assume it doesn't exist
-	}
-
-	// Get vault directory to construct unlocker instances
+// checkUnlockerExists reports whether the vault already has an unlocker
+// with the given ID. It returns an error, and no answer, when unlockers.d
+// cannot be read; the caller must then not create the unlocker.
+func (cli *Instance) checkUnlockerExists(
+	vlt *vault.Vault, unlockerID string,
+) (bool, error) {
 	vaultDir, err := vlt.GetDirectory()
 	if err != nil {
-		secret.Warn("Could not get vault directory during duplicate check",
-			"error", err)
-
-		return nil
+		return false, fmt.Errorf("failed to get vault directory: %w", err)
 	}
 
-	// Check each unlocker's ID
 	unlockersDir := filepath.Join(vaultDir, "unlockers.d")
 
+	unlockers, err := vlt.ListUnlockers()
+	if err != nil {
+		return false, fmt.Errorf(
+			"failed to list unlockers in %s: %w", unlockersDir, err,
+		)
+	}
+
 	for _, metadata := range unlockers {
 		// Construct the unlocker matching this metadata to get its ID
 		id, err := findUnlockerIDByMetadata(cli.fs, unlockersDir, metadata, true)
 		if err != nil {
-			secret.Warn(
-				"Could not read unlockers directory during duplicate check, "+
-					"skipping unlocker",
-				"unlockers_dir", unlockersDir, "error", err)
-
-			continue
+			// Unlike `unlocker list`, never skip here: a skipped entry may be the duplicate.
+			return false, err
 		}
 
 		if id != "" && id == unlockerID {
-			return errUnlockerExists
+			return true, nil
 		}
 	}
 
-	return nil
+	return false, nil
 }