fix(flows): save-safety — no silent live-arming on update, flag empty runs, close resume_flow_run HITL bypass

src/openhuman/flows/ops.rs

@@ -366,6 +366,24 @@ pub(crate) fn graph_has_outbound_side_effect(graph: &WorkflowGraph) -> bool {
     })
 }
 
+/// Whether `graph` has anything for [`flows_run`] to actually *do* — i.e. at
+/// least one non-`trigger` node, wired up by at least one edge. A graph made
+/// of nothing but a bare `trigger` node (or a `trigger` plus unreachable/
+/// disconnected nodes with no edges at all) can compile and "run" cleanly
+/// while producing no work whatsoever — the exact live finding this guards:
+/// a trigger-only flow reported `status="completed" pending_approvals=0`
+/// having done nothing, which reads as a successful automation to anyone not
+/// staring at the node count. Used by `flows_run` to attach a
+/// human-readable note to an otherwise-silent "success".
+pub(crate) fn graph_has_actionable_nodes(graph: &WorkflowGraph) -> bool {
+    let non_trigger_nodes = graph
+        .nodes
+        .iter()
+        .filter(|n| n.kind != NodeKind::Trigger)
+        .count();
+    non_trigger_nodes > 0 && !graph.edges.is_empty()
+}
+
 /// Produces host-side, **non-fatal** validation warnings for a graph — today
 /// exactly one: "this trigger kind does not fire automatically yet". Returns
 /// an empty vec when the trigger fires (`manual`/`schedule`/`app_event`), when
@@ -2462,6 +2480,26 @@ fn map_flow_update_error(e: store::FlowUpdateError) -> String {
 /// old cadence, or a newly-added schedule would never get bound at all.
 /// Skipped entirely for a name/`require_approval`-only update (no
 /// `graph_json` supplied), since the trigger definitely didn't change.
+///
+/// **B29 Rule 1 analogue for saves** (save/enable safety — same issue
+/// `flows_create` guards at creation time, see its doc): `flows_create`
+/// refuses to persist an automatic-trigger graph (`schedule` / `app_event` /
+/// `webhook`, see [`trigger_is_automatic`]) as `enabled`, but that guard only
+/// runs once, at creation. Without an equivalent here, a flow created
+/// `enabled: true` with a manual/no-op trigger could later have an
+/// automatic-trigger graph saved onto it — via the `save_workflow` agent
+/// tool, the canvas Save button, a proposal apply, or any other
+/// `flows_update` caller — and go LIVE immediately with no user review
+/// (confirmed live: a flow started firing on an unreviewed 8am schedule).
+/// So: when the *new* graph's trigger is automatic, the flow is *currently*
+/// enabled, and the *previous* graph's trigger was NOT automatic (a
+/// manual/none → automatic transition), this forces the persisted `enabled`
+/// back to `false` in the same store write — the user must explicitly
+/// re-arm via `flows_set_enabled` after reviewing the new trigger.
+/// Deliberately narrower than Rule 1's at-create version: a flow that was
+/// already an enabled *automatic*-trigger flow being legitimately re-edited
+/// (e.g. tweaking a cron expression) is left alone — the user already opted
+/// in once, and re-disarming on every edit would just be friction.
 pub async fn flows_update(
     config: &Config,
     id: &str,
@@ -2485,17 +2523,47 @@ pub async fn flows_update(
         }
     };
 
+    // B29 Rule 1 analogue: only disarm a manual/none → automatic transition
+    // on an already-enabled flow. An automatic → automatic re-edit, or a
+    // flow that isn't enabled to begin with, is untouched.
+    let was_auto = trigger_is_automatic(&existing.graph);
+    let now_auto = trigger_is_automatic(&graph);
+    let should_disarm = now_auto && existing.enabled && !was_auto;
+    let enabled_override = should_disarm.then_some(false);
+    tracing::debug!(
+        target: "flows",
+        flow_id = %id,
+        was_auto,
+        now_auto,
+        currently_enabled = existing.enabled,
+        should_disarm,
+        "[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");
+    // `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
+    // never race a concurrent read/write of `enabled`.
     let updated = store::update_flow_graph(
         config,
         id,
         new_name,
         graph,
         new_require_approval,
+        enabled_override,
         expected_version.as_deref(),
     )
     .map_err(map_flow_update_error)?;
 
+    if should_disarm {
+        tracing::info!(
+            target: "flows",
+            flow_id = %id,
+            "[flows] flows_update: auto-disabled — graph changed manual→automatic trigger on an enabled flow"
+        );
+    }
+
     if graph_changed && updated.enabled {
         let trigger_unchanged = bus::extract_trigger_kind(&existing)
             == bus::extract_trigger_kind(&updated)
@@ -2508,10 +2576,16 @@ pub async fn flows_update(
     }
 
     publish_flow_changed(id, "updated", "system");
-    Ok(RpcOutcome::single_log(
-        updated,
-        format!("flow updated: {id}"),
-    ))
+    let mut logs = vec![format!("flow updated: {id}")];
+    if should_disarm {
+        logs.push(
+            "Flow was auto-disabled because its trigger changed from manual to automatic \
+             (schedule / app_event / webhook). Enable it explicitly (flows_set_enabled) once \
+             you've reviewed the new trigger."
+                .to_string(),
+        );
+    }
+    Ok(RpcOutcome::new(updated, logs))
 }
 
 /// Lists a flow's revision history (prior graph snapshots), newest first,
@@ -2856,6 +2930,24 @@ pub async fn flows_run(
         .map_err(|e| e.to_string())?
         .ok_or_else(|| format!("flow '{flow_id}' not found"))?;
 
+    // Live finding: a graph with no actionable nodes (only a `trigger`, or a
+    // `trigger` plus nodes with no edges wiring them up) compiles and "runs"
+    // cleanly but does nothing — and previously reported
+    // `status="completed" pending_approvals=0` indistinguishably from a real
+    // run, reading as "triggered but nothing happened" was actually a
+    // success. Surface it loudly instead of letting it pass silently: warn
+    // now (independent of how the run below turns out), and attach a
+    // human-readable note to the returned outcome so the UI can show
+    // "nothing to run" rather than a bare "completed".
+    let no_actionable_nodes = !graph_has_actionable_nodes(&flow.graph);
+    if no_actionable_nodes {
+        tracing::warn!(
+            target: "flows",
+            flow_id = %flow_id,
+            "[flows] flows_run: flow has no actionable nodes — nothing to execute"
+        );
+    }
+
     // `store::get_flow` already ran the stored `graph_json` through
     // `tinyflows::migrate::migrate` before deserializing, so `flow.graph` is
     // always on the current schema here.
@@ -3011,17 +3103,26 @@ pub async fn flows_run(
         flow_id = %flow_id,
         status,
         pending_approvals = outcome.pending_approvals.len(),
+        no_actionable_nodes,
         "[flows] flows_run: finished"
     );
 
-    Ok(RpcOutcome::single_log(
-        json!({
-            "output": outcome.output,
-            "pending_approvals": outcome.pending_approvals,
-            "thread_id": thread_id,
-        }),
-        format!("flow run {status}"),
-    ))
+    const NO_ACTIONABLE_NODES_NOTE: &str = "This flow's graph has no actionable nodes beyond \
+         its trigger (no downstream action nodes, or no edges connecting them) — the run \
+         completed without doing anything. Add and wire up at least one action node.";
+
+    let mut result = json!({
+        "output": outcome.output,
+        "pending_approvals": outcome.pending_approvals,
+        "thread_id": thread_id,
+    });
+    let mut logs = vec![format!("flow run {status}")];
+    if no_actionable_nodes {
+        result["note"] = json!(NO_ACTIONABLE_NODES_NOTE);
+        logs.push(NO_ACTIONABLE_NODES_NOTE.to_string());
+    }
+
+    Ok(RpcOutcome::new(result, logs))
 }
 
 /// Resumes a `flows_run` that paused at a human-in-the-loop approval gate,
@@ -3949,7 +4050,9 @@ pub async fn flows_discover(
 const FLOW_BUILD_TIMEOUT_SECS: u64 = 600;
 
 /// Tools stripped from the `workflow_builder` belt on the direct `flows_build`
-/// RPC path (issue #4593).
+/// RPC path (issue #4593; widened for `resume_flow_run`/`cancel_flow_run`
+/// alongside issue #4881, which added both to the belt without extending
+/// this list).
 ///
 /// `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
@@ -3969,22 +4072,46 @@ const FLOW_BUILD_TIMEOUT_SECS: u64 = 600;
 /// 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"];
+///
+/// `resume_flow_run` ([`builder_tools::ResumeFlowRunTool`]) is the exact same
+/// concern as `run_flow`, one hop later: it is `external_effect() == true`
+/// (its own description says "This ADVANCES A REAL RUN — approved outbound
+/// nodes will fire") and would be auto-allowed by the same `Cli`-origin gate
+/// bypass, letting an authoring turn (or a confused/prompt-injected model)
+/// approve a live run's parked Slack/Gmail/HTTP node with zero human
+/// confirmation — the exact HITL hole #4593 closed, reopened by #4881
+/// widening the belt.
+///
+/// `cancel_flow_run` fires no new outbound effect
+/// (`external_effect() == false`), so it isn't a gate-bypass concern the same
+/// way — but an authoring turn still has no business tearing down a run the
+/// *user* started, so it is hidden alongside the two above out of caution.
+///
+/// `create_workflow` / `duplicate_flow` are deliberately **left visible**:
+/// both are hard-forced **born disabled** (see [`builder_tools::CreateWorkflowTool`]
+/// / [`builder_tools::DuplicateFlowTool`]), so even an unattended call can't
+/// leave anything live — lower risk than the run/resume/cancel trio above.
+const FLOWS_BUILD_HIDDEN_TOOLS: &[&str] = &[
+    "run_workflow",
+    "run_flow",
+    "resume_flow_run",
+    "cancel_flow_run",
+];
 
-/// Strip the live-run tool(s) in [`FLOWS_BUILD_HIDDEN_TOOLS`] from `agent`'s
-/// callable set for the direct `flows_build` RPC path.
+/// Strip the live-run / resume / cancel 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.
+/// The authoring tools (`propose`/`revise`/`save`/`dry_run`/reads/`create_workflow`/
+/// `duplicate_flow`) stay visible 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"
+        "[flows] flows_build: hiding live-run/resume/cancel tools from builder belt"
     );
     agent.hide_tools(FLOWS_BUILD_HIDDEN_TOOLS);
 }