Add shared BYOK provider support - #447
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a secure “bring your own key” (BYOK) configuration path for the Copilot agent integration in Intelligent Terminal/Windows Terminal. It adds settings/UI for a custom OpenAI-compatible provider endpoint while ensuring the provider API key is stored in Windows Credential Manager and only injected into the Copilot child process environment (not settings.json, argv, logs, or ACP payloads).
Changes:
- Add a WTA-side BYOK environment isolation module that scrubs provider env vars for all agent children and resolves the API key from Windows Credential Manager for Copilot only.
- Extend Settings Model + Settings Editor UI to configure Copilot provider base URL and securely save/clear the provider API key (via a credential ID).
- Update TerminalApp’s SharedWta spawn/restart flow to pass environment overrides (provider config + credential ID) and to suppress hosted model selection / Copilot sign-in UX when BYOK is active.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/wta/src/protocol/acp/spawn.rs | Calls BYOK env scrub/restore logic when spawning agent CLIs. |
| tools/wta/src/main.rs | Adds hidden --copilot-byok flag and wires it into app state. |
| tools/wta/src/copilot_byok.rs | New module: provider env key scrubbing + Credential Manager API key resolution. |
| tools/wta/src/app.rs | Adds helper-side BYOK state to suppress Copilot sign-in fallback and keep provider auth errors visible. |
| tools/wta/Cargo.toml | Enables windows-sys Credentials feature for Credential Manager access. |
| src/cascadia/UnitTests_SettingsModel/CustomAgentAndPolicyTests.cpp | Adds roundtrip unit test coverage for new Copilot provider settings fields. |
| src/cascadia/TerminalSettingsModel/MTSMSettings.h | Adds copilotProvider.baseUrl and copilotProvider.apiKeyCredential settings. |
| src/cascadia/TerminalSettingsModel/GlobalAppSettings.idl | Exposes new global settings properties for Copilot provider configuration. |
| src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw | Adds en-US resource strings for the new provider URL + API key UI. |
| src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.idl | Adds projected settings + commands for provider URL and credential-backed API key storage. |
| src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.h | Declares ViewModel surface for provider URL visibility, API key state, save/clear actions. |
| src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp | Implements provider UI visibility, model-list suppression under BYOK, and credential save/clear. |
| src/cascadia/TerminalSettingsEditor/AIAgents.xaml | Adds the provider URL TextBox and API key PasswordBox + buttons to the settings UI. |
| src/cascadia/TerminalApp/TerminalPage.h | Extends snapshots and adds SharedWta environment builder declaration. |
| src/cascadia/TerminalApp/TerminalPage.cpp | Detects BYOK configuration, suppresses --acp-model, passes env overrides, and adds --copilot-byok to helpers. |
| src/cascadia/TerminalApp/SharedWta.h | Extends AcquirePane/Restart/_SpawnLocked to accept environment overrides. |
| src/cascadia/TerminalApp/SharedWta.cpp | Builds a CreateProcess environment block with overrides and caches it for deduped restarts. |
| src/cascadia/inc/CopilotByok.h | New helper for storing/removing provider API keys in Windows Credential Manager. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
tools/wta/src/local_model_provider.rs:156
read_api_keyhard-codes "local model provider credential" in its error messages even when it is reading Copilot BYOK credentials. This makes troubleshooting difficult because the message doesn’t identify which Credential Manager target failed.
if unsafe { CredReadW(target.as_ptr(), CRED_TYPE_GENERIC, 0, &mut credential) } == 0 {
let error = unsafe { GetLastError() };
bail!("failed to read local model provider credential: Win32 error {error}");
}
if credential.is_null() {
tools/wta/src/local_model_provider.rs:166
- The remaining
read_api_keyerrors also hard-code "local model provider credential". Since this helper is used for multiple resources (shared local provider and Copilot BYOK), include the{credential_resource}/{credential_id}in these messages to make failures actionable.
if blob_size == 0 || blob.is_null() {
unsafe { CredFree(credential.cast()) };
bail!("local model provider credential is empty");
}
let bytes = unsafe { std::slice::from_raw_parts(blob, blob_size).to_vec() };
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
tools/wta/src/session_watcher/mod.rs:186
codex-byokdoesn’t appear to be used anywhere else in the repo (only referenced here), while the new Codex BYOK path islocal-model/codex-acp. If this isn’t intentional legacy compatibility, it’s extra noise in the watcher roots.
if let Some(root) = crate::runtime_paths::intelligent_terminal_root() {
roots.push(root.join("codex-byok").join("sessions"));
roots.push(root.join("local-model").join("codex").join("sessions"));
roots.push(root.join("local-model").join("codex-acp").join("sessions"));
src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp:416
- Copilot BYOK uses
AcpModelas the provider model ID (not necessarily present in the hostedavailable_modelslist). As written,_UsesLocalModelProvider()won’t detect Copilot BYOK, so_RebuildAcpModelListFromCache()can still clearAcpModelas “stale”, breaking BYOK configuration and re-enabling hosted model selection.
bool AIAgentsViewModel::_UsesLocalModelProvider() const
{
const auto agent = _GlobalSettings.AcpAgent();
const bool supportedAgent = agent == L"copilot" || agent == L"codex";
return !_isAddingCustomAcpAgent &&
src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw:760
- These new Settings Editor resource keys were added only to en-US. The repo’s .resw localization process expects new keys to be present across the supported locale
.reswfiles (at least added to the qps-ploc* pseudo-locales with English fallback), otherwise localized builds will fall back inconsistently.
<data name="AIAgents_LocalModelGroupHeader.Text" xml:space="preserve">
<value>Local model provider</value>
<comment>Section header for the shared local model provider settings.</comment>
</data>
<data name="AIAgents_LocalModelProviderBaseUrl.Header" xml:space="preserve">
<value>Provider URL</value>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
tools/wta/src/local_model_provider.rs:163
read_api_keyis used for both the shared local-model provider and the legacy Copilot BYOK credential, but these error messages hardcode “local model provider credential”, which will be misleading when the failing credential isIntelligentTerminal.CopilotBYOK. Consider making the messages resource-based so logs/errors are accurate without exposing secrets.
bail!("failed to read local model provider credential: Win32 error {error}");
}
if credential.is_null() {
bail!("Credential Manager returned a null local model provider credential");
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
tools/wta/src/session_watcher/mod.rs:176
watched_roots()returns early whenUSERPROFILEis unset, which also drops the Intelligent Terminal state roots that do not depend onUSERPROFILE(they resolve viaLOCALAPPDATA). This prevents watchinglocal-model/*sessions in environments whereUSERPROFILEis missing, despite the updated doc comment saying these roots are included.
pub fn watched_roots() -> Vec<PathBuf> {
let Some(home) = std::env::var_os("USERPROFILE").map(PathBuf::from) else {
return Vec::new();
};
src/cascadia/inc/CopilotByok.h:7
- This header relies on types/macros from WinRT, WIL, GSL, and COM APIs (e.g.,
winrt::hstring,wil::scope_exit,THROW_IF_FAILED,gsl::narrow,CoCreateGuid) but only includes<wincred.h>. As aninc/header intended for cross-project use, it should be self-contained to avoid brittle include-order / PCH coupling.
#pragma once
#include <wincred.h>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7ff40705-2c5c-4816-b238-58406963e31e
Probe native agent models without BYOK before starting the shared BYOK agent, merge both catalogs, and restart only when crossing the BYOK boundary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 96026d43-a1e7-4ccb-9d0f-27fb43cdc1bf
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tools/wta/src/app.rs:2819
merge_custom_modelsallocates a newStringforformat!("intelligent-terminal/{}", custom.model_id)on every element checked byretain, which is unnecessary work (and can become noticeable as the model list grows). Precompute the legacy id once per custom model and compare against that.
This comment has been minimized.
This comment has been minimized.
Keep restart-required BYOK choices visible but disabled in /model, while preserving cloud-to-cloud hot switching and Settings-based model changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 96026d43-a1e7-4ccb-9d0f-27fb43cdc1bf
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
tools/wta/src/protocol/acp/probe.rs:57
- probe_models_impl takes an agent_id parameter, but the non-without_byok branch ignores it and always passes None to spawn_agent_process. If a caller supplies an authoritative agent_id (as probe_cloud_models does), that identity should be forwarded consistently so command-based inference can’t accidentally select the wrong profile/BYOK behavior.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tools/wta/src/protocol/acp/probe.rs:57
probe_models_implaccepts anagent_idparameter but ignores it on the normal (BYOK-enabled) path by callingspawn_agent_process(..., None).
This defeats the intent of making agent_id authoritative (so a custom command that happens to match a built-in executable cannot inherit that built-in agent's BYOK behavior). It also makes probe_models_impl inconsistent with the without_byok branch, which does pass agent_id through.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 96026d43-a1e7-4ccb-9d0f-27fb43cdc1bf
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tools/wta/src/custom_model_provider.rs:174
- In
serde_json::json!, an un-parenthesized identifier key is stringified (e.g.PROVIDER_IDbecomes the literal key "PROVIDER_ID") rather than using the constant’s value ("intelligent-terminal"). This will produce an incorrectmodel_providersmap and break Codex’s provider selection.
Use a parenthesized expression key (or build a serde_json::Map) so the key is the actual provider id string.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 96026d43-a1e7-4ccb-9d0f-27fb43cdc1bf
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 96026d43-a1e7-4ccb-9d0f-27fb43cdc1bf
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 59 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tools/wta/src/app.rs:3280
/modellive-switch enablement is keyed offcustom_model_selection.is_none(). If--custom-model-selectionis present but--custom-modelsfails to parse (or the selection no longer exists in the models list), this will incorrectly treat the session as “BYOK active” and disable switching among cloud models.
Consider treating BYOK as active only when the selected custom id actually exists in self.custom_models.
This comment has been minimized.
This comment has been minimized.
Keep provider contracts, cloud discovery, host model catalogs, and agent launch metadata aligned across Terminal and WTA. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7f7b801e-4b01-456f-a2f8-146d5483b874
Show localized Base URL and Model ID headers without using input placeholders. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7f7b801e-4b01-456f-a2f8-146d5483b874
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 728daa6d-60e2-4d9e-b427-497d951bc85a
Limit OpenAI-compatible Chat Completions providers to Copilot and OpenCode, and explain unsupported agent selections without discarding saved providers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 728daa6d-60e2-4d9e-b427-497d951bc85a
This comment has been minimized.
This comment has been minimized.
Match origin/main CRLF line endings so the PR diff contains only the substantive BYOK changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 728daa6d-60e2-4d9e-b427-497d951bc85a
@check-spelling-bot Report
|
| Dictionary | Entries | Covers | Uniquely |
|---|---|---|---|
| cspell:csharp/csharp.txt | 32 | 2 | 2 |
| cspell:aws/aws.txt | 232 | 2 | 2 |
| cspell:fonts/fonts.txt | 536 | 1 | 1 |
Consider adding to the extra_dictionaries array (in the .github/actions/spelling/config.json file):
"cspell:csharp/csharp.txt",
"cspell:aws/aws.txt",
"cspell:fonts/fonts.txt",
To stop checking additional dictionaries, put (in the .github/actions/spelling/config.json file):
"check_extra_dictionaries": []Forbidden patterns 🙅 (1)
In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves.
These forbidden patterns matched content:
Should be preexisting
[Pp]re[- ]existing
Errors and Warnings ❌ (3)
See the 📂 files view, the 📜action log, 👼 SARIF report, or 📝 job summary for details.
| ❌ Errors and Warnings | Count |
|---|---|
| 54 | |
| ❌ check-file-path | 1 |
| ❌ forbidden-pattern | 2 |
See ❌ Event descriptions for more information.
✏️ Contributor please read this
By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
- ... misspelled, then please correct them instead of using the command.
- ... names, please add them to
.github/actions/spelling/allow/names.txt. - ... APIs, you can add them to a file in
.github/actions/spelling/allow/. - ... just things you're using, please add them to an appropriate file in
.github/actions/spelling/expect/. - ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in
.github/actions/spelling/patterns/.
See the README.md in each directory for more information.
🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉
If the flagged items are 🤯 false positives
If items relate to a ...
-
binary file (or some other file you wouldn't want to check at all).
Please add a file path to the
excludes.txtfile matching the containing file.File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
^refers to the file's path from the root of the repository, so^README\.md$would exclude README.md (on whichever branch you're using). -
well-formed pattern.
If you can write a pattern that would match it,
try adding it to thepatterns.txtfile.Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.
Note that patterns can't match multiline strings.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 117 out of 120 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
tools/wta/src/agent_registry.rs:148
- PR description says shared BYOK providers are supported for Codex, but the agent registry marks Codex as
byok_mode: ByokMode::Unsupported(and the shared provider path is documented/implemented as OpenAI-compatible Chat Completions only). Either update the PR description to remove Codex from the supported set, or extend the BYOK plumbing to cover Codex with an explicit ByokMode and provider contract.
tools/wta/src/ui/model_popup.rs:6 - PR description says
/modelshows all known models and disables choices that require an agent restart, but the/modelpopup documentation here states cloud/native models are intentionally omitted. Please align the PR description with the implemented behavior (or adjust the popup/model list behavior to match the description).
Summary
/modelremains limited to genuine live switchescopilot loginrequirement because Copilot CLI currently rejects logged-out BYOKsession/newcalls in ACP mode (upstream #4016)Scope
/modelshows all known models but disables choices that require an agent restartValidation
CustomAgentAndPolicyTests::CustomModelProvidersRoundtrip: passedTerminalSettingsEditorbuild