-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: persist OpenAI-compatible provider and respect embeddings disable #177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
4db79b0
ba09bb5
4d39296
d88fa4b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -597,7 +597,7 @@ impl EmbeddingsConfig { | |
| key: "EMBEDDING_ENABLED".to_string(), | ||
| message: format!("must be 'true' or 'false': {e}"), | ||
| })? | ||
| .unwrap_or_else(|| settings.embeddings.enabled || openai_api_key.is_some()); | ||
| .unwrap_or(settings.embeddings.enabled); | ||
|
|
||
| Ok(Self { | ||
| enabled, | ||
|
|
@@ -1463,3 +1463,92 @@ where | |
| .transpose() | ||
| .map(|opt| opt.unwrap_or(default)) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use crate::settings::Settings; | ||
|
|
||
| /// RAII guard that sets/clears an env var for the duration of a test. | ||
| struct EnvGuard { | ||
| key: &'static str, | ||
| original: Option<String>, | ||
| } | ||
|
|
||
| impl EnvGuard { | ||
| fn set(key: &'static str, val: &str) -> Self { | ||
| let original = std::env::var(key).ok(); | ||
| unsafe { | ||
| std::env::set_var(key, val); | ||
| } | ||
| Self { key, original } | ||
| } | ||
|
|
||
| fn clear(key: &'static str) -> Self { | ||
| let original = std::env::var(key).ok(); | ||
| unsafe { | ||
| std::env::remove_var(key); | ||
| } | ||
| Self { key, original } | ||
| } | ||
| } | ||
|
|
||
| impl Drop for EnvGuard { | ||
| fn drop(&mut self) { | ||
| unsafe { | ||
| if let Some(ref val) = self.original { | ||
| std::env::set_var(self.key, val); | ||
| } else { | ||
| std::env::remove_var(self.key); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn embeddings_resolve_respects_disabled_setting() { | ||
| // Clear env vars so resolve() falls through to settings | ||
| let _g1 = EnvGuard::clear("EMBEDDING_ENABLED"); | ||
| let _g2 = EnvGuard::clear("EMBEDDING_PROVIDER"); | ||
| let _g3 = EnvGuard::clear("EMBEDDING_MODEL"); | ||
| let _g4 = EnvGuard::set("OPENAI_API_KEY", "sk-test-key-present"); | ||
|
|
||
| let settings = Settings { | ||
| embeddings: crate::settings::EmbeddingsSettings { | ||
| enabled: false, | ||
| ..Default::default() | ||
| }, | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let result = EmbeddingsConfig::resolve(&settings).unwrap(); | ||
| // Must respect the user's explicit enabled=false even though OPENAI_API_KEY is set | ||
| assert!( | ||
| !result.enabled, | ||
| "embeddings should be disabled when settings.enabled=false, regardless of OPENAI_API_KEY" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn embeddings_resolve_respects_enabled_setting() { | ||
| let _g1 = EnvGuard::clear("EMBEDDING_ENABLED"); | ||
| let _g2 = EnvGuard::clear("EMBEDDING_PROVIDER"); | ||
| let _g3 = EnvGuard::clear("EMBEDDING_MODEL"); | ||
| let _g4 = EnvGuard::clear("OPENAI_API_KEY"); | ||
|
|
||
| let settings = Settings { | ||
| embeddings: crate::settings::EmbeddingsSettings { | ||
| enabled: true, | ||
| ..Default::default() | ||
| }, | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let result = EmbeddingsConfig::resolve(&settings).unwrap(); | ||
| assert!( | ||
| result.enabled, | ||
| "embeddings should be enabled when settings.enabled=true" | ||
| ); | ||
| } | ||
|
Comment on lines
+1846
to
+1943
|
||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -432,9 +432,9 @@ impl SessionManager { | |||||||||||||
| .get_setting(&user_id, "nearai.session_token") | ||||||||||||||
| .await | ||||||||||||||
| .map_err(|e| LlmError::SessionRenewalFailed { | ||||||||||||||
| provider: "nearai".to_string(), | ||||||||||||||
| reason: format!("DB query failed: {}", e), | ||||||||||||||
| })? { | ||||||||||||||
| provider: "nearai".to_string(), | ||||||||||||||
| reason: format!("DB query failed: {}", e), | ||||||||||||||
| })? { | ||||||||||||||
|
Comment on lines
+435
to
+437
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The indentation for the fields within the
Suggested change
|
||||||||||||||
| value | ||||||||||||||
| } else { | ||||||||||||||
| tracing::warn!( | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1527,6 +1527,18 @@ impl SetupWizard { | |
| env_vars.push(("LIBSQL_URL", url.clone())); | ||
| } | ||
|
|
||
| // LLM bootstrap vars: same chicken-and-egg problem as DATABASE_BACKEND. | ||
| // Config::from_env() needs the backend before the DB is connected. | ||
| if let Some(ref backend) = self.settings.llm_backend { | ||
| env_vars.push(("LLM_BACKEND", backend.clone())); | ||
| } | ||
| if let Some(ref url) = self.settings.openai_compatible_base_url { | ||
| env_vars.push(("LLM_BASE_URL", url.clone())); | ||
| } | ||
| if let Some(ref url) = self.settings.ollama_base_url { | ||
| env_vars.push(("OLLAMA_BASE_URL", url.clone())); | ||
| } | ||
|
Comment on lines
+1538
to
+1543
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
|
||
| if !env_vars.is_empty() { | ||
| let pairs: Vec<(&str, &str)> = | ||
| env_vars.iter().map(|(k, v)| (*k, v.as_str())).collect(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of
unsafe std::env::set_varmust be documented with aSAFETYcomment explaining why the usage is safe in this context, as per the general rules. Additionally, be aware that modifying environment variables viaset_varis not thread-safe and can cause data races or panics if tests are run in parallel (which is the default forcargo test).References
unsafe std::env::set_varis permissible during application startup or within a single-threaded context like an interactive setup wizard, provided this safety invariant is documented in aSAFETYcomment.