Allow BYOK endpoints to work without Copilot entitlement. - #324311
Daniel Robbins (danielrobbins) wants to merge 15 commits into
Conversation
Without this patch, when you sign into GitHub in VS Code but don't have a Copilot seat (e.g., you're a GitHub Enterprise user without Copilot, or your subscription lapsed), VS Code refuses to activate BYOK custom endpoint providers. This blocks access to your own AI endpoints even though they have nothing to do with Copilot. This patch fixes this. This is a logic bug: being signed in without Copilot access should not block independent BYOK endpoints. Users experience this as: "My custom AI endpoints work when I'm signed out of GitHub, but stop working the moment I sign in (unless I have Copilot)." This is due to improper nesting of logic inside VS Code. I have personally tested and verified this fix locally. Unit tests have also been updated. Fixes microsoft#324310.
There was a problem hiding this comment.
Pull request overview
This PR fixes a logic bug in the Copilot extension's BYOK (Bring Your Own Key) policy check so that custom endpoint providers remain available when a user is signed into GitHub without a Copilot entitlement. It changes isClientBYOKAllowed to return true (instead of false) for the "GitHub session present, but no Copilot token" case, and updates the corresponding unit test. This addresses issue #324310, where BYOK endpoints worked while signed out but broke after signing into GitHub without Copilot.
Changes:
isClientBYOKAllowednow allows BYOK when a GitHub session exists but no Copilot token is available, and the prior JSDoc describing the fail-closed behavior is replaced with an inline explanation.- The unit test for the missing-token case is renamed and inverted to expect
true.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
extensions/copilot/src/extension/byok/common/byokProvider.ts |
Returns true when signed-in without a Copilot token; replaces the fail-closed JSDoc with an inline comment. |
extensions/copilot/src/extension/byok/common/test/byokProvider.spec.ts |
Updates the test to expect BYOK to be allowed for the signed-in/no-token case. |
Allow BYOK custom endpoints to work for signed-in users who do not have a Copilot entitlement, while still respecting managed enterprise BYOK policy. Replace the previous boolean check with explicit policy source states and a final BYOK policy decision. Cache the last known managed policy so temporary policy lookup failures can preserve enterprise decisions, while still failing closed when managed policy is unavailable and no cached policy exists. Expand unit coverage across signed-out, no-entitlement, expired, individual, internal, managed allow/deny, cached managed policy, and unclassified auth failure cases. Fixes microsoft#324310
|
PR has been updated to comprehensively address all possible account states, with careful defaults to ensure correct, non-developer-blocking behavior while respecting Enterprise Policy settings related to BYOK models. |
|
The merge conflict with current main has been resolved and I reran the focused BYOK tests locally:
Both are passing. This should be ready for maintainer review when someone has a chance. Vritant Bhardwaj (@vritant24) Dmitriy Vasyura (@dmitrivMS) |
|
Vritant Bhardwaj (@vritant24) Giuseppe Cianci (@Giuspepe) Dmitriy Vasyura (@dmitrivMS) I’d appreciate a review when you have a chance. This PR fixes a high-impact BYOK issue: users who are signed into GitHub but do not have a Copilot entitlement are currently blocked from using non-Copilot custom endpoint models, even though those models are independently configured. The focused BYOK tests cover the key permutations, including signed-out users, signed-in users without Copilot entitlement, expired subscriptions, internal/individual accounts, managed allow/deny policy, cached managed policy, and unclassified auth failures. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
I think the version you had a little overengineered, updated with a smaller one. |
|
Vritant Bhardwaj (@vritant24) I still need to test this, but please take a look as well. |
First, thank you for taking a look at this. And yes -- mine is more elaborate -- but there is a method to my madness. The smaller implementation fixes the original bug, but any unclassified Copilot token failure still disables BYOK. This leaves several important failure modes:
The user impact of these scenarios is not trivial. In all three cases, the workaround is to sign out of GitHub/Copilot so VS Code stops trying to use the unavailable Copilot token source. Users may never discover that workaround and simply remain stuck. It is also genuinely silly that signing out of one AI service is required to use an entirely independent one. It's important to address all that potential user pain because it makes VS Code + Copilot brittle for real-world multi-model workflows. In my suggested fix, caching the last known Enterprise policy handles temporary outages and previously evaluated accounts. When Copilot has never been reachable, my patch allows BYOK unless there is an explicit Enterprise policy signal. Returning I am not attached to my exact implementation, and my code can be improved. But these scenarios should be addressed and represented in tests or comments so that the resulting behavior is deliberate and remains understandable to the next developer. Future developers should not have to reconstruct every account, entitlement, enterprise policy, and network failure scenario whenever they touch this code. That is how these kinds of bugs get introduced in the first place, and why I added the policy matrix to encode the decisions and explain their reasoning. Most importantly, no future VS Code user should have to discover that logging out of Copilot is the workaround for getting local models working. That is a painful user experience, and one we should do our best to eliminate. |
|
Dmitriy Vasyura (@dmitrivMS) There are still several issues with your most recent simplified fix. In your current code, BYOK remains gated by Copilot whenever a Copilot token source exists, and token retrieval errors disable BYOK. This is not technically correct or desirable. BYOK and Copilot are orthogonal concerns and should not be coupled in this way unless we have an explicit policy signal. Here are scenarios that should not disable BYOK, but currently do:
We should only block BYOK when the most recently obtained explicit enterprise BYOK policy was “deny.” An operational failure retrieving a Copilot token should not create an implicit denial. Allowing transient network and token failures to disable BYOK creates a brittle and confusing user experience. My original patch is a much more comprehensive fix, although it could still be improved. If you want, I can adjust my PR to use your code as its base and augment it to handle the other cases correctly. |
There was a problem hiding this comment.
Review details
Suppressed comments (3)
extensions/copilot/src/extension/contextKeys/vscode-node/contextKeys.contribution.ts:255
- This adds another uncoordinated async policy update. Authentication and token-change callbacks can overlap while token resolution and global-state updates are pending, allowing an older invocation to execute
setContextafter a newer decision;BYOKContribguards the same race with_policyApplyGeneration, but this context key can then disagree with the actual provider registrations and hide or expose BYOK UI incorrectly. Apply the same latest-generation guard here.
this._updateClientByokEnabledContext();
extensions/copilot/src/extension/byok/vscode-node/byokContribution.ts:50
- The policy cache key depends on the auth provider, enterprise URI, proxy/CAPI URLs, and auth type, but this contribution only reapplies policy on authentication/token events. These settings are handled live by
DomainService; proxy/CAPI/auth-type changes do not necessarily emit either subscribed event, so providers can remain registered under the previous source's policy (or remain incorrectly unregistered) until a later token refresh. Subscribe to configuration changes affecting every cache-key input and call_applyPolicy()there as well.
this._register(this._authService.onDidAuthenticationChange(() => void this._applyPolicy()));
this._register(this._authService.onDidCopilotTokenChange(() => void this._applyPolicy()));
extensions/copilot/src/extension/contextKeys/vscode-node/contextKeys.contribution.ts:229
- This value now depends on configuration inputs used by
resolveClientBYOKAllowed, but it is only recomputed at startup and on authentication/token changes. A live proxy/CAPI/auth-type source change can therefore leavegithub.copilot.clientByokEnabledstale, which continues to gate BYOK model visibility inHasByokModelsContributioneven after the provider policy changes. Add a configuration-change subscription for all inputs used by the resolver's cache key.
This issue also appears on line 255 of the same file.
commands.executeCommand('setContext', clientByokEnabledContextKey, await resolveClientBYOKAllowed(this._authenticationService, this._extensionContext, this._logService, this._configService));
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
|
|
||
| void this._inspectContext().catch(console.error); | ||
| void this._updatePermissiveSessionContext().catch(console.error); | ||
| void this._updateClientByokEnabledContext().catch(console.error); |
There was a problem hiding this comment.
Overall I like the idea of removing it from here, but we need to verify UI scenarios to make sure it is set soon enough from the other place so that things work correctly (i.e. we hide/show appropriate UI elements, especially on startup).
There was a problem hiding this comment.
Addressed with new commit. I moved BYOKContrib earlier. Also added some tests related to this. Hopefully this addresses the startup ordering concern. If this doesn't sufficiently address the UI concern, feel free to revert the parts you don't like. The policy and caching parts are all solid and are the important parts of the PR.
|
Dmitriy Vasyura (@dmitrivMS) let me know what you'd like me to do with this PR. I think it should be ready. If my most recent commits don't address your concerns with UI startup, I'm fine taking them out. The core fixes we're working on appear to be solid and should be ready to go. Let me know if you want me to make changes or if you can take it from here. Thanks! |
|
Field evidence for the failure mode under discussion in this thread — specifically Daniel Robbins (@danielrobbins)' point from 1 Aug that "token retrieval errors disable BYOK" and that failure-to-retrieve-the-token should not be treated as a policy signal. This is not hypothetical. It happened to me in production today, during the GitHub Copilot incident on 2026-08-17 ( The concrete result: a fully local, NPU-accelerated model served from Environment
The sequence, verbatimReproduced identically on three consecutive window reloads (12:52:33, 12:57:37, 13:01:06). The token failed on the first attempt and succeeded on retry every single time — the teardown is effectively guaranteed whenever GitHub is degraded. Three observations that may be useful for the design debate1. A transport error is being converted into a permanent policy decision. The token becomes available 1.368 seconds after the teardown, carrying a valid enterprise entitlement. Nothing re-registers the providers. A sub-two-second blip disables BYOK for the entire session, recoverable only by reloading — which re-enters the same race. This is the strongest argument I can offer for treating "token temporarily unavailable" as distinct from "not entitled": the entitlement was never in question, only its retrievability. 2. The log message asserts a cause that is demonstrably false.
3. Internal state ends up inconsistent. At Ruled outConfiguration ( Note on the two implementationsI haven't reviewed which variant is currently on the branch, so I won't comment on the specific diff. But as a data point for the tradeoff being discussed: the simplified approach, as characterised in the 1 Aug comment, would not have helped me today, because my failure was an unclassified token retrieval error with a valid entitlement behind it. The distinction between "we know policy denies BYOK" and "we could not reach GitHub to ask" is the entire difference between a working and a non-working local model during an outage. Happy to provide full unredacted logs, test a patched build, or reproduce on demand — during the next Copilot incident this appears to be reliably reproducible. |
Thanks for communicating and validating the severity of this issue. I have my earliest patch from this PR incorporated into Sprouted Code, which are test builds (including binary releases, for MacOS, Windows and Linux) of this source tree. I have not yet updated it with the most recent patch: https://github.com/danielrobbins/sprouted-code https://kernel-seeds.org/projects/sprouted-code/ |
Without this patch, when you sign into GitHub in VS Code but don't have a Copilot seat (e.g., you're a GitHub Enterprise user without Copilot, or your subscription lapsed), VS Code refuses to activate BYOK custom endpoint providers. This blocks access to your own AI endpoints even though they have nothing to do with Copilot.
This is a logic bug: being signed in without Copilot access should not block independent BYOK endpoints. Users experience this as: "My custom AI endpoints work when I'm signed out of GitHub, but stop working the moment I sign in (unless I have Copilot)." The current BYOK policy check conflates GitHub sign-in with Copilot entitlement.
I have personally tested and verified this fix locally. Unit tests have also been updated.
Fixes #324310. The issue contains reproduction steps.