fix: close UX/robustness gaps found in codebase audit

internal/cli/setup.go

@@ -351,11 +351,12 @@ func oauthLoggedInProviders() map[string]bool {
 }
 
 // firstUsableProvider returns the saved provider best suited to run without
-// onboarding: the first usable (inline credential present, or no-auth/local)
-// non-local provider, else the first usable local one. It lets the CLI fall back
-// to an already-configured login when the active provider happens to lack a
-// credential, instead of re-running onboarding every launch.
+// onboarding: the first usable (inline credential, stored OAuth login, or
+// no-auth/local) non-local provider, else the first usable local one. It lets
+// the CLI fall back to an already-configured login when the active provider
+// happens to lack a credential, instead of re-running onboarding every launch.
 func firstUsableProvider(providers []config.ProviderProfile) (config.ProviderProfile, bool) {
+	logins := oauthLoggedInProviders()
 	var localFallback config.ProviderProfile
 	haveLocal := false
 	for _, profile := range providers {
@@ -371,7 +372,11 @@ func firstUsableProvider(providers []config.ProviderProfile) (config.ProviderPro
 				continue
 			}
 		}
-		if _, missing := setupMissingCredentialEnv(profile); missing {
+		// A stored OAuth login (e.g. `zero auth login xai`) is a credential too, even
+		// when the profile has no inline key / env var — mirrors setupRequired and
+		// usableSavedProviders so this fallback doesn't force onboarding for a
+		// provider the user is already authenticated with.
+		if _, missing := setupMissingCredentialEnv(profile); missing && !providerHasOAuthLogin(profile, logins) {
 			continue
 		}
 		if providerProfileIsLocal(profile) {

internal/cli/setup_fallback_test.go

@@ -1,9 +1,12 @@
 package cli
 
 import (
+	"path/filepath"
 	"testing"
+	"time"
 
 	"github.com/Gitlawb/zero/internal/config"
+	"github.com/Gitlawb/zero/internal/oauth"
 )
 
 func TestFirstUsableProviderPrefersRemoteKeyed(t *testing.T) {
@@ -66,6 +69,31 @@ func TestFirstUsableProviderSkipsUnresolvableCatalogWithoutBaseURL(t *testing.T)
 	}
 }
 
+// An OAuth-only provider (no inline key, no env var) must be selectable as a
+// fallback, matching setupRequired/usableSavedProviders — otherwise a fully
+// authenticated user gets forced back into onboarding when activeProvider
+// goes stale.
+func TestFirstUsableProviderRecognizesOAuthLogin(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "tok.json")
+	t.Setenv("ZERO_OAUTH_STORAGE", "file") // an inherited "keyring" would ignore the temp path and hit the OS keychain
+	t.Setenv("ZERO_OAUTH_TOKENS_PATH", path)
+	store, err := oauth.NewStore(oauth.StoreOptions{FilePath: path})
+	if err != nil {
+		t.Fatalf("NewStore: %v", err)
+	}
+	if err := store.Save(oauth.ProviderKey("xai"), oauth.Token{AccessToken: "tok", ExpiresAt: time.Now().Add(time.Hour)}); err != nil {
+		t.Fatalf("seed token: %v", err)
+	}
+
+	providers := []config.ProviderProfile{
+		{Name: "xai", CatalogID: "xai", APIKeyEnv: "XAI_API_KEY"}, // no inline key/env, but logged in via OAuth
+	}
+	got, ok := firstUsableProvider(providers)
+	if !ok || got.Name != "xai" {
+		t.Fatalf("want OAuth-logged-in provider (xai), got %q ok=%v", got.Name, ok)
+	}
+}
+
 func TestProviderProfileIsLocal(t *testing.T) {
 	cases := []struct {
 		name    string

internal/sessions/exec_session.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"log"
 	"strings"
+	"unicode/utf8"
 )
 
 type ExecMode string
@@ -221,11 +222,24 @@ func summarizePayload(payload any) string {
 		text = string(data)
 	}
 	if len(text) > 500 {
-		return text[:500]
+		return truncateUTF8(text, 500)
 	}
 	return text
 }
 
+// truncateUTF8 returns the longest prefix of s that is at most n bytes,
+// backing off to the nearest rune boundary so a multi-byte character isn't
+// split — a split rune here would embed invalid UTF-8 into the exec prompt.
+func truncateUTF8(s string, n int) string {
+	if len(s) <= n {
+		return s
+	}
+	for n > 0 && !utf8.RuneStart(s[n]) {
+		n--
+	}
+	return s[:n]
+}
+
 func extractText(value any) string {
 	switch typed := value.(type) {
 	case string:

internal/sessions/store_test.go

@@ -11,6 +11,7 @@ import (
 	"sync"
 	"testing"
 	"time"
+	"unicode/utf8"
 )
 
 func TestStoreCreatesAppendsListsAndReadsEvents(t *testing.T) {
@@ -640,6 +641,23 @@ func TestFormatExecPromptTruncatesConversationMessagesAfterFilteringNoise(t *tes
 	}
 }
 
+// A truncation cut at a raw byte offset can land in the middle of a
+// multi-byte UTF-8 rune (e.g. CJK text), embedding invalid UTF-8 into the
+// exec prompt. summarizePayload must back off to a rune boundary instead.
+func TestSummarizePayloadTruncatesOnRuneBoundary(t *testing.T) {
+	content := strings.Repeat("中文", 300) // 900 bytes of 3-byte runes
+	payload := json.RawMessage(fmt.Sprintf(`{"role":"user","content":%q}`, content))
+
+	got := summarizePayload(payload)
+
+	if !utf8.ValidString(got) {
+		t.Fatalf("summarizePayload produced invalid UTF-8: %q", got)
+	}
+	if len(got) > 500 {
+		t.Fatalf("expected summary to be at most 500 bytes, got %d", len(got))
+	}
+}
+
 func TestPrepareExecPersistsSpecialistMetadataForNewSession(t *testing.T) {
 	store := NewStore(StoreOptions{RootDir: t.TempDir(), Now: fixedClock("2026-06-04T14:30:00Z")})
 

internal/tui/binding_test.go

@@ -289,3 +289,122 @@ func TestDispatchOptionO(t *testing.T) {
 		t.Errorf("model.keyMatch should NOT match ctrl+o when option+o is configured")
 	}
 }
+
+// A configured binding equal to a reserved hardcoded chord must be reverted
+// to default rather than silently making the hardcoded action unreachable
+// (e.g. configuring toggleDetailed to ctrl+f would otherwise swallow the
+// /model picker's "favorite" shortcut).
+func TestSanitizeKeyBindingsDropsReservedCollision(t *testing.T) {
+	cfg := config.KeyBindingsConfig{
+		ToggleDetailed: "ctrl+f", // collides with the hardcoded favorite-model shortcut
+		ToggleMouse:    "ctrl+e", // unaffected, should survive untouched
+	}
+	sanitized, warnings := sanitizeKeyBindings(resolveKeyBindings(cfg))
+
+	if !sanitized.toggleDetailed.isZero() {
+		t.Errorf("toggleDetailed should be reverted to default, got %q", sanitized.toggleDetailed.Label())
+	}
+	if sanitized.toggleMouse.isZero() || sanitized.toggleMouse.Label() != "Ctrl+E" {
+		t.Errorf("toggleMouse should be untouched, got %q", sanitized.toggleMouse.Label())
+	}
+	if len(warnings) != 1 {
+		t.Fatalf("want exactly 1 warning, got %d: %v", len(warnings), warnings)
+	}
+}
+
+// Two configurable actions bound to the same chord must not both fire —
+// sanitizeKeyBindings should keep the first and revert the rest to default.
+func TestSanitizeKeyBindingsDropsMutualCollision(t *testing.T) {
+	cfg := config.KeyBindingsConfig{
+		ToggleDetailed: "ctrl+o",
+		CycleReasoning: "ctrl+o", // collides with toggleDetailed above
+	}
+	sanitized, warnings := sanitizeKeyBindings(resolveKeyBindings(cfg))
+
+	if sanitized.toggleDetailed.isZero() || sanitized.toggleDetailed.Label() != "Ctrl+O" {
+		t.Errorf("toggleDetailed (first claimant) should keep ctrl+o, got %q", sanitized.toggleDetailed.Label())
+	}
+	if !sanitized.cycleReasoning.isZero() {
+		t.Errorf("cycleReasoning (second claimant) should be reverted to default, got %q", sanitized.cycleReasoning.Label())
+	}
+	if len(warnings) != 1 {
+		t.Fatalf("want exactly 1 warning, got %d: %v", len(warnings), warnings)
+	}
+}
+
+// Non-colliding configured bindings must pass through unchanged with no
+// warnings.
+func TestSanitizeKeyBindingsNoCollisions(t *testing.T) {
+	cfg := config.KeyBindingsConfig{
+		ToggleDetailed: "option+o",
+		ToggleSidebar:  "option+b",
+	}
+	sanitized, warnings := sanitizeKeyBindings(resolveKeyBindings(cfg))
+
+	if len(warnings) != 0 {
+		t.Fatalf("want no warnings, got %v", warnings)
+	}
+	if sanitized.toggleDetailed.Label() != "Alt+O" || sanitized.toggleSidebar.Label() != "Alt+B" {
+		t.Fatalf("bindings should be unchanged: toggleDetailed=%q toggleSidebar=%q",
+			sanitized.toggleDetailed.Label(), sanitized.toggleSidebar.Label())
+	}
+}
+
+// A configured binding must not shadow another action that is still on its
+// built-in default chord.
+func TestSanitizeKeyBindingsDropsCollisionWithOtherDefault(t *testing.T) {
+	cfg := config.KeyBindingsConfig{
+		ToggleDetailed: "ctrl+t", // collides with cycleReasoning default
+	}
+	sanitized, warnings := sanitizeKeyBindings(resolveKeyBindings(cfg))
+
+	if !sanitized.toggleDetailed.isZero() {
+		t.Errorf("toggleDetailed should be reverted to default, got %q", sanitized.toggleDetailed.Label())
+	}
+	if !sanitized.cycleReasoning.isZero() {
+		t.Errorf("cycleReasoning should remain default (zero), got %q", sanitized.cycleReasoning.Label())
+	}
+	if len(warnings) != 1 {
+		t.Fatalf("want exactly 1 warning, got %d: %v", len(warnings), warnings)
+	}
+}
+
+// A reversion to default made during one collision pass must be
+// re-evaluated against entries already processed earlier in that same pass:
+// toggleMouse="ctrl+p" doesn't collide with togglePlan while togglePlan is
+// still explicitly "ctrl+t", but once togglePlan reverts to its own default
+// of ctrl+p (because ctrl+t collides with cycleReasoning's default),
+// toggleMouse's explicit ctrl+p would otherwise silently shadow it.
+func TestSanitizeKeyBindingsResolvesChainedDefaultCollision(t *testing.T) {
+	cfg := config.KeyBindingsConfig{
+		ToggleMouse: "ctrl+p", // collides with togglePlan's default once togglePlan reverts
+		TogglePlan:  "ctrl+t", // collides with cycleReasoning's default, reverts to togglePlan's own default (ctrl+p)
+	}
+	sanitized, warnings := sanitizeKeyBindings(resolveKeyBindings(cfg))
+
+	if !sanitized.toggleMouse.isZero() {
+		t.Errorf("toggleMouse should be reverted to default once togglePlan adopts ctrl+p, got %q", sanitized.toggleMouse.Label())
+	}
+	if !sanitized.togglePlan.isZero() {
+		t.Errorf("togglePlan should be reverted to default, got %q", sanitized.togglePlan.Label())
+	}
+	if len(warnings) != 2 {
+		t.Fatalf("want exactly 2 warnings, got %d: %v", len(warnings), warnings)
+	}
+}
+
+// Bare hardcoded keys handled directly in updateModel (e.g. Tab navigation)
+// are reserved too and must not be shadowed by configurable bindings.
+func TestSanitizeKeyBindingsDropsBareHardcodedCollision(t *testing.T) {
+	cfg := config.KeyBindingsConfig{
+		ToggleDetailed: "tab",
+	}
+	sanitized, warnings := sanitizeKeyBindings(resolveKeyBindings(cfg))
+
+	if !sanitized.toggleDetailed.isZero() {
+		t.Errorf("toggleDetailed should be reverted to default, got %q", sanitized.toggleDetailed.Label())
+	}
+	if len(warnings) != 1 {
+		t.Fatalf("want exactly 1 warning, got %d: %v", len(warnings), warnings)
+	}
+}

internal/tui/keybindings.go

@@ -1,6 +1,7 @@
 package tui
 
 import (
+	"fmt"
 	"strings"
 	"unicode/utf8"
 
@@ -263,3 +264,114 @@ func (m model) keyMatch(b parsedBinding, msg tea.KeyMsg, defaultFn func(tea.KeyM
 	}
 	return defaultFn(msg)
 }
+
+// reservedBindings lists hardcoded (non-configurable) chords handled directly in
+// model.go's key dispatch. If a configurable binding uses one of these chords,
+// one of the actions becomes unreachable (depending on switch order), so
+// sanitizeKeyBindings reverts the configurable binding back to its default.
+var reservedBindings = []struct {
+	binding     parsedBinding
+	description string
+}{
+	{parseBinding("ctrl+c"), "cancel / exit"},
+	{parseBinding("esc"), "cancel / close"},
+	{parseBinding("enter"), "submit"},
+	{parseBinding("shift+tab"), "cycle permission mode"},
+	{parseBinding("tab"), "navigation / completion"},
+	{parseBinding("backspace"), "composer edit / attachment removal"},
+	{parseBinding("up"), "history/navigation"},
+	{parseBinding("down"), "history/navigation"},
+	{parseBinding("pgup"), "transcript scroll"},
+	{parseBinding("pgdown"), "transcript scroll"},
+	{parseBinding("ctrl+f"), "favorite model (in the /model picker)"},
+	{parseBinding("?"), "help overlay"},
+}
+
+// sanitizeKeyBindings drops (reverts to default) any configured binding that
+// collides with a reserved hardcoded chord above, or with another
+// configured binding, since either collision would silently make one of the
+// two actions permanently unreachable. Returns the sanitized bindings plus a
+// human-readable warning for each dropped binding, for the caller to surface
+// as a startup notice.
+func sanitizeKeyBindings(b keyBindings) (keyBindings, []string) {
+	entries := []struct {
+		name           string
+		binding        *parsedBinding
+		defaultBinding parsedBinding
+	}{
+		{"toggleDetailed", &b.toggleDetailed, parseBinding("ctrl+o")},
+		{"toggleMouse", &b.toggleMouse, parseBinding("ctrl+e")},
+		{"cycleReasoning", &b.cycleReasoning, parseBinding("ctrl+t")},
+		{"togglePlan", &b.togglePlan, parseBinding("ctrl+p")},
+		{"toggleSidebar", &b.toggleSidebar, parseBinding("ctrl+b")},
+	}
+
+	// Each pass below can revert a binding to its default, which can newly
+	// collide with another entry that was already checked earlier in the
+	// same pass (e.g. toggleMouse="ctrl+p" doesn't collide with togglePlan
+	// while togglePlan="ctrl+t" is explicit, but once togglePlan reverts to
+	// its own default of ctrl+p later in the same pass, toggleMouse's
+	// explicit ctrl+p now silently shadows it). Repeat all three checks to a
+	// fixed point so a reversion is always re-evaluated against the others.
+	var warnings []string
+	for {
+		changed := false
+
+		for _, e := range entries {
+			if e.binding.isZero() {
+				continue
+			}
+			for _, other := range entries {
+				if other.name == e.name || !other.binding.isZero() {
+					continue
+				}
+				if *e.binding == other.defaultBinding {
+					warnings = append(warnings, fmt.Sprintf(
+						"keybindings.%s (%s) conflicts with keybindings.%s default (%s); using the default instead.",
+						e.name, e.binding.Label(), other.name, other.defaultBinding.Label()))
+					*e.binding = parsedBinding{}
+					changed = true
+					break
+				}
+			}
+		}
+
+		for _, e := range entries {
+			if e.binding.isZero() {
+				continue
+			}
+			for _, reserved := range reservedBindings {
+				if *e.binding == reserved.binding {
+					warnings = append(warnings, fmt.Sprintf(
+						"keybindings.%s (%s) conflicts with the built-in %s shortcut; using the default instead.",
+						e.name, e.binding.Label(), reserved.description))
+					*e.binding = parsedBinding{}
+					changed = true
+					break
+				}
+			}
+		}
+
+		claimedBy := map[parsedBinding]string{}
+		for _, e := range entries {
+			if e.binding.isZero() {
+				continue
+			}
+			if other, ok := claimedBy[*e.binding]; ok {
+				warnings = append(warnings, fmt.Sprintf(
+					"keybindings.%s (%s) conflicts with keybindings.%s; using the default instead.",
+					e.name, e.binding.Label(), other))
+				*e.binding = parsedBinding{}
+				changed = true
+				continue
+			}
+			claimedBy[*e.binding] = e.name
+		}
+
+		if !changed {
+			break
+		}
+	}
+
+	return b, warnings
+}