diff --git a/.agents/skills/verify-mcode/references/features/thread-startup-progress.md b/.agents/skills/verify-mcode/references/features/thread-startup-progress.md index 747368d03..51a32ad6f 100644 --- a/.agents/skills/verify-mcode/references/features/thread-startup-progress.md +++ b/.agents/skills/verify-mcode/references/features/thread-startup-progress.md @@ -3,6 +3,7 @@ ## Behavior - Local, new-worktree, existing-worktree, and PR-created threads use the shared startup progress display. Selecting a PR only records its branch and PR number; the PR ref is fetched when startup begins, before its checkout is created. +- The startup fetch never rewrites a local branch that holds unpushed or divergent commits: it fast-forwards a branch that is merely behind, leaves an ahead branch alone, and fails startup explicitly when the histories have diverged. - The visible steps follow the operations that make the selected checkout ready. - The activity line uses chat-body type, a shared directional shimmer for its worktree icon and text while startup runs, and a static readable label when reduced motion is requested. - `More details` is a native collapsed disclosure. When opened, its live log receives checkout and Setup output. diff --git a/.agents/skills/verify-mcode/scripts/runtime.mjs b/.agents/skills/verify-mcode/scripts/runtime.mjs index 31c252638..abf4feca3 100644 --- a/.agents/skills/verify-mcode/scripts/runtime.mjs +++ b/.agents/skills/verify-mcode/scripts/runtime.mjs @@ -70,6 +70,7 @@ const FOCUSED_TEST_FILES = [ "src/features/agents/canonical/__tests__/canonical-agent-event-sink.test.ts", "src/features/agents/collaboration/adapters/__tests__/codex-collaboration-event-adapter.test.ts", "src/features/providers/composition/__tests__/provider-event-ingress.test.ts", + "src/features/projects/git/__tests__/git-repository-fetch.test.ts", "src/features/projects/git/__tests__/git-service-push.test.ts", ]; const CHECK_PHASES = [ diff --git a/apps/server/src/features/projects/git/__tests__/git-repository-fetch.test.ts b/apps/server/src/features/projects/git/__tests__/git-repository-fetch.test.ts new file mode 100644 index 000000000..52f4347ee --- /dev/null +++ b/apps/server/src/features/projects/git/__tests__/git-repository-fetch.test.ts @@ -0,0 +1,327 @@ +/** + * Real-Git regression tests for issue #1730: fetchBranchAt must never silently + * discard local-only branch history. Exercises the production + * GitRepositoryService through RealGitExecutor against disposable repositories. + */ +import "reflect-metadata"; +import * as NodeChildProcess from "node:child_process"; +import * as NodeFS from "node:fs"; +import * as NodeOS from "node:os"; +import * as NodePath from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import type { WorkspaceRepo } from "../../persistence/workspace-repo.js"; +import { RealGitExecutor } from "../execution/real-git-executor.js"; +import { GitRepositoryService } from "../git-repository-service.js"; + +const tempDirs: string[] = []; + +afterEach(() => { + for (const dir of tempDirs.splice(0)) { + NodeFS.rmSync(dir, { recursive: true, force: true }); + } +}); + +function git(cwd: string, ...args: string[]): string { + return NodeChildProcess.execFileSync("git", ["-C", cwd, ...args], { + encoding: "utf8", + timeout: 15_000, + }).trim(); +} + +function commitFile(root: string, name: string, content: string, message: string): void { + NodeFS.writeFileSync(NodePath.join(root, name), content); + git(root, "add", name); + git(root, "commit", "-m", message); +} + +interface RepoFixture { + root: string; + remote: string; + service: GitRepositoryService; +} + +/** Disposable clone-shaped repo: main committed and pushed to a bare origin. */ +function createRepo(): RepoFixture { + const root = NodeFS.mkdtempSync(NodePath.join(NodeOS.tmpdir(), "mcode-fetch-1730-")); + tempDirs.push(root); + git(root, "init", "-b", "main"); + git(root, "config", "commit.gpgSign", "false"); + git(root, "config", "core.hooksPath", root); + git(root, "config", "user.name", "Fetch Regression"); + git(root, "config", "user.email", "fetch@example.invalid"); + + const remote = NodeFS.mkdtempSync(NodePath.join(NodeOS.tmpdir(), "mcode-fetch-remote-1730-")); + tempDirs.push(remote); + NodeChildProcess.execFileSync("git", ["init", "--bare", remote], { + encoding: "utf8", + timeout: 15_000, + }); + git(root, "remote", "add", "origin", remote); + + commitFile(root, "base.txt", "base", "base"); + git(root, "push", "-u", "origin", "main"); + + const service = new GitRepositoryService( + { findById: () => ({ path: root }) } as unknown as WorkspaceRepo, + new RealGitExecutor(), + ); + return { root, remote, service }; +} + +/** Push a commit that exists only on origin/, advancing it past the local tip. */ +function pushRemoteOnlyCommit(fixture: RepoFixture, branch: string, message: string): void { + const { root } = fixture; + const current = git(root, "rev-parse", "--abbrev-ref", "HEAD"); + git(root, "checkout", "-b", "remote-side", `origin/${branch}`); + commitFile(root, "remote.txt", message, message); + git(root, "push", "origin", `remote-side:${branch}`); + git(root, "checkout", current); + git(root, "branch", "-D", "remote-side"); +} + +/** Point refs/pull//head on the bare origin at the given commit. */ +function setPullHead(fixture: RepoFixture, prNumber: number, sha: string): void { + NodeChildProcess.execFileSync( + "git", + ["-C", fixture.remote, "update-ref", `refs/pull/${prNumber}/head`, sha], + { encoding: "utf8", timeout: 15_000 }, + ); +} + +describe("GitRepositoryService.fetchBranchAt (issue #1730)", () => { + it.each([undefined, 42])( + "preserves an existing local branch that is ahead of the fetched head (PR %s)", + async (prNumber) => { + const fixture = createRepo(); + const { root, service } = fixture; + git(root, "checkout", "-b", "feature"); + git(root, "push", "-u", "origin", "feature"); + commitFile(root, "local.txt", "unpushed work", "local unpushed work"); + const before = git(root, "rev-parse", "feature"); + git(root, "checkout", "main"); + if (prNumber !== undefined) { + setPullHead(fixture, prNumber, git(root, "rev-parse", "origin/feature")); + } + + await service.fetchBranchAt(root, "feature", prNumber); + + const after = git(root, "rev-parse", "feature"); + expect(after).toBe(before); + expect(git(root, "log", "feature", "--format=%s")).toContain("local unpushed work"); + }, + ); + + it.each([undefined, 42])( + "rejects a diverged local branch without altering it (PR %s)", + async (prNumber) => { + const fixture = createRepo(); + const { root, service } = fixture; + git(root, "checkout", "-b", "feature"); + commitFile(root, "feat.txt", "feat", "shared base commit"); + git(root, "push", "-u", "origin", "feature"); + commitFile(root, "local.txt", "divergent work", "local divergent work"); + const before = git(root, "rev-parse", "feature"); + git(root, "checkout", "main"); + pushRemoteOnlyCommit(fixture, "feature", "remote divergent work"); + if (prNumber !== undefined) { + setPullHead(fixture, prNumber, git(root, "rev-parse", "origin/feature")); + } + + await expect(service.fetchBranchAt(root, "feature", prNumber)).rejects.toThrow(); + + expect(git(root, "rev-parse", "feature")).toBe(before); + expect(git(root, "log", "feature", "--format=%s")).toContain("local divergent work"); + }, + ); + + it.each([undefined, 42])( + "fast-forwards a local branch that is behind the fetched head (PR %s)", + async (prNumber) => { + const fixture = createRepo(); + const { root, service } = fixture; + git(root, "checkout", "-b", "feature"); + commitFile(root, "feat.txt", "feat", "local pushed commit"); + git(root, "push", "-u", "origin", "feature"); + git(root, "checkout", "main"); + pushRemoteOnlyCommit(fixture, "feature", "remote ahead commit"); + const remoteHead = git(root, "rev-parse", "origin/feature"); + if (prNumber !== undefined) { + setPullHead(fixture, prNumber, remoteHead); + } + + await service.fetchBranchAt(root, "feature", prNumber); + + expect(git(root, "rev-parse", "feature")).toBe(remoteHead); + expect(git(root, "log", "feature", "--format=%s")).toContain("local pushed commit"); + }, + ); + + it.each([undefined, 42])( + "creates the local branch when it does not exist (PR %s)", + async (prNumber) => { + const fixture = createRepo(); + const { root, service } = fixture; + git(root, "checkout", "-b", "incoming"); + commitFile(root, "incoming.txt", "incoming", "incoming commit"); + git(root, "push", "origin", "incoming"); + git(root, "checkout", "main"); + git(root, "branch", "-D", "incoming"); + const remoteHead = git(root, "rev-parse", "origin/incoming"); + if (prNumber !== undefined) { + setPullHead(fixture, prNumber, remoteHead); + } + + await service.fetchBranchAt(root, "incoming", prNumber); + + expect(git(root, "rev-parse", "incoming")).toBe(remoteHead); + if (prNumber === undefined) { + expect(git(root, "rev-parse", "--abbrev-ref", "incoming@{upstream}")).toBe( + "origin/incoming", + ); + } + }, + ); + + it.each([undefined, 42])( + "leaves an up-to-date local branch untouched (PR %s)", + async (prNumber) => { + const fixture = createRepo(); + const { root, service } = fixture; + git(root, "checkout", "-b", "feature"); + commitFile(root, "feat.txt", "feat", "feature commit"); + git(root, "push", "-u", "origin", "feature"); + const before = git(root, "rev-parse", "feature"); + git(root, "checkout", "main"); + if (prNumber !== undefined) { + setPullHead(fixture, prNumber, before); + } + + await service.fetchBranchAt(root, "feature", prNumber); + + expect(git(root, "rev-parse", "feature")).toBe(before); + }, + ); + + it.each([undefined, 42])( + "does not move a branch checked out in a linked worktree (PR %s)", + async (prNumber) => { + const fixture = createRepo(); + const { root, service } = fixture; + git(root, "checkout", "-b", "feature"); + commitFile(root, "feat.txt", "feat", "feature commit"); + git(root, "push", "-u", "origin", "feature"); + git(root, "checkout", "main"); + pushRemoteOnlyCommit(fixture, "feature", "remote ahead commit"); + const before = git(root, "rev-parse", "feature"); + if (prNumber !== undefined) { + setPullHead(fixture, prNumber, git(root, "rev-parse", "origin/feature")); + } + const worktreeDir = NodeFS.mkdtempSync( + NodePath.join(NodeOS.tmpdir(), "mcode-fetch-wt-1730-"), + ); + tempDirs.push(worktreeDir); + git(root, "worktree", "add", NodePath.join(worktreeDir, "wt"), "feature"); + + await expect(service.fetchBranchAt(root, "feature", prNumber)).rejects.toThrow(); + + expect(git(root, "rev-parse", "feature")).toBe(before); + }, + ); + + it.each([undefined, 42])( + "resolves without moving an up-to-date branch checked out in a worktree (PR %s)", + async (prNumber) => { + const fixture = createRepo(); + const { root, service } = fixture; + git(root, "checkout", "-b", "feature"); + commitFile(root, "feat.txt", "feat", "feature commit"); + git(root, "push", "-u", "origin", "feature"); + const before = git(root, "rev-parse", "feature"); + git(root, "checkout", "main"); + if (prNumber !== undefined) { + setPullHead(fixture, prNumber, before); + } + const worktreeDir = NodeFS.mkdtempSync( + NodePath.join(NodeOS.tmpdir(), "mcode-fetch-wt-eq-1730-"), + ); + tempDirs.push(worktreeDir); + git(root, "worktree", "add", NodePath.join(worktreeDir, "wt"), "feature"); + + await service.fetchBranchAt(root, "feature", prNumber); + + expect(git(root, "rev-parse", "feature")).toBe(before); + }, + ); + + it("rejects a behind branch checked out in the current worktree", async () => { + const fixture = createRepo(); + const { root, service } = fixture; + git(root, "checkout", "-b", "feature"); + commitFile(root, "feat.txt", "feat", "feature commit"); + git(root, "push", "-u", "origin", "feature"); + pushRemoteOnlyCommit(fixture, "feature", "remote ahead commit"); + const before = git(root, "rev-parse", "feature"); + git(root, "checkout", "feature"); + + await expect(service.fetchBranchAt(root, "feature")).rejects.toThrow(); + + expect(git(root, "rev-parse", "feature")).toBe(before); + }); + + it("treats a tag-only name as an absent branch", async () => { + const { root, service } = createRepo(); + git(root, "checkout", "-b", "incoming"); + commitFile(root, "incoming.txt", "incoming", "incoming commit"); + git(root, "push", "origin", "incoming"); + git(root, "checkout", "main"); + git(root, "branch", "-D", "incoming"); + commitFile(root, "unrelated.txt", "unrelated", "unrelated tag target"); + git(root, "tag", "incoming"); + const remoteHead = git(root, "rev-parse", "origin/incoming"); + + await service.fetchBranchAt(root, "incoming"); + + expect(git(root, "rev-parse", "refs/heads/incoming")).toBe(remoteHead); + }); + + it("fetches the remote branch, not a same-named remote tag", async () => { + const fixture = createRepo(); + const { root, service } = fixture; + git(root, "checkout", "-b", "shadow"); + commitFile(root, "feat.txt", "feat", "local pushed commit"); + git(root, "push", "-u", "origin", "shadow"); + git(root, "checkout", "main"); + pushRemoteOnlyCommit(fixture, "shadow", "remote ahead commit"); + const remoteHead = git(root, "rev-parse", "origin/shadow"); + git(root, "checkout", "-b", "tag-side"); + commitFile(root, "tag.txt", "tag", "tag-only commit"); + git(root, "push", "origin", "tag-side:refs/tags/shadow"); + git(root, "checkout", "main"); + git(root, "branch", "-D", "tag-side"); + + await service.fetchBranchAt(root, "shadow"); + + expect(git(root, "rev-parse", "refs/heads/shadow")).toBe(remoteHead); + }); + + it("keeps an existing local branch when the ordinary fetch fails", async () => { + const { root, service } = createRepo(); + git(root, "checkout", "-b", "local-only"); + commitFile(root, "local.txt", "local only", "local only commit"); + const before = git(root, "rev-parse", "local-only"); + git(root, "checkout", "main"); + + await service.fetchBranchAt(root, "local-only"); + + expect(git(root, "rev-parse", "local-only")).toBe(before); + }); + + it("rejects when the branch exists neither locally nor on origin", async () => { + const { root, service } = createRepo(); + + await expect(service.fetchBranchAt(root, "missing")).rejects.toThrow( + 'Branch "missing" not found locally or on origin', + ); + await expect(service.fetchBranchAt(root, "missing", 99)).rejects.toThrow(); + }); +}); diff --git a/apps/server/src/features/projects/git/__tests__/git-service-push.test.ts b/apps/server/src/features/projects/git/__tests__/git-service-push.test.ts index f927a10f6..e62dc6772 100644 --- a/apps/server/src/features/projects/git/__tests__/git-service-push.test.ts +++ b/apps/server/src/features/projects/git/__tests__/git-service-push.test.ts @@ -90,7 +90,7 @@ describe("GitRepositoryService.fetchBranchAt", () => { "/repo", "fetch", "origin", - "+pull/42/head:contributor/review", + "pull/42/head", ]); }); }); diff --git a/apps/server/src/features/projects/git/git-repository-service.ts b/apps/server/src/features/projects/git/git-repository-service.ts index 77dbdd565..8cd2d5d19 100644 --- a/apps/server/src/features/projects/git/git-repository-service.ts +++ b/apps/server/src/features/projects/git/git-repository-service.ts @@ -221,36 +221,71 @@ export class GitRepositoryService { async fetchBranchAt(repoPath: string, branch: string, prNumber?: number): Promise { validateBranchName(branch); - if (prNumber != null) { - await this.gitExecutor.exec([ - "-C", - repoPath, - "fetch", - "origin", - `+pull/${prNumber}/head:${branch}`, - ]); - return; - } - + // Fetch into FETCH_HEAD only; the local branch is moved afterwards, and + // only when the move cannot drop local-only commits. + // Qualify the ordinary source: fetch ref lookup prefers refs/tags/ over + // refs/heads/, so a bare name could fetch a same-named remote tag. + const source = prNumber != null ? `pull/${prNumber}/head` : `refs/heads/${branch}`; let fetchOk = true; try { - await this.gitExecutor.exec(["-C", repoPath, "fetch", "origin", branch]); - } catch { + await this.gitExecutor.exec(["-C", repoPath, "fetch", "origin", source]); + } catch (error) { + if (prNumber != null) throw error; fetchOk = false; } - if (fetchOk) { - if (await this.branchExists(repoPath, branch)) { - await this.gitExecutor.exec( - ["-C", repoPath, "branch", "-f", branch, `origin/${branch}`], - ); + const localRef = `refs/heads/${branch}`; + if (!fetchOk) { + if (!(await this.branchExists(repoPath, localRef))) { + throw new Error(`Branch "${branch}" not found locally or on origin`); + } + return; + } + + if (!(await this.branchExists(repoPath, localRef))) { + if (prNumber != null) { + await this.gitExecutor.exec(["-C", repoPath, "branch", branch, "FETCH_HEAD"]); } else { await this.gitExecutor.exec( ["-C", repoPath, "branch", "--track", branch, `origin/${branch}`], ); } - } else if (!fetchOk && !(await this.branchExists(repoPath, branch))) { - throw new Error(`Branch "${branch}" not found locally or on origin`); + return; + } + + // Ahead or equal: the local tip already contains the fetched head. + if (await this.isAncestor(repoPath, "FETCH_HEAD", localRef)) return; + if (!(await this.isAncestor(repoPath, localRef, "FETCH_HEAD"))) { + throw new Error( + `Local branch "${branch}" has diverged from "${source}"; refusing to overwrite local-only commits`, + ); + } + + // Strictly behind: a non-forced refspec moves the branch atomically — + // it applies only a fast-forward and refuses refs checked out in any + // worktree, so a concurrent local commit rejects instead of being dropped. + await this.gitExecutor.exec([ + "-C", + repoPath, + "fetch", + "origin", + `${source}:${localRef}`, + ]); + } + + /** Check whether `ancestor` is an ancestor commit of `descendant`. */ + private async isAncestor( + repoPath: string, + ancestor: string, + descendant: string, + ): Promise { + try { + await this.gitExecutor.exec( + ["-C", repoPath, "merge-base", "--is-ancestor", ancestor, descendant], + ); + return true; + } catch { + return false; } } diff --git a/packages/contracts/src/ws/methods.ts b/packages/contracts/src/ws/methods.ts index e5e01473e..a989738aa 100644 --- a/packages/contracts/src/ws/methods.ts +++ b/packages/contracts/src/ws/methods.ts @@ -926,7 +926,7 @@ export const WS_METHODS = lazySchema(() => ({ params: z.object({ workspaceId: z.string(), branch: z.string(), - prNumber: z.number().optional(), + prNumber: z.number().int().positive().optional(), }), result: z.void(), },