diff --git a/backend/windmill-api-schedule/src/lib.rs b/backend/windmill-api-schedule/src/lib.rs index 29fa215800..e2d495eaad 100644 --- a/backend/windmill-api-schedule/src/lib.rs +++ b/backend/windmill-api-schedule/src/lib.rs @@ -242,6 +242,23 @@ async fn create_schedule( let mut tx: Transaction<'_, Postgres> = user_db.begin(&authed).await?; + // A git-sync/merge/create write 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`. 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`. Read fork-ness + // on the non-RLS `db` pool (like the other two sites) so the determination is + // complete regardless of the caller's folder perms. + let target_is_fork: bool = sqlx::query_scalar!( + "SELECT parent_workspace_id IS NOT NULL FROM workspace WHERE id = $1", + w_id + ) + .fetch_optional(&db) + .await? + .flatten() + .unwrap_or(false); + // Check schedule for error ScheduleType::from_str(&ns.schedule, ns.cron_version.as_deref(), true)?; @@ -341,7 +358,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 +435,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..63b9e84863 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,78 @@ 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( + db: &DB, + table_name: &str, + parent_workspace_id: Option<&str>, +) -> Result> { + let Some(parent) = parent_workspace_id else { + return Ok(HashMap::new()); + }; + // Read the parent's rows on the non-RLS pool (like `workspace_is_fork`): the + // substitution must be complete regardless of the exporter's folder perms, + // otherwise a parent path the exporter can't read would fall back to the + // fork's own value and silently re-introduce the divergence we're fixing. + // No leak: only values for paths the fork already has (it's a clone) are used. + // 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(db) + .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( + db: &DB, + parent_workspace_id: Option<&str>, +) -> Result> { + let Some(parent) = parent_workspace_id else { + return Ok(HashMap::new()); + }; + // Non-RLS pool, same rationale as `fork_parent_trigger_modes`. + let rows: Vec<(String, bool)> = + sqlx::query_as("SELECT path, enabled FROM schedule WHERE workspace_id = $1") + .bind(parent) + .fetch_all(db) + .await?; + Ok(rows.into_iter().collect()) } enum ArchiveImpl { @@ -260,6 +326,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 +409,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 +595,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 +890,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(&db, 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 +914,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( + &db, + ::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 +949,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( + &db, + ::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 +979,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( + &db, + ::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 +1009,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( + &db, + ::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 +1039,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( + &db, + ::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 +1069,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( + &db, + ::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 +1099,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( + &db, + ::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 +1129,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( + &db, + ::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 +1159,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( + &db, + ::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 +1189,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( + &db, + ::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 +1224,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 +1520,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..3591c88931 100644 --- a/backend/windmill-trigger/src/handler.rs +++ b/backend/windmill-trigger/src/handler.rs @@ -32,6 +32,27 @@ 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 git-sync / +/// merge / clone / UI-create write into a fork 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. `setmode` is the intended +/// explicit mutator of a fork's mode (and carries its own conflict warning) — +/// runtime error handling may still auto-disable an errored trigger, which is +/// orthogonal to this rule. This is the write half 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 +461,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 +619,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..308d21f1be 100644 --- a/docs/fork-triggers.md +++ b/docs/fork-triggers.md @@ -51,27 +51,53 @@ 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. No git-sync / merge / create / update +> write sets a fork's operational state; the `setmode` / `setenabled` endpoint +> is the intended explicit mutator.** -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. +(Runtime error handling can still auto-disable an errored trigger or schedule — +that's orthogonal to this rule, which governs git-sync/merge/create/update +writes.) -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. +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 schedule `EditSchedule` payload already lacks an `enabled` field, so its -update path is naturally safe. +**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 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 +187,23 @@ 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 `windmill-utils-internal` + package, `cli/windmill-utils-internal/src/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