From a9050f2b59e780ae4001583c18a2bff9f706beff Mon Sep 17 00:00:00 2001 From: kai392 Date: Thu, 30 Jul 2026 04:47:56 +0800 Subject: [PATCH] fix(miner): check out baseBranch on the fresh-clone path so the first attempt succeeds (#9682) `ensureRepoClonedUnlocked`'s docstring promises both branches land on the base branch, but the fresh-clone branch returned right after `git clone`. A plain clone creates only origin's default-HEAD local branch, so the consumer's `git worktree add -b ` cannot rev-parse a bare non-default `` -- the first attempt against any repo whose base isn't the origin default fails with `invalid reference` (surfacing as blocked_worktree_preparation_failed), and only the second attempt (which hits the fetch+reset path that DWIM-creates the local branch) works. - After a successful clone, run `git checkout ` and return the identical `{ ok:false, error: stderr || "git_checkout_failed" }` shape the existing-clone branch already uses. No fetch/reset is added -- a fresh clone is already at origin's tip. The isUnsafeGitArgValue guard still runs first. Tests: three named regression cases (checkout happens after clone on the first-use path; a failing checkout returns git_checkout_failed / stderr; the fresh path emits exactly clone+checkout, no fetch/reset). The existing per-repo concurrency test's pinned git-op sequence is updated to include the new checkout (its non-overlapping guarantee is unchanged). All new assertions fail against the current code. Closes #9682 Co-Authored-By: Claude Opus 4.8 --- packages/loopover-miner/lib/repo-clone.ts | 7 +++ test/unit/miner-repo-clone.test.ts | 54 ++++++++++++++++++++++- 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/packages/loopover-miner/lib/repo-clone.ts b/packages/loopover-miner/lib/repo-clone.ts index 64d28fbb56..ecd2725e1b 100644 --- a/packages/loopover-miner/lib/repo-clone.ts +++ b/packages/loopover-miner/lib/repo-clone.ts @@ -326,6 +326,13 @@ async function ensureRepoClonedUnlocked( } const cloned = await runGit(["clone", cloneUrl, repoPath], cloneBaseDir, timeoutMs); if (!cloned.ok) return { ok: false, repoPath, error: cloned.stderr || "git_clone_failed" }; + // A plain `git clone` creates exactly one local branch (origin's default HEAD). When baseBranch is anything + // else, `git worktree add -b ` in the consumer can't rev-parse a bare + // `` name, so the first attempt fails with `invalid reference` until a later run's fetch+reset + // path DWIM-creates it. Check it out now (same call + error shape as the existing-clone path below); a + // fresh clone is already at origin's tip, so no fetch/reset is needed here. + const checkedOut = await runGit(["checkout", baseBranch], repoPath, timeoutMs); + if (!checkedOut.ok) return { ok: false, repoPath, error: checkedOut.stderr || "git_checkout_failed" }; return { ok: true, repoPath }; } diff --git a/test/unit/miner-repo-clone.test.ts b/test/unit/miner-repo-clone.test.ts index 8f0855669b..9fe399956b 100644 --- a/test/unit/miner-repo-clone.test.ts +++ b/test/unit/miner-repo-clone.test.ts @@ -185,6 +185,52 @@ describe("ensureRepoCloned (#5132)", () => { expect(result.error).toBe("git_clone_failed"); }); + it("REGRESSION (#9682): checks out baseBranch on the FIRST-USE clone path, after the clone (injected runGit)", async () => { + // Fresh cache dir + injected runGit -> existsSync(repoPath) is false -> the first-use clone branch runs. + const root = tempRoot("loopover-miner-repo-clone-freshcheckout-"); + const cloneBaseDir = join(root, "cache"); + const calls: string[][] = []; + const runGit = async (args: string[]) => { + calls.push(args); + return { ok: true, stdout: "", stderr: "" }; + }; + const result = await ensureRepoCloned("acme/widgets", { cloneBaseDir, remoteUrl: "unused", baseBranch: "develop", runGit }); + expect(result.ok).toBe(true); + // Before the fix the first-use path returned right after `clone`, never checking out `develop`. + expect(calls[0]?.[0]).toBe("clone"); + expect(calls.some((args) => args[0] === "checkout" && args[1] === "develop")).toBe(true); + }); + + it("REGRESSION (#9682): a failing checkout on the first-use clone path returns git_checkout_failed (injected runGit)", async () => { + const root = tempRoot("loopover-miner-repo-clone-freshcheckoutfail-"); + const cloneBaseDir = join(root, "cache"); + const runGit = async (args: string[]) => (args[0] === "checkout" ? { ok: false, stdout: "", stderr: "" } : { ok: true, stdout: "", stderr: "" }); + const result = await ensureRepoCloned("acme/widgets", { cloneBaseDir, remoteUrl: "unused", baseBranch: "develop", runGit }); + expect(result.ok).toBe(false); + expect(result.error).toBe("git_checkout_failed"); + + // And git's own stderr is surfaced verbatim when present, matching the existing-clone checkout branch. + const runGitWithStderr = async (args: string[]) => (args[0] === "checkout" ? { ok: false, stdout: "", stderr: "detached HEAD boom" } : { ok: true, stdout: "", stderr: "" }); + const withStderr = await ensureRepoCloned("acme/widgets", { cloneBaseDir: join(tempRoot("loopover-miner-repo-clone-freshcheckoutfail2-"), "cache"), remoteUrl: "unused", baseBranch: "develop", runGit: runGitWithStderr }); + expect(withStderr.error).toBe("detached HEAD boom"); + }); + + it("REGRESSION (#9682): the first-use clone path does NOT fetch or hard-reset (a fresh clone is already at origin's tip)", async () => { + const root = tempRoot("loopover-miner-repo-clone-freshnofetch-"); + const cloneBaseDir = join(root, "cache"); + const calls: string[][] = []; + const runGit = async (args: string[]) => { + calls.push(args); + return { ok: true, stdout: "", stderr: "" }; + }; + const result = await ensureRepoCloned("acme/widgets", { cloneBaseDir, remoteUrl: "unused", baseBranch: "develop", runGit }); + expect(result.ok).toBe(true); + expect(calls.some((args) => args[0] === "fetch")).toBe(false); + expect(calls.some((args) => args[0] === "reset")).toBe(false); + // Exactly the clone + checkout, in that order. + expect(calls.map((args) => args[0])).toEqual(["clone", "checkout"]); + }); + it("rejects a dash-prefixed baseBranch before invoking git (#5923)", async () => { // Real origin init + a real clone before the injected-runGit assertion. See the first test in this // block for why this needs an explicit timeout. @@ -259,8 +305,12 @@ describe("ensureRepoCloned per-repo concurrency guard (#6762)", () => { const [firstResult, secondResult] = await Promise.all([first, second]); expect(firstResult.ok).toBe(true); expect(secondResult.ok).toBe(true); - // Strict, non-overlapping ordering: first runs start->end fully before second starts. - expect(events).toEqual(["start:clone", "end:clone", "start:clone", "end:clone"]); + // Strict, non-overlapping ordering: the first call runs its full fresh-clone git sequence (clone then the + // #9682 baseBranch checkout) start->end before the second call starts any git op of its own. + expect(events).toEqual([ + "start:clone", "end:clone", "start:checkout", "end:checkout", + "start:clone", "end:clone", "start:checkout", "end:checkout", + ]); }); it("does NOT serialize across DIFFERENT repos -- they run in parallel", async () => {