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 or an unlocker's metadata file could not be read, reported no
duplicate and went on. The check now reads unlockers.d itself and stops
with an error naming the path and cause; `unlocker list` keeps skipping
unlockers it cannot read.

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.

`vault rm` and `unlocker rm` keep the state directory lock and now do
their work in an unexported function, as `vault import` does.

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
@@ -695,8 +694,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 +720,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 +731,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 +802,65 @@ 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
+// or an unlocker's metadata file cannot be read; the caller must then not
+// create the unlocker. It reads unlockers.d itself because
+// vault.ListUnlockers skips an unlocker it cannot read, which suits
+// `unlocker list` but not this check: the skipped unlocker may be the
+// duplicate. A directory whose metadata file is missing or corrupt is not
+// a working unlocker and is passed over.
+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")
 
-	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)
+	entries, err := afero.ReadDir(cli.fs, unlockersDir)
+	if errors.Is(err, os.ErrNotExist) {
+		return false, nil
+	}
 
+	if err != nil {
+		return false, fmt.Errorf(
+			"failed to read unlockers directory %s: %w", unlockersDir, err,
+		)
+	}
+
+	for _, entry := range entries {
+		if !entry.IsDir() {
 			continue
 		}
 
-		if id != "" && id == unlockerID {
-			return errUnlockerExists
+		unlockerDir := filepath.Join(unlockersDir, entry.Name())
+
+		metadataBytes, err := afero.ReadFile(
+			cli.fs, filepath.Join(unlockerDir, "unlocker-metadata.json"))
+		if errors.Is(err, os.ErrNotExist) {
+			continue
+		}
+
+		if err != nil {
+			return false, fmt.Errorf(
+				"failed to read metadata of unlocker %s: %w", unlockerDir, err,
+			)
+		}
+
+		var metadata secret.UnlockerMetadata
+
+		err = json.Unmarshal(metadataBytes, &metadata)
+		if err != nil {
+			continue
+		}
+
+		if unlockerIDFromDir(cli.fs, unlockerDir, metadata, true) == unlockerID {
+			return true, nil
 		}
 	}
 
-	return nil
+	return false, nil
 }