fix(flows): close Composio tool_call tier-gate bypass (C1) (#5000)

src/openhuman/flows/ops.rs

@@ -366,6 +366,28 @@ pub(crate) fn graph_has_outbound_side_effect(graph: &WorkflowGraph) -> bool {
     })
 }
 
+/// Shared Rule 2 enforcement (issue B29, and its `flows_update` compound-bypass
+/// closure): forces `require_approval` to `true` when `graph` contains an
+/// outbound side-effect node, no matter what the caller asked for. Used by both
+/// [`flows_create`] and [`flows_update`] so a flow can never persist
+/// `require_approval: false` alongside a `tool_call` / `http_request` / `code`
+/// node — on create OR on a later edit that *adds* such a node to a
+/// previously-read-only graph.
+///
+/// Returns `(effective_require_approval, was_forced)`: `was_forced` is `true`
+/// only when the caller's own toggle was `false` but a side-effect node
+/// required the override — callers use it to decide whether to emit the
+/// loud "forced to true" log/result note.
+pub(crate) fn enforce_side_effect_approval(
+    graph: &WorkflowGraph,
+    caller_require_approval: bool,
+) -> (bool, bool) {
+    let has_side_effect = graph_has_outbound_side_effect(graph);
+    let effective_require_approval = caller_require_approval || has_side_effect;
+    let was_forced = has_side_effect && !caller_require_approval;
+    (effective_require_approval, was_forced)
+}
+
 /// Whether `graph` has anything for [`flows_run`] to actually *do* — i.e. at
 /// least one non-`trigger` node **reachable from the trigger** by following
 /// directed edges. A graph made of nothing but a bare `trigger` node (or a
@@ -2178,9 +2200,9 @@ pub async fn flows_create(
 
     // Rule 2: any outbound side-effect node forces require_approval, no
     // matter what the caller asked for.
-    let has_side_effect = graph_has_outbound_side_effect(&graph);
-    let effective_require_approval = require_approval || has_side_effect;
-    if has_side_effect && !require_approval {
+    let (effective_require_approval, side_effect_forced) =
+        enforce_side_effect_approval(&graph, require_approval);
+    if side_effect_forced {
         tracing::info!(
             target: "flows",
             %name,
@@ -2218,7 +2240,7 @@ pub async fn flows_create(
              Enable it explicitly (flows_set_enabled) when you are ready for it to fire."
         ));
     }
-    if effective_require_approval && !require_approval {
+    if side_effect_forced {
         logs.push(
             "require_approval forced to true because the graph contains outbound side-effect \
              nodes (tool_call / http_request / code)."
@@ -2638,7 +2660,31 @@ pub async fn flows_update(
         "[flows] flows_update: auto-trigger disarm decision inputs"
     );
 
-    tracing::debug!(target: "flows", flow_id = %id, has_expected = expected_version.is_some(), "[flows] flows_update: persisting changes");
+    // Rule 2 analogue (compound-bypass closure): re-apply the same outbound
+    // side-effect check `flows_create` applies on save — via the shared
+    // [`enforce_side_effect_approval`] helper — so an update that *adds* a
+    // tool_call/http_request/code node to a previously read-only graph can
+    // never persist `require_approval: false` just because the update path
+    // trusted the caller's toggle unconditionally.
+    let (effective_require_approval, side_effect_forced) =
+        enforce_side_effect_approval(&graph, new_require_approval);
+    if side_effect_forced {
+        tracing::info!(
+            target: "flows",
+            flow_id = %id,
+            "[flows] flows_update: forcing require_approval=true — graph contains outbound \
+             side-effect node(s) (tool_call / http_request / code)"
+        );
+    }
+
+    tracing::debug!(
+        target: "flows",
+        flow_id = %id,
+        has_expected = expected_version.is_some(),
+        require_approval = effective_require_approval,
+        side_effect_forced,
+        "[flows] flows_update: persisting changes"
+    );
     // `enabled_override` is threaded into the same guarded UPDATE as the
     // graph/name/require_approval write (see `store::update_flow_graph`)
     // rather than a follow-up `flows_set_enabled` call, so the disarm can
@@ -2648,7 +2694,7 @@ pub async fn flows_update(
         id,
         new_name,
         graph,
-        new_require_approval,
+        effective_require_approval,
         enabled_override,
         expected_version.as_deref(),
     )
@@ -2683,6 +2729,13 @@ pub async fn flows_update(
                 .to_string(),
         );
     }
+    if side_effect_forced {
+        logs.push(
+            "require_approval forced to true because the graph contains outbound side-effect \
+             nodes (tool_call / http_request / code)."
+                .to_string(),
+        );
+    }
     Ok(RpcOutcome::new(updated, logs))
 }
 

src/openhuman/flows/ops_tests.rs

@@ -4337,6 +4337,77 @@ async fn flows_create_schedule_outbound_creates_disabled_and_approval() {
     );
 }
 
+#[tokio::test]
+async fn flows_update_forces_require_approval_when_adding_side_effect_nodes() {
+    // Compound bypass fix, half 2: `flows_create`'s Rule 2 (force
+    // require_approval when the graph gains an outbound side-effect node)
+    // must also re-apply on `flows_update` — a flow that starts read-only and
+    // is later edited to add a Composio/http_request/code node must not be
+    // able to keep require_approval=false just because the update path never
+    // re-checked.
+    let tmp = TempDir::new().unwrap();
+    let config = test_config(&tmp);
+    let created = flows_create(&config, "demo".to_string(), trigger_only_graph(), false)
+        .await
+        .unwrap();
+    assert!(
+        !created.value.require_approval,
+        "a trigger-only graph must not force require_approval on create"
+    );
+
+    let updated = flows_update(
+        &config,
+        &created.value.id,
+        None,
+        Some(tool_call_graph()),
+        Some(false),
+        None,
+    )
+    .await
+    .unwrap();
+
+    assert!(
+        updated.value.require_approval,
+        "flows_update must force require_approval when the replacement graph adds an outbound \
+         side-effect node (tool_call), even though the caller passed false"
+    );
+    assert!(
+        updated
+            .logs
+            .iter()
+            .any(|l| l.contains("require_approval forced to true")),
+        "flows_update must loudly log the forced require_approval: {:?}",
+        updated.logs
+    );
+}
+
+#[tokio::test]
+async fn flows_update_does_not_force_require_approval_on_readonly_graph() {
+    let tmp = TempDir::new().unwrap();
+    let config = test_config(&tmp);
+    let created = flows_create(&config, "demo".to_string(), trigger_only_graph(), false)
+        .await
+        .unwrap();
+    assert!(!created.value.require_approval);
+
+    // Name-only update — no graph change, no side-effect nodes.
+    let updated = flows_update(
+        &config,
+        &created.value.id,
+        Some("renamed".to_string()),
+        None,
+        None,
+        None,
+    )
+    .await
+    .unwrap();
+
+    assert!(
+        !updated.value.require_approval,
+        "a name-only update to a read-only graph must not force require_approval"
+    );
+}
+
 // ── graph_has_outbound_side_effect / trigger_is_automatic helper tests ────
 
 #[test]

src/openhuman/tinyflows/caps.rs

@@ -1616,6 +1616,7 @@ async fn resolve_composio_account(
 /// // approval when a trigger-driven run's tool/http config contains `=`-exprs.
 pub struct OpenHumanTools {
     pub config: Arc<Config>,
+    pub security: Arc<SecurityPolicy>,
 }
 
 /// Prefix marking a `tool_call` node's slug as a NATIVE OpenHuman tool (the
@@ -2562,18 +2563,13 @@ impl ToolInvoker for OpenHumanTools {
                 ));
             }
 
-            let security = SecurityPolicy::from_config(
-                &self.config.autonomy,
-                &self.config.workspace_dir,
-                &self.config.action_dir,
-            );
             let class = crate::openhuman::runtime_node::ops::classify_tool_call(
                 &self.config,
                 tool_name,
                 &args,
             )
             .map_err(EngineError::Capability)?;
-            let tier_decision = enforce_node_tier_gate(&security, class, "tool_call")?;
+            let tier_decision = enforce_node_tier_gate(&self.security, class, "tool_call")?;
             let summary = crate::openhuman::approval::summarize_action(tool_name, &args);
             let redacted = crate::openhuman::approval::redact_args(&args);
             let (outcome, _request_id) =
@@ -2601,6 +2597,23 @@ impl ToolInvoker for OpenHumanTools {
             });
         }
 
+        // Autonomy-tier gate (Phase 2): a Composio action reaches a
+        // third-party API over the network, so it is Network-class — same
+        // class as `http_request`. A read-only run `Block`s here and never
+        // reaches curation, the preflight, or the approval gate;
+        // Supervised/Full fall through to `gate_call_for_tier` below. Runs
+        // BEFORE the curation check (unlike the pre-fix behavior) so a
+        // read-only tier can never even probe which slugs are curated.
+        let tier_decision =
+            enforce_node_tier_gate(&self.security, CommandClass::Network, "tool_call")?;
+        tracing::debug!(
+            target: "flows",
+            %slug,
+            ?tier_decision,
+            tier = ?self.security.autonomy,
+            "[flows] tool_call: composio node tier gate evaluated"
+        );
+
         // Curation + scope gate — hard allowlist (see [`is_curated_flow_tool`]'s
         // doc for why this differs from the general agent tool-call path).
         // Runs before anything else — a rejected slug never reaches the
@@ -2623,22 +2636,24 @@ impl ToolInvoker for OpenHumanTools {
         // provider or asks the user to approve a call that cannot succeed.
         preflight_composio_args(&self.config, slug, &args).await?;
 
-        // Approval gate (see the struct doc). Mirrors
-        // `tinyagents/middleware.rs::ApprovalSecurityMiddleware::wrap_tool`'s
-        // shape exactly: compute summary/redacted args only when a gate is
-        // installed, deny short-circuits before any composio call, allow
-        // records an audit id to close out after the call resolves.
-        let mut audit_id: Option<String> = None;
-        if let Some(gate) = crate::openhuman::approval::ApprovalGate::try_global() {
-            let summary = crate::openhuman::approval::summarize_action(slug, &args);
-            let redacted = crate::openhuman::approval::redact_args(&args);
-            let (outcome, request_id) = gate.intercept_audited(slug, &summary, redacted).await;
-            match outcome {
-                crate::openhuman::approval::GateOutcome::Deny { reason } => {
-                    return Err(EngineError::Capability(reason));
-                }
-                crate::openhuman::approval::GateOutcome::Allow => audit_id = request_id,
-            }
+        // Approval gate (see the struct doc). Mirrors `OpenHumanHttp::request`'s
+        // shape exactly: `gate_call_for_tier` is what actually performs the
+        // `Prompt` round-trip — it escalates a Supervised `Prompt` decision
+        // into a forced approval regardless of the flow's own
+        // `require_approval` toggle (Codex P1), same as the `http_request` and
+        // `code` node paths. Deny short-circuits before any composio call;
+        // Allow records an audit id to close out after the call resolves.
+        let summary = crate::openhuman::approval::summarize_action(slug, &args);
+        let redacted = crate::openhuman::approval::redact_args(&args);
+        let (outcome, audit_id) = gate_call_for_tier(tier_decision, slug, &summary, redacted).await;
+        if let crate::openhuman::approval::GateOutcome::Deny { reason } = outcome {
+            tracing::warn!(
+                target: "flows",
+                %slug,
+                ?tier_decision,
+                "[flows] tool_call: approval gate denied before Composio dispatch"
+            );
+            return Err(EngineError::Capability(reason));
         }
 
         let kind = create_composio_client(&self.config)
@@ -3277,6 +3292,7 @@ pub fn build_capabilities(config: Arc<Config>, state_namespace: impl Into<String
         }),
         tools: Arc::new(OpenHumanTools {
             config: config.clone(),
+            security: security.clone(),
         }),
         http: Arc::new(OpenHumanHttp {
             security: security.clone(),
@@ -4171,6 +4187,35 @@ mod tests {
         }
     }
 
+    /// End-to-end at the adapter: a Composio `tool_call` node under a
+    /// read-only tier is refused BEFORE it ever reaches the curation gate or
+    /// any Composio dispatch — closes the compound bypass where the Composio
+    /// branch of `OpenHumanTools::invoke` reached `intercept_audited` without
+    /// ever consulting the autonomy tier, unlike the native `oh:`,
+    /// `http_request`, and `code` node paths, which all gate on tier first.
+    #[tokio::test]
+    async fn composio_tool_call_blocks_under_readonly_tier() {
+        use crate::openhuman::security::AutonomyLevel;
+
+        let tools = OpenHumanTools {
+            config: Arc::new(Config::default()),
+            security: Arc::new(policy(AutonomyLevel::ReadOnly)),
+        };
+
+        let err = tools
+            .invoke("SLACK_SEND_MESSAGE", json!({}), None)
+            .await
+            .expect_err("read-only tier must block a Composio tool_call node before dispatch");
+        if let EngineError::Capability(msg) = err {
+            assert!(
+                msg.contains(POLICY_BLOCKED_MARKER),
+                "expected a policy-blocked refusal, got: {msg}"
+            );
+        } else {
+            panic!("expected EngineError::Capability");
+        }
+    }
+
     // ── Codex P1: Prompt-tier decisions must escalate past a workflow's own
     // require_approval=false default, never silently auto-allow ────────────
 

src/openhuman/tinyflows/tests.rs

@@ -230,7 +230,12 @@ async fn code_adapter_javascript_passthrough_round_trips_json() {
 // without any global state.
 
 fn tools_adapter(config: Arc<Config>) -> OpenHumanTools {
-    OpenHumanTools { config }
+    let security = Arc::new(SecurityPolicy::from_config(
+        &config.autonomy,
+        &config.workspace_dir,
+        &config.action_dir,
+    ));
+    OpenHumanTools { config, security }
 }
 
 #[tokio::test]

tests/json_rpc_e2e.rs

@@ -12862,6 +12862,103 @@ async fn json_rpc_flows_lifecycle_round_trip() {
     rpc_join.abort();
 }
 
+/// JSON-RPC regression for the Rule 2 compound-bypass fix (C1): `flows_update`
+/// must re-derive `require_approval` from the *effective* graph, not trust the
+/// caller's raw toggle. This is the RPC-layer counterpart to the direct-API
+/// tests `flows_update_forces_require_approval_when_adding_side_effect_nodes`
+/// / `flows_update_does_not_force_require_approval_on_readonly_graph` in
+/// `src/openhuman/flows/ops_tests.rs` — same rule, exercised through the
+/// `openhuman.flows_update` controller (schema + handler wiring), not just
+/// the `ops::flows_update` fn directly.
+#[cfg(feature = "flows")]
+#[tokio::test]
+async fn json_rpc_flows_update_forces_require_approval_on_side_effect_graph() {
+    let _env_lock = json_rpc_e2e_env_lock();
+    let (rpc_base, _tmp, api_join, rpc_join, _guards) = boot_flows_rpc_env().await;
+
+    // 1. Create a trigger-only (read-only) flow with require_approval: false.
+    let create = post_json_rpc(
+        &rpc_base,
+        9401,
+        "openhuman.flows_create",
+        json!({
+            "name": "rpc-rule2-demo",
+            "graph": {
+                "name": "trigger-only",
+                "nodes": [ { "id": "t", "kind": "trigger", "name": "Trigger" } ],
+                "edges": []
+            },
+            "require_approval": false
+        }),
+    )
+    .await;
+    let flow = peel_logs_envelope(assert_no_jsonrpc_error(&create, "flows_create"));
+    let flow_id = flow
+        .get("id")
+        .and_then(Value::as_str)
+        .expect("flow id in create result")
+        .to_string();
+    assert_eq!(
+        flow.get("require_approval").and_then(Value::as_bool),
+        Some(false),
+        "a trigger-only graph must not force require_approval on create"
+    );
+
+    // 2. Update to a graph with an outbound Composio tool_call node, still
+    // passing require_approval: false — the RPC handler must force it to
+    // true rather than trust the caller's toggle.
+    let update = post_json_rpc(
+        &rpc_base,
+        9402,
+        "openhuman.flows_update",
+        json!({
+            "id": flow_id,
+            "graph": {
+                "name": "with-tool-call",
+                "nodes": [
+                    { "id": "t", "kind": "trigger", "name": "Trigger" },
+                    {
+                        "id": "post",
+                        "kind": "tool_call",
+                        "name": "Post",
+                        "config": { "slug": "SLACK_SEND_MESSAGE", "args": { "channel": "general" } }
+                    }
+                ],
+                "edges": [ { "from_node": "t", "to_node": "post" } ]
+            },
+            "require_approval": false
+        }),
+    )
+    .await;
+    let updated = peel_logs_envelope(assert_no_jsonrpc_error(&update, "flows_update"));
+    assert_eq!(
+        updated.get("require_approval").and_then(Value::as_bool),
+        Some(true),
+        "flows_update over JSON-RPC must force require_approval=true when the \
+         replacement graph adds an outbound side-effect node, even though the \
+         caller passed false"
+    );
+
+    // 3. The forced value must also be what's persisted, not just what's
+    // echoed back in the update response.
+    let get = post_json_rpc(
+        &rpc_base,
+        9403,
+        "openhuman.flows_get",
+        json!({ "id": flow_id }),
+    )
+    .await;
+    let persisted = peel_logs_envelope(assert_no_jsonrpc_error(&get, "flows_get"));
+    assert_eq!(
+        persisted.get("require_approval").and_then(Value::as_bool),
+        Some(true),
+        "the forced require_approval must be persisted, not just returned"
+    );
+
+    api_join.abort();
+    rpc_join.abort();
+}
+
 /// Flow Scout suggestion-lifecycle methods over JSON-RPC (no LLM involved):
 /// `flows_list_suggestions` starts empty, and `flows_dismiss_suggestion` /
 /// `flows_mark_suggestion_built` on an unknown id resolve cleanly and report