Skip to content

fix: skip NEAR AI session check when backend is not nearai #1413

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ilblackdragon merged 2 commits into from
Mar 19, 2026
Merged

fix: skip NEAR AI session check when backend is not nearai #1413

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c7b363d
fix: skip NEAR AI session check when backend is not nearai
doismellburning Mar 18, 2026
8115e79
fix(ci): avoid holding sync MutexGuard across await in doctor test
ilblackdragon Mar 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 59 additions & 3 deletions src/cli/doctor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ pub async fn run_doctor_command() -> anyhow::Result<()> {

check(
"NEAR AI session",
check_nearai_session().await,
check_nearai_session(&settings).await,
&mut passed,
&mut failed,
&mut skipped,
Expand Down Expand Up @@ -215,7 +215,22 @@ fn check_settings_file() -> CheckResult {

// ── NEAR AI session ─────────────────────────────────────────

async fn check_nearai_session() -> CheckResult {
async fn check_nearai_session(settings: &Settings) -> CheckResult {

@gemini-code-assist gemini-code-assist Bot Mar 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The function check_nearai_session is now taking settings: &Settings as an argument. This is good for accessing configuration values, but it's important to ensure that all call sites are updated to pass the settings. Double check to make sure all the tests are passing the settings.

// Skip entirely when the configured backend is not NEAR AI.
let llm_config = match crate::config::LlmConfig::resolve(settings) {
Ok(config) => config,
Err(e) => {
// check_llm_config will report the full error; just skip here.
return CheckResult::Skip(format!("LLM config error: {e}"));
Comment on lines +222 to +224

@gemini-code-assist gemini-code-assist Bot Mar 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. Log errors effectively to track application health in production.
  2. 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.

}
};
if llm_config.backend != "nearai" {
return CheckResult::Skip(format!(
"not using NEAR AI backend (backend={})",
llm_config.backend
));
}

// Check if session file exists
let session_path = crate::config::llm::default_session_path();
if !session_path.exists() {
Expand Down Expand Up @@ -620,12 +635,53 @@ mod tests {

#[tokio::test]
async fn check_nearai_session_does_not_panic() {
let result = check_nearai_session().await;
let settings = Settings::default();
let result = check_nearai_session(&settings).await;
match result {
CheckResult::Pass(_) | CheckResult::Fail(_) | CheckResult::Skip(_) => {}
}
}

#[test]
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),
}
}
}
Comment on lines +646 to +657

@gemini-code-assist gemini-code-assist Bot Mar 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. Ensure proper synchronization to prevent data races when accessing shared resources like environment variables.
  2. The use of unsafe std::env::set_var is permissible during application startup if it is guaranteed to be single-threaded (e.g., before tokio::spawn is called) and this safety invariant is documented in a SAFETY comment.
  3. The use of unsafe std::env::set_var is 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 a SAFETY comment.

}

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);
Comment on lines +660 to +666

@gemini-code-assist gemini-code-assist Bot Mar 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. Ensure proper synchronization to prevent data races when accessing shared resources like environment variables.
  2. Tests that modify shared global state should be serialized using a mutex to prevent race conditions and flakiness when run in parallel.


let settings = Settings::default();
let rt = tokio::runtime::Runtime::new().expect("tokio runtime");
let result = rt.block_on(check_nearai_session(&settings));
Comment on lines +669 to +670

@gemini-code-assist gemini-code-assist Bot Mar 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

match result {
CheckResult::Skip(msg) => {
assert!(
msg.contains("backend=anthropic"),
"expected backend name in skip message, got: {msg}"
);
}
other => panic!(
"expected Skip for non-nearai backend, got: {}",
format_result(&other)
),
}
}

#[test]
fn check_settings_file_handles_missing() {
// Settings::default_path() might or might not exist, but must not panic
Expand Down
Loading