fix(setup): remove redundant LLM config and API keys from bootstrap .env - #1448
Conversation
github-actions
Bot
added
scope: setup
Summary of ChangesHello, 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 enhances the setup process by ensuring that API keys for selected Large Language Model (LLM) providers are persistently stored in the bootstrap .env file. This change guarantees that these critical keys are available immediately upon application startup, even before the main secrets database is fully initialized, thereby improving the robustness and reliability of the system's initial configuration. Highlights
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. Footnotes
|
![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
This pull request persists LLM provider API keys to the ~/.ironclaw/.env file to ensure they are available on restart before the secrets database is connected. The implementation correctly retrieves the API key from the runtime overlay and adds it to the bootstrap environment variables. The documentation in src/setup/README.md has been updated accordingly to reflect this change and to fix some incorrect secret names. My review found one minor issue related to an outdated doc comment in wizard.rs that should be updated to align with the new behavior.
| // Persist the selected provider's API key to bootstrap .env so it's | ||
| // available on next startup before the secrets DB is connected. | ||
| // Uses the same thread-safe overlay that setup_api_key_provider() writes to. | ||
| if let Some(ref backend) = self.settings.llm_backend | ||
| && let Some(def) = registry.find(backend) | ||
| && let Some(ref env_name) = def.api_key_env | ||
| && env_name != "NEARAI_API_KEY" | ||
| && let Some(api_key) = crate::config::helpers::env_or_override(env_name) | ||
| && !api_key.is_empty() | ||
| { | ||
| env_vars.push((env_name.clone(), api_key)); | ||
| } |
There was a problem hiding this comment.
With this change, the function's doc comment is now outdated. It states that **Credentials are NOT written here.**, but this new logic explicitly persists provider API keys to the .env file. Please update the doc comment for write_bootstrap_env to reflect this new behavior and avoid confusion.
There was a problem hiding this comment.
Stale — this code block was removed in a subsequent push. We no longer persist any API keys to bootstrap .env.
There was a problem hiding this comment.
Pull request overview
This PR updates the setup/onboarding flow to persist the selected LLM provider’s API key into the bootstrap ~/.ironclaw/.env, ensuring the key is still available across restarts before the encrypted secrets DB can be opened. It also corrects provider secret-name documentation in the setup README.
Changes:
- Persist the selected registry provider’s
api_key_envvalue (e.g.,OPENAI_API_KEY,ANTHROPIC_API_KEY,LLM_API_KEY) into the bootstrap.envduringwrite_bootstrap_env(). - Fix incorrect secret-name entries for Anthropic/OpenAI in
src/setup/README.md. - Document bootstrap vars as including
SECRETS_MASTER_KEYand the provider API-key env var.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/setup/wizard.rs |
Adds logic to include the selected provider’s API key in the bootstrap .env written by write_bootstrap_env(). |
src/setup/README.md |
Corrects documented secret names for providers and updates the list of bootstrap vars written to ~/.ironclaw/.env. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // available on next startup before the secrets DB is connected. | ||
| // Uses the same thread-safe overlay that setup_api_key_provider() writes to. | ||
| if let Some(ref backend) = self.settings.llm_backend | ||
| && let Some(def) = registry.find(backend) | ||
| && let Some(ref env_name) = def.api_key_env | ||
| && env_name != "NEARAI_API_KEY" | ||
| && let Some(api_key) = crate::config::helpers::env_or_override(env_name) |
There was a problem hiding this comment.
The comment says this reads from the “thread-safe overlay”, but env_or_override() checks real env vars first and only then falls back to runtime overrides / injected vars. Consider rewording to avoid implying it only persists keys set via the overlay.
There was a problem hiding this comment.
Stale — this code block was removed. We no longer persist provider API keys to bootstrap .env at all.
| if let Some(ref backend) = self.settings.llm_backend | ||
| && let Some(def) = registry.find(backend) | ||
| && let Some(ref env_name) = def.api_key_env | ||
| && env_name != "NEARAI_API_KEY" | ||
| && let Some(api_key) = crate::config::helpers::env_or_override(env_name) | ||
| && !api_key.is_empty() | ||
| { | ||
| env_vars.push((env_name.clone(), api_key)); | ||
| } |
There was a problem hiding this comment.
This change introduces new behavior (persisting the selected provider’s API key to the bootstrap .env) but there doesn’t appear to be test coverage for write_bootstrap_env(). Please add a unit/integration test that sets llm_backend to a registry provider with api_key_env, injects the key via the overlay, runs write_bootstrap_env(), and asserts the key is present in the resulting ~/.ironclaw/.env (ideally using a temp base dir or a testable path variant to avoid touching the real home directory).
There was a problem hiding this comment.
Stale — the API key persistence code was removed entirely. The test_model_discovery_picks_up_injected_var test was added for the env_or_override() fix in for_model_discovery().
| - `OLLAMA_BASE_URL` (if ollama) | ||
| - `NEARAI_API_KEY` (if API key auth path) | ||
| - `SECRETS_MASTER_KEY` (if env key source selected in Step 2) | ||
| - Provider API key env var (e.g. `ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, `LLM_API_KEY`) — persisted from the thread-safe overlay when the selected provider has an `api_key_env` |
There was a problem hiding this comment.
The bullet says the provider API key env var is “persisted from the thread-safe overlay”, but the implementation uses env_or_override(), which also reads real env vars first. Please adjust this wording to match actual behavior (e.g., “persisted from env_or_override() / current env scope”) so operators understand when a key will be written.
| - Provider API key env var (e.g. `ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, `LLM_API_KEY`) — persisted from the thread-safe overlay when the selected provider has an `api_key_env` | |
| - Provider API key env var (e.g. `ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, `LLM_API_KEY`) — persisted from `env_or_override()` (current env scope) when the selected provider has an `api_key_env` |
There was a problem hiding this comment.
Stale — this README line was removed along with the API key persistence code.
| // Persist the selected provider's API key to bootstrap .env so it's | ||
| // available on next startup before the secrets DB is connected. | ||
| // Uses the same thread-safe overlay that setup_api_key_provider() writes to. |
There was a problem hiding this comment.
write_bootstrap_env()’s doc comment says credentials are not written here, but this new block persists the selected provider’s API key to ~/.ironclaw/.env. Please update the function-level documentation to reflect the new behavior (and clarify which credentials may be written) so future readers aren’t misled.
| // Persist the selected provider's API key to bootstrap .env so it's | |
| // available on next startup before the secrets DB is connected. | |
| // Uses the same thread-safe overlay that setup_api_key_provider() writes to. | |
| // Persist the selected non-NEAR AI provider's API key to the bootstrap | |
| // .env so it's available on next startup before the secrets DB is | |
| // connected. The API key value is read from the same thread-safe runtime | |
| // env overlay that setup_api_key_provider() writes to, and then written | |
| // out as a regular environment variable for that provider. |
There was a problem hiding this comment.
Stale — this code block was removed. No provider API keys are written to bootstrap .env anymore.
83247bc to
fbb4d83
Compare
github-actions
Bot
added
size: XS
ilblackdragon
changed the title
fbb4d83 to
8b474c0
Compare
github-actions
Bot
added
size: M
ilblackdragon
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// re-resolution in `AppBuilder::build_all()` fills them in after | ||
| /// `inject_llm_keys_from_secrets()` loads from encrypted storage. | ||
| fn write_bootstrap_env(&self) -> Result<(), SetupError> { | ||
| let registry = crate::llm::ProviderRegistry::load(); | ||
| let mut env_vars: Vec<(String, String)> = Vec::new(); | ||
|
|
||
| if let Some(ref backend) = self.settings.database_backend { |
There was a problem hiding this comment.
write_bootstrap_env()'s rustdoc (just above) describes persisting LLM bootstrap settings like LLM_BACKEND, but the function now only writes DB/secrets/onboarding (+ some channel/sandbox) vars. Please align the documentation with the current behavior (or re-add any non-secret LLM bootstrap vars that are still required before DB init).
There was a problem hiding this comment.
Fixed — updated the write_bootstrap_env() docstring to accurately list only DB/secrets/onboard/channel vars. LLM settings are explicitly called out as NOT written here.
| env_vars.push(("LIBSQL_URL".to_string(), 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".to_string(), backend.clone())); | ||
| } | ||
| if let Some(ref url) = self.settings.openai_compatible_base_url { | ||
| env_vars.push(("LLM_BASE_URL".to_string(), url.clone())); | ||
| } | ||
| if let Some(ref url) = self.settings.ollama_base_url { | ||
| env_vars.push(("OLLAMA_BASE_URL".to_string(), url.clone())); | ||
| } | ||
| if let Some(ref region) = self.settings.bedrock_region { | ||
| env_vars.push(("BEDROCK_REGION".to_string(), region.clone())); | ||
| } | ||
| if self.settings.llm_backend.as_deref() == Some("bedrock") { | ||
| if let Some(ref model) = self.settings.selected_model { | ||
| env_vars.push(("BEDROCK_MODEL".to_string(), model.clone())); | ||
| } | ||
| if let Some(ref cross) = self.settings.bedrock_cross_region { | ||
| env_vars.push(("BEDROCK_CROSS_REGION".to_string(), cross.clone())); | ||
| } | ||
| if let Some(ref profile) = self.settings.bedrock_profile { | ||
| env_vars.push(("AWS_PROFILE".to_string(), profile.clone())); | ||
| } | ||
| } | ||
|
|
||
| // Model name: same chicken-and-egg — Config::from_env() resolves the | ||
| // model before the DB is connected, so we must persist it to .env. | ||
| // Write the backend-specific env var so the correct resolution path | ||
| // picks it up (looked up from the provider registry). | ||
| // Bedrock model is already written above as BEDROCK_MODEL, skip here. | ||
| if self.settings.llm_backend.as_deref() != Some("bedrock") | ||
| && let Some(ref model) = self.settings.selected_model | ||
| { | ||
| let backend_str = self.settings.llm_backend.as_deref().unwrap_or("nearai"); | ||
| let model_env = registry.model_env_var(backend_str); | ||
| env_vars.push((model_env.to_string(), model.clone())); | ||
| } | ||
|
|
||
| // Also write provider-specific base URL env var if the provider | ||
| // defines one (e.g., GROQ doesn't need LLM_BASE_URL since its | ||
| // default is compiled in, but it doesn't hurt to be explicit). | ||
| if let Some(ref backend) = self.settings.llm_backend | ||
| && let Some(def) = registry.find(backend) | ||
| && let Some(ref base_url_env) = def.base_url_env | ||
| && let Some(ref base_url) = def.default_base_url | ||
| && base_url_env != "LLM_BASE_URL" | ||
| && base_url_env != "OLLAMA_BASE_URL" | ||
| { | ||
| env_vars.push((base_url_env.clone(), base_url.clone())); | ||
| } | ||
|
|
||
| // Preserve NEARAI_API_KEY if present (set by API key auth flow | ||
| // via the thread-safe runtime env overlay). | ||
| if let Some(api_key) = crate::config::helpers::env_or_override("NEARAI_API_KEY") | ||
| && !api_key.is_empty() | ||
| { | ||
| env_vars.push(("NEARAI_API_KEY".to_string(), api_key)); | ||
| } | ||
|
|
||
| // Secrets master key (env var mode): write to .env so it's available | ||
| // on next startup before the DB is connected. | ||
| if let Some(ref key_hex) = self.settings.secrets_master_key_hex { |
There was a problem hiding this comment.
This change removes all LLM-related bootstrap persistence from write_bootstrap_env() (backend/base URLs/model), not just plaintext API keys. The PR description calls out LLM_BACKEND as a chicken-and-egg var that should still be written to ~/.ironclaw/.env; please clarify the intended behavior and make the implementation + docs consistent.
There was a problem hiding this comment.
Intentional — LLM_BACKEND is NOT a chicken-and-egg var. Config::from_db_with_toml() loads it from DB settings after connection, and LlmConfig::resolve() falls back to settings.llm_backend when the env var is absent. Only DB connection params, SECRETS_MASTER_KEY, and ONBOARD_COMPLETED are true chicken-and-egg vars. Updated the docstring and README to clarify.
| Bootstrap vars written to `~/.ironclaw/.env` (only true chicken-and-egg vars | ||
| that are needed before the DB is connected): | ||
| - `DATABASE_BACKEND` (always) | ||
| - `DATABASE_URL` (if postgres) | ||
| - `LIBSQL_PATH` (if libsql) | ||
| - `LIBSQL_URL` (if turso sync) | ||
| - `LLM_BACKEND` (always, when set) | ||
| - `LLM_BASE_URL` (if openai_compatible) | ||
| - `OLLAMA_BASE_URL` (if ollama) | ||
| - `NEARAI_API_KEY` (if API key auth path) | ||
| - `SECRETS_MASTER_KEY` (if env key source selected in Step 2) | ||
| - `ONBOARD_COMPLETED` (always, "true") |
There was a problem hiding this comment.
The bootstrap var list here appears incomplete compared to what write_bootstrap_env() actually writes (e.g., CLAUDE_CODE_ENABLED and multiple SIGNAL_* vars are also persisted to ~/.ironclaw/.env). Please either document these additional vars or clarify that the list is non-exhaustive.
There was a problem hiding this comment.
Fixed — added CLAUDE_CODE_ENABLED, SIGNAL_HTTP_URL, SIGNAL_ACCOUNT, etc. to the bootstrap vars list with a note that channel init may precede DB.
Only true chicken-and-egg vars belong in ~/.ironclaw/.env — things needed to connect to the DB or decrypt secrets (DATABASE_BACKEND, DATABASE_URL, LIBSQL_PATH, SECRETS_MASTER_KEY, ONBOARD_COMPLETED). LLM settings (LLM_BACKEND, LLM_BASE_URL, OLLAMA_BASE_URL, model name, provider-specific URLs) are persisted to the DB via persist_settings() and loaded by Config::from_db_with_toml() after connection. API keys are stored encrypted in the secrets DB and injected via inject_llm_keys_from_secrets(). Writing them as plaintext to .env was redundant and a security regression. Also fixes for_model_discovery() and build_nearai_model_fetch_config() to use env_or_override() instead of std::env::var(), so they can read NEARAI_API_KEY from the thread-safe overlay during the onboarding wizard (where inject_single_var() sets the key after the user enters it). Also fixes incorrect secret names in README (anthropic_api_key → llm_anthropic_api_key, openai_api_key → llm_openai_api_key). Supersedes #266 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
8b474c0 to
4f183d6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| LLM settings (`LLM_BACKEND`, `LLM_BASE_URL`, model, API keys) are persisted | ||
| to the DB via `persist_settings()` and loaded by `Config::from_db_with_toml()` | ||
| after connection. API keys are stored encrypted in the secrets DB and injected | ||
| via `inject_llm_keys_from_secrets()`. |
There was a problem hiding this comment.
This updated section says bootstrap .env only includes DB/secrets/onboard vars, but earlier in this README there are still example ~/.ironclaw/.env blocks and explanations that include LLM_BACKEND/LLM_BASE_URL (and imply some LLM config must live in Layer 1). Please update those earlier examples/explanations to avoid contradicting the new bootstrap-var policy.
| LLM settings (`LLM_BACKEND`, `LLM_BASE_URL`, model, API keys) are persisted | |
| to the DB via `persist_settings()` and loaded by `Config::from_db_with_toml()` | |
| after connection. API keys are stored encrypted in the secrets DB and injected | |
| via `inject_llm_keys_from_secrets()`. | |
| No LLM-related variables (e.g. `LLM_BACKEND`, `LLM_BASE_URL`, model name, or | |
| API keys) are written to the bootstrap `~/.ironclaw/.env`. LLM settings are | |
| persisted to the DB via `persist_settings()` and loaded by | |
| `Config::from_db_with_toml()` after connection. API keys are stored encrypted | |
| in the secrets DB and injected via `inject_llm_keys_from_secrets()`. Any | |
| earlier example `.env` blocks that show `LLM_*` entries are legacy and should | |
| not be used with the current design. |
There was a problem hiding this comment.
Fixed — updated the Layer 1 .env examples to remove LLM_BACKEND/LLM_BASE_URL/OLLAMA_BASE_URL and show only DB + secrets + onboard vars. Also updated the "Why separate?" explanation.
| |----------|-------------|-------------|---------| | ||
| | NEAR AI Chat | Browser OAuth or session token | - | `NEARAI_SESSION_TOKEN` | | ||
| | NEAR AI Cloud | API key | `llm_nearai_api_key` | `NEARAI_API_KEY` | | ||
| | Anthropic | API key | `anthropic_api_key` | `ANTHROPIC_API_KEY` | | ||
| | OpenAI | API key | `openai_api_key` | `OPENAI_API_KEY` | | ||
| | Anthropic | API key | `llm_anthropic_api_key` | `ANTHROPIC_API_KEY` | | ||
| | OpenAI | API key | `llm_openai_api_key` | `OPENAI_API_KEY` | |
There was a problem hiding this comment.
The provider table now correctly uses secret names like llm_anthropic_api_key/llm_openai_api_key, but later in the README ("Secret Storage") the example still refers to legacy names like anthropic_api_key. Please update that example to use the current secret keys for consistency.
There was a problem hiding this comment.
Fixed — updated the secret name example from anthropic_api_key to llm_anthropic_api_key.
| /// re-resolution in `AppBuilder::build_all()` fills them in after | ||
| /// `inject_llm_keys_from_secrets()` loads from encrypted storage. | ||
| fn write_bootstrap_env(&self) -> Result<(), SetupError> { |
There was a problem hiding this comment.
The rustdoc immediately above this function still claims bootstrap vars include LLM_BACKEND (and other LLM-related chicken-and-egg settings), but this function no longer writes any LLM vars. Please update that rustdoc to reflect the new behavior (bootstrap env is DB/secrets/onboarding only).
There was a problem hiding this comment.
Fixed — the rustdoc now accurately states that only DB/secrets/onboard/channel vars are written, and explicitly says LLM settings and credentials are NOT written here.
Only true chicken-and-egg vars belong in ~/.ironclaw/.env — things needed to connect to the DB or decrypt secrets (DATABASE_BACKEND, DATABASE_URL, LIBSQL_PATH, SECRETS_MASTER_KEY, ONBOARD_COMPLETED). LLM settings (LLM_BACKEND, LLM_BASE_URL, OLLAMA_BASE_URL, model name, provider-specific URLs) are persisted to the DB via persist_settings() and loaded by Config::from_db_with_toml() after connection. API keys are stored encrypted in the secrets DB and injected via inject_llm_keys_from_secrets(). Writing them as plaintext to .env was redundant and a security regression. Also fixes for_model_discovery() and build_nearai_model_fetch_config() to use env_or_override() instead of std::env::var(), so they can read NEARAI_API_KEY from the thread-safe overlay during the onboarding wizard (where inject_single_var() sets the key after the user enters it). Also fixes incorrect secret names in README (anthropic_api_key → llm_anthropic_api_key, openai_api_key → llm_openai_api_key). Supersedes #266 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6a6b78c to
53e1395
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn write_bootstrap_env(&self) -> Result<(), SetupError> { | ||
| let registry = crate::llm::ProviderRegistry::load(); | ||
| let mut env_vars: Vec<(String, String)> = Vec::new(); | ||
|
|
||
| if let Some(ref backend) = self.settings.database_backend { |
There was a problem hiding this comment.
The doc comment immediately above write_bootstrap_env() still states bootstrap vars include LLM_BACKEND ("DATABASE_BACKEND, DATABASE_URL, LLM_BACKEND, etc."), but this function no longer writes any LLM-related vars. Please update that comment to match current behavior so readers don’t assume LLM config still comes from ~/.ironclaw/.env before DB connection.
There was a problem hiding this comment.
Fixed in latest push — docstring updated.
| env_vars.push(("LIBSQL_URL".to_string(), 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".to_string(), backend.clone())); | ||
| } | ||
| if let Some(ref url) = self.settings.openai_compatible_base_url { | ||
| env_vars.push(("LLM_BASE_URL".to_string(), url.clone())); | ||
| } | ||
| if let Some(ref url) = self.settings.ollama_base_url { | ||
| env_vars.push(("OLLAMA_BASE_URL".to_string(), url.clone())); | ||
| } | ||
| if let Some(ref region) = self.settings.bedrock_region { | ||
| env_vars.push(("BEDROCK_REGION".to_string(), region.clone())); | ||
| } | ||
| if self.settings.llm_backend.as_deref() == Some("bedrock") { | ||
| if let Some(ref model) = self.settings.selected_model { | ||
| env_vars.push(("BEDROCK_MODEL".to_string(), model.clone())); | ||
| } | ||
| if let Some(ref cross) = self.settings.bedrock_cross_region { | ||
| env_vars.push(("BEDROCK_CROSS_REGION".to_string(), cross.clone())); | ||
| } | ||
| if let Some(ref profile) = self.settings.bedrock_profile { | ||
| env_vars.push(("AWS_PROFILE".to_string(), profile.clone())); | ||
| } | ||
| } | ||
|
|
||
| // Model name: same chicken-and-egg — Config::from_env() resolves the | ||
| // model before the DB is connected, so we must persist it to .env. | ||
| // Write the backend-specific env var so the correct resolution path | ||
| // picks it up (looked up from the provider registry). | ||
| // Bedrock model is already written above as BEDROCK_MODEL, skip here. | ||
| if self.settings.llm_backend.as_deref() != Some("bedrock") | ||
| && let Some(ref model) = self.settings.selected_model | ||
| { | ||
| let backend_str = self.settings.llm_backend.as_deref().unwrap_or("nearai"); | ||
| let model_env = registry.model_env_var(backend_str); | ||
| env_vars.push((model_env.to_string(), model.clone())); | ||
| } | ||
|
|
||
| // Also write provider-specific base URL env var if the provider | ||
| // defines one (e.g., GROQ doesn't need LLM_BASE_URL since its | ||
| // default is compiled in, but it doesn't hurt to be explicit). | ||
| if let Some(ref backend) = self.settings.llm_backend | ||
| && let Some(def) = registry.find(backend) | ||
| && let Some(ref base_url_env) = def.base_url_env | ||
| && let Some(ref base_url) = def.default_base_url | ||
| && base_url_env != "LLM_BASE_URL" | ||
| && base_url_env != "OLLAMA_BASE_URL" | ||
| { | ||
| env_vars.push((base_url_env.clone(), base_url.clone())); | ||
| } | ||
|
|
||
| // Preserve NEARAI_API_KEY if present (set by API key auth flow | ||
| // via the thread-safe runtime env overlay). | ||
| if let Some(api_key) = crate::config::helpers::env_or_override("NEARAI_API_KEY") | ||
| && !api_key.is_empty() | ||
| { | ||
| env_vars.push(("NEARAI_API_KEY".to_string(), api_key)); | ||
| } | ||
|
|
||
| // Secrets master key (env var mode): write to .env so it's available | ||
| // on next startup before the DB is connected. | ||
| if let Some(ref key_hex) = self.settings.secrets_master_key_hex { |
There was a problem hiding this comment.
Because write_bootstrap_env() uses upsert_bootstrap_vars() (which preserves unknown keys in the existing ~/.ironclaw/.env), simply omitting LLM vars from env_vars will NOT remove previously-written entries like NEARAI_API_KEY, LLM_BACKEND, or LLM_BASE_URL from existing installations. This can leave plaintext API keys/config lingering in the bootstrap file even after re-running the wizard. Consider explicitly pruning known-deprecated keys from the file (or writing empty values / switching to a write-mode that rewrites only the allowed bootstrap keys) so the on-disk .env actually matches the “only chicken-and-egg vars” invariant.
There was a problem hiding this comment.
Valid point — upsert_bootstrap_vars() preserves unknown keys, so previously-written NEARAI_API_KEY/LLM_BACKEND entries will linger in existing installations. This is low-risk (they become redundant overrides, not incorrect), but a cleanup pass could be added as a follow-up. The lingering NEARAI_API_KEY is the main concern — it's a plaintext API key that should ideally be pruned. Filed as a TODO for a follow-up PR.
| crate::config::inject_single_var("NEARAI_API_KEY", "injected-wizard-key"); | ||
| let config = build_nearai_model_fetch_config(); | ||
|
|
||
| // Clean up | ||
| crate::config::inject_single_var("NEARAI_API_KEY", ""); |
There was a problem hiding this comment.
This test mutates the global INJECTED_VARS overlay via inject_single_var(), but the cleanup sets the key to an empty string rather than removing/restoring it. Because other config readers (notably optional_env()) don’t consistently treat empty injected values as unset, leaving NEARAI_API_KEY present-but-empty in INJECTED_VARS can leak state across tests and cause order-dependent failures. Please restore the previous injected value or add a test-only helper to remove a key from INJECTED_VARS (or adjust optional_env() to filter out empty injected values) so this test can’t pollute global state.
| crate::config::inject_single_var("NEARAI_API_KEY", "injected-wizard-key"); | |
| let config = build_nearai_model_fetch_config(); | |
| // Clean up | |
| crate::config::inject_single_var("NEARAI_API_KEY", ""); | |
| // Capture any previously configured API key (from env or injected overlay) | |
| let previous_injected_api_key = crate::config::optional_env("NEARAI_API_KEY"); | |
| crate::config::inject_single_var("NEARAI_API_KEY", "injected-wizard-key"); | |
| let config = build_nearai_model_fetch_config(); | |
| // Clean up: restore the previous value if there was one, rather than | |
| // forcing an empty string into the injected overlay. | |
| if let Some(prev_key) = previous_injected_api_key { | |
| crate::config::inject_single_var("NEARAI_API_KEY", &prev_key); | |
| } |
There was a problem hiding this comment.
Added a comment explaining the cleanup. env_or_override() explicitly filters empty values at every layer (real env line 54, runtime overrides line 64, INJECTED_VARS line 75 of helpers.rs), so setting to empty string is functionally equivalent to removal. No state leaks across tests since the ENV_MUTEX serializes all env-touching tests.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
the
scope: agent
- Update write_bootstrap_env() docstring to reflect current behavior (no LLM vars, no credentials) - Fix Layer 1 .env examples in README to remove LLM_BACKEND/LLM_BASE_URL - Fix legacy secret name in README example (anthropic_api_key → llm_anthropic_api_key) - Document channel/sandbox vars in bootstrap vars list - Add cleanup comment in test explaining empty-value-as-unset behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Clean up: empty values are treated as unset by env_or_override() | ||
| // at every layer (real env, runtime overrides, INJECTED_VARS). | ||
| crate::config::inject_single_var("NEARAI_API_KEY", ""); | ||
|
|
There was a problem hiding this comment.
Test cleanup sets NEARAI_API_KEY in the injected-vars overlay to an empty string via inject_single_var("NEARAI_API_KEY", ""). Unlike env_or_override(), optional_env() does not currently filter empty values for INJECTED_VARS, so this may leave global state that other tests/config resolution can observe as Some("") (and it can also race with parallel tests since ENV_MUTEX only guards process env). Prefer restoring the previous injected value, or provide a way to actually remove the key from the injected overlay (e.g., a test-only clear helper or making inject_single_var delete the entry when value is empty).
| // Clean up: empty values are treated as unset by env_or_override() | |
| // at every layer (real env, runtime overrides, INJECTED_VARS). | |
| crate::config::inject_single_var("NEARAI_API_KEY", ""); |
| ```env | ||
| LLM_BACKEND="ollama" | ||
| OLLAMA_BASE_URL="http://localhost:11434" | ||
| SECRETS_MASTER_KEY="..." |
There was a problem hiding this comment.
In the PostgreSQL bootstrap .env example, SECRETS_MASTER_KEY is shown without the “only if env key source selected” caveat that the libsql example includes. Since the master key may come from keychain mode (and therefore not be present in .env), this example can be read as implying it’s always required. Consider adding the same conditional note here for consistency and to avoid confusing operators.
| SECRETS_MASTER_KEY="..." | |
| SECRETS_MASTER_KEY="..." # only if env key source selected |
| - `ONBOARD_COMPLETED` (always, "true") | ||
| - Channel/sandbox vars: `CLAUDE_CODE_ENABLED`, `SIGNAL_HTTP_URL`, `SIGNAL_ACCOUNT`, etc. (channel init may precede DB) | ||
|
|
||
| LLM settings (`LLM_BACKEND`, `LLM_BASE_URL`, model, API keys) are persisted |
There was a problem hiding this comment.
This paragraph says “LLM settings (LLM_BACKEND, LLM_BASE_URL, model, API keys) are persisted to the DB via persist_settings()…”, but API keys are not persisted via persist_settings() (they’re stored in the encrypted secrets DB). To avoid a misleading statement, consider removing “API keys” from the parenthetical list (or explicitly separating “settings persisted via persist_settings” from “credentials stored as secrets”).
| LLM settings (`LLM_BACKEND`, `LLM_BASE_URL`, model, API keys) are persisted | |
| LLM settings (`LLM_BACKEND`, `LLM_BASE_URL`, model) are persisted |
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Looks good -- correctly removes redundant plaintext LLM config from bootstrap .env, fixes the env_or_override() bug for model discovery during wizard, and adds a regression test. CI is green. Approving.
…env (nearai#1448) * fix(setup): remove redundant LLM vars and API keys from bootstrap .env Only true chicken-and-egg vars belong in ~/.ironclaw/.env — things needed to connect to the DB or decrypt secrets (DATABASE_BACKEND, DATABASE_URL, LIBSQL_PATH, SECRETS_MASTER_KEY, ONBOARD_COMPLETED). LLM settings (LLM_BACKEND, LLM_BASE_URL, OLLAMA_BASE_URL, model name, provider-specific URLs) are persisted to the DB via persist_settings() and loaded by Config::from_db_with_toml() after connection. API keys are stored encrypted in the secrets DB and injected via inject_llm_keys_from_secrets(). Writing them as plaintext to .env was redundant and a security regression. Also fixes for_model_discovery() and build_nearai_model_fetch_config() to use env_or_override() instead of std::env::var(), so they can read NEARAI_API_KEY from the thread-safe overlay during the onboarding wizard (where inject_single_var() sets the key after the user enters it). Also fixes incorrect secret names in README (anthropic_api_key → llm_anthropic_api_key, openai_api_key → llm_openai_api_key). Supersedes nearai#266 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: add missing fallback_deliverable field to job_monitor tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: address review comments on bootstrap .env and README - Update write_bootstrap_env() docstring to reflect current behavior (no LLM vars, no credentials) - Fix Layer 1 .env examples in README to remove LLM_BACKEND/LLM_BASE_URL - Fix legacy secret name in README example (anthropic_api_key → llm_anthropic_api_key) - Document channel/sandbox vars in bootstrap vars list - Add cleanup comment in test explaining empty-value-as-unset behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…env (nearai#1448) * fix(setup): remove redundant LLM vars and API keys from bootstrap .env Only true chicken-and-egg vars belong in ~/.ironclaw/.env — things needed to connect to the DB or decrypt secrets (DATABASE_BACKEND, DATABASE_URL, LIBSQL_PATH, SECRETS_MASTER_KEY, ONBOARD_COMPLETED). LLM settings (LLM_BACKEND, LLM_BASE_URL, OLLAMA_BASE_URL, model name, provider-specific URLs) are persisted to the DB via persist_settings() and loaded by Config::from_db_with_toml() after connection. API keys are stored encrypted in the secrets DB and injected via inject_llm_keys_from_secrets(). Writing them as plaintext to .env was redundant and a security regression. Also fixes for_model_discovery() and build_nearai_model_fetch_config() to use env_or_override() instead of std::env::var(), so they can read NEARAI_API_KEY from the thread-safe overlay during the onboarding wizard (where inject_single_var() sets the key after the user enters it). Also fixes incorrect secret names in README (anthropic_api_key → llm_anthropic_api_key, openai_api_key → llm_openai_api_key). Supersedes nearai#266 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: add missing fallback_deliverable field to job_monitor tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: address review comments on bootstrap .env and README - Update write_bootstrap_env() docstring to reflect current behavior (no LLM vars, no credentials) - Fix Layer 1 .env examples in README to remove LLM_BACKEND/LLM_BASE_URL - Fix legacy secret name in README example (anthropic_api_key → llm_anthropic_api_key) - Document channel/sandbox vars in bootstrap vars list - Add cleanup comment in test explaining empty-value-as-unset behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…env (nearai#1448) * fix(setup): remove redundant LLM vars and API keys from bootstrap .env Only true chicken-and-egg vars belong in ~/.ironclaw/.env — things needed to connect to the DB or decrypt secrets (DATABASE_BACKEND, DATABASE_URL, LIBSQL_PATH, SECRETS_MASTER_KEY, ONBOARD_COMPLETED). LLM settings (LLM_BACKEND, LLM_BASE_URL, OLLAMA_BASE_URL, model name, provider-specific URLs) are persisted to the DB via persist_settings() and loaded by Config::from_db_with_toml() after connection. API keys are stored encrypted in the secrets DB and injected via inject_llm_keys_from_secrets(). Writing them as plaintext to .env was redundant and a security regression. Also fixes for_model_discovery() and build_nearai_model_fetch_config() to use env_or_override() instead of std::env::var(), so they can read NEARAI_API_KEY from the thread-safe overlay during the onboarding wizard (where inject_single_var() sets the key after the user enters it). Also fixes incorrect secret names in README (anthropic_api_key → llm_anthropic_api_key, openai_api_key → llm_openai_api_key). Supersedes nearai#266 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: add missing fallback_deliverable field to job_monitor tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: address review comments on bootstrap .env and README - Update write_bootstrap_env() docstring to reflect current behavior (no LLM vars, no credentials) - Fix Layer 1 .env examples in README to remove LLM_BACKEND/LLM_BASE_URL - Fix legacy secret name in README example (anthropic_api_key → llm_anthropic_api_key) - Document channel/sandbox vars in bootstrap vars list - Add cleanup comment in test explaining empty-value-as-unset behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Supersedes #266.
Investigation revealed that
~/.ironclaw/.envshould only contain true chicken-and-egg vars — things needed before the DB connects or to decrypt secrets. Everything else is redundant:LLM_BACKEND,LLM_BASE_URL,OLLAMA_BASE_URL, model name, Bedrock config, provider-specific URLs) are persisted to the DB viapersist_settings()and loaded byConfig::from_db_with_toml()after connectionNEARAI_API_KEY,ANTHROPIC_API_KEY, etc.) are stored encrypted in the secrets DB and injected viainject_llm_keys_from_secrets()— writing them as plaintext to.envwas redundant and a security regressioninject_llm_keys_from_secrets()skips injection whenstd::env::var(env_var)is already set (line 416-417 of config/mod.rs), so users who setNEARAI_API_KEYin their shell profile are unaffectedWhat stays in bootstrap
.envDATABASE_BACKENDDATABASE_URL/LIBSQL_PATH/LIBSQL_URLSECRETS_MASTER_KEYONBOARD_COMPLETEDChanges
LLM_BACKEND,LLM_BASE_URL,OLLAMA_BASE_URL, Bedrock vars, model name, provider-specific base URLs,NEARAI_API_KEYfromwrite_bootstrap_env()(-61 lines)NearAiConfig::for_model_discovery()andbuild_nearai_model_fetch_config()to useenv_or_override()instead ofstd::env::var()— these were reading API keys from the real process env, missing keys set viainject_single_var()during the onboarding wizard. This caused model fetching to fail after entering an API key during setup.anthropic_api_key→llm_anthropic_api_key,openai_api_key→llm_openai_api_key)What about PR #266?
Everything proposed in #266 already landed on
staging:.envloading inmain.rs(before command dispatch) ✅SECRETS_MASTER_KEYpersistence viasettings.secrets_master_key_hex✅inject_single_var()instead ofunsafe set_var✅Test plan
cargo clippy --all --benches --tests --examples --all-features— zero warningscargo test --lib setup— 63 tests passcargo test --lib build_nearai_model_fetch— 3 tests pass (API key overlay resolution)~/.ironclaw/.env🤖 Generated with Claude Code