fix(gh cli): migrate legacy gh cli auth token into keychain#1724
Merged
arnestrickmann merged 4 commits intomainfrom Apr 14, 2026
Merged
fix(gh cli): migrate legacy gh cli auth token into keychain#1724arnestrickmann merged 4 commits intomainfrom
arnestrickmann merged 4 commits intomainfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughAdds one-shot keychain migration from the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant GS as GitHubService
participant Keytar as Keytar
participant GHCLI as GH CLI
Caller->>GS: getStoredToken()
GS->>Keytar: getPassword('emdash-github','github-token')
Keytar-->>GS: token or null
alt token exists
GS-->>Caller: return token
else no token
GS->>Keytar: getPassword('emdash-github','github-migration-blocked')
Keytar-->>GS: blocked? ('1' or null)
alt blocked
GS-->>Caller: return null
else not blocked
GS->>GS: withAuthStateLock -> migrateTokenFromGHCLI()
GS->>GHCLI: run `gh auth token`
GHCLI-->>GS: stdout token or empty
alt token received
GS->>Keytar: setPassword('emdash-github','github-token', token) (source='migration')
Keytar-->>GS: stored
GS-->>Caller: return migrated token
else no token
GS-->>Caller: return null
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/services/GitHubService.ts`:
- Around line 108-110: The current legacy-token migration guarded by the boolean
migrationAttempted allows racing: multiple callers of getStoredToken() can see
null while one migration is in-flight and logout() can race with migration; fix
by introducing a shared in-flight Promise or mutex (e.g., migrationInFlight:
Promise<string|null> | null) to serialize the migration logic in
getStoredToken()/migrateLegacyToken(), have callers await that promise instead
of returning null, and ensure the actual write/persist of the migrated token is
performed only while holding the serialized section (the mutex/promise) and
after re-checking logout/blocked state so logout() cannot race to block
persistence—update references in getStoredToken(), migrateLegacyToken(), and
logout() to cooperate with the new migrationInFlight synchronization.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd27c5c9-f98b-459d-9ce5-01b7322472f0
📒 Files selected for processing (3)
src/main/ipc/githubIpc.tssrc/main/services/GitHubService.tssrc/test/main/GitHubService.test.ts
Collaborator
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/main/GitHubService.test.ts`:
- Around line 203-210: Before releasing the blocked token store, assert that the
logout process has not proceeded past the lock by checking the mock and
sentinel: after calling service.logout() but before calling releaseTokenStore(),
add assertions that deletePasswordMock (or whatever mock wraps key deletion) has
not been called and that keychain.get('emdash-github:github-migration-blocked')
is still null/undefined; then releaseTokenStore() and continue the existing
awaits and final assertions. This ensures the test verifies the serialization in
GitHubService.logout() rather than a late-running delete.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a2215b8-9e45-4ea0-b80d-7c376e81e291
📒 Files selected for processing (2)
src/main/services/GitHubService.tssrc/test/main/GitHubService.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/services/GitHubService.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/main/GitHubService.test.ts (1)
10-12: De-duplicate the keytar mock behavior.The blocking/store logic now lives in both the module-scope mock setup and the
beforeEachreset path. Extracting one installer/helper would keep the race harness from drifting over time.♻️ Suggested cleanup
const keychain = new Map<string, string>(); const keyFor = (serviceName: string, accountName: string) => `${serviceName}:${accountName}`; -const setPasswordMock = vi.fn( - async (serviceName: string, accountName: string, password: string) => { - if (accountName === 'github-token') { - notifyTokenStoreStarted?.(); - if (blockTokenStore) { - await new Promise<void>((resolve) => { - releaseTokenStore = resolve; - }); - } - } - - keychain.set(keyFor(serviceName, accountName), password); - } -); -const getPasswordMock = vi.fn(async (serviceName: string, accountName: string) => { - return keychain.get(keyFor(serviceName, accountName)) ?? null; -}); -const deletePasswordMock = vi.fn(async (serviceName: string, accountName: string) => { - keychain.delete(keyFor(serviceName, accountName)); -}); +const setPasswordMock = vi.fn(); +const getPasswordMock = vi.fn(); +const deletePasswordMock = vi.fn(); + +const installKeytarMockBehavior = () => { + setPasswordMock.mockImplementation( + async (serviceName: string, accountName: string, password: string) => { + if (accountName === 'github-token') { + notifyTokenStoreStarted?.(); + if (blockTokenStore) { + await new Promise<void>((resolve) => { + releaseTokenStore = resolve; + }); + } + } + + keychain.set(keyFor(serviceName, accountName), password); + } + ); + getPasswordMock.mockImplementation(async (serviceName: string, accountName: string) => { + return keychain.get(keyFor(serviceName, accountName)) ?? null; + }); + deletePasswordMock.mockImplementation(async (serviceName: string, accountName: string) => { + keychain.delete(keyFor(serviceName, accountName)); + }); +}; + +installKeytarMockBehavior();- setPasswordMock.mockImplementation( - async (serviceName: string, accountName: string, password: string) => { - if (accountName === 'github-token') { - notifyTokenStoreStarted?.(); - if (blockTokenStore) { - await new Promise<void>((resolve) => { - releaseTokenStore = resolve; - }); - } - } - - keychain.set(keyFor(serviceName, accountName), password); - } - ); - getPasswordMock.mockImplementation(async (serviceName: string, accountName: string) => { - return keychain.get(keyFor(serviceName, accountName)) ?? null; - }); - deletePasswordMock.mockImplementation(async (serviceName: string, accountName: string) => { - keychain.delete(keyFor(serviceName, accountName)); - }); + installKeytarMockBehavior();Also applies to: 76-99, 125-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/main/GitHubService.test.ts` around lines 10 - 12, The keytar mock blocking/store logic is duplicated between the module-scope setup and the beforeEach reset (symbols blockTokenStore, releaseTokenStore, notifyTokenStoreStarted); extract that logic into a single helper (e.g., installKeytarMock or resetKeytarMock) and replace both the module-level initialization and the beforeEach reset with calls to that helper so the race-harness behavior is defined in one place and reused consistently across the tests (update all places that reference blockTokenStore/releaseTokenStore/notifyTokenStoreStarted to use the helper’s API).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/main/GitHubService.test.ts`:
- Around line 10-12: The keytar mock blocking/store logic is duplicated between
the module-scope setup and the beforeEach reset (symbols blockTokenStore,
releaseTokenStore, notifyTokenStoreStarted); extract that logic into a single
helper (e.g., installKeytarMock or resetKeytarMock) and replace both the
module-level initialization and the beforeEach reset with calls to that helper
so the race-harness behavior is defined in one place and reused consistently
across the tests (update all places that reference
blockTokenStore/releaseTokenStore/notifyTokenStoreStarted to use the helper’s
API).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f5e1baa-89cb-4013-aeb3-267f1788c31a
📒 Files selected for processing (1)
src/test/main/GitHubService.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/services/GitHubService.ts (1)
1354-1363: Consider: Narrow remaining race window between migrationAttempted check and promise assignment.There's a small race between lines 1348 and 1354-1355: two callers passing the
migrationAttemptedcheck can each create a promise. The lock inmigrateTokenFromGHCLIensures only one actually migrates, but the "loser" promise returnsnull. A third caller arriving in this window might await the loser promise and receivenulldespite successful migration.In practice this is very minor—the token IS stored, so the next call succeeds—but if you want to eliminate it entirely:
♻️ Optional: Atomic check-and-assign
- if (this.migrationAttempted) return null; - } catch (error) { - console.error('Failed to retrieve token:', error); - return null; - } - - const inFlight = this.migrateTokenFromGHCLI(); - this.migrationInFlight = inFlight; + if (this.migrationAttempted) return null; + + // Atomically create and store the migration promise + const inFlight = this.migrateTokenFromGHCLI(); + this.migrationInFlight = inFlight; + } catch (error) { + console.error('Failed to retrieve token:', error); + return null; + }Moving the promise creation inside the try block doesn't fully fix the race—a proper fix would require a synchronous guard variable set immediately, then the async work. Given the minimal impact, the current implementation is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/GitHubService.ts` around lines 1354 - 1363, There’s a tiny race where two callers pass migrationAttempted and each creates a promise so a third caller could await the “loser” promise and get null; fix by making the check-and-assign atomic: ensure you synchronously set a guard/promise before starting async work (e.g., assign this.migrationInFlight immediately to a pending promise or set a boolean migrationStarted synchronously) and then run migrateTokenFromGHCLI inside that promise so subsequent callers see the same in-flight promise; keep the existing finally that clears this.migrationInFlight only if it still equals the promise you set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/services/GitHubService.ts`:
- Around line 1354-1363: There’s a tiny race where two callers pass
migrationAttempted and each creates a promise so a third caller could await the
“loser” promise and get null; fix by making the check-and-assign atomic: ensure
you synchronously set a guard/promise before starting async work (e.g., assign
this.migrationInFlight immediately to a pending promise or set a boolean
migrationStarted synchronously) and then run migrateTokenFromGHCLI inside that
promise so subsequent callers see the same in-flight promise; keep the existing
finally that clears this.migrationInFlight only if it still equals the promise
you set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41f36331-f81a-44a3-a066-9ab4fd7d3971
📒 Files selected for processing (1)
src/main/services/GitHubService.ts
Contributor
|
Thanks! @janburzinski |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
after i added the gh cli removal stuff so that people dont get ratelimited we now had the issue that prs in the sidebar didnt work for users
this should fix that since we now migrate the gh auth token into the keychain if the keychain is empty and that should make it work again
Type of change
Mandatory Tasks
Checklist
Summary by CodeRabbit
New Features
Improvements
Tests