From 78b7dd5103fb6a2f8758248463d0ddd253d7971a Mon Sep 17 00:00:00 2001 From: hugocasa Date: Wed, 8 Jul 2026 16:49:57 +0200 Subject: [PATCH] fix(git-sync): explain in-sync PR verdicts with the repo's sync filter scope A PR that only touches files outside the repository's include paths gets "In sync", which reads as a wrong verdict; the check summary (and managed comment) now name the filters, e.g. "Only files matching this repository's sync filters deploy on merge: `f/**` (excluding `f/pat/**`)." Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PP5gBSPfo1YtkL1sWVAjJm --- ...1b94fee0af407769e90b4a8bbf3339927623e.json | 22 ----- ...45a1eed3aec6d60de7bc894b78189c76da72.json} | 10 ++- .../windmill-worker/src/result_processor.rs | 84 ++++++++++++++++++- 3 files changed, 90 insertions(+), 26 deletions(-) delete mode 100644 backend/.sqlx/query-39ae0b237ded9dbbb0ef0883f461b94fee0af407769e90b4a8bbf3339927623e.json rename backend/.sqlx/{query-ab9090b2ec5399f7414d1846a5b0811b4f31b6fb66354d8c42916c77b2322ba3.json => query-a5fbc70721ea71796cf8e180688c45a1eed3aec6d60de7bc894b78189c76da72.json} (57%) diff --git a/backend/.sqlx/query-39ae0b237ded9dbbb0ef0883f461b94fee0af407769e90b4a8bbf3339927623e.json b/backend/.sqlx/query-39ae0b237ded9dbbb0ef0883f461b94fee0af407769e90b4a8bbf3339927623e.json deleted file mode 100644 index 21fc7dbbfb..0000000000 --- a/backend/.sqlx/query-39ae0b237ded9dbbb0ef0883f461b94fee0af407769e90b4a8bbf3339927623e.json +++ /dev/null @@ -1,22 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT args->'__git_sync_pr_check' FROM v2_job WHERE id = $1", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "?column?", - "type_info": "Jsonb" - } - ], - "parameters": { - "Left": [ - "Uuid" - ] - }, - "nullable": [ - null - ] - }, - "hash": "39ae0b237ded9dbbb0ef0883f461b94fee0af407769e90b4a8bbf3339927623e" -} diff --git a/backend/.sqlx/query-ab9090b2ec5399f7414d1846a5b0811b4f31b6fb66354d8c42916c77b2322ba3.json b/backend/.sqlx/query-a5fbc70721ea71796cf8e180688c45a1eed3aec6d60de7bc894b78189c76da72.json similarity index 57% rename from backend/.sqlx/query-ab9090b2ec5399f7414d1846a5b0811b4f31b6fb66354d8c42916c77b2322ba3.json rename to backend/.sqlx/query-a5fbc70721ea71796cf8e180688c45a1eed3aec6d60de7bc894b78189c76da72.json index 85dcd5732f..712cc41e54 100644 --- a/backend/.sqlx/query-ab9090b2ec5399f7414d1846a5b0811b4f31b6fb66354d8c42916c77b2322ba3.json +++ b/backend/.sqlx/query-a5fbc70721ea71796cf8e180688c45a1eed3aec6d60de7bc894b78189c76da72.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "SELECT args->'__git_sync_pr_check' AS \"pr\", args->'__git_sync_deploy_check' AS \"deploy\"\n FROM v2_job WHERE id = $1", + "query": "SELECT args->'__git_sync_pr_check' AS \"pr\", args->'__git_sync_deploy_check' AS \"deploy\",\n args->>'repo_url_resource_path' AS \"repo_path\"\n FROM v2_job WHERE id = $1", "describe": { "columns": [ { @@ -12,6 +12,11 @@ "ordinal": 1, "name": "deploy", "type_info": "Jsonb" + }, + { + "ordinal": 2, + "name": "repo_path", + "type_info": "Text" } ], "parameters": { @@ -20,9 +25,10 @@ ] }, "nullable": [ + null, null, null ] }, - "hash": "ab9090b2ec5399f7414d1846a5b0811b4f31b6fb66354d8c42916c77b2322ba3" + "hash": "a5fbc70721ea71796cf8e180688c45a1eed3aec6d60de7bc894b78189c76da72" } diff --git a/backend/windmill-worker/src/result_processor.rs b/backend/windmill-worker/src/result_processor.rs index 45b89bb338..53d6bdbb45 100644 --- a/backend/windmill-worker/src/result_processor.rs +++ b/backend/windmill-worker/src/result_processor.rs @@ -1072,6 +1072,60 @@ async fn maybe_open_git_sync_deploy_pr( /// When a git-sync pull job carrying a check marker completes, post the outcome /// to its GitHub check run: the PR diff preview (`__git_sync_pr_check`, phase 4) /// or the live deploy status (`__git_sync_deploy_check`, phase 6). +/// A one-line description of the repo's sync filters, appended to an "in sync" +/// PR verdict: a PR that only touches files outside these paths deploys +/// nothing on merge, which otherwise looks like a wrong verdict. +fn format_git_sync_scope_note(include: &[String], exclude: &[String]) -> Option { + if include.is_empty() { + return None; + } + let fmt = |paths: &[String]| { + paths + .iter() + .map(|p| format!("`{p}`")) + .collect::>() + .join(", ") + }; + let mut note = format!( + "\n\nOnly files matching this repository's sync filters deploy on merge: {}", + fmt(include) + ); + if !exclude.is_empty() { + note.push_str(&format!(" (excluding {})", fmt(exclude))); + } + note.push('.'); + Some(note) +} + +#[cfg(all(feature = "enterprise", feature = "private"))] +async fn git_sync_repo_scope_note( + db: &DB, + workspace_id: &str, + repo_path: Option<&str>, +) -> Option { + let repo_path = repo_path?; + let settings = sqlx::query_scalar!( + "SELECT git_sync FROM workspace_settings WHERE workspace_id = $1", + workspace_id + ) + .fetch_optional(db) + .await + .ok()? + .flatten()?; + let settings: windmill_common::workspaces::WorkspaceGitSyncSettings = + serde_json::from_value(settings).ok()?; + // Job args carry the bare resource path; stored settings keep the $res: prefix. + let repo = settings.repositories.iter().find(|r| { + r.git_repo_resource_path.trim_start_matches("$res:") + == repo_path.trim_start_matches("$res:") + })?; + let s = repo.settings.as_ref()?; + format_git_sync_scope_note( + &s.include_path, + s.exclude_path.as_deref().unwrap_or_default(), + ) +} + #[cfg(all(feature = "enterprise", feature = "private"))] async fn maybe_post_git_sync_check( db: &DB, @@ -1082,7 +1136,8 @@ async fn maybe_post_git_sync_check( ) { // Only git-sync pull jobs carry one of these markers; everything else no-ops. let row = match sqlx::query!( - r#"SELECT args->'__git_sync_pr_check' AS "pr", args->'__git_sync_deploy_check' AS "deploy" + r#"SELECT args->'__git_sync_pr_check' AS "pr", args->'__git_sync_deploy_check' AS "deploy", + args->>'repo_url_resource_path' AS "repo_path" FROM v2_job WHERE id = $1"#, job_id ) @@ -1107,6 +1162,13 @@ async fn maybe_post_git_sync_check( let Ok(check) = serde_json::from_value::(marker) else { return; }; + // "In sync" on a PR that visibly changes files reads as a bug when those + // files are outside the repo's sync filters — say what the scope is. + let scope_note = if !is_deploy && success { + git_sync_repo_scope_note(db, workspace_id, row.repo_path.as_deref()).await + } else { + None + }; let (conclusion, title, summary): (&str, String, String) = if is_deploy { // Phase 6: real deploy pull -> "Deployed N changes" / "In sync" / failure. @@ -1164,7 +1226,10 @@ async fn maybe_post_git_sync_check( Some((changes, settings_changed)) if changes.is_empty() && !settings_changed => ( "success", "In sync".to_string(), - "Merging this PR would make no changes to the workspace.".to_string(), + format!( + "Merging this PR would make no changes to the workspace.{}", + scope_note.as_deref().unwrap_or_default() + ), ), Some((changes, settings_changed)) => { let mut lines = vec![format!( @@ -1891,6 +1956,21 @@ mod git_sync_pr_tests { } } + #[test] + fn scope_note_lists_filters() { + use super::format_git_sync_scope_note; + assert_eq!(format_git_sync_scope_note(&[], &[]), None); + assert_eq!( + format_git_sync_scope_note(&["f/**".into()], &[]).unwrap(), + "\n\nOnly files matching this repository's sync filters deploy on merge: `f/**`." + ); + assert_eq!( + format_git_sync_scope_note(&["f/**".into(), "u/**".into()], &["f/pat/**".into()]) + .unwrap(), + "\n\nOnly files matching this repository's sync filters deploy on merge: `f/**`, `u/**` (excluding `f/pat/**`)." + ); + } + #[test] fn push_result_pushed_flag() { assert_eq!(