diff --git a/crates/openhuman-core/src/search/tools.rs b/crates/openhuman-core/src/search/tools.rs index c237144fb50..38cd3d940c4 100644 --- a/crates/openhuman-core/src/search/tools.rs +++ b/crates/openhuman-core/src/search/tools.rs @@ -6,7 +6,9 @@ //! through `modules::search::execute_tool`, so a provider or login change is //! honoured on the next call without rebuilding the session. -use std::sync::Arc; +use std::collections::hash_map::DefaultHasher; +use std::hash::{Hash, Hasher}; +use std::sync::{Arc, Mutex}; use async_trait::async_trait; use serde_json::Value; @@ -17,12 +19,35 @@ use crate::config::Config; /// One TinySearch tool (a role tool such as `web_search_tool`, or a provider /// tool in `all_tools` presentation). +/// What a call reports once every provider has already answered +/// "unavailable" in this session. It tells the model to stop rather than to +/// wait, because nothing about the session will change the answer (#6991). +pub const SEARCH_EXHAUSTED_MESSAGE: &str = + "Web search is not available with this setup: no provider could answer. Do not call \ + it again \u{2014} answer from the material you already have."; + pub struct TinySearchTool { spec: ToolSpec, /// Spawn-time config. `None` for a deferred instance rebuilt from a /// recorded transcript, which resolves the live config per call. config: Option>, exposure: ToolExposure, + /// The provider configuration a call last found nothing usable under. + /// + /// A credential alone makes the managed route look reachable + /// (`providers::backend_credential_available`), so a deployment that is + /// offline, firewalled, out of balance or holding a dead key offers this + /// tool on every turn and fails every call. The agent then spends turns on + /// a tool that cannot work, at the moment it is least sure what to do. + /// + /// Keyed by configuration rather than latched outright, because this + /// module's contract is that every call re-reads the live config so a + /// provider or login change is honoured without rebuilding the session. A + /// call whose signature differs from the recorded one tries again; only a + /// repeat under the same configuration is refused. The signature is this + /// tool's own view — its role order and the resolved providers — so one + /// tool's dead providers never answer for another's. + exhausted_for: Mutex>, } impl TinySearchTool { @@ -31,6 +56,7 @@ impl TinySearchTool { spec, config: Some(config), exposure: ToolExposure::Direct, + exhausted_for: Mutex::new(None), } } @@ -41,7 +67,46 @@ impl TinySearchTool { spec, config: None, exposure: ToolExposure::Direct, + exhausted_for: Mutex::new(None), + } + } + + /// Whether this tool already found nothing usable under `signature`. + pub(crate) fn is_exhausted_for(&self, signature: u64) -> bool { + self.exhausted_for + .lock() + .map(|recorded| *recorded == Some(signature)) + .unwrap_or(false) + } + + /// Record that no provider could answer under `signature`. Replaces any + /// earlier one, so the refusal always describes the current configuration. + pub(crate) fn mark_exhausted_for(&self, signature: u64) { + if let Ok(mut recorded) = self.exhausted_for.lock() { + *recorded = Some(signature); + } + } + + /// What this tool's providers look like right now: the order its role + /// draws from, and every provider's resolved reachability. Two calls agree + /// only while nothing a user could change — a key, a route, a provider + /// selection, a login — has moved. + pub(crate) fn provider_signature(&self, config: &Config) -> u64 { + let mut hasher = DefaultHasher::new(); + if let Some(role) = tinysearch_bus::role_for_tool(&self.spec.name) { + for provider in super::providers::role_order(config, role) { + provider.hash(&mut hasher); + } } + for provider in super::providers::resolve(config) { + provider.id.hash(&mut hasher); + provider.enabled.hash(&mut hasher); + provider.usable.hash(&mut hasher); + provider.key_configured.hash(&mut hasher); + provider.managed_available.hash(&mut hasher); + matches!(provider.route, crate::config::SearchRoute::Managed).hash(&mut hasher); + } + hasher.finish() } pub fn spec(&self) -> &ToolSpec { @@ -71,10 +136,7 @@ pub fn user_facing_error(error: &str) -> String { Some(code) if code == errors::RATE_LIMITED => { "Web search is rate limited right now. Wait a moment and try again.".to_string() } - Some(code) if code == errors::UNAVAILABLE => { - "Every configured search provider for this request is unavailable right now." - .to_string() - } + Some(code) if code == errors::UNAVAILABLE => SEARCH_EXHAUSTED_MESSAGE.to_string(), Some(code) if code == errors::INVALID_ARGUMENTS => { let marker = format!("{}{code}: ", errors::PREFIX); let detail = error @@ -92,6 +154,16 @@ pub fn error_code(error: &str) -> Option<&'static str> { errors::code_of(error) } +/// Whether this failure means no provider can answer for the rest of the +/// session, rather than something a later call could get past. +/// +/// Only the "every provider is unavailable" verdict qualifies. A rate limit +/// clears on its own, and a low balance or a rejected argument has its own +/// message telling the caller what to change, so neither latches. +pub(crate) fn exhausts_providers(error: &str) -> bool { + error_code(error) == Some(errors::UNAVAILABLE) +} + #[async_trait] impl Tool for TinySearchTool { fn name(&self) -> &str { @@ -133,6 +205,16 @@ impl Tool for TinySearchTool { options: ToolCallOptions, ) -> anyhow::Result { let config = self.live_config().await?; + // Re-read per call, so a key added or a provider switched mid-session + // clears an earlier refusal instead of outliving it. + let signature = self.provider_signature(&config); + if self.is_exhausted_for(signature) { + tracing::debug!( + tool = %self.spec.name, + "[search][tool] refused: no provider answered under this configuration" + ); + return Ok(ToolResult::failed(SEARCH_EXHAUSTED_MESSAGE.to_string())); + } let subject = super::render::subject(&args); let max_results = args .get("max_results") @@ -169,12 +251,20 @@ impl Tool for TinySearchTool { )) } Err(error) => { + if exhausts_providers(&error) { + self.mark_exhausted_for(signature); + } tracing::warn!( tool = %self.spec.name, code = error_code(&error).unwrap_or("unclassified"), + exhausted = self.is_exhausted_for(signature), "[search][tool] failed" ); - Ok(ToolResult::error(user_facing_error(&error))) + Ok(if exhausts_providers(&error) { + ToolResult::failed(user_facing_error(&error)) + } else { + ToolResult::error(user_facing_error(&error)) + }) } } } diff --git a/crates/openhuman-core/src/search/tools_tests.rs b/crates/openhuman-core/src/search/tools_tests.rs index 8cdc4e17dfc..0bd99870516 100644 --- a/crates/openhuman-core/src/search/tools_tests.rs +++ b/crates/openhuman-core/src/search/tools_tests.rs @@ -19,7 +19,10 @@ fn classified_errors_become_actionable_messages() { let balance = user_facing_error("tinysearch.insufficient_balance: 402 from backend"); assert!(balance.contains("balance is too low")); assert!(user_facing_error("tinysearch.rate_limited: slow down").contains("rate limited")); - assert!(user_facing_error("tinysearch.provider_unavailable: all down").contains("unavailable")); + assert_eq!( + user_facing_error("tinysearch.provider_unavailable: all down"), + SEARCH_EXHAUSTED_MESSAGE + ); assert_eq!( user_facing_error( "search ExecuteTool failed: tinysearch.invalid_arguments: urls must not be empty" @@ -100,3 +103,135 @@ fn standard_privacy_mode_allows_search_tool_dispatch() { assert!(local_only_search_block("web_search_tool").is_none()); } + +// --------------------------------------------------------------------------- +// #6991: a credential alone makes the managed route look reachable, so a +// deployment that is offline, firewalled, out of balance or holding a dead key +// offers these tools on every turn and fails every call. The old message ended +// "unavailable right now", which reads as "retry later": in the DeepSWE-10 run +// the agent called search three times on one task, then guessed, and the guess +// was what the hidden test rejected. +// --------------------------------------------------------------------------- + +fn spec(name: &str) -> ToolSpec { + ToolSpec { + name: name.to_string(), + description: "search".into(), + parameters: serde_json::json!({"type": "object"}), + } +} + +/// A config whose managed providers are selected but whose direct keys are +/// absent, which is the shape that offers the tools and cannot serve them. +fn nothing_usable_config() -> Config { + let mut config = Config::default(); + config.search.providers = [("brave".to_string(), SearchProviderSettings::direct())] + .into_iter() + .collect(); + config +} + +#[test] +fn the_unavailable_verdict_tells_the_model_to_stop_rather_than_wait() { + let message = user_facing_error("tinysearch.provider_unavailable: all down"); + + assert!( + message.contains("Do not call it again"), + "the model must be told to stop, got: {message}" + ); + assert!( + !message.contains("right now"), + "nothing the model can do will change the answer, so it must not read as a retry hint: {message}" + ); +} + +#[test] +fn only_the_exhaustion_verdict_is_treated_as_final() { + // A rate limit clears on its own; a low balance and a rejected argument + // each have a message naming what to change. None of them may latch. + assert!(exhausts_providers( + "tinysearch.provider_unavailable: all down" + )); + assert!(!exhausts_providers("tinysearch.rate_limited: slow down")); + assert!(!exhausts_providers("tinysearch.insufficient_balance: 402")); + assert!(!exhausts_providers( + "tinysearch.invalid_arguments: urls must not be empty" + )); + assert!(!exhausts_providers("boom")); +} + +#[test] +fn a_tool_starts_with_nothing_recorded() { + let config = nothing_usable_config(); + let tool = TinySearchTool::recorded(spec("web_search_tool")); + + assert!(!tool.is_exhausted_for(tool.provider_signature(&config))); +} + +#[test] +fn the_refusal_answers_only_for_the_configuration_that_failed() { + // The contract this module documents is that every call re-reads the live + // config, so a provider or login change is honoured without rebuilding the + // session. A refusal that outlived a config change would break it: the user + // adds a key and search stays dead until the thread is abandoned. + let dead = nothing_usable_config(); + let tool = TinySearchTool::recorded(spec("web_search_tool")); + let dead_signature = tool.provider_signature(&dead); + + tool.mark_exhausted_for(dead_signature); + assert!(tool.is_exhausted_for(dead_signature)); + + let mut fixed = dead.clone(); + fixed.search.brave.api_key = Some("added-after-the-failure".into()); + + assert_ne!( + tool.provider_signature(&fixed), + dead_signature, + "adding a provider key must change what the tool sees" + ); + assert!( + !tool.is_exhausted_for(tool.provider_signature(&fixed)), + "adding a key must clear the refusal" + ); +} + +#[test] +fn one_tools_dead_providers_do_not_answer_for_another() { + // `build_search_tools` can offer provider-specific tools beside the role + // tools, and the roles draw on different provider orders, so a shared + // verdict would let one provider's outage disable unrelated ones. + let config = nothing_usable_config(); + let failed = TinySearchTool::recorded(spec("web_search_tool")); + let other = TinySearchTool::recorded(spec("web_answer_tool")); + + failed.mark_exhausted_for(failed.provider_signature(&config)); + + assert!(!other.is_exhausted_for(other.provider_signature(&config))); +} + +#[tokio::test] +async fn a_refused_call_does_not_reach_the_module() { + // The second call under the same configuration is answered from the + // recorded verdict, so a dead search costs one module round trip per + // configuration rather than one per call. + let config = nothing_usable_config(); + let tool = TinySearchTool::new(std::sync::Arc::new(config.clone()), spec("web_search_tool")); + tool.mark_exhausted_for(tool.provider_signature(&config)); + + let result = tool + .execute(serde_json::json!({"query": "anything"})) + .await + .expect("a refusal is a reported failure, not a tool error"); + + assert!(result.is_error); + assert_eq!( + result.error_kind, + Some(tinytools::ToolErrorKind::Failed), + "the failure is permanent for this configuration, so it must not be tagged retryable" + ); + assert!( + result.text().contains("Do not call it again"), + "{:?}", + result.text() + ); +}