From f86f287033163a673d413a522a9a424432ccfa8a Mon Sep 17 00:00:00 2001 From: savioruz Date: Tue, 25 Aug 2026 11:26:06 +0700 Subject: [PATCH] fix(doctor): validate providers with real test calls, not /models (#48) The doctor diagnostic warned/failed when a provider's configured model name wasn't advertised in GET /models. Proxy gateways (e.g. 9Router) that don't expose /models, or expose different model naming, produced false positives even though the provider was fully functional. Replace the /models-list check with the same real test-call logic the setup/dashboard flows already use: - HttpEmbedderProvider::probe() issues a real POST /embeddings and reports the returned dimension. - HttpLlmProvider::probe() issues a real POST /chat/completions. - doctor now reports success when the real call succeeds, and still fails on 401/5xx/unreachable/parse errors. Regression tests use a mock with no /models endpoint at all (404) but working /embeddings and /chat/completions, covering both raw (embedder) and llm modes, plus a 500 misconfigured-provider case. --- bin/memayu/src/doctor.rs | 121 ++++++++++---------- bin/memayu/tests/doctor.rs | 136 +++++++++++++++++------ crates/memayu-llm-client/src/embedder.rs | 11 ++ crates/memayu-llm-client/src/llm.rs | 16 +++ 4 files changed, 190 insertions(+), 94 deletions(-) diff --git a/bin/memayu/src/doctor.rs b/bin/memayu/src/doctor.rs index a5ad653..cba60ea 100644 --- a/bin/memayu/src/doctor.rs +++ b/bin/memayu/src/doctor.rs @@ -2,11 +2,13 @@ //! //! Runs a battery of non-destructive checks against the effective config: //! config validity, storage reachability/schema, and LLM/embedder connectivity -//! (key validity + model availability). Exits 0 when everything is healthy and -//! 1 when problems are found. +//! (key validity + real test-call reachability). Exits 0 when everything is +//! healthy and 1 when problems are found. //! //! Nothing here mutates state: storage is inspected, not opened-for-write, and -//! providers are probed via `GET /models` rather than issuing completions. +//! providers are probed with a real (non-mutating) completion/embedding call +//! rather than a `GET /models` listing, so proxy gateways that don't expose +//! `/models` are not misreported. use console::style; use memayu_config::{Config, StorageBackend}; @@ -60,13 +62,7 @@ pub async fn cmd_doctor(config: &Config) -> i32 { // ── 3. Providers ── if config.extraction_mode != ExtractionMode::Raw { println!("\n{} Checking LLM provider", section("llm")); - let (passed, warned) = check_provider( - "LLM", - &config.llm.base_url, - config.llm.api_key.as_deref(), - &config.llm.model, - ) - .await; + let (passed, warned) = check_provider("LLM", &config.llm, ProviderKind::Llm).await; ok &= passed; warnings += usize::from(warned); } @@ -86,13 +82,8 @@ pub async fn cmd_doctor(config: &Config) -> i32 { ); println!(" model dir: {}", memayu_config::model_dir().display()); } else { - let (passed, warned) = check_provider( - "embedder", - &config.embedder.base_url, - config.embedder.api_key.as_deref(), - &config.embedder.model, - ) - .await; + let (passed, warned) = + check_provider("embedder", &config.embedder, ProviderKind::Embedder).await; ok &= passed; warnings += usize::from(warned); } @@ -274,56 +265,70 @@ async fn inspect_storage(config: &Config) -> Result { } } -/// Probe a provider's `GET /models` endpoint, print the outcome, and return -/// `(passed, warned)` so the caller can fold it into the overall result. +/// Probe a provider with a real test call (an embedding for the embedder, a +/// completion for the LLM) rather than a `GET /models` listing, print the +/// outcome, and return `(passed, warned)` so the caller can fold it into the +/// overall result. A proxy that never exposes `/models` or renames the model +/// still reports success as long as the real call works (issue #48). async fn check_provider( name: &str, - base_url: &str, - api_key: Option<&str>, - model: &str, + cfg: &memayu_config::ProviderConfig, + kind: ProviderKind, ) -> (bool, bool) { - match memayu_llm_client::check_models(base_url, api_key, model).await { - memayu_llm_client::ModelsCheck::Ok { model_available } => { - if model_available { - println!( - " {} reachable, key accepted, model \"{model}\" advertised", - style("✔").green().bold() - ); - (true, false) - } else { - println!( - " {} reachable, key accepted, but \"{model}\" not in /models list", - style("⚠").yellow().bold() - ); - (true, true) + match kind { + ProviderKind::Llm => { + let provider = memayu_llm_client::HttpLlmProvider::new(cfg.clone()); + match provider.probe().await { + Ok(()) => { + println!( + " {} reachable, key accepted, model \"{}\" answered a test completion", + style("✔").green().bold(), + cfg.model + ); + (true, false) + } + Err(e) => { + println!( + " {} {} failed a test completion: {}", + style("✘").red().bold(), + name, + truncate(e) + ); + (false, false) + } } } - memayu_llm_client::ModelsCheck::Unauthorized => { - println!( - " {} API key rejected (401) for {name}", - style("✘").red().bold() - ); - (false, false) - } - memayu_llm_client::ModelsCheck::Http { status, detail } => { - println!( - " {} {name} returned HTTP {status}: {}", - style("✘").red().bold(), - truncate(detail) - ); - (false, false) - } - memayu_llm_client::ModelsCheck::Unreachable(msg) => { - println!( - " {} cannot reach {name} at {base_url}: {}", - style("✘").red().bold(), - truncate(msg) - ); - (false, false) + ProviderKind::Embedder => { + let provider = memayu_llm_client::HttpEmbedderProvider::new(cfg.clone()); + match provider.probe().await { + Ok(dim) => { + println!( + " {} reachable, key accepted, model \"{}\" returned a {dim}-dim embedding", + style("✔").green().bold(), + cfg.model + ); + (true, false) + } + Err(e) => { + println!( + " {} {} failed a test embedding: {}", + style("✘").red().bold(), + name, + truncate(e) + ); + (false, false) + } + } } } } +/// Which kind of provider [`check_provider`] should exercise. +enum ProviderKind { + Llm, + Embedder, +} + fn truncate(s: String) -> String { const MAX: usize = 160; if s.len() <= MAX { diff --git a/bin/memayu/tests/doctor.rs b/bin/memayu/tests/doctor.rs index a7b38e8..c6da7b1 100644 --- a/bin/memayu/tests/doctor.rs +++ b/bin/memayu/tests/doctor.rs @@ -9,60 +9,88 @@ use std::sync::atomic::{AtomicU16, Ordering}; const EMB: [f32; 3] = [0.1, 0.2, 0.3]; -/// How the mock `/models` endpoint should behave. +/// How the mock provider should answer real test calls. #[derive(Clone, Copy)] -enum ModelsBehavior { - /// 200 with the configured model advertised. +enum ProviderBehavior { + /// `/embeddings` and `/chat/completions` both succeed. There is no + /// `/models` endpoint at all (proxy scenario). Good, - /// 200 but the configured model is NOT advertised. - ModelMissing, - /// 401 (bad API key). + /// 401 on real calls (bad API key). Unauthorized, + /// 500 on real calls (provider misconfigured). + ServerError, } -/// Bind an HTTP mock that answers `/models` (per `behavior`) and `/embeddings` -/// with a fixed embedding. Returns the bound port. -fn start_mock(behavior: ModelsBehavior) -> u16 { +/// Bind an HTTP mock that has NO `/models` endpoint and answers the real test +/// calls (`POST /embeddings`, `POST /chat/completions`) according to +/// `behavior`. Returns the bound port. +fn start_mock(behavior: ProviderBehavior) -> u16 { let listener = TcpListener::bind("127.0.0.1:0").expect("bind mock"); let port = listener.local_addr().unwrap().port(); std::thread::spawn(move || { for stream in listener.incoming() { let Ok(mut stream) = stream else { break }; + // Read the full request: headers plus Content-Length bytes of body. let mut buf = Vec::new(); - let mut tmp = [0u8; 1024]; + let mut tmp = [0u8; 2048]; loop { match stream.read(&mut tmp) { Ok(0) => break, Ok(n) => { buf.extend_from_slice(&tmp[..n]); - if buf.windows(4).any(|w| w == b"\r\n\r\n") { - break; + if let Some(header_end) = buf.windows(4).position(|w| w == b"\r\n\r\n") { + let header = String::from_utf8_lossy(&buf[..header_end]).into_owned(); + let content_length = header + .lines() + .find_map(|line| { + let mut parts = line.splitn(2, ':'); + if parts + .next() + .map(|k| k.trim().eq_ignore_ascii_case("content-length")) + == Some(true) + { + parts.next()?.trim().parse::().ok() + } else { + None + } + }) + .unwrap_or(0); + if buf.len() >= header_end + 4 + content_length { + break; + } } } Err(_) => break, } } + let head = String::from_utf8_lossy(&buf); let (status, body) = if head.starts_with("GET /models") { + // No /models endpoint: the proxy exposes only the real call + // surface. This is exactly what issue #48 exercises. + ("404 Not Found", r#"{"error":"not found"}"#.to_string()) + } else { match behavior { - ModelsBehavior::Good => ( - "200 OK", - r#"{"data":[{"id":"test-embed"},{"id":"other-model"}]}"#.to_string(), - ), - ModelsBehavior::ModelMissing => { - ("200 OK", r#"{"data":[{"id":"other-model"}]}"#.to_string()) + ProviderBehavior::Good => { + if head.starts_with("POST /embeddings") { + ( + "200 OK", + format!( + r#"{{"data":[{{"embedding":[{:.6},{:.6},{:.6}]}}]}}"#, + EMB[0], EMB[1], EMB[2] + ), + ) + } else { + // /chat/completions + ( + "200 OK", + r#"{"choices":[{"message":{"content":"{\"decision\":\"add\",\"memory_id\":null,\"content\":\"ok\"}"}}]}"#.to_string(), + ) + } } - ModelsBehavior::Unauthorized => ("401 Unauthorized", String::new()), + ProviderBehavior::Unauthorized => ("401 Unauthorized", String::new()), + ProviderBehavior::ServerError => ("500 Internal Server Error", String::new()), } - } else { - // /embeddings - ( - "200 OK", - format!( - r#"{{"data":[{{"embedding":[{:.6},{:.6},{:.6}]}}]}}"#, - EMB[0], EMB[1], EMB[2] - ), - ) }; let resp = format!( "HTTP/1.1 {status}\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", @@ -139,7 +167,7 @@ fn prime_schema(port: u16, db: &std::path::Path) { #[test] fn doctor_fresh_machine_exits_zero() { - let port = start_mock(ModelsBehavior::Good); + let port = start_mock(ProviderBehavior::Good); let db = temp_db_path(); // does not exist yet let (code, stdout, _) = run_doctor(port, &db, &[]); assert_eq!(code, 0, "fresh machine should be healthy"); @@ -148,7 +176,7 @@ fn doctor_fresh_machine_exits_zero() { #[test] fn doctor_used_store_exits_zero() { - let port = start_mock(ModelsBehavior::Good); + let port = start_mock(ProviderBehavior::Good); let db = temp_db_path(); prime_schema(port, &db); let (code, stdout, _) = run_doctor(port, &db, &[]); @@ -158,7 +186,7 @@ fn doctor_used_store_exits_zero() { #[test] fn doctor_bad_key_exits_one() { - let port = start_mock(ModelsBehavior::Unauthorized); + let port = start_mock(ProviderBehavior::Unauthorized); let db = temp_db_path(); let (code, stdout, _) = run_doctor(port, &db, &[]); assert_eq!(code, 1, "rejected key must fail doctor"); @@ -166,12 +194,48 @@ fn doctor_bad_key_exits_one() { } #[test] -fn doctor_model_missing_is_warning_but_healthy() { - let port = start_mock(ModelsBehavior::ModelMissing); +fn doctor_proxy_without_models_endpoint_is_healthy() { + // The mock has no /models endpoint at all (a proxy like 9Router), yet the + // real embed test call succeeds. Doctor must report success (issue #48). + let port = start_mock(ProviderBehavior::Good); + let db = temp_db_path(); + let (code, stdout, _) = run_doctor(port, &db, &[]); + assert_eq!(code, 0, "real test call must succeed through a proxy"); + assert!( + stdout.contains("returned a 3-dim embedding"), + "stdout: {stdout}" + ); +} + +#[test] +fn doctor_server_error_exits_one() { + let port = start_mock(ProviderBehavior::ServerError); let db = temp_db_path(); let (code, stdout, _) = run_doctor(port, &db, &[]); - assert_eq!(code, 0, "unadvertised model is only a warning"); - assert!(stdout.contains("not in /models list"), "stdout: {stdout}"); + assert_eq!(code, 1, "misconfigured provider must still fail doctor"); + assert!(stdout.contains("500"), "stdout: {stdout}"); +} + +#[test] +fn doctor_llm_proxy_without_models_endpoint_is_healthy() { + // In llm mode doctor also exercises the LLM with a real completion. The + // mock has no /models endpoint, but both real calls succeed. + let port = start_mock(ProviderBehavior::Good); + let db = temp_db_path(); + let (code, stdout, _) = run_doctor( + port, + &db, + &[ + ("MEMAYU_EXTRACTION_MODE", "llm"), + ("MEMAYU_LLM_BASE_URL", &format!("http://127.0.0.1:{port}")), + ("MEMAYU_LLM_MODEL", "proxy-llm-model"), + ], + ); + assert_eq!(code, 0, "LLM test completion must succeed through a proxy"); + assert!( + stdout.contains("answered a test completion"), + "stdout: {stdout}" + ); } #[test] @@ -185,7 +249,7 @@ fn doctor_missing_config_exits_one() { #[test] fn doctor_dimension_mismatch_fails() { - let port = start_mock(ModelsBehavior::Good); + let port = start_mock(ProviderBehavior::Good); let db = temp_db_path(); prime_schema(port, &db); // stores dimension 3 // Run doctor with a conflicting configured dimension. diff --git a/crates/memayu-llm-client/src/embedder.rs b/crates/memayu-llm-client/src/embedder.rs index c4d55e0..0240d38 100644 --- a/crates/memayu-llm-client/src/embedder.rs +++ b/crates/memayu-llm-client/src/embedder.rs @@ -37,6 +37,17 @@ impl HttpEmbedderProvider { ) .await } + + /// Issue a minimal, real embedding call and return the vector dimension. + /// A success here means the endpoint is reachable, the key is accepted, and + /// the configured model answers — the same test the setup/dashboard flows + /// use (PRD-01 §4.2) — without consulting any `/models` listing. + pub async fn probe(&self) -> Result { + self.embed("dimension probe") + .await + .map(|v| v.len()) + .map_err(|e| e.to_string()) + } } fn retry_after_ms(headers: &reqwest::header::HeaderMap) -> Option { diff --git a/crates/memayu-llm-client/src/llm.rs b/crates/memayu-llm-client/src/llm.rs index 9d37cb8..566b749 100644 --- a/crates/memayu-llm-client/src/llm.rs +++ b/crates/memayu-llm-client/src/llm.rs @@ -36,6 +36,22 @@ impl HttpLlmProvider { ) .await } + + /// Issue a minimal, real completion through the provider and return any + /// error. A success here means the endpoint is reachable, the key is + /// accepted, and the configured model answers — the same signal the real + /// extraction path relies on, without consulting any `/models` listing. + pub async fn probe(&self) -> Result<(), String> { + let messages = [ + memayu_core::Message::user( + "Reply with exactly the JSON: {\"decision\":\"add\",\"memory_id\":null,\"content\":\"ok\"}", + ), + ]; + self.extract(&messages) + .await + .map(|_| ()) + .map_err(|e| e.to_string()) + } } fn retry_after_ms(headers: &reqwest::header::HeaderMap) -> Option {