Stop secret mv deleting a secret moved onto itself (closes #73)

`secret mv --force x x` deleted the secret: a move within one vault
removes an existing destination before renaming the source onto it. The
same happened for `work:x work:`, `work:x work` and `work:x ""`, and for
`work:x work/:x`, which named one vault two ways and so was taken for a
move between vaults.

A move whose two names are the same is now rejected before anything
changes. Every vault named with `vault:` must be one of the existing
vaults by exact name, checked before choosing between the two kinds of
move. A move within a named vault no longer makes it the current vault.

The test runs each rejected move on a copy of two in-memory vaults and
requires the exact error and an unchanged state directory.

Model: opus-5-5

internal/cli/secrets.go

@@ -40,6 +40,7 @@ var (
 	errVaultDoesNotExist           = errors.New("does not exist")
 	errCrossVaultSourceUnqualified = errors.New(
 		"source must specify vault (e.g., vault:secret) for cross-vault move")
+	errMoveOntoItself = errors.New("cannot be moved onto itself")
 )
 
 // bufferInfo tracks a protected buffer and the number of bytes used in it
@@ -781,8 +782,8 @@ func (cli *Instance) moveSecret(
 	// with a new name
 	if srcQualified && !destQualified {
 		// Check if dest is actually a vault name
-		vaults, err := vault.ListVaults(cli.fs, cli.stateDir)
-		if err == nil && slices.Contains(vaults, dest) {
+		_, err := cli.existingVault(dest)
+		if err == nil {
 			// dest is a vault name, use source secret name
 			destVaultName = dest
 			destSecretName = srcSecretName
@@ -798,8 +799,8 @@ func (cli *Instance) moveSecret(
 		destSecretName = srcSecretName
 	}
 
-	// Check both names, for every form of the move, before selecting a vault
-	// below, so that a rejected move leaves the current vault as it was.
+	// Check both names, for every form of the move, before building any path
+	// from them.
 	err := vault.ValidateSecretName(srcSecretName)
 	if err != nil {
 		return err
@@ -810,38 +811,66 @@ func (cli *Instance) moveSecret(
 		return err
 	}
 
-	// If neither is qualified, this is a simple within-vault rename
-	if !srcQualified && !destQualified {
-		return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force)
-	}
-
-	// Same vault? Use simple rename if possible (optimization)
-	if srcVaultName == destVaultName {
-		// Select the vault and do a simple move
-		err = vault.SelectVault(cli.fs, cli.stateDir, srcVaultName)
+	// Neither name is qualified: a rename within the current vault.
+	if !srcQualified {
+		vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
 		if err != nil {
-			return fmt.Errorf("failed to select vault '%s': %w", srcVaultName, err)
+			return err
 		}
 
-		return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force)
+		return cli.moveSecretWithinVault(
+			cmd, vlt, srcSecretName, destSecretName, force)
 	}
 
-	// Cross-vault move
-	return cli.moveSecretCrossVault(
-		cmd, srcVaultName, srcSecretName, destVaultName, destSecretName, force)
-}
-
-// moveSecretWithinVault handles rename within the current vault. Its caller,
-// MoveSecret, has already checked both secret names.
-func (cli *Instance) moveSecretWithinVault(
-	cmd *cobra.Command, source, dest string, force bool,
-) error {
-	currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
+	// Both vaults must be existing vaults by exact name, so that two
+	// spellings of one vault, such as "work" and "work/", are never taken for
+	// two vaults. A named vault does not become the current vault.
+	srcVault, err := cli.existingVault(srcVaultName)
 	if err != nil {
 		return err
 	}
 
-	vaultDir, err := currentVlt.GetDirectory()
+	destVault, err := cli.existingVault(destVaultName)
+	if err != nil {
+		return err
+	}
+
+	if srcVaultName == destVaultName {
+		return cli.moveSecretWithinVault(
+			cmd, srcVault, srcSecretName, destSecretName, force)
+	}
+
+	return cli.moveSecretCrossVault(
+		cmd, srcVault, srcSecretName, destVault, destSecretName, force)
+}
+
+// existingVault returns the vault with the given name, or an error if there
+// is none. Unlike vault.SelectVault, it leaves the current vault as it is.
+func (cli *Instance) existingVault(name string) (*vault.Vault, error) {
+	vaults, err := vault.ListVaults(cli.fs, cli.stateDir)
+	if err != nil {
+		return nil, fmt.Errorf("failed to list vaults: %w", err)
+	}
+
+	if !slices.Contains(vaults, name) {
+		return nil, fmt.Errorf("vault '%s' %w", name, errVaultDoesNotExist)
+	}
+
+	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.
+func (cli *Instance) moveSecretWithinVault(
+	cmd *cobra.Command, vlt *vault.Vault, source, dest string, force bool,
+) error {
+	// With --force the destination is removed before the source is renamed
+	// onto it, which would delete the secret.
+	if source == dest {
+		return fmt.Errorf("secret '%s' %w", source, errMoveOntoItself)
+	}
+
+	vaultDir, err := vlt.GetDirectory()
 	if err != nil {
 		return err
 	}
@@ -887,57 +916,34 @@ func (cli *Instance) moveSecretWithinVault(
 	return nil
 }
 
-// moveSecretCrossVault handles moving between different vaults. Its caller,
-// MoveSecret, has already checked both secret names.
+// moveSecretCrossVault handles moving between two different vaults. Its
+// caller, MoveSecret, has already checked both secret names and that both
+// vaults exist.
 func (cli *Instance) moveSecretCrossVault(
 	cmd *cobra.Command,
-	srcVaultName, srcSecretName,
-	destVaultName, destSecretName string,
+	srcVault *vault.Vault, srcSecretName string,
+	destVault *vault.Vault, destSecretName string,
 	force bool,
 ) error {
-	// Get source vault
-	srcVault := vault.NewVault(cli.fs, cli.stateDir, srcVaultName)
-
 	srcVaultDir, err := srcVault.GetDirectory()
 	if err != nil {
 		return fmt.Errorf("failed to get source vault directory: %w", err)
 	}
 
-	// Verify source vault exists
-	exists, err := afero.DirExists(cli.fs, srcVaultDir)
-	if err != nil || !exists {
-		return fmt.Errorf("source vault '%s' %w", srcVaultName, errVaultDoesNotExist)
-	}
-
 	// Verify source secret exists
 	srcStorageName := strings.ReplaceAll(srcSecretName, "/", "%")
 	srcSecretDir := filepath.Join(srcVaultDir, "secrets.d", srcStorageName)
 
-	exists, err = afero.DirExists(cli.fs, srcSecretDir)
+	exists, err := afero.DirExists(cli.fs, srcSecretDir)
 	if err != nil || !exists {
 		return fmt.Errorf("secret '%s' %w in vault '%s'",
-			srcSecretName, errSecretNotFound, srcVaultName)
-	}
-
-	// Get destination vault
-	destVault := vault.NewVault(cli.fs, cli.stateDir, destVaultName)
-
-	destVaultDir, err := destVault.GetDirectory()
-	if err != nil {
-		return fmt.Errorf("failed to get destination vault directory: %w", err)
-	}
-
-	// Verify destination vault exists
-	exists, err = afero.DirExists(cli.fs, destVaultDir)
-	if err != nil || !exists {
-		return fmt.Errorf("destination vault '%s' %w",
-			destVaultName, errVaultDoesNotExist)
+			srcSecretName, errSecretNotFound, srcVault.Name)
 	}
 
 	// Unlock destination vault (will fail if neither mnemonic nor unlocker available)
 	_, err = destVault.GetOrDeriveLongTermKey()
 	if err != nil {
-		return fmt.Errorf("failed to unlock destination vault '%s': %w", destVaultName, err)
+		return fmt.Errorf("failed to unlock destination vault '%s': %w", destVault.Name, err)
 	}
 
 	// Count versions for user feedback
@@ -957,13 +963,13 @@ func (cli *Instance) moveSecretCrossVault(
 		// 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",
-			srcVaultName, srcSecretName, destVaultName, destSecretName, versionCount)
+			srcVault.Name, srcSecretName, destVault.Name, destSecretName, versionCount)
 
 		return nil
 	}
 
 	cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",
-		srcVaultName, srcSecretName, destVaultName, destSecretName, versionCount)
+		srcVault.Name, srcSecretName, destVault.Name, destSecretName, versionCount)
 
 	return nil
 }