diff --git a/apps/server/execution/orchestrator.test.ts b/apps/server/execution/orchestrator.test.ts index fb52e13..6cf293d 100644 --- a/apps/server/execution/orchestrator.test.ts +++ b/apps/server/execution/orchestrator.test.ts @@ -6,7 +6,12 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { ExecutionOrchestrator } from './orchestrator.js'; +import { ensureProjectClone } from '../git/project-clones.js'; import type { BranchManager } from '../git/branch-manager.js'; + +vi.mock('../git/project-clones.js', () => ({ + ensureProjectClone: vi.fn().mockResolvedValue('/tmp/test-workspace/clones/test'), +})); import type { PhaseRepository } from '../db/repositories/phase-repository.js'; import type { TaskRepository } from '../db/repositories/task-repository.js'; import type { InitiativeRepository } from '../db/repositories/initiative-repository.js'; @@ -39,7 +44,7 @@ function createMockEventBus(): EventBus & { handlers: Map; e function createMocks() { const branchManager: BranchManager = { ensureBranch: vi.fn(), - mergeBranch: vi.fn().mockResolvedValue({ success: true, message: 'merged' }), + mergeBranch: vi.fn().mockResolvedValue({ success: true, message: 'merged', previousRef: 'abc000' }), diffBranches: vi.fn().mockResolvedValue(''), deleteBranch: vi.fn(), branchExists: vi.fn().mockResolvedValue(true), @@ -51,6 +56,7 @@ function createMocks() { checkMergeability: vi.fn().mockResolvedValue({ mergeable: true }), fetchRemote: vi.fn(), fastForwardBranch: vi.fn(), + updateRef: vi.fn(), }; const phaseRepository = { @@ -306,4 +312,58 @@ describe('ExecutionOrchestrator', () => { expect(mocks.phaseDispatchManager.completePhase).not.toHaveBeenCalled(); }); }); + + describe('approveInitiative', () => { + function setupApproveTest(mocks: ReturnType) { + const initiative = { id: 'init-1', branch: 'cw/test', status: 'pending_review' }; + const project = { id: 'proj-1', name: 'test', url: 'https://example.com', defaultBranch: 'main' }; + vi.mocked(mocks.initiativeRepository.findById).mockResolvedValue(initiative as any); + vi.mocked(mocks.projectRepository.findProjectsByInitiativeId).mockResolvedValue([project] as any); + vi.mocked(mocks.branchManager.branchExists).mockResolvedValue(true); + vi.mocked(mocks.branchManager.mergeBranch).mockResolvedValue({ success: true, message: 'ok', previousRef: 'abc000' }); + return { initiative, project }; + } + + it('should roll back merge when push fails', async () => { + setupApproveTest(mocks); + vi.mocked(mocks.branchManager.pushBranch).mockRejectedValue(new Error('non-fast-forward')); + + const orchestrator = createOrchestrator(mocks); + + await expect(orchestrator.approveInitiative('init-1', 'merge_and_push')).rejects.toThrow('non-fast-forward'); + + // Should have rolled back the merge by restoring the previous ref + expect(mocks.branchManager.updateRef).toHaveBeenCalledWith( + expect.any(String), + 'main', + 'abc000', + ); + + // Should NOT have marked initiative as completed + expect(mocks.initiativeRepository.update).not.toHaveBeenCalled(); + }); + + it('should complete initiative when push succeeds', async () => { + setupApproveTest(mocks); + + const orchestrator = createOrchestrator(mocks); + + await orchestrator.approveInitiative('init-1', 'merge_and_push'); + + expect(mocks.branchManager.updateRef).not.toHaveBeenCalled(); + expect(mocks.initiativeRepository.update).toHaveBeenCalledWith('init-1', { status: 'completed' }); + }); + + it('should not attempt rollback for push_branch strategy', async () => { + setupApproveTest(mocks); + vi.mocked(mocks.branchManager.pushBranch).mockRejectedValue(new Error('auth failed')); + + const orchestrator = createOrchestrator(mocks); + + await expect(orchestrator.approveInitiative('init-1', 'push_branch')).rejects.toThrow('auth failed'); + + // No merge happened, so no rollback needed + expect(mocks.branchManager.updateRef).not.toHaveBeenCalled(); + }); + }); }); diff --git a/apps/server/execution/orchestrator.ts b/apps/server/execution/orchestrator.ts index 9dba50c..5b8c521 100644 --- a/apps/server/execution/orchestrator.ts +++ b/apps/server/execution/orchestrator.ts @@ -695,7 +695,18 @@ export class ExecutionOrchestrator { if (!result.success) { throw new Error(`Failed to merge ${initiative.branch} into ${project.defaultBranch} for project ${project.name}: ${result.message}`); } - await this.branchManager.pushBranch(clonePath, project.defaultBranch); + try { + await this.branchManager.pushBranch(clonePath, project.defaultBranch); + } catch (pushErr) { + // Roll back the merge so the diff doesn't disappear from the review tab. + // Without rollback, defaultBranch includes the initiative changes and the + // three-dot diff (defaultBranch...initiativeBranch) becomes empty. + if (result.previousRef) { + log.warn({ project: project.name, previousRef: result.previousRef }, 'push failed — rolling back merge'); + await this.branchManager.updateRef(clonePath, project.defaultBranch, result.previousRef); + } + throw pushErr; + } log.info({ initiativeId, project: project.name }, 'initiative branch merged into default and pushed'); } else { await this.branchManager.pushBranch(clonePath, initiative.branch); diff --git a/apps/server/git/branch-manager.ts b/apps/server/git/branch-manager.ts index ceb399c..9ba6d85 100644 --- a/apps/server/git/branch-manager.ts +++ b/apps/server/git/branch-manager.ts @@ -88,4 +88,10 @@ export interface BranchManager { * (i.e. the branches have diverged). */ fastForwardBranch(repoPath: string, branch: string, remote?: string): Promise; + + /** + * Force-update a branch ref to point at a specific commit. + * Used to roll back a merge when a subsequent push fails. + */ + updateRef(repoPath: string, branch: string, commitHash: string): Promise; } diff --git a/apps/server/git/simple-git-branch-manager.ts b/apps/server/git/simple-git-branch-manager.ts index e686a6f..b8147d0 100644 --- a/apps/server/git/simple-git-branch-manager.ts +++ b/apps/server/git/simple-git-branch-manager.ts @@ -39,6 +39,9 @@ export class SimpleGitBranchManager implements BranchManager { const tempBranch = `cw-merge-${Date.now()}`; try { + // Capture the target branch ref before merge so callers can roll back on push failure + const previousRef = (await repoGit.raw(['rev-parse', targetBranch])).trim(); + // Create worktree with a temp branch starting at targetBranch's commit await repoGit.raw(['worktree', 'add', '-b', tempBranch, tmpPath, targetBranch]); @@ -53,7 +56,7 @@ export class SimpleGitBranchManager implements BranchManager { await repoGit.raw(['update-ref', `refs/heads/${targetBranch}`, mergeCommit]); log.info({ repoPath, sourceBranch, targetBranch }, 'merge completed cleanly'); - return { success: true, message: `Merged ${sourceBranch} into ${targetBranch}` }; + return { success: true, message: `Merged ${sourceBranch} into ${targetBranch}`, previousRef }; } catch (mergeErr) { // Check for merge conflicts const status = await wtGit.status(); @@ -208,4 +211,10 @@ export class SimpleGitBranchManager implements BranchManager { await git.raw(['merge', '--ff-only', remoteBranch, branch]); log.info({ repoPath, branch, remoteBranch }, 'fast-forwarded branch'); } + + async updateRef(repoPath: string, branch: string, commitHash: string): Promise { + const git = simpleGit(repoPath); + await git.raw(['update-ref', `refs/heads/${branch}`, commitHash]); + log.info({ repoPath, branch, commitHash: commitHash.slice(0, 7) }, 'branch ref updated'); + } } diff --git a/apps/server/git/types.ts b/apps/server/git/types.ts index 8471b75..51a35b7 100644 --- a/apps/server/git/types.ts +++ b/apps/server/git/types.ts @@ -56,6 +56,8 @@ export interface MergeResult { conflicts?: string[]; /** Human-readable message describing the result */ message: string; + /** The target branch's commit hash before the merge (for rollback on push failure) */ + previousRef?: string; } // =============================================================================