feat: add Copilot CLI Keychain token discovery and multi-account CLI support#84
Open
Shaglock wants to merge 3 commits intoopgginc:mainfrom
Open
feat: add Copilot CLI Keychain token discovery and multi-account CLI support#84Shaglock wants to merge 3 commits intoopgginc:mainfrom
Shaglock wants to merge 3 commits intoopgginc:mainfrom
Conversation
…support Add macOS Keychain as a token source for GitHub Copilot, enabling automatic discovery of credentials stored by `copilot-cli`. Rewrite the CLI's CopilotCLIProvider to use TokenManager for multi-account support (matching the GUI provider), and fix table column alignment for long provider names.
Contributor
|
Here is the summary of the changes in this PR: This PR significantly enhances the CLI tool by adding multi-account support for Copilot (including discovery via macOS Keychain), aligns the CLI provider logic with the GUI version, and updates Code Review Feedback
This is just a quick preview. |
Shaglock
changed the title
Shaglock
changed the title
Shaglock
changed the title
Member
|
@opgginc/op-gg-ai-devops |
Contributor
✅ AI Code Review CompletedReview finished. Please address the feedback regarding usage logic, concurrency, and keychain access. Review Plan
Reviewed with |
![op-gg-ai-devops[bot]](https://avatars.githubusercontent.com/in/2680015?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🤖 OP.GG DevOps AI Code Review
This PR enhances the CopilotCLIProvider to support multi-account discovery via TokenManager, aligning the CLI behavior with the GUI application. It introduces Keychain access for token retrieval and improves output formatting for multiple accounts.
🔍 Key Findings
- Logic Change in Usage Calculation: The CLI now uses the shared
CopilotUsagemodel, which calculatesusedRequestsdifferently than the previous CLI implementation. The old implementation includednetQuantity(overage) and respectedfilteredUserPremiumRequestEntitlement. The shared model only usesdiscountQuantityanduserPremiumRequestEntitlement. Please verify if this simplification is intentional or if the shared model needs updating. - Concurrency Optimization: The token discovery process fetches plan info and user login sequentially for each account. For users with multiple accounts, this could be parallelized to improve performance.
- Keychain Access in CLI: The integration of
TokenManagermeans the CLI will now attempt to access the macOS Keychain. Ensure this behaves correctly in headless environments (e.g., CI/CD) and that users are aware of potential permission prompts.
🛠️ Suggestions
I have included inline suggestions to improve performance, clean up unused code, and refine the table formatting logic.
|
|
||
| private var cachedCustomerId: String? | ||
|
|
||
| private var cachedUserEmail: String? |
Contributor
There was a problem hiding this comment.
It seems cachedUserEmail is assigned but never read. If it's not needed for future logic, consider removing it to keep the state clean.
Suggested change
| private var cachedUserEmail: String? | |
| private var cachedCustomerId: String? |
Comment on lines
151
to
169
| private func fetchTokenInfos(_ accounts: [CopilotAuthAccount]) async -> [CopilotTokenInfo] { | ||
| var infos: [CopilotTokenInfo] = [] | ||
|
|
||
| for account in accounts { | ||
| let planInfo = await TokenManager.shared.fetchCopilotPlanInfo(accessToken: account.accessToken) | ||
| var login = account.login | ||
|
|
||
| if login == nil { | ||
| login = await fetchCopilotUserLogin(accessToken: account.accessToken) | ||
| } | ||
|
|
||
| let accountId = account.accountId ?? planInfo?.userId ?? login | ||
| infos.append( | ||
| CopilotTokenInfo( | ||
| accountId: accountId, | ||
| login: login, | ||
| planInfo: planInfo, | ||
| authSource: account.authSource, | ||
| source: account.source |
Contributor
There was a problem hiding this comment.
Fetching plan info and user login sequentially for each account might be slow if there are many accounts. Consider using TaskGroup to fetch these in parallel.
Suggested change
| private func fetchTokenInfos(_ accounts: [CopilotAuthAccount]) async -> [CopilotTokenInfo] { | |
| var infos: [CopilotTokenInfo] = [] | |
| for account in accounts { | |
| let planInfo = await TokenManager.shared.fetchCopilotPlanInfo(accessToken: account.accessToken) | |
| var login = account.login | |
| if login == nil { | |
| login = await fetchCopilotUserLogin(accessToken: account.accessToken) | |
| } | |
| let accountId = account.accountId ?? planInfo?.userId ?? login | |
| infos.append( | |
| CopilotTokenInfo( | |
| accountId: accountId, | |
| login: login, | |
| planInfo: planInfo, | |
| authSource: account.authSource, | |
| source: account.source | |
| await withTaskGroup(of: CopilotTokenInfo.self) { group in | |
| for account in accounts { | |
| group.addTask { | |
| let planInfo = await TokenManager.shared.fetchCopilotPlanInfo(accessToken: account.accessToken) | |
| var login = account.login | |
| if login == nil { | |
| login = await self.fetchCopilotUserLogin(accessToken: account.accessToken) | |
| } | |
| let accountId = account.accountId ?? planInfo?.userId ?? login | |
| return CopilotTokenInfo( | |
| accountId: accountId, | |
| login: login, | |
| planInfo: planInfo, | |
| authSource: account.authSource, | |
| source: account.source | |
| ) | |
| } | |
| } | |
| for await info in group { | |
| infos.append(info) | |
| } | |
| } |
CopilotMonitor/CLI/main.swift
Outdated
| let totalWidth = columnWidths.provider + columnWidths.type + columnWidths.usage + 30 + 6 | ||
|
|
||
| private static func formatSeparator(providerWidth: Int) -> String { | ||
| let totalWidth = providerWidth + typeWidth + usageWidth + 30 + 6 |
Contributor
There was a problem hiding this comment.
The hardcoded 30 for the metrics column width might cause the separator line to be too short if the metrics string is long (e.g., "$12.34 spent (resets Jan 1)"). Consider calculating the max width of the metrics column dynamically, similar to computeProviderWidth, or at least increasing this constant to cover typical output lengths.
…fetch, fix separator width, Add tests covering all three areas
Three changes from PR review feedback:
1. Remove unused `cachedUserEmail` (CopilotCLIProvider)
The property was declared and assigned in the cookie branch of `fetch()`
but never read. Remove the declaration and the assignment so the actor
carries no stale mutable state.
2. Parallelise `fetchTokenInfos` (CopilotCLIProvider)
The previous sequential `for` loop awaited each account's plan-info and
user-login network requests one at a time. Replace it with
`withTaskGroup` so every account fires its requests concurrently.
The `dedupeTokenInfos` call at the end is preserved unchanged.
For users with several Copilot accounts (e.g. personal + org), startup
time for the Copilot row is now bounded by the slowest single request
rather than the sum of all requests.
3. Dynamic metrics column width in TableFormatter (CLI)
`formatSeparator` previously hardcoded a 30-character metrics column,
which caused the separator line to be shorter than data rows whenever
auth-source labels or long email addresses pushed the metrics string
past 30 characters (e.g. "Browser Cookies (Chrome/Brave/Arc/Edge)").
Add `computeMetricsWidth()`, which mirrors `computeProviderWidth()` and
scans every rendered metrics string — including Gemini multi-account
rows, generic multi-account rows with auth-source brackets, and
single-account rows — to find the true maximum. Thread the computed
value through `formatSeparator(providerWidth:metricsWidth:)` so the
separator is always exactly as wide as the widest row.
Add tests covering all three areas:
- CLIFormatterTests: 8 new cases for TableFormatter
· separator length equals header length for a single-account result
· separator grows to accommodate long auth-source bracket labels
· `accountLabel` uses `accountId` when present, falls back to `#N`
· absolute-path auth sources are shortened to just the filename
· tilde-path auth sources are shortened to just the filename
· non-path auth sources (e.g. "Browser Cookies …") pass through verbatim
· separator is ≥ every data row width for Gemini multi-account results
- CopilotProviderTests: 4 new cases for CopilotAuthSource / sourcePriority
· all four enum cases exist with unique descriptions
· each case's `description` string matches the expected value
(including the newly-added `copilotCliKeychain` case)
· relative priority ordering: opencodeAuth > copilotCliKeychain >
vscodeHosts > vscodeApps
· absolute priority values: 3, 2, 1, 0
Contributor
Author
|
Tried to address the comments :) |
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.
Disclaimer
The initial issue (fix #73) I had was that my copilot sub was not detected through opencode auth token.
I have no Swift language knowledge, I tried to guide Claude Opus 4.6 to fix my problem. Then I went into a little rabbit hole trying to support two copilot subs at once. Obviously there is probably a lot of slop here, not sure if the keychain access is implemented correctly, and, but hopefully if it's not merged, at least it's a good starting point for someone.
Summary
copilot-cliservice) as a new GitHub Copilot token discovery sourceCopilotCLIProviderto useTokenManagerfor multi-account token discovery, matching the GUI provider's logicquota_snapshotsAPI format support for Copilot plan info#1/#2numbering in the menuDetailed Changes
Token Discovery — Keychain Support (TokenManager.swift)
copilotCliKeychaincase toCopilotAuthSourceenum withCustomStringConvertibleconformancereadCopilotCliKeychainAccounts()using a two-step Keychain query:kSecReturnAttributes+kSecMatchLimitAllto list all itemskSecReturnData+kSecMatchLimitOneto get the passwordkSecMatchLimitAll+kSecReturnDatareturnserrSecParam(-50) on macOShttps://github.com:username)getGitHubCopilotAccounts()between OpenCode auth and VS Code file discoverydedupeCopilotAccounts()priority ordering:Copilot API — quota_snapshots Format (TokenManager.swift)
quota_snapshotsAPI response format alongside the legacymonthly_quotas/limited_user_quotasformatquota_snapshotscontains per-type objects withentitlement,remaining, andunlimitedfieldssnapshotEntitlement = 0,snapshotRemaining = 0placeholder)GUI — Display Names (StatusBarController.swift, CopilotProvider.swift)
GitHub Copilot (Shaglock)) instead of generic index numbers (GitHub Copilot #1)GitHub Copilot (Shaglock) - OpenCodeinstead ofGitHub Copilot #1 (OpenCode)CopilotProvider.sourcePriority()to includecopilotCliKeychaincase with priority 2CLI — CopilotCLIProvider Rewrite (CopilotCLIProvider.swift)
Complete rewrite from ~276 lines to ~544 lines. The old provider only supported browser cookies and returned a single
ProviderResult. The new provider:TokenManager.shared.getGitHubCopilotAccounts()for token discovery (OpenCode auth, Keychain, VS Code files)ProviderAccountResultarrayCandidateDedupe.merge()for cross-source deduplication/userAPI when not available from token metadatafetchCustomerId,fetchUsageData) return optionals instead of throwing, enabling graceful fallbackCopilotUsagestruct (now uses the shared model fromCopilotUsage.swift)CLI — Table Formatter Alignment Fix (main.swift)
computeProviderWidth()that scans ALL row labels (regular providers, Gemini multi-account, generic multi-account) before renderingformatHeader(),formatSeparator(),formatRow(),formatGeminiAccountRow(),formatAccountRow()shortenAuthSource()to display file paths as just the filename (e.g.,/Users/x/.local/share/opencode/auth.jsonbecomesauth.json)accountLabel()andgeminiLabel()helpers shared between width computation and renderingBuild Configuration (project.pbxproj)
CopilotUsage.swiftto the CLI target's Sources build phase so the CLI can use the shared modelFiles Changed
CopilotMonitor/CopilotMonitor/Services/TokenManager.swiftCopilotMonitor/CopilotMonitor/Providers/CopilotProvider.swiftcopilotCliKeychaintosourcePriority()CopilotMonitor/CopilotMonitor/App/StatusBarController.swiftCopilotMonitor/CLI/Providers/CopilotCLIProvider.swiftCopilotMonitor/CLI/main.swiftCopilotMonitor/CopilotMonitor.xcodeproj/project.pbxprojTesting
Both targets build successfully (
BUILD SUCCEEDED)CLI discovers accounts from both OpenCode auth and Keychain sources
Table output is correctly aligned with dynamic column widths:
Example of new table render
JSON output correctly serializes both accounts with all fields
GUI menu shows multiple accounts correctly