fix(config): respect provider choice and embeddings disabled state

[bigguybobby]

Fixes #129

Summary

Respects user-selected provider and explicit embeddings disable state during config resolution.

Root Cause

EmbeddingsConfig::resolve() used this fallback for enabled:
settings.embeddings.enabled || openai_api_key.is_some()

If OPENAI_API_KEY existed, embeddings could be re-enabled even when disabled by settings.

Changes

• Use settings.embeddings.enabled as the fallback for enabled.
• Keep EMBEDDING_ENABLED env var as explicit override.
• Clarify NEAR AI config comment.
• Add regression tests for issue Bug: v0.4.0 forces NEAR AI/Clerk auth and embeddings despite selecting OpenAI-compatible provider #129 behavior.
• Serialize env-mutating tests with a static mutex to avoid parallel test flakiness.

Validation

• cargo test embeddings_ passes locally, including all new regression tests.

[gemini-code-assist]

Summary of Changes

Hello @bigguybobby, I'm Gemini Code Assist¹! 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 a critical configuration bug where embedding functionality was inadvertently activated due to an environment variable, overriding explicit user preferences. The changes ensure that user-defined settings for embeddings are respected, preventing unintended behavior and improving configuration reliability. Comprehensive tests have been added to validate the corrected logic and prevent future regressions.

Highlights

• Embeddings Configuration Fix: Resolved an issue where the presence of an OPENAI_API_KEY environment variable would force-enable embeddings, even if the user had explicitly disabled them in the settings.
• Logic Correction: Modified the EmbeddingsConfig::resolve() method to correctly prioritize user settings for embedding enablement over the presence of an OpenAI API key.
• Documentation Update: Updated a misleading comment related to NEAR AI configuration resolution for clarity.
• Regression Tests: Added three new regression tests to ensure the fix for issue Bug: v0.4.0 forces NEAR AI/Clerk auth and embeddings despite selecting OpenAI-compatible provider #129 is maintained and that other related embedding configuration scenarios behave as expected.

Changelog

• src/config.rs
  • Updated a comment to clarify the resolution logic for NEAR AI configuration.
  • Modified the EmbeddingsConfig::resolve() method to ensure user-disabled embeddings are not overridden by the presence of an OPENAI_API_KEY.
  • Added three new unit tests to cover scenarios for disabled embeddings, enabled embeddings from settings, and environment variable precedence.

Activity

• The pull request was created to fix issue Bug: v0.4.0 forces NEAR AI/Clerk auth and embeddings despite selecting OpenAI-compatible provider #129, which described an incorrect embedding enablement behavior.
• The author implemented a fix by adjusting the fallback logic in EmbeddingsConfig::resolve().
• Three regression tests were added to validate the fix and related configuration scenarios.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request correctly fixes a bug where the presence of an OPENAI_API_KEY would incorrectly enable embeddings, even when explicitly disabled by the user. The logic change in EmbeddingsConfig::resolve() is direct and effective. The addition of regression tests is excellent for preventing this issue from recurring. I've added one comment regarding the new tests to make them more robust against parallel execution, which could cause flakiness.

[gemini-code-assist]

These tests modify environment variables, which is a global state. When tests are run in parallel (the default for cargo test), this can lead to race conditions and flaky tests. For example, one test might be clearing an environment variable while another test expects it to be set.

To ensure these tests are robust, they should be serialized. A common way to achieve this in Rust is to use a static Mutex to ensure only one test that modifies the environment can run at a time.

Additionally, the unsafe block inside clear_embedding_env is redundant because the function itself is already marked as unsafe.

mod tests {
    use super::*;
    use crate::settings::{EmbeddingsSettings, Settings};
    use std::sync::Mutex;

    // Use a static mutex to serialize tests that modify environment variables.
    static ENV_MUTEX: Mutex<()> = Mutex::new(());

    /// Helper to set/remove env vars in tests. Marked unsafe because it modifies global state.
    unsafe fn clear_embedding_env() {
        std::env::remove_var("EMBEDDING_ENABLED");
        std::env::remove_var("EMBEDDING_PROVIDER");
        std::env::remove_var("EMBEDDING_MODEL");
        std::env::remove_var("OPENAI_API_KEY");
    }

    /// Regression test for #129: when the user disables embeddings in the wizard,
    /// the presence of OPENAI_API_KEY in the environment must not re-enable them.
    #[test]
    fn embeddings_disabled_not_overridden_by_openai_key() {
        let _guard = ENV_MUTEX.lock().unwrap();
        unsafe {
            clear_embedding_env();
            std::env::set_var("OPENAI_API_KEY", "sk-test-key-for-issue-129");
        }

        let settings = Settings {
            embeddings: EmbeddingsSettings {
                enabled: false,
                ..Default::default()
            },
            ..Default::default()
        };

        let config = EmbeddingsConfig::resolve(&settings).expect("resolve should succeed");
        assert!(
            !config.enabled,
            "embeddings should remain disabled when settings.embeddings.enabled=false, \
             even when OPENAI_API_KEY is set (issue #129)"
        );

        unsafe { std::env::remove_var("OPENAI_API_KEY"); }
    }

    /// When the user enables embeddings in settings, it should be enabled.
    #[test]
    fn embeddings_enabled_from_settings() {
        let _guard = ENV_MUTEX.lock().unwrap();
        unsafe { clear_embedding_env(); }

        let settings = Settings {
            embeddings: EmbeddingsSettings {
                enabled: true,
                ..Default::default()
            },
            ..Default::default()
        };

        let config = EmbeddingsConfig::resolve(&settings).expect("resolve should succeed");
        assert!(config.enabled, "embeddings should be enabled when settings say so");
    }

    /// EMBEDDING_ENABLED env var should override settings (explicit user override).
    #[test]
    fn embeddings_env_override_takes_precedence() {
        let _guard = ENV_MUTEX.lock().unwrap();
        unsafe {
            clear_embedding_env();
            std::env::set_var("EMBEDDING_ENABLED", "true");
        }

        let settings = Settings {
            embeddings: EmbeddingsSettings {
                enabled: false,
                ..Default::default()
            },
            ..Default::default()
        };

        let config = EmbeddingsConfig::resolve(&settings).expect("resolve should succeed");
        assert!(config.enabled, "EMBEDDING_ENABLED=true env var should override settings");

        unsafe { std::env::remove_var("EMBEDDING_ENABLED"); }
    }
}

References

1. While rules for unsafe std::env::set_var specifically address application startup or setup wizards, the underlying principle of ensuring single-threaded access to environment variables to prevent race conditions and maintain safety invariants is also critical in test environments, especially when tests run in parallel. The suggested static Mutex aligns with this principle by serializing access.

[bigguybobby]

Addressed in 27b39e8.

Changes made:

• Serialized env-mutating tests with a static Mutex.
• Simplified the env reset helper to avoid redundant unsafe structure.

Validation:

• cargo test embeddings_ passes locally (includes all new regression tests).

[bigguybobby]

Follow-up in fd5d1c1: switched the env-reset helper back to a safe function with an internal unsafe block to avoid unsafe_op_in_unsafe_fn warnings while keeping the test mutex serialization in place.

Re-validated with cargo test embeddings_.

[bigguybobby]

Addressed in follow-up commits:

• 27b39e8 (test(config): serialize env-mutating embeddings tests)
• 50e1ad2 (review: mark clear_embedding_env unsafe, remove verbose doc comments)
• fd5d1c1 (test(config): avoid unsafe-op warnings in env cleanup helper)

The env-mutating tests are now serialized via a static mutex, and the helper structure was updated to avoid unsafe_op_in_unsafe_fn warnings.

[ilblackdragon]

Thanks, I merged it into #177 - added co-authored there