diff --git a/src/openhuman/agent/harness/session/agent_tool_exec.rs b/src/openhuman/agent/harness/session/agent_tool_exec.rs index f7f165423f..3237d382fa 100644 --- a/src/openhuman/agent/harness/session/agent_tool_exec.rs +++ b/src/openhuman/agent/harness/session/agent_tool_exec.rs @@ -14,6 +14,7 @@ use crate::core::event_bus::{publish_global, DomainEvent}; use crate::openhuman::agent::dispatcher::{ParsedToolCall, ToolExecutionResult}; use crate::openhuman::agent::harness::engine::ProgressReporter; use crate::openhuman::agent::harness::payload_summarizer::PayloadSummarizer; +use crate::openhuman::agent::harness::session::policy_denial::PolicyDenial; use crate::openhuman::agent::harness::tool_result_artifacts::{ apply_per_result_persistence, ToolResultArtifactStore, }; @@ -105,15 +106,14 @@ pub(super) async fn run_agent_tool_call( } else if let Some(tool) = ctx.tools.iter().find(|t| t.name() == call.name) { let session_decision = ctx.tool_policy_session.decision_for(&call.name); if session_decision.is_denied() { - let required = session_decision - .required_permission - .map(|permission| permission.to_string()) - .unwrap_or_else(|| "unknown".to_string()); ( - format!( - "Tool '{}' blocked by tool policy: requires {}, channel '{}' allows {}", - call.name, required, ctx.event_channel, session_decision.allowed_permission - ), + PolicyDenial::SessionForbidden { + tool: &call.name, + required: session_decision.required_permission, + allowed: session_decision.allowed_permission, + channel: ctx.event_channel, + } + .render(), false, ) } else { @@ -126,13 +126,13 @@ pub(super) async fn run_agent_tool_call( "[agent_loop] tool action blocked by per-call permission check" ); ( - format!( - "Tool '{}' action requires {} permission, channel '{}' allows {}", - call.name, - call_required, - ctx.event_channel, - session_decision.allowed_permission - ), + PolicyDenial::PermissionTooLow { + tool: &call.name, + required: call_required, + allowed: session_decision.allowed_permission, + channel: ctx.event_channel, + } + .render(), false, ) } else { @@ -168,14 +168,21 @@ pub(super) async fn run_agent_tool_call( reason = %reason, "[agent_loop] tool blocked by policy" ); - ( - format!( - "Tool '{}' {blocked_action} by policy '{}': {reason}", - call.name, - ctx.tool_policy.name() - ), - false, - ) + let denial = match &policy_decision { + ToolPolicyDecision::RequireApproval { .. } => { + PolicyDenial::ApprovalRequired { + tool: &call.name, + policy: ctx.tool_policy.name(), + reason, + } + } + _ => PolicyDenial::PolicyDenied { + tool: &call.name, + policy: ctx.tool_policy.name(), + reason, + }, + }; + (denial.render(), false) } else { let options = ToolCallOptions { prefer_markdown: ctx.prefer_markdown, @@ -502,4 +509,183 @@ mod tests { assert_eq!(progress.completed.load(Ordering::Relaxed), 1); assert_eq!(progress.timeout_completions.load(Ordering::Relaxed), 1); } + + /// A tool whose required permission is configurable, used to trip the + /// session / per-call permission gates. + struct PermissionedTool { + name: &'static str, + permission: crate::openhuman::tools::PermissionLevel, + } + + #[async_trait] + impl Tool for PermissionedTool { + fn name(&self) -> &str { + self.name + } + + fn description(&self) -> &str { + "test tool with a fixed permission level" + } + + fn parameters_schema(&self) -> serde_json::Value { + json!({ "type": "object", "properties": {} }) + } + + async fn execute(&self, _args: serde_json::Value) -> anyhow::Result { + Ok(ToolResult::success("ran")) + } + + fn permission_level(&self) -> crate::openhuman::tools::PermissionLevel { + self.permission + } + } + + /// A pluggable policy that denies every call with a fixed reason, standing + /// in for a sandbox / permission-boundary block. + struct DenyingToolPolicy; + + #[async_trait] + impl ToolPolicy for DenyingToolPolicy { + fn name(&self) -> &str { + "sandbox" + } + + async fn check(&self, _request: &ToolPolicyRequest) -> ToolPolicyDecision { + ToolPolicyDecision::deny("sandbox restriction") + } + } + + /// Progress reporter that captures the last completion so tests can assert + /// the block was surfaced (not silently dropped). + struct CapturingProgress { + completed: AtomicUsize, + } + + #[async_trait] + impl ProgressReporter for CapturingProgress { + async fn tool_completed( + &self, + _call_id: &str, + _tool_name: &str, + _success: bool, + _output: &str, + _elapsed_ms: u64, + _iteration: u32, + ) { + self.completed.fetch_add(1, Ordering::Relaxed); + } + } + + #[tokio::test(flavor = "current_thread")] + async fn session_policy_denial_returns_structured_message_with_workaround() { + // Channel capped at read-only; an Execute tool is denied by the session + // policy rather than allowed to run. + let tools: Vec> = vec![Box::new(PermissionedTool { + name: "run_script", + permission: crate::openhuman::tools::PermissionLevel::Execute, + })]; + let visible_tool_names = HashSet::new(); + let mut channel_permissions = HashMap::new(); + channel_permissions.insert("web".to_string(), "read_only".to_string()); + let policy_session = ToolPolicyEngine::build_session( + "context_scout", + "web", + "test", + &channel_permissions, + &tools, + &visible_tool_names, + ); + assert!(policy_session.decision_for("run_script").is_denied()); + + 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: "run_script".to_string(), + arguments: json!({}), + tool_call_id: Some("call-1".to_string()), + }; + let progress = CapturingProgress { + completed: AtomicUsize::new(0), + }; + + let (result, record) = run_agent_tool_call(&ctx, &progress, &call, 0).await; + + assert!(!result.success); + assert!(!record.success); + // Structured: what was blocked, why, and a concrete workaround. + assert!(result.output.contains("Blocked: Tool 'run_script'")); + assert!(result.output.contains("requires Execute permission")); + assert!(result.output.contains("Workaround:")); + assert!(result.output.contains("channel_permissions")); + // The agent is told to relay rather than halt silently — and the block + // was surfaced through progress, not dropped. + assert!(result.output.contains("Relay this to the user")); + assert_eq!(progress.completed.load(Ordering::Relaxed), 1); + } + + #[tokio::test(flavor = "current_thread")] + async fn pluggable_policy_denial_returns_structured_message_with_alternative() { + // Tool passes the session/permission gates but a pluggable policy denies + // it — matching the issue's `run_script → blocked by sandbox` example. + let tools: Vec> = vec![Box::new(PermissionedTool { + name: "run_script", + permission: crate::openhuman::tools::PermissionLevel::ReadOnly, + })]; + 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 = DenyingToolPolicy; + 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: "run_script".to_string(), + arguments: json!({}), + tool_call_id: Some("call-1".to_string()), + }; + let progress = CapturingProgress { + completed: AtomicUsize::new(0), + }; + + let (result, _record) = run_agent_tool_call(&ctx, &progress, &call, 0).await; + + assert!(!result.success); + assert!(result.output.contains("denied by policy 'sandbox'")); + assert!(result.output.contains("sandbox restriction")); + assert!(result.output.contains("permitted alternative")); + assert!(result.output.contains("Relay this to the user")); + assert_eq!(progress.completed.load(Ordering::Relaxed), 1); + } } diff --git a/src/openhuman/agent/harness/session/mod.rs b/src/openhuman/agent/harness/session/mod.rs index 02e75b1e1a..fda715212b 100644 --- a/src/openhuman/agent/harness/session/mod.rs +++ b/src/openhuman/agent/harness/session/mod.rs @@ -23,6 +23,7 @@ mod agent_tool_exec; mod builder; pub mod migration; +pub(crate) mod policy_denial; mod runtime; pub(crate) mod transcript; mod turn; diff --git a/src/openhuman/agent/harness/session/policy_denial.rs b/src/openhuman/agent/harness/session/policy_denial.rs new file mode 100644 index 0000000000..4469ba5093 --- /dev/null +++ b/src/openhuman/agent/harness/session/policy_denial.rs @@ -0,0 +1,244 @@ +//! Structured, actionable messages for policy / permission denials. +//! +//! When the harness blocks a tool call at a policy or permission boundary, the +//! agent must not dead-end with a bare "blocked" line. Each denial is rendered +//! into a structured message — **what** was blocked, **why**, and a concrete +//! **workaround** (how to enable it, or a permitted alternative) — followed by +//! an explicit instruction to relay it to the user rather than halting +//! silently. The rendered string is returned as the (failed) tool result, so it +//! flows back into the turn the same way the unknown-tool corrective error is +//! surfaced to the model (see PR #4360). + +use crate::openhuman::tools::PermissionLevel; + +/// The boundary that blocked a tool call, with the context needed to explain it +/// and suggest a way forward. +pub(crate) enum PolicyDenial<'a> { + /// The session tool policy forbids this tool for the channel's permission + /// tier (it is not in the allowed set). + SessionForbidden { + tool: &'a str, + required: Option, + allowed: PermissionLevel, + channel: &'a str, + }, + /// The tool is allowed in general, but *this call's* arguments require a + /// higher permission than the channel grants. + PermissionTooLow { + tool: &'a str, + required: PermissionLevel, + allowed: PermissionLevel, + channel: &'a str, + }, + /// A pluggable [`ToolPolicy`](crate::openhuman::agent::tool_policy::ToolPolicy) + /// denied the call outright. + PolicyDenied { + tool: &'a str, + policy: &'a str, + reason: &'a str, + }, + /// A pluggable [`ToolPolicy`](crate::openhuman::agent::tool_policy::ToolPolicy) + /// requires an approval handoff this executor cannot complete inline. + ApprovalRequired { + tool: &'a str, + policy: &'a str, + reason: &'a str, + }, +} + +/// Suffix appended to every denial so the agent relays the block instead of +/// silently stopping. +const RELAY_INSTRUCTION: &str = "Relay this to the user: explain what was \ + blocked and why, then offer the workaround as the next step. Do not stop \ + silently."; + +impl PolicyDenial<'_> { + /// Render the denial as a structured `Blocked / Reason / Workaround / relay` + /// message for the model. + pub(crate) fn render(&self) -> String { + let (blocked, reason, workaround) = match self { + PolicyDenial::SessionForbidden { + tool, + required, + allowed, + channel, + } => { + let reason = match required { + Some(required) => format!( + "it requires {required} permission, but the '{channel}' channel only \ + grants {allowed} access" + ), + None => format!( + "it is not permitted at the '{channel}' channel's {allowed} access tier" + ), + }; + ( + format!("Tool '{tool}' is blocked by the session tool policy"), + reason, + raise_ceiling_workaround(*required, *allowed, channel), + ) + } + PolicyDenial::PermissionTooLow { + tool, + required, + allowed, + channel, + } => ( + format!("Tool '{tool}' is blocked by a per-call permission check"), + format!( + "this call needs {required} permission, but the '{channel}' channel only \ + grants {allowed} access" + ), + raise_ceiling_workaround(Some(*required), *allowed, channel), + ), + PolicyDenial::PolicyDenied { + tool, + policy, + reason, + } => ( + format!("Tool '{tool}' was denied by policy '{policy}'"), + (*reason).to_string(), + "Address the reason above, or reach the goal with a permitted alternative tool / \ + path. If this action is genuinely required, ask the user to adjust the policy." + .to_string(), + ), + PolicyDenial::ApprovalRequired { + tool, + policy, + reason, + } => ( + format!("Tool '{tool}' requires approval under policy '{policy}'"), + (*reason).to_string(), + "Ask the user to approve this action, then retry — or choose an alternative that \ + does not require approval." + .to_string(), + ), + }; + + format!( + "Blocked: {blocked}. Reason: {reason}. Workaround: {workaround} {RELAY_INSTRUCTION}" + ) + } +} + +/// Workaround shared by the permission-ceiling denials. The blocked `allowed` +/// level is the channel's cap from `[agent].channel_permissions` (see +/// `ToolPolicyEngine::build_session`), so the fix is to raise *that* channel's +/// entry — not the autonomy tier, which is a separate `readonly|supervised|full` +/// knob. Alternatively, fall back to a tool that fits the current ceiling. +fn raise_ceiling_workaround( + required: Option, + allowed: PermissionLevel, + channel: &str, +) -> String { + match required { + Some(required) => format!( + "Raise the '{channel}' channel's permission ceiling in the `[agent].channel_permissions` \ + config to at least `{}` (levels: read_only, write, execute, dangerous), or accomplish \ + the goal with a tool that needs only {allowed} access.", + permission_config_token(required) + ), + None => format!( + "Raise the '{channel}' channel's permission ceiling in the `[agent].channel_permissions` \ + config (levels: read_only, write, execute, dangerous), or accomplish the goal with a \ + tool that needs only {allowed} access." + ), + } +} + +/// The lowercase token `[agent].channel_permissions` accepts for a level +/// (mirrors `agent_tool_policy::engine::parse_permission_level`). +fn permission_config_token(level: PermissionLevel) -> &'static str { + match level { + PermissionLevel::None => "none", + PermissionLevel::ReadOnly => "read_only", + PermissionLevel::Write => "write", + PermissionLevel::Execute => "execute", + PermissionLevel::Dangerous => "dangerous", + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn session_forbidden_with_required_lists_reason_and_workaround() { + let msg = PolicyDenial::SessionForbidden { + tool: "run_script", + required: Some(PermissionLevel::Execute), + allowed: PermissionLevel::ReadOnly, + channel: "web", + } + .render(); + + assert!(msg.starts_with("Blocked: Tool 'run_script'")); + assert!(msg.contains("Reason:")); + assert!(msg.contains("requires Execute permission")); + assert!(msg.contains("Workaround:")); + // Points at the channel-permission ceiling (not the autonomy tier). + assert!(msg.contains("channel_permissions")); + assert!(msg.contains("`execute`")); + // The relay instruction is what keeps the agent from halting silently. + assert!(msg.contains("Relay this to the user")); + } + + #[test] + fn session_forbidden_without_required_still_has_workaround() { + let msg = PolicyDenial::SessionForbidden { + tool: "run_script", + required: None, + allowed: PermissionLevel::ReadOnly, + channel: "cron", + } + .render(); + + assert!(msg.contains("not permitted")); + assert!(msg.contains("Workaround:")); + assert!(msg.contains("Relay this to the user")); + } + + #[test] + fn permission_too_low_names_both_levels() { + let msg = PolicyDenial::PermissionTooLow { + tool: "shell", + required: PermissionLevel::Write, + allowed: PermissionLevel::ReadOnly, + channel: "web", + } + .render(); + + assert!(msg.contains("needs Write permission")); + assert!(msg.contains("only grants ReadOnly")); + assert!(msg.contains("Workaround:")); + } + + #[test] + fn policy_denied_carries_reason_and_alternative() { + let msg = PolicyDenial::PolicyDenied { + tool: "run_script", + policy: "sandbox", + reason: "sandbox restriction", + } + .render(); + + assert!(msg.contains("denied by policy 'sandbox'")); + assert!(msg.contains("sandbox restriction")); + assert!(msg.contains("permitted alternative")); + assert!(msg.contains("Relay this to the user")); + } + + #[test] + fn approval_required_suggests_approval_then_retry() { + let msg = PolicyDenial::ApprovalRequired { + tool: "send_email", + policy: "approval_gate", + reason: "outbound message needs sign-off", + } + .render(); + + assert!(msg.contains("requires approval under policy 'approval_gate'")); + assert!(msg.contains("approve this action")); + assert!(msg.contains("Relay this to the user")); + } +} diff --git a/src/openhuman/tinyagents/middleware.rs b/src/openhuman/tinyagents/middleware.rs index a6ea26b9dd..2c99c90c96 100644 --- a/src/openhuman/tinyagents/middleware.rs +++ b/src/openhuman/tinyagents/middleware.rs @@ -38,6 +38,7 @@ use tinyagents::harness::tool::{ToolCall as TaToolCall, ToolResult as TaToolResu use super::tools::UNKNOWN_TOOL_SENTINEL; use crate::openhuman::agent::harness::payload_summarizer::PayloadSummarizer; +use crate::openhuman::agent::harness::session::policy_denial::PolicyDenial; use crate::openhuman::approval::{ redact_args, summarize_action, ApprovalGate, ExecutionOutcome, GateOutcome, }; @@ -662,9 +663,11 @@ impl ToolMiddleware<()> for CliRpcOnlyMiddleware { /// `agent_tool_exec` (`ctx.tool_policy.check(...)`); the tinyagents path bypassed /// it, so a `.tool_policy()` deny/require-approval silently no-opped and the tool /// executed anyway — a security regression. This middleware restores it: a -/// blocking decision short-circuits with a model-consumable result carrying the -/// same `"Tool '' by policy '': "` -/// wording the engine produced. +/// blocking decision short-circuits with a model-consumable result. The message +/// is rendered by [`PolicyDenial`] — a structured `Blocked / Reason / Workaround` +/// line with an explicit instruction to relay it to the user (issue #4094) — +/// shared with the engine's `agent_tool_exec` path so both surface the same +/// wording. pub struct ToolPolicyMiddleware { policy: Arc, /// The session's channel-permission snapshot — enforces the per-channel deny @@ -718,22 +721,28 @@ impl ToolPolicyMiddleware { fn channel_permission_block(&self, call: &TaToolCall) -> Option { let decision = self.session.decision_for(&call.name); if decision.is_denied() { - let required = decision - .required_permission - .map(|permission| permission.to_string()) - .unwrap_or_else(|| "unknown".to_string()); - return Some(format!( - "Tool '{}' blocked by tool policy: requires {}, channel '{}' allows {}", - call.name, required, self.channel, decision.allowed_permission - )); + return Some( + PolicyDenial::SessionForbidden { + tool: &call.name, + required: decision.required_permission, + allowed: decision.allowed_permission, + channel: &self.channel, + } + .render(), + ); } let tool = self.resolve_tool(&call.name)?; let call_required = tool.permission_level_with_args(&call.arguments); if call_required > decision.allowed_permission { - return Some(format!( - "Tool '{}' action requires {} permission, channel '{}' allows {}", - call.name, call_required, self.channel, decision.allowed_permission - )); + return Some( + PolicyDenial::PermissionTooLow { + tool: &call.name, + required: call_required, + allowed: decision.allowed_permission, + channel: &self.channel, + } + .render(), + ); } None } @@ -849,11 +858,19 @@ impl ToolMiddleware<()> for ToolPolicyMiddleware { reason = %reason, "[tinyagents::mw] tool blocked by policy" ); - let content = format!( - "Tool '{}' {blocked_action} by policy '{}': {reason}", - call.name, - self.policy.name() - ); + let content = match &decision { + ToolPolicyDecision::RequireApproval { .. } => PolicyDenial::ApprovalRequired { + tool: &call.name, + policy: self.policy.name(), + reason, + }, + _ => PolicyDenial::PolicyDenied { + tool: &call.name, + policy: self.policy.name(), + reason, + }, + } + .render(); return Ok(MiddlewareToolOutcome::Result(TaToolResult { call_id: call.id, name: call.name, @@ -1577,6 +1594,82 @@ mod tests { assert_eq!(call.name, UNKNOWN_TOOL_SENTINEL); } + // ── ToolPolicyMiddleware (real turn path) ─────────────────────────────── + + /// A tool with a configurable required permission, to trip the channel + /// permission ceiling the middleware enforces on the tinyagents turn path. + struct PermTool { + name: &'static str, + permission: crate::openhuman::tools::PermissionLevel, + } + + #[async_trait] + impl Tool for PermTool { + fn name(&self) -> &str { + self.name + } + fn description(&self) -> &str { + "perm" + } + fn parameters_schema(&self) -> serde_json::Value { + json!({ "type": "object" }) + } + async fn execute( + &self, + _args: serde_json::Value, + ) -> anyhow::Result { + Ok(crate::openhuman::tools::ToolResult::success("ok")) + } + fn permission_level(&self) -> crate::openhuman::tools::PermissionLevel { + self.permission + } + } + + /// The real turn path (tinyagents middleware) must surface the structured + /// denial with a workaround, not the old bare string — otherwise the #4094 + /// fix never reaches production turns (only direct `execute_tool_call`). + #[test] + fn channel_permission_block_returns_structured_denial() { + let tools: Arc>> = Arc::new(vec![Box::new(PermTool { + name: "run_script", + permission: crate::openhuman::tools::PermissionLevel::Execute, + })]); + let visible = HashSet::new(); + let mut channel_permissions = std::collections::HashMap::new(); + channel_permissions.insert("web".to_string(), "read_only".to_string()); + let session = crate::openhuman::agent_tool_policy::ToolPolicyEngine::build_session( + "context_scout", + "web", + "test", + &channel_permissions, + tools.as_ref(), + &visible, + ); + let mw = ToolPolicyMiddleware::new( + Arc::new(crate::openhuman::agent::tool_policy::AllowAllToolPolicy), + session, + vec![tools], + visible, + "session-1".to_string(), + "web".to_string(), + "context_scout".to_string(), + ); + let call = TaToolCall { + id: "1".into(), + name: "run_script".into(), + arguments: json!({}), + }; + + let msg = mw + .channel_permission_block(&call) + .expect("execute tool must be blocked at the read_only channel ceiling"); + assert!(msg.contains("Blocked: Tool 'run_script'")); + assert!(msg.contains("requires Execute permission")); + assert!(msg.contains("Workaround:")); + assert!(msg.contains("channel_permissions")); + assert!(msg.contains("Relay this to the user")); + } + // ── CostBudgetMiddleware ──────────────────────────────────────────────── #[tokio::test]