fix(cli): persist login token durably on detached/headless boxes

cmd/login.go

@@ -152,14 +152,26 @@ func runLogin(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	// Step 4: Save credentials.
+	// Step 4: Save credentials. cfg.Save() durably persists the bearer token —
+	// keychain when it's available AND the write survives an immediate
+	// read-back, otherwise the 0600 ~/.instant-config file fallback (the path
+	// that the next `instant whoami` / authenticated call reads). If Save
+	// returns an error NEITHER store accepted the token: we MUST NOT print
+	// "✓ Logged in" (the headless false-success this fix exists to kill).
+	// Instead hand the user the token + an explicit `export INSTANT_TOKEN=…`
+	// escape hatch so a headless agent can still proceed in-flow.
 	cfg.APIKey = result.APIKey
 	cfg.Email = result.Email
 	cfg.Tier = result.Tier
 	cfg.TeamName = result.TeamName
 	cfg.APIBaseURL = APIBaseURL
 	if err := cfg.Save(); err != nil {
-		return fmt.Errorf("saving credentials: %w", err)
+		return fmt.Errorf(
+			"login succeeded but the API key could not be saved to the keychain "+
+				"or to ~/.instant-config (%w).\nUse the token directly instead:\n"+
+				"  export INSTANT_TOKEN=%s\n"+
+				"…then re-run your command. (Avoid committing the token.)",
+			err, result.APIKey)
 	}
 
 	fmt.Printf("\n✓  Logged in as %s (%s)\n", result.Email, result.Tier)

cmd/login_persist_test.go

@@ -0,0 +1,61 @@
+package cmd
+
+// login_persist_test.go — guards the headless/detached login no-false-success
+// contract. The live dogfood bug: `instant login` from a detached/headless box
+// printed "✓ Logged in" while persisting NOTHING readable, so the next
+// `instant whoami` said "Not logged in". The durable-persist fix lives in
+// cliconfig.Save (keychain write-then-readback → file fallback); this file
+// pins the runLogin SURFACE: when neither store accepts the token (cfg.Save
+// errors), runLogin must NOT print the success banner and must instead hand
+// the user the token + an `export INSTANT_TOKEN=…` escape hatch.
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// TestLogin_SaveFailure_NoFalseSuccess forces cfg.Save() to fail while leaving
+// cliconfig.Load() working, by pre-creating the atomic temp-write target
+// (~/.instant-config.tmp) as a DIRECTORY: os.WriteFile to that path errors,
+// but Load's read of the (absent) ~/.instant-config returns a clean empty
+// config. runLogin must then surface a hard error carrying the
+// export-INSTANT_TOKEN escape hatch and the token itself, and must NOT print
+// the "Logged in as" banner.
+func TestLogin_SaveFailure_NoFalseSuccess(t *testing.T) {
+	withCleanState(t)
+
+	// HOME is already a fresh temp dir (withCleanState). Block ONLY the Save
+	// temp-write by occupying ~/.instant-config.tmp with a directory.
+	home, err := os.UserHomeDir()
+	if err != nil {
+		t.Fatalf("home dir: %v", err)
+	}
+	if err := os.Mkdir(filepath.Join(home, ".instant-config.tmp"), 0700); err != nil {
+		t.Fatalf("seed temp-write blocker: %v", err)
+	}
+
+	srv := claimLoginServer(t, nil, 0)
+	defer srv.Close()
+	withTestAPI(t, srv.URL)
+
+	var runErr error
+	stdout, _ := captureStdout(t, func() {
+		runErr = runLogin(nil, nil)
+	})
+
+	if runErr == nil {
+		t.Fatalf("expected runLogin to error when credentials can't be saved; stdout:\n%s", stdout)
+	}
+	if strings.Contains(stdout, loggedInPrefix) {
+		t.Errorf("runLogin printed the success banner despite a failed save (false success); stdout:\n%s", stdout)
+	}
+	msg := runErr.Error()
+	if !strings.Contains(msg, "export INSTANT_TOKEN=") {
+		t.Errorf("error must offer the INSTANT_TOKEN escape hatch; got: %v", runErr)
+	}
+	if !strings.Contains(msg, claimTestAPIKey) {
+		t.Errorf("error must include the token so a headless agent can proceed; got: %v", runErr)
+	}
+}

internal/cliconfig/cliconfig.go

@@ -96,6 +96,46 @@ func warnFileFallback() {
 	})
 }
 
+// secretGet/secretSet are package-level seams over the secretstore so tests
+// can inject a keychain that "succeeds" on Set but doesn't durably persist
+// (the headless/detached failure mode — see persistSecret). Production wires
+// them straight through; tests swap them in a t.Cleanup-guarded block.
+var (
+	secretSet = secretstore.Set
+	secretGet = secretstore.Get
+)
+
+// persistSecret durably stores the bearer token, returning true iff it landed
+// in the OS keychain AND survives an immediate read-back. It returns false
+// (caller must use the on-disk file fallback) when the keychain is either
+// unavailable (Set errors) OR reports success but the value does NOT round-trip
+// on a fresh Get — the silent detached/headless failure mode that this whole
+// change exists to close.
+//
+// WHY THE READ-BACK. `instant login` from a detached/headless process (e.g.
+// `nohup instant login &` on an agent box) hits a keychain backend whose
+// Available() probe is a FALSE POSITIVE: a probing Get that returns "not found"
+// is read as "available, just empty", and the subsequent Set returns nil even
+// though the write landed in an ephemeral/locked session keyring the NEXT
+// process can't read. Set's nil return is therefore NOT proof of durability.
+// Verifying the write by reading it straight back is the only signal that
+// distinguishes a real keychain entry from a write that evaporates — and it is
+// what flips the file-fallback branch on so the token actually persists.
+func persistSecret(apiKey string) bool {
+	if err := secretSet(apiKey); err != nil {
+		// Keychain unavailable / no active backend — caller writes the file.
+		return false
+	}
+	// Read-back verification: a keychain that accepted the write but can't
+	// return the same value (detached session, locked collection) is NOT a
+	// durable store. Treat a mismatch or read error exactly like a Set failure.
+	got, err := secretGet()
+	if err != nil || got != apiKey {
+		return false
+	}
+	return true
+}
+
 // IsAuthenticated reports whether the config holds valid credentials.
 func (c *Config) IsAuthenticated() bool {
 	return c != nil && c.APIKey != ""
@@ -177,14 +217,19 @@ func (c *Config) Save() error {
 	}
 	c.SavedAt = time.Now().UTC()
 
-	// Decide where the secret lives.
+	// Decide where the secret lives. persistSecret returns true ONLY when the
+	// key landed in the keychain AND survives an immediate read-back; a silent
+	// detached/headless write that doesn't round-trip returns false so we fall
+	// back to the 0600 on-disk field (which Load() already reads). This guards
+	// the "✓ Logged in but next whoami says Not logged in" headless regression.
 	persisted := false
 	c.FallbackAPIKey = ""
 	if c.APIKey != "" {
-		if err := secretstore.Set(c.APIKey); err == nil {
+		if persistSecret(c.APIKey) {
 			persisted = true
 		} else {
-			// Keychain unavailable — fall back to writing it on disk.
+			// Keychain unavailable OR write didn't survive read-back — fall
+			// back to writing the key on disk so the next invocation finds it.
 			c.FallbackAPIKey = c.APIKey
 			warnFileFallback()
 		}

internal/cliconfig/headless_persist_test.go

@@ -0,0 +1,174 @@
+package cliconfig
+
+// White-box tests for the headless/detached login token-persistence fix.
+//
+// Root cause being guarded: on a detached/headless box the OS keychain's
+// Available() probe is a false positive — Set() returns nil even though the
+// write lands in an ephemeral/locked session keyring the next process can't
+// read. Because Set didn't error, the old Save() never took the file-fallback
+// branch, so `instant login` printed "✓ Logged in" while persisting NOTHING
+// the next `instant whoami` could read.
+//
+// The fix verifies the keychain write with an immediate read-back and falls
+// back to the 0600 ~/.instant-config file when the value doesn't round-trip.
+// These tests inject the secretSet/secretGet seams to simulate that exact
+// silent-write failure WITHOUT touching the real OS keychain.
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// withSecretSeams installs replacement secretSet/secretGet implementations for
+// the duration of one test and restores the originals on cleanup.
+func withSecretSeams(t *testing.T, set func(string) error, get func() (string, error)) {
+	t.Helper()
+	prevSet, prevGet := secretSet, secretGet
+	secretSet, secretGet = set, get
+	t.Cleanup(func() { secretSet, secretGet = prevSet, prevGet })
+}
+
+// TestSave_SilentKeychainWriteFallsBackToFile is the core regression test for
+// the detached/headless login bug. The injected keychain ACCEPTS the write
+// (Set returns nil) but the read-back returns a DIFFERENT value (the
+// ephemeral/locked-session keyring the next process can't read). Save must
+// detect the non-durable write and persist the token to the 0600 file
+// fallback, and a fresh Load must return it — i.e. the next `instant whoami`
+// finds the token.
+func TestSave_SilentKeychainWriteFallsBackToFile(t *testing.T) {
+	// Set "succeeds" but Get never returns what we wrote — the silent
+	// detached-keychain failure mode.
+	withSecretSeams(t,
+		func(string) error { return nil },         // Set: false success
+		func() (string, error) { return "", nil }, // Get: empty (write evaporated)
+	)
+
+	dir := t.TempDir()
+	t.Setenv("HOME", dir)
+	path := filepath.Join(dir, ".instant-config")
+
+	cfg := &Config{path: path, APIKey: "inst_live_headless_secret", Email: "agent@example.com"}
+	require.NoError(t, cfg.Save())
+
+	// The token MUST have landed in the on-disk fallback field.
+	data, err := os.ReadFile(path)
+	require.NoError(t, err)
+	body := string(data)
+	assert.Contains(t, body, FallbackAPIKeyField,
+		"silent keychain write must trigger the file fallback")
+	assert.Contains(t, body, "inst_live_headless_secret",
+		"token must be persisted to disk when the keychain write doesn't round-trip")
+
+	info, err := os.Stat(path)
+	require.NoError(t, err)
+	assert.Equal(t, os.FileMode(0600), info.Mode().Perm(),
+		"file fallback must stay mode 0600")
+
+	// And the next invocation (whoami / authed call) must read it back.
+	// Load consults secretstore first; our seam still returns empty there, so
+	// the value can only come from the on-disk fallback — exactly the path
+	// `instant whoami` exercises after a headless login.
+	loaded, err := Load()
+	require.NoError(t, err)
+	assert.Equal(t, "inst_live_headless_secret", loaded.APIKey,
+		"a file-persisted token must be found on the next load (no false 'Not logged in')")
+	assert.True(t, loaded.IsAuthenticated())
+	assert.Equal(t, "file-fallback", loaded.SecretBackendName())
+}
+
+// TestSave_KeychainSetErrorFallsBackToFile covers the OTHER headless mode:
+// Set itself errors (no DBus / locked collection / no active backend). The
+// file fallback must still capture the token.
+func TestSave_KeychainSetErrorFallsBackToFile(t *testing.T) {
+	withSecretSeams(t,
+		func(string) error { return errors.New("keychain set: dbus unavailable") },
+		func() (string, error) { return "", errors.New("unreachable") },
+	)
+
+	dir := t.TempDir()
+	t.Setenv("HOME", dir)
+	path := filepath.Join(dir, ".instant-config")
+
+	cfg := &Config{path: path, APIKey: "inst_live_set_errored", Email: "ci@example.com"}
+	require.NoError(t, cfg.Save())
+
+	loaded, err := Load()
+	require.NoError(t, err)
+	assert.Equal(t, "inst_live_set_errored", loaded.APIKey)
+}
+
+// TestSave_DurableKeychainWriteSkipsFileFallback is the happy-path guard: when
+// the keychain accepts the write AND it round-trips on read-back, the token
+// must NOT be written to disk (no plaintext leak — the T16 P1-1 property), and
+// persistSecret reports success.
+func TestSave_DurableKeychainWriteSkipsFileFallback(t *testing.T) {
+	var stored string
+	withSecretSeams(t,
+		func(v string) error { stored = v; return nil },
+		func() (string, error) { return stored, nil }, // faithful round-trip
+	)
+
+	dir := t.TempDir()
+	t.Setenv("HOME", dir)
+	path := filepath.Join(dir, ".instant-config")
+
+	cfg := &Config{path: path, APIKey: "inst_live_durable", Email: "desktop@example.com"}
+	require.NoError(t, cfg.Save())
+
+	data, err := os.ReadFile(path)
+	require.NoError(t, err)
+	body := string(data)
+	assert.NotContains(t, body, FallbackAPIKeyField,
+		"a durable keychain write must NOT write the file-fallback field")
+	assert.NotContains(t, body, "inst_live_durable",
+		"a durable keychain write must NOT leave the token in plaintext on disk")
+	assert.Empty(t, cfg.FallbackAPIKey)
+}
+
+// TestPersistSecret_TableDrivenOutcomes exercises persistSecret's three
+// outcomes directly so each branch (Set error / readback mismatch / readback
+// error / durable success) is unambiguously covered.
+func TestPersistSecret_TableDrivenOutcomes(t *testing.T) {
+	cases := []struct {
+		name string
+		set  func(string) error
+		get  func() (string, error)
+		want bool
+	}{
+		{
+			name: "set error -> not durable",
+			set:  func(string) error { return errors.New("boom") },
+			get:  func() (string, error) { return "tok", nil },
+			want: false,
+		},
+		{
+			name: "readback mismatch -> not durable",
+			set:  func(string) error { return nil },
+			get:  func() (string, error) { return "different", nil },
+			want: false,
+		},
+		{
+			name: "readback error -> not durable",
+			set:  func(string) error { return nil },
+			get:  func() (string, error) { return "", errors.New("locked") },
+			want: false,
+		},
+		{
+			name: "faithful round-trip -> durable",
+			set:  func(string) error { return nil },
+			get:  func() (string, error) { return "tok", nil },
+			want: true,
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			withSecretSeams(t, tc.set, tc.get)
+			assert.Equal(t, tc.want, persistSecret("tok"))
+		})
+	}
+}