From 277dc285b5d3e2fe4e0db694a7699d16d1a05234 Mon Sep 17 00:00:00 2001 From: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Date: Wed, 19 Aug 2026 12:49:54 -0700 Subject: [PATCH] fix: isolate openai provider configuration Co-authored-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> Signed-off-by: am <6e30cd56c30e030cd31bb0939b94a7c257c9a09d5ba2d92cf2735da45629f248@buzz.block.builderlab.xyz> --- crates/buzz-agent/src/config.rs | 76 ++++++++++++++- .../src-tauri/src/commands/agent_models.rs | 24 +++-- .../src/commands/personas/inbound.rs | 32 +++++- .../personas/inbound/inbound_tests.rs | 44 +++++++++ .../src-tauri/src/managed_agents/readiness.rs | 72 +++++--------- .../managed_agents/readiness_openai_tests.rs | 80 +++++++++++++++ .../src-tauri/src/managed_agents/runtime.rs | 14 +-- .../src/managed_agents/runtime/metadata.rs | 36 ++++++- .../src/migration/openai_credentials.rs | 53 +--------- .../src/migration/openai_credentials_tests.rs | 28 ++++-- desktop/src/features/agents/AGENTS.md | 11 +-- .../features/agents/ui/AgentConfigFields.tsx | 6 +- .../agents/ui/providerEnvVarUpdates.test.mjs | 97 +++---------------- .../agents/ui/providerEnvVarUpdates.ts | 64 ++---------- .../ui/runtimeModelProviderSelection.test.mjs | 27 ++++-- .../ui/runtimeModelProviderSelection.ts | 8 +- 16 files changed, 383 insertions(+), 289 deletions(-) create mode 100644 desktop/src-tauri/src/managed_agents/readiness_openai_tests.rs diff --git a/crates/buzz-agent/src/config.rs b/crates/buzz-agent/src/config.rs index bf0d99f28d..4e3ec919b5 100644 --- a/crates/buzz-agent/src/config.rs +++ b/crates/buzz-agent/src/config.rs @@ -530,10 +530,20 @@ impl Config { pub fn from_env() -> Result { let databricks_host = env("DATABRICKS_HOST"); let databricks_model = env("DATABRICKS_MODEL"); + let requested_provider = env("BUZZ_AGENT_PROVIDER"); + let openai_key = env("OPENAI_API_KEY"); + let legacy_openai_key = env("OPENAI_COMPAT_API_KEY"); + let openai_base_url = env("OPENAI_COMPAT_BASE_URL"); let provider = resolve_provider( - env("BUZZ_AGENT_PROVIDER").as_deref(), + normalize_legacy_openai_provider( + requested_provider.as_deref(), + openai_base_url.as_deref(), + openai_key.as_deref(), + legacy_openai_key.as_deref(), + ) + .as_deref(), env("ANTHROPIC_API_KEY").as_deref(), - env("OPENAI_API_KEY").as_deref(), + openai_key.as_deref().or(legacy_openai_key.as_deref()), env("OPENROUTER_API_KEY").as_deref(), )?; @@ -561,7 +571,11 @@ impl Config { OpenAiApi::Auto, // unused for Anthropic ), Provider::OpenAi => ( - req("OPENAI_API_KEY")?, + openai_key + .clone() + .or_else(|| legacy_openai_key.clone()) + .filter(|value| !value.trim().is_empty()) + .ok_or_else(|| "config: OPENAI_API_KEY required".to_string())?, resolve_model( buzz_agent_model.as_deref(), env("OPENAI_COMPAT_MODEL").as_deref(), @@ -807,6 +821,33 @@ fn present_nonempty(v: Option<&str>) -> bool { v.map(str::trim).is_some_and(|s| !s.is_empty()) } +fn normalize_legacy_openai_provider( + requested: Option<&str>, + openai_base_url: Option<&str>, + openai_key: Option<&str>, + legacy_openai_key: Option<&str>, +) -> Option { + let requested = requested?.trim(); + if !requested.eq_ignore_ascii_case("openai") { + return Some(requested.to_string()); + } + // OPENAI_API_KEY is the post-split discriminator. Only configurations that + // still rely exclusively on the legacy key are eligible for URL-based + // normalization; an explicit official key must never be routed elsewhere. + if present_nonempty(openai_key) || !present_nonempty(legacy_openai_key) { + return Some("openai".to_string()); + } + let custom_origin = openai_base_url + .map(str::trim) + .filter(|value| !value.is_empty()) + .is_some_and(|value| value.trim_end_matches('/') != "https://api.openai.com/v1"); + Some(if custom_origin { + "openai-compat".to_string() + } else { + "openai".to_string() + }) +} + fn resolve_provider( requested: Option<&str>, anthropic_key: Option<&str>, @@ -1151,6 +1192,35 @@ mod tests { assert!(err.contains("OPENAI_COMPAT_API=nope"), "{err}"); } + #[test] + fn legacy_openai_provider_normalizes_custom_origins_only_without_new_key() { + assert_eq!( + normalize_legacy_openai_provider( + Some("openai"), + Some("http://localhost:11434/v1"), + None, + Some("legacy-key"), + ) + .as_deref(), + Some("openai-compat") + ); + assert_eq!( + normalize_legacy_openai_provider( + Some("openai"), + Some("http://localhost:11434/v1"), + Some("official-key"), + Some("legacy-key"), + ) + .as_deref(), + Some("openai") + ); + assert_eq!( + normalize_legacy_openai_provider(Some("openai"), None, None, Some("legacy-key"),) + .as_deref(), + Some("openai") + ); + } + #[test] fn resolve_provider_keeps_requested_provider_when_token_present() { assert_eq!( diff --git a/desktop/src-tauri/src/commands/agent_models.rs b/desktop/src-tauri/src/commands/agent_models.rs index 0020012809..55610f12e2 100644 --- a/desktop/src-tauri/src/commands/agent_models.rs +++ b/desktop/src-tauri/src/commands/agent_models.rs @@ -8,12 +8,12 @@ use super::agent_model_process::run_agent_models_command; use super::managed_agent_definition::apply_model_provider_prompt_update; // The map-only lookup is reached solely from an Anthropic base-URL helper used // by unit tests; discovery itself always goes through the process-env variant. -#[cfg(test)] -use super::agent_models_env::env_value; use super::agent_models_env::{ - effective_discovery_provider, env_or_process_override, env_or_process_value, - openai_compatible_models_url_for_discovery, redaction_env_with_value, DiscoveryProvider, + effective_discovery_provider, env_or_process_value, openai_compatible_models_url_for_discovery, + redaction_env_with_value, DiscoveryProvider, }; +#[cfg(test)] +use super::agent_models_env::{env_or_process_override, env_value}; use super::agent_update_rollback::{rollback_failed_agent_update, AgentUpdateRollback}; use crate::{ @@ -96,11 +96,15 @@ pub async fn get_agent_models( command: _, } = discovery; - let merged_env = discovery_env_with_baked_floor(merged_env); + let mut merged_env = discovery_env_with_baked_floor(merged_env); // Resolve against the baked/process env when the record saved no provider, // so a build-provided provider still gets live discovery. let effective_provider = effective_discovery_provider(saved_provider.as_deref(), provider_env_var, &merged_env); + crate::managed_agents::readiness::canonicalize_openai_provider_env( + &mut merged_env, + effective_provider.as_deref(), + ); if let Some(models) = discover_openrouter_models( &state.http_client, &effective_provider, @@ -231,7 +235,7 @@ pub async fn discover_agent_models( &input.definition_env, &input.env_vars, ); - let merged_env = discovery_env_with_baked_floor(merged_env); + let mut merged_env = discovery_env_with_baked_floor(merged_env); // Recover a build-provided provider when the form has none, so the create // dialog discovers live models instead of falling through to the subprocess. let effective_provider = effective_discovery_provider( @@ -239,6 +243,10 @@ pub async fn discover_agent_models( runtime_meta.and_then(|meta| meta.provider_env_var), &merged_env, ); + crate::managed_agents::readiness::canonicalize_openai_provider_env( + &mut merged_env, + effective_provider.as_deref(), + ); // Buzz shared compute discovery must not depend on the local OpenAI ingress: that // client endpoint is started only after a live target is selected. @@ -495,7 +503,9 @@ async fn discover_openai_compatible_models( let api_key = if relay_mesh { crate::managed_agents::RELAY_MESH_API_KEY_PLACEHOLDER.to_string() } else if is_compat { - env_or_process_override(env, api_key_env_var).unwrap_or_default() + env.get(api_key_env_var) + .map(|value| value.trim().to_string()) + .unwrap_or_default() } else { match provider.required_env(env, api_key_env_var)? { Some(api_key) => api_key, diff --git a/desktop/src-tauri/src/commands/personas/inbound.rs b/desktop/src-tauri/src/commands/personas/inbound.rs index 5214dd5a27..30144841a1 100644 --- a/desktop/src-tauri/src/commands/personas/inbound.rs +++ b/desktop/src-tauri/src/commands/personas/inbound.rs @@ -2,6 +2,8 @@ //! projections and their NIP-09 tombstones. Extracted from the parent module to //! keep it under the file-size cap. +use std::collections::BTreeMap; + use tauri::{AppHandle, Emitter, Manager}; use crate::{ @@ -509,6 +511,32 @@ fn event_d_tag(event: &nostr::Event) -> Result { .ok_or_else(|| "inbound event missing d-tag".to_string()) } +fn normalize_inbound_openai_provider( + provider: Option, + local_env: &BTreeMap, +) -> Option { + let is_legacy_openai = provider + .as_deref() + .is_some_and(|value| value.trim().eq_ignore_ascii_case("openai")) + && local_env + .get("OPENAI_API_KEY") + .is_none_or(|value| value.trim().is_empty()) + && local_env + .get("OPENAI_COMPAT_API_KEY") + .is_some_and(|value| !value.trim().is_empty()); + let has_custom_origin = local_env + .get("OPENAI_COMPAT_BASE_URL") + .map(String::as_str) + .map(str::trim) + .filter(|value| !value.is_empty()) + .is_some_and(|value| value.trim_end_matches('/') != "https://api.openai.com/v1"); + if is_legacy_openai && has_custom_origin { + Some("openai-compat".to_string()) + } else { + provider + } +} + /// Merge a parsed inbound persona into the local set: patch the matching record /// in place, or push it when none matches. /// @@ -530,7 +558,7 @@ fn apply_inbound_persona(personas: &mut Vec, inbound: AgentDefi local.system_prompt = inbound.system_prompt; local.runtime = inbound.runtime; local.model = inbound.model; - local.provider = inbound.provider; + local.provider = normalize_inbound_openai_provider(inbound.provider, &local.env_vars); local.name_pool = inbound.name_pool; local.respond_to = inbound.respond_to; local.respond_to_allowlist = inbound.respond_to_allowlist; @@ -576,7 +604,7 @@ fn apply_inbound_managed_agent( if !definition_linked { local.system_prompt = inbound.system_prompt; local.model = inbound.model; - local.provider = inbound.provider; + local.provider = normalize_inbound_openai_provider(inbound.provider, &local.env_vars); local.persona_source_version = inbound.persona_source_version; } local.parallelism = inbound.parallelism; diff --git a/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs b/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs index fbfede3588..0c3411f680 100644 --- a/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs +++ b/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs @@ -33,6 +33,50 @@ fn local_in_app() -> AgentDefinition { } } +#[test] +fn inbound_legacy_openai_provider_keeps_local_custom_origin_compatible() { + let mut local = local_in_app(); + local.env_vars = BTreeMap::from([ + ( + "OPENAI_COMPAT_API_KEY".to_string(), + "compat-key".to_string(), + ), + ( + "OPENAI_COMPAT_BASE_URL".to_string(), + "https://gateway.example/v1".to_string(), + ), + ]); + let mut personas = vec![local]; + let mut inbound = inbound_for(UUID, "Remote"); + inbound.provider = Some("openai".to_string()); + + apply_inbound_persona(&mut personas, inbound); + + assert_eq!(personas[0].provider.as_deref(), Some("openai-compat")); +} + +#[test] +fn inbound_explicit_official_key_preserves_openai_provider() { + let mut local = local_in_app(); + local.env_vars = BTreeMap::from([ + ("OPENAI_API_KEY".to_string(), "official-key".to_string()), + ( + "OPENAI_COMPAT_API_KEY".to_string(), + "compat-key".to_string(), + ), + ( + "OPENAI_COMPAT_BASE_URL".to_string(), + "https://gateway.example/v1".to_string(), + ), + ]); + let mut personas = vec![local]; + let inbound = inbound_for(UUID, "Remote"); + + apply_inbound_persona(&mut personas, inbound); + + assert_eq!(personas[0].provider.as_deref(), Some("openai")); +} + /// An inbound persona as `persona_from_event` would produce it: id = d-tag, /// slug = Some(d-tag), empty env_vars, source_team None. fn inbound_for(d_tag: &str, display_name: &str) -> AgentDefinition { diff --git a/desktop/src-tauri/src/managed_agents/readiness.rs b/desktop/src-tauri/src/managed_agents/readiness.rs index 90b8b2e872..388b11d34f 100644 --- a/desktop/src-tauri/src/managed_agents/readiness.rs +++ b/desktop/src-tauri/src/managed_agents/readiness.rs @@ -269,6 +269,8 @@ fn resolve_effective_agent_env_with_def( ); env.extend(user_env); + canonicalize_openai_provider_env(&mut env, effective_provider.as_deref()); + // Buzz shared compute is a native Buzz provider. Translate it to buzz-agent's // OpenAI-compatible transport only in the effective runtime environment. #[cfg(feature = "mesh-llm")] @@ -285,6 +287,27 @@ fn resolve_effective_agent_env_with_def( } } +pub(crate) fn canonicalize_openai_provider_env( + env: &mut BTreeMap, + provider: Option<&str>, +) { + match provider + .map(str::trim) + .map(str::to_ascii_lowercase) + .as_deref() + { + Some("openai") => { + env.remove("OPENAI_COMPAT_BASE_URL"); + env.remove("OPENAI_COMPAT_API_KEY"); + } + Some("openai-compat") => { + env.remove("OPENAI_API_KEY"); + env.entry("OPENAI_COMPAT_API_KEY".to_string()).or_default(); + } + _ => {} + } +} + // ── Requirement types ───────────────────────────────────────────────────────── /// A single missing piece of configuration, tagged with the UI surface that /// owns it so the UI can route each gap to the right affordance. @@ -746,52 +769,6 @@ mod tests { })); } - #[test] - fn buzz_agent_missing_openai_key_returns_not_ready() { - let env = make_env( - "buzz-agent", - env_with(&[ - ("BUZZ_AGENT_PROVIDER", "openai"), - ("BUZZ_AGENT_MODEL", "gpt-4o"), - ("OPENAI_COMPAT_API_KEY", "must-not-cross-provider-boundary"), - ]), - ); - let result = agent_readiness(&env); - assert!(!result.is_ready()); - assert!(result.requirements().contains(&Requirement::EnvKey { - key: "OPENAI_API_KEY".to_string() - })); - } - - #[test] - fn buzz_agent_openai_compat_requires_url_but_not_key() { - let without_url = make_env( - "buzz-agent", - env_with(&[ - ("BUZZ_AGENT_PROVIDER", "openai-compat"), - ("BUZZ_AGENT_MODEL", "llama3"), - ]), - ); - let result = agent_readiness(&without_url); - assert!(!result.is_ready()); - assert!(result.requirements().contains(&Requirement::EnvKey { - key: "OPENAI_COMPAT_BASE_URL".to_string() - })); - assert!(!result.requirements().contains(&Requirement::EnvKey { - key: "OPENAI_COMPAT_API_KEY".to_string() - })); - - let with_url = make_env( - "buzz-agent", - env_with(&[ - ("BUZZ_AGENT_PROVIDER", " OpenAI-Compat "), - ("BUZZ_AGENT_MODEL", "llama3"), - ("OPENAI_COMPAT_BASE_URL", "http://localhost:11434/v1"), - ]), - ); - assert!(agent_readiness(&with_url).is_ready()); - } - #[test] fn buzz_agent_anthropic_with_all_fields_is_ready() { let env = make_env( @@ -1738,3 +1715,6 @@ mod tests { #[cfg(test)] #[path = "readiness_goose_file_config_tests.rs"] mod goose_file_config_tests; +#[cfg(test)] +#[path = "readiness_openai_tests.rs"] +mod openai_tests; diff --git a/desktop/src-tauri/src/managed_agents/readiness_openai_tests.rs b/desktop/src-tauri/src/managed_agents/readiness_openai_tests.rs new file mode 100644 index 0000000000..c0cbd8a7c9 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/readiness_openai_tests.rs @@ -0,0 +1,80 @@ +use super::*; + +fn make_env(command: &str, pairs: &[(&str, &str)]) -> EffectiveAgentEnv { + EffectiveAgentEnv { + env: pairs + .iter() + .map(|(key, value)| (key.to_string(), value.to_string())) + .collect(), + config_file_path: None, + effective_command: command.to_string(), + } +} + +#[test] +fn provider_env_canonicalization_isolates_consumers() { + let shared = BTreeMap::from([ + ("OPENAI_API_KEY".into(), "official".into()), + ("OPENAI_COMPAT_API_KEY".into(), "compat".into()), + ( + "OPENAI_COMPAT_BASE_URL".into(), + "http://localhost:11434/v1".into(), + ), + ]); + let mut official = shared.clone(); + canonicalize_openai_provider_env(&mut official, Some("openai")); + assert_eq!( + official.get("OPENAI_API_KEY").map(String::as_str), + Some("official") + ); + assert!(!official.contains_key("OPENAI_COMPAT_API_KEY")); + assert!(!official.contains_key("OPENAI_COMPAT_BASE_URL")); + + let mut compatible = shared; + canonicalize_openai_provider_env(&mut compatible, Some("openai-compat")); + assert!(!compatible.contains_key("OPENAI_API_KEY")); + assert_eq!( + compatible.get("OPENAI_COMPAT_API_KEY").map(String::as_str), + Some("compat") + ); +} + +#[test] +fn keyless_compatible_provider_gets_explicit_blank_credential() { + let mut env = BTreeMap::from([( + "OPENAI_COMPAT_BASE_URL".into(), + "http://localhost:11434/v1".into(), + )]); + canonicalize_openai_provider_env(&mut env, Some("openai-compat")); + assert_eq!( + env.get("OPENAI_COMPAT_API_KEY").map(String::as_str), + Some("") + ); +} + +#[test] +fn readiness_enforces_openai_provider_boundaries() { + let official = make_env( + "buzz-agent", + &[ + ("BUZZ_AGENT_PROVIDER", "openai"), + ("BUZZ_AGENT_MODEL", "gpt-4o"), + ("OPENAI_COMPAT_API_KEY", "must-not-cross-provider-boundary"), + ], + ); + assert!(agent_readiness(&official) + .requirements() + .contains(&Requirement::EnvKey { + key: "OPENAI_API_KEY".to_string(), + })); + + let keyless_compat = make_env( + "buzz-agent", + &[ + ("BUZZ_AGENT_PROVIDER", "OpenAI-Compat"), + ("BUZZ_AGENT_MODEL", "llama3"), + ("OPENAI_COMPAT_BASE_URL", "http://localhost:11434/v1"), + ], + ); + assert!(agent_readiness(&keyless_compat).is_ready()); +} diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 3a0364f694..cb764bbf7b 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -24,7 +24,7 @@ pub(crate) use super::access_policy::{build_respond_to_env_with_policy, RespondT mod metadata; pub(crate) use metadata::{ apply_agent_display_env, resolve_session_title, runtime_metadata_env_vars, - DISPLAY_NAME_ENV_VAR, SESSION_TITLE_ENV_VAR, + scrub_unconfigured_openai_compat_key, DISPLAY_NAME_ENV_VAR, SESSION_TITLE_ENV_VAR, }; mod stop; @@ -582,7 +582,6 @@ pub fn spawn_agent_child( config_file_path: runtime_meta.and_then(|r| r.config_file_path), effective_command: descriptor.command.clone(), }; - // Compute the optional payload before touching the command. let setup_payload_json = if let AgentReadiness::NotReady { requirements } = agent_readiness(&effective) { let reqs: Vec = requirements @@ -807,15 +806,16 @@ pub fn spawn_agent_child( ); } - // User env (descriptor.env): fully-layered floor→runtime→definition→global→persona→agent, - // reserved-key filtered. Written last so user-explicit values win over Buzz-set env. for (key, value) in &descriptor.env { command.env(key, value); } - // B5: carry persisted effort; harness resolves thought_level configId at first session. - // Written AFTER descriptor.env so the canonical persisted value wins over any - // user-supplied BUZZ_ACP_EFFORT_LEVEL entry, mirroring the A1 model-authority pattern + scrub_unconfigured_openai_compat_key( + &mut command, + effective_provider.as_deref(), + &descriptor.env, + ); + // (ANTHROPIC_MODEL is applied post-loop for the same reason). When effort_level is // None there is no canonical value to assert, so env passthrough stands — user env // legitimately seeds startup effort in that case. diff --git a/desktop/src-tauri/src/managed_agents/runtime/metadata.rs b/desktop/src-tauri/src/managed_agents/runtime/metadata.rs index 5aef424ea6..5cd66ffb08 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/metadata.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/metadata.rs @@ -24,6 +24,19 @@ pub(crate) fn runtime_metadata_env_vars<'a>( vars } +pub(crate) fn scrub_unconfigured_openai_compat_key( + command: &mut std::process::Command, + effective_provider: Option<&str>, + env: &std::collections::BTreeMap, +) { + if effective_provider + .is_some_and(|provider| provider.trim().eq_ignore_ascii_case("openai-compat")) + && !env.contains_key("OPENAI_COMPAT_API_KEY") + { + command.env_remove("OPENAI_COMPAT_API_KEY"); + } +} + /// Env var carrying the session title to the harness. Shared with /// `spawn_snapshot` so the restart badge records the same key the spawn writes. pub(crate) const SESSION_TITLE_ENV_VAR: &str = "BUZZ_ACP_SESSION_TITLE"; @@ -76,7 +89,28 @@ pub(crate) fn resolve_session_title(display_name: Option<&str>, name: &str) -> O #[cfg(test)] mod tests { - use super::resolve_session_title; + use super::{resolve_session_title, scrub_unconfigured_openai_compat_key}; + use std::collections::BTreeMap; + + #[test] + fn keyless_openai_compat_scrubs_parent_process_credential() { + let mut command = std::process::Command::new("echo"); + command.env("OPENAI_COMPAT_API_KEY", "ambient-secret"); + scrub_unconfigured_openai_compat_key(&mut command, Some("openai-compat"), &BTreeMap::new()); + assert!(command + .get_envs() + .any(|(key, value)| key == "OPENAI_COMPAT_API_KEY" && value.is_none())); + } + + #[test] + fn configured_openai_compat_key_is_preserved() { + let mut command = std::process::Command::new("echo"); + let env = BTreeMap::from([("OPENAI_COMPAT_API_KEY".to_string(), "key".to_string())]); + scrub_unconfigured_openai_compat_key(&mut command, Some("openai-compat"), &env); + assert!(!command + .get_envs() + .any(|(key, value)| key == "OPENAI_COMPAT_API_KEY" && value.is_none())); + } #[test] fn resolve_session_title_prefers_display_name() { diff --git a/desktop/src-tauri/src/migration/openai_credentials.rs b/desktop/src-tauri/src/migration/openai_credentials.rs index 89b0601b7d..e26b814f0d 100644 --- a/desktop/src-tauri/src/migration/openai_credentials.rs +++ b/desktop/src-tauri/src/migration/openai_credentials.rs @@ -212,7 +212,7 @@ fn classify_endpoint(endpoint: Option<&str>) -> Result { let Some(value) = endpoint.map(str::trim).filter(|v| !v.is_empty()) else { return Ok(Identity::Official); }; - let url = url::Url::parse(value).map_err(|_| format!("invalid OpenAI endpoint {value:?}"))?; + let url = url::Url::parse(value).map_err(|_| "invalid OpenAI endpoint".to_string())?; if !matches!(url.scheme(), "http" | "https") || url.host_str().is_none() || !url.username().is_empty() @@ -220,7 +220,7 @@ fn classify_endpoint(endpoint: Option<&str>) -> Result { || url.query().is_some() || url.fragment().is_some() { - return Err(format!("invalid OpenAI endpoint {value:?}")); + return Err("invalid OpenAI endpoint".to_string()); } let official = url.scheme() == "https" && url @@ -399,23 +399,6 @@ fn set_provider(store: &mut Store, owner: Owner, provider: &str) -> Result<(), S fn plan_store(mut store: Store) -> Result<(Store, Vec), String> { let consumers = collect_consumers(&store)?; - let all_env_consumers: Vec<_> = store - .records - .iter() - .enumerate() - .map(|(index, record)| { - let defs = definitions(&store); - let def = selected_definition(record, &defs); - layers(&store, record, def).map(|layers| { - ( - index, - resolve_key(&layers, OPENAI_COMPAT_API_KEY), - resolve_key(&layers, OPENAI_API_KEY), - resolve_key(&layers, OPENAI_COMPAT_BASE_URL), - ) - }) - }) - .collect::>()?; let mut provider_targets: HashMap = HashMap::new(); for consumer in &consumers { let expected = if consumer.identity == Identity::Official { @@ -476,26 +459,6 @@ fn plan_store(mut store: Store) -> Result<(Store, Vec), String> { } for (owner, identity) in legacy_targets { if identity == Identity::Official { - for (index, legacy, official, _) in &all_env_consumers { - if legacy.as_ref().is_some_and(|value| value.owner == owner) { - let relevant = consumers - .iter() - .find(|consumer| consumer.record_index == Some(*index)); - if relevant.is_none_or(|consumer| consumer.identity != Identity::Official) { - return Err(format!( - "credential owner {owner:?} is shared with a non-official consumer" - )); - } - if official - .as_ref() - .is_some_and(|value| !value.value.is_empty()) - { - return Err(format!( - "credential owner {owner:?} is also visible beside an existing distinct official credential" - )); - } - } - } let env = env_mut(&mut store, owner)?; if env.get(OPENAI_API_KEY).is_none_or(|value| value.is_empty()) { if let Some(value) = env.remove(OPENAI_COMPAT_API_KEY) { @@ -523,18 +486,6 @@ fn plan_store(mut store: Store) -> Result<(Store, Vec), String> { alias_owners.sort_by_key(|owner| format!("{owner:?}")); alias_owners.dedup(); for owner in alias_owners { - for (index, _, _, endpoint) in &all_env_consumers { - if endpoint.as_ref().is_some_and(|value| value.owner == owner) { - let relevant = consumers - .iter() - .find(|consumer| consumer.record_index == Some(*index)); - if relevant.is_none_or(|consumer| consumer.identity != Identity::Official) { - return Err(format!( - "endpoint owner {owner:?} is shared with a non-official consumer" - )); - } - } - } env_mut(&mut store, owner)?.insert( OPENAI_COMPAT_BASE_URL.into(), CANONICAL_OPENAI_ORIGIN.into(), diff --git a/desktop/src-tauri/src/migration/openai_credentials_tests.rs b/desktop/src-tauri/src/migration/openai_credentials_tests.rs index d6d4bc7e17..0c45d46b71 100644 --- a/desktop/src-tauri/src/migration/openai_credentials_tests.rs +++ b/desktop/src-tauri/src/migration/openai_credentials_tests.rs @@ -196,7 +196,7 @@ fn mixed_global_provider_owner_is_rejected() { } #[test] -fn mixed_global_credential_owner_is_rejected_even_for_unrelated_consumer() { +fn unrelated_consumer_does_not_claim_global_openai_credential() { let global = GlobalAgentConfig { provider: Some("openai".into()), env_vars: BTreeMap::from([(OPENAI_COMPAT_API_KEY.into(), "shared".into())]), @@ -205,9 +205,15 @@ fn mixed_global_credential_owner_is_rejected_even_for_unrelated_consumer() { let official = record("official", "official"); let mut unrelated = record("unrelated", "unrelated"); unrelated.provider = Some("anthropic".into()); - assert!(planned(global, vec![official, unrelated]) - .unwrap_err() - .contains("shared with a non-official consumer")); + let store = planned(global, vec![official, unrelated]).unwrap(); + assert_eq!( + store + .global + .env_vars + .get(OPENAI_API_KEY) + .map(String::as_str), + Some("shared") + ); } #[test] @@ -274,7 +280,7 @@ fn first_fold_builtin_link_does_not_block_unrelated_official_consumer() { } #[test] -fn shared_global_official_alias_with_non_openai_consumer_is_rejected() { +fn unrelated_consumer_does_not_claim_global_openai_endpoint() { let global = GlobalAgentConfig { provider: Some("openai".into()), env_vars: BTreeMap::from([( @@ -286,9 +292,15 @@ fn shared_global_official_alias_with_non_openai_consumer_is_rejected() { let official = record("official", "official"); let mut unrelated = record("unrelated", "unrelated"); unrelated.provider = Some("anthropic".into()); - assert!(planned(global, vec![official, unrelated]) - .unwrap_err() - .contains("endpoint owner")); + let store = planned(global, vec![official, unrelated]).unwrap(); + assert_eq!( + store + .global + .env_vars + .get(OPENAI_COMPAT_BASE_URL) + .map(String::as_str), + Some(CANONICAL_OPENAI_ORIGIN) + ); } #[test] diff --git a/desktop/src/features/agents/AGENTS.md b/desktop/src/features/agents/AGENTS.md index 9bbc036cae..35ba187a41 100644 --- a/desktop/src/features/agents/AGENTS.md +++ b/desktop/src/features/agents/AGENTS.md @@ -107,12 +107,11 @@ with a TypeScript lookup table or an id comparison in a component. starts on the Buzz Agent-first deployment fallback and carries that visible harness into the next saved edit. Provider credentials have distinct identities: official OpenAI uses `OPENAI_API_KEY`, while arbitrary - OpenAI-compatible origins use `OPENAI_COMPAT_API_KEY`. Provider changes - apply the shared managed-key clearing policy so one provider's credential is - never relabeled as the other's. `OPENAI_COMPAT_BASE_URL` is provider-owned - routing state and must be cleared whenever a defaults/onboarding transition - leaves `openai-compat`; otherwise official OpenAI discovery inherits the - stale custom endpoint. Persisted pre-split credentials are migrated once at + OpenAI-compatible origins use `OPENAI_COMPAT_API_KEY`. Provider changes preserve the shared env map; provider-specific consumers + select only their own credential and routing values. Official OpenAI pins its + origin and ignores compatible routing state, so changing the default never + destroys credentials or endpoints still used by explicitly configured agents + or card minting. Persisted pre-split credentials are migrated once at boot: official `api.openai.com` records move to `OPENAI_API_KEY`, while custom origins remain `openai-compat` and retain `OPENAI_COMPAT_API_KEY`. The `progressive-defaults` disclosure diff --git a/desktop/src/features/agents/ui/AgentConfigFields.tsx b/desktop/src/features/agents/ui/AgentConfigFields.tsx index 36136654b4..d97d2a7041 100644 --- a/desktop/src/features/agents/ui/AgentConfigFields.tsx +++ b/desktop/src/features/agents/ui/AgentConfigFields.tsx @@ -67,7 +67,7 @@ import { SettingsOptionGroup } from "@/features/settings/ui/SettingsOptionGroup" import { AdvancedRequiredBadge } from "./AdvancedRequiredBadge"; import { CardMintKeyCue } from "./CardMintKeyCue"; import { getGlobalAgentCredentialState } from "./globalAgentCredentialState"; -import { envVarsClearingManagedApiKey } from "./providerEnvVarUpdates"; +import { envVarsPreservingProviderState } from "./providerEnvVarUpdates"; export const EMPTY_GLOBAL_CONFIG: GlobalAgentConfig = { env_vars: {}, @@ -523,7 +523,7 @@ export function AgentConfigFields({ function handleProviderChange(value: string) { userEditedProviderRef.current = true; if (value === CUSTOM_PROVIDER_DROPDOWN_VALUE) { - const nextEnvVars = envVarsClearingManagedApiKey( + const nextEnvVars = envVarsPreservingProviderState( config.env_vars, effectiveProvider, "", @@ -534,7 +534,7 @@ export function AgentConfigFields({ } const nextProvider = value === AUTO_PROVIDER_DROPDOWN_VALUE || value === "" ? null : value; - const nextEnvVars = envVarsClearingManagedApiKey( + const nextEnvVars = envVarsPreservingProviderState( config.env_vars, effectiveProvider, nextProvider ?? bakedProvider ?? "", diff --git a/desktop/src/features/agents/ui/providerEnvVarUpdates.test.mjs b/desktop/src/features/agents/ui/providerEnvVarUpdates.test.mjs index a91d190ba3..7efee82ef2 100644 --- a/desktop/src/features/agents/ui/providerEnvVarUpdates.test.mjs +++ b/desktop/src/features/agents/ui/providerEnvVarUpdates.test.mjs @@ -1,95 +1,26 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { - envVarsClearingManagedApiKey, - envVarsClearingOpenAiCompatBaseUrl, - envVarsWithoutKey, -} from "./providerEnvVarUpdates.ts"; +import { envVarsPreservingProviderState } from "./providerEnvVarUpdates.ts"; -test("envVarsWithoutKey removes a present key", () => { - assert.deepEqual(envVarsWithoutKey({ A: "1", B: "2" }, "A"), { B: "2" }); -}); - -test("envVarsWithoutKey returns the same reference when the key is absent", () => { - const current = { A: "1" }; - assert.equal(envVarsWithoutKey(current, "B"), current); -}); - -test("envVarsClearingManagedApiKey clears the previous provider's key on switch", () => { - const next = envVarsClearingManagedApiKey( - { ANTHROPIC_API_KEY: "sk-1", KEEP: "x" }, - "anthropic", - "openai", - ); - assert.deepEqual(next, { KEEP: "x" }); -}); - -test("envVarsClearingManagedApiKey clears when leaving to a custom/empty provider", () => { - // The dialogs' CUSTOM-provider paths delete unconditionally; empty next - // provider has no managed key, so the inequality always holds — same rule. - const next = envVarsClearingManagedApiKey( - { ANTHROPIC_API_KEY: "sk-1" }, - "anthropic", - "", - ); - assert.deepEqual(next, {}); -}); - -test("envVarsClearingOpenAiCompatBaseUrl clears only the compatible URL", () => { +test("provider changes preserve shared credentials and routing state", () => { const current = { - OPENAI_COMPAT_API_KEY: "sk-1", + ANTHROPIC_API_KEY: "anthropic-key", + OPENAI_API_KEY: "openai-key", + OPENAI_COMPAT_API_KEY: "compat-key", OPENAI_COMPAT_BASE_URL: "http://localhost:11434/v1", - KEEP: "x", }; - assert.deepEqual( - envVarsClearingOpenAiCompatBaseUrl(current, " OpenAI-Compat ", "openai"), - { - OPENAI_COMPAT_API_KEY: "sk-1", - KEEP: "x", - }, + + assert.equal( + envVarsPreservingProviderState(current, "openai-compat", "openai"), + current, ); assert.equal( - envVarsClearingOpenAiCompatBaseUrl( - current, - "openai-compat", - "OPENAI-COMPAT", - ), + envVarsPreservingProviderState(current, "openai", "anthropic"), + current, + ); + assert.equal( + envVarsPreservingProviderState(current, "anthropic", ""), current, ); }); - -test("envVarsClearingManagedApiKey clears the compat credential and URL when switching to OpenAI", () => { - const next = envVarsClearingManagedApiKey( - { - OPENAI_COMPAT_API_KEY: "compat-key", - OPENAI_COMPAT_BASE_URL: "http://localhost:11434/v1", - KEEP: "x", - }, - "openai-compat", - "openai", - ); - assert.deepEqual(next, { KEEP: "x" }); -}); - -test("envVarsClearingManagedApiKey clears the OpenAI credential when switching to compat", () => { - const next = envVarsClearingManagedApiKey( - { OPENAI_API_KEY: "openai-key", KEEP: "x" }, - "openai", - "openai-compat", - ); - assert.deepEqual(next, { KEEP: "x" }); -}); - -test("envVarsClearingManagedApiKey is a no-op when the provider is unchanged or unmanaged", () => { - const current = { ANTHROPIC_API_KEY: "sk-1" }; - assert.equal( - envVarsClearingManagedApiKey(current, "anthropic", "anthropic"), - current, - ); - const noManaged = { X: "1" }; - assert.equal( - envVarsClearingManagedApiKey(noManaged, "", "openai"), - noManaged, - ); -}); diff --git a/desktop/src/features/agents/ui/providerEnvVarUpdates.ts b/desktop/src/features/agents/ui/providerEnvVarUpdates.ts index be2dc7e81f..5ac9e23ca0 100644 --- a/desktop/src/features/agents/ui/providerEnvVarUpdates.ts +++ b/desktop/src/features/agents/ui/providerEnvVarUpdates.ts @@ -1,66 +1,14 @@ import type { EnvVarsValue } from "./EnvVarsEditor"; -import { getProviderApiKeyEnvVar } from "./agentConfigOptions"; - -const OPENAI_COMPAT_BASE_URL = "OPENAI_COMPAT_BASE_URL"; - -function normalizedProvider(provider: string): string { - return provider.trim().toLowerCase(); -} /** - * Pure env-var update helpers shared by the persona / create-agent / - * edit-agent dialogs. Every function returns the SAME reference when nothing - * changes, so `setEnvVars(fn(current))` skips a no-op re-render. + * Provider selection changes which values a consumer reads. It must not delete + * values from the shared env map: global credentials and endpoints may still + * belong to another explicitly configured agent or to card minting. */ - -/** Remove `envKey` when present. */ -export function envVarsWithoutKey( +export function envVarsPreservingProviderState( current: EnvVarsValue, - envKey: string, + _previousProvider: string, + _nextProvider: string, ): EnvVarsValue { - if (!(envKey in current)) { - return current; - } - - const next = { ...current }; - delete next[envKey]; - return next; -} - -/** Remove the compatible endpoint when switching away from openai-compat. */ -export function envVarsClearingOpenAiCompatBaseUrl( - current: EnvVarsValue, - previousProvider: string, - nextProvider: string, -): EnvVarsValue { - if ( - normalizedProvider(previousProvider) === "openai-compat" && - normalizedProvider(nextProvider) !== "openai-compat" - ) { - return envVarsWithoutKey(current, OPENAI_COMPAT_BASE_URL); - } return current; } - -/** - * Clear the previous provider's managed API key and provider-owned endpoint - * when switching providers. - */ -export function envVarsClearingManagedApiKey( - current: EnvVarsValue, - previousProvider: string, - nextProvider: string, -): EnvVarsValue { - const previousEnvVar = getProviderApiKeyEnvVar(previousProvider); - const nextEnvVar = getProviderApiKeyEnvVar(nextProvider); - let next = current; - if (previousEnvVar && previousEnvVar !== nextEnvVar) { - next = envVarsWithoutKey(next, previousEnvVar); - } - next = envVarsClearingOpenAiCompatBaseUrl( - next, - previousProvider, - nextProvider, - ); - return next; -} diff --git a/desktop/src/features/agents/ui/runtimeModelProviderSelection.test.mjs b/desktop/src/features/agents/ui/runtimeModelProviderSelection.test.mjs index 3ddfe115b5..4346ae11b0 100644 --- a/desktop/src/features/agents/ui/runtimeModelProviderSelection.test.mjs +++ b/desktop/src/features/agents/ui/runtimeModelProviderSelection.test.mjs @@ -17,7 +17,7 @@ const base = { // --- selectionOnRuntimeChange --- -test("runtime change to a provider-locked runtime, full reset (Persona/Edit): clears provider, custom flags, and managed API key", () => { +test("runtime change to a provider-locked runtime, full reset (Persona/Edit): clears provider and custom flags but preserves env", () => { const next = selectionOnRuntimeChange( { ...base, @@ -37,7 +37,7 @@ test("runtime change to a provider-locked runtime, full reset (Persona/Edit): cl assert.equal(next.provider, ""); assert.equal(next.isCustomProviderEditing, false); assert.equal(next.isCustomModelEditing, false); - assert.deepEqual(next.envVars, { KEEP: "x" }); + assert.deepEqual(next.envVars, { ANTHROPIC_API_KEY: "sk-1", KEEP: "x" }); }); test("runtime change to a provider-locked runtime, provider-only reset (Create): keeps env vars and custom-model flag", () => { @@ -79,7 +79,7 @@ test("runtime change between provider-selection runtimes keeps provider state", // --- selectionOnProviderDropdownChange --- -test("provider switch clears the previous managed API key and sets the provider", () => { +test("provider switch preserves shared env and sets the provider", () => { const next = selectionOnProviderDropdownChange( { ...base, @@ -94,10 +94,10 @@ test("provider switch clears the previous managed API key and sets the provider" ); assert.equal(next.provider, "openai"); assert.equal(next.isCustomProviderEditing, false); - assert.deepEqual(next.envVars, { KEEP: "x" }); + assert.deepEqual(next.envVars, { ANTHROPIC_API_KEY: "sk-1", KEEP: "x" }); }); -test("provider switch between OpenAI origins clears the previous credential identity", () => { +test("provider switch between OpenAI origins preserves both credential identities", () => { const official = selectionOnProviderDropdownChange( { ...base, @@ -114,7 +114,11 @@ test("provider switch between OpenAI origins clears the previous credential iden clearModelWhenApiKeyMissing: false, }, ); - assert.deepEqual(official.envVars, { KEEP: "x" }); + assert.deepEqual(official.envVars, { + OPENAI_COMPAT_API_KEY: "compat-key", + OPENAI_COMPAT_BASE_URL: "https://gateway.example/v1", + KEEP: "x", + }); const compatible = selectionOnProviderDropdownChange( { @@ -128,10 +132,13 @@ test("provider switch between OpenAI origins clears the previous credential iden clearModelWhenApiKeyMissing: false, }, ); - assert.deepEqual(compatible.envVars, { KEEP: "x" }); + assert.deepEqual(compatible.envVars, { + OPENAI_API_KEY: "official-key", + KEEP: "x", + }); }); -test("custom-provider entry clears the managed key and enters custom editing", () => { +test("custom-provider entry preserves env and enters custom editing", () => { const next = selectionOnProviderDropdownChange( { ...base, @@ -146,7 +153,7 @@ test("custom-provider entry clears the managed key and enters custom editing", ( ); assert.equal(next.isCustomProviderEditing, true); assert.equal(next.provider, ""); - assert.deepEqual(next.envVars, {}); + assert.deepEqual(next.envVars, { ANTHROPIC_API_KEY: "sk-1" }); }); test("auto-provider selection maps to empty provider", () => { @@ -159,7 +166,7 @@ test("auto-provider selection maps to empty provider", () => { }, ); assert.equal(next.provider, ""); - assert.deepEqual(next.envVars, {}); + assert.deepEqual(next.envVars, { ANTHROPIC_API_KEY: "sk-1" }); }); test("Persona mode clears the model when the new provider's API key is missing", () => { diff --git a/desktop/src/features/agents/ui/runtimeModelProviderSelection.ts b/desktop/src/features/agents/ui/runtimeModelProviderSelection.ts index 89fdd18088..2c0bf709f4 100644 --- a/desktop/src/features/agents/ui/runtimeModelProviderSelection.ts +++ b/desktop/src/features/agents/ui/runtimeModelProviderSelection.ts @@ -8,7 +8,7 @@ import { shouldClearKnownModelForSelectionScope, } from "./agentConfigOptions"; import { shouldClearModelForRuntimeChange } from "./personaRuntimeModel"; -import { envVarsClearingManagedApiKey } from "./providerEnvVarUpdates"; +import { envVarsPreservingProviderState } from "./providerEnvVarUpdates"; /** * Pure transition functions for the runtime -> LLM provider -> model dropdown @@ -59,7 +59,7 @@ export function selectionOnRuntimeChange( if (!params.nextRuntimeCanChooseProvider) { if (params.lockedRuntimeReset === "full") { - next.envVars = envVarsClearingManagedApiKey( + next.envVars = envVarsPreservingProviderState( next.envVars, current.provider, "", @@ -89,7 +89,7 @@ export function selectionOnProviderDropdownChange( const next = { ...current }; if (params.nextValue === CUSTOM_PROVIDER_DROPDOWN_VALUE) { - next.envVars = envVarsClearingManagedApiKey( + next.envVars = envVarsPreservingProviderState( next.envVars, current.provider, "", @@ -101,7 +101,7 @@ export function selectionOnProviderDropdownChange( const nextProvider = params.nextValue === AUTO_PROVIDER_DROPDOWN_VALUE ? "" : params.nextValue; - next.envVars = envVarsClearingManagedApiKey( + next.envVars = envVarsPreservingProviderState( next.envVars, current.provider, nextProvider,