refactor: remove confusing dual ID method pattern from Unlocker interface - Removed redundant ID() method from Unlocker interface - Removed ID field from UnlockerMetadata struct - Modified GetID() to generate IDs dynamically based on unlocker type and data - Updated vault package to create unlocker instances when searching by ID - Fixed all tests and CLI code to remove ID field references - IDs are now consistently generated from unlocker data, preventing redundancy

internal/cli/unlockers.go

@@ -149,12 +149,12 @@ func (cli *CLIInstance) UnlockersList(jsonOutput bool) error {
 			// Check if this is the right unlocker by comparing metadata
 			metadataBytes, err := afero.ReadFile(cli.fs, metadataPath)
 			if err != nil {
-				continue
+				continue //FIXME this error needs to be handled
 			}
 
 			var diskMetadata secret.UnlockerMetadata
 			if err := json.Unmarshal(metadataBytes, &diskMetadata); err != nil {
-				continue
+				continue //FIXME this error needs to be handled
 			}
 
 			// Match by type and creation time
@@ -177,7 +177,8 @@ func (cli *CLIInstance) UnlockersList(jsonOutput bool) error {
 		if unlocker != nil {
 			properID = unlocker.GetID()
 		} else {
-			properID = metadata.ID // fallback to metadata ID
+			// Generate ID as fallback
+			properID = fmt.Sprintf("%s-%s", metadata.CreatedAt.Format("2006-01-02.15.04"), metadata.Type)
 		}
 
 		unlockerInfo := UnlockerInfo{

internal/secret/keychainunlocker.go

@@ -131,18 +131,14 @@ func (k *KeychainUnlocker) GetDirectory() string {
 	return k.Directory
 }
 
-// GetID implements Unlocker interface
+// GetID implements Unlocker interface - generates ID from keychain item name
 func (k *KeychainUnlocker) GetID() string {
-	return k.Metadata.ID
-}
-
-// ID implements Unlocker interface - generates ID from keychain item name
-func (k *KeychainUnlocker) ID() string {
 	// Generate ID using keychain item name
 	keychainItemName, err := k.GetKeychainItemName()
 	if err != nil {
-		// Fallback to metadata ID if we can't read the keychain item name
+		// Fallback to creation time-based ID if we can't read the keychain item name
-		return k.Metadata.ID
+		createdAt := k.Metadata.CreatedAt
+		return fmt.Sprintf("%s-keychain", createdAt.Format("2006-01-02.15.04"))
 	}
 	return fmt.Sprintf("%s-keychain", keychainItemName)
 }
@@ -362,12 +358,8 @@ func CreateKeychainUnlocker(fs afero.Fs, stateDir string) (*KeychainUnlocker, er
 	}
 
 	// Step 9: Create and write enhanced metadata
-	// Generate the key ID directly using the keychain item name
-	keyID := fmt.Sprintf("%s-keychain", keychainItemName)
-
 	keychainMetadata := KeychainUnlockerMetadata{
 		UnlockerMetadata: UnlockerMetadata{
-			ID:        keyID,
 			Type:      "keychain",
 			CreatedAt: time.Now(),
 			Flags:     []string{"keychain", "macos"},

internal/secret/metadata.go

@@ -14,7 +14,6 @@ type VaultMetadata struct {
 
 // UnlockerMetadata contains information about an unlocker
 type UnlockerMetadata struct {
-	ID        string    `json:"id"`
 	Type      string    `json:"type"` // passphrase, pgp, keychain
 	CreatedAt time.Time `json:"createdAt"`
 	Flags     []string  `json:"flags,omitempty"`

internal/secret/passphrase_test.go

@@ -40,7 +40,6 @@ func TestPassphraseUnlockerWithRealFS(t *testing.T) {
 
 	// Set up test metadata
 	metadata := secret.UnlockerMetadata{
-		ID:        "test-passphrase",
 		Type:      "passphrase",
 		CreatedAt: time.Now(),
 		Flags:     []string{},

internal/secret/passphraseunlocker.go

@@ -107,13 +107,8 @@ func (p *PassphraseUnlocker) GetDirectory() string {
 	return p.Directory
 }
 
-// GetID implements Unlocker interface
+// GetID implements Unlocker interface - generates ID from creation timestamp
 func (p *PassphraseUnlocker) GetID() string {
-	return p.Metadata.ID
-}
-
-// ID implements Unlocker interface - generates ID from creation timestamp
-func (p *PassphraseUnlocker) ID() string {
 	// Generate ID using creation timestamp: YYYY-MM-DD.HH.mm-passphrase
 	createdAt := p.Metadata.CreatedAt
 	return fmt.Sprintf("%s-passphrase", createdAt.Format("2006-01-02.15.04"))

internal/secret/pgpunlock_test.go

@@ -413,7 +413,6 @@ Passphrase: ` + testPassphrase + `
 
 	// Set up test metadata
 	metadata := secret.UnlockerMetadata{
-		ID:        fmt.Sprintf("%s-pgp", keyID),
 		Type:      "pgp",
 		CreatedAt: time.Now(),
 		Flags:     []string{"gpg", "encrypted"},

internal/secret/pgpunlocker.go

@@ -106,18 +106,14 @@ func (p *PGPUnlocker) GetDirectory() string {
 	return p.Directory
 }
 
-// GetID implements Unlocker interface
+// GetID implements Unlocker interface - generates ID from GPG key ID
 func (p *PGPUnlocker) GetID() string {
-	return p.Metadata.ID
-}
-
-// ID implements Unlocker interface - generates ID from GPG key ID
-func (p *PGPUnlocker) ID() string {
 	// Generate ID using GPG key ID: <keyid>-pgp
 	gpgKeyID, err := p.GetGPGKeyID()
 	if err != nil {
-		// Fallback to metadata ID if we can't read the GPG key ID
+		// Fallback to creation time-based ID if we can't read the GPG key ID
-		return p.Metadata.ID
+		createdAt := p.Metadata.CreatedAt
+		return fmt.Sprintf("%s-pgp", createdAt.Format("2006-01-02.15.04"))
 	}
 	return fmt.Sprintf("%s-pgp", gpgKeyID)
 }
@@ -290,12 +286,8 @@ func CreatePGPUnlocker(fs afero.Fs, stateDir string, gpgKeyID string) (*PGPUnloc
 	}
 
 	// Step 9: Create and write enhanced metadata
-	// Generate the key ID directly using the GPG key ID
-	keyID := fmt.Sprintf("%s-pgp", gpgKeyID)
-
 	pgpMetadata := PGPUnlockerMetadata{
 		UnlockerMetadata: UnlockerMetadata{
-			ID:        keyID,
 			Type:      "pgp",
 			CreatedAt: time.Now(),
 			Flags:     []string{"gpg", "encrypted"},

internal/secret/unlocker.go

@@ -10,7 +10,6 @@ type Unlocker interface {
 	GetType() string
 	GetMetadata() UnlockerMetadata
 	GetDirectory() string
-	GetID() string
+	GetID() string // Generate ID based on unlocker type and data
-	ID() string    // Generate ID from the unlocker's public key
 	Remove() error // Remove the unlocker and any associated resources
 }

internal/vault/unlockers.go

@@ -75,7 +75,6 @@ func (v *Vault) GetCurrentUnlocker() (secret.Unlocker, error) {
 	}
 
 	secret.DebugWith("Parsed unlocker metadata",
-		slog.String("unlocker_id", metadata.ID),
 		slog.String("unlocker_type", metadata.Type),
 		slog.Time("created_at", metadata.CreatedAt),
 		slog.Any("flags", metadata.Flags),
@@ -87,16 +86,16 @@ func (v *Vault) GetCurrentUnlocker() (secret.Unlocker, error) {
 	secretMetadata := secret.UnlockerMetadata(metadata)
 	switch metadata.Type {
 	case "passphrase":
-		secret.Debug("Creating passphrase unlocker instance", "unlocker_id", metadata.ID)
+		secret.Debug("Creating passphrase unlocker instance", "unlocker_type", metadata.Type)
 		unlocker = secret.NewPassphraseUnlocker(v.fs, unlockerDir, secretMetadata)
 	case "pgp":
-		secret.Debug("Creating PGP unlocker instance", "unlocker_id", metadata.ID)
+		secret.Debug("Creating PGP unlocker instance", "unlocker_type", metadata.Type)
 		unlocker = secret.NewPGPUnlocker(v.fs, unlockerDir, secretMetadata)
 	case "keychain":
-		secret.Debug("Creating keychain unlocker instance", "unlocker_id", metadata.ID)
+		secret.Debug("Creating keychain unlocker instance", "unlocker_type", metadata.Type)
 		unlocker = secret.NewKeychainUnlocker(v.fs, unlockerDir, secretMetadata)
 	default:
-		secret.Debug("Unsupported unlocker type", "type", metadata.Type, "unlocker_id", metadata.ID)
+		secret.Debug("Unsupported unlocker type", "type", metadata.Type)
 		return nil, fmt.Errorf("unsupported unlocker type: %s", metadata.Type)
 	}
 
@@ -200,23 +199,27 @@ func (v *Vault) RemoveUnlocker(unlockerID string) error {
 				continue
 			}
 
-			if metadata.ID == unlockerID {
+			unlockerDirPath = filepath.Join(unlockersDir, file.Name())
-				unlockerDirPath = filepath.Join(unlockersDir, file.Name())
 
-				// Convert our metadata to secret.UnlockerMetadata
+			// Convert our metadata to secret.UnlockerMetadata
-				secretMetadata := secret.UnlockerMetadata(metadata)
+			secretMetadata := secret.UnlockerMetadata(metadata)
 
-				// Create the appropriate unlocker instance
+			// Create the appropriate unlocker instance
-				switch metadata.Type {
+			var tempUnlocker secret.Unlocker
-				case "passphrase":
+			switch metadata.Type {
-					unlocker = secret.NewPassphraseUnlocker(v.fs, unlockerDirPath, secretMetadata)
+			case "passphrase":
-				case "pgp":
+				tempUnlocker = secret.NewPassphraseUnlocker(v.fs, unlockerDirPath, secretMetadata)
-					unlocker = secret.NewPGPUnlocker(v.fs, unlockerDirPath, secretMetadata)
+			case "pgp":
-				case "keychain":
+				tempUnlocker = secret.NewPGPUnlocker(v.fs, unlockerDirPath, secretMetadata)
-					unlocker = secret.NewKeychainUnlocker(v.fs, unlockerDirPath, secretMetadata)
+			case "keychain":
-				default:
+				tempUnlocker = secret.NewKeychainUnlocker(v.fs, unlockerDirPath, secretMetadata)
-					return fmt.Errorf("unsupported unlocker type: %s", metadata.Type)
+			default:
-				}
+				continue
+			}
+
+			// Check if this unlocker's ID matches
+			if tempUnlocker.GetID() == unlockerID {
+				unlocker = tempUnlocker
 				break
 			}
 		}
@@ -266,8 +269,27 @@ func (v *Vault) SelectUnlocker(unlockerID string) error {
 				continue
 			}
 
-			if metadata.ID == unlockerID {
+			unlockerDirPath := filepath.Join(unlockersDir, file.Name())
-				targetUnlockerDir = filepath.Join(unlockersDir, file.Name())
+
+			// Convert our metadata to secret.UnlockerMetadata
+			secretMetadata := secret.UnlockerMetadata(metadata)
+
+			// Create the appropriate unlocker instance
+			var tempUnlocker secret.Unlocker
+			switch metadata.Type {
+			case "passphrase":
+				tempUnlocker = secret.NewPassphraseUnlocker(v.fs, unlockerDirPath, secretMetadata)
+			case "pgp":
+				tempUnlocker = secret.NewPGPUnlocker(v.fs, unlockerDirPath, secretMetadata)
+			case "keychain":
+				tempUnlocker = secret.NewKeychainUnlocker(v.fs, unlockerDirPath, secretMetadata)
+			default:
+				continue
+			}
+
+			// Check if this unlocker's ID matches
+			if tempUnlocker.GetID() == unlockerID {
+				targetUnlockerDir = unlockerDirPath
 				break
 			}
 		}
@@ -298,8 +320,7 @@ func (v *Vault) CreatePassphraseUnlocker(passphrase string) (*secret.PassphraseU
 		return nil, fmt.Errorf("failed to get vault directory: %w", err)
 	}
 
-	// Create unlocker directory with timestamp
+	// Create unlocker directory
-	timestamp := time.Now().Format("2006-01-02.15.04")
 	unlockerDir := filepath.Join(vaultDir, "unlockers.d", "passphrase")
 	if err := v.fs.MkdirAll(unlockerDir, secret.DirPerms); err != nil {
 		return nil, fmt.Errorf("failed to create unlocker directory: %w", err)
@@ -331,9 +352,7 @@ func (v *Vault) CreatePassphraseUnlocker(passphrase string) (*secret.PassphraseU
 	}
 
 	// Create metadata
-	unlockerID := fmt.Sprintf("%s-passphrase", timestamp)
 	metadata := UnlockerMetadata{
-		ID:        unlockerID,
 		Type:      "passphrase",
 		CreatedAt: time.Now(),
 		Flags:     []string{},
@@ -368,13 +387,16 @@ func (v *Vault) CreatePassphraseUnlocker(passphrase string) (*secret.PassphraseU
 		return nil, fmt.Errorf("failed to write encrypted long-term private key: %w", err)
 	}
 
-	// Select this unlocker as current
-	if err := v.SelectUnlocker(unlockerID); err != nil {
-		return nil, fmt.Errorf("failed to select new unlocker: %w", err)
-	}
-
 	// Convert our metadata to secret.UnlockerMetadata for the constructor
 	secretMetadata := secret.UnlockerMetadata(metadata)
 
-	return secret.NewPassphraseUnlocker(v.fs, unlockerDir, secretMetadata), nil
+	// Create the unlocker instance
+	unlocker := secret.NewPassphraseUnlocker(v.fs, unlockerDir, secretMetadata)
+
+	// Select this unlocker as current
+	if err := v.SelectUnlocker(unlocker.GetID()); err != nil {
+		return nil, fmt.Errorf("failed to select new unlocker: %w", err)
+	}
+
+	return unlocker, nil
 }