diff --git a/Cargo.lock b/Cargo.lock index 32a43577..401d6320 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -305,6 +305,29 @@ version = "0.3.32" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7e3450815272ef58cec6d564423f6e755e25379b217b0bc688e295ba24df6b1d" +[[package]] +name = "futures-io" +version = "0.3.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cecba35d7ad927e23624b22ad55235f2239cfa44fd10428eecbeba6d6a717718" + +[[package]] +name = "futures-macro" +version = "0.3.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e835b70203e41293343137df5c0664546da5745f82ec9b84d40be8336958447b" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "futures-sink" +version = "0.3.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c39754e157331b013978ec91992bde1ac089843443c49cbc7f46150b0fad0893" + [[package]] name = "futures-task" version = "0.3.32" @@ -318,7 +341,11 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "389ca41296e6190b48053de0321d02a77f32f8a5d2461dd38762c0593805c6d6" dependencies = [ "futures-core", + "futures-io", + "futures-macro", + "futures-sink", "futures-task", + "memchr", "pin-project-lite", "slab", ] @@ -1079,12 +1106,14 @@ dependencies = [ "sync_wrapper", "tokio", "tokio-rustls", + "tokio-util", "tower", "tower-http", "tower-service", "url", "wasm-bindgen", "wasm-bindgen-futures", + "wasm-streams", "web-sys", "webpki-roots 1.0.7", ] @@ -1486,6 +1515,19 @@ dependencies = [ "tokio", ] +[[package]] +name = "tokio-util" +version = "0.7.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9ae9cec805b01e8fc3fd2fe289f89149a9b66dd16786abd8b19cfa7b48cb0098" +dependencies = [ + "bytes", + "futures-core", + "futures-sink", + "pin-project-lite", + "tokio", +] + [[package]] name = "toml" version = "0.8.23" @@ -1791,6 +1833,19 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "wasm-streams" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "15053d8d85c7eccdbefef60f06769760a563c7f0a9d6902a13d35c7800b0ad65" +dependencies = [ + "futures-util", + "js-sys", + "wasm-bindgen", + "wasm-bindgen-futures", + "web-sys", +] + [[package]] name = "web-sys" version = "0.3.95" diff --git a/crates/genie-core/Cargo.toml b/crates/genie-core/Cargo.toml index 40531a10..64bb79b3 100644 --- a/crates/genie-core/Cargo.toml +++ b/crates/genie-core/Cargo.toml @@ -46,7 +46,7 @@ libloading = "0.8" toml = { workspace = true } genie-skill-sdk = { path = "../genie-skill-sdk" } async-trait = "0.1" -reqwest = { version = "0.12", default-features = false, features = ["json", "multipart", "rustls-tls"] } +reqwest = { version = "0.12", default-features = false, features = ["json", "multipart", "rustls-tls", "stream"] } # Detached Ed25519 verification of native skill `.so` bytes before dlopen. # Verification only — no RNG/key-generation features pulled into the runtime. ed25519-dalek = { version = "2", default-features = false, features = ["std"] } diff --git a/crates/genie-core/src/llm/mod.rs b/crates/genie-core/src/llm/mod.rs index 1888921b..43a44f34 100644 --- a/crates/genie-core/src/llm/mod.rs +++ b/crates/genie-core/src/llm/mod.rs @@ -170,14 +170,15 @@ impl LlmClient { let provider = &config.optional_ai_provider; match provider.provider { OptionalAiProviderKind::OpenAiCompatible | OptionalAiProviderKind::OpenAi => { - Ok( - Self::from_openai_compatible_url_with_bearer_token_env_and_model( - provider.base_url.trim(), - provider.credential_env(), - provider.model.trim(), - timeouts, - ), - ) + let backend = OpenAiCompatibleBackend::try_new( + provider.base_url.trim(), + provider.model.trim(), + provider.credential_env(), + timeouts, + )?; + Ok(Self { + backend: Box::new(backend), + }) } OptionalAiProviderKind::Anthropic | OptionalAiProviderKind::Gemini diff --git a/crates/genie-core/src/llm/openai_compat.rs b/crates/genie-core/src/llm/openai_compat.rs index 0fb15b20..5821052b 100644 --- a/crates/genie-core/src/llm/openai_compat.rs +++ b/crates/genie-core/src/llm/openai_compat.rs @@ -1212,7 +1212,25 @@ fn flatten_system_into_first_user(messages: &[Message]) -> Vec { flattened } -fn backend_error_message(body: &str) -> String { +/// Serialize a generic OpenAI-compatible chat body (no `nvext` / session fields). +pub(crate) fn serialize_generic_chat_request( + model: &str, + messages: &[Message], + max_tokens: Option, + stream: bool, + response_format: Option, +) -> Result { + RequestProfile::generic_with_model(model).serialize_body( + messages, + max_tokens, + stream, + response_format, + None, + ) +} + +/// Extract a human-readable message from an OpenAI-compatible error body. +pub(crate) fn backend_error_message(body: &str) -> String { serde_json::from_str::(body) .ok() .and_then(|json| { @@ -1229,7 +1247,7 @@ fn backend_error_message(body: &str) -> String { .unwrap_or_else(|| truncate_body(body)) } -fn truncate_body(body: &str) -> String { +pub(crate) fn truncate_body(body: &str) -> String { const MAX_LEN: usize = 240; let trimmed = body.trim(); if trimmed.len() <= MAX_LEN { diff --git a/crates/genie-core/src/llm/openai_compatible.rs b/crates/genie-core/src/llm/openai_compatible.rs index 5bdd76cc..89b2d929 100644 --- a/crates/genie-core/src/llm/openai_compatible.rs +++ b/crates/genie-core/src/llm/openai_compatible.rs @@ -1,13 +1,66 @@ -use anyhow::Result; +//! Generic OpenAI-compatible HTTP/HTTPS transport for optional API providers (#569). +//! +//! Unlike the localhost raw-TCP client in [`super::openai_compat`], this backend +//! uses `reqwest` so it can reach loopback *and* remote HTTPS endpoints while +//! preserving the configured base path (for example `/v1`). + +use std::fmt; +use std::time::Duration; + +use anyhow::{Context, Result}; use async_trait::async_trait; +use reqwest::Url; -use super::openai_compat::{LlmTimeouts, OpenAiCompatClient, RequestProfile}; +use super::openai_compat::{ + ChatResponse, LlmTimeouts, backend_error_message, serialize_generic_chat_request, truncate_body, +}; use super::{LlmBackendClient, LlmRequestHints, Message, ResponseFormat}; +use crate::security::sandbox::sanitize_output; + +const MAX_ERROR_BODY_BYTES: usize = 64 * 1024; +const MAX_SSE_LINE_BYTES: usize = 64 * 1024; +const DEFAULT_MAX_RESPONSE_BYTES: usize = 8 * 1024 * 1024; + +/// How the backend obtains its bearer credential. +#[derive(Clone)] +enum CredentialSource { + /// Literal token (tests / explicit constructors). Never logged. + Literal(String), + /// Environment variable name resolved on every request (#569 fail-closed). + EnvVar(String), +} + +impl fmt::Debug for CredentialSource { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Literal(_) => f.write_str("CredentialSource::Literal([redacted])"), + Self::EnvVar(name) => f + .debug_tuple("CredentialSource::EnvVar") + .field(name) + .finish(), + } + } +} /// Generic OpenAI-compatible adapter for API providers that authenticate with /// bearer tokens, including OAuth access tokens. pub struct OpenAiCompatibleBackend { - inner: OpenAiCompatClient, + base_url: Url, + model: String, + credential: CredentialSource, + timeouts: LlmTimeouts, + http: reqwest::Client, +} + +impl fmt::Debug for OpenAiCompatibleBackend { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("OpenAiCompatibleBackend") + .field("base_url", &self.base_url.as_str()) + .field("model", &self.model) + .field("credential", &self.credential) + .field("timeouts", &self.timeouts) + .finish_non_exhaustive() + } } impl OpenAiCompatibleBackend { @@ -20,15 +73,13 @@ impl OpenAiCompatibleBackend { token: impl AsRef, timeouts: LlmTimeouts, ) -> Self { - Self { - inner: OpenAiCompatClient::from_url_with_profile_and_timeouts( - "openai-compatible", - url, - RequestProfile::generic(), - timeouts, - ) - .with_bearer_token(token), - } + Self::new( + url, + "default", + CredentialSource::Literal(token.as_ref().to_string()), + timeouts, + ) + .expect("literal-token OpenAI-compatible URL must be http/https") } pub fn from_url_with_bearer_token_env(url: &str, env_var: impl AsRef) -> Self { @@ -40,15 +91,13 @@ impl OpenAiCompatibleBackend { env_var: impl AsRef, timeouts: LlmTimeouts, ) -> Self { - Self { - inner: OpenAiCompatClient::from_url_with_profile_and_timeouts( - "openai-compatible", - url, - RequestProfile::generic(), - timeouts, - ) - .with_bearer_token_env(env_var), - } + Self::new( + url, + "default", + CredentialSource::EnvVar(env_var.as_ref().trim().to_string()), + timeouts, + ) + .expect("env-token OpenAI-compatible URL must be http/https") } /// Same as [`Self::from_url_with_bearer_token_env_and_timeouts`], but with @@ -60,26 +109,332 @@ impl OpenAiCompatibleBackend { model: impl Into, timeouts: LlmTimeouts, ) -> Self { - Self { - inner: OpenAiCompatClient::from_url_with_profile_and_timeouts( - "openai-compatible", - url, - RequestProfile::generic_with_model(model), - timeouts, + Self::new( + url, + model, + CredentialSource::EnvVar(env_var.as_ref().trim().to_string()), + timeouts, + ) + .expect("configured OpenAI-compatible URL must be http/https") + } + + /// Fallible constructor for config/load paths and integration tests. + pub fn try_new( + url: &str, + model: impl Into, + credential_env: impl AsRef, + timeouts: LlmTimeouts, + ) -> Result { + Self::new( + url, + model, + CredentialSource::EnvVar(credential_env.as_ref().trim().to_string()), + timeouts, + ) + } + + fn new( + url: &str, + model: impl Into, + credential: CredentialSource, + timeouts: LlmTimeouts, + ) -> Result { + let base_url = parse_openai_compatible_base_url(url)?; + // No client-wide request timeout: non-stream calls set one per request, + // and streaming relies on per-chunk idle timeouts instead. + let http = reqwest::Client::builder() + .connect_timeout(timeouts.connect) + .pool_max_idle_per_host(0) + .build() + .context("failed to build OpenAI-compatible HTTP client")?; + Ok(Self { + base_url, + model: model.into(), + credential, + timeouts, + http, + }) + } + + fn chat_completions_url(&self) -> Result { + join_chat_completions(&self.base_url) + } + + fn resolve_bearer_token(&self) -> Result { + let token = match &self.credential { + CredentialSource::Literal(token) => token.clone(), + CredentialSource::EnvVar(name) => { + if name.is_empty() { + anyhow::bail!( + "openai-compatible provider misconfigured: credential environment variable name is empty" + ); + } + match std::env::var(name) { + Ok(value) => value, + Err(_) => anyhow::bail!( + "openai-compatible provider misconfigured: environment variable {name} is not set" + ), + } + } + }; + let token = token.trim(); + if token.is_empty() { + anyhow::bail!( + "openai-compatible provider misconfigured: credential environment variable is empty" + ); + } + if token.contains(['\r', '\n']) { + anyhow::bail!( + "openai-compatible provider misconfigured: credential contains invalid header characters" + ); + } + Ok(token.to_string()) + } + + fn sanitize_error_detail(&self, body: &str, credential: &str) -> String { + let mut detail = backend_error_message(body); + if !credential.is_empty() { + detail = detail.replace(credential, "[REDACTED]"); + } + let detail = sanitize_output(&detail); + truncate_body(&detail) + } + + async fn post_chat( + &self, + stream: bool, + messages: &[Message], + max_tokens: Option, + response_format: Option, + ) -> Result { + let token = self.resolve_bearer_token()?; + let url = self.chat_completions_url()?; + let body = serialize_generic_chat_request( + &self.model, + messages, + max_tokens, + stream, + response_format, + )?; + + let mut request = self + .http + .post(url) + .header("Authorization", format!("Bearer {token}")) + .header("Content-Type", "application/json") + .body(body); + + if stream { + request = request.header("Accept", "text/event-stream"); + } else { + request = request.timeout(self.timeouts.request); + } + + let response = request.send().await.map_err(|err| { + // reqwest errors can embed URLs; never include Authorization. + anyhow::anyhow!( + "openai-compatible request failed: {}", + redact_reqwest_error(&err) ) - .with_bearer_token_env(env_var), + })?; + + Ok(response) + } + + async fn chat_once( + &self, + messages: &[Message], + max_tokens: Option, + response_format: Option, + ) -> Result { + let token = self.resolve_bearer_token()?; + let response = self + .post_chat(false, messages, max_tokens, response_format) + .await?; + let status = response.status(); + let body = response + .text() + .await + .map_err(|err| anyhow::anyhow!("openai-compatible response read failed: {err}"))?; + if body.len() > DEFAULT_MAX_RESPONSE_BYTES { + anyhow::bail!( + "openai-compatible response exceeded {} bytes", + DEFAULT_MAX_RESPONSE_BYTES + ); + } + if !status.is_success() { + anyhow::bail!( + "openai-compatible {}: {}", + status.as_u16(), + self.sanitize_error_detail(&body, &token) + ); + } + + let chat_resp: ChatResponse = serde_json::from_str(&body).map_err(|e| { + anyhow::anyhow!( + "failed to parse openai-compatible response: {}; body: {}", + e, + truncate_body(&body) + ) + })?; + Ok(chat_resp + .choices + .first() + .and_then(|c| c.message.as_ref()) + .map(|m| m.content.clone()) + .unwrap_or_default()) + } + + async fn chat_stream_once( + &self, + messages: &[Message], + max_tokens: Option, + on_token: &mut (dyn for<'a> FnMut(&'a str) + Send), + ) -> Result { + let token = self.resolve_bearer_token()?; + let response = self.post_chat(true, messages, max_tokens, None).await?; + let status = response.status(); + if !status.is_success() { + let body = response.text().await.unwrap_or_default(); + let body = if body.len() > MAX_ERROR_BODY_BYTES { + truncate_body(&body) + } else { + body + }; + anyhow::bail!( + "openai-compatible {}: {}", + status.as_u16(), + self.sanitize_error_detail(&body, &token) + ); + } + + let mut full_response = String::new(); + let mut line_buf = String::new(); + let mut total_bytes = 0usize; + let mut stream = response; + loop { + let chunk = tokio::time::timeout(self.timeouts.read, stream.chunk()) + .await + .map_err(|_| { + anyhow::anyhow!( + "openai-compatible stream read timed out after {}s", + self.timeouts.read.as_secs() + ) + })? + .map_err(|err| anyhow::anyhow!("openai-compatible stream read failed: {err}"))?; + let Some(chunk) = chunk else { + break; + }; + total_bytes = total_bytes.saturating_add(chunk.len()); + if total_bytes > DEFAULT_MAX_RESPONSE_BYTES { + anyhow::bail!( + "openai-compatible streaming response exceeded {} bytes", + DEFAULT_MAX_RESPONSE_BYTES + ); + } + let text = String::from_utf8_lossy(&chunk); + for ch in text.chars() { + if ch == '\n' { + let line = line_buf.trim_end_matches('\r').to_string(); + line_buf.clear(); + if line.len() > MAX_SSE_LINE_BYTES { + anyhow::bail!( + "openai-compatible streaming line exceeded {} bytes", + MAX_SSE_LINE_BYTES + ); + } + if let Some(data) = line.strip_prefix("data: ") { + if data == "[DONE]" { + return Ok(full_response); + } + if let Ok(chunk) = serde_json::from_str::(data) + && let Some(choice) = chunk.choices.first() + { + if let Some(delta) = &choice.delta + && let Some(content) = &delta.content + { + on_token(content); + full_response.push_str(content); + } + if choice.finish_reason.is_some() { + return Ok(full_response); + } + } + } + } else { + line_buf.push(ch); + if line_buf.len() > MAX_SSE_LINE_BYTES { + anyhow::bail!( + "openai-compatible streaming line exceeded {} bytes", + MAX_SSE_LINE_BYTES + ); + } + } + } + } + Ok(full_response) + } +} + +/// Parse and validate an OpenAI-compatible base URL (http/https only). +pub(crate) fn parse_openai_compatible_base_url(url: &str) -> Result { + let trimmed = url.trim(); + if trimmed.is_empty() { + anyhow::bail!("openai-compatible base_url must not be empty"); + } + let parsed = Url::parse(trimmed) + .with_context(|| format!("invalid openai-compatible base_url: {trimmed}"))?; + match parsed.scheme() { + "http" | "https" => {} + other => anyhow::bail!("openai-compatible base_url must use http or https (got {other})"), + } + if parsed.host_str().is_none() { + anyhow::bail!("openai-compatible base_url must include a host"); + } + Ok(parsed) +} + +/// Join `{base}/chat/completions`, preserving the configured base path. +pub(crate) fn join_chat_completions(base: &Url) -> Result { + let mut base_str = base.as_str().trim_end_matches('/').to_string(); + if base_str.is_empty() { + anyhow::bail!("openai-compatible base_url became empty"); + } + base_str.push_str("/chat/completions"); + Url::parse(&base_str).context("failed to join openai-compatible chat/completions URL") +} + +fn redact_reqwest_error(err: &reqwest::Error) -> String { + // Avoid dumping full debug which may include request builder state. + let mut msg = err.to_string(); + if let Some(url) = err.url() { + // URL is fine; strip any accidental userinfo. + let safe = url.as_str().split('@').next_back().unwrap_or(url.as_str()); + if !msg.contains(safe) { + msg = format!("{msg} ({safe})"); } } + sanitize_output(&msg) } #[async_trait] impl LlmBackendClient for OpenAiCompatibleBackend { fn backend_name(&self) -> &str { - self.inner.backend_name() + "openai-compatible" } async fn health(&self) -> bool { - self.inner.health().await + // Optional remote providers often lack a portable /health; treat a + // successful HEAD/GET on the base URL as best-effort reachability. + let url = self.base_url.clone(); + matches!( + self.http + .get(url) + .timeout(self.timeouts.connect.max(Duration::from_secs(2))) + .send() + .await, + Ok(resp) if resp.status().is_success() || resp.status().as_u16() == 404 + ) } async fn chat_with_format( @@ -88,9 +443,7 @@ impl LlmBackendClient for OpenAiCompatibleBackend { max_tokens: Option, response_format: Option, ) -> Result { - self.inner - .chat_with_format(messages, max_tokens, response_format) - .await + self.chat_once(messages, max_tokens, response_format).await } async fn chat_with_format_and_hints( @@ -98,11 +451,10 @@ impl LlmBackendClient for OpenAiCompatibleBackend { messages: &[Message], max_tokens: Option, response_format: Option, - hints: Option<&LlmRequestHints>, + _hints: Option<&LlmRequestHints>, ) -> Result { - self.inner - .chat_with_format_and_hints(messages, max_tokens, response_format, hints) - .await + // Generic providers ignore cache-aware hints (no nvext). + self.chat_once(messages, max_tokens, response_format).await } async fn chat_stream( @@ -111,18 +463,84 @@ impl LlmBackendClient for OpenAiCompatibleBackend { max_tokens: Option, on_token: &mut (dyn for<'a> FnMut(&'a str) + Send), ) -> Result { - self.inner.chat_stream(messages, max_tokens, on_token).await + self.chat_stream_once(messages, max_tokens, on_token).await } async fn chat_stream_with_hints( &self, messages: &[Message], max_tokens: Option, - hints: Option<&LlmRequestHints>, + _hints: Option<&LlmRequestHints>, on_token: &mut (dyn for<'a> FnMut(&'a str) + Send), ) -> Result { - self.inner - .chat_stream_with_hints(messages, max_tokens, hints, on_token) - .await + self.chat_stream_once(messages, max_tokens, on_token).await + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parse_accepts_http_and_https() { + let http = parse_openai_compatible_base_url("http://127.0.0.1:11434/v1").unwrap(); + assert_eq!(http.scheme(), "http"); + assert_eq!(http.path(), "/v1"); + + let https = parse_openai_compatible_base_url("https://api.openai.com/v1").unwrap(); + assert_eq!(https.scheme(), "https"); + assert_eq!(https.host_str(), Some("api.openai.com")); + } + + #[test] + fn parse_rejects_unsupported_schemes() { + let err = parse_openai_compatible_base_url("ftp://example.com/v1") + .unwrap_err() + .to_string(); + assert!(err.contains("http or https"), "{err}"); + } + + #[test] + fn join_preserves_base_path() { + let base = parse_openai_compatible_base_url("http://127.0.0.1:11434/v1").unwrap(); + let joined = join_chat_completions(&base).unwrap(); + assert_eq!( + joined.as_str(), + "http://127.0.0.1:11434/v1/chat/completions" + ); + + let with_slash = parse_openai_compatible_base_url("http://127.0.0.1:11434/v1/").unwrap(); + let joined = join_chat_completions(&with_slash).unwrap(); + assert_eq!( + joined.as_str(), + "http://127.0.0.1:11434/v1/chat/completions" + ); + } + + #[test] + fn debug_redacts_literal_credential() { + let backend = OpenAiCompatibleBackend::from_url_with_bearer_token( + "http://127.0.0.1:9/v1", + "super-secret-token", + ); + let rendered = format!("{backend:?}"); + assert!(rendered.contains("[redacted]"), "{rendered}"); + assert!(!rendered.contains("super-secret-token"), "{rendered}"); + } + + #[test] + fn resolve_bearer_token_requires_env_value() { + let backend = OpenAiCompatibleBackend::from_url_with_bearer_token_env_and_model( + "http://127.0.0.1:9/v1", + "OPENAI_COMPAT_MISSING_ENV_FOR_UNIT_TEST", + "test-model", + LlmTimeouts::default(), + ); + unsafe { + std::env::remove_var("OPENAI_COMPAT_MISSING_ENV_FOR_UNIT_TEST"); + } + let err = backend.resolve_bearer_token().unwrap_err().to_string(); + assert!(err.contains("is not set"), "{err}"); + assert!(!err.contains("Bearer"), "{err}"); } } diff --git a/crates/genie-core/src/llm/provider.rs b/crates/genie-core/src/llm/provider.rs index f63eba26..910d7a26 100644 --- a/crates/genie-core/src/llm/provider.rs +++ b/crates/genie-core/src/llm/provider.rs @@ -56,6 +56,21 @@ impl OptionalProviderPlan { OptionalAiProviderAuthMode::ApiKey => "missing_api_key_env", OptionalAiProviderAuthMode::OAuthBearer => "missing_oauth_token_env", }); + } else { + // Fail closed at complete-time if the operator unset/emptied the + // credential after boot (#569). Config load already checks this; + // readiness must too so GatedProvider cannot race a missing key. + match std::env::var(self.credential_env()) { + Ok(value) if !value.trim().is_empty() => {} + Ok(_) => reasons.push(match self.auth_mode { + OptionalAiProviderAuthMode::ApiKey => "empty_api_key_env_value", + OptionalAiProviderAuthMode::OAuthBearer => "empty_oauth_token_env_value", + }), + Err(_) => reasons.push(match self.auth_mode { + OptionalAiProviderAuthMode::ApiKey => "api_key_env_unset", + OptionalAiProviderAuthMode::OAuthBearer => "oauth_token_env_unset", + }), + } } if self.base_url.trim().is_empty() { reasons.push("missing_base_url"); @@ -110,6 +125,19 @@ fn blocked_reason_message(reason: &str) -> String { "[optional_ai_provider].oauth_token_env must be set when auth_mode = oauth_bearer" .into() } + "api_key_env_unset" => { + "optional_ai_provider enabled but api_key_env environment variable is not set".into() + } + "oauth_token_env_unset" => { + "optional_ai_provider enabled but oauth_token_env environment variable is not set" + .into() + } + "empty_api_key_env_value" => { + "optional_ai_provider enabled but api_key_env environment variable is empty".into() + } + "empty_oauth_token_env_value" => { + "optional_ai_provider enabled but oauth_token_env environment variable is empty".into() + } "missing_base_url" => "[optional_ai_provider].base_url must be set when enabled".into(), "remote_base_url_not_allowed" => { "[optional_ai_provider].base_url is remote; set allow_remote_base_url = true to opt in" @@ -136,8 +164,22 @@ mod tests { assert!(OptionalProviderPlan::from_config(&OptionalAiProviderConfig::default()).is_none()); } + fn with_env(name: &str, value: &str) { + // SAFETY: unit tests run single-threaded and restore/remove keys they set. + unsafe { + std::env::set_var(name, value); + } + } + + fn clear_env(name: &str) { + unsafe { + std::env::remove_var(name); + } + } + #[test] fn remote_provider_requires_explicit_allow_and_budget_fit() { + with_env("GENIE_PROVIDER_KEY", "test-token"); let provider = OptionalAiProviderConfig { enabled: true, provider: OptionalAiProviderKind::OpenAiCompatible, @@ -158,10 +200,12 @@ mod tests { "remote_base_url_not_allowed" ]) ); + clear_env("GENIE_PROVIDER_KEY"); } #[test] fn local_openai_compatible_provider_can_be_ready() { + with_env("LOCAL_PROVIDER_KEY", "test-token"); let provider = OptionalAiProviderConfig { enabled: true, provider: OptionalAiProviderKind::OpenAiCompatible, @@ -179,10 +223,12 @@ mod tests { plan.readiness(&AgentConfig::default()), ProviderReadiness::Ready ); + clear_env("LOCAL_PROVIDER_KEY"); } #[test] fn loopback_127_range_allowed_without_remote_flag() { + with_env("LOCAL_PROVIDER_KEY", "test-token"); let provider = OptionalAiProviderConfig { enabled: true, provider: OptionalAiProviderKind::OpenAiCompatible, @@ -200,10 +246,12 @@ mod tests { plan.readiness(&AgentConfig::default()), ProviderReadiness::Ready ); + clear_env("LOCAL_PROVIDER_KEY"); } #[test] fn loopback_looking_hostname_requires_remote_allow() { + with_env("LOCAL_PROVIDER_KEY", "test-token"); let provider = OptionalAiProviderConfig { enabled: true, provider: OptionalAiProviderKind::OpenAiCompatible, @@ -221,10 +269,12 @@ mod tests { plan.readiness(&AgentConfig::default()), ProviderReadiness::Blocked(vec!["remote_base_url_not_allowed"]) ); + clear_env("LOCAL_PROVIDER_KEY"); } #[test] fn oauth_provider_uses_oauth_token_env_for_readiness() { + with_env("OPENAI_OAUTH_ACCESS_TOKEN", "oauth-token"); let provider = OptionalAiProviderConfig { enabled: true, provider: OptionalAiProviderKind::OpenAi, @@ -243,5 +293,34 @@ mod tests { plan.readiness(&AgentConfig::default()), ProviderReadiness::Ready ); + clear_env("OPENAI_OAUTH_ACCESS_TOKEN"); + } + + #[test] + fn readiness_fails_closed_when_credential_env_disappears() { + let env_name = "GATED_PROVIDER_RUNTIME_CREDS"; + with_env(env_name, "present"); + let provider = OptionalAiProviderConfig { + enabled: true, + provider: OptionalAiProviderKind::OpenAiCompatible, + auth_mode: OptionalAiProviderAuthMode::ApiKey, + base_url: "http://127.0.0.1:11434/v1".into(), + model: "test-model".into(), + api_key_env: env_name.into(), + oauth_token_env: String::new(), + context_window_tokens: 4096, + allow_remote_base_url: false, + }; + let plan = OptionalProviderPlan::from_config(&provider).unwrap(); + assert_eq!( + plan.readiness(&AgentConfig::default()), + ProviderReadiness::Ready + ); + + clear_env(env_name); + assert_eq!( + plan.readiness(&AgentConfig::default()), + ProviderReadiness::Blocked(vec!["api_key_env_unset"]) + ); } } diff --git a/crates/genie-core/tests/provider_config_test.rs b/crates/genie-core/tests/provider_config_test.rs index 6db103a7..1a6d6c31 100644 --- a/crates/genie-core/tests/provider_config_test.rs +++ b/crates/genie-core/tests/provider_config_test.rs @@ -1,10 +1,16 @@ -//! Config-driven LLM provider selection (#568). +//! Config-driven LLM provider selection (#568) and optional OpenAI-compatible +//! wire behavior (#569). + +use std::sync::Arc; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::time::Duration; use genie_common::config::{ - ActiveLlmProviderKind, AgentConfig, Config, OptionalAiProviderAuthMode, - OptionalAiProviderConfig, OptionalAiProviderKind, + ActiveLlmProviderKind, AgentConfig, Config, LlmBackendKind, OptionalAiProviderAuthMode, + OptionalAiProviderConfig, OptionalAiProviderKind, ServiceEndpoint, }; -use genie_core::llm::LlmClient; +use genie_core::llm::{LlmBackendClient, LlmClient, LlmTimeouts, Message, OpenAiCompatibleBackend}; +use tokio::io::{AsyncReadExt, AsyncWriteExt}; fn test_config() -> Config { Config { @@ -24,6 +30,43 @@ fn test_config() -> Config { } } +fn short_timeouts() -> LlmTimeouts { + LlmTimeouts { + connect: Duration::from_secs(2), + read: Duration::from_secs(2), + request: Duration::from_secs(2), + } +} + +async fn spawn_capture_server( + response: String, + accept_count: Arc, +) -> (std::net::SocketAddr, tokio::sync::oneshot::Receiver) { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let (tx, rx) = tokio::sync::oneshot::channel(); + tokio::spawn(async move { + if let Ok((mut conn, _)) = listener.accept().await { + accept_count.fetch_add(1, Ordering::SeqCst); + let mut buf = vec![0u8; 64 * 1024]; + let n = conn.read(&mut buf).await.unwrap_or(0); + let request = String::from_utf8_lossy(&buf[..n]).to_string(); + let _ = tx.send(request); + let _ = conn.write_all(response.as_bytes()).await; + let _ = conn.shutdown().await; + } + }); + (addr, rx) +} + +fn http_json_response(body: &str) -> String { + format!( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", + body.len(), + body + ) +} + #[test] fn from_config_uses_local_service_by_default() { let config = test_config(); @@ -69,3 +112,313 @@ fn from_config_selects_optional_openai_compatible_provider() { std::env::remove_var("PROVIDER_CONFIG_TEST_KEY"); } } + +#[tokio::test] +async fn gate_off_makes_zero_optional_provider_calls() { + let accepts = Arc::new(AtomicUsize::new(0)); + let body = r#"{"choices":[{"message":{"role":"assistant","content":"remote"},"finish_reason":"stop"}]}"#; + let (addr, _rx) = spawn_capture_server(http_json_response(body), Arc::clone(&accepts)).await; + + let mut config = test_config(); + config.optional_ai_provider.enabled = false; + config.optional_ai_provider.base_url = format!("http://{addr}/v1"); + config.services.llm = ServiceEndpoint { + url: "http://127.0.0.1:9/unused".into(), + systemd_unit: String::new(), + backend: LlmBackendKind::GenieAiRuntime, + }; + + // Gate off: from_config must stay on the local service path, and a mock + // completion must not touch the optional-provider listener. + assert_eq!( + config.active_llm_provider_kind(), + ActiveLlmProviderKind::Local + ); + let local = LlmClient::mock(["local reply"]); + let out = local + .chat( + &[Message { + role: "user".into(), + content: "hi".into(), + }], + Some(8), + ) + .await + .unwrap(); + assert_eq!(out, "local reply"); + + // Give the accept task a moment; it must still see zero connections. + tokio::time::sleep(Duration::from_millis(50)).await; + assert_eq!( + accepts.load(Ordering::SeqCst), + 0, + "gate-off must not contact the optional provider listener" + ); +} + +#[tokio::test] +async fn gate_on_sends_auth_model_body_and_path() { + let env_name = "PROVIDER_WIRE_TEST_KEY"; + let secret = "wire-test-secret-token-value"; + unsafe { + std::env::set_var(env_name, secret); + } + + let accepts = Arc::new(AtomicUsize::new(0)); + let body = + r#"{"choices":[{"message":{"role":"assistant","content":"ok"},"finish_reason":"stop"}]}"#; + let (addr, rx) = spawn_capture_server(http_json_response(body), Arc::clone(&accepts)).await; + + let mut config = test_config(); + config.core.llm_connect_timeout_secs = 2; + config.core.llm_read_timeout_secs = 2; + config.core.llm_request_timeout_secs = 2; + config.optional_ai_provider = OptionalAiProviderConfig { + enabled: true, + provider: OptionalAiProviderKind::OpenAiCompatible, + auth_mode: OptionalAiProviderAuthMode::ApiKey, + base_url: format!("http://{addr}/v1"), + model: "test-model".into(), + api_key_env: env_name.into(), + oauth_token_env: String::new(), + context_window_tokens: 4096, + allow_remote_base_url: false, + }; + config.validate_llm_provider().unwrap(); + + let client = LlmClient::from_config(&config).unwrap(); + assert_eq!(client.backend_name(), "openai-compatible"); + + let response = client + .chat( + &[Message { + role: "user".into(), + content: "hello household".into(), + }], + Some(32), + ) + .await + .unwrap(); + assert_eq!(response, "ok"); + assert_eq!(accepts.load(Ordering::SeqCst), 1); + + let request = rx.await.unwrap(); + let request_lower = request.to_ascii_lowercase(); + assert!( + request.contains("POST /v1/chat/completions HTTP/1.1"), + "path missing: {request}" + ); + assert!( + request_lower.contains(&format!("authorization: bearer {secret}")), + "auth missing: {request}" + ); + assert!( + request.contains("\"model\":\"test-model\""), + "model missing: {request}" + ); + assert!( + request.contains("\"content\":\"hello household\""), + "message missing: {request}" + ); + assert!( + request.contains("\"max_tokens\":32"), + "max_tokens missing: {request}" + ); + assert!( + request.contains("\"stream\":false"), + "stream flag missing: {request}" + ); + assert!( + !request.contains("nvext") && !request.contains("conversation_id"), + "generic profile must omit runtime fields: {request}" + ); + + unsafe { + std::env::remove_var(env_name); + } +} + +#[tokio::test] +async fn missing_key_fails_before_connect() { + let env_name = "PROVIDER_MISSING_KEY_WIRE_TEST"; + unsafe { + std::env::remove_var(env_name); + } + + let accepts = Arc::new(AtomicUsize::new(0)); + let body = + r#"{"choices":[{"message":{"role":"assistant","content":"leak"},"finish_reason":"stop"}]}"#; + let (addr, _rx) = spawn_capture_server(http_json_response(body), Arc::clone(&accepts)).await; + + let backend = OpenAiCompatibleBackend::try_new( + &format!("http://{addr}/v1"), + "test-model", + env_name, + short_timeouts(), + ) + .unwrap(); + + let err = backend + .chat_with_format( + &[Message { + role: "user".into(), + content: "hi".into(), + }], + Some(8), + None, + ) + .await + .unwrap_err() + .to_string(); + assert!( + err.contains("not set") || err.contains("misconfigured"), + "expected clear missing-key error, got: {err}" + ); + + tokio::time::sleep(Duration::from_millis(50)).await; + assert_eq!( + accepts.load(Ordering::SeqCst), + 0, + "missing key must fail before connecting" + ); +} + +#[tokio::test] +async fn provider_error_does_not_echo_secret() { + let env_name = "PROVIDER_ERROR_REDACTION_KEY"; + let secret = "sk-proj-should-never-leak-in-errors-1234567890"; + unsafe { + std::env::set_var(env_name, secret); + } + + let accepts = Arc::new(AtomicUsize::new(0)); + let error_body = format!(r#"{{"error":{{"message":"invalid api key {secret} presented"}}}}"#); + let response = format!( + "HTTP/1.1 401 Unauthorized\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", + error_body.len(), + error_body + ); + let (addr, _rx) = spawn_capture_server(response, Arc::clone(&accepts)).await; + + let backend = OpenAiCompatibleBackend::try_new( + &format!("http://{addr}/v1"), + "test-model", + env_name, + short_timeouts(), + ) + .unwrap(); + + let err = backend + .chat_with_format( + &[Message { + role: "user".into(), + content: "hi".into(), + }], + Some(8), + None, + ) + .await + .unwrap_err() + .to_string(); + + assert!(err.contains("401"), "status missing: {err}"); + assert!(!err.contains(secret), "credential leaked into error: {err}"); + assert!( + err.contains("[REDACTED") || err.contains("invalid api key"), + "expected sanitized detail, got: {err}" + ); + assert_eq!(accepts.load(Ordering::SeqCst), 1); + + unsafe { + std::env::remove_var(env_name); + } +} + +#[tokio::test] +async fn streaming_sets_stream_flag_and_delivers_tokens() { + let env_name = "PROVIDER_STREAM_WIRE_TEST_KEY"; + let secret = "stream-secret-token"; + unsafe { + std::env::set_var(env_name, secret); + } + + let accepts = Arc::new(AtomicUsize::new(0)); + let sse = concat!( + "data: {\"choices\":[{\"delta\":{\"content\":\"Hel\"}}]}\n\n", + "data: {\"choices\":[{\"delta\":{\"content\":\"lo\"},\"finish_reason\":\"stop\"}]}\n\n", + "data: [DONE]\n\n", + ); + let response = format!( + "HTTP/1.1 200 OK\r\nContent-Type: text/event-stream\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", + sse.len(), + sse + ); + let (addr, rx) = spawn_capture_server(response, Arc::clone(&accepts)).await; + + let backend = OpenAiCompatibleBackend::try_new( + &format!("http://{addr}/v1"), + "stream-model", + env_name, + short_timeouts(), + ) + .unwrap(); + + let mut tokens = Vec::new(); + let full = backend + .chat_stream( + &[Message { + role: "user".into(), + content: "stream please".into(), + }], + Some(16), + &mut |tok| tokens.push(tok.to_string()), + ) + .await + .unwrap(); + + assert_eq!(full, "Hello"); + assert_eq!(tokens, vec!["Hel".to_string(), "lo".to_string()]); + assert_eq!(accepts.load(Ordering::SeqCst), 1); + + let request = rx.await.unwrap(); + let request_lower = request.to_ascii_lowercase(); + assert!( + request.contains("\"stream\":true"), + "stream flag missing: {request}" + ); + assert!( + request_lower.contains("accept: text/event-stream"), + "SSE accept missing: {request}" + ); + assert!( + request_lower.contains(&format!("authorization: bearer {secret}")), + "auth missing: {request}" + ); + + unsafe { + std::env::remove_var(env_name); + } +} + +#[test] +fn validate_rejects_missing_credential_env_value() { + let env_name = "PROVIDER_VALIDATE_MISSING_KEY"; + unsafe { + std::env::remove_var(env_name); + } + let mut config = test_config(); + config.optional_ai_provider = OptionalAiProviderConfig { + enabled: true, + provider: OptionalAiProviderKind::OpenAiCompatible, + auth_mode: OptionalAiProviderAuthMode::ApiKey, + base_url: "http://127.0.0.1:11434/v1".into(), + model: "test-model".into(), + api_key_env: env_name.into(), + oauth_token_env: String::new(), + context_window_tokens: 4096, + allow_remote_base_url: false, + }; + let err = config.validate_llm_provider().unwrap_err().to_string(); + assert!(err.contains(env_name), "{err}"); + assert!(err.contains("not set") || err.contains("empty"), "{err}"); +} diff --git a/crates/genie-core/tests/provider_test.rs b/crates/genie-core/tests/provider_test.rs index aadd86a8..3e799cfa 100644 --- a/crates/genie-core/tests/provider_test.rs +++ b/crates/genie-core/tests/provider_test.rs @@ -1,4 +1,4 @@ -//! Runtime `Provider` seam (issue #567) and gated optional API completions (#630). +//! Runtime `Provider` seam (issue #567) and gated optional API completions (#630/#569). use genie_common::config::{ ActiveLlmProviderKind, AgentConfig, Config, OptionalAiProviderAuthMode, @@ -78,12 +78,17 @@ async fn gated_provider_gate_off_uses_local_mock_without_optional_plan() { #[tokio::test] async fn gated_provider_gate_on_completes_when_plan_is_ready() { + let env_name = "GATED_PROVIDER_TEST_KEY"; + // SAFETY: single-threaded test; key is unique to this test. + unsafe { + std::env::set_var(env_name, "test-token"); + } let agent = AgentConfig::default(); let plan = OptionalProviderPlan { provider: OptionalAiProviderKind::OpenAiCompatible, auth_mode: OptionalAiProviderAuthMode::ApiKey, base_url: "http://127.0.0.1:11434/v1".into(), - api_key_env: "GATED_PROVIDER_TEST_KEY".into(), + api_key_env: env_name.into(), oauth_token_env: String::new(), context_window_tokens: 4096, remote_allowed: false, @@ -95,6 +100,9 @@ async fn gated_provider_gate_on_completes_when_plan_is_ready() { let out = provider.complete(&[user("hi")], None, None).await.unwrap(); assert_eq!(out, "optional api path"); + unsafe { + std::env::remove_var(env_name); + } } #[tokio::test] @@ -129,6 +137,54 @@ async fn gated_provider_key_missing_returns_clear_error() { assert!(err.contains("api_key_env")); } +#[tokio::test] +async fn gated_provider_fails_closed_when_credential_env_disappears() { + let env_name = "GATED_PROVIDER_DISAPPEARING_KEY"; + unsafe { + std::env::set_var(env_name, "present-at-plan-time"); + } + let agent = AgentConfig::default(); + let plan = OptionalProviderPlan { + provider: OptionalAiProviderKind::OpenAiCompatible, + auth_mode: OptionalAiProviderAuthMode::ApiKey, + base_url: "http://127.0.0.1:11434/v1".into(), + api_key_env: env_name.into(), + oauth_token_env: String::new(), + context_window_tokens: 4096, + remote_allowed: false, + }; + assert_eq!(plan.readiness(&agent), ProviderReadiness::Ready); + + unsafe { + std::env::remove_var(env_name); + } + + let llm = LlmClient::mock(["must not run"]); + let provider = GatedProvider::with_optional_plan(&llm, plan, &agent); + assert!(matches!( + provider.readiness(), + ProviderReadiness::Blocked(_) + )); + + let err = provider + .complete(&[user("hi")], None, None) + .await + .unwrap_err() + .to_string(); + assert!( + err.contains("optional_ai_provider misconfigured"), + "expected clear config error, got: {err}" + ); + assert!( + err.contains("not set") || err.contains("api_key_env"), + "expected missing-credential detail, got: {err}" + ); + assert!( + !err.contains("present-at-plan-time"), + "secret leaked: {err}" + ); +} + #[test] fn gated_provider_for_http_defaults_to_local_gate() { let llm = LlmClient::mock(["unused"]); diff --git a/deploy/config/geniepod.dev.toml b/deploy/config/geniepod.dev.toml index 3e9227c3..8c3069d1 100644 --- a/deploy/config/geniepod.dev.toml +++ b/deploy/config/geniepod.dev.toml @@ -13,9 +13,10 @@ home_runtime_boundary = "transitional_adapter" [optional_ai_provider] enabled = false -provider = "open_ai_compatible" +provider = "open_ai_compatible" # Only open_ai_compatible / open_ai are wired. auth_mode = "api_key" -base_url = "" +base_url = "" # e.g. "http://127.0.0.1:11434/v1" +model = "" # Required when enabled. api_key_env = "GENIEPOD_AI_PROVIDER_API_KEY" oauth_token_env = "GENIEPOD_AI_PROVIDER_OAUTH_TOKEN" context_window_tokens = 4096 diff --git a/deploy/config/geniepod.toml b/deploy/config/geniepod.toml index 4cb35d34..5584b358 100644 --- a/deploy/config/geniepod.toml +++ b/deploy/config/geniepod.toml @@ -22,12 +22,14 @@ home_runtime_boundary = "transitional_adapter" # HA provider today; target is t [optional_ai_provider] enabled = false provider = "open_ai_compatible" # "open_ai_compatible", "open_ai", "anthropic", "gemini", or "custom". + # Only open_ai_compatible / open_ai are wired (HTTP/HTTPS Chat Completions). auth_mode = "api_key" # "api_key" or "oauth_bearer" (OAuth access token in oauth_token_env). -base_url = "" +base_url = "" # e.g. "https://api.openai.com/v1" or "http://127.0.0.1:11434/v1" +model = "" # Required when enabled — sent as the request "model" field. api_key_env = "GENIEPOD_AI_PROVIDER_API_KEY" oauth_token_env = "GENIEPOD_AI_PROVIDER_OAUTH_TOKEN" context_window_tokens = 4096 -allow_remote_base_url = false +allow_remote_base_url = false # Required for non-loopback base_url. Credential is re-checked every request. [core] port = 3000 diff --git a/doc/configuration.md b/doc/configuration.md index 8cddd777..03882c70 100644 --- a/doc/configuration.md +++ b/doc/configuration.md @@ -37,12 +37,21 @@ portability, and transitional validation while preserving the local Jetson not the product runtime and must not become a shortcut around the small-context home harness. +When `enabled = true`, GenieClaw selects this provider instead of +`[services.llm]`. Only `open_ai_compatible` and `open_ai` are wired today; they +speak the OpenAI Chat Completions API over **HTTP or HTTPS**, preserve the +configured base path (for example `/v1` → `/v1/chat/completions`), and send the +configured `model` plus a bearer credential from the environment. Credentials +are resolved on every request and fail closed if the env var is missing or +empty after boot. Provider error bodies are sanitized so secrets are not +echoed back to callers. + | Key | Purpose | | --- | --- | | `enabled` | Turn optional provider planning on | -| `provider` | `open_ai_compatible`, `open_ai`, `anthropic`, `gemini`, or `custom` | +| `provider` | `open_ai_compatible`, `open_ai`, `anthropic`, `gemini`, or `custom` (`anthropic`/`gemini`/`custom` fail loud — not wired) | | `auth_mode` | `api_key` for provider keys or `oauth_bearer` for OAuth access tokens | -| `base_url` | Provider endpoint, for example `https://api.openai.com/v1` | +| `base_url` | Provider endpoint, for example `https://api.openai.com/v1` or `http://127.0.0.1:11434/v1` | | `model` | Model id sent as `model` in outbound requests. Required when enabled — most OpenAI-compatible backends reject the `"default"` placeholder | | `api_key_env` | Env var that stores an API key when `auth_mode = "api_key"` | | `oauth_token_env` | Env var that stores an OAuth access token when `auth_mode = "oauth_bearer"` | @@ -64,6 +73,21 @@ context_window_tokens = 4096 allow_remote_base_url = true ``` +API-key mode against a local OpenAI-compatible server (loopback, no remote +opt-in required): + +```toml +[optional_ai_provider] +enabled = true +provider = "open_ai_compatible" +auth_mode = "api_key" +base_url = "http://127.0.0.1:11434/v1" +model = "llama3.2" +api_key_env = "GENIEPOD_AI_PROVIDER_API_KEY" +context_window_tokens = 4096 +allow_remote_base_url = false +``` + Do not enable this path for household production by default. If a provider is used for validation, keep `context_window_tokens` at or below `[agent].context_window_tokens` and avoid sending household memory unless the