diff --git a/apps/desktop/src/lib/trpc/routers/changes/security/git-commands.ts b/apps/desktop/src/lib/trpc/routers/changes/security/git-commands.ts index 567ab1a7aca..e8b105fb5bf 100644 --- a/apps/desktop/src/lib/trpc/routers/changes/security/git-commands.ts +++ b/apps/desktop/src/lib/trpc/routers/changes/security/git-commands.ts @@ -1,4 +1,5 @@ import simpleGit from "simple-git"; +import { runWithPostCheckoutHookTolerance } from "../../utils/git-hook-tolerance"; import { assertRegisteredWorktree, assertValidGitPath, @@ -16,6 +17,22 @@ import { * 3. Uses the correct git command syntax */ +async function isCurrentBranch({ + worktreePath, + expectedBranch, +}: { + worktreePath: string; + expectedBranch: string; +}): Promise { + try { + const git = simpleGit(worktreePath); + const currentBranch = (await git.revparse(["--abbrev-ref", "HEAD"])).trim(); + return currentBranch === expectedBranch; + } catch { + return false; + } +} + /** * Switch to a branch. * @@ -42,21 +59,28 @@ export async function gitSwitchBranch( const git = simpleGit(worktreePath); - try { - // Prefer `git switch` - unambiguous branch operation (git 2.23+) - await git.raw(["switch", branch]); - } catch (switchError) { - // Check if it's because `switch` command doesn't exist (old git < 2.23) - // Git outputs: "git: 'switch' is not a git command. See 'git --help'." - const errorMessage = String(switchError); - if (errorMessage.includes("is not a git command")) { - // Fallback for older git versions - // Note: checkout WITHOUT -- is correct for branches - await git.checkout(branch); - } else { - throw switchError; - } - } + await runWithPostCheckoutHookTolerance({ + context: `Switched branch to "${branch}" in ${worktreePath}`, + run: async () => { + try { + // Prefer `git switch` - unambiguous branch operation (git 2.23+) + await git.raw(["switch", branch]); + } catch (switchError) { + // Check if it's because `switch` command doesn't exist (old git < 2.23) + // Git outputs: "git: 'switch' is not a git command. See 'git --help'." + const errorMessage = String(switchError); + if (errorMessage.includes("is not a git command")) { + // Fallback for older git versions + // Note: checkout WITHOUT -- is correct for branches + await git.checkout(branch); + } else { + throw switchError; + } + } + }, + didSucceed: async () => + isCurrentBranch({ worktreePath, expectedBranch: branch }), + }); } /** diff --git a/apps/desktop/src/lib/trpc/routers/utils/git-hook-tolerance.test.ts b/apps/desktop/src/lib/trpc/routers/utils/git-hook-tolerance.test.ts new file mode 100644 index 00000000000..69aa631afce --- /dev/null +++ b/apps/desktop/src/lib/trpc/routers/utils/git-hook-tolerance.test.ts @@ -0,0 +1,51 @@ +import { describe, expect, test } from "bun:test"; +import { runWithPostCheckoutHookTolerance } from "./git-hook-tolerance"; + +describe("runWithPostCheckoutHookTolerance", () => { + test("treats post-checkout hook failures as non-fatal when operation succeeded", async () => { + const hookError = Object.assign( + new Error("husky - post-checkout script failed"), + { + stderr: "husky - command not found in PATH=...", + }, + ); + + await expect( + runWithPostCheckoutHookTolerance({ + context: "Switched branch", + run: async () => { + throw hookError; + }, + didSucceed: async () => true, + }), + ).resolves.toBeUndefined(); + }); + + test("re-throws hook failures when operation did not succeed", async () => { + const hookError = new Error("post-checkout hook failed"); + + await expect( + runWithPostCheckoutHookTolerance({ + context: "Switched branch", + run: async () => { + throw hookError; + }, + didSucceed: async () => false, + }), + ).rejects.toThrow("post-checkout"); + }); + + test("re-throws non-hook failures", async () => { + const genericError = new Error("fatal: '../worktree' already exists"); + + await expect( + runWithPostCheckoutHookTolerance({ + context: "Created worktree", + run: async () => { + throw genericError; + }, + didSucceed: async () => true, + }), + ).rejects.toThrow("already exists"); + }); +}); diff --git a/apps/desktop/src/lib/trpc/routers/utils/git-hook-tolerance.ts b/apps/desktop/src/lib/trpc/routers/utils/git-hook-tolerance.ts new file mode 100644 index 00000000000..361b85cf9a0 --- /dev/null +++ b/apps/desktop/src/lib/trpc/routers/utils/git-hook-tolerance.ts @@ -0,0 +1,67 @@ +interface GitCommandException extends Error { + stdout?: string; + stderr?: string; +} + +function getErrorText(error: unknown): string { + if (error instanceof Error) { + const parts = [error.message]; + const gitError = error as GitCommandException; + if (typeof gitError.stderr === "string" && gitError.stderr.trim()) { + parts.push(gitError.stderr); + } + if (typeof gitError.stdout === "string" && gitError.stdout.trim()) { + parts.push(gitError.stdout); + } + return parts.join("\n"); + } + + return String(error); +} + +export function isPostCheckoutHookFailure(error: unknown): boolean { + const text = getErrorText(error).toLowerCase(); + if (!text.includes("post-checkout")) { + return false; + } + + return ( + text.includes("hook") || + text.includes("husky") || + text.includes("command not found") + ); +} + +export async function runWithPostCheckoutHookTolerance({ + run, + didSucceed, + context, +}: { + run: () => Promise; + didSucceed: () => Promise; + context: string; +}): Promise { + try { + await run(); + } catch (error) { + if (!isPostCheckoutHookFailure(error)) { + throw error; + } + + let succeeded = false; + try { + succeeded = await didSucceed(); + } catch { + succeeded = false; + } + + if (!succeeded) { + throw error; + } + + const message = getErrorText(error); + console.warn( + `[git] ${context} but post-checkout hook failed (non-fatal): ${message}`, + ); + } +} diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.ts index 226f14dd85a..043c65a374c 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.ts @@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { execSync } from "node:child_process"; import { existsSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; import { join } from "node:path"; +import { createWorktree } from "./git"; // We need to test the internal functions, so we'll import the module // and test the exported functions that use them @@ -20,6 +21,14 @@ function createTestRepo(name: string): string { return repoPath; } +function seedCommit(repoPath: string): void { + writeFileSync(join(repoPath, "README.md"), "# test\n"); + execSync("git add . && git commit -m 'init'", { + cwd: repoPath, + stdio: "ignore", + }); +} + describe("LFS Detection", () => { beforeEach(() => { mkdirSync(TEST_DIR, { recursive: true }); @@ -313,3 +322,56 @@ describe("Shell Environment", () => { expect(env.PATH || env.Path).toBeDefined(); }); }); + +describe("createWorktree hook tolerance", () => { + beforeEach(() => { + mkdirSync(TEST_DIR, { recursive: true }); + }); + + afterEach(() => { + if (existsSync(TEST_DIR)) { + rmSync(TEST_DIR, { recursive: true, force: true }); + } + }); + + test("continues when post-checkout hook fails but worktree is created", async () => { + const repoPath = createTestRepo("worktree-hook-failure"); + seedCommit(repoPath); + + const hookPath = join(repoPath, ".git", "hooks", "post-checkout"); + writeFileSync( + hookPath, + "#!/bin/sh\necho 'post-checkout failed' >&2\nexit 1\n", + ); + execSync(`chmod +x "${hookPath}"`); + + const worktreePath = join(TEST_DIR, "worktree-hook-failure-wt"); + await createWorktree( + repoPath, + "feature/hook-failure", + worktreePath, + "HEAD", + ); + + expect(existsSync(worktreePath)).toBe(true); + const currentBranch = execSync("git rev-parse --abbrev-ref HEAD", { + cwd: worktreePath, + }) + .toString() + .trim(); + expect(currentBranch).toBe("feature/hook-failure"); + }); + + test("throws when destination path exists but worktree is not created", async () => { + const repoPath = createTestRepo("worktree-existing-path"); + seedCommit(repoPath); + + const worktreePath = join(TEST_DIR, "worktree-existing-path-wt"); + mkdirSync(worktreePath, { recursive: true }); + writeFileSync(join(worktreePath, "keep.txt"), "keep"); + + await expect( + createWorktree(repoPath, "feature/existing-path", worktreePath, "HEAD"), + ).rejects.toThrow("already exists"); + }); +}); diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts b/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts index 70bad22f411..6d6e16ccf10 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts @@ -1,13 +1,14 @@ import { execFile, spawn } from "node:child_process"; import { randomUUID } from "node:crypto"; import { mkdir, readFile, rename, stat } from "node:fs/promises"; -import { dirname, join } from "node:path"; +import { dirname, join, resolve } from "node:path"; import { promisify } from "node:util"; import friendlyWords = require("friendly-words"); import type { BranchPrefixMode } from "@superset/local-db"; import simpleGit, { type StatusResult } from "simple-git"; +import { runWithPostCheckoutHookTolerance } from "../../utils/git-hook-tolerance"; import { checkGitLfsAvailable, getShellEnvironment } from "./shell-env"; const execFileAsync = promisify(execFile); @@ -32,6 +33,87 @@ function isExecFileException(error: unknown): error is ExecFileException { ); } +async function isWorktreeRegistered({ + mainRepoPath, + worktreePath, + env, +}: { + mainRepoPath: string; + worktreePath: string; + env: Record; +}): Promise { + try { + const { stdout } = await execFileAsync( + "git", + ["-C", mainRepoPath, "worktree", "list", "--porcelain"], + { env, timeout: 10_000 }, + ); + + const expectedPath = resolve(worktreePath); + for (const line of stdout.split("\n")) { + if (!line.startsWith("worktree ")) { + continue; + } + + const listedPath = line.slice("worktree ".length).trim(); + if (resolve(listedPath) === expectedPath) { + return true; + } + } + + return false; + } catch { + return false; + } +} + +/** + * Runs `git worktree add`, tolerating hook failures. + * Post-checkout hooks can exit non-zero after the worktree is created. + * If the worktree exists on disk despite the error, we warn and continue. + */ +async function execWorktreeAdd({ + mainRepoPath, + args, + env, + worktreePath, + timeout = 120_000, +}: { + mainRepoPath: string; + args: string[]; + env: Record; + worktreePath: string; + timeout?: number; +}): Promise { + await runWithPostCheckoutHookTolerance({ + context: `Worktree created at ${worktreePath}`, + run: async () => { + await execFileAsync("git", args, { env, timeout }); + }, + didSucceed: async () => + isWorktreeRegistered({ mainRepoPath, worktreePath, env }), + }); +} + +async function checkoutBranchWithHookTolerance({ + repoPath, + targetBranch, + run, +}: { + repoPath: string; + targetBranch: string; + run: () => Promise; +}): Promise { + await runWithPostCheckoutHookTolerance({ + context: `Switched branch to "${targetBranch}" in ${repoPath}`, + run, + didSucceed: async () => { + const current = await getCurrentBranch(repoPath); + return current === targetBranch; + }, + }); +} + async function getGitEnv(): Promise> { const shellEnv = await getShellEnvironment(); const result: Record = {}; @@ -457,9 +539,9 @@ export async function createWorktree( } } - await execFileAsync( - "git", - [ + await execWorktreeAdd({ + mainRepoPath, + args: [ "-C", mainRepoPath, "worktree", @@ -472,8 +554,9 @@ export async function createWorktree( // creating a new branch from a remote branch like origin/main. `${startPoint}^{commit}`, ], - { env, timeout: 120_000 }, - ); + env, + worktreePath, + }); // Enable autoSetupRemote so the first `git push` automatically creates // the remote branch and sets upstream (like `git push -u origin `). @@ -558,28 +641,24 @@ export async function createWorktreeFromExistingBranch({ } } - // First, check if the branch exists locally const git = simpleGit(mainRepoPath); const localBranches = await git.branchLocal(); const branchExistsLocally = localBranches.all.includes(branch); if (branchExistsLocally) { - // Branch exists locally - just checkout into the worktree - await execFileAsync( - "git", - ["-C", mainRepoPath, "worktree", "add", worktreePath, branch], - { env, timeout: 120_000 }, - ); + await execWorktreeAdd({ + mainRepoPath, + args: ["-C", mainRepoPath, "worktree", "add", worktreePath, branch], + env, + worktreePath, + }); } else { - // Branch doesn't exist locally - check if it's a remote branch const remoteBranches = await git.branch(["-r"]); const remoteBranchName = `origin/${branch}`; if (remoteBranches.all.includes(remoteBranchName)) { - // Create worktree with local tracking branch from remote - // This creates a new local branch that tracks the remote - await execFileAsync( - "git", - [ + await execWorktreeAdd({ + mainRepoPath, + args: [ "-C", mainRepoPath, "worktree", @@ -590,8 +669,9 @@ export async function createWorktreeFromExistingBranch({ worktreePath, remoteBranchName, ], - { env, timeout: 120_000 }, - ); + env, + worktreePath, + }); } else { throw new Error( `Branch "${branch}" does not exist locally or on remote`, @@ -1392,18 +1472,36 @@ export async function checkoutBranch( const localBranches = await git.branchLocal(); if (localBranches.all.includes(branch)) { - await git.checkout(branch); + await checkoutBranchWithHookTolerance({ + repoPath, + targetBranch: branch, + run: async () => { + await git.checkout(branch); + }, + }); return; } const remoteBranches = await git.branch(["-r"]); const remoteBranchName = `origin/${branch}`; if (remoteBranches.all.includes(remoteBranchName)) { - await git.checkout(["-b", branch, "--track", remoteBranchName]); + await checkoutBranchWithHookTolerance({ + repoPath, + targetBranch: branch, + run: async () => { + await git.checkout(["-b", branch, "--track", remoteBranchName]); + }, + }); return; } - await git.checkout(branch); + await checkoutBranchWithHookTolerance({ + repoPath, + targetBranch: branch, + run: async () => { + await git.checkout(branch); + }, + }); } /** @@ -1708,11 +1806,12 @@ export async function createWorktreeFromPr({ } } - await execFileAsync( - "git", - ["-C", mainRepoPath, "worktree", "add", worktreePath, branchName], - { env, timeout: 120_000 }, - ); + await execWorktreeAdd({ + mainRepoPath, + args: ["-C", mainRepoPath, "worktree", "add", worktreePath, branchName], + env, + worktreePath, + }); if (localCommit !== remoteCommit) { await execFileAsync( @@ -1727,7 +1826,7 @@ export async function createWorktreeFromPr({ args.push("--track"); } args.push("-b", branchName, worktreePath, remoteRef); - await execFileAsync("git", args, { env, timeout: 120_000 }); + await execWorktreeAdd({ mainRepoPath, args, env, worktreePath }); } // Enable autoSetupRemote so `git push` just works without -u flag.