Repository navigation
fix: skip NEAR AI session check when backend is not nearai - #1413
Conversation
When a user configures a non-NEAR AI backend (e.g. Anthropic), the doctor command was incorrectly failing with "session file not found" even though no NEAR AI session is needed. The check now skips with a descriptive message when LLM_BACKEND is not nearai/near_ai/near. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Convert check_nearai_session_skips_for_non_nearai_backend from #[tokio::test] to #[test] with block_on, matching the pattern used by all other ENV_MUTEX tests. Fixes clippy::await_holding_lock error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 fixes a bug in the 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
|
There was a problem hiding this comment.
Pull request overview
This PR fixes ironclaw doctor incorrectly failing the “NEAR AI session” check when the configured LLM backend is not NEAR AI, by skipping the session-file validation unless the resolved backend is nearai.
Changes:
- Pass
Settingsinto the NEAR AI session check and skip it when the resolved backend isn’t NEAR AI. - Add a regression test ensuring the session check returns
Skipfor a non-NEAR-AI backend. - Adjust tests to avoid the clippy
await_holding_lockpattern by usingblock_on()in the env-mutation test.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
The check_nearai_session function in src/cli/doctor.rs was updated to take a Settings argument. When LlmConfig::resolve(settings) fails, the function returns CheckResult::Skip, but the error e is not logged. The reviewer suggests logging the error at the warn level to provide more context for debugging. The EnvGuard struct uses an unsafe block, and the reviewer suggests documenting the safety invariant. The reviewer also suggests ensuring that all tests that modify environment variables use the same mutex to prevent race conditions. Finally, the reviewer suggests that the test check_nearai_session_skips_for_non_nearai_backend creates a new Tokio runtime, which introduces overhead and complexity, and suggests using block_on directly without creating a new runtime, or marking the test as #[tokio::test] if it's intended to be asynchronous.
| // ── NEAR AI session ───────────────────────────────────────── | ||
|
|
||
| async fn check_nearai_session() -> CheckResult { | ||
| async fn check_nearai_session(settings: &Settings) -> CheckResult { |
There was a problem hiding this comment.
| Err(e) => { | ||
| // check_llm_config will report the full error; just skip here. | ||
| return CheckResult::Skip(format!("LLM config error: {e}")); |
There was a problem hiding this comment.
If LlmConfig::resolve(settings) fails, the function returns CheckResult::Skip. However, the error e is not logged. It would be helpful to log the error at warn level to provide more context for debugging, as the skip message only contains a generic "LLM config error". This aligns with the principle of logging warnings when skipping items, as seen in resource cleanup scenarios.
References
- Log errors effectively to track application health in production.
- When calculating the age of a resource for cleanup, if the creation timestamp is unknown or cannot be parsed, treat it as un-reapable by logging a warning and skipping it.
| fn check_nearai_session_skips_for_non_nearai_backend() { | ||
| struct EnvGuard(&'static str, Option<String>); | ||
| impl Drop for EnvGuard { | ||
| fn drop(&mut self) { | ||
| // SAFETY: Under ENV_MUTEX. | ||
| unsafe { | ||
| match &self.1 { | ||
| Some(val) => std::env::set_var(self.0, val), | ||
| None => std::env::remove_var(self.0), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The EnvGuard struct is used to manage environment variables within the test. While this is a common pattern, the unsafe block within the drop function could be concerning. As per repository rules, if unsafe std::env::set_var is used, its safety invariant (e.g., single-threaded context) must be documented in a SAFETY comment. Additionally, ensure that the access to environment variables is properly synchronized to prevent data races. Consider using a more robust library for managing environment variables in tests, if available, to avoid unsafe code.
References
- Ensure proper synchronization to prevent data races when accessing shared resources like environment variables.
- The use of
unsafe std::env::set_varis permissible during application startup if it is guaranteed to be single-threaded (e.g., beforetokio::spawnis called) and this safety invariant is documented in aSAFETYcomment. - The use of
unsafe std::env::set_varis permissible within a single-threaded, sequential context like an interactive setup wizard that runs before any background tasks are spawned. This safety invariant must be documented in aSAFETYcomment.
| let _mutex = crate::config::helpers::ENV_MUTEX.lock().expect("env mutex"); | ||
| let prev = std::env::var("LLM_BACKEND").ok(); | ||
| // SAFETY: Under ENV_MUTEX, no concurrent env access. | ||
| unsafe { | ||
| std::env::set_var("LLM_BACKEND", "anthropic"); | ||
| } | ||
| let _env_guard = EnvGuard("LLM_BACKEND", prev); |
There was a problem hiding this comment.
The test sets the LLM_BACKEND environment variable to "anthropic" and then restores it using EnvGuard. It relies on crate::config::helpers::ENV_MUTEX for synchronization. While this approach works, it's important to ensure that all tests that modify environment variables use the same mutex to prevent race conditions, as tests that modify shared global state should be serialized. Consider creating a helper function or macro to encapsulate this pattern and ensure consistency across all tests.
References
- Ensure proper synchronization to prevent data races when accessing shared resources like environment variables.
- Tests that modify shared global state should be serialized using a mutex to prevent race conditions and flakiness when run in parallel.
| let rt = tokio::runtime::Runtime::new().expect("tokio runtime"); | ||
| let result = rt.block_on(check_nearai_session(&settings)); |
There was a problem hiding this comment.
The test check_nearai_session_skips_for_non_nearai_backend creates a new Tokio runtime using tokio::runtime::Runtime::new(). Since this test is not marked with #[tokio::test], it's running in a synchronous context. Creating a new Tokio runtime within a synchronous test can introduce overhead and complexity. Consider using block_on directly without creating a new runtime, or mark the test as #[tokio::test] if it's intended to be asynchronous.
* fix: skip NEAR AI session check when backend is not nearai When a user configures a non-NEAR AI backend (e.g. Anthropic), the doctor command was incorrectly failing with "session file not found" even though no NEAR AI session is needed. The check now skips with a descriptive message when LLM_BACKEND is not nearai/near_ai/near. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(ci): avoid holding sync MutexGuard across await in doctor test Convert check_nearai_session_skips_for_non_nearai_backend from #[tokio::test] to #[test] with block_on, matching the pattern used by all other ENV_MUTEX tests. Fixes clippy::await_holding_lock error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Kristian Glass <git@doismellburning.co.uk> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix: skip NEAR AI session check when backend is not nearai When a user configures a non-NEAR AI backend (e.g. Anthropic), the doctor command was incorrectly failing with "session file not found" even though no NEAR AI session is needed. The check now skips with a descriptive message when LLM_BACKEND is not nearai/near_ai/near. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(ci): avoid holding sync MutexGuard across await in doctor test Convert check_nearai_session_skips_for_non_nearai_backend from #[tokio::test] to #[test] with block_on, matching the pattern used by all other ENV_MUTEX tests. Fixes clippy::await_holding_lock error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Kristian Glass <git@doismellburning.co.uk> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix: skip NEAR AI session check when backend is not nearai When a user configures a non-NEAR AI backend (e.g. Anthropic), the doctor command was incorrectly failing with "session file not found" even though no NEAR AI session is needed. The check now skips with a descriptive message when LLM_BACKEND is not nearai/near_ai/near. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(ci): avoid holding sync MutexGuard across await in doctor test Convert check_nearai_session_skips_for_non_nearai_backend from #[tokio::test] to #[test] with block_on, matching the pattern used by all other ENV_MUTEX tests. Fixes clippy::await_holding_lock error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Kristian Glass <git@doismellburning.co.uk> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
When a user configures a non-NEAR AI backend (e.g. Anthropic), the
doctor command was incorrectly failing with "session file not found"
even though no NEAR AI session is needed. The check now skips with a
descriptive message when
LLM_BACKENDis not nearai/near_ai/near.Continuation of #1388 (by @doismellburning) with a clippy fix: converted
the test from
#[tokio::test]to#[test]withblock_on()to avoidholding a sync
MutexGuardacross an await point.Change Type
Linked Issue
Supersedes #1388
Validation
cargo fmtcargo clippy --all --benches --tests --examples --all-features— zero warningsSecurity Impact
None
Database Impact
None
Blast Radius
ironclaw doctor— low risk.Rollback Plan
Straightforward git revert
Review track: B
🤖 Generated with Claude Code