fix: avoid keychain secret reads for exists checks

.codereview/agents/go/implementation-tests/prompt.md

@@ -1,4 +1,4 @@
-You are reviewing Go implementation quality and test adequacy for codereview-cli.
+You are reviewing Go implementation quality and test adequacy for this repository.
 
 Optimize for high-signal findings. Return no findings when the Go code is idiomatic enough, the changed behavior is adequately tested for its risk, or a concern would require speculation. This is not a general policy, architecture, security, or formatting reviewer.
 

credstore/filebackend_nocgo_unix.go

@@ -42,6 +42,20 @@ func (b *fileKeyringBackend) get(itemKey string) (keyringItem, error) {
 	return decodeFileKeyringItem(string(bytes), b.password)
 }
 
+func (b *fileKeyringBackend) metadata(itemKey string) (keyringItem, error) {
+	name, err := b.filename(itemKey)
+	if err != nil {
+		return keyringItem{}, err
+	}
+	if _, err := os.Stat(name); err != nil {
+		if os.IsNotExist(err) {
+			return keyringItem{}, errKeyringItemNotFound
+		}
+		return keyringItem{}, err
+	}
+	return keyringItem{key: itemKey}, nil
+}
+
 func (b *fileKeyringBackend) set(it keyringItem) error {
 	if err := b.unlock(); err != nil {
 		return err

credstore/osbackend_byteness.go

@@ -65,6 +65,33 @@ func (b bytenessBackend) get(itemKey string) (keyringItem, error) {
 	}, nil
 }
 
+func (b bytenessBackend) metadata(itemKey string) (keyringItem, error) {
+	md, err := b.kr.GetMetadata(itemKey)
+	if err != nil {
+		switch {
+		case errors.Is(err, keyring.ErrKeyNotFound):
+			return keyringItem{}, errKeyringItemNotFound
+		case errors.Is(err, keyring.ErrMetadataNotSupported), errors.Is(err, keyring.ErrMetadataNeedsCredentials):
+			return keyringItem{}, errKeyringMetadataUnsupported
+		default:
+			return keyringItem{}, err
+		}
+	}
+	if md.Item == nil {
+		if md.ModificationTime.IsZero() {
+			return keyringItem{}, errKeyringMetadataUnsupported
+		}
+		return keyringItem{}, nil
+	}
+	return keyringItem{
+		key:                         md.Key,
+		label:                       md.Label,
+		description:                 md.Description,
+		keychainNotTrustApplication: md.KeychainNotTrustApplication,
+		keychainNotSynchronizable:   md.KeychainNotSynchronizable,
+	}, nil
+}
+
 func (b bytenessBackend) set(it keyringItem) error {
 	return b.kr.Set(keyring.Item{
 		Key:                         it.key,

credstore/osbackend_byteness_test.go

@@ -3,25 +3,40 @@
 package credstore
 
 import (
+	"errors"
 	"testing"
 	"time"
 
 	"github.com/byteness/keyring"
 )
 
 type captureBytenessKeyring struct {
-	setItem keyring.Item
+	setItem                           keyring.Item
+	getRet                            keyring.Item
+	getErr                            error
+	metadataRet                       keyring.Metadata
+	metadataErr                       error
+	getCalls, metadataCalls, setCalls int
 }
 
 func (c *captureBytenessKeyring) Get(string) (keyring.Item, error) {
-	return keyring.Item{}, keyring.ErrKeyNotFound
+	c.getCalls++
+	if c.getErr != nil {
+		return keyring.Item{}, c.getErr
+	}
+	return c.getRet, nil
 }
 
 func (c *captureBytenessKeyring) GetMetadata(string) (keyring.Metadata, error) {
-	return keyring.Metadata{}, keyring.ErrMetadataNotSupported
+	c.metadataCalls++
+	if c.metadataErr != nil {
+		return keyring.Metadata{}, c.metadataErr
+	}
+	return c.metadataRet, nil
 }
 
 func (c *captureBytenessKeyring) Set(item keyring.Item) error {
+	c.setCalls++
 	c.setItem = item
 	return nil
 }
@@ -63,6 +78,85 @@ func TestBytenessBackendSetPassesThroughMetadata(t *testing.T) {
 	}
 }
 
+func TestBytenessBackendMetadataUsesMetadataOnly(t *testing.T) {
+	kr := &captureBytenessKeyring{
+		metadataRet: keyring.Metadata{Item: &keyring.Item{
+			Key:                         "default/git_token",
+			Data:                        []byte("secret data must not be copied"),
+			Label:                       "label",
+			Description:                 "description",
+			KeychainNotTrustApplication: true,
+			KeychainNotSynchronizable:   true,
+		}},
+	}
+	be := bytenessBackend{kr: kr}
+
+	it, err := be.metadata("default/git_token")
+	if err != nil {
+		t.Fatalf("metadata: %v", err)
+	}
+	if kr.metadataCalls != 1 {
+		t.Fatalf("GetMetadata calls = %d, want 1", kr.metadataCalls)
+	}
+	if kr.getCalls != 0 {
+		t.Fatalf("Get calls = %d, want 0", kr.getCalls)
+	}
+	if it.key != "default/git_token" || it.label != "label" || it.description != "description" {
+		t.Fatalf("metadata item = %+v", it)
+	}
+	if len(it.data) != 0 {
+		t.Fatalf("metadata copied secret data: %q", string(it.data))
+	}
+	if !it.keychainNotTrustApplication {
+		t.Fatal("keychainNotTrustApplication = false, want true")
+	}
+	if !it.keychainNotSynchronizable {
+		t.Fatal("keychainNotSynchronizable = false, want true")
+	}
+}
+
+func TestBytenessBackendEmptyMetadataFallsBackForExists(t *testing.T) {
+	kr := &captureBytenessKeyring{getErr: keyring.ErrKeyNotFound}
+	b := &osKeyringBackend{
+		kr:          bytenessBackend{kr: kr},
+		backendKind: BackendPass,
+	}
+
+	ok, err := b.exists("default/git_token")
+	if err != nil {
+		t.Fatalf("exists: %v", err)
+	}
+	if ok {
+		t.Fatal("exists = true, want false")
+	}
+	if kr.metadataCalls != 1 {
+		t.Fatalf("GetMetadata calls = %d, want 1", kr.metadataCalls)
+	}
+	if kr.getCalls != 1 {
+		t.Fatalf("Get calls = %d, want 1", kr.getCalls)
+	}
+}
+
+func TestBytenessBackendMetadataErrorMapping(t *testing.T) {
+	for _, tc := range []struct {
+		name string
+		err  error
+		want error
+	}{
+		{"not found", keyring.ErrKeyNotFound, errKeyringItemNotFound},
+		{"not supported", keyring.ErrMetadataNotSupported, errKeyringMetadataUnsupported},
+		{"needs credentials", keyring.ErrMetadataNeedsCredentials, errKeyringMetadataUnsupported},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			be := bytenessBackend{kr: &captureBytenessKeyring{metadataErr: tc.err}}
+			_, err := be.metadata("default/git_token")
+			if !errors.Is(err, tc.want) {
+				t.Fatalf("metadata err = %v, want %v", err, tc.want)
+			}
+		})
+	}
+}
+
 func TestKeyringConfigFromBackendConfigForwardsOnePasswordOptions(t *testing.T) {
 	// #nosec G101 -- test fixture values are non-secret placeholders
 	cfg := keyringConfigFromBackendConfig(backendConfig{

credstore/osbackend_core.go

@@ -17,6 +17,7 @@ import (
 )
 
 var errKeyringItemNotFound = errors.New("credstore: keyring item not found")
+var errKeyringMetadataUnsupported = errors.New("credstore: keyring metadata unsupported")
 
 type promptFunc func(string) (string, error)
 
@@ -31,6 +32,7 @@ type keyringItem struct {
 
 type keyringBackend interface {
 	get(itemKey string) (keyringItem, error)
+	metadata(itemKey string) (keyringItem, error)
 	set(keyringItem) error
 	remove(itemKey string) error
 	keys() ([]string, error)
@@ -285,7 +287,13 @@ func (b *osKeyringBackend) delete(itemKey string) error {
 }
 
 func (b *osKeyringBackend) exists(itemKey string) (bool, error) {
-	if _, err := b.kr.get(itemKey); err != nil {
+	if _, err := b.kr.metadata(itemKey); err != nil {
+		if errors.Is(err, errKeyringMetadataUnsupported) {
+			_, err = b.kr.get(itemKey)
+		}
+		if err == nil {
+			return true, nil
+		}
 		if errors.Is(err, errKeyringItemNotFound) {
 			return false, nil
 		}

credstore/osbackend_test.go

@@ -170,8 +170,8 @@ func TestFileBackendPassphraseFuncError(t *testing.T) {
 // used to exercise osKeyringBackend's error mapping/wrapping arms
 // deterministically without a real OS keyring.
 type fakeKeyring struct {
-	items                           map[string]keyringItem
-	getErr, setErr, delErr, keysErr error
+	items                                        map[string]keyringItem
+	getErr, metadataErr, setErr, delErr, keysErr error
 }
 
 func newFakeKeyring() *fakeKeyring { return &fakeKeyring{items: map[string]keyringItem{}} }
@@ -186,6 +186,17 @@ func (f *fakeKeyring) get(k string) (keyringItem, error) {
 	}
 	return it, nil
 }
+func (f *fakeKeyring) metadata(k string) (keyringItem, error) {
+	if f.metadataErr != nil {
+		return keyringItem{}, f.metadataErr
+	}
+	it, ok := f.items[k]
+	if !ok {
+		return keyringItem{}, errKeyringItemNotFound
+	}
+	it.data = nil
+	return it, nil
+}
 func (f *fakeKeyring) set(it keyringItem) error {
 	if f.setErr != nil {
 		return f.setErr
@@ -248,7 +259,7 @@ func TestOSKeyringBackendErrorMapping(t *testing.T) {
 		op   string
 	}{
 		{"get", func(b *osKeyringBackend) error { _, e := b.get("p/k"); return e }, func(f *fakeKeyring) { f.getErr = sentinel }, "get"},
-		{"exists", func(b *osKeyringBackend) error { _, e := b.exists("p/k"); return e }, func(f *fakeKeyring) { f.getErr = sentinel }, "exists"},
+		{"exists", func(b *osKeyringBackend) error { _, e := b.exists("p/k"); return e }, func(f *fakeKeyring) { f.metadataErr = sentinel }, "exists"},
 		{"set", func(b *osKeyringBackend) error { return b.set("p/k", "v", true) }, func(f *fakeKeyring) { f.setErr = sentinel }, "set"},
 		// !overwrite + a non-not-found Get error hits the pre-check
 		// default arm (distinct from the overwrite=true write path above).
@@ -274,6 +285,44 @@ func TestOSKeyringBackendErrorMapping(t *testing.T) {
 	}
 }
 
+func TestOSKeyringBackendExistsUsesMetadataWhenAvailable(t *testing.T) {
+	f := newFakeKeyring()
+	f.items["p/k"] = keyringItem{key: "p/k", data: []byte("secret"), label: "label", description: "desc"}
+	f.getErr = errors.New("get should not run when metadata succeeds")
+	b := &osKeyringBackend{kr: f, backendKind: BackendKeychain}
+
+	ok, err := b.exists("p/k")
+	if err != nil {
+		t.Fatalf("exists: %v", err)
+	}
+	if !ok {
+		t.Fatal("exists = false, want true")
+	}
+}
+
+func TestOSKeyringBackendExistsFallsBackToGetWhenMetadataUnsupported(t *testing.T) {
+	f := newFakeKeyring()
+	f.items["p/k"] = keyringItem{key: "p/k", data: []byte("secret")}
+	f.metadataErr = errKeyringMetadataUnsupported
+	b := &osKeyringBackend{kr: f, backendKind: BackendFile}
+
+	ok, err := b.exists("p/k")
+	if err != nil {
+		t.Fatalf("exists: %v", err)
+	}
+	if !ok {
+		t.Fatal("exists = false, want true")
+	}
+
+	ok, err = b.exists("p/missing")
+	if err != nil {
+		t.Fatalf("exists missing: %v", err)
+	}
+	if ok {
+		t.Fatal("exists missing = true, want false")
+	}
+}
+
 // TestOpenEnvSelectsFileBackend exercises the real
 // selectBackend→Open integration through os.Getenv (the file backend
 // runs in CI), proving env selection actually changes the constructed

credstore/passbackend_nocgo_unix.go

@@ -68,6 +68,13 @@ func (b *passKeyringBackend) get(itemKey string) (keyringItem, error) {
 	return decodePassItemOutput(output)
 }
 
+func (b *passKeyringBackend) metadata(itemKey string) (keyringItem, error) {
+	if err := b.checkItemExists(itemKey); err != nil {
+		return keyringItem{}, err
+	}
+	return keyringItem{key: itemKey}, nil
+}
+
 func (b *passKeyringBackend) set(it keyringItem) error {
 	bytes, err := json.Marshal(persistedKeyringItem{Key: it.key, Data: it.data})
 	if err != nil {

credstore/secretservice_core.go

@@ -80,6 +80,20 @@ func (b *secretServiceBackend) get(itemKey string) (keyringItem, error) {
 	})
 }
 
+func (b *secretServiceBackend) metadata(itemKey string) (keyringItem, error) {
+	if err := b.openCollection(); err != nil {
+		return keyringItem{}, secretServiceNotFoundError(err)
+	}
+	items, err := b.collection.searchItems(itemKey)
+	if err != nil {
+		return keyringItem{}, err
+	}
+	if len(items) == 0 {
+		return keyringItem{}, errKeyringItemNotFound
+	}
+	return keyringItem{key: itemKey}, nil
+}
+
 func (b *secretServiceBackend) set(it keyringItem) error {
 	if err := b.openSecrets(); err != nil {
 		return err