fix: keep cookie caches in memory when Keychain is disabled - #2409
fix: keep cookie caches in memory when Keychain is disabled#2409gmkbenjamin wants to merge 2 commits into
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: 5d474c42ac
ℹ️ 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".
|
CI failure looks unrelated to this PR:
Pushed an empty commit to retrigger CI (no admin rights to rerun the upstream workflow). |
Cursor refreshes failed with “session changed” because Keychain store/load could not persist under Disable Keychain access. Use an in-process fallback and clear it when the setting is toggled. Co-authored-by: Cursor <cursoragent@cursor.com>
16e148a to
8ff516c
Compare
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs real behavior proof before merge. Reviewed July 28, 2026, 11:05 AM ET / 15:05 UTC. ClawSweeper reviewWhat this changesAdds a process-local cache for browser-cookie sessions while Keychain access is explicitly disabled, clears it when that setting changes, and keeps OAuth and other cache categories unavailable. Merge readiness⛔ Blocked until real behavior proof is added - 3 items remain The patch is a focused fix for the Cursor half of the Keychain-disabled refresh problem and includes the follow-up correction for truthful deletion reporting. It should stay open: current main does not contain this fallback, the related umbrella issue remains open, and the contributor still needs direct after-fix proof of the Keychain toggle lifecycle before merge. Priority: P2 Review scores
Verification
How this fits togetherCodexBar refreshes browser-backed providers using cached session cookies, normally stored in its Keychain cache. The new path lets cookie session reconciliation continue during an explicitly Keychain-disabled run without persisting new credentials, then discards that process memory when the setting changes. flowchart LR
Setting[Advanced Keychain setting] --> Gate[Keychain access gate]
Refresh[Cursor browser refresh] --> Cache[Cookie session cache]
Gate --> Decision{Explicitly disabled?}
Decision -->|No| Keychain[Persistent Keychain cache]
Decision -->|Yes, cookie category| Memory[Process-local cookie cache]
Cache --> Reconcile[Session ownership check]
Reconcile --> Usage[Provider usage result]
Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the cookie-only fallback and truthful clear failure semantics, then add redacted real-app or CLI evidence for a disabled refresh followed by an off/on Keychain toggle that shows the in-memory cookie is gone. Do we have a high-confidence way to reproduce the issue? Yes in source terms: the linked report gives a concrete Cursor refresh path and the patch targets the disabled-Keychain cache persistence path. The review could not execute the macOS provider flow in this read-only environment. Is this the best way to solve the issue? Yes, provisionally: a cookie-only process-local fallback preserves the existing ownership check without widening OAuth access, and the follow-up commit avoids falsely claiming durable deletion. The missing live toggle proof is the remaining merge gate. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 02b4ba278c81. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
Closing as superseded by #2426 (just merged), which includes this fallback scoped to cookie keys per the owner decision, plus the Claude-side fixes. Thanks @gmkbenjamin — your #2409 shape is what informed the final constraint. |
2 participants
Summary
cookiecategory; OAuth and other credential categories remain unavailable.mainand drop the unrelated CI-only/dashboard commits.Fixes #2408
Test plan
swift test --filter 'KeychainCacheStoreTests|CookieHeaderCacheConditionalMutationTests'— 26 testsmake checkTZ=UTC make test— 724 selections, 61/61 groups passed, no retriesSafety
Claude Auto background CLI remains blocked while Keychain is disabled; CodexBar cannot constrain Keychain access or prompts from that external process.