From a72db59a2fbcbc90b3754bba1dcde23f32f9e960 Mon Sep 17 00:00:00 2001 From: kai392 Date: Thu, 30 Jul 2026 04:58:54 +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` (blocked_worktree_preparation_failed), and only the second attempt (the fetch+reset path, which DWIM-creates the local branch) works. - repo-clone.ts: after a successful clone, run `git checkout ` and return the identical `{ ok:false, error: stderr || "git_checkout_failed" }` shape the existing-clone branch uses. No fetch/reset is added -- a fresh clone is already at origin's tip. The isUnsafeGitArgValue guard still runs first. - attempt-worktree.ts: the clone/checkout-failure return now forwards `repoPath` (ensureRepoCloned always sets it), so a clone/checkout failure and a worktree-add failure surface the same {ok:false, repoPath, error} shape -- an unknown base branch now fails at the checkout step, so callers keep getting repoPath on that path. Tests: three named repo-clone regression cases (checkout 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 per-repo concurrency test's pinned git-op sequence is updated for the new checkout (its non-overlapping guarantee is unchanged); the attempt-worktree unknown-base-branch test now asserts the {ok:false, repoPath, error} shape from the checkout step. All new assertions fail against the current code. Closes #9682 Co-Authored-By: Claude Opus 4.8 --- .../loopover-miner/lib/attempt-worktree.ts | 6 ++- packages/loopover-miner/lib/repo-clone.ts | 7 +++ test/unit/miner-attempt-worktree.test.ts | 8 +-- test/unit/miner-repo-clone.test.ts | 54 ++++++++++++++++++- 4 files changed, 68 insertions(+), 7 deletions(-) diff --git a/packages/loopover-miner/lib/attempt-worktree.ts b/packages/loopover-miner/lib/attempt-worktree.ts index ca17223a5e..d1ea99b16b 100644 --- a/packages/loopover-miner/lib/attempt-worktree.ts +++ b/packages/loopover-miner/lib/attempt-worktree.ts @@ -81,8 +81,10 @@ export async function prepareAttemptWorktree( }); // ensureRepoCloned's own EnsureRepoClonedResult declares `error` optional, but every one of its real ok:false // return sites (repo-clone.ts) sets a real, non-empty error string -- a non-null assertion here rather than a - // fake fallback string, since that fallback would be genuinely unreachable dead code. - if (!cloneResult.ok) return { ok: false, error: cloneResult.error! }; + // fake fallback string, since that fallback would be genuinely unreachable dead code. repoPath is always set on + // its failures too, so forward it (matching the worktree-add failure below) -- a clone/checkout failure and a + // worktree-add failure now surface the same {ok:false, repoPath, error} shape to callers. + if (!cloneResult.ok) return { ok: false, repoPath: cloneResult.repoPath, error: cloneResult.error! }; const exec = options.exec ?? createRealWorktreeExec(options.timeoutMs); const baseBranch = typeof options.baseBranch === "string" && options.baseBranch.trim() ? options.baseBranch.trim() : "main"; 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-attempt-worktree.test.ts b/test/unit/miner-attempt-worktree.test.ts index 900faac358..c1e16ccb92 100644 --- a/test/unit/miner-attempt-worktree.test.ts +++ b/test/unit/miner-attempt-worktree.test.ts @@ -138,9 +138,11 @@ describe("prepareAttemptWorktree / cleanupAttemptWorktree (#5132)", () => { expect(firstCwd).toBe(root); }); - it("returns ok:false with git's real stderr when git worktree add fails (e.g. an unknown base branch)", async () => { - // Real git subprocess round trip (origin init + a real clone + a failing `git worktree add`). See the - // REGRESSION test above for why this needs an explicit timeout. + it("returns ok:false with git's real stderr and the repoPath when an unknown base branch can't be prepared", async () => { + // Real git subprocess round trip (origin init + a real clone + a failing base-branch checkout). Since #9682 + // the fresh-clone path checks out baseBranch, so an unknown base branch now fails there (inside + // ensureRepoCloned) rather than at `git worktree add`; either way prepareAttemptWorktree surfaces + // {ok:false, repoPath, error}. See the REGRESSION test above for why this needs an explicit timeout. const root = tempRoot("loopover-miner-attempt-worktree-addfail-"); const originPath = initOriginRepo(root); const cloneBaseDir = join(root, "cache"); 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 () => {