Allow Claude Auto CLI on cold boot when Keychain is disabled - #2494
Allow Claude Auto CLI on cold boot when Keychain is disabled#2494gmkbenjamin wants to merge 6 commits into
Conversation
Restore CLI availability for Disable Keychain and the initial startup refresh so Claude usage loads without a manual Refresh, keep last-good snapshots quiet on gated background Auto ticks, and document the gate. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Codex review: found issues before merge. Reviewed July 31, 2026, 4:32 AM ET / 08:32 UTC. ClawSweeper reviewWhat this changesThis PR allows Claude Auto to use the installed Claude CLI on startup and later background refreshes when Disable Keychain access is enabled, preserves the stricter Keychain-enabled CLI gate, and keeps last-good Claude usage during intentionally deferred background refreshes. Merge readinessKeep this PR open for maintainer review. The focused Disable Keychain cold-boot path and real packaged-app transcript are credible, but merging intentionally permits an opaque Claude CLI child during automatic refresh without the regular Keychain-enabled establishment gate; a maintainer should explicitly confirm that this opt-out is the intended security boundary. A separate failure-handling condition also suppresses genuine no-strategy errors in Disable Keychain mode, which can recreate a silent “Not fetched yet” state when no CLI route exists. Priority: P1 Review scores
Verification
How this fits togetherCodexBar refreshes Claude usage through OAuth, browser, or local CLI strategies and then publishes the selected result to the menu and widget. The changed logic determines whether background Auto refresh may launch the Claude CLI and how a missing strategy changes the visible cached state. flowchart LR
Settings[Claude and Keychain settings] --> Refresh[Background Auto refresh]
Refresh --> Strategies[OAuth web and CLI strategies]
Strategies --> Gate[Background CLI safety gate]
Gate --> CLI[Claude CLI usage fetch]
Strategies --> Snapshot[Usage snapshot]
CLI --> Snapshot
Snapshot --> Display[Menu and widget]
Decision needed
Why: This is a product and security-boundary choice: the implementation cannot determine whether a user who opted out of CodexBar Keychain access also opted into any authentication behavior the external Claude CLI may initiate. Before merge
Findings
Agent review detailsSecurityNeeds attention: The diff intentionally broadens when CodexBar may start an opaque external Claude CLI process in the background, so a maintainer must confirm that Disable Keychain access is the intended authorization boundary. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Keep the explicit Disable Keychain cold-boot compatibility path only if maintainers affirm that it intentionally authorizes opaque background Claude CLI execution; otherwise retain a noninteractive safety check, and in either case limit quiet last-good preservation to genuinely deferred Keychain-enabled routes so unavailable Disable Keychain configurations remain diagnosable. Do we have a high-confidence way to reproduce the issue? Yes for the reported cold-start path: the supplied packaged-app transcript shows a fresh Disable Keychain launch publishing Claude usage without a manual refresh, and the source change clearly explains the prior background CLI rejection. The no-route silent-state concern is source-reproducible from the new failure predicate, though it was not run locally in this read-only review. Is this the best way to solve the issue? No: restoring the Disable Keychain cold-boot route is narrowly targeted, but the quiet-failure predicate is broader than the intentional Keychain-enabled deferral case and can suppress a real unavailable-route diagnostic. The external CLI authorization boundary also needs explicit maintainer approval. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8ef86077e70a. LabelsLabel changes:
Label justifications:
EvidenceSecurity concerns:
What 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
HistoryReview history (3 earlier review cycles)
|
Keep Keychain-enabled Only on user action behind the established-binary gate; preserve last-good quiet failure handling. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed ClawSweeper blockers on
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Summary
Fixes #2493
Related: #2425, #2426, #2408, #2241
Why
#2426 fixed cold boot with Disable Keychain, then absorbed Peter's background CLI establishment gate while merging
main, which reintroduced “Not fetched yet until Refresh” for that path. An earlier draft of this PR also treated startup like Manual Refresh when Keychain was enabled; review correctly flagged that as undoing the no-prompt safety boundary. This revision keeps the narrow Disable Keychain compatibility path only.Real behavior proof
Fresh packaged
CodexBar.appat head949812e1.Settings:
debugDisableKeychainAccess=true,claudeOAuthKeychainPromptMode=never.Cold start (
pkill+open -n), no Manual Refresh:2026-07-31T08:19:54Z— Session 100% left, Weekly 75% leftusage --provider claudematched (Session 100% / Weekly 75%)Artifact:
docs/screenshots/claude-cold-boot-disable-keychain-2026-07-31.txtTest plan
make checkswift test --filter 'ClaudeCLIBackgroundAvailabilityTests|ClaudeBackgroundAutoRefreshFailureTests'Commands run
make check(pass)Made with Cursor