Skip to content

UX/robustness fixes from the codebase audit (#480): OAuth fallback, UTF-8 truncation, keybinding collisions, Windows rename retry #546

Description

@PierrunoYT

Summary

Split out of #480 / #481 per review feedback on #481: grouping the UX/robustness findings from the multi-agent codebase audit separately from the security-sensitive ones so each can be reviewed and landed independently.

This covers 4 of the 9 original findings:

  1. Provider fallback ignores OAuth loginsinternal/cli/setup.go (firstUsableProvider). Only checked inline API keys/env vars, never OAuth logins, contradicting setupRequired's own logic. If activeProvider went stale/blank, a fully-authenticated OAuth-only provider (e.g. zero auth login xai) was skipped and the user got forced back into onboarding despite having working credentials.
  2. UTF-8 truncation can corrupt resumed session contextinternal/sessions/exec_session.go, summarizePayload truncated with a byte-index (text[:500]), which can split a multi-byte UTF-8 rune. Feeds directly into exec prompts for --resume/--fork.
  3. Keybinding collisions silently shadow hardcoded shortcutsinternal/tui/model.go. Dispatch is a priority-ordered switch with configurable bindings interleaved with hardcoded ones and zero collision detection. E.g. configuring toggleDetailed to ctrl+f permanently killed the "favorite model" shortcut.
  4. No retry on the restore-on-failure renameinternal/update/replace_windows.go. If installing the new binary failed and the restore rename back to the original also failed, replaceBinary gave up immediately, risking a permanently missing binary on a transient Windows file lock (antivirus/indexer scanning the just-renamed file).

Dropped from this split

The ninth original finding — the Windows shell-syntax pre-flight check not being quote-aware (internal/tools/shell_runtime.go) — needs no further action here. #476 landed on main independently since #481 was opened and already replaced that check with a more thorough segment/word-anchored implementation that resolves the same quoted-text false-positive.

Fix

PR: (to be linked)

All four fixes ship with regression tests. go build ./... and go test ./internal/cli/... ./internal/sessions/... ./internal/tui/... ./internal/update/... pass.

Scope note

Originally reported and fixed together in #481, which is being split into smaller, independently-reviewable PRs per maintainer request.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions