Secure Augment sessions and clarify Ollama Keychain recovery - #2595
Conversation
|
Proof for head
|
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: Secure Augment sessions and clarify Ollama Keychain recovery This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f7b96a434
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "\(browserName) cookie decryption was declined in Keychain. " + | ||
| "Open the provider card and click Refresh (⌘R) to request Keychain access again." |
There was a problem hiding this comment.
When this OllamaUsageError is surfaced by the CLI usage path, the new text tells users to open the app's provider card even though codexbar usage intentionally runs provider fetches as .background and then prints error.localizedDescription; in that scenario there may be no menu UI, and the actionable recovery is the CLI cookie refresh flow with --allow-keychain-prompt. Please keep the core error description context-neutral and apply the provider-card wording only in the UI mapper/localized UI layer.
Useful? React with 👍 / 👎.
Co-authored-by: Peter Steinberger <steipete@mac-studio-sf2.local>
ClawSweeper (P1) and Codex both flagged that fetchBrowserOrRethrow swallows every fetchBrowser failure — including the specific, actionable diagnoses (safariCookieAccessDenied, browserCookieDecryptionDenied, browserCookieDecryptionDisabled) — and always substitutes the generic stale cached-cookie auth error. On a user-initiated refresh this replaces a fixable diagnosis (grant Full Disk Access, retry a declined Keychain prompt) with a misleading 'please sign in again', undoing the explicit-retry guidance shipped in steipete#2249/steipete#2595. fetchBrowserOrRethrow now re-throws an actionable browser-access error as-is instead of substituting the original cached error; a non- actionable recovery failure (no candidates found, network error, etc.) still falls back to the original error as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HqBGFKF9M8etkRpgz2bk2Z
Fixes #2072. ollama.com rotates its session cookie independently of the user's signed-in state. When a background refresh (5-minute timer) sees the cached cookie fail with an auth error, fetchAutomatic cleared the cache and unconditionally fell through to a browser cookie read to recover — but BrowserCookieAccessGate denies that read in a background context (no interactive Keychain prompt outside a user-initiated action). The browser read then returned no candidates, surfacing as the misleading 'No Ollama session cookie found. Please sign in...' error, even though the user was still signed in and the cache had simply been cleared a moment earlier by this same call. The cache stayed empty, so every subsequent background tick repeated the same failure until the next manual refresh repopulated it — the reported flicker. fetchAutomatic now re-throws the original auth error instead of falling through to a browser read when the interaction context is .background, deferring recovery to the next user-initiated refresh (which already has the explicit-retry path from #2249/#2595). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HqBGFKF9M8etkRpgz2bk2Z
1 participant
Augment session storage
Route
augment-session.jsonwrites throughCredentialFileWriter, so session cookies are staged owner-only at 0600 and atomically published. Existing files with group or other access are repaired before the first disk read.Coverage verifies new-file permissions, legacy 0644 healing, disk round-trips, and failure before publication leaving no destination or staged file behind.
Ollama Keychain recovery guidance
Replace the ambiguous manual-refresh wording with the actual app action: open the provider card and click Refresh (⌘R) to request Keychain access again. The message is updated in core, UI mapping, and all 23 generated localization catalogs.
The CLI keeps its CLI-specific recovery path and now explicitly tells users to rerun with
--allow-keychain-prompt.BrowserCookieAccessGatebehavior is unchanged; explicit retries still bypass the denial cooldown.Proof
make checkswift test --filter AugmentSessionStoreTests— 4 passedswift test --filter OllamaUIErrorMapperTests— 4 passedswift test --filter CLICookieRefreshTests— 12 passedswift test --filter OllamaUsageFetcherTests— 46 passedmake test— 787 selections across 66 groups, zero failures, retries, or timeouts--allow-keychain-promptFixes #2567
Refs #2072