Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions src/openhuman/tinyagents/middleware.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1403,6 +1403,25 @@ impl Middleware<()> for ToolOutcomeCaptureMiddleware {
_state: &(),
result: &mut TaToolResult,
) -> TaResult<()> {
// Enrich a raw security-policy / autonomy block (issue #4094): the ~20
// `[policy-blocked]` denials emitted deep in `SecurityPolicy` / the tools
// return a bare marker line with no workaround and no relay directive, so
// the agent dead-ends. Rewrite the content into the structured
// `Blocked / Reason / Workaround / relay` shape here — the last `after_tool`
// hook, so the enriched text is what the transcript keeps. The marker is
// preserved, and already-structured `ToolPolicyMiddleware` denials (which
// carry a `Workaround:` suffix) are left untouched. This runs before
// classification below, which still recognises the preserved marker.
if let Some(enriched) =
super::policy_denial::maybe_enrich_policy_block(&result.name, &result.content)
{
Comment on lines +1415 to +1417

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Gate policy-block enrichment to failed tool results

When a successful tool returns content containing the literal [policy-blocked] marker, this unconditional enrichment rewrites the success payload into a fake security-policy denial before success is computed. This is reachable with file_read, which returns file contents verbatim on success, so reading source/docs that mention the marker corrupts the transcript while the call is still recorded as successful; only actual failed, marker-prefixed policy errors should be enriched.

Useful? React with 👍 / 👎.

tracing::debug!(
tool = result.name.as_str(),
"[tinyagents::mw] enriched raw security-policy block with workaround + relay"
);
result.content = enriched;
}

let success = result.error.is_none();
// Classify the failure so the live `ToolCallCompleted` event and the
// persisted timeline can explain it in plain language. A hard
Expand Down Expand Up @@ -1923,6 +1942,52 @@ mod tests {
}
}

// ── ToolOutcomeCaptureMiddleware policy-block enrichment (issue #4094) ───

fn outcome_capture_mw() -> ToolOutcomeCaptureMiddleware {
ToolOutcomeCaptureMiddleware::new(
std::sync::Arc::new(std::sync::Mutex::new(Vec::new())),
std::sync::Arc::new(std::sync::Mutex::new(std::collections::HashMap::new())),
)
}

#[tokio::test]
async fn raw_security_policy_block_is_enriched_with_workaround_and_relay() {
let mw = outcome_capture_mw();
let mut result = tool_result(
"run_command",
"[policy-blocked] Security policy: read-only mode — only read commands are allowed",
);
result.error = Some(result.content.clone());
mw.after_tool(&mut ctx(), &(), &mut result).await.unwrap();
// The bare denial now carries a workaround + relay directive, and keeps the
// marker so classification / the loop-breaker still recognise it.
assert!(result.content.contains("Workaround:"), "{}", result.content);
assert!(result.content.contains("Relay this to the user"));
assert!(result
.content
.contains(crate::openhuman::security::POLICY_BLOCKED_MARKER));
assert!(result.content.contains("read-only mode"));
}

#[tokio::test]
async fn already_structured_denial_is_not_double_wrapped() {
// A ToolPolicyMiddleware-style denial already has "Workaround:"; the capture
// middleware must leave it untouched (no second Workaround block).
let mw = outcome_capture_mw();
let structured =
"Blocked: Tool 'x' denied. Reason: nope. Workaround: do y. Relay this to the user: ...";
let mut result = tool_result("x", structured);
result.error = Some(result.content.clone());
mw.after_tool(&mut ctx(), &(), &mut result).await.unwrap();
assert_eq!(
result.content.matches("Workaround:").count(),
1,
"must not double-wrap: {}",
result.content
);
}

// ── TurnContextMiddleware config ────────────────────────────────────────

#[test]
Expand Down
107 changes: 107 additions & 0 deletions src/openhuman/tinyagents/policy_denial.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,29 @@
//! flows back into the turn the same way the unknown-tool corrective error is
//! surfaced to the model (see PR #4360).

use crate::openhuman::security::POLICY_BLOCKED_MARKER;
use crate::openhuman::tools::PermissionLevel;

/// Generic workaround for a raw security-policy / autonomy block. These denials
/// originate deep in the tools / `SecurityPolicy` layer (autonomy tier, command
/// classification, path checks) rather than the pluggable `ToolPolicy`, so there
/// is no single structured reason to lean on — point the agent at the levers that
/// actually unblock the family.
const SECURITY_POLICY_WORKAROUND: &str = "Raise the agent's access tier / autonomy \
(Settings → Agent access, or the `config.update_autonomy_settings` RPC / \
`[autonomy]` config) if this action should be allowed; otherwise reach the goal \
with a permitted (e.g. read-only) alternative, or report that it can't be done \
here.";

/// The boundary that blocked a tool call, with the context needed to explain it
/// and suggest a way forward.
pub(super) enum PolicyDenial<'a> {
/// A raw security-policy / autonomy denial emitted by the tools /
/// `SecurityPolicy` layer, recognised by its [`POLICY_BLOCKED_MARKER`] prefix.
/// It already states *what* and *why* but carries no workaround or relay
/// directive; [`render`](Self::render) keeps the marker and reason and appends
/// the missing guidance. `raw_reason` is the full marker-bearing tool output.
SecurityPolicyBlocked { tool: &'a str, raw_reason: &'a str },
/// The session tool policy forbids this tool for the channel's permission
/// tier (it is not in the allowed set).
SessionForbidden {
Expand Down Expand Up @@ -57,6 +75,24 @@ impl PolicyDenial<'_> {
/// message for the model.
pub(super) fn render(&self) -> String {
let (blocked, reason, workaround) = match self {
PolicyDenial::SecurityPolicyBlocked { tool, raw_reason } => {
// Strip the marker prefix from the reason (it is re-added on the
// `blocked` line so downstream `contains(POLICY_BLOCKED_MARKER)`
// checks — classification, the loop-breaker — keep matching).
let reason = raw_reason
.trim()
.strip_prefix(POLICY_BLOCKED_MARKER)
.unwrap_or(raw_reason)
.trim()
.to_string();
(
format!(
"{POLICY_BLOCKED_MARKER} Tool '{tool}' was blocked by the security policy"
),
reason,
SECURITY_POLICY_WORKAROUND.to_string(),
)
}
PolicyDenial::SessionForbidden {
tool,
required,
Expand Down Expand Up @@ -125,6 +161,33 @@ impl PolicyDenial<'_> {
}
}

/// Enrich a raw security-policy / autonomy tool result (issue #4094).
///
/// The pluggable-`ToolPolicy` and channel-permission denials are already rendered
/// with a `Blocked / Reason / Workaround / relay` shape by [`ToolPolicyMiddleware`]
/// (PR #4443). But the ~20 `[policy-blocked]` denials emitted deep in
/// `SecurityPolicy` / the tools themselves still return a bare marker line with no
/// workaround and no relay directive, so the agent dead-ends. This wraps such a
/// result into the structured form, appending the workaround + relay while keeping
/// the marker (so classification and the repeated-failure breaker still match).
///
/// Returns `None` — leaving the content untouched — when the result is not a raw
/// policy block, i.e. it lacks the marker, or it already carries a `Workaround:`
/// suffix (an already-structured `ToolPolicyMiddleware` denial that must not be
/// double-wrapped).
pub(super) fn maybe_enrich_policy_block(tool: &str, content: &str) -> Option<String> {
if !content.contains(POLICY_BLOCKED_MARKER) || content.contains("Workaround:") {
return None;
}
Some(
PolicyDenial::SecurityPolicyBlocked {
tool,
raw_reason: content,
}
.render(),
)
}

/// Workaround shared by the permission-tier denials: raise the channel's
/// agent-access tier, or fall back to a lower-permission tool.
fn raise_tier_workaround(
Expand Down Expand Up @@ -215,6 +278,50 @@ mod tests {
assert!(msg.contains("Relay this to the user"));
}

#[test]
fn security_policy_block_keeps_marker_and_adds_workaround_and_relay() {
let raw =
"[policy-blocked] Security policy: read-only mode — only read commands are allowed";
let msg = PolicyDenial::SecurityPolicyBlocked {
tool: "run_command",
raw_reason: raw,
}
.render();

// The marker survives so classification + the loop-breaker still match.
assert!(msg.contains(POLICY_BLOCKED_MARKER));
assert!(msg.starts_with("Blocked:"));
// The original reason is preserved (without a duplicated marker in it).
assert!(msg.contains("read-only mode — only read commands are allowed"));
assert!(msg.contains("Workaround:"));
assert!(msg.contains("agent-access tier / autonomy") || msg.contains("Agent access"));
assert!(msg.contains("Relay this to the user"));
}

#[test]
fn maybe_enrich_only_touches_raw_marker_results() {
// A raw marker line with no workaround → enriched.
let raw = "[policy-blocked] Command not allowed by security policy: rm -rf /";
let enriched = maybe_enrich_policy_block("run_command", raw)
.expect("a raw policy block should be enriched");
assert!(enriched.contains("Workaround:"));
assert!(enriched.contains("Relay this to the user"));
assert!(enriched.contains(POLICY_BLOCKED_MARKER));

// An already-structured ToolPolicyMiddleware denial (has "Workaround:") is
// left alone — no double-wrapping.
let already = PolicyDenial::PolicyDenied {
tool: "run_script",
policy: "sandbox",
reason: "sandbox restriction",
}
.render();
assert!(maybe_enrich_policy_block("run_script", &already).is_none());

// A plain non-policy error is untouched.
assert!(maybe_enrich_policy_block("read_file", "Error: file not found").is_none());
}

#[test]
fn approval_required_suggests_approval_then_retry() {
let msg = PolicyDenial::ApprovalRequired {
Expand Down
2 changes: 2 additions & 0 deletions tests/config_auth_app_state_connectivity_e2e.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2802,6 +2802,7 @@ async fn worker_a_controller_schemas_are_fully_exposed() {
"openhuman.config_get_meet_settings",
"openhuman.config_get_memory_sync_settings",
"openhuman.config_get_onboarding_completed",
"openhuman.config_get_privacy_mode",
"openhuman.config_get_runtime_flags",
"openhuman.config_get_sandbox_settings",
"openhuman.config_get_search_settings",
Expand All @@ -2811,6 +2812,7 @@ async fn worker_a_controller_schemas_are_fully_exposed() {
"openhuman.config_resolve_api_url",
"openhuman.config_set_browser_allow_all",
"openhuman.config_set_onboarding_completed",
"openhuman.config_set_privacy_mode",
"openhuman.config_set_super_context_enabled",
"openhuman.config_update_activity_level_settings",
"openhuman.config_update_agent_paths",
Expand Down
Loading