refactor(keychain): make credential-ref override application testable

Addresses review: the override-application in open() (swap credential_ref +
suppress the one-time legacy migration) had no direct coverage — only the pure
effectiveRef() precedence and the flag-wiring were tested, so a regression that
dropped runMigration=false could re-run the token.json migration against an
arbitrary --ref/env profile and misattribute credential data, undetected.

Extract applyCredentialRefOverride(cfg, runMigration) from open() and add
TestApplyCredentialRefOverride asserting cfg.CredentialRef is swapped AND
runMigration is forced false on a flag/env override (and both untouched with
none), mirroring TestOpenWith_FlagOverridesConfig.

Author: piekstra

internal/keychain/credref_test.go

@@ -1,6 +1,10 @@
 package keychain
 
-import "testing"
+import (
+	"testing"
+
+	"github.com/open-cli-collective/google-readonly/internal/config"
+)
 
 // resetCredRefOverride keeps the package-level --ref override clean across
 // tests so a leaked value can't tilt the next.
@@ -61,3 +65,53 @@ func TestEffectiveRef_Precedence(t *testing.T) {
 		}
 	})
 }
+
+// TestApplyCredentialRefOverride proves the safety-critical part open() relies
+// on: a present override swaps cfg.CredentialRef AND forces runMigration=false
+// (so the one-time legacy migration never runs against an arbitrary
+// --ref/env-selected profile), while no override leaves both untouched. This is
+// the open()-side coverage the pure effectiveRef/wiring tests don't provide.
+func TestApplyCredentialRefOverride(t *testing.T) {
+	const cfgRef = "google-readonly/cfg"
+
+	t.Run("no override: cfg untouched, runMigration passthrough", func(t *testing.T) {
+		resetCredRefOverride(t)
+		t.Setenv(CredentialRefEnvVar(), "")
+		cfg := &config.Config{CredentialRef: cfgRef}
+		if rm := applyCredentialRefOverride(cfg, true); !rm {
+			t.Errorf("runMigration = %v, want true (passthrough)", rm)
+		}
+		if cfg.CredentialRef != cfgRef {
+			t.Errorf("cfg.CredentialRef = %q, want unchanged %q", cfg.CredentialRef, cfgRef)
+		}
+		// passthrough must preserve a false caller value too (OpenNoMigrate path)
+		if rm := applyCredentialRefOverride(cfg, false); rm {
+			t.Errorf("runMigration = %v, want false (passthrough of caller's false)", rm)
+		}
+	})
+
+	t.Run("flag override: cfg swapped, migration suppressed", func(t *testing.T) {
+		resetCredRefOverride(t)
+		t.Setenv(CredentialRefEnvVar(), "")
+		SetCredentialRefOverride("google-readonly/flag", true)
+		cfg := &config.Config{CredentialRef: cfgRef}
+		if rm := applyCredentialRefOverride(cfg, true); rm {
+			t.Errorf("runMigration = %v, want false (override must suppress migration)", rm)
+		}
+		if cfg.CredentialRef != "google-readonly/flag" {
+			t.Errorf("cfg.CredentialRef = %q, want google-readonly/flag", cfg.CredentialRef)
+		}
+	})
+
+	t.Run("env override: cfg swapped, migration suppressed", func(t *testing.T) {
+		resetCredRefOverride(t)
+		t.Setenv(CredentialRefEnvVar(), "google-readonly/env")
+		cfg := &config.Config{CredentialRef: cfgRef}
+		if rm := applyCredentialRefOverride(cfg, true); rm {
+			t.Errorf("runMigration = %v, want false (env override must suppress migration)", rm)
+		}
+		if cfg.CredentialRef != "google-readonly/env" {
+			t.Errorf("cfg.CredentialRef = %q, want google-readonly/env", cfg.CredentialRef)
+		}
+	})
+}

internal/keychain/keychain.go

@@ -73,18 +73,28 @@ func open(overwrite, runMigration bool) (*Store, error) {
 	if err != nil {
 		return nil, err
 	}
-	// A per-invocation --ref flag or <SERVICE>_CREDENTIAL_REF env selects the
-	// credential ref for this process (precedence: flag > env > config), so
-	// concurrent processes can each target a different account without racing
-	// on the shared config.yml. An explicit override also suppresses the
-	// one-time §1.8 migration: it only ever targets the configured/default ref
-	// (running it against an arbitrary ref could write the default's legacy
-	// data under the wrong service/profile — the same reason OpenRef skips it).
+	runMigration = applyCredentialRefOverride(cfg, runMigration)
+	return openWith(cfg, overwrite, runMigration)
+}
+
+// applyCredentialRefOverride applies the per-invocation credential-ref override
+// to cfg in place and returns the effective runMigration decision. A
+// --ref flag or <SERVICE>_CREDENTIAL_REF env (precedence: flag > env > config)
+// selects the ref for this process, so concurrent processes can each target a
+// different account without racing on the shared config.yml. A present override
+// also FORCES runMigration=false: the one-time §1.8 migration only ever targets
+// the configured/default ref (running it against an arbitrary ref could write
+// the default's legacy data under the wrong service/profile — the same reason
+// OpenRef skips it). With no override, cfg is untouched and the caller's
+// runMigration is returned unchanged. Split out of open() so this
+// safety-critical swap+suppression is directly testable without
+// config.LoadConfigForRuntime or a real keyring.
+func applyCredentialRefOverride(cfg *config.Config, runMigration bool) bool {
 	if ref, overridden := effectiveRef(cfg.CredentialRef); overridden {
 		cfg.CredentialRef = ref
-		runMigration = false
+		return false
 	}
-	return openWith(cfg, overwrite, runMigration)
+	return runMigration
 }
 
 // CredentialRefEnvVar is the per-invocation credential-ref override env var