diff --git a/src/openhuman/flows/ops.rs b/src/openhuman/flows/ops.rs index 69b39cd5aa..9dfe9ccb8c 100644 --- a/src/openhuman/flows/ops.rs +++ b/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); } diff --git a/src/openhuman/flows/ops_tests.rs b/src/openhuman/flows/ops_tests.rs index 27731b6374..afdfcfb842 100644 --- a/src/openhuman/flows/ops_tests.rs +++ b/src/openhuman/flows/ops_tests.rs @@ -228,6 +228,79 @@ async fn flows_run_completes_trigger_only_graph() { assert!(reloaded.value.last_run_at.is_some()); } +/// Live finding: a trigger-only graph (no downstream action nodes at all) +/// used to report `status="completed" pending_approvals=0` from `flows_run` +/// completely indistinguishably from a run that actually did something — +/// "triggered but nothing happened" read as a plain success. This asserts +/// the run still completes (running an empty flow isn't an error), but now +/// carries a human-readable `note` in the result so the UI can show +/// "nothing to run" instead of a bare "completed". +#[tokio::test] +async fn flows_run_on_trigger_only_graph_surfaces_no_actionable_nodes_note() { + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + let created = flows_create(&config, "empty".to_string(), trigger_only_graph(), false) + .await + .unwrap(); + + let outcome = flows_run(&config, &created.value.id, json!({}), FlowRunTrigger::Rpc) + .await + .unwrap(); + + let note = outcome.value["note"] + .as_str() + .expect("trigger-only run must carry a human-readable 'note' field"); + assert!( + note.contains("no actionable nodes") || note.to_lowercase().contains("nothing"), + "note should explain that nothing ran, got: {note}" + ); + assert!( + outcome.logs.iter().any(|l| l.contains("no actionable")), + "the note should also surface via the RpcOutcome logs, got: {:?}", + outcome.logs + ); + + // Still a completed run, not an error — an empty flow isn't a failure, + // just a no-op that must not masquerade as having done real work. + let reloaded = flows_get(&config, &created.value.id).await.unwrap(); + assert_eq!(reloaded.value.last_status.as_deref(), Some("completed")); +} + +/// A graph with a real downstream node, wired up by an edge, must NOT carry +/// the "nothing to run" note — only a graph with no actionable nodes at all. +/// Uses `output_parser` nodes (like the approval-gated fixture above) rather +/// than an `agent`/`tool_call` node so the run completes deterministically +/// without needing a configured LLM provider or network access. +#[tokio::test] +async fn flows_run_on_graph_with_actionable_nodes_has_no_empty_flow_note() { + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + + let graph = json!({ + "name": "has-work", + "nodes": [ + { "id": "t", "kind": "trigger", "name": "Trigger" }, + { "id": "downstream", "kind": "output_parser", "name": "Downstream" } + ], + "edges": [ + { "from_node": "t", "to_node": "downstream" } + ] + }); + let created = flows_create(&config, "has-work".to_string(), graph, false) + .await + .unwrap(); + + let outcome = flows_run(&config, &created.value.id, json!({}), FlowRunTrigger::Rpc) + .await + .unwrap(); + + assert!( + outcome.value.get("note").is_none(), + "a graph with real downstream nodes must not get the empty-flow note, got: {:?}", + outcome.value.get("note") + ); +} + #[tokio::test] async fn flows_run_reports_pending_approval_and_blocks_downstream() { let tmp = TempDir::new().unwrap(); @@ -559,6 +632,141 @@ async fn flows_update_does_not_rebind_when_graph_is_not_supplied() { assert_eq!(job.expression, old_job.expression); } +// ── flows_update B29 Rule 1 analogue (save/enable safety on update) ─────── +// +// `flows_create` already refuses to persist an automatic-trigger graph as +// `enabled` (Rule 1, above). Live finding: `flows_update` had no equivalent +// — a flow created `enabled: true` with a manual trigger could later have an +// automatic-trigger graph (schedule / app_event / webhook) saved onto it via +// `flows_update` and go LIVE immediately with no user review. These tests +// cover the manual→automatic transition (must disarm), automatic→automatic +// re-edit (must NOT disarm — the user already opted in), and manual→manual +// (never touched). + +#[tokio::test] +async fn flows_update_disables_on_manual_to_automatic_trigger_transition_when_enabled() { + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + + // A manual-trigger flow persists enabled straight from create (Rule 1 + // only gates automatic triggers). + let created = flows_create( + &config, + "manual-then-scheduled".to_string(), + manual_trigger_graph(), + false, + ) + .await + .unwrap(); + assert!(created.value.enabled, "manual-trigger flows create enabled"); + + // Saving an automatic-trigger graph onto that enabled flow must disarm + // it — not go live unattended. + let updated = flows_update( + &config, + &created.value.id, + None, + Some(schedule_trigger_graph("0 8 * * *")), + None, + None, + ) + .await + .unwrap(); + + assert!( + !updated.value.enabled, + "an enabled flow whose trigger just changed from manual to automatic must be \ + auto-disabled, not armed live" + ); + assert!( + updated.logs.iter().any(|l| l.contains("auto-disabled")), + "the disarm must be surfaced in the outcome logs, got: {:?}", + updated.logs + ); + + // Persisted, not just returned in-memory. + let reloaded = flows_get(&config, &created.value.id).await.unwrap(); + assert!(!reloaded.value.enabled); + + // And no cron job was left bound — the flow never actually went live. + assert!( + crate::openhuman::cron::find_flow_schedule_job(&config, &created.value.id) + .unwrap() + .is_none(), + "an auto-disabled flow must not have its schedule cron job bound" + ); +} + +#[tokio::test] +async fn flows_update_preserves_enabled_when_already_automatic() { + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + + // Rule 1 creates an automatic-trigger flow disabled; the user arms it + // explicitly — this IS the "already reviewed and opted in" state. + let created = flows_create( + &config, + "scheduled".to_string(), + schedule_trigger_graph("0 9 * * *"), + false, + ) + .await + .unwrap(); + assert!(!created.value.enabled); + flows_set_enabled(&config, &created.value.id, true) + .await + .unwrap(); + + // A legitimate re-edit (still an automatic trigger, just a new cron + // expression) must NOT be treated as a fresh unattended arm. + let updated = flows_update( + &config, + &created.value.id, + None, + Some(schedule_trigger_graph("30 8 * * *")), + None, + None, + ) + .await + .unwrap(); + + assert!( + updated.value.enabled, + "re-editing an already-enabled automatic-trigger flow must not disarm it — the \ + user already opted in once" + ); + assert!(!updated.logs.iter().any(|l| l.contains("auto-disabled"))); +} + +#[tokio::test] +async fn flows_update_preserves_enabled_for_manual_target() { + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + + let created = flows_create(&config, "manual".to_string(), manual_trigger_graph(), false) + .await + .unwrap(); + assert!(created.value.enabled); + + // manual → manual: no automatic trigger ever enters the picture, so + // `enabled` must be left completely untouched. + let mut new_graph = manual_trigger_graph(); + new_graph["name"] = json!("manual-renamed"); + let updated = flows_update( + &config, + &created.value.id, + None, + Some(new_graph), + None, + None, + ) + .await + .unwrap(); + + assert!(updated.value.enabled); + assert!(!updated.logs.iter().any(|l| l.contains("auto-disabled"))); +} + // ── flows_resume (issue B2) ─────────────────────────────────────────────── fn approval_gated_graph() -> Value { @@ -3454,21 +3662,25 @@ fn finalize_terminal_status_no_error_when_clean() { 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). +/// Regression for issue #4593 (widened for #4881's `resume_flow_run`/ +/// `cancel_flow_run` addition to the belt): the `flows_build` builder turn +/// runs under `AgentTurnOrigin::Cli`, which makes the `ApprovalGate` +/// auto-allow every `external_effect` tool. The flows live-runner (`run_flow`) +/// and the run-resume tool (`resume_flow_run`) both execute/advance a *live* +/// saved flow's real outbound effects, so both must be unreachable on this +/// path — `restrict_builder_toolset` drops them (plus `cancel_flow_run`, out +/// of caution) 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. + // Document WHY each run-advancing tool must be hidden: running or + // resuming a saved flow fires real Slack/Gmail/HTTP/code effects, so both + // are external-effect tools. This pins that invariant independently of + // belt name-resolution so the hide-list can't silently stop covering a + // live-run/resume tool. use crate::openhuman::tools::Tool as _; let live_runner = crate::openhuman::flows::tools::RunFlowTool::new(std::sync::Arc::new(config.clone())); @@ -3476,6 +3688,14 @@ async fn flows_build_hides_the_live_run_tool_from_the_builder_belt() { live_runner.external_effect(), "the flows live-runner must be external-effect for the #4593 concern to apply" ); + let resumer = crate::openhuman::flows::builder_tools::ResumeFlowRunTool::new( + std::sync::Arc::new(config.clone()), + ); + assert!( + resumer.external_effect(), + "resume_flow_run advances a real run's outbound effects, so it must be \ + external-effect for the same #4593/#4881 concern to apply" + ); crate::openhuman::agent::harness::AgentDefinitionRegistry::init_global(&config.workspace_dir) .expect("agent registry init"); @@ -3484,27 +3704,36 @@ async fn flows_build_hides_the_live_run_tool_from_the_builder_belt() { .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() - ); + // Precondition: the builder advertises all four run-advancing tools on its + // belt before restriction — the exact set #4593/#4881 are about. + let visible_before = agent.visible_tool_names_for_test(); + for present in ["run_flow", "resume_flow_run", "cancel_flow_run"] { + assert!( + visible_before.contains(present), + "precondition: workflow_builder belt should advertise `{present}`; visible = \ + {visible_before:?}" + ); + } 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). + // After restriction none of the run-advancing tools are callable on the + // flows_build path — the hide-list covers all of them (#4593 + #4881). let visible = agent.visible_tool_names_for_test(); - for hidden in ["run_workflow", "run_flow"] { + for hidden in [ + "run_workflow", + "run_flow", + "resume_flow_run", + "cancel_flow_run", + ] { assert!( !visible.contains(hidden), - "live-run tool `{hidden}` must be hidden on the flows_build path; visible = {visible:?}" + "run-advancing tool `{hidden}` must be hidden on the flows_build path; visible = \ + {visible:?}" ); } - // Authoring / read tools stay reachable so the builder turn still works + // Authoring / read tools — including the born-disabled `create_workflow` + // and `duplicate_flow` — stay reachable so the builder turn still works // headlessly under the CLI origin (no fail-close). for keep in [ "propose_workflow", @@ -3512,6 +3741,8 @@ async fn flows_build_hides_the_live_run_tool_from_the_builder_belt() { "save_workflow", "dry_run_workflow", "list_flows", + "create_workflow", + "duplicate_flow", ] { assert!( visible.contains(keep), diff --git a/src/openhuman/flows/store.rs b/src/openhuman/flows/store.rs index 9d9b95f990..3eb7e0c1bf 100644 --- a/src/openhuman/flows/store.rs +++ b/src/openhuman/flows/store.rs @@ -374,12 +374,22 @@ impl std::fmt::Display for FlowUpdateError { /// flow's `updated_at` no longer matches — so an agent save and a concurrent /// canvas save can't silently clobber each other. `None` keeps the prior /// last-write-wins behaviour for callers that don't track a version. +/// +/// `enabled_override`, when `Some`, forces the persisted `enabled` flag to +/// that value in the *same* guarded `UPDATE` as the graph/name/ +/// `require_approval` write — used by `ops::flows_update`'s B29 Rule 1 +/// analogue (auto-disarming a flow whose trigger just changed from manual to +/// automatic) so the disarm can never race a concurrent read/write of +/// `enabled` (a separate `set_enabled` call after this one would leave a +/// TOCTOU window). `None` leaves `enabled` untouched, matching the previous +/// behaviour for every other caller. pub fn update_flow_graph( config: &Config, id: &str, name: String, graph: tinyflows::model::WorkflowGraph, require_approval: bool, + enabled_override: Option, expected_updated_at: Option<&str>, ) -> std::result::Result { let current = get_flow(config, id) @@ -400,21 +410,25 @@ pub fn update_flow_graph( let prior_graph_json = serde_json::to_string(¤t.graph).unwrap_or_else(|_| "null".to_string()); let now = Utc::now().to_rfc3339(); + let new_enabled = enabled_override.unwrap_or(current.enabled); with_connection(config, |conn| { // Guarded UPDATE keyed on the observed updated_at (race-safe even // without an explicit expected version) — a concurrent writer that // moved updated_at makes this match 0 rows. Targeted columns only, so a - // concurrent set_enabled/record_run isn't clobbered. + // concurrent set_enabled/record_run isn't clobbered (unless this call + // itself carries an `enabled_override`, in which case `enabled` is + // one of the targeted columns by design). let changed = conn .execute( "UPDATE flow_definitions SET name = ?1, graph_json = ?2, updated_at = ?3, \ - require_approval = ?4 WHERE id = ?5 AND updated_at = ?6", + require_approval = ?4, enabled = ?5 WHERE id = ?6 AND updated_at = ?7", params![ name, graph_json, now, if require_approval { 1 } else { 0 }, + if new_enabled { 1 } else { 0 }, id, current.updated_at, ], diff --git a/src/openhuman/flows/store_tests.rs b/src/openhuman/flows/store_tests.rs index 8192cb0e73..778712a0e9 100644 --- a/src/openhuman/flows/store_tests.rs +++ b/src/openhuman/flows/store_tests.rs @@ -98,6 +98,7 @@ fn update_flow_graph_bumps_updated_at_and_preserves_created_at() { new_graph, false, None, + None, ) .unwrap(); @@ -204,6 +205,7 @@ fn update_flow_graph_can_change_require_approval() { trigger_graph(), true, None, + None, ) .unwrap(); assert!(updated.require_approval);