Ollama: surface browser cookie access errors - #2249
Conversation
![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: 067f4617db
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| do { | ||
| return try self.selectSessionInfos(from: fallbackCandidates, logger: log) | ||
| } catch OllamaUsageError.noSessionCookie { | ||
| throw accessError ?? OllamaUsageError.noSessionCookie |
There was a problem hiding this comment.
When the default Chrome import is accessible but simply has no recognized Ollama session, fallback probing still includes Safari (which BrowserDetection.isCookieSourceAvailable(.safari) treats as available), and a Safari Full Disk Access failure captured in accessError makes this line throw the Safari permission hint instead of noSessionCookie. In that common Chrome-default scenario, the user is told to grant Safari Full Disk Access even though signing into Chrome would fix the issue, so the access error should only replace noSessionCookie when all relevant candidates were blocked by access, not when an accessible preferred browser was just unauthenticated.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦐 gold shrimp
|
Codex review: needs changes before merge. Reviewed July 16, 2026, 9:56 PM ET / July 17, 2026, 01:56 UTC. Summary Reproducibility: yes. A focused importer test can use an accessible Chrome source with no recognized Ollama session followed by Safari access denial and assert that the result remains Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Track whether any relevant browser was successfully accessible and surface a stored access error only when all relevant candidates were access-blocked; cover both that mixed-result case and manual-refresh cooldown retries with focused tests. Do we have a high-confidence way to reproduce the issue? Yes. A focused importer test can use an accessible Chrome source with no recognized Ollama session followed by Safari access denial and assert that the result remains Is this the best way to solve the issue? Not yet. Typed errors and localized mapping are appropriate, but access failures should override no-session guidance only when every relevant browser candidate was blocked. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ee84dca1a734. Label changesLabel changes:
Label justifications:
Evidence reviewedAcceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
Verified and merged.
|
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
Summary
Verification
swift test --filter OllamaUsageFetcherTests— 26 tests passedswift test --filter OllamaUIErrorMapperTests— 4 tests passednode Scripts/check-app-locales.mjs --test— passednode Scripts/check-app-locales.mjs— 22 catalogs matched 1,166 English keysmake check— SwiftFormat clean; SwiftLint 0 violations; script/docs/locale checks passedmake test— 664 selections completed; one full group recovered on retry; zero timeouts or isolated retriesRefs #2072. Keep the issue open until the reporter confirms the fix.