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.
It now fails. If the source secret's directory is still in place, the
error says the copy holds all versions and the source remains. If the
source left its vault and only the temporary directory it was moved into
could not be deleted, the error says the secret is moved and names that
directory, which the error of RemoveDirAtomic now does.

The tests are in package cli and call moveSecret, because the state
directory lock refuses the wrapped filesystems that make the removal fail.

Model: opus-5-5

README.md

@@ -67,6 +67,10 @@ and unlocker set. This allows for complete isolation between different contexts
 
 ## Commands Reference
 
+Every time `secret` prints, in a table or in `--json` output, is in UTC,
+whatever `TZ` or the system's time zone is set to. The date in a version's name
+is the UTC date too.
+
 ### Confirmation Before Removal
 
 `secret rm`, `secret version rm`, `secret vault remove` and

internal/cli/list_times_test.go

@@ -3,6 +3,9 @@ package cli_test
 import (
 	"bytes"
 	"encoding/json"
+	"os"
+	"os/exec"
+	"path/filepath"
 	"testing"
 	"time"
 
@@ -125,6 +128,57 @@ func TestMovedSecretKeepsItsTimes(t *testing.T) {
 		"updated %s, then %s", beforeMove.UpdatedAt, afterMove.UpdatedAt)
 }
 
+// TestListedTimesAreUTC is a regression test for
+// https://git.eeqj.de/sneak/secret/issues/169, where times were printed
+// in the local time zone. It runs `secret list` on the built binary with
+// TZ set to New York, where the secret's last update, 03:04:05 UTC on 2
+// January, is 22:04:05 the day before.
+func TestListedTimesAreUTC(t *testing.T) {
+	t.Parallel()
+
+	stateDir := t.TempDir()
+
+	vlt, err := vault.CreateVault(
+		afero.NewOsFs(), stateDir, "default", testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	vlt.Mnemonic = testMnemonicBuffer(t)
+	err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("one")), false)
+	require.NoError(t, err)
+
+	updated := time.Date(2026, time.January, 2, 3, 4, 5, 0, time.UTC)
+	current := filepath.Join(stateDir, "vaults.d", "default", "secrets.d", "x",
+		"current")
+	require.NoError(t, os.Chtimes(current, updated, updated))
+
+	tests := []struct {
+		flags []string
+		want  string
+	}{
+		{flags: nil, want: "2026-01-02 03:04"},
+		{
+			flags: []string{"--json"},
+			want:  `"updated_at": "2026-01-02T03:04:05Z"`,
+		},
+	}
+
+	for _, tt := range tests {
+		args := append([]string{"list"}, tt.flags...)
+
+		//nolint:gosec // G204: test executes the freshly built secret binary
+		cmd := exec.CommandContext(t.Context(), secretBinaryPath(t), args...)
+		cmd.Env = []string{
+			secret.EnvStateDir + "=" + stateDir,
+			"TZ=America/New_York",
+			"PATH=" + os.Getenv("PATH"),
+		}
+
+		output, err := cmd.CombinedOutput()
+		require.NoError(t, err, "%v: %s", args, output)
+		assert.Contains(t, string(output), tt.want, "%v", args)
+	}
+}
+
 // listSecrets returns the output of `secret list`, or of
 // `secret list --json` when jsonOutput is set.
 func listSecrets(t *testing.T, c *cli.Instance, jsonOutput bool) string {

internal/cli/move_cross_vault_test.go

@@ -3,6 +3,7 @@ package cli
 
 import (
 	"errors"
+	"path/filepath"
 	"testing"
 
 	"github.com/awnumar/memguard"
@@ -12,9 +13,9 @@ import (
 	"sneak.berlin/go/secret/internal/vault"
 )
 
-// errRenameFailed is returned by renameFailFs in place of a successful
-// rename.
-var errRenameFailed = errors.New("permission denied")
+// errPermissionDenied is returned by renameFailFs and removeAllFailFs in
+// place of a rename or removal that succeeds.
+var errPermissionDenied = errors.New("permission denied")
 
 // renameFailFs fails every Rename of one path, as a read-only parent
 // directory does.
@@ -26,12 +27,66 @@ type renameFailFs struct {
 
 func (f *renameFailFs) Rename(oldname, newname string) error {
 	if oldname == f.path {
-		return errRenameFailed
+		return errPermissionDenied
 	}
 
 	return f.Fs.Rename(oldname, newname)
 }
 
+// removeAllFailFs fails every RemoveAll of an entry of the directory dir,
+// as a file in it that cannot be deleted does, and records that entry's
+// path in failed.
+type removeAllFailFs struct {
+	afero.Fs
+
+	dir    string
+	failed string
+}
+
+func (f *removeAllFailFs) RemoveAll(path string) error {
+	if filepath.Dir(path) == f.dir {
+		f.failed = path
+
+		return errPermissionDenied
+	}
+
+	return f.Fs.RemoveAll(path)
+}
+
+// createCrossVaults creates in fs the vaults "work" and "default", where
+// "default" holds the secret "x" with the value "value".
+func createCrossVaults(t *testing.T, fs afero.Fs) {
+	t.Helper()
+
+	_, err := vault.CreateVault(fs, testStateDir, "work",
+		testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	source, err := vault.CreateVault(fs, testStateDir, "default",
+		testMnemonicBuffer(t), nil)
+	require.NoError(t, err)
+
+	err = source.AddSecret("x",
+		memguard.NewBufferFromBytes([]byte("value")), false)
+	require.NoError(t, err)
+}
+
+// requireValue requires that the vault vaultName in fs holds secretName
+// with the value createCrossVaults stored.
+func requireValue(t *testing.T, fs afero.Fs, vaultName, secretName string) {
+	t.Helper()
+
+	vlt := vault.NewVault(fs, 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())
+}
+
 // 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
@@ -42,18 +97,7 @@ 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)
+	createCrossVaults(t, base)
 
 	instance := NewCLIInstanceWithStateDir(&renameFailFs{
 		Fs:   base,
@@ -61,25 +105,48 @@ func TestCrossVaultMoveFailsWhenSourceRemains(t *testing.T) {
 	}, testStateDir)
 	instance.Mnemonic = testMnemonicBuffer(t)
 
-	err = instance.moveSecret(&cobra.Command{}, "default:x", "work:y", false)
-	require.ErrorIs(t, err, errRenameFailed)
+	err := instance.moveSecret(&cobra.Command{}, "default:x", "work:y", false)
+	require.ErrorIs(t, err, errPermissionDenied)
 	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)
+	requireValue(t, base, "default", "x")
+	requireValue(t, base, "work", "y")
+}
 
-		value, err := vlt.GetSecret(secretName)
-		require.NoError(t, err, vaultName+":"+secretName)
+// TestCrossVaultMoveFailsWhenOldFilesRemain makes the removal of the source
+// fail after the source has been moved out of its vault, when only the
+// temporary directory it was moved into cannot be deleted. The move must
+// fail, say that the secret is moved, and name that directory.
+func TestCrossVaultMoveFailsWhenOldFilesRemain(t *testing.T) {
+	t.Parallel()
 
-		defer value.Destroy()
+	base := afero.NewMemMapFs()
+	createCrossVaults(t, base)
 
-		require.Equal(t, []byte("value"), value.Bytes())
-	}
+	sourceVaultDir := testStateDir + "/vaults.d/default"
+	fs := &removeAllFailFs{Fs: base, dir: sourceVaultDir}
 
-	requireValue("default", "x")
-	requireValue("work", "y")
+	instance := NewCLIInstanceWithStateDir(fs, testStateDir)
+	instance.Mnemonic = testMnemonicBuffer(t)
+
+	err := instance.moveSecret(&cobra.Command{}, "default:x", "work:y", false)
+	require.ErrorIs(t, err, errPermissionDenied)
+	require.ErrorContains(t, err, "secret 'default:x' is moved with all "+
+		"1 version(s) to 'work:y', but its old files could not all be deleted")
+
+	// The error names the directory that holds the old files
+	require.NotEmpty(t, fs.failed)
+	require.ErrorContains(t, err, fs.failed)
+
+	oldFiles, err := afero.DirExists(base, filepath.Join(fs.failed, "x"))
+	require.NoError(t, err)
+	require.True(t, oldFiles)
+
+	// The source vault no longer holds the secret
+	remains, err := afero.DirExists(base, sourceVaultDir+"/secrets.d/x")
+	require.NoError(t, err)
+	require.False(t, remains)
+
+	requireValue(t, base, "work", "y")
 }

internal/cli/root.go

@@ -2,6 +2,7 @@ package cli
 
 import (
 	"os"
+	"time"
 
 	"github.com/awnumar/memguard"
 	"github.com/spf13/cobra"
@@ -14,6 +15,10 @@ import (
 // every memguard buffer before it returns, so the caller must do nothing
 // but exit with the code.
 func Entry() int {
+	// Every time secret prints or writes is in UTC, whatever TZ or the
+	// system's time zone is set to.
+	time.Local = time.UTC //nolint:gosmopolitan // this makes local time UTC
+
 	raiseLockedMemoryLimit()
 
 	// On SIGINT or SIGTERM memguard runs this function, wipes every buffer

internal/cli/secrets.go

@@ -1097,10 +1097,9 @@ func (cli *Instance) moveSecretCrossVault(
 	// Delete source secret
 	err = secret.RemoveDirAtomic(cli.fs, srcSecretDir)
 	if err != 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)
+		return cli.sourceRemovalError(err, srcSecretDir,
+			srcVault.Name+":"+srcSecretName, destVault.Name+":"+destSecretName,
+			versionCount)
 	}
 
 	cmd.Printf("Moved secret '%s:%s' to '%s:%s' (%d version(s))\n",
@@ -1108,3 +1107,31 @@ func (cli *Instance) moveSecretCrossVault(
 
 	return nil
 }
+
+// sourceRemovalError returns the error of a move between vaults that
+// copied all versionCount versions of the secret srcName to destName, and
+// then failed with removeErr to remove srcSecretDir, the directory of
+// srcName. The removal can fail after the source has left its vault, when
+// only the temporary directory it was moved into cannot be deleted, so the
+// error says the source remains only if srcSecretDir is still there.
+func (cli *Instance) sourceRemovalError(
+	removeErr error, srcSecretDir, srcName, destName string, versionCount int,
+) error {
+	remains, err := afero.DirExists(cli.fs, srcSecretDir)
+
+	switch {
+	case err != nil:
+		return fmt.Errorf("secret '%s' is copied with all %d version(s) "+
+			"to '%s', but removing the source failed and whether it "+
+			"remains is unknown: %w",
+			srcName, versionCount, destName, errors.Join(removeErr, err))
+	case remains:
+		return fmt.Errorf("secret '%s' is copied with all %d version(s) "+
+			"to '%s', but the source remains: %w",
+			srcName, versionCount, destName, removeErr)
+	default:
+		return fmt.Errorf("secret '%s' is moved with all %d version(s) "+
+			"to '%s', but its old files could not all be deleted: %w",
+			srcName, versionCount, destName, removeErr)
+	}
+}

internal/secret/atomic.go

@@ -149,7 +149,9 @@ func WriteDir(fs afero.Fs, dir string, write func(dir string) error) error {
 
 // RemoveDirAtomic deletes the directory dir so that it disappears in one
 // rename: dir is moved into a new directory from TempDirFor, which is then
-// deleted. A crash part-way leaves only that temporary directory behind.
+// deleted. A crash part-way leaves only that temporary directory behind. If
+// only deleting the temporary directory fails, dir is already gone, and the
+// error names the temporary directory, which holds what is left of it.
 func RemoveDirAtomic(fs afero.Fs, dir string) error {
 	tmp, err := TempDirFor(fs, dir)
 	if err != nil {
@@ -165,7 +167,8 @@ func RemoveDirAtomic(fs afero.Fs, dir string) error {
 
 	err = fs.RemoveAll(tmp)
 	if err != nil {
-		return fmt.Errorf("failed to remove %s: %w", dir, err)
+		return fmt.Errorf("failed to remove %s after moving %s into it: %w",
+			tmp, dir, err)
 	}
 
 	return nil