Draft
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to enable Copilot Chat usage in air-gapped environments by allowing client-side BYOK (Bring Your Own Key) to work without GitHub authentication, primarily by treating “BYOK enabled” as an alternative to “signed in” across chat UI/setup flows.
Changes:
- Update chat UI/setup gating so “signed out” (entitlement unknown) states don’t block chat/tips/setup when
clientByokEnabledis true. - Adjust Models management UI to allow adding models when
clientByokEnabledis true. - Modify the Copilot extension to always set the
github.copilot.clientByokEnabledcontext key and to register BYOK providers without requiring auth; remove the previous token-based BYOK enablement check.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/welcomeGettingStarted/common/gettingStartedContent.ts | Hides the signed-out Copilot setup step when BYOK is enabled via context key. |
| src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatTipContentPart.ts | Avoids triggering setup for command links when entitlement is unknown but BYOK is enabled. |
| src/vs/workbench/contrib/chat/browser/chatTipService.ts | Allows tips to show when signed out if BYOK is enabled. |
| src/vs/workbench/contrib/chat/browser/chatStatus/chatStatusEntry.ts | Treats “signed out” status UI as not applicable when BYOK is enabled. |
| src/vs/workbench/contrib/chat/browser/chatStatus/chatStatusDashboard.ts | Adjusts dashboard sections and “can use chat” logic to allow BYOK when signed out. |
| src/vs/workbench/contrib/chat/browser/chatSetup/chatSetupRunner.ts | Skips sign-in dialog button set when BYOK is enabled. |
| src/vs/workbench/contrib/chat/browser/chatSetup/chatSetupProviders.ts | Bypasses setup requirement when entitlement is unknown but BYOK is enabled. |
| src/vs/workbench/contrib/chat/browser/chatManagement/chatModelsWidget.ts | Allows “Add Models…” when BYOK is enabled (independent of entitlement). |
| src/vs/workbench/contrib/chat/browser/agentSessions/experiments/agentTitleBarStatusWidget.ts | Treats “signed out” titlebar state as not applicable when BYOK is enabled. |
| extensions/copilot/src/extension/contextKeys/vscode-node/contextKeys.contribution.ts | Sets the github.copilot.clientByokEnabled context key (now unconditionally). |
| extensions/copilot/src/extension/byok/vscode-node/byokContribution.ts | Registers BYOK providers without authentication and fetches known models from CDN. |
| extensions/copilot/src/extension/byok/common/byokProvider.ts | Removes the token/CAPI-based isBYOKEnabled helper that previously enforced policy gating. |
Copilot's findings
Comments suppressed due to low confidence (3)
extensions/copilot/src/extension/byok/vscode-node/byokContribution.ts:52
_byokProvidersRegisteredis set totruebefore awaitingfetchKnownModelList(). If that fetch throws (common in offline/air-gapped environments) you’ll end up in a state where providers were never registered but retries are blocked because_byokProvidersRegisteredremainstrue. Move the flag assignment to after the successful registration path (or reset it on failure).
this._byokProvidersRegistered = true;
const instantiationService = this._instantiationService;
// Update known models list from CDN so all providers have the same list
const knownModels = await this.fetchKnownModelList(this._fetcherService);
if (this._store.isDisposed) {
return;
}
extensions/copilot/src/extension/byok/vscode-node/byokContribution.ts:78
fetchKnownModelListalways fetches the known models list fromhttps://main.vscode-cdn.net/...and does not handle network errors. In an air-gapped/offline scenario this will throw and currently prevents BYOK providers from registering at all. Wrap the fetch/json parsing intry/catchand fall back to an empty known-models map (and log atwarn/info) so BYOK still works without internet access.
private async fetchKnownModelList(fetcherService: IFetcherService): Promise<Record<string, BYOKKnownModels>> {
const data = await (await fetcherService.fetch('https://main.vscode-cdn.net/extensions/copilotChat.json', { method: 'GET', callSite: 'byok-known-models' })).json();
// Use this for testing with changes from a local file. Don't check in
// const data = JSON.parse((await this._fileSystemService.readFile(URI.file('/Users/roblou/code/vscode-engineering/chat/copilotChat.json'))).toString());
let knownModels: Record<string, BYOKKnownModels>;
if (data.version !== 1) {
this._logService.warn('BYOK: Copilot Chat known models list is not in the expected format. Defaulting to empty list.');
knownModels = {};
} else {
knownModels = data.modelInfo;
}
this._logService.info('BYOK: Copilot Chat known models list fetched successfully.');
return knownModels;
extensions/copilot/src/extension/byok/common/byokProvider.ts:160
- Removing
isBYOKEnabledalso removes the existing policy gating (scenario automation override + the!isGHEcheck + token-based allowlist viaisInternal/isIndividual/isClientBYOKEnabled). With the rest of this PR registering BYOK providers unconditionally, there no longer appears to be any centralized check preventing BYOK enablement in environments that were previously excluded (e.g. GitHub Enterprise / non-dotcom API URLs) or where org policy might disallow it. If BYOK must be available without authentication, consider replacing this with an explicit, non-auth-based allow/deny mechanism (config/feature flag) so policy constraints are still enforceable.
/**
* Result of handling an API key update operation.
*/
export interface HandleAPIKeyUpdateResult {
/**
* The new API key value, or undefined if the key was deleted or operation was cancelled.
- Files reviewed: 12/12 changed files
- Comments generated: 3
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Contributor
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (3)
extensions/copilot/src/extension/byok/vscode-node/byokContribution.ts:77
- Same
ILogService.errormisuse here: the error object should be the first argument and the context message the second. As written, the error is passed as the message parameter and will be stringified, losing useful diagnostics.
this._register(lm.onDidChangeChatModels(() => {
void this._updateHasByokModelsContext().catch(err => {
this._logService.error('BYOK: Failed to update BYOK models context.', err);
});
extensions/copilot/src/extension/byok/vscode-node/byokContribution.ts:95
- Same
ILogService.errormisuse here: passerras the first argument and the context string as the second so the logger can record proper error details.
commands.executeCommand('setContext', hasByokModelsContextKey, hasModels);
} catch (err) {
this._logService.error('BYOK: Failed to update BYOK models context.', err);
commands.executeCommand('setContext', hasByokModelsContextKey, false);
}
src/vs/workbench/contrib/chat/browser/widget/input/chatModelPicker.ts:179
- New behavior:
shouldShowManageModelsActionnow returns true whenclientByokEnabledis set, even for signed-out /ChatEntitlement.Unknownusers. There are existing unit tests forbuildModelPickerItems, but none cover this new branch (e.g.entitlement: Unknown+clientByokEnabled: trueshould still include the “Manage Models…” entry). Adding a focused test would prevent regressions.
function shouldShowManageModelsAction(chatEntitlementService: IChatEntitlementService): boolean {
return chatEntitlementService.clientByokEnabled ||
chatEntitlementService.entitlement === ChatEntitlement.Free ||
chatEntitlementService.entitlement === ChatEntitlement.EDU ||
chatEntitlementService.entitlement === ChatEntitlement.Pro ||
chatEntitlementService.entitlement === ChatEntitlement.ProPlus ||
chatEntitlementService.entitlement === ChatEntitlement.Business ||
chatEntitlementService.entitlement === ChatEntitlement.Enterprise ||
chatEntitlementService.isInternal;
- Files reviewed: 19/19 changed files
- Comments generated: 2
Contributor
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
extensions/copilot/src/extension/byok/vscode-node/byokContribution.ts:109
_fetchKnownModelListWithTimeoutusesPromise.racewith asetTimeout, but the timer isn’t cleared when the CDN fetch wins. This can emit the "CDN fetch timed out" warning ~5s later even on success (and keeps an extra timer alive). Consider capturing the timeout handle andclearTimeoutwhenfetchKnownModelListresolves first (or use a cancellable timeout/AbortController-based helper).
private async _fetchKnownModelListWithTimeout(fetcherService: IFetcherService): Promise<Record<string, BYOKKnownModels>> {
const CDN_FETCH_TIMEOUT_MS = 5000;
return Promise.race([
this.fetchKnownModelList(fetcherService),
new Promise<Record<string, BYOKKnownModels>>(resolve => setTimeout(() => {
this._logService.warn('BYOK: CDN fetch timed out. Registering providers with empty known models list.');
resolve({});
}, CDN_FETCH_TIMEOUT_MS))
]);
- Files reviewed: 19/19 changed files
- Comments generated: 3
Comment on lines
+221
to
+222
| // When not signed in, BYOK is available by default | ||
| commands.executeCommand('setContext', clientByokEnabledContextKey, true); |
There was a problem hiding this comment.
_updateClientByokEnabledContext treats any failure to mint/read a Copilot token as "not signed in" and sets github.copilot.clientByokEnabled to true. Because getCopilotToken() can throw for reasons other than signed-out (e.g. transient token minting/network errors, subscription errors), this can temporarily/incorrectly enable BYOK UI and bypass enterprise policy checks. Consider distinguishing "no session" from other failures (e.g. use cached copilotToken/session state when available, or preserve the previous context value on error) instead of unconditionally defaulting to true in the catch block.
Suggested change
| // When not signed in, BYOK is available by default | |
| commands.executeCommand('setContext', clientByokEnabledContextKey, true); | |
| const cachedCopilotToken = this._authenticationService.copilotToken; | |
| if (cachedCopilotToken) { | |
| const byokEnabled = cachedCopilotToken.isInternal || cachedCopilotToken.isIndividual || cachedCopilotToken.isClientBYOKEnabled(); | |
| commands.executeCommand('setContext', clientByokEnabledContextKey, byokEnabled); | |
| return; | |
| } | |
| this._logService.trace(`[context keys] Failed to resolve client BYOK context: ${e instanceof Error ? e.message : String(e)}`); | |
| // Preserve the existing context value when token resolution fails without cached state. |
Comment on lines
258
to
269
| private async doInvoke(request: IChatAgentRequest, progress: (part: IChatProgress) => void, chatService: IChatService, languageModelsService: ILanguageModelsService, chatWidgetService: IChatWidgetService, chatAgentService: IChatAgentService, languageModelToolsService: ILanguageModelToolsService, defaultAccountService: IDefaultAccountService): Promise<IChatAgentResult> { | ||
| const hasByokModels = this.chatEntitlementService.hasByokModels; | ||
| if ( | ||
| !this.context.state.completed || // Setup not completed | ||
| (!this.context.state.completed && !hasByokModels) || // Setup not completed (unless BYOK models are configured) | ||
| this.context.state.disabled || // Extension disabled: run setup to enable | ||
| this.context.state.untrusted || // Workspace untrusted: run setup to ask for trust | ||
| this.context.state.entitlement === ChatEntitlement.Available || // Entitlement available: run setup to sign up | ||
| ( | ||
| this.context.state.entitlement === ChatEntitlement.Unknown && // Entitlement unknown: run setup to sign in / sign up | ||
| !this.chatEntitlementService.anonymous // unless anonymous access is enabled | ||
| !this.chatEntitlementService.anonymous && // unless anonymous access is enabled | ||
| !hasByokModels // unless BYOK models are configured | ||
| ) |
There was a problem hiding this comment.
hasByokModels is used here to bypass running the setup/sign-in flow. Because BYOK models are persisted in extension global storage (not account-scoped), hasByokModels can remain true after switching to a different signed-in account where BYOK is disabled by policy, which would incorrectly skip setup/sign-in. Consider gating this bypass on clientByokEnabled && hasByokModels (or another policy-aware signal) so stale BYOK config can’t override enterprise BYOK restrictions.
See below for a potential fix:
const canBypassSetupWithByok = this.chatEntitlementService.clientByokEnabled && hasByokModels;
if (
(!this.context.state.completed && !canBypassSetupWithByok) || // Setup not completed (unless BYOK models are configured and allowed)
this.context.state.disabled || // Extension disabled: run setup to enable
this.context.state.untrusted || // Workspace untrusted: run setup to ask for trust
this.context.state.entitlement === ChatEntitlement.Available || // Entitlement available: run setup to sign up
(
this.context.state.entitlement === ChatEntitlement.Unknown && // Entitlement unknown: run setup to sign in / sign up
!this.chatEntitlementService.anonymous && // unless anonymous access is enabled
!canBypassSetupWithByok // unless BYOK models are configured and allowed
Comment on lines
32
to
99
| constructor( | ||
| @IFetcherService private readonly _fetcherService: IFetcherService, | ||
| @ILogService private readonly _logService: ILogService, | ||
| @ICAPIClientService private readonly _capiClientService: ICAPIClientService, | ||
| @IVSCodeExtensionContext extensionContext: IVSCodeExtensionContext, | ||
| @IAuthenticationService authService: IAuthenticationService, | ||
| @IInstantiationService private readonly _instantiationService: IInstantiationService, | ||
| ) { | ||
| super(); | ||
| this._byokStorageService = new BYOKStorageService(extensionContext); | ||
| this._authChange(authService, this._instantiationService); | ||
|
|
||
| this._register(authService.onDidAuthenticationChange(() => { | ||
| this._authChange(authService, this._instantiationService); | ||
| })); | ||
| void this._registerProviders().catch(err => { | ||
| this._byokProvidersRegistered = false; | ||
| this._logService.error(err instanceof Error ? err : String(err), 'BYOK: Failed to register providers.'); | ||
| }); | ||
| } | ||
|
|
||
| private async _authChange(authService: IAuthenticationService, instantiationService: IInstantiationService) { | ||
| const byokEnabled = authService.copilotToken && isBYOKEnabled(authService.copilotToken, this._capiClientService); | ||
| private async _registerProviders() { | ||
| if (this._byokProvidersRegistered) { | ||
| return; | ||
| } | ||
|
|
||
| this._byokProvidersRegistered = true; | ||
| const instantiationService = this._instantiationService; | ||
|
|
||
| if (!byokEnabled && this._byokProvidersRegistered) { | ||
| this._logService.info('BYOK: Disabling BYOK providers due to account change.'); | ||
| this._byokRegistrations.clear(); | ||
| this._providers.clear(); | ||
| this._byokProvidersRegistered = false; | ||
| // Fetch known models from CDN for model metadata (capabilities, token limits). | ||
| // Uses a timeout to avoid blocking provider registration in air-gapped/offline environments. | ||
| const knownModels = await this._fetchKnownModelListWithTimeout(this._fetcherService); | ||
| if (this._store.isDisposed) { | ||
| return; | ||
| } | ||
|
|
||
| if (byokEnabled && !this._byokProvidersRegistered) { | ||
| this._byokProvidersRegistered = true; | ||
| // Update known models list from CDN so all providers have the same list | ||
| const knownModels = await this.fetchKnownModelList(this._fetcherService); | ||
| if (this._store.isDisposed) { | ||
| return; | ||
| } | ||
| this._providers.set(OllamaLMProvider.providerName.toLowerCase(), instantiationService.createInstance(OllamaLMProvider, this._byokStorageService)); | ||
| this._providers.set(AnthropicLMProvider.providerName.toLowerCase(), instantiationService.createInstance(AnthropicLMProvider, knownModels[AnthropicLMProvider.providerName], this._byokStorageService)); | ||
| this._providers.set(GeminiNativeBYOKLMProvider.providerName.toLowerCase(), instantiationService.createInstance(GeminiNativeBYOKLMProvider, knownModels[GeminiNativeBYOKLMProvider.providerName], this._byokStorageService)); | ||
| this._providers.set(XAIBYOKLMProvider.providerName.toLowerCase(), instantiationService.createInstance(XAIBYOKLMProvider, knownModels[XAIBYOKLMProvider.providerName], this._byokStorageService)); | ||
| this._providers.set(OAIBYOKLMProvider.providerName.toLowerCase(), instantiationService.createInstance(OAIBYOKLMProvider, knownModels[OAIBYOKLMProvider.providerName], this._byokStorageService)); | ||
| this._providers.set(OpenRouterLMProvider.providerName.toLowerCase(), instantiationService.createInstance(OpenRouterLMProvider, this._byokStorageService)); | ||
| this._providers.set(AzureBYOKModelProvider.providerName.toLowerCase(), instantiationService.createInstance(AzureBYOKModelProvider, this._byokStorageService)); | ||
| this._providers.set(CustomOAIBYOKModelProvider.providerName.toLowerCase(), instantiationService.createInstance(CustomOAIBYOKModelProvider, this._byokStorageService)); | ||
| this._providers.set(OllamaLMProvider.providerName.toLowerCase(), instantiationService.createInstance(OllamaLMProvider, this._byokStorageService)); | ||
| this._providers.set(AnthropicLMProvider.providerName.toLowerCase(), instantiationService.createInstance(AnthropicLMProvider, knownModels[AnthropicLMProvider.providerName], this._byokStorageService)); | ||
| this._providers.set(GeminiNativeBYOKLMProvider.providerName.toLowerCase(), instantiationService.createInstance(GeminiNativeBYOKLMProvider, knownModels[GeminiNativeBYOKLMProvider.providerName], this._byokStorageService)); | ||
| this._providers.set(XAIBYOKLMProvider.providerName.toLowerCase(), instantiationService.createInstance(XAIBYOKLMProvider, knownModels[XAIBYOKLMProvider.providerName], this._byokStorageService)); | ||
| this._providers.set(OAIBYOKLMProvider.providerName.toLowerCase(), instantiationService.createInstance(OAIBYOKLMProvider, knownModels[OAIBYOKLMProvider.providerName], this._byokStorageService)); | ||
| this._providers.set(OpenRouterLMProvider.providerName.toLowerCase(), instantiationService.createInstance(OpenRouterLMProvider, this._byokStorageService)); | ||
| this._providers.set(AzureBYOKModelProvider.providerName.toLowerCase(), instantiationService.createInstance(AzureBYOKModelProvider, this._byokStorageService)); | ||
| this._providers.set(CustomOAIBYOKModelProvider.providerName.toLowerCase(), instantiationService.createInstance(CustomOAIBYOKModelProvider, this._byokStorageService)); | ||
|
|
||
| for (const [providerName, provider] of this._providers) { | ||
| this._byokRegistrations.add(lm.registerLanguageModelChatProvider(providerName, provider)); | ||
| for (const [providerName, provider] of this._providers) { | ||
| this._byokRegistrations.add(lm.registerLanguageModelChatProvider(providerName, provider)); | ||
| } | ||
|
|
||
| await this._updateHasByokModelsContext(); | ||
|
|
||
| // Update context key when language models change (e.g., model configured/removed) | ||
| this._register(lm.onDidChangeChatModels(() => { | ||
| void this._updateHasByokModelsContext().catch(err => { | ||
| this._logService.error(err instanceof Error ? err : String(err), 'BYOK: Failed to update BYOK models context.'); | ||
| }); | ||
| })); | ||
| } | ||
|
|
||
| async _updateHasByokModelsContext(): Promise<void> { | ||
| try { | ||
| let hasModels = false; | ||
| for (const vendor of this._providers.keys()) { | ||
| const models = await lm.selectChatModels({ vendor }); | ||
| if (models.length > 0) { | ||
| hasModels = true; | ||
| break; | ||
| } | ||
| } | ||
| commands.executeCommand('setContext', hasByokModelsContextKey, hasModels); | ||
| } catch (err) { | ||
| this._logService.error(err instanceof Error ? err : String(err), 'BYOK: Failed to update BYOK models context.'); | ||
| commands.executeCommand('setContext', hasByokModelsContextKey, false); | ||
| } | ||
| } |
There was a problem hiding this comment.
BYOK model configuration is stored in extension globalState (not account-scoped), but this contribution now registers providers unconditionally and derives github.copilot.hasByokModels purely from lm.selectChatModels. This means BYOK models configured under a previous account can still make hasByokModels true after switching to an enterprise-managed account where BYOK is disabled, which can bypass setup/sign-in UI and potentially violate org BYOK policy. Consider scoping BYOK storage by account/org (or clearing/ignoring stored models on auth change for managed accounts), and/or incorporating the current BYOK policy signal (clientByokEnabled/token) when computing hasByokModels and exposing models.
This issue also appears on line 101 of the same file.
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
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.
To enable chat in air-gapped environments via BYOK.
Fixes #https://github.com/microsoft/vscode-internalbacklog/issues/7044
Fixes #309492
Ref #309501