fix(flows): gate live-run tool on flows_build builder path (#4593) (#4606)

src/openhuman/agent/harness/session/runtime.rs

@@ -236,6 +236,37 @@ impl Agent {
         self.rebuild_tool_policy_session();
     }
 
+    /// Remove `names` from the main agent's callable set for this session,
+    /// leaving every other currently-visible tool untouched.
+    ///
+    /// The hidden names resolve to `Deny` at the tool-call boundary (via the
+    /// rebuilt [`ToolPolicySession`]), not merely absent from the prompt — a
+    /// hard execution guarantee even if the model requests the tool anyway.
+    ///
+    /// When the session currently has *no* visible-tool filter (empty set =
+    /// "all visible"), the filter is first seeded from every registered tool
+    /// spec so hiding actually **restricts** the set rather than no-opping into
+    /// the still-"all visible" empty state. Used by callers that need to drop a
+    /// specific dangerous tool from an otherwise-unchanged belt (e.g. the
+    /// `flows_build` builder path dropping the live-run `run_flow` tool).
+    ///
+    /// Caveat: because an empty set is the "all visible" sentinel, hiding *every*
+    /// remaining tool collapses back to "all visible". Callers use this to drop
+    /// a handful of tools from a much larger belt, where that can't happen.
+    pub fn hide_tools(&mut self, names: &[&str]) {
+        if self.visible_tool_names.is_empty() {
+            self.visible_tool_names = self
+                .tool_specs
+                .iter()
+                .map(|spec| spec.name.clone())
+                .collect();
+        }
+        for name in names {
+            self.visible_tool_names.remove(*name);
+        }
+        self.rebuild_tool_policy_session();
+    }
+
     pub(super) fn rebuild_tool_policy_session(&mut self) {
         self.tool_policy_session = ToolPolicyEngine::build_session(
             &self.agent_definition_name,

src/openhuman/agent/harness/session/tests.rs

@@ -1256,3 +1256,56 @@ fn bound_cached_transcript_messages_snaps_past_leading_orphan_tool() {
         vec!["u2", "a2", "u3"]
     );
 }
+
+/// `hide_tools` on an agent that already has a visible-tool filter must drop
+/// only the named tools and leave the rest of the belt intact.
+#[test]
+fn hide_tools_drops_named_from_existing_filter() {
+    let mut agent = build_minimal_agent_with_definition_name(None);
+    agent.set_visible_tool_names(
+        ["alpha".to_string(), "beta".to_string(), "echo".to_string()]
+            .into_iter()
+            .collect(),
+    );
+
+    agent.hide_tools(&["echo"]);
+
+    let visible = agent.visible_tool_names_for_test();
+    assert!(visible.contains("alpha") && visible.contains("beta"));
+    assert!(
+        !visible.contains("echo"),
+        "hidden tool must be removed from the existing filter; visible = {visible:?}"
+    );
+}
+
+/// `hide_tools` on an agent with *no* filter (empty set = "all visible") must
+/// first seed the allowlist from every registered spec so the hide actually
+/// restricts — otherwise removing from an empty set would no-op and leave the
+/// tool still callable under the "empty == all visible" contract.
+#[test]
+fn hide_tools_seeds_allowlist_when_no_filter_present() {
+    let mut agent = build_minimal_agent_with_definition_name(None);
+    assert!(
+        agent.visible_tool_names_for_test().is_empty(),
+        "precondition: a freshly built minimal agent has no visible-tool filter"
+    );
+    assert!(
+        agent.tool_specs().iter().any(|spec| spec.name == "echo"),
+        "precondition: the mock belt includes `echo`"
+    );
+
+    // Hiding a name that isn't on the belt still forces the seed: the set goes
+    // from empty ("all visible") to a concrete allowlist of the real tools, so
+    // the previously-all-visible belt is now explicitly enumerated.
+    agent.hide_tools(&["not_on_belt"]);
+
+    let visible = agent.visible_tool_names_for_test();
+    assert!(
+        visible.contains("echo"),
+        "seeding must materialise the existing belt into a concrete allowlist; visible = {visible:?}"
+    );
+    assert!(
+        !visible.contains("not_on_belt"),
+        "an absent hidden name is a harmless no-op; visible = {visible:?}"
+    );
+}

src/openhuman/flows/ops.rs

@@ -2624,6 +2624,47 @@ pub async fn flows_discover(
 /// the RPC block indefinitely.
 const FLOW_BUILD_TIMEOUT_SECS: u64 = 300;
 
+/// Tools stripped from the `workflow_builder` belt on the direct `flows_build`
+/// RPC path (issue #4593).
+///
+/// `flows_build` runs the builder under [`AgentTurnOrigin::Cli`] so the approval
+/// gate does not fail-closed in a headless/streamed run — but that same origin
+/// makes [`crate::openhuman::approval::ApprovalGate`] **auto-allow** every
+/// `external_effect` tool. The flows live-runner (`run_flow`,
+/// [`crate::openhuman::flows::tools`]'s `RunFlowTool`) executes a *live* saved
+/// flow (real Slack/Gmail/HTTP/code effects via [`flows_run`]), so a stray call
+/// during an authoring turn would fire it with no HITL confirmation. This path
+/// has no routable approval surface yet (the copilot stream carries only a
+/// broadcast `thread_id`, no per-user `client_id`), so rather than
+/// park-then-TTL-deny we make it **unreachable** here — matching `flows_build`'s
+/// contract that it "never enables or runs a flow". The tool stays available
+/// (and properly gated behind a real `WebChat` approval card) when
+/// `workflow_builder` is invoked as the `build_workflow` chat delegate.
+///
+/// `run_flow` is the live-runner on the belt today. The legacy `run_workflow`
+/// name (now the unrelated harness spawn tool) is listed too as belt-and-braces
+/// against a re-rename or the name ever leaking back onto this belt;
+/// `hide_tools` no-ops on a name that isn't present.
+const FLOWS_BUILD_HIDDEN_TOOLS: &[&str] = &["run_workflow", "run_flow"];
+
+/// Strip the live-run tool(s) in [`FLOWS_BUILD_HIDDEN_TOOLS`] from `agent`'s
+/// callable set for the direct `flows_build` RPC path.
+///
+/// Delegates to [`crate::openhuman::agent::Agent::hide_tools`], which removes
+/// the names from the builder's (already narrow) visible belt and rebuilds the
+/// session's `ToolPolicySession` so they resolve to `Deny` at the tool-call
+/// boundary — a hard execution guarantee even if the model requests the tool.
+/// The authoring tools (`propose`/`revise`/`save`/`dry_run`/reads) are all
+/// `external_effect() == false` and untouched, so the turn never fail-closes.
+fn restrict_builder_toolset(agent: &mut crate::openhuman::agent::Agent) {
+    tracing::debug!(
+        target: "flows",
+        hidden = ?FLOWS_BUILD_HIDDEN_TOOLS,
+        "[flows] flows_build: hiding live-run tools from builder belt"
+    );
+    agent.hide_tools(FLOWS_BUILD_HIDDEN_TOOLS);
+}
+
 /// Runs the `workflow_builder` agent for one authoring turn and returns its
 /// proposal, invoking it as a first-class backend agent (exactly like the Flow
 /// Scout `flows_discover`) rather than routing a hand-crafted delegate prompt
@@ -2670,6 +2711,14 @@ pub async fn flows_build(
         .map_err(|e| format!("failed to build workflow_builder agent: {e:#}"))?;
     agent.set_agent_definition_name("workflow_builder".to_string());
 
+    // Strip the live-run tool(s) from the belt on this direct RPC path: under
+    // the `AgentTurnOrigin::Cli` origin below the approval gate auto-allows
+    // every external_effect tool, so `run_flow` could execute a live saved flow
+    // with no HITL confirmation (issue #4593). Restricting the visible set makes
+    // it `Deny` at the tool-call boundary; the authoring tools are untouched so
+    // the turn still runs headless without fail-closing.
+    restrict_builder_toolset(&mut agent);
+
     // When a chat thread is attached (the copilot pane), stream the builder turn
     // into it exactly like an interactive turn — text/tool deltas and the
     // `propose_workflow` tool result the frontend renders as a proposal card.

src/openhuman/flows/ops_tests.rs

@@ -2438,3 +2438,69 @@ fn finalize_terminal_status_no_error_when_clean() {
     assert_eq!(status, "completed");
     assert_eq!(error, None);
 }
+
+/// Regression for issue #4593: the `flows_build` builder turn runs under
+/// `AgentTurnOrigin::Cli`, which makes the `ApprovalGate` auto-allow every
+/// `external_effect` tool. The flows live-runner executes a *live* saved flow,
+/// so it must be unreachable on this path — `restrict_builder_toolset` drops it
+/// from the builder's callable belt while leaving the authoring tools in place
+/// so the turn still functions (never fail-closes).
+#[tokio::test]
+async fn flows_build_hides_the_live_run_tool_from_the_builder_belt() {
+    let tmp = TempDir::new().unwrap();
+    let config = test_config(&tmp);
+
+    // Document WHY the live-runner must be hidden: running a saved flow fires
+    // real Slack/Gmail/HTTP/code effects, so it is an external-effect tool. This
+    // pins that invariant independently of belt name-resolution so the
+    // hide-list can't silently stop covering a live-run tool.
+    use crate::openhuman::tools::Tool as _;
+    let live_runner =
+        crate::openhuman::flows::tools::RunFlowTool::new(std::sync::Arc::new(config.clone()));
+    assert!(
+        live_runner.external_effect(),
+        "the flows live-runner must be external-effect for the #4593 concern to apply"
+    );
+
+    crate::openhuman::agent::harness::AgentDefinitionRegistry::init_global(&config.workspace_dir)
+        .expect("agent registry init");
+    let mut agent =
+        crate::openhuman::agent::Agent::from_config_for_agent(&config, "workflow_builder")
+            .expect("build workflow_builder agent");
+    agent.set_agent_definition_name("workflow_builder".to_string());
+
+    // Precondition: the builder advertises the live-run tool (`run_flow`) on its
+    // belt before restriction — the exact tool #4593 is about.
+    assert!(
+        agent.visible_tool_names_for_test().contains("run_flow"),
+        "precondition: workflow_builder belt should advertise the live-run tool `run_flow`; \
+         visible = {:?}",
+        agent.visible_tool_names_for_test()
+    );
+
+    restrict_builder_toolset(&mut agent);
+
+    // After restriction neither the current name nor the post-rename name is
+    // callable on the flows_build path — the hide-list covers both (#4593).
+    let visible = agent.visible_tool_names_for_test();
+    for hidden in ["run_workflow", "run_flow"] {
+        assert!(
+            !visible.contains(hidden),
+            "live-run tool `{hidden}` must be hidden on the flows_build path; visible = {visible:?}"
+        );
+    }
+    // Authoring / read tools stay reachable so the builder turn still works
+    // headlessly under the CLI origin (no fail-close).
+    for keep in [
+        "propose_workflow",
+        "revise_workflow",
+        "save_workflow",
+        "dry_run_workflow",
+        "list_flows",
+    ] {
+        assert!(
+            visible.contains(keep),
+            "authoring tool `{keep}` must remain visible after restriction; visible = {visible:?}"
+        );
+    }
+}