refactor: abstract keyring logic to better enable DI #3262

cloud-on-prem · 2025-07-06T09:33:49Z

This change introduces a clean keyring abstraction layer to replace global keyring state with dependency injection, addressing critical runtime errors and improving testability.

The implementation creates a KeyringBackend trait with concrete implementations for system keyring (SystemKeyringBackend) and file-based storage (FileKeyringBackend), allowing the configuration system to use different keyring backends based on the GOOSE_DISABLE_KEYRING environment variable.

This architectural improvement eliminates the need to touch the user's real OS keyring and thus enables comprehensive unit testing without keychain popups. The refactoring maintains full backward compatibility while establishing a foundation for reliable testing and development workflows

feat: introduce a keyring abstraction to help with dep injection
feat: use dep injection for keyring ops
fix: lints and fmts
feat: more just recipes for qol
refactor: reuse keyring abstraction from goose crate
chore: add a lint-all just directive
fix: fix nested runtime issues by using shared future rt
refactor: move file based keyring logic to own module
refactor(tests): use a guard pattern to restore env vars

jsibbison-square · 2025-07-07T00:44:30Z

Justfile

+# Run comprehensive linting checks (includes tests, examples, benchmarks)
+lint-all:
+    @echo "Running comprehensive linting checks..."
+    cargo clippy --workspace --all-targets --all-features -- -D warnings


is there a reason that lint doesn't do all-targets too? I feel like having distinct lint commands means we won't use one of them.

yeah - currently lint-all fails; we should fix them up in another PR. I left it there for future use. Currently just lint is equivalent of what's run in CI

jsibbison-square · 2025-07-07T00:58:19Z

crates/goose-mcp/src/google_drive/storage.rs

+impl From<KeyringError> for StorageError {
+    fn from(err: KeyringError) -> Self {
+        match err {
+            KeyringError::NotFound { .. } => StorageError::NotFound,


Is this losing information that might be useful for debugging, can it be retained when converting to StorageError?

Possibly, although the only thing NotFound contains is NotFound { service: String, username: String }.

jsibbison-square · 2025-07-07T01:06:23Z

crates/goose-mcp/src/google_drive/storage.rs

+        // Read should succeed with mock keyring
        let read_result = manager.read_credentials::<TestCredentials>();
-        assert!(read_result.is_ok(), "Read should succeed with fallback");
+        assert!(read_result.is_ok(), "Read should succeed with mock keyring");


I think this test is still about using the 'fallback_to_disk'=true rather than about using a mock keyring.

jsibbison-square · 2025-07-07T01:11:45Z

crates/goose/src/config/base.rs

+            .expect("goose requires a home dir")
+            .config_dir();
+
+        let keyring: Arc<dyn KeyringBackend> = if env::var("GOOSE_DISABLE_KEYRING").is_ok() {


Is it it worth explicitly checking the value inside the GOOSE_DISABLE_KEYRING rather than just checking the var is present? Most vars can be true or false depending on the value.

yep, worth making it more explicit is a good idea; fixed it.

jsibbison-square · 2025-07-07T01:13:49Z

crates/goose/src/config/base.rs

+            .config_dir();
+
+        let keyring: Arc<dyn KeyringBackend> = if env::var("GOOSE_DISABLE_KEYRING").is_ok() {
+            Arc::new(FileKeyringBackend::new(config_dir.join("secrets.yaml")))


Bonus points if "secrets.yaml" is a constant.

yep good call; added it now

jsibbison-square · 2025-07-07T01:17:33Z

crates/goose/src/config/base.rs

    }

+    #[test]
+    fn test_multiple_secrets_with_mock() -> Result<(), ConfigError> {


Can we remove the test_secret_management_with_mock test and make this cover it all?

jsibbison-square · 2025-07-07T01:19:30Z

crates/goose/src/keyring/file.rs

+    secrets_path: PathBuf,
+}
+
+impl FileKeyringBackend {


Is this backwards compatible with the existing 'secrets.yaml' that was previously used?

good catch; I missed the old secrets.yaml flow here. Adding it now.

jsibbison-square · 2025-07-07T01:20:21Z

crates/goose/src/keyring/file.rs

+        let backend = FileKeyringBackend::new(temp_file.path().to_path_buf());
+
+        // Test setting a password
+        backend.set_password("test_service", "test_user", "test_password")?;


These comments don't add anything compared to just the code on the next line, please remove and check for similar issues throughout.

jsibbison-square · 2025-07-07T01:22:17Z

crates/goose/src/providers/factory.rs

+                // If it succeeds with fake keys, the logic worked
            }
            Err(error) => {
-                // If it fails, it should be due to missing API keys, confirming we tried to create providers


Maybe some of these original comments are useful context?

This comment I thought was more relevant: // Should not fail due to missing secrets, but may fail due to invalid fake keys and // Since we provided fake API keys, the creation should proceed to the provider instantiation
// It may still fail at the provider level, but we should not get keychain-related errors

jsibbison-square · 2025-07-07T01:24:42Z

crates/goose/src/providers/factory.rs

    use chrono::Utc;
    use mcp_core::{content::TextContent, Role};
    use std::env;



At a glance it seems like all the changes in this file are unrelated to the PR focus which is using a new keyring abstraction. Maybe it crept in from some other work and we should keep it separate?

Yep agree - it was an opportunistic refactor while I was removing the need for touching the user's keyring when running tests. Originally, I was setting the ENV in the tests to disable the keyring access. But I could not resist the temptation to use Drop trait :)

jsibbison-square

Left some comments but overall I think this looks ok. Double check the file implementation is backwards compatible. Would appreciate a second set of eyes from someone with history on keychain implementation (maybe @michaelneale ?) but looks ok to me.

lifeizhou-ap · 2025-07-07T10:27:41Z

crates/goose-mcp/src/google_drive/mod.rs


+        // Create the appropriate keyring backend based on environment
+        let keyring: Arc<dyn goose::keyring::KeyringBackend> =
+            if std::env::var("GOOSE_DISABLE_KEYRING").is_ok() {


Since it will create a MockKeyringBackend, just curious about: MockKeyringBackend condition only hit while running test? or any chances thatGOOSE_DISABLE_KEYRING is set when user runs goose?

Good catch; fixed it by using a better abstraction.

michaelneale · 2025-07-10T07:00:55Z

I still dont' quite follow this - was this tested once compiled, and also run as desktop bundle? as it seems a very risky change

michaelneale · 2025-07-10T07:38:55Z

(I did test it with it bundled and signed, seemed ok)

michaelneale · 2025-07-10T07:39:31Z

main concern is that goose-mcp has a goose crate dependency (don't really know what that means in the end, but feels potentially circular)

* 'main' of github.com:block/goose: refactor: abstract keyring logic to better enable DI (#3262) fix: correct tool use for anthropic (#3311)

This reverts commit 7e4a98d.

* main: (51 commits) docs: reflecting benefits of CLI providers (block#3399) feat: fetch openrouter supported models in `goose configure` (block#3347) Add the ability to configure rustyline to use a different edit mode (e.g. vi) (block#2769) docs: update CLI provider guide (block#3397) Streamable HTTP CLI flag (block#3394) docs: Show both remote options for extensions in CLI (block#3392) docs: fix YouTube Transcript MCP package manager (block#3390) docs: simplify alby mcp (block#3379) docs: add max turns (block#3372) feat(cli): add cost estimation per provider for Goose CLI (block#3330) feat: Allow Ollama for non-tool models for chat only (block#3308) [cli] Add --provider and --model CLI options to run command (block#3295) Docs: Lead/worker model in Goose Desktop (block#3342) revert: refactor: abstract keyring logic to better enable DI (block#3358) Drop temporal-service binary (block#3340) docs: add fuzzy search (block#3357) Fix name of GPT-4.1 System Prompt (block#3348) (block#3351) docs: add goose-mobile (block#3315) refactor: abstract keyring logic to better enable DI (block#3262) fix: correct tool use for anthropic (block#3311) ...

Signed-off-by: Adam Tarantino <[email protected]>

Signed-off-by: Soroosh <[email protected]>

Signed-off-by: Kyle Santiago <[email protected]>

cloud-on-prem force-pushed the prem/keyring-test-abstraction branch from ea8f3d6 to 4234f40 Compare July 6, 2025 23:58

jsibbison-square reviewed Jul 7, 2025

View reviewed changes

jsibbison-square approved these changes Jul 7, 2025

View reviewed changes

lifeizhou-ap reviewed Jul 7, 2025

View reviewed changes

angiejones requested a review from michaelneale July 7, 2025 16:58

cloud-on-prem added 13 commits July 9, 2025 20:01

feat: introduce a keyring abstraction to help with dep injection

5b772ac

feat: use dep injection for keyring ops

6cfe494

fix: lints and fmts

7172ff5

feat: more just recipes for qol

b3e8815

refactor: reuse keyring abstraction from goose crate

164c067

chore: add a lint-all just directive

81a4523

fix: fix nested runtime issues by using shared future rt

8fc3a31

refactor: move file based keyring logic to own module

07c8e73

refactor(tests): use a guard pattern to restore env vars

61d4225

fix: ensure old secrets.yaml format still works

076536f

fix: make the env assertion more robust for GOOSE_DISABLE_KEYRING

8a00bf4

refactor: make secrets file a constant

84e2440

wip

838bf76

cloud-on-prem force-pushed the prem/keyring-test-abstraction branch from 1ba4ce8 to 838bf76 Compare July 9, 2025 10:13

cloud-on-prem merged commit 7e4a98d into main Jul 10, 2025
8 checks passed

cloud-on-prem deleted the prem/keyring-test-abstraction branch July 10, 2025 06:41

cloud-on-prem added a commit that referenced this pull request Jul 11, 2025

Revert "refactor: abstract keyring logic to better enable DI (#3262)"

b30b6c8

This reverts commit 7e4a98d.

cloud-on-prem mentioned this pull request Jul 11, 2025

revert: refactor: abstract keyring logic to better enable DI #3358

Merged

ahau-square pushed a commit that referenced this pull request Jul 11, 2025

refactor: abstract keyring logic to better enable DI (#3262)

d388b20

atarantino pushed a commit to atarantino/goose that referenced this pull request Jul 14, 2025

refactor: abstract keyring logic to better enable DI (block#3262)

12ad22e

Signed-off-by: Adam Tarantino <[email protected]>

jamadeo mentioned this pull request Jul 15, 2025

chore(release): release version 1.0.36 #3436

Merged

s-soroosh pushed a commit to s-soroosh/goose that referenced this pull request Jul 18, 2025

refactor: abstract keyring logic to better enable DI (block#3262)

c73a5f1

Signed-off-by: Soroosh <[email protected]>

kwsantiago pushed a commit to kwsantiago/goose that referenced this pull request Jul 19, 2025

refactor: abstract keyring logic to better enable DI (block#3262)

270ebfd

Signed-off-by: Kyle Santiago <[email protected]>

cbruyndoncx pushed a commit to cbruyndoncx/goose that referenced this pull request Jul 20, 2025

refactor: abstract keyring logic to better enable DI (block#3262)

12f3b7c

refactor: abstract keyring logic to better enable DI #3262

refactor: abstract keyring logic to better enable DI #3262

Uh oh!

Conversation

cloud-on-prem commented Jul 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsibbison-square left a comment

Choose a reason for hiding this comment

Uh oh!

lifeizhou-ap Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelneale commented Jul 10, 2025

Uh oh!

michaelneale commented Jul 10, 2025

Uh oh!

michaelneale commented Jul 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lifeizhou-ap Jul 7, 2025 •

edited

Loading