From 60560a36715cf26f46cf2039d34b2f4f8ad60063 Mon Sep 17 00:00:00 2001 From: CodeGhost21 <164498022+CodeGhost21@users.noreply.github.com> Date: Tue, 7 Jul 2026 03:39:28 +0530 Subject: [PATCH] fix(flows): gate live-run tool on flows_build builder path (#4593) (#4606) --- .../agent/harness/session/runtime.rs | 31 +++++++++ src/openhuman/agent/harness/session/tests.rs | 53 +++++++++++++++ src/openhuman/flows/ops.rs | 49 ++++++++++++++ src/openhuman/flows/ops_tests.rs | 66 +++++++++++++++++++ 4 files changed, 199 insertions(+) diff --git a/src/openhuman/agent/harness/session/runtime.rs b/src/openhuman/agent/harness/session/runtime.rs index c39173012..4ea75c478 100644 --- a/src/openhuman/agent/harness/session/runtime.rs +++ b/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, diff --git a/src/openhuman/agent/harness/session/tests.rs b/src/openhuman/agent/harness/session/tests.rs index b2063a568..75142b77e 100644 --- a/src/openhuman/agent/harness/session/tests.rs +++ b/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:?}" + ); +} diff --git a/src/openhuman/flows/ops.rs b/src/openhuman/flows/ops.rs index 0de76055e..8c977492a 100644 --- a/src/openhuman/flows/ops.rs +++ b/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. diff --git a/src/openhuman/flows/ops_tests.rs b/src/openhuman/flows/ops_tests.rs index 8f1370290..1c3ba423b 100644 --- a/src/openhuman/flows/ops_tests.rs +++ b/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:?}" + ); + } +}