Cover credential refresh coalescing - #148
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
@macroscope-app review |
|
@codex review |
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesCredential refresh concurrency
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CopilotProviderTests
participant CopilotUsageProvider
participant CredentialRefreshCoordinator
participant IsolatedTestURLSession
CopilotProviderTests->>CopilotUsageProvider: start two concurrent fetches
CopilotUsageProvider->>CredentialRefreshCoordinator: refresh credentials for account
CredentialRefreshCoordinator->>IsolatedTestURLSession: perform one token refresh request
CopilotUsageProvider->>CredentialRefreshCoordinator: join existing account refresh
CredentialRefreshCoordinator-->>CopilotUsageProvider: return shared refreshed credential
CopilotUsageProvider->>IsolatedTestURLSession: send usage requests with refreshed token
IsolatedTestURLSession-->>CopilotProviderTests: return both usage results
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Manual reviews triggered for commit All prior checks · these links stay valid even if you push more commits. |
|
Review in progress. Results will be posted when the checks complete. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CodexBarMac/Services/CopilotUsageProvider.swift (1)
34-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelegate the public initializer to the internal one to remove the duplicated body.
The two initializers assign the same nine stored properties and repeat the
gitHubTokenResolverfallback that callsLocalCredentialDiscovery.gitHubAuthToken(for:). A future dependency or a change to the resolver fallback must be applied twice. Convergence failures in this type affect credential resolution.Keep the public signature and defaults, and forward to the internal initializer.
♻️ Proposed refactor
public init( secretStore: any SecretStore = KeychainService(), session: URLSession = .shared, usageEndpoint: URL = URL(string: "https://api.github.com/copilot_internal/user")!, githubAPIBaseURL: URL = URL(string: "https://api.github.com")!, tokenEndpoint: URL = CopilotWebAuthService.tokenEndpoint, oauthConfiguration: CopilotOAuthConfiguration = .bundled, gitHubTokenResolver: (`@Sendable` (String?) throws -> String?)? = nil, now: `@escaping` `@Sendable` () -> Date = { Date() } ) { - self.secretStore = secretStore - self.session = session - self.usageEndpoint = usageEndpoint - self.githubAPIBaseURL = githubAPIBaseURL - self.tokenEndpoint = tokenEndpoint - self.oauthConfiguration = oauthConfiguration - self.gitHubTokenResolver = gitHubTokenResolver ?? { username in - try LocalCredentialDiscovery.gitHubAuthToken(for: username) - } - self.now = now - self.onJoinInFlightRefresh = nil + self.init( + secretStore: secretStore, + session: session, + usageEndpoint: usageEndpoint, + githubAPIBaseURL: githubAPIBaseURL, + tokenEndpoint: tokenEndpoint, + oauthConfiguration: oauthConfiguration, + gitHubTokenResolver: gitHubTokenResolver, + now: now, + onJoinInFlightRefresh: nil + ) }Note: a delegating initializer in a class must be marked
convenience, and the internal initializer stays designated. Verify the build after the change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CodexBarMac/Services/CopilotUsageProvider.swift` around lines 34 - 79, Mark the public init in CopilotUsageProvider as convenience and delegate to the existing internal initializer, forwarding all parameters including the public defaults and passing through onJoinInFlightRefresh as nil. Remove its duplicated property assignments and gitHubTokenResolver fallback, leaving the internal initializer as the designated initializer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CodexBarMacTests/CredentialRefreshCoordinatorTests.swift`:
- Around line 103-123: Update TestAsyncGate in RefreshTestSupport.swift so
waitUntilBlocked() cannot complete until wait() has stored its continuation,
ensuring a concurrent release() always resumes the suspended operation. Preserve
the existing gate behavior and API while reordering the blocked
signal/continuation setup.
In `@CodexBarMacTests/RefreshTestSupport.swift`:
- Line 55: Add explicit empty deinitializers to all new classes required by
SwiftLint: TestWatchdogStartLatch, TestWatchdogTaskCoordinator, and
TestWatchdogOutcomeCoordinator in RefreshTestSupport.swift;
CredentialRefreshCoordinatorTests and LockedTestFlag in
CredentialRefreshCoordinatorTests.swift. Apply the same deinit convention
already used by the test suite at the cited anchor and sibling sites.
---
Outside diff comments:
In `@CodexBarMac/Services/CopilotUsageProvider.swift`:
- Around line 34-79: Mark the public init in CopilotUsageProvider as convenience
and delegate to the existing internal initializer, forwarding all parameters
including the public defaults and passing through onJoinInFlightRefresh as nil.
Remove its duplicated property assignments and gitHubTokenResolver fallback,
leaving the internal initializer as the designated initializer.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f3fa97fc-0628-42b8-8e3d-54bb4a091b0b
📒 Files selected for processing (8)
CHANGELOG.mdCodexBarMac.xcodeproj/project.pbxprojCodexBarMac/Services/CopilotUsageProvider.swiftCodexBarMac/Services/CredentialRefreshCoordinator.swiftCodexBarMacTests/CopilotProviderTests.swiftCodexBarMacTests/CredentialRefreshCoordinatorTests.swiftCodexBarMacTests/NetworkTestSupport.swiftCodexBarMacTests/RefreshTestSupport.swift
ApprovabilityVerdict: Would Approve This PR adds test coverage for credential refresh coalescing with minimal production code changes. The production modifications are limited to adding optional callback parameters (defaulting to nil) for test observability, maintaining full backward compatibility. Macroscope would have approved this PR. Enable approvability here. |
|
Addressed CodeRabbit’s outside-diff initializer finding in ca1849d: the public |
|
@coderabbitai review |
|
@macroscope-app review |
|
@codex review |
|
|
Review in progress. Results will be posted when the checks complete. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Closes #147
Summary
CredentialRefreshCoordinatorVerification
./test.sh(351 tests; cut-changelog, release-artifact, and run smoke scripts passed)xcodebuild ... -enableThreadSanitizer YES test(351 tests; no sanitizer reports)xcodebuild ... analyze-enableCodeCoverage YESplusxccov(existing-task return executed twice)Risk
Summary by cubic
Add deterministic concurrency coverage for account-scoped credential-refresh coalescing. Confirms concurrent Copilot browser refreshes join a single in-flight task per account and stay isolated across accounts.
onJoinExistingTasktoCredentialRefreshCoordinator.run()and wiredonJoinInFlightRefreshthroughCopilotUsageProvideras an internal-only test hook.IsolatedTestURLSession(customURLProtocol) to sandbox concurrent network tests.TestSignal,TestAsyncGate, and a bounded watchdog with a start latch and task coordination; verifies it doesn’t await cancellation-insensitive work.Written for commit ca1849d. Summary will update on new commits.
Note
Add tests covering credential refresh coalescing in
CredentialRefreshCoordinatorandCopilotUsageProviderCredentialRefreshCoordinatorTestswith tests for same-account coalescing, independent multi-account execution, and watchdog timeout behavior.testConcurrentCopilotFetchesCoalesceBrowserCredentialRefreshtoCopilotProviderTeststo verify concurrent fetches share a single refresh and propagate the new token correctly.onJoinInFlightRefreshcallback toCopilotUsageProviderandonJoinExistingTasktoCredentialRefreshCoordinator.runto make join events observable in tests without changing default behavior.NetworkTestSupport.swift(isolated per-testURLSession/URLProtocol) andRefreshTestSupport.swift(withTestWatchdog,TestAsyncGate,TestSignal) to enable deterministic async concurrency testing.Macroscope summarized 37fa304.
Greptile Summary
The PR adds deterministic test coverage for account-scoped credential-refresh coalescing without changing the default production behavior.
CredentialRefreshCoordinator.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant F1 as First fetch participant F2 as Second fetch participant C as Refresh coordinator participant R as Credential refresh F1->>C: run(account) C->>R: start refresh task F2->>C: run(same account) C-->>F2: join existing task R-->>C: refreshed credentials C-->>F1: shared result C-->>F2: shared resultReviews (2): Last reviewed commit: "Harden concurrency test synchronization" | Re-trigger Greptile