Repository navigation
fix: skip NEAR AI session check when backend is not nearai - #1388
doismellburning wants to merge 1 commit into
Conversation
github-actions
Bot
added
scope: channel/cli
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 addresses 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
|
github-actions
Bot
added
risk: low
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue where the doctor command would incorrectly check for a NEAR AI session when a different backend was configured. The change properly skips this check. My review includes a suggestion to make the backend detection logic more robust by aligning it with the application's main configuration resolution, which considers settings.json in addition to environment variables. I've also suggested an improvement to the new test to make it more robust against panics.
| let backend = std::env::var("LLM_BACKEND") | ||
| .unwrap_or_else(|_| "nearai".into()) | ||
| .to_ascii_lowercase(); | ||
| let is_nearai = matches!(backend.as_str(), "nearai" | "near_ai" | "near"); | ||
| if !is_nearai { | ||
| return CheckResult::Skip(format!("not using NEAR AI backend (backend={backend})")); | ||
| } |
There was a problem hiding this comment.
The current logic for determining the LLM backend only checks the LLM_BACKEND environment variable. This is inconsistent with the main application's configuration resolution, which also considers settings.json. This can lead to incorrect diagnostics, for example, if the backend is configured in settings.json but not as an environment variable.
To ensure the doctor command provides accurate information, it should use the same configuration resolution logic as the rest of the application. You can achieve this by using LlmConfig::resolve. This also simplifies the backend check, as LlmConfig::resolve normalizes backend aliases.
let settings = crate::settings::Settings::load();
let llm_config = match crate::llm::LlmConfig::resolve(&settings) {
Ok(config) => config,
Err(e) => {
// `check_llm_config` will also report this, but we can't proceed.
return CheckResult::Fail(format!("LLM config error: {e}"));
}
};
if llm_config.backend != "nearai" {
return CheckResult::Skip(format!(
"not using NEAR AI backend (backend={})",
llm_config.backend
));
}| async fn check_nearai_session_skips_for_non_nearai_backend() { | ||
| let _guard = crate::config::helpers::ENV_MUTEX.lock().expect("env mutex"); | ||
| // SAFETY: Under ENV_MUTEX, no concurrent env access. | ||
| unsafe { | ||
| std::env::set_var("LLM_BACKEND", "anthropic"); | ||
| } | ||
| let result = check_nearai_session().await; | ||
| // SAFETY: restore env. | ||
| unsafe { | ||
| std::env::remove_var("LLM_BACKEND"); | ||
| } | ||
| 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) | ||
| ), | ||
| } |
There was a problem hiding this comment.
This test correctly validates the new skipping logic. However, if check_nearai_session().await were to panic, the environment variable LLM_BACKEND would not be cleaned up. This could affect subsequent tests that rely on the environment state.
To make the test more robust, you can use a RAII guard to ensure the environment variable is always removed, even in case of a panic.
async fn check_nearai_session_skips_for_non_nearai_backend() {
struct EnvGuard(&'static str);
impl Drop for EnvGuard {
fn drop(&mut self) {
// SAFETY: Under ENV_MUTEX.
unsafe { std::env::remove_var(self.0); }
}
}
let _guard = crate::config::helpers::ENV_MUTEX.lock().expect("env mutex");
// SAFETY: Under ENV_MUTEX, no concurrent env access.
unsafe {
std::env::set_var("LLM_BACKEND", "anthropic");
}
let _env_guard = EnvGuard("LLM_BACKEND");
let result = check_nearai_session().await;
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)
),
}
}There was a problem hiding this comment.
Pull request overview
Adjusts ironclaw doctor to avoid reporting a missing NEAR AI session when the user is configured to use a different LLM backend.
Changes:
- Added a backend gate in the NEAR AI session diagnostic to return
SkipwhenLLM_BACKENDindicates a non-NEAR AI provider. - Added a tokio test asserting the session check skips when
LLM_BACKEND=anthropic.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // Skip entirely when the configured backend is not NEAR AI. | ||
| let backend = std::env::var("LLM_BACKEND") | ||
| .unwrap_or_else(|_| "nearai".into()) | ||
| .to_ascii_lowercase(); | ||
| let is_nearai = matches!(backend.as_str(), "nearai" | "near_ai" | "near"); | ||
| if !is_nearai { | ||
| return CheckResult::Skip(format!("not using NEAR AI backend (backend={backend})")); | ||
| } |
| unsafe { | ||
| std::env::set_var("LLM_BACKEND", "anthropic"); | ||
| } | ||
| let result = check_nearai_session().await; | ||
| // SAFETY: restore env. | ||
| unsafe { | ||
| std::env::remove_var("LLM_BACKEND"); |
| let backend = std::env::var("LLM_BACKEND") | ||
| .unwrap_or_else(|_| "nearai".into()) |
3a25c84 to
5da0d6b
Compare
github-actions
Bot
added
size: M
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>
5da0d6b to
c7b363d
Compare
|
The clippy CI failure is Fix: Convert the test from - #[tokio::test]
- async fn check_nearai_session_skips_for_non_nearai_backend() {
+ #[test]
+ fn check_nearai_session_skips_for_non_nearai_backend() {
struct EnvGuard(&'static str, Option<String>);
// ... (unchanged) ...
let settings = Settings::default();
- let result = check_nearai_session(&settings).await;
+ let rt = tokio::runtime::Runtime::new().expect("tokio runtime");
+ let result = rt.block_on(check_nearai_session(&settings));
match result {I've verified this passes all clippy targets and the test itself still passes. I've pushed this fix to |
|
Closing in favor of a new PR from the main repo branch with the clippy fix included. Thank you @doismellburning for the contribution! |
|
@ilblackdragon Brilliant thanks - didn't get a chance to sit down at a computer to apply the fix Should I target main in future? I went with staging because it was the project default |
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.Change Type
Linked Issue
None
Validation
cargo fmtcargo clippy --all --benches --tests --examples --all-featuresSecurity Impact
None
Database Impact
None
Blast Radius
ironclaw doctor- potential breakage risk should be low.Rollback Plan
Straightforward git revert
Review track: B