Skip to content

Commit e8a76df

Browse files
committed
fix(api): make Copilot provider visibility consistent with other providers
- Change Copilot from TOML-only check to has_value() pattern, consistent with all other providers. Now env var GITHUB_COPILOT_API_KEY works like other provider env vars in get_providers(). - Add helpful error message in delete_provider() when attempting to remove a provider that is only configured via environment variable. The message explains which env var to unset and that a restart is required. For Ollama, checks both OLLAMA_BASE_URL and OLLAMA_API_KEY env vars. - Add comment in test explaining why github_copilot_key is excluded from all_shorthand_keys_register_providers_via_toml test (uses token exchange, not standard shorthand registration). - Extract copilot_default_provider_config() helper to reduce duplication in Copilot provider construction.
1 parent ee04701 commit e8a76df

3 files changed

Lines changed: 83 additions & 20 deletions

File tree

src/api/providers.rs

Lines changed: 61 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -213,16 +213,7 @@ fn build_test_llm_config(provider: &str, credential: &str) -> crate::config::Llm
213213
// LlmManager::get_copilot_token() will exchange it for a real Copilot token.
214214
providers.insert(
215215
provider.to_string(),
216-
crate::config::ProviderConfig {
217-
api_type: crate::config::ApiType::OpenAiChatCompletions,
218-
base_url: crate::config::GITHUB_COPILOT_DEFAULT_BASE_URL.to_string(),
219-
api_key: credential.to_string(),
220-
name: Some("GitHub Copilot".to_string()),
221-
use_bearer_auth: true,
222-
extra_headers: vec![],
223-
api_version: None,
224-
deployment: None,
225-
},
216+
crate::config::copilot_default_provider_config(credential),
226217
);
227218
}
228219

@@ -464,15 +455,6 @@ pub(super) async fn get_providers(
464455
env_set(env_var)
465456
};
466457

467-
// Copilot: only check TOML key, not env var. The env var GITHUB_COPILOT_API_KEY
468-
// is a fallback for config loading but shouldn't keep the provider visible in
469-
// the settings UI after a remove — the env var can't be unset from the process.
470-
let copilot_configured = doc
471-
.get("llm")
472-
.and_then(|llm| llm.get("github_copilot_key"))
473-
.and_then(|val| val.as_str())
474-
.is_some_and(|s| resolve_value(s).is_some());
475-
476458
(
477459
has_value("anthropic_key", "ANTHROPIC_API_KEY"),
478460
has_value("openai_key", "OPENAI_API_KEY"),
@@ -496,7 +478,7 @@ pub(super) async fn get_providers(
496478
has_value("minimax_cn_key", "MINIMAX_CN_API_KEY"),
497479
has_value("moonshot_key", "MOONSHOT_API_KEY"),
498480
has_value("zai_coding_plan_key", "ZAI_CODING_PLAN_API_KEY"),
499-
copilot_configured,
481+
has_value("github_copilot_key", "GITHUB_COPILOT_API_KEY"),
500482
doc.get("llm")
501483
.and_then(|llm| llm.get("provider"))
502484
.and_then(|provider| provider.get("azure"))
@@ -1598,6 +1580,65 @@ pub(super) async fn delete_provider(
15981580
.await
15991581
.map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
16001582

1583+
let doc: toml_edit::DocumentMut = content
1584+
.parse()
1585+
.map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
1586+
1587+
// Check if the provider has a key in TOML
1588+
let has_toml_key = doc
1589+
.get("llm")
1590+
.and_then(|llm| llm.get(key_name))
1591+
.and_then(|val| val.as_str())
1592+
.is_some_and(|s| {
1593+
if let Some(alias) = s.strip_prefix("secret:") {
1594+
// Check if secret exists in secrets store
1595+
state
1596+
.secrets_store
1597+
.load()
1598+
.as_ref()
1599+
.as_ref()
1600+
.and_then(|store| store.get(alias).ok())
1601+
.is_some()
1602+
} else if let Some(var_name) = s.strip_prefix("env:") {
1603+
// Check if env var exists and is non-empty
1604+
std::env::var(var_name)
1605+
.ok()
1606+
.is_some_and(|v| !v.trim().is_empty())
1607+
} else {
1608+
// Direct value
1609+
!s.trim().is_empty()
1610+
}
1611+
});
1612+
1613+
// If no TOML key exists, check if configured via env var
1614+
if !has_toml_key {
1615+
let env_vars: Vec<String> = match provider.as_str() {
1616+
"ollama" => vec!["OLLAMA_BASE_URL".to_string(), "OLLAMA_API_KEY".to_string()],
1617+
_ => vec![format!(
1618+
"{}_API_KEY",
1619+
provider.to_uppercase().replace("-", "_")
1620+
)],
1621+
};
1622+
1623+
let configured_env_var = env_vars.iter().find(|name| {
1624+
std::env::var(name.as_str())
1625+
.ok()
1626+
.is_some_and(|v| !v.trim().is_empty())
1627+
});
1628+
1629+
if let Some(env_var) = configured_env_var {
1630+
return Ok(Json(ProviderUpdateResponse {
1631+
success: false,
1632+
message: format!(
1633+
"Provider '{}' is configured via the {} environment variable. \
1634+
To remove this provider, unset the environment variable and restart Spacebot.",
1635+
provider, env_var
1636+
),
1637+
}));
1638+
}
1639+
}
1640+
1641+
// Now remove from TOML
16011642
let mut doc: toml_edit::DocumentMut = content
16021643
.parse()
16031644
.map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;

src/config.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,10 @@ pub use permissions::{
1818
DiscordPermissions, MattermostPermissions, SignalPermissions, SlackPermissions,
1919
TelegramPermissions, TwitchPermissions,
2020
};
21+
// Only used in tests; #[allow(unused_imports)] prevents warnings during non-test builds.
22+
#[allow(unused_imports)]
2123
pub(crate) use providers::GITHUB_COPILOT_DEFAULT_BASE_URL;
24+
pub(crate) use providers::copilot_default_provider_config;
2225
pub(crate) use providers::default_provider_config;
2326
pub use runtime::RuntimeConfig;
2427
pub use types::*;
@@ -1307,6 +1310,10 @@ maintenance_merge_similarity_threshold = 1.1
13071310
"ollama",
13081311
"localhost:11434",
13091312
),
1313+
// Note: github_copilot_key is intentionally excluded — GitHub Copilot requires
1314+
// special token exchange handling and is registered via
1315+
// LlmManager::get_github_copilot_provider(), not via the standard shorthand
1316+
// provider registration mechanism used by other providers.
13101317
];
13111318

13121319
for (toml_key, toml_value, provider_name, url_substr) in cases {

src/config/providers.rs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,21 @@ pub(super) fn add_shorthand_provider(
286286
}
287287
}
288288

289+
/// Returns the default ProviderConfig for GitHub Copilot with the given API key.
290+
/// Used by API tests and other code that needs Copilot provider configs.
291+
pub fn copilot_default_provider_config(api_key: impl Into<String>) -> super::ProviderConfig {
292+
super::ProviderConfig {
293+
api_type: super::ApiType::OpenAiChatCompletions,
294+
base_url: GITHUB_COPILOT_DEFAULT_BASE_URL.to_string(),
295+
api_key: api_key.into(),
296+
name: Some("GitHub Copilot".to_string()),
297+
use_bearer_auth: true,
298+
extra_headers: vec![],
299+
api_version: None,
300+
deployment: None,
301+
}
302+
}
303+
289304
/// When `[defaults.routing]` is absent from the config file, pick routing
290305
/// defaults based on which provider the user actually has configured. This
291306
/// avoids the common pitfall where a user sets up OpenRouter (or another

0 commit comments

Comments
 (0)