fix: close UX/robustness gaps found in codebase audit - #548
Closed
PierrunoYT wants to merge 2 commits into
Closed
Conversation
Split out of Gitlawb#481 per review feedback, grouping the UX/robustness findings from issue Gitlawb#480 (a multi-agent codebase audit) separately from the security-sensitive ones: - cli: firstUsableProvider ignored OAuth logins when picking a fallback provider, forcing unnecessary re-onboarding for an already-authenticated user - sessions: exec prompt summarization truncated on a raw byte offset, which could split a multi-byte UTF-8 rune and embed invalid UTF-8 into a resumed session's prompt - tui: configurable keybindings had no collision detection, so a remapped chord could silently make a hardcoded shortcut (or another configured binding) permanently unreachable - update: replaceBinary's restore-on-failure rename had no retry, risking a permanently missing binary on a transient Windows file lock Note: the ninth original finding (the Windows shell-syntax pre-flight check not being quote-aware, internal/tools/shell_runtime.go) is dropped from this split entirely -- Gitlawb#476 landed on main independently and already replaced that check with a more thorough segment/word- anchored implementation that resolves the same false-positive. All fixes ship with regression tests. Refs Gitlawb#480.
4 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses four UX/robustness issues from the codebase audit (#546): making provider fallback respect stored OAuth logins, ensuring session prompt summarization doesn’t corrupt UTF-8, preventing silent TUI keybinding collisions, and improving Windows self-update robustness by retrying restore renames.
Changes:
- CLI: treat stored OAuth logins as usable credentials when selecting a fallback provider.
- Sessions: truncate exec prompt summaries on UTF-8 rune boundaries (not raw byte offsets) and add regression coverage.
- TUI + Update: sanitize user keybindings to avoid collisions and add a Windows rename-restore retry path (with tests).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/cli/setup.go | Includes OAuth login state in firstUsableProvider fallback selection. |
| internal/cli/setup_fallback_test.go | Adds regression test ensuring OAuth-only providers can be selected as fallback. |
| internal/sessions/exec_session.go | Introduces UTF-8-safe truncation via truncateUTF8 for exec prompt summarization. |
| internal/sessions/store_test.go | Adds regression test asserting summarizePayload returns valid UTF-8 and respects the 500-byte cap. |
| internal/tui/model.go | Applies sanitized keybindings and surfaces collision warnings as startup notices. |
| internal/tui/keybindings.go | Adds reserved-chord list and sanitizeKeyBindings collision detection/reversion logic. |
| internal/tui/binding_test.go | Adds tests covering reserved collisions, mutual collisions, and collisions with other defaults. |
| internal/update/replace_windows.go | Adds renameWithRetry for restore-on-failure robustness under transient Windows rename failures. |
| internal/update/replace_windows_test.go | Adds Windows-only tests for replaceBinary and renameWithRetry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+343
to
+358
| 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{} | ||
| continue | ||
| } | ||
| claimedBy[*e.binding] = e.name | ||
| } | ||
|
|
||
| return b, warnings |
Comment on lines
+41
to
+48
| for attempt := 0; attempt < restoreRenameRetryAttempts; attempt++ { | ||
| if err := os.Rename(oldPath, newPath); err == nil { | ||
| return nil | ||
| } else { | ||
| lastErr = err | ||
| } | ||
| time.Sleep(restoreRenameRetryDelay) | ||
| } |
WalkthroughThis PR updates provider fallback to recognize stored OAuth logins, sanitizes TUI key bindings for collisions and reserved chords, truncates session payload summaries on UTF-8 rune boundaries, and retries Windows binary renames during replacement. Each area includes added or updated tests. ChangesOAuth-login-aware provider fallback
Key binding collision sanitization
UTF-8 safe payload truncation
Windows binary restore retry
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Setup as firstUsableProvider
participant OAuthStore as oauthLoggedInProviders
participant Profile as Provider profile
Setup->>OAuthStore: fetch stored OAuth logins
OAuthStore-->>Setup: logins set
Setup->>Profile: check setupMissingCredentialEnv
alt credentials missing and no OAuth login
Setup->>Setup: skip provider
else OAuth login found or credentials present
Setup-->>Setup: select provider as usable
end
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/update/replace_windows.go (1)
26-37: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider retrying the initial install rename too, not just the restore.
The comment attributes the failure to a transient Windows lock (AV/indexer scanning, lingering handle). That same transient condition is just as likely to hit the initial
os.Rename(newPath, targetPath)as the restore rename, yet only the restore path gets retried. Wrapping the install rename withrenameWithRetryas well would reduce spurious update failures without adding meaningfully more risk.♻️ Symmetric retry on install
- if err := os.Rename(newPath, targetPath); err != nil { + if err := renameWithRetry(newPath, targetPath); err != nil { // Retry the restore: a transient Windows file lock (antivirus/indexer🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/update/replace_windows.go` around lines 26 - 37, The initial install rename in replaceWindowsBinary currently uses os.Rename(newPath, targetPath) without retry, while only the restore path uses renameWithRetry. Update the replaceWindowsBinary flow to wrap the first rename in renameWithRetry as well, using the same retry behavior already applied for restoring oldPath, so transient Windows file locks are handled symmetrically.internal/update/replace_windows_test.go (1)
39-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test exercises
replaceBinary's install-fails → restore-retry branch.Existing tests cover
renameWithRetryin isolation andreplaceBinary's happy path, but nothing drives the actual combined-error path inreplaceBinary(lines 26-37 of replace_windows.go) where the install rename fails and the restore is retried. A test that openstargetPathexclusively (denying rename) before callingreplaceBinary, then asserts the error mentions both failures and thatoldPathstill holds the original bytes, would validate the new behavior end-to-end rather than just the retry primitive.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/update/replace_windows_test.go` around lines 39 - 65, The current tests do not cover the combined failure path in replaceBinary where the install rename fails and the restore is retried. Add an end-to-end test around replaceBinary that holds targetPath open exclusively to force the initial rename failure, then verify the returned error includes both the install and restore failures. Also assert oldPath still contains the original bytes after the retry path, using replaceBinary and renameWithRetry as the key symbols to locate the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/keybindings.go`:
- Around line 296-359: The collision cleanup in sanitizeKeyBindings is
order-dependent, so chained conflicts like toggleMouse and togglePlan can still
end up sharing the same chord after one binding is reset to its default. Rework
sanitizeKeyBindings to resolve bindings to a fixed point (or precompute
effective chords before comparing) so every conflict is re-evaluated after any
default adoption. Make sure the final conflict pass considers bindings that were
just reset, not only non-zero entries, and add a regression test covering the
toggleMouse/togglePlan chained-default case.
In `@internal/update/replace_windows.go`:
- Around line 26-37: The rollback copy is being lost because
CleanupStaleBinary(executablePath) can run after a failed replace and delete the
only preserved <binary>.old backup. Update Apply and the Windows replace flow in
replace_windows.go so stale-binary cleanup only happens when targetPath still
exists, or move the cleanup to after a successful install path. Use the existing
replaceWithRetry/renameWithRetry logic as the place to preserve the backup until
the replacement succeeds.
---
Nitpick comments:
In `@internal/update/replace_windows_test.go`:
- Around line 39-65: The current tests do not cover the combined failure path in
replaceBinary where the install rename fails and the restore is retried. Add an
end-to-end test around replaceBinary that holds targetPath open exclusively to
force the initial rename failure, then verify the returned error includes both
the install and restore failures. Also assert oldPath still contains the
original bytes after the retry path, using replaceBinary and renameWithRetry as
the key symbols to locate the behavior.
In `@internal/update/replace_windows.go`:
- Around line 26-37: The initial install rename in replaceWindowsBinary
currently uses os.Rename(newPath, targetPath) without retry, while only the
restore path uses renameWithRetry. Update the replaceWindowsBinary flow to wrap
the first rename in renameWithRetry as well, using the same retry behavior
already applied for restoring oldPath, so transient Windows file locks are
handled symmetrically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0aeacfb9-ab92-4045-abf3-8f9c7e5b2bb0
📒 Files selected for processing (9)
internal/cli/setup.gointernal/cli/setup_fallback_test.gointernal/sessions/exec_session.gointernal/sessions/store_test.gointernal/tui/binding_test.gointernal/tui/keybindings.gointernal/tui/model.gointernal/update/replace_windows.gointernal/update/replace_windows_test.go
- tui: sanitizeKeyBindings' collision passes were order-dependent, so a reversion made mid-pass (e.g. togglePlan reverting to its own default of ctrl+p because ctrl+t collided with cycleReasoning's default) could newly collide with an entry already checked earlier in that same pass (toggleMouse's explicit ctrl+p), leaving both bound to the same chord undetected. Loop all three collision checks to a fixed point instead of running each once. Regression test added for the toggleMouse/togglePlan chained case. - update: CleanupStaleBinary unconditionally removed "<binary>.old" on every Apply() call. If a previous replaceBinary call failed to both install the new binary and restore the original, targetPath is left missing and oldPath is the only surviving working copy -- the next Apply() would delete it before the caller has any chance to recover. Guard cleanup on targetPath still existing. - update: replaceBinary only retried the restore-on-failure rename, not the initial install rename, though both are subject to the same transient Windows file lock (antivirus/indexer). Retry both via renameWithRetry. Also stopped renameWithRetry from sleeping after its final (already-failed) attempt, and dropped the needless else block CodeRabbit flagged alongside it. Regression tests added for the CleanupStaleBinary guard and the install-fails/restore-succeeds branch of replaceBinary. Skipped the suggested test for the double-failure (install AND restore both fail) branch: reliably forcing two independent os.Rename calls to fail in-process without a flaky low-level Windows locking hack isn't worth the fragility for a nitpick-level ask.
Contributor
Author
|
Addressed in 86125b7:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/update/replace_windows_test.go (1)
43-59: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid real retry sleeps in this test.
replaceBinarywill spend ~900ms inrenameWithRetrybackoff here (9 sleeps × 100ms) before it even reaches the restore path, so this adds a real delay to every test run. Inject the delay/backoff for tests or make the retry wait overrideable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/update/replace_windows_test.go` around lines 43 - 59, The replaceBinary test is incurring real backoff time because renameWithRetry uses fixed retry sleeps before the restore path is reached. Update replaceBinary/renameWithRetry so the retry delay is injectable or overrideable in tests, and adjust TestReplaceBinaryRestoresOriginalWhenInstallFails to use the fast test delay instead of waiting on the default sleep behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/update/replace_windows_test.go`:
- Around line 43-59: The replaceBinary test is incurring real backoff time
because renameWithRetry uses fixed retry sleeps before the restore path is
reached. Update replaceBinary/renameWithRetry so the retry delay is injectable
or overrideable in tests, and adjust
TestReplaceBinaryRestoresOriginalWhenInstallFails to use the fast test delay
instead of waiting on the default sleep behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 611effb0-166f-437b-a92d-1ea13297fa03
📒 Files selected for processing (4)
internal/tui/binding_test.gointernal/tui/keybindings.gointernal/update/replace_windows.gointernal/update/replace_windows_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/tui/binding_test.go
- internal/tui/keybindings.go
- internal/update/replace_windows.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #546. Split out of #481 per review feedback, grouping the UX/robustness findings from #480 separately from the security-sensitive ones (see #547 for those).
firstUsableProviderignored OAuth logins when picking a fallback provider, forcing unnecessary re-onboarding for an already-authenticated userreplaceBinary's restore-on-failure rename had no retry, risking a permanently missing binary on a transient Windows file lockDropped from this split: the ninth original finding (Windows shell-syntax pre-flight check not being quote-aware,
internal/tools/shell_runtime.go) needs no further action — #476 landed onmainindependently since #481 was opened and already replaced that check with a more thorough segment/word-anchored implementation that resolves the same false-positive.Test plan
go build ./...cleango test ./internal/cli/... ./internal/sessions/... ./internal/tui/... ./internal/update/...passesSummary by CodeRabbit