A failed secret add --force leaves the secret unchanged (closes #136)

addVersion wrote the new version before it unlocked the vault to set
notAfter on the version being replaced, so a wrong passphrase or mnemonic
left an extra version behind. It now unlocks the vault and loads that
version's metadata first; writing notAfter and switching current still
come after the new version is saved. generate secret --force and
import --force take the same path.

The time set as the new version's notBefore and the replaced version's
notAfter is taken after the unlock, which can wait for the passphrase.

updatePreviousVersion asks for the long-term key again. The vault keeps
it in memory after the first unlock, so this neither prompts nor derives
it a second time.

Model: opus-5-5

TODO.md

@@ -18,6 +18,14 @@ https://git.eeqj.de/sneak/secret/milestone/12
 
 # Completed Steps
 
+- 2026-10-14: A `secret add --force` that cannot unlock the vault, given a wrong
+  passphrase or mnemonic, leaves the secret as it was
+  (https://git.eeqj.de/sneak/secret/issues/136). The new version used to be
+  written before the vault was unlocked to set `notAfter` on the version it
+  replaces, so each failed add left one more version that `secret version list`
+  showed. The vault is now unlocked, and that version's metadata read, before
+  anything is written. `generate secret --force` and `import --force` take the
+  same path.
 - 2026-10-12: A mnemonic typed with extra spaces, tabs or line breaks gives the
   same long-term key as its words separated by single spaces
   (https://git.eeqj.de/sneak/secret/issues/135). The key was derived from the

internal/vault/secrets.go

@@ -219,6 +219,15 @@ func (v *Vault) addVersion(
 	name, secretDir string, value *memguard.LockedBuffer,
 	previousVersion *secret.Version,
 ) error {
+	// Unlock the vault before writing anything, so that a wrong passphrase
+	// or mnemonic leaves the secret as it was
+	err := v.loadPreviousVersionMetadata(previousVersion)
+	if err != nil {
+		return err
+	}
+
+	// Taken after the unlock, which can wait for the user to type the
+	// passphrase, so that the new version starts when it is created
 	now := time.Now()
 
 	// Create the new version and save the encrypted value
@@ -632,8 +641,35 @@ func (v *Vault) checkExistingSecret(
 	return true, previousVersion, nil
 }
 
+// loadPreviousVersionMetadata unlocks the vault and loads the metadata of
+// the version being superseded. It is a no-op when previousVersion is nil.
+func (v *Vault) loadPreviousVersionMetadata(
+	previousVersion *secret.Version,
+) error {
+	if previousVersion == nil {
+		return nil
+	}
+
+	ltIdentity, err := v.GetOrDeriveLongTermKey()
+	if err != nil {
+		secret.Debug("Failed to get long-term key for previous version", "error", err)
+
+		return fmt.Errorf("failed to get long-term key: %w", err)
+	}
+
+	err = previousVersion.LoadMetadata(ltIdentity)
+	if err != nil {
+		secret.Debug("Failed to load previous version metadata", "error", err)
+
+		return fmt.Errorf("failed to load previous version metadata: %w", err)
+	}
+
+	return nil
+}
+
 // updatePreviousVersion sets the notAfter timestamp on the version being
-// superseded. It is a no-op when previousVersion is nil.
+// superseded, whose metadata loadPreviousVersionMetadata has loaded. It is
+// a no-op when previousVersion is nil.
 func (v *Vault) updatePreviousVersion(
 	previousVersion *secret.Version, now *time.Time,
 ) error {
@@ -641,22 +677,13 @@ func (v *Vault) updatePreviousVersion(
 		return nil
 	}
 
-	// Get long-term key to decrypt/encrypt metadata
+	// loadPreviousVersionMetadata unlocked the vault, so this returns the
+	// key it keeps in memory
 	ltIdentity, err := v.GetOrDeriveLongTermKey()
 	if err != nil {
-		secret.Debug("Failed to get long-term key for metadata update", "error", err)
-
 		return fmt.Errorf("failed to get long-term key: %w", err)
 	}
 
-	// Load previous version metadata
-	err = previousVersion.LoadMetadata(ltIdentity)
-	if err != nil {
-		secret.Debug("Failed to load previous version metadata", "error", err)
-
-		return fmt.Errorf("failed to load previous version metadata: %w", err)
-	}
-
 	// Update notAfter timestamp
 	previousVersion.Metadata.NotAfter = now
 

internal/vault/secrets_version_test.go

@@ -10,6 +10,8 @@
 //   version
 // - TestVaultVersionTimestamps: Tests timestamp logic (notBefore/notAfter)
 //   across versions
+// - TestVaultVersionTimestampsAfterUnlock: Tests that those timestamps are
+//   taken after AddSecret unlocks the vault
 // - TestVaultGetNonExistentVersion: Tests error handling for invalid versions
 // - TestUpdateVersionMetadata: Tests metadata update functionality
 //
@@ -295,6 +297,70 @@ func TestVaultVersionTimestamps(t *testing.T) {
 	assert.Nil(t, secondVersion.Metadata.NotAfter) // Current version
 }
 
+// longTermKeyReadFs records when longterm.age, the last file an unlock
+// reads, was last opened.
+type longTermKeyReadFs struct {
+	afero.Fs
+
+	readAt time.Time
+}
+
+//nolint:ireturn // afero.File is the interface required by afero.Fs
+func (f *longTermKeyReadFs) Open(name string) (afero.File, error) {
+	if filepath.Base(name) == "longterm.age" {
+		f.readAt = time.Now()
+	}
+
+	return f.Fs.Open(name)
+}
+
+// TestVaultVersionTimestampsAfterUnlock checks that when AddSecret has to
+// unlock the vault to replace a version, the new version's notBefore and the
+// replaced version's notAfter are taken after the unlock, which can wait for
+// the user to type the passphrase.
+func TestVaultVersionTimestampsAfterUnlock(t *testing.T) {
+	t.Parallel()
+
+	fs := &longTermKeyReadFs{Fs: afero.NewMemMapFs()}
+
+	mnemonic := memguard.NewBufferFromBytes([]byte(testMnemonic))
+	defer mnemonic.Destroy()
+
+	passphrase := memguard.NewBufferFromBytes([]byte("test-passphrase"))
+	defer passphrase.Destroy()
+
+	created, err := CreateVault(fs, testStateDir, "test", mnemonic, passphrase)
+	require.NoError(t, err)
+
+	addTestSecretToVault(t, created, testSecretPath, []byte("version-1"), false)
+
+	// A new instance is locked; AddSecret unlocks it with the passphrase
+	vault := NewVault(fs, testStateDir, "test")
+	vault.SetUnlockPassphrase(passphrase)
+
+	addTestSecretToVault(t, vault, testSecretPath, []byte("version-2"), true)
+	require.False(t, fs.readAt.IsZero(), "AddSecret did not unlock the vault")
+
+	ltIdentity, err := vault.GetOrDeriveLongTermKey()
+	require.NoError(t, err)
+
+	vaultDir, _ := vault.GetDirectory()
+	secretDir := filepath.Join(vaultDir, "secrets.d", "test%secret")
+
+	versions, err := secret.ListVersions(fs, secretDir)
+	require.NoError(t, err)
+	require.Len(t, versions, 2)
+
+	newVersion := secret.NewVersion(vault, testSecretPath, versions[0])
+	require.NoError(t, newVersion.LoadMetadata(ltIdentity))
+
+	replacedVersion := secret.NewVersion(vault, testSecretPath, versions[1])
+	require.NoError(t, replacedVersion.LoadMetadata(ltIdentity))
+
+	assert.False(t, newVersion.Metadata.NotBefore.Before(fs.readAt))
+	assert.False(t, replacedVersion.Metadata.NotAfter.Before(fs.readAt))
+}
+
 func TestVaultGetNonExistentVersion(t *testing.T) {
 	t.Parallel()
 

internal/vault/vault_error_test.go

@@ -5,6 +5,7 @@ import (
 	"path/filepath"
 	"testing"
 
+	"filippo.io/age"
 	"github.com/awnumar/memguard"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
@@ -103,3 +104,78 @@ func TestAddSecretCleansUpOnFailure(t *testing.T) {
 	require.Len(t, entries, 1)
 	assert.Equal(t, "pub.age", entries[0].Name())
 }
+
+// TestAddSecretForceWithWrongPassphraseChangesNothing checks that adding a
+// version to an existing secret, with a passphrase that does not open the
+// vault, fails and leaves the secret's directory as it was.
+func TestAddSecretForceWithWrongPassphraseChangesNothing(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewMemMapFs()
+
+	passphrase := memguard.NewBufferFromBytes([]byte(testPassphrase))
+	t.Cleanup(passphrase.Destroy)
+
+	created, err := vault.CreateVault(fs, testStateDir, testVaultName,
+		testMnemonicBuffer(t), passphrase)
+	require.NoError(t, err)
+
+	first := memguard.NewBufferFromBytes([]byte("first"))
+	t.Cleanup(first.Destroy)
+
+	require.NoError(t, created.AddSecret(testSecretName, first, false))
+
+	wrong := memguard.NewBufferFromBytes([]byte("wrong passphrase"))
+	t.Cleanup(wrong.Destroy)
+
+	vlt := vault.NewVault(fs, testStateDir, testVaultName)
+	vlt.SetUnlockPassphrase(wrong)
+
+	secretDir := filepath.Join(testStateDir, "vaults.d", testVaultName,
+		"secrets.d", testSecretName)
+	before := dirContents(t, fs, secretDir)
+
+	second := memguard.NewBufferFromBytes([]byte("second"))
+	t.Cleanup(second.Destroy)
+
+	err = vlt.AddSecret(testSecretName, second, true)
+
+	var noMatch *age.NoIdentityMatchError
+
+	require.ErrorAs(t, err, &noMatch)
+	assert.Equal(t, before, dirContents(t, fs, secretDir))
+}
+
+// dirContents maps every file under dir to its contents, and every
+// directory, written with a trailing "/", to "".
+func dirContents(t *testing.T, fs afero.Fs, dir string) map[string]string {
+	t.Helper()
+
+	contents := map[string]string{}
+
+	err := afero.Walk(fs, dir, func(
+		path string, info os.FileInfo, err error,
+	) error {
+		if err != nil {
+			return err
+		}
+
+		if info.IsDir() {
+			contents[path+"/"] = ""
+
+			return nil
+		}
+
+		content, err := afero.ReadFile(fs, path)
+		if err != nil {
+			return err
+		}
+
+		contents[path] = string(content)
+
+		return nil
+	})
+	require.NoError(t, err)
+
+	return contents
+}