From 98878ffc90748bfc4aec519da8e6817da12eeffc Mon Sep 17 00:00:00 2001 From: hugocasa Date: Mon, 8 Jun 2026 17:13:31 +0200 Subject: [PATCH] fix(forks): defer trigger/schedule state to parent for clean git merge Co-Authored-By: Claude Opus 4.8 (1M context) --- backend/windmill-api-schedule/src/lib.rs | 24 +- backend/windmill-api/src/workspaces_export.rs | 365 ++++++++++++++---- backend/windmill-trigger/src/handler.rs | 42 +- docs/fork-triggers.md | 83 ++-- 4 files changed, 403 insertions(+), 111 deletions(-) diff --git a/backend/windmill-api-schedule/src/lib.rs b/backend/windmill-api-schedule/src/lib.rs index 29fa215800..35dc8b4f15 100644 --- a/backend/windmill-api-schedule/src/lib.rs +++ b/backend/windmill-api-schedule/src/lib.rs @@ -242,6 +242,21 @@ async fn create_schedule( let mut tx: Transaction<'_, Postgres> = user_db.begin(&authed).await?; + // Writing into a fork never sets operational state: force `enabled = false` + // so a cloned / synced / merged / UI-created schedule can't fire alongside + // the parent's. The fork owner re-enables locally via `setenabled`. This is + // the schedule analog of the trigger rule in + // `windmill-trigger::handler::workspace_is_fork`; the read half (parent-value + // substitution on fork export) lives in `workspaces_export.rs`. + let target_is_fork: bool = sqlx::query_scalar!( + "SELECT parent_workspace_id IS NOT NULL FROM workspace WHERE id = $1", + w_id + ) + .fetch_optional(&mut *tx) + .await? + .flatten() + .unwrap_or(false); + // Check schedule for error ScheduleType::from_str(&ns.schedule, ns.cron_version.as_deref(), true)?; @@ -341,7 +356,12 @@ async fn create_schedule( // flows (CLI merge, UI merge, `wmill push` of a fork tarball) — which // either send the source's actual flag (create case) or omit `enabled` // entirely (update case, where `EditSchedule` lacks the field). - ns.enabled.unwrap_or(true), + // A write into a fork always lands disabled regardless of the request. + if target_is_fork { + false + } else { + ns.enabled.unwrap_or(true) + }, resolved_email, resolved_permissioned_as, ns.on_failure, @@ -413,7 +433,7 @@ async fn create_schedule( .await?; } - if ns.enabled.unwrap_or(true) { + if !target_is_fork && ns.enabled.unwrap_or(true) { tx = push_scheduled_job(&db, tx, &schedule, Some(&authed.clone().into()), None).await? } tx.commit().await?; diff --git a/backend/windmill-api/src/workspaces_export.rs b/backend/windmill-api/src/workspaces_export.rs index 3f0cd3c646..63472a801b 100644 --- a/backend/windmill-api/src/workspaces_export.rs +++ b/backend/windmill-api/src/workspaces_export.rs @@ -125,10 +125,18 @@ pub fn is_none_or_false(val: &Option) -> bool { } } -/// Returns the keys to strip from trigger/schedule serialization when the -/// source workspace is a fork. Stripping these keys avoids propagating -/// fork-local operational state (enabled flag, runtime listener identifiers) -/// back to the parent workspace through the git-sync round-trip. +/// A fork's git-sync export rewrites each trigger's `mode` (and each schedule's +/// `enabled`) to the *parent* workspace's value, instead of emitting the fork's +/// own (clone-disabled / locally-toggled) state. This keeps the fork's synced +/// file byte-identical to the parent on the operational-state field, so a +/// normal-git PR merge has nothing to resolve — no dropped `mode:` line, no +/// flipped parent trigger. Fork-only paths (absent from the parent) keep the +/// fork's own value: there's no parent state to defer to, so the trigger lands +/// with whatever the fork creator set. The write half of the same rule lives in +/// `windmill-trigger::handler::workspace_is_fork`. +/// +/// Maps trigger `path` → parent `mode` (as the lowercase enum text that matches +/// `TriggerMode`'s serde representation). Empty when not a fork. #[cfg(any( feature = "http_trigger", feature = "websocket", @@ -148,20 +156,72 @@ pub fn is_none_or_false(val: &Option) -> bool { feature = "private" ) ))] -fn fork_trigger_ignore_keys(is_fork: bool) -> Option> { - if is_fork { - Some(vec!["mode", "enabled"]) - } else { - None - } +async fn fork_parent_trigger_modes( + tx: &mut sqlx::PgConnection, + table_name: &str, + parent_workspace_id: Option<&str>, +) -> Result> { + let Some(parent) = parent_workspace_id else { + return Ok(HashMap::new()); + }; + // SAFETY: `table_name` is a compile-time `TriggerCrud::TABLE_NAME` constant. + let rows: Vec<(String, String)> = sqlx::query_as(&format!( + "SELECT path, mode::text FROM {} WHERE workspace_id = $1", + table_name + )) + .bind(parent) + .fetch_all(&mut *tx) + .await?; + Ok(rows.into_iter().collect()) } -fn fork_schedule_ignore_keys(is_fork: bool) -> Option> { - if is_fork { - Some(vec!["enabled"]) - } else { - None - } +/// Build the `{ "mode": }` override for a single trigger, or +/// `None` (keep the fork's own value) when the path is fork-only. +#[cfg(any( + feature = "http_trigger", + feature = "websocket", + feature = "postgres_trigger", + feature = "mqtt_trigger", + feature = "native_trigger", + all( + feature = "enterprise", + any( + feature = "kafka", + feature = "sqs_trigger", + feature = "gcp_trigger", + feature = "azure_trigger", + feature = "nats", + feature = "smtp", + ), + feature = "private" + ) +))] +fn trigger_mode_override( + parent_modes: &HashMap, + path: &str, +) -> Option> { + parent_modes.get(path).map(|mode| { + let mut o = serde_json::Map::new(); + o.insert("mode".to_string(), Value::String(mode.clone())); + o + }) +} + +/// Schedule analog of [`fork_parent_trigger_modes`]: maps schedule `path` → +/// parent `enabled`. Empty when not a fork. +async fn fork_parent_schedule_enabled( + tx: &mut sqlx::PgConnection, + parent_workspace_id: Option<&str>, +) -> Result> { + let Some(parent) = parent_workspace_id else { + return Ok(HashMap::new()); + }; + let rows: Vec<(String, bool)> = + sqlx::query_as("SELECT path, enabled FROM schedule WHERE workspace_id = $1") + .bind(parent) + .fetch_all(&mut *tx) + .await?; + Ok(rows.into_iter().collect()) } enum ArchiveImpl { @@ -260,6 +320,25 @@ pub fn to_string_without_metadata( extra_perms: ExtraPermsBehavior, ignore_keys: Option>, ) -> Result +where + T: ?Sized + Serialize, +{ + to_string_without_metadata_inner(value, extra_perms, ignore_keys, None) +} + +/// Like [`to_string_without_metadata`] but additionally lets the caller +/// override top-level keys after stripping. Used for fork trigger/schedule +/// exports, where `mode`/`enabled` is rewritten to the *parent* workspace's +/// value so the fork's synced file is byte-identical to the parent on those +/// fields — a clean 3-way git merge instead of a dropped line. See the write +/// half of the rule in `windmill-trigger::handler::workspace_is_fork`. +#[inline] +pub fn to_string_without_metadata_inner( + value: &T, + extra_perms: ExtraPermsBehavior, + ignore_keys: Option>, + overrides: Option<&serde_json::Map>, +) -> Result where T: ?Sized + Serialize, { @@ -324,6 +403,12 @@ where obj.remove("default_permissioned_as"); } + if let Some(overrides) = overrides { + for (k, v) in overrides { + obj.insert(k.clone(), v.clone()); + } + } + serde_json::to_string_pretty(&obj).ok() }) .flatten() @@ -504,18 +589,18 @@ pub(crate) async fn tarball_workspace( let mut tx = user_db.begin(&authed).await?; - // Source-of-truth check for fork-ness: the workspace's parent_workspace_id - // column. The wm-fork-* prefix is a creation-time naming convention that - // could in principle drift (rename, manual SQL); the column is the - // contract that matches what the conflict-warning gates read. - let is_fork: bool = sqlx::query_scalar!( - "SELECT parent_workspace_id IS NOT NULL FROM workspace WHERE id = $1", - &w_id + // Source-of-truth for fork-ness: the workspace's parent_workspace_id column. + // The wm-fork-* prefix is a creation-time naming convention that could in + // principle drift (rename, manual SQL); the column is the contract that + // matches what the conflict-warning gates read. The id is also the workspace + // whose trigger `mode` / schedule `enabled` a fork export defers to. + let parent_workspace_id: Option = sqlx::query_scalar::<_, Option>( + "SELECT parent_workspace_id FROM workspace WHERE id = $1", ) + .bind(&w_id) .fetch_optional(&mut *tx) .await? - .flatten() - .unwrap_or(false); + .flatten(); let tmp_dir = TempDir::new_in(&*WINDMILL_DIR)?; @@ -799,12 +884,21 @@ pub(crate) async fn tarball_workspace( .fetch_all(&mut *tx) .await?; - let schedule_ignore_keys = fork_schedule_ignore_keys(is_fork); + // For a fork, defer each schedule's `enabled` to the parent so the + // synced file matches the parent and the merge doesn't flip it. + let parent_enabled = + fork_parent_schedule_enabled(&mut *tx, parent_workspace_id.as_deref()).await?; for schedule in schedules { - let app_str = &to_string_without_metadata( + let enabled_override = parent_enabled.get(&schedule.path).map(|enabled| { + let mut o = serde_json::Map::new(); + o.insert("enabled".to_string(), Value::Bool(*enabled)); + o + }); + let app_str = &to_string_without_metadata_inner( &schedule, ExtraPermsBehavior::Drop, - schedule_ignore_keys.clone(), + None, + enabled_override.as_ref(), ) .unwrap(); archive @@ -814,38 +908,25 @@ pub(crate) async fn tarball_workspace( } if include_triggers.unwrap_or(false) { - #[cfg(any( - feature = "http_trigger", - feature = "websocket", - feature = "postgres_trigger", - feature = "mqtt_trigger", - feature = "native_trigger", - all( - feature = "enterprise", - any( - feature = "kafka", - feature = "sqs_trigger", - feature = "gcp_trigger", - feature = "azure_trigger", - feature = "nats", - feature = "smtp", - ), - feature = "private" - ) - ))] - let trigger_ignore_keys = fork_trigger_ignore_keys(is_fork); - #[cfg(feature = "http_trigger")] { use crate::triggers::http::HttpTrigger; let handler = HttpTrigger; let http_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?; + let parent_modes = fork_parent_trigger_modes( + &mut *tx, + ::TABLE_NAME, + parent_workspace_id.as_deref(), + ) + .await?; for trigger in http_triggers { - let trigger_str = &to_string_without_metadata( + let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path); + let trigger_str = &to_string_without_metadata_inner( &trigger, ExtraPermsBehavior::Drop, - trigger_ignore_keys.clone(), + None, + mode_override.as_ref(), ) .unwrap(); archive @@ -862,12 +943,20 @@ pub(crate) async fn tarball_workspace( use crate::triggers::websocket::WebsocketTrigger; let handler = WebsocketTrigger; let websocket_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?; + let parent_modes = fork_parent_trigger_modes( + &mut *tx, + ::TABLE_NAME, + parent_workspace_id.as_deref(), + ) + .await?; for trigger in websocket_triggers { - let trigger_str = &to_string_without_metadata( + let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path); + let trigger_str = &to_string_without_metadata_inner( &trigger, ExtraPermsBehavior::Drop, - trigger_ignore_keys.clone(), + None, + mode_override.as_ref(), ) .unwrap(); archive @@ -884,12 +973,20 @@ pub(crate) async fn tarball_workspace( use crate::triggers::kafka::KafkaTrigger; let handler = KafkaTrigger; let kafka_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?; + let parent_modes = fork_parent_trigger_modes( + &mut *tx, + ::TABLE_NAME, + parent_workspace_id.as_deref(), + ) + .await?; for trigger in kafka_triggers { - let trigger_str = &to_string_without_metadata( + let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path); + let trigger_str = &to_string_without_metadata_inner( &trigger, ExtraPermsBehavior::Drop, - trigger_ignore_keys.clone(), + None, + mode_override.as_ref(), ) .unwrap(); archive @@ -906,12 +1003,20 @@ pub(crate) async fn tarball_workspace( use crate::triggers::sqs::SqsTrigger; let handler = SqsTrigger; let sqs_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?; + let parent_modes = fork_parent_trigger_modes( + &mut *tx, + ::TABLE_NAME, + parent_workspace_id.as_deref(), + ) + .await?; for trigger in sqs_triggers { - let trigger_str = &to_string_without_metadata( + let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path); + let trigger_str = &to_string_without_metadata_inner( &trigger, ExtraPermsBehavior::Drop, - trigger_ignore_keys.clone(), + None, + mode_override.as_ref(), ) .unwrap(); archive @@ -928,12 +1033,20 @@ pub(crate) async fn tarball_workspace( use crate::triggers::gcp::GcpTrigger; let handler = GcpTrigger; let gcp_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?; + let parent_modes = fork_parent_trigger_modes( + &mut *tx, + ::TABLE_NAME, + parent_workspace_id.as_deref(), + ) + .await?; for trigger in gcp_triggers { - let trigger_str = &to_string_without_metadata( + let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path); + let trigger_str = &to_string_without_metadata_inner( &trigger, ExtraPermsBehavior::Drop, - trigger_ignore_keys.clone(), + None, + mode_override.as_ref(), ) .unwrap(); archive @@ -950,12 +1063,20 @@ pub(crate) async fn tarball_workspace( use crate::triggers::azure::AzureTrigger; let handler = AzureTrigger; let azure_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?; + let parent_modes = fork_parent_trigger_modes( + &mut *tx, + ::TABLE_NAME, + parent_workspace_id.as_deref(), + ) + .await?; for trigger in azure_triggers { - let trigger_str = &to_string_without_metadata( + let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path); + let trigger_str = &to_string_without_metadata_inner( &trigger, ExtraPermsBehavior::Drop, - trigger_ignore_keys.clone(), + None, + mode_override.as_ref(), ) .unwrap(); archive @@ -972,12 +1093,20 @@ pub(crate) async fn tarball_workspace( use crate::triggers::nats::NatsTrigger; let handler = NatsTrigger; let nats_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?; + let parent_modes = fork_parent_trigger_modes( + &mut *tx, + ::TABLE_NAME, + parent_workspace_id.as_deref(), + ) + .await?; for trigger in nats_triggers { - let trigger_str: &String = &to_string_without_metadata( + let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path); + let trigger_str: &String = &to_string_without_metadata_inner( &trigger, ExtraPermsBehavior::Drop, - trigger_ignore_keys.clone(), + None, + mode_override.as_ref(), ) .unwrap(); archive @@ -994,12 +1123,20 @@ pub(crate) async fn tarball_workspace( use crate::triggers::postgres::PostgresTrigger; let handler = PostgresTrigger; let postgres_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?; + let parent_modes = fork_parent_trigger_modes( + &mut *tx, + ::TABLE_NAME, + parent_workspace_id.as_deref(), + ) + .await?; for trigger in postgres_triggers { - let trigger_str = &to_string_without_metadata( + let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path); + let trigger_str = &to_string_without_metadata_inner( &trigger, ExtraPermsBehavior::Drop, - trigger_ignore_keys.clone(), + None, + mode_override.as_ref(), ) .unwrap(); archive @@ -1016,12 +1153,20 @@ pub(crate) async fn tarball_workspace( use crate::triggers::mqtt::MqttTrigger; let handler = MqttTrigger; let mqtt_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?; + let parent_modes = fork_parent_trigger_modes( + &mut *tx, + ::TABLE_NAME, + parent_workspace_id.as_deref(), + ) + .await?; for trigger in mqtt_triggers { - let trigger_str = &to_string_without_metadata( + let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path); + let trigger_str = &to_string_without_metadata_inner( &trigger, ExtraPermsBehavior::Drop, - trigger_ignore_keys.clone(), + None, + mode_override.as_ref(), ) .unwrap(); archive @@ -1038,12 +1183,20 @@ pub(crate) async fn tarball_workspace( use crate::triggers::email::EmailTrigger; let handler = EmailTrigger; let email_triggers = handler.list_triggers(&mut *tx, &w_id, None).await?; + let parent_modes = fork_parent_trigger_modes( + &mut *tx, + ::TABLE_NAME, + parent_workspace_id.as_deref(), + ) + .await?; for trigger in email_triggers { - let trigger_str = &to_string_without_metadata( + let mode_override = trigger_mode_override(&parent_modes, &trigger.base.path); + let trigger_str = &to_string_without_metadata_inner( &trigger, ExtraPermsBehavior::Drop, - trigger_ignore_keys.clone(), + None, + mode_override.as_ref(), ) .unwrap(); archive @@ -1065,10 +1218,12 @@ pub(crate) async fn tarball_workspace( list_native_triggers(&mut *tx, &w_id, service_name, None, None, None, None) .await?; - let mut native_ignore_keys = vec!["webhook_token_hash"]; - if let Some(ref extra) = trigger_ignore_keys { - native_ignore_keys.extend_from_slice(extra); - } + // Native triggers (Nextcloud, Google Drive, GitHub) are never + // cloned into a fork — a fork only has one if its owner created + // it there, so it's always "fork-only" and keeps its own mode. + // No parent-value substitution applies; we only strip the + // webhook token hash. + let native_ignore_keys = vec!["webhook_token_hash"]; for trigger in native_triggers { let trigger_str = &to_string_without_metadata( @@ -1359,3 +1514,67 @@ pub(crate) async fn tarball_workspace( ]; Ok((headers, body)) } + +#[cfg(test)] +mod fork_export_tests { + use super::*; + use serde_json::json; + + /// A fork export rewrites `mode` to the parent's value: the serialized file + /// carries the parent's state (`enabled`), not the fork's clone-disabled DB + /// value — so a normal-git merge sees no change on that line. + #[test] + fn override_substitutes_parent_mode() { + let fork_trigger = json!({ + "path": "f/triggers/x", + "script_path": "f/scripts/x", + "mode": "disabled", // fork's local (clone-disabled) state + "is_flow": false, + }); + let mut overrides = serde_json::Map::new(); + overrides.insert("mode".to_string(), Value::String("enabled".to_string())); + + let out = to_string_without_metadata_inner( + &fork_trigger, + ExtraPermsBehavior::Drop, + None, + Some(&overrides), + ) + .unwrap(); + let parsed: Value = serde_json::from_str(&out).unwrap(); + + assert_eq!(parsed["mode"], json!("enabled"), "parent mode substituted"); + // `path` is in the metadata strip list, so it should be removed. + assert!(parsed.get("path").is_none()); + } + + /// A fork-only trigger (no parent counterpart, so no override) keeps the + /// fork creator's chosen state. + #[test] + fn no_override_keeps_fork_value() { + let fork_only = json!({ "mode": "enabled", "script_path": "f/scripts/x" }); + let out = + to_string_without_metadata_inner(&fork_only, ExtraPermsBehavior::Drop, None, None) + .unwrap(); + let parsed: Value = serde_json::from_str(&out).unwrap(); + assert_eq!(parsed["mode"], json!("enabled")); + } + + /// `trigger_mode_override` builds an override only when the parent has the + /// path; fork-only paths return `None` (keep the fork's own value). + #[cfg(feature = "http_trigger")] + #[test] + fn trigger_mode_override_defers_to_parent_or_self() { + let mut parent_modes = HashMap::new(); + parent_modes.insert("f/triggers/shared".to_string(), "enabled".to_string()); + + let shared = trigger_mode_override(&parent_modes, "f/triggers/shared"); + assert_eq!( + shared.as_ref().and_then(|o| o.get("mode")), + Some(&Value::String("enabled".to_string())), + ); + + // Fork-only path: no parent entry → no override → keep fork's own value. + assert!(trigger_mode_override(&parent_modes, "f/triggers/fork_only").is_none()); + } +} diff --git a/backend/windmill-trigger/src/handler.rs b/backend/windmill-trigger/src/handler.rs index f425f97b53..dcbea7e7c3 100644 --- a/backend/windmill-trigger/src/handler.rs +++ b/backend/windmill-trigger/src/handler.rs @@ -32,6 +32,26 @@ use std::sync::Arc; use windmill_audit::{audit_oss::audit_log, ActionKind}; use windmill_git_sync::handle_deployment_metadata; +/// True when the workspace is a fork (`parent_workspace_id IS NOT NULL`). +/// +/// Operational state (`mode`) belongs to the parent workspace: a write into a +/// fork (git-sync push, merge deploy, clone, or even a plain UI create) must +/// never set it. On create we force `disabled` so a fork trigger can't compete +/// with the parent's listener; on update we preserve the fork's existing value. +/// The only mutator of a fork trigger's mode is the `setmode` endpoint (which +/// carries its own conflict warning). This is the write half of the rule whose +/// read half lives in `workspaces_export.rs` (parent-value substitution on +/// fork export), and it is the single authority shared by both the git-sync +/// round-trip and the in-app compare-workspaces merge. +async fn workspace_is_fork(db: &DB, workspace_id: &str) -> Result { + let is_fork: Option = + sqlx::query_scalar("SELECT parent_workspace_id IS NOT NULL FROM workspace WHERE id = $1") + .bind(workspace_id) + .fetch_optional(db) + .await?; + Ok(is_fork.unwrap_or(false)) +} + #[async_trait] pub trait TriggerCrud: Send + Sync + 'static { type Trigger: Serialize @@ -440,6 +460,13 @@ async fn create_trigger( let mut tx = user_db.begin(&authed).await?; + // Writing into a fork never sets operational state: force `disabled` so a + // cloned / synced / merged / UI-created trigger can't compete with the + // parent's listener. The fork owner re-enables locally via `setmode`. + if workspace_is_fork(&db, &workspace_id).await? { + new_trigger.base.set_mode(TriggerMode::Disabled); + } + let new_path = new_trigger.base.path.clone(); let labels = new_trigger.base.labels.clone(); @@ -591,11 +618,16 @@ async fn update_trigger( let mut tx = user_db.begin(&authed).await?; - // When the request omits `mode`/`enabled`, preserve the existing DB value - // instead of falling back to the BaseTriggerData default (Enabled). This - // keeps fork→parent git-sync round-trips from flipping the parent's - // operational state — see fork_trigger_ignore_keys in workspaces_export.rs. - if edit_trigger.base.is_mode_unspecified() { + // Preserve the existing DB `mode` instead of writing the incoming value + // when either: + // * the target is a fork — a fork's operational state is fork-local and + // is never set through a git-sync/merge write (only via `setmode`); or + // * the request omits `mode`/`enabled` (legacy clients / YAML round-trip), + // where falling back to the BaseTriggerData default (Enabled) would flip + // the parent on a fork→parent merge. + // Read half of the rule: parent-value substitution on fork export in + // workspaces_export.rs. + if workspace_is_fork(&db, &workspace_id).await? || edit_trigger.base.is_mode_unspecified() { let existing_mode: Option = sqlx::query_scalar(&format!( "SELECT mode FROM {} WHERE workspace_id = $1 AND path = $2", T::TABLE_NAME diff --git a/docs/fork-triggers.md b/docs/fork-triggers.md index c2b5bf2231..7de67bbf72 100644 --- a/docs/fork-triggers.md +++ b/docs/fork-triggers.md @@ -51,27 +51,48 @@ parent and incoming mail would be delivered arbitrarily. The clone filter copies email triggers only when `workspaced_local_part IS TRUE` (or `CLOUD_HOSTED`, since cloud scopes email lookup by `workspace_id` natively). -## Merge-direction filter (always on) +## Operational state is owned by the parent -Whenever the source workspace has `parent_workspace_id IS NOT NULL` (i.e. -it's a fork), the tarball export at `/api/w/{workspace}/workspaces/tarball` -strips fork-local fields: +The rule that makes both the normal-git PR merge and the in-app merge behave: -- `mode` from every `*_trigger` row -- `enabled` from every `schedule` row +> **A trigger's `mode` (and a schedule's `enabled`) belongs to the parent +> workspace. Git-sync *reads* the parent's value into a fork's synced file and +> *never writes* a fork's value back. The only mutator of a fork's operational +> state is the `setmode` / `setenabled` endpoint.** -The fork-detection key is the column, not the `wm-fork-*` naming convention, -so it stays consistent with the conflict-warning gates in `set_trigger_mode` -and `set_schedule_enabled` and survives any future ID rename. +This is enforced in two halves, keyed off `parent_workspace_id IS NOT NULL` +(the column, not the `wm-fork-*` naming convention — it stays consistent with +the conflict-warning gates and survives any future ID rename): -The trigger update handler complements this: when an incoming `update_trigger` -request omits both `mode` and `enabled`, the existing DB value is preserved -instead of falling back to the BaseTriggerData default of `Enabled`. This -means the fork→parent merge cannot flip the parent's operational state, even -if the fork has an explicit (locally-disabled) state for that path. +**Read half — parent-value substitution on export.** When the source workspace +is a fork, the tarball export at `/api/w/{workspace}/workspaces/tarball` +rewrites each trigger's `mode` (and each schedule's `enabled`) to the +*parent's* value for the same path, looked up at export time. A fork-only path +(absent from the parent) keeps the fork's own value — there's no parent state +to defer to, so it lands with whatever the fork creator set. -The schedule `EditSchedule` payload already lacks an `enabled` field, so its -update path is naturally safe. +The earlier design *stripped* these fields instead. That broke a normal-git PR +merge: the parent branch (and the merge base) carries the line, the fork branch +dropped it, so the 3-way merge either silently deleted `mode`/`enabled` from +the parent — corrupting the source of truth — or conflicted outright when the +parent had also edited it. Substituting the parent's value makes the fork's +file byte-identical to the parent on that field, so the merge has nothing to +resolve. + +**Write half — fork writes never set operational state.** A write into a fork +(git-sync push, merge deploy, clone, or a plain UI create) must not set the +state, otherwise pulling the substituted parent value straight back into the +fork would re-enable it. So `create_trigger`/`create_schedule` force `disabled` +for a fork target, and `update_trigger` preserves the fork's existing `mode` +(`workspace_is_fork` in `windmill-trigger/src/handler.rs`; schedule `enabled` is +naturally preserved because `EditSchedule` has no `enabled` field). The same +handlers serve both merge paths, so the two can't diverge. The fork owner +re-enables locally via `setmode`/`setenabled` (which carry the conflict +warning below). + +For a non-fork target the incoming value is applied as given — so a fork→parent +merge of an existing trigger writes the parent's own (substituted) value (a +no-op), and a fork-only trigger lands with the fork creator's chosen state. ## Conflict warning on enable @@ -161,22 +182,22 @@ but `tally_deployed_object_changes` still records mutations against them; the deploy will fail at the workspace-collision check if the user tries to deploy a non-workspaced row to a fork. -Operational state (`mode` for triggers, `enabled` for schedules) is handled -asymmetrically between update and create: +Operational state (`mode` for triggers, `enabled` for schedules) follows the +same "owned by the parent" rule as the git round-trip (see *Operational state +is owned by the parent* above) — the two paths share the backend `create`/ +`update` handlers, so they can't diverge. -- **Update**: the merge deploy strips `mode`/`enabled` so the target's existing - state is preserved. Triggers rely on the backend's `is_mode_unspecified()` - safeguard in `update_trigger`; schedules rely on `EditSchedule` lacking the - `enabled` field. Both deploy paths perform the strip - (`stripOperationalState` in `utils_deployable.ts`, `preparePayload` in - `merge.ts`). This matches the YAML round-trip's `fork_trigger_ignore_keys` - / `fork_schedule_ignore_keys`. -- **Create**: the source's `mode`/`enabled` is passed through. There's no - target row to preserve, so a fork-only trigger or schedule lands with the - state the fork creator chose. When the source omits the flag entirely (e.g. - legacy clients), the backend defaults to `enabled` for both kinds — - `BaseTriggerData::mode()` returns `Enabled` and the schedule insert defaults - to `true` to match. +- **Update**: the merge deploy strips `mode`/`enabled` + (`stripOperationalStateOnUpdate` in the shared `deploy.ts`), so the target's + existing value is preserved — equivalent to substituting the target's value. + For a fork target the backend preserves it regardless (`workspace_is_fork`); + for a parent target the `is_mode_unspecified()` safeguard does. Schedules also + rely on `EditSchedule` lacking the `enabled` field. +- **Create**: the source's `mode`/`enabled` is passed through. Into a **parent** + there's no row to preserve, so a fork-only trigger/schedule lands with the + state the fork creator chose (omitting the flag defaults to `enabled`: + `BaseTriggerData::mode()` → `Enabled`, schedule insert → `true`). Into a + **fork** the backend forces `disabled` — a fork write never enables anything. ## Future work — runtime listener suffix