fix: address bugs found in a multi-agent 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/oauth/encrypt.go

@@ -122,17 +122,17 @@ func createSecretFile(path string) ([]byte, error) {
 		if !errors.Is(err, os.ErrExist) && !errors.Is(err, os.ErrPermission) {
 			return nil, fmt.Errorf("oauth: create token secret lock: %w", err)
 		}
+		// Remember the lock-creation error itself rather than a subsequent
+		// "secret file doesn't exist yet" read error -- otherwise a real
+		// contention/ACL problem is masked by the expected-while-waiting
+		// ErrNotExist once the retries are exhausted.
+		lastErr = err
 		if data, rerr := readSecretFileRetry(path); rerr == nil {
 			return data, nil
-		} else {
-			lastErr = rerr
 		}
 		time.Sleep(secretRetryDelay)
 	}
-	if lastErr != nil {
-		return nil, fmt.Errorf("oauth: timed out waiting for token secret %s: %w", path, lastErr)
-	}
-	return nil, fmt.Errorf("oauth: timed out waiting for token secret lock %s", lockPath)
+	return nil, fmt.Errorf("oauth: timed out waiting for token secret %s: %w", path, lastErr)
 }
 
 func writeNewSecretFile(path string) ([]byte, error) {

internal/sandbox/grant_scope.go

@@ -200,27 +200,38 @@ func normalizeHostScope(raw string) string {
 // directory itself or any descendant; a host grant matches its exact normalized
 // host. A narrower grant never covers a tool-wide request (reqScope == ""), so
 // such a request re-prompts (fail-safe).
+//
+// File and dir comparisons go through filepath.Rel (via scopePathEqual and the
+// shared pathWithinRoot helper) rather than a plain string comparison, so they
+// apply the same case-folding as the workspace-boundary check on Windows --
+// otherwise a persisted grant (including a deny) could be silently bypassed by
+// spelling the same path with different case.
 func grantCovers(grant Grant, reqScope string) bool {
 	switch grant.ScopeKind {
 	case ScopeToolWide:
 		return true
 	case ScopeFile:
-		return reqScope != "" && reqScope == grant.Scope
+		return reqScope != "" && scopePathEqual(grant.Scope, reqScope)
 	case ScopeDir:
 		if reqScope == "" || grant.Scope == "" {
 			return false
 		}
-		if reqScope == grant.Scope {
-			return true
-		}
-		return strings.HasPrefix(reqScope, grant.Scope+string(filepath.Separator))
+		return pathWithinRoot(grant.Scope, reqScope)
 	case ScopeHost:
 		return reqScope != "" && normalizeHostScope(reqScope) == normalizeHostScope(grant.Scope)
 	default:
 		return false
 	}
 }
 
+// scopePathEqual reports whether two absolute, cleaned scope paths refer to
+// the same file, applying the same case-folding filepath.Rel already uses for
+// directory-boundary checks (case-insensitive path components on Windows).
+func scopePathEqual(a, b string) bool {
+	rel, err := filepath.Rel(a, b)
+	return err == nil && rel == "."
+}
+
 // scopeSpecificity ranks scope kinds so the most precise covering allow wins when
 // several grants match the same request.
 func scopeSpecificity(kind ScopeKind) int {

internal/sandbox/grant_scope_test.go

@@ -2,6 +2,8 @@ package sandbox
 
 import (
 	"path/filepath"
+	"runtime"
+	"strings"
 	"testing"
 )
 
@@ -234,3 +236,25 @@ func TestGrantCovers(t *testing.T) {
 		})
 	}
 }
+
+// TestGrantCoversCaseInsensitiveOnWindows guards against a persisted grant
+// (including a deny) being silently bypassed by spelling the same path with
+// different case, since Windows path components are case-insensitive.
+func TestGrantCoversCaseInsensitiveOnWindows(t *testing.T) {
+	if runtime.GOOS != "windows" {
+		t.Skip("path case-insensitivity is a Windows-specific filesystem property")
+	}
+	dir := filepath.Join(string(filepath.Separator)+"proj", "src")
+	file := filepath.Join(dir, "main.go")
+	descendant := filepath.Join(dir, "api", "z.go")
+
+	fileGrant := Grant{Scope: file, ScopeKind: ScopeFile}
+	dirGrant := Grant{Scope: dir, ScopeKind: ScopeDir}
+
+	if !grantCovers(fileGrant, strings.ToUpper(file)) {
+		t.Fatalf("file grant %q should cover differently-cased request %q on Windows", file, strings.ToUpper(file))
+	}
+	if !grantCovers(dirGrant, strings.ToUpper(descendant)) {
+		t.Fatalf("dir grant %q should cover differently-cased descendant %q on Windows", dir, strings.ToUpper(descendant))
+	}
+}

internal/securefile/securefile.go

@@ -26,6 +26,8 @@ const (
 	secretRetryDelay    = 2 * time.Millisecond
 )
 
+var openSecretLockFile = os.OpenFile
+
 // Crypter encrypts a blob at rest with AES-256-GCM under a per-user random secret
 // persisted (0600) at secretPath.
 type Crypter struct {
@@ -107,7 +109,7 @@ func createSecretFile(path string) ([]byte, error) {
 	lockPath := path + ".lock"
 	var lastErr error
 	for attempt := 0; attempt < secretRetryAttempts; attempt++ {
-		lock, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600)
+		lock, err := openSecretLockFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600)
 		if err == nil {
 			_ = lock.Close()
 			defer os.Remove(lockPath)
@@ -118,20 +120,25 @@ func createSecretFile(path string) ([]byte, error) {
 			}
 			return writeNewSecretFile(path)
 		}
-		if !errors.Is(err, os.ErrExist) {
+		// On Windows a concurrent holder's os.Remove leaves the lock file in a
+		// "delete pending" state, so an O_EXCL create races it with
+		// ERROR_ACCESS_DENIED (os.ErrPermission) rather than ErrExist. Treat that
+		// as contention and retry too -- mirroring oauth's createSecretFile --
+		// otherwise concurrent secret creation spuriously fails on Windows.
+		if !errors.Is(err, os.ErrExist) && !errors.Is(err, os.ErrPermission) {
 			return nil, fmt.Errorf("securefile: create secret lock: %w", err)
 		}
+		// Remember the lock-creation error itself rather than a subsequent
+		// "secret file doesn't exist yet" read error -- otherwise a real
+		// contention/ACL problem is masked by the expected-while-waiting
+		// ErrNotExist once the retries are exhausted.
+		lastErr = err
 		if data, rerr := readSecretFileRetry(path); rerr == nil {
 			return data, nil
-		} else {
-			lastErr = rerr
 		}
 		time.Sleep(secretRetryDelay)
 	}
-	if lastErr != nil {
-		return nil, fmt.Errorf("securefile: timed out waiting for secret %s: %w", path, lastErr)
-	}
-	return nil, fmt.Errorf("securefile: timed out waiting for secret lock %s", lockPath)
+	return nil, fmt.Errorf("securefile: timed out waiting for secret %s: %w", path, lastErr)
 }
 
 func writeNewSecretFile(path string) ([]byte, error) {

internal/securefile/securefile_test.go

@@ -7,6 +7,33 @@ import (
 	"testing"
 )
 
+func TestCreateSecretFileRetriesOnPermissionContention(t *testing.T) {
+	secretPath := filepath.Join(t.TempDir(), "k.secret")
+	attempts := 0
+	origOpen := openSecretLockFile
+	openSecretLockFile = func(path string, flag int, perm os.FileMode) (*os.File, error) {
+		attempts++
+		if attempts == 1 {
+			return nil, os.ErrPermission
+		}
+		return os.OpenFile(path, flag, perm)
+	}
+	t.Cleanup(func() {
+		openSecretLockFile = origOpen
+	})
+
+	secret, err := createSecretFile(secretPath)
+	if err != nil {
+		t.Fatalf("createSecretFile should retry on permission contention: %v", err)
+	}
+	if len(secret) != secretBytes {
+		t.Fatalf("secret length = %d, want %d", len(secret), secretBytes)
+	}
+	if attempts < 2 {
+		t.Fatalf("expected at least 2 lock attempts, got %d", attempts)
+	}
+}
+
 func TestSealOpenRoundTrip(t *testing.T) {
 	secret := filepath.Join(t.TempDir(), "k.secret")
 	c := NewCrypter(secret)

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/tools/bash_tool_test.go

@@ -190,6 +190,33 @@ func TestDetectShellCommandIssueAllowsUnrelatedCommands(t *testing.T) {
 	}
 }
 
+// Operator/utility-name text that only appears inside a quoted argument must
+// not be mistaken for real shell syntax — e.g. `echo "foo; ls -la"` is a
+// harmless echo, not a bash-style `ls` invocation, and quoted `head`/`grep`
+// text isn't a piped POSIX utility.
+func TestDetectShellCommandIssueIgnoresQuotedContent(t *testing.T) {
+	for _, command := range []string{
+		`echo "foo; ls -la"`,
+		`echo "run head over the file"`,
+		`echo 'ls -la is a common bash command'`,
+		`echo "grep for the pattern"`,
+	} {
+		if issue := detectShellCommandIssue(command, "windows"); issue != nil {
+			t.Fatalf("expected quoted content to be ignored for %q, got %#v", command, issue)
+		}
+	}
+
+	// Sanity: the same utility names still get flagged when they're NOT quoted.
+	for _, command := range []string{
+		`echo "foo" ; ls -la`,
+		`git log | head`,
+	} {
+		if issue := detectShellCommandIssue(command, "windows"); issue == nil {
+			t.Fatalf("expected unquoted shell syntax to still be flagged for %q", command)
+		}
+	}
+}
+
 func TestDetectShellOutputIssueAddsWindowsSyntaxHint(t *testing.T) {
 	issue := detectShellOutputIssue(`cd /d/tmp/zero-pr-158 && ls -la`, "The syntax of the command is incorrect.", "windows")
 	if issue == nil {