From 0257475ebb35e428da51609d6cf2f233c3dcaac3 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 22 May 2026 07:54:12 +0000 Subject: [PATCH] refactor(cli): wmill sync git-deploy stops committing; caller owns commit+push (#9284) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Single contract for the deployment-callback path: the CLI does branch checkout + pull, the caller (hub script in production, test in test) does git add + commit + push. This restores the WIN-1974 invariant — GPG setup and `git commit` run back-to-back in the same process, so the agent's pre-warmed passphrase cache is still warm at sign time — without needing a `--skip-commit` flag for the hub case and a default "also-commit" for everything else. Same behavior in every call site. Changes: - sync.ts: drop the gitSyncDeployPush call from pull()'s deploy path (both the onlyCreateBranch fast-return and the post-pull commit). `gitSyncDeployPush` stays exported for any caller that wants the same commit/push semantics — just not invoked by the CLI subcommand. - gitsync_promotion.test.ts: e2e test now does its own git add + commit + push after `wmill sync git-deploy`, mirroring what the hub script does in production. Same regression coverage (wm_deploy branch created in Case A, main untouched; main updated in Case B, no new wm_deploy). CLI typecheck unchanged (two pre-existing TarAsZip errors at lines 2578/3307, present before this PR). All 743 unit tests still pass. The accompanying hub script (option-C — CLI for branch+pull, script for commit+push) lives at /tmp/git-sync-diff/sync-script-to-git-repo-windmill.option-C.ts. Once published, a follow-up bumps LATEST_GIT_SYNC_SCRIPT_PATH to its id. Co-authored-by: Claude Opus 4.7 (1M context) --- cli/src/commands/sync/sync.ts | 28 +++++++--------------------- cli/test/gitsync_promotion.test.ts | 29 +++++++++++++++++++++++++---- 2 files changed, 32 insertions(+), 25 deletions(-) diff --git a/cli/src/commands/sync/sync.ts b/cli/src/commands/sync/sync.ts index f9c648eef0..d4aa3d818c 100644 --- a/cli/src/commands/sync/sync.ts +++ b/cli/src/commands/sync/sync.ts @@ -2503,14 +2503,8 @@ export async function pull( } if (opts.onlyCreateBranch) { - gitSyncDeployPush({ - items: deployItems, - authorName: process.env["WM_USERNAME"] || "windmill", - authorEmail: process.env["WM_EMAIL"] || "windmill@windmill.dev", - committerName: opts.gitCommitterName, - committerEmail: opts.gitCommitterEmail, - onlyCreateBranch: true, - }); + // Branch is checked out locally; the caller pushes it. Symmetric with + // the non-onlyCreateBranch path: CLI does branch + pull, never push. return; } } @@ -2982,19 +2976,11 @@ export async function pull( log.warn(`Failed to pull shared UI folder: ${e}`); } - // Git-sync deployment-callback mode: commit the pulled files and push the - // current branch (the wm_deploy/fork branch checked out above, or the base - // branch in workspace-wide mode). - if (opts.gitDeployItems !== undefined && !opts.onlyCreateBranch) { - const deployItems: GitSyncDeployItem[] = JSON.parse(opts.gitDeployItems); - gitSyncDeployPush({ - items: deployItems, - authorName: process.env["WM_USERNAME"] || "windmill", - authorEmail: process.env["WM_EMAIL"] || "windmill@windmill.dev", - committerName: opts.gitCommitterName, - committerEmail: opts.gitCommitterEmail, - }); - } + // Git-sync deployment-callback mode stops here: branch checkout + pull have + // happened, but commit + push are the caller's job. The hub script does + // them in-process with `set_gpg_signing_secret` so the agent's pre-warmed + // passphrase cache is still warm at sign time (WIN-1974). `gitSyncDeployPush` + // stays exported for callers that want the same commit/push behavior. } // Internal git-sync deployment-callback entrypoint. Invoked only by the diff --git a/cli/test/gitsync_promotion.test.ts b/cli/test/gitsync_promotion.test.ts index f809d2032b..5b73e8f8e1 100644 --- a/cli/test/gitsync_promotion.test.ts +++ b/cli/test/gitsync_promotion.test.ts @@ -6,10 +6,11 @@ * `use_individual_branch` is set — NOT straight to the cloned base branch * (e.g. a protected `main`, which fails with GH006). * - * The CLI's `wmill sync pull --git-deploy-items ...` now owns that branch - * checkout + commit + push (previously hub-script-only, hence untestable). - * This drives it against a real local bare repo so the regression is caught - * deterministically, with no network and no GitHub. + * Contract: `wmill sync git-deploy` does branch checkout + pull only. Commit + * + push are the caller's job — the hub script does them in the same process + * as `set_gpg_signing_secret` so the GPG agent's passphrase cache is still + * warm at sign time (WIN-1974). This test replicates the caller half (git + * add + commit + push) inline so the full promotion regression stays caught. */ import { expect, test } from "bun:test"; @@ -121,6 +122,23 @@ test.skipIf(shouldSkipOnCI())( { path_type: "script", path: "f/promo/foo", commit_msg: "deploy foo" }, ]); + // Caller-half: stage anything the CLI's pull dropped, commit on the + // current branch (which the CLI just checked out), and push. Mirrors + // what the hub script does in production after `wmill sync git-deploy`. + const commitAndPush = (work: string) => { + git(work, "config", "user.email", "test@windmill.dev"); + git(work, "config", "user.name", "test"); + git(work, "add", "-A"); + try { + git(work, "diff", "--cached", "--quiet"); + // Exit 0 = nothing staged; nothing to commit. Still push the + // (possibly new) branch ref so the assertions see it. + } catch { + git(work, "commit", "-m", "deploy foo"); + } + git(work, "push", "--porcelain", "-u", "origin", "HEAD"); + }; + // --- Case A: use_individual_branch=true -> wm_deploy branch, main untouched --- const workA = await mkdtemp(join(tmpdir(), "wmill_promo_a_")); git(workA, "clone", `file://${bareDir}`, "."); @@ -141,6 +159,7 @@ test.skipIf(shouldSkipOnCI())( workA, ); expect(resA.code).toBe(0); + commitAndPush(workA); const branchesA = remoteBranches(bareDir); const expectedBranch = `refs/heads/wm_deploy/${ws}/script/f__promo__foo`; @@ -167,6 +186,8 @@ test.skipIf(shouldSkipOnCI())( workB, ); expect(resB.code).toBe(0); + commitAndPush(workB); + expect(remoteHead(bareDir, "main")).not.toBe(seedMain); expect( remoteBranches(bareDir).filter((b) => b.includes("wm_deploy")).length,