Claude: adopt a valid credential when the delegated-refresh touch changed nothing - #2739
Conversation
When the Claude CLI's own token is still valid, the delegated-refresh touch correctly changes nothing — so the keychain-change probe reports no change and the attempt is recorded as `.attemptedFailed`. The silent re-sync was gated on `.attemptedSucceeded`, so that outcome skipped the one step that would have adopted the valid credential already sitting in the keychain. The account then stayed pinned to its expired cached token, showing an error that blamed the Keychain prompt policy, until the user pressed Refresh by hand — a user-initiated read takes a different path and recovers. syncFromClaudeKeychainWithoutPrompt passes allowKeychainPrompt: false on every read and only adopts a credential that is not expired, so running it on any outcome cannot prompt and cannot regress a working account.
waitForClaudeKeychainChange returned Bool, so "could not read the keychain" and "the keychain did not change" collapsed into the same `false`. Those need opposite responses: the second is a real refresh failure, the first says nothing at all and should stay retryable. Observation is now a tri-state. An indeterminate reading records the short cooldown and reports an accurate message instead of asserting the touch failed to update anything. The per-baseline nil semantics are preserved deliberately: Security.framework observation still treats a missing baseline as movement, while security-CLI observation still treats it as inconclusive rather than letting a later successful read masquerade as a change.
![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: 653816c54a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs real behavior proof before merge. Reviewed August 7, 2026, 6:58 PM ET / 22:58 UTC. ClawSweeper reviewWhat this changesThis PR always attempts a non-interactive Claude Keychain credential re-sync after delegated refresh and records unreadable Keychain observations separately from unchanged entries. Merge readiness⛔ Blocked until real behavior proof is added - 5 items remain Keep open, but do not merge yet: the source change is narrowly plausible, while its central no-change adoption path lacks a regression test and an after-fix release-signed runtime trace. Priority: P2 Review scores
Verification
How this fits togetherCodexBar’s Claude usage fetcher delegates expired OAuth refreshes to Claude Code, then attempts to reload a usable credential for the usage retry. This path determines whether a valid Keychain credential can replace an expired cached credential without prompting. flowchart LR
A[Expired cached credential] --> B[Delegated Claude refresh]
B --> C[Keychain observation]
C --> D[Silent credential re-sync]
D --> E[Usage retry]
C --> F[Unreadable-source recovery guidance]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain the narrow non-prompting sync change only with a focused attemptedFailed/no-change regression and redacted release-signed evidence showing that the fresh credential is selected without prompting. Do we have a high-confidence way to reproduce the issue? Yes for a source-level reproduction: the existing recovery harness can seed an expired cache and valid Keychain override, return attemptedFailed with no fingerprint movement, and verify use of the fresh credential. No after-fix release-signed runtime reproduction is attached. Is this the best way to solve the issue? Yes, conditionally: removing the outcome gate is the narrowest way to attempt the already non-interactive sync, but it needs direct regression coverage and signed-runtime proof before merge. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 2436b9f95c8f. LabelsLabel changes:
Label 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
HistoryReview history (5 earlier review cycles)
|
A profile whose keychain CodexBar cannot read produces an indeterminate observation and also satisfies isRefreshResultUnreadable. The indeterminate branch was evaluated first, so it dropped isUnreadableAfterRefresh, replaced the switch-source guidance with a generic retry message, and put the CLI back on the short-cooldown relaunch loop removed in steipete#2650. Order the terminal verdict first and cover it with a regression test.
make check flagged swiftTestingTestCaseNames and redundantSwiftTestingSuite. Use backticked sentence names and drop the argument-less @suite, per AGENTS.md and the surrounding suites.
|
Both rank-up moves addressed where I can; being straight about the one I can't. 1. Restore the terminal unreadable-source handling + regression test — done in You and Codex independently found the same defect, and it was real. It cost two things, not one: Test added: Acceptance criteria: 2. Release-signed runtime trace — I can't produce this, and I'd rather say so than dress up something weaker. A local build is ad-hoc signed, so it cannot read CodexBar's own cache items: With no readable cache there is no expired-cached-credential precondition to recover from — the build lands in a different state ("credentials not found"), so any trace I captured would be evidence about code signing, not about this fix. Producing that proof requires the release signing identity, which only the maintainer has. What I can offer instead: the pre-fix trace is from 0.48.0, Developer ID signed (my machine auto-updated mid-investigation), showing the bug is live on current release rather than only on the 0.47.0 I first reported — Happy to add anything else that's actually obtainable without the signing identity. |
|
@codex review @clawsweeper re-review Both review findings are addressed as of |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
luisgonzaleznf
changed the title
Correcting my own claim — this PR was overstatedAn independent review pass caught something both bots and I missed, and it invalidates the headline claim I opened with. Retitled and rewrote the body; flagging it here rather than editing quietly. What I claimed: an expired Claude OAuth credential can never recover on its own, and this restores background recovery. What's actually true: under the default prompt policy this PR does not restore background recovery, because the code never reaches my change. With if policy.mode == .onlyOnUserAction,
policy.interaction != .userInitiated,
!allowBackgroundDelegatedRefresh // hardcoded false, ClaudeProviderDescriptor.swift:676
{
throw ClaudeUsageError.oauthFailed(
"Claude OAuth token expired, but background repair is suppressed when Keychain prompt policy "
+ "is set to only prompt on user action. Click Refresh in the CodexBar menu to retry.")
}That is thrown at It wouldn't help to simply move the sync earlier, either: What the PR does still fix: the Two smaller corrections in the same spirit: "runs the sync on every outcome" was overstated — in default Auto mode three outcomes still throw at On the real fix: making the default configuration self-heal means allowing a provably-non-prompting read to run under Also flagging a gap I should have closed myself: Take this PR as the narrow fix it is, or close it — both are reasonable. I'd rather hand you an accurate small change plus the real diagnosis than an impressive-sounding wrong one. |
clawsweeper
Bot
added
rating: 🧂 unranked krab
Closing — this change is a no-op in productionClosing my own PR. The fix cannot execute in a release build, and I'd rather say that plainly than leave it open for someone else to work out. The chain:
// Claude Code owns `Claude Code-credentials` and rewrites the item during token refreshes. That rewrite
// replaces its ACL, so any permission granted to CodexBar is inherently temporary and causes recurring
// macOS password dialogs. Production CodexBar therefore never reads the foreign item, with or without UI.
return falseSo I should have found What survives, and is worth keeping: the diagnosis in #2733. Under the default prompt policy an expired OAuth credential has no recovery path, and the message the user sees blames the Keychain prompt policy rather than saying "this profile can't use the OAuth source — switch to Web/CLI". The terminal Also worth a mention independent of any of this: a no-UI |
1 participant
What this fixes
ClaudeUsageFetcher.loadAfterDelegatedRefreshgated the silent keychain re-sync on the delegated-refresh outcome:The touch changes nothing when the Claude CLI's own token is still valid, so the keychain-change probe reports no change and the attempt is recorded
.attemptedFailed. The&&then short-circuits, skipping the step that would adopt the valid credential already present in the keychain. The subsequent retry atClaudeUsageFetcher.swift:464runs withallowKeychainPrompt: falseand so cannot benefit from a credential the sync never loaded.syncFromClaudeKeychainWithoutPromptpassesallowKeychainPrompt: falseon every read (ClaudeOAuthCredentials.swift:1219,:1288,:1316), its candidates probe is no-UI (:2360-2369), and it only adopts a credential that is not expired (:1225,:1296,:1321). Running it when the outcome was.attemptedFailedtherefore cannot prompt and cannot regress a working account.What this does NOT fix
This does not restore background recovery under the default prompt policy, and the original version of this PR wrongly implied it did.
With
claudeOAuthKeychainPromptModeat its default.onlyOnUserAction, a background refresh throws atClaudeUsageFetcher.swift:404—assertDelegatedRefreshAllowedInCurrentInteraction, withallowBackgroundDelegatedRefreshhardcodedfalseatClaudeProviderDescriptor.swift:676— long before reaching the sync at:440.syncFromClaudeKeychainWithoutPromptself-gates on the same policy anyway (shouldAllowClaudeCodeKeychainAccess:.onlyOnUserActionreturnsProviderInteractionContext.current == .userInitiated).So the effect of this PR is:
.always.onlyOnUserAction(default).neverTwo further scope limits, for accuracy: in default Auto mode
oauthKeychainPromptCooldownEnabledis true (ClaudeProviderDescriptor.swift:674), so.skippedByCooldown/.skippedByPromptPolicy/.cliUnavailablestill throw at:419-427before the sync — "runs on every outcome" was overstated. AndsaveClaudeKeychainFingerprintinside the sync now fires on paths where it previously did not.Making the default configuration self-heal is a separate, larger change: it means letting a provably-non-prompting read run under
.onlyOnUserAction. That is a policy decision I don't think a drive-by PR should make — I tried relaxing an adjacent prompt gate earlier in this branch and it caused 8 test regressions, which is a fair warning about that surface.Commits
508c9f3— the fix: drop the&&gate on the silent sync.653816c—waitForClaudeKeychainChangereturnedBool, collapsing "could not read the keychain" into "did not change". Now a tri-state. Honest caveat: with the ordering below, the practical effect is limited to log and message text — call it tidying rather than hardening, and it is separable if you'd rather not carry it.695f478— review fix. Both bots caught that the indeterminate branch precededisRefreshResultUnreadable; an unreadable profile satisfies both at once, which droppedisUnreadableAfterRefresh(losing the "switch source" guidance) and took the 20s cooldown instead of 5m, restoring the CLI relaunch loop Report unreadable Claude OAuth refresh as terminal #2650 removed. Precedence is nowunreadable→touchError→.indeterminate→.unchanged.c9775a2—make checknaming violations.Testing
swift test --filter ClaudeOAuthDelegatedRefresh→ 29/29.make check→ 0 violations across 1806 files.695f478is verified to actually catch its defect: against the pre-fix ordering it fails withisUnreadableAfterRefresh: falseand the wrong message string.mainat22b24b8. Residual failures under a wide filter come from suites that are order-dependent under parallel execution (ClaudeOAuthPromptCoalescingTests,ClaudeCLISessionTests,ClaudeLoginRunnerTests,ClaudeCLITimeoutRetryTests,ClaudeOAuthDelegatedRefreshProfileIsolationTests); the set differs between runs of the same commit, each also fails onmain, and all pass in isolation.Known gap: the
508c9f3change itself has no direct test — restoring the&&would still pass the suite. The existing tests cover the tri-state classifier and the695f478ordering. Worth fixing before merge; flagging it rather than leaving it to be discovered.Verification I could not do
I have not observed a card recover with this patch in the release-signed keychain state. A locally built app is ad-hoc signed and cannot durably read CodexBar's own ACL-bound cache items (
Keychain cache item is unusable by this executable (oauth.claude.profile.<hash>)), so the precondition can't be reproduced from a fork. That check needs the release signing identity.Prior traces in this thread are pre-fix, from a Developer ID signed 0.48.0: a credential ~44h expired with
hasRefreshToken=trueandsource=cacheKeychain, delegating every cycle without recovering. They establish the symptom on current release; they do not by themselves establish the mechanism, which is reasoning from the code paths cited above.