Claude: report the terminal verdict when no refresh can restore the profile - #2747
Claude: report the terminal verdict when no refresh can restore the profile#2747luisgonzaleznf wants to merge 1 commit into
Conversation
A profile with no Claude credentials file, whose Keychain item production CodexBar never reads, cannot be restored by any refresh. Background cycles still told the user to click Refresh: they click, the delegated path reaches its terminal "cannot read them back / switch source" verdict, and the next background poll overwrites that with the retry suggestion again. Report the terminal verdict directly when the profile is provably unreadable. Only the message changes: delegation is still skipped in background exactly as before, and user-initiated refreshes still run it, so the older-Claude-Code case where the touch itself creates the credentials file is untouched. The two existing tests that assert the retry wording now pin their credentials file explicitly. Without that the message they observe would depend on whether the host running the tests happens to have one.
![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: 7d5bffa23c
ℹ️ 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".
| /// production, so with no credentials file there is nothing a delegated refresh could hand back. | ||
| static func isDelegatedRefreshProvablyUnreadable(environment: [String: String]) -> Bool { | ||
| guard !ClaudeOAuthCredentialsStore.keychainAccessAllowed else { return false } | ||
| return !ClaudeOAuthCredentialsStore.hasSelectedProfileOAuthCredentialsFile(environment: environment) |
There was a problem hiding this comment.
When a background poll hits onlyOnUserAction before the user has tried a manual delegated refresh, the absence of a credentials file is not enough to prove the profile is unrecoverable: the delegated-refresh coordinator intentionally treats older Claude Code as potentially able to create that file during a retried touch (ClaudeOAuthDelegatedRefreshCoordinator.swift:307-311). Returning true here makes those recoverable cached CLI credentials display the terminal “Switch source” message instead of the only action that can create the file, so this should depend on an actual post-touch isUnreadableAfterRefresh verdict or equivalent persisted evidence.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed August 8, 2026, 1:03 AM ET / 05:03 UTC. ClawSweeper reviewWhat this changesThe PR changes background Claude OAuth refresh guidance so profiles without a readable credentials file receive a switch-source message instead of a repeated Refresh suggestion. Merge readiness⛔ Blocked until real behavior proof is added - 6 items remain Keep this PR open, but it is not ready to merge: its pre-touch file-absence heuristic can replace the only recovery action for older Claude Code profiles that create credentials during a manual refresh. Priority: P2 Review scores
Verification
How this fits togetherCodexBar polls Claude usage and surfaces OAuth recovery guidance in the menu bar. The refresh path first applies prompt policy, then may ask Claude Code to refresh credentials before returning usage or a user-facing failure message. flowchart LR
A[Claude usage poll] --> B[Prompt policy]
B --> C{Background delegation allowed?}
C -->|yes| D[Claude Code refresh touch]
C -->|no| E[Background guidance]
D --> F[Observed credential result]
F --> G[Menu bar status]
E --> G
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Preserve Refresh guidance until a per-profile, completed delegated touch has recorded the coordinator’s actual unreadable verdict; only then retain terminal switch-source guidance across later background polls. Do we have a high-confidence way to reproduce the issue? Yes from source: the PR’s background branch classifies a missing file before a delegated touch, while the coordinator documents that the touch can create that file on older Claude Code. Is this the best way to solve the issue? No. Terminal guidance should depend on the coordinator’s completed post-touch unreadable result, not on a pre-touch absence check. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against f33095395e19. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (2 earlier review cycles)
|
|
Thanks for digging into this — the stuck-card loop you're describing is real, but this implementation can't land in its current shape, and the reasoning is worth spelling out. The patch computes "provably unreadable" before the delegated touch (Keychain unavailable + credentials file absent) and reports terminal guidance from that. That widens terminal classification in exactly the way #2650's review deliberately rejected: on older Claude Code versions, the touch itself can create The underlying experience you're fixing is the target of #2675 (consented direct read + CLI usage fallback), which resolves the stuck card without weakening the classifier. Once that lands, if persistent terminal guidance across background polls is still desirable, the right construction is to persist the actual post-touch terminal verdict and surface that — happy to review a follow-up in that shape. Closing this one in the meantime rather than leaving it in limbo; the analysis in your PR body genuinely helped validate #2675's direction. |
|
Understood, and the Codex finding was right — thanks for the review time. My mistake was reasoning about the code path and not the user path. Background still skipped delegation exactly as before, so I convinced myself the older-Claude-Code touch was preserved. But dropping "Click Refresh" from the message removes the prompt to perform that touch, so a recoverable profile could be told to abandon the one action that would have fixed it. Worse than the bug I was aiming at. A corrected version would gate on an actual post-touch Worth me putting that together, or would you rather leave this alone? Happy either way — no reply needed if it's a no, I won't follow up. The observed-behaviour writeup in #2733 stands on its own regardless. |
2 participants
Fixes the user-facing half of #2733: a Claude profile that no refresh can restore is told to click Refresh, which sends the user round a loop that always lands back in the same place.
The loop
CodexBar already has the right message for this state —
delegatedRefreshFailureMessagereturns "Claude OAuth credentials expired and CodexBar cannot read them back… Switch Claude Usage source to Web/CLI." But under the default prompt policy the user rarely sees it, and never keeps it:ClaudeUsageFetcher.swift:404(assertDelegatedRefreshAllowedInCurrentInteraction, withallowBackgroundDelegatedRefreshhardcodedfalseatClaudeProviderDescriptor.swift:676) with "background repair is suppressed… Click Refresh in the CodexBar menu to retry.".userInitiated, so delegation runs and reaches the accurate terminal verdict.So the actionable guidance is transient and the misleading guidance is the steady state. Clicking Refresh cannot fix this profile — there is nothing for a refresh to hand back.
Why the profile is unrecoverable
keychainAccessAllowedis hardcodedfalseoutside#if DEBUG(ClaudeOAuthCredentials.swift:2855-2871) — production never reads Claude Code's Keychain item, deliberately, because Claude Code rewrites that item on every refresh and resets its ACL. With no~/.claude/.credentials.jsoneither, a delegated refresh has no channel to return a credential through.That is precisely what
isRefreshResultUnreadablealready detects — but only after the touch, which background cycles never reach.The change
When the prompt-policy branch is about to suppress background delegation, and the profile is provably unreadable, throw the terminal message instead of the retry suggestion.
Deliberately message-only:
.userInitiatedstill runs it. That matters because the coordinator notes "on older Claude Code a retried touch can still create the credentials file" — so the terminal verdict must not pre-empt the touch. It doesn't; this only picks the message on the path where delegation was already being skipped.unreadableCredentialsMessageis extracted fromdelegatedRefreshFailureMessageand shared, so both paths stay in sync by construction.Testing
ClaudeUnrecoverableOAuthGuidanceTests— 3 cases: unrecoverable profile gets the terminal message, recoverable profile keeps the retry message, user-initiated is never suppressed."…Click Refresh in the CodexBar menu to retry."where the terminal message is expected.appOAuthBackgroundReportsUnrecoverableProfileInsteadOfSuggestingRefreshcovering the same via the full fetcher path.make check→ 0 violations across 1806 files.mainat22b24b8. Remaining failures under a wide filter are pre-existing order-dependent suites (ClaudeOAuthPromptCoalescingTests,ClaudeCLISessionTests) — they fail onmaintoo, the set shifts between runs of the same commit, and both pass 12/12 twice in isolation.Two existing tests changed, and I want to flag why rather than bury it.
appOAuthBackgroundRespectsPlatformKeychainPromptPolicyandoauth delegated retry only on user action background suppresses delegationboth assert the retry wording while usingenvironment: [:]with no credentials-file override. My change makes that message depend on whether a credentials file exists, so as written they would have become host-dependent. Both now pin a credentials file explicitly and keep their original assertions unchanged — the retry wording is still correct for a profile that is recoverable. Their behavioral assertions (attempts == 0,delegatedCounter == 0) are untouched.Context
Reported from a real stuck profile: credential ~44h expired,
hasRefreshToken=true,owner=claudeCLI,source=cacheKeychain, delegating every cycle and never recovering, on Developer ID signed 0.48.0 whileclaudeitself worked throughout. The user spent a day on it because the message pointed at a Keychain setting; the fix was to stop using the OAuth source, which the app already knew and only said transiently.This supersedes #2739, which I closed as a no-op — that one un-gated a sync that
keychainAccessAlloweddisables in production anyway. This change is on a path I've confirmed executes in production, which is what that one wasn't.