test: git sync E2E tests + auto-manage git sync script version

[hugocasa]

Summary

• E2E git sync tests: 11 comprehensive tests covering sync, promotion mode, multi-repo routing, workspace fork, force_branch, exclude path filtering, rename, commit messages, and multiple object types
• Auto-managed git sync script version: script_path is now optional in git sync config — when omitted, the backend uses LATEST_GIT_SYNC_SCRIPT_PATH constant (hub/28160/...). Existing pinned configs continue to work unchanged
• Frontend fixes: test connection button now works for auto-managed repos (was incorrectly disabled when script_path was unset)
• EE fix: get_github_app_token_internal now resolves null script_path to the default constant instead of rejecting the job as "not a git sync job"
• Remove unused Docker mode (docker-compose.git-sync.yml, run_git_sync.sh)
• Remove Rust unit/integration tests for git sync filtering (fully covered by E2E tests)
• Dedicated CI workflow (.github/workflows/git-sync-test.yml) with relevance check for EE ref changes

E2E Test Coverage

Test	What it verifies
test_script_deploy_syncs_to_git	Basic script deploy → git commit
test_multi_repo_routing	Two repos with different path filters
test_script_update_creates_new_commit	Script update → new commit
test_deploy_multiple_object_types	Script + flow + variable in one repo
test_commit_message_format	Commit message includes path and kind
test_rename_removes_old_file	Create + delete simulating rename
test_promotion_mode	use_individual_branch=True → per-object branches
test_promotion_group_by_folder	Group by folder in promotion mode
test_force_branch_with_wmill_yaml	force_branch with wmill.yaml config
test_exclude_path_filtering	Excluded paths don't trigger sync
test_workspace_fork	Workspace fork creates branches correctly

Key Changes

Backend

• windmill-common/src/workspaces.rs: LATEST_GIT_SYNC_SCRIPT_PATH constant, script_path: Option<String> with effective_script_path() fallback
• backend/src/main.rs: force-cache latest git sync script regardless of hubPaths.json
• windmill-common/src/stats_oss.rs: sync OSS stub signatures with EE telemetry changes (minimal: bool param)
• windmill-api/openapi.yaml: script_path no longer required in EditGitSyncSettings

Frontend

• GitSyncRepositoryCard.svelte: pinned version warning with "Switch to auto-managed" button; test connection enabled for auto-managed repos
• GitSyncContext.svelte.ts: runTestJob no longer requires script_path

EE (windmill-ee-private PR #442)

• git_sync_ee.rs: get_github_app_token_internal resolves null script_path to LATEST_GIT_SYNC_SCRIPT_PATH
• Removed unit tests for git sync filtering (covered by E2E)

CI

• .github/workflows/git-sync-test.yml: builds with enterprise,private,license,zip features; relevance check for EE ref changes; startup health checks with failure exits

Test plan

• All 11 E2E tests pass locally (~3 min)
• check_ee, check_oss, check_ee_full, check_oss_full pass
• Auto-managed repos can test connection (frontend)
• GitHub App auth works with auto-managed script_path (EE fix)
• CI git_sync_e2e job passes

🤖 Generated with Claude Code

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	5d019b8
Status:	✅  Deploy successful!
Preview URL:	https://6333f8d0.windmill.pages.dev
Branch Preview URL:	https://git-sync-tests.windmill.pages.dev

View logs

[claude]

Claude finished @hugocasa's task in 4m 9s —— View job

---

PR #8253 Review: Git Sync E2E Tests + Auto-Manage Script Version

• Gather context and read changed files
• Review E2E test infrastructure (workflow, fixtures, utils)
• Review test cases (git_sync_test.py)
• Review backend changes (main.rs, workspaces, settings, openapi)
• Review frontend changes (GitSync components)
• Post comprehensive review

---

Overall Assessment

This is a solid PR that introduces comprehensive E2E git sync tests (11 tests) and an auto-managed script version mechanism. The test infrastructure is well-designed with proper setup/teardown, dynamic Gitea URLs, and job drain between tests to prevent race conditions.

---

Positive Highlights

1. Well-structured test design — Each test is focused, covers a distinct feature, and follows a clear arrange-act-assert pattern with good assertion messages.
2. Inter-test isolation — The setUp() method draining deployment callback jobs is a smart approach to prevent test interference.
3. Dynamic URL handling — Using GITEA_HOST_URL / GITEA_DOCKER_URL environment variables instead of hardcoded hostnames makes the tests portable between local and CI environments.
4. Auto-managed script versioning — The LATEST_GIT_SYNC_SCRIPT_PATH constant with effective_script_path() is a clean pattern. Making script_path optional (Option<String>) and defaulting to the constant is a good backwards-compatible approach.
5. Frontend UX for pinned versions — The warning alert with an opt-in "Switch to auto-managed" button is a thoughtful migration path for existing users.

---

Issues and Suggestions

1. Test timing fragility — time.sleep() scattered throughout

integration_tests/test/git_sync_test.py (lines 193, 268, 310, 323, etc.)

Every test calls time.sleep(3) after wait_for_sync_jobs(). Since wait_for_sync_jobs() already polls until jobs complete, the extra 3s sleep suggests a race between job completion and git push. This is brittle — if the underlying timing changes, tests could flake. Consider making wait_for_sync_jobs also check that the git commit actually landed (e.g., poll the Gitea API for the latest commit hash) rather than relying on a fixed sleep.

2. Potential UnboundLocalError in wait_for_sync_jobs

integration_tests/test/wmill_integration_test_utils.py:483-495

def wait_for_sync_jobs(self, initial_count: int, min_new: int = 1, timeout: int = 90) -> list:
    start = time.time()
    while time.time() - start < timeout:
        jobs = self.get_completed_jobs(job_kinds="deploymentcallback")
        current_count = len(jobs)
        if current_count >= initial_count + min_new:
            return jobs
        time.sleep(2)
    raise TimeoutError(
        f"Timed out waiting for sync jobs: expected {initial_count + min_new}, "
        f"got {current_count} after {timeout}s"
    )

If timeout <= 0 is somehow passed, the while loop never executes and current_count is undefined when the TimeoutError is raised. While unlikely in practice, initializing current_count = 0 before the loop would be more defensive.

Fix this →

3. Temp directory cleanup

integration_tests/test/git_sync_test.py:117-124

_clone_repo and _clone_repo_all_branches create temporary directories with tempfile.mkdtemp() but never clean them up. While CI runners are ephemeral, locally these would accumulate. Consider using addCleanup(shutil.rmtree, tmp_dir) or a class-level cleanup list.

4. Gitea admin user creation — fallback chain could be simplified

integration_tests/test/wmill_integration_test_utils.py:606-651

The setup_admin() method has a 3-level fallback for creating the Gitea admin user (admin API → sign_up form → admin API again without auth). The second fallback to /api/v1/admin/users without auth (line 638-648) likely won't succeed if the first call with auth returned 401. Consider simplifying or at minimum adding a comment explaining when each path is expected to be taken.

5. docs/git-sync-tests-plan.md — should this ship?

This 644-line planning document is useful for development context but may become stale over time in the repo. Consider whether it belongs in docs/ or should be linked from the PR description instead.

6. CI workflow — no timeout on Gitea startup

.github/workflows/git-sync-test.yml:163-170

The Gitea startup loop runs up to 30 iterations × 2s = 60s, but there's no else clause that fails the step if Gitea never becomes ready. If the loop exits without breaking, the job would silently proceed to the next step with a non-functional Gitea.

for i in $(seq 1 30); do
  if curl -sf http://localhost:3000/api/v1/version > /dev/null 2>&1; then
    echo "Gitea is ready"
    break
  fi
  sleep 2
done
# Missing: exit 1 if Gitea not ready

Same issue exists for the Windmill startup loop (lines 183-189).

Fix this →

7. exclude_path test could be more assertive

integration_tests/test/git_sync_test.py:712-714

The test_exclude_path_filtering waits for only 1 sync job (min_new=1) and then sleeps 5s. The assumption is that the excluded script doesn't trigger a deployment callback at all. This is correct, but the test can't distinguish "excluded script was correctly filtered" from "excluded script's job hasn't arrived yet". The 5s sleep is the only guard. A more robust approach would be to verify the job count didn't increase beyond 1.

8. Frontend: runTestJob checks repo?.script_path which is now undefined for auto-managed repos

frontend/src/lib/components/git_sync/GitSyncContext.svelte.ts:544

async function runTestJob(idx: number) {
    const repo = repositories[idx]
    if (!repo?.git_repo_resource_path || !repo?.script_path) {
        return  // This will always return early for auto-managed repos!
    }

Since auto-managed repos have script_path: undefined, the !repo?.script_path check will cause runTestJob to silently bail out. The "Test connection" button in the UI (GitSyncRepositoryCard.svelte:377) also has disabled={emptyString(repo.script_path)}, so it will be disabled for auto-managed repos. This seems like a bug — auto-managed repos should still be testable.

Fix this →

9. OpenAPI spec removed script_path from required but check backward compat

backend/windmill-api/openapi.yaml removed script_path from a required field list. Ensure that older clients that still send script_path work correctly (they should — since Option<String> will deserialize null or missing fields fine, and the existing value will be preserved).

---

Minor Nits

• git_sync.sql fixture (line 10): Uses hardcoded hub/28160/sync-script-to-git-repo-windmill — consider using a placeholder or referencing the constant to avoid drift. Though since this is a test fixture, it's acceptable.
• unique_name() uses only 8 hex chars from UUID — the collision probability is negligible for test runs, so this is fine.

---

How to Reproduce / Verify Changes via the App

Navigate to any workspace's Settings page, then the Git Sync tab. When adding a new git repository connection, notice that the script_path field is no longer shown — the backend auto-fills it with the latest version. For existing repositories that have a pinned script_path, a yellow warning banner appears saying "Pinned git sync script version" with a button labeled "Switch to auto-managed (requires save)". Clicking it clears the script_path, and saving commits the change. The "Test connection" button behavior for auto-managed repos should also be verified (see issue #8 above about it potentially being disabled).

---

[claude]

Claude finished @hugocasa's task in 2m 25s —— View job

---

PR #8253 Review: Git Sync E2E Tests + Auto-Manage Script Version

• Read and analyze all changed files
• Review backend changes (workspaces, settings, main.rs, openapi)
• Review frontend changes (GitSync components)
• Review E2E test infrastructure and test cases
• Review CI workflow
• Post comprehensive review

---

Overall Assessment

This is a well-structured PR that adds comprehensive E2E test coverage for git sync (11 tests) and introduces auto-managed script versioning. The architecture is sound — making script_path optional with a backend constant fallback is a clean, backwards-compatible approach. The test infrastructure (Gitea integration, dynamic URLs, job drain) is solid.

---

Issues Found

1. deprecated_gitSync_* keys still cached unnecessarily

backend/src/main.rs:246-250 — The new code ensures LATEST_GIT_SYNC_SCRIPT_PATH is always cached by the hub cache. However, hubPaths.json still contains 23 deprecated_gitSync_* entries that are now loaded into the paths HashMap and cached on startup. Since these are deprecated and the backend always uses LATEST_GIT_SYNC_SCRIPT_PATH, the deprecated entries add unnecessary hub-fetch overhead during startup. Consider either removing them from hubPaths.json entirely or filtering out deprecated_* keys in cache_hub_scripts.

2. deprecated_gitSync_latest duplicates LATEST_GIT_SYNC_SCRIPT_PATH

frontend/src/lib/hubPaths.json has "deprecated_gitSync_latest": "hub/28160/sync-script-to-git-repo-windmill" which is the same value as LATEST_GIT_SYNC_SCRIPT_PATH in the Rust backend. If this value is ever bumped, it needs to be updated in both places. Consider whether this dual-source-of-truth could cause drift. The old gitSync key (used by hub.ts) was removed correctly, but downstream consumers reading hubPaths.json for older script versions could still reference these deprecated paths.

3. stats_oss.rs signature change is unrelated and could break OSS builds

backend/windmill-common/src/stats_oss.rs and backend/windmill-api-settings/src/lib.rs — Adding the _minimal: bool parameter to send_stats and get_stats_payload is an EE signature sync change that appears unrelated to git sync. While harmless (OSS stubs just add an ignored parameter), bundling unrelated signature changes increases review surface and potential merge conflicts. Consider splitting this into its own commit or PR.

4. Test timing fragility with time.sleep() calls

integration_tests/test/git_sync_test.py — Every test calls time.sleep(3) (or 5) after wait_for_sync_jobs(). Since wait_for_sync_jobs already polls until the deployment callback job count increases, the additional fixed sleep suggests a race between job completion and the git push actually landing. If the sync script becomes slower (network, load), these sleeps may not be sufficient. Consider polling the Gitea API for the expected commit/file instead.

5. test_exclude_path_filtering timing assumption

integration_tests/test/git_sync_test.py:716-724 — The test waits for exactly 1 sync job and then sleeps 5s, asserting no extra jobs arrived. This can't distinguish "excluded script was correctly filtered" from "excluded script's job just hasn't arrived yet." A more robust approach would be to create another included script after the excluded one and wait for that second job — this confirms the system processed the excluded one's deployment event without triggering a sync.

6. test_rename_removes_old_file doesn't actually verify old file removal

integration_tests/test/git_sync_test.py:497-504 — The test creates a new script and deletes the old one, then verifies the new script exists. However, it never asserts that the old file was removed from the repo. Add an assertion like:

self.assertFalse(
    any(old_name in f for f in files2),
    f"Expected old script '{old_name}' to be removed: {files2}",
)

Fix this →

7. CI: Gitea runs on host network but GITEA_DOCKER_URL points to localhost:3000

.github/workflows/git-sync-test.yml:196 — GITEA_DOCKER_URL: http://localhost:3000. Since Windmill also runs on the host (not in Docker), localhost:3000 works. However, the env var name GITEA_DOCKER_URL is misleading — it implies a Docker-internal address. The code in wmill_integration_test_utils.py:590 defaults GITEA_DOCKER_URL to http://gitea:3000 (Docker service discovery), which would fail if the env var isn't set. The CI sets it correctly, but the naming could confuse contributors running locally without Docker.

8. Gitea admin setup: 3-level fallback is complex

integration_tests/test/wmill_integration_test_utils.py:606-652 — The setup_admin() method has a 3-level fallback (admin API with auth → sign_up form → admin API without auth). The third fallback (API without auth, line 639-648) is unlikely to succeed if the first call with auth returned 401 (meaning no admin exists yet and Gitea requires the first user to be created via the UI/signup endpoint). A comment explaining the expected scenarios for each branch would improve maintainability.

---

Minor Observations

• wait_for_sync_jobs defensive init (wmill_integration_test_utils.py:486): current_count is initialized before the loop via current_count = initial_count, which handles the edge case where the while loop never executes. This was addressed since the previous review.

• Temp directory cleanup (git_sync_test.py:121,132): addCleanup(shutil.rmtree, tmp_dir, ignore_errors=True) is used correctly. This was also addressed since the previous review.

• CI startup health checks (.github/workflows/git-sync-test.yml:171,191): Both Gitea and Windmill now have a curl -sf ... || exit 1 after the loop. This was fixed.

• runTestJob guard (GitSyncContext.svelte.ts:544): The !repo?.script_path check was removed, so auto-managed repos can now be tested. The disabled condition on the button (GitSyncRepositoryCard.svelte:376) was also fixed to check git_repo_resource_path instead of script_path. Both good fixes.

---

Positive Highlights

1. Clean effective_script_path() pattern — The fallback to LATEST_GIT_SYNC_SCRIPT_PATH when script_path is None is elegant and centralizes the logic.
2. Hub cache ensures availability — main.rs explicitly adds the latest git sync script to the cache list, ensuring it's always available even if hubPaths.json doesn't list it.
3. Well-isolated tests — setUp() drains pending deployment callbacks, tearDownClass() disables git sync and cleans up repos/workspaces.
4. Good coverage spread — Tests cover sync, multi-repo routing, updates, multiple object types, commit messages, renames, promotion mode, group_by_folder, force_branch, exclude paths, and workspace forking.
5. Frontend UX — The pinned version warning with "Switch to auto-managed" button is a thoughtful migration path.

---

How to Verify via the App

Navigate to any workspace's Settings > Git Sync tab. When adding a new git repository connection, notice that there is no script_path field — the backend auto-fills it with the latest version. For existing repositories that have a pinned script_path, a yellow warning banner labeled "Pinned git sync script version" should appear, showing the pinned path and a "Switch to auto-managed (requires save)" button. Clicking it clears the script_path to undefined; saving persists the change. The "Test connection" button should be enabled and functional for both pinned and auto-managed repos (previously it was disabled when script_path was empty). Verify that deploying scripts/flows/variables to a git-synced workspace produces commits in the connected repository.

[windmill-internal-app]

🤖 Updated ee-repo-ref.txt to 612d96a66f9d0cfdae335ef3eb4881f3444ce7cd after windmill-ee-private PR #442 was merged.