fix: persist OpenAI-compatible provider and respect embeddings disable#177
Conversation
#129) Three interrelated bugs caused the agent to ignore user choices made during onboarding when using an OpenAI-compatible LLM provider: 1. Session auth ran before DB config reload, so Config::from_env() defaulted to NearAi and attempted Clerk auth before the real backend was known. Moved session auth to after final config resolution. 2. EmbeddingsConfig::resolve() force-enabled embeddings whenever OPENAI_API_KEY was present, ignoring the user's explicit disable. Changed to respect the stored setting as source of truth. 3. LLM_BACKEND was not saved to the bootstrap .env file, so Config::from_env() always defaulted to NearAi before the DB was connected. Now saves LLM_BACKEND, LLM_BASE_URL, and OLLAMA_BASE_URL alongside the database bootstrap vars. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @ilblackdragon, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses several configuration and persistence issues that caused the application to ignore user-defined settings for OpenAI-compatible LLM providers and embeddings. The changes ensure that user choices made during the onboarding process, particularly regarding LLM backend selection and embeddings enablement, are correctly applied and persisted across sessions, improving the reliability and user experience of the 'ironclaw onboard' flow. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This pull request fixes issue #129 where the agent incorrectly defaulted to NEAR AI backend and force-enabled embeddings despite the user selecting an OpenAI-compatible provider during onboarding. The fix addresses three interrelated bugs in the configuration initialization flow.
Changes:
- Moved session-based authentication to occur after database configuration reload, ensuring the user's actual LLM backend choice is respected rather than early env-only defaults
- Fixed embeddings configuration to respect the user's explicit enabled/disabled choice instead of auto-enabling when OPENAI_API_KEY is present
- Persisted LLM_BACKEND, LLM_BASE_URL, and OLLAMA_BASE_URL to bootstrap .env file alongside database settings, resolving the chicken-and-egg problem of needing these values before database connection
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Moved session.ensure_authenticated() call from before database connection (line 308) to after DB config reload and secrets injection (line 511-517), ensuring authentication uses the user's persisted LLM backend choice |
| src/config.rs | Fixed EmbeddingsConfig::resolve() to respect settings.embeddings.enabled without auto-enabling based on OPENAI_API_KEY presence; added comprehensive tests for embeddings resolution behavior |
| src/setup/wizard.rs | Extended bootstrap .env persistence to include LLM_BACKEND, LLM_BASE_URL, and OLLAMA_BASE_URL alongside existing database bootstrap vars |
| src/setup/README.md | Updated documentation to reflect new bootstrap environment variables and explain why LLM backend must be available before database connection |
| src/settings.rs | Added test verifying OpenAI-compatible settings survive database round-trip with embeddings disabled |
| src/llm/session.rs | Formatting fix for .map_err() closure indentation (no functional change) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[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" | ||
| ); | ||
| } |
There was a problem hiding this comment.
These tests manipulate the same environment variables (EMBEDDING_ENABLED, EMBEDDING_PROVIDER, EMBEDDING_MODEL, OPENAI_API_KEY) and could potentially interfere with each other if run in parallel. While the EnvGuard RAII pattern properly restores variables, there's a race condition window.
Consider marking these tests with #[serial] from the serial_test crate, or use distinct environment variable names for each test (e.g., by using a test-specific prefix). However, since the test suite is reported as passing reliably, this may not be a practical issue yet.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request addresses several issues related to LLM provider persistence and embedding configuration, including moving session authentication to occur after database configuration reload and respecting the user's choice to disable embeddings. While these changes improve the reliability of the onboarding process and configuration management, a critical security vulnerability has been introduced. The setup wizard is susceptible to an environment variable injection vulnerability because it does not sanitize or escape double quotes when saving OpenAI-compatible and Ollama base URLs to the bootstrap .env file. This oversight could allow malicious users to inject arbitrary environment variables. It is recommended to sanitize these inputs or implement a safe .env file generation method that correctly handles escaping.
| unsafe { | ||
| std::env::set_var(key, val); | ||
| } |
There was a problem hiding this comment.
The use of unsafe std::env::set_var must be documented with a SAFETY comment explaining why the usage is safe in this context, as per the general rules. Additionally, be aware that modifying environment variables via set_var is not thread-safe and can cause data races or panics if tests are run in parallel (which is the default for cargo test).
// SAFETY: This is used in a test context to simulate environment variables.
// Note: Tests using this guard should ideally be run sequentially to avoid races.
unsafe {
std::env::set_var(key, val);
}References
- The use of
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.
| 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())); | ||
| } |
There was a problem hiding this comment.
The openai_compatible_base_url and ollama_base_url values are obtained from user input and subsequently passed to save_bootstrap_env, which writes them to a .env file. The save_bootstrap_env function (in src/bootstrap.rs) wraps values in double quotes but does not escape double quotes within the values themselves. This allows an attacker to inject additional environment variables into the .env file by providing a malicious string (e.g., http://localhost"\nINJECTED_VAR="value). These injected variables will be loaded by the application on subsequent starts, potentially allowing for persistent configuration manipulation, such as disabling security features or redirecting network requests.
| provider: "nearai".to_string(), | ||
| reason: format!("DB query failed: {}", e), | ||
| })? { |
There was a problem hiding this comment.
The indentation for the fields within the LlmError::SessionRenewalFailed struct initialization appears to have regressed and is no longer aligned with standard Rust formatting.
| provider: "nearai".to_string(), | |
| reason: format!("DB query failed: {}", e), | |
| })? { | |
| provider: "nearai".to_string(), | |
| reason: format!("DB query failed: {}", e), | |
| })? { |
Address PR review feedback: - Add SAFETY comments to all unsafe env var manipulation in config tests (gemini-code-assist). - Escape backslashes and double quotes in save_bootstrap_env() to prevent env var injection via malicious URLs (gemini-code-assist). - Add test verifying injection attempt is neutralized. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Addressed review feedback in ba09bb5SAFETY comments on unsafe env var manipulation (gemini-code-assist #2820476754): Env var injection via unescaped quotes (gemini-code-assist #2820476756): Env var test parallelism (Copilot #2820473835): session.rs formatting (gemini-code-assist #2820476758): |
…ol schemas) Includes all changes from bigguybobby's PR #138: - Use Chat Completions API for OpenAI-compatible providers (avoids Responses API assumptions like required tool call IDs) - Fall back to settings.selected_model when LLM_MODEL env var is unset - Update OpenAI model list (add gpt-5 family) with priority-based sorting - Add is_openai_chat_model() filter with broader exclusion patterns - Fix http tool: headers schema → array of {name,value}, body → string type, parse_headers_param() accepts both legacy object and array formats - Fix json tool: data schema → string type, parse_json_input() normalizer, validate uses strict string-only check - Add mutex-serialized config tests for env var manipulation - Update NEAR AI config comment for accuracy Co-Authored-By: Bobby (bigguybobby) <bobbymini@proton.me> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…settings) Merge origin/main into fix branch. Resolved conflicts in: - src/main.rs: kept session auth block + new tunnel startup code - src/settings.rs: kept openai_compatible round-trip test + new toml/tunnel tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "type": "array", | ||
| "description": "Optional headers as a list of {name, value} objects", | ||
| "items": { | ||
| "type": "object", | ||
| "properties": { | ||
| "name": { "type": "string" }, | ||
| "value": { "type": "string" } | ||
| }, | ||
| "required": ["name", "value"], | ||
| "additionalProperties": false | ||
| } |
There was a problem hiding this comment.
The schema now declares headers as type "array", but the implementation at lines 114-123 maintains backward compatibility by accepting both objects and arrays. While this provides graceful migration, it creates a mismatch between the schema and actual behavior. The schema should either use "oneOf" to document both accepted formats, or include a comment in the code explaining that legacy object format is still accepted for backward compatibility despite not being in the schema.
| "type": "array", | |
| "description": "Optional headers as a list of {name, value} objects", | |
| "items": { | |
| "type": "object", | |
| "properties": { | |
| "name": { "type": "string" }, | |
| "value": { "type": "string" } | |
| }, | |
| "required": ["name", "value"], | |
| "additionalProperties": false | |
| } | |
| "description": "Optional headers. Prefer a list of {name, value} objects, but a legacy object of {headerName: value} is also accepted.", | |
| "oneOf": [ | |
| { | |
| "type": "array", | |
| "items": { | |
| "type": "object", | |
| "properties": { | |
| "name": { "type": "string" }, | |
| "value": { "type": "string" } | |
| }, | |
| "required": ["name", "value"], | |
| "additionalProperties": false | |
| } | |
| }, | |
| { | |
| "type": "object", | |
| "additionalProperties": { "type": "string" } | |
| } | |
| ] |
| } else { | ||
| let bytes = body_str.as_bytes().to_vec(); | ||
| request = request.body(body_str.to_string()); | ||
| Some(bytes) | ||
| } |
There was a problem hiding this comment.
When body is a string and successfully parsed as JSON, the request is sent with Content-Type application/json via the json() method. However, when it's plain text, the body() method is used which may not set an appropriate Content-Type header. Consider explicitly setting Content-Type headers in both cases, or document that users should include Content-Type in the headers parameter when sending plain text.
| fn parse_json_input(data: &serde_json::Value) -> Result<serde_json::Value, ToolError> { | ||
| let json_str = data | ||
| .as_str() | ||
| .ok_or_else(|| ToolError::InvalidParameters("'data' must be a JSON string".to_string()))?; | ||
| serde_json::from_str(json_str) | ||
| .map_err(|e| ToolError::InvalidParameters(format!("invalid JSON input: {}", e))) | ||
| } | ||
|
|
There was a problem hiding this comment.
This PR includes changes to json.rs and http.rs tool schemas and implementations that are not mentioned in the PR description or linked issue #129. These changes modify the API contract for the json and http tools. Consider splitting these tool improvements into a separate PR to keep the scope focused on fixing the three bugs described in issue #129 (session auth timing, embeddings config, and LLM backend persistence).
Responses to new review commentsHeaders schema/impl mismatch (Copilot #2820626219, http.rs:189): Body Content-Type not set for plain text (Copilot #2820626227, http.rs:252): Tool changes should be separate PR (Copilot #2820626238, json.rs:114): |
ilblackdragon
deleted the
fix/129-openai-compat-provider-not-persisted
branch
nearai#177) * fix: persist OpenAI-compatible provider and respect embeddings disable (nearai#129) Three interrelated bugs caused the agent to ignore user choices made during onboarding when using an OpenAI-compatible LLM provider: 1. Session auth ran before DB config reload, so Config::from_env() defaulted to NearAi and attempted Clerk auth before the real backend was known. Moved session auth to after final config resolution. 2. EmbeddingsConfig::resolve() force-enabled embeddings whenever OPENAI_API_KEY was present, ignoring the user's explicit disable. Changed to respect the stored setting as source of truth. 3. LLM_BACKEND was not saved to the bootstrap .env file, so Config::from_env() always defaulted to NearAi before the DB was connected. Now saves LLM_BACKEND, LLM_BASE_URL, and OLLAMA_BASE_URL alongside the database bootstrap vars. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add SAFETY comments and sanitize .env value escaping Address PR review feedback: - Add SAFETY comments to all unsafe env var manipulation in config tests (gemini-code-assist). - Escape backslashes and double quotes in save_bootstrap_env() to prevent env var injection via malicious URLs (gemini-code-assist). - Add test verifying injection attempt is neutralized. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: incorporate PR nearai#138 changes (chat completions, model sorting, tool schemas) Includes all changes from bigguybobby's PR nearai#138: - Use Chat Completions API for OpenAI-compatible providers (avoids Responses API assumptions like required tool call IDs) - Fall back to settings.selected_model when LLM_MODEL env var is unset - Update OpenAI model list (add gpt-5 family) with priority-based sorting - Add is_openai_chat_model() filter with broader exclusion patterns - Fix http tool: headers schema → array of {name,value}, body → string type, parse_headers_param() accepts both legacy object and array formats - Fix json tool: data schema → string type, parse_json_input() normalizer, validate uses strict string-only check - Add mutex-serialized config tests for env var manipulation - Update NEAR AI config comment for accuracy Co-Authored-By: Bobby (bigguybobby) <bobbymini@proton.me> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Illia Polosukhin <ilblacdragon@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Bobby (bigguybobby) <bobbymini@proton.me>
nearai#177) * fix: persist OpenAI-compatible provider and respect embeddings disable (nearai#129) Three interrelated bugs caused the agent to ignore user choices made during onboarding when using an OpenAI-compatible LLM provider: 1. Session auth ran before DB config reload, so Config::from_env() defaulted to NearAi and attempted Clerk auth before the real backend was known. Moved session auth to after final config resolution. 2. EmbeddingsConfig::resolve() force-enabled embeddings whenever OPENAI_API_KEY was present, ignoring the user's explicit disable. Changed to respect the stored setting as source of truth. 3. LLM_BACKEND was not saved to the bootstrap .env file, so Config::from_env() always defaulted to NearAi before the DB was connected. Now saves LLM_BACKEND, LLM_BASE_URL, and OLLAMA_BASE_URL alongside the database bootstrap vars. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add SAFETY comments and sanitize .env value escaping Address PR review feedback: - Add SAFETY comments to all unsafe env var manipulation in config tests (gemini-code-assist). - Escape backslashes and double quotes in save_bootstrap_env() to prevent env var injection via malicious URLs (gemini-code-assist). - Add test verifying injection attempt is neutralized. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: incorporate PR nearai#138 changes (chat completions, model sorting, tool schemas) Includes all changes from bigguybobby's PR nearai#138: - Use Chat Completions API for OpenAI-compatible providers (avoids Responses API assumptions like required tool call IDs) - Fall back to settings.selected_model when LLM_MODEL env var is unset - Update OpenAI model list (add gpt-5 family) with priority-based sorting - Add is_openai_chat_model() filter with broader exclusion patterns - Fix http tool: headers schema → array of {name,value}, body → string type, parse_headers_param() accepts both legacy object and array formats - Fix json tool: data schema → string type, parse_json_input() normalizer, validate uses strict string-only check - Add mutex-serialized config tests for env var manipulation - Update NEAR AI config comment for accuracy Co-Authored-By: Bobby (bigguybobby) <bobbymini@proton.me> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Illia Polosukhin <ilblacdragon@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Bobby (bigguybobby) <bobbymini@proton.me>
Summary
Fixes #129. Supersedes #138.
When a user selects "OpenAI-compatible" during
ironclaw onboardand disables embeddings, the agent ignored both choices on next startup — defaulting to NEAR AI and force-enabling embeddings via Clerk auth (502 Bad Gateway).Root causes fixed
Config::from_env()defaulted to NearAi and attempted Clerk auth before the real backend was known. Moved session auth to after final config resolution.OPENAI_API_KEYwas present. Changed to respect the stored setting.Config::from_env()always defaulted to NearAi. Now savesLLM_BACKEND,LLM_BASE_URL, andOLLAMA_BASE_URLalongside database bootstrap vars.save_bootstrap_env()didn't escape quotes in values, allowing env var injection. Fixed with proper escaping.Additional improvements (from #138)
.completions_api()to avoid Responses API assumptions (e.g. required tool call IDs)LLM_MODELenv var is unset, falls back to the model chosen during onboardingopenai_model_priority(){name, value}, body gets explicitstringtype.parse_headers_param()accepts both legacy object and new array formats.stringtype.parse_json_input()normalizes input. Validate uses strict string-only check.Test plan
cargo fmt— cleancargo clippy -p ironclaw --all-features --tests— zero warningscargo test— 1179 passed, 0 failedironclaw onboard, select OpenAI-compatible, disable embeddings, restart — verify no NEAR AI auth attempt and embeddings stay disabledCo-Authored-By: Bobby (@bigguybobby) bobbymini@proton.me
🤖 Generated with Claude Code