From ee7002a2cc532d6091345132b775354239b79284 Mon Sep 17 00:00:00 2001
From: sneak <sneak@sneak.berlin>
Date: Thu, 15 Oct 2026 01:12:31 +0000
Subject: [PATCH] A cross-vault secret mv that cannot remove the source fails
 (closes #159)

When `secret mv` copied a secret into another vault but could not remove
the source, it printed a warning and the usual "Moved" line and exited 0,
so a script could not tell that the secret was now in both vaults. It now
prints no "Moved" line and fails with an error saying that the copy holds
all versions and the source remains.

The test is in package cli and calls moveSecret, because the state
directory lock refuses the wrapped filesystem that makes the removal fail.

Judgement call: if the source is moved aside but the temporary directory
holding it cannot then be deleted, the error still says the source remains.

Model: opus-5-5
---
 internal/cli/move_cross_vault_test.go | 85 +++++++++++++++++++++++++++
 internal/cli/secrets.go               | 10 ++--
 2 files changed, 89 insertions(+), 6 deletions(-)
 create mode 100644 internal/cli/move_cross_vault_test.go

diff --git a/internal/cli/move_cross_vault_test.go b/internal/cli/move_cross_vault_test.go
new file mode 100644
index 0000000..798d667
--- /dev/null
+++ b/internal/cli/move_cross_vault_test.go
@@ -0,0 +1,85 @@
+//nolint:testpackage // white-box test of unexported internals
+package cli
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/awnumar/memguard"
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/secret/internal/vault"
+)
+
+// errRenameFailed is returned by renameFailFs in place of a successful
+// rename.
+var errRenameFailed = errors.New("permission denied")
+
+// renameFailFs fails every Rename of one path, as a read-only parent
+// directory does.
+type renameFailFs struct {
+	afero.Fs
+
+	path string
+}
+
+func (f *renameFailFs) Rename(oldname, newname string) error {
+	if oldname == f.path {
+		return errRenameFailed
+	}
+
+	return f.Fs.Rename(oldname, newname)
+}
+
+// TestCrossVaultMoveFailsWhenSourceRemains is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/159, where a move between two
+// vaults whose source could not be removed reported the secret moved and
+// succeeded. It must fail, and the copy and the source must both be there.
+// The state directory lock refuses the wrapped filesystem, so the test
+// calls moveSecret, which MoveSecret runs once it holds the lock.
+func TestCrossVaultMoveFailsWhenSourceRemains(t *testing.T) {
+	t.Parallel()
+
+	base := afero.NewMemMapFs()
+
+	_, err := vault.CreateVault(base, testStateDir, "work",
+		testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	source, err := vault.CreateVault(base, testStateDir, "default",
+		testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	err = source.AddSecret("x",
+		memguard.NewBufferFromBytes([]byte("value")), false)
+	require.NoError(t, err)
+
+	instance := NewCLIInstanceWithStateDir(&renameFailFs{
+		Fs:   base,
+		path: testStateDir + "/vaults.d/default/secrets.d/x",
+	}, testStateDir)
+	instance.Mnemonic = testMnemonicBuffer(t)
+
+	err = instance.moveSecret(&cobra.Command{}, "default:x", "work:y", false)
+	require.ErrorIs(t, err, errRenameFailed)
+	require.ErrorContains(t, err, "secret 'default:x' is copied with all "+
+		"1 version(s) to 'work:y', but the source remains")
+
+	// requireValue requires that the vault vaultName holds secretName with
+	// the value stored above.
+	requireValue := func(vaultName, secretName string) {
+		vlt := vault.NewVault(base, testStateDir, vaultName)
+		vlt.Mnemonic = testMnemonicBuffer(t)
+
+		value, err := vlt.GetSecret(secretName)
+		require.NoError(t, err, vaultName+":"+secretName)
+
+		defer value.Destroy()
+
+		require.Equal(t, []byte("value"), value.Bytes())
+	}
+
+	requireValue("default", "x")
+	requireValue("work", "y")
+}
diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go
index 59c64e9..827c232 100644
--- a/internal/cli/secrets.go
+++ b/internal/cli/secrets.go
@@ -1097,12 +1097,10 @@ func (cli *Instance) moveSecretCrossVault(
 	// Delete source secret
 	err = secret.RemoveDirAtomic(cli.fs, srcSecretDir)
 	if err != nil {
-		// 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",
-			srcVault.Name, srcSecretName, destVault.Name, destSecretName, versionCount)
-
-		return nil
+		return fmt.Errorf("secret '%s:%s' is copied with all %d version(s) "+
+			"to '%s:%s', but the source remains: %w",
+			srcVault.Name, srcSecretName, versionCount,
+			destVault.Name, destSecretName, err)
 	}
 
 	cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",