Skip to content

fix: Disable Keychain — Cursor/Claude refresh, Claude boot, overview Weekly - #2426

Merged
steipete merged 13 commits into
steipete:mainfrom
gmkbenjamin:fix/keychain-disabled-claude-cursor
Jul 29, 2026
Merged

fix: Disable Keychain — Cursor/Claude refresh, Claude boot, overview Weekly#2426
steipete merged 13 commits into
steipete:mainfrom
gmkbenjamin:fix/keychain-disabled-claude-cursor

Conversation

@gmkbenjamin

Copy link
Copy Markdown
Contributor

Summary

  • Keep cookie/session caches in process when Disable Keychain access is on so Cursor (and other cookie providers) can refresh without Keychain persistence.
  • Allow Claude Auto background CLI on boot when Keychain is disabled so usage loads without a manual Refresh.
  • Keep Claude overview/switcher bar on account Weekly (ignore Fable/Sonnet/Routines carve-outs on that surface).

Fixes #2425

Supersedes/extends #2409 and #2424.

Test plan

  • Enable Disable Keychain access, quit and relaunch CodexBar
  • Confirm Cursor refreshes without “session changed” errors
  • Confirm Claude shows usage on boot (no manual Refresh required)
  • Confirm Claude overview bar matches account Weekly, not Fable
  • Toggle Keychain access off→on and confirm disabled-mode in-memory cookies do not resurface
  • CI green

Made with Cursor

user and others added 5 commits July 23, 2026 14:12
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>
Co-authored-by: Cursor <cursoragent@cursor.com>
Clock rollover tests called selectDays(7) on UserDefaults.standard,
which leaked into later ControllerTests and, once wall-clock moved past
the Jul 15 fixtures, made totalCost nil under a 7-day window.

Co-authored-by: Cursor <cursoragent@cursor.com>
Local test build: keep in-memory cookie caches when Keychain is disabled,
and ignore Claude Fable/scoped weeklies on the overview switcher bar.

Co-authored-by: Cursor <cursoragent@cursor.com>
Background Auto previously gated off Claude CLI whenever Keychain access
was disabled, so cold start had no OAuth/web cookies and no CLI until a
manual Refresh. Keep CLI available on that path and skip auth-status
preflight so boot does not probe Keychain via the child process.

Co-authored-by: Cursor <cursoragent@cursor.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7d289a2c10

ℹ️ 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".

Comment thread Sources/CodexBarCore/Providers/Claude/ClaudeProviderDescriptor.swift Outdated
Comment thread Sources/CodexBarCore/UsageSnapshot+SwitcherWeeklyWindow.swift
Use isExplicitlyDisabled so the DEBUG test-process Keychain block does not
unlock background Auto CLI. Update the characterization test to expect CLI
availability when Keychain access is explicitly disabled.

Co-authored-by: Cursor <cursoragent@cursor.com>
@gmkbenjamin

Copy link
Copy Markdown
Contributor Author

CI failures: characterization tests expected background Auto CLI to stay gated when Keychain is disabled. Fixed by scoping the boot exception to KeychainAccessGate.isExplicitlyDisabled (not the DEBUG test-process block) and updating the baseline test. Pushing a follow-up commit.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 28, 2026
@clawsweeper

clawsweeper Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 28, 2026, 2:19 AM ET / 06:19 UTC.

ClawSweeper review

What this changes

This PR keeps cookie session caches in process while CodexBar Keychain access is disabled, allows Claude background CLI refresh after a login-status probe in that mode, and incorporates the account-Weekly overview behavior from related work.

Merge readiness

Blocked until real behavior proof is added - 9 items remain

Keep this PR open for maintainer review. Its cache fallback is narrowly tested, and the Claude overview portion is already covered by the merged related PR, but the remaining disabled-Keychain Claude CLI exception still changes a security-sensitive boundary without proving the spawned CLI will avoid macOS Keychain access.

Priority: P1
Reviewed head: aa70509fb01387067be421f610d54934f025438a
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The patch has focused tests, but an unresolved credential-boundary defect and absent real behavior proof keep it from merge-ready quality.
Proof confidence 🧂 unranked krab (1/6) Needs real behavior proof before merge: The PR body lists manual validation steps, but none is completed and no after-fix recording, terminal output, live result, or redacted runtime log demonstrates the Cursor refresh, Claude boot, or Keychain-toggle behavior. Add redacted evidence; updating the PR body should trigger a fresh review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦪 silver shellfish (2/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The PR body lists manual validation steps, but none is completed and no after-fix recording, terminal output, live result, or redacted runtime log demonstrates the Cursor refresh, Claude boot, or Keychain-toggle behavior. Add redacted evidence; updating the PR body should trigger a fresh review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 4 items Current branch scopes a disabled-Keychain memory fallback: The current PR head changes the Keychain cache store and its focused tests to retain only selected cookie-session entries in process while Keychain access is explicitly disabled; the test suite also asserts that OAuth cache entries are not retained by that fallback.
Claude exception still launches an external credential-capable path: The changed availability logic permits background Auto Claude CLI refresh when Keychain access is explicitly disabled after a successful auth-status probe. A successful probe establishes login state but does not, by itself, establish that the external CLI avoided macOS Keychain credentials.
Prior review identified the same boundary: The earlier inline review on this PR explains that CodexBar cannot constrain an external Claude CLI's Keychain behavior; the latest branch replaces the unconditional exception with an auth-status check, but the credential-source guarantee remains unproven.
Findings 1 actionable finding [P1] Keep disabled mode from launching a Keychain-capable CLI
Security Needs attention External CLI Keychain use is not constrained: A successful noninteractive Claude login probe does not prove that its credentials came from files rather than macOS Keychain, and CodexBar cannot prevent the spawned CLI from reading or prompting through Keychain.

How this fits together

CodexBar collects provider usage through browser cookies, OAuth credentials, and provider CLIs, then presents condensed usage in the menu bar. This PR changes the fallback paths used when the user disables CodexBar Keychain access and affects Cursor session refresh plus Claude startup refresh.

flowchart LR
  Settings[Advanced Keychain setting] --> Gate[Keychain access gate]
  Gate --> Cache[In-process cookie cache]
  Cache --> Cursor[Cursor refresh]
  Gate --> Claude[Claude background refresh]
  Claude --> Login[Noninteractive login probe]
  Login --> Usage[Claude usage]
  Usage --> Overview[Menu-bar overview]
Loading

Decision needed

Question Recommendation
Should “Disable Keychain access” continue to block all automatic Claude CLI launches, or may it allow a logged-in external Claude CLI whose Keychain behavior CodexBar cannot enforce? Keep automatic CLI gated: Preserve the existing background gate in disabled-Keychain mode until the app can prove a non-Keychain authentication source.

Why: This is a user-facing privacy and credential-boundary contract rather than a purely mechanical implementation choice; the current probe confirms login state but not the credential storage used by the spawned process.

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR body lists manual validation steps, but none is completed and no after-fix recording, terminal output, live result, or redacted runtime log demonstrates the Cursor refresh, Claude boot, or Keychain-toggle behavior. Add redacted evidence; updating the PR body should trigger a fresh review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Keep disabled mode from launching a Keychain-capable CLI (P1) - The new exception treats a successful claude auth status --json probe as sufficient to launch background Auto CLI, but that only confirms login state—not that the external CLI avoided macOS Keychain credentials or prompts. Because CodexBar cannot constrain that child process, retain the background gate until a supported non-Keychain credential source is established, or obtain an explicit maintainer decision defining the setting’s boundary.
  • Resolve security concern: External CLI Keychain use is not constrained - A successful noninteractive Claude login probe does not prove that its credentials came from files rather than macOS Keychain, and CodexBar cannot prevent the spawned CLI from reading or prompting through Keychain.
  • Resolve merge risk (P1) - With “Disable Keychain access” enabled, this PR can start an external Claude CLI process whose credential source CodexBar cannot constrain; a successful login-status probe does not prove that the process avoided macOS Keychain access or prompts.
  • Resolve merge risk (P1) - The branch’s manual test plan remains unchecked, so there is no after-fix evidence for the real Cursor refresh, Claude cold-boot, and toggle-clear paths on a local app bundle.
  • Complete next step (P2) - A maintainer must define the disabled-Keychain security boundary before any mechanical repair can safely choose whether automatic Claude CLI refresh remains allowed.
  • Improve patch quality - Resolve the disabled-Keychain external CLI boundary with a narrow code change or explicit maintainer direction.
  • Improve patch quality - Add redacted live evidence for Cursor refresh, Claude cold boot, and the off-to-on Keychain toggle path.

Findings

  • [P1] Keep disabled mode from launching a Keychain-capable CLI — Sources/CodexBarCore/Providers/Claude/ClaudeProviderDescriptor.swift:662-665
  • [high] External CLI Keychain use is not constrained — Sources/CodexBarCore/Providers/Claude/ClaudeProviderDescriptor.swift:662
Agent review details

Security

Needs attention: The PR changes a credential-boundary path by allowing an external Claude CLI to run automatically while the user has disabled Keychain access.

Review metrics

Metric Value Why it matters
Patch surface 9 files; 426 additions, 28 deletions The branch combines cache persistence, provider availability, documentation, and several test changes, so the security-boundary decision should be resolved before the full set lands.
Focused coverage 1 added test file; 3 existing test files expanded The patch has characterization coverage, but those tests do not demonstrate the external CLI behavior in a real disabled-Keychain environment.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #2425
Summary: This PR is the active candidate implementation for the consolidated disabled-Keychain Cursor/Claude report; its Weekly-overview portion overlaps with work already merged separately.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

  1. Restore the conservative background gate (recommended)
    Keep Claude Auto background CLI unavailable while Keychain access is disabled unless a supported non-Keychain authentication path can be verified.
  2. Accept the external CLI boundary deliberately
    If the setting is intended to govern only CodexBar-owned Keychain calls, document that limitation and require redacted runtime proof that the approved CLI flow does not prompt unexpectedly.

Technical review

Best possible solution:

Keep the cookie-only in-process fallback, but preserve the background Claude CLI gate until maintainers approve the setting’s scope and there is a supported proof that the allowed path cannot access or prompt through macOS Keychain; then add redacted real-runtime proof for all three reported behaviors.

Do we have a high-confidence way to reproduce the issue?

No high-confidence live reproduction is included. The code and tests make the cache and background-availability paths source-reproducible, but the PR has no redacted runtime proof from a real disabled-Keychain app session.

Is this the best way to solve the issue?

No. The in-process cookie cache is a narrow solution, but allowing the external Claude CLI based only on login status does not establish that the disabled-Keychain boundary is preserved.

Full review comments:

  • [P1] Keep disabled mode from launching a Keychain-capable CLI — Sources/CodexBarCore/Providers/Claude/ClaudeProviderDescriptor.swift:662-665
    The new exception treats a successful claude auth status --json probe as sufficient to launch background Auto CLI, but that only confirms login state—not that the external CLI avoided macOS Keychain credentials or prompts. Because CodexBar cannot constrain that child process, retain the background gate until a supported non-Keychain credential source is established, or obtain an explicit maintainer decision defining the setting’s boundary.
    Confidence: 0.82

Overall correctness: patch is incorrect
Overall confidence: 0.78

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 0611e8e634c8.

Labels

Label justifications:

  • P1: The PR can change automatic provider authentication behavior for users who explicitly disabled Keychain access.
  • merge-risk: 🚨 auth-provider: Claude background availability now depends on an external login-status probe and can change which authentication route runs at startup.
  • merge-risk: 🚨 security-boundary: The proposed exception may invoke a process whose macOS Keychain use or prompts cannot be controlled by CodexBar.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists manual validation steps, but none is completed and no after-fix recording, terminal output, live result, or redacted runtime log demonstrates the Cursor refresh, Claude boot, or Keychain-toggle behavior. Add redacted evidence; updating the PR body should trigger a fresh review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Security concerns:

  • [high] External CLI Keychain use is not constrained — Sources/CodexBarCore/Providers/Claude/ClaudeProviderDescriptor.swift:662
    A successful noninteractive Claude login probe does not prove that its credentials came from files rather than macOS Keychain, and CodexBar cannot prevent the spawned CLI from reading or prompting through Keychain.
    Confidence: 0.82

What I checked:

  • Current branch scopes a disabled-Keychain memory fallback: The current PR head changes the Keychain cache store and its focused tests to retain only selected cookie-session entries in process while Keychain access is explicitly disabled; the test suite also asserts that OAuth cache entries are not retained by that fallback. (Sources/CodexBarCore/KeychainCacheStore.swift:95, aa70509fb013)
  • Claude exception still launches an external credential-capable path: The changed availability logic permits background Auto Claude CLI refresh when Keychain access is explicitly disabled after a successful auth-status probe. A successful probe establishes login state but does not, by itself, establish that the external CLI avoided macOS Keychain credentials. (Sources/CodexBarCore/Providers/Claude/ClaudeProviderDescriptor.swift:662, aa70509fb013)
  • Prior review identified the same boundary: The earlier inline review on this PR explains that CodexBar cannot constrain an external Claude CLI's Keychain behavior; the latest branch replaces the unconditional exception with an auth-status check, but the credential-source guarantee remains unproven. (Sources/CodexBarCore/Providers/Claude/ClaudeProviderDescriptor.swift:662, 7d289a2c1028)
  • Weekly overview work is already merged separately: Related PR fix: keep Claude switcher bar on account weekly quota #2424 merged on July 28, 2026 with the stated account-Weekly overview behavior; the current PR’s listed changed files do not include the overview implementation path. (3d423b9eb70e)

Likely related people:

  • steipete: Authored the current branch’s follow-up commit that constrains the disabled-Keychain fallback, making them the clearest available routing candidate for the remaining boundary decision. (role: recent area contributor; confidence: medium; commits: aa70509fb013, 46cabf96e183; files: Sources/CodexBarCore/KeychainAccessGate.swift, Sources/CodexBarCore/Providers/Claude/ClaudeProviderDescriptor.swift)
  • gmkbenjamin: Authored the related merged Weekly-overview PR and the current combined proposal, so they are relevant for the already-landed overview portion and any follow-up evidence. (role: recent merged adjacent contributor; confidence: medium; commits: a33a66b1b28f, 5d474c42ace4; files: Tests/CodexBarTests/ClaudeBaselineCharacterizationTests.swift, Tests/CodexBarTests/KeychainCacheStoreTests.swift)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper

clawsweeper Bot commented Jul 29, 2026

Copy link
Copy Markdown

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix: Disable Keychain — Cursor/Claude refresh, Claude boot, overview Weekly 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.

@steipete
steipete merged commit 5ce21f6 into steipete:main Jul 29, 2026
15 of 17 checks passed
@steipete

Copy link
Copy Markdown
Owner

Merged after full verification, with the two owner-required constraints implemented on top of the original branch: the in-process fallback while Keychain access is disabled is scoped to cookie-cache keys only (Claude OAuth material is never retained in RAM), and background Claude CLI launches are gated behind a noninteractive availability check — background Auto cannot spawn a claude process that could prompt (the ClaudeWebFetchDeadlineTests scenario was fixed by seeding availability through the test seam, not by weakening the gate). Focused suites green; exact-head CI fully green after a musl SDK-install infra rerun (https://github.com/steipete/CodexBar/actions/runs/30397545607). Review + autoreview clean. Thanks @gmkbenjamin for the thorough original work!

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

Labels

merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Disable Keychain access breaks Claude/Cursor refresh (boot + overview)

2 participants