fix: prevent keychain requests during cargo test #6219
Conversation
|
well worthwhile when running local unit tests for productivity. |
When OPENAI_API_KEY is set via environment variable, Goose still prompted for keychain access because secondary config lookups (e.g. OPENAI_CUSTOM_HEADERS) fell through to keyring. Add get_secrets(primary, maybe_secret) that uses env-only mode when the primary key exists in environment variables. This prevents unnecessary keychain prompts during normal usage and cargo test. Tests updated to use file-based storage or set required env vars. Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt
force-pushed
the
fix/lazy-keyring-eval
branch
from
77602a9 to
c10778b
Compare
| pub const CONFIG_YAML_NAME: &str = "config.yaml"; | ||
|
|
||
| #[cfg(test)] | ||
| const TEST_KEYRING_SERVICE: &str = "goose-test"; |
There was a problem hiding this comment.
FYI, this may look scary, but AFAICT tests never actually used the keyring.
The tests calling Config::new(..., TEST_KEYRING_SERVICE) only exercised set_param/get_param - which read/write YAML config, not secrets. The keyring service parameter was passed but never touched.
So, removing this is mainly to prevent us from accidentally depending on keyring access in tests that don't actually need it.
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where cargo test would trigger keychain access prompts even when OPENAI_API_KEY was set via environment variables. The fix introduces a new get_secrets() method that detects when the primary secret key is in the environment and, if so, bypasses keyring entirely for both primary and secondary keys.
Key Changes
- Added
Config::get_secrets()method that uses env-only mode when the primary key is in environment variables - Updated OpenAI and LiteLLM providers to use the new unified secrets lookup
- Modified tests to use file-based secrets instead of keyring for better isolation
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/goose/src/config/base.rs | Adds get_secrets() method for unified secret lookup with env-only mode; refactors tests to use new_test_config() helper with file-based secrets |
| crates/goose/src/providers/openai.rs | Migrates from separate get_secret() calls to unified get_secrets() approach |
| crates/goose/src/providers/litellm.rs | Migrates from separate get_secret() calls to unified get_secrets() approach |
| crates/goose/src/providers/factory.rs | Sets API keys in environment for tests to prevent keyring access |
| crates/goose/src/config/signup_tetrate/tests.rs | Updates to use new_with_file_secrets() constructor |
| .and_then(|v| Ok(serde_json::from_value(v.clone())?)) | ||
| } | ||
|
|
||
| /// Get secrets. If primary is in env, use env for all keys. Otherwise use secret storage. |
There was a problem hiding this comment.
so - prefer env? (what if you only want it in secrets? is that not needed here?)
There was a problem hiding this comment.
yeah if we want to add an ignore-env mode we'd want to propagate it down here in a future change.
|
|
||
| let config = crate::config::Config::global(); | ||
| let api_key: String = config.get_secret("OPENAI_API_KEY")?; | ||
| let secrets = config.get_secrets("OPENAI_API_KEY", &["OPENAI_CUSTOM_HEADERS"])?; |
There was a problem hiding this comment.
ah I think I get it - we still have get_secret which works as before (required in secrets?) vs get_secrets which will use env if it finds the key there (and then get all other values as well).
There was a problem hiding this comment.
actually I think this is simiar to original get_secret but this is for convenience.. yeah I like it.
|
yeah I think I like this, makes sense. Also will make it easier for goose to operate on itself as won't be unignorable dialogs popping up when you set it on a task. |
|
missed a couple spots.. hopefully these are the last #6231 |
Signed-off-by: Adrian Cole <adrian@tetrate.io>
* main: (155 commits) remove Tool Selection Strategy preview (#6250) fix(cli): correct bash syntax in terminal integration functions (#6181) fix : opening a session to view it modifies session history order in desktop (#6156) test: fix recipe and audio tests to avoid side effects (#6231) chore: Update gemini versions in test_providers.sh (#6246) feat: option to stream json - jsonl really (#6228) feat: add mcp app renderer (#6095) docs: update skills extension to support .agents/skills directories (#6199) Add YouTube short to Chrome DevTools MCP tutorial (#6244) docs: Caveats for privacy information in logs documentation (#6218) move goose issue solver to opus (#6233) feat: improved UX for tool calls via execute_code (#6205) Blog: Code Mode Doesn't Replace MCP (#6227) fix: prevent keychain requests during cargo test (#6219) test: fix test_max_turns_limit slow execution and wrong message type (#6221) Skills vs MCP blog (#6220) Add blog post: Does Your AI Agent Need a Plan? (#6209) fix(ui): enable MCP UI to send a prompt message when an element is clicked (#6207) docs: param option for recipe deeplink/open (#6206) docs: edit in place or fork session (#6203) ...
Signed-off-by: Adrian Cole <adrian@tetrate.io>
Signed-off-by: Adrian Cole <adrian@tetrate.io> Signed-off-by: James Loope <cronus@stolenshoe.com>
2 participants
Summary
When
OPENAI_API_KEYis set via environment variable, Goose still prompted for keychain access because secondary config lookups (e.g.OPENAI_CUSTOM_HEADERS) fell through to keyring.This adds
get_secrets(primary, maybe_secret)that checks if the primary key exists in environment variables. If it does, all lookups use env-only mode, completely bypassing keyring. Secondary keys also fall back toget_param()automatically.Type of Change
AI Assistance
Testing
Verified no keychain access occurs by temporarily adding a panic in the keyring path - all tests still passed.
Related Issues
Relates to #4018