From 7d5bffa23c2fc87081f1bb9dffbfc0d9d523b162 Mon Sep 17 00:00:00 2001 From: Luis Gonzalez Date: Sat, 8 Aug 2026 01:45:29 +0200 Subject: [PATCH] Tell users to switch source when no refresh can restore the profile 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. --- ...sageFetcher+DelegatedRefreshMessages.swift | 20 +++- .../Providers/Claude/ClaudeUsageFetcher.swift | 17 ++- ...laudeUnrecoverableOAuthGuidanceTests.swift | 101 ++++++++++++++++++ Tests/CodexBarTests/ClaudeUsageTests.swift | 43 +++++--- ...laudeOAuthDelegatedRefreshLinuxTests.swift | 58 +++++++--- 5 files changed, 205 insertions(+), 34 deletions(-) create mode 100644 Tests/CodexBarTests/ClaudeUnrecoverableOAuthGuidanceTests.swift diff --git a/Sources/CodexBarCore/Providers/Claude/ClaudeUsageFetcher+DelegatedRefreshMessages.swift b/Sources/CodexBarCore/Providers/Claude/ClaudeUsageFetcher+DelegatedRefreshMessages.swift index 69b0b4178f..c3ab2836a9 100644 --- a/Sources/CodexBarCore/Providers/Claude/ClaudeUsageFetcher+DelegatedRefreshMessages.swift +++ b/Sources/CodexBarCore/Providers/Claude/ClaudeUsageFetcher+DelegatedRefreshMessages.swift @@ -2,6 +2,20 @@ import Foundation /// Split out of `ClaudeUsageFetcher.swift` to keep that file within the file-length limit. extension ClaudeUsageFetcher { + /// Not "run `claude login`, then retry": that refreshes Claude Code's own Keychain item, which this build + /// never reads, so the same expired cache comes back. + static let unreadableCredentialsMessage = + "Claude OAuth credentials expired and CodexBar cannot read them back. Claude Code owns the " + + "Keychain item and no credentials file is present for this profile, so refreshing will not " + + "restore usage. Switch Claude Usage source to Web/CLI." + + /// True when no refresh can restore this profile: CodexBar never reads Claude Code's Keychain item in + /// 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) + } + static func delegatedRefreshOutcomeLabel( _ outcome: ClaudeOAuthDelegatedRefreshCoordinator.Outcome) -> String { @@ -30,11 +44,7 @@ extension ClaudeUsageFetcher { } if result.isUnreadableAfterRefresh { - // Not "run `claude login`, then retry": that refreshes Claude Code's own Keychain item, which this - // build never reads, so the same expired cache comes back. - return "Claude OAuth credentials expired and CodexBar cannot read them back. Claude Code owns the " - + "Keychain item and no credentials file is present for this profile, so refreshing will not " - + "restore usage. Switch Claude Usage source to Web/CLI." + return Self.unreadableCredentialsMessage } switch result.outcome { diff --git a/Sources/CodexBarCore/Providers/Claude/ClaudeUsageFetcher.swift b/Sources/CodexBarCore/Providers/Claude/ClaudeUsageFetcher.swift index 05b8d5a61a..715ae4f63a 100644 --- a/Sources/CodexBarCore/Providers/Claude/ClaudeUsageFetcher.swift +++ b/Sources/CodexBarCore/Providers/Claude/ClaudeUsageFetcher.swift @@ -191,7 +191,7 @@ public struct ClaudeUsageFetcher: ClaudeUsageFetching, Sendable { self.configuration.browserDetection } - private struct ClaudeOAuthKeychainPromptPolicy { + struct ClaudeOAuthKeychainPromptPolicy { let mode: ClaudeOAuthKeychainPromptMode let isApplicable: Bool let interaction: ProviderInteraction @@ -243,9 +243,10 @@ public struct ClaudeUsageFetcher: ClaudeUsageFetching, Sendable { interaction: ProviderInteractionContext.current) } - private static func assertDelegatedRefreshAllowedInCurrentInteraction( + static func assertDelegatedRefreshAllowedInCurrentInteraction( policy: ClaudeOAuthKeychainPromptPolicy, - allowBackgroundDelegatedRefresh: Bool) throws + allowBackgroundDelegatedRefresh: Bool, + isProvablyUnreadable: Bool) throws { if policy.mode == .never { throw ClaudeUsageError.oauthFailed("Delegated refresh is disabled by 'never' keychain policy.") @@ -254,6 +255,12 @@ public struct ClaudeUsageFetcher: ClaudeUsageFetching, Sendable { policy.interaction != .userInitiated, !allowBackgroundDelegatedRefresh { + // Why: "Click Refresh" is a loop for a profile no refresh can restore — the user clicks, the + // delegated path reaches its terminal verdict, and the next background poll overwrites that verdict + // with this message again. Report the terminal outcome the delegated path would reach anyway. + if isProvablyUnreadable { + throw ClaudeUsageError.oauthFailed(unreadableCredentialsMessage) + } 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.") @@ -403,7 +410,9 @@ public struct ClaudeUsageFetcher: ClaudeUsageFetching, Sendable { let delegatedPromptPolicy = ClaudeUsageFetcher.currentClaudeOAuthDelegatedRefreshPolicy() try ClaudeUsageFetcher.assertDelegatedRefreshAllowedInCurrentInteraction( policy: delegatedPromptPolicy, - allowBackgroundDelegatedRefresh: self.fetcher.allowBackgroundDelegatedRefresh) + allowBackgroundDelegatedRefresh: self.fetcher.allowBackgroundDelegatedRefresh, + isProvablyUnreadable: ClaudeUsageFetcher.isDelegatedRefreshProvablyUnreadable( + environment: self.fetcher.environment)) let delegatedResult = await ClaudeUsageFetcher.attemptDelegatedRefresh( environment: self.fetcher.environment) diff --git a/Tests/CodexBarTests/ClaudeUnrecoverableOAuthGuidanceTests.swift b/Tests/CodexBarTests/ClaudeUnrecoverableOAuthGuidanceTests.swift new file mode 100644 index 0000000000..42006529ee --- /dev/null +++ b/Tests/CodexBarTests/ClaudeUnrecoverableOAuthGuidanceTests.swift @@ -0,0 +1,101 @@ +import Foundation +import Testing +@testable import CodexBarCore + +/// Regression coverage for #2733: a profile no refresh can restore must not be told to click Refresh. +@Suite(.serialized) +struct ClaudeUnrecoverableOAuthGuidanceTests { + private func makeTemporaryDirectory() throws -> URL { + let root = URL(fileURLWithPath: NSTemporaryDirectory()) + .appendingPathComponent("codexbar-guidance-\(UUID().uuidString)") + try FileManager.default.createDirectory(at: root, withIntermediateDirectories: true) + return root + } + + private func makeCredentialsData(expiresAt: Date) -> Data { + let millis = Int(expiresAt.timeIntervalSince1970 * 1000) + return Data(""" + { + "claudeAiOauth": { + "accessToken": "from-file", + "expiresAt": \(millis), + "scopes": ["user:profile"] + } + } + """.utf8) + } + + private func backgroundPolicy() -> ClaudeUsageFetcher.ClaudeOAuthKeychainPromptPolicy { + ClaudeUsageFetcher.ClaudeOAuthKeychainPromptPolicy( + mode: .onlyOnUserAction, + isApplicable: true, + interaction: .background) + } + + private func message(from error: Error) -> String? { + guard case let ClaudeUsageError.oauthFailed(message) = error else { return nil } + return message + } + + @Test + func `a profile no refresh can restore is told to switch source, not to click Refresh`() async throws { + let root = try self.makeTemporaryDirectory() + defer { try? FileManager.default.removeItem(at: root) } + + // Keychain reads disabled (production always is) and no credentials file: nothing a delegated + // refresh produces can ever be read back, so "Click Refresh" would loop forever. + try await ClaudeOAuthCredentialsStore.withKeychainAccessOverrideForTesting(true) { + try await ClaudeOAuthCredentialsStore.withCredentialsURLOverrideForTesting( + root.appendingPathComponent(".credentials.json")) + { + #expect(ClaudeUsageFetcher.isDelegatedRefreshProvablyUnreadable(environment: [:])) + + let thrown = #expect(throws: ClaudeUsageError.self) { + try ClaudeUsageFetcher.assertDelegatedRefreshAllowedInCurrentInteraction( + policy: self.backgroundPolicy(), + allowBackgroundDelegatedRefresh: false, + isProvablyUnreadable: true) + } + #expect(self.message(from: thrown!) == ClaudeUsageFetcher.unreadableCredentialsMessage) + #expect(self.message(from: thrown!)?.contains("Click Refresh") != true) + } + } + } + + @Test + func `a recoverable profile keeps the click Refresh guidance`() async throws { + let root = try self.makeTemporaryDirectory() + defer { try? FileManager.default.removeItem(at: root) } + let credentialsURL = root.appendingPathComponent(".credentials.json") + try self.makeCredentialsData(expiresAt: Date(timeIntervalSinceNow: 3600)).write(to: credentialsURL) + + try await ClaudeOAuthCredentialsStore.withKeychainAccessOverrideForTesting(true) { + try await ClaudeOAuthCredentialsStore.withCredentialsURLOverrideForTesting(credentialsURL) { + // A credentials file is present, so a delegated refresh can still hand something back. + #expect(!ClaudeUsageFetcher.isDelegatedRefreshProvablyUnreadable(environment: [:])) + + let thrown = #expect(throws: ClaudeUsageError.self) { + try ClaudeUsageFetcher.assertDelegatedRefreshAllowedInCurrentInteraction( + policy: self.backgroundPolicy(), + allowBackgroundDelegatedRefresh: false, + isProvablyUnreadable: false) + } + #expect(self.message(from: thrown!)?.contains("Click Refresh") == true) + } + } + } + + @Test + func `a user initiated refresh is never blocked by the prompt policy`() throws { + let policy = ClaudeUsageFetcher.ClaudeOAuthKeychainPromptPolicy( + mode: .onlyOnUserAction, + isApplicable: true, + interaction: .userInitiated) + // Delegation must still run for user actions: on older Claude Code the touch itself can create the + // credentials file, which is exactly the case the terminal verdict must not pre-empt. + try ClaudeUsageFetcher.assertDelegatedRefreshAllowedInCurrentInteraction( + policy: policy, + allowBackgroundDelegatedRefresh: false, + isProvablyUnreadable: true) + } +} diff --git a/Tests/CodexBarTests/ClaudeUsageTests.swift b/Tests/CodexBarTests/ClaudeUsageTests.swift index 21cb1c7a41..dabc8c1529 100644 --- a/Tests/CodexBarTests/ClaudeUsageTests.swift +++ b/Tests/CodexBarTests/ClaudeUsageTests.swift @@ -269,6 +269,19 @@ struct ClaudeUsageTests { let loadCounter = AsyncCounter() let delegatedCounter = AsyncCounter() + // Pinned: a present credentials file means a refresh could still restore this profile, which is what + // makes the retry suggestion below genuine. Without pinning, the message would depend on whether the + // host running the tests happens to have one. + let root = URL(fileURLWithPath: NSTemporaryDirectory()) + .appendingPathComponent("codexbar-delegated-\(UUID().uuidString)") + try FileManager.default.createDirectory(at: root, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: root) } + let credentialsURL = root.appendingPathComponent(".credentials.json") + let expiresAt = Int(Date(timeIntervalSinceNow: 3600).timeIntervalSince1970 * 1000) + try Data(""" + {"claudeAiOauth":{"accessToken":"t","expiresAt":\(expiresAt),"scopes":["user:profile"]}} + """.utf8).write(to: credentialsURL) + let fetcher = ClaudeUsageFetcher( browserDetection: BrowserDetection(cacheTTL: 0), environment: [:], @@ -291,23 +304,27 @@ struct ClaudeUsageTests { } do { - _ = try await ClaudeOAuthKeychainReadStrategyPreference.withTaskOverrideForTesting( - .securityFramework, - operation: { - try await ClaudeOAuthKeychainPromptPreference.withTaskOverrideForTesting(.onlyOnUserAction) { - try await ProviderInteractionContext.$current.withValue(.background) { - try await ClaudeUsageFetcher.$delegatedRefreshAttemptOverride.withValue( - delegatedOverride) - { - try await ClaudeUsageFetcher.$loadOAuthCredentialsOverride.withValue( - loadCredsOverride) + _ = try await ClaudeOAuthCredentialsStore.withCredentialsURLOverrideForTesting(credentialsURL) { + try await ClaudeOAuthKeychainReadStrategyPreference.withTaskOverrideForTesting( + .securityFramework, + operation: { + try await ClaudeOAuthKeychainPromptPreference + .withTaskOverrideForTesting(.onlyOnUserAction) + { + try await ProviderInteractionContext.$current.withValue(.background) { + try await ClaudeUsageFetcher.$delegatedRefreshAttemptOverride.withValue( + delegatedOverride) { - try await fetcher.loadLatestUsage(model: "sonnet") + try await ClaudeUsageFetcher.$loadOAuthCredentialsOverride.withValue( + loadCredsOverride) + { + try await fetcher.loadLatestUsage(model: "sonnet") + } } } } - } - }) + }) + } Issue.record("Expected delegated refresh to be suppressed in background") } catch let error as ClaudeUsageError { guard case let .oauthFailed(message) = error else { diff --git a/TestsLinux/ClaudeOAuthDelegatedRefreshLinuxTests.swift b/TestsLinux/ClaudeOAuthDelegatedRefreshLinuxTests.swift index 21ca1b3707..909a151eea 100644 --- a/TestsLinux/ClaudeOAuthDelegatedRefreshLinuxTests.swift +++ b/TestsLinux/ClaudeOAuthDelegatedRefreshLinuxTests.swift @@ -57,11 +57,24 @@ struct ClaudeOAuthDelegatedRefreshLinuxTests { } @Test - func appOAuthBackgroundRespectsPlatformKeychainPromptPolicy() async { + func appOAuthBackgroundRespectsPlatformKeychainPromptPolicy() async throws { + // A credentials file means a delegated refresh could still hand something back, so the retry + // suggestion is genuine and must be preserved. + let root = URL(fileURLWithPath: NSTemporaryDirectory()) + .appendingPathComponent("codexbar-linux-\(UUID().uuidString)") + try FileManager.default.createDirectory(at: root, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: root) } + let credentialsURL = root.appendingPathComponent(".credentials.json") + let expiresAt = Int(Date(timeIntervalSinceNow: 3600).timeIntervalSince1970 * 1000) + try Data(""" + {"claudeAiOauth":{"accessToken":"t","expiresAt":\(expiresAt),"scopes":["user:profile"]}} + """.utf8).write(to: credentialsURL) + let result = await self.runDelegatedRefresh( runtime: .app, interaction: .background, - promptMode: .onlyOnUserAction) + promptMode: .onlyOnUserAction, + credentialsURL: credentialsURL) #expect(result.attempts == 0) #expect(result.message.contains("background repair is suppressed")) @@ -69,10 +82,29 @@ struct ClaudeOAuthDelegatedRefreshLinuxTests { #expect(!result.message.contains("Open the CodexBar menu or")) } + @Test + func appOAuthBackgroundReportsUnrecoverableProfileInsteadOfSuggestingRefresh() async { + // No credentials file and no readable Claude Keychain item: a refresh cannot restore this profile, + // so "Click Refresh" would send the user round a loop that always lands back here. + let result = await self.runDelegatedRefresh( + runtime: .app, + interaction: .background, + promptMode: .onlyOnUserAction) + + #expect(result.attempts == 0) + #expect(result.message == ClaudeUsageFetcher.unreadableCredentialsMessage) + #expect(!result.message.contains("Click Refresh in the CodexBar menu")) + } + private func runDelegatedRefresh( runtime: ProviderRuntime, interaction: ProviderInteraction, - promptMode: ClaudeOAuthKeychainPromptMode) async -> (attempts: Int, message: String) + promptMode: ClaudeOAuthKeychainPromptMode, + // Pinned so the suppression message does not depend on whether the host running the tests happens to + // have a Claude credentials file: its presence decides whether a refresh could restore this profile. + credentialsURL: URL = URL(fileURLWithPath: NSTemporaryDirectory()) + .appendingPathComponent("codexbar-absent-\(UUID().uuidString)") + .appendingPathComponent(".credentials.json")) async -> (attempts: Int, message: String) { let counter = Counter() let fetcher = ClaudeUsageFetcher( @@ -95,15 +127,17 @@ struct ClaudeOAuthDelegatedRefreshLinuxTests { } do { - _ = try await ClaudeOAuthKeychainPromptPreference.withTaskOverrideForTesting(promptMode) { - try await ProviderInteractionContext.$current.withValue(interaction) { - try await ClaudeUsageFetcher.$loadOAuthCredentialsOverride - .withValue(credentialsOverride) { - try await ClaudeUsageFetcher.$delegatedRefreshAttemptOverride - .withValue(delegatedOverride) { - try await fetcher.loadLatestUsage(model: "sonnet") - } - } + _ = try await ClaudeOAuthCredentialsStore.withCredentialsURLOverrideForTesting(credentialsURL) { + try await ClaudeOAuthKeychainPromptPreference.withTaskOverrideForTesting(promptMode) { + try await ProviderInteractionContext.$current.withValue(interaction) { + try await ClaudeUsageFetcher.$loadOAuthCredentialsOverride + .withValue(credentialsOverride) { + try await ClaudeUsageFetcher.$delegatedRefreshAttemptOverride + .withValue(delegatedOverride) { + try await fetcher.loadLatestUsage(model: "sonnet") + } + } + } } } Issue.record("Expected delegated-refresh path to fail with mocked stale credentials")