diff --git a/packages/server/src/server/session.test.ts b/packages/server/src/server/session.test.ts index e3de3e24f..7a1410921 100644 --- a/packages/server/src/server/session.test.ts +++ b/packages/server/src/server/session.test.ts @@ -65,7 +65,6 @@ interface SessionHandlerInternals { describeWorkspaceRecord(...args: unknown[]): Promise; describeWorkspaceRecordWithGitData(...args: unknown[]): Promise; handleValidateBranchRequest(params: unknown): Promise; - createBranchFromBase(params: unknown): Promise; handleCheckoutSwitchBranchRequest(params: unknown): Promise; handleBranchSuggestionsRequest(params: unknown): Promise; handleStashListRequest(params: unknown): Promise; @@ -3253,101 +3252,6 @@ describe("session branch validation", () => { }); }); -describe("session branch creation handling", () => { - test("validates the base branch through the workspace git service", async () => { - const workspaceGitService = { - getSnapshot: vi.fn(), - validateBranchRef: vi.fn().mockResolvedValue({ kind: "not-found" }), - hasLocalBranch: vi.fn(), - }; - const session = createSessionForTest({ workspaceGitService }); - - await expect( - asSessionInternals(session).createBranchFromBase({ - cwd: "/tmp/repo", - baseBranch: "missing-base", - newBranchName: "feature/new-work", - }), - ).rejects.toThrow("Base branch not found: missing-base"); - - expect(workspaceGitService.validateBranchRef).toHaveBeenCalledTimes(1); - expect(workspaceGitService.validateBranchRef).toHaveBeenCalledWith("/tmp/repo", "missing-base"); - expect(workspaceGitService.hasLocalBranch).not.toHaveBeenCalled(); - expect(spawnMocks.execCommand).not.toHaveBeenCalledWith( - "git", - ["rev-parse", "--verify", "missing-base"], - { cwd: "/tmp/repo" }, - ); - }); - - test("checks local branch existence through the workspace git service", async () => { - const workspaceGitService = { - getSnapshot: vi.fn(), - validateBranchRef: vi.fn().mockResolvedValue({ kind: "local", name: "main" }), - hasLocalBranch: vi.fn().mockResolvedValue(true), - }; - const session = createSessionForTest({ workspaceGitService }); - - await expect( - asSessionInternals(session).createBranchFromBase({ - cwd: "/tmp/repo", - baseBranch: "main", - newBranchName: "feature/existing", - }), - ).rejects.toThrow("Branch already exists: feature/existing"); - - expect(workspaceGitService.validateBranchRef).toHaveBeenCalledWith("/tmp/repo", "main"); - expect(workspaceGitService.hasLocalBranch).toHaveBeenCalledTimes(1); - expect(workspaceGitService.hasLocalBranch).toHaveBeenCalledWith( - "/tmp/repo", - "feature/existing", - ); - expect(spawnMocks.execCommand).not.toHaveBeenCalledWith( - "git", - ["show-ref", "--verify", "--quiet", "refs/heads/feature/existing"], - { cwd: "/tmp/repo" }, - ); - }); - - test("forces a workspace git snapshot refresh after creating a branch", async () => { - const workspaceGitService = { - getSnapshot: vi.fn().mockResolvedValue( - createWorkspaceGitSnapshot("/tmp/repo", { - git: { - isDirty: false, - }, - }), - ), - validateBranchRef: vi.fn().mockResolvedValue({ kind: "local", name: "main" }), - hasLocalBranch: vi.fn().mockResolvedValue(false), - }; - const session = createSessionForTest({ workspaceGitService }); - spawnMocks.execCommand.mockResolvedValue({ - stdout: "", - stderr: "", - exitCode: 0, - signal: null, - truncated: false, - }); - - await asSessionInternals(session).createBranchFromBase({ - cwd: "/tmp/repo", - baseBranch: "main", - newBranchName: "feature/new-work", - }); - - expect(spawnMocks.execCommand).toHaveBeenCalledWith( - "git", - ["checkout", "-b", "feature/new-work", "main"], - { cwd: "/tmp/repo" }, - ); - expect(workspaceGitService.getSnapshot).toHaveBeenCalledWith("/tmp/repo", { - force: true, - reason: "create-branch", - }); - }); -}); - describe("session checkout switch branch handling", () => { test("forces a workspace git snapshot refresh after switching branches", async () => { const messages: unknown[] = []; diff --git a/packages/server/src/server/session.ts b/packages/server/src/server/session.ts index a9e5259b5..3a1dc8d6f 100644 --- a/packages/server/src/server/session.ts +++ b/packages/server/src/server/session.ts @@ -159,12 +159,11 @@ import { } from "./workspace-archive-service.js"; import { WorkspaceReconciliationService } from "./workspace-reconciliation-service.js"; import type { ServiceProxySubsystem } from "./service-proxy.js"; +import { renameCurrentBranch as renameCurrentBranchDefault } from "../utils/checkout-git.js"; import { - checkoutResolvedBranch, - type CheckoutExistingBranchResult, - type GitMutationRefreshReason, - renameCurrentBranch as renameCurrentBranchDefault, -} from "../utils/checkout-git.js"; + createGitMutationService, + type GitMutationService, +} from "./session/git-mutation/git-mutation-service.js"; import { expandTilde } from "../utils/path.js"; import { searchHomeDirectories, searchWorkspaceEntries } from "../utils/directory-suggestions.js"; import type { CheckoutDiffManager } from "./checkout-diff-manager.js"; @@ -174,7 +173,6 @@ import type pino from "pino"; import { FileBackedChatService } from "./chat/chat-service.js"; import { LoopService } from "./loop-service.js"; import { ScheduleService } from "./schedule/service.js"; -import { execCommand } from "../utils/spawn.js"; import { createGitHubService, type GitHubService } from "../services/github-service.js"; import type { ProviderUsageService } from "../services/quota-fetcher/service.js"; import { @@ -196,7 +194,6 @@ import { type GeneratedWorkspaceName, } from "./worktree-branch-name-generator.js"; import { - assertSafeGitRef as assertWorktreeSafeGitRef, buildAgentSessionConfig as buildWorktreeAgentSessionConfig, createPaseoWorktreeWorkflow as createWorktreeWorkflow, type CreatePaseoWorktreeSetupContinuationInput, @@ -566,6 +563,7 @@ export class Session { private readonly renameCurrentBranch: typeof renameCurrentBranchDefault; private readonly generateWorkspaceName: typeof generateBranchNameFromFirstAgentContext; private readonly workspaceGitService: WorkspaceGitService; + private readonly gitMutation: GitMutationService; private readonly daemonConfigStore: DaemonConfigStore; private readonly mcpBaseUrl: string | null; private readonly pushTokenStore: PushTokenStore; @@ -697,17 +695,20 @@ export class Session { this.renameCurrentBranch = renameCurrentBranch ?? renameCurrentBranchDefault; this.generateWorkspaceName = generateWorkspaceName ?? generateBranchNameFromFirstAgentContext; this.workspaceGitService = workspaceGitService; + this.gitMutation = createGitMutationService({ + workspaceGitService: this.workspaceGitService, + github: this.github, + logger: this.sessionLogger, + }); this.checkoutSession = new CheckoutSession({ host: { emit: (msg) => this.emit(msg), - notifyGitMutation: (cwd, reason, mutationOptions) => - this.notifyGitMutation(cwd, reason, mutationOptions), emitWorkspaceUpdateForCwd: (cwd) => this.emitWorkspaceUpdateForCwd(cwd), handleWorkspaceGitBranchSnapshot: (cwd, branchName) => this.handleWorkspaceGitBranchSnapshot(cwd, branchName), renameCurrentBranch: (cwd, branch) => this.renameCurrentBranch(cwd, branch), - checkoutExistingBranch: (cwd, branch) => this.checkoutExistingBranch(cwd, branch), }, + gitMutation: this.gitMutation, workspaceGitService: this.workspaceGitService, github: this.github, checkoutDiffManager, @@ -3074,8 +3075,9 @@ export class Session { logger: this.sessionLogger, }, }), - checkoutExistingBranch: (cwd, branch) => this.checkoutExistingBranch(cwd, branch), - createBranchFromBase: (params) => this.createBranchFromBase(params), + checkoutExistingBranch: (cwd, branch) => + this.gitMutation.checkoutExistingBranch(cwd, branch), + createBranchFromBase: (params) => this.gitMutation.createBranchFromBase(params), github: this.github, }, config, @@ -3134,7 +3136,7 @@ export class Session { branch: result.branchName, promptTitle: resolveFirstAgentPromptTitle(input.firstAgentContext), }); - await this.notifyGitMutation(input.workspace.cwd, "rename-branch"); + await this.gitMutation.notifyGitMutation(input.workspace.cwd, "rename-branch"); await this.emitWorkspaceUpdateForCwd(input.workspace.cwd); } @@ -3237,13 +3239,6 @@ export class Session { ); } - private assertSafeGitRef(ref: string, label: string): void { - if (!/^[A-Za-z0-9._/-]+$/.test(ref)) { - throw new Error(`Invalid ${label}: ${ref}`); - } - assertWorktreeSafeGitRef(ref, label); - } - private isPathWithinRoot(rootPath: string, candidatePath: string): boolean { const resolvedRoot = resolve(rootPath); const resolvedCandidate = resolve(candidatePath); @@ -3253,93 +3248,6 @@ export class Session { return resolvedCandidate.startsWith(resolvedRoot + sep); } - private async ensureCleanWorkingTree(cwd: string): Promise { - const dirty = await this.isWorkingTreeDirty(cwd); - if (dirty) { - throw new Error( - "Working directory has uncommitted changes. Commit or stash before switching branches.", - ); - } - } - - private async isWorkingTreeDirty(cwd: string): Promise { - try { - const snapshot = await this.workspaceGitService.getSnapshot(cwd); - return snapshot.git.isDirty === true; - } catch (error) { - throw new Error(`Unable to inspect git status for ${cwd}: ${getErrorMessage(error)}`, { - cause: error, - }); - } - } - - private async checkoutExistingBranch( - cwd: string, - branch: string, - ): Promise { - this.assertSafeGitRef(branch, "branch"); - const resolution = await this.workspaceGitService.validateBranchRef(cwd, branch); - if (resolution.kind === "not-found") { - throw new Error(`Branch not found: ${branch}`); - } - await this.ensureCleanWorkingTree(cwd); - const result = await checkoutResolvedBranch({ - cwd, - resolution, - }); - await this.notifyGitMutation(cwd, "switch-branch", { invalidateGithub: true }); - return result; - } - - private async createBranchFromBase(params: { - cwd: string; - baseBranch: string; - newBranchName: string; - }): Promise { - const { cwd, baseBranch, newBranchName } = params; - this.assertSafeGitRef(baseBranch, "base branch"); - this.assertSafeGitRef(newBranchName, "new branch"); - - const baseResolution = await this.workspaceGitService.validateBranchRef(cwd, baseBranch); - if (baseResolution.kind === "not-found") { - throw new Error(`Base branch not found: ${baseBranch}`); - } - - const exists = await this.doesLocalBranchExist(cwd, newBranchName); - if (exists) { - throw new Error(`Branch already exists: ${newBranchName}`); - } - - await this.ensureCleanWorkingTree(cwd); - await execCommand("git", ["checkout", "-b", newBranchName, baseBranch], { - cwd, - }); - await this.notifyGitMutation(cwd, "create-branch"); - } - - private async doesLocalBranchExist(cwd: string, branch: string): Promise { - this.assertSafeGitRef(branch, "branch"); - return this.workspaceGitService.hasLocalBranch(cwd, branch); - } - - private async notifyGitMutation( - cwd: string, - reason: GitMutationRefreshReason, - options?: { invalidateGithub?: boolean }, - ): Promise { - if (options?.invalidateGithub) { - this.github.invalidate({ cwd }); - } - try { - await this.workspaceGitService.getSnapshot(cwd, { force: true, reason }); - } catch (error) { - this.sessionLogger.warn( - { err: error, cwd, reason }, - "Failed to force-refresh workspace git snapshot after mutation", - ); - } - } - /** * Handle clearing agent attention flag */ @@ -4715,8 +4623,8 @@ export class Session { workspaceGitService: this.workspaceGitService, }); void Promise.all([ - this.notifyGitMutation(input.cwd, "create-worktree"), - this.notifyGitMutation(result.worktree.worktreePath, "create-worktree"), + this.gitMutation.notifyGitMutation(input.cwd, "create-worktree"), + this.gitMutation.notifyGitMutation(result.worktree.worktreePath, "create-worktree"), ]).catch((error) => { this.sessionLogger.warn( { err: error, cwd: input.cwd, worktreePath: result.worktree.worktreePath }, diff --git a/packages/server/src/server/session/checkout/checkout-session.test.ts b/packages/server/src/server/session/checkout/checkout-session.test.ts index daece9f56..880d19821 100644 --- a/packages/server/src/server/session/checkout/checkout-session.test.ts +++ b/packages/server/src/server/session/checkout/checkout-session.test.ts @@ -5,6 +5,7 @@ import { CheckoutSession, type CheckoutSessionHost, } from "./checkout-session.js"; +import type { GitMutationService } from "../git-mutation/git-mutation-service.js"; import { createGitHubService, type GitHubService } from "../../../services/github-service.js"; import type { SessionOutboundMessage } from "../../messages.js"; import type { @@ -56,14 +57,19 @@ function createFakeDiffSubscriber(initial: CheckoutDiffSnapshotPayload) { } interface RecordedHostCalls { + emitWorkspaceUpdateForCwd: string[]; + handleWorkspaceGitBranchSnapshot: Array<{ cwd: string; branchName: string | null }>; + renameCurrentBranch: Array<{ cwd: string; branch: string }>; +} + +type GitMutationFake = Pick; + +interface RecordedGitMutationCalls { notifyGitMutation: Array<{ cwd: string; reason: string; options?: { invalidateGithub?: boolean }; }>; - emitWorkspaceUpdateForCwd: string[]; - handleWorkspaceGitBranchSnapshot: Array<{ cwd: string; branchName: string | null }>; - renameCurrentBranch: Array<{ cwd: string; branch: string }>; checkoutExistingBranch: Array<{ cwd: string; branch: string }>; } @@ -77,14 +83,17 @@ function makeCheckoutSession(options?: { diff?: CheckoutDiffSubscriber; github?: Partial; host?: Partial; + gitMutation?: Partial; gitMetadataGenerator?: Partial; }) { const emitted: SessionOutboundMessage[] = []; const hostCalls: RecordedHostCalls = { - notifyGitMutation: [], emitWorkspaceUpdateForCwd: [], handleWorkspaceGitBranchSnapshot: [], renameCurrentBranch: [], + }; + const gitMutationCalls: RecordedGitMutationCalls = { + notifyGitMutation: [], checkoutExistingBranch: [], }; const generatorCalls: RecordedGeneratorCalls = { @@ -93,9 +102,6 @@ function makeCheckoutSession(options?: { }; const host: CheckoutSessionHost = { emit: (msg) => emitted.push(msg), - notifyGitMutation: async (cwd, reason, opts) => { - hostCalls.notifyGitMutation.push({ cwd, reason, options: opts }); - }, emitWorkspaceUpdateForCwd: async (cwd) => { hostCalls.emitWorkspaceUpdateForCwd.push(cwd); }, @@ -106,11 +112,17 @@ function makeCheckoutSession(options?: { hostCalls.renameCurrentBranch.push({ cwd, branch }); return { previousBranch: null, currentBranch: branch }; }, + ...options?.host, + }; + const gitMutation: GitMutationFake = { + notifyGitMutation: async (cwd, reason, opts) => { + gitMutationCalls.notifyGitMutation.push({ cwd, reason, options: opts }); + }, checkoutExistingBranch: async (cwd, branch) => { - hostCalls.checkoutExistingBranch.push({ cwd, branch }); + gitMutationCalls.checkoutExistingBranch.push({ cwd, branch }); return { source: "local" }; }, - ...options?.host, + ...options?.gitMutation, }; const gitMetadataGenerator: GitMetadataGenerator = { generateCommitMessage: async (cwd) => { @@ -126,6 +138,7 @@ function makeCheckoutSession(options?: { const github: GitHubService = { ...createGitHubService(), ...options?.github }; const checkout = new CheckoutSession({ host, + gitMutation, workspaceGitService: createNoopWorkspaceGitService(options?.git), github, checkoutDiffManager: @@ -135,7 +148,7 @@ function makeCheckoutSession(options?: { worktreesRoot: undefined, logger: pino({ level: "silent" }), }); - return { checkout, emitted, hostCalls, generatorCalls }; + return { checkout, emitted, hostCalls, gitMutationCalls, generatorCalls }; } function createGitSnapshot( @@ -534,7 +547,9 @@ describe("CheckoutSession", () => { files: [], error: null, }); - const { checkout, emitted, hostCalls } = makeCheckoutSession({ diff: subscriber }); + const { checkout, emitted, hostCalls, gitMutationCalls } = makeCheckoutSession({ + diff: subscriber, + }); await checkout.handleCheckoutSwitchBranchRequest({ type: "checkout_switch_branch_request", @@ -543,7 +558,9 @@ describe("CheckoutSession", () => { requestId: "sw1", }); - expect(hostCalls.checkoutExistingBranch).toEqual([{ cwd: "/repo", branch: "feature" }]); + expect(gitMutationCalls.checkoutExistingBranch).toEqual([ + { cwd: "/repo", branch: "feature" }, + ]); expect(refreshedCwds).toEqual(["/repo"]); expect(hostCalls.emitWorkspaceUpdateForCwd).toEqual(["/repo"]); expect(emitted).toEqual([ @@ -563,7 +580,7 @@ describe("CheckoutSession", () => { it("emits an error response when the checkout fails", async () => { const { checkout, emitted } = makeCheckoutSession({ - host: { + gitMutation: { checkoutExistingBranch: async () => { throw new Error("branch missing"); }, @@ -617,7 +634,9 @@ describe("CheckoutSession", () => { files: [], error: null, }); - const { checkout, emitted, hostCalls } = makeCheckoutSession({ diff: subscriber }); + const { checkout, emitted, hostCalls, gitMutationCalls } = makeCheckoutSession({ + diff: subscriber, + }); await checkout.handleCheckoutRenameBranchRequest({ type: "checkout.rename_branch.request", @@ -627,7 +646,7 @@ describe("CheckoutSession", () => { }); expect(hostCalls.renameCurrentBranch).toEqual([{ cwd: "/repo", branch: "feature-renamed" }]); - expect(hostCalls.notifyGitMutation).toEqual([ + expect(gitMutationCalls.notifyGitMutation).toEqual([ { cwd: "/repo", reason: "rename-branch", options: { invalidateGithub: true } }, ]); expect(refreshedCwds).toEqual(["/repo"]); diff --git a/packages/server/src/server/session/checkout/checkout-session.ts b/packages/server/src/server/session/checkout/checkout-session.ts index a54e9bbf1..6c39014b2 100644 --- a/packages/server/src/server/session/checkout/checkout-session.ts +++ b/packages/server/src/server/session/checkout/checkout-session.ts @@ -27,6 +27,7 @@ import type { WorkspaceGitSnapshotOptions, } from "../../workspace-git-service.js"; import { assertSafeGitRef } from "../../worktree-session.js"; +import type { GitMutationService } from "../git-mutation/git-mutation-service.js"; import { assertPullRequestAutoMergeDisableReady, assertPullRequestAutoMergeEnableReady, @@ -34,10 +35,8 @@ import { type PullRequestTimelineItem, } from "../../../services/github-service.js"; import { - type CheckoutExistingBranchResult, commitChanges, createPullRequest, - type GitMutationRefreshReason, mergeFromBase, mergeToBase, pullCurrentBranch, @@ -49,24 +48,21 @@ import type { GitMetadataGenerator } from "./git-metadata-generator.js"; /** * The collaborators a checkout command reaches that are NOT part of the checkout - * domain: the git-mutation refresh primitive and workspace-update emitters owned - * by the Session shell (also used by worktree/workspace creation), and the injected - * branch operations. CheckoutSession orchestrates them but does not own them. + * domain and stay owned by the Session shell: client emit, workspace-update + * emission, the git branch-snapshot notifier, and the current-branch rename + * primitive. CheckoutSession orchestrates them but does not own them. The + * git-mutation primitives it performs (switch branch, force snapshot refresh) are + * injected separately as `gitMutation`, since they are shared with worktree and + * workspace creation. */ export interface CheckoutSessionHost { emit(msg: SessionOutboundMessage): void; - notifyGitMutation( - cwd: string, - reason: GitMutationRefreshReason, - options?: { invalidateGithub?: boolean }, - ): Promise; emitWorkspaceUpdateForCwd(cwd: string): Promise; handleWorkspaceGitBranchSnapshot(cwd: string, branchName: string | null): void; renameCurrentBranch( cwd: string, branch: string, ): Promise<{ previousBranch: string | null; currentBranch: string | null }>; - checkoutExistingBranch(cwd: string, branch: string): Promise; } type CurrentWorkspacePullRequest = NonNullable< @@ -90,6 +86,7 @@ export interface CheckoutDiffSubscriber { export interface CheckoutSessionOptions { host: CheckoutSessionHost; + gitMutation: Pick; workspaceGitService: WorkspaceGitService; github: GitHubService; checkoutDiffManager: CheckoutDiffSubscriber; @@ -106,13 +103,17 @@ export interface CheckoutSessionOptions { * merge/pull/push/stash and the GitHub-PR operations). * * Command operations keep the live diff in sync by calling scheduleDiffRefresh() - * and refresh the workspace git snapshot through host.notifyGitMutation(); the + * and refresh the workspace git snapshot through gitMutation.notifyGitMutation(); the * workspace git observer streams branch changes through emitStatusUpdate(). */ export class CheckoutSession { private static readonly PASEO_STASH_PREFIX = "paseo-auto-stash:"; private readonly host: CheckoutSessionHost; + private readonly gitMutation: Pick< + GitMutationService, + "checkoutExistingBranch" | "notifyGitMutation" + >; private readonly workspaceGitService: WorkspaceGitService; private readonly github: GitHubService; private readonly checkoutDiffManager: CheckoutDiffSubscriber; @@ -124,6 +125,7 @@ export class CheckoutSession { constructor(options: CheckoutSessionOptions) { this.host = options.host; + this.gitMutation = options.gitMutation; this.workspaceGitService = options.workspaceGitService; this.github = options.github; this.checkoutDiffManager = options.checkoutDiffManager; @@ -373,7 +375,7 @@ export class CheckoutSession { const { cwd, branch, requestId } = msg; try { - const checkoutResult = await this.host.checkoutExistingBranch(cwd, branch); + const checkoutResult = await this.gitMutation.checkoutExistingBranch(cwd, branch); this.scheduleDiffRefresh(cwd); // Push a workspace_update immediately so the sidebar/header reflect @@ -425,7 +427,7 @@ export class CheckoutSession { try { const result = await this.host.renameCurrentBranch(cwd, branch); - await this.host.notifyGitMutation(cwd, "rename-branch", { invalidateGithub: true }); + await this.gitMutation.notifyGitMutation(cwd, "rename-branch", { invalidateGithub: true }); this.scheduleDiffRefresh(cwd); this.host.handleWorkspaceGitBranchSnapshot(cwd, result.currentBranch); @@ -474,7 +476,7 @@ export class CheckoutSession { await execCommand("git", ["stash", "push", "--include-untracked", "-m", message], { cwd, }); - await this.host.notifyGitMutation(cwd, "stash-push"); + await this.gitMutation.notifyGitMutation(cwd, "stash-push"); this.scheduleDiffRefresh(cwd); this.host.emit({ type: "stash_save_response", @@ -496,7 +498,7 @@ export class CheckoutSession { await execCommand("git", ["stash", "pop", `stash@{${stashIndex}}`], { cwd, }); - await this.host.notifyGitMutation(cwd, "stash-pop"); + await this.gitMutation.notifyGitMutation(cwd, "stash-pop"); this.scheduleDiffRefresh(cwd); this.host.emit({ type: "stash_pop_response", @@ -548,7 +550,7 @@ export class CheckoutSession { message, addAll: msg.addAll ?? true, }); - await this.host.notifyGitMutation(cwd, "commit-changes"); + await this.gitMutation.notifyGitMutation(cwd, "commit-changes"); this.scheduleDiffRefresh(cwd); this.host.emit({ @@ -607,8 +609,8 @@ export class CheckoutSession { { paseoHome: this.paseoHome, worktreesRoot: this.worktreesRoot }, ); await Promise.all([ - this.host.notifyGitMutation(mutatedCwd, "merge-to-base", { invalidateGithub: true }), - ...(mutatedCwd !== cwd ? [this.host.notifyGitMutation(cwd, "merge-to-base")] : []), + this.gitMutation.notifyGitMutation(mutatedCwd, "merge-to-base", { invalidateGithub: true }), + ...(mutatedCwd !== cwd ? [this.gitMutation.notifyGitMutation(cwd, "merge-to-base")] : []), ]); this.scheduleDiffRefresh(cwd); @@ -651,7 +653,7 @@ export class CheckoutSession { baseRef: msg.baseRef, requireCleanTarget: msg.requireCleanTarget ?? true, }); - await this.host.notifyGitMutation(cwd, "merge-from-base", { invalidateGithub: true }); + await this.gitMutation.notifyGitMutation(cwd, "merge-from-base", { invalidateGithub: true }); this.scheduleDiffRefresh(cwd); this.host.emit({ @@ -683,7 +685,7 @@ export class CheckoutSession { try { await pullCurrentBranch(cwd); - await this.host.notifyGitMutation(cwd, "pull", { invalidateGithub: true }); + await this.gitMutation.notifyGitMutation(cwd, "pull", { invalidateGithub: true }); this.scheduleDiffRefresh(cwd); this.host.emit({ @@ -715,7 +717,7 @@ export class CheckoutSession { try { await pushCurrentBranch(cwd); - await this.host.notifyGitMutation(cwd, "push", { invalidateGithub: true }); + await this.gitMutation.notifyGitMutation(cwd, "push", { invalidateGithub: true }); this.host.emit({ type: "checkout_push_response", payload: { @@ -762,7 +764,7 @@ export class CheckoutSession { }, this.github, ); - await this.host.notifyGitMutation(cwd, "create-pr", { invalidateGithub: true }); + await this.gitMutation.notifyGitMutation(cwd, "create-pr", { invalidateGithub: true }); this.host.emit({ type: "checkout_pr_create_response", @@ -806,7 +808,7 @@ export class CheckoutSession { mergeMethod: msg.mergeMethod, status: pullRequest, }); - await this.host.notifyGitMutation(cwd, "merge-pr", { invalidateGithub: true }); + await this.gitMutation.notifyGitMutation(cwd, "merge-pr", { invalidateGithub: true }); this.host.emit({ type: "checkout_pr_merge_response", @@ -875,7 +877,7 @@ export class CheckoutSession { status: pullRequest, }); } - await this.host.notifyGitMutation( + await this.gitMutation.notifyGitMutation( cwd, msg.enabled ? "enable-pr-auto-merge" : "disable-pr-auto-merge", { diff --git a/packages/server/src/server/session/git-mutation/git-mutation-service.test.ts b/packages/server/src/server/session/git-mutation/git-mutation-service.test.ts new file mode 100644 index 000000000..a63986f64 --- /dev/null +++ b/packages/server/src/server/session/git-mutation/git-mutation-service.test.ts @@ -0,0 +1,218 @@ +import { execFileSync } from "node:child_process"; +import { mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { pino } from "pino"; +import { afterEach, describe, expect, test } from "vitest"; +import type { GitHubService } from "../../../services/github-service.js"; +import type { + WorkspaceGitBranchValidationResult, + WorkspaceGitRuntimeSnapshot, + WorkspaceGitService, +} from "../../workspace-git-service.js"; +import { createGitMutationService } from "./git-mutation-service.js"; + +// The production module reads only WorkspaceGitService.{validateBranchRef,getSnapshot,hasLocalBranch} +// and GitHubService.invalidate. The fakes below implement exactly that slice as in-memory +// adapters; the happy-path tests cross the real git boundary against a temp repo, since that is +// where checkoutResolvedBranch / `git checkout -b` actually run. + +type GitSource = Pick; + +const logger = pino({ level: "silent" }); + +interface FakeGitOptions { + resolution?: WorkspaceGitBranchValidationResult; + isDirty?: boolean | null; + branchExists?: boolean; + getSnapshotThrows?: boolean; +} + +function createFakeGit(opts: FakeGitOptions = {}) { + const resolution = opts.resolution ?? { kind: "local", name: "main" }; + const isDirty = opts.isDirty ?? false; + const branchExists = opts.branchExists ?? false; + const snapshotCalls: Array<{ cwd: string; force: boolean; reason?: string }> = []; + const git: GitSource = { + async validateBranchRef() { + return resolution; + }, + async getSnapshot(cwd, options) { + snapshotCalls.push({ cwd, force: options?.force === true, reason: options?.reason }); + if (opts.getSnapshotThrows) { + throw new Error("snapshot boom"); + } + return { git: { isDirty } } as unknown as WorkspaceGitRuntimeSnapshot; + }, + async hasLocalBranch() { + return branchExists; + }, + }; + return { git, snapshotCalls }; +} + +function createFakeGithub() { + const invalidateCalls: Array<{ cwd: string }> = []; + const github: Pick = { + invalidate(options) { + invalidateCalls.push(options); + }, + }; + return { github, invalidateCalls }; +} + +function buildService(gitOptions: FakeGitOptions = {}) { + const { git, snapshotCalls } = createFakeGit(gitOptions); + const { github, invalidateCalls } = createFakeGithub(); + const service = createGitMutationService({ workspaceGitService: git, github, logger }); + return { service, snapshotCalls, invalidateCalls }; +} + +const tempRepos: string[] = []; + +function initRepo(extraBranch?: string): string { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "git-mutation-"))); + tempRepos.push(dir); + const run = (...args: string[]) => execFileSync("git", args, { cwd: dir, stdio: "pipe" }); + run("init", "-b", "main"); + run("config", "user.email", "test@example.com"); + run("config", "user.name", "Paseo Test"); + writeFileSync(join(dir, "README.md"), "hello\n"); + run("add", "-A"); + run("commit", "-m", "init"); + if (extraBranch) { + run("branch", extraBranch); + } + return dir; +} + +function headBranch(dir: string): string { + return execFileSync("git", ["rev-parse", "--abbrev-ref", "HEAD"], { cwd: dir }).toString().trim(); +} + +afterEach(() => { + while (tempRepos.length > 0) { + const dir = tempRepos.pop(); + if (dir) { + rmSync(dir, { recursive: true, force: true }); + } + } +}); + +describe("checkoutExistingBranch", () => { + test("rejects an unsafe branch ref before touching git", async () => { + const { service } = buildService(); + await expect(service.checkoutExistingBranch("/tmp/nope", "bad branch")).rejects.toThrow( + /Invalid branch/, + ); + }); + + test("rejects when the branch does not resolve", async () => { + const { service } = buildService({ resolution: { kind: "not-found" } }); + await expect(service.checkoutExistingBranch("/tmp/nope", "missing")).rejects.toThrow( + /Branch not found: missing/, + ); + }); + + test("rejects when the working tree is dirty", async () => { + const { service } = buildService({ resolution: { kind: "local", name: "x" }, isDirty: true }); + await expect(service.checkoutExistingBranch("/tmp/nope", "x")).rejects.toThrow( + /uncommitted changes/, + ); + }); + + test("wraps a git-status failure with the inspecting-status message", async () => { + const { service } = buildService({ + resolution: { kind: "local", name: "x" }, + getSnapshotThrows: true, + }); + await expect(service.checkoutExistingBranch("/tmp/nope", "x")).rejects.toThrow( + /Unable to inspect git status/, + ); + }); + + test("checks out the branch and invalidates github (real repo)", async () => { + const dir = initRepo("feature"); + const { service, snapshotCalls, invalidateCalls } = buildService({ + resolution: { kind: "local", name: "feature" }, + }); + const result = await service.checkoutExistingBranch(dir, "feature"); + expect(result.source).toBe("local"); + expect(headBranch(dir)).toBe("feature"); + expect(invalidateCalls).toEqual([{ cwd: dir }]); + expect(snapshotCalls).toContainEqual({ cwd: dir, force: true, reason: "switch-branch" }); + }); +}); + +describe("createBranchFromBase", () => { + test("rejects an unsafe new-branch ref before touching git", async () => { + const { service } = buildService({ resolution: { kind: "local", name: "main" } }); + await expect( + service.createBranchFromBase({ + cwd: "/tmp/nope", + baseBranch: "main", + newBranchName: "bad x", + }), + ).rejects.toThrow(/Invalid new/); + }); + + test("rejects when the base branch does not resolve", async () => { + const { service } = buildService({ resolution: { kind: "not-found" } }); + await expect( + service.createBranchFromBase({ cwd: "/tmp/nope", baseBranch: "main", newBranchName: "feat" }), + ).rejects.toThrow(/Base branch not found: main/); + }); + + test("rejects when the new branch already exists", async () => { + const { service } = buildService({ + resolution: { kind: "local", name: "main" }, + branchExists: true, + }); + await expect( + service.createBranchFromBase({ cwd: "/tmp/nope", baseBranch: "main", newBranchName: "feat" }), + ).rejects.toThrow(/Branch already exists: feat/); + }); + + test("rejects when the working tree is dirty", async () => { + const { service } = buildService({ + resolution: { kind: "local", name: "main" }, + branchExists: false, + isDirty: true, + }); + await expect( + service.createBranchFromBase({ cwd: "/tmp/nope", baseBranch: "main", newBranchName: "feat" }), + ).rejects.toThrow(/uncommitted changes/); + }); + + test("creates the branch from base and refreshes without github invalidation (real repo)", async () => { + const dir = initRepo(); + const { service, snapshotCalls, invalidateCalls } = buildService({ + resolution: { kind: "local", name: "main" }, + }); + await service.createBranchFromBase({ cwd: dir, baseBranch: "main", newBranchName: "feature2" }); + expect(headBranch(dir)).toBe("feature2"); + expect(invalidateCalls).toEqual([]); + expect(snapshotCalls).toContainEqual({ cwd: dir, force: true, reason: "create-branch" }); + }); +}); + +describe("notifyGitMutation", () => { + test("invalidates github and force-refreshes when invalidateGithub is set", async () => { + const { service, snapshotCalls, invalidateCalls } = buildService(); + await service.notifyGitMutation("/tmp/repo", "commit-changes", { invalidateGithub: true }); + expect(invalidateCalls).toEqual([{ cwd: "/tmp/repo" }]); + expect(snapshotCalls).toEqual([{ cwd: "/tmp/repo", force: true, reason: "commit-changes" }]); + }); + + test("force-refreshes without invalidating github by default", async () => { + const { service, snapshotCalls, invalidateCalls } = buildService(); + await service.notifyGitMutation("/tmp/repo", "pull"); + expect(invalidateCalls).toEqual([]); + expect(snapshotCalls).toEqual([{ cwd: "/tmp/repo", force: true, reason: "pull" }]); + }); + + test("swallows a snapshot-refresh failure", async () => { + const { service } = buildService({ getSnapshotThrows: true }); + await expect(service.notifyGitMutation("/tmp/repo", "pull")).resolves.toBeUndefined(); + }); +}); diff --git a/packages/server/src/server/session/git-mutation/git-mutation-service.ts b/packages/server/src/server/session/git-mutation/git-mutation-service.ts new file mode 100644 index 000000000..f4cab9b24 --- /dev/null +++ b/packages/server/src/server/session/git-mutation/git-mutation-service.ts @@ -0,0 +1,129 @@ +import type pino from "pino"; +import { getErrorMessage } from "@getpaseo/protocol/error-utils"; +import type { GitHubService } from "../../../services/github-service.js"; +import { + checkoutResolvedBranch, + type CheckoutExistingBranchResult, + type GitMutationRefreshReason, +} from "../../../utils/checkout-git.js"; +import { execCommand } from "../../../utils/spawn.js"; +import type { WorkspaceGitService } from "../../workspace-git-service.js"; +import { assertSafeGitRef as assertWorktreeSafeGitRef } from "../../worktree-session.js"; + +/** + * The git branch / working-tree mutation primitives a client session performs on a + * workspace: switch to an existing branch, create a branch from a base, and force a + * snapshot refresh (plus optional GitHub cache invalidation) after any mutation. + * + * CheckoutSession (the branch/commit/merge commands), the worktree session-config + * builder, and the auto-naming + worktree-creation paths all funnel their git + * mutations through this one module, so the validate-ref → clean-tree → execute → + * refresh sequence lives in a single place instead of being smeared across the + * session as loose callbacks. + */ +export interface GitMutationService { + checkoutExistingBranch(cwd: string, branch: string): Promise; + createBranchFromBase(params: { + cwd: string; + baseBranch: string; + newBranchName: string; + }): Promise; + notifyGitMutation( + cwd: string, + reason: GitMutationRefreshReason, + options?: { invalidateGithub?: boolean }, + ): Promise; +} + +type GitMutationGitSource = Pick< + WorkspaceGitService, + "validateBranchRef" | "getSnapshot" | "hasLocalBranch" +>; + +export function createGitMutationService(deps: { + workspaceGitService: GitMutationGitSource; + github: Pick; + logger: pino.Logger; +}): GitMutationService { + const { workspaceGitService, github, logger } = deps; + + function assertSafeGitRef(ref: string, label: string): void { + if (!/^[A-Za-z0-9._/-]+$/.test(ref)) { + throw new Error(`Invalid ${label}: ${ref}`); + } + assertWorktreeSafeGitRef(ref, label); + } + + async function isWorkingTreeDirty(cwd: string): Promise { + try { + const snapshot = await workspaceGitService.getSnapshot(cwd); + return snapshot.git.isDirty === true; + } catch (error) { + throw new Error(`Unable to inspect git status for ${cwd}: ${getErrorMessage(error)}`, { + cause: error, + }); + } + } + + async function ensureCleanWorkingTree(cwd: string): Promise { + const dirty = await isWorkingTreeDirty(cwd); + if (dirty) { + throw new Error( + "Working directory has uncommitted changes. Commit or stash before switching branches.", + ); + } + } + + async function notifyGitMutation( + cwd: string, + reason: GitMutationRefreshReason, + options?: { invalidateGithub?: boolean }, + ): Promise { + if (options?.invalidateGithub) { + github.invalidate({ cwd }); + } + try { + await workspaceGitService.getSnapshot(cwd, { force: true, reason }); + } catch (error) { + logger.warn( + { err: error, cwd, reason }, + "Failed to force-refresh workspace git snapshot after mutation", + ); + } + } + + return { + async checkoutExistingBranch(cwd, branch) { + assertSafeGitRef(branch, "branch"); + const resolution = await workspaceGitService.validateBranchRef(cwd, branch); + if (resolution.kind === "not-found") { + throw new Error(`Branch not found: ${branch}`); + } + await ensureCleanWorkingTree(cwd); + const result = await checkoutResolvedBranch({ cwd, resolution }); + await notifyGitMutation(cwd, "switch-branch", { invalidateGithub: true }); + return result; + }, + + async createBranchFromBase({ cwd, baseBranch, newBranchName }) { + assertSafeGitRef(baseBranch, "base branch"); + assertSafeGitRef(newBranchName, "new branch"); + + const baseResolution = await workspaceGitService.validateBranchRef(cwd, baseBranch); + if (baseResolution.kind === "not-found") { + throw new Error(`Base branch not found: ${baseBranch}`); + } + + const exists = await workspaceGitService.hasLocalBranch(cwd, newBranchName); + if (exists) { + throw new Error(`Branch already exists: ${newBranchName}`); + } + + await ensureCleanWorkingTree(cwd); + await execCommand("git", ["checkout", "-b", newBranchName, baseBranch], { cwd }); + await notifyGitMutation(cwd, "create-branch"); + }, + + notifyGitMutation, + }; +}