Fix Claude no-prompt Keychain repair#2102
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: 1af050af15
ℹ️ 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".
| } | ||
|
|
||
| @Test | ||
| func `never mode repairs a missing credentials file from a valid no-UI Keychain read`() throws { |
There was a problem hiding this comment.
This new test runs on every platform, but the repair path it asserts is inside #if os(macOS) and returns nil on non-macOS. In Linux SwiftPM test runs, loadRecord(... allowClaudeKeychainRepairWithoutPrompt: true) still throws .notFound here even with the synthetic Keychain override, so this suite will break cross-platform CI unless the suite/test is wrapped in #if os(macOS) or the test uses a platform-neutral seam.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed July 13, 2026, 5:35 AM ET / 09:35 UTC. Summary Reproducibility: yes. at source level: current main blocks the documented missing-file repair at the never-mode gate, and the focused fixture reportedly reproduces the same Review metrics: 3 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Define Do we have a high-confidence way to reproduce the issue? Yes at source level: current main blocks the documented missing-file repair at the never-mode gate, and the focused fixture reportedly reproduces the same Is this the best way to solve the issue? Yes technically, provided the caller audit is complete: distinguishing fail-closed no-UI reads is narrower than re-enabling prompts and preserves the global zero-access switch. The preference semantics still require maintainer approval. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c852c135e10c. Label changesLabel changes:
Label justifications:
Evidence reviewedSecurity concerns:
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
Review history (7 earlier review cycles)
|
1af050a to
b822b4a
Compare
|
Thanks for the thorough review. I dug into the P1 ("Block the experimental CLI reader for no-prompt callers") and I don't think it holds as stated — this PR doesn't newly route a no-prompt caller through Under the default Under the I did initially try the suggested "skip the CLI reader when If you'd still like the experimental reader's |
|
@clawsweeper re-review — the P1 (experimental CLI reader under |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
316f969 to
db7bf21
Compare
clawsweeper
Bot
added
rating: 🦪 silver shellfish
Verification update — real-macOS evidence + honest scopeI ran this on real hardware (arm64e, macOS 14) and want to lay out precisely what is proven and where the boundary is, rather than overclaim. 1. Deterministic before/after (the core of the fix)The regression test reproduces the reporter's exact failure and the patch fixes it:
This ran live (not just CI): the full 2. Real-
|
|
@clawsweeper re-review — the verification comment above adds real-macOS evidence (deterministic before/after + the no-UI read now executing under real |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦐 gold shrimp
|
@steipete — a quick note on where this stands, since the remaining gate is now on your side rather than mine. ClawSweeper's one open ask is "complete real signed-runtime before-and-after recovery." I've taken that as far as an external contributor can:
The last step — end-to-end recovery from a real If it's useful, One other thing on your side: as a first-time contributor here, CI is gated behind your workflow approval (only GitGuardian has run so far). Happy to reshape the test or proof surface however you'd prefer — just let me know. |
devYRPauli
left a comment
devYRPauli
left a comment
There was a problem hiding this comment.
LGTM. I read through the post-change source at each call site rather than the description, and the .never no-prompt boundary holds.
The key invariant - under .never, no interactive Keychain read is ever permitted - is preserved because the three interactive entry points (loadFromClaudeKeychain, loadFromClaudeKeychainWithPromptIfAllowed, and the security-framework fallback decision) keep the default allowKeychainPrompt: true, so shouldAllowClaudeCodeKeychainAccess returns !true == false for them under .never and they stay blocked. Only the guaranteed no-UI paths (missing-file repair, freshness sync, fingerprint/candidate probes) pass allowKeychainPrompt: false. The guard self.keychainAccessAllowed kill switch is untouched and still short-circuits before the mode switch.
What makes it safe rather than just plausibly safe is that the same allowKeychainPrompt drives both the policy check and the actual query: loadClaudeKeychainData applies KeychainNoUIQuery.apply(&query) when !allowKeychainPrompt (ClaudeOAuthCredentials.swift ~L2071), which sets kSecUseAuthenticationUI = kSecUseAuthenticationUIFail. So passing false produces a query that fails instead of prompting - the policy gate and the query construction can't drift apart.
I built the PR head and ran the new suite locally: all 3 tests pass, including never mode still blocks an interactive Keychain read even with a valid item present, which covers the negative direction of the boundary (a valid item is present, prompt requested, still throws .notFound under .never) - not just the happy-path repair. The logic diff is minimal (+40/-17 in one file, a defaulted parameter plus mechanical call-site threading); the rest is the test file and a standalone verification script.
Two small notes, neither blocking:
Scripts/verify_1975_live.shtemporarily writes the realcom.steipete.codexbardefaults domain (claudeOAuthKeychainPromptMode) during a live run and restores it via a trap. Non-destructive, but worth flagging in case you'd rather a verification script not touch real user prefs.- This is really the security-framing decision that was flagged for maintainer sign-off: the implementation correctly keeps interactive reads blocked under
.neverand only newly permits guaranteed no-UI reads, so the behavior change is "avoid prompts" continuing to mean "never show a prompt" rather than "never touch the Keychain." That reading looks right to me, but the policy call is yours.
|
Thanks @devYRPauli — appreciate you reading it call-site by call-site and running the suite rather than trusting the description. You've restated the invariant exactly: the three interactive entry points keep On your two notes:
Thanks again for the thorough pass. |
Co-authored-by: OfficialAbhinavSingh <abhinav.25bcs10345@sst.scaler.com>
0955012 to
ad3a67c
Compare
steipete
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad3a67cfea
ℹ️ 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".
| guard ClaudeOAuthCredentialsStore.shouldAllowClaudeCodeKeychainAccess( | ||
| mode: mode, | ||
| allowKeychainPrompt: false) else { return false } |
There was a problem hiding this comment.
When the stored mode is Never, this guard now lets the method continue, but the status switch below still treats errSecInteractionNotAllowed as success. ClaudeProviderDescriptor.isPlausiblyAvailable relies on this method before fetching, so a Claude Keychain item that requires UI is now advertised as available even though the later credential load runs with allowKeychainPrompt: false and cannot read it. In Never mode, verify the secret can be loaded noninteractively or treat interaction-required results as unavailable.
Useful? React with 👍 / 👎.
| guard ClaudeOAuthCredentialsStore | ||
| .shouldAllowClaudeCodeKeychainAccess(mode: mode, allowKeychainPrompt: false) else { return nil } |
There was a problem hiding this comment.
This opens Never mode to the repair path, but the existing cooldown check below still runs for normal background loads because loadRecord derives respectKeychainPromptCooldown from !allowKeychainPrompt. If a user has a persisted denied-prompt cooldown and then relies on Never-mode no-UI repair, shouldAllowPrompt is false and the repair returns nil even though the subsequent Keychain reads are KeychainNoUIQuery reads that cannot show a prompt. Skip the prompt cooldown for the .never/noninteractive repair case.
Useful? React with 👍 / 👎.
|
Maintainer proof for exact head
Thanks @OfficialAbhinavSingh for finding and fixing this recovery gap. |
3 participants
Fixes #1975.
Summary
When Claude's Avoid Keychain prompts preference was set to Never, CodexBar blocked every Claude Keychain read. That also blocked guaranteed no-UI Security.framework reads, so a missing
~/.claude/.credentials.jsoncould not be repaired from an otherwise valid Claude Code Keychain item.This change makes the policy boundary explicit:
/usr/bin/securityreader because that subprocess can prompt.KeychainNoUIQueryremain available and fail closed instead of showing UI.The patch also updates the existing experimental-reader expectations and removes the proposed live verification script. Routine proof now uses isolated fixture stores only; it never changes real defaults or touches the login Keychain.
Verification
swift test --filter 'ClaudeOAuthCredentialsStoreTests|ClaudeOAuthCredentialsStoreSecurityCLITests|ClaudeOAuthCredentialsStoreNeverPromptCacheTests|ClaudeOAuthCredentialsStoreIsolatedSecurityCLITests'— 59 tests passed.make check— SwiftFormat and SwiftLint clean; repository/docs/package gates passed.CODEXBAR_TEST_RETRY_NON_TIMEOUT_FAILURES=0 make test— 652/652 selections, 55/55 groups passed first attempt, zero retries and zero timeouts.No live Keychain, defaults, browser, network, or account credentials were used during validation.
Thanks @OfficialAbhinavSingh for the report and original fix.