diff --git a/src/openhuman/agent/harness/session/agent_tool_exec.rs b/src/openhuman/agent/harness/session/agent_tool_exec.rs index f7f165423f..9b51f9414f 100644 --- a/src/openhuman/agent/harness/session/agent_tool_exec.rs +++ b/src/openhuman/agent/harness/session/agent_tool_exec.rs @@ -46,6 +46,55 @@ pub(super) struct AgentToolExecCtx<'a> { pub artifact_store: Option<&'a ToolResultArtifactStore>, } +fn sorted_tool_names<'a>(names: impl IntoIterator) -> Vec { + let mut names = names.into_iter().map(str::to_string).collect::>(); + names.sort(); + names.dedup(); + names +} + +fn format_available_tools_hint(available_tool_names: &[String]) -> String { + if available_tool_names.is_empty() { + "No tools are currently available.".to_string() + } else { + format!("Available tools: {}", available_tool_names.join(", ")) + } +} + +fn format_unknown_tool_message(tool_name: &str, available_tool_names: &[String]) -> String { + format!( + "Unknown tool: {tool_name}. {}", + format_available_tools_hint(available_tool_names) + ) +} + +fn available_tool_names_for_ctx(ctx: &AgentToolExecCtx<'_>) -> Vec { + let mut names = Vec::new(); + let mut filtered_out = Vec::new(); + for tool in ctx.tools { + let name = tool.name(); + let visible_by_scope = + ctx.visible_tool_names.is_empty() || ctx.visible_tool_names.contains(name); + let allowed_by_policy = ctx.tool_policy_session.is_allowed(name); + + if visible_by_scope && allowed_by_policy { + names.push(name); + } else if visible_by_scope { + filtered_out.push(name.to_string()); + } + } + + if !filtered_out.is_empty() { + log::debug!( + "[agent] filtered unavailable tools from unknown-tool hint channel={} tools={:?}", + ctx.event_channel, + filtered_out + ); + } + + sorted_tool_names(names.into_iter()) +} + /// Execute one parsed tool call end-to-end with the Agent's semantics, emitting /// `ToolCallStarted` / `ToolCallCompleted` through `progress`. Returns the /// result (for history formatting) + the call record (for post-turn hooks). @@ -98,8 +147,13 @@ pub(super) async fn run_agent_tool_call( "[agent] blocked tool call '{}' — not in visible tool set", call.name ); + let available = available_tool_names_for_ctx(ctx); ( - format!("Tool '{}' is not available to this agent", call.name), + format!( + "Tool '{}' is not available to this agent. {}", + call.name, + format_available_tools_hint(&available) + ), false, ) } else if let Some(tool) = ctx.tools.iter().find(|t| t.name() == call.name) { @@ -298,7 +352,8 @@ pub(super) async fn run_agent_tool_call( } } } else { - (format!("Unknown tool: {}", call.name), false) + let available = available_tool_names_for_ctx(ctx); + (format_unknown_tool_message(&call.name, &available), false) }; // Stage 1a — content-aware compaction via the TokenJuice content router. @@ -396,6 +451,7 @@ mod tests { use crate::openhuman::agent::tool_policy::AllowAllToolPolicy; use crate::openhuman::agent_tool_policy::ToolPolicyEngine; use crate::openhuman::tools::traits::{ToolResult, ToolTimeout}; + use crate::openhuman::tools::PermissionLevel; use async_trait::async_trait; use serde_json::json; use std::collections::{HashMap, HashSet}; @@ -403,6 +459,11 @@ mod tests { use std::time::Duration; struct HangingTool; + struct PermissionedTool { + name: &'static str, + permission: PermissionLevel, + } + struct TestProgress { completed: AtomicUsize, timeout_completions: AtomicUsize, @@ -450,6 +511,136 @@ mod tests { } } + #[async_trait] + impl Tool for PermissionedTool { + fn name(&self) -> &str { + self.name + } + + fn description(&self) -> &str { + "test permissioned tool" + } + + fn parameters_schema(&self) -> serde_json::Value { + json!({ "type": "object", "properties": {} }) + } + + async fn execute(&self, _args: serde_json::Value) -> anyhow::Result { + Ok(ToolResult::success("ok")) + } + + fn permission_level(&self) -> PermissionLevel { + self.permission + } + } + + #[tokio::test(flavor = "current_thread")] + async fn session_tool_executor_unknown_tool_lists_available_tools() { + let tools: Vec> = vec![Box::new(HangingTool)]; + let visible_tool_names = HashSet::new(); + let policy_session = ToolPolicyEngine::build_session( + "context_scout", + "web", + "test", + &HashMap::new(), + &tools, + &visible_tool_names, + ); + let tool_policy = AllowAllToolPolicy; + let ctx = AgentToolExecCtx { + tools: &tools, + visible_tool_names: &visible_tool_names, + tool_policy_session: &policy_session, + tool_policy: &tool_policy, + payload_summarizer: None, + event_session_id: "session-1", + event_channel: "web", + agent_definition_id: "context_scout", + prefer_markdown: false, + budget_bytes: 4096, + compaction_enabled: false, + tokenjuice_compression: crate::openhuman::tokenjuice::AgentTokenjuiceCompression::Off, + artifact_store: None, + }; + let call = ParsedToolCall { + name: "search_files".to_string(), + arguments: json!({}), + tool_call_id: Some("call-unknown".to_string()), + }; + let progress = TestProgress { + completed: AtomicUsize::new(0), + timeout_completions: AtomicUsize::new(0), + }; + + let (result, record) = run_agent_tool_call(&ctx, &progress, &call, 0).await; + + assert!(!result.success); + assert!(result.output.contains("Unknown tool: search_files")); + assert!(result.output.contains("Available tools: memory_tree")); + assert!(!record.success); + assert_eq!(progress.completed.load(Ordering::Relaxed), 1); + } + + #[tokio::test(flavor = "current_thread")] + async fn session_tool_executor_unknown_tool_hint_uses_policy_filtered_tools() { + let tools: Vec> = vec![ + Box::new(PermissionedTool { + name: "read_notes", + permission: PermissionLevel::ReadOnly, + }), + Box::new(PermissionedTool { + name: "write_notes", + permission: PermissionLevel::Write, + }), + ]; + let visible_tool_names = HashSet::new(); + let channel_permissions = HashMap::from([("web".to_string(), "readonly".to_string())]); + let policy_session = ToolPolicyEngine::build_session( + "context_scout", + "web", + "test", + &channel_permissions, + &tools, + &visible_tool_names, + ); + let tool_policy = AllowAllToolPolicy; + let ctx = AgentToolExecCtx { + tools: &tools, + visible_tool_names: &visible_tool_names, + tool_policy_session: &policy_session, + tool_policy: &tool_policy, + payload_summarizer: None, + event_session_id: "session-1", + event_channel: "web", + agent_definition_id: "context_scout", + prefer_markdown: false, + budget_bytes: 4096, + compaction_enabled: false, + tokenjuice_compression: crate::openhuman::tokenjuice::AgentTokenjuiceCompression::Off, + artifact_store: None, + }; + let call = ParsedToolCall { + name: "missing_tool".to_string(), + arguments: json!({}), + tool_call_id: Some("call-unknown".to_string()), + }; + let progress = TestProgress { + completed: AtomicUsize::new(0), + timeout_completions: AtomicUsize::new(0), + }; + + let (result, _record) = run_agent_tool_call(&ctx, &progress, &call, 0).await; + + assert!(!result.success); + assert!(result.output.contains("Unknown tool: missing_tool")); + assert!(result.output.contains("Available tools: read_notes")); + assert!( + !result.output.contains("write_notes"), + "unknown-tool hints must not advertise policy-denied tools: {}", + result.output + ); + } + #[tokio::test(flavor = "current_thread")] async fn session_tool_executor_enforces_tool_timeout_policy() { let tools: Vec> = vec![Box::new(HangingTool)]; diff --git a/src/openhuman/agent/harness/session/turn/core.rs b/src/openhuman/agent/harness/session/turn/core.rs index 88bb24866e..86ec50d8db 100644 --- a/src/openhuman/agent/harness/session/turn/core.rs +++ b/src/openhuman/agent/harness/session/turn/core.rs @@ -928,7 +928,11 @@ impl Agent { temperature, messages, tools: self.tools.clone(), - visible_tool_names: self.visible_tool_names.clone(), + visible_tool_names: self + .visible_tool_specs + .iter() + .map(|spec| spec.name.clone()) + .collect(), max_iterations, on_progress: self.on_progress.clone(), context_window, @@ -942,6 +946,7 @@ impl Agent { session_id: self.event_session_id.clone(), channel: self.event_channel().to_string(), agent_definition_id: self.agent_definition_id.clone(), + visibility_filter_active: !self.visible_tool_names.is_empty(), }), }), ) diff --git a/src/openhuman/tinyagents/middleware.rs b/src/openhuman/tinyagents/middleware.rs index a6ea26b9dd..2d4ed4d9e6 100644 --- a/src/openhuman/tinyagents/middleware.rs +++ b/src/openhuman/tinyagents/middleware.rs @@ -36,7 +36,7 @@ use tinyagents::harness::runtime::AgentHarness; use tinyagents::harness::steering::{SteeringCommand, SteeringHandle}; use tinyagents::harness::tool::{ToolCall as TaToolCall, ToolResult as TaToolResult}; -use super::tools::UNKNOWN_TOOL_SENTINEL; +use super::tools::{format_available_tools_hint, UNKNOWN_TOOL_SENTINEL}; use crate::openhuman::agent::harness::payload_summarizer::PayloadSummarizer; use crate::openhuman::approval::{ redact_args, summarize_action, ApprovalGate, ExecutionOutcome, GateOutcome, @@ -423,6 +423,18 @@ impl ToolOutputMiddleware { .find(|t| t.name() == name) .and_then(|t| t.max_result_size_chars()) } + + fn is_recovery_guidance(result: &TaToolResult) -> bool { + if result.error.is_none() { + return false; + } + let content = result.content.as_str(); + content.starts_with("Unknown tool: ") + || (content.starts_with("Tool '") + && content.contains(" is not available to this agent")) + || (content.starts_with("Error: tool '") + && content.contains(" is not available to this sub-agent")) + } } #[async_trait] @@ -481,7 +493,7 @@ impl Middleware<()> for ToolOutputMiddleware { // 3. Shared byte-cap backstop — truncate at a UTF-8 boundary with a marker. // Only for tools with no cap of their own (a capped tool already bounded // itself above; stacking the two markers would double-truncate). - if tool_cap.is_none() && self.budget_bytes > 0 { + if tool_cap.is_none() && self.budget_bytes > 0 && !Self::is_recovery_guidance(result) { let (capped, outcome) = apply_tool_result_budget(std::mem::take(&mut result.content), self.budget_bytes); if outcome.truncated { @@ -674,11 +686,13 @@ pub struct ToolPolicyMiddleware { /// `Tool` can be resolved for its generated-tool runtime context and its /// per-call permission level. tool_sets: Vec>>>, - /// The advertised (visible) tool-name whitelist. Non-empty = restricted; a - /// call outside it is "not available to this agent" (the engine's first gate). - /// A non-visible tool is never registered, so it reaches here rewritten onto - /// the recovery sentinel — its original name rides `requested_tool`. + /// The advertised/callable tool-name whitelist for this run. visible_tool_names: HashSet, + /// Whether the user/agent explicitly scoped the visible tool set. Keep this + /// separate from `visible_tool_names`: channel policy can narrow callable + /// names even when no explicit visibility filter was configured, and those + /// unknown calls should still reach the generic unknown-tool sentinel. + visibility_filter_active: bool, session_id: String, channel: String, agent_definition_id: String, @@ -690,6 +704,7 @@ impl ToolPolicyMiddleware { session: crate::openhuman::agent_tool_policy::ToolPolicySession, tool_sets: Vec>>>, visible_tool_names: HashSet, + visibility_filter_active: bool, session_id: String, channel: String, agent_definition_id: String, @@ -699,6 +714,7 @@ impl ToolPolicyMiddleware { session, tool_sets, visible_tool_names, + visibility_filter_active, session_id, channel, agent_definition_id, @@ -749,6 +765,12 @@ impl ToolPolicyMiddleware { .find(|t| t.name() == name) .and_then(|t| t.generated_runtime_context(args)) } + + fn available_tools_hint(&self) -> String { + let mut names = self.visible_tool_names.iter().cloned().collect::>(); + names.sort(); + format_available_tools_hint(&names) + } } #[async_trait] @@ -777,14 +799,17 @@ impl ToolMiddleware<()> for ToolPolicyMiddleware { // unknown call with no visibility restriction still falls through to the // sentinel's "Unknown tool" result. if call.name == UNKNOWN_TOOL_SENTINEL { - if !self.visible_tool_names.is_empty() { + if self.visibility_filter_active { let requested = call .arguments .get("requested_tool") .and_then(|v| v.as_str()) .unwrap_or_default(); if !requested.is_empty() && !self.visible_tool_names.contains(requested) { - let content = format!("Tool '{requested}' is not available to this agent"); + let content = format!( + "Tool '{requested}' is not available to this agent. {} Use one of the advertised tools, or answer directly.", + self.available_tools_hint() + ); return Ok(MiddlewareToolOutcome::Result(TaToolResult { call_id: call.id, name: call.name, @@ -1481,6 +1506,24 @@ mod tests { ); } + #[tokio::test] + async fn tool_output_preserves_unknown_tool_guidance_under_small_budget() { + let mw = ToolOutputMiddleware { + budget_bytes: 96, + payload_summarizer: None, + tool_sets: vec![], + }; + let content = "Unknown tool: hidden_tool. Available tools: cli_only, round17_boom, round17_error, round17_ok. Use one of the advertised tools, or answer directly."; + let mut result = tool_result(UNKNOWN_TOOL_SENTINEL, content); + result.error = Some(content.to_string()); + + mw.after_tool(&mut ctx(), &(), &mut result).await.unwrap(); + + assert!(result.content.contains("hidden_tool")); + assert!(result.content.contains("Available tools: cli_only")); + assert!(!result.content.contains("truncated by tool_result_budget")); + } + #[tokio::test] async fn tool_output_leaves_small_results_untouched() { let mw = ToolOutputMiddleware { diff --git a/src/openhuman/tinyagents/mod.rs b/src/openhuman/tinyagents/mod.rs index a41937ff8d..2586ff1a7a 100644 --- a/src/openhuman/tinyagents/mod.rs +++ b/src/openhuman/tinyagents/mod.rs @@ -72,6 +72,12 @@ pub struct ToolPolicyEnforcement { pub session_id: String, pub channel: String, pub agent_definition_id: String, + /// True when the agent definition/session explicitly scoped the visible tool + /// set. Keep this separate from the policy-filtered callable set: a readonly + /// channel can make `allowed` non-empty even though there was no agent-scope + /// filter, and genuinely unknown tools should still get the generic unknown + /// recovery wording in that case. + pub visibility_filter_active: bool, } /// Drain the run queue's pending steer messages and forward them to the @@ -356,6 +362,15 @@ pub async fn run_turn_via_tinyagents_shared( names.insert(UNKNOWN_TOOL_SENTINEL.to_string()); Arc::new(names) }; + let available_tool_names: Vec = { + let mut names = valid_tools + .iter() + .filter(|name| name.as_str() != UNKNOWN_TOOL_SENTINEL) + .cloned() + .collect::>(); + names.sort(); + names + }; let cursor: IterationCursor = Arc::default(); // Keep a provider handle for the context-window summarizer (the run consumes @@ -507,7 +522,10 @@ pub async fn run_turn_via_tinyagents_shared( // The unknown-tool sentinel: the model adapter rewrites any unadvertised tool // call onto it so the run recovers gracefully instead of aborting. Its wording // matches the legacy engine (sub-agent vs top-level). - harness.register_tool(Arc::new(UnknownToolAdapter::new(subagent_scope.is_some()))); + harness.register_tool(Arc::new(UnknownToolAdapter::new( + subagent_scope.is_some(), + available_tool_names, + ))); let tool_count = registered.len(); // Human-in-the-loop approval as a named tool middleware (issue #4249, @@ -545,6 +563,7 @@ pub async fn run_turn_via_tinyagents_shared( enforcement.session, tool_sets.clone(), allowed.clone(), + enforcement.visibility_filter_active, enforcement.session_id, enforcement.channel, enforcement.agent_definition_id, diff --git a/src/openhuman/tinyagents/tests.rs b/src/openhuman/tinyagents/tests.rs index 3c51204b68..ec686ac241 100644 --- a/src/openhuman/tinyagents/tests.rs +++ b/src/openhuman/tinyagents/tests.rs @@ -34,6 +34,29 @@ impl Tool for EchoTool { } } +struct NamedTool { + name: &'static str, +} + +#[async_trait] +impl Tool for NamedTool { + fn name(&self) -> &str { + self.name + } + + fn description(&self) -> &str { + "test named tool" + } + + fn parameters_schema(&self) -> serde_json::Value { + serde_json::json!({"type": "object", "properties": {}}) + } + + async fn execute(&self, _args: serde_json::Value) -> anyhow::Result { + Ok(ToolResult::success("ok")) + } +} + /// Mock provider: first call requests the echo tool, second call answers. struct EchoThenDone { calls: AtomicUsize, @@ -104,6 +127,126 @@ async fn turn_runs_through_the_tinyagents_harness_with_real_tools() { ); } +struct UnknownThenDone { + calls: AtomicUsize, + request_tool_names: std::sync::Mutex>>, + request_messages: std::sync::Mutex>>, +} + +#[async_trait] +impl Provider for UnknownThenDone { + async fn chat_with_system( + &self, + _s: Option<&str>, + _m: &str, + _model: &str, + _t: f64, + ) -> anyhow::Result { + Ok(String::new()) + } + + async fn chat( + &self, + request: ChatRequest<'_>, + _model: &str, + _temperature: f64, + ) -> anyhow::Result { + let tool_names = request + .tools + .unwrap_or(&[]) + .iter() + .map(|tool| tool.name.clone()) + .collect::>(); + self.request_tool_names.lock().unwrap().push(tool_names); + self.request_messages + .lock() + .unwrap() + .push(request.messages.to_vec()); + + let call = self.calls.fetch_add(1, Ordering::SeqCst); + if call == 0 { + Ok(ChatResponse { + tool_calls: vec![ToolCall { + id: "unknown-1".to_string(), + name: "missing_tool".to_string(), + arguments: "{}".to_string(), + extra_content: None, + }], + ..Default::default() + }) + } else { + Ok(ChatResponse { + text: Some("recovered".to_string()), + ..Default::default() + }) + } + } + + fn supports_native_tools(&self) -> bool { + true + } +} + +#[tokio::test] +async fn unknown_tool_hint_uses_the_policy_filtered_available_set() { + let provider = Arc::new(UnknownThenDone { + calls: AtomicUsize::new(0), + request_tool_names: std::sync::Mutex::new(Vec::new()), + request_messages: std::sync::Mutex::new(Vec::new()), + }); + let tools: Arc>> = Arc::new(vec![ + Box::new(NamedTool { name: "read_notes" }), + Box::new(NamedTool { + name: "write_notes", + }), + ]); + let allowed = std::collections::HashSet::from(["read_notes".to_string()]); + + let outcome = run_turn_via_tinyagents_shared( + provider.clone(), + "mock-model", + 0.0, + vec![ChatMessage::user("use a missing tool")], + vec![tools], + allowed, + 4, + None, + None, + None, + None, + &[], + false, + None, + TurnContextMiddleware::defaults(), + None, + ) + .await + .expect("unknown tool recovery should continue the turn"); + + assert_eq!(outcome.text, "recovered"); + let request_tool_names = provider.request_tool_names.lock().unwrap(); + assert!(request_tool_names[0].contains(&"read_notes".to_string())); + assert!( + !request_tool_names[0].contains(&"write_notes".to_string()), + "provider-visible tool schema must use the filtered allowlist: {:?}", + request_tool_names[0] + ); + + let messages = provider.request_messages.lock().unwrap(); + let joined = messages + .iter() + .flatten() + .map(|message| message.content.as_str()) + .collect::>() + .join("\n"); + assert!(joined.contains("Unknown tool: missing_tool")); + assert!(joined.contains("Available tools: read_notes")); + assert!( + !joined.contains("write_notes"), + "unknown-tool hint must not advertise policy-filtered tools: {joined}" + ); +} + /// A provider that streams visible text in chunks through the request's stream /// sender, then returns the aggregated reply — exercising `ProviderModel::stream`. struct StreamingProvider; diff --git a/src/openhuman/tinyagents/tools.rs b/src/openhuman/tinyagents/tools.rs index 6f1227702c..f0aac4df87 100644 --- a/src/openhuman/tinyagents/tools.rs +++ b/src/openhuman/tinyagents/tools.rs @@ -33,11 +33,23 @@ pub const UNKNOWN_TOOL_SENTINEL: &str = "__openhuman_unknown_tool__"; /// "Unknown tool" message (`engine::tools`). Tests and the model key off these. pub struct UnknownToolAdapter { subagent: bool, + available_tool_names: Arc>, } impl UnknownToolAdapter { - pub fn new(subagent: bool) -> Self { - Self { subagent } + pub fn new(subagent: bool, available_tool_names: Vec) -> Self { + Self { + subagent, + available_tool_names: Arc::new(available_tool_names), + } + } +} + +pub(crate) fn format_available_tools_hint(available_tool_names: &[String]) -> String { + if available_tool_names.is_empty() { + "No tools are currently available.".to_string() + } else { + format!("Available tools: {}", available_tool_names.join(", ")) } } @@ -65,14 +77,15 @@ impl Tool<()> for UnknownToolAdapter { .get("requested_tool") .and_then(|v| v.as_str()) .unwrap_or("unknown"); + let available_hint = format_available_tools_hint(&self.available_tool_names); let content = if self.subagent { format!( - "Error: tool '{requested}' is not available to this sub-agent. \ + "Error: tool '{requested}' is not available to this sub-agent. {available_hint} \ Use one of your listed tools, or answer directly." ) } else { format!( - "Unknown tool: {requested}. It is not available; do not call it again. \ + "Unknown tool: {requested}. {available_hint} It is not available; do not call it again. \ Use one of the advertised tools, or answer directly." ) }; diff --git a/tests/agent_session_turn_raw_coverage_e2e.rs b/tests/agent_session_turn_raw_coverage_e2e.rs index be87c85f93..75390db8e2 100644 --- a/tests/agent_session_turn_raw_coverage_e2e.rs +++ b/tests/agent_session_turn_raw_coverage_e2e.rs @@ -819,6 +819,22 @@ async fn turn_xml_failures_checkpoint_policy_visibility_and_hooks_are_publicly_e let hooks = hook_calls.lock().await; assert_eq!(hooks[0].assistant_response, checkpoint); assert_eq!(hooks[0].tool_calls.len(), 6); + let tool_names = hooks[0] + .tool_calls + .iter() + .map(|record| record.name.as_str()) + .collect::>(); + assert_eq!( + tool_names, + vec![ + "hidden_tool", + "cli_only", + "round17_error", + "round17_boom", + "round17_write", + "round17_ok", + ] + ); let joined = provider .requests() @@ -827,7 +843,23 @@ async fn turn_xml_failures_checkpoint_policy_visibility_and_hooks_are_publicly_e .map(|message| message.content) .collect::>() .join("\n"); - assert!(joined.contains("not available to this agent")); + assert!(joined.contains("")); + assert!(joined.contains("Tool 'hidden_tool' is not available to this agent")); + assert!(joined.contains("Available tools: cli_only, round17_boom, round17_error, round17_ok")); + let allowed_tool_lines = joined + .lines() + .filter(|line| line.starts_with("- Allowed tools:")) + .collect::>(); + assert!( + !allowed_tool_lines.is_empty(), + "expected policy boundary allowed-tools line in provider context" + ); + assert!( + allowed_tool_lines + .iter() + .all(|line| !line.contains("round17_write")), + "write tool should not be advertised as policy-allowed: {allowed_tool_lines:?}" + ); assert!(joined.contains("semantic failure")); assert!(joined.contains("Error executing round17_boom")); assert!(joined.contains("denied by policy 'round17-deny'"));