From 68502a7041008f65b59a575e1af13dc58cfff74c Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Tue, 6 Oct 2026 20:30:41 +0200 Subject: [PATCH 1/2] feat(config, llm): Read an API key from a list of env vars `api_key_env` accepts a list of environment variables. The list is one key: JP reads the variables in order and uses the first one holding a non-empty value. A blank value falls through to the next variable, as it already falls through between `auth` chain entries. ```toml [providers.llm.vllm] api_key_env = ["WORK_KEY", "USER_KEY"] ``` A named key in the map form may be a list too. `api_key:work` then tries each of its variables before the `auth` chain moves on to the next entry, and still reports the credential as `api_key:work`: ```toml api_key_env = { work = ["WORK_KEY", "WORK_KEY_OLD"], personal = "KEY" } ``` When nothing in a list is set, the error names every variable tried (`Missing environment variable: WORK_KEY or USER_KEY`). A named key with an empty list is reported by its name rather than as a missing variable with no name. `--cfg` accepts both shapes in JSON form, e.g. `--cfg 'providers.llm.vllm.api_key_env:=["A","B"]'`, where it rejected arrays before. `jp provider ls` lists one `api_key` row per variable, in the order they are tried, so it shows which one is set. Applies to every provider with an `api_key_env`: Anthropic, Cerebras, DeepSeek, Google, OpenAI, OpenRouter, and vLLM. Existing string and map configs are unchanged. Signed-off-by: Jean Mertz --- crates/jp_cli/src/cmd/provider.rs | 17 +- .../jp_config/src/providers/llm/anthropic.rs | 12 +- .../jp_config/src/providers/llm/cerebras.rs | 12 +- .../jp_config/src/providers/llm/deepseek.rs | 12 +- crates/jp_config/src/providers/llm/google.rs | 12 +- crates/jp_config/src/providers/llm/openai.rs | 12 +- .../jp_config/src/providers/llm/openrouter.rs | 12 +- crates/jp_config/src/providers/llm/vllm.rs | 12 +- crates/jp_config/src/providers/llm_tests.rs | 30 +++ ...onfig__tests__app_config_schema_shape.snap | 21 +- crates/jp_config/src/types/api_key_env.rs | 157 +++++++++++--- .../jp_config/src/types/api_key_env_tests.rs | 196 ++++++++++++++++-- .../jp_llm/src/provider/anthropic/resolve.rs | 11 +- crates/jp_llm/src/provider/api_key_chain.rs | 24 ++- .../src/provider/api_key_chain_tests.rs | 61 +++++- .../provider/openai/auth_rejected_tests.rs | 4 +- crates/jp_llm/src/provider/openai/resolve.rs | 11 +- .../src/provider/openai/resolve_tests.rs | 4 +- 18 files changed, 535 insertions(+), 85 deletions(-) diff --git a/crates/jp_cli/src/cmd/provider.rs b/crates/jp_cli/src/cmd/provider.rs index ab563a549..cf513a661 100644 --- a/crates/jp_cli/src/cmd/provider.rs +++ b/crates/jp_cli/src/cmd/provider.rs @@ -911,9 +911,22 @@ fn read_api_keys() -> Result, crate::Error> { match env { // Named for the chain entry that selects it. ApiKeyEnv::One(variable) => vec![(target, "api_key".to_owned(), variable)], - ApiKeyEnv::Many(variables) => variables + + // One key read from several places: a row per variable, in + // the order they are tried, so the table shows which is set. + ApiKeyEnv::FirstOf(variables) => variables + .into_iter() + .map(|variable| (target.clone(), "api_key".to_owned(), variable)) + .collect(), + ApiKeyEnv::Many(keys) => keys .into_iter() - .map(|(name, variable)| (target.clone(), name, variable)) + .flat_map(|(name, variables)| { + variables + .as_slice() + .iter() + .map(|variable| (target.clone(), name.clone(), variable.clone())) + .collect::>() + }) .collect(), } }) diff --git a/crates/jp_config/src/providers/llm/anthropic.rs b/crates/jp_config/src/providers/llm/anthropic.rs index 99781dae3..81d31a803 100644 --- a/crates/jp_config/src/providers/llm/anthropic.rs +++ b/crates/jp_config/src/providers/llm/anthropic.rs @@ -107,11 +107,19 @@ pub struct AnthropicConfig { /// Environment variable that contains the API key. /// /// A map names several keys, each selectable from the `auth` chain as - /// `api_key:`: + /// `api_key:`. + /// Each key is a variable, or a list read the same way as below: /// /// ```toml /// api_key_env = { work = "WORK_ANTHROPIC_KEY", personal = "MY_ANTHROPIC_KEY" } /// ``` + /// + /// A list is one key, read from the first variable that holds a non-empty + /// value: + /// + /// ```toml + /// api_key_env = ["WORK_ANTHROPIC_KEY", "MY_ANTHROPIC_KEY"] + /// ``` #[setting(default = "ANTHROPIC_API_KEY")] pub api_key_env: ApiKeyEnv, @@ -157,7 +165,7 @@ impl AssignKeyValue for PartialAnthropicConfig { fn assign(&mut self, mut kv: KvAssignment) -> AssignResult { match kv.key_string().as_str() { "" => kv.try_merge_object(self)?, - "api_key_env" => self.api_key_env = kv.try_some_object_or_from_str()?, + "api_key_env" => self.api_key_env = kv.try_some_value()?, "base_url" => self.base_url = kv.try_some_string()?, "subscription_flow" => self.subscription_flow = kv.try_some_object_or_from_str()?, "acp_config_dirs" => { diff --git a/crates/jp_config/src/providers/llm/cerebras.rs b/crates/jp_config/src/providers/llm/cerebras.rs index 8bde5bcc1..1590c131b 100644 --- a/crates/jp_config/src/providers/llm/cerebras.rs +++ b/crates/jp_config/src/providers/llm/cerebras.rs @@ -72,11 +72,19 @@ pub struct CerebrasConfig { /// Environment variable that contains the API key. /// /// A map names several keys, each selectable from the `auth` chain as - /// `api_key:`: + /// `api_key:`. + /// Each key is a variable, or a list read the same way as below: /// /// ```toml /// api_key_env = { work = "WORK_CEREBRAS_KEY", personal = "MY_CEREBRAS_KEY" } /// ``` + /// + /// A list is one key, read from the first variable that holds a non-empty + /// value: + /// + /// ```toml + /// api_key_env = ["WORK_CEREBRAS_KEY", "MY_CEREBRAS_KEY"] + /// ``` #[setting(default = "CEREBRAS_API_KEY")] pub api_key_env: ApiKeyEnv, @@ -105,7 +113,7 @@ impl AssignKeyValue for PartialCerebrasConfig { value => Err(format!("expected a string, got {value}").into()), })?; } - "api_key_env" => self.api_key_env = kv.try_some_object_or_from_str()?, + "api_key_env" => self.api_key_env = kv.try_some_value()?, "base_url" => self.base_url = kv.try_some_string()?, _ => return missing_key(&kv), } diff --git a/crates/jp_config/src/providers/llm/deepseek.rs b/crates/jp_config/src/providers/llm/deepseek.rs index c48d8bc5a..c05533b98 100644 --- a/crates/jp_config/src/providers/llm/deepseek.rs +++ b/crates/jp_config/src/providers/llm/deepseek.rs @@ -48,11 +48,19 @@ pub struct DeepseekConfig { /// Environment variable that contains the API key. /// /// A map names several keys, each selectable from the `auth` chain as - /// `api_key:`: + /// `api_key:`. + /// Each key is a variable, or a list read the same way as below: /// /// ```toml /// api_key_env = { work = "WORK_DEEPSEEK_KEY", personal = "MY_DEEPSEEK_KEY" } /// ``` + /// + /// A list is one key, read from the first variable that holds a non-empty + /// value: + /// + /// ```toml + /// api_key_env = ["WORK_DEEPSEEK_KEY", "MY_DEEPSEEK_KEY"] + /// ``` #[setting(default = "DEEPSEEK_API_KEY")] pub api_key_env: ApiKeyEnv, @@ -81,7 +89,7 @@ impl AssignKeyValue for PartialDeepseekConfig { value => Err(format!("expected a string, got {value}").into()), })?; } - "api_key_env" => self.api_key_env = kv.try_some_object_or_from_str()?, + "api_key_env" => self.api_key_env = kv.try_some_value()?, "base_url" => self.base_url = kv.try_some_string()?, _ => return missing_key(&kv), } diff --git a/crates/jp_config/src/providers/llm/google.rs b/crates/jp_config/src/providers/llm/google.rs index 2000286fc..2deef9b59 100644 --- a/crates/jp_config/src/providers/llm/google.rs +++ b/crates/jp_config/src/providers/llm/google.rs @@ -48,11 +48,19 @@ pub struct GoogleConfig { /// Environment variable that contains the API key. /// /// A map names several keys, each selectable from the `auth` chain as - /// `api_key:`: + /// `api_key:`. + /// Each key is a variable, or a list read the same way as below: /// /// ```toml /// api_key_env = { work = "WORK_GEMINI_KEY", personal = "MY_GEMINI_KEY" } /// ``` + /// + /// A list is one key, read from the first variable that holds a non-empty + /// value: + /// + /// ```toml + /// api_key_env = ["WORK_GEMINI_KEY", "MY_GEMINI_KEY"] + /// ``` #[setting(default = "GEMINI_API_KEY")] pub api_key_env: ApiKeyEnv, @@ -81,7 +89,7 @@ impl AssignKeyValue for PartialGoogleConfig { value => Err(format!("expected a string, got {value}").into()), })?; } - "api_key_env" => self.api_key_env = kv.try_some_object_or_from_str()?, + "api_key_env" => self.api_key_env = kv.try_some_value()?, "base_url" => self.base_url = kv.try_some_string()?, _ => return missing_key(&kv), } diff --git a/crates/jp_config/src/providers/llm/openai.rs b/crates/jp_config/src/providers/llm/openai.rs index 5001e8435..2a97182de 100644 --- a/crates/jp_config/src/providers/llm/openai.rs +++ b/crates/jp_config/src/providers/llm/openai.rs @@ -68,11 +68,19 @@ pub struct OpenaiConfig { /// Environment variable that contains the API key. /// /// A map names several keys, each selectable from the `auth` chain as - /// `api_key:`: + /// `api_key:`. + /// Each key is a variable, or a list read the same way as below: /// /// ```toml /// api_key_env = { work = "WORK_OPENAI_KEY", personal = "MY_OPENAI_KEY" } /// ``` + /// + /// A list is one key, read from the first variable that holds a non-empty + /// value: + /// + /// ```toml + /// api_key_env = ["WORK_OPENAI_KEY", "MY_OPENAI_KEY"] + /// ``` #[setting(default = "OPENAI_API_KEY")] pub api_key_env: ApiKeyEnv, @@ -128,7 +136,7 @@ impl AssignKeyValue for PartialOpenaiConfig { value => Err(format!("expected a string, got {value}").into()), })?; } - "api_key_env" => self.api_key_env = kv.try_some_object_or_from_str()?, + "api_key_env" => self.api_key_env = kv.try_some_value()?, "base_url" => self.base_url = kv.try_some_string()?, "base_url_env" => self.base_url_env = kv.try_some_string()?, "codex_base_url" => self.codex_base_url = kv.try_some_string()?, diff --git a/crates/jp_config/src/providers/llm/openrouter.rs b/crates/jp_config/src/providers/llm/openrouter.rs index 097f40503..108c26c24 100644 --- a/crates/jp_config/src/providers/llm/openrouter.rs +++ b/crates/jp_config/src/providers/llm/openrouter.rs @@ -48,11 +48,19 @@ pub struct OpenrouterConfig { /// Environment variable that contains the API key. /// /// A map names several keys, each selectable from the `auth` chain as - /// `api_key:`: + /// `api_key:`. + /// Each key is a variable, or a list read the same way as below: /// /// ```toml /// api_key_env = { work = "WORK_OPENROUTER_KEY", personal = "MY_OPENROUTER_KEY" } /// ``` + /// + /// A list is one key, read from the first variable that holds a non-empty + /// value: + /// + /// ```toml + /// api_key_env = ["WORK_OPENROUTER_KEY", "MY_OPENROUTER_KEY"] + /// ``` #[setting(default = "OPENROUTER_API_KEY")] pub api_key_env: ApiKeyEnv, @@ -90,7 +98,7 @@ impl AssignKeyValue for PartialOpenrouterConfig { value => Err(format!("expected a string, got {value}").into()), })?; } - "api_key_env" => self.api_key_env = kv.try_some_object_or_from_str()?, + "api_key_env" => self.api_key_env = kv.try_some_value()?, "app_name" => self.app_name = kv.try_some_string()?, "app_referrer" => self.app_referrer = kv.try_some_string()?, "base_url" => self.base_url = kv.try_some_string()?, diff --git a/crates/jp_config/src/providers/llm/vllm.rs b/crates/jp_config/src/providers/llm/vllm.rs index 05a7a5654..36db5fadc 100644 --- a/crates/jp_config/src/providers/llm/vllm.rs +++ b/crates/jp_config/src/providers/llm/vllm.rs @@ -57,11 +57,19 @@ pub struct VllmConfig { /// Environment variable that contains the API key. /// /// A map names several keys, each selectable from the `auth` chain as - /// `api_key:`: + /// `api_key:`. + /// Each key is a variable, or a list read the same way as below: /// /// ```toml /// api_key_env = { work = "WORK_VLLM_KEY", personal = "MY_VLLM_KEY" } /// ``` + /// + /// A list is one key, read from the first variable that holds a non-empty + /// value: + /// + /// ```toml + /// api_key_env = ["WORK_VLLM_KEY", "MY_VLLM_KEY"] + /// ``` #[setting(default = "VLLM_API_KEY")] pub api_key_env: ApiKeyEnv, @@ -93,7 +101,7 @@ impl AssignKeyValue for PartialVllmConfig { value => Err(format!("expected a string, got {value}").into()), })?; } - "api_key_env" => self.api_key_env = kv.try_some_object_or_from_str()?, + "api_key_env" => self.api_key_env = kv.try_some_value()?, "base_url" => self.base_url = kv.try_some_string()?, _ => return missing_key(&kv), } diff --git a/crates/jp_config/src/providers/llm_tests.rs b/crates/jp_config/src/providers/llm_tests.rs index 511950340..ee7bbf514 100644 --- a/crates/jp_config/src/providers/llm_tests.rs +++ b/crates/jp_config/src/providers/llm_tests.rs @@ -19,6 +19,36 @@ fn test_provider_config_anthropic() { ); } +#[test] +fn api_key_env_assigns_a_list_of_variables() { + let mut p = PartialLlmProviderConfig::default(); + + let kv = + KvAssignment::try_from_cli("vllm.api_key_env:", r#"["WORK_KEY", "USER_KEY"]"#).unwrap(); + p.assign(kv).unwrap(); + assert_eq!( + p.vllm.api_key_env, + Some(ApiKeyEnv::FirstOf(vec![ + "WORK_KEY".to_owned(), + "USER_KEY".to_owned() + ])) + ); +} + +#[test] +fn api_key_env_assigns_a_map_of_variables() { + let mut p = PartialLlmProviderConfig::default(); + + let kv = KvAssignment::try_from_cli("vllm.api_key_env:", r#"{"work": "WORK_KEY"}"#).unwrap(); + p.assign(kv).unwrap(); + assert_eq!( + p.vllm.api_key_env, + Some(ApiKeyEnv::Many( + [("work".to_owned(), "WORK_KEY".into())].into() + )) + ); +} + #[test] fn test_provider_config_openai() { let mut p = PartialLlmProviderConfig::default(); diff --git a/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap b/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap index 161ea695b..d76fe980d 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap @@ -1365,8 +1365,9 @@ providers: ProviderConfig *: string api_key_env?: ApiKeyEnv |: string + |: [string] |: - *: string + *: string | [string] auth?: [string] base_url?: string beta_headers?: MergeableVec @@ -1381,22 +1382,25 @@ providers: ProviderConfig cerebras: CerebrasConfig api_key_env?: ApiKeyEnv |: string + |: [string] |: - *: string + *: string | [string] auth?: [string] base_url?: string deepseek: DeepseekConfig api_key_env?: ApiKeyEnv |: string + |: [string] |: - *: string + *: string | [string] auth?: [string] base_url?: string google: GoogleConfig api_key_env?: ApiKeyEnv |: string + |: [string] |: - *: string + *: string | [string] auth?: [string] base_url?: string llamacpp: LlamacppConfig @@ -1406,8 +1410,9 @@ providers: ProviderConfig openai: OpenaiConfig api_key_env?: ApiKeyEnv |: string + |: [string] |: - *: string + *: string | [string] auth?: [string] base_url?: string base_url_env?: string @@ -1416,8 +1421,9 @@ providers: ProviderConfig openrouter: OpenrouterConfig api_key_env?: ApiKeyEnv |: string + |: [string] |: - *: string + *: string | [string] app_name?: string app_referrer: string | null auth?: [string] @@ -1425,8 +1431,9 @@ providers: ProviderConfig vllm: VllmConfig api_key_env?: ApiKeyEnv |: string + |: [string] |: - *: string + *: string | [string] auth?: [string] base_url?: string mcp: MergeableMap_McpProviderConfig diff --git a/crates/jp_config/src/types/api_key_env.rs b/crates/jp_config/src/types/api_key_env.rs index 6388ed46f..da3793633 100644 --- a/crates/jp_config/src/types/api_key_env.rs +++ b/crates/jp_config/src/types/api_key_env.rs @@ -1,6 +1,6 @@ //! Where a provider reads its API keys from. -use std::{collections::BTreeMap, convert::Infallible, fmt, str::FromStr}; +use std::{collections::BTreeMap, convert::Infallible, fmt, slice, str::FromStr}; use schematic::{Schema, SchemaBuilder, Schematic, schema::UnionType}; use serde::{Deserialize, Deserializer, Serialize, Serializer}; @@ -13,24 +13,93 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer}; /// api_key_env = "ANTHROPIC_API_KEY" /// ``` /// -/// Several are named, and the name is what an `auth` entry selects with +/// A list is still one key, read from the first variable that holds a non-empty +/// value: +/// +/// ```toml +/// api_key_env = ["WORK_ANTHROPIC_KEY", "ANTHROPIC_API_KEY"] +/// ``` +/// +/// Several keys are named, and the name is what an `auth` entry selects with /// `api_key:`: /// /// ```toml /// api_key_env = { work = "WORK_ANTHROPIC_KEY", personal = "PERSONAL_ANTHROPIC_KEY" } /// ``` /// +/// A named key can be a list too, read the same way as an unnamed one: +/// +/// ```toml +/// api_key_env = { work = ["WORK_ANTHROPIC_KEY", "ANTHROPIC_API_KEY"] } +/// ``` +/// /// Names where a key is read from; never holds one. #[derive(Debug, Clone, PartialEq, Eq)] pub enum ApiKeyEnv { /// One key, in the named variable. One(String), - /// Several keys, each with a name and its own variable. - Many(BTreeMap), + /// One key, in the first of these variables that holds a value. + FirstOf(Vec), + + /// Several keys, each with a name and its own variables. + Many(BTreeMap), } -/// Why [`ApiKeyEnv::variable`] could not name a variable. +/// Where one named key is read from. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(untagged)] +pub enum KeyVariables { + /// The named variable. + One(String), + + /// The first of these variables that holds a value. + FirstOf(Vec), +} + +impl KeyVariables { + /// The variables to read, in order. + #[must_use] + pub fn as_slice(&self) -> &[String] { + match self { + Self::One(variable) => slice::from_ref(variable), + Self::FirstOf(variables) => variables, + } + } +} + +impl From for KeyVariables { + fn from(variable: String) -> Self { + Self::One(variable) + } +} + +impl From<&str> for KeyVariables { + fn from(variable: &str) -> Self { + Self::One(variable.to_owned()) + } +} + +impl fmt::Display for KeyVariables { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::One(variable) => f.write_str(variable), + Self::FirstOf(variables) => write!(f, "[{}]", variables.join(", ")), + } + } +} + +impl Schematic for KeyVariables { + /// A variable name, or a list of them. + fn build_schema(mut schema: SchemaBuilder) -> Schema { + schema.union(UnionType::new_any([ + schema.infer::(), + schema.infer::>(), + ])) + } +} + +/// Why [`ApiKeyEnv::variables`] could not name a variable. #[derive(Debug, thiserror::Error)] pub enum ApiKeyEnvError { /// A name no key answers to. @@ -54,9 +123,16 @@ pub enum ApiKeyEnvError { available: Vec, }, - /// A map with nothing in it. + /// A list or map with nothing in it. #[error("no API keys are configured")] Empty, + + /// A named key whose list of variables is empty. + #[error("API key `{name}` names no environment variables")] + NoVariables { + /// The key with nothing to read. + name: String, + }, } /// Render the available names for an error, or nothing when there are none. @@ -69,39 +145,45 @@ fn render_available(names: &[String]) -> String { } impl ApiKeyEnv { - /// The variable holding the key `name` asks for, or the sole key when - /// `name` is `None`. + /// The variables that may hold the key `name` asks for, or the sole key + /// when `name` is `None`. + /// + /// The caller reads them in order and uses the first that holds a key. + /// Only a list, unnamed or named, yields more than one variable. /// /// # Errors /// /// Returns an error when the name is unknown, when `None` has several keys /// to choose between, or when none are configured. - pub fn variable(&self, name: Option<&str>) -> Result<&str, ApiKeyEnvError> { + pub fn variables(&self, name: Option<&str>) -> Result<&[String], ApiKeyEnvError> { match (self, name) { - (Self::One(variable), None) => Ok(variable), + (Self::One(variable), None) => Ok(slice::from_ref(variable)), + (Self::FirstOf(variables), None) if variables.is_empty() => Err(ApiKeyEnvError::Empty), + (Self::FirstOf(variables), None) => Ok(variables), - // A lone variable answers to no name. - (Self::One(_), Some(name)) => Err(ApiKeyEnvError::Unknown { + // A lone variable, or a list of places to read one key from, + // answers to no name. + (Self::One(_) | Self::FirstOf(_), Some(name)) => Err(ApiKeyEnvError::Unknown { name: name.to_owned(), available: vec![], }), (Self::Many(keys), Some(name)) => { - keys.get(name) - .map(String::as_str) - .ok_or_else(|| ApiKeyEnvError::Unknown { - name: name.to_owned(), - available: keys.keys().cloned().collect(), - }) + let variables = keys.get(name).ok_or_else(|| ApiKeyEnvError::Unknown { + name: name.to_owned(), + available: keys.keys().cloned().collect(), + })?; + + named_variables(name, variables) } (Self::Many(keys), None) => match keys.len() { 0 => Err(ApiKeyEnvError::Empty), 1 => keys - .values() + .iter() .next() - .map(String::as_str) - .ok_or(ApiKeyEnvError::Empty), + .ok_or(ApiKeyEnvError::Empty) + .and_then(|(name, variables)| named_variables(name, variables)), _ => Err(ApiKeyEnvError::Ambiguous { available: keys.keys().cloned().collect(), }), @@ -109,11 +191,11 @@ impl ApiKeyEnv { } } - /// Every name [`Self::variable`] accepts, empty for a lone variable. + /// Every name [`Self::variables`] accepts, empty unless keys are named. #[must_use] pub fn names(&self) -> Vec<&str> { match self { - Self::One(_) => vec![], + Self::One(_) | Self::FirstOf(_) => vec![], Self::Many(keys) => keys.keys().map(String::as_str).collect(), } } @@ -128,8 +210,8 @@ impl Default for ApiKeyEnv { impl FromStr for ApiKeyEnv { type Err = Infallible; - /// Read the single-variable form; the map form arrives as an object and - /// never reaches here. + /// Read the single-variable form; the list and map forms arrive as JSON and + /// never reach here. fn from_str(s: &str) -> Result { Ok(Self::One(s.to_owned())) } @@ -151,6 +233,7 @@ impl fmt::Display for ApiKeyEnv { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::One(variable) => f.write_str(variable), + Self::FirstOf(variables) => write!(f, "[{}]", variables.join(", ")), Self::Many(keys) => { let keys: Vec<_> = keys .iter() @@ -167,22 +250,38 @@ impl Serialize for ApiKeyEnv { fn serialize(&self, serializer: S) -> Result { match self { Self::One(variable) => variable.serialize(serializer), + Self::FirstOf(variables) => variables.serialize(serializer), Self::Many(keys) => keys.serialize(serializer), } } } +/// The variables a named key reads, rejecting a key with none. +fn named_variables<'a>( + name: &str, + variables: &'a KeyVariables, +) -> Result<&'a [String], ApiKeyEnvError> { + match variables.as_slice() { + [] => Err(ApiKeyEnvError::NoVariables { + name: name.to_owned(), + }), + variables => Ok(variables), + } +} + impl<'de> Deserialize<'de> for ApiKeyEnv { fn deserialize>(deserializer: D) -> Result { #[derive(Deserialize)] #[serde(untagged)] enum Raw { One(String), - Many(BTreeMap), + FirstOf(Vec), + Many(BTreeMap), } Ok(match Raw::deserialize(deserializer)? { Raw::One(variable) => Self::One(variable), + Raw::FirstOf(variables) => Self::FirstOf(variables), Raw::Many(keys) => Self::Many(keys), }) } @@ -193,11 +292,13 @@ impl Schematic for ApiKeyEnv { Some("ApiKeyEnv".into()) } - /// Either form: a variable name, or a map from key name to variable name. + /// Any form: a variable name, a list of variable names, or a map from key + /// name to either of those. fn build_schema(mut schema: SchemaBuilder) -> Schema { schema.union(UnionType::new_any([ schema.infer::(), - schema.infer::>(), + schema.infer::>(), + schema.infer::>(), ])) } } diff --git a/crates/jp_config/src/types/api_key_env_tests.rs b/crates/jp_config/src/types/api_key_env_tests.rs index 0c1394ed3..2a3deb56c 100644 --- a/crates/jp_config/src/types/api_key_env_tests.rs +++ b/crates/jp_config/src/types/api_key_env_tests.rs @@ -6,17 +6,97 @@ fn many(pairs: &[(&str, &str)]) -> ApiKeyEnv { ApiKeyEnv::Many( pairs .iter() - .map(|(name, variable)| ((*name).to_owned(), (*variable).to_owned())) + .map(|(name, variable)| ((*name).to_owned(), (*variable).into())) .collect(), ) } +fn named_list(name: &str, variables: &[&str]) -> ApiKeyEnv { + ApiKeyEnv::Many(BTreeMap::from([( + name.to_owned(), + KeyVariables::FirstOf(variables.iter().map(|v| (*v).to_owned()).collect()), + )])) +} + +fn first_of(variables: &[&str]) -> ApiKeyEnv { + ApiKeyEnv::FirstOf(variables.iter().map(|v| (*v).to_owned()).collect()) +} + /// The form every existing config is written in. #[test] fn a_single_variable_answers_a_bare_entry() { let keys = ApiKeyEnv::One("ANTHROPIC_API_KEY".to_owned()); - assert_eq!(keys.variable(None).unwrap(), "ANTHROPIC_API_KEY"); + assert_eq!(keys.variables(None).unwrap(), ["ANTHROPIC_API_KEY"]); +} + +/// The caller reads the variables in the order the user wrote them, so the +/// order is the contract. +#[test] +fn a_list_answers_a_bare_entry_in_order() { + let keys = first_of(&["WORK_KEY", "USER_KEY"]); + + assert_eq!(keys.variables(None).unwrap(), ["WORK_KEY", "USER_KEY"]); +} + +/// A list is one key read from several places, so it has no names to select. +#[test] +fn a_list_answers_to_no_name() { + let keys = first_of(&["WORK_KEY", "USER_KEY"]); + + assert!(matches!( + keys.variables(Some("work")), + Err(ApiKeyEnvError::Unknown { ref available, .. }) if available.is_empty() + )); + assert!(keys.names().is_empty()); +} + +/// A named key can itself be read from several variables, in order. +#[test] +fn a_named_key_can_be_a_list() { + let keys = ApiKeyEnv::Many(BTreeMap::from([ + ( + "work".to_owned(), + KeyVariables::FirstOf(vec!["WORK_KEY".to_owned(), "WORK_KEY_OLD".to_owned()]), + ), + ("personal".to_owned(), "PERSONAL_KEY".into()), + ])); + + assert_eq!(keys.variables(Some("work")).unwrap(), [ + "WORK_KEY", + "WORK_KEY_OLD" + ]); + assert_eq!(keys.variables(Some("personal")).unwrap(), ["PERSONAL_KEY"]); + assert_eq!(keys.names(), ["personal", "work"]); +} + +/// A sole named key answers a bare entry, list or not. +#[test] +fn a_bare_entry_resolves_a_sole_named_list() { + let keys = named_list("work", &["WORK_KEY", "USER_KEY"]); + + assert_eq!(keys.variables(None).unwrap(), ["WORK_KEY", "USER_KEY"]); +} + +/// A named key with no variables is a config mistake that names the key, not a +/// variable lookup that reports nothing. +#[test] +fn a_named_empty_list_reports_the_name() { + let keys = named_list("work", &[]); + + let error = keys.variables(Some("work")).unwrap_err(); + assert!(matches!(error, ApiKeyEnvError::NoVariables { ref name } if name == "work")); + assert_eq!( + error.to_string(), + "API key `work` names no environment variables" + ); +} + +#[test] +fn an_empty_list_configures_no_key() { + let keys = first_of(&[]); + + assert!(matches!(keys.variables(None), Err(ApiKeyEnvError::Empty))); } /// A lone variable has no name, so asking for one is asking for a key that is @@ -26,7 +106,7 @@ fn a_single_variable_answers_to_no_name() { let keys = ApiKeyEnv::One("ANTHROPIC_API_KEY".to_owned()); assert!(matches!( - keys.variable(Some("work")), + keys.variables(Some("work")), Err(ApiKeyEnvError::Unknown { .. }) )); } @@ -35,8 +115,8 @@ fn a_single_variable_answers_to_no_name() { fn a_named_key_is_selected_by_name() { let keys = many(&[("work", "WORK_KEY"), ("personal", "PERSONAL_KEY")]); - assert_eq!(keys.variable(Some("work")).unwrap(), "WORK_KEY"); - assert_eq!(keys.variable(Some("personal")).unwrap(), "PERSONAL_KEY"); + assert_eq!(keys.variables(Some("work")).unwrap(), ["WORK_KEY"]); + assert_eq!(keys.variables(Some("personal")).unwrap(), ["PERSONAL_KEY"]); } /// Which key pays is not a choice to make on the user's behalf. @@ -44,7 +124,7 @@ fn a_named_key_is_selected_by_name() { fn a_bare_entry_refuses_to_choose_between_several_keys() { let keys = many(&[("work", "WORK_KEY"), ("personal", "PERSONAL_KEY")]); - let error = keys.variable(None).unwrap_err(); + let error = keys.variables(None).unwrap_err(); assert!(matches!(error, ApiKeyEnvError::Ambiguous { .. })); // The message has to name both candidates and how to pick one, or it @@ -60,7 +140,7 @@ fn a_bare_entry_refuses_to_choose_between_several_keys() { fn a_bare_entry_resolves_a_sole_named_key() { let keys = many(&[("work", "WORK_KEY")]); - assert_eq!(keys.variable(None).unwrap(), "WORK_KEY"); + assert_eq!(keys.variables(None).unwrap(), ["WORK_KEY"]); } /// An unknown name reports what is configured, so the typo is visible. @@ -68,29 +148,45 @@ fn a_bare_entry_resolves_a_sole_named_key() { fn an_unknown_name_reports_the_configured_ones() { let keys = many(&[("work", "WORK_KEY"), ("personal", "PERSONAL_KEY")]); - let message = keys.variable(Some("persnoal")).unwrap_err().to_string(); + let message = keys.variables(Some("persnoal")).unwrap_err().to_string(); assert!(message.contains("persnoal"), "{message}"); assert!(message.contains("personal"), "{message}"); } -/// The schema describes both forms `Deserialize` accepts, so a validator or an -/// editor does not reject the named-key map. +/// The schema describes every form `Deserialize` accepts, so a validator or an +/// editor does not reject the list or the named-key map. #[test] -fn the_schema_accepts_a_variable_or_a_map_of_variables() { +fn the_schema_accepts_a_variable_a_list_or_a_map_of_variables() { let schema = SchemaBuilder::build_root::(); let SchemaType::Union(union) = schema.ty else { panic!("expected a union, got {:?}", schema.ty) }; let mut has_string = false; + let mut has_list = false; let mut has_map = false; for variant in union.variants_types { match variant.ty { SchemaType::String(_) => has_string = true, + SchemaType::Array(array) => { + assert!(matches!(array.items_type.ty, SchemaType::String(_))); + has_list = true; + } SchemaType::Object(object) => { assert!(matches!(object.key_type.ty, SchemaType::String(_))); - assert!(matches!(object.value_type.ty, SchemaType::String(_))); + + // Each named key is itself a variable or a list of them. + let SchemaType::Union(value) = object.value_type.ty else { + panic!("expected a union, got {:?}", object.value_type.ty) + }; + assert_eq!(value.variants_types.len(), 2); + assert!(matches!(value.variants_types[0].ty, SchemaType::String(_))); + let SchemaType::Array(array) = &value.variants_types[1].ty else { + panic!("expected an array, got {:?}", value.variants_types[1].ty) + }; + assert!(matches!(array.items_type.ty, SchemaType::String(_))); + has_map = true; } ty => panic!("unexpected variant: {ty:?}"), @@ -98,16 +194,17 @@ fn the_schema_accepts_a_variable_or_a_map_of_variables() { } assert!(has_string, "`api_key_env = \"KEY\"` must be described"); + assert!(has_list, "`api_key_env = [\"KEY\"]` must be described"); assert!( has_map, "`api_key_env = {{ work = \"KEY\" }}` must be described" ); } -/// Both forms have to survive a round trip, since one of them is what every -/// config written so far uses. +/// Every form has to survive a round trip, since a resolved config is written +/// back into the conversation stream. #[test] -fn both_forms_round_trip() { +fn every_form_round_trips() { let one = ApiKeyEnv::One("ANTHROPIC_API_KEY".to_owned()); let json = serde_json::to_string(&one).unwrap(); assert_eq!(json, r#""ANTHROPIC_API_KEY""#); @@ -117,4 +214,73 @@ fn both_forms_round_trip() { let json = serde_json::to_string(&many).unwrap(); assert_eq!(json, r#"{"work":"WORK_KEY"}"#); assert_eq!(serde_json::from_str::(&json).unwrap(), many); + + let list = first_of(&["WORK_KEY", "USER_KEY"]); + let json = serde_json::to_string(&list).unwrap(); + assert_eq!(json, r#"["WORK_KEY","USER_KEY"]"#); + assert_eq!(serde_json::from_str::(&json).unwrap(), list); + + let named = named_list("work", &["WORK_KEY", "USER_KEY"]); + let json = serde_json::to_string(&named).unwrap(); + assert_eq!(json, r#"{"work":["WORK_KEY","USER_KEY"]}"#); + assert_eq!(serde_json::from_str::(&json).unwrap(), named); +} + +#[test] +fn a_map_mixing_variables_and_lists_parses_from_toml() { + #[derive(Deserialize)] + struct Doc { + api_key_env: ApiKeyEnv, + } + + let doc: Doc = toml::from_str( + r#"api_key_env = { work = ["WORK_KEY", "USER_KEY"], personal = "PERSONAL_KEY" }"#, + ) + .unwrap(); + + assert_eq!( + doc.api_key_env, + ApiKeyEnv::Many(BTreeMap::from([ + ( + "work".to_owned(), + KeyVariables::FirstOf(vec!["WORK_KEY".to_owned(), "USER_KEY".to_owned()]), + ), + ( + "personal".to_owned(), + KeyVariables::One("PERSONAL_KEY".to_owned()) + ), + ])) + ); +} + +#[test] +fn a_list_parses_from_toml() { + #[derive(Deserialize)] + struct Doc { + api_key_env: ApiKeyEnv, + } + + let doc: Doc = toml::from_str(r#"api_key_env = ["WORK_KEY", "USER_KEY"]"#).unwrap(); + + assert_eq!(doc.api_key_env, first_of(&["WORK_KEY", "USER_KEY"])); +} + +#[test] +fn a_list_displays_as_a_list() { + assert_eq!( + first_of(&["WORK_KEY", "USER_KEY"]).to_string(), + "[WORK_KEY, USER_KEY]" + ); + + let keys = ApiKeyEnv::Many(BTreeMap::from([ + ( + "work".to_owned(), + KeyVariables::FirstOf(vec!["WORK_KEY".to_owned(), "USER_KEY".to_owned()]), + ), + ("personal".to_owned(), "PERSONAL_KEY".into()), + ])); + assert_eq!( + keys.to_string(), + "{ personal = PERSONAL_KEY, work = [WORK_KEY, USER_KEY] }" + ); } diff --git a/crates/jp_llm/src/provider/anthropic/resolve.rs b/crates/jp_llm/src/provider/anthropic/resolve.rs index fb753bae7..257e44460 100644 --- a/crates/jp_llm/src/provider/anthropic/resolve.rs +++ b/crates/jp_llm/src/provider/anthropic/resolve.rs @@ -36,7 +36,7 @@ use jp_credentials::{ use tracing::{debug, warn}; use super::{acp::Error as AcpError, oauth}; -use crate::{credential::Credential, error::StreamError}; +use crate::{credential::Credential, error::StreamError, provider::api_key_chain}; /// Errors from walking the credential chain. #[derive(Debug, thiserror::Error)] @@ -891,12 +891,12 @@ fn walk_chain( // A name no key answers to is a config mistake, not a // credential to fall past: the next entry would bill a // different key. - let variable = config + let variables = config .api_key_env - .variable(name.as_deref()) + .variables(name.as_deref()) .map_err(ResolveError::ApiKeyEnv)?; - if let Some(key) = super::super::api_key_chain::read_key(variable) { + if let Some(key) = api_key_chain::read_first_key(variables) { return Ok(( Landing::Ready(Credential::ApiKey(key)), Selected { @@ -909,8 +909,9 @@ fn walk_chain( // Under the single-entry default chain, a missing key is // the same failure it was before chains existed. + let variable = api_key_chain::describe_variables(variables); if config.auth.len() == 1 { - return Err(ResolveError::MissingEnv(variable.to_owned())); + return Err(ResolveError::MissingEnv(variable)); } skip( diff --git a/crates/jp_llm/src/provider/api_key_chain.rs b/crates/jp_llm/src/provider/api_key_chain.rs index 5a5cae1e8..5855bb366 100644 --- a/crates/jp_llm/src/provider/api_key_chain.rs +++ b/crates/jp_llm/src/provider/api_key_chain.rs @@ -27,6 +27,21 @@ pub fn read_key(variable: &str) -> Option { env::var(variable).ok().filter(|key| !key.trim().is_empty()) } +/// Read the key from the first of `variables` that holds one, in order. +/// +/// Each variable is read as [`read_key`] reads it, so a blank value falls +/// through to the next. +#[must_use] +pub fn read_first_key(variables: &[String]) -> Option { + variables.iter().find_map(|variable| read_key(variable)) +} + +/// Name `variables` in a message saying none of them holds a key. +#[must_use] +pub(crate) fn describe_variables(variables: &[String]) -> String { + variables.join(" or ") +} + /// Why an API-key chain produced no credential. #[derive(Debug, thiserror::Error)] pub enum ChainError { @@ -106,19 +121,20 @@ pub(crate) fn resolve( // A name no key answers to is a config mistake, not a credential to // fall past: the next entry would bill a different key. - let variable = keys - .variable(name) + let variables = keys + .variables(name) .map_err(|source| ChainError::UnknownKey { provider: provider.to_owned(), source, })?; - if let Some(key) = read_key(variable) { + if let Some(key) = read_first_key(variables) { return Ok((key, AuthEntry::ApiKey(name.map(str::to_owned)))); } + let variable = describe_variables(variables); if chain.len() == 1 { - return Err(ChainError::MissingEnv(variable.to_owned())); + return Err(ChainError::MissingEnv(variable)); } debug!(provider, %entry, variable, "Skipping API key with no value."); diff --git a/crates/jp_llm/src/provider/api_key_chain_tests.rs b/crates/jp_llm/src/provider/api_key_chain_tests.rs index 1abf0eb02..c38e33c17 100644 --- a/crates/jp_llm/src/provider/api_key_chain_tests.rs +++ b/crates/jp_llm/src/provider/api_key_chain_tests.rs @@ -1,5 +1,7 @@ use std::collections::BTreeMap; +use jp_config::types::api_key_env::KeyVariables; + use super::*; /// A variable the environment always holds, so no test has to set one and race @@ -27,7 +29,7 @@ fn many(pairs: &[(&str, &str)]) -> ApiKeyEnv { ApiKeyEnv::Many( pairs .iter() - .map(|(name, variable)| ((*name).to_owned(), (*variable).to_owned())) + .map(|(name, variable)| ((*name).to_owned(), (*variable).into())) .collect::>(), ) } @@ -114,6 +116,63 @@ fn test_single_entry_reports_the_missing_variable() { ); } +fn first_of(variables: &[&str]) -> ApiKeyEnv { + ApiKeyEnv::FirstOf(variables.iter().map(|v| (*v).to_owned()).collect()) +} + +/// A list is tried in order, so an unset first variable falls through to the +/// next one without the chain having to name a second entry. +#[test] +fn test_a_list_reads_the_first_variable_that_is_set() { + let expected = std::env::var(set()).unwrap(); + let keys = first_of(&[UNSET, set(), also_set()]); + + let (key, selected) = resolve("cerebras", &chain(&["api_key"]), &keys).unwrap(); + + assert_eq!(key, expected); + assert_eq!(selected, AuthEntry::ApiKey(None)); +} + +/// The earlier variable wins when both are set. +#[test] +fn test_a_list_prefers_the_earlier_variable() { + let expected = std::env::var(also_set()).unwrap(); + let keys = first_of(&[also_set(), set()]); + + let (key, _) = resolve("cerebras", &chain(&["api_key"]), &keys).unwrap(); + + assert_eq!(key, expected); +} + +/// A named key's list falls through within the one entry, and the entry it +/// reports is still the name the chain asked for. +#[test] +fn test_a_named_list_reads_the_first_variable_that_is_set() { + let expected = std::env::var(set()).unwrap(); + let keys = ApiKeyEnv::Many(BTreeMap::from([( + "work".to_owned(), + KeyVariables::FirstOf(vec![UNSET.to_owned(), set().to_owned()]), + )])); + + let (key, selected) = resolve("cerebras", &chain(&["api_key:work"]), &keys).unwrap(); + + assert_eq!(key, expected); + assert_eq!(selected, AuthEntry::ApiKey(Some("work".to_owned()))); +} + +/// With none set, the error names every variable that was tried. +#[test] +fn test_a_list_with_nothing_set_names_every_variable() { + let keys = first_of(&[UNSET, ALSO_UNSET]); + + let error = resolve("cerebras", &chain(&["api_key"]), &keys).unwrap_err(); + + assert_eq!( + error.to_string(), + "Missing environment variable: JP_TEST_CHAIN_KEY_UNSET or JP_TEST_CHAIN_KEY_ALSO_UNSET" + ); +} + #[test] fn test_named_key_is_selected_by_name() { let expected = std::env::var(also_set()).unwrap(); diff --git a/crates/jp_llm/src/provider/openai/auth_rejected_tests.rs b/crates/jp_llm/src/provider/openai/auth_rejected_tests.rs index 8a33e4e11..be73a263c 100644 --- a/crates/jp_llm/src/provider/openai/auth_rejected_tests.rs +++ b/crates/jp_llm/src/provider/openai/auth_rejected_tests.rs @@ -88,8 +88,8 @@ async fn refused_twice(model: ModelDetails) -> (Vec, StreamError) { AuthEntry::ApiKey(Some("personal".to_owned())), ]; config.api_key_env = ApiKeyEnv::Many(BTreeMap::from([ - ("work".to_owned(), WORK_KEY_ENV.to_owned()), - ("personal".to_owned(), PERSONAL_KEY_ENV.to_owned()), + ("work".to_owned(), WORK_KEY_ENV.into()), + ("personal".to_owned(), PERSONAL_KEY_ENV.into()), ])); let provider = Openai::new(&config).unwrap(); diff --git a/crates/jp_llm/src/provider/openai/resolve.rs b/crates/jp_llm/src/provider/openai/resolve.rs index 046081018..801d8f270 100644 --- a/crates/jp_llm/src/provider/openai/resolve.rs +++ b/crates/jp_llm/src/provider/openai/resolve.rs @@ -34,7 +34,7 @@ use tracing::{debug, warn}; use crate::{ credential::Credential, error::{StreamError, StreamErrorKind}, - provider::openai::oauth, + provider::{api_key_chain, openai::oauth}, }; /// Errors from walking the credential chain. @@ -696,12 +696,12 @@ fn walk_chain( // A name no key answers to is a config mistake, not a // credential to fall past: the next entry would bill a // different key. - let variable = config + let variables = config .api_key_env - .variable(name.as_deref()) + .variables(name.as_deref()) .map_err(ResolveError::ApiKeyEnv)?; - if let Some(key) = super::super::api_key_chain::read_key(variable) { + if let Some(key) = api_key_chain::read_first_key(variables) { return Ok(( Landing::Ready(Credential::ApiKey(key), Attribution::default()), entry.clone(), @@ -712,8 +712,9 @@ fn walk_chain( // A single-entry chain reports the missing variable by name, // rather than as an exhausted chain of one. + let variable = api_key_chain::describe_variables(variables); if config.auth.len() == 1 { - return Err(ResolveError::MissingEnv(variable.to_owned())); + return Err(ResolveError::MissingEnv(variable)); } skip( diff --git a/crates/jp_llm/src/provider/openai/resolve_tests.rs b/crates/jp_llm/src/provider/openai/resolve_tests.rs index 8a867a33c..18f30e04b 100644 --- a/crates/jp_llm/src/provider/openai/resolve_tests.rs +++ b/crates/jp_llm/src/provider/openai/resolve_tests.rs @@ -967,8 +967,8 @@ async fn test_an_entry_is_tried_once_per_request() { AuthEntry::ApiKey(Some("personal".to_owned())), ]); chain.api_key_env = ApiKeyEnv::Many(BTreeMap::from([ - ("work".to_owned(), SET_ENV_VAR.to_owned()), - ("personal".to_owned(), ALSO_SET_ENV_VAR.to_owned()), + ("work".to_owned(), SET_ENV_VAR.into()), + ("personal".to_owned(), ALSO_SET_ENV_VAR.into()), ])); let refused = StreamError::auth_rejected("invalid key"); From 4b0a4c2defdc40cb8442a1fbf1baa612934067f4 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Tue, 6 Oct 2026 20:42:16 +0200 Subject: [PATCH 2/2] refactor(config): Drop unused `From` for `KeyVariables` Nothing converts a `String` into `KeyVariables`; every caller builds one from a `&str` or a variant constructor. The impl was also a trivial wrap, which rustqual counts as boilerplate (BP-001), and it pushed `boilerplate_warnings` one above the committed baseline, failing `just qual-ci`. Signed-off-by: Jean Mertz --- crates/jp_config/src/types/api_key_env.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/crates/jp_config/src/types/api_key_env.rs b/crates/jp_config/src/types/api_key_env.rs index da3793633..2abc84086 100644 --- a/crates/jp_config/src/types/api_key_env.rs +++ b/crates/jp_config/src/types/api_key_env.rs @@ -68,12 +68,6 @@ impl KeyVariables { } } -impl From for KeyVariables { - fn from(variable: String) -> Self { - Self::One(variable) - } -} - impl From<&str> for KeyVariables { fn from(variable: &str) -> Self { Self::One(variable.to_owned())